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/3] 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/3] 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 7c8e84d3ec92c2731860ba400c6c093b086557b3 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:15:18 +0200 Subject: [PATCH 3/3] fix(media): confirm a permanent delete from the menu, test the components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #501 named two problems in this PR, both real: - The trash view's context menu purged a whole selection with no confirmation. A purge from the menu now always asks first, through the admin's one ConfirmDeleteProvider with `alwaysConfirm` — the `confirmBeforeDelete` preference defaults off and governs reversible trashing, not this. A single file asks too, worded as #497 words it, so whichever of the two lands second resolves to the same code. - The tests asserted on local copies of the logic. They now render MediaCanvas and BulkEditWindow inside the real session and confirm providers and drive the menu, the drag and the dialog. Each was checked against a deliberately broken component: no confirmation, a menu or a drag that ignores the selection, and a bulk purge of the whole selection — every one turns the suite red. Writing those tests found a bug the copies hid: the bulk window's dialog counted only the trashed members of a mixed selection, but the purge loop ran over all of them, firing a `purgeAsset` for every live file — an endpoint that 400s on an asset that was never soft-deleted. The purge now runs on exactly the files the dialog counts. The selection rule itself moves to `utils/actionTargets.ts`, and both the drag and the menu call it, so they cannot drift apart; the test that compared two copies of it is gone with them. Co-Authored-By: Claude Opus 5.5 --- .../media/multiSelectActions.test.tsx | 328 +++++++++++++++--- .../BulkEditWindow/BulkEditWindow.tsx | 20 +- .../components/MediaCanvas/MediaCanvas.tsx | 67 ++-- src/admin/pages/media/utils/actionTargets.ts | 12 + 4 files changed, 333 insertions(+), 94 deletions(-) create mode 100644 src/admin/pages/media/utils/actionTargets.ts diff --git a/src/__tests__/media/multiSelectActions.test.tsx b/src/__tests__/media/multiSelectActions.test.tsx index 025ba349b..1adc5a84c 100644 --- a/src/__tests__/media/multiSelectActions.test.tsx +++ b/src/__tests__/media/multiSelectActions.test.tsx @@ -2,84 +2,304 @@ * 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: + * 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. + * Every case renders the real component and drives it the way an operator + * does — the menu, the drag, the confirmation — so the tests fail if the + * component stops using the rule, not just if the rule changes. */ -import { describe, expect, it } from 'bun:test' +import { afterEach, describe, expect, it } from 'bun:test' +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import type { ReactElement } from 'react' +import { BulkEditWindow } from '@admin/pages/media/components/BulkEditWindow/BulkEditWindow' +import { MediaCanvas } from '@admin/pages/media/components/MediaCanvas/MediaCanvas' +import { + FOLDER_ALL, + FOLDER_TRASH, + type UseMediaWorkspaceResult, +} from '@admin/pages/media/hooks/useMediaWorkspace' +import { actionTargets } from '@admin/pages/media/utils/actionTargets' +import { buildFolderTree } from '@admin/pages/media/utils/folderTree' +import { MEDIA_ASSET_DRAG_TYPE } from '@admin/pages/media/utils/mediaDragDrop' +import { AdminSessionProvider } from '@admin/session' +import { ConfirmDeleteProvider } from '@admin/shared/dialogs/ConfirmDeleteDialog' +import type { CoreCapability } from '@core/capabilities' +import type { CmsCurrentUser } from '@core/persistence' +import type { CmsMediaAsset } from '@core/persistence/cmsMedia' -/** - * The rule as `contextMenuTargets` implements it, and as - * `handleAssetDragStart` has implemented it all along. - */ -function targetsFor(clickedId: string, selected: string[]): string[] { +afterEach(cleanup) + +const TRASHED = '2026-02-01T00:00:00.000Z' + +function asset(id: string, deletedAt: string | null = null): CmsMediaAsset { + return { + id, + filename: `${id}.png`, + mimeType: 'image/png', + sizeBytes: 1200, + publicPath: `/uploads/${id}.png`, + uploadedByUserId: null, + createdAt: '2026-01-01T00:00:00.000Z', + altText: '', + caption: '', + title: '', + tags: [], + width: null, + height: null, + durationMs: null, + dominantColor: null, + deletedAt, + replacedAt: null, + folderIds: [], + blurHash: null, + variants: [], + posterPath: null, + } +} + +const CAPABILITIES: CoreCapability[] = ['media.read', 'media.write', 'media.replace', 'media.delete'] + +function currentUser(): CmsCurrentUser { + return { + id: 'user_media_manager', + email: 'media-manager@example.com', + displayName: 'Media Manager', + status: 'active', + role: { + id: 'role_media_manager', + slug: 'media-manager', + name: 'Media Manager', + description: 'Media test role', + isSystem: false, + capabilities: CAPABILITIES, + }, + capabilities: CAPABILITIES, + lastLoginAt: null, + failedLoginCount: 0, + lockedUntil: null, + passwordUpdatedAt: null, + mfaEnabled: false, + mfaEnabledAt: null, + mfaRecoveryCodesRemaining: 0, + stepUpAuthMode: 'password', + stepUpWindowMinutes: 15, + avatarMediaId: null, + avatarUrl: null, + gravatarHash: '', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', + } +} + +/** What an operator's clicks made the component do, in order. */ +interface Calls { + trashed: string[] + purged: string[] +} + +function workspace( + assets: CmsMediaAsset[], + selected: string[], + options: { trash?: boolean } = {}, +): { workspace: UseMediaWorkspaceResult; calls: Calls } { + const calls: Calls = { trashed: [], purged: [] } const selectedIds = new Set(selected) - return selectedIds.has(clickedId) && selected.length > 0 ? [...selected] : [clickedId] + const ws: UseMediaWorkspaceResult = { + loading: false, + error: null, + clearError: () => {}, + refresh: async () => {}, + folders: [], + folderTree: buildFolderTree([]), + folderById: new Map(), + assets, + visibleAssets: assets, + tagPalette: [], + folderSelection: options.trash ? FOLDER_TRASH : FOLDER_ALL, + setFolderSelection: () => {}, + selectedAssetId: null, + selectedAsset: null, + setSelectedAssetId: () => {}, + selectedAssetIds: selectedIds, + selectedAssets: assets.filter((a) => selectedIds.has(a.id)), + toggleAssetInSelection: () => {}, + addToSelection: () => {}, + selectRange: () => {}, + clearSelection: () => {}, + uploadQueue: { + items: [], + active: false, + enqueue: () => {}, + retry: () => {}, + remove: () => {}, + clearFinished: () => {}, + cancelAll: () => {}, + }, + filters: { type: 'all', q: '', tag: '', sort: 'newest' }, + setFilterType: () => {}, + setQuery: () => {}, + setTag: () => {}, + setSort: () => {}, + uploadFiles: async () => {}, + renameAsset: async () => null, + updateAsset: async () => null, + replaceAssetFile: async () => null, + trashAsset: async (id: string) => { + calls.trashed.push(id) + }, + restoreAsset: async () => null, + purgeAsset: async (id: string) => { + calls.purged.push(id) + }, + setAssetFolders: async () => null, + moveAssetsToFolder: async () => {}, + createFolder: async () => null, + renameFolder: async () => null, + moveFolder: async () => null, + deleteFolder: async () => {}, + } + return { workspace: ws, calls } } -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) +/** The tree the canvas really runs in: the session and the one confirm host. */ +function renderInAdmin(ui: ReactElement) { + return render( + + {ui} + , + ) +} + +function rightClick(filename: string) { + fireEvent.contextMenu(screen.getByRole('button', { name: `Open ${filename}` })) +} + +function chooseMenuItem(label: RegExp) { + fireEvent.click(screen.getByRole('menuitem', { name: label })) +} + +const FIVE = ['a', 'b', 'c', 'd', 'e'] + +describe('the Finder rule', () => { + it('acts on the whole selection when the clicked item is part of it', () => { + expect(actionTargets('c', new Set(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 item alone when it sits outside the selection', () => { + expect(actionTargets('z', new Set(['a', 'b']))).toEqual(['z']) }) - it('acts on the clicked file when nothing is selected', () => { - expect(targetsFor('a', [])).toEqual(['a']) + it('acts on the clicked item when nothing is selected', () => { + expect(actionTargets('a', new Set())).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('the Media context menu acts on the selection', () => { + it('trashes every selected file when one of them is right-clicked', () => { + const { workspace: ws, calls } = workspace(FIVE.map((id) => asset(id)), FIVE) + renderInAdmin() + + rightClick('c.png') + chooseMenuItem(/trash 5 files/i) + + expect(calls.trashed).toEqual(FIVE) + }) + + it('trashes only the right-clicked file when it is outside the selection', () => { + const { workspace: ws, calls } = workspace(['a', 'b', 'z'].map((id) => asset(id)), ['a', 'b']) + renderInAdmin() + + rightClick('z.png') + chooseMenuItem(/move to trash/i) + + expect(calls.trashed).toEqual(['z']) + }) + + it('drags the same set the menu acts on', () => { + const { workspace: ws } = workspace(FIVE.map((id) => asset(id)), FIVE) + renderInAdmin() + + const data = new Map() + const transfer = { + types: [] as string[], + effectAllowed: 'uninitialized', + setData: (type: string, value: string) => { + data.set(type, value) + }, + getData: (type: string) => data.get(type) ?? '', + setDragImage: () => {}, } + fireEvent.dragStart(screen.getByRole('button', { name: 'Open c.png' }), { dataTransfer: transfer }) + + expect(JSON.parse(data.get(MEDIA_ASSET_DRAG_TYPE) ?? '{}').assetIds).toEqual(FIVE) }) }) -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) +describe('a permanent delete from the trash menu asks first', () => { + function inTrash(selected: string[]): Calls { + const setup = workspace(FIVE.map((id) => asset(id, TRASHED)), selected, { trash: true }) + renderInAdmin() + return setup.calls + } + + it('does not purge the selection on the menu click', () => { + const calls = inTrash(['a', 'b', 'c']) + rightClick('b.png') + chooseMenuItem(/delete 3 files/i) + + expect(calls.purged).toEqual([]) + expect(screen.getByText(/delete 3 files permanently\?/i)).toBeTruthy() + }) + + it('purges every selected file once the operator confirms', () => { + const calls = inTrash(['a', 'b', 'c']) + rightClick('b.png') + chooseMenuItem(/delete 3 files/i) + fireEvent.click(screen.getByRole('button', { name: /^delete 3 files$/i })) + + expect(calls.purged).toEqual(['a', 'b', 'c']) + }) + + it('purges nothing when the operator cancels', () => { + const calls = inTrash(['a', 'b', 'c']) + rightClick('b.png') + chooseMenuItem(/delete 3 files/i) + fireEvent.click(screen.getByRole('button', { name: /cancel/i })) + + expect(calls.purged).toEqual([]) }) - it('counts nothing when the selection is entirely live', () => { - expect(purgeable([{ deletedAt: null }, { deletedAt: null }])).toBe(0) + it('asks for a single file too', () => { + const calls = inTrash([]) + rightClick('d.png') + chooseMenuItem(/delete permanently/i) + + expect(calls.purged).toEqual([]) + expect(screen.getByText(/delete "d\.png" permanently\?/i)).toBeTruthy() + }) +}) + +describe('the bulk window deletes exactly what it counts', () => { + it('counts and purges only the trashed members of a mixed selection', async () => { + const assets = [asset('t1', TRASHED), asset('live'), asset('t2', TRASHED)] + const { workspace: ws, calls } = workspace(assets, ['t1', 'live', 't2']) + renderInAdmin( {}} />) + + fireEvent.click(screen.getByRole('button', { name: /delete permanently/i })) + expect(screen.getByText(/delete 2 files permanently\?/i)).toBeTruthy() + + // The dialog's own confirm button renders after the window's. + fireEvent.click(screen.getAllByRole('button', { name: /delete permanently/i }).at(-1)!) + + // `purgeAsset` 400s on a live asset: the purge must skip it, not just + // the count. + await waitFor(() => expect(calls.purged).toEqual(['t1', 't2'])) }) }) diff --git a/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx b/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx index f549e8b93..48a121f50 100644 --- a/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx +++ b/src/admin/pages/media/components/BulkEditWindow/BulkEditWindow.tsx @@ -210,18 +210,20 @@ export function BulkEditWindow({ workspace, open, onClose }: BulkEditWindowProps await runRestoreAll(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. The confirmation counts these AND the + // purge runs on these, so a mixed selection deletes exactly what the + // dialog promised and never fires a request bound to fail. + const purgeable = assets.filter((a) => a.deletedAt !== null) + const trashedCount = purgeable.length + async function purgeAll() { - if (!canDelete || busy) return + if (!canDelete || busy || trashedCount === 0) return setBusy(true) - setProgress({ done: 0, total: count }) - await runPurgeAll(assets, workspace, setBusy, setProgress) + setProgress({ done: 0, total: trashedCount }) + await runPurgeAll(purgeable, 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 ( diff --git a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx index 84810125a..72da99532 100644 --- a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx +++ b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx @@ -19,6 +19,7 @@ import { FilterBar, type FilterBarItem } from '@ui/components/FilterBar' import { Select } from '@ui/components/Select' import { Skeleton } from '@ui/components/Skeleton' import { canDeleteMedia, canWriteMedia } from '@admin/access' +import { useConfirmDelete } from '@admin/shared/dialogs/ConfirmDeleteDialog' import { useCurrentAdminUser } from '@admin/sessionContext' import { ExplorerItemContextMenu, @@ -48,6 +49,7 @@ import { type UseMediaWorkspaceResult, } from '../../hooks/useMediaWorkspace' import { childFoldersForParent } from '../../utils/folderTree' +import { actionTargets } from '../../utils/actionTargets' import { writeMediaAssetDragData, writeMediaFolderDragData, @@ -122,6 +124,7 @@ function folderMatchesQuery(folder: CmsMediaFolder, query: string): boolean { export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanvasProps) { const currentUser = useCurrentAdminUser() + const confirmDelete = useConfirmDelete() const [viewMode, setViewModeState] = useState(readStoredMediaViewMode) const [contextMenu, setContextMenu] = useState(null) const [renameTarget, setRenameTarget] = useState(null) @@ -189,33 +192,7 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv event.preventDefault() return } - const selectedIds = Array.from(workspace.selectedAssetIds) - const dragIds = workspace.selectedAssetIds.has(asset.id) && selectedIds.length > 0 - ? selectedIds - : [asset.id] - 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] + writeMediaAssetDragData(event.dataTransfer, actionTargets(asset.id, workspace.selectedAssetIds)) } function handleFolderDragStart(folder: CmsMediaFolder, event: DragEvent) { @@ -540,7 +517,14 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv {contextMenu && (() => { - const targets = contextMenuTargets(contextMenu.asset) + // `actionTargets`, not the clicked asset: the menu used to ignore the + // selection, so right-clicking one of five selected files and choosing + // Delete trashed exactly one. It deliberately does NOT adopt the clicked + // asset into the selection the way the site explorer does — Media's + // floating windows derive from the selection during render (viewer at + // <= 1, bulk edit at >= 2, MediaPage.tsx), so writing it here would pop + // a window open underneath the menu. + const targets = actionTargets(contextMenu.asset.id, workspace.selectedAssetIds) const many = targets.length > 1 return ( { + const target = contextMenu.asset setContextMenu(null) - for (const id of targets) { - if (trashView) void workspace.purgeAsset(id) - else void workspace.trashAsset(id) + if (!trashView) { + for (const id of targets) void workspace.trashAsset(id) + return } + // Purging is the one media action nothing can undo: it removes + // the original and every generated size from the storage + // adapter, not just the row — and from here it can reach a + // whole selection at once. `alwaysConfirm` because the + // `confirmBeforeDelete` preference defaults off, and an operator + // who turned it off was opting out of confirming a TRASH, which + // is reversible. + confirmDelete({ + title: many + ? `Delete ${targets.length} files permanently?` + : `Delete "${target.filename}" permanently?`, + description: many + ? 'This removes every one of these files and all their generated sizes from disk. It cannot be undone.' + : 'This removes the file and every generated size from disk. It cannot be undone.', + confirmLabel: many ? `Delete ${targets.length} files` : 'Delete permanently', + alwaysConfirm: true, + commit: () => { + for (const id of targets) void workspace.purgeAsset(id) + }, + }) }} deleteLabel={ many diff --git a/src/admin/pages/media/utils/actionTargets.ts b/src/admin/pages/media/utils/actionTargets.ts new file mode 100644 index 000000000..c7f40b6a0 --- /dev/null +++ b/src/admin/pages/media/utils/actionTargets.ts @@ -0,0 +1,12 @@ +/** + * Which assets an action on one tile applies to — the Finder rule. + * + * Acting on an item inside the selection acts on the whole selection; acting + * on one outside it acts on that item alone, so right-clicking away from a + * selection starts a new, unrelated action instead of sweeping the old + * selection in. Dragging and the context menu both resolve their ids here, + * so the same tile can never drag one set of files and delete another. + */ +export function actionTargets(clickedId: string, selectedIds: ReadonlySet): string[] { + return selectedIds.has(clickedId) ? [...selectedIds] : [clickedId] +}