Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical, blocking issues that violate the assignment’s functional requirements and can break core flows. First, the ESLint configuration is currently broken (CommonJS config loaded as ESM) and CI cannot run tests at all; this must be fixed by using a .cjs config or ESM export so the automated checks can execute. Second, key auth flows do not meet the checklist: after account activation and email-change confirmation the client does not set tokens/user or redirect to /profile, login redirects to / instead of /profile, and logout redirects to / instead of /login, so required navigation behavior is missing. Third, several pages that must be available only to non-authenticated users (registration, activation, login, forgot password, reset password, email confirmation) are not consistently guarded (or force-logout in a brittle way), meaning authenticated users can access non-auth-only routes, which directly contradicts the checklist. Finally, there are backend correctness bugs in auth/token handling (e.g. HttpOnly instead of httpOnly when setting the refresh cookie, missing await on tokenService.getByToken and Token.create) that can cause invalid refresh behavior or race conditions. Please address these blockers, then also consider the smaller suggested improvements (better error handling in profile/password flows, more robust email/DB handling, and minor UX tweaks) to make your solution more robust—your overall structure is solid, and fixing these points will bring it fully in line with 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
client/.eslintrc.cjs
Outdated
| {blankLine: 'always', prev: 'directive', next: '*'}, | ||
| {blankLine: 'always', prev: 'block-like', next: '*'}, | ||
| ], | ||
| 'implicit-arrow-linebreak:': 0, |
There was a problem hiding this comment.
The rule key contains an extra colon character so ESLint will not recognize it. Change 'implicit-arrow-linebreak:' to 'implicit-arrow-linebreak' so the rule actually disables implicit arrow linebreak warnings.
client/.eslintrc.cjs
Outdated
| 'plugin:cypress/recommended', | ||
| ], | ||
| parserOptions: { | ||
| ecmaVersion: "2024", |
There was a problem hiding this comment.
Prefer a numeric ecmaVersion (or use 'latest') instead of a string. Use ecmaVersion: 2024 (no quotes) or ecmaVersion: 'latest' to avoid any parser ambiguity.
| @@ -0,0 +1,5 @@ | |||
| export const Loader = () => ( | |||
There was a problem hiding this comment.
This file uses JSX without importing React. If your project does not use the automatic JSX runtime (React 17+ with the correct Babel/TS/Vite config), this will cause a runtime/build error. Either add import React from 'react' at the top or ensure the automatic JSX runtime is enabled in your build tool (e.g. Babel or Vite).
| } | ||
|
|
||
| if (user) { | ||
| return <Navigate to="/" replace />; |
There was a problem hiding this comment.
When an authenticated user tries to access a non-auth-only page we redirect to /. The checklist requires routing authenticated users to Profile for auth-related flows — change the target to /profile (or make the redirect path configurable) so authenticated users are sent to their Profile page instead of Home.
client/src/http/httpClient.js
Outdated
| httpClient.interceptors.response.use(onResponseSuccess, onResponseError); | ||
|
|
||
| function onRequest(request) { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
Use the accessToken service getter instead of reading localStorage directly for consistency and to centralize storage access. Replace localStorage.getItem('accessToken') with accessTokenService.get() (you already use accessTokenService.save elsewhere). See client AuthContext for how access tokens are saved via the service .
| } | ||
|
|
||
| async function onResponseError(error) { | ||
| const originalRequest = error.config; |
There was a problem hiding this comment.
Guard error.config (originalRequest). If error.config is undefined this will throw later when accessing _retry. Add a defensive check and rethrow if missing (e.g. if (!originalRequest) throw error;).
client/src/http/httpClient.js
Outdated
| async function onResponseError(error) { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response.status !== 401 || originalRequest._retry) { |
There was a problem hiding this comment.
Before reading error.response.status ensure error.response exists. Accessing .status when error.response is undefined (network error, CORS, etc.) will itself throw. Example: if (!error.response || error.response.status !== 401 || originalRequest._retry) { throw error; }
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { accessToken } = await authService.refresh(); |
There was a problem hiding this comment.
You call authService.refresh() to obtain a new access token — good. Make sure you know authService.refresh() uses the separate authClient instance (so it doesn't hit this interceptor recursively). This file relies on that behavior to avoid loops (authService implementation shown) .
| try { | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); |
There was a problem hiding this comment.
When you save the new access token you call accessTokenService.save(accessToken) (good). For full consistency, read the token with accessTokenService.get() in onRequest (see first comment) rather than localStorage.getItem so all token access goes through the same API.
| function onResponseSuccess(res) { | ||
| return res.data; |
There was a problem hiding this comment.
onResponseSuccess returns res.data, which is fine. Be aware if createClient() already applies response transforms elsewhere; duplicating unwrapping is harmless but keep consistent with your other clients (e.g. authClient sets an interceptor that returns res.data) .
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest); | ||
| } catch (error) { |
There was a problem hiding this comment.
Consider returning Promise.reject(error) instead of throw error inside the catch for clarity in interceptor async handlers — both will reject, but Promise.reject is explicit for interceptor flows. Current throw is acceptable in async function, so this is optional.
client/src/http/index.js
Outdated
|
|
||
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', |
There was a problem hiding this comment.
Hardcoded baseURL to 'http://localhost:3005' makes the client fail in other environments. Consider using an environment variable (e.g. VITE_API_URL / import.meta.env.VITE_API_URL or process.env) so the API host can change between dev and production.
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', | ||
| withCredentials: true, |
There was a problem hiding this comment.
Enabling withCredentials: true is appropriate for cookie-based auth (so refresh/activation cookies are sent). Ensure the server's CORS config allows credentials and the client origin (the server entrypoint shows credentials/CORS config must match) — otherwise requests will be blocked.
| import { useParams, Link } from 'react-router-dom'; | ||
| import { userService } from '../services/userService'; | ||
|
|
||
| export const EmailConfirmPage = () => { |
There was a problem hiding this comment.
This component does not enforce or check authentication state. The checklist expects email confirmation flows to be part of the auth-related flows; either wrap this route with RequireNonAuth in the router or read AuthContext here and redirect authenticated users to /profile to prevent inappropriate access.
| const [result, setResult] = useState('Verifying...'); | ||
|
|
||
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) |
There was a problem hiding this comment.
If activationToken is absent or undefined the API call may fail with an unclear message. Consider checking activationToken before calling userService.confirmEmailChange and set an appropriate error if it's missing so users see a helpful message.
| <div className="container has-text-centered mt-6"> | ||
| <div className="box"> | ||
| <h1 className="title">{result}</h1> | ||
| <Link to="/login" className="button is-primary">Go to Login</Link> |
There was a problem hiding this comment.
After successful confirmation you always render a link to /login. If the user is already authenticated it would be better UX to link to /profile (or navigate there). Consider reading user from AuthContext and conditionally render the CTA (Profile when authenticated, Login otherwise) or perform an automatic redirect.
|
|
||
| export const EmailConfirmPage = () => { | ||
| const { activationToken } = useParams(); | ||
| const [result, setResult] = useState('Verifying...'); |
There was a problem hiding this comment.
Currently the effect updates result on success/error but there is no explicit loading state beyond the initial 'Verifying...' text. This is acceptable, but consider showing a proper loader component while the request is in-flight for clearer UX (especially on slow networks).
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { | ||
| setErrors({ oldPassword: backendMsg }); | ||
| } else { | ||
| setErrors({ oldPassword: 'Failed to update password' }); | ||
| } |
There was a problem hiding this comment.
In the password update catch block you use backendMsg but the conditional is incorrect and can throw when backendMsg is falsy. Currently it’s if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) which evaluates the right-hand includes even when backendMsg is undefined. Wrap the OR inside the backendMsg check and use parentheses, e.g. if (backendMsg && (backendMsg.includes('old password') || backendMsg.includes('at least'))) { ... } to avoid runtime errors.
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { | ||
| setErrors({ oldPassword: backendMsg }); | ||
| } else { | ||
| setErrors({ oldPassword: 'Failed to update password' }); | ||
| } |
There was a problem hiding this comment.
When the backend complains about password length (the 'at least' branch) you set the error on oldPassword. That maps the message to the wrong field. For length/at least errors set the error on newPassword (e.g. setErrors({ newPassword: backendMsg })) and only set oldPassword when the backend indicates the old password is incorrect.
| } catch (err) { | ||
| const errorMsg = err.response?.data?.message || 'Failed to update name'; |
There was a problem hiding this comment.
In submitName's catch block you compute errorMsg but never use it or show feedback to the user. Consider calling setNameMsg(errorMsg) or storing the error into a separate state so the user sees why the update failed.
client/src/pages/ProfilePage.jsx
Outdated
| import React, { useContext, useState } from 'react'; | ||
| import { Formik, Form, Field } from 'formik'; | ||
| import { AuthContext } from '../components/AuthContext.jsx'; | ||
| import cn from 'classnames'; |
There was a problem hiding this comment.
You import cn (classnames) but never use it in this file; this will trigger an unused-import lint warning. Remove the import if not needed or use it where appropriate.
| } | ||
| }; | ||
|
|
||
| export const RegistrationPage = () => { |
There was a problem hiding this comment.
Registration pages must be available only to non-authenticated users (checklist requirement). This component doesn't guard itself — make sure the router wraps this route with RequireNonAuth or read AuthContext here and redirect authenticated users to /profile so logged-in users cannot access registration.
| className={cn('button is-success has-text-weight-bold', { | ||
| 'is-loading': isSubmitting, | ||
| })} | ||
| disabled={isSubmitting || errors.email || errors.password} |
There was a problem hiding this comment.
The submit button is disabled based only on errors.email and errors.password. That allows submitting when name is invalid. Include errors.name in the disabled condition (and/or use a combined validity flag or isValid) so the button remains disabled until all fields are valid.
| .then(() => { | ||
| setRegistered(true); |
There was a problem hiding this comment.
You set registered to true and show the “Check your email” message which is correct UI behavior, but the client can't guarantee the activation email was sent. Verify authService.register triggers the backend to send the activation email and handle/report server failures (e.g. show the server message if email delivery fails).
| onSubmit={({ name, email, password }, formikHelpers) => { | ||
| formikHelpers.setSubmitting(true); |
There was a problem hiding this comment.
You call formikHelpers.setSubmitting(true) at the top of onSubmit. When using Formik, if onSubmit returns a Promise you can let Formik manage isSubmitting automatically; if you keep the manual calls that's fine, but it's redundant — consider returning the Promise from authService.register(...) instead and let Formik manage submitting state to simplify the code.
| function requestEmailChange(data) { | ||
| return httpClient.patch('/profile/request-email-change', data); |
There was a problem hiding this comment.
requestEmailChange accepts a generic data object. The checklist requires that changing an email requires typing the password and confirmation of the new email. Ensure that when calling this function you pass both password and the newEmail (or email) fields — e.g. { newEmail, password } — and that server-side starts the confirmation flow (notify old email, send confirmation to new email). Consider documenting the expected shape here for clarity.
| function confirmEmailChange(activationToken) { | ||
| return httpClient.get(`/profile/confirm-email-change/${activationToken}`); |
There was a problem hiding this comment.
confirmEmailChange uses a GET request to /profile/confirm-email-change/${activationToken} which places the activation token in the URL. This works, but be aware that tokens in URLs can be logged in server logs or browser history. If your backend supports it, consider using POST with the token in the body to reduce exposure (optional improvement).
| function resetPassword(activationToken, password) { | ||
| return httpClient.post('/reset-password', { activationToken, password }); |
There was a problem hiding this comment.
resetPassword posts { activationToken, password } to /reset-password. This is fine and matches a typical flow, but for consistency and clarity you might prefer the function to accept a single object parameter (e.g. resetPassword({ activationToken, password })) like updatePassword does. This reduces caller confusion about argument order and makes future extension easier.
| @@ -0,0 +1,17 @@ | |||
| import 'dotenv/config'; | |||
| import { client } from './src/utils/db.js'; | |||
| import { User } from './src/models/User.js'; | |||
There was a problem hiding this comment.
User is imported but not referenced. If the import is intentional to register model definitions with the Sequelize instance, add a short comment explaining that; otherwise remove the unused import to avoid confusion.
| import 'dotenv/config'; | ||
| import { client } from './src/utils/db.js'; | ||
| import { User } from './src/models/User.js'; | ||
| import { Token } from './src/models/Token.js'; |
There was a problem hiding this comment.
Token is imported but not referenced. Same as above — either remove it or add a comment that importing the model registers it with client so tables are created/altered during sync.
|
|
||
| async function init() { | ||
| try { | ||
| await client.sync({ alter: true }); |
There was a problem hiding this comment.
client.sync({ alter: true }) will attempt to alter existing tables to match models rather than fully recreate them. The log below says "Database tables recreated!" which is misleading. If you intend to fully recreate tables use force: true (destructive) or change the message to reflect that tables were synchronized/updated.
| try { | ||
| await client.sync({ alter: true }); | ||
| console.log('Database tables recreated!'); | ||
| process.exit(0); |
There was a problem hiding this comment.
You call process.exit(0) immediately after sync; consider closing the DB connection gracefully first. For Sequelize use await client.close() (or the appropriate API) before exiting to ensure all connections are released. Do the same in the catch block before exiting with an error code.
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, |
There was a problem hiding this comment.
Consider making refreshToken unique to prevent storing duplicate tokens for different users or accidental duplicates. Add unique: true to the column definition so the DB enforces it.
src/models/Token.js
Outdated
| Token.belongsTo(User); | ||
| User.hasOne(Token); |
There was a problem hiding this comment.
Be explicit about the association options. Specify the foreign key and cascade behavior to ensure a Token is removed when its User is deleted, for example: Token.belongsTo(User, { foreignKey: 'userId', onDelete: 'CASCADE' }) and User.hasOne(Token, { foreignKey: 'userId' }). This avoids orphaned tokens if a user is removed.
src/services/jwt.service.js
Outdated
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '5s', |
There was a problem hiding this comment.
expiresIn: '5s' is extremely short and will cause frequent refresh requests and poor UX. Use a more realistic access token lifetime (e.g. '15m', '1h') or read the value from an env var like process.env.JWT_EXPIRES_IN so it can be tuned without code changes.
src/services/jwt.service.js
Outdated
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { |
There was a problem hiding this comment.
jwt.sign(user, ...) signs the entire user object. Prefer signing a minimal payload (for example { id: user.id, email: user.email }) to reduce token size and avoid leaking unnecessary fields in tokens.
src/services/jwt.service.js
Outdated
| } | ||
|
|
||
| function signRefresh(user) { | ||
| const token = jwt.sign(user, process.env.JWT_REFRESH_KEY); |
There was a problem hiding this comment.
signRefresh currently issues a refresh token with no expiresIn. While refresh tokens are typically longer-lived, consider adding an explicit expiry (e.g. from process.env.JWT_REFRESH_EXPIRES_IN) or implementing rotation/blacklisting logic to limit lifetime and improve security.
| function verify(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_KEY); | ||
| } catch (err) { |
There was a problem hiding this comment.
verify/verifyRefresh return null on error which is fine, but swallowing errors can make debugging harder. Consider logging the verification error (or include a debug-level log) so invalid token issues are easier to investigate in development/test environments.
| @@ -0,0 +1,9 @@ | |||
| import { Sequelize } from 'sequelize'; | |||
|
|
|||
| export const client = new Sequelize({ | |||
There was a problem hiding this comment.
You instantiate Sequelize here — consider adding common options for production robustness such as logging: false (to silence SQL logs) and pool settings. If your DB is hosted (Heroku, managed Postgres) you may also need dialectOptions.ssl to allow TLS connections.
src/utils/db.js
Outdated
| 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 config relies on environment variables that may be undefined at runtime. Consider validating required env vars at startup or supplying sensible defaults, so the app fails fast with a clear error instead of producing a vague DB connection failure later.
| username: process.env.DB_USER, | ||
| password: process.env.DB_PASSWORD, | ||
| database: process.env.DB_DATABASE, | ||
| dialect: 'postgres', |
There was a problem hiding this comment.
If your Postgres instance uses a non-default port include port: process.env.DB_PORT here. Without it Sequelize will use the default Postgres port (5432), which may be incorrect for some deployments.
.gitignore
Outdated
| # env files | ||
| *.env | ||
| .env* | ||
| .env |
There was a problem hiding this comment.
Good to ignore .env. To avoid accidentally committing env variants, consider ignoring all variants and client-side env files too (for example add .env* and client/.env* to the file) so .env.local, .env.production, etc. are also covered.
client/src/App.jsx
Outdated
| onClick={() => { | ||
| logout() | ||
| .then(() => { | ||
| navigate('/'); |
There was a problem hiding this comment.
Logout redirect: the logout handler navigates to '/' after sign-out, but the checklist requires "Redirect to login after logging out." Consider changing this to navigate('/login') (and ensure logout clears auth state).
client/src/App.jsx
Outdated
| <section className="section"> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Registration should be available only to non-authenticated users per the requirements. Currently the registration route is public; add a guard (e.g. a RequireNoAuth wrapper or redirect inside the RegistrationPage when user is authenticated) to prevent access for logged-in users. See checklist: registration only non-auth.
| <Route | ||
| path="activate/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> |
There was a problem hiding this comment.
Non-auth-only pages: Activation page should be accessible only to non-authenticated users. Consider protecting this route for logged-in users (or force logout then process activation), and ensure the UX redirects to /profile after successful activation as required by the task.
client/src/App.jsx
Outdated
| path="confirm-email-change/:activationToken" | ||
| element={<EmailConfirmPage />} | ||
| /> | ||
| <Route path="login" element={<LoginPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Login page should be available only to non-authenticated users. Add a guard so logged-in users cannot access /login (or redirect them immediately to /profile).
client/src/App.jsx
Outdated
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="reset-password/:activationToken" element={<ResetPasswordPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Forgot-password and Reset-password routes must be only for non-authenticated users per the checklist. Add a guard or redirect for authenticated users so these pages can't be used while logged in.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
Missing 404 route: the app needs a catch-all NotFound route (e.g. <Route path="*" element={} />) so unknown URLs show a 404 page as required by the task. Add this route inside (before closing).
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [user, setUser] = useState(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked initial value should start as false so the app can show a Loader while checkAuth is running. Currently it's true and the app skips the loading state immediately (App checks if (!isChecked) return <Loader/>). Initialize with useState(false) and setChecked(true) after checkAuth completes.
| async function activate(activationToken) { | ||
| // const { accessToken, user } = await authService.activate(activationToken); | ||
| await authService.activate(activationToken); | ||
| // accessTokenService.save(accessToken); | ||
| // setUser(user); | ||
| } |
There was a problem hiding this comment.
activate() does not set an access token or the user after a successful activation. The requirements state the user must be activated after email confirmation and redirected to Profile. Either have the backend return { accessToken, user } from the activation endpoint and uncomment/save the token and setUser here, or call authService.refresh() (or login) after activation to obtain tokens and then set the user and navigate to /profile. Right now the commented code shows the intended behavior but it's not executed.
| async function checkAuth() { | ||
| try { | ||
| const { accessToken, user } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
| setUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); | ||
| } finally { | ||
| setChecked(true); |
There was a problem hiding this comment.
checkAuth stores accessToken and sets user — good. Note: because isChecked was initialized to true, the loader won't appear while this runs (see prior comment). Keep the try/catch as is, but ensure the initial isChecked change (above) so the check sequence is visible to users.
| const value = useMemo( | ||
| () => ({ | ||
| isChecked, | ||
| user, | ||
| checkAuth, | ||
| activate, | ||
| login, | ||
| logout, | ||
| updateUser | ||
| }), | ||
| [user, isChecked, updateUser], |
There was a problem hiding this comment.
The value passed to useMemo includes updateUser in the dependency array but updateUser is a new function each render. This will cause the memo to be recomputed every render. Consider stabilizing updateUser with useCallback or removing it from the deps so the context value is stable across renders.
client/src/hooks/usePageError.js
Outdated
| @@ -0,0 +1,21 @@ | |||
| import { useEffect, useState } from 'react'; | |||
|
|
|||
| export const usePageError = initialError => { | |||
There was a problem hiding this comment.
Consider giving the initialError parameter a default value to avoid undefined when the hook is called without arguments (App.jsx calls usePageError() with no argument). For example: export const usePageError = (initialError = '') => { ... }. This makes intent explicit and avoids callers having to pass an empty string.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
There was a problem hiding this comment.
Ensure createClient is exported from ./index.js and returns an axios-like instance; this file depends on that implementation. If createClient is missing or returns a different shape, authClient will fail. (Used by client services such as authService.)
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
|
|
|||
| export const authClient = createClient(); | |||
There was a problem hiding this comment.
This line instantiates the client and is fine. Keep this close to the createClient import so interceptors are applied to the right instance.
client/src/http/authClient.js
Outdated
|
|
||
| export const authClient = createClient(); | ||
|
|
||
| authClient.interceptors.response.use(res => res.data); |
There was a problem hiding this comment.
Currently you only add a success interceptor that returns res.data. Consider adding an error interceptor (second parameter to interceptors.response.use) to normalize or re-throw errors in a predictable way, e.g. forward error with Promise.reject(error) or map to a consistent error shape so components receive expected error.response data.
client/src/pages/LoginPage.jsx
Outdated
| onSubmit={({ email, password }) => { | ||
| return login({ email, password }) | ||
| .then(() => { | ||
| navigate(location.state?.from?.pathname || '/'); |
There was a problem hiding this comment.
Redirect target after login should lead to Profile by default per the checklist. Change the fallback in the navigation to '/profile' so users are redirected to their profile after successful login (unless a specific 'from' location is required).
client/src/pages/LoginPage.jsx
Outdated
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { login } = useContext(AuthContext); |
There was a problem hiding this comment.
This page must be available only to non-authenticated users. Right now LoginPage uses only login from AuthContext. Pull user (and isChecked if needed) from AuthContext and, when user is present, immediately redirect to /profile (or render a ). This prevents logged-in users from accessing the login page.
|
|
||
| export const LoginPage = () => { | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); |
There was a problem hiding this comment.
Small improvement: consider making the redirect check conditional on isChecked (auth-check finished) to avoid flicker — e.g., wait for isChecked true then redirect if user exists. You can get isChecked from AuthContext together with user.
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
You need to redirect the user to the Profile page after successful activation. Add navigation here (e.g. useNavigate and navigate('/profile')) or call checkAuth() and then navigate. Right now the page only shows a success message but does not redirect as required by the checklist.
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); |
There was a problem hiding this comment.
Forcing logout before activation may be unnecessary and produces poor UX — it logs out any existing authenticated user even if they are already signed in. Instead, check whether the user is currently authenticated and redirect them appropriately, or only logout if there's a specific reason. Consider removing unconditional logout or replacing it with a guard/redirect for authenticated users.
| const { activate, logout } = useContext(AuthContext); | ||
| const { activationToken } = useParams(); | ||
|
|
||
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
activate() in AuthContext currently only calls the backend and does not save tokens or set the user (the token/user handling is commented out). AccountActivationPage relies on that to become authenticated after activation. Update AuthContext.activate to save the accessToken and setUser (or have activation return tokens), or call checkAuth() here after activation to obtain auth state. See AuthContext for the commented code.
| }; | ||
|
|
||
| processActivation(); | ||
| }, [activationToken]); |
There was a problem hiding this comment.
The useEffect dependency array contains only activationToken but references logout and activate from context. To avoid stale closure warnings and ensure correct behavior, add logout and activate (or stable callbacks) to the dependency array or wrap them with useCallback in AuthContext.
| @@ -0,0 +1,46 @@ | |||
| import { useContext, useEffect, useState } from 'react'; | |||
| import { useParams } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Missing import for navigation: if you choose to navigate to /profile after activation, import useNavigate from 'react-router-dom' at the top of this file and call it inside the success path. (Add the import near line 2 and call navigate after activate succeeds.)
| @@ -0,0 +1,72 @@ | |||
| import React, { useState } from 'react'; | |||
| import { Formik, Form, Field } from 'formik'; | |||
| import { userService } from '../services/userService'; | |||
There was a problem hiding this comment.
Verify the client-side userService provides a forgotPassword(email) method that calls POST /forgot-password. The backend route and handler exist (auth.route.js and auth.controller.js), but if the client service is missing or named differently this will throw at runtime. Ensure userService.forgotPassword is implemented and returns the API promise. See server endpoints for reference.
| import { userService } from '../services/userService'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| export const ForgotPasswordPage = () => { |
There was a problem hiding this comment.
The Forgot Password page must be available only to non-authenticated users according to the requirements. This component has no auth check or redirect. Add a guard (for example read AuthContext and redirect authenticated users to /profile, or create a RequireNoAuth wrapper) so logged-in users cannot access this page.
| <Field | ||
| name="email" | ||
| type="email" | ||
| className="input" | ||
| placeholder="e.g. alex@example.com" | ||
| required | ||
| /> |
There was a problem hiding this comment.
Consider adding client-side email validation before submitting (e.g. basic pattern check) to fail fast and avoid unnecessary API calls — currently the form uses required only and relies on server response for invalid addresses. This is a UX suggestion (not strictly required), but improves feedback if users mistype their email.
| } catch (err) { | ||
| setErrorMsg(err.response?.data?.message || 'Something went wrong'); |
There was a problem hiding this comment.
Error handling displays err.response?.data?.message which is good. Make sure the server returns useful messages for cases like "email not found" (the backend currently throws a meaningful error), so the UI message is informative.
client/src/services/authService.js
Outdated
| function login({ name, email, password }) { | ||
| return authClient.post('/login', { name, email, password }); |
There was a problem hiding this comment.
The login function destructures { name, email, password } and posts name to the /login endpoint. The server's login expects only email and password — including name is unnecessary and confusing. Change the parameter to ({ email, password }) and post only those fields to avoid accidental data leakage or confusion.
client/src/services/authService.js
Outdated
| function activate(activationToken) { | ||
| return authClient.get(`/activation/${activationToken}`); |
There was a problem hiding this comment.
activate calls GET /activation/:token which on the server currently returns only a success message. If you want the client to become authenticated immediately after activation (so it can redirect to Profile automatically), the backend must return { accessToken, user } or the client must call refresh/login after activation. Consider coordinating the backend return value or performing an auth refresh here. (This is about integration/expectation — not a syntax error in this file.)
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical, blocking issues that violate the assignment’s functional requirements and can break core flows. First, the ESLint configuration is currently broken (CommonJS config loaded as ESM) and CI cannot run tests at all; this must be fixed by using a .cjs config or ESM export so the automated checks can execute. Second, key auth flows do not meet the checklist: after account activation and email-change confirmation the client does not set tokens/user or redirect to /profile, login redirects to / instead of /profile, and logout redirects to / instead of /login, so required navigation behavior is missing. Third, several pages that must be available only to non-authenticated users (registration, activation, login, forgot password, reset password, email confirmation) are not consistently guarded (or force-logout in a brittle way), meaning authenticated users can access non-auth-only routes, which directly contradicts the checklist. Finally, there are backend correctness bugs in auth/token handling (e.g. HttpOnly instead of httpOnly when setting the refresh cookie, missing await on tokenService.getByToken and Token.create) that can cause invalid refresh behavior or race conditions. Please address these blockers, then also consider the smaller suggested improvements (better error handling in profile/password flows, more robust email/DB handling, and minor UX tweaks) to make your solution more robust—your overall structure is solid, and fixing these points will bring it fully in line with 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
client/.eslintrc.cjs
Outdated
| {blankLine: 'always', prev: 'directive', next: '*'}, | ||
| {blankLine: 'always', prev: 'block-like', next: '*'}, | ||
| ], | ||
| 'implicit-arrow-linebreak:': 0, |
There was a problem hiding this comment.
The rule key contains an extra colon character so ESLint will not recognize it. Change 'implicit-arrow-linebreak:' to 'implicit-arrow-linebreak' so the rule actually disables implicit arrow linebreak warnings.
client/.eslintrc.cjs
Outdated
| 'plugin:cypress/recommended', | ||
| ], | ||
| parserOptions: { | ||
| ecmaVersion: "2024", |
There was a problem hiding this comment.
Prefer a numeric ecmaVersion (or use 'latest') instead of a string. Use ecmaVersion: 2024 (no quotes) or ecmaVersion: 'latest' to avoid any parser ambiguity.
| @@ -0,0 +1,5 @@ | |||
| export const Loader = () => ( | |||
There was a problem hiding this comment.
This file uses JSX without importing React. If your project does not use the automatic JSX runtime (React 17+ with the correct Babel/TS/Vite config), this will cause a runtime/build error. Either add import React from 'react' at the top or ensure the automatic JSX runtime is enabled in your build tool (e.g. Babel or Vite).
| } | ||
|
|
||
| if (user) { | ||
| return <Navigate to="/" replace />; |
There was a problem hiding this comment.
When an authenticated user tries to access a non-auth-only page we redirect to /. The checklist requires routing authenticated users to Profile for auth-related flows — change the target to /profile (or make the redirect path configurable) so authenticated users are sent to their Profile page instead of Home.
client/src/http/httpClient.js
Outdated
| httpClient.interceptors.response.use(onResponseSuccess, onResponseError); | ||
|
|
||
| function onRequest(request) { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
Use the accessToken service getter instead of reading localStorage directly for consistency and to centralize storage access. Replace localStorage.getItem('accessToken') with accessTokenService.get() (you already use accessTokenService.save elsewhere). See client AuthContext for how access tokens are saved via the service .
| } | ||
|
|
||
| async function onResponseError(error) { | ||
| const originalRequest = error.config; |
There was a problem hiding this comment.
Guard error.config (originalRequest). If error.config is undefined this will throw later when accessing _retry. Add a defensive check and rethrow if missing (e.g. if (!originalRequest) throw error;).
client/src/http/httpClient.js
Outdated
| async function onResponseError(error) { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response.status !== 401 || originalRequest._retry) { |
There was a problem hiding this comment.
Before reading error.response.status ensure error.response exists. Accessing .status when error.response is undefined (network error, CORS, etc.) will itself throw. Example: if (!error.response || error.response.status !== 401 || originalRequest._retry) { throw error; }
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { accessToken } = await authService.refresh(); |
There was a problem hiding this comment.
You call authService.refresh() to obtain a new access token — good. Make sure you know authService.refresh() uses the separate authClient instance (so it doesn't hit this interceptor recursively). This file relies on that behavior to avoid loops (authService implementation shown) .
| try { | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); |
There was a problem hiding this comment.
When you save the new access token you call accessTokenService.save(accessToken) (good). For full consistency, read the token with accessTokenService.get() in onRequest (see first comment) rather than localStorage.getItem so all token access goes through the same API.
| function onResponseSuccess(res) { | ||
| return res.data; |
There was a problem hiding this comment.
onResponseSuccess returns res.data, which is fine. Be aware if createClient() already applies response transforms elsewhere; duplicating unwrapping is harmless but keep consistent with your other clients (e.g. authClient sets an interceptor that returns res.data) .
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest); | ||
| } catch (error) { |
There was a problem hiding this comment.
Consider returning Promise.reject(error) instead of throw error inside the catch for clarity in interceptor async handlers — both will reject, but Promise.reject is explicit for interceptor flows. Current throw is acceptable in async function, so this is optional.
client/src/http/index.js
Outdated
|
|
||
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', |
There was a problem hiding this comment.
Hardcoded baseURL to 'http://localhost:3005' makes the client fail in other environments. Consider using an environment variable (e.g. VITE_API_URL / import.meta.env.VITE_API_URL or process.env) so the API host can change between dev and production.
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', | ||
| withCredentials: true, |
There was a problem hiding this comment.
Enabling withCredentials: true is appropriate for cookie-based auth (so refresh/activation cookies are sent). Ensure the server's CORS config allows credentials and the client origin (the server entrypoint shows credentials/CORS config must match) — otherwise requests will be blocked.
| import { useParams, Link } from 'react-router-dom'; | ||
| import { userService } from '../services/userService'; | ||
|
|
||
| export const EmailConfirmPage = () => { |
There was a problem hiding this comment.
This component does not enforce or check authentication state. The checklist expects email confirmation flows to be part of the auth-related flows; either wrap this route with RequireNonAuth in the router or read AuthContext here and redirect authenticated users to /profile to prevent inappropriate access.
| const [result, setResult] = useState('Verifying...'); | ||
|
|
||
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) |
There was a problem hiding this comment.
If activationToken is absent or undefined the API call may fail with an unclear message. Consider checking activationToken before calling userService.confirmEmailChange and set an appropriate error if it's missing so users see a helpful message.
| <div className="container has-text-centered mt-6"> | ||
| <div className="box"> | ||
| <h1 className="title">{result}</h1> | ||
| <Link to="/login" className="button is-primary">Go to Login</Link> |
There was a problem hiding this comment.
After successful confirmation you always render a link to /login. If the user is already authenticated it would be better UX to link to /profile (or navigate there). Consider reading user from AuthContext and conditionally render the CTA (Profile when authenticated, Login otherwise) or perform an automatic redirect.
|
|
||
| export const EmailConfirmPage = () => { | ||
| const { activationToken } = useParams(); | ||
| const [result, setResult] = useState('Verifying...'); |
There was a problem hiding this comment.
Currently the effect updates result on success/error but there is no explicit loading state beyond the initial 'Verifying...' text. This is acceptable, but consider showing a proper loader component while the request is in-flight for clearer UX (especially on slow networks).
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { | ||
| setErrors({ oldPassword: backendMsg }); | ||
| } else { | ||
| setErrors({ oldPassword: 'Failed to update password' }); | ||
| } |
There was a problem hiding this comment.
In the password update catch block you use backendMsg but the conditional is incorrect and can throw when backendMsg is falsy. Currently it’s if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) which evaluates the right-hand includes even when backendMsg is undefined. Wrap the OR inside the backendMsg check and use parentheses, e.g. if (backendMsg && (backendMsg.includes('old password') || backendMsg.includes('at least'))) { ... } to avoid runtime errors.
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { | ||
| setErrors({ oldPassword: backendMsg }); | ||
| } else { | ||
| setErrors({ oldPassword: 'Failed to update password' }); | ||
| } |
There was a problem hiding this comment.
When the backend complains about password length (the 'at least' branch) you set the error on oldPassword. That maps the message to the wrong field. For length/at least errors set the error on newPassword (e.g. setErrors({ newPassword: backendMsg })) and only set oldPassword when the backend indicates the old password is incorrect.
| } catch (err) { | ||
| const errorMsg = err.response?.data?.message || 'Failed to update name'; |
There was a problem hiding this comment.
In submitName's catch block you compute errorMsg but never use it or show feedback to the user. Consider calling setNameMsg(errorMsg) or storing the error into a separate state so the user sees why the update failed.
client/src/pages/ProfilePage.jsx
Outdated
| import React, { useContext, useState } from 'react'; | ||
| import { Formik, Form, Field } from 'formik'; | ||
| import { AuthContext } from '../components/AuthContext.jsx'; | ||
| import cn from 'classnames'; |
There was a problem hiding this comment.
You import cn (classnames) but never use it in this file; this will trigger an unused-import lint warning. Remove the import if not needed or use it where appropriate.
| } | ||
| }; | ||
|
|
||
| export const RegistrationPage = () => { |
There was a problem hiding this comment.
Registration pages must be available only to non-authenticated users (checklist requirement). This component doesn't guard itself — make sure the router wraps this route with RequireNonAuth or read AuthContext here and redirect authenticated users to /profile so logged-in users cannot access registration.
| className={cn('button is-success has-text-weight-bold', { | ||
| 'is-loading': isSubmitting, | ||
| })} | ||
| disabled={isSubmitting || errors.email || errors.password} |
There was a problem hiding this comment.
The submit button is disabled based only on errors.email and errors.password. That allows submitting when name is invalid. Include errors.name in the disabled condition (and/or use a combined validity flag or isValid) so the button remains disabled until all fields are valid.
| .then(() => { | ||
| setRegistered(true); |
There was a problem hiding this comment.
You set registered to true and show the “Check your email” message which is correct UI behavior, but the client can't guarantee the activation email was sent. Verify authService.register triggers the backend to send the activation email and handle/report server failures (e.g. show the server message if email delivery fails).
| onSubmit={({ name, email, password }, formikHelpers) => { | ||
| formikHelpers.setSubmitting(true); |
There was a problem hiding this comment.
You call formikHelpers.setSubmitting(true) at the top of onSubmit. When using Formik, if onSubmit returns a Promise you can let Formik manage isSubmitting automatically; if you keep the manual calls that's fine, but it's redundant — consider returning the Promise from authService.register(...) instead and let Formik manage submitting state to simplify the code.
| function requestEmailChange(data) { | ||
| return httpClient.patch('/profile/request-email-change', data); |
There was a problem hiding this comment.
requestEmailChange accepts a generic data object. The checklist requires that changing an email requires typing the password and confirmation of the new email. Ensure that when calling this function you pass both password and the newEmail (or email) fields — e.g. { newEmail, password } — and that server-side starts the confirmation flow (notify old email, send confirmation to new email). Consider documenting the expected shape here for clarity.
| function confirmEmailChange(activationToken) { | ||
| return httpClient.get(`/profile/confirm-email-change/${activationToken}`); |
There was a problem hiding this comment.
confirmEmailChange uses a GET request to /profile/confirm-email-change/${activationToken} which places the activation token in the URL. This works, but be aware that tokens in URLs can be logged in server logs or browser history. If your backend supports it, consider using POST with the token in the body to reduce exposure (optional improvement).
| function resetPassword(activationToken, password) { | ||
| return httpClient.post('/reset-password', { activationToken, password }); |
There was a problem hiding this comment.
resetPassword posts { activationToken, password } to /reset-password. This is fine and matches a typical flow, but for consistency and clarity you might prefer the function to accept a single object parameter (e.g. resetPassword({ activationToken, password })) like updatePassword does. This reduces caller confusion about argument order and makes future extension easier.
| @@ -0,0 +1,17 @@ | |||
| import 'dotenv/config'; | |||
| import { client } from './src/utils/db.js'; | |||
| import { User } from './src/models/User.js'; | |||
There was a problem hiding this comment.
User is imported but not referenced. If the import is intentional to register model definitions with the Sequelize instance, add a short comment explaining that; otherwise remove the unused import to avoid confusion.
| import 'dotenv/config'; | ||
| import { client } from './src/utils/db.js'; | ||
| import { User } from './src/models/User.js'; | ||
| import { Token } from './src/models/Token.js'; |
There was a problem hiding this comment.
Token is imported but not referenced. Same as above — either remove it or add a comment that importing the model registers it with client so tables are created/altered during sync.
|
|
||
| async function init() { | ||
| try { | ||
| await client.sync({ alter: true }); |
There was a problem hiding this comment.
client.sync({ alter: true }) will attempt to alter existing tables to match models rather than fully recreate them. The log below says "Database tables recreated!" which is misleading. If you intend to fully recreate tables use force: true (destructive) or change the message to reflect that tables were synchronized/updated.
| try { | ||
| await client.sync({ alter: true }); | ||
| console.log('Database tables recreated!'); | ||
| process.exit(0); |
There was a problem hiding this comment.
You call process.exit(0) immediately after sync; consider closing the DB connection gracefully first. For Sequelize use await client.close() (or the appropriate API) before exiting to ensure all connections are released. Do the same in the catch block before exiting with an error code.
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, |
There was a problem hiding this comment.
Consider making refreshToken unique to prevent storing duplicate tokens for different users or accidental duplicates. Add unique: true to the column definition so the DB enforces it.
src/models/Token.js
Outdated
| Token.belongsTo(User); | ||
| User.hasOne(Token); |
There was a problem hiding this comment.
Be explicit about the association options. Specify the foreign key and cascade behavior to ensure a Token is removed when its User is deleted, for example: Token.belongsTo(User, { foreignKey: 'userId', onDelete: 'CASCADE' }) and User.hasOne(Token, { foreignKey: 'userId' }). This avoids orphaned tokens if a user is removed.
src/services/jwt.service.js
Outdated
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '5s', |
There was a problem hiding this comment.
expiresIn: '5s' is extremely short and will cause frequent refresh requests and poor UX. Use a more realistic access token lifetime (e.g. '15m', '1h') or read the value from an env var like process.env.JWT_EXPIRES_IN so it can be tuned without code changes.
src/services/jwt.service.js
Outdated
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { |
There was a problem hiding this comment.
jwt.sign(user, ...) signs the entire user object. Prefer signing a minimal payload (for example { id: user.id, email: user.email }) to reduce token size and avoid leaking unnecessary fields in tokens.
src/services/jwt.service.js
Outdated
| } | ||
|
|
||
| function signRefresh(user) { | ||
| const token = jwt.sign(user, process.env.JWT_REFRESH_KEY); |
There was a problem hiding this comment.
signRefresh currently issues a refresh token with no expiresIn. While refresh tokens are typically longer-lived, consider adding an explicit expiry (e.g. from process.env.JWT_REFRESH_EXPIRES_IN) or implementing rotation/blacklisting logic to limit lifetime and improve security.
| function verify(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_KEY); | ||
| } catch (err) { |
There was a problem hiding this comment.
verify/verifyRefresh return null on error which is fine, but swallowing errors can make debugging harder. Consider logging the verification error (or include a debug-level log) so invalid token issues are easier to investigate in development/test environments.
| @@ -0,0 +1,9 @@ | |||
| import { Sequelize } from 'sequelize'; | |||
|
|
|||
| export const client = new Sequelize({ | |||
There was a problem hiding this comment.
You instantiate Sequelize here — consider adding common options for production robustness such as logging: false (to silence SQL logs) and pool settings. If your DB is hosted (Heroku, managed Postgres) you may also need dialectOptions.ssl to allow TLS connections.
src/utils/db.js
Outdated
| 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 config relies on environment variables that may be undefined at runtime. Consider validating required env vars at startup or supplying sensible defaults, so the app fails fast with a clear error instead of producing a vague DB connection failure later.
| username: process.env.DB_USER, | ||
| password: process.env.DB_PASSWORD, | ||
| database: process.env.DB_DATABASE, | ||
| dialect: 'postgres', |
There was a problem hiding this comment.
If your Postgres instance uses a non-default port include port: process.env.DB_PORT here. Without it Sequelize will use the default Postgres port (5432), which may be incorrect for some deployments.
.gitignore
Outdated
| # env files | ||
| *.env | ||
| .env* | ||
| .env |
There was a problem hiding this comment.
Good to ignore .env. To avoid accidentally committing env variants, consider ignoring all variants and client-side env files too (for example add .env* and client/.env* to the file) so .env.local, .env.production, etc. are also covered.
client/src/App.jsx
Outdated
| onClick={() => { | ||
| logout() | ||
| .then(() => { | ||
| navigate('/'); |
There was a problem hiding this comment.
Logout redirect: the logout handler navigates to '/' after sign-out, but the checklist requires "Redirect to login after logging out." Consider changing this to navigate('/login') (and ensure logout clears auth state).
client/src/App.jsx
Outdated
| <section className="section"> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Registration should be available only to non-authenticated users per the requirements. Currently the registration route is public; add a guard (e.g. a RequireNoAuth wrapper or redirect inside the RegistrationPage when user is authenticated) to prevent access for logged-in users. See checklist: registration only non-auth.
| <Route | ||
| path="activate/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> |
There was a problem hiding this comment.
Non-auth-only pages: Activation page should be accessible only to non-authenticated users. Consider protecting this route for logged-in users (or force logout then process activation), and ensure the UX redirects to /profile after successful activation as required by the task.
client/src/App.jsx
Outdated
| path="confirm-email-change/:activationToken" | ||
| element={<EmailConfirmPage />} | ||
| /> | ||
| <Route path="login" element={<LoginPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Login page should be available only to non-authenticated users. Add a guard so logged-in users cannot access /login (or redirect them immediately to /profile).
client/src/App.jsx
Outdated
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="reset-password/:activationToken" element={<ResetPasswordPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Forgot-password and Reset-password routes must be only for non-authenticated users per the checklist. Add a guard or redirect for authenticated users so these pages can't be used while logged in.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
Missing 404 route: the app needs a catch-all NotFound route (e.g. <Route path="*" element={} />) so unknown URLs show a 404 page as required by the task. Add this route inside (before closing).
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [user, setUser] = useState(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked initial value should start as false so the app can show a Loader while checkAuth is running. Currently it's true and the app skips the loading state immediately (App checks if (!isChecked) return <Loader/>). Initialize with useState(false) and setChecked(true) after checkAuth completes.
| async function activate(activationToken) { | ||
| // const { accessToken, user } = await authService.activate(activationToken); | ||
| await authService.activate(activationToken); | ||
| // accessTokenService.save(accessToken); | ||
| // setUser(user); | ||
| } |
There was a problem hiding this comment.
activate() does not set an access token or the user after a successful activation. The requirements state the user must be activated after email confirmation and redirected to Profile. Either have the backend return { accessToken, user } from the activation endpoint and uncomment/save the token and setUser here, or call authService.refresh() (or login) after activation to obtain tokens and then set the user and navigate to /profile. Right now the commented code shows the intended behavior but it's not executed.
| async function checkAuth() { | ||
| try { | ||
| const { accessToken, user } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
| setUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); | ||
| } finally { | ||
| setChecked(true); |
There was a problem hiding this comment.
checkAuth stores accessToken and sets user — good. Note: because isChecked was initialized to true, the loader won't appear while this runs (see prior comment). Keep the try/catch as is, but ensure the initial isChecked change (above) so the check sequence is visible to users.
| const value = useMemo( | ||
| () => ({ | ||
| isChecked, | ||
| user, | ||
| checkAuth, | ||
| activate, | ||
| login, | ||
| logout, | ||
| updateUser | ||
| }), | ||
| [user, isChecked, updateUser], |
There was a problem hiding this comment.
The value passed to useMemo includes updateUser in the dependency array but updateUser is a new function each render. This will cause the memo to be recomputed every render. Consider stabilizing updateUser with useCallback or removing it from the deps so the context value is stable across renders.
client/src/hooks/usePageError.js
Outdated
| @@ -0,0 +1,21 @@ | |||
| import { useEffect, useState } from 'react'; | |||
|
|
|||
| export const usePageError = initialError => { | |||
There was a problem hiding this comment.
Consider giving the initialError parameter a default value to avoid undefined when the hook is called without arguments (App.jsx calls usePageError() with no argument). For example: export const usePageError = (initialError = '') => { ... }. This makes intent explicit and avoids callers having to pass an empty string.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
There was a problem hiding this comment.
Ensure createClient is exported from ./index.js and returns an axios-like instance; this file depends on that implementation. If createClient is missing or returns a different shape, authClient will fail. (Used by client services such as authService.)
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
|
|
|||
| export const authClient = createClient(); | |||
There was a problem hiding this comment.
This line instantiates the client and is fine. Keep this close to the createClient import so interceptors are applied to the right instance.
client/src/http/authClient.js
Outdated
|
|
||
| export const authClient = createClient(); | ||
|
|
||
| authClient.interceptors.response.use(res => res.data); |
There was a problem hiding this comment.
Currently you only add a success interceptor that returns res.data. Consider adding an error interceptor (second parameter to interceptors.response.use) to normalize or re-throw errors in a predictable way, e.g. forward error with Promise.reject(error) or map to a consistent error shape so components receive expected error.response data.
client/src/pages/LoginPage.jsx
Outdated
| onSubmit={({ email, password }) => { | ||
| return login({ email, password }) | ||
| .then(() => { | ||
| navigate(location.state?.from?.pathname || '/'); |
There was a problem hiding this comment.
Redirect target after login should lead to Profile by default per the checklist. Change the fallback in the navigation to '/profile' so users are redirected to their profile after successful login (unless a specific 'from' location is required).
client/src/pages/LoginPage.jsx
Outdated
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { login } = useContext(AuthContext); |
There was a problem hiding this comment.
This page must be available only to non-authenticated users. Right now LoginPage uses only login from AuthContext. Pull user (and isChecked if needed) from AuthContext and, when user is present, immediately redirect to /profile (or render a ). This prevents logged-in users from accessing the login page.
|
|
||
| export const LoginPage = () => { | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); |
There was a problem hiding this comment.
Small improvement: consider making the redirect check conditional on isChecked (auth-check finished) to avoid flicker — e.g., wait for isChecked true then redirect if user exists. You can get isChecked from AuthContext together with user.
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
You need to redirect the user to the Profile page after successful activation. Add navigation here (e.g. useNavigate and navigate('/profile')) or call checkAuth() and then navigate. Right now the page only shows a success message but does not redirect as required by the checklist.
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); |
There was a problem hiding this comment.
Forcing logout before activation may be unnecessary and produces poor UX — it logs out any existing authenticated user even if they are already signed in. Instead, check whether the user is currently authenticated and redirect them appropriately, or only logout if there's a specific reason. Consider removing unconditional logout or replacing it with a guard/redirect for authenticated users.
| const { activate, logout } = useContext(AuthContext); | ||
| const { activationToken } = useParams(); | ||
|
|
||
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
activate() in AuthContext currently only calls the backend and does not save tokens or set the user (the token/user handling is commented out). AccountActivationPage relies on that to become authenticated after activation. Update AuthContext.activate to save the accessToken and setUser (or have activation return tokens), or call checkAuth() here after activation to obtain auth state. See AuthContext for the commented code.
| }; | ||
|
|
||
| processActivation(); | ||
| }, [activationToken]); |
There was a problem hiding this comment.
The useEffect dependency array contains only activationToken but references logout and activate from context. To avoid stale closure warnings and ensure correct behavior, add logout and activate (or stable callbacks) to the dependency array or wrap them with useCallback in AuthContext.
| @@ -0,0 +1,46 @@ | |||
| import { useContext, useEffect, useState } from 'react'; | |||
| import { useParams } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Missing import for navigation: if you choose to navigate to /profile after activation, import useNavigate from 'react-router-dom' at the top of this file and call it inside the success path. (Add the import near line 2 and call navigate after activate succeeds.)
| @@ -0,0 +1,72 @@ | |||
| import React, { useState } from 'react'; | |||
| import { Formik, Form, Field } from 'formik'; | |||
| import { userService } from '../services/userService'; | |||
There was a problem hiding this comment.
Verify the client-side userService provides a forgotPassword(email) method that calls POST /forgot-password. The backend route and handler exist (auth.route.js and auth.controller.js), but if the client service is missing or named differently this will throw at runtime. Ensure userService.forgotPassword is implemented and returns the API promise. See server endpoints for reference.
| import { userService } from '../services/userService'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| export const ForgotPasswordPage = () => { |
There was a problem hiding this comment.
The Forgot Password page must be available only to non-authenticated users according to the requirements. This component has no auth check or redirect. Add a guard (for example read AuthContext and redirect authenticated users to /profile, or create a RequireNoAuth wrapper) so logged-in users cannot access this page.
| <Field | ||
| name="email" | ||
| type="email" | ||
| className="input" | ||
| placeholder="e.g. alex@example.com" | ||
| required | ||
| /> |
There was a problem hiding this comment.
Consider adding client-side email validation before submitting (e.g. basic pattern check) to fail fast and avoid unnecessary API calls — currently the form uses required only and relies on server response for invalid addresses. This is a UX suggestion (not strictly required), but improves feedback if users mistype their email.
| } catch (err) { | ||
| setErrorMsg(err.response?.data?.message || 'Something went wrong'); |
There was a problem hiding this comment.
Error handling displays err.response?.data?.message which is good. Make sure the server returns useful messages for cases like "email not found" (the backend currently throws a meaningful error), so the UI message is informative.
client/src/services/authService.js
Outdated
| function login({ name, email, password }) { | ||
| return authClient.post('/login', { name, email, password }); |
There was a problem hiding this comment.
The login function destructures { name, email, password } and posts name to the /login endpoint. The server's login expects only email and password — including name is unnecessary and confusing. Change the parameter to ({ email, password }) and post only those fields to avoid accidental data leakage or confusion.
client/src/services/authService.js
Outdated
| function activate(activationToken) { | ||
| return authClient.get(`/activation/${activationToken}`); |
There was a problem hiding this comment.
activate calls GET /activation/:token which on the server currently returns only a success message. If you want the client to become authenticated immediately after activation (so it can redirect to Profile automatically), the backend must return { accessToken, user } or the client must call refresh/login after activation. Consider coordinating the backend return value or performing an auth refresh here. (This is about integration/expectation — not a syntax error in this file.)
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical, blocking issues that violate the assignment’s functional requirements and can break core flows. First, the ESLint configuration is currently broken (CommonJS config loaded as ESM) and CI cannot run tests at all; this must be fixed by using a .cjs config or ESM export so the automated checks can execute. Second, key auth flows do not meet the checklist: after account activation and email-change confirmation the client does not set tokens/user or redirect to /profile, login redirects to / instead of /profile, and logout redirects to / instead of /login, so required navigation behavior is missing. Third, several pages that must be available only to non-authenticated users (registration, activation, login, forgot password, reset password, email confirmation) are not consistently guarded (or force-logout in a brittle way), meaning authenticated users can access non-auth-only routes, which directly contradicts the checklist. Finally, there are backend correctness bugs in auth/token handling (e.g. HttpOnly instead of httpOnly when setting the refresh cookie, missing await on tokenService.getByToken and Token.create) that can cause invalid refresh behavior or race conditions. Please address these blockers, then also consider the smaller suggested improvements (better error handling in profile/password flows, more robust email/DB handling, and minor UX tweaks) to make your solution more robust—your overall structure is solid, and fixing these points will bring it fully in line with 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
client/.eslintrc.cjs
Outdated
| {blankLine: 'always', prev: 'directive', next: '*'}, | ||
| {blankLine: 'always', prev: 'block-like', next: '*'}, | ||
| ], | ||
| 'implicit-arrow-linebreak:': 0, |
There was a problem hiding this comment.
The rule key contains an extra colon character so ESLint will not recognize it. Change 'implicit-arrow-linebreak:' to 'implicit-arrow-linebreak' so the rule actually disables implicit arrow linebreak warnings.
client/.eslintrc.cjs
Outdated
| 'plugin:cypress/recommended', | ||
| ], | ||
| parserOptions: { | ||
| ecmaVersion: "2024", |
There was a problem hiding this comment.
Prefer a numeric ecmaVersion (or use 'latest') instead of a string. Use ecmaVersion: 2024 (no quotes) or ecmaVersion: 'latest' to avoid any parser ambiguity.
| @@ -0,0 +1,5 @@ | |||
| export const Loader = () => ( | |||
There was a problem hiding this comment.
This file uses JSX without importing React. If your project does not use the automatic JSX runtime (React 17+ with the correct Babel/TS/Vite config), this will cause a runtime/build error. Either add import React from 'react' at the top or ensure the automatic JSX runtime is enabled in your build tool (e.g. Babel or Vite).
| } | ||
|
|
||
| if (user) { | ||
| return <Navigate to="/" replace />; |
There was a problem hiding this comment.
When an authenticated user tries to access a non-auth-only page we redirect to /. The checklist requires routing authenticated users to Profile for auth-related flows — change the target to /profile (or make the redirect path configurable) so authenticated users are sent to their Profile page instead of Home.
client/src/http/httpClient.js
Outdated
| httpClient.interceptors.response.use(onResponseSuccess, onResponseError); | ||
|
|
||
| function onRequest(request) { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
Use the accessToken service getter instead of reading localStorage directly for consistency and to centralize storage access. Replace localStorage.getItem('accessToken') with accessTokenService.get() (you already use accessTokenService.save elsewhere). See client AuthContext for how access tokens are saved via the service .
| } | ||
|
|
||
| async function onResponseError(error) { | ||
| const originalRequest = error.config; |
There was a problem hiding this comment.
Guard error.config (originalRequest). If error.config is undefined this will throw later when accessing _retry. Add a defensive check and rethrow if missing (e.g. if (!originalRequest) throw error;).
client/src/http/httpClient.js
Outdated
| async function onResponseError(error) { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response.status !== 401 || originalRequest._retry) { |
There was a problem hiding this comment.
Before reading error.response.status ensure error.response exists. Accessing .status when error.response is undefined (network error, CORS, etc.) will itself throw. Example: if (!error.response || error.response.status !== 401 || originalRequest._retry) { throw error; }
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { accessToken } = await authService.refresh(); |
There was a problem hiding this comment.
You call authService.refresh() to obtain a new access token — good. Make sure you know authService.refresh() uses the separate authClient instance (so it doesn't hit this interceptor recursively). This file relies on that behavior to avoid loops (authService implementation shown) .
| try { | ||
| const { accessToken } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); |
There was a problem hiding this comment.
When you save the new access token you call accessTokenService.save(accessToken) (good). For full consistency, read the token with accessTokenService.get() in onRequest (see first comment) rather than localStorage.getItem so all token access goes through the same API.
| function onResponseSuccess(res) { | ||
| return res.data; |
There was a problem hiding this comment.
onResponseSuccess returns res.data, which is fine. Be aware if createClient() already applies response transforms elsewhere; duplicating unwrapping is harmless but keep consistent with your other clients (e.g. authClient sets an interceptor that returns res.data) .
| accessTokenService.save(accessToken); | ||
|
|
||
| return httpClient.request(originalRequest); | ||
| } catch (error) { |
There was a problem hiding this comment.
Consider returning Promise.reject(error) instead of throw error inside the catch for clarity in interceptor async handlers — both will reject, but Promise.reject is explicit for interceptor flows. Current throw is acceptable in async function, so this is optional.
client/src/http/index.js
Outdated
|
|
||
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', |
There was a problem hiding this comment.
Hardcoded baseURL to 'http://localhost:3005' makes the client fail in other environments. Consider using an environment variable (e.g. VITE_API_URL / import.meta.env.VITE_API_URL or process.env) so the API host can change between dev and production.
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', | ||
| withCredentials: true, |
There was a problem hiding this comment.
Enabling withCredentials: true is appropriate for cookie-based auth (so refresh/activation cookies are sent). Ensure the server's CORS config allows credentials and the client origin (the server entrypoint shows credentials/CORS config must match) — otherwise requests will be blocked.
| import { useParams, Link } from 'react-router-dom'; | ||
| import { userService } from '../services/userService'; | ||
|
|
||
| export const EmailConfirmPage = () => { |
There was a problem hiding this comment.
This component does not enforce or check authentication state. The checklist expects email confirmation flows to be part of the auth-related flows; either wrap this route with RequireNonAuth in the router or read AuthContext here and redirect authenticated users to /profile to prevent inappropriate access.
| const [result, setResult] = useState('Verifying...'); | ||
|
|
||
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) |
There was a problem hiding this comment.
If activationToken is absent or undefined the API call may fail with an unclear message. Consider checking activationToken before calling userService.confirmEmailChange and set an appropriate error if it's missing so users see a helpful message.
| <div className="container has-text-centered mt-6"> | ||
| <div className="box"> | ||
| <h1 className="title">{result}</h1> | ||
| <Link to="/login" className="button is-primary">Go to Login</Link> |
There was a problem hiding this comment.
After successful confirmation you always render a link to /login. If the user is already authenticated it would be better UX to link to /profile (or navigate there). Consider reading user from AuthContext and conditionally render the CTA (Profile when authenticated, Login otherwise) or perform an automatic redirect.
|
|
||
| export const EmailConfirmPage = () => { | ||
| const { activationToken } = useParams(); | ||
| const [result, setResult] = useState('Verifying...'); |
There was a problem hiding this comment.
Currently the effect updates result on success/error but there is no explicit loading state beyond the initial 'Verifying...' text. This is acceptable, but consider showing a proper loader component while the request is in-flight for clearer UX (especially on slow networks).
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { | ||
| setErrors({ oldPassword: backendMsg }); | ||
| } else { | ||
| setErrors({ oldPassword: 'Failed to update password' }); | ||
| } |
There was a problem hiding this comment.
In the password update catch block you use backendMsg but the conditional is incorrect and can throw when backendMsg is falsy. Currently it’s if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) which evaluates the right-hand includes even when backendMsg is undefined. Wrap the OR inside the backendMsg check and use parentheses, e.g. if (backendMsg && (backendMsg.includes('old password') || backendMsg.includes('at least'))) { ... } to avoid runtime errors.
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { | ||
| setErrors({ oldPassword: backendMsg }); | ||
| } else { | ||
| setErrors({ oldPassword: 'Failed to update password' }); | ||
| } |
There was a problem hiding this comment.
When the backend complains about password length (the 'at least' branch) you set the error on oldPassword. That maps the message to the wrong field. For length/at least errors set the error on newPassword (e.g. setErrors({ newPassword: backendMsg })) and only set oldPassword when the backend indicates the old password is incorrect.
| } catch (err) { | ||
| const errorMsg = err.response?.data?.message || 'Failed to update name'; |
There was a problem hiding this comment.
In submitName's catch block you compute errorMsg but never use it or show feedback to the user. Consider calling setNameMsg(errorMsg) or storing the error into a separate state so the user sees why the update failed.
client/src/pages/ProfilePage.jsx
Outdated
| import React, { useContext, useState } from 'react'; | ||
| import { Formik, Form, Field } from 'formik'; | ||
| import { AuthContext } from '../components/AuthContext.jsx'; | ||
| import cn from 'classnames'; |
There was a problem hiding this comment.
You import cn (classnames) but never use it in this file; this will trigger an unused-import lint warning. Remove the import if not needed or use it where appropriate.
| } | ||
| }; | ||
|
|
||
| export const RegistrationPage = () => { |
There was a problem hiding this comment.
Registration pages must be available only to non-authenticated users (checklist requirement). This component doesn't guard itself — make sure the router wraps this route with RequireNonAuth or read AuthContext here and redirect authenticated users to /profile so logged-in users cannot access registration.
| className={cn('button is-success has-text-weight-bold', { | ||
| 'is-loading': isSubmitting, | ||
| })} | ||
| disabled={isSubmitting || errors.email || errors.password} |
There was a problem hiding this comment.
The submit button is disabled based only on errors.email and errors.password. That allows submitting when name is invalid. Include errors.name in the disabled condition (and/or use a combined validity flag or isValid) so the button remains disabled until all fields are valid.
| .then(() => { | ||
| setRegistered(true); |
There was a problem hiding this comment.
You set registered to true and show the “Check your email” message which is correct UI behavior, but the client can't guarantee the activation email was sent. Verify authService.register triggers the backend to send the activation email and handle/report server failures (e.g. show the server message if email delivery fails).
| onSubmit={({ name, email, password }, formikHelpers) => { | ||
| formikHelpers.setSubmitting(true); |
There was a problem hiding this comment.
You call formikHelpers.setSubmitting(true) at the top of onSubmit. When using Formik, if onSubmit returns a Promise you can let Formik manage isSubmitting automatically; if you keep the manual calls that's fine, but it's redundant — consider returning the Promise from authService.register(...) instead and let Formik manage submitting state to simplify the code.
| function requestEmailChange(data) { | ||
| return httpClient.patch('/profile/request-email-change', data); |
There was a problem hiding this comment.
requestEmailChange accepts a generic data object. The checklist requires that changing an email requires typing the password and confirmation of the new email. Ensure that when calling this function you pass both password and the newEmail (or email) fields — e.g. { newEmail, password } — and that server-side starts the confirmation flow (notify old email, send confirmation to new email). Consider documenting the expected shape here for clarity.
| function confirmEmailChange(activationToken) { | ||
| return httpClient.get(`/profile/confirm-email-change/${activationToken}`); |
There was a problem hiding this comment.
confirmEmailChange uses a GET request to /profile/confirm-email-change/${activationToken} which places the activation token in the URL. This works, but be aware that tokens in URLs can be logged in server logs or browser history. If your backend supports it, consider using POST with the token in the body to reduce exposure (optional improvement).
| function resetPassword(activationToken, password) { | ||
| return httpClient.post('/reset-password', { activationToken, password }); |
There was a problem hiding this comment.
resetPassword posts { activationToken, password } to /reset-password. This is fine and matches a typical flow, but for consistency and clarity you might prefer the function to accept a single object parameter (e.g. resetPassword({ activationToken, password })) like updatePassword does. This reduces caller confusion about argument order and makes future extension easier.
| @@ -0,0 +1,17 @@ | |||
| import 'dotenv/config'; | |||
| import { client } from './src/utils/db.js'; | |||
| import { User } from './src/models/User.js'; | |||
There was a problem hiding this comment.
User is imported but not referenced. If the import is intentional to register model definitions with the Sequelize instance, add a short comment explaining that; otherwise remove the unused import to avoid confusion.
| import 'dotenv/config'; | ||
| import { client } from './src/utils/db.js'; | ||
| import { User } from './src/models/User.js'; | ||
| import { Token } from './src/models/Token.js'; |
There was a problem hiding this comment.
Token is imported but not referenced. Same as above — either remove it or add a comment that importing the model registers it with client so tables are created/altered during sync.
|
|
||
| async function init() { | ||
| try { | ||
| await client.sync({ alter: true }); |
There was a problem hiding this comment.
client.sync({ alter: true }) will attempt to alter existing tables to match models rather than fully recreate them. The log below says "Database tables recreated!" which is misleading. If you intend to fully recreate tables use force: true (destructive) or change the message to reflect that tables were synchronized/updated.
| try { | ||
| await client.sync({ alter: true }); | ||
| console.log('Database tables recreated!'); | ||
| process.exit(0); |
There was a problem hiding this comment.
You call process.exit(0) immediately after sync; consider closing the DB connection gracefully first. For Sequelize use await client.close() (or the appropriate API) before exiting to ensure all connections are released. Do the same in the catch block before exiting with an error code.
| refreshToken: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, |
There was a problem hiding this comment.
Consider making refreshToken unique to prevent storing duplicate tokens for different users or accidental duplicates. Add unique: true to the column definition so the DB enforces it.
src/models/Token.js
Outdated
| Token.belongsTo(User); | ||
| User.hasOne(Token); |
There was a problem hiding this comment.
Be explicit about the association options. Specify the foreign key and cascade behavior to ensure a Token is removed when its User is deleted, for example: Token.belongsTo(User, { foreignKey: 'userId', onDelete: 'CASCADE' }) and User.hasOne(Token, { foreignKey: 'userId' }). This avoids orphaned tokens if a user is removed.
src/services/jwt.service.js
Outdated
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { | ||
| expiresIn: '5s', |
There was a problem hiding this comment.
expiresIn: '5s' is extremely short and will cause frequent refresh requests and poor UX. Use a more realistic access token lifetime (e.g. '15m', '1h') or read the value from an env var like process.env.JWT_EXPIRES_IN so it can be tuned without code changes.
src/services/jwt.service.js
Outdated
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| function sign(user) { | ||
| const token = jwt.sign(user, process.env.JWT_KEY, { |
There was a problem hiding this comment.
jwt.sign(user, ...) signs the entire user object. Prefer signing a minimal payload (for example { id: user.id, email: user.email }) to reduce token size and avoid leaking unnecessary fields in tokens.
src/services/jwt.service.js
Outdated
| } | ||
|
|
||
| function signRefresh(user) { | ||
| const token = jwt.sign(user, process.env.JWT_REFRESH_KEY); |
There was a problem hiding this comment.
signRefresh currently issues a refresh token with no expiresIn. While refresh tokens are typically longer-lived, consider adding an explicit expiry (e.g. from process.env.JWT_REFRESH_EXPIRES_IN) or implementing rotation/blacklisting logic to limit lifetime and improve security.
| function verify(token) { | ||
| try { | ||
| return jwt.verify(token, process.env.JWT_KEY); | ||
| } catch (err) { |
There was a problem hiding this comment.
verify/verifyRefresh return null on error which is fine, but swallowing errors can make debugging harder. Consider logging the verification error (or include a debug-level log) so invalid token issues are easier to investigate in development/test environments.
| @@ -0,0 +1,9 @@ | |||
| import { Sequelize } from 'sequelize'; | |||
|
|
|||
| export const client = new Sequelize({ | |||
There was a problem hiding this comment.
You instantiate Sequelize here — consider adding common options for production robustness such as logging: false (to silence SQL logs) and pool settings. If your DB is hosted (Heroku, managed Postgres) you may also need dialectOptions.ssl to allow TLS connections.
src/utils/db.js
Outdated
| 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 config relies on environment variables that may be undefined at runtime. Consider validating required env vars at startup or supplying sensible defaults, so the app fails fast with a clear error instead of producing a vague DB connection failure later.
| username: process.env.DB_USER, | ||
| password: process.env.DB_PASSWORD, | ||
| database: process.env.DB_DATABASE, | ||
| dialect: 'postgres', |
There was a problem hiding this comment.
If your Postgres instance uses a non-default port include port: process.env.DB_PORT here. Without it Sequelize will use the default Postgres port (5432), which may be incorrect for some deployments.
.gitignore
Outdated
| # env files | ||
| *.env | ||
| .env* | ||
| .env |
There was a problem hiding this comment.
Good to ignore .env. To avoid accidentally committing env variants, consider ignoring all variants and client-side env files too (for example add .env* and client/.env* to the file) so .env.local, .env.production, etc. are also covered.
client/src/App.jsx
Outdated
| onClick={() => { | ||
| logout() | ||
| .then(() => { | ||
| navigate('/'); |
There was a problem hiding this comment.
Logout redirect: the logout handler navigates to '/' after sign-out, but the checklist requires "Redirect to login after logging out." Consider changing this to navigate('/login') (and ensure logout clears auth state).
client/src/App.jsx
Outdated
| <section className="section"> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Registration should be available only to non-authenticated users per the requirements. Currently the registration route is public; add a guard (e.g. a RequireNoAuth wrapper or redirect inside the RegistrationPage when user is authenticated) to prevent access for logged-in users. See checklist: registration only non-auth.
| <Route | ||
| path="activate/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> |
There was a problem hiding this comment.
Non-auth-only pages: Activation page should be accessible only to non-authenticated users. Consider protecting this route for logged-in users (or force logout then process activation), and ensure the UX redirects to /profile after successful activation as required by the task.
client/src/App.jsx
Outdated
| path="confirm-email-change/:activationToken" | ||
| element={<EmailConfirmPage />} | ||
| /> | ||
| <Route path="login" element={<LoginPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Login page should be available only to non-authenticated users. Add a guard so logged-in users cannot access /login (or redirect them immediately to /profile).
client/src/App.jsx
Outdated
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="reset-password/:activationToken" element={<ResetPasswordPage />} /> |
There was a problem hiding this comment.
Non-auth-only pages: Forgot-password and Reset-password routes must be only for non-authenticated users per the checklist. Add a guard or redirect for authenticated users so these pages can't be used while logged in.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
Missing 404 route: the app needs a catch-all NotFound route (e.g. <Route path="*" element={} />) so unknown URLs show a 404 page as required by the task. Add this route inside (before closing).
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [user, setUser] = useState(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked initial value should start as false so the app can show a Loader while checkAuth is running. Currently it's true and the app skips the loading state immediately (App checks if (!isChecked) return <Loader/>). Initialize with useState(false) and setChecked(true) after checkAuth completes.
| async function activate(activationToken) { | ||
| // const { accessToken, user } = await authService.activate(activationToken); | ||
| await authService.activate(activationToken); | ||
| // accessTokenService.save(accessToken); | ||
| // setUser(user); | ||
| } |
There was a problem hiding this comment.
activate() does not set an access token or the user after a successful activation. The requirements state the user must be activated after email confirmation and redirected to Profile. Either have the backend return { accessToken, user } from the activation endpoint and uncomment/save the token and setUser here, or call authService.refresh() (or login) after activation to obtain tokens and then set the user and navigate to /profile. Right now the commented code shows the intended behavior but it's not executed.
| async function checkAuth() { | ||
| try { | ||
| const { accessToken, user } = await authService.refresh(); | ||
|
|
||
| accessTokenService.save(accessToken); | ||
| setUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); | ||
| } finally { | ||
| setChecked(true); |
There was a problem hiding this comment.
checkAuth stores accessToken and sets user — good. Note: because isChecked was initialized to true, the loader won't appear while this runs (see prior comment). Keep the try/catch as is, but ensure the initial isChecked change (above) so the check sequence is visible to users.
| const value = useMemo( | ||
| () => ({ | ||
| isChecked, | ||
| user, | ||
| checkAuth, | ||
| activate, | ||
| login, | ||
| logout, | ||
| updateUser | ||
| }), | ||
| [user, isChecked, updateUser], |
There was a problem hiding this comment.
The value passed to useMemo includes updateUser in the dependency array but updateUser is a new function each render. This will cause the memo to be recomputed every render. Consider stabilizing updateUser with useCallback or removing it from the deps so the context value is stable across renders.
client/src/hooks/usePageError.js
Outdated
| @@ -0,0 +1,21 @@ | |||
| import { useEffect, useState } from 'react'; | |||
|
|
|||
| export const usePageError = initialError => { | |||
There was a problem hiding this comment.
Consider giving the initialError parameter a default value to avoid undefined when the hook is called without arguments (App.jsx calls usePageError() with no argument). For example: export const usePageError = (initialError = '') => { ... }. This makes intent explicit and avoids callers having to pass an empty string.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
There was a problem hiding this comment.
Ensure createClient is exported from ./index.js and returns an axios-like instance; this file depends on that implementation. If createClient is missing or returns a different shape, authClient will fail. (Used by client services such as authService.)
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
|
|
|||
| export const authClient = createClient(); | |||
There was a problem hiding this comment.
This line instantiates the client and is fine. Keep this close to the createClient import so interceptors are applied to the right instance.
client/src/http/authClient.js
Outdated
|
|
||
| export const authClient = createClient(); | ||
|
|
||
| authClient.interceptors.response.use(res => res.data); |
There was a problem hiding this comment.
Currently you only add a success interceptor that returns res.data. Consider adding an error interceptor (second parameter to interceptors.response.use) to normalize or re-throw errors in a predictable way, e.g. forward error with Promise.reject(error) or map to a consistent error shape so components receive expected error.response data.
client/src/pages/LoginPage.jsx
Outdated
| onSubmit={({ email, password }) => { | ||
| return login({ email, password }) | ||
| .then(() => { | ||
| navigate(location.state?.from?.pathname || '/'); |
There was a problem hiding this comment.
Redirect target after login should lead to Profile by default per the checklist. Change the fallback in the navigation to '/profile' so users are redirected to their profile after successful login (unless a specific 'from' location is required).
client/src/pages/LoginPage.jsx
Outdated
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { login } = useContext(AuthContext); |
There was a problem hiding this comment.
This page must be available only to non-authenticated users. Right now LoginPage uses only login from AuthContext. Pull user (and isChecked if needed) from AuthContext and, when user is present, immediately redirect to /profile (or render a ). This prevents logged-in users from accessing the login page.
|
|
||
| export const LoginPage = () => { | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); |
There was a problem hiding this comment.
Small improvement: consider making the redirect check conditional on isChecked (auth-check finished) to avoid flicker — e.g., wait for isChecked true then redirect if user exists. You can get isChecked from AuthContext together with user.
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
You need to redirect the user to the Profile page after successful activation. Add navigation here (e.g. useNavigate and navigate('/profile')) or call checkAuth() and then navigate. Right now the page only shows a success message but does not redirect as required by the checklist.
|
@mateacademy-ai-mentor I have addressed your feedback and made a new commit. Please re-review |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback, especially on the authentication redirects and route guards! I'm approving this submission as an educational override to help you move forward, but there are a few critical issues left to resolve to make the application fully functional.
On the frontend, several pages will crash due to undefined variables:
AccountActivationPage.jsx:setResultis not defined; you likely meantsetError.LoginPage.jsx: TheisCheckedvariable fromAuthContextneeds to be destructured.EmailConfirmationPage.jsx: TheisLoadingstate variable is used but never declared.
On the backend, please address these key points:
- In
src/controllers/auth.controller.js, the cookie option should behttpOnly(camelCase), and you're missing anawaitwhen callingtokenService.getByToken. - In
src/routes/auth.route.jsandsrc/routes/profile.route.js, several asynchronous controller functions are not wrapped with yourcatchErrorutility, which could crash the server.
Fixing these will make your application much more stable. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| useEffect(() => { | ||
| if (!activationToken) { | ||
| setResult('Invalid or missing confirmation link.'); |
There was a problem hiding this comment.
The function setResult is not defined in this component, which will cause a runtime error if activationToken is missing. Did you mean to use setError instead?
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { user, login } = useContext(AuthContext); |
There was a problem hiding this comment.
The isChecked variable is used in the if conditions below, but it hasn't been destructured from the AuthContext. You need to add isChecked to the destructuring assignment here to avoid a ReferenceError.
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) | ||
| .then(() => setResult('Success! Your email has been updated.')) | ||
| .catch(() => setResult('Error: Link is invalid or expired.')); | ||
| }, [activationToken]); |
There was a problem hiding this comment.
The useEffect hook correctly calls the service to confirm the email change. However, based on the requirements and previous feedback, after a successful confirmation, the user should be logged in and redirected to their profile page. Currently, this implementation only sets a success message and provides a link.
Consider updating this flow to be similar to account activation: the service call should return the user's data and tokens, which you can then use to update the AuthContext and navigate to /profile.
| return ( | ||
| <div className="container has-text-centered mt-6"> | ||
| <div className="box"> | ||
| {isLoading ? ( |
There was a problem hiding this comment.
The isLoading variable is used here but it has not been defined within the component. This will cause a runtime error and prevent the page from rendering. You'll need to declare it as a state variable, for example: const [isLoading, setIsLoading] = useState(true);, and then update its value to false after the API call in your useEffect has completed.
| const { refreshToken } = req.cookies; | ||
|
|
||
| const userData = jwtService.verifyRefresh(refreshToken); | ||
| const token = tokenService.getByToken(refreshToken); |
There was a problem hiding this comment.
The tokenService.getByToken function returns a promise. You need to use await here to resolve the promise and get the actual token object from the database. Without it, the token variable will be a pending promise, and the check on line 103 will not function correctly.
|
|
||
| res.cookie('refreshToken', refreshToken, { | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| HttpOnly: true, |
There was a problem hiding this comment.
The property name for setting an HTTP-only cookie should be httpOnly (camelCase), not HttpOnly (PascalCase). While some servers or clients might be lenient, the standard specifies camel case, and using the wrong casing can lead to the cookie not being set correctly.
| const confirmEmailChange = async (req, res) => { | ||
| const { activationToken } = req.params; | ||
|
|
||
| const user = await User.findOne({ | ||
| where: { | ||
| activationToken, | ||
| pendingEmail: { [Op.ne]: null }, | ||
| }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.badRequest('Link is invalid or has already been used.'); | ||
| } | ||
|
|
||
| await user.update({ | ||
| email: user.pendingEmail, | ||
| pendingEmail: null, | ||
| activationToken: null, | ||
| }); | ||
|
|
||
| res.send({ message: 'Email updated successfully!' }); | ||
| }; |
There was a problem hiding this comment.
According to the requirements from the previous review, after a successful email change confirmation, the user should be logged in and redirected to their profile. Currently, this controller only updates the email and sends a success message. To meet the requirement, you should call generateTokens(res, user) after successfully updating the user, similar to how it's done in the login controller. This will provide the client with the necessary tokens to log the user in automatically.
| authRouter.post('/forgot-password', authController.forgotPassword); | ||
| authRouter.post('/reset-password', authController.resetPassword); |
There was a problem hiding this comment.
The forgotPassword and resetPassword controller functions are asynchronous and can throw errors. To prevent unhandled promise rejections that could crash your server, you should wrap them with your catchError utility, just as you've done for the other routes in this file.
| profileRoute.patch( | ||
| '/update-name', | ||
| authMiddleware, | ||
| profileController.updateName, | ||
| ); | ||
|
|
||
| profileRoute.patch( | ||
| '/update-password', | ||
| authMiddleware, | ||
| profileController.updatePassword, | ||
| ); | ||
|
|
||
| profileRoute.patch( | ||
| '/request-email-change', | ||
| authMiddleware, | ||
| profileController.requestEmailChange, | ||
| ); | ||
|
|
||
| profileRoute.get( | ||
| '/confirm-email-change/:activationToken', | ||
| authController.confirmEmailChange, |
There was a problem hiding this comment.
All of the controller functions used in this file (updateName, updatePassword, requestEmailChange, and confirmEmailChange) are asynchronous. To prevent unhandled promise rejections from crashing your server, you should wrap each of them with the catchError utility function, just as you've done in other route files.
No description provided.