fix: remove rewrites and make everything full urls#4133
Conversation
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR migrates documentation links from internal routes to external URLs. It removes Next.js rewrites that previously proxied Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
apps/dashboard/app/auth/layout.tsx (1)
97-104: Addrel="noopener noreferrer"for security.The external documentation link uses
target="_blank"but is missing therel="noopener noreferrer"attribute, which prevents potential tabnabbing attacks. Other external links in this file (lines 112-118 and 121-127) correctly include this attribute.Apply this diff to add the security attribute:
<Link className="flex items-center h-8 gap-2 px-4 text-sm text-white duration-500 border rounded-lg bg-white/5 hover:bg-white hover:text-black border-white/10" href="https://www.unkey.com/docs" target="_blank" + rel="noopener noreferrer" > <FileText className="w-4 h-4" strokeWidth={1} /> Documentation </Link>
🧹 Nitpick comments (1)
apps/dashboard/components/dashboard/command-menu.tsx (1)
71-89: Consider usingwindow.open()for external URLs.The
GenericLinkCommandusesrouter.push()for navigation, which is primarily intended for client-side navigation within the Next.js app. For external URLs like the documentation link,window.open()would be more semantically appropriate.Apply this diff to handle external URLs explicitly:
const GenericLinkCommand: React.FC<{ href: string; label: string; icon: LucideIcon; close: () => void; }> = (props) => { const router = useRouter(); return ( <CommandItem onSelect={() => { - router.push(props.href); + if (props.href.startsWith('http://') || props.href.startsWith('https://')) { + window.open(props.href, '_blank', 'noopener,noreferrer'); + } else { + router.push(props.href); + } props.close(); }} > <props.icon className="w-4 h-4 mr-2" /> <span>{props.label}</span> </CommandItem> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/_components/list/index.tsx(1 hunks)apps/dashboard/app/auth/layout.tsx(1 hunks)apps/dashboard/components/dashboard/command-menu.tsx(1 hunks)apps/dashboard/next.config.js(0 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/next.config.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/_components/list/index.tsx (1)
51-60: LGTM! Proper external link implementation.The documentation link is correctly implemented with an anchor tag,
target="_blank", andrel="noopener noreferrer"for security. This is the correct pattern for external links.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/24/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
4 participants
What does this PR do?
A user reported a bug when they used slug of
docswhich kept redirecting them. We have rewrites in the dashboard that are no longer in use as far as I can tell.So I removed them and quickly found any /docs and made them a full URL
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
There isn't much too test, the rewrites were from everything living in the original app, as far as I can tell we handle everything via CNAME / A records
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated