fix(terminal): preserve scroll intent across keyboard resize, surface history truncation - #286
Merged
Merged
Conversation
… history truncation Closes#259, closes#258. Both bottom out in the same gap: nothing tracked whether the user was following live output or reading history. #259 — the keyboard path forced the terminal to the bottom unconditionally (onKeyboardShow/onKeyboardHide passed scrollToBottom:true, applied with no check), so opening the keyboard while scrolled up yanked the user down. The settle cycle now captures intent on its FIRST event, before any fit() has reflowed the buffer, and returns to that anchor when the user was reading. A later capture would read an already-moved viewportY, which is why the capture point matters. The param is renamed restoreScroll to match. Separately, flushPendingWrites gated viewport preservation on _hasRecentUserScrollUp(), a 1500ms decay window, so a user who scrolled up and then actually READ for longer lost protection mid-read. Being scrolled up IS the intent however long ago it was expressed, so it now keys off position. The recency window stays as a race guard on the sticky scroll-to-bottom. The full-history repull already held the user's place and is unchanged. #258 — truncation was reported by a grey line written INTO the terminal ("earlier output truncated"), which scrolls away with the output it describes, cannot be acted on, and said the same thing whether the rest was one click away or gone forever. The server set one `truncated` boolean at two sites meaning opposite things, and the client discarded fullSize and source entirely. The route now reports truncationReason ('tail' = intentional partial replay, the rest is retained; 'capped' = the byte ceiling dropped it) plus retainedBytes, and 'capped' is not downgraded by a later tail cut. The client renders a dismissible banner outside terminal output with three honest states: recoverable (offers Load full history), at-ceiling, and exhausted. The Load button forces past the scroll cooldown but NOT past _replayWouldShrinkBuffer, which still refuses a downgrade for repaint-mode panes. The banner is an overlay, not a flex child: FitAddon derives rows/cols from the terminal parent's computed height, so occupying real layout space would SIGWINCH the CLI on every truncation-state change. Verified in a real browser on the 7 skins: banner text and button clear 4.5:1 contrast on all of them, and terminal height is byte-identical with the banner shown. The first cut used --bg-elevated and --accent-muted, which do not exist, so light skins rendered a hardcoded dark bar under dark text; it now uses only tokens every skin redefines. test/terminal-scroll-intent.test.ts lives outside test/mobile/ deliberately — that suite is excluded from test:ci, so a guard placed there is invisible to CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e buffer Two further instances of the same root cause, both in _onSessionNeedsRefresh, which is SERVER-triggered (it fires after SSE backpressure clears) so the user has no gesture to blame the result on. 1. It ended in an unconditional scrollToBottom, so a user quietly reading scrollback was dropped to the live output by a background event. It now holds their place. The rewrite REPLACES the buffer, so an absolute viewportY captured beforehand is meaningless afterwards; distance from the bottom is the anchor that survives, via computeRewriteScrollLine(). 2. It rebuilt the terminal from a 1MB TAIL. Measured end to end on a 900-line shell pane: an 869-row buffer came back as 158 rows, so the refresh meant to REPAIR the display was destroying most of the scrollback every time it ran. It now asks for full history, and falls back to the tail only when _replayWouldShrinkBuffer refuses the capture, which keeps repaint-mode panes (tmux holds roughly one frame for them) exactly as they were. Also records truncation state here, so the #258 banner stops describing the pre-refresh buffer. Verified in a real browser against a live session: baseY 869 -> 869 where it used to be 869 -> 158, a reader 200 lines up stays 200 lines up, and a follower stays pinned to the bottom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The refresh can now issue two fetches (full history, then the tail as a downgrade fallback), which widens an existing window where the user switches tabs mid-flight and this session's history gets painted into the terminal they are now looking at. Guard it the way _maybeRefetchFullHistory already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…history Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 freeto 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.
Closes#259, closes#258. Both reported by @DodgyBadger.
Both issues bottom out in the same gap: nothing tracked whether the user was following live output or reading history. Fixing that once makes both cheap, which is why this is one PR rather than two touching the same functions.
#259 mobile scroll position
Three places dragged a reader to the bottom. The keyboard was the reported one; the other two turned up while fixing it.
Keyboard resize.
onKeyboardShow/onKeyboardHidepassedscrollToBottom: trueand the settle timer applied it without checking where the user was. The settle cycle now captures intent on its first event, beforefit()reflows anything, and returns to that anchor when the user was reading. Capturing later would read an already-movedviewportY, so there is a test pinning that ordering specifically. The param is renamedrestoreScrollto stop the name lying.Live writes.
flushPendingWritesgated viewport preservation on_hasRecentUserScrollUp(), a 1500ms decay window, so a user who scrolled up and then actually read for longer than that lost the protection mid-read and got dragged along by the next repaint. Being scrolled up is the intent however long ago it was expressed, so it now keys off position. The recency window stays where it still earns its place, as a race guard on the sticky scroll-to-bottom.Backpressure refresh.
_onSessionNeedsRefreshis server-triggered, so there is no gesture to blame the jump on. It now holds the reader's place. Because the rewrite replaces the buffer, an absoluteviewportYis meaningless across it; distance from the bottom is the anchor that survives.The full-history repull already held the user's place and is unchanged.
Bonus: the refresh was destroying scrollback
Found while verifying the above, and arguably the most serious thing here.
_onSessionNeedsRefreshrebuilt the terminal from a 1MB tail. Measured end to end on a 900-line shell pane: an 869-row buffer came back as 158 rows. The refresh that exists to repair the display was silently discarding most of the scrollback every time SSE backpressure cleared.It now requests full history, falling back to the tail only when
_replayWouldShrinkBufferrefuses the capture, which leaves repaint-mode panes (tmux holds roughly one frame for them) exactly as they were.#258 history truncation
Truncation was reported by a grey line written into the terminal, so it scrolled away with the output it described, could not be acted on, and read identically whether the rest was one click away or gone forever. Underneath, the route set one
truncatedboolean at two sites meaning opposite things, and the client discardedfullSizeandsourceentirely.The route now reports
truncationReason(tail= intentional partial replay, the rest is retained;capped= the byte ceiling dropped it) plusretainedBytes, andcappedis not downgraded by a later tail cut. The client renders a dismissible banner outside terminal output with three honest states: recoverable (offers Load full history), at-ceiling, and exhausted.The Load button forces past the scroll cooldown but not past
_replayWouldShrinkBuffer, so it still cannot destroy history on a repaint-mode pane.The banner is an overlay, not a flex child: FitAddon derives rows/cols from the terminal parent's computed height, so occupying real layout space would SIGWINCH the CLI on every truncation-state change.
Verification
Real browser against a live session on an isolated instance:
baseY869 -> 869 across a refresh, where it was 869 -> 158 beforeTwo things unit tests could not have caught:
--bg-elevatedand--accent-muted, which do not exist, so the fallbacks painted a hardcoded dark bar and the four light skins would have rendered dark text on it. It now uses only tokens every skin redefines.daylight-bluebecause thelocalStorageseed was not taking). Drivingdata-skindirectly produced 5 distinct backgrounds and a real result.npm run test:ci: 5061 passed. One pre-existing static guard interminal-scroll-routing.test.tsanchored on the empty_maybeRefetchFullHistory()signature; its invariant still holds, so the anchor is now arity-independent rather than weakened.Note for reviewers
test/mobile/keyboard.test.tscoupled to the renamed param at two call sites. That suite is excluded fromtest:ci, so this would have gone red behind two green checks, the same way #279/#280 landed broken. It is updated here, and the new regression tests live outsidetest/mobile/so CI can actually see them.The banner currently shows on phones as well as desktop, since #258 asks for an accessible persistent indicator and phones suffer truncation most. Easy to gate if that is not wanted.
No changeset, on the assumption this gets folded into a COM.