Feature/293 remove backgrounds from inventory #296

Merged
bermudalamb merged 9 commits from feature/293-remove-backgrounds-from-inventory into main 2026-09-04 12:20:01 -05:00
14 changed files with 1940 additions and 18 deletions
+2
View File
@@ -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);
+120 -9
View File
@@ -169,21 +169,132 @@ export async function restoreImageOriginal(imageId: number): Promise<void> {
}
/**
* 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<void> {
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.
*
* 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. */
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]
);
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<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.
*
* Stops at the first genuine failure and reports the count, the same shape
* `removeBackgroundsForItem` uses and for the same reason: a caller standing
* in front of the screen needs to know how far it got, and rethrowing here
* would discard that in favour of a bare 500. `NoOriginalToRestoreError` is
* not a genuine failure — it is skipped, as before — so it never reaches this
* stop.
*
* The `failed` path has no integration test, deliberately. The only failure it
* 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 — the same pool `afterAll` calls `pool.end()`
* on. Tests that did exactly that 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. The
* branch is covered instead by `tests/unit/backgroundRemoval.test.ts`, which
* stubs the database module in its own module registry and shares nothing.
*/
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, logged for the same reason removeBackgroundsForItem
// logs rather than throws: the caller gets the count, which is the
// thing it can act on, and the reason belongs in the log.
if (err instanceof NoOriginalToRestoreError) continue;
console.error(`[background-removal] restoring item ${itemId}, image ${imageId}:`, err);
return { total: imageIds.length, restored, failed: true };
}
}
return { total: imageIds.length, restored, failed: false };
}
+10 -2
View File
@@ -165,8 +165,16 @@ export async function draftQueued(limit = DEFAULT_BATCH): Promise<SweepResult> {
//
// 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
+34 -2
View File
@@ -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[];
}
/**
+61
View File
@@ -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,64 @@ router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Re
res.json(available);
}));
/**
* 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
* `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.
*
* 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);
if (itemId === null || !(await itemExists(itemId))) {
return res.status(404).json({ error: 'not found' });
}
res.json(await restoreOriginalsForItem(itemId));
}));
export default router;
+27
View File
@@ -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;
@@ -0,0 +1,202 @@
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, failed: false });
});
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);
});
// 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
// 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);
});
});
@@ -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, 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`,
[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, failed: false });
});
});
+77 -1
View File
@@ -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);
});
});
@@ -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.
@@ -0,0 +1,119 @@
# Removing backgrounds from the inventory item editor
**Issue:** #293. Follows #281, which built the same capability for the submission page and the review queue and deliberately scoped this out.
An admin adding stock themselves goes straight to `Admin | Inventory`, uploads photos, and never touches the review queue — so for those items background removal does not exist. This adds it where the photos actually get edited.
## Decisions, and what each one rests on
**It applies to a live product image.** Removing a background on an `available` item changes what a customer sees while they are browsing. Accepted deliberately: the original is kept and restoring it is one click, so the worst case is a photo that looks wrong until somebody notices. The alternative is a shop whose published items can never be tidied up, which is worse.
**Every status, with no exceptions.** Not `pending` only, and not a carve-out for `sold` or `reserved`. The precedent in `unpublish` — which refuses both by name — does not carry here, because what it protects against is a customer losing an item mid-checkout, or a completed sale being quietly rewritten. Neither is at stake in a photograph's background. A sold item's photos are still the shop's photos.
**The action is per upload, not per photo.** This is the decision that shapes everything else, and it is a deliberate departure from the review queue.
An upload is one item. Somebody photographing a vase sends the front, the back and the chipped base; those are three views of one thing. They are not separate items and they should not be cut out one at a time. So the control acts on every image belonging to the item, as a single action.
**The shared function reports a summary rather than nothing.** `removeBackgroundsForItem` returns `void` today and throws on the first failure, which is enough for the worker — it catches and logs, and a draft is not worth failing over. It is not enough for an admin standing in front of the screen, who needs to know whether the thing they clicked actually happened.
**It keeps stopping at the first failure.** Pushing on through six photos against a sidecar that is not answering helps nobody. The retry story works because `removeImageBackground` is already idempotent: a photo that already has an `original_image_path` is skipped, so a second attempt resumes where the first stopped rather than starting over or double-cutting anything.
## Architecture
```
Admin | Inventory → edit item → Existing Images
├─ POST /api/admin/items/:id/remove-backgrounds → { total, removed, failed }
└─ POST /api/admin/items/:id/restore-originals → { total, restored }
backgroundRemoval.ts (already shared with the worker and the review queue)
removeBackgroundsForItem(itemId) → RemovalSummary
restoreOriginalsForItem(itemId) → RestoreSummary
```
### `removeBackgroundsForItem` gains a return value
```ts
export interface RemovalSummary {
/** How many images the item has. */
total: number;
/** How many now have a cut-out, including any that already did. */
removed: number;
/** Whether it stopped early because one of them failed. */
failed: boolean;
}
```
**The one existing caller does not change.** `draftingWorker.ts:175` ignores the result, and ignoring a returned value is legal — the same reason widening `sendMail` in #260 was additive rather than breaking. `npm run build` is what proves it.
A matching `restoreOriginalsForItem` is new — the review queue restores one photo at a time through `restoreImageOriginal`, and nothing yet does a whole item:
```ts
export interface RestoreSummary {
/** How many images the item has. */
total: number;
/** How many were put back. Images that were never cut out are skipped, not counted. */
restored: number;
/** Whether it stopped early because one of them failed. */
failed: boolean;
}
```
It carries `failed` too, for the same reason `RemovalSummary` does. Restoring is a database swap with no sidecar involved, so it fails far less often than removing does — but a database error partway through a multi-photo restore is still a real possibility, and rethrowing it would turn a partial success into an opaque 500 that discards how far the restore got. An image that was never cut out is skipped rather than being an error either way.
### The endpoints
Both take their id through `readId` and answer 404 for an unreadable or absent one, which is now what every route in `admin.ts` does (#207). Neither checks status.
**These two routes always answer 200 once the id is valid**, and that is a deliberate departure from the per-photo endpoints in #281.
Those act on one image, so the request either worked or it did not, and 502 says which. This one acts on several, so "did it work" has no single answer — two of four is the normal shape of a bad day, not an exception. Returning 502 would throw away the count that makes the outcome actionable, and 200-with-a-summary would then contradict it. So the summary *is* the result: `failed` says whether it stopped early, `removed` says how far it got, and the admin retries.
Non-200 is reserved for not being able to try at all, which here means only an unreadable or absent id. The underlying `SidecarRequestError` from #281 still exists and still distinguishes a sidecar failure from an unreadable file — it is caught here and folded into `failed`, with the real reason logged rather than shown, because the admin's next action is the same either way: press it again.
### The screen
One button per item, in the "Existing Images" block, beside the per-thumbnail delete buttons rather than on them.
Remove and Restore are two independently-gated buttons, not two labels for one button — there is no second flag and no stored state, the images already say which they are, but a mixed item genuinely needs both offered at once.
**Remove backgrounds** is rendered when the server reports the feature configured *and* at least one image is not yet cut out. An unconfigured environment shows no Remove button rather than one that reports zero of four done every time.
**Restore originals** is rendered whenever at least one image on the item already carries an `original_image_path` — regardless of whether the feature is currently configured. That is deliberate, not an oversight: the mixed state a partial removal leaves behind is not hypothetical, and neither is `REMBG_URL` being unset after some photos were already cut out. Either way, gating Restore on `backgroundRemoval` would strand those cut-out photos with no way back. On a partly cut-out item both buttons appear together, and that is correct — Remove finishes the job on what is left, Restore undoes what is already done.
On a partial result the admin is told plainly — "2 of 4 photos done" — with the button still there to try again.
**The editor is a modal, and this changes images on the server while it is open.** The thumbnails have to be refreshed after the action or they show the previous files, which would look like the button doing nothing. This is the detail most likely to turn into a confusing bug, so it is called out here rather than discovered.
## Why `DraftPhoto` is not lifted
The obvious-looking reuse is wrong. The review queue's control is per photo and this one is per item; sharing the component would force one of them to pretend to be the other. What is genuinely shared is underneath — `removeImageBackground` and `restoreImageOriginal`, which both already exist and are already idempotent. That is where the reuse belongs.
## Failure handling
| What happens | Result |
|---|---|
| Unreadable or absent item id | 404. Nothing touched. |
| Feature not configured | No Remove button. Restore is still offered, and still works, whenever an image is already cut out. The remove-backgrounds endpoint still answers, reporting `total` with `removed: 0`. |
| Sidecar will not answer | 200, `failed: true`, `removed: 0`. Nothing was touched. The reason is logged. |
| A file cannot be read | 200, `failed: true`, `removed` short of `total`. Photos done before it keep their cut-outs. |
| Some succeeded, then one failed | 200, `failed: true`, `removed` short of `total`. The admin retries; the second pass skips what already worked. |
| All succeeded | 200, `removed === total`, `failed: false`. |
Nothing is ever left in a state a retry cannot resolve, and nothing is deleted — the same rule the whole feature has followed since #281.
## Testing
- **Integration:** both endpoints on an item with several images; a partial failure leaving a usable item and a retry completing it; 404 for a bad id; an item with no images answering `total: 0` rather than failing; restore returning the originals.
- **Unit:** none needed. The pure parts (`cutoutPathFor`) are already covered from #281, and the new code is all database and HTTP.
- **Worker:** the existing drafting tests prove the widened return did not disturb the one caller.
- **E2E:** the button appears on an item that has images, and does not when the feature is unconfigured.
## Out of scope
**Bulk application across the catalogue.** Still. This is one item at a time, from the editor for that item.
**Any change to the submission page or the review queue.** They keep behaving exactly as #281 built them, including the queue's per-photo control.
**A progress indicator for a long-running removal.** Measured at 1.12.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.
+107 -2
View File
@@ -66,6 +66,11 @@ function Inventory() {
const [tags, setTags] = useState<TagRecord[]>([]);
const [saving, setSaving] = useState(false);
const [filters, setFilters] = useState<ItemFilters>(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
@@ -80,18 +85,26 @@ 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
// 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 +205,57 @@ 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. 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';
setBusyBackgrounds(true);
try {
const res = await fetch(`/api/admin/items/${itemId}/${action}`, { method: 'POST' });
if (!res.ok) {
message.error('That did not work.');
} else {
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. 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));
} catch (err) {
message.error(`Couldn't ${label}${(err as Error).message}`);
} finally {
setBusyBackgrounds(false);
}
}
const columns = [
{
title: 'Image',
@@ -322,6 +386,47 @@ function Inventory() {
</div>
))}
</Space>
{(() => {
// `!= 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 (
<div style={{ marginTop: 8 }}>
<Space>
{showRemove && (
<Button
size="small"
loading={busyBackgrounds}
onClick={() => void handleBackgrounds(editingItem.id, 'remove-backgrounds')}
>
Remove backgrounds
</Button>
)}
{showRestore && (
<Button
size="small"
loading={busyBackgrounds}
onClick={() => void handleBackgrounds(editingItem.id, 'restore-originals')}
>
Restore originals
</Button>
)}
</Space>
</div>
);
})()}
</Form.Item>
)}
<Form.Item label={editingItem ? 'Add More Images' : 'Images (front, back, etc.)'}>
+9 -1
View File
@@ -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;
@@ -0,0 +1,66 @@
import { test, expect, createAdminContext, createCategory, 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}`;
// 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.
const PNG = Buffer.from(
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
'base64'
);
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', {
multipart: {
name: NAME,
description: '',
price: '50',
category_id: String(categoryId),
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, adminInventory }) => {
await admin.open('Inventory');
// 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();
});
});