refactor: remove all instances of react-router-dom#1410
refactor: remove all instances of react-router-dom#1410paanSinghCoder wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes direct react-router-dom coupling from admin SDK views and pages by replacing Link/NavLink/useNavigate usage with optional callback props (onNavigateToOrg, onSelectPlan, onSelectPreference, onNavigate, onNavigateToUser). Pages supply navigation handlers; react-router-dom was removed from the SDK peerDependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 |
Pull Request Test Coverage Report for Build 22306651313Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/sdk/package.json (1)
109-109:⚠️ Potential issue | 🟠 Major
@tanstack/react-routerremains deeply coupled throughout the SDK — contradicts the "router-agnostic" goal.The SDK source imports from
@tanstack/react-routerin 50+ files across the organization, billing, teams, api-keys, sessions, profiles, projects, plans, domains, and members components. Hooks likeuseNavigate,useParams,useRouterState,useRouteContext, and components likeOutletandLinkare used directly in the SDK code.Removing
react-router-domfrompeerDependencieswhile keeping@tanstack/react-routerindependenciesdoes not achieve router agnosticity—it simply couples the SDK to a different router. To be truly router-agnostic, the SDK should accept router-agnostic abstractions from consumers or provide a plugin system instead of hard-coupling to a specific router implementation.
🧹 Nitpick comments (8)
web/sdk/admin/views/webhooks/webhooks/update/index.tsx (1)
49-51: UnnecessaryuseCallbackindirectionThe memoized
onCloseonly wrapsonCloseProp?.()with no additional logic. The extrauseCallbackadds complexity without benefit.♻️ Simplified close handler (assumes `onClose` is made required per above)
- const onClose = useCallback(() => { - onCloseProp?.(); - }, [onCloseProp]);Remove the wrapper entirely and use
onCloseprop directly at the two call sites (line 84 and line 119).web/sdk/admin/views/webhooks/webhooks/header.tsx (1)
17-18:handleCreateis a redundant wrapper — can passonOpenCreatedirectly.♻️ Proposed simplification
-export const WebhooksHeader = ({ header = pageHeader, onOpenCreate }: WebhooksHeaderProps) => { - const handleCreate = () => onOpenCreate?.(); - - return ( +export const WebhooksHeader = ({ header = pageHeader, onOpenCreate }: WebhooksHeaderProps) => { + return ( <PageHeader ...> ... - <Button ... onClick={handleCreate}> + <Button ... onClick={onOpenCreate}>web/sdk/admin/views/admins/index.tsx (1)
38-38: Memoizecolumnsto avoid recreating column definitions on every render.
getColumns({ onNavigateToOrg })is called at the top level of the render function. WhenAdminsPagepasses an inline arrow foronNavigateToOrg(which it currently does), new column objects are produced on every render, which can causeDataTableto recalculate internals unnecessarily.♻️ Proposed fix
+import { useMemo } from "react"; ... - const columns = getColumns({ onNavigateToOrg }); + const columns = useMemo(() => getColumns({ onNavigateToOrg }), [onNavigateToOrg]);web/apps/admin/src/pages/admins/AdminsPage.tsx (1)
4-11: Inline arrow creates a new callback reference on every render; useuseCallback.
onNavigateToOrgis a new function on every render, causingAdminsView(and ultimatelygetColumns) to re-execute each timeAdminsPagere-renders. The same pattern applies toonSelectPlaninPlansPage.tsx— both should be stabilized. The explicit(orgId: string)type annotation is also redundant since TypeScript can infer it fromAdminsViewProps.♻️ Proposed fix
+import { useCallback } from "react"; import { AdminsView } from "@raystack/frontier/admin"; import { useNavigate } from "react-router-dom"; export function AdminsPage() { const navigate = useNavigate(); + const handleNavigateToOrg = useCallback( + (orgId: string) => navigate(`/organizations/${orgId}`), + [navigate] + ); return ( - <AdminsView - onNavigateToOrg={(orgId: string) => navigate(`/organizations/${orgId}`)} - /> + <AdminsView onNavigateToOrg={handleNavigateToOrg} /> ); }web/apps/admin/src/pages/plans/PlansPage.tsx (1)
8-14: Same inline-arrow render concern asAdminsPage.tsx.
onSelectPlanis recreated on each render. Consider wrapping it withuseCallbackfor the same reasons noted inAdminsPage.tsx.web/apps/admin/src/pages/preferences/PreferencesPage.tsx (1)
12-12: Redundant explicit type annotation on callback parameter.TypeScript infers
prefName's type fromPreferencesViewProps.onSelectPreference?: (name: string) => void, so the inline annotation is unnecessary.♻️ Suggested simplification
- onSelectPreference={(prefName: string) => navigate(`/preferences/${prefName}`)} + onSelectPreference={(prefName) => navigate(`/preferences/${prefName}`)}web/sdk/admin/views/users/details/layout/navbar.tsx (1)
34-53: Breadcrumbhrefprops are still hard-coded, leaving navigation partially router-coupled.The
Breadcrumb.Itemelements (lines 35 and 42) use rawhrefattributes that render as native<a>tags. In a SPA these cause full-page navigations, bypassing the newonNavigatecallback pattern added by this PR. To complete the router-agnostic refactor, consider accepting breadcrumb navigation callbacks (e.g.,onNavigateToUsers/onNavigateToUser) analogous to theonNavigatepattern already introduced here.web/sdk/admin/views/plans/columns.tsx (1)
16-26:cursor: "pointer"is unconditional — misleading whenonSelectPlanis absent.When
onSelectPlanis not provided, the ID cell still renders with a pointer cursor but the click is a no-op, which implies interactivity that doesn't exist.🎨 Proposed fix
- <Text - style={{ cursor: "pointer" }} - onClick={() => onSelectPlan?.(id)} - > + <Text + role={onSelectPlan ? "button" : undefined} + tabIndex={onSelectPlan ? 0 : undefined} + style={{ cursor: onSelectPlan ? "pointer" : "default" }} + onClick={() => onSelectPlan?.(id)} + onKeyDown={(e) => + onSelectPlan && (e.key === "Enter" || e.key === " ") && onSelectPlan(id) + } + >Note: the keyboard-accessibility gap (missing
role="button"/tabIndex) mentioned inweb/sdk/admin/views/preferences/columns.tsxapplies identically here.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
web/apps/admin/src/pages/admins/AdminsPage.tsxweb/apps/admin/src/pages/plans/PlansPage.tsxweb/apps/admin/src/pages/preferences/PreferencesPage.tsxweb/apps/admin/src/pages/users/UsersPage.tsxweb/sdk/admin/views/admins/columns.tsxweb/sdk/admin/views/admins/index.tsxweb/sdk/admin/views/plans/columns.tsxweb/sdk/admin/views/plans/index.tsxweb/sdk/admin/views/preferences/PreferencesView.tsxweb/sdk/admin/views/preferences/columns.tsxweb/sdk/admin/views/preferences/index.tsxweb/sdk/admin/views/users/UsersView.tsxweb/sdk/admin/views/users/details/layout/layout.tsxweb/sdk/admin/views/users/details/layout/navbar.tsxweb/sdk/admin/views/users/details/user-details.tsxweb/sdk/admin/views/users/list/columns.tsxweb/sdk/admin/views/users/list/list.tsxweb/sdk/admin/views/webhooks/webhooks/create/index.tsxweb/sdk/admin/views/webhooks/webhooks/header.tsxweb/sdk/admin/views/webhooks/webhooks/update/index.tsxweb/sdk/package.json
2 participants
Summary
Remove all instances of react-router-dom to make sdk truly router agnostic.
Changes
Technical Details
Test Plan