console: apply group indent to count/time/assert and no-args log paths - #36470
console: apply group indent to count/time/assert and no-args log paths#36470robobun wants to merge 5 commits into
Conversation
console.count, console.timeLog, and console.timeEnd wrote their output at column 0 regardless of the current console.group nesting level. The log/info/warn/error/dir/assert/trace paths already prefix each line with ConsoleObject.default_indent * 2 spaces via Formatter.write_indent; the count and time* host-call paths write directly and skipped it. Expose formatter::write_indent_n as pub(super) and call it with the current default_indent before each of these writes, matching Node.
WalkthroughChangesConsole indentation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Same bug class as count/timeLog/timeEnd: the Assert-with-no-message early return and the print_length==0 fallthroughs wrote directly to the writer without the default_indent prefix.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the sibling-site feedback from the earlier pass is now addressed in c68b37c.
What was reviewed:
- Verified all six touched output paths (count, timeLog, timeEnd, bare assert, bare log→
\n, other→undefined\n) readdefault_indentbefore borrowing the writer, and thatwrite_indent_n(0, ...)is a no-op so level-0 output is unchanged. - Checked
Output::error_writer()in the timeLog/timeEnd path is the same streamprint_elapsedwrites to, so the indent and[…ms]land in order. - Test exercises two nesting levels plus level-0 for each path and asserts exact stdout / regex-matched stderr.
Extended reasoning...
Overview
Cosmetic Node-compat fix in src/jsc/ConsoleObject.rs: six console output paths that previously wrote directly to a writer now prepend formatter::write_indent_n(default_indent, …) so console.group() indentation applies. Paths covered: Bun__ConsoleObject__count, Bun__ConsoleObject__timeLog, Bun__ConsoleObject__timeEnd, the MessageType::Assert && len == 0 early return, and both print_length == 0 fallthroughs (\n and undefined\n). write_indent_n visibility widened from private to pub(super) so the extern-C host calls can reach it. One new test added to test/js/web/console/console-log.test.ts.
Security risks
None. Output-formatting only; no new inputs parsed, no allocation sizing, no unsafe blocks beyond the pre-existing vm_console_mut pattern already used at every neighboring call site with the same SAFETY justification.
Level of scrutiny
Low. The change is mechanical — the same one-line write_indent_n call inserted at each site — and write_indent_n(0, …) short-circuits, so behavior at group depth 0 is byte-identical to before. Borrow ordering was checked at each site: default_indent (a u16 copy) is read before writer()/error_writer() takes the mutable borrow, matching the existing snapshot-then-borrow pattern at line 448. For timeEnd/timeLog, the indent goes to Output::error_writer(), which is the same stream print_elapsed/print_errorln write to, so ordering is preserved.
Other factors
The previous review round flagged the bare-assert and print_length == 0 sibling paths; c68b37c covers all three and extends the test to assert them at two nesting depths plus level 0 (both stdout and stderr). The stderr assertions use a regex over the timing value rather than a fixed string, so they won't flake on duration. The _global → global_this rename in timeEnd is required to reach vm_console_mut. The PR description explicitly scopes out the pre-existing stderr/bracket-format divergence for time* and the separately-tracked console.table case (#34236).
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ConsoleObject.rs`:
- Around line 550-553: Update the empty-message logging arm in ConsoleObject to
reuse the already level-selected writer variable for indentation, newline, and
flush operations. Remove the unsafe re-derivation via writer() so no-argument
console.warn() and console.error() output remains routed to the selected error
writer.
In `@test/js/web/console/console-log.test.ts`:
- Around line 146-195: Expand the src scenario in the console.group test to
invoke no-argument console.info(), console.warn(), and console.error() at
appropriate group nesting levels. Update stdout and stderr assertions to verify
each method’s expected indentation and stream, while preserving the existing
count, assert, and timing checks in the test.
🪄 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: 222e02b8-c160-4c79-8ebb-eb91ab39039e
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/web/console/console-log.test.ts
The print_length==0 Log arm re-derived (*console).writer() (always stdout) instead of using the level-selected writer, so bare console.warn()/console.error() wrote their newline to stdout. Use the already-selected writer so the indented blank line lands on stderr, matching Node. Test now covers info/warn/error with no args.
There was a problem hiding this comment.
LGTM — the sibling sites I flagged earlier and CodeRabbit's writer/coverage points are all addressed.
What was reviewed:
- Each of the six direct-write paths (
count,timeLog,timeEnd, bareassert, and bothprint_length == 0arms) now prependswrite_indent_n(default_indent, …); the empty-Logarm reuses the level-selectedwriterso barewarn()/error()land on stderr. - Checked that
Output::error_writer()is the same streamprint_elapsedwrites to, so the indent lands on the same line as[…ms] label. - Removed SAFETY comment was only about the now-deleted re-derive;
raw_writeris already a raw-deref borrow so no aliasing concern remains. - Test covers two nesting levels + level 0, both streams, and the timing regex accommodates
ms/sunderNO_COLOR=1frombunEnv.
Extended reasoning...
Overview
Cosmetic Node-compat fix in src/jsc/ConsoleObject.rs: six console output paths that wrote directly to a writer now prepend the current default_indent (group nesting) via formatter::write_indent_n, which is widened to pub(super). The print_length == 0 && Log arm additionally switches from re-deriving (*console).writer() (always stdout) to the already level-selected writer, so no-arg console.warn()/console.error() route to stderr. A new subprocess test in test/js/web/console/console-log.test.ts asserts indentation and stream at levels 0/1/2 for count, bare assert(false), bare log/info/warn/error, and timeLog/timeEnd.
Security risks
None. Pure output-formatting change; no parsing of untrusted input, no allocation sizing from external data, no new unsafe patterns beyond the existing vm_console_mut(global) idiom already used at neighboring sites with the same SAFETY justification.
Level of scrutiny
Low. This is a narrow cosmetic Node-compat divergence (WHATWG console group indentation) with no control-flow or lifetime changes beyond dropping one raw-pointer re-deref that is no longer needed. The removed SAFETY comment described why the re-derive was safe; with the re-derive gone, writer (derived once via raw *console) is simply reused — no new aliasing. Output::error_writer() is the same stderr stream that print_elapsed/print_errorln write to, so the indent prefix and the timing text land on one line without an intervening flush.
Other factors
All three prior review threads are resolved: my earlier comment on the Assert && len == 0 and print_length == 0 sibling sites was fixed in c68b37c; CodeRabbit's writer-routing and variant-coverage comments were fixed in 0ecfeca. The test follows harness conventions (bunEnv/bunExe, await using, concurrent drain of stdout/stderr/exited, exitCode asserted last, it.concurrent). The timing regex [mnµ]?s is loose enough for both print_elapsed branches (ms/s), and bunEnv sets NO_COLOR=1 so ANSI codes won't break it. PR description explicitly scopes out the pre-existing time*-on-stderr divergence and console.table (#34236).
|
Triage note: #37128 (the console output rework) does not indent count / timeLog / timeEnd / bare assert output inside console.group. On its current head This will need a rebase once #37128 lands: count and time* are emitted through a new path there, so the |
2 participants
Repro
Node v26:
stderr:
Assertion failed,Bun (before):
cnt: 1,Assertion failed,[0.00ms] tall at column 0; bareconsole.warn()/console.error()wrote their newline to stdout instead of stderr.Cause
Several console output paths in
src/jsc/ConsoleObject.rswrite directly to a writer, bypassing thedefault_indentprefix and the level-based stdout/stderr selection that the log-family path applies viaFormatter::write_indent:Bun__ConsoleObject__countBun__ConsoleObject__timeLog/Bun__ConsoleObject__timeEndMessageType::Assert && len == 0early return (Assertion failed\n)print_length == 0fallthroughs: bareconsole.log()/info()/warn()/error()wrote\nand also re-derived(*console).writer()(always stdout) instead of reusing the level-selected writer, so no-argswarn()/error()landed on stdoutThe
time*output landing on stderr in bracket format is a separate pre-existing divergence and not changed here;console.tableindentation is tracked separately in #34236.Fix
Expose
formatter::write_indent_naspub(super)and call it with the currentConsoleObject.default_indentbefore each of these writes. Theprint_length == 0 && Logarm now reuses the already level-selectedwriterso barewarn()/error()route to stderr.Verification
New test in
test/js/web/console/console-log.test.tsexercises nested groups withcount,assert(false), barelog/info/warn/error, and thetimefamily at two indent levels plus back at level 0, asserting both stream and indentation.USE_SYSTEM_BUN=1 bun test test/js/web/console/console-log.test.ts -t 'console.group indents count'failsbun bd test test/js/web/console/console-log.test.tspasses (5 tests)bun bd test test/js/web/console/console-timeLog.test.tspasses (3 tests)bun bd test test/js/node/console/console.test.tspasses (7 tests)Fixes #30017
[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file