feat: Added a Page to be shown in case of no authorization/not found #113
Conversation
WalkthroughThe pull request modifies the BlueWave HRM application by updating dependencies in Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant App
participant UpdatesPage
participant ErrorPage
User->>Router: Navigate to route
Router->>App: Check authentication
alt User is authenticated
App->>UpdatesPage: Render dashboard
else User is not authenticated
App->>ErrorPage: Redirect to error page
ErrorPage-->>User: Display error message
end
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)src/App.js(1 hunks)src/components/ErrorPage/ErrorPage.js(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/App.js (1)
13-13:⚠️ Potential issueInconsistent routing behaviour between root and dashboard
The root path still allows access without authentication while dashboard requires it. This could lead to security issues.
-<Route path="/" element={<UpdatesPage />} /> +<Route + path="/" + element={ + <PrivateRoute> + <UpdatesPage /> + </PrivateRoute> + } +/>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(3 hunks)src/App.js(1 hunks)src/components/ErrorPage/ErrorPage.js(1 hunks)src/components/PrivateRoute.js(1 hunks)src/context/AuthContext.js(1 hunks)
🔇 Additional comments (3)
src/components/ErrorPage/ErrorPage.js (1)
1-51: 🧹 Nitpick (assertive)Add test coverage for ErrorPage component
The component lacks test coverage. Consider adding tests for different scenarios.
Would you like me to generate test cases for the ErrorPage component? This should include tests for:
- Default error message
- Custom message prop
- Different error codes
- Button navigation
package.json (2)
6-7: LGTM! Package versions are compatible.The updates to @emotion/* and @mui/material packages maintain compatibility while providing new features for styling components.
Also applies to: 10-10
20-20: Verify react-router-dom compatibility with existing code.The version is correct (6.22.1). However, since this is a major architectural change:
- Ensure all route definitions follow v6 syntax
- Verify that any existing navigation logic is updated to use v6 hooks (useNavigate instead of useHistory)
Let's verify the routing implementation:
✅ Verification successful
React Router v6 Implementation Verified
The codebase is fully compatible with react-router-dom v6:
- Route definitions use correct v6 syntax with
Routesandelementprops- Navigation implemented using v6
useNavigatehook- No legacy v5 patterns detected in routing logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for routing-related code to ensure v6 compatibility echo "Checking route definitions..." rg -l "Route|Routes|Router|useNavigate|useHistory" --type js echo "Checking for potential v5 patterns..." rg -l "Redirect|Switch|withRouter" --type jsLength of output: 587
Script:
#!/bin/bash # Check route definitions and navigation patterns echo "=== Route Definitions ===" rg "Route|Routes|BrowserRouter|HashRouter" -A 2 src/App.js src/components/PrivateRoute.js echo -e "\n=== Navigation Hooks ===" rg "use(Navigate|History)" -A 1 --type js echo -e "\n=== UpdatesList Router Imports ===" rg "^import.*from ['\"]react-router" src/components/UpdatesPage/UpdatesList.jsxLength of output: 1916
| export const AuthProvider = ({ children }) => { | ||
| const [isAuthenticated, setIsAuthenticated] = useState(false); | ||
|
|
||
| return ( | ||
| <AuthContext.Provider value={{ isAuthenticated, setIsAuthenticated }}> | ||
| {children} | ||
| </AuthContext.Provider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance authentication context with loading and error states
The current implementation lacks loading and error states which are crucial for handling authentication flow properly.
export const AuthProvider = ({ children }) => {
- const [isAuthenticated, setIsAuthenticated] = useState(false);
+ const [authState, setAuthState] = useState({
+ isAuthenticated: false,
+ isLoading: true,
+ error: null
+ });
+
+ const setIsAuthenticated = (value) => {
+ setAuthState(prev => ({
+ ...prev,
+ isAuthenticated: value,
+ isLoading: false
+ }));
+ localStorage.setItem('isAuthenticated', value);
+ };
return (
- <AuthContext.Provider value={{ isAuthenticated, setIsAuthenticated }}>
+ <AuthContext.Provider value={{ ...authState, setIsAuthenticated }}>
{children}
</AuthContext.Provider>
);
};📝 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.
| export const AuthProvider = ({ children }) => { | |
| const [isAuthenticated, setIsAuthenticated] = useState(false); | |
| return ( | |
| <AuthContext.Provider value={{ isAuthenticated, setIsAuthenticated }}> | |
| {children} | |
| </AuthContext.Provider> | |
| ); | |
| }; | |
| export const AuthProvider = ({ children }) => { | |
| const [authState, setAuthState] = useState({ | |
| isAuthenticated: false, | |
| isLoading: true, | |
| error: null | |
| }); | |
| const setIsAuthenticated = (value) => { | |
| setAuthState(prev => ({ | |
| ...prev, | |
| isAuthenticated: value, | |
| isLoading: false | |
| })); | |
| localStorage.setItem('isAuthenticated', value); | |
| }; | |
| return ( | |
| <AuthContext.Provider value={{ ...authState, setIsAuthenticated }}> | |
| {children} | |
| </AuthContext.Provider> | |
| ); | |
| }; |
| export const AuthContext = createContext(); | ||
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [isAuthenticated, setIsAuthenticated] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add persistence for authentication state
The authentication state will be reset on page reload since it's using a simple useState. Consider using localStorage or sessionStorage to persist the authentication state.
-const [isAuthenticated, setIsAuthenticated] = useState(false);
+const [isAuthenticated, setIsAuthenticated] = useState(() => {
+ return localStorage.getItem('isAuthenticated') === 'true';
+});📝 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 [isAuthenticated, setIsAuthenticated] = useState(false); | |
| const [isAuthenticated, setIsAuthenticated] = useState(() => { | |
| return localStorage.getItem('isAuthenticated') === 'true'; | |
| }); |
| const PrivateRoute = () => { | ||
| const { isAuthenticated } = useContext(AuthContext); | ||
|
|
||
| return isAuthenticated ? <UpdatesPage /> : <Navigate to="/error-unauthenticated" />; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance PrivateRoute component reusability
The current implementation hard-codes the UpdatesPage component, making it non-reusable for other protected routes.
-const PrivateRoute = () => {
+const PrivateRoute = ({ children }) => {
const { isAuthenticated } = useContext(AuthContext);
- return isAuthenticated ? <UpdatesPage /> : <Navigate to="/error-unauthenticated" />;
+ return isAuthenticated ? children : <Navigate to="/error-unauthenticated" />;
};📝 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 PrivateRoute = () => { | |
| const { isAuthenticated } = useContext(AuthContext); | |
| return isAuthenticated ? <UpdatesPage /> : <Navigate to="/error-unauthenticated" />; | |
| }; | |
| const PrivateRoute = ({ children }) => { | |
| const { isAuthenticated } = useContext(AuthContext); | |
| return isAuthenticated ? children : <Navigate to="/error-unauthenticated" />; | |
| }; |
| import UpdatesPage from './UpdatesPage/UpdatesPage'; | ||
|
|
||
| const PrivateRoute = () => { | ||
| const { isAuthenticated } = useContext(AuthContext); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle loading state in authentication check
The component should handle the loading state from AuthContext to prevent flashing content.
- const { isAuthenticated } = useContext(AuthContext);
+ const { isAuthenticated, isLoading } = useContext(AuthContext);
+
+ if (isLoading) {
+ return <div>Loading...</div>; // Consider using a proper loading component
+ }📝 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 { isAuthenticated } = useContext(AuthContext); | |
| const { isAuthenticated, isLoading } = useContext(AuthContext); | |
| if (isLoading) { | |
| return <div>Loading...</div>; // Consider using a proper loading component | |
| } |
| <Routes> | ||
| <Route path="/" element={<UpdatesPage />} /> | ||
| <Route path="/dashboard" element={<PrivateRoute />} /> | ||
| <Route path="/error-unauthenticated" element={<ErrorPage />} /> | ||
| <Route path="*" element={<ErrorPage />} /> | ||
| </Routes> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Organize routes for better maintainability
Consider grouping related routes and using route constants for better maintainability.
+const ROUTES = {
+ ROOT: '/',
+ DASHBOARD: '/dashboard',
+ ERROR: '/error-unauthenticated',
+};
<Routes>
- <Route path="/" element={<UpdatesPage />} />
- <Route path="/dashboard" element={<PrivateRoute />} />
- <Route path="/error-unauthenticated" element={<ErrorPage />} />
- <Route path="*" element={<ErrorPage />} />
+ {/* Protected Routes */}
+ <Route
+ path={ROUTES.ROOT}
+ element={
+ <PrivateRoute>
+ <UpdatesPage />
+ </PrivateRoute>
+ }
+ />
+ <Route
+ path={ROUTES.DASHBOARD}
+ element={
+ <PrivateRoute>
+ <UpdatesPage />
+ </PrivateRoute>
+ }
+ />
+
+ {/* Error Routes */}
+ <Route path={ROUTES.ERROR} element={<ErrorPage />} />
+ <Route path="*" element={<ErrorPage />} />
</Routes>Committable suggestion skipped: line range outside the PR's diff.
| const ErrorPage = ({ message }) => { | ||
| return ( | ||
| <Container role="alert" aria-live="polite"> | ||
| <Heading>We cannot find this page</Heading> | ||
| <Text>{message || 'Either the URL doesn’t exist, or you don’t have access to it.'}</Text> | ||
| <Button to="/">Go to the main dashboard</Button> | ||
| </Container> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error code prop for different error scenarios
The component could be more flexible by accepting an error code prop to display different messages based on the error type.
-const ErrorPage = ({ message }) => {
+const ErrorPage = ({ message, errorCode }) => {
+ const getDefaultMessage = (code) => {
+ switch(code) {
+ case 401:
+ return 'You need to be authenticated to access this page.';
+ case 404:
+ return 'The page you are looking for does not exist.';
+ default:
+ return 'Either the URL doesn't exist, or you don't have access to it.';
+ }
+ };
+
return (
<Container role="alert" aria-live="polite">
<Heading>We cannot find this page</Heading>
- <Text>{message || 'Either the URL doesn't exist, or you don't have access to it.'}</Text>
+ <Text>{message || getDefaultMessage(errorCode)}</Text>
<Button to="/">Go to the main dashboard</Button>
</Container>
);
};📝 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 ErrorPage = ({ message }) => { | |
| return ( | |
| <Container role="alert" aria-live="polite"> | |
| <Heading>We cannot find this page</Heading> | |
| <Text>{message || 'Either the URL doesn’t exist, or you don’t have access to it.'}</Text> | |
| <Button to="/">Go to the main dashboard</Button> | |
| </Container> | |
| ); | |
| }; | |
| const ErrorPage = ({ message, errorCode }) => { | |
| const getDefaultMessage = (code) => { | |
| switch(code) { | |
| case 401: | |
| return 'You need to be authenticated to access this page.'; | |
| case 404: | |
| return 'The page you are looking for does not exist.'; | |
| default: | |
| return 'Either the URL doesn't exist, or you don't have access to it.'; | |
| } | |
| }; | |
| return ( | |
| <Container role="alert" aria-live="polite"> | |
| <Heading>We cannot find this page</Heading> | |
| <Text>{message || getDefaultMessage(errorCode)}</Text> | |
| <Button to="/">Go to the main dashboard</Button> | |
| </Container> | |
| ); | |
| }; |
| const Button = styled(Link)` | ||
| display: inline-block; | ||
| padding: 10px 20px; | ||
| background-color: #6c63ff; | ||
| color: #fff; | ||
| text-decoration: none; | ||
| border-radius: 5px; | ||
| `; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add hover state to Button component
The button lacks visual feedback on hover, which is important for user interaction.
const Button = styled(Link)`
display: inline-block;
padding: 10px 20px;
background-color: #6c63ff;
color: #fff;
text-decoration: none;
border-radius: 5px;
+ transition: background-color 0.2s ease;
+
+ &:hover {
+ background-color: #5a52cc;
+ }
`;📝 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 Button = styled(Link)` | |
| display: inline-block; | |
| padding: 10px 20px; | |
| background-color: #6c63ff; | |
| color: #fff; | |
| text-decoration: none; | |
| border-radius: 5px; | |
| `; | |
| const Button = styled(Link)` | |
| display: inline-block; | |
| padding: 10px 20px; | |
| background-color: #6c63ff; | |
| color: #fff; | |
| text-decoration: none; | |
| border-radius: 5px; | |
| transition: background-color 0.2s ease; | |
| &:hover { | |
| background-color: #5a52cc; | |
| } | |
| `; |
|
@imrehankhan can you please also add a screenshot of the page? @GabrielChan1 assigned the review for you. |
@gorkem-bwl uploaded screenshot of error page
Screen.Recording.2025-01-10.at.7.15.46.PM.mov@gorkem-bwl also uploaded screen recording of the feature |
|
Thanks @imrehankhan - both the video and the screen look good. |
You're welcome @gorkem-bwl |
|
@gorkem-bwl can you please merge this pr? |
For this one we'll need @GabrielChan1's input. |
Solved isssue #56
Changes made
1. Error Page Component:
• Created a reusable ErrorPage component that displays a user-friendly error message.
• Added a “Go to the Dashboard” button to redirect users to the UpdatesPage.
• Added PropTypes validation for the message prop.
• Enhanced accessibility by adding role="alert" and aria-live="polite" attributes.
• Moved inline styles to use Material-UI's styling solution using @emotion/styled.
2. Routing Logic:
• Updated App.js to handle routing based on the isAuthenticated state:
• Authenticated users are shown an error page (ErrorPage) for undefined routes.
• Unauthenticated users are also redirected to the same error page for restricted or invalid routes.
How to Test
1. Log in to the application, by setting "isAuthenticated" value to "true" in AuthContext.js file, located in src/Context/AuthContext.js:
• Navigate to an undefined route, e.g., /random-page.
• Verify that the error page is displayed and the “Go to the Dashboard” button redirects to /updatesPage.
• Navigate to a defined route, e.g., /dashboard
• Verify that the error page is not displayed.
3. Without logging in (when isAuthenticated value is set to false):
• Navigate to any restricted or invalid route, e.g., /dashboard.
• Confirm that the error page is displayed.
Components Added
• ErrorPage.js: Located under the components/ErrorPage
Screen.Recording.2025-01-10.at.7.15.46.PM.mov