530a2e14db5eda34230503402753b2eec3776e13
502
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
530a2e14db |
Merge pull request 'fix(tests): regenerate the schema mirror and name the controls #56 moved (#320)' (#321) from fix/320-post-brevo-test-fallout into main
Reviewed-on: #321 |
||
 synAdminandClaude Opus 5
|
5b40ac25db |
fix(tests): regenerate the schema mirror and name the controls #56 moved (#320)
PR #317 was merged while its run was still failing, so four failures landed on main: one integration and three end-to-end. All of them are consequences of #56, and none could have been caught on the dev machine, which has no database and no browser to run those suites with. The schema mirror was never regenerated. The migration added three columns to customers and src/db-kysely/schema.ts still described the table without them, which is the drift guard from #305 doing exactly what it exists for. Hand-edited to match what kysely-codegen emits — alphabetical, and Generated on the column that has a default — because regenerating properly needs a live database. The other three are the same mistake three times: a control addressed by position, and the position moved. An unscoped getByRole('checkbox') became ambiguous once the register form had two consents. A toHaveCount(2) on the account modal's switches became three. And favoriteAlertsSwitch was getByRole('switch').last(), which did not error when a switch was appended below it — it silently retargeted, toggled analytics consent instead of favourite alerts, and then failed on a text assertion in favorites.spec.ts, naming neither the file nor the control actually at fault. The reason position was ever used is that antd's Switch renders a bare role="switch" with no accessible name; the adjacent Text is a sibling, not a label. So each one now carries an explicit aria-label and is addressed by it. That is what makes them addressable from a test, and it is what a screen reader needed regardless — the fix and the accessibility improvement are the same change. The count assertion stays, but alongside naming each switch, because a count on its own would pass if two of them were swapped for each other. Two coverage gaps closed while here, both properties the compliance work in #56 depends on and neither previously asserted anywhere a customer could see: the analytics checkbox is unchecked on the register form, and the account toggle is off for a new customer. Quebec's Law 25 s.8.1 requires profiling to start off, the integration suite asserts the server half of that, and nothing asserted the half rendered on screen. The fourth Playwright entry, the logged-out header surviving a reload, is reported flaky rather than failed and passed on retry. Left alone; it is unrelated to #56 and #257 covers flakes in this suite. Verified: backend tsc clean, frontend tsc against the test config clean, production build green, both lint suites 0 errors, 478 unit tests passing. The integration and e2e suites still cannot run here, so whether this actually clears run 875's failures is for CI to say — which is the same gap that produced them. Closes #320 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
0ec6064391 |
Merge pull request 'test(e2e): make the resend allowance failure say why (#257)' (#319) from fix/257-resend-verification-flake into main
Reviewed-on: #319 |
||
|
synAdminandClaude Opus 5
|
218be6d298 |
test(e2e): make the resend allowance failure say why (#257)
The test asserted on toasts, and toasts were the wrong instrument twice over. It failed roughly one full-suite run in three with a strict mode violation — getByText(/already sent several/) resolving to three refusal toasts where one was expected — and two investigations could not establish the mechanism. The second investigation corrected the arithmetic the first depended on: antd toasts auto-dismiss, so the number visible at the moment of an assertion is a lower bound on how many refusals happened rather than a count. Three visible refusals is equally consistent with four where the first had already faded. That removed the only evidence anyone had for the original theory that the customer's bucket held two hits before the test clicked anything, which left the issue with a symptom and no way to read it. So this asserts the sequence of response statuses instead. Toasts are a lossy, timing-dependent rendering of the thing the test is actually about, and the responses are the behaviour itself. A failure now reports what happened: four 429s means the bucket really did carry hits from somewhere else, while more than four entries means the UI sent more requests than there were clicks. Either reading identifies the mechanism from one failing run, where before it needed a temporary probe re-added and the suite run until it failed again. Each click now waits for its own response. The previous "await expect(resend).toBeEnabled()" looked like pacing but was a no-op, since the button is never disabled, so four requests raced. Removing that ordering means a failure cannot be blamed on it. The copy assertion stays, because it is what the test is named for, but scoped with .first() so strict mode does not treat several identical toasts as ambiguous. This does not fix the underlying flake, and is not meant to. The issue asks for the mechanism to be found before a fix is attempted rather than guessed at, and nothing here changes the limiter or the store. Verified by typecheck and lint only. The e2e suite needs a database and a browser this machine cannot run, so whether this passes is for CI to say — the same gap that let the integration regression through earlier today. Refs #257 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
946ea4de1a |
Merge pull request 'feat(analytics): report consenting customers' activity to Brevo (#56)' (#317) from feature/56-brevo-tracker into main
Reviewed-on: #317 |
||
|
synAdminandClaude Opus 5
|
e7196f440b |
test(customers): let the public shape guard see analytics_consent (#56)
The register route now returns analytics_consent, and customers.integration.test.ts asserts the exact key set the public customer shape may contain. That test failed in CI, which is the guard doing its job rather than a problem with it: its whole point is that the shape cannot quietly grow, and a field appearing without someone deciding it belongs there is what it exists to catch. This field does belong there, so the expected set gains it. Three cases added while here, all of them properties the compliance work depends on and none of them observable from a unit test. Analytics consent is off for a registration that does not mention it, which is what Quebec's Law 25 s.8.1 requires and needs the column default, the register route and the stored wording to agree. Opting in to marketing alone leaves analytics off, which is the bundling GDPR treats as invalid and the mistake this branch already made once. And an analytics-only opt-in works with marketing left off, so the granularity holds in both directions rather than only the convenient one. Found by CI rather than locally: the integration suite needs a database this machine has no Docker to run, which was called out as unverified when the change went up. Typechecked, linted and the 478 unit tests still pass, but the assertion itself is only proven by the next CI run. Refs #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
25bec50902 |
docs(privacy): disclose every cookie and browser-storage item (#56)
Completes the consent picture the rest of this branch builds. The claim worth being able to check is that no cookie needing permission is set before it is asked for, so the policy now lists everything rather than asserting it: the rd_session sign-in cookie, which is strictly necessary and therefore exempt, the two preferences kept in localStorage and never sent anywhere, and Brevo's cookie, which cannot exist unless analytics consent was given because the script that would set it is never loaded otherwise. No cookie banner, and that is a finding rather than an omission. ePrivacy requires consent before storing anything non-essential, and this application does not store anything non-essential until the customer has asked for the feature that needs it. A banner would be asking permission for things that are either exempt or already separately consented to, which teaches people to dismiss the one consent that does matter. Described in terms of what each thing does rather than by category, since a list of cookie names tells a customer nothing they can act on. Refs #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
955049eac9 |
fix(privacy): separate analytics consent from email consent (#56)
The previous commit widened the marketing consent sentence to cover the Brevo tracker, so one checkbox carried both purposes. That is the specific pattern GDPR rejects: consent has to be granular, and current EDPB guidance treats bundling tracking consent with subscription consent as invalid because the customer cannot accept one purpose and refuse the other. Quebec's Law 25 s.8.1 is stricter again — profiling technology has to be off until the person switches it on, with no pre-ticked box and no consent inherited from agreeing to something else. Building to both standards was the decision, since the storefront is publicly reachable and anyone can register. So the marketing sentence is restored to exactly what it was, which leaves every existing email consent valid and untouched, and analytics gets its own column, its own sentence, its own checkbox at registration, its own toggle in the account page and its own endpoint. A customer can now hold either, both, or neither, and withdrawing one does not disturb the other. The migration defaults analytics_consent to false, which is both the honest answer — none of the existing customers was ever asked — and what Law 25 requires. Nothing about this change opts anybody in. Two details that are compliance requirements rather than wording preferences. The sentence names Brevo instead of saying "our email provider", because informed consent means the customer can tell who receives their data and a description they cannot act on is not disclosure. And the account toggle is as prominent and as easy to switch off as it is to switch on, because withdrawal has to be as easy as consenting. The analytics endpoint is separate from the marketing one rather than a second field on it, so that a single call cannot change an answer the customer did not touch — the bundling problem moved from the form into the API. The unit tests now assert the two consents stay apart in both directions, including that the marketing sentence still says nothing about tracking, because re-bundling them would otherwise pass silently and is the mistake this project already made once. Verified: backend tsc clean, both lint suites 0 errors with no new warnings, 478 unit tests passing across 33 suites, frontend production build green. Not verified: the migration has not been run against a database, and integration and e2e need a Node this machine does not have active. None of this is legal advice and the wording is worth a lawyer's eye before it ships. Refs #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
ac3f6e91f5 |
feat(analytics): report consenting customers' activity to Brevo (#56)
Loads Brevo's web tracker for a signed-in customer who has consented, reports route changes as page views, and tracks the three events the issue asked for: added_to_cart, favorited, and checkout_completed. The four design questions were settled on the issue in August and this implements those answers. The consent gate is the part worth reading. The decision recorded on the issue was "gate it behind consent", but the sentence customers actually agreed to named only email: "I want to receive occasional emails about new one-of-a-kind items". Gating a tracker on `marketing_consent` while that was the stored wording would have treated "email me about new items" as authorisation to send someone's browsing to a third party, which it does not say — and this project stores the wording verbatim against each customer precisely so that a record says what the customer saw. So the sentence is widened here, and the tracker is gated on `analytics_consent`, a field the server computes by comparing the wording stored against a customer with the current constant. Changing the sentence therefore does not retroactively widen anybody's consent: everyone who agreed to the old text keeps their email consent and is not tracked until they re-consent through the account page. A boolean alone could not tell those two populations apart, which is the whole reason the text is stored per customer. `analyticsConsent` is exported and has its own unit test, because "agreeing to the old wording does not authorise tracking" is the rule that silently tracks people if it regresses — their flag really is true. QA stays out of the live Brevo account by construction rather than by remembering. The key is per-environment, the tracker never loads without one, and `docker-compose.qa.yml` sets an empty literal with no stack variable behind it, so nothing can inherit a value from the host or be pasted in from production's stack. Same reasoning as QA_DB_PASSWORD and the QA_SMTP_ names beside it. Events are reported from the API layer rather than the UI call sites, so no caller can add to the cart or favorite an item without it being counted, and each fires only after the response was accepted — a refused add is not reported as one. The two checkout completions each name their processor, because a demo purchase charges nothing and counting it as a sale would overstate revenue. The privacy policy gains an analytics section in this change rather than a follow-up, since the published policy previously described none of this and would otherwise have lagged the code. It is deliberate about the limits: withdrawing consent stops further reporting, but anything already sent stays with Brevo, and a script already injected cannot be un-injected — `stopBrevoTracking` stops calls, it does not unload sa.js. That is said in the code too, because "tracking stops" reads as a stronger promise than any web tracker can make. Verified: backend tsc clean, both lint suites 0 errors with no new warnings, 474 unit tests passing across 33 suites, and the frontend production build green including the compose-environment guard. Not verified: integration and e2e, which need a database and a Node this machine does not have active, and no real Brevo key was exercised — the tracker has never been observed reporting to an actual account. Closes #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e0afa1b43e |
Merge pull request 'fix(admin): keep a modal's controls on screen on a phone (#314)' (#316) from bugfix/314-admin-modal-unclosable-on-phone into main
Reviewed-on: #316 |
||
|
synAdminandClaude Opus 5
|
2866901187 |
fix(admin): keep a modal's controls on screen on a phone (#314)
antd treats a Modal's width as a fixed pixel value and does not adapt it to the viewport, so the item editor's `width={720}` in Admin.tsx still laid out at 720px on a roughly 390px screen. The footer's OK and Cancel are right-aligned inside that width and the close X sits in the modal's top-right corner, so all three ended up off-screen, and antd sets `overflow: hidden` on the body while a modal is open, so the page behind could not be scrolled to reach them either. The dialog had no way out short of the browser's back button.
Capped in styles.css rather than at each call site, because `Modal.confirm` — used by the review queue when publishing at a price nobody chose — has no call site to edit and would have been left broken by a per-modal fix. The same rule covers the 640 and 480 widths in Customers.tsx and the preview drawer, all of which overflow a phone for the same reason. `max-width` beats the inline `width` antd writes on the element, so none of it needs `!important`.
Bounding `.ant-modal-body` rather than the modal is what actually keeps the buttons reachable: the footer is a sibling of the body, not a child, so a tall form scrolls inside the modal while the footer stays where it is. Sizing the modal alone would have moved the overflow rather than removed it.
Verified in the built bundle rather than only in source — `@media (max-width: 767px)` and all three rules are present in dist after a production build, which is the step that distinguishes a fix that shipped from one that merely compiled. Not verified visually on a device: this is a layout change with no unit coverage, and the five overlays worth checking at 390x844 are listed on the issue.
MDEditor's `preview="live"` still splits the pane in two and is cramped at this width. That is a behaviour change rather than a layout fix and is deliberately left out; it is recorded on the issue instead.
Closes #314
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
bermudalamb
|
39d59d55ce |
Merge pull request 'docs(ops): the free tier cannot be applied to this hostname (#285)' (#315) from feature/285-cloudflare-free-tier into main
Reviewed-on: #315 |
||
|
synAdminandClaude Opus 5
|
3e3932b231 |
docs(ops): the free tier cannot be applied to this hostname (#285)
The spike asked what Cloudflare's free plan would and would not do for a self-hosted site on a dynamic-DNS hostname. The answer is narrower than either option the issue anticipated: the free plan supports only full setup, which requires delegating the zone's nameservers at its registrar. The zone here is synology.me and belongs to Synology, so we cannot delegate it. Partial (CNAME) setup is Business or Enterprise, and subdomain NS delegation is Enterprise. The blocker is therefore not that the useful features are paid — it is that the hostname cannot go on the plan at all. The issue asked for exactly this kind of finding to be recorded rather than treated as a reason to widen the spike. The remaining questions are answered anyway, on the assumption that a domain we control is bought later, so that decision is made with the consequences already known rather than discovered afterwards. Two of those consequences are worth pulling out. Cloudflare appends to X-Forwarded-For rather than replacing it, so a second proxy in front of Nginx Proxy Manager makes `trust proxy: 1` in app.ts resolve req.ip to a Cloudflare edge address instead of the client. Four limiters key on req.ip and would silently stop distinguishing callers while continuing to look healthy, which is the dangerous direction for that failure to go. Separately, Bot Fight Mode cannot be skipped with WAF or Page Rules on any plan because it does not run on the Ruleset Engine, and Cloudflare documents that it may challenge API traffic — a challenged POST /webhooks/paypal is a capture notification we never receive. Everything asserted about this repository was checked against the code; the Cloudflare behaviour was checked against their documentation on 2026-09-05 and not against a live account, because one cannot be set up here. What was not established is listed at the end of the document rather than left implied. Closes #285 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
f99ac108bb |
Merge pull request 'fix(tests): close the route guard's factory hole and two regex warnings (#307)' (#312) from fix/307-sonar-cleanup into main
Reviewed-on: #312 |
||
|
bermudalambandClaude Opus 5
|
36e6057c6e |
fix(tests): close the route guard's factory hole and two regex warnings (#307)
Three of the items on the cleanup issue, and the first is the one that mattered.
routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed.
Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone.
The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added.
The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it.
An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
020f437326 |
Merge pull request 'docs(scripts): record the Node pin decision at the constant (#208)' (#310) from fix/208-node-pin-decision into main
Reviewed-on: #310 |
||
|
bermudalambandClaude Opus 5
|
936dcb60a8 |
docs(scripts): record the Node pin decision at the constant (#208)
The last open item on #208, and the only one that needed a person rather than a patch. Local runs stay pinned to 26.7.0 while CI and the production image run 20, and CI on 20 is the backstop.
The comment said this was "an open decision rather than an oversight", which was true when it was written and is not any more. Left as-is it would read to the next person as something still to settle, and they would either re-litigate it or quietly change the pin.
What the decision costs is written down rather than glossed: passing locally does not mean it ships, because a post-20 syntax or node: API is caught after a push rather than before one. That is the whole of the trade, and it is acceptable precisely because it is known — the failure mode this file's own docstring warns about is the one nobody knew they were exposed to.
Items 1, 2, 3 and 5 were already done in
|
||
|
bermudalamb
|
3cbf73ee87 |
Merge pull request 'Feature/308 kysely dynamic queries' (#309) from feature/308-kysely-dynamic-queries into main
Reviewed-on: #309 |
||
|
bermudalambandClaude Opus 5
|
abe8ac8184 |
test(filters): merge the duplicate itemFilters import (#308)
backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
306db81966 |
fix(db): narrow the status column instead of asserting the row (#308)
`.$castTo<AdminItemRow>()` / `.$castTo<PublicItemRow>()` at the two list-query call sites replaced the entire result type with an assertion rather than narrowing the one column that actually disagreed, which meant a projection that silently lost a column would still compile — exactly the failure mode this task exists to close, and the opposite of what the commit body claims. Fixed at the source instead: `itemSelect.ts` now defines `ItemsWithStatus`/`ItemDB`, narrowing `items.status` from the schema mirror's `Generated<string>` (a CHECK-constrained text column, so `kysely-codegen` has no literal union to give it) to `Generated<ItemStatus>`, and builds `ItemContext`, `adminItemQuery()` and `publicItemQuery()` from an `itemDb` typed with `ItemDB` instead of `db`/`DB`. Both `$castTo` calls and their comments are gone; the `AdminItemRow[]` / `PublicItemRow[]` annotations at the two call sites now check for real. Verified by temporarily dropping a column from `adminItemQuery`'s projection: the `AdminItemRow[]` assignment failed to compile as expected, confirming the guarantee actually holds. Also corrected two now-false statements left over from the conversion: `db-kysely/CONVENTIONS.md`'s worked-example section said `buildItemFilterSql` was "still raw `pg`" and that converting it "would put a second copy of a live function in `src/` that nothing calls" — both untrue since #308 shipped it as `itemFilterExpressions`. And two doc comments in `itemSelect.ts` still named the deleted `PUBLIC_ITEM_SELECT`/`ADMIN_ITEM_SELECT` constants instead of the functions that replaced them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ec1020891f |
refactor(db): build the item queries through Kysely (#308)
The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in. All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one. The second thing this buys may matter more than the first. pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error. The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one. Closes #308 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3248ac656e |
docs(db): plan converting the item queries to Kysely (#308)
One task, and that is a decision rather than a shortcut. itemSelect.ts, itemFilters.ts, both routes and the unit test file are coupled — the exports the routes call are the ones being replaced, and #298 put the test file under a tsconfig that type-checks it, so any partial commit is a red build. Every line of it was verified by probe against the real generated schema before it was written, not sketched. The projections type-check, jsonArrayFrom correlates through whereRef, the mixed array of sql templates and builder expressions composes under eb.and, and the emitted SQL is quoted in the steps so a wrong result is caught at the step that produces it rather than three steps later. The probe also settled the question the spec left open with a fallback: the row type is assignable to the hand-written contract, so no cast is needed. The two invariant tests are rewritten rather than ported. They used to read the clause strings the builder returned; they now compile the expressions against the same items-and-categories shape the real queries use and assert on the SQL Kysely emits, with the hostile value present in the parameters and absent from the text. Building a narrower query in the helper would have needed a cast, and a cast in that test would be testing the cast. The step that verifies the conversion is the one that runs the integration suite unedited. Those tests are the contract — same JSON, same ordering, same statuses — so the plan says plainly that a test needing an edit means the query changed behaviour and the query is what to fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
12e1616392 |
docs(db): design converting the two dynamic queries to Kysely (#308)
The work #305 made possible and deliberately did not do. These are the two queries the builder was ever wanted for: admin.ts and items.ts both splice a run-time-composed where clause into query text, and they are the only places S2077 has a real point after #294 hoisted the seven fixed-shape queries into named constants. They are safe today, and itemFilters.ts spells out why in sixteen lines — which is the problem, because a property that takes sixteen lines to explain is one an edit can quietly break. All four call sites convert rather than only the two flagged ones. The by-id constants carry no hotspot and are already safe, but they are built by interpolating the same projection strings the list queries use, so converting only the list queries would leave itemSelect.ts holding a Kysely builder and a raw string that must produce an identical projection — two spellings to keep in step by hand where the file's own header already warns about one. The filter builder returns an array of expressions rather than taking a query builder and returning it filtered, because the two callers do different things with the result: the storefront prepends its own not-pending clause and the admin route does not. A function that owned the builder would have to be told about that difference. startIndex disappears with the splicing it existed for. The aggregate subqueries become jsonArrayFrom, which emits the same coalesce(json_agg(agg), '[]') they hand-write today. That is the second thing this buys and it may matter more than the first: pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why itemSelect.ts's header says the selects and their types are kept in step by hand and the integration suite is the only thing that catches a drop. Afterwards that is a compile error. The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused on purpose: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to. The two invariant tests survive and get stronger. They currently inspect the clause strings the function returns; afterwards they compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim tested against the real artefact instead of an intermediate one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
3126e12fc0 |
Merge pull request 'Feature/305 kysely swap' (#306) from feature/305-kysely-swap into main
Reviewed-on: #306 |
||
|
bermudalambandClaude Opus 5
|
add28c5f16 |
fix(db): repoint the lint and drift guards at the new mirror (#305)
The eslint config's ignores list and comment still named the deleted src/db-drizzle/schema.ts and relations.ts and never named src/db-kysely/schema.ts, so the generated mirror was being linted for the first time and tripping sonarjs/redundant-type-aliases — exactly the trap the config's own comment already described from #261 and #217. The ignores list now names src/db-kysely/schema.ts and the comment is updated to match. The schema mirror drift test built one flat Set of every two-space-indented key in the whole generated file and asked only whether a live column name appeared anywhere in it, rather than checking it against the specific table it belongs to. Seventeen column names are declared on two or more tables and created_at is on fourteen of eighteen, so a migration adding created_at, updated_at, status, name, sort_order, token, or expires_at to a table that lacks it would pass vacuously. Replaced mirroredTables with mirroredColumns, which reads the DB interface to map each table name to its declaring interface and then reads that interface's own columns, and changed the column-mirroring test to look up columns per table. Verified the guard can actually fail: removing customer_id from the Carts interface made the test fail naming carts.customer_id exactly, and restoring the file made it pass again. The root .gitignore still carried a comment block and two patterns for drizzle-kit pull output under backend/src/db-drizzle, a directory this branch deleted along with backend/drizzle.config.ts. kysely-codegen writes only the single tracked file it's pointed at, so nothing replaces the rule — deleted the block and both patterns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ad5cb28e80 |
docs(db): stop calling a live query an unimported example (#305)
The paragraph describing buildItemFilterSql claimed "nothing imports it" but it is actually a live production function defined at src/itemFilters.ts:264 and imported by both src/routes/admin.ts and src/routes/items.ts. The conversion example in the conventions file was mistakenly described as though it were the function itself rather than as an example demonstrating the query pattern. Fixed the wording to clarify that the function remains raw pg code and the shown conversion is an example of how to convert it, not a committed version in src/ waiting to be called. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
aa3f75520b |
docs(db): write the Kysely conventions (#305)
Replaces the Drizzle conventions, and is much shorter, because three of that document's four warnings described the library rather than the practice and stopped being true when the library changed. An array is one bind parameter with no ceremony, a column reference in a raw fragment is the text you wrote, and the generated names are the database's own so nothing needs mapping back. What survives is what was never about Drizzle. The mirror is generated and refreshing it is manual, so the drift test is the thing that catches forgetting — and it exists because the drift already happened once and nobody noticed for a week. Both drivers share one pool, because a transaction on a second pool would be invisible to the first and the limits would silently double. Migrations stay hand-written, and the reasoning survives the change of library: only the expression-index complaint was specific to drizzle-kit, while losing the prose and being unable to express data migrations are true of any generator. One warning is genuinely new, and it is the inverse of an old one: driver errors are no longer wrapped, so a SQLSTATE sits on err.code again. That is worth stating precisely because it was not true before, and the last time it moved it turned a handled 409 into a 500 with nothing failing to compile. The worked example moved into this file rather than staying a source file nothing imports. It is documentation, and it was only ever documentation. Closes #305 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
731716f760 |
docs(db): correct the drift test's own stale references (#305)
The #217 doc comment above the describe block still named db-drizzle/schema.ts, drizzle-kit pull, and "Drizzle infers row types" — a file, a command, and a library this same commit had already removed. A comment pointing at deleted paths is worse than no comment at all on a test whose whole job is proving trust in a generated mirror, so it is corrected to name npm run db:types and src/db-kysely/schema.ts while keeping every sentence of the history intact: #217, #222, item_drafts and upload_links, the week nobody noticed. A closing note was added recording that the generator changed in #305 and the test did not, because the drift it guards is a property of generating a mirror at all rather than of any particular library. mirroredTables' regex also gets the same digit fix the column check already had. Both regexes parse the same generated file for the same kind of identifier, and a table name with a digit would otherwise be read out of the DB interface but reported missing by mirroredTables, sending someone to regenerate a file that was never wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c71b11e05e |
refactor(db): swap the query builder from Drizzle to Kysely (#305)
One commit, because a main that carries both builders is one where the next person converting a query has to guess which to reach for, and where two generated mirrors of one database can disagree. There was nothing to stage anyway: one file used the builder. The safety property that motivated adopting a builder at all is untouched, and was never the thing being traded. A value interpolated into a sql template becomes a bind parameter in either library, so #202's invariant stays a property of the type system and #180's hotspots retire either way. What changes is the three ways the old library made it easy to be quietly wrong, each verified in #297 against the SQL actually emitted: an array interpolating as a placeholder list unless every site remembered sql.param(), a column reference inside a raw fragment silently losing its table so a correlated subquery correlated with itself, and a camelCase mirror that had to be mapped back at every select or the JSON contract changed with no test noticing. CATEGORY_COLUMNS stops being a translation layer and becomes what it looks like — four column names four selects share. The generated types carry parent_id and sort_order because kysely-codegen emits the database's own names, so there is nothing left to map and nothing left to get wrong by forgetting to. The drift guard survives the swap rather than being rewritten, and loses its library name in the process: it is schemaMirror.integration.test.ts now, so the next such change renames nothing. It also got stricter for free. The Drizzle version had to match each column two ways and its own comment called that deliberately loose; a generated Kysely interface spells the database's name verbatim as a bare key, so one exact match is the whole rule and snakeToCamel is gone. isUniqueViolation keeps accepting both error shapes and now has a test behind it. Kysely uses the pg driver directly and should leave the SQLSTATE on err.code, but "should" is the word that turned two 409s into 500s when the last conversion moved it to err.cause.code with nothing failing to compile. Migrations are untouched. #219 stands, they remain hand-written node-pg-migrate files, and Kysely has no generator to refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
22c51b6af7 |
docs(db): plan the Kysely swap (#305)
Two tasks. The first is the whole swap in one commit — dependencies, generated types, db.ts, the reconverted file, the ported drift test and the new 409 assertion — because splitting it would put a commit on the branch where the build is broken or both builders are present, and neither is a state worth being able to bisect to. The second is the conventions document, which touches no code and is much shorter than the one it replaces. The plan carries the converted adminCategories.ts in full rather than describing it, and names the two places the conversion could silently change behaviour: the four selects must keep answering id, name, parent_id, sort_order and item_count, and the unique-violation catch must keep producing a 409. The existing category integration suite is the gate on the first, and a new test is the gate on the second. Three expected outputs are written down so a wrong one is caught at the step rather than three steps later. Codegen must report 18 tables, not 19 — 19 means pgmigrations leaked past the exclude flag. The generated Categories interface must spell parent_id and sort_order, because camelCase there means --camel-case got turned on and the mapping layer this swap removes has come straight back. And the integration count should rise by exactly one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
4f8b15cafa |
docs(db): design the Kysely swap (#305)
Carries out what #297 decided. Migrations are untouched — #219 stands, they are hand-written node-pg-migrate files, and Drizzle was never doing them. Both builders must not coexist at any commit. A main that carries Drizzle and Kysely together, even briefly, is one where the next person converting a query has to guess which to reach for and where two generated mirrors of one database can disagree. There is nothing to stage anyway: one file uses the builder. Three things the swap gets for free, recorded so they are not mistaken for scope creep. The worked example stops being a source file — itemFilters.drizzle.ts was never imported by anything, so it was dead code in src/ that only documentation justified, and its replacement belongs inside CONVENTIONS.md where a worked example goes. The drift test loses its library name, becoming schemaMirror.integration.test.ts, so the next such change renames nothing. And that test gets stricter rather than merely ported: the Drizzle version had to check every column two ways and its own comment calls that deliberately loose, where generated Kysely types emit the database's names verbatim and the check becomes one exact match. CATEGORY_COLUMNS disappears rather than being translated. It exists only because Drizzle's mirror is camelCase while the API answers snake_case, and its comment says selecting the table directly would silently change the JSON contract with no test noticing. With the generated types carrying parent_id and sort_order the mapping object has nothing left to do, which is the clearest single illustration of what the swap buys. isUniqueViolation keeps tolerating both error shapes and gains a test that proves which one actually arrives. Kysely uses the pg driver directly and is expected to leave the SQLSTATE on err.code, but "expected" is the word that turned two 409s into 500s last time. Closes #305 is deliberately not claimed here — this is the design, and the implementation follows on the same branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
a74e950772 |
Merge pull request 'docs(db): weigh Kysely against the Drizzle decision (#297)' (#304) from feature/297-kysely-vs-drizzle into main
Reviewed-on: #304 |
||
|
bermudalambandClaude Opus 5
|
7ca55eaa1a |
docs(db): weigh Kysely against the Drizzle decision (#297)
The question is not whether Kysely is good, it is whether it is enough better for this codebase to reverse a decision already made in #216 and partly built in #217. That is a higher bar than being the nicer library, so this answers it against the same target #216 used: buildItemFilterSql, with six optional clauses composed at run time, a recursive CTE, an ANY(...::int[]) tag match with a count equality, and array parameters. Kysely compiles without a connection, so the document quotes the SQL it actually emitted rather than a reading of its documentation. Three of the four hazards that src/db-drizzle/CONVENTIONS.md exists to warn about turn out to be properties of Drizzle rather than of type-safe query building, and two of them are the silent kind. An array interpolates as one bind parameter with no sql.param() ceremony, so the trap that document calls "the rule that will bite you" does not exist. A column reference inside a raw fragment is the text you wrote, so the correlated-subquery rewrite that returned a quietly wrong count in #218 cannot happen. And the generated types carry the database's own snake_case names, so the explicit column mapping that exists to stop a select silently changing the JSON contract is not needed at all. The property that motivated the whole exercise is unchanged: a hostile value lands in the parameters either way, so #202's invariant becomes a type-system property and #180's hotspots retire either way. What decides it is how little is actually built. One file is converted — adminCategories.ts, three calls — against 238 raw query sites, and the generated mirror and its drift test are things any builder needs an equivalent of. The recommendation is to switch now, while the cost is reconverting one file and rewriting a conventions document that gets substantially shorter. The counter-argument is recorded rather than hidden: Drizzle is more widely used, and #219's migration reasoning was measured against drizzle-kit specifically. That reasoning survives, because losing the prose and being unable to express data migrations are true of any generator, and Kysely simply has nothing to refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e437584d7e |
Merge pull request 'Feature/301 rotate photos' (#303) from feature/301-rotate-photos into main
Reviewed-on: #303 |
||
|
bermudalambandClaude Opus 5
|
0bdfd100e8 |
fix(admin): re-request a rotated photo even when the turn failed (#301)
Linting / lint (pull_request) Failing after 0s
The turn handler only bumped the cache-busting version on the success path, so a failure between the backend's two writes (displayed file rotated, then pristine original rotated) left the admin looking at an error toast next to a photo whose src string had not changed and whose bytes the browser still served from cache — even though the displayed file on disk had already turned. The design's stated mitigation for this failure, that the admin can see the photo moved and press back once, depended on the browser re-requesting the file regardless of outcome. Moving setVersion(Date.now()) into the finally block, alongside setTurning(false), makes that re-request happen on both the success and failure paths, so the failure is now visible and recoverable the way the design intends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
dd533ffd63 |
feat(admin): rotate a photo from the review queue (#301)
Two icon buttons under every thumbnail, and the client for the item-scoped endpoints behind them. Icon-only with an aria-label rather than visible text, because three labelled buttons under a 120px thumbnail is more furniture than the photo — and a button with no text has no accessible name at all without one. They are not gated on the background-removal flag. That flag is about the sidecar, and rotation has nothing to do with it: turning a photo is a local file operation that works in every environment, including one where REMBG_URL was never set. The cache-busting src is the part most likely to have shipped broken. Rotation does not change image_path, so after a successful turn the src is byte-for-byte the string the browser already holds a copy for, and the photo would appear not to have moved. express.static is mounted with no maxAge and would serve the new bytes on a full page reload, but nothing in a session asks it to. A version held in component state is what makes the button visibly do something, and it needs no column and no server change, because the file's identity has not changed — only this page's need to see it again. The client lives in its own module rather than in draftsApi, whose send() hardcodes the item-drafts prefix these routes deliberately do not use. The inventory editor imports this same module unchanged when it follows, which is the whole reason the endpoints went on the item. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3891f4fd75 |
feat(admin): endpoints to rotate one photo of an item (#301)
Two POST routes and the module behind them. They live on the item rather than on the draft, and that is the decision that makes the inventory editor free when it follows: an image belongs to an item whether or not a draft row exists, so the second screen to want this is the same call from a different place with no new backend at all. A cut-out and its pristine original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then quietly un-rotate the photo, so the undo of one feature becomes a regression of another. 204 rather than 200. Rotation changes no column: the paths are identical afterwards and only the bytes differ, so there is no row worth returning, which is the same reason deleting an image is already a 204. Only "not on this item" is a 404, and it is indistinguishable from an absent id on purpose, because an image id is a serial and this endpoint should not confirm which ones exist. Everything else stays loud as a 500, and the file is untouched in every one of those cases — rotateInPlace renames over the original only once the new file has been written. One asymmetry is recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a failure between the two files turns the displayed one twice. That needs the disk to break between two writes, and the remedy is one press in the other direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3659608abe |
test(images): release the sharp handle before teardown (#301)
libvips caches input file mappings in memory after a pipeline finishes. On Windows, this keeps an open handle on the input file, and the OS refuses to delete a file with an open handle. The animated-WebP test triggers a pipeline rejection (correctly refusing a multi-page rotation), so the mapping stays in cache and afterEach cannot remove the test directory. Disabling the cache costs these tests nothing: each file is read exactly once during its test, so there is no reuse to cache. With caching disabled, Windows can delete the input files and afterEach succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5c0c7186ff |
feat(images): turn a stored photo a quarter turn (#301)
The file half of the remedy for what #300 could only stop. Fixing the EXIF strip means new uploads arrive the way the sender saw them; it cannot repair what is already stored, because the tag that said which way up the pixels went is gone. Those photos need a person to look at each one and turn it. Rewrites the pixels rather than recording an angle, because an angle obliges every consumer to honour it — the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar — and any one that forgets shows the photo sideways. The sidecar is not ours to teach. Left is anticlockwise and right is clockwise, which is rotate(-90) and rotate(90); sharp reads a positive angle as clockwise. The direction test asserts a pixel rather than a dimension, because dimensions swap whichever way the turn goes — a reversed sign would pass every size assertion and ship a control that does the opposite of its label. The animated WebP case is the one that could destroy someone's file quietly. Reading such a file without the animated flag succeeds and hands back the first frame alone, so a rotation that omitted it would write a still back over the animation and report success. Passing the same flag reencodeInPlace passes makes sharp refuse instead — multi-page images turn only by 180° — which is the honest answer and leaves the file untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
df508b3783 |
docs(admin): plan the photo rotation work (#301)
Three tasks, each with its own test cycle: the file operation, the endpoints and the module behind them, then the control in the review queue. Two things were probed rather than assumed while writing it, and both changed the design. sharp reads a positive angle as clockwise, so left is rotate(-90) and right is rotate(90) — and the direction test asserts a pixel rather than a dimension, because a rectangle's dimensions swap whichever way the turn goes and a reversed sign would pass every size assertion while shipping a control that does the opposite of its label. And a quarter turn of an animated WebP is refused by sharp itself, which is what makes passing the same animated flag reencodeInPlace passes the safe choice: omitting it would read the first frame alone and write a still back over someone's animation while reporting success. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
1178129110 |
docs(admin): design rotating a photo from the admin (#301)
The remedy for what #300 could only stop. Fixing the EXIF strip means new uploads arrive the way the sender saw them; it cannot repair what is already stored, because that metadata is gone and the originals kept for #281's cut-outs were re-encoded on the way in too. Every portrait photo since #226 needs a person to look at it and turn it. Rotation rewrites the file rather than recording an angle. Storing an angle keeps the bytes pristine and makes undo exact, but it puts an obligation on every consumer — the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar — and any one that forgets shows the photo sideways. The sidecar in particular is not ours to teach. Rewriting means nothing else in the system has to know rotation exists, and the cost is bounded: one rotation is a second generation at quality 82, which is why the control offers both directions rather than making somebody press one button three times to undo. Per photo, and that is deliberately the opposite of what #293 decided for background removal. The reason there does not carry: three photos of a vase can each be wrong in a different direction, so turning them together would fix one and break two. A cut-out and its original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then silently un-rotate the photo, turning the undo of one feature into a regression of another. The endpoints go on the item rather than the draft, so the inventory editor needs no backend work at all when it follows: an image belongs to an item whether or not a draft row exists, and the second screen is then the same call from a different place. Two things pinned so they are not settled by a coin-flip while implementing. Left is anticlockwise and right is clockwise, which is sharp.rotate(-90) and sharp.rotate(90) — sharp reads a positive angle as clockwise, so the sign is the whole mapping and reversing it produces a control that works and does the opposite of its label. And the displayed image has to be forced to reload: rotation does not change image_path, so the img src is identical afterwards and the browser keeps what it has. express.static is mounted with no maxAge and would serve the new bytes on a page reload, but nothing in a session asks it to, so the src gets a cache-busting parameter after a successful turn. One honest asymmetry recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a half-done failure turns the displayed file twice. That needs the disk to break between two writes, and the remedy is one press in the other direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
2ee0f2b35a |
Merge pull request 'fix(uploads): apply the EXIF orientation before discarding it (#300)' (#302) from fix/300-apply-exif-orientation into main
Reviewed-on: #302 |
||
|
bermudalambandClaude Opus 5
|
d3ccc1b1f6 |
fix(uploads): apply the EXIF orientation before discarding it (#300)
Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them. A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not. The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis. Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail. The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time. This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301. Closes #300 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
522b8f1f74 |
Merge pull request 'fix(lint): bring the backend test suites into scope (#298)' (#299) from fix/298-lint-backend-tests into main
Reviewed-on: #299 |
||
|
bermudalambandClaude Opus 5
|
0b6cc85c4f |
fix(lint): bring the backend test suites into scope (#298)
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
1683fbbf0f |
Merge pull request 'Feature/293 remove backgrounds from inventory' (#296) from feature/293-remove-backgrounds-from-inventory into main
Reviewed-on: #296 |
||
|
bermudalambandClaude Opus 5
|
b70e4a68f0 |
docs(specs): match the design to the shipped behaviour (#293)
The spec asserted two things the implementation disproved. RestoreSummary was described as having no `failed` because "restoring cannot fail the way removing can" — true about the sidecar, wrong about the database, and rethrowing turned a partial success into an opaque 500. And the single-button-with-two-labels rule was described as deliberately covering the mixed case, when in fact it stranded it: a partly cut-out item offered only Remove, so its existing cut-outs had no way back. Both sections now describe what the code does and why, including why Restore is not gated on the feature being configured, and the outcome table's "feature not configured" row is corrected to say Restore is still offered and still works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
64f8efb617 |
fix(admin): offer Remove and Restore independently (#293)
One button whose label flipped on "is every photo cut out?" could not serve a partly cut-out item, which is not a hypothetical state: it is what a partial removal leaves behind, and it is also what happens when REMBG_URL goes away after some photos were already done. In that state the single button read "Remove backgrounds", so the cut-out photos the item already had could never be restored from this screen. Remove and Restore are now separately gated and can appear together, which is correct — Remove finishes the job on what is left, Restore undoes what is already done. Restore is deliberately not gated on the backgroundRemoval config flag. Gating it would strand cut-out photos with no way back in exactly the environment that most needs the undo. Remove stays gated, so an unconfigured environment shows no button rather than one that reports zero of four done every time. The emptiness check moves from `!== null` to `!= null`: original_image_path is optional on the shared Item type because the public storefront response omits it, so a stray undefined has to count as "not cut out" — `undefined !== null` is true, which would misread a public-shaped item as fully cut out. The modal now refreshes on a non-ok response too. A restore that fails partway can still have swapped some files back before it failed, so returning early left the thumbnails showing files that are no longer on the server. The warning text is now driven off whichever count the action reports, so a partial restore says how far it got the same way a partial removal already did. The e2e spec seeds its item into a category of its own and filters the table down to it. The inventory table paginates at 10 and the suite runs fullyParallel, so an unfiltered page one was never a reliable place to find the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
445c9c4a22 |
fix(backgrounds): report a partial restore instead of throwing (#293)
restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had. The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved. The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
dbb63bc3d2 |
fix(admin): keep the active inventory filter and surface background-swap failures (#293)
handleBackgrounds re-read /api/admin/items unfiltered and called setItems(all) after every remove-backgrounds or restore-originals call, so an admin who had filtered Inventory to one category and opened an item from that filtered view saw the table silently repopulate with the entire unfiltered catalogue the moment the request resolved. Every other mutation in this file goes through load(), which respects the active filters; this one didn't, for no reason the spec required. The fix reuses load() instead: it now hands back the rows it fetched (previously discarded after setItems), and handleBackgrounds picks the edited item's fresh row out of that filtered result to refresh the open modal, rather than issuing a second unfiltered fetch. There is no GET /api/admin/items/:id route to fetch a single item directly, and the remove-backgrounds/restore-originals routes return only a summary, not the item, so load()'s own result is what's actually available. A background swap never touches the fields anything filters on, so the edited item stays in the filtered result whenever it was in it before. Also added a catch to handleBackgrounds, matching the message.error shape every sibling handler (handleDelete, handleDeleteImage, handleStatusChange) already uses — previously a network drop or a malformed JSON body became an unhandled rejection with no toast, silently different from how the rest of the file reports failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |