Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR modifies the ChangesConsole#table Buffer Detection and Tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced on stock 1.4.0 and on Fail-before/pass-after: CI status on
|
| passed | 275 |
| failed | 0 |
| expired | 11 |
The 11 expired jobs have started_at: null and exit_status: null — they were never dispatched to an agent, they aged out of the queue. They are all on the scarcer pools (darwin x6, ubuntu x2, alpine x3). 219 of the 230 test-bun lanes ran and passed, and the build carries no error annotations at all; the only non-info annotation is a flaky warning for test/napi/napi.test.ts on Windows x64-baseline, which passed on its retry.
Nothing here is reachable from this change: the diff is two .ts files, one of them a test.
I'd rather not force-push a no-op commit just to re-queue 277 jobs into the pool that just dropped 11, so I've left it. A "Rebuild" or "Retry failed jobs" from the Buildkite UI is the cheap fix if a green tick is wanted before merge (my API token is read-scoped, so I can't retry the expired jobs myself).
For context on the earlier red run (build 68845): that was a genuine failure, but not from this change either. The branch predated #33425, so it carried cookie-map.test.ts's pre-IMF-fixdate assertions against the new serializer from #32926. Rebasing onto 48ff9eb fixed it, and every cookie-map lane has passed since.
There was a problem hiding this comment.
LGTM — one-line destructuring fix matching Node's own constructor.js, with solid snapshot coverage.
Extended reasoning...
Overview
This PR fixes a single incorrect destructuring in src/js/builtins/ConsoleObject.ts: const { isBuffer } = require("node:buffer") becomes const { Buffer: { isBuffer } } = require("node:buffer"). The node:buffer module has no top-level isBuffer export (in Bun or Node) — it lives on Buffer. The undefined binding meant Console#table threw TypeError: undefined is not a function whenever a cell was a non-array object, since the isArray classifier fell through to calling isBuffer(v). The fix mirrors Node's own lib/internal/console/constructor.js verbatim. Eight new inline-snapshot tests are added to the existing test/js/node/console/console.test.ts covering object cells, array rows, Map/Set with object keys/values, Buffer cells, the properties filter, the >2-key collapse path, and a primitive-only baseline.
Security risks
None. This is output formatting for console.table on a user-constructed Console instance. No parsing of untrusted input, no auth, no filesystem or network I/O, no memory management. The change only makes an already-referenced predicate resolve to a real function instead of undefined.
Level of scrutiny
Low. The source change is three lines (a destructuring pattern) and is mechanically verifiable: Buffer.isBuffer is a documented static method and Bun defines it in src/js/builtins/JSBufferConstructor.ts. The only consumer of the binding is the local isArray helper used inside table(). There are no other call sites to audit and no behavioral change beyond "stops throwing and renders like Node".
Other factors
Tests are added to the existing test file (not a new one), use inline snapshots verified against Node v26.3.0 output per the PR description, and exercise every branch the isArray predicate feeds (_inspect depth selection, Map/Set paths, properties filter). Six of the eight new tests fail on main with the exact TypeError, so the regression guard is real. No outstanding reviewer comments; only bot noise (CodeRabbit rate-limit notice, robobun build link) in the timeline. No prior review from me.
|
No changes needed from the review. One note for anyone triaging this next to the other open Relatedly, Bun's |
54161f6 to
2291e1a
Compare
There was a problem hiding this comment.
LGTM — one-line destructuring fix that mirrors Node's own constructor.js, with solid snapshot coverage.
Extended reasoning...
Overview
The PR fixes Console#table (the node:console class, not the native global console.table) throwing TypeError: undefined is not a function whenever a table cell is a non-array object, or when passed a Map/Set containing objects. The root cause is a bad destructure: const { isBuffer } = require("node:buffer") binds undefined because node:buffer has no top-level isBuffer export (only Buffer.isBuffer). The fix changes it to const { Buffer: { isBuffer } } = require("node:buffer"), which is exactly what Node's lib/internal/console/constructor.js does. The only source change is that single destructure in src/js/builtins/ConsoleObject.ts; the rest is 8 new snapshot tests in test/js/node/console/console.test.ts.
Security risks
None. This is a pure output-formatting path (Console#table) with no auth, filesystem, network, or crypto involvement. The change swaps one undefined binding for a correct static method reference; it does not introduce any new user-controlled input handling or change any trust boundary.
Level of scrutiny
Low. The functional change is a single, mechanical destructuring fix in a Node-compat JS builtin, with a clear root cause and a direct upstream reference implementation to compare against. It is not in a hot path for the default global console (which uses the native renderer), and it cannot make anything worse — the previous binding was undefined, so any code path reaching isBuffer(v) already threw.
Other factors
- No CODEOWNERS entries cover the touched files.
- Bug hunter found no issues.
- Tests are well-constructed: they exercise object cells, array rows, Map (object key + object value), Set, Buffer cells, the
propertiesfilter, the> 2 keys → [Object]collapse, and a primitive-only baseline. 6 of 8 fail-before/pass-after per the PR verification. - No outstanding human review comments; only bot noise (CodeRabbit rate limit, robobun build/verification).
- The pre-existing divergences noted (centering vs left-align,
previewEntries) are correctly scoped out and unrelated to this fix.
|
Force-pushed a rebase onto Worth flagging why the earlier run on this PR was red, because it was not this change. Every The branch was cut from The rebase picks it up. |
The Console class renderer classifies cells with `$isJSArray(v) || $isTypedArrayView(v) || isBuffer(v)`, but isBuffer was destructured off the node:buffer module namespace, which has no top-level isBuffer export. It was undefined, so the first non-array object cell threw "TypeError: undefined is not a function". Destructure it off Buffer, matching node's lib/internal/console/constructor.js.
2291e1a to
d4c969e
Compare
There was a problem hiding this comment.
LGTM — one-line destructuring fix (Buffer.isBuffer instead of the nonexistent top-level isBuffer export) with solid snapshot coverage.
Extended reasoning...
Overview
The source change is a single destructuring fix in src/js/builtins/ConsoleObject.ts: const { isBuffer } = require("node:buffer") → const { Buffer: { isBuffer } } = require("node:buffer"). Verified independently that require("node:buffer").isBuffer is undefined in Node (and Bun), while Buffer.isBuffer is a function — so the previous binding was always undefined and the isArray predicate would throw on the first non-array/non-typed-array object cell. The rest of the diff is 8 new inline-snapshot tests in test/js/node/console/console.test.ts exercising object cells, array rows, Map/Set, Buffer cells, the properties filter, and the >2-key collapse path.
Security risks
None. This is a Node-compat rendering path for console.Console#table; no auth, crypto, filesystem, or network surface is touched, and the change only affects which existing static method is bound at module init.
Level of scrutiny
Low. The fix is mechanical and matches Node's own lib/internal/console/constructor.js (const { Buffer: { isBuffer } } = require('buffer')). It only affects the JS Console constructor path, not the native global console.table renderer, so blast radius is limited to code that constructs its own Console instance.
Other factors
Tests are placed in the correct existing file, use colorMode: false for stable snapshots, and 6 of 8 fail on main per the PR's fail-before/pass-after check. The earlier CI red was an unrelated cookie-map snapshot drift resolved by rebase. No prior reviewer comments to address and no CODEOWNER-gated paths involved.
|
New evidence for this PR, found in the review of #42359. The bug is not limited to const { Worker, isMainThread } = require("node:worker_threads");
if (isMainThread) {
const w = new Worker(__filename);
w.on("error", e => console.log("worker error:", e.message));
w.on("exit", code => console.log("worker exit", code));
} else {
console.table([{ createdAt: new Date(0) }]);
}
// bun 1.4.2: worker error: undefined is not a function, then worker exit 1
// node 26.3: prints the table, then worker exit 0#42359 carries the identical import line, because Node's per-cell path for |
new Console(stream).table(data)throwsTypeError: undefined is not a functionfor any table with an object cell, and for everyMaporSetwhose values are objects. Only tables made entirely of primitives render, which is why a smoke test never catches it. The globalconsole.tableis a separate native renderer, so a library that builds its ownConsoleover a file stream or a capture buffer is the only thing that hits this.Repro
Cause
src/js/builtins/ConsoleObject.tsclassifies table cells withand imported the predicate as
const { isBuffer } = require("node:buffer").node:bufferhas no top-levelisBufferexport (in Bun or in Node), soisBufferwasundefined.isArrayis evaluated for every cell that is an object, and the first one that is neither an array nor a typed array reaches theisBuffer(v)call and throws. Arrays and typed arrays short-circuit, and primitives never reachisArrayat all, which is why primitive-only tables work.This has been wrong since
console.Consolelanded in #5448.Fix
Destructure
isBufferoffBuffer, which is what Node'slib/internal/console/constructor.jsdoes:Verification
Checked every case against
node v26.3.0: object cells in object rows and in array rows,Mapwith object keys and object values,Setwith object values,Buffercells, typed array cells, nested array cells, thepropertiesfilter, and the> 2keys path that collapses to[Object]. Cell contents and column layout now match Node in all of them.New tests live in
test/js/node/console/console.test.ts; 6 of the 8 fail onmainwith theTypeErrorand pass with the fix.Before / after