feat(cozy-sharing): Implement federated folder sharing modal#2931
Open
doubleface wants to merge 2 commits intomasterfrom
Open
feat(cozy-sharing): Implement federated folder sharing modal#2931doubleface wants to merge 2 commits intomasterfrom
doubleface wants to merge 2 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds federated shared-folder support to the cozy-sharing package. Introduces new locale keys 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 unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 8
🧹 Nitpick comments (9)
packages/cozy-sharing/src/components/ShareByEmail.spec.jsx (1)
176-180:createContactis declared but never reaches the component.
setup()doesn't accept acreateContactparameter (see line 36-41); it always creates its ownjest.fn(). The test still passes becausegetOrCreateFromArrayis fully mocked, but if you ever need to assert oncreateContactbeing called, this won't work.♻️ Suggested fix (either propagate or remove)
it('should directly share when selecting a recipient in federated mode', async () => { const sharingDesc = 'test' - const createContact = jest.fn() - setup({ sharingDesc, createContact }) + setup({ sharingDesc })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx` around lines 176 - 180, The test declares a createContact mock but setup() currently ignores it; update the setup function to accept an optional createContact parameter and use that mock instead of always creating a new jest.fn() so the test's createContact instance reaches the component (referencing setup and createContact in ShareByEmail.spec.jsx); ensure calls to getOrCreateFromArray mocks remain intact and that any places where setup created its own mock are replaced to use the passed-in createContact.packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx (2)
64-103: Fragile index parsing withsplit('virtual-shared-drive-sharing-').Both
onSetTypeandonRevokeextract_idby splitting on the'virtual-shared-drive-sharing-'prefix. This creates tight coupling with the index format produced byformatRecipients. If the prefix format changes, these functions silently break.Consider extracting the prefix as a shared constant or passing the
_iddirectly rather than encoding/decoding it.🤖 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.jsx` around lines 64 - 103, The handlers onSetType and onRevoke are fragile because they parse _id by splitting the hard-coded 'virtual-shared-drive-sharing-' string; change them to not depend on that literal: introduce a shared constant (e.g. RECIPIENT_INDEX_PREFIX) and use it when extracting the id or—preferably—change the callers/formatRecipients to pass the raw _id into onSetType/onRevoke directly so these functions accept an id param and use it (update onSetType, onRevoke and the code that calls them/formatRecipients accordingly).
29-29: UnnecessaryuseStatefor an immutable value.
folderNameis never updated (no setter is destructured). A plainconstwould be clearer and avoids the React state overhead.♻️ Suggested fix
- const [folderName] = useState(existingDocument?.name || '') + const folderName = existingDocument?.name || ''🤖 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.jsx` at line 29, Replace the unnecessary React state with a plain constant: in the FederatedFolderModal component, change "const [folderName] = useState(existingDocument?.name || '')" to "const folderName = existingDocument?.name || ''" and remove the unused useState import if it becomes unused; this keeps the value immutable and avoids React state overhead.packages/cozy-sharing/src/components/ShareByEmail.jsx (2)
185-185:isFederatedModeinshowShareControlis dead logic.
showShareControlincludes|| isFederatedMode, but the only consumer (line 202) is guarded by!isFederatedMode &&, making the federated branch of the OR unreachable. This is harmless but slightly confusing.♻️ Simplify
- const showShareControl = recipients.length > 0 || isFederatedMode + const showShareControl = recipients.length > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByEmail.jsx` at line 185, The showShareControl boolean includes an unnecessary || isFederatedMode term that is never used because the only consumer already checks !isFederatedMode; remove isFederatedMode from the showShareControl expression (leave it as recipients.length > 0) so the logic is not confusing, or alternatively consolidate by using showShareControl directly in the consumer instead of re-checking isFederatedMode — update references to the showShareControl constant (and any conditional that redundantly checks isFederatedMode) accordingly.
56-92: FederatedonRecipientPickswallows error details.The
catchblock at line 73 catches all errors but doesn't log them or include any details in the alert. Consider loggingerrfor debugging purposes, especially since this is a new code path.♻️ Suggested improvement
} catch (err) { + console.error('Failed to share with recipient in federated mode:', err) if (showNotifications) { showAlert({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByEmail.jsx` around lines 56 - 92, In onRecipientPick, the catch block for the federated branch currently swallows the error; update the catch to capture and log the error (e.g., via console.error or the existing logger) and include relevant error details in the showAlert payload so the alert and logs contain the actual err message/stack; ensure you still call reset() in finally and keep the existing showNotifications guard and alert fields (t('Share.shareByEmail.error.addingRecipient')) while appending the error details for debugging.packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx (2)
47-86: Action buttons logic is clear but the branching could benefit from a brief inline note.The three-way branching (create mode → share mode → rename mode) is sound for the supported use cases. Just noting: when
showNameFieldistruebut neitheronCreatenoronRenameis provided,actionButtonswill benull. This is presumably not a real usage scenario since consumers always provide one of these, but adding a PropTypescustomvalidator or a dev-mode warning could help catch misconfiguration early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx` around lines 47 - 86, The actionButtons computed branch can yield null when showNameField is true but neither onCreate nor onRename is provided; add a runtime dev-mode check and/or a PropTypes custom validator to warn developers so misconfiguration is caught early: update the component to validate props (use a custom PropTypes function that checks the combination of showNameField, onCreate and onRename) and emit a console.warn in development if showNameField is true while both onCreate and onRename are missing, referencing the actionButtons, showNameField, onCreate, onRename and onSend symbols to locate the logic.
153-178: PropTypes look reasonable buttitleand label props could be.isRequired.
title,nameLabel,addPeopleLabel,addButtonLabel,shareLabelare all used directly in the rendered output. Marking them.isRequiredwould surface misuse earlier in development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx` around lines 153 - 178, Update the propTypes for DumbBatchSharedFolderModal by marking the props that are rendered directly as required: change DumbBatchSharedFolderModal.propTypes for title, nameLabel, addPeopleLabel, addButtonLabel, and shareLabel to use PropTypes.string.isRequired (or the appropriate type) so missing values are caught earlier; keep other prop definitions as-is and ensure PropTypes is already imported.packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx (1)
54-54:shareLabeluses aFederatedFolderi18n key in a SharedDrive component.Line 54 passes
t("FederatedFolder.share")as theshareLabel. SinceSharedDriveModalalways setsshowNameField={true}, this label is never actually rendered in the shared-drive flow (it's only shown whenshowNameField=false). However, using aFederatedFolder.*key here is slightly misleading. Consider using a shared/generic i18n key or aSharedDrive.*key for clarity, even if it maps to the same string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx` at line 54, Replace the misleading FederatedFolder i18n key used for the shareLabel prop in SharedDriveModal: currently shareLabel={t("FederatedFolder.share")}, but SharedDriveModal always renders with showNameField={true} so that key is confusing; update the call to use a clearer i18n key such as t("SharedDrive.share") or a generic key like t("shared.share") (and add the corresponding entry in your i18n files), keeping the prop name shareLabel and the component SharedDriveModal unchanged.packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx (1)
36-42: Feature-flagged federated flow looks correct.The guard
isFederatedMode && !hasSharingscorrectly short-circuits toFederatedFolderModalonly for unshared documents when the feature flag is enabled, falling through to the existing editable/details flow otherwise. The optional chaining onbyDocId[document.id]?.sharings?.lengthis safe.One minor note: the comment says "for creating federated folders" but this is actually for sharing an existing folder in federated mode (based on the PR description and
FederatedFolderModal's behavior withexistingDocument). Consider updating the comment to match.Suggested comment update
- // In federated mode, show FederatedFolderModal for creating federated folders + // In federated mode, show FederatedFolderModal for sharing existing folders🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx` around lines 36 - 42, Update the inline comment above the federated flow guard to accurately describe behavior: change "for creating federated folders" to indicate this branch is for sharing an existing folder in federated mode (the guard using isFederatedMode and hasSharings returns FederatedFolderModal for unshared documents), referencing the isFederatedMode, hasSharings variables and the FederatedFolderModal usage so the comment matches the modal's existingDocument/sharing behavior.
🤖 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/locales/ru.json`:
- Around line 460-464: Update the Russian translation under the FederatedFolder
key so the success message reflects "shared" not "added": change the value of
successNotification (currently "Папка была добавлена") to a sharing-oriented
phrase such as "Папка была расшарена" or "Папка успешно расшарена" while keeping
share and errorNotification unchanged.
In `@packages/cozy-sharing/locales/vi.json`:
- Around line 458-462: The Vietnamese translations for FederatedFolder are using
"thêm" (added) instead of "chia sẻ" (shared); update the values of the
"FederatedFolder" object's "successNotification" and "errorNotification" keys to
reflect "shared" wording (e.g., use "Thư mục đã được chia sẻ" and "Lỗi khi chia
sẻ thư mục") so they match the English source and other locales; ensure you only
modify the "FederatedFolder" -> "successNotification" and "errorNotification"
entries in vi.json.
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 1-135: Add unit tests for FederatedFolderModal covering the core
flows: create a new spec file following the package .spec.jsx pattern and
mount/render the wrapped component (FederatedFolderModal) while mocking
useSharingContext (share, getSharingLink), useAlert (showAlert), and useClient
(create). Test onSend by simulating onShare to set federatedRecipients, then
triggering the onSend prop and asserting share was called with
description=document.name/sharedDrive:true and that showAlert was called with
success (and error path when share throws). Test onSetType by providing an
initial federatedRecipients (via calling the onShare prop or by mocking state)
that contains a recipient in readOnlyRecipients, call the onSetType handler with
an index like "virtual-shared-drive-sharing-<id>" and assert the recipient moves
to recipients; likewise test the opposite branch. Test onRevoke by calling
onRevoke with the same index and assert the recipient is removed from both
recipients and readOnlyRecipients. Use the DumbFederatedFolderModal props
(onShare, onSetType, onRevoke, onSend) to invoke handlers and assert state
transitions and mocked function calls.
- Around line 108-109: The modalTitle fallback uses
t('SharedDrive.sharedDriveModal.title') which resolves to "New shared drive" and
is misleading for federated folders; update the fallback to use a dedicated
federated key (e.g., t('FederatedFolder.title')) or add/rename a locale entry
for FederatedFolder and replace the current fallback in the modalTitle
assignment (referencing modalTitle and folderName) so the UI shows an
appropriate title when folderName is empty.
- Around line 38-46: The share() call in the onSend async handler is missing the
required openSharing argument; update the share invocation in
FederatedFolderModal.jsx (inside onSend) to pass openSharing (for example
openSharing: true or openSharing: federatedRecipients.recipients.length > 0 to
match ShareByEmail.jsx), and apply the same fix to the analogous call in
SharedDriveModal.jsx so both share(...) calls include the openSharing parameter
alongside description, document, recipients, readOnlyRecipients, and
sharedDrive.
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Around line 73-80: The code in ShareByEmail.jsx is using a non-existent
translation key t('Share.shareByEmail.error.addingRecipient') inside the catch
that shows an alert (when showNotifications is true); either add that key to all
locale files or, simpler, reuse the existing keys under Share.errors—update the
t(...) call in the ShareByEmail.jsx catch block (where showAlert is invoked) to
use the correct key from Share.errors (e.g., t('Share.errors.addingRecipient')
or whichever existing key matches) so the alert displays a real translated
message across locales.
In `@packages/cozy-sharing/src/components/SharedDrive/SharedDriveModal.jsx`:
- Around line 124-134: The prop document is being explicitly set to undefined
which breaks DumbShareByEmail and WhoHasAccess; either (A) supply a valid
minimal document object in creation mode (when showNameField is true) so
components like DumbShareByEmail (uses document._id and document.name) and
WhoHasAccess receive a bona fide object, or (B) make the components robust by
guarding against undefined: update DumbShareByEmail and WhoHasAccess to use
optional chaining (e.g., document?. _id, document?.name) and relax PropTypes to
not require document, ensuring both call sites (including SharedDriveModal) can
omit the document prop safely.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 42-44: The hasRecipients local state (hasRecipients /
setHasRecipients) is only set to true in onShare and never reset when recipients
are revoked, leaving the Send button enabled; change it to derive from the
actual recipients prop or sync via useEffect: remove the manual-only updates or
add a useEffect that sets setHasRecipients(Boolean(recipients?.length)) whenever
recipients changes, and also update onRevoke to call
setHasRecipients(Boolean(recipients?.length)) if you keep manual updates;
reference hasRecipients, setHasRecipients, onShare, onRevoke, and the recipients
prop/WhoHasAccess callback to locate the changes.
---
Nitpick comments:
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 64-103: The handlers onSetType and onRevoke are fragile because
they parse _id by splitting the hard-coded 'virtual-shared-drive-sharing-'
string; change them to not depend on that literal: introduce a shared constant
(e.g. RECIPIENT_INDEX_PREFIX) and use it when extracting the id
or—preferably—change the callers/formatRecipients to pass the raw _id into
onSetType/onRevoke directly so these functions accept an id param and use it
(update onSetType, onRevoke and the code that calls them/formatRecipients
accordingly).
- Line 29: Replace the unnecessary React state with a plain constant: in the
FederatedFolderModal component, change "const [folderName] =
useState(existingDocument?.name || '')" to "const folderName =
existingDocument?.name || ''" and remove the unused useState import if it
becomes unused; this keeps the value immutable and avoids React state overhead.
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Line 185: The showShareControl boolean includes an unnecessary ||
isFederatedMode term that is never used because the only consumer already checks
!isFederatedMode; remove isFederatedMode from the showShareControl expression
(leave it as recipients.length > 0) so the logic is not confusing, or
alternatively consolidate by using showShareControl directly in the consumer
instead of re-checking isFederatedMode — update references to the
showShareControl constant (and any conditional that redundantly checks
isFederatedMode) accordingly.
- Around line 56-92: In onRecipientPick, the catch block for the federated
branch currently swallows the error; update the catch to capture and log the
error (e.g., via console.error or the existing logger) and include relevant
error details in the showAlert payload so the alert and logs contain the actual
err message/stack; ensure you still call reset() in finally and keep the
existing showNotifications guard and alert fields
(t('Share.shareByEmail.error.addingRecipient')) while appending the error
details for debugging.
In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx`:
- Around line 176-180: The test declares a createContact mock but setup()
currently ignores it; update the setup function to accept an optional
createContact parameter and use that mock instead of always creating a new
jest.fn() so the test's createContact instance reaches the component
(referencing setup and createContact in ShareByEmail.spec.jsx); ensure calls to
getOrCreateFromArray mocks remain intact and that any places where setup created
its own mock are replaced to use the passed-in createContact.
In `@packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx`:
- Line 54: Replace the misleading FederatedFolder i18n key used for the
shareLabel prop in SharedDriveModal: currently
shareLabel={t("FederatedFolder.share")}, but SharedDriveModal always renders
with showNameField={true} so that key is confusing; update the call to use a
clearer i18n key such as t("SharedDrive.share") or a generic key like
t("shared.share") (and add the corresponding entry in your i18n files), keeping
the prop name shareLabel and the component SharedDriveModal unchanged.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 47-86: The actionButtons computed branch can yield null when
showNameField is true but neither onCreate nor onRename is provided; add a
runtime dev-mode check and/or a PropTypes custom validator to warn developers so
misconfiguration is caught early: update the component to validate props (use a
custom PropTypes function that checks the combination of showNameField, onCreate
and onRename) and emit a console.warn in development if showNameField is true
while both onCreate and onRename are missing, referencing the actionButtons,
showNameField, onCreate, onRename and onSend symbols to locate the logic.
- Around line 153-178: Update the propTypes for DumbBatchSharedFolderModal by
marking the props that are rendered directly as required: change
DumbBatchSharedFolderModal.propTypes for title, nameLabel, addPeopleLabel,
addButtonLabel, and shareLabel to use PropTypes.string.isRequired (or the
appropriate type) so missing values are caught earlier; keep other prop
definitions as-is and ensure PropTypes is already imported.
In `@packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx`:
- Around line 36-42: Update the inline comment above the federated flow guard to
accurately describe behavior: change "for creating federated folders" to
indicate this branch is for sharing an existing folder in federated mode (the
guard using isFederatedMode and hasSharings returns FederatedFolderModal for
unshared documents), referencing the isFederatedMode, hasSharings variables and
the FederatedFolderModal usage so the comment matches the modal's
existingDocument/sharing behavior.
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx
Show resolved
Hide resolved
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx
Show resolved
Hide resolved
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx
Outdated
Show resolved
Hide resolved
packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx
Show resolved
Hide resolved
doubleface
force-pushed
the
feat/deepSharedFolders
branch
from
9fd4c68 to
a5ec334
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/cozy-sharing/src/components/ShareByEmail.jsx (1)
185-185:|| isFederatedModeinshowShareControlis dead code.
showShareControlis only consumed at line 202 inside!isFederatedMode && showShareControl && (...). WhenisFederatedModeistrue, the entire block is already skipped by!isFederatedMode, so the extra|| isFederatedModeterm never influences rendering and is unreachable. The expression simplifies torecipients.length > 0.♻️ Proposed simplification
- const showShareControl = recipients.length > 0 || isFederatedMode + const showShareControl = recipients.length > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByEmail.jsx` at line 185, The showShareControl constant contains unreachable logic: change its definition from "const showShareControl = recipients.length > 0 || isFederatedMode" to simply "const showShareControl = recipients.length > 0" (remove the "|| isFederatedMode" term) so the value reflects actual usage in the "!isFederatedMode && showShareControl && ..." render branch and eliminate the dead condition.packages/cozy-sharing/src/components/ShareByLink.jsx (1)
109-110: MoveisFederatednear the other constant declarations at the top of the component.All other
constbindings (canShare,showCopyAndSendButtons,showOnlyCopyButton) are grouped at lines 29–31, right after the hook calls. DeclaringisFederatedimmediately before thereturnis inconsistent with that pattern and makes it easy to miss when reading the component.♻️ Suggested re-ordering
const { shareByLink } = useSharingContext() const [isGenerating, setIsGenerating] = useState(false) + const isFederated = flag('drive.federated-shared-folder.enabled') const canShare = typeof navigator?.share === 'function' const showCopyAndSendButtons = isMobile && canShare const showOnlyCopyButton = !isMobile || !canShare // ... function declarations ... - - const isFederated = flag('drive.federated-shared-folder.enabled') - return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByLink.jsx` around lines 109 - 110, Move the isFederated = flag('drive.federated-shared-folder.enabled') declaration up with the other const bindings right after the hook calls so it sits alongside canShare, showCopyAndSendButtons, and showOnlyCopyButton; update the top of the ShareByLink component to declare isFederated with those constants (keeping the same flag(...) call) and remove the late declaration before the return so the grouping of related feature flags/constants is consistent and easier to read.
🤖 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/ShareByEmail.jsx`:
- Around line 56-85: The federated branch of onRecipientPick (guarded by
isFederatedMode) never sets loading before the async work, so concurrent picks
can trigger overlapping onShare writes; call setLoading(true) at the start of
the federated branch (before getOrCreateFromArray and onShare) and rely on the
existing finally/reset() to clear it (reset calls setLoading(false)); also
consider disabling the ShareRecipientsInput while loading so the input cannot be
interacted with during the federated onShare flow (reference: onRecipientPick,
isFederatedMode, getOrCreateFromArray, onShare, reset, setLoading,
ShareRecipientsInput).
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 153-156: DumbBatchSharedFolderModal.propTypes currently lists
document as PropTypes.object but it must be required because the component
passes it directly to AntivirusAlert, DumbShareByEmail, and WhoHasAccess (see
usages in render). Update DumbBatchSharedFolderModal.propTypes to mark document
as PropTypes.object.isRequired (or a more specific shape if available) so
callers are warned and runtime null/undefined will be prevented.
- Around line 102-112: The TextField is conditionally rendered by showNameField
&& handleFolderNameChange but passes folderName directly, which can be undefined
and cause uncontrolled-to-controlled warnings; update the value passed to
TextField (in the SharedFolder/DumbBatchSharedFolderModal.jsx component) to
coalesce folderName to an empty string (or another safe default) so the input is
always controlled (e.g., use folderName ?? '' or String(folderName) fallback)
and keep the existing onChange handler handleFolderNameChange.
- Around line 64-70: The Send button uses the optional prop onSend as its sole
onClick handler when showNameField is false, which can cause a silent no-op;
update the component so onSend cannot be missing: either mark onSend as required
in the PropTypes for this component (i.e. PropTypes.func.isRequired for onSend)
or guard the button by changing its disabled condition to include the handler
(e.g. disabled={!hasRecipients || !onSend}) so the Button (where
label={shareLabel} and onClick={onSend}) is not clickable when onSend is absent.
---
Duplicate comments:
In `@packages/cozy-sharing/locales/ru.json`:
- Around line 460-464: The FederatedFolder translation uses verbs for "adding"
instead of "sharing": update the values for the keys
FederatedFolder.successNotification and FederatedFolder.errorNotification to use
sharing vocabulary (e.g., "Папка успешно расшарена" for success and "Ошибка при
расшаривании папки" or "Не удалось расшарить папку" for error) so the Russian
matches the intended action and the French counterpart; locate the
FederatedFolder object and replace the two strings accordingly.
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Around line 73-80: Add the missing translation key
"Share.shareByEmail.error.addingRecipient" to all locale files used by the app
(e.g., en, fr, es, de) with appropriate localized strings so the call in
ShareByEmail.jsx (the showAlert message
t('Share.shareByEmail.error.addingRecipient')) resolves; update each locale's
translation JSON/JS module to include this key under the Share -> shareByEmail
-> error namespace and run i18n/lint checks to ensure no other missing keys
remain.
In `@packages/cozy-sharing/src/components/SharedDrive/SharedDriveModal.jsx`:
- Line 134: SharedDriveModal currently passes document={undefined} which causes
runtime TypeErrors in ShareByEmail and WhoHasAccess (they access document._id
and document.name and declare document: PropTypes.object.isRequired). Fix by
either (A) supplying a minimal placeholder document object when in creation mode
(e.g. { _id: null, name: '' }) from SharedDriveModal so ShareByEmail and
WhoHasAccess always receive an object, or (B) update ShareByEmail and
WhoHasAccess to safely guard accesses with optional chaining (document?._id,
document?.name) and relax PropTypes to PropTypes.object (not required); ensure
this change covers the showNameField={true} rendering path.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 42-44: The hasRecipients state (initialized via
hasRecipientsInitially) is only updated in the onShare callback, so when
WhoHasAccess triggers an onRevoke and all recipients are removed the Send button
remains enabled; update the revoke path to call setHasRecipients(false) when the
recipients list becomes empty—specifically wire the WhoHasAccess onRevoke
handler (or the parent callback that receives revoke events) to compute whether
recipients.length === 0 and call setHasRecipients accordingly, and ensure any
sharedRecipient change code (e.g., in the component rendering WhoHasAccess) also
updates setHasRecipients to reflect the current recipients count.
---
Nitpick comments:
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Line 185: The showShareControl constant contains unreachable logic: change its
definition from "const showShareControl = recipients.length > 0 ||
isFederatedMode" to simply "const showShareControl = recipients.length > 0"
(remove the "|| isFederatedMode" term) so the value reflects actual usage in the
"!isFederatedMode && showShareControl && ..." render branch and eliminate the
dead condition.
In `@packages/cozy-sharing/src/components/ShareByLink.jsx`:
- Around line 109-110: Move the isFederated =
flag('drive.federated-shared-folder.enabled') declaration up with the other
const bindings right after the hook calls so it sits alongside canShare,
showCopyAndSendButtons, and showOnlyCopyButton; update the top of the
ShareByLink component to declare isFederated with those constants (keeping the
same flag(...) call) and remove the late declaration before the return so the
grouping of related feature flags/constants is consistent and easier to read.
packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx
Show resolved
Hide resolved
packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx
Show resolved
Hide resolved
Comment on lines
+153
to
+156
| DumbBatchSharedFolderModal.propTypes = { | ||
| title: PropTypes.string, | ||
| document: PropTypes.object, | ||
| folderName: PropTypes.string, |
There was a problem hiding this comment.
document prop should be marked .isRequired.
document is passed directly to AntivirusAlert, DumbShareByEmail, and WhoHasAccess (lines 101, 119, 138) without any null-guard. If a caller omits it, child components will encounter a null/undefined document and throw at runtime. The PropTypes declaration should reflect this:
🛡️ Proposed fix
- document: PropTypes.object,
+ document: PropTypes.object.isRequired,📝 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.
Suggested change
| DumbBatchSharedFolderModal.propTypes = { | |
| title: PropTypes.string, | |
| document: PropTypes.object, | |
| folderName: PropTypes.string, | |
| DumbBatchSharedFolderModal.propTypes = { | |
| title: PropTypes.string, | |
| document: PropTypes.object.isRequired, | |
| folderName: PropTypes.string, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`
around lines 153 - 156, DumbBatchSharedFolderModal.propTypes currently lists
document as PropTypes.object but it must be required because the component
passes it directly to AntivirusAlert, DumbShareByEmail, and WhoHasAccess (see
usages in render). Update DumbBatchSharedFolderModal.propTypes to mark document
as PropTypes.object.isRequired (or a more specific shape if available) so
callers are warned and runtime null/undefined will be prevented.
a5ec334 to
6ac35c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx (1)
54-54:FederatedFolder.sharei18n key cross-pollinatesSharedDrivenamespace.Using
t('FederatedFolder.share')inside aSharedDrivecomponent couples two distinct feature areas at the translation layer. If theFederatedFolderlocale keys are ever restructured or the component is extracted, the label will silently fall back or break. A dedicatedSharedDrivekey (e.g.SharedDrive.sharedDriveModal.share) would keep the namespaces self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx` at line 54, The translation key t('FederatedFolder.share') used in DumbSharedDriveModal.jsx couples namespaces; change the prop assignment for shareLabel to use a SharedDrive-specific key (for example t('SharedDrive.sharedDriveModal.share')) and add the new key to the SharedDrive locale files; update any tests or snapshots that assert the old text if present and search for other uses of FederatedFolder.share in this component to replace them with the SharedDrive key so the SharedDrive namespace is self-contained.packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx (1)
40-42: Federated mode with an already-shared folder falls through silently to the legacy modals — document the intentWhen
isFederatedModeistrueandhasSharingsistrue, the code falls through to the existingEditableSharingModal/SharingDetailsModalpath. The comment on line 39 only explains the!hasSharingsbranch. It is unclear (and untested) whether the legacy modals correctly handle a folder that was shared via the federated API. If this is out of scope for this PR, add a comment to make the intent explicit and prevent future regressions.📝 Suggested clarifying comment
// In federated mode, show FederatedFolderModal for unshared documents (sharing an existing folder) if (isFederatedMode && !hasSharings) { return <FederatedFolderModal document={document} onClose={rest.onClose} /> } + // For documents already shared in federated mode, fall through to the standard + // sharing modals (managing existing recipients is not yet part of the federated UI).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx` around lines 40 - 42, Add a clarifying comment above the isFederatedMode check explaining that when isFederatedMode is true and hasSharings is true we intentionally fall through to the legacy modals (EditableSharingModal / SharingDetailsModal) because federated-shared folders are handled by those components for now; reference the condition (isFederatedMode && hasSharings), the early-return branch using FederatedFolderModal, and add a TODO note to add tests or revisit handling of federated-shared folders if/when support is moved out of the legacy modals.packages/cozy-sharing/src/components/ShareByEmail.spec.jsx (1)
176-180: DeadcreateContactvariable —setup()never accepts it as a parameter.
setup()always creates its ownjest.fn()forcreateContactinternally. Passing a localcreateContacttosetup({ sharingDesc, createContact })has no effect, so the variable on line 178 is unused.♻️ Proposed cleanup
- const sharingDesc = 'test' - const createContact = jest.fn() - - setup({ sharingDesc, createContact }) + const sharingDesc = 'test' + + setup({ sharingDesc })If you want to assert specific
createContactusage, extendsetup()to accept and forward acreateContactparameter:const setup = ({ sharingDesc = 'test', document = defaultDocument, currentRecipients = [], - sharedDrive = false + sharedDrive = false, + createContact = jest.fn() } = {}) => { const props = { ... - createContact: jest.fn(), + createContact,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx` around lines 176 - 180, The local createContact variable in the test 'should directly share when selecting a recipient in federated mode' is dead because setup() creates its own jest.fn() internally; either remove the unused const createContact and the argument passed to setup({ sharingDesc, createContact }) so the test uses the internal mock, or modify setup(...) to accept a createContact parameter and forward it into the component/mocks it constructs (update setup's signature and internal wiring) so that the test's createContact jest.fn() is actually used; target the test case and the setup function to implement the chosen change.
🤖 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/DumbFederatedFolderModal.jsx`:
- Around line 40-41: The component DumbFederatedFolderModal.jsx is using the
SharedDrive translation key t('SharedDrive.sharedDriveModal.addPeople') which
produces "shared drive" wording; add a new FederatedFolder.addPeople key across
all locale files (en.json, fr.json, ru.json, vi.json) with the correct
folder-specific copy, then update DumbFederatedFolderModal.jsx to use
t('FederatedFolder.addPeople') for addPeopleLabel (and verify addButtonLabel if
it also points to SharedDrive keys and switch it to a FederatedFolder key if
needed) so the modal uses the folder-specific translations.
In `@packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx`:
- Around line 36-42: The federated-modal branch is rendering prematurely because
hasSharings uses byDocId[document.id] before the sharing context finishes
loading; update the guard in the ShareModal component (where isFederatedMode and
hasSharings are computed) to wait for the sharing context to finish (use the
allLoaded flag from the SharingProvider or check a loaded boolean from
fetchAllSharings) before deciding to render FederatedFolderModal: only
evaluate/render FederatedFolderModal when isFederatedMode && allLoaded &&
!hasSharings (or when hasSharings is explicitly false and the context is
loaded), leaving other rendering paths unchanged.
---
Duplicate comments:
In `@packages/cozy-sharing/locales/fr.json`:
- Around line 330-332: The translation key is nested under
Organizations.share.shareByEmail.error but the code (see ShareByEmail.jsx,
reference at line 77) expects Share.shareByEmail.error.addingRecipient; move or
duplicate the French entry so the key path exists as
Share.shareByEmail.error.addingRecipient (matching the other locale vi.json) so
the lookup in ShareByEmail.jsx resolves correctly.
In `@packages/cozy-sharing/locales/vi.json`:
- Around line 332-334: The translation key "addingRecipient" is under
Organizations.share.shareByEmail.error but the code calls
t('Share.shareByEmail.error.addingRecipient') in ShareByEmail.jsx; move the
"addingRecipient" entry from Organizations.share.shareByEmail.error to
Share.shareByEmail.error (i.e., ensure the JSON path matches the lookup) so the
t('Share.shareByEmail.error.addingRecipient') call resolves correctly.
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Around line 74-80: The i18n lookup in ShareByEmail.jsx uses
t('Share.shareByEmail.error.addingRecipient') but the locale files currently
define the key under Organizations.share.shareByEmail.error.addingRecipient;
update the code so the t() call points to the existing document-type-scoped path
(t('Organizations.share.shareByEmail.error.addingRecipient')) or alternatively
move the translation entries in all locale files (en/fr/ru/vi) into
Share.share.shareByEmail.error.addingRecipient to match the current t()
usage—pick one approach and make the t() string and the JSON keys consistent
across ShareByEmail.jsx and the locale files.
---
Nitpick comments:
In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx`:
- Around line 176-180: The local createContact variable in the test 'should
directly share when selecting a recipient in federated mode' is dead because
setup() creates its own jest.fn() internally; either remove the unused const
createContact and the argument passed to setup({ sharingDesc, createContact })
so the test uses the internal mock, or modify setup(...) to accept a
createContact parameter and forward it into the component/mocks it constructs
(update setup's signature and internal wiring) so that the test's createContact
jest.fn() is actually used; target the test case and the setup function to
implement the chosen change.
In `@packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx`:
- Line 54: The translation key t('FederatedFolder.share') used in
DumbSharedDriveModal.jsx couples namespaces; change the prop assignment for
shareLabel to use a SharedDrive-specific key (for example
t('SharedDrive.sharedDriveModal.share')) and add the new key to the SharedDrive
locale files; update any tests or snapshots that assert the old text if present
and search for other uses of FederatedFolder.share in this component to replace
them with the SharedDrive key so the SharedDrive namespace is self-contained.
In `@packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx`:
- Around line 40-42: Add a clarifying comment above the isFederatedMode check
explaining that when isFederatedMode is true and hasSharings is true we
intentionally fall through to the legacy modals (EditableSharingModal /
SharingDetailsModal) because federated-shared folders are handled by those
components for now; reference the condition (isFederatedMode && hasSharings),
the early-return branch using FederatedFolderModal, and add a TODO note to add
tests or revisit handling of federated-shared folders if/when support is moved
out of the legacy modals.
packages/cozy-sharing/src/components/FederatedFolder/DumbFederatedFolderModal.jsx
Outdated
Show resolved
Hide resolved
doubleface
force-pushed
the
feat/deepSharedFolders
branch
2 times, most recently
from
24412f8 to
569f201
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx (1)
97-104:sharingContextValuemay be unused — module-leveljest.mockalready provides the sharing context.
useSharingContextis mocked at module scope (Lines 14-20) to returnmockShare/mockGetSharingLinkdirectly. ThesharingContextValueconstructed increateSharingContextValueand passed toAppLike(Line 140) duplicates those mocks, but since the hook is fully replaced at module level, theAppLikeprop likely has no effect. ThecreateSharingContextValuefactory andsharingContextValueassignment can be removed to reduce noise.♻️ Suggested cleanup
-const createSharingContextValue = () => ({ - refresh: jest.fn(), - hasWriteAccess: jest.fn(), - getRecipients: jest.fn(), - getSharingLink: mockGetSharingLink, - getDocumentPermissions: jest.fn().mockReturnValue([]), - share: mockShare -}) describe('FederatedFolderModal', () => { let client - let sharingContextValue beforeEach(() => { jest.clearAllMocks() mockGetSharingLink.mockReturnValue('https://example.com/share/abc123') client = createTestClient() - sharingContextValue = createSharingContextValue() }) const setup = (props = {}) => { return render( - <AppLike client={client} sharingContextValue={sharingContextValue}> + <AppLike client={client}>Also applies to: 130-136
🤖 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 97 - 104, The createSharingContextValue factory and the sharingContextValue it returns are redundant because useSharingContext is already mocked at module scope; remove the createSharingContextValue function and any sharingContextValue variable/assignment, and stop passing sharingContextValue into the AppLike wrapper in the tests (e.g., remove the prop passed at the render call that uses AppLike). Ensure tests still rely on the module-level jest.mock of useSharingContext (mockShare and mockGetSharingLink) and delete any now-unused imports or jest.fn() stubs (like getRecipients/getDocumentPermissions) created only for that factory.packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx (1)
38-63:onSendhas no guard for an empty recipient list.Unlike
ShareByEmail.share()(which returns early whenrecipients.length === 0),onSendwill callshare()withrecipients: []andreadOnlyRecipients: []if the user clicks "Send" without adding anyone. Consider adding a guard or ensuring the Send button is disabled at theDumbFederatedFolderModallevel when both lists are empty.♻️ Suggested guard
const onSend = async () => { + if ( + federatedRecipients.recipients.length === 0 && + federatedRecipients.readOnlyRecipients.length === 0 + ) { + return + } try {🤖 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.jsx` around lines 38 - 63, Add a guard in the onSend handler to prevent calling share() with empty recipient lists: check federatedRecipients.recipients.length and federatedRecipients.readOnlyRecipients.length and if both are zero, show an error alert (or return early) instead of calling share(); alternatively ensure the Send control in DumbFederatedFolderModal is disabled when both lists are empty—update the onSend function (and/or DumbFederatedFolderModal props/state) so share() is only invoked when there is at least one recipient.packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx (1)
126-128: RedundantonSharewrapper can be simplified.Since the
setHasRecipientscall was moved to theuseEffect(lines 46-48), the arrow wrapperparams => { onShare(params) }is now equivalent to passingonSharedirectly. It unnecessarily allocates a new function reference on each render, which could cause spurious re-renders ifDumbShareByEmailever becomes memoized.♻️ Proposed simplification
- onShare={params => { - onShare(params) - }} + onShare={onShare}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx` around lines 126 - 128, The onShare prop is being passed through an unnecessary arrow wrapper (onShare={params => { onShare(params) }}), which creates a new function every render; replace that wrapper by passing the handler directly (onShare={onShare}) in the DumbShareByEmail/DumbBatchSharedFolderModal usage to avoid allocating a new function reference and prevent potential spurious re-renders if the child is memoized.
🤖 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/locales/ru.json`:
- Line 467: The Russian localization key "errorNotification" currently uses an
"adding" phrasing; update the value for "errorNotification" to use a
sharing-related term (e.g., replace "Ошибка при добавлении папки" with "Ошибка
при расшаривании папки" or "Ошибка при совместном доступе к папке") so it
matches the corrected "successNotification" wording and removes the
adding/sharing inconsistency.
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 31-36: Keep the replace semantics in FederatedFolderModal.onShare
as-is (federated mode intentionally triggers immediate per-recipient shares via
onRecipientPick); the actual fix is in ShareByEmail: when invoking the federated
flow (the branch that currently calls onRecipientPick / passes federated-mode
props), ensure it does not pass openSharing: true — set openSharing: false to
match FederatedFolderModal.onSend and the federated requirement so
ShareByEmail.jsx's call site no longer sends a conflicting openSharing flag.
In `@packages/cozy-sharing/src/components/WhoHasAccess.jsx`:
- Line 92: The document prop was made optional but is passed unchecked into
callbacks, so update the call sites in GroupRecipientPermissions and
MemberRecipientPermissions to guard against undefined: when invoking
revokeGroup(document,...), revokeSelf(document), onRevoke(document,...) and
onRevokeSelf(document) only call them if document is present (e.g. if (document)
...), or alternatively forward a safe value (document?._id) per LinkRecipient's
pattern; ensure these callbacks (revokeGroup, revokeSelf, onRevoke,
onRevokeSelf) never receive undefined from WhoHasAccess.jsx by adding null
checks before calling them.
---
Duplicate comments:
In `@packages/cozy-sharing/locales/en.json`:
- Around line 332-334: The locale key "addingRecipient" is currently under
Organizations.share.shareByEmail.error but ShareByEmail.jsx calls
t('Share.shareByEmail.error.addingRecipient'), so move/create the key under
Share.shareByEmail.error.addingRecipient (create the Share.shareByEmail.error
subtree in packages/cozy-sharing/locales/en.json), copy the message from
Organizations.share.shareByEmail.error.addingRecipient into that new path, and
remove or keep the Organizations copy as needed for DRY; update other locale
files to mirror the same Share.shareByEmail.error.addingRecipient placement so
t() resolves correctly in ShareByEmail.jsx.
In `@packages/cozy-sharing/locales/fr.json`:
- Around line 330-332: The French locale places the string at
Organizations.share.shareByEmail.error.addingRecipient but the code
(ShareByEmail.jsx) expects Share.shareByEmail.error.addingRecipient; update one
side to match: either move/rename the key in
packages/cozy-sharing/locales/fr.json to
Share.shareByEmail.error.addingRecipient (matching ShareByEmail.jsx), or update
ShareByEmail.jsx to read Organizations.share.shareByEmail.error.addingRecipient;
ensure the unique identifiers (the JSON key path and the component name
ShareByEmail.jsx) are consistent across locales and code.
In `@packages/cozy-sharing/locales/vi.json`:
- Around line 461-468: The FederatedFolder translation block is correct and
needs no changes; keep the "FederatedFolder" object with keys title, share,
successNotification, errorNotification, addPeople, and add as-is (the corrected
"chia sẻ" for "share" is already applied) and ensure it remains consistent with
other locale files such as the same key names and structure.
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Around line 77-80: The error message currently uses the wrong translation key
('Share.shareByEmail.error.addingRecipient') which doesn't exist in locales;
update the translation call in ShareByEmail.jsx to use the
document-type-prefixed key (the same pattern as
t(`${documentType}.share.shareByEmail.emailPlaceholder`), i.e. use
`${documentType}.share.shareByEmail.error.addingRecipient`) and add the
`error.addingRecipient` string under `shareByEmail` in each document-type
section (Files.share, Notes.share, Albums.share, Organizations.share) for all
locale files (en/fr/ru/vi) so the key resolves to a proper translated message.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 152-155: The propTypes for DumbBatchSharedFolderModal currently
leave document optional but the component passes document directly into
AntivirusAlert, DumbShareByEmail, and WhoHasAccess; update
DumbBatchSharedFolderModal.propTypes to mark document as required (e.g.,
PropTypes.object.isRequired) so runtime consumers and linters enforce presence,
and ensure any tests or callers supply a non-null document to the component
(targets: DumbBatchSharedFolderModal.propTypes and usages in AntivirusAlert,
DumbShareByEmail, WhoHasAccess).
---
Nitpick comments:
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Around line 38-63: Add a guard in the onSend handler to prevent calling
share() with empty recipient lists: check federatedRecipients.recipients.length
and federatedRecipients.readOnlyRecipients.length and if both are zero, show an
error alert (or return early) instead of calling share(); alternatively ensure
the Send control in DumbFederatedFolderModal is disabled when both lists are
empty—update the onSend function (and/or DumbFederatedFolderModal props/state)
so share() is only invoked when there is at least one recipient.
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.spec.jsx`:
- Around line 97-104: The createSharingContextValue factory and the
sharingContextValue it returns are redundant because useSharingContext is
already mocked at module scope; remove the createSharingContextValue function
and any sharingContextValue variable/assignment, and stop passing
sharingContextValue into the AppLike wrapper in the tests (e.g., remove the prop
passed at the render call that uses AppLike). Ensure tests still rely on the
module-level jest.mock of useSharingContext (mockShare and mockGetSharingLink)
and delete any now-unused imports or jest.fn() stubs (like
getRecipients/getDocumentPermissions) created only for that factory.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 126-128: The onShare prop is being passed through an unnecessary
arrow wrapper (onShare={params => { onShare(params) }}), which creates a new
function every render; replace that wrapper by passing the handler directly
(onShare={onShare}) in the DumbShareByEmail/DumbBatchSharedFolderModal usage to
avoid allocating a new function reference and prevent potential spurious
re-renders if the child is memoized.
packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx
Show resolved
Hide resolved
| }) | ||
| ), | ||
| document: PropTypes.object.isRequired, | ||
| document: PropTypes.object, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how LinkRecipient and RecipientList consume the document prop
echo "=== LinkRecipient document usage ==="
rg -n 'document' packages/cozy-sharing/src/components/Recipient/LinkRecipient.jsx -B1 -A2 | head -40
echo ""
echo "=== RecipientList document usage ==="
rg -n 'document' packages/cozy-sharing/src/components/Recipient/RecipientList.jsx -B1 -A2 | head -40
echo ""
echo "=== Check PropTypes of LinkRecipient and RecipientList for document ==="
rg -n 'document.*PropTypes' packages/cozy-sharing/src/components/Recipient/Repository: linagora/cozy-libs
Length of output: 1085
🏁 Script executed:
#!/bin/bash
# Find the PropTypes definitions for LinkRecipient and RecipientList
echo "=== LinkRecipient full content ==="
cat packages/cozy-sharing/src/components/Recipient/LinkRecipient.jsx
echo ""
echo "=== RecipientList full content ==="
wc -l packages/cozy-sharing/src/components/Recipient/RecipientList.jsx
head -100 packages/cozy-sharing/src/components/Recipient/RecipientList.jsxRepository: linagora/cozy-libs
Length of output: 4942
🏁 Script executed:
#!/bin/bash
# Check PropTypes for GroupRecipient and MemberRecipient
echo "=== GroupRecipient PropTypes ==="
rg -n 'propTypes\|document.*PropTypes' packages/cozy-sharing/src/components/Recipient/GroupRecipient.jsx -A 10
echo ""
echo "=== MemberRecipient PropTypes ==="
rg -n 'propTypes\|document.*PropTypes' packages/cozy-sharing/src/components/Recipient/MemberRecipient.jsx -A 10Repository: linagora/cozy-libs
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Find the files first
echo "=== Finding GroupRecipient file ==="
fd 'GroupRecipient' packages/cozy-sharing/src/
echo ""
echo "=== Finding MemberRecipient file ==="
fd 'MemberRecipient' packages/cozy-sharing/src/
echo ""
echo "=== GroupRecipient content ==="
find packages/cozy-sharing/src -name '*GroupRecipient*' -type f -exec cat {} \;
echo ""
echo "=== MemberRecipient content ==="
find packages/cozy-sharing/src -name '*MemberRecipient*' -type f -exec cat {} \;Repository: linagora/cozy-libs
Length of output: 30214
Document must be safely handled in callback functions within child components.
Making document optional introduces a risk: GroupRecipientPermissions (line 55) passes it to revokeGroup(document, ...) and revokeSelf(document) without null checks. Similarly, MemberRecipientPermissions (line 45) passes it to onRevoke(document, ...) and onRevokeSelf(document). While LinkRecipient uses safe optional chaining (document?._id), the callback invocations in permission components should guard against undefined since document is no longer guaranteed at the component level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cozy-sharing/src/components/WhoHasAccess.jsx` at line 92, The
document prop was made optional but is passed unchecked into callbacks, so
update the call sites in GroupRecipientPermissions and
MemberRecipientPermissions to guard against undefined: when invoking
revokeGroup(document,...), revokeSelf(document), onRevoke(document,...) and
onRevokeSelf(document) only call them if document is present (e.g. if (document)
...), or alternatively forward a safe value (document?._id) per LinkRecipient's
pattern; ensure these callbacks (revokeGroup, revokeSelf, onRevoke,
onRevokeSelf) never receive undefined from WhoHasAccess.jsx by adding null
checks before calling them.
569f201 to
15b5350
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/cozy-sharing/src/components/Recipient/LinkRecipient.jsx (1)
76-78:documentis used but absent frompropTypes.Now that the optional-chaining makes
documentexplicitly optional, it's worth documenting that contract inpropTypes.♻️ Proposed addition to PropTypes
LinkRecipient.propTypes = { + document: PropTypes.shape({ _id: PropTypes.string }), link: PropTypes.string.isRequired }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/Recipient/LinkRecipient.jsx` around lines 76 - 78, LinkRecipient currently declares only link in LinkRecipient.propTypes but uses a document prop (now accessed via optional chaining), so add document to LinkRecipient.propTypes as an optional PropTypes.object (or a more specific shape if known) to document the contract and suppress prop-type warnings; update the LinkRecipient.propTypes declaration to include document: PropTypes.object (or PropTypes.shape({...})) without .isRequired to reflect its optional nature.packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx (1)
29-29:useStatewithout a setter forfolderNameis an anti-pattern.
folderNameis never updated — the setter is discarded. Use a plainconstinstead.♻️ Proposed fix
- const [folderName] = useState(existingDocument?.name || '') + const folderName = existingDocument?.name || ''🤖 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.jsx` at line 29, The hook usage for folderName is incorrect: replace the useState call that declares "const [folderName] = useState(existingDocument?.name || '')" with a plain constant "folderName" initialized from existingDocument (e.g., const folderName = existingDocument?.name || ''), and remove the unused useState import if it becomes unused; update any nearby code expecting a setter accordingly.packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx (1)
126-128:onSharewrapper creates a new function reference every render unnecessarily.
onShare={params => { onShare(params) }}is functionally identical toonShare={onShare}. The wrapper allocates a new closure on each render and may cause spurious re-renders in memoized children.♻️ Proposed fix
- onShare={params => { - onShare(params) - }} + onShare={onShare}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx` around lines 126 - 128, The inline arrow wrapper passed to the onShare prop creates a new function each render and can trigger unnecessary re-renders in memoized children; update the component to pass the handler directly (use the existing onShare reference instead of wrapping it) in DumbBatchSharedFolderModal.jsx so the prop is stable across renders (i.e., replace the params => { onShare(params) } wrapper with the direct onShare reference).packages/cozy-sharing/src/components/ShareByEmail.spec.jsx (1)
176-180:createContactlocal variable is unused dead code.
setup()does not destructurecreateContactfrom its options, so the localconst createContact = jest.fn()at line 178 is silently ignored —setupalways uses its internally createdjest.fn()at line 48. Either addcreateContacttosetup's destructuring, or remove this local declaration.♻️ Proposed fix
const setup = ({ sharingDesc = 'test', document = defaultDocument, currentRecipients = [], - sharedDrive = false + sharedDrive = false, + createContact = jest.fn() } = {}) => { const props = { documentType: 'Files', onShare, document, sharingDesc, currentRecipients, - createContact: jest.fn(), + createContact, sharedDrive }Then in the federated-mode test:
- const createContact = jest.fn() - setup({ sharingDesc, createContact }) + setup({ sharingDesc })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx` around lines 176 - 180, The test defines a local const createContact = jest.fn() but setup() ignores it, so either remove the unused createContact or make setup accept and use it; specifically, update the setup(options = {}) helper to destructure createContact from its options (e.g., const { createContact = jest.fn(), ... } = options) and ensure the setup uses that createContact mock when mounting/returning the component, or delete the local createContact declaration from the federated-mode test to remove dead code; refer to the createContact variable and the setup function to implement the change.packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx (1)
54-54:shareLabelborrows from theFederatedFolderi18n namespace — consider a SharedDrive-owned key.
t('FederatedFolder.share')couplesDumbSharedDriveModal's label to theFederatedFolderlocale namespace. In the current SharedDrive creation flow (showNameField=true) the label is never rendered (DumbBatchSharedFolderModal shows Cancel/Create instead), so there is no visible impact now. But ifDumbSharedDriveModalis ever called withshowNameField=false, the SharedDrive UI would silently inherit the FederatedFolder text, breaking independent label evolution.🛠️ Proposed refactor
Add a dedicated key in the SharedDrive locale namespace (e.g.
SharedDrive.sharedDriveModal.share) and use it here:- shareLabel={t('FederatedFolder.share')} + shareLabel={t('SharedDrive.sharedDriveModal.share')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx` at line 54, DumbSharedDriveModal currently uses t('FederatedFolder.share') for shareLabel which couples it to the FederatedFolder namespace; add a new i18n key under the SharedDrive namespace (e.g. "SharedDrive.sharedDriveModal.share") in the locale files and replace the call in DumbSharedDriveModal to use t('SharedDrive.sharedDriveModal.share') for shareLabel so the component owns its label text and can evolve independently.
🤖 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/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 120-131: DumbShareByEmail is called from
DumbBatchSharedFolderModal without the required sharingDesc prop; update the
DumbBatchSharedFolderModal component to accept a sharingDesc prop, add
sharingDesc to its PropTypes, and forward that prop to the DumbShareByEmail call
(i.e., include sharingDesc={sharingDesc} alongside the existing props in the
JSX). Ensure PropTypes.declarations for DumbBatchSharedFolderModal include
sharingDesc: PropTypes.string.isRequired so the warning is eliminated and
downstream share calls receive a defined description.
---
Duplicate comments:
In `@packages/cozy-sharing/locales/ru.json`:
- Line 467: The Russian translation key errorNotification currently reads
"Ошибка при добавлении папки" which is inconsistent with successNotification's
"расшарена"; update errorNotification to use the same "sharing" terminology
(e.g., "Ошибка при расшаривании папки" or a matching phrasing used by
successNotification) so both messages reflect the same action; change the value
for the errorNotification key in the ru.json to the chosen "sharing" wording to
restore consistency.
In `@packages/cozy-sharing/src/components/ShareByEmail.jsx`:
- Around line 66-73: The call to onShare in ShareByEmail.jsx passes openSharing:
true which looks odd; add a concise in-line comment next to this onShare
invocation explaining that FederatedFolderModal.onShare intentionally ignores
openSharing (it only extracts recipients/readOnlyRecipients) and that the actual
API call performed in onSend uses openSharing: false, so passing true here is
safe and preserved for compatibility; reference the onShare callback,
openSharing flag, FederatedFolderModal.onShare, and onSend in the comment for
future maintainers.
- Around line 74-81: The alert is using a non-existent locale path
(Share.shareByEmail.error.addingRecipient); update ShareByEmail.jsx to use the
documentType-scoped key (e.g. use
t(`${documentType}.share.shareByEmail.error.addingRecipient`) where t is called
in the catch block that calls showAlert) to match how DumbBatchSharedFolderModal
passes documentType="Files" into DumbShareByEmail, and add the missing
translation entry under Files.share.shareByEmail.error.addingRecipient in the
locale files (e.g. en.json and other locales) so the key resolves correctly.
In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx`:
- Around line 210-219: The test assertion in ShareByEmail.spec.jsx incorrectly
expects openSharing: true even though ShareByEmail.jsx (line ~71) only sets that
prop for the modal and the actual sharing path (FederatedFolderModal.onSend /
onShare callback) does not propagate openSharing and uses openSharing: false for
the real API; update the test to remove or correct the openSharing assertion so
it matches the real behavior (either drop openSharing from the onShare
expectation or assert openSharing: false) and ensure the expectation targets the
same fields the FederatedFolderModal.onShare handler consumes
(recipients/readOnlyRecipients/description/sharedDrive).
In `@packages/cozy-sharing/src/components/SharedDrive/SharedDriveModal.jsx`:
- Around line 130-135: The JSX passes explicit undefined props (onSend,
sharingLink, document) and an explicit true for showNameField to
DumbSharedDriveModal which is noisy and unnecessary; remove the redundant props
from the SharedDriveModal JSX (remove onSend={undefined},
sharingLink={undefined}, document={undefined}, and omit showNameField if
DumbSharedDriveModal defaults it to true) and, before removing document, verify
that components that read document (DumbShareByEmail/ShareByEmail, WhoHasAccess,
DumbBatchSharedFolderModal) guard against undefined (use optional chaining or
conditional checks around document._id and document.name) so no runtime errors
occur. Ensure onClose and onShare props remain unchanged.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 152-154: The propTypes for DumbBatchSharedFolderModal still
declare document as optional but it is passed unguarded into AntivirusAlert,
DumbShareByEmail, and WhoHasAccess; update DumbBatchSharedFolderModal.propTypes
to mark document as required (use PropTypes.object.isRequired) so the contract
matches usage and prevents missing-document runtime errors.
In `@packages/cozy-sharing/src/components/ShareModal/ShareModal.jsx`:
- Around line 37-46: No code change needed: the conditional guard using
isFederatedMode, allLoaded, and hasSharings correctly prevents premature
rendering of FederatedFolderModal and allows fallthrough to the legacy modals
(EditableSharingModal / SharingDetailsModal) when a document is already shared;
keep the if (isFederatedMode && allLoaded && !hasSharings) return
<FederatedFolderModal ... /> logic as-is.
---
Nitpick comments:
In
`@packages/cozy-sharing/src/components/FederatedFolder/FederatedFolderModal.jsx`:
- Line 29: The hook usage for folderName is incorrect: replace the useState call
that declares "const [folderName] = useState(existingDocument?.name || '')" with
a plain constant "folderName" initialized from existingDocument (e.g., const
folderName = existingDocument?.name || ''), and remove the unused useState
import if it becomes unused; update any nearby code expecting a setter
accordingly.
In `@packages/cozy-sharing/src/components/Recipient/LinkRecipient.jsx`:
- Around line 76-78: LinkRecipient currently declares only link in
LinkRecipient.propTypes but uses a document prop (now accessed via optional
chaining), so add document to LinkRecipient.propTypes as an optional
PropTypes.object (or a more specific shape if known) to document the contract
and suppress prop-type warnings; update the LinkRecipient.propTypes declaration
to include document: PropTypes.object (or PropTypes.shape({...})) without
.isRequired to reflect its optional nature.
In `@packages/cozy-sharing/src/components/ShareByEmail.spec.jsx`:
- Around line 176-180: The test defines a local const createContact = jest.fn()
but setup() ignores it, so either remove the unused createContact or make setup
accept and use it; specifically, update the setup(options = {}) helper to
destructure createContact from its options (e.g., const { createContact =
jest.fn(), ... } = options) and ensure the setup uses that createContact mock
when mounting/returning the component, or delete the local createContact
declaration from the federated-mode test to remove dead code; refer to the
createContact variable and the setup function to implement the change.
In `@packages/cozy-sharing/src/components/SharedDrive/DumbSharedDriveModal.jsx`:
- Line 54: DumbSharedDriveModal currently uses t('FederatedFolder.share') for
shareLabel which couples it to the FederatedFolder namespace; add a new i18n key
under the SharedDrive namespace (e.g. "SharedDrive.sharedDriveModal.share") in
the locale files and replace the call in DumbSharedDriveModal to use
t('SharedDrive.sharedDriveModal.share') for shareLabel so the component owns its
label text and can evolve independently.
In
`@packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx`:
- Around line 126-128: The inline arrow wrapper passed to the onShare prop
creates a new function each render and can trigger unnecessary re-renders in
memoized children; update the component to pass the handler directly (use the
existing onShare reference instead of wrapping it) in
DumbBatchSharedFolderModal.jsx so the prop is stable across renders (i.e.,
replace the params => { onShare(params) } wrapper with the direct onShare
reference).
packages/cozy-sharing/src/components/SharedFolder/DumbBatchSharedFolderModal.jsx
Show resolved
Hide resolved
doubleface
force-pushed
the
feat/deepSharedFolders
branch
from
15b5350 to
f7f0ee2
Compare
zatteo
reviewed
Feb 23, 2026
| recipients: prev.recipients.filter(r => r._id !== _id), | ||
| readOnlyRecipients: [...prev.readOnlyRecipients, recipientToMove] | ||
| } | ||
| }) |
Member
There was a problem hiding this comment.
Why not an helper function for this moving from read only to read write method?
Member
|
What does exactly means "federated mode"? Is is "just" the feature to create shared drive from folders? Or is it "more"? |
rezk2ll
reviewed
Feb 23, 2026
rezk2ll
reviewed
Feb 23, 2026
Member
rezk2ll
left a comment
rezk2ll
left a comment
There was a problem hiding this comment.
A few things I noticed:
-
DumbBatchSharedFolderModalpassesDumbShareByEmailwithout thesharingDescprop, which is marked as required in its propTypes. -
The title construction in
FederatedFolderModal.jsx:108concatenates strings manually:const modalTitle = t('FederatedFolder.share') + ` "${folderName}"`
This won't work well for languages with different word orders. Better to use interpolation in the translation key itself.
-
In
ShareByEmail.jsx, when federated mode is enabled, each recipient pick triggers an immediate share. Nothing prevents rapid clicks from firing multiple concurrent requests - consider disabling input while loading. -
The
hasRecipientsInitiallyprop inDumbBatchSharedFolderModalgets immediately overwritten by the useEffect that syncs withrecipients. Seems unnecessary. -
The magic string
'virtual-shared-drive-sharing-'appears in multiple places for parsing recipient IDs. Would be cleaner as a shared constant.
Otherwise looks good.
Contributor
Author
@zatteo This also means that federated shared folders will replace the previous way of sharing a folder. With this flag, when you will share a folder, it will be shared like a shared drive (ie without copying all the files to the recipient) |
added 2 commits
February 23, 2026 18:20
- This allows to share an existing folder using the shared drives API. - The feature is gated behind the drive.federated-shared-folder.enabled flag. - Created DumbBatchSharedFolderModal for code sharing between DumbSharedDriveModal and DumbFederatedFolderModal.
doubleface
force-pushed
the
feat/deepSharedFolders
branch
from
f7f0ee2 to
d0e7e5d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Capture.video.du.2026-02-18.17-17-48.webm
https://www.notion.so/linagora/New-Sharing-modal-for-federation-folder-2ef62718bad18017b62dcc321dfe6137
Summary by CodeRabbit
New Features
Localization
Bug Fixes / UX
Tests