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.
This commit is contained in:
ethernet
parent
bffc098d09
commit
d6c76cfbfa
2 changed files with 167 additions and 39 deletions
|
|
@ -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')
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue