From c0236254a215e380043fee7a83e2612a55185101 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 7 Apr 2026 23:37:52 -0500 Subject: [PATCH] fix(pi-tui): revert contentCursorRow, use hardwareCursorRow as movement baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #3744 and #3765 introduced contentCursorRow which diverges from the actual terminal cursor position after IME repositioning. computeLineDiff computes ANSI escape movements which are relative to where the cursor physically is — that must be hardwareCursorRow, not a phantom position. Remove contentCursorRow entirely and revert computeLineDiff baseline to hardwareCursorRow. The ghost-line test was asserting wrong movement direction (UP from phantom position vs DOWN from actual cursor). Closes #3764 --- packages/pi-tui/src/tui.ts | 10 +---- .../tui-autocomplete-ghost-lines.test.ts | 7 ++- src/tests/tui-content-cursor-desync.test.ts | 43 ++++++++----------- 3 files changed, 24 insertions(+), 36 deletions(-) diff --git a/packages/pi-tui/src/tui.ts b/packages/pi-tui/src/tui.ts index b0240aad3..7a078f2d3 100644 --- a/packages/pi-tui/src/tui.ts +++ b/packages/pi-tui/src/tui.ts @@ -239,7 +239,6 @@ export class TUI extends Container { public onDebug?: () => void; private renderRequested = false; private cursorRow = 0; // Logical cursor row (end of rendered content) - private contentCursorRow = 0; // Cursor row after content rendering, before IME repositioning private hardwareCursorRow = 0; // Actual terminal cursor row (may differ due to IME positioning) private inputBuffer = ""; // Buffer for parsing terminal responses private cellSizeQueryPending = false; @@ -499,7 +498,6 @@ export class TUI extends Container { this.previousWidth = -1; // -1 triggers widthChanged, forcing a full clear this.previousHeight = -1; // -1 triggers heightChanged, forcing a full clear this.cursorRow = 0; - this.contentCursorRow = 0; this.hardwareCursorRow = 0; this.maxLinesRendered = 0; this.previousViewportTop = 0; @@ -618,7 +616,7 @@ export class TUI extends Container { const height = this.terminal.rows; let viewportTop = Math.max(0, this.maxLinesRendered - height); let prevViewportTop = this.previousViewportTop; - let hardwareCursorRow = this.contentCursorRow; + let hardwareCursorRow = this.hardwareCursorRow; const computeLineDiff = (targetRow: number): number => { const currentScreenRow = hardwareCursorRow - prevViewportTop; const targetScreenRow = targetRow - viewportTop; @@ -665,7 +663,6 @@ export class TUI extends Container { buffer += "\x1b[?2026l"; // End synchronized output this.terminal.write(buffer); this.cursorRow = Math.max(0, newLines.length - 1); - this.contentCursorRow = this.cursorRow; this.hardwareCursorRow = this.cursorRow; // Reset max lines when clearing, otherwise track growth if (clear) { @@ -773,7 +770,6 @@ export class TUI extends Container { buffer += "\x1b[?2026l"; this.terminal.write(buffer); this.cursorRow = targetRow; - this.contentCursorRow = targetRow; this.hardwareCursorRow = targetRow; } this.positionHardwareCursor(cursorPos, newLines.length); @@ -891,10 +887,8 @@ export class TUI extends Container { // Track cursor position for next render // cursorRow tracks end of content (for viewport calculation) - // contentCursorRow tracks cursor after content rendering (before IME repositioning) - // hardwareCursorRow tracks actual terminal cursor position (may differ due to IME) + // hardwareCursorRow tracks actual terminal cursor position (for movement) this.cursorRow = Math.max(0, newLines.length - 1); - this.contentCursorRow = finalCursorRow; this.hardwareCursorRow = finalCursorRow; // Track terminal's working area (grows but doesn't shrink unless cleared) this.maxLinesRendered = Math.max(this.maxLinesRendered, newLines.length); diff --git a/src/tests/tui-autocomplete-ghost-lines.test.ts b/src/tests/tui-autocomplete-ghost-lines.test.ts index 2faf22247..601692e2a 100644 --- a/src/tests/tui-autocomplete-ghost-lines.test.ts +++ b/src/tests/tui-autocomplete-ghost-lines.test.ts @@ -77,9 +77,12 @@ describe("TUI autocomplete shrink clearing (#3721)", () => { (tui as any).doRender(); assert.ok(terminal.writtenData.length >= 1, "shrink render should write a differential buffer"); + // After IME positioning, cursor is at row 1 (CURSOR_MARKER line). + // To clear deleted rows 4-5, cursor must move DOWN to content bottom (row 3), + // then clear the extra lines below. Movement is relative to actual cursor position. assert.ok( - terminal.writtenData[0].startsWith("\x1b[?2026h\x1b[2A\r"), - `expected shrink diff to move up from prior content bottom, got ${JSON.stringify(terminal.writtenData[0])}`, + terminal.writtenData[0].startsWith("\x1b[?2026h\x1b[2B\r"), + `expected shrink diff to move down from IME cursor to content bottom, got ${JSON.stringify(terminal.writtenData[0])}`, ); }); }); diff --git a/src/tests/tui-content-cursor-desync.test.ts b/src/tests/tui-content-cursor-desync.test.ts index 1fa31148e..dd840ed5d 100644 --- a/src/tests/tui-content-cursor-desync.test.ts +++ b/src/tests/tui-content-cursor-desync.test.ts @@ -1,10 +1,10 @@ /** * Regression test for #3764: TUI input clears and jumps up after PR #3744. * - * PR #3744 used this.cursorRow (content end) as the movement baseline in - * computeLineDiff, but it should be the post-render cursor position - * (finalCursorRow). This test verifies that after IME cursor repositioning, - * the next render computes correct movement deltas — no spurious jumps. + * PR #3744 introduced contentCursorRow which diverged from the actual terminal + * cursor position, causing computeLineDiff to compute wrong movement deltas. + * The fix reverts to using hardwareCursorRow (actual cursor position) as the + * baseline for all cursor movement calculations. */ import { describe, it } from "node:test"; @@ -59,7 +59,7 @@ class DynamicLinesComponent implements Component { invalidate(): void {} } -describe("TUI contentCursorRow tracking (#3764)", () => { +describe("TUI cursor tracking regression (#3764)", () => { it("does not produce spurious cursor jumps when content changes after IME positioning", () => { const terminal = new MockTTYTerminal(); const tui = new TUI(terminal, false); @@ -72,18 +72,11 @@ describe("TUI contentCursorRow tracking (#3764)", () => { tui.addChild(component); (tui as any).doRender(); - // After first render, hardwareCursorRow is at IME position (row 1), - // but contentCursorRow should be at finalCursorRow (row 2, end of content). - // Verify contentCursorRow is set correctly. - assert.strictEqual( - (tui as any).contentCursorRow, - 2, - "contentCursorRow should be at content end (row 2) after first render", - ); + // After first render, hardwareCursorRow is at IME position (row 1) assert.strictEqual( (tui as any).hardwareCursorRow, 1, - "hardwareCursorRow should be at IME cursor position (row 1) after positionHardwareCursor", + "hardwareCursorRow should be at IME cursor position (row 1)", ); // Simulate typing — content changes on the same line @@ -96,14 +89,10 @@ describe("TUI contentCursorRow tracking (#3764)", () => { (tui as any).doRender(); - // The differential render should update line 1 (the changed input line). - // With the bug from PR #3744, computeLineDiff would use this.cursorRow (2) - // instead of contentCursorRow (2), which happened to be the same — but the - // critical test is that the buffer does NOT contain large cursor jumps. assert.ok(terminal.writtenData.length >= 1, "typing should trigger a render"); const buffer = terminal.writtenData[0]; - // Should not contain \x1b[2A or \x1b[3A etc. (large upward jumps) + // Should not contain large upward jumps (3+ rows) const largeUpJump = buffer.match(/\x1b\[([3-9]|\d{2,})A/); assert.strictEqual( largeUpJump, @@ -112,7 +101,7 @@ describe("TUI contentCursorRow tracking (#3764)", () => { ); }); - it("contentCursorRow persists correctly across renders with shrinking content", () => { + it("hardwareCursorRow tracks actual terminal position through IME and shrink", () => { const terminal = new MockTTYTerminal(); const tui = new TUI(terminal, false); const component = new DynamicLinesComponent([ @@ -126,10 +115,11 @@ describe("TUI contentCursorRow tracking (#3764)", () => { tui.addChild(component); (tui as any).doRender(); + // After IME positioning, hardwareCursorRow is at CURSOR_MARKER line (row 1) assert.strictEqual( - (tui as any).contentCursorRow, - 4, - "contentCursorRow should be 4 after rendering 5 lines", + (tui as any).hardwareCursorRow, + 1, + "hardwareCursorRow should be at IME position (row 1) after first render", ); // Shrink content @@ -142,10 +132,11 @@ describe("TUI contentCursorRow tracking (#3764)", () => { (tui as any).doRender(); + // After shrink, hardwareCursorRow should be at IME position again assert.strictEqual( - (tui as any).contentCursorRow, - 2, - "contentCursorRow should update to 2 after shrinking to 3 lines", + (tui as any).hardwareCursorRow, + 1, + "hardwareCursorRow should be at IME position after shrink render", ); }); });