perf(ui): stop the transcript from moving after a session switch - #4105
perf(ui): stop the transcript from moving after a session switch#4105Astro-Han wants to merge 5 commits into
Conversation
Switching sessions scheduled a requestIdleCallback that fired roughly 150ms later and called requestEarlier() unconditionally, inserting earlier-history turns above the reader's scroll position. Those turns then inflated in waves as their content resolved, moving the transcript under the reader three times after it had already settled. Nothing asked for that history. The prefetch was speculative: it ran on arrival rather than on any reader intent, and the upward-scroll and wheel handlers already load earlier history when the reader actually approaches the top. Measured with alternating within-instance A/B (3 repetitions per configuration, sigma about 0.02): mean CLS across four sessions drops from 0.481 to 0.085. Removing the only reader of requestEarlierRef also removes the cross-effect mutable-callback coupling between the history loader and the arrival gate, so the arrival effect no longer depends on hasOlderHistory or canLoadEarlier. Capability given up: earlier history is no longer warmed during the switch, so the reader's first upward scroll pays one load. Generated-by: Claude Code
chat-scroll-anchor captured a turn id plus its offset before content landed above the reader and restored that position a frame later. This is what browser scroll anchoring already does, and does better: the browser compensates during layout, not a frame after it. Measured directly: inserting 1500px above the viewport mid-scroll moves the visible content 0px. Measured in place: with the arrival prefetch gone, removing the hand-rolled anchoring leaves mean CLS unchanged (0.085 to 0.090, byte-identical on three of four sessions). The anchoring was carrying no load once nothing inserted content the reader had not asked for, which is why it could only go after the prefetch. Removing it also removes the double compensation it sat behind — a scrollTop adjustment by the scrollHeight delta, applied whenever the restore reported failure — and the pendingAnchor round trip through the virtualizer's window installs and resize observer. Added in its place: overflow-anchor stated on the transcript column so the dependency is legible rather than inherited from the default, and a one-pixel nudge when the earlier-history request starts at the very top, where anchoring is suppressed and would otherwise let the incoming turns jump the reader. Generated-by: Claude Code
chat-surface-layout states that Astryx owns scrolling and new-message following. arrival-bottom-pin was a second implementation of exactly that, added in apache#2239 because ChatLayout exposed only scrollContainerRef and contentRef, so its controller could not be reached. apache#2923 opened that seam for unlockAutoFollow and the pin was never revisited. Reading Astryx's controller, it already covers what six review rounds put into the pin: resize-synthetic scroll events are excluded by comparing scrollHeight and offsetHeight, a horizontal wheel is excluded by requiring deltaY < 0, and gestures are scoped by binding to the scroller itself rather than by testing where the pointer was. Its initial fill positions in one frame instead of springing from the top, which is what the pin's clamp existed to produce. The one gap was reachability again: on a conversation change the patch called lock(), which re-enters through the spring because the hook's initial-fill flag was consumed at mount. Asking for the instant jump directly closes it, in the patch that was already there. Removing the pin leaves two moves Astryx cannot see, both now going through the context: navigating to a turn and loading earlier history release auto-follow, and "return to latest" resumes it. The second needed the other half of apache#2923's seam, so the patch also exposes scrollToBottom. Both are additive context fields to upstream. data-turn-window went with the pin it gated: its ready state existed to release the pin, and the fonts.ready wait plus fifty markdown polls plus double rAF existed to time that release. The two E2E tests that waited on it wait for a mounted turn instead, which is what they were after. latestNavigationNonce was left write-only and goes too. arrival-bottom-pin.test.ts is replaced by a test of what Maka still owns, the two release moments, rather than a test of Astryx's internals. Capability given up: a wheel or touch over the dock while the transcript is animating now releases following, where the pin discriminated by gesture origin; and returning to the bottom re-locks following, where the pin's release was permanent for that arrival. Generated-by: Claude Code
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I found no P0–P3 issues on exact head e7baf0d75847edf0d2f953a83980e1d6ab186457.
The session-switch jump was a real path: after arrival, useChatScroll scheduled an idle callback that called requestEarlier() whenever the scroller was still near the top, which is exactly where a newly filled transcript sits. That inserted history the reader had not asked for, and those turns then grew under them. The upward-scroll and wheel handlers already load earlier history on intent, so the prefetch had no job.
What remains of Maka's scroll work is the pair Astryx cannot see: jump-to-turn and earlier-history load both call unlockAutoFollow, and "return to latest" calls scrollToBottom({ behavior: 'instant' }). @astryxdesign/core@0.5.0 already has that method; the patch only exposes it and uses an instant jump on conversationKey change instead of lock(), which re-entered the spring. The one-pixel nudge at scrollTop === 0 matches the documented overflow-anchor hole. I did not treat issue #4099 as evidence.
I am not merging. This is a behavior-changing scroll-ownership change; merge is a human call. Hosted test was still queued when I posted. This review does not claim CI is green.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
精确 head e7baf0d75847edf0d2f953a83980e1d6ab186457 上我没有发现 P0–P3。
会话切换后的跳动是真实路径：到达后 useChatScroll 会在空闲时无条件预取更早历史，而新填满的转录往往还停在顶部附近，于是读者没要的回合被插进来并在下方长高。向上滚动和滚轮已经会按意图加载，这段预取没有工作可做。
Maka 还留下 Astryx 看不见的两处：跳到指定回合、加载更早历史会 unlockAutoFollow；回到最新会 scrollToBottom({ behavior: 'instant' })。@astryxdesign/core@0.5.0 已有这个方法，补丁只是暴露它，并在 conversationKey 变化时立刻跳到底，而不是 lock() 再走弹簧。scrollTop === 0 时的 1px 挪动对得上 overflow-anchor 在最顶端不生效的缺口。我没有把 issue #4099 当证据。
我不合入。这是会改行为的滚动归属调整，合入由人类决定。发这条时 hosted test 还在排队，这次审查不表示 CI 已绿。
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
I am approving exact head e7baf0d75847edf0d2f953a83980e1d6ab186457 because I found no P0 or P1 issues. I left one non-blocking P2 inline: when a session's transcript arrives asynchronously, the conversation-change jump consumes Astryx's pending first-fill state while the scroller is still empty, so the later content still enters through the spring instead of landing at the bottom in one step.
The simplification is otherwise real: the change removes the speculative prefetch, two product-owned scroll authorities, their gating state, and their dedicated tests instead of moving them elsewhere. The remaining host seams are limited to the two moves Astryx cannot infer itself: releasing auto-follow for reader-directed navigation and re-locking it when returning to the latest turn.
I verified all 237 UI tests, the Desktop production build, seven real Electron prompt-rail tests, Biome, and the changed diff. The exact-head hosted windows_recovery check is green; hosted test is still queued, so this approval does not claim CI is fully green.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
The instant jump on conversation change assumed the incoming transcript was already mounted. It is not: setActiveId clears messages and marks the load pending in the same update that changes the key, so the scroller the swap sees holds only the loading placeholder and has nothing to scroll. scrollToBottom consumes the hook's pending first fill whether or not the jump could do anything, so the transcript arriving a few frames later took the spring path and flew down from the top — the exact motion the pin used to prevent, reintroduced for cold switches only. Arming the fill again after the jump covers both shapes: a transcript already on screen is positioned by the jump, and one that arrives later is positioned in a single frame by the first scrollIfLocked that sees scrollable content. This matches what the measurements showed and I misread at the time: cold first visits settled 677px from the bottom while warm switches settled at 5px, which I attributed to load cost rather than to this path. Reported by M4n5ter's review agent on apache#4105. Generated-by: Claude Code
Adversarial review found three more defects, all one mistake. The mechanisms this branch deleted were continuous: the anchor restored a captured position whenever the content landed, and the pin clamped the bottom frame by frame for the whole arrival. Their replacements are one-shot — Astryx's controller acts on the frame it is called, and the browser anchors at the instant content is inserted. The call sites moved across unchanged, and each one now fires before the DOM reaches the state it assumes. Loading earlier history nudged the scroller off zero when the request was made, but anchoring is suppressed or not at the moment the turns land, a whole IPC round trip later — and the reader coasting upward is back at zero by then. The compensation moves to the frame after the turns are on screen, which is also the first moment their height is known. Return to latest jumped in a promise callback that resolves before React commits the newer range, so it jumped against the old geometry and consumed the pending first fill, leaving the range that arrived after it to spring down from the top. The jump is removed rather than repaired: nothing ever required this button to arrive instantly, and the transcript scrolling to the newest turn shows the reader what happened. With no host calling it, scrollToBottom comes back out of the layout context. Navigating to a turn released auto-follow on every transcript update, not once per chosen target: the effect re-runs on messages so a target that arrives before its turn still lands, and the release it used to make was an idempotent no-op. Astryx's unlock is persistent, and the search target is never cleared, so following stayed off for the rest of the session. Also here, because this change is what surfaced them: the gesture releases were gated on a spring being in flight, which never happens under prefers-reduced-motion, leaving those readers unable to leave the tail by wheel or touch — the predicate is following, not animating. And overflow-anchor moves to the scroller that actually runs it; on the content column it was inert. hasTurns had no reader left. Generated-by: Claude Code
3 participants
Summary
Switching sessions moved the transcript under the reader after it had already settled.
The cause was one speculative prefetch: on arrival,
useChatScrollscheduled an idle callback that loaded earlier history nobody asked for and inserted it above the reader's position, where it then inflated in waves. The upward-scroll and wheel handlers already load that history on real intent, so the prefetch had no job.Removing it left two hand-rolled mechanisms with nothing to do, and both duplicate an authority that already exists:
chat-scroll-anchorrestored a captured scroll position a frame after content landed. Browser scroll anchoring does the same thing during layout instead of a frame late.arrival-bottom-pinre-implemented following the tail, whichchat-surface-layoutsays Astryx owns. It was added in fix(ui): restore the prompt anchor rail and fix its hover and jump #2923's predecessor because Astryx's controller was unreachable from the layout context; fix(ui): restore the prompt anchor rail and fix its hover and jump #2923 opened that seam and the pin was never revisited.data-turn-windowwent with the pin it gated, along with thefonts.readywait, fifty markdown polls and double rAF that existed to time its release.Net 636 lines removed. Added in their place:
overflow-anchorstated on the scroller, a compensation for the one case the browser declines to anchor, and two additive fields on the vendored Astryx patch.Refs #4099
Verification
Measured with alternating within-instance A/B, four sessions, three repetitions each. Only same-instance comparisons are used — the same build across restarts varies far more than the effect.
Left:
main. Right: this branch. Same session switch, the three seconds after it.Review then found four defects that CLS cannot see, because it measures layout shift and all four were wrong scroll positions. They are one mistake: the deleted mechanisms were continuous, their replacements are one-shot, and the call sites moved across unchanged, so each fired before the DOM reached the state it assumes. Each is fixed and covered by a test that fails without it (
96d274f63,fa97885be).Ran:
npm run format,tsc --noEmit -p packages/ui, and the affected@maka/uisuites. Not run: the full repository suite, and theprompt-railE2E whose two waits ondata-turn-windownow wait for a mounted turn.Review focus
Three behaviours change, all inherited from the pin's hand-rolled discrimination:
Two upstream defects surfaced while adopting the hook, both filed: gesture release is dead under
prefers-reduced-motion(facebook/astryx#5662), patched locally because it is an accessibility regression againstmain; andjumpToBottomdoes not cancel its in-flight spring (facebook/astryx#5663), left unpatched because the remaining path to it is narrow.AI use
Select exactly one:
Tool(s) and scope: Claude Code — root-cause investigation, the CDP measurement and recording harness, the code changes, and this description. The contributor of record reviewed the final diff and owns the merge decision.
Checklist
Does this PR entail a change in behavior?