Skip to content

perf(desktop): AppShell subscribes to all session UI state at one granularity, so every stream token re-renders the whole shell #1985

Description

@Astro-Han

Summary

createAppShellSessionUiStateController is an external store with per-session, per-slice maps, but useAppShellSessionUiState exposes it through a single forceRender(). Every write to any slice — including one per streaming token — re-renders all of AppShellContent, including subtrees that cannot have changed.

Split out of #1979, which fixed one instance (idle event-health polling). This issue is the shared root cause: the store's granularity and the subscription's granularity don't match.

Line numbers below are against 75e085354, i.e. after #1988 merged.

Evidence

  • app-shell-session-ui-state.ts:112-118,161-172replaceState calls a single onChange, wired to one useReducer bump. No selector, no per-slice subscription.
  • app-shell-session-events.ts:65-74setLiveTurnBySession is written per streaming delta. Each write is a fresh map → passes the reference check → forceRender().
  • app-shell-effects.ts:478-484 — shell-run updates arrive on their own high-rate IPC channel and write the second rendered map at tool-output rate.
  • app-shell-effects.ts:384-392 still writes event-health on every session event, but perf(desktop): stop idle-session event-health polling from re-rendering the whole shell #1988 moved that map into a controller-owned ref (app-shell-session-ui-state.ts:106-110,144), so it no longer notifies React. That is why this issue now has two token-rate rendered slices rather than three — worth stating explicitly, because it is the reason a measurement taken before perf(desktop): stop idle-session event-health polling from re-rendering the whole shell #1988 does not reproduce.
  • use-app-shell-session-workspace.ts:70-82 is called from AppShellContent (app-shell.tsx:277), whose return renders the entire shell (2663-line file).
  • app-shell.tsx:312-321 destructures the whole store at the top of the component; :549 reads the full active projection; :606-625 feeds it to useShellLiveTurn.
  • app-shell.tsx:2135SessionListPanel takes ~25 props including inline lambdas (onWidthChange, onSelectSession), so it re-renders on every shell render and would keep doing so even if wrapped in memo.

Net effect during a stream: the session list, composer, and any open non-chat surface re-render at token rate, though none of them read liveTurnBySession.

What the first framing got wrong

Three corrections, each of which changes the fix:

  1. subscribe + useSyncExternalStore + selector is necessary but not sufficient. The component holding the selector is AppShellContent (app-shell.tsx:312-321,549,606-625). As long as it selects the full projection, it re-renders on every delta and the whole subtree goes with it — zero renders saved. The subscription fix only pays off once the reader boundary moves.

    The cheap form of "moving the boundary" is not to relocate every read: the top level can keep subscribing, as long as it selects a low-entropy semantic snapshot (phase, is-streaming, settled messageId, has-in-flight-tool). Those values are invariant across consecutive text deltas. Only the chat transcript genuinely needs the full projection, and only that read has to sink.

  2. JSX reads are not the only consumers. app-shell.tsx:1787-1790 is an effect whose dep array contains activeLiveTurn, so it runs per token; the terminal-settle fallback below it, the health polling, and homeSurfaceActive (app-shell.tsx:2034) also derive from it. Any sinking plan must give these a home — an effect-only return null subscriber can run per delta without owning a subtree.

  3. Second shallow-compare killer. use-shell-live-turn.ts:57-62 builds streamingSessionIds as a fresh Set on every liveTurnBySession change and passes it to SessionListPanel. It is not a trigger today — it is a by-product of the parent render — but it defeats any future shallow comparison at that boundary. Note its membership is not turn-invariant: the predicate is text?.text && !text.complete, so a session drops out between a settled step and the next text step. It needs a value-equality-cached selector, not just a useMemo.

Also worth downgrading: session rows are already memoized (packages/ui/src/session-history-list.tsx:332,622, locked by ui-render-memo-boundary-contract.test.ts:50), so the waste does not scale into per-row internals. It is AppShellContent itself, the panel/list traversal, and the composer.

Non-fixes

  • rAF / microtask coalescing in the store. React 19 + createRoot already batches the multiple forceRenders inside one IPC listener, so microtask coalescing is redundant and cannot merge across separate IPC tasks. rAF caps whole-tree renders at min(R, F) — near-zero benefit at typical token rates below 60 Hz, at the cost of a frame of latency and background-window throttling. It also does not meet the acceptance bar. Do not put a scheduler in the external-store notify path: it would delay the terminal handoff.
  • Blanket memo. The inline-lambda props defeat it, and fixing that means stabilizing ~25 props per panel. Once AppShellContent stops re-rendering, the children are never re-invoked and no memo is needed.

Fix direction

Extend the existing seam. The controller is already an external store; it only lacks per-subscriber notification.

  1. Add a synchronous subscribe(listener), notified immediately after the state swap.
  2. Drop the aggregate useReducer subscription. The top level subscribes to the low-frequency maps' raw references plus the live-turn semantic snapshot — so consecutive deltas change nothing it reads.
  3. Sink the full liveTurnBySession[activeId] and the active shell-run record into the ChatMessageSurface desktop wrapper.
  4. Move the per-delta reconcile / terminal-handoff effects into an effect-only subscriber that renders nothing.
  5. Sidebar gets a value-equality-cached membership selector; the composer consumes only the semantic snapshot, so it re-renders at turn boundaries, not at token rate.

Correctness constraints

  • getSnapshot must not build a fresh Set/array/object per call. React compares with Object.is and requires a stable snapshot when nothing changed, or it throws "getSnapshot should be cached" / loops. Return the raw per-session record and do the Object.values inside the component.
  • activeId goes into the selector directly — the first render after a switch must read the new session's snapshot, not copy it in via an Effect.
  • Preserve the synchronous activeIdRef write order (use-app-shell-session-workspace.ts:23-35) and the layout-effect cleanup of the previous session's subscription (app-shell-effects.ts:414).
  • useSettledSessionTransientReconcile (app-shell-effects.ts:604) still has to clear settled background projections; sinking the read must not delete it.

Severity

P1. The waste scales with token rate and lands on the app's hottest path — the whole shell re-renders for every delta of a stream that only the chat surface reads. Perceived responsiveness during streaming is a primary quality bar, so this is not deferrable polish.

Acceptance

  • A streaming session re-renders the chat surface but not SessionListPanel / Composer.
  • Render-count contract test, using the existing client-commit harness (createRoot + act + fake DOM) at apps/desktop/src/main/__tests__/ui-render-memo-boundary-contract.test.ts:255 — no new test framework, and not a controller-only notification count, which would not pin the component boundary. Cover: a second text_delta bumps the chat render count and nothing else; activeId switch; terminal complete.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions