refactor(ui): make the transcript the only writer of its scroll position - #1
Closed
Astro-Han wants to merge 2 commits into
Closed
Conversation
Three writers moved `scrollTop` in the chat transcript — Astryx's auto-follow lock and spring, Maka's height-delta compensation and `scrollIntoView`, and the browser's own anchoring — and none of them held the answer to "where should the viewport be". They avoided each other through flags, effect ordering and heuristics reconstructed from DOM signals, and each heuristic had more than one cause. Collapse the policy to one boolean. `pinned` means growth writes `scrollTop = scrollHeight`; not pinned means nothing here writes it, ever, and `overflow-anchor: auto` — already the initial value — keeps the reader where they were reading for free. Around it sit three one-shot commands: return to the tail, jump to a turn, and compensate earlier history at `scrollTop === 0`, the one place native anchoring declines to help. Every command releases the pin first, so a command can never race the policy. Being the only writer is what makes the state exact. A write flags itself, so an unflagged `scroll` event is the reader by construction — and because `scroll` does not bubble, a gesture a nested scroller consumed never reaches the authority at all. Astryx's scroll layer is turned off per call site through a new `scrollOwner` prop rather than globally: the workhub surfaces render no `ChatView` and still want stock auto-follow. The growth signal reuses the turn virtualizer's existing `ResizeObserver`; no new observer is added. The patch's scroll-related surface shrinks from six files and ten hunks to three files and five hunks — `conversationKey`, `unlockAutoFollow` and `resetInitialFill` all disappear, replaced by forwarding one `autoScroll` flag into the hook's existing `enabled` switch. Generated-by: Claude Opus 5 via Claude Code
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Five Playwright assertions over the real Electron window, one per property the single scroll authority is supposed to have: a streaming answer keeps the tail on screen, content arriving after the reader scrolls up does not pull them back, a gesture a nested scroller consumed does not release the tail, the dock affordance returns the reader to the tail, and earlier history lands above the turn the reader is on. They assert element positions rather than pixel deltas. Where lag has to be measured at all it is self-calibrating — the worst frame's lag is compared against that frame's own growth, because following by `ResizeObserver` is one frame behind by construction and that frame is never painted. A fixed pixel budget would encode the machine it was written on. Generated-by: Claude Opus 5 via Claude Code
Astro-Hanforce-pushed
the
refactor/chat-scroll-single-authority
branch
from
August 29, 2026 03:46
9ddbeb8 to
ecd3290CompareAstro-Han
commented
Aug 29, 2026
OwnerAuthor
Superseded: this work is being merged into apache#4105 directly, since that PR is the teardown and this is the rebuild — they cannot land independently. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on apache#4105 — review that first; this diff is against its branch.
Three writers moved
scrollTopin the chat transcript: Astryx's auto-follow lock and spring, Maka's height-delta compensation andscrollIntoView, and the browser's own anchoring. None of them held the answer to "where should the viewport be", so they avoided each other through flags, effect ordering, and heuristics rebuilt from DOM signals — scroll direction, height deltas, wheel events — each of which has more than one cause. apache#4105 fixed the symptoms that were reachable; the three regressions it records as knowingly unfixed are the ones that need the ownership itself to move, and this PR eliminates all three.The policy is now one boolean:
pinned— content that grows writesscrollTop = scrollHeightscrollTop, ever, andoverflow-anchor: auto(already the initial value, no CSS added) keeps the reader where they were readingAround it sit three one-shot commands: return to the tail, jump to a turn, and compensate earlier history at
scrollTop === 0— the single place native anchoring declines to help. Each releases the pin before it moves anything, so a command cannot race the policy.Being the only writer is what makes the state exact rather than guessed. A write flags itself, so an unflagged
scrollevent is the reader by construction. And becausescrolldoes not bubble, a gesture a nested scroller consumed never reaches the authority at all.Deliberate choices worth a reviewer's attention:
scrollOwnerprop onChatSurfaceLayout, not globally. The workhub surfaces render noChatViewand still want stock auto-follow.ResizeObserver, called synchronously in its callback so the pin writes in the frame the content grew.scrollHeightdelta counts growth below the reader too, and counts a load that returned nothing as a push. It also picks the first visible turn, not the first mounted one — the virtual window is free to drop turns above the viewport while the load lands.attachruns in a passive effect, not a layout one. The scroller is Astryx's layout root, an ancestor, and React attaches a parent's ref after its children's layout effects have run.animatingRef, and CDP measurement on the built branch found that ref sitting attrueon a resting transcript (scrollHeightconstant for 2.5s, spring target reachable, six seconds of quiet before the gesture) — emptying the stock wheel handler made the release disappear and restoring it brought it back, three times over. A "is something animating" flag desyncs from the rAF chains it is supposed to describe, so no equivalent flag was kept for compatibility.Because Maka now owns the position, the patch's scroll-related surface shrinks from six files and ten hunks to three files and five hunks:
conversationKey,unlockAutoFollowandresetInitialFillare gone, replaced by forwarding oneautoScrollflag intouseChatStreamScroll's existingenabledswitch. Patch file: 55 hunks → 49, 149 of its lines rewritten.Two unit test files are deleted rather than ported. Both exercised
unlockAutoFollowand the conversation-swap reset — mechanisms this PR removes, not behaviours it changes.Refs apache#4105
Verification
apps/desktop/e2e/transcript-scroll.spec.ts— 5 passed (1.0m), real Electron window:mouse.wheelover a real overflowing child, guarded by asserting the child actually moved)packages/uiscroll + rail unit tests: 13 passed, 0 failed (node --test --test-concurrency 4), including 4 new cases for the authority.prompt-rail,streaming-remount,quote-selection: 12 passed, 1 skipped.npm run build -w @maka/ui,npm run typecheck -w apps/desktop(preload/main/renderer/storybook),npm run typecheck:stories -w apps/desktop,npm run format— all clean.git apply --check --reverse patches/@astryxdesign+core+0.5.0.patchagainst annpm ci-installed tree exits 0.Review focus
The e2e assertions deliberately avoid fixed pixel budgets. They compare element positions before and after, and where lag has to be measured at all it is self-calibrating — the worst frame's lag is compared against that same frame's growth, because following by
ResizeObserveris one frame behind by construction and that frame is never painted.AI use
Tool(s) and scope: Claude Opus 5 via Claude Code — implementation, tests, and this description, under human direction and review. Affected commits carry
Generated-bytrailers.Checklist
Does this PR entail a change in behavior?