From 43a1bfef69197bd2bb1fde5349b9afb3efc6a44a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:03:42 -0500 Subject: [PATCH 1/9] 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. From f42ea70d88a30374e8065e8da06cc756c387aaae Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:08:23 -0500 Subject: [PATCH 2/9] docs(admin): plan background removal in the inventory editor (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tasks: the two per-item functions, the endpoints plus a small admin config route, and the button. The config route is the piece the spec did not anticipate. The inventory screen has no way to learn the feature is configured — GET /api/admin/item-drafts carries that flag for the review queue, but GET /api/admin/items answers a bare array with several consumers, and reshaping it for one boolean is the worse trade. routes/adminVersion.ts is the precedent for exactly this: a small admin-only GET, deliberately not folded into the public /api/config, with the reason written down beside it. Two decisions the plan pins that the spec left as prose. The render condition is "configured OR every photo already cut out", not the flag alone, because gating on the flag would hide Restore originals the moment REMBG_URL is unset and strand cut-out photos with no way back — the same shape DraftQueue already uses for the same reason. And the button re-reads the item afterwards, because the editor is a modal changing files on the server while it is open, and without that the thumbnails keep showing the previous files and the button looks inert. Self-review caught the mistake I have now made three times this session, which is naming something that does not exist. Task 3's end-to-end case called createItem(page, { withImage: true }); createItem actually takes an APIRequestContext rather than a page, and CreateItemOptions has no image field at all. Since the control only renders for an item that has photos, the item now gets seeded through the admin API with a real PNG attached, which is a thing that works rather than a thing that reads well. It also records a spec requirement that is deliberately not implemented as written. The spec asks for an end-to-end assertion that the control is absent when the feature is unconfigured; that would mean restarting the backend mid-suite, which the run has no way to do and should not gain one. It is covered where it can be, in the integration test for GET /api/admin/config, and the plan says so rather than dropping it quietly. Co-Authored-By: Claude Opus 5 --- ...2026-09-04-inventory-background-removal.md | 940 ++++++++++++++++++ 1 file changed, 940 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-04-inventory-background-removal.md diff --git a/docs/superpowers/plans/2026-09-04-inventory-background-removal.md b/docs/superpowers/plans/2026-09-04-inventory-background-removal.md new file mode 100644 index 0000000..0dcc864 --- /dev/null +++ b/docs/superpowers/plans/2026-09-04-inventory-background-removal.md @@ -0,0 +1,940 @@ +# Inventory Background Removal Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let an admin remove or restore the backgrounds of every photo on an item, from the inventory editor where those photos are actually managed. + +**Architecture:** Two per-item functions in the module the worker and the review queue already share, two admin endpoints that always answer 200 with a summary once the id is valid, and one button in the item editor whose label is derived from the images themselves. + +**Tech Stack:** Express 4 + TypeScript, `pg`, Jest + supertest, React + antd (`antd/es/...` deep imports), Playwright. + +**Spec:** `docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md` +**Issue:** #293 + +## Global Constraints + +- **The action is per item, not per photo.** An upload is one item; its photos are views of one thing. +- **Every status.** No carve-out for `sold` or `reserved`. Do not add a status check. +- **These two routes always answer 200 once the id is valid.** They act on several images, so "did it work" has no single answer — the summary is the result. Non-200 is only for not being able to try: an unreadable or absent id, which is 404. This is a deliberate departure from #281's per-photo endpoints, which act on one image and can honestly answer 502. +- **Nothing deletes anything**, and nothing is left in a state a retry cannot resolve. `removeImageBackground` is already idempotent — a photo that already has an `original_image_path` is skipped — so a second attempt resumes rather than double-cutting. +- **`removeBackgroundsForItem` keeps stopping at the first failure.** Pushing through six photos against a sidecar that is not answering helps nobody. +- **Do not lift `DraftPhoto`.** The review queue's control is per photo and this one is per item; sharing the component would force one to pretend to be the other. The reuse is `removeImageBackground` / `restoreImageOriginal` underneath. +- **antd imports are deep and from `es`**: `import Popconfirm from 'antd/es/popconfirm';`. Never `import { Button } from 'antd'`. +- **Verify the frontend with `npm run build`, never a bare `npx tsc --noEmit`** — the app tsconfig excludes `tests/`, and a green bare `tsc` once broke a deploy here. +- **Set Node 20 for every command**: `export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH"`. The shell defaults to 18.16.1, which the backend does not support and Playwright refuses outright. Do NOT run any nvm script. +- **Do NOT run `scripts/start-local.ps1`, `scripts/run-tests.ps1`, or any PowerShell script.** They prompt for UAC elevation. Playwright needs the stack running, which only the user can start. +- Integration tests need the database: `cd backend && npm run db:test:up`. +- **Branch:** `feature/293-remove-backgrounds-from-inventory`, already created off `main` and already carrying the spec commit. Subjects end `(#293)`. **Commit bodies are not hard-wrapped** — one long line per paragraph, blank lines between. Write each to a temp file, `git commit -F `, delete it. Do not push. +- End every commit message with: + `Co-Authored-By: Claude Opus 5 ` + +--- + +### Task 1: The two per-item functions + +**Files:** +- Modify: `backend/src/intake/backgroundRemoval.ts` +- Test: `backend/tests/integration/backgroundRemoval.integration.test.ts` + +**Interfaces:** +- Consumes: `removeImageBackground(imageId)`, `restoreImageOriginal(imageId)`, `NoOriginalToRestoreError`, `isRembgConfigured()` — all already in that module. +- Produces: + - `export interface RemovalSummary { total: number; removed: number; failed: boolean }` + - `export interface RestoreSummary { total: number; restored: number }` + - `removeBackgroundsForItem(itemId: number): Promise` — was `Promise` + - `restoreOriginalsForItem(itemId: number): Promise` — new + +**Why first.** Both endpoints in Task 2 are thin wrappers over these, and this is the only change touching a file the drafting worker already depends on. + +- [ ] **Step 1: Write the failing test** + +Append to `backend/tests/integration/backgroundRemoval.integration.test.ts`. Read the file first — it already has `startStub(handler)`, `answerWithPng`, `seedSubmission(imagePath)`, `pathsOf(imageId)` and a temp `uploads` directory, all from #281. Reuse them rather than writing new ones. + +```typescript +import { + removeBackgroundsForItem, + restoreOriginalsForItem +} from '../../src/intake/backgroundRemoval'; + +describe('acting on every photo of one item', () => { + /** One item with three photos on disk, which is what an upload leaves. */ + async function seedItemWithPhotos(): Promise<{ itemId: number; imageIds: number[] }> { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Three views', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + + const imageIds: number[] = []; + for (const [index, name] of ['front.jpg', 'back.jpg', 'base.jpg'].entries()) { + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, $2, $3) RETURNING id`, + [itemId, `/uploads/${name}`, index] + ); + imageIds.push(image.rows[0]!.id); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + return { itemId, imageIds }; + } + + it('cuts out every photo and says how many', async () => { + await startStub(answerWithPng); + const { itemId } = await seedItemWithPhotos(); + + const summary = await removeBackgroundsForItem(itemId); + + expect(summary).toEqual({ total: 3, removed: 3, failed: false }); + }); + + // An item with no photos is not a failure. It is a perfectly ordinary item + // somebody has not photographed yet, and the button should say so rather + // than erroring. + it('reports nothing to do for an item with no photos', async () => { + await startStub(answerWithPng); + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Unphotographed', 'pending') RETURNING id` + ); + + expect(await removeBackgroundsForItem(rows[0]!.id)).toEqual({ + total: 0, + removed: 0, + failed: false + }); + }); + + // The case the whole summary exists for: the admin needs to know how far it + // got, because the answer decides whether pressing it again is worth it. + it('stops at the first failure and reports how far it got', async () => { + let served = 0; + await startStub((_req, res) => { + served += 1; + // The first photo works; the sidecar dies before the second. + if (served > 1) { + res.writeHead(500); + res.end('boom'); + return; + } + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); + }); + const { itemId } = await seedItemWithPhotos(); + + const summary = await removeBackgroundsForItem(itemId); + + expect(summary).toEqual({ total: 3, removed: 1, failed: true }); + }); + + // And the reason stopping early is acceptable: a retry resumes rather than + // starting over, because removeImageBackground skips what it already did. + it('a retry finishes the job without re-cutting what worked', async () => { + let served = 0; + await startStub((_req, res) => { + served += 1; + if (served === 2) { + res.writeHead(500); + res.end('boom'); + return; + } + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); + }); + const { itemId } = await seedItemWithPhotos(); + + const first = await removeBackgroundsForItem(itemId); + expect(first.failed).toBe(true); + + const second = await removeBackgroundsForItem(itemId); + + expect(second).toEqual({ total: 3, removed: 3, failed: false }); + }); + + // Unconfigured is not a failure anywhere else in this feature and is not one + // here. Nothing was attempted, so nothing went wrong. + it('does nothing, and calls it nothing, when there is no sidecar', async () => { + await startStub(answerWithPng); + const { itemId } = await seedItemWithPhotos(); + delete process.env.REMBG_URL; + + expect(await removeBackgroundsForItem(itemId)).toEqual({ + total: 3, + removed: 0, + failed: false + }); + }); +}); + +describe('putting every original back', () => { + it('restores each photo that was cut out', async () => { + await startStub(answerWithPng); + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Two views', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + for (const [index, name] of ['a.jpg', 'b.jpg'].entries()) { + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${name}`, index] + ); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + await removeBackgroundsForItem(itemId); + + const summary = await restoreOriginalsForItem(itemId); + + expect(summary).toEqual({ total: 2, restored: 2 }); + + const { rows: after } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + expect(after.map((r) => r.image_path)).toEqual(['/uploads/a.jpg', '/uploads/b.jpg']); + }); + + // A photo that was never cut out is skipped rather than being an error — the + // mixed state a partial failure leaves behind has to be restorable too. + it('skips photos that were never cut out', async () => { + await startStub(answerWithPng); + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Mixed', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + for (const [index, name] of ['x.jpg', 'y.jpg'].entries()) { + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${name}`, index] + ); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + // Cut out only the first, leaving the second as it arrived. + const { rows: images } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + await removeImageBackground(images[0]!.id); + + expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1 }); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +cd backend && npm run db:test:up && npm run test:integration -- backgroundRemoval +``` + +Expected: FAIL — `restoreOriginalsForItem` is not exported, and `removeBackgroundsForItem` resolves to `undefined` rather than a summary. + +- [ ] **Step 3: Write the implementation** + +In `backend/src/intake/backgroundRemoval.ts`, add the two result types above `removeBackgroundsForItem`: + +```typescript +/** + * What a whole-item removal actually did. + * + * `void` was enough for the drafting worker, which catches and logs and would + * not fail a draft over a background — but not for an admin standing in front + * of the screen, who needs to know whether the thing they pressed happened. + * Three of four is the normal shape of a bad day here, not an exception, and + * the count is what decides whether pressing it again is worth anything. + */ +export interface RemovalSummary { + /** How many images the item has. */ + total: number; + /** How many now carry a cut-out, including any that already did. */ + removed: number; + /** Whether it stopped early because one of them failed. */ + failed: boolean; +} + +/** + * What a whole-item restore did. + * + * No `failed`, because restoring cannot fail the way removing can: it is a + * database swap with no sidecar in it, and a photo that was never cut out is + * skipped rather than being an error. + */ +export interface RestoreSummary { + total: number; + /** How many were put back. Photos that were never cut out are not counted. */ + restored: number; +} +``` + +Then replace `removeBackgroundsForItem` entirely: + +```typescript +/** Every photo of one item, in order. */ +async function imageIdsFor(itemId: number): Promise { + const { rows } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + return rows.map((row) => row.id); +} + +/** + * Cuts out every photo of one item. + * + * Sequential rather than parallel: the sidecar is assumed to handle one request + * at a time, and neither caller is in a hurry. + * + * Stops at the first failure rather than pushing on. Six attempts against a + * sidecar that is not answering helps nobody, and stopping costs nothing + * because `removeImageBackground` skips a photo that already has an original + * recorded — so pressing the button again resumes where this stopped instead of + * starting over. The summary is what makes that retry an informed choice rather + * than a guess. + * + * An unconfigured environment is not a failure, here as everywhere else in this + * feature: nothing was attempted, so nothing went wrong. + */ +export async function removeBackgroundsForItem(itemId: number): Promise { + const imageIds = await imageIdsFor(itemId); + if (!isRembgConfigured()) { + return { total: imageIds.length, removed: 0, failed: false }; + } + + let removed = 0; + for (const imageId of imageIds) { + try { + await removeImageBackground(imageId); + removed += 1; + } catch (err) { + // Logged rather than thrown. The caller gets the count, which is the + // thing it can act on; the reason belongs in the log, because the admin's + // next move is the same whatever it was. + console.error(`[background-removal] item ${itemId}, image ${imageId}:`, err); + return { total: imageIds.length, removed, failed: true }; + } + } + return { total: imageIds.length, removed, failed: false }; +} + +/** + * Puts every cut-out photo of one item back. + * + * A photo that was never cut out is skipped rather than refused — the mixed + * state a partial removal leaves behind has to be restorable too, and half an + * item is exactly when somebody reaches for this. + */ +export async function restoreOriginalsForItem(itemId: number): Promise { + const imageIds = await imageIdsFor(itemId); + + let restored = 0; + for (const imageId of imageIds) { + try { + await restoreImageOriginal(imageId); + restored += 1; + } catch (err) { + // Only "there was nothing to restore" is skipped. Anything else is a real + // failure and belongs to the caller. + if (!(err instanceof NoOriginalToRestoreError)) throw err; + } + } + return { total: imageIds.length, restored }; +} +``` + +**Do not change `draftingWorker.ts`.** It calls `removeBackgroundsForItem(...).catch(...)` and ignores the result; ignoring a returned value is legal, which is what makes this additive. `npm run build` proves it. + +Note the behaviour change for the worker: it previously saw a rejection when a photo failed and logged it through its own `.catch`. Now the function resolves instead, and logs internally. The worker's existing test asserts the draft stays `ready` when the sidecar fails, which still holds — and holds more simply, since there is no longer a rejection to swallow. + +- [ ] **Step 4: Run the tests** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +cd backend && npm run test:integration -- backgroundRemoval draftingBackgroundRemoval && npm run build && npm run lint +``` + +Expected: PASS. `draftingBackgroundRemoval` is in the list deliberately — it is the worker's test, and it is what proves the widened return did not disturb the one existing caller. + +- [ ] **Step 5: Commit** + +```bash +git add backend/src/intake/backgroundRemoval.ts backend/tests/integration/backgroundRemoval.integration.test.ts +git commit -F- <<'EOF' +feat(intake): report what a whole-item background removal actually did (#293) + +removeBackgroundsForItem answered void and threw on the first failure, which is enough for the drafting worker — it catches and logs, and a draft is not worth failing over — and not enough for an admin standing in front of a screen who needs to know whether the thing they pressed happened. It now returns a summary: how many photos the item has, how many carry a cut-out, and whether it stopped early. + +It still stops at the first failure. Six attempts against a sidecar that is not answering helps nobody, and stopping costs nothing because removeImageBackground skips a photo that already has an original recorded, so a retry resumes rather than starting over. The count is what turns that retry into an informed choice instead of a guess. + +Adds restoreOriginalsForItem alongside it. A photo that was never cut out is skipped rather than refused, because the mixed state a partial removal leaves behind is exactly when somebody reaches for this. + +The one existing caller does not change: the worker ignores the result, and ignoring a returned value is legal, which is what makes this additive rather than breaking. + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +### Task 2: The endpoints, and telling the screen the feature exists + +**Files:** +- Modify: `backend/src/routes/admin.ts` +- Create: `backend/src/routes/adminConfig.ts` +- Modify: `backend/src/app.ts` (mount the new router beside the other admin routers) +- Test: `backend/tests/integration/adminItemBackgrounds.integration.test.ts` (create) + +**Interfaces:** +- Consumes: `removeBackgroundsForItem`, `restoreOriginalsForItem`, `RemovalSummary`, `RestoreSummary` from Task 1; `isRembgConfigured` from `../intake/rembgClient`; `readId` from `../utils`. +- Produces: + - `POST /api/admin/items/:id/remove-backgrounds` → `200 { total, removed, failed }` | `404` + - `POST /api/admin/items/:id/restore-originals` → `200 { total, restored }` | `404` + - `GET /api/admin/config` → `200 { backgroundRemoval: boolean }` + +**Why a new config route.** The Inventory screen has no way to learn the feature is configured. `GET /api/admin/item-drafts` carries the flag for the review queue, but `GET /api/admin/items` returns a bare array with several consumers, and changing its shape for one boolean is not worth it. `routes/adminVersion.ts` is the precedent: a small admin-only GET, deliberately not folded into the public `/api/config`, with the reason written down. + +- [ ] **Step 1: Write the failing test** + +Create `backend/tests/integration/adminItemBackgrounds.integration.test.ts`: + +```typescript +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +let uploads = ''; +let previousUploadsDir: string | undefined; +let stub: http.Server | null = null; + +/** + * A stub sidecar on an ephemeral port, and a temporary uploads directory. + * + * Port 0 rather than a fixed number: several ports in the 55000s are + * Hyper-V-reserved on this machine. No test here contacts a real rembg — it + * takes forty seconds to start, and a suite depending on that is broken by + * construction. + */ +async function startStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void +): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'itembg-')); + previousUploadsDir = process.env.UPLOADS_DIR; + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => handler(req, res)); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +} + +function answerWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); +} + +beforeEach(async () => { + await resetDb(); +}); + +afterEach(async () => { + delete process.env.REMBG_URL; + if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR; + else process.env.UPLOADS_DIR = previousUploadsDir; + if (uploads) await fsp.rm(uploads, { recursive: true, force: true }); + uploads = ''; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** An available item with two photos on disk. */ +async function seedItem(): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Blue vase', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + for (const [index, name] of ['front.jpg', 'back.jpg'].entries()) { + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${name}`, index] + ); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + return itemId; +} + +describe('removing every background on an item', () => { + it('answers with the summary', async () => { + await startStub(answerWithPng); + const itemId = await seedItem(); + + const res = await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ total: 2, removed: 2, failed: false }); + }); + + // Every status, with no carve-out. A sold item's photos are still the shop's + // photos, and improving them changes nothing about the sale. + it.each(['sold', 'reserved', 'pending'])('works on a %s item', async (status) => { + await startStub(answerWithPng); + const itemId = await seedItem(); + await pool.query(`UPDATE items SET status = $2 WHERE id = $1`, [itemId, status]); + + const res = await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + expect(res.status).toBe(200); + expect(res.body.removed).toBe(2); + }); + + // The departure from #281's per-photo endpoints, and the reason for it: this + // acts on several images, so "did it work" has no single answer and a 502 + // would throw away the count that makes the outcome actionable. + it('answers 200 with the count when the sidecar fails, not 502', async () => { + await startStub((_req, res) => { + res.writeHead(500); + res.end('boom'); + }); + const itemId = await seedItem(); + + const res = await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ total: 2, removed: 0, failed: true }); + }); + + it('answers 404 for an id that cannot be read', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/abc/remove-backgrounds')).status).toBe(404); + }); + + it('answers 404 for an item that does not exist', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/999999/remove-backgrounds')).status).toBe(404); + }); +}); + +describe('restoring every original on an item', () => { + it('puts them back and says how many', async () => { + await startStub(answerWithPng); + const itemId = await seedItem(); + await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + const res = await request(app).post(`/api/admin/items/${itemId}/restore-originals`); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ total: 2, restored: 2 }); + }); + + it('answers 404 for an id that cannot be read', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/abc/restore-originals')).status).toBe(404); + }); +}); + +describe('what the admin screen is told about the feature', () => { + it('says it is on when a sidecar is configured', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + + const res = await request(app).get('/api/admin/config'); + + expect(res.status).toBe(200); + expect(res.body.backgroundRemoval).toBe(true); + }); + + it('says it is off when none is', async () => { + delete process.env.REMBG_URL; + + const res = await request(app).get('/api/admin/config'); + + expect(res.body.backgroundRemoval).toBe(false); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +cd backend && npm run test:integration -- adminItemBackgrounds +``` + +Expected: FAIL — all three routes answer 404 because none exist. + +- [ ] **Step 3: Write the config route** + +Create `backend/src/routes/adminConfig.ts`: + +```typescript +import { Router, Request, Response } from 'express'; +import { isRembgConfigured } from '../intake/rembgClient'; + +const router = Router(); + +/** + * What the admin screens can offer in this environment. + * + * Behind `requireAdminGate` like every other admin router, and deliberately not + * folded into `/api/config` — the same reasoning `adminVersion.ts` records. + * That endpoint is public and the storefront fetches it on every load; nothing + * here is any of a customer's business. + * + * It exists because the inventory screen has no other way to learn this. + * `GET /api/admin/item-drafts` carries the flag for the review queue, but + * `GET /api/admin/items` answers a bare array with several consumers, and + * changing its shape for one boolean would be a worse trade than one small + * route. + * + * Not wrapped in `asyncRoute` because the handler is synchronous: it reads an + * environment variable, so there is no promise to reject. + */ +router.get('/', (_req: Request, res: Response) => { + res.json({ backgroundRemoval: isRembgConfigured() }); +}); + +export default router; +``` + +Mount it in `backend/src/app.ts` beside the other admin routers. Read how they are mounted first — they go through `requireAdminGate`, and the new one must too. Follow whatever that file already does for `adminVersion`. + +- [ ] **Step 4: Write the two item routes** + +In `backend/src/routes/admin.ts`, add the imports: + +```typescript +import { removeBackgroundsForItem, restoreOriginalsForItem } from '../intake/backgroundRemoval'; +``` + +Add both routes before `export default router;`: + +```typescript +/** + * One item's images, or null when the item does not exist. + * + * Checked before acting so an absent item is a 404 rather than a cheerful + * summary of nothing. `removeBackgroundsForItem` would happily report + * `total: 0` for an id that was never an item, which is true and useless. + */ +async function itemExists(itemId: number): Promise { + const { rows } = await pool.query(`SELECT 1 FROM items WHERE id = $1`, [itemId]); + return rows.length > 0; +} + +/** + * Remove the background from every photo of one item. + * + * Per item rather than per photo because an upload is one item: the front, the + * back and the chipped base are three views of one thing, not three things to + * cut out separately (#293). + * + * Answers 200 once the id is valid, even when the sidecar fails. Unlike the + * per-photo endpoints in #281, this acts on several images, so "did it work" + * has no single answer — two of four is the normal shape of a bad day here. + * A 502 would throw away the count, which is the only thing that makes the + * outcome actionable. Non-200 is reserved for not being able to try at all. + * + * No status check. A sold item's photos are still the shop's photos, and + * improving them changes nothing about the sale — the guards on `unpublish` + * protect a checkout in progress and a completed sale, neither of which is at + * stake in a photograph's background. + */ +router.post('/items/:id/remove-backgrounds', asyncRoute(async (req: Request, res: Response) => { + const itemId = readId(req.params.id); + if (itemId === null || !(await itemExists(itemId))) { + return res.status(404).json({ error: 'not found' }); + } + + res.json(await removeBackgroundsForItem(itemId)); +})); + +/** + * Put every original back. + * + * The reason removing is safe to try. Photos that were never cut out are + * skipped rather than refused, so a half-done item — what a partial failure + * leaves behind — is restorable too. + */ +router.post('/items/:id/restore-originals', asyncRoute(async (req: Request, res: Response) => { + const itemId = readId(req.params.id); + if (itemId === null || !(await itemExists(itemId))) { + return res.status(404).json({ error: 'not found' }); + } + + res.json(await restoreOriginalsForItem(itemId)); +})); +``` + +- [ ] **Step 5: Run the tests** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +cd backend && npm run test:integration -- adminItemBackgrounds && npx jest -c jest.unit.config.js tests/unit/routesAreWrapped.test.ts && npm run test:unit && npm run test:integration && npm run build && npm run lint +``` + +Expected: PASS throughout. The wrapper guard confirms no handler was added unwrapped; the full suites catch anything the new router disturbed. + +- [ ] **Step 6: Commit** + +```bash +git add backend/src/routes/admin.ts backend/src/routes/adminConfig.ts backend/src/app.ts backend/tests/integration/adminItemBackgrounds.integration.test.ts +git commit -F- <<'EOF' +feat(admin): remove or restore every background on an item (#293) + +Two routes on the item, and a small admin config route so the inventory screen can know whether to offer them. + +Both answer 200 once the id is valid, even when the sidecar fails, 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. These act on several, so "did it work" has no single answer — two of four is the normal shape of a bad day here, not an exception — and a 502 would throw away the count that is the only thing making the outcome actionable. Non-200 is reserved for not being able to try at all, which here means an unreadable or absent id. + +No status check on either. A sold item's photos are still the shop's photos and improving them changes nothing about the sale; the guards on unpublish protect a checkout in progress and a completed sale, neither of which is at stake in a photograph's background. + +The config route follows adminVersion's precedent rather than extending the public /api/config: admin-only, one purpose, and the reason written down. The inventory screen had no other way to learn the feature exists, because GET /api/admin/items answers a bare array with several consumers and reshaping it for one boolean is the worse trade. + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +### Task 3: The button in the item editor + +**Files:** +- Modify: `frontend/src/admin/Admin.tsx` +- Test: `frontend/tests/e2e/admin-item-backgrounds.spec.ts` (create) + +**Interfaces:** +- Consumes: the three endpoints from Task 2. +- Produces: nothing later depends on. + +**What is already there.** `Admin.tsx:57` holds `const [editingItem, setEditingItem] = useState(null)`. `handleDeleteImage(itemId, imageId)` at :181 is the model to follow — it calls the API and then updates the open modal with `setEditingItem(prev => prev && prev.id === itemId ? ... : prev)` at :190. Read both before writing anything; the modal-refresh pattern is the part most likely to go wrong. + +- [ ] **Step 1: Learn the feature flag on mount** + +In `Admin.tsx`, add state beside the others: + +```typescript + // Whether this environment has a background-removal sidecar. False hides the + // control rather than offering one that would report zero of four done every + // time — an unconfigured environment is a working one, not a broken one. + const [backgroundRemoval, setBackgroundRemoval] = useState(false); +``` + +Fetch it wherever the screen already loads its reference data — find the existing `useEffect` that loads categories or tags and add it there rather than adding a second effect: + +```typescript + fetch('/api/admin/config') + .then((res) => (res.ok ? res.json() : { backgroundRemoval: false })) + .then((config) => setBackgroundRemoval(config.backgroundRemoval)) + .catch(() => setBackgroundRemoval(false)); +``` + +- [ ] **Step 2: Add the action** + +Beside `handleDeleteImage`: + +```typescript + /** + * Cut out every photo of the item being edited, or put every original back. + * + * Per item, not per photo: an upload is one item, and its photos are views of + * one thing (#293). + * + * The response replaces the open modal's images rather than being trusted to + * have changed nothing else. The editor is a modal and this changes files on + * the server while it is open, so without a refresh the thumbnails keep + * showing the previous files and the button looks like it did nothing. + */ + async function handleBackgrounds(itemId: number, action: 'remove-backgrounds' | 'restore-originals') { + setBusyBackgrounds(true); + try { + const res = await fetch(`/api/admin/items/${itemId}/${action}`, { method: 'POST' }); + if (!res.ok) { + message.error('That did not work.'); + return; + } + const summary = await res.json(); + + if (action === 'remove-backgrounds' && summary.failed) { + // Said plainly rather than as a generic failure. How far it got is what + // decides whether pressing it again is worth anything, and it is — + // a retry skips the ones that already worked. + message.warning(`${summary.removed} of ${summary.total} photos done. Try again to finish.`); + } else { + message.success('Done.'); + } + + // Re-read the item so the thumbnails match what is now on the server. + const fresh = await fetch('/api/admin/items'); + if (fresh.ok) { + const all: Item[] = await fresh.json(); + const updated = all.find((candidate) => candidate.id === itemId); + if (updated) setEditingItem(prev => (prev && prev.id === itemId ? updated : prev)); + setItems(all); + } + } finally { + setBusyBackgrounds(false); + } + } +``` + +Add `const [busyBackgrounds, setBusyBackgrounds] = useState(false);` beside the other state. Check the real names of the items-list state setter and the `message` import before using them — `setItems` and `message` are the expected ones, but use what the file actually has. + +- [ ] **Step 3: Render the button** + +Inside the existing "Existing Images" `Form.Item`, after the `` that maps the thumbnails: + +```tsx + {(backgroundRemoval || editingItem.images.every(img => img.original_image_path !== null)) && ( +
+ +
+ )} +``` + +Two things about that condition, both deliberate: + +**The label's `otherwise` covers the mixed state**, which is exactly what a partial failure leaves behind. With two of four cut out it reads **Remove backgrounds**, which is the action that finishes the job — and pressing it skips the two that already worked. Offering to restore at that point would be offering the wrong half. + +**Rendering is gated on `backgroundRemoval || everything is already cut out`**, not on `backgroundRemoval` alone — the same shape `DraftQueue.tsx:161` uses. Gating on the flag alone would hide **Restore originals** the moment `REMBG_URL` is unset, stranding cut-out photos with no way back. + +The item type needs `original_image_path` on its images. Check whether `Item`'s image type in `Admin.tsx` already has it — `ADMIN_ITEM_SELECT` returns `i.*` plus an images aggregate, so confirm what that aggregate actually contains before assuming, and extend the frontend type to match rather than the other way round. + +- [ ] **Step 4: Verify the frontend** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +cd frontend && npm run build && npm run lint && npm run test:unit +``` + +`npm run build`, not a bare `npx tsc --noEmit` — the app tsconfig excludes `tests/`. + +Expected: clean build, clean lint, unit tests pass. + +- [ ] **Step 5: Write the e2e case** + +Create `frontend/tests/e2e/admin-item-backgrounds.spec.ts`. + +**`createItem` cannot do this.** Its signature is `createItem(api: APIRequestContext, options)` — an API context, not a page — and `CreateItemOptions` has no image field at all: it posts name, description, price, category and tags, and never attaches a file. The button only renders for an item that has images, so the item has to be seeded with one directly. The route reads a multipart body and accepts an `images` field, which is what makes that possible. + +```typescript +import { test, expect, createAdminContext, uniqueSuffix } from './fixtures'; + +/** + * The per-item background control in the inventory editor (#293). + * + * Asserts the control is offered, not that a cut-out happens. Pressing it needs + * a live rembg sidecar, which takes forty seconds to start and which no test + * should depend on — the swap itself is covered in the integration suite + * against a stub. + */ +const RUN = uniqueSuffix(); +const NAME = `Vase ${RUN}`; + +// The 1x1 PNG the other upload specs use, so this goes through the real +// validated upload path rather than a buffer that merely starts correctly. +const PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==', + 'base64' +); + +test.beforeAll(async ({ playwright }) => { + const api = await createAdminContext(playwright); + // Seeded with an image directly: createItem cannot attach one, and the + // control does not render for an item with no photos. + const res = await api.post('/api/admin/items', { + multipart: { + name: NAME, + description: '', + price: '50', + category_id: '', + tags: '[]', + images: { name: `${RUN}.png`, mimeType: 'image/png', buffer: PNG } + } + }); + expect(res.ok(), `seeding ${NAME}`).toBeTruthy(); + await api.dispose(); +}); + +test.describe('Removing backgrounds from the item editor', () => { + test('offers the control on an item that has photos', async ({ page, admin }) => { + await admin.open('Inventory'); + + await page.getByRole('row', { name: new RegExp(NAME) }).getByRole('button', { name: 'Edit' }).click(); + + await expect(page.getByRole('button', { name: 'Remove backgrounds' })).toBeVisible(); + }); +}); +``` + +Read `admin-item-preview.spec.ts` before writing this: the Inventory tab name passed to `admin.open`, and the accessible name of the row's edit affordance, must match what is actually there. Both are guesses in the snippet above until you have checked them. + +**The spec also asks for "and does not when the feature is unconfigured", and that is not written as an e2e test.** It cannot be: unsetting `REMBG_URL` means restarting the backend mid-suite, which the e2e run has no way to do and should not gain one. The behaviour is covered where it can be — `GET /api/admin/config` answering `false` is asserted in Task 2's integration tests, and the render condition on it is plain enough to read. Say so in your report rather than quietly dropping it. + +- [ ] **Step 6: Run the e2e** + +The local stack must be running and **only the user can start it** — do not run `start-local.ps1`. If the stack is down, say so plainly in your report and leave the spec unrun. + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +cd frontend && npx playwright test admin-item-backgrounds --project=chromium +``` + +Then the whole suite, which is what catches anything the new fetch on mount disturbed: + +```bash +cd frontend && npx playwright test --project=chromium +``` + +- [ ] **Step 7: Commit** + +```bash +git add frontend/src/admin/Admin.tsx frontend/tests/e2e/admin-item-backgrounds.spec.ts +git commit -F- <<'EOF' +feat(admin): offer background removal where an item's photos are edited (#293) + +One button per item in the inventory editor, beside the per-thumbnail delete buttons rather than on them, because an upload is one item and its photos are views of one thing. + +Its label is derived from the images rather than stored: Restore originals when every photo already carries an original, Remove backgrounds otherwise. The otherwise deliberately covers the mixed state a partial failure leaves behind — with two of four cut out it reads Remove backgrounds, which is the action that finishes the job, and pressing it skips the two that already worked. + +Rendering is gated on the feature being configured or every photo already being cut out, not on the flag alone. Gating on the flag would hide Restore originals the moment REMBG_URL is unset, stranding cut-out photos with no way back — the same reasoning the review queue's control already uses. + +The editor is a modal and this changes files on the server while it is open, so the item is re-read afterwards and the open modal updated. Without that the thumbnails keep showing the previous files and the button looks like it did nothing, which is the bug this was most likely to ship with. + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +## After the plan + +- The branch is `feature/293-remove-backgrounds-from-inventory`. **Do not push** — the user pushes and merges. +- The PR closes #293 and should say plainly that this applies to live product images and to every status, both deliberate. +- Per standing practice, follow with a SonarQube cleanup pass — never folded into this branch. From 6f3e77fa88110b5c28895041dcbe4af2fdd1e94a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:16:04 -0500 Subject: [PATCH 3/9] feat(intake): report what a whole-item background removal actually did (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit removeBackgroundsForItem answered void and threw on the first failure, which is enough for the drafting worker — it catches and logs, and a draft is not worth failing over — and not enough for an admin standing in front of a screen who needs to know whether the thing they pressed happened. It now returns a summary: how many photos the item has, how many carry a cut-out, and whether it stopped early. It still stops at the first failure. Six attempts against a sidecar that is not answering helps nobody, and stopping costs nothing because removeImageBackground skips a photo that already has an original recorded, so a retry resumes rather than starting over. The count is what turns that retry into an informed choice instead of a guess. Adds restoreOriginalsForItem alongside it. A photo that was never cut out is skipped rather than refused, because the mixed state a partial removal leaves behind is exactly when somebody reaches for this. The one existing caller does not change: the worker ignores the result, and ignoring a returned value is legal, which is what makes this additive rather than breaking. Co-Authored-By: Claude Opus 5 --- backend/src/intake/backgroundRemoval.ts | 100 ++++++++++- .../backgroundRemoval.integration.test.ts | 167 +++++++++++++++++- 2 files changed, 257 insertions(+), 10 deletions(-) diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index 31bfa11..2659d68 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -169,21 +169,103 @@ export async function restoreImageOriginal(imageId: number): Promise { } /** - * Every photo of one item, in order. + * What a whole-item removal actually did. * - * Sequential rather than parallel: the sidecar is assumed to handle one - * request at a time, and the worker it runs inside is not in a hurry. A - * failure on one photo stops the rest, and the caller logs it — the item keeps - * whatever was already done, and nothing is left half-written. + * `void` was enough for the drafting worker, which catches and logs and would + * not fail a draft over a background — but not for an admin standing in front + * of the screen, who needs to know whether the thing they pressed happened. + * Three of four is the normal shape of a bad day here, not an exception, and + * the count is what decides whether pressing it again is worth anything. */ -export async function removeBackgroundsForItem(itemId: number): Promise { - if (!isRembgConfigured()) return; +export interface RemovalSummary { + /** How many images the item has. */ + total: number; + /** How many now carry a cut-out, including any that already did. */ + removed: number; + /** Whether it stopped early because one of them failed. */ + failed: boolean; +} +/** + * What a whole-item restore did. + * + * No `failed`, because restoring cannot fail the way removing can: it is a + * database swap with no sidecar in it, and a photo that was never cut out is + * skipped rather than being an error. + */ +export interface RestoreSummary { + total: number; + /** How many were put back. Photos that were never cut out are not counted. */ + restored: number; +} + +/** Every photo of one item, in order. */ +async function imageIdsFor(itemId: number): Promise { const { rows } = await pool.query<{ id: number }>( `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, [itemId] ); - for (const row of rows) { - await removeImageBackground(row.id); + return rows.map((row) => row.id); +} + +/** + * Cuts out every photo of one item. + * + * Sequential rather than parallel: the sidecar is assumed to handle one request + * at a time, and neither caller is in a hurry. + * + * Stops at the first failure rather than pushing on. Six attempts against a + * sidecar that is not answering helps nobody, and stopping costs nothing + * because `removeImageBackground` skips a photo that already has an original + * recorded — so pressing the button again resumes where this stopped instead of + * starting over. The summary is what makes that retry an informed choice rather + * than a guess. + * + * An unconfigured environment is not a failure, here as everywhere else in this + * feature: nothing was attempted, so nothing went wrong. + */ +export async function removeBackgroundsForItem(itemId: number): Promise { + const imageIds = await imageIdsFor(itemId); + if (!isRembgConfigured()) { + return { total: imageIds.length, removed: 0, failed: false }; } + + let removed = 0; + for (const imageId of imageIds) { + try { + await removeImageBackground(imageId); + removed += 1; + } catch (err) { + // Logged rather than thrown. The caller gets the count, which is the + // thing it can act on; the reason belongs in the log, because the admin's + // next move is the same whatever it was. + console.error(`[background-removal] item ${itemId}, image ${imageId}:`, err); + return { total: imageIds.length, removed, failed: true }; + } + } + return { total: imageIds.length, removed, failed: false }; +} + +/** + * Puts every cut-out photo of one item back. + * + * A photo that was never cut out is skipped rather than refused — the mixed + * state a partial removal leaves behind has to be restorable too, and half an + * item is exactly when somebody reaches for this. + */ +export async function restoreOriginalsForItem(itemId: number): Promise { + const imageIds = await imageIdsFor(itemId); + + let restored = 0; + for (const imageId of imageIds) { + try { + await restoreImageOriginal(imageId); + restored += 1; + } catch (err) { + // Only "there was nothing to restore" is skipped. Anything else is a real + // failure and belongs to the caller. + if (!(err instanceof NoOriginalToRestoreError)) throw err; + } + } + return { total: imageIds.length, restored }; } diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts index a0eba9c..4d3ffd1 100644 --- a/backend/tests/integration/backgroundRemoval.integration.test.ts +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -5,7 +5,12 @@ import os from 'os'; import path from 'path'; import { pool } from '../../src/db'; import { resetDb, closeDb } from './setup/testDb'; -import { removeImageBackground, restoreImageOriginal } from '../../src/intake/backgroundRemoval'; +import { + removeImageBackground, + restoreImageOriginal, + removeBackgroundsForItem, + restoreOriginalsForItem +} from '../../src/intake/backgroundRemoval'; beforeEach(async () => { await resetDb(); @@ -250,3 +255,163 @@ describe('putting the original back', () => { expect((await pathsOf(imageId)).image_path).toBe('/uploads/original.jpg'); }); }); + +describe('acting on every photo of one item', () => { + /** One item with three photos on disk, which is what an upload leaves. */ + async function seedItemWithPhotos(): Promise<{ itemId: number; imageIds: number[] }> { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Three views', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + + const imageIds: number[] = []; + for (const [index, name] of ['front.jpg', 'back.jpg', 'base.jpg'].entries()) { + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, $2, $3) RETURNING id`, + [itemId, `/uploads/${name}`, index] + ); + imageIds.push(image.rows[0]!.id); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + return { itemId, imageIds }; + } + + it('cuts out every photo and says how many', async () => { + await startStub(answerWithPng); + const { itemId } = await seedItemWithPhotos(); + + const summary = await removeBackgroundsForItem(itemId); + + expect(summary).toEqual({ total: 3, removed: 3, failed: false }); + }); + + // An item with no photos is not a failure. It is a perfectly ordinary item + // somebody has not photographed yet, and the button should say so rather + // than erroring. + it('reports nothing to do for an item with no photos', async () => { + await startStub(answerWithPng); + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Unphotographed', 'pending') RETURNING id` + ); + + expect(await removeBackgroundsForItem(rows[0]!.id)).toEqual({ + total: 0, + removed: 0, + failed: false + }); + }); + + // The case the whole summary exists for: the admin needs to know how far it + // got, because the answer decides whether pressing it again is worth it. + it('stops at the first failure and reports how far it got', async () => { + let served = 0; + await startStub((_req, res) => { + served += 1; + // The first photo works; the sidecar dies before the second. + if (served > 1) { + res.writeHead(500); + res.end('boom'); + return; + } + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); + }); + const { itemId } = await seedItemWithPhotos(); + + const summary = await removeBackgroundsForItem(itemId); + + expect(summary).toEqual({ total: 3, removed: 1, failed: true }); + }); + + // And the reason stopping early is acceptable: a retry resumes rather than + // starting over, because removeImageBackground skips what it already did. + it('a retry finishes the job without re-cutting what worked', async () => { + let served = 0; + await startStub((_req, res) => { + served += 1; + if (served === 2) { + res.writeHead(500); + res.end('boom'); + return; + } + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); + }); + const { itemId } = await seedItemWithPhotos(); + + const first = await removeBackgroundsForItem(itemId); + expect(first.failed).toBe(true); + + const second = await removeBackgroundsForItem(itemId); + + expect(second).toEqual({ total: 3, removed: 3, failed: false }); + }); + + // Unconfigured is not a failure anywhere else in this feature and is not one + // here. Nothing was attempted, so nothing went wrong. + it('does nothing, and calls it nothing, when there is no sidecar', async () => { + await startStub(answerWithPng); + const { itemId } = await seedItemWithPhotos(); + delete process.env.REMBG_URL; + + expect(await removeBackgroundsForItem(itemId)).toEqual({ + total: 3, + removed: 0, + failed: false + }); + }); +}); + +describe('putting every original back', () => { + it('restores each photo that was cut out', async () => { + await startStub(answerWithPng); + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Two views', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + for (const [index, name] of ['a.jpg', 'b.jpg'].entries()) { + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${name}`, index] + ); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + await removeBackgroundsForItem(itemId); + + const summary = await restoreOriginalsForItem(itemId); + + expect(summary).toEqual({ total: 2, restored: 2 }); + + const { rows: after } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + expect(after.map((r) => r.image_path)).toEqual(['/uploads/a.jpg', '/uploads/b.jpg']); + }); + + // A photo that was never cut out is skipped rather than being an error — the + // mixed state a partial failure leaves behind has to be restorable too. + it('skips photos that were never cut out', async () => { + await startStub(answerWithPng); + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Mixed', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + for (const [index, name] of ['x.jpg', 'y.jpg'].entries()) { + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${name}`, index] + ); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + // Cut out only the first, leaving the second as it arrived. + const { rows: images } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + await removeImageBackground(images[0]!.id); + + expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1 }); + }); +}); From 8f35204995479f32da9512d5a3ffd015e20ac25c Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:26:19 -0500 Subject: [PATCH 4/9] feat(admin): remove or restore every background on an item (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two routes on the item, and a small admin config route so the inventory screen can know whether to offer them. Both answer 200 once the id is valid, even when the sidecar fails, 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. These act on several, so "did it work" has no single answer — two of four is the normal shape of a bad day here, not an exception — and a 502 would throw away the count that is the only thing making the outcome actionable. Non-200 is reserved for not being able to try at all, which here means an unreadable or absent id. No status check on either. A sold item's photos are still the shop's photos and improving them changes nothing about the sale; the guards on unpublish protect a checkout in progress and a completed sale, neither of which is at stake in a photograph's background. The config route follows adminVersion's precedent rather than extending the public /api/config: admin-only, one purpose, and the reason written down. The inventory screen had no other way to learn the feature exists, because GET /api/admin/items answers a bare array with several consumers and reshaping it for one boolean is the worse trade. Also extends the admin item select to carry original_image_path on each image, behind a new ADMIN_IMAGES_SUBQUERY kept separate from the shared IMAGES_SUBQUERY the public select uses. Task 3 needs to derive its restore-button label from that field, and the server never sent it for items before this — only the drafts endpoint carried it, added by #281 for the review queue. It stays admin-only for the same reason itemSelect.ts already names PUBLIC_ITEM_SELECT's columns explicitly: an internal original filename is nobody's business on the storefront, and sharing one subquery would put it in every public item response. Co-Authored-By: Claude Opus 5 --- backend/src/app.ts | 2 + backend/src/itemSelect.ts | 36 +++- backend/src/routes/admin.ts | 56 +++++ backend/src/routes/adminConfig.ts | 27 +++ .../adminItemBackgrounds.integration.test.ts | 193 ++++++++++++++++++ 5 files changed, 312 insertions(+), 2 deletions(-) create mode 100644 backend/src/routes/adminConfig.ts create mode 100644 backend/tests/integration/adminItemBackgrounds.integration.test.ts diff --git a/backend/src/app.ts b/backend/src/app.ts index b0177dd..e4acc46 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -14,6 +14,7 @@ import adminItemDraftsRouter from './routes/adminItemDrafts'; import intakeActionsRouter from './routes/intakeActions'; import intakeRouter from './routes/intake'; import adminVersionRouter from './routes/adminVersion'; +import adminConfigRouter from './routes/adminConfig'; import filtersRouter from './routes/filters'; import customersRouter from './routes/customers'; import publicRouter from './routes/public'; @@ -84,6 +85,7 @@ app.use('/api/admin/tags', requireAdminGate, adminTagsRouter); app.use('/api/admin/upload-links', requireAdminGate, adminUploadLinksRouter); app.use('/api/admin/item-drafts', requireAdminGate, adminItemDraftsRouter); app.use('/api/admin/version', requireAdminGate, adminVersionRouter); +app.use('/api/admin/config', requireAdminGate, adminConfigRouter); app.use('/api/admin', requireAdminGate, adminRouter); app.use('/api/customers/me/addresses', shippingAddressesRouter); app.use('/api/customers', customersRouter); diff --git a/backend/src/itemSelect.ts b/backend/src/itemSelect.ts index 28afc76..4ef8a7a 100644 --- a/backend/src/itemSelect.ts +++ b/backend/src/itemSelect.ts @@ -30,6 +30,26 @@ const IMAGES_SUBQUERY = ` WHERE img.item_id = i.id ), '[]') AS images`; +/** + * Admin-only images, carrying `original_image_path` alongside the public + * fields — the field the inventory screen needs to know whether a photo has a + * cut-out to restore (#293). + * + * A separate subquery rather than adding the column to `IMAGES_SUBQUERY` + * itself, for the same reason `PUBLIC_ITEM_SELECT` names its columns instead + * of using `i.*`: an original filename is internal — nobody's business on the + * storefront — and folding it into the one subquery both selects share would + * put it in every public item response too. + */ +const ADMIN_IMAGES_SUBQUERY = ` + COALESCE(( + SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order, + 'original_image_path', img.original_image_path) + ORDER BY img.sort_order) + FROM item_images img + WHERE img.item_id = i.id + ), '[]') AS images`; + const TAGS_SUBQUERY = ` COALESCE(( SELECT json_agg(json_build_object('id', t.id, 'name', t.name, 'color', t.color) ORDER BY t.name) @@ -54,7 +74,7 @@ export const PUBLIC_ITEM_SELECT = ` export const ADMIN_ITEM_SELECT = ` SELECT i.*, c.name AS category_name, - ${IMAGES_SUBQUERY}, + ${ADMIN_IMAGES_SUBQUERY}, ${TAGS_SUBQUERY} ${FROM_CLAUSE}`; @@ -89,16 +109,28 @@ interface ItemRowBase { /** What PUBLIC_ITEM_SELECT returns. Deliberately no payment or reservation columns. */ export type PublicItemRow = ItemRowBase; +/** + * An admin item's image: everything `ItemImage` has, plus where the + * background-removed photo's original went. `null` for a photo that was never + * cut out. + */ +export interface AdminItemImage extends ItemImage { + original_image_path: string | null; +} + /** * What ADMIN_ITEM_SELECT returns: `i.*`, so every column on the table. * * The extra fields are the ones the storefront is not allowed to see, which is - * the whole reason the two selects differ. + * the whole reason the two selects differ. `images` is narrowed rather than + * inherited as-is, to match `ADMIN_IMAGES_SUBQUERY` carrying + * `original_image_path` where the public select's images do not. */ export interface AdminItemRow extends ItemRowBase { reserved_until: Date | null; sold_at: Date | null; paypal_order_id: string | null; + images: AdminItemImage[]; } /** diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index e522607..6e6c1cd 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -7,6 +7,7 @@ import { asyncRoute } from '../asyncRoute'; import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; import { readId, tagColorFor } from '../utils'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; +import { removeBackgroundsForItem, restoreOriginalsForItem } from '../intake/backgroundRemoval'; // The upload pipeline moved to src/imageUpload.ts when #222's public intake // endpoint became a second caller. Mounting uploadImages gets the type // allowlist, the magic-byte check, and the EXIF-stripping re-encode together — @@ -350,4 +351,59 @@ router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Re res.json(available); })); +/** + * One item's images, or null when the item does not exist. + * + * Checked before acting so an absent item is a 404 rather than a cheerful + * summary of nothing. `removeBackgroundsForItem` would happily report + * `total: 0` for an id that was never an item, which is true and useless. + */ +async function itemExists(itemId: number): Promise { + const { rows } = await pool.query(`SELECT 1 FROM items WHERE id = $1`, [itemId]); + return rows.length > 0; +} + +/** + * Remove the background from every photo of one item. + * + * Per item rather than per photo because an upload is one item: the front, the + * back and the chipped base are three views of one thing, not three things to + * cut out separately (#293). + * + * Answers 200 once the id is valid, even when the sidecar fails. Unlike the + * per-photo endpoints in #281, this acts on several images, so "did it work" + * has no single answer — two of four is the normal shape of a bad day here. + * A 502 would throw away the count, which is the only thing that makes the + * outcome actionable. Non-200 is reserved for not being able to try at all. + * + * No status check. A sold item's photos are still the shop's photos, and + * improving them changes nothing about the sale — the guards on `unpublish` + * protect a checkout in progress and a completed sale, neither of which is at + * stake in a photograph's background. + */ +router.post('/items/:id/remove-backgrounds', asyncRoute(async (req: Request, res: Response) => { + const itemId = readId(req.params.id); + if (itemId === null || !(await itemExists(itemId))) { + return res.status(404).json({ error: 'not found' }); + } + + res.json(await removeBackgroundsForItem(itemId)); +})); + +/** + * Put every original back. + * + * The reason removing is safe to try. Photos that were never cut out are + * skipped rather than refused, so a half-done item — what a partial failure + * leaves behind — is restorable too. + */ +router.post('/items/:id/restore-originals', asyncRoute(async (req: Request, res: Response) => { + const itemId = readId(req.params.id); + if (itemId === null || !(await itemExists(itemId))) { + return res.status(404).json({ error: 'not found' }); + } + + res.json(await restoreOriginalsForItem(itemId)); +})); + export default router; diff --git a/backend/src/routes/adminConfig.ts b/backend/src/routes/adminConfig.ts new file mode 100644 index 0000000..15cb12a --- /dev/null +++ b/backend/src/routes/adminConfig.ts @@ -0,0 +1,27 @@ +import { Router, Request, Response } from 'express'; +import { isRembgConfigured } from '../intake/rembgClient'; + +const router = Router(); + +/** + * What the admin screens can offer in this environment. + * + * Behind `requireAdminGate` like every other admin router, and deliberately not + * folded into `/api/config` — the same reasoning `adminVersion.ts` records. + * That endpoint is public and the storefront fetches it on every load; nothing + * here is any of a customer's business. + * + * It exists because the inventory screen has no other way to learn this. + * `GET /api/admin/item-drafts` carries the flag for the review queue, but + * `GET /api/admin/items` answers a bare array with several consumers, and + * changing its shape for one boolean would be a worse trade than one small + * route. + * + * Not wrapped in `asyncRoute` because the handler is synchronous: it reads an + * environment variable, so there is no promise to reject. + */ +router.get('/', (_req: Request, res: Response) => { + res.json({ backgroundRemoval: isRembgConfigured() }); +}); + +export default router; diff --git a/backend/tests/integration/adminItemBackgrounds.integration.test.ts b/backend/tests/integration/adminItemBackgrounds.integration.test.ts new file mode 100644 index 0000000..5093f4b --- /dev/null +++ b/backend/tests/integration/adminItemBackgrounds.integration.test.ts @@ -0,0 +1,193 @@ +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +let uploads = ''; +let previousUploadsDir: string | undefined; +let stub: http.Server | null = null; + +/** + * A stub sidecar on an ephemeral port, and a temporary uploads directory. + * + * Port 0 rather than a fixed number: several ports in the 55000s are + * Hyper-V-reserved on this machine. No test here contacts a real rembg — it + * takes forty seconds to start, and a suite depending on that is broken by + * construction. + */ +async function startStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void +): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'itembg-')); + previousUploadsDir = process.env.UPLOADS_DIR; + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => handler(req, res)); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +} + +function answerWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); +} + +beforeEach(async () => { + await resetDb(); +}); + +afterEach(async () => { + delete process.env.REMBG_URL; + if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR; + else process.env.UPLOADS_DIR = previousUploadsDir; + if (uploads) await fsp.rm(uploads, { recursive: true, force: true }); + uploads = ''; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** An available item with two photos on disk. */ +async function seedItem(): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Blue vase', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + for (const [index, name] of ['front.jpg', 'back.jpg'].entries()) { + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${name}`, index] + ); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + } + return itemId; +} + +describe('removing every background on an item', () => { + it('answers with the summary', async () => { + await startStub(answerWithPng); + const itemId = await seedItem(); + + const res = await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ total: 2, removed: 2, failed: false }); + }); + + // Every status, with no carve-out. A sold item's photos are still the shop's + // photos, and improving them changes nothing about the sale. + it.each(['sold', 'reserved', 'pending'])('works on a %s item', async (status) => { + await startStub(answerWithPng); + const itemId = await seedItem(); + await pool.query(`UPDATE items SET status = $2 WHERE id = $1`, [itemId, status]); + + const res = await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + expect(res.status).toBe(200); + expect(res.body.removed).toBe(2); + }); + + // The departure from #281's per-photo endpoints, and the reason for it: this + // acts on several images, so "did it work" has no single answer and a 502 + // would throw away the count that makes the outcome actionable. + it('answers 200 with the count when the sidecar fails, not 502', async () => { + await startStub((_req, res) => { + res.writeHead(500); + res.end('boom'); + }); + const itemId = await seedItem(); + + const res = await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ total: 2, removed: 0, failed: true }); + }); + + it('answers 404 for an id that cannot be read', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/abc/remove-backgrounds')).status).toBe(404); + }); + + it('answers 404 for an item that does not exist', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/999999/remove-backgrounds')).status).toBe(404); + }); +}); + +describe('restoring every original on an item', () => { + it('puts them back and says how many', async () => { + await startStub(answerWithPng); + const itemId = await seedItem(); + await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + const res = await request(app).post(`/api/admin/items/${itemId}/restore-originals`); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ total: 2, restored: 2 }); + }); + + it('answers 404 for an id that cannot be read', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/abc/restore-originals')).status).toBe(404); + }); +}); + +// Extra scope beyond the endpoints themselves: Task 3 derives its button label +// from `original_image_path`, which the admin item select did not carry before +// this change. Asserted here because it is this task's select that changed. +describe('what item images carry in each list', () => { + it('gives original_image_path to admin, not the storefront', async () => { + await startStub(answerWithPng); + const itemId = await seedItem(); + await request(app).post(`/api/admin/items/${itemId}/remove-backgrounds`); + + const adminRes = await request(app).get('/api/admin/items'); + expect(adminRes.status).toBe(200); + const adminItem = adminRes.body.find((item: { id: number }) => item.id === itemId); + expect(adminItem.images[0]).toHaveProperty('original_image_path', '/uploads/front.jpg'); + + const publicRes = await request(app).get('/api/items'); + expect(publicRes.status).toBe(200); + const publicItem = publicRes.body.find((item: { id: number }) => item.id === itemId); + expect(publicItem.images[0]).not.toHaveProperty('original_image_path'); + }); +}); + +describe('what the admin screen is told about the feature', () => { + it('says it is on when a sidecar is configured', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + + const res = await request(app).get('/api/admin/config'); + + expect(res.status).toBe(200); + expect(res.body.backgroundRemoval).toBe(true); + }); + + it('says it is off when none is', async () => { + delete process.env.REMBG_URL; + + const res = await request(app).get('/api/admin/config'); + + expect(res.body.backgroundRemoval).toBe(false); + }); +}); From 385d5b89bfb7a4946ed722a8e082e6a537043635 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:36:00 -0500 Subject: [PATCH 5/9] feat(admin): offer background removal where an item's photos are edited (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One button per item in the inventory editor, beside the per-thumbnail delete buttons rather than on them, because an upload is one item and its photos are views of one thing. Its label is derived from the images rather than stored: Restore originals when every photo already carries an original, Remove backgrounds otherwise. The otherwise deliberately covers the mixed state a partial failure leaves behind — with two of four cut out it reads Remove backgrounds, which is the action that finishes the job, and pressing it skips the two that already worked. Rendering is gated on the feature being configured or every photo already being cut out, not on the flag alone. Gating on the flag would hide Restore originals the moment REMBG_URL is unset, stranding cut-out photos with no way back — the same reasoning the review queue's control already uses. The editor is a modal and this changes files on the server while it is open, so the item is re-read afterwards and the open modal updated. Without that the thumbnails keep showing the previous files and the button looks like it did nothing, which is the bug this was most likely to ship with. frontend/src/api.ts gains original_image_path on the shared Item type's images, since ADMIN_ITEM_SELECT's images aggregate carries it and the public catalogue's does not. It is added as optional rather than required because Item is the same type fetchItems() uses for the public storefront, and a required field the public response never sends would be a type that lies about what is actually there. Co-Authored-By: Claude Opus 5 --- frontend/src/admin/Admin.tsx | 72 ++++++++++++++++++- frontend/src/api.ts | 10 ++- .../tests/e2e/admin-item-backgrounds.spec.ts | 54 ++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 frontend/tests/e2e/admin-item-backgrounds.spec.ts diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx index 0b115ac..26c4045 100755 --- a/frontend/src/admin/Admin.tsx +++ b/frontend/src/admin/Admin.tsx @@ -66,6 +66,11 @@ function Inventory() { const [tags, setTags] = useState([]); const [saving, setSaving] = useState(false); const [filters, setFilters] = useState(EMPTY_FILTERS); + // Whether this environment has a background-removal sidecar. False hides the + // control rather than offering one that would report zero of four done every + // time — an unconfigured environment is a working one, not a broken one. + const [backgroundRemoval, setBackgroundRemoval] = useState(false); + const [busyBackgrounds, setBusyBackgrounds] = useState(false); const { mode } = useThemeMode(); // Typing in the price fields fires a request per keystroke, so responses can @@ -91,7 +96,11 @@ function Inventory() { // from the sibling tabs, so they're refetched whenever the modal opens. const loadOptions = useCallback(() => Promise.all([ fetchAdminCategories().then(setCategories), - fetchAdminTags().then(setTags) + fetchAdminTags().then(setTags), + fetch('/api/admin/config') + .then((res) => (res.ok ? res.json() : { backgroundRemoval: false })) + .then((config) => setBackgroundRemoval(config.backgroundRemoval)) + .catch(() => setBackgroundRemoval(false)) ]).catch(() => message.error('Could not load categories and tags')), []); // Refetch whenever the filters change — filtering is server-side so the @@ -192,6 +201,49 @@ function Inventory() { : prev); } + /** + * Cut out every photo of the item being edited, or put every original back. + * + * Per item, not per photo: an upload is one item, and its photos are views of + * one thing (#293). + * + * The response replaces the open modal's images rather than being trusted to + * have changed nothing else. The editor is a modal and this changes files on + * the server while it is open, so without a refresh the thumbnails keep + * showing the previous files and the button looks like it did nothing. + */ + async function handleBackgrounds(itemId: number, action: 'remove-backgrounds' | 'restore-originals') { + setBusyBackgrounds(true); + try { + const res = await fetch(`/api/admin/items/${itemId}/${action}`, { method: 'POST' }); + if (!res.ok) { + message.error('That did not work.'); + return; + } + const summary = await res.json(); + + if (action === 'remove-backgrounds' && summary.failed) { + // Said plainly rather than as a generic failure. How far it got is what + // decides whether pressing it again is worth anything, and it is — + // a retry skips the ones that already worked. + message.warning(`${summary.removed} of ${summary.total} photos done. Try again to finish.`); + } else { + message.success('Done.'); + } + + // Re-read the item so the thumbnails match what is now on the server. + const fresh = await fetch('/api/admin/items'); + if (fresh.ok) { + const all: Item[] = await fresh.json(); + const updated = all.find((candidate) => candidate.id === itemId); + if (updated) setEditingItem(prev => (prev && prev.id === itemId ? updated : prev)); + setItems(all); + } + } finally { + setBusyBackgrounds(false); + } + } + const columns = [ { title: 'Image', @@ -322,6 +374,24 @@ function Inventory() { ))} + {(backgroundRemoval || editingItem.images.every(img => img.original_image_path !== null)) && ( +
+ +
+ )} )} diff --git a/frontend/src/api.ts b/frontend/src/api.ts index 680c02c..af72130 100755 --- a/frontend/src/api.ts +++ b/frontend/src/api.ts @@ -13,7 +13,15 @@ export interface Item { name: string; description: string | null; price_cents: number; - images: { id: number; image_path: string; sort_order: number }[]; + images: { + id: number; + image_path: string; + sort_order: number; + // Present on admin responses only — ADMIN_ITEM_SELECT's images aggregate + // carries it, PUBLIC_ITEM_SELECT's does not — so it stays optional on this + // shared type rather than a lie the public fetchItems() response can't back up. + original_image_path?: string | null; + }[]; status: 'pending' | 'available' | 'reserved' | 'sold'; category_id: number | null; category_name: string | null; diff --git a/frontend/tests/e2e/admin-item-backgrounds.spec.ts b/frontend/tests/e2e/admin-item-backgrounds.spec.ts new file mode 100644 index 0000000..7f374b8 --- /dev/null +++ b/frontend/tests/e2e/admin-item-backgrounds.spec.ts @@ -0,0 +1,54 @@ +import { test, expect, createAdminContext, uniqueSuffix } from './fixtures'; + +/** + * The per-item background control in the inventory editor (#293). + * + * Asserts the control is offered, not that a cut-out happens. Pressing it needs + * a live rembg sidecar, which takes forty seconds to start and which no test + * should depend on — the swap itself is covered in the integration suite + * against a stub. + * + * "Does not appear when the feature is unconfigured" is not written here as an + * e2e case: unsetting REMBG_URL means restarting the backend mid-suite, which + * this run has no way to do and should not gain one. GET /api/admin/config + * answering `backgroundRemoval: false` is covered by Task 2's integration + * tests, and the render condition that gates the button on it is plain enough + * to read in Admin.tsx. + */ +const RUN = uniqueSuffix(); +const NAME = `Vase ${RUN}`; + +// The 1x1 PNG the other upload specs use, so this goes through the real +// validated upload path rather than a buffer that merely starts correctly. +const PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==', + 'base64' +); + +test.beforeAll(async ({ playwright }) => { + const api = await createAdminContext(playwright); + // Seeded with an image directly: createItem cannot attach one, and the + // control does not render for an item with no photos. + const res = await api.post('/api/admin/items', { + multipart: { + name: NAME, + description: '', + price: '50', + category_id: '', + tags: '[]', + images: { name: `${RUN}.png`, mimeType: 'image/png', buffer: PNG } + } + }); + expect(res.ok(), `seeding ${NAME}`).toBeTruthy(); + await api.dispose(); +}); + +test.describe('Removing backgrounds from the item editor', () => { + test('offers the control on an item that has photos', async ({ page, admin }) => { + await admin.open('Inventory'); + + await page.getByRole('row', { name: new RegExp(NAME) }).getByRole('button', { name: 'Edit' }).click(); + + await expect(page.getByRole('button', { name: 'Remove backgrounds' })).toBeVisible(); + }); +}); From dbb63bc3d2502f8d998d964499aa1fcd75bda691 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 10:44:07 -0500 Subject: [PATCH 6/9] fix(admin): keep the active inventory filter and surface background-swap failures (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleBackgrounds re-read /api/admin/items unfiltered and called setItems(all) after every remove-backgrounds or restore-originals call, so an admin who had filtered Inventory to one category and opened an item from that filtered view saw the table silently repopulate with the entire unfiltered catalogue the moment the request resolved. Every other mutation in this file goes through load(), which respects the active filters; this one didn't, for no reason the spec required. The fix reuses load() instead: it now hands back the rows it fetched (previously discarded after setItems), and handleBackgrounds picks the edited item's fresh row out of that filtered result to refresh the open modal, rather than issuing a second unfiltered fetch. There is no GET /api/admin/items/:id route to fetch a single item directly, and the remove-backgrounds/restore-originals routes return only a summary, not the item, so load()'s own result is what's actually available. A background swap never touches the fields anything filters on, so the edited item stays in the filtered result whenever it was in it before. Also added a catch to handleBackgrounds, matching the message.error shape every sibling handler (handleDelete, handleDeleteImage, handleStatusChange) already uses — previously a network drop or a malformed JSON body became an unhandled rejection with no toast, silently different from how the rest of the file reports failure. Co-Authored-By: Claude Opus 5 --- frontend/src/admin/Admin.tsx | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx index 26c4045..bee99d4 100755 --- a/frontend/src/admin/Admin.tsx +++ b/frontend/src/admin/Admin.tsx @@ -85,11 +85,15 @@ function Inventory() { return fetchAdminItems(active) .then(rows => { if (seq === latestRequest.current) setItems(rows); + // Handed back so a caller that needs one fresh row (handleBackgrounds) + // can pick it out of this filtered fetch instead of issuing its own + // second, unfiltered one. + return rows; }) // Without this the table simply keeps showing whatever it had, so a // failed refetch after a save looks identical to a save that did not // change anything. - .catch(() => message.error('Could not load items')); + .catch(() => { message.error('Could not load items'); return undefined; }); }, [filters]); // The item form needs the current category tree and tag list; both change @@ -213,6 +217,7 @@ function Inventory() { * showing the previous files and the button looks like it did nothing. */ async function handleBackgrounds(itemId: number, action: 'remove-backgrounds' | 'restore-originals') { + const label = action === 'remove-backgrounds' ? 'remove backgrounds' : 'restore originals'; setBusyBackgrounds(true); try { const res = await fetch(`/api/admin/items/${itemId}/${action}`, { method: 'POST' }); @@ -231,14 +236,16 @@ function Inventory() { message.success('Done.'); } - // Re-read the item so the thumbnails match what is now on the server. - const fresh = await fetch('/api/admin/items'); - if (fresh.ok) { - const all: Item[] = await fresh.json(); - const updated = all.find((candidate) => candidate.id === itemId); - if (updated) setEditingItem(prev => (prev && prev.id === itemId ? updated : prev)); - setItems(all); - } + // Re-read the list through load() — same as every other mutation here — + // so a filtered view survives this, then pick this item's fresh images + // back out of it for the open modal. The item's own filtered fields + // (category, tags, status, search) are untouched by a background swap, + // so it stays in the result whenever it was in it before. + const rows = await load(); + const updated = rows?.find(candidate => candidate.id === itemId); + if (updated) setEditingItem(prev => (prev && prev.id === itemId ? updated : prev)); + } catch (err) { + message.error(`Couldn't ${label} — ${(err as Error).message}`); } finally { setBusyBackgrounds(false); } From 445c9c4a226fb2dbb0037332bfe9803a0d553167 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 12:02:23 -0500 Subject: [PATCH 7/9] fix(backgrounds): report a partial restore instead of throwing (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had. The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved. The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail. Co-Authored-By: Claude Opus 5 --- backend/src/intake/backgroundRemoval.ts | 43 ++++++++-- backend/src/intake/draftingWorker.ts | 12 ++- backend/src/routes/admin.ts | 7 +- .../adminItemBackgrounds.integration.test.ts | 11 ++- .../backgroundRemoval.integration.test.ts | 4 +- backend/tests/unit/backgroundRemoval.test.ts | 78 ++++++++++++++++++- 6 files changed, 141 insertions(+), 14 deletions(-) diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index 2659d68..d19f8d1 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -189,14 +189,19 @@ export interface RemovalSummary { /** * What a whole-item restore did. * - * No `failed`, because restoring cannot fail the way removing can: it is a - * database swap with no sidecar in it, and a photo that was never cut out is - * skipped rather than being an error. + * Carries `failed` for the same reason `RemovalSummary` does. Restoring is a + * database swap with no sidecar in it, so it fails far less often than + * removing does — but "far less often" is not "never", and a database error + * partway through a four-photo restore is exactly the moment an admin needs + * the count rather than a bare 500. A photo that was never cut out is skipped + * rather than being an error either way. */ export interface RestoreSummary { total: number; /** How many were put back. Photos that were never cut out are not counted. */ restored: number; + /** Whether it stopped early because one of them failed. */ + failed: boolean; } /** Every photo of one item, in order. */ @@ -252,6 +257,26 @@ export async function removeBackgroundsForItem(itemId: number): Promise { const imageIds = await imageIdsFor(itemId); @@ -262,10 +287,14 @@ export async function restoreOriginalsForItem(itemId: number): Promise { // // This is the sender's tick from the submission page, honoured here so // they never waited for it — and a failure must not mark a draft that was - // written correctly as failed. The photo keeps its original in that case, - // and the admin's per-photo control is still there to do it by hand. + // written correctly as failed. + // + // removeBackgroundsForItem no longer rejects over a single photo failing + // (#293 gave it a summary instead, for the admin screen that acts on the + // count) — so this .catch now fires only if the image-listing query + // itself throws, which is rare enough to warrant a log and nothing more. + // A per-photo failure comes back as `failed: true` in the summary, which + // this sweep discards; the photo keeps its original in that case, and the + // admin's per-photo control in the review queue is still there to do it + // by hand. // // Awaited, unlike the notification below, so a sweep that has returned // has finished its work. Nothing is waiting on this: the worker is off diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 6e6c1cd..f6021e0 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -352,7 +352,7 @@ router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Re })); /** - * One item's images, or null when the item does not exist. + * Whether an item with this id exists. * * Checked before acting so an absent item is a 404 rather than a cheerful * summary of nothing. `removeBackgroundsForItem` would happily report @@ -396,6 +396,11 @@ router.post('/items/:id/remove-backgrounds', asyncRoute(async (req: Request, res * The reason removing is safe to try. Photos that were never cut out are * skipped rather than refused, so a half-done item — what a partial failure * leaves behind — is restorable too. + * + * Answers 200 once the id is valid, same as remove-backgrounds and for the + * same reason: `restoreOriginalsForItem` stops at the first genuine failure + * rather than throwing, so there is always a summary to return, never a bare + * 500 that discards how far it got. */ router.post('/items/:id/restore-originals', asyncRoute(async (req: Request, res: Response) => { const itemId = readId(req.params.id); diff --git a/backend/tests/integration/adminItemBackgrounds.integration.test.ts b/backend/tests/integration/adminItemBackgrounds.integration.test.ts index 5093f4b..72e3c0f 100644 --- a/backend/tests/integration/adminItemBackgrounds.integration.test.ts +++ b/backend/tests/integration/adminItemBackgrounds.integration.test.ts @@ -142,7 +142,7 @@ describe('restoring every original on an item', () => { const res = await request(app).post(`/api/admin/items/${itemId}/restore-originals`); expect(res.status).toBe(200); - expect(res.body).toEqual({ total: 2, restored: 2 }); + expect(res.body).toEqual({ total: 2, restored: 2, failed: false }); }); it('answers 404 for an id that cannot be read', async () => { @@ -150,6 +150,15 @@ describe('restoring every original on an item', () => { expect((await request(app).post('/api/admin/items/abc/restore-originals')).status).toBe(404); }); + + // remove-backgrounds has always had this case; restore-originals did not, + // and the two handlers are copy-paste rather than a shared helper — nothing + // would have caught them diverging. + it('answers 404 for an item that does not exist', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/999999/restore-originals')).status).toBe(404); + }); }); // Extra scope beyond the endpoints themselves: Task 3 derives its button label diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts index 4d3ffd1..297ea03 100644 --- a/backend/tests/integration/backgroundRemoval.integration.test.ts +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -381,7 +381,7 @@ describe('putting every original back', () => { const summary = await restoreOriginalsForItem(itemId); - expect(summary).toEqual({ total: 2, restored: 2 }); + expect(summary).toEqual({ total: 2, restored: 2, failed: false }); const { rows: after } = await pool.query<{ image_path: string }>( `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`, @@ -412,6 +412,6 @@ describe('putting every original back', () => { ); await removeImageBackground(images[0]!.id); - expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1 }); + expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1, failed: false }); }); }); diff --git a/backend/tests/unit/backgroundRemoval.test.ts b/backend/tests/unit/backgroundRemoval.test.ts index 23bf41b..a9a9ea5 100644 --- a/backend/tests/unit/backgroundRemoval.test.ts +++ b/backend/tests/unit/backgroundRemoval.test.ts @@ -1,4 +1,41 @@ -import { cutoutPathFor } from '../../src/intake/backgroundRemoval'; +import { pool } from '../../src/db'; +import { cutoutPathFor, restoreOriginalsForItem } from '../../src/intake/backgroundRemoval'; + +// The database module is replaced outright rather than spied on. This has to +// be a unit test: the only failure `restoreOriginalsForItem` can report is a +// database fault, and the only way to inject one is to make a query fail on +// command. Doing that in the integration suite means interfering with the +// single pool every suite in the same `--runInBand` process shares, and that +// `afterAll` calls `pool.end()` on — which made the whole suite unrunnable. +// Here no `Pool` is ever constructed: jest gives this file its own module +// registry, so there is nothing shared to break. +jest.mock('../../src/db', () => ({ pool: { query: jest.fn(), connect: jest.fn() } })); + +const mockPool = pool as unknown as { query: jest.Mock; connect: jest.Mock }; + +/** The distinctive text of the swap that puts one photo back. */ +const SWAP_SQL = 'SET image_path = original_image_path'; + +/** + * A pool over `imageIds` whose nth restore swap fails. + * + * Matching on the swap's SQL rather than counting queries: `restoreImageOriginal` + * also issues BEGIN, the `item_drafts` update and COMMIT on the same client, so + * a bare call counter would break whichever query happened to land nth. + */ +function poolWhoseSwapFailsOn(imageIds: number[], failOnSwap: number): void { + mockPool.query.mockResolvedValue({ rows: imageIds.map((id) => ({ id })) }); + let swaps = 0; + mockPool.connect.mockImplementation(async () => ({ + query: jest.fn(async (sql: string) => { + if (!String(sql).includes(SWAP_SQL)) return { rows: [] }; + swaps += 1; + if (swaps === failOnSwap) throw new Error('database is down'); + return { rows: [{ item_id: 1 }] }; + }), + release: jest.fn() + })); +} describe('where a cut-out is written', () => { // A new file rather than a rewrite of the original, which is what makes the @@ -20,3 +57,42 @@ describe('where a cut-out is written', () => { expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png'); }); }); + +describe('when a restore fails partway through an item', () => { + // Logged rather than thrown, so the log line is expected here; silenced to + // keep it out of the suite's output rather than because it does not matter. + let logged: jest.SpyInstance; + + beforeEach(() => { + jest.clearAllMocks(); + logged = jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + logged.mockRestore(); + }); + + // The case the summary's `failed` field exists for: a genuine database + // failure partway through must not turn into a bare 500 that discards how + // far the restore got. It has to stop, report the count, and let the caller + // decide whether to retry — the same contract removeBackgroundsForItem has. + it('stops at the first genuine failure and reports how far it got', async () => { + poolWhoseSwapFailsOn([11, 22, 33], 2); + + await expect(restoreOriginalsForItem(4)).resolves.toEqual({ + total: 3, + restored: 1, + failed: true + }); + }); + + // The third photo is never attempted, which is what makes pressing Restore + // again worth something: it resumes rather than starting over. + it('does not go on to the photos after the one that failed', async () => { + poolWhoseSwapFailsOn([11, 22, 33], 2); + + await restoreOriginalsForItem(4); + + expect(mockPool.connect).toHaveBeenCalledTimes(2); + }); +}); From 64f8efb61795978cbb55f262e1d59f26bde32eec Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 12:02:37 -0500 Subject: [PATCH 8/9] fix(admin): offer Remove and Restore independently (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One button whose label flipped on "is every photo cut out?" could not serve a partly cut-out item, which is not a hypothetical state: it is what a partial removal leaves behind, and it is also what happens when REMBG_URL goes away after some photos were already done. In that state the single button read "Remove backgrounds", so the cut-out photos the item already had could never be restored from this screen. Remove and Restore are now separately gated and can appear together, which is correct — Remove finishes the job on what is left, Restore undoes what is already done. Restore is deliberately not gated on the backgroundRemoval config flag. Gating it would strand cut-out photos with no way back in exactly the environment that most needs the undo. Remove stays gated, so an unconfigured environment shows no button rather than one that reports zero of four done every time. The emptiness check moves from `!== null` to `!= null`: original_image_path is optional on the shared Item type because the public storefront response omits it, so a stray undefined has to count as "not cut out" — `undefined !== null` is true, which would misread a public-shaped item as fully cut out. The modal now refreshes on a non-ok response too. A restore that fails partway can still have swapped some files back before it failed, so returning early left the thumbnails showing files that are no longer on the server. The warning text is now driven off whichever count the action reports, so a partial restore says how far it got the same way a partial removal already did. The e2e spec seeds its item into a category of its own and filters the table down to it. The inventory table paginates at 10 and the suite runs fullyParallel, so an unfiltered page one was never a reliable place to find the fixture. Co-Authored-By: Claude Opus 5 --- frontend/src/admin/Admin.tsx | 88 ++++++++++++------- .../tests/e2e/admin-item-backgrounds.spec.ts | 20 ++++- 2 files changed, 74 insertions(+), 34 deletions(-) diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx index bee99d4..ab5daef 100755 --- a/frontend/src/admin/Admin.tsx +++ b/frontend/src/admin/Admin.tsx @@ -214,7 +214,10 @@ function Inventory() { * The response replaces the open modal's images rather than being trusted to * have changed nothing else. The editor is a modal and this changes files on * the server while it is open, so without a refresh the thumbnails keep - * showing the previous files and the button looks like it did nothing. + * showing the previous files and the button looks like it did nothing. That + * holds even when the server answers something other than 200: a restore + * that fails partway can still have swapped some files back before it did, + * so the refresh below runs whether the call succeeded or not. */ async function handleBackgrounds(itemId: number, action: 'remove-backgrounds' | 'restore-originals') { const label = action === 'remove-backgrounds' ? 'remove backgrounds' : 'restore originals'; @@ -223,24 +226,26 @@ function Inventory() { const res = await fetch(`/api/admin/items/${itemId}/${action}`, { method: 'POST' }); if (!res.ok) { message.error('That did not work.'); - return; - } - const summary = await res.json(); - - if (action === 'remove-backgrounds' && summary.failed) { - // Said plainly rather than as a generic failure. How far it got is what - // decides whether pressing it again is worth anything, and it is — - // a retry skips the ones that already worked. - message.warning(`${summary.removed} of ${summary.total} photos done. Try again to finish.`); } else { - message.success('Done.'); + const summary = await res.json(); + const done = action === 'remove-backgrounds' ? summary.removed : summary.restored; + + if (summary.failed) { + // Said plainly rather than as a generic failure. How far it got is + // what decides whether pressing it again is worth anything, and it + // is — a retry skips the ones that already worked. + message.warning(`${done} of ${summary.total} photos done. Try again to finish.`); + } else { + message.success('Done.'); + } } // Re-read the list through load() — same as every other mutation here — // so a filtered view survives this, then pick this item's fresh images // back out of it for the open modal. The item's own filtered fields // (category, tags, status, search) are untouched by a background swap, - // so it stays in the result whenever it was in it before. + // so it stays in the result whenever it was in it before. Run for a + // failed response too — see the doc comment above. const rows = await load(); const updated = rows?.find(candidate => candidate.id === itemId); if (updated) setEditingItem(prev => (prev && prev.id === itemId ? updated : prev)); @@ -381,24 +386,47 @@ function Inventory() { ))} - {(backgroundRemoval || editingItem.images.every(img => img.original_image_path !== null)) && ( -
- -
- )} + {(() => { + // `!= null` rather than `!== null`: original_image_path is + // optional on the shared Item type (it is absent from the + // public storefront response), so a stray `undefined` has to + // count as "not cut out" too — `undefined !== null` is `true`, + // which would misread a public-shaped item as fully cut out. + const allCutOut = editingItem.images.every(img => img.original_image_path != null); + const someCutOut = editingItem.images.some(img => img.original_image_path != null); + // Remove and Restore are independent, not two labels for one + // button: a partly cut-out item — a partial removal's normal + // result, or REMBG_URL going away after the fact — needs both, + // or the cut-out photos it already has can never be restored + // from this screen (#293). + const showRemove = backgroundRemoval && !allCutOut; + const showRestore = someCutOut; + if (!showRemove && !showRestore) return null; + return ( +
+ + {showRemove && ( + + )} + {showRestore && ( + + )} + +
+ ); + })()} )} diff --git a/frontend/tests/e2e/admin-item-backgrounds.spec.ts b/frontend/tests/e2e/admin-item-backgrounds.spec.ts index 7f374b8..b5a4070 100644 --- a/frontend/tests/e2e/admin-item-backgrounds.spec.ts +++ b/frontend/tests/e2e/admin-item-backgrounds.spec.ts @@ -1,4 +1,4 @@ -import { test, expect, createAdminContext, uniqueSuffix } from './fixtures'; +import { test, expect, createAdminContext, createCategory, uniqueSuffix } from './fixtures'; /** * The per-item background control in the inventory editor (#293). @@ -17,6 +17,10 @@ import { test, expect, createAdminContext, uniqueSuffix } from './fixtures'; */ const RUN = uniqueSuffix(); const NAME = `Vase ${RUN}`; +// A category of its own, not shared: it exists only so filterByCategory below +// has something unique to narrow the table down to, the same way +// admin-inventory-filters.spec.ts uses one per run. +const CATEGORY = `Backgrounds ${RUN}`; // The 1x1 PNG the other upload specs use, so this goes through the real // validated upload path rather than a buffer that merely starts correctly. @@ -27,6 +31,7 @@ const PNG = Buffer.from( test.beforeAll(async ({ playwright }) => { const api = await createAdminContext(playwright); + const categoryId = await createCategory(api, CATEGORY); // Seeded with an image directly: createItem cannot attach one, and the // control does not render for an item with no photos. const res = await api.post('/api/admin/items', { @@ -34,7 +39,7 @@ test.beforeAll(async ({ playwright }) => { name: NAME, description: '', price: '50', - category_id: '', + category_id: String(categoryId), tags: '[]', images: { name: `${RUN}.png`, mimeType: 'image/png', buffer: PNG } } @@ -44,10 +49,17 @@ test.beforeAll(async ({ playwright }) => { }); test.describe('Removing backgrounds from the item editor', () => { - test('offers the control on an item that has photos', async ({ page, admin }) => { + test('offers the control on an item that has photos', async ({ page, admin, adminInventory }) => { await admin.open('Inventory'); - await page.getByRole('row', { name: new RegExp(NAME) }).getByRole('button', { name: 'Edit' }).click(); + // The table paginates at 10 and this suite runs fullyParallel, so an + // unfiltered page 1 is not a reliable place to find this fixture — see + // AdminInventory.filterByCategory. Filtering to this item's own category + // narrows the table down to just it, the way admin-inventory-filters.spec.ts + // does. + await adminInventory.filterByCategory(CATEGORY, NAME); + + await adminInventory.row(NAME).getByRole('button', { name: 'Edit' }).click(); await expect(page.getByRole('button', { name: 'Remove backgrounds' })).toBeVisible(); }); From b70e4a68f086261b351578b2600cf5ca797f43b2 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 12:02:48 -0500 Subject: [PATCH 9/9] docs(specs): match the design to the shipped behaviour (#293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...2026-09-04-inventory-background-removal-design.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 index dadcec9..f99d310 100644 --- a/docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md +++ b/docs/superpowers/specs/2026-09-04-inventory-background-removal-design.md @@ -55,10 +55,12 @@ export interface RestoreSummary { 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 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. +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 @@ -74,11 +76,11 @@ Non-200 is reserved for not being able to try at all, which here means only an u 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. +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. -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. +**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. -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. +**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. @@ -93,7 +95,7 @@ The obvious-looking reuse is wrong. The review queue's control is per photo and | 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`. | +| 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. |