fix(tui): align composer cursorLayout with wrap-ansi to kill multiline cursor drift#27489
Conversation
…e cursor drift The composer's `cursorLayout` (in `ui-tui/src/lib/inputMetrics.ts`) used a hand-rolled word-wrap algorithm to decide where `useDeclaredCursor` should park the hardware cursor. But Ink's `<Text wrap="wrap">` renders the same text via `wrap-ansi`. The two algorithms disagreed on common real-world inputs — `"branch investigate"` at cols=20, `"hello world"` at cols=8, exact-fill strings like `"abcdefgh"` at cols=8 — so the hardware cursor parked several cells past where Ink actually rendered the last character. Users saw a multi-cell blank gap between their last-typed letter and the cursor block, especially on narrow terminals (the Cursor IDE built-in terminal was the worst offender). Three previous PRs (#26717, #25860, #22197) chased fast-echo displayCursor/cursorDeclaration drift and in-band-vs-native cursor heuristics. None of them touched the underlying wrap-algorithm mismatch, which is why the bug kept resurfacing. Fix: source cursorLayout's line breaks from wrap-ansi directly. Walk its emitted string char-by-char, tracking original-string offsets, push a VisualLine at each '\n'. Also drop the buggy `column >= w` overflow rule in cursorLayout — that's what pushed exact-fill text onto a phantom next row. canFastBackspaceShape now detects the wrap boundary in BOTH coordinate conventions (column === 0 OR column >= columns), since exact-fill now reports as (0, columns) instead of the previous (1, 0). The physical state is identical — the terminal auto-wraps at column N either way — but the layout function reports the position more honestly. Tests: - ui-tui/src/__tests__/textInputWrap.test.ts: 3 tests that pinned the BUGGY behavior were updated to assert wrap-ansi parity (the real invariant). Added a typing-prefix invariant: cursorLayout must agree with wrap-ansi at every character of a long input. - ui-tui/src/__tests__/cursorDriftRegression.test.ts: new file. Walks the user-reported bug message char-by-char at 7 widths and asserts agreement with wrap-ansi at every prefix. Verification: - 791/791 vitest tests pass. - 84/84 tui-gateway pytest tests pass via scripts/run_tests.sh. - PTY repro (typing into a real `hermes --tui` PTY at cols=50/55/60): cursor lands exactly 1 cell past the last typed char in every case the bug previously drifted.
🔎 Lint report:
|
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing TUI composer cursor-drift bug by replacing the hand-rolled word-wrap algorithm used by cursorLayout with one that mirrors Ink's <Text wrap="wrap"> (which delegates to wrap-ansi). Since the cursor declaration and the rendered text now derive line breaks from the same algorithm, the hardware cursor stays aligned with the last rendered character. The column >= w "overflow to next line" rule is dropped, and canFastBackspaceShape is updated to recognize both representations of a wrap boundary.
Changes:
- Rewrite
visualLines/cursorLayoutinui-tui/src/lib/inputMetrics.tsto source visual line boundaries fromwrap-ansiand remove the phantom-row overflow rule. - Update
canFastBackspaceShapeinui-tui/src/components/textInput.tsxto also reject fast-backspace at exact-fill (column >= columns) in addition tocolumn === 0. - Update
textInputWrap.test.tsto assert wrap-ansi parity and add a typing-prefix test; add a newcursorDriftRegression.test.tscovering exact-fill, word-wrap, and the user-reported message at multiple widths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ui-tui/src/lib/inputMetrics.ts | Replaces hand-rolled visualLines with a wrap-ansi-driven walker; drops the column>=w cursor overflow rule. |
| ui-tui/src/components/textInput.tsx | canFastBackspaceShape rejects both column === 0 and column >= columns wrap boundaries. |
| ui-tui/src/tests/textInputWrap.test.ts | Updates 3 assertions to pin wrap-ansi parity; adds typing-prefix parity test. |
| ui-tui/src/tests/cursorDriftRegression.test.ts | New regression tests pinning agreement with wrap-ansi across widths and edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
…n runtime parity) Copilot caught an important runtime parity gap on PR #27489: the fix imported the npm `wrap-ansi` package directly, but Ink's `<Text wrap="wrap">` uses a runtime-selecting shim (`ui-tui/packages/hermes-ink/src/ink/wrapAnsi.ts`) that prefers `Bun.wrapAnsi` when running under Bun and falls back to the npm package elsewhere. So under Bun, Ink would render via `Bun.wrapAnsi` while `cursorLayout` would compute breaks via the npm package — any disagreement reintroduces the exact cursor-drift symptom the PR is meant to eliminate. Fix: - Export `wrapAnsi` from `@hermes/ink` (`packages/hermes-ink/src/entry-exports.ts` and `packages/hermes-ink/index.d.ts`) so the shim is the public surface. - Switch `ui-tui/src/lib/inputMetrics.ts` from `import wrapAnsi from 'wrap-ansi'` to `import { wrapAnsi } from '@hermes/ink'`. Both renderer (Ink) and cursor layout now traverse the same shim, so they share the runtime-selected implementation by construction. - Same swap in `textInputWrap.test.ts` and `cursorDriftRegression.test.ts` — tests now assert parity through the shim, which means under Bun they actually exercise Bun's implementation instead of asserting a tautology against the npm package. - Drop the direct `"wrap-ansi": "^9.0.0"` from `ui-tui/package.json`. `@hermes/ink` (which IS a declared dep) pulls wrap-ansi in transitively — that's not a phantom dep because the import path goes through `@hermes/ink`'s public exports, not through a hoisting accident. Verified: 791/791 vitest tests pass. `@hermes/ink` rebuilt (`dist/entry-exports.js` includes `wrapAnsi` export). TUI bundle rebuilt clean.
…n runtime parity) Copilot caught an important runtime parity gap on PR NousResearch#27489: the fix imported the npm `wrap-ansi` package directly, but Ink's `<Text wrap="wrap">` uses a runtime-selecting shim (`ui-tui/packages/hermes-ink/src/ink/wrapAnsi.ts`) that prefers `Bun.wrapAnsi` when running under Bun and falls back to the npm package elsewhere. So under Bun, Ink would render via `Bun.wrapAnsi` while `cursorLayout` would compute breaks via the npm package — any disagreement reintroduces the exact cursor-drift symptom the PR is meant to eliminate. Fix: - Export `wrapAnsi` from `@hermes/ink` (`packages/hermes-ink/src/entry-exports.ts` and `packages/hermes-ink/index.d.ts`) so the shim is the public surface. - Switch `ui-tui/src/lib/inputMetrics.ts` from `import wrapAnsi from 'wrap-ansi'` to `import { wrapAnsi } from '@hermes/ink'`. Both renderer (Ink) and cursor layout now traverse the same shim, so they share the runtime-selected implementation by construction. - Same swap in `textInputWrap.test.ts` and `cursorDriftRegression.test.ts` — tests now assert parity through the shim, which means under Bun they actually exercise Bun's implementation instead of asserting a tautology against the npm package. - Drop the direct `"wrap-ansi": "^9.0.0"` from `ui-tui/package.json`. `@hermes/ink` (which IS a declared dep) pulls wrap-ansi in transitively — that's not a phantom dep because the import path goes through `@hermes/ink`'s public exports, not through a hoisting accident. Verified: 791/791 vitest tests pass. `@hermes/ink` rebuilt (`dist/entry-exports.js` includes `wrapAnsi` export). TUI bundle rebuilt clean. (cherry picked from commit 8c78f53)
Three small follow-ups from the Copilot review on NousResearch#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.
…n runtime parity) Copilot caught an important runtime parity gap on PR NousResearch#27489: the fix imported the npm `wrap-ansi` package directly, but Ink's `<Text wrap="wrap">` uses a runtime-selecting shim (`ui-tui/packages/hermes-ink/src/ink/wrapAnsi.ts`) that prefers `Bun.wrapAnsi` when running under Bun and falls back to the npm package elsewhere. So under Bun, Ink would render via `Bun.wrapAnsi` while `cursorLayout` would compute breaks via the npm package — any disagreement reintroduces the exact cursor-drift symptom the PR is meant to eliminate. Fix: - Export `wrapAnsi` from `@hermes/ink` (`packages/hermes-ink/src/entry-exports.ts` and `packages/hermes-ink/index.d.ts`) so the shim is the public surface. - Switch `ui-tui/src/lib/inputMetrics.ts` from `import wrapAnsi from 'wrap-ansi'` to `import { wrapAnsi } from '@hermes/ink'`. Both renderer (Ink) and cursor layout now traverse the same shim, so they share the runtime-selected implementation by construction. - Same swap in `textInputWrap.test.ts` and `cursorDriftRegression.test.ts` — tests now assert parity through the shim, which means under Bun they actually exercise Bun's implementation instead of asserting a tautology against the npm package. - Drop the direct `"wrap-ansi": "^9.0.0"` from `ui-tui/package.json`. `@hermes/ink` (which IS a declared dep) pulls wrap-ansi in transitively — that's not a phantom dep because the import path goes through `@hermes/ink`'s public exports, not through a hoisting accident. Verified: 791/791 vitest tests pass. `@hermes/ink` rebuilt (`dist/entry-exports.js` includes `wrapAnsi` export). TUI bundle rebuilt clean.
…er-cursor-drift-v2 fix(tui): align composer cursorLayout with wrap-ansi to kill multiline cursor drift
What
Fixes the long-running "multi-cell blank gap between last typed character and cursor block" bug in
hermes --tuicomposer. Reproduces reliably on narrow terminals (~50-60 cols), especially the Cursor IDE built-in terminal, but happens at any width once the composer wraps.Closes the bug class that PRs #26717, #25860, #22197 all chased without resolving.
Root cause
The composer's
cursorLayout(ui-tui/src/lib/inputMetrics.ts) used a hand-rolled word-wrap algorithm (visualLines) to decide whereuseDeclaredCursorshould park the hardware cursor. But Ink's<Text wrap="wrap">renders the same text viawrap-ansi. The two algorithms disagreed on many real inputs.Concrete examples I verified by feeding both functions the same strings:
"branch investigate"["branch investigate"]end=(0,18)"hello world"["hello ", "world"]end=(1,5)"abcdefgh"["abcdefgh"]end=(0,8)"hello investigatex"useDeclaredCursorpublishedcursorLayout's phantom position. Ink rendered the text at wrap-ansi's actual position. Ink then parked the hardware cursor at the published(rect.x + relativeX, rect.y + relativeY). The visible gap between the last rendered char and the cursor block is exactly that mismatch.Why previous PRs missed it: they focused on fast-echo display-cursor synchronization (#26717), in-band vs native cursor rendering (#25860), and arrow-key navigation (#22197). All correct fixes for their respective bugs, none of them touched the wrap-algorithm mismatch underneath.
Fix
cursorLayoutis now sourced from wrap-ansivisualLinesFromWrappedOutputcallswrapAnsi(value, cols, { hard: true, trim: false })(the exact same call Ink makes) and walks the emitted string char-by-char to map back to original-string offsets. Every soft-wrap\nwrap-ansi inserts becomes aVisualLineboundary; hard\ns in the input are consumed normally.This guarantees the cursor position and the rendered text agree by construction — same input, same algorithm, same output.
Drop the
column >= woverflow ruleThe old code had:
…which pushed exact-fill text onto a phantom next row. wrap-ansi does NOT do this (an 8-char string in an 8-col viewport stays on one line). Removing it eliminates the phantom-row drift. The hardware cursor naturally auto-wraps if the terminal's CUP target is past the row's right edge — there's no need to lie about the logical position.
canFastBackspaceShapechecks both representationsAfter the rewrite, exact-fill reports as
(line: 0, column: cols)instead of the previous(line: 1, column: 0). The physical state is identical (terminal auto-wraps either way), but the boolean for "are we at a wrap boundary" needs to check both:Tests
Updated
ui-tui/src/__tests__/textInputWrap.test.tsThree tests were pinning the buggy behavior (asserting cursorLayout returned the phantom-row position). They're updated to assert wrap-ansi parity — the actual invariant. Plus a new typing-prefix test that walks a long string char-by-char and asserts agreement with wrap-ansi at every prefix.
New
ui-tui/src/__tests__/cursorDriftRegression.test.ts4 tests pinning the fix directly:
"branch investigate"case from the user's screenshot."hello world"at cols=8).Verification
scripts/run_tests.sh tests/tui_gateway/.npm run type-checkintroduces zero new errors.npm run buildclean.npm run lintintroduces zero new errors.Live PTY reproduction
I wrote a
pyte+ptyprocessharness that spawns realhermes --tuiin a PTY, types the user's bug-report message char-by-char at human-realistic speed (40ms/char), and captures the resulting screen + cursor position.At cols=55 typing the message ending in "right":
tin "right", row 29.Same result at cols=50 and cols=60.
Risk
Low. The hot path is a pure-function rewrite that delegates to a library Ink already depends on. The behavior change is in the direction of "match what's actually on the screen" — no caller can rely on the phantom-row reports (they were always visually wrong). The test suite update converts 3 wrong-behavior assertions into right-behavior assertions; everything else passes unchanged.
Branch:
bb/tui-composer-cursor-drift-v2