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>
Eight tasks over the approved design, each ending in something independently testable: the two columns, the sidecar client, the shared swap-and-restore, the worker step, the intake route, the admin endpoints, the submitter's checkbox, and the review queue's per-photo control.
Three things the plan pins down that the spec left to implementation.
The `model=u2net` assertion lives in a unit test against a real stub HTTP server rather than a mocked fetch, because what has to be checked is the shape of the request that reaches the wire. Nothing in the returned image would reveal that the non-commercial default had been used, so that assertion is the only thing standing between this and a licensing problem that produces perfectly good pictures.
Removal in the worker follows drafting rather than running on its own pass, which couples the two: an environment with no ANTHROPIC_API_KEY drafts nothing and so cuts out nothing. That is the deliberate trade — a separate pass would re-attempt an unreachable sidecar on every five-minute sweep for a row that is going to sit at `queued` indefinitely — and the plan says so in the worker's own header comment rather than leaving it to be rediscovered.
`removeImageBackground` is idempotent through the `original_image_path IS NOT NULL` check rather than a separate flag, and that guard is load-bearing twice: it makes a repeat call a no-op, and it stops a second pass recording the cut-out as the original and losing the real one for good.
Writing it turned up two things worth knowing about the existing tests. `drafting.integration.test.ts` has never produced a successful draft — every case in it either has no key or no readable photo — so the worker's new cases need their own file with `draftListing` mocked, rather than a mock added file-wide to a suite that deliberately never reaches the model. And `adminItemDrafts.integration.test.ts` calls `request(app)` directly with no helper, so the plan spells out the seed it needs instead of pointing at one that does not exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design behind #281, written before any code so the decisions can be argued with while they are still cheap to change. The implementation follows on this branch.
Six decisions, each recorded with what it rests on rather than just what it is. The one that matters most is that `model=u2net` goes on every request: the sidecar's default is `bria-rmbg`, which is licensed non-commercial, and it is reached by simply not specifying a model — a silent licensing problem that produces a perfectly good image. A test asserts the parameter is present, because nothing in the output would reveal its absence.
The other consequential one is that the submitter's tick records an intent rather than doing the work during their upload. Inline removal would make them wait, would put a CPU-heavy model run in a path anyone holding a link can trigger — the surface #227 exists to bound — and would force a choice, when the sidecar is unreachable, between failing their submission and silently ignoring what they asked for. Recording the intent means the submission always succeeds and keeps its original photo, and the cut-out arrives with the AI draft seconds later.
Everything else follows the rule the pipeline already runs on: a submission is the only irreplaceable thing here. The original is never destroyed, every failure path leaves the photo exactly as it was, and an unset REMBG_URL means the feature simply does not exist rather than that the environment is broken.
Documents what is not established too — quality on a real photograph is unknown, because the engine evaluation used a generated rectangle on a flat ground. The per-photo control and Restore original are what make a poor result survivable rather than something to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The list finally arrived from the reporting added earlier, and confirmed what #181 could only suspect: these are not the five eslint-plugin-sonarjs warnings that issue lists. Those were fixed under #261 and the count staying at five was a coincidence. It is six now, 25 minutes of debt, and one of them was mine.
admin.ts imported '../utils' twice — I added readId in #207 without noticing the file already imported from there. One import now.
filters.ts had a redundant `as ItemStatus[]`. TypeScript narrows an array through `.every()` with a type predicate from 5.5, and this project is on 5.9, so the assertion stopped telling the compiler anything. Removed, and the build confirms the narrowing holds without it.
adminSettings.ts was the only CRITICAL: cognitive complexity 18 against a limit of 15, almost all of it three near-identical loops differing only in how they validated. Each validation is now a small pure reader returning a refusal rather than sending one, and the handler is one loop over a table. Adding a setting type means adding a row.
That refactor is deliberately behaviour-preserving. Two things were left alone on purpose: the blanket rejection of empty text, which is wrong for the two settings whose documented default is empty and is filed as #280 rather than folded in where it would be invisible; and the absence of the `count` settings, which no caller submits and which the admin screen has no control for. I had started adding count validation and reverted it — widening behaviour under cover of a complexity fix is how a refactor stops being reviewable.
The three S6478s are render props, not components defined during render. ErrorBoundary's `fallback` is typed `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)`, so React only ever sees returned elements and never a new component type — the subtree destruction the rule describes does not happen, and the rule's own message offers `allowAsProps` for this shape, which cannot be set from here. Hoisted rather than suppressed because none of them closes over anything local, so at module level each is one stable function instead of a new closure per render. That is a mild improvement, not a contortion.
Verified: 402 backend unit, 358 backend integration, 30 frontend unit, 157 e2e, both lints clean, both builds clean. The e2e run matters most here — storefront-errors.spec.ts exercises all three hoisted fallbacks, and it was run on its own first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured rather than described. Everything in here was run against danielgatis/rembg:latest on 2026-09-02 and the numbers are from those runs, with the caveat recorded that a dev box under Docker Desktop is not the NAS.
The finding that matters is the default model. This image downloads bria-rmbg on first use, and BRIA's RMBG models are licensed for non-commercial use — the same trap that ruled out @imgly/background-removal-node, reached silently by making one request. Sending model=u2net explicitly is both the licensing answer and ten times faster: 1.1-2.3s against 14-20s for a 2000x1500 image, and 168MB on disk against 977MB. That speed difference decides the interaction on its own, because a click can wait two seconds and cannot wait twenty.
Also records the volume path, which most documentation online gets wrong for this image: models land in /root/.rembg, not /root/.u2net, and mounting the old path silently re-downloads 168MB on every start.
The output was verified rather than assumed — PNG, four channels, alpha spanning 0 to 255, dimensions preserved, 1.7x the input JPEG's bytes.
Marked as evaluated rather than adopted, since the engine choice is still open, and the three things not established are listed: quality on a real photograph, behaviour on the NAS, and concurrency. The test input was a generated rectangle on a flat ground, which says nothing about a chipped vase on a patterned rug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bermudalambandClaude Opus 5