Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the core authentication and profile management features! The overall structure is solid. However, some changes are needed because a key feature is missing and a few requirements are not fully met.
Here are the main points to address:
- Password Reset is Missing: The entire password reset flow is required but has not been implemented. Please add the routes in
client/src/Root.jsxand create the necessary pages for a user to request a password reset and confirm it. - Incorrect Activation Flow: After successful account activation in
ActivationPage.jsx, the user must be automatically logged in and redirected to their profile page, rather than just seeing a link to the login page. - Broken Modal Styles: The CSS in
client/src/components/Modal/Modal.module.scsshas invalid syntax for fallbacks. For example,background-color: var(--card-background), grey;should bebackground-color: var(--card-background, grey);to be valid. - Incorrect Barrel File Exports: In
client/src/components/CustomSelect/index.jsandclient/src/components/Header/index.js, you are usingexport *for default exports. This doesn't work. Please change it toexport { default } from './CustomSelect';to correctly re-export the components.
Once these issues are resolved, the project will be in great shape. Keep up the excellent effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| <Routes> | ||
| <Route path="/" element={<App />}> | ||
| <Route index element={<HomePage />} /> | ||
| <Route path="home" element={<Navigate replace to="/" />} /> | ||
| <Route path="login" element={<LoginPage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> | ||
|
|
||
| <Route path="auth/activate/:token" element={<ActivationPage />} /> | ||
|
|
||
| {/* ЗАХИЩЕНІ МАРШРУТИ */} | ||
| <Route element={<ProtectedRoute />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| <Route path="categories" element={<CategoriesPage />} /> | ||
| <Route path="expenses" element={<ExpensesPage />} /> | ||
| {/* Тут можна додати інші приватні сторінки, наприклад /settings */} | ||
| </Route> | ||
|
|
||
| <Route path="*" element={<NotFound />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
The routing configuration is missing the required pages for the password reset flow. According to the task description, you need to implement pages for asking for an email, a password reset confirmation, and a success page. Please add the corresponding routes for this functionality.
| @@ -0,0 +1 @@ | |||
| export * from './CustomSelect'; | |||
There was a problem hiding this comment.
This export * statement only re-exports named exports, but the CustomSelect2 component in ./CustomSelect.tsx is a default export. This means the component itself is not being exported from this barrel file. To fix this and make the component importable from the directory, you should re-export the default. For example: export { default } from './CustomSelect';
| <select | ||
| required | ||
| value={formData.categoryId} | ||
| onChange={e => setFormData({...formData, categoryId: e.target.value})} | ||
| > | ||
| <option value="">Оберіть категорію</option> | ||
| {categories.map(cat => ( | ||
| <option key={cat.id} value={cat.id}>{cat.name}</option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
Good job on the form logic! I noticed the comment here suggests using a custom select component. While the native <select> element is functional, this would be a great opportunity to use your CustomSelect component to ensure a consistent look and feel across your application.
| @@ -0,0 +1 @@ | |||
| export * from './Header'; | |||
There was a problem hiding this comment.
This syntax (export * from './Header') only re-exports named exports from the ./Header module. However, the Header component is exported as a default export. To correctly re-export a default export, you should use export { default } from './Header';.
| background-color: var(--card-background), grey; | ||
| border-radius: var(--card-radius), 8px; |
There was a problem hiding this comment.
The syntax for background-color and border-radius is incorrect. If grey and 8px are meant to be fallback values for the CSS variables, they should be placed inside the var() function, like var(--variable-name, fallback-value). The current comma-separated values will cause these rules to be ignored by the browser.
| justify-content: center; | ||
| align-items: center; | ||
| background: none; | ||
| color: var(--primary), black; |
There was a problem hiding this comment.
This color property has invalid syntax. A fallback value for a CSS variable must be provided as the second argument to the var() function, not as a comma-separated value.
| border-radius: var(--button-round-square-radius), 8px; | ||
| border: 1px solid var(--button-border-color-hover), blue; | ||
| background-color: var(--button-bg-color-hover), purple; |
There was a problem hiding this comment.
These properties (border-radius, border, background-color) have invalid syntax. Comma-separated values are not a valid way to provide fallbacks for CSS custom properties. This will cause the hover effect to be broken. Please correct the syntax to use the var() function's fallback mechanism.
| <> | ||
| <h1>Акаунт активовано успішно! ✅</h1> | ||
| <p>Тепер ви можете увійти у свій профіль.</p> | ||
| <Link to="/login" className="button">Перейти до логіну</Link> | ||
| </> |
There was a problem hiding this comment.
The success message is helpful, but the task requires redirecting the user to their Profile page after activation. Currently, you are only showing a link to the login page. To meet the requirement, the user should be automatically logged in and then redirected to their profile.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the password reset flow and fixing the account activation logic! The project is moving in the right direction. However, a few critical issues need to be addressed before this can be approved.
Here are the required changes:
- Missing "Change Password" Feature: This is a core requirement. The form is currently commented out in
client/src/pages/ProfilePage/ProfilePage.jsx. Additionally, the backend logic insrc/controllers/users.controller.jsmust be updated to verify the user's old password before setting a new one. - Broken Logout Functionality: The logout process is failing silently. In
src/controllers/auth.controller.js, thelogoutfunction incorrectly usesusersRepository.getByToken()which searches byactivationToken. You'll need to create and use a new repository method likegetByRefreshTokento correctly clear the token. - Password Rules on Registration: The registration page (
client/src/pages/RegistrationPage/RegistrationPage.jsx) needs to display the password requirements to users, as this is a required part of the user experience.
Please fix these issues to meet the project requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| amount: parseFloat(formData.amount) | ||
| }; | ||
|
|
||
| const response = await axios.post('http://localhost:5700/expenses', dataToSend); |
There was a problem hiding this comment.
It's great that you've set up a dedicated api instance for making requests. For consistency, it would be better to use that instance here instead of calling axios directly. This helps centralize your API configuration.
For example: const response = await api.post('/expenses', dataToSend);
| <select | ||
| required | ||
| value={formData.categoryId} | ||
| onChange={e => setFormData({...formData, categoryId: e.target.value})} | ||
| > | ||
| <option value="">Оберіть категорію</option> | ||
| {categories.map(cat => ( | ||
| <option key={cat.id} value={cat.id}>{cat.name}</option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
The comment on the line above indicates that a custom select component should be used here. You've already created a CustomSelect component in the project. Please replace this native <select> element with your CustomSelect component to maintain a consistent UI and reuse your code effectively.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this pull request! You have successfully addressed all the feedback from the previous review and delivered a complete and robust authentication system. The code is very well-structured, particularly with the clean implementation of route guards and the reusable password validation component. All requirements have been met, and the application functions perfectly. This is a high-quality submission, and I am happy to approve it.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.