Repository navigation
fix(chat): follow your own accepted send back to the end unless you navigate - #1831
Merged
Merged
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 38 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 |
Owner
Author
|
Merge pass: a rebase is needed. #1828 merged as 6e0f88f, and this branch now conflicts with main in |
Owner
Author
|
Merge pass review-work for ce27dab: PASSED. Gate APPROVE (medium-high). The only thing left is the rebase noted above, for the Checks
MINOR follow-ups
|
devswha
force-pushed
the
fix/chat-local-send-follow
branch
from
October 24, 2026 11:59
ce27dab to
3434288
Compare
Owner
Author
|
Rebased onto main (after #1828, 6e0f88f) and pushed 3434288. The only conflict was the browser lane's script count in On the earlier Native Windows install failure: the rerun of run 40088346948 passed. It was the intermittent |
devswha
added a commit
that referenced
this pull request
Oct 24, 2026
… a pinned count (#1874) ## What `scripts/ci-browser.test.ts` pinned the number of browser scripts (`toHaveLength(83)`), a list of `toContain` lines and the last script. So every PR that adds a browser script edited the same lines, and parallel PRs conflicted on them just for that (#1831, #1837, #1849 and #1852 needed rebases for it). This PR removes those pins and checks the same things against the files on disk: - The serial lane's summary must equal `scripts/demo-build.ts` followed by the list parsed from `scripts/ci-browser.sh` (`serialBrowserScripts()`), in order. This checks more than the old count did. - The lane must not repeat a script or name one that does not exist. - Every `scripts/*-regression.ts` and `scripts/*-browser-qa.ts` must be run by the lane in one of three ways: it has its own `run_script` line; it is imported by a script the lane runs (transitively, e.g. through `ui-regression.ts` or `ui-contexts-checks.ts`); or it is spawned by one (`join(import.meta.dir, "x.ts")`). Spawns are followed only from files that run as scripts, not from imported helpers, because those spawns sit under `import.meta.main`. A script CI does not run has to be named in a new `MANUAL_BROWSER_SCRIPTS` list. Today that list holds 13 scripts that are run by hand (Android emulator, SSH fixture, native Windows, update QA, ...). - The manual list must only name scripts that exist and that CI does not reach, so it cannot go stale. A PR that adds a browser script now adds only its `run_script` line in `scripts/ci-browser.sh` (or a line in the manual list), so two such PRs no longer touch the same line. `docs/development.md` describes the check. Test-only change: CI behaviour is unchanged. ## Verification - `bun test scripts/ci-browser.test.ts`: 7 pass. - Mutation checks, each restored afterwards: - Deleting the `run_script scripts/workspace-rows-regression.ts` line fails "runs every browser script on disk ..." and lists that script as unassigned. - Adding `run_script scripts/does-not-exist-regression.ts` fails "runs each script once, and only scripts that exist". - Adding an untracked `scripts/brand-new-regression.ts` fails the assignment check. - The restored tree passes. - `bun run check fast`: green (typecheck, build, 6396 unit tests). - Reviewed by a code-review agent. Its one HIGH finding (a spawn inside an imported helper's `import.meta.main` block was being counted as run by CI, hiding `keyboard-viewport-demo-regression.ts`) is fixed in this commit. --------- Co-authored-by: mp <mp@local>
Owner
Author
|
Merge pass: one rebase needed. #1874 is merged on main. |
A reader scrolled back to older turns who sent a message from the box stayed
where they were, so their own message and the reply landed out of view.
PaneTerminal now keeps a per-pane local send receipt, { key,
navigationGeneration }, separate from sentKey. It advances only for this
browser's own immediate composer submission whose receipt is ok with no
pending item, and only while the pane, socket and session that sent it are
still current. Refusals, uncertain answers, pending/queued receipts, Send now,
another browser's message, restore and mount never advance it. The chat's
navigation generation is captured when the message goes out, before the
receipt is awaited.
ChatView raises that generation on deliberate upward wheel, touch or keyboard
scrolling, page navigation, a minimap jump, a restored place, a replaced
history and leaving the pane (a retained pane hidden off-tab, or unmount). A
fresh receipt whose generation still matches re-engages the existing follow
through the Jump to latest path; an upward scroll before the turn is published
ends it as before. Each key is consumed once and a newly mounted chat starts
from the current key. No wire, endpoint or demo transport change.
Checked with the new scripts/chat-local-send-follow-demo-regression.ts on the
demo build (gated submit receipts and transcript reads; it fails on main and
passes here): accepted immediate local send resumes follow, navigation before
receipt cancels follow, navigation before transcript publication cancels
follow, and remote pending refused and hydrated sends preserve place (refused,
uncertain, remote browser, queued and Send now, pane switch before the
receipt, reconnect before the receipt). It is enrolled in the browser lane.
scripts/chat-virtual-browser-qa.ts and
scripts/restored-draft-demo-regression.ts still pass; bun run check fast
passes apart from server opencode, conversation and session-history timeouts
that fail the same way on main on this loaded PC.
devswha
force-pushed
the
fix/chat-local-send-follow
branch
from
October 24, 2026 15:15
3434288 to
9bd37a6
Compare
Owner
Author
|
Rebased onto main after #1874 and pushed 9bd37a6 (was 3434288). I dropped this PR's change to |
Owner
Author
|
Merge pass review-work report. Overall: PASSED.
Manual QA
Code review and gate
MINOR follow-ups
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. |
This was referenced Oct 24, 2026
devswha
added a commit
that referenced
this pull request
Oct 24, 2026
… follow allowances (#1902) ## 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: - **A held scrollbar press** (3a). In Chromium a thumb drag sends `pointerdown` on the scroller at the press, no `pointermove` while the thumb moves, then scroll events, then `pointerup` at release (measured in Playwright's Chromium with scrollbars shown). `ChatView` now 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 on `pointerup`/`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). - **The 1 px and 48 px values** (3c) are named: `READER_ALLOWANCE_PX` (endpoint rounding only, after the reader's own input) and `LAYOUT_ALLOWANCE_PX` (a composer growing a few lines before the correction to the end lands). - **Screen-reader scrolling** (3b) is not fixed and is documented beside `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 #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 `allowance` line in `onScroll` (it adds `byReader` and `navigated()`). Whichever lands second keeps both: #1831's `byReader` with 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 real `ChatView` in 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.now` is 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` (no `pointerup`, the next pointer event has no button down): the press is over, so the same 20 px correction keeps following. - On origin/main the script fails at `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 fast` passes on this commit (typecheck, build, unit tests). - Reviewed by a code-review agent (omo, approved) and by Codex (gpt-6.1-sol). Codex's one MEDIUM (a thumb released before the browser delivered its scroll lost the intent) is fixed and covered by `scrollbar-held-released-first`. omo's LOWs: - **Fixed:** the hold now needs the primary button, and it is cleared when the chat's input listeners go away (the pane hidden or left mid-press). - **Left:** a new press clearing the hold has no case of its own; it duplicates the release paths. Not reproduced on a real Firefox or with a screen reader; see the screen-reader note above. --------- Co-authored-by: mp <mp@local>
This was referenced Oct 24, 2026
devswha
added a commit
that referenced
this pull request
Oct 25, 2026
…ough its rewrites (#1937) ## What it does for the user While a GJC turn streams tool calls into Chat, a tool you opened now stays open and stays where it is on screen: - **Opening is reading.** Opening a tool row, a folded work block or a tool's whole output stops following the end, just as scrolling up does. New records arrive below without moving what you are reading. Jump to latest (or New messages) brings you back to the end, and following resumes from there. A reader who opens nothing keeps following exactly as before. - **GJC rewrites that keep the turns no longer reset the chat.** gjc writes its whole session file again and renames it over the old one: on its first save, and when it trims old tool outputs (it does the same when it compacts). The server reads the new file as another history (`history_id` is the file's identity), so the chat started over: every opened detail folded, a reader's place was lost, and at phone width the end was no longer followed. Now a new history ID whose newest page still opens with every turn already shown, in the same places, keeps the rows, what was opened and where you read. Each shown turn's role and start time, each part's kind, and each tool's call id and input must be unchanged (gjc's tool parts now carry their `toolCall` id as `call_id` for this); what a tool answered may be trimmed. Anything else still starts over as before: a compaction that dropped turns or moved where the newest page starts, a new session, a cleared context, a rewind (even one that keeps a turn's tools' names), a moved branch, or a reader holding older pages. - **A todo update you opened stays open.** Since #1846, contiguous todo updates fold into one Plan row. An update you had opened turned into a closed Plan row as soon as the next update arrived, so what you were reading folded away. The Plan row now opens when you had opened one of its updates, with that update still open inside it. A Plan row nobody opened still starts closed. ## Why #1388: "the viewport alternates between the user message and the bottom" and "tool details that the reader expanded collapse when new records arrive". Two causes, both reproduced on current main with GJC-shaped records: 1. `ChatTimeline` remembered a click on a disclosure only until the next rows change (`manualLayout` cleared on every `rows` update). Then ChatView's layout effect and the list's size callback followed the end again, dragging an opened running tool's header up 383–455 px on every record. 2. gjc 0.18's session manager rewrites its file through a temp file + rename (`rewriteEntries` after compaction eviction and tool-output pruning; I read this from the installed gjc 0.18.12 binary). A rename is a new inode, so `transcriptStream` gives a new `history_id` and ChatView's read dropped the row slots, the row memories and the reader's intent. Since #1831, a message sent from this browser's box follows the chat back to the end unless the reader navigated after it went out. Opening a tool now counts as that navigation too, as scrolling up does, so a send on its way no longer pulls the reader off a tool they opened. A gjc rewrite that continues the chat is not navigation (the reader's place is kept); any other new history still is. Design: T3 Code took the other side for a reader at the very bottom (pingdotgg/t3code#17954 keeps a reader who toggles a tool there pinned to the bottom). Here the issue asks for the opened row to stay put while a turn streams, and Ovis already treats scrolling up as "stop following until I come back". Opening a detail joins that rule instead of adding a new one, so nothing new is drawn: the existing Jump to latest and New messages controls carry it. DESIGN.md's chat follow paragraph says so. The server's history identity is unchanged. **HTTP contract:** `GET /api/pane/conversation` now gives a gjc tool part its own `call_id`, the optional field `shared/protocol.ts` already declares for Claude and Codex; omp and pi still carry none (`server/api.contract.test.ts`). CodeRabbit is rate-limited here, so a code-review agent's review stands in, and this needs a real review before merging. A byte-prefix check on the server would miss gjc's rewrites, because pruning changes earlier records. The continuity check runs only in the chat that already holds the turns. ## How it was verified - New `scripts/chat-gjc-streaming-browser-qa.ts` (added to the browser lane). It feeds GJC-shaped `message`/`toolCall`/`toolResult` records one at a time through the real `transcriptPage("gjc-transcript")` parser and the real ChatView → ChatTimeline → LegendList path in Chromium, at 1440 and 390 px. Three readers are covered: one who opened a running tool, one following the end, and one scrolled up. Each runs with plain appends and with a gjc-style temp+rename rewrite. It samples every frame from each refresh until the view settles: - the opened tool's header and the scrolled reader's row stay within 1 px in every frame - the opened tool stays `aria-expanded="true"` - a follower ends at the end after every record - Jump to latest resumes following - On main (d3eefef, before this branch was rebased) it fails 62 assertions: - header displacement −383 to −455 px per record - `aria-expanded="false"` after a rewrite - the scrolled reader's row lost after a rewrite - the 390 px follower 179–205 px short of the end - in a chat that still fit the screen when the tool was opened, the opened tool dragged up to 1502 px once later records overflow it - On this branch every scenario holds at 0 px (20 scenarios, rebased on ddee8be). Current main (ddee8be) still fails 62 assertions. - Its `todo` scenario (added with the rebase) opens a gjc `todo_write` update and appends the next one, plainly and through a rewrite: the Plan row's head stays where the opened update's head was in every frame, it is open, and the update read stays open in it. Run against the commit before that fix, the script fails only those 16 todo assertions. - `server/conversation.test.ts` runs a gjc rewrite through the real parser: the same calls written again continue, and a rewind that reads the same file again under another call id, later, does not. - `src/lib/chatRows.test.ts` covers `continuesHistory`: continuation, a dropped/moved/retimed turn, a page that starts later (dropped first turns), a rewind that puts another call of the same tool in a shown place (other input or call id), another source, scrollback, empty and untimed pages. - Its `jump` scenario opens a tool and takes Jump to latest at once; closes and reopens it and jumps again; closes it while following, scrolls away and jumps back; and does that again but scrolls back to the end by hand. After each Jump the last row grows with no record between, and the end is followed. The script counts an answer as served only once the chat has read its body and React has committed after it. - `scripts/chat-virtual-browser-qa.ts` passes at 1440 and 390 px, dark and light (paging, pagination, virtualization, selection and disclosure restoration). The other chat browser scripts run in this PR's browser lane. - `scripts/todo-snapshots-demo-regression.ts` passes, and so do `chat-local-send-follow-demo-regression.ts` (#1831), `chat-browser-qa.ts` and `chat-tool-paths-demo-regression.ts` on the rebased branch. - `bun run check fast` passes: typecheck, build, generated files, workflow syntax, 6772 unit tests. - Reviewed by a code-review agent (no HIGH; its one real gap, a chat too short to scroll re-arming follow, is fixed in the second commit) and by Codex Sol 6.1 (no HIGH; three MEDIUM findings on the history continuation are fixed in the third commit: it applies to gjc only and needs each shown part's kind and tool to match, an older-page read in flight is dropped, and the kept reading place moves to the new history ID). A Codex re-review of that commit found no remaining HIGH or MEDIUM issue. - After the rebase onto current main, Codex Sol reviewed the rebase delta. The two conflict resolutions with #1831 were clean. It found one MEDIUM, the #1846 todo row folding an update the reader had opened, which the last commit fixes. Its re-review found one more MEDIUM (a remounted Plan row lost the openness it had inherited), fixed in the same commit; its 13-case repro passes. Not covered: native iOS/Safari, a gjc compaction that drops turns (it starts over, as before), real gjc runs (the records are synthetic and the rewrite is simulated as gjc does it), and a reader who has loaded older pages when gjc rewrites. That reader still starts over, because those pages were read from the old file. Fixes #1388 --------- Co-authored-by: mp <mp@local>
devswha
added a commit
that referenced
this pull request
Oct 28, 2026
## 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does for the user
When you scroll up in Chat to read older turns and then send a message from the message box, the chat now takes you back to its end once the PC accepts the message, and follows the reply there. Before this, the view stayed where you were reading, so your own message and the agent's answer appeared off screen behind a "New messages" pill.
It only follows your own message from this browser that the bridge accepted at once (a receipt with no pending item, not a refusal, not an unconfirmed send to an older bridge). These keep the reader's place:
Scrolling or paging back, a wheel over the minimap rail, a minimap jump, a restored place, a replaced history, or leaving the pane before the receipt or before the turn shows cancels the follow. Nothing new is drawn: the existing Jump to latest and New messages controls stay as they are.
Why
Agent of Empires re-pins its view on every prompt, local, remote or replayed. Ovis's sends carry a receipt, so only that accepted local receipt counts, and the reader's own navigation after sending always wins. #1734 and #1431 changed how scroll intent is read. I checked this against current main: the regression script below still fails there, at the step where the accepted receipt should bring the view to the end.
How it was verified
scripts/chat-local-send-follow-demo-regression.ts(added to the browser lane) runs on the demo app over its fixture transport. The submit receipts and transcript reads are gated, so each ordering is forced rather than timed. It covers:src/reset to origin/main it fails at "the accepted receipt brings the view to the end".bun run check fastpasses (typecheck, build, 6380 unit tests).Follow-up
The regression script exercises wheel navigation only. PageUp and minimap tick jumps cancelling the follow are covered by code reading, not by a scripted step yet.
Rebased onto current main; the composer send moved to
src/lib/useComposerSends.tssince the branch was cut, so the change was ported there.