From ed2070f67cd5de4119ed1c6d5b97184334529020 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 00:36:02 -0400 Subject: [PATCH 1/3] chore(web): extract App.tsx's connection lifecycle into useConnectionLifecycle Phase 2 step 3 of the App.tsx decomposition (#2154, under #2129/#2126). Moves the whole connection cluster out of App.tsx and into clients/web/src/hooks/useConnectionLifecycle.ts: setupClientForServer (the single place a connection is configured), onToggleConnection, onDisconnect, onReauthenticateFromBanner, the deep-link auto-connect effect and its three phase latches, the connected/teardown/disconnect effects, connectErrorMessage, and the session-scoped reset a disconnect triggers. Handshake telemetry rides along as useHandshakeTelemetry, split out only because useOAuthRecovery runs first and stamps the same ref. No behavior change. App.tsx goes 3,039 -> 2,412 lines. Per #2129, resetSessionScopedUiState takes a narrow SessionResetSurface - one reset() per owner - rather than every phase-1 setter, so the hook does not end up depending on everything phase 1 extracted. The two log-level operations stay App-owned because the connect path seeds the same state. Closes #2154 Signed-off-by: cliffhall --- clients/web/src/App.tsx | 912 ++----------- .../src/hooks/useConnectionLifecycle.test.tsx | 1208 +++++++++++++++++ .../web/src/hooks/useConnectionLifecycle.ts | 968 +++++++++++++ 3 files changed, 2310 insertions(+), 778 deletions(-) create mode 100644 clients/web/src/hooks/useConnectionLifecycle.test.tsx create mode 100644 clients/web/src/hooks/useConnectionLifecycle.ts diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 8b5d849c1..41e49a528 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1,11 +1,4 @@ -import { - useCallback, - useEffect, - useLayoutEffect, - useMemo, - useRef, - useState, -} from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { Box, Text } from "@mantine/core"; import { notifications } from "@mantine/notifications"; import type { @@ -32,24 +25,14 @@ import type { ServerType, } from "@inspector/core/mcp/types.js"; import { - DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS, - eraToVersionNegotiation, resolveModernLogLevel, } from "@inspector/core/mcp/types.js"; import { - applyStdioSettingsToConfig, cleanRoots, - oauthAuthorizationParamsFromSettings, - oauthEndpointOverridesFromSettings, serializeMcpConfig, } from "@inspector/core/mcp/serverList.js"; import type { ClientConfig } from "@inspector/core/client/types.js"; -import { - getActiveCimdClientMetadataUrl, - getActiveEnterpriseManagedAuthIdp, -} from "@inspector/core/client/types.js"; -import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; import { loadClientConfigRemote, saveClientConfigRemote, @@ -75,6 +58,10 @@ import { type SetupClientForServer, } from "./hooks/useOAuthRecovery"; import { useInspectorStores } from "./hooks/useInspectorStores"; +import { + useConnectionLifecycle, + useHandshakeTelemetry, +} from "./hooks/useConnectionLifecycle"; import { useMcpApps } from "./hooks/useMcpApps"; import { useExportActions } from "./hooks/useExportActions"; import { useProgressToasts } from "./hooks/useProgressToasts"; @@ -90,7 +77,6 @@ import type { } from "./components/screens/ToolsScreen/ToolsScreen"; import type { GetPromptState } from "./components/screens/PromptsScreen/PromptsScreen"; import type { ReadResourceState } from "./components/screens/ResourcesScreen/ResourcesScreen"; -import { clearScrollMemory } from "./hooks/useScrollMemory"; import { AppElicitationHost } from "./components/elements/AppElicitation/AppElicitationHost"; import type { LogEntryData } from "./components/elements/LogEntry/LogEntry"; import { @@ -120,19 +106,10 @@ import { import { downloadJsonFile } from "./lib/downloadFile"; import { enrichProtocolEntries } from "./utils/correlateTransportErrors"; import { visibleMalformedListItems } from "./utils/malformedListReport"; -import { - parseDeepLink, - deepLinkConfigEquals, - deepLinkParseStatus, -} from "./utils/deepLink"; +import { parseDeepLink, deepLinkParseStatus } from "./utils/deepLink"; import type { DeepLink, DeepLinkParseStatus } from "./utils/deepLink"; -import { clearOAuthResumeSnapshot } from "./lib/oauthResume"; -import { createWebEnvironment } from "./lib/environmentFactory"; -import { OAUTH_CALLBACK_PATH, isUnauthorizedError } from "./utils/oauthFlow"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; -import { clearServerOAuthState } from "./lib/clearServerOAuthState"; -import { authRecoveryRestoredMessage } from "./utils/oauthUx"; -import { getAuthToken, redirectUrlProvider } from "./lib/authToken"; +import { getAuthToken } from "./lib/authToken"; import { messagesToLogEntries } from "./lib/protocolReplay"; import { errorCodeOf, @@ -200,6 +177,10 @@ function App() { >(undefined); const [clientSettingsOpen, setClientSettingsOpen] = useState(false); const [connectionInfoModalOpen, setConnectionInfoModalOpen] = useState(false); + const closeConnectionInfoModal = useCallback( + () => setConnectionInfoModalOpen(false), + [], + ); const [removeTarget, setRemoveTarget] = useState(null); // Details for the output-schema-mismatch modal opened from the warning toast. const [outputValidationDetails, setOutputValidationDetails] = useState<{ @@ -229,14 +210,6 @@ function App() { undefined, ); - // Id of the server that just connected successfully (#1682) — the success - // mirror of `failedServerId`. Drives the green highlight + scroll-into-view on - // that ServerCard. Set on the →connected transition, cleared when a new - // connection attempt starts or the session disconnects. - const [connectedServerId, setConnectedServerId] = useState< - string | undefined - >(undefined); - // InspectorClient + per-primitive state managers. All recreated together // whenever the user switches active servers, then destroyed when the // next switch happens (or when the component unmounts). @@ -282,15 +255,6 @@ function App() { }); }, [configBaseUrl]); - // `setupClientForServer` is synchronous and memoized, so a caller that - // awaited the config would still resume with the `sandboxUrl` captured by the - // render it STARTED in — undefined, on the very load this matters for. The - // ref is written every render, so client construction reads the current value - // whichever entry point (connect, deep link, OAuth callback) reached it. - const sandboxUrlRef = useRef(undefined); - // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) - sandboxUrlRef.current = sandboxUrl; - // Whether the sandbox exists is only known once `/api/config` resolves, and // the answer is baked into the client at construction (it decides whether the // nested MCP Apps `elicitation` capability is advertised). So a connect waits // for it rather than guessing: guessing "available" over-claims a capability @@ -366,11 +330,42 @@ function App() { inspectorClient, }); - // Handshake telemetry. `connectStartRef` is set at the "connecting" edge - // and consumed at the "connected" edge — a ref (not state) so the - // intervening rerenders don't reset it. - const connectStartRef = useRef(undefined); - const [latencyMs, setLatencyMs] = useState(undefined); + // The narrow reset surface `useConnectionLifecycle` drops session-scoped UI + // state through on disconnect — one entry per owner, rather than that + // owner's individual setters (#2129). The log-level pair stays here because + // it is App-owned state the connect path also seeds. + const resetLogLevels = useCallback(() => { + setCurrentLogLevel("info"); + // Re-seed rather than blank: the client restores its own opt-in from the + // server setting at connect (`resetSessionState`), so blanking here would + // leave the control reading Off while every modern request still carries + // the level — visible on the auth-recovery path, which reconnects the same + // client instance rather than rebuilding it (#1629, #1797). + // The session ref is synced in a passive effect, so it still holds the + // outgoing server's id when this runs from `onDisconnect` — which is what + // lets the re-seed find its settings. Clearing that ref eagerly would take + // the no-server branch below and silently drop this to Off. + // Branch on the *server*, not its settings: an entry with no settings node + // is the common case (`mcp.json` written by hand, never opened in Server + // Settings), and there the default is right — it is what the seed and the + // client both use. Only "no server at all" means Off. + const outgoing = sessionRef.current.servers.find( + (s) => s.id === sessionRef.current.activeServerId, + ); + setModernLogLevel( + outgoing ? (resolveModernLogLevel(outgoing.settings) ?? null) : null, + ); + }, [sessionRef]); + // #1629: seed the live modern per-request log level from the server setting + // so the Logs-tab control reflects what the client stamps by default (the + // client is seeded the same way in its constructor). "off" means not opted + // in (null). Only affects modern connections. + const seedModernLogLevel = useCallback( + (settings: InspectorServerSettings | undefined) => { + setModernLogLevel(resolveModernLogLevel(settings) ?? null); + }, + [], + ); // Progress and task-status toasts, and the taskId → progress map the Tasks // screen renders from. Both hooks subscribe to the live client's events and @@ -415,6 +410,14 @@ function App() { paginatedListsOverride.valueFor(activeServerId) ?? persistedPaginatedLists; const connected = connectionStatus === "connected"; + // Handshake telemetry. `connectStartRef` is stamped at the "connecting" edge + // and consumed at the "connected" edge — a ref (not state) so the + // intervening rerenders don't reset it. Declared here rather than inside + // `useConnectionLifecycle` because `useOAuthRecovery`, which runs first, + // stamps it too (#2154). + const { connectStartRef, latencyMs } = + useHandshakeTelemetry(connectionStatus); + // The per-session state managers and the finished lists they feed. Owns the // create / destroy lifecycle, so the connection code below sees two stable // callbacks rather than twelve state slots. @@ -547,6 +550,28 @@ function App() { const prompts = promptsPagination.items; const resources = resourcesPagination.items; + // The session-scoped state each owner drops on disconnect. One `reset()` + // per owner, handed to `useConnectionLifecycle` as a single object so the + // connection hook does not take a dependency on every phase-1 hook (#2129). + const sessionReset = useMemo( + () => ({ + clearResultPanels, + resetTabUiState, + resetTaskProgress, + resetOAuthRecoveryState, + resetLogLevels, + closeConnectionInfoModal, + }), + [ + clearResultPanels, + resetTabUiState, + resetTaskProgress, + resetOAuthRecoveryState, + resetLogLevels, + closeConnectionInfoModal, + ], + ); + // MCP Apps runtime wiring (#2156): the sandbox bridge factories, the renderer // handle, and the app-rendered elicitation controller. Called here rather // than beside the other client-scoped hooks above because it reads the @@ -561,6 +586,63 @@ function App() { handleAppElicitationFail, } = useMcpApps({ inspectorClient, configBaseUrl, resources }); + // Deep-link parameters parsed once from the initial URL. Security gating + // (auth-token match, http(s)-only serverUrl) happens inside `parseDeepLink`, + // so a `DeepLink` value here is already validated. The parse status is + // surfaced as `data-deeplink` so an automated driver can tell "no deep link" + // from "deep link present but rejected" — both leave `data-status` idle. + const [deepLink, deepLinkStatus] = useMemo< + [DeepLink | undefined, DeepLinkParseStatus] + >(() => { + /* v8 ignore next -- SSR guard: happy-dom always defines window in tests */ + if (typeof window === "undefined") return [undefined, "none"]; + const search = window.location.search; + const parsed = parseDeepLink(search, getAuthToken()); + return [parsed, deepLinkParseStatus(search, parsed)]; + }, []); + + // Everything about bringing a session up and taking it down: building the + // client for a server, connecting, disconnecting, the effects that observe + // those transitions, and the session-scoped reset a disconnect triggers + // (#2154). + const { + connectedServerId, + connectErrorMessage, + onToggleConnection, + onDisconnect, + onReauthenticateFromBanner, + } = useConnectionLifecycle({ + sessionRef, + servers, + activeServerId, + inspectorClient, + connectionStatus, + addServer, + updateServer, + setActiveServerId, + setFailedServerId, + setInspectorClient, + createStores, + destroyStores, + lastPersistedSettings, + clientConfig, + newAppElicitationSession, + sandboxUrl, + initialConfigSettledRef, + connectStartRef, + setupClientForServerRef, + deepLink, + webOAuthStorage, + sessionStorageAdapter, + onBeforeOAuthRedirect, + prepareOAuthRedirect, + finalizeExplicitDisconnect, + reAuthBanner, + setReAuthBanner, + sessionReset, + seedModernLogLevel, + }); + // Fold the transport errors the SDK throws rather than delivers (e.g. -32601 // on HTTP 404) onto their still-pending Protocol requests, by correlating with // the Network log via JSON-RPC id. Returns `messages` unchanged when nothing @@ -619,120 +701,6 @@ function App() { const { pendingSamples, pendingElicitations } = usePendingClientRequests(inspectorClient); - // Capture observed handshake latency at the connecting → connected edge. - // Reset when the status leaves "connected" so the next connect starts - // clean (otherwise a stale latency would render on the next session). - useEffect(() => { - if ( - connectionStatus === "connected" && - connectStartRef.current !== undefined - ) { - setLatencyMs(Date.now() - connectStartRef.current); - connectStartRef.current = undefined; - } else if (connectionStatus !== "connected") { - setLatencyMs(undefined); - } - }, [connectionStatus]); - - // Track the just-connected server so its card gets the green highlight + - // scroll-into-view (#1682). Unlike `failedServerId` (which must survive the - // `disconnect` event a failed connect fires), "connected" is a stable status, - // so a status-driven effect can both set and clear it: set on connect, clear - // whenever the session isn't connected (disconnect, a new attempt's - // "connecting", or an error). - useEffect(() => { - // eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161) - setConnectedServerId( - connectionStatus === "connected" ? activeServerId : undefined, - ); - }, [connectionStatus, activeServerId]); - - // Disconnect the previous InspectorClient when it's replaced (server - // switch) or when App unmounts (HMR, tests). Without this the prior - // session's transport — a spawned stdio subprocess, an SSE stream, or - // an HTTP session — stays open until GC eventually lets go. The - // state-manager destroys in `setupClientForServer` only handle the - // listener side; this effect handles the transport side. `disconnect()` - // is the canonical lifecycle hook (InspectorClient has no `destroy()`); - // it closes the transport, clears subscriptions, cancels receiver TTLs. - useEffect(() => { - return () => { - if (inspectorClient) { - void inspectorClient.disconnect(); - } - }; - }, [inspectorClient]); - - // Reset the session-scoped UI state that lives in App.tsx (rather than - // inside the per-server state managers), so the next server's screens don't - // show server A's last result. The per-call panels (`toolCallState` / - // `getPromptState` / `readResourceState`) and the optimistic - // `currentLogLevel` all survive a disconnect/reconnect cycle otherwise — - // see #1368. `latencyMs` is intentionally excluded: it resets via the - // `connectionStatus` effect above, which has its own connecting-edge ref to - // coordinate with. Colocated with the setters it touches so this is the - // single place to extend as App.tsx accrues more per-session state (#1394). - // Does not clear the OAuth resume snapshot — that is tied to an in-flight - // full-page redirect and is cleared on explicit disconnect or consumed on callback. - const resetSessionScopedUiState = useCallback(() => { - setToolCallState(undefined); - setGetPromptState(undefined); - setReadResourceState(undefined); - resetTabUiState(); - resetTaskProgress(); - setCurrentLogLevel("info"); - // Re-seed rather than blank: the client restores its own opt-in from the - // server setting at connect (`resetSessionState`), so blanking here would - // leave the control reading Off while every modern request still carries - // the level — visible on the auth-recovery path, which reconnects the same - // client instance rather than rebuilding it (#1629, #1797). - // The session ref is synced in a passive effect, so it still holds the - // outgoing server's id when this runs from `onDisconnect` — which is what - // lets the re-seed find its settings. Clearing that ref eagerly would take - // the no-server branch below and silently drop this to Off. - // Branch on the *server*, not its settings: an entry with no settings node - // is the common case (`mcp.json` written by hand, never opened in Server - // Settings), and there the default is right — it is what the seed and the - // client both use. Only "no server at all" means Off. - const activeServer = sessionRef.current.servers.find( - (s) => s.id === sessionRef.current.activeServerId, - ); - setModernLogLevel( - activeServer - ? (resolveModernLogLevel(activeServer.settings) ?? null) - : null, - ); - resetOAuthRecoveryState(); - // Remembered scroll offsets are session-scoped too — drop them so the next - // session's screens start at the top (#1417). - clearScrollMemory(); - }, [sessionRef, resetTabUiState, resetTaskProgress, resetOAuthRecoveryState]); - - // Reset activeServerId whenever the live session ends. Without this the - // other ServerCards stay `inert` after disconnect — ServerCard dims any - // card whose id differs from `activeServer`. Subscribing to - // InspectorClient's own `disconnect` event covers all three paths - // (explicit toggle, header Disconnect button, mid-session transport - // failure / process exit) and avoids the first-render-clobbers-new-id - // trap that watching connectionStatus has (status starts as - // "disconnected" for the new client before connect() runs). The - // session-scoped panel/level reset rides along here too via - // `resetSessionScopedUiState`. - useEffect(() => { - if (!inspectorClient) return; - const onDisconnect = () => { - setActiveServerId(undefined); - // Drop the open flag too — without this the modal would pop back the - // next time `initializeResult` re-becomes truthy (e.g. reconnect). - setConnectionInfoModalOpen(false); - resetSessionScopedUiState(); - }; - inspectorClient.addEventListener("disconnect", onDisconnect); - return () => { - inspectorClient.removeEventListener("disconnect", onDisconnect); - }; - }, [inspectorClient, resetSessionScopedUiState]); - // The Server Info modal needs the active server's transport and (optional) // OAuth details — both are co-located here so the modal opens against the // same connection snapshot the header is reading. Also feeds the @@ -790,37 +758,6 @@ function App() { activeServer, ]); - // Last connection-level error message, surfaced as `data-error-message` on - // the InspectorView header so an automated driver can read *why* a connect - // failed without scraping a transient toast. Cleared on the next connect - // attempt and on successful connection. - const [connectErrorMessage, setConnectErrorMessage] = useState< - string | undefined - >(undefined); - // Named writer so a single call site can be extended later (e.g. telemetry) - // and the intent (record a connection-level failure) stays explicit. - const recordConnectError = useCallback((message: string) => { - setConnectErrorMessage(message); - }, []); - - // Deep-link parameters parsed once from the initial URL. Security gating - // (auth-token match, http(s)-only serverUrl) happens inside `parseDeepLink`, - // so a `DeepLink` value here is already validated. The parse status is - // surfaced as `data-deeplink` so an automated driver can tell "no deep link" - // from "deep link present but rejected" — both leave `data-status` idle. - const [deepLink, deepLinkStatus] = useMemo< - [DeepLink | undefined, DeepLinkParseStatus] - >(() => { - /* v8 ignore next -- SSR guard: happy-dom always defines window in tests */ - if (typeof window === "undefined") return [undefined, "none"]; - const search = window.location.search; - const parsed = parseDeepLink(search, getAuthToken()); - return [parsed, deepLinkParseStatus(search, parsed)]; - }, []); - const deepLinkEnsureRef = useRef(false); - const deepLinkUpdateRef = useRef(false); - const deepLinkConnectRef = useRef(false); - // Surface a mid-session transport failure (stdio crash, SSE drop, HTTP 5xx) // as a toast. The handshake case is handled in `onToggleConnection`'s catch; // this covers the `status: connected → error` transition that fires the @@ -895,587 +832,6 @@ function App() { [sessionRef, fetchLogRef], ); - // Wire up + tear down per active server. Called by `onToggleConnection` - // when the user switches targets. Returns the new client so the toggle - // can call `connect()` against it before React re-renders. - const setupClientForServer = useCallback( - (server: ServerEntry, sessionId?: string): InspectorClient => { - // Tear down the previous session's managers before building the new - // client — each destroy() unsubscribes from the old client's events, and - // doing it here (rather than leaving it to `createStores` below, which - // also tears down whatever is live) means a throw while constructing the - // client leaves nothing still listening to the outgoing one. A no-op on - // the first call. - destroyStores(); - - const { environment, logger } = createWebEnvironment( - getAuthToken(), - redirectUrlProvider, - onBeforeOAuthRedirect, - ); - // The settings node persisted in mcp.json for this server — distinct - // from the InspectorClient options we're about to derive from it. - // - // Through the tracker, not `server.settings` directly: the entry only - // advances on a successful list read, so a write that landed while reads - // were failing — including one made from the settings modal for a server - // that was not connected at the time — would otherwise be undone at the - // next connect, which builds the client from that frozen entry. This is - // the one place the whole connection is configured, so every construction - // path (connect, reconnect, OAuth resume) goes through it (#2089). - const savedSettings = - lastPersistedSettings.resolve(server.id) ?? server.settings; - const activeIdp = getActiveEnterpriseManagedAuthIdp(clientConfig); - const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig); - // Flatten the persisted settings into the InspectorClient options shape. - // Empty / zero values stay unset so the SDK defaults apply. - // Per-server default `_meta` is already a JSON object (#1910) — no - // pair-array flattening left to do; `{}` means "no defaults". - const defaultMetadata = savedSettings?.metadata; - const serverAuthorizationParams = savedSettings - ? oauthAuthorizationParamsFromSettings(savedSettings) - : undefined; - const serverEndpointOverrides = savedSettings - ? oauthEndpointOverridesFromSettings(savedSettings) - : undefined; - const oauthFromServer = - savedSettings && - (savedSettings.oauthClientId || - savedSettings.oauthClientSecret || - savedSettings.oauthScopes || - serverAuthorizationParams || - serverEndpointOverrides || - savedSettings.enterpriseManaged || - savedSettings.oauthRequestRefreshToken === false) - ? { - ...(savedSettings.oauthClientId && { - clientId: savedSettings.oauthClientId, - }), - ...(savedSettings.oauthClientSecret && { - clientSecret: savedSettings.oauthClientSecret, - }), - ...(savedSettings.oauthScopes && { - scope: savedSettings.oauthScopes, - }), - ...(serverAuthorizationParams && { - authorizationParams: serverAuthorizationParams, - }), - ...serverEndpointOverrides, - ...(savedSettings.enterpriseManaged && { - enterpriseManaged: true, - }), - // #2068: only the explicit opt-out is forwarded; omitting the key - // leaves the provider's default (declare `refresh_token`) in place. - ...(savedSettings.oauthRequestRefreshToken === false && { - requestRefreshToken: false, - }), - } - : undefined; - const oauth = - oauthFromServer || activeCimdUrl - ? { - ...(oauthFromServer ?? {}), - ...(activeCimdUrl && { clientMetadataUrl: activeCimdUrl }), - } - : undefined; - // The stdio `env` / `cwd` are edited as *settings* but stored on — and - // read by the transport from — *config*, so the tracker's account of the - // former has to be carried onto the latter. `server.config` comes off the - // same frozen `servers` entry `server.settings` does, so without this a - // save that landed while list reads were failing spawns the child process - // with the pre-save environment while the modal, re-seeded from the - // tracker, shows the new one (#2096). Same mapping the PUT route applies - // when persisting it, from the same helper. - const effectiveConfig = applyStdioSettingsToConfig( - server.config, - savedSettings, - ); - const client = new InspectorClient(effectiveConfig, { - environment, - // The Tasks tab needs the receiver-task pipeline; the - // requestor-task list comes from the client's task store. - receiverTasks: true, - // Sampling / elicitation are on by default; keep the parameterized - // options off until the UI grows the surface to render them. - elicit: { form: true, url: true }, - // Web only, and only when the sandbox renderer is actually available: - // supplying this advertises the nested MCP Apps `elicitation` - // capability, and a client that cannot host an app must not claim it - // (#1854). Callers await `initialConfigSettled` first, so `sandboxUrl` - // here means "confirmed absent" rather than "not known yet" — a - // connection that reaches this with no sandbox behaves like the - // CLI/TUI: native elicitation queue, no claim made to the server. - ...(sandboxUrlRef.current && { - appElicitation: newAppElicitationSession().render, - }), - // Always advertise the roots capability (even with no configured - // roots) so the server can issue roots/list and receive - // roots/list_changed; the configured roots are the answer to - // roots/list. Empty-uri rows are dropped before they reach the wire. - roots: cleanRoots(savedSettings?.roots ?? []), - ...(savedSettings && - savedSettings.requestTimeout > 0 && { - timeout: savedSettings.requestTimeout, - }), - ...(defaultMetadata && - Object.keys(defaultMetadata).length > 0 && { - defaultMetadata, - }), - ...(oauth && { oauth }), - ...(activeIdp && { - enterpriseManagedAuth: { idp: activeIdp }, - }), - ...(clientConfig.enterpriseManagedAuth && { - installEnterpriseManagedAuth: clientConfig.enterpriseManagedAuth, - }), - ...(savedSettings && { serverSettings: savedSettings }), - // Per-server protocol era (SEP §7.8) → SDK versionNegotiation. Absent - // settings or an unset era default to legacy inside - // eraToVersionNegotiation / the InspectorClient constructor (#1626). - ...(savedSettings?.protocolEra && { - versionNegotiation: eraToVersionNegotiation( - savedSettings.protocolEra, - ), - }), - // Per-server advertised-extension overrides (#1739). Absent/empty falls - // back to the registry defaults in the InspectorClient constructor. - ...(savedSettings?.advertisedExtensions && - Object.keys(savedSettings.advertisedExtensions).length > 0 && { - advertisedExtensions: savedSettings.advertisedExtensions, - }), - // Set on the `/oauth/callback` rebuild so the client's `saveSession` - // events (and any later persistence) key off the same OAuth authId - // the pre-redirect page saved under. - ...(sessionId && { sessionId }), - }); - - setInspectorClient(client); - // #1629: seed the live modern per-request log level from the server - // setting so the Logs-tab control reflects what the client stamps by - // default (the client was seeded the same way in its constructor). "off" - // means not opted in (null). Only affects modern connections. - setModernLogLevel(resolveModernLogLevel(savedSettings) ?? null); - // Wire session storage so the fetch log survives the OAuth redirect. - // When `sessionId` is supplied (the `/oauth/callback` rebuild) the prior - // page's `auth` entries are restored on construction; the actual save is - // driven synchronously from `onBeforeOAuthRedirect` above (keyed by the - // same authId). `createStores` points `fetchLogRef` at the new instance - // so that hook reads the current log. - createStores(client, { - sessionStorage: sessionStorageAdapter, - logger, - maxFetchRequests: - savedSettings?.maxFetchRequests ?? DEFAULT_MAX_FETCH_REQUESTS, - ...(sessionId && { sessionId }), - }); - - return client; - }, - [ - createStores, - destroyStores, - sessionStorageAdapter, - onBeforeOAuthRedirect, - clientConfig, - newAppElicitationSession, - lastPersistedSettings, - ], - ); - // Publish it to the `/oauth/callback` effect, which needs to rebuild the - // client for the server that started the flow and cannot reach a callback - // declared this far down. - // - // A *layout* effect, not a render-phase write and not a passive one. React - // runs every layout effect before any passive effect of the same commit, and - // the callback effect inside `useOAuthRecovery` is passive — so this is - // always published before its first read, without the render-phase ref - // mutation the compiler rule (rightly) rejects. - useLayoutEffect(() => { - setupClientForServerRef.current = setupClientForServer; - }, [setupClientForServer]); - - const onToggleConnection = useCallback( - async (id: string) => { - // Whether this client may advertise app-rendered elicitation is decided - // at construction and cannot be revised afterwards, so wait for the fact - // rather than guess it (see `initialConfigSettledRef`). Already resolved - // by the time any human clicks; this only orders a deep-link auto-connect - // that races the same page load. - await initialConfigSettledRef.current?.promise; - // Same server, already connected → disconnect. - if ( - id === activeServerId && - connectionStatus === "connected" && - inspectorClient - ) { - try { - await inspectorClient.disconnect(); - } finally { - finalizeExplicitDisconnect(); - } - return; - } - - // Read from the ref so a caller that already awaited an - // addServer/updateServer in the same async tick (e.g. the deep-link - // auto-connect IIFE) sees the freshly-mutated list, not the stale array - // captured by this callback's closure. - const target = sessionRef.current.servers.find((s) => s.id === id); - if (!target) return; - - // Always rebuild the InspectorClient on a (re)connect so the latest - // `target.settings` (headers, metadata, timeouts, OAuth credentials) - // are picked up. Reusing the previous client object would freeze the - // settings at the moment it was first constructed, which would be - // surprising right after the user edited them in the settings modal. - const client = setupClientForServer(target); - if (id !== activeServerId) { - setActiveServerId(id); - } - // A new connection attempt has begun: clear any previous failure flag so - // the red border on the last-failed card is removed (#1621). If this - // attempt also fails, the catch below re-sets it for this server. - setFailedServerId(undefined); - // Clear the machine-readable connect error for the same reason; a fresh - // attempt starts from a clean `data-error-message`. - setConnectErrorMessage(undefined); - - connectStartRef.current = Date.now(); - try { - // `settings.connectionTimeout` is consumed inside InspectorClient.connect - // (Promise.race + transport teardown live there now), so this branch - // stays unaware of the per-server timeout. TUI/CLI consumers get the - // same behavior by reading from `serverSettings` on the client. - await client.connect(); - } catch (err) { - // Handshake-only. A mid-session transport failure does not throw; the - // client's `error` event surfaces those, consumed via - // `useInspectorClient`'s `lastError` and toasted in the effect above - // (#1323). - connectStartRef.current = undefined; - - if (isEmaClientNotConfiguredError(err)) { - notifications.show({ - title: `Cannot connect to "${target.name}"`, - message: err.message, - color: "red", - autoClose: false, - }); - return; - } - - // A 401 from an OAuth-protected server means we have no (valid) token - // yet. Kick off the authorization-code flow: `authenticate()` runs - // discovery + DCR (proxied through the backend), then redirects the - // whole page to the auth server via `BrowserNavigation`. Persist the - // initiating server id first so the `/oauth/callback` load can resume - // against the right client. The redirect unloads this page, so there's - // nothing to do after the await on the success path. - if (err instanceof AuthRecoveryRequiredError) { - try { - if (await client.checkAuthChallengeSatisfied(err.authChallenge)) { - connectStartRef.current = Date.now(); - await client.connect(); - return; - } - } catch (recoveryErr) { - // Both awaits above are unguarded connect work sitting inside a - // `catch`, so a rejection escapes `onToggleConnection` altogether: - // no toast, no red border, no sidebar — the #2108 failure mode in - // its most invisible form. Surface it as the failed connect attempt - // it is. A throw from `checkAuthChallengeSatisfied` lands here too - // rather than falling through to `prepareOAuthRedirect`: it is not - // the same as the challenge being *unsatisfied*, and navigating the - // whole page away on the strength of an error would bury it. - connectStartRef.current = undefined; - // Tear the session down before reporting, as the sibling OAuth - // catch below does. The outer `connect()` rejected with an - // auth-recovery error, which deliberately holds the status at - // `"connecting"` rather than moving it to `"error"` — so if the - // challenge check is what rejected, nothing else ever ends the - // attempt and the toggle spins while the active-server lock is - // held. The fetch log survives a disconnect, so the Network - // diagnostics this issue is about are unaffected. - await client.disconnect().catch(() => {}); - setFailedServerId(id); - const message = - recoveryErr instanceof Error - ? recoveryErr.message - : String(recoveryErr); - setConnectErrorMessage(message); - notifications.show({ - title: `Failed to connect to "${target.name}"`, - message, - color: "red", - }); - return; - } - prepareOAuthRedirect({ - serverId: id, - authKind: "reauth", - authorizationUrl: err.authorizationUrl, - preRedirectContext: "connect", - client, - }); - return; - } - - if (isUnauthorizedError(err)) { - try { - const authUrl = await client.authenticate(); - if (authUrl === undefined) { - connectStartRef.current = Date.now(); - await client.connect(); - } else { - prepareOAuthRedirect({ - serverId: id, - authKind: "reauth", - authorizationUrl: authUrl, - preRedirectContext: "connect", - client, - }); - } - return; - } catch (authErr) { - clearOAuthResumeSnapshot(); - await client.disconnect().catch(() => {}); - if (isEmaClientNotConfiguredError(authErr)) { - notifications.show({ - title: `Cannot connect to "${target.name}"`, - message: authErr.message, - color: "red", - autoClose: false, - }); - return; - } - // The connect attempt failed, same as any other handshake error — - // flag the card (#1621) and, with it, open the monitoring sidebar - // onto the OAuth requests that explain the failure (#2108). This - // leg never reaches the `"error"` connection status (the - // `disconnect()` above settles it at `"disconnected"`), so this - // flag is the only signal the view has that a connect attempt died. - setFailedServerId(id); - const message = - authErr instanceof Error ? authErr.message : String(authErr); - setConnectErrorMessage(message); - notifications.show({ - title: `OAuth authorization failed for "${target.name}"`, - message, - color: "red", - }); - return; - } - } - - // Non-auth handshake error: toast so the user sees what went wrong - // instead of the ConnectionToggle silently reverting to - // "disconnected", and flag the card with a red border (#1621). - setFailedServerId(id); - const message = err instanceof Error ? err.message : String(err); - setConnectErrorMessage(message); - notifications.show({ - title: `Failed to connect to "${target.name}"`, - message, - color: "red", - }); - } - }, - [ - sessionRef, - activeServerId, - connectionStatus, - inspectorClient, - setupClientForServer, - prepareOAuthRedirect, - finalizeExplicitDisconnect, - ], - ); - - const onDisconnect = useCallback(async () => { - if (!inspectorClient) return; - try { - await inspectorClient.disconnect(); - } finally { - finalizeExplicitDisconnect(); - } - }, [inspectorClient, finalizeExplicitDisconnect]); - - // Deep-link auto-connect (the URL-driven case of #1183). `useServers` - // 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 the session ref, 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; - - const existing = servers.find((s) => s.id === deepLink.serverId); - if (!existing) { - if (deepLinkEnsureRef.current) return; - deepLinkEnsureRef.current = true; - 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; - // Connect unless we're already *connected* to the deep-link server. Gating - // on `activeServerId` identity alone would skip the connect when a prior - // session restored `activeServerId` to the `deep-link` id while the socket - // is disconnected — a reload of the same deep-link URL would then silently - // never connect. `onToggleConnection` only disconnects when the id is the - // active one AND the status is connected, so this condition also avoids - // toggling a live connection off. - const alreadyConnected = - activeServerId === deepLink.serverId && connectionStatus === "connected"; - if (!alreadyConnected) { - // eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161) - 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, - activeServerId, - connectionStatus, - addServer, - updateServer, - onToggleConnection, - recordConnectError, - ]); - - const onReauthenticateFromBanner = useCallback(() => { - if (!reAuthBanner) return; - const serverId = reAuthBanner.serverId; - const bannerKind = reAuthBanner.kind; - setReAuthBanner(null); - - // Lost-authorization-state recovery (#1808). The stale half of the flow - // (code verifier without discovery state, possibly a stale registration) - // would make a plain retry fail the same way, so drop the persisted OAuth - // state for this server first, then start a fresh authorization. This - // banner is only raised from the `/oauth/callback` failure path, where the - // session never reached "connected", so connecting is always the right - // toggle direction here. - if (bannerKind === "lost_authorization_state") { - void (async () => { - const server = sessionRef.current.servers.find( - (s) => s.id === serverId, - ); - if (server) { - try { - await clearServerOAuthState({ - config: server.config, - inspectorClient: - serverId === activeServerId ? inspectorClient : null, - isActiveConnection: serverId === activeServerId, - oauthStorage: webOAuthStorage, - }); - } catch (err) { - notifications.show({ - title: "Could not clear the stored authorization state", - message: err instanceof Error ? err.message : String(err), - color: "red", - // The banner is already dismissed and the flow is dead, so this - // is the only remaining explanation — don't time it out. - autoClose: false, - }); - return; - } - } - await onToggleConnection(serverId); - })(); - return; - } - - if ( - serverId === activeServerId && - connectionStatus === "connected" && - inspectorClient - ) { - void (async () => { - const server = servers.find((s) => s.id === serverId); - try { - const authUrl = await inspectorClient.authenticate(); - if (authUrl === undefined) { - await inspectorClient.pushRemoteAuthState(); - notifications.show({ - title: "Authorization restored", - message: authRecoveryRestoredMessage(), - color: "green", - autoClose: 4000, - }); - return; - } - prepareOAuthRedirect({ - serverId, - authKind: "reauth", - authorizationUrl: authUrl, - }); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - notifications.show({ - title: server - ? `OAuth authorization failed for "${server.name}"` - : "OAuth authorization failed", - message, - color: "red", - }); - } - })(); - return; - } - - void onToggleConnection(serverId); - }, [ - sessionRef, - reAuthBanner, - activeServerId, - connectionStatus, - inspectorClient, - servers, - prepareOAuthRedirect, - onToggleConnection, - webOAuthStorage, - setReAuthBanner, - ]); - // --- Action handlers that route directly to the InspectorClient. --- const onCallTool = useCallback( diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx new file mode 100644 index 000000000..3839cfc12 --- /dev/null +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -0,0 +1,1208 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { useLayoutEffect, useRef } from "react"; +import { InspectorClient } from "@inspector/core/mcp/index.js"; +import type { + ConnectionStatus, + InspectorServerSettings, + MCPServerConfig, + ServerEntry, +} from "@inspector/core/mcp/types.js"; +import type { ClientConfig } from "@inspector/core/client/types.js"; +import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; +import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; +import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; +import { EMPTY_SETTINGS } from "../utils/serverSettingsDefaults"; +import { DEEP_LINK_SERVER_ID } from "../utils/deepLink"; +import type { DeepLink } from "../utils/deepLink"; +import { useSessionRef } from "./useSessionRef"; +import type { SetupClientForServer } from "./useOAuthRecovery"; +import type { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage"; +import { + useConnectionLifecycle, + useHandshakeTelemetry, + type ConnectionLifecycle, + type SessionResetSurface, +} from "./useConnectionLifecycle"; + +// --- Module doubles --------------------------------------------------------- +// Everything the hook reaches outside React and outside the client: the toast +// layer, the web environment factory (which would otherwise build real +// transports), the auth token, and the OAuth-state clear. `InspectorClient` +// itself is NOT mocked — `setupClientForServer` exists to translate a server +// entry into that constructor's options, so a stand-in would leave the whole +// translation unverified. Its network-touching methods are spied on the +// prototype instead. + +const { notificationsMock } = vi.hoisted(() => ({ + notificationsMock: { show: vi.fn(), update: vi.fn(), hide: vi.fn() }, +})); +vi.mock("@mantine/notifications", () => ({ notifications: notificationsMock })); + +const { environmentMock } = vi.hoisted(() => ({ + environmentMock: vi.fn(), +})); +vi.mock("../lib/environmentFactory", () => ({ + createWebEnvironment: (...args: unknown[]) => { + environmentMock(...args); + return { environment: {}, logger: { level: "silent" } }; + }, +})); + +const { clearServerOAuthStateMock } = vi.hoisted(() => ({ + clearServerOAuthStateMock: vi.fn(), +})); +vi.mock("../lib/clearServerOAuthState", () => ({ + clearServerOAuthState: clearServerOAuthStateMock, +})); + +vi.mock("../lib/authToken", () => ({ + getAuthToken: () => "test-token", + redirectUrlProvider: () => "http://localhost/oauth/callback", +})); + +// --- Fixtures --------------------------------------------------------------- + +const entry = (id: string, over: Partial = {}): ServerEntry => ({ + id, + name: `Server ${id}`, + config: { type: "streamable-http", url: "https://mcp.example/mcp" }, + connection: { status: "disconnected" }, + ...over, +}); + +const deepLinkConfig: MCPServerConfig = { + type: "streamable-http", + url: "https://deep.example/mcp", +}; + +const deepLink = (over: Partial = {}): DeepLink => ({ + serverId: DEEP_LINK_SERVER_ID, + serverConfig: deepLinkConfig, + appArgs: {}, + autoOpen: false, + ...over, +}); + +/** + * A 401 the way the SDK surfaces one. `isUnauthorizedError` keys off the + * status/code rather than the message, so a plain `Error("401")` would take + * the non-auth arm and the test would pass for the wrong reason. + */ +const unauthorized = (): Error => + Object.assign(new Error("Unauthorized"), { status: 401 }); + +/** The one OAuth-storage member `clearServerOAuthState` is handed. */ +const oauthStorage = {} as ReturnType; +const sessionStorageAdapter = {} as RemoteInspectorClientStorage; + +interface HarnessProps { + servers?: ServerEntry[]; + activeServerId?: string; + connectionStatus?: ConnectionStatus; + /** + * The live client. `undefined` means "whatever the last + * `setupClientForServer` built", which is what App.tsx's `inspectorClient` + * state settles to a render later. + */ + client?: InspectorClient | null; + clientConfig?: ClientConfig; + sandboxUrl?: string; + deepLink?: DeepLink; + reAuthBanner?: { + serverId: string; + message: string; + kind?: "lost_authorization_state"; + } | null; + /** Feeds `lastPersistedSettings.resolve`, which wins over `entry.settings`. */ + persisted?: Record; + addServerImpl?: (id: string, config: MCPServerConfig) => Promise; + updateServerImpl?: ( + id: string, + nextId: string, + config: MCPServerConfig, + ) => Promise; +} + +function spies() { + return { + addServer: vi.fn().mockResolvedValue(undefined), + updateServer: vi.fn().mockResolvedValue(undefined), + setActiveServerId: vi.fn(), + setFailedServerId: vi.fn(), + setInspectorClient: vi.fn(), + createStores: vi.fn(), + destroyStores: vi.fn(), + newAppElicitationSession: vi.fn(() => ({ + render: vi.fn(), + close: vi.fn(), + })), + onBeforeOAuthRedirect: vi.fn(), + prepareOAuthRedirect: vi.fn(), + finalizeExplicitDisconnect: vi.fn(), + setReAuthBanner: vi.fn(), + seedModernLogLevel: vi.fn(), + clearResultPanels: vi.fn(), + resetTabUiState: vi.fn(), + resetTaskProgress: vi.fn(), + resetOAuthRecoveryState: vi.fn(), + resetLogLevels: vi.fn(), + closeConnectionInfoModal: vi.fn(), + }; +} + +type Spies = ReturnType; + +interface Harness { + api: () => ConnectionLifecycle; + rerender: (next: HarnessProps) => void; + spies: Spies; + /** The `setupClientForServer` published to the OAuth callback path. */ + published: () => SetupClientForServer | null; +} + +function harness(initial: HarnessProps = {}): Harness { + let latest: ConnectionLifecycle | undefined; + const s = spies(); + + function Probe({ p }: { p: HarnessProps }) { + const servers = p.servers ?? []; + const sessionRef = useSessionRef({ + activeServerId: p.activeServerId, + servers, + inspectorClient: p.client ?? null, + }); + // Held across renders and published from a layout effect, exactly as + // App.tsx does — a fresh object per render would give the hook a changing + // dependency the real one never has. + const setupClientForServerRef = useRef(null); + const initialConfigSettledRef = useRef<{ promise: Promise }>(null); + initialConfigSettledRef.current ??= { promise: Promise.resolve() }; + const sessionReset: SessionResetSurface = { + clearResultPanels: s.clearResultPanels, + resetTabUiState: s.resetTabUiState, + resetTaskProgress: s.resetTaskProgress, + resetOAuthRecoveryState: s.resetOAuthRecoveryState, + resetLogLevels: s.resetLogLevels, + closeConnectionInfoModal: s.closeConnectionInfoModal, + }; + const { connectStartRef } = useHandshakeTelemetry( + p.connectionStatus ?? "disconnected", + ); + latest = useConnectionLifecycle({ + sessionRef, + servers, + activeServerId: p.activeServerId, + inspectorClient: p.client ?? null, + connectionStatus: p.connectionStatus ?? "disconnected", + addServer: p.addServerImpl ?? s.addServer, + updateServer: p.updateServerImpl ?? s.updateServer, + setActiveServerId: s.setActiveServerId, + setFailedServerId: s.setFailedServerId, + setInspectorClient: s.setInspectorClient, + createStores: s.createStores, + destroyStores: s.destroyStores, + lastPersistedSettings: { + resolve: (id: string) => p.persisted?.[id], + }, + clientConfig: p.clientConfig ?? {}, + newAppElicitationSession: s.newAppElicitationSession, + sandboxUrl: p.sandboxUrl, + initialConfigSettledRef, + connectStartRef, + setupClientForServerRef, + deepLink: p.deepLink, + webOAuthStorage: oauthStorage, + sessionStorageAdapter, + onBeforeOAuthRedirect: s.onBeforeOAuthRedirect, + prepareOAuthRedirect: s.prepareOAuthRedirect, + finalizeExplicitDisconnect: s.finalizeExplicitDisconnect, + reAuthBanner: p.reAuthBanner ?? null, + setReAuthBanner: s.setReAuthBanner, + sessionReset, + seedModernLogLevel: s.seedModernLogLevel, + }); + useLayoutEffect(() => { + publishedRef = setupClientForServerRef.current; + }); + return null; + } + + let publishedRef: SetupClientForServer | null = null; + const { rerender } = renderWithMantine(); + return { + api: () => { + if (!latest) throw new Error("hook did not render"); + return latest; + }, + rerender: (next) => rerender(), + spies: s, + published: () => publishedRef, + }; +} + +/** The client `setupClientForServer` most recently constructed. */ +const lastClient = (h: Harness): InspectorClient => { + const calls = h.spies.setInspectorClient.mock.calls; + const client = calls.at(-1)?.[0] as InspectorClient | null | undefined; + if (!client) throw new Error("no client was constructed"); + return client; +}; + +const toastTitles = (): string[] => + notificationsMock.show.mock.calls.map((c) => String(c[0]?.title)); + +let connectSpy: ReturnType; +let disconnectSpy: ReturnType; +let authenticateSpy: ReturnType; +let checkSpy: ReturnType; +let pushAuthSpy: ReturnType; + +beforeEach(() => { + vi.clearAllMocks(); + window.history.replaceState({}, "", "/"); + connectSpy = vi + .spyOn(InspectorClient.prototype, "connect") + .mockResolvedValue(undefined); + disconnectSpy = vi + .spyOn(InspectorClient.prototype, "disconnect") + .mockResolvedValue(undefined); + authenticateSpy = vi + .spyOn(InspectorClient.prototype, "authenticate") + .mockResolvedValue(undefined); + checkSpy = vi + .spyOn(InspectorClient.prototype, "checkAuthChallengeSatisfied") + .mockResolvedValue(false); + pushAuthSpy = vi + .spyOn(InspectorClient.prototype, "pushRemoteAuthState") + .mockResolvedValue(undefined); + clearServerOAuthStateMock.mockResolvedValue(true); +}); + +afterEach(() => { + vi.restoreAllMocks(); + window.history.replaceState({}, "", "/"); +}); + +describe("useHandshakeTelemetry", () => { + function TelemetryProbe({ + status, + onValue, + start, + }: { + status: ConnectionStatus; + onValue: (latency: number | undefined) => void; + start?: number; + }) { + const { connectStartRef, latencyMs } = useHandshakeTelemetry(status); + if (start !== undefined) connectStartRef.current = start; + onValue(latencyMs); + return null; + } + + it("measures the connecting → connected edge and consumes the stamp", async () => { + const seen: (number | undefined)[] = []; + const onValue = (v: number | undefined) => seen.push(v); + const { rerender } = renderWithMantine( + , + ); + rerender(); + await waitFor(() => expect(seen.at(-1)).toBeGreaterThanOrEqual(50)); + }); + + it("leaves the latency unset when the connected edge carries no stamp", async () => { + const seen: (number | undefined)[] = []; + const onValue = (v: number | undefined) => seen.push(v); + renderWithMantine(); + await waitFor(() => expect(seen.at(-1)).toBeUndefined()); + }); + + it("clears the latency once the session is no longer connected", async () => { + const seen: (number | undefined)[] = []; + const onValue = (v: number | undefined) => seen.push(v); + const { rerender } = renderWithMantine( + , + ); + rerender(); + await waitFor(() => expect(seen.at(-1)).not.toBeUndefined()); + rerender(); + await waitFor(() => expect(seen.at(-1)).toBeUndefined()); + }); +}); + +describe("useConnectionLifecycle", () => { + describe("setupClientForServer", () => { + it("builds a client from the persisted settings, not the frozen entry", () => { + const persistedSettings: InspectorServerSettings = { + ...EMPTY_SETTINGS, + requestTimeout: 4000, + maxFetchRequests: 42, + metadata: { tenant: "acme" }, + roots: [{ uri: "file:///work", name: "work" }], + protocolEra: "modern", + advertisedExtensions: { "io.modelcontextprotocol/tasks": true }, + oauthClientId: "cid", + oauthClientSecret: "secret", + oauthScopes: "a b", + enterpriseManaged: true, + oauthRequestRefreshToken: false, + }; + const stale: InspectorServerSettings = { + ...EMPTY_SETTINGS, + requestTimeout: 1, + }; + const h = harness({ + servers: [entry("a", { settings: stale })], + persisted: { a: persistedSettings }, + sandboxUrl: "http://localhost:6275/sandbox", + clientConfig: { + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example", clientId: "ema-client" }, + }, + }, + }); + + const client = h.published()!(entry("a", { settings: stale })); + + expect(client.getServerSettings()?.requestTimeout).toBe(4000); + expect(client.getRoots()).toEqual([ + { uri: "file:///work", name: "work" }, + ]); + expect(h.spies.destroyStores).toHaveBeenCalled(); + expect(h.spies.seedModernLogLevel).toHaveBeenCalledWith( + persistedSettings, + ); + expect(h.spies.createStores).toHaveBeenCalledWith( + client, + expect.objectContaining({ maxFetchRequests: 42 }), + ); + // The sandbox is present, so the nested MCP Apps elicitation session is + // opened and the capability may be advertised (#1854). + expect(h.spies.newAppElicitationSession).toHaveBeenCalled(); + }); + + it("falls back to the entry's own settings and the default log size", () => { + const h = harness({ servers: [entry("a")] }); + + const client = h.published()!(entry("a")); + + expect(client.getServerSettings()).toBeUndefined(); + expect(h.spies.seedModernLogLevel).toHaveBeenCalledWith(undefined); + expect(h.spies.createStores).toHaveBeenCalledWith( + client, + expect.objectContaining({ maxFetchRequests: expect.any(Number) }), + ); + // No sandbox URL — the client must not claim app-rendered elicitation. + expect(h.spies.newAppElicitationSession).not.toHaveBeenCalled(); + }); + + it("carries the OAuth session id onto both the client and its stores", () => { + const h = harness({ servers: [entry("a")] }); + + const client = h.published()!(entry("a"), "auth-1"); + + expect(client.getSessionId()).toBe("auth-1"); + expect(h.spies.createStores).toHaveBeenCalledWith( + client, + expect.objectContaining({ sessionId: "auth-1" }), + ); + }); + + it("adds the install CIMD metadata URL even with no per-server OAuth", () => { + const h = harness({ + servers: [entry("a")], + clientConfig: { + cimd: { enabled: true, clientMetadataUrl: "https://cimd.example/m" }, + }, + }); + + expect(() => h.published()!(entry("a"))).not.toThrow(); + expect(h.spies.setInspectorClient).toHaveBeenCalled(); + }); + + it("forwards the refresh-token opt-out on its own", () => { + // The last operand of the "is there any per-server OAuth?" chain, so it + // is the only shape that reaches it — every other field short-circuits. + const settings: InspectorServerSettings = { + ...EMPTY_SETTINGS, + oauthRequestRefreshToken: false, + }; + const h = harness({ + servers: [entry("a", { settings })], + persisted: { a: settings }, + }); + + const client = h.published()!(entry("a", { settings })); + expect(client.getServerSettings()?.oauthRequestRefreshToken).toBe(false); + }); + + it("threads the per-server authorization params and endpoint overrides", () => { + const settings: InspectorServerSettings = { + ...EMPTY_SETTINGS, + oauthAuthorizationParams: [{ key: "audience", value: "api" }], + oauthAuthorizationUrl: "https://as.example/authorize", + oauthTokenUrl: "https://as.example/token", + }; + const h = harness({ + servers: [entry("a", { settings })], + persisted: { a: settings }, + }); + + const client = h.published()!(entry("a", { settings })); + expect(client.getServerSettings()).toEqual(settings); + }); + }); + + describe("onToggleConnection", () => { + it("connects a fresh server and clears the stale failure flags", async () => { + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(connectSpy).toHaveBeenCalledTimes(1); + expect(h.spies.setActiveServerId).toHaveBeenCalledWith("a"); + expect(h.spies.setFailedServerId).toHaveBeenCalledWith(undefined); + expect(h.api().connectErrorMessage).toBeUndefined(); + }); + + it("does not re-set the active id when the target is already active", async () => { + const h = harness({ servers: [entry("a")], activeServerId: "a" }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.spies.setActiveServerId).not.toHaveBeenCalled(); + expect(connectSpy).toHaveBeenCalledTimes(1); + }); + + it("disconnects when the target is the live session", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(disconnectSpy).toHaveBeenCalled(); + expect(h.spies.finalizeExplicitDisconnect).toHaveBeenCalled(); + }); + + it("finalizes the disconnect even when the transport close rejects", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + }); + disconnectSpy.mockRejectedValueOnce(new Error("close failed")); + + await act(async () => { + await expect(h.api().onToggleConnection("a")).rejects.toThrow( + "close failed", + ); + }); + + expect(h.spies.finalizeExplicitDisconnect).toHaveBeenCalled(); + }); + + it("is a no-op for an id the catalog does not hold", async () => { + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("missing"); + }); + + expect(connectSpy).not.toHaveBeenCalled(); + expect(h.spies.setInspectorClient).not.toHaveBeenCalled(); + }); + + it("reports an unconfigured enterprise IdP without flagging the card", async () => { + connectSpy.mockRejectedValueOnce( + new EmaClientNotConfiguredError("not_configured"), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain('Cannot connect to "Server a"'); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + }); + + it("retries the connect when the auth challenge is already satisfied", async () => { + connectSpy.mockRejectedValueOnce( + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "unauthorized", + }), + ); + checkSpy.mockResolvedValueOnce(true); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(connectSpy).toHaveBeenCalledTimes(2); + expect(h.spies.prepareOAuthRedirect).not.toHaveBeenCalled(); + }); + + it("redirects when the challenge is unsatisfied", async () => { + const authorizationUrl = new URL("https://as.example/authorize"); + connectSpy.mockRejectedValueOnce( + new AuthRecoveryRequiredError(authorizationUrl, { + reason: "insufficient_scope", + }), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.spies.prepareOAuthRedirect).toHaveBeenCalledWith( + expect.objectContaining({ + serverId: "a", + authKind: "reauth", + authorizationUrl, + preRedirectContext: "connect", + }), + ); + }); + + it("surfaces a throw from the challenge check as the failed connect it is", async () => { + connectSpy.mockRejectedValueOnce( + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "unauthorized", + }), + ); + checkSpy.mockRejectedValueOnce(new Error("discovery exploded")); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + expect(h.api().connectErrorMessage).toBe("discovery exploded"); + expect(disconnectSpy).toHaveBeenCalled(); + expect(h.spies.prepareOAuthRedirect).not.toHaveBeenCalled(); + expect(toastTitles()).toContain('Failed to connect to "Server a"'); + }); + + it("reconnects when a 401 turns out to need no authorization", async () => { + connectSpy.mockRejectedValueOnce(unauthorized()); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(authenticateSpy).toHaveBeenCalled(); + expect(connectSpy).toHaveBeenCalledTimes(2); + }); + + it("redirects when a 401 yields an authorization URL", async () => { + const authUrl = new URL("https://as.example/authorize?x=1"); + connectSpy.mockRejectedValueOnce(unauthorized()); + authenticateSpy.mockResolvedValueOnce(authUrl); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.spies.prepareOAuthRedirect).toHaveBeenCalledWith( + expect.objectContaining({ authorizationUrl: authUrl }), + ); + }); + + it("reports an unconfigured IdP raised by the 401 authorization attempt", async () => { + connectSpy.mockRejectedValueOnce(unauthorized()); + authenticateSpy.mockRejectedValueOnce( + new EmaClientNotConfiguredError("disabled"), + ); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(toastTitles()).toContain('Cannot connect to "Server a"'); + expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a"); + }); + + it("flags the card when the 401 authorization attempt fails outright", async () => { + connectSpy.mockRejectedValueOnce(unauthorized()); + authenticateSpy.mockRejectedValueOnce("registration rejected"); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + expect(h.api().connectErrorMessage).toBe("registration rejected"); + expect(toastTitles()).toContain( + 'OAuth authorization failed for "Server a"', + ); + }); + + it("flags the card on a plain handshake failure", async () => { + connectSpy.mockRejectedValueOnce(new Error("ECONNREFUSED")); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a"); + expect(h.api().connectErrorMessage).toBe("ECONNREFUSED"); + expect(toastTitles()).toContain('Failed to connect to "Server a"'); + }); + + it("stringifies a non-Error challenge-check rejection and survives a failed teardown", async () => { + connectSpy.mockRejectedValueOnce( + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "unauthorized", + }), + ); + checkSpy.mockRejectedValueOnce("discovery blew up"); + // The teardown is best-effort: its rejection must not replace the real + // cause with a close error. + disconnectSpy.mockRejectedValueOnce(new Error("close failed")); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.api().connectErrorMessage).toBe("discovery blew up"); + }); + + it("reports an Error from the 401 authorization attempt and survives a failed teardown", async () => { + connectSpy.mockRejectedValueOnce(unauthorized()); + authenticateSpy.mockRejectedValueOnce(new Error("token endpoint 500")); + disconnectSpy.mockRejectedValueOnce(new Error("close failed")); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.api().connectErrorMessage).toBe("token endpoint 500"); + }); + + it("stringifies a non-Error handshake rejection", async () => { + connectSpy.mockRejectedValueOnce("boom"); + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onToggleConnection("a"); + }); + + expect(h.api().connectErrorMessage).toBe("boom"); + }); + }); + + describe("onDisconnect", () => { + it("is a no-op with no live client", async () => { + const h = harness({ servers: [entry("a")] }); + + await act(async () => { + await h.api().onDisconnect(); + }); + + expect(disconnectSpy).not.toHaveBeenCalled(); + expect(h.spies.finalizeExplicitDisconnect).not.toHaveBeenCalled(); + }); + + it("closes the transport and finalizes the session", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client: lastClient(h), + }); + + await act(async () => { + await h.api().onDisconnect(); + }); + + expect(disconnectSpy).toHaveBeenCalled(); + expect(h.spies.finalizeExplicitDisconnect).toHaveBeenCalled(); + }); + }); + + describe("the session-end effects", () => { + it("tracks the connected server and clears it when the session ends", async () => { + const h = harness({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + }); + await waitFor(() => expect(h.api().connectedServerId).toBe("a")); + + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "disconnected", + }); + await waitFor(() => expect(h.api().connectedServerId).toBeUndefined()); + }); + + it("resets the session-scoped UI state on the client's disconnect event", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + }); + + act(() => { + client.dispatchEvent(new CustomEvent("disconnect")); + }); + + expect(h.spies.setActiveServerId).toHaveBeenCalledWith(undefined); + expect(h.spies.closeConnectionInfoModal).toHaveBeenCalled(); + expect(h.spies.clearResultPanels).toHaveBeenCalled(); + expect(h.spies.resetTabUiState).toHaveBeenCalled(); + expect(h.spies.resetTaskProgress).toHaveBeenCalled(); + expect(h.spies.resetLogLevels).toHaveBeenCalled(); + expect(h.spies.resetOAuthRecoveryState).toHaveBeenCalled(); + }); + + it("closes the outgoing client's transport when it is replaced", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const first = lastClient(h); + h.rerender({ servers: [entry("a")], client: first }); + disconnectSpy.mockClear(); + + h.rerender({ servers: [entry("a")], client: null }); + + await waitFor(() => expect(disconnectSpy).toHaveBeenCalled()); + }); + }); + + describe("the deep-link auto-connect", () => { + it("does nothing without a deep link", async () => { + const h = harness({ servers: [] }); + await waitFor(() => expect(h.spies.addServer).not.toHaveBeenCalled()); + }); + + it("stands down on the OAuth callback path", async () => { + window.history.replaceState({}, "", "/oauth/callback?code=x"); + const h = harness({ servers: [], deepLink: deepLink() }); + await waitFor(() => expect(h.spies.addServer).not.toHaveBeenCalled()); + }); + + it("adds the row, then updates it, then connects — one phase per render", async () => { + const link = deepLink(); + const h = harness({ servers: [], deepLink: link }); + + await waitFor(() => + expect(h.spies.addServer).toHaveBeenCalledWith( + DEEP_LINK_SERVER_ID, + deepLinkConfig, + ), + ); + + // The row hydrates carrying a stale transport from an earlier load. + const stale = entry(DEEP_LINK_SERVER_ID, { + config: { type: "streamable-http", url: "https://old.example/mcp" }, + }); + h.rerender({ servers: [stale], deepLink: link }); + await waitFor(() => + expect(h.spies.updateServer).toHaveBeenCalledWith( + DEEP_LINK_SERVER_ID, + DEEP_LINK_SERVER_ID, + deepLinkConfig, + ), + ); + + const fresh = entry(DEEP_LINK_SERVER_ID, { config: deepLinkConfig }); + h.rerender({ servers: [fresh], deepLink: link }); + await waitFor(() => expect(connectSpy).toHaveBeenCalled()); + }); + + it("swallows the already-exists race and still connects", async () => { + const link = deepLink(); + const addServerImpl = vi + .fn() + .mockRejectedValue(new Error('Server "deep-link" already exists')); + const h = harness({ servers: [], deepLink: link, addServerImpl }); + + await waitFor(() => expect(addServerImpl).toHaveBeenCalled()); + expect(h.api().connectErrorMessage).toBeUndefined(); + }); + + it("records any other add failure on the machine-readable surface", async () => { + const addServerImpl = vi + .fn() + .mockRejectedValue(new Error("catalog is read-only")); + const h = harness({ servers: [], deepLink: deepLink(), addServerImpl }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("catalog is read-only"), + ); + }); + + it("records an update failure", async () => { + const link = deepLink(); + const updateServerImpl = vi.fn().mockRejectedValue("backend 500"); + const stale = entry(DEEP_LINK_SERVER_ID, { + config: { type: "streamable-http", url: "https://old.example/mcp" }, + }); + const h = harness({ + servers: [stale], + deepLink: link, + updateServerImpl, + }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("backend 500"), + ); + }); + + it("does not toggle a session that is already connected to the deep link", async () => { + const fresh = entry(DEEP_LINK_SERVER_ID, { config: deepLinkConfig }); + const h = harness({ + servers: [fresh], + deepLink: deepLink(), + activeServerId: DEEP_LINK_SERVER_ID, + connectionStatus: "connected", + }); + + await waitFor(() => expect(h.api().connectedServerId).toBe("deep-link")); + expect(connectSpy).not.toHaveBeenCalled(); + }); + + it("stringifies a non-Error add failure", async () => { + const addServerImpl = vi.fn().mockRejectedValue("catalog exploded"); + const h = harness({ servers: [], deepLink: deepLink(), addServerImpl }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("catalog exploded"), + ); + }); + + it("records an Error update failure by message", async () => { + const updateServerImpl = vi + .fn() + .mockRejectedValue(new Error("read-only catalog")); + const stale = entry(DEEP_LINK_SERVER_ID, { + config: { type: "streamable-http", url: "https://old.example/mcp" }, + }); + const h = harness({ + servers: [stale], + deepLink: deepLink(), + updateServerImpl, + }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("read-only catalog"), + ); + }); + + it("records a rejection that escapes the connect toggle", async () => { + // Client construction runs outside the toggle's try/catch, so a throw + // there escapes `onToggleConnection` altogether rather than being + // toasted inside it — the case this catch exists for. + const fresh = entry(DEEP_LINK_SERVER_ID, { config: deepLinkConfig }); + const h = harness({ servers: [fresh], deepLink: deepLink() }); + h.spies.destroyStores.mockImplementationOnce(() => { + throw new Error("teardown wedged"); + }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("teardown wedged"), + ); + }); + }); + + describe("onReauthenticateFromBanner", () => { + it("does nothing with no banner raised", () => { + const h = harness({ servers: [entry("a")] }); + act(() => h.api().onReauthenticateFromBanner()); + expect(h.spies.setReAuthBanner).not.toHaveBeenCalled(); + }); + + it("clears the stale OAuth state before reconnecting on lost state", async () => { + const h = harness({ + servers: [entry("a")], + reAuthBanner: { + serverId: "a", + message: "lost", + kind: "lost_authorization_state", + }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + expect(h.spies.setReAuthBanner).toHaveBeenCalledWith(null); + await waitFor(() => expect(clearServerOAuthStateMock).toHaveBeenCalled()); + await waitFor(() => expect(connectSpy).toHaveBeenCalled()); + }); + + it("abandons the lost-state recovery when the clear fails", async () => { + clearServerOAuthStateMock.mockRejectedValueOnce(new Error("storage 500")); + const h = harness({ + servers: [entry("a")], + reAuthBanner: { + serverId: "a", + message: "lost", + kind: "lost_authorization_state", + }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(toastTitles()).toContain( + "Could not clear the stored authorization state", + ), + ); + expect(connectSpy).not.toHaveBeenCalled(); + }); + + it("hands the live client to the clear when the banner names the active server", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + reAuthBanner: { + serverId: "a", + message: "lost", + kind: "lost_authorization_state", + }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(clearServerOAuthStateMock).toHaveBeenCalledWith( + expect.objectContaining({ + inspectorClient: client, + isActiveConnection: true, + }), + ), + ); + }); + + it("stringifies a non-Error clear failure", async () => { + clearServerOAuthStateMock.mockRejectedValueOnce("storage unreachable"); + const h = harness({ + servers: [entry("a")], + reAuthBanner: { + serverId: "a", + message: "lost", + kind: "lost_authorization_state", + }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect( + notificationsMock.show.mock.calls.some( + (c) => c[0]?.message === "storage unreachable", + ), + ).toBe(true), + ); + }); + + it("reconnects directly when the lost-state server is gone from the catalog", async () => { + const h = harness({ + servers: [], + reAuthBanner: { + serverId: "ghost", + message: "lost", + kind: "lost_authorization_state", + }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + expect(clearServerOAuthStateMock).not.toHaveBeenCalled(); + // No catalog row, so the toggle finds no target and stands down. + expect(connectSpy).not.toHaveBeenCalled(); + }); + + it("re-authorizes in place when the session is still connected", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "a", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => expect(pushAuthSpy).toHaveBeenCalled()); + expect(toastTitles()).toContain("Authorization restored"); + }); + + it("redirects when the in-place re-authorization needs the browser", async () => { + const authUrl = new URL("https://as.example/authorize?y=2"); + authenticateSpy.mockResolvedValue(authUrl); + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "a", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(h.spies.prepareOAuthRedirect).toHaveBeenCalledWith( + expect.objectContaining({ serverId: "a", authorizationUrl: authUrl }), + ), + ); + }); + + it("names the server when the in-place re-authorization fails", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + authenticateSpy.mockRejectedValue(new Error("token endpoint 500")); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "a", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(toastTitles()).toContain( + 'OAuth authorization failed for "Server a"', + ), + ); + }); + + it("falls back to an unnamed failure toast for an unknown server", async () => { + const h = harness({ servers: [entry("a")] }); + await act(async () => { + await h.api().onToggleConnection("a"); + }); + const client = lastClient(h); + authenticateSpy.mockRejectedValue("nope"); + // The banner names a server the current list no longer carries, while + // the *session* is still the connected one — the in-place arm runs but + // has no entry to name. + h.rerender({ + servers: [entry("ghost")], + activeServerId: "ghost", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "ghost", message: "lapsed" }, + }); + h.rerender({ + servers: [], + activeServerId: "ghost", + connectionStatus: "connected", + client, + reAuthBanner: { serverId: "ghost", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(toastTitles()).toContain("OAuth authorization failed"), + ); + }); + + it("toggles the connection when the banner's server is not the live one", async () => { + const h = harness({ + servers: [entry("a"), entry("b")], + activeServerId: "a", + connectionStatus: "connected", + reAuthBanner: { serverId: "b", message: "lapsed" }, + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => expect(connectSpy).toHaveBeenCalled()); + }); + }); +}); diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts new file mode 100644 index 000000000..44e2aaed6 --- /dev/null +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -0,0 +1,968 @@ +import { + useCallback, + useEffect, + useLayoutEffect, + useRef, + useState, +} from "react"; +import type { RefObject } from "react"; +import { notifications } from "@mantine/notifications"; +import { InspectorClient } from "@inspector/core/mcp/index.js"; +import type { + ConnectionStatus, + InspectorServerSettings, + MCPServerConfig, + ServerEntry, +} from "@inspector/core/mcp/types.js"; +import { + DEFAULT_MAX_FETCH_REQUESTS, + eraToVersionNegotiation, +} from "@inspector/core/mcp/types.js"; +import { + applyStdioSettingsToConfig, + cleanRoots, + oauthAuthorizationParamsFromSettings, + oauthEndpointOverridesFromSettings, +} from "@inspector/core/mcp/serverList.js"; +import type { ClientConfig } from "@inspector/core/client/types.js"; +import { + getActiveCimdClientMetadataUrl, + getActiveEnterpriseManagedAuthIdp, +} from "@inspector/core/client/types.js"; +import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; +import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; +import type { SessionRef } from "./useSessionRef"; +import type { FetchLogOptions } from "./useInspectorStores"; +import type { LastPersistedSettings } from "./useLastPersistedSettings"; +import type { AppElicitationSession } from "../lib/appElicitationController"; +import type { + PrepareOAuthRedirectArgs, + ReAuthBannerState, + SetupClientForServer, +} from "./useOAuthRecovery"; +import { clearScrollMemory } from "./useScrollMemory"; +import { createWebEnvironment } from "../lib/environmentFactory"; +import { clearOAuthResumeSnapshot } from "../lib/oauthResume"; +import { clearServerOAuthState } from "../lib/clearServerOAuthState"; +import type { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage"; +import { getAuthToken, redirectUrlProvider } from "../lib/authToken"; +import { OAUTH_CALLBACK_PATH, isUnauthorizedError } from "../utils/oauthFlow"; +import { authRecoveryRestoredMessage } from "../utils/oauthUx"; +import { deepLinkConfigEquals } from "../utils/deepLink"; +import type { DeepLink } from "../utils/deepLink"; + +/** + * Handshake telemetry: the "connecting" edge stamps `connectStartRef` and the + * "connected" edge consumes it into `latencyMs`. + * + * Split out of `useConnectionLifecycle` below only because of call order. + * `useOAuthRecovery` stamps the same ref — its `/oauth/callback` reconnect is a + * connect attempt like any other — and runs *before* the lifecycle hook, so the + * ref has to exist earlier than the hook that otherwise owns it. Keeping the + * pair here rather than in `App.tsx` keeps both halves in the file that owns + * connecting. + */ +export function useHandshakeTelemetry(connectionStatus: ConnectionStatus): { + connectStartRef: RefObject; + latencyMs: number | undefined; +} { + const connectStartRef = useRef(undefined); + const [latencyMs, setLatencyMs] = useState(undefined); + + // Capture observed handshake latency at the connecting → connected edge. + // Reset when the status leaves "connected" so the next connect starts + // clean (otherwise a stale latency would render on the next session). + useEffect(() => { + if ( + connectionStatus === "connected" && + connectStartRef.current !== undefined + ) { + setLatencyMs(Date.now() - connectStartRef.current); + connectStartRef.current = undefined; + } else if (connectionStatus !== "connected") { + setLatencyMs(undefined); + } + }, [connectionStatus]); + + return { connectStartRef, latencyMs }; +} + +/** + * The narrow reset surface `useConnectionLifecycle` takes from each hook that + * owns a slice of session-scoped UI state, rather than the individual setters. + * + * Taking the setters one at a time would make this hook depend on nearly + * everything phase 1 extracted, which would decouple nothing (#2129). + */ +export interface SessionResetSurface { + /** Drops the in-flight tool / prompt / resource result panels. */ + clearResultPanels: () => void; + /** Clears per-screen selection, search and filter state. */ + resetTabUiState: () => void; + /** Clears the taskId → progress map. */ + resetTaskProgress: () => void; + /** Drops the re-auth banner and both pending-OAuth slots. */ + resetOAuthRecoveryState: () => void; + /** Re-seeds both log-level controls for the outgoing session. */ + resetLogLevels: () => void; + /** Closes the Connection Info modal so it can't pop back on reconnect. */ + closeConnectionInfoModal: () => void; +} + +export interface UseConnectionLifecycleOptions { + /** The stable session mirror from `useSessionRef` (#2129). */ + sessionRef: SessionRef; + servers: ServerEntry[]; + activeServerId: string | undefined; + inspectorClient: InspectorClient | null; + connectionStatus: ConnectionStatus; + /** Catalog writes the deep-link auto-connect performs before connecting. */ + addServer: (id: string, config: MCPServerConfig) => Promise; + updateServer: ( + id: string, + nextId: string, + config: MCPServerConfig, + ) => Promise; + setActiveServerId: (id: string | undefined) => void; + /** Red-borders a server in the list (#1621). */ + setFailedServerId: (id: string | undefined) => void; + /** Publishes the client this hook constructs back to `App.tsx`. */ + setInspectorClient: (client: InspectorClient | null) => void; + /** Per-session state managers, torn down and rebuilt around each connect. */ + createStores: ( + client: InspectorClient, + fetchLogOptions: FetchLogOptions, + ) => void; + destroyStores: () => void; + /** What each settings write actually put on disk, per server (#2089). */ + lastPersistedSettings: Pick; + /** Install-level client config — the EMA idp and CIMD URL come from it. */ + clientConfig: ClientConfig; + /** + * Opens an app-rendered elicitation session for a newly built client, and + * returns the renderer the client advertises the capability with (#1854). + */ + newAppElicitationSession: () => AppElicitationSession; + /** + * The MCP Apps sandbox URL, or `undefined` once `/api/config` has settled + * and reported none. Whether the client may advertise the nested + * `elicitation` capability is decided at construction from this (#1854). + */ + sandboxUrl: string | undefined; + /** + * Resolves once `/api/config` has settled, so a connect waits for the + * sandbox answer rather than guessing it. + */ + initialConfigSettledRef: RefObject<{ promise: Promise } | null>; + /** Handshake telemetry from `useHandshakeTelemetry` above. */ + connectStartRef: RefObject; + /** + * Where the `/oauth/callback` rebuild reads the connect path from. Published + * here in a layout effect; see `useOAuthRecovery` for why it is a ref. + */ + setupClientForServerRef: RefObject; + /** The validated deep link, or `undefined` when the URL carries none. */ + deepLink: DeepLink | undefined; + + // --- The OAuth recovery surface this hook consumes (#2153). --- + webOAuthStorage: ReturnType; + sessionStorageAdapter: RemoteInspectorClientStorage; + onBeforeOAuthRedirect: (authorizationUrl: URL) => void; + prepareOAuthRedirect: (args: PrepareOAuthRedirectArgs) => void; + finalizeExplicitDisconnect: () => void; + reAuthBanner: ReAuthBannerState | null; + setReAuthBanner: (next: ReAuthBannerState | null) => void; + + /** See `SessionResetSurface`. */ + sessionReset: SessionResetSurface; + /** Seeds the modern per-request log level from a server's settings. */ + seedModernLogLevel: (settings: InspectorServerSettings | undefined) => void; +} + +export interface ConnectionLifecycle { + /** The server that just connected — drives its card's green highlight. */ + connectedServerId: string | undefined; + /** Last connection-level failure, surfaced as `data-error-message`. */ + connectErrorMessage: string | undefined; + /** Connect to `id`, or disconnect when it is already the live session. */ + onToggleConnection: (id: string) => Promise; + /** Header Disconnect: end the live session explicitly. */ + onDisconnect: () => Promise; + /** Re-auth banner action — retry, or clear stale state and reconnect. */ + onReauthenticateFromBanner: () => void; +} + +/** + * The whole connection lifecycle: constructing the `InspectorClient` for a + * server, connecting and disconnecting it, the effects that observe those + * transitions, and the session-scoped reset a disconnect triggers. Lifted out + * of `App.tsx` by phase-2 step 3 of the decomposition (#2154, under + * #2129/#2126). + * + * It is one hook because the pieces share the same edge. `setupClientForServer` + * is the single place a connection is configured, and every entry point — the + * user's toggle, the header's Disconnect, the deep-link auto-connect, the + * re-auth banner, and (through `setupClientForServerRef`) the + * `/oauth/callback` rebuild — has to reach the *same* one, or two of them + * would build differently-configured clients for one server. + * + * Direction is one-way, as it was for `useOAuthRecovery`: this hook consumes + * that one's redirect plumbing (`onBeforeOAuthRedirect`, + * `sessionStorageAdapter`, `prepareOAuthRedirect`) and never the reverse. The + * one thing OAuth recovery needs from here — rebuilding a client — is injected + * back through `setupClientForServerRef`, published in a *layout* effect so it + * is set before any passive effect of the same commit can read it. + */ +export function useConnectionLifecycle({ + sessionRef, + servers, + activeServerId, + inspectorClient, + connectionStatus, + addServer, + updateServer, + setActiveServerId, + setFailedServerId, + setInspectorClient, + createStores, + destroyStores, + lastPersistedSettings, + clientConfig, + newAppElicitationSession, + sandboxUrl, + initialConfigSettledRef, + connectStartRef, + setupClientForServerRef, + deepLink, + webOAuthStorage, + sessionStorageAdapter, + onBeforeOAuthRedirect, + prepareOAuthRedirect, + finalizeExplicitDisconnect, + reAuthBanner, + setReAuthBanner, + sessionReset, + seedModernLogLevel, +}: UseConnectionLifecycleOptions): ConnectionLifecycle { + const [connectedServerId, setConnectedServerId] = useState< + string | undefined + >(undefined); + + // `setupClientForServer` is synchronous and memoized, so a caller that + // awaited the config would still resume with the `sandboxUrl` captured by the + // render it STARTED in — undefined, on the very load this matters for. The + // ref is written every render, so client construction reads the current value + // whichever entry point (connect, deep link, OAuth callback) reached it. + const sandboxUrlRef = useRef(undefined); + // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) + sandboxUrlRef.current = sandboxUrl; + + const { + clearResultPanels, + resetTabUiState, + resetTaskProgress, + resetOAuthRecoveryState, + resetLogLevels, + closeConnectionInfoModal, + } = sessionReset; + + const deepLinkEnsureRef = useRef(false); + const deepLinkUpdateRef = useRef(false); + const deepLinkConnectRef = useRef(false); + // Track the just-connected server so its card gets the green highlight + + // scroll-into-view (#1682). Unlike `failedServerId` (which must survive the + // `disconnect` event a failed connect fires), "connected" is a stable status, + // so a status-driven effect can both set and clear it: set on connect, clear + // whenever the session isn't connected (disconnect, a new attempt's + // "connecting", or an error). + useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161) + setConnectedServerId( + connectionStatus === "connected" ? activeServerId : undefined, + ); + }, [connectionStatus, activeServerId]); + + // Disconnect the previous InspectorClient when it's replaced (server + // switch) or when App unmounts (HMR, tests). Without this the prior + // session's transport — a spawned stdio subprocess, an SSE stream, or + // an HTTP session — stays open until GC eventually lets go. The + // state-manager destroys in `setupClientForServer` only handle the + // listener side; this effect handles the transport side. `disconnect()` + // is the canonical lifecycle hook (InspectorClient has no `destroy()`); + // it closes the transport, clears subscriptions, cancels receiver TTLs. + useEffect(() => { + return () => { + if (inspectorClient) { + void inspectorClient.disconnect(); + } + }; + }, [inspectorClient]); + + // Reset the session-scoped UI state that lives outside the per-server state + // managers, so the next server's screens don't show server A's last result. + // The per-call panels (`toolCallState` / `getPromptState` / + // `readResourceState`) and the optimistic log levels all survive a + // disconnect/reconnect cycle otherwise — see #1368. `latencyMs` is + // intentionally excluded: it resets via the `connectionStatus` effect in + // `useHandshakeTelemetry`, which has its own connecting-edge ref to + // coordinate with. Each piece is dropped through its owner's `reset()` + // rather than through that owner's setters, so this stays one call per + // owner as more session-scoped state accrues (#1394, #2129). + // Does not clear the OAuth resume snapshot — that is tied to an in-flight + // full-page redirect and is cleared on explicit disconnect or consumed on callback. + const resetSessionScopedUiState = useCallback(() => { + clearResultPanels(); + resetTabUiState(); + resetTaskProgress(); + resetLogLevels(); + resetOAuthRecoveryState(); + // Remembered scroll offsets are session-scoped too — drop them so the next + // session's screens start at the top (#1417). + clearScrollMemory(); + }, [ + clearResultPanels, + resetTabUiState, + resetTaskProgress, + resetLogLevels, + resetOAuthRecoveryState, + ]); + + // Reset activeServerId whenever the live session ends. Without this the + // other ServerCards stay `inert` after disconnect — ServerCard dims any + // card whose id differs from `activeServer`. Subscribing to + // InspectorClient's own `disconnect` event covers all three paths + // (explicit toggle, header Disconnect button, mid-session transport + // failure / process exit) and avoids the first-render-clobbers-new-id + // trap that watching connectionStatus has (status starts as + // "disconnected" for the new client before connect() runs). The + // session-scoped panel/level reset rides along here too via + // `resetSessionScopedUiState`. + useEffect(() => { + if (!inspectorClient) return; + const onDisconnect = () => { + setActiveServerId(undefined); + // Drop the open flag too — without this the modal would pop back the + // next time `initializeResult` re-becomes truthy (e.g. reconnect). + closeConnectionInfoModal(); + resetSessionScopedUiState(); + }; + inspectorClient.addEventListener("disconnect", onDisconnect); + return () => { + inspectorClient.removeEventListener("disconnect", onDisconnect); + }; + }, [ + inspectorClient, + setActiveServerId, + closeConnectionInfoModal, + resetSessionScopedUiState, + ]); + + // Last connection-level error message, surfaced as `data-error-message` on + // the InspectorView header so an automated driver can read *why* a connect + // failed without scraping a transient toast. Cleared on the next connect + // attempt and on successful connection. + const [connectErrorMessage, setConnectErrorMessage] = useState< + string | undefined + >(undefined); + // Named writer so a single call site can be extended later (e.g. telemetry) + // and the intent (record a connection-level failure) stays explicit. + const recordConnectError = useCallback((message: string) => { + setConnectErrorMessage(message); + }, []); + + // Wire up + tear down per active server. Called by `onToggleConnection` + // when the user switches targets. Returns the new client so the toggle + // can call `connect()` against it before React re-renders. + const setupClientForServer = useCallback( + (server: ServerEntry, sessionId?: string): InspectorClient => { + // Tear down the previous session's managers before building the new + // client — each destroy() unsubscribes from the old client's events, and + // doing it here (rather than leaving it to `createStores` below, which + // also tears down whatever is live) means a throw while constructing the + // client leaves nothing still listening to the outgoing one. A no-op on + // the first call. + destroyStores(); + + const { environment, logger } = createWebEnvironment( + getAuthToken(), + redirectUrlProvider, + onBeforeOAuthRedirect, + ); + // The settings node persisted in mcp.json for this server — distinct + // from the InspectorClient options we're about to derive from it. + // + // Through the tracker, not `server.settings` directly: the entry only + // advances on a successful list read, so a write that landed while reads + // were failing — including one made from the settings modal for a server + // that was not connected at the time — would otherwise be undone at the + // next connect, which builds the client from that frozen entry. This is + // the one place the whole connection is configured, so every construction + // path (connect, reconnect, OAuth resume) goes through it (#2089). + const savedSettings = + lastPersistedSettings.resolve(server.id) ?? server.settings; + const activeIdp = getActiveEnterpriseManagedAuthIdp(clientConfig); + const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig); + // Flatten the persisted settings into the InspectorClient options shape. + // Empty / zero values stay unset so the SDK defaults apply. + // Per-server default `_meta` is already a JSON object (#1910) — no + // pair-array flattening left to do; `{}` means "no defaults". + const defaultMetadata = savedSettings?.metadata; + const serverAuthorizationParams = savedSettings + ? oauthAuthorizationParamsFromSettings(savedSettings) + : undefined; + const serverEndpointOverrides = savedSettings + ? oauthEndpointOverridesFromSettings(savedSettings) + : undefined; + const oauthFromServer = + savedSettings && + (savedSettings.oauthClientId || + savedSettings.oauthClientSecret || + savedSettings.oauthScopes || + serverAuthorizationParams || + serverEndpointOverrides || + savedSettings.enterpriseManaged || + savedSettings.oauthRequestRefreshToken === false) + ? { + ...(savedSettings.oauthClientId && { + clientId: savedSettings.oauthClientId, + }), + ...(savedSettings.oauthClientSecret && { + clientSecret: savedSettings.oauthClientSecret, + }), + ...(savedSettings.oauthScopes && { + scope: savedSettings.oauthScopes, + }), + ...(serverAuthorizationParams && { + authorizationParams: serverAuthorizationParams, + }), + ...serverEndpointOverrides, + ...(savedSettings.enterpriseManaged && { + enterpriseManaged: true, + }), + // #2068: only the explicit opt-out is forwarded; omitting the key + // leaves the provider's default (declare `refresh_token`) in place. + ...(savedSettings.oauthRequestRefreshToken === false && { + requestRefreshToken: false, + }), + } + : undefined; + const oauth = + oauthFromServer || activeCimdUrl + ? { + ...(oauthFromServer ?? {}), + ...(activeCimdUrl && { clientMetadataUrl: activeCimdUrl }), + } + : undefined; + // The stdio `env` / `cwd` are edited as *settings* but stored on — and + // read by the transport from — *config*, so the tracker's account of the + // former has to be carried onto the latter. `server.config` comes off the + // same frozen `servers` entry `server.settings` does, so without this a + // save that landed while list reads were failing spawns the child process + // with the pre-save environment while the modal, re-seeded from the + // tracker, shows the new one (#2096). Same mapping the PUT route applies + // when persisting it, from the same helper. + const effectiveConfig = applyStdioSettingsToConfig( + server.config, + savedSettings, + ); + const client = new InspectorClient(effectiveConfig, { + environment, + // The Tasks tab needs the receiver-task pipeline; the + // requestor-task list comes from the client's task store. + receiverTasks: true, + // Sampling / elicitation are on by default; keep the parameterized + // options off until the UI grows the surface to render them. + elicit: { form: true, url: true }, + // Web only, and only when the sandbox renderer is actually available: + // supplying this advertises the nested MCP Apps `elicitation` + // capability, and a client that cannot host an app must not claim it + // (#1854). Callers await `initialConfigSettled` first, so `sandboxUrl` + // here means "confirmed absent" rather than "not known yet" — a + // connection that reaches this with no sandbox behaves like the + // CLI/TUI: native elicitation queue, no claim made to the server. + ...(sandboxUrlRef.current && { + appElicitation: newAppElicitationSession().render, + }), + // Always advertise the roots capability (even with no configured + // roots) so the server can issue roots/list and receive + // roots/list_changed; the configured roots are the answer to + // roots/list. Empty-uri rows are dropped before they reach the wire. + roots: cleanRoots(savedSettings?.roots ?? []), + ...(savedSettings && + savedSettings.requestTimeout > 0 && { + timeout: savedSettings.requestTimeout, + }), + ...(defaultMetadata && + Object.keys(defaultMetadata).length > 0 && { + defaultMetadata, + }), + ...(oauth && { oauth }), + ...(activeIdp && { + enterpriseManagedAuth: { idp: activeIdp }, + }), + ...(clientConfig.enterpriseManagedAuth && { + installEnterpriseManagedAuth: clientConfig.enterpriseManagedAuth, + }), + ...(savedSettings && { serverSettings: savedSettings }), + // Per-server protocol era (SEP §7.8) → SDK versionNegotiation. Absent + // settings or an unset era default to legacy inside + // eraToVersionNegotiation / the InspectorClient constructor (#1626). + ...(savedSettings?.protocolEra && { + versionNegotiation: eraToVersionNegotiation( + savedSettings.protocolEra, + ), + }), + // Per-server advertised-extension overrides (#1739). Absent/empty falls + // back to the registry defaults in the InspectorClient constructor. + ...(savedSettings?.advertisedExtensions && + Object.keys(savedSettings.advertisedExtensions).length > 0 && { + advertisedExtensions: savedSettings.advertisedExtensions, + }), + // Set on the `/oauth/callback` rebuild so the client's `saveSession` + // events (and any later persistence) key off the same OAuth authId + // the pre-redirect page saved under. + ...(sessionId && { sessionId }), + }); + + setInspectorClient(client); + // #1629: seed the live modern per-request log level from the server + // setting so the Logs-tab control reflects what the client stamps by + // default (the client was seeded the same way in its constructor). "off" + // means not opted in (null). Only affects modern connections. + seedModernLogLevel(savedSettings); + // Wire session storage so the fetch log survives the OAuth redirect. + // When `sessionId` is supplied (the `/oauth/callback` rebuild) the prior + // page's `auth` entries are restored on construction; the actual save is + // driven synchronously from `onBeforeOAuthRedirect` above (keyed by the + // same authId). `createStores` points `fetchLogRef` at the new instance + // so that hook reads the current log. + createStores(client, { + sessionStorage: sessionStorageAdapter, + logger, + maxFetchRequests: + savedSettings?.maxFetchRequests ?? DEFAULT_MAX_FETCH_REQUESTS, + ...(sessionId && { sessionId }), + }); + + return client; + }, + [ + createStores, + destroyStores, + setInspectorClient, + seedModernLogLevel, + sessionStorageAdapter, + onBeforeOAuthRedirect, + clientConfig, + newAppElicitationSession, + lastPersistedSettings, + ], + ); + // Publish it to the `/oauth/callback` effect, which needs to rebuild the + // client for the server that started the flow and cannot reach a callback + // declared this far down. + // + // A *layout* effect, not a render-phase write and not a passive one. React + // runs every layout effect before any passive effect of the same commit, and + // the callback effect inside `useOAuthRecovery` is passive — so this is + // always published before its first read, without the render-phase ref + // mutation the compiler rule (rightly) rejects. + useLayoutEffect(() => { + setupClientForServerRef.current = setupClientForServer; + }, [setupClientForServerRef, setupClientForServer]); + + const onToggleConnection = useCallback( + async (id: string) => { + // Whether this client may advertise app-rendered elicitation is decided + // at construction and cannot be revised afterwards, so wait for the fact + // rather than guess it (see `initialConfigSettledRef`). Already resolved + // by the time any human clicks; this only orders a deep-link auto-connect + // that races the same page load. + await initialConfigSettledRef.current?.promise; + // Same server, already connected → disconnect. + if ( + id === activeServerId && + connectionStatus === "connected" && + inspectorClient + ) { + try { + await inspectorClient.disconnect(); + } finally { + finalizeExplicitDisconnect(); + } + return; + } + + // Read from the ref so a caller that already awaited an + // addServer/updateServer in the same async tick (e.g. the deep-link + // auto-connect IIFE) sees the freshly-mutated list, not the stale array + // captured by this callback's closure. + const target = sessionRef.current.servers.find((s) => s.id === id); + if (!target) return; + + // Always rebuild the InspectorClient on a (re)connect so the latest + // `target.settings` (headers, metadata, timeouts, OAuth credentials) + // are picked up. Reusing the previous client object would freeze the + // settings at the moment it was first constructed, which would be + // surprising right after the user edited them in the settings modal. + const client = setupClientForServer(target); + if (id !== activeServerId) { + setActiveServerId(id); + } + // A new connection attempt has begun: clear any previous failure flag so + // the red border on the last-failed card is removed (#1621). If this + // attempt also fails, the catch below re-sets it for this server. + setFailedServerId(undefined); + // Clear the machine-readable connect error for the same reason; a fresh + // attempt starts from a clean `data-error-message`. + setConnectErrorMessage(undefined); + + connectStartRef.current = Date.now(); + try { + // `settings.connectionTimeout` is consumed inside InspectorClient.connect + // (Promise.race + transport teardown live there now), so this branch + // stays unaware of the per-server timeout. TUI/CLI consumers get the + // same behavior by reading from `serverSettings` on the client. + await client.connect(); + } catch (err) { + // Handshake-only. A mid-session transport failure does not throw; the + // client's `error` event surfaces those, consumed via + // `useInspectorClient`'s `lastError` and toasted in the effect above + // (#1323). + connectStartRef.current = undefined; + + if (isEmaClientNotConfiguredError(err)) { + notifications.show({ + title: `Cannot connect to "${target.name}"`, + message: err.message, + color: "red", + autoClose: false, + }); + return; + } + + // A 401 from an OAuth-protected server means we have no (valid) token + // yet. Kick off the authorization-code flow: `authenticate()` runs + // discovery + DCR (proxied through the backend), then redirects the + // whole page to the auth server via `BrowserNavigation`. Persist the + // initiating server id first so the `/oauth/callback` load can resume + // against the right client. The redirect unloads this page, so there's + // nothing to do after the await on the success path. + if (err instanceof AuthRecoveryRequiredError) { + try { + if (await client.checkAuthChallengeSatisfied(err.authChallenge)) { + connectStartRef.current = Date.now(); + await client.connect(); + return; + } + } catch (recoveryErr) { + // Both awaits above are unguarded connect work sitting inside a + // `catch`, so a rejection escapes `onToggleConnection` altogether: + // no toast, no red border, no sidebar — the #2108 failure mode in + // its most invisible form. Surface it as the failed connect attempt + // it is. A throw from `checkAuthChallengeSatisfied` lands here too + // rather than falling through to `prepareOAuthRedirect`: it is not + // the same as the challenge being *unsatisfied*, and navigating the + // whole page away on the strength of an error would bury it. + connectStartRef.current = undefined; + // Tear the session down before reporting, as the sibling OAuth + // catch below does. The outer `connect()` rejected with an + // auth-recovery error, which deliberately holds the status at + // `"connecting"` rather than moving it to `"error"` — so if the + // challenge check is what rejected, nothing else ever ends the + // attempt and the toggle spins while the active-server lock is + // held. The fetch log survives a disconnect, so the Network + // diagnostics this issue is about are unaffected. + await client.disconnect().catch(() => {}); + setFailedServerId(id); + const message = + recoveryErr instanceof Error + ? recoveryErr.message + : String(recoveryErr); + setConnectErrorMessage(message); + notifications.show({ + title: `Failed to connect to "${target.name}"`, + message, + color: "red", + }); + return; + } + prepareOAuthRedirect({ + serverId: id, + authKind: "reauth", + authorizationUrl: err.authorizationUrl, + preRedirectContext: "connect", + client, + }); + return; + } + + if (isUnauthorizedError(err)) { + try { + const authUrl = await client.authenticate(); + if (authUrl === undefined) { + connectStartRef.current = Date.now(); + await client.connect(); + } else { + prepareOAuthRedirect({ + serverId: id, + authKind: "reauth", + authorizationUrl: authUrl, + preRedirectContext: "connect", + client, + }); + } + return; + } catch (authErr) { + clearOAuthResumeSnapshot(); + await client.disconnect().catch(() => {}); + if (isEmaClientNotConfiguredError(authErr)) { + notifications.show({ + title: `Cannot connect to "${target.name}"`, + message: authErr.message, + color: "red", + autoClose: false, + }); + return; + } + // The connect attempt failed, same as any other handshake error — + // flag the card (#1621) and, with it, open the monitoring sidebar + // onto the OAuth requests that explain the failure (#2108). This + // leg never reaches the `"error"` connection status (the + // `disconnect()` above settles it at `"disconnected"`), so this + // flag is the only signal the view has that a connect attempt died. + setFailedServerId(id); + const message = + authErr instanceof Error ? authErr.message : String(authErr); + setConnectErrorMessage(message); + notifications.show({ + title: `OAuth authorization failed for "${target.name}"`, + message, + color: "red", + }); + return; + } + } + + // Non-auth handshake error: toast so the user sees what went wrong + // instead of the ConnectionToggle silently reverting to + // "disconnected", and flag the card with a red border (#1621). + setFailedServerId(id); + const message = err instanceof Error ? err.message : String(err); + setConnectErrorMessage(message); + notifications.show({ + title: `Failed to connect to "${target.name}"`, + message, + color: "red", + }); + } + }, + [ + sessionRef, + activeServerId, + connectionStatus, + inspectorClient, + initialConfigSettledRef, + connectStartRef, + setupClientForServer, + setActiveServerId, + setFailedServerId, + prepareOAuthRedirect, + finalizeExplicitDisconnect, + ], + ); + + const onDisconnect = useCallback(async () => { + if (!inspectorClient) return; + try { + await inspectorClient.disconnect(); + } finally { + finalizeExplicitDisconnect(); + } + }, [inspectorClient, finalizeExplicitDisconnect]); + + // Deep-link auto-connect (the URL-driven case of #1183). `useServers` + // 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 the session ref, 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; + + const existing = servers.find((s) => s.id === deepLink.serverId); + if (!existing) { + if (deepLinkEnsureRef.current) return; + deepLinkEnsureRef.current = true; + 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; + // Connect unless we're already *connected* to the deep-link server. Gating + // on `activeServerId` identity alone would skip the connect when a prior + // session restored `activeServerId` to the `deep-link` id while the socket + // is disconnected — a reload of the same deep-link URL would then silently + // never connect. `onToggleConnection` only disconnects when the id is the + // active one AND the status is connected, so this condition also avoids + // toggling a live connection off. + const alreadyConnected = + activeServerId === deepLink.serverId && connectionStatus === "connected"; + if (!alreadyConnected) { + // eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161) + 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, + activeServerId, + connectionStatus, + addServer, + updateServer, + onToggleConnection, + recordConnectError, + ]); + + const onReauthenticateFromBanner = useCallback(() => { + if (!reAuthBanner) return; + const serverId = reAuthBanner.serverId; + const bannerKind = reAuthBanner.kind; + setReAuthBanner(null); + + // Lost-authorization-state recovery (#1808). The stale half of the flow + // (code verifier without discovery state, possibly a stale registration) + // would make a plain retry fail the same way, so drop the persisted OAuth + // state for this server first, then start a fresh authorization. This + // banner is only raised from the `/oauth/callback` failure path, where the + // session never reached "connected", so connecting is always the right + // toggle direction here. + if (bannerKind === "lost_authorization_state") { + void (async () => { + const server = sessionRef.current.servers.find( + (s) => s.id === serverId, + ); + if (server) { + try { + await clearServerOAuthState({ + config: server.config, + inspectorClient: + serverId === activeServerId ? inspectorClient : null, + isActiveConnection: serverId === activeServerId, + oauthStorage: webOAuthStorage, + }); + } catch (err) { + notifications.show({ + title: "Could not clear the stored authorization state", + message: err instanceof Error ? err.message : String(err), + color: "red", + // The banner is already dismissed and the flow is dead, so this + // is the only remaining explanation — don't time it out. + autoClose: false, + }); + return; + } + } + await onToggleConnection(serverId); + })(); + return; + } + + if ( + serverId === activeServerId && + connectionStatus === "connected" && + inspectorClient + ) { + void (async () => { + const server = servers.find((s) => s.id === serverId); + try { + const authUrl = await inspectorClient.authenticate(); + if (authUrl === undefined) { + await inspectorClient.pushRemoteAuthState(); + notifications.show({ + title: "Authorization restored", + message: authRecoveryRestoredMessage(), + color: "green", + autoClose: 4000, + }); + return; + } + prepareOAuthRedirect({ + serverId, + authKind: "reauth", + authorizationUrl: authUrl, + }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + notifications.show({ + title: server + ? `OAuth authorization failed for "${server.name}"` + : "OAuth authorization failed", + message, + color: "red", + }); + } + })(); + return; + } + + void onToggleConnection(serverId); + }, [ + sessionRef, + reAuthBanner, + activeServerId, + connectionStatus, + inspectorClient, + servers, + prepareOAuthRedirect, + onToggleConnection, + webOAuthStorage, + setReAuthBanner, + ]); + + return { + connectedServerId, + connectErrorMessage, + onToggleConnection, + onDisconnect, + onReauthenticateFromBanner, + }; +} From 005e4c655acc148e2c6c1cac17e12675c1db6b7d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 01:14:56 -0400 Subject: [PATCH 2/3] chore(web): address Copilot review on #2154 - Terminate the two banner-path `onToggleConnection` calls through a `retryConnect` helper. The toggle handles every handshake failure, but client construction runs ahead of its try/catch, so a throw there escaped into a discarded promise with the banner already dismissed. Recorded on `connectErrorMessage` rather than toasted, matching the deep-link phases' own catch, so the move stays inert. - Add the unsettled-`/api/config` race test: a connect that starts before the sandbox answer is held at the gate and builds from the URL as of the release, which is what `sandboxUrlRef` exists for. - Correct the harness's `client` doc, which described a last-constructed fallback the harness never implemented. Signed-off-by: cliffhall --- .../src/hooks/useConnectionLifecycle.test.tsx | 100 +++++++++++++++++- .../web/src/hooks/useConnectionLifecycle.ts | 30 +++++- 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index 3839cfc12..e09a44b97 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -101,13 +101,20 @@ interface HarnessProps { activeServerId?: string; connectionStatus?: ConnectionStatus; /** - * The live client. `undefined` means "whatever the last - * `setupClientForServer` built", which is what App.tsx's `inspectorClient` - * state settles to a render later. + * The live client, i.e. what App.tsx holds in `inspectorClient`. There is no + * implicit "whatever was last constructed" fallback — the hook publishes + * through `setInspectorClient`, which is a spy here, so a test that wants a + * live session rerenders with the client it read back out of that spy (see + * `lastClient`), the same render-later settle App.tsx has. */ client?: InspectorClient | null; clientConfig?: ClientConfig; sandboxUrl?: string; + /** + * The `/api/config` gate a connect awaits. Defaults to already-settled; + * supply a pending promise to hold a connect at that gate. + */ + configSettled?: Promise; deepLink?: DeepLink; reAuthBanner?: { serverId: string; @@ -177,7 +184,9 @@ function harness(initial: HarnessProps = {}): Harness { // dependency the real one never has. const setupClientForServerRef = useRef(null); const initialConfigSettledRef = useRef<{ promise: Promise }>(null); - initialConfigSettledRef.current ??= { promise: Promise.resolve() }; + initialConfigSettledRef.current ??= { + promise: p.configSettled ?? Promise.resolve(), + }; const sessionReset: SessionResetSurface = { clearResultPanels: s.clearResultPanels, resetTabUiState: s.resetTabUiState, @@ -404,6 +413,43 @@ describe("useConnectionLifecycle", () => { expect(h.spies.newAppElicitationSession).not.toHaveBeenCalled(); }); + it("waits for the config gate, then reads the sandbox URL as of then", async () => { + // The whole reason `onToggleConnection` awaits the gate and + // `setupClientForServer` reads `sandboxUrlRef` rather than a captured + // value: on the load this matters for, the connect starts before + // `/api/config` has answered. Guessing "no sandbox" there would strand + // the session on the native elicitation form despite having one. + let settle!: () => void; + const gate = new Promise((resolve) => (settle = resolve)); + const props: HarnessProps = { + servers: [entry("a")], + configSettled: gate, + sandboxUrl: undefined, + }; + const h = harness(props); + + let toggled: Promise; + act(() => { + toggled = h.api().onToggleConnection("a"); + }); + // Held at the gate — nothing constructed, nothing connected. + expect(h.spies.setInspectorClient).not.toHaveBeenCalled(); + expect(connectSpy).not.toHaveBeenCalled(); + + // `/api/config` answers: there IS a sandbox after all. + h.rerender({ ...props, sandboxUrl: "http://localhost:6275/sandbox" }); + + await act(async () => { + settle(); + await toggled; + }); + + expect(connectSpy).toHaveBeenCalledTimes(1); + // Built from the URL as of the release, not the `undefined` in scope when + // the toggle was called. + expect(h.spies.newAppElicitationSession).toHaveBeenCalled(); + }); + it("carries the OAuth session id onto both the client and its stores", () => { const h = harness({ servers: [entry("a")] }); @@ -1190,6 +1236,52 @@ describe("useConnectionLifecycle", () => { ); }); + it("records a construction throw instead of leaving it unhandled", async () => { + // Client construction runs ahead of the toggle's try/catch, so a throw + // there escapes `onToggleConnection`. Both banner call sites discard the + // promise, so without `retryConnect` this is an unhandled rejection with + // the banner already dismissed and nothing left to explain it. + const h = harness({ + servers: [entry("a"), entry("b")], + activeServerId: "a", + connectionStatus: "connected", + reAuthBanner: { serverId: "b", message: "lapsed" }, + }); + h.spies.destroyStores.mockImplementationOnce(() => { + throw new Error("teardown wedged"); + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("teardown wedged"), + ); + }); + + it("records a construction throw on the lost-state retry too", async () => { + const h = harness({ + servers: [entry("a")], + reAuthBanner: { + serverId: "a", + message: "lost", + kind: "lost_authorization_state", + }, + }); + h.spies.destroyStores.mockImplementationOnce(() => { + throw "teardown wedged"; + }); + + await act(async () => { + h.api().onReauthenticateFromBanner(); + }); + + await waitFor(() => + expect(h.api().connectErrorMessage).toBe("teardown wedged"), + ); + }); + it("toggles the connection when the banner's server is not the live one", async () => { const h = harness({ servers: [entry("a"), entry("b")], diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 44e2aaed6..934b73e65 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -862,6 +862,30 @@ export function useConnectionLifecycle({ recordConnectError, ]); + /** + * The banner's connect, terminated. `onToggleConnection` handles every + * failure the *handshake* can produce, but client construction runs ahead of + * its try/catch — so a throw there (a wedged store teardown, a config the + * `InspectorClient` constructor rejects) escapes it. Both banner call sites + * discard the promise, which would make that an unhandled rejection with the + * banner already dismissed and nothing left to explain it. + * + * Recorded on `connectErrorMessage` rather than toasted, matching the + * deep-link phases' own catch: the toggle already toasts everything it + * handles, so a toast here would be new user-visible behavior in what is + * otherwise an inert move. + */ + const retryConnect = useCallback( + async (serverId: string) => { + try { + await onToggleConnection(serverId); + } catch (err) { + recordConnectError(err instanceof Error ? err.message : String(err)); + } + }, + [onToggleConnection, recordConnectError], + ); + const onReauthenticateFromBanner = useCallback(() => { if (!reAuthBanner) return; const serverId = reAuthBanner.serverId; @@ -901,7 +925,7 @@ export function useConnectionLifecycle({ return; } } - await onToggleConnection(serverId); + await retryConnect(serverId); })(); return; } @@ -944,16 +968,16 @@ export function useConnectionLifecycle({ return; } - void onToggleConnection(serverId); + void retryConnect(serverId); }, [ sessionRef, reAuthBanner, + retryConnect, activeServerId, connectionStatus, inspectorClient, servers, prepareOAuthRedirect, - onToggleConnection, webOAuthStorage, setReAuthBanner, ]); From c37e795dca99a2218db4be6b3211d8997badcead Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 01:32:44 -0400 Subject: [PATCH 3/3] test(web): assert the deep-link 409 race actually reaches the connect phase Copilot round 2: the "swallows the already-exists race and still connects" test only asserted that nothing was recorded, so a deep link that swallowed the 409 and then stopped forever would have stayed green. Rerender with the hydrated row and wait for the connect. Signed-off-by: cliffhall --- clients/web/src/hooks/useConnectionLifecycle.test.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/clients/web/src/hooks/useConnectionLifecycle.test.tsx b/clients/web/src/hooks/useConnectionLifecycle.test.tsx index e09a44b97..f09c020d2 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.test.tsx +++ b/clients/web/src/hooks/useConnectionLifecycle.test.tsx @@ -917,6 +917,15 @@ describe("useConnectionLifecycle", () => { await waitFor(() => expect(addServerImpl).toHaveBeenCalled()); expect(h.api().connectErrorMessage).toBeUndefined(); + + // A 409 means the row is already on disk, so hydration surfaces it on a + // later render and the connect phase must still run. Asserting only that + // nothing was recorded would stay green for a deep link that swallows the + // error and then stops forever. + const fresh = entry(DEEP_LINK_SERVER_ID, { config: deepLinkConfig }); + h.rerender({ servers: [fresh], deepLink: link, addServerImpl }); + await waitFor(() => expect(connectSpy).toHaveBeenCalled()); + expect(h.api().connectErrorMessage).toBeUndefined(); }); it("records any other add failure on the machine-readable surface", async () => {