Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 5 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:43 AM PT - Aug 15th, 2026
❌ @robobun, your commit 8d5d46d has some failures in 🧪 To try this PR locally: bunx bun-pr 38737That installs a local version of the PR into your bun-38737 --bun |
|
Status: ready for review; the diff is green. Two full CI runs of this head's diff (builds 97015 on d2d047c and 97499 on 8d5d46d, an empty retrigger) each passed 177 of 179 jobs. The two remaining jobs in each run are the macOS 14 aarch64 test lane and its automatic retry, which have not been able to get an agent from the Reproduced with the released binary using the two new dev-server tests in |
b46903b to
88ebc56
Compare
The dev server does not bundle runtime.js. Modules it serves import their lowering helpers from "bun:wrap", which the HMR runtime registers as a synthetic module built from a hand-written list of eight helpers. The list was never updated for standard decorator lowering, so a module containing a TC39 decorator or an accessor field fails to evaluate with "import_bun_wrap.__decoratorStart is not a function" in dev mode while the same source works in bun build and bun run. Register the runtime.bun.js module namespace as "bun:wrap" instead, which is the same module bun run serves for that specifier, so the synthetic module exposes every helper the transpiler can emit without a list to keep in sync. The unused ambient "bun:wrap" declaration that mirrored the old list is removed.
There was a problem hiding this comment.
LGTM — replacing the hand-maintained helper list with the full runtime.bun namespace eliminates the drift that broke standard decorators in the dev server.
What was reviewed:
registerSynthetic("bun:wrap", runtimeHelpers)flows throughrequire→toCommonJS, which iteratesgetOwnPropertyNameson the bundled namespace object — same mechanism as before, strict superset of exports.- Confirmed nothing in
src/runtime/bake/imports from"bun:wrap", so the removed ambientdeclare modulewas dead. - New tests exercise all nine decorator/private helpers with exact-value assertions across dev/prod browser and dev server routes; the Windows CI failures (
uv_os_getppid,uv_translate_sys_error) are in unrelated Rust files this PR does not touch.
Extended reasoning...
Overview
This PR fixes TypeError: import_bun_wrap.__decoratorStart is not a function when using TC39 standard decorators, accessor fields, or private members of decorated classes in the Bake dev server. The transpiler lowers these into calls to helpers imported from "bun:wrap", but the HMR runtime's synthetic bun:wrap module was built from a hand-written list of 8 helpers that had drifted from runtime.js (which now exports 30). The fix replaces import { __name, __legacyDecorateClassTS, ... } from "../../runtime.bun" + an 8-key object literal with import * as runtimeHelpers from "../../runtime.bun" and registerSynthetic("bun:wrap", runtimeHelpers). It also removes the now-dead ambient declare module "bun:wrap" from bake.private.d.ts and adds three tests (dev+prod browser, dev server route) exercising all nine previously-missing helpers.
Security risks
None. This is a build-time change to which JavaScript helper functions are bundled into the dev-server HMR runtime and exposed under an internal module specifier. No user input, no auth/crypto/permissions, no network or filesystem surface.
Level of scrutiny
Low-to-medium. The production code change is ~20 lines, almost entirely deletions, in a single well-understood location. The mechanism (registerSynthetic → registry lookup → toCommonJS over the exports object) is unchanged; only the input object grew from 8 keys to 30. I verified runtime.bun.js re-exports * from "./runtime" and that runtime.js defines all nine helpers the tests exercise. I also grepped src/runtime/bake/ for any from "bun:wrap" imports to confirm the removed .d.ts declaration was unused.
Other factors
- The fix follows REVIEW.md's "one source of truth" principle: rather than adding the 9 missing names to the hand-maintained list (which would drift again), it derives the synthetic module directly from
runtime.bun.js— the same filebun runserves for this specifier viaHardcodedModule::BunWrap. - The PR body documents the ~4.4 KB size cost per dev runtime and justifies it against the alternative of a 26-name explicit list (only 1.1 KB smaller, and reintroduces the maintenance burden).
- The comment-cop bot's feedback about a long justification comment was addressed in 262b578 (comment reverted to the original two lines).
- The Windows CI build failures reference
uv_os_getppid/uv_translate_sys_errorinipc_host.rs,node_cluster_binding.rs, andnode_util_binding.rs— none of which this PR touches; those are unrelated to this TypeScript-only change. - Tests assert exact sorted decorator-application order and computed values (not just "doesn't throw"), and cover both the client (
bake.client.js) and server (bake.server.js) HMR runtimes.
262b578 to
d2d047c
Compare
There was a problem hiding this comment.
LGTM — replacing the hand-maintained 8-helper list with the full runtime.bun namespace is the right fix and eliminates the drift class.
What was reviewed:
- Confirmed
runtime.bun.jsre-exports all ofruntime.js(including__decoratorStart/__decorateElement/__privateAddetc.), so the namespace is a strict superset of the old object. - Traced
registerSynthetic→loadModuleSync→toCommonJS: the bundled namespace object works identically to the old literal forhmr.require("bun:wrap"). - Checked
__requireis safely absent (dev-server disablesauto_polyfill_require; the existing "using runtime import" test pinsrequire→hmr.require). - Tests exercise all nine decorator/private helpers with exact-value assertions across dev browser, prod browser, and dev server route;
minimalFrameworkalready exported from the harness.
Extended reasoning...
Overview
The PR fixes TypeError: import_bun_wrap.__decoratorStart is not a function when using TC39 standard decorators or accessor fields in the Bake dev server. The root cause: hmr-module.ts registered a synthetic "bun:wrap" module from a hand-written 8-entry object that was never updated when the ES decorator helpers were added to runtime.js. The fix replaces the named-import list + object literal with import * as runtimeHelpers from "../../runtime.bun" and registers the namespace directly. The now-redundant declare module "bun:wrap" ambient type (a third stale copy of the list) is removed. Net: -20 src lines, +65 test lines.
Security risks
None. This is dev-server-only glue code that decides which transpiler runtime helpers are reachable via hmr.require("bun:wrap"). No user input parsing, no auth, no crypto, no filesystem/network surface. The exported helpers are the same ones bun run and bun build already expose under this specifier.
Level of scrutiny
Low-to-medium. The src change is a mechanical simplification (8 named imports → 1 namespace import) whose output is a strict superset of before. I verified: runtime.bun.js does export * from "./runtime" plus Bun-flavored __using/__callDispose, so all 28 runtime.js exports flow through; registerSynthetic just stores the object as module.exports and the require path runs it through toCommonJS (which iterates Object.getOwnPropertyNames) — identical behavior to the old plain object. The PR body cross-checked the generated bake.client.js/bake.server.js __export block against Imports::ALL and accounted for the one intentional gap (__require, unused because auto_polyfill_require is off for dev-server output). The 4.4 KB size increase is acknowledged and weighed against re-introducing a list to keep in sync.
Other factors
- No CODEOWNERS entries cover
src/runtime/bake/or the test file. - The comment-cop bot flagged an over-long code comment on an intermediate commit; the author trimmed it back to the original two lines (threads resolved).
- Test quality is solid:
standardDecoratorsSourcedeliberately triggers all nine helper imports (class/static-field/private-field/accessor/method/getter/private-method decorators +#p in o), asserts exact sorted context strings and computed values (not just "no throw"), and runs in three configurations (dev browser viadevAndProductionTest, prod browser, dev server route viaminimalFramework). The PR body demonstrates the dev tests fail on the released binary and pass with the fix, while the production test passes either way — satisfying the "fails for the right reason" bar. - This addresses the bug class rather than the symptom: any future helper added to
runtime.jsis automatically available in the dev server without a matching edit here.
There was a problem hiding this comment.
LGTM — replaces the hand-maintained eight-helper bun:wrap list with the full runtime.bun namespace, so the dev server exposes the same set bun run does.
What was reviewed:
- Confirmed
runtime.bun.jsre-exports all ofruntime.js(including__decoratorStart,__decorateElement, the__private*set) plus__using/__callDispose, so the namespace is a strict superset of the old list. - Nothing in
src/imports from"bun:wrap"as a TS module, so dropping the ambient declaration inbake.private.d.tsis safe. - The new tests exercise all nine standard-decorator helpers on both client (dev+prod) and server (dev,
minimalFramework), assert exact values, and follow the bake-harness conventions;minimalFrameworkis exported frombake-harness.ts.
Extended reasoning...
Overview
The PR fixes standard (TC39) decorators and accessor fields being unusable under the Bun dev server / Bake HMR runtime. The transpiler lowers those constructs to helpers imported from "bun:wrap", but the dev server's synthetic bun:wrap module in src/runtime/bake/hmr-module.ts was a hand-written eight-entry object that predated the standard-decorator helpers, so __decoratorStart et al. were undefined at runtime. The fix swaps the named import + object literal for import * as runtimeHelpers from "../../runtime.bun" and registerSynthetic("bun:wrap", runtimeHelpers), deletes the now-unused ambient declare module "bun:wrap" from bake.private.d.ts, and adds two tests in test/bake/dev-and-prod.test.ts covering a decorated class that pulls in all nine helpers on the client (dev + production) and on the server (dev, via minimalFramework).
Security risks
None. This is dev-server bundler-runtime wiring — no user-input parsing, auth, crypto, or filesystem/network surface. The namespace object handed to registerSynthetic is the bundled export set of runtime.js, the same code bun run and bun build already ship for this specifier.
Level of scrutiny
Low-to-medium. The source change is a net simplification (−20 lines) that replaces a duplicated list with its single source of truth, matching the "one implementation, in the right place" guidance in REVIEW.md. runtime.bun.js re-exports runtime.js plus the Bun-flavored __using/__callDispose, so the new synthetic module is a strict superset of the old one — no previously-available helper is dropped. Because hmr-module.ts is bundled by Bun.build in bake-codegen.ts, the namespace import compiles to a plain __export-populated object, not an exotic module-namespace object, so registerSynthetic sees the same shape it did before. The one intentionally-absent name, __require, is not emitted for dev-server output (auto_polyfill_require off; require prints as hmr.require), and the pre-existing "using runtime import" test already pins that.
Other factors
The tests are well-formed: they assert exact expected strings (decorator kinds/names and computed values), use await using for the client, and reuse emptyHtmlFile/minimalFramework from the harness. The .d.ts deletion is dead code — a repo-wide grep found no from "bun:wrap" TS imports. The comment-cop bot's feedback about a long code comment was addressed (the registration comment is back to its original two lines). CI on the current head has 177/179 jobs green with the remainder being a macOS agent-availability issue, not a test failure. No CODEOWNERS entries cover these paths.
Problem
Bun.servewith an HTML route in development, or a Bake app), any module containing a TC39 standard decorator, or just an undecoratedaccessorfield, fails to evaluate:TypeError: import_bun_wrap.__decoratorStart is not a function. In the browser this is a runtime error overlay; in a server route it is a 500.bun buildandbun runhandle the same source."bun:wrap":__decoratorStart,__decorateElement,__runInitializers,__decoratorMetadata,__privateAdd,__privateGet,__privateSet,__privateIn,__privateMethod(src/js_parser/lower/lower_decorators.rs). In dev-server output that import becomeshmr.require("bun:wrap")(src/bundler/linker_context/convertStmtsForChunkForDevServer.rs:123), so the only implementation of"bun:wrap"is the synthetic module the HMR runtime registers.src/runtime/bake/hmr-module.ts:949built that module from a hand-written object of eight helpers (__name, the three legacy TS decorator helpers,__using,__callDispose, the two React Compiler sentinels). The ES decorator helpers were added toruntime.jsand to the parser's import table (Imports::ALLinsrc/ast/runtime.rs) in feat(transpiler): implement TC39 standard ES decorators lowering #26436 but never to this list, so the list has been incomplete since standard decorators shipped.Fix
hmr-module.tsimportsruntime.bun.jsas a namespace and registers that namespace as"bun:wrap", replacing the eight-name object and its matching import list.runtime.bun.jsisruntime.jsplus the Bun-flavoredusinghelpers, and it is already whatbun runserves forimport ... from "bun:wrap"(HardcodedModule::BunWrapinsrc/runtime/jsc_hooks.rsreturns the bundledruntime.out.jsof that file). The dev server now hands out the same 30 exports, which cover every name inImports::ALLexcept__require.__requireis only imported whenauto_polyfill_requireis set, andParseTask.rsturns that off for dev-server output (requireis printed ashmr.requirethere; the existing "using runtime import" test pins this). Helpers added toruntime.jslater are picked up with no list to keep in sync.declare module "bun:wrap"inbake.private.d.tswas a third copy of the old list; nothing in the bake sources imports from"bun:wrap", so it is removed.bake.client.js48.5 KB to 53.0 KB,bake.server.js11.0 KB to 15.4 KB). An explicit list of the 26 helpers would have been 3.3 KB; the extra 1.1 KB is__toESM,__toCommonJS,__commonJSand__esm, which the parser never emits butbun runandbun buildalso expose under this specifier.test/bake/dev-and-prod.test.ts: a module whose decorated class imports all nine helpers, loaded through an HTML route in the browser (dev and production) and through aminimalFrameworkserver route (dev). Without the src change the two dev tests fail with the error above and the production test passes; with it all ofdev-and-prod.test.ts(15),test/bake/dev/esm.test.tsandtest/bake/dev/bundle.test.ts(38) pass on the debug build.tscon the bake project reports nothing forhmr-module.tsorbake.private.d.ts.Background
runtime.jsholds the helper functions the transpiler's lowerings call (__toESM,__legacyDecorateClassTS,__decorateElement, ...). The parser records which helpers a file uses and emits a singleimport { ... } from "bun:wrap"for them;Imports::ALLis the table of names it can emit.bun buildresolves that import toruntime.jsand bundles the used helpers in;bun runresolves it to a module built fromruntime.bun.js.internal_bake_dev) and evaluates them with a small module loader, the HMR runtime (hmr-module.ts, built intobake.client.jsandbake.server.jsbysrc/codegen/bake-codegen.ts). Builtins and"bun:wrap"are not bundled in that format; they are looked up in the loader's registry at evaluation time, andregisterSyntheticis how the runtime pre-populates that registry with modules it implements itself.Export set of the generated synthetic module vs
Imports::ALLCross-checking the
__export(exports_runtime_bun, {...})block in the regeneratedbake.client.js/bake.server.jsagainstImports::ALLinsrc/ast/runtime.rs:The 30 names are the same set
build/debug/codegen/runtime.out.js(thebun runmodule forbun:wrap) exports.Failure without the fix, from the new tests on the released binary: