[robobun]

Problem

• Bun.FileSystemRouter({ style: "nextjs" }) picks the wrong route whenever a catch-all route competes with a shallower plain dynamic route. With pages/[slug].js and pages/opt/[[...rest]].js, match("/opt") returns /[slug] ({ slug: "opt" }) while match("/opt/x") returns /opt/[[...rest]], so the same directory serves different routes depending on whether a trailing segment is present. Next.js serves both from opt/[[...rest]].
• Same cause, other shapes: posts/[...id] shadows posts/wow/[[...id]] completely (that file can never match), [team]/[...rest] beats [team]/docs/[[...path]] for /acme/docs, and [ab]/[page] beats [a]/settings for /acme/settings while [a]/[page] loses to [abc]/settings: the parameter name length decided precedence.
• Cause: sorter::sort_by_name (src/router/lib.rs) ordered the dynamic list primarily by Route::kind, the most dynamic segment anywhere in the route (dynamic < catch-all < optional catch-all), and within a kind by a byte table that weighted ] above every letter. Routes::match_dynamic returns the first route in that list that matches, so a route's position was decided by its deepest segment rather than by the first segment where two routes differ.
• Second problem, exposed by fixing the first: two failure exits of Pattern::match_ (a [...rest] that finds nothing left to consume, and a pattern that ends while URL segments remain) return without dropping the params the candidate pushed before giving up. The next candidate inherits them, so the winner reports a duplicated value (team: ["acme", "acme"]) or, when the leaked name is not one of its own parameters, .params crashes (Segmentation fault at address 0x0 on the released binary; issue Bun.FileSystemRouter router match unknown behaviour #12206). Today this needs the losing route to sort before the winner by accident of kind or parameter name; with per-segment precedence a deeper static-prefixed route is tried before the shallower winner as a matter of course ([team]/docs/[...topic] before [team]/[page]), so trees that work on the released binary would start hitting it.

Fix

• sorter::compare replaces the kind ordering and the byte table. Routes are compared one /-segment at a time; at the first depth where they differ, static beats [x] beats [...x] beats [[...x]] (static segments compare by name among themselves), and a route that ends first comes first. Ties (only possible for routes that differ in parameter names alone, like [a]/x vs [b]/x) fall back to the name so the winner does not depend on directory read order.
• Fully static routes still sort first as one group: load_all splits the list at the first non-static route and matches static routes through the hash map, so only the dynamic suffix's order matters for matching.
• Why this is the right order: it is what Next.js's getSortedRoutes (packages/next/src/shared/lib/router/utils/sorted-routes.ts) produces, which is the contract style: "nextjs" advertises. At each tree node it emits the node's own route, then static children, then [slug], then [...rest], then [[...rest]], depth first. The old order already agreed with this for static-vs-dynamic shapes (that is what the byte table approximated); the per-segment comparison keeps those results and fixes the catch-all and parameter-name cases.
• Routes::match_dynamic clears params after every rejected candidate, so each candidate starts from an empty list whatever exit Pattern::match_ took. This is the loop that reuses the list across candidates, and the hunk is independent of FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680, which makes Pattern::match_ itself truncate on every failure (and also fixes the shapes that leak under the current order); the two merge cleanly in either order and each is correct on its own.
• load_all loses the index_id bookkeeping. The index route has kind == Static and has always sorted into the static group, so the "index route can be the last route" branch never ran; its comment described the ordering this PR replaces.
• Behavior change to note in release notes: where a catch-all or optional catch-all route shares a static prefix with a shallower dynamic route, the prefixed route now wins, as in Next.js. The existing "should support catch-all routes" test asserted the old result for /posts/wow/hey/there; it now asserts the Next.js result. router.routes keys also come out in the new order (static routes first, then match order).
• Verified with test/js/bun/util/filesystem_router.test.ts. On the released binary: the five new-behavior "route precedence" cases, the updated catch-all test and "lists routes in match order" fail with the old winner, and "a candidate rejected before the winner leaves no params behind" fails with the segfault above (two of its four trees leak under the old order too; the other two only under the new order, and all four fail on a build with the reorder but without the params.clear(), with panic: assertion failed: name_slice.length > 0). All 45 tests in the file pass with this build; the other precedence cases pass on both builds and pin what must not move. cargo clippy -p bun_router, cargo fmt --check and test/regression/issue/18242.test.ts (the other FileSystemRouter consumer) are clean.

Background

• Route::kind is the highest pattern::Tag among a route's segments (Static < Dynamic < CatchAll < OptionalCatchAll), computed by Pattern::validate when the route is loaded. param_count is the number of non-static segments and is what append_route uses to send fully static routes to the hash map.
• Routes::match_ looks the path up in the static hash map first and only then walks the dynamic list in order with one params list shared by every candidate, so for dynamic routes list order is precedence, and a candidate that pushes params and then fails must not leave them behind.
• .params and .query are built later by scanning the winning route's name for the recorded parameter names; a name that is not in the winner's route name produces an empty key, which is the crash.
• match_name is the route name without its leading slash and lowercased; [[...x]], [...x] and [x] are the three prefixes Pattern::init dispatches on, and every name in the list has already passed Pattern::validate, so classifying a segment by prefix in the sorter agrees with how the matcher consumes it. Walking Pattern::init in the comparator instead would tie the sorter to the len/is_end encoding that FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 is changing, for no difference in result.
• Separate bugs found while testing, each handed off on its own rather than folded in here: a root-level [[...slug]] never matches / without an index route and a trailing [param] accepts an empty segment (FileSystemRouter: match / against a root-level optional catch-all route #39290 has both); bake's dev-server FrameworkRouter matches its dynamic routes in directory scan order and needs the same precedence rule. FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 also fixes single-character trailing static segments ([a]/x); the tests here use multi-character segments so they do not depend on it.

---

no test proof · iteration 0 · 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

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 seconds

Limit details: You’ve used all 5 included reviews currently available under your plan.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d58f2997-9a59-40db-8f5a-c29b0015b80f

📥 Commits

Reviewing files that changed from the base of the PR and between 8c5296a and 88dd4d3.

📒 Files selected for processing (2)

• src/router/lib.rs
• test/js/bun/util/filesystem_router.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 1:59 AM PT - Aug 16th, 2026}

@robobun, your commit 88dd4d3 is building: #99370

[robobun]

Status: reproduced on the released bun (1.4.0) with pages/[slug].js + pages/opt/[[...rest]].js: match("/opt") returned /[slug], match("/opt/x") returned /opt/[[...rest]]. The same ordering also made posts/wow/[[...id]] unreachable next to posts/[...id] and let parameter name length decide between [a]/settings and [ab]/[page].

Fix: sorter::compare in src/router/lib.rs orders the dynamic list by per-segment precedence (the Next.js getSortedRoutes order), and match_dynamic clears params after each rejected candidate, since the new order tries deeper static-prefixed routes before the shallower winner and a rejected candidate could leave its params behind (#12206). The new and updated cases in test/js/bun/util/filesystem_router.test.ts fail on the released binary and pass with this branch.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes user-visible route precedence for Bun.FileSystemRouter (and updates an existing test to assert the new winner), a human look to confirm the behavior change is acceptable would still be worthwhile.

Checked: sorter::compare orders segments by Tag value (Static < Dynamic < CatchAll < OptionalCatchAll) via strings::tokenize, which drops empty fields so leading/trailing / and the index route's match_name are handled; iterator .cmp gives shorter-route-first as documented.
Checked: the removed index_id branch was dead — the index route's kind is ValidationResult::default().kind == Static, so it always sorts before dynamic_start.
Checked: no other callers of the removed sort_by_name* functions remain in src/; segment_key's prefix checks match Pattern::init's dispatch and every name has passed Pattern::validate.

Extended reasoning...

Overview

Rewrites sorter in src/router/lib.rs to compare dynamic routes one /-segment at a time (static beats [x] beats [...x] beats [[...x]], shorter first) instead of by Route::kind then a byte-weight table. Drops the index_id bookkeeping in load_all as dead. Adds a large it.each precedence matrix and a route-order test to filesystem_router.test.ts, and updates the existing catch-all test so /posts/wow/hey/there now expects posts/wow/[[...id]] instead of posts/[...id].

Security risks

None. This is a pure comparator over already-validated route names; no new parsing of untrusted input, no allocation, no FFI. segment_key classifies by prefix on names that already passed Pattern::validate.

Level of scrutiny

Medium-high. The comparator itself is small and mechanically verifiable against Tag's numeric order and strings::tokenize semantics, and the test coverage is thorough (nine precedence scenarios plus an explicit order assertion). But this is a behavior change to a public API: existing apps whose route trees hit the old ordering (e.g. posts/[...id] beating posts/wow/[[...id]]) will now route differently, and one existing test assertion was flipped to match. The PR description flags this for release notes; a maintainer should confirm that's the right call versus gating it.

Other factors

• Verified the removed index_id > dynamic_i branch was unreachable: the index route gets kind = Tag::Static from ValidationResult::default() and the new comparator (like the old one) groups all Static routes first, so the index can never land past dynamic_start.
• Verified strings::tokenize filters empty segments, so match_name values (no leading slash) and the index route's b"/" compare sanely; iterator cmp on (u8, &[u8]) tuples yields the documented "shorter route first, then per-segment kind, static segments by name" order, with the trailing a_name.cmp(b_name) making differing-param-name ties deterministic.
• Grepped for other callers of the removed sort_by_name/sort_by_name_string/sort_by_name_cmp — none remain in src/.
• The PR description's mapping to Next.js getSortedRoutes looks right (per-node: self, static children, [slug], [...rest], [[...rest]], DFS), but I did not independently diff against the upstream Next.js source.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes user-facing route precedence in Bun.FileSystemRouter (an intentional behavior change the description flags for release notes), a human look would still be worthwhile to sign off on the compat trade-off.

Checked: sorter::compare produces the Next.js getSortedRoutes order (static-first grouping preserved for the dynamic_start split; iterator .cmp puts a route before its own children); the removed index_id branch was provably dead (index route always has kind == Static); the two Pattern::match_ exits that leak params (catch-all on empty remainder, trailing static with URL left over) are the ones the new params.clear() covers and both are exercised by the subprocess test; no other callers of the deleted sort_by_name* helpers.

Extended reasoning...

Overview

Two files: src/router/lib.rs replaces the route sorter (sort_by_name + byte table → sorter::compare doing per-segment lexicographic comparison on (Tag, static-name) tuples), adds params.clear() after each rejected candidate in match_dynamic, and drops the index_id special case in load_all. test/js/bun/util/filesystem_router.test.ts adds 10 it.each precedence scenarios, a route-listing order test, a subprocess test pinning the params-leak fix across four tree shapes, and updates one existing catch-all assertion to the Next.js result.

Security risks

None. The comparator operates on filesystem-derived route names that have already passed Pattern::validate; no untrusted network input reaches the changed code. params.clear() only shrinks a caller-owned Vec.

Level of scrutiny

Medium-high. The implementation itself is compact and well-covered, but it is a deliberate behavior change to a public API: routes that previously matched /[slug] for /opt will now match opt/[[...rest]], and router.routes key order changes. The PR description explicitly calls this out for release notes. That is a product/compat decision a maintainer should ratify rather than an automated approval.

Other factors

• Verified the removed index_id branch was dead: the index route is built with ValidationResult::default() (kind = Static, param_count = 0), so it always sorts into the static prefix and can never land at or after dynamic_start.
• Verified strings::tokenize filters empty segments, so match_name (no leading slash) tokenizes cleanly; the index route's match_name = "/" only compares within the static group where order is irrelevant to matching.
• Traced the two Pattern::match_ failure paths that return false without clearing (CatchAll with empty path_; the fall-through false after a trailing static segment with URL left over) and confirmed each maps to a labelled case in the new subprocess test.
• Confirmed the deleted sort_by_name/sort_by_name_string/sort_by_name_cmp have no other callers (the sort_by_name_then_version hit in src/install/dedupe.rs is unrelated).
• All comment-cop bot threads are resolved; the current diff's comments are one-liners.

[robobun]

Superseded by #47206. It brings the routing half of this pull request and of #39290 onto today's main as one change. The crash fix of both landed in #46021.

What #47206 keeps from this pull request:

• The order: at the first place where two dynamic routes differ, static text beats [x] beats [...x] beats [[...x]], and a route comes before the routes under it.
• The removal of the dead index_id branch in load_all.
• The precedence tests, as rows of one table. Each row that Next.js accepts is checked against next@16.4.3.

What it does differently:

• The sort key is cut by the matcher's own tokenizer (Pattern::init), not by a split on /. The two disagree for a name like foo[id].tsx.
• The key is built once per route, so a comparison does not tokenize.
• Static routes keep the order they have on main, and routes of the same shape keep their order. Only the order between routes of different shapes changes.

Closing in favour of #47206.