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 1/4] 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 2/4] 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 3/4] 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 0a9b8fdfdc513a33df203f22a9c0c2488c8b8040 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Thu, 17 Sep 2026 01:01:16 +0200 Subject: [PATCH 4/4] docs(db): say which ids sit either side of the backfill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note still said #335 claims 031. It now claims 033, because #495 took 031 — the backfill stays at 032 between them. Co-Authored-By: Claude Opus 5 --- server/db/migrations-pg.ts | 8 +++----- server/db/migrations-sqlite.ts | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/server/db/migrations-pg.ts b/server/db/migrations-pg.ts index 3d39a433f..75c6521f3 100644 --- a/server/db/migrations-pg.ts +++ b/server/db/migrations-pg.ts @@ -1370,11 +1370,9 @@ export const pgMigrations: Migration[] = [ // what makes the id itself safe to change: an installation that recorded // it under another number runs it again and inserts nothing. // - // `032`, skipping `031`, on purpose: #335 is in review and claims - // `031_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. That is not true of #335's own migration — an ALTER re-run - // under a new id fails the boot — which is why it keeps its number here. + // `032` sits between two ids other open PRs claim: `031` (#495) and `033` + // (#335). Ids are only ever sorted, so the gaps cost nothing, and + // whichever lands first, nobody has to renumber. id: '032_backfill_avatar_usage_refs', sql: ` insert into media_usage_refs (asset_id, ref_kind, ref_id, ref_path) diff --git a/server/db/migrations-sqlite.ts b/server/db/migrations-sqlite.ts index f2aeb5460..364726679 100644 --- a/server/db/migrations-sqlite.ts +++ b/server/db/migrations-sqlite.ts @@ -1526,11 +1526,9 @@ export const sqliteMigrations: Migration[] = [ // what makes the id itself safe to change: an installation that recorded // it under another number runs it again and inserts nothing. // - // `032`, skipping `031`, on purpose: #335 is in review and claims - // `031_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. That is not true of #335's own migration — an ALTER re-run - // under a new id fails the boot — which is why it keeps its number here. + // `032` sits between two ids other open PRs claim: `031` (#495) and `033` + // (#335). Ids are only ever sorted, so the gaps cost nothing, and + // whichever lands first, nobody has to renumber. id: '032_backfill_avatar_usage_refs', sql: ` insert into media_usage_refs (asset_id, ref_kind, ref_id, ref_path)