Refactor/router create browser router - #1694
Conversation
…rchitecture using React Router
…, mentor, and settings pages
|
@TanCodeX is attempting to deploy a commit to the durdana3105's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRouting is migrated from an in-component ChangesNested routing architecture
Data access and security updates
Test setup maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant RouterProvider
participant RootLayout
participant RouteLayout
participant LazyPage
App->>RouterProvider: mount router
RouterProvider->>RootLayout: render root route
RootLayout->>RouteLayout: render matched nested layout
RouteLayout->>LazyPage: render matched lazy page through Outlet
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
@durdana3105 Please review!! |
|
@durdana3105 Please review and merge! |
|
PLEASE RESOLVE MERGE CONFLICTS |
|
@durdana3105 Merge conflict resolved. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/layouts/MainLayout.tsx`:
- Around line 1-2: Update MainLayout to import Suspense and wrap the Outlet
component with a Suspense boundary, using the existing or appropriate loading
fallback. Keep Navbar outside this boundary so it remains mounted during lazy
route transitions.
In `@src/layouts/ProtectedLayout.tsx`:
- Around line 7-9: Centralize MainLayout to prevent remounts across route
branches. In src/layouts/ProtectedLayout.tsx#L7-L9,
src/layouts/ProtectedMentorLayout.tsx#L7-L9, and
src/layouts/AdminLayout.tsx#L7-L9, replace MainLayout with Outlet and import
Outlet from react-router-dom. In src/router/public.routes.tsx#L19-L28, remove
the MainLayout parent route and export the public routes as a flat array; nest
the unified route tree under one top-level MainLayout parent in the central
router configuration.
In `@src/router/public.routes.tsx`:
- Around line 13-16: Update IndexRoute to use the loading state returned by
useAuth(); render the existing loading spinner while authentication is
unresolved, then preserve the current user-based redirect to /dashboard or Index
page once loading completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d87a971-e544-44fa-96c2-242635462c1c
📒 Files selected for processing (13)
src/App.tsxsrc/layouts/AdminLayout.tsxsrc/layouts/MainLayout.tsxsrc/layouts/ProtectedLayout.tsxsrc/layouts/ProtectedMentorLayout.tsxsrc/layouts/RootLayout.tsxsrc/router/admin.routes.tsxsrc/router/auth.routes.tsxsrc/router/index.tsxsrc/router/mentor.routes.tsxsrc/router/protected.routes.tsxsrc/router/public.routes.tsxsrc/router/settings.routes.tsx
| import React from "react"; | ||
| import { Outlet } from "react-router-dom"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap <Outlet /> with <Suspense> to handle lazy-loaded routes without unmounting the navigation.
The feature route modules use React.lazy() for page components. Without a <Suspense> boundary inside MainLayout, navigating to a lazy-loaded route will suspend the layout hierarchy up to the nearest root boundary (likely in App.tsx), causing the Navbar to completely unmount and flash during page transitions.
Wrapping <Outlet /> ensures the navigation UI remains visible while the lazy page loads.
🛠️ Proposed fix
-import React from "react";
+import React, { Suspense } from "react";
import { Outlet } from "react-router-dom";
// ... existing imports ...
export default function MainLayout() {
const { user } = useAuth();
return (
<>
<Navbar />
{user && <StreakBadge />}
- <Outlet />
+ <Suspense fallback={
+ <div className="flex min-h-[calc(100vh-4rem)] items-center justify-center">
+ <div className="h-8 w-8 animate-spin rounded-full border-4 border-primary border-t-transparent" />
+ </div>
+ }>
+ <Outlet />
+ </Suspense>
</>
);
}Also applies to: 10-14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/layouts/MainLayout.tsx` around lines 1 - 2, Update MainLayout to import
Suspense and wrap the Outlet component with a Suspense boundary, using the
existing or appropriate loading fallback. Keep Navbar outside this boundary so
it remains mounted during lazy route transitions.
| <ProtectedRoute> | ||
| <MainLayout /> | ||
| </ProtectedRoute> |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Prevent layout remounts and duplicate API calls by centralizing MainLayout.
Because <MainLayout /> is independently defined as a nested element inside ProtectedLayout, ProtectedMentorLayout, AdminLayout, and the publicRoutes configuration, React Router evaluates each as a distinct layout branch. When a user navigates across these boundaries (e.g., from / to /dashboard, or /dashboard to /mentor-dashboard), React Router fully unmounts and remounts MainLayout. This destroys local Navbar state and triggers StreakBadge to unnecessarily re-fetch data from the backend on every layout transition.
To fix this, render <Outlet /> inside these layout guards instead of <MainLayout />. Then, in your central router configuration (e.g., src/router/index.tsx), wrap the unified route tree that requires the global shell with a single, top-level <MainLayout /> parent route.
src/layouts/ProtectedLayout.tsx#L7-L9: Replace<MainLayout />with<Outlet />(ensureOutletis imported fromreact-router-dom).src/layouts/ProtectedMentorLayout.tsx#L7-L9: Replace<MainLayout />with<Outlet />(ensureOutletis imported).src/layouts/AdminLayout.tsx#L7-L9: Replace<MainLayout />with<Outlet />(ensureOutletis imported).src/router/public.routes.tsx#L19-L28: Remove the<MainLayout />parent object and directly export the flat array of public routes, so they can be nested under the central<MainLayout />in your router configuration.
📍 Affects 4 files
src/layouts/ProtectedLayout.tsx#L7-L9(this comment)src/layouts/ProtectedMentorLayout.tsx#L7-L9src/layouts/AdminLayout.tsx#L7-L9src/router/public.routes.tsx#L19-L28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/layouts/ProtectedLayout.tsx` around lines 7 - 9, Centralize MainLayout to
prevent remounts across route branches. In
src/layouts/ProtectedLayout.tsx#L7-L9,
src/layouts/ProtectedMentorLayout.tsx#L7-L9, and
src/layouts/AdminLayout.tsx#L7-L9, replace MainLayout with Outlet and import
Outlet from react-router-dom. In src/router/public.routes.tsx#L19-L28, remove
the MainLayout parent route and export the public routes as a flat array; nest
the unified route tree under one top-level MainLayout parent in the central
router configuration.
| const IndexRoute = () => { | ||
| const { user } = useAuth(); | ||
| return user ? <Navigate to="/dashboard" replace /> : <Index />; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle the loading state to prevent unauthenticated UI flashing.
Because useAuth() loads asynchronously, user is initially null. When an authenticated user visits /, the application will briefly render the <Index /> page before user is populated and triggers the redirect to /dashboard.
Check the loading state and render a loading spinner until the authentication resolution is complete.
🛠️ Proposed fix
const IndexRoute = () => {
- const { user } = useAuth();
- return user ? <Navigate to="/dashboard" replace /> : <Index />;
+ const { user, loading } = useAuth();
+
+ if (loading) {
+ return (
+ <div className="flex min-h-screen items-center justify-center">
+ <div className="h-8 w-8 animate-spin rounded-full border-4 border-primary border-t-transparent" />
+ </div>
+ );
+ }
+
+ return user ? <Navigate to="/dashboard" replace /> : <Index />;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const IndexRoute = () => { | |
| const { user } = useAuth(); | |
| return user ? <Navigate to="/dashboard" replace /> : <Index />; | |
| }; | |
| const IndexRoute = () => { | |
| const { user, loading } = useAuth(); | |
| if (loading) { | |
| return ( | |
| <div className="flex min-h-screen items-center justify-center"> | |
| <div className="h-8 w-8 animate-spin rounded-full border-4 border-primary border-t-transparent" /> | |
| </div> | |
| ); | |
| } | |
| return user ? <Navigate to="/dashboard" replace /> : <Index />; | |
| }; |
🧰 Tools
🪛 GitHub Check: test
[warning] 13-13:
Fast refresh only works when a file only exports components. Move your component(s) to a separate file
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/router/public.routes.tsx` around lines 13 - 16, Update IndexRoute to use
the loading state returned by useAuth(); render the existing loading spinner
while authentication is unresolved, then preserve the current user-based
redirect to /dashboard or Index page once loading completes.
…base query patterns across hooks and tests
…b.com/TanCodeX/peer-learning into refactor/router-create-browser-router
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks/useSkillEndorsements.ts (1)
50-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestore typed Supabase access across these hooks.
The changes bypass the generated database contract, allowing schema and payload mismatches to reach runtime.
src/hooks/useSkillEndorsements.ts#L50-L54: removeas anyand restore typed endorsement reads.src/hooks/useResources.ts#L66-L69: retainsafeSupabaseCall<SavedResource[]>.src/hooks/useSkillEndorsements.ts#L118-L125: type-check endorsement deletes.src/hooks/useSkillEndorsements.ts#L128-L134: type-check endorsement inserts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useSkillEndorsements.ts` around lines 50 - 54, Restore generated Supabase typing across the affected hooks: in src/hooks/useSkillEndorsements.ts lines 50-54 remove the any cast from the skill_endorsements read; retain safeSupabaseCall<SavedResource[]> in src/hooks/useResources.ts lines 66-69; and type-check the skill_endorsements delete and insert operations in src/hooks/useSkillEndorsements.ts lines 118-125 and 128-134 using the generated database contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260617000000_consolidate_rls_policies.sql`:
- Around line 342-344: Replace the unrestricted USING (true) policies for
peer_submissions and the corresponding review policy with predicates limited to
the submission owner, participants, or authorized reviewers, then enable RLS
without exposing unrelated records. Add regression coverage confirming anonymous
and unrelated authenticated users cannot read these rows.
---
Nitpick comments:
In `@src/hooks/useSkillEndorsements.ts`:
- Around line 50-54: Restore generated Supabase typing across the affected
hooks: in src/hooks/useSkillEndorsements.ts lines 50-54 remove the any cast from
the skill_endorsements read; retain safeSupabaseCall<SavedResource[]> in
src/hooks/useResources.ts lines 66-69; and type-check the skill_endorsements
delete and insert operations in src/hooks/useSkillEndorsements.ts lines 118-125
and 128-134 using the generated database contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9847d886-9863-4fda-a0b8-a75f23d67cdd
📒 Files selected for processing (5)
backend/tests/uploadPhoto.test.jssrc/hooks/useResources.tssrc/hooks/useSkillEndorsements.tssrc/pages/Contact.test.tsxsupabase/migrations/20260617000000_consolidate_rls_policies.sql
| ALTER TABLE public.peer_submissions ENABLE ROW LEVEL SECURITY; | ||
| CREATE POLICY "Users can view submissions" | ||
| ON public.peer_submissions FOR SELECT USING (true); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose all peer data when enabling RLS.
These statements activate the adjacent USING (true) policies, allowing any role with table SELECT access—including Supabase API roles if granted—to read every submission and review, including unrelated users’ data. Scope the policies to the owner, participant, or authorized reviewer before enabling RLS, and add regression tests for anonymous and unrelated authenticated users.
Also applies to: 368-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260617000000_consolidate_rls_policies.sql` around lines
342 - 344, Replace the unrestricted USING (true) policies for peer_submissions
and the corresponding review policy with predicates limited to the submission
owner, participants, or authorized reviewers, then enable RLS without exposing
unrelated records. Add regression coverage confirming anonymous and unrelated
authenticated users cannot read these rows.
|
RESOLVE MERGE CONFLICTS |
@durdana3105 Done |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/uploadPhoto.test.js (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
onceimport
backend/tests/uploadPhoto.test.js:3no longer referencesonce, so the import can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/uploadPhoto.test.js` at line 3, Remove the unused once import from the uploadPhoto test module, leaving the remaining imports and test logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/tests/uploadPhoto.test.js`:
- Line 3: Remove the unused once import from the uploadPhoto test module,
leaving the remaining imports and test logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b49c79e4-f7aa-43ae-9353-375f6396d9a5
📒 Files selected for processing (3)
backend/tests/uploadPhoto.test.jssrc/App.tsxsrc/hooks/useSkillEndorsements.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useSkillEndorsements.ts
- src/App.tsx
…e test mocks accordingly
|
RESOLVE MERGE CONFLICTS |
@durdana3105 Done. |
2 participants
Summary
Closes #1535
This PR refactors the application's routing architecture by migrating from a single, flat
<Routes>configuration to React Router v6 Data Routers usingcreateBrowserRouter.The routing configuration is now organized into feature-specific modules with shared layouts, making the codebase significantly easier to maintain and extend while preserving the existing lazy-loading behavior.
What Changed
Routing Architecture
<BrowserRouter><Routes>implementation withcreateBrowserRouter.RouterProvideras the application's routing entry point.New Layout System
Added reusable layout components under
src/layouts/:RootLayoutHosts global UI components including:
SplashScreenSuspenseboundary for lazy-loaded routesCookieConsentBannerChatbotMainLayoutProvides the shared application shell with:
NavbarStreakBadgeProtected layout wrappers
ProtectedLayoutProtectedMentorLayoutAdminLayoutThese layouts centralize shared UI and authentication logic while reducing duplication across routes.
Feature-Based Route Modules
Created a dedicated
src/router/directory with modular route definitions:public.routes.tsxauth.routes.tsxprotected.routes.tsxsettings.routes.tsxmentor.routes.tsxadmin.routes.tsxindex.tsx(aggregates all routes into a single router instance)App Entry Point
Simplified
App.tsxto focus exclusively on application-wide providers before rendering:Benefits
Testing
RouterProvider.Suspenseboundary.Notes
useResourcesanduseSkillEndorsementswere present prior to this refactor and are unrelated to the routing migration.Checklist
createBrowserRouterSummary by CodeRabbit
New Features
Bug Fixes / Security
Tests