feat: add bulk membership removal#17
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!bulkRemovalEnabled) { | ||
| setBulkRemovalBarVisible(false); | ||
| setBulkRemovalBarActive(false); | ||
| setLastSelectedRemovalCount(0); | ||
| return; | ||
| } | ||
|
|
||
| if (selectedRemovalCount > 0) { | ||
| setLastSelectedRemovalCount(selectedRemovalCount); | ||
| setBulkRemovalBarVisible(true); | ||
| let secondFrame = 0; | ||
| const frame = window.requestAnimationFrame(() => { | ||
| secondFrame = window.requestAnimationFrame(() => { | ||
| setBulkRemovalBarActive(true); | ||
| }); | ||
| }); | ||
| return () => { | ||
| window.cancelAnimationFrame(frame); | ||
| window.cancelAnimationFrame(secondFrame); | ||
| }; | ||
| } | ||
|
|
||
| setBulkRemovalBarActive(false); | ||
| if (!bulkRemovalBarVisible) { | ||
| return; | ||
| } | ||
|
|
||
| const timeout = window.setTimeout(() => { | ||
| setBulkRemovalBarVisible(false); | ||
| setLastSelectedRemovalCount(0); | ||
| }, 200); | ||
|
|
||
| return () => window.clearTimeout(timeout); | ||
| }, [bulkRemovalBarVisible, bulkRemovalEnabled, selectedRemovalCount]); |
There was a problem hiding this comment.
📝 Info: Bulk removal bar animation state machine uses bulkRemovalBarVisible as both a dependency and internal state
The useEffect at src/renderer/components/console/directory/recipient-detail-dialog.tsx:369-403 uses bulkRemovalBarVisible as both a dependency and a value it reads/writes. This initially looks like it could cause an infinite loop, but it converges correctly: when selectedRemovalCount drops to 0, setBulkRemovalBarActive(false) fires, then a 200ms timeout sets bulkRemovalBarVisible to false. The subsequent effect re-run hits the if (!bulkRemovalBarVisible) return guard and stops. The double-requestAnimationFrame pattern for the show transition is a well-known technique to ensure the browser has committed the DOM change before toggling the active class for CSS transitions.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
No code change needed. The animation state effect intentionally uses separate mounted and active state so the bar can animate both entering and leaving, and the bulkRemovalBarVisible dependency converges after the delayed unmount guard. I verified the component test covers the visible/hidden action bar states, and the full suite passes.
| function showRemoveGroupsToast( | ||
| memberName: string, | ||
| batchResults: Array<{ group: ExchangeGroupListItem; result: GroupsRemoveMembersResult }>, | ||
| ): boolean { | ||
| const issues = batchResults.flatMap(({ group, result }) => | ||
| getRemoveMembersIssueDescriptions(result).map((description) => | ||
| `${group.displayName}: ${description}`, | ||
| ), | ||
| ); | ||
|
|
||
| if (issues.length > 0) { | ||
| toast.warning("Some removals need attention", { | ||
| description: issues.join("\n"), | ||
| }); | ||
| return false; | ||
| } | ||
|
|
||
| const cleanStatuses = batchResults.flatMap(({ result }) => | ||
| result.items.map((item) => item.status), | ||
| ); | ||
| const removedCount = cleanStatuses.filter((status) => status === "removed").length; | ||
| const notMemberCount = cleanStatuses.filter((status) => status === "notMember").length; | ||
|
|
||
| if (removedCount > 0 && notMemberCount === 0) { | ||
| toast.success(removedCount === 1 ? "Removed from group" : "Removed from groups", { | ||
| description: `${memberName} was removed from ${removedCount} group${removedCount === 1 ? "" : "s"}`, | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| if (removedCount === 0) { | ||
| toast.success("Not a member", { | ||
| description: `${memberName} was not a member of ${notMemberCount} selected group${notMemberCount === 1 ? "" : "s"}`, | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| toast.success("Group memberships updated", { | ||
| description: `${removedCount} removed; ${notMemberCount} already absent`, | ||
| }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
📝 Info: Duplicated showRemoveGroupsToast / showRemoveMembershipGroupsToast functions across screens
showRemoveGroupsToast in src/renderer/components/console/screens/directory-screen.tsx:309-350 and showRemoveMembershipGroupsToast in src/renderer/components/console/screens/groups-screen.tsx:486-525 are nearly identical functions with the same logic and return type. Both could be extracted into a shared utility (perhaps alongside the existing helpers in group-members-mutation-outcome.ts) to reduce duplication and ensure consistent behavior if the toast logic needs to change.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
No code change needed for this PR. The duplication is limited to screen-local toast copy around otherwise different parent flows; extracting it would broaden this bug-fix/update beyond the review issue. I verified both functions now share the corrected partial-failure behavior through their respective handlers, with npm run typecheck, npm run lint, and npm run test passing.
| const handleRemoveMembershipGroups = useCallback(async () => { | ||
| if (!membershipMember || removeMembershipGroupsTarget.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const memberRef: GroupMemberWriteRef = { | ||
| exchangeIdentity: membershipMember.exchangeIdentity, | ||
| objectId: membershipMember.objectId, | ||
| primaryEmail: membershipMember.primaryEmail, | ||
| }; | ||
|
|
||
| setRemovePending(true); | ||
| setRemoveError(null); | ||
| try { | ||
| const batchResults: Array<{ group: ExchangeGroupListItem; result: GroupsRemoveMembersResult }> = []; | ||
|
|
||
| for (const group of removeMembershipGroupsTarget) { | ||
| const result = await removeGroupMembersMutation.mutateAsync({ | ||
| groupRef: groupRefFromListItem(group), | ||
| memberRefs: [memberRef], | ||
| }); | ||
| batchResults.push({ group, result }); | ||
| } | ||
|
|
||
| const allClean = showRemoveMembershipGroupsToast(detailTarget?.displayName ?? "Member", batchResults); | ||
| if (allClean) { | ||
| setRemoveMembershipGroupsTarget([]); | ||
| setSelectedRemovalGroupKeys(new Set()); | ||
| } else { | ||
| const issueMessage = batchResults | ||
| .flatMap(({ group, result }) => | ||
| getRemoveMembersIssueDescriptions(result).map((description) => | ||
| `${group.displayName}: ${description}`, | ||
| ), | ||
| ) | ||
| .join("\n"); | ||
| setRemoveError(issueMessage || "Some selected removals could not be completed."); | ||
| } | ||
|
|
||
| await refetchMemberships(); | ||
| void refetchMembers(); | ||
| } catch (err) { | ||
| const message = formatPresentedCommandFailure( | ||
| presentCommandFailure(err, "Remove Member Error", "Failed to remove member from selected groups."), | ||
| ); | ||
| setRemoveError(message); | ||
| toast.error("Failed to remove member from selected groups", { description: message }); | ||
| } finally { | ||
| setRemovePending(false); | ||
| } | ||
| }, [ | ||
| detailTarget?.displayName, | ||
| membershipMember, | ||
| refetchMembers, | ||
| refetchMemberships, | ||
| removeGroupMembersMutation, | ||
| removeMembershipGroupsTarget, | ||
| ]); |
There was a problem hiding this comment.
📝 Info: Shared removePending state in groups-screen multiplexes three removal flows
In src/renderer/components/console/screens/groups-screen.tsx, the removePending state is shared across handleRemoveMember (member removal from selected group), handleRemoveMembershipGroup (single membership removal from detail dialog), and handleRemoveMembershipGroups (bulk membership removal from detail dialog). All three set it to true at the start and false in finally. This works because the confirmation dialog button is disabled when removePending is true, preventing concurrent execution. However, the shared state means any of these operations blocks all three remove paths simultaneously, which is the intended behavior but worth noting for future maintainability if these flows are decoupled.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
No code change needed. The shared removePending state is intentional because these removal flows share the same confirmation surface and should block competing remove actions while one mutation is running. I verified the dialog buttons are disabled from that shared pending state and the full test suite passes.
| useEffect(() => { | ||
| const currentMembershipKeys = new Set(currentMemberships.map((group) => group.exchangeIdentity)); | ||
| setSelectedRemovalGroupKeys((previous) => { | ||
| const next = new Set( | ||
| [...previous].filter((exchangeIdentity) => currentMembershipKeys.has(exchangeIdentity)), | ||
| ); | ||
|
|
||
| return next.size === previous.size ? previous : next; | ||
| }); | ||
| }, [currentMemberships]); |
There was a problem hiding this comment.
📝 Info: useEffect syncing selectedRemovalGroupKeys with currentMemberships runs on every memberships change
The useEffect at src/renderer/components/console/screens/directory-screen.tsx:611-620 (and the equivalent at groups-screen.tsx:698-706) filters selectedRemovalGroupKeys against currentMembershipKeys on every currentMemberships change. The optimization next.size === previous.size ? previous : next avoids unnecessary state updates when the filter is a no-op (since filtering can only remove items, equal sizes means identical sets). This is a correct and efficient reconciliation pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
No code change needed. The reconciliation effect is intentionally scoped to membership list changes and returns the previous Set when filtering is a no-op. I verified both parent screens reset selection on dialog open/close and prune stale selected keys after membership refreshes.
| const [bulkRemovalBarVisible, setBulkRemovalBarVisible] = useState( | ||
| () => bulkRemoval.enabled && bulkRemoval.selectedGroupsCount > 0, | ||
| ); | ||
| const [bulkRemovalBarActive, setBulkRemovalBarActive] = useState( | ||
| () => bulkRemoval.enabled && bulkRemoval.selectedGroupsCount > 0, | ||
| ); | ||
| const [lastSelectedRemovalCount, setLastSelectedRemovalCount] = useState( | ||
| () => bulkRemoval.enabled ? bulkRemoval.selectedGroupsCount : 0, | ||
| ); |
There was a problem hiding this comment.
📝 Info: useState lazy initializers in dialog component assume fresh mount on each open
The useState lazy initializers for bulkRemovalBarVisible, bulkRemovalBarActive, and lastSelectedRemovalCount at src/renderer/components/console/directory/recipient-detail-dialog.tsx:336-344 only run on initial mount. This is safe because both callers (openRecipientDialog in directory-screen and openMemberDetailDialog in groups-screen) reset selectedRemovalGroupKeys to an empty set before opening the dialog, and the Radix Dialog unmounts content when closed. If the Dialog were ever changed to use forceMount, these initializers would not re-run on subsequent opens, and the useEffect would need to handle the reset explicitly.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
No code change needed. The lazy initializers are safe with the current Dialog lifecycle and both parent screens reset selected removal keys before opening. The follow-up effect also resets the bar state whenever bulk removal is disabled or the selected count returns to zero.
CarlosCommits
force-pushed
the
feature/bulk-membership-removal
branch
from
19c7ebf to
84e29ba
Compare
| bulkRemoval: | ||
| | { | ||
| enabled: true; | ||
| selectedGroupKeys: Set<string>; | ||
| selectedGroupsCount: number; | ||
| pending: boolean; | ||
| onToggleGroupSelection: (exchangeIdentity: string) => void; | ||
| onSelectAllCurrentMemberships: () => void; | ||
| onClearSelection: () => void; | ||
| onRequestRemoveSelectedGroups: () => void; | ||
| } | ||
| | { | ||
| enabled: false; | ||
| }; |
There was a problem hiding this comment.
📝 Info: bulkRemoval discriminated union is always passed with enabled: true
The bulkRemoval prop on RecipientDetailDialogProps is typed as a discriminated union with { enabled: true; ... } | { enabled: false }, but both callers (directory-screen.tsx:2219-2234 and groups-screen.tsx:2103-2120) always pass enabled: true. The { enabled: false } variant is dead code — it's future-proofing that may be intentional, but worth noting since the animation state logic in the component initializers (recipient-detail-dialog.tsx:336-344) and the conditional property access patterns rely on this being true.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
No code change needed. The { enabled: false } union branch is intentional: it makes the shared dialog API explicit for any future caller that should not expose bulk removal, instead of silently omitting a group of optional props. Both current callers opt in with enabled: true, and TypeScript now forces future callers to make that choice deliberately.
CarlosCommits
force-pushed
the
feature/bulk-membership-removal
branch
from
84e29ba to
b4cfefb
Compare
1 participant
Summary
Verification