diff --git a/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx b/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx index d5a6d76d..c6f60eff 100644 --- a/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx +++ b/frontend/src/app/pages/Settings/sections/subscription/SubscriptionCards.tsx @@ -17,13 +17,7 @@ import { API_BASE } from '@/shared/config'; import { SUBSCRIPTION_PROVIDERS } from './subscriptionProviders'; import SubscriptionCard from './SubscriptionCard'; import { runConnectFlow } from './subscriptionConnect'; - -/** What POST /agents/subscriptions/disconnect answers; `ok` is the backend's verified end state, never a guess. */ -interface DisconnectResponse { - ok?: boolean; - removed?: number; - error?: string; -} +import { performDisconnect } from './subscriptionDisconnect'; function isProviderActive(connections: SubscriptionConnection[], providerId: string): boolean { return connections.some((p) => p.provider === providerId && (p.isActive || p.testStatus === 'active')); @@ -117,32 +111,24 @@ const SubscriptionCards: React.FC = () => { return; } const data = await r.json(); - runConnectFlow({ providerId, data, setConnecting, setUserCode, setPollTimer, fetchStatus, refreshPickerModels, markConnected }); + runConnectFlow({ providerId, data, setConnecting, setUserCode, setPollTimer, fetchStatus, refreshPickerModels, markConnected, setConnectError }); } catch { setConnecting(null); } }; const handleDisconnect = async (providerId: string) => { if (disconnecting) return; setConfirmingDisconnect(null); - setDisconnectError(null); - setDisconnecting(providerId); // No settle delay: the backend only answers ok after re-reading 9Router, so the lane is already gone. - try { - const r = await fetch(`${API_BASE}/agents/subscriptions/disconnect`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ provider: providerId }), - }); - const data = (await r.json().catch(() => ({}))) as DisconnectResponse; - if (!r.ok || !data.ok) { - setDisconnectError({ provider: providerId, message: data.error || 'Could not disconnect. Please try again.' }); - } - } catch { - setDisconnectError({ provider: providerId, message: 'Could not reach OpenSwarm. Please try again.' }); - } - await fetchStatus(); - refreshPickerModels(); - setDisconnecting(null); + // Body lives in subscriptionDisconnect.ts so the "spinner always releases" invariant is reachable + // by the test runner, which has no DOM on purpose. + await performDisconnect({ + providerId, + apiBase: API_BASE, + fetchStatus, + refreshPickerModels, + setDisconnectError, + setDisconnecting, + }); }; // 4s safety-net poller while connecting; clears Connecting state whenever 9Router reports the provider isActive (handles Windows postMessage failures). diff --git a/frontend/src/app/pages/Settings/sections/subscription/subscriptionConnect.ts b/frontend/src/app/pages/Settings/sections/subscription/subscriptionConnect.ts index a5fcf6e4..7a05715a 100644 --- a/frontend/src/app/pages/Settings/sections/subscription/subscriptionConnect.ts +++ b/frontend/src/app/pages/Settings/sections/subscription/subscriptionConnect.ts @@ -1,5 +1,7 @@ import { API_BASE } from '@/shared/config'; +// A timeout that only clears the spinner leaves the user with nothing to read. +const TIMED_OUT = 'Connecting timed out. The sign-in window may have been closed or never finished. Click Connect to try again.'; interface ConnectCtx { providerId: string; data: any; @@ -9,6 +11,7 @@ interface ConnectCtx { fetchStatus: (opts?: { preserveTransient?: boolean }) => Promise; refreshPickerModels: () => void; markConnected: (provider: string) => void; + setConnectError?: (v: string | null) => void; } // Device-code OAuth flow: popup + dual poller (device-code + status) + focus-listener safety net + 5min hard timeout. @@ -115,7 +118,7 @@ function runDeviceCodeFlow(ctx: ConnectCtx) { if (!stopped) window.addEventListener('focus', onFocus); }, 2000); - // 5-minute hard timeout; cleans up everything. + // 5-minute hard timeout; cleans up everything AND says so. setTimeout(() => { if (stopped) return; stopped = true; @@ -123,8 +126,7 @@ function runDeviceCodeFlow(ctx: ConnectCtx) { clearInterval(devicePollTimer); clearInterval(statusPollTimer); setPollTimer(null); - setConnecting(null); - setUserCode(''); + setConnecting(null); setUserCode(''); ctx.setConnectError?.(TIMED_OUT); if (devicePopup && !devicePopup.closed) { try { devicePopup.close(); } catch {} } @@ -280,7 +282,7 @@ function runAuthCodeFlow(ctx: ConnectCtx) { if (ipcUnsub) ipcUnsub(); window.removeEventListener('blur', onBlur); window.removeEventListener('focus', onFocus); - setConnecting(null); + setConnecting(null); ctx.setConnectError?.(TIMED_OUT); }, timeoutMs); } @@ -291,6 +293,6 @@ export function runConnectFlow(ctx: ConnectCtx) { } else if (ctx.data.flow === 'authorization_code') { runAuthCodeFlow(ctx); } else { - ctx.setConnecting(null); + ctx.setConnecting(null); ctx.setConnectError?.('This provider could not start a sign-in. Please try again.'); } } diff --git a/frontend/src/app/pages/Settings/sections/subscription/subscriptionDisconnect.test.ts b/frontend/src/app/pages/Settings/sections/subscription/subscriptionDisconnect.test.ts new file mode 100644 index 00000000..ea87680e --- /dev/null +++ b/frontend/src/app/pages/Settings/sections/subscription/subscriptionDisconnect.test.ts @@ -0,0 +1,94 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { performDisconnect, type DisconnectCtx } from './subscriptionDisconnect'; + +// The spinner is the whole subject. Every case below asserts it ended, because the reported bug is +// not "disconnect failed" -- it is a row that spins forever after the disconnect already worked. + +function harness(over: Partial = {}) { + const calls: { spinner: (string | null)[]; errors: unknown[]; refreshed: number } = { + spinner: [], errors: [], refreshed: 0, + }; + const ctx: DisconnectCtx = { + providerId: 'anthropic', + apiBase: 'http://x', + fetchStatus: async () => ({}), + refreshPickerModels: () => { calls.refreshed++; }, + setDisconnectError: (e) => { calls.errors.push(e); }, + setDisconnecting: (v) => { calls.spinner.push(v); }, + fetchImpl: (async () => ({ ok: true, json: async () => ({ ok: true }) })) as unknown as typeof fetch, + ...over, + }; + return { ctx, calls }; +} + +const spinnerEnded = (calls: { spinner: (string | null)[] }) => + calls.spinner.length >= 2 && calls.spinner[calls.spinner.length - 1] === null; + +test('happy path: spinner starts and ends', async () => { + const { ctx, calls } = harness(); + await performDisconnect(ctx); + assert.equal(calls.spinner[0], 'anthropic', 'precondition: the spinner actually started'); + assert.ok(spinnerEnded(calls)); + assert.equal(calls.refreshed, 1); +}); + +test('THE BUG: a throwing status refresh must still release the spinner', async () => { + // This is the exact shape that wedged the row: fetchStatus() unwraps a thunk, and a rejected + // thunk throws. Before the fix the rejection escaped past setDisconnecting(null). + const { ctx, calls } = harness({ + fetchStatus: async () => { throw new Error('Rejected'); }, + }); + await performDisconnect(ctx); + assert.ok(spinnerEnded(calls), 'the row must not spin forever when the refresh fails'); +}); + +test('a throwing model-picker refresh must still release the spinner', async () => { + const { ctx, calls } = harness({ + refreshPickerModels: () => { throw new Error('dispatch blew up'); }, + }); + await performDisconnect(ctx); + assert.ok(spinnerEnded(calls)); +}); + +test('network failure reports a reason AND releases the spinner', async () => { + const { ctx, calls } = harness({ + fetchImpl: (async () => { throw new Error('offline'); }) as unknown as typeof fetch, + }); + await performDisconnect(ctx); + assert.ok(spinnerEnded(calls)); + const last = calls.errors[calls.errors.length - 1] as { message: string }; + assert.match(last.message, /Could not reach OpenSwarm/); +}); + +test('a backend refusal reports the backend reason AND releases the spinner', async () => { + const { ctx, calls } = harness({ + fetchImpl: (async () => ({ + ok: false, json: async () => ({ ok: false, error: 'lane is busy' }), + })) as unknown as typeof fetch, + }); + await performDisconnect(ctx); + assert.ok(spinnerEnded(calls)); + const last = calls.errors[calls.errors.length - 1] as { message: string }; + assert.equal(last.message, 'lane is busy'); +}); + +test('unparseable body still yields a readable message, never a blank card', async () => { + const { ctx, calls } = harness({ + fetchImpl: (async () => ({ + ok: false, json: async () => { throw new Error('not json'); }, + })) as unknown as typeof fetch, + }); + await performDisconnect(ctx); + assert.ok(spinnerEnded(calls)); + const last = calls.errors[calls.errors.length - 1] as { message: string }; + assert.match(last.message, /Could not disconnect/); +}); + +test('NEGATIVE CONTROL: the harness can observe a stuck spinner', async () => { + // Without this, every assertion above could be passing because spinnerEnded() is simply always + // true -- the vacuous green VERIFICATION.md section 3 warns about. Prove the detector can fail. + const calls = { spinner: ['anthropic'] as (string | null)[] }; + assert.equal(spinnerEnded(calls), false, 'the check must be able to catch a wedged spinner'); +}); diff --git a/frontend/src/app/pages/Settings/sections/subscription/subscriptionDisconnect.ts b/frontend/src/app/pages/Settings/sections/subscription/subscriptionDisconnect.ts new file mode 100644 index 00000000..fb0b0711 --- /dev/null +++ b/frontend/src/app/pages/Settings/sections/subscription/subscriptionDisconnect.ts @@ -0,0 +1,68 @@ +/** + * Disconnect, extracted from SubscriptionCards so the one invariant that matters is testable: + * the spinner is ALWAYS released, whatever any step does. + * + * Inline, the refresh that follows the disconnect sat outside the try. fetchStatus() unwraps a + * redux thunk, so a rejected refresh throws, and that throw sailed straight past + * setDisconnecting(null): the row spun forever with no exit but closing Settings, while the + * disconnect it was reporting on had usually already succeeded. Ending a spinner belongs in a + * finally, never on the happy path. It lives here rather than in the component because the test + * runner deliberately has no DOM (see scripts/run-tests.mjs). + */ + +export interface DisconnectResponse { + ok?: boolean; + error?: string; +} + +export interface DisconnectCtx { + providerId: string; + apiBase: string; + fetchStatus: () => Promise; + refreshPickerModels: () => void; + setDisconnectError: (e: { provider: string; message: string } | null) => void; + setDisconnecting: (v: string | null) => void; + // Injected so the test can drive real failure shapes without a DOM or a network. + fetchImpl?: typeof fetch; +} + +export async function performDisconnect(ctx: DisconnectCtx): Promise { + const { + providerId, apiBase, fetchStatus, refreshPickerModels, + setDisconnectError, setDisconnecting, + } = ctx; + const doFetch = ctx.fetchImpl ?? fetch; + + setDisconnectError(null); + setDisconnecting(providerId); + try { + try { + const r = await doFetch(`${apiBase}/agents/subscriptions/disconnect`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ provider: providerId }), + }); + const data = (await r.json().catch(() => ({}))) as DisconnectResponse; + if (!r.ok || !data.ok) { + setDisconnectError({ + provider: providerId, + message: data.error || 'Could not disconnect. Please try again.', + }); + } + } catch { + setDisconnectError({ + provider: providerId, + message: 'Could not reach OpenSwarm. Please try again.', + }); + } + try { + // A stale row self-corrects on the next poll; a stuck spinner never does, so this is best-effort. + await fetchStatus(); + refreshPickerModels(); + } catch { + // Swallowed on purpose: a failed refresh must not decide whether the spinner ends. + } + } finally { + setDisconnecting(null); + } +}