-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Implement 404 redirect handling with middleware #16563
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
base: master
Are you sure you want to change the base?
Conversation
…mHQ/pipedream into danny/fixing-docs-apps-redirect
…mHQ/pipedream into danny/fixing-docs-apps-redirect
…mHQ/pipedream into danny/fixing-docs-apps-redirect
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Warning Rate limit exceeded@dannyroosevelt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a middleware to handle unmatched routes by redirecting them to the home page with a 301 status, while excluding static assets and API paths. Additionally, a custom 404 page component is added as a fallback, which also redirects users to the home page after a brief delay. A minor whitespace cleanup is made in an API documentation file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Middleware
participant 404Page
User->>Middleware: Request to unknown path
alt Path is static/API/file
Middleware-->>User: Proceed as normal
else Path is unmatched
Middleware-->>User: 301 Redirect to "/"
end
User->>404Page: If middleware did not redirect, render 404 page
404Page-->>User: Show "Not Found" message
404Page-->>User: After 100ms, redirect to "/"
Possibly related PRs
Poem
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 (
|
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)
docs-v2/middleware.ts (1)
9-41
: Consider adding a safety check to prevent redirect loops.The current implementation might create redirect loops if there are issues with the matcher configuration. Consider adding logic to detect and prevent infinite redirects.
export function middleware(request: NextRequest) { // We only want to handle 404s, not other pages // This check isn't foolproof but helps avoid redirecting existing pages const pathname = request.nextUrl.pathname + + // Prevent redirect loops + if (pathname === "/") { + return NextResponse.next() + } // Check if this is a static asset or API route - we don't want to redirect these if ( pathname.startsWith("/_next") || pathname.startsWith("/api/") || pathname.startsWith("/images/") || pathname.includes(".") // Likely a file, e.g. favicon.ico ) { return NextResponse.next() } // Return a 301 (permanent) redirect to the home page return NextResponse.redirect(new URL("/", request.url), 301) }🧰 Tools
🪛 GitHub Check: Lint Code Base
[failure] 41-41:
Newline required at end of file but not found🪛 GitHub Actions: Pull Request Checks
[error] 41-41: ESLint: Newline required at end of file but not found (eol-last).
docs-v2/pages/404.tsx (1)
24-36
: Consider enhancing the user experience with styling.While the current implementation is functional, you might want to match the styling with your site's design system for a more cohesive look and feel.
Adding your brand colors, typography, and potentially a logo would make the fallback page feel more integrated with the rest of the site.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.gitignore
(1 hunks)docs-v2/middleware.ts
(1 hunks)docs-v2/next.config.mjs
(3 hunks)docs-v2/pages/404.tsx
(1 hunks)docs-v2/pages/apps/apps.mdx
(0 hunks)docs-v2/pages/connect/api.mdx
(9 hunks)docs-v2/pages/connect/components.mdx
(1 hunks)docs-v2/pages/connect/environments.mdx
(1 hunks)docs-v2/pages/connect/managed-auth/connect-link.mdx
(1 hunks)docs-v2/pages/connect/managed-auth/quickstart.mdx
(2 hunks)docs-v2/pages/connect/managed-auth/tokens.mdx
(2 hunks)docs-v2/pages/connect/managed-auth/users.mdx
(1 hunks)docs-v2/pages/connect/managed-auth/webhooks.mdx
(1 hunks)docs-v2/pages/privacy-and-security/index.mdx
(1 hunks)
💤 Files with no reviewable changes (1)
- docs-v2/pages/apps/apps.mdx
🧰 Additional context used
🪛 GitHub Actions: Pull Request Checks
.gitignore
[warning] 1-1: File ignored because of a matching ignore pattern. Use "--no-ignore" to disable file ignore settings or use "--no-warn-ignored" to suppress this warning.
docs-v2/middleware.ts
[error] 41-41: ESLint: Newline required at end of file but not found (eol-last).
🪛 GitHub Check: Lint Code Base
docs-v2/middleware.ts
[failure] 41-41:
Newline required at end of file but not found
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pnpm publish
🔇 Additional comments (27)
.gitignore (1)
22-22
: LGTM on ignoring Claude settings file.Adding the Claude AI settings local file pattern to gitignore ensures that local configuration doesn't get committed to the repository.
docs-v2/middleware.ts (2)
15-22
: Good implementation of middleware exclusions.The checks correctly identify static assets, API routes, and files with extensions that should bypass the 404 middleware handling, ensuring that these important paths remain functional.
24-26
: Effective implementation of 301 redirect.Using permanent redirects (301) is the correct approach for SEO as it informs search engines that the redirect is permanent and they should update their indexes.
docs-v2/pages/404.tsx (1)
12-22
: Well-implemented fallback mechanism.The useEffect hook with a short timeout is a good approach to ensure the middleware has a chance to run first, and the cleanup function properly handles the timeout when the component unmounts.
docs-v2/next.config.mjs (4)
37-41
: Good cleanup of redundant paths.This redirect correctly handles users who might navigate to the redundant
/apps/apps/
path, ensuring they're properly redirected to the simpler/apps/
path.
484-488
: Proper route consolidation for integrations.Redirecting
/integrations/
to/apps/
aligns with the existing pattern for path-specific redirects and helps consolidate related content under a single URL structure.
569-572
: Appropriate rewrite for consistency in URL structure.Using a rewrite instead of a redirect for the errors path is suitable here since it preserves the user-facing URL while serving the content from the new location.
37-572
:✅ Verification successful
Verify that removed redirects don't break existing links.
I notice that with the addition of these new redirects, there might have been some removals of previous redirects. Ensure that any removed redirects don't break existing external links or bookmarks pointing to your documentation.
🏁 Script executed:
#!/bin/bash # Look for evidence of removed redirects in the diff # Check the git diff for removed redirect entries git diff HEAD~1 docs-v2/next.config.mjs | grep -B 5 -A 5 "^-.*source:.*destination:"Length of output: 86
No removed redirects detected
I ran a diff against the base branch and found no removedsource
→destination
entries in docs-v2/next.config.mjs. All existing redirects remain intact, so there’s no risk of breaking current links.docs-v2/pages/connect/managed-auth/users.mdx (1)
72-72
: Link anchor updated correctly
The fragment/connect/api/#delete-end-user
now matches the renamed section header "Delete end user" and is consistent with other Connect API references.docs-v2/pages/connect/managed-auth/webhooks.mdx (1)
3-3
: API link fragment corrected
The link now points to/connect/api/#create-token
, aligning with the updated section header "Create token".docs-v2/pages/privacy-and-security/index.mdx (1)
127-127
: Server-side token link updated
The anchor/connect/api/#create-token
correctly reflects the renamed "Create token" section.docs-v2/pages/connect/managed-auth/connect-link.mdx (1)
30-30
: Redirect parameter link corrected
Updated to/connect/api/#create-token
, ensuring consistency with the new API anchor.docs-v2/pages/connect/managed-auth/tokens.mdx (2)
20-20
: Token creation endpoint link fixed
The fragment/connect/api/#create-token
now matches the revised header "Create token".
30-30
: Token scope link updated
Correctly references/connect/api/#create-token
for token creation, aligning with the updated docs.docs-v2/pages/connect/environments.mdx (1)
30-30
: Anchor Fragment Update for Consistency
The link to the "Create token" section has been updated to/connect/api/#create-token
, matching the renamed header in the API reference. This ensures the anchor is valid and keeps documentation consistent.docs-v2/pages/connect/components.mdx (1)
836-836
: Corrected Link to Updated Anchor
The hyperlink now points to/connect/api/#update-webhooks-listening-to-deployed-trigger
, aligning with the new header ID. Good catch for consistency across the Connect docs.docs-v2/pages/connect/managed-auth/quickstart.mdx (2)
81-81
: Updated API Anchor Link
The link now uses/connect/api/#create-token
, reflecting the revised API section header. This maintains accuracy in the quickstart callout.
124-124
: Consistent Reference to Token Creation Endpoint
The anchor link has been corrected to/connect/api/#create-token
here as well, ensuring uniformity in all documentation examples.docs-v2/pages/connect/api.mdx (9)
70-70
: Adjusted Fragment for Short-Lived Token Section
The link in the browser SDK overview now correctly references the "Create token" anchor (#create-token
), matching the updated header in this document.
243-243
: Header Renamed to Simplify Anchor
The section header was updated to "Create token" (from "Create a new token"), which streamlines the documentation and improves anchor readability.
1002-1002
: Simplified Header for End User Deletion
The header now reads "Delete end user", removing the article for consistency with other section titles.
2691-2691
: Consistent Naming for Trigger Deletion Section
The section header "Delete deployed trigger" aligns naming conventions and matches the updated anchor used elsewhere.
2801-2801
: Normalized Title for Event Retrieval
The header "Retrieve events emitted by deployed trigger" has been standardized by removing extra wording, ensuring consistency across retrieval endpoints.
2998-2998
: Aligned Header for Webhook Retrieval
The section header now reads "Retrieve webhooks listening to deployed trigger", matching the corresponding anchor in related docs.
3121-3121
: Updated Title for Webhook Update Endpoint
The header "Update webhooks listening to deployed trigger" mirrors the path and anchor naming conventions used in the API reference.
3258-3258
: Standardized Section Header for Workflow Retrieval
The title "Retrieve workflows listening to deployed trigger" aligns with the anchor naming strategy, improving discoverability.
3380-3380
: Consistent Naming for Workflow Update Section
The header "Update workflows listening to deployed trigger" reflects the renamed API anchor and maintains uniform documentation language.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation