Conversation
|
Updated 12:00 AM PT - Jun 29th, 2026
❌ @robobun, your commit 71f18b1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32619That installs a local version of the PR into your bun-32619 --bun |
|
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)
Walkthrough
Changesconsole.table Node Compatibility
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/bun/console/console-table.test.ts`:
- Around line 251-257: The test function starting with "table from node:console
matches global console.table" executes two subprocess calls via run() on lines
253-254 but does not verify their exit status. Add assertions to check that both
the fromImport and fromGlobal results include a successful exit code (typically
exitCode === 0 or similar property from the run() return value) to ensure
subprocess failures are properly detected rather than silently masked by
subsequent output comparisons.
🪄 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: 56c5aa2b-7fcf-4228-9047-0e77512d4236
⛔ Files ignored due to path filters (2)
test/js/bun/console/__snapshots__/bun-inspect-table.test.ts.snapis excluded by!**/*.snaptest/js/bun/console/__snapshots__/console-table.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/bun/console/console-table.test.ts
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
LGTM — the crash fix in 6ad9434 looks right (all four is_map consumers now share the single strict field), and the remaining MapIterator/SetIterator header nit is cosmetic and non-blocking.
Extended reasoning...
Overview
This PR aligns console.table / Bun.inspect.table / node:console's table with Node's output: the first column is now labeled (index) (or (iteration index) for Map/Set) and left-aligned, instead of blank and right-aligned. Touches:
src/jsc/ConsoleObject.rs: ~30 lines — adds a strictis_map: boolfield toTablePrinter, swaps padding order inprint_row, and names the index column inprint_table.- Two snapshot files (mechanical regeneration).
test/js/bun/console/console-table.test.ts: updated existing literal + newdescribe.concurrentblock with 7 subprocess tests covering arrays, objects, Map, Set, WeakMap/WeakSet, and the WeakMap-with-expando crash case.docs/runtime/utils.mdx: regenerated two example output blocks.
Review history
This PR has been through two prior review rounds from me. Round 1 flagged stale docs, sequential subprocess tests, and the WeakMap/WeakSet header edge case — all fixed in 0e1ff80. Round 2 caught a real crash regression introduced by 0e1ff80 (producer/consumer desync on the Key column for a WeakMap with an own property) — fixed in 6ad9434 by storing a single strict is_map on TablePrinter that all four sites share. I verified the current code: lines 740, 865, 870, 954, 965 all read self.is_map; no loose jstype.is_map() consumers remain. A regression test for the crash case was added. CodeRabbit's exit-code feedback was also addressed in e3a4c0e.
Security risks
None. This is purely cosmetic output formatting of a debugging API (console.table). No untrusted input parsing beyond what already existed, no auth/crypto/permissions, no network or filesystem effects.
Level of scrutiny
Low-to-moderate. The logic change is small and self-contained; the bulk of the diff is regenerated snapshots. The one risky part (the is_map refactor) was caught, fixed, and now has a dedicated crash test. The remaining inline nit (MapIterator/SetIterator getting (index) instead of Node's (iteration index)) is cosmetic-only for an unusual input where Bun's body rendering already diverges from Node, so it doesn't block.
Other factors
Test coverage is good: existing snapshots regenerated, new exact-output subprocess tests for the common cases, plus the WeakMap crash regression test. All prior reviewer comments (mine and CodeRabbit's) are resolved.
The native TablePrinter (shared by console.table, node:console's table, and Bun.inspect.table) left the first column header blank and right-aligned that column. Node labels it "(index)", or "(iteration index)" for Map and Set, and left-aligns it. - Name the index column "(index)", or "(iteration index)" for real Map/Set. A strict is_map (computed once in init) gates the header, the "Key" column, and the Map-entry key/value extraction so the producer and consumers can't desync: the loose JSType::is_map()/is_set() also match WeakMap/WeakSet, which are not iterable and render as plain objects with "(index)" in Node. - Left-align the index column. Regenerates the console table snapshots and the two Bun.inspect.table examples in docs/runtime/utils.mdx, and adds console.table / node:console regression coverage including WeakMap/WeakSet and a WeakMap with an own property (which must not take the Map "Key" column path). Refs #32614
6ad9434 to
ba42b03
Compare
There was a problem hiding this comment.
LGTM — all earlier feedback addressed (including the WeakMap-with-own-property crash), and the declined MapIterator header nit is reasonable to defer.
Extended reasoning...
Overview
This PR changes console.table's index column to match Node: the header is now (index) (or (iteration index) for Map/Set) and the column is left-aligned. The Rust change in src/jsc/ConsoleObject.rs is small: a new is_map: bool field on TablePrinter (strict JSType::Map only, computed once in init and shared by the header, the Key-column push, and the row-handling consumer), the header string, and a swap of leading/trailing padding in print_row. The two snapshot files are regenerated mechanically, docs/runtime/utils.mdx example output is updated to match, and console-table.test.ts gains a describe.concurrent block of subprocess tests covering arrays, plain objects, Map, Set, WeakMap/WeakSet, and the WeakMap-with-expando-property regression case.
Security risks
None. This is purely cosmetic output formatting in console.table / Bun.inspect.table — no parsing of untrusted input, no auth/crypto/permissions, no network or filesystem behavior.
Level of scrutiny
Low-to-moderate. The native change is narrow (header text + padding order + a stored boolean), and the single risky interaction — desync between the Key-column producer and its consumers for WeakMap — was caught in review, fixed by hoisting a single is_map source of truth onto the struct, and now has a dedicated "WeakMap with an own property renders as a plain object (no crash)" test. I verified the current ConsoleObject.rs has no remaining jstype.is_map() call sites; all three users gate on self.is_map.
Other factors
Across three review rounds the author addressed every actionable comment: docs example regeneration, describe.concurrent, strict Map/Set vs WeakMap/WeakSet, and the columns[1] index-out-of-bounds crash. The one remaining MapIterator/SetIterator header nit was declined with a coherent rationale (Bun renders those bodies as generic iterables, so widening only the header would be internally inconsistent; full Node parity there needs the iterator-preview path, which is out of scope). Snapshot diffs are large but mechanical, and the new subprocess tests assert exact stdout plus exit code. No outstanding reviewer comments remain.
There was a problem hiding this comment.
LGTM — all prior feedback addressed and the rebase onto #32924 looks clean.
Extended reasoning...
Overview
This PR changes console.table / Bun.inspect.table / node:console's table to match Node's index-column rendering: the first column is now labelled (index) (or (iteration index) for Map/Set) and left-aligned, instead of a blank header with right alignment. The Rust change in src/jsc/ConsoleObject.rs is small: a new strict is_map: bool field on TablePrinter (computed once in init), the header-name selection in print_table, and a padding swap in print_row. The remaining diff is regenerated snapshots, updated hard-coded table strings in the existing getter-once tests, two doc example blocks, and a new describe.concurrent block of subprocess tests covering arrays, plain objects, Map/Set, WeakMap/WeakSet, and the WeakMap-with-expando case.
Security risks
None. This is purely cosmetic output formatting of a debugging API; no auth, crypto, filesystem, network, or untrusted-input parsing is touched.
Level of scrutiny
Low-to-moderate. The behavioral surface is debug output only, and the change is tightly scoped. The one real risk class — desync between the Key-column producer and its consumers when narrowing Map detection — was caught in an earlier review round, fixed by storing a single is_map field, and is now covered by a regression test (WeakMap with an own property renders as a plain object (no crash)). I re-checked the current ConsoleObject.rs: after the rebase onto #32924, print_row no longer has any Map-specific branching (it reads pre-collected row.cells), and the only remaining Map gates (update_columns_for_row:786, print_table:962/973) all read self.is_map, so producer and consumers cannot diverge.
Other factors
All four of my prior inline comments are resolved: docs regenerated, describe.concurrent applied, strict Map/Set gating, and the WeakMap-expando crash fix. The remaining MapIterator/SetIterator header nit was reasonably declined as out of scope (Bun's body rendering for those already differs from Node, so changing only the header would be inconsistent). CodeRabbit's exit-code suggestion was also applied. The PR was rebased onto main after #32924's TablePrinter refactor and the rebase note accurately describes the re-application; the latest push is just a CI retrigger. No outstanding human review comments.
|
Diff is green; the red CI lanes are unrelated infrastructure/flake, not this change (which only touches
No |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
console.table(andnode:console'stable, plusBun.inspect.table, which share the nativeTablePrinter) left the first column header blank and right-aligned that column. Node labels it(index), or(iteration index)forMap/Set, and left-aligns it.Before:
After (Node's header and alignment):
Cause
print_tablealways created the first column named" "(a single space), andprint_rowright-aligned that column. The blank header was invisible because the column was never wider than its numeric keys; widening it to the real header name also exposed the right-alignment, which Node does not do.Fix
(index), or(iteration index)forMap/Set(via the existingJSType::is_map/is_set). The column width already grows to fit the header name.Map/Setalso pick up the(iteration index)header:Scope
Issue #32614 also notes that Node quotes string cell values (
'Y'vs Bun's bareY). Whether to quote/escape string cells is the same user-visible decision already open in #32224 and #29084, which touch the same lines. To avoid competing changes there, this PR is limited to the index column and leaves string-cell rendering unchanged.Verification
passes. The added
console.table/node:consoleregression tests fail on the current release and pass with this change. Both snapshot files are regenerated; the only diffs are the index column header and its alignment.Rebase note
Rebased onto main after #32924 ("console.table: invoke each cell getter once") landed and refactored the same
TablePrinter(rows are now collected once intoCollectedRow,RowKey::Strbecame a struct variant, andprint_rowis&self). Re-applied the three changes on top of the new structure:(index)/(iteration index)header naming,print_row,is_mapfield gating the Map "Key" column and key/value extraction.Also updated the hardcoded table strings in #32924's new "console.table reads each cell once" tests to the
(index)layout, and regenerated both snapshot files. No behavioral interaction with the getter-once change: the cell-collection path is unchanged.