From 0beb6e65afa4b1532a63856d766b302cd1f078c8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 11 Jul 2026 23:19:13 -0400 Subject: [PATCH] =?UTF-8?q?web:=20address=20#1670=20review=20=E2=80=94=20o?= =?UTF-8?q?bservable=20deep-link=20failures=20+=20correct=20stale-config?= =?UTF-8?q?=20connect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The ensure-phase addServer catch now records non-409 failures (read-only catalog, backend 5xx) on data-error-message instead of swallowing every error and leaving the deep link permanently stuck at the guard with no signal (review finding #1). - Split the update and connect phases across renders (new deepLinkUpdateRef): connect only once servers reflects the updated config, so onToggleConnection (which reads serversRef, synced by an earlier passive effect) builds the client from the fresh transport rather than the pre-update config (finding #2 — the exact case deepLinkConfigEquals was added for). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --- clients/web/src/App.tsx | 75 ++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 5eb7f760..1511ce27 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1381,6 +1381,7 @@ function App() { return [parsed, deepLinkParseStatus(search, parsed)]; }, []); const deepLinkEnsureRef = useRef(false); + const deepLinkUpdateRef = useRef(false); const deepLinkConnectRef = useRef(false); const showReAuthBanner = useCallback( @@ -2518,15 +2519,20 @@ function App() { }, [inspectorClient, finalizeExplicitDisconnect]); // Deep-link auto-connect (the URL-driven case of #1183). `useServers` - // hydrates asynchronously (initial `servers` is `[]`), so this effect runs - // in two phases: first render attempts a one-shot `addServer` (a 409 means - // the row already exists on disk and hydration will surface it); a later - // render where `servers` actually contains the deep-link row then updates the - // URL if it has changed and connects. Connecting in the same closure as - // `addServer` would resolve against a stale (empty) `servers` and silently - // no-op. The OAuth callback path takes precedence; a deep link on - // `/oauth/callback` would be a misconfiguration, and the callback handler - // clears the URL. + // hydrates asynchronously (initial `servers` is `[]`), so this effect runs in + // discrete phases keyed on what `servers` currently reflects, one per render: + // 1. ensure — no row yet: one-shot `addServer`. + // 2. update — row present but its persisted config differs from the deep + // link (a stale transport/url from an earlier load under the stable + // `deep-link` id): `updateServer`, then return so the effect re-runs. + // 3. connect — row present AND its config already matches: connect. + // Splitting update and connect across renders (rather than awaiting both in + // one closure) is what makes the connect correct: `onToggleConnection` reads + // the target from `serversRef`, which an earlier passive effect syncs from + // `servers` — so connecting only once `servers` reflects the updated config + // guarantees the client is built from the fresh transport, not the stale one. + // The OAuth callback path takes precedence; a deep link on `/oauth/callback` + // would be a misconfiguration, and the callback handler clears the URL. useEffect(() => { if (!deepLink) return; if (window.location.pathname === OAUTH_CALLBACK_PATH) return; @@ -2535,33 +2541,42 @@ function App() { if (!existing) { if (deepLinkEnsureRef.current) return; deepLinkEnsureRef.current = true; - void addServer(deepLink.serverId, deepLink.serverConfig).catch(() => { - // 409 = already on disk; hydration will surface it on a later render. + void addServer(deepLink.serverId, deepLink.serverConfig).catch((err) => { + const message = err instanceof Error ? err.message : String(err); + // A 409 ("already exists") means the row is on disk and hydration will + // surface it on a later render, so the connect phase still proceeds — + // swallow it. Any other failure (read-only catalog, backend 5xx) would + // otherwise leave the deep link permanently stuck at this guard with no + // signal, so record it on the machine-readable error surface. + if (!message.includes("already exists")) recordConnectError(message); + }); + return; + } + + if (!deepLinkConfigEquals(existing.config, deepLink.serverConfig)) { + if (deepLinkUpdateRef.current) return; + deepLinkUpdateRef.current = true; + void updateServer( + deepLink.serverId, + deepLink.serverId, + deepLink.serverConfig, + ).catch((err) => { + const message = err instanceof Error ? err.message : String(err); + recordConnectError(message); }); return; } if (deepLinkConnectRef.current) return; deepLinkConnectRef.current = true; - void (async () => { - if (!deepLinkConfigEquals(existing.config, deepLink.serverConfig)) { - await updateServer( - deepLink.serverId, - deepLink.serverId, - deepLink.serverConfig, - ); - } - if (activeServerId !== deepLink.serverId) { - await onToggleConnection(deepLink.serverId); - } - })().catch((err) => { - // Surface deep-link automation failures (updateServer 5xx, connect throw) - // on the machine-readable error attribute instead of dropping them. The - // toast still fires from inside `onToggleConnection` for the common - // cases; this catch covers the rest. - const message = err instanceof Error ? err.message : String(err); - recordConnectError(message); - }); + if (activeServerId !== deepLink.serverId) { + void onToggleConnection(deepLink.serverId).catch((err) => { + // The toast fires from inside `onToggleConnection` for the common + // cases; this catch covers the rest (surfaced on `data-error-message`). + const message = err instanceof Error ? err.message : String(err); + recordConnectError(message); + }); + } }, [ deepLink, servers,