fix(edit-user): redirect current user to profile page when accessing … #2743
Conversation
…self via team route
Mohansai-M
requested review from
ajhollid,
mohicody,
Br0wnHammer,
Owaiseimdad,
karenvicent and
shanikauwu1
as code owners
WalkthroughIntroduces a redirect in EditUser to prevent the authenticated user from editing their own account (guard implemented twice in the component). TeamPanel uses optional chaining on Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditUserPage
participant ReduxStore
participant Router
User->>EditUserPage: Navigate to /account/edit/:userId
EditUserPage->>ReduxStore: Read currentUser (state.auth.user)
EditUserPage->>EditUserPage: Compare route userId with currentUser._id
alt userId == currentUser._id
EditUserPage->>Router: Navigate to /account/profile (replace)
else
EditUserPage->>EditUserPage: Render edit form for requested userId
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
(Also acceptable approach per issue — redirect implemented.) — Why did the Canadian refuse to edit their own profile on the wrong page? They politely redirected themselves, eh. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: Fixes self-editing via team route by redirecting current users to profile page
- Key components modified: EditUser page and TeamPanel component
- Cross-component impacts: Affects user management flows and navigation behavior
- Business value alignment: Improves UX consistency and prevents state management conflicts
1.2 Technical Architecture
- System design modifications: Added route interception logic in EditUser component
- Component interaction changes: TeamPanel now safely handles undefined members array
- Integration points impact: Maintains separation between profile/team editing contexts
- Dependency changes: Added navigation hooks and Redux selectors
2. Critical Findings
2.1 Must Fix (P0🔴)
No critical issues found requiring immediate resolution
2.2 Should Fix (P1🟡)
Issue: Missing loading state during user validation in EditUser
- Analysis Confidence: High
- Impact: Potential UI flicker during auth resolution before redirect
- Suggested Solution:
// Add before useEffect redirect logic
if (!currentUser) return <LoadingSpinner />;Issue: Undefined team state handling in TeamPanel
- Analysis Confidence: High
- Impact: May cause rendering errors when members data is unavailable
- Suggested Solution:
// Replace current setData(team) with:
setData(team || []);2.3 Consider (P2🟢)
Area: Centralize user comparison logic
- Analysis Confidence: Medium
- Improvement Opportunity: Reduces code duplication and improves maintainability
Area: Add redirect confirmation UX
- Analysis Confidence: Medium
- Improvement Opportunity: Better user communication about navigation changes
Area: Enhance test coverage
- Analysis Confidence: High
- Improvement Opportunity: Validate redirect behavior and null member handling
2.4 Summary of Action Items
- Add loading state to EditUser (P1 - immediate)
- Ensure TeamPanel always returns array (P1 - immediate)
- Centralize user comparison logic (P2 - future sprint)
- Add redirect notification (P2 - future sprint)
- Expand test coverage (P2 - next release)
3. Technical Analysis
3.1 Code Logic Analysis
📁 client/src/Pages/Account/EditUser/index.jsx - EditUser Component
- Submitted PR Code:
useEffect(() => {
if (currentUser?._id && userId === currentUser._id) {
navigate("/account/profile");
}
}, [userId, currentUser?._id, navigate]);- Analysis:
- Correctly implements redirect logic with proper dependency array
- Safely handles null currentUser with optional chaining
- Prevents UI state conflicts by enforcing profile-based self-edits
- Business logic aligns with UX consistency requirements
- LlamaPReview Suggested Improvements:
// Add loading state before useEffect
if (!currentUser) return <LoadingSpinner />;
useEffect(() => {
if (currentUser?._id && userId === currentUser._id) {
// Optional: Add UX notification
// showToast(t("redirect.selfEditPrevented"));
navigate("/account/profile");
}
}, [userId, currentUser?._id, navigate]);- Improvement rationale:
- Prevents UI flicker during authentication resolution
- Provides better user feedback about navigation changes
- Maintains smooth UX during redirect process
📁 client/src/Pages/Account/components/TeamPanel.jsx - TeamPanel Data Processing
- Submitted PR Code:
team = members?.filter((member) => { ... });
team = team?.map((member) => ({ ... }));
setData(team);- Analysis:
- Correctly prevents errors with optional chaining
- Handles initial undefined/null states safely
- Maintains functional chaining pattern
- Business logic ensures stable rendering
- LlamaPReview Suggested Improvements:
team = members?.filter((member) => { ... }) || [];
team = team?.map((member) => ({ ... })) || [];
setData(team);- Improvement rationale:
- Guarantees data always resolves to array
- Prevents undefined state in downstream components
- Improves rendering stability during data loading
4. Overall Evaluation
- Technical assessment: Well-implemented solution with minor edge case improvements needed
- Business impact: Resolves critical UX inconsistency and prevents state conflicts
- Risk evaluation: Low risk with clear scope and limited surface area
- Notable positive aspects: Clean hook usage, proper dependency management, defensive programming
- Implementation quality: Follows project conventions and maintains separation of concerns
- Final recommendation: Approve with suggested P1 improvements
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
There was a problem hiding this comment.
I appreciate the current approach, but there's still a brief moment when users can see the edit page, which we need to eliminate. Our goal is to ensure that users never encounter this page at all not even for a second as any visibility of this page is considered a poor user experience. Could you find a solution that completely bypasses this page for users?
There was a problem hiding this comment.
I think you can dispense with the useEffect hook here and just render a redirect if the user IDs match.
This should avoid the issue @Br0wnHammer mentioned.
| navigate("/account/profile"); | ||
| } | ||
| }, [userId, currentUser?._id, navigate]); | ||
|
|
There was a problem hiding this comment.
I don't think a useEffect hook is necessary here, I think it is sufficient to render a redirect if userId matches currentUser._id
…self via team route
|
I've updated the logic as requested.it now ensures a smooth redirect without rendering the edit page. |
|
@Mohansai-M can you share a video of the flow? |
There was a problem hiding this comment.
LGTM! We should be good to go. After @Mohansai-M shares a video of the flow.
There was a problem hiding this comment.
The concept here is on target, however there is a violation of the React rules of hooks here, specifically the conditional calling of hooks.
There's also an unused import that should be removed. Please be sure to lint code before submitting PRs using the included eslint configuration. Thanks!
| import { useTheme } from "@emotion/react"; | ||
| import { useTranslation } from "react-i18next"; | ||
| import { useGetUser, useEditUser } from "../../../Hooks/userHooks"; | ||
| import { EDITABLE_ROLES, ROLES } from "../../../Utils/roleUtils"; | ||
| import { useEditUserForm, useValidateEditUserForm } from "./hooks/editUser"; | ||
| import { useSelector } from "react-redux"; | ||
| import { useEffect } from "react"; |
There was a problem hiding this comment.
This dependency is unused, please do remove it
| replace | ||
| /> | ||
| ); | ||
| } | ||
| const BREADCRUMBS = [ |
There was a problem hiding this comment.
Conditionally returning a redirect before calling hooks is a violation of the rules of hooks.
This redirect should occur only after hooks have been called. Please use our include .eslintrc config to lint your code before submitting PRs, these types of errors should be caught by the linter.
|
@Mohansai-M did you still want to work on this PR? |
I am working on this will complete it by tomorrow as per requirement |
Cool, thank you! 👌 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/src/Pages/Account/EditUser/index.jsx (1)
26-33: Remove early redirect before calling all hooks (Rules of Hooks violation)Returning before all hooks in this component are called can cause hook order mismatches across renders (e.g., when currentUser loads after the first render). This was previously flagged by ajhollid and should be resolved by keeping only the post-hook guard.
Apply this diff to remove the early return and rely on the later guard:
- if (currentUser?._id && userId === currentUser._id) { - return ( - <Navigate - to="/account/profile" - replace - /> - ); - }
🧹 Nitpick comments (1)
client/src/Pages/Account/EditUser/index.jsx (1)
51-58: LGTM: Redirecting after all hooks are called is the correct placementThis guard location respects the Rules of Hooks and addresses the earlier feedback. Optionally, you could compute a small isSelf boolean once for readability, but that’s purely a nit.
Optional micro-refactor:
- Introduce const isSelf = currentUser?._id === userId; near other consts, and then use if (isSelf) { return <Navigate .../> } to keep the condition DRY and self-documenting. Americans might call it “syntactic candy,” Canadians might say it’s “a polite touch.” Either way, it reads nicely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/src/Pages/Account/EditUser/index.jsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/src/Pages/Account/EditUser/index.jsx (3)
client/src/Pages/Account/index.jsx (1)
useSelector(26-26)client/src/Pages/Account/components/ProfilePanel.jsx (1)
useSelector(32-32)client/src/Hooks/useIsAdmin.js (2)
useSelector(4-4)useSelector(12-12)
🔇 Additional comments (3)
client/src/Pages/Account/EditUser/index.jsx (3)
12-12: Importing Navigate is appropriate for this redirect use caseUsing Navigate from react-router-dom is the right call for declarative redirects here. The replace prop below avoids back-button ping-pong. Smooth as a Canadian zipper merge.
18-18: Using useSelector to source currentUser is consistent with the codebaseImporting useSelector aligns with existing patterns (see other account hooks). No concerns.
24-24: Selector path is correctSelecting state.auth.user matches the surrounding codebase usage. Naming is clear and consistent.
|
Hi @ajhollid I have updated the code please review it |
…self via team route
(Please remove this line only before submitting your PR. Ensure that all relevant items are checked before submission.)
Describe your changes
Briefly describe the changes you made and their purpose.
Added a check to detect if the current user is trying to access their own edit page via the /account/team/:userId route.
Implemented a redirect to /account/profile when self-edit is attempted from the team route.
Ensures consistent user experience by handling self-edit only through the profile page.
Prevents UI issues where Redux state wouldn’t update properly on self-edit via team view.
🔗 Attached a short video demonstrating the behavior before and after the fix for clarity.
Write your issue number after "Fixes "
Fixes #2716
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Edit.user.mp4
Summary by CodeRabbit