perf(dashboard): stop the rail collapse thrashing the transcript - #1074
Conversation
Collapsing the left nav rail animates `grid-template-columns` on the shell
grid (App.tsx:1482) for 150ms. That is a LAYOUT property, so the content
column's width changes on every frame of the animation and every mounted
transcript row rewraps -- each producing a ResizeObserver entry with a new
offsetHeight.
Measured in isolation (7 mounted markdown-weight rows, 8 collapse+expand
cycles, headless Chromium): animating the track vs not multiplied the
virtualizer's ResizeObserver fires and its forced offsetHeight reads by
13-18x, while the FINAL cached heights came out identical -- every extra
measurement is discarded.
The damaging part is not the reads, though. It is the read/write interleave:
each genuine height change calls pinAuto(), a scrollTop WRITE, in between
those forced reads. One write per animation frame, ~9 per toggle.
Note what was ALREADY protected and is therefore not the problem:
HEIGHT_SYNC_DEBOUNCE_MS (120ms) coalesces the height-sync re-render inside
the 150ms window, so there was no per-frame React render storm. An earlier
reading of this bug over-claimed that; the scroll writes are the real cost.
Fix: a settle window, published from useRailWidth -- the module that already
owns "the rail's collapse is a 150ms grid-template transition" as a fact its
consumers need. `setRailWidth` is already the single point App notifies on a
track change, so the window is armed there and cannot be forgotten by a
future edit to how the rail collapses.
While the window is open the virtualizer's ResizeObserver:
- KEEPS its height-cache updates (layout is already dirty, so reading is
cheap, and this leaves no stale heights), and
- HOLDS BACK the pinAuto() scrollTop write, the height sync, and the
window recompute.
Exactly one sync -- plus one re-pin for a user who was following -- runs when
the window closes.
The actively-streaming row is deliberately EXEMPT. Stalling ITS growth for
the length of the animation re-creates the spacer lurch that
`streamingIndex`'s immediate-sync path exists to prevent (PR #824, #966).
Collapsing the rail mid-turn is rare; a visible lurch is not an acceptable
trade for it.
The animation is unchanged. Framer-motion never animated the rail's width on
desktop -- `motion.nav`'s desktop props are `animate={{ x: 0 }}` with
`transition={undefined}` and no `layout` prop; the collapse motion is
entirely the CSS grid transition, which this does not touch.
Tests: 9 new in website/src/test/useVirtualChat.railCollapse.test.tsx.
Four cover the window itself: not settling at rest, arming on a genuine
track change, NOT arming on an unchanged width (arming on a no-op write
would hold the window open under any churn and silently disable height
syncing), and closing after it elapses.
Five cover the virtualizer. The discriminating one asserts scrollTop writes:
zero during the animation, exactly one re-pin after -- against the pre-fix
code it is nine, one per frame (verified by checking out the base version of
the file with `git show`). The others pin that heights are NOT left stale
(the post-window sync reflects the final width), that nothing is suppressed
once the window has closed, that the streaming row keeps its immediate path,
and that the pending settle timer is cleared on unmount.
Gates: tsc clean, eslint clean, 6,572 frontend tests pass across 544 files.
No Python touched.
Scope, honestly: the transition dates to 2026-07-20 (#94), so this reduces a
long-standing cost rather than reverting a recent regression. It also removes
the JS-side amplification, not the browser's own reflow -- the engine still
re-lays-out the content column each frame because a layout property is
animating. The structural fix for that is to snap the grid track and animate
only the rail (compositable), which is a larger change deliberately not
attempted here.
github-actions
Bot
added
the
readiness: checking
UX Review (Fable 5) — ✅ PASSAdvisory UX-level review of UX-Verdict: PASS Pure perf change with no new UI surface; the felt result is a smoother rail collapse, and the streaming-row exemption protects the one visible risk. Watch
[UX-REVIEWED] bcd5b64 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Arbiter — ✅ human override acceptedHuman judgment by @CrysisDeu overrides the Arbiter finding for
A new push requires a new judgment. The |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound thrash removal, but the PR's own root cause — animating a layout property — stays, and the user-facing lag fix is unverified. Watch
Suggestions
[DESIGN-REVIEWED] bcd5b64 |
Opus 5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
/ai-review override arbiter bcd5b64: Rebutting rather than complying, because each premise of the escalation is checkably wrong and the suggested fix breaks a different gate. Evidence: 1. "this public repo" — the repository is private. 2. "a one-way door created by this diff" — it is a 58-commit-deep repository standard.
3. The proposed "smallest possible fix" would break the UX Review gate. 4. The suggested alternative has no working form here. What I accept from the escalation: committed binaries are permanent, and that is a real cost worth keeping small. I kept it small before pushing — the decisive artifact is a 330 KB HUD crop, re-encoded down from a 6.2 MB capture, with the two full-frame stills folded into The four non-escalated items are all fair and I am not contesting them; three are worth doing as follow-ups rather than widening this PR:
On the UX reviewer's Watch item and the manual check: both are called out as open in the PR body's own "Manual verification" section — the isolated dev gateway does not boot on this machine, so a human still needs to confirm the collapse on a real long transcript pinned to bottom. I did not claim otherwise. |
Human judgment recorded@CrysisDeu marked the arbiter AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
github-actions
Bot
added
readiness: passed
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Approved: independently re-reviewed the full diff against the GPT + Opus/AUTOSDE contracts (reachable correctness/security, fail-closed/permission boundaries, cross-OS, and deleted-in-use i18n keys per the #976 class). No reachable Critical/High found; all CI checks green.
github-actions
Bot
removed
the
readiness: passed
…odotdev#1074) Collapsing the left nav rail animates `grid-template-columns` on the shell grid (App.tsx:1482) for 150ms. That is a LAYOUT property, so the content column's width changes on every frame of the animation and every mounted transcript row rewraps -- each producing a ResizeObserver entry with a new offsetHeight. Measured in isolation (7 mounted markdown-weight rows, 8 collapse+expand cycles, headless Chromium): animating the track vs not multiplied the virtualizer's ResizeObserver fires and its forced offsetHeight reads by 13-18x, while the FINAL cached heights came out identical -- every extra measurement is discarded. The damaging part is not the reads, though. It is the read/write interleave: each genuine height change calls pinAuto(), a scrollTop WRITE, in between those forced reads. One write per animation frame, ~9 per toggle. Note what was ALREADY protected and is therefore not the problem: HEIGHT_SYNC_DEBOUNCE_MS (120ms) coalesces the height-sync re-render inside the 150ms window, so there was no per-frame React render storm. An earlier reading of this bug over-claimed that; the scroll writes are the real cost. Fix: a settle window, published from useRailWidth -- the module that already owns "the rail's collapse is a 150ms grid-template transition" as a fact its consumers need. `setRailWidth` is already the single point App notifies on a track change, so the window is armed there and cannot be forgotten by a future edit to how the rail collapses. While the window is open the virtualizer's ResizeObserver: - KEEPS its height-cache updates (layout is already dirty, so reading is cheap, and this leaves no stale heights), and - HOLDS BACK the pinAuto() scrollTop write, the height sync, and the window recompute. Exactly one sync -- plus one re-pin for a user who was following -- runs when the window closes. The actively-streaming row is deliberately EXEMPT. Stalling ITS growth for the length of the animation re-creates the spacer lurch that `streamingIndex`'s immediate-sync path exists to prevent (PR kirodotdev#824, kirodotdev#966). Collapsing the rail mid-turn is rare; a visible lurch is not an acceptable trade for it. The animation is unchanged. Framer-motion never animated the rail's width on desktop -- `motion.nav`'s desktop props are `animate={{ x: 0 }}` with `transition={undefined}` and no `layout` prop; the collapse motion is entirely the CSS grid transition, which this does not touch. Tests: 9 new in website/src/test/useVirtualChat.railCollapse.test.tsx. Four cover the window itself: not settling at rest, arming on a genuine track change, NOT arming on an unchanged width (arming on a no-op write would hold the window open under any churn and silently disable height syncing), and closing after it elapses. Five cover the virtualizer. The discriminating one asserts scrollTop writes: zero during the animation, exactly one re-pin after -- against the pre-fix code it is nine, one per frame (verified by checking out the base version of the file with `git show`). The others pin that heights are NOT left stale (the post-window sync reflects the final width), that nothing is suppressed once the window has closed, that the streaming row keeps its immediate path, and that the pending settle timer is cleared on unmount. Gates: tsc clean, eslint clean, 6,572 frontend tests pass across 544 files. No Python touched. Scope, honestly: the transition dates to 2026-07-20 (kirodotdev#94), so this reduces a long-standing cost rather than reverting a recent regression. It also removes the JS-side amplification, not the browser's own reflow -- the engine still re-lays-out the content column each frame because a layout property is animating. The structural fix for that is to snap the grid track and animate only the rail (compositable), which is a larger change deliberately not attempted here. Co-authored-by: Zezhen Xu <zezhexu@amazon.com>
2 participants
Problem
Collapsing the left nav rail is laggy on the chat route, and the lag scales with the transcript.
The collapse animates
grid-template-columnson the shell grid (website/src/App.tsx:1482) for 150ms. That is a layout property, so the content column's width changes on every frame and every mounted transcript row rewraps — each producing aResizeObserverentry with a newoffsetHeight.Profiled in isolation (7 mounted markdown-weight rows, 8 collapse+expand cycles, headless Chromium): animating the track vs not multiplied the virtualizer's RO fires and its forced
offsetHeightreads by 13–18×, while the final cached heights came out identical — every extra measurement is discarded.Why it matters
The reads are not the damaging part. The interleave is: each genuine height change calls
pinAuto(), ascrollTopwrite, in between those forced reads. Read → write → read, one write per animation frame, ~9 per toggle. That is textbook layout thrash, and it lands on the largest surface in the app.What was already protected, and is therefore not the problem:
HEIGHT_SYNC_DEBOUNCE_MS(120ms) coalesces the height-sync re-render inside the 150ms window, so there was never a per-frame React render storm. An earlier reading of this bug over-claimed that; the discrimination check on the tests is what corrected it. The scroll writes are the real cost.Fix (symptom → root cause → change)
Symptom: a laggy collapse that gets worse with transcript length. Root cause: ~9 interleaved
scrollTopwrites per toggle, driven by a ResizeObserver reacting to transitional widths that are all superseded at the final width.A settle window, published from
useRailWidth— the module that already owns "the rail's collapse is a 150ms grid-template transition" as a fact its consumers need (see its existing note on publishing the stepped track value rather than measuring the DOM).setRailWidthis already the single pointAppnotifies on a track change, so the window is armed there and cannot be forgotten by a future edit to how the rail collapses.While the window is open, the virtualizer's ResizeObserver:
pinAuto()write, the height sync, and the window recompute.Exactly one sync — plus one re-pin for a user who was following — runs when the window closes.
The actively-streaming row is exempt. Stalling its growth for the length of the animation re-creates the spacer lurch that
streamingIndex's immediate-sync path exists to prevent (#824, #966). Collapsing the rail mid-turn is rare; a visible lurch is not an acceptable trade for it.The animation is untouched
Worth stating explicitly because it is easy to assume otherwise: framer-motion never animated the rail's width on desktop.
motion.nav's desktop props areanimate={{ x: 0 }}(a no-op —xis already 0) withtransition={undefined}and nolayoutprop; its job on that element is the mobile drawer slide. The collapse motion is entirely the CSS grid transition, which this PR does not modify.Recording
Same 150ms animation in both modes; watch the counter.
scrollTop writes this toggleclimbs once per frame in BEFORE, and stays at the single post-window re-pin in AFTER — whileResizeObserver firesis unchanged, which is the point (the cache stays warm, so nothing goes stale).Full-frame stills
BEFORE — mid-collapse, writes accumulating per frame:
AFTER — one write, same RO fire count:
What this recording is and is not. It is a harness that reproduces the exact structure — the real
grid-template-columns 150ms cubic-bezier(0.2,0,0,1)transition, 7 mounted variable-height rows (whatuseVirtualChatkeeps mounted atoverscan: 6+ tail), and a per-row ResizeObserver that readsoffsetHeightand writesscrollTop. It is not the dashboard: the isolated dev gateway does not boot on this machine (it dies after the agent-home warning and never binds its port, which also blocked an unrelated profiling task earlier), so I could not record the real app. Treat it as evidence of the mechanism and the delta, not of an end-user frame rate.Tests
9 new in
website/src/test/useVirtualChat.railCollapse.test.tsx.Four cover the window itself: not settling at rest; arming on a genuine track change; not arming on an unchanged width (arming on a no-op write would hold the window open under any churn and silently disable height syncing); closing after it elapses.
Five cover the virtualizer. The discriminating one asserts
scrollTopwrites — zero during the animation, exactly one re-pin after. Against the pre-fix code it is nine, one per frame (expected 9 to be +0), verified by checking out the base version of the file withgit show. The rest pin that heights are not left stale (the post-window sync reflects the final width), that nothing is suppressed once the window has closed, that the streaming row keeps its immediate path, and that the pending settle timer is cleared on unmount — deliberately, since the timer callssyncHeightsNow/pinAuto/recomputeWindow, all of which touch state and the scroller.My first version of these tests passed against the unfixed code. That was not a test bug — it is what exposed that the height-sync storm was already debounced, and made me retarget the assertion at the writes.
All 54 existing virtualizer tests still pass, including the
spacerLurchandpostStreamLurchguards. Gates:tsc -bclean, eslint clean, 6,572 tests across 544 files.Manual verification
Not done, and I want to be straight about why: the isolated dev gateway will not boot here, so I could not click the real UI. The behavioural claims are all about observer/write counts across a frame sequence, which the tests assert precisely and a human cannot see — the intended user-visible outcome is no change at all to the animation. The one thing a person should still confirm on a real long transcript is that the collapse feels better and that scroll position does not jump; that is the check I could not perform.
Scope, honestly
SmoothResize, Jun 17; the streaming-settle grace, Jul 31) also predate the window. So something else may still be raising per-row reflow cost — this fix helps regardless of what that turns out to be.