Cap bun --filter frame to terminal height - #28802
Conversation
|
Updated 6:12 AM PT - Jul 12th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 28802That installs a local version of the PR into your bun-28802 --bun |
WalkthroughDetects terminal height and changes elision semantics; redraw now caps per-handle rendered lines to terminal rows, reserves space for an elision indicator, and clamps cursor-up clearing. Adds a regression test validating redraw and frame sizes stay within terminal height limits. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/filter_run.zig`:
- Around line 304-322: The clamp that limits the cursor-up run is being disabled
during abort because terminal_rows is set to null when is_abort is true; restore
terminal row information for clamping while still disabling content elision in
abort. Concretely, don't make terminal_rows null for abort redraws—always call
getTerminalRows() (or compute a separate rows_for_clamp) and use that value for
computing `up` / clamping with `this.last_lines_written`, but keep the elision
logic (e.g., `terminal_cap` or per-handle overhead calculation) conditional on
`is_abort` so abort only disables elision not the clear-loop clamp; reference
`terminal_rows`, `terminal_cap`, `this.last_lines_written`, and `up` when making
this change.
In `@test/regression/issue/28800.test.ts`:
- Around line 11-12: The test currently skips Windows via
describe.skipIf(isWindows) so the new Windows-specific
GetConsoleScreenBufferInfo path in src/cli/filter_run.zig never runs in CI;
update the test in test/regression/issue/28800.test.ts to cover the Windows
branch by either removing the skip so it runs on all platforms or adding an
additional Windows-only test (e.g., using describe.onlyOnWindows or a
conditional test guarded by isWindows) that exercises the same "--filter frame
stays within terminal rows when output is tall" scenario; ensure the
new/modified test invokes the same behavior that triggers the
GetConsoleScreenBufferInfo code path so the Windows logic is validated in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3c466f8-7fb1-4d66-8fcf-4cabd8f32a43
📒 Files selected for processing (2)
src/cli/filter_run.zigtest/regression/issue/28800.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — all substantive review feedback has been addressed in commit 23e3d8e.
Extended reasoning...
Overview
The PR modifies src/cli/filter_run.zig to fix duplicated output when bun --filter runs in a terminal shorter than the combined frame height. It adds getTerminalRows(), rewrites elide() semantics (null=show-all, 0=elide-all), and introduces per-handle content capping with a cursor-up clamp. A regression test covers the 2-package and 4-package scenarios.
Security risks
None. This touches only terminal rendering/escape-sequence logic with no auth, crypto, or user-data exposure.
Level of scrutiny
Medium: non-trivial rendering logic, but well-contained within the filter CLI and thoroughly tested. The change is mechanical (height query → cap computation → clamp), with clear correctness arguments.
Other factors
The three substantive issues I raised in prior review rounds have all been resolved:
- The abort-path cursor-up clamp was broken (terminal_rows forced to null on abort) — fixed by always calling getTerminalRows() and driving content elision separately via show_indicator/terminal_cap.
- The terminal_cap=0 case with many packages still emitted 3 lines per handle — fixed by computing show_indicator first, setting per_handle_overhead=2 when the indicator is suppressed, and adding a 4-package test case.
- The elide() bare-newline edge case — fixed with the post-trim empty guard.
The remaining nit (Windows i16 arithmetic on srWindow.Bottom - srWindow.Top + 1) is theoretical — it requires a 32767-row console — and does not affect real-world usage.
|
The failing CI job ( |
|
Updated: all 5 failing jobs are pre-existing flaky tests unrelated to this PR.
Neither test touches |
23e3d8e to
29ef67e
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and I found nothing new, but the core redraw() rework (~170 lines of row-budget arithmetic, new RedrawMode with an unconditional final-frame redraw in finalize(), and changed elide(Some(0)) semantics) plus the incidental rustfmt reflow across 10 unrelated files is substantial enough that a human should give it a once-over.
Extended reasoning...
Overview
The substantive change is confined to src/runtime/cli/filter_run.rs (~170 net lines) and a new test/regression/issue/28800.test.ts (170 lines, 3 PTY-driven tests). redraw() now queries terminal height via ioctl(TIOCGWINSZ) / GetConsoleScreenBufferInfo, budgets per-handle content against rows - 1, gates the [N lines elided] indicator on usable_rows >= 4*n, clamps the cursor-up clear loop at terminal_rows, and replaces the is_abort: bool parameter with a three-state RedrawMode (Live/Abort/Final). finalize() now unconditionally renders one more frame (previously only on abort), and elide() semantics changed so Some(0) means "elide everything" rather than "show everything". The other 10 files (crash_handler, errno, tracy, Arguments, run_command, upgrade_command, jsc_hooks, ChromeProcess, spawn/process, spawn_sys) are pure rustfmt whitespace reflows of long #[cfg(...)] / #[cfg_attr(...)] attributes — zero logic change.
Security risks
None identified. The new syscalls are read-only terminal-size queries on stdout; no new input parsing, file access, or privilege boundaries. The rustfmt-only hunks in spawn/crash-handler/webview don't alter behavior.
Level of scrutiny
Moderate. This is cosmetic CLI rendering (worst-case regression is garbled bun --filter output, not data loss or crashes), and the PR is a strict improvement over the pre-existing unbounded behavior. However, the row-budget arithmetic has already required several correctness iterations in this review thread (abort-path clamp, n*3 overflow, rows-1 off-by-one, 4*n monotonicity threshold, post-trim empty guard, i16 widening), the Windows branch is untested in CI (describe.skipIf(isWindows)), and finalize() now fires an extra redraw on every clean exit — a user-visible behavior change worth a maintainer's eye. The elide(Some(0)) semantic flip is internal but worth confirming no other caller relied on the old meaning.
Other factors
Every inline comment from prior review rounds (mine and CodeRabbit's) is marked resolved, and the diff reflects each fix. The regression test now awaits PTY EOF before closing, anchors content assertions to the │ prefix, asserts <= rows - 1 for live frames, and orders content checks before exitCode. Two acknowledged follow-ups remain out of scope: the rows < 2*n header+footer floor and the ws_col soft-wrap blind spot — both pre-existing and noted as strict-improvement deferrals. The 10-file rustfmt churn is harmless but inflates the diff; a human may want to confirm that's intentional rather than accidental cargo fmt spillover.
7f0a6cc to
dedc4a4
Compare
There was a problem hiding this comment.
All earlier review comments have been addressed and no new issues turned up on this pass. I'm not auto-approving only because the change is non-trivial — it flips elide(Some(0)) semantics, adds the RedrawMode axis, and makes finalize() redraw unconditionally on clean exit — so a quick human sanity check on the final shape seems worthwhile.
Extended reasoning...
Overview
The PR modifies src/runtime/cli/filter_run.rs (~200 lines net) and adds test/regression/issue/28800.test.ts (170 lines). It fixes #28800 by querying terminal height on each redraw(), capping per-handle content so the live frame fits in rows - 1, suppressing the elision indicator when there isn't room for it, clamping the cursor-up clear loop at terminal_rows, and introducing a RedrawMode {Live, Abort, Final} enum so that finalize() now does an uncapped final redraw on clean exit (previously only on abort). elide()'s contract changes: Some(0) now means "elide everything" (was "show everything"), with the --elide-lines=0 → None translation moved to the caller; a post-trim empty guard was also added.
Security risks
None. The change is confined to terminal output formatting (ANSI escapes, line counting) plus a read-only ioctl(TIOCGWINSZ) / GetConsoleScreenBufferInfo query. No user input is parsed, no filesystem/network/auth surface is touched, and the new unsafe blocks are standard FFI calls on stack-allocated POD out-params.
Level of scrutiny
Moderate. This is CLI presentation code, not a correctness-critical path — failure modes are cosmetic (duplicated/truncated output) rather than data-affecting. But the patch carries real logic: a row-budget computation with several interacting thresholds (usable_rows = rows-1, show_indicator at rows >= 4n, per_handle_overhead 2 vs 3), a semantic flip in elide(), and a behavior change in finalize() (now always emits one more frame on clean exit). It went through ~12 rounds of inline feedback over six weeks, every one of which was addressed — including the off-by-one rows-1 budget, the abort-path clamp, the show_indicator non-monotonicity, the PTY-EOF race in the test, the .concurrent convention, and the safe zeroed() for CSBI. That iteration history gives me reasonable confidence the arithmetic is right, but the cumulative shape is no longer "simple/mechanical," so it doesn't meet the bar for bot-only approval.
Other factors
- The latest commit (5be2c1f) addresses the last two nits I raised (
.concurrentsuite, safezeroed()forCONSOLE_SCREEN_BUFFER_INFO); all 14 prior inline comments are resolved. - Regression test exercises 2 packages × 8 lines, 4 packages (overhead-exceeds-rows path), and the
--elide-lines=0final-frame contract in a 10-row PTY; existingfilter-workspace.test.ts(54 tests) reportedly still passes. - Known residual gaps explicitly deferred as follow-ups (not regressions): the
rows < 2*nheader+footer floor and soft-wrap width handling. Windows PTY coverage is still skipped, which is consistent with the rest of the terminal test suite. - robobun reports the only CI failures are pre-existing flakes unrelated to this file.
4ff3564 to
233aabf
Compare
233aabf to
fd31d81
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and this round found no issues — deferring only because the redraw budget arithmetic, the elide() semantic change, and the new always-redraw-on-finalize() behavior are substantive enough that a human should give it a quick look.
Extended reasoning...
Overview
This PR fixes #28800 by making bun --filter's in-place redraw aware of the terminal height. It touches one production file (src/runtime/cli/filter_run.rs, ~200 net lines) and adds a 170-line PTY-based regression test. The core changes are: a new get_terminal_rows() helper (POSIX ioctl(TIOCGWINSZ) / Windows GetConsoleScreenBufferInfo), a per-handle row-budget computation with a show_indicator tier, a clamp on the cursor-up clear loop, a new RedrawMode {Live, Abort, Final} enum replacing the old is_abort: bool, a semantic change to elide() so Some(0) now means "elide everything" (vs. the old "show everything"), and finalize() now unconditionally renders one last frame (previously only on abort).
Security risks
None. This is purely cosmetic terminal-rendering logic for the --filter workspace runner. No auth, no network, no untrusted input parsing beyond reading the terminal size from the OS. The two unsafe blocks added are well-scoped FFI calls (ioctl, GetConsoleScreenBufferInfo) on owned stack out-params with SAFETY comments, matching existing patterns elsewhere in the codebase (Progress.rs, update_interactive_command.rs).
Level of scrutiny
Medium. The worst-case failure mode is garbled terminal output — not data loss, crashes, or security exposure — so this isn't critical-path. But the change is not mechanical: the row-budget arithmetic is subtle (it took seven rounds of bot review to converge on rows-1 budgeting, the >= 4*n indicator threshold, the post-trim empty guard in elide(), etc.), the elide(Some(0)) contract inversion is a real semantic change, and finalize() now always emits an uncapped final frame on clean exit, which is a user-visible behavior change beyond the original bug fix. These are reasonable design choices and well-commented in the code, but they're the kind of thing a maintainer should sign off on rather than a bot.
Other factors
Every prior inline comment from me and CodeRabbit is marked resolved, and the most recent commit (fd31d81) addresses my last two nits (.concurrent on the describe block, safe zeroed() for CONSOLE_SCREEN_BUFFER_INFO). The current bug-hunting pass found nothing. CI failures noted earlier in the thread were unrelated flakes per robobun. No CODEOWNERS entry covers these paths. The remaining known limitations (the 2*n header+footer floor and the column-width soft-wrap blind spot) were explicitly flagged as pre-existing / acceptable follow-ups in resolved threads. Given all that, this is close to approvable — I'm deferring on the "when in doubt" rule because of the accumulated complexity and the behavioral change to the clean-exit path.
The redraw loop emits one `\x1b[1A\x1b[K` per line in `last_lines_written` to clear the previous frame. Cursor-up escapes clamp at the top of the viewport, so once the frame grew taller than the terminal window, each redraw could only clear the visible slice and left stale content on screen — producing the duplicated-header output reported in #28800. Query the terminal height at each redraw, cap per-handle content to fit, and clamp the upward cursor movement at the terminal row count so the next frame always fits. When the terminal is too short even for header + elision-indicator + footer per handle, drop the indicator so the frame still fits. Fixes #28800
darwin-14-aarch64-test-bun expired on the runner without producing any log output; all 50+ other platforms passed. Re-rolling once.
Every emitted frame line ends in '\n'. With the cursor at row 1, N newlines land the cursor at row N+1; writing exactly 'rows' lines scrolls the top line off into scrollback. Over many redraws those top lines (one per frame) accumulate as duplicated `pkg $ script` headers — the same visible duplication #28800 originally reported, just one line at a time. Budget the content cap (and the show_indicator threshold) against 'rows - 1' instead of 'rows' so the frame can always fit without scrolling. The regression test's frame-size assertion is tightened accordingly: before this commit the 2-pkg×10-row case rendered a 10-line frame in a 10-row terminal and the old 'maxFrameLines <= rows' assertion passed while the top line still scrolled; after, the assertion is 'maxFrameLines <= rows - 1' and the test fails without the fix.
Two small UX fixes flagged on review: 1. `--elide-lines=0` is documented as "show all lines" but the terminal cap was still applied on the final frame, so a successful run in a short TTY silently truncated. There is no next redraw to corrupt, so the final frame doesn't need the cap. Introduce a RedrawMode enum (Live / Abort / Final); `finalize()` now always renders once — Abort on Ctrl+C (dump all for debugging, existing behavior), Final on clean exit (honor `--elide-lines` including 0, skip terminal_cap). 2. The `show_indicator = rows > 3*n` threshold was non-monotonic: resizing the terminal from 3n to 3n+1 rows flipped the indicator on, pushed overhead to 3n, and dropped the content budget to 0, so each handle showed the elision marker and no actual output until rows reached 4n. Tighten to `rows >= 4*n` so the indicator only appears when there is room for at least one content line per handle alongside it — content visibility is now monotonic in terminal height. Regression test: - `assertFramesFit` now only checks LIVE frames for the per-frame size budget; the final frame is intentionally uncapped. - New test: `--elide-lines=0` in a 10-row PTY — verifies the final frame contains every line from every package with no "lines elided" marker. Fails against the unfixed code (last frame still shows `[11 lines elided]` for 12-line packages).
Two review nits:
- The per-line `toContain(\`${prefix}-${i}\`)` loop was effectively
a no-op because the script header echoes the literal printf
argument (which already contains every `a-1`..`a-12` substring),
so all 24 assertions passed from the header alone regardless of
whether content rendered. Strip ANSI escapes and match the content
line format (`│ a-N\r\n`) so the assertion actually exercises
content rendering. Gate-checked: reverting the Rust change causes
the new assertion to fail with 'Expected to contain: "│ a-1\r\n"'
(received shows `[11 lines elided]` and only `a-12` rendered).
- Move `expect(exitCode).toBe(0)` to after the content assertions
per CLAUDE.md — consistent with the other tests in this file and
gives more useful failure diagnostics.
`proc.exited` and the terminal `data` callback are driven by independent poll events with no ordering guarantee between them — on the POSIX path too. If `terminal.close()` ran before the kernel-buffered Final frame was dispatched, the --elide-lines=0 assertion on `frames[frames.length - 1]` would intermittently see a stale Live frame. Add an `exit:` callback (fires on PTY EOF, after all buffered data is delivered) and await it before closing, matching the pattern used by test/js/bun/terminal/terminal-spawn.test.ts.
…or CSBI - Per test/CLAUDE.md "prefer concurrent tests over sequential tests when multiple tests in the same file spawn processes or write files", switch to `describe.skipIf(isWindows).concurrent(...)`. The three cases each create their own tempDir + PTY and share no state. - `CONSOLE_SCREEN_BUFFER_INFO` has a Zeroable impl in bun_core::windows_sys, so the safe `bun_core::ffi::zeroed()` works here (matches the unix branch right above, Progress.rs, and update_interactive_command.rs). Drops the `unsafe` block + SAFETY comment.
fd31d81 to
f13a3b4
Compare
There was a problem hiding this comment.
No new issues found — the latest commits address the remaining nits (concurrent describe, safe zeroed() for CSBI, PTY-EOF await, tightened assertions). This has been through ~10 review rounds and looks correct to me now, but given the accumulated design surface (the new RedrawMode split, elide(Some(0)) semantics flip, unconditional final-frame redraw in finalize(), and the untested-on-Windows GetConsoleScreenBufferInfo path), a human sign-off on the UX/design choices is worth having.
What was reviewed:
redraw()budget arithmetic:usable_rows = rows-1,show_indicatorat>= 4*n, per-handle overhead 2 vs 3 — verified frames stay withinrows-1and content visibility is monotonic in terminal height.RedrawMode::{Live,Abort,Final}— cursor-up clamp applies in all three; terminal cap only in Live;--elide-lines=0honored on the final frame.elide()post-trim empty guard andSome(0)branch; Windowsi32widening forsrWindowarithmetic.- Regression test: PTY-EOF ordering,
│ prefixanchored assertions,rows-1frame bound, exit-code-last ordering.
Extended reasoning...
Overview
The PR fixes #28800 by making bun --filter's in-place redraw aware of terminal height. It touches src/runtime/cli/filter_run.rs (adds get_terminal_rows() with unix/windows branches, a RedrawMode enum, reworks redraw() to compute a per-handle content cap and clamp cursor-up runs, changes elide() semantics so Some(0) means "elide everything", and makes finalize() always emit one more frame) plus a new 3-test regression suite in test/regression/issue/28800.test.ts that spawns a 10-row PTY and asserts frame/cursor-up bounds.
Security risks
None. This is CLI terminal-rendering logic — no untrusted input parsing, no auth/crypto/permissions, no network. The two new FFI calls (ioctl(TIOCGWINSZ), GetConsoleScreenBufferInfo) are read-only queries on stdout with zeroed out-params and checked return values.
Level of scrutiny
Moderate. Not a simple mechanical fix: it introduces a three-state mode enum, changes the documented meaning of an internal helper (elide(Some(0)) was previously a no-op, now elides everything — the sole caller was updated to translate CLI 0 → None first), adds platform-gated FFI, and changes finalize() from "redraw only on abort" to "always redraw one final frame". These are all reasonable and well-commented, but they are design choices a maintainer should ratify — particularly the --elide-lines clamp-during-live / uncap-on-final tradeoff, and whether the acknowledged residual gaps (2*n header+footer floor when packages outnumber rows/2; soft-wrap width blindness) are acceptable as follow-ups.
Other factors
- This PR has been through ~10 iterations since April; every prior inline finding (abort-path clamp,
n*3overhead overflow, off-by-onerowsbudget, non-monotonic3n<rows<4nvalley,elide("\n", 0)miscount, i16 overflow, PTY-EOF race, vacuoustoContainloop, assertion ordering,describe.concurrent,zeroed_unchecked) is resolved in the current diff, and the bug-hunting pass on the head commit found nothing new. - The Windows branch is compile-checked (
rust:check-allacross 10 targets per the description) but not exercised by the regression test (PTY spawn is POSIX-only). Issue #28800 was filed on Windows. - The rebase onto main was signature-only (
crate::Result<()>); 3 new + 56 existingfilter-workspacetests pass per the rebase note. - Two limitations were explicitly deferred as follow-ups in resolved threads: the
2*nfloor (many packages in a tiny terminal) and column-width soft-wrap. Both are pre-existing / strict improvements over baseline, not regressions.
|
Summary of the design decisions for whoever does the final sign-off:
All 17 review threads resolved. Verification: 3 PTY regression tests (fail on main, pass here), 56 existing filter-workspace tests, |
|
Status check against current main (f426a8e), since #28800 was closed in the meantime. #28800 was closed by the reporter on 2026-07-26 without a comment, and nothing referencing it has landed: there is no commit mentioning 28800 on main, and I copied this PR's So this is not covered by main; leaving it open. The |
1 participant
Fixes #28800.
What does this PR do?
bun --filterredraws its multi-package build log in place. When thecombined frame height exceeds the terminal window, the cursor-up
escapes used to clear the previous frame get clamped at the top of
the viewport — stale lines scroll off and stay on screen, producing
the duplicated output from the issue:
Cause.
src/runtime/cli/filter_run.rsredraw()emits\x1b[1A\x1b[Konce per line inlast_lines_writtenwithout anyawareness of the current terminal height. Once
last_lines_written > terminal_rows, each redraw can only clear the visible slice, andthe new frame is appended below the uncleared stale content.
Fix. Query the terminal height on every redraw:
normally reserves 3 rows (header + elision indicator + footer);
the remainder is split evenly across handles.
elide()hides theoldest lines behind the
[N lines elided]marker.rows <= 3 * n. Otherwise eachhandle still emits 3 lines with
cap=0and the frame overflows;in that case per-handle overhead drops to
2(header + footer)and
show_indicatorsuppresses the marker.terminal_rows, so a frame thatpredates a resize-smaller still clears only what's actually on
screen.
is disabled so we dump everything for debugging.
elide("\n", 0)now reportselided_count=0(post-trim empty guard).How did you verify your code works?
Regression test spawns
bun --filter *against 2 / 4 workspace packagesin a 10-row PTY and asserts (a) no contiguous cursor-up run exceeds
rowsand (b) no rendered frame exceedsrows. Both tests fail on main(20 / 40 cursor-ups) and pass with the fix. All 54 existing
filter-workspace.test.tstests still pass, including--elide-lines=0 shows all outputand--elide-lines is a no-op (not an error) when stdout is not a terminal.bun run rust:check-allpasses across all 10 targets.Rebase note
Rebased on current main and ported from
src/cli/filter_run.zigtosrc/runtime/cli/filter_run.rs(the live code path since the Rustrewrite). The
.zigfile insrc/runtime/cli/is now a read-onlyporting reference per
CLAUDE.md; only the.rsis compiled.no test proof · iteration 10 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/regression/issue/28800.test.ts