Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an excellent submission that implements nearly all the required features for a full-stack authentication system. I am requesting a few changes to meet all the requirements.
The main issue is that the application does not redirect the user to their Profile page after account activation. Currently, it only shows a success message with a link to log in. To fix this, the backend's activate function in src/controllers/authController.js should create a session for the user, and the frontend's Activate.jsx component should use this to log the user in and redirect them to /profile.
Additionally, please consider these improvements:
- Add error handling to the
fetchcall inclient/src/pages/ForgotPassword.jsxto handle cases where the request fails. - Refactor the
Profile.jsxcomponent to use the user data fromAuthContextinstead of fetching it again.
You've done a great job, and with these adjustments, the project will be complete.
✨ 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 [Oxc](https://oxc.rs) | ||
| - [@vitejs/plugin-react-swc](https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react-swc) uses [SWC](https://swc.rs/) | ||
|
|
||
| ## 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 README is the default one from the Vite template. It's a good practice to replace it with a description of your own project. Consider adding information about what the application does, how to install dependencies, and how to run it locally.
| .counter { | ||
| font-size: 16px; | ||
| padding: 5px 10px; | ||
| border-radius: 5px; | ||
| color: var(--accent); | ||
| background: var(--accent-bg); | ||
| border: 2px solid transparent; | ||
| transition: border-color 0.3s; | ||
| margin-bottom: 24px; | ||
|
|
||
| &:hover { | ||
| border-color: var(--accent-border); | ||
| } | ||
| &:focus-visible { | ||
| outline: 2px solid var(--accent); | ||
| outline-offset: 2px; | ||
| } | ||
| } | ||
|
|
||
| .hero { | ||
| position: relative; | ||
|
|
||
| .base, | ||
| .framework, | ||
| .vite { | ||
| inset-inline: 0; | ||
| margin: 0 auto; | ||
| } | ||
|
|
||
| .base { | ||
| width: 170px; | ||
| position: relative; | ||
| z-index: 0; | ||
| } | ||
|
|
||
| .framework, | ||
| .vite { | ||
| position: absolute; | ||
| } | ||
|
|
||
| .framework { | ||
| z-index: 1; | ||
| top: 34px; | ||
| height: 28px; | ||
| transform: perspective(2000px) rotateZ(300deg) rotateX(44deg) rotateY(39deg) | ||
| scale(1.4); | ||
| } | ||
|
|
||
| .vite { | ||
| z-index: 0; | ||
| top: 107px; | ||
| height: 26px; | ||
| width: auto; | ||
| transform: perspective(2000px) rotateZ(300deg) rotateX(40deg) rotateY(39deg) | ||
| scale(0.8); | ||
| } | ||
| } | ||
|
|
||
| #center { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 25px; | ||
| place-content: center; | ||
| place-items: center; | ||
| flex-grow: 1; | ||
|
|
||
| @media (max-width: 1024px) { | ||
| padding: 32px 20px 24px; | ||
| gap: 18px; | ||
| } | ||
| } | ||
|
|
||
| #next-steps { | ||
| display: flex; | ||
| border-top: 1px solid var(--border); | ||
| text-align: left; | ||
|
|
||
| & > div { | ||
| flex: 1 1 0; | ||
| padding: 32px; | ||
| @media (max-width: 1024px) { | ||
| padding: 24px 20px; | ||
| } | ||
| } | ||
|
|
||
| .icon { | ||
| margin-bottom: 16px; | ||
| width: 22px; | ||
| height: 22px; | ||
| } | ||
|
|
||
| @media (max-width: 1024px) { | ||
| flex-direction: column; | ||
| text-align: center; | ||
| } | ||
| } | ||
|
|
||
| #docs { | ||
| border-right: 1px solid var(--border); | ||
|
|
||
| @media (max-width: 1024px) { | ||
| border-right: none; | ||
| border-bottom: 1px solid var(--border); | ||
| } | ||
| } | ||
|
|
||
| #next-steps ul { | ||
| list-style: none; | ||
| padding: 0; | ||
| display: flex; | ||
| gap: 8px; | ||
| margin: 32px 0 0; | ||
|
|
||
| .logo { | ||
| height: 18px; | ||
| } | ||
|
|
||
| a { | ||
| color: var(--text-h); | ||
| font-size: 16px; | ||
| border-radius: 6px; | ||
| background: var(--social-bg); | ||
| display: flex; | ||
| padding: 6px 12px; | ||
| align-items: center; | ||
| gap: 8px; | ||
| text-decoration: none; | ||
| transition: box-shadow 0.3s; | ||
|
|
||
| &:hover { | ||
| box-shadow: var(--shadow); | ||
| } | ||
| .button-icon { | ||
| height: 18px; | ||
| width: 18px; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 1024px) { | ||
| margin-top: 20px; | ||
| flex-wrap: wrap; | ||
| justify-content: center; | ||
|
|
||
| li { | ||
| flex: 1 1 calc(50% - 8px); | ||
| } | ||
|
|
||
| a { | ||
| width: 100%; | ||
| justify-content: center; | ||
| box-sizing: border-box; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #spacer { | ||
| height: 88px; | ||
| border-top: 1px solid var(--border); | ||
| @media (max-width: 1024px) { | ||
| height: 48px; | ||
| } | ||
| } | ||
|
|
||
| .ticks { | ||
| position: relative; | ||
| width: 100%; | ||
|
|
||
| &::before, | ||
| &::after { | ||
| content: ''; | ||
| position: absolute; | ||
| top: -4.5px; | ||
| border: 5px solid transparent; | ||
| } | ||
|
|
||
| &::before { | ||
| left: 0; | ||
| border-left-color: var(--border); | ||
| } | ||
| &::after { | ||
| right: 0; | ||
| border-right-color: var(--border); | ||
| } | ||
| } |
There was a problem hiding this comment.
These styles seem to be part of the default Vite template and are not used in your application. It's best to remove this file to keep the codebase clean.
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>client</title> |
There was a problem hiding this comment.
It's a good practice to set a more descriptive title for your application. Consider changing this from "client" to something like "Authentication App" to better reflect the page's content.
| {status === 'success' ? ( | ||
| <> | ||
| <h1>Account activated!</h1> | ||
| <div className="alert alert-success">{message}</div> | ||
| <div className="links"><Link to="/login">Log in</Link></div> | ||
| </> |
There was a problem hiding this comment.
The task description requires that the user be redirected to the Profile page after successful activation. Currently, you are displaying a link to the login page, which requires an extra step from the user. To implement the redirect, you would need to automatically log the user in after they activate their account and then navigate them to the /profile page. This would likely require the backend to create a session and return the user data upon successful activation.
| await fetch('/api/auth/forgot-password', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| credentials: 'include', | ||
| body: JSON.stringify({ email }), | ||
| }); |
There was a problem hiding this comment.
The current implementation doesn't handle potential errors from the fetch call. If the server returns an error status (e.g., 500), or if there's a network issue, setSent(true) will still be executed, misleading the user into thinking the request was successful. You should check the response status (e.g., res.ok) and wrap the call in a try...catch block to handle network failures, similar to how you've done in other components like Login.jsx.
| useEffect(() => { | ||
| fetch('/api/profile', { credentials: 'include' }) | ||
| .then((res) => res.json()) | ||
| .then((data) => { | ||
| setUser(data.user); | ||
| setNameField(data.user.name); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
This useEffect duplicates the functionality that already exists in AuthContext.jsx. The AuthContext fetches the user's data when the application loads, making it available globally via the useAuth hook.
Fetching the data again here is unnecessary and introduces a separate local user state that could potentially become out of sync with the global state.
Consider refactoring this component to use the user object directly from the context:
- Remove this
useEffectand the local[user, setUser]state. - Get the user object from the context:
const { user, login } = useAuth(); - Initialize the
nameFieldstate based on theuserfrom the context.
This change will make AuthContext the single source of truth for user data, simplify your component, and remove a redundant API call.
| async function activate(req, res) { | ||
| const token = await Token.findOne({ | ||
| where: { | ||
| id: req.params.token, | ||
| type: 'activation', | ||
| expiresAt: { [Op.gt]: new Date() }, | ||
| }, | ||
| }); | ||
|
|
||
| if (!token) { | ||
| return res.status(400).json({ error: 'This activation link is invalid or has expired.' }); | ||
| } | ||
|
|
||
| await User.update({ isActive: true }, { where: { id: token.userId } }); | ||
| await token.destroy(); | ||
|
|
||
| res.json({ message: 'Your account has been activated! You can now log in.' }); |
There was a problem hiding this comment.
The task requires redirecting the user to their Profile page after activation. To enable this, the backend should automatically log the user in upon successful activation. Currently, you only return a success message.
Consider finding the user associated with the token, creating a session for them (e.g., req.session.userId = token.userId;), and returning the user data in the JSON response, similar to what you do in the login function. This will allow the frontend to update its auth state and perform the required redirect.
| function validatePassword(password) { | ||
| if (password.length < 8) { | ||
| return 'Password must be at least 8 characters long.'; | ||
| } | ||
|
|
||
| if (!/[A-Z]/.test(password)) { | ||
| return 'Password must contain at least one uppercase letter.'; | ||
| } | ||
|
|
||
| if (!/[a-z]/.test(password)) { | ||
| return 'Password must contain at least one lowercase letter.'; | ||
| } | ||
|
|
||
| if (!/[0-9]/.test(password)) { | ||
| return 'Password must contain at least one digit.'; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
This validatePassword function is identical to the one in authController.js. To avoid duplicating code and ensure consistency, it's a good practice to move shared functions like this to a common utility file (e.g., src/utils/validation.js). You could then import and use it in both controllers.
2 participants
Solution