-
Notifications
You must be signed in to change notification settings - Fork 1
feat(auth): Authentication and Login UI #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(auth): Authentication and Login UI #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements a comprehensive authentication and login system for the frontend, including login/logout pages, protected routes, and JWT token management. The implementation adds role-based access control to redirect admins and volunteers to appropriate pages after authentication.
Changes:
- Added LoginPage and LogoutPage components with authentication flow
- Implemented AuthContext with useAuth hook for managing authentication state
- Added ProtectedRoute component for role-based route protection
- Created auth API integration with JWT token handling in localStorage
- Updated API client to use Bearer token authentication
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/LoginPage.tsx | Login form with email/password inputs and role-based redirect logic |
| frontend/src/pages/LogoutPage.tsx | Logout page that clears tokens and redirects to login |
| frontend/src/pages/LoginPage.test.tsx | Comprehensive tests for login flows (admin/volunteer redirects and error handling) |
| frontend/src/contexts/AuthContext.tsx | AuthProvider implementation using React Query mutations |
| frontend/src/contexts/authContext.shared.ts | Shared auth context type definitions and useAuth hook |
| frontend/src/contexts/index.ts | Context exports for AuthProvider and useAuth |
| frontend/src/contexts/AuthContext.test.tsx | Tests for session persistence and logout functionality |
| frontend/src/components/ProtectedRoute.tsx | Route guard component for authenticated and role-based access |
| frontend/src/api/auth.api.ts | API functions for login, logout, and getCurrentUser |
| frontend/src/api/apiClient.ts | Updated to use Bearer token authentication with accessToken |
| frontend/src/actions/useAuth.ts | React Query hooks for login, logout, and current user queries |
| frontend/src/App.tsx | Integrated AuthProvider and added login/logout/protected routes |
| frontend/.env.example | Updated API base URL from /api/v1 to /api |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
21d61dc to
5948c68
Compare
5948c68 to
5927634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5927634 to
f43c14c
Compare
f43c14c to
654e088
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if (hasSubmitted && isAuthenticated && user) { | ||
| navigate(user.role === 'ADMIN' ? '/admin' : '/'); | ||
| } | ||
| }, [hasSubmitted, isAuthenticated, user, navigate]); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for the scenario where an already authenticated user accesses the login page. The useEffect at lines 13-17 suggests the page should redirect authenticated users, but there's no test verifying this behavior.
| } | ||
|
|
||
| export const authApi = { | ||
| login: async (email: string, password: string) => { |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation for the login function. Email and password parameters should be validated before making the API call to ensure they are non-empty strings, improving user experience and potentially reducing unnecessary API calls.
| login: async (email: string, password: string) => { | |
| login: async (email: string, password: string) => { | |
| if (typeof email !== 'string' || email.trim().length === 0) { | |
| throw new Error('Email is required and must be a non-empty string.'); | |
| } | |
| if (typeof password !== 'string' || password.trim().length === 0) { | |
| throw new Error('Password is required and must be a non-empty string.'); | |
| } |
| const hasAccessToken = !!localStorage.getItem('accessToken'); | ||
| return useQuery({ | ||
| queryKey: currentUserQueryKey, | ||
| queryFn: authApi.getCurrentUser, | ||
| enabled: hasAccessToken, |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query is only enabled when the accessToken exists, but the check happens once at hook creation time. If the token is added or removed during the component's lifecycle, the query won't automatically enable or disable. Consider moving the token check inside the enabled function or using a state variable that updates when localStorage changes.
| loginError: loginMutation.error ? String(loginMutation.error) : null, | ||
| logoutError: logoutMutation.error ? String(logoutMutation.error) : null, |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting errors to strings using String(error) loses important error information. Error objects may contain additional context like status codes or validation details. Consider preserving the full error object or extracting the message more carefully using error.message for Error instances.
| @@ -0,0 +1,96 @@ | |||
| import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest' | |||
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent semicolon usage across test files. LoginPage.test.tsx and AuthContext.test.tsx omit semicolons, while LogoutPage.test.tsx and ProtectedRoute.test.tsx include them. Consider standardizing on one style across all test files for consistency.
| await logout(); | ||
| navigate('/login', { replace: true }); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling in the performLogout function. If the logout operation fails, the user won't be notified and navigation to /login may not occur. Consider wrapping the await logout() call in a try-catch block or using .catch() to handle errors gracefully.
| await logout(); | |
| navigate('/login', { replace: true }); | |
| try { | |
| await logout(); | |
| } catch (error) { | |
| console.error('Failed to log out:', error); | |
| } finally { | |
| navigate('/login', { replace: true }); | |
| } |
| <AuthProvider> | ||
| <Routes> | ||
| {/* --- Public Routes --- */} | ||
| <Route path="/" element={<HomePage />} /> |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root path "/" is not protected, but VOLUNTEER users are redirected there after login. This creates an inconsistency where authenticated volunteers access the same page as unauthenticated users. Consider either protecting the root route with a ProtectedRoute component (allowing both roles) or redirecting volunteers to a different protected page.
| <Route path="/" element={<HomePage />} /> | |
| <Route | |
| path="/" | |
| element={ | |
| <ProtectedRoute> | |
| <HomePage /> | |
| </ProtectedRoute> | |
| } | |
| /> |
| @@ -0,0 +1,69 @@ | |||
| import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest' | |||
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent semicolon usage across test files. AuthContext.test.tsx and LoginPage.test.tsx omit semicolons, while LogoutPage.test.tsx and ProtectedRoute.test.tsx include them. Consider standardizing on one style across all test files for consistency.
| <div className="login-container"> | ||
| <h1>Login</h1> | ||
|
|
||
| {error && <div className="error-message">{error}</div>} |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message div lacks appropriate ARIA attributes for screen readers. Consider adding role="alert" and aria-live="polite" to ensure error messages are announced to users with assistive technologies.
| {error && <div className="error-message">{error}</div>} | |
| {error && ( | |
| <div className="error-message" role="alert" aria-live="polite"> | |
| {error} | |
| </div> | |
| )} |
| apiClient.interceptors.request.use((config) => { | ||
| const token = localStorage.getItem('authToken'); // Or get from context | ||
| const token = localStorage.getItem('accessToken'); | ||
| if (token) { | ||
| config.headers.Authorization = `Token ${token}`; | ||
| config.headers.Authorization = `Bearer ${token}`; | ||
| } | ||
| return config; | ||
| }); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing response interceptor to handle 401 Unauthorized errors and token refresh logic. When an access token expires, the API will return 401, but there's no automatic token refresh mechanism. Consider adding a response interceptor that attempts to refresh the token using the refresh token before logging the user out.
Summary
Creates the frontend login experience for admins and volunteers.
Related Issues
Changes
-
POST /api/auth/login→ store JWT or session tokenHow to Test
http://localhost:5173/login/admin(ADMIN) or/(VOLUNTEER)http://localhost:5173/logout(redirects to login)/adminis inaccessible without authenticationScreenshots / UI
Checklist