refactor(websearch-settings): remove local providers and use v2 preferences#14443
refactor(websearch-settings): remove local providers and use v2 preferences#14443
Conversation
3c064a3 to
cddbf64
Compare
There was a problem hiding this comment.
Note
This issue/comment/review was translated by Claude.
Found a regression that needs fixing: Web Search provider availability now synchronously reads from Preference cache, and when the cache is not loaded, it treats the saved provider configuration as the default empty configuration, potentially clearing the user's selected external search provider when the chat page mounts for the first time. The other directions (removing local provider, RAG compression consolidation, Preference schema/migration) overall align with the v2 data consolidation approach.
Original Content
发现一个需要修复的回归:Web Search provider 可用性现在同步读取 Preference cache,cache 未加载时会把已保存的 provider 配置当成默认空配置,从而可能在聊天页首次挂载时清掉用户已选择的外部搜索 provider。其余方向(移除本地 provider、RAG 压缩收口、Preference schema/migration)整体符合 v2 数据收口思路。
32cb1a4 to
bce099a
Compare
0xfullex
left a comment
There was a problem hiding this comment.
Comprehensive Multi-Agent Review
I ran a multi-aspect review (general code review, silent-failure hunt, test coverage, comment audit, type design analysis) on this PR. Overall the shared-boundary type design is excellent (WebSearchCompressionMethod tightening, normalizeWebSearchCutoffLimit constructor, default-provider normalization, full rag removal across schema/main/renderer), but there are systemic silent-failure patterns on the renderer side that I think are worth addressing before merge.
Verifying the two existing review comments
| Comment | Conclusion | Action |
|---|---|---|
| @kangfenmao — cold-cache reads can clear saved provider | Real, and the current fix is incomplete | Expand the fix beyond Inputbar.tsx |
@zhangjiadi225 — searchWithTime permanently disabled |
Looks like a misread | No code change needed (see below) |
On the second point: WebSearchState.searchWithTime is boolean, not the literal false. The literal-false field on WebSearchState is overwrite (which is dead — isOverwriteEnabled() is @deprecated with no callers). searchWithTime is read from preference chat.web_search.search_with_time (default true), and src/renderer/src/services/__tests__/webSearchPreferences.test.ts:181-235 already asserts both true/false flow into BochaProvider/QueritProvider via WebSearchService.search. So the runtime behavior is preserved.
Critical
1. Cold-cache silent failure is systemic — Inputbar.tsx guard alone isn't enough
getCachedRendererWebSearchState() in src/renderer/src/services/WebSearchService.ts:529-540 substitutes getDefaultValue(key) when the cache is cold, returning a state that is observationally indistinguishable from "user has nothing configured." Consequences while preferences are still hydrating:
apiKeyresolves to''for every preset, soisWebSearchEnabled(id)returnsfalseeverywheresearchWithTime: true,compressionConfig.method: 'none',maxResults: 5override the user's persisted choices
The isWebSearchProviderOverridesLoaded guard added in Inputbar.tsx:168 is correct but only protects one surface. The same sync read still happens in:
src/renderer/src/pages/home/Inputbar/tools/components/WebSearchQuickPanelManager.tsx:132,139— quick panel shows every provider as disabledsrc/renderer/src/pages/settings/WebSearchSettings/WebSearchProviderSetting.tsx:151—canSetAsDefaultsrc/renderer/src/pages/settings/WebSearchSettings/BasicSettings.tsx:62src/renderer/src/services/WebSearchService.ts:161,518—processWebsearchreadssearchWithTimeandcompressionConfigfrom cached state, so even the async tool flow can silently fall back to defaults for the first request after launch
Suggested shape:
// WebSearchService.ts
getCachedRendererWebSearchState(): WebSearchState | null {
const missing = WEB_SEARCH_PREFERENCE_KEYS_LIST.filter(k => !preferenceService.isCached(k))
if (missing.length) {
logger.warn('Web-search state read before cache loaded; defaults applied', { missing })
return null
}
// existing build logic
}Then have callers handle null (skeleton / skip / suspense). At minimum, replicate the isCached guard at the four call sites above. For processWebsearch, prefer await getRendererWebSearchState() for the runtime config, or accept a snapshot from the caller.
2. Migration silently discards RAG configuration and unsupported provider ids
src/main/data/migration/v2/migrators/transformers/PreferenceTransformers.ts doesn't import loggerService at all, so when normalizeCompressionMethod collapses 'rag' -> 'none' (and the type strips embeddingModel/rerankModel/documentCount/embeddingDimensions at lines 238-242), and when normalizeWebSearchDefaultProvider collapses unsupported ids -> null, there is no trace of what was discarded. A v1 user upgrading will lose their RAG model selection with zero indication.
Minimum fix:
import { loggerService } from '@logger'
const logger = loggerService.withContext('PreferenceTransformers')
function normalizeCompressionMethod(value: unknown) {
if (value === 'rag') {
logger.warn('Legacy web-search RAG compression downgraded to none during v2 migration')
} else if (typeof value === 'string' && !WEB_SEARCH_COMPRESSION_METHODS.includes(value as any)) {
logger.warn(`Unknown web-search compression method "${value}" coerced to "none"`)
}
return isStringInList(value, WEB_SEARCH_COMPRESSION_METHODS) ? value : 'none'
}Same treatment for normalizeWebSearchDefaultProvider and migrateWebSearchProviders (the silent if (!preset) return drop). If you want to go further, surface a one-time post-migration toast pointing to the breaking-changes doc — that's the only honest UX for a destructive migration.
3. processWebsearch JSDoc is now stale
src/renderer/src/services/WebSearchService.ts:264-280 still says "应用结果压缩(RAG或截断)", but RAG is gone in this PR. Either trim it to "(截断)" or delete the JSDoc — it currently just narrates the function steps, which is what the project guideline asks us to avoid.
Important
isWebSearchEnabledis unsafe under undefined inputs —src/renderer/src/services/WebSearchService.ts:138-142:undefined?.trim() !== ''evaluates totrue, so any future caller that constructs aWebSearchProviderwithoutapiKey/apiHost(both?: stringin the renderer type) silently reports the provider as enabled. UseBoolean(provider.apiKey?.trim()).void someAsync()hides preference write failures across settings UI —BasicSettings.tsx:51,67,113,134,useWebSearchProviders.ts:64,WebSearchProviderSetting.tsx:48,58,68,78,ProviderSetting.tsx:104. When IPC rejects, the user sees a slider/toggle snap back with no toast. Consider asafeSetPreferencehelper that toasts on failure.filterSupportedWebSearchProviderssilently drops legacy ids —src/renderer/src/config/webSearchProviders.ts:139-143. Addlogger.warnwhen entries are dropped; ideally a one-time bootstrap toast tells the user that legacy local providers were removed.WebSearchProviderSettings.tsx:14-22redirects unsupported provider ids without notifying — add a toast beforenavigate(...).WebSearchProviderFactorydefault:falls back toDefaultProviderwhich throwsMethod not implemented.— the user-facing error has no signal of root cause. At leastlogger.errorthe unsupported id; in dev, throw at construction.WebSearchToolreturns{ query: '', results: [] }when provider is missing — the LLM will tell the user "I couldn't find anything." Consider returning a structured error so the model can surface the real reason.WebSearchState.overwrite: falseis a zombie field —isOverwriteEnabled()is@deprecatedand unused. Drop both for a clean type.WebSearchPhaserenderer copy is out of sync with shared —src/renderer/src/types/index.ts:783-789is missing'partial_failure', which the main process already emits.CitationBlock.tsx'sswitch (status.phase)silently falls into the default branch (showing "搜索中") instead of a partial-failure label. Delete the renderer copy, re-export from@shared/data/types/webSearch, and add the missingcase.- Comment cleanup —
WebSearchService.tsandWebSearchTool.tscarry several Chinese WHAT-translation comments that the project guideline asks us to drop (e.g.,// 重置状态,// 处理 summarize,// 使用 token 截断,// 检查是否需要搜索).
Test coverage gaps (highest priority)
getCachedRendererWebSearchStatecold-cache path — no test coversgetCachedValue -> undefinedInputbar.tsxisWebSearchProviderOverridesLoadedmatrix (cache-loaded x mandatory-model x supported-id) — no testuseWebSearchProviders.tshooks (4 hooks, +100/-55 lines) — zero direct test coverage; particularlyuseWebSearchProviderthrowing on unsupported id, anduseBlacklist.addSubscribeSourceempty-list behavior
Smaller gaps: full legacy rag payload (with cutoffLimit/cutoffUnit/discarded RAG fields together) in PreferenceTransformers.test.ts, malformed migrateWebSearchProviders inputs (null/string/missing id), parameterBuilder caller-override-vs-preference fallback, the five branches of isWebSearchEnabled.
Doc nits
v2-refactor-temp/docs/breaking-changes/2026-04-23-web-search-remove-local-providers-and-rag.md: "不做默认 provider 迁移" contradicts the actualnormalizeWebSearchDefaultProvidertransformer registered inComplexPreferenceMappings.ts. Also, the referenced commit hashcddbf6440doesn't match the current branch (likely a rebase artifact); prefer the PR link, which survives squash-merge.v2-refactor-temp/tools/data-classify/data/target-key-definitions.json: thechat.web_search.search_with_timedescription ("recent-date constraints where supported") doesn't match the actual behavior (it prepends today's date as a textual hint, no provider-side filtering). Suggest: "Prepend today's date to the query as a textual hint (no provider-side time filtering)."
Type design follow-ups (optional, can be a separate PR)
- Make
WebSearchCompressionConfiga discriminated union — currently{ method: 'none', cutoffLimit: 2000 }is a legal but meaningless state. - Deduplicate
WebSearchProviderId(declared in bothpackages/shared/data/preference/preferenceTypes.ts:135andsrc/renderer/src/types/index.ts:705); they happen to match today, but adding a new id on one side without the other will silently diverge. - Delete the unused
interface WebSearchProviderblock inpackages/shared/data/preference/preferenceTypes.ts:160-177— zero importers, parallel to the renderer/main types.
Strengths worth calling out
- Shared-boundary type work is excellent:
WebSearchCompressionMethodreduced to'none' | 'cutoff'so any stale=== 'rag'comparison is now a compile error;cutoffLimit: number | nulltightened tonumberwithnormalizeWebSearchCutoffLimitas a single normalization point called from main, renderer, and migration paths. Textbook "fix upstream, don't hack downstream." - Migration normalization for unsupported provider ids (compile-time + runtime double-defense) is well structured.
- i18n cleanup is consistent across all 14 locales for the 7 RAG keys.
- DataApi/Preference boundary respected — Web Search is user settings, correctly placed in Preference.
- All logger usage routes through
loggerService.withContext; noconsole.log. - Conventional commits are clean (kebab-case, domain-specific scopes).
- Removing the preexisting
// FIXME: 有待商榷,效果一般was the right call.
Happy to discuss any of the Critical items if you'd prefer a different approach. The cold-cache item is the one I'd most encourage tightening; the rest can be addressed incrementally or split into follow-up PRs.
…oviders - Updated WebSearchProviderSettings to navigate to general settings if the provider ID is invalid. - Simplified WebSearchSettings by removing local provider filtering and directly mapping all providers. - Consolidated type imports for web search providers across multiple files. - Removed unused local search provider classes (LocalBaiduProvider, LocalBingProvider, LocalGoogleProvider, LocalSearchProvider). - Enhanced WebSearchService to manage provider overrides and preferences more effectively. - Added tests for web search preferences to ensure correct functionality of provider resolution and state building.
Signed-off-by: kangfenmao <kangfenmao@qq.com>
…line provider selection logic
…and streamline state building
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: jdzhang <625013594@qq.com>
97f3a23 to
974e1e2
Compare
Signed-off-by: jdzhang <625013594@qq.com>
|
Thanks for the detailed review. I pushed two follow-up commits focused on the review feedback:
I did not make any extra code change for the earlier searchWithTime=false comment; per the later review, that was a misread. The searchWithTime-related runtime change above is only part of the cold-cache fix, so runtime config comes from async Preference instead of sync defaults. The branch has also been rebased onto the latest origin/v2 and is currently mergeable. Current checks at the time of this comment: basic-checks, changes, changeset-check, comment-legacy-css-vars, and general-test have passed; render-test is still running. |
There was a problem hiding this comment.
Note
This review was translated by Claude.
Second Comprehensive Review (Post Fix Wave)
After the comprehensive review on 2026-04-27, the author pushed ~16 follow-up commits with extensive targeted fixes. This time, I used an independent agent for "differential audit" to verify item-by-item whether the original review items were implemented; and also conducted multi-angle checks on silent-failure / test coverage / general code review in parallel.
Overall conclusion: Most of the original review has been addressed, but the fix wave introduced 3 new critical issues, and key fixes lack regression test protection.
I. Resolution of Previous Review
Resolved (11 items)
- @kangfenmao cold cache clearing provider — getCachedRendererWebSearchState() now returns null, Inputbar guards with preferenceService.isCached (974e1e2).
- @zhangjiadi225 [A1] searchWithTime silent failure — search() now uses await getRendererWebSearchState(), preference end-to-end connected to Bocha/Querit (5d97a74).
- C1 Systematic silent failure on cold cache — sync path only has Inputbar with guard, rest switched to reactive hooks.
- C2 Migration silently dropping RAG/unknown providers — all three normalize functions now logger.warn.
- C3 processWebsearch JSDoc outdated RAG description — corrected.
- I1 isWebSearchEnabled unsafe for undefined — changed to Boolean(provider.apiKey?.trim()).
- I8 WebSearchPhase renderer copy — changed to re-export @shared/data/types/webSearch, CitationBlock added case 'partial_failure'.
- T1 Cold cache tests — webSearchPreferences.test.ts now asserts cold-cache → null.
- D1/D2 Documentation errors — self-contradictory paragraphs deleted, search_with_time description corrected.
Partially Resolved
- I9 Chinese WHAT-translation comments — WebSearchService.ts still has // 重置状态 (×2) / // 处理 summarize / // 使用 token 截断 / // 检查 websearch 和 question 是否有效.
- T3 useWebSearchProviders.ts 4 hooks still have zero direct coverage.
Not Addressed
- I2 6+ places void setPreference(...) swallow write failures (BasicSettings.tsx:113,134, useWebSearchProviders.ts, WebSearchProviderSetting.tsx, ProviderSetting.tsx, CutoffSettings.tsx).
- I3 filterSupportedWebSearchProviders silent drop (webSearchProviders.ts:142-144).
- I4 WebSearchProviderSettings.tsx:14-22 redirect without toast.
- I5 Factory default: → DefaultProvider.search() throws Method not implemented. without logger.
- I6 Provider missing returns empty results (see N2).
- I7 WebSearchState.overwrite + isOverwriteEnabled() still zombie code.
- T2 Inputbar.isWebSearchProviderOverridesLoaded matrix has zero tests.
- Y2/Y3 WebSearchProviderId double definition; interface WebSearchProvider dead code.
II. New/Deepened Issues from Fix Wave
Critical (Must Fix Before Merge)
N1. Renderer processWebsearch throws on first reject — partial_failure is effectively dead code for renderer path
In src/renderer/src/services/WebSearchService.ts:
searchResults.forEach((result) => {
if (result.status === 'fulfilled') { finalResults.push(...result.value.results) }
if (result.status === 'rejected') {
throw result.reason // ← discards all successful results
}
})
Issue: The UI side (CitationBlock.tsx's case 'partial_failure'), i18n strings, and shared type are all in place, main-process WebSearchService also correctly implements partial-failure collection and emit. But WebSearchTool.ts in chat actually calls the renderer's processWebsearch, which throws on the first rejection, discarding all successful results. When multi-question partial failure occurs, users see a hard error and never see partial_failure.
The fix(web-search): show partial failure status commit only added the display side.
Suggested fix: Copy the main-process implementation — collect rejections, only rethrow AbortError, logger.warn others, and emit {phase:'partial_failure', countAfter:successfulCount} when successfulCount > 0 && successfulCount < total.
N2. WebSearchTool returns "no search needed" signal when provider missing — model gets gaslighted
src/renderer/src/aiCore/tools/WebSearchTool.ts:94-101
When getWebSearchProviderAsync(id) returns undefined, the tool returns {query:'', results:[]}. toModelOutput renders this as summary: 'No search needed based on the query analysis.' — the model believes "the system determined no search was needed" and may hallucinate answers based on training data, while the reality is provider configuration is missing or search backend unavailable.
logger.warn helps developers but is invisible to both the model and users.
Suggested fix:
if (!webSearchProvider) {
window.toast.error(t('settings.tool.websearch.provider_unavailable'))
return {
query: finalQueries.join(' | '),
results: [{
title: 'Web search unavailable',
content: 'The configured web search provider is missing or invalid. Tell the user to reconfigure it in settings.',
url: ''
}]
}
}
N3. basicAuthPassword.trim() asymmetry between migration and runtime — data corruption path
Migration: src/main/data/migration/v2/migrators/transformers/PreferenceTransformers.ts:228
const basicAuthPassword = provider.basicAuthPassword?.trim() // trimmed
Runtime: normalizeWebSearchProviderOverride()
if (override.basicAuthPassword !== undefined) {
normalizedOverride.basicAuthPassword = override.basicAuthPassword // no trim
}
Issue: If v1 user password has leading/trailing spaces (rare but valid), v2 first migration silently corrupts it via trim; subsequent UI edits won't trigger the same trim. Data loss only on migration path, no rollback.
Suggested fix: Remove .trim() from migration (passwords shouldn't be trimmed anyway).
High (Strongly Recommend Fix in Same PR)
N4. isWebSearchEnabled three-value collapse — non-Inputbar call sites unprotected by new guard
WebSearchService.ts:139-156 returns same false for both "cache not loaded" and "loaded but not configured". Inputbar bypasses via isWebSearchProviderOverridesLoaded guard, but these three places still treat "not loaded" as "not configured":
- WebSearchQuickPanelManager.tsx:132,139 — quick panel shows all providers as disabled
- BasicSettings.tsx:62
- WebSearchProviderSetting.tsx:151 — canSetAsDefault
This is the tail left by the C1 "patch-style" fix.
Suggested fix: Make isWebSearchEnabled return boolean | 'unknown', or expose isCacheReady(), short-circuit all three call sites; or more thoroughly switch these three to reactive hooks.
N5. Key fixes lack regression tests — fixed original bug but didn't install guardrails
- isWebSearchEnabled matrix: Original review's core bug (undefined?.trim() !== '' evaluates to true) fixed to Boolean(provider.apiKey?.trim()), but no test guards. Any refactoring will silently reintroduce the original bug.
- Inputbar guard matrix: isWebSearchProviderOverridesLoaded is key to the entire PR not breaking assistant.webSearchProviderId, zero tests.
- BochaProvider/QueritProvider behavior: Current tests only verify state reaches engine, not whether Bocha maps searchWithTime: true to freshness: 'oneDay', or whether Querit sets filters.timeRange={date:'d1'} — zhangjiadi225 [A1]'s fix has test blind spots.
Suggested fix: Add (1) apiKey: undefined / '' / ' ' / 'k' matrix, (2) Inputbar guard (cache-loaded × mandatory-model × supported-id) tests, (3) at least one BochaProvider and QueritProvider request-shape assertion.
Medium / Suggestions
- Migration 'rag' → 'none' warn doesn't include discarded embeddingModel/rerankModel/documentCount/embeddingDimensions — untraceable what was lost after migration.
- flattenCompressionConfig: legacy cutoffLimit: null ("unlimited" semantics) silently changed to 2000 without warn. Suggest also recording breaking change.
- BlacklistSettings.handleDeleteSubscribe try/catch wraps synchronous code but receives void promise — catch never catches IPC rejection.
- ComplexPreferenceMappings.test.ts:121 uses expect(keys.length).toBe(31) magic number — breaks when v2 adds mappings, suggest toBeGreaterThanOrEqual(31) or assert membership.
III. Commendable Fixes
- Main-process WebSearchService.ts rewrite is very clean: layered (prepareSearchContext/executeSearches/buildFinalResponse), mutex guards state writes, finally cleanup — textbook implementation. Ironically, renderer path didn't follow it (see N1).
- normalizeWebSearchCutoffLimit converges number | null to number, deleting multiple null checks — standard "fix upstream".
- Shared type convergence, 'rag' compile-time elimination, migration end-to-end logger.warn.
- getCachedRendererWebSearchState returns null rather than synthesized default object — cold-cache handling approach correct.
- 14 locales' RAG string cleanup consistent.
IV. Merge Threshold Recommendations
Required (block merge): N1 (renderer partial_failure dead code), N2 (LLM gaslight), N3 (password trim), N5's isWebSearchEnabled and Inputbar guard regression tests.
Strongly recommend fixing in same PR: I2 (centralize void setPreference → safeSetPreference), I7 (delete zombie overwrite), I9 (Chinese WHAT-comment cleanup), N4 (isWebSearchEnabled three-value return).
Can split into follow-up PR: I3/I4/I5 (logging + toast consistency), I6 (LLM-friendly error signal), Y2/Y3 (type deduplication), N6 (provider behavior tests).
Happy to discuss implementation path for any of N1 / N2 / N3, or if you prefer, I can create a patch for N1 directly.
Original Content
二次综合 Review(修复浪潮后)
在 2026-04-27 综合 review 之后,作者推送了 ~16 个 follow-up commit,做了大量针对性修复。这次专门用一个独立 agent 做"差分审计",逐项核对原 review item 是否落地;并并行做了 silent-failure / 测试覆盖 / 通用 code review 的多视角检查。
整体结论:原始 review 大部分已解决,但修复浪潮自身引入了 3 个新 critical,且关键修复缺少回归测试守护。
一、之前 review 的解决情况
已解决(11 项)
- @kangfenmao 冷缓存清空 provider — getCachedRendererWebSearchState() 现在返回 null,Inputbar 用 preferenceService.isCached 守卫(974e1e2)。
- @zhangjiadi225 [A1] searchWithTime 静默失效 — search() 改用 await getRendererWebSearchState(),preference 端到端打通到 Bocha/Querit(5d97a74)。
- C1 冷缓存系统性 silent failure — sync 路径只剩 Inputbar 且有守卫,其余切到 reactive hooks。
- C2 迁移静默丢弃 RAG/未知 provider — 三个 normalize 函数全部 logger.warn。
- C3 processWebsearch JSDoc 过期 RAG 描述 — 已修正。
- I1 isWebSearchEnabled 对 undefined 不安全 — 改为 Boolean(provider.apiKey?.trim())。
- I8 WebSearchPhase renderer 副本 — 改 re-export @shared/data/types/webSearch,CitationBlock 加 case 'partial_failure'。
- T1 冷缓存测试 — webSearchPreferences.test.ts 已断言 cold-cache → null。
- D1/D2 文档错误 — 自相矛盾段落删除、search_with_time description 修正。
部分解决
- I9 中文 WHAT-translation 注释 — WebSearchService.ts 仍有 // 重置状态(×2)/ // 处理 summarize / // 使用 token 截断 / // 检查 websearch 和 question 是否有效。
- T3 useWebSearchProviders.ts 4 个 hook 仍零直接覆盖。
未处理
- I2 6+ 处 void setPreference(...) 吞掉写入失败(BasicSettings.tsx:113,134、useWebSearchProviders.ts、WebSearchProviderSetting.tsx、ProviderSetting.tsx、CutoffSettings.tsx)。
- I3 filterSupportedWebSearchProviders 静默 drop(webSearchProviders.ts:142-144)。
- I4 WebSearchProviderSettings.tsx:14-22 重定向无 toast。
- I5 Factory default: → DefaultProvider.search() 抛 Method not implemented. 无 logger。
- I6 provider 缺失时返回空结果(详见 N2)。
- I7 WebSearchState.overwrite + isOverwriteEnabled() 仍是僵尸代码。
- T2 Inputbar.isWebSearchProviderOverridesLoaded 矩阵零测试。
- Y2/Y3 WebSearchProviderId 双定义;interface WebSearchProvider 死代码。
二、修复浪潮中新出现/加深的问题
Critical(合并前必修)
N1. Renderer processWebsearch 第一个 reject 即 throw — partial_failure 对 renderer 路径实际是死代码
src/renderer/src/services/WebSearchService.ts 中:
searchResults.forEach((result) => {
if (result.status === 'fulfilled') { finalResults.push(...result.value.results) }
if (result.status === 'rejected') {
throw result.reason // ← 丢弃所有已成功的结果
}
})
问题:UI 端(CitationBlock.tsx 的 case 'partial_failure')、i18n 字串、shared type 全部就位,main-process WebSearchService 也正确实现了 partial-failure 收集与 emit。但 WebSearchTool.ts 在 chat 中实际调用的是 renderer 的 processWebsearch,第一个 rejection 就 throw 整个流程,丢弃所有已成功的结果。多 question 部分失败时用户看到硬错误,永远见不到 partial_failure。
fix(web-search): show partial failure status 这个 commit 只加了显示侧。
建议修复:照搬 main-process 的实现 — 收集 rejection、仅 rethrow AbortError、其他 logger.warn,当 successfulCount > 0 && successfulCount < total 时 emit {phase:'partial_failure', countAfter:successfulCount}。
N2. provider 缺失时 WebSearchTool 给 LLM 返回"无需搜索"信号 — 模型被 gaslight
src/renderer/src/aiCore/tools/WebSearchTool.ts:94-101
当 getWebSearchProviderAsync(id) 返回 undefined 时,工具返回 {query:'', results:[]}。toModelOutput 据此渲染成 summary: 'No search needed based on the query analysis.' — 模型据此相信"系统判断无需搜索",可能直接基于训练数据幻觉答案,而真实情况是 provider 配置丢失、search backend 不可用。
logger.warn 对开发者有用,但对模型与用户均不可见。
建议修复:
if (!webSearchProvider) {
window.toast.error(t('settings.tool.websearch.provider_unavailable'))
return {
query: finalQueries.join(' | '),
results: [{
title: 'Web search unavailable',
content: 'The configured web search provider is missing or invalid. Tell the user to reconfigure it in settings.',
url: ''
}]
}
}
N3. basicAuthPassword.trim() 在迁移与运行时不对称 — 数据腐蚀路径
迁移:src/main/data/migration/v2/migrators/transformers/PreferenceTransformers.ts:228
const basicAuthPassword = provider.basicAuthPassword?.trim() // trimmed
运行时:normalizeWebSearchProviderOverride()
if (override.basicAuthPassword !== undefined) {
normalizedOverride.basicAuthPassword = override.basicAuthPassword // 不 trim
}
问题:v1 用户密码若有前后空格(少见但合法),v2 首次迁移即被静默 trim 损坏;之后通过 UI 修改不会触发同样的 trim。仅迁移路径的数据丢失,无回滚。
建议修复:去掉迁移中的 .trim()(密码本就不应被 trim)。
High(强烈建议同 PR 修复)
N4. isWebSearchEnabled 三值塌缩 — 非 Inputbar 调用点未受新守卫保护
WebSearchService.ts:139-156 对"缓存未加载"和"已加载但未配置"返回同一个 false。Inputbar 通过 isWebSearchProviderOverridesLoaded 守卫绕过,但下面三处仍把"未加载"视作"未配置":
- WebSearchQuickPanelManager.tsx:132,139 — quick panel 显示所有 provider 为 disabled
- BasicSettings.tsx:62
- WebSearchProviderSetting.tsx:151 — canSetAsDefault
这是 C1 修复"打补丁式"留下的尾巴。
建议修复:让 isWebSearchEnabled 返回 boolean | 'unknown',或暴露 isCacheReady(),三处调用点同样短路;或者更彻底地把这三处也切到 reactive hook。
N5. 关键修复缺少回归测试 — 修了原 bug 但没装栏杆
- isWebSearchEnabled 矩阵:原 review 的核心 bug (undefined?.trim() !== '' 评估为 true) 已修复成 Boolean(provider.apiKey?.trim()),但无测试守护。任何反向重构都会让原 bug 静默回归。
- Inputbar guard 矩阵:isWebSearchProviderOverridesLoaded 是整个 PR 不破坏 assistant.webSearchProviderId 的关键,零测试。
- BochaProvider/QueritProvider 行为:当前测试只验证 state 传到 engine,未验证 Bocha 是否将 searchWithTime: true 映射为 freshness: 'oneDay'、Querit 是否设 filters.timeRange={date:'d1'} —— 即 zhangjiadi225 [A1] 的修复有测试盲点。
建议修复:补 (1) apiKey: undefined / '' / ' ' / 'k' 矩阵、(2) Inputbar guard 的 (cache-loaded × mandatory-model × supported-id) 测试、(3) 至少一个 BochaProvider 与 QueritProvider 的 request-shape 断言。
Medium / Suggestions
- 迁移 'rag' → 'none' warn 不包含被丢弃的 embeddingModel/rerankModel/documentCount/embeddingDimensions — 迁移完即不可追溯被丢失了什么。
- flattenCompressionConfig:legacy cutoffLimit: null("不限"语义)被静默改为 2000,无 warn。建议同时记录 breaking change。
- BlacklistSettings.handleDeleteSubscribe try/catch 包同步代码却接 void promise — catch 永远抓不到 IPC 拒绝。
- ComplexPreferenceMappings.test.ts:121 用 expect(keys.length).toBe(31) 这种 magic number — v2 后续添加映射就破,建议改为 toBeGreaterThanOrEqual(31) 或断言 membership。
三、值得肯定的修复
- 主进程 WebSearchService.ts 重写非常干净:分层 (prepareSearchContext/executeSearches/buildFinalResponse)、mutex 守卫状态写入、finally 清理 — 教科书式实现。讽刺的是 renderer 路径没沿用(见 N1)。
- normalizeWebSearchCutoffLimit 把 number | null 收敛为 number,删除多处 null 判断 — 标准 "fix upstream"。
- 共享 type 收敛、'rag' 编译期消除、迁移端到端 logger.warn。
- getCachedRendererWebSearchState 返回 null 而非合成默认对象 — cold-cache 处理思路正确。
- 14 个 locale 的 RAG 字串清理一致。
四、合并门槛建议
必修(block merge):N1(renderer partial_failure dead code)、N2(LLM gaslight)、N3(password trim)、N5 中的 isWebSearchEnabled 与 Inputbar guard 回归测试。
强烈建议同 PR 修复:I2(void setPreference 集中改 safeSetPreference)、I7(删除 zombie overwrite)、I9(中文 WHAT-comment 清理)、N4(isWebSearchEnabled 三值返回)。
可拆分 follow-up PR:I3/I4/I5(logging + toast 一致性)、I6(LLM-friendly error signal)、Y2/Y3(类型去重)、N6(provider 行为测试)。
很乐意就 N1 / N2 / N3 任一项进一步讨论实现路径,或如果你希望,我可以把 N1 直接做成 patch。
Signed-off-by: jdzhang <625013594@qq.com>
|
Thanks @0xfullex. I pushed a follow-up commit addressing the blocking review items:
What changed:
Validation:
One note: I focused the N5 test changes on the merge-threshold coverage you called out for isWebSearchEnabled and the Inputbar guard. I did not add Bocha/Querit request-shape tests in this follow-up commit. |
0xfullex
left a comment
There was a problem hiding this comment.
Third Comprehensive Review (post fix(web-search): address review feedback)
After commit 93bcfca5, the author addressed N1 / N2 / N3 and added the Inputbar guard regression test (T2). However, N4, the rest of N5, several Important items, and all four Medium items from the previous review still stand. Filing as CHANGES_REQUESTED because N4 plus the missing isWebSearchEnabled / Bocha / Querit regression tests are still merge-blocking on my original threshold.
I. Resolution Update
Resolved
- N1 Renderer
partial_failuredead code —src/renderer/src/services/WebSearchService.ts:351-407now collects rejections, rethrows onlyAbortError, logs the rest, and emitspartial_failurewhensuccessfulCount > 0 && failed > 0. Mirrors the main-process implementation. - N2 WebSearchTool gaslighting the LLM —
src/renderer/src/aiCore/tools/WebSearchTool.ts:97-108returns a structuredWeb search provider unavailableresult, andtoModelOutput(:132-134) outputs an explicit "configured provider is unavailable" message instead of "No search needed". - N3
basicAuthPassword.trim()asymmetry —.trim()removed from both migration (PreferenceTransformers.ts:401-403) and runtime (resolveWebSearchProvidersnow uses?? ''); regression test seeded withpass with spacesto lock in the round-trip. - T2 Inputbar guard had zero coverage — extracted into
src/renderer/src/pages/home/Inputbar/utils/webSearchProviderGuard.tswith a 4-case unit test matrix (cold cache / loaded-disabled / mandatory-model / no-override). Good shape.
Still outstanding — block merge
-
N4
isWebSearchEnabledthree-value collapse —src/renderer/src/services/WebSearchService.ts:140-157still returnsfalsefor both "cache cold" and "loaded but not configured". The three call sites I flagged are still unprotected:src/renderer/src/pages/home/Inputbar/tools/components/WebSearchQuickPanelManager.tsx:132,139— quick panel shows every provider as disabled during hydrationsrc/renderer/src/pages/settings/WebSearchSettings/BasicSettings.tsx:62src/renderer/src/pages/settings/WebSearchSettings/WebSearchProviderSetting.tsx:151—canSetAsDefault
Suggested: have
isWebSearchEnabledreturnboolean | 'unknown'(or exposeisCacheReady()from the cached state helpers) so these three surfaces can short-circuit, the same way the Inputbar guard does. The current shape preserves a known regression in three surfaces and undermines the value of93bcfca5. -
N5 (partial) Regression tests for the original review bugs:
isWebSearchEnabledapiKey: undefined / '' / ' ' / 'k'matrix — still no test. TheBoolean(provider.apiKey?.trim())fix has zero guardrail; any future refactor can silently re-introduceundefined?.trim() !== '' === true.BochaProvider/QueritProviderrequest-shape — current tests assert that state reaches the engine, but not thatsearchWithTime: truemaps tofreshness: 'oneDay'(Bocha) orfilters.timeRange={date: 'd1'}(Querit). The @zhangjiadi225 [A1] fix is still in a test blind spot.
II. Strongly recommend fixing in this PR
-
I2
void setPreference(...)swallowing IPC rejections — eight call sites in this PR's surface area still fire-and-forget:BasicSettings.tsx:113,134CompressionSettings/index.tsx:21CompressionSettings/CutoffSettings.tsx:16,20WebSearchQuickPanelManager.tsx:80,82WebSearchButton.tsx:22BlacklistSettings.tsx:218
Suggested: a small
safeSetPreferencehelper that toasts on rejection, then replacevoid setX(...)at those eight sites. One-time fix, eliminates an entire class of silent failures. -
I7
WebSearchState.overwritezombie —WebSearchService.ts:166-168isOverwriteEnabled()is@deprecatedwith no callers, andoverwrite: falseat:587is structurally unreachable. The PR is already converging the type surface — finishing the cleanup is one more delete pass. -
I9 Chinese WHAT-translation comments — six still in tree, project guideline asks us to drop these:
WebSearchService.ts:264 // 使用 token 截断WebSearchService.ts:303 // 重置状态WebSearchService.ts:306 // 检查 websearch 和 question 是否有效WebSearchService.ts:331 // 处理 summarizeWebSearchService.ts:441 // 重置状态WebSearchTool.ts:88 // 检查是否需要搜索
III. Can split into follow-up PR
- I3
filterSupportedWebSearchProviderssilent drop —src/renderer/src/config/webSearchProviders.ts:142-144still nologger.warnwhen a legacy id is filtered out. - I4
WebSearchProviderSettings.tsx:17-18redirects unsupported provider ids viavoid navigate(...)with no toast — same silent-failure pattern. - I5 Factory
default:→DefaultProvider.search()throwsMethod not implemented.with no logger context:WebSearchProviderFactory.ts:30-31,DefaultProvider.ts:6-7. At minimum,logger.errorthe unsupported id at construction; in dev, throw at construction so the bug is visible. - T3
useWebSearchProviders.tsfour hooks — still zero direct test coverage. - Y2
WebSearchProviderIddeclared in bothpackages/shared/data/preference/preferenceTypes.ts:137andsrc/renderer/src/types/index.ts:717. They happen to match today; adding an id to one side without the other will silently diverge. - Y3 Unused
interface WebSearchProviderblock atpreferenceTypes.ts:160— zero importers, parallel to the renderer/main types.
Medium suggestions still standing
- Migration
'rag' → 'none'logger.warnatPreferenceTransformers.ts:262does not include the discardedembeddingModel/rerankModel/documentCount/embeddingDimensions. Once migration runs, what was lost is unrecoverable from logs. flattenCompressionConfig: legacycutoffLimit: null(semantically "unlimited") is silently coerced to2000with nowarn. Worth either alogger.warnor an entry in the breaking-changes doc.BlacklistSettings.tsx:213-224try/catchwraps a synchronous block that firesvoid setSubscribeSources(...)— thecatchwill never see an IPC rejection.ComplexPreferenceMappings.test.ts:121expect(keys.length).toBe(31)is a magic number that breaks every time v2 adds a mapping — prefertoBeGreaterThanOrEqual(31)or membership assertions.
IV. Merge threshold
Block merge: N4, plus N5's isWebSearchEnabled matrix and Bocha / Querit request-shape regression tests.
Strongly recommend in same PR: I2 (safeSetPreference), I7 (drop zombie overwrite), I9 (remove the six remaining Chinese WHAT-comments).
Can split into follow-up PR: I3 / I4 / I5, T3, Y2 / Y3, and the four Medium items above.
Happy to draft a patch for N4 or the safeSetPreference helper if that helps land them in this PR.
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
|
@0xfullex 继续麻烦帮忙复审一下。 这轮根据你在 review 里的剩余意见,我把需要在当前 PR 收口的部分又补了一次,当前 head 是 已修复 / 本 PR 已收口
暂未修改 / 建议 follow-up 的部分这些我没有继续塞进当前 PR,主要是因为不是这次阻塞门槛,且会扩大改动面:
验证本地已跑:
推送后 GitHub checks 也已全绿: |
0xfullex
left a comment
There was a problem hiding this comment.
Approve
Verified against f6d7c20d. All merge-blocking items from my previous review (N4 / N5 / I2 / I7 / I9) are addressed; I3 / I4 / I5 were also folded in even though they were tagged as splittable. Thanks for the thorough turnaround.
Verified resolved
- N4
isWebSearchEnabledthree-state —WebSearchService.ts:34,133-156introducesWebSearchAvailability = boolean | 'unknown'and returns'unknown'while the preference cache is cold. The three call sites I flagged are all adapted:WebSearchQuickPanelManager.tsx:131-152(loading description + disabled),BasicSettings.tsx:62-65(silent short-circuit),WebSearchProviderSetting.tsx:150(strict=== trueforcanSetAsDefault). - N5 Regression tests —
webSearchPreferences.test.ts:285-344covers cold-cachenull/'unknown'and theit.eachmatrix for empty / blank / valid API key, plus host-required and preset-host paths. NewsearchWithTime.test.tsasserts Bocha body containsfreshness: 'oneDay'and Querit body containsfilters.timeRange={ date: 'd1' }.WebSearchProviderFactory.test.tslocks in the unsupported-idlogger.errorpath. - I2
void setPreference(...)swallowing —useWebSearchProviders.ts:20-27introducessafeSetWebSearchPreference(action, update)(try/await/catch withlogger.error+window.toast.error), and the nine surrounding hook entry points all route through it. Keepingvoid setX(...)at the call sites is the right call — error handling is internalised in the hook.ProviderSetting.tsx:141-142Zhipu sync now.catch-logs, andBlacklistSettings.tsx:213-227switches toasync+await setSubscribeSources+ catch + toast (also fixes the prior medium item where thetry/catchwrapped a synchronous block). - I7 Zombie
overwriteremoved —isOverwriteEnabled()deleted fromWebSearchService.ts,overwrite: falseno longer constructed inbuildRendererWebSearchState, andWebSearchStateinsrc/renderer/src/types/websearch.tsis clean. - I9 Six remaining Chinese WHAT-translation comments removed — verified via grep.
- I3 / I4 / I5 also folded in (I had originally tagged these as splittable):
webSearchProviders.ts:136,146-162addswarnedUnsupportedProviderIds: Set<string>for one-timelogger.warnper filtered id.WebSearchProviderSettings.tsx:20-29adds warn + toast + redirect-failure catch.WebSearchProviderFactory.ts:14,34addslogger.errorbefore theDefaultProviderfallback.
Worth calling out
- The decision to put
safeSetWebSearchPreferenceinside the hook rather than expose a global helper is the better shape — call sites stay unceremonious (void setSearchWithTime(checked)) while the safety guarantee is structurally enforced at the boundary. - The
availability !== falsefilter atWebSearchQuickPanelManager.tsx:151correctly distinguishes "still loading" (kept, shown disabled with loading copy) from "known disabled" (excluded) — exactly the semantic the three-state return enables. - The renderer
processWebsearchrewrite (collect rejections, rethrow onlyAbortError, emitpartial_failurewhensuccessfulCount > 0 && failed > 0) now mirrors the main-process implementation. End-to-end consistency for thepartial_failureUI path.
Non-blocking follow-up suggestions
These are fine to defer — none are merge-blocking — but worth tracking in a follow-up issue so they don't get lost:
useWebSearchProviders.tsfour hooks — direct hook-level tests (current coverage is behavioural viawebSearchPreferences.test.ts, which is sufficient but not granular).WebSearchProviderIddeclared in bothpackages/shared/data/preference/preferenceTypes.ts:137andsrc/renderer/src/types/index.ts:717— type design dedup.- Unused
interface WebSearchProviderblock atpackages/shared/data/preference/preferenceTypes.ts:160— zero importers. - Migration
'rag' → 'none'logger.warncould include the discardedembeddingModel/rerankModel/documentCount/embeddingDimensionsso post-migration audit can recover the lost configuration. flattenCompressionConfig: legacycutoffLimit: null(semantically "unlimited") silently coerced to2000— at minimum alogger.warn, ideally a line in the breaking-changes doc since this is a behaviour change for a small but real cohort.ComplexPreferenceMappings.test.ts:121expect(keys.length).toBe(31)— prefertoBeGreaterThanOrEqual(31)or membership assertions so the test doesn't break every time v2 adds a mapping.
Approving — happy to file the follow-up issue if that helps.
What this PR does
Before this PR:
The Web Search settings UI still exposed local search providers such as local Google, local Bing, and local Baidu, even though the backend no longer supports them. Renderer Web Search settings also still depended on legacy store-backed state in several paths. Web Search compression still exposed and persisted RAG/knowledge-base related settings, which added runtime overhead and made configuration harder for users.
After this PR:
Unsupported local Web Search providers are removed from the renderer UI and provider factory, Web Search settings and chat entry points read from the v2 Preference-backed data source, and legacy leftover provider values are handled defensively without crashing the UI. Web Search compression is simplified to the supported
noneandcutoffmodes; RAG/knowledge-base compression UI, renderer runtime logic, Preference schema keys, migration mappings, and related translations are removed.Fixes # N/A
Why we need it and why it was done in this way
The following tradeoffs were made:
This PR keeps the scope focused on the v2 Web Search renderer and preference migration path. It does not clean up the legacy Redux store shape or perform default provider migration beyond the v2 preference mapping work, so that broader store cleanup can happen separately with the rest of the v2 refactor. Removing Web Search RAG compression reduces search-time work and keeps the settings surface simpler for users.
The following alternatives were considered:
Keeping local providers hidden only at render time was considered, but removing their renderer provider implementations and factory branches makes the unsupported state easier to reason about. Moving all Web Search UI state directly to DataApi was not used because these values are user preferences, so Preference is the appropriate v2 data source. Keeping RAG compression as an advanced option was considered, but the added model/knowledge-base configuration complexity was not worth the runtime cost for the Web Search flow.
Links to places where the discussion took place: N/A
Breaking changes
No breaking changes for supported Web Search providers. Unsupported local Web Search providers are no longer shown or selectable in the renderer UI. Web Search RAG compression settings are removed; legacy migrated
method: 'rag'values fall back tononeinstead of being persisted as RAG preferences.Special notes for your reviewer
Please review the Web Search settings flows with existing migrated preference values, especially default provider selection, provider checks, blacklist updates, max results, and compression settings. The generated preference schema/mapping files were updated through the v2 data-classify pipeline. The migration transformer intentionally collapses legacy Web Search RAG compression to
none. This PR also fixes a stalepackages/uidocs README link that was blockingpnpm build:check.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note