Repository navigation
feat(media): select every visible asset with Ctrl/Cmd+A or a toolbar button - #501
mostafasadeghidev wants to merge 7 commits into
Conversation
Three reports from one install, all the same shape: an affordance that reads as if it already honours the selection, and does not. RIGHT-CLICK IGNORED THE SELECTION. `openContextMenu` stored only the clicked asset and the delete handler acted on `contextMenu.asset` alone, so right-clicking one of five selected files and choosing Delete trashed exactly one and left four selected — no error, nothing to notice. The fix is the rule the same component already uses forty lines above, in `handleAssetDragStart`: an item inside the selection acts on the whole selection, an item outside acts on itself. The menu now shows a "5 files" header and a "Trash 5 files" label so the scope is visible before the click, and hides Rename for a multi-selection because there is one name field. Deliberately NOT adopting the clicked asset into the selection the way the site explorer does: Media derives its floating windows from the selection during render — viewer at <= 1, bulk edit at >= 2 — so writing the selection from a menu opener would pop a window open underneath the menu. THE TRASH HAD NO BULK DELETE. A selection there offered Restore and nothing else, leaving "empty the trash" a one-file-at-a-time job through the preview window. `runPurgeAll` sits beside its Trash and Restore siblings and loops the same single-id endpoint they do — no server work needed. It confirms first, counting only the trashed members: `purgeAsset` 400s on a live asset, so a mixed selection would otherwise promise more than it does. ESCAPE DID NOT CLOSE THE WINDOWS. Every other overlay in the admin takes it. These windows overlay the grid they were opened from, and the only way out was the header's close button. `useTopmostEscape` carries the settings modal's rule — only the topmost layer reacts, so a confirmation opened from inside a window owns Escape until it closes rather than collapsing the stack in one press. `FloatingWindow` uses it, which covers bulk edit, the upload queue and the agent image preview; the media viewer renders its own shell, so it wires the hook directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI caught what the stacking check missed. `AgentImagePreview` is a `FloatingWindow` too, and its test opens a context menu inside the preview, presses Escape, and asserts the menu closes while the preview stays open. With the window listening, one press closed both. The check only looked for `dialog` and `alertdialog`, and only for elements that follow the panel in document order. A menu is neither: it carries `role="menu"` and portals to `document.body`, so it can render before the panel it belongs to. An open menu now owns Escape wherever it sits. That is the right rule regardless of the test — closing a window out from under the menu the user just opened loses their place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…button Media had no way to select more than a range by hand. Emptying the trash meant shift-clicking from the first row to the last, and there was no shortcut and no button — while the Data workspace has both a "Select all rows" header checkbox and a bulk action bar. "All" is `visibleAssets`: whatever the folder, filter, search and trash toggle have already narrowed to. That is what every file manager means, and it is the only reading that keeps "select all, then delete permanently" in the trash from reaching live assets. Two ways in, because they fail differently. Ctrl/Cmd+A is what people try first, but only if they already know it is there; the toolbar button is what tells them. It carries the count, so the scope is visible before the click. The shortcut is document-level — the grid is a plain div with no tabindex, so a React `onKeyDown` would only fire while focus happened to sit on a tile — and stands down in two cases: inside a text field, where Ctrl/Cmd+A means select-the-text, and while any dialog is open, where it belongs to whatever that dialog contains. Selecting the grid behind a confirmation would change what the confirmed action applies to. No new workspace API: `addToSelection` already existed and already keeps the selection order list in step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…bling it Two corrections to the commit before this one, both found by using it. THE SHORTCUT BARELY WORKED. The guard stood down whenever any `role="dialog"` was in the document — written for confirmation dialogs, but the media viewer window carries that role too, and selecting a single asset opens it. So Ctrl/Cmd+A was dead for almost the whole time anyone spends in Media. `role="dialog"` was the wrong test. What marks a surface as owning the keyboard is `aria-modal="true"`: `Dialog` sets it, the floating windows deliberately do not, because the grid stays usable behind them. The guard now matches that instead, which is both narrower and the thing it always meant. THE BUTTON ONLY WENT ONE WAY. Select All with no way back is half a control — the obvious second press should undo it. It now clears when the selection already covers every visible asset, reads "None" in that state, and reports `pressed` so it looks like the toggle it is. Ctrl/Cmd+A follows the same rule, so the two entry points stay one feature. "Already covers" is deliberately not an equality check: a selection made before narrowing the filter can hold ids that are no longer visible, and those should not stop the button offering to clear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dismissed THE UPLOAD WINDOW COULD NOT BE CLOSED MID-TRANSFER. The close button worked and an effect immediately undid it: the guard read `active && !uploadQueueOpen` with BOTH in its dependencies, so every close re-ran it with `open` now false and reopened the window on the next commit. While files were uploading the window simply would not go away. Keying it on the transition into `active` alone fixes it. Closing now hides the transfer rather than cancelling it, so the toolbar button carries the count — `Uploads 3/7` — which is what makes dismissing safe rather than lossy. A failed or cancelled item counts as finished, not in flight, or the count would stick mid-way forever. MINIMIZE, in the shared shell so all three windows get it from one place. Collapsing leaves the title bar and hides the body; the header's other actions go with it, since they act on content that is no longer visible. Two decisions worth stating: Collapsed windows stay WHERE THEY ARE. The position is the user's own — `useDraggablePanel` persists it — so folding to a corner would discard a choice they made, and expanding would then have nowhere honest to return to. With three windows able to collapse, a shared corner would also stack them. The close button still closes. Turning it into "minimize while busy" would make a control that does something other than what it says, which is the same defect as a keycap that animates and ignores the click. What DOES move is the upload window's DEFAULT position: bottom-left, where a browser puts its download shelf and Finder its copy progress. Default only — a stored position still wins, so a window the user has moved stays moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TWO GAPS FOUND BY LOOKING AT THE RESULT. The image viewer window had no minimize control. `FloatingWindow` grew one, which covered the upload queue, bulk edit and the agent preview — but this window builds its own shell and only borrows the drag hook, so it got nothing. Same shape as the Escape gap earlier, and the same fix: wire the control directly, with the same per-session scope and the same reason for staying put. Dialog body copy rendered at the browser's 16px default while the rest of the admin runs at 12-14px. `.body` set no `font-size`, and neither does anything above it — `globals.css` contains no `font-size` rule at all, so there is no base to inherit. Every existing caller had quietly worked around it by sizing its own children (ImportHtmlModal has five such rules, SchedulePublishDialog one), which is why it went unnoticed until a dialog shipped a bare <p>: the delete confirmation's text sat visibly larger than the panel behind it. Fixed in the primitive rather than in the caller, since the next bare <p> would land in the same hole. `--text-m` and `--text-muted` match what sibling surfaces use for secondary copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The minimize control hid the body and left the window at full size, so collapsing it produced a title bar over a tall empty pane rather than a title bar alone. `FloatingWindow` did not have this problem because its height comes from a custom property the component already overrides when collapsed. This window sets `height` to a fixed length in its own stylesheet, which no amount of hiding children can shrink. `height: auto` under `[data-minimized]` lets it size to the header that remains. `top` keeps clamping against the FULL height on purpose — using the collapsed height there would let a window pinned near the bottom edge jump upward as it folds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…at had no gate The fork gate stopped at CoreBunch#359. Everything merged onto the stack since — CoreBunch#497 through CoreBunch#501, and now CoreBunch#505 — was carried with nothing to catch it being dropped by a merge resolution, which is the exact failure the gate exists for. Pinning them turned one up immediately: CoreBunch#498's guard around `removePluginVersionAssets(…, fromVersion)` is gone, because CoreBunch#359 removed that call outright — an upgrade now leaves the old version's assets on disk so published pages keep resolving. What survives of CoreBunch#498 is the rollback guard, so that is what the row pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The conflicts here are history rather than content: the stack took CoreBunch#507's warning through its own commits, combined with CoreBunch#500 and CoreBunch#501, so both sides look new to git. Each file was settled by what CoreBunch#507's commits since then actually changed: - MediaCanvas, MediaViewerWindow, the confirmation test, mediaPrefetch — untouched by those commits, so the stack's copies already hold all of CoreBunch#507 plus the multi-select and minimize work. Kept. - contentUsage.ts and its test — the branch-aware lookup is new, so CoreBunch#507's version is taken, with the stack's four-argument walker (`rootNodeId` scopes hole and loop fragments here). - usageWarning copy + tests, the ref schema, docs — merged cleanly. Before choosing a side, the stack's copies of both test files were checked against CoreBunch#507's pre-sync versions: identical, so nothing stack-only is lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for this, select-all in the media grid is something we want. I am closing the PR in this form though, because it carries much more than the title says. Besides select-all, the diff includes the upload-queue reopen fix, a new Could you resubmit these as separate PRs on main? Select-all alone, the upload-queue reopen fix, minimize, and the Dialog typography if you still think it's needed. I will get to each of them quickly. Btw. #335 is merged now, thanks again for that one. Have a good one! |
…ents Review on CoreBunch#501 named two problems in this PR, both real: - The trash view's context menu purged a whole selection with no confirmation. A purge from the menu now always asks first, through the admin's one ConfirmDeleteProvider with `alwaysConfirm` — the `confirmBeforeDelete` preference defaults off and governs reversible trashing, not this. A single file asks too, worded as CoreBunch#497 words it, so whichever of the two lands second resolves to the same code. - The tests asserted on local copies of the logic. They now render MediaCanvas and BulkEditWindow inside the real session and confirm providers and drive the menu, the drag and the dialog. Each was checked against a deliberately broken component: no confirmation, a menu or a drag that ignores the selection, and a bulk purge of the whole selection — every one turns the suite red. Writing those tests found a bug the copies hid: the bulk window's dialog counted only the trashed members of a mixed selection, but the purge loop ran over all of them, firing a `purgeAsset` for every live file — an endpoint that 400s on an asset that was never soft-deleted. The purge now runs on exactly the files the dialog counts. The selection rule itself moves to `utils/actionTargets.ts`, and both the drag and the menu call it, so they cannot drift apart; the test that compared two copies of it is gone with them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CoreBunch#659 is the upload-queue fix split out of CoreBunch#501, rewritten per review: `useUploadQueueWindow(active)` opens the window on the transition into an active upload, detected during render, instead of the effect the stack carried from CoreBunch#501. Same behaviour, no set-state-in-effect disable, and a test that drives the real hook. The stack's progress label on the toolbar button (CoreBunch#501, not part of CoreBunch#659) stays, with its tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hat render the canvas The stack already had select-all, extracted into `useMediaSelection`. The merge also dropped a second copy of it into MediaCanvas — a second document keydown listener would have selected and then cleared on one Ctrl/Cmd+A — so that copy is removed and MediaCanvas is unchanged by this merge. CoreBunch#658's tests, which render MediaCanvas and press the real shortcut, replace CoreBunch#501's tests of local copies, and pass against the hook unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tests The stack already carried CoreBunch#501's minimize code, identical to CoreBunch#660's; the two conflicts were lines CoreBunch#660 does not have (CoreBunch#500's useTopmostEscape, the viewer's purge-confirmation state from CoreBunch#497/CoreBunch#507), kept as they were, and the merge's second `minimizable` attribute on two windows is dropped. What this adds is CoreBunch#660's docs and its tests, which render the real windows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ch#501 is now three PRs CoreBunch#335 and CoreBunch#499 move to the landed table (shipped in v0.0.21). CoreBunch#501 was closed in review and asked to come back split: its rows become CoreBunch#658 (select-all), CoreBunch#659 (upload queue stays closed) and CoreBunch#660 (minimize). The three parts of CoreBunch#501 that are in none of them — the Uploads progress label, the queue's bottom-left default, and the Dialog body size — are listed as carried-but-not-proposed, so the README still says what a server runs. Also: what an installation needs before moving to plain upstream, since upstream's 031 repeats an ALTER this branch ran as 025. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks David, that's fair. And thanks for merging #335! I've split it up and opened each part on its own against main: #658 for select-all None of them depend on #500 or on each other anymore, and I rewrote the tests so they render the actual components instead of testing copies of the logic. I left out the Dialog typography change and the new default position for the upload queue. If you think the dialog font size is worth looking at, I can open that one separately so we can talk about it there. |
Media had no way to select more than a range by hand. Emptying the trash meant shift-clicking from the first row to the last — there was no shortcut and no button, while the Data workspace has both a "Select all rows" header checkbox (
DataGridHeaderRow.tsx) and a bulk action bar.Stacked on #500, which is what gives the resulting selection somewhere to go — a context menu that honours it and a bulk permanent-delete.
What "all" means
visibleAssets— whatever the folder, filter, search and trash toggle have already narrowed to. That is what every file manager means by Select All, and it is the only reading that keeps "select all, then delete permanently" in the trash from reaching live assets.Two ways in, because they fail differently
Ctrl/Cmd+A is what people try first, but only if they already know it is there. The toolbar button is what tells them, and it carries the count so the scope is visible before the click.
Where the shortcut stands down
Document-level, because the grid is a plain div with no tabindex — a React
onKeyDownwould only fire while focus happened to sit on a tile. It yields in two cases:Seven tests cover exactly those boundaries, including
alertdialog, which is whatDialogrenders for the permanent-delete confirmation.No new workspace API —
addToSelectionalready existed and already keeps the selection-order list in step.Co-Authored-By: Claude Opus 5 noreply@anthropic.com