Exclude manage from routing #437
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe updates introduce a permanent redirect from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Next.js Server
participant Middleware
User->>Next.js Server: Request /nl/manage
Next.js Server->>Next.js Server: Check redirects()
Next.js Server-->>User: 301 Redirect to /manage
User->>Next.js Server: Request /manage
Next.js Server->>Middleware: Path matching
Middleware-->>Next.js Server: Skip i18n processing (excluded path)
Next.js Server-->>User: Serve /manage content
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: e2e
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
middleware.js (4)
3-15: Consider handling path variants.The middleware correctly blocks the exact paths '/manage' and '/nl/manage', but does not account for trailing slashes or other path variations. Users might still access these routes by adding a trailing slash (e.g., '/manage/').
Consider using a more flexible path matching approach:
- if (url.pathname === '/manage' || url.pathname === '/nl/manage') { + if (url.pathname === '/manage' || url.pathname === '/nl/manage' || + url.pathname === '/manage/' || url.pathname === '/nl/manage/') {Or even better, use a regex pattern:
- if (url.pathname === '/manage' || url.pathname === '/nl/manage') { + if (/^\/(?:nl\/)?manage\/?$/.test(url.pathname)) {🧰 Tools
🪛 ESLint
[error] 8-11: Replace
⏎······{·message:·'Not·Found'·},⏎······{·status:·404·}⏎····with{·message:·'Not·Found'·},·{·status:·404·}(prettier/prettier)
8-11: Format JSON response per linting rules.There's a formatting issue flagged by ESLint regarding the JSON response structure.
- return NextResponse.json( - { message: 'Not Found' }, - { status: 404 } - ) + return NextResponse.json({ message: 'Not Found' }, { status: 404 })🧰 Tools
🪛 ESLint
[error] 8-11: Replace
⏎······{·message:·'Not·Found'·},⏎······{·status:·404·}⏎····with{·message:·'Not·Found'·},·{·status:·404·}(prettier/prettier)
17-19: Config setup looks good, but add a newline.The matcher configuration correctly targets the intended paths. There's a minor linting issue about adding a newline at the end of the file.
export const config = { matcher: ['/manage', '/nl/manage'], // Apply middleware to these paths } +🧰 Tools
🪛 ESLint
[error] 19-19: Insert
⏎(prettier/prettier)
7-12: Potential redundancy in path checking.The middleware function checks for specific paths that are already specified in the
matcherconfiguration. While this adds an extra layer of validation, it's worth noting that thematcheralready ensures the middleware only runs on these paths.If you prefer to keep the explicit check (which is fine for clarity), you might consider making the comment reflect this redundant validation:
- // Block /manage and /nl/manage by returning a 404 response + // Double-check paths and block /manage and /nl/manage by returning a 404 response🧰 Tools
🪛 ESLint
[error] 8-11: Replace
⏎······{·message:·'Not·Found'·},⏎······{·status:·404·}⏎····with{·message:·'Not·Found'·},·{·status:·404·}(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware.js(1 hunks)
🧰 Additional context used
🪛 ESLint
middleware.js
[error] 8-11: Replace ⏎······{·message:·'Not·Found'·},⏎······{·status:·404·}⏎···· with {·message:·'Not·Found'·},·{·status:·404·}
(prettier/prettier)
[error] 19-19: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (1)
middleware.js (1)
1-1: Good import choice.Correctly importing
NextResponsefrom 'next/server' which is the appropriate Next.js API for handling middleware responses.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
next.config.js (1)
6-14: Consider updating the comment to match the actual implementationThe comment on line 10 says "Redirect to a 404 page or any other page", but the actual destination is
/manage. This is misleading since you're not redirecting to a 404 page but rather consolidating the paths. According to the summary, you're using middleware to intercept both the/manageand/nl/managepaths to return a 404 JSON response.Consider updating the comment to better reflect what's happening, such as:
- destination: '/manage', // Redirect to a 404 page or any other page + destination: '/manage', // Consolidate Dutch path to the main manage path (both handled by middleware)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (2)
next.config.js (2)
6-14: Verify middleware implementation for both pathsThis redirect from
/nl/manageto/manageworks as intended only if your middleware is correctly intercepting both paths as mentioned in the summary. Make sure that the middleware handling is properly implemented to return 404 responses for both/manageand/nl/managepaths.Since the redirect will send users from
/nl/manageto/manage, you only need to ensure the middleware handles the/managepath properly, as users will never actually see the/nl/managepath.
6-14: LGTM: The previous infinite redirect loop is fixedThe current implementation correctly resolves the previous issue where
/managewas redirecting to itself, creating an infinite loop. Now you're only redirecting/nl/manageto/manage, which works well with the middleware approach described in the summary.
Summary by CodeRabbit
New Features
Refactor