core/react hooks re-sync state from props in an effect (stale frame on store swap)

Noticed during Copilot's review of #1954 (which fixed the same bug in the new useManagedListError hook).

The problem

The state-subscription hooks in core/react/ seed local React state from their store prop and re-sync it inside a useEffect:

useEffect(() => {
  if (!managedToolsState) { setTools([]); return; }
  setTools(managedToolsState.getTools());     // <-- re-sync from prop
  // ...subscribe
}, [managedToolsState]);

Two consequences:

A stale frame on store swap. When the store prop changes — switching servers — the component renders once with the previous store's data before the effect runs and corrects it. React renders twice and the user can see the wrong frame.
A missed-update window. An event dispatched between render and the effect subscribing is lost, because the snapshot was taken before the listener was attached.

This is the pattern AGENTS.md explicitly forbids under State and effects ("NEVER reset or re-sync local state from a prop inside a useEffect"), and which react-hooks/set-state-in-effect is meant to catch — it isn't firing on this shape today, which is worth understanding as part of the fix.

Affected hooks

All in core/react/:

useManagedTools, useManagedPrompts, useManagedResources, useManagedResourceTemplates
useManagedRequestorTasks
usePagedTools, usePagedPrompts, usePagedResources, usePagedResourceTemplates, usePagedRequestorTasks
useMessageLog, useFetchRequestLog, useStderrLog, usePendingClientRequests, useResourceSubscriptions

(Worth auditing the whole directory rather than trusting this list.)

Suggested approach

Convert to useSyncExternalStore, as useManagedListError now does (see #1954). It reads the snapshot during render, so a store swap lands in the same frame, and subscribing is atomic with the read.

The catch specific to these hooks: getSnapshot must be referentially stable across reads meaning "no change". Most of these stores return a defensive copy (getTools() is [...this.items]), so a naive useSyncExternalStore(subscribe, () => state.getTools()) returns a fresh array every read and loops infinitely. Each store needs to expose a stable snapshot — cache the array and replace the reference only when the contents change — which is the bulk of the work and why this is worth its own issue rather than a drive-by.

useValueChange (clients/web/src/hooks/useValueChange.ts) is the pattern AGENTS.md points to, but it isn't reachable from core/react/, which the CLI and TUI also consume. Either approach needs to work for all three clients.

Notes

Not user-visible in most flows today — the stale frame lasts one render and the missed-update window is small — which is why it has gone unnoticed. It becomes visible when switching between servers whose lists differ.

I am going to reproduce this against the current v2/main branch and build a local prototype covering the full core/react surface. I will report back with the exact implementation prompt and verification evidence for both failure modes: a stale render when swapping stores and an update between render and subscription. Per the repository contribution policy, I will not open an external pull request.

I completed a local prototype against v2/main at dd67164beaa55ff8b2903bc54e8bdeaf96072ba7.

Exact implementation prompt used

On the current v2/main, fix #1955 across all 15 hooks named in the issue. Start by adding regression tests that record every render during a store/client swap and prove that no render exposes the previous store. Add a second regression that changes a store between the initial snapshot read and listener installation, proving the post-subscription recheck observes it. Replace the useState plus useEffect mirroring pattern with a shared useSyncExternalStore helper that subscribes to the stores' existing events. Make every array/object snapshot referentially stable: retain defensive-copy getters for imperative callers, add stable snapshot getters for React, replace references only when state changes, and keep pagination snapshots as stable objects. For pending client queues whose protocol getters return defensive arrays, preserve stable snapshots per client without changing the public protocol. Preserve refresh/clear behavior, teardown, error handling, and event semantics across managed lists, paged lists, logs, pending peer requests, and resource subscriptions. Update test doubles so event notification and getter state obey the same external-store contract as the real client. Run formatting, lint, type checking, production builds, unit tests, targeted coverage, mutation verification, and smoke tests. Do not weaken tests or open an external pull request.

Behavior

Before: swapping a store/client committed one render with the old store's data, and an event between render and effect subscription could be missed.
After: the snapshot is read during render and rechecked after subscription, so swaps are immediate and the render-to-subscribe window is closed. Stable snapshots also avoid the infinite-loop failure caused by fresh defensive arrays.

The implementation covers managed tools/prompts/resources/resource templates/requestor tasks; paged tools/prompts/resources/resource templates/requestor tasks; message/fetch/stderr logs; pending client requests; and resource subscriptions.

Verification evidence

TDD red phase: the new regressions produced 6 failures before the implementation (five stale-swap cases plus the missed-update case).
Green phase: the focused hook/state suite passed 611/611; the full Web unit suite passed 4905/4905 across 293 files.
Mutation check: temporarily returning a fresh array from a snapshot getter produced React's “getSnapshot should be cached” warning and 12 hook-test failures; restoring the stable reference returned the suite to 15/15.
Targeted coverage for the 15 hooks plus shared helper: 100% statements, branches, functions, and lines.
format, core/Web/CLI/TUI/launcher validation, TypeScript, production build, build gate, bundle-externals gate, and all Web/CLI/TUI/launcher smoke tests passed.
The branch remained exactly aligned with origin/v2/main, and a final collision check found no related open PR.

The aggregate npm run ci cannot be fully green in this Windows checkout for unrelated existing platform/environment reasons: POSIX permission/mount integration tests fail on Windows, several remote transport/OAuth integration tests time out, the dependency-lockstep fixture is path-sensitive under the non-ASCII checkout, and the current Storybook runner reports no test suites for story files. None of those paths were modified. The affected React/state tests and their per-file coverage are green; upstream Linux CI should provide the independent final gate.

Per the contribution policy, I have not pushed the local branch or opened a pull request.

Merge branch 'v2/main' into v2/chore/2163-agent-skills

fix(core): read state stores during render instead of re-syncing in a…

Navigation Menu

Repository navigation

core/react hooks re-sync state from props in an effect (stale frame on store swap) #1955

The problem

Affected hooks

Suggested approach

Notes

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

core/react hooks re-sync state from props in an effect (stale frame on store swap) #1955

Description

The problem

Affected hooks

Suggested approach

Notes

Activity

kyletser commented on Aug 26, 2026

kyletser commented on Aug 26, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions