ai slop#30018
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTimer console output behavior changed: Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Review rate limit: 0/5 reviews remaining, refill in 53 minutes and 46 seconds. Comment |
|
Updated 7:22 PM PT - May 4th, 2026
❌ @robobun, your commit ac4bf0a has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30018That installs a local version of the PR into your bun-30018 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, focused fix that threads console.default_indent into the timer output paths, with a regression test.
Extended reasoning...
Overview
This PR makes console.timeEnd and console.timeLog honor console.group() indentation. It adds a small printTimerIndent helper in src/bun.js/ConsoleObject.zig that writes 2 * default_indent spaces to stderr via Output.printError before the elapsed-time prefix, changes timeEnd to capture global (instead of discarding it) so it can read global.bunVM().console.default_indent, and sets .indent = console.default_indent on the timeLog argument formatter so multi-line object output nests correctly. A new test in test/js/web/console/console-timeLog.test.ts covers no-group, single-group, nested-group, string-arg, and multi-line-object cases.
Security risks
None. This is purely output-formatting (leading whitespace) for developer-facing console timer messages. No parsing, no allocation driven by untrusted input, no auth/crypto/permissions surface. The indent buffer is a fixed 64-byte stack array, chunked at 32 levels per iteration with saturating subtraction, so there's no overflow or OOB risk even at the u16 max indent.
Level of scrutiny
Low. The change is ~20 lines of additive logic plus a one-line reorder, follows the exact same default_indent pattern already used elsewhere in this file (e.g. FormatOptions.default_indent → Formatter.indent at lines 844/925, and table_printer.value_formatter.indent += console.default_indent at line 203). It writes to the same stderr stream and flushes in the same place as before, so ordering/buffering behavior is unchanged.
Other factors
The only finding from the bug-hunting pass is a test-style nit (assert exitCode after stdout/stderr per CLAUDE.md), which the pre-existing test in this file also violates and which doesn't affect correctness. No CODEOWNERS apply to these paths, there are no outstanding human review comments, and the PR includes a targeted regression test that fails without the fix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/console/console-timeLog.test.ts`:
- Line 49: The exit-code assertion is placed before the output assertions, which
can hide useful stdout/stderr diffs; in the test in console-timeLog.test.ts move
the expect(exitCode).toBe(0) to the end of the test block so that you first
assert on stdout and stderr expectations (references: stdout, stderr) and only
then assert exitCode, ensuring clearer diagnostics when output mismatches occur.
🪄 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: 8a67c831-be22-41ab-bdad-d326e4ec1969
📒 Files selected for processing (2)
src/bun.js/ConsoleObject.zigtest/js/web/console/console-timeLog.test.ts
There was a problem hiding this comment.
LGTM — small, well-scoped cosmetic fix with test coverage; the inline notes are non-blocking nits.
Extended reasoning...
Overview
This PR fixes #30017: console.timeEnd() and console.timeLog() were writing directly to stderr via Output.printElapsed / Output.printErrorln without consulting console.default_indent, so their output ignored console.group() nesting. The fix adds a small printTimerIndent helper that writes 2 * default_indent spaces to the same stderr stream before the elapsed-time prefix, and sets Formatter.indent = console.default_indent for timeLog's argument formatter so multi-line objects nest correctly. timeEnd now binds the previously-unused global parameter to reach global.bunVM().console.default_indent, mirroring timeLog. A new test in test/js/web/console/console-timeLog.test.ts covers no-group, single-group, nested-group, string-arg, and multi-line-object cases.
Security risks
None. This is purely cosmetic console-output formatting — no parsing of untrusted input, no auth/crypto/permissions, no network or filesystem effects. The indent helper does bounded arithmetic on a u16 (max ~128 KiB of spaces in 64-byte chunks), so there's no overflow or unbounded-allocation concern.
Level of scrutiny
Low. The change is ~25 lines in a non-critical path (developer-facing console output), follows the exact pattern already used for console.log indentation (FormatOptions.default_indent → Formatter.indent at ConsoleObject.zig:844/925), and is covered by a focused regression test that strips the variable elapsed-time prefix to assert exact indentation.
Other factors
Two inline notes are attached, both non-blocking: (1) console.count() in adjacent code has the same pre-existing bug — explicitly out of scope for #30017 and flagged only as a possible follow-up; (2) the new test asserts exitCode before stdout/stderr, contrary to the CLAUDE.md convention — a diagnostic-quality nit only (the pre-existing test in the same file does the same), not a correctness issue. Neither warrants holding the PR.
There was a problem hiding this comment.
LGTM — small spec-conformance fix with good test coverage; my earlier nit on assertion ordering was addressed in 847aa4b.
Extended reasoning...
Overview
This PR fixes #30017 by making console.timeEnd() and console.timeLog() respect console.group() indentation, matching the WHATWG Console spec and the existing behavior of console.log. The change touches two files: src/bun.js/ConsoleObject.zig (~20 lines added — a small printTimerIndent helper, two call sites, and threading default_indent into the timeLog Formatter) and test/js/web/console/console-timeLog.test.ts (one new test covering no-group, single-group, nested-group, string-arg, and multi-line-object cases).
Security risks
None. This is purely cosmetic output formatting — it writes leading spaces to stderr before existing timer output. No user input is parsed differently, no new code paths handle untrusted data, and the indent value is a bounded u16 already managed by the existing console.group() machinery.
Level of scrutiny
Low. The fix is narrow, mechanical, and mirrors how messageWithTypeAndLevel_ already handles indentation for console.log. The helper writes spaces in 64-byte chunks via Output.printError (the same stderr stream used by Output.printElapsed), so the indent and the timer prefix end up in the same buffer. The timeEnd signature change (_ → global) only un-discards an existing parameter to reach bunVM().console.default_indent — timeLog already used global the same way.
Other factors
- The bug-hunting system found no issues.
- My earlier inline nit (assert
exitCodelast per CLAUDE.md) was addressed in commit 847aa4b and the thread is resolved. - I left a non-blocking note that
console.count()has the same pre-existing bug; that's explicitly out of scope for #30017 and doesn't affect this PR. - No CODEOWNERS cover the modified files.
- The new test is deterministic (strips the variable elapsed-time prefix before comparing) and exercises both the leading-indent and the Formatter
.indentchange for multi-line objects.
There was a problem hiding this comment.
LGTM — small, well-scoped indentation fix with a targeted test; the earlier exit-code-ordering nit was addressed in 847aa4b.
Extended reasoning...
Overview
This PR touches two files: src/bun.js/ConsoleObject.zig (~20 lines changed in timeEnd/timeLog plus a new 9-line printTimerIndent helper) and test/js/web/console/console-timeLog.test.ts (one new ~45-line test case). The fix prepends 2 * default_indent spaces to the stderr stream before the elapsed-time prefix, and threads default_indent into the timeLog argument formatter so multi-line objects nest correctly. This mirrors the existing pattern in messageWithTypeAndLevel_ that already handles group indentation for console.log.
Security risks
None. This is purely cosmetic output formatting — leading whitespace before timer output on stderr. No user input flows into control paths beyond what was already there (the timer label and args, which are already handled by the existing code). The new helper writes at most indent * 2 space bytes via a fixed 64-byte chunk loop with no allocation and a clean termination condition.
Level of scrutiny
Low. The change is narrow, additive, and behavior-preserving for the ungrouped case (default_indent == 0 → zero iterations → no output change). The timeEnd signature change (_ → global for the second parameter) is binary-compatible since it was already a *JSGlobalObject parameter, just previously discarded. Moving const console = global.bunVM().console; a few lines earlier in timeLog is a no-op refactor enabling the new .indent field initialization.
Other factors
- No CODEOWNERS cover these paths.
- The new test exercises no-group, single-group, nested-group,
timeLogwith a string arg, andtimeLogwith a multi-line object — exactly the cases described in the PR body, with the elapsed-time prefix stripped so assertions are deterministic. - My only prior comment (assert
exitCodelast) was fixed in commit 847aa4b and the inline thread is resolved. The other inline note aboutconsole.count()was explicitly flagged as pre-existing/out-of-scope and is also resolved. - No outstanding human reviewer comments.
There was a problem hiding this comment.
LGTM — exit-code ordering addressed in 847aa4b; the indent fix follows the existing default_indent → Formatter.indent pattern used elsewhere in this file.
Extended reasoning...
Overview
Two files touched: src/bun.js/ConsoleObject.zig (+22/-3) adds a printTimerIndent helper and calls it from timeEnd/timeLog before Output.printElapsed, plus sets Formatter.indent = console.default_indent for timeLog's extra-argument formatter. test/js/web/console/console-timeLog.test.ts (+47) adds a regression test spawning bun -e and asserting indentation of timer output under nested console.group().
Security risks
None. This is purely cosmetic console-output formatting — writing leading spaces to stderr. No user input parsing, no auth, no filesystem/network access beyond what the existing code path already does.
Level of scrutiny
Low. The fix is a narrow spec-conformance change (WHATWG Console group nesting) that mirrors the existing default_indent → .indent threading already used at ConsoleObject.zig:183/844/925 for console.log. The new helper is a trivial chunked space-writer using the same Output.printError stream as the subsequent printElapsed call, so there's no buffering/interleaving concern. default_indent is u16 and the loop widens to usize before * 2, so no overflow. No CODEOWNERS entry covers this path.
Other factors
My only prior feedback (assert exit code last) was addressed in commit 847aa4b and the thread is resolved. The remaining inline note about console.count() was explicitly flagged as pre-existing/non-blocking and out of scope for #30017. CodeRabbit raised the same exit-code point and confirmed it addressed. The bug-hunting system found no issues. The new test exercises no-group, single-group, nested-group, string arg, and multi-line object cases — adequate coverage for a formatting fix.
Timer output bypassed the indentation stack maintained by console.group() because timeEnd/timeLog write through Output.printElapsed directly without consulting console.default_indent. Ordinary console.log goes through messageWithTypeAndLevel_, which forwards default_indent into FormatOptions. Write the indentation (2 spaces per group depth) before the elapsed-time prefix in both timeEnd and timeLog, and propagate the indent into the timeLog argument formatter so multi-line objects also nest correctly. Fixes #30017
…ows ARM64, unrelated to fix)
Jarred-Sumner
force-pushed
the
farm/bcf84045/timer-group-indent
branch
from
a5d26e1 to
9ee3747
Compare
There was a problem hiding this comment.
LGTM — small, focused fix with test coverage; prior feedback addressed.
Extended reasoning...
Overview
This PR makes console.timeEnd() and console.timeLog() honor console.group() nesting by writing 2 * default_indent leading spaces to stderr before the elapsed-time prefix, and threading default_indent into the timeLog argument formatter so multi-line objects nest correctly. The change touches two files: ~20 lines added to src/jsc/ConsoleObject.zig (a new printTimerIndent helper plus call sites in timeEnd/timeLog) and a new ~50-line test in test/js/web/console/console-timeLog.test.ts.
Security risks
None. This is purely cosmetic console-output formatting — it reads an internal u16 indent counter and writes space characters to stderr. No user input parsing, no auth/crypto/permissions, no new attack surface. The chunked space-writing loop has no overflow risk (u16 * 2 widened to usize).
Level of scrutiny
Low. This is a spec-conformance fix for console output indentation (WHATWG Console group nesting). I verified that Output.printError and Output.printElapsed both write to the same stderr stream (src/bun_core/output.zig), so the indent prefix and elapsed-time text are correctly ordered on the same buffer. The .indent = console.default_indent addition mirrors the existing pattern at ConsoleObject.zig:925 used by console.log.
Other factors
- The bug-hunting system found no issues.
- My earlier inline comment (assert exit code last) was addressed in commit 847aa4b and the thread is resolved.
- My other inline note about
console.count()having the same pre-existing bug was explicitly non-blocking and out of scope for #30017. - The new test covers no-group, single-group, nested-group,
timeLogwith a string arg, andtimeLogwith a multi-line object — exercising both the prefix indent and the formatter indent. - The two later commits are CI retriggers for an unrelated Windows ARM64 HMR flake; no code changes.
There was a problem hiding this comment.
LGTM — small, well-tested cosmetic fix; the exit-code assertion ordering nit was addressed in 9bee600.
Extended reasoning...
Overview
This PR fixes #30017 by making console.timeEnd() and console.timeLog() honor the active console.group() indentation level. It touches two files: src/jsc/ConsoleObject.zig (~20 lines: adds a printTimerIndent helper, names the previously-unused global parameter in timeEnd, prepends indent spaces before Output.printElapsed, and sets Formatter.indent = console.default_indent for timeLog arguments) and test/js/web/console/console-timeLog.test.ts (one new subprocess test covering no-group, single-group, nested-group, and multi-line-object cases).
Security risks
None. This is purely cosmetic stderr-output formatting — no parsing of untrusted input, no auth/crypto/permissions, no resource lifecycle changes. The indent count is a u16 derived from the user's own console.group() calls and is written via a bounded 64-byte chunk loop that mirrors the existing Formatter.writeIndent pattern in the same file.
Level of scrutiny
Low. The fix follows the established pattern exactly: default_indent is the same field messageWithTypeAndLevel_ already threads into Formatter.indent for console.log (lines 183/844/925), the 2-spaces-per-level convention matches existing behavior, and Output.printError writes to the same stderr stream as Output.printElapsed (which delegates to prettyError). The timeEnd signature change only names a previously-discarded parameter — no binding/ABI impact.
Other factors
My earlier inline nit (assert exit code last per CLAUDE.md) was addressed in commit 9bee600 and confirmed resolved. The other inline note about console.count() was explicitly flagged as pre-existing/out-of-scope and non-blocking. The bug-hunting system found no issues. Subsequent commits were a no-behavior-change loop simplification (e3b1087) and CI retriggers for unrelated infra/flake. The new test is deterministic (strips elapsed-time values, normalizes CRLF) and provides good regression coverage.
There was a problem hiding this comment.
LGTM — small, well-scoped spec-conformance fix with solid test coverage; prior feedback addressed.
Extended reasoning...
Overview
This PR fixes #30017 by making console.timeEnd() and console.timeLog() respect console.group() indentation, matching the WHATWG Console spec and the existing behavior of console.log(). Two files touched:
src/jsc/ConsoleObject.zig(+22/−2): adds a smallprintTimerIndenthelper that writes2 * default_indentspaces to the stderr stream before the elapsed-time prefix intimeEndandtimeLog; also setsFormatter.indent = console.default_indentintimeLogso multi-line argument objects nest correctly. ThetimeEndsignature now binds the previously-discardedglobalparameter to readbunVM().console.default_indent.test/js/web/console/console-timeLog.test.ts(+47): a new test spawningbun -eto exercise no-group, single-group, nested-group,timeLogwith a string arg, andtimeLogwith a multi-line object. It strips the variable elapsed-time prefix and normalizes CRLF before asserting exact indentation.
Security risks
None. This is purely cosmetic console-output formatting — no user input parsing, no auth/crypto/permissions, no filesystem or network paths. The indent value is an internal u16 counter incremented/decremented by console.group/groupEnd; the helper widens it to usize before multiplying by 2 and writes in 64-byte chunks, so there is no overflow or unbounded-allocation concern.
Level of scrutiny
Low. The fix mirrors exactly how messageWithTypeAndLevel_ already threads console.default_indent → Formatter.indent (ConsoleObject.zig:183, 844, 925), so it follows an established pattern with no ambiguity. The behavior change is additive (leading whitespace only) and the existing [Xms] label format is preserved. The chunked space-writing loop is straightforward and handles depths >32 correctly.
Other factors
- My one actionable inline comment (assert
exitCodelast per CLAUDE.md) was addressed in 847aa4b and the thread is resolved. - My other inline note about
console.count()having the same pre-existing bug was explicitly flagged as non-blocking / out of scope for #30017 — it does not gate this PR. - CodeRabbit raised the same exitCode-ordering point and confirmed it resolved; no other actionable findings.
- The bug-hunting system found no issues.
- CI retriggers in the commit history are infra-related (agent expiration, an unrelated Windows ARM64 HMR flake), not failures of the new test.
- The test is deterministic: timing values are regex-stripped, CRLF is normalized, and
bunEnvis used for the subprocess.
There was a problem hiding this comment.
LGTM — small, well-scoped spec-conformance fix with a targeted regression test; my earlier exitCode-ordering nit was addressed in 847aa4b.
Extended reasoning...
Overview
This PR fixes #30017: console.timeEnd() and console.timeLog() were writing directly to stderr without consulting console.default_indent, so their output wasn't nested inside active console.group() blocks. The fix adds a small printTimerIndent helper in src/jsc/ConsoleObject.zig that writes 2 × default_indent spaces to the same stderr stream before the elapsed-time prefix, and sets Formatter.indent = default_indent in timeLog so multi-line object arguments also nest correctly. timeEnd now uses its previously-discarded global parameter to reach global.bunVM().console.default_indent. A new test case in test/js/web/console/console-timeLog.test.ts covers no-group, single-group, nested-group, string-arg, and multi-line-object cases.
Security risks
None. This is a cosmetic/output-formatting change to console logging — no input parsing, no auth, no filesystem or network surface, no new allocations beyond the existing buffered stderr writer. The indent helper writes a bounded number of spaces (chunked 64 at a time) derived from a u16 counter that's already used elsewhere in the same file for console.log indentation.
Level of scrutiny
Low. The change is ~20 lines of Zig that mirror the existing default_indent handling in messageWithTypeAndLevel_ (lines 99–183 of the same file). It only affects the visual indentation of timer output and cannot break correctness of timed values, since the indent write happens strictly before the existing Output.printElapsed call. The const console = global.bunVM().console; hoist in timeLog is a pure reordering (it was already computed two lines later).
Other factors
- My only prior actionable comment (assert
exitCodelast per CLAUDE.md) was addressed in commit 847aa4b and the thread is resolved. - My other comment was an explicitly non-blocking, out-of-scope observation about
console.count()having the same pre-existing bug — not a reason to hold this PR. - All inline review threads on the PR are resolved; subsequent commits are CI retriggers for unrelated infra/flake issues per their commit messages.
- The new test is deterministic (strips the variable elapsed-time prefix before asserting) and follows the repo's subprocess-test conventions (
bunExe/bunEnv,await using, exit-code-last).
…mit-close, unrelated to fix)
There was a problem hiding this comment.
LGTM — small, well-contained spec-conformance fix with solid test coverage; the inline note is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes #30017 by making console.timeEnd() and console.timeLog() honor console.group() indentation. It touches two files: src/jsc/ConsoleObject.zig (adds a 10-line printTimerIndent helper, calls it from timeEnd/timeLog before the elapsed-time prefix, and sets Formatter.indent = console.default_indent so multi-line object args nest correctly) and test/js/web/console/console-timeLog.test.ts (adds one subprocess test covering no-group, single-group, nested-group, string-arg, and multi-line-object cases).
Security risks
None. This is purely cosmetic output formatting — it writes leading spaces to stderr before existing timer output. No user input flows into format strings, no auth/crypto/permissions/network/filesystem surface is touched, and the only new state read is the existing console.default_indent: u16 counter.
Level of scrutiny
Low. The change follows the exact pattern already used by format2() (lines 844/925: .indent = options.default_indent), and Output.printError / Output.printElapsed both target the same buffered error stream so ordering is preserved. The timeEnd signature change is just renaming an already-present _: *JSGlobalObject parameter to global — no ABI/binding impact. The chunked space-write loop in printTimerIndent correctly handles the full u16 range without overflow (remaining is usize). Worst-case failure mode is mis-indented diagnostic output, not a correctness or stability issue.
Other factors
All prior review feedback is addressed and resolved: my earlier note about asserting exitCode last was fixed in 847aa4b, and the console.count() observation was acknowledged as out-of-scope. CodeRabbit raised the same exitCode point and confirmed it resolved. The remaining CI failures are Windows ARM64 agent-creation infra issues (per the commit messages and robobun status), not test failures related to this change. No CODEOWNERS coverage applies to these paths. The single inline finding from this run (missing defer fmt.deinit() in timeLog) is explicitly pre-existing on main and non-blocking — a reasonable while-you're-here, but not a reason to hold the PR.
There was a problem hiding this comment.
LGTM — small, well-contained spec-conformance fix with solid test coverage; the inline note is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes #30017 by making console.timeEnd() and console.timeLog() honor console.group() indentation. It touches two files: src/jsc/ConsoleObject.zig (adds a 10-line printTimerIndent helper, calls it from timeEnd/timeLog before the elapsed-time prefix, and sets Formatter.indent = console.default_indent so multi-line object args nest correctly) and test/js/web/console/console-timeLog.test.ts (adds one subprocess test covering no-group, single-group, nested-group, string-arg, and multi-line-object cases).
Security risks
None. This is purely cosmetic output formatting — it writes leading spaces to stderr before existing timer output. No user input flows into format strings, no auth/crypto/permissions/network/filesystem surface is touched, and the only new state read is the existing console.default_indent: u16 counter.
Level of scrutiny
Low. The change follows the exact pattern already used by format2() (lines 844/925: .indent = options.default_indent), and Output.printError / Output.printElapsed both target the same buffered error stream so ordering is preserved. The timeEnd signature change is just renaming an already-present _: *JSGlobalObject parameter to global — no ABI/binding impact. The chunked space-write loop in printTimerIndent correctly handles the full u16 range without overflow (remaining is usize). Worst-case failure mode is mis-indented diagnostic output, not a correctness or stability issue.
Other factors
All prior review feedback is addressed and resolved: my earlier note about asserting exitCode last was fixed in 847aa4b, and the console.count() observation was acknowledged as out-of-scope. CodeRabbit raised the same exitCode point and confirmed it resolved. The remaining CI failures are Windows ARM64 agent-creation infra issues (per the commit messages and robobun status), not test failures related to this change. No CODEOWNERS coverage applies to these paths. The single inline finding from this run (missing defer fmt.deinit() in timeLog) is explicitly pre-existing on main and non-blocking — a reasonable while-you're-here, but not a reason to hold the PR.
The Formatter literal in timeLog() was never released. When timeLog is called with an object/array argument, printAs() acquires a Visited.Pool node that only Formatter.deinit() returns, leaking one ObjectPool node + backing HashMap per call. Matches the pattern in format2().
There was a problem hiding this comment.
LGTM — all prior feedback addressed (exit-code ordering, defer fmt.deinit()); remaining CI red is the Windows ARM64 agent-creation infra flake, not this change.
Extended reasoning...
Overview
Two-file change fixing #30017: console.timeEnd() / console.timeLog() now honor console.group() nesting by writing 2 * default_indent leading spaces to stderr before the elapsed-time prefix, and timeLog's argument Formatter is initialized with .indent = default_indent so multi-line objects nest correctly. A new printTimerIndent helper does the chunked space write via Output.printError (same stream as Output.printElapsed). The diff also hoists const console = global.bunVM().console and adds defer fmt.deinit() (addressing the pre-existing pool-node leak I flagged earlier). A focused subprocess test in console-timeLog.test.ts covers no-group, single-group, nested-group, string-arg, and multi-line-object cases.
Security risks
None. This is purely cosmetic console output formatting — no parsing of untrusted input, no auth, crypto, filesystem, or network surface. default_indent is a u16 controlled by the user's own console.group/groupEnd calls; the helper's chunked loop is bounded by indent * 2 and cannot overflow or recurse.
Level of scrutiny
Low. The change is ~20 functional lines confined to two callbacks in ConsoleObject.zig that already write directly to stderr. It reads the same default_indent field that messageWithTypeAndLevel_ already uses for console.log (line 183) and applies the same .indent = pattern as format2 (lines 844/925), so it follows established conventions exactly. The only signature change (timeEnd now naming its global param instead of _) is ABI-neutral under callconv(jsc.conv).
Other factors
All three of my earlier review threads are resolved: the test now asserts exitCode last (847aa4b), the console.count() note was acknowledged as out-of-scope, and a593c00 added defer fmt.deinit() to release the Visited.Pool node. No bugs were found by the bug-hunting system on the current revision. The remaining CI failure ("Failed to create agent for 🪟 11 aarch64") is an infrastructure agent-provisioning flake unrelated to the diff, consistent with the several "retrigger CI" commits in this branch's history.
…infra recovering)
There was a problem hiding this comment.
LGTM — small spec-conformance fix with test coverage; all earlier feedback addressed.
Extended reasoning...
Overview
This PR makes console.timeEnd() and console.timeLog() honor console.group() nesting, matching the WHATWG Console spec and Bun's existing behavior for console.log. It touches two files: src/jsc/ConsoleObject.zig (~25 lines: a new printTimerIndent helper, two call sites, threading .indent into the timeLog Formatter, and a drive-by defer fmt.deinit() to release a pooled node) and test/js/web/console/console-timeLog.test.ts (one new test case).
Security risks
None. This is purely cosmetic console output formatting — leading whitespace before timer lines on stderr. No user input parsing, no auth/crypto/permissions, no network or filesystem interaction.
Level of scrutiny
Low. The implementation reuses the established default_indent pattern already used by messageWithTypeAndLevel_ (lines 99/183/203) and the Formatter construction sites at lines 844/925. The printTimerIndent helper is a trivial chunked space-writer using the same Output.printError stream that Output.printElapsed already targets, so the indent and the elapsed-time text land on the same buffered writer. The timeEnd signature change (_ → global) just un-discards an already-passed parameter; timeLog already accessed global.bunVM().console so the pattern is proven safe here.
Other factors
All three of my earlier inline comments have been addressed and resolved: the exitCode assertion was moved last (847aa4b), and the missing defer fmt.deinit() was added (a593c00) — the latter actually fixes a small pre-existing pool-node leak as a bonus. The console.count() note was explicitly out-of-scope. No CODEOWNERS coverage applies to these paths. The new test exercises no-group, single-group, nested-group, string-arg, and multi-line-object cases, and CI has been retriggered on the latest commit. No bugs were found in the current review pass.
…e timeout, unrelated)
There was a problem hiding this comment.
LGTM — thanks for adding the defer fmt.deinit() in a593c00.
Extended reasoning...
Overview
This PR fixes #30017 by making console.timeEnd() and console.timeLog() respect console.group() indentation per the WHATWG Console spec. Changes are confined to two functions in src/jsc/ConsoleObject.zig (~30 lines: a new printTimerIndent helper, two call sites, threading default_indent into the timeLog Formatter, and a defer fmt.deinit()) plus a new test case in test/js/web/console/console-timeLog.test.ts.
Security risks
None. This is purely cosmetic console-output formatting — it writes leading spaces to stderr based on an internal u16 counter that's already bounded by the existing console.group machinery. No user-controlled data flows into anything other than the existing print paths; no auth, crypto, FS, or network surface is touched.
Level of scrutiny
Low. The fix is mechanical (prefix N×2 spaces before the existing elapsed-time print), mirrors the indentation logic already used by messageWithTypeAndLevel_ for console.log, and is covered by a targeted test that exercises no-group, single-group, nested-group, string-arg, and multi-line-object cases. The printTimerIndent chunked-write loop is straightforward and handles arbitrary depths correctly.
Other factors
All prior review feedback has been addressed and resolved: the exit-code assertion was moved last (847aa4b), and the pre-existing Formatter pool-node leak I flagged was fixed by adding defer fmt.deinit() (a593c00). The remaining console.count() indentation note was explicitly non-blocking and out of scope for #30017. No CODEOWNERS apply to the touched files. The bug-hunting system found no issues this run. Recent commits are CI-retrigger empties for unrelated infra flakes.
There was a problem hiding this comment.
LGTM — small spec-conformance fix with good test coverage; thanks for picking up the fmt.deinit() leak while you were here.
Extended reasoning...
Overview
This PR makes console.timeEnd() and console.timeLog() respect console.group() indentation, fixing #30017. Changes are confined to two files: src/jsc/ConsoleObject.zig (adds a small printTimerIndent helper, reads default_indent in timeEnd/timeLog, sets Formatter.indent for multi-line args, and adds a defer fmt.deinit() to fix a pre-existing pool-node leak) and test/js/web/console/console-timeLog.test.ts (one new subprocess test covering no-group, single-group, nested-group, string arg, and multi-line object cases).
Security risks
None. This is purely cosmetic console-output formatting — writing leading spaces to stderr before existing timer output. No user input is parsed, no allocations beyond a fixed 64-byte stack buffer, no auth/crypto/network paths touched.
Level of scrutiny
Low. The fix follows the exact pattern already used by messageWithTypeAndLevel_ for console.log (reading console.default_indent and multiplying by 2). The printTimerIndent helper is straightforward chunked-write logic with no edge-case risk (the u16 → usize widening prevents overflow). The timeEnd signature change (_ → global) is ABI-neutral since the parameter was already passed; it's now just used. Neither file is covered by CODEOWNERS.
Other factors
All prior review feedback has been addressed: the exitCode assertion was moved last (847aa4b), and the pre-existing Formatter leak I flagged was fixed by adding defer fmt.deinit() (a593c00). The remaining console.count() observation was explicitly non-blocking and out of scope for #30017. The new test is deterministic (strips elapsed-time values, normalizes CRLF) and exercises both the indent prefix and the nested-object formatter path. CI has been retriggered for unrelated Windows ARM64 flakes.
The hot.test.ts 'should work with sourcemap generation' flake hit ERROR bucket on build 51604 instead of WARNING bucket — but this is a well-documented global flake, not PR-specific: - #28645 retriggered 3x ('seen 5+ times, unrelated to PR') - #29945, #30018, #30026 all retriggered for the same test - #29587 build 51335: 'windows-11-aarch64 shard 6/8: hot.test.ts — known hot-reload flake' - #29735 open: 'test(hot): use atomic rename for 2MB rewrites in sourcemap-generation test' — test-level workaround - #29740 merged: 'hot: defer reload while a rejected module is unreported' — prior runtime fix attempt Local reproduction: ~5% flake rate at my branch tip (e86f538), at 5215594 (before drain-in-reload), and at 0ce2096 (before the forever-timer carve-out). Rate is identical across all three, which means the flake is inherent to the test (qemu-aarch64 agent slowness hitting the 10s timeout before 50 hot-reload cycles complete) and not caused by any change on this PR. Also same plain-old test-http-should-emit-close flake on 2019 x64 baseline lanes (the one I've been retrying since build 51163).
Hot reload 'should work with sourcemap generation' flakes at ~5% on windows-11-aarch64 under qemu: the 10s per-test timeout fires before 50 hot-reload cycles complete, independent of this PR. Cross-PR evidence from the last 24h: - #28645 retriggered 3 times for the same test - #29945, #30018, #30026 all retriggered - #29587 build 51335: documented as 'known hot-reload flake' - #29735 open: test-level workaround in progress - #29740 merged: runtime fix for a related --hot sourcemap race Local repro: identical ~5% rate at my tip (e86f538), at 5215594 (before the stranded-defer fix), and at 0ce2096 (before the forever-timer carve-out). Rate unchanged across the three, confirming the flake is not in this PR's footprint. Same persistent test-http-should-emit-close flake on 2019 x64-baseline (seen in 51163, 51423, 51448, 51473, 51503, 51528, 51604).
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.