e437584d7e6254d046bfdf9c9fe1d101d84e76da
469
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
1a5a8b837b |
refactor(uploads): extract the validated image pipeline for a second caller (#222)
A pure move, no behaviour change. #222's public intake endpoint needs the same path from a multipart request to files on the uploads volume that the admin routes use, and the alternative to sharing it is a near-copy that has to reproduce every safety property exactly: the type allowlist, the magic-byte check after the write, names from a CSPRNG rather than from `originalname`, the re-encode that strips EXIF, and the cleanup of whatever a refused request left behind. A copy that drifted on any of those is the gap #95, #103, #180 and #226 exist to close. `stripUploadedImages` moved with the rest, which the plan originally did not say — it was written before #226 added it. Leaving it behind would have given the intake route an upload path that skips EXIF stripping, and no test would have failed to say so, because the intake tests are written against a route that does not exist yet. The refreshed plan added a check with a definite answer, and it now holds: routes/admin.ts no longer imports imageProcessing at all. Every comment came across verbatim. They record why the code is shaped as it is and are the most valuable part of what moved. Lint caught something the compiler did not: MAX_IMAGES_PER_REQUEST was left imported into admin.ts, where its only use — `upload.array('images', MAX_IMAGES_PER_REQUEST)` — had moved away with the middleware. It stays exported from imageUpload for the intake route's caps, but admin.ts does not need it. Verified as a refactor rather than as a change: the four suites that exercise this path hardest were run before the move and after it, 44 tests both times, same suites, same count. Full backend: 263 integration, 302 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222 |
||
|
bermudalamb
|
6df32af784 |
feat(intake): add upload_links and item_drafts, and default an item's price (#222)
The schema for the intake pipeline. A submission becomes an `items` row at status 'pending' — already invisible to every public and storefront query since #90 — with an `item_drafts` row beside it holding the submitter's note, which link it arrived through, and the fields the drafting worker will fill in later. `upload_links` stores a digest rather than a token, so a leaked database is not also a leaked set of working links, and the admin screen can show a token exactly once. `max_submissions` is nullable for "no cap", but the route will default it to a finite number: an unbounded link should be something asked for, not something that happens when nobody thought about it. `item_drafts.upload_link_id` is ON DELETE SET NULL rather than CASCADE. Deleting a link must not delete the items that arrived through it — provenance is lost, the goods are not. `items.price_cents` keeps NOT NULL and gains a default of 80.00, so an arriving item is always priced. That is the decision taken in the design review over making the column nullable: it costs the schema-level guarantee that nothing can publish at a price nobody chose, and buys not having to teach the cart, the checkout and thirteen other files about an item without a price. The protection moves into the review queue, and `price_source` exists so that queue can say whether a number came from a model, the default, or a person. The number lives in the migration rather than in configuration. Changing a default price is a rare, deliberate act that deserves a record; an environment variable would let it drift silently between environments, and a wrong default is invisible until something has already sold at it. Verified up, down and up again rather than only forwards — an irreversible migration is one that cannot be tested. Then verified by inspection rather than assumption: the default reads 8000, both tables and the state index exist, and an item inserted with no price comes back at 8000. Backend: 263 integration, 302 unit, all passing against the new schema. Ref #222 |
||
|
bermudalamb
|
7d8ac15ef8 |
docs(intake): refresh the extraction task for the changes #226 made (#222)
This plan was written on 2026-08-29, before #226 landed. Its Task 2 lists what to move out of routes/admin.ts into the shared image pipeline, and that list is now missing `stripUploadedImages` and the `reencodeInPlace` import it depends on, because neither existed when the list was written. Executing it as written would have left the re-encode behind in admin.ts, and the public intake route added in Task 5 would then have had an upload path that skips EXIF stripping entirely. That is precisely what #226 exists to prevent — a stranger photographing an item at home publishing the coordinates it was taken at — and nothing in the suite would have failed to say so, because the intake tests are written against a route that does not exist yet. The task now names the function, says why it matters, and adds a check with a definite answer: after the move, routes/admin.ts must no longer import imageProcessing. If it still does, something was left behind. Ref #222, #226 |
||
|
bermudalamb
|
7100352d98 |
Merge pull request 'chore(ci): remove the registry spike workflow (#237)' (#249) from chore/237-remove-spike-workflow into main
Reviewed-on: #249 |
||
|
bermudalamb
|
f5deb31370 |
chore(ci): remove the registry spike workflow (#237)
The spike is answered, so the throwaway goes as it always said it would. What it established. The runner can build images once a docker CLI is installed — the socket was mounted all along and only the client was missing. The container registry works and accepts pushes; it had never been exercised, so that was genuinely unknown. TLS from the runner to the Gitea host is trusted, which also answers the certificate half of the Portainer question. A personal access token with write:package authenticates where the token Actions injects automatically does not. And a full image build and push costs 9m14s on that runner. What it disproved, which was the point. A CI-built image still reports `commit: "unknown"`. #235 removed `COPY .git` from the Dockerfile to stop the version stamp breaking every Portainer deploy, so it does not matter that an Actions checkout has history — the Dockerfile never copies it. Build location was never the problem, and moving builds to CI would have delivered nothing on its own. #248 carries the actual fix, a build arg, which is a few lines and does not need the registry at all. Deleting this from main rather than only from the spike branch: it was merged here in #238, before iteration 2 showed that workflow_dispatch fires from a branch and a spike never needed to reach main at all. Still owed by hand: the spike-trivial and spike-3085970 packages in the registry. Closes #237 |
||
|
bermudalamb
|
a800533be7 |
Merge pull request 'test(e2e): design, plan, and the find-or-fail helper (#241)' (#247) from fix/241-e2e-isolation into main
Reviewed-on: #247 |
||
|
bermudalamb
|
64324609e1 |
test(e2e): add a find-or-fail helper for collection lookups (#241)
Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal. The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase. It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures. Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails. Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean. Ref #241 |
||
|
bermudalamb
|
d16aba1647 |
docs(test): plan the e2e trustworthiness work (#241)
Four tasks. A pure findOrFail helper with Vitest coverage, the nine unchecked lookups converted to use it, the expect timeout raised from Playwright's unset default of 5s to 10s, and the test #245 skipped brought back. The helper deliberately imports nothing and lives apart from support/api.ts. api.ts imports @playwright/test, and vitest.config.ts runs with environment: 'node' over tests/unit only — putting the helper there would drag a browser harness into the unit suite to test six lines of pure logic. Two things the survey changed. filters.spec.ts:216 is excluded: its .find() searches CSS class names on a string array rather than test data, so it has no missing-row failure mode. And three `expect(row).toBeTruthy()` assertions are deleted rather than kept, because findOrFail has already thrown by then — leaving them would tell the next reader the value might be falsy, which is the confusion the change exists to remove. Worker-count reduction is explicitly not in this plan. It is a real lever and may still be needed, but applying it at the same time would make it impossible to tell which change fixed anything. The plan states the criterion it cannot check: CI's load is not reproducible here, so two consecutive green local runs mean the refactor is sound, not that the flakiness is gone. Several consecutive green CI runs are the real bar, and #241 stays open until then. Ref #241 |
||
|
bermudalamb
|
f8b2b68f0d |
docs(test): design for making the e2e suite trustworthy (#241)
Five distinct specs failed across two runs of identical code with no overlap between the sets, so which test fails is decided by the scheduler. The cost is already being paid: #245 skipped the only end-to-end check that adding an item reaches the database, purely to get main green. The design separates two mechanisms that had been treated as one. Unchecked lookups into shared collections — `collection.find(...)` dereferenced immediately, found at eight or more sites — are a defect regardless of concurrency: when the row is missing the test dies with "Cannot read properties of undefined" naming test plumbing rather than failing an assertion that says what it wanted. Load-induced timing is the other, and is the larger share of what has actually been observed: three of four local failures and the CI one are assertions in a spec's own browser context that nothing else can touch. Two claims from earlier in this investigation are retracted in the document rather than quietly dropped. The verification-resend limiter is not a shared axis — it is keyed per customer and every test registers its own — and the suite contains no snapshot-style assertions, so there is nothing to convert to web-first. Both were stated as fact on the issue, and both would have justified work that was not needed. Per-worker databases are ruled out structurally: every worker talks to one backend on :3000, so isolation there means N backends, not N databases. Worker-count reduction is deliberately deferred rather than taken now, because applying it at the same time would mask whether fixing the defects worked. The honest limit is recorded too. CI's load cannot be reproduced here on demand, so the timing changes rest on reasoning rather than a red-to-green demonstration, and the success criterion is several consecutive green runs rather than one. Ref #241 |
||
|
bermudalamb
|
cd4618e266 |
Merge pull request 'test(e2e): skip the admin save happy-path test while #241 stands (#245)' (#246) from fix/245-skip-flaky-admin-save into main
Reviewed-on: #246 |
||
|
bermudalamb
|
66c696ce40 |
test(e2e): skip the admin save happy-path test while #241 stands (#245)
`main` has been failing on one e2e test since the sold-filter fix landed, and it is a different test from the one #239 corrected: admin-save-failures' "saves an item successfully when the server accepts it". Skipped rather than fixed, deliberately. It fails in CI and passes locally, and which test fails moves around — a local parallel run of the whole suite on the same commit failed four *different* specs (admin-inventory-filters, auth, favorites-filter, resend-verification) and not this one. That is #241: fullyParallel against a single shared database. Fixing this test on its own would be guessing at a symptom that reappears somewhere else next run. Ruled out before disabling anything: the re-encoding from #226 is not involved. AdminInventory.addItem fills a name and a price and saves, attaching no files, so stripUploadedImages iterates an empty array and the image path is never entered. Checked rather than assumed, because this spec is on the admin save route and that is exactly where a regression of mine would surface. What this stops covering is not trivial, and the comment says so at the call site: it is the only end-to-end check that adding an item actually reaches the database rather than merely firing a toast. #245 exists so that it is un-skipped when #241 lands, rather than left behind. A skipped test on the core admin save path is worse than a red build, because a red build is at least visible. Ref #245, #241 |
||
|
bermudalamb
|
feb714c49d |
Merge pull request 'test(perf): stop hashing test passwords at production cost (#242)' (#243) from fix/242-bcrypt-cost-in-tests into main
Reviewed-on: #243 |
||
|
bermudalamb
|
d7dacffa11 |
test(perf): stop hashing test passwords at production cost (#242)
The integration suite registers around thirty-five customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is a pure-JS implementation, so it pays that cost several times over compared with a native build, and hashing was most of the suite's wall clock. On a contended runner it pushed adminInventory.integration.test.ts past its twenty-second timeout, which then surfaced as a foreign key violation somewhere else entirely — the test timed out, jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted. Measured rather than asserted, warm run against warm run with only the constant changed: 34.5s at cost 12, 9.8s at cost 4. Three and a half times faster, about twenty-five seconds off every integration run, with all 263 tests passing either way. The first attempt at that measurement was wrong and worth recording. Comparing a cold run at cost 4 against a warm run at cost 12 made the change look like a 36% regression-shaped improvement of the wrong size; the difference was ts-jest and Postgres warming up, not the cost factor. Both numbers above are second runs, and the cost-12 figure was taken twice — 34.3s and 34.5s — before being believed. Deliberately not configurable. An environment variable here would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune it. The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend, since app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than warned about, which is the reasoning that already made DEMO_MODE strict. `hashRoundsFor` is pure and separately tested because the failure it guards against is silent: only the exact string 'test' earns the cheap cost, and an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions too — without that the branch tests pass while the numbers drift to something useless. Closes #242 |
||
|
bermudalamb
|
2ba35f8732 |
Merge pull request 'fix(test): correct the sold-filter tally assertion stranded by #188 (#239)' (#240) from fix/239-sold-filter-tally into main
Reviewed-on: #240 |
||
|
bermudalamb
|
30227fb1e1 |
fix(test): correct the sold-filter tally assertion stranded by #188 (#239)
main has been red since 2026-08-25. Every SonarQube run reported 147 passed, 1 failed, and it was this test every time — expected "Filters", received "Filters (1)". The test is stale, not the code. It was last touched on 2026-08-23 in #137; the tally logic changed on 2026-08-25 in #188, which never touched the spec. #188 redefined the tally as the number of chips and moved the availability preset into the dimension system as a bar dimension — one that still emits a chip for any non-default status, deliberately, because without it `?status=reserved` is an empty grid with no Clear filters button and no way out but editing the URL. The test asserted the rule that held before that change. Counting bar chips differently from drawer chips would restore exactly the per-screen special-casing #188 removed, and the drift it fixed was the admin's tally disagreeing with the storefront's. So the assertion moves, not the tally. The replacement also checks the tally comes back down when the default is restored. The original only ever asserted one direction, which would pass against a count that incremented and never decremented — worth fixing while the test is open rather than leaving a second gap behind the first. There is a real wart left standing: `Filters (1)` opens a drawer with nothing selected in it, because the filter it is counting lives in the bar. That is a cost of #188's design rather than a defect in it, and the comment now says so rather than leaving the next reader to rediscover it. Verified by running the spec in isolation with a single worker: 6 passed. Closes #239 |
||
|
bermudalamb
|
65fcba24f2 |
spike(ci): probe whether the runner can build and push an image (#237)
Throwaway. Deleted once #237 has an answer, whichever way it goes. #233 was designed on an assumption about the Docker build context and broke every Portainer deploy. The chosen fix for the commit stamp — build in Actions, push to Gitea's registry, have Portainer pull — rests on three more assumptions about infrastructure that nothing in this repository can confirm. This probes them instead of designing around them. Whether a job here can run `docker build` at all is genuinely unknown: the other workflows use `services:`, which proves the runner can start containers, not that it can build images. Whether the registry is usable is likewise unconfirmed — the packages API answers but lists nothing, so it has never been exercised. And the real build is timed because backend-integration.yml is manual after a job once held this runner for 3h12m, so what an image build costs here is part of deciding whether building on every merge is tenable at all. The trivial image is built and pushed before the real one on purpose. It separates "can this runner build and push anything" from "does our Dockerfile work here", so a later failure still says which half is broken. workflow_dispatch only, with no push or pull_request trigger, so merging this changes nothing until somebody presses the button. Bounded by timeout-minutes on the same reasoning backend-integration.yml already documents. Ref #237 |
||
|
bermudalamb
|
71f8a21e45 |
Merge pull request 'fix(build): stop the version stamp from breaking every Portainer deploy (#235)' (#236) from fix/235-remove-git-copy into main
Reviewed-on: #236 |
||
|
bermudalamb
|
1c69ac71f6 |
fix(build): stop the version stamp from breaking every Portainer deploy (#235)
`COPY .git ./.git`, added in #233, fails with `"/.git": not found` in Portainer's build context, so every stack deploy died before anything else ran. This is the exact outcome #233 set out to prevent. That issue states that a version stamp must never be the thing that stops a deploy, and the resolution code honours it — every unreadable-.git path returns "unknown" and warns. The guard was simply in the wrong layer: COPY fails at image-build time, long before any of that code executes. Graceful degradation in the application buys nothing once the Dockerfile has refused to build. The assumption came from the local build context, where there is no .dockerignore and .git is therefore present. It was checked with a local `docker build`, which passed, and never against the only environment that actually deploys. Verified properly this time, by building from `git archive HEAD` — a context containing exactly the tracked files and no history, which is what a clean checkout gives. That build now succeeds and stamps `commit: "unknown"` with a real `builtAt`. The failing case is reproduced and fixed rather than reasoned about. `commit` will read "unknown" wherever Portainer builds. `builtAt` is still real, and is the half that matters most: Portainer already reports which commit it cloned, but cannot tell you whether the running container is that build. A build time can, and a stale one is exactly what the QA incident earlier today would have shown. Locally nothing changes — writeBuildInfo reads ../.git directly and still resolves a real commit. Sourcing the real commit inside a Portainer build needs a different mechanism, and #235 records the three candidates rather than guessing at a fourth. Closes #235 |
||
|
bermudalamb
|
a685229527 |
Merge pull request 'feat(admin): show the deployed commit and build time in the admin (#233)' (#234) from feature/233-admin-version-stamp into main
Reviewed-on: #234 |
||
|
bermudalamb
|
44328d0b5c |
feat(admin): show the deployed commit and build time in the admin (#233)
There was no way to tell which build an environment was running. That is not hypothetical: minutes after #232 merged, `npm run backfill:images` in QA failed with `tsx: not found` because the container was still serving the pre-merge image, and the only thing that revealed it was npm echoing the old script line. Had the change been anywhere other than a package.json script, the container would have looked healthy while running the wrong code.
The header now reads something like `a5076cc · built 29 Aug 20:36`. The commit answers "is this the code I expect"; the build time answers "did my redeploy actually rebuild", which is a different question and the one that would have caught the case above.
The commit is read out of `.git` directly rather than by shelling out, because node:20-bookworm-slim has no git binary and adding an apt layer so the image can print seven characters is a poor trade. `.git` is copied into the build stage only — verified absent from the final image — so no repository history reaches a deployed container.
Resolution is pure and separately tested across every shape that actually occurs: a detached HEAD holding the object name, which is what a checkout of a ref produces; a symbolic HEAD followed to a loose ref file; the same followed to packed-refs, which is what a fresh clone commonly has; peeled `^` tag lines ignored so an annotated tag cannot yield the wrong commit; and every failure path returning `unknown`. That last part is the one that matters most — this runs during a Docker build, and a version stamp must never be the thing that stops a deploy.
Served from a gated /api/admin/version rather than folded into /api/config. That endpoint is public, and a commit hash there would tell any storefront visitor exactly which revision of a public repository is deployed. An integration test asserts the gate and asserts the public config does not carry it, because the boundary is the whole point rather than an implementation detail.
Verified in the built image rather than argued: the stamp inside it reads
|
||
|
bermudalamb
|
a5076cc217 |
Merge pull request 'fix(uploads): ship the image backfill script in the container image (#231)' (#232) from fix/231-ship-backfill-script into main
Reviewed-on: #232 |
||
|
bermudalamb
|
167c8ad97c |
fix(uploads): ship the image backfill script in the container image (#231)
`npm run backfill:images` could not run in QA or production. Three reasons, each sufficient alone: tsconfig includes only `src`, so `scripts/` was never compiled; the Dockerfile copies `dist`, `migrate.js` and `migrations` and never `scripts/`; and `tsx`, which the npm script invoked, is a devDependency that `npm install --omit=dev` strips from the final stage. The half of #226 that closes the exposure on already-stored photos had no way to run where the photos are. Moved to `src/backfillImageReencode.ts` so it compiles into `dist` and ships. Both of its runtime dependencies, sharp and pg, were already production dependencies, so the image needs nothing else. `scripts/bench-hash-latency.ts` was the pattern followed originally, and it is a development tool that never needs to run deployed; this one is an operational task that can only be useful where the images are, which makes `migrate.js` the right precedent instead. The npm script now runs the compiled output rather than tsx, so one command behaves identically on a laptop and inside a container. The entry point is guarded with `require.main === module`: putting a catalogue-wide irreversible rewrite in the same directory the server imports at boot means an accidental import would otherwise run it, and nothing should depend on people continuing not to write that import. Proven in the built production image rather than argued. `node_modules/.bin/tsx` and `scripts/` are both absent from it, and `npm run backfill:images` still runs: report mode found the planted file, `--apply` rewrote it 35760 to 16019 bytes, a second `--apply` reported skipped 1 processed 0, and on the mounted volume the EXIF was gone with the image bounded to 2000x1333 and still JPEG. That is the exact scenario the previous version would have failed. Backend: 285 unit, 260 integration, tsc clean, lint unchanged at six warnings, all six pre-existing. Closes #231 |
||
|
bermudalamb
|
846646f7ec |
Merge pull request 'fix(uploads): re-encode uploaded images to strip EXIF and cut stored bytes (#226)' (#229) from feature/226-strip-exif into main
Reviewed-on: #229 |
||
|
bermudalamb
|
32c1f23379 |
Merge main into feature/226-strip-exif
main gained the Drizzle spike (#216) after this branch was cut, and both changes add a production dependency, so `backend/package-lock.json` conflicted. `backend/package.json` merged cleanly and carries both `drizzle-orm` and `sharp`. The lockfile was regenerated rather than hand-merged: main's version taken as the base, then `npm install` re-resolved it. That install was deliberately run under Node 24 rather than the machine's default 18.16.1, because sharp's platform binaries are optional dependencies that npm silently omits when the engine check fails — regenerating this file on Node 18 would have quietly dropped every `@img/sharp-*` entry and produced a lockfile that installs a sharp which cannot load. Verified afterwards that linux-x64, linux-arm64 and win32-x64 are all present and that drizzle-orm survived. Backend: 285 unit tests pass, tsc clean. Lint reports six warnings rather than three; the three new ones are in src/db-drizzle from the spike, not from this branch. Ref #226 |
||
|
bermudalamb
|
fe8f2aed90 |
Merge pull request 'spike(db): evaluate Drizzle and Tinqer against the hardest query we have (#216)' (#230) from feature/216-drizzle-spike into main
Reviewed-on: #230 |
||
 bermudalambandClaude Opus 5
|
664a0c30ed |
spike(db): evaluate Drizzle and Tinqer against the hardest query we have (#216)
Both libraries converted the same target — `buildItemFilterSql`, six clauses composed at run time, a recursive CTE for the category subtree, an `ANY(...::int[])` tag match with a count equality. Nothing in `src/routes` or `src/itemFilters.ts` is touched; this branch only adds spike artifacts alongside them.
Drizzle cleared the blocker the issue named first. `backend/tsconfig.json` is `module: commonjs` and Drizzle is ESM-first, but it compiles under the existing config and requires at runtime, so no ESM migration is hiding inside this one.
`drizzle-kit pull` introspected all sixteen tables plus `pgmigrations`, 104 columns, 8 indexes and 20 foreign keys, and got the hard parts right: the self-referencing `categories.parent_id`, and both partial unique indexes with `lower(name)` and their `WHERE` predicates.
The converted filter produces byte-equivalent results. Five filter combinations run against the dev database return identical id lists to the current implementation, including the recursive subtree — 1805, 2145, 4, 1918 and 2145 rows respectively.
The injection question the issue asked about is answered yes, and it is stronger than expected. In a Drizzle `sql` template `${value}` emits a bind parameter, not text, so there is no way to spell "interpolate this as SQL" by accident. Feeding `"1); DROP TABLE items; --"` as a status produced it in the parameter array and nowhere in the query text. That is the #202 invariant enforced by the type system rather than by a comment and two tests.
Two Drizzle findings worth having before committing to 187 call sites. Arrays do not bind the way the raw driver does: `${array}` expands into a placeholder list, so `ANY(($1, $2)::int[])` type-checks, reads correctly, and fails at run time as invalid Postgres. `sql.param()` is required, and nothing warns. And the first generated migration after a pull carried spurious drops and recreations of the three expression indexes; re-running with no schema change reports nothing to migrate, so it settles rather than recurring, but that first migration would need hand-editing.
Tinqer is genuinely LINQ-to-SQL — it parses the lambda with OXC at run time and compiles a real expression tree — and it cannot express this query. Compound conditions and array membership work. A ternary fails. A block body with an `if` fails. Those are the only two ways to make a clause optional inside the lambda, and there is no raw-SQL escape hatch in its API, so six independent optional clauses would mean 64 hand-written plans or neutral sentinels that do not exist for the category and tag clauses.
Its failure mode compounds that: `defineSelect` parses eagerly and throws, so an unsupported query type-checks cleanly and crashes when the module is first required. `src/db-tinqer/probe.ts` wraps every case in a function for that reason.
It is also `0.0.27` with 24 stars, and its Postgres support is a `pg-promise` adapter rather than the `pg` driver already in use.
I was wrong earlier to say LINQ-to-SQL is impossible in TypeScript because it needs C# expression trees. Tinqer reconstructs the tree by parsing the lambda source. The claim should have been that it is possible and rare, and the constraint is what the parser accepts.
Verified: backend build clean, 280 unit tests and 255 integration tests pass, unchanged by this branch.
Refs #216
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
bermudalamb
|
502d56d9fd |
feat(uploads): backfill re-encoding over already-stored photos (#226)
Stripping new uploads does nothing for the catalogue that is already on the storefront, which is where the exposure actually lives today. This is the other half. Reports by default and rewrites nothing without --apply, because the transform is lossy and there is no undo. Idempotency comes from `needsProcessing` rather than from a marker or a schema change: a file with no EXIF already inside the bounds is already in its final state, so a second run skips it instead of putting it through another lossy pass. Proven rather than assumed — a second --apply immediately after the first reports skipped 1, processed 0. Verified end to end against a real row and a real file. 3000x2000 carrying GPS EXIF became 2000x1333 with the metadata gone, 35760 bytes down to 16019, the format preserved, no temporary file left behind, and `item_images.image_path` untouched. That last part is what preserving the format bought: the backfill rewrites bytes and writes nothing to the database, so there is no window where a row points at a file that no longer exists. Both degenerate branches are exercised too, since a script that dies partway through a catalogue leaves the rest of it exposed: a row pointing at a missing file and a row with an extension the application would refuse to serve are each reported and counted, and the run continues. `handleRow` is split out of `run` for cognitive complexity, and while doing that a miscount was introduced and caught — incrementing `processed` before the rewrite meant a file that threw would have been counted as both processed and failed, which makes the summary unreadable at the moment it matters most. Ref #226 |
||
|
bermudalamb
|
aecccef418 |
feat(uploads): strip metadata from every accepted upload (#226)
Hooked into uploadImages rather than into the routes. That middleware is where verifyUploadedImages already runs and is the single choke point every upload path passes through, so the admin create and update routes are both covered and the intake route from #222 will inherit it rather than having to remember. The same reasoning discardUnlessAccepted already gives for being a hook instead of a call. Runs after verification, deliberately: re-encoding a file whose bytes do not match its declared type would be work on something already refused, and sharp's error would replace the clearer message that check produces. A re-encode failure refuses the upload rather than storing the original, because the one case where a photo keeps the coordinates it was taken at should not be the case nobody was told about. The test builds a JPEG carrying GPS tags rather than committing a binary fixture, so what it contains is readable, and it asserts the fixture really carries EXIF before asserting the stored file does not — otherwise the test would pass while proving nothing. GPS tags go in IFD3, which is the GPS IFD as libvips names it; sharp's Exif type has no separate GPS key, and putting them in IFD0 would have produced EXIF without producing the tags this issue is about. Backend suites: 285 unit, 260 integration, lint clean, build clean. One caveat worth recording. Across three full integration runs, `uploadValidation` failed once on "removes the upload when the request is refused for its other fields". It is a pre-existing race rather than a regression: discardUnlessAccepted cleans up in an unawaited `void discardUploads(...)` inside a `res.on('close')` handler, so a test asserting on the directory immediately after the response has always been able to observe the state before the unlink lands. Re-encoding adds enough libvips work to lose that race occasionally where it previously did not. The property still holds in production, where the process keeps running and the unlink completes. Filed separately rather than fixed here. Ref #226 |
||
|
bermudalamb
|
e85be0f970 |
feat(uploads): re-encode images to strip metadata and bound dimensions (#226)
Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. The same reasoning that makes uploadTypes.ts an allowlist rather than a denylist. `needsProcessing` is pure and separately tested because it is the whole of the backfill's idempotency argument: a file with no EXIF already inside the bounds is already in its final state, so a second run skips it instead of putting it through another lossy pass. Being wrong there degrades every image a little more on every run. Anything sharp cannot describe is processed rather than skipped, since a file we understand least is not one to assume is safe. Verified end to end on a real image before wiring anything up: 3000x2000 with EXIF present became 2000x1333 with EXIF absent, and no temporary file was left behind. Corrects something this README claimed an hour ago. Installing under a Node below 20.9.0 does produce a broken sharp, because npm skips the optional platform binary when the engine check fails and still reports success. But once that binary is present sharp loads and runs fine on 18.16.1 — `engines` is enforced at install time, not at require time. The README said the runtime was blocked, which would have sent someone switching Node versions to fix a problem that only the install created. Ref #226 |
||
|
bermudalamb
|
7d45b69305 |
build(uploads): add sharp for image re-encoding (#226)
Verified where it actually has to run rather than only here: the production image builds and `require('sharp')` succeeds inside it on Node v20.20.2, linux/x64, with libvips 8.18.6 and `withExif` available, needing no build toolchain. The architecture question is already settled by this same node:20-bookworm-slim base running in production today, and sharp ships glibc prebuilds for both linux-x64 and linux-arm64, so it adds no constraint that deployment did not already satisfy.
Installing it locally found a trap worth recording. sharp requires Node >=20.9.0 and its platform binary is an *optional* dependency, so npm skips it when the engine check fails and still reports success. Installed under this machine's default 18.16.1 the result is a node_modules that looks complete and throws `Could not load the "sharp" module using the win32-x64 runtime` at require time — which reads as a broken package rather than as a wrong Node version. The fix is `npm install --include=optional sharp` under Node 20+, and the prevention is using start-local.ps1 or run-tests.ps1, which switch first.
`engines` is now declared so npm at least warns, and the README's existing Node 20 section says what the failure looks like, since the error message names a runtime rather than a version and points nowhere useful.
The lockfile carries every platform variant including linux-x64 and linux-arm64, so a build on another platform resolves correctly. The Dockerfile does not copy the lockfile at all and installs fresh, so this matters for contributors rather than for the image.
Ref #226
|
||
|
bermudalamb
|
5a3c0db0bc |
Merge pull request 'docs(intake): design and implementation plans for the intake pipeline (#220)' (#221) from feature/220-intake-pipeline-design into main
Reviewed-on: #221 |
||
|
bermudalamb
|
65f9d00785 |
docs(uploads): plan the EXIF stripping and re-encode (#226)
Five tasks: prove sharp installs where it actually runs, the re-encode policy as a pure module, wiring it into the single middleware every upload path already passes through, the backfill over already-stored photos, and the deployment sequence. Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. Same reasoning that makes uploadTypes.ts an allowlist. Format is preserved rather than normalised to WebP. Converting would compress better but changes every stored extension, and therefore item_images.image_path, turning the backfill into a rename with a window where rows point at files that no longer exist. A privacy fix does not need that risk, and the backfill consequently touches no database rows at all. The backfill is lossy and irreversible, so it reports by default and needs --apply, writes to a temporary file and renames so an interruption cannot leave a half-written image being served, and is idempotent by construction: a file already stripped and already within bounds is skipped rather than put through a second lossy pass. That property is a pure function with its own unit test, because being wrong about it degrades every image a little more on every run. Two traps the plan handles that the issue only named. An animated WebP read without the animated flag decodes to a single frame and is silently written back as a still, so the flag is set for WebP and only WebP — it changes how resize reads height, which would be wrong for the other types. And sharp before 0.33 has no withExif, which the tests need to build their fixture; on an older version they fail as though the stripping were broken. Ref #226 |
||
|
bermudalamb
|
c76828122d |
docs(intake): guard the uploads volume and bound a link by default (#220)
Two tasks added to the slice-1 plan, closing the half of the upload gap the middleware ordering does not. The ordering fix stops a caller with a bad token writing anything. A caller with a working one can still send six eight-megabyte files per request against a limiter that allows twenty requests a window, and nothing checks whether the volume can take it. That volume is shared with the admin upload path, so intake filling it is a shop outage rather than an intake outage. Task 8 refuses an upload when less than a gigabyte remains, on the admin item routes as well as intake, failing closed because a volume that cannot be measured is not one to assume is empty. Task 9 makes an absent cap mean the bounded default of twenty-five rather than unlimited: the router as written treated omission as "no limit", so the ordinary act of creating a link produced an unbounded one, and a cap that has to be remembered is not a control. Two larger findings are filed rather than folded in. Re-encoding uploads to strip EXIF and cut stored bytes (#226) touches the shared pipeline and adds a native dependency; a global ceiling with an abuse alert (#227) needs its own state and an email. The EXIF one is worth stating plainly: nothing strips metadata today, so an uploaded phone photo publishes the coordinates it was taken at, at a public URL. That is already true of the admin path and is not introduced here, but this slice widens who can put such a file there. Ref #220 |
||
|
bermudalamb
|
783d10abc4 |
docs(intake): plan the upload-link and submission-page slice (#220)
The first of four slices from the intake design, and the only one that is worth planning in detail yet — the later slices' shape depends on what this one actually produces. Seven tasks: the schema, extracting the validated image-upload pipeline out of routes/admin.ts so the public endpoint reuses it rather than growing a near-copy of it, token generation and hashing, the admin API for issuing and revoking links, the public submission endpoint, the submission page, and the admin screen. Three things the plan settles that the design left open or got wrong. The image caps become the constants already in the codebase rather than the 10-photo and 10 MB figures the design invented, because two different caps on one pipeline is a defect waiting to happen. The feature flag is dropped from this slice: nothing is reachable until a link exists, and the flag earns its keep in slice 2 where a paid API call appears. And the link is resolved before multer runs, so a stranger holding a bad token cannot cause a byte to be written to the uploads volume — cleanup afterwards would leave an unauthenticated caller in control of disk churn, and leans on an unlink that a crash between write and delete would skip. That ordering is asserted by a test, so a later reordering fails loudly rather than silently. Ref #220 |
||
|
bermudalamb
|
722bade383 |
docs(intake): price arriving items rather than leaving them unpriced (#220)
`price_cents` stays NOT NULL and gains a default of 80.00. Where the model suggests a price the worker writes it onto the item; where it does not, the default stands. This is cheaper to build than the nullable design it replaces — the column's type is unchanged, so the fifteen files that read `price_cents`, the cart and the checkout among them, keep working untouched, and the migration adds a default and nothing else. It also gives up a guarantee. The storefront can now be reached by a price the admin never chose, so the protection moves out of the schema and into the review queue, where it is weaker. 80.00 is a plausible number rather than an obvious sentinel, so a default left unnoticed sells the item instead of announcing itself the way "$0.00" would. `price_source` is added to make that legible: the queue labels a price as coming from the model, the default, or the admin, and marks anything unconfirmed as such at the point of publishing. Publishing unconfirmed remains allowed — that is the decision taken — but it is stated rather than silent. Publishing still happens only from the queue, and the notification email still carries no publish button. Ref #220 |
||
|
bermudalamb
|
dbe6a0cf8f |
docs(intake): design the upload-link, AI-draft and review-queue pipeline (#220)
A named, revocable link lets someone without an account send in photos of one item plus a note; a background worker drafts the listing; the admin is emailed and publishes it deliberately from a review queue. Most of the lifecycle already exists and is reused rather than rebuilt: `pending` has been the unpublished state since #90 and is already excluded from every public query, the upload path already validates magic bytes against a three-type allowlist, mail already has editable templates and an allowlist guard, and node-cron is already the background-work pattern. What is new is a way in for someone with no admin account, the first LLM integration in this codebase, and somewhere to review a draft. The design turns on one invariant: nothing reaches the storefront at a price a model guessed. The suggested price lives on the draft and never on the item, the email carries no publish button, and the publish path refuses an item with no price. That is also why `price_cents` becomes nullable rather than defaulting to zero — a sentinel that formats as "$0.00" is the same class of quiet failure as `DEMO_MODE` once being "demo unless the value is exactly false", and nullability makes the compiler enumerate all fifteen call sites instead. Ref #220 |
||
|
bermudalamb
|
e933cd19f9 |
Merge pull request 'fix(cart): say what the demo button does rather than what the shop does (#203)' (#214) from feature/203-demo-notice-wording into main
Reviewed-on: #214 |
||
|
bermudalambandClaude Opus 5
|
b897e99363 |
fix(cart): say what the demo button does rather than what the shop does (#203)
#195 added a notice reading "Demonstration only — This shop is not taking payments at the moment", gated on `demoMode` alone. That claim is false in a configuration the ops runbook actively steers towards. `demoMode` and `paypalClientId` are independent. `checkDemoMode` and `checkPayPal` only make the PayPal secrets *required* when `DEMO_MODE=false`; nothing forbids them while it is `true`. And `production-stack-cutover.md:65` says flipping to `false` without all three crash-loops the container — so the only safe order is to populate the secrets while demo mode is still on, verify, then flip. In that window `Cart.tsx` renders live PayPal buttons directly beneath a banner telling the customer the shop takes no payments, and it is precisely the window in which someone is clicking around production checking their work. That is the same failure #195 fixed, pointed the other way: silent where a warning was needed, then confidently wrong where a customer can actually be charged. Telling someone nothing will be shipped above a live PayPal button is worse than saying nothing. The notice now describes the button instead of the shop, which is true in both configurations and stays visible in the one with two controls that do different things — where a customer most needs to be told they differ. Gating it on `!paypalClientId` would also have removed the false claim, by hiding the notice exactly there, which is the worse trade. The test for it was also not testing it. "Says so before the customer commits" seeded an address with `isDefault: true`, and the cart auto-selects the default on load — so an address was already selected and the button already rendered when it asserted. It would have passed with the notice moved inside the `selectedAddressId` guard, which is the regression it exists to catch. It now seeds no address and asserts the notice is up while the checkout button is absent, which states the property directly. Mutation-tested rather than assumed: moving the Alert inside that guard fails the new test, and would not have failed the old one. Verified: 3 end-to-end tests pass against a browser, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #203 Refs #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
ec08a73ebc |
Merge pull request 'docs(security): put the SQL injection invariant where it is enforced (#202)' (#212) from feature/202-sql-invariant into main
Reviewed-on: #212 |
||
|
bermudalambandClaude Opus 5
|
c704c07b89 |
docs(security): put the SQL injection invariant where it is enforced (#202)
#180 cleared the three `typescript:S2077` hotspots and marked them Reviewed/Safe on the dashboard, but the repository half never reached main — the branch carrying it was deleted before merge, so the markers are cleared, the issue is closed, and nothing in the code said why. That is the exact state #180 set out to avoid: "the justification has to live in the repository, not only in SonarQube's UI". The three call-site comments are restored, with two corrections a review of the original found. They were in the wrong file. `buildItemFilterSql` is where the rule actually lives: both callers splice its clauses straight into query text, so only a placeholder index may ever be interpolated into one and every value must go onto `params`. That function's header said nothing about it, and it is where a seventh clause would be added. This matters more than ordinary comment placement because of how a cleared hotspot behaves. Reviewed/Safe stays marked and does not re-raise when a *different* file changes, so the one edit that would break this — interpolating a filter value in `itemFilters.ts` — was the one edit that would have got neither a warning nor a fresh marker. `items.ts` gets the same note. It builds `${PUBLIC_ITEM_SELECT} WHERE ${where}` from the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one. It also records why joining with AND cannot weaken `EXCLUDE_PENDING`: no fragment carries a top-level OR for the join to re-associate against. The wording was slightly false. "The single interpolation is `$${next}`" — the tags clause also interpolates `$${next + 1}`. Same category, so the argument is untouched, but a reader checking it literally finds a counter-example immediately, and a comment asserting safety cannot afford that. Two tests make the invariant fail a build rather than depend on being read. One feeds values built by hand rather than parsed — `"1); DROP TABLE items; --"` in every field — and asserts none of it reaches the clause text, which states directly that these literals are safe with no parser at all. The other asserts two disjoint filter sets produce byte-identical SQL, which catches a value that happens not to look hostile. Both were mutation-tested rather than assumed: interpolating `filters.minPriceCents` into the price clause — the precise edit the comment forbids — fails both, and one pre-existing test besides. Reverted, and the diff against main for `itemFilters.ts` is comment-only. Verified: backend build clean, 280 unit tests pass. Closes #202 Refs #180 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
7421676568 |
Merge pull request 'fix(email): stop a demo purchase telling real customers an item sold (#206)' (#211) from feature/206-no-demo-sale-emails into main
Reviewed-on: #211 |
||
|
bermudalambandClaude Opus 5
|
41840d4890 |
fix(email): stop a demo purchase telling real customers an item sold (#206)
A demo purchase called `notifyFavoritersOfSale`, which mails everyone who favorited the item through production's configured SMTP: "An item you favorited has been sold to another customer, so it is no longer available… this one will not be restocked." Nobody bought it and nobody is shipping anything, so both halves are false. It is also the only outbound consequence a demo purchase has — everything #195 and #203 fixed is on screen, in front of the person who clicked and who has now been told it is a demo. These recipients never saw the cart. They just get told something they cared about is gone, and while production runs the demo interim (#191) they are real customers on real SMTP. The demo route no longer notifies. The PayPal capture and webhook paths are untouched, because those are sales. The item is still marked `sold`, so the storefront stays truthful about availability and the favoriter who goes looking finds what the database says. Only the claim that somebody bought it goes away. That a demo purchase permanently consumes real production inventory is a larger question than this issue and is left alone. Removing the call broke two tests and quietly hollowed out three more, which is the more interesting half of this change. Five tests in `favorites.integration.test.ts` used the demo purchase as a convenient way to make a sale happen; with the notification gone, the two asserting mail *is* sent failed, and the three asserting it is *not* sent would have passed for the wrong reason for ever. They were always about who gets told rather than about the demo route, so they now call the notifier the way the PayPal routes do — after the purchase, with the sold ids and the buyer. `buyThenNotify` says so at the point of use. Route-level coverage is unaffected: the admin mark-sold path already had its own test, and the new test asserts the demo route notifies nobody. Both halves were mutation-tested rather than assumed. The new test fails without the fix. Dropping the buyer exclusion from `collectFavoriteRecipients` fails "does not tell the buyer their own purchase is unavailable" and "emails every opted-in favoriter except the buyer" — so the restored tests are guarding the logic again rather than passing on an empty inbox. Verified: 255 integration tests pass (the suite needs `--runInBand`; these share one database), 278 unit tests pass, backend build clean. Closes #206 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
a1948a50c9 |
Merge pull request 'docs(ops): stop the compose header contradicting itself, and name the four variables step 2 dropped (#204)' (#210) from feature/204-ops-doc-accuracy into main
Reviewed-on: #210 |
||
|
bermudalambandClaude Opus 5
|
70d04186b1 |
docs(ops): stop the compose header contradicting itself, and name the four variables step 2 dropped (#204)
Three corrections a review of #196 found, all in text #196 itself rewrote. **The compose header asserted something it falsified three lines later.** "All must be set in Portainer for this stack. All are secrets except DEMO_MODE" — but three PayPal entries are unused while `DEMO_MODE` is `true`, three more are marked Optional, and `SMTP_FROM` is not a secret. The runbook sends the operator to that exact block as authoritative, so an operator cutting over during the demo interim reads "all must be set", has no live PayPal credentials — which is the whole reason the interim exists — and either stops or invents a `BACKUP_PASSPHRASE`, which is how you get archives nobody can decrypt. The blanket claim is gone; each entry already says whether it is required and when. **Step 2 enumerated nine of the thirteen interpolated names.** `USPS_CLIENT_ID`, `USPS_CLIENT_SECRET`, `UPLOADS_BASE_URL` and `BACKUP_PASSPHRASE` were missing. The USPS pair is the one that matters, and it now gets a sentence of its own: losing it is the only silent failure in this step. Address validation is skipped when those are empty rather than failing, so checkout keeps working and quietly stops validating addresses, with no crash loop and nothing in step 7 that would notice. The list also now says to take everything the stack holds rather than working from the list, because an enumeration reads as a checklist however it is introduced. **The `DB_PASSWORD` failure was described wrongly, and its error names a variable the operator never typed.** It does not fail to authenticate against its data directory — the app never reaches a connection attempt, `checkAlwaysRequired` refuses at boot, and the message says `PGPASSWORD` because the compose file injects it as `PGPASSWORD=${DB_PASSWORD}`. An operator grepping for `DB_PASSWORD` finds nothing. That is now stated. The crash-loop examples were reordered to match the case the section claims to be about. It headlines "the likeliest outcome of a missed step 2", but the only block shown was the PayPal triple, which cannot occur during the demo interim, and #196 replaced it with a `DEMO_MODE`-only block that is not what a missed step 2 produces either. A wholesale miss is two problems led by `PGPASSWORD`; that is now first, with the `DEMO_MODE`-only and mistyped-value forms after it. Every quoted line was verified by running the real validator against production's entry set rather than by reading the source — `2 problem(s)` and `1 problem(s)` counts included — and all four now match character for character. The mistyped-value message is quoted in full rather than truncated, which was the point of the complaint that produced it. `DEMO_MODE` is no longer called "the only one that is not a secret", which was false of `SMTP_FROM` and `UPLOADS_BASE_URL`. It is the only one that is a setting rather than a credential, which is true and a better hook. Verified: 278 backend unit tests pass, including the compose guard that parses this file. Closes #204 Refs #196 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
6562208995 |
Merge pull request 'fix(orders): mark a demo order in the history rather than leaving it to read as real (#205)' (#209) from feature/205-demo-order-history into main
Reviewed-on: #209 |
||
|
bermudalambandClaude Opus 5
|
39c82ff3a4 |
fix(orders): mark a demo order in the history rather than leaving it to read as real (#205)
#195 and #203 made the cart say a demo order is a demo order. That message is an antd toast lasting about three seconds, after which the cart empties and the card unmounts. Order history is what the customer comes back to when they wonder where their item is, and it said nothing. A demo row was a real row: item name, `$80.00`, status `completed` rendered as a neutral tag because `STATUS_COLORS` has no `completed` key, and `demo` printed raw under a heading reading "Processor". That is not an explanation — a customer has no reason to read `demo` as "this did not happen", and "processor" is not a word they have any reason to know. Two things now say it, for the same reason the cart needed two. The `demo` cell renders as a tag reading "Demo (not charged)", which marks *which* order. A notice above the table, shown only when there is one, says what that means — a tag reading "Demo" still assumes the reader knows what a demo order is, and what they actually want to know is whether to expect a parcel. Nothing changes on the backend: `orders.processor = 'demo'` was already written at checkout and already selected for this page. The row is a real row in a real table and stays visible, because hiding it would be its own kind of lie — the customer did do something, and it did have an effect on the catalogue. Written test-first against a browser: the new case drives a real demo purchase through the cart, opens `/orders`, and failed on both assertions before the change. Verified: 4 end-to-end tests in this spec pass, the 5 existing order-history tests still pass, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #205 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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> |