feat(framework): generate the preview framework code #1
Conversation
WalkthroughEnd-to-end routing was rewritten: scanner, generator, and router now support async matching, nested layouts, dynamic and catch‑all segments; a new shared router-utils module plus virtual/client re-exports were added; many tests and fixtures were introduced; Vite was pinned in multiple package.json files; small playground and CI tweaks applied. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant Router as Router.svelte
participant Scanner as RouteScanner
participant Generator as RouteGenerator
participant Utils as RouterUtils
participant Layout
participant Page
Browser->>Router: navigate(path)
Router->>Scanner: async findMatchingRoute(path)
Scanner-->>Router: matched route nodes + params
Router->>Generator: async resolve imports & helpers
Generator-->>Router: moduleCode + typeDefinitions
Router->>Utils: setRouterContext(routerContext)
Router->>Layout: render layout stack
Layout->>Page: render page component
Page-->>Router: rendered fragment
Router-->>Browser: update DOM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
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: 5
🧹 Nitpick comments (14)
apps/playground/src/app/note.ts (1)
10-15: Confirm the returned value shape and avoid leaking driver metadataYou now return whatever
db.insert(note).values(...)yields. Please double‑check that this is actually the created note (or array of notes) and not a driver‑specific metadata object (e.g., a run result / command result). If consumers expect the created note, you likely want something along the lines of:- const result = await db.insert(note).values({ - name: input.name, - content: input.content, - userId: context.user?.id, - }); - return result; + const [createdNote] = await db + .insert(note) + .values({ + name: input.name, + content: input.content, + userId: context.user.id, + }) + .returning(); + + return createdNote;This also avoids returning raw DB result structures and tight coupling to the DB client.
Minor: since you early‑return when
!context.user, you can safely drop the optional chaining and usecontext.user.iddirectly.packages/framework/src/scanner.ts (4)
1-2: Unused import:basename.The
basenamefunction is imported but never used in the file.-import { join, relative, extname, basename } from "node:path"; +import { join, relative, extname } from "node:path";
47-49: Consider logging or distinguishing error types.The empty catch block handles the expected case of a missing directory, but also silently swallows permission errors or other unexpected failures. For debugging purposes, consider checking the error code or adding debug-level logging.
- } catch { - // Directory doesn't exist + } catch (error: unknown) { + // Only silently ignore ENOENT (directory doesn't exist) + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn(`Warning: Could not scan directory ${dir}:`, error); + } }
249-258: Unnecessary dot character in validation regex.The comment mentions dots are allowed for catch-all routes like
[...rest], but the three dots in[...rest]are stripped with the extension before validation. After removing.sveltefrom[...rest].svelte, the remaining[...rest]doesn't require literal dots in the pattern since they're inside brackets. Allowing dots could permit unexpected filenames likefoo..bar.svelte.- // Allow: a-z, A-Z, 0-9, _, -, [, ], (, ), . (for catch-all routes like [...rest]) + // Allow: a-z, A-Z, 0-9, _, -, [, ], (, ) for dynamic and catch-all routes // Reject files with spaces, @, $, and other special characters - const validPattern = /^[a-zA-Z0-9_\-\[\]().]+$/; + const validPattern = /^[a-zA-Z0-9_\-\[\]()]+$/;
119-202: Consider caching for performance with large route trees.The
processLayoutLevelmethod iterates overrouteMapfor each layout file found, resulting in O(n²) complexity. For typical applications with fewer than 100 routes this is fine, but for monorepos or large apps, consider building an index by path prefix for O(n) lookups.packages/framework/src/generator.ts (3)
41-43: Window shadowing pattern may cause confusion.Declaring
const windowshadows the global window object. While this works for SSR compatibility, consider using a more explicit name likebrowserWindoworglobalWindowto avoid confusion during debugging.-// Declare window for SSR compatibility -const window = typeof globalThis !== 'undefined' && 'window' in globalThis ? globalThis.window : undefined; +// Reference browser window for SSR compatibility (undefined in non-browser environments) +const browserWindow = typeof globalThis !== 'undefined' && 'window' in globalThis ? globalThis.window : undefined;Then update references on lines 61-62 and 66-67 accordingly.
353-360: Simplify catch-all parameter extraction.The logic to find the catch-all parameter name is unnecessarily complex. It can be simplified by directly accessing the last element of the params array.
- const catchAllParam = route.params?.find(p => - route.params.indexOf(p) === route.params.length - 1 - ) || 'catchAll'; + const catchAllParam = route.params?.[route.params.length - 1] || 'catchAll';
291-295: Consider secondary sort criteria for route stability.The sort only uses route type priority (static=1, dynamic=2, catch-all=3). Routes with the same priority retain their original order, which may vary. Consider adding path length or specificity as a secondary criterion for more deterministic matching.
const sortedRoutes = allRoutes.sort((a, b) => { const aPriority = getRoutePriority(a.fullPath || a.path); const bPriority = getRoutePriority(b.fullPath || b.path); - return aPriority - bPriority; + if (aPriority !== bPriority) return aPriority - bPriority; + // Secondary: more segments = more specific + const aSegments = (a.fullPath || a.path).split('/').filter(Boolean).length; + const bSegments = (b.fullPath || b.path).split('/').filter(Boolean).length; + return bSegments - aSegments; });packages/framework/tests/fixtures/app/(auth)/login.svelte (1)
2-6: Unusedparamsprop.The
paramsprop is declared but never used in this component. Consider removing it if not needed, or use it to pre-fill form fields if that was the intent.- interface Props { - params?: Record<string, string>; - } - - let { params = {} }: Props = $props(); + // No props needed for this fixturepackages/framework/tests/scanner.test.ts (1)
33-38: Usedirnameinstead ofjoin(path, "..")for parent directory.Using
join(fullPath, "..")is less readable than the idiomaticdirname(fullPath)fromnode:path.+import { join, dirname } from "node:path"; ... async function createFile(path: string, content = "<div>test</div>") { const fullPath = join(appDir, path); - const dir = join(fullPath, ".."); + const dir = dirname(fullPath); await mkdir(dir, { recursive: true }); await writeFile(fullPath, content); }packages/framework/tests/virtual.test.ts (1)
225-276: TypeScript interface tests don't validate types at runtime.These tests create object literals and check
typeofon properties, but they don't actually verify TypeScript interface compliance. Thetypeof virtualModule.Locationreturnsundefinedat runtime since interfaces don't exist in JavaScript. Consider removing these tests or converting them to compile-time type checks in a separate.d.tstest file.If you want to verify type exports, consider using a type-only test file:
// virtual.types.test.ts (compile-time only) import type { Location, RouteParams, Route, Router } from "../src/virtual"; // These will fail compilation if types are wrong const _location: Location = { pathname: "/", search: "", hash: "" }; const _params: RouteParams = { id: "123" };packages/framework/src/components/router.svelte (3)
96-121: Missingawaiton async route search.
searchRoutesis declared asasyncand returns aPromise, butfindMatchingRoutereturnssearchRoutes(routes)without awaiting it. This meansfindMatchingRoutereturns a Promise, which is then awaited inupdateLocation(line 226), so it works but the function signature is misleading.Either make the outer function explicitly async and await the result, or make the naming clearer:
- function findMatchingRoute(pathname: string): any { + async function findMatchingRoute(pathname: string): Promise<any> { async function searchRoutes(routeList: any[], parentPath = '', ancestors: any[] = []): Promise<any> { // ... } - return searchRoutes(routes); + return await searchRoutes(routes); }
449-462: Redundant props spreading.The component receives
params={currentParams}and also spreads{...currentParams}. This passes each parameter twice - once underparamsand once at the top level. If any param name conflicts with a component prop, this could cause issues.Choose one approach for passing params:
{#if comp.type === 'layout'} - <Component params={currentParams} {...currentParams}> + <Component params={currentParams}> {#snippet children()} {@render renderComponents(components, index + 1)} {/snippet} </Component> {:else} - <Component params={currentParams} {...currentParams} /> + <Component params={currentParams} /> {/if}Or if spreading is preferred for convenience, document this convention and be aware of potential conflicts.
364-368: Silent error catch may hide issues.The empty catch block silently swallows errors during SSR initialization. Consider logging at debug level or being more specific about expected exceptions.
} catch { - // Contexts not available during client-side rendering + // Contexts not available during client-side rendering - this is expected + // and not an error condition }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
apps/playground/package.json(1 hunks)apps/playground/src/app/note.ts(1 hunks)apps/playground/src/server.ts(0 hunks)apps/playground/vite.config.ts(1 hunks)packages/framework/package.json(1 hunks)packages/framework/src/components/router.svelte(11 hunks)packages/framework/src/generator.ts(4 hunks)packages/framework/src/index.ts(1 hunks)packages/framework/src/plugin.ts(1 hunks)packages/framework/src/scanner.ts(8 hunks)packages/framework/src/virtual.ts(1 hunks)packages/framework/tests/fixtures/app/(auth)/login.svelte(1 hunks)packages/framework/tests/fixtures/app/about.svelte(1 hunks)packages/framework/tests/fixtures/app/catch-all.svelte(1 hunks)packages/framework/tests/fixtures/app/index.svelte(1 hunks)packages/framework/tests/fixtures/app/users.svelte(1 hunks)packages/framework/tests/fixtures/app/users/[id].svelte(1 hunks)packages/framework/tests/fixtures/app/users/index.svelte(1 hunks)packages/framework/tests/generator.test.ts(1 hunks)packages/framework/tests/scanner.test.ts(1 hunks)packages/framework/tests/virtual.test.ts(1 hunks)packages/framework/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/playground/src/server.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/framework/src/virtual.ts (1)
packages/framework/src/index.ts (8)
Location(13-13)RouteParams(14-14)Route(15-15)Router(16-16)useRouter(18-18)useRoute(18-18)href(18-18)setRouterContext(18-18)
packages/framework/tests/generator.test.ts (2)
packages/framework/src/index.ts (2)
PluginContext(8-8)RouteNode(6-6)packages/framework/src/generator.ts (1)
RouteGenerator(3-467)
packages/framework/tests/scanner.test.ts (2)
packages/framework/src/index.ts (1)
PluginContext(8-8)packages/framework/src/scanner.ts (1)
RouteScanner(5-355)
packages/framework/src/generator.ts (1)
packages/framework/src/types.ts (1)
RouteNode(27-38)
🔇 Additional comments (35)
apps/playground/vite.config.ts (1)
29-29: Plugin order is correct—oxide()must run first to provide virtual modules.The
oxide()plugin is correctly positioned first in the plugins array because it needs to register the virtual module system (e.g.,$oxide) that downstream code depends on. The server.ts file imports from this virtual module (import { router } from "$oxide"), confirming thatoxide()must initialize before other plugins process files.The plugin order is not arbitrary—it's essential for the framework to function. No reordering is needed.
packages/framework/tsconfig.json (1)
2-5: LGTM!The addition of DOM and ES2022 library typings aligns with the framework's new routing utilities that use
URLSearchParamsand modern JavaScript features.packages/framework/tests/fixtures/app/index.svelte (1)
1-12: LGTM!The component correctly demonstrates the props interface pattern used throughout the fixture set. The unused
paramsprop is acceptable for a test fixture.packages/framework/package.json (1)
29-30: LGTM!The test script addition enables the new test suite introduced in this PR and follows Bun's standard testing conventions.
packages/framework/tests/fixtures/app/users/index.svelte (1)
1-21: LGTM!The fixture appropriately demonstrates a user list component with hardcoded test data. The unused
paramsprop maintains consistency with other fixtures in the test suite.packages/framework/tests/fixtures/app/users.svelte (1)
1-17: LGTM!The layout component correctly demonstrates Svelte 5's Snippet pattern for nested routing. The props interface and children rendering follow best practices.
packages/framework/tests/fixtures/app/users/[id].svelte (1)
1-22: LGTM!The dynamic route component correctly demonstrates Svelte 5's
$derivedrunes for reactive computation. The fallback to empty string for missing params and "Unknown User" for unrecognized IDs provides safe defaults for test scenarios.packages/framework/src/plugin.ts (1)
301-304: LGTM!The refactor to assign the result to a local variable before returning improves code readability while maintaining the same functionality. The cleanup of debug logs (mentioned in the summary) reduces noise during development.
packages/framework/src/virtual.ts (5)
1-35: LGTM!The interface definitions are well-structured and the SSR-safe window declaration appropriately guards against server-side access. Using a Symbol for the context key is best practice to prevent naming collisions. The separation of public exports (Location, RouteParams, Route, Router) from internal types (RouterContext) maintains a clean API surface.
37-62: LGTM!The
useRouter()implementation correctly:
- Validates context availability with a clear error message
- Delegates navigation to the context's navigate method
- Guards
back()andforward()with SSR-safe window checksThe silent no-op behavior for history navigation in SSR environments is appropriate since server-side rendering has no browser history to manipulate.
64-80: LGTM!The
useRoute()implementation correctly extracts route information and creates aURLSearchParamsinstance for convenient query parameter access. The error handling is consistent withuseRouter(), and the DOM typing support added intsconfig.jsonenables the URLSearchParams usage.
82-100: LGTM!The
href()template tag helper provides a clean API for URL construction with proper handling of:
- URLSearchParams for query strings
- Arrays for path segments
- Automatic encoding via
encodeURIComponentfor other valuesThe defensive
?? ""operators handle edge cases gracefully, and the explicitString()conversion before encoding ensures predictable behavior for all value types.
102-104: LGTM!The
setRouterContext()wrapper correctly delegates to Svelte'ssetContextwith the private symbol key. This establishes the router context for downstreamuseRouter()anduseRoute()calls.packages/framework/src/index.ts (1)
10-18: LGTM! Clean public API consolidation.The aliasing of
Routertype asRouterInterfacecorrectly avoids naming collision with theRoutercomponent exported on line 3. The virtual module exports are well-organized.packages/framework/src/scanner.ts (1)
85-117: LGTM! Layout organization logic is correct.The algorithm properly handles both layout-based routes and standalone routes that aren't part of any layout hierarchy.
packages/framework/src/generator.ts (2)
399-430: LGTM! Type definitions are well-structured.The generated types properly expose
RouteRecordwith optionalfullPath, and theRouteParamsinterface provides strong typing for route parameters.
46-89: Well-implemented router hooks with clear error handling.The
useRouteranduseRoutefunctions correctly check for context and provide helpful error messages. The SSR-safe window checks forback()andforward()are appropriate.packages/framework/tests/fixtures/app/about.svelte (1)
1-12: LGTM as test fixture.The component correctly uses Svelte 5 runes syntax. The
paramsprop is declared but unused in the template, which is acceptable for a test fixture that may be testing prop passing without displaying the values.packages/framework/tests/fixtures/app/catch-all.svelte (1)
1-18: LGTM! Good catch-all route fixture.The component correctly demonstrates catch-all route parameter usage with Svelte 5's
$derivedrune. The conditional display of the path and fallback to empty string are appropriate patterns.packages/framework/tests/scanner.test.ts (6)
40-49: LGTM!Good test coverage for basic route file scanning with clear assertions on file detection.
75-93: LGTM!Solid test coverage for dynamic route parameter extraction and naming conventions.
95-111: LGTM!Good coverage of catch-all route handling with proper parameter extraction.
179-189: LGTM!Good test for filtering out invalid file names - validates scanner security against potentially problematic file names.
250-259: LGTM!Important test ensuring hidden directories and
node_modulesare properly excluded from scanning.
148-158: Child route paths are relative to parent layout by design.The test correctly expects child routes under a layout to have relative paths (
"/index"and"/profile"). This is the intended behavior in the scanner implementation—child routes are scoped relative to their parent layout, not absolute from the application root. Root-level routes have absolute paths (e.g.,"/login","/users/:id"), while layout children use relative paths within their parent scope.packages/framework/tests/virtual.test.ts (2)
23-56: LGTM!Good test coverage for
useRouterincluding error handling when called outside context and verification of navigation methods.
147-206: LGTM!Comprehensive test coverage for the
hreftemplate tag with various interpolation scenarios including URL encoding, arrays, URLSearchParams, and edge cases.packages/framework/tests/generator.test.ts (5)
23-36: LGTM!Well-designed helper function with sensible defaults that reduces test boilerplate while allowing targeted overrides.
70-92: LGTM!Good test verifying the default async import behavior with negative assertion to ensure sync imports aren't generated.
94-121: LGTM!Properly tests the sync import mode by reconfiguring the context and verifying both import statement and component reference generation.
278-318: LGTM!Comprehensive test for type definition generation including RouteRecord interface, RouteNames union type, RouteParams interface with per-route parameter types, and virtual module function signatures.
368-393: LGTM!Important test ensuring non-component routes (layouts without their own component) are filtered from the generated output.
packages/framework/src/components/router.svelte (3)
4-24: LGTM!Clean type definitions for Location and RouteParams with proper Symbol-based context key for isolation.
132-202: LGTM!Well-structured route matching implementation with proper handling of exact matches, dynamic segments, and catch-all routes. The URL decoding of path segments is a good security practice.
278-314: LGTM!Good separation of layout and page component loading with proper error handling and state management.
| <div class="error">{error}</div> | ||
| {/if} | ||
|
|
||
| <form onsubmit={handleSubmit}> |
There was a problem hiding this comment.
Missing preventDefault on form submit.
The onsubmit handler doesn't prevent the default form submission, which will cause a full page reload before the handleSubmit logic completes.
- <form onsubmit={handleSubmit}>
+ <form onsubmit={(e) => { e.preventDefault(); handleSubmit(); }}>📝 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.
| <form onsubmit={handleSubmit}> | |
| <form onsubmit={(e) => { e.preventDefault(); handleSubmit(); }}> |
🤖 Prompt for AI Agents
In packages/framework/tests/fixtures/app/(auth)/login.svelte around line 35, the
form uses a plain onsubmit attribute and doesn't prevent the browser's default
submit behavior; replace it with Svelte's event modifier to prevent default and
wire the handler properly by changing the attribute to
on:submit|preventDefault={handleSubmit} (or alternatively ensure handleSubmit
receives the event and calls event.preventDefault()) so the page won't reload
before your submit logic runs.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/framework/src/components/router.svelte (1)
95-120: Missingawaiton asyncsearchRoutescall.
searchRoutesis declared asasync(line 96) but the call at line 119 is not awaited. This causesfindMatchingRouteto return a Promise wrapping undefined instead of the actual match result.- return searchRoutes(routes); + return await searchRoutes(routes);
♻️ Duplicate comments (1)
packages/framework/src/components/router.svelte (1)
165-176: Catch-all parameter extraction logic remains fragile.This was previously flagged: the condition
route.params.indexOf(p) === route.params.length - 1selects the last param regardless of whether it's actually the catch-all. This logic depends on implementation details of the scanner that may not hold.
🧹 Nitpick comments (2)
packages/framework/src/shared/router-utils.ts (1)
94-98: Array elements inhrefare not URL-encoded.When an array is passed,
value.join('/')concatenates without encoding individual elements. If array elements contain special characters (e.g.,/,?,#), the resulting URL could be malformed.Consider encoding each array element:
} else if (Array.isArray(value)) { - result += value.join("/"); + result += value.map(v => encodeURIComponent(String(v))).join("/"); } else {packages/framework/src/components/router.svelte (1)
5-19: Consider importing types from the shared module instead of re-declaring.
Location,RouteParams, andRouterContextare already defined inpackages/framework/src/shared/router-utils.ts. Re-declaring them here creates duplication that may drift out of sync.- import { ROUTER_CONTEXT_KEY } from '@oxidejs/framework/client'; + import { ROUTER_CONTEXT_KEY, type Location, type RouteParams, type RouterContext } from '@oxidejs/framework/client'; - export interface Location { - pathname: string; - search: string; - hash: string; - } - - export interface RouteParams { - [key: string]: string; - } - - interface RouterContext { - navigate: (path: string, options?: { replace?: boolean }) => void; - location: () => Location; - params: () => RouteParams; - } + export type { Location, RouteParams };Note: You'll need to add
RouterContextto the client.ts exports if it's not already exported as a type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/framework/bunup.config.ts(1 hunks)packages/framework/package.json(2 hunks)packages/framework/src/client.ts(1 hunks)packages/framework/src/components/router.svelte(11 hunks)packages/framework/src/generator.ts(5 hunks)packages/framework/src/index.ts(1 hunks)packages/framework/src/plugin.ts(2 hunks)packages/framework/src/shared/router-utils.ts(1 hunks)packages/framework/src/virtual.ts(1 hunks)packages/framework/tests/generator.test.ts(1 hunks)packages/framework/tests/virtual.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/framework/tests/virtual.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/framework/src/plugin.ts
- packages/framework/tests/generator.test.ts
- packages/framework/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/framework/src/generator.ts (2)
packages/framework/src/index.ts (1)
RouteNode(6-6)packages/framework/src/types.ts (1)
RouteNode(27-38)
🔇 Additional comments (10)
packages/framework/src/shared/router-utils.ts (2)
3-30: LGTM!The type definitions are well-structured and provide a clear contract for the routing API surface.
32-39: SSR-safe window shim is correctly implemented.The guard for
globalThis.windowensures safe operation during server-side rendering.packages/framework/bunup.config.ts (1)
13-19: LGTM!The browser-targeted build configuration correctly complements the main Node build and aligns with the new
./clientexport inpackage.json.packages/framework/package.json (2)
18-21: LGTM!The new
./clientexport correctly exposes the browser-safe module surface with proper type definitions.
39-39: Vite 7.2.6 is a patch release with no breaking changes. This version is safe to use and does not introduce any compatibility concerns with the framework.packages/framework/src/client.ts (1)
1-11: LGTM!The browser-safe client module correctly re-exports only the minimal runtime value (
ROUTER_CONTEXT_KEY) and types needed for client-side use, avoiding Node.js dependencies.packages/framework/src/components/router.svelte (2)
3-3: Good fix for the Symbol mismatch issue.Importing
ROUTER_CONTEXT_KEYfrom@oxidejs/framework/clientensures the same Symbol instance is used across the codebase, resolving the previously flagged critical issue.
448-462: LGTM!The recursive
renderComponentssnippet cleanly handles nested layout rendering with proper slot-based composition for child components.packages/framework/src/virtual.ts (1)
1-12: LGTM!Clean barrel export that centralizes the public API surface from the shared router utilities module.
packages/framework/src/generator.ts (1)
220-257: LGTM on type generation.The generated TypeScript interfaces correctly mirror the runtime types and provide proper type safety for route names and parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/framework/src/components/router.svelte (1)
169-179: Simplify catch-all parameter extraction logic.The catch-all parameter extraction combines multiple fallback strategies (
startsWith('catch-'), position-based, literal 'catchAll'), making it fragile. Consider simplifying by relying on a single, consistent convention from the scanner.Apply this diff to simplify:
- // Look for explicit catch- prefixed params, fallback to 'catchAll' - const catchAllParam = route.params?.find((p: string) => p.startsWith('catch-')) || 'catchAll'; - const remainingSegments = pathSegments.slice(catchAllIndex); - const paramName = catchAllParam.startsWith('catch-') - ? catchAllParam.replace('catch-', '') - : catchAllParam; + // Use the last param as catch-all, defaulting to 'catchAll' if none defined + const paramName = route.params?.[route.params.length - 1] || 'catchAll'; params[paramName] = remainingSegments.join('/');packages/framework/src/generator.ts (1)
84-91: Enhance matchRoute to support catch-all routes.The current implementation only handles basic dynamic segments (
:param) but not catch-all routes (*), which are now supported by the router. Consider extending the regex pattern to handle catch-all segments.Apply this diff to add catch-all support:
export function matchRoute(pathname) { const flattened = flattenRoutes(routes); return flattened.find(route => { - const routePath = route.path.replace(/:[^/]+/g, '[^/]+'); + const routePath = route.path + .replace(/:[^/]+/g, '[^/]+') // Dynamic segments + .replace(/\*/g, '.*'); // Catch-all segments const regex = new RegExp(\`^\${routePath}$\`); return regex.test(pathname); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/framework/src/components/router.svelte(11 hunks)packages/framework/src/generator.ts(4 hunks)packages/framework/tests/fixtures/app/(auth)/login.svelte(1 hunks)packages/framework/tests/generator.test.ts(1 hunks)packages/framework/tests/virtual.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/framework/tests/fixtures/app/(auth)/login.svelte
🧰 Additional context used
🧬 Code graph analysis (2)
packages/framework/tests/generator.test.ts (2)
packages/framework/src/index.ts (2)
PluginContext(8-8)RouteNode(6-6)packages/framework/src/generator.ts (1)
RouteGenerator(9-346)
packages/framework/src/generator.ts (2)
packages/framework/src/index.ts (1)
RouteNode(6-6)packages/framework/src/types.ts (1)
RouteNode(27-38)
🔇 Additional comments (7)
packages/framework/tests/generator.test.ts (1)
1-423: Excellent test coverage for RouteGenerator.The test suite comprehensively validates the new
generate()API surface and covers all major scenarios including async/sync imports, route parameters, metadata, aliases, nested routes, helper functions, type definitions, catch-all routes, and edge cases like routes without parameters. The tests are well-structured and follow a consistent pattern.packages/framework/tests/virtual.test.ts (1)
1-268: Comprehensive test suite for the virtual module API.The tests thoroughly validate all public exports (
useRouter,useRoute,href,setRouterContext) and their behavior, including error handling when called outside router context, URL construction with encoding, and TypeScript interface contracts. The use ofmock.moduleand dynamic imports ensures clean test isolation.packages/framework/src/components/router.svelte (2)
228-262: Robust error handling in updateLocation.The try-catch block properly wraps the async route matching logic and handles all failure scenarios (no match, catch-all fallback, errors), ensuring the router state remains consistent.
452-467: Clean recursive rendering pattern for nested layouts.The
renderComponentssnippet elegantly handles nested layout rendering by recursively calling itself for children. The distinction between layout and page components is clear, and the implementation follows Svelte 5 conventions.packages/framework/src/generator.ts (3)
44-49: Correct use of shared module re-exports.Re-exporting router utilities from the shared module ensures a single
ROUTER_CONTEXT_KEYSymbol instance is used throughout the codebase, preventing context mismatch issues.
287-308: Well-structured parameter type generation.The
generateParamInterfacesmethod correctly generates per-route parameter typings, usingRecord<string, never>for routes without parameters and properly handling nested routes through recursion.
72-82: This review concern is not valid. The template literal escaping at lines 78 and 88 is correct and produces syntactically valid JavaScript. The escaped backticks (\``) in the template literal source code properly render as literal backticks in the generated output, creating valid template literals (e.g., ``:${key}` ``) that will interpolate correctly at runtime. The generated utility functions have been verified to be syntactically correct.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/framework/src/generator.ts (1)
168-171: Optional: Simplify child filtering logic.The second
.filter((entry) => entry)is redundant sinceprocessRoutealready returns""for routes without components (line 135), and the first filter already checkshasComponent.For clarity, you could either:
- Keep both filters for explicitness (current approach is fine)
- Remove the second filter and rely on the first:
const childEntries = route.children .filter((child) => child.hasComponent) - .map((child) => processRoute(child, depth + 1)) - .filter((entry) => entry); + .map((child) => processRoute(child, depth + 1));packages/framework/tests/generator.test.ts (1)
1-423: LGTM! Comprehensive test coverage for the generator.The test suite thoroughly covers:
- Basic route generation
- Import mode switching (async/sync)
- Route features (parameters, meta, aliases, nesting)
- Helper function generation
- Type definition generation
- Edge cases (catch-all params, routes without components)
Consider adding tests for edge cases:
- Empty route arrays
- Routes with circular references in children
- Invalid route data (missing required fields)
- Very deeply nested routes (performance/stack overflow)
These would improve robustness but are not critical for the current implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/playground/.gitignore(1 hunks)packages/framework/src/client.ts(1 hunks)packages/framework/src/generator.ts(4 hunks)packages/framework/tests/generator.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/playground/.gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
packages/framework/tests/generator.test.ts (1)
packages/framework/src/generator.ts (1)
RouteGenerator(9-346)
packages/framework/src/generator.ts (2)
packages/framework/src/index.ts (1)
RouteNode(6-6)packages/framework/src/types.ts (1)
RouteNode(27-38)
🔇 Additional comments (7)
packages/framework/src/client.ts (3)
1-3: LGTM!The comment block clearly explains the purpose and rationale for this browser-safe client module.
4-10: All exported symbols exist in the source module and no Node.js dependencies detected.The exports for
ROUTER_CONTEXT_KEY,useRouter,useRoute,href, andsetRouterContextare all properly defined in./shared/router-utils.ts. No Node.js-specific code (fs, path, process, etc.) was detected, confirming browser-safety.
11-17: Verify type exports and renaming consistency.The
Routertype is renamed toRouterInterfaceon export. All five exported types (Location,RouteParams,Router,Route,RouterContext) are confirmed to exist in the source module./shared/router-utils.js. The renaming is intentional and consistent: bothclient.tsandindex.tsuseRouter as RouterInterfaceto avoid naming conflicts with theRoutercomponent also exported fromindex.ts.packages/framework/src/generator.ts (4)
1-7: LGTM! Clean interface definition.The new
GeneratedOutputinterface properly separates module code from type definitions, improving the API surface for consumers.
16-42: LGTM! Public API change is well-designed.The updated signature
generate(routes: RouteNode[]): GeneratedOutputprovides better structure for consumers who need both module code and type definitions separately.
44-49: LGTM! Symbol duplication issue properly resolved.Re-exporting from the shared module ensures a single Symbol instance is used across the codebase, correctly addressing the previous review concern.
209-345: LGTM! Type generation is comprehensive and handles edge cases.The type generation methods properly handle empty routes (using
"never"), generate per-route parameter types, and create a complete module declaration for downstream consumers.
| export function generatePath(name, params = {}) { | ||
| const route = findRouteByName(name); | ||
| if (!route) return null; | ||
| let path = route.path; | ||
| Object.entries(params).forEach(([key, value]) => { | ||
| path = path.replace(new RegExp(\`:\${key}\`, 'g'), encodeURIComponent(String(value))); | ||
| }); | ||
| return path; | ||
| } |
There was a problem hiding this comment.
Performance: RegExp created on every iteration.
The generatePath function creates a new RegExp object for each parameter, which is inefficient when a route has multiple parameters.
Consider caching the regex or using a single pass replacement:
export function generatePath(name, params = {}) {
const route = findRouteByName(name);
if (!route) return null;
- let path = route.path;
- Object.entries(params).forEach(([key, value]) => {
- path = path.replace(new RegExp(\`:\${key}\`, 'g'), encodeURIComponent(String(value)));
- });
-
- return path;
+ return route.path.replace(/:([^/]+)/g, (match, paramName) => {
+ return paramName in params
+ ? encodeURIComponent(String(params[paramName]))
+ : match;
+ });
}🤖 Prompt for AI Agents
In packages/framework/src/generator.ts around lines 72–82, the current loop
creates a new RegExp for each param which is wasteful; change to a single-pass
replacement or cache regexes: replace the loop with one call to
path.replace(/:([A-Za-z0-9_]+)/g, (m, key) => { if key in params return
encodeURIComponent(String(params[key])); return m; }) or, if you prefer per-key
regexes, build a Map of RegExp objects outside the loop and reuse them when
replacing each param so you avoid allocating a new RegExp per iteration.
| export function matchRoute(pathname) { | ||
| const flattened = flattenRoutes(routes); | ||
| return flattened.find(route => { | ||
| const routePath = route.path.replace(/:[^/]+/g, '[^/]+'); | ||
| const regex = new RegExp(\`^\${routePath}$\`); | ||
| return regex.test(pathname); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Incomplete route matching: catch-all and optional parameters not handled.
The matchRoute function only handles dynamic segments (:param) but doesn't support catch-all routes (*) or optional parameters, which are mentioned in the test suite and likely supported by the scanner.
Verify the scanner's route pattern capabilities and ensure matchRoute handles all supported patterns:
#!/bin/bash
# Check if scanner produces catch-all or optional parameter patterns
rg -nC3 --type=ts "catch.*all|optional.*param|\\\*|\\[.*\\]" packages/framework/src/scanner.ts🤖 Prompt for AI Agents
In packages/framework/src/generator.ts around lines 84 to 91, matchRoute
currently only converts :param segments to [^/]+ and builds a RegExp, which
misses catch-all (*) and optional parameters (e.g. :id? or [..rest] style) used
by the scanner; update the conversion logic to: first escape literal regex
characters in route.path, then replace catch-all tokens (e.g. '*' or '[...name]'
or ':...name') with '.*' (or '(?:.*)' as appropriate), replace optional params
(':name?' or '[name?]') with '(?:/[^/]+)?' so the segment is optional, and
replace required params (':name' or '[name]') with '/[^/]+'. Ensure
leading/trailing slashes are handled consistently and anchors ^ and $ remain;
rebuild the RegExp from that transformed string so matchRoute supports dynamic,
optional and catch-all patterns consistent with the scanner.
| export function getRouteParams(pathname, route) { | ||
| const params = {}; | ||
| const pathParts = pathname.split('/').filter(Boolean); | ||
| const routeParts = route.path.split('/').filter(Boolean); | ||
| return `${imports}${routesArray}\n\nexport default routes;\n\n${helpers}`; | ||
| routeParts.forEach((part, index) => { | ||
| if (part.startsWith(':')) { | ||
| const paramName = part.slice(1); | ||
| params[paramName] = pathParts[index]; | ||
| } | ||
| }); | ||
| return params; | ||
| } |
There was a problem hiding this comment.
Missing URI decoding and catch-all support in parameter extraction.
getRouteParams should decode URI components (user input may contain encoded characters) and doesn't handle catch-all routes that capture multiple segments.
Apply this diff to decode parameters:
export function getRouteParams(pathname, route) {
const params = {};
const pathParts = pathname.split('/').filter(Boolean);
const routeParts = route.path.split('/').filter(Boolean);
routeParts.forEach((part, index) => {
if (part.startsWith(':')) {
const paramName = part.slice(1);
- params[paramName] = pathParts[index];
+ params[paramName] = decodeURIComponent(pathParts[index] || '');
}
});
return params;
}Additionally, verify if catch-all routes need special handling here.
📝 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.
| export function getRouteParams(pathname, route) { | |
| const params = {}; | |
| const pathParts = pathname.split('/').filter(Boolean); | |
| const routeParts = route.path.split('/').filter(Boolean); | |
| return `${imports}${routesArray}\n\nexport default routes;\n\n${helpers}`; | |
| routeParts.forEach((part, index) => { | |
| if (part.startsWith(':')) { | |
| const paramName = part.slice(1); | |
| params[paramName] = pathParts[index]; | |
| } | |
| }); | |
| return params; | |
| } | |
| export function getRouteParams(pathname, route) { | |
| const params = {}; | |
| const pathParts = pathname.split('/').filter(Boolean); | |
| const routeParts = route.path.split('/').filter(Boolean); | |
| routeParts.forEach((part, index) => { | |
| if (part.startsWith(':')) { | |
| const paramName = part.slice(1); | |
| params[paramName] = decodeURIComponent(pathParts[index] || ''); | |
| } | |
| }); | |
| return params; | |
| } |
commit: |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.