fix(core): read state stores during render instead of re-syncing in an effect

.github/copilot-instructions.md

@@ -55,6 +55,10 @@ const CardContent = Group.withProps({
 - 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).
+- **Subscribing to an `@inspector/core` state store is `useSyncExternalStore`, never `useState` + a subscribing `useEffect`** — that shape re-seeds state from the store prop, so it carries the same stale frame plus a window where an event dispatched between render and subscribe is lost. `useValueChange` is web-only and `core/react/` is shared with the CLI and TUI.
+- Use **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`), once per value, when the getter returns a **fresh value per read** (a defensive copy, or a freshly built object) — nearly all of them. When the snapshot is **already referentially stable**, subscribe directly and skip the helper, as `useListError` does with its `Error | null`. `read`/`whenAbsent` must be **referentially stable** — they are part of the cache key. `read` is a function, so module scope; `whenAbsent` needs a module-scope constant only when it is an object or array, since a primitive fallback (`false`, `undefined`, a string) is stable already and is passed inline in these hooks. An unstable one fails **quietly** — no throw, no React warning, no extra render, just a fresh value each render that defeats downstream memoization.
+- The snapshot is cached against the store's per-event dispatch counter (`TypedEventTarget.getEventRevision`), **not** its contents — a contents compare cannot see a dispatch that mutated an entry already in the list. Don't replace it with a shallow compare.
+- The store is the source of truth; the event is only the signal. A test firing a store event must set the value on the store first, and a fake store must extend the real `TypedEventTarget` (a bare `EventTarget` lacks `getEventRevision`).
 
 ### Theme files vs. element components
 

AGENTS.md

@@ -392,7 +392,17 @@ v2/main/
 │   │                                   #   predicate the guard is built on, isLoopbackHost gates the OAuth
 │   │                                   #   callback listener; also stripBrackets. Used across
 │   │                                   #   clients/web/server, clients/cli, and core/auth/node — #1795)
-│   ├── react/                          # React hooks over the state stores
+│   ├── react/                          # React hooks over the state stores. Every store
+│   │                                   #   subscription reads its snapshot DURING RENDER via
+│   │                                   #   useSyncExternalStore, so a store swap lands in the
+│   │                                   #   same frame and an event dispatched between render
+│   │                                   #   and subscribe is not lost (#1955). Getters that
+│   │                                   #   return a fresh value per read — a defensive copy,
+│   │                                   #   or a freshly built object — go through
+│   │                                   #   useStoreSnapshot.ts, which caches the snapshot
+│   │                                   #   against TypedEventTarget's per-event dispatch
+│   │                                   #   counter; useListError subscribes directly, its
+│   │                                   #   Error|null snapshot being stable already
 │   └── storage/                        # File I/O helpers (store-io.ts) used by OAuth persist backends
 ├── test-servers/                       # Composable MCP test servers + fixtures used by integration tests.
 │   ├── src/                            # TypeScript sources. (modern-tasks.ts: SEP-2663 modern
@@ -1205,6 +1215,15 @@ Nothing **enforces** the boundary: no path alias keys off it, and the coverage `
   - Use **`useValueChange(value, onChange)`** (`clients/web/src/hooks/useValueChange.ts`) instead. It is React's documented ["adjusting state during render"](https://react.dev/reference/react/useState#storing-information-from-previous-renders) pattern: it compares `value` against the previous render's with `Object.is` and calls `onChange(next)` during render, so React discards the in-progress output and re-runs the component before anything reaches the DOM. It does **not** fire on the first render — seed the dependent state with `useState` instead. Because the comparison is `Object.is`, the value you pass **must be referentially stable** across renders that mean "no change": prefer a primitive key derived from the data (an id, a name, a URI), and otherwise a memoized value. A fresh object/array literal would compare unequal every render and loop.
   - The `onChange` you pass runs **during render**, so it must be pure — `setState` calls and nothing else. No fetches, DOM writes, logging, ref mutation, or parent callbacks: a render can be replayed (StrictMode) or abandoned (concurrent React), so external work would run an unpredictable number of times.
   - An effect is still the right tool for genuine synchronization with an external system (DOM measurement, `requestAnimationFrame`, subscriptions, timers). The rule is about deriving React state from React props, not about effects in general. `NetworkEntry` shows the split: the reveal's force-open is a state update and uses `useValueChange`, while its `requestAnimationFrame` scroll stays a `useEffect`.
+  - **Subscribing to an `@inspector/core` state store is `useSyncExternalStore`, never `useState` + a subscribing `useEffect`.** That second shape looks like the legitimate "synchronize with an external system" case above and is not, because it also *seeds and re-seeds local state from the store prop* — so it carries the same stale frame (switching servers paints the previous server's tools for one frame) plus a window where an event dispatched between the render and the effect is lost outright. Every hook in `core/react/` was converted away from it in #1955.
+    - Reach for **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`) when the getter returns a **fresh value per read** — a defensive copy (`getTools()` is `[...this.items]`) or a freshly built object (`getPagination()`). That is nearly all of them, and the caching it adds is what keeps `useSyncExternalStore` from looping. Call it once per value.
+    - When a getter's value is **already referentially stable**, subscribe with `useSyncExternalStore` directly and skip the helper — `useListError` does, because its snapshot is the stored `Error` instance itself (or `null`). The rule is read-during-render, not "always use the helper".
+    - Either way `useValueChange` is *not* the tool here — it lives in `clients/web/src`, and `core/react/` is consumed by the CLI and TUI too.
+    - `read` and `whenAbsent` must be **referentially stable across renders** — they are part of the snapshot's cache key. `read` is a function, so declare it at module scope. `whenAbsent` only needs a module-scope constant when it is an **object or array** (`NO_TOOLS`, `NO_PAGINATION`); a primitive fallback (`false`, `undefined`, `"disconnected"`) is already stable under `Object.is` and is passed inline throughout these hooks.
+    - An unstable one **fails quietly, so don't expect to be told**: measured on React 19, an inline `read` throws nothing, logs nothing — not even React's "getSnapshot should be cached" dev warning, whose double-call happens within a single render where the closure is unchanged — and forces no extra render. It simply returns a fresh value every render, defeating every downstream `useMemo` / `React.memo` / effect dep that keys on it.
+    - The snapshot is cached against the store's **per-event dispatch counter** (`TypedEventTarget.getEventRevision`), which every dispatch advances automatically — not against the snapshot's contents. That is deliberate and load-bearing: these getters return a defensive copy, so contents are the only alternative, and a contents comparison cannot see a dispatch that mutated an entry the list already holds (`MessageLogState` folding a response into its request entry does exactly that). Don't "optimize" it into a shallow compare.
+    - Consequently the **store is the source of truth and the event is only the signal** — the hook re-reads the store rather than taking the event's `detail`. A test that fires a store event must put the value on the store first; dispatching alone announces a change that isn't there. A hand-rolled fake must extend the real `TypedEventTarget` (a bare `EventTarget` has no `getEventRevision` and fails at runtime).
+    - Genuinely local state accumulated from an event stream, with no getter to read it back from, stays `useState` — `useInspectorClient`'s `lastError` is the one such case. Reset it on a store swap **during render**, not in an effect.
 - Theme files vs. Storybook element components
   - **Theme files** (`src/theme/<Component>.ts`) and **element components** (`src/components/elements/`) serve different purposes and both are needed.
   - Theme files customize every instance of a Mantine component app-wide — defaults (size, radius), custom variants, and global style overrides. They are applied automatically by `MantineProvider`.

README.md

@@ -43,7 +43,7 @@ inspector/
 │   ├── mcp/          # InspectorClient runtime, state stores, transports, config import,
 │   │                 #   and the RFC 6570 URI-template helpers the web form and TUI expand through
 │   ├── node/         # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection)
-│   ├── react/        # React hooks over the state stores
+│   ├── react/        # React hooks over the state stores, each reading its snapshot during render
 │   └── storage/      # File I/O helpers for the OAuth persist backends
 ├── test-servers/     # Composable MCP test servers + fixtures used by integration tests
 ├── scripts/          # Root build/verify tooling (install cascade, smokes, verify-build-gate, verify-format-coverage, verify-dep-lockstep, pack:verify)

clients/web/src/App.test.tsx

@@ -44,12 +44,21 @@ const { messageLogClear } = vi.hoisted(() => ({ messageLogClear: vi.fn() }));
 // trigger the handlers). See #1368.
 
 // --- Fake InspectorClient ---------------------------------------------------
-// Extends EventTarget so the App's `addEventListener("disconnect", …)` wiring
-// is real; the test fires `dispatchEvent(new Event("disconnect"))` to simulate
-// any of the three disconnect paths (toggle, header button, transport failure).
+// Extends the real TypedEventTarget so the App's `addEventListener("disconnect",
+// …)` wiring is real; the test fires `dispatchEvent(new Event("disconnect"))` to
+// simulate any of the three disconnect paths (toggle, header button, transport
+// failure).
+//
+// The real base rather than a bare EventTarget because the hooks this fake is
+// handed to read state through `useStoreSnapshot`, which caches its snapshot
+// against `getEventRevision` — a method the base owns and a bare EventTarget
+// does not have (#1955). Inheriting it also means the counter is advanced by
+// the `dispatchEvent` calls below, exactly as it would be in production.
 vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
   const actual =
     await importOriginal<typeof import("@inspector/core/mcp/index.js")>();
+  const { TypedEventTarget } =
+    await import("@inspector/core/mcp/typedEventTarget.js");
   // Each armed value makes one `connect()` reject, in FIFO order, so a test can
   // exercise the handshake-failure path — or a two-connect sequence such as the
   // auth-recovery retry, where the first call rejects with the recovery error
@@ -68,7 +77,9 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
   // control-flow decision from its resolving `false` (#2108): a throw is
   // surfaced as a failed attempt rather than falling through to the redirect.
   let nextChallengeCheckRejection: unknown = null;
-  class FakeInspectorClient extends EventTarget {
+  class FakeInspectorClient extends TypedEventTarget<
+    import("@inspector/core/mcp/index.js").InspectorClientEventMap
+  > {
     connect = vi.fn(() => {
       if (connectRejections.length > 0) {
         return Promise.reject(connectRejections.shift());
@@ -695,6 +706,50 @@ const clientInstances = (
   McpIndex as unknown as { __clientInstances: EventTarget[] }
 ).__clientInstances;
 
+/**
+ * Mirror how the real client enqueues a server-initiated request: put it on the
+ * queue, then announce the change.
+ *
+ * Both halves matter now that `usePendingClientRequests` reads the queue back
+ * off the client (`useStoreSnapshot`, #1955) rather than taking the event's
+ * `detail`. The store is the source of truth and the event is only the signal
+ * that it moved, so dispatching alone would announce a change that isn't there
+ * — and the modal under test would never open.
+ */
+function enqueuePendingRequests(
+  client: EventTarget,
+  queue: "getPendingSamples" | "getPendingElicitations",
+  event: "pendingSamplesChange" | "pendingElicitationsChange",
+  entries: readonly unknown[],
+): void {
+  const withQueue = client as EventTarget &
+    Record<typeof queue, ReturnType<typeof vi.fn>>;
+  withQueue[queue].mockReturnValue(entries);
+  client.dispatchEvent(new CustomEvent(event, { detail: entries }));
+}
+
+const enqueuePendingSamples = (
+  client: EventTarget,
+  samples: readonly unknown[],
+): void =>
+  enqueuePendingRequests(
+    client,
+    "getPendingSamples",
+    "pendingSamplesChange",
+    samples,
+  );
+
+const enqueuePendingElicitations = (
+  client: EventTarget,
+  elicitations: readonly unknown[],
+): void =>
+  enqueuePendingRequests(
+    client,
+    "getPendingElicitations",
+    "pendingElicitationsChange",
+    elicitations,
+  );
+
 // The mock factory adds four test-only arming hooks to the module namespace.
 // Intersecting with `typeof McpIndex` keeps the real module's shape checked and
 // narrows this to a single cast — `as unknown as` would discard the former and
@@ -1417,9 +1472,7 @@ describe("App pending server-initiated request modal", () => {
       reject: vi.fn(),
     };
     act(() => {
-      clientInstances[0].dispatchEvent(
-        new CustomEvent("pendingSamplesChange", { detail: [sample] }),
-      );
+      enqueuePendingSamples(clientInstances[0], [sample]);
     });
 
     await waitFor(() =>
@@ -1433,9 +1486,7 @@ describe("App pending server-initiated request modal", () => {
 
     // The client clearing its queue (empty event) closes the modal.
     act(() => {
-      clientInstances[0].dispatchEvent(
-        new CustomEvent("pendingSamplesChange", { detail: [] }),
-      );
+      enqueuePendingSamples(clientInstances[0], []);
     });
     await waitFor(() =>
       expect(screen.queryByText("Sampling Request")).not.toBeInTheDocument(),
@@ -1581,9 +1632,7 @@ describe("App task wiring", () => {
       respond,
     };
     act(() => {
-      clientInstances[0].dispatchEvent(
-        new CustomEvent("pendingElicitationsChange", { detail: [elicitation] }),
-      );
+      enqueuePendingElicitations(clientInstances[0], [elicitation]);
     });
 
     await waitFor(() =>