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>
When the integration run lost its schema partway through, it presented as 36 assertion errors about categories, price filters and favourite notifications. The real message — `relation "items" does not exist` — was further down the same log, and hours went into chasing the assertions instead.
The cause is still open and needs runner-side evidence this cannot reach: whether the Postgres service container is being recreated mid-run, which would come back with an empty data directory. This is the half that can be fixed from here — whatever the cause, the next occurrence reads as "the database lost its schema" on the first line, names which tables are gone, and says that nothing in the suite drops tables so the database was replaced underneath the run.
globalSetup asserts once after migrating, which establishes the fact the rest of the run depends on. Without it, a run that never had a schema and one that lost it midway are indistinguishable from the failures they produce.
resetDb checks only when its TRUNCATE fails, rather than on every reset. It runs in a beforeEach several hundred times a suite, and an extra round trip each time to guard against a rare event would be paying continuously for it. When the schema is fine, an unrelated failure is passed through untouched rather than dressed up as a schema problem.
The diagnostic is tested against a database that has actually lost its schema, not reasoned about. An earlier attempt dropped the schema before the run and proved nothing — globalSetup re-migrates, so it repaired itself and every test passed. The suite drops and rebuilds around each case and restores in afterAll; the full integration suite was then run twice to confirm the restore holds for everything ordered after it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The admin version stamp has reported commit "unknown" everywhere. #233 read it out of .git during the build and #235 removed that, because Portainer's build context has no repository history and the COPY failed every deploy — the version stamp became the thing that stopped deployments. #237 then established that building in Gitea Actions does not help: the Dockerfile no longer copies .git, so where the build runs is irrelevant.
So the builder hands the commit over rather than the build going to look for it. ARG GIT_COMMIT, empty by default, passed through to writeBuildInfo, which prefers it and still falls back to reading .git so a local build stamps itself with no argument needed.
An empty value is treated as absent rather than stamped. `--build-arg GIT_COMMIT=` is what an unset shell variable expands to, and a blank commit reads as one that happens to be empty rather than one nobody supplied.
Nothing regresses for Portainer. It cannot pass the argument, so its images keep saying "unknown" exactly as today, and they still deploy — the property #235 was bought with.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerate set state='queued', cleared attempts and ai_error, and answered 200 — then nothing ran the worker, so the row sat until the five-minute sweeper happened along. From the admin's side that is indistinguishable from a dead button, and the obvious response is to press it again.
The submission path has kicked the worker since #223. Both paths put a row into 'queued'; only one asked for it to be drafted. That was an oversight in #225 rather than a decision: the kick was added to the submission path in a later task and the action routes were never revisited.
Fixed in both places that re-queue — the admin route and the signed regenerate link from the notification email, which sets the identical state. Fire and forget with a logged catch, exactly as the submission path does: a slow or failing model call must not become a failed request for the admin, and the sweeper is still the backstop if the kick misses.
The card still does not update itself once the draft lands, since drafting takes a few seconds and the screen has no way to know it finished. That is a UI question rather than this bug, and is noted on #272.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every draft in QA failed with a 400: "anthropic-workspace-id is required when authenticating with an identity-linked API key". A key issued against a workspace, rather than standing alone, is refused unless the request names the workspace it acts in — and the client was constructed with an API key and nothing else.
Nothing about a key's shape says which kind it is, so no amount of configuration checking would have caught this. Only a real call would, which is exactly what #223's task 8 existed to make.
Sent only when ANTHROPIC_WORKSPACE_ID is set. Plenty of keys need no workspace, and sending an empty header would turn the ordinary case into a different error rather than leaving it working. Both compose files carry it with an empty default so an unset variable cannot fail a deploy, and the cutover doc goes from fifteen interpolated names to sixteen — checked against the file, and every name in the list now matches one in the compose.
The failure handling needed no change and got none. The submission kept its photos, the draft recorded ai_error, and the review queue showed the reason. A model call failing must never lose somebody's consignment, and it did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The measures step reported "Quality gate ERROR" and "Code smells 5", which is enough to notice debt and useless for clearing it. It now also prints the failing gate conditions, the open issues with their rule, file, line and effort, and the security hotspots awaiting review.
#181 is why this matters. It assumed the five smells were the five eslint-plugin-sonarjs warnings, on the strength of the counts matching, and hedged that the server's rule set is not the plugin's. The hedge was right: those five warnings were fixed under #261, both workspaces lint at zero, and the analysis of the #261 merge still reported five smells and 24 minutes. They are a different five, and nothing short of the list settles which.
The gate condition list matters for the same reason. "ERROR" sends a reader to a dashboard, which is the thing this script exists to avoid needing.
Issues are capped at 25 rather than paged: past a couple of dozen the answer is not "read the list", and an unbounded fetch on every CI run is a cost with no reader.
Verified against a stub SonarQube serving canned responses, so the parsing and formatting are exercised rather than reasoned about — failing conditions filtered from passing and NO_VALUE ones, components stripped of their project-key prefix, absent metrics rendered as a dash, and every path still exiting 0. The no-server and unreachable-server paths were exercised too; the step still cannot fail the job.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
start-local.ps1 said "something is already listening on 3000; leaving it alone" and carried on. That is safe only while the database has not changed underneath that process. When it has, the old backend is serving a database it no longer owns and its in-memory state describes rows that no longer exist.
The rate limiter is the sharpest example. It keys on customer id and keeps buckets in memory for an hour, so a recreated database restarting ids at 1 hands a brand-new customer a previous run's spent allowance. That is #257: the resend-verification allowance test failing roughly one full run in three, never in isolation, with three refusal toasts where one was expected.
Reproduced deterministically rather than reasoned about. Fresh database and fresh backend: three of three pass. Recreate the database only, leaving the same backend running: the same test fails with exactly the reported "resolved to 3 elements". Control — recreate the ids again but restart the backend as well: passes. So the variable is the process outliving the database, not the id restart on its own.
That also explains why #257 could not find the mechanism. It had ruled out contention, a mis-keyed limiter, a shared fixture and identity reuse in the test helpers, all correctly. The recycling happens outside the suite entirely, in a process the suite never sees.
Now it refuses, names the reason, and says to run -Stop. A run that stops loudly is recoverable; one that quietly tests the wrong thing is not — and a stale listener on 3000 has already produced two wrong measurements in this project, a rate-limiter reading and an e2e run reported as 23 passed when the backend was talking to a deleted database.
DatabaseIsNew is set when -Fresh removes the container, when the container is created, and always under -E2eDb, whose tmpfs storage means it comes up empty whether created or restarted.
Verified by AST-parsing the script and confirming every reference to the flag is script-scoped — a function-local read would see $null and the guard would never fire. The script is deliberately never executed from an agent shell.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The e2e suite ran against the development database and nothing truncated it. Every run seeded more fixtures and left them, so the unfiltered storefront grew monotonically — 1,662 items by the time #186 was filed — until rendering it outran the assertions' timeouts. It failed locally, passed in CI where the database is fresh, and got steadily worse, which is the combination nobody can act on.
start-local.ps1 -E2eDb runs the stack against a separate container on a separate port with tmpfs storage, so it starts empty every time. Migrations already run on every start, so an empty volume is a working one. The development database is untouched, so anything set up there by hand survives.
Deliberately a third database rather than sharing either existing one. The integration suite truncates between tests, so an e2e run sharing with it would have its fixtures deleted underneath it (#116) — different container, different port, different credentials, so the mistake is impossible rather than discouraged.
Verified by recreating the database from the compose file, confirming it came up with zero items, and running the full suite against it: 157 of 157. An earlier attempt at this reported 23 passed and 53 not run, which was worthless — a stale backend from a previous run still held port 3000 and was talking to a database I had already deleted. The port is checked before the run now, and the same mistake produced a wrong rate-limiter measurement earlier in this work.
Pagination is the other half of #186 and is filed separately: a shop that renders its whole catalogue in one page is worth fixing on its own merits, not as a side effect of a test fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
discardUnlessAccepted unlinks from a res.on('close') handler, so nothing awaits it and nothing can. `await request()` resolves when the response completes, which is when close fires — so a read taken straight afterwards races the unlink it is meant to observe. The property is an eventual one and the assertions were synchronous.
The issue counted three tests. Injecting a 400ms delay into the unlink to make the race deterministic showed five: "leaves nothing on the volume when it refuses the content" and "discards the valid files from a request that also carried an invalid one" race too, and are not in the describe block the issue named.
It also showed that polling alone is not enough. The race runs in both directions, and the second direction is easy to miss: a deletion still pending from the *previous* test corrupts the next test's baseline before its request is even sent. No amount of waiting fixes a baseline that is already wrong. My first attempt waited for the directory to look quiet, which only works while the unlink is faster than the wait — precisely the assumption this issue is about, and it still failed four tests under the injected delay.
So the baseline is removed as a variable: beforeEach empties the directory, every test starts from empty, and the assertions poll for the expected count. Any orphan from an earlier suite goes with it, which is correct — the directory is temporary and nothing outside these tests owns it. discardUploads already catches per-file errors, so a pending unlink finding its file gone logs and moves on.
Verified by injecting the 400ms delay again: four to five failures before, twelve passing after, with the production file restored untouched.
Closes#228
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
node-pg-migrate keeps the schema; Drizzle is for queries only. The conventions doc said this was unsettled and now says what was settled and why: drizzle-kit cannot diff expression indexes and emitted six statements for one column, our migrations are mostly prose that generated SQL does not carry, and data migrations cannot be generated at all.
The reasoning in full is on the issue. This is the version a reader converting a query will actually find.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Columns in a sql template render unqualified, so a correlated subquery correlates with itself and returns a plausible wrong number rather than failing. That is worse than the array trap already recorded here, which at least produces invalid SQL — this produces valid SQL and quietly wrong data, and only an integration test asserting a value caught it.
And a driver error code moves when Drizzle wraps it, so a catch keyed on a SQLSTATE still compiles and silently stops matching.
Also records that the camelCase mirror and the snake_case API mean every select must map columns explicitly, because selecting the table changes the JSON contract with nothing to notice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All nine sites, chosen because the file is awkward rather than easy: a recursive CTE consumed two ways, a correlated subquery, an array match, and two error paths keyed on a Postgres SQLSTATE. A file of plain CRUD would have produced a flattering number that does not generalise.
The hand-declared row interfaces are gone. CATEGORY_COLUMNS is written once and the row type is inferred from it, which closes the drift itemSelect.ts documents as "KEPT IN STEP BY HAND". That mapping has to be explicit rather than selecting the table: the mirror names columns in camelCase and this API answers in snake_case, so selecting the table directly would have silently changed the JSON contract the admin frontend reads, and no test asserting status codes would have caught it.
strict and noUncheckedIndexedAccess hold with no non-null assertions added. requireRow covers the RETURNING rows and the existing lookup destructures and branches, exactly as before.
Two bugs were introduced and caught by the integration suite, and both are worth recording because neither produced a type error.
Drizzle renders a column reference inside a `sql` template UNQUALIFIED. `${items.categoryId} = ${categories.id}` became `WHERE "category_id" = "id"`, which Postgres resolved against items on both sides — so the item count came back plausible and wrong rather than failing. That is worse than the documented array trap, which at least produces invalid SQL. The fragment is now literal text, which is honest since it binds no values.
And the driver's error code moved. Drizzle wraps errors, so the SQLSTATE that sat on err.code now sits on err.cause.code; the old check compiled, never matched, and turned two 409s into 500s. isUniqueViolation accepts both shapes.
inArray replaced the ANY(...::int[]) match and sidesteps the sql.param trap entirely — there is no template to forget it in, and the builder emits the placeholder list correctly by construction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backend/drizzle/ was `drizzle-kit`'s output from the #216 spike, superseded now that pull writes into src/db-drizzle. Removing it also removes something that should never have been on main.
#216's closing comment said the spike branch carried "an experimental condition_note column that must not reach main". Checking backend/migrations for it found nothing, which is where I stopped looking last time — but it was here, as backend/drizzle/0001_add_condition_note.sql. It reached main in the same merge that brought the Tinqer probe #261 removed.
Nothing ran it. node-pg-migrate only executes backend/migrations, so this SQL was inert and no database has the column. It was a loaded gun rather than a fired one, which is the only reason this is a cleanup rather than an incident.
That file is also the evidence #219 needs, so it is quoted in that issue before being deleted: adding one nullable column emitted three DROP INDEX statements and three CREATE UNIQUE INDEX statements alongside it, for the expression indexes drizzle-kit could not diff. On a large table those recreations take real locks, and a generated migration nobody read would have taken them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Infrastructure only. No route is converted, nothing changes at run time.
The mirror had already drifted, which settles how it should be maintained. schema.ts was missing item_drafts and upload_links from the moment #222 landed, because the spike pulled into ./drizzle and copied the file into src/ by hand, and nobody had reason to look at the copy for a week. So `out` now points at src/db-drizzle and pull refreshes in place — the copy step that made the drift possible is gone — and tablesFilter excludes pgmigrations, which is node-pg-migrate's bookkeeping and has no business in a model of the application's schema.
A stale mirror is worse than no mirror, because Drizzle infers row types from it: a converted query would type-check against a schema the database does not have and fail at run time on a column that does not exist. drizzleSchema.integration.test.ts fails when the two disagree, on tables and on columns. It was checked by removing item_drafts from the mirror and confirming the test fails naming it, rather than trusting a green run on a file that already matched.
pull also emits 0000_*.sql and meta/ into `out`, because that directory serves both purposes. Both are gitignored: this project's migration history is backend/migrations, hand-written and mostly prose, and #219 has not chosen otherwise — a stray SQL file in src/ is at best noise and at worst mistaken for real history.
db is exported beside pool and shares its connections. Both must work at once, since conversion is file by file across 187 sites; separate pools would make a transaction on one invisible to the other and silently double the configured limits.
The generated files are excluded from linting. #261 hand-fixed an unused-parameter warning in schema.ts and this re-pull put it straight back, which is the argument in one line: linting generated code buys a fix the next regeneration undoes. itemFilters.drizzle.ts, which is hand-written, is still linted.
CONVENTIONS.md records the sql.param() array trap before anyone hits it — the wrong form type-checks, reads correctly and fails at run time as invalid Postgres — and the reason the adoption is worth doing at all, which is that ${value} emits a bind parameter and there is no way to spell "interpolate this as SQL" by accident.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bermudalamb
