Bun.FileSystemRouter: expose the assetPrefix getter on the prototype - #33433
Bun.FileSystemRouter: expose the assetPrefix getter on the prototype#33433robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 16 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 10:32 AM PT - Jul 6th, 2026
❌ @robobun, your commit 3e42223 has some failures in 🧪 To try this PR locally: bunx bun-pr 33433That installs a local version of the PR into your bun-33433 --bun |
There was a problem hiding this comment.
LGTM — wires up an already-implemented getter that was missing from the prototype definition.
Extended reasoning...
Overview
This PR exposes assetPrefix as a readable prototype accessor on Bun.FileSystemRouter. The Rust getter get_asset_prefix has existed since the class was introduced but was never listed in filesystem_router.classes.ts, so codegen never emitted the accessor and the function was dead code. Three files touched: a 4-line addition to the .classes.ts proto block (identical shape to the neighboring origin/style getters), a small tweak to the getter body to return "" instead of null when unset, and two new tests.
Security risks
None. This is a read-only accessor exposing a value the user themselves passed into the constructor. No new input parsing, no allocation changes, no new codepaths reachable from untrusted data.
Level of scrutiny
Low. The .classes.ts addition is purely mechanical and follows the exact pattern of the two getters directly above it. The Rust change touches only a previously-unreachable function and mirrors the existing get_origin pattern (.leak() on a BackRef<RefString> → zs_to_js). cache: true is safe because asset_prefix is set once at construction and never mutated by reload() — the added test verifies this.
Other factors
- The
null→""change is well-reasoned:packages/bun-types/bun.d.tsdeclaresreadonly assetPrefix: string(notstring | null), and""is exactly whatget_script_srcapplies when no prefix is set, so the reported value now matches both the type and the observable behavior inMatchedRoute.src. - Tests are placed in the existing test file, cover the set/unset/empty-string cases, verify the accessor is on the prototype (not own), and confirm it survives
reload(). - No prior human reviews or outstanding comments; bug-hunting system found nothing.
|
Status: the diff is green. The remaining red is CI infrastructure, not this change. Build 69028 (sha The one failure is The 3 Both symptoms point at the same thing: the macOS agent pool is currently not picking up work. Every build in the recent range (69153 through 69163, across unrelated branches) has its I'm deliberately not pushing a Nothing in any annotation mentions Earlier red on build 68819 (resolved)The first run of this PR was red on That was tree-wide rather than specific to this branch (the same annotation was on builds 68820 through 68825, six other unrelated branches). Three assertions in the |
getAssetPrefix has existed since the API was introduced but was never listed in the class definition, so the property was missing at runtime and reads returned undefined despite bun-types declaring it. Report the empty string when no prefix is configured: an omitted assetPrefix and assetPrefix: "" collapse to the same internal state, and the empty string is the prefix the router applies to MatchedRoute.src.
5361765 to
3e42223
Compare
There was a problem hiding this comment.
LGTM — wires up an already-implemented getter that was missing from the codegen proto list.
Extended reasoning...
Overview
This PR exposes Bun.FileSystemRouter.prototype.assetPrefix, a getter that has been implemented in Rust since the class was introduced but was never listed in filesystem_router.classes.ts's proto block, so codegen never emitted the accessor. The change is three parts: (1) a 4-line addition to .classes.ts mirroring the adjacent origin/style entries; (2) a small refactor of get_asset_prefix to return "" instead of null when unset, matching the declared readonly assetPrefix: string type in bun-types and the actual prefix applied to MatchedRoute.src; (3) two new tests in the existing filesystem_router.test.ts.
Security risks
None. This is a read-only accessor returning a string the user themselves passed to the constructor. No new input parsing, no allocation changes, no path handling.
Level of scrutiny
Low. The Rust change is a trivial refactor of dead code into a match expression with a different fallback (b"" vs JSValue::NULL) — no new memory management, no new JS re-entry points, and zs_to_js on an empty static slice is well-trodden. The .classes.ts addition is byte-for-byte the same shape as origin two entries above. cache: true is safe because reload() doesn't touch this.asset_prefix (only router/arena), and the added test explicitly covers the post-reload() value.
Other factors
- The bug hunting system found no issues.
- The new tests are placed correctly in the existing test file, follow harness conventions, and cover both the set and unset cases plus the prototype descriptor shape.
- CI ran green for the touched test file on all lanes including ASAN and Windows; the only red is a tree-wide
cookie-map.test.tsassertion mismatch unrelated to this diff (since fixed on main by #33425 per the recent-commits log). - No prior human or bot reviews to address.
|
Note for the rebase: this branch conflicts because #35002 (merged after this PR was opened) deleted The other half of the same declaration/runtime survey, |
Bun.FileSystemRouter.prototype.assetPrefixdoes not exist at runtime. The constructor acceptsassetPrefix,bun-typesdeclaresreadonly assetPrefix: string, and the prefix is demonstrably applied tomatch(...).src, but reading it back off the router returnsundefined.Repro
Before:
Cause
getAssetPrefixhas been implemented sinceBun.FileSystemRouterwas introduced in d21aee5, but it was never listed inprotoinfilesystem_router.classes.ts, so the codegen never emitted the accessor. The getter was unreachable dead code, and the only place a consumer can learn the prefix back off the router reportedundefined.Fix
Add
assetPrefixto the class definition'sproto, alongside the existingorigin/stylegetters (samecache: trueshape;reload()preserves the configured prefix, so the cached value stays correct).The getter now reports the empty string rather than
nullwhen no prefix is configured. An omittedassetPrefixandassetPrefix: ""already collapse to the same internal state (get_truthy+is_empty()both leave it unset), and the empty string is the prefix the router actually applies toMatchedRoute.src, so this keeps the declaredreadonly assetPrefix: stringtype honest.originkeeps returningnullwhen absent, which is unchanged.After:
Verification
Two tests added to
test/js/bun/util/filesystem_router.test.ts: the accessor exists on the prototype and returns the configured prefix (including acrossreload(), and tied to the prefix applied tosrc), and it is""when unset or set to"".Both fail on the released binary and pass with the change; the file's other 27 tests still pass.