From 2ba25d63e348e2c7d3c20faeb168ba9d881b9328 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:08:29 -0400 Subject: [PATCH] fix(web): reset pending OAuth slots during render, not in an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 71 ++++++++++++++++++- clients/web/src/hooks/useOAuthRecovery.ts | 30 +++++--- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 91b3f10d..e2ede1da 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/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; setFailedServerId: ReturnType; @@ -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(), + 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 }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 428352b7..d5a3d891 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/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 => {