refactor(desktop): unify validate+test into single ConnectionCheck by hqhq1025 · Pull Request #38 · OpenCoworkAI/open-codesign

hqhq1025 · 2026-04-18T17:24:38Z

Problem

The onboarding PasteKey component had two parallel, inconsistent validation flows:

Auto-validate on key change (via onboarding.validateKey → pingProvider in packages/providers/validate.ts): built URLs by naively appending /v1/models to whatever baseUrl the user typed — no normalization.
Test button (via connection:v1:test in connection-ipc.ts): used normalizeBaseUrl + provider-aware path construction before hitting the endpoint.

Result: with an OpenAI-compat proxy at https://host/v1, auto-validate would hit https://host/v1/v1/models (404) while the Test button correctly hit https://host/v1/models (200). The two flows disagreed on the same inputs, producing confusing UX and silent failures.

Solution

Delete the split-state architecture. Replace ValidationState + ConnectionState with a single unified type:

```ts
type ConnectionCheck =
| { status: 'idle' }
| { status: 'testing' }
| { status: 'ok' }
| { status: 'failed'; code: ErrorCode; hint: string };
```

What changed

Removed the ValidationState type and the auto-validate useEffect that called onboarding.validateKey / pingProvider.
Kept provider detection (lightweight prefix-match IPC, no network auth, 300ms debounce).
Test button is now the ONLY authority. It always calls connection:v1:test which uses normalizeBaseUrl + correct per-provider path logic.
Default base URL fallback: when no custom baseUrl is entered, the renderer falls back to the same defaults as buildDefaultBaseUrl in connection-ipc.ts (https://api.anthropic.com, https://api.openai.com/v1, https://openrouter.ai/api/v1). The Test button works for the official endpoint without requiring the advanced section.
Continue button gated on connectionCheck.status === 'ok' with tooltip hint "Run Test to verify your connection first".
onboarding.validateKey / providers.validate IPC no longer called from PasteKey (the IPC handler still exists for Settings usage).

Deleted paths

Removed	Replaced by
`ValidationState` (idle/detecting/validating/ok/error)	`ConnectionCheck` (idle/testing/ok/failed)
`connState` (ConnectionState)	merged into `ConnectionCheck`
`useEffect` → `onboarding.validateKey` call	Test button → `connection.test`
Auto-trigger on key change/blur	Explicit user action only

Principles check

Compatibility ✅ — IPC contract unchanged; onboarding:validate-key still registered
Upgradeability ✅ — simpler state, easier to extend
No bloat ✅ — removed ~80 lines of duplicate logic
Elegance ✅ — single source of truth for connection status

Test plan

pnpm -r typecheck passes
pnpm lint — no new errors in changed files
pnpm test — 104/104 tests pass including new PasteKey.test.ts cases covering default-baseUrl fallback, 401/ECONNREFUSED errors, and Continue gate
Manual: paste Anthropic key → detect shows "Recognized: Anthropic Claude" → click Test → success → Continue enabled
Manual: paste key with custom proxy URL https://host/v1 → Test hits correct endpoint → no double /v1
Manual: Continue button is disabled before Test runs; tooltip shown

…/ segment When a user provides a custom baseUrl ending with /v1 (e.g. DuckCoding relay), the validate endpoint function was constructing https://proxy/v1/v1/models instead of https://proxy/v1/models. The connection:v1:test IPC used normalizeBaseUrl() and hit the correct URL, so validate showed a network error while Test showed Connected. Add normalizeValidateBaseUrl() that strips trailing slash and /v1 suffix before appending /v1/models, matching the behaviour of connection-ipc.ts. Also: update onboarding-ipc.test.ts to capture handler closures so we can assert that the IPC handler forwards baseUrl to pingProvider. Signed-off-by: hqhq1025 <[email protected]>

+ * - anthropic: strip trailing /v1 — we append /v1/models below
+ */
+function normalizeValidateBaseUrl(baseUrl: string): string {
+  return baseUrl.replace(/\/+$/, '').replace(/\/v1$/, '');


github-actions

Findings

None.

Summary

Review mode: initial
No diff-attributable issues found in added/modified lines.
Constraint checks in touched files: no direct provider SDK imports, no new dependencies/licenses, no UI token violations in scope, no newly introduced silent fallback behavior detected.
Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.

Testing

Not run (automation)

open-codesign Bot

Remove the dual-state anti-pattern where `ValidationState` (from `onboarding.validateKey` / `pingProvider`) and `ConnectionState` (from `connection.test`) ran in parallel with inconsistent URL-building logic. Before: key paste triggered an auto-validate useEffect that called `pingProvider` (packages/providers/validate.ts) which built URLs by naively appending `/v1/models` to the raw baseUrl. Separately, the Test button called `connection.test` which uses `normalizeBaseUrl` + provider- aware path construction. The two paths could disagree on endpoints (e.g. double /v1 for OpenAI-compat proxies) and expose conflicting UX. After: - Single `ConnectionCheck` type: idle | testing | ok | failed - Provider detection (lightweight IPC, no network) runs on key change - Test button is the ONLY authority for key + endpoint verification, always using `connection:v1:test` (normalizeBaseUrl path) - When no custom baseUrl is set the renderer falls back to the same default URLs as `buildDefaultBaseUrl` in connection-ipc.ts - Continue button disabled until ConnectionCheck = ok - `ValidationState`, `onboarding.validateKey` call, and the 500ms auto-validate useEffect are fully removed from PasteKey - Test file rewritten to cover the unified ConnectionCheck logic including default-baseUrl fallback behaviour Signed-off-by: hqhq1025 <[email protected]>

github-actions

Findings

[Major] Detection IPC/network errors are silently downgraded to an "unknown key prefix" state, which hides operational failures and violates the no-silent-fallback constraint. In detectProvider, exceptions are caught and mapped to unknown without surfacing error context; the UI then shows an unsupported-prefix message, which is incorrect for IPC/network failures. Evidence: apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:93, apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:95, apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:434.
Suggested fix:

type DetectState =
  | { kind: 'idle' }
  | { kind: 'detecting' }
  | { kind: 'detected'; provider: SupportedOnboardingProvider }
  | { kind: 'unknown' }
  | { kind: 'error'; message: string };

// in detectProvider catch
} catch (err) {
  if (reqId !== reqIdRef.current) return;
  setDetectState({
    kind: 'error',
    message: err instanceof Error ? err.message : 'Provider detection failed.',
  });
  setConnCheck({ status: 'failed', code: 'NETWORK', hint: 'Provider detection failed.' });
  return;
}

Summary

Review mode: follow-up after new commits
1 Major issue found in added/modified lines.
Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.

Testing

Not run (automation)

open-codesign Bot

github-actions · 2026-04-18T17:30:30Z

+    detected = await window.codesign.detectProvider(trimmed);
+  } catch {
+    if (reqId !== reqIdRef.current) return;
+    setDetectState({ kind: 'unknown' });


This catch block suppresses detection failures by converting them to unknown and later rendering an "Unrecognized key prefix" message. That is a silent fallback and a misleading UX for IPC/network failures.

Suggested minimal change:

} catch (err) { if (reqId !== reqIdRef.current) return; setDetectState({ kind: 'error', message: err instanceof Error ? err.message : 'Provider detection failed.', }); return; }

Then render DetectLine for kind: 'error' with the message.

… detectProvider Previously detectProvider's catch block collapsed every failure into { kind: 'unknown' } and the UI rendered an 'unrecognized key prefix' message — silently downgrading IPC crashes and network outages to a user-input problem. This violated the no-silent-fallback principle. - Introduce DetectResult discriminated union: ok | unknown_prefix | ipc_error | network_error. - classifyDetectError inspects the thrown error (TypeError or fetch/ECONN/ENOTFOUND/ETIMEDOUT/EAI_AGAIN tokens) to choose between ipc_error and network_error; defaults to ipc_error since the detect-provider IPC is local-only. - DetectLine renders three distinct error messages so users see the actual failure mode. - Failed IPC/network detection also surfaces in ConnectionCheck so the status row reflects the real fault rather than appearing idle. - Adds 11 vitest cases covering each branch of detectProvider and classifyDetectError.

github-actions

Findings

No issues found in added/modified lines.

Summary

Review mode: follow-up after new commits
No new issues found in the latest diff.
Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.
Residual risk: no Playwright coverage changes were included for the onboarding flow transitions (detect error -> test -> continue gating).

Testing

Not run (automation)

open-codesign Bot

github-advanced-security AI found potential problems Apr 18, 2026

View reviewed changes

Comment thread packages/providers/src/validate.ts

* - anthropic: strip trailing /v1 — we append /v1/models below

*/

function normalizeValidateBaseUrl(baseUrl: string): string {

return baseUrl.replace(/\/+$/, '').replace(/\/v1$/, '');

github-actions Bot reviewed Apr 18, 2026

View reviewed changes

hqhq1025 changed the title ~~fix(providers): API key validate respects custom baseUrl — no more contradictory status~~ refactor(desktop): unify validate+test into single ConnectionCheck Apr 18, 2026

github-actions Bot reviewed Apr 18, 2026

View reviewed changes

hqhq1025 added 2 commits April 19, 2026 01:55

style(desktop): biome formatting for detectProvider fix

988443c

github-actions Bot reviewed Apr 18, 2026

View reviewed changes

hqhq1025 merged commit e847d63 into main Apr 18, 2026
5 of 6 checks passed

hqhq1025 deleted the wt/fix-validate-baseurl branch April 18, 2026 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(desktop): unify validate+test into single ConnectionCheck#38

refactor(desktop): unify validate+test into single ConnectionCheck#38
hqhq1025 merged 4 commits intomainfrom
wt/fix-validate-baseurl

hqhq1025 commented Apr 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot Apr 18, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hqhq1025 commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

What changed

Deleted paths

Principles check

Test plan

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hqhq1025 commented Apr 18, 2026 •

edited

Loading