docs(admin): design rotating a photo from the admin (#301)
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 that metadata is gone and the originals kept for #281's cut-outs were re-encoded on the way in too. Every portrait photo since #226 needs a person to look at it and turn it. Rotation rewrites the file rather than recording an angle. Storing an angle keeps the bytes pristine and makes undo exact, but it puts an obligation on every consumer — 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 in particular is not ours to teach. Rewriting means nothing else in the system has to know rotation exists, and the cost is bounded: one rotation is a second generation at quality 82, which is why the control offers both directions rather than making somebody press one button three times to undo. Per photo, and that is deliberately the opposite of what #293 decided for background removal. The reason there does not carry: three photos of a vase can each be wrong in a different direction, so turning them together would fix one and break two. A cut-out and its 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 silently un-rotate the photo, turning the undo of one feature into a regression of another. The endpoints go on the item rather than the draft, so the inventory editor needs no backend work at all when it follows: an image belongs to an item whether or not a draft row exists, and the second screen is then the same call from a different place. Two things pinned so they are not settled by a coin-flip while implementing. Left is anticlockwise and right is clockwise, which is sharp.rotate(-90) and sharp.rotate(90) — sharp reads a positive angle as clockwise, so the sign is the whole mapping and reversing it produces a control that works and does the opposite of its label. And the displayed image has to be forced to reload: rotation does not change image_path, so the img src is identical afterwards and the browser keeps what it has. express.static is mounted with no maxAge and would serve the new bytes on a page reload, but nothing in a session asks it to, so the src gets a cache-busting parameter after a successful turn. One honest asymmetry recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a half-done failure turns the displayed file 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 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,92 @@
|
||||
# Rotating a photo from the admin
|
||||
|
||||
**Issue:** #301. Follows #300, which fixed the cause; this is the remedy for the photos that cause has already damaged.
|
||||
|
||||
#300 stopped the EXIF strip from discarding orientation without applying it, so new uploads arrive the way the sender saw them. It cannot repair what is already stored: that metadata is gone, and the originals kept for #281's cut-outs were re-encoded on the way in too. Every portrait photo uploaded since #226 needs a person to look at it and turn it.
|
||||
|
||||
## Decisions, and what each one rests on
|
||||
|
||||
**Rotation rewrites the file.** The alternative — storing an angle and applying it on read — keeps the bytes pristine and makes undo exact, but it puts an obligation on every consumer: the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar. Any one of them that forgets shows the photo sideways, and the sidecar is not ours to teach. Rewriting means nothing else in the system has to know rotation exists.
|
||||
|
||||
The cost is real and bounded: JPEG re-encoding is lossy, so a rotation is a second generation at quality 82. That is imperceptible once, and the control offers both directions precisely so nobody has to press one button three times to undo.
|
||||
|
||||
**Both directions.** Rotate left and rotate right. With a rewrite, undo is "turn it back", and one press should do it — three presses to undo one mistake would be three more generations of loss for no reason.
|
||||
|
||||
Pinned so it is not decided by a coin-flip in the implementation: **left is anticlockwise, `sharp.rotate(-90)`; right is clockwise, `sharp.rotate(90)`**, matching what the icons say and what every photo viewer means by those words. sharp takes a positive angle as clockwise, so the sign is the whole of the mapping and getting it backwards produces a control that works and does the opposite of what it says.
|
||||
|
||||
**Per photo, not per item.** This is deliberately the opposite of what #293 decided for background removal, and for a reason that does not carry over: three photos of a vase can each be wrong in a different direction, so rotating them together would fix one and break two. Background removal is one job across an upload; rotation is three separate judgements about three separate photographs.
|
||||
|
||||
**A cut-out and its original rotate together.** An image that has been through #281 has two files: the one being displayed and the pristine original recorded in `original_image_path`. Rotating only the displayed one leaves them disagreeing, and **Restore original** would then silently un-rotate the photo — turning an undo of one feature into a regression of another. Both files turn.
|
||||
|
||||
**The endpoint lives on the item, not the draft.** The review queue is the first screen to get the control, but an image belongs to an item whether or not a draft row exists. Putting rotation on `/api/admin/items/:id/images/:imageId/...` means the inventory editor needs no new backend at all when it follows — it is the same call from a different screen.
|
||||
|
||||
## Architecture
|
||||
|
||||
```
|
||||
Admin | Review queue → a photo → ↺ ↻
|
||||
│
|
||||
▼
|
||||
POST /api/admin/items/:id/images/:imageId/rotate-left
|
||||
POST /api/admin/items/:id/images/:imageId/rotate-right
|
||||
│
|
||||
▼
|
||||
imageRotation.ts rotateItemImage(imageId, direction)
|
||||
│ ├─ the displayed file
|
||||
│ └─ original_image_path, when there is one
|
||||
▼
|
||||
imageProcessing.ts rotateInPlace(filePath, mimetype, direction)
|
||||
```
|
||||
|
||||
### `rotateInPlace`
|
||||
|
||||
A sibling of `reencodeInPlace` in `backend/src/imageProcessing.ts`, and it borrows that function's shape for the same reasons: sharp cannot read and write one path in a single pass, so it writes a sibling temporary file and renames over the original, which also means a process that dies mid-write leaves the old photo intact rather than half a new one.
|
||||
|
||||
It does **not** resize. The file is already bounded — it went through `reencodeInPlace` on upload — and re-applying the bound would be a second lossy pass for nothing. It does not strip metadata either, because there is none left to strip.
|
||||
|
||||
**An animated WebP cannot be rotated a quarter turn, and must not be silently flattened.** sharp documents that multi-page images rotate only by 180°, and a probe confirms `.rotate(90)` on an animated WebP throws `Rotate is not supported for multi-page images`. The trap is the other branch: reading the same file *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 — the identical failure `reencodeInPlace`'s comment already warns about. So `rotateInPlace` passes exactly the same `animated` argument that function does, and sharp's own refusal becomes the 500. No extra guard: the one unsafe case is the one the library already rejects, and a still WebP read with the flag has a single page and rotates normally.
|
||||
|
||||
### `rotateItemImage`
|
||||
|
||||
Reads the image row, rotates `image_path`, and rotates `original_image_path` when it is set. No database column changes: the paths are the same afterwards, only the bytes differ.
|
||||
|
||||
### The endpoints
|
||||
|
||||
Two paths rather than one endpoint taking a direction in the body, matching how `/remove-background` and `/restore-original` are already spelled in #281. `readId` on both ids, 404 for an unreadable or absent one, and the image must belong to the item — the same ownership check those endpoints use, for the same reason: an image id is a serial and guessing one is easy.
|
||||
|
||||
Unlike the per-item background endpoints, these act on exactly one file, so they can honestly answer whether it worked: success, or 500 with a real message when sharp cannot read the file.
|
||||
|
||||
Success is **204**. Rotation changes no column — the paths are identical afterwards and only the bytes differ — so there is no row worth returning, and the same reasoning already makes `DELETE /items/:id/images/:imageId` a 204. The screen does not need a body; it needs to know it may re-request the file.
|
||||
|
||||
### The screen
|
||||
|
||||
Two small buttons on each photo in `DraftQueue`'s `DraftPhoto`, beside the background control that is already there.
|
||||
|
||||
**The displayed image must be forced to reload.** This is the detail most likely to ship broken. Rotation does not change `image_path`, so the `<img src>` is identical afterwards and the browser keeps the copy it already has — the photo appears not to have moved. `express.static` is mounted with no `maxAge`, so it sends `ETag` and `Last-Modified` and a full page reload would fetch the new bytes; but within a session nothing asks it to. The fix is a cache-busting query parameter appended to the `src` after a successful rotation, held in component state. No new column and no server change: the file's identity has not changed, only this page's need to see it again.
|
||||
|
||||
## Failure handling
|
||||
|
||||
| What happens | Result |
|
||||
|---|---|
|
||||
| Unreadable or absent item or image id | 404. Nothing touched. |
|
||||
| The image does not belong to that item | 404, indistinguishable from absent. |
|
||||
| sharp cannot read the file | 500 naming the problem. The file is untouched — the rename only happens after a successful write. |
|
||||
| The rotation succeeds but the original cannot be rotated | The displayed file is already turned. Reported as a failure so it is not silently half-done, and a retry rotates only what is still wrong — see below. |
|
||||
| Everything works | 204. The screen re-requests the image. |
|
||||
|
||||
The half-done case is the one worth stating plainly: a retry after it would rotate the displayed file **again**, because nothing records how far the last attempt got. Rotation is not idempotent the way background removal is. That is accepted rather than engineered around — the operation is one file write on a local volume, the failure needs the disk to break between two calls, and the remedy is visible and one press away in the other direction.
|
||||
|
||||
## Testing
|
||||
|
||||
- **Unit:** `rotateInPlace` turns a landscape image into a portrait one and back, preserves the format, and leaves the original file untouched when sharp throws.
|
||||
- **Integration:** both endpoints on an item with a photo; the dimensions swap; a cut-out and its original both turn; 404 for a bad id, an absent item, and an image belonging to another item.
|
||||
- **E2E:** the two buttons appear on a photo in the review queue.
|
||||
|
||||
## Out of scope
|
||||
|
||||
**The inventory editor.** It gets the same buttons in a second pass, and needs no backend work because the endpoints are already on the item.
|
||||
|
||||
**Arbitrary angles, cropping, straightening.** Quarter turns only.
|
||||
|
||||
**Rotating in bulk, or re-running anything across the catalogue.** Every affected photo needs a person to decide which way is up, which is the whole reason this exists.
|
||||
|
||||
**Regenerating a draft after a rotation.** A photo turned after the model has described it does not re-run the worker. The admin has a Regenerate button already, and using it is a decision rather than a side effect.
|
||||
Reference in New Issue
Block a user