From 43a1bfef69197bd2bb1fde5349b9afb3efc6a44a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:03:42 -0500 Subject: [PATCH] docs(admin): design background removal in the inventory item editor (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design behind #293, with the three decisions the issue left open now answered. It applies to a live product image, and to every status including sold and reserved. The precedent in unpublish, which refuses both by name, does not carry: what that protects is a customer losing an item mid-checkout and a completed sale being quietly rewritten, and neither is at stake in a photograph's background. A sold item's photos are still the shop's photos. The action is per upload rather than per photo, and that is the decision shaping everything else. An upload is one item — the front, the back and the chipped base are three views of one vase, not three things to cut out separately. It also means DraftPhoto is not the component to lift, despite looking like it: the queue's control is per photo and this one is per item, so sharing it would force one to pretend to be the other. The real reuse is underneath, in removeImageBackground and restoreImageOriginal, which already exist and are already idempotent. removeBackgroundsForItem gains a summary return. It answers void today and throws on the first failure, which is enough for the worker — it catches and logs, and a draft is not worth failing over — and not enough for an admin standing in front of the screen. The one existing caller ignores the result, so widening it is additive, the same way sendMail was in #260. Writing it caught a contradiction in my own first draft worth recording. The failure table said a sidecar failure answers 502 while the screen section promised the admin sees "2 of 4 photos done", and both cannot be true, because a 502 throws away the count that makes the outcome actionable. Resolved by these two routes always answering 200 once the id is valid: they act on several images, so "did it work" has no single answer, and the summary is the result. Non-200 is reserved for not being able to try at all. That is a deliberate departure from #281's per-photo endpoints, which act on one image and can honestly say yes or no. Two ambiguities also fixed before they became implementation coin-flips: what the button says in a mixed state, which is exactly what a partial failure leaves behind and which reads Remove backgrounds because that is the action finishing the job; and that restore has no failure mode of its own, being a database swap with no sidecar in it. Co-Authored-By: Claude Opus 5 --- ...-04-inventory-background-removal-design.md | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md diff --git a/docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md b/docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md new file mode 100644 index 0000000..dadcec9 --- /dev/null +++ b/docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md @@ -0,0 +1,117 @@ +# Removing backgrounds from the inventory item editor + +**Issue:** #293. Follows #281, which built the same capability for the submission page and the review queue and deliberately scoped this out. + +An admin adding stock themselves goes straight to `Admin | Inventory`, uploads photos, and never touches the review queue — so for those items background removal does not exist. This adds it where the photos actually get edited. + +## Decisions, and what each one rests on + +**It applies to a live product image.** Removing a background on an `available` item changes what a customer sees while they are browsing. Accepted deliberately: the original is kept and restoring it is one click, so the worst case is a photo that looks wrong until somebody notices. The alternative is a shop whose published items can never be tidied up, which is worse. + +**Every status, with no exceptions.** Not `pending` only, and not a carve-out for `sold` or `reserved`. The precedent in `unpublish` — which refuses both by name — does not carry here, because what it protects against is a customer losing an item mid-checkout, or a completed sale being quietly rewritten. Neither is at stake in a photograph's background. A sold item's photos are still the shop's photos. + +**The action is per upload, not per photo.** This is the decision that shapes everything else, and it is a deliberate departure from the review queue. + +An upload is one item. Somebody photographing a vase sends the front, the back and the chipped base; those are three views of one thing. They are not separate items and they should not be cut out one at a time. So the control acts on every image belonging to the item, as a single action. + +**The shared function reports a summary rather than nothing.** `removeBackgroundsForItem` returns `void` today and throws on the first failure, which is enough for the worker — it catches and logs, and a draft is not worth failing over. It is not enough for an admin standing in front of the screen, who needs to know whether the thing they clicked actually happened. + +**It keeps stopping at the first failure.** Pushing on through six photos against a sidecar that is not answering helps nobody. The retry story works because `removeImageBackground` is already idempotent: a photo that already has an `original_image_path` is skipped, so a second attempt resumes where the first stopped rather than starting over or double-cutting anything. + +## Architecture + +``` +Admin | Inventory → edit item → Existing Images + │ + ├─ POST /api/admin/items/:id/remove-backgrounds → { total, removed, failed } + └─ POST /api/admin/items/:id/restore-originals → { total, restored } + │ + ▼ + backgroundRemoval.ts (already shared with the worker and the review queue) + removeBackgroundsForItem(itemId) → RemovalSummary + restoreOriginalsForItem(itemId) → RestoreSummary +``` + +### `removeBackgroundsForItem` gains a return value + +```ts +export interface RemovalSummary { + /** How many images the item has. */ + total: number; + /** How many now have a cut-out, including any that already did. */ + removed: number; + /** Whether it stopped early because one of them failed. */ + failed: boolean; +} +``` + +**The one existing caller does not change.** `draftingWorker.ts:175` ignores the result, and ignoring a returned value is legal — the same reason widening `sendMail` in #260 was additive rather than breaking. `npm run build` is what proves it. + +A matching `restoreOriginalsForItem` is new — the review queue restores one photo at a time through `restoreImageOriginal`, and nothing yet does a whole item: + +```ts +export interface RestoreSummary { + /** How many images the item has. */ + total: number; + /** How many were put back. Images that were never cut out are skipped, not counted. */ + restored: number; +} +``` + +It has no `failed`, because restoring cannot fail the way removing can: it is a database swap with no sidecar involved, and an image that was never cut out is skipped rather than being an error. + +### The endpoints + +Both take their id through `readId` and answer 404 for an unreadable or absent one, which is now what every route in `admin.ts` does (#207). Neither checks status. + +**These two routes always answer 200 once the id is valid**, and that is a deliberate departure from the per-photo endpoints in #281. + +Those act on one image, so the request either worked or it did not, and 502 says which. This one acts on several, so "did it work" has no single answer — two of four is the normal shape of a bad day, not an exception. Returning 502 would throw away the count that makes the outcome actionable, and 200-with-a-summary would then contradict it. So the summary *is* the result: `failed` says whether it stopped early, `removed` says how far it got, and the admin retries. + +Non-200 is reserved for not being able to try at all, which here means only an unreadable or absent id. The underlying `SidecarRequestError` from #281 still exists and still distinguishes a sidecar failure from an unreadable file — it is caught here and folded into `failed`, with the real reason logged rather than shown, because the admin's next action is the same either way: press it again. + +### The screen + +One button per item, in the "Existing Images" block, beside the per-thumbnail delete buttons rather than on them. + +Its label comes from the same single source of truth the review queue uses: **Restore originals** when every image already carries an `original_image_path`, **Remove backgrounds** otherwise. There is no second flag and no stored state — the images already say which they are. + +The "otherwise" deliberately covers the mixed case, which is not hypothetical: it is exactly what a partial failure leaves behind. With two of four cut out the button reads **Remove backgrounds**, which is the action that finishes the job, and pressing it skips the two that already succeeded. A button offering to restore at that point would be offering the wrong half of the work. + +Rendered only when the server reports the feature configured, exactly as the review queue's control is. An unconfigured environment shows no button rather than one that reports zero of four done every time. + +On a partial result the admin is told plainly — "2 of 4 photos done" — with the button still there to try again. + +**The editor is a modal, and this changes images on the server while it is open.** The thumbnails have to be refreshed after the action or they show the previous files, which would look like the button doing nothing. This is the detail most likely to turn into a confusing bug, so it is called out here rather than discovered. + +## Why `DraftPhoto` is not lifted + +The obvious-looking reuse is wrong. The review queue's control is per photo and this one is per item; sharing the component would force one of them to pretend to be the other. What is genuinely shared is underneath — `removeImageBackground` and `restoreImageOriginal`, which both already exist and are already idempotent. That is where the reuse belongs. + +## Failure handling + +| What happens | Result | +|---|---| +| Unreadable or absent item id | 404. Nothing touched. | +| Feature not configured | No button. The endpoint still answers, reporting `total` with `removed: 0`. | +| Sidecar will not answer | 200, `failed: true`, `removed: 0`. Nothing was touched. The reason is logged. | +| A file cannot be read | 200, `failed: true`, `removed` short of `total`. Photos done before it keep their cut-outs. | +| Some succeeded, then one failed | 200, `failed: true`, `removed` short of `total`. The admin retries; the second pass skips what already worked. | +| All succeeded | 200, `removed === total`, `failed: false`. | + +Nothing is ever left in a state a retry cannot resolve, and nothing is deleted — the same rule the whole feature has followed since #281. + +## Testing + +- **Integration:** both endpoints on an item with several images; a partial failure leaving a usable item and a retry completing it; 404 for a bad id; an item with no images answering `total: 0` rather than failing; restore returning the originals. +- **Unit:** none needed. The pure parts (`cutoutPathFor`) are already covered from #281, and the new code is all database and HTTP. +- **Worker:** the existing drafting tests prove the widened return did not disturb the one caller. +- **E2E:** the button appears on an item that has images, and does not when the feature is unconfigured. + +## Out of scope + +**Bulk application across the catalogue.** Still. This is one item at a time, from the editor for that item. + +**Any change to the submission page or the review queue.** They keep behaving exactly as #281 built them, including the queue's per-photo control. + +**A progress indicator for a long-running removal.** Measured at 1.1–2.3 s per image, so six photos is a slow click rather than a background job. If a real catalogue makes that intolerable, moving it off the request path is a separate change with its own decisions.