Repository navigation
Conversation
`RouteScan::on_router_collision_error` printed the collision and did not count it. The build went on, wrote dist/ with one of the two files and exited with code 0. The arm now counts the report, as the arm for an invalid route file does, so the build exits with code 1 before it bundles. The counter is now `error_count`, because it counts both kinds of route error.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 24 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updated 2:01 AM PT - Oct 28th, 2026
❌ @robobun, your commit 977d462 has 8 failures in
🧪 To try this PR locally: bunx bun-pr 47146That installs a local version of the PR into your bun-47146 --bun |
|
Status: ready for a maintainer. The one decision is the exit code: CI (build 134270). Every How I reproduced it. A framework with no dependencies: // app.ts
export default { app: { framework: { fileSystemRouterTypes: [{ root: "routes", style: "nextjs-pages", serverEntryPoint: "./server.ts" }] } } };
// server.ts
export function render(req, meta) { return new Response(String(meta.pageModule.default())); }
export async function prerender(meta) { return { files: { "/index.html": String(meta.pageModule.default()) } }; }
// routes/about.ts
export default () => "routes/about.ts";
// routes/about/index.ts
export default () => "routes/about/index.ts";Run The exit code is 0, and With this PR the build prints the same three error lines, then stops: no PR: #47146 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the mechanics of the change and found nothing else: the invalid_routes rename has no stale call sites in src/, the exit goes through the same Global::crash() the syntax-error arm already used, and the dev server's handler at src/runtime/bake/dev_server/router_glue.rs:137 prints without counting by design (it keeps serving across hot reloads), so it is not a missed sibling site. The remaining question is the policy one raised inline, which is a maintainer call.
Extended reasoning...
The change adds one counter increment to the collision arm of RouteScan in src/runtime/bake/production/entry_points.rs, renames the counter, and extends test/bake/dev/production.test.ts with a concurrent test.each matrix of collision fixtures plus a positive control; no security-sensitive surface is touched. The code is small and the test coverage is strong, but the inline finding is a behavior-policy question (which router collision reports should be fatal for bun build --app) that the author acknowledges as a downside, so a human maintainer should decide it rather than this review approving.
| )) | ||
| ); | ||
| Output::flush(); | ||
| self.error_count += 1; |
There was a problem hiding this comment.
🟡 (optional) Users with valid app layouts, such as a layout in each of two route groups, or two routers with different prefix values, can no longer run bun build --app at all. The collision arm at entry_points.rs:62 now counts every report the router emits, and the router also reports shapes that are not real URL collisions: route groups are dropped from the pattern (router/mod.rs:809), so shop/(a)/layout.tsx and shop/(b)/layout.tsx collide at router/mod.rs:373, and prefix is never passed into router::Type (production/mod.rs:572-593), so two prefixed routers sharing a file name collide. Fix: before the count can fail the build, the router must only report real URL collisions, honoring prefix in the pattern and keeping per-group layout nodes.
Why this was flagged
A bun build --app project uses nextjs-app-ui with layouts: true and a layout in each of two route groups, e.g. shop/(a)/layout.tsx and shop/(b)/layout.tsx; or two fileSystemRouterTypes entries with different prefix values whose roots both contain index.ts. scan_all builds the static pattern with Part::Group(_) => {} (src/runtime/bake/router/mod.rs:809), so both layouts land on the same route node and insert returns RouteCollision at src/runtime/bake/router/mod.rs:373; prefix is parsed (src/runtime/bake/config/framework.rs:471) but never placed in router::Type at src/runtime/bake/production/mod.rs:572-593, so prefixed routers collide too. on_router_collision_error now does self.error_count += 1 (src/runtime/bake/production/entry_points.rs:62) and build_with_vm calls Global::crash() at src/runtime/bake/production/mod.rs:606. On the base the same tree printed the message and still wrote dist/ with exit 0; after merge it prints "Multiple layout matching the same route pattern is ambiguous" and exits 1 with no dist/.
Verification: src/runtime/bake/router/mod.rs:801-813 builds the static pattern with Part::Group(_) => {}; insert at mod.rs:368-373 returns Err(InsertError::RouteCollision). src/runtime/bake/production/entry_points.rs:62 adds self.error_count += 1;, and production/mod.rs:605-607 calls Global::crash(), so both shapes now exit 1 before bundling.
There was a problem hiding this comment.
The facts are right, and this is the trade-off in Downsides. No code change for now. Two points for the maintainer who decides.
Neither shape builds correctly on main today. I ran both on a release build of main 9b8347a.
(a)/layout.tsxbeside(b)/layout.tsx, with a page in each group: main already exits with code 1. It prints the collision report, thenTypeError: No default valueat prerender (src/js/builtins/Bake.ts:132, a separate defect for any page below a layout). With that defect worked around, main exits with code 0 and renders pagexof group(a)with the layout of group(b):dist/x/index.html: x layouts=b-layout dist/y/index.html: y layouts=b-layout- Two routers that differ only in
prefix(pages/users.tsbesideapi/users.tswithprefix: "/api"): main prints the report, leavesapi/users.tsout ofdist/, and exits with code 0.
The one build that worked and now fails is a tree with two group layouts where no page is rendered with a layout.
The router fixes are separate changes. #42904 mounts each router on its prefix. Commit bfcffab in #39488 gives a route group its own layout node, and a port of it to main is in work. Until they land, this PR makes the build stop with a report where main writes a wrong dist/. If the preferred order is router fixes first, this PR can wait for them.
I leave this thread open for that decision.
Related to #42836, #39488
Problem
bun build --appprintserror: Multiple pages matching the same route pattern is ambiguous. Then it writesdist/with one of the two files and exits with code 0.signup.tsxswappedabout.tsxforabout/index.tsx.RouteScan::on_router_collision_errorprints and does not count (src/runtime/bake/production/entry_points.rs:43).build_with_vmstops only when the count is above 0 (production/mod.rs:605).Fix
error_count(renamed). The build then exits with code 1 before it bundles or writesdist/.test/bake/dev/production.test.ts(route collisions): 9 of 10 cases fail before, 10 pass after. Alsotest/cli/bake/bake.test.tsand 4 more suites.Background
FrameworkRouterreports scan problems to a handler:RouteScaninbun build --app. The dev server's handler keeps serving.about.tsbesideabout/index.ts,[id].tsbeside[slug].ts.Downsides
BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1): every collision report fails the build.next buildaccepts 3 of 6 shapes silently.prefix(bake: mount each file system router on its prefix #42904).Notes
Source and reach. An automated test run of
bun build --appfound this. No user reported it, and no issue quotes the message.--appneeds a canary or debug build, orBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1. Release 1.4.4 with that variable has the bug. The original handler (src/bake/production.zig, dc01a5d) was a copy of the dev server's handler, which prints and keeps serving.Repro. A framework with no dependencies:
app.tswith onefileSystemRouterTypesentry (root: "routes",style: "nextjs-pages",serverEntryPoint: "./server.ts"), and aserver.tsthat exportsrender,prerenderandgetParams. On main 9b8347a (debug and release builds), on canary 1.4.4-canary.1+a0cc096ce and on release 1.4.4 with the variable:With this PR: the same three error lines, no
Rendering routes, nodone, nodist/, exit code 1.Which file ships. The scan walks the names of a directory in hash bucket order, and the first file keeps the route. Tree:
routes/withabout.tsx,about/index.tsx,blog.tsx,contact.tsx,index.tsx,login.tsx,pricing.tsx. Main shipsabout.tsx. Addroutes/signup.tsxand it shipsabout/index.tsx. Remove it andabout.tsxis back. Exit code 0 each time. #40731 (open) sorts the scan. This applies to a collision inside one router. Across routers the first entry wins.Collision shapes. Both
RouteCollisionarms ofFrameworkRouter::insert(router/mod.rs:373,:410) reach the one handler arm, so the one line covers all of them. The cases: a file beside the index file of a directory, one file name with two extensions,[id]beside[slug], a route group (nextjs-app-ui), one path in two routers, one file in two routers, two layouts of one directory, and two collisions in one build (both are reported before the exit).A URL that two router types claim. It fails the build now, also when it is one file: a router entry listed twice, or two entries on one root whose files overlap. The report then lists the file twice. Before, the first entry kept the URL and the second never rendered it. Two types on one root that share no URL still build (
nextjs-app-uibesidenextjs-app-routes). The control case pins that.next build. I ran Next.js 16.4.3 on six shapes. It fails three:[id]beside[slug], a page in a route group beside the same path outside it, one path inapp/and inpages/. It builds three with no message:about.jsbesideabout/index.js, one name with two extensions, two layout files in one directory. This PR fails all of them. Bun already printserror:for each, names no winner, and the winner follows hash order.Tests. All in
test/bake/dev/production.test.ts.route collisions: 8 collision cases and 1 control, each onebun build --appchild process. The fixture's framework hasprerenderandgetParams, so a build that ignores the collision completes, and the exit code can tell the two apart.minimalFrameworkhas noprerenderand exits with code 1 for that reason.routers that share no URL build) has four router types, two of them on one root, with a static and a dynamic route. It compares every file indist/.two pages resolving to the same route are reported(the built-in React framework) now also asserts exit code 1 and nodist/. No existing assertion changed.toEqual, with exit code 0,doneon stdout anddist/written. A first version of the block (7 collision cases) gave the same result on a debug build (ASAN) of that commit and on the canary.production.test.ts30 pass,test/cli/bake/bake.test.ts85 pass,framework-router.test.ts37 pass,app-options.test.ts13 pass,dev/ssg-pages-router.test.ts10 pass,dev-and-prod.test.ts12 pass. No other test file runsbun build --app.production.test.tstake 4.4 s to 5.0 s each, and 5 to 8 of them reach the 5 s limit in a run, on main and with this PR.bun run rust:mordant. Its pinned nightly andcargo mordantare not installed on my machine. CI runs it.Measurements. Two release builds (LTO) in one build directory: main 9b8347a, and the same tree with this change. A second build of the change gave a byte-identical binary.
strace,perf,valgrindandbloatyare not installed on my machine, so the numbers come fromllvm-nm -S,llvm-objdump -d,gdband a ptrace syscall counter.RouteScan::on_router_collision_error511 to 517 bytes of.text(+6: oneincq, and two register encodings that are 1 byte longer), 0 bytes of data. The stripped binary has 109,037,072 bytes in both builds, and.texthas 83,497,513 bytes in both.bun build --app(3 pages): the changed function is entered 0 times (gdb breakpoint count).scan_inner,insert,scan,scan_allandbuild_with_vmhave the same instructions in both builds (1274, 774, 95, 48 and 2520).about.tsbesideabout/index.ts), 3 runs each: exit code 1 (was 0),mkdirandopencalls belowdist0 (was 2 and 6), threads started 0 (was 12 to 14), syscalls 255 (was 658 to 760).mkdirandopencalls belowdist5 and 9 on main, 5 and 9 with this change.bun build --appchild processes intest/bake/dev/production.test.ts.bun-darwin-aarch64. I measured the 6 bytes on linux x64 and did not inspect the macOS binary. Its segments are aligned to 16 KB pages.Reports that are not a user mistake. Both fail the build now. An exemption for them was considered and rejected: the build they let through is wrong.
shop/(a)/layout.tsxbesideshop/(b)/layout.tsx(nextjs-app-ui,layouts: true) is reported as a layout collision. The scan drops a route group from a static pattern (router/mod.rs:749,:809), so both layouts land on theshoproute. The dev server then serves every page with one group's layout. A build of such a tree exits with code 0 on main only when no page is rendered with a layout: a page below a layout fails at prerender withTypeError: No default value(src/js/builtins/Bake.ts:132). bake: consolidate the open robobun dev-server, HMR runtime, production build and router fixes #39488 has a router change for this (commit bfcffab). It must land no later than the prerender fix. Both are handed off.prefix: the router ignores it, sopages/users.tsbesideapi/users.tswithprefix: "/api"is reported as one pattern. Main droppedapi/users.tsand exited with code 0. bake: mount each file system router on its prefix #42904 mounts each router on its prefix.Not in this PR. Each of the first five still exits with code 0 on a build of this PR.
dist/printsFailed to write ... to output directory, thendone(production/mod.rs:720,:731,:812). These arms need a new exit point after the write loop, not a count. bake: consolidate the open robobun dev-server, HMR runtime, production build and router fixes #39488 has a fix (from bake: fail bun build --app when an output file cannot be written #39295, which is closed), and the work is handed off.getParamsreturns{ pages }and a render throws:error:, thendone(src/js/builtins/Bake.ts:173). The array form exits with code 1. bake: fail the build when a component throws during static prerender #33216.--target=node:error: target must be 'bun' when using --app, then a full build (src/runtime/cli/Arguments.rs:2584). cli: exit 1 when bun build --app is given a --target other than bun #38581.first.tsbeside[id].tswhengetParamsyieldsfirst: no message, and one page overwrites the other indist/. The patterns differ, so the scan sees no collision. Handed off.production/mod.rs:561). Handed off.dev_server/router_glue.rs:137) prints both kinds of report and keeps serving. That is its contract, andtest/cli/bake/bake.test.tspins it.router/js.rs:316) still haspanic!("TODO: onRouterCollisionError for JSFrameworkRouter"). Onlybun:internal-for-testingreaches it. bake: report route scan errors instead of aborting, and fail bun build --app on them #42836 has a fix.Earlier work. The same count is in #42836 (open, conflicts with main), in #39174 (closed), and in commit fdf1604 of #39488. #46021 merged
RouteScanand the exit from #42836 without the collision line ("crash fix only"), and said that PR stays open for the rest. After this PR, what is left in #42836 is the test binding, a privateTinyLog::write, and a message wording.Self-review. 22 concerns: 5 about tests and about wording that was false, 17 about what this text must say. Changed: the control has several routers, a case for one file in two routers, the React case pins the exit code, a failed case prints the child's stderr, the title, and the Downsides and Notes text above. Not done: I did not remove the same hunk from the branch of #42836. That branch does not apply to main. I posted the relation there.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/production.test.ts