chore(web): address Copilot review on #2153

- 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>

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
+   * hook we are about to call, so it cannot be declared before it.
-   * `/oauth/callback` effect only ever reads this from an effect, well after
-   * the render that writes it.
    */
   const setupClientForServerRef = useRef<SetupClientForServer | null>(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
+  // declared this far down.
-  // `sandboxUrlRef` above uses — because the only reader is an effect, which
+  //
-  // React runs strictly after the render that writes this.
+  // A *layout* effect, not a render-phase write and not a passive one. React
-  // 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)
+  // runs every layout effect before any passive effect of the same commit, and
-  setupClientForServerRef.current = setupClientForServer;
+  // 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) => {

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 = {
+    const fetchLogRef = useRef<FetchRequestSource | null>(null);
-      current: p.noFetchLog
+    // 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<SetupClientForServer | null>(null);
+    useLayoutEffect(() => {
+      fetchLogRef.current = p.noFetchLog
         ? null
-        : { getFetchRequests: () => p.fetchRequests ?? [] },
+        : { getFetchRequests: () => p.fetchRequests ?? [] };
-    } as unknown as Parameters<typeof useOAuthRecovery>[0]["fetchLogRef"];
+      setupClientForServerRef.current = p.setupClient
-    const setupRef = {
-      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.
+      expect(client.resumeAfterOAuth).not.toHaveBeenCalled();
-      h.rerender({ servers: [entry("a")], setupClient: () => client });
+
+      // 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());
     });
 

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<FetchRequestLogState, "getFetchRequests">;
+
 /** 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<FetchRequestLogState | null>;
+  fetchLogRef: RefObject<FetchRequestSource | null>;
   /** Handshake telemetry, cleared/started around the callback's reconnect. */
   connectStartRef: RefObject<number | undefined>;
   /**