Skip to content

refactor(websearch-settings): remove local providers and use v2 preferences#14443

Merged
0xfullex merged 21 commits intov2from
v2-remove-local-websearch
May 6, 2026
Merged

refactor(websearch-settings): remove local providers and use v2 preferences#14443
0xfullex merged 21 commits intov2from
v2-remove-local-websearch

Conversation

@kangfenmao
Copy link
Copy Markdown
Collaborator

@kangfenmao kangfenmao commented Apr 21, 2026

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 none and cutoff modes; 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 to none instead 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 stale packages/ui docs README link that was blocking pnpm 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.

Release note

Removed unsupported local Web Search providers and Web Search RAG compression settings. Web Search settings now use the v2 Preference-backed data source.

@kangfenmao kangfenmao requested a review from a team April 21, 2026 08:03
@kangfenmao kangfenmao requested a review from 0xfullex as a code owner April 21, 2026 08:03
@kangfenmao kangfenmao marked this pull request as draft April 21, 2026 08:04
@kangfenmao kangfenmao added the v2 label Apr 21, 2026
@kangfenmao kangfenmao force-pushed the v2-remove-local-websearch branch from 3c064a3 to cddbf64 Compare April 23, 2026 06:28
@kangfenmao kangfenmao marked this pull request as ready for review April 23, 2026 06:31
Copy link
Copy Markdown
Collaborator Author

@kangfenmao kangfenmao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 数据收口思路。

Comment thread src/renderer/src/services/WebSearchService.ts Outdated
Copy link
Copy Markdown
Collaborator

@zhangjiadi225 zhangjiadi225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed 1 behavior change issue: searchWithTime is permanently disabled after v2 Preference migration, but supported providers like Bocha/Querit still rely on this field to determine recent search parameters.

Comment thread src/renderer/src/services/WebSearchService.ts Outdated
@kangfenmao kangfenmao force-pushed the v2-remove-local-websearch branch from 32cb1a4 to bce099a Compare April 27, 2026 08:48
Copy link
Copy Markdown
Member

@0xfullex 0xfullex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@zhangjiadi225searchWithTime 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:

  • apiKey resolves to '' for every preset, so isWebSearchEnabled(id) returns false everywhere
  • searchWithTime: true, compressionConfig.method: 'none', maxResults: 5 override 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 disabled
  • src/renderer/src/pages/settings/WebSearchSettings/WebSearchProviderSetting.tsx:151canSetAsDefault
  • src/renderer/src/pages/settings/WebSearchSettings/BasicSettings.tsx:62
  • src/renderer/src/services/WebSearchService.ts:161,518processWebsearch reads searchWithTime and compressionConfig from 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

  • isWebSearchEnabled is unsafe under undefined inputssrc/renderer/src/services/WebSearchService.ts:138-142: undefined?.trim() !== '' evaluates to true, so any future caller that constructs a WebSearchProvider without apiKey/apiHost (both ?: string in the renderer type) silently reports the provider as enabled. Use Boolean(provider.apiKey?.trim()).
  • void someAsync() hides preference write failures across settings UIBasicSettings.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 a safeSetPreference helper that toasts on failure.
  • filterSupportedWebSearchProviders silently drops legacy idssrc/renderer/src/config/webSearchProviders.ts:139-143. Add logger.warn when entries are dropped; ideally a one-time bootstrap toast tells the user that legacy local providers were removed.
  • WebSearchProviderSettings.tsx:14-22 redirects unsupported provider ids without notifying — add a toast before navigate(...).
  • WebSearchProviderFactory default: falls back to DefaultProvider which throws Method not implemented. — the user-facing error has no signal of root cause. At least logger.error the unsupported id; in dev, throw at construction.
  • WebSearchTool returns { 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: false is a zombie field — isOverwriteEnabled() is @deprecated and unused. Drop both for a clean type.
  • WebSearchPhase renderer copy is out of sync with sharedsrc/renderer/src/types/index.ts:783-789 is missing 'partial_failure', which the main process already emits. CitationBlock.tsx's switch (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 missing case.
  • Comment cleanupWebSearchService.ts and WebSearchTool.ts carry several Chinese WHAT-translation comments that the project guideline asks us to drop (e.g., // 重置状态, // 处理 summarize, // 使用 token 截断, // 检查是否需要搜索).

Test coverage gaps (highest priority)

  • getCachedRendererWebSearchState cold-cache path — no test covers getCachedValue -> undefined
  • Inputbar.tsx isWebSearchProviderOverridesLoaded matrix (cache-loaded x mandatory-model x supported-id) — no test
  • useWebSearchProviders.ts hooks (4 hooks, +100/-55 lines) — zero direct test coverage; particularly useWebSearchProvider throwing on unsupported id, and useBlacklist.addSubscribeSource empty-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 actual normalizeWebSearchDefaultProvider transformer registered in ComplexPreferenceMappings.ts. Also, the referenced commit hash cddbf6440 doesn'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: the chat.web_search.search_with_time description ("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 WebSearchCompressionConfig a discriminated union — currently { method: 'none', cutoffLimit: 2000 } is a legal but meaningless state.
  • Deduplicate WebSearchProviderId (declared in both packages/shared/data/preference/preferenceTypes.ts:135 and src/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 WebSearchProvider block in packages/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: WebSearchCompressionMethod reduced to 'none' | 'cutoff' so any stale === 'rag' comparison is now a compile error; cutoffLimit: number | null tightened to number with normalizeWebSearchCutoffLimit as 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; no console.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.

kangfenmao and others added 16 commits April 30, 2026 13:58
…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>
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>
@zhangjiadi225 zhangjiadi225 force-pushed the v2-remove-local-websearch branch from 97f3a23 to 974e1e2 Compare April 30, 2026 06:01
Signed-off-by: jdzhang <625013594@qq.com>
@zhangjiadi225
Copy link
Copy Markdown
Collaborator

Thanks for the detailed review. I pushed two follow-up commits focused on the review feedback:

  • 974e1e2 fix(web-search): handle cold preference cache

    • Made synchronous renderer web-search state reads cache-aware: when the Preference cache is cold, getCachedRendererWebSearchState now returns null instead of building a default-looking state.
    • Updated synchronous provider availability checks to avoid treating unloaded provider overrides as an empty configuration, and fixed the optional trim check with Boolean(...?.trim()).
    • Switched runtime search/compression reads to async Preference state so the first request after launch does not silently fall back to default searchWithTime, maxResults, or compressionConfig values.
    • Added migration warnings when legacy RAG compression, unknown compression methods, unsupported default providers, or unsupported legacy providers are downgraded/dropped.
    • Added cold-cache tests and corrected the breaking-changes/search_with_time documentation notes.
  • 1d58713 fix(web-search): show partial failure status

    • Reused the shared WebSearchPhase/WebSearchStatus types in the renderer so partial_failure stays in sync with the main/shared contract.
    • Added a CitationBlock branch and i18n strings for partial_failure so partial web-search failures no longer fall through to the generic searching state.
    • Added a CitationBlock test covering the partial_failure display.

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.

@zhangjiadi225 zhangjiadi225 requested a review from 0xfullex April 30, 2026 06:24
Copy link
Copy Markdown
Member

@0xfullex 0xfullex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@zhangjiadi225
Copy link
Copy Markdown
Collaborator

Thanks @0xfullex. I pushed a follow-up commit addressing the blocking review items:

  • 93bcfca fix(web-search): address review feedback

What changed:

  • N1 renderer partial_failure: renderer processWebsearch now preserves fulfilled search results, rethrows AbortError, logs non-abort query failures, emits partial_failure with countAfter when some queries succeed, and still throws when all queries fail.
  • N2 provider unavailable: WebSearchTool no longer returns an empty query/results payload that maps to “No search needed”. It now returns an explicit model-facing unavailable result and summary when the configured provider cannot be resolved.
  • N3 basicAuthPassword: the migration path no longer trims basicAuthPassword, and renderer provider resolution also preserves password whitespace. Username, host, and API key normalization remain unchanged.
  • N5 regression coverage: added tests for the isWebSearchEnabled API-key/host matrix, Inputbar cold-cache provider guard, renderer partial failure/abort behavior, provider-unavailable tool output, and basicAuthPassword whitespace preservation.

Validation:

  • pnpm build:check passed after the push commit locally.
  • Full test suite passed: 398 test files, 6693 tests passed, 72 skipped.

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.

Copy link
Copy Markdown
Member

@0xfullex 0xfullex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_failure dead code — src/renderer/src/services/WebSearchService.ts:351-407 now collects rejections, rethrows only AbortError, logs the rest, and emits partial_failure when successfulCount > 0 && failed > 0. Mirrors the main-process implementation.
  • N2 WebSearchTool gaslighting the LLM — src/renderer/src/aiCore/tools/WebSearchTool.ts:97-108 returns a structured Web search provider unavailable result, and toModelOutput (: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 (resolveWebSearchProviders now uses ?? ''); regression test seeded with pass with spaces to lock in the round-trip.
  • T2 Inputbar guard had zero coverage — extracted into src/renderer/src/pages/home/Inputbar/utils/webSearchProviderGuard.ts with a 4-case unit test matrix (cold cache / loaded-disabled / mandatory-model / no-override). Good shape.

Still outstanding — block merge

  • N4 isWebSearchEnabled three-value collapse — src/renderer/src/services/WebSearchService.ts:140-157 still returns false for 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 hydration
    • src/renderer/src/pages/settings/WebSearchSettings/BasicSettings.tsx:62
    • src/renderer/src/pages/settings/WebSearchSettings/WebSearchProviderSetting.tsx:151canSetAsDefault

    Suggested: have isWebSearchEnabled return boolean | 'unknown' (or expose isCacheReady() 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 of 93bcfca5.

  • N5 (partial) Regression tests for the original review bugs:

    • isWebSearchEnabled apiKey: undefined / '' / ' ' / 'k' matrix — still no test. The Boolean(provider.apiKey?.trim()) fix has zero guardrail; any future refactor can silently re-introduce undefined?.trim() !== '' === true.
    • BochaProvider / QueritProvider request-shape — current tests assert that state reaches the engine, but not that searchWithTime: true maps to freshness: 'oneDay' (Bocha) or filters.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,134
    • CompressionSettings/index.tsx:21
    • CompressionSettings/CutoffSettings.tsx:16,20
    • WebSearchQuickPanelManager.tsx:80,82
    • WebSearchButton.tsx:22
    • BlacklistSettings.tsx:218

    Suggested: a small safeSetPreference helper that toasts on rejection, then replace void setX(...) at those eight sites. One-time fix, eliminates an entire class of silent failures.

  • I7 WebSearchState.overwrite zombie — WebSearchService.ts:166-168 isOverwriteEnabled() is @deprecated with no callers, and overwrite: false at :587 is 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 // 处理 summarize
    • WebSearchService.ts:441 // 重置状态
    • WebSearchTool.ts:88 // 检查是否需要搜索

III. Can split into follow-up PR

  • I3 filterSupportedWebSearchProviders silent drop — src/renderer/src/config/webSearchProviders.ts:142-144 still no logger.warn when a legacy id is filtered out.
  • I4 WebSearchProviderSettings.tsx:17-18 redirects unsupported provider ids via void navigate(...) with no toast — same silent-failure pattern.
  • I5 Factory default:DefaultProvider.search() throws Method not implemented. with no logger context: WebSearchProviderFactory.ts:30-31, DefaultProvider.ts:6-7. At minimum, logger.error the unsupported id at construction; in dev, throw at construction so the bug is visible.
  • T3 useWebSearchProviders.ts four hooks — still zero direct test coverage.
  • Y2 WebSearchProviderId declared in both packages/shared/data/preference/preferenceTypes.ts:137 and src/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 WebSearchProvider block at preferenceTypes.ts:160 — zero importers, parallel to the renderer/main types.

Medium suggestions still standing

  • Migration 'rag' → 'none' logger.warn at PreferenceTransformers.ts:262 does not include the discarded embeddingModel / rerankModel / documentCount / embeddingDimensions. Once migration runs, what was lost is unrecoverable from logs.
  • flattenCompressionConfig: legacy cutoffLimit: null (semantically "unlimited") is silently coerced to 2000 with no warn. Worth either a logger.warn or an entry in the breaking-changes doc.
  • BlacklistSettings.tsx:213-224 try/catch wraps a synchronous block that fires void setSubscribeSources(...) — the catch will never see an IPC rejection.
  • ComplexPreferenceMappings.test.ts:121 expect(keys.length).toBe(31) is a magic number that breaks every time v2 adds a mapping — prefer toBeGreaterThanOrEqual(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>
@kangfenmao kangfenmao requested a review from 0xfullex May 6, 2026 05:11
Signed-off-by: kangfenmao <kangfenmao@qq.com>
@kangfenmao
Copy link
Copy Markdown
Collaborator Author

@0xfullex 继续麻烦帮忙复审一下。

这轮根据你在 review 里的剩余意见,我把需要在当前 PR 收口的部分又补了一次,当前 head 是 f6d7c20dcf61540841963bf17a7db5a7bbb86927

已修复 / 本 PR 已收口

  • N4 cold cache 三态问题WebSearchService.isWebSearchEnabled() 现在在 preference cache 未 ready 时返回 unknown,不再把“缓存未加载”和“provider 已加载但未配置”都压成 false
  • N4 调用点同步处理:Quick Panel / Basic Settings / Provider Setting 都已识别 unknown,避免冷缓存阶段显示误导性不可用状态。
  • cold cache runtime search 配置:搜索执行路径改用 async preference state,避免第一轮搜索在缓存未 ready 时使用默认 searchWithTime / compression 配置。
  • Bocha / Querit request-shape 测试:新增断言 searchWithTime=true 时 Bocha 请求包含 freshness: "oneDay",Querit 请求包含 filters.timeRange={ date: "d1" }
  • isWebSearchEnabled 矩阵测试:补了 cold cache unknown,并覆盖 API-key provider 空 key / 空白 key / 有效 key、Searxng host、Exa-MCP preset host 等矩阵。
  • I2 偏好写入失败处理:Web Search Settings 侧的 preference 写入已集中走 safe helper,失败会 logger + toast;Blacklist 删除订阅也改为 await + try/catch。
  • Zhipu provider API key 同步到 web-search preferenceProviderSetting 里这处异步写入现在也会捕获失败并提示。
  • I7 zombie state:renderer WebSearchState.overwriteisOverwriteEnabled() 已删除。
  • I9 点名 WHAT 注释:review 中列出的几处中文 WHAT-style 注释已清掉/改掉。
  • provider missing model output:WebSearchTool 在 provider 不可用时会返回显式 unavailable result,不再让模型误解为没有搜索结果。
  • unsupported provider 可观测性
    • migration 丢弃 unsupported legacy default/provider 时已有 logger.warn
    • renderer filterSupportedWebSearchProviders 过滤 unsupported provider 时新增一次性 logger.warn
    • unsupported web-search provider settings route 现在会 warn + toast,并捕获 redirect 失败。
    • WebSearchProviderFactory 遇到未知 provider id 时会先 logger.error,再 fallback 到 DefaultProvider
  • partial_failure 类型/UI:renderer 现在复用 shared WebSearchPhaseCitationBlock 已处理 partial_failure
  • 文档描述search_with_time 的 target-key 描述已改成 textual hint,避免暗示 provider-side time filtering。

暂未修改 / 建议 follow-up 的部分

这些我没有继续塞进当前 PR,主要是因为不是这次阻塞门槛,且会扩大改动面:

  • useWebSearchProviders 四个 hook 的 hook-level 测试:当前已有关键行为测试覆盖 cold cache、provider availability、Bocha/Querit request shape、partial failure;hook 级测试更适合单独补,避免这个 PR 继续变大。
  • WebSearchProviderId 去重:涉及 shared/renderer 类型边界整理,属于类型设计 follow-up,不影响当前 runtime 修复。
  • shared preferenceTypes.ts 里 unused WebSearchProvider interface 删除:是清理项,不影响当前行为。
  • migration RAG 丢弃日志进一步带出 embedding/rerank/documentCount 等细节:当前已经有 downgrade warn;更详细的审计日志可以后续单独增强。
  • legacy cutoffLimit: null warn / breaking-change 说明:属于迁移语义说明增强,不影响当前 web-search provider 移除修复。
  • ComplexPreferenceMappings.test.ts magic number:测试维护性问题,建议单独清理。
  • QuickPanel / WebSearchButton 的 void updateWebSearchProvider(...):这两处是 Redux assistant update,不是 PreferenceService IPC 写入;不属于 I2 中“偏好写入 rejection 被吞”的同类问题,所以这轮没有改。

验证

本地已跑:

  • pnpm test:renderer -- src/renderer/src/config/__tests__/webSearchProviders.test.ts src/renderer/src/providers/WebSearchProvider/__tests__/WebSearchProviderFactory.test.ts src/renderer/src/services/__tests__/webSearchPreferences.test.ts src/renderer/src/providers/WebSearchProvider/__tests__/searchWithTime.test.ts
  • pnpm format
  • pnpm build:check

推送后 GitHub checks 也已全绿:basic-checks / general-test / render-test / changeset-check / comment-legacy-css-vars 均 pass。

Copy link
Copy Markdown
Member

@0xfullex 0xfullex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 isWebSearchEnabled three-state — WebSearchService.ts:34,133-156 introduces WebSearchAvailability = 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 === true for canSetAsDefault).
  • N5 Regression tests — webSearchPreferences.test.ts:285-344 covers cold-cache null / 'unknown' and the it.each matrix for empty / blank / valid API key, plus host-required and preset-host paths. New searchWithTime.test.ts asserts Bocha body contains freshness: 'oneDay' and Querit body contains filters.timeRange={ date: 'd1' }. WebSearchProviderFactory.test.ts locks in the unsupported-id logger.error path.
  • I2 void setPreference(...) swallowing — useWebSearchProviders.ts:20-27 introduces safeSetWebSearchPreference(action, update) (try/await/catch with logger.error + window.toast.error), and the nine surrounding hook entry points all route through it. Keeping void setX(...) at the call sites is the right call — error handling is internalised in the hook. ProviderSetting.tsx:141-142 Zhipu sync now .catch-logs, and BlacklistSettings.tsx:213-227 switches to async + await setSubscribeSources + catch + toast (also fixes the prior medium item where the try/catch wrapped a synchronous block).
  • I7 Zombie overwrite removed — isOverwriteEnabled() deleted from WebSearchService.ts, overwrite: false no longer constructed in buildRendererWebSearchState, and WebSearchState in src/renderer/src/types/websearch.ts is 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-162 adds warnedUnsupportedProviderIds: Set<string> for one-time logger.warn per filtered id.
    • WebSearchProviderSettings.tsx:20-29 adds warn + toast + redirect-failure catch.
    • WebSearchProviderFactory.ts:14,34 adds logger.error before the DefaultProvider fallback.

Worth calling out

  • The decision to put safeSetWebSearchPreference inside 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 !== false filter at WebSearchQuickPanelManager.tsx:151 correctly distinguishes "still loading" (kept, shown disabled with loading copy) from "known disabled" (excluded) — exactly the semantic the three-state return enables.
  • The renderer processWebsearch rewrite (collect rejections, rethrow only AbortError, emit partial_failure when successfulCount > 0 && failed > 0) now mirrors the main-process implementation. End-to-end consistency for the partial_failure UI 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:

  1. useWebSearchProviders.ts four hooks — direct hook-level tests (current coverage is behavioural via webSearchPreferences.test.ts, which is sufficient but not granular).
  2. WebSearchProviderId declared in both packages/shared/data/preference/preferenceTypes.ts:137 and src/renderer/src/types/index.ts:717 — type design dedup.
  3. Unused interface WebSearchProvider block at packages/shared/data/preference/preferenceTypes.ts:160 — zero importers.
  4. Migration 'rag' → 'none' logger.warn could include the discarded embeddingModel / rerankModel / documentCount / embeddingDimensions so post-migration audit can recover the lost configuration.
  5. flattenCompressionConfig: legacy cutoffLimit: null (semantically "unlimited") silently coerced to 2000 — at minimum a logger.warn, ideally a line in the breaking-changes doc since this is a behaviour change for a small but real cohort.
  6. ComplexPreferenceMappings.test.ts:121 expect(keys.length).toBe(31) — prefer toBeGreaterThanOrEqual(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.

@0xfullex 0xfullex merged commit 60efa15 into v2 May 6, 2026
8 of 9 checks passed
@0xfullex 0xfullex deleted the v2-remove-local-websearch branch May 6, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants