future(upload): asset actions menu on files - #27263
Conversation
Wire the "..." button on asset cards and rows in the new Media Library, which until now only stopped propagation. The menu mirrors FolderActionsMenu and follows the Figma node: replace / copy link / download, then move / delete behind a separator. It always acts on that one asset, whatever the current multi-selection is. Replace, copy link and download duplicate what the details drawer offers as icon buttons, so the same action is one click away without opening the asset first. They are re-implemented rather than lifted out of the drawer: those versions render as IconButtons and report through the drawer's own toast slot, neither of which fits a menu item on a row. The drawer's error unwrapping (`error.data.error.message`) is not copied over — fetchBaseQuery already unwraps the API envelope, so the server-sent reason lands directly on `error.message`. Asset card/row click and keydown handlers gain the same isEventFromWithin guard the folder ones already have: the menu mounts its dialogs as React children of the card, so their portaled events would otherwise open the details drawer or start a drag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The design system caps Menu.Content at 15rem with `overflow: auto` and hides the scrollbar (`scrollbar-width: none`, `::-webkit-scrollbar` display none). The asset menu is ~200px tall, so on a trigger near the bottom of the viewport Radix flipped it to `side="top"` and clamped max-height to the 150px available: "Delete" fell outside the container entirely and "Move to folder" was cut mid-item, with nothing on screen to suggest anything was hidden. Setting `maxHeight="fit-content"` lets the menu keep its natural height, so the collision flip — which already picks the side with room — is the only thing that repositions it. Applied to the folder menu too; it is short enough not to clip today, but shares the same cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two fixes to the asset actions menu found in review. The replace dialog promised regenerated AI metadata whenever the feature was enabled, but `admin-upload.replaceFile` only regenerates for images the provider can read — it hands the file to `aiMetadata.processFiles`, which filters on the `AI_METADATA_SUPPORTED_IMAGE_TYPES` allowlist. So replacing a PDF advertised metadata that never arrived, and a GIF cleared the controller's looser `image/*` gate only to be skipped downstream. Gate the paragraph on `isAIMetadataSupportedMime`, the same helper the bulk actions bar already uses for its disabled-reason tooltip. The trigger also rendered for roles holding none of update, copy-link or download, opening a popup with zero items. Return null for that combination, once the RBAC check has settled — every flag is false while it is in flight, so keying off it directly would unmount and remount the trigger on first paint for everyone. The separator condition was dead while we were in here: `showTopGroup` short-circuits on `canUpdate`, so `showTopGroup && canUpdate` reduced to `canUpdate` and put a separator above move/delete with only Replace over it. It now requires the top group to hold something other than Replace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mathildeleg
left a comment
There was a problem hiding this comment.
A suggestion and then two small issues to fix:
- if you open the Menu on one asset, then directly click on another asset's Menu, the first one doesn't close, which means when you want to close both you have to click twice away from the Menus to close both (in this video I do it on 3 assets: https://github.com/user-attachments/assets/c314c050-a0e8-4094-aebe-df01d9af82e4)
- I have the AI metadata sentence when wanting to replace media, even though I don't have AI enabled because I'm not on a Growth license: https://github.com/user-attachments/assets/0463ea86-4b5d-465f-9720-0f9f1eae670c (I have the same issue when wanting to replace media through the drawer so might not be related to your PR though)
Otherwise LGTM :)
| }; | ||
|
|
||
| const handleCopyLink = async () => { | ||
| const url = prefixFileUrlWithBackendUrl(asset.url); |
There was a problem hiding this comment.
why not use asset id (that you used for the bug fix you fixed earlier) here?
Because for example if I replace the media, I then have the URL of the old media name, instead of the new one, so if we use asset id here, the id remains the same anyways, so that would make more sense
wdyt?
There was a problem hiding this comment.
uhm, not sure I understood the question.
The link that gets copied here is the public url of the file, the same one served from the content-api. That url is stable across replacements by design, it doesn't change.
If the concern is around stale data replaceAsset invalidates both Asset:<id> and Asset:LIST (services/assets.ts:137-140), so the row refetches and asset.url updates.
There was a problem hiding this comment.
Sorry this wasn't clear! I meant the params in the url: http://localhost:1337/uploads/71_Lbmh_Cp_Wi_L_SY_522_13e6da390f.png
So 71_Lbmh_Cp_Wi_L_SY_522_13e6da390f.png is the name of my asset, if I replace or rename the media, the params remains the same even though my name's asset has technically changed.
But that makes sense with your comment, so ignore me haha :)
Open one row's actions menu while another is open and the first stays open: Radix's default modal dropdown marks the rest of the document aria-hidden and swallows pointer events, so the click on the sibling trigger only dismissed the current menu. Every menu you touched then needed a click of its own to close. Both row menus go non-modal, which makes that one click close this menu and open that one. The replace dialog also promised AI-generated metadata on licenses without AI: GET /upload/settings returns the stored aiMetadata toggle regardless of entitlement, and it defaults to true. Gate on EE AI availability as well, mirroring the server's own aiMetadata.isEnabled(). The same bug was in the details drawer, which additionally ignored the provider's mime allowlist, so both now share one useAIMetadataEnabled hook. Finally, rename list.asset.actions.* to list.assets.actions.* to match the plural list.assets.* keys that dominate the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AssetsPage and BulkActionsBar read AI availability through the legacy useAIAvailability, which fetches /upload/settings over react-query, while the rest of future/ fetches the same endpoint over RTK Query. AssetsPage called both, so one component held two caches and two requests for one endpoint. Point both at useAIMetadataEnabled. The gating is unchanged — the legacy hook already ANDed the EE licence check with the stored setting — but future/ now has a single AI hook, a single data layer, and no react-query left in the tree. Neither call site is scoped to one file, so both pass no mime; BulkActionsBar keeps its own per-asset allowlist filter for eligibleCount. Repoints the AssetsTable mock at the EE hook (mocking the legacy module would have silently stopped gating those assertions) and covers the setting half of the gate, which had no test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the review @mathildeleg! 🙏
Nice catch, should be fixed!
Fixed! I also replaced all the the places that used the legacy media lib hook in the the new media lib so we don't have a mix of react query and RTK query anymore. Ready for another review 😅 |
| return false; | ||
| } | ||
|
|
||
| return mime === undefined ? true : isAIMetadataSupportedMime(mime); |
There was a problem hiding this comment.
The "skip the mime gate" sentinel can be hit by accident.
mime === undefined means "don't check the mime". But File.mime is declared optional (mime?: string, shared/contracts/files.ts:82), so a caller that hands over an asset with no mime doesn't get the strict path — it gets the permissive one, and the replace dialog promises AI metadata again. That's the exact thing this hook was added to stop.
Not reachable today: the server marks mime as required: true (server/src/content-types/file.ts:65), so every real asset has one. It's the type signature that won't protect the next caller.
Could the two cases be made impossible to confuse? useAIMetadataEnabled({ mime }) for the gated call and a bare call for the ungated one, or two separate hooks — either way "I have no mime to give you" stops looking like "don't check the mime".
Generated by Claude, edited by me.
There was a problem hiding this comment.
Good catch — it takes an options object now, so an omitted argument skips the gate while { mime: undefined } still gates and fails closed.
| return; | ||
| } | ||
|
|
||
| const res = await replaceAsset({ id: asset.id, file }); |
There was a problem hiding this comment.
A replace started from the row menu gives no sign it's happening.
Between picking a file and the "File replaced." toast, nothing changes on screen. On a large file over a slow connection the row just sits there looking inert.
The drawer covers this with a busy overlay, and the string already exists — asset-details.replace.loading ("Replacing the file…"). disabled={isReplacing} on line 223 only helps someone who reopens the menu, which is closed by then.
Worth reusing the drawer's treatment here, or at minimum a pending toast.
Generated by Claude, edited by me.
There was a problem hiding this comment.
Good call — reused the drawer's overlay rather than a toast: extracted it as BusyOverlay, and the row/card show it via a per-asset busy context, so it clears exactly when the mutation settles.
| open | ||
| onClose={() => setIsMoveOpen(false)} | ||
| items={moveItems} | ||
| onSuccess={clear} |
There was a problem hiding this comment.
Acting on one asset clears the entire selection.
onSuccess={clear} here, and on the delete dialog at line 331, wipes everything. Select ten assets, then delete an eleventh from its own row menu, and those ten drop out of the selection too — even though the menu never touched them.
The reason given in the comment above — a stale asset:<id> key for a file that no longer exists — only calls for dropping that one key. isSelected(key) && toggle(key) would be enough.
Flagging rather than requesting a change: FolderActionsMenu does the same thing, so this is a shared pattern and probably worth settling once for both.
Generated by Claude, edited by me.
There was a problem hiding this comment.
Agreed, and agreed it's worth settling once for both menus — tracked as CMS-1597 rather than fixed here.
| popoverPlacement="bottom-end" | ||
| zIndex={2} | ||
| minWidth="22rem" | ||
| maxHeight="fit-content" |
There was a problem hiding this comment.
fit-content fixes the clipping but takes the scroll fallback with it.
The design system pairs its 15rem cap with overflow: auto. Overriding the cap with fit-content means the menu can no longer scroll at all — it just grows.
Fine at five items in a normal window. Less fine at 200% browser zoom or in a short window, where the menu can end up taller than the viewport with no way to reach the bottom of it. That's WCAG 1.4.4 / 1.4.10 territory, and it's the same class of problem the override was fixing.
Capping to Radix's available-height custom property and un-hiding the scrollbar would solve the original clipping without giving up the fallback. Same applies to the matching change in FolderActionsMenu.tsx:89.
Generated by Claude, edited by me.
There was a problem hiding this comment.
Fixed — capped to --radix-popper-available-height instead of fit-content and un-hid the scrollbar, shared as ActionsMenuContent so FolderActionsMenu gets it too.
… on an explicit mime
Two follow-ups from review.
`useAIMetadataEnabled` took a positional `mime`, and treated `undefined` as
"skip the mime gate". `File.mime` is optional on the contract, so
`useAIMetadataEnabled(asset.mime)` silently collapsed into the ungated,
permissive call whenever the mime was missing — the case the hook exists to
close. It takes an options object now: an omitted argument skips the gate,
`{ mime: undefined }` still gates and fails closed.
The menus capped `Menu.Content` at `fit-content` to escape the design system's
flat 15rem clamp, which was cutting Delete off under a last-row trigger. That
lifted the clamp but took the scroll fallback with it — the box grows to its
content, so the DS's `overflow: auto` can never fire, and in a short window or
at 200% zoom the menu runs past the viewport with no way to reach the bottom
(WCAG 1.4.4 / 1.4.10). Cap to `--radix-popper-available-height` instead, shared
via `ActionsMenuContent` so both menus stay in sync, and un-hide the scrollbar
the DS suppresses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up: a replace started from a row's "..." menu gave no sign it was
running. The menu is closed by the time the file picker returns, so the
`disabled={isReplacing}` guard on the menu item has nothing left to disable, and
the row sat inert for the whole upload.
Reuses the drawer's treatment rather than a toast. `DrawerBusyOverlay` was never
drawer-specific — it is `inset: 0` and a centred `Loader` — so it moves out as
`BusyOverlay`, with the z-index it needs to clear the in-drawer toast slot as a
defaulted prop.
The mutation stays in the menu; only the busy flag travels up, through a small
per-asset context. The menu sits in a leaf `<td>` / footer `Flex`, so an overlay
mounted there would cover the button and nothing else, and hoisting
`replaceAsset` into both `AssetRow` and `AssetCard` would duplicate it away from
the control that owns it. The flag is released in a `finally`, so the row clears
on failure too — no timeout to guess at.
The card gets the overlay on its header. The table row does not: a `<tr>` can't
reliably host `position: relative`, and the column count is responsive, so a
`colSpan` cell would have to track it. It swaps the thumbnail for a labelled
spinner and goes inert instead.
Also covers the drawer's busy overlay, which had none. `getBusyMessage` is
exported for it: the branch order only has an effect when two flags are true at
once, which the rendered drawer can't stage because each trigger disables itself
while its own mutation runs. Verified by mutation — reordering the chain, and
changing the overlay's z-index default, both fail the new tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mathildeleg
left a comment
There was a problem hiding this comment.
LGTM with latest changes, thanks for the quick fixes 💪🏻
I think you'll just need to fix the conflicts with the test file and then can be merged 🥳
…sset-actions-menu-button-on-files # Conflicts: # packages/core/upload/admin/src/future/pages/Assets/components/AssetDetails/tests/AssetDetailsDrawer.test.tsx
…stments Resolve AssetsPage/AssetsTable conflicts: adopt develop's infinite-scroll sentinel (#27230), file actions menu + busy-asset spinner (#27263) and BusyAssetsProvider, while keeping the design branch's sticky header, responsive toolbar/table, metadata warning and single-select filters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What does it do?
Wires up the "..." button on asset cards and rows in the new Media Library, which until now only stopped propagation.
AssetActionsMenu, the file mirror ofFolderActionsMenu. It always acts on the one asset it belongs to, whatever the current multi-selection is.BulkMoveDialogandDeleteItemsDialog, mounted only while open. Both clear the selection on success.assets.update; copy link and download on their own permissions. The trigger itself is hidden when a role holds none of the three, so it can never open an empty popup.isAIMetadataSupportedMime— the same allowlist the bulk actions bar uses for its disabled-reason tooltip.replaceFilegates onimage/*but then defers toaiMetadata.processFiles, which filters on that narrower list, so a PDF (and a GIF) would otherwise have been promised metadata that never arrives.useFileDraggablenow re-exposes itsdragData, the same wayuseFolderDraggableDroppablealready does, so the move dialog can validate against the asset's real folder.Two things beyond the ticket:
isEventFromWithinguard the folder ones already had. The menu mounts its dialogs as React children of the card, so without it their portaled events opened the details drawer or started a drag.maxHeight="fit-content". The design system capsMenu.Contentat 15rem and hides the scrollbar, so on a trigger near the bottom of the viewport the menu was clamped and "Delete" fell outside it entirely, with nothing on screen to suggest anything was hidden.The menu deliberately has no "Details" item — clicking the card or the filename already opens the drawer. "Create metadata" from the Figma is out of scope here.
Why is it needed?
The "..." button was dead on files in the new Media Library. It was noticed during QA of the folder tree work, and the folder equivalent already shipped in #27175, so files were the odd one out.
How to test it?
Environment:
examples/getstartedwithUNSTABLE_MEDIA_LIBRARY=truein.env, thenyarn develop. Go to the Media Library at/admin/plugins/unstable-uploadand upload a few images, plus one PDF and one GIF for steps 5 and 6.Manual test plan, in both Grid view and Table view:
/admin/settings/global/media-library); on CE the line never shows and steps 5–6 are not testable.image/*check but the AI provider skips it, so promising metadata there would be a lie.Permissions — create a role under Settings → Roles and toggle the Media Library asset permissions:
assets.update: only Copy link and Download appear, and there is no separator.assets.updatebut withoutcopy-linkanddownload: Replace, Move and Delete appear, and there is still no separator (Replace is the whole top group, so a divider above Move would fence it off rather than separate two groups).update,copy-linkanddownload: the "..." button is not rendered at all on any row or card. It must not appear and then vanish as permissions resolve.Automated tests, in addition to the above:
Related issue(s)/PR(s)
fix CMS-1499