From 5f875e33930069a3dbccb75d700958ad5850580d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 23:19:49 -0400 Subject: [PATCH] chore(web): address Copilot review on #2153 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Publish `setupClientForServerRef` from a layout effect rather than during render. React runs every layout effect before any passive effect of the same commit, and the /oauth/callback effect is passive, so the seam is still published before its first read — without the render-phase ref mutation the compiler rule rejects. This was the one `react-hooks/refs` finding the PR introduced, so its suppression is gone rather than deferred to #2161 under a justification that did not describe it. - Narrow the hook's `fetchLogRef` option to `FetchRequestSource`, the `getFetchRequests` slice it actually consumes, mirroring `InspectorClientEventSource` in useProgressToasts. That removes the test harness's `as unknown as` cast, and the narrowed contract is real: it immediately rejected the loosely-typed fixture, which is now a properly typed `FetchRequestEntry`. - Make the test harness hold both refs across renders and publish them from a layout effect, as App.tsx does. The deferred-publication test previously passed because `Probe` built a fresh ref each render and the rerender also changed `servers`; it now asserts what the guard is actually for — a run that cannot exchange must not spend the single-use code — and re-triggers on the `servers` change that carries it in production. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- clients/web/src/App.tsx | 22 +++--- .../web/src/hooks/useOAuthRecovery.test.tsx | 69 ++++++++++++++----- clients/web/src/hooks/useOAuthRecovery.ts | 11 ++- 3 files changed, 74 insertions(+), 28 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index fde3b3bd..b7108b27 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, + useLayoutEffect, useMemo, useRef, useState, @@ -636,11 +637,9 @@ function App() { ); /** - * Filled in below, once the connect path's `setupClientForServer` exists — + * Published below, once the connect path's `setupClientForServer` exists — * it reads `onBeforeOAuthRedirect` and `sessionStorageAdapter` out of the - * hook we are about to call, so it cannot be declared before it. The - * `/oauth/callback` effect only ever reads this from an effect, well after - * the render that writes it. + * hook we are about to call, so it cannot be declared before it. */ const setupClientForServerRef = useRef(null); @@ -1228,11 +1227,16 @@ function App() { ); // 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. Written during render — the same latest-ref shape - // `sandboxUrlRef` above uses — because the only reader is an effect, which - // React runs strictly after the render that writes this. - // 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) - setupClientForServerRef.current = setupClientForServer; + // 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) => { diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 5fd5bc69..08b1c9bd 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -3,11 +3,13 @@ import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client"; import { InspectorClient } from "@inspector/core/mcp/index.js"; import type { ConnectionStatus, + FetchRequestEntry, ServerEntry, } from "@inspector/core/mcp/types.js"; import type { AuthChallenge } from "@inspector/core/auth/challenge.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; +import { useLayoutEffect, useRef } from "react"; import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; import { OAUTH_RESUME_KEY, @@ -18,7 +20,12 @@ import type { StepUpSource } from "../utils/stepUp"; import { EMPTY_SETTINGS } from "../utils/serverSettingsDefaults"; import { useSessionRef } from "./useSessionRef"; import { useTabUiState } from "./useTabUiState"; -import { useOAuthRecovery, type OAuthRecovery } from "./useOAuthRecovery"; +import { + useOAuthRecovery, + type FetchRequestSource, + type OAuthRecovery, + type SetupClientForServer, +} from "./useOAuthRecovery"; // --- Module doubles --------------------------------------------------------- // Everything the hook reaches outside React: the toast layer, the two storage @@ -100,6 +107,16 @@ const challenge = ( reason: AuthChallenge["reason"] = "unauthorized", ): AuthChallenge => ({ reason }); +/** One Network-log row, as the pre-redirect flush would find it. */ +const fetchEntry = (id: string): FetchRequestEntry => ({ + id, + timestamp: new Date(0), + method: "POST", + url: "https://as.example/token", + requestHeaders: {}, + category: "auth", +}); + /** * A stand-in `InspectorClient`. * @@ -138,7 +155,7 @@ interface HarnessProps { connectionStatus?: ConnectionStatus; /** Left null to exercise the "connect path not wired yet" callback arm. */ setupClient?: (server: ServerEntry) => InspectorClient; - fetchRequests?: unknown[]; + fetchRequests?: FetchRequestEntry[]; /** Set to drop the Network log entirely, as before the first connect. */ noFetchLog?: boolean; } @@ -173,19 +190,24 @@ function harness(initial: HarnessProps = {}): Harness { inspectorClient: client, }); const { ui, setUi, activeTab, setActiveTab } = useTabUiState(); - const fetchLogRef = { - current: p.noFetchLog + const fetchLogRef = useRef(null); + // Both refs are held across renders and published from a layout effect, + // exactly as App.tsx does — a fresh object per render would give the + // callback effect a changing dependency the real one never has, and the + // deferred-publication test below would then be passing for the wrong + // reason. + const setupClientForServerRef = useRef(null); + useLayoutEffect(() => { + fetchLogRef.current = p.noFetchLog ? null - : { getFetchRequests: () => p.fetchRequests ?? [] }, - } as unknown as Parameters[0]["fetchLogRef"]; - const setupRef = { - current: p.setupClient + : { getFetchRequests: () => p.fetchRequests ?? [] }; + setupClientForServerRef.current = p.setupClient ? (server: ServerEntry) => { spies.setupClient(server); return p.setupClient!(server); } - : null, - }; + : null; + }); latest = useOAuthRecovery({ sessionRef, servers: p.servers ?? [], @@ -199,7 +221,7 @@ function harness(initial: HarnessProps = {}): Harness { fetchLogRef, connectStartRef: { current: undefined }, initialConfigSettledRef: { current: { promise: Promise.resolve() } }, - setupClientForServerRef: setupRef, + setupClientForServerRef, setActiveServerId: spies.setActiveServerId, setFailedServerId: spies.setFailedServerId, clearResultPanels: spies.clearResultPanels, @@ -423,17 +445,18 @@ describe("useOAuthRecovery", () => { describe("onBeforeOAuthRedirect", () => { it("saves the pre-redirect fetch log under the authorization state's authId", () => { - const h = harness({ fetchRequests: [{ id: 1 }] }); + const entries = [fetchEntry("f1")]; + const h = harness({ fetchRequests: entries }); const url = new URL(`https://as.example/authorize?state=${AUTH_ID}`); act(() => h.api().onBeforeOAuthRedirect(url)); expect(saveSessionMock).toHaveBeenCalledWith( AUTH_ID, - expect.objectContaining({ fetchRequests: [{ id: 1 }] }), + expect.objectContaining({ fetchRequests: entries }), ); }); it("does nothing without a parseable state parameter", () => { - const h = harness({ fetchRequests: [{ id: 1 }] }); + const h = harness({ fetchRequests: [fetchEntry("f1")] }); act(() => h.api().onBeforeOAuthRedirect(new URL(AUTH_URL))); act(() => h @@ -471,7 +494,7 @@ describe("useOAuthRecovery", () => { it("swallows a failed save", async () => { saveSessionMock.mockRejectedValue(new Error("offline")); - const h = harness({ fetchRequests: [{ id: 1 }] }); + const h = harness({ fetchRequests: [fetchEntry("f1")] }); act(() => h .api() @@ -1068,16 +1091,26 @@ describe("useOAuthRecovery", () => { }); }; - it("does nothing until the connect path has published its client builder", async () => { + it("keeps the code unspent when no client builder has been published", async () => { onCallbackUrl(`?code=abc&state=${AUTH_ID}`); snapshot(); const client = fakeClient(); + // App.tsx publishes the builder from a layout effect, so this arm is + // defensive rather than reachable there. What it has to guarantee is + // that the guard returns *before* the one-shot latch: a run that cannot + // exchange must not spend the single-use authorization code. const h = harness({ servers: [entry("a")] }); await waitFor(() => expect(window.location.pathname).toBe("/oauth/callback"), ); - // Publishing it on a later render is what lets the exchange run at all. - h.rerender({ servers: [entry("a")], setupClient: () => client }); + expect(client.resumeAfterOAuth).not.toHaveBeenCalled(); + + // The next `servers` change — the same dependency the hydration wait + // rides in production — still finds the latch open. + h.rerender({ + servers: [entry("a"), entry("b")], + setupClient: () => client, + }); await waitFor(() => expect(client.resumeAfterOAuth).toHaveBeenCalled()); }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 96094176..7468799e 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -101,6 +101,15 @@ export interface PrepareOAuthRedirectArgs { client?: InspectorClient; } +/** + * The only part of the Network log this hook touches: reading the entries to + * flush before a redirect. Naming that rather than the whole + * `FetchRequestLogState` states the real dependency, and lets a test supply a + * plain object instead of casting a partial one through `unknown` — the same + * shape `useProgressToasts` uses for its slice of `InspectorClient`. + */ +export type FetchRequestSource = Pick; + /** The one thing the `/oauth/callback` rebuild needs from the connect path. */ export type SetupClientForServer = ( server: ServerEntry, @@ -127,7 +136,7 @@ export interface UseOAuthRecoveryOptions { setUi: TabUiStateSetters; setActiveTab: (next: string) => void; /** The live Network log, flushed to session storage before a redirect. */ - fetchLogRef: RefObject; + fetchLogRef: RefObject; /** Handshake telemetry, cleared/started around the callback's reconnect. */ connectStartRef: RefObject; /**