fix(tui): keep Ink displayCursor in sync with fast-echo writes so cursor stops drifting (#26717)

* fix(tui): keep Ink displayCursor in sync with fast-echo writes so cursor stops drifting

TextInput's fast-echo bypass writes characters directly to stdout to
avoid waiting on a React re-render for each keystroke. The hardware
cursor advances by text.length cells, but Ink's cached `displayCursor`
(the basis for the next frame's relative cursor-move preamble in
log-update) stayed unchanged. When ANY unrelated component re-rendered
between the fast-echo write and the deferred composer setCur/setParent
flush — status bar timer, streaming reasoning, etc. — the next frame's
preamble emitted a relative cursor move from a stale parked position
and the hardware cursor parked N cells offset from the actual caret.

Visible symptom: extra whitespace between the just-typed character and
the cursor block, intermittent, worse on long sessions during streaming.
Alt-screen was immune because frames begin with absolute CSI H.

This adds a small API in @hermes/ink:

  - `Ink.noteExternalCursorAdvance(dx, dy?)` — bumps displayCursor if
    set, otherwise seeds from frontFrame.cursor so the next preamble's
    relative move correctly cancels the external advance. No-op on
    alt-screen.
  - `CursorAdvanceContext` + `useCursorAdvance()` hook to expose it.

TextInput then calls `noteCursorAdvance(text.length)` after the
fast-echo `stdout.write(text)` append, and `noteCursorAdvance(-1)`
after the fast-backspace `\b \b` sequence.

Tests: 4 new vitest cases pin the API contract (bumps when set, seeds
from frontFrame.cursor when null, alt-screen no-op, zero-delta no-op).
All 751 ui-tui tests pass; tests/test_tui_gateway_server.py (177) pass.

* fix(tui): also advance cursorDeclaration so fast-echo survives deferred React state

Copilot review on PR #26717 flagged a gap in the original fix:
TextInput's fast-echo path defers the React `cur` state update by
16ms (perf optimization that batches re-renders during heavy typing).
Inside that window, `useDeclaredCursor` still publishes a target
computed from the PRE-keystroke `cur` — `cursorLayout(display, cur,
columns)`. Advancing only `displayCursor` would let any unrelated
re-render in that 16ms window run onRender's cursor-park branch with
the stale declaration and visually undo the fast-echo's advance.

The fix is symmetric: `noteExternalCursorAdvance` now bumps BOTH
`displayCursor` (the log-update relative-move basis) AND, if non-null,
`cursorDeclaration.relativeX/Y` (the target the cursor parks at after
every frame). When React finally flushes `setCur`, `useDeclaredCursor`
publishes a fresh declaration that supersedes our bumped one — exactly
what we want.

Adds two new vitest cases covering both halves:
  - active declaration advances in lock-step with displayCursor
  - null declaration stays null (no spurious bump)

All 753 ui-tui tests pass; tests/test_tui_gateway_server.py (177) pass.

Closes review threads:
  PRRT_kwDOPRF1G86ChKtD (textInput.tsx:1016 fast-echo append)
  PRRT_kwDOPRF1G86ChKtF (textInput.tsx:924 fast-backspace)
  PRRT_kwDOPRF1G86ChKtG (ink-cursor-advance.test.ts:57 missing coverage)

* fix(tui): make fast-echo survive TextInput rerenders + alt-screen (Copilot round 2)

Round 2 of PR #26717 review. Three real holes Copilot flagged after the
initial cursorDeclaration bump:

1. alt-screen early-return skipped BOTH halves of the notifier. But the
   default TUI wraps the composer in <AlternateScreen> — that IS the
   production path. CSI H resets log-update's relative-move basis, but
   the alt-screen park branch uses absolute CUP =
   `rect.x + decl.relativeX`, so a stale declaration there still parks
   the cursor at the pre-keystroke caret. Fix: skip ONLY the
   displayCursor half on alt-screen; still bump cursorDeclaration.

2. TextInput's own rerender could clobber the Ink-level bump. The fast-
   echo path defers setCur by 16ms; if a parent state change rerenders
   TextInput in that window, the layout effect inside useDeclaredCursor
   reads the stale React `cur` state and re-publishes a declaration at
   the OLD column. Fix:
   `cursorLayout(display, curRef.current, columns)` — read the always-
   up-to-date ref, not the deferred state. useMemo dropped (compute is
   cheap, single-line wrap-text in the common case).

3. Tests bypassed the production wiring. Added two structural tests:
   - `still advances cursorDeclaration on alt-screen` in the Ink-level
     suite, asserting displayCursor stays put but the declaration
     advances by the delta.
   - `textInputCursorSourceOfTruth.test.ts` pins three structural
     invariants: layout reads curRef.current, never the bare `cur`
     state, and the fast-echo stdout.write calls remain paired with
     noteCursorAdvance(±N). Source-grep invariants > flaky Ink mount
     tests for this kind of regression.

757/757 ui-tui tests pass (+3 over round 1). type-check clean. lint
introduces zero new errors on touched files. tests/test_tui_gateway_server.py
(177) pass.

Closes review threads:
  PRRT_kwDOPRF1G86ChOG2 (ink.tsx alt-screen guard)
  PRRT_kwDOPRF1G86ChOG9 (textInput.tsx fast-backspace rerender window)
  PRRT_kwDOPRF1G86ChOHC (textInput.tsx fast-append rerender window)
  PRRT_kwDOPRF1G86ChOHJ (alt-screen test asserts wrong invariant)
  PRRT_kwDOPRF1G86ChOHP (missing integration-style coverage)

* fix(tui): reject fast-backspace at soft-wrap boundary (Copilot round 3)

PR #26717 round 3. Copilot caught two real things:

1. `\b \b` cannot move the terminal cursor onto the previous visual
   row across a soft-wrap boundary. When the caret sits at visual
   column 0 of a wrapped row (e.g. value 'hello ' at width 6 →
   cursorLayout produces (line 1, col 0)), backspace would leave the
   physical cursor in place while the logical caret moves up to the
   end of the previous visual line. `noteCursorAdvance(-1)` would then
   feed Ink a wrong delta. Fix: `canFastBackspaceShape` now takes the
   composer width and rejects when `cursorLayout(value, cursor, columns).column === 0`.
   The fast path falls through to the normal Ink render, which
   correctly lays out the new caret position. The PR-description
   inconsistency about alt-screen is fixed in a separate gh pr edit.

Adds 4 new tests in textInputFastEcho.test.ts pinning the rejection at
exact-multiple wrap boundaries plus a positive control inside a
wrapped line and a back-compat case where `columns` is omitted.

761/761 ui-tui tests pass. type-check / lint clean. 177/177 Python
tests/test_tui_gateway_server.py pass.

Closes review threads:
  PRRT_kwDOPRF1G86ChxE5 (textInput.tsx:933 wrap-boundary regression)

* fix(tui): polish doc + tests after Copilot round 4

Three polish points Copilot raised:

1. canFastBackspaceShape doc comment overstated the legacy contract —
   said it conservatively rejects potential wrap boundaries when
   columns is omitted, but the implementation actually skips the
   wrap-boundary check entirely. Reworded to make the legacy behavior
   explicit and warn callers not to rely on protection they don't get.

2. ink-cursor-advance.test.ts rationale comment for the
   'advances cursorDeclaration in lock-step' case still referenced
   the pre-fix `cursorLayout(display, cur, columns)` expression. Now
   accurately describes the current source of truth — `curRef.current`
   in textInput.tsx — and explains the window the bump is bridging.

3. Removed the three `__get*ForTest` accessors from Ink. The test
   file already cast the instance to inspect private state in the
   couple of tests that needed declaration mutation; the rest now use
   a small `peek(ink)` helper that does the same cast for reads. No
   test-only API surface ships in production.

761/761 ui-tui tests pass. type-check clean. lint introduces zero new
errors on touched files. 177/177 tests/test_tui_gateway_server.py pass.

Closes review threads:
  PRRT_kwDOPRF1G86Ch23W (canFastBackspaceShape doc accuracy)
  PRRT_kwDOPRF1G86Ch23f (stale test rationale)
  PRRT_kwDOPRF1G86Ch23p (test-only API surface in production)

* fix(tui): tighten doc + add dy test coverage (Copilot round 5)

Two polish points from round 5:

1. canFastBackspaceShape doc had two paragraphs that conflicted —
   the main 'Additionally rejects when the physical cursor sits at
   visual column 0' was stated unconditionally, then the columns-param
   paragraph qualified that it only happens when columns is passed.
   Reworked into clear 'When supplied / When omitted' branches with a
   concrete example value ('hello ' returns true without columns even
   though it would be unsafe at width 6). No more inconsistency.

2. Added a test asserting cursorDeclaration.relativeY advances when dy
   is non-zero. Existing tests exercised dy on displayCursor only.
   Newlines in fast-echoed text don't currently hit the bypass
   (canFastAppendShape rejects '\n'), but dy is part of the public
   notifier contract and must propagate symmetrically with dx so
   future callers get a fully-implemented contract.

762/762 ui-tui tests pass (+1). type-check / lint / build clean.

Closes review threads:
  PRRT_kwDOPRF1G86Ch6Sz (doc inconsistency)
  PRRT_kwDOPRF1G86Ch6TE (missing dy coverage on declaration)

* fix(tui): doc polish (Copilot round 6)

Four small but valid points:

1. textInputCursorSourceOfTruth.test.ts used bare 'fs'/'path'/'url'
   imports; the rest of ui-tui consistently uses the 'node:' prefix
   (see src/__tests__/useSessionLifecycle.test.ts, src/lib/editor.test.ts).
   Switched to node:fs / node:path / node:url to match convention.

2. CursorAdvanceContext.ts type-level doc described only displayCursor.
   The notifier intentionally also mutates the active cursorDeclaration
   and that's the only part that matters on alt-screen. Reworked the
   doc into a two-part 'updates both' summary with the alt-screen
   asymmetry called out explicitly.

3. use-cursor-advance.ts hook doc had the same problem. Same fix —
   document both pieces of state, both screen modes.

4. App.tsx onCursorAdvance prop comment was incomplete. Same fix —
   describe both state updates and the screen-mode asymmetry.

No behavior change. 762/762 ui-tui tests pass. type-check / lint /
build clean.

Closes review threads (auto-resolved on PR but valid critiques):
  PRRT_kwDOPRF1G86Ch926 (node: prefix on built-in imports)
  PRRT_kwDOPRF1G86Ch92_ (use-cursor-advance.ts doc)
  PRRT_kwDOPRF1G86Ch93H (CursorAdvanceContext.ts type doc)
  PRRT_kwDOPRF1G86Ch93J (App.tsx prop comment)
This commit is contained in:
brooklyn! 2026-05-16 00:28:12 -05:00 committed by GitHub
parent 559c6ad94a
commit 70b663504f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 547 additions and 5 deletions

View file

@ -0,0 +1,50 @@
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')
// Closes Copilot follow-up on PR #26717: the original cursor-drift
// fix bumped Ink's displayCursor / cursorDeclaration on fast-echo, but
// if TextInput itself re-renders before the deferred 16ms `setCur`
// flushes (parent state change, status-bar tick, spinner) the layout
// effect inside `useDeclaredCursor` re-publishes a declaration
// computed from the STALE React `cur` state and clobbers the Ink-level
// 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*\)/)
})
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('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)
const appendPattern = /stdout!\.write\(text\)[\s\S]{0,1000}?noteCursorAdvance\(text\.length\)/
expect(source).toMatch(appendPattern)
})
})

View file

@ -133,4 +133,42 @@ describe('canFastBackspaceShape', () => {
it('rejects deleting an emoji', () => {
expect(canFastBackspaceShape('hi🙂', 'hi🙂'.length)).toBe(false)
})
// Closes Copilot PR #26717 round 3: the "\b \b" sequence cannot move
// the terminal cursor onto the previous visual row across a
// soft-wrap boundary. When the caret sits at visual column 0 of a
// wrapped row (column == 0 in the computed cursor layout), backspace
// would leave the physical cursor in place while the logical caret
// moves up to the end of the previous visual line — desyncing both
// Ink's displayCursor model and the user-visible position. The fast
// path must fall through in that case so the normal Ink render path
// can lay out the correct cursor position.
it('rejects fast-backspace at a soft-wrap boundary when columns is known', () => {
// value width 6 in a column of 6 → cursorLayout produces (line 1, col 0)
// i.e. the caret has overflowed onto the next visual line.
const value = 'hello '
expect(canFastBackspaceShape(value, value.length, 6)).toBe(false)
})
it('rejects fast-backspace at an exact multiple of columns (wide wrap)', () => {
// 12 chars at width 6 → two full visual rows, caret at (line 2, col 0).
const value = 'abcdefghijkl'
expect(canFastBackspaceShape(value, value.length, 6)).toBe(false)
})
it('still accepts fast-backspace inside a wrapped line', () => {
// Caret mid-visual-line — "\b \b" can move the cursor one cell left
// without crossing a wrap boundary.
expect(canFastBackspaceShape('hello world', 'hello world'.length, 20)).toBe(true)
expect(canFastBackspaceShape('abcdefghi', 9, 6)).toBe(true) // visual line 1, col 3 → ok
})
it('skips the wrap-boundary check when columns is omitted (legacy contract)', () => {
// Callers that don't pass `columns` fall back to the pre-wrap-aware
// behavior — the function does NOT magically reject anything that
// could be a wrap boundary without the width. Production callers
// must always pass `columns`; this case is for unit tests of the
// pre-wrap shape contract.
expect(canFastBackspaceShape('hello ', 'hello '.length)).toBe(true)
})
})

View file

@ -16,13 +16,14 @@ import {
type InkExt = typeof Ink & {
stringWidth: (s: string) => number
useCursorAdvance: () => (dx: number, dy?: number) => void
useDeclaredCursor: (a: { line: number; column: number; active: boolean }) => (el: any) => void
useStdout: () => { stdout?: NodeJS.WriteStream }
useTerminalFocus: () => boolean
}
const ink = Ink as unknown as InkExt
const { Box, Text, useStdin, useInput, useStdout, stringWidth, useDeclaredCursor, useTerminalFocus } = ink
const { Box, Text, useStdin, useInput, useStdout, stringWidth, useCursorAdvance, useDeclaredCursor, useTerminalFocus } = ink
const ESC = '\x1b'
const INV = `${ESC}[7m`
@ -238,8 +239,26 @@ export function canFastAppendShape(
* ASCII. Anything else (combining marks, IME compositions, wide chars,
* tabs, ANSI fragments) goes through the normal render path so Ink can
* recompute cell widths.
*
* When `columns` is supplied, ALSO rejects when the physical cursor
* sits at visual column 0 i.e., right after a soft-wrap boundary.
* The "\b \b" sequence cannot move the cursor onto the previous visual
* row (terminals don't back-step across line wraps), so the physical
* cursor would stay put while the logical caret moves to the end of
* the previous visual line, desyncing both Ink's `displayCursor` model
* and the user-visible position.
*
* When `columns` is OMITTED, the wrap-boundary check is skipped
* entirely and the function reverts to the legacy non-wrap-aware
* contract values like `'hello '` will return `true` even though
* they would be unsafe at a width of 6. Production callers (the
* composer's `canFastBackspace` helper) always pass `columns`;
* `columns` is optional only so unit tests of the pre-wrap shape
* contract can keep calling the helper without threading width
* through. Do NOT omit it from any new caller that relies on the
* wrap-boundary protection.
*/
export function canFastBackspaceShape(current: string, cursor: number): boolean {
export function canFastBackspaceShape(current: string, cursor: number, columns?: number): boolean {
if (cursor !== current.length) {
return false
}
@ -252,6 +271,13 @@ export function canFastBackspaceShape(current: string, cursor: number): boolean
return false
}
// 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
}
const removed = current.slice(prevPos(current, cursor), cursor)
return ASCII_PRINTABLE_RE.test(removed)
@ -333,6 +359,7 @@ export function TextInput({
const fwdDel = useFwdDelete(focus)
const termFocus = useTerminalFocus()
const { stdout } = useStdout()
const noteCursorAdvance = useCursorAdvance()
const curRef = useRef(cur)
const selRef = useRef<null | { end: number; start: number }>(null)
@ -368,7 +395,19 @@ export function TextInput({
[sel]
)
const layout = useMemo(() => cursorLayout(display, cur, columns), [columns, cur, display])
// Read `curRef.current` (always up-to-date) rather than the `cur`
// React state. 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 we used the stale
// `cur` state here, the layout effect inside `useDeclaredCursor`
// would publish a stale cursor declaration and clobber the Ink-level
// bump from `noteCursorAdvance(...)`. `cur` is still in scope and
// referenced by setSel/setCur paths below, so React tracks the
// dependency naturally — we just don't use it as the source of truth
// 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 boxRef = useDeclaredCursor({
line: layout.line,
@ -526,7 +565,7 @@ export function TextInput({
canFastEchoBase() && canFastAppendShape(current, cursor, text, columns, lineWidthRef.current)
const canFastBackspace = (current: string, cursor: number) =>
canFastEchoBase() && canFastBackspaceShape(current, cursor)
canFastEchoBase() && canFastBackspaceShape(current, cursor, columns)
const commit = (
next: string,
@ -911,6 +950,12 @@ export function TextInput({
v = v.slice(0, t) + v.slice(c)
c = t
stdout!.write('\b \b')
// 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)
commit(v, c, true, false, false, Math.max(0, lineWidthRef.current - 1))
return
@ -998,6 +1043,14 @@ export function TextInput({
if (simpleAppend) {
stdout!.write(text)
// 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
// / log-update relative-move basis advances with it; otherwise
// 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)
commit(v, c, true, false, false, lineWidthRef.current + stringWidth(text))
return

View file

@ -164,6 +164,7 @@ declare module '@hermes/ink' {
readonly column: number
readonly active: boolean
}): (el: unknown) => void
export function useCursorAdvance(): (dx: number, dy?: number) => void
export function useStdin(): {
readonly stdin: NodeJS.ReadStream
readonly setRawMode: (value: boolean) => void