solution #224
solution #224
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your backend implementation is excellent and shows a strong understanding of authentication flows and security. However, I am requesting changes because the frontend is missing several critical features required by the task. The most significant issues are the complete absence of the password reset flow and the profile management UI for changing user details. Additionally, the authentication state in App.jsx is not managed correctly, causing UI inconsistencies after a page refresh or logout. Finally, the registration process cannot be completed because the client-side route for account activation is missing. Please implement these core features to meet the project requirements.
✨ 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
| # React + Vite | ||
|
|
||
| This template provides a minimal setup to get React working in Vite with HMR and some ESLint rules. | ||
|
|
||
| Currently, two official plugins are available: | ||
|
|
||
| - [@vitejs/plugin-react](https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react) uses [Babel](https://babeljs.io/) (or [oxc](https://oxc.rs) when used in [rolldown-vite](https://vite.dev/guide/rolldown)) for Fast Refresh | ||
| - [@vitejs/plugin-react-swc](https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react-swc) uses [SWC](https://swc.rs/) for Fast Refresh | ||
|
|
||
| ## React Compiler | ||
|
|
||
| The React Compiler is not enabled on this template because of its impact on dev & build performances. To add it, see [this documentation](https://react.dev/learn/react-compiler/installation). | ||
|
|
||
| ## Expanding the ESLint configuration | ||
|
|
||
| If you are developing a production application, we recommend using TypeScript with type-aware lint rules enabled. Check out the [TS template](https://github.com/vitejs/vite/tree/main/packages/create-vite/template-react-ts) for information on how to integrate TypeScript and [`typescript-eslint`](https://typescript-eslint.io) in your project. |
There was a problem hiding this comment.
This appears to be the default README file generated by Vite. It's a good practice to update this file to describe your actual project, including what it does, how to set it up, and how to run it. This makes it easier for others (and your future self!) to understand the project.
|
|
||
| import { Navigate } from "react-router-dom"; | ||
|
|
||
| export default function ProtectedRoute({ children }) { |
There was a problem hiding this comment.
The parent component (App.jsx) passes an isAuthed prop, but this component doesn't accept or use it. Instead, it determines the authentication status on its own by reading from localStorage. To avoid confusion and potential bugs, it's best to have a single source of truth. Consider modifying this component to accept and use the isAuthed prop.
| #root { | ||
| max-width: 1280px; | ||
| margin: 0 auto; | ||
| padding: 2rem; | ||
| text-align: center; | ||
| } | ||
|
|
||
| .logo { | ||
| height: 6em; | ||
| padding: 1.5em; | ||
| will-change: filter; | ||
| transition: filter 300ms; | ||
| } | ||
| .logo:hover { | ||
| filter: drop-shadow(0 0 2em #646cffaa); | ||
| } | ||
| .logo.react:hover { | ||
| filter: drop-shadow(0 0 2em #61dafbaa); | ||
| } | ||
|
|
||
| @keyframes logo-spin { | ||
| from { | ||
| transform: rotate(0deg); | ||
| } | ||
| to { | ||
| transform: rotate(360deg); | ||
| } | ||
| } | ||
|
|
||
| @media (prefers-reduced-motion: no-preference) { | ||
| a:nth-of-type(2) .logo { | ||
| animation: logo-spin infinite 20s linear; | ||
| } | ||
| } | ||
|
|
||
| .card { | ||
| padding: 2em; | ||
| } | ||
|
|
||
| .read-the-docs { | ||
| color: #888; | ||
| } |
There was a problem hiding this comment.
This file seems to contain the default styles from the Vite template. Many of the application's components use class names like header, table, and field which are not defined here. It's good practice to remove unused styles to keep the codebase clean and relevant to your project.
| @@ -0,0 +1,85 @@ | |||
| import { Routes, Route, Link, Navigate, useNavigate } from 'react-router-dom'; | |||
| import './App.css' | |||
| import Register from './pages/Registe'; | |||
There was a problem hiding this comment.
There appears to be a typo in the filename for this import. The file is named Registe.jsx instead of Register.jsx. It's a good practice to maintain consistent and correct naming for files and components.
client/src/App.jsx
Outdated
| try { | ||
| const data = await apiRequest('/auth/refresh', { method: 'POST', body: {} }); | ||
| localStorage.setItem('accessToken', data.accessToken); | ||
| setAccessToken(data.accessToken); |
There was a problem hiding this comment.
After a successful token refresh, you are setting the accessToken but not updating the isAuthed state. This means that even if a user has a valid session, the UI will not reflect their authenticated state on a page refresh. You should also call setIsAuthed(true) here.
| localStorage.removeItem('accessToken'); | ||
| setAccessToken(''); |
There was a problem hiding this comment.
When the user logs out, the isAuthed state is not updated to false. This can lead to an inconsistent UI where parts of the application might still behave as if the user is logged in. You should add setIsAuthed(false) here to correctly reflect the change in authentication status.
| <Routes> | ||
| <Route | ||
| path="/" | ||
| element={ | ||
| <ProtectedRoute isAuthed={isAuthed}> | ||
| <Home /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/login" | ||
| element={ | ||
| <Login | ||
| onLoggedIn={(token) => { | ||
| localStorage.setItem('accessToken', token); | ||
| setAccessToken(token); | ||
| setIsAuthed(true); | ||
| }} | ||
| /> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/register" | ||
| element={isAuthed ? <Navigate to="/" replace /> : <Register />} | ||
| /> | ||
| <Route path="*" element={<div>404</div>} /> | ||
| </Routes> |
There was a problem hiding this comment.
The routing is missing the required pages for the password reset flow. According to the task description, you need to implement routes for a page to request a password reset email and a confirmation page to set the new password.
| return ( | ||
| <div> | ||
| <h1>Profile</h1> | ||
| <p><b>Name:</b> {me.userName}</p> | ||
| <p><b>Email:</b> {me.email}</p> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
This profile page successfully displays the user's name and email. However, it's missing the required functionality for a user to manage their profile. According to the task description, you need to add UI elements (like forms and buttons) to allow the user to change their name, password, and email.
| <p className="appeal"> | ||
| No account? | ||
| </p> | ||
| <Link to="/register" className="linkRegistration">Registration</Link> |
There was a problem hiding this comment.
The task requires a password reset flow for non-authenticated users. This page is the ideal place to add a "Forgot Password?" link to guide users to the password reset page.
| "dialect": "mysql" | ||
| }, | ||
| "test": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_test", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" | ||
| }, | ||
| "production": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_production", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" |
There was a problem hiding this comment.
The database configuration in this file specifies mysql as the dialect. However, the application's actual database setup in src/db.js uses postgres. This file appears to be unused and could cause confusion.
| <form onSubmit={onSubmit} className="table"> | ||
| <input | ||
| className="field" | ||
| type="Name" | ||
| value={name} | ||
| onChange={e => setName(e.target.value)} | ||
| /> | ||
|
|
||
| <input | ||
| className="field" | ||
| type="Email" | ||
| placeholder="Email" | ||
| value={email} | ||
| onChange={e => setEmail(e.target.value)} | ||
| /> | ||
|
|
||
| {/* <input | ||
| className="field" | ||
| placeholder="Email" | ||
| value={password} | ||
| onChange={e => setPassword(e.target.value)} | ||
| /> */} | ||
| <input | ||
| className="field" | ||
| type="password" | ||
| placeholder="Password" | ||
| value={password} | ||
| onChange={e => setPassword(e.target.value)} | ||
| /> | ||
|
|
||
| <input | ||
| className="field" | ||
| type="password" | ||
| placeholder="Confirm password" | ||
| value={confirmation} | ||
| onChange={e => setConfirmation(e.target.value)} | ||
| /> | ||
|
|
||
| {error && <div style={{ color: 'crimson' }}>{error}</div>} | ||
|
|
||
| <button type="submit">Create account</button> |
There was a problem hiding this comment.
The task requires you to inform users about the rules for a password. While the validation is likely handled on the backend, the frontend should display these rules (e.g., "Password must be at least 8 characters long and contain letters and numbers") to the user near the password input fields.
client/src/pages/Registe.jsx
Outdated
| <form onSubmit={onSubmit} className="table"> | ||
| <input | ||
| className="field" | ||
| type="Name" |
There was a problem hiding this comment.
The type attribute value "Name" is not a valid HTML input type. For a name field, you should use type="text".
client/src/pages/Registe.jsx
Outdated
|
|
||
| <input | ||
| className="field" | ||
| type="Email" |
There was a problem hiding this comment.
Similarly, the type attribute value "Email" is not valid. The correct value for an email input is type="email". Using the correct type helps browsers with validation and can provide a better user experience on mobile devices by showing the appropriate keyboard.
client/src/pages/Registe.jsx
Outdated
| {/* <input | ||
| className="field" | ||
| placeholder="Email" | ||
| value={password} | ||
| onChange={e => setPassword(e.target.value)} | ||
| /> */} |
There was a problem hiding this comment.
This commented-out code block should be removed to keep the codebase clean.
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'activation_token', |
There was a problem hiding this comment.
This activationToken field appears to be unused. The application logic generates a token, hashes it, and stores it in the activationTokenHash field. The original token is sent via email but never stored in the database. This field can likely be removed.
| resetToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'reset_token', |
There was a problem hiding this comment.
Similar to the activationToken, this resetToken field seems to be redundant. The password reset flow uses the resetTokenHash field for verification, and the raw token is never saved here. Consider removing this field to simplify the model.
|
|
||
| const { sequelize } = require('./db'); | ||
|
|
||
| require('./models/User'); |
There was a problem hiding this comment.
This script is missing the import for the RefreshSession model. If this script is run, it will not create the refresh_sessions table, leading to an incomplete database schema. The server.js file correctly imports all models before syncing, and you should do the same here if this script is intended for use.
| res.cookie(REFRESH_COOKIE, refreshToken, { | ||
| httpOnly: true, | ||
| sameSite: 'lax', | ||
| secure: false, // в проде true (https) |
There was a problem hiding this comment.
For security, the secure flag for cookies should be set to true in a production environment to ensure they are only sent over HTTPS. You can make this dynamic by checking process.env.NODE_ENV, for example: secure: process.env.NODE_ENV === 'production'. The current hardcoded value of false is insecure for production.
| await user.save(); | ||
|
|
||
| const payload = { id: user.id }; | ||
| // const accessToken = signAccessToken(payload); |
There was a problem hiding this comment.
This commented-out code should be removed to keep the codebase clean and avoid confusion.
| await createRefreshSession(user.id, refreshToken); | ||
| setRefreshCookie(res, refreshToken); | ||
|
|
||
| return res.redirect(`${process.env.CLIENT_URL}/profile?activated=1`); |
There was a problem hiding this comment.
While the requirement is to redirect to the profile page, this implementation will likely cause an issue on the client side. The client application's protected routes rely on an accessToken in localStorage, which is not available immediately after this redirect. As a result, the user will be redirected to /profile and then immediately back to /login. A better approach might be to redirect to a special client-side route that handles the session refresh before navigating to the profile.
| } | ||
|
|
||
| function computeRefreshExpiryDate() { | ||
| const days = 7; |
There was a problem hiding this comment.
This function hardcodes the expiration to 7 days, which matches the default for signRefreshToken. However, if process.env.JWT_REFRESH_EXPIRES is changed to a different value (e.g., '30d'), the expiration date stored in the database will be out of sync with the token's actual expiration. To maintain a single source of truth, this value should be derived from the same configuration used for signing the token.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the project so far! The backend is very well-implemented, and you've successfully built the entire password reset flow on the frontend.
However, this requires changes because some core requirements are not yet met. The most critical issue is that the profile page (client/src/pages/Home.jsx) is missing the required functionality. It currently only displays user data, but it needs forms to allow users to change their name, password, and email.
Additionally, the account activation flow needs to be adjusted. After a successful activation in client/src/pages/Activate.jsx, the user should be automatically redirected to their profile page, not just shown a link to log in. Finally, please remember to add the password rules to the registration page (client/src/pages/Registe.jsx) to inform users of the requirements.
✨ 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
| path="/" | ||
| element={ | ||
| <ProtectedRoute isAuthed={isAuthed}> | ||
| <Home /> |
There was a problem hiding this comment.
The Home component rendered here is meant to be the user's profile page. According to the task requirements, this page must allow the user to change their name, password, and email. Currently, the Home component only displays user information.
| return ( | ||
| <div> | ||
| <h1>Account activated</h1> | ||
| <p>You can login now.</p> | ||
| <Link to="/login">Go to login</Link> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
According to the task requirements, the user should be redirected to the Profile page after successful activation. This implementation shows a success message and a link to the login page instead of performing the redirect.
| return ( | ||
| <div> | ||
| <h1>Profile</h1> | ||
| <p><b>Name:</b> {me.userName}</p> | ||
| <p><b>Email:</b> {me.email}</p> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The profile page is missing the required forms to manage user details. You need to add functionality here to allow the user to:
- Change their name.
- Change their password (requires old password, new password, and confirmation).
- Change their email (requires password and new email confirmation).
| <input | ||
| className="field" | ||
| type="password" | ||
| placeholder="Password" | ||
| value={password} | ||
| onChange={e => setPassword(e.target.value)} | ||
| /> | ||
|
|
||
| <input | ||
| className="field" | ||
| type="password" | ||
| placeholder="Confirm password" | ||
| value={confirmation} | ||
| onChange={e => setConfirmation(e.target.value)} | ||
| /> |
There was a problem hiding this comment.
The task requires informing users about the rules for a password. Consider adding a small text element near the password fields to display these rules, for example: 'Password must be at least 8 characters long and contain both letters and numbers.'
| "dialect": "mysql" | ||
| }, | ||
| "test": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_test", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" | ||
| }, | ||
| "production": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_production", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" |
There was a problem hiding this comment.
The database dialect is set to mysql here, but the application's database setup in src/db.js uses postgres. This configuration file appears to be inconsistent with the actual implementation.
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'activation_token', | ||
| }, | ||
| activationTokenHash: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'activation_token_hash', | ||
| }, | ||
| resetTokenHash: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'reset_token_hash', | ||
| }, | ||
| resetToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'reset_token', |
There was a problem hiding this comment.
These activationToken and resetToken fields seem to be redundant. The application logic correctly uses the hashed versions (activationTokenHash and resetTokenHash) for security, but these plain token fields are never written to or read from the database. You can safely remove them to keep the model clean.
| await createRefreshSession(user.id, refreshToken); | ||
| setRefreshCookie(res, refreshToken); | ||
|
|
||
| return res.redirect(`${process.env.CLIENT_URL}/profile?activated=1`); |
There was a problem hiding this comment.
The task requires redirecting to the Profile page after activation. In your frontend application (App.jsx), the profile page is mapped to the root path (/). This redirect points to /profile, which will result in a 404 error on the client side. Please update the URL to point to the correct path.
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 solution as an educational override to help you progress, but there are a couple of critical frontend issues to address. Your backend is exceptionally well-built—it's secure, robust, and meets all requirements perfectly!
On the frontend, there are two main areas for improvement:
- Post-Activation Flow: In
client/src/pages/Activate.jsx, after a user activates their account, they are redirected to the login page instead of their profile. To fix this, you should automatically log the user in by fetching an access token after activation, updating the application's state, and then navigating to the profile page. - Email Change Form: The profile page in
client/src/pages/Home.jsxdoesn't correctly implement the email change feature. It's missing the requiredpasswordand new email confirmation fields and calls a non-existent API endpoint. You'll need to create a separate form that sends aPATCHrequest to the correct/profile/emailendpoint with all the required data.
You've made fantastic progress, and resolving these frontend issues will make your application fully functional. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| await apiRequest(`/auth/activate/${token}`, { method: 'GET' }); | ||
| setStatus('ok'); | ||
|
|
||
| navigate('/', { replace: true }); |
There was a problem hiding this comment.
It's great that you've implemented the redirect after activation. However, this redirect to / currently leads the user back to the login page.
The reason is that after the activation call succeeds, the application's main state in App.jsx (which holds the accessToken) is not updated. The ProtectedRoute therefore still considers the user as unauthenticated.
To fix this, after a successful activation, you should immediately log the user in on the client side before navigating. A good approach would be to make another API call to /auth/refresh to get an access token, update the application state (perhaps using a callback prop from App.jsx similar to how the Login component works), and then perform the navigation.
| <form onSubmit={onSaveProfile} className="table"> | ||
| <input | ||
| className="field" | ||
| type="text" | ||
| placeholder="Name" | ||
| value={userName} | ||
| onChange={(e) => setUserName(e.target.value)} | ||
| /> | ||
|
|
||
| <input | ||
| className="field" | ||
| type="email" | ||
| placeholder="Email" | ||
| value={email} | ||
| onChange={(e) => setEmail(e.target.value)} | ||
| /> | ||
|
|
||
| <button type="submit">Save</button> | ||
| </form> |
There was a problem hiding this comment.
This form attempts to update both the user's name and email with a single PATCH request to /profile. According to your backend code in src/routes/profile.routes.js, this endpoint does not exist. You have separate endpoints for updating the name (PATCH /profile/name) and email (PATCH /profile/email).
Additionally, the task requires that changing an email must include the user's current password and a confirmation of the new email. This form is missing those fields. You should create a separate form specifically for changing the email that includes these required fields and submits to the correct /profile/email endpoint.
| { | ||
| "development": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_development", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" | ||
| }, | ||
| "test": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_test", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" | ||
| }, | ||
| "production": { | ||
| "username": "root", | ||
| "password": null, | ||
| "database": "database_production", | ||
| "host": "127.0.0.1", | ||
| "dialect": "mysql" | ||
| } | ||
| } |
There was a problem hiding this comment.
This configuration file is set up for a MySQL database. However, your application's database connection logic in src/db.js is configured to connect to a PostgreSQL database using environment variables. This means the running application is ignoring this file entirely.
To avoid confusion, it would be best to either update this file to reflect your PostgreSQL setup or remove it if it's not being used for migrations.
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'activation_token', | ||
| }, | ||
| activationTokenHash: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'activation_token_hash', | ||
| }, | ||
| resetTokenHash: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'reset_token_hash', | ||
| }, | ||
| resetToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: true, | ||
| field: 'reset_token', |
There was a problem hiding this comment.
This model defines activationToken and resetToken fields, but they don't appear to be used in your application logic. In src/routes/auth.routes.js, you create tokens, hash them, and store only the hashed versions (activationTokenHash and resetTokenHash). To keep the database schema clean and avoid confusion, you could consider removing these unused activationToken and resetToken fields.
| function computeRefreshExpiryDate() { | ||
| const days = 7; | ||
| const ms = days * 24 * 60 * 60 * 1000; | ||
|
|
||
| return new Date(Date.now() + ms); | ||
| } |
There was a problem hiding this comment.
This function correctly calculates an expiration date that matches the '7d' expiration set in signRefreshToken. While this works perfectly, be aware that these two values are now coupled. If you ever need to change the refresh token's lifetime, you'll need to update it in both places.
A potential improvement for maintainability would be to define the duration (e.g., const REFRESH_TOKEN_LIFETIME_DAYS = 7;) as a constant at the top of the file and use it in both this function and signRefreshToken to ensure they always stay in sync.
2 participants
No description provided.