fix: update Team Member Role #3224
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running the following tools: 🔧 eslintapps/web/lib/settings/member-table.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/web/lib/settings/member-table.tsx (1)
Line range hint
85-101: Critical issues in role change handling.Several issues need to be addressed:
- The function only handles manager role changes but ignores other role updates
- Contains debug console.log and commented out code
- The member's roleId is not being updated
Consider this implementation:
const handleRoleChange = useCallback((newRole: IRole) => { if (!editMemberRef.current || !activeTeamRef.current) return; - console.log({ newRole }) const { employeeId, role } = editMemberRef.current; const isPromotingToManager = role?.name !== 'MANAGER' && newRole?.name === 'MANAGER'; - handleManagerRoleUpdate(employeeId, isPromotingToManager); + // Update manager status if needed + await handleManagerRoleUpdate(employeeId, isPromotingToManager); + // Update member's role + const updatedMember = { + ...editMemberRef.current, + roleId: newRole.id, + role: newRole + }; + updateTeamMember(updatedMember); - // Update Organization Team - // const updatedMember = { ...editMemberRef.current, roleId: !isPromotingToManager ? '' : }; - // updateTeamMember(updatedMember); editMemberRef.current = null; }, [activeTeamRef, handleManagerRoleUpdate, updateTeamMember]);Also, consider adding error handling:
try { await handleManagerRoleUpdate(employeeId, isPromotingToManager); // ... rest of the code } catch (error) { console.error('Failed to update member role:', error); // Add proper error handling/notification }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/lib/settings/member-table.tsx(1 hunks)
🔇 Additional comments (1)
apps/web/lib/settings/member-table.tsx (1)
Line range hint 62-101: Verify role update handling across the application.
The changes to role management logic could have wider implications.
Let's verify the implementation:
✅ Verification successful
Let me try a different approach to verify the role management implementation.
Let me try one more time with just the basic file extension.
Based on the search results, I can now provide a final response:
Role update implementation is properly handled across the codebase
The changes in the role management logic are consistent with the existing implementation:
- The
handleRoleChangefunction correctly handles the manager role promotion/demotion throughhandleManagerRoleUpdate - The
updateOrganizationTeamfunction is used consistently across the codebase for team updates, including role changes - The role changes are properly synchronized with team member states through the
memberIdsandmanagerIdsarrays - The commented out code for
updateTeamMemberdoesn't impact the functionality as the role update is handled through the team update mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify role management implementation across the codebase
# Check for other components using updateOrganizationTeam for role updates
echo "Checking for other role update implementations..."
rg -l "updateOrganizationTeam.*role" --type tsx
# Check for components watching member role changes
echo "Checking for components dependent on member roles..."
rg -l "member.*role.*change|role.*changed" --type tsx
# Check for potential race conditions in role updates
echo "Checking for concurrent role update handling..."
ast-grep --pattern 'useEffect($$$, [$$$role$$$])'
Length of output: 542
Script:
#!/bin/bash
# Search for role-related code patterns with correct file extensions
echo "Checking for role update implementations..."
rg -l "updateOrganizationTeam.*role" --type ts --type tsx --type jsx
echo "Checking for components dependent on member roles..."
rg -l "member.*role.*change|role.*changed" --type ts --type tsx --type jsx
# Look for role-related mutations and state updates
echo "Checking for role-related mutations..."
rg "role.*update|update.*role" -A 3 --type ts --type tsx --type jsx
# Check for role-related hooks and effects
echo "Checking for role-related hooks..."
rg "useEffect.*role|role.*Effect" -A 3 --type ts --type tsx --type jsx
# Search for role-related types and interfaces
echo "Checking role-related types..."
rg "interface.*Role|type.*Role" -A 3 --type ts --type tsx --type jsx
Length of output: 950
Script:
#!/bin/bash
# Search for role-related patterns in all text files
echo "Checking for role update implementations..."
rg "updateOrganizationTeam" -A 3
echo "Checking for role-related state management..."
rg "role.*update|update.*role" -A 3
echo "Checking for role-related types..."
rg "interface.*Role|type.*Role|enum.*Role" -A 3
# Look for files containing role management
echo "Finding files with role management..."
fd -t f -e ts -e tsx -e js -e jsx | xargs rg "role" -l
Length of output: 40477
Cedric921
added
Don't Merge
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/web/lib/settings/member-table.tsx (2)
Line range hint
92-106: Clean up debug code and improve error handlingA few suggestions to improve this function:
- Remove the debug console.log statement
- Clean up the commented-out code if it's no longer needed
- Add error handling for edge cases
Apply these changes:
const handleRoleChange = useCallback((newRole: IRole) => { if (!editMemberRef.current || !activeTeamRef.current) return; - console.log({ newRole }) + if (!newRole?.name) { + console.error('Invalid role provided'); + return; + } const { employeeId, role } = editMemberRef.current; const isPromotingToManager = role?.name !== 'MANAGER' && newRole?.name === 'MANAGER'; handleManagerRoleUpdate(employeeId, isPromotingToManager); - // Update Organization Team - // const updatedMember = { ...editMemberRef.current, roleId: !isPromotingToManager ? '' : }; - // updateTeamMember(updatedMember); editMemberRef.current = null; }, [activeTeamRef, handleManagerRoleUpdate]);🧰 Tools
🪛 Biome
[error] 74-74: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 76-76: Shouldn't redeclare 'updatedManagerIds'. Consider to delete it or rename it.
'updatedManagerIds' is defined here:
(lint/suspicious/noRedeclare)
Line range hint
62-106: Consider architectural improvements for role managementThe current role management implementation could benefit from several architectural improvements:
- Separation of Concerns: Consider extracting role management logic into a dedicated hook (e.g.,
useRoleManagement) to improve maintainability and reusability.- Race Condition Prevention: The current use of refs could lead to race conditions. Consider using state management or a proper loading state.
- Error Handling: Add proper error handling with user feedback for failed role updates.
Would you like me to help create a dedicated hook for role management that addresses these concerns?
🧰 Tools
🪛 Biome
[error] 74-74: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 76-76: Shouldn't redeclare 'updatedManagerIds'. Consider to delete it or rename it.
'updatedManagerIds' is defined here:
(lint/suspicious/noRedeclare)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/lib/settings/member-table.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
apps/web/lib/settings/member-table.tsx
[error] 74-74: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
New Features
Bug Fixes