diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d8352a62..766dc977 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -55,7 +55,8 @@ 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. Use **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`), once per value; `useValueChange` is web-only and `core/react/` is shared with the CLI and TUI. Pass `read`/`whenAbsent` as **module-scope constants**. +- **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`. Pass `read`/`whenAbsent` as **module-scope constants**: they are part of the cache key, so an unstable one trips React's "getSnapshot should be cached" error. - 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`). diff --git a/AGENTS.md b/AGENTS.md index 549b30f5..25abd8ac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -393,12 +393,16 @@ v2/main/ │ │ # callback listener; also stripBrackets. Used across │ │ # clients/web/server, clients/cli, and core/auth/node — #1795) │ ├── react/ # React hooks over the state stores. Every store -│ │ # subscription reads through useStoreSnapshot.ts -│ │ # (useSyncExternalStore + a snapshot cached against -│ │ # TypedEventTarget's per-event dispatch counter), so a -│ │ # store swap lands in the same frame and an event -│ │ # dispatched between render and subscribe is not lost -│ │ # — #1955 +│ │ # 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 @@ -1212,8 +1216,10 @@ Nothing **enforces** the boundary: no path alias keys off it, and the coverage ` - 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. - - Use **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`) for one value driven by one store event; call it once per value. `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 **module-scope constants**, not built in the component body: they are `getSnapshot` dependencies, and a fresh `[]` literal per render defeats the caching for no reason. + - 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 **module-scope constants**, not built in the component body. They are part of the snapshot's cache key, so an unstable one defeats the cache and React fails it outright with "The result of getSnapshot should be cached" — deliberately, since the alternative is silently serving a value derived from the previous argument. - 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. diff --git a/README.md b/README.md index 53527241..e6855ceb 100644 --- a/README.md +++ b/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, all reading through useStoreSnapshot +│ ├── 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) diff --git a/clients/web/src/test/core/react/useStoreSnapshot.test.tsx b/clients/web/src/test/core/react/useStoreSnapshot.test.tsx index e26482e7..9b277eff 100644 --- a/clients/web/src/test/core/react/useStoreSnapshot.test.tsx +++ b/clients/web/src/test/core/react/useStoreSnapshot.test.tsx @@ -196,6 +196,66 @@ describe("useStoreSnapshot", () => { expect(result.current).toBe(NO_ITEMS); }); + it("re-reads when the reader changes under an unchanged store", () => { + // The cache key covers every input the value was derived from, not just + // the store and its revision — otherwise a new reader is answered with the + // previous reader's output. + store.add("a"); + const readLabels = (s: FakeStore): string[] => + s.getItems().map((i) => i.label); + const readCount = (s: FakeStore): string[] => [String(s.getItems().length)]; + + const { result, rerender } = renderHook( + ({ read }: { read: (s: FakeStore) => string[] }) => + useStoreSnapshot(store, "itemsChange", read, []), + { initialProps: { read: readLabels } }, + ); + expect(result.current).toEqual(["a"]); + + rerender({ read: readCount }); + expect(result.current).toEqual(["1"]); + }); + + it("re-reads when the fallback changes while the store is absent", () => { + // With no store the revision is pinned at 0, so the revision alone can + // never notice this one. + const first: Item[] = [{ label: "first-fallback" }]; + const second: Item[] = [{ label: "second-fallback" }]; + + const { result, rerender } = renderHook( + ({ absent }: { absent: Item[] }) => + useStoreSnapshot(null, "itemsChange", readItems, absent), + { initialProps: { absent: first } }, + ); + expect(result.current).toBe(first); + + rerender({ absent: second }); + expect(result.current).toBe(second); + }); + + it("re-reads when the watched event changes", () => { + store.add("a"); + // Widen the prop up front rather than at each `rerender` — `initialProps` + // is what fixes the type, so a bare literal there pins it to that one + // event and the swap below stops compiling. + const initialProps: { event: keyof FakeStoreEventMap } = { + event: "itemsChange", + }; + const { result, rerender } = renderHook( + ({ event }: { event: keyof FakeStoreEventMap }) => + useStoreSnapshot(store, event, readItems, NO_ITEMS), + { initialProps }, + ); + const first = result.current; + + // Same store and same reader, so only the event distinguishes the two — + // and each event carries its own revision, so the cached entry must not + // be reused across them. + rerender({ event: "otherChange" }); + expect(result.current).not.toBe(first); + expect(result.current).toEqual([{ label: "a" }]); + }); + it("stops listening on unmount", () => { const { result, unmount } = renderHook(() => useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), diff --git a/core/react/useStoreSnapshot.ts b/core/react/useStoreSnapshot.ts index 3b9bbf93..ea466bfc 100644 --- a/core/react/useStoreSnapshot.ts +++ b/core/react/useStoreSnapshot.ts @@ -52,12 +52,19 @@ export interface SnapshotStore { * * ## Caller contract * - * `read` and `whenAbsent` should be module-scope constants, not values built - * in the component body. They are `getSnapshot` dependencies, so a fresh - * closure or a fresh `[]` literal per render makes React re-run its - * snapshot check every render. It stays *correct* if they aren't — the cache - * is keyed on the store and its revision, not on these — but there is no - * reason to pay for it. + * `read` and `whenAbsent` must be module-scope constants, not values built in + * the component body — every caller in this directory declares them beside the + * hook. + * + * They are part of the cache key below, along with `event`, so that a changed + * argument can never be answered with a value computed from the previous one. + * The consequence is that passing a fresh closure (or a fresh `[]`) per render + * defeats the cache entirely: each read returns a new defensive copy, and + * React fails it outright with "The result of getSnapshot should be cached". + * That is the intended failure mode. The alternative — keying only on the + * store and its revision — would tolerate an unstable argument by silently + * serving a stale value, and a loud error at first render beats a wrong list + * nobody can account for. * * @param store the state store, or `null` when no server is active * @param event the store event that signals this value changed @@ -75,6 +82,9 @@ export function useStoreSnapshot< // (store, revision) that any render can recompute identically. const cache = useRef<{ store: S | null; + event: E; + read: (store: S) => V; + whenAbsent: V; revision: number; value: V; } | null>(null); @@ -95,15 +105,22 @@ export function useStoreSnapshot< // arrived while nothing was listening is still visible here. const revision = store ? store.getEventRevision(event) : 0; const cached = cache.current; + // Every input the value was derived from is part of the key, not just the + // store and its revision: a changed `read` (or `event`, or — with a null + // store, where the revision is pinned at 0 — a changed `whenAbsent`) means + // the cached value answers a question nobody asked any more. if ( cached && Object.is(cached.store, store) && + cached.event === event && + cached.read === read && + Object.is(cached.whenAbsent, whenAbsent) && cached.revision === revision ) { return cached.value; } const value = store ? read(store) : whenAbsent; - cache.current = { store, revision, value }; + cache.current = { store, event, read, whenAbsent, revision, value }; return value; }, [store, event, read, whenAbsent]);