fix: read the list error via useSyncExternalStore; clear correlation maps
Two fixes from Copilot's review of #1953. useManagedListError re-synced React state from the `state` prop inside a useEffect, which renders one frame carrying the PREVIOUS store's error after `state` changes (switching servers) before the effect corrects it — the pattern AGENTS.md forbids. useSyncExternalStore reads the snapshot during render, so a store swap lands in the same frame, and it also closes the window where an error recorded between render and subscribe was missed. The snapshot returns the stored Error instance (or null), so it is referentially stable as the API requires. The `markResponseRejected` correlation maps were never cleared. Ids of requests that never got a response — a timeout, a dropped connection — are the only entries trackResponse can't remove, so they accumulated across reconnects. Cleared on the start-clean connect path, per the convention documented on resetSessionState (one route out, `onerror` with no `onclose`, tears down nothing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
This commit is contained in:
parent
fad8480376
commit
a6f233eb6a
1 changed files with 66 additions and 19 deletions
|
|
@ -1,4 +1,4 @@
|
|||
import { useState, useEffect } from "react";
|
||||
import { useCallback, useSyncExternalStore } from "react";
|
||||
import type { ManagedListEventMap } from "../mcp/state/managedListState.js";
|
||||
import type { TypedEventGeneric } from "../mcp/typedEventTarget.js";
|
||||
|
||||
|
|
@ -30,28 +30,39 @@ export interface ManagedListErrorSource {
|
|||
* Shared by the four `useManaged*` hooks so a list load that fails — including
|
||||
* the connect-time one, which has no caller to await it — reaches the UI
|
||||
* instead of only the console. `null` means the last fetch succeeded.
|
||||
*
|
||||
* Built on `useSyncExternalStore` rather than the `useState` + `useEffect`
|
||||
* subscribe pattern the sibling hooks use. Re-syncing state from the `state`
|
||||
* prop inside an effect would render one frame carrying the PREVIOUS store's
|
||||
* error after `state` changes (switching servers) before the effect corrects
|
||||
* it — the "don't derive state from props in an effect" rule in AGENTS.md.
|
||||
* `useSyncExternalStore` has no such window: the snapshot is read during
|
||||
* render, so a store swap is reflected in the same frame, and it also closes
|
||||
* the gap where an error recorded between render and subscribe would be missed.
|
||||
*
|
||||
* The snapshot must be referentially stable across reads that mean "no change",
|
||||
* which it is: it returns the stored `Error` instance itself (or `null`), never
|
||||
* a fresh object.
|
||||
*/
|
||||
export function useManagedListError(
|
||||
state: ManagedListErrorSource | null,
|
||||
): Error | null {
|
||||
const [error, setError] = useState<Error | null>(state?.getError() ?? null);
|
||||
const subscribe = useCallback(
|
||||
(onStoreChange: () => void) => {
|
||||
if (!state) return () => {};
|
||||
const listener = () => onStoreChange();
|
||||
state.addEventListener("errorChange", listener);
|
||||
return () => {
|
||||
state.removeEventListener("errorChange", listener);
|
||||
};
|
||||
},
|
||||
[state],
|
||||
);
|
||||
|
||||
useEffect(() => {
|
||||
if (!state) {
|
||||
setError(null);
|
||||
return;
|
||||
}
|
||||
setError(state.getError());
|
||||
const onErrorChange = (
|
||||
event: TypedEventGeneric<ManagedListEventMap, "errorChange">,
|
||||
) => {
|
||||
setError(event.detail);
|
||||
};
|
||||
state.addEventListener("errorChange", onErrorChange);
|
||||
return () => {
|
||||
state.removeEventListener("errorChange", onErrorChange);
|
||||
};
|
||||
}, [state]);
|
||||
const getSnapshot = useCallback(() => state?.getError() ?? null, [state]);
|
||||
|
||||
return error;
|
||||
// Server snapshot: same read. The 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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue