Repository navigation
fix(chat): follow measured rows without a competing glide - #2624
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Measured row growth and follow validation src/components/ChatTimeline.tsx, scripts/chat-virtual-browser-qa.ts, changelog.d/chat-follow-measured-end.md |
ChatTimeline uses non-animated end scrolling after item-size changes when its existing follow and manual-layout checks allow it. Browser QA checks row growth, upward-wheel behavior, and scroll lag during 70 streaming updates. The changelog records the change. |
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix
Possibly related issues
- Follow-up to #2564: a wheel up during a long chat's first-open jump to the end is undone #2594 — Reports automatic scrolling that overrides an upward-wheel action during long-chat opening. This PR changes the same auto-follow path but does not change the first-open jump logic, so the issue may remain.
Possibly related PRs
- devswha/ovis#2564 — Both changes adjust Chat end-follow behavior during layout changes.
- devswha/ovis#1937 — Both changes use the
manualLayoutandfollowingstate to govern Chat follow behavior. - devswha/ovis#2538 — Both changes prevent a competing scroll from interfering with following the current end.
Suggested reviewers: devswha
Merge Risk: 
d03b9
-
Adjusting an open control can pull the reader back to the bottom. The issue is narrow and recoverable.
Pre-merge checks: 
-
Check name Status Explanation Title check The title "fix(chat): follow measured rows without a competing glide" matches the main change. ChatTimeline now calls scrollToEnd with animated false after item-size changes while following, which rem… Description check The description follows the repository template. The Change section explains the problem (a smooth DOM scroll chasing an outdated endpoint during row growth), the resulting behavior, and the guards th… Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(chat): follow measured rows without a competing glide" matches the main change. ChatTimeline now calls scrollToEnd with animated false after item-size changes while following, which removes the competing smooth scroll. The title is specific, concise, and uses the conventional prefix. It does not cover the test changes, but that is expected.
Full details: Description check
-
Explanation
The description follows the repository template. The Change section explains the problem (a smooth DOM scroll chasing an outdated endpoint during row growth), the resulting behavior, and the guards that stay in place. The Validation section lists the commands run with their results, the failing unit lane, and the checks skipped. The template asks for a linked issue for features, but this is a fix. The description does not mention the changelog.d entry, which the template references, although the diff adds one. The description is complete and specific enough to review.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Closing a disclosure while following now also drops a list scroll queued before the click, so the deferred follow can't land after the fold. The continuous-content QA logs its lag frames instead of bounding them (main and this fix both land near two per change), comments describe the remaining call, and the changelog says following no longer glides.
|
Thanks @odfalik, nice fix; the before/after recordings made it easy to check. Merge-pass review (omo and Codex Terra xhigh): the change is right, the new regression fails on main's code (p95 gap 119 px) and passes here, and a reader still escapes following with an upward wheel while rows grow (more reliably than with the old glide). I pushed one small commit on top of your branch (no force):
|
|
Thanks for the follow-up commit. Dropping the queued scroll on fold covers the case CodeRabbit raised, and the logged lag frames are a fair read of the measurements. Looks good from my side. |
|
Thanks @odfalik for the fix, the regression and the before/after recordings. While a chat row keeps growing, following now uses LegendList's deferred, non-animated Merge-pass review: omo approved, Codex Terra xhigh approved the final head. A reader still escapes following with an upward wheel while rows grow (reproduced at both widths, more reliably than with the old glide). The merge pass added one commit: a click that folds a disclosure while following now drops a scroll the list had already queued, so the deferred follow can't land after the fold; and the QA logs instead of bounding its lag-frame count (both branches land near the bound, so it separated nothing). Left as is (LOW / not reproduced): no browser test yet clicks a disclosure while a follow is queued; a non-animated snap could in theory land between an upward touch gesture's start and its first scroll event (main's idle follow already uses the same call; the synthetic touch gesture moved neither branch, so a real-phone check is still to do); the scrollend-settle effect and CodeRabbit's check shows skipping on this head; the merge pass and both reviewers reviewed it. |
) ## Change Four browser scripts failed now and then in "Browser regression (other scripts)" on main (run 41558343287 on c0cdc8e, and later runs). The take-over failure was a product race, fixed in #2630. This PR fixes the other three, plus the relay-media failure seen on bef2be1. Test-only: no product code changes. ### `chat-local-send-follow-demo-regression.ts:95`: "the reader moves away" `wheelUp` waited for each wheel notch to lower `scrollTop` by more than 20 px. A write trace on the page showed every notch moving the view 120 px. Then, as a row above the view was measured, LegendList's `ScrollAdjust` called `scrollBy(+103)` to keep the reader's rows in place, with `scrollHeight` growing by the same 103 px. That undid the notch in `scrollTop`, but the distance from the end stayed exactly 600 px before and after. The reader had moved away; the wait could not see it. The wait now measures the distance from the end, which that adjustment leaves alone. Not a #2624 regression. An A/B of the unmodified script, alternating commits under `dori heavy`, gave passes/runs of: 05c9133 (before #2624) 5/7, 9ef34f8 (#2624) 5/6, main 2/6. The step fails before #2624 too; #2624 changes when rows get measured, so it lands more often. No `scrollToEnd` or follow write appears in the trace. ### `file-viewer-regression.ts`: "video not visible" The CI failure came right after `setFileOpen("panel")` reloads the page: `videoLink.click()` was a pointer click on a transcript link while the reloaded chat followed to its end, so the click landed on nothing and no popup opened (0 "locator resolved" lines on CI and locally). The script's own header comment describes this trap, and its other steps already use `activateInTranscript`. Under load, three more steps hit the same trap: after a `goto` (big code), after the panel closes (the extensionless and unknown-text loop, which keeps the link focused so the focus-return assertion still holds), and after a reload and Back at 390 px. The click on the framed page's table-of-contents link is dispatched to the link: the frame's own navigation is under test, not where the panel lays the frame out. ### `files-prompt-regression.ts:71`: Escape left Files open after a pane switch Not reproduced in about 435 local attempts (15 per run, fresh pages, 6x CPU throttle, load up to 92), and nothing found by reading the code. The Escape check stays: it is the only place a script checks Escape closing Files. When it fails now, the script writes what decided it to the log and to `files-<page>-escape-diagnostics.json` in the browser-evidence artifact: whether a window keydown probe saw the key, the focused element, the modal and dialog stack, whether FilesDialog stood down (file or diff over it, native modal), and any open picker. Tracked as #2631. ### `relay-media-browser-qa.ts:44`: "Timed out: copied-link clipboard event" Seen inside `relay-e2e-regression.ts` on bef2be1 (run 41568570526). The stubbed `writeText` always resolves, so the click never reached Copy link. `copy.click({ force: true })` is aimed at where the button was, under a frozen browser clock, while the viewer may still move it. The click is now dispatched to the button itself; the clipboard listener is still subscribed before it. ## Validation All runs went through `dori heavy`; host load ranged from about 4 to 92 (70 CPU hogs for part of it). - chat-local-send-follow: 22/22 consecutive. - file-viewer: 20 consecutive and counting on the final script (`bun run check run`, isolated herdr session). Earlier rounds under load caught the other lost-click steps fixed here. - files-prompt: 22/22 consecutive. A forced failure (key withheld) wrote the diagnostics JSON and screenshot. The review's try/catch, which keeps the original timeout as the reported error, came after those runs; it runs only when the check fails. - relay-media (fixed): 22/22 consecutive; the unmodified script passed 10/10 locally, so CI is the only evidence for that one. - `bun run check fast`: success on 6069b0a. An earlier run caught a regression in this branch: `ci-browser.test.ts` forbids `??` on evidence dirs (#1554). Fixed with `||`. - omo code reviewer: APPROVE, LOW notes only. Applied: the diagnostics never replace the original error. Not changed: browser-other uploads PNGs only on failure; the 390 px reopen dispatches directly because `activateInTranscript` waits for a frame that section lacks; the `wheelUp` callers assert the distance afterwards. - Skipped: full check, Windows VM. No changelog fragment: test-only.
|
Thank you @odfalik for this change, and for the thorough evidence and recordings in the description. The lag you measured was real, and your new continuous-content check is what made the problem below visible. We're reverting it for now in #2647, and an improved version will land on top of it. On GitHub-hosted runners, which fork PRs use and whose frames are slower than our own machine's, The same follow also stops following in the script's fold-together step, where a row above folds as the last row grows. LegendList's own adjustment, and then a A follow that lands in the same frame, with your measured gain kept, is being worked on for the re-land, and it will reuse your check. Thanks again. |
…2647) ## Change Reverts #2624 (9ef34f8). An improved version will land again on top of this. #2624 follows a growing chat row with LegendList's `scrollToEnd`, which lands one frame after each measurement. Each streamed change is therefore two frames behind: the frame it is appended in, and the next one. On a fast machine that passes. On GitHub-hosted runners the frames are slower, so the next change often lands before the reader has caught up. `scripts/chat-virtual-browser-qa.ts:717` ("continuous content left the reader behind for N consecutive animation frames") then failed on every attempt of fork PRs: #2616 twice (4 frames), and #2636 (6 frames). In each of those runs every change lagged exactly 2 frames (140 lag frames for 70 changes). The same follow also stops following in the script's fold-together step. A row above the view folds as the last row grows. LegendList's own adjustment, followed by a `scrollToEnd` aimed at an end it computed before the rows moved, nets out as a move up with no shrink seen. ChatView then reads that as the reader leaving. The glide it replaced did not do this. Reproduced locally with the unmodified script, alternating commits under `dori heavy`: | Profile | 05c9133 (before #2624) | 9ef34f8 (#2624) | main | |---|---|---|---| | 2 cores, 2 CPU hogs, 8 rounds | 0/8 (p95 bound, the old glide's known lag) | 3/6 valid (fold-together 2, line 717 once) | 3/8 (fold-together 3, line 717 twice) | | 2 cores, 2x CPU throttle (CDP) | not run | not run | 0/4 (line 717, 4-10 frames) | The 05c9133 column shows why #2624 was written: the old glide fails the p95 bound that #2624 added. That bound goes away with this revert, together with the rest of #2624's script changes. ## Validation - `scripts/chat-virtual-browser-qa.ts` (as reverted): 1 consecutive passes so far (the run continues to 22; I'll update this line) on 2 cores with 2 CPU hogs and 2x CDP CPU throttle, `UI_EVIDENCE_DIR` set as CI does, under `dori heavy`. The fold steps passed in every run. - `bun run check fast`: success on a5c4e5b8 (same tree as this head; only the commit message changed). - omo code reviewer: APPROVE. The revert is exact: `git diff` against 9ef34f8~1 is empty for the reverted files. Nothing merged after #2624 depends on it, and the fragment was never released. Its LOW is applied: the commit message no longer claims grow-above. - Skipped: full check, Windows VM. No changelog fragment: #2624's own fragment was not released yet, and this revert removes it.
…and) #2624 followed a growing chat row with LegendList's scrollToEnd, which lands a frame or more after the measurement: each streamed change stayed two frames behind, and on a slow runner the next change came first (#2647 reverted it). The follow now runs in a microtask queued after the list's own, once per batch of measurements, and puts the view at the list's measured end (its content length, not the scroller's height, which the list pads for a frame while it holds rows in place). A scroll the chat has not heard yet is read first, so a reader who left the end is not pulled back. The glide, its scrollend settle and followGlides go: nothing glides any more. While the list pads its content for a frame (its ScrollAdjust room), neither this follow nor ChatView's short-landing fix aims at the scroller's end, which lies past the list's there. #2624's continuous-content check is back, and the QA script takes CPU_THROTTLE (Chrome CPU throttling) to run it as a slow runner does. It now also bounds frames painted behind, follows a stream React renders, and opens a long chat while the agent works, with and without reduced motion. Co-authored-by: Oded Falik <33235305+odfalik@users.noreply.github.com>
…and) #2624 followed a growing chat row with LegendList's scrollToEnd, which lands a frame or more after the measurement: each streamed change stayed two frames behind, and on a slow runner the next change came first (#2647 reverted it). The follow now runs in a microtask queued after the list's own, once per batch of measurements, and puts the view at the list's measured end (its content length, not the scroller's height, which the list pads for a frame while it holds rows in place). A scroll the chat has not heard yet is read first, so a reader who left the end is not pulled back. The glide, its scrollend settle and followGlides go: nothing glides any more. While the list pads its content for a frame (its ScrollAdjust room), neither this follow nor ChatView's short-landing fix aims at the scroller's end, which lies past the list's there. #2624's continuous-content check is back, and the QA script takes CPU_THROTTLE (Chrome CPU throttling) to run it as a slow runner does. It now also bounds frames painted behind, follows a stream React renders, and opens a long chat while the agent works, with and without reduced motion. Co-authored-by: Oded Falik <33235305+odfalik@users.noreply.github.com>
…and) (#2655) ## Change Re-lands @odfalik's #2624 (reverted in #2647) in a form that holds on slow machines. #2624 replaced the chat's smooth follow glide with LegendList's `scrollToEnd` whenever a row's measured size changed. That call lands a frame or more after the measurement, so each streamed change stayed two frames behind. On GitHub-hosted runners the next change often came first, and `scripts/chat-virtual-browser-qa.ts` failed there (#2616, #2636). Its fold-together case also stopped following. This PR follows a measured row before the frame paints: - **Same-frame follow.** `ChatTimeline` queues its follow in a microtask after LegendList's, once per batch of measurements. LegendList sets its total before it calls `onItemSizeChanged`, and React draws that total in the microtask it queued then. The follow then puts the view at the end. - **Where "the end" is.** That is the scroller's end, except while LegendList pads the content for a frame to hold rows in place (its `ScrollAdjust` sets an inline `padding-bottom`). During that frame the scroller's end lies past the list's, and following it is what broke the earlier same-frame attempt and fold-together. `listEndPadded` detects it. Outside `anchoredEndSpace`, which the chat doesn't use, that inline padding is the only one LegendList sets. ChatView's short-landing fix (`stoppedShort`) skips that frame too. - **A reader who left is not pulled back.** Before moving the view, the follow reads any scroll the chat hasn't heard yet, exactly as its scroll event would, and re-checks that the reader is still following. "Heard" starts at the position the list attached with. Before that change, the first follow in a short chat called the chat's scroll handler with no scroll at all, and at the top that handler loads the earlier page. CI caught it in `scripts/chat-history-browser-qa.ts`: the Earlier messages button had already been used. - **No glide.** The glide, its `scrollend` settle and `followGlides` are removed, since nothing glides any more. The changelog says so. - **Kept from the merge pass on #2624.** Closing a disclosure while following drops a scroll the list already queued. Like the Codex and Claude apps, the chat holds the end of a streaming answer without animating to it. I'm describing those apps from use; I didn't re-measure them for this PR. ### QA script `scripts/chat-virtual-browser-qa.ts`: - **`CPU_THROTTLE`.** Takes Chrome CPU throttling (`CPU_THROTTLE=2`) to run as a slow runner does. - **Continuous-content check.** #2624's check is back, with two changes: - **What the reader sees is measured, not the moment after an append.** #2624 sampled the gap right after each append, before any layout. Every frame that carries a change counted as behind, whatever was painted. On a runner slow enough that several frames in a row take a change's 110 ms each, that run grows with the machine, not the follow. In one 2x run here, four such frames in a row gave a pre-layout run of 4 while 0 frames were painted behind. That pre-layout run is still logged. Two checks now look at what is painted: - **Painted behind:** at most 10% of changes. A ResizeObserver on the streamed content, created after the list's own, runs in each frame the row grew: after layout, after the list's measurement and the chat's follow, before that frame paints. Callbacks run in creation order with a microtask checkpoint between them ([ResizeObserver](https://drafts.csswg.org/resize-observer/#resize-observer-interface), [HTML](https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script)). - **Consecutive frames behind:** `<= 2`, unchanged, read as each next frame starts. The lane lead approved this change on the condition that it still fails on main and on #2624's code at 2x. The controls below show it does. - **A held-up frame lands every change that came due meanwhile at once**, as a stream backlog does. Before, the fixed 110 ms grid appended one overdue change per frame after a stall. - **Rendered stream.** A new case grows the last answer through the chat's own conversation reads. React renders each paragraph, and the gap is read in the frame the row grew, after the list measured it. A ResizeObserver made after the list's own runs at that point. - **First open.** The long-chat first-open case now also runs with the agent working, with and without reduced motion. Main misses the end by 1,117 px in one frame at 1440 while working. - **Follow cases no longer require a glide in progress.** The old `during > 1` check is replaced by #2624's "the grown last row was not followed". - **Wheel-up cleanup.** Taking the test's blocks out can shrink the transcript under a reader who went away onto its end. That counts as the end reached, so the cleanup clicks New messages only while it is still shown. The case's assertions run before this step and are unchanged. ## Evidence All numbers are from desktop Chromium at phone and desktop widths, not iOS Safari. | ChatView QA fixture | main p95 gap | #2624 p95 gap | this PR p95 gap | |---|---:|---:|---:| | 1440×900 | 119 px | 41 px | 22 px | | 390×844 | 120 px | 96 px | 96 px | - **Frames painted behind, the same script at 2 cores + 2 CPU hogs + `CPU_THROTTLE=2`:** | Code | 1440 painted behind | 390 painted behind | p95 1440/390 | Result | |---|---:|---:|---:|---| | this PR | 0 of 70 | 0 of 70 | 22 / 96 px | pass | | #2624 (9ef34f8) | 69 of 70 | not reached | 41 px / not reached | fails painted behind, then fold-together at 390 before its stream | | main | 70 of 70 | 70 of 70 | 100 / 194 px | fails all three bounds | | this PR with its follow moved to `setTimeout(0)`, after paint | 70 of 70 | 70 of 70 | 22 / 96 px | fails painted behind (read as the next frame starts, it shows 0) | - **Rendered stream.** 0 of 38 changes behind at both widths. - **Failing-first.** Main with this script fails both unthrottled and on 2 cores at 2x CPU throttle: continuous content, the rendered stream, and first open while working. - **Reader escape.** An upward wheel and a touch drag both stop following while rows stream, at 1440 and 390, every run (probe). On main, one 390 touch drag didn't escape. - **#2650, drag selection while streaming.** The selection drifts the same on main and on this PR (about 550–790 px moved, probe). This PR doesn't make it worse; that issue stays open. ## Validation - `bun scripts/chat-virtual-browser-qa.ts`, on 2 cores with 2 CPU hogs and `CPU_THROTTLE=2` under `dori heavy`, with `UI_EVIDENCE_DIR` set as CI does: **20 of 20 consecutive passes on this head** (61e231b). Every run painted 0 of 70 changes behind, at both widths. Before the last fix (a short chat loading its earlier page by itself), the previous head had 18 of 18. - `bun scripts/chat-virtual-browser-qa.ts` unthrottled: pass. - **The other 18 chat browser scripts** (every `scripts/chat-*-browser-qa.ts` and `chat-*-regression.ts`), on 2 cores at 2x: 17 pass. `chat-browser-qa.ts` fails the same way on main, with and without throttling (#2654). CI doesn't run that script. - `bun run check fast`: pass (10,792 unit tests, typecheck, build). - Reviews: omo and Codex Terra (gpt-6.1-terra, xhigh) reviewed the change and each of three rounds of fixes; both approve the final head. Fixed from review: painted lag unasserted, no React-rendered stream, no backstop for a long chat opened while working (also under reduced motion), an unused `during` value, a flaky wheel-up cleanup, and painted sampling that a follow landing between frames could pass. Left as notes: - `listEndPadded` reads a LegendList internal (inline padding from `ScrollAdjust`; `anchoredEndSpace` is unused here). - Chromium is the only engine checked. - As on main, an automatic move that reaches the scroll handler near the top can still ask for the earlier page. This PR no longer makes one without a move. - CI on GitHub-hosted runners, dispatched with `workflow_dispatch` because a same-repo PR runs on self-hosted ones: [run 41627972491](https://github.com/devswha/ovis/actions/runs/41627972491) on this head passed all 10 jobs (ubuntu-24.04, windows-latest, macos-15). That includes `chat-virtual-browser-qa.ts` (129.5 s) and `chat-history-browser-qa.ts` (13.5 s). - Not run here: the full integration suite, native Windows, and a real iOS device. No HTTP/WS contract, dependency or stored-format change. Co-authored with @odfalik: his #2624 regression check and disclosure fix are reused. Co-authored-by: Oded Falik <33235305+odfalik@users.noreply.github.com>
Change
While a chat row keeps growing,
onItemSizeChangedstarts a smooth DOM scroll before LegendList finishes measuring the list. That scroll can chase an outdated endpoint and leave the newest text behind.Use LegendList's deferred, non-animated
scrollToEndfor this callback instead. The existing following and manual-layout guards stay in place. Reader navigation and other follow animations are unchanged.The browser regression appends 70 ordinary paragraphs to a rendered row, 110ms apart, without waiting for each change to settle. It checks the gap from the bottom, consecutive frames behind, and total frames behind. The older growth/folding check no longer requires an animation to exist when following reaches the end immediately.
Evidence
Before is upstream main
05c91332. After is this fix on the same base. Both demo builds use the same generic conversation and paragraph-growth sequence. These recordings exercise controlled content-size changes in the real chat UI. Provider and network streaming are outside their scope. All chats and workspace labels are fictional demo data.The generic-growth QA fixture does not fail the phone bound on upstream main. The full demo has a different layout and shows more phone lag with the same growth sequence. Its measurements are listed separately. These 390px captures are desktop Chromium at a phone-sized viewport, not iOS Safari.
Desktop, 1440×900
Before:
main-1440.mp4
After:
fixed-1440.mp4
Phone-sized viewport, 390×844
Before:
main-390.mp4
After:
fixed-390.mp4
Validation
bun scripts/chat-virtual-browser-qa.ts: passed all four viewport/theme cases, including continuous growth, reader-away behavior, folding, selection, pagination and restoration.bun test src/lib/followScroll.test.ts src/lib/chatRows.test.ts src/components/ChatTurn.test.ts src/conventions.test.ts scripts/conventions.test.ts: 58 passed, none failed.bun run check fast: failed in the unit lane (10,426 passed, 24 failed, 264 skipped). Failures include Codex store, shell harness and bridge tests outside the changed files. One private-terminal sign-in test process stalled past its timeout and was stopped, leaving that file without a result. Generated types/themes, typecheck and production build passed. Workflow syntax was skipped on macOS. Local Node is 25 rather than CI's 22.