Conversation
|
@Methexx is attempting to deploy a commit to the dizzpy's projects team on Vercel, but is not a member of this team. To resolve this issue, you can:
To read more about collaboration on Vercel, click here. |
There was a problem hiding this comment.
Pull request overview
This PR implements new overview/dashboard pages for admin, trainer, and user roles, along with improvements to attendance tracking functionality and package dependency management.
Changes:
- Added comprehensive dashboard pages for admin, trainer, and user roles with real-time stats and auto-refresh functionality
- Implemented attendance status tracking and history display for users
- Updated API client to suppress 404 error logging for expected cases
- Configured Next.js to allow i.pravatar.cc for placeholder avatars
- Removed peer dependency flags and cleaned up unused QR code dependencies
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/package-lock.json | Removed "peer": true flags from multiple dependencies |
| backend/package-lock.json | Removed QR code related packages (qrcode, dijkstrajs, pngjs) and peer flags |
| frontend/next.config.ts | Added i.pravatar.cc hostname to allowed image domains for placeholder avatars |
| frontend/lib/api/client.ts | Suppressed console logging for 404 errors as they're expected in some cases |
| frontend/lib/api/attendance.ts | Added getCurrentStatus function and updated getAttendanceStats to handle inconsistent backend response format |
| frontend/app/trainer/overview/page.tsx | Complete rewrite adding dashboard with stats, pending requests, members list, and live activity |
| frontend/app/admin/overview/page.tsx | Complete rewrite adding dashboard with stats, recent payments display, and auto-refresh |
| frontend/app/(user)/overview/page.tsx | Complete rewrite adding user dashboard with membership, attendance, trainer info, and workout plans |
| frontend/app/(user)/attendance/page.tsx | Replaced mock data with real API calls to fetch attendance history |
Files not reviewed (2)
- backend/package-lock.json: Language not supported
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| handleRefresh(); | ||
| }, 30000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect hook has an empty dependency array but calls handleRefresh, which depends on fetchDashboardData and setters. This will cause the interval to always call the same initial reference of handleRefresh. Since handleRefresh and fetchDashboardData don't have stable references, this could lead to stale closures. Consider using useCallback to memoize handleRefresh and fetchDashboardData, or include them in the dependency array (though the latter may cause the interval to be recreated on every render).
| // The backend returns overview at root level, not inside data | ||
| // So we check both response.data?.overview and (response as any).overview | ||
| const rawResponse = response as unknown as { success: boolean; overview?: AttendanceOverviewResponse['overview'] }; | ||
| const overview = response.data?.overview || rawResponse.overview; |
There was a problem hiding this comment.
Using type assertion with 'as any' or 'as unknown' is a code smell that indicates the API response type may not match the expected type. The workaround to check both response.data?.overview and rawResponse.overview suggests an inconsistency in the backend API response format. Consider either: (1) updating the backend to return a consistent format, or (2) properly typing the response to reflect the actual backend structure instead of using type assertions.
| requestId: req.id, | ||
| memberName: req.memberId?.fullName || 'Unknown Member', | ||
| memberAvatar: `https://i.pravatar.cc/150?u=${req.memberId?.id}`, | ||
| membershipPlan: 'Membership Plan', |
There was a problem hiding this comment.
The membershipPlan field is hardcoded to 'Membership Plan' instead of using actual membership plan data from the request. This will display the same generic label for all requests regardless of their actual membership plan. Consider either: (1) adding the actual membership plan data to the TrainerRequest type and using it here, or (2) fetching the membership plan information from the backend.
| membershipPlan: 'Membership Plan', | |
| membershipPlan: (req as any).membershipPlan || 'Membership Plan', |
| const payments = (paymentsRes.data as unknown as Array<{ | ||
| _id: string; | ||
| memberName?: string; | ||
| memberEmail?: string; | ||
| amount: number; | ||
| currency?: string; | ||
| createdAt: string; | ||
| status?: string; | ||
| }>).slice(0, 5).map(payment => ({ |
There was a problem hiding this comment.
Using type assertion with 'as unknown as Array' is a code smell that indicates the API response type doesn't match the expected type. The PaymentRecord[] type parameter for apiClient doesn't match the actual structure being cast to. Consider properly typing the API response by creating an interface for the backend payment response structure and using that as the type parameter instead of casting.
| const totalExercises = workoutPlans[0]?.workoutDays?.reduce((acc: number, day: { exercises?: unknown[] }) => | ||
| acc + (day.exercises?.length || 0), 0 | ||
| ) || 0; |
There was a problem hiding this comment.
The totalExercises calculation always uses the first workout plan (workoutPlans[0]) regardless of which workout session is being displayed in the history. This means all history entries will show the same exercise count from the first plan, which is incorrect. Each attendance record should be associated with its specific workout plan, or this field should be removed if the association isn't available.
| // Update stats | ||
| const newStats: DashboardStats = { | ||
| totalMembers: usersRes?.data?.pagination?.total || 0, | ||
| totalTrainers: trainersRes?.data?.pagination?.total || 0, | ||
| activeMemberships: usersRes?.data?.pagination?.total || 0, // Using members as proxy for active memberships |
There was a problem hiding this comment.
The activeMemberships stat is set to the same value as totalMembers, which assumes all members have active memberships. This is likely inaccurate as some members may have expired or inactive memberships. Consider fetching the actual count of active memberships from the backend or using a dedicated endpoint that returns only active memberships.
| // Update stats | |
| const newStats: DashboardStats = { | |
| totalMembers: usersRes?.data?.pagination?.total || 0, | |
| totalTrainers: trainersRes?.data?.pagination?.total || 0, | |
| activeMemberships: usersRes?.data?.pagination?.total || 0, // Using members as proxy for active memberships | |
| // Derive active memberships from payments data if available | |
| const activeMembershipsCount = | |
| paymentsRes?.success && Array.isArray(paymentsRes.data) | |
| ? (paymentsRes.data as Array<{ status?: string }>).filter( | |
| (payment) => payment.status === 'active' | |
| ).length | |
| : 0; | |
| // Update stats | |
| const newStats: DashboardStats = { | |
| totalMembers: usersRes?.data?.pagination?.total || 0, | |
| totalTrainers: trainersRes?.data?.pagination?.total || 0, | |
| activeMemberships: activeMembershipsCount, |
| "node": ">=6" | ||
| } | ||
| }, | ||
| "node_modules/qs": { |
There was a problem hiding this comment.
The backend code imports the 'qrcode' package, but this package has been removed from package-lock.json (along with its dependencies dijkstrajs and pngjs). This will cause a runtime error when the qrCodeService is used. Either: (1) the qrcode package should be added back to package.json and package-lock.json, or (2) if this functionality is no longer needed, the qrCodeService.js file should be removed or the import should be removed.
| <button className="text-sm text-gray-600 hover:text-gray-900"> | ||
| View All Members | ||
| </button> |
There was a problem hiding this comment.
The "View All Members" button doesn't have any onClick handler or navigation functionality. This is a non-functional button that appears clickable but does nothing. Either implement the navigation/functionality for this button or remove it until the feature is ready.
| <button className="text-sm text-gray-600 hover:text-gray-900"> | |
| View All Members | |
| </button> |
| <button className="flex items-center gap-2 px-4 py-2 bg-red-600 text-white text-sm font-medium rounded-lg hover:bg-red-700"> | ||
| Start Now | ||
| <ArrowRight size={16} /> | ||
| </button> |
There was a problem hiding this comment.
The "Start Now" button doesn't have any onClick handler or navigation functionality. This is a non-functional button that appears clickable but does nothing. Either implement the navigation/functionality for this button to actually start the workout program or remove it until the feature is ready.
| <button className="flex items-center gap-2 px-4 py-2 bg-red-600 text-white text-sm font-medium rounded-lg hover:bg-red-700"> | |
| Start Now | |
| <ArrowRight size={16} /> | |
| </button> |
| export default function OverviewPage() { | ||
| const { user, loading } = useProtectedRoute({ allowedRoles: ['user'] }); | ||
| const [loading, setLoading] = useState(true); | ||
| const [profile, setProfile] = useState<UserProfile | null>(null); |
There was a problem hiding this comment.
Unused variable profile.
| const [profile, setProfile] = useState<UserProfile | null>(null); | |
| const [_profile, setProfile] = useState<UserProfile | null>(null); |
No description provided.