[devswha]

Change

scripts/chat-selection-turn-end-demo-regression.ts (#2658) failed intermittently in "Browser regression (other scripts)" on PRs that do not touch chat, e.g. #2621 run 41725988136: desktop-1280-fold, one frame at 533.3 px against 572.8. It is not the test: the selection hold from #2658 loses the words under the selection when frames are slow. Instrumenting every frame (rAF, every ResizeObserver callback, every scrollTop set and scrollBy, the scroll events) on a slow machine showed three races, all in the hold:

1. A fold in the held words' own row was painted uncorrected for a frame. LegendList puts a row's shrink of more than 1 px off to the next animation frame (scheduleWebShrinkMeasurement), so when the ending turn's work block folds above the words in the same row, no observer the hold listens to fires in that frame. On a slow frame the slide moves 40-50 px at once, and that frame is painted with the words moved. The hold now also watches the held words' own row ([data-chat-row]) with the chat's resize observer, which the chat made when it mounted, so the move is put back in the frame it happens, before the paint.
2. The hold's own correction made the chat think the reader left the end. keep() set a fractional scrollTop (253.5625), reported the read-back (254) through onFollowed, and the scroll event then read 253: a 1 px move up with no input, which ChatView.onScroll took for the reader leaving the end. following went false, keep() stopped holding, and the rest of the fold drifted the words 10-27 px. keep() now scrolls in whole pixels, and keeps the words' aimed place as its reference instead of re-reading it, so the rounding adds up with the next move instead of drifting by a pixel.
3. The list and the browser move the view up after the hold, with no input. A frame later LegendList measures the same fold and its MVCP scrollBys again (−24, −36 px), and the browser trims the view as the transcript shrinks. ChatView.onScroll now takes a move up without the reader's input as the layout's while a selection holds the view (the reader's wheel, keys, touch and scrollbar still count as leaving), and the hold hears the list's adjust (listen("scrollAdjust")) and puts the words' whole move back, as it already did for the list's padded frame (listEndPadded).

scrollAdjust is a signal LegendList's listen already delivers, but its public LegendListListenerType leaves it out; the existing patches/@legendapp%2Flist@3.3.5.patch gains a one-line react.d.ts hunk adding it (no runtime change, bun.lock unchanged; checked with a clean install from an empty cache).

The test: readings are compared unrounded. Rounding each to 0.1 px first made a 0.96 px step (inside the hold's own sub-pixel rule, put back the next frame) read as exactly 1 px and fail < 1. And a new case, desktop-1280-fold-reduced-motion: with reduced motion the block folds in one frame, which is what one slow frame of the slide does, so it fails on main every time instead of only on a slow runner (on main the words move to 608.8 and then 460.8 px; reduced-motion users saw that).

Review round (1db6fa9): only a move up of at most one pixel without input is excused while a selection stands (the browser's whole-pixel trim), only a move of at most a pixel landing within a pixel above where the chat itself last put the view (a4e9fa8, 8181b42), so a reader who scrolls up with no input the page sees, in large steps or a pixel at a time, is still away from the end once the selection goes; the script gains desktop-1280-reader-away for it (fails 3/3 on c9901b9), and watches each fold until it has slid shut. Details in the review comment.

No changelog fragment: changelog.d/chat-selection-holds-still.md from #2658 is still unreleased, and this makes it true.

Validation

Before/after on this PC (load average 36-98 from other lanes and CI runners), the demo build as CI builds it (bun scripts/demo-build.ts), through dori heavy:

	origin/main 4bc043d	this PR
chat-selection-turn-end-demo-regression.ts, whole script	0/20 pass (old script); 0/5 with the new script, the reduced-motion case failing every time it is reached	24/24 pass, 6 in parallel
desktop-1280-fold only, looped (instrumented copy)	fails within the first 3 iterations	150/150 (6 × 25 in parallel)
phone-390-fold only, looped		25/25
desktop-1280-fold with reduced motion, looped	0/5	10/10

Intermediate states, for the record: with only the row watch (1), 18/20 whole-script runs passed and the remaining failures were the drift of (2) and (3); with (1) and (2) but no list-adjust signal, transitions passed 150/150 but reduced motion still failed.

• bun run check fast (through dori heavy) on c9901b9 and again on 1db6fa9: workflow syntax, generated types and themes, typecheck, build, unit tests (10883 pass) all ok.
• On 1db6fa9, under the same load: whole script (then 7 cases) 24/24; desktop-1280-fold looped 150/150, phone-390-fold 25/25, reduced motion 10/10.
• On a4e9fa8 (last review round: only a landing a pixel above where the chat last put the view is excused), whole script (10 cases: reader-away in 30 px steps, 1 px steps, 1 px after growth below, and down-then-up) 24/24; the same loops 150/150, 25/25, 10/10; the 11 adjacent scripts pass; check fast ok. Each new reader-away case fails on the commit before its fix (1db6fa9, c358899, f37fab8). On the merged head 0830c48: check fast ok (10894 unit tests), whole script 6/6. On 8181b42 (the event's own move must be a pixel at most too; new 151 px case, which fails 2/2 on 0830c48): whole script (11 cases) 6/6, check fast ok.
• Adjacent browser scripts on the final build, all pass: chat-virtual-browser-qa.ts, chat-history-browser-qa.ts, chat-local-send-follow-demo-regression.ts, chat-quote-selection-end-regression.ts, chat-scroll-padding-regression.ts, chat-select-quote-demo-regression.ts, chat-selection-copy-regression.ts, chat-working-line-demo-regression.ts, chat-no-rewind-demo-regression.ts, reply-comments-demo-regression.ts, chat-gjc-streaming-browser-qa.ts.
• chat-browser-qa.ts could not run here: it times out waiting for .conn-live on this PC, the same on origin/main (no live connection in this environment); CI runs it.
• chat-virtual-browser-qa.ts's failure on feat(server): support IPv6 loopback beside IPv4 #2644 (continuous content left the reader behind for 6 consecutive animation frames, line 717) is a different cause: it is about following streamed content, and that run (2026-11-03 08:03 UTC) predates fix(chat): hold the chat still while a selection is in progress (#2650) #2658. Not addressed here.
• Not run: bun run check full, the Mac and Windows lanes. No server, shared or install code changed.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: devswha/ovis
• Branch: fix/flaky-chat-selection-turn-end
• File: .coderabbit.yaml
• Commit: 2a03a03
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: bee6c7f0-556c-451f-a001-e530725e9cd9

Commits

Reviewing files that changed from the base of the PR and between c5f5331 and 2a03a03.

Files selected for processing: 4

• patches/@legendapp%2Flist@3.3.5.patch
• scripts/chat-selection-turn-end-demo-regression.ts
• src/components/ChatTimeline.tsx
• src/components/ChatView.tsx

• 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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[devswha]

Review round on c9901b9 (omo: BLOCK; Codex gpt-6.1-terra xhigh: REQUEST_CHANGES, no HIGH). Fixed in 1db6fa9:

• MEDIUM, reproduced (Codex; omo reproduced it): ChatView.onScroll excused every move up without input while a selection stood, so a reader who scrolled up with no input the page sees (a screen reader, find in page) was pulled back to the end once the selection went. Now only a move up of at most one pixel is excused under a selection: the browser's whole-pixel trim as the transcript shrinks, which is all the hold needed. The turn-end script gains desktop-1280-reader-away (selection stands, three 30 px scrolls with no input, selection cleared, the reader must stay 90 px away): it fails 3/3 on c9901b9 and passes on 1db6fa9.
• LOW (Codex), test only: the script now watches each fold until no .animated-height.is-moving remains, plus the frames the list measures it in, instead of stopping at a fixed point.
• MEDIUM, not reproduced (Codex): a scripted scroll landing in the same frame as the list's scrollAdjust would be put back for that frame along with the list's move. It is the same one-frame trade-off fix(chat): hold the chat still while a selection is in progress (#2650) #2658 accepted for the list's padded frame (listEndPadded), and LegendList's adjust delta is not exposed to separate them. Left as is; no follow-up issue since it was not reproduced.
• LOW (omo), not reproduced: the scrollAdjust subscription could be missing if listRef.current were null when the effect runs. The list's imperative ref is attached in the commit's layout phase, before passive effects run, so it is set; and a replaced list would also leave the existing resize observer on the old node (same pattern as before this PR). Left as is.

Validation on 1db6fa9: bun run check fast ok (10883 unit tests). Under load (6 in parallel, load average ~60): whole script 24/24; desktop-1280-fold looped 150/150, phone-390-fold 25/25, reduced motion 10/10. The 11 adjacent chat browser scripts listed in the PR body pass.

CodeRabbit is rate-limited on this PR (no review ran); the omo and Codex reviews above stand in for it.

[devswha]

Second review round on 1db6fa9 (Codex gpt-6.1-terra xhigh: one MEDIUM, no HIGH; omo: BLOCK on the same MEDIUM, reproduced). Fixed in c358899:

• MEDIUM, reproduced: each move up of at most one pixel without input was excused under a selection however many came, so a reader who left the end a pixel at a time (90 × 1 px) was pulled back once the selection went. The moves excused under one selection now add up to the layout allowance (48 px) at most, reset when the selection goes; past it they are the reader's, as without a selection. desktop-1280-reader-away now runs in 30 px and in 1 px steps: the 1 px case fails 3/3 on 1db6fa9 and passes on c358899.
• omo's two LOWs (the adjusted flag taking a same-frame reader scroll back with the list's adjust; the listener bound to the list instance once) are the ones already answered in the previous comment; not reproduced, left as is.

Validation on c358899: bun run check fast ok (10883 unit tests). Under load (6 in parallel): whole script (now 8 cases) 24/24; desktop-1280-fold looped 150/150, phone-390-fold 25/25, reduced motion 10/10; the 11 adjacent chat browser scripts in the PR body all pass. CodeRabbit is still rate-limited on this PR.

[devswha]

Third (last) review round, on c358899 (omo BLOCK and Codex REQUEST_CHANGES on the same reproduced MEDIUM, no HIGH), then a Codex finding on the local fix; head is now 0830c48 (a4e9fa8 plus a merge of main after #2693):

• MEDIUM, reproduced (omo): a 48 px budget of excused one-pixel moves still let a reader already far from the end (the transcript grew below the selection) drift up and be pulled back. The heuristic is gone: under a selection, only a landing within a pixel above where the chat itself last put the view (onFollowed: a follow, or the hold's correction) counts as the browser's trim (f37fab8). New case desktop-1280-reader-away-1px-grown-150: fails 3/3 on c358899, passes now.
• Codex on f37fab8 (rated P1, not pushed): that bound was one-sided, so after the reader scrolled down past the chat's last placement every move back up was excused. Now 0 < placed - scrollTop <= 1 (a4e9fa8). New case desktop-1280-reader-away-1px-grown-300-down-200 (the hold first puts the view back for a row growing above the words, then the reader scrolls down into what grew and back up a pixel at a time): fails 2/2 on f37fab8, passes now. Codex re-review of a4e9fa8: APPROVE, no findings.
• The earlier carry-over MEDIUM (a 48 px budget reaching into the next selection) is moot: the budget no longer exists.

Validation on a4e9fa8, under load (6 in parallel): whole script (now 10 cases) 24/24; desktop-1280-fold looped 150/150, phone-390-fold 25/25, reduced motion 10/10; the 11 adjacent chat browser scripts pass; bun run check fast ok. On the merged head 0830c48: check fast ok (10894 unit tests) and the whole script 6/6.

chat-virtual-browser-qa.ts:81 (seen on #2621/#2644) is a different cause and is not addressed here: no selection is involved; after page.reload() a reopened chat can end about 54 px short of its end (CI's screenshot shows Jump to latest). It reproduces on main about once in a few hundred reloads on this PC; it goes to a separate PR.

[devswha]

Review of 0830c48: omo BLOCK on one reproduced MEDIUM, no HIGH (its Codex Terra pass reported the same one; Codex's own pass on a4e9fa8 approved). Per the last-round rule for this PR, it is not fixed here:

• MEDIUM, reproduced, follow-up chat: a no-input scroll landing a pixel above the chat's last placement is taken for a trim under a selection #2704: the trim excuse checks only where the view lands (within a pixel above where the chat last put it), not how far that scroll event moved, so a no-input reader scroll landing exactly there is excused. The fix is to also require the event's own move up to be at most one pixel; it is written up in chat: a no-input scroll landing a pixel above the chat's last placement is taken for a trim under a selection #2704 with the reproduction.
• LOW, not reproduced (omo): the scrollAdjust listener subscribes once from listRef.current. The list's imperative ref is set before passive effects run, so it is there today; noted only.

CI on 0830c48 is running.

[devswha]

Head 8181b42: the fix for the MEDIUM omo reproduced on 0830c48 (a no-input scroll of any size landing a pixel above where the chat last put the view was excused). The scroll event's own move up must now be at most a pixel too, read before onScroll records the new position. (Comparing after that point, as first suggested, compares the position with itself; a debug build showed it.) The new case desktop-1280-reader-away-151px-grown-300-down-150 fails 2/2 on 0830c48 and passes on 8181b42. #2704 (filed for that MEDIUM) is resolved by this commit.

Validation on 8181b42: the whole script (11 cases) 6/6 under load; bun run check fast ok (10894 unit tests).

omo review at 8181b42: CLEAR (with a Codex Terra pass folded in). Remaining notes, none blocking:

• LOW, reproduced: under a selection, after the hold has corrected the view, a single one-pixel no-input move up is still excused as the browser's trim even when nothing shrank, so a reader who leaves the end by exactly one pixel that way is brought back once the selection goes. Wheel, keys, touch and the scrollbar count as the reader, so this needs assistive tech moving exactly one pixel. Not filed as an issue (LOW); a possible tightening is to also require the transcript to have shrunk in that event.
• LOW, not reproduced: the scrollAdjust subscription is made once from listRef.current; the list's imperative ref is attached before the effect runs, so it is there today.

CodeRabbit is rate-limited on this PR; the omo and Codex reviews stand in for it.

[devswha]

Merge note: CodeRabbit was rate-limited for these heads; merged on the omo review (CLEAR receipt at the head), Codex Terra (APPROVE; every reproduced MEDIUM across 4 rounds fixed with a failing-first case) and green required checks on GitHub-hosted runners. Remaining LOW notes are in the comments above.