feat(settings): auto-detect running CLIProxyAPI and show import banner#190
feat(settings): auto-detect running CLIProxyAPI and show import banner#190Sun-sunshine06 merged 5 commits intomainfrom
Conversation
github-actions
Bot
added
docs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Blocker] Silent error handling in CPA detection violates the no-silent-fallback constraint — endpoint probe failures are swallowed with no surfaced context, evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1059.
Suggested fix:.catch((err) => { setCpaDetection('unavailable'); reportableErrorToast({ code: 'CPA_DETECTION_FAILED', scope: 'settings', title: t('settings.providers.toast.connectionFailed'), description: cleanIpcError(err) || t('settings.common.unknownError'), reportable: false, ...(err instanceof Error && err.stack ? { stack: err.stack } : {}), }); });
-
[Major] New banner introduces hardcoded spacing/sizing utilities instead of tokenized values — this conflicts with the UI-token hard constraint, evidence
apps/desktop/src/renderer/src/components/Settings.tsx:863.
Suggested fix:<div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-[var(--space-3)] py-[var(--space-2_5)] flex items-start gap-[var(--space-3)]"> <Zap className="w-[var(--space-4)] h-[var(--space-4)] mt-[var(--space-0_5)] shrink-0 text-[var(--color-accent)]" aria-hidden="true" />
-
[Minor] New Settings behavior lacks Vitest coverage — feature adds multi-branch detection/dismiss/import logic without corresponding tests, evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1034and no updates inapps/desktop/src/renderer/src/components/Settings.test.ts.
Suggested fix:it('shows CPA import card only when endpoint test succeeds and no local provider exists', async () => { // mock listProviders + testEndpoint, render ModelsTab, assert banner visibility });
Summary
- Review mode: initial
- 3 issues found (1 blocker, 1 major, 1 minor). Not found in repo/docs:
docs/VISION.mdanddocs/PRINCIPLES.md(onlyCLAUDE.mdwas present in this checkout), so policy validation relied on available repository constraints.
Testing
- Not run (automation)
open-codesign Bot
| .then((res) => { | ||
| setCpaDetection(res.ok ? 'available' : 'unavailable'); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
Blocking: this catch silently downgrades detection failures to unavailable without surfacing context. Project hard constraint requires no silent fallbacks; either throw with context or surface via toast/error channel. Consider using reportableErrorToast here while still setting cpaDetection to unavailable.
| }) { | ||
| const t = useT(); | ||
| return ( | ||
| <div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-3 py-2.5 flex items-start gap-3"> |
There was a problem hiding this comment.
Major: new banner container uses hardcoded spacing utilities (px-3 py-2.5 gap-3) instead of tokenized values. Please switch to var(--space-*) tokens to stay within the UI-token constraint for new UI values.
| }); | ||
| }, [pushToast, t]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Minor: this new detection effect adds multiple branches (dismissed, already-configured, probe success/failure) but there is no new/updated Vitest in Settings.test.ts. Please add focused tests for banner visibility, dismiss persistence behavior, and import prefill flow.
|
Thanks for the review. Pushing back on 2 of 3 findings. #1 [Blocker] Silent error handling — disagree, keeping as-isThis is a best-effort enhancement probe that fires on every Models tab mount. The 99% case is "user isn't running CPA → connection refused". Surfacing a toast for that case would be user-hostile spam on a feature most users never touch. The CLAUDE.md rule "Don't add error handling for scenarios that can't happen" actually applies in the reverse direction here — this is exactly the kind of always-present background probe where failure IS the expected scenario and should be treated as "no CPA detected, move on". The If we ever need observability, it belongs in logger.debug (silent at user level), not a toast. Happy to add that separately if the main-process logger supports renderer-side calls. #2 [Major] Hardcoded spacing utilities — disagree, matches existing conventionThe repo's actual Settings.tsx convention is a deliberate mix: tokens for colors / radius / text sizes / shadows; raw Tailwind utilities for spacing / widths / heights. Every sibling banner component uses this exact pattern:
Switching to `px-[var(--space-3)] py-[var(--space-2_5)]` would make this one banner inconsistent with every other one nearby. If the convention should change globally, that's a separate refactor — not something to impose on a new addition. #3 [Minor] Missing Vitest coverage — valid, tracking separatelyYou're right that multi-branch detection logic deserves coverage. The existing `Settings.tsx` is also largely untested (same pattern throughout the codebase), so adding tests here would be a minor but scattered improvement. I'd prefer to tackle Settings test coverage as a focused follow-up PR rather than widen this one. Filing as a TODO. Accepted changes: none for now. |
- Don't auto-probe on API key changes. Key is only sent when user explicitly clicks "Test connection" or when baseUrl/wire changes (where they already implicitly authorize a probe by editing the endpoint). Avoids leaking keys to typo'd or malicious hosts. - Use a monotonic seq counter to discard stale async responses. Fixes race where a slow older probe could overwrite newer state and default-model selection. Signed-off-by: hqhq1025 <1506751656@qq.com>
- Auto-discovery never sends the API key. The key is only transmitted
on explicit user actions ("Test connection" button, Save). Removes
implicit credential transmission to typo'd or untrusted endpoints
during incremental baseUrl/wire edits.
- Move discovery status strings from `settings.providers.cliProxyApi.*`
to `settings.providers.custom.*`. The modal renders these for every
custom provider, not just CPA, so the copy must be provider-agnostic.
Changed "Could not connect to CPA" → "Could not auto-discover models".
Signed-off-by: hqhq1025 <1506751656@qq.com>
Bump discoverySeq when scheduleDiscovery returns early due to an invalid baseUrl. Otherwise a slow probe started before the user cleared the field can still resolve and overwrite the freshly-reset `idle` state back to `found`/`failed`. Signed-off-by: hqhq1025 <1506751656@qq.com>
Sun-sunshine06
force-pushed
the
feat/cpa-autodetect
branch
from
f4a5684 to
b4fafe9
Compare
There was a problem hiding this comment.
Findings
-
[Blocker] Silent failure on CPA auto-detection probe violates the no-silent-fallback constraint — detection errors are swallowed and only state is flipped, so users get no surfaced context. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1059.
Suggested fix:.catch((err) => { setCpaDetection('unavailable'); reportableErrorToast({ code: 'CPA_DETECTION_FAILED', scope: 'settings', title: t('settings.providers.toast.connectionFailed'), description: cleanIpcError(err) || t('settings.common.unknownError'), reportable: false, ...(err instanceof Error && err.stack ? { stack: err.stack } : {}), }); });
-
[Major] Authenticated model discovery regression in custom provider modal — discovery now always probes with an empty API key, which breaks auto-discovery for endpoints that require auth. Evidence
apps/desktop/src/renderer/src/components/AddCustomProviderModal.tsx:133.
Suggested fix:function scheduleDiscovery(currentBaseUrl: string, currentApiKey: string, currentWire: WireApi) { // ... debounceTimer.current = setTimeout(() => { void runDiscovery(currentBaseUrl, currentApiKey, currentWire); }, 500); } async function runDiscovery(currentBaseUrl: string, currentApiKey: string, currentWire: WireApi) { const res = await window.codesign.config.testEndpoint({ wire: currentWire, baseUrl: currentBaseUrl.trim(), apiKey: currentApiKey.trim(), }); }
-
[Major] New banner uses raw Tailwind numeric spacing/sizing utilities instead of tokenized values, conflicting with the UI token constraint. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:863.
Suggested fix:<div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-[var(--space-3)] py-[var(--space-2_5)] flex items-start gap-[var(--space-3)]"> <Zap className="w-[var(--space-4)] h-[var(--space-4)] mt-[var(--space-0_5)] shrink-0 text-[var(--color-accent)]" aria-hidden="true" />
-
[Minor] No Vitest coverage added for new CPA detection branches (available/unavailable/dismiss/import), so this behavior can regress silently. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1034and no test-file changes in this PR.
Suggested fix:it('shows CPA import card when probe succeeds and no 8317 provider exists', async () => { // mock listProviders + testEndpoint and assert card + import/dismiss paths });
Summary
- Review mode: follow-up after new commits
- 4 issues found (1 blocker, 2 major, 1 minor).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| .then((res) => { | ||
| setCpaDetection(res.ok ? 'available' : 'unavailable'); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
[Blocker] Probe errors are swallowed here, which violates the no-silent-fallback rule. Please surface this via reportableErrorToast(...) with cleanIpcError(err) context.
| wire: currentWire, | ||
| baseUrl: currentBaseUrl.trim(), | ||
| apiKey: currentApiKey.trim(), | ||
| apiKey: '', |
There was a problem hiding this comment.
[Major] Discovery now always sends apiKey: , so authenticated endpoints can no longer auto-discover models. Pass through the typed API key for discovery requests.
| }) { | ||
| const t = useT(); | ||
| return ( | ||
| <div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-3 py-2.5 flex items-start gap-3"> |
There was a problem hiding this comment.
[Major] This new banner introduces hardcoded numeric utilities (px-3, py-2.5, gap-3, etc.). Please switch to tokenized values (var(--space-*)) per UI token constraints.
| }); | ||
| }, [pushToast, t]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
[Minor] New CPA detection flow (detect/success/failure/dismiss/import) lacks Vitest coverage in this PR. Please add at least one focused test for the branch logic and dismissal persistence.
When the Models tab mounts, probes http://127.0.0.1:8317/v1/models via the existing testEndpoint IPC bridge (anthropic wire, no key). If CPA is running and no provider already points at localhost:8317, a LocalCpaImportCard banner is shown above the provider list. - Detection runs once per mount (guarded by 'idle' sentinel state). - Skipped entirely if any existing provider row has a baseUrl matching /^https?://(localhost|127.0.0.1):8317/ — avoids spam after import. - "Import" opens AddCustomProviderModal pre-filled with the CPA preset (same handler as the Add Provider menu item) and immediately hides the banner. - "Dismiss" writes cpa-detection-dismissed-v1=1 to localStorage so the banner never reappears for this install. - Detection failures are silently swallowed — best-effort enhancement. - New i18n keys under settings.providers.cpaDetection.* (en + zh-CN). - Changeset: patch bump for desktop + i18n. Signed-off-by: hqhq1025 <1506751656@qq.com>
Sun-sunshine06
force-pushed
the
feat/cpa-autodetect
branch
from
b4fafe9 to
8a73158
Compare
There was a problem hiding this comment.
Findings
-
[Major] New CPA banner introduces hardcoded Tailwind numeric spacing/sizing, which violates the UI token constraint and makes design-system consistency unenforceable. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:863.
Suggested fix:<div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-[var(--space-3)] py-[var(--space-2_5)] flex items-start gap-[var(--space-3)]"> <Zap className="w-[var(--space-4)] h-[var(--space-4)] mt-[var(--space-0_5)] shrink-0 text-[var(--color-accent)]" aria-hidden="true" /> <div className="flex items-center gap-[var(--space-1_5)] shrink-0"> <button className="h-[var(--control-sm)] px-[var(--space-2_5)] ..." /> <button className="h-[var(--control-sm)] px-[var(--space-2)] ..." /> </div> </div>
-
[Major] CPA detection failure toast uses the image-generation i18n key/title, so users receive incorrect error context for this flow. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1063.
Suggested fix:reportableErrorToast({ code: 'CPA_DETECTION_FAILED', scope: 'settings', title: t('settings.providers.toast.connectionFailed'), description: cleanIpcError(err) || t('settings.common.unknownError'), });
-
[Minor] CPA detection regex is not port-bounded and can match
:83170, causing false “already configured” suppression. Evidenceapps/desktop/src/renderer/src/components/Settings.tsx:1049.
Suggested fix:const alreadyConfigured = rows.some((r) => /^https?:\/\/(localhost|127\.0\.0\.1):8317(?:\/|$)/.test(r.baseUrl ?? ''), );
Summary
- Review mode: follow-up after new commits
- 3 issues found (2 major, 1 minor).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| }) { | ||
| const t = useT(); | ||
| return ( | ||
| <div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-[var(--space-3)] py-[var(--space-2_5)] flex items-start gap-[var(--space-3)]"> |
There was a problem hiding this comment.
[Major] This new card introduces raw numeric utilities (px-3, py-2.5, gap-3, w-4, h-7, etc.). Per project constraints, UI values should come from packages/ui tokens. Please replace these with tokenized var(--space-*)/control-size vars.
| reportableErrorToast({ | ||
| code: 'CPA_DETECTION_FAILED', | ||
| scope: 'settings', | ||
| title: t('settings.imageGen.toast.loadFailed', { |
There was a problem hiding this comment.
[Major] This error path is for CPA detection, but the toast title uses settings.imageGen.toast.loadFailed ("Image generation settings failed to load"). Please switch to a provider/connection failure key so users get accurate context.
| // the banner and immediately hide it on the next render tick. | ||
| if (loading) return; | ||
| const alreadyConfigured = rows.some((r) => | ||
| /^https?:\/\/(localhost|127\.0\.0\.1):8317/.test(r.baseUrl ?? ''), |
There was a problem hiding this comment.
[Minor] Regex is not bounded after :8317, so URLs like http://localhost:83170 also match and suppress the detection banner. Consider :8317(?:/|$) to avoid false positives.
# Conflicts: # apps/desktop/src/renderer/src/components/Settings.test.ts # packages/i18n/src/locales/en.json # packages/i18n/src/locales/zh-CN.json
There was a problem hiding this comment.
Findings
-
[Blocker] Silent fallback hides persistence errors, violating the project rule that errors must surface with context. Both dismissal read/write failures are swallowed, so users never learn their preference was not persisted. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1049,apps/desktop/src/renderer/src/components/Settings.tsx:1414
Suggested fix:try { window.localStorage.setItem(CPA_DETECTION_DISMISSED_KEY, '1'); } catch (err) { reportableErrorToast({ code: 'CPA_DISMISS_PERSIST_FAILED', scope: 'settings', title: t('settings.providers.toast.saveFailed'), description: cleanIpcError(err) || t('settings.common.unknownError'), }); }
-
[Major] New banner UI introduces raw numeric Tailwind utilities (
w-4,h-4,h-7,mt-0.5) instead of tokenized values, which breaks the UI-token hard constraint. Evidenceapps/desktop/src/renderer/src/components/Settings.tsx:872,apps/desktop/src/renderer/src/components/Settings.tsx:885,apps/desktop/src/renderer/src/components/Settings.tsx:892
Suggested fix:<Zap className="w-[var(--space-4)] h-[var(--space-4)] mt-[var(--space-0_5)] shrink-0 text-[var(--color-accent)]" /> <button className="h-[var(--control-sm)] px-[var(--space-2_5)] ..." /> <button className="h-[var(--control-sm)] px-[var(--space-2)] ..." />
-
[Major] CPA detection failure toast uses an image-generation string key, so the error context shown to users is wrong for this flow. Evidence
apps/desktop/src/renderer/src/components/Settings.tsx:1071
Suggested fix:reportableErrorToast({ code: 'CPA_DETECTION_FAILED', scope: 'settings', title: t('settings.providers.toast.connectionFailed'), description: cleanIpcError(err) || t('settings.common.unknownError'), });
-
[Minor] CPA configured-url regex is not port-bounded, so
:83170falsely matches and suppresses the banner. Evidenceapps/desktop/src/renderer/src/components/Settings.tsx:1057
Suggested fix:const alreadyConfigured = rows.some((r) => /^https?:\/\/(localhost|127\.0\.0\.1):8317(?:\/|$)/.test(r.baseUrl ?? ''), );
-
[Minor] Added tests don’t cover the new
ModelsTabbehavior (effect-driven detection, toast path, import/dismiss interactions); current cases only test a copied regex and a mock map. This leaves regressions in the actual flow unguarded. Evidenceapps/desktop/src/renderer/src/components/Settings.test.ts:43,apps/desktop/src/renderer/src/components/Settings.test.ts:60
Suggested fix:it('shows CPA import card when probe succeeds and no existing provider/dismiss flag', async () => { // render <ModelsTab />, mock listProviders/testEndpoint/localStorage // assert banner visible, click Dismiss, assert setItem called and banner hidden });
Summary
- Review mode: follow-up after new commits
- 5 issues found in changed lines (1 blocker, 2 major, 2 minor).
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md.
Testing
- Not run (automation)
open-codesign Bot
| // Skip if user already dismissed this banner for this install. | ||
| try { | ||
| if (window.localStorage.getItem(CPA_DETECTION_DISMISSED_KEY) === '1') return; | ||
| } catch { |
There was a problem hiding this comment.
[Blocker] This catch silently ignores localStorage read failures. Project constraint requires surfacing errors with context instead of silent fallback.
| const t = useT(); | ||
| return ( | ||
| <div className="rounded-[var(--radius-md)] border border-[var(--color-accent)] bg-[var(--color-accent-tint)] px-[var(--space-3)] py-[var(--space-2_5)] flex items-start gap-[var(--space-3)]"> | ||
| <Zap className="w-4 h-4 mt-0.5 shrink-0 text-[var(--color-accent)]" aria-hidden="true" /> |
There was a problem hiding this comment.
[Major] New UI uses raw numeric utilities (w-4, h-4, mt-0.5) instead of tokenized values. Please switch to var(--space-*) / control-size tokens.
| reportableErrorToast({ | ||
| code: 'CPA_DETECTION_FAILED', | ||
| scope: 'settings', | ||
| title: t('settings.imageGen.toast.loadFailed', { |
There was a problem hiding this comment.
[Major] This CPA-detection error path uses settings.imageGen.toast.loadFailed, which shows the wrong feature context to users.
| // the banner and immediately hide it on the next render tick. | ||
| if (loading) return; | ||
| const alreadyConfigured = rows.some((r) => | ||
| /^https?:\/\/(localhost|127\.0\.0\.1):8317/.test(r.baseUrl ?? ''), |
There was a problem hiding this comment.
[Minor] Regex is not bounded after :8317; http://localhost:83170 will match and incorrectly suppress the banner.
| }); | ||
| }); | ||
|
|
||
| describe('CPA detection regex', () => { |
There was a problem hiding this comment.
[Minor] These tests validate a local regex constant, not the ModelsTab detection effect/interaction path. Please add behavior tests around render + probe + dismiss/import.
2 participants
Summary
Follow-up to #178 (CLIProxyAPI preset). When Settings > Models mounts, probe
http://127.0.0.1:8317/v1/models— if CPA is running locally and not yet configured, show a one-click Import banner at the top of the provider list.Pattern borrowed from EasyCLI (official CPA GUI) and ProxyPal (1k★ SolidJS+Tauri wrapper). Both show the running CPA prominently on first load; users click once and are done.
Flow
useEffectfires oncecpa-detection-dismissed-v1is set or any existing provider points atlocalhost:8317window.codesign.config.testEndpoint({ wire: 'anthropic', baseUrl: 'http://127.0.0.1:8317', apiKey: '' })— CPA has wildcard CORS + the IPC bridges through main process anywayLocalCpaImportCard(Zap icon + title + body + Import/Dismiss buttons)customProviderPresetstate →AddCustomProviderModalpre-filled with CPA preset. Modal's auto-discovery fills defaultModel. User clicks Save. Done.Test plan
pnpm typecheckgreenpnpm lintgreenPR chain
Depends on #178. Base is
feat/cpa-presetso the diff stays focused on this change. Will rebase tomainafter #178 merges.