fix(terminal): preserve row boundaries in alternate history preview - #3297
fix(terminal): preserve row boundaries in alternate history preview#3297binaricat wants to merge 10 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
binaricat
added
the
automation:codex-loop
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f18ac2de36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((command === "H" || command === "f") && (Number(params[1]) || 1) === 1) { | ||
| cursor = 0; | ||
| cursorCell = 0; | ||
| } |
There was a problem hiding this comment.
When an application positions output away from column 1, such as CSI 2;10Htext, this handler changes the tracked row but leaves the cursor at column 1; similarly, committing a line for A, B, or d resets the cursor even though those commands preserve the terminal column. Consequently, indented content and partial Vim/full-screen redraws appear left-shifted or overwrite the wrong text in the history preview. Apply the requested CUP/HVP column, and retain the existing column for vertical-only controls.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1e5a0fe49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const width = pieceCellWidth(current); | ||
| if (targetCell > width) current += " ".repeat(targetCell - width); |
There was a problem hiding this comment.
When a row-control sequence moves to a nonzero column without printing afterward, this padding makes the open line nonempty, so getLines() exposes a whitespace-only transcript row; repeated vertical moves also commit those spaces and accumulate blank history rows. This occurs with final cursor placement such as abc\x1b[2;4H, which currently yields ["abc", " "], so retain the logical column but materialize padding only when printable output is written.
Useful? React with 👍 / 👎.
| cursor = targetCell === 0 ? 0 | ||
| : isAsciiOnly(current) ? targetCell | ||
| : sliceStringByCellColumns(current, 0, targetCell).length; |
There was a problem hiding this comment.
When CUP/HVP positions the cursor before a multi-code-unit glyph and a shorter span overwrites it, this cursor index feeds writeSpan, which removes piece.length UTF-16 units rather than the overwritten glyph. For example, A😀B\x1b[1;2HX produces AX\ude00B, leaving an unpaired surrogate in the history preview; positioned writes need to replace complete grapheme/cell boundaries rather than splice by the incoming string length.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc90b0afd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : command === "B" || command === "E" | ||
| ? Math.min(1_000_000, screenRow + amount) | ||
| : amount; |
There was a problem hiding this comment.
When the cursor is already on the terminal's bottom row, CSI n B/E and absolute positions beyond the viewport are clamped by the terminal, but this tracker caps them only at 1,000,000 and therefore treats them as real row transitions. For example, in a 24-row terminal, CSI 24;1Hstatus CSI 1BNEW overwrites the status row on screen, while the preview calls commitCurrentLine() and retains status and NEW as separate lines. Track or receive the actual terminal row count so these same-row redraws are not appended as historical output.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02da767463
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // CUP/HVP set both coordinates; vertical-only moves keep the column. | ||
| // Keep placement logical until text arrives; cursor moves print no spaces. | ||
| // Bound eventual padding by the transcript budget. | ||
| const targetCell = Math.min(maxChars - 1, column); |
There was a problem hiding this comment.
When CUP/HVP requests a column beyond the terminal width, xterm clamps it to the last viewport column, but this clamps only to the much larger transcript budget. For example, CSI 1;999H X in an 80-column terminal places X at column 80 on screen while the preview inserts 998 spaces, creating many bogus wrapped rows and potentially evicting useful history. Capture the live column count alongside the row count and clamp targetCell to it.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
| if (ch === "\n") { | ||
| screenRow = Math.min(1_000_000, screenRow + 1); |
There was a problem hiding this comment.
When an LF occurs on the terminal's bottom row, xterm scrolls and leaves the cursor on that row, but this advances screenRow past viewportRows. A subsequent CUP back to the bottom row is therefore treated as a row transition and commits a stale redraw; with 24 rows, CSI 24;1Hstatus\r\nnew CSI 24;1HNEW retains new as a separate history line even though NEW overwrites it on screen. Clamp this update using the reported viewport size.
Useful? React with 👍 / 👎.
| setViewportRows(rows: number): void { | ||
| viewportRows = Math.max(0, Math.floor(rows)); |
There was a problem hiding this comment.
When the terminal is resized to fewer rows while its cursor is below the new bottom, xterm immediately clamps the cursor but this setter changes only viewportRows, leaving screenRow stale. If the next output uses CSI B/E, the tracker sees a transition from the old row to the new bottom and commits the current redraw even though the real cursor never moved. Clamp screenRow when updating the viewport row count.
Useful? React with 👍 / 👎.
| : command === "B" || command === "E" | ||
| ? Math.min(rowLimit, screenRow + amount) | ||
| : Math.min(rowLimit, amount); |
There was a problem hiding this comment.
When a full-screen application establishes a scroll region smaller than the viewport, xterm clamps CUD/CNL at that region's bottom, while this code clamps only at the viewport bottom and the parser discards the region-setting sequence. For example, after CSI 1;20r and CSI 20;1Hstatus, CSI 1BNEW redraws row 20 in xterm but is recorded as a transition to row 21, preserving a stale extra history line. Track the active scroll margins when interpreting these relative movements.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402c07c29f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| screenRow = Math.min(screenRow, viewportRows); | ||
| scrollBottomMargin = Math.min(scrollBottomMargin, viewportRows); |
There was a problem hiding this comment.
When the terminal grows after its initial size is reported, xterm resets the scrolling region to the full new viewport, but this preserves the old bottom margin. For example, after changing from 24 to 40 rows without any DECSTBM sequence, output at row 24 followed by CSI 1 B moves to row 25 in xterm, while the preview clamps at row 24 and concatenates the two rows. Detect a row-count change and reset both margins to the full viewport.
Useful? React with 👍 / 👎.
| setViewportCols(cols: number): void { | ||
| viewportCols = Math.max(0, Math.floor(cols)); |
There was a problem hiding this comment.
When a resize reduces the terminal width below cursorCell, xterm immediately clamps its cursor to the new last column, but this setter leaves both cursorCell and cursor at their old positions. Printable output arriving without another absolute cursor command is consequently written at the old column in the preview; resizing from 80 to 10 columns while at column 80, then printing a character, retains roughly 70 bogus spaces instead of writing at column 10.
Useful? React with 👍 / 👎.
| totalChars = 0; | ||
| pendingEscape = ""; | ||
| screenRow = 1; |
There was a problem hiding this comment.
When a terminal that emitted DECSTBM reconnects or boots again, useTerminalEffects calls clear() before creating the new xterm instance, whose scroll region starts at the full viewport. Because this reset leaves scrollTopMargin and scrollBottomMargin from the prior boot intact, relative row moves in the new session can be clamped to an obsolete margin and concatenate distinct rows; reset the margins along with screenRow.
Useful? React with 👍 / 👎.
| scrollTopMargin = Math.max(1, Number(params[0]) || 1); | ||
| scrollBottomMargin = Math.max( | ||
| scrollTopMargin, | ||
| Number(params[1]) || (viewportRows > 0 ? viewportRows : 1_000_000), | ||
| ); |
There was a problem hiding this comment.
When an application emits a scroll region whose bottom is not greater than its top, xterm ignores the sequence and does not home the cursor, whereas this code coerces the bottom to the top and applies it. For example, after writing on row 5, CSI 20;10r followed by more text remains on row 5 in xterm but causes the preview to commit the existing text, home to row 1, and start another history line. Validate the range using the viewport before changing margin or cursor state.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbe3726461
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cursorCell += pieceWidth; | ||
| currentCellWidth = Math.max(width, cursorCell); |
There was a problem hiding this comment.
When printable output crosses the reported viewport width, xterm advances to the next screen row and wraps the cursor column, but this only increments cursorCell and leaves screenRow unchanged. With a five-column viewport, abcdef\x1b[2;1HXY overwrites the wrapped f on xterm's second row, while the preview produces abcdef plus a separate XY line, leaving the stale f as an extra wrapped row. Update the tracked row and column for xterm's automatic-wrap behavior before interpreting subsequent cursor controls.
Useful? React with 👍 / 👎.
| if (preserveRowControls | ||
| && (input[i] === C1_CSI || input[i + 1] === "[") | ||
| && /^[0-9;]*[HfABEFdr]$/.test(input.slice(i + escapeIntroducerLength(input, i), end))) { |
There was a problem hiding this comment.
When a full-screen application enables DEC origin mode with CSI ?6h, CUP/HVP rows are relative to and clamped within the active scroll region, but this whitelist discards the mode change and later treats those rows as viewport-relative. For example, in a 24-row terminal, CSI 5;20r CSI ?6h CSI 16;1Hstatus CSI 99;1HNEW targets row 20 twice in xterm, whereas the preview resolves the second position to row 24 and retains status as a separate stale line. Track origin-mode changes and resolve absolute row controls against the margins while it is enabled.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49d98da9b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (screenRow !== 1 && current) commitCurrentLine(); | ||
| screenRow = 1; |
There was a problem hiding this comment.
When DECOM is enabled before the scroll region is changed, xterm homes the cursor at the new top margin, but this always records row 1. For example, CSI ?6h CSI 5;20r status CSI 1;1H NEW writes and redraws row 5 in xterm, while the preview commits status and emits NEW as a separate line. The current DECOM support therefore needs to use scrollTopMargin as the DECSTBM home row whenever originMode is active.
Useful? React with 👍 / 👎.
| // xterm defers the wrap until the next printable character arrives; a | ||
| // cursor move or carriage return in between cancels it instead. | ||
| if (viewportCols > 0 && cursorCell >= viewportCols) wrapCursor(); |
There was a problem hiding this comment.
When an application sends CSI ?7l to disable DECAWM, xterm keeps subsequent printable characters on the current row, but the stripper drops that mode and this new check still wraps unconditionally. With a five-column viewport, CSI ?7l abcdef CSI 1;1H XY leaves XYcdf on xterm's first row, whereas the preview produces abcde, f, and XY as three lines. Fresh evidence in the current head is this unconditional wrapCursor() path added for the earlier autowrap issue; track ?7h/?7l and gate wrapping on that mode.
Useful? React with 👍 / 👎.
| if (preserveRowControls | ||
| && (input[i] === C1_CSI || input[i + 1] === "[") | ||
| && (/^[0-9;]*[HfABEFdr]$/.test(sequence) || /^\?6[hl]$/.test(sequence))) { | ||
| output += input.slice(i, end); |
There was a problem hiding this comment.
When a full-screen application uses ESC 7/ESC 8 or CSI s/CSI u, these controls are discarded even though they can restore a different row. For example, CSI 2;1H foo ESC 7 CSI 3;1H bar ESC 8 NEW writes fooNEW on row 2 and bar on row 3 in xterm, but the preview returns foo and barNEW because it still believes the cursor is on row 3. Preserve and interpret the save/restore controls so output after a restored row is not concatenated onto the wrong transcript line.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b95fe52392
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // instead of wrapping to the next row. | ||
| fitting = piece; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
When DECAWM is disabled and a wide glyph does not fit in the remaining columns, this branch sets fitting = piece but then immediately continues without advancing offset or changing the cursor. For example, with a five-column viewport, appending CSI ?7l followed by abcd中 loops forever while processing the glyph, synchronously freezing the terminal renderer. Continue only after wrapping, or let the no-autowrap branch proceed to write and consume the glyph.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a3f0b496b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (viewportCols > 0) { | ||
| const room = viewportCols - cursorCell; | ||
| fitting = spanIsAscii | ||
| ? piece.slice(0, room) | ||
| : sliceStringByCellColumns(piece, 0, room); |
There was a problem hiding this comment.
When a tab stop lies beyond the right edge, the new viewport cap processes the expanded tab padding as printable spaces and calls wrapCursor(), although xterm clamps a tab to the last column without wrapping. For example, with five columns, a\tb renders as a b on one terminal row, while the preview records ["a ", " b"]; tabs emitted near the right edge therefore create spurious history rows and make later cursor addressing target the wrong row.
Useful? React with 👍 / 👎.
| if (savedCursor.row !== screenRow && current) commitCurrentLine(); | ||
| screenRow = savedCursor.row; |
There was a problem hiding this comment.
When a saved cursor returns to an earlier row after another row has been written, this commits the active row but leaves current empty rather than recovering the saved row's contents. Fresh evidence in the current implementation is that CSI 2;1H foo ESC 7 CSI 3;1H bar ESC 8 NEW now produces foo, bar, and a padded NEW line, whereas xterm has fooNEW on row 2 and bar on row 3; the newly preserved restore control therefore still creates stale and duplicate preview rows.
Useful? React with 👍 / 👎.
| const saveTrackedCursor = () => { | ||
| savedCursor = { row: screenRow, cell: cursorCell }; |
There was a problem hiding this comment.
When DECAWM or DECOM changes between a cursor save and restore, xterm restores the saved mode state along with the coordinates, but this record stores only row and cell. In a five-column terminal, CSI ?7l ESC 7 CSI ?7h ESC 8 abcdef keeps wrapping disabled and renders abcdf on one row, while the preview leaves autowrap enabled and records abcde plus f on a second row, so subsequent row tracking diverges.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f721002829
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bottomLimit = screenRow <= scrollBottomMargin | ||
| ? Math.min(scrollBottomMargin, screenBottom) | ||
| : screenBottom; | ||
| commitCurrentLine(); |
There was a problem hiding this comment.
When printable output exceeds the live viewport width, this commits the wrapped row as an ordinary transcript line. Consequently, a five-column capture of abcdef produces preview rows with wrap flags [false, false] rather than [false, true]; joinHistoryPreviewSelectionText then treats the boundary as a hard newline, so copying the alternate-history preview inserts characters that were not in the output, and widening the preview cannot reflow the line. Preserve whether a committed row resulted from automatic wrapping so the continuation remains marked as soft-wrapped.
Useful? React with 👍 / 👎.
| if (viewportCols > 0 && cursorCell >= viewportCols) { | ||
| if (autowrap) { | ||
| wrapCursor(); |
There was a problem hiding this comment.
When a base character fills the last column and its combining mark arrives in the next display chunk, this unconditional pending-wrap handling runs before inspecting the new grapheme. Xterm retains its preceding-join state and attaches the zero-width mark to the final cell without wrapping, but appending abcde and then ́Z at five columns yields ["abcde", "́Z"] here instead of rows containing abcdé and Z. Delay the wrap decision until determining whether the next grapheme joins the preceding cell.
Useful? React with 👍 / 👎.
|
No description provided. |
2 participants
Summary
Shift+PageUp inside Vim could show separate lines concatenated in Netcatty's alternate-screen history preview. Real Vim output positions consecutive lines with cursor-addressing sequences rather than always emitting newlines; the history collector discarded those positions. Preserve row transitions in the transcript so those lines remain separate, while same-row home redraws replace the current progress line.
Type of Change
Related Issue (optional)
Follow-up to #3165, related to #2516. Discovered during post-v1.1.82 development-app smoke testing.
Changes Made
Screenshots / Demo
In the actual
npm run devapplication, opened a synthetic 240-line file withvim -Nu NONE -i NONE -n -R, paged forward three times, then used Shift+PageUp to return to earlier output. Before the fix, rows 002/003/004 appeared concatenated. After a fresh terminal with the fix, each row appeared separately. Escape returned to the same live Vim page, and Vim exited normally. Also captured real Vim PTY output and replayed it through the production collector in 1/7/1024-byte and whole-stream chunks.This remains a bounded text transcript, not an exact replay of every full-screen layout.
Testing
npm run dev)npm run lint)npm test)npm run generate:capability-tools)The earlier 66 focused history/runtime tests passed; the latest two review regressions and history/scroll tests pass (41 tests in this targeted run). the new regression assertions fail before the fix. Full suite was not rerun locally. Capability generation is not applicable.
A local parser-only comparison alternated versions for 10 measured runs after warm-up (10,000 chunks per run): median plain output ~30.5ms before /29.5ms after; ANSI-colored output ~18.0/18.9ms; cursor-addressed rows ~3.1/9.7ms. The latter now performs row handling previously omitted. These are microbenchmarks, not end-to-end throughput claims. Development hot reload invalidated existing test-terminal callbacks, so final GUI validation used a fresh terminal after reload.
Checklist
Review follow-up: preserved column positioning, added two regression cases (including wide characters and bounded padding), and repeated actual development-app Vim validation. A comparison against the first fix revision showed comparable parser times: plain30.13/30.05ms, ANSI18.67/18.69ms, row-addressed9.36/9.26ms per10,000chunks.
Latest review follow-up: cursor-only placement now retains its column without creating whitespace transcript rows. Positioned overwrites replace complete graphemes and leave spaces in the remaining cells of a partially overwritten wide glyph. Both reported examples fail before this follow-up and pass afterward; emoji, ZWJ, CJK, combining marks, and repeated cursor-only moves are covered. Full lint passed, and the final development-app Vim history preview was rechecked in a fresh terminal. Cached line width avoids rescanning growing Unicode lines on every chunk. Ten alternating measured parser runs against the prior revision, 10,000 chunks each: Unicode chunks 26.36/26.71ms, plain41.06/41.51ms, ANSI26.44/26.89ms, cursor rows11.28/11.29ms (before/after). Latest CI and review are pending.
Latest smoke follow-up (7a3f0b4)
The reviewed history-capture changes introduced a renderer-blocking loop when automatic wrapping is disabled and a wide glyph reaches the last column. A five-column capture of
abcdfollowed by a Chinese character reproducibly timed out before this fix. Capture now consumes the unprintable grapheme, matching the bundled xterm handling, and continues processing following characters.Latest validation (f721002)
Repeated unchanged viewport-size reports were also cancelling deferred wrap between chunks (
abcdethenfat five columns becameabcdf). The setter now preserves the pending wrap when dimensions are unchanged. Non-ASCII spans that fit are measured once without repeated per-grapheme slicing.npm run devon macOS: ten right-edge Chinese/joined-emoji runs complete; following ASCII remains at the last column, andNOWRAP_WIDE_ALIVEand split-chunkSPLIT_WRAP_OKappear.vim-nowrap-wide-live.png,vim-latest-preview-page.png,vim-latest-return-live.png.