From 70b663504fee1d58a6763e862df478cf101fe51e Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Sat, 16 May 2026 00:28:12 -0500 Subject: [PATCH] fix(tui): keep Ink displayCursor in sync with fast-echo writes so cursor stops drifting (#26717) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 — 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) --- ui-tui/packages/hermes-ink/index.d.ts | 1 + .../packages/hermes-ink/src/entry-exports.ts | 1 + .../hermes-ink/src/ink/components/App.tsx | 17 +- .../ink/components/CursorAdvanceContext.ts | 35 +++ .../src/ink/hooks/use-cursor-advance.ts | 33 +++ .../src/ink/ink-cursor-advance.test.ts | 234 ++++++++++++++++++ ui-tui/packages/hermes-ink/src/ink/ink.tsx | 81 ++++++ .../textInputCursorSourceOfTruth.test.ts | 50 ++++ .../src/__tests__/textInputFastEcho.test.ts | 38 +++ ui-tui/src/components/textInput.tsx | 61 ++++- ui-tui/src/types/hermes-ink.d.ts | 1 + 11 files changed, 547 insertions(+), 5 deletions(-) create mode 100644 ui-tui/packages/hermes-ink/src/ink/components/CursorAdvanceContext.ts create mode 100644 ui-tui/packages/hermes-ink/src/ink/hooks/use-cursor-advance.ts create mode 100644 ui-tui/packages/hermes-ink/src/ink/ink-cursor-advance.test.ts create mode 100644 ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts diff --git a/ui-tui/packages/hermes-ink/index.d.ts b/ui-tui/packages/hermes-ink/index.d.ts index 637c4bb43b6..5d5ae9387c0 100644 --- a/ui-tui/packages/hermes-ink/index.d.ts +++ b/ui-tui/packages/hermes-ink/index.d.ts @@ -21,6 +21,7 @@ export { default as Text } from './src/ink/components/Text.tsx' export type { Props as TextProps } from './src/ink/components/Text.tsx' export type { Key } from './src/ink/events/input-event.ts' export { default as useApp } from './src/ink/hooks/use-app.ts' +export { useCursorAdvance } from './src/ink/hooks/use-cursor-advance.ts' export { useDeclaredCursor } from './src/ink/hooks/use-declared-cursor.ts' export { default as useInput } from './src/ink/hooks/use-input.ts' export { useHasSelection, useSelection } from './src/ink/hooks/use-selection.ts' diff --git a/ui-tui/packages/hermes-ink/src/entry-exports.ts b/ui-tui/packages/hermes-ink/src/entry-exports.ts index 355faa16f97..d173e0c9bb1 100644 --- a/ui-tui/packages/hermes-ink/src/entry-exports.ts +++ b/ui-tui/packages/hermes-ink/src/entry-exports.ts @@ -12,6 +12,7 @@ export { default as ScrollBox } from './ink/components/ScrollBox.js' export { default as Spacer } from './ink/components/Spacer.js' export { default as Text } from './ink/components/Text.js' export { default as useApp } from './ink/hooks/use-app.js' +export { useCursorAdvance } from './ink/hooks/use-cursor-advance.js' export { useDeclaredCursor } from './ink/hooks/use-declared-cursor.js' export { type RunExternalProcess, useExternalProcess, withInkSuspended } from './ink/hooks/use-external-process.js' export { default as useInput } from './ink/hooks/use-input.js' diff --git a/ui-tui/packages/hermes-ink/src/ink/components/App.tsx b/ui-tui/packages/hermes-ink/src/ink/components/App.tsx index 5851c4bef66..54892e3b7b1 100644 --- a/ui-tui/packages/hermes-ink/src/ink/components/App.tsx +++ b/ui-tui/packages/hermes-ink/src/ink/components/App.tsx @@ -33,6 +33,7 @@ import { DBP, DFE, DISABLE_MOUSE_TRACKING, EBP, EFE, SHOW_CURSOR } from '../term import AppContext from './AppContext.js' import { ClockProvider } from './ClockContext.js' +import CursorAdvanceContext, { type CursorAdvanceNotifier } from './CursorAdvanceContext.js' import CursorDeclarationContext, { type CursorDeclarationSetter } from './CursorDeclarationContext.js' import ErrorOverview from './ErrorOverview.js' import StdinContext from './StdinContext.js' @@ -100,6 +101,18 @@ type Props = { // Enables IME composition at the input caret and lets screen readers / // magnifiers track the input. Optional so testing.tsx doesn't stub it. readonly onCursorDeclaration?: CursorDeclarationSetter + // Receives notifications that the physical cursor was advanced out-of-band + // (e.g. TextInput's fast-echo bypass writing directly to stdout). The + // handler in ink.tsx updates two pieces of state from a single call: + // - `displayCursor` (the relative-move basis log-update uses on the + // next frame; skipped on alt-screen where CSI H resets it every + // frame anyway), and + // - the active `cursorDeclaration.relativeX/Y` (the target the cursor + // parks at after every frame; bumped on BOTH screens because + // onRender's alt-screen branch emits an absolute CUP from it and + // a stale declaration there is still visibly wrong). + // Optional so testing.tsx doesn't need to stub it. + readonly onCursorAdvance?: CursorAdvanceNotifier // Dispatch a keyboard event through the DOM tree. Called for each // parsed key alongside the legacy EventEmitter path. readonly dispatchKeyboardEvent: (parsedKey: ParsedKey) => void @@ -196,7 +209,9 @@ export default class App extends PureComponent { {})}> - {this.state.error ? : this.props.children} + {})}> + {this.state.error ? : this.props.children} + diff --git a/ui-tui/packages/hermes-ink/src/ink/components/CursorAdvanceContext.ts b/ui-tui/packages/hermes-ink/src/ink/components/CursorAdvanceContext.ts new file mode 100644 index 00000000000..52566c1a917 --- /dev/null +++ b/ui-tui/packages/hermes-ink/src/ink/components/CursorAdvanceContext.ts @@ -0,0 +1,35 @@ +import { createContext } from 'react' + +/** + * Notify Ink that the physical terminal cursor was advanced by an + * out-of-band stdout.write (e.g. the TextInput fast-echo path). + * + * This is a two-part notification — calling it updates both: + * + * 1. Ink's cached `displayCursor` (the basis log-update uses to + * compute relative cursor moves for the next frame's preamble). + * Without this, the next frame's preamble starts from a stale + * parked position and the diff is rendered N cells offset. + * This half is SKIPPED on alt-screen — every alt-screen frame + * begins with CSI H which absolutely repositions the cursor, so + * the relative-move basis is reset for free. + * + * 2. Ink's active `cursorDeclaration` (the target the cursor parks + * at after every frame, set by `useDeclaredCursor`). Without + * this, an unrelated component re-rendering before the deferred + * React state catches up would publish a stale declaration and + * visually undo the fast-echo's advance. This half applies to + * BOTH main-screen and alt-screen — on alt-screen the cursor- + * park branch in onRender emits an absolute CUP to + * `rect.x + decl.relativeX`, so a stale declaration there is + * still wrong even though displayCursor is skipped. + * + * `dx`/`dy` are deltas in terminal cells (positive = right/down, + * negative = left/up). The caller is responsible for ensuring the + * physical cursor really did move by that amount. + */ +export type CursorAdvanceNotifier = (dx: number, dy?: number) => void + +const CursorAdvanceContext = createContext(() => {}) + +export default CursorAdvanceContext diff --git a/ui-tui/packages/hermes-ink/src/ink/hooks/use-cursor-advance.ts b/ui-tui/packages/hermes-ink/src/ink/hooks/use-cursor-advance.ts new file mode 100644 index 00000000000..15831ed86ab --- /dev/null +++ b/ui-tui/packages/hermes-ink/src/ink/hooks/use-cursor-advance.ts @@ -0,0 +1,33 @@ +import { useContext } from 'react' + +import CursorAdvanceContext, { type CursorAdvanceNotifier } from '../components/CursorAdvanceContext.js' + +/** + * Returns a function that notifies Ink the physical terminal cursor was + * advanced out-of-band (e.g. by a direct stdout.write from the + * TextInput fast-echo bypass). + * + * Calling the returned function updates two pieces of Ink state: + * + * - `displayCursor` — the cached parked-cursor position log-update + * uses as the relative-move basis for the next frame. Skipped on + * alt-screen, where every frame's CSI H resets the cursor anyway. + * + * - The active `cursorDeclaration` — the target the cursor parks at + * after every frame. Bumped on BOTH main- and alt-screen, because + * onRender's alt-screen park branch emits an absolute CUP from + * this value and a stale declaration there is still visibly wrong. + * The next React commit that publishes a fresh declaration + * supersedes the bump. + * + * The caller is responsible for the stdout write itself; this hook + * only reports the resulting cursor delta. Pass `dx` and optional + * `dy` in terminal cells (positive = moved right/down, negative = + * moved left/up). + * + * If the host isn't an Ink render root (test stubs, non-Ink renderer) + * the returned callback is a safe no-op. + */ +export function useCursorAdvance(): CursorAdvanceNotifier { + return useContext(CursorAdvanceContext) +} diff --git a/ui-tui/packages/hermes-ink/src/ink/ink-cursor-advance.test.ts b/ui-tui/packages/hermes-ink/src/ink/ink-cursor-advance.test.ts new file mode 100644 index 00000000000..a3cc1757ab6 --- /dev/null +++ b/ui-tui/packages/hermes-ink/src/ink/ink-cursor-advance.test.ts @@ -0,0 +1,234 @@ +import { EventEmitter } from 'events' + +import React from 'react' +import { describe, expect, it } from 'vitest' + +import Text from './components/Text.js' +import Ink from './ink.js' + +class FakeTty extends EventEmitter { + chunks: string[] = [] + columns = 40 + rows = 8 + isTTY = true + + write(chunk: string | Uint8Array, cb?: (err?: Error | null) => void): boolean { + this.chunks.push(typeof chunk === 'string' ? chunk : Buffer.from(chunk).toString('utf8')) + cb?.() + + return true + } +} + +function makeInk() { + const stdout = new FakeTty() + const stdin = new FakeTty() + const stderr = new FakeTty() + + const ink = new Ink({ + exitOnCtrlC: false, + patchConsole: false, + stderr: stderr as unknown as NodeJS.WriteStream, + stdin: stdin as unknown as NodeJS.ReadStream, + stdout: stdout as unknown as NodeJS.WriteStream + }) + + return { ink, stdout, stdin, stderr } +} + +// Cast helper instead of exposing __get*ForTest methods on production Ink — +// these are internal frame/cursor caches we only inspect from tests. +type InkPrivate = { + displayCursor: { x: number; y: number } | null + cursorDeclaration: { node: unknown; relativeX: number; relativeY: number } | null + frontFrame: { cursor: { x: number; y: number } } +} +const peek = (ink: Ink): InkPrivate => ink as unknown as InkPrivate + +// Closes the cursor-drift bug: when TextInput's fast-echo path writes a +// printable character directly to stdout, the hardware cursor advances by +// one cell BUT Ink's `displayCursor` cache (used as the basis for the +// next frame's relative cursor preamble) wasn't being updated. On long +// sessions an unrelated re-render (status bar timer, streaming +// reasoning, etc.) would then park the hardware cursor N cells offset +// from the actual caret — visible as "extra whitespace between my last +// typed character and the cursor block". +describe('Ink.noteExternalCursorAdvance', () => { + it('bumps an already-tracked displayCursor by the given delta', () => { + const { ink } = makeInk() + + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + + // Seed a known parked position directly. In production this is set by + // the cursor-park branch in onRender when a useDeclaredCursor caller + // commits a declaration; this test bypasses React for hermeticity. + peek(ink).displayCursor = { x: 5, y: 0 } + + ink.noteExternalCursorAdvance(3) + expect(peek(ink).displayCursor).toEqual({ x: 8, y: 0 }) + + ink.noteExternalCursorAdvance(-1) + expect(peek(ink).displayCursor).toEqual({ x: 7, y: 0 }) + + ink.noteExternalCursorAdvance(0, 2) + expect(peek(ink).displayCursor).toEqual({ x: 7, y: 2 }) + + ink.unmount() + }) + + it('seeds displayCursor from frontFrame.cursor when nothing was parked', () => { + const { ink } = makeInk() + + ink.render(React.createElement(Text, null, 'hello')) + ink.onRender() + + expect(peek(ink).displayCursor).toBeNull() + const base = { x: peek(ink).frontFrame.cursor.x, y: peek(ink).frontFrame.cursor.y } + + ink.noteExternalCursorAdvance(4) + expect(peek(ink).displayCursor).toEqual({ x: base.x + 4, y: base.y }) + + ink.unmount() + }) + + it('is a no-op when the delta is zero', () => { + const { ink } = makeInk() + + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + + ink.noteExternalCursorAdvance(0) + expect(peek(ink).displayCursor).toBeNull() + + ink.noteExternalCursorAdvance(0, 0) + expect(peek(ink).displayCursor).toBeNull() + + ink.unmount() + }) + + it('skips displayCursor on alt-screen — CSI H resets every frame', () => { + const { ink } = makeInk() + + ink.setAltScreenActive(true) + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + peek(ink).displayCursor = { x: 5, y: 0 } + + ink.noteExternalCursorAdvance(3) + + expect(peek(ink).displayCursor).toEqual({ x: 5, y: 0 }) + + ink.unmount() + }) + + // Closes Copilot follow-up on PR #26717: the default TUI wraps the + // composer in , so alt-screen is the production + // path. CSI H only resets the log-update relative-move basis — the + // declared cursor target is still consulted by onRender's alt-screen + // park branch (`cursorPosition(row, col)` using rect + decl). So + // cursorDeclaration MUST advance on alt-screen too, even though + // displayCursor doesn't need to. + it('still advances cursorDeclaration on alt-screen', () => { + const { ink } = makeInk() + + ink.setAltScreenActive(true) + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + + const fakeNode = {} as unknown as Record + + peek(ink).cursorDeclaration = { node: fakeNode, relativeX: 7, relativeY: 0 } + peek(ink).displayCursor = { x: 12, y: 0 } + + ink.noteExternalCursorAdvance(3) + + // displayCursor untouched on alt-screen + expect(peek(ink).displayCursor).toEqual({ x: 12, y: 0 }) + // declaration still advanced — onRender's alt-screen park reads this + expect(peek(ink).cursorDeclaration).toEqual({ node: fakeNode, relativeX: 10, relativeY: 0 }) + + ink.unmount() + }) + + // Closes Copilot review feedback on PR #26717: even after the + // TextInput-level fix where layout reads `curRef.current` directly, + // there's still a window where a fast-echo wrote to stdout but the + // current cursor declaration on Ink (set by an earlier render's + // useDeclaredCursor commit) points at the PRE-keystroke caret + // column. If we advanced only `displayCursor`, an unrelated re-render + // in that window would re-run onRender's cursor-park branch with the + // stale declaration and visually undo the fast-echo's advance. We + // must bump BOTH so the cursor stays anchored to the physical caret + // until the next React commit publishes a fresh declaration + // (computed from `curRef.current` via the cursorLayout call in + // textInput.tsx) that supersedes the bump. + it('advances the active cursorDeclaration in lock-step with displayCursor', () => { + const { ink } = makeInk() + + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + + const fakeNode = {} as unknown as Record + + peek(ink).cursorDeclaration = { node: fakeNode, relativeX: 7, relativeY: 0 } + peek(ink).displayCursor = { x: 12, y: 0 } + + ink.noteExternalCursorAdvance(3) + + expect(peek(ink).displayCursor).toEqual({ x: 15, y: 0 }) + expect(peek(ink).cursorDeclaration).toEqual({ node: fakeNode, relativeX: 10, relativeY: 0 }) + + ink.noteExternalCursorAdvance(-1) + expect(peek(ink).displayCursor).toEqual({ x: 14, y: 0 }) + expect(peek(ink).cursorDeclaration).toEqual({ node: fakeNode, relativeX: 9, relativeY: 0 }) + + ink.unmount() + }) + + // Closes Copilot follow-up on PR #26717: the dy half of the notifier + // contract was tested for `displayCursor` but not for + // `cursorDeclaration.relativeY`. Newlines in fast-echoed text never + // hit the bypass today (canFastAppendShape rejects '\n'), but `dy` + // is part of the public API and must propagate symmetrically with + // dx so future callers (e.g. multi-line paste shortcuts) don't get + // a half-implemented contract. + it('advances cursorDeclaration.relativeY when dy is non-zero', () => { + const { ink } = makeInk() + + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + + const fakeNode = {} as unknown as Record + + peek(ink).cursorDeclaration = { node: fakeNode, relativeX: 2, relativeY: 1 } + peek(ink).displayCursor = { x: 4, y: 2 } + + ink.noteExternalCursorAdvance(1, 3) + + expect(peek(ink).displayCursor).toEqual({ x: 5, y: 5 }) + expect(peek(ink).cursorDeclaration).toEqual({ node: fakeNode, relativeX: 3, relativeY: 4 }) + + // Negative dy too — cursor moving up across visual rows. + ink.noteExternalCursorAdvance(0, -2) + expect(peek(ink).displayCursor).toEqual({ x: 5, y: 3 }) + expect(peek(ink).cursorDeclaration).toEqual({ node: fakeNode, relativeX: 3, relativeY: 2 }) + + ink.unmount() + }) + + it('leaves cursorDeclaration unchanged when no declaration is active', () => { + const { ink } = makeInk() + + ink.render(React.createElement(Text, null, 'hi')) + ink.onRender() + + expect(peek(ink).cursorDeclaration).toBeNull() + + ink.noteExternalCursorAdvance(3) + + expect(peek(ink).cursorDeclaration).toBeNull() + + ink.unmount() + }) +}) diff --git a/ui-tui/packages/hermes-ink/src/ink/ink.tsx b/ui-tui/packages/hermes-ink/src/ink/ink.tsx index 8cdfe781395..49fdf704488 100644 --- a/ui-tui/packages/hermes-ink/src/ink/ink.tsx +++ b/ui-tui/packages/hermes-ink/src/ink/ink.tsx @@ -16,6 +16,7 @@ import { logError } from '../utils/log.js' import { colorize } from './colorize.js' import App from './components/App.js' +import type { CursorAdvanceNotifier } from './components/CursorAdvanceContext.js' import type { CursorDeclaration, CursorDeclarationSetter } from './components/CursorDeclarationContext.js' import { FRAME_INTERVAL_MS } from './constants.js' import * as dom from './dom.js' @@ -2219,6 +2220,85 @@ export default class Ink { this.cursorDeclaration = decl } + // Caller writes raw bytes to stdout that move the physical terminal + // cursor (e.g. TextInput's fast-echo bypass). Without this notification, + // Ink's `displayCursor` cache and log-update's prevFrame.cursor stay + // unchanged, so the next frame's relative cursor moves compute from a + // stale position and the hardware cursor parks `dx` cells offset from + // the actual caret. Visible symptom: extra whitespace between the just- + // typed character and the cursor block, more pronounced on long + // sessions where unrelated components re-render between fast-echo and + // the deferred composer re-render. + // + // If displayCursor was already tracked, just bump it. Otherwise seed it + // to (prevFrame.cursor + delta) so the next frame's preamble emits a + // (-dx, -dy) relative move that brings the cursor back to log-update's + // expected start position before the diff body runs. + // + // Public so tests can drive it directly without mounting App. + // + // Bumps BOTH `displayCursor` (used by log-update's relative-move + // preamble) AND, if non-null, `cursorDeclaration.relativeX/Y` (the + // target the cursor parks at after every frame). Advancing only one + // of the two would leave the other stale: e.g. if the deferred React + // `setCur` hasn't flushed yet, the next unrelated re-render would + // re-compute `target` from the stale declaration and park the + // hardware cursor back at the old caret column. We advance both so + // the fast-echo is invisible to intervening frames until React + // catches up. + noteExternalCursorAdvance: CursorAdvanceNotifier = (dx, dy = 0) => { + if (dx === 0 && dy === 0) { + return + } + + // displayCursor / log-update relative-move basis only matters on + // main screen — alt-screen frames begin with absolute CSI H every + // frame so the next preamble naturally resets to (0,0). cursorDeclaration, + // however, IS still consulted on alt-screen — onRender's park branch + // emits an absolute CUP using `rect.x + decl.relativeX`, so a stale + // declaration in the deferred-setCur window would park the cursor + // at the pre-keystroke caret. We therefore skip ONLY the displayCursor + // half on alt-screen, not the declaration half. + if (!this.altScreenActive) { + if (this.displayCursor !== null) { + this.displayCursor = { + x: this.displayCursor.x + dx, + y: this.displayCursor.y + dy + } + } else { + // No prior parked position. Seed from frontFrame.cursor (where + // log-update parked the cursor at the end of the last frame) so + // the next preamble's relative move correctly cancels the + // external advance. + const baseX = this.frontFrame.cursor.x + const baseY = this.frontFrame.cursor.y + this.displayCursor = { x: baseX + dx, y: baseY + dy } + } + } + + // Also advance the active cursor declaration if any. Without this, + // a TextInput that defers its React `cur` state update (16ms timer + // in textInput.tsx — perf optimization that batches re-renders + // during heavy typing) leaves `cursorDeclaration.relativeX` pointing + // at the pre-keystroke caret column. If an unrelated component + // re-renders before the deferred `setCur` flushes, the cursor-park + // branch at the end of onRender would move the hardware cursor back + // to that stale relativeX and visually undo the fast-echo's + // advance. Bumping relativeX here keeps the declared target in + // lock-step with the physical cursor until React state catches up. + // Applies to BOTH main-screen and alt-screen — the alt-screen park + // branch uses an absolute CUP to (rect.x + decl.relativeX), so a + // stale declaration there would still produce the wrong column. + const decl = this.cursorDeclaration + + if (decl !== null) { + this.cursorDeclaration = { + node: decl.node, + relativeX: decl.relativeX + dx, + relativeY: decl.relativeY + dy + } + } + } render(node: ReactNode): void { this.currentNode = node @@ -2228,6 +2308,7 @@ export default class Ink { exitOnCtrlC={this.options.exitOnCtrlC} getHyperlinkAt={this.getHyperlinkAt} onClickAt={this.dispatchClick} + onCursorAdvance={this.noteExternalCursorAdvance} onCursorDeclaration={this.setCursorDeclaration} onExit={this.unmount} onHoverAt={this.dispatchHover} diff --git a/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts b/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts new file mode 100644 index 00000000000..b52894d1587 --- /dev/null +++ b/ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts @@ -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) + }) +}) diff --git a/ui-tui/src/__tests__/textInputFastEcho.test.ts b/ui-tui/src/__tests__/textInputFastEcho.test.ts index 7f246f19f21..2e08111ffb4 100644 --- a/ui-tui/src/__tests__/textInputFastEcho.test.ts +++ b/ui-tui/src/__tests__/textInputFastEcho.test.ts @@ -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) + }) }) diff --git a/ui-tui/src/components/textInput.tsx b/ui-tui/src/components/textInput.tsx index 91e109fa366..b3c79357368 100644 --- a/ui-tui/src/components/textInput.tsx +++ b/ui-tui/src/components/textInput.tsx @@ -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) @@ -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 diff --git a/ui-tui/src/types/hermes-ink.d.ts b/ui-tui/src/types/hermes-ink.d.ts index b84f843d322..ca2a05dc449 100644 --- a/ui-tui/src/types/hermes-ink.d.ts +++ b/ui-tui/src/types/hermes-ink.d.ts @@ -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