Fix multi-organization selection not being retained during selective share#10468
Fix multi-organization selection not being retained during selective share#10468SujanSanjula96 wants to merge 1 commit into
Conversation
…share organization tree
📝 WalkthroughWalkthroughAdds a patch changeset and updates the selective org share tree to reconcile bulk selection changes from ChangesSelective org share selection
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🤖 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
`@features/common.ui.shared-access.v1/components/selective-org-share-with-selective-roles.tsx`:
- Around line 861-872: The bulk-selection path in handleSelectedItemsChange can
add the same parent org multiple times because
resolveSelectedItems/selectParentNodes rely on render-time selectedItems and
addedOrgs closures while processing several siblings in one synchronous pass.
Fix this by reconciling additions with a locally accumulated Set or by deduping
inside the functional state updaters used in selectParentNodes so each parent ID
is only appended once, and verify the recursion guard in selectParentNodes still
terminates correctly after the change.
🪄 Autofix (Beta)
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d64931b9-5af5-43ac-8008-d423cc0d27b5
📒 Files selected for processing (2)
.changeset/shy-rules-share.mdfeatures/common.ui.shared-access.v1/components/selective-org-share-with-selective-roles.tsx
| const handleSelectedItemsChange = (newSelectedItems: string[]): void => { | ||
| const newSet: Set<string> = new Set(newSelectedItems); | ||
| const prevSet: Set<string> = new Set(selectedItems); | ||
|
|
||
| // Items newly selected: present in the new selection but not in the previous one. | ||
| const addedItems: string[] = newSelectedItems.filter((itemId: string) => !prevSet.has(itemId)); | ||
| // Items deselected: present in the previous selection but not in the new one. | ||
| const removedItems: string[] = selectedItems.filter((itemId: string) => !newSet.has(itemId)); | ||
|
|
||
| addedItems.forEach((itemId: string) => resolveSelectedItems(itemId, true)); | ||
| removedItems.forEach((itemId: string) => resolveSelectedItems(itemId, false)); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Batching multiple selections can push duplicate parent org IDs into selectedItems/addedOrgs.
resolveSelectedItems(itemId, true) calls selectParentNodes, which guards with selectedItems.includes(parentNode.id) and addedOrgs.includes(...) read from the render-time closure, then appends via setSelectedItems(prev => [ ...prev, parentNode.id ]). When the array handler processes several newly-added siblings that share a parent in a single synchronous pass, each iteration sees the same stale arrays (no parent yet) and appends the parent again, producing duplicate IDs in selectedItems and addedOrgs.
The previous per-toggle path re-rendered between events, refreshing those closures, so duplicates didn't accumulate — this bulk path is exactly the multi-select scenario this PR enables, so it's newly reachable.
Consider reconciling against a locally-accumulated set rather than relying on the stale closure between iterations (or dedupe in the functional updaters):
🛠️ One option: dedupe in the parent-selection updater
- if (!selectedItems.includes(parentNode.id)) {
- setSelectedItems((prev: string[]) => [ ...prev, parentNode.id ]);
- selectParentNodes(parentNode.id);
-
- // add the selected organization to the addedOrgs list if it is not already there
- if (!addedOrgs.includes(parentNode.id)) {
- setAddedOrgs((addedOrgs: string[]) => [ ...addedOrgs, parentNode.id ]);
- }
- }
+ setSelectedItems((prev: string[]) =>
+ prev.includes(parentNode.id) ? prev : [ ...prev, parentNode.id ]);
+ setAddedOrgs((prev: string[]) =>
+ prev.includes(parentNode.id) ? prev : [ ...prev, parentNode.id ]);
+ selectParentNodes(parentNode.id);Note: the recursion guard for selectParentNodes also needs rethinking if you change the early-exit; verify termination on cyclic-free trees.
Want me to draft a full reconciliation that builds the next selectedItems/addedOrgs/removedOrgs from a single accumulated set to avoid stale-closure batching issues?
🤖 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
`@features/common.ui.shared-access.v1/components/selective-org-share-with-selective-roles.tsx`
around lines 861 - 872, The bulk-selection path in handleSelectedItemsChange can
add the same parent org multiple times because
resolveSelectedItems/selectParentNodes rely on render-time selectedItems and
addedOrgs closures while processing several siblings in one synchronous pass.
Fix this by reconciling additions with a locally accumulated Set or by deduping
inside the functional state updaters used in selectParentNodes so each parent ID
is only appended once, and verify the recursion guard in selectParentNodes still
terminates correctly after the change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10468 +/- ##
========================================
Coverage 72.72% 72.73%
========================================
Files 469 469
Lines 70888 70908 +20
Branches 484 240 -244
========================================
+ Hits 51555 51572 +17
- Misses 19037 19225 +188
+ Partials 296 111 -185 🚀 New features to boost your workflow:
|
1 participant
Purpose
Related Issues
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.