Skip to content

fix(chat): drive mobile panel slides on the compositor - #6868

Merged
buluoray merged 1 commit into
mainfrom
fix/mobile-panels-compositor
Aug 30, 2026
Merged

fix(chat): drive mobile panel slides on the compositor#6868
buluoray merged 1 commit into
mainfrom
fix/mobile-panels-compositor

Conversation

@buluoray

@buluoray buluoray commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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:

  • the sessions drawer settled via a framer tween (per-frame main-thread transform writes);
  • the nav drawer likewise, plus an independently-tweened scrim;
  • the right side panel animated width: 0 → auto — a layout property, so every frame reflowed the panel and the chat column it squeezes;
  • the notification sheet's keyframes animated 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). animateDrawer now drives the panel's transform and the scrim's opacity via 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, publishArrival writes the terminal value into the element's own inline style first, then updates the MotionValue, then cancels the fill: forwards animation. 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 composes rowAnimEnabled={rowAnimEnabled && !staticRows} into SessionRow's existing animation gate (post-#6703), and NavItem's layout="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 layout but keeping layoutId) still flies; the guard test pins both halves.

3. Right side panel becomes a slide-in overlay on mobile (ChatPage.tsx). width cannot run on the compositor, and the old reveal reflowed the squeezed chat column every frame. Mobile now mirrors the sessions drawer (fixed overlay, transform slide); 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). Percentage translateX resolves against the element's own width, so one keyframe pair (calc(100% + 20px)) replaces the desktop-px/mobile-percent variant split; the -full variants and the isMobile animation 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 — transform is 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-in goes .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 — main already uses [0.32, 0.72, 0, 1] in five places, including OverlayDrawer.tsx:45 where it is the named EASE constant and ChatPage.tsx:7174 as a literal cubic-bezier(.32,.72,0,1) — so this converges the sheet onto the established overlay curve, and animateDrawer.curve.test.ts pins 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 in tailwind.config.js and dropping one assertion.

6. Sidebar memo repair (ChatPage.tsx). Two inline callbacks (onSelectSlot in both branches) and openSlotInNewTab depending on the whole sessionTabs object defeated memo(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 is aria-hidden and 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

  • Full frontend suite green: 1648 files / 26,027 tests; tsc -b clean; 0 new lint errors.
  • 7 new/extended test files; every guard was mutation-verified (~20 mutations across the session: easing/curve swaps, jump-vs-cancel order, inline-style arrival writes, staticRows half-fixes, forgotten pairings, hold-scheduling drops — each turns its specific test red).
  • The one deliberate tradeoff is documented in code: inside the mobile drawer, row reorder/pin animations become instant and the flat↔tree shared-element transition is off (rowAnimEnabled composition); desktop unchanged.
  • Verified on-device (iPhone) across all four panels: smooth open/close while multiple sessions stream; no row fly-in; no arrival bounce; live-app iframe content survives right-panel close/reopen.

Known limitations

  • The Chromium probe covers Blink; if Safari treats a transformed ancestor as a backdrop root (notification sheet, cards' frosted material), the blur would drop only during the 320ms slide and restore at rest. On-device spot check showed no visible artifact.
  • test:electron fails locally on this host (no website/electron dev deps installed); the diff touches no electron files, so the electron-test CI 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).

@buluoray
buluoray requested a review from a team August 29, 2026 22:58
@buluoray
buluoray requested a review from a team as a code owner August 29, 2026 22:58
@buluoray
buluoray requested a review from smeyffret August 29, 2026 22:58
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of ff86f4656ae09159205f015ec6b1aa251327dd3d — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • releaseStreamingFlushes() zeroes the single global deadline, so when two panels animate concurrently (e.g. route change closes the nav drawer while the right overlay is mid-slide), the first finisher un-holds the second's remaining frames — make release scoped (only clear if the caller's deadline is still the max, or drop release and let deadlines lapse; the deadline design already makes that safe).
  • registerDrawerTargets' no-projection-descendants precondition is enforced only by comment plus per-consumer guard tests; a cheap dev-mode assert (scan the registered panel for data-projection-id / framer layout attrs) would catch the fly-in bug at the third consumer instead of on-device.

[DESIGN-REVIEWED] ff86f46

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ff86f4656ae09159205f015ec6b1aa251327dd3d — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 .32s cubic-bezier(.16,1,.3,1), now .42s cubic-bezier(.32,.72,0,1); exit unchanged). The review follows.

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 ships

Intent: stop mobile panel slides from dropping frames while sessions stream, by moving them off the main thread — a FIX.

  1. Sessions drawer settles on the compositor (WAAPI) — justified; the fix.
  2. Nav drawer + scrim slide on the compositor, framer removed from the panel — justified; the fix.
  3. Mobile right panel overlays the chat instead of squeezing it — justified (width cannot composite; declared).
  4. Notification sheet animates transform, one keyframe pair for both widths — justified, declared.
  5. Drawer open/close feel changes: 320ms spring → asymmetric 420ms/240ms beziers — derived (WAAPI cannot take a spring), device-tested.
  6. Notification sheet entry slows .32s→.42s on a different curve — undeclared, rides along.
  7. Transcript pauses (≤1s cap) during any slide or drag — declared, justified (drag is main-thread; frosted scrim re-samples per repaint).
  8. Drawer rows and mobile nav rows lose reorder/pin/tree-morph glide — declared tradeoff, derived (projection under a WAAPI ancestor, 4,170px probe).
  9. Escape now closes the mobile nav drawer; scrim is aria-hidden — undeclared, rides along (a11y-derived, keyboard dismissal for a modal surface).
  10. Sidebar memo repair, three stabilized callbacks — declared, measured (41→1 renders).

Watch

  • Item 6 is separable by reading the diff: the keyframe-property change alone is the threading fix; the entry duration/curve retune (tailwind.config.js, 'nc-slide-in': .32s → .42s) is a motion-language unification the description's item 5 never mentions. Declare it or defer it.
  • Item 9: description says nothing about Escape or the scrim's a11y posture (App.tsx:2331). Keep it — keyboard dismissal for a scrimmed modal is platform-derived — but it is an undeclared behavior change.
  • Point patch by scope, accepted-and-deferred: grep animate={{ (width|height|margin counts 22 remaining framer layout-property animations, including transcript expanders that run mid-stream (ToolCallLine.tsx, ThinkingBlock.tsx) — the same jank class the PR's own analysis names. The four panels fixed are the worst offenders; the class survives.

Subtractions

  • Defer the nc-slide-in retune (tailwind.config.js: keep .32s cubic-bezier(.16,1,.3,1) with the new transform keyframes) and the one animateDrawer.curve.test.ts assertion pinning sheet↔drawer curve equality — the threading fix does not need either.

[FIRST-PRINCIPLES-REVIEWED] ff86f46

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of ff86f4656ae09159205f015ec6b1aa251327dd3d — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Give the new mobile right-panel overlay (side-panel-inline, ChatPage.tsx) the same Escape-to-close handler this PR added to the nav drawer — it is now a layered surface covering the chat, and Escape parity was the pattern this diff itself established.

[UX-REVIEWED] ff86f46

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @buluoray overrides the GPT 5.6 finding for ff86f4656ae09159205f015ec6b1aa251327dd3d; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt ff86f4656ae09159205f015ec6b1aa251327dd3d: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed ff86f4656ae09159205f015ec6b1aa251327dd3d — this comment is updated in place on each push.

Review details

Both candidates fail falsification.

CANDIDATE 1 claims the new <div ... onClick={closeMobileNavDrawer} /> scrim in App.tsx trips the blocking accessible-interactive-elements gate. The deterministic enforcement in .github/workflows/code-review.yml is:

git diff "$BASE".."$HEAD" -- 'src/**/*.tsx' | grep -nP '^\+.*<(div|span)(?![^>]*role=)[^>]*onClick'

grep matches line-by-line. In the diff the <div token and the onClick={closeMobileNavDrawer} token are on separate added lines; no single line carries both, so the pattern never matches and the gate does not fire. The candidate's entire theory rested on [^>]* "crossing newlines," but the actual config is a per-line grep that cannot cross lines regardless. Dead on (c).

CANDIDATE 2 claims the shared holdUntil in streamHold.ts lets a finishing slide zero the hold for another in-flight slide. The single global is intentional ("the shared clock"), and the failure requires two different drawer MotionValues (drawerX, sideOverlayX, mobileNavX) animating simultaneously — an overlap the candidate itself concedes "may be rare" and could not tie to a concrete triggering flow. Even granting the overlap, the slide runs on the compositor and is immune to resumed main-thread flushes; the hold only guards ancillary main-thread churn described as "cheap insurance." Both (a) the concrete guaranteed input and (c) the observable failure resolve to "might," below the bar. The proposed fix (reference-counting) is also a design refinement, not a reachable defect.

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 ff86f4656ae09159205f015ec6b1aa251327dd3d.

False positive or not applicable? A repository writer can comment:
/ai-review override fable ff86f4656ae09159205f015ec6b1aa251327dd3d: <one-sentence reason>

@buluoray
buluoray force-pushed the fix/mobile-panels-compositor branch from 9c4ce31 to 4ff3d85 Compare August 29, 2026 23:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — round 1 (head 4ff3d8511)

GPT 5.6 BLOCKING (App.tsx nav backdrop keyboard access): fixed, with a different remedy than suggested.
The concern is legitimate — the drawer had no keyboard dismissal path. The suggested Clickable remedy would make the full-screen scrim a role="button" tab stop covering the entire page, which is the pattern screen-reader and keyboard users complain about (a giant unlabeled stop between them and the page). Instead:

  • the scrim is now aria-hidden="true" (decorative; its click-to-dismiss is a pointer convenience), and
  • an Escape keydown handler closes the drawer while it is open — the standard keyboard dismissal for a modal surface, matching the notification popover's existing Escape path in the same file.
    Both halves are pinned by mobilePanels.compositor.test.ts (mutation-verified: removing aria-hidden or neutering the Escape handler each turns the guard red).

Automated Rule Check (.innerHTML in animateDrawer.compositor.test.ts): fixed — test cleanup now uses document.body.replaceChildren().

Frontend Lint warning ratchet (663 > 659): fixed — the diff added exactly 4 warnings, all mine: an unused test import (removed), the two jsx-a11y warnings on the scrim (resolved by the a11y fix above), and a deliberate narrowed-dependency callback in ChatPage now carrying an explicit eslint-disable-next-line with rationale. Branch total is now one below the ratchet (a pre-existing warning was also absorbed).

Screenshot Evidence: dispositioned via the documented waiver — a **Why no screenshot:** line was added to the body: frame pacing is not visible in a still, and at-rest states are unchanged except the documented right-panel overlay tradeoff, verified on-device.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@buluoray
buluoray force-pushed the fix/mobile-panels-compositor branch from 4ff3d85 to 0299a96 Compare August 29, 2026 23:42
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — round 2 (head 0299a96a0, rebased onto current main)

GPT 5.6 BLOCKING (reduced-motion users cannot open the mobile side panel): legitimate, fixed.

Verified against source rather than the finding text: animateDrawer returns mainThread() early under prefersReducedMotion() (useDrawerSwipe.ts:213), and that path only tweened the MotionValue — publishArrival, which writes the elements' own inline styles, lives on the compositor path alone. The nav drawer is a plain <nav> and the right overlay's transform is a template string serialized once per React render, so neither is bound to x; and neither has a drag gesture, which makes this fallback their only path whenever the compositor one is unavailable. So the value travelled to 0 while the DOM stayed at the mounted closed offset: the panel opened into nowhere. Same root cause as the cancel-fill bounce fixed earlier in this PR — a non-framer-bound element needs the write spelled out — reappearing on the fallback path.

The finding also reaches one case GPT did not name: the bounded mount-grace fallback (tries-- exhausted → stopMain = mainThread()) had the identical hole with motion enabled.

Fix: mainThread() now subscribes to x and paints the panel's transform and the scrim's opacity for the life of the tween (initial paint up front, unsubscribed on completion and on cancellation). Rather than binding the mobile transform to a Framer x style as suggested — that would make framer own the ancestor transform, which is what staticRows exists to prevent, and would re-open the row fly-in this PR fixes.

Guard: animateDrawer.compositor.test.ts now asserts the element starts painted, tracks x through the tween, moves the scrim in lockstep, and stops writing once the settle is cancelled. Mutation-verified — dropping the subscription reddens it, and leaking it past stop() reddens it too. The pre-existing reduced-motion case only asserted "no compositor animation", which is exactly why this shipped.

Screenshot Evidence: still red, and deliberately not waived by me. The workflow accepts a waiver only with BOTH <!-- no-visual-delta --> and a justification line; I added the justification but will not add the marker, because it would be a false claim — the mobile right panel genuinely changes at rest (it overlays the chat column instead of squeezing it). That needs either the maintainer no-screenshots label or real before/after captures. Flagged to the author rather than routed around.

Rebased onto current main (10 commits) with no conflicts; targeted gates green (60 drawer/panel tests, tsc -b, no new lint warnings).

@bolichen97
bolichen97 enabled auto-merge August 29, 2026 23:43
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@buluoray
buluoray force-pushed the fix/mobile-panels-compositor branch from 0299a96 to 698ac36 Compare August 29, 2026 23:54
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — round 3 (head 698ac3673)

GPT 5.6 BLOCKING (reversing a drawer leaves the stale close animation active): legitimate, fixed — and the impact is worse than the finding states.

Verified against source: start() assigned rt.running = anims without cancelling the previous settle's animations, and no caller keeps animateDrawer's returned canceller (all six call sites in App.tsx / ChatPage.tsx discard it), so nothing else retired them. Two independent failures follow from one survivor:

  1. The replacement keyframes from the wrong place. x deliberately does not move while the compositor owns the offset (pinned by this file's own first case). On a reversal, from = x.get() therefore still reads the offset the outgoing settle started from — a close→open reversal keyframes 0 → 0, so the reopen snaps instead of sliding. GPT did not name this half.
  2. The survivor wins after arrival. It is fill: 'forwards', so once the replacement is cancelled on arrival (the deliberate jump-then-cancel order this PR established), the stale fill re-presents its own end state over the just-published inline style. The panel lands open and then goes back offscreen with its phase still open — unreachable, the same user-visible shape as the bounce fixed earlier in this PR, reached by a different mechanism.

Fix: start() now calls takeOverDrawer(x) before keyframing. That is the existing primitive for exactly this — it reads the presented offset out of the element's matrix, writes it into both the inline style and the MotionValue, and cancels rt.running — so it repairs both halves at once (the reversal starts from where the panel actually is, and the outgoing animation is retired rather than left filling). This is GPT's suggested remedy; adopted as-is because it composes with the design rather than working around it.

Idempotent on the paths that already take over: the drag-release path (useDrawerSwipe) and the mount-grace rAF retries both re-enter with rt.running empty, where it reduces to a no-op x.stop().

Guard: animateDrawer.compositor.test.ts adds a mid-settle reversal case asserting the outgoing animation is cancelled, the replacement keyframes from the presented -260px rather than a stale 0, the retired animation's finish publishes nothing, and the live one still arrives. Mutation-verified — removing the takeOverDrawer(x) call reddens it.

Gates: 164 tests across the 9 affected suites, tsc -b clean, no new lint warnings.

Screenshot Evidence remains red by the author's decision (documented in round 2): waiving it would require the <!-- no-visual-delta --> marker, which would be a false claim since the mobile right panel genuinely changes at rest.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 30, 2026
@buluoray
buluoray force-pushed the fix/mobile-panels-compositor branch from 1e4c9a7 to f2586ca Compare August 30, 2026 05:20
@buluoray

Copy link
Copy Markdown
Contributor Author

/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.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for f2586ca3b42f607583872a956a9c035ffd2b587e.

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.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 30, 2026
@buluoray
buluoray force-pushed the fix/mobile-panels-compositor branch from f2586ca to a7f2f6d Compare August 30, 2026 06:10
@buluoray

Copy link
Copy Markdown
Contributor Author

/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.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for a7f2f6db2599f7070c7ba77b136fe055ff909c08.

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.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 30, 2026
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.
@buluoray
buluoray force-pushed the fix/mobile-panels-compositor branch from a7f2f6d to ff86f46 Compare August 30, 2026 07:00
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Aug 30, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

/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 ff86f4656; the previous override was scoped to a7f2f6db2, which the comment-only amend replaced. Nothing about the scrim changed between the two commits — the amend touches two header comments and no code.

@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — First Principles Review (CONCERNS on a7f2f6db2)

Three points raised. Two were real and are fixed in ff86f4656; the headline one is a false positive, with the counter-evidence below.

Rebutted — the layoutId under the right overlay is not a rendered node

The two premises hold: ActivityViewer.tsx:908 does pass layoutId="activity-tab", and sideOverlayPanelRef is attached at ChatPage.tsx:8228 with <SidePanel> as a child inside it. What the count misses is reachability.

That SegmentedControl sits inside {!view && ( … )} (ActivityViewer.tsx:902). SidePanel.tsx:763 passes view={t.kind} unconditionally, and every tab kind is a non-empty literal ('summary', 'pins', 'changes', 'issues', 'files', 'links', 'artifacts', 'subagents', …). So when ActivityViewer is mounted from SidePanel — the only non-test <ActivityViewer mount in the repo — the tab bar never renders.

Projection nodes under the registered overlay: 0, not 1. The grep counts a layoutId literal in a file; the precondition is about nodes in the rendered tree.

One thing worth adding beyond the finding: SegmentedControl's layoutId is optional and defaults to 'segment', and it feeds a framer layoutId at SegmentedControl.tsx:176. So the hazard is the component, not that one string — a future <SegmentedControl> mounted under a registered panel would be a projection node even with no layoutId prop written at the call site. Searching for the prop name would not find it.

Fixed — the two new files no longer carry a disproved rationale

Correct, and it violated AGENTS.md's comment rule ("state current behavior", no "was tried" narration) in files this PR adds. Both headers now state that the slides run on the compositor and that the hold covers what the compositor cannot — the panel's own mount, tool events, subagent status pushes, and the main-thread drag path — and point at animateDrawer's comment (hooks/useDrawerSwipe.ts:227-239) as the authority rather than contradicting it.

Comment-only: tsc -b exit 0 and the 7 guard files' 41 tests pass unchanged.

Fixed — Escape-to-close is now declared

Added to the PR description. It is keyboard dismissal for an aria-hidden scrim, which is also the load-bearing half of the accessibility argument in the GPT override, so leaving it undeclared was a real gap in the write-up.

@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@buluoray marked the gpt AI finding as false positive, not applicable, or explicitly accepted for ff86f4656ae09159205f015ec6b1aa251327dd3d.

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.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 30, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — First Principles Review (CONCERNS on ff86f4656)

The round-1 layoutId finding is gone from this pass, so I'll take the rebuttal as accepted. Three points this round: one declared, one already fixed before the review read it, one deferred with an issue.

Declared — the notification sheet's entry retune

Correct, and the numbers are exact: nc-slide-in goes .32s cubic-bezier(.16,1,.3,1).42s cubic-bezier(.32,.72,0,1), and the threading fix does not require it. Now declared in the description as a separable choice, with how to revert it (two literals in tailwind.config.js plus one assertion).

I'm keeping it rather than deferring it, and the reason is that the framing of "a motion-language unification" understates what is already on main. [0.32, 0.72, 0, 1] is not introduced by this PR — it appears in five places on main:

  • components/OverlayDrawer.tsx:45 — the named EASE constant
  • pages/ChatPage.tsx:7174 — a literal cubic-bezier(.32,.72,0,1)
  • pages/ChatPage.tsx:8120duration: 0.4, ease: [0.32, 0.72, 0, 1]
  • pages/chat/SessionFlyout.tsx:217
  • App.tsx:3467

So this is the repo's established overlay curve, and the sheet was the outlier. The suggested subtraction — keep .32s cubic-bezier(.16,1,.3,1) with the new transform keyframes — would ship the sheet on a different entry curve from the four panels it sits beside, and would mean deleting the assertion that keeps them in step. That is the drift that does not get cleaned up later.

Already fixed — Escape is declared

The description gained an item declaring Escape-to-close and the scrim's aria-hidden posture before this review ran, so this point reads a stale description. Worth naming the mechanism rather than just correcting it: a description edit does not change the head SHA, so a bot that re-reads on SHA can serve its previous read of the body. Nothing to do beyond confirming it is there now.

Accepted and deferred — the surviving jank class → #6957

The count is right: 22 framer animations still animate height/width/margin, and the transcript expanders among them are the ones that matter, for exactly the reason given — they expand while tokens arrive and the main thread is already contended. Filed as #6957 with the full inventory.

Two corrections to the specifics, neither affecting the conclusion. The two exemplars are at pages/chat/ToolCallLine.tsx and pages/chat/ThinkingBlock.tsx, not components/. And the pattern as written (animate={{ (width|height|margin) matches none of them — the properties are not in first position, so animate=\{\{[^}]*(height|width|marginRight|marginLeft): is what reproduces the 22.

Out of scope here for a substantive reason, not just size: height: auto has no compositable equivalent, so each site needs its own judgment about what it can give up (counter-scaled scaleY, grid-template-rows, a clip reveal, or a measured fixed-height inner). #6957 carries that, plus the two constraints this PR established — the projection precondition, and that the hazard is the component (SegmentedControl's layoutId defaults to 'segment', so a call site writing no layoutId is still a projection node).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants