fix: keep cursor on the last line when output has no trailing newline - #982
Merged
sindresorhus merged 4 commits intoAug 3, 2026
Merged
Conversation
buildCursorSuffix assumed the cursor always sits at line visibleLineCount, i.e. just past the last output line. That only holds when the output ends with a newline. Without one — fullscreen mode — the renderer leaves the cursor on the last line itself (line visibleLineCount - 1): the incremental renderer skips the final cursorNextLine and omits the trailing newline on purpose, and the standard renderer simply writes a string that does not end in a newline. Every cursorUp built from that assumption therefore overshoots by one line, placing the terminal cursor one row above where the app asked for it. Thread the trailing-newline flag from log-update into buildCursorSuffix and drop one line when it is absent. The parameter defaults to true, so frames that end with a newline keep emitting byte-identical output.
Every existing sync() test passes a string ending in a newline, so the two sync() paths touched by the previous commit had no committed coverage for the case the fix is about. Add it for both renderers. Also rewrite the buildReturnToBottom() comment. Its arithmetic is already correct for both cases, but the comment explained only the trailing-newline one — an asymmetry that is easy to misread now that buildCursorSuffix() distinguishes them explicitly.
…bling
Replaces the hasTrailingNewline flag introduced in the previous two commits.
The flag was never necessary: the row the renderer leaves the cursor on is
`lines.length - 1` for `lines = str.split('\n')` in both cases. With a trailing
newline the split yields one extra empty element and the renderer stops just
past the last visible line; without one there is no extra element and it stops
on the last visible line. Both are `lines.length - 1`.
That is the basis buildReturnToBottom() already measures from. buildCursorSuffix()
was the odd one out, taking visibleLineCount — which subtracts the trailing empty
element and so only coincides with the cursor's row when a trailing newline is
present. Its first parameter becomes bottomLine and the call sites pass
`lines.length - 1`.
CursorOnlyInput drops visibleLineCount in favour of the previousLineCount it
already carries: buildReturnToBottom() has just placed the cursor on that exact
row, so deriving it from the output a second time recomputed a known value from
a weaker input.
The body of buildCursorSuffix() is unchanged, which is the diagnosis in one line:
the defect was never in the helper, only in what the call sites handed it. Output
is byte-identical to the flag-threading version across 10656 render scenarios.
Collaborator
|
One thing needed: Add an Ink-level regression test that renders exactly to the viewport height with useCursor() on a nonzero row, then verifies a changed rerender and a cursor-only update. The current tests cover the helper directly, but not the fullscreen and sync wiring. |
The existing tests hand log-update a hand-built string, but the trailing newline is decided a layer above, in `outputToRender = isFullscreen ? output : output + '\n'`. A test that skips that layer cannot reach the condition this fix is about, which is why the defect survived the helper's own unit tests. These drive `render()` instead. Fullscreen is induced by setting `rows` on the fake stdout, as `test/terminal-resize.tsx` already does: - A frame that exactly fills the viewport, asserted across a first render, a content-changing rerender, and a cursor-only update. - A frame taller than the viewport, whose second render clears the terminal and repositions through `log.sync()` — the only route by which that call site is reachable from a real app. Both run against `createStandard` and `createIncremental`. They are separate implementations, so present-day equivalence is not a reason to leave one untested; and the behaviour that creates the precondition — skipping the final `cursorNextLine` when there is no trailing newline — exists only in the incremental renderer. Each case asserts that the corrected sequence is present *and* that the pre-fix one is absent, so a frame emitting both would still fail. Also covers the incremental shrink branch with an active cursor. It reaches the shared cursor suffix through different arithmetic than the grow path (`eraseLines()` + `cursorUp(visibleCount)` rather than `cursorUp(previousLines.length - 1)`), and this change altered what that call site is passed. All 11 tests added by this PR fail on master.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
buildCursorSuffix()states its precondition in its own docstring:log-updatebreaks that precondition whenever the rendered string has no trailing newline (fullscreen mode). In that case both renderers deliberately leave the cursor on the last line instead of past it:createIncrementalskips the finalcursorNextLineand omits the trailing\nfor the last line — the code says so explicitly: "Don't move past the last line when there's no trailing newline, otherwise the cursor overshoots the rendered block."createStandardwrites the raw string, which ends mid-line.So the cursor is at line
visibleLineCount - 1, whilebuildCursorSuffixcomputesmoveUp = visibleLineCount - cursorPosition.y. Every such frame overshoots by exactly one row.Impact
Apps that use
useCursor()in fullscreen (an Ink frame that fills the viewport) put the terminal's real cursor one line above the line they asked for. The column is always correct — only the row is off, and it is off on every render.From a real capture: the app requested
y = 1in a 4-line frame and Ink emittedESC[3A ESC[8GwhereESC[2A ESC[8Gwas correct, landing the cursor on the divider line above the input line.Fix
The row the renderer leaves the cursor on is
lines.length - 1forlines = str.split('\n')— in both cases:splityields one extra empty element and the renderer stops just past the last visible line:lines.length - 1.lines.length - 1.That is exactly the basis
buildReturnToBottom()already measures from (previousLineCount - 1).buildCursorSuffix()was the odd one out: it tookvisibleLineCount, which subtracts the trailing empty element and therefore only coincides with the cursor's row when a trailing newline is present.So the fix is not to thread a flag around — it is to give
buildCursorSuffix()the same row basis its sibling already uses. Its first parameter becomesbottomLine, callers passlines.length - 1, andCursorOnlyInputdropsvisibleLineCountin favour of thepreviousLineCountit already carries:buildReturnToBottom()has just placed the cursor on that exact row, so deriving it from the output a second time was recomputing a known value from a weaker input.log-update.tscomes out shorter than it went in. The function body ofbuildCursorSuffix()is unchanged — only its contract and its callers move — which is itself the diagnosis: the defect was never in the helper, it was in what the call sites handed it. The existingbuildCursorSuffixunit tests pass untouched, sincevisibleLineCountandbottomLinecoincide for the trailing-newline inputs they use.Frames that end with a newline emit byte-identical output. Verified by diffing the raw stream writes against
masteracross 14 render paths on both renderers — cursor-only updates, shrink, grow, unchanged-last-line, trailing→non-trailing transition, andsync().Note for anyone tempted by an even smaller diff: subtracting 1 from
visibleLineCountunconditionally is not correct. It breaks trailing-newline frames and produces a visible drift where the frame walks down the screen on each keystroke.Relation to existing work
cursorUp(previousVisible - 1)→cursorUp(previousLines.length - 1). This PR fixes the other end of the same frame — the cursor suffix — which was left with the original assumption.buildCursorSuffixwas never taught about.ink@7.0.3— hit this in practice and shipped the same fix as a vendoredpatch-packagepatch in QwenLM/qwen-code#7998 (merged 2026-07-29), closing their issue #7980: the hardware cursor rendering one row above the input line, and IME composition windows appearing one row too high as a result. Their patch threads ahasTrailingNewlineflag through the helpers; this PR corrects the row basis instead, which turns out to make the flag unnecessary. The diagnosis and the observable signature are identical, though — their pty capture showscursorUp(3)emitted wherecursorUp(2)was correct, and they reached the same conclusion thatbuildReturnToBottom()needs no change. I arrived at the same diagnosis independently while debugging the same symptom. Since upstream still carries the bug, downstream consumers are each maintaining their own copy of it.viewportHeightparameter to the samebuildCursorSuffixsignature for an unrelated concern (clampingcursorUpto the viewport), so the two will conflict textually. The changes are orthogonal — clamping bounds the distance, this PR fixes the starting point — and I'm happy to rebase on top of fix: clamp cursor-up to viewport height, preventing terminal scroll-to-top #917 in whichever order suits you.Verification
Replayed the emitted escape sequences through a cursor tracker: after the fix the cursor lands exactly on the requested
(x, y)in every tested case on both renderers; before the fix it is 1–2 rows high whenever the frame has no trailing newline (the error compounds across consecutive renders, because the next frame'sbuildReturnToBottomstarts from a position that is already wrong).11 tests added, all of which fail on
masterand pass with this change. They are integration tests by necessity — as noted above,buildCursorSuffix()'s body is unchanged, so no unit test of it can distinguish the two versions.Four drive Ink itself (
test/cursor.tsx), which is where the trailing newline is decided. Both are parameterised overincrementalRendering: falseandtrue, sincecreateStandardandcreateIncrementalare separate implementations:log.sync()— the only route by which that call site is reachable from a real app.Each asserts both that the corrected sequence is present and that the pre-fix one is absent, so a frame emitting both would still fail. Fullscreen is induced by assigning
rowson the fake stdout, following the existing convention intest/terminal-resize.tsx; the tests awaitwaitUntilRenderFlush()rather than a fixed delay.Seven exercise
log-updatedirectly (test/log-update.tsx), parameterised over both renderers where the path exists in both: the cursor suffix after a normal render, after a cursor-only update, aftersync(), and after the incremental shrink branch — which establishes the cursor's starting row with different arithmetic than the grow path but shares the same suffix call.The incremental test also asserts its first write, covering the
previousOutput.length === 0branch. Ink reaches that branch in production wheneveruseStdout().write()clears and restores the frame, so in fullscreen it runs with no trailing newline and a live cursor.npm testpasses: 1047 tests, 4 known failures (pre-existingtest.failing()declarations inwidth-height/flex-justify-content, unrelated to this area).