Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core requirements of the auth task are not yet satisfied and there are a few technical issues that will break the app in real use.
The most important blockers are:
- Registration currently only uses
emailandpassword(for example inclient/src/services/authService.tsandclient/src/pages/RegistrationPage.tsx), but the task requires registering with name, email, and password, and theUsertype/model also lacks anamefield, so the profile-related requirements cannot be met. - The activation flow is not aligned with the requirements: the server’s
activateendpoint returns only the user while the client (authService.activate) expects{ accessToken, user }, there is no guaranteed redirect to a Profile page after activation, and login does not reliably enforce “user must be activated before they can log in” (it should detect inactive users and prompt them to activate their email). - A Profile page and password-reset flow (request reset by email, show “email sent” page, reset confirmation with password + confirmation, and final success page) are missing entirely, so multiple mandatory features (update name, change password, reset password) are not implemented.
- The login/logout redirections do not send users to the required destinations (e.g. after login and activation you should navigate to the Profile route, and after logout you should redirect to the login page), so the user flow does not match the specification.
- The Axios interceptors (e.g. in
client/src/http/httpClient.ts) lack a retry guard and proper error handling aroundauthService.refresh(), which can cause infinite retry loops or unhandled errors if the refresh token is invalid, and they directly assignrequest.headers.Authorizationwithout ensuringheadersexists, which can lead to runtime/TypeScript issues. - In CI, the root
package.jsoncannot be found, so dependencies cannot be installed and tests cannot run; you need to restore a validpackage.jsonat the repository root (or adjust the workflow tocdinto the correct subfolder) so the project builds.
Please address these critical points—adding name support end-to-end, fixing the activation/login/Profile flows and types, implementing the password-reset and profile features, hardening the HTTP interceptors, and restoring a working project structure—then your solution will be much closer to meeting all the task requirements. You already have a solid foundation (token storage, email sending, guarded routes); refining these areas will make the implementation complete and robust.
✨ 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
| <div className="loader-wrapper"> | ||
| <div className="loader is-loading"></div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
This component's props use the React.ReactNode type but React is not imported in this file. That will cause a TypeScript error. Import the type (for example: import type { ReactNode } from 'react') and update the prop type to children?: ReactNode, or import the default React if you prefer.
| const accessToken = localStorage.getItem('accessToken'); | ||
|
|
||
| if (accessToken) { | ||
| request.headers.Authorization = `Bearer ${accessToken}`; |
There was a problem hiding this comment.
Setting request.headers.Authorization directly may fail if request.headers is undefined (and can produce a TypeScript error). Ensure headers exists before assigning, e.g. create/merge a headers object (request.headers = { ...(request.headers || {}), Authorization: \\Bearer ${accessToken}\ }) or check/initialize request.headers first.
| const originalRequest = error.config; | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest!); |
There was a problem hiding this comment.
There is no retry-guard on the originalRequest. If the refreshed token is invalid or refresh fails repeatedly, the interceptor can loop/retry indefinitely. Add a flag like originalRequest._retry (set it before calling refresh) and skip retrying if it's already true.
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); |
There was a problem hiding this comment.
The call to authService.refresh() is not wrapped in try/catch. If refresh fails (e.g. refresh token expired), the interceptor will throw and may leave the app in an inconsistent state. Catch refresh errors, clear saved tokens (or call logout), and redirect the user to login or rethrow a clear error.
|
|
||
| // add `Authorization` header to all requests | ||
| httpClient.interceptors.request.use(request => { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
For consistency use the accessTokenService.get() helper instead of reading localStorage directly in the request interceptor, so token access is centralized and easier to change/test.
| const timerId = setTimeout(() => setError(''), 3000); | ||
|
|
||
| return () => clearTimeout(timerId); | ||
| }, [error]); |
There was a problem hiding this comment.
You read the access token directly from localStorage here while the project has an accessTokenService. Prefer using accessTokenService.get() for consistency and to centralize storage logic (and to make future changes easier).
| }, [error]); | ||
|
|
||
| return [error, setError] as const; | ||
| }; |
There was a problem hiding this comment.
Assigning request.headers.Authorization = ... can fail in TypeScript if request.headers is undefined. Ensure headers exist first (for example: request.headers = { ...(request.headers || {}), Authorization: ... }) or use the axios-provided AxiosRequestHeaders helper to safely set the header.
| if (!isChecked) { | ||
| return <Loader />; | ||
| } | ||
|
|
There was a problem hiding this comment.
You read the access token directly from localStorage here. You already have an accessTokenService in the project — consider using accessTokenService.get() instead to keep a single source of truth and avoid duplicating storage logic.
|
|
||
| if (currentUser) { | ||
| return <Navigate to="/" replace />; | ||
| } |
There was a problem hiding this comment.
Assigning request.headers.Authorization = ... may fail if request.headers is undefined or not the expected shape. Ensure the headers object exists or use bracket notation (request.headers = request.headers || {}; request.headers['Authorization'] = ...) to avoid runtime/type issues.
| // to awoid getting `res.data` everywhere | ||
| authClient.interceptors.response.use( | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| res => res.data, |
There was a problem hiding this comment.
You read the access token directly from localStorage here. Since accessTokenService is imported, prefer using accessTokenService.get() to centralize token handling and avoid duplicate access patterns.
| Your account is now active | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
After a successful login the code navigates to the previous location or '/' (home). The task requires redirecting the user to the Profile page after login. Consider changing the default redirect to the Profile route (for example '/profile') or explicitly navigate to the profile after login instead of '/'.
| ); | ||
| }; |
There was a problem hiding this comment.
The catch block only shows the server error message. The requirement states: “If user is not active ask them to activate their email.” Consider detecting the specific server response for an inactive account and then (a) show a clear activation prompt/link, or (b) navigate the user to an activation/resend page so they are explicitly asked to activate their email rather than only seeing a generic error notification.
| onSubmit={({ email, password }) => { | ||
| return login(email, password) | ||
| .then(() => { | ||
| const state = location.state as { from?: Location }; | ||
| navigate(state.from?.pathname ?? '/'); |
There was a problem hiding this comment.
The default redirect after successful login falls back to '/' here. The task requires redirecting the user to the Profile page after login. Update the default path to the Profile route (for example '/profile') or to whichever route you implement as the Profile page.
| if (isChecked && currentUser) { | ||
| return <Navigate to="/" />; |
There was a problem hiding this comment.
When the user is already authenticated the component redirects to '/' (<Navigate to="/" />). According to the requirements authenticated users should be redirected to the Profile page — change this to the Profile route (e.g. '/profile').
| const { login, isChecked, currentUser } = useAuth(); | ||
|
|
||
| if (isChecked && currentUser) { | ||
| return <Navigate to="/" />; | ||
| } |
There was a problem hiding this comment.
The component allows rendering the login form while isChecked might be false (auth check in progress). To enforce "only non-authenticated" access more reliably, show a loader while isChecked is false or wrap the route with RequireNonAuth so the user isn't able to interact with the form until the auth check completes.
| .catch((error: AxiosError<{ message?: string }>) => { | ||
| setError(error.response?.data?.message ?? ''); |
There was a problem hiding this comment.
If the server responds that the user is not active, the app should guide the user to activate their email (requirement: "If user is not active ask them to activate their email"). The current catch only shows the raw message. Consider detecting that specific error (by message or status) and show a clear prompt/link to resend activation or to the activation flow.
| const { logout } = useAuth(); | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
The register method only accepts email and password. The task description requires registration using name, email and password. Either update this client method to accept and send the name field (and update the backend if necessary), or clarify why name is omitted.
| const [users, setUsers] = useState<User[]>([]); | ||
|
|
There was a problem hiding this comment.
activate is declared to return Promise<AuthData> (accessToken + user) but your activation endpoint typically returns only the user (no access token). This creates a mismatch with code that expects an accessToken after activation. Either change the return type here or ensure the server returns tokens on activation.
| const location = useLocation(); | ||
| const [error, setError] = usePageError(''); | ||
| const [users, setUsers] = useState<User[]>([]); | ||
|
|
There was a problem hiding this comment.
You place email directly into the URL path. Emails can contain characters that should be encoded in a URL path segment. Use encodeURIComponent(email) (and possibly for token) to avoid malformed requests for certain emails.
| export const UsersPage = () => { | ||
| const { logout } = useAuth(); | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const [error, setError] = usePageError(''); | ||
| const [users, setUsers] = useState<User[]>([]); | ||
|
|
||
| useEffect(() => { | ||
| userService | ||
| .getAll() | ||
| .then(setUsers) | ||
| .catch(async (error: AxiosError) => { | ||
| if (error.response?.status !== 401) { | ||
| setError(error.message); |
There was a problem hiding this comment.
For clarity/strictness consider adding explicit return types for register and logout (e.g. Promise<void> or the actual response type) instead of relying on inferred any from authClient. This helps catch API mismatches earlier.
| register: (email: string, password: string) => { | ||
| return client.post('/registration', { email, password }); |
There was a problem hiding this comment.
The task requires registration to collect name, email and password, but this register function only accepts email and password. Either update this method to accept name and send it to the backend, or change the frontend form and requirements to match. Without name the app does not meet the "Register using name, email and password" requirement.
| activate: (email: string, token: string): Promise<AuthData> => { | ||
| return client.get(`/activate/${email}/${token}`); |
There was a problem hiding this comment.
activate is declared to return Promise<AuthData> (i.e. { accessToken, user }) but the backend activate endpoint currently returns only the user object (no accessToken). This mismatch will cause runtime errors where callers expect accessToken. Align the client typing/implementation with the backend (or make the backend return tokens upon activation).
| register: (email: string, password: string) => { | ||
| return client.post('/registration', { email, password }); | ||
| }, | ||
|
|
||
| activate: (email: string, token: string): Promise<AuthData> => { | ||
| return client.get(`/activate/${email}/${token}`); | ||
| }, | ||
|
|
||
| login: (email: string, password: string): Promise<AuthData> => { | ||
| return client.post('/login', { email, password }); | ||
| }, | ||
|
|
||
| logout: () => client.post('/logout'), |
There was a problem hiding this comment.
For clarity and type-safety consider adding explicit return types to register and logout (for example Promise<{ message: string }> for register if backend returns a message, and Promise<void | AxiosResponse> for logout) so callers can rely on correct shapes rather than implicit any.
|
|
||
| type RegistrationError = AxiosError<{ |
There was a problem hiding this comment.
The register method only accepts email and password, but the task requires registration with a name as well. Add a name: string parameter and include it in the payload sent to the backend (e.g. client.post('/registration', { name, email, password })).
| import { useAuth } from '../components/AuthContext'; | ||
|
|
||
| type RegistrationError = AxiosError<{ | ||
| errors?: { email?: string; password?: string }; |
There was a problem hiding this comment.
This service is inconsistent in typing: activate, login, and refresh return Promise<AuthData> but register has no explicit return type. Give register a proper return type matching the API (for example Promise<{ message: string }> or the real response shape) to improve type safety and consistency.
| import { httpClient } from '../http/httpClient'; | ||
| import { User } from '../types/user'; | ||
|
|
||
| export const userService = { |
There was a problem hiding this comment.
The User type is missing a name property. The task requires registering users with a name and allowing name changes on the Profile page, so add name: string (or name?: string if optional) to this interface so the rest of the client code can work with the user's name in a type-safe way.
|
|
||
|
|
||
| export class ApiError extends Error { | ||
| constructor({ message, status, errors = {} }) { |
There was a problem hiding this comment.
The constructor destructures its single parameter but does not provide a default value — calling new ApiError() (without an object) would throw. Consider making the parameter optional with a default empty object, e.g. constructor({ message, status, errors = {} } = {}) { ... } to make the class more robust.
|
|
||
| export class ApiError extends Error { | ||
| constructor({ message, status, errors = {} }) { | ||
| super(message); |
There was a problem hiding this comment.
After calling super(message) it's helpful to set this.name = 'ApiError' so the error type appears clearly in stack traces and logs (this does not affect instanceof checks but improves diagnostics). Add this assignment right after the super(message); line.
| import { catchError } from '../utils/catchError.js'; | ||
|
|
||
| export const authRouter = express.Router() | ||
|
|
There was a problem hiding this comment.
process.env.SMTP_PORT is used directly for the transporter port. Environment variables are strings — convert it to a number (port: Number(process.env.SMTP_PORT)) to avoid type issues with some transports and make intent explicit.
| activationToken: { | ||
| type: DataTypes.STRING, | ||
| } | ||
| }) |
There was a problem hiding this comment.
The transporter.sendMail call omits a from field. Nodemailer typically requires a from address (unless a default is configured on the transporter), and missing from may cause the send to fail. Provide a from (for example process.env.SMTP_FROM || process.env.SMTP_USER) in the send options or configure a default in the transporter.
|
|
||
| export const User = client.define('user', { | ||
| email: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
port is taken directly from process.env.SMTP_PORT (a string). Pass a number to nodemailer (e.g. port: parseInt(process.env.SMTP_PORT, 10)) or validate/coerce it to avoid subtle runtime issues. Also consider setting secure based on the port (e.g. secure: true for 465).
|
|
||
| const transporter = nodemailer.createTransport({ | ||
| host: process.env.SMTP_HOST, | ||
| port: process.env.SMTP_PORT, |
There was a problem hiding this comment.
You pass process.env.SMTP_PORT directly to Nodemailer. Environment variables are strings — call parseInt(process.env.SMTP_PORT, 10) (or coerce to number) so the transporter receives a numeric port value and to avoid subtle runtime/config issues.
| } | ||
|
|
||
| function sendActivationEmail(email, token) { | ||
| const href = `${process.env.CLIENT_HOST}/activate/${email}/${token}` |
There was a problem hiding this comment.
The activation link embeds email and token directly into the URL. Use encodeURIComponent(email) and encodeURIComponent(token) when building the path to avoid broken/unsafe URLs for emails or tokens containing special characters.
| return transporter.sendMail({ | ||
| to: email, | ||
| subject: subject, | ||
| html: html, |
There was a problem hiding this comment.
The send wrapper calls transporter.sendMail({ to, subject, html }) without a from field. Some SMTP providers require an explicit from address; either set a default from in the transporter config or include from here (e.g. from process.env.SMTP_FROM) to ensure delivery.
| import { authMiddleware } from '../middlewares/authMiddleware.js'; | ||
|
|
||
| export const userRouter = express.Router() | ||
|
|
There was a problem hiding this comment.
You pass process.env.SMTP_PORT directly to createTransport. Environment variables are strings — parse it to a number (e.g. port: Number(process.env.SMTP_PORT)) and consider setting secure based on the port (usually secure: true for 465).
| export const client = new Sequelize({ | ||
| host: process.env.DB_HOST, | ||
| username: process.env.DB_USER, | ||
| password: process.env.DB_PASSWORD, | ||
| database: process.env.DB_DATABASE, | ||
| dialect: 'postgres', |
There was a problem hiding this comment.
You're not specifying a DB port. If your Postgres instance doesn't use the default port, add port: Number(process.env.DB_PORT) (coerce to number) to the options so connections work for non-default ports.
| host: process.env.DB_HOST, | ||
| username: process.env.DB_USER, | ||
| password: process.env.DB_PASSWORD, | ||
| database: process.env.DB_DATABASE, |
There was a problem hiding this comment.
The code uses environment variables directly without validation. If any of DB_HOST, DB_USER, DB_PASSWORD, or DB_DATABASE is undefined the Sequelize constructor may fail at runtime; consider validating required env vars and throwing a clear error during startup so problems are detected early.
| export const client = new Sequelize({ | ||
| host: process.env.DB_HOST, | ||
| username: process.env.DB_USER, | ||
| password: process.env.DB_PASSWORD, | ||
| database: process.env.DB_DATABASE, | ||
| dialect: 'postgres', |
There was a problem hiding this comment.
For production deployments you may need extra options (SSL/dialectOptions, pool settings, or logging control). Consider making dialectOptions and logging configurable via env vars to support secure/managed DB instances.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed in several critical areas before this implementation can be considered complete and compliant with the task requirements.
Key blockers:
- Registration and user typing are inconsistent with the spec:
authService.registerand the registration-related pages send onlyemailandpassword(seeclient/src/services/authService.tsandclient/src/pages/RegistrationPage.tsx), while the server expectsname,email, andpasswordand theUsertype inclient/src/types/user.tsis missingname[CHECKLIST ITEM #1, #30]; update the form, service, andUserinterface to includename. - The activation flow is broken by a client/server shape mismatch:
authService.activateandAuthContext.activateexpect{ accessToken, user }, butserver/src/controllers/user.controller.jscurrently returns only the user object, so you must either change the server to return{ accessToken, user }on activation or change the client to handle aUser-only response and obtain tokens viarefresh/login[CHECKLIST ITEM #7, #8, #31]. - Login and activation do not enforce or surface activation correctly: the login controller in
server/src/controllers/user.controller.jsdoes not block users whoseactivationTokenis still set, and the login/activation pages (client/src/pages/LoginPage.tsx,client/src/pages/AccountActivationPage.tsx) do not detect an inactive-user error and prompt the user to activate [CHECKLIST ITEM #11]; add a server-side check for activation and client-side handling that guides the user to the activation flow. - Several required client features are missing: there is no Profile page or UI for changing name/email/password, and the password-reset request/reset/confirmation pages and redirects are not implemented [CHECKLIST ITEM #15–#24]; you need to add these routes/components and protect the Profile route with
RequireAuthso that profile and reset flows work end-to-end. - The Axios interceptors in
client/src/http/httpClient.ts(and similar logic elsewhere) are unsafe: they write torequest.headers.Authorizationwithout ensuringrequest.headersexists and retry 401 responses without a_retryguard or try/catch aroundauthService.refresh, and they do not reliably attach the refreshed token to the retried request [CHECKLIST ITEM #32–#34]; initializeheaderswhen missing, add an_retryflag on the original request, wraprefreshin try/catch, clear tokens on failure, and set the newAuthorizationheader before retrying. - Critical auth-flow redirects and flags are incorrect:
isCheckedinAuthContextstarts astrueso some pages render before auth is actually checked, authenticated users are redirected to'/'instead of'/profile'on login/activation, and logout navigates to'/'instead of'/login'[CHECKLIST ITEM #8, #10, #12, #14]; initializeisCheckedtofalse, show a loader until auth is checked, and adjust redirects to use'/profile'after auth and'/login'after logout. - On the server,
updatePasswordandupdateEmailinserver/src/controllers/user.controller.jsinvert their password checks and do not validate confirmation fields as required (old/new/confirmation for password, new email + confirmation for email) [CHECKLIST ITEM #23, #24]; fix the conditions to throw when the provided password is invalid and enforce equality of new and confirmation fields before saving. - Finally, CI cannot run because there is no
package.jsonat the repository root, causingnpm installto fail [CHECKLIST ITEM #35]; restore or add the rootpackage.json(or adjust the workflow path) so dependencies can be installed and tests can execute.
Once you address these functional and flow-critical issues, your solid base of models, controllers, and guards will support a fully working auth system, and the remaining minor improvements (like SMTP port casting and adding a from address in the email service) can be polished afterward.
✨ 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
|
|
||
| export const AuthProvider = ({ children }: { children: React.ReactNode }) => { | ||
| const [currentUser, setCurrentUser] = useState<User | null>(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked is initialized to true. This means the app will not show the Loader and will behave as if the auth check is already complete. Initialize isChecked to false so the app waits for authService.refresh()/checkAuth() before rendering protected content.
| const [isChecked, setChecked] = useState(true); | ||
|
|
||
| async function activate(email: string, token: string) { | ||
| const { accessToken, user } = await authService.activate(email, token); |
There was a problem hiding this comment.
This code destructures the result of authService.activate as { accessToken, user }. The client and server must agree on the response shape (see checklist item #31). If the server returns only the user (or a different shape), this will throw. Make sure authService.activate and the server endpoint return the same structure (or handle both cases).
| setCurrentUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); | ||
| } finally { |
There was a problem hiding this comment.
After logout the app navigates to '/'. Per the task requirements the app should redirect to the login page after logout — change this to navigate('/login').
| import { Routes, Route, Link, useNavigate, NavLink } from 'react-router-dom'; | ||
|
|
||
| import '@fortawesome/fontawesome-free/css/all.min.css'; | ||
| import 'bulma/css/bulma.css'; |
There was a problem hiding this comment.
The props type references React.ReactNode but this file does not import React or the React types. This will produce a TypeScript error. Import React (import React from 'react') or import the type (import type { ReactNode } from 'react') and use children?: ReactNode instead.
| }, [error]); | ||
|
|
||
| return [error, setError] as const; | ||
| }; |
There was a problem hiding this comment.
You assign request.headers.Authorization directly but request.headers may be undefined. Ensure request.headers exists before setting Authorization (e.g. request.headers = request.headers || {}; request.headers.Authorization = ...) to avoid runtime/TypeScript errors (checklist item #34).
| const timerId = setTimeout(() => setError(''), 3000); | ||
|
|
||
| return () => clearTimeout(timerId); | ||
| }, [error]); |
There was a problem hiding this comment.
Minor: you read the access token from localStorage.getItem('accessToken') instead of using accessTokenService.get(). Using the service keeps token access centralized and consistent (optional improvement).
| // to awoid getting `res.data` everywhere | ||
| authClient.interceptors.response.use( | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| res => res.data, |
There was a problem hiding this comment.
You're reading the token directly from localStorage here. Prefer using the shared service (accessTokenService.get()) so token access is consistent and easier to change/mocking in tests.
| } | ||
|
|
||
| if (currentUser) { | ||
| return <Navigate to="/" replace />; | ||
| } |
There was a problem hiding this comment.
You assign request.headers.Authorization directly but request.headers can be undefined. Ensure request.headers exists before setting Authorization (e.g. request.headers = request.headers || {};) or use request.headers = { ...(request.headers || {}), Authorization: ... } to avoid runtime/TypeScript errors (checklist item #34).
| httpClient.interceptors.request.use(request => { | ||
| const accessToken = localStorage.getItem('accessToken'); | ||
|
|
||
| if (accessToken) { | ||
| request.headers.Authorization = `Bearer ${accessToken}`; |
There was a problem hiding this comment.
Do not assign request.headers.Authorization directly — request.headers can be undefined in some Axios configs and that causes runtime/TS errors. Ensure headers exist before assignment (for example: request.headers = request.headers || {}; request.headers.Authorization = ...). This addresses checklist item #34.
| const originalRequest = error.config; | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest!); |
There was a problem hiding this comment.
The refresh + retry logic lacks a retry guard and error handling. Add a guard on the original request (e.g. if ((originalRequest as any)._retry) throw error; (originalRequest as any)._retry = true) to prevent infinite loops, and wrap authService.refresh() in try/catch. If refresh fails, remove stored tokens and rethrow or return a rejected promise so the app can handle unauthorized flows. This fixes the infinite retry and unhandled error issues referenced in checklist items #32 and #33.
| }, []); | ||
|
|
There was a problem hiding this comment.
| <p className="notification is-success is-light"> | ||
| Your account is now active | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
After successful login you navigate to state.from?.pathname ?? '/'. The app must redirect to the Profile page after login per the spec. Use '/profile' (or prefer state.from when it exists but fall back to '/profile') to meet checklist item #12.
| ); | ||
| }; |
There was a problem hiding this comment.
The catch block sets a generic error message but does not specifically detect an unactivated user and prompt them to activate their email. Per checklist item #11 you should detect the server response for inactive users (status or message) and either show a clear instruction to activate or navigate to the activation request/activation page with guidance.
| if (isChecked && currentUser) { | ||
| return <Navigate to="/" />; |
There was a problem hiding this comment.
When the user is already authenticated the component redirects to /. Per the task requirements the app should redirect authenticated users to the Profile page after login/activation. Change this to redirect to the profile route (for example '/profile') so it matches checklist item #12.
| .then(() => { | ||
| const state = location.state as { from?: Location }; | ||
| navigate(state.from?.pathname ?? '/'); |
There was a problem hiding this comment.
After successful login you navigate to state.from?.pathname ?? '/'. The default fallback should be the Profile route ('/profile') instead of /, to satisfy the requirement "Redirect to profile after login" (checklist item #12).
| .catch((error: AxiosError<{ message?: string }>) => { | ||
| setError(error.response?.data?.message ?? ''); |
There was a problem hiding this comment.
The catch handler simply shows a generic error. The spec requires that if a user is not active when attempting to log in the system must ask them to activate their email. Detect the specific server response for inactive users (e.g. a specific message or status) and show activation instructions or navigate to the activation page instead of only showing a generic error (checklist item #11).
| const { login, isChecked, currentUser } = useAuth(); | ||
|
|
||
| if (isChecked && currentUser) { | ||
| return <Navigate to="/" />; | ||
| } |
There was a problem hiding this comment.
This page renders the form while isChecked may still be false. That can cause a flash or allow the form to appear before auth state is confirmed. Add a guard to show a Loader while auth status is being checked (for example if (!isChecked) return <Loader />) so the page is available only to non-authenticated users after the check completes (checklist item #10).
| interface AuthData { | ||
| accessToken: string; | ||
| user: User; |
There was a problem hiding this comment.
AuthData declares { accessToken, user } but the server's /activate endpoint currently returns only the user (no accessToken). activate() is typed to return AuthData which will cause runtime/type errors when the server doesn't provide accessToken. Align client/server: either make the server return { accessToken, user } on activation or change activate() to return User and obtain tokens with a subsequent call (for example refresh() or login). See checklist item #31 (activation response shape) and checklist items #7/#8.
| register: (email: string, password: string) => { | ||
| return client.post('/registration', { email, password }); |
There was a problem hiding this comment.
The register method only accepts email and password and sends { email, password }. The task requires registration using name, email, and password. Update the method signature to register(name: string, email: string, password: string) and POST { name, email, password }. Also update the registration form and client types to include name so profile features can work (checklist item #1 and previous-review item #30).
| activate: (email: string, token: string): Promise<AuthData> => { | ||
| return client.get(`/activate/${email}/${token}`); |
There was a problem hiding this comment.
activate() is declared to return Promise<AuthData> but calls client.get('/activate/...') which (given the current server) returns only the user. This mismatch (declaration vs actual response) will break AuthContext.activate() which expects { accessToken, user }. Either change this method's return type to Promise<User> and update the activation flow in the AuthContext, or change the server to return tokens upon activation as noted above (checklist items #7 and #8).
| const { logout } = useAuth(); | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
Registration currently sends only { email, password } but the server register controller reads name from req.body and the task requires registering with name, email and password. Update this function to accept and send name (e.g. register(name, email, password)), so the client request matches the server and checklist item #1 is satisfied.
| import { User } from '../types/user'; | ||
| import { AxiosError } from 'axios'; | ||
| import { useLocation, useNavigate } from 'react-router-dom'; | ||
| import { useAuth } from '../components/AuthContext'; | ||
|
|
||
| export const UsersPage = () => { | ||
| const { logout } = useAuth(); | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const [error, setError] = usePageError(''); | ||
| const [users, setUsers] = useState<User[]>([]); | ||
|
|
There was a problem hiding this comment.
The AuthData interface declares { accessToken, user }, but the server activate endpoint returns only the user (no accessToken). activate is typed to return Promise<AuthData> which will lead to runtime/destructuring errors in the AuthContext. Either change the server to return { accessToken, user } on activation or update the client to expect a User (and handle obtaining tokens via refresh/login). Align client/server shapes as required by checklist item #31.
| const [users, setUsers] = useState<User[]>([]); | ||
|
|
There was a problem hiding this comment.
This activate method simply forwards the GET and is typed as returning AuthData. Given the server returns only the user on activation, update this method's return type (or its implementation) so callers don't assume an accessToken exists. Also ensure the activation flow redirects to Profile after activation as required by the spec once the shapes are aligned (checklist items #7 and #8).
| get: () => localStorage.getItem(key), | ||
| save: (token: string) => localStorage.setItem(key, token), | ||
| remove: () => localStorage.removeItem(key), |
There was a problem hiding this comment.
The AuthData interface assumes responses include { accessToken, user }. Make sure this matches the server responses for all endpoints (login/refresh/activate). Currently server's activate endpoint returns only the user (mismatch noted in checklist item #31). Either adjust this type or change the server to return { accessToken, user } on activation so the client can store tokens reliably.
|
|
||
| type RegistrationError = AxiosError<{ |
There was a problem hiding this comment.
The register method accepts only email and password. The task requires registration with name, email and password (checklist item #1 and previous-review requirement #30). Modify this method to accept a name parameter and send { name, email, password } to the server so the backend can create a user with a name.
| }>; | ||
|
|
There was a problem hiding this comment.
The activate method is typed to return AuthData ({ accessToken, user }) but the server's activate endpoint currently returns only the user object. This mismatch will break AuthContext.activate which expects an accessToken to save. Either update the server to return tokens on activation or change the client to handle the server shape (for example: accept user only, then call refresh or login to obtain tokens). See checklist item #31.
| import cn from 'classnames'; | ||
|
|
||
| import { authService } from '../services/authService'; |
There was a problem hiding this comment.
The AuthData interface references user: User. The client-side User type must include name so profile features (change name, show profile) work correctly (previous-review high priority, checklist item #30). Ensure the shared User type on the client matches the server (id, name, email) to avoid type/usage bugs.
| export interface User { | ||
| id: number; | ||
| email: string; |
There was a problem hiding this comment.
The User interface is missing the name field required by the task. Update the interface to include name: string so client types align with the server and the app can implement profile features (registration with name, showing/changing name).
| @charset "utf-8"; | ||
|
|
||
| $navbar-breakpoint: 480px !default; |
There was a problem hiding this comment.
The User interface is missing the required name property. Add name: string; (so the interface becomes { id: number; name: string; email: string; }) — the task requires registering with name and profile features depend on this (checklist items #1, #21, #22 and previous-review item #30). Also verify and update any code that consumes User (forms, pages, auth flows) to use the new field.
| res.send(userService.normalize(user)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The activation controller reads activationToken from params but the route includes :email/:activationToken. The controller ignores the email param — either use it for extra validation or update the route to only include the token. Also make sure client and server agree on the expected URL shape.
|
|
||
| if (!user) { | ||
| res.sendStatus(404); | ||
| return; |
There was a problem hiding this comment.
This endpoint returns the user object only (res.send(user)) after activation. The client authService.activate/AuthContext currently expect an { accessToken, user } shape. Align server/client: either return { accessToken, user } here or update the client to handle a user response and obtain tokens separately (checklist item #31).
| if (isOldPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); | ||
| } | ||
|
|
||
| user.password = await bcrypt.hash(newPassword, 10); | ||
|
|
||
| await user.save(); |
There was a problem hiding this comment.
The login controller does not check whether the user is activated (e.g. activationToken should be null). Per requirements, users must be activated before they can log in — detect inactive users and return an appropriate error so the client can ask them to activate their email (checklist item #11).
|
|
||
| const isOldPasswordValid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (isOldPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); |
There was a problem hiding this comment.
In updatePassword the old-password validation is inverted: you throw when isOldPasswordValid is true. It should throw when the old password is NOT valid (i.e. if (!isOldPasswordValid) throw ApiError.badRequest('Invalid password')). Fix the condition so users must provide a correct old password to change it (checklist item #23).
| const updatePassword = async (req, res) => { | ||
| const { oldPassword, newPassword } = req.body; |
There was a problem hiding this comment.
The updatePassword endpoint accepts oldPassword and newPassword but does not require or validate a confirmation field for the new password. The task requires old + new + confirmation fields that must match. Add a confirmation parameter and verify newPassword === confirmation before saving (checklist item #23).
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (isPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); |
There was a problem hiding this comment.
In updateEmail the password check is inverted: you throw when isPasswordValid is true. It should throw when the provided password is NOT valid (if (!isPasswordValid) throw ApiError.badRequest('Invalid password')). Fix this condition so email changes require the correct current password (checklist item #24).
| const updateEmail = async (req, res) => { | ||
| const { newEmail, password } = req.body; |
There was a problem hiding this comment.
The updateEmail controller accepts newEmail and password but does not require the user to confirm the new email (no newEmailConfirmation), nor does it validate the confirmation. The spec requires confirming the new email before changing it. Enforce a confirmation field (newEmailConfirmation) matching newEmail or add a confirm step to the flow (checklist item #24).
|
|
||
| if (!user) { | ||
| res.sendStatus(404); | ||
| return; |
There was a problem hiding this comment.
Minor but important: after user.save() in activation you don't return a normalized user (you send raw model). Consider sending the normalized user shape (id, name, email) like you do in other endpoints so the client receives consistent data and the User type matches (helps checklist items #7/#8 and client type alignment).
|
|
||
| export const User = client.define('user', { | ||
| name: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
SMTP_PORT is read from environment as-is. Nodemailer expects a numeric port; consider casting the value to a number (e.g. port: Number(process.env.SMTP_PORT)) and optionally configuring secure/TLS settings depending on the port to avoid runtime/connection issues.
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, |
There was a problem hiding this comment.
The send wrapper calls transporter.sendMail(...) but does not set a from address. Many SMTP providers require a from field or will replace it; include a from (e.g. from: process.env.SMTP_FROM || 'no-reply@example.com') to ensure consistent delivery and clearer sender identity.
| resetToken: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
When building the activation link you interpolate the raw email into the URL. To avoid problems with special characters (e.g. +, @) encode the email with encodeURIComponent(email) when placing it in the path (/activate/${encodeURIComponent(email)}/${token}) so links remain valid for all emails.
| subject: subject, | ||
| html: html, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Auth check flag is initialized to true which skips the initial auth-refresh check. Set isChecked to false so the app waits for checkAuth() (otherwise guarded routes and non-auth pages can behave incorrectly).
| }, | ||
| }); |
There was a problem hiding this comment.
| const transporter = nodemailer.createTransport({ | ||
| host: process.env.SMTP_HOST, | ||
| port: process.env.SMTP_PORT, | ||
| auth: { | ||
| user: process.env.SMTP_USER, | ||
| pass: process.env.SMTP_PASSWORD, | ||
| }, | ||
| }); | ||
|
|
||
| export function send({ email, subject, html }) { | ||
| return transporter.sendMail({ | ||
| to: email, |
There was a problem hiding this comment.
AuthData and activate() are typed to return { accessToken, user } but the server's activation handler currently returns only the user. Align client/server: either make server return { accessToken, user } on activation or change client activate() and AuthContext.activate to handle a User and then obtain tokens (e.g. call refresh() or require login). Right now this mismatch will throw at runtime in AuthContext.activate.
| import 'dotenv/config' | ||
| import nodemailer from 'nodemailer'; | ||
|
|
|
|
||
| function sendEmailChange(email) { | ||
| const href = `${process.env.CLIENT_HOST}/login` | ||
|
|
||
| const html = ` | ||
| <h1>Your email has changed</h1> | ||
| <a href="${href}" target="_blank">${href}</a> | ||
| `; | ||
|
|
||
| return send({ | ||
| email, |
There was a problem hiding this comment.
Registration form does not collect name (initial values and submit call only include email/password) yet the server expects a name. Add a name field to the form and pass it to authService.register(name, email, password) so activation email and profile name work end-to-end.
| <a href="${href}" target="_blank">${href}</a> | ||
| `; | ||
|
|
||
| return send({ | ||
| email, | ||
| subject: 'Activate', | ||
| html, | ||
| }) | ||
| }; | ||
|
|
||
| function sendPasswordResetEmail(email, token) { | ||
| const href = `${process.env.CLIENT_HOST}/reset-password/${token}` | ||
|
|
||
| const html = ` | ||
| <h1>Reset password</h1> | ||
| <a href="${href}" target="_blank">${href}</a> | ||
| `; | ||
|
|
||
| return send({ |
There was a problem hiding this comment.
Login page redirects already-authenticated users to / and falls back to / after login. The spec requires redirecting to the Profile page after login/activation. Change these redirects/fallbacks to /profile (or to the saved from when present, falling back to /profile) and ensure the page waits for isChecked before rendering (show a Loader while checking auth).
| subject: 'Reset', | ||
| html, |
There was a problem hiding this comment.
The login error handler shows generic backend messages but does not detect the "user not active" case and prompt activation. Ensure the server returns a distinguishable error for inactive users and add client logic to detect it and guide the user to activate their email (checklist item #11).
| }) | ||
| }; | ||
|
|
||
| function sendPasswordResetEmail(email, token) { |
There was a problem hiding this comment.
App's logout handler currently navigates to '/' after logout. Per requirements the app must redirect to the login page after logging out — change navigate('/') to navigate('/login').
|
|
||
| const html = ` | ||
| <h1>Activate account</h1> | ||
| <a href="${href}" target="_blank">${href}</a> | ||
| `; |
There was a problem hiding this comment.
Account activation flow calls activate(...) but does not navigate to the Profile page after a successful activation. After activation complete (and after ensuring tokens are set or refresh called) navigate to /profile to satisfy the requirement "redirect to Profile after the activation" (checklist item #8).
| }); | ||
|
|
||
| export function send({ email, subject, html }) { | ||
| return transporter.sendMail({ | ||
| to: email, |
There was a problem hiding this comment.
httpClient request interceptor assigns request.headers.Authorization directly — request.headers may be undefined and this can cause runtime/TypeScript errors. Ensure request.headers exists before assignment (e.g. request.headers = request.headers || {};) or build a new headers object (checklist item #34).
| <a href="${href}" target="_blank">${href}</a> | ||
| `; | ||
|
|
||
| return send({ | ||
| email, | ||
| subject: 'Activate', | ||
| html, | ||
| }) | ||
| }; | ||
|
|
||
| function sendPasswordResetEmail(email, token) { |
There was a problem hiding this comment.
The response error interceptor retries after a 401 by calling authService.refresh() without a retry guard and without try/catch. Add a _retry flag on the original request to avoid infinite refresh loops, and wrap authService.refresh() in try/catch so an invalid refresh token does not create unhandled errors. On refresh failure clear tokens and redirect to login (checklist items #32 and #33).
| subject: 'Activate', | ||
| html, | ||
| }) | ||
| }; | ||
|
|
||
| function sendPasswordResetEmail(email, token) { |
There was a problem hiding this comment.
After obtaining a new access token you save it but you do not attach it to the retried originalRequest. Ensure originalRequest.headers exists and set Authorization: 'Bearer ' + accessToken before retrying so the retried request uses the refreshed token.
| const href = `${process.env.CLIENT_HOST}/login` | ||
|
|
||
| const html = ` | ||
| <h1>Your email has changed</h1> |
There was a problem hiding this comment.
Server activation handler returns the raw user model (res.send(user)) after activation. The client currently expects tokens on activation (see authService/AuthContext). Return a normalized user and/or tokens consistently from activation, or update client activation handling. Also consider returning the normalized shape { user, accessToken } if you want to auto-login on activation (checklist items #7/#8 and #31).
| `; | ||
|
|
||
| return send({ | ||
| email, | ||
| subject: 'Email Change', | ||
| html, | ||
| }) | ||
| } | ||
|
|
||
| export const emailService = { | ||
| send, |
There was a problem hiding this comment.
In updatePassword the old-password validation is inverted: you throw when isOldPasswordValid is true. It should throw when the old password is NOT valid (if (!isOldPasswordValid) throw ApiError.badRequest('Invalid password')). Also add a confirmation field and validate newPassword === confirmation per the spec (checklist item #23).
| }); | ||
|
|
||
| export function send({ email, subject, html }) { | ||
| return transporter.sendMail({ |
There was a problem hiding this comment.
| import { catchError } from '../utils/catchError.js'; | ||
|
|
||
| export const authRouter = express.Router() | ||
|
|
There was a problem hiding this comment.
You pass process.env.SMTP_PORT directly to nodemailer. Environment variables are strings — convert to a number (e.g. port: Number(process.env.SMTP_PORT) or parseInt(...)) to avoid surprises or type issues with some transports.
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 so you can keep progressing, but there are still several important auth and infrastructure issues to fix. You did a good job wiring up the main pages (registration, login, profile, password reset), including name in registration, and your route guards plus Loader component are conceptually on the right track. On the client, AuthContext and related pages start with isChecked set to true, the activation flow expects { accessToken, user } while the server currently returns only a user, and the Axios HTTP client’s interceptors do not initialize request.headers, lack an originalRequest._retry flag, and call authService.refresh() without robust error handling, which together break reliable authentication and token refresh [CHECKLIST ITEM #8, #31–#34]. On the server, the login endpoint does not block inactive users, the password‑reset request forgets to await the findByEmail call, and the profile updatePassword/updateEmail handlers invert the password checks and skip confirmation validation, so they can reject correct passwords or accept unconfirmed changes [CHECKLIST ITEM #11, #23, #24]. Your CI is currently failing because there is no package.json at the repository root for npm install, so restoring or recreating it with the expected scripts is necessary for automated tests to run [CHECKLIST ITEM #35]. As refinements, update redirects to send authenticated users to /profile (e.g., in RequireNonAuth, login, registration, activation, and reset pages), add non‑auth guards where required, clean up small typos in console messages, and align the activation route parameters with what the controller actually uses. Overall, you have a solid structural foundation; addressing these correctness and robustness issues will be a great learning step and will bring your auth flow much closer to production-ready quality.
✨ 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
| import { ProfilePage } from './pages/ProfilePage'; | ||
| import { ForgotPasswordPage } from './pages/ForgotPasswordPage'; | ||
| import { ResetPasswordConfirmPage } from './pages/ResetPasswordConfirmPage'; | ||
| import { AxiosError } from 'axios'; |
There was a problem hiding this comment.
High severity: isChecked is initialized to true, which allows the app to render pages before the auth check finishes. Per the requirements you should initialize isChecked to false and show a loader until checkAuth() completes so unauthenticated state is not rendered prematurely (see checklist item about isChecked behavior).
| export function App() { | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
High severity: activate destructures { accessToken, user } from authService.activate. The server currently returns only a user object on activation (not { accessToken, user }), so this will throw or produce undefined tokens. Either change the server to return { accessToken, user } on activation or update the client to accept a user-only response and obtain tokens via refresh/login (checklist items #7/#8/#31).
| return <Loader />; | ||
| } | ||
|
|
||
| const handleLogout = () => { |
There was a problem hiding this comment.
Minor: the console.log message in checkAuth() has a typo ('authentincated'). Not functionally critical, but consider fixing the message for clarity and debugging.
| httpClient.interceptors.request.use(request => { | ||
| const accessToken = localStorage.getItem('accessToken'); | ||
|
|
||
| if (accessToken) { | ||
| request.headers.Authorization = `Bearer ${accessToken}`; | ||
| } |
There was a problem hiding this comment.
The request interceptor writes to request.headers.Authorization but doesn't ensure request.headers exists. Initialize request.headers when missing (e.g. request.headers = request.headers || {}) before setting Authorization. Also consider using accessTokenService.get() instead of reading localStorage directly for consistency. (See checklist item #32.)
| async (error: AxiosError) => { | ||
| if (error.response?.status !== 401) { | ||
| throw error; | ||
| } | ||
|
|
||
| const originalRequest = error.config; | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest!); |
There was a problem hiding this comment.
The response interceptor retries every 401 without a _retry guard on the original request. Add a flag like if (originalRequest._retry) throw error; originalRequest._retry = true; to prevent infinite loops when refresh fails. (See checklist item #33.)
| const originalRequest = error.config; | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest!); |
There was a problem hiding this comment.
Calling authService.refresh() must be wrapped in try/catch. If refresh fails, clear stored tokens (e.g. accessTokenService.remove()) and rethrow or redirect to login instead of retrying. If refresh succeeds, set originalRequest.headers.Authorization = 'Bearer ' + accessToken before retrying so the retried request includes the new token. (See checklist item #34.)
| } | ||
|
|
||
| if (currentUser) { | ||
| return <Navigate to="/" replace />; | ||
| } |
There was a problem hiding this comment.
The request interceptor writes to request.headers.Authorization but doesn't ensure request.headers exists. Initialize request.headers = request.headers ?? {} (or a typed equivalent) before assigning the Authorization header to avoid runtime errors. (See checklist item #32.)
| if (!activationToken || !email) { | ||
| setError('Wrong activation link'); | ||
| setDone(true); | ||
|
|
There was a problem hiding this comment.
isChecked is initialized to true, which lets the app render before the auth check completes. Initialize isChecked to false so the app shows the Loader until checkAuth() finishes (prevents premature rendering of protected/unprotected routes).
|
|
||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical mismatch: activate expects { accessToken, user } from authService.activate and saves accessToken. The server activation endpoint currently returns only a user object. Either change the server to return { accessToken, user } on activation or update the client to handle a user-only response and obtain tokens via refresh/login.
| } | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
Minor: typo in console message User is not authentincated. Fix the message for clarity (not functionally blocking).
|
|
||
| const { activate } = useAuth(); | ||
| const { activationToken = '', email = '' } = useParams(); | ||
|
|
||
| useEffect(() => { | ||
| if (!activationToken || !email) { |
There was a problem hiding this comment.
Request interceptor writes to request.headers.Authorization but doesn't ensure request.headers exists. Initialize or merge headers before setting Authorization (e.g. request.headers = request.headers || {}) to avoid runtime errors. Also consider using accessTokenService.get() instead of reading localStorage directly for consistency.
| }, []); | ||
|
|
||
| if (!done) { | ||
| return <Loader />; |
There was a problem hiding this comment.
Response interceptor retries 401s unconditionally. Add a _retry flag on the original request (check originalRequest._retry and set it before attempting refresh) to avoid infinite retry loops when refresh fails.
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Calling authService.refresh() without try/catch can throw and break the interceptor. Wrap the refresh call in try/catch; on failure clear stored tokens (accessTokenService.remove()), and rethrow or return a rejected promise so the app can handle unauthorized state.
| return ( | ||
| <> | ||
| <h1 className="title">Account activation</h1> |
There was a problem hiding this comment.
After obtaining a refreshed accessToken, attach it to originalRequest.headers.Authorization (ensure originalRequest.headers exists) before retrying the request so the retried call uses the new token.
|
|
||
| const { activate } = useAuth(); | ||
| const { activationToken = '', email = '' } = useParams(); | ||
|
|
There was a problem hiding this comment.
RequireNonAuth redirects authenticated users to /. Per the task spec, authenticated users should be taken to the Profile page after auth — redirect to /profile instead of /.
|
|
||
| const { activate } = useAuth(); |
There was a problem hiding this comment.
ForgotPasswordPage redirects authenticated users to /. Authenticated users should be redirected to /profile (their profile) rather than the home page.
| activate(email, activationToken) | ||
| .catch((error: AxiosError<{ message?: string }>) => { | ||
| setError(error.response?.data?.message ?? `Wrong activation link`); | ||
| }) | ||
| .finally(() => setDone(true)); |
There was a problem hiding this comment.
AccountActivationPage calls activate(...) and shows a success message but does not redirect the user to the Profile page after activation. Per the requirements, redirect to /profile on successful activation (use useNavigate() and navigate when activation succeeds).
| import { Link, Navigate } from 'react-router-dom'; | ||
| import { Formik, Form, Field } from 'formik'; | ||
| import cn from 'classnames'; | ||
|
|
There was a problem hiding this comment.
This file imports authService but does not import useAuth. To enforce the "only non authenticated" requirement you should import useAuth here so you can check isChecked/currentUser and redirect authenticated users.
| message: string; | ||
| }>; |
There was a problem hiding this comment.
Missing non-auth guard: add a check (similar to ForgotPasswordPage) that redirects authenticated users to /profile (after auth is checked). Right now any logged-in user can access the reset page; the task explicitly requires password reset pages to be only for non-authenticated users.
| if (!value) return 'Email is required'; | ||
| if (!EMAIL_PATTERN.test(value)) return 'Email is not valid'; | ||
| } | ||
|
|
||
| function validatePassword(value: string) { |
There was a problem hiding this comment.
Missing non-auth guard: this page should be accessible only to non-authenticated users. Import useAuth() and: 1) while isChecked is false show a Loader, and 2) if isChecked is true and currentUser exists, redirect to /profile. Right now the page does neither which violates the "only non authenticated" requirement.
| {successMessage && ( | ||
| <div className="notification is-success is-light"> |
There was a problem hiding this comment.
Redirects authenticated users to / here. Per the task requirements authenticated users should be taken to the Profile page after auth; change this to return <Navigate to="/profile" /> so already-authenticated users land on their profile instead of the home page.
| showSuccess('Name updated successfully!'); | ||
| } catch { |
There was a problem hiding this comment.
After successful login you navigate to the saved from location or default to '/'. The default should be '/profile' (user profile) to match the spec. Replace navigate(state.from?.pathname ?? '/') with navigate(state.from?.pathname ?? '/profile').
| } finally { | ||
| setSubmitting(false); |
There was a problem hiding this comment.
The .catch only displays the server message but does not specially handle the case when the user is not activated. The spec requires prompting/redirecting inactive users to activate their email. Detect the inactive-user response (by message or status) in this catch and guide the user to the activation flow (for example show a link or redirect to /activate/:email/:token flow).
| enableReinitialize | ||
| onSubmit={async (values, { setSubmitting }) => { |
There was a problem hiding this comment.
Registration page redirects already-authenticated users to /. Per the requirements redirect them to /profile instead so logged-in users land on their profile page.
| <Formik | ||
| initialValues={{ newEmail: '', password: '' }} |
There was a problem hiding this comment.
Profile > Change Email: the form only asks for newEmail and password. The task requires confirming the new email (a confirmation field) before sending the update. Add a confirmNewEmail field in initialValues, render an input for it, and validate equality before submitting.
| <div className="field"> | ||
| <label className="label">New Email</label> | ||
| <div className="control"> | ||
| <Field name="newEmail" type="email" className="input" required /> | ||
| </div> | ||
| </div> | ||
| <div className="field"> | ||
| <label className="label">Current Password</label> | ||
| <div className="control"> | ||
| <Field name="password" type="password" className="input" required /> |
There was a problem hiding this comment.
Profile > Change Email UI: the current inputs render only a single New Email field. Add a second field for confirming the new email and client-side validation that newEmail === confirmNewEmail before calling userService.updateEmail (this enforces the checklist requirement to confirm the new email).
|
|
||
| return ( | ||
| <div className="container section"> | ||
| <h1 className="title">Profile Settings</h1> | ||
|
|
There was a problem hiding this comment.
Account activation currently only shows a success message after activation. The spec requires redirecting to the Profile page after successful activation. Use useNavigate() and navigate to /profile on success (but be mindful of the activation/token shape mismatch — ensure tokens are obtained before redirecting so the Profile route is accessible).
| const activate = async (req, res) => { | ||
| const { activationToken } = req.params; | ||
| const user = await User.findOne({ where: { activationToken }}) | ||
|
|
||
| if (!user) { | ||
| res.sendStatus(404); | ||
|
|
||
| return; | ||
| } | ||
| user.activationToken = null; | ||
| user.save(); | ||
|
|
||
| return res.send(user); |
There was a problem hiding this comment.
Activation handler returns only the user object. The client (and auth flows) expect activation to produce an authenticated session (access token + user) or the client should immediately request refresh/login after activation. Either call generateTokens(res, user) here (so the response includes { accessToken, user } and sets the refresh cookie) or ensure the client obtains tokens via refresh after activation. Without that the client-side activate will not receive tokens as expected.
| const user = await userService.findByEmail(email); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.badRequest('No such user') | ||
| } | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password) | ||
|
|
||
| if (!isPasswordValid) { | ||
| throw ApiError.badRequest('Wrong password') | ||
| } |
There was a problem hiding this comment.
Login does not check whether the user is activated. Per the requirements users with a non-null activationToken should be blocked from logging in and instructed to activate their email. Add a check after you find the user (e.g. if (user.activationToken) throw ApiError.badRequest('User is not activated')) so inactive accounts cannot log in.
| const requestResetPassword = async (req, res) => { | ||
| const { email } = req.body; | ||
|
|
||
| const emailExist = userService.findByEmail(email); | ||
|
|
||
| if (!emailExist) { | ||
| throw ApiError.badRequest('No such email registered'); | ||
| } | ||
|
|
||
| await userService.createResetToken(email); | ||
| res.sendStatus(200); |
There was a problem hiding this comment.
requestResetPassword incorrectly calls userService.findByEmail(email) without awaiting it. findByEmail returns a Promise, so emailExist will always be truthy (a Promise) and the existence check is wrong. await the call. Also, to avoid leaking whether an email is registered (the client shows a generic 'check your email' message regardless), consider not throwing when the email is missing and always respond with 200 — or at least keep server behavior aligned with the client's UX.
| res.cookie('refreshToken', refreshToken, { | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| HttpOnly: true, | ||
| }) |
There was a problem hiding this comment.
In generateTokens the cookie options use HttpOnly: true (capital H). The Express/cookie API expects httpOnly (lowercase). Use httpOnly: true so the refresh token cookie is correctly flagged as HttpOnly.
| const activate = async (req, res) => { | ||
| const { activationToken } = req.params; | ||
| const user = await User.findOne({ where: { activationToken }}) |
There was a problem hiding this comment.
Minor: the activate route path (in router) includes :email/:activationToken but this handler only reads activationToken from req.params and ignores the email param. Validate or use the email param if your activation scheme requires it, or update the route signature to avoid confusion.
| const authorization = req.headers['authorization'] || ''; | ||
| const [,token] = authorization.split(' '); |
There was a problem hiding this comment.
Token extraction is fragile: splitting the header by a single space can produce unexpected results if spacing differs. Consider a more robust approach such as if (!authorization.startsWith('Bearer ')) return 401; const token = authorization.slice(7).trim(); or use a regex to extract the token.
| if (!authorization || !token) { | ||
| res.sendStatus(401); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Currently the middleware responds with res.sendStatus(401) directly. For consistent API error formatting (the app uses ApiError + errorMiddleware elsewhere), consider return next(ApiError.unauthorized()) so clients receive the same JSON error shape.
| if (!userData) { | ||
| res.sendStatus(401); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Same as above: when verification fails you call res.sendStatus(401). Prefer forwarding a standardized error (next(ApiError.unauthorized())) to use the centralized error middleware and keep responses consistent.
|
|
||
| const isOldPasswordValid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (isOldPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); |
There was a problem hiding this comment.
Critical: this condition is inverted. isOldPasswordValid is true when the provided old password matches the stored password — you should allow the update to proceed in that case. Instead the code throws when the old password is valid. Change the logic to throw when the old password is NOT valid (e.g. if (!isOldPasswordValid) throw ApiError.badRequest('Invalid password')). Also add validation to require newPassword and confirmation and verify newPassword === confirmation before saving. This matches the checklist requirement to require old/new/confirmation and validate them.
| const updatePassword = async (req, res) => { | ||
| const { oldPassword, newPassword } = req.body; | ||
|
|
||
| const id = req.user.id; | ||
|
|
||
| const user = await User.findByPk(id); | ||
|
|
||
| if (!user) { | ||
| res.sendStatus(404); | ||
| return; | ||
| }; | ||
|
|
||
| const isOldPasswordValid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (isOldPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); | ||
| } | ||
|
|
||
| user.password = await bcrypt.hash(newPassword, 10); | ||
|
|
||
| await user.save(); |
There was a problem hiding this comment.
Missing confirmation/validation: the handler does not check that newPassword and confirmation are provided and equal. Enforce presence of oldPassword, newPassword, and confirmation, validate that newPassword has minimum length (use validatePassword) and that newPassword === confirmation before hashing and saving.
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (isPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); |
There was a problem hiding this comment.
Critical: this password check is inverted. isPasswordValid is true when the provided password matches the stored one; the code currently throws when the password is valid. Change to if (!isPasswordValid) throw ApiError.badRequest('Invalid password'). Also require and validate a confirmNewEmail field and ensure newEmail === confirmNewEmail before changing the email.
| const updateEmail = async (req, res) => { | ||
| const { newEmail, password } = req.body; | ||
|
|
||
| const id = req.user.id; | ||
|
|
||
| const user = await User.findByPk(id); | ||
|
|
||
| if (!user) { | ||
| res.sendStatus(404); | ||
| return; | ||
| }; | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (isPasswordValid) { | ||
| throw ApiError.badRequest('Invalid password'); | ||
| } | ||
|
|
||
| emailService.sendEmailChange(user.email); | ||
|
|
||
| user.email = newEmail; | ||
|
|
||
| await user.save(); | ||
| res.sendStatus(201); |
There was a problem hiding this comment.
Missing email confirmation validation: the endpoint does not validate that the client confirmed the new email (no confirmNewEmail/equality check) and does not validate format for newEmail. Add server-side checks to ensure the new email is provided, equals the confirmation, and is a valid email before persisting and notifying the old email.
| name: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, |
There was a problem hiding this comment.
The activation route includes :email in the path (/activate/:email/:activationToken) but the activation controller only reads activationToken and ignores the email param. Align the route and controller: either validate/use the email param in the controller or remove the :email segment from the route to avoid a misleading API surface.
| res.status(error.status).send({ | ||
| message: error.message, | ||
| errors: error.errors, | ||
| }); |
There was a problem hiding this comment.
Inconsistent route/controller shape: the route defines '/activate/:email/:activationToken' but the activation handler only reads activationToken (the email param is not used). Align route and controller — either remove the unused :email param or validate/use it in the controller so the API shape is consistent with the client/activation link format.
| export const Token = client.define('token', { | ||
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, |
There was a problem hiding this comment.
The activation route includes :email in the path but the controller's activate handler only reads activationToken from req.params. Make the route and controller consistent — either remove the unused :email param here or update the controller to read/validate the email param as part of activation.
3 participants
No description provided.