Develop #220
Develop #220
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your submission to help you move forward, as this is a great learning opportunity. You've done an excellent job building a robust backend with a clean architecture and a well-structured frontend.
However, there are a few important areas to review for your next project:
- Build Failure: The project build is failing because of a
postinstallscript inserver/package.json. Removing this script will fix the installation. - Critical Bugs: The email reset page (
ResetEmailPage.tsx) has a few bugs in itsuseEffecthook that leave the user on an infinite loading screen if an error occurs. Also, on the server, thegetUsercontroller (user.controller.js) incorrectly tries to usereq.bodyfor a GET request; it should usereq.userfrom the authentication middleware instead. - Routing and Navigation: The activation page is currently accessible to logged-in users but should be restricted to non-authenticated users by moving it inside the
RequireNonAuthguard inRoot.tsx. Additionally, the navigation in theHeader.tsxshould use absolute paths (e.g.,'/login') instead of relative ones to prevent broken links.
Keep up the fantastic work! Your progress is impressive, and paying attention to these details will make your future projects even stronger.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <Link to="/profile/details" className="not-found__button"> | ||
| Back to main | ||
| </Link> |
There was a problem hiding this comment.
This link is hardcoded to /profile/details. Consider the scenario where a user who is not logged in encounters this page. While the RequireAuth component will correctly redirect them to the login page, a more user-friendly approach would be to link to the root / and let the main routing logic determine the appropriate destination based on the user's authentication status.
| setError(null); | ||
|
|
||
| login(email, password) | ||
| .then(() => navigate('/profile')) |
There was a problem hiding this comment.
Redirecting to /profile meets the basic requirement, but it leads to an empty content area on the profile page because no specific tab (like 'details') is selected. For a better user experience and consistency with other parts of the app (like activation), consider redirecting to /profile/details to show the user's information immediately.
| {item === 'details' && ( | ||
| <ProfileTab /> | ||
| )} |
There was a problem hiding this comment.
When the user navigates to the base /profile path, the item param is undefined. This causes none of the tab components to be rendered, leaving the content area blank. To provide a better user experience, a default tab (like 'details') should be displayed. You can achieve this by adjusting the rendering condition for the ProfileTab to also account for when item is not present.
| {serverResponse && serverResponse} | ||
| {error && <h1>{error}</h1>} |
There was a problem hiding this comment.
Rendering the success message directly as a string and the error message inside an <h1> tag can lead to inconsistent styling and is semantically incorrect for an error. For better user experience and code quality, consider wrapping these messages in dedicated, styled components (e.g., a <p> or a <div> with a specific class for success or error states) similar to how it's done on other pages.
| /> | ||
| </div> | ||
| </div> | ||
| <button className="profile-form__submit-button" type="submit" disabled={newPassword !== confirmNewPass}>Update Password</button> |
There was a problem hiding this comment.
The current disabled logic only checks if the newPassword and confirmNewPass fields match. This means the button is enabled even when all fields are empty, which could lead to an unnecessary API call and a confusing user experience. Consider expanding this condition to also ensure that all three password fields have a value before the button is enabled.
| <button | ||
| onClick={() => navigate('login')} | ||
| className="btn-text" | ||
| > | ||
| Login | ||
| </button> | ||
| <button | ||
| onClick={() => navigate('registration')} | ||
| className="btn-primary" | ||
| > | ||
| Sign Up | ||
| </button> |
There was a problem hiding this comment.
Similar to the logo navigation, using relative paths for these buttons can lead to unexpected behavior. It's more reliable to use absolute paths like '/login' and '/registration' to ensure the user is always directed to the correct top-level page.
| const location = useLocation(); | ||
|
|
||
| if (!isChecked) { | ||
| return <Loader2 />; |
There was a problem hiding this comment.
This component uses the Loader2 icon directly from lucide-react, which will appear as a static, non-animated icon. For a consistent user experience, you should use the custom <Loader /> component from ../components/Loader/Loader (which you've used correctly in App.tsx). This will display the centered, animated loading spinner that users see elsewhere in the application.
| async function logout() { | ||
| await authService.logout(); | ||
|
|
||
| accessTokenService.remove(); | ||
| setUser(null); | ||
| } |
There was a problem hiding this comment.
The current implementation of the logout function is not fully resilient. If the authService.logout() call fails (for example, due to a network error), the promise will reject, and the subsequent lines to remove the access token and clear the user state will never be executed. This would leave the user logged in on the client side.
To ensure the user is always logged out locally, you should wrap the API call in a try...catch block or use a finally block to guarantee that the client-side cleanup logic runs regardless of the API call's outcome.
| const getUser = async (req, res) => { | ||
| const { email } = req.body; | ||
|
|
||
| const user = await userService.findByEmail(email); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.badRequest('No such user'); | ||
| } | ||
|
|
||
| res.send(userService.normalize(user)); | ||
| }; |
There was a problem hiding this comment.
This controller is set up for a GET request, which does not have a request body. To pass data with a GET request, you should use query parameters (req.query).
A better and more secure approach here would be to use the authenticated user's information, which is already available in req.user thanks to the authMiddleware. You could fetch the user's details using req.user.id and remove the need to pass the email from the client entirely.
| if (error) { | ||
| res.statusCode = 500; | ||
|
|
||
| res.send({ | ||
| message: error, | ||
| }); |
There was a problem hiding this comment.
This is a good catch-all for unexpected errors. However, sending the raw error object directly in the response can leak sensitive information to the client, such as stack traces or database details. For better security, it's a best practice to log the full error on the server for debugging and send a generic, user-friendly message to the client, like { message: 'An internal server error occurred' }.
2 participants
No description provided.