diff --git a/ui-tui/packages/hermes-ink/index.d.ts b/ui-tui/packages/hermes-ink/index.d.ts index 5d5ae9387c0..66fed32ae60 100644 --- a/ui-tui/packages/hermes-ink/index.d.ts +++ b/ui-tui/packages/hermes-ink/index.d.ts @@ -34,5 +34,6 @@ export { default as measureElement } from './src/ink/measure-element.ts' export { createRoot, forceRedraw, default as render, renderSync } from './src/ink/root.ts' export type { Instance, RenderOptions, Root } from './src/ink/root.ts' export { stringWidth } from './src/ink/stringWidth.ts' +export { wrapAnsi } from './src/ink/wrapAnsi.ts' export { default as TextInput, UncontrolledTextInput } from 'ink-text-input' export type { Props as TextInputProps } from 'ink-text-input' diff --git a/ui-tui/packages/hermes-ink/src/entry-exports.ts b/ui-tui/packages/hermes-ink/src/entry-exports.ts index d173e0c9bb1..a113660385f 100644 --- a/ui-tui/packages/hermes-ink/src/entry-exports.ts +++ b/ui-tui/packages/hermes-ink/src/entry-exports.ts @@ -26,5 +26,6 @@ export { default as measureElement } from './ink/measure-element.js' export { scrollFastPathStats, type ScrollFastPathStats } from './ink/render-node-to-output.js' export { createRoot, forceRedraw, default as render, renderSync } from './ink/root.js' export { stringWidth } from './ink/stringWidth.js' +export { wrapAnsi } from './ink/wrapAnsi.js' export { isXtermJs } from './ink/terminal.js' export { default as TextInput, UncontrolledTextInput } from 'ink-text-input' diff --git a/ui-tui/src/__tests__/cursorDriftRegression.test.ts b/ui-tui/src/__tests__/cursorDriftRegression.test.ts new file mode 100644 index 00000000000..3f9082dcefc --- /dev/null +++ b/ui-tui/src/__tests__/cursorDriftRegression.test.ts @@ -0,0 +1,114 @@ +/** + * Pinned regression for the multi-line composer cursor-drift bug. + * + * Symptom: in `hermes --tui`, typing into the composer until the input + * wraps across multiple visual rows would leave several blank cells + * between the last typed character and the (hardware) cursor block. + * Worse on narrow terminals (the Cursor IDE built-in terminal in + * particular). + * + * Root cause: the composer's `cursorLayout` (used by `useDeclaredCursor` + * to place the hardware cursor) ran a hand-rolled word-wrap algorithm, + * while Ink's `` renders via `wrap-ansi`. The two + * disagreed on many real inputs — wrap-ansi would keep "branch + * investigate" on one row while cursorLayout claimed it had wrapped, + * etc. — so the declared cursor position drifted from where the text + * was actually rendered. The fix sources cursorLayout's line breaks + * directly from wrap-ansi, guaranteeing agreement. + * + * This test pins the contract: for every char that would be typed into + * the composer, the cursor position reported by cursorLayout MUST equal + * the end-of-text position that wrap-ansi would render. Any future + * regression that lets the two diverge re-introduces the drift. + */ +import { wrapAnsi } from '@hermes/ink' +import { describe, expect, it } from 'vitest' + +import { cursorLayout, inputVisualHeight } from '../lib/inputMetrics.js' + +function wrapAnsiEnd(text: string, cols: number): { line: number; column: number } { + const wrapped = wrapAnsi(text, cols, { hard: true, trim: false }) + const lines = wrapped.split('\n') + const last = lines[lines.length - 1] ?? '' + + return { line: lines.length - 1, column: last.length } +} + +const USER_REPORT_MESSAGE = + // Paraphrase of the user's actual bug report, included verbatim so the + // test is grounded in a realistic typing pattern (long single line, + // mixed-length words, punctuation, no hard newlines). + 'im in cursor terminal using hermes --tui and as i type multiline my caret at the end will often ' + + 'go.. randomly.. like multiple spaces away lol and idk why. theres no rhyme/reason really but ' + + 'there should literally never be a non-user added space at the end of my composer input right? ' + + 'i dont think it happens on new sessions but only existing ones. there have been a few prs to ' + + 'try to fix this and all not working. ok it just happened, to me, nowso attaching screenshot ' + + 'and you can see its multiline, new session. on a new bb/ branch investigate' + +describe('cursor-drift regression — composer cursorLayout matches Ink rendering', () => { + it('agrees with wrap-ansi at every typing-prefix of the user-reported message', () => { + // Walks the message char-by-char (mirroring what the TUI sees when a + // user types). At every prefix, cursorLayout must place the cursor + // exactly where wrap-ansi would render the end of the text. + // + // Pre-fix: this failed on most narrow widths because the hand-rolled + // wrap algorithm broke at slightly different points than wrap-ansi. + for (const cols of [40, 50, 55, 60, 65, 70, 80]) { + let acc = '' + + for (const ch of USER_REPORT_MESSAGE) { + acc += ch + const layout = cursorLayout(acc, acc.length, cols) + const expected = wrapAnsiEnd(acc, cols) + + expect( + layout, + `mismatch at cols=${cols}, len=${acc.length}, last-char=${JSON.stringify(ch)}, ` + + `tail=${JSON.stringify(acc.slice(-30))}` + ).toEqual(expected) + } + } + }) + + it('keeps cursor on the same row when text exactly fills the terminal width', () => { + // wrap-ansi does NOT push exact-fill text onto a phantom next line. + // The previous algorithm did — that's what produced the visible + // "cursor parked one row below the last char" symptom on narrow + // terminals at certain message lengths. + for (const cols of [8, 12, 18, 24]) { + const text = 'a'.repeat(cols) + const layout = cursorLayout(text, text.length, cols) + const inkLines = wrapAnsi(text, cols, { hard: true, trim: false }).split('\n') + + expect(layout.line).toBe(0) + expect(layout.column).toBe(cols) + expect(inkLines).toHaveLength(1) + expect(inputVisualHeight(text, cols)).toBe(1) + } + }) + + it('does not stuff a trailing whitespace word onto a phantom line', () => { + // "branch investigate" at cols=20 fits on one row in wrap-ansi. The + // bug claimed otherwise, parking the cursor at (line=1, col=?) and + // leaving the user's "branch investigate" rendered alone on row 0 + // with the cursor block several cells past it. + const text = 'branch investigate' + const cols = 20 + + expect(cursorLayout(text, text.length, cols)).toEqual({ column: text.length, line: 0 }) + expect(cursorLayout(text, text.length, cols)).toEqual(wrapAnsiEnd(text, cols)) + }) + + it('agrees with wrap-ansi for word-wrap that pushes a word onto the next line', () => { + // "hello world" at cols=8 wraps to ["hello ", "world"] in wrap-ansi. + // The cursor at end-of-text must land at line=1, col=5 — where Ink + // actually renders the last 'd'. The previous algorithm reported + // (line=2, col=0) here (phantom extra wrap), which parked the + // cursor on a row Ink never painted. + const text = 'hello world' + const cols = 8 + + expect(cursorLayout(text, text.length, cols)).toEqual({ column: 5, line: 1 }) + expect(cursorLayout(text, text.length, cols)).toEqual(wrapAnsiEnd(text, cols)) + }) +}) diff --git a/ui-tui/src/__tests__/textInputWrap.test.ts b/ui-tui/src/__tests__/textInputWrap.test.ts index c25c9629e77..22b33c9480e 100644 --- a/ui-tui/src/__tests__/textInputWrap.test.ts +++ b/ui-tui/src/__tests__/textInputWrap.test.ts @@ -1,8 +1,20 @@ +import { wrapAnsi } from '@hermes/ink' import { describe, expect, it } from 'vitest' import { offsetFromPosition } from '../components/textInput.js' import { composerPromptWidth, cursorLayout, inputVisualHeight, stableComposerColumns } from '../lib/inputMetrics.js' +// Helper: compute the "end of text" position that wrap-ansi would render +// the input to. This is what Ink's uses, so cursorLayout +// MUST agree. Disagreement is the cursor-drift bug. +function wrapAnsiEndPosition(text: string, cols: number): { line: number; column: number } { + const wrapped = wrapAnsi(text, cols, { hard: true, trim: false }) + const lines = wrapped.split('\n') + const last = lines[lines.length - 1] ?? '' + + return { line: lines.length - 1, column: last.length } +} + describe('cursorLayout — word-wrap parity with wrap-ansi', () => { it('places cursor mid-line at its column', () => { expect(cursorLayout('hello world', 6, 40)).toEqual({ column: 6, line: 0 }) @@ -12,19 +24,36 @@ describe('cursorLayout — word-wrap parity with wrap-ansi', () => { expect(cursorLayout('hi', 2, 10)).toEqual({ column: 2, line: 0 }) }) - it('wraps to next line when cursor lands exactly at the right edge', () => { - // 8 chars on an 8-col line: text fills the row exactly; the cursor's - // inverted-space cell overflows to col 0 of the next row. - expect(cursorLayout('abcdefgh', 8, 8)).toEqual({ column: 0, line: 1 }) + it('does not push exact-fill text onto a phantom next line', () => { + // Regression: the previous hand-rolled wrap algorithm forced the cursor + // onto (line+1, 0) when the text exactly filled the row. wrap-ansi keeps + // it on the same row (no soft-wrap), so the cursor must too — otherwise + // useDeclaredCursor parks the hardware cursor below the last char and + // the user sees several blank cells between text and cursor block + // (#cursor-drift-multiline). + expect(cursorLayout('abcdefgh', 8, 8)).toEqual({ column: 8, line: 0 }) + expect(cursorLayout('abcdefgh', 8, 8)).toEqual(wrapAnsiEndPosition('abcdefgh', 8)) + }) + + it('keeps short words on the current line when they fit (no phantom wrap)', () => { + // wrap-ansi: "hello wo" at cols=8 stays as one line "hello wo". + // The old cursorLayout incorrectly pushed to (1,0) because column=8 hit + // the column>=width check, but that disagreed with what Ink actually + // rendered. + expect(cursorLayout('hello wo', 8, 8)).toEqual({ column: 8, line: 0 }) + expect(cursorLayout('hello wo', 8, 8)).toEqual(wrapAnsiEndPosition('hello wo', 8)) }) it('moves words across wrap boundaries instead of splitting them', () => { - // With wordWrap:true, "hello wor" at cols=8 is "hello \nwor" rather - // than "hello wo\nr". - expect(cursorLayout('hello wo', 8, 8)).toEqual({ column: 0, line: 1 }) + // "hello wor" at cols=8: wrap-ansi breaks at the space, "hello \nwor". expect(cursorLayout('hello wor', 9, 8)).toEqual({ column: 3, line: 1 }) expect(cursorLayout('hello worl', 10, 8)).toEqual({ column: 4, line: 1 }) expect(cursorLayout('hello world', 11, 8)).toEqual({ column: 5, line: 1 }) + + // Each must match what wrap-ansi would actually render. + expect(cursorLayout('hello wor', 9, 8)).toEqual(wrapAnsiEndPosition('hello wor', 8)) + expect(cursorLayout('hello worl', 10, 8)).toEqual(wrapAnsiEndPosition('hello worl', 8)) + expect(cursorLayout('hello world', 11, 8)).toEqual(wrapAnsiEndPosition('hello world', 8)) }) it('wraps the next word instead of splitting it at the right edge', () => { @@ -42,12 +71,33 @@ describe('cursorLayout — word-wrap parity with wrap-ansi', () => { it('does not wrap when cursor is before the right edge', () => { expect(cursorLayout('abcdefg', 7, 8)).toEqual({ column: 7, line: 0 }) }) + + it('matches wrap-ansi end-position for typing-style incremental input', () => { + // Pins the actual fix: type a long message char-by-char at a narrow + // width and assert the cursor follows wrap-ansi every step of the way. + // Before the fix, ~5 boundary positions per pass disagreed and Ink + // parked the cursor several cells past the last rendered character. + const MSG = 'on a new bb branch investigate and fix the cursor drift bug here' + + for (const cols of [10, 14, 20, 30, 50, 80]) { + let acc = '' + + for (const ch of MSG) { + acc += ch + expect(cursorLayout(acc, acc.length, cols)).toEqual(wrapAnsiEndPosition(acc, cols)) + } + } + }) }) describe('input metrics helpers', () => { - it('computes visual height from the wrapped cursor line', () => { - expect(inputVisualHeight('abcdefgh', 8)).toBe(2) + it('computes visual height matching wrap-ansi line count', () => { + // Exact-fill text stays on one line in wrap-ansi (no phantom wrap), so + // visual height is 1. The previous implementation reported 2 here. + expect(inputVisualHeight('abcdefgh', 8)).toBe(1) expect(inputVisualHeight('one\ntwo', 40)).toBe(2) + // Multi-line wrap case sanity + expect(inputVisualHeight('hello world', 8)).toBe(2) }) it('counts the prompt gap as its own cell', () => { diff --git a/ui-tui/src/components/textInput.tsx b/ui-tui/src/components/textInput.tsx index ace2f479dc1..92082280a04 100644 --- a/ui-tui/src/components/textInput.tsx +++ b/ui-tui/src/components/textInput.tsx @@ -272,10 +272,22 @@ export function canFastBackspaceShape(current: string, cursor: number, columns?: } // If we know the wrap width, reject at the soft-wrap boundary: the - // caret's visual column is 0, so "\b \b" can't represent the physical - // move back to the previous visual line. - if (columns !== undefined && cursorLayout(current, cursor, columns).column === 0) { - return false + // caret's physical column would be at (or past) the terminal's right + // edge, so the terminal has already auto-wrapped to the next row. + // "\b \b" can't represent the physical move back across that wrap. + // + // We check `column === 0` for the "wrap-ansi broke onto a new line" + // case AND `column >= columns` for the "exact-fill, terminal auto-wraps" + // case. Both manifest as the same physical state (cursor parked at + // col 0 of the next row) but cursorLayout reports them differently + // because it now mirrors wrap-ansi's break points exactly (see the + // cursor-drift-multiline fix in lib/inputMetrics.ts). + if (columns !== undefined) { + const layout = cursorLayout(current, cursor, columns) + + if (layout.column === 0 || layout.column >= columns) { + return false + } } const removed = current.slice(prevPos(current, cursor), cursor) diff --git a/ui-tui/src/lib/inputMetrics.ts b/ui-tui/src/lib/inputMetrics.ts index b5645b43310..4c624da167a 100644 --- a/ui-tui/src/lib/inputMetrics.ts +++ b/ui-tui/src/lib/inputMetrics.ts @@ -1,4 +1,4 @@ -import { stringWidth } from '@hermes/ink' +import { stringWidth, wrapAnsi } from '@hermes/ink' import type { Role } from '../types.js' @@ -12,8 +12,6 @@ interface VisualLine { start: number } -const isWhitespace = (value: string) => /\s/.test(value) - const graphemes = (value: string) => [...seg().segment(value)].map(({ segment, index }) => ({ end: index + segment.length, @@ -22,76 +20,81 @@ const graphemes = (value: string) => width: Math.max(1, stringWidth(segment)) })) +// Build VisualLines from wrap-ansi's output by mapping each emitted character +// back to its original offset in `value`. wrap-ansi only INSERTS '\n' at wrap +// 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 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 }] + } + const width = Math.max(1, cols) + const wrapped = wrapAnsi(value, width, { hard: true, trim: false }) const lines: VisualLine[] = [] - let sourceLineStart = 0 - for (const sourceLine of value.split('\n')) { - const parts = graphemes(sourceLine) + let originalIdx = 0 + let lineStart = 0 - if (!parts.length) { - lines.push({ start: sourceLineStart, end: sourceLineStart }) - sourceLineStart += 1 + for (let i = 0; i < wrapped.length; i += 1) { + const ch = wrapped[i]! + + if (ch === '\n') { + // wrap-ansi inserts '\n' to mark a soft-wrap boundary OR copies a + // literal '\n' from the input. Either way the next char in `wrapped` + // begins a new visual line. If the source character is a hard '\n', + // consume it (it doesn't appear in either line). Otherwise the '\n' + // is purely a wrap marker and originalIdx stays put. + lines.push({ start: lineStart, end: originalIdx }) + const isHardNewline = originalIdx < value.length && value[originalIdx] === '\n' + + if (isHardNewline) { + originalIdx += 1 + } + + lineStart = originalIdx continue } - let lineStartPart = 0 - let lineStartOffset = sourceLineStart - let column = 0 - let breakPart: null | number = null - let i = 0 - - while (i < parts.length) { - const part = parts[i]! - const partStart = sourceLineStart + part.index - - if (column + part.width > width && i > lineStartPart) { - if (breakPart !== null && breakPart > lineStartPart) { - const breakOffset = sourceLineStart + parts[breakPart - 1]!.end - lines.push({ start: lineStartOffset, end: breakOffset }) - lineStartPart = breakPart - lineStartOffset = breakOffset - } else { - lines.push({ start: lineStartOffset, end: partStart }) - lineStartPart = i - lineStartOffset = partStart - } - - column = 0 - breakPart = null - i = lineStartPart - continue - } - - column += part.width - - if (isWhitespace(part.segment)) { - breakPart = i + 1 - } - - i += 1 - - if (column >= width && i < parts.length) { - const next = parts[i]! - const nextStartsWord = !isWhitespace(next.segment) - - if (breakPart !== null && breakPart > lineStartPart && nextStartsWord) { - const breakOffset = sourceLineStart + parts[breakPart - 1]!.end - lines.push({ start: lineStartOffset, end: breakOffset }) - lineStartPart = breakPart - lineStartOffset = breakOffset - column = 0 - breakPart = null - i = lineStartPart - } - } + // 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 } - lines.push({ start: lineStartOffset, end: sourceLineStart + sourceLine.length }) - sourceLineStart += sourceLine.length + 1 + if (value[originalIdx] !== ch) { + const reSync = value.indexOf(ch, originalIdx) + + if (reSync === -1) { + break + } + + originalIdx = reSync + } + + originalIdx += 1 } + lines.push({ start: lineStart, end: originalIdx }) + + // wrap-ansi collapses an empty input into [""] which we already handled + // above; preserve the invariant that lines is never empty for any input. return lines.length ? lines : [{ start: 0, end: 0 }] } @@ -108,6 +111,12 @@ function widthBetween(value: string, start: number, end: number) { /** * Mirrors the word-wrap behavior used by the composer TextInput. * Returns the zero-based visual line and column of the cursor cell. + * + * IMPORTANT: this MUST stay in lock-step with how Ink's `` + * lays the value out (which uses `wrap-ansi`). Any divergence parks the + * hardware cursor several cells off the last rendered character — see the + * "cursor drift past blank cells" bug. `visualLines` is sourced directly + * from wrap-ansi to enforce that invariant. */ export function cursorLayout(value: string, cursor: number, cols: number) { const pos = Math.max(0, Math.min(cursor, value.length)) @@ -124,14 +133,14 @@ export function cursorLayout(value: string, cursor: number, cols: number) { } const line = lines[lineIndex]! - let column = widthBetween(value, line.start, Math.min(pos, line.end)) - - // trailing cursor-cell overflows to the next row at the wrap column - if (column >= w) { - lineIndex++ - column = 0 - } + const column = widthBetween(value, line.start, Math.min(pos, line.end)) + // NOTE: the previous implementation forced an extra line break when + // `column >= w` (the "trailing cursor-cell overflows" rule). With + // `visualLines` sourcing breaks from wrap-ansi, the line wrapping + // above already matches what Ink will actually render. Pushing the + // cursor onto a phantom next line here would re-introduce the same + // drift we're fixing, so we don't. return { column, line: lineIndex } }