Feat react router style file system router #24065
Conversation
…t-for-react-router Add React Router file-system routing support
WalkthroughAdds React Router-style routing to FileSystemRouter, exposes a new style option ("nextjs" | "react-router") across API and types, extends route parsing/matching to support React Router segments (static, dynamic, optional, splat), and adds tests covering the new behavior. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/api/file-system-router.md (1)
131-135: Document the default value for style.State that the default remains
"nextjs"for backwards compatibility.Apply this diff:
- style: "nextjs" | "react-router"; + style: "nextjs" | "react-router"; // default: "nextjs"src/router.zig (1)
514-536: getKind is Next-only; use route.kind so it works for both styles.Current logic inspects "[]" tokens and will return "exact" for React Router routes like "/:id" or "/*".
- pub fn getKind(this: *MatchedRoute, globalThis: *jsc.JSGlobalObject) JSValue { - return KindEnum.init(this.route.name).toJS(globalThis); - } + pub fn getKind(this: *MatchedRoute, globalThis: *jsc.JSGlobalObject) JSValue { + const kind_str = switch (this.route.kind) { + .static => KindEnum.exact, + .dynamic => KindEnum.dynamic, + .catch_all => KindEnum.catch_all, + .optional_catch_all => KindEnum.optional_catch_all, + }; + return ZigString.init(kind_str).withEncoding().toJS(globalThis); + }This uses the canonical kind computed at parse time. As per coding guidelines.
src/bun.js/api/filesystem_router.zig (1)
84-96: Normalize fileExtensions to handle both with and without leading dot.The issue is valid. The default extensions (
"tsx","jsx") are stored without dots, but the current code blindly removes the first character via[1..], assuming all user-provided extensions have a leading dot. If a user passes"tsx"(matching the default format), it becomes corrupted to"sx". The suggested fix is appropriate—it checks whether the first character is a dot before stripping it, making the code defensive and compatible with both formats.- if (try val.getLength(globalThis) == 0) continue; - extensions.appendAssumeCapacity((try val.toUTF8Bytes(globalThis, allocator))[1..]); + const len = try val.getLength(globalThis); + if (len == 0) continue; + const bytes = try val.toUTF8Bytes(globalThis, allocator); + const slice = if (bytes.len > 0 and bytes[0] == '.') bytes[1..] else bytes[0..]; + if (slice.len == 0) continue; + extensions.appendAssumeCapacity(slice);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
docs/api/file-system-router.md(2 hunks)packages/bun-types/bun.d.ts(3 hunks)src/bun.js/api/filesystem_router.zig(7 hunks)src/options.zig(3 hunks)src/router.zig(9 hunks)test/js/bun/util/filesystem_router.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/options.zigsrc/bun.js/api/filesystem_router.zigsrc/router.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/options.zigsrc/bun.js/api/filesystem_router.zigsrc/router.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/filesystem_router.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/filesystem_router.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/util/filesystem_router.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/util/filesystem_router.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/util/filesystem_router.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/util/filesystem_router.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/util/filesystem_router.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/util/filesystem_router.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/util/filesystem_router.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be created under test/ and end with .test.ts or .test.tsx
In tests that open network ports, always use port: 0; do not hardcode port numbers or roll your own random port
Use normalizeBunSnapshot to normalize snapshot output instead of comparing raw stdout
Do not write tests that merely assert absence of strings like "panic" or "uncaught exception"
Use tempDir from "harness" to create temporary directories; do not use fs.mkdtempSync or tmpdirSync
When spawning processes in tests, assert output before asserting the exit code
Avoid shell commands in tests (e.g., find, grep); prefer Bun's Glob and built-in tools
Prefer snapshot tests over direct string equality assertions on stdout
Files:
test/js/bun/util/filesystem_router.test.ts
🧠 Learnings (2)
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
docs/api/file-system-router.md
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/react-spa.test.ts : react-spa.test.ts should contain React SPA, react-refresh, and basic server component transform tests
Applied to files:
test/js/bun/util/filesystem_router.test.ts
🪛 markdownlint-cli2 (0.18.1)
docs/api/file-system-router.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (9)
packages/bun-types/bun.d.ts (2)
6011-6011: Constructor option type widening looks correct.
6025-6026: Readonly style property type widening looks correct.src/options.zig (3)
2400-2404: New RouteStyle enum is fine.Naming aligns with other enums; u8 backing is good.
2441-2441: Default.nextjsin zero() is appropriate.
2421-2429: Route configuration style field is not exposed in the public API schema; verify whether this omission is intentional or requires plumbing.The verification confirms the original concern:
RouteConfig.styleis defined (src/options.zig:2420) and used in router.zig, but:
api.LoadedRouteConfig(src/api/schema.zig:1526) does not include astylefieldtoAPI()(src/options.zig:2422) does not mapstyleto the returned API structfromLoadedRoutes()(src/options.zig:2444) does not restorestylefrom the APIIf
styleshould be configurable via the public API (as suggested by src/bun.js/api/filesystem_router.zig:42-44 extracting it from user input), addstyle: RouteStyletoapi.LoadedRouteConfigand updatetoAPI()andfromLoadedRoutes()to propagate it. Ifstyleis internal-only, document this constraint.src/bun.js/api/filesystem_router.zig (2)
131-132: Style plumbing looks correct.Passing style into Router.init, storing on FileSystemRouter, and using in reload is consistent.
Consider adding an integration test that omits style and expects default "nextjs" once constructor fallback is applied.
Also applies to: 168-169, 253-254
371-377: getStyle OK; consider aligning naming.Returns "react-router" and "nextjs" strings as expected by public API. No changes needed.
test/js/bun/util/filesystem_router.test.ts (1)
32-44: Helper looks good.Creates app/routes tree cleanly and normalizes paths for assertions.
src/router.zig (1)
18-161: Parser and matcher look sound.
- Correctly split on '/', '.', honor escapes [], optionals (), and $ for params/splats.
- Matching handles optional segments with backtracking; splat uses remaining() and param name "*".
- Routing branch for react_router normalizes trailing slashes and index path.
Add a parser unit test for nested optionals like "(admin.)users.$id" and escaped separators in nested segments to lock behavior.
Also applies to: 199-306, 562-580, 399-454
| ## React Router-style | ||
|
|
||
| `FileSystemRouter` also understands React Router's file routing conventions. Pass `style: "react-router"` and point `dir` at the directory containing your `routes` files (commonly `app/routes`). Bun will infer the same URL structure as React Router, including pathless layouts, optional segments, and splat routes. | ||
|
|
||
| ```txt | ||
| app/routes | ||
| ├── _index.tsx | ||
| ├── about.tsx | ||
| ├── concerts._index.tsx | ||
| ├── concerts.$city.tsx | ||
| ├── concerts.trending.tsx | ||
| ├── concerts_.mine.tsx | ||
| ├── _auth.login.tsx | ||
| ├── _auth.register.tsx | ||
| ├── ($lang).categories.tsx | ||
| ├── files.$.tsx | ||
| ├── $.tsx | ||
| ├── sitemap[.]xml.tsx | ||
| ├── dashboard | ||
| │ └── route.tsx | ||
| └── dashboard.projects.tsx | ||
| ``` | ||
|
|
||
| ```ts | ||
| const router = new Bun.FileSystemRouter({ | ||
| dir: "./app/routes", | ||
| style: "react-router", | ||
| }); | ||
|
|
||
| router.match("/concerts/salt-lake-city"); | ||
| // => { name: "/concerts/:city", params: { city: "salt-lake-city" } } | ||
|
|
||
| router.match("/categories"); | ||
| // => { name: "/:lang?/categories", params: {} } | ||
| ``` | ||
|
|
||
| Pathless layout files (prefixed with `_`) are ignored, optional segments created with parentheses are supported, and splat routes (`$.tsx`) capture the remainder of the path in the `*` parameter—matching the React Router documentation. |
There was a problem hiding this comment.
Clarify example shape and pathless layout wording.
- Examples only show
name/params; either show the fullMatchedRouteshape or add a brevity note. - “Pathless layout files … are ignored” can be misread; clarify “ignored for URL segments (they do not add a segment), but still participate in route nesting.”
Apply this diff:
@@
## React Router-style
@@
```ts
const router = new Bun.FileSystemRouter({
dir: "./app/routes",
style: "react-router",
});
router.match("/concerts/salt-lake-city");
-// => { name: "/concerts/:city", params: { city: "salt-lake-city" } }
+// => {
+// filePath: "/abs/path/app/routes/concerts.$city.tsx",
+// kind: "dynamic",
+// name: "/concerts/:city",
+// pathname: "/concerts/salt-lake-city",
+// src: "",
+// params: { city: "salt-lake-city" }
+// }
router.match("/categories");
-// => { name: "/:lang?/categories", params: {} }
+// => {
+// filePath: "/abs/path/app/routes/($lang).categories.tsx",
+// kind: "dynamic",
+// name: "/:lang?/categories",
+// pathname: "/categories",
+// src: "",
+// params: {}
+// }-Pathless layout files (prefixed with _) are ignored, optional segments created with parentheses are supported, and splat routes ($.tsx) capture the remainder of the path in the * parameter—matching the React Router documentation.
+Note: examples above omit unrelated fields where empty.
+
+Pathless layout files (prefixed with _) are ignored for URL segments (they don’t add a path segment), optional segments created with parentheses are supported, and splat routes ($.tsx) capture the remainder of the path in the * parameter—matching the React Router documentation.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/api/file-system-router.md around lines 86 to 122, the examples and
wording are ambiguous: update the two router.match examples to show the full
MatchedRoute shape (include filePath, kind, name, pathname, src, params) or, if
you want brevity, add a short note that examples omit unrelated empty fields;
then replace the sentence “Pathless layout files (prefixed with _) are
ignored” with clarified wording such as “Pathless layout files (prefixed with
_) are ignored for URL segments (they don’t add a path segment), but still
participate in route nesting,” and append the brief note “Note: examples above
omit unrelated fields where empty.” to match the requested diff.
</details>
<!-- fingerprinting:phantom:poseidon:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| const style: Options.RouteStyle = blk: { | ||
| if (try argument.get(globalThis, "style")) |style_val| { | ||
| const style_str = try style_val.getZigString(globalThis); | ||
| if (style_str.eqlComptime("nextjs")) break :blk .nextjs; | ||
| if (style_str.eqlComptime("react-router")) break :blk .react_router; | ||
| return globalThis.throwInvalidArguments("Invalid style. Expected 'nextjs' or 'react-router'", .{}); | ||
| } else { | ||
| return globalThis.throwInvalidArguments("Expected 'style' option (ex: \"style\": \"nextjs\")", .{}); | ||
| } | ||
| } else { | ||
| return globalThis.throwInvalidArguments("Expected 'style' option (ex: \"style\": \"nextjs\")", .{}); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Don’t make style mandatory; preserve default.
Requiring style breaks existing code that relied on implicit "nextjs". Default to .nextjs when absent; still validate when provided.
Apply:
- const style: Options.RouteStyle = blk: {
- if (try argument.get(globalThis, "style")) |style_val| {
- const style_str = try style_val.getZigString(globalThis);
- if (style_str.eqlComptime("nextjs")) break :blk .nextjs;
- if (style_str.eqlComptime("react-router")) break :blk .react_router;
- return globalThis.throwInvalidArguments("Invalid style. Expected 'nextjs' or 'react-router'", .{});
- } else {
- return globalThis.throwInvalidArguments("Expected 'style' option (ex: \"style\": \"nextjs\")", .{});
- }
- };
+ const style: Options.RouteStyle = blk: {
+ // Back-compat: default to nextjs when absent
+ var s: Options.RouteStyle = .nextjs;
+ if (try argument.get(globalThis, "style")) |style_val| {
+ const style_str = try style_val.getZigString(globalThis);
+ if (style_str.eqlComptime("nextjs")) break :blk .nextjs;
+ if (style_str.eqlComptime("react-router")) break :blk .react_router;
+ return globalThis.throwInvalidArguments("Invalid style. Expected 'nextjs' or 'react-router'", .{});
+ }
+ break :blk s;
+ };As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const style: Options.RouteStyle = blk: { | |
| if (try argument.get(globalThis, "style")) |style_val| { | |
| const style_str = try style_val.getZigString(globalThis); | |
| if (style_str.eqlComptime("nextjs")) break :blk .nextjs; | |
| if (style_str.eqlComptime("react-router")) break :blk .react_router; | |
| return globalThis.throwInvalidArguments("Invalid style. Expected 'nextjs' or 'react-router'", .{}); | |
| } else { | |
| return globalThis.throwInvalidArguments("Expected 'style' option (ex: \"style\": \"nextjs\")", .{}); | |
| } | |
| } else { | |
| return globalThis.throwInvalidArguments("Expected 'style' option (ex: \"style\": \"nextjs\")", .{}); | |
| } | |
| }; | |
| const style: Options.RouteStyle = blk: { | |
| // Back-compat: default to nextjs when absent | |
| var s: Options.RouteStyle = .nextjs; | |
| if (try argument.get(globalThis, "style")) |style_val| { | |
| const style_str = try style_val.getZigString(globalThis); | |
| if (style_str.eqlComptime("nextjs")) break :blk .nextjs; | |
| if (style_str.eqlComptime("react-router")) break :blk .react_router; | |
| return globalThis.throwInvalidArguments("Invalid style. Expected 'nextjs' or 'react-router'", .{}); | |
| } | |
| break :blk s; | |
| }; |
🤖 Prompt for AI Agents
In src/bun.js/api/filesystem_router.zig around lines 42 to 51, the current code
forces the caller to provide a "style" option and returns an error when missing;
change it to treat the option as optional by defaulting to
Options.RouteStyle.nextjs when the property is absent, and only perform
validation when a "style" value is present (returning the same invalid-argument
error if an unrecognized string is provided). Ensure the control flow sets style
to .nextjs on the else branch and preserves the existing validation branches for
"nextjs" and "react-router" when the option exists.
| it("react-router file routes", () => { | ||
| const { routesDir } = makeReactRouter([ | ||
| `_index.tsx`, | ||
| `about.tsx`, | ||
| `concerts._index.tsx`, | ||
| `concerts.$city.tsx`, | ||
| `concerts.trending.tsx`, | ||
| `concerts_.mine.tsx`, | ||
| `_auth.tsx`, | ||
| `_auth.login.tsx`, | ||
| `_auth.register.tsx`, | ||
| `($lang).categories.tsx`, | ||
| `files.$.tsx`, | ||
| `$.tsx`, | ||
| `sitemap[.]xml.tsx`, | ||
| `dashboard/route.tsx`, | ||
| `dashboard.projects.tsx`, | ||
| ]); | ||
|
|
||
| const router = new FileSystemRouter({ | ||
| dir: routesDir, | ||
| fileExtensions: [".tsx"], | ||
| style: "react-router", | ||
| }); | ||
|
|
||
| const routes = router.routes; | ||
| const fixture: Record<string, string> = { | ||
| "/": `${routesDir}/_index.tsx`, | ||
| "/about": `${routesDir}/about.tsx`, | ||
| "/concerts": `${routesDir}/concerts._index.tsx`, | ||
| "/concerts/:city": `${routesDir}/concerts.$city.tsx`, | ||
| "/concerts/trending": `${routesDir}/concerts.trending.tsx`, | ||
| "/concerts/mine": `${routesDir}/concerts_.mine.tsx`, | ||
| "/login": `${routesDir}/_auth.login.tsx`, | ||
| "/register": `${routesDir}/_auth.register.tsx`, | ||
| "/:lang?/categories": `${routesDir}/($lang).categories.tsx`, | ||
| "/files/*": `${routesDir}/files.$.tsx`, | ||
| "/sitemap.xml": `${routesDir}/sitemap[.]xml.tsx`, | ||
| "/dashboard": `${routesDir}/dashboard/route.tsx`, | ||
| "/dashboard/projects": `${routesDir}/dashboard.projects.tsx`, | ||
| "/*": `${routesDir}/$.tsx`, | ||
| }; | ||
|
|
||
| expect(Object.keys(routes).sort()).toEqual(Object.keys(fixture).sort()); | ||
| for (const route in fixture) { | ||
| expect(routes[route]).toBe(fixture[route]); | ||
| } | ||
|
|
||
| const dynamic = router.match("/concerts/salt-lake-city")!; | ||
| expect(dynamic.name).toBe("/concerts/:city"); | ||
| expect(dynamic.filePath).toBe(`${routesDir}/concerts.$city.tsx`); | ||
| expect(dynamic.params.city).toBe("salt-lake-city"); | ||
|
|
||
| const optionalMissing = router.match("/categories")!; | ||
| expect(optionalMissing.name).toBe("/:lang?/categories"); | ||
| expect(optionalMissing.filePath).toBe(`${routesDir}/($lang).categories.tsx`); | ||
| expect("lang" in optionalMissing.params).toBe(false); | ||
|
|
||
| const optionalPresent = router.match("/en/categories")!; | ||
| expect(optionalPresent.params.lang).toBe("en"); | ||
|
|
||
| const catchAll = router.match("/files/talks/react.pdf")!; | ||
| expect(catchAll.name).toBe("/files/*"); | ||
| expect(catchAll.params["*"]).toBe("talks/react.pdf"); | ||
|
|
||
| const globalCatchAll = router.match("/totally/unmatched")!; | ||
| expect(globalCatchAll.name).toBe("/*"); | ||
| expect(globalCatchAll.params["*"]).toBe("totally/unmatched"); | ||
|
|
||
| const login = router.match("/login")!; | ||
| expect(login.name).toBe("/login"); | ||
| expect(Object.keys(login.params).length).toBe(0); | ||
|
|
||
| const sitemap = router.match("/sitemap.xml")!; | ||
| expect(sitemap.name).toBe("/sitemap.xml"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Great E2E coverage for React Router style. Add one default-style test.
After restoring constructor fallback, add a test that omits style and still matches Next.js routes to guard back-compat.
Example:
it("defaults to nextjs when style omitted", () => {
const { dir } = make(["index.tsx"]);
// Note: no style
const router = new FileSystemRouter({ dir, fileExtensions: [".tsx"] });
expect(router.match("/")!.name).toBe("/");
expect(router.style).toBe("nextjs");
});Based on learnings.
🤖 Prompt for AI Agents
In test/js/bun/util/filesystem_router.test.ts around lines 108 to 183, add a
small test that verifies the FileSystemRouter falls back to "nextjs" when the
style option is omitted: create a minimal fixture dir (e.g. using
make(["index.tsx"])), construct FileSystemRouter with dir and fileExtensions
only (no style), assert router.match("/")!.name === "/" and router.style ===
"nextjs". Place this new it(...) test near the other router-style tests so it
runs with the same helpers and teardown.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/router.zig (1)
1027-1041: Route.parse dispatch is fine; avoid unused config warning in parseNext.You already silence config in parseNext with _ = config. Consider moving config-independent helpers shared by both styles to reduce duplication between parseNext/parseReactRouter.
♻️ Duplicate comments (1)
docs/api/file-system-router.md (1)
88-122: Clarify example shape and pathless layout wording (unchanged).This repeats prior feedback: either show the full MatchedRoute shape in the react-router examples or add a note that unrelated fields are omitted; also clarify “ignored” to “ignored for URL segments (do not add a segment)”.
Proposed diff:
@@ router.match("/concerts/salt-lake-city"); -// => { name: "/concerts/:city", params: { city: "salt-lake-city" } } +// => { +// filePath: "/abs/path/app/routes/concerts.$city.tsx", +// kind: "dynamic", +// name: "/concerts/:city", +// pathname: "/concerts/salt-lake-city", +// src: "", +// params: { city: "salt-lake-city" } +// } @@ router.match("/categories"); -// => { name: "/:lang?/categories", params: {} } +// => { +// filePath: "/abs/path/app/routes/($lang).categories.tsx", +// kind: "dynamic", +// name: "/:lang?/categories", +// pathname: "/categories", +// src: "", +// params: {} +// } @@ -Pathless layout files (prefixed with `_`) are ignored, optional segments created with parentheses are supported, and splat routes (`$.tsx`) capture the remainder of the path in the `*` parameter—matching the React Router documentation. +Note: examples above omit unrelated empty fields. + +Pathless layout files (prefixed with `_`) are ignored for URL segments (they don’t add a path segment), optional segments created with parentheses are supported, and splat routes (`$.tsx`) capture the remainder of the path in the `*` parameter—matching the React Router documentation.Also applies to: 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/api/file-system-router.md(2 hunks)packages/bun-types/bun.d.ts(3 hunks)src/router.zig(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/router.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/router.zig
🔇 Additional comments (4)
src/router.zig (1)
199-206: Case sensitivity diverges between static and dynamic matching. Decide and align.Static routes are stored with both lowercased match_name and the original-cased key (when has_uppercase), enabling case-insensitive matches. Here, for dynamic routes with static prefixes, static segments are compared with strings.eql (case-sensitive). This means “/Foo/:id” will not match “/foo/123”, unlike a purely static “/Foo”.
Either:
- Make static segment comparisons case-insensitive (preferred for parity with Next style default), or
- Document/enforce case-sensitive routing for react-router style across both static and dynamic.
If choosing case-insensitive matching, here’s a targeted change:
- if (strings.eql(value, segment.name)) { + if (strings.eqlCaseInsensitive(value, segment.name)) {Or pass a flag derived from route.has_uppercase to select comparison per-route. Please confirm intended behavior.
Also applies to: 215-246, 257-276, 278-296, 300-306
packages/bun-types/bun.d.ts (3)
5996-5997: LGTM - Default value documented.The JSDoc now correctly documents the default value as requested in the previous review. The formatting is clear and follows standard JSDoc conventions.
6025-6025: LGTM - Type consistency maintained.The readonly property type correctly matches the constructor parameter type, ensuring type consistency throughout the
FileSystemRouterclass. Thereadonlymodifier is appropriate since the routing style should be immutable after instantiation.
6011-6011: No issues found with the type definition change.The type definition correctly narrows
styleto"nextjs" | "react-router", which aligns with the implementation. Verification confirms:
- Implementation explicitly handles both styles with runtime validation:
if (style_str.eqlComptime("react-router")) break :blk .react_router;and enforces only valid options- Comprehensive test coverage exists for react-router routing with distinct file naming conventions (
$city,$lang,$.tsx) that differ from nextjs patterns- The style parameter directly affects route parsing—tests use different filename conventions per style and validate against corresponding route patterns, confirming the style setting influences routing behavior
| const ReactRouterParser = struct { | ||
| const Segment = struct { | ||
| segment: []const u8, | ||
| raw: []const u8, | ||
| }; | ||
|
|
||
| const State = enum { | ||
| normal, | ||
| escape, | ||
| optional, | ||
| optional_escape, | ||
| }; | ||
|
|
||
| fn isSeparator(char: u8) bool { | ||
| return char == '/' or char == '.' or char == '\\'; | ||
| } | ||
|
|
||
| fn pushSegment( | ||
| allocator: std.mem.Allocator, | ||
| segments: *std.ArrayList(Segment), | ||
| segment_builder: *std.ArrayList(u8), | ||
| raw_builder: *std.ArrayList(u8), | ||
| ) !void { | ||
| if (segment_builder.items.len == 0) { | ||
| raw_builder.clearRetainingCapacity(); | ||
| return; | ||
| } | ||
|
|
||
| const seg_slice = try allocator.dupe(u8, segment_builder.items); | ||
| const raw_slice = try allocator.dupe(u8, raw_builder.items); | ||
| try segments.append(.{ .segment = seg_slice, .raw = raw_slice }); | ||
| segment_builder.clearRetainingCapacity(); | ||
| raw_builder.clearRetainingCapacity(); | ||
| } |
There was a problem hiding this comment.
Allocator hygiene: Segment/RAW buffers leak and tie lifetime to global allocator. Intern names instead.
ReactRouterParser.parse dupes segment/raw and returns an owned slice; parseReactRouter then copies names into ReactSegment without interning. The returned arrays (and inner dupes) are never freed, and ReactSegment.name points into those buffers, making cleanup impossible after a reload. Prefer interning names into FileSystem.DirnameStore (or a route-scoped arena) and avoid retaining the parser’s temporary arrays.
Suggested approach:
- In parseReactRouter, after deriving base_slice/param_name, append them to DirnameStore and store the interned slice on ReactSegment.
- Drop the need to retain the parser’s Segment list entirely (free it or avoid allocating dupes in the parser by writing directly into a threadlocal buffer + DirnameStore like the name/match_name code).
Example change sketch inside the loop in parseReactRouter (see detailed diff below at Lines 1283-1336):
- For dynamic: intern param_name = base_slice[1..].
- For static: intern base_slice.
- For splat: keep name "*" as a single shared interned string.
This removes per-route leaks and clearly establishes ownership.
Also applies to: 53-61, 65-71, 95-107, 108-146, 148-151
🤖 Prompt for AI Agents
In src/router.zig around lines 18 to 51, the parser currently dupes segment/raw
buffers with the global allocator and returns owned slices that live beyond the
parser, causing memory leaks and tying route name lifetimes to those temporary
allocations; instead, intern all segment names (static names, dynamic param
names without the leading ':', and the single splat "*" token) into
FileSystem.DirnameStore (or a route-scoped arena) and store the interned slices
on ReactSegment, avoid duplicating into the parser-owned ArrayList entirely
(write directly into the intern store or a thread-local buffer then intern), and
free/stop retaining the parser buffers; update pushSegment/parse so they do not
allocator.dupe for long-lived slices, remove or free the temporary segments
array after parse, and ensure ReactSegment.name points to interned memory
managed by DirnameStore to eliminate per-route leaks and clarify ownership.
| pub fn matchPageWithAllocator(this: *Routes, _: string, url_path: URLPath, params: *Param.List, allocator: std.mem.Allocator) ?Match { | ||
| if (this.config.style == .react_router) { | ||
| var path = url_path.path; | ||
|
|
||
| if (path.len > 0 and path[path.len - 1] == '/') { | ||
| path = path[0 .. path.len - 1]; | ||
| } | ||
|
|
||
| while (path.len > 1 and path[path.len - 1] == '/') { | ||
| path = path[0 .. path.len - 1]; | ||
| } | ||
|
|
||
| if (strings.eqlComptime(path, ".")) { | ||
| path = ""; | ||
| } | ||
|
|
||
| if (path.len == 0) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Normalize path once; remove duplicate trailing-slash trims.
The react-router branch trims trailing slashes twice. Simplify to a single canonicalization:
- if (path.len > 0 and path[path.len - 1] == '/') {
- path = path[0 .. path.len - 1];
- }
- while (path.len > 1 and path[path.len - 1] == '/') {
- path = path[0 .. path.len - 1];
- }
+ while (path.len > 1 and path[path.len - 1] == '/') {
+ path = path[0 .. path.len - 1];
+ }Small cleanup, fewer branches.
Also applies to: 417-431, 433-451, 453-455
🤖 Prompt for AI Agents
In src/router.zig around lines 399-415, the react_router branch canonicalizes
trailing slashes twice; remove the duplicate trimming logic and replace with a
single canonicalization that: trims one trailing '/' if path ends with '/', then
collapses any further duplicate trailing slashes in a single loop, converts "."
to empty string, and preserves behavior when path.len == 0. Apply the same
single-pass canonicalization cleanup to the other react_router blocks at lines
417-431, 433-451, and 453-455 so all branches use the same simplified
normalization logic.
| fn match(this: *Routes, allocator: std.mem.Allocator, pathname_: string, comptime MatchContext: type, ctx: MatchContext) ?*Route { | ||
| if (this.config.style == .react_router) { | ||
| return this.matchReact(allocator, pathname_, MatchContext, ctx); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dispatch looks correct; static-first then dynamic for react-router.
Branching to matchReact when style == .react_router is clear, and static map gets precedence. Consider a brief comment noting that catch-all ordering still relies on Route.Sorter (routing specificity).
Also applies to: 562-580
🤖 Prompt for AI Agents
In src/router.zig around lines 547-551 (and likewise at 562-580), add a brief
inline comment above the branch to matchReact explaining that the dispatch is
static-first then dynamic for react-router and that catch-all ordering is
handled by Route.Sorter (routing specificity), so the current branching
intentionally defers ordering to Route.Sorter; insert the comment in both places
to document intent.
| style_data: StyleData = .{ .none = {} }, | ||
|
|
||
| pub const Ptr = TinyPtr; |
There was a problem hiding this comment.
Carry only interned, owned slices in style_data.
style_data.react_router.segments points to ReactSegment.name slices that currently reference transient parser allocations. After interning names in DirnameStore, you can safely keep []const ReactSegment without leaking and with clear lifetime equal to route lifetime. See parseReactRouter change below.
Also applies to: 1006-1026
🤖 Prompt for AI Agents
In src/router.zig around lines 929 to 931 (and similarly 1006–1026), style_data
currently holds ReactSegment.name slices that reference transient parser
allocations; change style_data to carry only interned, owned slices so no
transient pointers are kept. Modify parseReactRouter (and any code that
constructs ReactSegment) to intern segment names into DirnameStore and store the
returned owned slice pointer/length in the ReactSegment; then change style_data
to keep []const ReactSegment (or the equivalent owned slice type) that refers
only to those interned names so lifetimes match the route lifetime and no parser
heap is leaked.
| fn parseReactRouter( | ||
| base_: string, | ||
| extname: string, | ||
| entry: *Fs.FileSystem.Entry, | ||
| log: *Logger.Log, | ||
| allocator: std.mem.Allocator, | ||
| public_dir_: string, | ||
| routes_dirname_len: u16, | ||
| config: Options.RouteConfig, | ||
| ) ?Route { | ||
| _ = config; | ||
| var abs_path_str: string = if (entry.abs_path.isEmpty()) | ||
| "" | ||
| else | ||
| entry.abs_path.slice(); | ||
|
|
||
| const base = base_[0 .. base_.len - extname.len]; | ||
| const public_dir = std.mem.trim(u8, public_dir_, std.fs.path.sep_str); | ||
|
|
||
| var public_path: string = brk: { | ||
| if (base.len == 0) break :brk public_dir; | ||
| var buf: []u8 = &route_file_buf; | ||
|
|
||
| if (public_dir.len > 0) { | ||
| route_file_buf[0] = '/'; | ||
| buf = buf[1..]; | ||
| bun.copy(u8, buf, public_dir); | ||
| } | ||
| buf[public_dir.len] = '/'; | ||
| buf = buf[public_dir.len + 1 ..]; | ||
| bun.copy(u8, buf, base); | ||
| buf = buf[base.len..]; | ||
| bun.copy(u8, buf, extname); | ||
| buf = buf[extname.len..]; | ||
|
|
||
| if (comptime Environment.isWindows) { | ||
| bun.path.platformToPosixInPlace(u8, route_file_buf[0 .. @intFromPtr(buf.ptr) - @intFromPtr(&route_file_buf)]); | ||
| } | ||
|
|
||
| break :brk route_file_buf[0 .. @intFromPtr(buf.ptr) - @intFromPtr(&route_file_buf)]; | ||
| }; | ||
|
|
||
| var name_prefix = public_path[0 .. public_path.len - extname.len]; | ||
|
|
||
| while (name_prefix.len > 1 and name_prefix[name_prefix.len - 1] == '/') { | ||
| name_prefix = name_prefix[0 .. name_prefix.len - 1]; | ||
| } | ||
|
|
||
| name_prefix = name_prefix[routes_dirname_len..]; | ||
| name_prefix = std.mem.trimRight(u8, name_prefix, "/"); | ||
|
|
||
| const route_id_full = if (name_prefix.len > 0 and name_prefix[0] == '/') name_prefix[1..] else name_prefix; | ||
| var normalized_route_id = route_id_full; | ||
|
|
||
| if (normalized_route_id.len > 0 and strings.endsWith(normalized_route_id, "/route")) { | ||
| normalized_route_id = normalized_route_id[0 .. normalized_route_id.len - "/route".len]; | ||
| } | ||
|
|
||
| const segments = ReactRouterParser.parse(allocator, normalized_route_id) catch { | ||
| log.addErrorFmt(null, Logger.Loc.Empty, allocator, "Invalid React Router route: {s}", .{normalized_route_id}) catch unreachable; | ||
| return null; | ||
| }; | ||
|
|
||
| const is_index_route = normalized_route_id.len == 0 or strings.endsWith(normalized_route_id, "_index"); | ||
|
|
||
| var effective_segments = segments; | ||
| if (is_index_route and segments.len > 0) { | ||
| effective_segments = segments[0 .. segments.len - 1]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Intern React segment names to prevent leaks; remove dependency on parser-owned buffers.
Within parseReactRouter, ReactSegment.name is set to slices into parser-owned buffers. Intern names when building match_builder so the route owns stable memory and the parser arrays can be discarded.
Apply this minimal diff in the segment loop:
@@
- var optional = false;
- var base_slice = trimmed;
+ var optional = false;
+ var base_slice = trimmed;
if (base_slice.len > 0 and base_slice[base_slice.len - 1] == '?') {
optional = true;
base_slice = base_slice[0 .. base_slice.len - 1];
}
@@
- switch (base_slice[0]) {
+ switch (base_slice[0]) {
':' => {
if (route_kind == .static) route_kind = .dynamic;
param_count += 1;
- const param_name = base_slice[1..];
+ const param_name = base_slice[1..];
if (param_name.len == 0) {
log.addErrorFmt(null, Logger.Loc.Empty, allocator, "Missing parameter name in route: {s}", .{normalized_route_id}) catch unreachable;
return null;
}
+ const interned = FileSystem.DirnameStore.instance.append(@TypeOf(param_name), param_name) catch unreachable;
match_builder.append(.{
.kind = .dynamic,
- .name = param_name,
+ .name = interned,
.optional = optional,
}) catch unreachable;
},
'*' => {
route_kind = if (optional) .optional_catch_all else .catch_all;
param_count += 1;
+ const star = FileSystem.DirnameStore.instance.append(@TypeOf("*"), "*") catch unreachable;
match_builder.append(.{
.kind = .splat,
- .name = "*",
+ .name = star,
.optional = optional,
}) catch unreachable;
},
else => {
if (optional) {
has_optional_static = true;
}
+ const interned_static = FileSystem.DirnameStore.instance.append(@TypeOf(base_slice), base_slice) catch unreachable;
match_builder.append(.{
.kind = .static,
- .name = base_slice,
+ .name = interned_static,
.optional = optional,
}) catch unreachable;
},
}Optionally, free (or avoid allocating) ReactRouterParser.parse’s Segment/raw arrays after building match_segments_slice, since all referenced strings are now interned.
Also applies to: 1275-1336, 1338-1350, 1351-1369, 1375-1388, 1389-1403, 1404-1410, 1411-1428
1 participant
What does this PR do?
Implements the react router file based routing style for bun.
https://reactrouter.com/how-to/file-route-conventions
How did you verify your code works?
I didn't everything was coded using ChatGPT Agents.
I would love to use this feature in the company I work for but I don't know how to code in Rust.
EDIT: I don't have the skills to apply all the AI coderabbit suggestions. Can someone take the lead of this please?