chore: lint core/react with eslint-plugin-react-hooks

.github/copilot-instructions.md

@@ -51,7 +51,7 @@ const CardContent = Group.withProps({
 
 ### State and effects
 
-- **Never reset or re-sync local state from a prop inside a `useEffect`.** `useEffect(() => setX(prop), [prop])` paints the stale value first and renders twice; it is an error under `react-hooks/set-state-in-effect`.
+- **Never reset or re-sync local state from a prop inside a `useEffect`.** `useEffect(() => setX(prop), [prop])` paints the stale value first and renders twice; it is an error under `react-hooks/set-state-in-effect` — enforced in `clients/web` and, since #2192, in `core/react/**`. The same rule set forbids writing a ref during render (`react-hooks/refs`): sync a "latest value" ref in an effect instead. `clients/tui` is the exception — it enables only `rules-of-hooks` and `exhaustive-deps`, so do not cite these two against TUI code.
 - Use **`useValueChange(value, onChange)`** (`src/hooks/useValueChange.ts`) — React's documented "adjusting state during render" pattern. It does not fire on the first render; seed the state with `useState`. The comparison is `Object.is`, so pass a **referentially stable** value — a primitive key (id/name/URI) or a memoized one, never a fresh object literal.
 - The `onChange` runs **during render**, so it must be pure — `setState` and nothing else. No fetches, DOM writes, logging, ref mutation, or parent callbacks; a render can be replayed or abandoned.
 - Effects remain correct for real external-system synchronization (DOM measurement, rAF, subscriptions, timers).

README.md

@@ -618,6 +618,8 @@ Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …),
 
 **Linting is type-aware.** All five ESLint scopes (`clients/{web,cli,tui,launcher}` plus the root `core/` + shared gate) enable `@typescript-eslint/no-floating-promises` at `error`, so a promise that is neither awaited, returned, `.catch(…)`-terminated, nor explicitly discarded with `void` fails `lint` — and therefore `validate` ([#1959](https://github.com/modelcontextprotocol/inspector/issues/1959)). The rule needs type information, so each scope's config names a parser project; the root scope's is **`tsconfig.lint.json`**, a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, which have no tsconfig of their own. It emits nothing and changes no typecheck — but a new first-party TS location added to the root lint scope must be added to its `include`. See **TypeScript instructions** in [`AGENTS.md`](./AGENTS.md) for when `void` is acceptable.
 
+**The React hook rules cover `core/react/` too.** Every copy of `eslint-plugin-react-hooks` used to be client-local — a devDependency of `clients/web` and of `clients/tui`, each reachable only from that client's own `eslint .` — so the whole hook rule set, `set-state-in-effect` included, had never looked at `core/react/`, a directory of nothing but React hooks ([#2192](https://github.com/modelcontextprotocol/inspector/issues/2192)). "Neither surface has JSX" was the stated reason and it was never a good one: the rules judge hooks, not JSX. The plugin is now a root devDependency as well, applied by a `core/react/**` block in the root `eslint.config.js` — scoped there rather than to all of `core/**`, since the rules key off the `use`-prefix convention and a non-React `useFoo` helper elsewhere would be judged as a hook it is not.
+
 **And lint has no warning tier.** Every `lint` script runs with `--max-warnings 0`, so a warning fails `validate` exactly as an error does ([#2085](https://github.com/modelcontextprotocol/inspector/issues/2085)) — a `warn`-level `react-hooks/exhaustive-deps` finding had otherwise let a stale-closure bug pass the mandatory pre-push gate and reach review. Fix the finding rather than silencing it; if a rule genuinely must be waived, use its inline disable comment with a one-line justification.
 
 For the full testing rules — the ≥90% per-file gate, where test files live, the unit vs. integration vs. storybook projects, and the `v8 ignore` policy — see [`AGENTS.md`](./AGENTS.md).

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

@@ -51,17 +51,31 @@ interface Harness {
  * Hands the hook's API back to the test. The hook holds refs, so it has to be
  * read from inside a rendered component; `setServers` re-renders it with a new
  * list the way `useServers` does after a read.
+ *
+ * The returned object *delegates* rather than being the API from the first
+ * render. `resolve` is keyed on the current `servers` (#2192) instead of on a
+ * ref, so its identity changes with the list — handing back a snapshot would
+ * give every test a `resolve` closed over the initial list, and `setServers`
+ * would appear to do nothing.
  */
 function harness(initial: ServerEntry[]): Harness {
-  let api: LastPersistedSettings | undefined;
+  let latest: LastPersistedSettings | undefined;
   function Probe({ servers }: { servers: ServerEntry[] }) {
-    api = useLastPersistedSettings(servers);
+    latest = useLastPersistedSettings(servers);
     return null;
   }
   const { rerender } = renderWithMantine(<Probe servers={initial} />);
-  if (!api) throw new Error("hook did not render");
+  if (!latest) throw new Error("hook did not render");
+  const current = () => {
+    if (!latest) throw new Error("hook did not render");
+    return latest;
+  };
   return {
-    api,
+    api: {
+      begin: (serverId) => current().begin(serverId),
+      resolve: (serverId) => current().resolve(serverId),
+      lastWriteFailed: (serverId) => current().lastWriteFailed(serverId),
+    },
     setServers: (next) => rerender(<Probe servers={next} />),
   };
 }
@@ -222,4 +236,28 @@ describe("useLastPersistedSettings", () => {
     setServers([]);
     expect(api.resolve("A")).toBeUndefined();
   });
+  it("answers from the list of the render it is called in", () => {
+    // `useSettingsDraft` seeds its draft during render (#2192), so `resolve`
+    // is called there. Reading the effect-mirrored `serversRef` would answer
+    // from the *previous* commit's list on exactly the render where a
+    // refreshed list arrives — which is the render that seeds the draft.
+    const before = entry("A", settings({ paginatedLists: false }));
+    const after = entry("A", settings({ paginatedLists: true }));
+    const answers: Array<InspectorServerSettings | undefined> = [];
+
+    function Probe({ servers }: { servers: ServerEntry[] }) {
+      const api = useLastPersistedSettings(servers);
+      answers.push(api.resolve("A"));
+      return null;
+    }
+
+    const { rerender } = renderWithMantine(<Probe servers={[before]} />);
+    expect(answers.at(-1)).toBe(before.settings);
+
+    answers.length = 0;
+    rerender(<Probe servers={[after]} />);
+
+    expect(answers.length).toBeGreaterThan(0);
+    for (const answer of answers) expect(answer).toBe(after.settings);
+  });
 });

clients/web/src/hooks/useLastPersistedSettings.ts

@@ -145,16 +145,33 @@ export function useLastPersistedSettings(
   const outcomesRef = useRef<Map<string, { sequence: number; ok: boolean }>>(
     new Map(),
   );
-  // Read the list through a ref so a write pairs with the entry as it stands
-  // when it finishes, not the one captured when it was issued. Mirrored in an
-  // effect rather than assigned during render: writing a ref mid-render is an
-  // error under `react-hooks/refs`, and every reader runs from a settled
-  // promise or an event handler, long after the commit.
+  // `landed` reads the list through a ref so a write pairs with the entry as
+  // it stands when it *finishes*, not the one captured when it was issued.
+  // Mirrored in an effect rather than assigned during render: writing a ref
+  // mid-render is an error under `react-hooks/refs`, and this reader runs from
+  // a settled promise, long after the commit. `resolve` deliberately does not
+  // use it — see there.
   const serversRef = useRef(servers);
   useEffect(() => {
     serversRef.current = servers;
   }, [servers]);
 
+  // Drop records the list has moved past. This used to happen lazily inside
+  // `resolve`, which is no longer allowed: `useSettingsDraft` seeds its draft
+  // during render (#2192) and so calls `resolve` there, where a ref mutation
+  // would run an unpredictable number of times — React may replay a render
+  // under StrictMode or abandon it altogether. Nothing depends on the eviction
+  // happening at any particular moment; it only keeps a dead record from being
+  // re-checked on every later write for that server, so an effect is the right
+  // home for it.
+  useEffect(() => {
+    for (const [serverId, record] of recordsRef.current) {
+      if (record.entry !== servers.find((s) => s.id === serverId)) {
+        recordsRef.current.delete(serverId);
+      }
+    }
+  }, [servers]);
+
   const begin = useCallback((serverId: string): SettingsWrite => {
     const sequence = ++nextSequenceRef.current;
     const pending = pendingRef.current.get(serverId) ?? new Set<number>();
@@ -194,22 +211,25 @@ export function useLastPersistedSettings(
     [],
   );
 
+  // Pure, and over `servers` rather than `serversRef`. Both properties are
+  // required because `useSettingsDraft` calls this during render to seed its
+  // draft (#2192): a render must not mutate shared state, and the ref trails
+  // the value it mirrors by one passive-effect flush, so reading it here would
+  // answer from the previous commit's list on exactly the render where a
+  // refreshed list arrives. Reading `servers` also makes the supersession test
+  // fire a render earlier, which is the direction that matters — a list that
+  // has genuinely been re-read is what the record is being checked against.
   const resolve = useCallback(
     (serverId: string): InspectorServerSettings | undefined => {
-      const entry = serversRef.current.find((s) => s.id === serverId);
+      const entry = servers.find((s) => s.id === serverId);
       const current = recordsRef.current.get(serverId);
       if (!current) return entry?.settings;
       // Identity, not deep equality: the question is whether the list has been
       // re-read since the write, not whether the values happen to match.
-      if (current.entry !== entry) {
-        // Superseded — drop it rather than re-checking a dead record on every
-        // later write for this server.
-        recordsRef.current.delete(serverId);
-        return entry?.settings;
-      }
+      if (current.entry !== entry) return entry?.settings;
       return current.written;
     },
-    [],
+    [servers],
   );
 
   return { begin, resolve, lastWriteFailed };

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,4 +189,155 @@ describe("useEmaIdpLoginState", () => {
     ).not.toHaveBeenCalled();
     expect(result.current.loginState).toBe("none");
   });
+  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, "-")
+      .replace(/\//g, "_")
+      .replace(/=+$/, "");
+    vi.mocked(storage.getIdpSession).mockResolvedValue({
+      idToken: `h.${payload}.s`,
+    });
+
+    const { result, rerender } = renderHook(
+      ({ issuer }: { issuer: string }) =>
+        useEmaIdpLoginState(storage, issuer, true),
+      { initialProps: { issuer: "https://idp.test" } },
+    );
+
+    await waitFor(() => {
+      expect(result.current.loginState).toBe("logged_in");
+    });
+
+    // 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 {
+      vi.mocked(storage.getIdpSession).mockRejectedValue(
+        new Error("storage unreachable"),
+      );
+      await act(async () => {
+        rerender({ issuer: "https://other-idp.test" });
+        await new Promise((resolve) => setTimeout(resolve, 0));
+      });
+    } finally {
+      process.off("unhandledRejection", unhandled);
+    }
+
+    expect(storage.getIdpSession).toHaveBeenCalledWith(
+      "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");
+  });
 });