Conversation
|
Warning Review limit reached
Next review available in: 22 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 (3)
Comment |
|
Updated 4:15 PM PT - Jul 6th, 2026
❌ @robobun, your commit 114ec8f has some failures in 🧪 To try this PR locally: bunx bun-pr 33471That installs a local version of the PR into your bun-33471 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR does not fix #23041, so I'm leaving #23041 is about The two bugs look alike ( |
There was a problem hiding this comment.
I didn't find any issues — the approach mirrors Node's lib/internal/perf/performance_entry.js and the test is solid — but since this adds a new internal module and wires up a prototype chain against the native PerformanceEntry, it's worth a quick human look.
Extended reasoning...
Overview
This PR fixes Node.js compatibility for PerformanceObserver entries of type net and http. Previously stopPerf() in src/js/internal/shared.ts emitted plain object literals, so entry instanceof PerformanceEntry was false and entry.toJSON() threw. The fix adds a new internal module src/js/internal/perf_hooks/performance_entry.ts (~86 lines) defining a PerformanceNodeEntry class whose prototype chain is wired to the native PerformanceEntry via Object.setPrototypeOf, mirroring Node's own lib/internal/perf/performance_entry.js. stopPerf() now lazy-requires and constructs these instead of literals. A ~65-line test in test/js/node/perf_hooks/perf_hooks.test.ts exercises the full flow with a real HTTP server/client and asserts instanceof, toJSON(), JSON.stringify round-tripping, and detail payloads.
Security risks
None. This is pure JS shaping of already-emitted metrics objects; no new inputs are parsed, no filesystem/network/auth surface is touched. The lazy require follows the existing pattern used for monitorEventLoopDelay in the same directory.
Level of scrutiny
Medium. It's a focused, well-tested Node-compat fix with clear precedent in the file next door ($toClass(PerformanceNodeTiming, ..., PerformanceEntry) in perf_hooks.ts), and the PR description explains why setPrototypeOf is used instead of extends (native PerformanceEntry has no callable constructor). That said, it introduces a new internal module and hand-wires a prototype chain against a native WebCore class, which changes user-observable instanceof/constructor.name/enumerability semantics — the kind of Node/Web compat surface that CLAUDE.md flags for a human sanity check.
Other factors
- No CODEOWNERS match for the touched paths.
- The bug-hunting system found no issues.
- The lazy-require +
export default { ... }pattern matches the siblinginternal/perf_hooks/monitorEventLoopDelaymodule, so the module registry/bundler should pick it up without additional registration. - The test uses
Promise.withResolvers, wires error handlers to reject, and cleans up the observer/server infinally— it follows the harness conventions well. - No prior human or bot reviews to consider; CI was still building at time of review.
|
Good call on the
The fix: I had marked The one remaining divergence is On the prototype chain, the one structural difference is unavoidable: Node splits |
There was a problem hiding this comment.
LGTM — focused Node compat fix that swaps the object-literal entries for a proper PerformanceNodeEntry mirroring Node's own internal/perf/performance_entry.js, with solid test coverage.
Extended reasoning...
Overview
This PR fixes PerformanceObserver entries for the Node-only net/http/dns entry types. Previously stopPerf() in src/js/internal/shared.ts built each entry as a plain object literal, so entry instanceof PerformanceEntry was false and entry.toJSON() threw. The fix adds src/js/internal/perf_hooks/performance_entry.ts (a new ~85-line internal module) defining a PerformanceNodeEntry class with symbol-backed storage, prototype accessors, toJSON(), and a util.inspect.custom hook, then wires its prototype chain to the native PerformanceEntry via setPrototypeOf (since the native constructor is not callable). stopPerf() now constructs one of these instead of an object literal — a mechanical five-argument swap. A new test in test/js/node/perf_hooks/perf_hooks.test.ts exercises a real net + http observation and asserts the full observable shape (instanceof, prototype chain, toJSON(), JSON.stringify round-trip, own-keys empty, four enumerable prototype keys, and the detail payloads).
Security risks
None. This is pure-JS Node.js compatibility code that shapes diagnostic/telemetry objects handed to user PerformanceObserver callbacks. No auth, crypto, filesystem, network parsing, or untrusted-input validation is touched. The __proto__: null on the descriptor object and symbol-keyed storage are defensive but not security-critical here.
Level of scrutiny
Low-to-moderate. It's a self-contained Node compat fix in built-in JS with no native code changes. The implementation is a close port of Node's own lib/internal/perf/performance_entry.js (symbol slots, enumerable base accessors, non-enumerable detail/toJSON, hand-wired prototype chain), and the new file lives alongside the existing internal/perf_hooks/monitorEventLoopDelay.ts following the same lazy-require pattern already used in this codebase. The shared.ts change is a mechanical object-literal → constructor-call swap with identical argument semantics.
Other factors
- The new test is well-constructed:
port: 0, error events wired to reject, awaits the actual observer callback rather than sleeping, and cleans up viafinally. It asserts strong invariants (exact key sets, prototype identity, JSON round-trip) rather than loose shape checks. - No CODEOWNERS entries cover the touched paths.
- The bug-hunting system found no issues.
- The PR description explicitly verified the vendored
test-net-perf_hooks.jsandtest-http-perf_hooks.jsstill pass, and the author correctly declined to auto-close #23041 (different root cause).
The 'net' and 'http' entries handed to a PerformanceObserver were plain object literals, so entry.toJSON() threw and entry instanceof PerformanceEntry was false. Build them as PerformanceNodeEntry, whose prototype chains to the native PerformanceEntry, matching Node.
1a09403 to
81a8ad8
Compare
There was a problem hiding this comment.
No bugs found — the implementation looks correct and well-tested, but this adds a new internal module with prototype-chain wiring against the native PerformanceEntry, and you've flagged an open Symbol.toStringTag tradeoff for a maintainer to weigh in on, so worth a human look.
Extended reasoning...
Overview
This PR fixes Node.js compat for perf_hooks observer entries: the net/http/dns entries dispatched via the JS-side stopPerf() registry in internal/shared.ts were plain object literals, so entry instanceof PerformanceEntry was false and entry.toJSON() threw. The fix adds a new ~86-line internal module src/js/internal/perf_hooks/performance_entry.ts defining PerformanceNodeEntry (symbol-keyed storage, accessors, toJSON(), inspect.custom, prototype chained to the native PerformanceEntry via setPrototypeOf), swaps the object literal in stopPerf() for a lazy-required constructor call, and adds a ~75-line test asserting the full observable shape (instanceof, prototype chain, own keys, for...in enumerability, toJSON()/JSON.stringify round-trip, detail payloads).
Security risks
None. This is pure JS-side Node compat surface — no native code, no untrusted input parsing, no auth/crypto/permissions. The new module only reads globalThis.PerformanceEntry at load time and lazy-requires node:util for inspect.
Level of scrutiny
Medium. It's a well-scoped compat fix that closely mirrors Node's own lib/internal/perf/performance_entry.js, with thorough verification against Node v26.3.0 documented in the thread. But it's ~170 lines of new code touching prototype-chain wiring against a native WebCore class, and Node-compat shape details (enumerability, own vs. prototype accessors, toStringTag) are exactly the kind of thing that benefits from a second pair of eyes.
Other factors
- The author explicitly flags one deliberate divergence from Node (
Object.prototype.toString.call(entry)reports[object PerformanceEntry]vs. Node's[object Object]) and invites a maintainer to flip it if they disagree — that's a small design call someone should ack. - The lazy-require pattern and module placement (
src/js/internal/perf_hooks/, alongside the existingmonitorEventLoopDelay.ts) follow existing conventions; no manual registry update appears needed. - Test coverage is strong: wires error events to reject, uses
port: 0, cleans up infinally, and asserts bothObject.keys(entry)and thefor...inkey set to lock in the enumerability fix from the follow-up commit. - CI build #68983 was still in progress at review time.
81a8ad8 to
dfa60d1
Compare
There was a problem hiding this comment.
LGTM — focused Node-compat fix that swaps the plain-object entries for a proper PerformanceNodeEntry chained to the native prototype, with a solid test locking down the observable shape.
Extended reasoning...
Overview
This PR touches three files: it adds src/js/internal/perf_hooks/performance_entry.ts (a new ~85-line internal module defining PerformanceNodeEntry), makes a mechanical change in src/js/internal/shared.ts to construct one of those instead of an object literal in stopPerf(), and extends test/js/node/perf_hooks/perf_hooks.test.ts with a test that observes net/http entries and asserts instanceof PerformanceEntry, prototype chain, enumerability, toJSON(), and detail payloads. The new module mirrors Node's lib/internal/perf/performance_entry.js: symbol-backed storage behind accessors, toJSON(), a util.inspect.custom, and a hand-wired prototype chain via setPrototypeOf (since the native PerformanceEntry constructor is not callable).
Security risks
None. This is pure built-in JS in the Node-compat layer — no auth, crypto, filesystem, or untrusted-input parsing. The only external interaction is the test's loopback HTTP request on port: 0.
Level of scrutiny
Low-to-moderate. The shared.ts edit is a straight refactor: the same five values that populated the object literal are now passed positionally to createPerformanceNodeEntry in the same order the constructor consumes them. The new class is small and closely tracks upstream Node; the prototype-wiring approach matches the existing internal/perf_hooks/monitorEventLoopDelay.ts sibling's placement and the lazy-require idiom used throughout src/js. No CODEOWNER paths are touched.
Other factors
The author already ran a property-by-property diff against Node v26.3.0 (posted in the thread), caught and fixed an enumerability mismatch in a follow-up commit, and documented the one intentional remaining divergence (Symbol.toStringTag inherited from the native prototype) with a reasonable rationale. The test is well-constructed per repo conventions — awaits real conditions, wires error events to reject, cleans up in finally, and asserts exact key sets so the shape can't silently drift. The bug-hunting pass found nothing. This is a clean, self-contained compatibility improvement.
detail and toJSON are non-enumerable in Node, so a for...in over an entry yields name/entryType/startTime/duration and nothing else.
dfa60d1 to
114ec8f
Compare
There was a problem hiding this comment.
LGTM — clean Node-compat fix that mirrors Node's own PerformanceNodeEntry, with a thorough test.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap: net/http entries delivered to a PerformanceObserver were plain object literals rather than real PerformanceEntry instances, so entry instanceof PerformanceEntry was false and entry.toJSON() threw. It adds a new internal module src/js/internal/perf_hooks/performance_entry.ts defining PerformanceNodeEntry (symbol-backed fields, accessor getters, toJSON(), custom inspect, prototype chain wired to the native PerformanceEntry via setPrototypeOf), swaps the object literal in stopPerf() (src/js/internal/shared.ts) for a lazily-required factory call, and adds a comprehensive test in test/js/node/perf_hooks/perf_hooks.test.ts.
Security risks
None. This only changes the prototype/shape of performance-timing entry objects handed to user observers. No parsing of untrusted input, no auth, no filesystem/network surface beyond the existing test's local loopback server.
Level of scrutiny
Low-to-moderate. This is a self-contained Node-compat shim in built-in JS that only executes when a PerformanceObserver is registered for net/http/dns (the new module is lazy-required inside stopPerf(), so there's no startup cost). The implementation is a close transcription of Node's lib/internal/perf/performance_entry.js — the PR description cites the upstream source and explains why extends PerformanceEntry can't be used (native constructor throws) and why setPrototypeOf is used instead, which is the same technique Node uses. No CODEOWNERS cover these paths.
Other factors
- The bug-hunting system found no issues.
- The new test is hermetic (
port: 0, loopback,finallycleanup, awaits the observer callback rather than sleeping) and asserts the full Node-observable contract:instanceof,constructor.name, prototype parent,toJSON()shape,JSON.stringifyround-trip, own-keys empty,for...inenumerating exactly the four base accessors, plus per-entry-typedetailpayloads. - Enumerability handling (four base accessors enumerable,
detail/toJSONnot) matches Node and is verified by the test. - The
shared.tschange is a minimal mechanical rewrite of the existing object literal into a factory call with identical arguments; thedetailmerge logic is unchanged. - No outstanding reviewer comments and no prior reviews on the timeline.
|
One more verification worth recording, since a new built-in JS module can behave differently once That matters for one assertion in particular. The test checks |
CI status: the diff is green; two lanes are failing on infrastructureThis is ready for review. The red X comes from two That is the agent failing to fetch the compiled binary, before any test code runs. The artifact it times out on, Everything else passes:
It reproduced identically on two independent builds (#69012, #69146), same lane, same 120s timeout, so it is not transient flake I can shake off by re-running. A three-file change to built-in JS has no way to affect whether a macOS agent can download a 70 MB artifact in time, so I have stopped re-triggering rather than burn CI capacity on it. Happy to rebase or re-run if someone with access to the |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, 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
The
netandhttpentries Bun hands to aPerformanceObserverare plain object literals. They have notoJSON(), andentry instanceof PerformanceEntryis false. In Node every observed entry is a realPerformanceEntry. Serializing what you observe is the whole point of observing it, so an observer that callsentry.toJSON()throws, and because observer callbacks swallow exceptions the metrics just disappear.Cause
The native (WebCore)
PerformanceObserveronly knowsmark/measure/resource, so Bun routes the Node-only entry types through a small JS registry ininternal/shared.ts.stopPerf()built each entry as an object literal:Fix
Add
internal/perf_hooks/performance_entry, which mirrorslib/internal/perf/performance_entry.js: aPerformanceNodeEntryholding its fields on symbols behindname/entryType/startTime/duration/detailaccessors, withtoJSON()and autil.inspect.customthat prints the serialized entry. The nativePerformanceEntryhas no public constructor, soextendswould makesuper()throw; the prototype chain is wired up withsetPrototypeOf, the same way Node does it for its own internal entries.stopPerf()now constructs one of these.Result matches Node's observable shape:
instanceof PerformanceEntry,constructor.name === "PerformanceNodeEntry", own properties[], prototype parent=== PerformanceEntry.prototype, workingtoJSON()andJSON.stringify().performance.nodeTiminghas the same symptom (nodeTiming.namethrows the samePerformanceEntry.name getter can only be used on instances of PerformanceEntry) but a different cause:$toClassreplaces a class declaration's prototype object with an empty one, dropping the class body's accessors. That is already fixed at the$toClasslevel by #32163, so it is left alone here.Verification
test/js/node/perf_hooks/perf_hooks.test.tsgrows a test that observesnet+http, asserts every entry is aPerformanceEntrywith atoJSON()whose output round-trips throughJSON.stringify, and checks thedetailpayloads. It fails onmainand passes with this change. The vendoredtest-net-perf_hooks.jsandtest-http-perf_hooks.jsstill pass.