f875e802c262a4908c558b69560cd4f18fc71fd5
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 github-actions[bot]
|
75f45a0692 |
fmt(js): npm run fix on merge (#65229)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> |
||
|
ethernet
|
d6c76cfbfa |
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.
|
||
|
brooklyn!
|
70b663504f |
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)
|