Three of the items on the cleanup issue, and the first is the one that mattered.
routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed.
Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone.
The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added.
The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it.
An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last open item on #208, and the only one that needed a person rather than a patch. Local runs stay pinned to 26.7.0 while CI and the production image run 20, and CI on 20 is the backstop.
The comment said this was "an open decision rather than an oversight", which was true when it was written and is not any more. Left as-is it would read to the next person as something still to settle, and they would either re-litigate it or quietly change the pin.
What the decision costs is written down rather than glossed: passing locally does not mean it ships, because a post-20 syntax or node: API is caught after a push rather than before one. That is the whole of the trade, and it is acceptable precisely because it is known — the failure mode this file's own docstring warns about is the one nobody knew they were exposed to.
Items 1, 2, 3 and 5 were already done in 06933ae and the commits around it: the alias check fails closed on a positive match, three stale documents were corrected, the throw quotes `nvm install $Version`, Use-NodeLatest is gone, DEFAULT_NODE_VERSION sits beside NODE_VERSION rather than being duplicated in two scripts, and the floor check runs before the switch where it can actually fire.
Closes#208
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.$castTo<AdminItemRow>()` / `.$castTo<PublicItemRow>()` at the two list-query call sites replaced the entire result type with an assertion rather than narrowing the one column that actually disagreed, which meant a projection that silently lost a column would still compile — exactly the failure mode this task exists to close, and the opposite of what the commit body claims.
Fixed at the source instead: `itemSelect.ts` now defines `ItemsWithStatus`/`ItemDB`, narrowing `items.status` from the schema mirror's `Generated<string>` (a CHECK-constrained text column, so `kysely-codegen` has no literal union to give it) to `Generated<ItemStatus>`, and builds `ItemContext`, `adminItemQuery()` and `publicItemQuery()` from an `itemDb` typed with `ItemDB` instead of `db`/`DB`. Both `$castTo` calls and their comments are gone; the `AdminItemRow[]` / `PublicItemRow[]` annotations at the two call sites now check for real. Verified by temporarily dropping a column from `adminItemQuery`'s projection: the `AdminItemRow[]` assignment failed to compile as expected, confirming the guarantee actually holds.
Also corrected two now-false statements left over from the conversion: `db-kysely/CONVENTIONS.md`'s worked-example section said `buildItemFilterSql` was "still raw `pg`" and that converting it "would put a second copy of a live function in `src/` that nothing calls" — both untrue since #308 shipped it as `itemFilterExpressions`. And two doc comments in `itemSelect.ts` still named the deleted `PUBLIC_ITEM_SELECT`/`ADMIN_ITEM_SELECT` constants instead of the functions that replaced them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in.
All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one.
The second thing this buys may matter more than the first. pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error.
The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it.
The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one.
Closes#308
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One task, and that is a decision rather than a shortcut. itemSelect.ts, itemFilters.ts, both routes and the unit test file are coupled — the exports the routes call are the ones being replaced, and #298 put the test file under a tsconfig that type-checks it, so any partial commit is a red build.
Every line of it was verified by probe against the real generated schema before it was written, not sketched. The projections type-check, jsonArrayFrom correlates through whereRef, the mixed array of sql templates and builder expressions composes under eb.and, and the emitted SQL is quoted in the steps so a wrong result is caught at the step that produces it rather than three steps later. The probe also settled the question the spec left open with a fallback: the row type is assignable to the hand-written contract, so no cast is needed.
The two invariant tests are rewritten rather than ported. They used to read the clause strings the builder returned; they now compile the expressions against the same items-and-categories shape the real queries use and assert on the SQL Kysely emits, with the hostile value present in the parameters and absent from the text. Building a narrower query in the helper would have needed a cast, and a cast in that test would be testing the cast.
The step that verifies the conversion is the one that runs the integration suite unedited. Those tests are the contract — same JSON, same ordering, same statuses — so the plan says plainly that a test needing an edit means the query changed behaviour and the query is what to fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The work #305 made possible and deliberately did not do. These are the two queries the builder was ever wanted for: admin.ts and items.ts both splice a run-time-composed where clause into query text, and they are the only places S2077 has a real point after #294 hoisted the seven fixed-shape queries into named constants. They are safe today, and itemFilters.ts spells out why in sixteen lines — which is the problem, because a property that takes sixteen lines to explain is one an edit can quietly break.
All four call sites convert rather than only the two flagged ones. The by-id constants carry no hotspot and are already safe, but they are built by interpolating the same projection strings the list queries use, so converting only the list queries would leave itemSelect.ts holding a Kysely builder and a raw string that must produce an identical projection — two spellings to keep in step by hand where the file's own header already warns about one.
The filter builder returns an array of expressions rather than taking a query builder and returning it filtered, because the two callers do different things with the result: the storefront prepends its own not-pending clause and the admin route does not. A function that owned the builder would have to be told about that difference. startIndex disappears with the splicing it existed for.
The aggregate subqueries become jsonArrayFrom, which emits the same coalesce(json_agg(agg), '[]') they hand-write today. That is the second thing this buys and it may matter more than the first: pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why itemSelect.ts's header says the selects and their types are kept in step by hand and the integration suite is the only thing that catches a drop. Afterwards that is a compile error.
The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused on purpose: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to.
The two invariant tests survive and get stronger. They currently inspect the clause strings the function returns; afterwards they compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim tested against the real artefact instead of an intermediate one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The eslint config's ignores list and comment still named the deleted src/db-drizzle/schema.ts and relations.ts and never named src/db-kysely/schema.ts, so the generated mirror was being linted for the first time and tripping sonarjs/redundant-type-aliases — exactly the trap the config's own comment already described from #261 and #217. The ignores list now names src/db-kysely/schema.ts and the comment is updated to match.
The schema mirror drift test built one flat Set of every two-space-indented key in the whole generated file and asked only whether a live column name appeared anywhere in it, rather than checking it against the specific table it belongs to. Seventeen column names are declared on two or more tables and created_at is on fourteen of eighteen, so a migration adding created_at, updated_at, status, name, sort_order, token, or expires_at to a table that lacks it would pass vacuously. Replaced mirroredTables with mirroredColumns, which reads the DB interface to map each table name to its declaring interface and then reads that interface's own columns, and changed the column-mirroring test to look up columns per table. Verified the guard can actually fail: removing customer_id from the Carts interface made the test fail naming carts.customer_id exactly, and restoring the file made it pass again.
The root .gitignore still carried a comment block and two patterns for drizzle-kit pull output under backend/src/db-drizzle, a directory this branch deleted along with backend/drizzle.config.ts. kysely-codegen writes only the single tracked file it's pointed at, so nothing replaces the rule — deleted the block and both patterns.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The paragraph describing buildItemFilterSql claimed "nothing imports it" but it is actually a live production function defined at src/itemFilters.ts:264 and imported by both src/routes/admin.ts and src/routes/items.ts. The conversion example in the conventions file was mistakenly described as though it were the function itself rather than as an example demonstrating the query pattern. Fixed the wording to clarify that the function remains raw pg code and the shown conversion is an example of how to convert it, not a committed version in src/ waiting to be called.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the Drizzle conventions, and is much shorter, because three of that document's four warnings described the library rather than the practice and stopped being true when the library changed. An array is one bind parameter with no ceremony, a column reference in a raw fragment is the text you wrote, and the generated names are the database's own so nothing needs mapping back.
What survives is what was never about Drizzle. The mirror is generated and refreshing it is manual, so the drift test is the thing that catches forgetting — and it exists because the drift already happened once and nobody noticed for a week. Both drivers share one pool, because a transaction on a second pool would be invisible to the first and the limits would silently double. Migrations stay hand-written, and the reasoning survives the change of library: only the expression-index complaint was specific to drizzle-kit, while losing the prose and being unable to express data migrations are true of any generator.
One warning is genuinely new, and it is the inverse of an old one: driver errors are no longer wrapped, so a SQLSTATE sits on err.code again. That is worth stating precisely because it was not true before, and the last time it moved it turned a handled 409 into a 500 with nothing failing to compile.
The worked example moved into this file rather than staying a source file nothing imports. It is documentation, and it was only ever documentation.
Closes#305
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The #217 doc comment above the describe block still named db-drizzle/schema.ts, drizzle-kit pull, and "Drizzle infers row types" — a file, a command, and a library this same commit had already removed. A comment pointing at deleted paths is worse than no comment at all on a test whose whole job is proving trust in a generated mirror, so it is corrected to name npm run db:types and src/db-kysely/schema.ts while keeping every sentence of the history intact: #217, #222, item_drafts and upload_links, the week nobody noticed. A closing note was added recording that the generator changed in #305 and the test did not, because the drift it guards is a property of generating a mirror at all rather than of any particular library.
mirroredTables' regex also gets the same digit fix the column check already had. Both regexes parse the same generated file for the same kind of identifier, and a table name with a digit would otherwise be read out of the DB interface but reported missing by mirroredTables, sending someone to regenerate a file that was never wrong.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One commit, because a main that carries both builders is one where the next person converting a query has to guess which to reach for, and where two generated mirrors of one database can disagree. There was nothing to stage anyway: one file used the builder.
The safety property that motivated adopting a builder at all is untouched, and was never the thing being traded. A value interpolated into a sql template becomes a bind parameter in either library, so #202's invariant stays a property of the type system and #180's hotspots retire either way. What changes is the three ways the old library made it easy to be quietly wrong, each verified in #297 against the SQL actually emitted: an array interpolating as a placeholder list unless every site remembered sql.param(), a column reference inside a raw fragment silently losing its table so a correlated subquery correlated with itself, and a camelCase mirror that had to be mapped back at every select or the JSON contract changed with no test noticing.
CATEGORY_COLUMNS stops being a translation layer and becomes what it looks like — four column names four selects share. The generated types carry parent_id and sort_order because kysely-codegen emits the database's own names, so there is nothing left to map and nothing left to get wrong by forgetting to.
The drift guard survives the swap rather than being rewritten, and loses its library name in the process: it is schemaMirror.integration.test.ts now, so the next such change renames nothing. It also got stricter for free. The Drizzle version had to match each column two ways and its own comment called that deliberately loose; a generated Kysely interface spells the database's name verbatim as a bare key, so one exact match is the whole rule and snakeToCamel is gone.
isUniqueViolation keeps accepting both error shapes and now has a test behind it. Kysely uses the pg driver directly and should leave the SQLSTATE on err.code, but "should" is the word that turned two 409s into 500s when the last conversion moved it to err.cause.code with nothing failing to compile.
Migrations are untouched. #219 stands, they remain hand-written node-pg-migrate files, and Kysely has no generator to refuse.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two tasks. The first is the whole swap in one commit — dependencies, generated types, db.ts, the reconverted file, the ported drift test and the new 409 assertion — because splitting it would put a commit on the branch where the build is broken or both builders are present, and neither is a state worth being able to bisect to. The second is the conventions document, which touches no code and is much shorter than the one it replaces.
The plan carries the converted adminCategories.ts in full rather than describing it, and names the two places the conversion could silently change behaviour: the four selects must keep answering id, name, parent_id, sort_order and item_count, and the unique-violation catch must keep producing a 409. The existing category integration suite is the gate on the first, and a new test is the gate on the second.
Three expected outputs are written down so a wrong one is caught at the step rather than three steps later. Codegen must report 18 tables, not 19 — 19 means pgmigrations leaked past the exclude flag. The generated Categories interface must spell parent_id and sort_order, because camelCase there means --camel-case got turned on and the mapping layer this swap removes has come straight back. And the integration count should rise by exactly one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Carries out what #297 decided. Migrations are untouched — #219 stands, they are hand-written node-pg-migrate files, and Drizzle was never doing them.
Both builders must not coexist at any commit. A main that carries Drizzle and Kysely together, even briefly, is one where the next person converting a query has to guess which to reach for and where two generated mirrors of one database can disagree. There is nothing to stage anyway: one file uses the builder.
Three things the swap gets for free, recorded so they are not mistaken for scope creep. The worked example stops being a source file — itemFilters.drizzle.ts was never imported by anything, so it was dead code in src/ that only documentation justified, and its replacement belongs inside CONVENTIONS.md where a worked example goes. The drift test loses its library name, becoming schemaMirror.integration.test.ts, so the next such change renames nothing. And that test gets stricter rather than merely ported: the Drizzle version had to check every column two ways and its own comment calls that deliberately loose, where generated Kysely types emit the database's names verbatim and the check becomes one exact match.
CATEGORY_COLUMNS disappears rather than being translated. It exists only because Drizzle's mirror is camelCase while the API answers snake_case, and its comment says selecting the table directly would silently change the JSON contract with no test noticing. With the generated types carrying parent_id and sort_order the mapping object has nothing left to do, which is the clearest single illustration of what the swap buys.
isUniqueViolation keeps tolerating both error shapes and gains a test that proves which one actually arrives. Kysely uses the pg driver directly and is expected to leave the SQLSTATE on err.code, but "expected" is the word that turned two 409s into 500s last time.
Closes#305 is deliberately not claimed here — this is the design, and the implementation follows on the same branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The question is not whether Kysely is good, it is whether it is enough better for this codebase to reverse a decision already made in #216 and partly built in #217. That is a higher bar than being the nicer library, so this answers it against the same target #216 used: buildItemFilterSql, with six optional clauses composed at run time, a recursive CTE, an ANY(...::int[]) tag match with a count equality, and array parameters. Kysely compiles without a connection, so the document quotes the SQL it actually emitted rather than a reading of its documentation.
Three of the four hazards that src/db-drizzle/CONVENTIONS.md exists to warn about turn out to be properties of Drizzle rather than of type-safe query building, and two of them are the silent kind. An array interpolates as one bind parameter with no sql.param() ceremony, so the trap that document calls "the rule that will bite you" does not exist. A column reference inside a raw fragment is the text you wrote, so the correlated-subquery rewrite that returned a quietly wrong count in #218 cannot happen. And the generated types carry the database's own snake_case names, so the explicit column mapping that exists to stop a select silently changing the JSON contract is not needed at all. The property that motivated the whole exercise is unchanged: a hostile value lands in the parameters either way, so #202's invariant becomes a type-system property and #180's hotspots retire either way.
What decides it is how little is actually built. One file is converted — adminCategories.ts, three calls — against 238 raw query sites, and the generated mirror and its drift test are things any builder needs an equivalent of. The recommendation is to switch now, while the cost is reconverting one file and rewriting a conventions document that gets substantially shorter.
The counter-argument is recorded rather than hidden: Drizzle is more widely used, and #219's migration reasoning was measured against drizzle-kit specifically. That reasoning survives, because losing the prose and being unable to express data migrations are true of any generator, and Kysely simply has nothing to refuse.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The turn handler only bumped the cache-busting version on the success path, so a failure between the backend's two writes (displayed file rotated, then pristine original rotated) left the admin looking at an error toast next to a photo whose src string had not changed and whose bytes the browser still served from cache — even though the displayed file on disk had already turned. The design's stated mitigation for this failure, that the admin can see the photo moved and press back once, depended on the browser re-requesting the file regardless of outcome. Moving setVersion(Date.now()) into the finally block, alongside setTurning(false), makes that re-request happen on both the success and failure paths, so the failure is now visible and recoverable the way the design intends.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
libvips caches input file mappings in memory after a pipeline finishes. On Windows, this keeps an open handle on the input file, and the OS refuses to delete a file with an open handle. The animated-WebP test triggers a pipeline rejection (correctly refusing a multi-page rotation), so the mapping stays in cache and afterEach cannot remove the test directory.
Disabling the cache costs these tests nothing: each file is read exactly once during its test, so there is no reuse to cache. With caching disabled, Windows can delete the input files and afterEach succeeds.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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>
Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them.
A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not.
The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis.
Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail.
The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time.
This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301.
Closes#300
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all.
That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see.
Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them.
What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause.
It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing.
Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all.
Closes#298
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spec asserted two things the implementation disproved. RestoreSummary was described as having no `failed` because "restoring cannot fail the way removing can" — true about the sidecar, wrong about the database, and rethrowing turned a partial success into an opaque 500. And the single-button-with-two-labels rule was described as deliberately covering the mixed case, when in fact it stranded it: a partly cut-out item offered only Remove, so its existing cut-outs had no way back.
Both sections now describe what the code does and why, including why Restore is not gated on the feature being configured, and the outcome table's "feature not configured" row is corrected to say Restore is still offered and still works.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One button whose label flipped on "is every photo cut out?" could not serve a partly cut-out item, which is not a hypothetical state: it is what a partial removal leaves behind, and it is also what happens when REMBG_URL goes away after some photos were already done. In that state the single button read "Remove backgrounds", so the cut-out photos the item already had could never be restored from this screen. Remove and Restore are now separately gated and can appear together, which is correct — Remove finishes the job on what is left, Restore undoes what is already done.
Restore is deliberately not gated on the backgroundRemoval config flag. Gating it would strand cut-out photos with no way back in exactly the environment that most needs the undo. Remove stays gated, so an unconfigured environment shows no button rather than one that reports zero of four done every time.
The emptiness check moves from `!== null` to `!= null`: original_image_path is optional on the shared Item type because the public storefront response omits it, so a stray undefined has to count as "not cut out" — `undefined !== null` is true, which would misread a public-shaped item as fully cut out.
The modal now refreshes on a non-ok response too. A restore that fails partway can still have swapped some files back before it failed, so returning early left the thumbnails showing files that are no longer on the server. The warning text is now driven off whichever count the action reports, so a partial restore says how far it got the same way a partial removal already did.
The e2e spec seeds its item into a category of its own and filters the table down to it. The inventory table paginates at 10 and the suite runs fullyParallel, so an unfiltered page one was never a reliable place to find the fixture.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had.
The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved.
The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function 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 and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they 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.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
handleBackgrounds re-read /api/admin/items unfiltered and called setItems(all) after every remove-backgrounds or restore-originals call, so an admin who had filtered Inventory to one category and opened an item from that filtered view saw the table silently repopulate with the entire unfiltered catalogue the moment the request resolved. Every other mutation in this file goes through load(), which respects the active filters; this one didn't, for no reason the spec required.
The fix reuses load() instead: it now hands back the rows it fetched (previously discarded after setItems), and handleBackgrounds picks the edited item's fresh row out of that filtered result to refresh the open modal, rather than issuing a second unfiltered fetch. There is no GET /api/admin/items/:id route to fetch a single item directly, and the remove-backgrounds/restore-originals routes return only a summary, not the item, so load()'s own result is what's actually available. A background swap never touches the fields anything filters on, so the edited item stays in the filtered result whenever it was in it before.
Also added a catch to handleBackgrounds, matching the message.error shape every sibling handler (handleDelete, handleDeleteImage, handleStatusChange) already uses — previously a network drop or a malformed JSON body became an unhandled rejection with no toast, silently different from how the rest of the file reports failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
frontend/src/api.ts gains original_image_path on the shared Item type's images, since ADMIN_ITEM_SELECT's images aggregate carries it and the public catalogue's does not. It is added as optional rather than required because Item is the same type fetchItems() uses for the public storefront, and a required field the public response never sends would be a type that lies about what is actually there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Also extends the admin item select to carry original_image_path on each image, behind a new ADMIN_IMAGES_SUBQUERY kept separate from the shared IMAGES_SUBQUERY the public select uses. Task 3 needs to derive its restore-button label from that field, and the server never sent it for items before this — only the drafts endpoint carried it, added by #281 for the review queue. It stays admin-only for the same reason itemSelect.ts already names PUBLIC_ITEM_SELECT's columns explicitly: an internal original filename is nobody's business on the storefront, and sharing one subquery would put it in every public item response.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Three tasks: the two per-item functions, the endpoints plus a small admin config route, and the button.
The config route is the piece the spec did not anticipate. The inventory screen has no way to learn the feature is configured — GET /api/admin/item-drafts carries that flag for the review queue, but GET /api/admin/items answers a bare array with several consumers, and reshaping it for one boolean is the worse trade. routes/adminVersion.ts is the precedent for exactly this: a small admin-only GET, deliberately not folded into the public /api/config, with the reason written down beside it.
Two decisions the plan pins that the spec left as prose. The render condition is "configured OR every photo already cut out", not the flag alone, because gating on the flag would hide Restore originals the moment REMBG_URL is unset and strand cut-out photos with no way back — the same shape DraftQueue already uses for the same reason. And the button re-reads the item afterwards, because the editor is a modal changing files on the server while it is open, and without that the thumbnails keep showing the previous files and the button looks inert.
Self-review caught the mistake I have now made three times this session, which is naming something that does not exist. Task 3's end-to-end case called createItem(page, { withImage: true }); createItem actually takes an APIRequestContext rather than a page, and CreateItemOptions has no image field at all. Since the control only renders for an item that has photos, the item now gets seeded through the admin API with a real PNG attached, which is a thing that works rather than a thing that reads well.
It also records a spec requirement that is deliberately not implemented as written. The spec asks for an end-to-end assertion that the control is absent when the feature is unconfigured; that would mean restarting the backend mid-suite, which the run has no way to do and should not gain one. It is covered where it can be, in the integration test for GET /api/admin/config, and the plan says so rather than dropping it quietly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design behind #293, with the three decisions the issue left open now answered.
It applies to a live product image, and to every status including sold and reserved. The precedent in unpublish, which refuses both by name, does not carry: what that protects is a customer losing an item mid-checkout and a completed sale being quietly rewritten, and 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 rather than per photo, and that is the decision shaping everything else. An upload is one item — the front, the back and the chipped base are three views of one vase, not three things to cut out separately. It also means DraftPhoto is not the component to lift, despite looking like it: the queue's control is per photo and this one is per item, so sharing it would force one to pretend to be the other. The real reuse is underneath, in removeImageBackground and restoreImageOriginal, which already exist and are already idempotent.
removeBackgroundsForItem gains a summary return. It answers 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 — and not enough for an admin standing in front of the screen. The one existing caller ignores the result, so widening it is additive, the same way sendMail was in #260.
Writing it caught a contradiction in my own first draft worth recording. The failure table said a sidecar failure answers 502 while the screen section promised the admin sees "2 of 4 photos done", and both cannot be true, because a 502 throws away the count that makes the outcome actionable. Resolved by these two routes always answering 200 once the id is valid: they act on several images, so "did it work" has no single answer, and the summary is the result. Non-200 is reserved for not being able to try at all. That is a deliberate departure from #281's per-photo endpoints, which act on one image and can honestly say yes or no.
Two ambiguities also fixed before they became implementation coin-flips: what the button says in a mixed state, which is exactly what a partial failure leaves behind and which reads Remove backgrounds because that is the action finishing the job; and that restore has no failure mode of its own, being a database swap with no sidecar in it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The quality gate was red on one condition only — new_security_hotspots_reviewed at 75 against a threshold of 100 — and the outstanding hotspot was admin.ts's `${ADMIN_ITEM_SELECT} WHERE i.id = $1`.
Worth being exact about what was wrong with it, because it was not what it looked like. The value was already parameterized: itemId was bound as $1, travelled through the driver's separate parameter channel, and never entered the query text. What was interpolated was a module constant containing no caller data. S2077 fires on the template literal rather than on the value, because the rule cannot tell a constant from a request field — and neither, at a glance, can a person reading it.
So the fix is not to parameterize something already parameterized. It is to stop interpolating at query call sites at all, which turns a property somebody has to verify into one they can see. Every query whose shape is fixed is now a named constant and every such call passes an identifier: ADMIN_ITEM_BY_ID for the two admin routes, PUBLIC_ITEM_BY_ID, LINK_LIST, and the two draft-queue shapes. Seven interpolating call sites become three.
The three that remain cannot become constants and now say so rather than looking like ones nobody got to. admin.ts and items.ts build their WHERE at run time from buildItemFilterSql, whose fragments are string literals whose only interpolations are placeholder indices; that reasoning was already written down and is unchanged. draftingWorker interpolates a table name, and this is the one query here that genuinely cannot be parameterized in any form — a bound parameter is a value, and Postgres will not accept an identifier as one, so the choice is interpolation or nothing. What makes it safe is the closed 'categories' | 'tags' union, and the comment now says that instead of merely asserting there is nothing to worry about.
Also clears the project's only open Sonar issue, S1854 on adminUploadLinks, which #260 introduced and which I had deferred as a tidiness point. It was more than that: outcome was initialised at its declaration and assigned the same value again in the catch, which made two different failures look like one. A template that will not render, or a stored template that cannot be loaded, is not an SMTP problem, and reporting it as "not configured" sent the admin looking in the wrong place. The SMTP-rejection conflation that was actually agreed stays, and is now the only thing that catch conflates.
Closes#294
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spec's E2E section requires that a created link shows its address in the table, but the test that creates one filled a throwaway address and never asserted the cell. The address is now kept in a variable and asserted on the row after creation, using it to locate the row's own cell rather than any other row's.
Not run: the local stack is down and Playwright was explicitly out of bounds for this pass, per the task instructions. This is written and verified by inspection and lint only.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The revoke route's RETURNING clause omitted contact_email while its result was typed as UploadLinkRow, which declares the field as present. No user-visible effect since the response was never checked for it, but the type asserted something the query did not actually return. Added the column so the two agree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The table headed its address column "Sent to", but contact_email records only the intent an admin typed in, never whether delivery happened — that outcome is shown once, at creation, and is not persisted. In QA, where every send is blocked by design, every row read "Sent to ..." for links that were never emailed, and the honest warning that appears at creation is guarded on `issued`, so it vanishes on refresh, leaving the false heading as the only surviving statement. Renamed to "Email", which is true of what the column actually stores.
Separately, create() called setMailed(null) before every request, including one that would go on to 400. If an admin creates link A while mail is down (warning shown, token A still on screen) and then mistypes an address on a second attempt, the 400 path returned early — but the reset had already run, so the warning for link A disappeared while token A was still displayed on the same screen. setMailed is now only called after a successful create, alongside setIssued, so a request that never produces a new link can no longer clear a warning that belongs to the one still shown.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
submitUrl, label and submissionsAllowed were added to the uploadLink template but SAMPLE_VALUES had no entries for any of them, so an admin opening Email templates, Upload link for a contributor, Preview saw literal {{submitUrl}} in the body — the preview being the only way to check an edit before saving.
The unit test meant to catch exactly this, in emailTemplates.test.ts, iterated a hardcoded KEYS array that predated intakeDraft and uploadLink, so it never checked either template's samples. KEYS is now Object.keys(TEMPLATES) as TemplateKey[], so the guard covers every template automatically and cannot go stale the same way again. intakeDraft already had samples for all its placeholders and passes once included, as expected.
One other test in the same file, "every template can address the customer", asserts that available contains greeting/firstName/lastName — a real invariant of the six customer-facing templates, but not of intakeDraft or uploadLink, which notify the shop and a contributor rather than a customer with a name on file. Switching that test to the new all-templates KEYS would have made it fail for both, so it now uses its own explicit CUSTOMER_FACING_KEYS list instead. That is a deliberate, commented exception: a hardcoded list is correct there because the claim itself does not extend to every template, whereas the SAMPLE_VALUES guard's claim does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adminUploadLinks.ts awaits sendMail on the admin's request path, the first awaited send on a user-facing request in this codebase, but the transporter in mailer.ts set no connectionTimeout, greetingTimeout or socketTimeout. nodemailer's defaults then apply: two minutes to connect, ten minutes on the socket. If the SMTP host is unreachable in a way that drops packets rather than refusing, the link row and its token are already committed by the time sendMail is called, the response hangs for up to two minutes, the browser or reverse proxy gives up first, and the token — shown exactly once and unrecoverable — is never rendered. That is the link being lost in exactly the way this feature's central invariant forbids.
All three timeouts are now set to 5000ms, with a comment explaining why a send on a request path has to fail fast rather than inherit nodemailer's fire-and-forget defaults. Five seconds is generous for a reachable host and short enough that a dead one fails while the admin is still willing to wait, leaving them the "not emailed" warning and a link they can still copy instead of a stuck spinner and a token nobody ever saw.
Also corrects the comment directly above the skipped-blocked return, which said "returning as though it sent" — true before #260, and precisely backwards now that the outcome is reported through MailOutcome rather than swallowed. Reworded to describe what the code actually does today, keeping the explanation of why it skips rather than throws.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
intake.integration.test.ts, intakeCeiling.integration.test.ts and uploadLinks.integration.test.ts all create upload links, and since #260 that now sends real mail. env.setup.ts never clears SMTP_USER, SMTP_PASSWORD or MAIL_ALLOWLIST, so with those inherited from a developer's shell these three files opened live TLS connections to smtp.gmail.com:465 and, with no allowlist set, actually delivered to sarah@example.com. This is the exact hazard the "Do not add one back" comment in tests/unit/mailOutcome.test.ts already warns about, reintroduced at the integration layer.
All three now mock ../../src/mailer the same way accountDetails.integration.test.ts, favorites.integration.test.ts and resendVerification.integration.test.ts already do. uploadLinks.integration.test.ts is the one place that needs to see specific MailOutcome values come back through the route, so its two outcome tests were restructured to drive the mock's return value directly (sentMail.mockResolvedValueOnce(...)) instead of threading SMTP_USER/MAIL_ALLOWLIST through the real sendMail. That is a cleaner test anyway: it isolates the route's job (reporting whatever outcome sendMail returns) from sendMail's own skip logic, which is already covered hermetically by mailOutcome.test.ts and mailAllowlist.test.ts.
Also addresses the related minor finding that nothing asserted the mail actually carried the working link: required: ['submitUrl'] on the template only guards that the placeholder is present in the body, not that the route supplied a correct value for it. A new test in uploadLinks.integration.test.ts inspects the mock's captured call and asserts the html contains the created token's /submit/ URL, and covers the three submissionsAllowed phrasings (a numeric cap, a cap of exactly one, and uncapped).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The address is now a required field beside the label, the links table shows where each link was sent, and the admin is told plainly when the mail did not go — with the link still on screen to copy, which is the case that matters in QA and in local development where there is no mail at all.
Two specs in unrelated features created links with only a label and the route now refuses that, so they are updated here rather than left to go red on somebody else's branch. That is the cost of making the address required, and it is a small one: the compiler and the suite find every call site.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Creating a link now requires a valid email address and mails the link to it, which is the whole point: getting a link to a contributor was previously a copy-and-paste into whatever the admin happened to use.
The send is awaited and its outcome reported, unlike every other sender in this codebase, which fires and forgets because nobody is waiting on the answer. Here somebody is. The admin is looking at the screen, and whether they now have to send the link by hand is exactly the thing they need to know — and QA blocks delivery to any address outside MAIL_ALLOWLIST by design, so a link that was never emailed would otherwise look precisely like one that was.
A send that could not happen does not roll the link back. The token is displayed exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that had succeeded. They end up with a usable link and an honest statement about delivery instead.
One inaccuracy left deliberately: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome of its own. The distinction is real but nothing consumes it, and the admin's next action is identical either way.
Also updates the other integration tests that created a link with only a label, since an address is now required, and adds the uploadLink template key that GET /api/admin/email-templates was missing from its list — an omission left by the template's addition in the prior commit on this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds upload_links.contact_email and the uploadLink mail template.
The column is nullable on purpose. Links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered rather than backfilled with something untrue; the requirement belongs in the create route, which is where new links are actually made.
The template requires submitUrl, the same guard verification has on verifyUrl. An email inviting somebody to send in photos, with no way for them to do it, sends perfectly happily and looks fine in the log — it is the one failure here worth making impossible, and a test asserts the default body satisfies the guard it declares.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the Task 1 commit approved the implementation but flagged the third test in mailOutcome.test.ts as a defect carried over from the brief: it set SMTP_USER, SMTP_PASSWORD, and an allowlist the recipient satisfied, so sendMail fell through both early-return guards and reached the real transporter, opening a live TLS connection to smtp.gmail.com:465. The .catch(() => 'threw') wrapper hid a fast auth rejection, a slow timeout, or an accidental real send equally, and on a restricted CI runner it would hang to the Jest timeout rather than fail fast.
The fix deletes that test rather than replacing it. What it was trying to prove — that an allowlisted recipient is not blocked — is already covered hermetically by backend/tests/unit/mailAllowlist.test.ts, which exercises isAllowedRecipient directly across exact matches, plus-suffixes, domains, and refusals. The other two tests in mailOutcome.test.ts are untouched; they cover the two paths that return early, which is the entire point of the change, and neither one reaches the transporter.
The file's top doc comment is updated to match: it now says only the two skip paths are covered here, names mailAllowlist.test.ts as where the allowlist's own behaviour is tested, and spells out why a third test that reaches the transporter does not belong in this file, so nobody adds one back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It returned Promise<void> and returned early in two cases that were indistinguishable from success at the call site: no SMTP credentials, and a recipient outside MAIL_ALLOWLIST. A caller could therefore report that it had emailed somebody a message nobody would ever receive, and in QA — which restricts delivery deliberately, as its entire safety property — that is the normal case rather than an edge one.
It now returns a MailOutcome saying which of the three happened. No existing caller changes: there are seven and every one ignores the result, so this is additive. Re-deriving the answer at a second site would have duplicated isAllowedRecipient and the SMTP check, which is exactly the drift the guard-in-one-place comment above them exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four tasks over the approved design: sendMail gains an outcome, the column and template land together, the route requires an address and reports what the send did, and the admin screen asks for it.
sendMail goes first deliberately. Everything else depends on being able to tell a skipped send from a real one, and it is the only change touching a file seven other things already use — so if it is going to break anything, it should break before three tasks are stacked on top of it.
The plan is explicit that no existing caller changes. Ignoring a returned value is legal, which is what makes widening the return type additive rather than breaking, and re-deriving "would this address be blocked?" in the route would have duplicated isAllowedRecipient and the SMTP check at a second site.
Two specs in unrelated features create links with only a label, and the route will refuse that. They are fixed in the same task as the form rather than left for the suite to find, because the alternative is two unrelated features going red on somebody else's branch. That cost is named in the plan rather than discovered.
One inaccuracy is left in deliberately and said out loud: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome. The distinction is real, nothing consumes it, and the admin's next action is identical either way — copy the link and send it by hand.
Self-review caught the failure mode from #281, where tasks referred to helpers that did not exist. Task 4 originally said the created-link state "may not be called created". Reading the component showed it is `issued` and holds a bare URL string with nowhere to put a delivery outcome, so the plan now adds a separate `mailed` state beside it rather than widening the one-time token display. Every name in that task is now one that exists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The issue asked for the security question to be settled before any code, and it is: the mail carries the working link.
The issue framed that as a loosening comparable to a password reset, and on inspection that framing overstates it. A reset token takes over an account. An upload token grants exactly one capability — submit photos into a queue where a person must approve them before anything reaches the storefront. It reads nothing, it is revocable, max_submissions caps it, and #227 caps the whole intake surface regardless of any single link. The worst outcome of a leaked upload link is junk in the review queue, which is bounded and reversible. That is a reasonable thing to put in an inbox, and this project already makes the much larger bet with reset links.
The address is required for new links while the column stays nullable, which is not a contradiction: links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered, and the requirement belongs in the route where new links are actually made. It lives on the link rather than on a contributor entity, because a link already carries a label naming who it is for and nothing yet suggests the same people submit repeatedly.
A failed send does not roll the link back. The token is shown exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that succeeded. The link is created, the send is attempted, and the response says which happened — which matters concretely because QA's MAIL_ALLOWLIST silently skips any address outside it and returns as though it sent. Without an explicit outcome, testing this in QA against a contributor's real address looks exactly like success, which is the afternoon the issue warned would otherwise be wasted.
Writing it turned up one thing the design had assumed and the code does not support. sendMail returns Promise<void> and returns early both when SMTP is unconfigured and when the recipient is not allowlisted, so a caller cannot tell either from success. It gains a MailOutcome return value instead. No existing caller changes — there are seven and every one ignores the result — and the alternative would have duplicated isAllowedRecipient and the SMTP check at a second site, which is the drift the guard-in-one-place comment in mailer.ts exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The end-to-end helper opens its own connection to read a password-reset token, and nothing guaranteed it pointed at the database the application was actually using. With -E2eDb the app runs on redefined_e2e at 55501 while the helper kept its redefined_local default at 55500, so the app wrote to one database and the suite read another — and the specs failed for a reason that had nothing to do with them.
start-local.ps1 now records the coordinates it chose in .local/database.json, and run-tests.ps1 reads them into TEST_PGHOST, TEST_PGPORT, TEST_PGUSER, TEST_PGPASSWORD and TEST_PGDATABASE before Playwright starts. The answer now comes from one place, written by the thing that made the decision at the moment it made it.
It is written after Start-Database rather than before, so the file never names a database that failed to come up, and removed by -Stop, so a stopped stack does not leave a record pointing at a container that is gone.
Setting all five closes the second fault in the same change. Invoke-IntegrationSuite sets TEST_PGPORT and PowerShell keeps it for the rest of the session, so a -Suite all run leaked the integration port into the e2e run that followed — with none of the matching credentials, leaving the helper offering redefined_local's password to the integration database. Overwriting every one of them is what makes that leak harmless.
A missing record throws rather than falling back. A default is what produced both faults in the first place: always plausible, silently wrong, and it fails in ways that look like application bugs rather than configuration.
The guard test is the point of the change as much as the fix is. This is the third instance today of two files having to agree with nothing comparing them — #107 and #118 were envValidation against a compose file, #287 was the workflow against start-local.ps1, and this is three files rather than two. The test pins the whole chain: that the writer records the five settings, that the runner reads each of them from the record rather than a default, and that the helper reads no connection variable the runner does not set. Removing a single line from the runner fails four of its assertions, which was checked rather than assumed.
What it cannot do is run PowerShell, so these are text assertions against the scripts. That is weaker than executing them and still catches the drift that actually happened.
Closes#273
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
intakeNotifyEmail and intakeCeilingResetAt both document empty as their default and as a working configuration — no notification address, and no ceiling reset recorded. The validator refused every empty text value, so either could be set and then never removed through the admin at all; the only way back was a DELETE against admin_settings. An admin who turned intake notifications on could not turn them off.
Whether empty is a mistake is a fact about the setting rather than about its type, so it is now declared on the setting, in the DEFINITIONS row that already carries its type and fallback. A new setting states it once, in the place someone adding one is already editing, and nothing else has to know. That is what makes this different from special-casing two names in the validator, which would have left the next such setting to rediscover the same bug.
The blanket refusal stays the default, because for a setting with a non-empty fallback an empty value really is a mistake: an empty greeting format renders every greeting as nothing at all, which reads as a broken email rather than as something a person cleared. Both those cases keep their tests.
Whitespace is normalised to empty rather than stored. Somebody clearing a field they cannot see the end of leaves spaces behind, and they meant cleared.
The tests check that the clearing survives the request rather than only being echoed back — the last one sets a value, clears it, and then reads it again through GET, which is the assertion that would have caught this had it existed.
Closes#280
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The issue asked for two things and only one had been done. A well-formed but absent id already answered 404 — the PUT route carries a comment saying so. A malformed one still reached Postgres as text, raised 22P02 on an integer column, and surfaced through the route's catch as a 500, telling the admin the server had broken when the truth is that no such item can exist. That half is now closed everywhere rather than on the three routes that happened to have it.
Guarded: DELETE an item, DELETE an image, unpublish, and every route in adminItemDrafts — publish, regenerate, discard, restore, and the two background-removal endpoints added by #281. The last of those were flagged in that feature's own final review as sharing this pre-existing shape, so they are fixed with the rest rather than left to be found again.
Routes carrying two ids guard both. A route can guard the first and forget the second, and the forgotten one fails exactly as loudly, so there is a case each way for both image endpoints and for DELETE image.
DELETE deliberately still answers 204 for a well-formed id that is absent. The method is idempotent and the caller's intent, that the item should not exist, is satisfied either way; what must not happen is a 500. There is a test pinning that so the distinction is a decision rather than an omission.
Also replaced the raw req.params.id and Number(req.params.id) uses that sat inside routes which had already computed a validated id. They were safe, because the guard above them made them safe, but a validated id and a raw one side by side in the same handler is how this bug comes back.
The test block named "a non-numeric id on every admin item route" covered two routes. It now covers every route that takes an id, which is what makes its name true.
Closes#207
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main went red on 86095cb with "Playwright: 157 passed, 3 failed" while every other step in the job passed. The three are the background-removal cases from #281, and they fail for the most boring reason available: REMBG_URL appears nowhere in .gitea/workflows, so isRembgConfigured() is false, both endpoints report backgroundRemoval as false, and neither the submitter's checkbox nor the review queue's per-photo button is rendered for the tests to find. The application is right and the tests are right; the environment they run in was missing one variable.
Deliberately a URL that does not resolve. Nothing in the suite reaches the sidecar — the worker only cuts a background out after a draft is written, and drafting needs an ANTHROPIC_API_KEY this job does not have. Standing up a real rembg in CI would cost a 4.24 GB image and about forty seconds of startup to prove that a control is on screen.
How it got in is worth recording, because the missing line is the symptom. #281 added the variable to scripts/start-local.ps1 and stopped there, and its three end-to-end tests were merged unrun — verified locally against a stack that did have the variable, then handed over as outstanding rather than run before the merge. A test that has never been run is not evidence of anything, and two of the three would have failed on the first honest execution.
The deeper gap is that a variable which changes rendered behaviour now lives in two places nothing compares. composeEnvironment.test.ts exists for exactly this failure and caught it twice before, in #107 and #118, but it guards the deploying compose files rather than the CI workflow. Whether it should reach into .gitea/workflows is a decision rather than another hand-added line, and #287 leaves it open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two assertions named a fixture and expected it visible in the unfiltered grid. No paginated catalogue can promise that — the item is on some page, not necessarily the first — so both would have started failing the moment paging landed. They were only ever proxies for "the result set got bigger", and the visible total lets them say that directly, which is what the issue predicted when it asked for a count.
The new cases assert the control and the URL rather than which item is on which page, because the development database never truncates and which item lands where is not something a test may rely on. That is the same trap the two rewritten assertions had fallen into, and repeating it in new tests would have been worse than leaving them alone.
Writing them found a real defect rather than just covering the feature. The control was rendering while the catalogue was still loading, showing "0 items" for a moment before the real count arrived — the empty-state early return only fires once loading has finished, so a mid-load render fell through to the grid branch with a total of zero. It is now suppressed until there is something to count, which is both true and what makes the count usable as a signal in a test. StorefrontPage.totalItems waits for the control for the same reason: reading during the load returned zero and quietly made "the result set shrank" compare against nothing.
The conditional skips carry a file-level eslint exception with its reasoning rather than being left to add four warnings. They are honest about a real limit: against a catalogue of ten items or fewer these cases prove nothing, and if the e2e database is ever seeded that thinly they need fixtures of their own instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The page number joins the filters in the URL, which is already the single source of truth for what the storefront is showing. That is the whole reason numbered pages were chosen over infinite scroll: a page is a place you can send someone, and a scroll position is not. The page size deliberately does not go there — it is a preference belonging to one person, and putting it in the URL would mean sharing a link to an item also imposed your page size on whoever opened it.
Changing a filter returns to page one, and it does so for free: filtersToSearchParams builds a fresh URLSearchParams, so applying filters drops the page parameter while goToPage copies the existing params and keeps the filters. That is behaviour worth having rather than an accident to tidy up — landing on page seven of a two-page result is a state a customer cannot get out of without understanding the URL.
The control carries the total, because showing the count was a requirement in its own right and the only count that existed before this was on the filter drawer's "Show N items" button, which is hidden whenever the drawer is closed. It is therefore shown even when everything fits on one page: hiding the control on a single page would hide the count with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every decision paging needs, as pure functions: which page a URL is asking for, which page is actually showable given how much there is, which slice of the items that is, and what page size to use. Pure because that is the only thing this project can unit-test — vitest runs in a node environment with no jsdom and no testing-library, so a hook or a component is only reachable through Playwright. Keeping the rules here means the rules have tests and the React wrapper stays thin enough not to need any, which is the same split filters.ts already uses for the URL.
An unrecognised page size is refused rather than clamped. A stored or hand-edited 5000 would render the entire catalogue in one page, which is the exact failure this issue exists to prevent, and clamping would quietly honour a value nobody offered. Storage access is guarded on both sides because localStorage is absent when there is no window and throws outright in some privacy modes, and neither is a reason for a customer to lose the catalogue — the worst acceptable outcome of a broken preference is the default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tasks over the decisions recorded on the issue: the pure paging rules with their tests, the wiring into the catalogue, and the end-to-end cases including the two assertions this finally lets say what they mean.
Two decisions the issue left open are settled here. The page number goes in the URL beside the filters, because a numbered page being linkable is the whole reason numbered pages were chosen over infinite scroll, and the URL is already the single source of truth for what the storefront is showing. The page size deliberately does not, because it is a preference belonging to one person — putting it in the URL would mean sharing a link to an item also imposed your page size on whoever opened it. It lives in localStorage instead, which also keeps it inside the issue's own scope boundary of not touching the API.
The third open question, whether page size changes with viewport, is answered no and written down as such rather than left silent. A size that moved on rotation would fight the preference the customer had just set.
Everything testable is a pure function, because that is all this project can unit-test: vitest runs in a node environment with no jsdom and no testing-library, so a hook or a component is only reachable through Playwright. That is the same split filters.ts already uses, and it is why the React wrapper is thin enough to need no test of its own.
Writing it turned up one piece of luck worth not breaking: filtersToSearchParams builds a fresh URLSearchParams, so applying a filter already drops the page parameter and returns to page one, while paging copies the existing params and keeps the filters. The plan says so explicitly so nobody 'fixes' it later.
It also confirmed the issue's own warning. filters.spec.ts:165 asserts a named fixture is visible in the unfiltered grid, and favorites-filter.spec.ts:106 does the same — with ten items to a page over thousands, both would start failing the moment paging landed. They were always proxies for 'the result set got bigger', and a visible total lets them assert that directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backgroundRemoval.integration.test.ts exported seedSubmission for no reason — nothing imports it, since draftingBackgroundRemoval.integration.test.ts and adminItemDrafts.integration.test.ts each wrote their own seeding helpers. Dropped the export, kept the function for local use.
All three of these suites create a temporary uploads directory with mkdtemp and point UPLOADS_DIR at it, but none of them removed the directory afterward or restored the previous UPLOADS_DIR value — checked and the leak existed in all three, not just the one the review flagged. Each afterEach now removes its temp directory with fs.rm and restores (or deletes) UPLOADS_DIR to what it held before the test touched it, so this suite no longer leaves rubbish in the OS temp directory or a stale environment variable for whatever runs after it in the same process.
This is test scaffolding cleanup, not a feature change — no runtime path in the application deletes anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ANTHROPIC_API_KEY's explanatory paragraph already had a dangling continuation trailing after later entries. When REMBG_URL was added, its entry was inserted ahead of that continuation, so the file read as though "put a spend limit on the key in the Anthropic console" belonged to the background-removal sidecar rather than to Anthropic. This file is read during the cutover runbook, so a misattributed sentence there is not just cosmetic.
Reordered the comment lines so ANTHROPIC_API_KEY's full paragraph is contiguous and REMBG_URL's own two-line entry stands on its own at the end. No environment: line was touched — only the comment block above the services: section.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The module comment on backgroundRemoval.ts said "The original file stays on disk and so does every cut-out ever made." The original half is true and load-bearing; the cut-out half is not. cutoutPathFor is deterministic, so a photo that is restored and then cut out again overwrites the previous cut-out at the same path. Harmless — no original is ever touched — but the comment overstated what the module guarantees. Corrected it to say what is actually true.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The remove-background route's catch block turned every throw from removeImageBackground into a 502 "the background-removal service did not answer". But removeImageBackground also throws for an unrecognised file extension (a legacy .jpeg), for a file missing from the uploads volume, and when REMBG_URL is not set at all — none of which involve contacting the sidecar. The admin was told to retry a service that was never reached, while the real reason existed only in the server log.
Added SidecarRequestError in rembgClient.ts, following the NoOriginalToRestoreError pattern already in backgroundRemoval.ts. It is thrown only for failures that happen after actually attempting to reach the sidecar: the fetch call itself throwing (now wrapped in a try/catch, covering unreachable and timed-out), a non-2xx response, or a response that is not a PNG. It is deliberately not thrown for "REMBG_URL is not set", since that path never attempts contact at all.
The remove-background handler now checks err instanceof SidecarRequestError before answering 502; everything else answers 500 with a message that says what actually went wrong.
Added a unit test pairing (rembgClient.test.ts) asserting the sidecar-contacted failures are SidecarRequestError and the unconfigured case is not, and an integration test (adminItemDrafts.integration.test.ts) proving a missing upload file answers something other than 502 with a message that does not claim the service did not answer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
item_drafts.remove_background is written once at intake and never updated afterward. Restoring a photo clears original_image_path, which is exactly what makes the row look "never cut out" to removeImageBackground — so a submitter's ticked checkbox, followed by the worker cutting the photo out, followed by an admin restoring a poor result, followed by a click on Regenerate, would silently re-cut the same photo the admin had just put back. Nothing was lost, but the control the design calls "what makes a poor result survivable" was quietly defeated by the button sitting next to it.
restoreImageOriginal now swaps the image's paths back and clears item_drafts.remove_background for that item in one transaction, so a restore that succeeds while the flag update fails cannot reintroduce the bug. An admin restoring any photo on an item is treated as overriding the submitter's original request for the whole item — the flag is per-item while the swap is per-photo, so there is no narrower place to record the decision, and turning off the whole item's auto-removal is the conservative direction: the alternative is re-cutting something a person deliberately undid.
Added an integration test in draftingBackgroundRemoval.integration.test.ts that drafts a submission with the intent set, cuts it out, restores it, mirrors what the admin's Regenerate button does (state back to queued, attempts cleared), runs the worker again, and asserts the photo is still not cut out.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DraftQueue background-removal control gated both "Remove background" and "Restore original" on the same `backgroundRemoval` flag, which only reflects whether a sidecar is currently configured. Restoring is a pure database swap and never calls the sidecar, so once photos had already been cut out and REMBG_URL was later removed from the stack, the admin was left looking at a cut-out photo with no control at all and no way back to the original short of a hand-written SQL UPDATE — directly breaking the "the original is always restorable" invariant the feature is built on.
DraftCard now computes `enabled` per photo as `backgroundRemoval || image.original_image_path !== null`, so Restore original stays available whenever a photo has an original regardless of whether the sidecar is configured, while Remove background still requires a configured sidecar. Also corrected the docstring on `DraftQueueResponse.backgroundRemoval` in draftsApi.ts, which claimed the flag hides "the control" generically — it only ever governed the remove-background control.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the per-photo control that closes out the background-removal feature: each photo in the review queue now gets a "Remove background" or "Restore original" button, whichever matches its current state, and the button only appears when the server reports a sidecar is configured. The label is read from original_image_path alone rather than a second flag, so there is nothing that could disagree with what the button actually does.
draftsApi.ts's fetchDrafts now returns { drafts, backgroundRemoval } instead of a bare Draft[], matching the breaking change Task 6 made to GET /api/admin/item-drafts. DraftImage gains original_image_path, and a new setImageBackground(itemId, imageId, action) posts to the remove-background/restore-original endpoints, preferring the server's error message the same way publishDraft does.
Also updates docs/ops/image-background-removal-stack.md: the status line no longer says "evaluated, not adopted", since the feature is adopted here, and the closing "If this is adopted" section is replaced with "How the application uses it", describing the two real entry points (the drafting worker's default-on checkbox, and this per-photo control) and confirming that nothing in the feature deletes a file or a row.
Adds an e2e case asserting the button's label appears on a freshly submitted item's card, scoped to that card by the sender's note per #241. It is unrun in this environment — the local stack was not started, per standing instruction not to run start-local.ps1 or Playwright without the user's supervision.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a checkbox to the public submission page that lets a sender opt out of background removal, ticked by default because most items look better cut out and the reverse default would mean almost nobody got it. It only renders when the server reports the sidecar is configured, matching the intake link's new backgroundRemoval flag from Task 5 — an unconfigured environment gets no checkbox rather than one that would do nothing.
submitItem now takes removeBackground as a required fourth parameter, sent as the multipart string 'true' or 'false' to match the backend's exact-string opt-out contract. Making the parameter required rather than optional was deliberate, so the compiler would catch any call site left unupdated; the frontend build (which also type-checks tests/ via tsconfig.test.json) confirmed the only call site, in Submit.tsx, was updated.
scripts/start-local.ps1 now sets REMBG_URL for the local backend so the checkbox is visible during local and e2e runs; the value need not resolve, since no e2e submission reaches the sidecar without a configured drafting step.
Adds two e2e cases to intake-submit.spec.ts: the checkbox appears ticked by default, and a sender can uncheck it and still submit successfully. Both are written per the task-7 brief but not run in this session, since running Playwright requires the full local stack (database, backend, frontend dev server) which was not started.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 1 caught every throw from restoreImageOriginal() and reported it as a 404, on the theory that losing the concurrent-restore race is the only way that call fails. But the throw carried nothing to distinguish that race from a genuinely different failure during the same UPDATE — a dropped database connection, a transient outage — so a real failure was now silently reinterpreted as "someone already restored this" instead of surfacing as the loud 500 it was before.
backend/src/intake/backgroundRemoval.ts now exports NoOriginalToRestoreError, a named subclass of Error thrown in place of the bare Error restoreImageOriginal previously threw. The message text is unchanged, so backgroundRemoval.integration.test.ts's rejects.toThrow(/no original/) assertion keeps passing without modification.
backend/src/routes/adminItemDrafts.ts catches that class specifically in the restore-original handler and rethrows anything else, so a real failure still reaches the app-level error handler and comes back as a 500 instead of being mislabeled as "already done".
backend/tests/integration/adminItemDrafts.integration.test.ts adds a test that spies on restoreImageOriginal via jest.spyOn on the module namespace (the project compiles to CommonJS, so the route's call site reads the export off that object at call time, which makes the spy effective without jest.mock) to reject once with a plain Error, and asserts the response is 500 rather than 404 — proving the narrowing changes real behavior, not just internal structure. The spy is restored in a finally block.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
restore-original's precheck (existing.original_image_path === null) and restoreImageOriginal's own guard (WHERE ... AND original_image_path IS NOT NULL) could disagree under a race: two concurrent restores, or a rapid double-click, could both pass the precheck before either commits, and the loser's UPDATE would then match zero rows and throw. The handler had no try/catch around that call, so the throw propagated through asyncRoute to the app-level error handler and the caller got a bare 500, breaking the route's documented 200 | 404 contract even though the row itself was left correct.
Wraps the restoreImageOriginal call in a try/catch, matching the shape remove-background already uses in this file, but answering 404 rather than 502: losing this race means another admin already finished the restore, not that a downstream service failed. Adds a comment on the catch explaining why it exists, and a test that fires two restores concurrently and asserts neither comes back 500.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
The public intake POST now reads a `removeBackground` multipart field and stores it on the new `item_drafts.remove_background` column. The checkbox on the submission page is ticked by default, so a client that sends nothing gets `true` — only the exact string `'false'` opts out, so a stray or unexpected value is treated as consent rather than a silent refusal.
The GET now also reports `backgroundRemoval: isRembgConfigured()` alongside the label, so the submission page knows up front whether the feature exists in this environment at all. Neither handler calls the sidecar or the AI — this task only records intent for the drafting worker to act on later, and the existing ordering of `requireUsableLink` and `requireCapacity` ahead of `uploadImages` is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wires removeBackgroundsForItem into draftQueued, gated on the submitter's remove_background intent recorded on item_drafts. The step runs after the draft is committed and catches for itself, so an unreachable or erroring sidecar never turns a draft that was written correctly into a failed one — the photo simply keeps its original, and the admin's per-photo control in the review queue is still there to do it by hand. It is awaited, unlike the notification below it, so a sweep that has returned has finished its work; nothing on the request path waits on it.
Adds backend/tests/integration/draftingBackgroundRemoval.integration.test.ts as a new file rather than extending drafting.integration.test.ts, because that suite has never produced a successful draft and therefore has no draftListing mock — adding one there would be file-wide and would change what its existing tests exercise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds backend/src/intake/backgroundRemoval.ts, the shared module the drafting worker and the admin endpoints both call so a cut-out obtained either way is undoable the same way.
cutoutPathFor is pure and writes a new file beside the original rather than overwriting it, which is what keeps the original restorable and makes the JPEG-to-PNG change free. removeImageBackground only points the row at the new file after it is already on disk, and is idempotent via the original_image_path IS NOT NULL check — load-bearing twice, since it also stops a second pass from recording the cut-out as the original and losing the real one for good. restoreImageOriginal swaps the paths back and deliberately leaves the cut-out file on disk.
Extends the Task 1 integration test file with a stub sidecar bound to an ephemeral port and covers the no-op-on-repeat case plus three failure modes (500, non-image body, unreachable), asserting the row is left untouched in every failure case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the client that will let the intake path remove backgrounds from submitted photos via the rembg sidecar over HTTP. isRembgConfigured() reports whether REMBG_URL is set (unconfigured is a normal, working state, not a failure), and removeBackground() posts a file to /api/remove and resolves with the PNG bytes it gets back, rejecting on every failure — unconfigured, unreachable, a non-2xx response, or a body that fails the same magic-byte PNG check the upload path already uses.
The one hard rule: every request names model=u2net explicitly and this is never configurable. The sidecar's default model, reached simply by omitting the parameter, is bria-rmbg, which is licensed non-commercial — a licensing problem that a shop cannot silently ship, and one that would produce a perfectly good image with nothing in it to reveal the mistake. The test that posts against a real stub HTTP server and asserts model=u2net appears on the wire is the only thing guarding against that regressing.
Wires REMBG_URL into both docker-compose.qa.yml and docker-compose.prod.yml as an optional variable, right after ANTHROPIC_WORKSPACE_ID, following the existing style in each file's environment block and header comment. It is deliberately left out of envValidation.ts's ALWAYS_REQUIRED — requiring it would make an environment with no sidecar refuse to boot, which is exactly the failure mode this feature is designed to avoid.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the two columns the background-removal feature (#281) is built on: item_drafts.remove_background (boolean, not null, default true) records the submitter's per-submission intent, and item_images.original_image_path (nullable text, no default) records where a cut-out photo came from so it can be restored. The default on remove_background is load-bearing — any row written by a path that does not mention the column behaves like the new default, so no backfill is needed. original_image_path stays null until a photo has actually been cut out, which doubles as the answer to "can this be restored?" rather than needing a separate flag. Also updates the Drizzle mirror in src/db-drizzle/schema.ts by hand (the local dev database was not running to re-pull from) and adds the integration test backgroundRemoval.integration.test.ts, including the exported seedSubmission helper that Task 3 will reuse.
Closes#281
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bermudalamb
