refacout: account setttings#69
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughMigrates profile UX into a new /account hub: adds client user API, server endpoints for profile/password/subscription, introduces tabbed AccountSettings with PersonalInfo, SecuritySettings, SubscriptionHistory, ProfilesManagement pages, updates routing/auth modal wiring and AppBar, and removes legacy /profiles pages. ChangesAccount Settings Hub Migration
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant AccountSettings
participant PersonalInfo
participant userApi
participant Server
User->>Browser: Navigate to /account
Browser->>AccountSettings: Load tabs
AccountSettings->>PersonalInfo: Render Personal Info tab
User->>PersonalInfo: Select avatar + edit fields
PersonalInfo->>userApi: updateUserProfileApi(FormData)
userApi->>Server: PUT /users/profile
Server-->>userApi: 200 updated profile
userApi-->>PersonalInfo: updated data
PersonalInfo->>Browser: invalidate queries / show toast
User->>AccountSettings: Open Security tab
AccountSettings->>userApi: changePasswordApi(current,new)
userApi->>Server: POST /users/change-password
Server-->>userApi: 200 success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
client/src/pages/Account/AccountSettings.jsx (1)
44-69: ⚡ Quick winAdd ARIA tab semantics for accessibility.
The tab UI is functional, but it should expose tab roles/states (
tablist,tab,tabpanel,aria-selected,aria-controls) for keyboard/screen-reader parity.🤖 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 `@client/src/pages/Account/AccountSettings.jsx` around lines 44 - 69, Wrap the tab navigation and content with proper ARIA semantics: add role="tablist" to the nav, give each tab button role="tab" plus an id like `tab-${tab.id}` and set aria-selected={isActive} and aria-controls={`panel-${tab.id}`}; ensure clicking still calls setActiveTab(tab.id). Update the content renderer (renderContent or the element that renders the panels) so each panel has role="tabpanel", id={`panel-${tab.id}`} and aria-labelledby={`tab-${tab.id}`}, and only the active panel is visible to screen readers (hide others with hidden or aria-hidden). Also add a keyboard handler (onKeyDown) on the buttons or nav to support ArrowLeft/ArrowRight/Home/End to change activeTab via setActiveTab for full keyboard tab behavior.client/src/pages/Account/SecuritySettings.jsx (1)
226-231: 💤 Low value"Đăng xuất khỏi các thiết bị khác" button has no handler.
This button renders but isn't wired to any action, so it's a no-op for users. Either wire it to the relevant logout-other-sessions flow or hide it until the backend support exists.
Want me to open an issue to track wiring this button to a session-revocation endpoint?
🤖 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 `@client/src/pages/Account/SecuritySettings.jsx` around lines 226 - 231, The "Đăng xuất khỏi các thiết bị khác" button in SecuritySettings.jsx is missing an onClick handler; add a handler named handleLogoutOtherDevices and wire it to the button (onClick={handleLogoutOtherDevices}). Implement handleLogoutOtherDevices to call the session-revocation API (e.g., POST /api/sessions/revoke-others or an existing revokeOtherSessions helper), show a confirmation prompt, display loading/error/success states (toast or inline message), and refresh the session list/UI on success; if the backend endpoint is not available, hide the button or disable it until the endpoint exists.
🤖 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 `@client/src/api/userApi.js`:
- Around line 8-12: The current truthy checks (e.g., if (data.fullName) ... if
(data.avatar) ...) prevent sending intentional empty values; change them to
explicit undefined checks so fields can be cleared—replace those conditionals to
use "data.fullName !== undefined", "data.phone !== undefined", "data.dateOfBirth
!== undefined", "data.gender !== undefined", and "data.avatar !== undefined"
before calling formData.append in the userApi.js function that builds the
FormData.
In `@client/src/pages/Account/PersonalInfo.jsx`:
- Around line 16-21: The form state is initialized with hardcoded defaults
(gender: 'MALE') and inputs use fallbacks like value={formData.gender ||
displayGender}, causing fetched values to be ignored and preventing clearing;
update PersonalInfo.jsx to seed formData from the fetched profile once the query
returns (e.g., in a useEffect that calls setFormData with the profile fields)
and change all input/select bindings to read directly from formData
(value={formData.fullName}, value={formData.phone},
value={formData.dateOfBirth}, value={formData.gender}) instead of using the OR
fallbacks; ensure setFormData updates inputs on change so submits send the
intended values and do not overwrite gender when unchanged.
- Around line 54-64: handleAvatarChange currently accepts any file; validate the
selected file's MIME type and size before setting state or generating a preview.
Check file.type against allowed types (image/jpeg, image/png, image/gif) and
file.size <= 5 * 1024 * 1024, and if invalid call the app's notification/toast
(e.g., toast.error or showError) to inform the user, then return early without
calling setSelectedFile or setAvatarPreview; only proceed to create a FileReader
and readAsDataURL when the file passes validation. Ensure references to
handleAvatarChange, setSelectedFile and setAvatarPreview are updated
accordingly.
In `@client/src/pages/Account/ProfilesManagement.jsx`:
- Around line 101-103: The User icon lacks an explicit height class causing
layout issues; update the JSX in ProfilesManagement.jsx where the <User
className="w-8 text-gray-300 stroke-1" /> is rendered to include a height class
(e.g., change className to include "h-8") so the icon matches expected
dimensions like the avatar branch and keeps the container layout stable.
- Around line 120-129: The edit button in ProfilesManagement.jsx currently uses
legacy hard-navigation by setting window.location.href =
`/profiles/edit/${profile.id}`, which will 404 because App.jsx only mounts
/account routes; replace this with a client-side navigation using React Router's
useNavigate (or navigate prop) to push an in-/account path (e.g.,
navigate(`/account/profiles/edit/${profile.id}`) or open an account-scoped
modal) from the ProfilesManagement component and remove the window.location.href
assignment; also fix the placeholder User icon by adding a matching height class
(e.g., h-8) to match the existing w-8.
In `@client/src/pages/Account/SecuritySettings.jsx`:
- Around line 68-82: The reuse-password check currently runs unconditionally and
can overwrite earlier empty-field errors: modify the validation so the condition
that sets newErrors.newPassword = 'Mật khẩu mới phải khác mật khẩu hiện tại'
only runs when both formData.currentPassword and formData.newPassword are
non-empty (e.g. check trim() or length > 0) and there is no existing
newErrors.newPassword from prior checks; update the validation block around the
currentPassword === newPassword check to first verify both fields contain text
(and optionally that newErrors.newPassword is not already set) before setting
the reuse error.
In `@server/src/controllers/user.controller.js`:
- Line 41: The code blindly assigns new Date(dateOfBirth) to
updateData.dateOfBirth which allows invalid dates; in the user update handler in
user.controller.js (where updateData is built) validate dateOfBirth first by
constructing a Date object and checking !isNaN(date.getTime()) (or Date.parse)
and if invalid respond with res.status(400).json(...) instead of persisting,
otherwise set updateData.dateOfBirth = date.toISOString()/the Date object as
before.
- Around line 90-100: Add a server-side check that rejects reuse of the current
password by comparing the candidate newPassword against the stored hash before
hashing and updating: after fetching user and before computing
hashedNewPassword, call bcrypt.compare(newPassword, user.password) (similar to
isPasswordCorrect) and if it returns true throw an AppError like 'Mật khẩu mới
phải khác mật khẩu hiện tại' with StatusCodes.BAD_REQUEST; only proceed to
bcrypt.hash(newPassword, 12) and prisma.user.update if that check passes.
---
Nitpick comments:
In `@client/src/pages/Account/AccountSettings.jsx`:
- Around line 44-69: Wrap the tab navigation and content with proper ARIA
semantics: add role="tablist" to the nav, give each tab button role="tab" plus
an id like `tab-${tab.id}` and set aria-selected={isActive} and
aria-controls={`panel-${tab.id}`}; ensure clicking still calls
setActiveTab(tab.id). Update the content renderer (renderContent or the element
that renders the panels) so each panel has role="tabpanel",
id={`panel-${tab.id}`} and aria-labelledby={`tab-${tab.id}`}, and only the
active panel is visible to screen readers (hide others with hidden or
aria-hidden). Also add a keyboard handler (onKeyDown) on the buttons or nav to
support ArrowLeft/ArrowRight/Home/End to change activeTab via setActiveTab for
full keyboard tab behavior.
In `@client/src/pages/Account/SecuritySettings.jsx`:
- Around line 226-231: The "Đăng xuất khỏi các thiết bị khác" button in
SecuritySettings.jsx is missing an onClick handler; add a handler named
handleLogoutOtherDevices and wire it to the button
(onClick={handleLogoutOtherDevices}). Implement handleLogoutOtherDevices to call
the session-revocation API (e.g., POST /api/sessions/revoke-others or an
existing revokeOtherSessions helper), show a confirmation prompt, display
loading/error/success states (toast or inline message), and refresh the session
list/UI on success; if the backend endpoint is not available, hide the button or
disable it until the endpoint exists.
🪄 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: 4481c32b-b280-453d-ad14-38496e3ca66b
📒 Files selected for processing (15)
client/src/App.jsxclient/src/api/userApi.jsclient/src/components/common/AppBar/AppBar.jsxclient/src/pages/Account/AccountSettings.jsxclient/src/pages/Account/PersonalInfo.jsxclient/src/pages/Account/ProfilesManagement.jsxclient/src/pages/Account/SecuritySettings.jsxclient/src/pages/Account/SubscriptionHistory.jsxclient/src/pages/Profile/ProfileForm.jsxclient/src/pages/Profile/ProfileManage.jsxclient/src/pages/Profile/ProfileSelection.jsxclient/src/pages/Profile/index.jsserver/src/controllers/user.controller.jsserver/src/routes/index.jsserver/src/routes/user.route.js
💤 Files with no reviewable changes (4)
- client/src/pages/Profile/ProfileManage.jsx
- client/src/pages/Profile/index.js
- client/src/pages/Profile/ProfileForm.jsx
- client/src/pages/Profile/ProfileSelection.jsx
Comment on lines
+16
to
+21
| const [formData, setFormData] = useState({ | ||
| fullName: '', | ||
| phone: '', | ||
| dateOfBirth: '', | ||
| gender: 'MALE', | ||
| }) |
Contributor
There was a problem hiding this comment.
Form state isn't seeded from the fetched profile, causing gender to be overwritten with MALE.
formData starts with gender: 'MALE' and the other fields empty. Because the select uses value={formData.gender || displayGender} and formData.gender is always truthy ('MALE'), the dropdown ignores the user's actual fetched gender. On submit, updateUserProfileApi appends gender whenever it's truthy, so a user who never touches this field will have their gender silently overwritten to MALE.
The same value={formData.x || displayX} pattern on the text inputs also prevents clearing a field (deleting all characters reverts to the fetched value).
Seed formData from the query result once it loads, then bind inputs directly to formData:
🐛 Proposed fix
-import { useState, useRef } from 'react'
+import { useState, useRef, useEffect } from 'react' const { data: user, isLoading: isUserLoading } = useQuery({
queryKey: ['userProfile'],
queryFn: getUserProfileApi,
})
const [formData, setFormData] = useState({
fullName: '',
phone: '',
dateOfBirth: '',
gender: 'MALE',
})
+
+ useEffect(() => {
+ if (user) {
+ setFormData({
+ fullName: user.fullName || '',
+ phone: user.phone || '',
+ dateOfBirth: user.dateOfBirth ? user.dateOfBirth.split('T')[0] : '',
+ gender: user.gender || 'MALE',
+ })
+ }
+ }, [user])Then bind each control to the state value directly, e.g. value={formData.fullName}, value={formData.gender}, etc.
Also applies to: 66-75, 164-216
🤖 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 `@client/src/pages/Account/PersonalInfo.jsx` around lines 16 - 21, The form
state is initialized with hardcoded defaults (gender: 'MALE') and inputs use
fallbacks like value={formData.gender || displayGender}, causing fetched values
to be ignored and preventing clearing; update PersonalInfo.jsx to seed formData
from the fetched profile once the query returns (e.g., in a useEffect that calls
setFormData with the profile fields) and change all input/select bindings to
read directly from formData (value={formData.fullName}, value={formData.phone},
value={formData.dateOfBirth}, value={formData.gender}) instead of using the OR
fallbacks; ensure setFormData updates inputs on change so submits send the
intended values and do not overwrite gender when unchanged.
Comment on lines
+54
to
+64
| const handleAvatarChange = (e) => { | ||
| const file = e.target.files?.[0] | ||
| if (file) { | ||
| setSelectedFile(file) | ||
| const reader = new FileReader() | ||
| reader.onload = (event) => { | ||
| setAvatarPreview(event.target?.result) | ||
| } | ||
| reader.readAsDataURL(file) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avatar selection isn't validated against the stated constraints.
The UI advertises "JPG, PNG, GIF, max 5MB" but handleAvatarChange accepts any image and any size. Consider validating type/size and toasting an error before previewing, so oversized/unsupported files fail fast on the client rather than only at the server.
🤖 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 `@client/src/pages/Account/PersonalInfo.jsx` around lines 54 - 64,
handleAvatarChange currently accepts any file; validate the selected file's MIME
type and size before setting state or generating a preview. Check file.type
against allowed types (image/jpeg, image/png, image/gif) and file.size <= 5 *
1024 * 1024, and if invalid call the app's notification/toast (e.g., toast.error
or showError) to inform the user, then return early without calling
setSelectedFile or setAvatarPreview; only proceed to create a FileReader and
readAsDataURL when the file passes validation. Ensure references to
handleAvatarChange, setSelectedFile and setAvatarPreview are updated
accordingly.
Comment on lines
+120
to
+129
| <div className="absolute inset-0 bg-black/50 opacity-0 group-hover:opacity-100 flex items-center justify-center gap-2 transition-opacity duration-200"> | ||
| <button | ||
| onClick={() => | ||
| (window.location.href = `/profiles/edit/${profile.id}`) | ||
| } | ||
| className="p-2 bg-blue-600 hover:bg-blue-700 rounded-lg transition-colors" | ||
| title="Chỉnh sửa" | ||
| > | ||
| <Edit2 className="w-4 h-4 text-white" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for the edit route registration and ProfileForm usage in the client router.
rg -nP "profiles/edit|ProfileForm|ProfileManage|ProfileSelection" --type=js --type=jsx -g '!**/node_modules/**'
fd -e jsx -e js . client/src/App.jsx --exec cat {}Repository: LoiTang1710/MovieApp
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search literal route usage =="
rg -n "profiles/edit" client/src -S || true
echo
echo "== Search for /profiles edit path variants =="
rg -n "\/profiles\/edit" client/src -S || true
echo
echo "== Find router setup files =="
rg -n "createBrowserRouter|useRoutes|<Routes>|<Route |react-router-dom" client/src -S || true
echo
echo "== List candidate top-level app/router files (small) =="
fd -e js -e jsx -e ts -e tsx App client/src -t f || true
fd -e js -e jsx -e ts -e tsx routes client/src -t f || trueRepository: LoiTang1710/MovieApp
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== literal route usage: profiles/edit =="
rg -n 'profiles/edit' client/src -S || true
echo
echo "== literal route usage: /profiles/edit =="
rg -n '/profiles/edit' client/src -S || true
echo
echo "== any legacy /profiles prefix usages =="
rg -n '/profiles' client/src -S || true
echo
echo "== locate router setup files =="
rg -l 'react-router-dom' client/src -S || true
rg -l 'createBrowserRouter|useRoutes|<Routes|<Route\b' client/src -S || true
echo
echo "== list likely router/app entry files =="
fd -e js -e jsx -e ts -e tsx App client/src -t f || true
fd -e js -e jsx -e ts -e tsx router client/src -t f || true
fd -e js -e jsx -e ts -e tsx routes client/src -t f || trueRepository: LoiTang1710/MovieApp
Length of output: 2855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find router route table in client/src/App.jsx =="
rg -n 'profiles/edit|\/profiles|profiles' client/src/App.jsx -S || true
echo
rg -n '<Routes>|<Route\b|useRoutes|createBrowserRouter|react-router-dom' client/src/App.jsx -S || true
echo
echo "== Show relevant sections of client/src/App.jsx (around profiles mentions) =="
# If rg found lines, show a window around each; otherwise show first 220 lines as a fallback.
if rg -n 'profiles/edit|\/profiles|profiles' client/src/App.jsx -S >/tmp/app_profiles_hits.txt 2>/dev/null; then
while read -r line; do
ln="$(echo "$line" | cut -d: -f1)"
start=$((ln-20)); end=$((ln+40))
echo "---- App.jsx lines ${start}-${end} (hit at ${ln}) ----"
sed -n "${start},${end}p" client/src/App.jsx
done < /tmp/app_profiles_hits.txt
else
sed -n '1,220p' client/src/App.jsx
fi
echo
echo "== Also search whole client/src for react-router <Route path=\"/profiles\" =="
rg -n 'path\s*=\s*["'\'']/profiles' client/src -S || trueRepository: LoiTang1710/MovieApp
Length of output: 5014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set -x
echo "== ProfilesManagement.jsx around edit button =="
sed -n '90,150p' client/src/pages/Account/ProfilesManagement.jsx
echo
echo "== AccountSettings.jsx =="
sed -n '1,200p' client/src/pages/Account/AccountSettings.jsx
echo
echo "== Search for any nested routing/edit flow under /account (client-side) =="
rg -n "useNavigate|useParams|<Routes|path\s*=\s*['\"]/account|profiles/edit" client/src/pages/Account -S || trueRepository: LoiTang1710/MovieApp
Length of output: 6525
Fix legacy hard-navigation from ProfilesManagement edit button
client/src/pages/Account/ProfilesManagement.jsx sets window.location.href = \/profiles/edit/${profile.id}`, but client/src/App.jsxonly registers the/accountroute (viaAccountSettings) and contains no /profiles/edit/:id(or other/profiles/*) client routes—this will 404 and forces a full reload. Use an in-/accountedit flow (e.g., React Router withuseNavigateto an/account/...` route or a modal) instead.
- Minor: the placeholder
<User />icon hasw-8but no matchingh-*class.
🤖 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 `@client/src/pages/Account/ProfilesManagement.jsx` around lines 120 - 129, The
edit button in ProfilesManagement.jsx currently uses legacy hard-navigation by
setting window.location.href = `/profiles/edit/${profile.id}`, which will 404
because App.jsx only mounts /account routes; replace this with a client-side
navigation using React Router's useNavigate (or navigate prop) to push an
in-/account path (e.g., navigate(`/account/profiles/edit/${profile.id}`) or open
an account-scoped modal) from the ProfilesManagement component and remove the
window.location.href assignment; also fix the placeholder User icon by adding a
matching height class (e.g., h-8) to match the existing w-8.
|
|
||
| if (fullName !== undefined) updateData.fullName = fullName | ||
| if (phone !== undefined) updateData.phone = phone | ||
| if (dateOfBirth !== undefined) updateData.dateOfBirth = new Date(dateOfBirth) |
Contributor
There was a problem hiding this comment.
Validate dateOfBirth before persisting.
new Date(dateOfBirth) accepts invalid input and can lead to runtime/DB errors. Return a 400 on invalid dates instead of failing deeper.
💡 Proposed fix
- if (dateOfBirth !== undefined) updateData.dateOfBirth = new Date(dateOfBirth)
+ if (dateOfBirth !== undefined) {
+ const parsedDate = new Date(dateOfBirth)
+ if (Number.isNaN(parsedDate.getTime())) {
+ throw new AppError('Ngày sinh không hợp lệ', StatusCodes.BAD_REQUEST)
+ }
+ updateData.dateOfBirth = parsedDate
+ }🤖 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 `@server/src/controllers/user.controller.js` at line 41, The code blindly
assigns new Date(dateOfBirth) to updateData.dateOfBirth which allows invalid
dates; in the user update handler in user.controller.js (where updateData is
built) validate dateOfBirth first by constructing a Date object and checking
!isNaN(date.getTime()) (or Date.parse) and if invalid respond with
res.status(400).json(...) instead of persisting, otherwise set
updateData.dateOfBirth = date.toISOString()/the Date object as before.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/pages/Account/ProfilesManagement.jsx (1)
18-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle profile-query failures explicitly.
When the fetch fails, the component only checks loading state and then renders as if data were empty. Add an error branch so users see a clear failure state instead of a silent empty grid.
Proposed fix
- const { data: profiles = [], isLoading } = useQuery({ + const { data: profiles = [], isLoading, isError, error } = useQuery({ queryKey: ['userProfiles'], queryFn: getProfilesApi, }) @@ if (isLoading) { @@ } + + if (isError) { + return ( + <div className="rounded-lg border border-red-500/30 bg-red-500/10 p-4 text-sm text-red-200"> + {error?.response?.data?.message || 'Không thể tải danh sách hồ sơ.'} + </div> + ) + }Also applies to: 67-73
🤖 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 `@client/src/pages/Account/ProfilesManagement.jsx` around lines 18 - 21, The profile fetch currently ignores query errors; update the useQuery call(s) that destructure profiles to also return isError and error (e.g., const { data: profiles = [], isLoading, isError, error } = useQuery({ queryKey: ['userProfiles'], queryFn: getProfilesApi })) and add an explicit error branch in the component render (and the other identical query usage around the 67-73 area) to show a clear failure UI/message using the error details instead of rendering an empty grid when the fetch fails; ensure any existing loading and empty-state logic remains but is reached only when isLoading is false and isError is false.
♻️ Duplicate comments (1)
client/src/pages/Account/ProfilesManagement.jsx (1)
122-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace legacy hard navigation for edit action.
Line 123 still forces
window.location.hrefto/profiles/edit/${profile.id}. In this migration, legacy/profiles/*flows were removed in favor of/account, so this can trigger a full reload and route miss. Use React Router navigation within the account flow instead.Proposed fix
-import { useState } from 'react' +import { useState } from 'react' +import { useNavigate } from 'react-router-dom' @@ export default function ProfilesManagement() { + const navigate = useNavigate() @@ - <button - onClick={() => - (window.location.href = `/profiles/edit/${profile.id}`) - } + <button + onClick={() => navigate(`/account/profiles/edit/${profile.id}`)} className="p-2 bg-blue-600 hover:bg-blue-700 rounded-lg transition-colors" title="Chỉnh sửa" >🤖 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 `@client/src/pages/Account/ProfilesManagement.jsx` around lines 122 - 124, Replace the legacy full-page navigation using window.location.href in the onClick handler with React Router's navigation: import and call useNavigate() in ProfilesManagement.jsx (e.g., const navigate = useNavigate()) and change the handler from (window.location.href = `/profiles/edit/${profile.id}`) to navigate('/account/profiles/edit/' + profile.id) (or the exact account-scoped route used by your app). Ensure useNavigate is imported from react-router-dom and the component calls navigate instead of mutating window.location.
🧹 Nitpick comments (1)
client/src/pages/Account/SecuritySettings.jsx (1)
230-235: 💤 Low valueUnwired button may confuse users.
The "Đăng xuất khỏi các thiết bị khác" button has no
onClickhandler and does nothing when clicked. Consider either wiring it to the actual logout-all-sessions functionality, adding adisabledstate with a tooltip indicating it's coming soon, or removing it until the feature is implemented.🤖 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 `@client/src/pages/Account/SecuritySettings.jsx` around lines 230 - 235, The "Đăng xuất khỏi các thiết bị khác" button in SecuritySettings.jsx is interactive but has no onClick handler; either wire it to the logout-all-sessions flow by calling a new handler (e.g., implement handleLogoutAllSessions that calls your logoutAllSessions API/mutation and shows success/error toasts) and attach it to the button, or mark the button disabled and add a tooltip/aria-label indicating the feature is coming soon; update the button element (in SecuritySettings.jsx) to use the chosen approach and ensure accessibility (disabled attribute or proper click handler and loading state).
🤖 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 `@client/src/pages/Home/Home.jsx`:
- Line 65: The Sidebar conditional never renders because isOpen is never set
true; add a visible trigger that calls setIsOpen(true) (for example a button or
icon near the header) and keep the existing Sidebar onClick handler to close it,
or if you prefer remove the dead branch. Locate the isOpen state hook
(useState(...) that defines isOpen and setIsOpen) and the JSX branch {isOpen &&
<Sidebar onClick={() => setIsOpen(!isOpen)} />} and implement a trigger element
that invokes setIsOpen(true) so the Sidebar becomes reachable.
In `@client/src/pages/Movies/Movies.jsx`:
- Around line 11-15: The Movies component is reading
isLoginModalOpen/setIsLoginModalOpen from useHome which no longer provides them
and also mounts the LoginModal that is already globally rendered in AppContent;
remove the useHome import and any destructuring of
isLoginModalOpen/setIsLoginModalOpen from the Movies component and stop
rendering a local LoginModal to avoid duplicate mounts and undefined onClose
calls. If Movies must trigger the modal, switch to URL-driven state (e.g.,
useSearchParams or router navigation) to set/clear a query like ?login=true and
wire the trigger to update that query; ensure any onClose logic removes the
query so the global LoginModal in AppContent controls visibility.
---
Outside diff comments:
In `@client/src/pages/Account/ProfilesManagement.jsx`:
- Around line 18-21: The profile fetch currently ignores query errors; update
the useQuery call(s) that destructure profiles to also return isError and error
(e.g., const { data: profiles = [], isLoading, isError, error } = useQuery({
queryKey: ['userProfiles'], queryFn: getProfilesApi })) and add an explicit
error branch in the component render (and the other identical query usage around
the 67-73 area) to show a clear failure UI/message using the error details
instead of rendering an empty grid when the fetch fails; ensure any existing
loading and empty-state logic remains but is reached only when isLoading is
false and isError is false.
---
Duplicate comments:
In `@client/src/pages/Account/ProfilesManagement.jsx`:
- Around line 122-124: Replace the legacy full-page navigation using
window.location.href in the onClick handler with React Router's navigation:
import and call useNavigate() in ProfilesManagement.jsx (e.g., const navigate =
useNavigate()) and change the handler from (window.location.href =
`/profiles/edit/${profile.id}`) to navigate('/account/profiles/edit/' +
profile.id) (or the exact account-scoped route used by your app). Ensure
useNavigate is imported from react-router-dom and the component calls navigate
instead of mutating window.location.
---
Nitpick comments:
In `@client/src/pages/Account/SecuritySettings.jsx`:
- Around line 230-235: The "Đăng xuất khỏi các thiết bị khác" button in
SecuritySettings.jsx is interactive but has no onClick handler; either wire it
to the logout-all-sessions flow by calling a new handler (e.g., implement
handleLogoutAllSessions that calls your logoutAllSessions API/mutation and shows
success/error toasts) and attach it to the button, or mark the button disabled
and add a tooltip/aria-label indicating the feature is coming soon; update the
button element (in SecuritySettings.jsx) to use the chosen approach and ensure
accessibility (disabled attribute or proper click handler and loading state).
🪄 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: 992d5eee-9c7b-4318-8a2d-a31b0dd9489a
📒 Files selected for processing (11)
client/src/App.jsxclient/src/api/userApi.jsclient/src/components/common/ActionButton/FavouriteButton/FavouriteButton.jsxclient/src/components/common/AppBar/AppBar.jsxclient/src/pages/Account/ProfilesManagement.jsxclient/src/pages/Account/SecuritySettings.jsxclient/src/pages/Home/Home.jsxclient/src/pages/Movies/Movies.jsxclient/src/providers/AuthProvider.jsxclient/src/providers/HomeProvider.jsxserver/src/controllers/user.controller.js
| onClose={() => setIsLoginModalOpen(false)} | ||
| message="Bạn cần đăng nhập để lưu phim vào danh sách yêu thích." | ||
| /> | ||
| {isOpen && <Sidebar onClick={() => setIsOpen(!isOpen)} />} |
Contributor
There was a problem hiding this comment.
Sidebar toggle is unreachable in current state flow.
isOpen is never set to true, so this branch never renders and users cannot open it. Add an explicit trigger button (or remove this state/branch until wired).
💡 Minimal wiring example
+ <button
+ type="button"
+ onClick={() => setIsOpen((prev) => !prev)}
+ className="fixed bottom-4 right-4 z-50"
+ >
+ <Sidebar />
+ </button>
<MediaCollection />
- {isOpen && <Sidebar onClick={() => setIsOpen(!isOpen)} />}
+ {isOpen && <Sidebar onClick={() => setIsOpen(false)} />}🤖 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 `@client/src/pages/Home/Home.jsx` at line 65, The Sidebar conditional never
renders because isOpen is never set true; add a visible trigger that calls
setIsOpen(true) (for example a button or icon near the header) and keep the
existing Sidebar onClick handler to close it, or if you prefer remove the dead
branch. Locate the isOpen state hook (useState(...) that defines isOpen and
setIsOpen) and the JSX branch {isOpen && <Sidebar onClick={() =>
setIsOpen(!isOpen)} />} and implement a trigger element that invokes
setIsOpen(true) so the Sidebar becomes reachable.
Comment on lines
+11
to
15
| import { useHome } from '../../contexts/HomeContext.jsx' | ||
|
|
||
| const Movies = () => { | ||
| const { isLoginModalOpen, setIsLoginModalOpen } = useHome() | ||
| // 1. Quản lý State qua URL |
Contributor
There was a problem hiding this comment.
Fix modal state source mismatch (and avoid duplicate modal mount).
Line 14 reads modal state from useHome(), but HomeProvider no longer provides isLoginModalOpen/setIsLoginModalOpen. This leaves onClose calling undefined and breaks the login-modal flow on this page. Also, the modal is already mounted globally in AppContent.
Suggested fix
-import RequireLoginModal from '../../components/common/Modals/RequireLoginModal.jsx'
-import { useHome } from '../../contexts/HomeContext.jsx'
+import { useAuth } from '../../hooks/useAuth.jsx'
const Movies = () => {
- const { isLoginModalOpen, setIsLoginModalOpen } = useHome()
+ const { setIsLoginModalOpen } = useAuth()
@@
- <RequireLoginModal
- isOpen={isLoginModalOpen}
- onClose={() => setIsLoginModalOpen(false)}
- message="Bạn cần đăng nhập để lưu phim vào danh sách yêu thích."
- />Also applies to: 98-102
🤖 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 `@client/src/pages/Movies/Movies.jsx` around lines 11 - 15, The Movies
component is reading isLoginModalOpen/setIsLoginModalOpen from useHome which no
longer provides them and also mounts the LoginModal that is already globally
rendered in AppContent; remove the useHome import and any destructuring of
isLoginModalOpen/setIsLoginModalOpen from the Movies component and stop
rendering a local LoginModal to avoid duplicate mounts and undefined onClose
calls. If Movies must trigger the modal, switch to URL-driven state (e.g.,
useSearchParams or router navigation) to set/clear a query like ?login=true and
wire the trigger to update that query; ensure any onClose logic removes the
query so the global LoginModal in AppContent controls visibility.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
User Experience
Removed