bermudalambandClaude Opus 5
b70e4a68f0
The spec asserted two things the implementation disproved. RestoreSummary was described as having no `failed` because "restoring cannot fail the way removing can" — true about the sidecar, wrong about the database, and rethrowing turned a partial success into an opaque 500. And the single-button-with-two-labels rule was described as deliberately covering the mixed case, when in fact it stranded it: a partly cut-out item offered only Remove, so its existing cut-outs had no way back. Both sections now describe what the code does and why, including why Restore is not gated on the feature being configured, and the outcome table's "feature not configured" row is corrected to say Restore is still offered and still works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
120 lines
9.5 KiB
Markdown
120 lines
9.5 KiB
Markdown
# 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;
|
||
/** Whether it stopped early because one of them failed. */
|
||
failed: boolean;
|
||
}
|
||
```
|
||
|
||
It carries `failed` too, for the same reason `RemovalSummary` does. Restoring is a database swap with no sidecar involved, so it fails far less often than removing does — but a database error partway through a multi-photo restore is still a real possibility, and rethrowing it would turn a partial success into an opaque 500 that discards how far the restore got. An image that was never cut out is skipped rather than being an error either way.
|
||
|
||
### 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.
|
||
|
||
Remove and Restore are two independently-gated buttons, not two labels for one button — there is no second flag and no stored state, the images already say which they are, but a mixed item genuinely needs both offered at once.
|
||
|
||
**Remove backgrounds** is rendered when the server reports the feature configured *and* at least one image is not yet cut out. An unconfigured environment shows no Remove button rather than one that reports zero of four done every time.
|
||
|
||
**Restore originals** is rendered whenever at least one image on the item already carries an `original_image_path` — regardless of whether the feature is currently configured. That is deliberate, not an oversight: the mixed state a partial removal leaves behind is not hypothetical, and neither is `REMBG_URL` being unset after some photos were already cut out. Either way, gating Restore on `backgroundRemoval` would strand those cut-out photos with no way back. On a partly cut-out item both buttons appear together, and that is correct — Remove finishes the job on what is left, Restore undoes what is already done.
|
||
|
||
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 Remove button. Restore is still offered, and still works, whenever an image is already cut out. The remove-backgrounds 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.
|