From 546dacffe21f2fdaea2fccf03bc7cdd603ecb3ee Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 30 Aug 2026 00:22:28 -0400 Subject: [PATCH] chore: address Copilot review round 2 (#2192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `useEmaIdpLoginState` attributed a read to the wrong issuer. A bare `EmaIdpLoginState` carries the previous issuer's answer over to a new one until a fresh read lands — and if that read fails, indefinitely, so the settings surface reports "logged in" for an IdP nobody authenticated against. Round 1's `.catch()` made that worse by design, and the test added with it asserted the wrong behaviour outright. State is now the pair (issuer, value) and `loginState` derives from it, so a mismatch reports "none". A monotonic read token — the device `useInitialConfig` already uses — drops completions that were overtaken, including a read still in flight when `logout` clears the session; issuer keying cannot catch those, since both carry the same issuer. Both guards are pinned by tests verified to fail without them. Deriving the value also removed the last synchronous commit on that path, so the `set-state-in-effect` suppression is gone — one fewer than the PR started with. - The draft-seeding change had no test that could tell it from the effect it replaced: the regression is a committed frame, and RTL flushes passive effects before any assertion on `result.current`. Both hooks now have a commit-level trace recorded from inside the tree, asserting the previous target's (or session's) draft is never committed for the new one. Both were verified to fail against the effect implementation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../react/useClientSettingsDraft.test.tsx | 42 ++++++- .../core/react/useEmaIdpLoginState.test.tsx | 116 +++++++++++++++++- .../test/core/react/useSettingsDraft.test.tsx | 45 ++++++- core/react/useEmaIdpLoginState.ts | 76 ++++++++---- 4 files changed, 252 insertions(+), 27 deletions(-) diff --git a/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx b/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx index 23592a35..884b5019 100644 --- a/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx +++ b/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx @@ -6,7 +6,8 @@ * persist, synchronous flush-on-close, and reset-on-close behaviors. */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { act, renderHook } from "@testing-library/react"; +import { act, render, renderHook } from "@testing-library/react"; +import { useEffect } from "react"; import { useClientSettingsDraft } from "@inspector/core/react/useClientSettingsDraft"; interface SettingsShape { @@ -422,4 +423,43 @@ describe("useClientSettingsDraft", () => { }); expect(result.current.flush).toBe(flushAfterMount); }); + it("never commits the previous session's draft after reopening", () => { + // Same reasoning as the `useSettingsDraft` counterpart: the regression is + // a committed frame, not an end state. `rerender()` flushes the old + // seeding effect before any assertion on `result.current`, so an + // effect-seeded implementation passes those too. Record every commit from + // inside the tree instead, and assert the previous session's draft is + // never committed while the new one is open. + const commits: Array<{ opened: boolean; text: string | null }> = []; + let nextInitial: SettingsShape = { text: "first-session", rows: [] }; + + function Harness({ opened }: { opened: boolean }) { + const { draft } = useClientSettingsDraft({ + opened, + resolveInitial: () => nextInitial, + onPersist: vi.fn(), + onError: vi.fn(), + }); + useEffect(() => { + commits.push({ opened, text: draft?.text ?? null }); + }); + return null; + } + + const { rerender } = render(); + expect(commits).toContainEqual({ opened: true, text: "first-session" }); + + rerender(); + + // Reopen against different saved config — the modal must never paint the + // values from the session that just closed. + nextInitial = { text: "second-session", rows: [] }; + commits.length = 0; + rerender(); + + expect(commits.length).toBeGreaterThan(0); + for (const commit of commits) { + expect(commit).toEqual({ opened: true, text: "second-session" }); + } + }); }); diff --git a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx index 65c1780a..d346677d 100644 --- a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx +++ b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx @@ -189,7 +189,7 @@ describe("useEmaIdpLoginState", () => { ).not.toHaveBeenCalled(); expect(result.current.loginState).toBe("none"); }); - it("swallows a session-read rejection and leaves loginState unchanged", async () => { + it("a failed read for a new issuer reports none, not the old issuer's state", async () => { const exp = Math.floor(Date.now() / 1000) + 3600; const payload = btoa(JSON.stringify({ exp })) .replace(/\+/g, "-") @@ -209,9 +209,10 @@ describe("useEmaIdpLoginState", () => { expect(result.current.loginState).toBe("logged_in"); }); - // The storage backend goes away. The effect-triggered refresh must not - // surface an unhandled rejection, and must leave the last state we - // actually read in place rather than falsely reporting signed-out. + // The storage backend goes away *and* the issuer changes. The rejection + // must not surface as an unhandled promise, and the new issuer must not + // inherit the previous one's "logged_in" — nobody has authenticated + // against it, and a read that never succeeded cannot say otherwise. const unhandled = vi.fn(); process.on("unhandledRejection", unhandled); try { @@ -230,6 +231,113 @@ describe("useEmaIdpLoginState", () => { "https://other-idp.test", ); expect(unhandled).not.toHaveBeenCalled(); + expect(result.current.loginState).toBe("none"); + }); + + it("keeps the last answer when a re-read of the same issuer fails", async () => { + const exp = Math.floor(Date.now() / 1000) + 3600; + const payload = btoa(JSON.stringify({ exp })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + vi.mocked(storage.getIdpSession).mockResolvedValue({ + idToken: `h.${payload}.s`, + }); + + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + await waitFor(() => { + expect(result.current.loginState).toBe("logged_in"); + }); + + // A transient storage outage is not evidence that the session ended, so + // the issuer we are still looking at keeps the answer we actually got. + vi.mocked(storage.getIdpSession).mockRejectedValue( + new Error("storage unreachable"), + ); + await act(async () => { + await result.current.refresh().catch(() => {}); + }); + expect(result.current.loginState).toBe("logged_in"); }); + + it("ignores an overtaken read that resolves last for the same issuer", async () => { + const jwt = (secondsFromNow: number) => { + const payload = btoa( + JSON.stringify({ exp: Math.floor(Date.now() / 1000) + secondsFromNow }), + ) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + return `h.${payload}.s`; + }; + + // Two reads for the *same* issuer overlap, and the older one resolves + // last. Issuer keying cannot tell them apart — both carry this issuer — + // so only the read token stops the superseded answer from committing. + let releaseFirst: () => void = () => {}; + const first = new Promise<{ idToken: string }>((resolve) => { + releaseFirst = () => resolve({ idToken: jwt(3600) }); + }); + vi.mocked(storage.getIdpSession) + .mockReturnValueOnce(first) + .mockResolvedValue({ idToken: jwt(-3600) }); + + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + // Second read starts and finishes while the first is still pending. + await act(async () => { + await result.current.refresh(); + }); + expect(result.current.loginState).toBe("expired"); + + await act(async () => { + releaseFirst(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(result.current.loginState).toBe("expired"); + }); + + it("a logout supersedes a read that was already in flight", async () => { + const exp = Math.floor(Date.now() / 1000) + 3600; + const payload = btoa(JSON.stringify({ exp })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + + let releaseRead: () => void = () => {}; + const pending = new Promise<{ idToken: string }>((resolve) => { + releaseRead = () => resolve({ idToken: `h.${payload}.s` }); + }); + vi.mocked(storage.getIdpSession).mockReturnValue(pending); + + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + act(() => { + result.current.logout(); + }); + // Let the clear settle on its own before releasing the read, so the read + // is unambiguously the *last* writer — without the token it would land on + // top of the clear and report the cleared session as logged in. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + expect(result.current.loginState).toBe("none"); + + await act(async () => { + releaseRead(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(storage.clearIdpSession).toHaveBeenCalledWith("https://idp.test"); + expect(result.current.loginState).toBe("none"); + }); }); diff --git a/clients/web/src/test/core/react/useSettingsDraft.test.tsx b/clients/web/src/test/core/react/useSettingsDraft.test.tsx index aa34f202..451a166e 100644 --- a/clients/web/src/test/core/react/useSettingsDraft.test.tsx +++ b/clients/web/src/test/core/react/useSettingsDraft.test.tsx @@ -6,7 +6,8 @@ * appear to eat keystrokes. The PUT itself debounces. */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { act, renderHook } from "@testing-library/react"; +import { act, render, renderHook } from "@testing-library/react"; +import { useEffect } from "react"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft"; interface SettingsShape { @@ -375,4 +376,46 @@ describe("useSettingsDraft", () => { }); expect(result.current.flush).toBe(flushAfterMount); }); + + it("never commits the previous target's draft for a new target", () => { + // The regression this pins is a *frame*, not an end state, so it cannot + // be observed through `result.current`: RTL's `act` flushes passive + // effects before the assertion runs, so an effect-seeded implementation + // reaches the same final value and passes any check made after the + // rerender settles. What distinguishes them is what was *committed* on + // the way there — an effect renders once with the previous target's draft + // and paints it before correcting itself. + // + // So record every commit from inside the tree and assert no committed + // frame ever paired "beta" with alpha's draft. + const commits: Array<{ targetId: string; text: string | null }> = []; + const SETTINGS: Record = { + alpha: { text: "alpha-value", rows: [] }, + beta: { text: "beta-value", rows: [] }, + }; + + function Harness({ targetId }: { targetId: string }) { + const { draft } = useSettingsDraft({ + targetId, + resolveInitial: (id) => SETTINGS[id] ?? EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }); + useEffect(() => { + commits.push({ targetId, text: draft?.text ?? null }); + }); + return null; + } + + const { rerender } = render(); + expect(commits).toContainEqual({ targetId: "alpha", text: "alpha-value" }); + + commits.length = 0; + rerender(); + + expect(commits.length).toBeGreaterThan(0); + for (const commit of commits) { + expect(commit).toEqual({ targetId: "beta", text: "beta-value" }); + } + }); }); diff --git a/core/react/useEmaIdpLoginState.ts b/core/react/useEmaIdpLoginState.ts index d03d9853..5af72c1c 100644 --- a/core/react/useEmaIdpLoginState.ts +++ b/core/react/useEmaIdpLoginState.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import type { OAuthStorage } from "../auth/storage.js"; import { clearEmaIdpSession, @@ -13,6 +13,21 @@ export interface UseEmaIdpLoginStateResult { logout: () => void; } +/** + * What a completed read told us, and *which issuer* it was told about. + * + * The pair is stored together rather than as a bare `EmaIdpLoginState` + * because a login state is only ever meaningful for the issuer it was read + * for. With a bare value, changing issuer carries the previous issuer's + * answer over to the new one until a fresh read lands — and if that read + * fails, indefinitely, so the settings surface reports "logged in" for an + * IdP nobody has authenticated against. + */ +interface ReadResult { + issuer: string; + value: EmaIdpLoginState; +} + /** * Read and clear EMA IdP session state for Client Settings UX. * Pass `active: true` while the settings surface is open to refresh on open. @@ -23,48 +38,67 @@ export function useEmaIdpLoginState( active: boolean, ): UseEmaIdpLoginStateResult { const normalizedIssuer = issuer ? normalizeIdpIssuer(issuer) : ""; - const [loginState, setLoginState] = useState("none"); + const [read, setRead] = useState({ issuer: "", value: "none" }); + + /** + * Monotonic read token — the same device `useInitialConfig` uses. Reads are + * unordered, so an issuer flipped A → B → A can have the first A read + * resolve last; a completion may commit only while it still holds the + * current token. `logout` claims one too, so a read already in flight when + * the session is cleared cannot land on top of the clear. + */ + const generation = useRef(0); + + // Derived rather than stored: with no issuer there is nothing to be logged + // in to, and a result read for some *other* issuer says nothing about this + // one. A mismatch therefore reports "none" instead of the previous issuer's + // answer, which is also what makes the failure path below safe. + const loginState: EmaIdpLoginState = + normalizedIssuer && read.issuer === normalizedIssuer ? read.value : "none"; const refresh = useCallback(async () => { - if (!normalizedIssuer) { - setLoginState("none"); - return; - } - setLoginState(await getEmaIdpLoginState(storage, normalizedIssuer)); + const mine = ++generation.current; + if (!normalizedIssuer) return; + const value = await getEmaIdpLoginState(storage, normalizedIssuer); + if (generation.current !== mine) return; + setRead({ issuer: normalizedIssuer, value }); }, [storage, normalizedIssuer]); useEffect(() => { if (!active) return; // Reading the persisted IdP session *is* the external system this effect - // exists to synchronize with, and the read is asynchronous: the - // `loginState` commit happens in a continuation after - // `getEmaIdpLoginState` resolves. `set-state-in-effect` follows the call - // into `refresh` without modelling the `await`, so it reports every - // "fetch on open" effect alike — verified against a minimal hook whose - // only setState is after an `await`. The one commit here that *is* - // synchronous is the no-issuer reset to "none", which settles in a single - // extra render rather than cascading. - // eslint-disable-next-line react-hooks/set-state-in-effect -- async read of an external store + // exists to synchronize with, and no commit happens here: `refresh` + // returns before its first `await` when there is no issuer, and otherwise + // commits in a continuation. That is also why this needs no + // `set-state-in-effect` suppression, where `useServers` and + // `useInitialConfig` do — deriving `loginState` from an issuer-keyed read + // took the last synchronous setState out of the path. refresh().catch(() => { // `refresh` does not own its failures: `getEmaIdpLoginState` awaits // `storage.getIdpSession`, which rejects when the backend is // unreachable. Swallow it here so it does not surface as an unhandled - // rejection, and leave `loginState` untouched — the same choice `logout` - // makes, so the UI keeps reflecting the last state we actually read - // rather than falsely reporting signed-out. + // rejection, and commit nothing — the issuer keying above is what makes + // that safe. A failed read for a *new* issuer reports "none", because + // no result for that issuer exists; a failed re-read of the issuer + // already on screen keeps the last answer we actually got, which is the + // right reading of a transient storage outage. }); }, [active, refresh]); const logout = useCallback(() => { if (!normalizedIssuer) return; + // Supersede any read in flight: it was started before the session was + // cleared, so its answer is already wrong by the time it resolves. + const mine = ++generation.current; void clearEmaIdpSession(storage, normalizedIssuer) .then(() => { - setLoginState("none"); + if (generation.current !== mine) return; + setRead({ issuer: normalizedIssuer, value: "none" }); }) .catch(() => { // Clearing the persisted IdP session failed (e.g. the storage backend // is unreachable). Swallow the rejection so it does not surface as an - // unhandled promise, and leave loginState unchanged so the UI keeps + // unhandled promise, and leave the last read in place so the UI keeps // reflecting the still-present session rather than falsely showing // signed-out. });