BA-2123: Multiple Profiles - Removing Member #203
Conversation
|
|
Warning Rate limit exceeded@joaosoutto has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
WalkthroughThis pull request introduces a new GraphQL mutation and corresponding custom hook to remove a member from a profile. The changes add the necessary UI logic in the Member components to trigger a confirmation dialog before removal, update state management, and handle success/error notifications. Additionally, optional chaining is applied in list item components for safer property access, member status and role definitions are restructured, and minor updates are made to the lazy query and loading states. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MemberItem
participant ConfirmDialog
participant MutationHook
participant GraphQLServer
participant Notifier
User->>MemberItem: Select "Remove" action
MemberItem->>ConfirmDialog: Open confirmation dialog
User->>ConfirmDialog: Confirm removal
ConfirmDialog->>MemberItem: Trigger confirmRemoveProfileMember
MemberItem->>MutationHook: Call removeProfileMember (commit)
MutationHook->>GraphQLServer: Execute RemoveMember mutation
GraphQLServer-->>MutationHook: Return deletion response
MutationHook->>MemberItem: Notify success/error
MemberItem->>Notifier: sendToast (success/error)
Possibly related PRs
Suggested reviewers
Poem
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 (
|
joaosoutto
requested review from
deboracosilveira,
anicioalexandre and
hpmoreira05
and removed request for
deboracosilveira,
anicioalexandre and
hpmoreira05
joaosoutto
requested review from
deboracosilveira and
anicioalexandre
and removed request for
anicioalexandre and
deboracosilveira
joaosoutto
force-pushed
the
BA--2123-multiple-profiles-remove-member
branch
from
9de7851 to
b63a6d6
Compare
joaosoutto
requested review from
deboracosilveira and
priscilladeroode
as code owners
![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
🧹 Nitpick comments (4)
packages/components/modules/profiles/web/ProfileMembers/constants.ts (1)
29-32: Consider using a separate array for destructive actions.While the implementation works, mixing the remove action with role options might not be the best UX pattern. Consider separating destructive actions from role changes.
-export const roleOptions = [ +export const roleOptions = [ { value: MemberRoles.admin, label: capitalizeFirstLetter(MemberRoles.admin.toLowerCase()), }, { value: MemberRoles.manager, label: capitalizeFirstLetter(MemberRoles.manager.toLowerCase()), }, +] + +export const memberActions = [ { value: MemberActions.remove, label: capitalizeFirstLetter(MemberActions.remove.toLowerCase()), }, ]packages/components/modules/profiles/common/graphql/mutations/RemoveMember.ts (1)
15-38: Consider enhancing success notification message.The success message could be more specific by including the removed member's name.
- sendToast('Member removed successfully', { type: 'success' }) + sendToast(`${config.variables?.input?.memberName || 'Member'} has been removed from the organization`, { type: 'success' })packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (2)
36-38: LGTM! Consider adding error handling for the removal mutation.The implementation is well-structured with proper null checks and clear separation of concerns.
Consider adding error handling for failed removals:
const removeProfileMember = () => { if (currentProfile?.id && userId) { removeMember({ variables: { input: { profileId: currentProfile.id, userId } }, + onError: (error) => { + console.error('Failed to remove member:', error); + }, }) } }Also applies to: 47-64
103-114: Consider using more specific cancel button text.The implementation matches the requirements well, with the remove option in red and the correct confirmation message. However, the cancel button text could be more specific.
Consider changing the cancel button text to be more descriptive:
- cancelText="Back" + cancelText="Cancel"Also applies to: 168-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/RemoveMemberMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (5)
packages/components/modules/profiles/common/graphql/mutations/RemoveMember.ts(1 hunks)packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx(6 hunks)packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileMembers/constants.ts(2 hunks)packages/components/modules/profiles/web/ProfileMembers/index.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Unit Test Packages
🔇 Additional comments (5)
packages/components/modules/profiles/web/ProfileMembers/constants.ts (1)
16-18: LGTM! Clean implementation of member removal action.The
MemberActionsenum provides a clear and type-safe way to represent the member removal action.packages/components/modules/profiles/common/graphql/mutations/RemoveMember.ts (1)
7-13: LGTM! Well-structured GraphQL mutation.The mutation is correctly defined with proper input type and deletion handling.
packages/components/modules/profiles/web/ProfileMembers/index.tsx (1)
40-40: LGTM! Improved loading state presentation.The size customization for the loading indicator enhances the user experience.
packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx (1)
23-26: LGTM! Enhanced null safety with optional chaining.The addition of optional chaining operators improves code robustness by safely handling potential null/undefined values.
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (1)
7-7: LGTM! Imports are well-organized and support the new removal functionality.The imports correctly include all necessary components and hooks for implementing the member removal feature.
Also applies to: 12-13
packages/components/modules/profiles/web/ProfileMembers/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/web/ProfileMembers/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/web/ProfileMembers/constants.ts
Outdated
Show resolved
Hide resolved
joaosoutto
force-pushed
the
BA--2123-multiple-profiles-remove-member
branch
from
b63a6d6 to
803aa3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (1)
103-110: Improve type safety in the Select onChange handler.The current implementation uses type casting which could be made more type-safe.
Consider this implementation:
-onChange={(event, _) => { - const { value } = event.target +onChange={(event: SelectChangeEvent<MemberRoles | MemberActions>) => { + const value = event.target.value if (value === MemberActions.remove) { handleRemoveMemberDialog() } else { - handleRoleChange(event as SelectChangeEvent<{ value: MemberRoles }>) + handleRoleChange(event) } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/RemoveMemberMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (5)
packages/components/modules/profiles/common/graphql/mutations/RemoveMember.ts(1 hunks)packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx(6 hunks)packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileMembers/constants.ts(2 hunks)packages/components/modules/profiles/web/ProfileMembers/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/modules/profiles/web/ProfileMembers/constants.ts
- packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx
- packages/components/modules/profiles/web/ProfileMembers/index.tsx
- packages/components/modules/profiles/common/graphql/mutations/RemoveMember.ts
🔇 Additional comments (3)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (3)
36-38: LGTM! Mutation and state management are well implemented.The new mutation hook and state for member removal are properly initialized with appropriate types.
117-123: LGTM! UI implementation matches requirements.The remove action is correctly styled in red using the theme's error color, matching the PR requirements.
168-184: LGTM! Confirmation dialog implementation matches requirements perfectly.The confirmation dialog:
- Uses the exact message specified in requirements
- Maintains consistent error color theme for the remove action
- Provides clear cancel and confirm options
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (1)
48-54:⚠️ Potential issueAdd error handling and success feedback.
The member removal function should handle errors and provide feedback to users.
const removeProfileMember = () => { if (currentProfile?.id && userId) { removeMember({ variables: { input: { profileId: currentProfile.id, userId } }, + onCompleted: () => { + // Show success notification + enqueueSnackbar('Member removed successfully', { variant: 'success' }) + }, + onError: (error) => { + // Show error notification + enqueueSnackbar(error.message || 'Failed to remove member', { variant: 'error' }) + } }) } }
🧹 Nitpick comments (2)
packages/components/modules/profiles/web/ProfileMembers/MembersList/index.tsx (1)
103-103: Consider improving type alignment to avoid explicit casting.While the type cast works, it suggests a potential type mismatch between
MEMBER_STATUSESandProfileRoleStatus. Consider definingMEMBER_STATUSESwithProfileRoleStatustype to avoid explicit casting.-import { MEMBER_STATUSES, NUMBER_OF_MEMBERS_TO_LOAD_NEXT } from '../constants' +import { MEMBER_STATUSES, NUMBER_OF_MEMBERS_TO_LOAD_NEXT } from '../constants' + +// In ../constants.ts: +export const MEMBER_STATUSES: Record<string, ProfileRoleStatus> = { + active: 'ACTIVE', + // ... other statuses +} as constpackages/components/modules/profiles/web/ProfileMembers/constants.ts (1)
21-34: Consider type safety for role options.While the implementation works, we could improve type safety by using a type union of the values.
+type MemberRoleValue = typeof MEMBER_ROLES[keyof typeof MEMBER_ROLES] +type MemberActionValue = typeof MEMBER_ACTIONS[keyof typeof MEMBER_ACTIONS] +type RoleOptionValue = MemberRoleValue | MemberActionValue export const roleOptions = [ { value: MEMBER_ROLES.admin, label: capitalizeFirstLetter(MEMBER_ROLES.admin.toLowerCase()), - }, + } as const, { value: MEMBER_ROLES.manager, label: capitalizeFirstLetter(MEMBER_ROLES.manager.toLowerCase()), - }, + } as const, { value: MEMBER_ACTIONS.remove, label: capitalizeFirstLetter(MEMBER_ACTIONS.remove.toLowerCase()), - }, + } as const, -] +] as const satisfies readonly { value: RoleOptionValue; label: string }[]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx(7 hunks)packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx(4 hunks)packages/components/modules/profiles/web/ProfileMembers/MembersList/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileMembers/constants.ts(1 hunks)packages/components/modules/profiles/web/ProfileMembers/index.tsx(1 hunks)packages/components/schema.graphql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx
- packages/components/modules/profiles/web/ProfileMembers/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (9)
packages/components/modules/profiles/web/ProfileMembers/MembersList/index.tsx (3)
11-14: LGTM! Enhanced type safety with GraphQL generated types.The addition of
ProfileRoleStatustype from the generated GraphQL types improves type safety for member status handling.
19-19: LGTM! Improved constant naming convention.The renaming to
MEMBER_STATUSESfollows standard JavaScript/TypeScript conventions for constants.
51-54:❓ Verification inconclusive
Verify member list updates after removal.
Since this PR implements member removal functionality, ensure that the members list is properly updated after a member is removed. The current
useMemoimplementation should handle updates correctly when the edges array changes, but verify that the removal mutation triggers a proper cache update.
🏁 Script executed:
#!/bin/bash # Search for cache update handling in the removal mutation rg -A 5 "updateCache|invalidateQueries|refetchQueries" --type tsLength of output: 2536
Verify proper cache invalidation on member removal
The current
useMemoimplementation correctly recalculates the member list upon changes todata?.members?.edges. However, our search for cache invalidation keywords did not reveal any explicit cache update logic for the member removal functionality (unlike what’s used in the MFA and user modules). Please verify that the removal mutation for members is configured to trigger an appropriate cache update (for example, viainvalidateQueries,updateCache, or similar) so that the list reflects removals immediately.packages/components/modules/profiles/web/ProfileMembers/constants.ts (2)
6-10: LGTM! Good job converting enums to constant objects.The conversion from enums to constant objects for
MEMBER_STATUSESandMEMBER_ROLESimproves type safety and reduces potential TypeScript bugs.Also applies to: 12-15
17-19: LGTM! Well-structured member action constant.The
MEMBER_ACTIONSconstant is appropriately defined for the member removal feature.packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (3)
56-61: LGTM! Clean dialog state management.The dialog state management and confirmation handlers are well implemented.
Also applies to: 63-65
105-112: LGTM! Well-structured event handling.The Select's onChange handler appropriately differentiates between role changes and member removal.
170-186: LGTM! Confirmation dialog matches requirements.The confirmation dialog implementation matches the PR requirements:
- Title: "Remove member"
- Message: "Are you sure you want to remove this member? This action will revoke their access to the organization profile."
- Red remove button
packages/components/schema.graphql (1)
1273-1277: LGTM! Well-defined GraphQL types for member removal.The input and payload types are properly defined with:
- Required fields: profileId, userId
- Optional clientMutationId
- Standard error handling through ErrorType
- Deleted ID in the response
Also applies to: 1279-1285
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (3)
56-65: Enhance user feedback during member removal.The UI should reflect the loading state during member removal to prevent multiple clicks and provide visual feedback.
Consider this enhancement:
const confirmRemoveProfileMember = () => { if (currentProfile?.id && userId) { removeProfileMember() } setOpenConfirmRemoveMember(false) } const handleRemoveMemberDialog = () => { + if (isRemovingMember) return setOpenConfirmRemoveMember(!openConfirmRemoveMember) }
118-124: Enhance accessibility for the remove action.The remove action is only distinguished by color, which may not be sufficient for users with color vision deficiencies.
Consider adding an icon or additional visual indicator:
<MenuItem key={value} value={value} sx={{ color: value === MEMBER_ACTIONS.remove ? theme.palette.error.main : 'inherit', + '& .MuiSvgIcon-root': { + color: value === MEMBER_ACTIONS.remove ? theme.palette.error.main : 'inherit', + marginRight: theme.spacing(1) + } }} > + {value === MEMBER_ACTIONS.remove && <DeleteIcon fontSize="small" />} {label} </MenuItem>
169-185: Enhance confirmation dialog button states.The remove button should reflect the loading state to prevent multiple submissions.
Consider this enhancement:
<Button variant="contained" color="error" + disabled={isRemovingMember} onClick={confirmRemoveProfileMember} > - Remove + {isRemovingMember ? 'Removing...' : 'Remove'} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx(7 hunks)packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx(4 hunks)packages/components/modules/profiles/web/ProfileMembers/MembersList/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileMembers/constants.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/profiles/web/ProfileMembers/MembersList/index.tsx
- packages/components/modules/profiles/web/ProfileMembers/MemberListItem/index.tsx
- packages/components/modules/profiles/web/ProfileMembers/constants.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (1)
packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx (1)
48-54: Add error handling and loading state management.The member removal functions should handle error cases and provide feedback to users.
Consider this implementation:
const removeProfileMember = () => { if (currentProfile?.id && userId) { removeMember({ variables: { input: { profileId: currentProfile.id, userId } }, + onCompleted: () => { + // Show success notification + }, + onError: (error) => { + // Show error notification + } }) } }
joaosoutto
dismissed
priscilladeroode’s stale review
the changes was already approved by AA
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
3-8: Clarify the New Feature EntryThe new changelog entry for version 1.0.10 clearly indicates the implementation of the "remove profile member" functionality. To further enhance clarity, consider adding a brief description (or referencing the related PR/feature ID such as BA-2123) so that users and developers quickly understand the significance and impact of this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Test Packages
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
joaosoutto
force-pushed
the
BA--2123-multiple-profiles-remove-member
branch
from
83d187a to
8dfde20
Compare
|
As a user, on the BaseApp Profile,I would like to remove an existing member, In order to prevent them from doing anything in the profile.
Original Story:
Acceptance Criteria
Business Rules - Removing Member - 2
Given the user wants to remove a member, they can choose the remove option in the dropdown.
The remove option should be prominently displayed and in red font to dintinguish it from other options.
Given the user has selected to remove the member, display a confirmation dialog to make sure he intends to do this change.
Given the user has confirmed the removal of member, then remove the member from the organization and he should not be able to access the organizations profile anymore.
Copy Text
Removal Confirmation: Are you sure you want to remove this member? This action will revoke access to the organizational profile
Current behavior
Expected behavior
Code Snippet
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3680-68785&node-type=instance&t=4tZzXNioOJs6iIeL-0
Approvd
https://app.approvd.io/projects/BA/stories/35056
DEMO:
https://www.loom.com/share/c267ee0a8eb14ae787dc174abfd67c4b
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores