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 1/7] 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, ) From 76ce90bd02c6dbe6b188b59020080d635e0020c9 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 21:37:14 +0200 Subject: [PATCH 2/7] feat(media): record what depends on an asset, starting with avatars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `media_usage_refs` has existed since the media schema landed and nothing has ever written to it. So the library cannot tell a decorative upload from an asset the product depends on, and one case of that loses data quietly. A profile picture is stored as an ordinary `media_assets` row. Nothing marks it: no `role` column, no filter hiding it, nothing in the grid to distinguish it from any other image. Tidying the library sweeps it into the trash, purging hard-deletes the row, and `users.avatar_media_id` goes to NULL through its `on delete set null` foreign key. The profile falls back to a Gravatar identicon and nothing anywhere says why. On one install it happened three times before anyone connected the two. This wires the table that was already designed for it. `setMediaUsageRef` points a `(kind, id)` pair at an asset, deleting the previous row first so a reference MOVES rather than accumulating — four avatar changes leave one row, not four, or the fifth deletion would warn about pictures replaced months ago and the warning becomes noise. Clearing an avatar clears the reference, because the asset deliberately stays in the library but nothing depends on it any more. `listMediaUsageRefs` answers for a whole selection in one query, since the question is always asked about a selection, and resolves a label an operator can act on — "Ada Lovelace", not "u1". `POST /media/usage` exposes it: a POST because a hundred ids is the wrong shape for a query string. `ref_kind` namespaces the source, so favicons, page nodes and CMS cells can register without touching consumers. This change only registers avatars and only reads them back; the confirmation copy that consumes it is separate. Co-Authored-By: Claude Opus 5 --- server/handlers/cms/me.ts | 14 +++ server/handlers/cms/media.ts | 28 +++++ server/repositories/media.ts | 83 ++++++++++++++ src/__tests__/server/mediaUsageRefs.test.ts | 114 ++++++++++++++++++++ 4 files changed, 239 insertions(+) create mode 100644 src/__tests__/server/mediaUsageRefs.test.ts diff --git a/server/handlers/cms/me.ts b/server/handlers/cms/me.ts index 46eb9feb2..b40ef4a36 100644 --- a/server/handlers/cms/me.ts +++ b/server/handlers/cms/me.ts @@ -54,6 +54,7 @@ import { acceptUploadedMedia, readUploadForm, } from './mediaUpload' +import { setMediaUsageRef } from '../../repositories/media' import { Type } from '@core/utils/typeboxHelpers' import { isValidEmail } from '@core/utils/email' import { MIN_PASSWORD_LENGTH, PASSWORD_TOO_SHORT_MESSAGE } from '@core/utils/passwordPolicy' @@ -328,6 +329,15 @@ export async function handleMeRoutes( if (asset instanceof Response) return asset const updated = await setUserAvatarMediaId(db, user.id, asset.id) + // Register the dependency so the media library stops treating a profile + // picture as an anonymous upload. Without it the asset is indistinguishable + // from a decorative one, and purging it nulls `avatar_media_id` through + // the column's `on delete set null` — silently, from the operator's side. + await setMediaUsageRef(db, { + assetId: asset.id, + refKind: 'user.avatar', + refId: user.id, + }) if (!updated) { // The user row vanished between auth and the update (e.g. concurrent // soft-delete). The uploaded asset stays in the media library — it's @@ -351,6 +361,10 @@ export async function handleMeRoutes( const updated = await setUserAvatarMediaId(db, user.id, null) if (!updated) return jsonResponse({ error: 'User not found' }, { status: 404 }) + // The asset stays in the library on purpose (see the file header), but it + // is no longer depended on — so the warning has to stop firing for it. + await setMediaUsageRef(db, { assetId: null, refKind: 'user.avatar', refId: user.id }) + await createAuditEvent(db, { actorUserId: user.id, action: 'user.update', diff --git a/server/handlers/cms/media.ts b/server/handlers/cms/media.ts index 00c122be2..3f5de3852 100644 --- a/server/handlers/cms/media.ts +++ b/server/handlers/cms/media.ts @@ -14,6 +14,8 @@ * permitted on already-trashed * assets) and removes the file * (`media.delete`) + * POST /admin/api/cms/media/usage — which of these assets are still + * depended on, and by what * POST /admin/api/cms/media/:id/restore — restore a soft-deleted asset * (`media.write`) * POST /admin/api/cms/media/:id/replace — overwrite the bytes for an asset @@ -43,6 +45,7 @@ import { deleteMediaAsset, getMediaAsset, listMediaAssets, + listMediaUsageRefs, restoreMediaAsset, softDeleteMediaAsset, updateMediaAssetMetadata, @@ -293,7 +296,32 @@ async function handleDeleteMedia( const ID_PATTERN = '(?[^/]+)' +/** + * Which of the given assets something still depends on. + * + * A POST because the id list is a selection and can be long — a query string + * of a hundred ids is the wrong shape for a read this cheap. + * + * The UI calls this before a destructive action so it can name what is about + * to break instead of warning in the abstract. Requires `media.read`: the + * response reveals nothing beyond what the library already lists. + */ +async function handleMediaUsage(req: Request, db: DbClient): Promise { + const user = await requireCapability(req, db, 'media.read') + if (user instanceof Response) return user + + const body = await readValidatedBody(req, MediaUsageQuerySchema) + if (!body) return badRequest('Invalid request body') + + return jsonResponse({ usage: await listMediaUsageRefs(db, body.assetIds) }) +} + +const MediaUsageQuerySchema = Type.Object({ + assetIds: Type.Array(Type.String(), { maxItems: 500 }), +}, { additionalProperties: false }) + const MEDIA_ROUTES: readonly Route<[]>[] = [ + { method: 'POST', pattern: `${MEDIA_PREFIX}/usage`, handler: handleMediaUsage }, { method: 'GET', pattern: MEDIA_PREFIX, handler: handleListMedia }, { method: 'POST', pattern: MEDIA_PREFIX, handler: handleUploadMedia }, { diff --git a/server/repositories/media.ts b/server/repositories/media.ts index 5878f6702..a50477e39 100644 --- a/server/repositories/media.ts +++ b/server/repositories/media.ts @@ -542,3 +542,86 @@ export async function importMediaAsset( externally_hosted = excluded.externally_hosted ` } + +// --------------------------------------------------------------------------- +// Usage references +// +// `media_usage_refs` has existed since the media schema landed but nothing +// wrote to it, so the library could not tell a decorative upload from an +// asset something depends on. That gap is how a profile picture — stored as +// an ordinary library row, with no marker distinguishing it — could be swept +// into the trash during a tidy-up and purged, silently nulling +// `users.avatar_media_id` through its `on delete set null` foreign key. +// +// `ref_kind` namespaces the source so more can be registered without touching +// consumers: `user.avatar` here, page nodes and site settings next. +// --------------------------------------------------------------------------- + +/** A thing that depends on an asset, resolved for display. */ +export interface MediaUsageRef { + assetId: string + refKind: string + refId: string + /** Human-readable, e.g. a person's name for an avatar. Never a raw id. */ + label: string +} + +/** + * Point a `(kind, id)` pair at an asset, replacing whatever it pointed at + * before. + * + * Deleting first is what makes this a MOVE rather than an accumulation: a + * user who changes their avatar four times should leave one row, not four, + * or the fifth deletion would warn about pictures they replaced months ago. + */ +export async function setMediaUsageRef( + db: DbClient, + args: { assetId: string | null; refKind: string; refId: string; refPath?: string }, +): Promise { + const refPath = args.refPath ?? '' + await db` + delete from media_usage_refs + where ref_kind = ${args.refKind} and ref_id = ${args.refId} and ref_path = ${refPath} + ` + if (!args.assetId) return + await db` + insert into media_usage_refs (asset_id, ref_kind, ref_id, ref_path) + values (${args.assetId}, ${args.refKind}, ${args.refId}, ${refPath}) + ` +} + +/** + * Which of these assets are still depended on, with something an operator can + * recognise. + * + * Takes a LIST because the question is always asked about a selection — the + * deletion path needs one round trip, not one per file. + */ +export async function listMediaUsageRefs( + db: DbClient, + assetIds: string[], +): Promise { + if (assetIds.length === 0) return [] + const placeholders = assetIds.map((_, i) => placeholder(db.dialect, i + 1)).join(", ") + const { rows } = await db.unsafe<{ + asset_id: string + ref_kind: string + ref_id: string + label: string | null + }>( + `select r.asset_id, r.ref_kind, r.ref_id, + case when r.ref_kind = 'user.avatar' + then coalesce(nullif(u.display_name, ''), u.email) + else null end as label + from media_usage_refs r + left join users u on u.id = r.ref_id and r.ref_kind = 'user.avatar' + where r.asset_id in (${placeholders})`, + assetIds, + ) + return rows.map((row) => ({ + assetId: row.asset_id, + refKind: row.ref_kind, + refId: row.ref_id, + label: row.label ?? row.ref_kind, + })) +} diff --git a/src/__tests__/server/mediaUsageRefs.test.ts b/src/__tests__/server/mediaUsageRefs.test.ts new file mode 100644 index 000000000..f89e5c061 --- /dev/null +++ b/src/__tests__/server/mediaUsageRefs.test.ts @@ -0,0 +1,114 @@ +/** + * Knowing that something still depends on a media asset. + * + * `media_usage_refs` shipped with the media schema and nothing ever wrote to + * it, so the library could not tell a decorative upload from an asset the + * product depends on. A profile picture is stored as an ordinary library row + * with no marker of any kind — so tidying up the library swept one into the + * trash, purging it hard-deleted the row, and `users.avatar_media_id` went + * quietly to NULL through its `on delete set null` foreign key. The profile + * fell back to a Gravatar identicon with nothing to explain why. + * + * These cover the two behaviours the warning depends on: a reference MOVES + * rather than accumulating, and a cleared one stops reporting. + */ + +import { afterEach, describe, expect, it } from 'bun:test' +import { createTestDb } from '../helpers/createTestDb' +import { + listMediaUsageRefs, + setMediaUsageRef, +} from '../../../server/repositories/media' + +const cleanups: Array<() => Promise> = [] + +afterEach(async () => { + for (const cleanup of cleanups.splice(0)) await cleanup() +}) + +async function freshDb() { + const { db, cleanup } = await createTestDb() + cleanups.push(cleanup) + await db` + insert into users (id, email, email_normalized, display_name, password_hash, status, role_id) + values ('u1', 'ada@example.com', 'ada@example.com', 'Ada Lovelace', 'hash', 'active', 'owner') + ` + return db +} + +async function insertAsset(db: Awaited>, id: string) { + await db` + insert into media_assets (id, filename, mime_type, size_bytes, storage_path, public_path) + values (${id}, ${`${id}.png`}, 'image/png', 10, ${`/s/${id}`}, ${`/uploads/${id}.png`}) + ` +} + +describe('media usage references', () => { + it('reports nothing for an asset nobody depends on', async () => { + const db = await freshDb() + await insertAsset(db, 'a1') + expect(await listMediaUsageRefs(db, ['a1'])).toEqual([]) + }) + + it('names the person whose avatar it is, not the raw id', async () => { + // The whole point is a confirmation an operator can act on. "u1" tells + // them nothing; "Ada Lovelace" tells them what breaks. + const db = await freshDb() + await insertAsset(db, 'a1') + await setMediaUsageRef(db, { assetId: 'a1', refKind: 'user.avatar', refId: 'u1' }) + + const refs = await listMediaUsageRefs(db, ['a1']) + expect(refs).toHaveLength(1) + expect(refs[0]!.label).toBe('Ada Lovelace') + expect(refs[0]!.refKind).toBe('user.avatar') + }) + + it('falls back to the email when there is no display name', async () => { + const db = await freshDb() + await db`update users set display_name = '' where id = 'u1'` + await insertAsset(db, 'a1') + await setMediaUsageRef(db, { assetId: 'a1', refKind: 'user.avatar', refId: 'u1' }) + expect((await listMediaUsageRefs(db, ['a1']))[0]!.label).toBe('ada@example.com') + }) + + it('MOVES the reference when the avatar is replaced', async () => { + // Four avatar changes must leave one row, not four — otherwise deleting + // the fifth picture would warn about ones replaced months ago, and the + // warning becomes noise the operator learns to click past. + const db = await freshDb() + await insertAsset(db, 'old') + await insertAsset(db, 'new') + await setMediaUsageRef(db, { assetId: 'old', refKind: 'user.avatar', refId: 'u1' }) + await setMediaUsageRef(db, { assetId: 'new', refKind: 'user.avatar', refId: 'u1' }) + + expect(await listMediaUsageRefs(db, ['old'])).toEqual([]) + expect((await listMediaUsageRefs(db, ['new']))[0]!.label).toBe('Ada Lovelace') + }) + + it('stops reporting once the avatar is cleared', async () => { + // The asset deliberately stays in the library, but nothing depends on it + // any more — so deleting it should no longer warn. + const db = await freshDb() + await insertAsset(db, 'a1') + await setMediaUsageRef(db, { assetId: 'a1', refKind: 'user.avatar', refId: 'u1' }) + await setMediaUsageRef(db, { assetId: null, refKind: 'user.avatar', refId: 'u1' }) + expect(await listMediaUsageRefs(db, ['a1'])).toEqual([]) + }) + + it('answers for a whole selection in one call', async () => { + // The deletion path asks about every selected file at once; asking per + // file would mean one round trip per row of a bulk delete. + const db = await freshDb() + for (const id of ['a1', 'a2', 'a3']) await insertAsset(db, id) + await setMediaUsageRef(db, { assetId: 'a2', refKind: 'user.avatar', refId: 'u1' }) + + const refs = await listMediaUsageRefs(db, ['a1', 'a2', 'a3']) + expect(refs).toHaveLength(1) + expect(refs[0]!.assetId).toBe('a2') + }) + + it('returns nothing for an empty selection without touching the database', async () => { + const db = await freshDb() + expect(await listMediaUsageRefs(db, [])).toEqual([]) + }) +}) From 2960f0878ac9f5223715f645ca835359d892ab52 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sat, 5 Sep 2026 21:43:59 +0200 Subject: [PATCH 3/7] feat(media): the copy a confirmation shows when part of a selection is in use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildUsageWarning` turns the reference rows into the sentence a destructive confirmation puts above its buttons, and `useMediaWorkspace.lookupUsage` fetches them. Kept as a pure function with its own tests because the rules are judgement, not mechanics, and each one came from imagining the actual moment: eleven files selected, one of them a profile picture. SEPARATE. "1 of 11 is still in use" lets the operator see the other ten are safe. A blanket "some of these are in use" is the kind of warning people learn to click past, because it never says which. NAME IT. "profile picture — Ada Lovelace", not "has a reference". They have to recognise what they are about to lose. DO NOT BLOCK. Deleting an in-use asset is a legitimate thing to want — replacing an avatar begins exactly that way. The confirmation informs; the operator still decides. Past three named items it summarises, or the dialog becomes a wall of text nobody reads. An asset two things depend on counts once, because one file is what disappears. And the lookup never throws: it decorates a confirmation that must still appear if the request fails, so a network blip degrades to the plain warning rather than blocking the delete. The confirmation itself lives in a separate PR — this is the data and the copy it will render. Co-Authored-By: Claude Opus 5 --- src/__tests__/media/usageWarning.test.ts | 70 +++++++++++++++++++ .../pages/media/hooks/useMediaWorkspace.ts | 22 ++++++ src/admin/pages/media/utils/usageWarning.ts | 64 +++++++++++++++++ src/core/persistence/cmsMedia.ts | 33 +++++++++ src/core/persistence/responseSchemas.ts | 17 +++++ 5 files changed, 206 insertions(+) create mode 100644 src/__tests__/media/usageWarning.test.ts create mode 100644 src/admin/pages/media/utils/usageWarning.ts diff --git a/src/__tests__/media/usageWarning.test.ts b/src/__tests__/media/usageWarning.test.ts new file mode 100644 index 000000000..64f2ab9b7 --- /dev/null +++ b/src/__tests__/media/usageWarning.test.ts @@ -0,0 +1,70 @@ +/** + * What a destructive confirmation says when part of the selection is in use. + * + * The scenario that produced these rules: eleven files selected, one of them + * a profile picture, and the operator about to purge the lot. A warning that + * says "some of these are in use" tells them nothing actionable; one that + * blocks the delete stops them replacing their own avatar. + */ + +import { describe, expect, it } from 'bun:test' +import { buildUsageWarning } from '@admin/pages/media/utils/usageWarning' + +const avatar = (assetId: string, label: string) => ({ + assetId, refKind: 'user.avatar', refId: 'u1', label, +}) + +describe('the usage warning', () => { + it('says nothing when nothing is depended on', () => { + // The ordinary confirmation stands on its own; an empty warning box + // would train people to ignore the space it occupies. + expect(buildUsageWarning(11, [])).toBeNull() + }) + + it('separates the used from the safe', () => { + // "1 of 11" is the whole point — it tells the operator the other ten + // carry no risk, which a blanket warning never does. + const warning = buildUsageWarning(11, [avatar('a1', 'Ada Lovelace')]) + expect(warning?.heading).toBe('1 of 11 is still in use:') + }) + + it('drops the count when everything selected is in use', () => { + // "2 of 2" reads like arithmetic. Naming the state is clearer. + const warning = buildUsageWarning(2, [avatar('a1', 'Ada'), avatar('a2', 'Grace')]) + expect(warning?.heading).toBe('These files are still in use:') + }) + + it('names what breaks rather than describing the reference', () => { + const warning = buildUsageWarning(3, [avatar('a1', 'Ada Lovelace')]) + expect(warning?.lines).toEqual(['profile picture — Ada Lovelace']) + }) + + it('counts an asset once even when two things depend on it', () => { + // One file is lost, not two — the count is about what disappears. + const warning = buildUsageWarning(4, [ + avatar('a1', 'Ada Lovelace'), + { assetId: 'a1', refKind: 'user.avatar', refId: 'u2', label: 'Grace Hopper' }, + ]) + expect(warning?.heading).toBe('1 of 4 is still in use:') + expect(warning?.lines).toHaveLength(1) + }) + + it('summarises past three so the dialog stays readable', () => { + const refs = ['a1', 'a2', 'a3', 'a4', 'a5'].map((id, i) => avatar(id, `User ${i + 1}`)) + const warning = buildUsageWarning(20, refs) + expect(warning?.lines).toHaveLength(4) + expect(warning?.lines.at(-1)).toBe('and 2 more') + }) + + it('does not summarise at exactly three', () => { + const refs = ['a1', 'a2', 'a3'].map((id, i) => avatar(id, `User ${i + 1}`)) + expect(buildUsageWarning(9, refs)?.lines).toHaveLength(3) + }) + + it('uses singular and plural correctly', () => { + expect(buildUsageWarning(5, [avatar('a1', 'Ada')])?.heading).toContain(' is still') + expect( + buildUsageWarning(5, [avatar('a1', 'Ada'), avatar('a2', 'Grace')])?.heading, + ).toContain(' are still') + }) +}) diff --git a/src/admin/pages/media/hooks/useMediaWorkspace.ts b/src/admin/pages/media/hooks/useMediaWorkspace.ts index 298135168..e82cc4b94 100644 --- a/src/admin/pages/media/hooks/useMediaWorkspace.ts +++ b/src/admin/pages/media/hooks/useMediaWorkspace.ts @@ -24,6 +24,7 @@ import { listCmsMediaAssets, listCmsMediaFolders, normalizeCmsMediaAsset, + listCmsMediaUsage, purgeCmsMediaAsset, renameCmsMediaAsset, replaceCmsMediaAssetFile, @@ -33,6 +34,7 @@ import { updateCmsMediaFolder, type CmsMediaAsset, type CmsMediaFolder, + type CmsMediaUsageRef, type UpdateCmsMediaAssetInput, } from '@core/persistence/cmsMedia' import { buildFolderTree, type MediaFolderNode } from '../utils/folderTree' @@ -107,6 +109,11 @@ export interface UseMediaWorkspaceResult extends WorkspaceLoadState { trashAsset: (assetId: string) => Promise restoreAsset: (assetId: string) => Promise purgeAsset: (assetId: string) => Promise + /** + * Which of these assets something still depends on. Asked before a + * destructive action so the confirmation can name what breaks. + */ + lookupUsage: (assetIds: string[]) => Promise setAssetFolders: ( assetId: string, input: { add?: string[]; remove?: string[] }, @@ -417,6 +424,20 @@ export function useMediaWorkspace(): UseMediaWorkspaceResult { return null }) + /** + * Never throws. A usage lookup is advisory — it decorates a confirmation + * that must still appear if the request fails, so a network blip degrades + * to the plain warning rather than blocking the delete. + */ + const lookupUsage = async (assetIds: string[]): Promise => { + try { + return await listCmsMediaUsage(assetIds) + } catch (err) { + console.error('[useMediaWorkspace] usage lookup failed:', err) + return [] + } + } + const purgeAsset = async (assetId: string): Promise => { await assetMut('Could not delete asset permanently', async () => { await purgeCmsMediaAsset(assetId) @@ -549,6 +570,7 @@ export function useMediaWorkspace(): UseMediaWorkspaceResult { trashAsset, restoreAsset, purgeAsset, + lookupUsage, setAssetFolders, moveAssetsToFolder, createFolder, diff --git a/src/admin/pages/media/utils/usageWarning.ts b/src/admin/pages/media/utils/usageWarning.ts new file mode 100644 index 000000000..bd5b1e514 --- /dev/null +++ b/src/admin/pages/media/utils/usageWarning.ts @@ -0,0 +1,64 @@ +/** + * The sentence a destructive confirmation shows when part of a selection is + * still depended on. + * + * Three rules, from watching a real deletion go wrong: + * + * SEPARATE. "1 of 11" lets the operator see the other ten are safe. A + * blanket "some of these are in use" is the kind of warning people learn to + * click past, because it never tells them which. + * + * NAME IT. "your profile picture", not "has a reference". The point is that + * they recognise what they are about to lose. + * + * DO NOT BLOCK. Deleting an in-use asset is a legitimate thing to want — + * replacing an avatar starts exactly that way. The confirmation informs; + * the operator still decides. + */ + +import type { CmsMediaUsageRef } from '@core/persistence/cmsMedia' + +/** Beyond this many named items the list becomes a wall rather than a warning. */ +const MAX_NAMED = 3 + +export interface UsageWarning { + /** e.g. `1 of 11 is still in use:` */ + heading: string + /** One line per named dependency, already resolved for display. */ + lines: string[] +} + +function describe(ref: CmsMediaUsageRef): string { + switch (ref.refKind) { + case 'user.avatar': + return `profile picture — ${ref.label}` + default: + return ref.label + } +} + +/** + * `null` when nothing in the selection is depended on — the caller shows its + * ordinary confirmation and says nothing extra. + */ +export function buildUsageWarning( + selectionSize: number, + refs: readonly CmsMediaUsageRef[], +): UsageWarning | null { + if (refs.length === 0) return null + + // One row per asset: an asset used twice is still one file to lose. + const byAsset = new Map() + for (const ref of refs) if (!byAsset.has(ref.assetId)) byAsset.set(ref.assetId, ref) + const used = [...byAsset.values()] + + const heading = selectionSize > used.length + ? `${used.length} of ${selectionSize} ${used.length === 1 ? 'is' : 'are'} still in use:` + : `${used.length === 1 ? 'This file is' : 'These files are'} still in use:` + + const named = used.slice(0, MAX_NAMED).map(describe) + const rest = used.length - named.length + if (rest > 0) named.push(`and ${rest} more`) + + return { heading, lines: named } +} diff --git a/src/core/persistence/cmsMedia.ts b/src/core/persistence/cmsMedia.ts index 45cce28bc..1be2a5ffb 100644 --- a/src/core/persistence/cmsMedia.ts +++ b/src/core/persistence/cmsMedia.ts @@ -5,7 +5,9 @@ import { CmsMediaFolderEnvelopeSchema, CmsMediaFolderListResponseSchema, CmsMediaListResponseSchema, + CmsMediaUsageEnvelopeSchema, type CmsMediaAssetWire, + type CmsMediaUsageRef, type CmsMediaFolder, } from './responseSchemas' @@ -202,6 +204,33 @@ export async function renameCmsMediaAsset( * — the file stays on disk; restore() un-stamps; `purgeCmsMediaAsset()` * finishes the job. */ +/** + * Which of these assets something still depends on. + * + * Called before a destructive action so the confirmation can name what + * breaks rather than warning in the abstract. A POST because a selection can + * carry a hundred ids, which is the wrong shape for a query string. + */ +export async function listCmsMediaUsage( + assetIds: string[], + options: ClientBase = {}, +): Promise { + if (assetIds.length === 0) return [] + const { fetchImpl, basePath } = resolveClient(options) + const res = await fetchImpl(`${basePath}/media/usage`, { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ assetIds }), + }) + const payload = await readEnvelope( + res, + CmsMediaUsageEnvelopeSchema, + `CMS media usage lookup failed with ${res.status}`, + ) + return payload.usage +} + export async function deleteCmsMediaAsset( assetId: string, options: ClientBase = {}, @@ -338,3 +367,7 @@ export async function deleteCmsMediaFolder( }) await assertOk(res, `CMS folder delete failed with ${res.status}`) } + +// Re-exported so consumers import media types from the media module rather +// than reaching into the shared schema file. +export type { CmsMediaUsageRef } diff --git a/src/core/persistence/responseSchemas.ts b/src/core/persistence/responseSchemas.ts index 218855836..b6c483718 100644 --- a/src/core/persistence/responseSchemas.ts +++ b/src/core/persistence/responseSchemas.ts @@ -140,6 +140,23 @@ export const CmsMediaAssetEnvelopeSchema = Type.Object({ asset: CmsMediaAssetSchema, }) +/** + * Something that depends on a media asset. `label` is already resolved for + * display — a person's name for an avatar — because the caller renders it + * into a confirmation and has no way to look an id up. + */ +export const CmsMediaUsageRefSchema = Type.Object({ + assetId: Type.String(), + refKind: Type.String(), + refId: Type.String(), + label: Type.String(), +}) +export type CmsMediaUsageRef = Static + +export const CmsMediaUsageEnvelopeSchema = Type.Object({ + usage: Type.Array(CmsMediaUsageRefSchema), +}) + const CmsMediaFolderSchema = Type.Object({ id: Type.String(), parentId: Type.Union([Type.String(), Type.Null()]), From 1d36cd0e3eabd4e71fe32bf7d0861b12d1df7dc7 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:33:01 +0200 Subject: [PATCH 4/7] feat(media): protect the avatar that is already set, not just the next one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registering a usage reference on upload leaves every existing install in the one state the feature was built for and silent about it: an avatar set before this code shipped has no reference, so the first confirmation that warns before a permanent delete would say nothing about it. The operator would have to re-upload the same picture to be told it matters. The backfill inserts a `user.avatar` reference for every user who has one. Idempotent by construction — `not exists` on the same key `setMediaUsageRef` writes, which is also what makes the backfilled row indistinguishable from an app-written one, so a later avatar change MOVES it instead of leaving the old picture warning forever. Co-Authored-By: Claude Opus 5 --- server/db/migrations-pg.ts | 30 +++++++++ server/db/migrations-sqlite.ts | 30 +++++++++ src/__tests__/server/mediaUsageRefs.test.ts | 74 +++++++++++++++++++++ 3 files changed, 134 insertions(+) diff --git a/server/db/migrations-pg.ts b/server/db/migrations-pg.ts index 7e3130d06..d6efc2aba 100644 --- a/server/db/migrations-pg.ts +++ b/server/db/migrations-pg.ts @@ -1190,4 +1190,34 @@ export const pgMigrations: Migration[] = [ on plugin_media_sources (asset_id); `, }, + { + // Existing avatars predate anything writing `media_usage_refs`, so the + // first build that warns before a delete would still have said nothing + // about the avatar already set — the one case the feature exists for. + // + // Idempotent by construction: `not exists` on the same key + // `setMediaUsageRef` writes, so re-running inserts nothing and the row a + // later avatar change moves is the row this created. + // + // `028`, skipping `027`, on purpose: #335 is in review and already claims + // `027_data_tables_created_by_plugin`. Ids are only ever sorted, so a gap + // costs nothing — and whichever of the two lands first, neither has to be + // renumbered. A migration an installation has already recorded can never + // be renamed: the runner keys on the full id, so a new one re-runs SQL + // that is not idempotent and fails the boot. + id: '028_backfill_avatar_usage_refs', + sql: ` + insert into media_usage_refs (asset_id, ref_kind, ref_id, ref_path) + select u.avatar_media_id, 'user.avatar', u.id, '' + from users u + where u.avatar_media_id is not null + and not exists ( + select 1 from media_usage_refs r + where r.asset_id = u.avatar_media_id + and r.ref_kind = 'user.avatar' + and r.ref_id = u.id + and r.ref_path = '' + ); + `, + }, ] diff --git a/server/db/migrations-sqlite.ts b/server/db/migrations-sqlite.ts index db3deb93a..958e24bb5 100644 --- a/server/db/migrations-sqlite.ts +++ b/server/db/migrations-sqlite.ts @@ -1266,4 +1266,34 @@ export const sqliteMigrations: Migration[] = [ on plugin_media_sources (asset_id); `, }, + { + // Existing avatars predate anything writing `media_usage_refs`, so the + // first build that warns before a delete would still have said nothing + // about the avatar already set — the one case the feature exists for. + // + // Idempotent by construction: `not exists` on the same key + // `setMediaUsageRef` writes, so re-running inserts nothing and the row a + // later avatar change moves is the row this created. + // + // `028`, skipping `027`, on purpose: #335 is in review and already claims + // `027_data_tables_created_by_plugin`. Ids are only ever sorted, so a gap + // costs nothing — and whichever of the two lands first, neither has to be + // renumbered. A migration an installation has already recorded can never + // be renamed: the runner keys on the full id, so a new one re-runs SQL + // that is not idempotent and fails the boot. + id: '028_backfill_avatar_usage_refs', + sql: ` + insert into media_usage_refs (asset_id, ref_kind, ref_id, ref_path) + select u.avatar_media_id, 'user.avatar', u.id, '' + from users u + where u.avatar_media_id is not null + and not exists ( + select 1 from media_usage_refs r + where r.asset_id = u.avatar_media_id + and r.ref_kind = 'user.avatar' + and r.ref_id = u.id + and r.ref_path = '' + ); + `, + }, ] diff --git a/src/__tests__/server/mediaUsageRefs.test.ts b/src/__tests__/server/mediaUsageRefs.test.ts index f89e5c061..9dc1104a3 100644 --- a/src/__tests__/server/mediaUsageRefs.test.ts +++ b/src/__tests__/server/mediaUsageRefs.test.ts @@ -15,6 +15,8 @@ import { afterEach, describe, expect, it } from 'bun:test' import { createTestDb } from '../helpers/createTestDb' +import { pgMigrations } from '../../../server/db/migrations-pg' +import { sqliteMigrations } from '../../../server/db/migrations-sqlite' import { listMediaUsageRefs, setMediaUsageRef, @@ -112,3 +114,75 @@ describe('media usage references', () => { expect(await listMediaUsageRefs(db, [])).toEqual([]) }) }) + +/** + * Run one shipped migration's own SQL, by id. + * + * `createTestDb` has already applied every migration before the test writes a + * row, so the backfill ran against an empty `users` table and the tracker now + * says it is done. Replaying its SQL directly is what actually exercises it — + * and running it twice is the only honest test of the `not exists` guard. + */ +async function replayMigration(db: Awaited>, id: string) { + const list = db.dialect === 'postgres' ? pgMigrations : sqliteMigrations + const migration = list.find((m) => m.id === id) + if (!migration) throw new Error(`No migration ${id} — was it renamed?`) + await db.unsafe(migration.sql) +} + +const BACKFILL = '028_backfill_avatar_usage_refs' + +describe('the avatar backfill', () => { + it('protects an avatar that was set before anything recorded usage', async () => { + // Every install that already has an avatar is in exactly this state. + // Without the backfill, the first build that warns before a delete would + // still say nothing about the picture already set — the one case the + // whole feature exists for. + const db = await freshDb() + await insertAsset(db, 'a1') + await db`update users set avatar_media_id = 'a1' where id = 'u1'` + + await replayMigration(db, BACKFILL) + + const refs = await listMediaUsageRefs(db, ['a1']) + expect(refs).toHaveLength(1) + expect(refs[0]!.refKind).toBe('user.avatar') + expect(refs[0]!.label).toBe('Ada Lovelace') + }) + + it('adds nothing on top of a reference that is already there', async () => { + const db = await freshDb() + await insertAsset(db, 'a1') + await db`update users set avatar_media_id = 'a1' where id = 'u1'` + await setMediaUsageRef(db, { assetId: 'a1', refKind: 'user.avatar', refId: 'u1' }) + + await replayMigration(db, BACKFILL) + await replayMigration(db, BACKFILL) + + expect(await listMediaUsageRefs(db, ['a1'])).toHaveLength(1) + }) + + it('leaves a user with no avatar alone', async () => { + const db = await freshDb() + await insertAsset(db, 'a1') + await replayMigration(db, BACKFILL) + expect(await listMediaUsageRefs(db, ['a1'])).toEqual([]) + }) + + it('writes the row a later avatar change will MOVE, not a second one', async () => { + // The backfilled row has to be indistinguishable from one the app wrote, + // or changing the avatar afterwards would leave the old picture warning + // forever. `setMediaUsageRef` deletes on (ref_kind, ref_id, ref_path) — + // so the backfill must write the same key. + const db = await freshDb() + await insertAsset(db, 'old') + await insertAsset(db, 'new') + await db`update users set avatar_media_id = 'old' where id = 'u1'` + await replayMigration(db, BACKFILL) + + await setMediaUsageRef(db, { assetId: 'new', refKind: 'user.avatar', refId: 'u1' }) + + expect(await listMediaUsageRefs(db, ['old'])).toEqual([]) + expect(await listMediaUsageRefs(db, ['new'])).toHaveLength(1) + }) +}) From e0ea3a838eb0e130e9b2f23c9eec1c7882f36ec4 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Sun, 6 Sep 2026 15:05:32 +0200 Subject: [PATCH 5/7] feat(media): tell the operator which files are still in use before deleting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recording and the wording shipped without the thing that shows them, so a permanent delete still said nothing about an avatar in the selection. This connects the two, on both surfaces that offer that delete: the viewer window and the grid's context menu. Each owns its own dialog, so each is wired separately — and there is a test per surface, because one being left behind is exactly the failure this shape invites. The lookup is awaited BEFORE the dialog opens. A warning that arrives while the operator is already reading the dialog is a warning they will not see. Two things fell out of building it: `resolveUsageWarning` is a plain async function, not a hook holding state. The state version could not work: every caller opens its dialog in the same closure that requested the lookup, where a `setState` is not yet visible, so the warning would always have been one delete behind. It is also where "never rejects" now lives, instead of in each `lookupUsage`. Every dialog opens from a floating `void (async () => …)()`, which drops a rejection silently — so a failed lookup would have produced a delete button that did nothing at all, which is worse than one that deletes without the extra warning. One door, one guarantee. `ConfirmDeleteRequest` gains a generic `details` slot rather than a media concept: the shared dialog also confirms page and layer deletes and should not learn what a media asset is. Co-Authored-By: Claude Opus 5 --- .../media/purgeConfirmation.test.tsx | 28 ++- .../media/usageWarningInConfirmation.test.tsx | 181 ++++++++++++++++++ src/admin/pages/media/MediaPage.tsx | 1 + .../components/MediaCanvas/MediaCanvas.tsx | 25 ++- .../MediaViewerWindow/MediaViewerWindow.tsx | 24 ++- .../UsageWarningNotice.module.css | 47 +++++ .../UsageWarningNotice/UsageWarningNotice.tsx | 31 +++ .../components/UsageWarningNotice/index.ts | 1 + .../pages/media/hooks/useMediaWorkspace.ts | 17 +- .../media/hooks/useStandaloneMediaEditor.ts | 8 + src/admin/pages/media/utils/usageWarning.ts | 35 ++++ .../ConfirmDeleteContext.tsx | 1 + .../ConfirmDeleteDialog.tsx | 6 +- .../ConfirmDeleteDialog/confirmDeleteHook.ts | 8 +- 14 files changed, 378 insertions(+), 35 deletions(-) create mode 100644 src/__tests__/media/usageWarningInConfirmation.test.tsx create mode 100644 src/admin/pages/media/components/UsageWarningNotice/UsageWarningNotice.module.css create mode 100644 src/admin/pages/media/components/UsageWarningNotice/UsageWarningNotice.tsx create mode 100644 src/admin/pages/media/components/UsageWarningNotice/index.ts diff --git a/src/__tests__/media/purgeConfirmation.test.tsx b/src/__tests__/media/purgeConfirmation.test.tsx index fb49dbdf5..5c3bd7afd 100644 --- a/src/__tests__/media/purgeConfirmation.test.tsx +++ b/src/__tests__/media/purgeConfirmation.test.tsx @@ -64,6 +64,7 @@ function renderViewer() { purgeAsset: async (id: string) => { purged.push(id) }, + lookupUsage: async () => [], } // `media.delete` is what renders the purge button at all. const session = { @@ -82,37 +83,44 @@ function renderViewer() { return { purged } } -/** The button in the sidebar, not the one inside the dialog footer. */ -function clickPurgeButton() { +/** + * The button in the sidebar, not the one inside the dialog footer. + * + * Awaited because the click looks up what still depends on the asset before + * opening the dialog — the warning has to be part of what the operator reads, + * not something that appears after they have decided. + */ +async function clickPurgeButton() { const buttons = screen.getAllByRole('button', { name: /delete permanently/i }) fireEvent.click(buttons[0]!) + await screen.findByRole('alertdialog') } describe('permanent media deletion asks first', () => { - it('does not purge on the first click', () => { + it('does not purge on the first click', async () => { const { purged } = renderViewer() - clickPurgeButton() + await clickPurgeButton() expect(purged).toEqual([]) }) - it('names the asset so the operator can see what they are about to lose', () => { + it('names the asset so the operator can see what they are about to lose', async () => { renderViewer() - clickPurgeButton() + await 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', () => { + it('cancelling leaves the asset alone', async () => { const { purged } = renderViewer() - clickPurgeButton() + await clickPurgeButton() fireEvent.click(screen.getByRole('button', { name: /cancel/i })) expect(purged).toEqual([]) }) - it('confirming is what commits the purge', () => { + it('confirming is what commits the purge', async () => { const { purged } = renderViewer() - clickPurgeButton() + await 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 }) diff --git a/src/__tests__/media/usageWarningInConfirmation.test.tsx b/src/__tests__/media/usageWarningInConfirmation.test.tsx new file mode 100644 index 000000000..3f2705991 --- /dev/null +++ b/src/__tests__/media/usageWarningInConfirmation.test.tsx @@ -0,0 +1,181 @@ +/** + * The confirmation names what it is about to break. + * + * `buildUsageWarning` is tested on its own (usageWarning.test.ts) — these + * cover the part that only shows up once it is wired: that every surface + * offering a permanent delete looks the usage up BEFORE it opens the dialog, + * that the answer reaches the dialog the operator is reading, and that a + * failed lookup degrades to the plain confirmation instead of blocking a + * delete or swallowing one. + * + * Each surface that offers the delete owns its own dialog, so each has to be + * wired separately — and a test per surface is the only thing that catches one + * being left behind. The bulk window is a third such surface; its permanent + * delete arrives with #500, and its wiring belongs in whichever of the two + * lands second. + */ + +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' +import type { CmsMediaUsageRef } from '@core/persistence/cmsMedia' + +afterEach(cleanup) + +function asset(id: string, filename: string) { + return { + id, + filename, + mimeType: 'image/png', + sizeBytes: 1200, + publicPath: `/uploads/${filename}`, + 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, + } +} + +const AVATAR_REF: CmsMediaUsageRef = { + assetId: 'a1', + refKind: 'user.avatar', + refId: 'u1', + label: 'Ada Lovelace', +} + +const session = { + user: { id: 'u1', capabilities: ['media.delete', 'media.write'] }, + setUser: () => {}, +} + +// ── Viewer window ─────────────────────────────────────────────────────────── + +function renderViewer(lookupUsage: (ids: string[]) => Promise) { + const asked: string[][] = [] + const editor = { + asset: asset('a1', 'logo.png'), + tagPalette: [], + folderById: new Map(), + updateAsset: async () => undefined, + renameAsset: async () => undefined, + replaceAssetFile: async () => undefined, + restoreAsset: async () => undefined, + purgeAsset: async () => undefined, + lookupUsage: async (ids: string[]) => { + asked.push(ids) + return await lookupUsage(ids) + }, + } + render( + + [0]['editor']} + open + onClose={() => {}} + /> + , + ) + return { asked } +} + +async function openViewerPurge() { + const buttons = screen.getAllByRole('button', { name: /delete permanently/i }) + fireEvent.click(buttons[0]!) + await screen.findByRole('alertdialog') +} + +describe('the viewer window', () => { + it('names the person whose avatar it is', async () => { + renderViewer(async () => [AVATAR_REF]) + await openViewerPurge() + expect(screen.getByText(/profile picture — Ada Lovelace/)).toBeTruthy() + }) + + it('asks before the dialog opens, not after', async () => { + // If the lookup were fired alongside the dialog, the warning would land + // after the operator has already read it and moved to the button. + const { asked } = renderViewer(async () => [AVATAR_REF]) + await openViewerPurge() + expect(asked).toEqual([['a1']]) + }) + + it('says nothing extra when nothing depends on the file', async () => { + renderViewer(async () => []) + await openViewerPurge() + expect(screen.queryByText(/still in use/)).toBeNull() + expect(screen.getByText('Delete "logo.png" permanently?')).toBeTruthy() + }) + + it('still confirms when the lookup fails', async () => { + // Advisory, not a gate. A network blip must not turn a delete into a + // dead button — nor into an unconfirmed one. + renderViewer(async () => { + throw new Error('network') + }) + await openViewerPurge() + expect(screen.getByText('Delete "logo.png" permanently?')).toBeTruthy() + expect(screen.queryByText(/still in use/)).toBeNull() + }) +}) + +// ── The grid's context menu ───────────────────────────────────────────────── +// +// It routes through the shared `ConfirmDeleteDialog` rather than a local one, +// so what has to hold is the generic `details` slot that carries the warning +// across that boundary — plus the guarantee the surrounding +// `void (async () => …)()` depends on. + +describe('the shared confirm dialog', () => { + it('renders the caller-owned details below the description', async () => { + const { ConfirmDeleteDialog } = await import( + '@admin/shared/dialogs/ConfirmDeleteDialog/ConfirmDeleteDialog' + ) + render( + 1 of 3 is still in use:

} + onCancel={() => {}} + onConfirm={() => {}} + />, + ) + expect(screen.getByText('1 of 3 is still in use:')).toBeTruthy() + }) +}) + +describe('resolveUsageWarning', () => { + it('never rejects, so the dialog it precedes always opens', async () => { + // Every surface opens its dialog from a floating `void (async () => …)()`. + // A rejection there is dropped silently and the dialog never appears — a + // delete button that does nothing at all. + const { resolveUsageWarning } = await import('@admin/pages/media/utils/usageWarning') + const warning = await resolveUsageWarning(async () => { + throw new Error('network') + }, ['a1']) + expect(warning).toBeNull() + }) + + it('does not ask the server about an empty selection', async () => { + const { resolveUsageWarning } = await import('@admin/pages/media/utils/usageWarning') + let called = false + const warning = await resolveUsageWarning(async () => { + called = true + return [] + }, []) + expect(called).toBe(false) + expect(warning).toBeNull() + }) +}) diff --git a/src/admin/pages/media/MediaPage.tsx b/src/admin/pages/media/MediaPage.tsx index f81e3aa29..8b2246188 100644 --- a/src/admin/pages/media/MediaPage.tsx +++ b/src/admin/pages/media/MediaPage.tsx @@ -65,6 +65,7 @@ export function MediaPage() { replaceAssetFile: workspace.replaceAssetFile, restoreAsset: workspace.restoreAsset, purgeAsset: workspace.purgeAsset, + lookupUsage: workspace.lookupUsage, } : null diff --git a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx index ef314bdad..fe200e671 100644 --- a/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx +++ b/src/admin/pages/media/components/MediaCanvas/MediaCanvas.tsx @@ -54,6 +54,8 @@ import { writeMediaFolderDragData, } from '../../utils/mediaDragDrop' import { useMediaDnd } from '../../hooks/useMediaDnd' +import { UsageWarningNotice } from '../UsageWarningNotice' +import { resolveUsageWarning } from '../../utils/usageWarning' import styles from './MediaCanvas.module.css' import { AssetRow, @@ -541,14 +543,21 @@ export function MediaCanvas({ workspace, selectionMode = 'standard' }: MediaCanv // 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), - }) + // + // The usage lookup is awaited BEFORE the dialog opens so the + // warning is on screen when the operator reads it, not after. + void (async () => { + const warning = await resolveUsageWarning(workspace.lookupUsage, [target.id]) + confirmDelete({ + title: `Delete "${target.filename}" permanently?`, + description: + 'This removes the file and every generated size from disk. It cannot be undone.', + details: , + 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 726b0f7ae..6bd340ff1 100644 --- a/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx +++ b/src/admin/pages/media/components/MediaViewerWindow/MediaViewerWindow.tsx @@ -38,7 +38,14 @@ import { TrashSolidIcon } from 'pixel-art-icons/icons/trash-solid' import { VideoSolidIcon } from 'pixel-art-icons/icons/video-solid' import { PanelHeader } from '@admin/shared/PanelHeader' import { useDraggablePanel } from '@admin/shared/FloatingWindow' -import type { CmsMediaAsset, CmsMediaFolder, UpdateCmsMediaAssetInput } from '@core/persistence/cmsMedia' +import type { + CmsMediaAsset, + CmsMediaFolder, + CmsMediaUsageRef, + UpdateCmsMediaAssetInput, +} from '@core/persistence/cmsMedia' +import { resolveUsageWarning, type UsageWarning } from '../../utils/usageWarning' +import { UsageWarningNotice } from '../UsageWarningNotice' import { bucketForMime } from '../../utils/filters' import { useDebouncedSave } from '../../hooks/useDebouncedSave' import { TagEditor } from '../TagEditor/TagEditor' @@ -64,6 +71,7 @@ export interface MediaAssetEditor { replaceAssetFile: (id: string, file: File) => Promise restoreAsset: (id: string) => Promise purgeAsset: (id: string) => Promise + lookupUsage: (assetIds: string[]) => Promise } interface MediaViewerWindowProps { @@ -103,6 +111,7 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) { const { asset } = editor const [replaceOpen, setReplaceOpen] = useState(false) const [purgeConfirmOpen, setPurgeConfirmOpen] = useState(false) + const [purgeWarning, setPurgeWarning] = useState(null) const bucket = bucketForMime(asset.mimeType) const canWrite = canWriteMedia(currentUser) const canReplace = canReplaceMedia(currentUser) @@ -330,7 +339,17 @@ function ViewerForAsset({ editor, onClose }: ViewerForAssetProps) {