fix(chat): drive mobile panel slides on the compositor - #6868
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Compositor migration with derived-from-one-constant dual paths, documented fallbacks, and mutation-verified guards — inherent complexity, not accidental; no one-way doors. Suggestions
[DESIGN-REVIEWED] ff86f46 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of I have everything I need. Let me verify one last detail — that the base notification sheet entry timing is what the diff shows being replaced (checking the tailwind hunk again confirms: entry was First-Principles-Verdict: CONCERNS Two undeclared riders in a fix: the notification sheet's entry gets 30% slower on a new curve, and Escape-to-close appears — neither named by the description. What this change shipsIntent: stop mobile panel slides from dropping frames while sessions stream, by moving them off the main thread — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] ff86f46 |
UX Review (Fable 5) — ✅ PASSUX-level review of All evidence reviewed: full diff, PR intent, and sibling dismissal patterns. No screenshots exist (justified — animation threading). No user-facing strings changed; at-rest layouts are unchanged except the mobile right panel, which already covered the full window. The trade-offs (row animations snap inside the mobile drawer, transcript flushes deferred ≤1s during slides with data never dropped) are deliberate, bounded, and invisible-or-mild. The PR also adds an Escape path to the nav drawer. One parity gap: the new mobile right-panel overlay is a layered surface this PR created, and it didn't receive the same Escape dismissal the nav drawer gained in the same diff. UX-Verdict: PASS Compositor-driven slides remove real jank with no comprehension, copy, or control regressions; the one trade-off (instant row reorders in the mobile drawer) is imperceptible-by-design. Suggestions
[UX-REVIEWED] ff86f46 |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @buluoray overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates fail falsification. CANDIDATE 1 claims the new
CANDIDATE 2 claims the shared No independent finding cleared the same bar in Step 2. No findings. [OPUS-REVIEWED] ff86f46 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
9c4ce31 to
4ff3d85
Compare
Disposition — round 1 (head
|
4ff3d85 to
0299a96
Compare
Disposition — round 2 (head
|
0299a96 to
698ac36
Compare
Disposition — round 3 (head
|
1e4c9a7 to
f2586ca
Compare
|
/ai-review override gpt f2586ca: The scrim is aria-hidden and decorative — dismissal is keyboard-reachable via the nav toggle button (App.tsx:2570) and the Escape handler this PR adds (2336) — the repo's own Modal.tsx:193 scrim is the same pattern but less accessible, the blocking accessible-interactive-elements gate passes, and an earlier round's remedy (add Clickable) would have made a full-viewport role=button tab stop. Re-posted because a rebase onto current main voided the SHA-scoped override. The argument is unchanged and was verified against source in earlier rounds; the rebase carries no code change of its own. |
Human judgment recorded@buluoray marked the gpt 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. |
f2586ca to
a7f2f6d
Compare
|
/ai-review override gpt a7f2f6d: The scrim is aria-hidden and decorative — dismissal is keyboard-reachable via the nav toggle button (App.tsx:2570) and the Escape handler this PR adds (2336) — the repo's own Modal.tsx:193 scrim is the same pattern but less accessible, the blocking accessible-interactive-elements gate passes, and an earlier round's remedy (add Clickable) would have made a full-viewport role=button tab stop. Re-posted because a rebase onto current main voided the SHA-scoped override. The argument is unchanged and was verified against source in earlier rounds; the rebase carries no code change of its own. |
Human judgment recorded@buluoray marked the gpt 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. |
Mobile panel open/close stuttered whenever the main thread was busy (streaming sessions, long message lists), because every panel animated on the main thread: the sessions drawer via a framer tween, the nav drawer likewise, the right side panel by animating width (a layout property), and the notification sheet by animating margin-right (also layout). Fixes, layered: - Sessions/nav drawers and the right panel settle via WAAPI (Element.animate) on transform+opacity, so the slide runs on the compositor regardless of main-thread load. Arrival writes the final transform into the element's own inline style BEFORE cancelling the fill-forwards animation, so non-framer-bound panels do not fall back to their mounted (closed) position. - Session rows render without framer layout projection inside the drawer (staticRows): projection requires framer to own the ancestor transform, and re-measurement under a WAAPI-driven ancestor mis-attributes the panel's motion to the rows (they fly in from the right). NavItem projection is likewise gated off on mobile. Desktop keeps projection. - The right side panel becomes a slide-in overlay on mobile instead of a width reveal (width cannot run on the compositor); live-app iframe keep-alive is preserved. - Streaming flush pipelines (chat chunks, subagent chunks, slot activity) pause while a drawer is moving (streamHold, 1s hard cap, data delayed never dropped) so projection has no re-measure trigger mid-slide. - The notification sheet's keyframes animate transform instead of margin-right; percentage translateX resolves against the element's own width, so one keyframe pair replaces the desktop/mobile variant split. A Chromium pixel probe disproved the old comment's claim that a transformed ancestor breaks descendant backdrop-filter. - Sidebar memo repair: two inline callbacks and a dependency on the whole sessionTabs object defeated memo(ChatSidebar), re-rendering the sidebar every streamed frame. Guard tests pin each pairing; every guard was mutation-verified.
a7f2f6d to
ff86f46
Compare
|
/ai-review override gpt ff86f46: The scrim is aria-hidden and decorative — dismissal is keyboard-reachable via the nav toggle button (App.tsx:2570) and the Escape handler this PR adds (2336) — the repo's own Modal.tsx:193 scrim is the same pattern but less accessible, the blocking accessible-interactive-elements gate passes, and an earlier round's remedy (add Clickable) would have made a full-viewport role=button tab stop. Re-posted for |
Disposition — First Principles Review (CONCERNS on
|
Human judgment recorded@buluoray marked the gpt 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. |
Disposition — First Principles Review (CONCERNS on
|
Problem / Motivation
On mobile, opening or closing any of the three panels — the sessions drawer, the left nav drawer, the right side panel — dropped frames whenever the main thread was busy: long message lists, multiple sessions streaming. Every panel animated on the main thread, each in its own way:
transformwrites);width: 0 → auto— a layout property, so every frame reflowed the panel and the chat column it squeezes;margin-right— also layout, same jank class.A main-thread animation cannot be protected by throttling work: streamed chunks, tool events, subagent status, and panel mount cost are an open-ended list. The only structural fix is to move the slide off the main thread.
What changed
1. Compositor-driven settle (
useDrawerSwipe.ts).animateDrawernow drives the panel'stransformand the scrim'sopacityvia WAAPI (Element.animate) when targets are registered (registerDrawerTargets), so the settle runs on the compositor regardless of main-thread load. Dragging stays on the main thread (the finger is a main-thread event source). Fallbacks (no registered element,prefers-reduced-motion, embed frames) keep the original framer tween.Arrival ordering matters: on finish/cancel/gesture-takeover,
publishArrivalwrites the terminal value into the element's own inline style first, then updates the MotionValue, then cancels thefill: forwardsanimation. For framer-bound panels this is harmless redundancy; for plain elements (nav drawer, right panel) it is the only correct order — cancelling first snaps the element back to its mounted (closed) inline style, which shipped as a visible "opens, then vanishes, then flashes back" bug during development and is pinned by tests.2. Rows opt out of layout projection inside the drawer (
ChatSidebar.tsx,App.tsx). Framer's layout projection requires framer to own the ancestor transform. Under a WAAPI-driven ancestor, any mid-slide re-render re-measures rows against a moved viewport box and mis-attributes the panel's motion to the rows — the corrective transform accumulates (probe: 12px → 4,170px) and rows visibly "fly in from the right". The drawer mount composesrowAnimEnabled={rowAnimEnabled && !staticRows}intoSessionRow's existing animation gate (post-#6703), andNavItem'slayout="position"is gated off on mobile. Desktop keeps projection and its row-reorder animations.Interaction with #6703: row memoization reduces how often rows re-render mid-slide, but any single re-render (drawer mount, a displaced row) still re-measures — reduction is not elimination, so both changes are needed. The probe also showed the "half-fix" (dropping
layoutbut keepinglayoutId) still flies; the guard test pins both halves.3. Right side panel becomes a slide-in overlay on mobile (
ChatPage.tsx).widthcannot run on the compositor, and the old reveal reflowed the squeezed chat column every frame. Mobile now mirrors the sessions drawer (fixed overlay,transformslide); desktop and embed keep the deliberate width reveal. The live-app iframe keep-alive path (closed panel stays mounted, hidden) is preserved and pinned by tests.4. Streaming flush pipelines pause while a drawer moves (
useWebSocket.ts+lib/streamHold.ts). The three per-frame rAF flush lanes (chat chunks, subagent chunks, slot activity) defer their flush while a slide is in flight (drag extends the hold per sample; a 1s hard cap guarantees streaming can never be starved). Data is delayed, never dropped — the buffers already existed. This removes the projection re-measure trigger and keeps the transcript from competing for GPU during the slide.5. Notification sheet keyframes animate
transform(tailwind.config.js,App.tsx). PercentagetranslateXresolves against the element's own width, so one keyframe pair (calc(100% + 20px)) replaces the desktop-px/mobile-percent variant split; the-fullvariants and theisMobileanimation branch are gone. The old margin choice was justified by a comment claiming a transformed ancestor becomes a backdrop root and breaks descendants'backdrop-filter; a Chromium pixel probe (card blur measured mid-animation under a transformed ancestor) disproved that —transformis not in the backdrop-root trigger list. At rest neither animation fills, so the element carries no transform either way.Its entry timing is retuned with them, and that is a separable choice:
nc-slide-ingoes.32s cubic-bezier(.16,1,.3,1)→.42s cubic-bezier(.32,.72,0,1), putting the sheet on the same entry curve as the four panels rather than leaving it on its own. That curve is not introduced here —mainalready uses[0.32, 0.72, 0, 1]in five places, includingOverlayDrawer.tsx:45where it is the namedEASEconstant andChatPage.tsx:7174as a literalcubic-bezier(.32,.72,0,1)— so this converges the sheet onto the established overlay curve, andanimateDrawer.curve.test.tspins the two in step from then on. The threading fix (the keyframe-property change) does not require it; it is here because shipping one surface on a different entry curve from the panels it sits beside is the kind of drift that never gets cleaned up later. Revertable on its own by restoring the two literals intailwind.config.jsand dropping one assertion.6. Sidebar memo repair (
ChatPage.tsx). Two inline callbacks (onSelectSlotin both branches) andopenSlotInNewTabdepending on the wholesessionTabsobject defeatedmemo(ChatSidebar)at the shell boundary — the sidebar re-rendered ~3× per streamed frame. Complements #6703, which memoizes inside the sidebar; this makes the outer boundary hold (measured 41 → 1 renders across 20 streamed frames in the guard test).7. Escape closes the mobile nav drawer (
App.tsx). The nav drawer's scrim isaria-hiddenand decorative — it is a paint surface, not a control — so this PR adds a keydown handler rather than making a full-viewport element focusable. Dismissal is therefore keyboard-reachable two ways: the nav toggle<button>and Escape. Small enough to have ridden along unmentioned in the first draft of this description; called out here because it is also the load-bearing half of the scrim's accessibility argument.Verification
tsc -bclean; 0 new lint errors.rowAnimEnabledcomposition); desktop unchanged.Known limitations
test:electronfails locally on this host (nowebsite/electrondev deps installed); the diff touches no electron files, so theelectron-testCI job sees identical bytes to main.Why no screenshot: The change is animation threading (main-thread tween → compositor WAAPI) and timing curves — frame pacing cannot be shown in a still image, and the panels' at-rest states are unchanged except that the mobile right side panel now overlays the chat column instead of squeezing it (verified on-device by the author across all four panels, streaming and idle).