From d6c76cfbfa13e7fef02ad42a9ae439ddb2c998a7 Mon Sep 17 00:00:00 2001 From: ethernet Date: Wed, 8 Jul 2026 10:20:21 -0400 Subject: [PATCH] test(tui): extract cursor-layout + fast-echo helpers for real unit tests textInputCursorSourceOfTruth.test.ts read textInput.tsx as text and regexed it to check that cursorLayout() is called with curRef.current (not the stale cur React state), and that the fast-echo backspace/append stdout writes are paired with noteCursorAdvance calls. Extract three pure functions from textInput.tsx: - resolveCursorLayout(display, cur, curRefCurrent, columns): wraps cursorLayout(display, curRefCurrent, columns), making the curRef.current-over-cur choice a directly testable pure call instead of a regex match on the render-site call expression. - fastBackspaceEffect(current, cursor) / fastAppendEffect(current, cursor, text): return a single object bundling {newValue, newCursor, write, advanceDelta} for each fast-echo path. Bundling the stdout write and the noteCursorAdvance delta into one return value makes the pairing impossible to silently drift apart (a caller can't get without ), instead of relying on the two call sites appearing near each other in source text. textInput.tsx's render site and backspace/append handlers now call these helpers directly, preserving exact existing behavior (the same '\b \b' write sequence, noteCursorAdvance(-1)/noteCursorAdvance(text.length) calls). textInputCursorSourceOfTruth.test.ts imports and calls the three pure functions directly with a deliberately stale cur vs a fresh curRefCurrent to reconstruct the exact regression scenario, and asserts the bundled effect objects -- no readFileSync, no regex against textInput.tsx's source text. Full ui-tui suite (107 files, 1117 tests) still green. --- .../textInputCursorSourceOfTruth.test.ts | 112 +++++++++++++----- ui-tui/src/components/textInput.tsx | 94 +++++++++++++-- 2 files changed, 167 insertions(+), 39 deletions(-) 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