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`, `clients/tui`, and `core/react/**` (#2192). The same rule set forbids writing a ref during render (`react-hooks/refs`): sync a "latest value" ref in an effect instead.
 - 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.** `eslint-plugin-react-hooks` used to be a `clients/web` devDependency only, and each client's `eslint .` is scoped to its own directory — 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, 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).

core/react/useClientSettingsDraft.ts

@@ -33,25 +33,33 @@ export function useClientSettingsDraft<T>({
   const timerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);
   const latestValuesRef = useRef<T | null>(null);
 
-  const resolveInitialRef = useRef(resolveInitial);
+  // Seeded during render rather than in an effect, for the reason spelled
+  // out on {@link useSettingsDraft}: an effect would paint the previous
+  // session's draft for one frame before correcting itself. `seeded` is
+  // tracked separately from `draft` so a `resolveInitial` that legitimately
+  // returns `null` is not re-seeded on every render.
+  const [seeded, setSeeded] = useState(false);
+  if (seeded !== opened) {
+    setSeeded(opened);
+    setDraft(opened ? resolveInitial() : null);
+  }
+
   const onPersistRef = useRef(onPersist);
   const onErrorRef = useRef(onError);
   const openedRef = useRef(opened);
-  resolveInitialRef.current = resolveInitial;
-  onPersistRef.current = onPersist;
-  onErrorRef.current = onError;
-  openedRef.current = opened;
-
+  // Written in an effect rather than during render (`react-hooks/refs`).
+  // Every reader — the debounce timer, `flush`, and `onChange` — runs after
+  // a commit, so a post-commit write is what they need. `latestValuesRef`
+  // mirrors `draft` here so it is re-seeded (and cleared on close) without a
+  // second copy of the seeding rule; `onChange` still writes it
+  // synchronously, because a functional update in the same tick has to see
+  // the value the previous call produced.
   useEffect(() => {
-    if (!opened) {
-      setDraft(null);
-      latestValuesRef.current = null;
-      return;
-    }
-    const initial = resolveInitialRef.current();
-    setDraft(initial);
-    latestValuesRef.current = initial;
-  }, [opened]);
+    onPersistRef.current = onPersist;
+    onErrorRef.current = onError;
+    openedRef.current = opened;
+    latestValuesRef.current = draft;
+  });
 
   useEffect(() => {
     return () => {

core/react/useEmaIdpLoginState.ts

@@ -35,6 +35,16 @@ export function useEmaIdpLoginState(
 
   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
     void refresh();
   }, [active, refresh]);
 

core/react/useInitialConfig.ts

@@ -245,11 +245,25 @@ export function useInitialConfig(
   }, [base, authToken, doFetch]);
 
   useEffect(() => {
+    // `load` commits nothing before its first `await`: it claims a generation
+    // token, builds headers, and only then fetches, so every setState it makes
+    // runs in a continuation. `set-state-in-effect` follows the call into an
+    // async function without modelling the `await` boundary, so it reports
+    // this even though there is no synchronous commit to cascade from.
+    // eslint-disable-next-line react-hooks/set-state-in-effect -- no commit before the first await
     void load();
     return () => {
       // Invalidate whatever is in flight: this fires on unmount and on a
       // `baseUrl`/`authToken` change, and in both cases a response for the
       // old inputs must not commit.
+      //
+      // `generation` is a monotonic request token, not a ref to a rendered
+      // node. `exhaustive-deps`' advice — copy `generation.current` into a
+      // variable inside the effect and use the copy here — is exactly wrong:
+      // the cleanup must bump whatever the counter holds *at teardown*, since
+      // that is what invalidates the load currently in flight. A value
+      // captured at effect setup would let a superseded response commit.
+      // eslint-disable-next-line react-hooks/exhaustive-deps -- must read the token at teardown
       generation.current++;
     };
   }, [load]);

core/react/useServers.ts

@@ -231,6 +231,14 @@ export function useServers(opts: UseServersOptions): UseServersResult {
   );
 
   useEffect(() => {
+    // Loading the server list from the backend is the external-system
+    // synchronization this effect is for, and every list/error commit happens
+    // after `loadServers()` resolves. `set-state-in-effect` follows the call
+    // without modelling the `await`, so it reports the whole shape. The
+    // synchronous part is the pair that *opens* a load — `setLoading(true)`
+    // and clearing a stale error — which settles in one extra render, and is
+    // the point of having a loading flag at all.
+    // eslint-disable-next-line react-hooks/set-state-in-effect -- async load of an external list
     void refresh();
   }, [refresh]);
 

core/react/useSettingsDraft.ts

@@ -28,11 +28,12 @@ export interface UseSettingsDraftOptions<T> {
   targetId: string | undefined;
   /**
    * Resolves the initial draft for a given server id at the moment the
-   * modal opens. Called once per target — must be stable enough that
-   * the caller doesn't accidentally provoke re-initialization while
-   * the user is editing. (We do not include this function in the
-   * effect deps; it's read through a ref so callers don't have to
-   * `useCallback` it.)
+   * modal opens. Called once per target, **during render** — so it must
+   * be pure: a render can be replayed under StrictMode or abandoned by
+   * concurrent React, which would run any side effect an unpredictable
+   * number of times. Its identity is never a dependency of anything, so
+   * callers do not have to `useCallback` it, and re-creating it on every
+   * render cannot re-seed an in-progress edit.
    */
   resolveInitial: (id: string) => T;
   /** Persist the draft. Called from the debounced flush and from `flush`. */
@@ -73,31 +74,41 @@ export function useSettingsDraft<T>({
   const [draft, setDraft] = useState<T | null>(null);
   const timerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);
 
+  // Seeding happens during render, not in an effect. An effect keyed on
+  // `targetId` renders once with the *previous* target's draft, paints
+  // it, and only then corrects itself — so opening the modal on a second
+  // server briefly shows the first one's values. React's documented
+  // "adjusting state when a prop changes" pattern discards the
+  // in-progress render instead, so nothing stale reaches the DOM.
+  // (`clients/web` spells the same thing `useValueChange`; `core/` cannot
+  // import from a client.)
+  //
+  // `seededFor` is compared rather than `draft`, because "already seeded
+  // for this target" and "the draft happens to be null" are different
+  // states: a caller whose `resolveInitial` legitimately yields `null`
+  // would otherwise be re-seeded on every render.
+  const [seededFor, setSeededFor] = useState<string | undefined>(undefined);
+  if (seededFor !== targetId) {
+    setSeededFor(targetId);
+    setDraft(targetId ? resolveInitial(targetId) : null);
+  }
+
   // Read callbacks (and the current draft) through refs so the consumer
   // doesn't have to `useCallback` them, and so the returned `flush`
-  // identity is stable across keystrokes. The effect that
-  // re-initializes the draft must depend on `targetId` only — anything
-  // else in its deps risks resetting an in-progress edit when the
-  // parent re-renders for unrelated reasons (e.g. a background
-  // server-list refresh).
-  const resolveInitialRef = useRef(resolveInitial);
+  // identity is stable across keystrokes. Written in an effect rather
+  // than during render (`react-hooks/refs`): a render can be replayed or
+  // abandoned, and every reader here — the debounce timer and `flush` —
+  // runs after a commit, so a post-commit write is what they need.
   const onPersistRef = useRef(onPersist);
   const onErrorRef = useRef(onError);
   const draftRef = useRef<T | null>(null);
   const targetIdRef = useRef(targetId);
-  resolveInitialRef.current = resolveInitial;
-  onPersistRef.current = onPersist;
-  onErrorRef.current = onError;
-  draftRef.current = draft;
-  targetIdRef.current = targetId;
-
   useEffect(() => {
-    if (!targetId) {
-      setDraft(null);
-      return;
-    }
-    setDraft(resolveInitialRef.current(targetId));
-  }, [targetId]);
+    onPersistRef.current = onPersist;
+    onErrorRef.current = onError;
+    draftRef.current = draft;
+    targetIdRef.current = targetId;
+  });
 
   // Unmount cleanup — a stale timer firing after unmount is harmless
   // today (the persist call still goes through to the backend) but

eslint.config.js

@@ -1,5 +1,6 @@
 import js from "@eslint/js";
 import globals from "globals";
+import reactHooks from "eslint-plugin-react-hooks";
 import tseslint from "typescript-eslint";
 import { defineConfig, globalIgnores } from "eslint/config";
 
@@ -10,22 +11,25 @@ import { defineConfig, globalIgnores } from "eslint/config";
 // isomorphic TypeScript (browser-side OAuth + Node backends + shared runtime);
 // the shared surface is Node-only tooling/tests.
 //
-// ⚠️ There is no React plugin here, and "neither surface has JSX" is NOT a
-// sufficient reason for that — `core/react/` is a directory of React hooks,
-// which the hook rules judge whether or not any JSX is present. So the whole
-// `eslint-plugin-react-hooks` set, `set-state-in-effect` included, has never
-// looked at them: it is a devDependency of `clients/web` only, and Node
-// resolution walks up rather than down, so the root cannot even load it.
-// That is why the prop-into-effect re-sync #1955 fixed sat here unreported
-// while the identical shape is an error over in `clients/web/src`. Closing the
-// gap means adding the plugin at the root and taking what it then reports —
-// measured during #1955 as 15 findings, every one of them in a hook that issue
-// did not convert (the two settings-draft hooks and the three async fetch
-// hooks) and none in the 16 it did. Tracked on its own rather than folded in.
+// `core/react/` gets the React hook rules, and "neither surface has JSX" was
+// never a reason to withhold them — the hook rules judge hooks, not JSX, and
+// that directory is nothing but hooks. Until #2192 the whole
+// `eslint-plugin-react-hooks` set, `set-state-in-effect` included, had never
+// looked at them: the plugin was a devDependency of `clients/web` only, and
+// Node resolution walks up rather than down, so the root could not load it.
+// That is how the prop-into-effect re-sync #1955 fixed sat here unreported
+// while the identical shape was an error over in `clients/web/src`. The plugin
+// is now a root devDependency and the block below applies it.
 //
-// Type-aware linting for the two blocks below. `no-floating-promises` needs
-// type information, which `tseslint.configs.recommended` (the non-type-aware
-// set) does not provide — so each block adds a parser project (#1959).
+// Scoped to `core/react/**` rather than all of `core/**`: everything else
+// there is plain isomorphic TypeScript, and the rules key off the `use`-prefix
+// naming convention, so a non-React `useFoo` helper elsewhere in `core/` would
+// be judged as a hook it is not.
+//
+// Type-aware linting for the `core/` and shared surfaces below.
+// `no-floating-promises` needs type information, which
+// `tseslint.configs.recommended` (the non-type-aware set) does not provide —
+// so a block of its own adds a parser project (#1959).
 //
 // `tsconfig.lint.json` exists for this and nothing else: the root has no
 // tsconfig of its own (`core/` is typechecked through `clients/web`'s
@@ -81,6 +85,17 @@ export default defineConfig([
     },
     rules: sharedRules,
   },
+  {
+    // #2192. `exhaustive-deps` is promoted from the recommended set's `warn`
+    // for the same reason `clients/web` promotes it (#2085): `--max-warnings 0`
+    // already fails the gate on it, and matching the severity keeps an editor
+    // squiggle honest rather than depending on a CLI flag.
+    files: ["core/react/**/*.{ts,tsx}"],
+    extends: [reactHooks.configs.flat.recommended],
+    rules: {
+      "react-hooks/exhaustive-deps": "error",
+    },
+  },
   {
     files: [
       "test-servers/src/**/*.{ts,tsx,mts,cts}",