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