chore: address Copilot review round 2 (#2192)

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

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<SettingsShape>({
+        opened,
+        resolveInitial: () => nextInitial,
+        onPersist: vi.fn(),
+        onError: vi.fn(),
+      });
+      useEffect(() => {
+        commits.push({ opened, text: draft?.text ?? null });
+      });
+      return null;
+    }
+
+    const { rerender } = render(<Harness opened={true} />);
+    expect(commits).toContainEqual({ opened: true, text: "first-session" });
+
+    rerender(<Harness opened={false} />);
+
+    // 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(<Harness opened={true} />);
+
+    expect(commits.length).toBeGreaterThan(0);
+    for (const commit of commits) {
+      expect(commit).toEqual({ opened: true, text: "second-session" });
+    }
+  });
 });

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");
+  });
 });

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<string, SettingsShape> = {
+      alpha: { text: "alpha-value", rows: [] },
+      beta: { text: "beta-value", rows: [] },
+    };
+
+    function Harness({ targetId }: { targetId: string }) {
+      const { draft } = useSettingsDraft<SettingsShape>({
+        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(<Harness targetId="alpha" />);
+    expect(commits).toContainEqual({ targetId: "alpha", text: "alpha-value" });
+
+    commits.length = 0;
+    rerender(<Harness targetId="beta" />);
+
+    expect(commits.length).toBeGreaterThan(0);
+    for (const commit of commits) {
+      expect(commit).toEqual({ targetId: "beta", text: "beta-value" });
+    }
+  });
 });