From 792f67ed93a5a608e387499df388b484918e9d0c Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:12:30 +0200 Subject: [PATCH] fix(media): confirm before permanently deleting an asset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Purging is the only media action with no undo. It removes the original binary AND every generated size from the storage adapter — `handleMediaItem` sweeps `existing.variants` alongside the original — and it fired on a single unguarded click. The button sits in the trash preview beside Restore, same row, same size, one colour apart, so the two read as a pair of equally reversible choices. The same applies to the trash view's right-click Delete. Both paths now confirm, naming the file and saying what goes with it. Two details worth flagging for review: `alwaysConfirm` on the context-menu path. The `confirmBeforeDelete` preference defaults off, and someone who turned it off was opting out of confirming a TRASH — a reversible move — not a purge. Without the flag this change would be a no-op for exactly the operators most likely to hit it. A local `Dialog` rather than `useConfirmDelete` in the viewer window. That hook falls back to running `commit()` immediately when no provider is mounted (confirmDeleteHook.ts:53-56), and this window also renders from the dashboard media widget, which mounts none. Routing it through the hook would have left one surface unguarded while reading as covered on all five. Also worth a maintainer's eye: `capabilityMeta.ts:87-90` tells operators that hard-purge "also requires step-up", but the route only calls `requireCapability(req, db, 'media.delete')` — no `requireStepUp`, unlike `data/tables.ts:169`. Left alone here because a server-side gate is a behaviour change that deserves its own PR, but the docs and the code currently disagree. Four tests pin the behaviour: no purge on first click, the dialog names the asset, Cancel is inert, and the second click is what commits. Co-Authored-By: Claude Opus 5 --- .../media/purgeConfirmation.test.tsx | 122 ++++++++++++++++++ .../components/MediaCanvas/MediaCanvas.tsx | 21 ++- .../MediaViewerWindow/MediaViewerWindow.tsx | 38 +++++- 3 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 src/__tests__/media/purgeConfirmation.test.tsx diff --git a/src/__tests__/media/purgeConfirmation.test.tsx b/src/__tests__/media/purgeConfirmation.test.tsx new file mode 100644 index 000000000..fb49dbdf5 --- /dev/null +++ b/src/__tests__/media/purgeConfirmation.test.tsx @@ -0,0 +1,122 @@ +/** + * Permanent deletion of a media asset has to ask first. + * + * "Delete permanently" removes the original binary AND every generated size + * from the storage adapter — not just the row. It is the only media action + * with no undo, and it sat one unguarded click away inside the trash preview, + * beside the Restore button it visually matches. + * + * These tests pin the confirmation itself rather than the wording: that the + * mutation does NOT fire on the first click, that Cancel leaves the asset + * alone, and that the second click is what commits. + * + * The dialog is local to the window on purpose. `useConfirmDelete` falls back + * to running `commit()` immediately when no `ConfirmDeleteProvider` is mounted + * (confirmDeleteHook.ts), and this window also renders from the dashboard + * media widget, which mounts none — so routing through that hook would have + * left one surface silently unguarded while looking covered everywhere. + */ + +import { afterEach, describe, expect, it } from 'bun:test' +import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import { MediaViewerWindow } from '@admin/pages/media/components/MediaViewerWindow/MediaViewerWindow' +import { AdminSessionContext } from '@admin/sessionContext' + +afterEach(cleanup) + +/** A trashed asset — the only state in which the purge button renders. */ +function trashedAsset() { + return { + id: 'asset_1', + filename: 'logo.png', + mimeType: 'image/png', + sizeBytes: 1200, + publicPath: '/uploads/logo.png', + uploadedByUserId: null, + createdAt: '2026-01-01T00:00:00.000Z', + altText: '', + caption: '', + title: '', + tags: [], + width: null, + height: null, + durationMs: null, + dominantColor: null, + deletedAt: '2026-02-01T00:00:00.000Z', + replacedAt: null, + folderIds: [], + blurHash: null, + variants: [], + posterPath: null, + } +} + +function renderViewer() { + const purged: string[] = [] + const editor = { + asset: trashedAsset(), + tagPalette: [], + folderById: new Map(), + updateAsset: async () => undefined, + renameAsset: async () => undefined, + replaceAssetFile: async () => undefined, + restoreAsset: async () => undefined, + purgeAsset: async (id: string) => { + purged.push(id) + }, + } + // `media.delete` is what renders the purge button at all. + const session = { + user: { id: 'u1', capabilities: ['media.delete', 'media.write'] }, + setUser: () => {}, + } + render( + + [0]['editor']} + open + onClose={() => {}} + /> + , + ) + return { purged } +} + +/** The button in the sidebar, not the one inside the dialog footer. */ +function clickPurgeButton() { + const buttons = screen.getAllByRole('button', { name: /delete permanently/i }) + fireEvent.click(buttons[0]!) +} + +describe('permanent media deletion asks first', () => { + it('does not purge on the first click', () => { + const { purged } = renderViewer() + clickPurgeButton() + expect(purged).toEqual([]) + }) + + it('names the asset so the operator can see what they are about to lose', () => { + renderViewer() + clickPurgeButton() + // The filename also appears in the window chrome, so match the dialog's + // own heading rather than any occurrence of it. + expect(screen.getByText('Delete "logo.png" permanently?')).toBeTruthy() + }) + + it('cancelling leaves the asset alone', () => { + const { purged } = renderViewer() + clickPurgeButton() + fireEvent.click(screen.getByRole('button', { name: /cancel/i })) + expect(purged).toEqual([]) + }) + + it('confirming is what commits the purge', () => { + const { purged } = renderViewer() + clickPurgeButton() + // The footer button — the last match, since the sidebar button is still + // mounted behind the dialog. + const buttons = screen.getAllByRole('button', { name: /delete permanently/i }) + fireEvent.click(buttons[buttons.length - 1]!) + expect(purged).toEqual(['asset_1']) + }) +}) diff --git a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx index 3dba08963..ef314bdad 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, @@ -122,6 +123,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) @@ -530,8 +532,23 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv onDelete={() => { const target = contextMenu.asset setContextMenu(null) - if (trashView) void workspace.purgeAsset(target.id) - else void workspace.trashAsset(target.id) + if (!trashView) { + void workspace.trashAsset(target.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. `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: `Delete "${target.filename}" permanently?`, + description: + 'This removes the file and every generated size from disk. It cannot be undone.', + confirmLabel: 'Delete permanently', + alwaysConfirm: true, + commit: () => void workspace.purgeAsset(target.id), + }) }} showRename={canWrite} showDelete={canDelete} diff --git a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx index 27b3a6a3b..726b0f7ae 100644 --- a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx +++ b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx @@ -27,6 +27,7 @@ import { useState, type ReactNode } from 'react' import { createPortal } from 'react-dom' import { Button } from '@ui/components/Button' +import { Dialog } from '@ui/components/Dialog' import { Input, Textarea } from '@ui/components/Input' import { canDeleteMedia, canReplaceMedia, canWriteMedia } from '@admin/access' import { useCurrentAdminUser } from '@admin/sessionContext' @@ -101,6 +102,7 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { const currentUser = useCurrentAdminUser() const { asset } = editor const [replaceOpen, setReplaceOpen] = useState(false) + const [purgeConfirmOpen, setPurgeConfirmOpen] = useState(false) const bucket = bucketForMime(asset.mimeType) const canWrite = canWriteMedia(currentUser) const canReplace = canReplaceMedia(currentUser) @@ -328,7 +330,7 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { + + + } + > +

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

+ , document.body, )