mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-20 05:01:30 +00:00
* 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)
174 lines
7.7 KiB
TypeScript
174 lines
7.7 KiB
TypeScript
import { describe, expect, it } from 'vitest'
|
|
|
|
import { canFastAppendShape, canFastBackspaceShape } from '../components/textInput.js'
|
|
|
|
// The fast-echo path bypasses Ink and writes characters directly to stdout
|
|
// for the common case of typing plain English at the end of the line. These
|
|
// tests pin the shape preconditions that make that bypass safe.
|
|
//
|
|
// Regression intent: any non-ASCII text — Vietnamese precomposed letters
|
|
// (one grapheme, `text.length === 1`, `stringWidth === 1`, but produced
|
|
// via IME composition across multiple keystrokes), combining marks
|
|
// (zero width), CJK (double width), emoji (variable width), or anything
|
|
// that could be produced by an in-flight IME composition — must NOT
|
|
// take the bypass. Closes:
|
|
// - "TUI is experiencing font errors when using Unicode to type Vietnamese"
|
|
// - #5221 TUI input box renders incorrectly for CJK / East-Asian wide
|
|
// - #7443 CLI TUI renders and deletes Chinese characters incorrectly
|
|
// - #17602 / #17603 Chinese text scattering / ghosting
|
|
|
|
describe('canFastAppendShape', () => {
|
|
const COLS = 40
|
|
|
|
it('accepts plain ASCII appended at end of single-line input', () => {
|
|
expect(canFastAppendShape('hello', 5, 'x', COLS, 5)).toBe(true)
|
|
expect(canFastAppendShape('hello', 5, ' world', COLS, 5)).toBe(true)
|
|
})
|
|
|
|
it('rejects when cursor is not at end of line', () => {
|
|
expect(canFastAppendShape('hello', 3, 'x', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects when current is empty (placeholder render path needed)', () => {
|
|
expect(canFastAppendShape('', 0, 'x', COLS, 0)).toBe(false)
|
|
})
|
|
|
|
it('rejects when current contains a newline (multi-line layout)', () => {
|
|
expect(canFastAppendShape('hi\nthere', 8, 'x', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects when appending would hit the wrap column', () => {
|
|
// Reaching cols on append must trigger a wrap, which the bypass
|
|
// cannot draw. Stay strictly below cols.
|
|
expect(canFastAppendShape('hello', 5, 'x', 6, 5)).toBe(false)
|
|
})
|
|
|
|
// -- Regression coverage: Vietnamese / combining marks / IME --
|
|
|
|
it('rejects Vietnamese precomposed letter ề (U+1EC1) — IME composition path', () => {
|
|
// 'ề' is one grapheme, length 1, width 1, but Vietnamese Telex/IME
|
|
// produces it via a multi-key composition. Fast-echo would commit the
|
|
// intermediate state to stdout and desync once the final commit
|
|
// arrives.
|
|
expect(canFastAppendShape('hello', 5, 'ề', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects Vietnamese tone marks ă, ơ, ư (Latin-Extended-A/B)', () => {
|
|
for (const ch of ['ă', 'ắ', 'ơ', 'ờ', 'ư', 'ự']) {
|
|
expect(canFastAppendShape('hello', 5, ch, COLS, 5)).toBe(false)
|
|
}
|
|
})
|
|
|
|
it('rejects NFD combining marks (U+0300 grave, U+0301 acute, U+0302 circumflex)', () => {
|
|
// Decomposed Vietnamese: 'e' + combining circumflex + combining grave
|
|
// = 'ề'. Each combining mark is zero-width but length 1; without the
|
|
// ASCII guard the second/third keypress would be fast-echoed and
|
|
// desync the cell column.
|
|
expect(canFastAppendShape('hello', 5, '\u0300', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, '\u0301', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, '\u0302', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects CJK (East-Asian wide) characters', () => {
|
|
expect(canFastAppendShape('hello', 5, '你', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, '日本', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects emoji', () => {
|
|
expect(canFastAppendShape('hello', 5, '🙂', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects ANSI-bearing or control text', () => {
|
|
expect(canFastAppendShape('hello', 5, '\x1b[31m', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, '\t', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, '\x7f', COLS, 5)).toBe(false)
|
|
})
|
|
|
|
it('rejects NBSP and Latin-1 letters that would change the line shape', () => {
|
|
expect(canFastAppendShape('hello', 5, '\u00a0', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, 'é', COLS, 5)).toBe(false)
|
|
expect(canFastAppendShape('hello', 5, 'ñ', COLS, 5)).toBe(false)
|
|
})
|
|
})
|
|
|
|
describe('canFastBackspaceShape', () => {
|
|
it('accepts deleting the last ASCII char', () => {
|
|
expect(canFastBackspaceShape('hello', 5)).toBe(true)
|
|
})
|
|
|
|
it('rejects when cursor is not at end', () => {
|
|
expect(canFastBackspaceShape('hello', 3)).toBe(false)
|
|
})
|
|
|
|
it('rejects when there is nothing to delete', () => {
|
|
expect(canFastBackspaceShape('', 0)).toBe(false)
|
|
expect(canFastBackspaceShape('hello', 0)).toBe(false)
|
|
})
|
|
|
|
it('rejects when value contains a newline', () => {
|
|
expect(canFastBackspaceShape('hi\nthere', 8)).toBe(false)
|
|
})
|
|
|
|
it('rejects deleting Vietnamese precomposed letter ề', () => {
|
|
// The "\b \b" shortcut clears one terminal cell; that's fine for a
|
|
// 1-cell ASCII char but if the previous grapheme is a Vietnamese
|
|
// letter that the IME may still be holding open, we want Ink to
|
|
// re-render so composition state stays consistent.
|
|
expect(canFastBackspaceShape('helloề', 'helloề'.length)).toBe(false)
|
|
})
|
|
|
|
it('rejects deleting a CJK character (2 cells)', () => {
|
|
expect(canFastBackspaceShape('hi你', 'hi你'.length)).toBe(false)
|
|
})
|
|
|
|
it('rejects deleting a NFD-composed grapheme with combining marks', () => {
|
|
// 'e' + U+0302 (circumflex) + U+0300 (grave) — final grapheme is one
|
|
// cluster but the previous-grapheme slice is multi-codepoint. Width
|
|
// is 1 but the bypass would be unsafe because the rendered cell
|
|
// already contained the combined glyph.
|
|
const s = 'hello' + 'e\u0302\u0300'
|
|
expect(canFastBackspaceShape(s, s.length)).toBe(false)
|
|
})
|
|
|
|
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)
|
|
})
|
|
})
|