From 9ced34ad194c0481ba76e6427b58ea472c8fe1e5 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:49:38 -0500 Subject: [PATCH] feat(admin): remove and restore a photo's background per image (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two admin-gated endpoints on the review queue router: POST /:itemId/images/:imageId/remove-background and POST /:itemId/images/:imageId/restore-original. Both run synchronously and reuse the same backgroundRemoval module the drafting worker uses, so a cut-out obtained either way is identical and either can be undone by Restore. Ownership is scoped by item as well as by image (imageOfItem selects on id AND item_id), because the image id is a serial and guessing one is easy — a photo belonging to a different submission must not be reachable through another item's URL. A sidecar failure returns 502, not 500, and leaves the row untouched, since removeImageBackground only writes the row after the cut-out file already exists on disk. GET /api/admin/item-drafts now returns { drafts, backgroundRemoval } instead of { drafts }, and each image in the payload gains original_image_path, which is what the review queue UI will use to decide between "Remove background" and "Restore original". DRAFT_SELECT's images aggregate is extended accordingly, keeping the deliberate column spelling that guards against the upload_links token digest leaking into the response. Co-Authored-By: Claude Opus 5 --- backend/src/routes/adminItemDrafts.ts | 89 ++++++++++- .../adminItemDrafts.integration.test.ts | 146 ++++++++++++++++++ 2 files changed, 233 insertions(+), 2 deletions(-) diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 4954413..cf4123a 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -3,6 +3,8 @@ import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; import { draftQueued } from '../intake/draftingWorker'; import { nextPriceSource, PriceSource } from '../intake/priceSource'; +import { removeImageBackground, restoreImageOriginal } from '../intake/backgroundRemoval'; +import { isRembgConfigured } from '../intake/rembgClient'; const router = Router(); @@ -27,7 +29,10 @@ const DRAFT_SELECT = ` i.price_cents, i.status, l.label AS upload_link_label, COALESCE(( - SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path) + SELECT json_agg(json_build_object( + 'id', img.id, + 'image_path', img.image_path, + 'original_image_path', img.original_image_path) ORDER BY img.sort_order) FROM item_images img WHERE img.item_id = d.item_id ), '[]'::json) AS images @@ -52,7 +57,9 @@ router.get( ? await pool.query(`${DRAFT_SELECT} WHERE d.state = $1 ORDER BY d.created_at DESC`, [state]) : await pool.query(`${DRAFT_SELECT} WHERE d.state <> 'discarded' ORDER BY d.created_at DESC`); - res.json({ drafts: rows }); + // Whether the control has anything behind it, alongside the rows. A second + // endpoint for one boolean would be a round trip the queue already makes. + res.json({ drafts: rows, backgroundRemoval: isRembgConfigured() }); }) ); @@ -220,4 +227,82 @@ router.post( }) ); +/** + * One photo's current paths, if it belongs to this item. + * + * Scoped by item as well as by image so an image id from a different + * submission cannot be acted on through this item's URL — the id is a serial, + * so guessing one is not hard. + */ +async function imageOfItem( + itemId: string, + imageId: string +): Promise<{ image_path: string; original_image_path: string | null } | null> { + const { rows } = await pool.query<{ image_path: string; original_image_path: string | null }>( + `SELECT image_path, original_image_path + FROM item_images WHERE id = $1 AND item_id = $2`, + [imageId, itemId] + ); + return rows[0] ?? null; +} + +/** + * Remove the background from one photo. + * + * The other half of the submitter's checkbox: for the photos nobody ticked it + * for, and for the ones where the worker could not reach the sidecar. Both go + * through the same module, so a cut-out obtained either way is identical and + * either can be undone by Restore. + * + * Synchronous, unlike the worker's path. A warm request measures 1.1–2.3 s and + * this is an admin who just clicked a button and is watching for the result. + * The reason drafting was moved off the request path — that a stranger can + * trigger it and must never wait — does not apply behind the admin gate. + */ +router.post( + '/:itemId/images/:imageId/remove-background', + asyncRoute(async (req: Request, res: Response) => { + const { itemId = '', imageId = '' } = req.params; + if ((await imageOfItem(itemId, imageId)) === null) { + return res.status(404).json({ error: 'no such photo on this item' }); + } + + try { + await removeImageBackground(Number(imageId)); + } catch (err) { + // 502, not 500. The request was fine and so is this app — the service it + // depends on did not answer. The message says the photo is unchanged, + // because that is the thing the admin actually needs to know. + console.error(`[drafts] background removal for image ${imageId}:`, err); + return res + .status(502) + .json({ error: 'the background-removal service did not answer — the photo is unchanged' }); + } + + res.json(await imageOfItem(itemId, imageId)); + }) +); + +/** + * Put the original photo back. + * + * The reason a cut-out is safe to try at all. Background removal produces the + * occasional poor result on an unusual object, and this makes that survivable + * rather than something to prevent. Nothing is deleted: the cut-out file stays + * on disk, because somebody restoring one is quite likely to try again. + */ +router.post( + '/:itemId/images/:imageId/restore-original', + asyncRoute(async (req: Request, res: Response) => { + const { itemId = '', imageId = '' } = req.params; + const existing = await imageOfItem(itemId, imageId); + if (existing === null || existing.original_image_path === null) { + return res.status(404).json({ error: 'this photo has no original to restore' }); + } + + await restoreImageOriginal(Number(imageId)); + res.json(await imageOfItem(itemId, imageId)); + }) +); + export default router; diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 3c76836..89891f5 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -2,6 +2,11 @@ import request from 'supertest'; import app from '../../src/app'; import { pool } from '../../src/db'; import { resetDb, closeDb } from './setup/testDb'; +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; beforeEach(async () => { await resetDb(); @@ -304,3 +309,144 @@ describe('the other three actions', () => { } }); }); + +describe('the review queue’s background-removal control', () => { + 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 stub: http.Server | null = null; + + /** A stub sidecar on an ephemeral port, and a temporary uploads directory. */ + async function startStub(status: number, body: Buffer | string): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'adminbg-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => { + res.writeHead(status, { 'Content-Type': 'image/png' }); + res.end(body); + }); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; + } + + afterEach(async () => { + delete process.env.REMBG_URL; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } + }); + + /** A ready draft with one photo, on disk, named to match the assertions. */ + async function seedDraftWithImage(): Promise<{ itemId: number; imageId: number }> { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = rows[0]!.id; + await pool.query(`INSERT INTO item_drafts (item_id, state) VALUES ($1, 'ready')`, [itemId]); + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, '/uploads/original.jpg', 0) RETURNING id`, + [itemId] + ); + if (uploads !== '') await fsp.writeFile(path.join(uploads, 'original.jpg'), JPEG_BYTES); + return { itemId, imageId: image.rows[0]!.id }; + } + + it('says whether there is a sidecar behind the control at all', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + const on = await request(app).get('/api/admin/item-drafts'); + expect(on.body.backgroundRemoval).toBe(true); + + delete process.env.REMBG_URL; + const off = await request(app).get('/api/admin/item-drafts'); + expect(off.body.backgroundRemoval).toBe(false); + }); + + // The UI decides between "Remove background" and "Restore original" from + // this field alone, so it has to be in the payload the queue is built from. + it('includes original_image_path on every image', async () => { + await seedDraftWithImage(); + + const res = await request(app).get('/api/admin/item-drafts'); + + expect(res.body.drafts[0].images[0]).toHaveProperty('original_image_path', null); + }); + + it('cuts out one photo and answers with its new paths', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).toBe(200); + expect(res.body.image_path).toBe('/uploads/original-cutout.png'); + expect(res.body.original_image_path).toBe('/uploads/original.jpg'); + }); + + it('puts the original back', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + + expect(res.status).toBe(200); + expect(res.body.image_path).toBe('/uploads/original.jpg'); + expect(res.body.original_image_path).toBeNull(); + }); + + // 502 rather than 500: the request was fine and the app is fine, and saying + // which of the two failed is what stops somebody searching the application + // logs for a fault that is not there. + it('answers 502 when the sidecar will not, and leaves the photo alone', async () => { + await startStub(500, 'boom'); + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).toBe(502); + const { rows } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE id = $1`, + [imageId] + ); + expect(rows[0]?.image_path).toBe('/uploads/original.jpg'); + }); + + // Scoped by item as well as by image. The id is a serial, so guessing one is + // not hard, and a photo from another submission must not be reachable + // through this item's URL. + it('refuses an image that does not belong to the item', async () => { + await startStub(200, PNG_BYTES); + const first = await seedDraftWithImage(); + const second = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${first.itemId}/images/${second.imageId}/remove-background` + ); + + expect(res.status).toBe(404); + }); + + it('refuses to restore a photo that was never cut out', async () => { + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + + expect(res.status).toBe(404); + }); +});