Uh oh!
There was an error while loading. Please reload this page.
fix(ui): land a switched-to session at its latest turn instead of flying there - #2239
Conversation
…ing there Switching sessions played a visible scroll animation: the new transcript appeared near its top and travelled down to the latest turn over ~1.2s. Measured on the 24-turn scroll-geometry fixture, the scroller covered 10844px of that flight in front of the reader. Astryx's `useChatStreamScroll` positions the FIRST fill of its scroller instantly and springs every later growth. That one-shot lives on the hook instance, and `ChatSurfaceLayout` mounts once for the whole app shell, so it is spent on whichever session was open at boot. Every switch after that counts as "later growth" — and a switched-to transcript does not arrive in one piece: the apache#2052 progressive mount commits a tail window, idle chunks fill the prefix, and the content-visibility warm-up then inflates every placeholder. Each step grew the document under a scroller the spring was chasing, so the session opened mid-document and animated to its end. `useChatScroll` wrote `scrollTop` once on the session change, which is too early to help: at that point the switched-to transcript is still an empty scroller, and every piece that lands afterwards restarts the flight. An arrival-scoped bottom pin consumes those growth steps instead. It writes `scrollTop` from a ResizeObserver on the message list — after layout, before paint — so the growth the spring would have animated is already spent by the time a frame is painted, and the spring settles against a zero delta rather than running. It owns the arrival window only: the warm-up releases it once the geometry settles, and steady-state following (streaming, appended turns) stays Astryx's, as does everything after the reader takes over. Any sign they did takes the pin off for good, using the signals Astryx itself unlocks on — an upward wheel, a touch drag, or a scroll that moved up while the geometry held still, since the arrival window is nothing but resizes and Chromium fires a synthetic scroll for each one. Turn navigation releases it too. The pin publishes `data-arrival-pin` the way the warm-up publishes `data-turn-warmup`, so a test can wait on the real boundary. The new E2E samples the scroller every frame across a switch — a polled probe reads an animation as a sequence of reasonable positions — and asserts the transcript is flush in every frame it exists, with the document proven to have grown under the watch. It fails on the parent commit with maxDistance=11267 and passes here with 0.
Astro-Han
commented
Aug 5, 2026
Thanks for the fix and the detailed write-up — the trace-first diagnosis is great, and I verified the key claims against the Astryx source: the one-shot I ran the new unit tests (9/9 pass) and reviewed the e2e. The regression coverage is solid — per-frame sampling is a real upgrade over polling. A few non-blocking suggestions: P2 — worth a quick add
P3 — optional polish
None of these block the fix — happy to help with any of them. |
Review item 1 on apache#2239: no unit case drove growth through a real pin instance into the synthetic scroll Chromium fires for it, and out the other side into the reader taking over. Dropping the geometry snapshot refresh in the scroll handler's growth branch left all nine cases and the E2E green while reader takeover after growth silently broke. Two cases, one per half. The first rides a growth the observer reports: the pin follows it, ignores the resize's own scroll event, and still releases on the upward scroll that follows. The second covers growth the observer never sees — the dock lives inside the scroller but outside the observed message list, so it moves scrollHeight with nothing but a scroll event — and fails without that refresh, which is the mutant the first case cannot catch on its own.
Review item 2 on apache#2239: the watch checked its 30s deadline only inside the rAF callback, so it could only expire while frames were arriving. A compositor that stops ticking never reaches it, and the run dies on the 60s Playwright timeout instead — 'Target page closed', which says nothing about what the scroller did. climbToTop in this same file already guards its frame waits for exactly this reason. A setTimeout now rejects independently of the frame clock, both exits run through one settled flag, and every rejection carries the counters the watch had collected.
Review item 3 on apache#2239: the fake let scrollTop hold any value written to it, so the growth cases asserted the implementation's literal write (scrollTop === scrollHeight) rather than the outcome that write is for. The fake now clamps to scrollHeight - clientHeight the way a scroller does, and the cases read distanceFromBottom === 0 — which a pin that overshot, or one that stopped short, both fail.
Review item 4 on apache#2239: only an upward wheel was covered as "holds", so a threshold written as `deltaY <= 0` would have passed. A horizontal wheel reports deltaY 0 — a sideways swipe across a wide code block or table is not the reader leaving the latest turn.
Review item 5 on apache#2239: the watch collected a frame count and never asserted it. It is now the sampling-density guard — frames must exceed the number of distinct heights, so at least some samples landed on a quiet frame rather than on the moments the document changed. A spring mid-flight lives exactly in those quiet frames, which is what makes the flush assertion below evidence.
…ranscript Review item 6 on apache#2239: the wheel and touch handlers listen on the scroller root, and the dock — composer, plan panel, graph status — is inside it, so a wheel over the composer or any touch anywhere released the pin without evidence the reader had left the latest turn. Astryx gates the same two signals on its animation state; the equivalent here is where the gesture happened. Both now require the event to have started inside the observed transcript element. Nothing is lost by being strict: these handlers only exist to beat the scroll event they cause, and a gesture that really moves the scroller still reaches the scroll handler, which decides on what the geometry did rather than on where the pointer was. That fallback is what the new dock case asserts, so the strictness cannot silently cost the reader control.
ARE404
commented
Aug 5, 2026
Thanks — all six suggestions are in, one commit each, pushed on top of the original. P2
P3
On the
Local: |
Astro-Han
commented
Aug 5, 2026
Thanks — all six are in and verified, and I checked them one by one. The P2-1 pair is exactly as advertised: I confirmed by removing the snapshot refresh that exactly one test fails ( P2 — the scoping commit silently turned the progressive-fill e2e's unlock into a race. P2 — the stated reason for leaving turn-navigation release uncovered doesn't hold. The gap itself is fine to defer, but "fixture-seeded transcripts are written straight to storage and never reach the search index" isn't right: fixture headers default to P3 (optional): the watchdog's deadline-fail path leaves the timer and rAF loop running (dangling 35s timer in a closing page — harmless); d82f242's message overstates — an overshooting pin does not fail the new clamp assertions (overshoot is unobservable behind the clamp, as in a real scroller; the old literal assertion caught mutants the new one can't, but those mutants are behaviorally inert); "data-arrival-pin cleanup is exercised by e2e" is overstated — the The original P2/P3 list is fully addressed and the fix itself is sound. Approving — happy to re-review the tryHold scoping if you take it. |
Follow-up on apache#2239: scoping the pin's eager release to gestures over the transcript turned this test's unlock into a race. tryHold dispatches its WheelEvent on the scroller root, which is no longer inside the pin's content element, so the wheel stopped releasing the pin and the only remaining path was the upward write's scroll event — which the pin ignores whenever a growth lands in the same rendering update. The loop retries at most 20 times and CI runs no retries, so the test was left depending on a growth-free frame arriving in time. Dispatching at the message list restores the intent: the gesture is over the transcript, which is what a reader's wheel is. Astryx's own unlock is unaffected either way.
Review nit on apache#2239: the deadline rejection left the 35s watchdog armed and the rAF loop running in a page that is about to close. Harmless, but both exits should look the same — they now clear the timer and the loop returns on the settled flag.
Review nit on apache#2239: d82f242's note claimed the clamp catches a pin that overshoots. It does not — a real scroller clamps too, so overshoot is unobservable on both sides, which is why it is also behaviourally inert. The clamp earns its place in the other direction: it turns any arithmetic that stops short into a distance, independently of how the value was computed.
ARE404
commented
Aug 5, 2026
Thanks for the approval, and for catching the 1. 2. The deferral reason — you're right that mine was wrong, and the corrected one still lands somewhere else. I re-checked against the code and then against a live fixture window instead of arguing from either. You're right on both facts I got wrong: there is no index to miss ( So the corrected reason is narrower than my original one and narrower than "it works": in the long-transcript fixture, title hits come back and content hits do not. And a title hit's target carries no I don't want to assert a cause I haven't proven. What I can say is black-box: the transcript renders 24 turns from the same session while 3. P3 nits — both taken. Local after the three commits: |
Uh oh!
There was an error while loading. Please reload this page.
ARE404
commented
Aug 6, 2026
Filed the search finding as #2305 — probe output, the |
chat-surface-layout states that Astryx owns scrolling and new-message following. arrival-bottom-pin was a second implementation of exactly that, added in #2239 because ChatLayout exposed only scrollContainerRef and contentRef, so its controller could not be reached. #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 #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
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
Problem
Switching sessions plays a visible scroll animation: the new transcript appears near its top and travels down to the latest turn. On the 24-turn
long-transcriptfixture the scroller covers 10844px over ~1.2s in front of the reader, sampled every frame across the switch:Cause
Astryx's
useChatStreamScrollpositions the first fill of its scroller instantly and springs every later growth. That one-shot (initialFillPendingRef) lives on the hook instance, andChatSurfaceLayoutmounts once for the whole app shell —app-shell.tsxtoggleshidden, it never remounts — so the instant path is spent on whichever session was open at boot. Every switch afterwards is "later growth".And a switched-to transcript does not arrive in one piece. #2191's progressive mount commits a tail window, idle chunks fill the prefix, and the
content-visibilitywarm-up (#827) then inflates every 250px placeholder. Each step grows the document under a scroller the spring is already chasing — which is why the flight lasts a second rather than a frame, and why the trace above shows the spring restarting at every jump inscrollHeight.useChatScrollwrotescrollTop = scrollHeightonce on the session change, which is too early to help: at that point the switched-to transcript is still an empty scroller.Fix
packages/ui/src/arrival-bottom-pin.ts— a bottom pin scoped to the arrival window of a switched-to transcript.It writes
scrollTopfrom a ResizeObserver on the message list, i.e. after layout and before paint, so the growth the spring would have animated is already consumed by the time a frame is painted and the spring settles against a zero delta instead of running.It owns the arrival and nothing else:
The pin publishes
data-arrival-pinon the scroller, the way the warm-up publishesdata-turn-warmupand the fill publishesdata-progressive-fill, so a test can wait on the real boundary instead of guessing at timing.After the fix, the same switch, same sampling:
Tests
packages/ui/src/__tests__/arrival-bottom-pin.test.ts— 9 cases over the pure module: follows every growth step, ignores the synthetic scroll a resize fires, holds through a sub-pixel readback of its own write, releases permanently on reader intent, detaches on dispose.apps/desktop/e2e/scroll-geometry.spec.ts—a session switch lands on the latest turn instead of flying to it. Sampled per frame rather than polled, because the regression is an animation and a poll reads it as a sequence of individually reasonable positions. It asserts the transcript is flush in every frame it exists, and that the document actually grew under the watch, so a run that measured nothing fails loudly. On the parent commit it fails withmaxDistance=11267; here it reports0.Verified locally:
format:check,lint,typecheck,knip --workspace packages/ui,@maka/ui(352 tests), and the fullscroll-geometrysuite (8/8) — includingprogressive fill preserves the reading anchor while earlier turns mount, which is the case where the pin has to get out of the way.Notes for review
ChatLayoutpassesref={mergeRefs(ref, rootRef)}, a fresh callback ref each render, so React detaches and reattaches its root ref around every commit andscrollContainerRef.currentisnullwhile a child's layout effects run. That is the same reason the warm-up effect below it is passive.Reported by a user watching every session switch scroll itself into place.
🤖 Generated with Claude Code