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/2] 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/2] 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(