From 1c0e59e557d00476e1ac0a35ceeb611e17533761 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sun, 17 May 2026 11:34:06 -0500 Subject: [PATCH] review(tui): address Copilot feedback on cursorLayout wrap-ansi rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small follow-ups from the Copilot review on #27489: 1. Declare `wrap-ansi` as a direct dependency of `ui-tui`. It was a phantom dep that resolved via npm hoisting from `@hermes/ink`'s transitive graph — fine on hoisted installs, but breaks under pnpm or `npm install --no-install-strategy=hoisted` style isolated installs. Now listed as `"wrap-ansi": "^9.0.0"` matching the @hermes/ink version. Lockfile regenerated. 2. Implement the defensive resync the comment promised. Previously the comment claimed the loop would "fall back to advancing by one to stay in lockstep" on wrap-ansi desync, but the code unconditionally advanced `originalIdx` with no actual check — so any future wrap-ansi option change or styled-input caller could silently slide `originalIdx` past the end of `value` and emit garbage line ranges. Now actually compares `value[originalIdx] === ch`, re-syncs via `indexOf` on mismatch, and bails out (returning whatever was built so far) if the desync is unrecoverable. Production paths still hit the equality fast-path on every char. 3. Drop the `visualLines` wrapper. It was a one-line indirection over `visualLinesFromWrappedOutput`. Renamed the implementation to `visualLines` and removed the wrapper — same name, no extra layer. No behavior change beyond the defensive realign; all 791 vitest tests still pass. --- ui-tui/package-lock.json | 28 ++------------------ ui-tui/package.json | 3 ++- ui-tui/src/lib/inputMetrics.ts | 48 ++++++++++++++++++++++------------ 3 files changed, 36 insertions(+), 43 deletions(-) diff --git a/ui-tui/package-lock.json b/ui-tui/package-lock.json index bbbf9552399..255c4e1b3cd 100644 --- a/ui-tui/package-lock.json +++ b/ui-tui/package-lock.json @@ -14,7 +14,8 @@ "ink-text-input": "^6.0.0", "nanostores": "^1.2.0", "react": "^19.2.4", - "unicode-animations": "^1.0.3" + "unicode-animations": "^1.0.3", + "wrap-ansi": "^9.0.0" }, "devDependencies": { "@babel/cli": "^7.28.6", @@ -503,31 +504,6 @@ "node": ">=6.9.0" } }, - "node_modules/@emnapi/core": { - "version": "1.10.0", - "resolved": "https://registry.npmjs.org/@emnapi/core/-/core-1.10.0.tgz", - "integrity": "sha512-yq6OkJ4p82CAfPl0u9mQebQHKPJkY7WrIuk205cTYnYe+k2Z8YBh11FrbRG/H6ihirqcacOgl2BIO8oyMQLeXw==", - "dev": true, - "license": "MIT", - "optional": true, - "peer": true, - "dependencies": { - "@emnapi/wasi-threads": "1.2.1", - "tslib": "^2.4.0" - } - }, - "node_modules/@emnapi/runtime": { - "version": "1.10.0", - "resolved": "https://registry.npmjs.org/@emnapi/runtime/-/runtime-1.10.0.tgz", - "integrity": "sha512-ewvYlk86xUoGI0zQRNq/mC+16R1QeDlKQy21Ki3oSYXNgLb45GV1P6A0M+/s6nyCuNDqe5VpaY84BzXGwVbwFA==", - "dev": true, - "license": "MIT", - "optional": true, - "peer": true, - "dependencies": { - "tslib": "^2.4.0" - } - }, "node_modules/@emnapi/wasi-threads": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/@emnapi/wasi-threads/-/wasi-threads-1.2.1.tgz", diff --git a/ui-tui/package.json b/ui-tui/package.json index f28debb313e..1e11f5484da 100644 --- a/ui-tui/package.json +++ b/ui-tui/package.json @@ -22,7 +22,8 @@ "ink-text-input": "^6.0.0", "nanostores": "^1.2.0", "react": "^19.2.4", - "unicode-animations": "^1.0.3" + "unicode-animations": "^1.0.3", + "wrap-ansi": "^9.0.0" }, "devDependencies": { "@babel/cli": "^7.28.6", diff --git a/ui-tui/src/lib/inputMetrics.ts b/ui-tui/src/lib/inputMetrics.ts index 208b3533678..3b66a3dba8e 100644 --- a/ui-tui/src/lib/inputMetrics.ts +++ b/ui-tui/src/lib/inputMetrics.ts @@ -26,14 +26,14 @@ const graphemes = (value: string) => // boundaries — it never drops, reorders, or substitutes existing characters — // so a parallel walk uniquely identifies each line's source range. // -// This used to be a hand-rolled word-wrap (visualLines below) whose break -// points disagreed with wrap-ansi in subtle but visible ways: exact-fill rows -// pushed the cursor to a phantom next line, mid-word breaks landed one -// grapheme off, etc. The composer's TextInput renders text via Ink's -// , which delegates to wrap-ansi — so any drift between the -// two algorithms parks the hardware cursor several cells away from the last -// rendered character. Sourcing both from wrap-ansi guarantees agreement. -function visualLinesFromWrappedOutput(value: string, cols: number): VisualLine[] { +// This used to be a hand-rolled word-wrap whose break points disagreed with +// wrap-ansi in subtle but visible ways: exact-fill rows pushed the cursor to +// a phantom next line, mid-word breaks landed one grapheme off, etc. The +// composer's TextInput renders text via Ink's , which +// delegates to wrap-ansi — so any drift between the two algorithms parks the +// hardware cursor several cells away from the last rendered character. +// Sourcing both from wrap-ansi guarantees agreement. +function visualLines(value: string, cols: number): VisualLine[] { if (!value.length) { return [{ start: 0, end: 0 }] } @@ -65,10 +65,30 @@ function visualLinesFromWrappedOutput(value: string, cols: number): VisualLine[] continue } - // Defensive: if wrap-ansi's emitted character ever desyncs from - // `value[originalIdx]` (would only happen if it substituted, which it - // doesn't for the wrap+hard option set we use), fall back to advancing - // by one to stay in lockstep. The lines/cursor map still terminates. + // Defensive sync check. wrap-ansi (with `hard: true, trim: false`, no + // styled input) is documented to only insert '\n' at break points and + // never substitute, drop, or reorder source characters — so under those + // options `wrapped[i]` should always equal `value[originalIdx]`. But + // future option changes, library upgrades, or callers that start passing + // styled input (ANSI escapes) could violate that invariant silently. If + // they do, we'd slide `originalIdx` past the end of `value` and emit + // garbage line ranges with no diagnostic. Realign by scanning forward + // for the matching character; bail out (return whatever we have) if the + // sync is unrecoverable rather than producing wrong-but-plausible output. + if (originalIdx >= value.length) { + break + } + + if (value[originalIdx] !== ch) { + const reSync = value.indexOf(ch, originalIdx) + + if (reSync === -1) { + break + } + + originalIdx = reSync + } + originalIdx += 1 } @@ -79,10 +99,6 @@ function visualLinesFromWrappedOutput(value: string, cols: number): VisualLine[] return lines.length ? lines : [{ start: 0, end: 0 }] } -function visualLines(value: string, cols: number): VisualLine[] { - return visualLinesFromWrappedOutput(value, cols) -} - function widthBetween(value: string, start: number, end: number) { let width = 0