f0bdc590c977366b4bf398ffcba0ba2083f1fc1a
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
f0bdc590c9 |
Merge pull request 'fix(scripts): switch Node to a pinned version rather than asking nvm for latest (#198)' (#201) from feature/198-nvm-node-version into main
Reviewed-on: #201 |
||
 bermudalambandClaude Opus 5
|
95325c75d4 |
fix(scripts): switch Node to a pinned version rather than asking nvm for latest (#198)
`start-local.ps1` failed on a machine that had everything it needed, and then blamed the one thing that was definitely not the problem: "the newest version nvm has installed is too old. Install a newer one" — printed on a machine holding 26.7.0 and 24.13.1, both well past the floor. `nvm use latest` does not mean "the newest version I have installed". nvm-windows resolves `latest` against the remote release list, and `newest` is the alias for the newest installed. The docstring stated the opposite and the code was written against it. Here that resolved to 26.8.1, which is not installed, so nvm reported `activation error: Version not installed`, left v18.16.1 running, and exited 0. Two things had to change, and fixing either alone leaves it broken. The version asked for is now pinned in `NODE_VERSION` rather than chosen by alias, so two machines run the same Node instead of whatever each happens to have installed, and there is one line to bump for both entry points. The alias names are recorded in the docstring anyway, because `latest` and `newest` are easy to swap back by accident and the difference is the whole of this bug. `Use-Node` no longer treats an alias as automatically successful. That special case is why the error was wrong rather than merely unhelpful: it short-circuited on `$Version -eq 'latest'` regardless of what was running, swallowing nvm's `activation error` — which the function had already captured in `$output` for exactly this purpose — and returned success holding v18. The floor check downstream then reported the only explanation left to it. An alias switch is now verified against nvm's own report, so a failure says what nvm said. Keeping that half matters even with a pinned version, because no caller passes an alias today. The bug was someone reaching for one, and the next person reaching for one gets a truthful failure rather than a confident wrong answer. The floor check survives as a backstop against pinning `NODE_VERSION` below 20, and its message now says that rather than describing installed versions — a version that is not installed is `Use-Node`'s error to report, and it reports nvm's reason. Verified from a real v18.16.1 baseline: the pinned switch takes 18.16.1 to 26.7.0; a concrete version that is not installed throws with nvm's reason; `latest` throws instead of silently succeeding. `start-local.ps1` then runs the whole way through — Node switch, migrations, backend build, ready. Parse check clean. Shared by `start-local.ps1` and `run-tests.ps1`, so this broke both and fixes both. Closes #198 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
1b39e354b6 |
Merge pull request 'fix(cart): say it is a demo where the customer can see it (#195)' (#200) from feature/195-demo-checkout-label into main
Reviewed-on: #200 |
||
|
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> |
||
|
bermudalamb
|
ff6cbe18c5 |
Merge pull request 'docs(ops): make the cutover runbook agree with the compose file about DEMO_MODE (#196)' (#199) from feature/196-runbook-demo-mode into main
Reviewed-on: #199 |
||
|
bermudalambandClaude Opus 5
|
2fe3aa055f |
docs(ops): make the cutover runbook agree with the compose file about DEMO_MODE (#196)
#190 turned `DEMO_MODE` from a hardcoded compose value into a Portainer stack variable with no default, and updated the compose header. The runbook that actually creates the stack was not updated with it, so the document and the file it deploys have been disagreeing since — in the three places most likely to be read under pressure. Step 2's list of stack variables to record did not include `DEMO_MODE`, and step 6 says to add the variables from steps 2 and 3. An operator following this literally creates a stack that cannot boot. It is now in the list, with a paragraph of its own: it is the only one of the nine that is not a secret, which is exactly why it is the easy one to skip past. The troubleshooting section said `DEMO_MODE` was hardcoded and therefore could not be missing, so its absence from the error list proved nothing. That was the most dangerous sentence in the file — an unset `DEMO_MODE` is now the *first* thing to check rather than something to rule out. The line now says it moved and why. The crash-loop example was the PayPal triple, which cannot occur while `DEMO_MODE` is `true`. The message an operator will actually see during the demo interim — `DEMO_MODE is required and must be exactly 'true' or 'false'` — appeared nowhere in the runbook. Both forms are shown now, in the order they are likely to be hit. Added what neither document said: Compose only *warns* about an unset variable and deploys anyway. In Portainer's stack UI that warning is easy to miss, and the container then crash-loops under `restart: unless-stopped` — loud in the log, invisible in a glance at the stack list. The container log is the signal, not the deploy output. The compose header carried two claims that #190 falsified and did not correct: that the PayPal secrets are required "because DEMO_MODE is false below", and that only secrets are interpolated. Both now describe the file as it is. This is the drift `composeEnvironment.test.ts` exists to prevent, surfacing in the one place no test can reach — the guard keeps the compose file honest about its own intent and cannot see the runbook beside it. Verified: 278 backend unit tests pass, including the compose guard that parses this file, and a sweep for the stale claims finds none left. Closes #196 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
fdb5b7a0be |
Merge pull request 'Feature/192 backup directories' (#193) from feature/192-backup-directories into main
Reviewed-on: #193 |
||
|
bermudalamb
|
1a59edec18 |
docs(ops): prove the backups during the cutover rather than waiting for a schedule (#192)
Step 4b created the directories and step 7 checked the containers were Up. Neither established that a restorable file actually gets written, and those are not the same claim — the database backup does not run until 03:00 and the uploads archive not until Sunday 04:00, so a stack that looks correct at the end of a cutover can be four days from its first evidence. Both tools take a manual trigger, so the wait is unnecessary. The runbook now forces one run of each, checks the sizes are plausible, and greps the dump for `COPY` lines on the real tables — a dump of an empty database succeeds and looks fine, which is the one way this check could otherwise lie. It also confirms the healthchecks agree with where the files landed. A check whose `find` path disagrees with where the tool actually writes reports unhealthy forever while the backups are working perfectly, and that is a thing to discover on the day the stack is built rather than a year later. `starting` corrected to `unhealthy` in the surrounding text: during `start_period` Docker reports `starting`, which is what an operator actually sees and what the previous wording got wrong. Proven against production during the cutover on 2026-08-26 — a 41K dump and a 15M archive, both landing where the healthchecks look. |
||
|
bermudalamb
|
724e9ce19d |
docs(ops): say that the backup directories have to be created (#192)
The stack gained two backup services in #147 and nothing has ever told anyone to create the directories they mount. `backup-and-restore.md` reads from both paths and the compose file mounts both, but no document creates them — while the README does exactly that for QA's data directories, ownership notes and all. Production's cutover runbook said nothing. Hit for real during the cutover: both backup containers sat in Created, never started, and `docker logs` on them reported only that nothing matched the filter, because a container that never ran has no output. Portainer showed them beside the healthy ones and the stack looked deployed. That silence is the reason this is worth a step of its own rather than a footnote. A backup regime that never started is indistinguishable from a working one until someone needs a restore, which is the failure mode the healthchecks in #147 exist to catch — and those healthchecks cannot fire on a container that is not running. The cutover runbook gains the directory creation before the stack is created, and step 7 now counts containers rather than only checking the app: four, all Up, with Created called out as the thing to look for. Counted from the compose file rather than from memory — the first draft said five. `backup-and-restore.md` gains the same note where it describes the destinations, since anyone reading that page is already thinking about paths. No `chown`, deliberately stated: both backup images run as root, unlike the Postgres image whose data directory needs uid 999, and an unnecessary chown instruction is how people learn to run them without thinking. |
||
|
bermudalamb
|
085684c1d2 |
Merge pull request 'Feature/191 production demo mode interim' (#192) from feature/191-production-demo-mode-interim into main
Reviewed-on: #192 |
||
|
bermudalamb
|
6104ebb459 |
feat(ops): read DEMO_MODE from the stack rather than the compose file (#190)
Reverses the position the previous commit took. Hardcoding it made a value that gets flipped without a code change require a commit and a merge to flip, which is backwards — and it is the operator's call, not the file's.
No default, deliberately. `${DEMO_MODE:-false}` is the obvious form and the wrong one: a default decides whether the shop takes money on the operator's behalf, silently, whichever way it points. Having none is safe rather than fragile because `checkDemoMode` is strict — an unset stack variable substitutes to an empty string, and anything that is not exactly `true` or `false` refuses to boot naming DEMO_MODE. That strictness is the whole reason interpolating this one is defensible, so the line says so.
The compose guard had to learn the difference. It hands each deploying file's entries to the real validator, and a literal `${DEMO_MODE}` is not a value `checkDemoMode` accepts, so both the DEMO_MODE assertion and the validateEnv check failed the moment the file stopped holding a literal. Each deployment now declares the stack variables it supplies, and a bare `${VAR}` named there resolves to the declared value before the file is validated. Every other bare `${VAR}` stays opaque exactly as before — those are secrets, and what is checked of them is that the line exists.
Be clear about what that guard can prove. It cannot see Portainer, so it does not verify the stack actually holds `true`; nothing in this repository can. What it does is keep the intent beside the file and make the pair inseparable — hardcode the compose line and the registry disagrees, change the registry and it no longer describes the file. The runtime half is the boot check, which fails loudly rather than falling back. Verified by mutation: hardcoding `false` fails the DEMO_MODE assertion, and deleting the line fails that and `validateEnv`.
Restoring real payments is now two Portainer values and a redeploy, with no commit — which is what #190 asks for.
|
||
|
bermudalamb
|
12b2f09d79 |
docs(ops): point the demo-mode notes at the right issue (#190)
The tracking issue was filed as #190; the compose banner and the guard test both said #191, guessing the number before it existed. A note that points at the wrong issue is worse than no note when the thing it tracks is production not taking money. |
||
|
bermudalamb
|
0a830cad1f |
feat(ops): put production in demo mode to complete the cutover (#191)
Production could not boot during the cutover to the committed compose file: `DEMO_MODE` is false there, which makes the three PayPal secrets required, and they were not available. Demo mode is the interim the compose file's own header sanctions for exactly this — the whole cart and checkout flow works and nobody is ever charged.
Two things made this cost more than it should have, and both are now written down rather than left to be rediscovered.
`DEMO_MODE` is hardcoded rather than interpolated, so setting a `DEMO_MODE` stack variable in Portainer does nothing at all — there is no `${...}` for it to substitute into and the file's value wins silently. That hardcoding is right: the one value deciding whether the shop takes money should not be flippable from a web UI without a commit anybody can read. But the failure mode reads as "I set it and it ignored me", so the line now says so.
Declaring `PAYPAL_CLIENT_ID=` with an empty value is identical to not declaring it. `isPresent` rejects a blank string deliberately, because set-to-nothing is a mistake rather than a value.
The state is loud in both places that can see it. The compose file leads with a banner saying production is taking no money, and `composeEnvironment.test.ts` asserts `DEMO_MODE` is `true` — that assertion is the guard, not a formality: it fails the moment the file and the expectation disagree, in either direction, so this cannot be switched back quietly and cannot be left on unnoticed.
#191 restores it.
|
||
|
bermudalamb
|
5f21d97178 |
Merge pull request 'docs(ops): name the crash loop the cutover runbook was most likely to cause (#175)' (#191) from feature/190-cutover-runbook-secrets into main
Reviewed-on: #191 |
||
|
bermudalamb
|
0cec9b3c1f |
docs(ops): name the crash loop the cutover runbook was most likely to cause (#175)
Step 7 listed a missing `ADMIN_GATE_SECRET` — a warning the container starts through — and said nothing about the three PayPal secrets, which are a hard error that crash-loops it. That is backwards: the secrets are the likeliest thing to be missing after a stack replacement, because Portainer stack variables belong to the stack and are discarded with it, and `DEMO_MODE` is false in production so the app refuses to start without them. Hit for real following the runbook. The verify section now shows the actual log block, says what it means, and gives the way to tell a missing variable from a misnamed one: a hardcoded value cannot be missing, so its absence from the error list proves nothing, while an interpolated variable that stays quiet while others complain proves substitution works and those others are simply unset. That is the reading that turns the log into a diagnosis instead of a list. It also says the loop is harmless while the values are fetched — the container refuses before it serves anything or touches data — and where the values live if the old stack is already gone, since the webhook id in particular is readable rather than only recreatable. Step 2 now names every interpolated variable rather than describing them in general, and states the consequence of each class going missing. A general instruction to record the environment is easy to read as already done. |
||
|
bermudalamb
|
f5e3ec3e99 |
Merge pull request 'Feature/188 filter dimensions' (#189) from feature/188-filter-dimensions into main
Reviewed-on: #189 |
||
|
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
|
60b76cae82 |
docs(plan): correct the cumulative unit test counts (#188)
Task 1 leaves 4 tests and Task 2 adds 7, so the running total is 11 rather than 12, and the same off-by-one carried into Tasks 3, 4 and the completion criteria. Caught by the Task 2 implementer, which flagged the mismatch rather than inventing a test to reach the stated number. |
||
|
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
|
e695d91670 |
docs(plan): implementation plan for filter dimensions (#188)
Nine tasks, each ending in something independently testable, in an order where every task leaves both screens working. The dimensions are built and unit-tested first, the shell after them, and the two screens are wired last — so the old components stay in place until the thing replacing them is proven. The two behaviour changes the design accepted are each covered twice: a unit test on the chips that produce them, and an end-to-end assertion on what a person sees. The admin tally reading three for three statuses, and a non-default availability producing a removable chip. Task 8 carries the deletions, deliberately last. Removing `activeFilterCount` and `hasActiveFilters` turns every remaining caller into a compile error, which is the cheapest way to find them. Two defects found reviewing the plan against the code rather than against itself: the test fixture omitted `Category.item_count` and would not have compiled, and Task 9 added a page object method that nothing used — `chooseAvailability` and `filterChip` already exist. The plan also records what must not be read as a regression. Two assertions in the storefront specs fail on every branch because the unpaginated grid cannot render 1,600+ development rows inside Playwright's default timeout, which is #186 and predates this work. Refs #188 |
||
|
bermudalamb
|
20a38b66bd |
docs(design): filter dimensions, one composable component both screens extend (#188)
#169 made the filter drawer shared, which was the right first move and not the finish. Per-screen differences are booleans, the bar around the drawer was never shared at all, and the storefront's availability preset sits outside the system because the shared component cannot express "this belongs in the bar, not the drawer". The design replaces the flags with composition: a screen contributes a list of filter dimensions, each declaring where it renders, how to render it, and what chips it contributes. A screen-specific control becomes an ordinary dimension, appearing in the chip row and counting toward the tally without the shared code knowing what it is. Dimensions are plain data rather than components or context, for a concrete reason rather than a stylistic one. The drawer sets destroyOnHidden, so its sections are unmounted whenever it is closed — exactly when the chip row matters most. Anything that registers on mount would lose those chips the moment the drawer closed, which rules out the otherwise-idiomatic context-and-children approach. The tally becomes the number of chips, so the count and the chip row cannot disagree — today they are computed by two routes and agree by coincidence, which the admin already has to correct by hand. Three visible behaviours change as a result, recorded in the spec rather than left to be discovered. Adding vitest is in scope. The design's value rests on chips() being pure, and the frontend has no unit runner at all, so without one the core of it would ship covered only indirectly and expensively through Playwright. The spec also records what is deliberately untouched: the filter state, the URL serialisation, the backend, and the two e2e assertions already failing on #186 — which must not be read as regressions from this work. Refs #188 |
||
|
bermudalamb
|
2a19238109 |
Merge pull request 'feat(filters): show a tag's own colour on its active filter chip (#185)' (#187) from feature/185-tag-chip-colour into main
Reviewed-on: #187 |
||
|
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
|
3cffcf772c |
Merge pull request 'refactor: remove the duplicated blocks SonarQube found (#182)' (#184) from feature/182-remove-duplication into main
Reviewed-on: #184 |
||
|
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
|
76797ee6f6 |
Merge pull request 'fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)' (#183) from feature/180-security-hotspots into main
Reviewed-on: #183 |
||
|
bermudalamb
|
b616b9f0ab |
fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)
SonarQube reported three security hotspots, all in `routes/admin.ts`. A hotspot is not a defect — it marks code that touches something security-sensitive and needs a human decision — so the work is a recorded review, with a change only where the review finds a real gap. It found one. The gap: multer writes every file to disk before any route logic runs, and multer's own cleanup only covers errors it raised itself. Everything after that left the bytes behind with nothing referencing them. A request carrying a perfectly valid photograph and a malformed `category_id` is refused with a 400 after the write, and the file stays on the volume permanently — no database row to find it by, and no bound on how many can accumulate. The same held for a malformed `tags` field, for a database error rolling the transaction back, and for `readHead` itself throwing, which returned no message and so cleaned up nothing. That is the substance of the limits the first hotspot points at. Bounding one request to 8 MB across six files does nothing if every refused request keeps its bytes for ever, and the admin API is the one surface where that is reachable. The fix is a hook rather than a call at each `return`, registered the moment multer succeeds. A route added later inherits it instead of having to remember it, which matters because the failure being prevented is precisely someone adding a fourth early return. It listens on `close` rather than `finish` so an aborted connection is covered, and checks `writableEnded` so a response that never completed is not mistaken for a success whatever its status code reads. `verifyUploadedImages` goes back to checking only. Removing the files there as well would unlink twice and log an ENOENT for every refused upload, and the single mechanism covers the case it used to miss. The other two hotspots are safe, and now say why in the file rather than only in SonarQube's UI — following the precedent of the existing comment that names S5693 by rule number. The upload path is not caller-controlled despite arriving from a request: multer composes it from a server constant and a `randomUUID()` plus an extension looked up from the validated content type, so the caller's `originalname` never reaches the filesystem. That reasoning belongs next to the `fs.open` that depends on it. Three tests, written first and failing first: a refused sibling field, a refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass. Refs #180 |
||
|
bermudalamb
|
e3514e8ef4 |
Merge pull request 'fix(ci): stop the test summarisers failing the job on an unreadable results file (#178)' (#179) from feature/178-resilient-summarisers into main
Reviewed-on: #179 |
||
|
bermudalamb
|
5d427bc1a1 |
fix(ci): stop the test summarisers failing the job on an unreadable results file (#178)
Run 525 was the first with #174's graceful failure, and it worked: the integration suite failed, the end-to-end suite ran again, and `SonarQube Scan` succeeded for the first time since #154 started. But `Summarize integration tests` failed, and that step should not be able to. Both scripts already carried the principle in a comment — report plainly and exit 0, because the job fails on the real step and a stack trace here would only bury it — and both only implemented it for the file being absent. A file that exists and cannot be read crashed them. Two ways to reach that, both reproduced. `--forceExit`, which the integration script passes to paper over a post-run hang, can end the process around the write and leave partial JSON. And a suite that fails to *run* rather than to assert arrives without the array the failure renderer walks, which is exactly the shape this suite has been producing under #154. Reading is now guarded as thoroughly as `summarize-playwright.js` already guarded its traversal, and that traversal's `|| []` discipline is extended to the jest renderer. `summarize-playwright.js` had the same hole by the narrower path of an unguarded `JSON.parse`. The reason reaches the log rather than being swallowed. "Could not read the results file" with the parse error is diagnostic; a silent empty summary is not. This matters beyond tidiness because of where the failure lands. A crash here reports the job as failing at a step named for summarising rather than for testing, which is the misdirection #142 fixed once already — and a summariser whose job is to make a failing run readable should not crash on the output of the worst failures, which is the moment it is most needed. Verified against a truncated file, a suite entry with no `testResults`, an absent file, and a real 278-test run: the first three now exit 0 naming the reason, the absent case is unchanged, and the happy path still reports its counts. Closes #178 |
||
|
bermudalamb
|
ed81de3b06 |
Merge pull request 'docs(ops): write down how to cut production over to the committed compose file (#175)' (#177) from feature/175-prod-cutover-runbook into main
Reviewed-on: #177 |
||
|
bermudalamb
|
cf9003bc2d |
docs(ops): write down how to cut production over to the committed compose file (#175)
#118 put `docker-compose.prod.yml` in the repository and argued at length for why it belongs there. It never produced the procedure for getting production from where it is — a stack living in Portainer's web editor — to where that file expects it to be. What existed was scattered and none of it was a procedure. The compose header described the destination: name the stack this, deploy it as a git repository stack with these settings, rename any variables whose names differ. The README's deployment section is a good runbook for the routine redeploy, and assumes the stack is already in the right form. So the riskiest deployment operation this project has was the one with no steps written down. `docs/ops/production-stack-cutover.md` is those steps, in the order that matters, with the two facts that decide whether the operation is safe stated up front rather than left to be inferred from the volumes block. The first is that all persistent data is bind-mounted from the NAS filesystem rather than held in Docker-managed volumes, so deleting the stack cannot lose the database or the product images. That is what makes the cutover recoverable at all, and step 2 verifies it rather than trusting it — the `docker inspect` there discriminates `bind` from `volume` and says to stop on `volume`, because this runbook does not cover that case. The second is that both services set an explicit `container_name`, so the old containers must be gone before the new stack starts or the deploy fails on a collision that reads like a Portainer bug rather than a sequencing mistake. It also captures what is simply lost if not recorded first. Portainer stack variables belong to the stack and are discarded with it; they are all secrets, and a stack brought back up with a different `DB_PASSWORD` than the data directory was initialised with cannot authenticate against its own database. Recording them is step 2 and it is what makes the rollback credible. The verification section separates the boot warnings that are correct in production — no mail allowlist, no uploads origin yet — from the one that means a variable never arrived. That distinction is the whole failure mode #118 was about: a stack variable whose name matches nothing in the file is substituted nowhere and never reaches the container, and the failure reads as "I set it and it says it is not set". Every diagnostic command in it was run rather than written from memory. The mount inspection was checked against a container that genuinely uses a named volume, to confirm it reports the difference the step depends on. The compose header now points at the runbook for the procedure and keeps the rationale that belongs at the point of use — why there is no `build:`, why most values are hardcoded, why `NODE_ENV` is absent. The README says plainly that its section is the routine deploy and links to the other one. Closes #175 |
||
|
bermudalamb
|
050420858f |
Merge pull request 'ci: fail at the end rather than part way through, so the scan still runs (#174)' (#176) from feature/174-graceful-sonarqube-failure into main
Reviewed-on: #176 |
||
|
bermudalamb
|
e3842b1a4c |
ci: fail at the end rather than part way through, so the scan still runs (#174)
`sonarqube.yml` already had a documented design for this: run every suite, produce coverage, scan, summarise, then fail at the end from recorded step outcomes. The comments on the gate spell it out and #142 fixed it once already. The integration suite was never wired into it — no `id`, no `continue-on-error`, and absent from the gate, where the unit and end-to-end suites had all three. So it was the one suite whose failure aborted the job. On every push since #154 started, step 9 failed and steps 10 through 15 were skipped, which means there has been no SonarQube analysis at all for the duration — not a degraded one, none. The end-to-end suite has not run in CI either, which is separately why #116 cannot be verified: the step that would demonstrate its fix is skipped rather than failing. Fixing only that step would have left the same shape in four other places, so every step from the first suite to the scan is now guarded and named in the gate: starting the backend, installing browsers, merging frontend coverage, and the scan itself, which until now took the summaries down with it. The preconditions before the suites — checkout, installs, build checks, migrations — still fail hard, because when they fail there is genuinely nothing to analyse. The job still fails. It fails at the end, having produced everything it could. The integration suite also gains the summary the other two already had. It was the only suite without one, so the failure this workflow has been stuck on presented as 36 assertion errors about categories and price filters rather than as a count — #154 records how expensive that misdirection was to read. `summarize-jest.js` already takes a label, so this is reuse. `tests/unit/workflowGate.test.ts` asserts the pairing that makes the design work: a step carries `continue-on-error` so it cannot abort the job, and the gate names it so it can still fail the job. Both halves are needed and nothing connected them, which is how this happened — and the other direction is worse, since a step guarded but unnamed cannot fail the job at all. The test checks the invariant rather than a list of names, for the same reason `composeEnvironment.test.ts` reads the real list rather than a copy. Verified by mutation: dropping the integration suite from the gate, un-guarding it, and removing `always()` each fail it. Confirmed by running the new flag combination rather than assuming it: the integration suite writes `integration-results.json`, the summariser reads it and exits 0, and both that file and `coverage/integration/lcov.info` are still written when the suite fails — which is what makes scanning with a failing suite produce real coverage rather than a fabricated regression. Closes #174 |
||
|
bermudalamb
|
667a40c4eb |
Merge pull request 'feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)' (#173) from feature/103-uploads-origin into main
Reviewed-on: #173 |
||
|
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
|
5d72c1c88b |
Merge pull request 'test(perf): measure what concurrent hashing actually costs a bystander request (#163)' (#172) from feature/163-measure-hash-latency into main
Reviewed-on: #172 |
||
|
bermudalamb
|
313b48f582 |
test(perf): measure what concurrent hashing actually costs a bystander request (#163)
#163 claimed `bcryptjs` blocks the event loop and that every login stalls every other request in flight. That was asserted without measurement and is wrong: the asynchronous API chunks its work and yields between rounds, and all six call sites in `routes/customers.ts` use it — there is no `hashSync` or `compareSync` anywhere in `src`. A smaller effect is real, though, and this measures it instead of arguing about it. The probe is `GET /api/customers/me` with no cookie, chosen for doing almost nothing: it rejects before touching the database, so nearly all of its latency is time spent waiting for the event loop rather than work of its own. The load is real registrations against the real route, because the question is what a deployed server does rather than what bcrypt does on a bench. Measured on the dev machine, Node 24, cost 12: | Concurrent registrations | Each registration (p50) | Bystander p95 | Bystander worst case | | --- | --- | --- | --- | | idle | — | 0.3 ms | 5.4 ms | | 1 | 213 ms | 0.6 ms | 102 ms | | 4 | 803 ms | 101.9 ms | 405 ms | | 8 | 1626 ms | 15.6 ms | 808 ms | Both columns are linear in the number of queued hashes. Registration is roughly 200 ms times the concurrency, because the hashes serialize onto the one thread. The bystander's worst case is roughly 100 ms times the concurrency, which matches the coarseness of the chunks measured earlier — about 100 ms of un-yielding time per hash. The median stays under a millisecond throughout, so this is a tail-latency characteristic and not the stall the issue described. The benchmark creates real customers and deletes them again, because it is normally pointed at a development database that nothing truncates. Lint now covers `scripts` as well as `src`, so the one file in it is held to the same standard as the rest. |
||
|
bermudalamb
|
1ba1cac1db |
Merge pull request 'Feature/169 admin filter flyout' (#171) from feature/169-admin-filter-flyout into main
Reviewed-on: #171 |
||
|
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
|
70cc3056e7 |
Merge pull request 'feat(admin): make the placeholder chips insert at the cursor (#143)' (#168) from feature/143-clickable-placeholders into main
Reviewed-on: #168 |
||
|
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
|
77565723d6 |
Merge pull request 'refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)' (#167) from feature/101-unchecked-indexed-access into main
Reviewed-on: #167 |
||
|
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 |
||
|
bermudalamb
|
5d0b14d6ad |
Merge pull request 'refactor(backend): type the remaining query results (#159)' (#165) from feature/159-type-remaining-queries into main
Reviewed-on: #165 |
||
|
bermudalamb
|
179cbad225 |
refactor(backend): type the remaining query results (#159)
Completes the typing. Every `.query(...)` in backend/src whose rows are read now carries a row type: adminCustomers, adminCategories, shippingAddresses, adminTags, adminEmailTemplates, adminSettings, public, server and the auth middleware. Typed sites go from 49 to 78, and there are no untyped reads left anywhere. Writes and transaction control stay untyped, which is the exemption #159's criteria allow for and the reason is stated in each file: they return nothing anyone reads, and annotating them would bury the ones that matter. The aggregates needed checking rather than guessing, and the answer was not what the shapes suggest. Postgres returns COUNT as bigint and SUM as numeric, and node-postgres hands both back as strings — only an explicit ::int cast arrives as a number. Probed against the real database: COUNT(*) is a string, COUNT(*)::int is a number, SUM() is a string, MAX(timestamptz) is a Date. That makes the admin customer list a mixture. order_count and total_spent_cents are strings; reserved_count, which the query casts, is a number. They are typed as what they are. Which surfaces a mismatch worth knowing about and not fixed here. frontend/src/admin/adminCustomersApi.ts declares both as `number`, and Customers.tsx sorts with `a.order_count - b.order_count` and renders with `(v / 100).toFixed(2)`. Those work, because `-` and `/` coerce a numeric string. The first `+` written against either — a column total, say — will concatenate instead. Nothing is broken today; the types on both sides simply disagree about reality, and one of them is now right. Changing the API to cast would alter the response shape, which is a behaviour change and belongs in its own issue. Two smaller shapes worth a note. shipping_addresses.usps_standardized is jsonb that is only ever handed to the client, so it is `unknown` rather than a guessed object. And `SELECT 1 ... ` used purely for `.length` has no column name of its own — Postgres calls it `?column?` — so it is an index signature with nothing read out of it rather than a fabricated field. Verified: tsc clean, unit 254/254, integration 238/238, backend lint unchanged from main. Closes #159 |
||
|
bermudalamb
|
1f470c0c02 |
Merge pull request 'docs(ci): correct the #154 hypothesis — the database is wiped before the tests start (#154)' (#166) from feature/154-correct-hypothesis into main
Reviewed-on: #166 |
||
|
bermudalamb
|
ae20568601 |
docs(ci): correct the #154 hypothesis — the database is wiped before the tests start (#154)
dmesg came back empty, and the evidence that actually settles it was in the original log the whole time. Step 7 `Run migrations` succeeded, then step 9's globalSetup applied all six migrations again from scratch. Both point at the same database — migrate.js reads PGHOST/PGDATABASE and the job sets those and TEST_PG* to the same service and the same redefined_test — so had step 7 migrated it, globalSetup would have printed "No migrations to run!", which is what a local run prints. It found an empty database. The wipe was already happening before the tests started, which makes this a database being reset repeatedly rather than a container dying partway through a heavy run, and accounts cleanly for the empty dmesg: a restart is not a kill. The suspect moves from memory pressure to the runner's handling of `services:`, where act_runner has been uneven across releases. The diagnostics change with it: A3 is dropped because it was designed to catch starvation, A2 is demoted to a fallback, and the new first check needs nothing but the Gitea UI — compare step 7 and step 9's migration output in any failing run. The superseded hypothesis is kept rather than deleted. A future reader finding memory ruled out is better served by seeing why it was suspected and what refuted it than by a document that never mentions it. Refs #154 |
||
|
bermudalamb
|
fc3a190ec2 |
Merge pull request 'refactor(backend): type the admin item queries, and fix the stale status union (#159)' (#164) from feature/159-type-admin-queries into main
Reviewed-on: #164 |
||
|
bermudalamb
|
d43e2d5871 |
refactor(backend): type the admin item queries, and fix the stale status union (#159)
admin.ts has no untyped reads left. Typed sites go from 43 to 49. New ItemRecord in itemSelect.ts for the bare `items` row that `RETURNING *` gives back. Deliberately not AdminItemRow: that describes a select which joins the category and adds images and tags as subqueries, so typing a RETURNING * as it would promise three fields the result does not contain. Three shapes for one table, because three different queries return three different things. The typing found a real defect on its first run, which is the case for doing this at all. `ItemStatus` in types.ts was `'available' | 'reserved' | 'sold'`. The database has four values and defaults to 'pending' — items have arrived pending since #90. itemFilters.ts declared its own copy that had all four and was correct. Two declarations of one union with nothing connecting them: one went stale and nothing said so. It was invisible while query rows were `any`. Typing them turned `if (status === 'pending')` in admin.ts into TS2367, "this comparison appears to be unintentional because the types 'ItemStatus' and '\"pending\"' have no overlap" — a compiler telling us the unpublish route's guard could never be true, against a type that was simply wrong. Confirmed against the database rather than by picking the more plausible of the two declarations: `SELECT DISTINCT status FROM items` returns pending, available, reserved and sold. Fixed by removing the duplication rather than by patching both copies. types.ts now holds the only declaration and itemFilters.ts imports it, re-exporting so its existing importers are unaffected. Patching both would have left the next drift free to happen the same way. Verified: tsc clean, unit 254/254, integration 238/238, and backend lint unchanged — the four warnings it reports are identical to those on main with these changes stashed, so none of them are new. Refs #159 |
||
|
bermudalamb
|
36c9fe9227 |
Merge pull request 'refactor(backend): type the customer query results (#159)' (#162) from feature/159-type-customer-queries into main
Reviewed-on: #162 |
||
|
bermudalamb
|
c5fe84fba5 |
refactor(backend): type the customer query results (#159)
The largest file, 43 query sites, now with none of its reads untyped. Typed sites across the backend go from 22 to 43. CustomerRecord extends the existing CustomerRow rather than restating it, because that is the relationship that actually holds. CustomerRow was already there and is not a table row — it is the subset safe to return to the customer, written that way so adding a column could not silently start being echoed back by a `...c` downstream. The full row read by `SELECT *` is that subset plus seven fields that are deliberately not on it, password_hash among them. Extending keeps the two connected: adding a column to the table means adding it to CustomerRecord and deciding at that moment whether it belongs in CustomerRow, which is exactly the decision the older comment is about. The column list came from the live schema rather than from reading migrations, since the migrations are additive and reconstructing the current shape from six files invites getting a nullability wrong. Typing the data export surfaced something worth a decision, and it is recorded in the code rather than quietly changed. `GET /me/export` runs `SELECT * FROM orders` and sends every column verbatim, including raw_event — the processor's entire capture payload. That is defensible for a GDPR export, since it is the customer's own transaction, but it is a decision rather than an accident, and it is now visible in a type instead of hidden behind `any`. The order-history route two functions above deliberately selects six named columns instead, which is the contrast that makes the export's behaviour worth confirming. No behaviour changed here; #159 is about types. Nullability follows the schema rather than optimism: orders.amount_cents, status, item_id, customer_id and checkout_id are all nullable in Postgres, and customers.first_name and last_name are nullable despite registration requiring them, because customers who registered while the field was optional genuinely have none. Verified: tsc clean, and the full integration suite passes 238/238 across 17 suites. Refs #159 |
||
|
bermudalamb
|
bd30d20c20 |
Merge pull request 'refactor(backend): type the cart and checkout query results (#159)' (#161) from feature/159-type-checkout-queries into main
Reviewed-on: #161 |
||
|
bermudalamb
|
72c49719fc |
refactor(backend): type the cart and checkout query results (#159)
The transaction paths, taken before the larger files because this is where `any` is most expensive: these are the queries that lock rows, move money and mark items sold, and where a mistyped field reaches a customer as a wrong price rather than a broken page. Typed query sites go from 6 to 22. Only queries whose rows are actually read carry a type. The INSERTs, UPDATEs, DELETEs and the BEGIN/COMMIT/ROLLBACK calls return nothing anyone looks at, and annotating them would be ceremony that makes the ones that matter harder to pick out. The convention is stated once in each file rather than implied, since #159's acceptance criteria say every call is typed "or explicitly exempted with a reason" and this is that reason. Two hand-written annotations are gone as a direct consequence. `items.reduce((sum: number, it: CartItem) => …)` and `checkoutItems.map((ci: { item_id: number }) => …)` existed only because `rows` was `any` and inference had nothing to work from. With the query typed, both infer, and the second one is the more interesting of the two: it was a structural type written inline that duplicated the real row shape and could have drifted from it silently. CART_ITEM_SELECT's type records something the SQL states and no reader would otherwise know: the images aggregate selects only id and image_path, so it is `Pick<ItemImage, 'id' | 'image_path'>[]` rather than `ItemImage[]`. Typing it as the full shape would have promised a sort_order that is not in the projection. The same hand-kept caveat as the item selects applies and is written into both files: `query<T>` asserts a shape rather than checking it, because TypeScript never reads the SQL. The integration suite is what catches a select and its type disagreeing. Verified: tsc clean, and the suites covering these paths pass — cart, favorites and adminInventory 53/53, then cart and soldFilter 19/19. Refs #159 |
||
|
bermudalamb
|
d99cf28e18 |
Merge pull request 'refactor(backend): type the item query results (#159)' (#160) from feature/159-type-query-results into main
Reviewed-on: #160 |
||
|
bermudalamb
|
a75d9fe155 |
refactor(backend): type the item query results (#159)
First stage of typing the query results, and the one that sets the pattern. `pg` types `rows` as `any[]`, so every row this application reads entered a strict codebase as `any` — 1 of roughly 184 query sites carried a type before this. The row types live in itemSelect.ts, beside the selects that produce them, rather than in types.ts. They describe a projection rather than a table, and the two projections differ on purpose: ADMIN_ITEM_SELECT takes `i.*` while PUBLIC_ITEM_SELECT names its columns so the storefront never sees paypal_order_id or reserved_until. Typing both as "an items row" would quietly re-admit exactly the columns that select was written to exclude, so PublicItemRow and AdminItemRow share a base and the admin one adds the three fields it is allowed. types.ts gains ItemTag, which the tags subquery has always built and nothing had named. What this buys, demonstrated rather than claimed: introducing `rows[0].price_cent` at a read site now fails the build with "Property 'price_cent' does not exist on type 'ItemRowBase'. Did you mean 'price_cents'?". Before this it compiled, returned undefined, and reached the customer as an empty price. What it does not buy is written into itemSelect.ts rather than left for the next reader to assume. `pool.query<T>` asserts a shape; it does not check the SQL, which TypeScript never reads. Dropping a column from a select without dropping it from its type compiles cleanly and every read goes on type-checking while being undefined at runtime. The selects and their types are kept in step by hand, and the integration suite is the only thing that catches them disagreeing, because it runs the real queries against a real schema. The acceptance criteria on #159 originally claimed the compiler would catch that; it will not, and the issue has been corrected. Verified: tsc clean, and the four integration suites that exercise these selects pass 87/87. Refs #159 |
||
|
bermudalamb
|
9a953b060f |
Merge pull request 'refactor(frontend): triage the setState-in-effect sites (#99)' (#158) from feature/99-setstate-triage into main
Reviewed-on: #158 |
||
|
bermudalamb
|
45f1c77160 |
refactor(frontend): triage the setState-in-effect sites (#99)
Eleven warnings that looked alike and were not. This is a decision per site rather than eleven fixes, which is what the issue asked for — some of these would be made worse by "fixing" them. One was a real defect. VerifyEmail routed a fact through an effect that was already knowable during render: whether the link carries a token comes from the URL. The component therefore rendered once as a spinner in a state that was never true — a link with no token was never "verifying". Both pieces of state now derive their initial value from the token, and the effect's missing-token branch becomes an early return, so the failure is what the first render shows. Two are a defensible reset. CartProvider and FavoritesProvider clear their collection when the customer becomes null, which is synchronisation with the session rather than derived state. Deriving instead would push "signed out" onto every consumer of those contexts, and remounting on a `key` is more indirection than the problem deserves. Decided and written down rather than left for the next reader to re-investigate. Eight are legitimate and flagged conservatively. Six are a pending flag before a fetch — the rule cannot tell a spinner from a value that was already known. Cart's lapsed-item refetch is synchronisation with a server-side release the client cannot observe. useNow subscribes to the clock, which is the case the rule's own documentation names as correct. Each of the ten that stay carries the reason and a targeted disable, so lint drops from thirteen warnings to two — and the two left are the unrelated no-alphabetical-sort pair. Suppressing per site rather than switching the rule off keeps it live for new code, which is where the next VerifyEmail would be caught. The trap #60 recorded caught this, in a variant it does not describe. Placing the disable above `useEffect(` works only for a single-line effect: where the effect spans several lines the flagged line is the setState inside the body, so the directive covered nothing and produced both an unused-disable warning and the original one. Three sites were wrong that way on the first attempt. Confirmed fixed by the absence of "Unused eslint-disable directive" from the output — a disable that covers nothing reports itself, which is what makes this checkable rather than assumed. Verified: tsc clean over src and tests, and the specs covering the changed behaviour pass — verify-email, auth, favorites, orders, cart-countdown, resend-verification, 32 of 33 with the one failure passing 10/10 in a serial re-run. Closes #99 |
||
|
bermudalamb
|
88dc627a58 |
Merge pull request 'refactor(frontend): declare props read-only, and drop the deprecated antd prop (#100)' (#157) from feature/100-readonly-props into main
Reviewed-on: #157 |
||
|
bermudalamb
|
8de261538b |
refactor(frontend): declare props read-only, and drop the deprecated antd prop (#100)
Seventeen components declared props the compiler was free to assume were mutable, and one antd prop had gone stale. Both mechanical, neither with any behaviour attached.
React never writes to props, and `Readonly<>` says so to the compiler rather than only to the reader. This finishes a pattern the codebase had already chosen rather than introducing one: AccountDetails and EmailTemplateEditor were already written as `type Props = Readonly<{…}>`, so the thirteen named prop interfaces are converted to that same shape and the four context providers, which annotate `{ children }` inline, get `Readonly<{ children: React.ReactNode }>`.
Cart.tsx was the last place passing `destroyOnClose`, deprecated in antd 5.20. Twelve other call sites across the admin screens, the filter drawer and four customer modals already use `destroyOnHidden`, so this one was simply stale. Deprecated props keep working until they do not, and the failure then arrives as an antd upgrade breaking something unrelated to the change being made.
Counted rather than assumed, which the issue specifically asks for, because a `Readonly<>` in the wrong position type-checks and fixes nothing: lint goes from 31 warnings to 13, a drop of exactly eighteen, and both rules disappear from the breakdown entirely rather than merely thinning out.
What that leaves is the point of doing it. The remaining thirteen are eleven `set-state-in-effect` and two `no-alphabetical-sort` — so the frontend's warnings are now only the ones that need a decision, which is what makes #99 tractable. It had grown from the eight in that issue's title to eleven, two of them added by #97's clock tick and lapsed-cart refetch.
No behaviour change intended, so the bar was the end-to-end suite. Full run: 121 passed, 8 failed; all eight pass in a 45/45 serial re-run, which is the shared-database and event-loop flakiness this suite has had throughout.
Closes #100
|
||
|
bermudalamb
|
e926ad23b9 |
Merge pull request 'feat(ops): schedule database and uploads backups, and document the restore (#147)' (#156) from feature/147-scheduled-backups into main
Reviewed-on: #156 |
||
|
bermudalamb
|
ebefcbb76b |
feat(ops): schedule database and uploads backups, and document the restore (#147)
The only copy of every customer, order and one-of-a-kind item was the live Postgres data directory, plus whatever the deploy checklist's manual pg_dump happened to have caught. That dump is good and stays, but it only runs when someone deploys: a quiet week meant the newest copy of real customer data was a week old, and nothing bounded the gap. Two services rather than one, because they are different jobs. The database is small, changes constantly and wants a logical dump — daily, gzipped, 7/4/6 daily-weekly-monthly retention. Uploads are large and append-mostly and want an archive — weekly, 56 days, mounted read-only so a backup process cannot damage the thing it is backing up. Forcing both through one tool would serve one of them badly. The dumper is pinned to postgres-backup-local:16 to match the server. pg_dump refuses to dump a server newer than itself, so a floating tag is a backup that stops working the day Postgres is upgraded — silently, because nothing reads a dump until it is needed. It depends_on the database's existing pg_isready healthcheck, which is the constraint that shaped this: a dumper is a client, and without that the first run after a NAS reboot races Postgres coming up. Both carry a staleness healthcheck rather than trusting the schedule. A regime that stopped a month ago is indistinguishable from a working one until a restore is attempted, and `find -mmin` is the cheapest thing that tells them apart. It surfaces in Portainer beside the app rather than somewhere separate to remember to look. Windows are the interval plus grace — 26 hours daily, 9 days weekly — so a late run is not a failure, and start_period covers the first cycle when nothing has been written yet. Verified rather than assumed. Both images were pulled and checked to have a shell and `find`, since a CMD-SHELL healthcheck against an image without one reports unhealthy forever. postgres-backup-local:16 ships pg_dump 16.10 against the postgres:16 server. The healthcheck expression was exercised three ways in the image itself — empty directory, fresh artifact, and one aged three days — and returns unhealthy, healthy, unhealthy. The compose file parses and the #118 drift guard still passes over it. Three things these deliberately do not cover, written into the compose file and the doc rather than left to be discovered: They run while the stack runs, so they cannot protect the stack's own teardown. Deleting the Portainer stack deletes them too. That is why the deploy checklist's manual dump stays, and README now says so where the checklist is. They write to the same volume as the data they protect. That survives a bad migration, a dropped table, a bad deploy and a stack deletion, and not the disk. Getting a copy off /volume1 is a Synology-side job and is what turns this from a convenience into a guarantee. Daily database against weekly uploads leaves a window where a restore pairs the two from different moments. An orphaned image is harmless; a row without its image is a broken thumbnail on one recent item, usually still on the admin's machine. Neither is data loss, and a synchronised snapshot is not worth the complexity to avoid it. The restore procedure leads with practising on a throwaway database, because an untested backup is a file of unknown validity and a truncated dump looks exactly like a good one until it matters. It checks row counts and the pgmigrations head — the migration check being the one most easily skipped and most likely to bite, since a dump older than the code restores a schema the app will fail against. Both open items are listed as unticked in the doc: no off-volume copy exists yet, and no restore has been performed. Until the second is done this documents an untested procedure, and it says so. Refs #147 |
||
|
bermudalamb
|
949734d1e1 |
docs(ci): add the working document for the #154 schema-loss investigation (#154)
The diagnosis so far, what has been ruled out, and the three read-only checks that would confirm or refute it — with slots to paste the output into and a note against each saying what the answer means. Written as a working document rather than a summary because the decisive check has a timing constraint that is easy to miss: the Postgres service container is deleted when the job finishes, so watching it has to happen during the run. Discovering that after the fact costs another full run. It also records the negative result deliberately. If the container is healthy throughout, the hypothesis is wrong and the document says which suspect is next, rather than leaving an abandoned theory for the next reader to re-derive. Refs #154 |
||
|
bermudalamb
|
c55b2b3a25 |
Merge pull request 'ci: let the summarisers summarise and the gate do the failing (#142)' (#155) from feature/142-ci-failure-reporting into main
Reviewed-on: #155 |
||
|
bermudalamb
|
491c2652f3 |
ci: let the summarisers summarise and the gate do the failing (#142)
A failing end-to-end run reported itself like this:
Run node scripts/summarize-playwright.js frontend/playwright-results.json
❌ Failure - Main Summarize end-to-end tests
exitcode '1': failure
which reads as a broken summary script. It was not — it was the summariser correctly reporting that tests had failed, with nothing said about it.
Two things combined to produce that.
The summariser doubled as the gate. It ended with `process.exit(stats.unexpected > 0 ? 1 : 0)`, so it failed the job itself. The workflow already has a step written for exactly that — `Fail if either suite failed`, whose comment explains it exists so `continue-on-error` on the suites cannot turn a failing suite into a passing job. That step was being skipped while its condition was true, because a step whose `if:` omits `always()` still implicitly requires its predecessors to have succeeded, and the summariser had already failed the job one step earlier. The gate written to be the place the job fails was dead code.
And the explanation went where the log is not. `report()` writes to GITEA_STEP_SUMMARY when it is set, which under Actions is always, so the counts and the list of failing tests landed in the Summary tab while the log showed a bare exit with no output at all.
So both scripts now exit 0 whatever they find, both print one line of counts to stdout as well as the markdown to the summary, and the gate carries `always() &&` so it actually runs. A failing suite now fails at a step named for what failed, and the log says how many.
Verified by executing both scripts against crafted results rather than by reading them: failing counts, passing counts, and a missing results file, each with and without GITEA_STEP_SUMMARY set. All four exit 0, the headline appears on stdout in every case, and the step summary still receives the full table and the failure list.
Not covered: nothing in this repository runs the scripts under `scripts/`. Jest's testMatch is scoped to backend/tests/unit, so a permanent regression test would need a runner these files do not have. Worth its own issue rather than widening the backend suite's roots to reach the repository root.
Closes #142
|
||
|
bermudalamb
|
d4e1b3ac51 |
Merge pull request 'refactor(storefront): extract the catalogue machine from App.tsx as useCatalogue (#98)' (#153) from feature/98-use-catalogue into main
Reviewed-on: #153 |
||
|
bermudalamb
|
461ab01d4c |
refactor(storefront): extract the catalogue machine from App.tsx as useCatalogue (#98)
App.tsx was 361 lines, and roughly sixty of them were one cohesive concern with nothing to do with laying out a page: fetching the catalogue for the current filters, debouncing it, and negotiating with the session before it could ask. Six pieces of state, four callbacks and two effects, sharing scope with the header, footer, filter drawer and auth modal that make up the rest of the file. This is the same shape #81 dealt with once. That change took out the rendering half by extracting Catalogue and brought the file under the cognitive-complexity limit. The state half stayed. App keeps the URL as the source of filter truth, because that genuinely belongs to the page: a reload, a shared link and the back button all have to restore the same view. What moves is the request, the debounce, and the auth negotiation — and that last one is the reason this is worth doing. The rule the comments explain at length, that firing before the session resolves would 401 and show an outage banner to someone who is in fact signed in, now has somewhere to live rather than being a pair of derived booleans in a page component. The hook decides when the favorites filter needs a session; the page decides what to do about it, through an onAuthRequired callback, because the prompt is a modal the page owns. The serialise-then-reparse of the filters is kept, with the reason written down rather than left to be rediscovered. Depending on a string is what keeps `load` referentially stable while the filters are value-equal, and `load` is what the debounce effect depends on — depending on the object would give a new `load` every render, restart the debounce each time, and fire a request per keystroke. The alternatives considered were a ref written during render and threading the key through the page, and both are worse than one honest comment. filterKey is returned rather than recomputed by the caller: the page needs exactly that value to reset the catalogue's error boundary, so a crashed grid gets another chance when the filters change. App.tsx is 300 lines. No behaviour change is intended, so the bar is the end-to-end suite unchanged — the storefront listing, the filters, the favorites-requires-sign-in prompt, the failure banner and the boundary reset all pass. Also removes what the extraction left dead in App.tsx: the useEffect import, the debounce constant, and `authLoading`, which existed only to decide whether to fire the request. Refs #98 |
||
|
bermudalamb
|
c779b5a180 |
Merge pull request 'fix(cart): make the reservation countdown tick, and warn against the real hold (#97)' (#152) from feature/97-cart-countdown into main
Reviewed-on: #152 |
||
|
bermudalamb
|
5ef97bef21 |
fix(cart): make the reservation countdown tick, and warn against the real hold (#97)
The cart showed "2h 15m left" and turned it red in the final hour. Neither updated. Both readings happened during render from the wall clock, and nothing scheduled a re-render — no setInterval anywhere in the file — so the number a customer read was whatever it was when the page loaded, and the warning colour could only appear by accident, because the component had already rendered before the final stretch began. That matters more here than in most shops: every item is one of a kind, so a lapsed reservation is not "buy it later", it is someone else buying the only one. New useNow hook returns the time as state rather than merely forcing a re-render, and that is the point. A component reading Date.now() while rendering produces output that depends on the clock, which React is entitled to assume it does not — react-hooks/purity says so, and this was the only instance in the codebase precisely because it was the only place doing it. Reading `now` from state makes render a function of its inputs again, so the rule is satisfied rather than suppressed. It ticks every 30s, which matches the display's one-minute resolution, and only while the cart holds something, so an empty cart is not waking React forever. A second defect the issue did not mention. The red warning was hardcoded to the final hour, but the hold became admin-configurable in #136 and accepts values as low as half an hour — so on any setting below an hour every item was red from the moment it was reserved, and a warning that is always on is not a warning. It now keys off the last tenth of the item's own added_at-to-expires_at span. Reading it from the item rather than from the setting also means an admin changing the value does not retroactively relabel a reservation granted under the old one. At zero the row keeps saying "expiring…" and the page refetches on each tick while anything is lapsed, so it clears within one interval of the server's sweep actually releasing it. The client cannot know when that lands — the sweep runs every few minutes — so the wording claims imminence, which is true, rather than completion, which is not ours to say. The header badge is refreshed alongside, since it counts held items and goes stale the same way. Verified against the unfixed component, not just the fixed one: two of the three new tests fail on the old code. The third documents the "expiring…" wording rather than the fix, and passes either way — worth having, but it is not evidence. The tests use Playwright's clock control rather than waiting in real time, which also makes them deterministic: without it, "the text changed" would depend on where in the minute the run happened to start. Refs #97 |
||
|
bermudalamb
|
3b3bd06bbe |
test(e2e): convert the remaining admin specs, completing the POM refactor (#137)
The last nine: admin-taxonomy, admin-save-failures, admin-theme, admin-inline-category, admin-item-preview, admin-disable-customer, admin-reserved-items, admin-inventory-filters, email-templates and admin-email-settings. Every spec in tests/e2e now goes through page objects. Measured rather than asserted: - raw CSS and antd-internal locators in spec files: 0 (was 13) - local `register` helpers: 0 (was 9) - hardcoded http://localhost:5173: 0 (was 3) The antd knowledge that was spread across seven spec files is now in four page objects, each with the reason written next to it. Three of those are things no reader could have guessed from the locator: Segmented hides its real radio behind a styled label, so the input is found by role and cannot be clicked — the title attribute is the handle. The status multi-select renders an invisible role="listbox" shim beside the real list, so getByRole('option') finds something zero-sized; and a selected status renders again as a tag carrying the same title, so an unscoped getByTitle is ambiguous. Matching the visible option class avoids both. The dropdown is also opened only when closed, because antd keeps it open after a selection in multiple mode. Select popups render into a portal at the end of <body>, outside the tab panel they belong to, so a dropdown cannot be found by scoping to the panel. AdminPage.tab gained an `exact` flag for one specific collision: the Emails tab contains a rail that also renders tabs, and "Email verification" contains "Email". Without exact matching, opening the Emails tab is ambiguous with the template inside it. Two helpers stayed local rather than moving into page objects, because they belong to their file's subject rather than to a surface: favorites-filter's `favorite()`, which settles the opt-in modal and waits for its fade before the wrapper stops intercepting pointer events, and admin-theme's `luminance()`, which is a WCAG calculation and not a locator. Both now take page objects as parameters instead of reaching for locators themselves. Verified: 26/26 spec files converted, tsc clean over the whole tree, lint at the 30-warning src baseline with nothing added. Full suite 123 passed / 5 failed; all five pass in a 33/33 serial re-run, which is the load-related flakiness this suite has had throughout and not a change here — the backend hashes passwords with bcryptjs, a pure-JS implementation that blocks the event loop for every request while it runs. Closes #137 |
||
|
bermudalamb
|
549a08038e |
test(e2e): convert the favorites, availability and orders specs (#137)
favorites, favorites-filter, sold-filter, orders and pending-publish. Five more copies of "register a customer" and three more of the hardcoded base URL go with them. Two locators that were hiding real knowledge are now named. `gridCell` is the item's whole antd column rather than its card, needed because the SOLD ribbon renders outside the card — three specs reached for `.ant-col` directly to get at it. And `chooseAvailability` goes through the title attribute because antd's Segmented hides the real radio behind a styled label, so the input is found by role and cannot be clicked; that fact was written out twice in comments and is now written once in code. The favorite control is located page-wide rather than within a card. The storefront paginates as items accumulate and the control is named for its item anyway, so scoping to a card bought nothing and broke whenever the card was on another page. favorites-filter keeps its local `favorite()` helper. It is genuinely local — decline the opt-in, wait for the fading modal to stop intercepting pointer events, confirm the heart flipped — and belongs to that file's subject rather than to the storefront. It now takes page objects as parameters instead of reaching for locators itself, which is what a spec-level helper should look like. Two specs still drive the registration form rather than taking the `customer` fixture, and deliberately. Both are about a signed-out visitor being interrupted mid-action — favoriting an item, or switching on the favorites filter — and the claim is that the thing they asked for survives the interruption. Replacing the interruption with an API call would delete the test. Verified: favorites 6/6, favorites-filter 7/7, sold-filter 6/6, orders and pending-publish 8/8. Notably favorites-filter's "keeps showing a favorite after it sells" passes, which had been failing on a strict-mode violation from two items sharing a name across runs. Refs #137 |
||
|
bermudalamb
|
507c56bbb8 |
Merge pull request 'Feature/137 convert account specs' (#151) from feature/137-convert-account-specs into main
Reviewed-on: #151 |
||
|
bermudalamb
|
9b3a03d1bf |
test(e2e): convert the storefront and filter specs onto page objects (#137)
storefront, storefront-errors, theme, error-boundary and filters. filters.spec.ts carried the last hand-rolled copies of createCategory, createTag and createItem, and the hardcoded `http://localhost:5173` that meant changing the port in the config would have moved every test except this one. Seeding now goes through support/api, and the host it needs lives in one constant. It has to be a constant rather than the config's baseURL: beforeAll runs with worker-scoped fixtures only and cannot read a test-scoped option, which is why the URL was inlined there in the first place. New FilterDrawer object. The two rules it encodes are the ones the tests exist to pin down and neither is guessable from a locator: categories are a tree because the filter matches a node and everything filed beneath it, and tags combine with AND rather than OR, so selecting two means "must have both". The active-filter chips go on StorefrontPage rather than the drawer, because that is where they render — and the scoping matters, since the drawer carries a "Clear all" of its own that an unscoped locator also matches. StorefrontPage gains the three things the catalogue says instead of listing items. They are named together deliberately: the distinction between "No items yet" and "Couldn't load items" is the point, and several tests assert one is showing while the other is not, because telling a customer the shop is empty when the server is broken hides the outage. The theme switch and the attribute it writes are both on Header now. The switch is in the header and `data-theme` lands on <body>, so a spec previously had to know about `body` to observe the control it had just clicked. Verified: 12/12 across the four small specs, 8/8 on filters, tsc clean, lint unchanged at the 30-warning src baseline. Refs #137 |
||
|
bermudalamb
|
4b0c01068f |
test(e2e): convert the account specs onto page objects (#137)
account-modal and account-details, taking two more copies of "register a customer" and two more of the `accountModal(page)` helper that every account-touching spec had rewritten.
Both files had grown their own vocabulary for the same dialog. account-details reached for `modal.getByLabel('Current password', { exact: true })` and its five siblings inline in each test, so a change to the account form meant editing six places in one file and more in the next. Those are named locators now, and the three multi-step operations — saving a name, changing a password, changing an email — are actions, because each is a disclosure to open and three fields to fill before the button does anything.
AccountModal.open() gains the 20s timeout the header already had, and for the same reason. Arriving at /account means booting the app and resolving the session against the server. The old specs never noticed because they registered through the form first, which loaded the app and confirmed the session before navigating; taking the API-registered `customer` fixture arrives cold, and the 5s default is comfortably beaten on an idle machine and missed on a loaded one.
Verified: 18/18 across three serial repeats, and 12/13 in parallel. The one failure is `changes the email address and marks it unverified again`, which fails identically on the unconverted file and passes whenever the suite is not saturated — the same load-related flakiness as the rest of the family, not something this commit introduced.
Refs #137
|
||
|
bermudalamb
|
ed679986de |
Merge pull request 'test(e2e): convert the auth specs onto page objects (#137)' (#150) from feature/137-convert-auth-specs into main
Reviewed-on: #150 |
||
|
bermudalamb
|
abcc684447 |
test(e2e): convert the auth specs onto page objects (#137)
First conversion batch: auth, password-reset, resend-verification. verify-email is left alone — it already used semantic locators and duplicated nothing, and rewriting it to prove a point would be churn. Four of the nine copies of "register a customer" go here. auth.spec.ts and password-reset.spec.ts each carried their own `register`, and resend-verification.spec.ts its own `registerCustomer`, all three re-explaining the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning in slightly different words. Two also carried their own `logout`, and two their own `uniqueEmail` with different prefixes. Most of those tests were not about registering. They needed an account to exist so they could test logging out, resetting a password, or resending a verification email, and they paid for a bcrypt round-trip through the form to get one. Those now take the `customer` fixture, which registers through the API. The three tests that genuinely are about the registration form still drive it, because the thing under test has to be the thing exercised. The batch drops from 47s to 26s as a side effect, which is the cost of that round-trip made visible. password-reset.spec.ts loses its inline pg.Client. The reasoning for reading the database directly is unchanged and still right — an endpoint returning a reset token for an arbitrary address is account takeover if it is ever reachable — but it now lives in support/db.ts where it cannot be copied into the next spec wanting a shortcut. It also stops defaulting to port 55432, which is the integration suite's disposable Postgres rather than the database the app under test is connected to, and is Hyper-V-reserved on at least one machine here. Both tests in that file previously failed with a bare ECONNREFUSED unless TEST_PGPORT was set by hand; they now pass with no environment at all. New PasswordResetPages object covers both halves of recovery — requesting a link, and using one — because they are one flow and a test usually crosses between them. One lint decision worth recording. Requesting a Playwright fixture IS using it: destructuring `customer` is what makes the account exist, whether or not the body then reads the address. The linter cannot see that side effect and reports every such fixture as an unused variable. The first attempt at appeasing it was a `void customer;` line per test, which is noise standing in for a comment — and sonarjs flags that too, so it traded one warning for another. `no-unused-vars` is now configured with `args: 'none'` for tests only, with the reason written next to it. Variables are still checked; only parameters are exempt. Verified: 26/26 in the converted batch, and 127 passed in the full suite with four failures — three in the known #116 flaky family, and resend-verification's rate-limit test, which passes 9/9 across three repeats in isolation and is timing-sensitive under parallel load rather than changed by this commit. Refs #137 |
||
|
bermudalamb
|
882f42447b |
Merge pull request 'test(e2e): add page objects, fixtures and a typed test build (#137)' (#149) from feature/137-playwright-page-objects into main
Reviewed-on: #149 |
||
|
bermudalamb
|
5c907fcf9a |
test(e2e): add page objects, fixtures and a typed test build (#137)
Foundation only. No spec is converted in this commit, so the suite behaves exactly as before — the conversions follow in themed batches, each leaving the suite green. The suite had grown by copy-paste. Registering a customer was implemented nine times, as `register` in five files and `registerCustomer` in four more, each carrying its own re-explanation of the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning. `uniqueEmail` was reinvented per file with a different prefix and a different encoding each time. Thirteen locators reached into antd's internals — `.ant-tabs-tab-active .ant-tabs-tab-btn`, `.ant-select-item-option[title=...]`, `.ant-col` — spread across seven files, so an antd upgrade breaks tests that have nothing to do with it. Page objects hold named locators and the actions that operate on them; assertions stay in the specs, so a test reads as its own statement of what it verifies. The exception is an action waiting for its own completion — registering waits for the account button, opening an admin tab waits for its panel — because that wait is the action's contract, and pushing it to callers would recreate the duplication being removed. Fixtures carry the setup rather than the specs. `customer` registers through the API rather than the form: nine specs drove the registration form purely to arrive at a signed-in session, so a broken form failed a hundred tests that were not about it, and each paid for a bcrypt round-trip through the UI. `page.request` shares the browser context's cookie jar, so the session belongs to `page`. The specs that are genuinely about registration drive the form properly through `authModal`. `adminApi` takes its base URL from the Playwright config. One spec built its own request context against a hardcoded http://localhost:5173, so changing the port in the config would have moved every test except that one. The inline pg.Client in password-reset.spec.ts moves to support/db.ts. The reasoning for reading the database directly is unchanged and still right — an endpoint that returns a reset token for an arbitrary address is account takeover if it is ever reachable, and an environment gate is a thin thing to stand between that and production — but it no longer sits in a spec where it can be copied into the next one wanting a shortcut. Its default port becomes 55500, the local stack's, rather than 55432: that is the integration suite's disposable Postgres, a different database with different credentials that the app under test is not connected to, and it is Hyper-V-reserved on at least one machine here, so the spec failed with a bare ECONNREFUSED naming a port nobody had chosen. tsconfig.test.json type-checks the tree and runs as part of `npm run build`. It is separate from tsconfig.json rather than widening its `include`, because scripts/check-sonar-tsconfig.js compares the two configs' include arrays, and pulling the Playwright suite into SonarQube's analysis program is a different decision from type-checking it. The whole existing suite type-checks clean on the first run. Lint now covers tests/ with `project` rather than `projectService` — the service resolves a file to the nearest tsconfig.json, which for tests/ is the one that excludes them, and every file then errors as not part of a project. no-floating-promises is an error here: Playwright's API is almost entirely promises, and a missing await on an assertion does not fail, it passes having asserted nothing. Four rule families are switched off for tests rather than left as warnings. Bringing these files in scope added 45, of which none were defects, and #60's argument is that a gate nobody reads is not a gate. There is no React in this directory, and the hooks rules fire on ordinary functions whose parameter is named `use` — which Playwright fixtures are, by its own API. Test credentials are the point of a test and the project's own rule is that they live only in test paths, which is here. Math.random builds unique fixture names so parallel workers do not collide, and a cryptographic generator would say something untrue about what the value is for. The count is back to the 30 that src carried before. Refs #137 |
||
|
bermudalamb
|
b2afa40800 |
Merge pull request 'fix(deploy): run the image QA reviewed rather than rebuilding production (#146)' (#148) from feature/146-promote-tested-image into main
Reviewed-on: #148 |
||
|
bermudalamb
|
e48c7f585b |
fix(deploy): run the image QA reviewed rather than rebuilding production (#146)
The production compose committed in #145 carried `build:` and `pull_policy: build`, copied from the QA stack without thinking about what they mean there. QA builds from this repository because QA is where a change is first assembled and reviewed. Production is not that; production is where the reviewed thing runs. Worse, it contradicted the deploy this repository already documents. README's production steps promote the exact image QA reviewed — `docker tag redefined-designs:qa redefined-designs:latest` — with the stated reason that what ships is what was tested. A compose file that rebuilds instead quietly overrode that, and the two would have disagreed at the moment it mattered. Rebuilding would be a defensible shortcut if a rebuild of the same commit produced the same image. It does not. The Dockerfile copies package.json without package-lock.json and installs with `npm install`, so both lockfiles in this repository are ignored in every build stage and every dependency range is resolved afresh. Two builds of one commit, minutes apart, can differ in any transitive dependency that published in between. "Same git ref" is therefore not "same image", and the reviewed bytes are the only thing that is. So the service now declares `image: redefined-designs:latest` and nothing else. The image has to exist before the stack starts; a first deploy or a pruned NAS fails with "image not found" rather than silently building something new. That is the intended behaviour and is written into the file rather than left to be discovered. The header records what removing `pull_policy: build` costs, because it is not free. That option exists in QA to stop a redeploy reusing a stale tag and appearing to succeed while running old code. Production reintroduces the same risk by a different route — a redeploy that reuses the previous `latest` because nobody re-tagged — so the promotion is load-bearing rather than a convenience, and the deploy has to say which image it is promoting. Also corrects the README's claim that production runs from a stack outside this repository and so cannot be checked. That was true when it was written and stopped being true in #118; leaving it would have taught the next reader that the environment which just failed to boot is the one nothing watches. The remaining half of #146 — copying the lockfiles and installing with `npm ci` so any rebuild means something — is not in this commit. It changes what CI and QA build as well as production, and belongs with its own verification that an unchanged commit produces an unchanged dependency tree. Refs #146 |
||
|
bermudalamb
|
50c9b0dd39 |
Merge pull request 'fix(deploy): commit production's compose and bring it under the drift guard (#118)' (#145) from feature/118-production-compose into main
Reviewed-on: #145 |
||
|
bermudalamb
|
3fde6fc6bf |
fix(deploy): commit production's compose and bring it under the drift guard (#118)
Production refused to boot with "UPLOADS_DIR is required and is not set" while UPLOADS_DIR was set in Portainer's stack variables. Both statements were true at once. Portainer substitutes stack variables into the compose file rather than handing them to the container, so a variable with no line in the file never reaches the app — behaviour the QA compose already warns about at its ADMIN_GATE_SECRET entry, hit in production where nothing was watching for it.
Nothing could have caught it. The drift guard reads docker-compose.qa.yml, and production ran from a Portainer stack outside the repository that no test could see. That is worse than an even gap: UPLOADS_DIR is in ALWAYS_REQUIRED and the test was green, so the natural reading was that the deploying environments set it. QA did. Production did not.
So production's compose is now a file in the repository, deployed as a git repository stack rather than pasted into the web editor — otherwise the committed copy and the running copy drift apart again, which is the whole problem.
Values are hardcoded rather than interpolated wherever they are not secrets. Only a secret has a reason to stay out of the repository, and every interpolation is another chance for the failure above. UPLOADS_DIR in particular has to agree with the volume mapping, and splitting it across two files is how they drift.
The guard now runs over every deployment rather than QA alone, and checks each by handing its parsed entries to validateEnv itself rather than restating the rules. A restatement is one more copy to drift; running the real validator means the file is checked against exactly what the container checks at boot. Interpolated ${SECRET} values count as present, which is right — what is being guarded is that the line exists, since that is what decides whether the value reaches the container.
Environments differ on purpose, so the expectations are registered per file rather than shared: QA is demo mode with a mail allowlist and no PayPal credentials, production is the reverse of all three. A root-level compose file that is not registered fails the last test, so adding an environment forces the decision instead of silently inheriting whatever the loop asserted.
Verified by removing the UPLOADS_DIR line from the production file and confirming three tests fail, one of them reproducing the exact boot error. A guard of this kind that has never been seen to fire is indistinguishable from one that cannot.
Two things found while writing this and deliberately not changed here. RESERVATION_MINUTES is set in QA's compose and read nowhere in the code — drift in the opposite direction, which #118's scan half should catch. And production publishes 32750 on every interface exactly as QA does; that is #117, and the reasoning is recorded in a comment at the ports block rather than acted on, since changing it needs the proxy host entry repointed in the same pass.
Refs #118
|
||
|
bermudalamb
|
c798ba08d7 |
Merge pull request 'feat(scripts): switch Node automatically, and add a test runner (#140)' (#144) from feature/140-node-scripts into main
Reviewed-on: #144 |
||
|
bermudalamb
|
9ac2fa3ba1 |
feat(scripts): switch Node automatically, and add a test runner (#140)
start-local.ps1 knew exactly what was wrong when Node was too old and then made you fix it by hand. Assert-NodeVersion read node --version, found a major below 20, and threw a message telling you to run `nvm use 24.13.1` and start again in a new shell. A good error for a problem the script could simply solve — and since nvm's default here is 18.16.1, it was hit on every fresh shell. It now runs `nvm use latest` itself and -Stop puts the machine back to 18.16.1. The revert also runs when a start fails partway: without that, a run dying in migrations leaves the machine switched with nothing started, and the -Stop that would restore it is never reached. nvm rewrites a machine-global symlink rather than changing one shell, so this changes the Node version for every terminal on the machine while a script runs. That is the intent — the point is to work in whatever shell is already open — but it is announced every time rather than done quietly. The switch is verified rather than trusted. nvm-windows exits 0 for switches that did not happen: a version it cannot find, a symlink it cannot rewrite without elevation, and — observed here — a rewrite issued immediately after another one, where the directory symlink is briefly still the old target. That last case turned up while testing this change: `nvm use latest` reported success and left Node on 18.16.1. So the result is read back and retried once, and nvm's own output is captured rather than discarded, because suppressing it hid the only message that explained the failure. run-tests.ps1 runs the suites: -Suite unit|integration|e2e|all. One script with a parameter rather than three, because the version switch, the database bring-up and the TEST_PGPORT handling are shared and three copies would drift. The integration suite gets its own throwaway Postgres started and stopped around it, in a finally so a failing suite still tidies up. The e2e suite checks the backend is answering first and says what to start, rather than leaving twenty-five specs to fail on a refused connection that names nothing. `all` runs cheapest and most isolated first, so a break several suites would show is reported by the one that localises it best. The version switching lives in scripts/NodeVersion.ps1, dot-sourced by both, since two copies of it would drift and the half that drifts is the half nobody runs. Closes #140 |
||
|
bermudalamb
|
792daccafb |
Merge pull request 'feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)' (#141) from feature/136-configurable-token-lifetimes into main
Reviewed-on: #141 |
||
|
bermudalamb
|
2f7268704a |
feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)
Giving the customer emails a tab of their own (#135) made a family of related holes visible: an admin could edit the wording of every customer email, but not the facts that wording asserted, and two templates could not address the customer at all. Both token lifetimes were hardcoded twice. VERIFY_TOKEN_TTL_MS sat in routes/customers.ts while the verification body separately said "This link expires in 24 hours", and RESET_TOKEN_TTL_MS sat beside a body separately saying "one hour". The prose was not derived from the constant, it was a second hand-written copy of the same fact — so making the constants configurable without addressing that would have made things worse, not better: the setting moves to two hours and the email keeps confidently promising one. Both are now settings, and both templates state their lifetime through an {{expiresIn}} placeholder rendered from the setting. The cart reminder gains {{holdDuration}} for the same reason. Per-item deadlines were already inside {{itemList}}, but there was no way to write a sentence about the hold itself without hardcoding a number the cart expiry setting could change underneath the author. All three durations render through one formatDuration(), so the reset email and the cart reminder say "one hour" the same way rather than in two authors' phrasing. A fractional hour drops to minutes, because "0.5 hours" reads badly and "1.5 hours" reads worse in a sentence a customer is meant to act on. Every template now offers greeting, firstName and lastName. favoriteSold and favoriteWithdrawn previously offered only itemName and siteUrl and could not address anyone — the query behind them never selected a name, so it does now. The greeting itself is two settings, a format and a fallback, rather than the wording baked into greeting(). The fallback is separate rather than the format with the name edited out: that editing is guesswork that has to be right every time, and getting it wrong ships "Hi ," to everyone who registered while first names were still optional (#106). Those customers exist, which is why greeting() guarded the case in the first place. The admin preview renders the durations and the greeting from the live settings rather than from a static sample. The preview exists so an admin sees the email that will be sent, and a sample reading "one hour" while the setting says two is the precise failure these placeholders were added to remove. Settings are read through a new adminSettings accessor. cart_expiry_hours was previously read by an inline query in two places, each with its own `|| '24'`; with five settings and read sites across three routes and the cron job, a default written twice is a default that will eventually disagree with itself. Values are stored as text, so each definition declares how to read it back — numbers were the only kind until the greeting format arrived. The new placeholders are available but never required, so every template an admin has already saved keeps rendering and keeps sending. Also fixes updateAdminSettings announcing success for a save the server refused: it returned the 400 body as though it were the saved settings, so the form reported "Settings saved" either way. Closes #136 |
||
|
bermudalamb
|
6c837725bd |
Merge pull request 'feat(admin): give the customer emails a tab of their own (#135)' (#138) from feature/135-emails-tab into main
Reviewed-on: #138 |
||
|
bermudalamb
|
7df897c0fd |
feat(admin): give the customer emails a tab of their own (#135)
The six email templates lived at the bottom of the Settings tab, under the cart-expiry card and inside a 720px wrapper. Finding them took knowing they were there — "Settings" reads as app configuration and the only thing visible on that tab was a 480px card about cart expiry. Reaching them, the editor was then crushed: EmailTemplateEditor splits a markdown pane and a rendered preview side by side, and 720px left each half under 350px, so the preview showed the email at a width nothing like how it will be read and the markdown toolbar wrapped. Emails is now its own tab, between Customers and Settings, with no width cap. Within it the email types are a left vertical rail rather than a strip across the top: six labels wrapped on narrower displays, and stacking them is what leaves the editor the width the split needs. Settings keeps the cart-expiry card and nothing else. The Default/Customised tag comes off the labels. Six antd tags stacked down a rail stop it being scannable, so a customised template gets a dot and the state in full moves into the editor beside the Restore default button that acts on it. The dot carries aria-label="Customised" so the word stays in the tab's accessible name and the state is not conveyed by a mark alone. Emails owns the fetch it inherited from Settings, and adds a Spin over it. templates starts empty, so the gap before the request lands would otherwise render an empty rail that reads as "there are no emails to edit". Closes #135 |
||
|
bermudalamb
|
717a32c923 |
Merge pull request 'feat(admin): filter inventory by status directly, so Published and Unpublished are reachable (#132)' (#134) from feature/132-admin-status-multiselect into main
Reviewed-on: #134 |
||
|
bermudalambandClaude Opus 5
|
4de1c9b34d |
feat(admin): filter inventory by status directly, so Published and Unpublished are reachable (#132)
There was no way to find unpublished items. Every item has arrived pending since #90 and has to be published, so "what is waiting for me to publish" is a routine question the inventory could not answer. #105 replaced the four-way status dropdown with a Sold / Not sold / All preset and recorded at the time that this gave up isolating a single status, that the pending workflow was the likeliest thing to miss it, and that the fix would be to restore the ability rather than remove the preset. That turned out to be right, and sooner than expected. The admin now selects statuses directly - Pending, Available, Reserved, Sold - rather than choosing among presets over them. The API has accepted several statuses since #105, so this exposes the dimension itself. Everything becomes expressible in one control: Unpublished is Pending, Published is the other three, Sold and Not sold are the sets they always were, and Reserved on its own is reachable again. Two alternatives were rejected. Growing the preset list to five would have kept one click per answer while leaving Reserved unreachable and growing again at the next new question. A second control for publication beside the one for availability would have read more naturally and reintroduced exactly what #105 was built to avoid: Sold and Unpublished is an impossible pair, since a sold item is necessarily published, and two dimensions have to either give that a meaning or block it. One dimension cannot contradict itself. The storefront keeps its three-way preset unchanged. Pending is excluded from every public read, so Published and Unpublished are not distinctions a customer can draw, and the simpler control is the right one there. An empty selection means no filter rather than no statuses, or clearing the box would empty the table. Verification: the admin filter spec is rewritten rather than deleted, and now asserts what the preset could not - Pending alone finds the staged fixture and hides the published ones, and Available plus Reserved plus Sold finds the published ones and hides the staged one. That second case is the one a preset would have had to be invented for. All 7 admin filter tests pass, along with 122 of the suite. Two locator details cost time and are written into the spec so they do not have to be rediscovered: antd renders an invisible role="listbox" shim beside the real option list, so getByRole('option') resolves something zero-sized that cannot be clicked; and a selected status renders as a tag carrying the same title as its option, so an unscoped getByTitle becomes ambiguous once anything is chosen. Beyond the two pre-existing password-reset failures that need a database on port 55432, two storefront specs failed under the full concurrent run and pass six-for-six in isolation, twice. That is the shared-database contention filed as #116, not a regression here: this change touches the admin only. Closes #132 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
7798c847c1 |
Merge pull request 'feat(admin): use the item description's markdown editor for email templates (#131)' (#133) from feature/131-md-editor-for-emails into main
Reviewed-on: #133 |