Repository navigation
fix(site): keep agent chat scroll on tool toggles and log streams - #29354
Conversation
1c39899 to
d91e6d4
Compare
794de60 to
b22535d
Compare
b22535d to
7f24fe8
Compare
The agent chat transcript scrolls through `@shadcn/react`'s MessageScroller, and its scroll defects have to be fixed in the package. The package ships only minified dist, so the fix would be an unreviewable patch against minified code, and 0.3.1 still has the identical defect, so an upgrade buys nothing. This vendors the readable upstream source (`@shadcn/react@0.3.0`, commit `b1c580c63`, MIT) under `site/src/vendor/` and drops the npm dependency. It is a pure move with no behavior change; the fix itself follows in #29354. Each file header pins the upstream tree, and any deviation from upstream must be marked `LOCAL CHANGE`.
7f24fe8 to
df2af46
Compare
99fd41a to
7dafdc3
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dafdc3880
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7dafdc3 to
a741dd3
Compare
The vendored message scroller cannot tell a tool call toggle from new output. A toggle can scroll the transcript to the bottom, or move the anchored prompt out of view. ToolCall now reports the toggle through a Viewport context before it changes size. The scroller then stops following until the next user scroll or scroll command. While a prompt is anchored, the scroller hands off to following only near the live edge, so a large block of output does not move the view to the bottom. When a collapse clamps the view at the bottom, browser scroll anchoring can scroll back down as output arrives. The scroller now scrolls 1px and back to make the browser pick a new anchor.
a741dd3 to
872eb9c
Compare
The build log and agent startup log boxes in create_workspace and start_workspace rows used scrollIntoView to show new lines. That also scrolled the chat transcript, so the view jumped while a workspace built and could stop following the reply. The boxes now scroll only themselves.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 4 | Last posted: Round 4, 40 findings (2 P2, 22 P3, 8 P4, 8 Nit), APPROVE. Review Finding inventoryFinding inventory: PR #29354Findings
Contested and acknowledgedCRF-2 (P3, use-message-scroller-controller.ts:674) - browser-anchor reset paths untested
CRF-3 (P3, WorkspaceAgentLogSection.tsx:80) - agent log box self-scroll untested
CRF-4 (P3, WorkspaceBuildLogSection.tsx:108) - build log box self-scroll untested
CRF-8 (P3, use-message-scroller-controller.ts:181) - non-gesture scroll to bottom never resumes following
Round 2 dispute check-in (Netero)
Round 2 dispute check-in (panel)
CRF-29 (P3, PR description) - description omits behavior changes
CRF-32 (P4, WorkspaceBuildLogSection.tsx:104) - duplicated scroll-box effect
CRF-41 (P3, use-message-scroller-controller.ts:494) - same-count new-turn clear untested
CRF-45 (P4, ToolCall.test.tsx:241) - collapse-while-following test asserts too little
CRF-46 (P3, use-message-scroller-controller.ts:703) - collapse while following stops following
CRF-47 (P3, WorkspaceAgentLogSection.test.tsx:187) - agent log tests pass with no render
Round 3 dispute check-in
CRF-49 (P4, ToolCall.test.tsx:257) - handoff-band tests live away from scroller code
CRF-34 (OOS) - human acceptance
Round 4 dispute check-in
Round logRound 1Netero first pass: 1 P2, 4 P3, 2 Note. No P0, panel proceeds. Reviewed against 515bda9..9fdb72b.
Round 2Churn guard: PROCEED. 25 author fixed in 0be0099, 6 contested (CRF-2, 3, 4, 8, 29, 32), 0 silent.
Round 3Churn guard: PROCEED. CRF-2, 42, 43, 44 author fixed in 0cb57d6; CRF-41, 45, 46, 47 contested. Author accepted CRF-34 (OOS) gap in an issue comment; it still needs a human decision.
Round 4Churn guard: PROCEED. CRF-48 author fixed in d1c7408 (content observer covered; viewport observer reset still untested, author says fixed-height test viewport cannot model it). CRF-49 contested. CRF-50 note addressed in the ref comment. CRF-34 accepted by maintainer. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This PR adds a toggle latch to the vendored message scroller and makes the workspace log boxes scroll only themselves. Findings: 2 P2, 15 P3, 6 P4, 9 Nit.
The two P2s are missing tests: removing the latch release in scroll commands, or forcing the new handoff band to true or false, leaves every test passing.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/vendor/message-scroller/use-message-scroller-controller.ts:534: the PR refs shadcn-ui/ui#11175, but a status row that consumes a small spacer still ends within 64px of the live edge, so the handoff fires before reply text exists and the reply then pulls the prompt off the reading line.
Notes:
components.tsx:147: the overlay-scrollbar branch ofisScrollbarPress(offsetWidth - OVERLAY_SCROLLBAR_WIDTH) has no test; the test viewport mocks a classic scrollbar, so macOS overlay scrollbars run untested code.WorkspaceAgentLogSection.tsx:82: after the call completes, the agent log box now scrolls to its bottom on every batch, not only the first, so a reader who scrolls up inside the box is pulled back down.WorkspaceBuildLogSection.tsx:102: "scrollIntoView also scrolls the chat transcript" is written in four places (both sections and both tests).
In reply to IC_kwDOGkVX1s8AAAABZ-tC-g:
P3 [CRF-29] The PR description leaves out three scroller behavior changes that apply to every MessageScroller user. (Leorio)
A scrollbar press now counts as user scroll intent (components.tsx:200), Space on a <button> no longer does (components.tsx:219), and the spacer ref setter resets spacerHeightRef when Content remounts (use-message-scroller-controller.ts:757); the reasons are only in test names. Add a sentence for each, and say whether the Refs shadcn-ui/ui#... links are upstream bug reports or fixes, so the next re-sync knows whether upstream replaces these local changes.
🤖
In reply to IC_kwDOGkVX1s8AAAABZ-tC-g:
Nit [CRF-31] The PR title is 80 characters; CONTRIBUTING.md:291-292 sets a 72-character maximum, and the squash merge uses the title as the commit subject. (Leorio)
For example: fix(site): keep agent chat scroll on tool toggles and log streams (65 characters).
🤖
🤖 This review was automatically generated with Coder Agents.
The anchored handoff again needs upstream's spacer transition, so a turn that starts a little taller than the view no longer jumps to the end. A toggle above the anchored turn now releases the anchor, so the clicked row stays in view. Scroll commands and new turns clear the follow flag where they start, and the toggle's MutationObserver disconnects after its commit. Tests now cover the handoff band, the new-turn release, and both fixes.
|
Replies to the items in the review body:
|
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 checked 0be009961. 25 of the 32 round 1 findings are fixed, and the fixes hold up under mutation runs. Of the six you contested, I accept five: the FE10 defense for both log-box tests, the find-in-page and autoscroll resume gap, the spacer reset not being in the description, and the duplicated log-box effect. One I re-raise: the browser scroll-anchor reset still has no test that fails when nudgeScroll does nothing (reply on that thread). New in this round: 4 P3 and 2 P4.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/vendor/message-scroller/use-message-scroller-controller.ts:554: a status row that uses up a small tail spacer still ends within the 64px band, so the handoff fires before any reply text exists. This is the second case in shadcn-ui/ui#11175. The author has declined to fix it in this PR, so a human needs to file a ticket or accept the gap.
🤖 This review was automatically generated with Coder Agents.
The ToolCall test mock now keeps a shrink's clamp only until the next write, as Chromium does, so the tests fail without the scroll nudge. The streaming agent log story now overflows its box, so its screenshot shows the box scrolled to the newest line. Also reword the handoff band, layout intent and scroll anchor comments.
|
Out-of-scope item in the round 2 review (shadcn-ui/ui#11175): I accept this gap for this PR. The base behaves the same, and no local threshold can tell a status row from reply text. The fix needs an upstream way to mark rows as temporary.
|
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3 checked 0cb57d6bb. The fixes for the scroll-anchor nudge test, the handoff band comment, the hook doc comment, and the rounding-tolerance comment all hold, and the reviewers who raised the four contested findings accept your defenses. New in this round: 1 P3 and 1 P4.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/vendor/message-scroller/use-message-scroller-controller.ts:554: a status row that uses up a small tail spacer ends within the handoff band, so the handoff runs before any reply text exists (the second case in shadcn-ui/ui#11175). The author has accepted this gap for this PR. A maintainer still needs to file a ticket or accept it.
Notes:
use-message-scroller-commands.ts:161:scrollToStart,scrollToEnd,scrollToMessage, both new-turn branches, anduserScrollIntenteach clearfollowSuppressedRefthemselves. No comment says that every new scroll command must do the same. An upstream resync that adds a command, or a new caller ofscrollToElement, would keep the hold through that command. The ref comment inuse-message-scroller-refs.ts:61-63could state the rule.
🤖 This review was automatically generated with Coder Agents.
A removed row can clamp the view after a collapse. The new test reports that resize only to the content observer, as a browser does, so it fails without that observer's scroll anchor reset. Also rename a handoff band test and state when followSuppressedRef must be cleared.
|
Replies to the round 3 review body:
|
|
/coder-agents-review |
There was a problem hiding this comment.
Round 4 checked d1c74087e. The new test fails when the Content ResizeObserver's anchor reset is removed, and the followSuppressedRef comment now matches every place that clears the flag. The reviewer who raised the test-placement finding accepts your reason: the vendor folder is excluded from Biome, oxlint, and the dash check. No findings remain open.
One untested path remains, and both the author and the reviewer have accepted it: the Viewport ResizeObserver's resetBrowserScrollAnchor at components.tsx:252. If it regresses, then after a toggle and a viewport resize, new output scrolls the view down until the next gesture or turn.
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c74087e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
At the bottom, a wheel down, a scroll key, or a scrollbar press fires no scroll event, so nothing re-armed following after a collapse or a scrollbar press. These gestures now sync the scroll state themselves.
A scrollbar press at the bottom left following off if output landed before the release, because the view was no longer at the end. The release now catches up unless the press moved the view up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ee74dbe2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A log box jumped to its end on every new batch, even after the reader scrolled up in it. Agent logs can keep arriving after the tool call completes. Both boxes now follow new lines only while at their end.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
coder/site/src/vendor/message-scroller/components.tsx
Lines 247 to 249 in ff9519a
When a disclosure collapse clamps the viewport to the live edge, a swipe toward the end on a touch device can emit touchmove but no scroll because the viewport is already at the bottom. This handler clears followSuppressedRef through userScrollIntent() but never reconciles the scroll state, leaving the mode as free-scrolling; the next streamed chunk then grows below the viewport without being followed. Wheel and toward-end key handlers explicitly cover this stationary case, so touch gestures need equivalent reconciliation.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(watchAgentLogs).toHaveBeenCalledTimes(1); | ||
| expect(socketServer()?.isConnectionOpen).toBe(true); | ||
| expect(scrollIntoView).toHaveBeenCalledTimes(1); | ||
| expect(scrollIntoView).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Cover the inner log follow and preservation behavior
The fresh followLogEnd fix adds two important behaviors, following late lines at the end and preserving the position after the reader scrolls up, but this modified test only proves that the old global scrollIntoView command is absent. It never overflows or scrolls the inner viewport and would still pass if followLogEnd always scrolled or never scrolled; add behavior coverage through the consuming log section for both branches.
AGENTS.md reference: site/AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed with unit tests of followLogEnd in utils.test.ts, one per branch. Each fails when the condition is forced the other way. Its parameter now takes only the three properties it reads, so the tests pass a plain object. We don't test this through the rendered section: that needs a scrollTop assertion, and our frontend rules (FE1, FE10) don't allow DOM-geometry assertions in component tests.
Generated with Coder Agents
At the bottom, a swipe toward the end fires no scroll event, so it did not resume following after a collapse. It now syncs the scroll state, as a wheel down does. Also cover both followLogEnd branches with unit tests and drop two tests that other tests already cover.
|
Re the touch finding in this review: fixed.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Help keep position better in Coder Agents chat transcript during active scrolling
LLM slop description
In the agent chat, a tool call that expands or collapses can make the transcript jump. The clicked row can scroll out of view, or the next streamed output can pull the view to the bottom. The scroller cannot tell a layout change from new output, so it reacts to a toggle as if the reply grew.
ToolCallnow tells the scroller before it toggles. The scroller then holds the view in place until the reader scrolls, a scroll command runs, or a new turn starts. The changes areLOCAL CHANGEedits to the scroller source that #29473 vendored.The
create_workspaceandstart_workspacerows also made the transcript jump while the workspace built. Their log boxes usedscrollIntoViewto show new lines, and that also scrolls the transcript. At the end of the build, the view could stop following the reply. Now each box scrolls only itself, and only while the reader is at its end, so a reader who scrolls up in the box keeps their place. Other pages use the scroll in the sharedWorkspaceBuildLogscomponent, so that component does not change.Four behavior changes are intentional. While a turn is anchored, a large block of output no longer pulls the view to the bottom. The reader stays on the turn and can use the jump-to-bottom button. Collapsing a tool while the view follows the reply also stops following, so the rows near the pointer stay put for the next click. A press on the scrollbar now counts as a scroll, so it releases the held view like a wheel or key scroll. Space on a button does not count as a scroll, because Space activates the button.
Related upstream bug reports, all open with no upstream fix: shadcn-ui/ui#11175, shadcn-ui/ui#11577, shadcn-ui/ui#11224
Note
Generated with Coder Agents
W6-start-reload-expand.mp4
W5-start-wheel-up.mp4
W4-start-no-input.mp4
W3-create-reload-expand.mp4
W2-create-wheel-up.mp4
W1-create-no-input.mp4
T11-next-turn-follows-after-toggle.mp4
T10-normal-turn-follows.mp4
T9-scrollbar-drag-after-toggle.mp4
T8-block-lands-while-anchored.mp4
T7-expand-previous-turn-tool.mp4
T6-mouse-collapse-while-anchored.mp4
T5-collapse-while-following.mp4
T4-space-collapse-while-anchored.mp4
T3-enter-while-following.mp4
T2-expand-while-following.mp4
T1-expand-while-anchored.mp4