Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical blockers that mean the task requirements are not yet fully met. Most importantly, the auth flows around activation/login/reset are incomplete: after activation the user is not actually authenticated or redirected to /profile (AuthContext activate doesn’t save token/user, and AccountActivationPage does not navigate), login currently redirects back or to / instead of always taking the user to their Profile, and logout goes to / instead of /login as required. Several pages that must be only for non‑authenticated users (registration, login, activation, forgot password, reset password) are either unguarded or enforce this in a surprising way by force‑logging‑out on mount, instead of using your RequireNonAuth guard and redirecting authenticated users to /profile. On the server side, the refresh flow has a real bug where tokenService.getByToken(refreshToken) is used without await, and the refresh cookie uses HttpOnly instead of httpOnly, which can break token refresh and the security guarantees around the refresh cookie. The app is also missing a catch‑all 404 route on the client and a 404 handler on the server, so unknown paths do not clearly return the required “Not Found” behavior.
The critical fixes I recommend before this can pass are:
- Make activation log the user in and redirect to
/profile(either by having the server return tokens in theactivateendpoint and wiring them intoAuthContext.activate, or by following activation withcheckAuth/login) and updateAccountActivationPageto navigate to/profileon success. - Enforce the “only non‑authenticated” requirement by wrapping registration, login, activation, forgot‑password, and reset‑password routes with
RequireNonAuth(or equivalent) and remove force‑logout side effects from those pages; ensure authenticated users are consistently redirected to/profile. - Fix the refresh flow by awaiting
tokenService.getByToken(refreshToken)and correcting the cookie option tohttpOnly: true, and harden the HTTP client’s 401 interceptor to safely handle network errors. - Add a client
Route path='*'404 page and a server‑side 404 handler before the error middleware so unknown routes are handled explicitly.
Aside from these, much of your implementation is solid: registration, profile updates, email change, and most client–server integrations are in place. Once you address the blocking auth‑flow and error‑handling issues, your solution should align well with the task 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
| # env files | ||
| *.env | ||
| .env* | ||
| .env |
There was a problem hiding this comment.
Consider ignoring other environment-file variants to avoid accidental commits. For example use a pattern like .env* or explicitly add .env.local, .env.production, .env.test so all env variants are covered.
| {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 is mistyped: it includes a trailing colon in the property name. The intended rule is implicit-arrow-linebreak (without a colon) — with the current key ESLint will not match/apply this rule. Remove the extra colon from the property name.
| { | ||
| 'files': ['**/*.spec.jsx'], | ||
| 'rules': { | ||
| 'react/jsx-filename-extension': ['off'], |
There was a problem hiding this comment.
You define react/jsx-filename-extension inside the overrides as ['off'] and again later at top-level as [1, {extensions: ['.jsx']}]. This duplicate may be confusing — the last definition wins. If the intent is to disable the rule for test files only, keep the override but use the standard severity format (e.g., 'off' instead of ['off']) and remove or clarify the top-level setting.
| 'jsx-a11y/label-has-associated-control': ["error", { | ||
| assert: "either", | ||
| }], | ||
| 'jsx-a11y/label-has-for': [2, { |
There was a problem hiding this comment.
The rule jsx-a11y/label-has-for is deprecated in newer versions of eslint-plugin-jsx-a11y in favor of label-has-associated-control. Consider removing or replacing this deprecated rule to avoid plugin warnings/errors.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
There was a problem hiding this comment.
These patterns have a leading space ( .idea) so Git will treat the space as part of the filename and the actual .idea directory will not be ignored. Remove the leading space so the entry is .idea.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
| .vscode | |||
There was a problem hiding this comment.
Same issue as line 1: leading space before .vscode prevents correct matching. Remove the leading space so the entry is .vscode.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
| .vscode | |||
| build | |||
There was a problem hiding this comment.
Leading space before build — remove the space so build is ignored as intended.
| .idea | ||
| .vscode | ||
| build | ||
| dist |
There was a problem hiding this comment.
Leading space before dist — remove the space so dist is ignored as intended.
| .vscode | ||
| build | ||
| dist | ||
| node_modules |
There was a problem hiding this comment.
Leading space before node_modules — remove the space so node_modules is ignored (this is important to avoid committing dependencies).
| build | ||
| dist | ||
| node_modules | ||
| .DS_Store |
There was a problem hiding this comment.
Leading space before .DS_Store — remove the space so .DS_Store files are ignored on macOS.
| useEffect(() => { | ||
| checkAuth(); | ||
| }, []); |
There was a problem hiding this comment.
The effect calls checkAuth() but checkAuth is not listed in the dependency array. This can cause a lint warning or stale closure if checkAuth changes. Either include checkAuth in the dependency array or ensure checkAuth is stable (e.g., memoized in context).
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> | ||
| <Route | ||
| path="activate/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> | ||
| <Route | ||
| path="confirm-email-change/:activationToken" | ||
| element={<EmailConfirmPage />} | ||
| /> | ||
| <Route path="login" element={<LoginPage />} /> | ||
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="reset-password/:activationToken" element={<ResetPasswordPage />} /> |
There was a problem hiding this comment.
Routes for non-authenticated flows — registration, activation, email-confirmation, login, forgot-password, reset-password — are defined here without any guard to restrict access to non-authenticated users. The task requires these pages to be accessible only to non-authenticated users. Add a RequireNoAuth wrapper or perform redirects inside these pages to enforce “only non authenticated” behavior.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
There is no catch-all 404 route (e.g., path='*') defined. The task requires the application to show a 404 for all other pages. Add a Route path='*' element={<NotFoundPage/>} (and create the NotFoundPage) so unknown paths show a 404.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="users" element={<UsersPage />} /> | ||
| </Route> | ||
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> |
There was a problem hiding this comment.
You have two separate parent routes using path='/' with element={<RequireAuth/>} wrapping users and profile. This works but is redundant — consider consolidating protected child routes under a single RequireAuth parent to simplify routing structure.
| aria-label="main navigation" | ||
| > | ||
| <div className="navbar-start"> | ||
| <NavLink to="/" className="navbar-item"> |
There was a problem hiding this comment.
Minor UX: NavLink to='/' for Home may remain active on child routes. Consider adding the end prop to the root NavLink to ensure it’s only active on the exact / path.
| 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.
The activate implementation only calls authService.activate but does not save the returned accessToken or set the user. That prevents the app from becoming authenticated after activation and will break the requirement to redirect to Profile after activation. Restore the logic to save the token (via accessTokenService.save) and call setUser using the response from authService.activate (the commented lines show the intended behavior).
| // accessTokenService.save(accessToken); | ||
| // setUser(user); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is no useEffect in this provider to call checkAuth on mount. If checkAuth is not invoked elsewhere, the app will never refresh the session and isChecked/user will remain stale. Add useEffect(() => { checkAuth(); }, []) (or ensure checkAuth is called from a top-level component) so the provider actually verifies existing sessions on app start.
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [user, setUser] = useState(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked is initialized to true. That indicates auth has already been checked before checkAuth runs and can cause UI to skip loading/activation flows. Initialize it to false so the provider indicates verification is pending until checkAuth sets it to true in the finally block.
| accessTokenService.save(accessToken); | ||
| setUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); |
There was a problem hiding this comment.
Minor: console message is misspelled ('User is not authentincated'). Fix the typo to 'User is not authenticated' to keep logs clear.
| const value = useMemo( | ||
| () => ({ | ||
| isChecked, | ||
| user, | ||
| checkAuth, | ||
| activate, | ||
| login, | ||
| logout, | ||
| updateUser | ||
| }), | ||
| [user, isChecked, updateUser], |
There was a problem hiding this comment.
useMemo dependencies include updateUser, which is re-created every render (it isn't wrapped in useCallback) — that makes the memo less effective. Consider wrapping functions (checkAuth, activate, login, logout, updateUser) in useCallback or remove updateUser from deps if you intentionally want to recreate the memo when user changes only.
| @@ -0,0 +1,19 @@ | |||
| import { Navigate, Outlet, useLocation } from 'react-router-dom'; | |||
| import React, { useContext } from 'react'; | |||
| import { AuthContext } from './AuthContext.jsx'; | |||
There was a problem hiding this comment.
Import path uses the .jsx extension here while other files import AuthContext without the extension. This is not a functional error but creates inconsistent imports across the project. Consider using the same import style everywhere for clarity (e.g., ./AuthContext).
| import { Loader } from './Loader.jsx'; | ||
|
|
||
| export const RequireAuth = ({ children }) => { | ||
| const { isChecked, user } = useContext(AuthContext); |
There was a problem hiding this comment.
The component correctly reads isChecked and user from context, but if the user object can be nullish or in an unexpected shape you may want to defensively ensure user is the expected object shape before using it elsewhere. This is a light suggestion — current code is fine if the context guarantees user is either null or a valid user object.
| return <Loader />; | ||
| } | ||
|
|
||
| if (!user) { |
There was a problem hiding this comment.
When an unauthenticated visitor is detected you redirect to /login. The task requires that users who are authenticated but not active should be asked to activate their email. This component currently does not inspect an active flag on user. If you want protected routes to require an activated account, add a check like if (user && !user.isActive) return <Navigate to="/activate" .../> (or redirect to the appropriate activation page) so inactive-but-authenticated users cannot access protected pages.
| return <Navigate to="/login" state={{ from: location }} replace />; | ||
| } | ||
|
|
||
| return children || <Outlet />; |
There was a problem hiding this comment.
Returning children || <Outlet /> is fine and supports both usage styles. No change required, but if your app only uses nested routes you can simplify to just <Outlet /> to avoid handling both patterns unless you intentionally allow children.
| if (user) { | ||
| return <Navigate to="/" replace />; |
There was a problem hiding this comment.
Redirects authenticated users to /. According to the app requirements, authenticated users should end up on the Profile page after login/activation — consider redirecting to /profile instead of / so protected users land on their profile.
| @@ -0,0 +1,18 @@ | |||
| import { Navigate, Outlet } from 'react-router-dom'; | |||
| import React, { useContext } from 'react'; | |||
| import { AuthContext } from './AuthContext.jsx'; | |||
There was a problem hiding this comment.
Import path includes the .jsx extension while other files (e.g., App.jsx) import AuthContext without the extension. This works, but you may want to keep imports consistent across the project (either always include extensions or omit them) to avoid confusion.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
There was a problem hiding this comment.
Import is fine — createClient is provided in client/src/http/index.js and sets baseURL and withCredentials: true, which is required for cookie-based auth flows.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
|
|
|||
| export const authClient = createClient(); | |||
There was a problem hiding this comment.
Exporting the axios instance produced by createClient() is correct — this authClient is used by authService to call auth endpoints.
|
|
||
| export const authClient = createClient(); | ||
|
|
||
| authClient.interceptors.response.use(res => res.data); |
There was a problem hiding this comment.
Consider adding an explicit error handler to the response interceptor so errors are forwarded as rejected Promises, e.g. authClient.interceptors.response.use(res => res.data, err => Promise.reject(err)). Right now it relies on axios default behavior; being explicit improves readability and avoids ambiguity.
| httpClient.interceptors.response.use(onResponseSuccess, onResponseError); | ||
|
|
||
| function onRequest(request) { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
Prefer using the accessTokenService getter for consistency rather than calling localStorage directly. This keeps token storage logic centralized and matches how other modules retrieve/save tokens (see accessTokenService).
| async function onResponseError(error) { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response.status !== 401 || originalRequest._retry) { |
There was a problem hiding this comment.
This condition assumes error.response exists. On network errors error.response can be undefined and accessing error.response.status will throw — causing the app to crash. Guard the access, e.g. if (!error.response || error.response.status !== 401 || originalRequest._retry) { throw error; } so network errors are re-thrown safely.
| } | ||
|
|
||
| async function onResponseError(error) { | ||
| const originalRequest = error.config; |
There was a problem hiding this comment.
originalRequest may be undefined in some abnormal error shapes. Ensure originalRequest is present before setting _retry or re-sending it, or short-circuit earlier. A safe pattern: const originalRequest = error.config || {}; and then check existence before mutating/using.
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { accessToken } = await authService.refresh(); |
There was a problem hiding this comment.
This function calls authService.refresh() to obtain a new access token — ensure the refresh endpoint returns { accessToken } as expected. The authService implementation used elsewhere shows refresh() is defined and used for this purpose (see authService).
| const [error, setError] = useState(''); | ||
| const [done, setDone] = useState(false); | ||
|
|
||
| const { activate, logout } = useContext(AuthContext); |
There was a problem hiding this comment.
You're only destructuring activate and logout from AuthContext — to enforce “only non-authenticated” access you should also read user here and use useNavigate to redirect authenticated users to /profile instead of calling logout() implicitly. This keeps the activation page reserved for non-authenticated flows (per checklist).
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); |
There was a problem hiding this comment.
Calling await logout() here force-logs-out any current user. The checklist requires the Activation page to be accessible only to non-authenticated users — instead of logging out, consider redirecting existing authenticated users to the Profile page or preventing access to this page via routing/guard. Forcing logout may produce surprising UX.
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
After successful activation you must redirect to Profile (navigate('/profile')). Add a navigate call in the success path. Note: currently AuthContext.activate does not save the access token or set user, so redirecting to a protected Profile will only work if activate (or a subsequent checkAuth/login) sets authentication (see AuthContext.jsx). Update activate or call checkAuth()/login after activation so Profile becomes accessible.
| }; | ||
|
|
||
| processActivation(); | ||
| }, [activationToken]); |
There was a problem hiding this comment.
The effect dependency array only includes activationToken. Add activate, logout, and navigate (if you use it) to the dependencies or ensure those functions are stable (memoized) to avoid stale-closure/lint issues.
| {error ? ( | ||
| <p className="notification is-danger is-light">{error}</p> | ||
| ) : ( | ||
| <p className="notification is-success is-light"> |
There was a problem hiding this comment.
If activation can fail you currently show an error message — consider also providing a link to Login or Home on success/failure so users have a clear next step after activation completes.
| @@ -0,0 +1,8 @@ | |||
| import axios from 'axios'; | |||
There was a problem hiding this comment.
Import is correct — axios is required to create the HTTP client instance used throughout the app.
| @@ -0,0 +1,8 @@ | |||
| import axios from 'axios'; | |||
|
|
|||
| export function createClient() { | |||
There was a problem hiding this comment.
Exporting a factory createClient() is fine and allows creating multiple differently-configured clients if needed. This matches how other files import it (e.g., authClient and httpClient).
|
|
||
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', |
There was a problem hiding this comment.
Hardcoded baseURL works for local development but reduces flexibility. Consider using an environment variable (e.g., import.meta.env.VITE_API_BASE_URL or process.env.REACT_APP_API_URL) so the app can target different backends in dev/staging/production without code changes.
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', | ||
| withCredentials: true, |
There was a problem hiding this comment.
withCredentials: true is appropriate for cookie-based authentication flows (ensures cookies are sent). Keep this if the backend uses cookies for refresh/access tokens; otherwise document the choice.
| import { userService } from '../services/userService'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| export const ForgotPasswordPage = () => { |
There was a problem hiding this comment.
This component implements the email request and the “email sent” page (it calls userService.forgotPassword) which satisfies the checklist items “Ask for an email” and “Show email sent page”. However, the password-reset flow must be available only to non-authenticated users. Use AuthContext (or a RequireNoAuth wrapper) here to redirect authenticated users to /profile or otherwise prevent access from signed-in users. AuthContext is available in the project and exposes user/auth helpers.
| <Form> | ||
| <div className="field"> | ||
| <label className="label">Email</label> | ||
| <Field |
There was a problem hiding this comment.
There is no client-side validation for the email field. Add a simple Formik validate function or use an HTML5 pattern to avoid sending empty/invalid emails (prevents unnecessary API calls and improves UX).
|
|
||
| {errorMsg && <p className="help is-danger mb-3">{errorMsg}</p>} | ||
|
|
||
| <button |
There was a problem hiding this comment.
You show a loading state via class but the submit button is still clickable. Add disabled={isSubmitting} to the <button> to prevent duplicate submissions while the request is in-flight.
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { login } = useContext(AuthContext); |
There was a problem hiding this comment.
Destructure user from AuthContext and prevent authenticated users from using this page. The task requires the login page to be accessible only to non-authenticated users; redirect authenticated users (e.g., if (user) navigate('/profile')).
| onSubmit={({ email, password }) => { | ||
| return login({ email, password }) | ||
| .then(() => { | ||
| navigate(location.state?.from?.pathname || '/'); |
There was a problem hiding this comment.
After successful login the app must redirect to Profile. Current code navigates to the previous pathname or '/'. Update this to navigate('/profile') (or ensure you always navigate to /profile after login) to satisfy the checklist requirement.
| password: '', | ||
| }} | ||
| validateOnMount={true} | ||
| onSubmit={({ email, password }) => { |
There was a problem hiding this comment.
Because you use location.state?.from?.pathname as the post-login target, this may override the requirement to always redirect to Profile. Decide desired behavior: either always redirect to /profile per checklist, or preserve return-to behavior but still satisfy the checklist by navigating to /profile when no return path is required.
| } | ||
| } | ||
|
|
||
| export const LoginPage = () => { |
There was a problem hiding this comment.
If you implement a redirect for already-authenticated users, add an effect like useEffect(() => { if (user) navigate('/profile'); }, [user, navigate]). Ensure navigate is included in the dependency array to avoid stale-closure/lint issues.
| @@ -0,0 +1,23 @@ | |||
| import React, { useEffect, useState } from 'react'; | |||
| import { useParams, Link } from 'react-router-dom'; | |||
| import { userService } from '../services/userService'; | |||
There was a problem hiding this comment.
If the user is currently logged in, the client-side user object will still show the old email after a successful confirmation. Consider reading AuthContext here and calling checkAuth() or updating the user in context so the UI reflects the new email immediately (the server updates email on confirm; see controller).
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) | ||
| .then(() => setResult('Success! Your email has been updated.')) |
There was a problem hiding this comment.
This effect updates result but provides no explicit loading state beyond the initial text. Consider setting an explicit isLoading state while the request is in-flight (disable UI actions / show spinner) to improve UX and avoid duplicate requests.
| return ( | ||
| <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.
On success you currently display a link to Login. Consider auto-redirecting after a short delay or conditionally redirecting to /profile when appropriate. Also, if you auto-redirect to profile, ensure the client auth state is refreshed (see first comment) so the profile shows the updated email.
| export const RegistrationPage = () => { | ||
| const [error, setError] = usePageError(''); | ||
| const [registered, setRegistered] = useState(false); |
There was a problem hiding this comment.
The registration page does not prevent an already-authenticated user from accessing /using the registration form. Per the task requirements the registration flow should be “only non-authenticated”. Consider importing AuthContext/useContext + useNavigate here and redirecting authenticated users to /profile, or wrap this route in a RequireNoAuth guard so authenticated users cannot access registration. This is required to meet the checklist item for registration being accessible only to non-authenticated users.
| 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 Sign up button’s disabled prop checks errors.email || errors.password but omits errors.name. This allows the form to be submitted when the name field is invalid. Update the condition to include name (e.g. disabled={isSubmitting || errors.name || errors.email || errors.password}) or use a generic check like Object.keys(errors).length > 0 to cover all validation errors.
| if (registered) { | ||
| return ( | ||
| <section className=""> | ||
| <h1 className="title">Check your email</h1> | ||
| <p>We have sent you an email with the activation link</p> | ||
| </section> | ||
| ); |
There was a problem hiding this comment.
After successful registration you show a simple “Check your email” message. Consider adding a link/button to Login (or auto-redirect after a short delay) so users have a clear next step. Also ensure this screen makes it explicit that an activation email was sent and next actions (login after activation). This improves UX and aligns with the activation-email requirement.
| import { userService } from '../services/userService.js'; | ||
|
|
||
| export const ProfilePage = () => { | ||
| const { user, updateUser } = useContext(AuthContext); |
There was a problem hiding this comment.
This component relies on user and updateUser from AuthContext. Make sure the profile route is protected with RequireAuth so only authenticated users can access this page (the app has RequireAuth for profile routes). If you rely on the route guard, document that here or assert user exists before rendering heavy UI.
| } catch (err) { | ||
| const errorMsg = err.response?.data?.message || 'Failed to update name'; |
There was a problem hiding this comment.
In the submitName catch you compute errorMsg but never set it into state or surface it to the user. Consider calling setNameMsg(errorMsg) or showing field-level errors so failures are visible to the user (currently failures are silent).
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { |
There was a problem hiding this comment.
The conditional in the password-change catch is unsafe and can throw when backendMsg is undefined. Change it to if (backendMsg && (backendMsg.includes('old password') || backendMsg.includes('at least'))) so you only call includes when backendMsg is present.
| <h3 className="subtitle">Change Password</h3> | ||
| <Formik | ||
| initialValues={{ oldPassword: '', newPassword: '', confirmPassword: '' }} | ||
| validate={values => { |
There was a problem hiding this comment.
The client-side validate for the password-change form checks equality and that new != old, but does not enforce a minimum length (server requires at least 6 chars). Add a values.newPassword.length < 6 check to provide immediate feedback and match server rules (improves UX).
| try { | ||
| await userService.requestEmailChange(values); |
There was a problem hiding this comment.
Email change request correctly calls userService.requestEmailChange(values) which triggers server-side behavior (pendingEmail + activationToken + email notifications). After confirmation the server updates the email; consider instructing the user in UI that the change is pending until they confirm the new email link sent by the server. See userService and server controller for details.
| useEffect(() => { | ||
| logout().catch(() => { | ||
| /* ignore if already logged out */ | ||
| }); |
There was a problem hiding this comment.
Forcing a logout on mount here (logout() inside useEffect) will abruptly sign out any authenticated user. The checklist requires this flow to be accessible only to non-authenticated users — instead of force-logging-out, restrict the route (or check user and navigate('/profile') if already authenticated). Forcing logout is surprising UX and may lose user state. See the task requirements.
| try { | ||
| await userService.resetPassword(activationToken, values.password); | ||
| alert('Password reset successful! Please log in with your new password.'); | ||
| navigate('/login'); |
There was a problem hiding this comment.
After successful reset you navigate('/login') immediately (after an alert). The requirements ask to “Show Success page with a link to login” — implement a dedicated success screen (or render a success state with a button/link to Login) instead of directly redirecting so users see the success message and can click the login link.
| import { AuthContext } from '../components/AuthContext'; | ||
|
|
||
| export const ResetPasswordPage = () => { | ||
| const { activationToken } = useParams(); |
There was a problem hiding this comment.
You read activationToken from params but do not validate its presence before showing the form. Consider checking activationToken and showing an error/invalid-link UI early (or disabling the form) to avoid sending requests with a missing token; the server returns a Bad Request for invalid tokens.
| const handleSubmit = async (values, { setSubmitting }) => { | ||
| try { | ||
| await userService.resetPassword(activationToken, values.password); | ||
| alert('Password reset successful! Please log in with your new password.'); |
There was a problem hiding this comment.
Using alert() for success/error feedback is brittle for UX — prefer rendering an inline success/error message or navigating to a success page with a link to Login (per requirements). This will produce a more consistent user experience and satisfy the “Success page” checklist item.
| // Matching validation | ||
| if (values.confirmPassword !== values.password) { | ||
| errors.confirmPassword = 'Passwords do not match'; | ||
| } |
There was a problem hiding this comment.
Validation correctly checks that confirmPassword equals password — this satisfies the requirement that the confirmation fields must be equal. Ensure any additional password rules required by the app (e.g., length or complexity) match the registration rules consistently.
| import React, { useEffect, useState } from 'react'; | ||
| import { usePageError } from '../hooks/usePageError.js'; | ||
| import { userService } from '../services/userService.js'; |
There was a problem hiding this comment.
The /users endpoint is protected on the server with authMiddleware and the client userService.getAll() is used here — good. Keep in mind that this page must remain accessible only to authenticated users (routing currently wraps the page with RequireAuth). See server route and client service for context.
| userService | ||
| .getAll() | ||
| .then(setUsers) | ||
| .catch(error => { | ||
| setError(error.message); | ||
| }); |
There was a problem hiding this comment.
The effect uses a promise chain; extract server-side error messages (if present) rather than only using error.message. Prefer setError(error.response?.data?.message || error.message) so users see meaningful messages from the backend when available.
| useEffect(() => { | ||
| userService | ||
| .getAll() | ||
| .then(setUsers) | ||
| .catch(error => { | ||
| setError(error.message); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Consider showing a loading state while the request is in-flight (e.g., isLoading state and a spinner or ‘Loading…’ text). Right now the UI will show an empty list until the request resolves which can be confusing.
| <ul> | ||
| {users.map(user => ( | ||
| <li key={user.id}>{user.email}</li> | ||
| ))} |
There was a problem hiding this comment.
When users is empty you render an empty
- . Provide a friendly fallback (e.g., “No users found”) so the UI communicates the empty state clearly to users.
| function get() { | ||
| return localStorage.getItem(key); |
There was a problem hiding this comment.
Accessing localStorage can throw in non-browser environments (SSR) or when storage is disabled. Consider wrapping calls in try/catch or checking typeof window !== 'undefined' before using localStorage to avoid runtime errors in edge cases.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical blockers that mean the task requirements are not yet fully met. Most importantly, the auth flows around activation/login/reset are incomplete: after activation the user is not actually authenticated or redirected to /profile (AuthContext activate doesn’t save token/user, and AccountActivationPage does not navigate), login currently redirects back or to / instead of always taking the user to their Profile, and logout goes to / instead of /login as required. Several pages that must be only for non‑authenticated users (registration, login, activation, forgot password, reset password) are either unguarded or enforce this in a surprising way by force‑logging‑out on mount, instead of using your RequireNonAuth guard and redirecting authenticated users to /profile. On the server side, the refresh flow has a real bug where tokenService.getByToken(refreshToken) is used without await, and the refresh cookie uses HttpOnly instead of httpOnly, which can break token refresh and the security guarantees around the refresh cookie. The app is also missing a catch‑all 404 route on the client and a 404 handler on the server, so unknown paths do not clearly return the required “Not Found” behavior.
The critical fixes I recommend before this can pass are:
- Make activation log the user in and redirect to
/profile(either by having the server return tokens in theactivateendpoint and wiring them intoAuthContext.activate, or by following activation withcheckAuth/login) and updateAccountActivationPageto navigate to/profileon success. - Enforce the “only non‑authenticated” requirement by wrapping registration, login, activation, forgot‑password, and reset‑password routes with
RequireNonAuth(or equivalent) and remove force‑logout side effects from those pages; ensure authenticated users are consistently redirected to/profile. - Fix the refresh flow by awaiting
tokenService.getByToken(refreshToken)and correcting the cookie option tohttpOnly: true, and harden the HTTP client’s 401 interceptor to safely handle network errors. - Add a client
Route path='*'404 page and a server‑side 404 handler before the error middleware so unknown routes are handled explicitly.
Aside from these, much of your implementation is solid: registration, profile updates, email change, and most client–server integrations are in place. Once you address the blocking auth‑flow and error‑handling issues, your solution should align well with the task 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
| # env files | ||
| *.env | ||
| .env* | ||
| .env |
There was a problem hiding this comment.
Consider ignoring other environment-file variants to avoid accidental commits. For example use a pattern like .env* or explicitly add .env.local, .env.production, .env.test so all env variants are covered.
| {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 is mistyped: it includes a trailing colon in the property name. The intended rule is implicit-arrow-linebreak (without a colon) — with the current key ESLint will not match/apply this rule. Remove the extra colon from the property name.
| { | ||
| 'files': ['**/*.spec.jsx'], | ||
| 'rules': { | ||
| 'react/jsx-filename-extension': ['off'], |
There was a problem hiding this comment.
You define react/jsx-filename-extension inside the overrides as ['off'] and again later at top-level as [1, {extensions: ['.jsx']}]. This duplicate may be confusing — the last definition wins. If the intent is to disable the rule for test files only, keep the override but use the standard severity format (e.g., 'off' instead of ['off']) and remove or clarify the top-level setting.
| 'jsx-a11y/label-has-associated-control': ["error", { | ||
| assert: "either", | ||
| }], | ||
| 'jsx-a11y/label-has-for': [2, { |
There was a problem hiding this comment.
The rule jsx-a11y/label-has-for is deprecated in newer versions of eslint-plugin-jsx-a11y in favor of label-has-associated-control. Consider removing or replacing this deprecated rule to avoid plugin warnings/errors.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
There was a problem hiding this comment.
These patterns have a leading space ( .idea) so Git will treat the space as part of the filename and the actual .idea directory will not be ignored. Remove the leading space so the entry is .idea.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
| .vscode | |||
There was a problem hiding this comment.
Same issue as line 1: leading space before .vscode prevents correct matching. Remove the leading space so the entry is .vscode.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
| .vscode | |||
| build | |||
There was a problem hiding this comment.
Leading space before build — remove the space so build is ignored as intended.
| .idea | ||
| .vscode | ||
| build | ||
| dist |
There was a problem hiding this comment.
Leading space before dist — remove the space so dist is ignored as intended.
| .vscode | ||
| build | ||
| dist | ||
| node_modules |
There was a problem hiding this comment.
Leading space before node_modules — remove the space so node_modules is ignored (this is important to avoid committing dependencies).
| build | ||
| dist | ||
| node_modules | ||
| .DS_Store |
There was a problem hiding this comment.
Leading space before .DS_Store — remove the space so .DS_Store files are ignored on macOS.
| useEffect(() => { | ||
| checkAuth(); | ||
| }, []); |
There was a problem hiding this comment.
The effect calls checkAuth() but checkAuth is not listed in the dependency array. This can cause a lint warning or stale closure if checkAuth changes. Either include checkAuth in the dependency array or ensure checkAuth is stable (e.g., memoized in context).
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> | ||
| <Route | ||
| path="activate/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> | ||
| <Route | ||
| path="confirm-email-change/:activationToken" | ||
| element={<EmailConfirmPage />} | ||
| /> | ||
| <Route path="login" element={<LoginPage />} /> | ||
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="reset-password/:activationToken" element={<ResetPasswordPage />} /> |
There was a problem hiding this comment.
Routes for non-authenticated flows — registration, activation, email-confirmation, login, forgot-password, reset-password — are defined here without any guard to restrict access to non-authenticated users. The task requires these pages to be accessible only to non-authenticated users. Add a RequireNoAuth wrapper or perform redirects inside these pages to enforce “only non authenticated” behavior.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
There is no catch-all 404 route (e.g., path='*') defined. The task requires the application to show a 404 for all other pages. Add a Route path='*' element={<NotFoundPage/>} (and create the NotFoundPage) so unknown paths show a 404.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="users" element={<UsersPage />} /> | ||
| </Route> | ||
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> |
There was a problem hiding this comment.
You have two separate parent routes using path='/' with element={<RequireAuth/>} wrapping users and profile. This works but is redundant — consider consolidating protected child routes under a single RequireAuth parent to simplify routing structure.
| aria-label="main navigation" | ||
| > | ||
| <div className="navbar-start"> | ||
| <NavLink to="/" className="navbar-item"> |
There was a problem hiding this comment.
Minor UX: NavLink to='/' for Home may remain active on child routes. Consider adding the end prop to the root NavLink to ensure it’s only active on the exact / path.
| 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.
The activate implementation only calls authService.activate but does not save the returned accessToken or set the user. That prevents the app from becoming authenticated after activation and will break the requirement to redirect to Profile after activation. Restore the logic to save the token (via accessTokenService.save) and call setUser using the response from authService.activate (the commented lines show the intended behavior).
| // accessTokenService.save(accessToken); | ||
| // setUser(user); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is no useEffect in this provider to call checkAuth on mount. If checkAuth is not invoked elsewhere, the app will never refresh the session and isChecked/user will remain stale. Add useEffect(() => { checkAuth(); }, []) (or ensure checkAuth is called from a top-level component) so the provider actually verifies existing sessions on app start.
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [user, setUser] = useState(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked is initialized to true. That indicates auth has already been checked before checkAuth runs and can cause UI to skip loading/activation flows. Initialize it to false so the provider indicates verification is pending until checkAuth sets it to true in the finally block.
| accessTokenService.save(accessToken); | ||
| setUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); |
There was a problem hiding this comment.
Minor: console message is misspelled ('User is not authentincated'). Fix the typo to 'User is not authenticated' to keep logs clear.
| const value = useMemo( | ||
| () => ({ | ||
| isChecked, | ||
| user, | ||
| checkAuth, | ||
| activate, | ||
| login, | ||
| logout, | ||
| updateUser | ||
| }), | ||
| [user, isChecked, updateUser], |
There was a problem hiding this comment.
useMemo dependencies include updateUser, which is re-created every render (it isn't wrapped in useCallback) — that makes the memo less effective. Consider wrapping functions (checkAuth, activate, login, logout, updateUser) in useCallback or remove updateUser from deps if you intentionally want to recreate the memo when user changes only.
| @@ -0,0 +1,19 @@ | |||
| import { Navigate, Outlet, useLocation } from 'react-router-dom'; | |||
| import React, { useContext } from 'react'; | |||
| import { AuthContext } from './AuthContext.jsx'; | |||
There was a problem hiding this comment.
Import path uses the .jsx extension here while other files import AuthContext without the extension. This is not a functional error but creates inconsistent imports across the project. Consider using the same import style everywhere for clarity (e.g., ./AuthContext).
| import { Loader } from './Loader.jsx'; | ||
|
|
||
| export const RequireAuth = ({ children }) => { | ||
| const { isChecked, user } = useContext(AuthContext); |
There was a problem hiding this comment.
The component correctly reads isChecked and user from context, but if the user object can be nullish or in an unexpected shape you may want to defensively ensure user is the expected object shape before using it elsewhere. This is a light suggestion — current code is fine if the context guarantees user is either null or a valid user object.
| return <Loader />; | ||
| } | ||
|
|
||
| if (!user) { |
There was a problem hiding this comment.
When an unauthenticated visitor is detected you redirect to /login. The task requires that users who are authenticated but not active should be asked to activate their email. This component currently does not inspect an active flag on user. If you want protected routes to require an activated account, add a check like if (user && !user.isActive) return <Navigate to="/activate" .../> (or redirect to the appropriate activation page) so inactive-but-authenticated users cannot access protected pages.
| return <Navigate to="/login" state={{ from: location }} replace />; | ||
| } | ||
|
|
||
| return children || <Outlet />; |
There was a problem hiding this comment.
Returning children || <Outlet /> is fine and supports both usage styles. No change required, but if your app only uses nested routes you can simplify to just <Outlet /> to avoid handling both patterns unless you intentionally allow children.
| if (user) { | ||
| return <Navigate to="/" replace />; |
There was a problem hiding this comment.
Redirects authenticated users to /. According to the app requirements, authenticated users should end up on the Profile page after login/activation — consider redirecting to /profile instead of / so protected users land on their profile.
| @@ -0,0 +1,18 @@ | |||
| import { Navigate, Outlet } from 'react-router-dom'; | |||
| import React, { useContext } from 'react'; | |||
| import { AuthContext } from './AuthContext.jsx'; | |||
There was a problem hiding this comment.
Import path includes the .jsx extension while other files (e.g., App.jsx) import AuthContext without the extension. This works, but you may want to keep imports consistent across the project (either always include extensions or omit them) to avoid confusion.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
There was a problem hiding this comment.
Import is fine — createClient is provided in client/src/http/index.js and sets baseURL and withCredentials: true, which is required for cookie-based auth flows.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
|
|
|||
| export const authClient = createClient(); | |||
There was a problem hiding this comment.
Exporting the axios instance produced by createClient() is correct — this authClient is used by authService to call auth endpoints.
|
|
||
| export const authClient = createClient(); | ||
|
|
||
| authClient.interceptors.response.use(res => res.data); |
There was a problem hiding this comment.
Consider adding an explicit error handler to the response interceptor so errors are forwarded as rejected Promises, e.g. authClient.interceptors.response.use(res => res.data, err => Promise.reject(err)). Right now it relies on axios default behavior; being explicit improves readability and avoids ambiguity.
| httpClient.interceptors.response.use(onResponseSuccess, onResponseError); | ||
|
|
||
| function onRequest(request) { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
Prefer using the accessTokenService getter for consistency rather than calling localStorage directly. This keeps token storage logic centralized and matches how other modules retrieve/save tokens (see accessTokenService).
| async function onResponseError(error) { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response.status !== 401 || originalRequest._retry) { |
There was a problem hiding this comment.
This condition assumes error.response exists. On network errors error.response can be undefined and accessing error.response.status will throw — causing the app to crash. Guard the access, e.g. if (!error.response || error.response.status !== 401 || originalRequest._retry) { throw error; } so network errors are re-thrown safely.
| } | ||
|
|
||
| async function onResponseError(error) { | ||
| const originalRequest = error.config; |
There was a problem hiding this comment.
originalRequest may be undefined in some abnormal error shapes. Ensure originalRequest is present before setting _retry or re-sending it, or short-circuit earlier. A safe pattern: const originalRequest = error.config || {}; and then check existence before mutating/using.
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { accessToken } = await authService.refresh(); |
There was a problem hiding this comment.
This function calls authService.refresh() to obtain a new access token — ensure the refresh endpoint returns { accessToken } as expected. The authService implementation used elsewhere shows refresh() is defined and used for this purpose (see authService).
| const [error, setError] = useState(''); | ||
| const [done, setDone] = useState(false); | ||
|
|
||
| const { activate, logout } = useContext(AuthContext); |
There was a problem hiding this comment.
You're only destructuring activate and logout from AuthContext — to enforce “only non-authenticated” access you should also read user here and use useNavigate to redirect authenticated users to /profile instead of calling logout() implicitly. This keeps the activation page reserved for non-authenticated flows (per checklist).
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); |
There was a problem hiding this comment.
Calling await logout() here force-logs-out any current user. The checklist requires the Activation page to be accessible only to non-authenticated users — instead of logging out, consider redirecting existing authenticated users to the Profile page or preventing access to this page via routing/guard. Forcing logout may produce surprising UX.
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
After successful activation you must redirect to Profile (navigate('/profile')). Add a navigate call in the success path. Note: currently AuthContext.activate does not save the access token or set user, so redirecting to a protected Profile will only work if activate (or a subsequent checkAuth/login) sets authentication (see AuthContext.jsx). Update activate or call checkAuth()/login after activation so Profile becomes accessible.
| }; | ||
|
|
||
| processActivation(); | ||
| }, [activationToken]); |
There was a problem hiding this comment.
The effect dependency array only includes activationToken. Add activate, logout, and navigate (if you use it) to the dependencies or ensure those functions are stable (memoized) to avoid stale-closure/lint issues.
| {error ? ( | ||
| <p className="notification is-danger is-light">{error}</p> | ||
| ) : ( | ||
| <p className="notification is-success is-light"> |
There was a problem hiding this comment.
If activation can fail you currently show an error message — consider also providing a link to Login or Home on success/failure so users have a clear next step after activation completes.
| @@ -0,0 +1,8 @@ | |||
| import axios from 'axios'; | |||
There was a problem hiding this comment.
Import is correct — axios is required to create the HTTP client instance used throughout the app.
| @@ -0,0 +1,8 @@ | |||
| import axios from 'axios'; | |||
|
|
|||
| export function createClient() { | |||
There was a problem hiding this comment.
Exporting a factory createClient() is fine and allows creating multiple differently-configured clients if needed. This matches how other files import it (e.g., authClient and httpClient).
|
|
||
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', |
There was a problem hiding this comment.
Hardcoded baseURL works for local development but reduces flexibility. Consider using an environment variable (e.g., import.meta.env.VITE_API_BASE_URL or process.env.REACT_APP_API_URL) so the app can target different backends in dev/staging/production without code changes.
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', | ||
| withCredentials: true, |
There was a problem hiding this comment.
withCredentials: true is appropriate for cookie-based authentication flows (ensures cookies are sent). Keep this if the backend uses cookies for refresh/access tokens; otherwise document the choice.
| import { userService } from '../services/userService'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| export const ForgotPasswordPage = () => { |
There was a problem hiding this comment.
This component implements the email request and the “email sent” page (it calls userService.forgotPassword) which satisfies the checklist items “Ask for an email” and “Show email sent page”. However, the password-reset flow must be available only to non-authenticated users. Use AuthContext (or a RequireNoAuth wrapper) here to redirect authenticated users to /profile or otherwise prevent access from signed-in users. AuthContext is available in the project and exposes user/auth helpers.
| <Form> | ||
| <div className="field"> | ||
| <label className="label">Email</label> | ||
| <Field |
There was a problem hiding this comment.
There is no client-side validation for the email field. Add a simple Formik validate function or use an HTML5 pattern to avoid sending empty/invalid emails (prevents unnecessary API calls and improves UX).
|
|
||
| {errorMsg && <p className="help is-danger mb-3">{errorMsg}</p>} | ||
|
|
||
| <button |
There was a problem hiding this comment.
You show a loading state via class but the submit button is still clickable. Add disabled={isSubmitting} to the <button> to prevent duplicate submissions while the request is in-flight.
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { login } = useContext(AuthContext); |
There was a problem hiding this comment.
Destructure user from AuthContext and prevent authenticated users from using this page. The task requires the login page to be accessible only to non-authenticated users; redirect authenticated users (e.g., if (user) navigate('/profile')).
| onSubmit={({ email, password }) => { | ||
| return login({ email, password }) | ||
| .then(() => { | ||
| navigate(location.state?.from?.pathname || '/'); |
There was a problem hiding this comment.
After successful login the app must redirect to Profile. Current code navigates to the previous pathname or '/'. Update this to navigate('/profile') (or ensure you always navigate to /profile after login) to satisfy the checklist requirement.
| password: '', | ||
| }} | ||
| validateOnMount={true} | ||
| onSubmit={({ email, password }) => { |
There was a problem hiding this comment.
Because you use location.state?.from?.pathname as the post-login target, this may override the requirement to always redirect to Profile. Decide desired behavior: either always redirect to /profile per checklist, or preserve return-to behavior but still satisfy the checklist by navigating to /profile when no return path is required.
| } | ||
| } | ||
|
|
||
| export const LoginPage = () => { |
There was a problem hiding this comment.
If you implement a redirect for already-authenticated users, add an effect like useEffect(() => { if (user) navigate('/profile'); }, [user, navigate]). Ensure navigate is included in the dependency array to avoid stale-closure/lint issues.
| @@ -0,0 +1,23 @@ | |||
| import React, { useEffect, useState } from 'react'; | |||
| import { useParams, Link } from 'react-router-dom'; | |||
| import { userService } from '../services/userService'; | |||
There was a problem hiding this comment.
If the user is currently logged in, the client-side user object will still show the old email after a successful confirmation. Consider reading AuthContext here and calling checkAuth() or updating the user in context so the UI reflects the new email immediately (the server updates email on confirm; see controller).
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) | ||
| .then(() => setResult('Success! Your email has been updated.')) |
There was a problem hiding this comment.
This effect updates result but provides no explicit loading state beyond the initial text. Consider setting an explicit isLoading state while the request is in-flight (disable UI actions / show spinner) to improve UX and avoid duplicate requests.
| return ( | ||
| <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.
On success you currently display a link to Login. Consider auto-redirecting after a short delay or conditionally redirecting to /profile when appropriate. Also, if you auto-redirect to profile, ensure the client auth state is refreshed (see first comment) so the profile shows the updated email.
| export const RegistrationPage = () => { | ||
| const [error, setError] = usePageError(''); | ||
| const [registered, setRegistered] = useState(false); |
There was a problem hiding this comment.
The registration page does not prevent an already-authenticated user from accessing /using the registration form. Per the task requirements the registration flow should be “only non-authenticated”. Consider importing AuthContext/useContext + useNavigate here and redirecting authenticated users to /profile, or wrap this route in a RequireNoAuth guard so authenticated users cannot access registration. This is required to meet the checklist item for registration being accessible only to non-authenticated users.
| 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 Sign up button’s disabled prop checks errors.email || errors.password but omits errors.name. This allows the form to be submitted when the name field is invalid. Update the condition to include name (e.g. disabled={isSubmitting || errors.name || errors.email || errors.password}) or use a generic check like Object.keys(errors).length > 0 to cover all validation errors.
| if (registered) { | ||
| return ( | ||
| <section className=""> | ||
| <h1 className="title">Check your email</h1> | ||
| <p>We have sent you an email with the activation link</p> | ||
| </section> | ||
| ); |
There was a problem hiding this comment.
After successful registration you show a simple “Check your email” message. Consider adding a link/button to Login (or auto-redirect after a short delay) so users have a clear next step. Also ensure this screen makes it explicit that an activation email was sent and next actions (login after activation). This improves UX and aligns with the activation-email requirement.
| import { userService } from '../services/userService.js'; | ||
|
|
||
| export const ProfilePage = () => { | ||
| const { user, updateUser } = useContext(AuthContext); |
There was a problem hiding this comment.
This component relies on user and updateUser from AuthContext. Make sure the profile route is protected with RequireAuth so only authenticated users can access this page (the app has RequireAuth for profile routes). If you rely on the route guard, document that here or assert user exists before rendering heavy UI.
| } catch (err) { | ||
| const errorMsg = err.response?.data?.message || 'Failed to update name'; |
There was a problem hiding this comment.
In the submitName catch you compute errorMsg but never set it into state or surface it to the user. Consider calling setNameMsg(errorMsg) or showing field-level errors so failures are visible to the user (currently failures are silent).
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { |
There was a problem hiding this comment.
The conditional in the password-change catch is unsafe and can throw when backendMsg is undefined. Change it to if (backendMsg && (backendMsg.includes('old password') || backendMsg.includes('at least'))) so you only call includes when backendMsg is present.
| <h3 className="subtitle">Change Password</h3> | ||
| <Formik | ||
| initialValues={{ oldPassword: '', newPassword: '', confirmPassword: '' }} | ||
| validate={values => { |
There was a problem hiding this comment.
The client-side validate for the password-change form checks equality and that new != old, but does not enforce a minimum length (server requires at least 6 chars). Add a values.newPassword.length < 6 check to provide immediate feedback and match server rules (improves UX).
| try { | ||
| await userService.requestEmailChange(values); |
There was a problem hiding this comment.
Email change request correctly calls userService.requestEmailChange(values) which triggers server-side behavior (pendingEmail + activationToken + email notifications). After confirmation the server updates the email; consider instructing the user in UI that the change is pending until they confirm the new email link sent by the server. See userService and server controller for details.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical blockers that mean the task requirements are not yet fully met. Most importantly, the auth flows around activation/login/reset are incomplete: after activation the user is not actually authenticated or redirected to /profile (AuthContext activate doesn’t save token/user, and AccountActivationPage does not navigate), login currently redirects back or to / instead of always taking the user to their Profile, and logout goes to / instead of /login as required. Several pages that must be only for non‑authenticated users (registration, login, activation, forgot password, reset password) are either unguarded or enforce this in a surprising way by force‑logging‑out on mount, instead of using your RequireNonAuth guard and redirecting authenticated users to /profile. On the server side, the refresh flow has a real bug where tokenService.getByToken(refreshToken) is used without await, and the refresh cookie uses HttpOnly instead of httpOnly, which can break token refresh and the security guarantees around the refresh cookie. The app is also missing a catch‑all 404 route on the client and a 404 handler on the server, so unknown paths do not clearly return the required “Not Found” behavior.
The critical fixes I recommend before this can pass are:
- Make activation log the user in and redirect to
/profile(either by having the server return tokens in theactivateendpoint and wiring them intoAuthContext.activate, or by following activation withcheckAuth/login) and updateAccountActivationPageto navigate to/profileon success. - Enforce the “only non‑authenticated” requirement by wrapping registration, login, activation, forgot‑password, and reset‑password routes with
RequireNonAuth(or equivalent) and remove force‑logout side effects from those pages; ensure authenticated users are consistently redirected to/profile. - Fix the refresh flow by awaiting
tokenService.getByToken(refreshToken)and correcting the cookie option tohttpOnly: true, and harden the HTTP client’s 401 interceptor to safely handle network errors. - Add a client
Route path='*'404 page and a server‑side 404 handler before the error middleware so unknown routes are handled explicitly.
Aside from these, much of your implementation is solid: registration, profile updates, email change, and most client–server integrations are in place. Once you address the blocking auth‑flow and error‑handling issues, your solution should align well with the task 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
| # env files | ||
| *.env | ||
| .env* | ||
| .env |
There was a problem hiding this comment.
Consider ignoring other environment-file variants to avoid accidental commits. For example use a pattern like .env* or explicitly add .env.local, .env.production, .env.test so all env variants are covered.
| {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 is mistyped: it includes a trailing colon in the property name. The intended rule is implicit-arrow-linebreak (without a colon) — with the current key ESLint will not match/apply this rule. Remove the extra colon from the property name.
| { | ||
| 'files': ['**/*.spec.jsx'], | ||
| 'rules': { | ||
| 'react/jsx-filename-extension': ['off'], |
There was a problem hiding this comment.
You define react/jsx-filename-extension inside the overrides as ['off'] and again later at top-level as [1, {extensions: ['.jsx']}]. This duplicate may be confusing — the last definition wins. If the intent is to disable the rule for test files only, keep the override but use the standard severity format (e.g., 'off' instead of ['off']) and remove or clarify the top-level setting.
| 'jsx-a11y/label-has-associated-control': ["error", { | ||
| assert: "either", | ||
| }], | ||
| 'jsx-a11y/label-has-for': [2, { |
There was a problem hiding this comment.
The rule jsx-a11y/label-has-for is deprecated in newer versions of eslint-plugin-jsx-a11y in favor of label-has-associated-control. Consider removing or replacing this deprecated rule to avoid plugin warnings/errors.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
There was a problem hiding this comment.
These patterns have a leading space ( .idea) so Git will treat the space as part of the filename and the actual .idea directory will not be ignored. Remove the leading space so the entry is .idea.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
| .vscode | |||
There was a problem hiding this comment.
Same issue as line 1: leading space before .vscode prevents correct matching. Remove the leading space so the entry is .vscode.
| @@ -0,0 +1,10 @@ | |||
| .idea | |||
| .vscode | |||
| build | |||
There was a problem hiding this comment.
Leading space before build — remove the space so build is ignored as intended.
| .idea | ||
| .vscode | ||
| build | ||
| dist |
There was a problem hiding this comment.
Leading space before dist — remove the space so dist is ignored as intended.
| .vscode | ||
| build | ||
| dist | ||
| node_modules |
There was a problem hiding this comment.
Leading space before node_modules — remove the space so node_modules is ignored (this is important to avoid committing dependencies).
| build | ||
| dist | ||
| node_modules | ||
| .DS_Store |
There was a problem hiding this comment.
Leading space before .DS_Store — remove the space so .DS_Store files are ignored on macOS.
| useEffect(() => { | ||
| checkAuth(); | ||
| }, []); |
There was a problem hiding this comment.
The effect calls checkAuth() but checkAuth is not listed in the dependency array. This can cause a lint warning or stale closure if checkAuth changes. Either include checkAuth in the dependency array or ensure checkAuth is stable (e.g., memoized in context).
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="registration" element={<RegistrationPage />} /> | ||
| <Route | ||
| path="activate/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> | ||
| <Route | ||
| path="confirm-email-change/:activationToken" | ||
| element={<EmailConfirmPage />} | ||
| /> | ||
| <Route path="login" element={<LoginPage />} /> | ||
| <Route path="forgot-password" element={<ForgotPasswordPage />} /> | ||
| <Route path="reset-password/:activationToken" element={<ResetPasswordPage />} /> |
There was a problem hiding this comment.
Routes for non-authenticated flows — registration, activation, email-confirmation, login, forgot-password, reset-password — are defined here without any guard to restrict access to non-authenticated users. The task requires these pages to be accessible only to non-authenticated users. Add a RequireNoAuth wrapper or perform redirects inside these pages to enforce “only non authenticated” behavior.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> | ||
| </Routes> |
There was a problem hiding this comment.
There is no catch-all 404 route (e.g., path='*') defined. The task requires the application to show a 404 for all other pages. Add a Route path='*' element={<NotFoundPage/>} (and create the NotFoundPage) so unknown paths show a 404.
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="users" element={<UsersPage />} /> | ||
| </Route> | ||
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| </Route> |
There was a problem hiding this comment.
You have two separate parent routes using path='/' with element={<RequireAuth/>} wrapping users and profile. This works but is redundant — consider consolidating protected child routes under a single RequireAuth parent to simplify routing structure.
| aria-label="main navigation" | ||
| > | ||
| <div className="navbar-start"> | ||
| <NavLink to="/" className="navbar-item"> |
There was a problem hiding this comment.
Minor UX: NavLink to='/' for Home may remain active on child routes. Consider adding the end prop to the root NavLink to ensure it’s only active on the exact / path.
| 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.
The activate implementation only calls authService.activate but does not save the returned accessToken or set the user. That prevents the app from becoming authenticated after activation and will break the requirement to redirect to Profile after activation. Restore the logic to save the token (via accessTokenService.save) and call setUser using the response from authService.activate (the commented lines show the intended behavior).
| // accessTokenService.save(accessToken); | ||
| // setUser(user); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is no useEffect in this provider to call checkAuth on mount. If checkAuth is not invoked elsewhere, the app will never refresh the session and isChecked/user will remain stale. Add useEffect(() => { checkAuth(); }, []) (or ensure checkAuth is called from a top-level component) so the provider actually verifies existing sessions on app start.
|
|
||
| export const AuthProvider = ({ children }) => { | ||
| const [user, setUser] = useState(null); | ||
| const [isChecked, setChecked] = useState(true); |
There was a problem hiding this comment.
isChecked is initialized to true. That indicates auth has already been checked before checkAuth runs and can cause UI to skip loading/activation flows. Initialize it to false so the provider indicates verification is pending until checkAuth sets it to true in the finally block.
| accessTokenService.save(accessToken); | ||
| setUser(user); | ||
| } catch (error) { | ||
| console.log('User is not authentincated'); |
There was a problem hiding this comment.
Minor: console message is misspelled ('User is not authentincated'). Fix the typo to 'User is not authenticated' to keep logs clear.
| const value = useMemo( | ||
| () => ({ | ||
| isChecked, | ||
| user, | ||
| checkAuth, | ||
| activate, | ||
| login, | ||
| logout, | ||
| updateUser | ||
| }), | ||
| [user, isChecked, updateUser], |
There was a problem hiding this comment.
useMemo dependencies include updateUser, which is re-created every render (it isn't wrapped in useCallback) — that makes the memo less effective. Consider wrapping functions (checkAuth, activate, login, logout, updateUser) in useCallback or remove updateUser from deps if you intentionally want to recreate the memo when user changes only.
| @@ -0,0 +1,19 @@ | |||
| import { Navigate, Outlet, useLocation } from 'react-router-dom'; | |||
| import React, { useContext } from 'react'; | |||
| import { AuthContext } from './AuthContext.jsx'; | |||
There was a problem hiding this comment.
Import path uses the .jsx extension here while other files import AuthContext without the extension. This is not a functional error but creates inconsistent imports across the project. Consider using the same import style everywhere for clarity (e.g., ./AuthContext).
| import { Loader } from './Loader.jsx'; | ||
|
|
||
| export const RequireAuth = ({ children }) => { | ||
| const { isChecked, user } = useContext(AuthContext); |
There was a problem hiding this comment.
The component correctly reads isChecked and user from context, but if the user object can be nullish or in an unexpected shape you may want to defensively ensure user is the expected object shape before using it elsewhere. This is a light suggestion — current code is fine if the context guarantees user is either null or a valid user object.
| return <Loader />; | ||
| } | ||
|
|
||
| if (!user) { |
There was a problem hiding this comment.
When an unauthenticated visitor is detected you redirect to /login. The task requires that users who are authenticated but not active should be asked to activate their email. This component currently does not inspect an active flag on user. If you want protected routes to require an activated account, add a check like if (user && !user.isActive) return <Navigate to="/activate" .../> (or redirect to the appropriate activation page) so inactive-but-authenticated users cannot access protected pages.
| return <Navigate to="/login" state={{ from: location }} replace />; | ||
| } | ||
|
|
||
| return children || <Outlet />; |
There was a problem hiding this comment.
Returning children || <Outlet /> is fine and supports both usage styles. No change required, but if your app only uses nested routes you can simplify to just <Outlet /> to avoid handling both patterns unless you intentionally allow children.
| if (user) { | ||
| return <Navigate to="/" replace />; |
There was a problem hiding this comment.
Redirects authenticated users to /. According to the app requirements, authenticated users should end up on the Profile page after login/activation — consider redirecting to /profile instead of / so protected users land on their profile.
| @@ -0,0 +1,18 @@ | |||
| import { Navigate, Outlet } from 'react-router-dom'; | |||
| import React, { useContext } from 'react'; | |||
| import { AuthContext } from './AuthContext.jsx'; | |||
There was a problem hiding this comment.
Import path includes the .jsx extension while other files (e.g., App.jsx) import AuthContext without the extension. This works, but you may want to keep imports consistent across the project (either always include extensions or omit them) to avoid confusion.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
There was a problem hiding this comment.
Import is fine — createClient is provided in client/src/http/index.js and sets baseURL and withCredentials: true, which is required for cookie-based auth flows.
| @@ -0,0 +1,5 @@ | |||
| import { createClient } from './index.js'; | |||
|
|
|||
| export const authClient = createClient(); | |||
There was a problem hiding this comment.
Exporting the axios instance produced by createClient() is correct — this authClient is used by authService to call auth endpoints.
|
|
||
| export const authClient = createClient(); | ||
|
|
||
| authClient.interceptors.response.use(res => res.data); |
There was a problem hiding this comment.
Consider adding an explicit error handler to the response interceptor so errors are forwarded as rejected Promises, e.g. authClient.interceptors.response.use(res => res.data, err => Promise.reject(err)). Right now it relies on axios default behavior; being explicit improves readability and avoids ambiguity.
| httpClient.interceptors.response.use(onResponseSuccess, onResponseError); | ||
|
|
||
| function onRequest(request) { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
Prefer using the accessTokenService getter for consistency rather than calling localStorage directly. This keeps token storage logic centralized and matches how other modules retrieve/save tokens (see accessTokenService).
| async function onResponseError(error) { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response.status !== 401 || originalRequest._retry) { |
There was a problem hiding this comment.
This condition assumes error.response exists. On network errors error.response can be undefined and accessing error.response.status will throw — causing the app to crash. Guard the access, e.g. if (!error.response || error.response.status !== 401 || originalRequest._retry) { throw error; } so network errors are re-thrown safely.
| } | ||
|
|
||
| async function onResponseError(error) { | ||
| const originalRequest = error.config; |
There was a problem hiding this comment.
originalRequest may be undefined in some abnormal error shapes. Ensure originalRequest is present before setting _retry or re-sending it, or short-circuit earlier. A safe pattern: const originalRequest = error.config || {}; and then check existence before mutating/using.
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { accessToken } = await authService.refresh(); |
There was a problem hiding this comment.
This function calls authService.refresh() to obtain a new access token — ensure the refresh endpoint returns { accessToken } as expected. The authService implementation used elsewhere shows refresh() is defined and used for this purpose (see authService).
| const [error, setError] = useState(''); | ||
| const [done, setDone] = useState(false); | ||
|
|
||
| const { activate, logout } = useContext(AuthContext); |
There was a problem hiding this comment.
You're only destructuring activate and logout from AuthContext — to enforce “only non-authenticated” access you should also read user here and use useNavigate to redirect authenticated users to /profile instead of calling logout() implicitly. This keeps the activation page reserved for non-authenticated flows (per checklist).
| useEffect(() => { | ||
| const processActivation = async () => { | ||
| try { | ||
| await logout(); |
There was a problem hiding this comment.
Calling await logout() here force-logs-out any current user. The checklist requires the Activation page to be accessible only to non-authenticated users — instead of logging out, consider redirecting existing authenticated users to the Profile page or preventing access to this page via routing/guard. Forcing logout may produce surprising UX.
| const processActivation = async () => { | ||
| try { | ||
| await logout(); | ||
| await activate(activationToken); |
There was a problem hiding this comment.
After successful activation you must redirect to Profile (navigate('/profile')). Add a navigate call in the success path. Note: currently AuthContext.activate does not save the access token or set user, so redirecting to a protected Profile will only work if activate (or a subsequent checkAuth/login) sets authentication (see AuthContext.jsx). Update activate or call checkAuth()/login after activation so Profile becomes accessible.
| }; | ||
|
|
||
| processActivation(); | ||
| }, [activationToken]); |
There was a problem hiding this comment.
The effect dependency array only includes activationToken. Add activate, logout, and navigate (if you use it) to the dependencies or ensure those functions are stable (memoized) to avoid stale-closure/lint issues.
| {error ? ( | ||
| <p className="notification is-danger is-light">{error}</p> | ||
| ) : ( | ||
| <p className="notification is-success is-light"> |
There was a problem hiding this comment.
If activation can fail you currently show an error message — consider also providing a link to Login or Home on success/failure so users have a clear next step after activation completes.
| @@ -0,0 +1,8 @@ | |||
| import axios from 'axios'; | |||
There was a problem hiding this comment.
Import is correct — axios is required to create the HTTP client instance used throughout the app.
| @@ -0,0 +1,8 @@ | |||
| import axios from 'axios'; | |||
|
|
|||
| export function createClient() { | |||
There was a problem hiding this comment.
Exporting a factory createClient() is fine and allows creating multiple differently-configured clients if needed. This matches how other files import it (e.g., authClient and httpClient).
|
|
||
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', |
There was a problem hiding this comment.
Hardcoded baseURL works for local development but reduces flexibility. Consider using an environment variable (e.g., import.meta.env.VITE_API_BASE_URL or process.env.REACT_APP_API_URL) so the app can target different backends in dev/staging/production without code changes.
| export function createClient() { | ||
| return axios.create({ | ||
| baseURL: 'http://localhost:3005', | ||
| withCredentials: true, |
There was a problem hiding this comment.
withCredentials: true is appropriate for cookie-based authentication flows (ensures cookies are sent). Keep this if the backend uses cookies for refresh/access tokens; otherwise document the choice.
| import { userService } from '../services/userService'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| export const ForgotPasswordPage = () => { |
There was a problem hiding this comment.
This component implements the email request and the “email sent” page (it calls userService.forgotPassword) which satisfies the checklist items “Ask for an email” and “Show email sent page”. However, the password-reset flow must be available only to non-authenticated users. Use AuthContext (or a RequireNoAuth wrapper) here to redirect authenticated users to /profile or otherwise prevent access from signed-in users. AuthContext is available in the project and exposes user/auth helpers.
| <Form> | ||
| <div className="field"> | ||
| <label className="label">Email</label> | ||
| <Field |
There was a problem hiding this comment.
There is no client-side validation for the email field. Add a simple Formik validate function or use an HTML5 pattern to avoid sending empty/invalid emails (prevents unnecessary API calls and improves UX).
|
|
||
| {errorMsg && <p className="help is-danger mb-3">{errorMsg}</p>} | ||
|
|
||
| <button |
There was a problem hiding this comment.
You show a loading state via class but the submit button is still clickable. Add disabled={isSubmitting} to the <button> to prevent duplicate submissions while the request is in-flight.
| const location = useLocation(); | ||
|
|
||
| const [error, setError] = usePageError(''); | ||
| const { login } = useContext(AuthContext); |
There was a problem hiding this comment.
Destructure user from AuthContext and prevent authenticated users from using this page. The task requires the login page to be accessible only to non-authenticated users; redirect authenticated users (e.g., if (user) navigate('/profile')).
| onSubmit={({ email, password }) => { | ||
| return login({ email, password }) | ||
| .then(() => { | ||
| navigate(location.state?.from?.pathname || '/'); |
There was a problem hiding this comment.
After successful login the app must redirect to Profile. Current code navigates to the previous pathname or '/'. Update this to navigate('/profile') (or ensure you always navigate to /profile after login) to satisfy the checklist requirement.
| password: '', | ||
| }} | ||
| validateOnMount={true} | ||
| onSubmit={({ email, password }) => { |
There was a problem hiding this comment.
Because you use location.state?.from?.pathname as the post-login target, this may override the requirement to always redirect to Profile. Decide desired behavior: either always redirect to /profile per checklist, or preserve return-to behavior but still satisfy the checklist by navigating to /profile when no return path is required.
| } | ||
| } | ||
|
|
||
| export const LoginPage = () => { |
There was a problem hiding this comment.
If you implement a redirect for already-authenticated users, add an effect like useEffect(() => { if (user) navigate('/profile'); }, [user, navigate]). Ensure navigate is included in the dependency array to avoid stale-closure/lint issues.
| @@ -0,0 +1,23 @@ | |||
| import React, { useEffect, useState } from 'react'; | |||
| import { useParams, Link } from 'react-router-dom'; | |||
| import { userService } from '../services/userService'; | |||
There was a problem hiding this comment.
If the user is currently logged in, the client-side user object will still show the old email after a successful confirmation. Consider reading AuthContext here and calling checkAuth() or updating the user in context so the UI reflects the new email immediately (the server updates email on confirm; see controller).
| useEffect(() => { | ||
| userService.confirmEmailChange(activationToken) | ||
| .then(() => setResult('Success! Your email has been updated.')) |
There was a problem hiding this comment.
This effect updates result but provides no explicit loading state beyond the initial text. Consider setting an explicit isLoading state while the request is in-flight (disable UI actions / show spinner) to improve UX and avoid duplicate requests.
| return ( | ||
| <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.
On success you currently display a link to Login. Consider auto-redirecting after a short delay or conditionally redirecting to /profile when appropriate. Also, if you auto-redirect to profile, ensure the client auth state is refreshed (see first comment) so the profile shows the updated email.
| export const RegistrationPage = () => { | ||
| const [error, setError] = usePageError(''); | ||
| const [registered, setRegistered] = useState(false); |
There was a problem hiding this comment.
The registration page does not prevent an already-authenticated user from accessing /using the registration form. Per the task requirements the registration flow should be “only non-authenticated”. Consider importing AuthContext/useContext + useNavigate here and redirecting authenticated users to /profile, or wrap this route in a RequireNoAuth guard so authenticated users cannot access registration. This is required to meet the checklist item for registration being accessible only to non-authenticated users.
| 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 Sign up button’s disabled prop checks errors.email || errors.password but omits errors.name. This allows the form to be submitted when the name field is invalid. Update the condition to include name (e.g. disabled={isSubmitting || errors.name || errors.email || errors.password}) or use a generic check like Object.keys(errors).length > 0 to cover all validation errors.
| if (registered) { | ||
| return ( | ||
| <section className=""> | ||
| <h1 className="title">Check your email</h1> | ||
| <p>We have sent you an email with the activation link</p> | ||
| </section> | ||
| ); |
There was a problem hiding this comment.
After successful registration you show a simple “Check your email” message. Consider adding a link/button to Login (or auto-redirect after a short delay) so users have a clear next step. Also ensure this screen makes it explicit that an activation email was sent and next actions (login after activation). This improves UX and aligns with the activation-email requirement.
| import { userService } from '../services/userService.js'; | ||
|
|
||
| export const ProfilePage = () => { | ||
| const { user, updateUser } = useContext(AuthContext); |
There was a problem hiding this comment.
This component relies on user and updateUser from AuthContext. Make sure the profile route is protected with RequireAuth so only authenticated users can access this page (the app has RequireAuth for profile routes). If you rely on the route guard, document that here or assert user exists before rendering heavy UI.
| } catch (err) { | ||
| const errorMsg = err.response?.data?.message || 'Failed to update name'; |
There was a problem hiding this comment.
In the submitName catch you compute errorMsg but never set it into state or surface it to the user. Consider calling setNameMsg(errorMsg) or showing field-level errors so failures are visible to the user (currently failures are silent).
| } catch (err) { | ||
| const backendMsg = err.response?.data?.message; | ||
|
|
||
| if (backendMsg && (backendMsg.includes('old password')) || backendMsg.includes('at least')) { |
There was a problem hiding this comment.
The conditional in the password-change catch is unsafe and can throw when backendMsg is undefined. Change it to if (backendMsg && (backendMsg.includes('old password') || backendMsg.includes('at least'))) so you only call includes when backendMsg is present.
| <h3 className="subtitle">Change Password</h3> | ||
| <Formik | ||
| initialValues={{ oldPassword: '', newPassword: '', confirmPassword: '' }} | ||
| validate={values => { |
There was a problem hiding this comment.
The client-side validate for the password-change form checks equality and that new != old, but does not enforce a minimum length (server requires at least 6 chars). Add a values.newPassword.length < 6 check to provide immediate feedback and match server rules (improves UX).
| try { | ||
| await userService.requestEmailChange(values); |
There was a problem hiding this comment.
Email change request correctly calls userService.requestEmailChange(values) which triggers server-side behavior (pendingEmail + activationToken + email notifications). After confirmation the server updates the email; consider instructing the user in UI that the change is pending until they confirm the new email link sent by the server. See userService and server controller for details.
2 participants
No description provided.