Conversation
Signed-off-by: HUANG <15866338256@163.com>
Signed-off-by: HUANG <15866338256@163.com>
Signed-off-by: HUANG <15866338256@163.com>
Contributor
There was a problem hiding this comment.
Findings
- [Nit] The enable/disable toggle wipes an unsaved API key.
save()unconditionally runssetApiKey('')before it inspects the patch, so the switch handleronChange={(event) => void save({ enabled: event.target.checked })}clears any pasted-but-unsaved key. Users who paste a key and then flip the switch lose the input and must re-paste. Evidence:apps/desktop/src/renderer/src/components/settings/WebSearchTab.tsx:50(setApiKey('')at the top ofsave) and the switch atapps/desktop/src/renderer/src/components/settings/WebSearchTab.tsx:141.
Suggested fix / next action: only clear the transient field when the patch actually touches the credential, e.g.The existing key-clearing test still passes because it submitsif (input.apiKey !== undefined || input.clearKey === true) setApiKey('');
{ apiKey }/{ clearKey: true }.
Questions
apps/desktop/src/main/provider-settings.ts:117now skips every secret whose id is neither incfg.providersnor a supported onboarding provider (if (entry === null) continue;). That is exactly the intended Tavily case, but it also changes the previously documented "union of provider entries and stored secrets" behavior whose stated goal was to avoid silently dropping rows (apps/desktop/src/main/provider-settings.ts:104-113). Is there any supported config path that persists a model credential whose id is neither a stored provider entry nor a built-in onboarding provider (for example an older/custom provider whoseprovidersentry was lost)? If not, this is fine and can be ignored. If yes, consider an explicit search-only secret key (tavily) rather than allow-listing by entry, so legacy model credentials still surface in Models → API Services even if only readable.
Summary
- Review mode: initial
- Scope looks correct and self-consistent: versioned
settings:v1:get/save/test-web-searchchannels, strict zod contracts inpackages/shared/src/web-search-settings.ts, a preload bridge that never accepts credentials/authorization on test, and a renderer tab that keeps the key as transient UI state and reports sanitized statuses only. - Runtime wiring is sound.
createWebResearchRunsnapshotsenabled/limits and the ciphertext at run start, defers decryption to firstweb_searchuse viagetApiKey, and keepsweb_fetchworking when the Tavily credential is missing/unreadable (apps/desktop/src/main/web-research-run.ts:14-41). Both generation entry points now take the per-run snapshot (apps/desktop/src/main/ipc/generate.ts:1098,apps/desktop/src/main/ipc/generate.ts:1610), preserving the prior "no key decrypt when disabled" behavior. - The probe is bounded and safe: one
POST https://api.tavily.com/search, no redirects with the credential, 1 MiB cap, local timeout distinct from network error, and no budget consumption (apps/desktop/src/main/web-research-network.ts:326-379). HTTP→status mapping matches the linked public Tavily reference (401/429/432/433). - Settings persistence is serialized (
pendingSavechain) so overlapping saves cannot lose the key or switch, errors are surfaced asCodesignErrorrather than silently swallowed, andhasKey/key is never returned over IPC (apps/desktop/src/main/web-search-settings.ts:25-85). - Constraints: no new dependencies (lockfile unchanged), no direct provider-SDK imports, no release/distribution files touched, and a changeset is present covering the user-visible
desktop/shared/i18nchanges. This branch has no linked issue, so there is no completion claim to validate. - Residual risk: the PR body states native Settings-window visual smoke testing was not performed; automated coverage is strong but does not exercise real Electron
safeStorageor an actual network round trip.
Testing
- Not run (automation). Suggested additions if the maintainer wants more coverage: an assertion that the enable/disable toggle does not clear an unsaved key after the fix above, and (if the question above is resolved as a real path) a
toProviderRowscase showing a legacy non-builtin model credential still renders when it has noprovidersentry.
Open-CoDesign Bot
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a bilingual Settings → Web Search entry so users can enable/disable Tavily search and save, replace, clear, or explicitly test their API key without editing configuration files or restarting the app. Reuse the existing config cache, credential store, versioned IPC, and bounded public-network transport.
secretsmap now read when taking the initial run snapshot; the suite's 26 tests pass with no skipped assertions.Request fields, basic-search credit cost, and HTTP mappings were checked against the official Tavily Search reference: 401 authentication, 429 rate limit, 432/433 usage limits; other responses remain service/unknown failures.
Type of change
Linked issue
No linked issue. This is a focused settings follow-up to the existing web-research implementation.
Checklist
pnpm -r typecheck,pnpm lint, andpnpm test(no hooks bypassed)Validation
Design principles
Screenshots / recordings
Not captured in this environment. UI interaction, navigation, locale coverage, and safe bridge behavior are covered by automated tests; native visual review remains pending.