Skip to content

fix(cli): truncate Spinner message to terminal width on TTY output (#6975)#7157

Open
LeSingh1 wants to merge 2 commits into
denoland:mainfrom
LeSingh1:fix/6975-spinner-truncate
Open

fix(cli): truncate Spinner message to terminal width on TTY output (#6975)#7157
LeSingh1 wants to merge 2 commits into
denoland:mainfrom
LeSingh1:fix/6975-spinner-truncate

Conversation

@LeSingh1

Copy link
Copy Markdown
Contributor

Fixes #6975.

Spinner clears the current line with \r\x1b[K before each frame, but when the rendered message is wider than the terminal, the cursor is now on the wrapped line and the clear only nukes that one. Each tick then prints a fresh wrapped block, so the output scrolls indefinitely. Repro from the issue:

import { Spinner } from "@std/cli/unstable-spinner";
new Spinner({ message: "x".repeat(1000) }).start();

Followed the truncation approach BlackAsLight and tomas-zijdemans agreed on in the thread — simpler than save/restore cursor and self-contained in @std/cli/unstable-spinner. Truncate the message so prefixWidth (glyph + space) + unicodeWidth(message) ≤ columns. Width is measured with the existing unicodeWidth helper so wide CJK / emoji characters that would straddle the boundary are dropped, not half-rendered.

Only truncates when the output is a TTY (Deno.stdout.isTerminal() etc.). Piped or redirected output gets the full message untouched so logs stay grep-able.

Tests

Three new cases in cli/unstable_spinner_test.ts:

  • truncates message to terminal width when output is a TTY — 50-x message, 20-column terminal, asserts each frame ends at 18 x's.
  • does not truncate when output is not a TTY — stubs isTerminal() false with consoleSize reporting 10 columns, asserts the full 50-x message is rendered.
  • truncates respecting wide-character (emoji) width — 10 dinos in 10 columns yields 4 (each 🦕 is 2 columns).
$ deno test --allow-read --allow-write cli/unstable_spinner_test.ts
ok | 14 passed (9 steps) | 0 failed (3s)

$ deno lint cli/unstable_spinner.ts cli/unstable_spinner_test.ts
Checked 2 files

$ deno check cli/unstable_spinner.ts
Check cli/unstable_spinner.ts

…enoland#6975)

When the spinner message is wider than the terminal, `\r[K` only clears
the line the cursor was on. The next frame then prints a wrapped block
again, so each tick adds another stack of stale lines instead of
overwriting the previous one. The repro from the issue
(`new Spinner({ message: "x".repeat(1000) }).start()`) scrolls the
terminal indefinitely.

Truncate the rendered message so the visual width of the prefix
(`spinner glyph + space`) plus the message stays within
`Deno.consoleSize().columns`. Width is measured with the existing
`unicodeWidth` helper so wide CJK / emoji characters that would
straddle the boundary are dropped rather than half-rendered.

Truncation only happens when the output is a TTY. Piped or redirected
output (where wrapping is not visually catastrophic) keeps the full
message so logs are still grep-able.

This matches the approach BlackAsLight and tomas-zijdemans agreed on in
the issue thread — simpler than save/restore cursor and self-contained
within `@std/cli/unstable-spinner`.

Adds three tests in `cli/unstable_spinner_test.ts`: ASCII truncation
to a stubbed 20-column terminal, emoji truncation to 10 columns (4 of
the 10 input dinos fit), and non-TTY no-truncation. All 14 spinner
tests pass; lint and typecheck clean.
@github-actions github-actions Bot added the cli label May 26, 2026
@codecov

codecov Bot commented May 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.57%. Comparing base (f0c9f14) to head (1fe7d55).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cli/unstable_spinner.ts 83.33% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7157      +/-   ##
==========================================
- Coverage   94.57%   94.57%   -0.01%     
==========================================
  Files         636      636              
  Lines       52138    52161      +23     
  Branches     9399     9407       +8     
==========================================
+ Hits        49311    49330      +19     
- Misses       2249     2251       +2     
- Partials      578      580       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@BlackAsLight

Copy link
Copy Markdown
Contributor

I think enabling and disabling auto wrap would be far more efficient and reliable than counting the string and truncating it by hand.

Simply put \x1b[?7l before the message displayed to the user and \x1b[?7h afterwards.

The terminal will truncate the string itself then and this is far more reliable as there isn't a good way to tell which UTF-8 version a terminal is using and may have slightly different behaviour around less common characters.

Per @BlackAsLight's review on denoland#7157: drop the unicode_width dependency
and let the terminal do the truncation by toggling DECAWM (\\u001b[?7l
off / \\u001b[?7h on) around each frame. The terminal silently drops
anything past the right edge so \\r\\u001b[K keeps clearing the same row.

Avoids guessing display widths for emoji, ZWJ sequences, combining
marks, and unusual terminal Unicode handling — the terminal already
knows. Drops the truncateToWidth + #terminalColumns helpers and the
unicodeWidth import.

The TTY-vs-not check is preserved: on non-TTY output we don't emit the
toggles, so piped/redirected output stays clean of escape bytes.

Tests updated:
- The TTY case now asserts the full frame is bracketed by DECAWM
  toggles (no user-space truncation expected).
- The non-TTY case unchanged in spirit (full message, no toggles).
- The wide-character/emoji truncation test is removed: width is no
  longer computed in JS.
@LeSingh1

Copy link
Copy Markdown
Contributor Author

Addressed in 1fe7d55. Dropped the unicode_width import + the JS-side width math and now toggle DECAWM around each TTY frame, exactly as you suggested: �[?7l before the frame, �[?7h after. The terminal silently drops anything past the right edge so \r�[K keeps clearing the same row.

Net diff: -64 lines. truncateToWidth, #terminalColumns, and the unicodeWidth import are gone, replaced by a small #isTerminal check and a pair of pre-encoded Uint8Arrays for the DECAWM bytes.

On non-TTY output the toggles are skipped so piped/redirected output stays clean of escape bytes. Tests updated:

  • the TTY case now asserts the full frame is bracketed by �[?7l … �[?7h (no user-space truncation expected),
  • the non-TTY case is unchanged in spirit,
  • the wide-character/emoji case is removed since width isn't computed in JS anymore.

All 13 spinner tests pass.

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this — the wrapping bug is real and the DECAWM approach is clean and self-contained. I reviewed the code and traced the escape sequencing. The fix is sound for the common case, but I have one important process note and a couple of robustness points.

⚠️ PR description doesn't match the implementation

The body describes a user-space truncation approach (measure unicodeWidth, cut the message to columns, "asserts each frame ends at 18 x's", "10 dinos in 10 columns yields 4"). The actual diff implements DECAWM auto-wrap toggling (\x1b[?7l / \x1b[?7h) and the tests assert the escape brackets, not truncated content. Since this picks the implementation strategy for an unstable public API, please rewrite the description (and the Tests section) to describe what's actually being merged, and note why DECAWM was chosen over the truncation the thread had converged on — the in-code comment's rationale (avoid guessing display widths for emoji/ZWJ/combining marks) is good and belongs in the PR.

Correctness — looks right

  • Each frame is a single atomic writeSync of LINE_CLEAR + DECAWM_OFF + frame + DECAWM_ON, so DECAWM is only off for the duration of one syscall and is restored every frame. With auto-wrap off the over-long line never wraps, so the next frame's \r\x1b[K clears the same single row — exactly the fix for #6975. ✅
  • Non-TTY path is byte-for-byte unchanged ([LINE_CLEAR, frame]), so pipes/redirects stay clean and existing tests (which run with isTerminal() === false) are unaffected. ✅

Test coverage

The new tests assert that the escape brackets are emitted, not that overflow is actually truncated (the stubbed writeSync can't truncate). Acceptable for a unit test, but it means the user-visible behavior (#6975 no longer scrolls) isn't actually exercised — and the description shouldn't claim width-based assertions that aren't there.

Net: approve once the description is corrected, plus please consider the inline points below.

Comment thread cli/unstable_spinner.ts
// Letting the terminal truncate avoids guessing display widths for emoji,
// ZWJ sequences, combining marks, etc.
const DECAWM_OFF = encoder.encode("\u001b[?7l");
const DECAWM_ON = encoder.encode("\u001b[?7h");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Portability caveat worth a line in the doc/PR: \x1b[?7l/\x1b[?7h (DECAWM) is a DEC private mode. Well-supported in xterm/modern terminals/Windows Terminal, but on a TTY that doesn't implement it (some CI pseudo-terminals, dumb-ish terminals) isTerminal() is still true, the toggles are ignored, and the original wrapping bug remains — silent degradation. The truncation approach the body describes would be universal. Not necessarily blocking, but the trade-off (no width-guessing vs. relies on terminal support) should be called out so maintainers can decide consciously.

Comment thread cli/unstable_spinner.ts
// (#6975). On non-TTY streams the literal escape bytes would just
// clutter the output, so skip them.
const parts: Uint8Array[] = isTty
? [LINE_CLEAR, DECAWM_OFF, frame, DECAWM_ON]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OFF/frame/ON brackets are atomic per frame, so in the normal path DECAWM is always restored — good, and the only window where auto-wrap stays off is a kill mid-writeSync, so the leak risk is minimal.

One coupling to note: stop() writes only LINE_CLEAR and relies on the last frame having ended with DECAWM_ON. For belt-and-suspenders, consider emitting DECAWM_ON from stop() too (TTY-only), so a terminal is guaranteed to be left in wrap-on even if a future refactor makes this OFF/ON non-atomic. Optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spinner cannot overwrite the same line when the message is longer than terminal width

3 participants