feat(media): warn before permanently deleting a file the site still uses - #507
Open
mostafasadeghidev wants to merge 13 commits into
Open
mostafasadeghidev wants to merge 13 commits into
mostafasadeghidev wants to merge 13 commits into
Conversation
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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
…s in use `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 <noreply@anthropic.com>
…t one 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 <noreply@anthropic.com>
…leting 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 <noreply@anthropic.com>
The spacing gate caught a `margin-top: 1px` optical alignment on the warning icon. A 1px nudge is not worth an exception to the token scale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mostafasadeghidev
added a commit
to mostafasadeghidev/Instatic
that referenced
this pull request
Sep 6, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Avatars could be RECORDED because a profile picture has one writer and an explicit set/unset. Page content has neither: it is written continuously by the collab relay, and taking an image off a page emits no event at all. A table fed by that would slowly fill with references to nodes that no longer exist, and the delete warning would start naming pages that are fine. A warning that is sometimes wrong is worse than none — it gets ignored. So content usage is computed when it is asked for, from the draft site document, which cannot drift because there is nothing to keep in sync. The test that matters pins exactly that: remove the image, and the reference is gone on the next question. The walk already existed. `mediaPrefetch` has resolved image/media props on every publish for a long time, including background images and the definition tree of every Visual Component a page references; it splits into a page-scoped half this can reuse, with publish behaviour unchanged. The module schema is already the registry of which props hold a reference, so there is no second list to keep honest. The cost lands on permanent delete and nowhere else — never a page load, never a trash. Reading the DRAFT document rather than published artefacts is deliberate: an image on an unpublished page is still in use, and a check that only knew about live pages would let a delete quietly break the next publish. Two things worth stating: `contentUsage.ts` imports `@modules/base` itself. Without the registry populated the walk matches nothing and reports NO usage — a warning that is silently always empty, which is the one failure mode worse than not having it. The warning now counts per asset but names per place. One file used on three pages is one file lost — "3 of 11" would overstate the damage — but it is three pages to go and fix, and naming one of them sends the operator to repair a third of the breakage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Site branches took 027–029 and the ISO timestamp rewrite took 030, so the avatar backfill moves from 028 to 032. It skips 031, which CoreBunch#335 now claims. Renumbering this one is safe in a way renumbering an ALTER is not: the backfill inserts only where `not exists`, so an installation that recorded it under 028 runs it again under 032 and inserts nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ide its new tone Brings in CoreBunch#505's renumbered backfill (032) and upstream main with it. `ConfirmDeleteContext` was the one conflict: site branches added a `tone` to the confirm request so a merge can ask without looking destructive, and this branch added `details` so a purge can say what is still in use. They are independent props, so the provider passes both. Site branches also made every draft read branch-scoped. This commit only supplies `MAIN_SCOPE` where the signatures now demand one — reading the other branches is a behaviour change, and it gets its own commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…used
Media is shared across branches; pages are not. Purging a file removes it
from all of them at once, but the usage lookup only read main — so an image
placed on a branch alone came back as "not in use", and deleting it broke
that branch's preview immediately and the live site when the branch merged.
The lookup now reads main explicitly, then every other branch, and reports:
- a use on main plainly, as before;
- a use on a branch that main does not share, with the branch's name;
- nothing extra for a branch that shares main's use. Every fork starts as
a full copy of main, so naming the page once per branch would repeat
the same line for every branch that exists.
Page ids are logical on every branch, which is what lets "the same page"
match across them. The copy renders the branch only when it is set:
`on the page — About us (branch "Redesign")`.
The cost is O(branches × site), still only on a permanent delete. Main is
read first by name rather than by relying on `listBranches` ordering it
first, so correctness does not hang on a sort order in another file.
docs/features/media.md gains the section this area was missing: what the
usage lookup reads, why settings are recorded and content is computed, and
the branch rule.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mostafasadeghidev
added a commit
to mostafasadeghidev/Instatic
that referenced
this pull request
Sep 13, 2026
CoreBunch#498 landing Twenty upstream commits, including two of this stack's own PRs (CoreBunch#359, CoreBunch#498) merged exactly as submitted. Resolved hunk by hunk, and each file checked against the resolution its PR branch reached independently: - content.ts, import.ts — every handler gains `MAIN_SCOPE`; every `@own-created` access check stays on the resolved table. Identical to CoreBunch#335's branch. - ConfirmDeleteContext — upstream's `tone` beside this stack's `details`. Identical to CoreBunch#507's branch. - publishSite.ts — only an import collided; the file now matches upstream byte for byte, as it should once CoreBunch#359 has landed. Migrations are the one place the stack deliberately differs from the PRs. `025_data_tables_created_by_plugin` keeps its id: two installations recorded it, and the ALTER cannot run twice. Its comment now says what to do when CoreBunch#335 lands as `031_…`. The avatar backfill takes CoreBunch#505's `032` — safe to renumber because it inserts only where `not exists`. `contentUsage.ts` gets `MAIN_SCOPE` to type-check; reading every branch arrives with CoreBunch#507's merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mostafasadeghidev
added a commit
to mostafasadeghidev/Instatic
that referenced
this pull request
Sep 13, 2026
The conflicts here are history rather than content: the stack took CoreBunch#507's warning through its own commits, combined with CoreBunch#500 and CoreBunch#501, so both sides look new to git. Each file was settled by what CoreBunch#507's commits since then actually changed: - MediaCanvas, MediaViewerWindow, the confirmation test, mediaPrefetch — untouched by those commits, so the stack's copies already hold all of CoreBunch#507 plus the multi-select and minimize work. Kept. - contentUsage.ts and its test — the branch-aware lookup is new, so CoreBunch#507's version is taken, with the stack's four-argument walker (`rootNodeId` scopes hole and loop fragments here). - usageWarning copy + tests, the ref schema, docs — merged cleanly. Before choosing a side, the stack's copies of both test files were checked against CoreBunch#507's pre-sync versions: identical, so nothing stack-only is lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mostafasadeghidev
added a commit
to mostafasadeghidev/Instatic
that referenced
this pull request
Sep 13, 2026
… and refuse a doubled ALTER README: CoreBunch#359 and CoreBunch#498 move to the landed table, the count drops to ten, and CoreBunch#507's row says what it does now — every page, on every branch. The fork gate's own rule is that a merged PR's row is deleted, since the capability then arrives with main. Nine merged rows had never been removed; they go. CoreBunch#336 was never pinned at all; it is now. And one new check, for the one merge mistake that takes a server down on boot: no column may be added by two different migrations. The stack runs CoreBunch#335's ALTER as `025_…` while upstream will carry it as `031_…`; keeping both when CoreBunch#335 lands makes every recorded installation run it twice. Verified by planting that exact `031` entry — the test names the collision and fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The note still said CoreBunch#335 claims 031. It now claims 033, because CoreBunch#495 took 031 — the backfill stays at 032 between them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… and 033 (CoreBunch#335) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mostafasadeghidev
added a commit
to mostafasadeghidev/Instatic
that referenced
this pull request
Sep 16, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #505, which records what depends on a media asset and builds the sentence. This is the part that puts it in front of the operator.
Important
Depends on #505 and #497. GitHub cannot base a cross-fork PR on another fork branch, so this targets
mainand the diff carries both of those PRs until they land. The new work is the commits after #505's — the warning wiring, the computed page usage, and reading every site branch — everything before them belongs to #505 (the recording and the wording) and #497 (the confirmation this decorates). Merge those two first and this diff collapses to that work.What it does
media_usage_refsandbuildUsageWarningshipped in #505 without anything that shows them, so a permanent delete still said nothing about an avatar in the selection:Both surfaces that offer a permanent delete are wired — the viewer window and the grid's context menu — and each owns its own dialog, so each was wired separately. There is a test per surface. The bulk window is a third such surface; its permanent delete arrives with #500, so its wiring belongs in whichever of the two lands second.
Page content is computed, not recorded
Avatars could be recorded (#505) because a profile picture has one writer and an explicit set/unset. Page content has neither: it is written continuously by the collab relay, and taking an image off a page emits no event at all. A table fed by that fills with references to nodes that no longer exist, and the warning starts naming pages that are fine — and a warning that is sometimes wrong is worse than none, because it gets ignored.
So
collectContentUsageRefscomputes the answer from the draft site document when it is asked. It cannot drift, because there is nothing to keep in sync. The test that matters pins exactly that: remove the image, ask again, the reference is gone.The walk already existed.
mediaPrefetchhas resolved image/media props on every publish for a long time — including background images and the definition tree of every Visual Component a page references. It splits into a page-scoped half this reuses; publish behaviour is unchanged. The module schema is already the registry of which props hold a media reference, so there is no second list to keep honest.The cost lands on permanent delete and nowhere else — never a page load, never a trash. Walking every page tree is O(site) on an action that is about to be irreversible. If a site ever outgrows that, the fix is to cache it with the walk still the source of truth, so the cache can be checked against it.
It reads the draft, not the published artefacts. An image on an unpublished page is still in use — a check that only knew about live pages would let a delete quietly break the next publish.
And it reads every site branch. Media is shared across branches while pages are not, so a purge removes a file from all of them at once. An image used only on a branch breaks that branch's preview immediately and the live site when the branch merges — but reading main alone would have called it unused. Main is reported plainly; a branch is named only where its use is not already on main:
Every fork starts as a copy of main, so naming a shared page once per branch would repeat the same line for every branch that exists. Page ids are logical on every branch, which is what lets the same page match across them. Main is read first by name rather than by relying on
listBranchesto order it first. The cost becomes O(branches × site), still only on a permanent delete.One detail worth flagging in review:
contentUsage.tsimports@modules/baseitself. Without the registry populated the walk matches nothing and reports no usage — a warning that is silently always empty, which is the one failure mode worse than not having it.Three decisions worth stating
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. It costs one round trip on a click that is about to be irreversible.
resolveUsageWarningis a plain async function, not a hook holding state. The state version cannot work here: every caller opens its dialog in the same closure that requested the lookup, where asetStateis not yet visible — the warning would always have been one delete behind. Callers that keep a dialog open across renders store the returned value themselves.It is also where "never rejects" lives, rather than in each
lookupUsage. Every dialog opens from a floatingvoid (async () => …)(), which drops a rejection silently — a failed lookup would have produced a delete button that does nothing at all, which is worse than one that deletes without the extra warning. One door, one guarantee, and the two hooks lose their duplicatedtry/catch.The shared dialog stays generic
ConfirmDeleteRequestgainsdetails?: ReactNode— caller-owned content below the description. Media passes the warning; nothing else passes anything. The alternative was teaching a dialog that also confirms page and layer deletes what a media asset is.The warning now counts per asset but names per place: one file on three pages is one file lost — "3 of 11" would overstate the damage — but it is three pages to go and fix, and naming one sends the operator to repair a third of the breakage.
docs/features/media.mdgains the section this area was missing: what the lookup reads, why settings are recorded and content computed, and the branch rule.Co-Authored-By: Claude Opus 5 noreply@anthropic.com