diff --git a/src/__tests__/media/multiSelectActions.test.tsx b/src/__tests__/media/multiSelectActions.test.tsx new file mode 100644 index 000000000..025ba349b --- /dev/null +++ b/src/__tests__/media/multiSelectActions.test.tsx @@ -0,0 +1,85 @@ +/** + * The Media workspace's multi-selection reaching the actions that read as if + * they already honour it. + * + * Three behaviours, all reported from a real install: + * + * 1. Right-clicking one of five selected files and choosing Delete trashed + * exactly one. The menu never consulted the selection — while the same + * component's drag path already used the Finder rule. + * 2. The trash offered Restore for a selection but no permanent delete, so + * emptying it was a one-file-at-a-time job. + * 3. Escape did not close the floating windows, though every other overlay + * in the admin takes it. + * + * These cover the selection rule itself. It is pure and the interesting + * cases are the boundaries: an item inside the selection acts on all of it, + * an item outside acts on itself alone, and an empty selection never widens + * a single right-click into nothing. + */ + +import { describe, expect, it } from 'bun:test' + +/** + * The rule as `contextMenuTargets` implements it, and as + * `handleAssetDragStart` has implemented it all along. + */ +function targetsFor(clickedId: string, selected: string[]): string[] { + const selectedIds = new Set(selected) + return selectedIds.has(clickedId) && selected.length > 0 ? [...selected] : [clickedId] +} + +describe('which assets a Media right-click acts on', () => { + it('acts on the whole selection when the clicked file is part of it', () => { + const five = ['a', 'b', 'c', 'd', 'e'] + expect(targetsFor('c', five)).toEqual(five) + }) + + it('acts on the clicked file alone when it sits outside the selection', () => { + // Right-clicking away from a selection is how every file manager starts a + // new, unrelated action — it must not sweep the old selection in. + expect(targetsFor('z', ['a', 'b'])).toEqual(['z']) + }) + + it('acts on the clicked file when nothing is selected', () => { + expect(targetsFor('a', [])).toEqual(['a']) + }) + + it('is the same rule the drag path uses', () => { + // `handleAssetDragStart` resolves its ids identically. If these ever + // diverge, dragging and right-clicking the same file would act on + // different sets, which is the state this change removed. + const dragIds = (clicked: string, selected: string[]) => { + const ids = [...selected] + return new Set(selected).has(clicked) && ids.length > 0 ? ids : [clicked] + } + for (const [clicked, selected] of [ + ['c', ['a', 'b', 'c']], + ['z', ['a', 'b']], + ['a', []], + ] as const) { + expect(targetsFor(clicked, [...selected])).toEqual(dragIds(clicked, [...selected])) + } + }) +}) + +describe('which assets a bulk purge counts', () => { + /** `trashedCount` — only soft-deleted rows are purgeable. */ + const purgeable = (assets: { deletedAt: string | null }[]) => + assets.filter((a) => a.deletedAt !== null).length + + it('counts only the trashed members of a mixed selection', () => { + // `purgeAsset` 400s on an asset that was never soft-deleted, so a + // confirmation promising to delete the whole selection would overstate + // what is about to happen. + expect(purgeable([ + { deletedAt: '2026-01-01T00:00:00.000Z' }, + { deletedAt: null }, + { deletedAt: '2026-01-02T00:00:00.000Z' }, + ])).toBe(2) + }) + + it('counts nothing when the selection is entirely live', () => { + expect(purgeable([{ deletedAt: null }, { deletedAt: null }])).toBe(0) + }) +}) diff --git a/src/__tests__/media/selectAll.test.tsx b/src/__tests__/media/selectAll.test.tsx new file mode 100644 index 000000000..daf247335 --- /dev/null +++ b/src/__tests__/media/selectAll.test.tsx @@ -0,0 +1,140 @@ +/** + * Select All in the Media workspace. + * + * Two questions decide whether this is safe, and both are about scope rather + * than mechanics: + * + * 1. What does "all" mean? `visibleAssets` — whatever the folder, filter, + * search and trash toggle have already narrowed to. Anything wider would + * let "select all, then delete" in the trash reach live assets. + * 2. When must the shortcut stay out of the way? While the caret is in a + * text field, where Ctrl/Cmd+A means select-the-text, and while a dialog + * is open, where it belongs to whatever that dialog contains. + * + * The guard predicate is pure, so it is tested directly rather than through a + * mounted grid — the interesting cases are inputs, textareas, contenteditable + * and an open dialog, and a render harness would obscure rather than clarify + * them. + */ + +import { afterEach, describe, expect, it } from 'bun:test' + +/** + * `true` when the Ctrl/Cmd+A handler should claim the event, mirroring the + * guards in `MediaCanvas`. + */ +function claimsSelectAll(target: EventTarget | null): boolean { + if ( + target instanceof HTMLInputElement + || target instanceof HTMLTextAreaElement + || (target instanceof HTMLElement && target.isContentEditable) + ) return false + // Only a real modal — `aria-modal="true"` — takes the shortcut. The + // floating windows carry `role="dialog"` but leave the grid usable behind + // them, so matching the role would disable the shortcut almost everywhere. + if (document.querySelector('[aria-modal="true"]')) return false + return true +} + +afterEach(() => { + document.body.innerHTML = '' +}) + +describe('when Ctrl/Cmd+A selects every visible asset', () => { + it('claims the shortcut over the grid', () => { + const grid = document.createElement('div') + document.body.append(grid) + expect(claimsSelectAll(grid)).toBe(true) + }) + + it('leaves it to the browser inside the search box', () => { + // Someone typing a filter means "select what I typed", not "select every + // file in this folder". + const input = document.createElement('input') + document.body.append(input) + expect(claimsSelectAll(input)).toBe(false) + }) + + it('leaves it alone in a textarea', () => { + const textarea = document.createElement('textarea') + document.body.append(textarea) + expect(claimsSelectAll(textarea)).toBe(false) + }) + + it('leaves it alone in a contenteditable field', () => { + // The alt-text and caption fields in the viewer are rich inputs, and they + // are not elements. + const editable = document.createElement('div') + editable.setAttribute('contenteditable', 'true') + document.body.append(editable) + // jsdom does not derive isContentEditable from the attribute. + Object.defineProperty(editable, 'isContentEditable', { value: true }) + expect(claimsSelectAll(editable)).toBe(false) + }) + + it('stands down while a modal dialog is open', () => { + // A rename dialog or a delete confirmation owns the shortcut — selecting + // the grid behind it would change what a confirmed action applies to. + const grid = document.createElement('div') + const dialog = document.createElement('div') + dialog.setAttribute('role', 'dialog') + dialog.setAttribute('aria-modal', 'true') + document.body.append(grid, dialog) + expect(claimsSelectAll(grid)).toBe(false) + }) + + it('keeps working while a floating window is open', () => { + // The regression this guard caused: selecting one asset opens the viewer + // window, which carries `role="dialog"` but no `aria-modal` because the + // grid stays usable behind it. Matching the role disabled Ctrl/Cmd+A for + // almost the whole time anyone spends in Media. + const grid = document.createElement('div') + const viewer = document.createElement('aside') + viewer.setAttribute('role', 'dialog') + document.body.append(grid, viewer) + expect(claimsSelectAll(grid)).toBe(true) + }) +}) + +describe('the toggle', () => { + /** `allVisibleSelected` — the condition the button and shortcut both read. */ + const allSelected = (visible: string[], selected: string[]) => + visible.length > 0 && visible.every((id) => new Set(selected).has(id)) + + it('is not "all selected" when the grid is empty', () => { + // Otherwise `every` on an empty array reports true and the button would + // offer to clear a selection that does not exist. + expect(allSelected([], [])).toBe(false) + }) + + it('is not "all selected" when only some are', () => { + expect(allSelected(['a', 'b', 'c'], ['a', 'b'])).toBe(false) + }) + + it('is "all selected" once every visible asset is in the selection', () => { + expect(allSelected(['a', 'b'], ['a', 'b'])).toBe(true) + }) + + it('stays "all selected" when the selection reaches past the filter', () => { + // A selection made before narrowing the filter can hold ids that are no + // longer visible. The button asks about what is on screen, so those do + // not stop it offering to clear. + expect(allSelected(['a', 'b'], ['a', 'b', 'offscreen'])).toBe(true) + }) +}) + +describe('what "all" means', () => { + it('is the visible set, not the whole library', () => { + // `visibleAssets` is already narrowed by folder, filter, search and the + // trash toggle. Selecting past it would let "select all, then delete + // permanently" in the trash reach live assets. + const library = [ + { id: 'a', deletedAt: '2026-01-01T00:00:00.000Z' }, + { id: 'b', deletedAt: null }, + { id: 'c', deletedAt: '2026-01-02T00:00:00.000Z' }, + ] + const visibleInTrash = library.filter((a) => a.deletedAt !== null) + expect(visibleInTrash.map((a) => a.id)).toEqual(['a', 'c']) + expect(visibleInTrash).not.toContain(library[1]) + }) +}) diff --git a/src/__tests__/media/uploadQueueWindow.test.tsx b/src/__tests__/media/uploadQueueWindow.test.tsx new file mode 100644 index 000000000..54c7fec08 --- /dev/null +++ b/src/__tests__/media/uploadQueueWindow.test.tsx @@ -0,0 +1,72 @@ +/** + * Dismissing the upload queue window while files are still uploading. + * + * The close button worked; an effect immediately undid it. It depended on + * both `uploadQueue.active` AND `uploadQueueOpen`, so every close re-ran it + * with `open` now false and the guard `active && !open` re-opened the window + * on the next commit. While a transfer was in flight the window could not be + * made to stay shut. + * + * The rule these pin is the one the fix restores: an upload STARTING opens + * the window, and nothing reopens it after that. Closing hides the transfer + * rather than cancelling it, so the toolbar button carries the count instead. + */ + +import { describe, expect, it } from 'bun:test' + +/** + * The effect's guard in both shapes: the old one that read the open state, + * and the fixed one keyed only on the transition into `active`. + */ +const oldGuard = (active: boolean, open: boolean) => active && !open +const newGuard = (active: boolean) => active + +describe('what reopens the upload queue window', () => { + it('the old guard fires again the moment the user closes it', () => { + // Upload running, user clicks close → open flips false → the effect's + // dependencies changed → it runs → active && !open → reopened. + expect(oldGuard(true, false)).toBe(true) + }) + + it('the fixed guard does not re-run on close', () => { + // `active` has not changed, so the effect does not re-run at all. This + // test documents the dependency, not the boolean: the value is the same + // either way, and the bug was the extra dependency. + expect(newGuard(true)).toBe(true) + }) + + it('an upload starting still opens the window', () => { + expect(newGuard(false)).toBe(false) + expect(newGuard(true)).toBe(true) + }) +}) + +describe('the progress the toolbar button reports', () => { + type Item = { status: 'queued' | 'uploading' | 'succeeded' | 'failed' | 'cancelled' } + + const inFlight = (items: Item[]) => + items.filter((i) => i.status === 'queued' || i.status === 'uploading').length + + it('counts queued and uploading as still running', () => { + expect(inFlight([ + { status: 'queued' }, { status: 'uploading' }, { status: 'succeeded' }, + ])).toBe(2) + }) + + it('counts a failed upload as finished, not in flight', () => { + // A failure is done — it needs a retry, not a progress bar. Counting it + // as running would leave the button stuck mid-count forever. + expect(inFlight([{ status: 'failed' }, { status: 'cancelled' }])).toBe(0) + }) + + it('reports nothing to show when the queue is empty', () => { + expect(inFlight([])).toBe(0) + }) + + it('derives the done count from the total', () => { + const items: Item[] = [ + { status: 'succeeded' }, { status: 'succeeded' }, { status: 'uploading' }, + ] + expect(items.length - inFlight(items)).toBe(2) + }) +}) diff --git a/src/admin/pages/media/MediaPage.tsx b/src/admin/pages/media/MediaPage.tsx index f81e3aa29..65eb231e9 100644 --- a/src/admin/pages/media/MediaPage.tsx +++ b/src/admin/pages/media/MediaPage.tsx @@ -84,24 +84,43 @@ export function MediaPage() { // completes (the user dismisses it) and the toolbar button toggles it — so // it can't be derived. Auto-opening on the async upload transition is the // legitimate "sync UI to an external async system" use of an effect. + // + // Keyed on the TRANSITION into `active`, not on the flag plus the current + // open state. Depending on `uploadQueueOpen` re-ran this on every close and + // immediately re-opened the window, so while an upload was in flight the + // close button could not be made to stick. /* eslint-disable react-hooks/set-state-in-effect */ useEffect(() => { - if (workspace.uploadQueue.active && !uploadQueueOpen) { - setUploadQueueOpen(true) - } - }, [workspace.uploadQueue.active, uploadQueueOpen]) + if (workspace.uploadQueue.active) setUploadQueueOpen(true) + }, [workspace.uploadQueue.active]) /* eslint-enable react-hooks/set-state-in-effect */ + // Closing the queue window hides the transfer, it does not cancel it — so + // the button that reopens it carries the progress. Without this, dismissing + // the window during an upload left no sign anything was still running. + const uploadsInFlight = workspace.uploadQueue.items.filter( + (item) => item.status === 'queued' || item.status === 'uploading', + ).length + const uploadsTotal = workspace.uploadQueue.items.length + const toolbarRightSlot = ( )} + {canDelete && anyTrashed && ( + + )} )} + setPurgeConfirmOpen(false)} + tone="danger" + eyebrow="Cannot be undone" + title={`Delete ${trashedCount} ${trashedCount === 1 ? 'file' : 'files'} permanently?`} + footer={ + <> + + + + } + > +

+ This removes each file and every generated size from disk. Any page + still referencing one will render a broken image. +

+
) } diff --git a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx index 3dba08963..b1e6b7c55 100644 --- a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx +++ b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx @@ -6,6 +6,8 @@ * navigation land in M3/M4 — this component is the first interactive surface. */ import { + useEffect, + useEffectEvent, useState, type ChangeEvent, type DragEvent, @@ -196,6 +198,83 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv writeMediaAssetDragData(event.dataTransfer, dragIds) } + /** + * Select everything currently on screen. + * + * "Everything" is `visibleAssets` — what the active folder, filter, search + * and trash toggle have already narrowed to — not the whole library. That is + * what every file manager means by Select All, and it is the only reading + * that makes the trash view's "select all, then delete" safe: it cannot + * reach past the filter into live assets. + */ + const allVisibleSelected = + workspace.visibleAssets.length > 0 + && workspace.visibleAssets.every((asset) => workspace.selectedAssetIds.has(asset.id)) + + /** Select every visible asset, or clear the selection when it already covers them. */ + function toggleSelectAllVisible() { + if (workspace.visibleAssets.length === 0) return + if (allVisibleSelected) { + workspace.clearSelection() + return + } + workspace.addToSelection(workspace.visibleAssets.map((asset) => asset.id)) + } + + // Ctrl/Cmd+A, the shortcut people try first. Document-level because the grid + // is a plain div with no tabindex, so a React `onKeyDown` would only fire + // while focus happened to sit on a tile. + // + // Ignored while focus is in a text field — the browser's own select-all is + // what someone typing in the search box means — and while any dialog is + // open, so a rename or a delete confirmation keeps its own select-all. + const selectAllEvent = useEffectEvent(() => toggleSelectAllVisible()) + useEffect(() => { + function onKeyDown(event: globalThis.KeyboardEvent) { + if (event.key !== 'a' && event.key !== 'A') return + if (!(isMacLike() ? event.metaKey : event.ctrlKey)) return + const target = event.target + if ( + target instanceof HTMLInputElement + || target instanceof HTMLTextAreaElement + || (target instanceof HTMLElement && target.isContentEditable) + ) return + // Only a real MODAL takes the shortcut away. `aria-modal` is what marks + // one: `Dialog` sets it, the floating windows do not. Matching + // `role="dialog"` instead disabled the shortcut almost everywhere in + // Media, because selecting a single asset opens the viewer window — and + // that carries `role="dialog"` while deliberately leaving the grid + // usable behind it. + if (document.querySelector('[aria-modal="true"]')) return + event.preventDefault() + selectAllEvent() + } + document.addEventListener('keydown', onKeyDown) + return () => document.removeEventListener('keydown', onKeyDown) + }, []) + + /** + * Which assets a right-click acts on. + * + * The same Finder rule `handleAssetDragStart` already uses: acting on an + * item inside the selection acts on the whole selection; acting on one + * outside it acts on that item alone. The menu used to ignore the + * selection entirely, so right-clicking one of five selected files and + * choosing Delete trashed exactly one and left the other four selected. + * + * Deliberately does NOT adopt the clicked asset into the selection the way + * the site explorer does. Media's floating windows are derived from the + * selection during render — `viewerOpen` at <= 1, `bulkEditOpen` at >= 2 + * (MediaPage.tsx) — so writing the selection here would pop a window open + * underneath the menu. + */ + function contextMenuTargets(asset: CmsMediaAsset): string[] { + const selectedIds = Array.from(workspace.selectedAssetIds) + return workspace.selectedAssetIds.has(asset.id) && selectedIds.length > 0 + ? selectedIds + : [asset.id] + } + function handleFolderDragStart(folder: CmsMediaFolder, event: DragEvent) { if (!canWrite) { event.preventDefault() @@ -346,6 +425,30 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv groupLabel="Filter media type" trailing={(
+ {/* Discoverability for Ctrl/Cmd+A. The shortcut is the one people + reach for, but only if they already know it exists — and the + trash is where it matters most, because emptying it was + otherwise a file-at-a-time job. */} + - {contextMenu && ( - setContextMenu(null)} - onRename={() => { - setRenameTarget(contextMenu.asset) - setContextMenu(null) - }} - onDelete={() => { - const target = contextMenu.asset - setContextMenu(null) - if (trashView) void workspace.purgeAsset(target.id) - else void workspace.trashAsset(target.id) - }} - showRename={canWrite} - showDelete={canDelete} - extraItems={buildExtraMenuItems(contextMenu.asset)} - /> - )} + {contextMenu && (() => { + const targets = contextMenuTargets(contextMenu.asset) + const many = targets.length > 1 + return ( + setContextMenu(null)} + {...(many ? { headerLabel: `${targets.length} files` } : {})} + onRename={() => { + setRenameTarget(contextMenu.asset) + setContextMenu(null) + }} + onDelete={() => { + setContextMenu(null) + for (const id of targets) { + if (trashView) void workspace.purgeAsset(id) + else void workspace.trashAsset(id) + } + }} + deleteLabel={ + many + ? `${trashView ? 'Delete' : 'Trash'} ${targets.length} files` + : trashView ? 'Delete permanently' : 'Move to Trash' + } + // Renaming is a single-file operation — there is one name field. + showRename={canWrite && !many} + showDelete={canDelete} + extraItems={buildExtraMenuItems(contextMenu.asset)} + /> + ) + })()} {renameTarget && ( ({ x: window.innerWidth - 880, y: 80 }), ) + // This window renders its own shell rather than `FloatingWindow`, so it + // needs the same Escape rule wired up directly. The stacking check is what + // keeps the purge confirmation below owning Escape until it closes. + useTopmostEscape(true, panelRef, onClose) + // ── Save callbacks ──────────────────────────────────────────────────────── const saveTitle = async (next: string) => { if (!canWrite) return @@ -164,20 +172,38 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { -
+ } {canReplace && ( @@ -42,6 +58,7 @@ export function FloatingWindow({ bodyClassName, ariaLabel, testId, + minimizable = false, onClose, children, ref: forwardedRef, @@ -52,12 +69,16 @@ export function FloatingWindow({ ) useImperativeHandle(forwardedRef, () => panelRef.current as HTMLDivElement) + const [minimized, setMinimized] = useState(false) + + useTopmostEscape(open, panelRef, onClose) + if (!open) return null const style = { '--floating-window-w': cssLength(width), - '--floating-window-h': cssLength(height), - '--floating-window-max-h': cssLength(maxHeight), + '--floating-window-h': minimized ? 'auto' : cssLength(height), + '--floating-window-max-h': minimized ? 'none' : cssLength(maxHeight), ...panelPositionStyle, } as CSSProperties @@ -78,9 +99,23 @@ export function FloatingWindow({ onClose={onClose} dragHandleProps={headerDragProps} > - {headerActions} + {minimized ? null : headerActions} + {minimizable && ( + + )} -
{children}
+ {!minimized &&
{children}
} , document.body, ) diff --git a/src/admin/shared/FloatingWindow/index.ts b/src/admin/shared/FloatingWindow/index.ts index c69ae3b01..34dd0571d 100644 --- a/src/admin/shared/FloatingWindow/index.ts +++ b/src/admin/shared/FloatingWindow/index.ts @@ -8,3 +8,4 @@ export { clampFloatingPanelSize, useResizablePanel, } from './useResizablePanel' +export { useTopmostEscape } from './useTopmostEscape' diff --git a/src/admin/shared/FloatingWindow/useTopmostEscape.ts b/src/admin/shared/FloatingWindow/useTopmostEscape.ts new file mode 100644 index 000000000..1c58e95c5 --- /dev/null +++ b/src/admin/shared/FloatingWindow/useTopmostEscape.ts @@ -0,0 +1,66 @@ +/** + * Escape-to-close for a floating panel, honouring whatever is stacked above it. + * + * Every overlay in this admin takes Escape — the settings modal, the shared + * `Dialog`, Spotlight. The draggable windows did not: they overlay the grid + * they were opened from, and the only way out was the header's close button. + * + * The stacking check is the same one `SettingsModal` uses, and it is the + * reason this is a hook rather than three copies. A window can open a + * `Dialog` of its own — a delete confirmation, the replace-file picker — and + * that dialog must own Escape until it closes. Without the check, one press + * would collapse the confirmation and the window underneath it together. + * + * `alertdialog` counts as a layer: `Dialog` renders that role instead of + * `dialog` when `tone === 'danger'`, which is exactly what a destructive + * confirmation opened from one of these windows is. + * + * So does `menu`. A context menu opened inside a window owns Escape while it + * is up — closing the menu and the window together on one press loses the + * user's place. Menus portal to `document.body`, so they are not descendants + * of the panel and the document-order test alone would miss them. + */ + +import { useEffect, useEffectEvent, type RefObject } from 'react' + +export function useTopmostEscape( + open: boolean, + panelRef: RefObject, + onClose: () => void, +): void { + // `useEffectEvent` keeps `onClose` out of the dependency array — callers + // pass an inline arrow, and re-subscribing the listener on every render + // would drop keystrokes between removal and re-add. + const closeEvent = useEffectEvent(() => onClose()) + + useEffect(() => { + if (!open) return undefined + + function onKeyDown(event: globalThis.KeyboardEvent) { + if (event.key !== 'Escape') return + const self = panelRef.current + if (!self) return + + // An open menu owns Escape wherever it sits — it is a transient layer + // above everything, and unlike a dialog it may render before the panel + // in document order. + if (document.querySelector('[role="menu"]')) return + + const stackedAbove = Array.from( + document.querySelectorAll('[role="dialog"], [role="alertdialog"]'), + ).some( + (el) => + el !== self + && Boolean(self.compareDocumentPosition(el) & Node.DOCUMENT_POSITION_FOLLOWING), + ) + if (stackedAbove) return + + event.preventDefault() + event.stopPropagation() + closeEvent() + } + + document.addEventListener('keydown', onKeyDown) + return () => document.removeEventListener('keydown', onKeyDown) + }, [open, panelRef]) +} diff --git a/src/ui/components/Dialog/Dialog.module.css b/src/ui/components/Dialog/Dialog.module.css index 0bc1007a5..35d86e9f2 100644 --- a/src/ui/components/Dialog/Dialog.module.css +++ b/src/ui/components/Dialog/Dialog.module.css @@ -83,6 +83,15 @@ .body { padding: var(--space-2xl); overflow-y: auto; + /* The shell never set a size, and nothing above it does either — no rule + in globals.css sets `font-size` at all — so body copy fell through to + the browser's 16px default while the rest of the admin runs at 12-14px. + Every existing caller worked around it by sizing its own children; + a bare

in a dialog rendered noticeably larger than the panel + behind it. */ + font-size: var(--text-m); + line-height: 1.5; + color: var(--text-muted); } .footer {