Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (9)
Comment |
|
Updated 4:33 AM PT - Sep 16th, 2026
✅ @robobun, your commit e082ddb24d59004b27c9bd1d78f48a65c90370d5 passed in 🧪 To try this PR locally: bunx bun-pr 42904That installs a local version of the PR into your bun-42904 --bun |
|
Status Reproduced on 1.4.3 (09bb546) with With this branch Reviewed: this PR should stay open because main ignores a documented option without a message, and the only other fix (the throw in #39488) is stalled and leaves PR: #42904 |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the hoisted :/// checks in src/runtime/server/server_body.rs for { dir, style } keys — every key they now reject would also fail the new validate_prefix call a few lines below, so they only change the error message, not which configs are accepted. I also traced prefix through production.rs into the shared Type field, so the build path reads it via the same scan code as the dev server.
Extended reasoning...
Five confirmed findings are posted inline, so the review body only records what else was examined. The hoisted : and // checks in server_body.rs apply to styled directory routes for the first time, but validate_prefix (called right after the /* strip) independently rejects : and ./empty-segment prefixes, so no config that would have passed the new validator is newly rejected by the hoist; the only observable difference is which message a bad key gets. The production.rs hunk is a one-line field population consumed by the same FrameworkRouter::scan path, with no separate consumer to audit. Neither of these is a bug, and the inline findings already establish that a human should review before merge.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/runtime/bake/FrameworkRouter.rs— Users who mount a second router on a prefix under a root router that has a catch-all get the root catch-all page for every dynamic URL of the prefixed router, silently. match_slow at FrameworkRouter.rs:1290 walks dynamic_routes in scan order and returns the first match, and scan_all inserts the root router first, so/:rest*frompages/[...rest].tsxclaims/docs/foobeforedocs/[slug].tsxis tried. Fix: pick dynamic candidates by longest literal prefix (or try the router whose prefix matches the path first) so a prefixed router owns its own URL subtree for dynamic routes too.Extended reasoning...
Config: fileSystemRouterTypes [ { root: 'pages', prefix: '/' }, { root: 'docs', prefix: '/docs' } ] or routes { '/': {dir,style}, '/docs/': {dir,style} }. pages has [...rest].tsx, docs has [slug].tsx. scan_all (FrameworkRouter.rs:262) scans type 0 first, so dynamic_routes holds
/:rest*at index 0 and/docs/:slugat index 1. Request /docs/foo: static_routes miss at 1286. Loop at 1290 tests/:rest*first; CatchAll matches any path and returns type 0's route. The docs router's [slug] page is never served for any URL. The dismissing finder called it pre-existing, but the population is new: prefixed multi-router configs only become startable with this PR (on main both routers sat at / and panicked on the shared index route), and the PR's stated purpose is to isolate routers by prefix; the isolation holds for static routes only. Same shadowing for[[...rest]]optional catch-alls and for a root[slug]versus/docs/[id]. Remedy: order dynamic candidates so longer literal prefixes win, or match per router by prefix first.Verification: pre-existing (acknowledged in diff: PR description "The order of dynamic routes across routers is the scan order (#40731)" — accurate; the limitation is the base's, not newly introduced). Trigger: a root router (prefix
/) containing a catch-all page ([...rest].tsx) plus a second router mounted on a prefix whose dynamic pages must win by specificity. Mechanism verified in… | normal —… -
🟣
src/runtime/bake/DevServer.rs— Users with two prefixed routers that share one serverEntryPoint get stale pages from all but the last router after editing that entry file, until they restart the dev server. route_lookup.put at DevServer.rs:969 is keyed by server_file only, so the second router's RouteIndex overwrites the first's, and the hot-update reader at incremental_graph.rs:1102 invalidates only the surviving root. Fix: key route_lookup so one server file can map to every router root that uses it (a list of RouteIndex per file, or a per-type entry), and push all of them into framework_routes_affected.Extended reasoning...
Every
{ dir, style }route shares entry_server 'bun-framework-react/server.tsx' (server_body.rs:866); custom frameworks commonly reuse one serverEntryPoint across fileSystemRouterTypes. In init, insert_stale_extra at DevServer.rs:933 returns the same ServerFileIndex for both types. route_lookup.put at 969 stores RouteIndex(i) with recurse=true; the second put replaces the first. User edits the server entry. incremental_graph.rs:1098-1115 looks the file up once and pushes only the last router's root into framework_routes_affected, so routes under the first router keep their old bundle. The dismissing finder called it pre-existing, but on main a two-router config with index files could not start (aliased-route panic), so no user could reach this overwrite; this PR makes multi-router the advertised configuration and its own app-options test uses two routers. Remedy: store multiple RouteIndex per server file and invalidate each.Verification: pre-existing (untouched line, but newly made the mainline configuration by this PR; its own test uses two prefixed routers sharing
./framework.ts). Trigger: twofileSystemRouterTypesentries (or two{ dir, style }routes, which all useentry_server: "bun-framework-react/server.tsx"at /home/claude/bun/src/runtime/server/server_body.rs:866) share one server entry, and that entry file is… -
🟣
src/runtime/bake/DevServer.rs— Users with two prefixed routers where the first root directory is missing get a wrong or out-of-range RouteIndex on hot reload, which can panic the dev server, where a single-router setup just skipped the router. Thecontinueat DevServer.rs:929 skips the types.push but not the enumerate index, so route_lookup at 969 records RouteIndex(i) for a type that is now at position i-1 intypes. Fix: index route_lookup by the position intypes(types.len() before the push), and report the missing root instead of skipping it.Extended reasoning...
Config: fileSystemRouterTypes [ { root: 'missing', prefix: '/' }, { root: 'docs', prefix: '/docs' } ]. read_dir_info_ignore_error at 927 returns None for 'missing',
continueat 929. For docs, i == 1 but types has one element, so its root route is RouteIndex(0) (FrameworkRouter.rs:174), while route_lookup maps its server file to RouteIndex(1). After scan, RouteIndex(1) is the first child node (thedocsprefix node) or, with no routes, out of range ofroutes. Editing the server entry pushes RouteIndex(1) into framework_routes_affected (incremental_graph.rs:1113); the consumer either invalidates the wrong subtree or indexes routes out of bounds. The dismissing finder called it pre-existing and tracked; the base has the samecontinue, but multi-router configs are only startable with this PR, and the PR's ignoreDirs guidance encourages nested roots that can be absent on a fresh checkout. Remedy: use types.len() as the index and surface the missing root as an error.Verification: pre-existing. Triggering condition: a framework with two or more
fileSystemRouterTypeswhere an earlier entry'srootdirectory does not exist, followed by any edit that reaches a later router'sserverEntryPoint(hot reload). Mechanism verified in /home/claude/bun/src/runtime/bake/DevServer.rs: the loop at 912-916 enumeratesframework.file_system_router_typesas(i, fsr); a missing root…
FrameworkRouter never read `fileSystemRouterTypes[n].prefix`, so every
router served its files at `/`. The same applied to the prefix that a
`{ dir, style }` route takes from its key.
`framework_router::Type` now holds the prefix. `scan` puts its segments
in front of every pattern. The static route keys, the dynamic patterns
and the route tree then hold the full URL, so the dev server matches on
it and `bun build --app` writes each page under it.
The prefix must be a fixed, normalized URL path. A route whose URL does
not fit in a path buffer is reported as a route error.
`Framework::from_js` iterated the `fileSystemRouterTypes` array where it meant the `ignoreDirs` array of one entry. The parsed list held "[object Object]" strings, so no directory was ever ignored. With prefixes this matters: a router for `pages` at `/` and a router for `pages/api` at `/api` both claim `/api/ping` unless the first one can skip `api`.
A prefix under `/_bun` collides with the dev server's own routes and with the asset directory of `bun build --app`. The build test is a todo: on main a custom framework aborts the build before any route renders (#32142).
With prefixes two routers can be told apart, and they can also still produce the same URL: the same prefix, or a router for `pages` at `/` beside a router for `pages/api` at `/api`. `insert` aborted on that with `TODO: propagate aliased route error`. It now returns `RouteCollision`, so the dev server prints both files and the router scanned first keeps the URL. The hunk is the one in #42836, unchanged. A client sends `" < > ^ ` { }` percent-encoded, so a prefix that holds one of them never matches a request. `validate_prefix` rejects them.
ffeb569 to
4016b1a
Compare
|
Reply to the review of ffeb569. The two inline findings are fixed in 4016b1a and answered in their threads. The three findings outside the diff: A catch-all in the Two routers that share one A missing root directory before another router. #42840 fixes that index (it takes the type index from |
There was a problem hiding this comment.
I re-reviewed the pushed commits and found no new bugs; both points from my earlier review are addressed in code (the cross-router alias panic now surfaces as a reported RouteCollision, and validate_prefix rejects the percent-encoded path set). A maintainer look is still worthwhile because the PR itself asks for a direction call (implement prefix here vs. the throw in #39488), and this now overlaps with #42836.
What was reviewed:
insert()reordering: the file pointer is written only after the URL-map insert succeeds, andfile_pageis never cleared anywhere insrc/runtime/bake, so the.expecton the aliased route's page holds.validate_prefixcall sites:bake_body.rs,server_body.rs({ dir, style }), and the JSFrameworkRouterconstructor all use the same predicate;/and the empty default tokenize to zero segments, so unprefixed routers are unchanged.- Hoisting the
:///checks inserver_body.rsruns them beforeStyle::from_js, so noStyleis created on those error paths. - Note: the description's "Not fixed here" paragraph still says the alias panic is left to #42836, but commit 4016b1a fixes it in this PR — the description is stale on that point.
Extended reasoning...
Overview
The diff makes Bake's file-system router honor prefix: framework_router::Type gains a prefix field populated at all three constructor sites (DevServer.rs, production.rs, the JS FrameworkRouter constructor), scan prepends the prefix segments as Part::Text to every parsed route, and a new Type::validate_prefix is applied from bake_body.rs, server_body.rs, and the JS constructor. insert() replaces the panic!("TODO: propagate aliased route error") with a RouteCollision report, and bake_body.rs fixes ignoreDirs reading from the wrong array. Tests cover multi-router prefixes, nested roots with ignoreDirs, alias reporting, an invalid-prefix matrix, a { dir, style } dev test, and unit tests for tree shape.
Security risks
The prefix is user config, not request data. validate_prefix rejects ./.. segments, backslash, and non-graphic bytes before the value is joined into a dist/ path by bun build --app, and the length guard in scan keeps prefix + rel_path under MAX_PATH_BYTES so the fixed-size pattern buffers used by effective_url_hash/PatternBuffer cannot overflow. The reserved /_bun check prevents shadowing the dev server's own routes. I did not find a path by which an accepted prefix escapes the output directory or exceeds a buffer.
Level of scrutiny
Moderate-to-high. This changes user-visible routing semantics across the dev server, production build, and Bun.serve directory routes, and it converts a previously-unreachable panic into a reported error whose test expectations depend on scan order. It is not a mechanical change, and the author explicitly requests a maintainer decision between implementing prefix and rejecting it (#39488). The overlap with #42836 (which the author says carries the same alias fix) is a coordination question a human should settle. For those reasons approval is not appropriate even though I found no defects.
Other factors
Both inline findings from my prior review are addressed by commits 4016b1a and ae7bfe0. I checked the insert() refactor for behavior drift: the old code wrote file_ptr before the map insert; the new code writes it only after success, and since nothing in src/runtime/bake ever clears file_page, routes present in the URL maps always have a page, so the .expect is a true invariant. Hoisting the :/// checks in server_body.rs happens before Style::from_js, so no Style value is leaked on those error paths. The bun build --app prefix assertion is a test.todo gated on #32142, meaning the production.rs path is compiled but not exercised by CI here. The PR description's "Not fixed here" section is stale about the alias panic.
|
The description is current now. I updated it after the push: the "Not fixed here" list no longer has the alias panic, and the Notes paragraph "Routers that claim the same URL" describes the |
Direction needed. This PR implements
prefix. #39488 throws'fileSystemRouterTypes[n].prefix' other than "/" is not supported yetinstead. Say so if the throw is preferred. The swap is about 14src/lines.Problem
fileSystemRouterTypes[n].prefixis documented inbake.d.ts, parsed, and never read. Every router serves at/: withprefix: "/docs",/docs/aboutis 404 and/aboutanswers. No release honored it. No user reported it.routes: { "/docs/*": { dir, style } }has the same fault. Its key goes into the same field.framework_router::Typehas no prefix field. The value thatbake_body.rs:909andserver_body.rs:848store never reaches the router.Fix
Typeholds the prefix.scan_innerputs its segments, asPart::Text, in front of each route file's parts. Every consumer reads the pattern, so none needs a special case.Type::validate_prefixthrows unless the prefix is a fixed, normalized URL path outside/_bun. A route whose URL does not fit inMAX_PATH_BYTESis a route error.pagesat/,pages/apiat/api).ignoreDirswas read from the wrong array and ignored nothing. Two routers on one URL are now a reported collision, not a panic (theinserthunk of bake: report route scan errors instead of aborting, and fail bun build --app on them #42836).test/bake/app-options.test.ts,framework-router.test.ts,dev/ssg-pages-router.test.ts. 8 tests fail on 1.4.3. Self-reviewed: 18 concerns, 16 addressed (Notes).Background
FrameworkRoutermaps route files to URLs for the Bake dev server andbun build --app. ATypeis onefileSystemRouterTypesentry.Parts. All types share thestatic_routesanddynamic_routesmaps, so the prefix must be in the key.Routenodes. The root layout now sits on the last prefix node.Notes
Demand. No issue, discussion or public config asks for a prefix other than
/. The same URLs are possible today with a nested directory:{ root: "site" }withsite/docs/about.tsgives the same responses and route tree as{ root: "routes", prefix: "/docs" }. So the choice is between two ways to stop the silent no-op: implement (this PR) or throw. The throw in #39488 does not cover{ dir, style }keys, which stay mounted at/. A full throw would also reject every{ dir, style }key other than/*.What the dev server does now. Routers
routesat/docsandapiat/api/v1/, both withindex.tsandabout.ts:On main the same config aborts with
panic: TODO: propagate aliased route error, because bothindex.tsfiles map to/.Routers that claim the same URL. The same prefix twice,
/withdocs/a.tsbeside/docswitha.ts, or nested roots withoutignoreDirs({ dir, style }keys have noignoreDirs). On main the nested config runs, with the inner router at/. With prefixes it would reach theTODO: propagate aliased route errorpanic ininsert. Soinsertnow returnsRouteCollision: the dev server printsMultiple pages matching the same route pattern is ambiguouswith both files, and the router scanned first keeps the URL. The hunk is copied from #42836 without a change, so either merge order is clean. #42836 also covers the test binding and the exit code ofbun build --app.Not fixed here.
bun build --app:production.rspasses the prefix, because the struct needs the field. CI cannot observe it. Only a custom framework can set a prefix, and on main every custom framework aborts the build withpanic: Runtime file not foundbefore a route renders (bun build --app panics with "Runtime file not found" for any custom Bake.Framework #32142, PR bake: fix "Runtime file not found" panic in production builds of custom frameworks #32143). With that panic patched out locally, the build wrotedist/docs/index.html,dist/docs/about/index.htmlanddist/api/v1/ping/index.html. The assertion is inapp-options.test.tsastest.todo./docs/with a trailing slash is 404, like/about/is forabout.tson main. bake: consolidate the open robobun dev-server, HMR runtime, production build and router fixes #39488 has a change that ignores one trailing slash.production.rsasks every router's server entry point forgetParamswhen any router has a dynamic route. The check is per build, not per router. It has no tracker yet, and bun build --app panics with "Runtime file not found" for any custom Bake.Framework #32142 hides it on main.{ dir, style }with a directory that cannot be opened), bake: serve routes behind symlinks, reload every route that shares a file #42813 (symlinks), bake: serve routes from a router root outside the project root instead of aborting or answering 404 #42828 (root outside the project).ignoreDirs. The fix is the hunk of closed #38234, which went into #39488.Framework::from_jsiterated thefileSystemRouterTypesarray where it meant theignoreDirsarray, so the list held[object Object]strings. The error message for a value that is not an array no longer mentions"*", whichignoreDirsnever accepted.Prefix rules. It must start with
/. It must be shorter thanMAX_PATH_BYTES. It can hold printable ASCII only, and none of? # \ : * " < > ^ ` { }. The last seven are the bytes a URL parser percent-encodes in a path, so a request never holds them raw.%is allowed:/my%20docsis the form a client sends. It cannot have a.or..segment. It cannot be under/_bun. A trailing/and empty segments are ignored. The dev server compares the prefix with the raw request path, andbun build --appjoins it into a file path underdist/. So a prefix with a query, a dot segment or a backslash either never matches or leavesdist/. For{ dir, style }keys the:parametersand//checks of{ dir }keys now apply too.Why the length check is in the scan.
EncodedPattern::effective_url_hashwrites a pattern into a stack buffer of2 * MAX_PATH_BYTES, andPatternBuffer(the build) holds a rendered pattern in onePathBuffer. Both were safe because a pattern was never longer than its file path. With a prefix that is no longer true, soscan_innerreportsThe URL of this route is too longwhen prefix plus file path reachesMAX_PATH_BYTES.If this lands, #39488 has to drop its
prefixthrow and the matching snapshot row.Suites run with the debug build.
test/bake/app-options.test.ts,test/bake/framework-router.test.ts,test/bake/dev/ssg-pages-router.test.ts,test/bake/dev/bundle.test.ts,test/bake/dev/esm.test.ts,test/bake/deinitialization.test.ts,test/bake/dev-and-prod.test.ts,test/bake/serve-plugins-dev-server.test.ts,test/js/bun/http/serve-directory-routes.test.ts,test/internal/source-lints/. Alsocargo clippy -p bun_runtime.Self-review. 18 concerns raised, 16 addressed. Most are about demand and direction, and the top of this description answers them. Addressed in code: the
ignoreDirsfix with its message and snapshot row, the nested-roots test, the/_bunrule, the build assertion astest.todo, the reported collision. Addressed in text: the scope of the build claim, the links to the other trackers. Not taken:no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/framework-router.test.ts