Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe filtered workspace runner centralizes successful completion timing output. Silent successful scripts now render as one compact line. Tests cover compact success, expanded success with output, and expanded failure cases. ChangesFiltered run output
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized renderer change collapses only silent successful scripts while preserving output and failure rendering; no actionable merge-blocking risk remains, so it is merge-ready after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the primary collapse objective in issue Full details: Description checkExplanation The description explains the problem, fix, scope, test coverage, verification results, and known limitations. It does not use the exact template headings, but it provides the required information and is mostly complete. Comment |
There was a problem hiding this comment.
LGTM — small, focused renderer tweak with the right test coverage.
What was reviewed:
write_doneis a pure extraction, byte-identical to the removed inline block; both call sites wired.- Collapsed path gates on
buffer.is_empty()+Status::Exited+code == 0andcontinues — theis_abortbranch below is unaffected (nothing to expand when the buffer is empty anyway). - Tests anchor the whole final frame (
^…$) for all three variants (silent-success, noisy-success, silent-fail), assert frame before exit code, and reuse the file's existingspawnSync+FORCE_COLOR/skipIf(isWindows)pattern.
Extended reasoning...
Overview
This PR touches two files: src/runtime/cli/filter_run.rs (the interactive terminal renderer for bun run --filter) and test/cli/run/filter-workspace.test.ts. The Rust change extracts the existing "Done in N ms/s" formatting into a write_done associated function and adds an early continue in the per-handle redraw loop that renders a compact one-line form (pkg script · Done in N ms) when a script exited with code 0 and produced no output. The expanded box form (header + │ body + └─ footer) is unchanged for scripts with output or nonzero exit. The test change replaces a single loose toMatch(/Done in .../) assertion with a runTerminalRenderTest helper plus lastTerminalFrame (split on the synchronized-update end marker, strip ANSI), and asserts the exact final frame for three cases.
Security risks
None. This is display-only formatting in the pretty terminal renderer — no input parsing, no filesystem/network operations, no unsafe blocks, no user-controlled data reaching a trust boundary. The only inputs are the already-captured package/script names and the process's own output buffer, both handled identically to the surrounding code.
Level of scrutiny
Low. The write_done extraction is byte-for-byte identical to the removed inline block (same fmt! strings, same ms/s threshold, same end_time.is_none() fallback). The new collapsed branch is a narrow additive guard that only fires on buffer.is_empty() && Status::Exited && code == 0, uses the same fmt!/bstr::BStr idioms and ? propagation as the neighboring header write, and continues cleanly. I checked that skipping the is_abort-driven elide_lines = None path is harmless here since an empty buffer has nothing to elide. No CODEOWNERS cover these paths.
Other factors
Test quality is good per REVIEW.md: the new tests anchor the entire final frame with ^…$ (strongest invariant, not toContain), cover the three-way variant matrix (collapsed vs. two expanded forms), assert output before exit code, use tempDir/bunExe/bunEnv, and follow the file's existing spawnSync + FORCE_COLOR: "1" + test.skipIf(isWindows) convention already used by runElideLinesTest in the same file. The ANSI-stripping regex /\x1b\[[0-9;?]*[a-zA-Z]/g correctly handles both color/cursor CSI sequences and the ?2026h start marker. The PR description states the collapse test fails on stock bun and the full file passes on the debug build. Bug hunt exited on dry_streak with no findings and no ruled-out candidates.
|
Closing in favor of #41325. That PR rewrites the same renderer and puts the status on the header line. A script that exits 0 with no output then takes one line, for example |
Problem
bun run --filteroutput more concise for scripts with no output #41196. Withbun run --filter, every successful script takes two lines in the terminal renderer, even when it prints nothing:dep0 script $ exit 0plus└─ Done in 1 ms. In a monorepo with many packages, an incremental build fills the screen with these pairs.State::redraw()insrc/runtime/cli/filter_run.rs) always writes the header and footer, with no case for a script that produced no output.Fix
redraw()now renders one line:dep0 script · Done in 1 ms. Scripts with any output, and scripts that fail, keep the expanded header, body, and footer.Done in N mstiming block is extracted into awrite_donehelper, used by both the collapsed and the expanded form.test/cli/run/filter-workspace.test.ts(one updated test plus two new ones, the collapse test fails on stock bun). The whole file passes with the debug build (88 pass).Background
--hide-silentflag that hides silent successes entirely. This PR ships only the default collapse. A hide option changes what the user sees per a mode choice, so it needs maintainer sign-off and a design shaped like the ecosystem (an output-mode value keyed on success, like turbo's--output-logs=errors-only), paired with a bunfig key per the--elide-linesprecedent (add --elide-lines override flag for workspace filtering #15837).exit 0script. Whichever PR lands second needs a small test rebase. I left a note there.Notes
handle.buffer.is_empty()plusStatus::Exitedwith code 0. In the pretty path the buffer is append-only during the run, so a script that printed anything can never collapse.\x1b[?2026h/\x1b[?2026l), so the test splits on the end marker, takes the last frame, and strips ANSI escapes.bun bd test test/cli/run/filter-workspace.test.ts(88 pass, 2 skip).cargo check -p bun_runtimeclean.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/filter-workspace.test.ts