-
Notifications
You must be signed in to change notification settings - Fork 460
adds redirects for old docs links #800
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
import { redirect } from 'next/navigation'; | ||
|
||
type Props = { | ||
params: Promise<{ |
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.
The params type is incorrectly defined as a Promise. In Next.js, page parameters for dynamic routes are passed as direct values, not promises. The type should be { params: { slug: string[] } }. The unnecessary Promise type and await operation could cause type checking issues and unnecessary asynchronous operations. This could lead to runtime errors if the code is used in a context where Promise handling is not properly implemented.
params: Promise<{ | |
params: { |
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
😱 Found 1 issue. Time to roll up your sleeves! 😱 🗒️ View all ignored comments in this repo
Need help? Join our Discord for support! |
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.
Greptile Summary
This PR adds a redirect system to handle broken old documentation links by implementing a catch-all route strategy. The main changes include:
-
Catch-all Route Implementation: A new
[...slug]/page.tsx
file intercepts unmatched routes and redirects them to appropriate documentation sections based on URL pattern matching. -
Platform Detection Logic: The system attempts to detect the intended platform (next, react, js, python) from URL patterns using string matching on the slug array.
-
Section Mapping: Common documentation sections like SDK, components, getting-started, concepts, etc. are mapped to their new locations in the current URL structure (
/docs/{platform}/{section}
). -
Fallback Strategy: All redirects ultimately fall back to
/docs/{platform}/overview
to ensure users never encounter 404 errors. -
Config Update: The
next.config.mjs
file is updated with a comment indicating that redirects are now handled by the catch-all route instead of explicit configuration.
This approach replaces the need to maintain exhaustive URL mappings in the Next.js config with a more flexible pattern-based system that can handle various URL structures dynamically. The implementation ensures backward compatibility for old bookmarks and external links while directing users to the most relevant documentation sections.
Confidence score: 3/5
• This PR has moderate complexity with potential edge cases in the redirect logic that could cause unexpected behavior
• The platform detection logic uses simple string matching which may not handle all URL variations correctly, and some redirect conditions could conflict
• The [...slug]/page.tsx
file needs more attention due to complex conditional logic and potential for infinite redirect loops
2 files reviewed, 1 comment
if (path.startsWith('react/') || path.includes('/react/')) { | ||
platform = 'react'; | ||
} else if (path.startsWith('js/') || path.includes('/js/')) { | ||
platform = 'js'; | ||
} else if (path.startsWith('python/') || path.includes('/python/')) { | ||
platform = 'python'; |
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.
logic: Platform detection uses includes()
which could cause false positives. Path /docs/react-native/guide
would incorrectly detect as 'react' platform
import { redirect } from 'next/navigation'; | ||
|
||
type Props = { | ||
params: Promise<{ |
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.
Consider revising the Props type. In Next.js app directory, the route params are passed synchronously (i.e. { slug: string[] }) so wrapping it in a Promise and awaiting it may be unnecessary.
|
||
// Handle API routes → /api/overview | ||
if (path.startsWith('next/rest-api/') || path.startsWith('rest-api/') || path.includes('/rest-api/')) { | ||
redirect('/api/overview'); |
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.
For constructing redirect URLs, consider using a tagged template literal (e.g. urlString) to safely interpolate dynamic segments per our URL guidelines.
This comment was generated because it violated a code review rule: mrule_pmzJAgHDlFZgwIwD.
this PR is replaced with #804 |
quick fix for broken URLs old deep links. RIght now, redirect to overview for api deep links, and for old docs deep links, redirects to /docs/next/overview.
Important
Adds catch-all redirect logic in
page.tsx
for old documentation links, updatingnext.config.mjs
to reflect this change.page.tsx
to handle old documentation links, redirecting to relevant sections based on URL patterns./api/overview
./docs/{platform}/
respective sections./docs/{platform}/overview
.next
, with detection forreact
,js
, andpython
.next.config.mjs
to note that other redirects are handled bypage.tsx
.This description was created by
for 9e26b23. You can customize this summary. It will automatically update as commits are pushed.