singularity/singularity-forge
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Merge pull request #3768 from jeremymcs/fix/tui-cursor-baseline-revert

Browse source 
fix(pi-tui): revert contentCursorRow — use hardwareCursorRow for movement baseline
This commit is contained in:
Jeremy McSpadden 2026-04-07 23:50:18 -05:00 • committed by GitHub
commit eb7e195cd5
3 changed files with 24 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
packages/pi-tui/src/tui.ts
View file

@ -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);

7
src/tests/tui-autocomplete-ghost-lines.test.ts
View file

@ -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])}`,
);
});
});

43
src/tests/tui-content-cursor-desync.test.ts
View file

@ -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",
);
});
});