perf(desktop): subscribe to session UI state at the granularity each surface reads - #1998

Astro-Han · 2026-08-03T11:26:01Z

Summary

AppShellContent destructured the whole session UI store, so every write to any slice re-rendered the entire shell — including one write per streamed token. The session list, the composer, and any open non-chat surface all re-rendered at token rate, though none of them read liveTurnBySession.

The controller was already an external store; it only lacked per-subscriber notification. This adds subscribe and a selector hook, then moves each read to the boundary that owns it:

useAppShellSessionUiReads is the shell's complete read, in one named place: six low-frequency maps by raw reference, plus a LiveTurnSnapshot and the sidebar's pulse set by value. A delta changes none of them.
ChatMessageSurface subscribes to the projection and the shell-run record itself. It is their only renderer, so they never reach the shell.
<LiveTurnReconciler/> carries the per-delta reconcile that used to sit in AppShell's effect list. It follows every delta and owns no subtree.

Two findings shaped this beyond the issue's original framing:

subscribe + selector alone saves nothing. The component holding the selector is AppShellContent. Selecting the full projection there re-renders the whole subtree exactly as before. What makes it work is that the shell's reads turn out to be low-entropy already — even homeSurfaceActive only wanted three .length === 0 booleans. Only ChatMessageSurface's two props genuinely need token-rate data, so only that read had to sink.

The chat surface needs no memo. The sidebar and composer stop re-rendering because their parent does, not because a comparison blocks them. That is why this avoids stabilizing ~25 props per panel.

useShellLiveTurn now takes the snapshot rather than the projection, and deriveModelWait takes booleans — it only ever asked whether the buffers were empty.

Rebased onto #1987. LiveTurnSnapshot carries turnId so deriveTurnActive keeps both its witnesses; the snapshot's phase is already absent once terminal, which is the other one. streamingTextComplete was dropped from the snapshot in the same pass — streamingMessageId is set only when the text step closed, so its presence already carried that fact.

Closes #1985

Verification

npm --workspace apps/desktop run test:dist — 1384 pass, 0 fail (includes fix(desktop): take the running turn from the run, not from session status #1987's deriveTurnActive matrix).
npm --workspace apps/desktop run typecheck — clean (preload, main, renderer, storybook).
npm run format:check / npm run lint — clean.
npm --workspace apps/desktop run e2e — 60 passed, 1 failed: plan-reminders.spec.ts:75 › closes a reminder menu before opening its edit dialog (an autofocus assertion in the reminder form). Pre-existing: it fails identically on unmodified origin/main at 1530ac4d5, verified by checking that commit out in the same worktree and running the same spec. Not addressed here.

Contract coverage in ui-render-memo-boundary-contract.test.ts, on the existing createRoot + act + fake-DOM harness — no new test framework:

a text delta re-renders a subscriber that selects the full projection, and leaves a subscriber selecting the semantic snapshot untouched;
driving useAppShellSessionUiReads itself, no delta after the first re-renders the shell;
switching activeId under a still store reads the new session on the first render.

Plus two snapshot facts in app-shell-session-ui-state.test.ts: a settled turn drops its phase but keeps its id, and the handoff message id stays absent while the text step is open.

Verified by mutation, not just by passing. Adding a state.liveTurnBySession selection to the production hook turns the render-count test red. Dropping the value-equality step does not merely over-render — useSyncExternalStore rejects the fresh snapshot outright with "The result of getSnapshot should be cached" and loops.

Not run: no screenshot or Storybook evidence — this PR removes renders and changes no pixels.

Review focus

useSyncExternalStore requires a snapshot that keeps its identity while nothing it selects changed, so a selector deriving a fresh object must supply an equality. Three places depend on that discipline:

getSnapshot must never build a Set/array/object without a matching comparator. ChatMessageSurface selects the raw per-session shell-run record and does Object.values in a useMemo for this reason.
Selectors are module-level and take what they vary by as arg, so the snapshot is memoized rather than published through a render-phase ref write — React permits that write only for lazy initialization, and a discarded concurrent render would otherwise hand its selector to the committed subscription.
LiveTurnSnapshot must stay free of buffers. Anything added to it whose identity changes per delta silently puts the whole shell back on the token path; the render-count contract is the guard.

Notification is synchronous, immediately after the state swap. A scheduler there (rAF or otherwise) would delay the terminal-turn handoff, which reads the state it announces.

activeSessionId is passed to ChatMessageSurface explicitly rather than derived from activeSession.id. They are equal today, but only because the shell substitutes a placeholder carrying the same id for an unsaved chat.

Known gap

ChatMessageSurface's own subscription is not pinned by a unit contract — removing it would leave these tests green and be caught only by the E2E streaming journey. Doing better means rendering the real surface with its full ChatView dependency tree, which the fake DOM does not currently support. Tracked as a follow-up rather than fixed here.

useAppShellSessionUiState still re-exports the controller's eleven setters alongside the controller itself. Collapsing that mirror touches every setter call site across the session-event and effect wiring, so it is deliberately out of scope.

`createAppShellSessionUiStateController` is an external store, but it only ever had a single `onChange` wired to one `forceRender()`. Add `subscribe` and a selector hook so a component can follow one derived reading of the store instead of every write to it (#1985). The selector caches on a caller-supplied equality: `useSyncExternalStore` requires a snapshot that keeps its identity while nothing it selects changed, so a selector deriving a fresh object must say what "unchanged" means for it. Without that the shell's own snapshot selector loops rather than merely over-rendering, which the new contract test pins. `LiveTurnSnapshot` is the low-entropy reading of a live turn — phase, a few booleans, the settled message id — everything the shell derives from the active projection except the streamed content itself. A text delta cannot change it. The aggregate top-level subscription still stands; moving its readers is the next commit.

AppShell destructured the whole session UI store, so a write to any slice re-rendered the entire shell — including one write per streamed token. The subscription now matches what each surface reads (#1985): - AppShell selects the six low-frequency maps by raw reference, plus a `LiveTurnSnapshot` and the sidebar's pulse set by value. None of them change when a delta grows the streamed text. - ChatMessageSurface subscribes to the projection and the shell-run record itself. It is their only renderer, so they never reach the shell. - The per-delta reconcile moves into <LiveTurnReconciler/>, which follows every delta and owns no subtree. `useShellLiveTurn` now takes the snapshot rather than the projection, and `deriveModelWait` takes booleans — it only ever asked whether the buffers were empty. This needs no `memo`: the sidebar and composer stop re-rendering because their parent does, not because a comparison blocks them.

…fect event `useEffectEvent` returns a fresh identity every render and must not cross a component boundary or enter a dep array. `reconcilePersistedMessages` already comes from `useStableActions`, whose whole purpose is a fixed identity bound to the latest committed render — the wrapper made the dep array dishonest and re-ran the reconcile for unrelated shell renders.

…oize it Three things the review surfaced, one cause: the selector seam was shaped so that neither the compiler nor the tests could see what the shell reads. - `useAppShellSessionUiReads` is now that list, in one place. The contract test drives the hook itself, so adding a token-rate selection to it fails the test — before, the test asserted against a copy of the list and a real regression in AppShell would have gone green. - Selectors are module-level and take what they vary by as `arg`, so the snapshot is memoized rather than published through a render-phase ref write. React permits that write only for lazy initialization; a discarded concurrent render would otherwise hand its selector to the committed subscription. `arg` also makes the activeId switch explicit, now covered. - `LiveTurnSnapshot.streamingTextComplete` is gone: `streamingMessageId` is set only when the text step completed, so its presence already carried that fact. The freed field is `turnId`, which `deriveTurnActive` needs.

A settled turn must drop its phase but keep its id — `deriveTurnActive` reads the first to retire this renderer's arm and the second to tell a sibling turn apart from it. And the handoff message id must stay absent while the text step is open, since its presence is what says the step closed.

`useSyncExternalStore` reads a snapshot several times for one store state — in the subscription callback, during render, and again in a passive effect — and demands the same value each time. The cache only compared the previous VALUE, so a selector deriving a fresh object handed React a new identity on every call and force-rendered forever; the only thing standing between the app and a freeze was every caller remembering to pass `isEqual`. Key the cache by the state it derived from. Idempotence per store state now belongs to the one adapter that connects arbitrary derivations to the store, `isEqual` drops to what it should have been (carrying a value's identity ACROSS a state that did not change the selection), and each selector runs at most once per store change instead of once per read.

…ribers `selectLiveTurn` was defined word-for-word in both the chat surface and the reconciler, so changing one would silently leave the other behind. Keep it with the rest of the session-UI selectors, where the snapshot selector can build on it too. `deriveLiveTurnSnapshot` also allocated a flattened tool array per call just to ask two yes/no questions of it.

Three of these tests could not fail for the reason they named. The settled-turn snapshot case built `{...armLiveTurn(id), terminal: true}`, a projection the reducer cannot produce: `complete` and `abort` both return `undefined` for a turn with no steps. It also only asserted on fields, while the two #1987 Stop witnesses actually meet one layer up, in `useShellLiveTurn` — which had no behavioural test at all. Build the projection from a real `text_delta` + `complete` and assert on `turnActive` through that adapter, so dropping `turnId` on the way in fails here. The render-boundary case selected through a COPY of the shell's snapshot selector, the very pattern this branch removed elsewhere. Fold its positive assertions (the arm and the first token each cost one render) into the case that drives `useAppShellSessionUiReads` itself, which was asserting only that deltas cost nothing — leaving the snapshot comparator free to be gutted.

Astro-Han force-pushed the perf/desktop-session-ui-subscription branch from 265f207 to 3a8ea86 Compare August 3, 2026 11:58

Astro-Han added 5 commits August 3, 2026 20:01

Astro-Han force-pushed the perf/desktop-session-ui-subscription branch from 3a8ea86 to b2ff08e Compare August 3, 2026 12:02

Astro-Han added 3 commits August 3, 2026 20:19

Astro-Han marked this pull request as ready for review August 3, 2026 12:23

Astro-Han merged commit 2139ed0 into main Aug 3, 2026
11 checks passed

Navigation Menu

Uh oh!

perf(desktop): subscribe to session UI state at the granularity each surface reads - #1998