feat(cozy-sharing): Show shared drive links#3032
Conversation
Walkthrough
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
doubleface
changed the title
doubleface
force-pushed
the
feat/readSharedDriveLinks
branch
from
4bc9c7d to
971c2ae
Compare
Track the current fetch attempt per document so the federated folder modal does not refetch indefinitely when fetched permissions stay unindexed.
doubleface
force-pushed
the
feat/readSharedDriveLinks
branch
from
971c2ae to
2e1a2cf
Compare
![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)
packages/cozy-sharing/src/SharingProvider.jsx (1)
451-453: Inconsistent input format foraddSharingLinkaction may reduce code clarity.Line 381 passes a single permission object (
resp.data), while line 451-453 passes an array of permissions. The reducer handles both formats via anArray.isArray()check, so the code works correctly, but this inconsistency could confuse maintainers. Consider normalizing the input format at the call sites (e.g., always passing an array, or wrapping single objects) for clearer intent.🤖 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/SharingProvider.jsx` around lines 451 - 453, The addSharingLink action is being called with inconsistent input formats across the codebase—sometimes with a single permission object and sometimes with an array of permissions. Normalize the input format at all call sites of addSharingLink to use a consistent approach (either always passing an array, or always wrapping single objects in an array). This will eliminate the need for the Array.isArray() check in the reducer and make the expected input format immediately clear to maintainers.
🤖 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
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 189-218: The useEffect hook that fetches sharing links performs
state updates in the finally block even if the component unmounts before the
promise settles, causing React warnings. Add a cleanup mechanism to prevent
state updates after unmount by either using an AbortController to cancel the
fetch operation or by tracking a mounted flag with a ref that gets set to false
in the cleanup function returned by the effect. Before calling
setFetchedSharingLinksDocumentId and setIsFetchingSharingLinks in the finally
block, check the abort signal status or mounted flag to ensure the component is
still mounted before performing the state updates.
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx`:
- Around line 287-294: The test assertion
`expect(mockFetchSharedDriveSharingLinks).toHaveBeenCalledTimes(1)` immediately
after the `rerender()` call may execute before the `useEffect` hook runs,
causing the test to miss a real duplicate-fetch regression. Wrap the assertion
in an asynchronous utility like `waitFor()` to ensure the component's
`useEffect` has completed and the mock call count is stable before verifying
that the fetch was only called once after the rerender.
---
Nitpick comments:
In `@packages/cozy-sharing/src/SharingProvider.jsx`:
- Around line 451-453: The addSharingLink action is being called with
inconsistent input formats across the codebase—sometimes with a single
permission object and sometimes with an array of permissions. Normalize the
input format at all call sites of addSharingLink to use a consistent approach
(either always passing an array, or always wrapping single objects in an array).
This will eliminate the need for the Array.isArray() check in the reducer and
make the expected input format immediately clear to maintainers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8eef2511-80ca-415c-b543-05f7b3ac4f54
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/cozy-sharing/package.jsonpackages/cozy-sharing/src/SharingProvider.jsxpackages/cozy-sharing/src/SharingProvider.spec.jsxpackages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsxpackages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx
| useEffect(() => { | ||
| if (!existingDocument?.driveId) return | ||
| if (!documentId) return | ||
| if (!fetchSharedDriveSharingLinks) return | ||
| if (documentPermissions.length > 0) return | ||
| if (isFetchingSharingLinks) return | ||
| if (fetchedSharingLinksDocumentId === documentId) return | ||
|
|
||
| const fetchSharingLinks = async () => { | ||
| setIsFetchingSharingLinks(true) | ||
|
|
||
| try { | ||
| await fetchSharedDriveSharingLinks(existingDocument) | ||
| } catch (error) { | ||
| log.error('Failed to fetch shared drive sharing links', error) | ||
| } finally { | ||
| setFetchedSharingLinksDocumentId(documentId) | ||
| setIsFetchingSharingLinks(false) | ||
| } | ||
| } | ||
|
|
||
| fetchSharingLinks() | ||
| }, [ | ||
| documentId, | ||
| existingDocument, | ||
| documentPermissions.length, | ||
| fetchedSharingLinksDocumentId, | ||
| fetchSharedDriveSharingLinks, | ||
| isFetchingSharingLinks | ||
| ]) |
There was a problem hiding this comment.
Avoid state updates after unmount in the async prefetch effect.
If the modal closes before fetchSharedDriveSharingLinks settles, the finally block still calls setState, which can trigger React “update on unmounted component” warnings.
Suggested fix
useEffect(() => {
+ let isMounted = true
+
if (!existingDocument?.driveId) return
if (!documentId) return
if (!fetchSharedDriveSharingLinks) return
if (documentPermissions.length > 0) return
if (isFetchingSharingLinks) return
if (fetchedSharingLinksDocumentId === documentId) return
const fetchSharingLinks = async () => {
- setIsFetchingSharingLinks(true)
+ if (isMounted) setIsFetchingSharingLinks(true)
try {
await fetchSharedDriveSharingLinks(existingDocument)
} catch (error) {
log.error('Failed to fetch shared drive sharing links', error)
} finally {
- setFetchedSharingLinksDocumentId(documentId)
- setIsFetchingSharingLinks(false)
+ if (isMounted) {
+ setFetchedSharingLinksDocumentId(documentId)
+ setIsFetchingSharingLinks(false)
+ }
}
}
fetchSharingLinks()
+
+ return () => {
+ isMounted = false
+ }
}, [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!existingDocument?.driveId) return | |
| if (!documentId) return | |
| if (!fetchSharedDriveSharingLinks) return | |
| if (documentPermissions.length > 0) return | |
| if (isFetchingSharingLinks) return | |
| if (fetchedSharingLinksDocumentId === documentId) return | |
| const fetchSharingLinks = async () => { | |
| setIsFetchingSharingLinks(true) | |
| try { | |
| await fetchSharedDriveSharingLinks(existingDocument) | |
| } catch (error) { | |
| log.error('Failed to fetch shared drive sharing links', error) | |
| } finally { | |
| setFetchedSharingLinksDocumentId(documentId) | |
| setIsFetchingSharingLinks(false) | |
| } | |
| } | |
| fetchSharingLinks() | |
| }, [ | |
| documentId, | |
| existingDocument, | |
| documentPermissions.length, | |
| fetchedSharingLinksDocumentId, | |
| fetchSharedDriveSharingLinks, | |
| isFetchingSharingLinks | |
| ]) | |
| useEffect(() => { | |
| let isMounted = true | |
| if (!existingDocument?.driveId) return | |
| if (!documentId) return | |
| if (!fetchSharedDriveSharingLinks) return | |
| if (documentPermissions.length > 0) return | |
| if (isFetchingSharingLinks) return | |
| if (fetchedSharingLinksDocumentId === documentId) return | |
| const fetchSharingLinks = async () => { | |
| if (isMounted) setIsFetchingSharingLinks(true) | |
| try { | |
| await fetchSharedDriveSharingLinks(existingDocument) | |
| } catch (error) { | |
| log.error('Failed to fetch shared drive sharing links', error) | |
| } finally { | |
| if (isMounted) { | |
| setFetchedSharingLinksDocumentId(documentId) | |
| setIsFetchingSharingLinks(false) | |
| } | |
| } | |
| } | |
| fetchSharingLinks() | |
| return () => { | |
| isMounted = false | |
| } | |
| }, [ | |
| documentId, | |
| existingDocument, | |
| documentPermissions.length, | |
| fetchedSharingLinksDocumentId, | |
| fetchSharedDriveSharingLinks, | |
| isFetchingSharingLinks | |
| ]) |
🤖 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 189 - 218, The useEffect hook that fetches sharing links performs
state updates in the finally block even if the component unmounts before the
promise settles, causing React warnings. Add a cleanup mechanism to prevent
state updates after unmount by either using an AbortController to cancel the
fetch operation or by tracking a mounted flag with a ref that gets set to false
in the cleanup function returned by the effect. Before calling
setFetchedSharingLinksDocumentId and setIsFetchingSharingLinks in the finally
block, check the abort signal status or mounted flag to ensure the component is
still mounted before performing the state updates.
| rerender( | ||
| <AppLike client={client} sharingContextValue={sharingContextValue}> | ||
| <FederatedFolderModal document={document} onClose={mockOnClose} /> | ||
| </AppLike> | ||
| ) | ||
|
|
||
| expect(mockFetchSharedDriveSharingLinks).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
Make the post-rerender “no refetch” check asynchronous.
The immediate toHaveBeenCalledTimes(1) can pass before useEffect after rerender runs, so this test can miss a real duplicate-fetch regression.
Suggested fix
rerender(
<AppLike client={client} sharingContextValue={sharingContextValue}>
<FederatedFolderModal document={document} onClose={mockOnClose} />
</AppLike>
)
- expect(mockFetchSharedDriveSharingLinks).toHaveBeenCalledTimes(1)
+ await waitFor(() => {
+ expect(mockFetchSharedDriveSharingLinks).toHaveBeenCalledTimes(1)
+ })🤖 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.spec.jsx`
around lines 287 - 294, The test assertion
`expect(mockFetchSharedDriveSharingLinks).toHaveBeenCalledTimes(1)` immediately
after the `rerender()` call may execute before the `useEffect` hook runs,
causing the test to miss a real duplicate-fetch regression. Wrap the assertion
in an asynchronous utility like `waitFor()` to ensure the component's
`useEffect` has completed and the mock call count is stable before verifying
that the fetch was only called once after the rerender.
2 participants
https://app.notion.com/p/linagora/LINKS-on-reciver-side-doen-t-work-37c62718bad1801c8041e23d64eae181
Needs next cozy-stack-client with linagora/cozy-client#1689
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes