Add bulk deletion for Access Control groups - #752
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR adds safe row-ID handling and configurable row selection to ChangesGroup deletion and table selection
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds page-scoped group selection and bulk deletion with confirmation, notifications, partial-failure handling, and selection reset behavior; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant GroupsTable
participant GroupsMultiSelect
participant GroupDeletionAPI
participant GroupDataCache
GroupsTable->>GroupsMultiSelect: submit selected deletable groups
GroupsMultiSelect->>GroupsMultiSelect: confirm destructive action
GroupsMultiSelect->>GroupDeletionAPI: delete groups concurrently
GroupDeletionAPI-->>GroupsMultiSelect: return deletion results
GroupsMultiSelect->>GroupDataCache: refresh group data
GroupsMultiSelect-->>GroupsTable: report result and clear selection
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the feature, lists the main changes, includes an image, and selects that documentation is not needed. The issue ticket link is empty, and the documentation exemption has no explanation, but the description is otherwise mostly complete.
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/modules/groups/GroupsMultiSelect.tsx (1)
47-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider limiting deletion concurrency.
Promise.allSettledstarts one DELETE request per selected group at the same time. Selection persists across pages, so the batch can grow large. A large batch sends many parallel requests to the API and can trigger rate limiting. A small concurrency pool, for example 5 requests at a time, keeps the batch predictable while preserving the per-group result order thatresults.forEachdepends on.🤖 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 `@src/modules/groups/GroupsMultiSelect.tsx` around lines 47 - 55, Limit the DELETE requests initiated by the Promise.allSettled flow in GroupsMultiSelect to a small concurrency pool, such as five active requests at a time, while preserving the original groupsToDelete order in the resulting per-group statuses so the existing results.forEach handling remains correct.
🤖 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 `@src/modules/groups/table/GroupsTable.tsx`:
- Around line 417-421: Update the columnVisibility.select value in GroupsTable
to explicitly coerce permission.groups.delete to a boolean, matching the
existing handling near the table configuration and ensuring undefined
permissions hide the selection column.
- Around line 50-59: Update the page-selection checkbox in the GroupsTable
header to pass an indeterminate state when table.getIsSomePageRowsSelected() is
true, and ensure Checkbox renders a distinct non-check indicator for that state
instead of <Check />. Preserve getIsAllPageRowsSelected() for the checked state
and the existing selectable-row disabled logic.
---
Nitpick comments:
In `@src/modules/groups/GroupsMultiSelect.tsx`:
- Around line 47-55: Limit the DELETE requests initiated by the
Promise.allSettled flow in GroupsMultiSelect to a small concurrency pool, such
as five active requests at a time, while preserving the original groupsToDelete
order in the resulting per-group statuses so the existing results.forEach
handling remains correct.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 268cd5ab-633f-43e5-83e5-f710bd5af2ff
📒 Files selected for processing (6)
src/components/table/DataTable.tsxsrc/components/table/DataTableMultiSelectPopup.tsxsrc/interfaces/Group.tssrc/modules/groups/GroupsMultiSelect.tsxsrc/modules/groups/table/GroupsTable.tsxsrc/modules/groups/useGroupsUsage.tsx
d3c0841 to
b78a95b
Compare
2 participants
Description
Adds page-scoped group selection and bulk deletion to the Access Control Groups page, following the existing Peers page interaction pattern.
Changes
Issue ticket number and link
Documentation
Select exactly one:
Summary by CodeRabbit
New Features
Bug Fixes