3659608abe66efcfae1ba39528ec51af7503e00b
108
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
64f8efb617 |
fix(admin): offer Remove and Restore independently (#293)
One button whose label flipped on "is every photo cut out?" could not serve a partly cut-out item, which is not a hypothetical state: it is what a partial removal leaves behind, and it is also what happens when REMBG_URL goes away after some photos were already done. In that state the single button read "Remove backgrounds", so the cut-out photos the item already had could never be restored from this screen. Remove and Restore are now separately gated and can appear together, which is correct — Remove finishes the job on what is left, Restore undoes what is already done. Restore is deliberately not gated on the backgroundRemoval config flag. Gating it would strand cut-out photos with no way back in exactly the environment that most needs the undo. Remove stays gated, so an unconfigured environment shows no button rather than one that reports zero of four done every time. The emptiness check moves from `!== null` to `!= null`: original_image_path is optional on the shared Item type because the public storefront response omits it, so a stray undefined has to count as "not cut out" — `undefined !== null` is true, which would misread a public-shaped item as fully cut out. The modal now refreshes on a non-ok response too. A restore that fails partway can still have swapped some files back before it failed, so returning early left the thumbnails showing files that are no longer on the server. The warning text is now driven off whichever count the action reports, so a partial restore says how far it got the same way a partial removal already did. The e2e spec seeds its item into a category of its own and filters the table down to it. The inventory table paginates at 10 and the suite runs fullyParallel, so an unfiltered page one was never a reliable place to find the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
dbb63bc3d2 |
fix(admin): keep the active inventory filter and surface background-swap failures (#293)
handleBackgrounds re-read /api/admin/items unfiltered and called setItems(all) after every remove-backgrounds or restore-originals call, so an admin who had filtered Inventory to one category and opened an item from that filtered view saw the table silently repopulate with the entire unfiltered catalogue the moment the request resolved. Every other mutation in this file goes through load(), which respects the active filters; this one didn't, for no reason the spec required. The fix reuses load() instead: it now hands back the rows it fetched (previously discarded after setItems), and handleBackgrounds picks the edited item's fresh row out of that filtered result to refresh the open modal, rather than issuing a second unfiltered fetch. There is no GET /api/admin/items/:id route to fetch a single item directly, and the remove-backgrounds/restore-originals routes return only a summary, not the item, so load()'s own result is what's actually available. A background swap never touches the fields anything filters on, so the edited item stays in the filtered result whenever it was in it before. Also added a catch to handleBackgrounds, matching the message.error shape every sibling handler (handleDelete, handleDeleteImage, handleStatusChange) already uses — previously a network drop or a malformed JSON body became an unhandled rejection with no toast, silently different from how the rest of the file reports failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
385d5b89bf |
feat(admin): offer background removal where an item's photos are edited (#293)
One button per item in the inventory editor, beside the per-thumbnail delete buttons rather than on them, because an upload is one item and its photos are views of one thing. Its label is derived from the images rather than stored: Restore originals when every photo already carries an original, Remove backgrounds otherwise. The otherwise deliberately covers the mixed state a partial failure leaves behind — with two of four cut out it reads Remove backgrounds, which is the action that finishes the job, and pressing it skips the two that already worked. Rendering is gated on the feature being configured or every photo already being cut out, not on the flag alone. Gating on the flag would hide Restore originals the moment REMBG_URL is unset, stranding cut-out photos with no way back — the same reasoning the review queue's control already uses. The editor is a modal and this changes files on the server while it is open, so the item is re-read afterwards and the open modal updated. Without that the thumbnails keep showing the previous files and the button looks like it did nothing, which is the bug this was most likely to ship with. frontend/src/api.ts gains original_image_path on the shared Item type's images, since ADMIN_ITEM_SELECT's images aggregate carries it and the public catalogue's does not. It is added as optional rather than required because Item is the same type fetchItems() uses for the public storefront, and a required field the public response never sends would be a type that lies about what is actually there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
046937cdce |
test(e2e): assert a created link's address shows in the table (#260)
The spec's E2E section requires that a created link shows its address in the table, but the test that creates one filled a throwaway address and never asserted the cell. The address is now kept in a variable and asserted on the row after creation, using it to locate the row's own cell rather than any other row's. Not run: the local stack is down and Playwright was explicitly out of bounds for this pass, per the task instructions. This is written and verified by inspection and lint only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c12d40dc02 |
fix(admin): stop the upload links table claiming delivery it never recorded (#260)
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> |
||
|
bermudalambandClaude Opus 5
|
9c5935e8e0 |
feat(admin): ask for the contributor's address when creating a link (#260)
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> |
||
|
bermudalambandClaude Opus 5
|
1bd0024e6f |
test(storefront): cover paging, and say what the filter tests meant (#269)
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> |
||
|
bermudalambandClaude Opus 5
|
f9073c515e |
feat(storefront): page the catalogue instead of rendering all of it (#269)
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> |
||
|
bermudalambandClaude Opus 5
|
013b6abb52 |
feat(storefront): the rules for paging the catalogue (#269)
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> |
||
|
bermudalambandClaude Opus 5
|
786996b7ac |
fix(admin): keep Restore original working after REMBG_URL is unset (#281)
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> |
||
|
bermudalambandClaude Opus 5
|
09686d9f94 |
feat(admin): remove or restore a photo's background from the review queue (#281)
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>
|
||
|
bermudalambandClaude Opus 5
|
1902db6d04 |
feat(intake): offer background removal on the submission page, ticked (#281)
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> |
||
|
bermudalambandClaude Opus 5
|
cd2676233c |
chore: clear the six code smells SonarQube reported (#181)
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> |
||
|
bermudalambandClaude Opus 5
|
06933aec75 |
fix(scripts): make the alias check fail closed, and correct three stale docs (#208)
The alias check was a negative match on an allowlist of English error strings, which returned True for empty output, for $null, and for "exit status 1: Access is denied." — so an alias switch producing nothing, or failing on the symlink permission error this file's own header warns about, was reported as success while the old version kept running. That is the bug #198 was filed about, narrowed rather than removed, and it also broke whenever nvm reworded an error. It is now a positive match on "Now using node v<what is actually running>". The floor check moves ahead of the switch and reads the constant rather than the result. Where it sat, $major was always whatever NODE_VERSION says, so it validated the switch it had just made instead of the pin it exists to guard, and could never fire. Use-NodeLatest is now Use-PinnedNode. In a change whose whole subject is that "latest" means something people do not expect, the name was an avoidable trap. The restore default moves beside NODE_VERSION. It deliberately is not a param default: a param block runs before the dot-source, so $script:DEFAULT_NODE_VERSION is still $null there and the restore would have quietly restored nothing — leaving the machine on the pinned version, which is the exact failure the restore exists to prevent. It is resolved after the dot-source instead, and an explicit -DefaultNodeVersion still wins. Three documents described behaviour the code no longer has: README's "both scripts run nvm use latest", run-tests.ps1's .DESCRIPTION, and project-context.md's instruction to agents. All corrected, and project-context.md now also says not to run these scripts from an agent shell, which is how this machine once ended up with no Node at all. Part 4 of the issue is partly stale: backend/package.json already declares engines >=20.9.0. frontend now matches it. The larger question — whether local should be pinned to the Node 20 that CI and the production image actually run — is a decision rather than an oversight and is left open on the issue. Verified by parsing all three scripts with the PowerShell AST parser, which does not execute them. They are deliberately never run from an agent shell. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
fdc92d8f19 |
test(intake): cover reviewing and publishing a submission (#225)
Seeded through the intake route rather than POST /api/admin/items, which writes no item_drafts row — an item created that way would never appear in a queue that joins that table. Going through the link is also the path a real submission takes, so the test exercises what actually ships. Two cases. The first edits the price and publishes, which confirms the number, so no dialog appears and the item reaches the storefront at what was typed. The second publishes an untouched price and asserts the confirmation names the figure and the fact that nobody chose it, then cancels and checks the item is still unconfirmed. That second case is the protection this whole screen exists to provide. Scoped to the card each test creates, located by the sender's note carrying the run id — the item's own name is a submission timestamp and not unique to the test. The dialog is matched on its accessible name because antd nests the confirm title in two elements and getByText resolves to both. Full suite 157 of 157, up from 155. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
2b06538ef9 |
feat(intake): review, edit and publish drafts from the admin (#225)
The screen that makes the intake pipeline usable. Until now a draft existed only in item_drafts and nothing rendered it, so a successful draft and a failed one looked identical from the admin — the item shows its placeholder submission-timestamp name either way, and telling them apart needed SQL.
The price carries the weight the schema no longer does. It is labelled with where the number came from, anything not set by a person is marked unconfirmed, and publishing at an unconfirmed price asks first rather than reporting afterwards. Editing the field is what confirms it, so opening the card and leaving the price alone is not recorded as approval — the same rule the server applies, which this only has to agree with.
Discard is offered rather than delete, and a discarded card offers Restore in its place.
The e2e page object's AdminTab union is extended alongside the tab itself. It is a closed union, so admin.open('Review queue') would not type-check without it — and the tab strip and that union have to be changed together or the next spec to use it fails to compile.
Both frontend lint and build clean, still at zero warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
fe28c97e0f |
chore(sonar): remove the rejected Tinqer spike, clear the lint debt, and report measures in CI (#261)
The standing cleanup, three features behind. Four changes. Report the measures in CI. This is the one that matters, because the rest was only findable by reading the tree. SonarQube here is 9.9 Community: no Bearer auth, so the official MCP cannot connect, and the host is a CI secret, so hotspots, duplication, debt and coverage existed only on a dashboard — which made "reduce the debt" an instruction nobody could act on without a browser open beside them. scripts/summarize-sonar.js queries the measures API with the secrets the workflow already holds and prints the result into the job log. The scanner masks the URL and token; measures are not secret. It polls the compute task before reading. The workflow does not set sonar.qualitygate.wait, so the scan step returns once the report is uploaded and the server computes measures afterwards — reading immediately would return the previous analysis, indistinguishable from this one and quietly wrong. When it cannot confirm, it says so in the output rather than presenting stale numbers as current. It is deliberately not guarded with continue-on-error: it exits 0 on every path, and guarding it would oblige it to appear in the final gate, whose job is to fail the build. Remove the Tinqer spike. #216 evaluated Drizzle against Tinqer and rejected Tinqer, and its closing comment said the throwaway src/db-tinqer/ probe must not reach main. The whole spike commit was merged, so it did. The probe is 71 lines imported by nothing, and @tinqerjs/tinqer, @tinqerjs/pg-promise-adapter and pg-promise were dependencies for a library nobody chose. The condition_note column that warning also named did not reach main. Clear the lint debt, both projects now at zero warnings from six and two. One of these was a real defect rather than tidiness: the third catch block in shippingAddresses.ts rolled back and returned 500 while discarding the error, so a failed default-address change left nothing behind to say why — the two catch blocks above it in the same file already logged, and this one had simply been missed. The Express namespace augmentation is a false positive and is disabled with the reason written beside it, because an interface that must merge into one Express declares inside a namespace has no ES module spelling. Dedupe the extension map. backfillImageReencode.ts kept its own .jpg/.png/.webp table whose comment named uploadTypes.ts as the source of truth, directly above duplicating it. That file rewrites stored images, so the two disagreeing would silently skip files it should re-encode. src/db-drizzle/ deliberately stays. #217 is open to promote exactly those files properly, with tablesFilter and the sql.param() array rule; deleting them here would be doing #217 badly in the wrong issue. Only their unused-symbol warnings are fixed, and if drizzle-kit pull regenerates schema.ts the table warning returns — worth #217 knowing. Hotspots and coverage are untouched because both numbers are still invisible. They are the next pass, once the step above has printed them once. Closes #261 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
94b2dfd186 |
fix(e2e): match the modal OK button exactly so a random suffix cannot collide (#253)
Playwright matches an accessible name case-insensitively and as a substring unless exact is passed, so `getByRole('button', { name: 'OK' })` matched any button whose name merely contained "ok".
Caught on a full serial run of main:
strict mode violation: getByRole('button', { name: 'OK' }) resolved to 2 elements:
1) aka getByRole('button', { name: 'Freed rmtiq9okg22k9e' })
2) aka getByRole('button', { name: 'OK', exact: true })
A leftover "Freed …" toast was still on screen and the random base36 suffix happened to contain "ok". Roughly one suffix in a few hundred does, which is the profile of a test that fails occasionally and reproduces for nobody.
This is not the contention #241 addressed. It reproduced with workers: 1, serially, on a fresh database — it needs only a stale toast and an unlucky suffix. It is at least part of what #245 skipped a real test to work around.
Also makes the taxonomy modal's OK and the sold-filter 'All' radio exact, the only other targets short enough to appear inside a random suffix. The dozen or so remaining short names are left alone deliberately: they would need a collision in deliberately-chosen test data rather than a random one, and substring matching is load-bearing in some of them.
Verified with two consecutive full e2e passes, 155 of 155 both times.
Closes #253
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
42412e8eeb |
fix(e2e): restore the frontend build by typing the responses findOrFail searches (#254)
main did not build, so every Portainer deploy failed with `npm run build` exit code 2. My regression from #241. findOrFail<T>(items: T[], predicate: (item: T) => boolean) infers T from both parameters. The call sites annotate the predicate as documentation, and the arrays come from .json(), which is any and offers no competing candidate — so T became the one-field shape written in the lambda and every caller failed on the field it actually wanted. Array.prototype.find has no such problem, which is why the code this replaced type-checked. The four responses are now typed at their call sites, so T is inferred from real data and the predicates need no annotation. findOrFail additionally takes NoInfer<T> on its predicate, so a stray annotation can never drive the element type again. The specs are better typed than before this change: `.json()` was plain any, and the annotations only ever documented a shape nothing enforced. I did not catch this because I verified with a bare `npx tsc --noEmit`, and tsconfig.json is `"include": ["src"]` — it structurally cannot see tests/. The specs are checked by the second command in `npm run build`, which is the step Docker runs and the step that failed. Verified this time with `npm run build` itself, plus lint, the unit suite, and two consecutive full e2e passes. Closes #254 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d27dcae62b |
feat(admin): choose the drafting model from Settings (#223)
The model was going to be an environment variable, which meant a redeploy to change it. It is now an admin setting, so it can be changed from the Settings page like the cart expiry and the greeting. A dropdown validated on the server, not a free-text field. The API only rejects an unknown model at the point of use, so a typo would be stored happily and then fail on every submission, surfacing as drafts quietly not appearing rather than as an error anybody could act on. The PUT refuses anything outside the offered set, and getSettings falls back rather than handing on a value that is no longer offered — drafting with the default beats drafting with a model the API will refuse. One catalogue rather than two lists. The dropdown needs the models, costMicros needs their rates, and the price shown beside a model in Admin has to be the price it is actually billed at, which it cannot be if the two are maintained separately. Rates were confirmed against the pricing page rather than recalled: Sonnet 5 $2/$10, Opus 5 $5/$25, Haiku 4.5 $1/$5 per million tokens. The unknown-model fallback is deliberately the most expensive rate and never zero, because a budget that reads as unspent however much was spent is the one failure a spend guard cannot have. Adding a third setting type pushed getSettings past the cognitive complexity limit, so the per-type resolution moved out into one small function each — the same shape the definitions block above it already argues for. The exhaustive assertion in the GET test gained the new field rather than being loosened. It exists to catch a setting silently vanishing from the response, and that is worth more than not having to touch it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
6489486de3 |
test(e2e): run the suite on one worker so a red run means something (#241)
The lever the plan deliberately held back, applied now that there is evidence it is needed. Better failure messages and a wider assertion timeout were not enough. Measured on this branch, same commit, same machine. Two parallel runs each failed 3 of 155, and not the same three: password-reset and admin-inventory-filters in one, admin-email-settings, admin-inventory-filters and resend-verification in the next. All 16 tests from those three specs then passed when run serially, and the full suite passed 155 of 155 twice in a row. Failures that move between runs of identical code are contention, not defects — specs asserting over tables other specs are concurrently writing to. The cost is about three minutes, roughly one minute parallel against 3.9 and 4.0 serially. A red run that means something is worth three minutes; the previous state was a suite whose result nobody could act on, which is what #245 had to skip a real test to work around. If that time is ever needed back, the cheaper fix is giving each worker its own database rather than raising this number and reopening #241. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
895c08d1a7 |
test(e2e): replace unchecked lookups, widen the assertion timeout, restore the skipped test (#241)
Tasks 2 to 4 of the plan. Nine `collection.find(...)` dereferences become findOrFail, so a missing row fails as a named assertion naming what was wanted and how many rows were searched, rather than "Cannot read properties of undefined" pointing at test plumbing. Where the old code followed the lookup with expect(x).toBeTruthy(), that assertion is dropped: findOrFail already guarantees it, and with a better message. The expect timeout goes from Playwright's default 5s to 10s. It costs nothing on a green run — it bounds how long a failing assertion waits, not how long a passing one takes — and #239 died reporting exactly Timeout: 5000ms on a runner that also builds, migrates and runs three other suites. The admin-save happy path comes back from the #245 skip. It is the only end-to-end check that adding an item reaches the database rather than merely firing a toast, and it passed both full parallel runs and in isolation. filters.spec.ts:216 is deliberately untouched: its .find() searches CSS class names on a string array, not test data, and has no missing-row failure mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
bcecda9122 |
fix(intake): stop a throttled sender being told their link is dead (#222)
Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for. One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it. Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message. Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want. The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions. Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store. The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole. Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings. Ref #222, #241 |
||
|
bermudalamb
|
9b4d7f2d03 |
feat(intake): manage upload links from the admin (#222)
An Upload links tab beside Tags: issue a named link, see how much of its allowance is spent, revoke it. Until now the only way to create one was curl, which is how the earlier tasks were exercised. The token is shown once, in an alert that says so plainly, because the server stores only a digest and genuinely cannot produce it again. A refresh loses it — that is the honest behaviour rather than a bug, so the copy says to revoke and reissue if it is lost instead of leaving somebody hunting for a reveal button. The cap field starts at 25 and unlimited is a checkbox rather than an empty field. Blank-means-unlimited would make the least deliberate action produce the least bounded link, and this screen sends all three cases explicitly so the server's default only ever has to cover callers that are not this screen. Two things came out of driving it in a browser rather than reading it. The revoke confirmation said "OK", and every other destructive confirm in this admin names its action — Delete, Disable, Re-enable — so it now says Revoke, in danger styling. A confirm button reading OK makes the reader go back and re-read the question to find out what they are agreeing to. And Popconfirm turned out to be a component nothing else here uses; the rest use Modal.confirm with an explicit okText. Keeping Popconfirm but matching its labelling to the established pattern seemed the smaller inconsistency, since the interaction is a row action rather than a page-level one. The load-on-mount effect carries the same eslint-disable and reasoning Tags and Categories already use, rather than a new shape. Verified in a browser: create shows the one-time reveal, the row lists as 0 of 25 and Active, revoke flips it to Revoked, and an explicitly unlimited link shows a bare count with no cap. The database then confirmed a default of 25, a null for the unlimited one, and a stamped revoked_at. Frontend: build clean, lint unchanged at 2 pre-existing warnings, 30 unit tests pass. Ref #222 |
||
|
bermudalamb
|
e7b01fdb36 |
feat(intake): add the public submission page (#222)
Where someone with no account sends in photos of one item. Route /submit/:token, outside the authentik gate by design: the token in the URL is the whole access control, which is what #222 chose deliberately over accounts. One state for every refusal, matching the server's single 404. Unknown, revoked and used-up links all render the same "this link is not active" card, because saying which kind of dead it was would tell a stranger whether a link they guessed at exists — the server is careful about that and the page must not undo it. `beforeUpload` returns false so antd keeps the files rather than uploading each one as it is picked. The submission is then a single request the server can accept or refuse as a unit, which is what makes the transaction on the other side meaningful. The accepted types and the six-file cap are stated here so the picker offers exactly what will be taken, but both are checked again server-side, because everything on this page is under the sender's control. The fetch effect guards against a late response from a previous token overwriting the current answer, which is reachable simply by editing the URL. TypeScript caught a real mistake rather than a stylistic one: `.filter((f): f is File => ...)` on antd's originFileObj does not narrow, because RcFile extends File and the predicate would widen rather than narrow. flatMap avoids the predicate entirely. Verified in a browser rather than by inspection: a throwaway Playwright run against the live stack confirmed the form renders for a good token, the inactive card renders for a bad one, and a photo can actually be sent and acknowledged. The database then showed the item at status pending with the default price, the draft carrying the note and its originating link, the image row written, the link's counter at one — and zero storefront-visible items, which is the property that matters most. Ref #222 |
||
|
bermudalamb
|
64324609e1 |
test(e2e): add a find-or-fail helper for collection lookups (#241)
Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal. The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase. It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures. Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails. Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean. Ref #241 |
||
|
bermudalamb
|
66c696ce40 |
test(e2e): skip the admin save happy-path test while #241 stands (#245)
`main` has been failing on one e2e test since the sold-filter fix landed, and it is a different test from the one #239 corrected: admin-save-failures' "saves an item successfully when the server accepts it". Skipped rather than fixed, deliberately. It fails in CI and passes locally, and which test fails moves around — a local parallel run of the whole suite on the same commit failed four *different* specs (admin-inventory-filters, auth, favorites-filter, resend-verification) and not this one. That is #241: fullyParallel against a single shared database. Fixing this test on its own would be guessing at a symptom that reappears somewhere else next run. Ruled out before disabling anything: the re-encoding from #226 is not involved. AdminInventory.addItem fills a name and a price and saves, attaching no files, so stripUploadedImages iterates an empty array and the image path is never entered. Checked rather than assumed, because this spec is on the admin save route and that is exactly where a regression of mine would surface. What this stops covering is not trivial, and the comment says so at the call site: it is the only end-to-end check that adding an item actually reaches the database rather than merely firing a toast. #245 exists so that it is un-skipped when #241 lands, rather than left behind. A skipped test on the core admin save path is worse than a red build, because a red build is at least visible. Ref #245, #241 |
||
|
bermudalamb
|
30227fb1e1 |
fix(test): correct the sold-filter tally assertion stranded by #188 (#239)
main has been red since 2026-08-25. Every SonarQube run reported 147 passed, 1 failed, and it was this test every time — expected "Filters", received "Filters (1)". The test is stale, not the code. It was last touched on 2026-08-23 in #137; the tally logic changed on 2026-08-25 in #188, which never touched the spec. #188 redefined the tally as the number of chips and moved the availability preset into the dimension system as a bar dimension — one that still emits a chip for any non-default status, deliberately, because without it `?status=reserved` is an empty grid with no Clear filters button and no way out but editing the URL. The test asserted the rule that held before that change. Counting bar chips differently from drawer chips would restore exactly the per-screen special-casing #188 removed, and the drift it fixed was the admin's tally disagreeing with the storefront's. So the assertion moves, not the tally. The replacement also checks the tally comes back down when the default is restored. The original only ever asserted one direction, which would pass against a count that incremented and never decremented — worth fixing while the test is open rather than leaving a second gap behind the first. There is a real wart left standing: `Filters (1)` opens a drawer with nothing selected in it, because the filter it is counting lives in the bar. That is a cost of #188's design rather than a defect in it, and the comment now says so rather than leaving the next reader to rediscover it. Verified by running the spec in isolation with a single worker: 6 passed. Closes #239 |
||
|
bermudalamb
|
44328d0b5c |
feat(admin): show the deployed commit and build time in the admin (#233)
There was no way to tell which build an environment was running. That is not hypothetical: minutes after #232 merged, `npm run backfill:images` in QA failed with `tsx: not found` because the container was still serving the pre-merge image, and the only thing that revealed it was npm echoing the old script line. Had the change been anywhere other than a package.json script, the container would have looked healthy while running the wrong code.
The header now reads something like `a5076cc · built 29 Aug 20:36`. The commit answers "is this the code I expect"; the build time answers "did my redeploy actually rebuild", which is a different question and the one that would have caught the case above.
The commit is read out of `.git` directly rather than by shelling out, because node:20-bookworm-slim has no git binary and adding an apt layer so the image can print seven characters is a poor trade. `.git` is copied into the build stage only — verified absent from the final image — so no repository history reaches a deployed container.
Resolution is pure and separately tested across every shape that actually occurs: a detached HEAD holding the object name, which is what a checkout of a ref produces; a symbolic HEAD followed to a loose ref file; the same followed to packed-refs, which is what a fresh clone commonly has; peeled `^` tag lines ignored so an annotated tag cannot yield the wrong commit; and every failure path returning `unknown`. That last part is the one that matters most — this runs during a Docker build, and a version stamp must never be the thing that stops a deploy.
Served from a gated /api/admin/version rather than folded into /api/config. That endpoint is public, and a commit hash there would tell any storefront visitor exactly which revision of a public repository is deployed. An integration test asserts the gate and asserts the public config does not carry it, because the boundary is the whole point rather than an implementation detail.
Verified in the built image rather than argued: the stamp inside it reads
|
||
|
bermudalambandClaude Opus 5
|
b897e99363 |
fix(cart): say what the demo button does rather than what the shop does (#203)
#195 added a notice reading "Demonstration only — This shop is not taking payments at the moment", gated on `demoMode` alone. That claim is false in a configuration the ops runbook actively steers towards. `demoMode` and `paypalClientId` are independent. `checkDemoMode` and `checkPayPal` only make the PayPal secrets *required* when `DEMO_MODE=false`; nothing forbids them while it is `true`. And `production-stack-cutover.md:65` says flipping to `false` without all three crash-loops the container — so the only safe order is to populate the secrets while demo mode is still on, verify, then flip. In that window `Cart.tsx` renders live PayPal buttons directly beneath a banner telling the customer the shop takes no payments, and it is precisely the window in which someone is clicking around production checking their work. That is the same failure #195 fixed, pointed the other way: silent where a warning was needed, then confidently wrong where a customer can actually be charged. Telling someone nothing will be shipped above a live PayPal button is worse than saying nothing. The notice now describes the button instead of the shop, which is true in both configurations and stays visible in the one with two controls that do different things — where a customer most needs to be told they differ. Gating it on `!paypalClientId` would also have removed the false claim, by hiding the notice exactly there, which is the worse trade. The test for it was also not testing it. "Says so before the customer commits" seeded an address with `isDefault: true`, and the cart auto-selects the default on load — so an address was already selected and the button already rendered when it asserted. It would have passed with the notice moved inside the `selectedAddressId` guard, which is the regression it exists to catch. It now seeds no address and asserts the notice is up while the checkout button is absent, which states the property directly. Mutation-tested rather than assumed: moving the Alert inside that guard fails the new test, and would not have failed the old one. Verified: 3 end-to-end tests pass against a browser, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #203 Refs #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
39c82ff3a4 |
fix(orders): mark a demo order in the history rather than leaving it to read as real (#205)
#195 and #203 made the cart say a demo order is a demo order. That message is an antd toast lasting about three seconds, after which the cart empties and the card unmounts. Order history is what the customer comes back to when they wonder where their item is, and it said nothing. A demo row was a real row: item name, `$80.00`, status `completed` rendered as a neutral tag because `STATUS_COLORS` has no `completed` key, and `demo` printed raw under a heading reading "Processor". That is not an explanation — a customer has no reason to read `demo` as "this did not happen", and "processor" is not a word they have any reason to know. Two things now say it, for the same reason the cart needed two. The `demo` cell renders as a tag reading "Demo (not charged)", which marks *which* order. A notice above the table, shown only when there is one, says what that means — a tag reading "Demo" still assumes the reader knows what a demo order is, and what they actually want to know is whether to expect a parcel. Nothing changes on the backend: `orders.processor = 'demo'` was already written at checkout and already selected for this page. The row is a real row in a real table and stays visible, because hiding it would be its own kind of lie — the customer did do something, and it did have an effect on the catalogue. Written test-first against a browser: the new case drives a real demo purchase through the cart, opens `/orders`, and failed on both assertions before the change. Verified: 4 end-to-end tests in this spec pass, the 5 existing order-history tests still pass, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #205 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8572e62514 |
fix(cart): say it is a demo where the customer can see it (#195)
Production runs demo mode with no PayPal credentials — that combination is the whole reason #191 turned it on — and in exactly that configuration the storefront rendered a full-width primary button reading plainly `Checkout`. The `(Demo)` suffix was gated on a PayPal client id being present, so the one configuration that needs the word was the only one that never got it. The button is not decorative. It posts to `/demo/purchase`, which marks the item `sold`, writes a `completed` row into `orders` at the real price, and emails everyone who favorited it through production's real SMTP. Nobody is charged, which is what the compose banner promises and is true — but a customer cannot tell they have placed a pretend order, the inventory says otherwise, other customers are told it sold, and nobody is expecting to ship anything. #190, #191 and #192 all reason carefully about not charging by accident; none of them consider accepting an order by accident. Three things now say so, because one of them was never going to be enough: The label is unconditional. `type` still follows the PayPal client id — secondary when real PayPal buttons sit above it, primary when it is the only way to check out — and that distinction is worth keeping, but it is about prominence rather than about what the order is. A notice sits above it for the whole of demo mode, before an address is picked and whether or not PayPal is configured. A parenthesis on the control someone has already decided to press is the weakest possible moment to tell them. The confirmation stopped saying `Order complete!`, which is exactly what a real order says. It now names the two things a customer would otherwise assume: nothing was charged, and nothing will be shipped. Three end-to-end tests, written first and failing first against a browser — the label assertion failed with `Expected "Checkout (Demo)", Received "Checkout"` on an `ant-btn-primary ant-btn-block` element, which is the defect exactly as reported. The suite already runs `DEMO_MODE=true` with no PayPal credentials, so it reproduces production's configuration without any new fixture. `CartPage.checkoutButton` matches the label by prefix rather than in full, deliberately: a locator naming the correct label would have gone looking for the right button and found nothing, which is how a test for this can quietly pass by being wrong in the same direction as the bug. Verified: 3 new tests pass, frontend build clean, lint 0 errors, 25 unit tests pass, and the cart-countdown and orders suites still pass. The favorites and favorites-filter suites fail here, and fail identically with this change stashed — 9 failures without it, 8 with — so they are pre-existing and not from this. Worth their own issue. Closes #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3242018782 |
fix(filters): address the final review findings (#188)
Restores the escape hatch for a status list matching no preset. filtersFromSearchParams accepts any non-empty subset of {available, reserved, sold}, and only three of those seven lists are presets; the other four reported as the not-sold fallback and produced no chip, so ?status=reserved was an empty grid reading "No items yet - check back soon" with no Clear filters button and no way out but editing the URL. hasActiveFilters covered all seven before this branch, so that was a regression against main. availabilityDimension now emits a chip for any status that is not the not-sold preset, labelled from SALE_STATE_LABELS when the list matches one and from statusLabel when it does not. The Segmented still reads "Not sold" beside such a chip, which is a cosmetic wart and the cheaper half of the trade.
Runs the frontend unit suite in CI. The 22 tests added over the dimensions were invoked by nothing: the workflow's only frontend steps were the build and the end-to-end run, and its test:unit:cov step is the backend's. The new step is guarded and named in the gate like every other suite, per the invariant workflowGate.test.ts asserts.
Restores the comment explaining why availabilityDimension's render and chips pass different fallbacks to saleStateFromStatuses. The control must read "All" while sold favorites are on screen; chips must stay silent because the customer never chose it. Unifying them would give every signed-in favorites view a phantom All chip and a tally of 2, and nothing said so after the old markup was deleted.
Derives the storefront's "is anything filtered" and FilterBar's tally through one exported chipsFor rather than two expressions over two identically-built contexts. They agreed only by convention, which is the exact defect this branch exists to remove.
Documents that statusDimension and availabilityDimension are alternatives over one field, since composing both type-checks and would render two controls that double-count it, and asserts the price chip's label text, which was the only chip label nothing checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
8771deeaca | test(filters): cover the tally and availability chip behaviour changes (#188) | ||
|
bermudalamb
|
147e280f88 |
refactor(filters): compose the storefront from dimensions and delete the old components (#188)
Replaces App.tsx's hand-written Segmented/Filters-button/ActiveFilterChips region with a single FilterBar composed from five dimensions, and removes the FilterDrawer and ActiveFilterChips components along with the activeFilterCount and hasActiveFilters helpers they were the only callers of. Catalogue now receives a filtered boolean computed the same way FilterBar computes its own chip tally, rather than the ItemFilters object it only ever used for that one check. |
||
|
bermudalamb
|
2f0da9afe1 | refactor(admin): compose the inventory filters from dimensions (#188) | ||
|
bermudalamb
|
50d048a1d6 | feat(filters): add FilterBar, one component both screens compose (#188) | ||
|
bermudalamb
|
918d6eeab9 | refactor(filters): extract the chip row from ActiveFilterChips (#188) | ||
|
bermudalamb
|
1d7f928c48 | feat(filters): move the availability preset into a bar dimension (#188) | ||
|
bermudalamb
|
eb5799dd17 | feat(filters): add favorites and status dimensions (#188) | ||
|
bermudalamb
|
abd3dd4e2e | feat(filters): add tag and price dimensions (#188) | ||
|
bermudalamb
|
5d0b66a982 | test(filters): add a unit runner and the filter dimension contract (#188) | ||
|
bermudalamb
|
d6e0942487 |
feat(filters): show a tag's own colour on its active filter chip (#185)
A tag carries a colour, and every place a tag appears shows it — a product card, the filter drawer's control, the admin taxonomy screen — except the removable chips beside the Filters button, which rendered every filter as a default grey. Picking `vintage` from a control that showed it in red produced a grey chip of the same name right next to it.
Only tags get a colour, because only tags have one. Category, price, favorites and status keep the default, and that asymmetry is the point: in a row mixing four kinds of filter, colour now means "this is a tag". Nothing depends on it — every chip still carries its label — so this reads the same to anyone who cannot distinguish the colours.
The close control inherits the tag's text colour, so a coloured chip gets a matching cross rather than a grey one on a coloured ground. A tag not yet in the loaded options has no colour to use and keeps the default, which is the same window the existing `Tag {id}` label fallback covers.
The test asserts the chip's colour equals the same tag's colour on a product card, rather than asserting it is red. The colour is derived from the tag's name and free to change; what must hold is that a tag looks like itself wherever it appears, and comparing the two places says that directly. Confirmed to fail without the change — the old grey chip sets no colour class at all.
Verified visually as well as by assertion: three tags selected together render in the drawer, in the chip row and on the card in the same colours.
Closes #185
|
||
|
bermudalamb
|
61c12fd438 |
refactor: remove the duplicated blocks SonarQube found (#182)
Three of the four candidates were real. The fourth was my mistake in the issue. **The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation. `Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly. **The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string. Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did. **The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once. **`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves. Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation. Closes #182 |
||
|
bermudalamb
|
cf1680dbfb |
feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)
The uploads directory is the only place in this application where content someone else authored is served over HTTP. #95 stopped a dangerous file being stored; this stops a stored file doing damage if one ever gets there anyway — through a gap, a path added later, a restore, or a file written before that validation existed. Two halves, complementary rather than alternative. The app's own origin now serves uploads defensively. An allowlist of the three extensions the upload path can produce, so a `.html` or a `.svg` on disk is simply not a file this application hands out — 404, the same answer as a file that is not there, so the response cannot be used to learn which paths exist. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which is a moving target across browsers and years, while this only has to know three. The content type is stated explicitly from that same list rather than sniffed or guessed from a name someone else chose, paired with `nosniff`. `default-src 'none'; sandbox` gives a directly-navigated file no capabilities at all, which is the only way one of these can do harm — an `<img>` embed does not execute script. Writes get 405 rather than falling through to a 404 that suggests the path is wrong. The other half is the separate origin, which is the real fix, because the origin is the whole unit of trust in a browser. That needs a hostname and a certificate, which live outside this repository, so what is here is the switch: `UPLOADS_BASE_URL`, sent to the frontend at runtime through `/api/config` and joined onto stored paths by `uploadUrl`. Empty means the app's own origin, which is the default and what local development has, so nothing changes until it is pointed somewhere. Stored paths stay site-relative. A stored value outlives any hostname baked into it, and rewriting them would be a migration to undo the day the hostname changes. Runtime rather than built in, so one image serves every environment — the same reason `paypalClientId` and `demoMode` are already there. `UPLOADS_BASE_URL` has a line in `docker-compose.prod.yml` while still empty, deliberately: a Portainer stack variable with no line there is substituted into the file and never reaches the container, which is exactly how `UPLOADS_DIR` went missing on 2026-08-23. Unset warns at boot, in the same shape as the admin gate — a working configuration with one defence switched off is worth saying out loud. Set without a scheme is refused outright, because a bare hostname joins onto a stored path as if it were relative and breaks every image on the site rather than failing visibly. The compose guard now resolves `${VAR:-default}` to its default, which is what the container actually receives when the stack variable behind it is unset. A bare `${VAR}` is still left opaque, so a required variable referenced that way goes on counting as present — that check is about the line existing, not about the stack being filled in. Closes #103 |
||
|
bermudalamb
|
9db3c6d94c |
feat(admin): filter inventory through the same flyout the storefront uses (#169)
The two screens asked the same questions through different UI. The storefront had searchable multi-selects in a flyout; the admin still had an always-visible row of controls with a single-select category that held a list of at most one, which is what #139 left behind so the shared filter type would not have to change shape twice. The drawer is now one component with the sections that differ driven by props rather than a second copy that would drift. Favorites is storefront-only. Status is admin-only, since pending is excluded from every public read and Published or Unpublished are not distinctions a customer can draw — the storefront keeps its three-way preset outside the drawer. The price slider needs real catalogue-wide bounds to be honest about where the prices are, and the admin has none, so there it is the two number inputs alone. What is shared is not only the markup but the phrasing: that categories are OR and tags are AND has to read the same on both screens or it stops being one rule. This reverses a decision `InventoryFilters.tsx` argued for in a comment — that hiding controls above a data table costs more than the space it saves, and that a drawer overlays the very rows being filtered. Both are true and both are traded for consistency between the panels. The active-filter chips are what makes the trade bearable: the current filter stays readable beside the button without opening anything, which is the part the always-visible row was really protecting. Status gets chips too, since it is now behind the button and is the filter most likely to empty a table. `STATUS_OPTIONS` moves beside the filter type, because the drawer and the chips both need to turn a status into a label and a second copy is a second place for a new status to be forgotten. The admin page object opens the flyout, acts, and closes it again — closing matters, because the drawer overlays the table every assertion in those specs is about. Closes #169 |
||
|
bermudalamb
|
856d8c4511 |
fix(filters): give the category tree selectable values, and drive both controls by search (#139)
Three things the e2e run found.
`toTreeData` still emitted `key`, which is how an antd `Tree` identifies a node and not how a `TreeSelect` selects one. Nothing could be picked, and `treeNodeFilterProp="title"` had nothing to filter against. It now emits `value`, matching the admin's CategoryTreeSelect.
The page object typed the name before clicking it, in both controls. Not for realism: the option lists are virtualized, so against a database holding hundreds of categories the wanted row never renders until a search narrows to it, and scrolling to it would be testing the virtual list rather than the filter.
Tag options are matched by class rather than by role, for the reason AdminInventory.toggleStatus already records — antd renders an invisible role="listbox" shim beside the real list, so getByRole('option') resolves to something zero-sized that can never be clicked. The drawer's own title is a plain element rather than a heading, so the click that closes an option list lands on the Favorites heading instead.
The multi-category URL assertion accepts the comma either percent-encoded or literal. URLSearchParams encodes it, which is how the `tags` parameter has always looked, and both spellings parse.
|
||
|
bermudalamb
|
faf38be91a |
feat(filters): make the storefront filter panel searchable and multi-select (#139)
The filter drawer did not scale with the taxonomy behind it. Categories were a bare antd `Tree` rendered at whatever depth it had grown to, with no search and single selection, and tags were a wall of every tag in the system. Neither said what was selected except through highlighting and chip colour. Both are now searchable multi-selects. Categories keep their hierarchy in a `TreeSelect`, matching the admin's `CategoryTreeSelect` so the two screens behave alike; tags become a multiple `Select` whose selected pills keep their colours, which is the only place a tag's colour was load-bearing. Several categories combine as OR. A customer picking Furniture and Decor wants both, not the empty intersection, and each selected id still expands to its descendants, so the answer is the union of the subtrees. That is deliberately the opposite of the tag rule, which stays AND, and both headings now state their rule rather than leaving it to be discovered. `ItemFilters.categoryId` becomes `categoryIds` end to end. The recursive CTE is seeded with `= ANY($n::int[])` rather than one id, which walks every selected root in one recursion and gives the OR for free; matching on `IN` keeps it a set test, so an item under two selected branches still appears once. The query parameter keeps its singular name and becomes comma-separated, the shape `tags` and `status` already use, so every `?category=1` link written before this still parses as a list of one. A list containing anything unreadable is still a 400, per decision 9 — honouring the readable half would answer a narrower question than the one asked and look indistinguishable from a filter that worked. The admin's inventory filter stays single-select, since it asks what is in a category rather than in any of several, but reads and writes a list of at most one so there is one shared filter type rather than two that drift. Closes #139 |
||
|
bermudalamb
|
7edc08e6eb |
feat(admin): make the placeholder chips insert at the cursor (#143)
The chips above each email editor named the placeholders and left an admin to retype `{{holdDuration}}` by hand, getting the braces and the spelling right unaided. A typo did not announce itself either: a misspelled placeholder is not a required one, so the save succeeded and the email shipped with a literal `{{holdDuraton}}` in it.
Clicking one now inserts its tag at the caret in whichever field was last focused, replacing any selection.
Four things this needed that a click handler alone would not have given.
The field has to be remembered rather than read. Clicking a chip blurs whichever of the subject or body had focus, so `lastFocused` is tracked on focus instead. It starts on the body, because that is where placeholders almost always go and because a chip clicked on arrival should do something predictable rather than nothing.
The insert goes through setState, not the DOM. Writing into the element's `value` would appear to work and would not: both fields are controlled, so the next keystroke re-renders from state and the insert vanishes.
The caret has to be put back. A controlled re-render leaves it at the end, so the new position is stashed in a ref and applied in an effect once the value has landed — just past what was inserted, with focus retained, so typing carries on from there.
And the chips had to become buttons. An antd Tag renders a span, so a keyboard user could neither reach one nor activate it. The button carries the semantics and the Tag the appearance, which makes enter and space work with no key handling of our own.
The textarea is found by querying the wrapper rather than through MDEditor's ref, which exposes an internal store that is not part of its API. The editor renders exactly one.
Five end-to-end tests, checked against a naive implementation rather than only against the finished one: reverted to append-and-forget, four of the five fail. The one that still passes is the plain insert-into-empty case, which appending also satisfies — worth knowing, since on its own it would have proved nothing.
Two mistakes worth recording, because both were mine and both were caught by running things rather than reading them. The spec first drove the passwordReset template, which email-templates.spec.ts already owns; stored templates are global per database, so the two files raced across Playwright's workers. Moved to the verification template — different key, no race. And its last assertion claimed the body did not contain `{{greeting}}`, which is false for that template before any click, since its default body already has one. Comparing the body against its own earlier value is what was actually meant.
Verified: tsc clean over src and tests, lint unchanged, build clean, and the three email specs pass 18/18 together.
Closes #143
|
||
|
bermudalamb
|
f32913ef51 |
refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)
Indexing an array now yields `T | undefined`, which is what it always did — the compiler simply says so. Enabled in backend/tsconfig.json and frontend/tsconfig.json, and in tsconfig.sonar.json alongside it so the drift guard stays satisfied. The sequencing this issue insisted on was right, and the numbers show why. Before #159 typed the query results, `rows[0]` was `any` and `any` indexes to `any`: the flag would have found close to nothing and the count would have changed completely afterwards. With the rows typed it finds 57 — 50 in the backend, 4 in the frontend, 3 in the Playwright suite — and they fall into three kinds. Thirty are `rows[0]` after a `rows.length` guard. TypeScript cannot connect the two, and rewriting them as `const [row] = rows; if (!row) …` makes the guard and the use the same check, which is better code independently of the flag. Ten are rows a statement guarantees — `INSERT … RETURNING`, or a lookup for an id the session middleware has already matched. These get `requireRow(rows, what)`, a new helper in db.ts that throws naming the query. A thrown error rather than a non-null assertion: if the assumption is ever wrong, an assertion hands `undefined` to the next line and fails somewhere unrelated, whereas this fails at the query and says which. asyncRoute turns it into a 500, which is the right answer for "the database did not do what the statement says it does". It also states the assumption once instead of ten times. The rest is ordinary indexing the compiler cannot prove: a regex capture group that the pattern guarantees, `split('+')[0]`, a modulo kept in range, `hasOwnProperty` failing to narrow an index signature, and Express typing route params as an index signature so `req.params.itemId` is `string | undefined` on a route that cannot match without it. One correction to this issue's premise, which matters for what it was expected to find. The body says "in a handful it does not guard at all", and the unguarded-500 risk it describes was not found. Every `rows[0]` either sits behind a length check or behind a statement that guarantees a row. What the flag actually bought was the ten places where that guarantee was real but unstated, and those now say so. Two changes worth calling out because they are not mechanical. TAG_COLORS is typed `[string, ...string[]]` in both copies rather than `as const` — the first attempt used `as const`, which narrowed the elements to literals and broke adminTags, so the annotation keeps `string` while telling the compiler index 0 exists. And the filter drawer's slider falls back to the bounds it was given rather than to null, because null there reads as "no filter" and would widen the results rather than leave them unchanged. Test files needed changes too, since ts-jest compiles them against the same config: a regex destructure in the compose guard, and ten `mock.calls[0][0]` reads where the surrounding assertions already establish the call happened. Verified: tsc clean over backend, frontend src and the Playwright suite; unit 254/254; integration 238/238; frontend build clean; lint unchanged in both workspaces. Closes #101 |