fix: ghost cursor at (0,0) and ESC k title-set payload leak#16
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses two main issues: a 'ghost cursor' bug and the leaking of 'ESC k' title sequences onto the terminal grid. In lib/renderer.ts, the logic was updated to always redraw the old cursor row when the cursor moves, ensuring the previous glyph is erased. In lib/terminal.ts, a stripUnimplementedTitleSequences method was introduced to filter out title sequences before they reach the WASM parser. Feedback indicates that the filtering logic is stateless and susceptible to failure if sequences are split across data chunks. Additionally, the renderer change introduces redundant draws for horizontal cursor movements, and there is a logic inconsistency between the string and byte-level implementations of the sequence filter.
| private stripUnimplementedTitleSequences(data: string | Uint8Array): string | Uint8Array { | ||
| if (typeof data === 'string') { | ||
| // ESC = \x1b, ST = \x1b\x5c (ESC followed by backslash), BEL = \x07 | ||
| return data.replace(/\x1bk[^\x1b\x07]*(?:\x1b\\|\x07)/g, ''); | ||
| } | ||
| // Byte-level scan for Uint8Array. We only allocate a copy when we | ||
| // actually find a sequence to strip. | ||
| let i = 0; | ||
| let writeIdx = -1; | ||
| let out: Uint8Array | null = null; | ||
| while (i < data.length) { | ||
| if (data[i] === 0x1b && i + 1 < data.length && data[i + 1] === 0x6b) { | ||
| // Found ESC k — scan forward to ESC \ or BEL | ||
| let j = i + 2; | ||
| while (j < data.length) { | ||
| if (data[j] === 0x07) { | ||
| j++; | ||
| break; | ||
| } | ||
| if (data[j] === 0x1b && j + 1 < data.length && data[j + 1] === 0x5c) { | ||
| j += 2; | ||
| break; | ||
| } | ||
| // No terminator yet — keep scanning (handles split writes if WASM | ||
| // ever assembles them; defensively bail if we hit another ESC k). | ||
| j++; | ||
| } | ||
| if (out === null) { | ||
| out = new Uint8Array(data.length); | ||
| out.set(data.subarray(0, i)); | ||
| writeIdx = i; | ||
| } | ||
| i = j; | ||
| continue; | ||
| } | ||
| if (out !== null) { | ||
| out[writeIdx++] = data[i]; | ||
| } | ||
| i++; | ||
| } | ||
| if (out === null) return data; | ||
| return out.subarray(0, writeIdx); | ||
| } |
There was a problem hiding this comment.
The stripUnimplementedTitleSequences method is stateless and does not handle escape sequences split across multiple write() calls. If a chunk ends mid-sequence, the remainder of the payload will be passed to the WASM terminal as plain text in the next call, leading to the "leak" this PR intends to fix. For a robust solution, this filtering should be integrated into a stateful parser or the WASM parser itself should be updated to handle these sequences.
| if (data[j] === 0x1b && j + 1 < data.length && data[j + 1] === 0x5c) { | ||
| j += 2; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This check for the ST terminator (ESC \) will fail if the sequence is split across chunks such that the ESC is at the end of the current Uint8Array. In this case, the ESC will be incorrectly treated as part of the payload and stripped, and the subsequent \ in the next chunk will be passed to the terminal as literal text, potentially corrupting the output stream.
| if (cursorMoved && !forceAll) { | ||
| // Always redraw the OLD cursor row to erase the previous cursor | ||
| // glyph, whether or not the row is dirty and whether or not it | ||
| // differs from the new cursor row (issue #122: ghost cursor | ||
| // persisted at the initial (0,0) position because the prior | ||
| // logic skipped the redraw when the row was already dirty — | ||
| // assuming the regular dirty pass would handle it — but the | ||
| // regular dirty pass only runs when buffer cells changed, not | ||
| // when the cursor moved across unchanged cells. A double redraw | ||
| // when the row is both dirty AND cursor-moved is a trivial perf | ||
| // cost compared to the visual correctness gain.). | ||
| const line = buffer.getLine(this.lastCursorPosition.y); | ||
| if (line) { | ||
| this.renderLine(line, this.lastCursorPosition.y, dims.cols); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic results in redundant rendering when the cursor moves horizontally on the same row. In this case, cursor.y and this.lastCursorPosition.y are identical. The preceding block (lines 309-315) already ensures the row is rendered if it is not dirty. By removing the row inequality check, this row is now rendered twice in a single frame. Adding a check to ensure the rows are different would be more efficient.
if (cursorMoved && !forceAll && this.lastCursorPosition.y !== cursor.y) {
// Redraw the OLD cursor row to erase the previous cursor glyph
// when the cursor has moved to a different row. If the cursor
// moved horizontally on the same row, that row is already
// handled by the redraw logic above or the regular dirty pass.
const line = buffer.getLine(this.lastCursorPosition.y);
if (line) {
this.renderLine(line, this.lastCursorPosition.y, dims.cols);
}
}| private stripUnimplementedTitleSequences(data: string | Uint8Array): string | Uint8Array { | ||
| if (typeof data === 'string') { | ||
| // ESC = \x1b, ST = \x1b\x5c (ESC followed by backslash), BEL = \x07 | ||
| return data.replace(/\x1bk[^\x1b\x07]*(?:\x1b\\|\x07)/g, ''); |
There was a problem hiding this comment.
The regex used for string input ([^\x1b\x07]*) will stop at any ESC character, even if it is not followed by a backslash (the ST terminator). In contrast, the Uint8Array implementation (lines 577-589) correctly continues scanning until it finds a valid ST (ESC \) or BEL. This inconsistency means that certain payloads containing internal escape sequences will be handled differently depending on the input type.
| return data.replace(/\x1bk[^\x1b\x07]*(?:\x1b\\|\x07)/g, ''); | |
| return data.replace(/\x1bk.*?(?:\x1b\\|\x07)/gs, ''); |
Bundles two renderer/parser fixes that don't share scope: **coder#122 — ghost cursor at (0,0) on init.** The renderer skipped redrawing the previous cursor row when the new cursor stayed on the SAME row as the previous frame. The cursor-line redraw at the top of the cursor- moved block only fires for the NEW cursor row; the symmetric branch for the OLD cursor row had a `lastCursorPosition.y !== cursor.y` guard that skipped same-row moves and an `!isRowDirty` guard that skipped any move where the regular dirty pass was already going to redraw the row. The combination left a stale cursor glyph at the initial (0,0) position whenever later content moved the cursor on the same row via positional sequences (no cell content changing on row 0). Always redrawing the previous cursor row on cursorMoved is a trivial extra-render cost and guarantees the ghost is erased. **coder#153 — ESC k title sequence leaks onto the grid.** Ghostty WASM (commit 5714ed07) does not consume `ESC k <text> ESC \` — the GNU screen / tmux title-setting extension. The parser logs `unimplemented ESC action: ESC k` and then prints `<text>` onto the grid, also consuming the trailing `ESC \`. Same for the BEL-terminated variant. We pre-filter input in `Terminal.write` to strip ESC k sequences before they reach WASM. Implemented for both `string` and `Uint8Array` (the Uint8Array path does a single-pass byte scan and only allocates when a sequence is actually found). OSC 0/1/2 title-setting (`ESC ] …`) is untouched and continues to be consumed by the WASM parser as before. Adds four regression tests for the title-set behaviour (string input with ST, BEL terminator, OSC 0 untouched, Uint8Array equivalence). The cursor-ghost fix is structural and cannot be asserted in a headless render context; manual smoke test pending in bun run dev. Reported-by: mats16 (coder#122) Reported-by: Fisher-Wang (coder#153)
diegosouzapw
force-pushed
the
fix/upstream-issues-batch-renderer
branch
from
7899e08 to
51a33ab
Compare
|
1 participant
Summary
Bundles two renderer/parser fixes for unrelated upstream issues, ported as a single PR because the changes don't touch overlapping files.
coder#122 — Ghost cursor stuck at (0,0)
The renderer skipped redrawing the previous cursor row in two cases:
lastCursorPosition.y === cursor.y(cursor moved on the same row)buffer.isRowDirty(lastCursorPosition.y)(relied on the regular dirty pass)Both shortcuts left a stale cursor glyph at the initial
(0,0)position whenever later content moved the cursor on the same row using positional sequences (no cell content changing). Always redrawing the previous cursor row oncursorMovedis a trivial extra-render cost and guarantees the ghost is erased.coder#153 —
ESC k <text> ESC \leaks payload as visible textGhostty WASM (commit
5714ed07) does not consume the GNU screen / tmux title-setting sequence. The parser logsunimplemented ESC action: ESC kand then prints<text>onto the grid plus consumes the trailingESC \.Fix: pre-filter input in
Terminal.writeto stripESC k …(string andUint8Arraypaths) before reaching WASM. OSC 0/1/2 title-set (ESC ] …) is untouched.Attribution
ESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text coder/ghostty-web#153: thanks to @Fisher-Wang for the standalone reproTest plan
bun run fmt && bun run lint && bun run typecheckbun test— 335 tests pass (4 new regression tests forESC k <text> ESC \(screen/tmux title sequence) leaks payload as visible text coder/ghostty-web#153 cover ST terminator, BEL terminator, OSC 0 not-stripped, Uint8Array equivalence), 0 failbun run build:libbun run devfor Ghost cursor appears at top-left (0,0) on initialization coder/ghostty-web#122 (cursor visual cannot be asserted headless): write text + reposition cursor via\x1b[5;10Hand confirm only one cursor visible at the new positionRisk
Low. The coder#122 fix adds at most one extra row-redraw per cursor move (negligible). The coder#153 strip runs a regex on every write payload but only matches in the rare
ESC kpath; OSC and other sequences pass through untouched.Related issues observed as also-resolved by prior PRs
Posted as a separate rollup at
_tasks/upstream-issues/_rollup-2026-05-23.md(gitignored). Notably coder#126 (line-drawing char gap) is the same root cause as #146a (font metric device-pixel alignment), already shipped in PR #6 — no new code needed.