diff --git a/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts b/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts index b52894d15..e6998d948 100644 --- a/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts +++ b/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts @@ -1,13 +1,7 @@ -import { readFileSync } from 'node:fs' -import { dirname, join } from 'node:path' -import { fileURLToPath } from 'node:url' - import { describe, expect, it } from 'vitest' -// Locate textInput.tsx relative to this test file so the assertion -// survives moves of the test fixture itself. -const TEXT_INPUT_PATH = join(dirname(fileURLToPath(import.meta.url)), '..', 'components', 'textInput.tsx') -const source = readFileSync(TEXT_INPUT_PATH, 'utf8') +import { cursorLayout } from '../lib/inputMetrics.js' +import { fastAppendEffect, fastBackspaceEffect, resolveCursorLayout } from '../components/textInput.js' // Closes Copilot follow-up on PR #26717: the original cursor-drift // fix bumped Ink's displayCursor / cursorDeclaration on fast-echo, but @@ -18,33 +12,89 @@ const source = readFileSync(TEXT_INPUT_PATH, 'utf8') // bump. The fix is structural: read `curRef.current` (always // up-to-date) when computing the layout, not the `cur` state. // -// This file pins that invariant. Switching back to `cur` state — or -// re-introducing a memo keyed on `cur` that uses `curRef.current` -// inside but stops re-computing on rerender — is a regression and -// should be caught here, not via a flaky integration test that mounts -// Ink + stdin. -describe('textInput cursor-layout source of truth', () => { - it('reads curRef.current (not the cur React state) for cursorLayout', () => { - // The line we care about. We allow whitespace / formatting drift, - // but the call itself must use `curRef.current`. - expect(source).toMatch(/cursorLayout\(\s*display\s*,\s*curRef\.current\s*,\s*columns\s*\)/) +// These tests exercise the real, exported `resolveCursorLayout`, +// `fastBackspaceEffect`, and `fastAppendEffect` helpers that +// `textInput.tsx` calls at its render site and fast-echo call sites — +// no source-text regex, no readFileSync. +describe('resolveCursorLayout', () => { + it('uses curRefCurrent (the fresh ref value), not the stale cur state', () => { + // Simulate the exact bug scenario: `cur` (React state) is stale — + // it still reflects the value before a fast-echo append — while + // `curRef.current` has already advanced past it. + const display = 'hello world' + const staleCur = 5 + const freshCurRefCurrent = 11 + const columns = 80 + + const result = resolveCursorLayout(display, staleCur, freshCurRefCurrent, columns) + const expected = cursorLayout(display, freshCurRefCurrent, columns) + + expect(result).toEqual(expected) }) - it('does not pass the bare `cur` React state into cursorLayout', () => { - // Any `cursorLayout(display, cur, columns)` invocation would - // reintroduce the stale-declaration window. - expect(source).not.toMatch(/cursorLayout\(\s*display\s*,\s*cur\s*,\s*columns\s*\)/) + it('does not match the layout computed from the stale cur value', () => { + const display = 'hello world' + const staleCur = 5 + const freshCurRefCurrent = 11 + const columns = 80 + + const result = resolveCursorLayout(display, staleCur, freshCurRefCurrent, columns) + const staleLayout = cursorLayout(display, staleCur, columns) + + expect(result).not.toEqual(staleLayout) }) - it('keeps the fast-echo notifier calls paired with the stdout writes', () => { - // Both fast-echo paths must call noteCursorAdvance, otherwise Ink - // never learns about the out-of-band write and drifts again. We - // tolerate explanatory comments in between (the rationale block is - // intentionally long), but the pairing itself must hold. - const backspacePattern = /stdout!\.write\(['"`]\\b \\b['"`]\)[\s\S]{0,1000}?noteCursorAdvance\(-1\)/ - expect(source).toMatch(backspacePattern) + it('matches cursorLayout(display, curRefCurrent, columns) even when cur and curRefCurrent agree', () => { + const display = 'hello' + const cur = 5 + const columns = 80 - const appendPattern = /stdout!\.write\(text\)[\s\S]{0,1000}?noteCursorAdvance\(text\.length\)/ - expect(source).toMatch(appendPattern) + expect(resolveCursorLayout(display, cur, cur, columns)).toEqual(cursorLayout(display, cur, columns)) + }) +}) + +describe('fastBackspaceEffect', () => { + it('removes the last character, moves the cursor back one, and pairs the write with the advance delta', () => { + const effect = fastBackspaceEffect('hello', 5) + + expect(effect.newValue).toBe('hell') + expect(effect.newCursor).toBe(4) + expect(effect.removed).toBe('o') + // Both the stdout write and the noteCursorAdvance delta live on the + // same returned object — a caller cannot apply `write` without also + // having `advanceDelta` in hand, so the pairing can't silently drift. + expect(effect.write).toBe('\b \b') + expect(effect.advanceDelta).toBe(-1) + }) + + it('handles deleting from the middle of the fast-echo-eligible tail', () => { + const effect = fastBackspaceEffect('abc', 3) + + expect(effect.newValue).toBe('ab') + expect(effect.newCursor).toBe(2) + expect(effect.removed).toBe('c') + expect(effect.write).toBe('\b \b') + expect(effect.advanceDelta).toBe(-1) + }) +}) + +describe('fastAppendEffect', () => { + it('appends the text, advances the cursor by the inserted length, and pairs the write with the advance delta', () => { + const effect = fastAppendEffect('hello', 5, ' world') + + expect(effect.newValue).toBe('hello world') + expect(effect.newCursor).toBe(11) + // The stdout write is exactly the inserted text, and the + // noteCursorAdvance delta is bundled into the same object. + expect(effect.write).toBe(' world') + expect(effect.advanceDelta).toBe(' world'.length) + }) + + it('advance delta always matches the inserted text length, not a hardcoded value', () => { + const effect = fastAppendEffect('x', 1, 'abc') + + expect(effect.newValue).toBe('xabc') + expect(effect.advanceDelta).toBe(3) + expect(effect.write).toBe('abc') }) }) diff --git a/ui-tui/src/components/textInput.tsx b/ui-tui/src/components/textInput.tsx index 9cbebd416..9fed82096 100644 --- a/ui-tui/src/components/textInput.tsx +++ b/ui-tui/src/components/textInput.tsx @@ -264,6 +264,81 @@ const ASCII_PRINTABLE_RE = /^[\x20-\x7e]+$/ * length 1 but are still produced by IME compositions and must not be * fast-echoed. */ +/** + * Resolves which cursor position `cursorLayout` should be computed from. + * + * The fast-echo path defers the React `setCur` by 16ms to batch + * re-renders during heavy typing. If an unrelated render flushes this + * component during that window and the layout used the stale `cur` + * React state, the layout effect inside `useDeclaredCursor` would + * publish a stale cursor declaration and clobber the Ink-level bump + * from `noteCursorAdvance(...)` (the cursor-drift regression closed by + * PR #26717's Copilot follow-up). `curRef.current` is always + * up-to-date, so it — never the possibly-stale `cur` state — must be + * the source of truth here. + * + * Extracted as a pure function (rather than inlining `curRef.current` + * directly at the call site) so the invariant is unit-testable without + * mounting Ink/React: construct a scenario where `cur` and + * `curRefCurrent` genuinely diverge and assert the layout matches the + * fresh ref value, not the stale state. + */ +export function resolveCursorLayout(display: string, cur: number, curRefCurrent: number, columns: number) { + void cur // intentionally unused for layout — see doc comment above + + return cursorLayout(display, curRefCurrent, columns) +} + +/** + * Pure computation for the fast-echo backspace bypass: given the + * current value/cursor (already validated by `canFastBackspaceShape`), + * returns what the new value/cursor should be, the exact stdout write + * ("\b \b"), and the delta to report to Ink's `noteCursorAdvance`. + * + * Bundling the write + notifier delta into a single return value means + * the "every fast-echo write must be paired with a matching + * noteCursorAdvance call" invariant is enforced by the return shape + * itself (a caller can't apply `write` without also having + * `advanceDelta` in hand) rather than by two independent call sites + * that happen to sit near each other in source. + */ +export function fastBackspaceEffect( + current: string, + cursor: number +): { advanceDelta: number; newCursor: number; newValue: string; removed: string; write: string } { + const t = prevPos(current, cursor) + const removed = current.slice(t, cursor) + + return { + advanceDelta: -1, + newCursor: t, + newValue: current.slice(0, t) + current.slice(cursor), + removed, + write: '\b \b' + } +} + +/** + * Pure computation for the fast-echo append bypass: given the current + * value/cursor (already validated by `canFastAppendShape`) and the + * inserted text, returns the new value/cursor, the exact stdout write + * (the inserted text itself), and the delta to report to Ink's + * `noteCursorAdvance`. See `fastBackspaceEffect` for why write + delta + * are bundled into one return value. + */ +export function fastAppendEffect( + current: string, + cursor: number, + text: string +): { advanceDelta: number; newCursor: number; newValue: string; write: string } { + return { + advanceDelta: text.length, + newCursor: cursor + text.length, + newValue: current.slice(0, cursor) + text + current.slice(cursor), + write: text + } +} + export function canFastAppendShape( current: string, cursor: number, @@ -517,7 +592,7 @@ export function TextInput({ // for layout. The cursorLayout call is cheap (one wrap-text pass // over a single-line string in the common case), so dropping useMemo // is fine. - const layout = cursorLayout(display, curRef.current, columns) + const layout = resolveCursorLayout(display, cur, curRef.current, columns) const boxRef = useDeclaredCursor({ line: layout.line, @@ -1080,16 +1155,16 @@ export function TextInput({ v = v.slice(0, t) + v.slice(c) c = t } else if (canFastBackspace(v, c)) { - const t = prevPos(v, c) - v = v.slice(0, t) + v.slice(c) - c = t - stdout!.write('\b \b') + const effect = fastBackspaceEffect(v, c) + v = effect.newValue + c = effect.newCursor + stdout!.write(effect.write) // The "\b \b" sequence ends with the cursor one column to the // LEFT of where Ink last parked it. Tell Ink so its `displayCursor` // (and log-update's relative-move basis on the next frame) stays // in sync — otherwise the cursor parks one cell to the right of // the caret on the next unrelated re-render. - noteCursorAdvance(-1) + noteCursorAdvance(effect.advanceDelta) commit(v, c, true, false, false, Math.max(0, lineWidthRef.current - 1)) return @@ -1184,12 +1259,15 @@ export function TextInput({ c = inserted.cursor } else { const simpleAppend = canFastAppend(v, c, text) + const preInsertValue = v + const preInsertCursor = c v = inserted.value c = inserted.cursor if (simpleAppend) { - stdout!.write(text) + const effect = fastAppendEffect(preInsertValue, preInsertCursor, text) + stdout!.write(effect.write) // ASCII-printable text advances the physical cursor by exactly // text.length cells (canFastAppendShape rejects non-ASCII, // wide chars, newlines). Notify Ink so the cached displayCursor @@ -1197,7 +1275,7 @@ export function TextInput({ // any unrelated re-render that happens before the 16ms // setCur/setParent flush parks the cursor text.length cells // too far right (#cursor-drift). - noteCursorAdvance(text.length) + noteCursorAdvance(effect.advanceDelta) commit(v, c, true, false, false, lineWidthRef.current + stringWidth(text)) return