From 50502269f413447ee1e6c87837468451922263f0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 02:07:53 -0400 Subject: [PATCH] fix(core): read state stores during render instead of re-syncing in an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every state-subscription hook in core/react/ seeded local React state from its store prop and re-synced it inside a useEffect. That shape paints one frame of the previous store's data when the prop changes (switching servers), and loses any event dispatched between the render that took the snapshot and the effect that attached the listener. Convert all 16 to useSyncExternalStore through a new shared useStoreSnapshot, following useManagedListError (#1954). useSyncExternalStore needs a referentially stable snapshot, and these getters hand back a defensive copy, so TypedEventTarget now keeps a per-event-type dispatch counter (getEventRevision) that the snapshot is cached against. A contents comparison would not do: MessageLogState folds a response into its request entry by mutating that entry in place, which a shallow compare reports as unchanged. The bump lives on the dispatchEvent override so no dispatch — including a hand-rolled CustomEvent — can bypass it. useInspectorClient is included though the issue did not list it; its nine subscribed slices made the stale frame most visible. Its lastError stays useState (accumulated from events, no getter to read back) but resets on client swap during render rather than in an effect. The settings-draft and async fetch hooks are deliberately untouched: the former hold genuinely local editable state, the latter are I/O rather than store subscriptions. Also answers the issue's open question. react-hooks/set-state-in-effect never fired here because eslint-plugin-react-hooks has never linted core/react/ at all — it is a clients/web devDependency and the root config loads no React plugin. Corrected the config comment that gave "no JSX" as the reason; the gap itself is #2192, measured at 15 findings, none of them in the hooks converted here. Closes #1955 Signed-off-by: cliffhall --- .github/copilot-instructions.md | 3 + AGENTS.md | 14 +- README.md | 2 +- clients/web/src/App.test.tsx | 75 ++++- .../test/core/react/useStoreSnapshot.test.tsx | 218 +++++++++++++++ core/mcp/typedEventTarget.ts | 49 ++++ core/react/pagination.ts | 15 + core/react/useFetchRequestLog.ts | 51 ++-- core/react/useInspectorClient.ts | 256 ++++++++---------- core/react/useManagedPrompts.ts | 79 ++---- core/react/useManagedRequestorTasks.ts | 57 ++-- core/react/useManagedResourceTemplates.ts | 70 ++--- core/react/useManagedResources.ts | 88 ++---- core/react/useManagedTools.ts | 72 ++--- core/react/useMessageLog.ts | 48 ++-- core/react/usePagedPrompts.ts | 69 ++--- core/react/usePagedRequestorTasks.ts | 64 ++--- core/react/usePagedResourceTemplates.ts | 65 ++--- core/react/usePagedResources.ts | 76 ++---- core/react/usePagedTools.ts | 67 ++--- core/react/usePendingClientRequests.ts | 72 ++--- core/react/useResourceSubscriptions.ts | 73 ++--- core/react/useStderrLog.ts | 45 ++- core/react/useStoreSnapshot.ts | 114 ++++++++ eslint.config.js | 17 +- 25 files changed, 987 insertions(+), 772 deletions(-) create mode 100644 clients/web/src/test/core/react/useStoreSnapshot.test.tsx create mode 100644 core/react/pagination.ts create mode 100644 core/react/useStoreSnapshot.ts diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 96b1bd74..d8352a62 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -55,6 +55,9 @@ 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**. +- 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 diff --git a/AGENTS.md b/AGENTS.md index 5dfdb218..549b30f5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -392,7 +392,13 @@ 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 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 │ └── 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 +1211,12 @@ 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. + - 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. + - 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/.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`. diff --git a/README.md b/README.md index b9efa7bf..53527241 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 +│ ├── react/ # React hooks over the state stores, all reading through useStoreSnapshot │ └── 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/App.test.tsx b/clients/web/src/App.test.tsx index 82937f28..f9fc7f8a 100644 --- a/clients/web/src/App.test.tsx +++ b/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(); + 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>; + 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(() => diff --git a/clients/web/src/test/core/react/useStoreSnapshot.test.tsx b/clients/web/src/test/core/react/useStoreSnapshot.test.tsx new file mode 100644 index 00000000..e26482e7 --- /dev/null +++ b/clients/web/src/test/core/react/useStoreSnapshot.test.tsx @@ -0,0 +1,218 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { TypedEventTarget } from "@inspector/core/mcp/typedEventTarget"; +import { useStoreSnapshot } from "@inspector/core/react/useStoreSnapshot"; + +interface Item { + label: string; + seen?: boolean; +} + +interface FakeStoreEventMap { + itemsChange: Item[]; + otherChange: number; +} + +/** + * A minimal stand-in for the real state stores, reproducing the two properties + * that make them awkward to read from `useSyncExternalStore`: the getter hands + * back a fresh defensive copy on every call, and the list is mutated in place + * (both by appending and, in `markSeen`, by mutating an entry the list already + * holds — the shape `MessageLogState` uses when it folds a response into its + * request entry). + */ +class FakeStore extends TypedEventTarget { + private items: Item[] = []; + + getItems(): Item[] { + return [...this.items]; + } + + add(label: string): void { + this.items.push({ label }); + this.dispatchTypedEvent("itemsChange", this.getItems()); + } + + /** Mutate an existing entry in place, then announce it. */ + markSeen(label: string): void { + const entry = this.items.find((i) => i.label === label); + if (entry) entry.seen = true; + this.dispatchTypedEvent("itemsChange", this.getItems()); + } + + /** A dispatch on a different event, to prove per-event isolation. */ + bumpOther(): void { + this.dispatchTypedEvent("otherChange", 1); + } +} + +const NO_ITEMS: Item[] = []; +const readItems = (store: FakeStore): Item[] => store.getItems(); + +/** + * The shared subscription primitive behind every list/log hook in + * `core/react` (#1955). Its job is to read a store during render — closing + * both the stale-frame-on-store-swap and the missed-update windows the old + * `useState` + `useEffect` shape carried — while still handing + * `useSyncExternalStore` a referentially stable snapshot. + */ +describe("useStoreSnapshot", () => { + let store: FakeStore; + + beforeEach(() => { + store = new FakeStore(); + }); + + it("reports the fallback when there is no store", () => { + const { result } = renderHook(() => + useStoreSnapshot(null, "itemsChange", readItems, NO_ITEMS), + ); + expect(result.current).toBe(NO_ITEMS); + }); + + it("reads the store's current value on the first render", () => { + store.add("a"); + const { result } = renderHook(() => + useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), + ); + expect(result.current).toEqual([{ label: "a" }]); + }); + + it("updates when the store dispatches its event", () => { + const { result } = renderHook(() => + useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), + ); + expect(result.current).toEqual([]); + + act(() => store.add("a")); + expect(result.current).toEqual([{ label: "a" }]); + }); + + it("returns the identical value across renders with no dispatch", () => { + store.add("a"); + const { result, rerender } = renderHook(() => + useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), + ); + const first = result.current; + + rerender(); + rerender(); + + // Identity, not equality: a fresh array per read is what makes + // `useSyncExternalStore` loop, and what defeats every downstream memo. + expect(result.current).toBe(first); + }); + + it("reflects a store swap in the same render, with no stale frame", () => { + const next = new FakeStore(); + store.add("from-first"); + next.add("from-second"); + + // Every render's value, captured during render rather than after commit — + // the whole point is that no committed frame ever carries the previous + // store's data. + const rendered: Item[][] = []; + const { rerender } = renderHook( + ({ s }: { s: FakeStore }) => { + const value = useStoreSnapshot(s, "itemsChange", readItems, NO_ITEMS); + rendered.push(value); + return value; + }, + { initialProps: { s: store } }, + ); + expect(rendered.at(-1)).toEqual([{ label: "from-first" }]); + + const rendersBeforeSwap = rendered.length; + rerender({ s: next }); + + // Not one render after the swap carried the old store's list. + expect(rendered.slice(rendersBeforeSwap)).not.toContainEqual([ + { label: "from-first" }, + ]); + expect(rendered.at(-1)).toEqual([{ label: "from-second" }]); + }); + + it("picks up a change dispatched between the render and the subscribe", () => { + // The second defect the conversion fixes: the old shape took its snapshot + // during render and attached its listener in an effect, so anything the + // store did in between was lost. Reproduce that window exactly, by having + // the store change on its way into `addEventListener`. + const racing = new FakeStore(); + const realAdd = racing.addEventListener.bind(racing); + racing.addEventListener = (( + type: "itemsChange", + listener: (event: Event) => void, + ) => { + racing.add("slipped-in"); + realAdd(type, listener); + }) as FakeStore["addEventListener"]; + + const { result } = renderHook(() => + useStoreSnapshot(racing, "itemsChange", readItems, NO_ITEMS), + ); + + expect(result.current).toEqual([{ label: "slipped-in" }]); + }); + + it("re-reads when a dispatch mutated an entry the list already held", () => { + // The `MessageLogState` fold: same entries, same length, same references — + // only a field inside one of them changed. Caching against the dispatch + // count rather than the contents is what keeps this visible. + store.add("a"); + const { result } = renderHook(() => + useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), + ); + const before = result.current; + + act(() => store.markSeen("a")); + + expect(result.current).not.toBe(before); + expect(result.current).toEqual([{ label: "a", seen: true }]); + }); + + it("ignores a dispatch of a different event on the same store", () => { + store.add("a"); + const { result, rerender } = renderHook(() => + useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), + ); + const before = result.current; + + act(() => store.bumpOther()); + rerender(); + + expect(result.current).toBe(before); + }); + + it("falls back when the store is taken away", () => { + store.add("a"); + const { result, rerender } = renderHook( + ({ s }: { s: FakeStore | null }) => + useStoreSnapshot(s, "itemsChange", readItems, NO_ITEMS), + { initialProps: { s: store as FakeStore | null } }, + ); + expect(result.current).toEqual([{ label: "a" }]); + + rerender({ s: null }); + expect(result.current).toBe(NO_ITEMS); + }); + + it("stops listening on unmount", () => { + const { result, unmount } = renderHook(() => + useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), + ); + const before = result.current; + unmount(); + + // No act(): after unmount there is nothing to re-render, and a listener + // still attached here would be a leak rather than an update. + store.add("a"); + expect(result.current).toBe(before); + }); + + it("unmounts cleanly with no store to unsubscribe from", () => { + const { unmount } = renderHook(() => + useStoreSnapshot(null, "itemsChange", readItems, NO_ITEMS), + ); + expect(() => unmount()).not.toThrow(); + }); +}); diff --git a/core/mcp/typedEventTarget.ts b/core/mcp/typedEventTarget.ts index 32923b91..75133c72 100644 --- a/core/mcp/typedEventTarget.ts +++ b/core/mcp/typedEventTarget.ts @@ -25,6 +25,55 @@ export class TypedEventGeneric< * implementation so the subclass remains assignable to EventTarget. */ export class TypedEventTarget extends EventTarget { + /** + * Per-event-type dispatch counter. Read through `getEventRevision` by + * `useStoreSnapshot` (core/react) to tell "nothing has happened on this + * event since I last looked" from "something has", without comparing the + * snapshot's contents (#1955). + */ + private readonly eventRevisions = new Map(); + + /** + * How many times an event of this type has been dispatched from this + * target. Monotonic per type, and per type only: an `errorChange` does not + * advance `toolsChange`, so a subscriber to one is not re-read because of + * the other. + * + * The counter — not the list's contents — is what a `useSyncExternalStore` + * snapshot is cached against. That distinction is load-bearing rather than + * an optimization: these stores hand out a defensive copy, so comparing + * contents is the only alternative, and a contents comparison cannot see an + * in-place mutation of an entry the list already holds. `MessageLogState` + * does exactly that when a response is folded into its request entry + * (filling in `response`/`duration`), and it dispatches — so the counter + * moves and the fold reaches the UI, where a shallow compare would report + * "unchanged" and never re-render it. + */ + getEventRevision(type: keyof EventMap & string): number { + return this.eventRevisions.get(type) ?? 0; + } + + /** + * Overridden solely to advance the counter. It sits on `dispatchEvent` + * rather than on `dispatchTypedEvent` because that is the one funnel every + * dispatch passes through — the typed helper below delegates to it, and so + * does a hand-rolled `dispatchEvent(new CustomEvent(...))`, which the tests + * use extensively. A counter bumped only in the typed helper would be + * silently stale for those, i.e. exactly where nobody would look for it. + * + * Bumped BEFORE dispatching: the state mutation always precedes the + * dispatch, so by the time a listener runs the counter must already reflect + * it, or a snapshot read from inside that listener would return the cached + * pre-change value. + */ + override dispatchEvent(event: Event): boolean { + this.eventRevisions.set( + event.type, + (this.eventRevisions.get(event.type) ?? 0) + 1, + ); + return super.dispatchEvent(event); + } + dispatchTypedEvent( type: K, ...args: EventMap[K] extends void ? [] : [detail: EventMap[K]] diff --git a/core/react/pagination.ts b/core/react/pagination.ts new file mode 100644 index 00000000..95e010a7 --- /dev/null +++ b/core/react/pagination.ts @@ -0,0 +1,15 @@ +import type { PagePaginationState } from "../mcp/state/pagedToolsState.js"; + +/** + * The pagination snapshot reported while no paged store is attached (no + * active server): nothing loaded, nowhere to go next. + * + * Module scope and frozen because it is a `useStoreSnapshot` fallback — a + * fresh object literal per render would make the snapshot look different on + * every read. Shared by the three `usePaged*` list hooks, which is safe + * precisely because it is immutable. + */ +export const NO_PAGINATION: PagePaginationState = Object.freeze({ + nextCursor: undefined, + pageCount: 0, +}); diff --git a/core/react/useFetchRequestLog.ts b/core/react/useFetchRequestLog.ts index 3eaf5b37..64fa43e8 100644 --- a/core/react/useFetchRequestLog.ts +++ b/core/react/useFetchRequestLog.ts @@ -1,10 +1,16 @@ -import { useState, useEffect } from "react"; import type { FetchRequestEntry } from "../mcp/types.js"; -import type { - FetchRequestLogState, - FetchRequestLogStateEventMap, -} from "../mcp/state/fetchRequestLogState.js"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import type { FetchRequestLogState } from "../mcp/state/fetchRequestLogState.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_FETCH_REQUESTS: FetchRequestEntry[] = []; + +const readFetchRequests = (state: FetchRequestLogState): FetchRequestEntry[] => + state.getFetchRequests(); export interface UseFetchRequestLogResult { fetchRequests: FetchRequestEntry[]; @@ -17,35 +23,12 @@ export interface UseFetchRequestLogResult { export function useFetchRequestLog( fetchRequestLogState: FetchRequestLogState | null, ): UseFetchRequestLogResult { - const [fetchRequests, setFetchRequests] = useState( - fetchRequestLogState?.getFetchRequests() ?? [], + const fetchRequests = useStoreSnapshot( + fetchRequestLogState, + "fetchRequestsChange", + readFetchRequests, + NO_FETCH_REQUESTS, ); - useEffect(() => { - if (!fetchRequestLogState) { - setFetchRequests([]); - return; - } - setFetchRequests(fetchRequestLogState.getFetchRequests()); - const onFetchRequestsChange = ( - event: TypedEventGeneric< - FetchRequestLogStateEventMap, - "fetchRequestsChange" - >, - ) => { - setFetchRequests(event.detail); - }; - fetchRequestLogState.addEventListener( - "fetchRequestsChange", - onFetchRequestsChange, - ); - return () => { - fetchRequestLogState.removeEventListener( - "fetchRequestsChange", - onFetchRequestsChange, - ); - }; - }, [fetchRequestLogState]); - return { fetchRequests }; } diff --git a/core/react/useInspectorClient.ts b/core/react/useInspectorClient.ts index 5e9de64a..1d22a94c 100644 --- a/core/react/useInspectorClient.ts +++ b/core/react/useInspectorClient.ts @@ -1,4 +1,4 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback, useEffect, useState } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { AppRendererClient } from "../mcp/inspectorClientProtocol.js"; import type { TypedEvent } from "../mcp/inspectorClientEventTarget.js"; @@ -12,6 +12,7 @@ import type { } from "@modelcontextprotocol/client"; import type { ExcludedTool } from "../mcp/types.js"; import type { MalformedListItem } from "../mcp/listSalvage.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; // Module-scope frozen object so the `?? EMPTY_CLIENT_CAPABILITIES` // fallback below doesn't return a fresh literal on every render — @@ -19,6 +20,40 @@ import type { MalformedListItem } from "../mcp/listSalvage.js"; // would otherwise invalidate every tick when no client is attached. const EMPTY_CLIENT_CAPABILITIES: ClientCapabilities = Object.freeze({}); +/** + * Stable fallbacks for the no-client case, module scope for the same reason as + * the constant above — a `useStoreSnapshot` fallback built in the component + * body would look like a new value on every read. + */ +const NO_EXCLUDED_TOOLS: ExcludedTool[] = []; +const NO_MALFORMED_LIST_ITEMS: MalformedListItem[] = []; + +const readStatus = (client: InspectorClientProtocol): ConnectionStatus => + client.getStatus(); +const readCapabilities = ( + client: InspectorClientProtocol, +): ServerCapabilities | undefined => client.getCapabilities(); +const readServerInfo = ( + client: InspectorClientProtocol, +): Implementation | undefined => client.getServerInfo(); +const readInstructions = ( + client: InspectorClientProtocol, +): string | undefined => client.getInstructions(); +const readProtocolVersion = ( + client: InspectorClientProtocol, +): string | undefined => client.getProtocolVersion(); +const readProtocolEra = ( + client: InspectorClientProtocol, +): ProtocolEra | undefined => client.getProtocolEra(); +const readDiscoverResult = ( + client: InspectorClientProtocol, +): DiscoverResult | undefined => client.getDiscoverResult(); +const readExcludedTools = (client: InspectorClientProtocol): ExcludedTool[] => + client.getExcludedTools(); +const readMalformedListItems = ( + client: InspectorClientProtocol, +): MalformedListItem[] => client.getMalformedListItems(); + export interface UseInspectorClientResult { status: ConnectionStatus; capabilities?: ServerCapabilities; @@ -69,6 +104,11 @@ export interface UseInspectorClientResult { * connection state. Log lists (message / stderr / fetch) live in dedicated * state managers consumed via useMessageLog / useStderrLog / useFetchRequestLog. * + * Every value the client can be *asked* for is read through `useStoreSnapshot` + * rather than mirrored into local state by an effect (#1955): the effect ran + * after the commit, so swapping servers painted one frame of the previous + * client's status and capabilities before correcting itself. + * * Note: `appRendererClient` is read lazily from the client on every render * and is NOT subscribed. It changes once at connect time and is not expected * to change again during a session, so callers will see the current value @@ -80,169 +120,99 @@ export interface UseInspectorClientResult { export function useInspectorClient( inspectorClient: InspectorClientProtocol | null, ): UseInspectorClientResult { - const [status, setStatus] = useState( - inspectorClient?.getStatus() ?? "disconnected", + const status = useStoreSnapshot( + inspectorClient, + "statusChange", + readStatus, + "disconnected" as ConnectionStatus, ); - const [capabilities, setCapabilities] = useState< - ServerCapabilities | undefined - >(inspectorClient?.getCapabilities()); - const [serverInfo, setServerInfo] = useState( - inspectorClient?.getServerInfo(), + const capabilities = useStoreSnapshot( + inspectorClient, + "capabilitiesChange", + readCapabilities, + undefined, ); - const [instructions, setInstructions] = useState( - inspectorClient?.getInstructions(), + const serverInfo = useStoreSnapshot( + inspectorClient, + "serverInfoChange", + readServerInfo, + undefined, ); - const [protocolVersion, setProtocolVersion] = useState( - inspectorClient?.getProtocolVersion(), + const instructions = useStoreSnapshot( + inspectorClient, + "instructionsChange", + readInstructions, + undefined, ); - const [protocolEra, setProtocolEra] = useState( - inspectorClient?.getProtocolEra(), + const protocolVersion = useStoreSnapshot( + inspectorClient, + "protocolVersionChange", + readProtocolVersion, + undefined, ); - const [discoverResult, setDiscoverResult] = useState< - DiscoverResult | undefined - >(inspectorClient?.getDiscoverResult()); - const [excludedTools, setExcludedTools] = useState( - inspectorClient?.getExcludedTools() ?? [], + const protocolEra = useStoreSnapshot( + inspectorClient, + "protocolEraChange", + readProtocolEra, + undefined, ); - const [malformedListItems, setMalformedListItems] = useState< - MalformedListItem[] - >(inspectorClient?.getMalformedListItems() ?? []); - const [lastError, setLastError] = useState(undefined); + const discoverResult = useStoreSnapshot( + inspectorClient, + "discoverResultChange", + readDiscoverResult, + undefined, + ); + const excludedTools = useStoreSnapshot( + inspectorClient, + "excludedToolsChange", + readExcludedTools, + NO_EXCLUDED_TOOLS, + ); + const malformedListItems = useStoreSnapshot( + inspectorClient, + "malformedListItemsChange", + readMalformedListItems, + NO_MALFORMED_LIST_ITEMS, + ); + + // `lastError` is the one value here that is NOT a snapshot of something the + // client stores: there is no `getLastError()`, because the client emits the + // failure and moves on. It is accumulated from the event stream instead, so + // it stays local state — but it is still tied to one client, and must reset + // when a different one is passed in. + // + // That reset is done during render (React's documented "adjusting state + // when a prop changes" pattern, the same one `useValueChange` implements for + // the web client) rather than in an effect. An effect would paint one frame + // carrying the previous client's error, which is precisely the defect this + // hook was converted to fix. + const [errorState, setErrorState] = useState<{ + client: InspectorClientProtocol | null; + message?: string; + }>({ client: inspectorClient }); + if (errorState.client !== inspectorClient) { + setErrorState({ client: inspectorClient }); + } + const lastError = + errorState.client === inspectorClient ? errorState.message : undefined; useEffect(() => { - if (!inspectorClient) { - setStatus("disconnected"); - setCapabilities(undefined); - setServerInfo(undefined); - setInstructions(undefined); - setProtocolVersion(undefined); - setProtocolEra(undefined); - setDiscoverResult(undefined); - setExcludedTools([]); - setMalformedListItems([]); - setLastError(undefined); - return; - } - - setStatus(inspectorClient.getStatus()); - setCapabilities(inspectorClient.getCapabilities()); - setServerInfo(inspectorClient.getServerInfo()); - setInstructions(inspectorClient.getInstructions()); - setProtocolVersion(inspectorClient.getProtocolVersion()); - setProtocolEra(inspectorClient.getProtocolEra()); - setDiscoverResult(inspectorClient.getDiscoverResult()); - setExcludedTools(inspectorClient.getExcludedTools()); - setMalformedListItems(inspectorClient.getMalformedListItems()); - setLastError(undefined); - + if (!inspectorClient) return; const onStatusChange = (event: TypedEvent<"statusChange">) => { - setStatus(event.detail); // A fresh connection attempt clears any stale error from the prior // session so the UI doesn't keep showing why the last transport died. if (event.detail === "connecting") { - setLastError(undefined); + setErrorState({ client: inspectorClient }); } }; const onError = (event: TypedEvent<"error">) => { - setLastError(event.detail.message); + setErrorState({ client: inspectorClient, message: event.detail.message }); }; - const onCapabilitiesChange = (event: TypedEvent<"capabilitiesChange">) => { - setCapabilities(event.detail); - }; - const onServerInfoChange = (event: TypedEvent<"serverInfoChange">) => { - setServerInfo(event.detail); - }; - const onInstructionsChange = (event: TypedEvent<"instructionsChange">) => { - setInstructions(event.detail); - }; - const onProtocolVersionChange = ( - event: TypedEvent<"protocolVersionChange">, - ) => { - setProtocolVersion(event.detail); - }; - const onProtocolEraChange = (event: TypedEvent<"protocolEraChange">) => { - setProtocolEra(event.detail); - }; - const onDiscoverResultChange = ( - event: TypedEvent<"discoverResultChange">, - ) => { - setDiscoverResult(event.detail); - }; - const onMalformedListItemsChange = ( - event: TypedEvent<"malformedListItemsChange">, - ) => { - setMalformedListItems(event.detail); - }; - const onExcludedToolsChange = ( - event: TypedEvent<"excludedToolsChange">, - ) => { - setExcludedTools(event.detail); - }; - inspectorClient.addEventListener("statusChange", onStatusChange); inspectorClient.addEventListener("error", onError); - inspectorClient.addEventListener( - "capabilitiesChange", - onCapabilitiesChange, - ); - inspectorClient.addEventListener("serverInfoChange", onServerInfoChange); - inspectorClient.addEventListener( - "instructionsChange", - onInstructionsChange, - ); - inspectorClient.addEventListener( - "protocolVersionChange", - onProtocolVersionChange, - ); - inspectorClient.addEventListener("protocolEraChange", onProtocolEraChange); - inspectorClient.addEventListener( - "discoverResultChange", - onDiscoverResultChange, - ); - inspectorClient.addEventListener( - "excludedToolsChange", - onExcludedToolsChange, - ); - inspectorClient.addEventListener( - "malformedListItemsChange", - onMalformedListItemsChange, - ); - return () => { inspectorClient.removeEventListener("statusChange", onStatusChange); inspectorClient.removeEventListener("error", onError); - inspectorClient.removeEventListener( - "capabilitiesChange", - onCapabilitiesChange, - ); - inspectorClient.removeEventListener( - "serverInfoChange", - onServerInfoChange, - ); - inspectorClient.removeEventListener( - "instructionsChange", - onInstructionsChange, - ); - inspectorClient.removeEventListener( - "protocolVersionChange", - onProtocolVersionChange, - ); - inspectorClient.removeEventListener( - "protocolEraChange", - onProtocolEraChange, - ); - inspectorClient.removeEventListener( - "discoverResultChange", - onDiscoverResultChange, - ); - inspectorClient.removeEventListener( - "excludedToolsChange", - onExcludedToolsChange, - ); - inspectorClient.removeEventListener( - "malformedListItemsChange", - onMalformedListItemsChange, - ); }; }, [inspectorClient]); diff --git a/core/react/useManagedPrompts.ts b/core/react/useManagedPrompts.ts index 8a14d0df..35bc01f8 100644 --- a/core/react/useManagedPrompts.ts +++ b/core/react/useManagedPrompts.ts @@ -1,12 +1,21 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; -import type { - ManagedPromptsState, - ManagedPromptsStateEventMap, -} from "../mcp/state/managedPromptsState.js"; +import type { ManagedPromptsState } from "../mcp/state/managedPromptsState.js"; import type { Prompt } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_PROMPTS: Prompt[] = []; + +const readPrompts = (state: ManagedPromptsState): Prompt[] => + state.getPrompts(); +const readListChanged = (state: ManagedPromptsState): boolean => + state.getListChanged(); export interface UseManagedPromptsResult { /** @@ -30,52 +39,23 @@ export function useManagedPrompts( client: InspectorClientProtocol | null, managedPromptsState: ManagedPromptsState | null, ): UseManagedPromptsResult { - const [prompts, setPrompts] = useState( - managedPromptsState?.getPrompts() ?? [], + const prompts = useStoreSnapshot( + managedPromptsState, + "promptsChange", + readPrompts, + NO_PROMPTS, ); - const [listChanged, setListChanged] = useState( - managedPromptsState?.getListChanged() ?? false, + const listChanged = useStoreSnapshot( + managedPromptsState, + "listChangedChange", + readListChanged, + false, ); - useEffect(() => { - if (!managedPromptsState) { - setPrompts([]); - setListChanged(false); - return; - } - setPrompts(managedPromptsState.getPrompts()); - setListChanged(managedPromptsState.getListChanged()); - const onPromptsChange = ( - event: TypedEventGeneric, - ) => { - setPrompts(event.detail); - }; - const onListChangedChange = ( - event: TypedEventGeneric< - ManagedPromptsStateEventMap, - "listChangedChange" - >, - ) => { - setListChanged(event.detail); - }; - managedPromptsState.addEventListener("promptsChange", onPromptsChange); - managedPromptsState.addEventListener( - "listChangedChange", - onListChangedChange, - ); - return () => { - managedPromptsState.removeEventListener("promptsChange", onPromptsChange); - managedPromptsState.removeEventListener( - "listChangedChange", - onListChangedChange, - ); - }; - }, [managedPromptsState]); - const error = useListError(managedPromptsState); const refresh = useCallback(async (): Promise => { - if (!managedPromptsState || !client) return []; + if (!managedPromptsState || !client) return NO_PROMPTS; // A user-initiated refresh acknowledges the change — clear the indicator // BEFORE awaiting the fetch, not after. If a `prompts/list_changed` arrives // mid-fetch, the state re-sets the flag (and auto-refreshes); clearing @@ -84,10 +64,9 @@ export function useManagedPrompts( managedPromptsState.clearListChanged(); // A user-initiated refresh forces a cache-bypassing round trip // (`cacheMode: "refresh"`) so a modern server's `ttlMs`-cached list can't - // return stale — and re-stores the fresh aggregate. - const next = await managedPromptsState.refresh(undefined, "refresh"); - setPrompts(next); - return next; + // return stale — and re-stores the fresh aggregate. The store dispatches + // `promptsChange` as it commits, so the snapshot above updates on its own. + return managedPromptsState.refresh(undefined, "refresh"); }, [client, managedPromptsState]); const clearListChanged = useCallback(() => { diff --git a/core/react/useManagedRequestorTasks.ts b/core/react/useManagedRequestorTasks.ts index 05d23ac1..25dc6b5d 100644 --- a/core/react/useManagedRequestorTasks.ts +++ b/core/react/useManagedRequestorTasks.ts @@ -1,11 +1,18 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; -import type { - ManagedRequestorTasksState, - ManagedRequestorTasksStateEventMap, -} from "../mcp/state/managedRequestorTasksState.js"; +import type { ManagedRequestorTasksState } from "../mcp/state/managedRequestorTasksState.js"; import type { Task } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_TASKS: Task[] = []; + +const readTasks = (state: ManagedRequestorTasksState): Task[] => + state.getTasks(); export interface UseManagedRequestorTasksResult { tasks: Task[]; @@ -21,38 +28,18 @@ export function useManagedRequestorTasks( client: InspectorClientProtocol | null, managedRequestorTasksState: ManagedRequestorTasksState | null, ): UseManagedRequestorTasksResult { - const [tasks, setTasks] = useState( - managedRequestorTasksState?.getTasks() ?? [], + const tasks = useStoreSnapshot( + managedRequestorTasksState, + "tasksChange", + readTasks, + NO_TASKS, ); - useEffect(() => { - if (!managedRequestorTasksState) { - setTasks([]); - return; - } - setTasks(managedRequestorTasksState.getTasks()); - const onTasksChange = ( - event: TypedEventGeneric< - ManagedRequestorTasksStateEventMap, - "tasksChange" - >, - ) => { - setTasks(event.detail); - }; - managedRequestorTasksState.addEventListener("tasksChange", onTasksChange); - return () => { - managedRequestorTasksState.removeEventListener( - "tasksChange", - onTasksChange, - ); - }; - }, [managedRequestorTasksState]); - const refresh = useCallback(async (): Promise => { - if (!managedRequestorTasksState || !client) return []; - const next = await managedRequestorTasksState.refresh(); - setTasks(next); - return next; + if (!managedRequestorTasksState || !client) return NO_TASKS; + // The store dispatches `tasksChange` as it commits the new list, so the + // snapshot above updates on its own — nothing local to push it into. + return managedRequestorTasksState.refresh(); }, [client, managedRequestorTasksState]); const clearCompleted = useCallback((): void => { diff --git a/core/react/useManagedResourceTemplates.ts b/core/react/useManagedResourceTemplates.ts index 61a4c382..9b129b73 100644 --- a/core/react/useManagedResourceTemplates.ts +++ b/core/react/useManagedResourceTemplates.ts @@ -1,12 +1,20 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; -import type { - ManagedResourceTemplatesState, - ManagedResourceTemplatesStateEventMap, -} from "../mcp/state/managedResourceTemplatesState.js"; +import type { ManagedResourceTemplatesState } from "../mcp/state/managedResourceTemplatesState.js"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_RESOURCE_TEMPLATES: ResourceTemplate[] = []; + +const readResourceTemplates = ( + state: ManagedResourceTemplatesState, +): ResourceTemplate[] => state.getResourceTemplates(); export interface UseManagedResourceTemplatesResult { /** @@ -27,49 +35,25 @@ export function useManagedResourceTemplates( client: InspectorClientProtocol | null, managedResourceTemplatesState: ManagedResourceTemplatesState | null, ): UseManagedResourceTemplatesResult { - const [resourceTemplates, setResourceTemplates] = useState< - ResourceTemplate[] - >(managedResourceTemplatesState?.getResourceTemplates() ?? []); - - useEffect(() => { - if (!managedResourceTemplatesState) { - setResourceTemplates([]); - return; - } - setResourceTemplates(managedResourceTemplatesState.getResourceTemplates()); - const onResourceTemplatesChange = ( - event: TypedEventGeneric< - ManagedResourceTemplatesStateEventMap, - "resourceTemplatesChange" - >, - ) => { - setResourceTemplates(event.detail); - }; - managedResourceTemplatesState.addEventListener( - "resourceTemplatesChange", - onResourceTemplatesChange, - ); - return () => { - managedResourceTemplatesState.removeEventListener( - "resourceTemplatesChange", - onResourceTemplatesChange, - ); - }; - }, [managedResourceTemplatesState]); + const resourceTemplates = useStoreSnapshot( + managedResourceTemplatesState, + "resourceTemplatesChange", + readResourceTemplates, + NO_RESOURCE_TEMPLATES, + ); const error = useListError(managedResourceTemplatesState); const refresh = useCallback(async (): Promise => { - if (!managedResourceTemplatesState || !client) return []; + if (!managedResourceTemplatesState || !client) { + return NO_RESOURCE_TEMPLATES; + } // A user-initiated refresh forces a cache-bypassing round trip // (`cacheMode: "refresh"`) so a modern server's `ttlMs`-cached list can't - // return stale — and re-stores the fresh aggregate. - const next = await managedResourceTemplatesState.refresh( - undefined, - "refresh", - ); - setResourceTemplates(next); - return next; + // return stale — and re-stores the fresh aggregate. The store dispatches + // `resourceTemplatesChange` as it commits, so the snapshot updates on its + // own. + return managedResourceTemplatesState.refresh(undefined, "refresh"); }, [client, managedResourceTemplatesState]); return { resourceTemplates, error, refresh }; diff --git a/core/react/useManagedResources.ts b/core/react/useManagedResources.ts index ad9b1eb5..972ab453 100644 --- a/core/react/useManagedResources.ts +++ b/core/react/useManagedResources.ts @@ -1,12 +1,21 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; -import type { - ManagedResourcesState, - ManagedResourcesStateEventMap, -} from "../mcp/state/managedResourcesState.js"; +import type { ManagedResourcesState } from "../mcp/state/managedResourcesState.js"; import type { Resource } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_RESOURCES: Resource[] = []; + +const readResources = (state: ManagedResourcesState): Resource[] => + state.getResources(); +const readListChanged = (state: ManagedResourcesState): boolean => + state.getListChanged(); export interface UseManagedResourcesResult { /** @@ -32,61 +41,23 @@ export function useManagedResources( client: InspectorClientProtocol | null, managedResourcesState: ManagedResourcesState | null, ): UseManagedResourcesResult { - const [resources, setResources] = useState( - managedResourcesState?.getResources() ?? [], + const resources = useStoreSnapshot( + managedResourcesState, + "resourcesChange", + readResources, + NO_RESOURCES, ); - const [listChanged, setListChanged] = useState( - managedResourcesState?.getListChanged() ?? false, + const listChanged = useStoreSnapshot( + managedResourcesState, + "listChangedChange", + readListChanged, + false, ); - useEffect(() => { - if (!managedResourcesState) { - setResources([]); - setListChanged(false); - return; - } - setResources(managedResourcesState.getResources()); - setListChanged(managedResourcesState.getListChanged()); - const onResourcesChange = ( - event: TypedEventGeneric< - ManagedResourcesStateEventMap, - "resourcesChange" - >, - ) => { - setResources(event.detail); - }; - const onListChangedChange = ( - event: TypedEventGeneric< - ManagedResourcesStateEventMap, - "listChangedChange" - >, - ) => { - setListChanged(event.detail); - }; - managedResourcesState.addEventListener( - "resourcesChange", - onResourcesChange, - ); - managedResourcesState.addEventListener( - "listChangedChange", - onListChangedChange, - ); - return () => { - managedResourcesState.removeEventListener( - "resourcesChange", - onResourcesChange, - ); - managedResourcesState.removeEventListener( - "listChangedChange", - onListChangedChange, - ); - }; - }, [managedResourcesState]); - const error = useListError(managedResourcesState); const refresh = useCallback(async (): Promise => { - if (!managedResourcesState || !client) return []; + if (!managedResourcesState || !client) return NO_RESOURCES; // A user-initiated refresh acknowledges the change — clear the indicator // BEFORE awaiting the fetch, not after. If a `resources/list_changed` // arrives mid-fetch, the state re-sets the flag (and auto-refreshes); @@ -95,10 +66,9 @@ export function useManagedResources( managedResourcesState.clearListChanged(); // A user-initiated refresh forces a cache-bypassing round trip // (`cacheMode: "refresh"`) so a modern server's `ttlMs`-cached list can't - // return stale — and re-stores the fresh aggregate. - const next = await managedResourcesState.refresh(undefined, "refresh"); - setResources(next); - return next; + // return stale — and re-stores the fresh aggregate. The store dispatches + // `resourcesChange` as it commits, so the snapshot updates on its own. + return managedResourcesState.refresh(undefined, "refresh"); }, [client, managedResourcesState]); const clearListChanged = useCallback(() => { diff --git a/core/react/useManagedTools.ts b/core/react/useManagedTools.ts index 3b805038..3c126e6c 100644 --- a/core/react/useManagedTools.ts +++ b/core/react/useManagedTools.ts @@ -1,10 +1,20 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { ManagedToolsState } from "../mcp/state/managedToolsState.js"; -import type { ManagedToolsStateEventMap } from "../mcp/state/managedToolsState.js"; import type { Tool } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_TOOLS: Tool[] = []; + +const readTools = (state: ManagedToolsState): Tool[] => state.getTools(); +const readListChanged = (state: ManagedToolsState): boolean => + state.getListChanged(); export interface UseManagedToolsResult { /** @@ -33,49 +43,23 @@ export function useManagedTools( client: InspectorClientProtocol | null, managedToolsState: ManagedToolsState | null, ): UseManagedToolsResult { - const [tools, setTools] = useState( - managedToolsState?.getTools() ?? [], + const tools = useStoreSnapshot( + managedToolsState, + "toolsChange", + readTools, + NO_TOOLS, ); - const [listChanged, setListChanged] = useState( - managedToolsState?.getListChanged() ?? false, + const listChanged = useStoreSnapshot( + managedToolsState, + "listChangedChange", + readListChanged, + false, ); - useEffect(() => { - if (!managedToolsState) { - setTools([]); - setListChanged(false); - return; - } - setTools(managedToolsState.getTools()); - setListChanged(managedToolsState.getListChanged()); - const onToolsChange = ( - event: TypedEventGeneric, - ) => { - setTools(event.detail); - }; - const onListChangedChange = ( - event: TypedEventGeneric, - ) => { - setListChanged(event.detail); - }; - managedToolsState.addEventListener("toolsChange", onToolsChange); - managedToolsState.addEventListener( - "listChangedChange", - onListChangedChange, - ); - return () => { - managedToolsState.removeEventListener("toolsChange", onToolsChange); - managedToolsState.removeEventListener( - "listChangedChange", - onListChangedChange, - ); - }; - }, [managedToolsState]); - const error = useListError(managedToolsState); const refresh = useCallback(async (): Promise => { - if (!managedToolsState || !client) return []; + if (!managedToolsState || !client) return NO_TOOLS; // A user-initiated refresh acknowledges the change — clear the indicator // BEFORE awaiting the fetch, not after. If a `tools/list_changed` arrives // mid-fetch, the state re-sets the flag (and auto-refreshes); clearing @@ -84,10 +68,10 @@ export function useManagedTools( managedToolsState.clearListChanged(); // A user-initiated refresh forces a cache-bypassing round trip // (`cacheMode: "refresh"`) so a modern server's `ttlMs`-cached list can't - // return stale — and re-stores the fresh aggregate. - const next = await managedToolsState.refresh(undefined, "refresh"); - setTools(next); - return next; + // return stale — and re-stores the fresh aggregate. The store dispatches + // `toolsChange` as it commits, so the snapshot above updates on its own; + // there is no local state left to push the result into. + return managedToolsState.refresh(undefined, "refresh"); }, [client, managedToolsState]); const clearListChanged = useCallback(() => { diff --git a/core/react/useMessageLog.ts b/core/react/useMessageLog.ts index d00c5437..773079d8 100644 --- a/core/react/useMessageLog.ts +++ b/core/react/useMessageLog.ts @@ -1,10 +1,16 @@ -import { useState, useEffect } from "react"; import type { MessageEntry } from "../mcp/types.js"; -import type { - MessageLogState, - MessageLogStateEventMap, -} from "../mcp/state/messageLogState.js"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import type { MessageLogState } from "../mcp/state/messageLogState.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_MESSAGES: MessageEntry[] = []; + +const readMessages = (state: MessageLogState): MessageEntry[] => + state.getMessages(); export interface UseMessageLogResult { messages: MessageEntry[]; @@ -12,30 +18,22 @@ export interface UseMessageLogResult { /** * React hook that subscribes to MessageLogState and returns the message list. + * + * Note this log's changes are not all appends: folding a response into its + * request entry mutates that entry in place and dispatches. The snapshot is + * cached against the store's dispatch count rather than the list's contents + * precisely so that case still produces a new list identity and re-renders — + * see `TypedEventTarget.getEventRevision`. */ export function useMessageLog( messageLogState: MessageLogState | null, ): UseMessageLogResult { - const [messages, setMessages] = useState( - messageLogState?.getMessages() ?? [], + const messages = useStoreSnapshot( + messageLogState, + "messagesChange", + readMessages, + NO_MESSAGES, ); - useEffect(() => { - if (!messageLogState) { - setMessages([]); - return; - } - setMessages(messageLogState.getMessages()); - const onMessagesChange = ( - event: TypedEventGeneric, - ) => { - setMessages(event.detail); - }; - messageLogState.addEventListener("messagesChange", onMessagesChange); - return () => { - messageLogState.removeEventListener("messagesChange", onMessagesChange); - }; - }, [messageLogState]); - return { messages }; } diff --git a/core/react/usePagedPrompts.ts b/core/react/usePagedPrompts.ts index 6f62c524..67486429 100644 --- a/core/react/usePagedPrompts.ts +++ b/core/react/usePagedPrompts.ts @@ -1,14 +1,26 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { RequestMetadata } from "../mcp/types.js"; import type { PagedPromptsState, - PagedPromptsStateEventMap, LoadPageResult, } from "../mcp/state/pagedPromptsState.js"; +import type { PagePaginationState } from "../mcp/state/pagedToolsState.js"; import type { Prompt } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; +import { NO_PAGINATION } from "./pagination.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_PROMPTS: Prompt[] = []; + +const readPrompts = (state: PagedPromptsState): Prompt[] => state.getPrompts(); +const readPagination = (state: PagedPromptsState): PagePaginationState => + state.getPagination(); export interface UsePagedPromptsResult { prompts: Prompt[]; @@ -38,47 +50,18 @@ export function usePagedPrompts( client: InspectorClientProtocol | null, pagedPromptsState: PagedPromptsState | null, ): UsePagedPromptsResult { - const [prompts, setPrompts] = useState( - pagedPromptsState?.getPrompts() ?? [], + const prompts = useStoreSnapshot( + pagedPromptsState, + "promptsChange", + readPrompts, + NO_PROMPTS, ); - const [nextCursor, setNextCursor] = useState( - pagedPromptsState?.getPagination().nextCursor, + const { nextCursor, pageCount } = useStoreSnapshot( + pagedPromptsState, + "paginationChange", + readPagination, + NO_PAGINATION, ); - const [pageCount, setPageCount] = useState( - pagedPromptsState?.getPagination().pageCount ?? 0, - ); - - useEffect(() => { - if (!pagedPromptsState) { - setPrompts([]); - setNextCursor(undefined); - setPageCount(0); - return; - } - setPrompts(pagedPromptsState.getPrompts()); - setNextCursor(pagedPromptsState.getPagination().nextCursor); - setPageCount(pagedPromptsState.getPagination().pageCount); - const onPromptsChange = ( - event: TypedEventGeneric, - ) => { - setPrompts(event.detail); - }; - const onPaginationChange = ( - event: TypedEventGeneric, - ) => { - setNextCursor(event.detail.nextCursor); - setPageCount(event.detail.pageCount); - }; - pagedPromptsState.addEventListener("promptsChange", onPromptsChange); - pagedPromptsState.addEventListener("paginationChange", onPaginationChange); - return () => { - pagedPromptsState.removeEventListener("promptsChange", onPromptsChange); - pagedPromptsState.removeEventListener( - "paginationChange", - onPaginationChange, - ); - }; - }, [pagedPromptsState]); const error = useListError(pagedPromptsState); @@ -88,7 +71,7 @@ export function usePagedPrompts( metadata?: RequestMetadata, ): Promise => { if (!pagedPromptsState || !client) { - return { prompts: [], nextCursor: undefined }; + return { prompts: NO_PROMPTS, nextCursor: undefined }; } return pagedPromptsState.loadPage(cursor, metadata); }, diff --git a/core/react/usePagedRequestorTasks.ts b/core/react/usePagedRequestorTasks.ts index aefb4cc7..b87d78e1 100644 --- a/core/react/usePagedRequestorTasks.ts +++ b/core/react/usePagedRequestorTasks.ts @@ -1,12 +1,24 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { PagedRequestorTasksState, - PagedRequestorTasksStateEventMap, LoadPageResult, } from "../mcp/state/pagedRequestorTasksState.js"; import type { Task } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_TASKS: Task[] = []; + +const readTasks = (state: PagedRequestorTasksState): Task[] => state.getTasks(); +// This store has no dedicated pagination event — the cursor advances with the +// page, so it rides `tasksChange` like the list does. +const readNextCursor = (state: PagedRequestorTasksState): string | undefined => + state.getNextCursor() ?? undefined; export interface UsePagedRequestorTasksResult { tasks: Task[]; @@ -23,45 +35,27 @@ export function usePagedRequestorTasks( client: InspectorClientProtocol | null, pagedRequestorTasksState: PagedRequestorTasksState | null, ): UsePagedRequestorTasksResult { - const [tasks, setTasks] = useState( - pagedRequestorTasksState?.getTasks() ?? [], + const tasks = useStoreSnapshot( + pagedRequestorTasksState, + "tasksChange", + readTasks, + NO_TASKS, ); - const [nextCursor, setNextCursor] = useState( - pagedRequestorTasksState?.getNextCursor() ?? undefined, + const nextCursor = useStoreSnapshot( + pagedRequestorTasksState, + "tasksChange", + readNextCursor, + undefined, ); - useEffect(() => { - if (!pagedRequestorTasksState) { - setTasks([]); - setNextCursor(undefined); - return; - } - setTasks(pagedRequestorTasksState.getTasks()); - setNextCursor(pagedRequestorTasksState.getNextCursor() ?? undefined); - const onTasksChange = ( - event: TypedEventGeneric, - ) => { - setTasks(event.detail); - setNextCursor(pagedRequestorTasksState.getNextCursor() ?? undefined); - }; - pagedRequestorTasksState.addEventListener("tasksChange", onTasksChange); - return () => { - pagedRequestorTasksState.removeEventListener( - "tasksChange", - onTasksChange, - ); - }; - }, [pagedRequestorTasksState]); - const loadPage = useCallback( async (cursor?: string): Promise => { if (!pagedRequestorTasksState || !client) { - return { tasks: [], nextCursor: undefined }; + return { tasks: NO_TASKS, nextCursor: undefined }; } - const result = await pagedRequestorTasksState.loadPage(cursor); - setTasks(pagedRequestorTasksState.getTasks()); - setNextCursor(pagedRequestorTasksState.getNextCursor() ?? undefined); - return result; + // The store dispatches `tasksChange` as it commits the page, so both + // snapshots above update on their own. + return pagedRequestorTasksState.loadPage(cursor); }, [client, pagedRequestorTasksState], ); diff --git a/core/react/usePagedResourceTemplates.ts b/core/react/usePagedResourceTemplates.ts index c5202596..6798805b 100644 --- a/core/react/usePagedResourceTemplates.ts +++ b/core/react/usePagedResourceTemplates.ts @@ -1,13 +1,23 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { RequestMetadata } from "../mcp/types.js"; import type { PagedResourceTemplatesState, - PagedResourceTemplatesStateEventMap, LoadPageResult, } from "../mcp/state/pagedResourceTemplatesState.js"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_RESOURCE_TEMPLATES: ResourceTemplate[] = []; + +const readResourceTemplates = ( + state: PagedResourceTemplatesState, +): ResourceTemplate[] => state.getResourceTemplates(); export interface UsePagedResourceTemplatesResult { resourceTemplates: ResourceTemplate[]; @@ -26,35 +36,12 @@ export function usePagedResourceTemplates( client: InspectorClientProtocol | null, pagedResourceTemplatesState: PagedResourceTemplatesState | null, ): UsePagedResourceTemplatesResult { - const [resourceTemplates, setResourceTemplates] = useState< - ResourceTemplate[] - >(pagedResourceTemplatesState?.getResourceTemplates() ?? []); - - useEffect(() => { - if (!pagedResourceTemplatesState) { - setResourceTemplates([]); - return; - } - setResourceTemplates(pagedResourceTemplatesState.getResourceTemplates()); - const onResourceTemplatesChange = ( - event: TypedEventGeneric< - PagedResourceTemplatesStateEventMap, - "resourceTemplatesChange" - >, - ) => { - setResourceTemplates(event.detail); - }; - pagedResourceTemplatesState.addEventListener( - "resourceTemplatesChange", - onResourceTemplatesChange, - ); - return () => { - pagedResourceTemplatesState.removeEventListener( - "resourceTemplatesChange", - onResourceTemplatesChange, - ); - }; - }, [pagedResourceTemplatesState]); + const resourceTemplates = useStoreSnapshot( + pagedResourceTemplatesState, + "resourceTemplatesChange", + readResourceTemplates, + NO_RESOURCE_TEMPLATES, + ); const loadPage = useCallback( async ( @@ -62,14 +49,14 @@ export function usePagedResourceTemplates( metadata?: RequestMetadata, ): Promise => { if (!pagedResourceTemplatesState || !client) { - return { resourceTemplates: [], nextCursor: undefined }; + return { + resourceTemplates: NO_RESOURCE_TEMPLATES, + nextCursor: undefined, + }; } - const result = await pagedResourceTemplatesState.loadPage( - cursor, - metadata, - ); - setResourceTemplates(pagedResourceTemplatesState.getResourceTemplates()); - return result; + // The store dispatches `resourceTemplatesChange` as it commits the page, + // so the snapshot above updates on its own. + return pagedResourceTemplatesState.loadPage(cursor, metadata); }, [client, pagedResourceTemplatesState], ); diff --git a/core/react/usePagedResources.ts b/core/react/usePagedResources.ts index 00b52a26..0e60ed56 100644 --- a/core/react/usePagedResources.ts +++ b/core/react/usePagedResources.ts @@ -1,14 +1,27 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { RequestMetadata } from "../mcp/types.js"; import type { PagedResourcesState, - PagedResourcesStateEventMap, LoadPageResult, } from "../mcp/state/pagedResourcesState.js"; +import type { PagePaginationState } from "../mcp/state/pagedToolsState.js"; import type { Resource } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; +import { NO_PAGINATION } from "./pagination.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_RESOURCES: Resource[] = []; + +const readResources = (state: PagedResourcesState): Resource[] => + state.getResources(); +const readPagination = (state: PagedResourcesState): PagePaginationState => + state.getPagination(); export interface UsePagedResourcesResult { resources: Resource[]; @@ -38,53 +51,18 @@ export function usePagedResources( client: InspectorClientProtocol | null, pagedResourcesState: PagedResourcesState | null, ): UsePagedResourcesResult { - const [resources, setResources] = useState( - pagedResourcesState?.getResources() ?? [], + const resources = useStoreSnapshot( + pagedResourcesState, + "resourcesChange", + readResources, + NO_RESOURCES, ); - const [nextCursor, setNextCursor] = useState( - pagedResourcesState?.getPagination().nextCursor, + const { nextCursor, pageCount } = useStoreSnapshot( + pagedResourcesState, + "paginationChange", + readPagination, + NO_PAGINATION, ); - const [pageCount, setPageCount] = useState( - pagedResourcesState?.getPagination().pageCount ?? 0, - ); - - useEffect(() => { - if (!pagedResourcesState) { - setResources([]); - setNextCursor(undefined); - setPageCount(0); - return; - } - setResources(pagedResourcesState.getResources()); - setNextCursor(pagedResourcesState.getPagination().nextCursor); - setPageCount(pagedResourcesState.getPagination().pageCount); - const onResourcesChange = ( - event: TypedEventGeneric, - ) => { - setResources(event.detail); - }; - const onPaginationChange = ( - event: TypedEventGeneric, - ) => { - setNextCursor(event.detail.nextCursor); - setPageCount(event.detail.pageCount); - }; - pagedResourcesState.addEventListener("resourcesChange", onResourcesChange); - pagedResourcesState.addEventListener( - "paginationChange", - onPaginationChange, - ); - return () => { - pagedResourcesState.removeEventListener( - "resourcesChange", - onResourcesChange, - ); - pagedResourcesState.removeEventListener( - "paginationChange", - onPaginationChange, - ); - }; - }, [pagedResourcesState]); const error = useListError(pagedResourcesState); @@ -94,7 +72,7 @@ export function usePagedResources( metadata?: RequestMetadata, ): Promise => { if (!pagedResourcesState || !client) { - return { resources: [], nextCursor: undefined }; + return { resources: NO_RESOURCES, nextCursor: undefined }; } return pagedResourcesState.loadPage(cursor, metadata); }, diff --git a/core/react/usePagedTools.ts b/core/react/usePagedTools.ts index 844a862d..2ebf8a94 100644 --- a/core/react/usePagedTools.ts +++ b/core/react/usePagedTools.ts @@ -1,13 +1,25 @@ -import { useState, useEffect, useCallback } from "react"; +import { useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { PagedToolsState, - PagedToolsStateEventMap, + PagePaginationState, LoadPageResult, } from "../mcp/state/pagedToolsState.js"; import type { Tool } from "@modelcontextprotocol/client"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; +import { NO_PAGINATION } from "./pagination.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_TOOLS: Tool[] = []; + +const readTools = (state: PagedToolsState): Tool[] => state.getTools(); +const readPagination = (state: PagedToolsState): PagePaginationState => + state.getPagination(); export interface UsePagedToolsResult { tools: Tool[]; @@ -35,52 +47,25 @@ export function usePagedTools( client: InspectorClientProtocol | null, pagedToolsState: PagedToolsState | null, ): UsePagedToolsResult { - const [tools, setTools] = useState(pagedToolsState?.getTools() ?? []); - const [nextCursor, setNextCursor] = useState( - pagedToolsState?.getPagination().nextCursor, + const tools = useStoreSnapshot( + pagedToolsState, + "toolsChange", + readTools, + NO_TOOLS, ); - const [pageCount, setPageCount] = useState( - pagedToolsState?.getPagination().pageCount ?? 0, + const { nextCursor, pageCount } = useStoreSnapshot( + pagedToolsState, + "paginationChange", + readPagination, + NO_PAGINATION, ); - useEffect(() => { - if (!pagedToolsState) { - setTools([]); - setNextCursor(undefined); - setPageCount(0); - return; - } - setTools(pagedToolsState.getTools()); - setNextCursor(pagedToolsState.getPagination().nextCursor); - setPageCount(pagedToolsState.getPagination().pageCount); - const onToolsChange = ( - event: TypedEventGeneric, - ) => { - setTools(event.detail); - }; - const onPaginationChange = ( - event: TypedEventGeneric, - ) => { - setNextCursor(event.detail.nextCursor); - setPageCount(event.detail.pageCount); - }; - pagedToolsState.addEventListener("toolsChange", onToolsChange); - pagedToolsState.addEventListener("paginationChange", onPaginationChange); - return () => { - pagedToolsState.removeEventListener("toolsChange", onToolsChange); - pagedToolsState.removeEventListener( - "paginationChange", - onPaginationChange, - ); - }; - }, [pagedToolsState]); - const error = useListError(pagedToolsState); const loadPage = useCallback( async (cursor?: string): Promise => { if (!pagedToolsState || !client) { - return { tools: [], nextCursor: undefined }; + return { tools: NO_TOOLS, nextCursor: undefined }; } return pagedToolsState.loadPage(cursor); }, diff --git a/core/react/usePendingClientRequests.ts b/core/react/usePendingClientRequests.ts index a3759f1a..15cc3701 100644 --- a/core/react/usePendingClientRequests.ts +++ b/core/react/usePendingClientRequests.ts @@ -1,8 +1,25 @@ -import { useState, useEffect } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; import type { SamplingCreateMessage } from "../mcp/samplingCreateMessage.js"; import type { ElicitationCreateMessage } from "../mcp/elicitationCreateMessage.js"; -import type { TypedEvent } from "../mcp/inspectorClientEventTarget.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty lists for the no-client case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_SAMPLES: SamplingCreateMessage[] = []; +const NO_ELICITATIONS: ElicitationCreateMessage[] = []; + +// Both getters already hand back a defensive copy of a queue the client +// mutates in place (`push`/`splice`), which is what makes them safe to read as +// a snapshot: the returned array can't change underneath React afterwards. +const readPendingSamples = ( + client: InspectorClientProtocol, +): SamplingCreateMessage[] => client.getPendingSamples(); +const readPendingElicitations = ( + client: InspectorClientProtocol, +): ElicitationCreateMessage[] => client.getPendingElicitations(); export interface UsePendingClientRequestsResult { pendingSamples: SamplingCreateMessage[]; @@ -21,47 +38,18 @@ export interface UsePendingClientRequestsResult { export function usePendingClientRequests( inspectorClient: InspectorClientProtocol | null, ): UsePendingClientRequestsResult { - const [pendingSamples, setPendingSamples] = useState( - inspectorClient?.getPendingSamples() ?? [], + const pendingSamples = useStoreSnapshot( + inspectorClient, + "pendingSamplesChange", + readPendingSamples, + NO_SAMPLES, + ); + const pendingElicitations = useStoreSnapshot( + inspectorClient, + "pendingElicitationsChange", + readPendingElicitations, + NO_ELICITATIONS, ); - const [pendingElicitations, setPendingElicitations] = useState< - ElicitationCreateMessage[] - >(inspectorClient?.getPendingElicitations() ?? []); - - useEffect(() => { - if (!inspectorClient) { - setPendingSamples([]); - setPendingElicitations([]); - return; - } - setPendingSamples(inspectorClient.getPendingSamples()); - setPendingElicitations(inspectorClient.getPendingElicitations()); - - const onSamplesChange = (event: TypedEvent<"pendingSamplesChange">) => { - setPendingSamples([...event.detail]); - }; - const onElicitationsChange = ( - event: TypedEvent<"pendingElicitationsChange">, - ) => { - setPendingElicitations([...event.detail]); - }; - - inspectorClient.addEventListener("pendingSamplesChange", onSamplesChange); - inspectorClient.addEventListener( - "pendingElicitationsChange", - onElicitationsChange, - ); - return () => { - inspectorClient.removeEventListener( - "pendingSamplesChange", - onSamplesChange, - ); - inspectorClient.removeEventListener( - "pendingElicitationsChange", - onElicitationsChange, - ); - }; - }, [inspectorClient]); return { pendingSamples, pendingElicitations }; } diff --git a/core/react/useResourceSubscriptions.ts b/core/react/useResourceSubscriptions.ts index b71fe937..4b3c7c0c 100644 --- a/core/react/useResourceSubscriptions.ts +++ b/core/react/useResourceSubscriptions.ts @@ -1,14 +1,24 @@ -import { useState, useEffect } from "react"; -import type { - ResourceSubscriptionsState, - ResourceSubscriptionsStateEventMap, -} from "../mcp/state/resourceSubscriptionsState.js"; +import type { ResourceSubscriptionsState } from "../mcp/state/resourceSubscriptionsState.js"; import type { InspectorResourceSubscription, ResourceSubscriptionStreamState, } from "../mcp/types.js"; import { INACTIVE_SUBSCRIPTION_STREAM_STATE } from "../mcp/types.js"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_SUBSCRIPTIONS: InspectorResourceSubscription[] = []; + +const readSubscriptions = ( + state: ResourceSubscriptionsState, +): InspectorResourceSubscription[] => state.getSubscriptions(); +const readStreamState = ( + state: ResourceSubscriptionsState, +): ResourceSubscriptionStreamState => state.getStreamState(); export interface UseResourceSubscriptionsResult { subscriptions: InspectorResourceSubscription[]; @@ -29,45 +39,18 @@ export interface UseResourceSubscriptionsResult { export function useResourceSubscriptions( state: ResourceSubscriptionsState | null, ): UseResourceSubscriptionsResult { - const [subscriptions, setSubscriptions] = useState< - InspectorResourceSubscription[] - >(state?.getSubscriptions() ?? []); - const [streamState, setStreamState] = - useState( - state?.getStreamState() ?? INACTIVE_SUBSCRIPTION_STREAM_STATE, - ); - - useEffect(() => { - if (!state) { - setSubscriptions([]); - setStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE); - return; - } - setSubscriptions(state.getSubscriptions()); - setStreamState(state.getStreamState()); - const onSubscriptionsChange = ( - event: TypedEventGeneric< - ResourceSubscriptionsStateEventMap, - "subscriptionsChange" - >, - ) => { - setSubscriptions(event.detail); - }; - const onStreamStateChange = ( - event: TypedEventGeneric< - ResourceSubscriptionsStateEventMap, - "streamStateChange" - >, - ) => { - setStreamState(event.detail); - }; - state.addEventListener("subscriptionsChange", onSubscriptionsChange); - state.addEventListener("streamStateChange", onStreamStateChange); - return () => { - state.removeEventListener("subscriptionsChange", onSubscriptionsChange); - state.removeEventListener("streamStateChange", onStreamStateChange); - }; - }, [state]); + const subscriptions = useStoreSnapshot( + state, + "subscriptionsChange", + readSubscriptions, + NO_SUBSCRIPTIONS, + ); + const streamState = useStoreSnapshot( + state, + "streamStateChange", + readStreamState, + INACTIVE_SUBSCRIPTION_STREAM_STATE, + ); return { subscriptions, streamState }; } diff --git a/core/react/useStderrLog.ts b/core/react/useStderrLog.ts index 9df6b58e..9214dd66 100644 --- a/core/react/useStderrLog.ts +++ b/core/react/useStderrLog.ts @@ -1,10 +1,16 @@ -import { useState, useEffect } from "react"; import type { StderrLogEntry } from "../mcp/types.js"; -import type { - StderrLogState, - StderrLogStateEventMap, -} from "../mcp/state/stderrLogState.js"; -import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import type { StderrLogState } from "../mcp/state/stderrLogState.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty list for the no-server case. Module scope so the + * snapshot doesn't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a list this hook returns. + */ +const NO_STDERR_LOGS: StderrLogEntry[] = []; + +const readStderrLogs = (state: StderrLogState): StderrLogEntry[] => + state.getStderrLogs(); export interface UseStderrLogResult { stderrLogs: StderrLogEntry[]; @@ -16,29 +22,12 @@ export interface UseStderrLogResult { export function useStderrLog( stderrLogState: StderrLogState | null, ): UseStderrLogResult { - const [stderrLogs, setStderrLogs] = useState( - stderrLogState?.getStderrLogs() ?? [], + const stderrLogs = useStoreSnapshot( + stderrLogState, + "stderrLogsChange", + readStderrLogs, + NO_STDERR_LOGS, ); - useEffect(() => { - if (!stderrLogState) { - setStderrLogs([]); - return; - } - setStderrLogs(stderrLogState.getStderrLogs()); - const onStderrLogsChange = ( - event: TypedEventGeneric, - ) => { - setStderrLogs(event.detail); - }; - stderrLogState.addEventListener("stderrLogsChange", onStderrLogsChange); - return () => { - stderrLogState.removeEventListener( - "stderrLogsChange", - onStderrLogsChange, - ); - }; - }, [stderrLogState]); - return { stderrLogs }; } diff --git a/core/react/useStoreSnapshot.ts b/core/react/useStoreSnapshot.ts new file mode 100644 index 00000000..3b9bbf93 --- /dev/null +++ b/core/react/useStoreSnapshot.ts @@ -0,0 +1,114 @@ +import { useCallback, useRef, useSyncExternalStore } from "react"; + +/** + * The slice of a state store `useStoreSnapshot` needs, declared structurally + * rather than as a concrete class so every list/log hook can share it without + * threading its item type through. `TypedEventTarget` satisfies it, and the + * `E` parameter is what makes a mistyped event name a compile error: the + * concrete stores declare `getEventRevision(type: keyof EventMap & string)`, + * so a store is assignable to `SnapshotStore<"toolsChange">` only when that + * name is really in its event map. + */ +export interface SnapshotStore { + getEventRevision(type: E): number; + addEventListener(type: E, listener: (event: Event) => void): void; + removeEventListener(type: E, listener: (event: Event) => void): void; +} + +/** + * Subscribe to one event on one state store and read a value from it (#1955). + * + * This replaces the `useState(read(prop)) + useEffect(re-read; subscribe)` + * shape every list/log hook in this directory used to carry. That shape has + * two defects, both invisible in the common case and both fixed here: + * + * 1. **A stale frame when the store prop changes.** Switching servers hands + * the hook a different store, but the effect that re-reads it runs *after* + * the render commits — so React paints one frame of the previous server's + * tools, then corrects itself. `useSyncExternalStore` reads during render, + * so the swap lands in the same frame. + * 2. **A missed update between render and subscribe.** An event dispatched in + * that gap was lost, because the snapshot was taken before the listener + * was attached. React re-checks the snapshot immediately after subscribing, + * which closes the window — provided the snapshot is read from the live + * store rather than from something the (not-yet-attached) listener would + * have had to invalidate. That is why the cache below keys off the store's + * own dispatch counter and not off a dirty flag set by our listener. + * + * It is the same reasoning `useListError` documents; that hook needs no cache + * because its snapshot is the stored `Error` instance itself. + * + * ## Why a revision counter + * + * `useSyncExternalStore` requires `getSnapshot` to return a referentially + * stable value across reads that mean "no change" — otherwise every read looks + * like a new value and React re-renders forever. These stores return a + * defensive copy (`getTools()` is `[...this.items]`), so the raw getter cannot + * be used directly. Caching against `getEventRevision(event)` gives an O(1) + * check that is exact in both directions: unchanged revision returns the + * identical value, and any dispatch produces a fresh one — including a + * dispatch that mutated an entry *inside* the list, which a contents + * comparison could not detect (see `TypedEventTarget.getEventRevision`). + * + * ## 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. + * + * @param store the state store, or `null` when no server is active + * @param event the store event that signals this value changed + * @param read reads the value out of a non-null store + * @param whenAbsent the value to report while `store` is `null` + */ +export function useStoreSnapshot< + E extends string, + S extends SnapshotStore, + V, +>(store: S | null, event: E, read: (store: S) => V, whenAbsent: V): V { + // One cache cell per hook instance. Written from `getSnapshot`, which React + // calls during render — that is the documented shape for a cached + // `getSnapshot`, and it is safe here because the cell is a pure memo of + // (store, revision) that any render can recompute identically. + const cache = useRef<{ + store: S | null; + revision: number; + value: V; + } | null>(null); + + const subscribe = useCallback( + (onStoreChange: () => void) => { + if (!store) return () => {}; + store.addEventListener(event, onStoreChange); + return () => { + store.removeEventListener(event, onStoreChange); + }; + }, + [store, event], + ); + + const getSnapshot = useCallback((): V => { + // Read the revision from the live store on every call, so an event that + // arrived while nothing was listening is still visible here. + const revision = store ? store.getEventRevision(event) : 0; + const cached = cache.current; + if ( + cached && + Object.is(cached.store, store) && + cached.revision === revision + ) { + return cached.value; + } + const value = store ? read(store) : whenAbsent; + cache.current = { store, revision, value }; + return value; + }, [store, event, read, whenAbsent]); + + // Server snapshot: the same read. These stores are browser/Node runtime + // objects with no SSR path, and passing the same getter keeps hydration + // consistent rather than throwing on a server render. + return useSyncExternalStore(subscribe, getSnapshot, getSnapshot); +} diff --git a/eslint.config.js b/eslint.config.js index 17bca7e4..3fd534b1 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -8,8 +8,21 @@ import { defineConfig, globalIgnores } from "eslint/config"; // (#1689) and the root-owned "shared" surface — `test-servers/src/**`, the // root `vitest.shared.mts`, and this config file itself (#1767). `core/` is // isomorphic TypeScript (browser-side OAuth + Node backends + shared runtime); -// the shared surface is Node-only tooling/tests. Neither has JSX, so no React -// plugin is needed. +// 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. +// // 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).