refactor authentication components and services for improved user exp… - #40
Conversation
…erience and implement multi-step signup process with form validation and health conditions
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors auth pages by extracting the three-step signup form into ChangesAuth Signup/Login Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/components/auth/StepOne.jsx (1)
14-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssociate
<label>elements with their inputs for accessibility.Labels here are siblings of the inputs without
htmlFor/id, so assistive technologies won't announce the field name when the input is focused, and clicking the label won't focus the input. This pattern repeats across every field in StepOne (and StepTwo). Add matchingid/htmlFor(or wrap the input inside the label as done inAllergiesDropdown).♿ Example for First Name
- <label className="text-sm font-medium">First Name</label> + <label htmlFor="firstName" className="text-sm font-medium">First Name</label> <div className="relative"> <FaUser className="absolute top-3 left-3 text-gray-500" /> <input + id="firstName" type="text" name="firstName"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/auth/StepOne.jsx` around lines 14 - 116, The StepOne form fields use sibling <label> and <input> elements without matching associations, so accessibility and label-click focus are missing. Update the StepOne component (and the same pattern in StepTwo) to use unique input ids with matching htmlFor on each label, or wrap each input inside its label like AllergiesDropdown; ensure the First Name, Last Name, Phone number, Email, Password, and Confirm Password controls are all linked correctly.src/pages/auth/Signup.jsx (1)
41-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStep navigation performs no validation.
nextStepadvances unconditionally, so users can reach StepThree and submit with empty required fields (name, email, etc.). GatingnextStepon per-step validation would prevent invalid submissions and improve UX. This is the root cause behind the two issues above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/auth/Signup.jsx` around lines 41 - 42, The step navigation in Signup.jsx currently advances unconditionally in nextStep, which lets users reach later steps and submit with missing required data. Update nextStep in the Signup component to validate the current step’s required fields before calling setStep, and only advance when that step is valid; keep prevStep unchanged. Use the existing Signup, nextStep, and setStep symbols to wire the validation into the per-step flow so StepThree cannot be reached with invalid input.src/components/ui/AllergiesDropdown.jsx (1)
35-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
key={index}is acceptable here butkey={option}is more robust.The options list is static, so index keys won't cause issues today. Using the stable
optionstring as the key is the idiomatic choice and survives any future list reordering. The ast-grep "list-component-needs-key" hint on the<input>is a false positive—the<label>already carries the key.♻️ Proposed tweak
- {options.map((option, index) => ( + {options.map((option) => ( <label - key={index} + key={option} className="flex items-center gap-2 cursor-pointer" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/AllergiesDropdown.jsx` around lines 35 - 48, Update the options rendering in AllergiesDropdown so the mapped <label> uses the stable option value as its key instead of the array index; this is a small cleanup in the options.map block. Keep the existing key on the <label> (not the <input>), since the list item is already keyed correctly and the ast-grep list-component-needs-key hint is a false positive here.src/pages/auth/Login.jsx (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePost-await
getState()works, but returning status fromloginis cleaner.Reading
useAuthStore.getState().isAuthenticatedright afterawait login(...)is functionally correct with Zustand (fresh snapshot), but couples the page to store internals. Havingloginresolve to a success boolean (or throw) lets the caller branch directly without a second store read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/auth/Login.jsx` around lines 22 - 27, The submit flow in handleSubmit currently reads auth state directly from useAuthStore after awaiting login, which couples Login to store internals. Update login so it returns a success boolean or throws on failure, then change handleSubmit to branch on that return value and navigate("/") only on success, removing the post-await getState() read.src/store/authStore.js (1)
22-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the commented-out
isValidUservalidator.Dead/commented code adds noise. If user-shape validation is still desired post-refactor, reintroduce it as active code; otherwise delete this block (version control preserves history).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/authStore.js` around lines 22 - 32, Remove the commented-out isValidUser validator from authStore.js; it is dead code and should not remain in the file. If user-shape validation is still needed, restore it as an active helper in authStore instead of keeping the commented block, otherwise delete the entire isValidUser section cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/auth/Login.jsx`:
- Around line 54-62: Add the missing autocomplete attributes to the login form
inputs in Login: set the email field to use autoComplete="email" and the
password field to use autoComplete="current-password". Update the relevant input
elements in the Login component so browser autofill and password managers can
recognize both fields correctly.
In `@src/pages/auth/Signup.jsx`:
- Around line 60-66: The Signup form submission currently converts age, height,
and weight with Number(...) in the submit payload, which lets empty or invalid
input become 0/NaN instead of being blocked. Update Signup.jsx to validate these
fields before calling register, either by enforcing required checks in the
per-step flow or by rejecting the submit when age, height, or weight are
missing/invalid, and keep the conversion logic only after validation in the
submit handler.
- Around line 44-79: The password confirmation check is happening too late in
Signup.jsx inside handleSubmit, so the mismatch only appears on the final step
instead of where the passwords are entered. Move this validation into nextStep
when leaving StepOne, and set the existing error state there so the user sees
feedback before advancing. Keep handleSubmit focused on registration and use the
StepOne password fields and nextStep flow to locate and fix the issue.
In `@src/services/auth.service.js`:
- Around line 4-6: The auth signup flow now posts to /auth/signup via
register(), but the mock handler still only matches /auth/register. Update the
matching entry in handlers.js so the mock for register() uses /auth/signup
instead, keeping the mock response aligned with the auth.service.js endpoint.
In `@src/store/authStore.js`:
- Line 80: In authStore.js, the login flow is overriding the backend-provided
expiry with a local timestamp, which can desync the persisted session from the
server token lifetime. Update the login handling in the auth store so it
preserves response.data.expiresAt when the server supplies it, and only uses
Date.now() + TOKEN_LIFETIME as a fallback when that field is missing; use the
login action and the expiresAt assignment as the main points to locate the
change.
- Around line 68-83: The login flow in authStore’s response handling is reading
flat fields from response.data, but the actual login payload provides a nested
user object, so map from response.data.user before calling set. Update the
destructuring in the login success path to use the user payload returned by
loginService, and keep the stored user shape consistent with the mock/session
contract while preserving the token and auth state checks.
---
Nitpick comments:
In `@src/components/auth/StepOne.jsx`:
- Around line 14-116: The StepOne form fields use sibling <label> and <input>
elements without matching associations, so accessibility and label-click focus
are missing. Update the StepOne component (and the same pattern in StepTwo) to
use unique input ids with matching htmlFor on each label, or wrap each input
inside its label like AllergiesDropdown; ensure the First Name, Last Name, Phone
number, Email, Password, and Confirm Password controls are all linked correctly.
In `@src/components/ui/AllergiesDropdown.jsx`:
- Around line 35-48: Update the options rendering in AllergiesDropdown so the
mapped <label> uses the stable option value as its key instead of the array
index; this is a small cleanup in the options.map block. Keep the existing key
on the <label> (not the <input>), since the list item is already keyed correctly
and the ast-grep list-component-needs-key hint is a false positive here.
In `@src/pages/auth/Login.jsx`:
- Around line 22-27: The submit flow in handleSubmit currently reads auth state
directly from useAuthStore after awaiting login, which couples Login to store
internals. Update login so it returns a success boolean or throws on failure,
then change handleSubmit to branch on that return value and navigate("/") only
on success, removing the post-await getState() read.
In `@src/pages/auth/Signup.jsx`:
- Around line 41-42: The step navigation in Signup.jsx currently advances
unconditionally in nextStep, which lets users reach later steps and submit with
missing required data. Update nextStep in the Signup component to validate the
current step’s required fields before calling setStep, and only advance when
that step is valid; keep prevStep unchanged. Use the existing Signup, nextStep,
and setStep symbols to wire the validation into the per-step flow so StepThree
cannot be reached with invalid input.
In `@src/store/authStore.js`:
- Around line 22-32: Remove the commented-out isValidUser validator from
authStore.js; it is dead code and should not remain in the file. If user-shape
validation is still needed, restore it as an active helper in authStore instead
of keeping the commented block, otherwise delete the entire isValidUser section
cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0270d041-29e4-457c-a01f-253910efe7e1
📒 Files selected for processing (8)
src/components/auth/StepOne.jsxsrc/components/auth/StepThree.jsxsrc/components/auth/StepTwo.jsxsrc/components/ui/AllergiesDropdown.jsxsrc/pages/auth/Login.jsxsrc/pages/auth/Signup.jsxsrc/services/auth.service.jssrc/store/authStore.js
…gin components, and improve session management tests
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/store/authStore.js`:
- Around line 78-89: The auth flow in authStore is persisting the backend user
object verbatim, so normalize it to the store’s safe user shape before calling
set. Update the login/signup path that builds the user object to whitelist only
the expected fields (id, email, role, firstName, lastName), and validate that
the required keys are present before marking the session authenticated. If any
required field is missing, abort the login instead of storing a malformed user.
In `@src/tests/services/session.test.js`:
- Around line 21-27: The test teardown in session.test.js is leaving the Axios
mock attached to the shared api client because mock.reset() only clears state
and does not unpatch the adapter. Update the afterEach cleanup to use the mock’s
restore behavior so the AxiosMockAdapter created for api is fully removed
between tests, keeping later tests isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 564a8b86-3960-47f0-b705-37a53b891846
📒 Files selected for processing (6)
src/components/auth/StepTwo.jsxsrc/mocks/handlers.jssrc/pages/auth/Login.jsxsrc/pages/auth/Signup.jsxsrc/store/authStore.jssrc/tests/services/session.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/auth/StepTwo.jsx
- src/pages/auth/Signup.jsx
- src/pages/auth/Login.jsx
| const user = data.user ?? { | ||
| id: data.userId, | ||
| email: data.emailString, | ||
| role: data.role, | ||
| firstName: data.firstName, | ||
| lastName: data.lastName, | ||
| }; | ||
|
|
||
| set({ | ||
| token, | ||
| user, | ||
| expiresAt: data.expiresAt ?? Date.now() + TOKEN_LIFETIME, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Normalize and whitelist data.user before persisting it.
partialize persists state.user to localStorage, and this branch now stores data.user verbatim. With the new health-condition signup data, any extra backend profile fields can now end up in browser storage, and missing required keys still leave isAuthenticated true with a malformed user. Project the response down to the safe store shape before set, and fail the login if the required fields are absent.
Suggested fix
- const user = data.user ?? {
- id: data.userId,
- email: data.emailString,
- role: data.role,
- firstName: data.firstName,
- lastName: data.lastName,
- };
+ const rawUser = data.user ?? {
+ id: data.userId,
+ email: data.emailString,
+ role: data.role,
+ firstName: data.firstName,
+ lastName: data.lastName,
+ };
+
+ const user =
+ rawUser?.id != null &&
+ typeof rawUser.email === "string" &&
+ rawUser.email.trim().length > 0
+ ? {
+ id: rawUser.id,
+ email: rawUser.email,
+ role: rawUser.role,
+ firstName: rawUser.firstName,
+ lastName: rawUser.lastName,
+ }
+ : null;
+
+ if (!user) {
+ set({
+ user: null,
+ token: null,
+ expiresAt: null,
+ isAuthenticated: false,
+ error: "Invalid user payload",
+ loading: false,
+ });
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const user = data.user ?? { | |
| id: data.userId, | |
| email: data.emailString, | |
| role: data.role, | |
| firstName: data.firstName, | |
| lastName: data.lastName, | |
| }; | |
| set({ | |
| token, | |
| user, | |
| expiresAt: data.expiresAt ?? Date.now() + TOKEN_LIFETIME, | |
| const rawUser = data.user ?? { | |
| id: data.userId, | |
| email: data.emailString, | |
| role: data.role, | |
| firstName: data.firstName, | |
| lastName: data.lastName, | |
| }; | |
| const user = | |
| rawUser?.id != null && | |
| typeof rawUser.email === "string" && | |
| rawUser.email.trim().length > 0 | |
| ? { | |
| id: rawUser.id, | |
| email: rawUser.email, | |
| role: rawUser.role, | |
| firstName: rawUser.firstName, | |
| lastName: rawUser.lastName, | |
| } | |
| : null; | |
| if (!user) { | |
| set({ | |
| user: null, | |
| token: null, | |
| expiresAt: null, | |
| isAuthenticated: false, | |
| error: "Invalid user payload", | |
| loading: false, | |
| }); | |
| return; | |
| } | |
| set({ | |
| token, | |
| user, | |
| expiresAt: data.expiresAt ?? Date.now() + TOKEN_LIFETIME, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/authStore.js` around lines 78 - 89, The auth flow in authStore is
persisting the backend user object verbatim, so normalize it to the store’s safe
user shape before calling set. Update the login/signup path that builds the user
object to whitelist only the expected fields (id, email, role, firstName,
lastName), and validate that the required keys are present before marking the
session authenticated. If any required field is missing, abort the login instead
of storing a malformed user.
| // Mock Axios | ||
| mock = new AxiosMockAdapter(api); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| mock.reset(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== session.test.js excerpt ==\n'
sed -n '1,140p' src/tests/services/session.test.js
printf '\n== axios-mock-adapter usage in repo ==\n'
rg -n "mock\.(reset|restore)\(|new AxiosMockAdapter|axios-mock-adapter" src tests . --glob '!**/node_modules/**' || true
printf '\n== package references ==\n'
rg -n '"axios-mock-adapter"|axios-mock-adapter' package.json package-lock.json yarn.lock pnpm-lock.yaml . --glob '!**/node_modules/**' || trueRepository: Revive-Graduation-Project/Revive-Front-End
Length of output: 7703
🌐 Web query:
axios-mock-adapter reset restore semantics official docs
💡 Result:
In axios-mock-adapter, the reset and restore methods serve distinct purposes regarding the state of the mock adapter [1][2]. reset The reset method removes all registered mock handlers and clears all history [1][3]. It keeps the mocking behavior active on the axios instance, meaning that if you make a request after calling reset, it will result in a 404 error because no handlers remain [2]. This is useful for clearing state between individual tests while maintaining the interceptor [2]. restore The restore method removes the mocking from the axios instance completely [1][2]. It reverts the instance to its original state, effectively uninstalling the adapter and allowing actual network requests to proceed again [1][2]. This is typically used in cleanup hooks (like after) to ensure tests do not leak mocking behavior to other parts of a test suite [2]. Summary Table Method | Impact on Handlers | Impact on History | Impact on Interceptor --- | --- | --- | --- reset | Cleared [1] | Cleared [1] | Active (intercepts requests) [1][2] restore | Removed [1] | Removed [1] | Inactive (removes interceptor) [1][2] Additional cleanup methods available include resetHandlers (clears only handlers) and resetHistory (clears only history) [1][3].
Citations:
- 1: https://github.com/ctimmerm/axios-mock-adapter
- 2: Correct way to cleanup mock ctimmerm/axios-mock-adapter#34
- 3: https://unpkg.com/axios-mock-adapter@1.21.1/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/tests/services/api.test.js ==\n'
sed -n '1,120p' src/tests/services/api.test.js
printf '\n== axios-mock-adapter package README excerpt (if vendored in lockfile only, skip) ==\n'
node - <<'JS'
const fs = require('fs');
const path = 'node_modules/axios-mock-adapter/README.md';
if (fs.existsSync(path)) {
const lines = fs.readFileSync(path, 'utf8').split('\n');
const hits = [];
for (let i = 0; i < lines.length; i++) {
if (/reset|restore/i.test(lines[i])) {
hits.push(`${i+1}: ${lines[i]}`);
}
}
console.log(hits.slice(0, 40).join('\n'));
} else {
console.log('node_modules/axios-mock-adapter/README.md not present');
}
JSRepository: Revive-Graduation-Project/Revive-Front-End
Length of output: 5232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== api service ==\n'
sed -n '1,220p' src/services/api.js 2>/dev/null || sed -n '1,220p' src/services/api.ts 2>/dev/null || true
printf '\n== tests using ../../services/api or src/services/api ==\n'
rg -n "from ['\"].*services/api['\"]|from ['\"].*services/api['\"]|import .*api.*services/api" src/tests src --glob '!**/node_modules/**' || true
printf '\n== tests that create AxiosMockAdapter(api) ==\n'
rg -n "new AxiosMockAdapter\\(api" src/tests src --glob '!**/node_modules/**' || trueRepository: Revive-Graduation-Project/Revive-Front-End
Length of output: 6044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== VITE_USE_MOCK references ==\n'
rg -n "VITE_USE_MOCK|USE_MOCK" . --glob '!**/node_modules/**' || true
printf '\n== tests importing api without AxiosMockAdapter ==\n'
python3 - <<'PY'
import os, re
matches = []
for root, dirs, files in os.walk('src/tests'):
for f in files:
if f.endswith(('.js','.ts','.jsx','.tsx')):
path = os.path.join(root, f)
txt = open(path, encoding='utf-8').read()
if re.search(r"from\s+['\"].*services/api['\"]", txt) and 'AxiosMockAdapter' not in txt:
matches.append(path)
print('\n'.join(matches) if matches else 'NONE')
PYRepository: Revive-Graduation-Project/Revive-Front-End
Length of output: 1005
Use mock.restore() in teardown
reset() clears handlers/history but leaves the mock installed on the shared api singleton, which can affect later tests that reuse the same client.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/services/session.test.js` around lines 21 - 27, The test teardown
in session.test.js is leaving the Axios mock attached to the shared api client
because mock.reset() only clears state and does not unpatch the adapter. Update
the afterEach cleanup to use the mock’s restore behavior so the AxiosMockAdapter
created for api is fully removed between tests, keeping later tests isolated.
3 participants
…erience and implement multi-step signup process with form validation and health conditions
Summary by CodeRabbit