refactor(ui): make the transcript the only writer of its scroll position - #4105
refactor(ui): make the transcript the only writer of its scroll position#4105Astro-Han wants to merge 12 commits into
Conversation
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
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
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
Widening the wheel and touchmove release from "a spring is running" to "we are following" reaches every gesture that merely bubbles through the scroller. The transcript is self-scrolling, so the scroller is the whole ChatLayout root: a tool output body, the pty terminal, the composer, and the graph panel all sit inside it and all scroll on their own. A wheel they consume never moves the outer scroller, so no scroll and no scrollend follow, and the scrollend re-lock can never run. Following is off, the distance to the bottom is still zero so the scroll-to-bottom button stays hidden, and the reader has no way back. That widening was aimed at readers who prefer reduced motion, for whom the spring never runs and both releases are therefore dead code. It is a real gap, but scroll-direction detection still serves them correctly, so what they lose is a shortcut rather than the behaviour. Trading that for a way to silently stop following is the wrong exchange, and it is not what this branch set out to change. Reported upstream instead. Generated-by: Claude Code
fa97885 to
c0de634
Compare
Astro-Han
changed the title
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
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
ecd3290 to
782b5b7
Compare
The transcript carried its own compensation for content landing above the reader: capture a turn's box before an earlier-history load, restore that box after it. It existed for one reason — the browser declines to anchor while a scroller sits at `scrollTop === 0`, which is where the wheel path asks for history. Measured in Chromium, that hole is one pixel deep. Inserting 501px above the reader moves `scrollTop` by 501 at an offset of 1, by 503 at 2, by 551 at 50, and by nothing at all at 0. So raise the offset to 1 before asking, and native anchoring covers the case its absence was being compensated for. Everywhere else the compensation was already redundant: the load fires at `scrollTop <= max(640, clientHeight * 2)`, and at every one of those offsets the browser was anchoring anyway, with the restore computing a delta of zero on top of it. A second authority for a fact the platform already owned, live on one boundary out of 641. Deletes `chat-scroll-anchor`, the `holdAnchor` command and the `anchor` field, leaving the authority with one boolean and two commands. The E2E covers the offset that made this possible: it fails, holding the scroller at zero, without the raise. Generated-by: Claude Code
Three leftovers from when the authority was optional, all of them second behaviours for states nothing can reach: The provider was installed only for `scrollOwner="host"`, so every consumer carried a null branch — a detached snapshot for the dock button, optional calls in `ChatView`, a ref mirror and a guard in `useChatScroll`. Install it unconditionally instead. An authority nobody hands a scroller to writes nothing and costs one object, and `useTranscriptScrollAuthority` can now throw on a missing provider the way `ChatView` already does for a missing layout, because a missing one means the tree is assembled wrong. `autoScroll` sat on the public props while `scrollOwner` decided it, and the internal value was spread last — a caller-supplied one was silently dropped. Omit it from the type; `scrollOwner` remains the only answer. Asking for earlier history was deduplicated twice: `app-shell` refuses a request while one is in flight, and the hook kept its own in-flight ref because the shell's guard read React state that had not updated yet within the same task. Move that guard to a ref at the shell, where the request actually originates, and the hook stops tracking a load it does not own — along with the `historyLoadPending` prop it only needed for the guard. Generated-by: Claude Code
…tent grows The pin had one growth signal, and it came from the transcript: a resize of the content the virtualizer mounts. Nothing told it about the box doing the looking. A window resize, a composer that gains a line, a dock that changes height — each takes pixels from `clientHeight` without touching `scrollHeight`, and a pinned reader is left exactly that far from the bottom with no further signal to correct it. CI caught the case on Linux, where the window settles into its size after the first turn has already streamed: the tail lag through the stream was within a frame's growth, and the transcript still came to rest 302px short of the end. Observe the scroller itself alongside the content. It is the same correction on the same policy — pinned writes the tail, released writes nothing — so it adds a signal rather than a second way to decide. Asking for earlier history no longer releases the pin either. That release assumed the reader had scrolled up to ask, but the request also fires on a transcript short enough that its tail is inside the load band, where the reader is following and must keep following. The authority already sees the scroll that means the reader moved; nothing else needs an opinion. The E2E assertions that failed now report `scrollTop`, `scrollHeight` and `clientHeight` alongside the distance, because a bare distance cannot say whether the content outgrew the reader or the viewport shrank under them. Generated-by: Claude Code
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I am not approving exact head 9eb824f9a9225e615236fd0816d88a05d6c002c5. 1×P1, no P0, no P2/P3.
P1. Earlier-history load is no longer tied to reader intent, and a real ask at the top keeps the pin.
The single-writer pin is the right cut. These two handlers sit outside it. 9eb824f9a stopped unpinning on a history request because a short latest window was requesting while still following — that request should not fire while the reader is at the tail, and a wheel-up at scrollTop === 0 is a reader ask the authority cannot see (scroll does not fire there).
I did not treat issue #4099 as evidence. I am not merging: this is a behavior-changing scroll refactor, and 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 9eb824f9a9225e615236fd0816d88a05d6c002c5 上我不 Approve。1×P1，没有 P0，没有 P2/P3。
P1. 更早历史的加载已经不再绑定读者意图；真正在顶端要历史时，钉住也不会松开。
单一写入者这个切法是对的。出问题的是钉住外面的两个 handler。9eb824f9a 不再在请求历史上松钉，是因为短窗口在跟随时也会发出请求——跟随时根本不该请求；而 scrollTop === 0 时向上滚轮是读者在要历史，authority 看不见（这里不会有 scroll）。
我没有把 issue #4099 当证据。我不合入：这是会改行为的滚动重构，合入由人类决定。发这条时 hosted test 还在排队，这次审查不表示 CI 已绿。
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
| // Position, not direction: a shrinking transcript also lowers `scrollTop`. | ||
| const nearStart = (): boolean => | ||
| root.scrollTop <= Math.max(640, root.clientHeight * 2); | ||
| const onScroll = (): void => { | ||
| const nextScrollTop = root.scrollTop; | ||
| if (nextScrollTop < previousScrollTop && nearStart()) requestEarlier(); | ||
| previousScrollTop = nextScrollTop; | ||
| if (nearStart()) requestEarlier(); |
There was a problem hiding this comment.
[P1] Class ①. This calls requestEarlier() on every scroll while scrollTop is inside max(640, 2×clientHeight), including the authority's own pin writes.
On main this handler required an upward move (nextScrollTop < previousScrollTop). Dropping that check means a pinned write to the tail is enough. For any latest window shorter than about three viewports, the tail is still inside that band, so session switch → pinToTail() / notifyContentResize() → scroll → load earlier history the reader did not ask for. That is the same speculative insert the first commit of this PR existed to remove.
The Playwright specs do not assert that a session switch leaves history unloaded, and they assign scrollTop (which un-pins) before testing a top-of-scroller load.
Minimal fix: do not ask for earlier history while the reader is at the tail, or while the authority is the one writing. An upward-only check, or skipping the request when distance-to-tail is within the pin threshold, both close it.
简体中文
[P1] 可达性 ①。只要 scrollTop 落在 max(640, 2×clientHeight) 里，任何 scroll 都会 requestEarlier()，包括 authority 自己钉住时写下的那次。
main 上这里要求向上移动（nextScrollTop < previousScrollTop）。去掉这个判断后，钉住写到尾部就够触发。最新窗口短于大约三屏时，尾部仍在加载带里，于是会话切换 → pinToTail() / notifyContentResize() → scroll → 加载读者没要的更早历史。这就是这个 PR 第一笔提交要删掉的那种预取。
Playwright 没有断言会话切换不会加载更早历史，并且测试顶端加载前会先赋值 scrollTop（这会松钉）。
最小修法：读者在尾部时、或当前是 authority 在写时，不要请求更早历史。只在向上滚动时请求，或距离尾部仍在钉住阈值内就跳过，都可以关掉这条路径。
| const requestEarlier = (): void => { | ||
| if (historyLoadPendingRef.current || earlierLoadRequest.current) return; | ||
| const scrollHeight = root.scrollHeight; | ||
| const anchor = captureChatScrollAnchor(root); | ||
| const sessionId = sessionIdRef.current; | ||
| const request = {}; | ||
| earlierLoadRequest.current = request; | ||
| arrivalPin.current?.release(); | ||
| void Promise.resolve(loadEarlierRef.current?.()).catch(() => undefined).finally(() => { | ||
| window.requestAnimationFrame(() => { | ||
| if ( | ||
| earlierLoadRequest.current === request && | ||
| sessionIdRef.current === sessionId && | ||
| input.scrollRef.current === root && | ||
| root.isConnected && | ||
| !restoreChatScrollAnchor(root, anchor) | ||
| ) { | ||
| root.scrollTop += root.scrollHeight - scrollHeight; | ||
| } | ||
| if (earlierLoadRequest.current === request) earlierLoadRequest.current = null; | ||
| }); | ||
| }); | ||
| // Nothing here touches the pin. Asking for history is not a decision | ||
| // about where the viewport belongs: if the reader scrolled up to ask, | ||
| // the authority already saw that scroll and released; if the transcript | ||
| // is merely short enough that its tail is also near its start, they are | ||
| // still following it and must keep following it. |
There was a problem hiding this comment.
[P1] Class ①. This comment is false at scrollTop === 0, which is the wheel-to-load path this file documents a few lines below.
scroll does not fire when the scroller is already at zero and the reader wheels up, so the authority never releases. After 9eb824f9a this function also does not release. The load then grows the transcript under a still-pinned reader, and notifyContentResize writes scrollTop = scrollHeight — the history they asked for is scrolled off the bottom.
That is the normal gesture on a short latest window that still has older history: the whole transcript is on screen (pinned, distance to tail ≈ 0), and they wheel up for more context.
Minimal fix: treat a wheel-up at zero as the reader (releasePin() there). Do not keep the pin on a history request the reader actually made. The “short transcript must keep following” case is the other half of this P1: that request should not fire at all while they are at the tail.
简体中文
[P1] 可达性 ①。这条注释在 scrollTop === 0 时不成立，而那正是下面写的滚轮加载路径。
已经在 0 时向上滚，不会有 scroll，authority 不会松钉。9eb824f9a 之后这个函数自己也不松。历史加载后钉住仍在，notifyContentResize 把 scrollTop 写成 scrollHeight，读者刚要的更早内容被甩到下面去了。
短的最新窗口后面还有更早历史时，这是正常手势：整段都在屏上（钉住，离尾部 ≈ 0），向上滚是在要更早上下文。
最小修法：把 0 处向上滚轮当成读者（在那里 releasePin()）。读者真正要历史时不要保持钉住。「短转录必须继续跟随」是这个 P1 的另一半：人还在尾部时，那个请求根本不该发出。
…ming The authority flagged itself while it wrote and cleared the flag on the next frame, so an event that took longer than a frame to arrive came back as a reader's gesture and released the tail. CI showed exactly that, twice, with the numbers to name it: `clientHeight` was a steady 660 — no viewport shrank — while the transcript rested at `scrollTop` 944 against a `scrollHeight` of 1906. 944 is a tail, just not the current one: the write landed, the answer kept streaming, and the late event found the scroller 302px from where the tail had moved to, which is indistinguishable from a reader who scrolled up by 302. Timing cannot make that distinction, so stop asking it to. Remember the offset written, clamped as the browser stored it, and treat an event that finds the scroller still on it as the echo of that write however late it is. A reader's gesture has by definition moved `scrollTop` somewhere else. The frame flag and its `requestAnimationFrame` go with it. The previous commit's viewport observer stays — following the tail through a window resize is right on its own terms — but this, not that, is what those two failures were. Generated-by: Claude Code
zhiiw
left a comment
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head 9eb824f9 (23 files, +1059/−994). The mechanism reads clean on inspection — one writer for scrollTop, navigation as a one-shot command while unpinned, the duplicated prefetch removed. I did not find correctness issues in the code itself worth a P-level.
However, this head cannot be approved: its own evidence spec is red in CI. Run 33237483941 (test, Linux/xvfb) fails two of the six new transcript-scroll assertions, both with the same signature — the transcript settles 302px off the tail where the budget is ≤4px:
a streaming answer keeps the viewport at the taila gesture a nested scroller consumed does not release the tail
The author verified them locally and they pass there; on the hosted runner they do not. Until that gap is reconciled (either the tests are environment-fragile or the behavior differs off the author's machine), the PR's own claims are unproven on CI. Inline notes on the two failing specs.
简体中文
代码本身读完没有发现值得定级的问题，但这个 head 不能批：PR 自己新增的证据测试在 CI（Linux/xvfb）上挂了 2/6，失败签名一致——收敛后离尾部 302px（预算是 ≤4px）。作者本地是过的。这个落差没 reconciled 之前，本 PR 自己的声明在 CI 上未被证明。
| }), count); | ||
| } | ||
|
|
||
| test('a streaming answer keeps the viewport at the tail', async ({ window: page }) => { |
There was a problem hiding this comment.
[P1] This spec fails in CI on the exact head (test run 33237483941, Linux/xvfb): expect(settled.distance).toBeLessThanOrEqual(4) received 302. The transcript does not settle at the tail in that environment, so the PR's headline evidence is currently unproven outside the author's machine.
| expect(await distanceToTail(page)).toBeGreaterThan(before); | ||
| }); | ||
|
|
||
| test('a gesture a nested scroller consumed does not release the tail', async ({ |
There was a problem hiding this comment.
[P1] Same failure signature in CI on the exact head: distance 302 vs the ≤4 budget. Either the authority misses a settle path on this platform or the measurement is environment-fragile — the distinction needs answering before this spec can stand as evidence.
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
I reviewed exact head 14f894da459463ead9a2bde47d327208f49b30ce. I found no P0 or P1 issue, so I am approving it with non-blocking P2 feedback.
The latest commit closes the event-ordering defect from the preceding head: the authority now matches its clamped written offset instead of owning a whole animation-frame window. The production-state probe now preserves a reader scroll across later growth, and the real Electron dock regression passed 5/5. The earlier delayed conversation-fill problem also remains closed.
I independently reproduced both active use-chat-scroll.ts history-loading threads and did not add duplicate inline comments. I classify their observed effects as P2 rather than P1: one path performs bounded earlier-history work without reader intent, and the already-at-zero wheel path causes a recoverable viewport jump after the first page loads. Neither loses data or creates an unrecoverable state. I added one separate P2 inline for a supported transcript-growth path that still does not reach the new authority.
The refactor's simplification is genuine: it removes the two previous scroll authorities, speculative prefetch machinery, duplicate DOM/state seams, and their dedicated suites. All 258 UI tests and the Desktop production build pass locally. Hosted checks for this exact head were still running at my last read, so this approval does not claim that CI is 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
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
| // Before the window work, and synchronously: this is the observer | ||
| // callback, so the pin still writes `scrollTop` in the same frame the | ||
| // content grew and the reader never sees the tail slip. | ||
| contentResizeRef.current?.(); |
There was a problem hiding this comment.
[P2] This is now the transcript's only content-growth signal, but it observes only virtual Turn wrappers. Supported nodes rendered outside those wrappers can still grow the scroll height: an ordinary Follow Up is projected as a next_turn transient below the Turns, and the no-tail live fallback and orphan conversation item use the same outside path in chat-view.tsx. Observing the fixed-height scroll root does not report overflow-content growth. In a production-hook DOM probe, the root and Turn were observed, the inserted data-transient-message-id node was not, onContentResize fired zero times, and a pinned viewport stayed at 600 after its expected bottom became 800. Current unit tests call notifyContentResize() manually, and the E2E streaming case grows inside an observed Turn, so neither guard covers this path. Please observe the actual transcript content box (or every outside growth source) and add a pinned next_turn optimistic-message regression.
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
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
4 participants
Summary
Switching sessions moved the transcript under the reader after it had already settled.
The immediate 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 exposed the real one.
scrollTophad three writers — Astryx's lock and spring, this hook's compensation andscrollIntoView, and the browser's own anchoring — and nothing decided where the viewport should be. They coordinated through ref flags and effect ordering, so every defect in this area is two writers colliding in one commit.unlockAutoFollowwas the tell: a seam whose only purpose is to let one owner overrule another.So this PR ends with one writer.
transcript-scroll-authorityholds a single boolean:pinned→ content that grows writesscrollTop = scrollHeight!pinned→ nothing here writesscrollTop, ever"Keep the reader where they were reading" is the definition of
overflow-anchor: auto, already the initial value and free, and "the reader is dragging" is also don't touch it — both are the same instruction to this code. Navigation becomes a one-shot command that only runs while unpinned, so a command cannot race the policy. Because the authority is the only writer, every write flags itself and an unflaggedscrollevent is the reader — exact, not inferred from direction, height deltas or wheel events, each of which has more than one cause.Removed along the way, because the authority makes them redundant:
arrival-bottom-pin, the idle prefetch,chat-scroll-anchor,data-turn-windowand thefonts.readywait, fifty markdown polls and double rAF that timed its release, and three of the four injection points in the vendored Astryx patch. The patch's scroll surface is now one seam —ChatLayoutforwardingautoScrollinto theenabledoptionuseChatStreamScrollalready published.The hand-rolled compensation for content landing above the reader went too. It existed because the browser declines to anchor at
scrollTop === 0, and that hole is exactly one pixel deep: measured in Chromium, a 501px insert above the reader movesscrollTopby 501 at an offset of 1 and by nothing at 0. Raising the offset to 1 before asking for history hands the case to native anchoring, which was already doing the work at every other offset the load can fire from.The authority is also unconditional: every
ChatSurfaceLayoutprovides one, so nothing below carries a second behaviour for its absence, andautoScroll— whichscrollOwnerdecides and the layout spread last — is off the public props. Deduplicating an earlier-history request moved toapp-shell, where the request originates, so the hook no longer tracks a load it does not own.23 files changed, 985 insertions, 994 deletions.
Refs #4099
Verification
Layout shift, 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.Both the table and the capture were measured at
31539bc66, where the prefetch was removed and Astryx still owned scrolling. They are not re-measured at the tip: the single authority only removes writers ofscrollTop, so it cannot reintroduce the shift, and what it changes beyond that is scroll position, which CLS cannot see. The Playwright assertions below are the tip's evidence.CLS was the wrong instrument for the rest of it: it measures layout shift, and every remaining defect was a wrong scroll position, which it cannot see. Those are now covered by six Playwright assertions against a real Electron window (
apps/desktop/e2e/transcript-scroll.spec.ts), each asserting where an element ended up rather than a pixel delta:mouse.wheelover a genuinely overflowing nested container, with a guard assertion proving the wheel was consumedpointer-events/opacity, sincetoBeVisibleis true atopacity: 0)Run locally:
transcript-scroll6 passed,prompt-rail7 passed,transcript-measure1 passed,@maka/uitranscript-scroll-authority4 passed, plusnpm run format:checkandtsc --noEmitforpackages/uiand the desktop renderer. Not run: the full repository suite.What review found
Two rounds of adversarial review found eleven defects. Eight were the three-writer problem, and the collapse to a single boolean makes them structurally inexpressible rather than fixed one at a time:
isVisibleisauthority.getSnapshot().awayFromTailand itsonClickisauthority.pinToTail()— read and write on one object, with no second source that can go stale.pinToTail()consults no precondition, so no state exists that could veto it.pinToTail(), a jump isreleasePin(). They write the same variable instead of overruling each other, and the later call wins because the call order says so.deltaY < 0 && animatingRef.current), and gestures bubble — a wheel a nested scroller consumed passed through the transcript anyway. CDP also showedanimatingRefis true at rest, so that conjunct never constrained anything. The only release signal now is an unflaggedscrollevent on the root.scrolldoes not bubble, and nowheelortouchlistener sits on the scroll path (use-chat-scroll's wheel listener serves only history loading atscrollTop === 0and never touches the pin). The gesture cannot physically reach the authority.Review focus
Two behaviours change deliberately:
Two upstream defects surfaced while adopting the Astryx hook, both filed: gesture release is dead under
prefers-reduced-motion(facebook/astryx#5662), andjumpToBottomdoes not cancel its in-flight spring (facebook/astryx#5663). Neither is reachable now that Astryx's scroll layer is off for the transcript.AI use
Select exactly one:
Tool(s) and scope: Claude Code — root-cause investigation, the CDP measurement harness, the code changes, the E2E spec, and this description. Four independent adversarial review agents produced the findings above; each was verified against source, and the behaviours were then measured against a running build. The contributor of record reviewed the final diff and owns the merge decision.
Checklist
Does this PR entail a change in behavior?