fix(cozy-sharing): Accumulate recipients in federated folder sharing#2961
fix(cozy-sharing): Accumulate recipients in federated folder sharing#2961doubleface merged 3 commits intomasterfrom
Conversation
WalkthroughThe PR updates FederatedFolderModal to use a functional state updater for Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 1
🧹 Nitpick comments (1)
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx (1)
65-75: Add a duplicate-recipient assertion so deduplication is actually tested.Line 241 currently shares only a new recipient, so this test proves accumulation but not deduplication. Include one already-shared recipient in the second
onSharepayload and keep expected total at3to cover the dedupe path.Suggested test adjustment
<button data-testid="btn-share-other" onClick={() => onShare({ - recipients: [{ _id: 'r3', id: 'r3', displayName: 'Charlie' }], + recipients: [ + { _id: 'r1', id: 'r1', displayName: 'Alice' }, // duplicate + { _id: 'r3', id: 'r3', displayName: 'Charlie' } // new + ], readOnlyRecipients: [] }) } >Also applies to: 228-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx` around lines 65 - 75, The second simulated share in the test currently only adds a new recipient so deduplication isn't exercised; update the onShare payload invoked by the button with data-testid "btn-share-other" to include one recipient already present in the initial share (use the same _id/id value used in the first onShare) along with the new recipient, then keep the assertion expecting total recipients to be 3 so the test verifies deduplication logic in the FederatedFolderModal component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 70-79: The merge/deduplicate call is losing recipients because
mergeAndDeduplicateRecipients dedups by item.id while this modal sometimes uses
_id; before calling setFederatedRecipients/mergeAndDeduplicateRecipients (in the
setFederatedRecipients block that handles params.recipients and
params.readOnlyRecipients) normalize each recipient to ensure an id property
exists (e.g., map recipients -> ({ ...r, id: r.id || r._id })) for both incoming
params arrays and previous recipients, avoid mutating originals by returning new
objects, then pass those normalized arrays into mergeAndDeduplicateRecipients so
deduplication uses a consistent id field.
---
Nitpick comments:
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx`:
- Around line 65-75: The second simulated share in the test currently only adds
a new recipient so deduplication isn't exercised; update the onShare payload
invoked by the button with data-testid "btn-share-other" to include one
recipient already present in the initial share (use the same _id/id value used
in the first onShare) along with the new recipient, then keep the assertion
expecting total recipients to be 3 so the test verifies deduplication logic in
the FederatedFolderModal component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a913e246-de8b-4510-9aba-50a66ccfc863
📒 Files selected for processing (2)
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsxpackages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cozy-sharing/src/components/SharedDrive/helpers.js (1)
15-21:⚠️ Potential issue | 🟡 MinorAvoid dropping multiple recipients when
idis missing.At Line 16, recipients with no
id/_idall collapse under the sameundefinedkey, so only the first one is kept.💡 Suggested hardening
const uniqueArray = combinedArray.filter(item => { - if (!seenIds.has(item.id)) { - seenIds.add(item.id) - return true - } - return false + if (item.id == null) return true + if (seenIds.has(item.id)) return false + seenIds.add(item.id) + return true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedDrive/helpers.js` around lines 15 - 21, The current dedupe logic in the uniqueArray filter collapses recipients lacking item.id into a single undefined key via seenIds, dropping all but the first; update the filter to compute a stable dedupe key (e.g., const key = item.id || item._id || some other unique property like item.email || JSON.stringify(item)) and use that key with seenIds (check seenIds.has(key) and seenIds.add(key)) instead of item.id directly so recipients without id/_id are deduplicated correctly.
🧹 Nitpick comments (1)
packages/cozy-sharing/src/components/SharedDrive/helpers.spec.js (1)
111-163: Optional: add a formatter fallback test fornamewhendisplayNameis absent.
formatRecipientshas a fallback path (displayName || name) that isn’t explicitly asserted yet; a small test would prevent regressions there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedDrive/helpers.spec.js` around lines 111 - 163, Add a new unit test for formatRecipients that verifies the fallback to the name field when displayName is missing: create input where a recipient object has no displayName but has name (and an email/ _id), call formatRecipients, and assert the returned item's public_name equals the original name; also assert index uses RECIPIENT_INDEX_PREFIX + _id and that readOnly vs recipients map to types ('two-way'/'one-way') as in existing tests to prevent regressions in the fallback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cozy-sharing/src/components/SharedDrive/helpers.js`:
- Around line 15-21: The current dedupe logic in the uniqueArray filter
collapses recipients lacking item.id into a single undefined key via seenIds,
dropping all but the first; update the filter to compute a stable dedupe key
(e.g., const key = item.id || item._id || some other unique property like
item.email || JSON.stringify(item)) and use that key with seenIds (check
seenIds.has(key) and seenIds.add(key)) instead of item.id directly so recipients
without id/_id are deduplicated correctly.
---
Nitpick comments:
In `@packages/cozy-sharing/src/components/SharedDrive/helpers.spec.js`:
- Around line 111-163: Add a new unit test for formatRecipients that verifies
the fallback to the name field when displayName is missing: create input where a
recipient object has no displayName but has name (and an email/ _id), call
formatRecipients, and assert the returned item's public_name equals the original
name; also assert index uses RECIPIENT_INDEX_PREFIX + _id and that readOnly vs
recipients map to types ('two-way'/'one-way') as in existing tests to prevent
regressions in the fallback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b05dbae6-9746-47b5-8155-5e6592cf3396
📒 Files selected for processing (2)
packages/cozy-sharing/src/components/SharedDrive/helpers.jspackages/cozy-sharing/src/components/SharedDrive/helpers.spec.js
When the drive.federated-shared-folder.enable flag is active, each recipient added via onShare was overwriting the previous ones. Use mergeAndDeduplicateRecipients to properly accumulate recipients, matching the behavior of SharedDriveModal.
doubleface
force-pushed
the
fix/accumulateSharingRecipients
branch
2 times, most recently
from
6182960 to
29bbbdf
Compare
rezk2ll
left a comment
rezk2ll
left a comment
There was a problem hiding this comment.
_id-only code paths downstream. The rest of the helpers (moveRecipientToReadWrite, moveRecipientToReadOnly, formatRecipients) and onRevoke in FederatedFolderModal all match by r._id. This works today because recipients always arrive with _id (cozy-client guarantees it), but the normalization is asymmetric: it ensures id exists, not _id. If a recipient ever arrives with only id, the move/revoke/format functions silently fail. Low risk in practice, but the inconsistency is worth being aware of.
Recipients without any identifier. If a recipient has neither id nor _id, the normalized id is undefined. Multiple such recipients collapse into one because Set treats all undefined keys as equal. Unlikely in production, but the dedup silently drops data without warning. CodeRabbit flagged this too.
Added id/_id normalization into mergeAndDeduplicateRecipients to have correct deduplication of recipients Added unit tests for SharedDrive helpers.
doubleface
force-pushed
the
fix/accumulateSharingRecipients
branch
from
29bbbdf to
578c3e0
Compare
3 participants
When the drive.federated-shared-folder.enable flag is active, each recipient added via onShare was overwriting the previous ones. Use mergeAndDeduplicateRecipients to properly accumulate recipients, matching the behavior of SharedDriveModal.
https://www.notion.so/linagora/Drive-Board-20062718bad180d687d1f517b2ed7dda?p=32062718bad180fdbd6ad1f8915b52d3&pm=s&pvs=31
Summary by CodeRabbit
Bug Fixes
Tests