Copilot review round 2.
The contract claimed an unstable `read`/`whenAbsent` trips React's "The
result of getSnapshot should be cached" error. It does not. Measured on
React 19: an inline `read` closure throws nothing, logs nothing, and forces
no extra render pass. React's dev check calls getSnapshot twice within a
single render, where the closure is unchanged, so the cache answers both
calls identically and the warning never fires.
The real cost is quiet — a fresh value every render, defeating downstream
useMemo/React.memo/effect deps — which is a better reason to state the
contract than an error that never arrives.
Also stop requiring module scope for every fallback. A primitive
`whenAbsent` (`false`, `undefined`, `"disconnected"`) is stable under
Object.is and is passed inline throughout these hooks, so the rule
contradicted the code it was describing. Only object/array fallbacks need a
constant, which is what NO_TOOLS and NO_PAGINATION are.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
The cache was keyed on the store and its revision alone, so a changed
`read` was answered with the previous reader's output, and with a null
store — where the revision is pinned at 0 — a changed `whenAbsent` was
ignored outright. Key on `event`, `read` and `whenAbsent` too.
This makes an unstable argument defeat the cache rather than silently
serve a value derived from the previous one, so React fails it with "The
result of getSnapshot should be cached" at first render. That is the
intended failure mode and is now documented as such: a loud error beats a
wrong list nobody can account for.
Also correct the docs, which described the helper as universal. That
misread useListError as a violation: its snapshot is the stored Error
instance, already referentially stable, so it subscribes with
useSyncExternalStore directly and needs no cache. The rule is
read-during-render; the helper is for getters that return a fresh value
per read.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
