feat: Implement dashboard selector #179
Conversation
WalkthroughImplements role-aware dashboard routing and access control using a new useUserRole hook and a RoleGuard component. The dashboard entry page conditionally redirects or renders a selector. Host and tenant dashboards are wrapped with RoleGuard. Adds a DashboardSelector component and restructures tenant UI into tabbed sections. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant DP as /dashboard (DashboardPage)
participant UR as useUserRole
participant R as Next Router
participant DS as DashboardSelector
U->>DP: Navigate
DP->>UR: Resolve role, access, loading
UR-->>DP: { role, canAccessHostDashboard, isLoading }
alt isLoading
DP-->>U: Show Loading
else role == null
DP->>R: replace('/login' or '/dashboard')
else role == 'dual'
DP-->>U: Render DashboardSelector
U->>DS: Choose Guest/Host
DS->>R: push('/dashboard/tenant-dashboard' or '/dashboard/host-dashboard')
else role == 'host' and canAccessHostDashboard
DP->>R: replace('/dashboard/host-dashboard')
else
DP->>R: replace('/dashboard/tenant-dashboard')
end
sequenceDiagram
autonumber
participant U as User
participant HD as /dashboard/host-dashboard
participant TD as /dashboard/tenant-dashboard
participant RG as RoleGuard
participant UR as useUserRole
participant R as Next Router
U->>HD: Navigate
HD->>RG: Wrap children (requiredRole='host')
RG->>UR: Get role/access/loading
UR-->>RG: role, canAccessHostDashboard, isLoading
alt Loading
RG-->>U: Full-screen loading
else Authorized host (role in ['host','dual'] and canAccessHostDashboard)
RG-->>U: Render Host Dashboard
else
RG->>R: replace('/dashboard')
end
U->>TD: Navigate
TD->>RG: Wrap children (requiredRole='guest')
RG->>UR: Get role/access/loading
UR-->>RG: role, canAccessHostDashboard, isLoading
alt Loading
RG-->>U: Full-screen loading
else Authorized guest (role in ['guest','dual'] or host without access)
RG-->>U: Render Tenant Dashboard
else
RG->>R: replace('/dashboard')
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ 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 |
![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
🧹 Nitpick comments (1)
apps/web/src/components/dashboard/DashboardSelector.tsx (1)
5-37: Consider adding a color class to the Home icon for consistency.The User icon at line 16 has
text-blue-600, but the Home icon at line 26 lacks a color class. For visual consistency, consider adding a color class to the Home icon.Apply this diff to add a color class:
- <Home className="w-12 h-12 mb-4" /> + <Home className="w-12 h-12 text-green-600 mb-4" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
apps/backend/bun.config.ts(0 hunks)apps/backend/src/config/supabase.ts(7 hunks)apps/backend/src/controllers/booking.controller.ts(4 hunks)apps/backend/src/services/sync.service.ts(1 hunks)apps/backend/tests/integration/booking-integration.test.ts(3 hunks)apps/backend/tests/utils/booking-test.utils.ts(1 hunks)apps/web/src/app/dashboard/page.tsx(1 hunks)apps/web/src/components/dashboard/DashboardSelector.tsx(1 hunks)apps/web/src/hooks/useUserRole.ts(1 hunks)apps/web/test-results/.last-run.json(1 hunks)apps/web/tests/e2e/auth-guards.spec.ts(4 hunks)apps/web/tests/e2e/auth.spec.ts(1 hunks)apps/web/tests/e2e/dashboards.spec.ts(2 hunks)apps/web/tests/e2e/role-separation.spec.ts(1 hunks)apps/web/tsconfig.json(3 hunks)
💤 Files with no reviewable changes (1)
- apps/backend/bun.config.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/src/app/dashboard/page.tsx (2)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(9-53)apps/web/src/components/dashboard/DashboardSelector.tsx (1)
DashboardSelector(5-37)
apps/web/src/hooks/useUserRole.ts (2)
apps/web/src/hooks/auth/use-auth.tsx (1)
useAuth(251-251)apps/web/src/services/authService.ts (1)
isAuthenticated(86-105)
apps/web/src/components/dashboard/DashboardSelector.tsx (1)
apps/web/src/app/page.tsx (1)
Home(8-39)
🪛 ast-grep (0.39.5)
apps/web/tests/e2e/auth-guards.spec.ts
[warning] 28-28: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(PUBLIC_ROUTE)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (8)
apps/web/tests/e2e/role-separation.spec.ts (1)
59-59: LGTM!Formatting improvement adds a trailing newline for consistency with project standards.
apps/web/tests/e2e/dashboards.spec.ts (1)
1-1: LGTM!Formatting improvements: standardized import order and added trailing newline for consistency.
Also applies to: 130-130
apps/web/tests/e2e/auth.spec.ts (1)
64-64: LGTM!Formatting improvement adds a trailing newline for consistency.
apps/web/tsconfig.json (1)
4-4: LGTM!Formatting improvements collapse arrays to single lines for better readability without affecting TypeScript configuration behavior.
Also applies to: 26-27, 45-45
apps/web/tests/e2e/auth-guards.spec.ts (2)
1-1: LGTM!Formatting improvements standardize import order and improve readability of localStorage setup blocks by expanding them to multi-line format.
Also applies to: 36-42, 55-61, 76-82, 98-104
110-119: LGTM!The logout test flow addition enhances test coverage by verifying session clearing and redirection behavior.
apps/web/src/app/dashboard/page.tsx (1)
9-26: LGTM!The refactored dashboard page correctly implements role-based routing:
- Redirects host users with verified status to
/dashboard/host- Redirects guest users to
/dashboard/guest- Shows
DashboardSelectorfor dual-role users- Includes appropriate dependency array in useEffect
apps/web/src/hooks/useUserRole.ts (1)
9-53: LGTM with note about mock implementation.The hook correctly implements role derivation logic with appropriate error handling and state management. The mock profile data (lines 25-29) is clearly documented for demonstration purposes.
Remember to replace the mock implementation with real API calls when the
profileAPIis implemented, as noted in the comments.Based on the PR objectives and issue #167, ensure that the real API implementation will fetch:
- User role ('guest', 'host', or 'dual')
- Host status ('verified', 'unverified', or 'pending')
- Property ownership data to validate host dashboard access
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
The implementation has several critical issues that need to be addressed: First, the route redirects are incorrect - they point to /dashboard/host and /dashboard/guest which don't exist, when they should point to /dashboard/host-dashboard and /dashboard/tenant-dashboard (update both page.tsx and DashboardSelector.tsx). Second, there's an infinite loading state bug when a user has role === 'host' but canAccessHostDashboard === false (verified host without properties) - this condition is never handled in the redirect logic, leaving users stuck on "Loading...". Third, the implementation lacks authentication checks (isAuthenticated is not being used) and proper loading/error states. Fourth, the DashboardSelector component has a security vulnerability as it allows any user to navigate to any dashboard without permission verification - consider integrating the existing RoleGuard component for route protection. Finally, there are minor UI issues: the Home icon is missing color styling (should be text-green-600 to match the User icon pattern), and the component could benefit from loading spinners and transition animations to match the rest of the application's UX. Please fix the route paths and the role logic first as these are blocking issues, then address the authentication and security concerns.
Thanks for the detailed feedback @respp! I’ll fix the route paths and role logic first, then address the authentication and security issues next. |
This commit addresses several critical issues in the user dashboard functionality. It resolves an infinite loading bug for hosts without properties by implementing a dedicated UI state that prompts them to create a property instead of causing a redirect loop. The RoleGuard component has been refactored to provide stricter and more centralized route protection, ensuring users can only access dashboards appropriate for their role. This closes a security vulnerability where users could bypass role checks. Additionally, the dashboard selector UI has been enhanced with loading spinners and transition animations to improve user experience.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/src/app/dashboard/host-dashboard/page.tsx(3 hunks)apps/web/src/app/dashboard/page.tsx(1 hunks)apps/web/src/app/dashboard/tenant-dashboard/page.tsx(3 hunks)apps/web/src/components/dashboard/DashboardSelector.tsx(1 hunks)apps/web/src/hooks/auth/RoleGuard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/src/app/dashboard/host-dashboard/page.tsx (2)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
RoleGuard(13-38)apps/web/src/app/dashboard/host-dashboard/components/CalendarModal.tsx (1)
CalendarModal(50-142)
apps/web/src/app/dashboard/page.tsx (3)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(9-53)apps/web/src/hooks/auth/use-auth.tsx (1)
useAuth(251-251)apps/web/src/components/dashboard/DashboardSelector.tsx (1)
DashboardSelector(7-85)
apps/web/src/app/dashboard/tenant-dashboard/page.tsx (1)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
RoleGuard(13-38)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(9-53)
This commit addresses two issues identified in code review: - The RoleGuard now correctly allows users with the 'dual' role to access the host dashboard, preventing them from being redirected. - Keyboard navigation on the dashboard selector is fixed. The onClick handler has been moved to the Link component, allowing keyboard users to navigate to the dashboards correctly.
This commit refactors the condition for the host guard to more explicitly handle dual-role users. The previous implementation was logically equivalent, but the new condition `!(role === "host" || role === "dual")` is clearer and more directly reflects the requirement to allow both "host" and "dual" roles.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/src/hooks/auth/RoleGuard.tsx (3)
26-29: Add a comment to clarify the guest role logic.The condition
role !== 'guest' && role !== 'dual' && !(role === 'host' && !canAccessHostDashboard)is correct but uses a triple-negative structure that is difficult to parse. Consider adding a brief comment explaining that the guest dashboard allows: guest users, dual users, and host users who cannot access the host dashboard.Example:
} else if (requiredRole === 'guest') { + // Allow: guest, dual, or host without host-dashboard access if (role !== 'guest' && role !== 'dual' && !(role === 'host' && !canAccessHostDashboard)) { router.replace('/dashboard');
33-35: Consider enhancing the loading state.The current loading indicator is functional but basic. Per the PR feedback, consider adding a loading spinner and transition animations to match the app's UX patterns. This would provide a more polished experience during role verification.
Example with a spinner:
if (isLoading) { - return <div className="flex items-center justify-center min-h-screen">Loading...</div>; + return ( + <div className="flex items-center justify-center min-h-screen"> + <div className="flex flex-col items-center gap-2"> + <div className="animate-spin rounded-full h-8 w-8 border-b-2 border-gray-900" /> + <p className="text-sm text-gray-600">Loading...</p> + </div> + </div> + ); }
17-31: Consider adding explicit unauthenticated user handling.While the current logic implicitly handles unauthenticated users (when
roleisnull) by redirecting them to/dashboard, adding an explicit check would make the intent clearer and address the "missing auth handling" concern from the PR feedback.Example:
useEffect(() => { if (isLoading) { return; } + + // Redirect unauthenticated users to main dashboard + if (!role) { + router.replace('/dashboard'); + return; + } if (requiredRole === 'host') {apps/web/src/components/dashboard/DashboardSelector.tsx (1)
35-39: Enhance accessibility for loading states.The loading overlays lack screen reader announcements. Users relying on assistive technology won't know when navigation is in progress.
Add
aria-liveregions to announce loading states:{loading === 'guest' && ( - <div className="absolute inset-0 bg-white dark:bg-gray-800 bg-opacity-50 flex items-center justify-center rounded-xl"> + <div + className="absolute inset-0 bg-white dark:bg-gray-800 bg-opacity-50 flex items-center justify-center rounded-xl" + role="status" + aria-live="polite" + > <Loader2 className="w-8 h-8 animate-spin text-blue-600" /> + <span className="sr-only">Loading Guest Dashboard...</span> </div> )}Apply the same pattern to the host loading overlay (lines 56-60).
Also applies to: 56-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/components/dashboard/DashboardSelector.tsx(1 hunks)apps/web/src/hooks/auth/RoleGuard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(9-53)
🔇 Additional comments (2)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
22-25: Past review concern addressed.The condition now correctly allows dual-role users to access the host dashboard by checking
!(role === 'host' || role === 'dual'), which resolves the blocker identified in the previous review.apps/web/src/components/dashboard/DashboardSelector.tsx (1)
22-41: RoleGuard protection verified. The tenant-dashboard page at apps/web/src/app/dashboard/tenant-dashboard/page.tsx is wrapped in<RoleGuard requiredRole="guest">.
This commit addresses several points of feedback from code review to improve the dashboard functionality. In `RoleGuard.tsx`: - Adds a comment to clarify the guest role validation logic. - Enhances the loading state with a spinner for a better user experience. - Adds an explicit check to handle unauthenticated users. In `DashboardSelector.tsx`: - Improves accessibility of the loading state by adding `aria-live` regions for screen readers. - Implements a more robust navigation handler that clears the loading state on navigation success, failure, or timeout.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
33-38: Clarify the guest dashboard access logic.The condition on line 35 is logically correct but complex. It redirects users who are neither
guest,dual, norhostwithout host access—effectively sending dual users and hosts with access back to/dashboard(the selector). This enforces that those users should choose their dashboard explicitly rather than directly accessing the guest route.Add a comment explaining the intent:
} else if (requiredRole === 'guest') { + // Guest dashboard: allow guest, dual, or host without host access. + // Redirect dual users and hosts with access to the selector. if (role !== 'guest' && role !== 'dual' && !(role === 'host' && !canAccessHostDashboard)) {This improves maintainability for future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/components/dashboard/DashboardSelector.tsx(1 hunks)apps/web/src/hooks/auth/RoleGuard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(9-53)
🔇 Additional comments (1)
apps/web/src/components/dashboard/DashboardSelector.tsx (1)
13-32: LGTM: Navigation and loading state are well handled.The implementation correctly:
- Prevents default link behavior to manage loading state.
- Sets an 8-second fallback timeout to clear loading if navigation stalls.
- Cleans up the timeout on unmount and after navigation completes.
This addresses the previous review feedback about loading state persistence.
| <Link | ||
| href="/dashboard/host-dashboard" | ||
| onClick={(e) => handleNavigation(e, 'host', '/dashboard/host-dashboard')} | ||
| className="group relative block focus:outline-none" | ||
| > | ||
| <div className="p-8 bg-white dark:bg-gray-800 rounded-xl shadow-lg hover:shadow-2xl transition-all duration-300 transform hover:-translate-y-1 border-2 border-transparent hover:border-green-500"> | ||
| <Home className="w-12 h-12 text-green-600 mb-4" /> | ||
| <h2 className="text-2xl font-bold mb-2 text-gray-900 dark:text-white"> | ||
| Host Dashboard | ||
| </h2> | ||
| <p className="text-gray-600 dark:text-gray-300"> | ||
| Manage properties, bookings, and earnings | ||
| </p> | ||
| {loading === 'host' && ( | ||
| <div | ||
| className="absolute inset-0 bg-white dark:bg-gray-800 bg-opacity-50 flex items-center justify-center rounded-xl" | ||
| // biome-ignore lint/a11y/useSemanticElements: Using a div with role='status' for the loading overlay. | ||
| role="status" | ||
| aria-live="polite" | ||
| > | ||
| <Loader2 className="w-8 h-8 animate-spin text-green-600" /> | ||
| <span className="sr-only">Loading Host Dashboard...</span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </Link> |
There was a problem hiding this comment.
Poor UX: selector allows navigation to inaccessible dashboards.
The host dashboard card is always enabled, so users lacking host access can click it, see a loading spinner, navigate to /dashboard/host-dashboard, then get immediately redirected back by RoleGuard. This creates a confusing loop.
Integrate useUserRole to check canAccessHostDashboard and conditionally disable or hide the host card for users without access:
+'use client';
+import { useUserRole } from '@/hooks/useUserRole';
export function DashboardSelector() {
+ const { canAccessHostDashboard } = useUserRole();
const [loading, setLoading] = useState<string | null>(null);
...
return (
...
- <Link
+ <Link
href="/dashboard/host-dashboard"
onClick={(e) => handleNavigation(e, 'host', '/dashboard/host-dashboard')}
- className="group relative block focus:outline-none"
+ className={`group relative block focus:outline-none ${!canAccessHostDashboard ? 'opacity-50 pointer-events-none' : ''}`}
+ aria-disabled={!canAccessHostDashboard}
>This prevents navigation attempts and clarifies to users which dashboards they can access.
Based on reviewer feedback and PR objectives.
📝 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.
| <Link | |
| href="/dashboard/host-dashboard" | |
| onClick={(e) => handleNavigation(e, 'host', '/dashboard/host-dashboard')} | |
| className="group relative block focus:outline-none" | |
| > | |
| <div className="p-8 bg-white dark:bg-gray-800 rounded-xl shadow-lg hover:shadow-2xl transition-all duration-300 transform hover:-translate-y-1 border-2 border-transparent hover:border-green-500"> | |
| <Home className="w-12 h-12 text-green-600 mb-4" /> | |
| <h2 className="text-2xl font-bold mb-2 text-gray-900 dark:text-white"> | |
| Host Dashboard | |
| </h2> | |
| <p className="text-gray-600 dark:text-gray-300"> | |
| Manage properties, bookings, and earnings | |
| </p> | |
| {loading === 'host' && ( | |
| <div | |
| className="absolute inset-0 bg-white dark:bg-gray-800 bg-opacity-50 flex items-center justify-center rounded-xl" | |
| // biome-ignore lint/a11y/useSemanticElements: Using a div with role='status' for the loading overlay. | |
| role="status" | |
| aria-live="polite" | |
| > | |
| <Loader2 className="w-8 h-8 animate-spin text-green-600" /> | |
| <span className="sr-only">Loading Host Dashboard...</span> | |
| </div> | |
| )} | |
| </div> | |
| </Link> | |
| 'use client'; | |
| import { useState } from 'react'; | |
| import Link from 'next/link'; | |
| import { useUserRole } from '@/hooks/useUserRole'; | |
| import { Loader2 } from 'lucide-react'; | |
| import { Home } from 'lucide-react'; | |
| export function DashboardSelector() { | |
| const { canAccessHostDashboard } = useUserRole(); | |
| const [loading, setLoading] = useState<string | null>(null); | |
| const handleNavigation = (e: React.MouseEvent, dashboard: string, path: string) => { | |
| e.preventDefault(); | |
| setLoading(dashboard); | |
| // existing navigation logic, e.g. router.push(path).finally(() => setLoading(null)) | |
| }; | |
| return ( | |
| <> | |
| {/* … other cards … */} | |
| <Link | |
| href="/dashboard/host-dashboard" | |
| onClick={(e) => handleNavigation(e, 'host', '/dashboard/host-dashboard')} | |
| className={`group relative block focus:outline-none ${ | |
| !canAccessHostDashboard ? 'opacity-50 pointer-events-none' : '' | |
| }`} | |
| aria-disabled={!canAccessHostDashboard} | |
| > | |
| <div className="p-8 bg-white dark:bg-gray-800 rounded-xl shadow-lg hover:shadow-2xl transition-all duration-300 transform hover:-translate-y-1 border-2 border-transparent hover:border-green-500"> | |
| <Home className="w-12 h-12 text-green-600 mb-4" /> | |
| <h2 className="text-2xl font-bold mb-2 text-gray-900 dark:text-white"> | |
| Host Dashboard | |
| </h2> | |
| <p className="text-gray-600 dark:text-gray-300"> | |
| Manage properties, bookings, and earnings | |
| </p> | |
| {loading === 'host' && ( | |
| <div | |
| className="absolute inset-0 bg-white dark:bg-gray-800 bg-opacity-50 flex items-center justify-center rounded-xl" | |
| role="status" | |
| aria-live="polite" | |
| > | |
| <Loader2 className="w-8 h-8 animate-spin text-green-600" /> | |
| <span className="sr-only">Loading Host Dashboard...</span> | |
| </div> | |
| )} | |
| </div> | |
| </Link> | |
| {/* … other cards … */} | |
| </> | |
| ); | |
| } |
| } | ||
|
|
||
| if (requiredRole === 'host') { | ||
| // TODO: Add check for hasProperties when available in useUserRole |
There was a problem hiding this comment.
Critical: Implement the missing properties check.
The TODO flags a critical gap: per the PR objectives, "Host dashboard accessible only if hostStatus === 'verified' and the user has properties." Currently, canAccessHostDashboard from useUserRole only verifies hostStatus === 'verified', omitting the properties check. This allows verified hosts without properties to access the host dashboard, violating the requirement.
Update useUserRole to include a properties check in canAccessHostDashboard, or add an explicit property-count verification here before allowing host access.
Based on PR objectives and reviewer feedback.
🤖 Prompt for AI Agents
In apps/web/src/hooks/auth/RoleGuard.tsx around line 29, the TODO notes a
missing properties check: ensure that host dashboard access requires both
hostStatus === 'verified' and that the user actually has properties; either
update useUserRole to make canAccessHostDashboard return hostStatus ===
'verified' && hasProperties (or properties?.length > 0) or add an explicit check
here before granting host access (validate properties array/flag is present and
non-empty), and use that combined boolean in the access decision.
The host dashboard should only be accessible to verified hosts who have at least one property. This commit updates the `useUserRole` hook to include a `hasProperties` check in the `canAccessHostDashboard` logic. The `RoleGuard` component now correctly prevents access to the host dashboard if the user has no properties, resolving the `TODO`.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
32-37: Simplify the guest guard logic for readability.The current condition correctly allows guests, dual users, and hosts without host access, but the triple-negative logic is hard to parse. Consider simplifying to:
} else if (requiredRole === 'guest') { - // Allow: guest, dual, or host without host-dashboard access - if (role !== 'guest' && role !== 'dual' && !(role === 'host' && !canAccessHostDashboard)) { + // Redirect only hosts with host dashboard access + if (role === 'host' && canAccessHostDashboard) { router.replace('/dashboard'); }Both implementations produce the same behavior, but the simplified version directly expresses the intent: "redirect hosts who can access the host dashboard."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/hooks/auth/RoleGuard.tsx(1 hunks)apps/web/src/hooks/useUserRole.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/hooks/auth/RoleGuard.tsx (1)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(15-66)
apps/web/src/hooks/useUserRole.ts (2)
apps/web/src/hooks/auth/use-auth.tsx (1)
useAuth(251-251)apps/web/src/services/authService.ts (1)
isAuthenticated(86-105)
The `useUserRole` hook is updated to fetch user profiles from a mock API instead of using a hardcoded mock profile. This change introduces a `profileAPI` mock that simulates fetching user data, with different profiles for different user IDs in a development environment. This makes the hook more realistic and easier to test with different user roles.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/app/dashboard/page.tsx (2)
17-32: Verify the redirect logic handles all edge cases.The redirect logic correctly addresses the previously reported issues:
- Routes to correct paths (
/dashboard/host-dashboardand/dashboard/tenant-dashboard)- Handles host without dashboard access by not redirecting (allowing the CTA UI to show)
However, consider explicitly handling the
role === nullcase after loading completes. Currently, ifisLoading === falseandisAuthenticated === truebutrole === null, no redirect occurs, and the user sees the default dashboard selection UI. While unlikely ifuseUserRoleworks correctly, adding an explicit check would make the intent clearer:if (role === null) { // Log error or show error state return; }
59-61: PasscanAccessHostDashboardtoDashboardSelectorfor better UX.Currently,
DashboardSelectorshows both dashboard options to all dual-role users, even if they cannot access the host dashboard (canAccessHostDashboard === false). While the dashboard pages themselves should be protected (as mentioned in the PR objectives viaRoleGuard), this creates a poor user experience where users click the host option only to be blocked.Consider passing
canAccessHostDashboardas a prop toDashboardSelectorso it can conditionally hide, disable, or show a message for the host dashboard option when access is not available:- if (role === 'dual') { - return <DashboardSelector />; - } + if (role === 'dual') { + return <DashboardSelector canAccessHostDashboard={canAccessHostDashboard} />; + }Then update
DashboardSelectorto accept this prop and adjust its UI accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/app/dashboard/page.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/app/dashboard/page.tsx (3)
apps/web/src/hooks/useUserRole.ts (1)
useUserRole(58-102)apps/web/src/hooks/auth/use-auth.tsx (1)
useAuth(251-251)apps/web/src/components/dashboard/DashboardSelector.tsx (1)
DashboardSelector(8-107)
🔇 Additional comments (2)
apps/web/src/app/dashboard/page.tsx (2)
64-149: RoleGuard protection verified on both dashboards. Both host and tenant dashboard pages wrap their content in<RoleGuard requiredRole="host">and<RoleGuard requiredRole="guest">respectively, preventing unauthorized access.
42-57: Confirm/add-propertyroute exists
I couldn’t find anadd-propertypage underapps/web/src/app; please verify (or add) the/add-propertyroute implementation.
2 participants
🔥 Title
feat: Implement Dashboard Selector
📝 Summary
Added a dashboard selector at
/dashboardto route users based on their role or show a selector for dual-role users.🔄 Changes
page.tsxfor dashboard redirection logicDashboardSelector.tsxfor dual-role selection UIuseUserRole.tsto fetch actual user role and propertiesgetPropertiesByOwnertoapi.ts🧪 Testing
/dashboardredirects correctly based on role⚙️ Related Issue
Closes #167
Summary by CodeRabbit