Uh oh!
There was an error while loading. Please reload this page.
perf(desktop): stop idle-session event-health polling from re-rendering the whole shell - #1988
Merged
Merged
Conversation
`useSessionEventHealthPolling` armed a 5s interval for any active session, but once a session no longer expects an event stream the tick has nothing to observe: `deriveSessionEventStreamStatus` short-circuits to `closed` and `shouldRefreshStaleSessionEventStream` never fires. Every tick still wrote a snapshot carrying a fresh `checkedAt`, which only ever cleared the session UI state controller's reference check and forced a full AppShell render — the only periodic work the app did while idle. Gate the interval (and the visibilitychange listener) on `sessionExpectsEventStream`. The one-off `evaluate` before the gate still records the transition into `closed`, and both inputs to `expected` are already effect deps, so a session that starts running re-arms on its own. Refs #1979
…tate `sessionEventHealthBySession` lived in `AppShellSessionUiState`, whose only purpose is to force an AppShell render on change. Nothing renders it — the sole reader is the polling effect, through `sessionEventHealthBySessionRef` — yet every probe wrote a snapshot with a fresh `checkedAt`, and each SessionEvent did too, so a stream forced one extra full render per event. Move it to a controller-owned ref. `setSessionEventHealthBySession` keeps its updater signature, so all four call sites are untouched; `clearSessionUiState` drops the session from the ref alongside the rendered maps. The `MissingSessionUiMapKey` guard keeps the remaining maps exhaustive. Refs #1979
The gate sat after a one-off `evaluate()`, so settling into idle still wrote one snapshot. That write cannot do anything: `expected` is false on this path, so the status derives to `closed` and no refresh is ever requested, and neither `status` nor `staleSince` has a renderer consumer (the sidebar's `staleSessionIds` comes from `stale-sessions.ts`, an unrelated backend/slug classifier). `markSessionEventStreamClosed` already records the closed stream when the subscription goes away. Moving the gate above `evaluate` makes an idle session cost nothing at all. The test asserted that dead write, which would have failed a smaller correct implementation; it now asserts zero writes. Also cover the failure mode this gate introduces: a session that starts running must re-arm. Neither single-status case caught it — shrinking the dep array to `[activeId]` kept them green. The round-trip case fails on that mutation. Refs #1979
… move The turn-transient comment still listed event-stream health among the maps `clearAppShellSessionUiStateForSession` clears; it now lives in a controller ref and is cleared by `clearSessionUiState`. Point at the ref instead, and drop the interface preamble that described a field no longer declared there — the ref's own comment already carries that rationale. Narrow `omitSessionKey` back to `Record<string, unknown>`; widening it to `object` for the ref accepted arrays and non-record objects, which would return silently unchanged.
Astro-Han
marked this pull request as ready for review
August 3, 2026 10:55
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useSessionEventHealthPollingre-rendered the whole AppShell every 5s for the entire lifetime of any open session, idle or not — the only periodic work the app did while idle, and it changed nothing visible.Four commits, one root cause:
deriveSessionEventStreamStatusshort-circuits toclosedandshouldRefreshStaleSessionEventStreamnever fires. The interval and thevisibilitychangelistener are now gated onsessionExpectsEventStream.sessionEventHealthBySessionlived inAppShellSessionUiState, whose only purpose is to force a render. Nothing renders it — the sole reader is the polling effect viasessionEventHealthBySessionRef. It now lives in a controller-owned ref, so neither the probes nor the per-SessionEvent writes (app-shell-effects.ts:380, one extra full render per streamed event) notify React.setSessionEventHealthBySessionkeeps its updater signature, so all four call sites are untouched;clearSessionUiStatedrops the session from the ref alongside the rendered maps.evaluate(), so settling into idle still wrote one snapshot. That write cannot do anything —expectedis false on this path, and neitherstatusnorstaleSincehas a renderer consumer (the sidebar'sstaleSessionIdscomes fromstale-sessions.ts, an unrelated backend/slug classifier).markSessionEventStreamClosedalready records the closed stream when the subscription goes away, so an idle session now costs nothing at all.Gating alone would have fixed the idle symptom, but only step 2 removes the per-event renders during a stream. Note that "skip unchanged writes" is not a valid alternative to either: the ref is only synced on write, so skipping writes would strand
refreshRequestedAtand turn the 10s refresh cooldown into a refresh every tick.Closes#1979
Verification
npm --workspace apps/desktop run test:dist— 1348 pass, 0 fail.npm --workspace apps/desktop run typecheck— clean (preload, main, renderer, storybook).npm run format/npm run lint— clean.npm --workspace apps/desktop run e2e— 58 passed, includingsession-health-notice.spec.ts.New coverage in
app-shell-effect-stability-contract.test.ts, driven by a fake clock added to the existing fake DOM (observablewindow.setInterval/document.addEventListener, controllableDate.now):activesession arms no interval, registers novisibilitychangelistener, and writes nothing at all — before or after the clock advances;active → running → activere-arms and then disarms both the interval and the listener;runningsession goesstalepast the threshold, refreshes sessions and messages exactly once, and re-probes when the window becomes visible again.Each was verified by mutation, not just by passing: removing the gate, moving the gate back after
evaluate, making health writes notify again, dropping the ref cleanup inclearSessionUiState, and no-oping the interval callback each turn the corresponding test red. The round-trip case specifically covers the one failure mode this gate introduces — a session that starts running but never re-arms; shrinking the dep array to[activeId]leaves every other case green and fails only that one.Not run: no screenshot or Storybook evidence — this PR removes renders and changes no pixels.
Review focus
clearSessionUiStatenow mutates the health ref in addition toreplaceState. That is the one place where the ref's lifecycle could drift from the rendered maps, and it is covered bydrops event-stream health along with the rest of a cleared session.Known pre-existing behavior, not addressed here
If a session sits idle past the 15s threshold and then starts running before a fresh SessionEvent or
sessions:changedobservation reaches the renderer, the firstevaluatereads stale observation timestamps, derivesstale, and fires one extra refresh. The inputs to that first evaluate are bit-identical before and after this PR —evaluateSessionEventStreamSnapshotnever advancessubscribedAt/lastEventAt/lastChangedAt, so the idle ticks this PR removes never affected it. The refresh is idempotent and self-healing. Out of scope; worth a separate issue if it ever shows up in practice.Related