BA-2123: Multiple Profiles - Removing Member #192
Conversation
joaosoutto
requested review from
anicioalexandre,
deboracosilveira,
ficast,
priscilladeroode and
sundayguru
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/components/modules/profiles/Members/MemberItem/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/profiles/graphql/mutations/RemoveMember.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/profiles/Members/constants.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThe pull request introduces functionality for removing a member from a profile, enhancing the Changes
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 (
|
![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
🧹 Nitpick comments (5)
packages/components/modules/profiles/Members/constants.ts (1)
29-32: Consider separating actions from roles.While the implementation works, mixing member actions with role options might not be the most intuitive design. Consider creating a separate dropdown or button for the remove action to:
- Better separate concerns between role management and member removal
- Prevent accidental member removal when changing roles
- Align with common UX patterns where destructive actions are separated from regular options
packages/components/modules/profiles/graphql/mutations/RemoveMember.ts (1)
15-38: Consider adding loading state feedback.The mutation implementation is solid with good error handling and notifications. However, consider showing a loading state to users during the mutation.
const commit = (config: UseMutationConfig<RemoveMemberMutation>) => commitMutation({ ...config, + optimisticResponse: { + profileRemoveMember: { + deletedId: config.variables.input.userId, + }, + }, onCompleted: (response, errors) => {packages/components/modules/profiles/Members/MemberItem/index.tsx (2)
57-70: Consider adding error recovery mechanism.The member removal functions are well-implemented, but consider adding a way to recover from failed removals.
const removeProfileMember = () => { if (currentProfile?.id && userId) { removeMember({ variables: { input: { profileId: currentProfile.id, userId } }, + onError: () => { + setHideMember(false); // Reset UI state on error + }, }) } }
103-110: Consider using a dedicated handler for remove action.The current implementation mixes role change and remove action handling. Consider separating these concerns for better maintainability.
-onChange={(event, _) => { - const { value } = event.target - if (value === MemberActions.remove) { - handleRemoveMemberDialog() - } else { - handleRoleChange(event as SelectChangeEvent<{ value: MemberRoles }>) - } -}} +onChange={handleSelectChange} +const handleSelectChange = (event: SelectChangeEvent<unknown>, child: React.ReactNode) => { + const { value } = event.target + if (value === MemberActions.remove) { + handleRemoveMemberDialog() + return + } + handleRoleChange(event as SelectChangeEvent<{ value: MemberRoles }>) +}packages/components/schema.graphql (1)
1236-1242: Consider adding more details in the payload.The payload could be more informative by including the affected profile or user details.
type ProfileRemoveMemberPayload { """May contain more than one error for same field.""" errors: [ErrorType] _debug: DjangoDebug deletedId: ID + profile: Profile + removedUserId: ID clientMutationId: String }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a8256fe and 934acd66321aee7a51c68400e6828a96de3301f3.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/RemoveMemberMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (4)
packages/components/modules/profiles/Members/MemberItem/index.tsx(6 hunks)packages/components/modules/profiles/Members/constants.ts(2 hunks)packages/components/modules/profiles/graphql/mutations/RemoveMember.ts(1 hunks)packages/components/schema.graphql(7 hunks)
🔇 Additional comments (5)
packages/components/modules/profiles/Members/constants.ts (1)
16-18: LGTM! Clean enum implementation.The
MemberActionsenum is well-defined and follows TypeScript best practices.packages/components/modules/profiles/graphql/mutations/RemoveMember.ts (1)
7-13: LGTM! Well-structured GraphQL mutation.The mutation is properly defined with the correct input type and deletion record handling.
packages/components/modules/profiles/Members/MemberItem/index.tsx (2)
34-35: LGTM! Clean state management.Good use of state hooks for managing member visibility and removal mutation.
166-182: LGTM! Well-implemented confirmation dialog.The confirmation dialog follows the PR requirements with clear messaging and proper action buttons.
packages/components/schema.graphql (1)
1230-1234: LGTM! Well-defined input type.The
ProfileRemoveMemberInputtype is properly defined with required fields.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/components/package.json (1)
67-67: Fix caret ranges in dependencies.Some dependencies use caret ranges (
^) which can lead to unexpected updates:
"@faker-js/faker": "^9.0.3""eslint-plugin-storybook": "^0.8.0"Consider using fixed versions for better predictability:
- "@faker-js/faker": "^9.0.3", + "@faker-js/faker": "9.0.3", - "eslint-plugin-storybook": "^0.8.0", + "eslint-plugin-storybook": "0.8.0",Also applies to: 100-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 934acd66321aee7a51c68400e6828a96de3301f3 and 27a9e1ff1de37f95ae0c5301a85ec702346b0230.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/components/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Component Test Applications and Packages
- GitHub Check: Lint
🔇 Additional comments (2)
packages/components/package.json (2)
55-56: Verify React 18.3.1 compatibility.The update to React 18.3.1 is significant and may introduce breaking changes.
Run this script to check for potential compatibility issues:
24-48: Verify breaking changes in core dependencies.Several core dependencies have major version updates which may introduce breaking changes:
- @mui/* packages (5.x series)
- @tanstack/react-query (5.x)
- react-relay (16.x)
Run this script to check for potential breaking changes:
✅ Verification successful
Dependencies are safe to update - no breaking changes impact detected
The codebase is already using modern patterns and APIs across all core dependencies:
- MUI v5: Using recommended
createThemeandThemeProvider- React Query v5: Proper usage of new object config syntax
- React Relay v16: Following recommended hooks and patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in core dependencies # Check MUI v5 breaking changes echo "Checking for MUI v5 breaking changes..." rg "createTheme|ThemeProvider|makeStyles|withStyles|createStyles" # Check react-query v5 breaking changes echo "Checking for react-query v5 breaking changes..." rg "useQuery|useMutation|useInfiniteQuery" # Check react-relay v16 breaking changes echo "Checking for react-relay v16 breaking changes..." rg "useLazyLoadQuery|usePreloadedQuery|usePaginationFragment"Length of output: 26359
packages/components/package.json
Outdated
There was a problem hiding this comment.
Avoid using Next.js canary version in production.
Using next: "14.3.0-canary.24" in production dependencies is risky as canary releases are unstable and may contain breaking changes.
Consider using the latest stable version instead:
- "next": "14.3.0-canary.24",
+ "next": "14.2.0",📝 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.
| "next": "14.3.0-canary.24", | |
| "next": "14.2.0", |
packages/components/package.json
Outdated
There was a problem hiding this comment.
💡 Codebase verification
The current webpack version is vulnerable to a moderate severity XSS issue through the AutoPublicPathRuntimeModule. Please upgrade to webpack 5.94.0 or later.
- Update
webpackfrom"5.93.0"to"^5.94.0"inpackages/components/package.json
🔗 Analysis chain
Verify security advisories for dev dependencies.
Several development dependencies have been updated to new major versions. Let's verify there are no known security vulnerabilities.
Run this script to check for security advisories:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories in dev dependencies
# Check for security advisories using GitHub API
for pkg in "@storybook/react" "@testing-library/react" "jest" "cypress" "webpack"; do
echo "Checking $pkg..."
gh api graphql -f query='{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "'$pkg'") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
done
Length of output: 3049
joaosoutto
force-pushed
the
BA-2123-multiple-profiles-removing-member
branch
from
27a9e1f to
b91f4fd
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/components/package.json (1)
Line range hint
1-120: Fix the out-of-sync lockfile.The pipeline is failing because pnpm-lock.yaml is out of sync with package.json. Run
pnpm installto update the lockfile.pnpm install🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] pnpm-lock.yaml is out of sync with package.json. The lockfile's dependency specifications do not match those in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
🧹 Nitpick comments (2)
packages/components/modules/profiles/Members/MemberItem/index.tsx (2)
104-111: Add loading state feedback during member removal.The UI should indicate when a member removal is in progress.
onChange={(event, _) => { const { value } = event.target if (value === MemberActions.remove) { + if (isRemovingMember) return handleRemoveMemberDialog() } else { handleRoleChange(event as SelectChangeEvent<{ value: MemberRoles }>) } }}
170-186: Improve accessibility of the confirmation dialog.Add ARIA labels and role attributes to improve accessibility.
<ConfirmDialog title="Remove member" open={openConfirmRemoveMember} onClose={handleRemoveMemberDialog} + aria-label="Remove member confirmation dialog" + role="alertdialog" content={ <Typography variant="body1"> Are you sure you want to remove this member? This action will revoke their access to the organization profile. </Typography> } cancelText="Back" action={ - <Button variant="contained" color="error" onClick={confirmRemoveProfileMember}> + <Button + variant="contained" + color="error" + onClick={confirmRemoveProfileMember} + aria-label="Confirm member removal" + > Remove </Button> } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 27a9e1ff1de37f95ae0c5301a85ec702346b0230 and b91f4fd.
⛔ Files ignored due to path filters (2)
packages/components/__generated__/RemoveMemberMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/components/modules/profiles/Members/MemberItem/index.tsx(6 hunks)packages/components/modules/profiles/Members/constants.ts(2 hunks)packages/components/modules/profiles/graphql/mutations/RemoveMember.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/profiles/Members/constants.ts
- packages/components/modules/profiles/graphql/mutations/RemoveMember.ts
🧰 Additional context used
🪛 GitHub Actions: Main Workflow
packages/components/package.json
[error] pnpm-lock.yaml is out of sync with package.json. The lockfile's dependency specifications do not match those in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/components/package.json (2)
39-39: Avoid using Next.js canary version in production.Using
next: "14.3.0-canary.24"in production dependencies is risky as canary releases are unstable. Consider using the latest stable version instead.🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] pnpm-lock.yaml is out of sync with package.json. The lockfile's dependency specifications do not match those in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
118-118: Update webpack to patch security vulnerability.The current webpack version (5.93.0) has a known XSS vulnerability. Please upgrade to version 5.94.0 or later.
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] pnpm-lock.yaml is out of sync with package.json. The lockfile's dependency specifications do not match those in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
packages/components/schema.graphql (1)
1258-1270: LGTM! The GraphQL schema changes look good.The new types and mutation for member removal are well-structured and follow GraphQL best practices:
- Required fields are properly marked
- Consistent naming conventions
- Proper error handling structure
| const [hideMember, setHideMember] = useState(false) | ||
| const [removeMember, isRemovingMember] = useRemoveMemberMutation(setHideMember) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the remove member mutation.
The mutation should handle potential errors and provide feedback to the user.
- const [removeMember, isRemovingMember] = useRemoveMemberMutation(setHideMember)
+ const [removeMember, isRemovingMember] = useRemoveMemberMutation(setHideMember, {
+ onError: (error) => {
+ // Show error notification to user
+ console.error('Failed to remove member:', error)
+ }
+ })Committable suggestion skipped: line range outside the PR's diff.
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
Release Notes
New Features
Improvements
Bug Fixes
The update introduces a streamlined process for managing profile members, including a new confirmation step before removing a member and more robust role-based controls.