Repository navigation
FileSystemRouter: decide route precedence and the route for / at load, reject empty [param] segments - #47206
FileSystemRouter: decide route precedence and the route for / at load, reject empty [param] segments#47206robobun wants to merge 4 commits into
/ at load, reject empty [param] segments#47206Conversation
…, reject empty [param] segments
match() takes the first dynamic route that accepts a path, and the
dynamic routes were ordered by their most dynamic segment and by the
bytes of parameter names. So /docs/intro went to /[org]/[repo] when
docs/[...path] existed.
load_all now sorts dynamic routes by a key that it builds once per route
from the matcher's own tokens. At the first token where two routes
differ, static text beats [x] beats [...x] beats [[...x]], and a route
comes before the routes under it. This is the order of Next.js's
getSortedRoutes. Static routes keep their order, and routes of the same
shape keep their winner.
Routes.index becomes Routes.root: the index route, else the first
root-level [[...x]] in match order. Such a route now serves / when there
is no index route, and match("/") stays one pointer read.
A [param] no longer accepts a missing or empty segment.
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Pattern precedence and matching src/router/lib.rs, test/js/bun/util/filesystem_router.test.ts |
Pattern tokens now define route ordering by precedence. Dynamic matching rejects empty path segments. Tests cover route-tree precedence, case-only name conflicts, catch-all matching, and exposed route order. |
Root route selection and matching src/router/lib.rs, test/js/bun/util/filesystem_router.test.ts |
The router selects a root-level optional catch-all when no index route exists and uses the selected route for empty-path matching. Tests cover path variants, unmatched paths, and route changes after reload(). |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
Minimal · up to 6d0bf
-
No merge-blocking issue was identified. Root fallback and reload behavior are supported by source inspection; merge readiness remains subject to normal build and test checks.
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description is highly detailed and covers the problem, implementation, behavior changes, verification results, performance impact, and known limitations. It does not use the exact template headings, but it provides the required information in equivalent sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
|
Updated 12:51 AM PT - Oct 29th, 2026
❌ @robobun, your commit 6d0bfb2 has 10 failures in
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.cbe3baa8-c65b-4f0d-a0cc-d01dc34cd0e2
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.e880fd69-8bb8-421a-a1cd-58933e2b8a4e
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.c15f349f-2707-41e5-af49-e1796bef075e
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.7dce192d-4392-4342-b418-e630095a5347
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.6dbd4501-7f4b-42df-8e63-898dd57b8de0
CapacityLimitExceededThis account has reached its capacity limit. Reach out to your account team for assistance.b6dad4fc-51b1-4b0b-acef-102a0276f422🧪 To try this PR locally: bunx bun-pr 47206That installs a local version of the PR into your bun-47206 --bun |
|
Status
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new Routes::root raw pointer (the sorted position from order.iter().position(..) is exactly the slot apply_permutation_in_place fills and route_list.route[i] is that same Box, so it does not dangle), the debug_assert!(!pathname.is_empty()) in match_ (its only caller, match_page_with_allocator, has already collapsed "", //, /index and . to the root path before calling it), and the removal of the dynamic_len -= 1 exclusion (the index route's name yields no tokens, so its kind is Static and it never sat in the dynamic range) — none of these is a problem.
Extended reasoning...
The change rewrites dynamic-route precedence in src/router/lib.rs (new tokenized sort key, root-level optional catch-all serving /, rejection of empty [param] segments) and adds a large Next.js-parity test matrix in test/js/bun/util/filesystem_router.test.ts; it touches no auth, crypto or injection surface. Inline findings already flag the intentional breaking [param] change and two ordering edge cases, so a human needs to look regardless; this note only records the raw-pointer, debug-assert and index-route concerns that were verified sound.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Two dynamic routes whose names differ only in case, such as [id].tsx beside [Id].js, compared equal. The sort is unstable, so either one could come first, and match() takes the first route that accepts. The winner changed with the number of other routes and with the order in which the files were created. The comparator gets a last step on the name as written, in descending byte order, so the lowercase spelling goes first. A URL reaches static text only by its lowercase spelling, and for that URL Next.js serves the file with the lowercase name.
There was a problem hiding this comment.
I re-reviewed the follow-up commit and found no new bugs; because this is a labeled-breaking change to route precedence whose release train is still an open question, a maintainer's look is still the next step.
What was reviewed this pass:
- The new
nametie-break inKeys::compareplus the 6-tree test closes the case-only nondeterminism raised last round. rootis taken fromroute_list.route[i]after the permutation is applied and the Boxes are moved;this.indexstays valid acrossapply_permutation_in_placesince only theBoxpointers move, not the heapRoute.- The
debug_assert!(!pathname.is_empty())inmatch_has one caller, which trims trailing slashes and answers the empty path fromrootfirst, so the precondition holds structurally in release too. position(|&i| keys.takes_empty_path(i))anditer().cmp()are predicate/ordering calls, not byte-literal searches, so the byte-search lints don't apply.
Extended reasoning...
The PR rewrites the dynamic-route sort in src/router/lib.rs as a precomputed token key, moves the / decision to load time (index route, else first root-level [[...x]]), and makes [x] reject an empty or missing segment, with ~380 lines of new tests in test/js/bun/util/filesystem_router.test.ts. It touches raw-pointer plumbing (Routes::root, static_) across a new permutation step but no security-sensitive surface. The follow-up commit addressed the case-tie finding from the prior review; the remaining open thread is the silent-breaking-change question, which the PR description itself escalates to a maintainer for release placement. The bug hunt exited on dry_streak with no findings, but a breaking user-facing behavior change with an unresolved release decision is not something to approve automatically.
The comment on the co-owned pointer goes back to the field it was on. No code changes.
|
@Jarred-Sumner, one more question for the same function. It is the item under "Not in this PR" about a Today (1.4.3 and main) the tokenizer cuts such a name inside the segment.
No issue reports this. I did not open a code PR, because each fix is a visible change and the choice is yours. Which behavior do you want for such a name?
In 166 public repositories that use the router, I found none that calls #47254 is true for each answer. Its notes have the table of today's answers and the public trees with such names. |
Problem
Bun.FileSystemRoutergives the wrong route ornull, with no error. On random route trees, 710 of 39,538 lookups differ from next@16.4.3. No user reported it.sorter::sort_by_name(src/router/lib.rs) orders dynamic routes by their most dynamic segment, andmatch()takes the first that accepts./docs/introgoes to/[org]/[repo], not/docs/[...path]./reads only the index route, anddocs/[page]matches/docswithpage: "".Fix
load_allsorts dynamic routes by a key built once per route with the matcher's tokenizer,Pattern::init. Static text beats[x]beats[...x]beats[[...x]], as in Next.js. Of two names that differ only in case, the lowercase one goes first.Routes.rootholds the index route, else the first root-level[[...x]].Pattern::match_rejects an empty[param].test/js/bun/util/filesystem_router.test.ts(23 of 80 fail on main). Also18242.test.tsandresolve.test.ts.Background
match()tries the index route, the static routes, then the sorted dynamic list./(FileSystemRouter: match / against a root-level optional catch-all route #39290) and a key split on/(FileSystemRouter: order dynamic routes by per-segment precedence like Next.js #39291).Downsides
[param]route no longer matches it. One public site serves/proceduresfromprocedures/[category].tsx. @Jarred-Sumner, which release takes this?[param]hit now also tries the catch-alls sorted before it: in a tree of 400 routes, 201 against 100 on main (a catch-all hit: 199 against 250.5). Construction costs up to 6.4% more instructions.Notes
This supersedes #39290 and #39291. #46021 took the crash fix of each and left both open for the routing half. This PR brings that half to today's
mainas one change. From #39291 it keeps the order rule and the removal of the deadindex_idbranch. From #39290 it keeps the[param]rule. The key has the shape ofprecedence_keyin #39488.Repro (
bun repro.mjs, main prints 5 faults, this PR prints none):Behavior changes (for the release notes). None of them gives an error or a warning.
[param]takes one segment that is not empty.docs/[page].tsxno longer matches/docs, andblog/[slug]/[id].tsxno longer matches/blog//42. The URL goes to the next route that accepts it, or tonull./docsserved, adddocs/index.tsx(itsparamsare{}, not{ page: "" }), or name the filedocs/[[...page]].tsx, which also takes deeper paths.null: alexandre-dosreis.me hasprocedures/[category].tsx, noprocedures/index.tsx, and/proceduresin its prerender list. The page handles an emptycategory. The site answers anullmatch with a 404, and its Dockerfile builds onoven/bun:1.api/sources/[id].tsandapi/sources/callback/[provider].ts./api/sources/callbackwas/api/sources/callback/[provider]withprovider: "". It is now/api/sources/[id]withid: "callback", as in Next.js.[x], then[...x], then[[...x]]. A token is a piece of a route name as the matcher'sPattern::initcuts it. A route comes before the routes under it. No option restores the old order. Routes of the same shape keep the order they had.[[...slug]]serves/when there is no index route. The index route still wins when both exist./[id]over/[Id], and/[org]/docsover/[org]/Docs. For the second pair next@16.4.3 serves/acme/docsfrom the same file. On main the winner of such a pair changes with the number of other routes. With[id].tsxbeside[Id].jsand 0 to 80 other routes, main gives/xto[Id].jsat 20 of the 81 counts, first at 32 other routes, and in all 10 creation orders of 52 files that I tried. The sort is unstable: two equal items can change places, and whether they do depends on the number of items. So the winner can differ from the one that main gives for the same tree.router.routeslists the dynamic routes in the new order. The static routes keep their order.One existing assertion changes. "should support catch-all routes" (b249ed7, 2022) asserted that
/posts/wow/hey/theregoes to/posts/[...id]althoughposts/wow/[[...id]].tsxexists. It now goes to/posts/wow/[[...id]], which is what next@16.4.3 picks for that tree. The sort that this PR replaces is from 8e0230b (2021).How far each change reaches
[param]rulenull, 128 to another routenull, 6 to another route/Bun.FileSystemRouter. The URLs come from each tree's own route names: every route, every prefix of it, one more segment, a trailing slash, and an empty segment in each[param]place.[param]rule changes a lookup. In every one of the 231, main had bound a[param]to"". The order ofrouter.routesdiffers on 5 of the 63 trees and moves no lookup there./on these public trees is zero. They make the router agree with its documented reference. The[param]rule is the one that can break a site.[id].tsxbeside[Id].js. I did not search the 63 trees for such a pair, and the random trees have none.//and/indexspellings.Who asked
FileSystemRouterto support non-rest optional path parameters #8754 (open) asks for optional parameters that are not catch-alls. The empty[param]is the nearest thing today, and change 1 removes it.Why in place, and which release
[x]matches exactly one non-empty segment,[...x]one or more,[[...x]]zero or more, and among matching dynamic patterns the winner is by Next.js precedence (text < param < catch-all < optional catch-all; same rule FileSystemRouter: order dynamic routes by per-segment precedence like Next.js #39291 givesBun.FileSystemRouter)". Its router change targets a file that bake: move the code into folders by job (no behavior change) #45489 moved, so no Bun file router ships the contract yet.Pattern::match_: "FileSystemRouterno longer matches URLs shorter than the route pattern". It left the case where the URL ends exactly where a[param]starts. Change 1 closes it. Robustness pass across install, css, ffi, crypto, spawn, shell, and node compat #36165 and FileSystemRouter: return null from match() when the path string does not start with '/' #34028 are both on the 1.4 breaking list (List of breaking changes for 1.4 #28792).Bun.serveroutes already behave this way:packages/bun-uws/src/HttpRouter.hsorts static text before:parambefore*, and does not bind an empty:param.[param]rule, a route with an empty[param]is tried before the route that should win.How the reference was checked
The expected routes come from the published
next@16.4.3package.getSortedRoutes(dist/shared/lib/router/utils/sorted-routes.js) orders the pages, and the first page whose route regex matches wins, as indist/server/lib/router-utils/filesystem.js. The server builds the regex withgetNamedRouteRegexand three options. The runs below usegetRouteRegex. The two give the same route and params on 301,888 lookups over 2,848 generated trees.A differential run builds random trees of 1 to 4 route files, keeps the trees that Next.js accepts, and compares the route name and the params of 106 URLs per tree.
blog/[slug],blog/archive/[[...date]],docs/[...path],api/[...proxy],[org]/[repo]/...), main gives 2 of 26 URLs to another file than next@16.4.3:/docs/introgoes to/[org]/[repo], and/blog/archivegoes to/blog/[slug]. This PR gives 0 of 26.The key
sorter::Keysholds one byte string per dynamic route. Per token it has the rank of the tag (1static text,2[x],3[...x],4[[...x]]), and for static text also the text and a0. A route name cannot contain a NUL byte, so the byte order of two keys is the order of their tokens, and a route that ends first sorts first. Routes with the same key fall back to the order they had before. Two names that are still equal then differ only in case. The last step compares them as written, in descending byte order, so the lowercase spelling goes first. After that step no two dynamic routes compare equal, so the sort has one result for any order of the files.The tokens come from
Pattern::tokens, whichvalidatenow uses too. It calls the matcher's ownPattern::init, so the key and the matcher cut a name at the same places.Deleted
sorter::sort_by_name(the order byRoute::kindandparam_count) and its adaptersort_by_name_cmp.index_idbranch ofload_all. The index route haskind == Static, so it was never in the dynamic part of the list.Routes::match_.match_page_with_allocatortrims every trailing slash first, so it never passed a path that trims to nothing. It answers the empty path itself, fromroot.Measurements (release builds of main 9b8347a and of that commit with this PR. They were taken with the last comparison step in the other direction, which is the same code with the two operands swapped. No measured tree reaches that step.)
match(), index route hitRoutes::match_page_with_allocator,gdbstepi, third callmatch(), static route hitmatch(), first dynamic route hitmatch(), miss over 300 dynamic routesmatch("/"), no index route, root[[...slug]], 10 / 31 / 301 dynamic routesnull)match("/"), no index route, no root[[...slug]], 300 dynamic routesinstructions:uof 400 constructions minus 200, per construction[p]/sNNNgdbbreakpoint hits on the mimalloc entry points, 2 constructions minus 1match(), dynamic hitsizebun_routersymbolsnm -S, sizes summed[param]rule once per[param]token that it reaches.Keys::new, about 400 instructions per dynamic route). A comparison is cheaper than before (88 instructions against 108 on the first tree), because the old adapter calledsort_by_nameup to twice per comparison. So a router with few dynamic routes gets cheaper and a router with many gets dearer.match()walks the dynamic list in the new order, so the cost of a hit moves with the place of its route. Main tries every route that has only[param]segments before any catch-all. This PR sorts by path, so a catch-all under a static name comes before a[param]beside it. In a tree of 100 folders that each hold[id],[id]/[sub],all/[...rest]andopt/[[...rest]](400 dynamic routes), the mean place of the winner goes from 100 to 201 for[id], from 101 to 202 for[id]/[sub], from 250.5 to 199 for[...rest]and from 350.5 to 200 for[[...rest]]. The mean over the 400 routes is 200.5 on both builds, because the list holds the same routes. A rejected candidate costs about 243 instructions (the miss above).[param]route, 17.4% fewer for a catch-all route. The 888 lookups that go to another route on this PR visit 42.1% fewer. Over all lookups the walk is 12.7% longer. On 400 trees of 1 to 5 route files (1,145 lookups) it is 1.6% longer. An earlier version of this bullet gave only the small-tree figure of the differential run (8.0% for the lookups that keep their dynamic route, on trees of 1 to 4 files).[param]rule 5 tests fail, without the slot for/5, without the key 9, with plain byte order in place of the old order for ties 2, without the last step on the name as written 3.perf,valgrindandbloatyare not on the machine. The instruction counts useperf_event_opendirectly andgdb.Other designs
/(FileSystemRouter: match / against a root-level optional catch-all route #39290). Every/on a router with no index route then pays the walk (72,760 instructions for 300 dynamic routes), also when the answer isnull. The slot is one pointer read, andload_allfills it for the constructor and forreload()./(FileSystemRouter: order dynamic routes by per-segment precedence like Next.js #39291). The matcher also cuts a name at a[inside a segment, so that key and the matcher disagree for a name likefoo[id].tsx.match_tokenizes each candidate again on each request. Keeping the tokens would make the key a by-product and shorten the walk. That is a rewrite of the matcher and is not in this PR.Open pull requests on the same file
Bun.FileSystemRoutercrashes on a directory listing that is not the one asked for, andreload()through a symlink #46286 fixesreload()through a symlinkeddir. On main that call never returns:Pattern::validateloops on an empty route name.Pattern::tokensends on an empty name, so with this PR alone the call returns, and the routes still have the wrong names. FixBun.FileSystemRoutercrashes on a directory listing that is not the one asked for, andreload()through a symlink #46286 merges cleanly with this branch.src/router/lib.rs). Whichever lands second needs the sort and the slot for/ported.Not in this PR
[a]/x.tsxserves/fooand not/foo/x(Bun.FileSystemRouter router match unknown behaviour #12206, still open). The cause is the clamp inPattern::init_maybe_hash. FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 had a fix and was closed after Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021. It is a second cause with its own behavior changes, so it gets its own PR on top of this branch.Docs/[id].tsxserves/docs/1and not/Docs/1(FileSystemRouter: match a dynamic route with uppercase letters by its on-disk spelling #43003).[param]that does not span its segment is cut in two:[id]-edit.tsxserves/7/edit, andfoo[id].tsxserves/foo/1. Next.js reads both names as static text./café.reload()does not throw for a tree on which the constructor throws (FileSystemRouter: surface route-load errors from reload() like the constructor does #34614).FrameworkRouter::match_slowin bake (src/runtime/bake/router/mod.rs) is separate code with the same first-match walk. bake: consolidate the open robobun dev-server, HMR runtime, production build and router fixes #39488 and bake: sort the route scan by file name, pick the most specific dynamic route in match_slow #40731 change a file that bake: move the code into folders by job (no behavior change) #45489 moved./docs/indexis read as/docs, and leading slashes are trimmed. Next.js does neither.Self-review
24 concerns survived. 21 are addressed: the breaking label and the wording above, the reach of each change, two table rows for a
[param]that hands a URL on, thefoo[id]test (removed), the exclusions with their trackers, and the notes on #46286, #46584, #39488 andBun.serve. Three are declined:[param]rule into a first PR. The order gives wrong answers without it, and the rule is a six-line hunk that can be carved out if the order has to wait.The self-review also had a step removed that broke a tie between names that differ only in case: it put the uppercase spelling first and no test pinned it. A later review asked for a decided winner. The step is back in the other direction (lowercase first, which is the file that Next.js serves), with mirrored test rows.
no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts