-
-
Notifications
You must be signed in to change notification settings - Fork 26
Fix incorrect priority on route matching #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: f264a12 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughPre-filters and pre-sorts route candidates in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/server/helpers/match.ts (2)
60-62: Consider pre-computing scores to improve performance.The
scoreRoutefunction is invoked multiple times for each route during the sort operation, causing redundant parsing of route paths. For better performance, especially with many routes, consider computing scores once before sorting.🔎 Proposed optimisation
- const sortedRoutes = Object.entries(appPathRoutes) - .filter(([routePath, file]) => routePath && file) - .sort(([a], [b]) => scoreRoute(b) - scoreRoute(a)); + const sortedRoutes = Object.entries(appPathRoutes) + .filter(([routePath, file]) => routePath && file) + .map(([routePath, file]) => ({ routePath, file, score: scoreRoute(routePath) })) + .sort((a, b) => b.score - a.score) + .map(({ routePath, file }) => [routePath, file] as const);
64-70: Early exit pattern improves clarity and efficiency.The refactoring to return immediately upon finding a match is cleaner and more efficient than accumulating a result. Good change.
Note: The function can return
undefinedwhen no route matches. Consider adding an explicit return type annotation (e.g.,{ filename: string; params: Record<string, string | string[]> } | undefined) for better type safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/server/helpers/match.ts (1)
73-89: Critical: Catch-all segments have the same priority as dynamic segments.While the root route fix (Infinity) and optional catch-all distinction are now correct, a critical bug remains: catch-all segments
[...param](line 83) receive the same score (+2) as dynamic segments[param](line 84). This means routes like/user/[id]and/blog/[...slug]have equal priority, even though the dynamic segment should be more specific and match first.Expected priority (high to low):
- Static segments
about→ +3 ✓- Dynamic segments
[param]→ +2 ✓- Catch-all
[...param]→ should be lower (e.g., +1)- Optional catch-all
[[...param]]→ -1 ✓🔎 Proposed fix
function scoreRoute(routePath: string) { const parts = routePath.split('/').filter(Boolean); let score = 0; // Root path if(parts.length === 0) score = Infinity; for (const part of parts) { // Optional catch-all if (part.startsWith('[[...')) score += -1; // Catch-all - else if (part.startsWith('[...')) score += 2; + else if (part.startsWith('[...')) score += 1; else if (part.startsWith('[')) score += 2; else score += 3; } return score + parts.length; }
🧹 Nitpick comments (1)
src/server/helpers/match.ts (1)
60-62: Verify thatfileis non-empty before destructuring.The filter checks that
fileis truthy, but sincefileis an array, an empty array would pass the filter yet causefilePathto beundefinedduring destructuring at line 64. Consider strengthening the filter to verify the array has at least one element.🔎 Proposed fix
const sortedRoutes = Object.entries(appPathRoutes) - .filter(([routePath, file]) => routePath && file) + .filter(([routePath, file]) => routePath && file && file.length > 0) .sort(([a], [b]) => scoreRoute(b) - scoreRoute(a));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/helpers/match.ts
🔇 Additional comments (1)
src/server/helpers/match.ts (1)
64-70: Good refactor: early return simplifies control flow.The early return pattern is cleaner than the previous
matchedRouteaccumulation approach. Note that the function implicitly returnsundefinedwhen no route matches, so ensure downstream code handles this case appropriately.
|
That should be clear to merge. |
|
Build is failing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/helpers/match.ts (1)
47-73: Add explicit return type annotation tofindMatchingRoute.With strict TypeScript enabled, the function should have an explicit return type. Add
ReturnTypeannotation to clarify what this function returns:export function findMatchingRoute( nextServer: NextNodeServer, requestPathname: string, ): { filename: string; params: Record<string, string | string[]> } | undefinedThis improves type safety and helps the TypeScript compiler with type inference for callers.
🧹 Nitpick comments (2)
src/server/helpers/match.ts (2)
64-72: LGTM: Early return on first match is a good pattern.The early return improves readability and ensures the highest-priority match is returned immediately.
Minor observation: the guard on line 65 (
if (!filePath) continue) is redundant given the filter on line 61 already ensures non-empty arrays. Consider removing it for cleaner code.
75-91: Previous review concerns have been addressed.The scoring logic now correctly handles:
- Root path
/receivesInfinityscore (highest priority)- Optional catch-all
[[...param]]receives-1(lower than catch-all's0)This establishes the correct priority order: static > dynamic > catch-all > optional catch-all.
Minor style nit: missing space after
ifon line 80.🔎 Optional style fix
- if(parts.length === 0) score = Infinity; + if (parts.length === 0) score = Infinity;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/helpers/match.ts
🔇 Additional comments (1)
src/server/helpers/match.ts (1)
60-62: LGTM: Route sorting logic is correct.The filtering and descending sort by score ensures more specific routes are evaluated first, which is the correct approach for route-matching priority.
|
Linter is failing too |
|
oh my god a single space |
Summary by CodeRabbit
Performance Improvements
Bug Fixes / Refactor
✏️ Tip: You can customize this high-level summary in your review settings.