From 51a33abf2acac372e38dffd77aee16097a8dd6ae Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Sat, 23 May 2026 12:48:31 -0300 Subject: [PATCH] fix: ghost cursor at (0,0) (#122) and ESC k title leak (#153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundles two renderer/parser fixes that don't share scope: **#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. **#153 — ESC k title sequence leaks onto the grid.** Ghostty WASM (commit 5714ed07) does not consume `ESC k ESC \` — the GNU screen / tmux title-setting extension. The parser logs `unimplemented ESC action: ESC k` and then prints `` 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 (https://github.com/coder/ghostty-web/issues/122) Reported-by: Fisher-Wang (https://github.com/coder/ghostty-web/issues/153) --- lib/renderer.ts | 21 ++++++---- lib/terminal.test.ts | 98 ++++++++++++++++++++++++++++++++++++++++++++ lib/terminal.ts | 61 ++++++++++++++++++++++++++- 3 files changed, 172 insertions(+), 8 deletions(-) diff --git a/lib/renderer.ts b/lib/renderer.ts index e6cbe8f..02e2ff1 100644 --- a/lib/renderer.ts +++ b/lib/renderer.ts @@ -324,13 +324,20 @@ export class CanvasRenderer { this.renderLine(line, cursor.y, dims.cols); } } - if (cursorMoved && this.lastCursorPosition.y !== cursor.y) { - // Also redraw old cursor line if cursor moved to different line - if (!forceAll && !buffer.isRowDirty(this.lastCursorPosition.y)) { - const line = buffer.getLine(this.lastCursorPosition.y); - if (line) { - this.renderLine(line, this.lastCursorPosition.y, dims.cols); - } + 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); } } } diff --git a/lib/terminal.test.ts b/lib/terminal.test.ts index 1ed8440..106c648 100644 --- a/lib/terminal.test.ts +++ b/lib/terminal.test.ts @@ -3090,3 +3090,101 @@ describe('preserveScrollOnWrite option', () => { term.dispose(); }); }); + +describe('ESC k title sequence (issue #153)', () => { + let container: HTMLElement | null = null; + + beforeEach(() => { + if (typeof document !== 'undefined') { + container = document.createElement('div'); + document.body.appendChild(container); + } + }); + + afterEach(() => { + if (container && container.parentNode) { + container.parentNode.removeChild(container); + container = null; + } + }); + + test('ESC k ESC \\ does not leak the title payload onto the grid', async () => { + if (!container) return; + + const term = await createIsolatedTerminal({ cols: 80, rows: 24 }); + term.open(container); + + // GNU screen / tmux title-set: ESC k /tmp ESC \ then ESC k ls ESC \ + // then the actual visible content. Before the strip pass landed, + // /tmp leaked onto row 0 and "ls" merged with the next line. + term.write('\x1bk/tmp\x1b\\\x1bkls\x1b\\demo.txt\r\n'); + + const line0 = term.wasmTerm!.getLine(0); + const text0 = line0 + .map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : '')) + .join('') + .trimEnd(); + expect(text0).toBe('demo.txt'); + expect(text0).not.toContain('/tmp'); + expect(text0).not.toContain('ls'); + + term.dispose(); + }); + + test('ESC k variant terminated by BEL is also stripped', async () => { + if (!container) return; + + const term = await createIsolatedTerminal({ cols: 80, rows: 24 }); + term.open(container); + + term.write('\x1bktitle\x07after\r\n'); + + const line0 = term.wasmTerm!.getLine(0); + const text0 = line0 + .map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : '')) + .join('') + .trimEnd(); + expect(text0).toBe('after'); + + term.dispose(); + }); + + test('OSC 0 title-set continues to be consumed by the WASM parser', async () => { + if (!container) return; + + const term = await createIsolatedTerminal({ cols: 80, rows: 24 }); + term.open(container); + + // OSC 0 ; BEL — handled by WASM. The strip pass should not + // touch this sequence. + term.write('\x1b]0;mywindow\x07visible\r\n'); + + const line0 = term.wasmTerm!.getLine(0); + const text0 = line0 + .map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : '')) + .join('') + .trimEnd(); + expect(text0).toBe('visible'); + + term.dispose(); + }); + + test('Uint8Array input is stripped equivalently to string input', async () => { + if (!container) return; + + const term = await createIsolatedTerminal({ cols: 80, rows: 24 }); + term.open(container); + + const bytes = new TextEncoder().encode('\x1bktitle\x1b\\done\r\n'); + term.write(bytes); + + const line0 = term.wasmTerm!.getLine(0); + const text0 = line0 + .map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : '')) + .join('') + .trimEnd(); + expect(text0).toBe('done'); + + term.dispose(); + }); +}); diff --git a/lib/terminal.ts b/lib/terminal.ts index 902ea94..c4145e0 100644 --- a/lib/terminal.ts +++ b/lib/terminal.ts @@ -553,6 +553,61 @@ export class Terminal implements ITerminalCore { this.writeInternal(data, callback); } + /** + * Strip unimplemented escape sequences that Ghostty WASM does not consume + * cleanly. The current parser (5714ed07) prints the inner text of + * `ESC k <text> ESC \` (screen/tmux title set) onto the grid instead of + * silently consuming it — the same is true for any 7-bit terminator + * (`BEL`). We pre-filter the input so those titles don't leak as visible + * text. Issue: coder/ghostty-web#153. + * + * Only `ESC k …` is stripped. OSC sequences (`ESC ] …`) already work in + * the WASM parser and are untouched. + */ + 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); + } + /** * Internal write implementation (extracted from write()) */ @@ -561,6 +616,10 @@ export class Terminal implements ITerminalCore { // preserve selection when new data arrives. Selection is cleared by user actions // like clicking or typing, not by incoming data. + // Strip unimplemented escape sequences (e.g. ESC k …) that would + // otherwise leak their payload onto the grid. See issue #153. + const sanitized = this.stripUnimplementedTitleSequences(data); + // Save scroll state before writing, ONLY when preserveScrollOnWrite is // active. viewportY is relative to the bottom, so if new lines push // content into scrollback we need to bump viewportY by the same amount @@ -571,7 +630,7 @@ export class Terminal implements ITerminalCore { preserveScroll && savedViewportY > 0 ? this.wasmTerm!.getScrollbackLength() : 0; // Write directly to WASM terminal (handles VT parsing internally) - this.wasmTerm!.write(data); + this.wasmTerm!.write(sanitized); // Process any responses generated by the terminal (e.g., DSR cursor position) // These need to be sent back to the PTY via onData