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.
|
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.
|
Thanks — all six suggestions are in, one commit each, pushed on top of the original. P2
P3
On the
Local: |
|
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.
|
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: |
|
Filed the search finding as #2305 — probe output, the |
2 participants
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