Repository navigation
fix(chat): count a held scrollbar drag as scroll intent, and name the follow allowances - #1902
Conversation
|
Warning Review limit reachedYou'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 23 minutes. View limit details
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. Comment |
|
Merge pass review of eae26cf. Under the maintainer's rule this was reviewed by a Codex Sol review, the omo code reviewer and the merge pass's QA. CodeRabbit was rate-limited. Verdicts: the code reviewer passes it, with LOW notes only. Codex Sol fails it on two MEDIUM findings, below, that this PR introduces. Please fix those two. 1. A missed release can leave the hold set (
2. While the thumb is held still, programmatic upward corrections count as the reader's (
3. Not caused by this PR (LOW, follow-up): overlay scrollbars sit inside Holds
The merge pass re-checks the fix commit, looking only at the changed range. |
|
Both MEDIUM findings are fixed in f8e1f6a. A held thumb's move is now decided when it is let go, not while it is held. 2. Upward corrections while the thumb rests. ChatView no longer treats an upward scroll during a held press as the reader's. While the thumb is held, every upward move keeps main's layout allowance (48 px), so a correction the layout makes under a resting thumb keeps following, as on main. The page sees no pointer events during a thumb drag, so the release tells the two apart:
There is no separate "own scroll" marker to reuse. The component's own scrolls are downward ( 1. A release this page never sees. The press is now cleared on window Evidence (
A mutant without the The overlay-scrollbar note (3) is unchanged: with overlay scrollbars the press is not seen as a scrollbar press, so Chat keeps main's 48 px behaviour there. |
|
Follow-up on the mutant: f8e1f6a with only the |
… follow allowances A scrollbar thumb held past the 500 ms input window and then dragged up less than 48 px kept following: Chromium sends no pointermove while the thumb moves, so only the press was recorded. While a primary press on the transcript's scrollbar is held, an upward move is the reader's own, also when the thumb is released before the browser delivers the scroll. The 1 px and 48 px allowances are named, and the screen-reader limit is explained beside them. Fixes #1434
The merge pass found two ways the held-thumb rule from the previous commit misread the reader: an upward layout correction while the thumb only rested counted as the reader's, and a press whose release went unseen (a focus change, the page hidden) stayed held. While the thumb is held, upward moves now keep the layout allowance; when it is let go away from where it was pressed (a drag), a transcript that moved up stops following then. Blur and the page going hidden clear the press.
f8e1f6a to
e765586
Compare
|
Rebased onto main after the batch (#1831, #1890, #1895, #1897, #1830) and pushed e765586 (was f8e1f6a). Conflict: one, in const byReader = performance.now() - scrollInputAt.current < SCROLL_INPUT_MS;
const allowance = byReader ? READER_ALLOWANCE_PX : LAYOUT_ALLOWANCE_PX;
if (scrollingUp && byReader) navigated();
One integration line, as its own commit e765586: the let-go branch for a dragged thumb (from e87c195) stops following through Verified on e765586: |
|
Merge pass review of e765586 only (Codex gpt-6.1-sol xhigh, and an omo code reviewer, independently): the fix is partial; please extend it and add a test. The rebase part (f8e1f6a → e87c195) is fine. P2 / MEDIUM (both reviewers): a larger delayed drag still lets a pending send override navigation (
P3 / MEDIUM: no regression coverage.
What holds: the call's placement on the branch it does reach. No double counting, no cancellation from stationary or layout-only scrolls or from moves of 1 px or less, and no regression for a drag that returns to the bottom. 24 + 42 related unit tests pass. The merge pass rechecks the next commit (changed range only) and queues it. |
|
Both points are fixed in 2afc247. P2, a larger delayed drag: a dragged thumb that moved up now counts as navigation however far it went, in both release paths (
A let-go where the thumb went down (it only rested), and a drag that returns to the end, call nothing. P3, coverage:
Gate on 2afc247: the follow regression passes with all four new cases, as do |
|
Merge pass review-work report. Overall: PASSED. Under the maintainer's rule this ordinary client change was reviewed by a Codex Sol review, the omo code reviewer and the merge pass's QA. CodeRabbit was rate-limited.
The fix in f8e1f6a
Checked
Follow-up (LOW): overlay scrollbars sit inside All required checks are green and no review thread is open.
CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it. |
…#1916) ## What this fixes `main` is red since #1840 merged: `Browser regression (other scripts)` fails in `scripts/chat-local-send-follow-demo-regression.ts` at "remote pending refused and hydrated sends preserve place: remote" (line 112, `committed()` sees no transcript read carrying more within 10 s). It fails on 5e673ef (#1840) and ff11d94; e1c329b (#1902), just before #1840, passed. Every PR's CI inherits it. ## Why The script plays "another browser" with a raw submit frame whose `payload` is `text + "\r"`, the old shape. #1840 checks an immediate submit's framing in the bridge and the demo transport: the payload must be the text with LF as CR, bare or inside one paste pair, and the Enter is the submit's own (`src/lib/compose.ts`: "The text a composer message types, without its submit (HerdrSocket.submit adds the Enter)"). The demo transport therefore refuses the frame with `invalid_submit_text`, the remote message never reaches the transcript, and the step times out. The transport is right; the script's frame is stale. ## The change One line, test-only: the other browser's frame sends `payload: text`, as the composer shapes it. No product code changes. ## Verification - On main (ff11d94), unchanged, the script fails at "remote pending refused and hydrated sends preserve place: remote", as in CI. - With this line it passes, including that step's queued, pane-switch and reconnect parts. - `bun run check fast` passes. Test-only, so it doesn't need a CodeRabbit review. Co-authored-by: mp <mp@local>
## What it does for the user Chat now keeps following new output to the end when part of the transcript folds on its own. Before this, a turn settling at the end (its work folding together with a block above it), or a block above the view folding while the view was still gliding to the end, could stop following mid-transcript and show **New messages**, with no scroll by the reader. Fixes #2042. Reported by @odfalik. ## Cause `onScroll` in `src/components/ChatView.tsx` treats an upward move with no reader input that lands past the 48 px layout allowance as the reader leaving the end: it sets `pagingAway` and drops the list's pending scroll to the end. LegendList (`maintainVisibleContentPosition`) and the browser make such moves themselves when the transcript shrinks. The list holds the rows on screen where they were, and the browser keeps the view inside a shorter transcript. For one shrink both can move the view up, so it lands hundreds of pixels from the end, or past the end of a follow glide that was still on its way. ## Change - Without reader input, an upward move during which the transcript got shorter (the last row moved up in it, or its height dropped since the previous scroll event) now counts as the layout's: following goes on, and a view left more than 48 px short of the end is put back there. - That pin-back is skipped while `pagingAway` is set and while a following reader's own click is laid out where they are (ChatTimeline `manualLayout`), as the timeline's own follow paths already do. - Over a transcript that did not shrink, nothing changes: a no-input scroll (a screen reader's, a Firefox scrollbar's) leaves the end once the view is past 48 px from it, however small its steps (#1902). Reader input (wheel, keys, touch, a scrollbar drag) is unchanged. - The new scroll baselines reset with the chat's other scroll state on a pane change or a history replacement. Known trade-off: a no-input reader scroll that lands in the same scroll event as a fold is put back at the end once; the next move leaves as before. During a turn that keeps folding this can happen more than once. The page has no signal that tells such a scroll from the layout's own move in that event. Design precedent: none of the coding-agent apps exposes how it tells its own re-layout from a reader's scroll; this follows the rule #1734 set and #1902 kept, that only input that scrolls the transcript up is the reader's intent. ## How it was verified - `scripts/chat-virtual-browser-qa.ts` (real `ChatView` in Chromium, 1440 and 390 px, light and dark, already in CI's browser lane) gains: - `fold-together`: at the end, the last row and a row above fold in one layout (a turn settling). Following goes on to the end and the next output is followed. **Fails on main** ("fold-together: the next output is followed"), passes here. - `fold-above` / `grow-above`: a row above folds or grows while the view glides to the end after the last row grew. These pass here; on main they fail only when the fold lands in the same frame as the glide, so they do not fail on every main run. - `wheel-up`: a real upward wheel (`page.mouse.wheel`) still stops following ("Jump to latest", then **New messages** with the reader left away from the end). - Two 30 px upward steps without input from the end stop following past the allowance. This catches the first commit's regression found in review. - Full script: 60 PASS over all four width/theme combinations, exit 0. - `scripts/chat-local-send-follow-demo-regression.ts` (#1831), `scripts/chat-gjc-streaming-browser-qa.ts` (#1388/#1937) and `scripts/composer-fit-demo-regression.ts` (#1431): pass on the final commit. - `bun run check fast`: green on the final commit. - Reviews: Codex (gpt-6.1-sol, fast, xhigh) and the omo code reviewer. Both blocked the first commit on one finding: the 48 px allowance applied per scroll event, so small no-input steps were pinned back (a #1902 regression). The second commit measures it from the end again. The omo reviewer's other findings were fixed: the `manualLayout` guard and the baseline resets. Codex re-reviewed the fix: APPROVE. Not reproduced on a real Firefox or with a screen reader. Once, during QA, a real wheel landing early in a glide did not stop following; it did not reproduce in six later runs (three on main, three here) and this change does not touch moves made with reader input.
What it does for the user
Holding the chat's scrollbar still for a moment and then dragging it up a little now stops Chat from following new output, as any other upward scroll does. Before this, only a drag that began within half a second of the press counted. A thumb that rested first and then moved up less than 48 px kept following, so the next update pulled the reader back to the end.
Fixes #1434. #1734 already fixed items 1, 2, 4 and 5. This PR fixes what was left of item 3:
pointerdownon the scroller at the press, nopointermovewhile the thumb moves, then scroll events, thenpointerupat release (measured in Playwright's Chromium with scrollbars shown).ChatViewnow remembers that a primary-button press on the transcript's scrollbar is held. While it is held, an upward move counts as the reader's own, however long the thumb rested first. The hold ends onpointerup/pointercancel, on a new press, on any pointer event with no button down (a release this page did not see), and when the chat's input listeners are removed (the pane hidden or left).READER_ALLOWANCE_PX(endpoint rounding only, after the reader's own input) andLAYOUT_ALLOWANCE_PX(a composer growing a few lines before the correction to the end lands).LAYOUT_ALLOWANCE_PX. A scroll that sends no input this page can see, from a screen reader or from Firefox's scrollbar (which may send no press), leaves the end only past 48 px. The page has no signal to tell such a scroll from the composer's own growth, and guessing would bring back the composer-growth bug fix(chat): keep following the end while a growing draft shrinks the transcript #1431 fixed.No visible change otherwise. Design precedent: none of the coding-agent apps exposes this. It follows the rule #1734 set: only input that scrolls the transcript up is the reader's intent.
#1831: that open PR edits the same
allowanceline inonScroll(it addsbyReaderandnavigated()). Whichever lands second keeps both: #1831'sbyReaderwith the named constants. A held-thumb drag then also counts as navigation for #1831's follow of the reader's own send.How it was verified
scripts/chat-virtual-browser-qa.ts(already in CI's browser lane) runs the realChatViewin Chromium at 1440 px and 390 px, light and dark. Its scroll-intent loop gains four cases. Each fires the scrollbar press on the scroller right of its content box, then moves the transcript up 20 px, inside the layout allowance but past endpoint rounding.performance.nowis skewed by 600 ms around that move, so the move comes after the input window as a resting thumb's would, with no sleep.scrollbar-held: the press is still held when the move lands. Following stops and New messages shows.scrollbar-held-released-first: the thumb is released before the scroll event is delivered (the order Codex's review found). Following stops.scrollbar-released,scrollbar-unseen-release(nopointerup, the next pointer event has no button down): the press is over, so the same 20 px correction keeps following.scrollbar-held: only upward input stops following(0 !== 1). On this branch it passes, 52 PASS, together with the existing cases: a downward wheel, a click, typing and composing keys keep following; an upward wheel, navigation keys and a drag stop it; a composer growing a line paints no frame off the end.bun run check fastpasses on this commit (typecheck, build, unit tests).scrollbar-held-released-first. omo's LOWs:Not reproduced on a real Firefox or with a screen reader; see the screen-reader note above.