docs(admin): plan background removal in the inventory editor (#293)
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 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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 <file>`, delete it. Do not push.
|
||||
- End every commit message with:
|
||||
`Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`
|
||||
|
||||
---
|
||||
|
||||
### 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<RemovalSummary>` — was `Promise<void>`
|
||||
- `restoreOriginalsForItem(itemId: number): Promise<RestoreSummary>` — 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<number[]> {
|
||||
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<RemovalSummary> {
|
||||
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<RestoreSummary> {
|
||||
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 <noreply@anthropic.com>
|
||||
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<void> {
|
||||
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<void>((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<void>((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<number> {
|
||||
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<boolean> {
|
||||
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 <noreply@anthropic.com>
|
||||
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<Item | null>(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 `<Space wrap>` that maps the thumbnails:
|
||||
|
||||
```tsx
|
||||
{(backgroundRemoval || editingItem.images.every(img => img.original_image_path !== null)) && (
|
||||
<div style={{ marginTop: 8 }}>
|
||||
<Button
|
||||
size="small"
|
||||
loading={busyBackgrounds}
|
||||
onClick={() => void handleBackgrounds(
|
||||
editingItem.id,
|
||||
editingItem.images.every(img => img.original_image_path !== null)
|
||||
? 'restore-originals'
|
||||
: 'remove-backgrounds'
|
||||
)}
|
||||
>
|
||||
{editingItem.images.every(img => img.original_image_path !== null)
|
||||
? 'Restore originals'
|
||||
: 'Remove backgrounds'}
|
||||
</Button>
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
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 <noreply@anthropic.com>
|
||||
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.
|
||||
Reference in New Issue
Block a user