Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. Walkthrough
ChangesConsole.table output
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable issue remains in the supplied review evidence; this change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:47 PM PT - Oct 5th, 2026
❌ @robobun, your commit e598b01 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33453That installs a local version of the PR into your bun-33453 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward Node compat fix switching cell padding from centered to left-aligned.
Extended reasoning...
Overview
This PR changes two lines in renderRow in src/js/builtins/ConsoleObject.ts so that Console#table (the node:console constructor path) left-aligns cell content instead of centering it, matching Node.js behavior since v18. It updates one existing inline snapshot in console-table-iterators.test.ts and adds two new snapshot tests in console.test.ts covering mixed columns, absent cells, object-keyed index rows, and wide-character (CJK) width handling.
Security risks
None. This is pure output-formatting logic in a builtin JS module — no parsing of untrusted input, no I/O, no auth/crypto/permissions surface. needed = columnWidths[i] - len is guaranteed non-negative because columnWidths[i] is computed as the max of all cell widths in that column, so String.prototype.repeat cannot throw.
Level of scrutiny
Low. The source change is a 2-line mechanical replacement that mirrors Node's lib/internal/cli_table.js implementation verbatim (minus the vestigial MathCeil). The PR description cites the upstream source, provides a byte-for-byte diff against Node v26.3.0, and confirms the tests fail without the fix. This only affects the new Console(...) path; the global console.table native renderer is untouched.
Other factors
No CODEOWNERS on these paths, no outstanding reviewer comments, and the bug hunter found nothing. The updated snapshot in console-table-iterators.test.ts is a necessary consequence of the fix (it was recording the old centered output). Test coverage is solid and the expected outputs were transcribed from Node rather than captured from Bun.
|
Not a duplicate of #32619, though they are adjacent. Bun has two independent
There is no file overlap between the two diffs, so they can land in either order. The two also fix different things. #32619 changes the native renderer's index column (header text and alignment); its data cells were already left-aligned. This PR changes the builtin renderer's cell padding from centered to right-padded, which affects every cell including the header row. Today the builtin path is the only one that centers: |
202a9cd to
276e34d
Compare
There was a problem hiding this comment.
LGTM — straightforward Node compat fix; the cookie-map.test.ts CI failure is unrelated to this change.
Extended reasoning...
Overview
This PR changes two lines in renderRow (src/js/builtins/ConsoleObject.ts) to right-pad table cells instead of centering them, matching Node's lib/internal/cli_table.js since v18. It updates one existing inline snapshot and adds two new snapshot tests in test/js/node/console/ covering mixed-width columns, absent cells, string-keyed indexes, and CJK width handling.
Security risks
None. This is pure output-formatting logic for a debugging utility (Console#table). No user input parsing beyond what already existed, no I/O, no auth/crypto/permissions surface.
Level of scrutiny
Low. The functional diff is a two-line arithmetic simplification: (columnWidths[i] - len) / 2 split across two .repeat() calls becomes a single trailing .repeat(columnWidths[i] - len). columnWidths[i] is computed as the max of getStringWidth over the header and every cell in that column, so needed is a non-negative integer and .repeat(needed) cannot throw. The total padding (and therefore column width / box borders) is unchanged — only the position of the whitespace moves. The expected snapshots were transcribed from Node v26.3.0 rather than captured from Bun, which is the right verification methodology for a compat fix.
Other factors
- No CODEOWNERS entry covers
src/js/builtins/ConsoleObject.ts. - Grepping
test/for(index)/(iteration index)table snapshots turns up only the two files already updated in this PR, so no other snapshots will break. - The single CI failure is
test/js/bun/cookie/cookie-map.test.ts, which this PR does not touch; it's the IMF-fixdate assertion issue addressed by #33425 on main and is unrelated. - The bug-hunting pass found no issues.
- The PR description already clarifies the non-overlap with #32619 (native renderer vs. builtin
node:consolerenderer).
There was a problem hiding this comment.
LGTM — straightforward Node compat fix with matching snapshot tests.
Extended reasoning...
Overview
This PR changes 3 lines in renderRow inside src/js/builtins/ConsoleObject.ts to right-pad table cells instead of centering them, matching Node's lib/internal/cli_table.js behavior since v18. It updates one existing inline snapshot in console-table-iterators.test.ts (which was recording the old centered output) and adds two new snapshot tests in console.test.ts covering mixed-width columns, absent cells, object-keyed index columns, and CJK wide-character width handling.
Security risks
None. This is pure output-formatting code for node:console's Console#table — no parsing of untrusted input, no auth/crypto/permissions, no filesystem or network access. needed = columnWidths[i] - len is guaranteed non-negative because columnWidths[i] is computed as the max of header width and every cell's getStringWidth, so String.prototype.repeat cannot receive a negative count.
Level of scrutiny
Low. The source change is a mechanical one-liner that mirrors upstream Node's implementation verbatim (the PR description quotes the Node source). It only affects the JS-side new Console(...) path, not the native global console.table renderer. The updated snapshot and new tests were transcribed from Node v26.3.0 output rather than captured from Bun, so they act as a real Node-compat regression check.
Other factors
The bug-hunting system found no issues. There are no outstanding human review comments — timeline activity is all bot noise (CodeRabbit rate-limit notice, build status, duplicate-PR detector which the author already addressed). The change follows existing file conventions (StringPrototypeRepeat.$call). Given the tiny surface area, clear upstream reference, and solid test coverage, this is safe to approve without human review.
StatusThe fix is ready for review. The diff is green. CI is red because of two jobs, and neither failure comes from this diff. CI (build 123479, e598b01). The build is finished: 179 jobs passed and 2 failed.
A maintainer can run the two failed jobs again, or merge without them. How to reproduce. Run this script under Node and under Bun: import { Console } from "node:console";
import { Writable } from "node:stream";
let out = "";
const sink = () => new Writable({ decodeStrings: false, write(c, e, cb) { out += c; cb(); } });
new Console({ stdout: sink(), stderr: sink() }).table([{ aaaaa: 1, b: "x" }, { aaaaa: 22222, c: true }]);
process.stdout.write(out);Node v26.3.0 pads each cell on the right. Bun 1.4.3 (367d939) centers each cell. This branch prints the same bytes as Node. How it is verified.
|
|
Closing this PR because it has been inactive for more than 90 days. |
The Console class's renderRow split the leftover width across both sides of the cell, which centers it. That is Node's behavior up to v20.10 and v21.1. Node has padded only on the right since v20.11.0 and v21.2.0 (nodejs/node#50135), and Bun reports a newer Node version than that. Every other part of the output (box frame, column widths, quoting) already matched, so a byte comparison of Console#table output against Node differed in the padding alone.
379dfce to
7d86567
Compare
|
Reopened. The stale sweep closed this PR before a review, and the bug still reproduces on main (13a98b0): Changes since the last push:
Node's own |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
test/js/node/console/console-table-iterators.test.ts:32— Users passing a Map iterator to Console#table still get a single 'Values' column of [key, value] pairs after merging, while Node prints separate 'Key' and 'Values' columns; the rewritten snapshot now certifies that divergence under a PR titled as Node parity. The mapIter branch that would split keys and values is dead because previewEntries is commented out at src/js/builtins/ConsoleObject.ts:666-670, so isKeyValue stays false and the iterator falls into the setlike path at src/js/builtins/ConsoleObject.ts:695-703. Fix: wire the Map-iterator path to Node's Key/Values layout (or document the exclusion) and update this snapshot to Node's three-column output instead of re-recording the wrong one.Why this was flagged
Trigger: any caller doing new Console(...).table(map.entries()) or table(mapSymbol.iterator), the exact input this test exercises at test/js/node/console/console-table-iterators.test.ts:14. In src/js/builtins/ConsoleObject.ts:663 mapIter is true, but the previewEntries call at lines 666-670 is commented out, so isKeyValue never becomes true and the keys/values split at lines 676-681 is unreachable; the iterator is consumed by the setlike loop at lines 699-702 and each entry is rendered as the inspected array [ 'a', 1 ]. Node's cli_table path renders (iteration index) | Key | Values for the same input. The base branch already behaved this way, but this PR rewrote the inline snapshot at lines 27-32 to the new padding while keeping the single-column layout, which per the repo rules certifies the bug in a PR whose stated goal is byte-identical Node output. Remedy: implement the Key/Values split for Map iterators (previewEntries equivalent) and record Node's actual output.
Verification: pre-existing. Trigger: any caller passing a Map iterator to new Console(...).table(). In src/js/builtins/ConsoleObject.ts lines 666-670 are commented out, so the key/value split at 676-681 is unreachable and line 695 routes the Map iterator into the loop at 699-702, yielding a single Values column. The base snapshot already pinned the identical structure; the diff only changes padding whitespace.
Node prints a Map iterator with Key and Values columns. Bun prints the pairs in one Values column. The snapshot records Bun's layout, so say that it is not Node's.
|
On the Map iterator finding: it is correct, and it is older than this PR. For The cause is in #42359 is the open PR for that layout. It reads the iterator entries natively and replaces this snapshot with Node's three columns. The description of this PR lists the gap under Downsides. The snapshot must not read as Node's layout, so e598b01 adds one comment above it. The comment names the gap and links #42359. |
There was a problem hiding this comment.
LGTM — a small, self-contained padding change in Console#table that matches Node's current cli_table.js, with snapshot coverage.
What was reviewed:
renderRowinsrc/js/builtins/ConsoleObject.ts:neededismax(header width, every cell width) - cell width, so it is always a non-negative integer andString.prototype.repeatcannot throw; theas anycast was only needed for the old fractional value.- The new
Mapsnapshot inconsole.test.tsdoes not contradict the "known gap" note: a plainMaptakes the$isMapbranch and already getsKey/Valuescolumns; the gap is only for Map iterators, which the sibling test still covers with the singleValuescolumn. - Checked the CJK snapshot arithmetic (
'日本語'width 8, divider 10) and grepped the suite for other assertions on the old centered layout — the only ones are the two files updated here;test/js/bun/console/exercises the native globalconsole.table, which this PR does not touch.
Extended reasoning...
The diff changes three lines in the renderRow helper of the node:console Console class builtin (src/js/builtins/ConsoleObject.ts) to pad each table cell on the right instead of centering it, adds a Node source permalink, updates one existing inline snapshot, and adds five new inline-snapshot tests following the file's existing writable() helper pattern. It touches no security-sensitive surface (no parsing of untrusted input, no auth or crypto), keeps the $call primordial-safe style, and the padding arithmetic is provably non-negative from the columnWidths computation a few lines above. No CODEOWNERS entry covers these paths, the hunt ran dry with zero findings, and the second commit since the last review only adds an explanatory comment to a test, so the change is simple enough to approve without a human pass.
Problem
new Console(stream).table(...)fromnode:consolecenters every cell. Node v26.3.0 pads each cell on the right only. The frame, column widths and quoting already match, so the outputs differ in the padding alone.renderRow(src/js/builtins/ConsoleObject.ts:198). It writes half of the spare width on each side of the cell, as Node did up to v20.10 and v21.1.Fix
renderRowwrites the cell and then the whole spare width as spaces, as Node'slib/internal/cli_table.jsdoes.process.versionv26.3.0.test/js/node/console/console.test.ts(5 new tests) andconsole-table-iterators.test.ts. Stock Bun 1.4.3 fails 6 of the 13 tests in those two files. Also all oftest/js/node/console/andtest/js/bun/console/.Background
Consoleclass ofnode:consoleis a JS builtin that cites Node v20.6.1 as its source. The globalconsole.tableis native code with its own format. This PR changes only the first.Console#tableinto the native printer, to keep one renderer. That printer prints no(index)header and no quotes, so it differs more from Node.Downsides
Console#tableoutput recorded with an earlier Bun.Console#tablestill differs from Node for a Map entries iterator and a nested object cell (console, Bun.inspect, util.inspect: print a Map or Set iterator without running it #42359, node:console: fix Console#table throwing on object cells, Maps and Sets #33452). The globalconsole.tablekeeps its own format.repeatcall, where it made two.Notes
Repro
Which Node versions pad on the right.
lib/internal/cli_table.jsline 43 at each release tag:const needed = (columnWidths[i] - len) / 2;const needed = (columnWidths[i] - len);The Node commit is nodejs/node@14af167209 (2023-10-28). Bun's file cites nodejs/node@d2c7c367 (v20.6.1, 2023-09-07), which still centers. An earlier revision of this description said that Node pads on the right since v18. That was wrong.
The tests. Each expected table in
console.test.tsis the output of node v26.3.0 for the same call. A script with the same calls, plus a Set iterator, prints 4106 identical bytes under node v26.3.0 and under this branch. The snapshot inconsole-table-iterators.test.tsrecorded the centered padding. Only its padding changes. Its first table is Bun's layout for a Map iterator, not Node's, and a comment above the snapshot says so.Node's own test.
test/parallel/test-console-table.jsat v26.3.0 has 33 cases. With each case in its owntry, stock Bun 1.4.3 passes 8 and this branch passes 31. Node passes 33 with the same wrapper. The 2 cases that remain are not about padding:new Map([[1, 1], [2, 2], [3, 3]]).entries(): Node printsKeyandValuescolumns. console, Bun.inspect, util.inspect: print a Map or Set iterator without running it #42359 covers it.{ a: { a: { a: 1, b: 2, c: 3 } } }: Bun throwsundefined is not a function. node:console: fix Console#table throwing on object cells, Maps and Sets #33452 covers it.The file cannot be added to
test/js/node/test/parallel/verbatim until those two land.Other PRs on this renderer. #33452 and #42359 add snapshots with the centered padding to the same two test files. The PR that lands later needs its snapshots updated.
Also run on the debug build.
test/js/node/console/andtest/js/bun/console/: 104 pass, 1 skip (a test that this PR does not touch), 0 fail. The vendoredtest-console-with-frozen-intrinsics.js,test-console-instance.js,test-console-methods.js,test-console-group.js,test-console-tty-colors.js,test-console-tty-colors-per-stream.jsandtest-console-formatTime.jsexit 0.bun x tsc --noEmit -p src/js/tsconfig.jsonreports no error.Binary size. The CI size report of build 123479 shows +0.0 KB on eleven targets and +0.5 KB on
bun-windows-aarch64.History. The stale sweep closed this PR on 2026-10-05 with no review.
renderRowon main was unchanged, so the branch is rebased onto 13a98b0 and the PR is open again.[human-review] gate passed · iteration 4 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 4
evidence per changed file