From f476da90f2005826f82fec8481df75069f51a7ab Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:59:09 +0200 Subject: [PATCH 1/7] feat(media): let the multi-selection reach the actions that imply it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../media/multiSelectActions.test.tsx | 85 +++++++++++++++++++ .../BulkEditWindow/BulkEditWindow.tsx | 80 +++++++++++++++++ .../components/MediaCanvas/MediaCanvas.tsx | 76 ++++++++++++----- .../MediaViewerWindow/MediaViewerWindow.tsx | 9 +- .../shared/FloatingWindow/FloatingWindow.tsx | 3 + src/admin/shared/FloatingWindow/index.ts | 1 + .../shared/FloatingWindow/useTopmostEscape.ts | 56 ++++++++++++ 7 files changed, 287 insertions(+), 23 deletions(-) create mode 100644 src/__tests__/media/multiSelectActions.test.tsx create mode 100644 src/admin/shared/FloatingWindow/useTopmostEscape.ts 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/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx b/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx index 496894e19..f549e8b93 100644 --- a/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx +++ b/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx @@ -11,6 +11,7 @@ */ import { useState } from 'react' import { Button } from '@ui/components/Button' +import { Dialog } from '@ui/components/Dialog' import { Input, Textarea } from '@ui/components/Input' import { canDeleteMedia, canWriteMedia } from '@admin/access' import { useCurrentAdminUser } from '@admin/sessionContext' @@ -143,10 +144,39 @@ async function runRestoreAll( } } +/** + * Permanent deletion for a whole selection. + * + * Same per-asset loop as its Trash / Restore siblings — `purgeAsset` is a + * single-id endpoint like the other two, so no server work is needed. The + * trash view offered Restore but no counterpart, which left emptying the + * trash a one-file-at-a-time job through the preview window. + */ +async function runPurgeAll( + assets: UseMediaWorkspaceResult['selectedAssets'], + workspace: UseMediaWorkspaceResult, + setBusy: (v: boolean) => void, + setProgress: (v: { done: number; total: number } | null) => void, +): Promise { + const count = assets.length + try { + let done = 0 + for (const asset of assets) { + await workspace.purgeAsset(asset.id) + done += 1 + setProgress({ done, total: count }) + } + } finally { + setBusy(false) + setTimeout(() => setProgress(null), 800) + } +} + export function BulkEditWindow({ workspace, open, onClose }: BulkEditWindowProps) { const currentUser = useCurrentAdminUser() const [plan, setPlan] = useState(EMPTY_PLAN) const [busy, setBusy] = useState(false) + const [purgeConfirmOpen, setPurgeConfirmOpen] = useState(false) const [progress, setProgress] = useState<{ done: number; total: number } | null>(null) const assets = workspace.selectedAssets @@ -180,7 +210,18 @@ export function BulkEditWindow({ workspace, open, onClose }: BulkEditWindowProps await runRestoreAll(assets, workspace, setBusy, setProgress) } + async function purgeAll() { + if (!canDelete || busy) return + setBusy(true) + setProgress({ done: 0, total: count }) + await runPurgeAll(assets, workspace, setBusy, setProgress) + } + const anyTrashed = assets.some((a) => a.deletedAt !== null) + // Only the trashed members are purgeable — `purgeAsset` 400s on an asset + // that has not been soft-deleted — so the confirmation counts those, not + // the whole selection. + const trashedCount = assets.filter((a) => a.deletedAt !== null).length const anyActive = assets.some((a) => a.deletedAt === null) return ( @@ -322,9 +363,48 @@ export function BulkEditWindow({ workspace, open, onClose }: BulkEditWindowProps Restore )} + {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..84810125a 100644 --- a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx +++ b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx @@ -196,6 +196,28 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv writeMediaAssetDragData(event.dataTransfer, dragIds) } + /** + * 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() @@ -517,27 +539,39 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv )} - {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 diff --git a/src/admin/shared/FloatingWindow/FloatingWindow.tsx b/src/admin/shared/FloatingWindow/FloatingWindow.tsx index 1c7b368fa..dd02aaaea 100644 --- a/src/admin/shared/FloatingWindow/FloatingWindow.tsx +++ b/src/admin/shared/FloatingWindow/FloatingWindow.tsx @@ -4,6 +4,7 @@ import { PanelHeader } from '@admin/shared/PanelHeader' import type { FloatingPanelId, PanelPosition } from '@admin/state/workspaceLayoutStorage' import { cn } from '@ui/cn' import { useDraggablePanel } from './useDraggablePanel' +import { useTopmostEscape } from './useTopmostEscape' import styles from './FloatingWindow.module.css' interface FloatingWindowProps { @@ -52,6 +53,8 @@ export function FloatingWindow({ ) useImperativeHandle(forwardedRef, () => panelRef.current as HTMLDivElement) + useTopmostEscape(open, panelRef, onClose) + if (!open) return null const style = { 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..350b6ff3a --- /dev/null +++ b/src/admin/shared/FloatingWindow/useTopmostEscape.ts @@ -0,0 +1,56 @@ +/** + * 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. + */ + +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 + + 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]) +} From 93308862e6baf24171a7a99e2f80572e56c03051 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:41:33 +0200 Subject: [PATCH 2/7] fix(media): let an open menu keep Escape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/admin/shared/FloatingWindow/useTopmostEscape.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/admin/shared/FloatingWindow/useTopmostEscape.ts b/src/admin/shared/FloatingWindow/useTopmostEscape.ts index 350b6ff3a..1c58e95c5 100644 --- a/src/admin/shared/FloatingWindow/useTopmostEscape.ts +++ b/src/admin/shared/FloatingWindow/useTopmostEscape.ts @@ -14,6 +14,11 @@ * `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' @@ -36,6 +41,11 @@ export function useTopmostEscape( 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( From d8c2cdc1b3bda3723ab0e8ae3733fc06a04d2a8a Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:46:32 +0200 Subject: [PATCH 3/7] feat(media): select every visible asset with Ctrl/Cmd+A or a toolbar button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/__tests__/media/selectAll.test.tsx | 107 ++++++++++++++++++ .../components/MediaCanvas/MediaCanvas.tsx | 57 ++++++++++ 2 files changed, 164 insertions(+) create mode 100644 src/__tests__/media/selectAll.test.tsx diff --git a/src/__tests__/media/selectAll.test.tsx b/src/__tests__/media/selectAll.test.tsx new file mode 100644 index 000000000..3ddda86eb --- /dev/null +++ b/src/__tests__/media/selectAll.test.tsx @@ -0,0 +1,107 @@ +/** + * 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 + if (document.querySelector('[role="dialog"], [role="alertdialog"]')) 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 any 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') + document.body.append(grid, dialog) + expect(claimsSelectAll(grid)).toBe(false) + }) + + it('stands down for an alertdialog too', () => { + // `Dialog` renders `alertdialog` when `tone === 'danger'`, which is + // exactly what the permanent-delete confirmation is. + const grid = document.createElement('div') + const alert = document.createElement('div') + alert.setAttribute('role', 'alertdialog') + document.body.append(grid, alert) + expect(claimsSelectAll(grid)).toBe(false) + }) +}) + +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/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx index 84810125a..9d474acd9 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,46 @@ 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. + */ + function selectAllVisible() { + if (workspace.visibleAssets.length === 0) 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(() => selectAllVisible()) + 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 + if (document.querySelector('[role="dialog"], [role="alertdialog"]')) return + event.preventDefault() + selectAllEvent() + } + document.addEventListener('keydown', onKeyDown) + return () => document.removeEventListener('keydown', onKeyDown) + }, []) + /** * Which assets a right-click acts on. * @@ -368,6 +410,21 @@ 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. */} + Date: Sat, 5 Sep 2026 19:38:56 +0200 Subject: [PATCH 4/7] fix(media): make Select all a toggle, and stop the viewer window disabling it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/__tests__/media/selectAll.test.tsx | 51 +++++++++++++++---- .../components/MediaCanvas/MediaCanvas.tsx | 38 +++++++++++--- 2 files changed, 73 insertions(+), 16 deletions(-) diff --git a/src/__tests__/media/selectAll.test.tsx b/src/__tests__/media/selectAll.test.tsx index 3ddda86eb..daf247335 100644 --- a/src/__tests__/media/selectAll.test.tsx +++ b/src/__tests__/media/selectAll.test.tsx @@ -29,7 +29,10 @@ function claimsSelectAll(target: EventTarget | null): boolean { || target instanceof HTMLTextAreaElement || (target instanceof HTMLElement && target.isContentEditable) ) return false - if (document.querySelector('[role="dialog"], [role="alertdialog"]')) 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 } @@ -69,24 +72,54 @@ describe('when Ctrl/Cmd+A selects every visible asset', () => { expect(claimsSelectAll(editable)).toBe(false) }) - it('stands down while any dialog is open', () => { + 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('stands down for an alertdialog too', () => { - // `Dialog` renders `alertdialog` when `tone === 'danger'`, which is - // exactly what the permanent-delete confirmation is. + 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 alert = document.createElement('div') - alert.setAttribute('role', 'alertdialog') - document.body.append(grid, alert) - expect(claimsSelectAll(grid)).toBe(false) + 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) }) }) diff --git a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx index 9d474acd9..b1e6b7c55 100644 --- a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx +++ b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx @@ -207,8 +207,17 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv * that makes the trash view's "select all, then delete" safe: it cannot * reach past the filter into live assets. */ - function selectAllVisible() { + 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)) } @@ -219,7 +228,7 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv // 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(() => selectAllVisible()) + const selectAllEvent = useEffectEvent(() => toggleSelectAllVisible()) useEffect(() => { function onKeyDown(event: globalThis.KeyboardEvent) { if (event.key !== 'a' && event.key !== 'A') return @@ -230,7 +239,13 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv || target instanceof HTMLTextAreaElement || (target instanceof HTMLElement && target.isContentEditable) ) return - if (document.querySelector('[role="dialog"], [role="alertdialog"]')) 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() } @@ -417,13 +432,22 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv Date: Sat, 5 Sep 2026 19:44:58 +0200 Subject: [PATCH 5/7] feat(media): minimize a floating window, and let the upload queue be dismissed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../media/uploadQueueWindow.test.tsx | 72 +++++++++++++++++++ src/admin/pages/media/MediaPage.tsx | 31 ++++++-- .../BulkEditWindow/BulkEditWindow.tsx | 1 + .../UploadQueueWindow/UploadQueueWindow.tsx | 7 +- .../shared/FloatingWindow/FloatingWindow.tsx | 42 +++++++++-- 5 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 src/__tests__/media/uploadQueueWindow.test.tsx 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 = ( + )} -
{children}
+ {!minimized &&
{children}
} , document.body, ) From 6c3d625872864324762630ff27b069e5bf0ef509 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 20:00:54 +0200 Subject: [PATCH 6/7] fix(media, ui): minimize the image viewer too, and size dialog body copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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

: 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

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 --- .../MediaViewerWindow/MediaViewerWindow.tsx | 26 ++++++++++++++++--- src/ui/components/Dialog/Dialog.module.css | 9 +++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx index d23eab7a5..94506c974 100644 --- a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx +++ b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx @@ -33,6 +33,8 @@ import { useCurrentAdminUser } from '@admin/sessionContext' import { Copy2SolidIcon } from 'pixel-art-icons/icons/copy-2-solid' import { ExternalLinkSolidIcon } from 'pixel-art-icons/icons/external-link-solid' import { ReloadIcon } from 'pixel-art-icons/icons/reload' +import { MinusIcon } from 'pixel-art-icons/icons/minus' +import { PlusIcon } from 'pixel-art-icons/icons/plus' import { TrashSolidIcon } from 'pixel-art-icons/icons/trash-solid' import { VideoSolidIcon } from 'pixel-art-icons/icons/video-solid' import { PanelHeader } from '@admin/shared/PanelHeader' @@ -101,6 +103,7 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { const currentUser = useCurrentAdminUser() const { asset } = editor const [replaceOpen, setReplaceOpen] = useState(false) + const [minimized, setMinimized] = useState(false) const bucket = bucketForMime(asset.mimeType) const canWrite = canWriteMedia(currentUser) const canReplace = canReplaceMedia(currentUser) @@ -175,14 +178,31 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { style={panelPositionStyle} onClick={(event) => event.stopPropagation()} > + {/* This window builds its own shell rather than `FloatingWindow`, so the + collapse control is wired here directly — same behaviour, same + per-session scope, same reason for staying put: the position is the + user's own and folding to a corner would discard it. */} + > + + -

+ {!minimized &&
@@ -343,7 +363,7 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { )} -
+
} {canReplace && ( 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 { From 699f42e8c3cbddfdf61ee6f639e473dc8ac51071 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 20:45:15 +0200 Subject: [PATCH 7/7] fix(media): collapse the image viewer's height, not just its body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../MediaViewerWindow/MediaViewerWindow.module.css | 9 +++++++++ .../components/MediaViewerWindow/MediaViewerWindow.tsx | 1 + 2 files changed, 10 insertions(+) diff --git a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.module.css b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.module.css index 0b054f14c..9985395e6 100644 --- a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.module.css +++ b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.module.css @@ -20,6 +20,15 @@ overflow: hidden; } +/* Collapsed: shrink to the header. The height above is a fixed length, so + hiding the body alone left the window at full size with an empty pane + under the title bar. `height: auto` sizes it to the header that remains, + while `top` above keeps clamping against the FULL height — which is what + stops a window collapsed near the bottom edge from jumping. */ +.window[data-minimized="true"] { + height: auto; +} + .body { flex: 1 1 auto; display: grid; diff --git a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx index 94506c974..5dc531860 100644 --- a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx +++ b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx @@ -172,6 +172,7 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) {