fix(cozy-sharing): Refresh link state in share modal#3022
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThree sharing dialog components are refactored to derive share links from the sharing context hook instead of receiving links as props. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx (1)
107-127: ⚡ Quick winRemove unnecessary state update for non-federated documents.
The
elseclause (lines 114-116) setssharingLinkstate for non-federated documents, but this state is never used—line 190 callsgetSharingLink(...)directly instead. This causes unnecessary state updates every timedocumentPermissionschanges, adding complexity without benefit.♻️ Simplify by removing the unused else clause
useEffect(() => { const fetchSharingLink = async () => { if (!existingDocument) return if (existingDocument.driveId) { const link = await getFederatedShareLink(existingDocument) setSharingLink(link) - } else { - setSharingLink(getSharingLink(existingDocument._id)) } } fetchSharingLink() }, [ existingDocument, documentPermissions, getFederatedShareLink, - getSharingLink, - getOwner, - getSharingById + getFederatedShareLink ])Also consider removing
getSharingLink,getOwner, andgetSharingByIdfrom the dependency array since they're not used in the effect body.🤖 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 `@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx` around lines 107 - 127, The effect in FederatedFolderModal's useEffect updates sharingLink for non-federated docs unnecessarily: remove the else branch in fetchSharingLink so setSharingLink is only called when existingDocument.driveId is present (keep only the getFederatedShareLink path), and then simplify the dependency array by removing getSharingLink, getOwner, and getSharingById since they are not referenced in the effect body; this preserves the existing getSharingLink usage at render-time (line ~190) and avoids redundant state updates.
🤖 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.
Nitpick comments:
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 107-127: The effect in FederatedFolderModal's useEffect updates
sharingLink for non-federated docs unnecessarily: remove the else branch in
fetchSharingLink so setSharingLink is only called when existingDocument.driveId
is present (keep only the getFederatedShareLink path), and then simplify the
dependency array by removing getSharingLink, getOwner, and getSharingById since
they are not referenced in the effect body; this preserves the existing
getSharingLink usage at render-time (line ~190) and avoids redundant state
updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b3fe2f8-b296-4c0b-af02-2b6089e588d4
📒 Files selected for processing (6)
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsxpackages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsxpackages/cozy-sharing/src/components/ShareDialogCozyToCozy.jsxpackages/cozy-sharing/src/components/ShareDialogCozyToCozy.spec.jsxpackages/cozy-sharing/src/components/ShareDialogOnlyByLink.jsxpackages/cozy-sharing/src/components/ShareDialogOnlyByLink.spec.jsx
| const existingRecipients = existingDocument | ||
| ? getRecipients(existingDocument._id) | ||
| : [] | ||
| const displayedLink = existingDocument?.driveId |
There was a problem hiding this comment.
why it's static if the document has a driveId?
There was a problem hiding this comment.
Good catch. getFederatedShareLink is actually synchronous
So the useState + useEffect wrapper is unnecessary here .
I'll compute the federated link directly in displayedLink, just like getSharingLink
is used for non-federated documents, and remove the effect/state.
6d569c1 to
e79f4f6
Compare
e79f4f6 to
e05c313
Compare
- Bump `cozy-sharing` from `33.1.1` to `33.2.2` - [linagora/cozy-libs#2994](linagora/cozy-libs#2994) (`fix(cozy-sharing): Make matching diacritic-insensitive`) - [linagora/cozy-libs#3021](linagora/cozy-libs#3021) (`feat(cozy-sharing): Show progress icon on done button during share`) - [linagora/cozy-libs#3022](linagora/cozy-libs#3022) (`fix(cozy-sharing): Refresh link state in share modal`) - [linagora/cozy-libs#3023](linagora/cozy-libs#3023) (`fix(cozy-sharing): Allow sharing shared drive folder root by email`)
https://app.notion.com/p/linagora/5d2f7429646042378356ecb072890d35?v=1c062718bad180d3847f000cd4c97139&p=37462718bad18075bab2e83c56b23d26&pm=s
Summary by CodeRabbit
Refactor
Tests