From df508b3783503c53b7ba047ae5d7f981e2d54c5c Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 13:25:25 -0500 Subject: [PATCH] docs(admin): plan the photo rotation work (#301) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tasks, each with its own test cycle: the file operation, the endpoints and the module behind them, then the control in the review queue. Two things were probed rather than assumed while writing it, and both changed the design. sharp reads a positive angle as clockwise, so left is rotate(-90) and right is rotate(90) — and the direction test asserts a pixel rather than a dimension, because a rectangle's dimensions swap whichever way the turn goes and a reversed sign would pass every size assertion while shipping a control that does the opposite of its label. And a quarter turn of an animated WebP is refused by sharp itself, which is what makes passing the same animated flag reencodeInPlace passes the safe choice: omitting it would read the first frame alone and write a still back over someone's animation while reporting success. Co-Authored-By: Claude Opus 5 --- .../plans/2026-09-04-photo-rotation.md | 917 ++++++++++++++++++ 1 file changed, 917 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-04-photo-rotation.md diff --git a/docs/superpowers/plans/2026-09-04-photo-rotation.md b/docs/superpowers/plans/2026-09-04-photo-rotation.md new file mode 100644 index 0000000..e27719e --- /dev/null +++ b/docs/superpowers/plans/2026-09-04-photo-rotation.md @@ -0,0 +1,917 @@ +# Photo Rotation 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:** Give an admin two buttons on every photo in the review queue that turn it a quarter turn left or right, rewriting the stored file. + +**Architecture:** A file operation (`rotateInPlace`) beside the existing re-encode, a thin database-aware wrapper (`rotateItemImage`) that also turns the pristine original when one exists, two POST routes on the item rather than the draft so the inventory editor needs no backend work later, and a per-photo control in `DraftQueue` that cache-busts its own `` because rotation does not change the path. + +**Tech Stack:** Express 4 + TypeScript, sharp 0.35, `pg`, Jest + supertest (`--runInBand` for integration), React + TypeScript + antd 5, Playwright. + +**Spec:** `docs/superpowers/specs/2026-09-04-photo-rotation-design.md` + +## Global Constraints + +- **Left is anticlockwise, `sharp.rotate(-90)`; right is clockwise, `sharp.rotate(90)`.** sharp reads a positive angle as clockwise; this was verified by probe, not assumed. Reversing the sign produces a control that works and does the opposite of its label. +- **All SQL is parameterized.** Never interpolate a value into a query string. +- **Every Express route handler is wrapped in `asyncRoute`.** `backend/tests/unit/routesAreWrapped.test.ts` fails the build otherwise. +- **antd imports are deep and from `antd/es/...`** (e.g. `import Button from 'antd/es/button'`), matching every existing file in `frontend/src/admin/`. Icons are named imports from `@ant-design/icons`. +- **Rotation rewrites the file; it never changes `item_images.image_path` and adds no column.** The paths are identical afterwards and only the bytes differ. +- **Success is 204.** There is no row worth returning. +- **Commit subjects end with `(#301)`. Commit bodies are never hard-wrapped** — write each paragraph as one long line. End every commit body with `Co-Authored-By: Claude Opus 5 `. +- **Do not push.** Commit locally and stop; the repository owner pushes. +- **Do not run `scripts/start-local.ps1` or `scripts/run-tests.ps1`.** They prompt for UAC and hang non-interactively. Run npm scripts directly from `backend/` and `frontend/`. +- Integration tests need the test database up (`npm run db:test:up` in `backend/`); if port 55432 is unavailable, override with `TEST_PGPORT`. + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `backend/src/imageProcessing.ts` (modify) | Gains `RotateDirection` and `rotateInPlace` — the file operation, no database. Sits beside `reencodeInPlace`, whose temp-file-and-rename shape it borrows. | +| `backend/tests/unit/imageRotation.test.ts` (create) | Proves the direction mapping, format preservation, the untouched-on-failure guarantee, and the animated-WebP refusal. | +| `backend/src/imageRotation.ts` (create) | `rotateItemImage` — reads the row, turns the displayed file, turns `original_image_path` when set. A new file rather than a sixth export on `backgroundRemoval.ts`: rotation and background removal share a table and nothing else. | +| `backend/src/routes/admin.ts` (modify) | Two POST routes, both built from one shared handler factory. | +| `backend/tests/integration/itemImageRotation.integration.test.ts` (create) | Both endpoints end to end, the cut-out-and-original pair, and every 404. | +| `frontend/src/admin/imagesApi.ts` (create) | `rotateImage` — the client for the item-scoped endpoints. Separate from `draftsApi.ts`, whose `send()` hardcodes the `/api/admin/item-drafts` prefix these routes do not use, and which the inventory editor imports unchanged in the follow-up. | +| `frontend/src/admin/DraftQueue.tsx` (modify) | Two buttons on `DraftPhoto`, plus the cache-busting ``. | +| `frontend/tests/e2e/admin-draft-queue.spec.ts` (modify) | The buttons appear, and pressing one does not error. | + +--- + +## Task 1: `rotateInPlace` + +**Files:** +- Modify: `backend/src/imageProcessing.ts` +- Test: `backend/tests/unit/imageRotation.test.ts` (create) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `export type RotateDirection = 'left' | 'right'` and `export async function rotateInPlace(filePath: string, mimetype: string, direction: RotateDirection): Promise`, both from `backend/src/imageProcessing.ts`. Task 2 imports both. + +- [ ] **Step 1: Write the failing tests** + +Create `backend/tests/unit/imageRotation.test.ts`: + +```ts +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import sharp from 'sharp'; +import { rotateInPlace } from '../../src/imageProcessing'; + +let dir = ''; + +beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'rotate-')); +}); + +afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); +}); + +/** + * A square image, left half red and right half blue. + * + * Square on purpose. Dimensions swap whichever way a quarter turn goes, so a + * rectangle proves that something rotated but says nothing about which way — + * only the pixels can say that, and this is the smallest picture that makes + * the direction visible. + */ +async function halvesPng(): Promise { + const red = await sharp({ + create: { width: 100, height: 200, channels: 3, background: { r: 255, g: 0, b: 0 } } + }) + .png() + .toBuffer(); + const blue = await sharp({ + create: { width: 100, height: 200, channels: 3, background: { r: 0, g: 0, b: 255 } } + }) + .png() + .toBuffer(); + return sharp({ + create: { width: 200, height: 200, channels: 3, background: { r: 0, g: 0, b: 0 } } + }) + .composite([ + { input: red, left: 0, top: 0 }, + { input: blue, left: 100, top: 0 } + ]) + .png() + .toBuffer(); +} + +/** The colour a little way in from the top-left corner, clear of any edge. */ +async function topLeft(file: string): Promise { + const { data, info } = await sharp(file).raw().toBuffer({ resolveWithObject: true }); + const at = (10 * info.width + 10) * info.channels; + return [data[at], data[at + 1], data[at + 2]].join(','); +} + +/** + * A two-frame animated WebP, built rather than committed as a binary fixture so + * what it contains is readable here. `pageHeight` is what makes libvips treat + * one tall buffer as a strip of frames. + */ +async function animatedWebp(): Promise { + const width = 40; + const height = 40; + const raw = Buffer.alloc(width * height * 2 * 4); + for (let i = 0; i < width * height * 4; i += 4) { + raw[i] = 255; + raw[i + 3] = 255; + } + for (let i = width * height * 4; i < raw.length; i += 4) { + raw[i + 2] = 255; + raw[i + 3] = 255; + } + return sharp(raw, { raw: { width, height: height * 2, channels: 4, pageHeight: height } }) + .webp() + .toBuffer(); +} + +describe('rotateInPlace', () => { + it('turns a landscape photo into a portrait one', async () => { + const file = path.join(dir, 'wide.jpg'); + await fs.writeFile( + file, + await sharp({ + create: { width: 400, height: 200, channels: 3, background: { r: 20, g: 60, b: 120 } } + }) + .jpeg() + .toBuffer() + ); + + await rotateInPlace(file, 'image/jpeg', 'right'); + + const after = await sharp(file).metadata(); + expect(after.width).toBe(200); + expect(after.height).toBe(400); + }); + + // The direction mapping, which is the one thing here that can be exactly + // backwards while every dimension assertion still passes. Turning clockwise + // moves the left edge to the top, so the red half ends up along the top. + it('turns right clockwise', async () => { + const file = path.join(dir, 'halves.png'); + await fs.writeFile(file, await halvesPng()); + + await rotateInPlace(file, 'image/png', 'right'); + + expect(await topLeft(file)).toBe('255,0,0'); + }); + + it('turns left anticlockwise', async () => { + const file = path.join(dir, 'halves.png'); + await fs.writeFile(file, await halvesPng()); + + await rotateInPlace(file, 'image/png', 'left'); + + expect(await topLeft(file)).toBe('0,0,255'); + }); + + // Format is preserved for the reason #226 preserved it: image_path carries + // the extension, so changing the format here would leave the row pointing at + // a file that no longer exists. + it('keeps the file in its own format', async () => { + const file = path.join(dir, 'halves.png'); + await fs.writeFile(file, await halvesPng()); + + await rotateInPlace(file, 'image/png', 'left'); + + expect((await sharp(file).metadata()).format).toBe('png'); + }); + + // The whole reason for the temp-file-and-rename: a failure must leave the + // photo exactly as it was, and must not leave a stray file behind either. + it('leaves the file untouched when sharp cannot read it', async () => { + const file = path.join(dir, 'broken.jpg'); + await fs.writeFile(file, 'not an image'); + + await expect(rotateInPlace(file, 'image/jpeg', 'right')).rejects.toThrow(); + + expect(await fs.readFile(file, 'utf8')).toBe('not an image'); + expect(await fs.readdir(dir)).toEqual(['broken.jpg']); + }); + + // The trap this guards. Reading an animated WebP without `animated: true` + // succeeds and yields the first frame alone, so a rotation that omitted the + // flag would write back a still and destroy the uploader's animation while + // reporting success. With the flag, sharp refuses — which is the correct + // answer, because a quarter turn of a multi-page image is not something it + // can do. + it('refuses an animated WebP rather than flattening it', async () => { + const file = path.join(dir, 'moving.webp'); + await fs.writeFile(file, await animatedWebp()); + + await expect(rotateInPlace(file, 'image/webp', 'right')).rejects.toThrow(/multi-page/i); + + expect((await sharp(file, { animated: true }).metadata()).pages).toBe(2); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd backend && npx jest -c jest.unit.config.js tests/unit/imageRotation.test.ts` +Expected: FAIL — TypeScript reports that `rotateInPlace` is not exported from `../../src/imageProcessing`. + +- [ ] **Step 3: Implement `rotateInPlace`** + +In `backend/src/imageProcessing.ts`, append after `reencodeInPlace` (which ends at the file's last line): + +```ts +/** Which way a quarter turn goes, in the words the buttons use. */ +export type RotateDirection = 'left' | 'right'; + +/** + * sharp reads a positive angle as clockwise, so the sign is the whole of the + * mapping — and getting it backwards produces a control that works perfectly + * and does the opposite of what its label says, which no dimension assertion + * would ever catch. + */ +const QUARTER_TURN: Record = { left: -90, right: 90 }; + +/** + * Turns the file at `filePath` a quarter turn, in its own format. + * + * The remedy for photos uploaded before #300 taught the re-encode to apply EXIF + * orientation instead of discarding it. Those files cannot be repaired + * automatically — the tag that said which way up they went is gone — so a + * person has to look at each one and decide. + * + * Rewrites the pixels rather than recording an angle. An angle would keep the + * bytes pristine, but it would put an obligation on every consumer — the + * storefront, both admin screens, the drafting worker's photo reader, and the + * rembg sidecar — and any one that forgot would show the photo sideways. The + * sidecar in particular is not ours to teach. + * + * Same temporary-file-and-rename shape as `reencodeInPlace`, for the same two + * reasons: sharp cannot read and write one path in a single pass, and a process + * that dies mid-write must leave the old photo intact rather than half a new + * one. + * + * No `resize`. The file went through `reencodeInPlace` on upload and is already + * within bounds, so re-applying the cap would be a second lossy pass buying + * nothing. No metadata handling either: the re-encode already stripped it, and + * there is nothing left to strip. + */ +export async function rotateInPlace( + filePath: string, + mimetype: string, + direction: RotateDirection +): Promise { + const temporary = `${filePath}.rotating`; + try { + await encoderFor( + // Exactly the `animated` argument reencodeInPlace uses, and the reason is + // sharper here. Reading an animated WebP without it decodes the first + // frame alone, so omitting it would silently write back a still and + // destroy the animation while reporting success. With it, sharp refuses + // the rotation outright — multi-page images can only be turned 180° — + // which is the honest answer and surfaces as a 500 with the file + // untouched. A still WebP has one page and turns normally. + sharp(filePath, mimetype === 'image/webp' ? { animated: true } : {}).rotate( + QUARTER_TURN[direction] + ), + mimetype + ).toFile(temporary); + + await fs.rename(temporary, filePath); + } catch (err) { + await fs.unlink(temporary).catch(() => undefined); + throw err; + } +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd backend && npx jest -c jest.unit.config.js tests/unit/imageRotation.test.ts` +Expected: PASS, 6 tests. + +- [ ] **Step 5: Verify the build and lint** + +Run: `cd backend && npm run build && npm run lint` +Expected: both clean. + +- [ ] **Step 6: Commit** + +```bash +git add backend/src/imageProcessing.ts backend/tests/unit/imageRotation.test.ts +git commit -F- <<'EOF' +feat(images): turn a stored photo a quarter turn (#301) + +The file half of the remedy for what #300 could only stop. Fixing the EXIF strip means new uploads arrive the way the sender saw them; it cannot repair what is already stored, because the tag that said which way up the pixels went is gone. Those photos need a person to look at each one and turn it. + +Rewrites the pixels rather than recording an angle, because an angle obliges every consumer to honour it — the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar — and any one that forgets shows the photo sideways. The sidecar is not ours to teach. + +Left is anticlockwise and right is clockwise, which is rotate(-90) and rotate(90); sharp reads a positive angle as clockwise. The direction test asserts a pixel rather than a dimension, because dimensions swap whichever way the turn goes — a reversed sign would pass every size assertion and ship a control that does the opposite of its label. + +The animated WebP case is the one that could destroy someone's file quietly. Reading such a file without the animated flag succeeds and hands back the first frame alone, so a rotation that omitted it would write a still back over the animation and report success. Passing the same flag reencodeInPlace passes makes sharp refuse instead — multi-page images turn only by 180° — which is the honest answer and leaves the file untouched. + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +## Task 2: `rotateItemImage` and the two endpoints + +**Files:** +- Create: `backend/src/imageRotation.ts` +- Modify: `backend/src/routes/admin.ts` +- Test: `backend/tests/integration/itemImageRotation.integration.test.ts` (create) + +**Interfaces:** +- Consumes: `rotateInPlace(filePath: string, mimetype: string, direction: RotateDirection): Promise` and `type RotateDirection = 'left' | 'right'`, both from `../imageProcessing` (Task 1). +- Produces: `POST /api/admin/items/:id/images/:imageId/rotate-left` and `POST /api/admin/items/:id/images/:imageId/rotate-right`, both answering 204 on success, `{ error: string }` with 404 or 500 otherwise. Task 3 calls these. + +- [ ] **Step 1: Write the failing integration tests** + +Create `backend/tests/integration/itemImageRotation.integration.test.ts`: + +```ts +import request from 'supertest'; +import sharp from 'sharp'; +import { promises as fs } from 'fs'; +import path from 'path'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +const UPLOADS_DIR = process.env.UPLOADS_DIR as string; + +beforeAll(async () => { + await fs.mkdir(UPLOADS_DIR, { recursive: true }); +}); + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** A 400x200 landscape JPEG — wide enough that a quarter turn is unmistakable. */ +async function landscapeJpeg(): Promise { + return sharp({ + create: { width: 400, height: 200, channels: 3, background: { r: 20, g: 60, b: 120 } } + }) + .jpeg() + .toBuffer(); +} + +async function writeUpload(name: string): Promise { + await fs.writeFile(path.join(UPLOADS_DIR, name), await landscapeJpeg()); + return `/uploads/${name}`; +} + +async function sizeOf(storedPath: string): Promise { + const meta = await sharp(path.join(UPLOADS_DIR, path.basename(storedPath))).metadata(); + return `${meta.width}x${meta.height}`; +} + +/** An item with one photo on disk. The prefix keeps each test's file its own. */ +async function seedItem(prefix: string): Promise<{ itemId: number; imageId: number }> { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Blue vase', 'available') RETURNING id` + ); + const itemId = rows[0]!.id; + const imagePath = await writeUpload(`${prefix}-front.jpg`); + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, 0) RETURNING id`, + [itemId, imagePath] + ); + return { itemId, imageId: image.rows[0]!.id }; +} + +describe('rotating one photo of an item', () => { + it('turns the file and answers 204', async () => { + const { itemId, imageId } = await seedItem('turns'); + + const res = await request(app).post( + `/api/admin/items/${itemId}/images/${imageId}/rotate-right` + ); + + expect(res.status).toBe(204); + expect(await sizeOf('/uploads/turns-front.jpg')).toBe('200x400'); + }); + + // The undo story. One press back is why the control offers both directions + // rather than making somebody turn the same button three times, each turn a + // further generation of JPEG loss. + it('turns back the other way', async () => { + const { itemId, imageId } = await seedItem('back'); + + await request(app).post(`/api/admin/items/${itemId}/images/${imageId}/rotate-right`); + await request(app).post(`/api/admin/items/${itemId}/images/${imageId}/rotate-left`); + + expect(await sizeOf('/uploads/back-front.jpg')).toBe('400x200'); + }); + + // Without this, Restore original would silently un-rotate the photo, turning + // the undo of #281 into a regression of #301. + it('turns the pristine original too', async () => { + const { itemId, imageId } = await seedItem('cutout'); + const originalPath = await writeUpload('cutout-original.jpg'); + await pool.query(`UPDATE item_images SET original_image_path = $2 WHERE id = $1`, [ + imageId, + originalPath + ]); + + const res = await request(app).post( + `/api/admin/items/${itemId}/images/${imageId}/rotate-right` + ); + + expect(res.status).toBe(204); + expect(await sizeOf('/uploads/cutout-front.jpg')).toBe('200x400'); + expect(await sizeOf(originalPath)).toBe('200x400'); + }); + + // An image id is a serial, so guessing one is easy — the photo has to belong + // to the item named in the path (#207). + it('refuses a photo belonging to another item', async () => { + const mine = await seedItem('mine'); + const theirs = await seedItem('theirs'); + + const res = await request(app).post( + `/api/admin/items/${mine.itemId}/images/${theirs.imageId}/rotate-right` + ); + + expect(res.status).toBe(404); + expect(await sizeOf('/uploads/theirs-front.jpg')).toBe('400x200'); + }); + + it('answers 404 for an unreadable id', async () => { + const { itemId } = await seedItem('unreadable'); + + const res = await request(app).post(`/api/admin/items/${itemId}/images/abc/rotate-right`); + + expect(res.status).toBe(404); + }); + + it('answers 404 for an absent item', async () => { + const res = await request(app).post('/api/admin/items/999999/images/1/rotate-left'); + + expect(res.status).toBe(404); + }); + + // Reported rather than swallowed: a row pointing at a file that is not there + // is a real fault, and answering "done" would hide it. + it('answers 500 when the file is missing from disk', async () => { + const { itemId, imageId } = await seedItem('missing'); + await fs.rm(path.join(UPLOADS_DIR, 'missing-front.jpg')); + + const res = await request(app).post( + `/api/admin/items/${itemId}/images/${imageId}/rotate-right` + ); + + expect(res.status).toBe(500); + expect(res.body.error).toMatch(/could not be rotated/); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd backend && npm run db:test:up && npx jest -c jest.integration.config.js --runInBand tests/integration/itemImageRotation.integration.test.ts` +Expected: FAIL — every test gets a 404 from Express, because no such route is registered. + +- [ ] **Step 3: Write `imageRotation.ts`** + +Create `backend/src/imageRotation.ts`: + +```ts +import path from 'path'; +import { pool } from './db'; +import { typeForExtension } from './uploadTypes'; +import { rotateInPlace, RotateDirection } from './imageProcessing'; + +/** + * Turning one stored photo, and everything that photo is stored alongside. + * + * Its own module rather than another export on backgroundRemoval.ts: the two + * features share a table and nothing else. Rotation touches no sidecar, records + * no provenance, and is not idempotent — bundling them would put a function + * with none of that module's invariants under its documentation. + */ + +interface ImageRow { + image_path: string; + original_image_path: string | null; +} + +/** + * Thrown when the photo is not on that item. + * + * A named class for the same reason NoOriginalToRestoreError is one: the route + * has to tell "no such photo" apart from "the application is in trouble", and + * the two need opposite replies. Everything else stays loud. + */ +export class ImageNotOnItemError extends Error {} + +/** + * basename only. `image_path` is stored as `/uploads/` and the directory + * it lives in is a server constant — the same rule readPhotos and + * removeImageBackground both follow. + */ +async function rotateStoredFile(storedPath: string, direction: RotateDirection): Promise { + const name = path.basename(storedPath); + const mediaType = typeForExtension(path.extname(name)); + if (mediaType === null) { + throw new Error(`cannot rotate ${name}: unrecognised extension`); + } + await rotateInPlace(path.join(process.env.UPLOADS_DIR ?? '', name), mediaType, direction); +} + +/** + * Turns one photo of one item, and its pristine original when it has one. + * + * Both files or neither is not achievable here and is not claimed. What matters + * is that they cannot end up disagreeing silently: an image that has been + * through #281 has a displayed cut-out and a recorded original, and rotating + * only the first would leave Restore original quietly un-rotating the photo — + * the undo of one feature becoming a regression of another. + * + * The displayed file goes first, so a failure on the original leaves the admin + * looking at a photo that visibly moved, with the failure reported. A retry + * would turn the displayed file a second time: rotation is not idempotent the + * way removeImageBackground is, because nothing records how far the last + * attempt got. That is accepted rather than engineered around — it takes the + * disk failing between two writes, and the remedy is one press in the other + * direction. + * + * No column is written. The paths are the same afterwards; only the bytes + * differ. + */ +export async function rotateItemImage( + itemId: number, + imageId: number, + direction: RotateDirection +): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1 AND item_id = $2`, + [imageId, itemId] + ); + const row = rows[0]; + if (!row) { + throw new ImageNotOnItemError(`no image ${imageId} on item ${itemId}`); + } + + await rotateStoredFile(row.image_path, direction); + if (row.original_image_path !== null) { + await rotateStoredFile(row.original_image_path, direction); + } +} +``` + +- [ ] **Step 4: Add the routes** + +In `backend/src/routes/admin.ts`, add beside the existing `backgroundRemoval` import: + +```ts +import { rotateItemImage, ImageNotOnItemError } from '../imageRotation'; +import { RotateDirection } from '../imageProcessing'; +``` + +Then add immediately before the final `export default router;`: + +```ts +/** + * Turn one photo a quarter turn. + * + * On the item rather than on the draft, which is the decision that makes the + * inventory editor free later: an image belongs to an item whether or not a + * draft row exists, so the second screen to want this is the same call from a + * different place, with no new backend at all. + * + * Unlike the per-item background endpoints, this acts on exactly one file, so + * it can honestly answer whether it worked. 204 rather than 200 because + * rotation changes no column — the paths are identical afterwards and only the + * bytes differ, so there is no row worth returning, which is also why + * DELETE /items/:id/images/:imageId is a 204. + * + * A factory rather than two copied handlers: the direction is the only thing + * that differs. Two paths rather than one endpoint taking a direction in the + * body matches how remove-background and restore-original are already spelled. + */ +function rotationRoute(direction: RotateDirection) { + return asyncRoute(async (req: Request, res: Response) => { + // Both ids, not just the first. A route carrying two of them can guard one + // and forget the other, and the forgotten one fails as a 500 rather than + // the 404 that "no such photo" actually means (#207). + const itemId = readId(req.params.id); + const imageId = readId(req.params.imageId); + if (itemId === null || imageId === null) { + return res.status(404).json({ error: 'no such photo on this item' }); + } + + try { + await rotateItemImage(itemId, imageId, direction); + } catch (err) { + // Only "not on this item" is a 404, and it is indistinguishable from an + // absent one on purpose: an image id is a serial, and confirming which + // ids exist is not something this endpoint should do. Everything else is + // a real fault and stays loud — the file is untouched in every one of + // those cases, because rotateInPlace renames over the original only once + // the new file has been written successfully. + if (err instanceof ImageNotOnItemError) { + return res.status(404).json({ error: 'no such photo on this item' }); + } + console.error(`[rotation] item ${itemId}, image ${imageId}:`, err); + return res.status(500).json({ + error: + err instanceof Error + ? `this photo could not be rotated: ${err.message}` + : 'this photo could not be rotated' + }); + } + + res.status(204).end(); + }); +} + +router.post('/items/:id/images/:imageId/rotate-left', rotationRoute('left')); +router.post('/items/:id/images/:imageId/rotate-right', rotationRoute('right')); +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `cd backend && npx jest -c jest.integration.config.js --runInBand tests/integration/itemImageRotation.integration.test.ts` +Expected: PASS, 7 tests. + +- [ ] **Step 6: Verify nothing else broke** + +Run: `cd backend && npm run build && npm run lint && npm run test:unit` +Expected: all clean. `routesAreWrapped.test.ts` in particular must still pass — it is what proves both new handlers are wrapped in `asyncRoute`. + +- [ ] **Step 7: Commit** + +```bash +git add backend/src/imageRotation.ts backend/src/routes/admin.ts backend/tests/integration/itemImageRotation.integration.test.ts +git commit -F- <<'EOF' +feat(admin): endpoints to rotate one photo of an item (#301) + +Two POST routes and the module behind them. They live on the item rather than on the draft, and that is the decision that makes the inventory editor free when it follows: an image belongs to an item whether or not a draft row exists, so the second screen to want this is the same call from a different place with no new backend at all. + +A cut-out and its pristine original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then quietly un-rotate the photo, so the undo of one feature becomes a regression of another. + +204 rather than 200. Rotation changes no column: the paths are identical afterwards and only the bytes differ, so there is no row worth returning, which is the same reason deleting an image is already a 204. + +Only "not on this item" is a 404, and it is indistinguishable from an absent id on purpose, because an image id is a serial and this endpoint should not confirm which ones exist. Everything else stays loud as a 500, and the file is untouched in every one of those cases — rotateInPlace renames over the original only once the new file has been written. + +One asymmetry is recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a failure between the two files turns the displayed one twice. That needs the disk to break between two writes, and the remedy is one press in the other direction. + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +## Task 3: The buttons in the review queue + +**Files:** +- Create: `frontend/src/admin/imagesApi.ts` +- Modify: `frontend/src/admin/DraftQueue.tsx` +- Test: `frontend/tests/e2e/admin-draft-queue.spec.ts` (modify) + +**Interfaces:** +- Consumes: `POST /api/admin/items/:id/images/:imageId/rotate-left` and `.../rotate-right`, answering 204 on success and `{ error: string }` with 404 or 500 otherwise (Task 2). +- Produces: `export type RotateDirection = 'left' | 'right'` and `export async function rotateImage(itemId: number, imageId: number, direction: RotateDirection): Promise` from `frontend/src/admin/imagesApi.ts` — the inventory editor imports these unchanged in the follow-up. + +- [ ] **Step 1: Write the client** + +Create `frontend/src/admin/imagesApi.ts`: + +```ts +/** + * The client for the item-scoped image endpoints. + * + * Separate from draftsApi.ts, whose `send()` hardcodes the + * `/api/admin/item-drafts` prefix these routes deliberately do not use. The + * review queue is only the first screen to want rotation; the inventory editor + * imports this same module when it follows, which is the whole reason the + * endpoints were put on the item. + */ + +export type RotateDirection = 'left' | 'right'; + +/** + * Turn one photo a quarter turn. + * + * The server's message is preferred over a generic one for the same reason + * publishDraft prefers it: its refusals name the actual problem — a file + * missing from the uploads volume, an animated image that cannot be turned a + * quarter turn — and replacing those with "could not rotate" throws away the + * only thing that says what to do next. + */ +export async function rotateImage( + itemId: number, + imageId: number, + direction: RotateDirection +): Promise { + const res = await fetch(`/api/admin/items/${itemId}/images/${imageId}/rotate-${direction}`, { + method: 'POST' + }); + if (res.ok) return; + + let message = 'could not rotate this photo'; + try { + message = (await res.json()).error ?? message; + } catch { + // A non-JSON body is a proxy or gateway error rather than the app refusing. + // The generic message is the honest thing to show in that case. + } + throw new Error(message); +} +``` + +- [ ] **Step 2: Add the buttons to `DraftPhoto`** + +In `frontend/src/admin/DraftQueue.tsx`, add to the imports: + +```ts +import { RotateLeftOutlined, RotateRightOutlined } from '@ant-design/icons'; +import { rotateImage, RotateDirection } from './imagesApi'; +``` + +Append these two paragraphs to `DraftPhoto`'s existing doc comment, immediately before its closing `*/`: + +``` + * The rotation buttons are not gated on `enabled`. That flag is about the + * background-removal sidecar, and rotation has nothing to do with it — turning + * a photo is a local file operation that works in every environment, including + * one where REMBG_URL was never set. + * + * Icon-only, with an aria-label rather than visible text: three labelled + * buttons under a 120px thumbnail is more furniture than the photo, and a + * button with no text has no accessible name at all without one. +``` + +Then replace the body of `DraftPhoto` — everything from `const [busy, setBusy] = useState(false);` down to and including the closing `);` and `}` of the component — with: + +```tsx + const [busy, setBusy] = useState(false); + const [turning, setTurning] = useState(false); + const cutOut = image.original_image_path !== null; + + // Rotation does not change image_path, so after a successful turn the src is + // byte-for-byte the string the browser already holds a copy for, and the + // photo appears not to have moved. express.static is mounted with no maxAge + // and would serve the new bytes on a full page reload — but nothing in a + // session asks it to. This is what makes the button visibly do something. No + // column and no server change: the file's identity has not changed, only this + // page's need to see it again. + const [version, setVersion] = useState(0); + const src = version === 0 ? image.image_path : `${image.image_path}?v=${version}`; + + const act = async () => { + setBusy(true); + try { + await setImageBackground(itemId, image.id, cutOut ? 'restore-original' : 'remove-background'); + onChanged(); + } catch (err) { + message.error(err instanceof Error ? err.message : 'that did not work'); + } finally { + setBusy(false); + } + }; + + // No onChanged(): rotation changes nothing in the queue payload, so + // refetching it would be a request that returns exactly what is on screen. + const turn = async (direction: RotateDirection) => { + setTurning(true); + try { + await rotateImage(itemId, image.id, direction); + setVersion(Date.now()); + } catch (err) { + message.error(err instanceof Error ? err.message : 'that did not work'); + } finally { + setTurning(false); + } + }; + + return ( + + + + + )} + + ); +} +``` + +- [ ] **Step 3: Verify the frontend builds** + +Run: `cd frontend && npm run build` +Expected: clean. This is the type check that matters — a bare `npx tsc --noEmit` does not see `tests/` and has let a broken deploy through before. + +- [ ] **Step 4: Add the e2e coverage** + +In `frontend/tests/e2e/admin-draft-queue.spec.ts`, add inside the existing `test.describe('The review queue', ...)` block, immediately after the `offers to remove the background on each photo` test: + +```ts + // Rotation is what repairs the photos uploaded before #300 taught the + // re-encode to apply EXIF orientation rather than discard it. The 1x1 PNG + // this spec uploads is square, so there is nothing visual to assert — what is + // being proved is that the control is there and the whole path answers + // without an error. + test('offers to rotate each photo', async ({ page, admin }) => { + const note = `Sideways ${RUN}`; + await submitAnItem(page, note); + + await admin.open('Review queue'); + + const card = page.locator('.ant-card').filter({ hasText: note }); + await expect(card.getByRole('button', { name: 'Rotate left' })).toBeVisible(); + await expect(card.getByRole('button', { name: 'Rotate right' })).toBeVisible(); + + // Asserted on the response rather than on the absence of an error toast. + // toHaveCount(0) passes the instant it is evaluated, before a failure has + // had time to appear, so it would report success on a broken round trip — + // which is barely an assertion at all. Waiting for the POST proves the + // whole path: the button is wired, the route exists, and it answered 204. + const rotated = page.waitForResponse( + (res) => res.url().includes('/rotate-right') && res.request().method() === 'POST' + ); + await card.getByRole('button', { name: 'Rotate right' }).click(); + expect((await rotated).status()).toBe(204); + }); +``` + +- [ ] **Step 5: Run the e2e spec** + +Run: `cd frontend && npx playwright test tests/e2e/admin-draft-queue.spec.ts --project=chromium` +Expected: PASS. This needs the local stack already running — if it is not, ask the repository owner to start it rather than running `scripts/start-local.ps1`, which prompts for UAC and hangs. + +- [ ] **Step 6: Commit** + +```bash +git add frontend/src/admin/imagesApi.ts frontend/src/admin/DraftQueue.tsx frontend/tests/e2e/admin-draft-queue.spec.ts +git commit -F- <<'EOF' +feat(admin): rotate a photo from the review queue (#301) + +Two icon buttons under every thumbnail, and the client for the item-scoped endpoints behind them. Icon-only with an aria-label rather than visible text, because three labelled buttons under a 120px thumbnail is more furniture than the photo — and a button with no text has no accessible name at all without one. + +They are not gated on the background-removal flag. That flag is about the sidecar, and rotation has nothing to do with it: turning a photo is a local file operation that works in every environment, including one where REMBG_URL was never set. + +The cache-busting src is the part most likely to have shipped broken. Rotation does not change image_path, so after a successful turn the src is byte-for-byte the string the browser already holds a copy for, and the photo would appear not to have moved. express.static is mounted with no maxAge and would serve the new bytes on a full page reload, but nothing in a session asks it to. A version held in component state is what makes the button visibly do something, and it needs no column and no server change, because the file's identity has not changed — only this page's need to see it again. + +The client lives in its own module rather than in draftsApi, whose send() hardcodes the item-drafts prefix these routes deliberately do not use. The inventory editor imports this same module unchanged when it follows, which is the whole reason the endpoints went on the item. + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +## Self-Review + +**Spec coverage:** + +| Spec requirement | Task | +|---|---| +| Rotation rewrites the file | 1 | +| Both directions; left anticlockwise, right clockwise | 1 (mapping and pixel assertions), 3 (buttons) | +| An animated WebP is refused rather than flattened | 1 | +| No resize, no metadata handling | 1 | +| Per photo, not per item | 3 (the control sits on `DraftPhoto`) | +| A cut-out and its original rotate together | 2 | +| The endpoints live on the item, not the draft | 2 | +| `rotateInPlace` a sibling of `reencodeInPlace`, temp file and rename | 1 | +| `rotateItemImage` in a new module | 2 | +| Two paths rather than a direction in the body | 2 | +| 204 on success, 500 with a real message | 2 | +| 404 for unreadable, absent, or another item's image | 2 | +| The cache-busting query parameter | 3 | +| Buttons on `DraftQueue`'s `DraftPhoto` | 3 | +| Unit / integration / e2e testing | 1 / 2 / 3 | +| Inventory editor out of scope and needs no backend | 2 (endpoints on the item), 3 (client in its own module) | + +No gaps. + +**Placeholder scan:** none. Every code step carries the literal code; every run step carries the literal command and the expected result. + +**Type consistency:** `RotateDirection` is defined once on the backend (`src/imageProcessing.ts`, Task 1) and imported by `src/imageRotation.ts` and `src/routes/admin.ts` (Task 2). The frontend declares its own in `imagesApi.ts` (Task 3), which is correct — separate packages, no shared types. `rotateInPlace(filePath, mimetype, direction)`, `rotateItemImage(itemId, imageId, direction)` and `rotateImage(itemId, imageId, direction)` are each named consistently everywhere they appear, and the backend and frontend rotate functions are deliberately named differently so the two never read as the same function.