[eric] settings: the disconnect spinner always releases, even when the refresh throws

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014wtspwSFzZmjCx9UNPAorQ

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).

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<any>;
   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.');
   }
 }

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<DisconnectCtx> = {}) {
+  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');
+});

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<unknown>;
+  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<void> {
+  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);
+  }
+}