fix(web): reset pending OAuth slots during render, not in an effect

`useOAuthRecovery` cleared its two server-scoped pending-OAuth slots — the
deferred re-auth and the step-up prompt — in a `useEffect` keyed on
`activeServerId`. An effect runs after the commit, so a server switch painted
one frame still carrying the previous server's re-auth banner or step-up
prompt: an authorization affordance attached to the wrong server.

Reset both during render with `useValueChange` instead. `activeServerId` is a
primitive, so it satisfies the helper's `Object.is` stability requirement, and
the previous value is read from the `setState` updater argument rather than
from `sessionRef.current`, keeping the render-phase callback pure.

`react-hooks/set-state-in-effect` did not fire on the original because the
effect read the previous value through a ref rather than from a prop or state.

The test records one entry per committed render and asserts both slots are
empty in the first frame after the switch — the pre-existing `waitFor`-based
tests pass against the effect version too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/hooks/useOAuthRecovery.test.tsx

@@ -9,7 +9,7 @@ import type {
 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 { useEffect, useLayoutEffect, useRef } from "react";
 import { renderWithMantine, act, waitFor } from "../test/renderWithMantine";
 import {
   OAUTH_RESUME_KEY,
@@ -169,9 +169,22 @@ interface HarnessProps {
   noFetchLog?: boolean;
 }
 
+/** One committed render's view of the two server-scoped pending slots. */
+interface PendingCommit {
+  stepUpServerId: string | undefined;
+  reauthServerId: string | undefined;
+}
+
 interface Harness {
   api: () => OAuthRecovery;
   rerender: (next: HarnessProps) => void;
+  /**
+   * Every committed render's pending slots, in order. Recorded from an effect
+   * with no dependency array, so one entry lands per commit — which is what
+   * lets a test assert that a reset was visible in the *first* frame after a
+   * server switch rather than merely eventually (#2223).
+   */
+  commits: PendingCommit[];
   spies: {
     setActiveServerId: ReturnType<typeof vi.fn>;
     setFailedServerId: ReturnType<typeof vi.fn>;
@@ -183,6 +196,7 @@ interface Harness {
 
 function harness(initial: HarnessProps = {}): Harness {
   let latest: OAuthRecovery | undefined;
+  const commits: PendingCommit[] = [];
   const spies = {
     setActiveServerId: vi.fn(),
     setFailedServerId: vi.fn(),
@@ -236,6 +250,15 @@ function harness(initial: HarnessProps = {}): Harness {
       clearResultPanels: spies.clearResultPanels,
       setSourceScopedError: spies.setSourceScopedError,
     });
+    // No dependency array: one entry per commit. `pendingReauth` is not part
+    // of the hook's public surface, so it is read back through the session ref
+    // the hook mirrors it into.
+    useEffect(() => {
+      commits.push({
+        stepUpServerId: latest?.pendingStepUp?.serverId,
+        reauthServerId: sessionRef.current.pendingReauth?.serverId,
+      });
+    });
     return null;
   }
 
@@ -246,6 +269,7 @@ function harness(initial: HarnessProps = {}): Harness {
       return latest;
     },
     rerender: (next) => rerender(<Probe p={next} />),
+    commits,
     spies,
   };
 }
@@ -322,6 +346,51 @@ describe("useOAuthRecovery", () => {
       expect(client.handleAuthChallenge).not.toHaveBeenCalled();
     });
 
+    it("clears both slots in the first committed frame after a server switch", async () => {
+      visibility.visible = false;
+      const client = fakeClient();
+      const servers = [entry("a"), entry("b")];
+      const h = harness({ servers, activeServerId: "a", client });
+
+      await act(async () => {
+        await h
+          .api()
+          .handleCommandScopedAuthRecovery(
+            new AuthRecoveryRequiredError(
+              AUTH_URL,
+              challenge("insufficient_scope"),
+            ),
+            { serverId: "a", source: "tool" },
+          );
+      });
+      await act(async () => {
+        client.emit("authChallengeInteractive", {
+          challenge: challenge(),
+          authorizationUrl: AUTH_URL,
+        });
+      });
+      await waitFor(() => {
+        const last = h.commits[h.commits.length - 1];
+        expect(last?.stepUpServerId).toBe("a");
+        expect(last?.reauthServerId).toBe("a");
+      });
+
+      const before = h.commits.length;
+      await act(async () => {
+        h.rerender({ servers, activeServerId: "b", client });
+      });
+
+      // The point of the test: the *first* frame after the switch already
+      // shows both slots empty. Resetting them in an effect instead would
+      // commit one frame still carrying server "a"'s step-up prompt and
+      // deferred re-auth, and would pass a `waitFor` assertion just the same.
+      expect(h.commits.length).toBeGreaterThan(before);
+      expect(h.commits[before]).toEqual({
+        stepUpServerId: undefined,
+        reauthServerId: undefined,
+      });
+    });
+
     it("refuses a second step-up while one is open", async () => {
       const client = fakeClient();
       const h = harness({ servers: [entry("a")], activeServerId: "a", client });

clients/web/src/hooks/useOAuthRecovery.ts

@@ -54,6 +54,7 @@ import {
 import { OAUTH_CALLBACK_PATH } from "../utils/oauthFlow";
 import { INSPECTOR_SERVERS_TAB } from "../utils/inspectorTabs";
 import type { PendingReauth } from "../utils/pendingReauth";
+import { useValueChange } from "./useValueChange";
 import {
   authRecoveryRestoredMessage,
   authRecoveryAbandonedMessage,
@@ -318,16 +319,25 @@ export function useOAuthRecovery({
     sessionRef.current.pendingReauth = pendingReauth;
   });
 
-  useEffect(() => {
-    const pending = sessionRef.current.pendingReauth;
-    if (pending && pending.serverId !== activeServerId) {
-      setPendingReauth(null);
-    }
-    const stepUp = sessionRef.current.pendingStepUp;
-    if (stepUp && stepUp.serverId !== activeServerId) {
-      setPendingStepUp(null);
-    }
-  }, [sessionRef, activeServerId]);
+  // Both slots are server-scoped, so switching servers has to clear whichever
+  // one belongs to the server we just left. Adjusted **during render** rather
+  // than in an effect: an effect only runs after the commit, so the frame that
+  // shows the new server would still paint the previous server's re-auth
+  // banner or step-up prompt — a modal-grade authorization affordance attached
+  // to the wrong server (#2223). `activeServerId` is a primitive, so it is
+  // referentially stable across renders that mean "no change", which is what
+  // `useValueChange`'s `Object.is` comparison requires. The previous value is
+  // read from the updater argument rather than from `sessionRef.current`,
+  // keeping the callback pure — a render can be replayed or abandoned, so it
+  // must not depend on external mutable state.
+  useValueChange(activeServerId, () => {
+    setPendingReauth((current) =>
+      current && current.serverId !== activeServerId ? null : current,
+    );
+    setPendingStepUp((current) =>
+      current && current.serverId !== activeServerId ? null : current,
+    );
+  });
 
   const trySetPendingStepUp = useCallback(
     (next: PendingStepUp): boolean => {