main
128
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
3958bda489 |
fix(auth): offer Google on the sign-up tab, not only on Log In (#345)
The button was rendered inside the Log In tab's form only, so a visitor on Create Account saw no social option at all. Reported from a local run. The mistake came from following the passkey button too closely. A passkey belongs only on Log In, and correctly so: you cannot register an account with one, since registration requires an account to register it against. Google is the opposite case. Creating an account is precisely what a new customer reaches for it to do, so leaving it off the sign-up tab hid the feature from the people it helps most — and hid it on the tab the modal opens on by default. The label differs by tab and nothing else does. One endpoint serves both: it signs in a known identity, links a verified address, or creates an account, and the customer neither knows nor cares which will happen. So the wording matches what they came to that tab to do rather than what the server ends up doing. Both spellings are given in Google's identity guidelines alongside the mark. The two consent checkboxes above it are deliberately not carried across. Google takes the customer off this site entirely, and a tick that survived that round trip would be a consent recorded from a form nobody submitted. They are asked again, with the same wording and through the same endpoints, on the step they land on afterwards — which is what #342 built that step for. The end-to-end test asserts absence, like the one beside it, because local and QA have no credentials and absence is the behaviour that actually runs there. Verified: frontend tsc, lint and build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
c2ee0b0c5d |
fix(e2e): match an email template tab on its whole name, not a prefix
CI failed on a test nobody had touched:
strict mode violation: getByRole('tab', { name: /Email address changed/ })
resolved to 2 elements
The locators in AdminEmails built an unanchored regex from the label, so a template whose name merely began with another's matched both. #337 added "Email address changed by the shop" alongside "Email address changed" and broke the assertion above it.
The failure named the assertion rather than the new template, which is what made it worth more than a rename. It is the same shape as the switch locator that silently retargeted in #317: a loose locator that keeps passing until something new is added nearby, and then fails somewhere that says nothing about the cause.
So the fix is the locator rather than the label. One helper now builds a matcher anchored at both ends, allowing only the optional "Customised" suffix a tab carries once its template has been edited, and escaping the label because these are copy and copy acquires brackets and full stops eventually. Both railTab and customisedTab go through it, and callers pass plain strings instead of assembling regexes at each call site.
The new template is added to the list the test walks, which is what it should have had in #337.
Verified the matcher against every real label plus the pairs that collide, including that a customised-only match still rejects an unedited tab. Frontend tsc and lint clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
synAdminandClaude Opus 5
|
2c6ac4d2be |
feat(auth): offer Google sign-in on the login form (#345)
The last of the six, and the first a customer can see. The auth form is the single sign-in implementation rendered by both the route modal and the cart prompt, so the button goes in one place and appears in both. Below the passkey button, which is below the password form. The order is deliberate and it is not about preference: a passkey is already on the device in front of the customer, while Google is a round trip to somebody else's site, and passwords are how every existing customer signs in. Each step down that list asks more of the person using it. Absent rather than disabled where it is not configured, which is the same call #41 made for a browser without WebAuthn. It matters more here, because being unconfigured is the normal state rather than the exception: local development has no credentials, and QA cannot have any until #313. The storefront advertises a boolean through the existing public config, never the client id — the browser has no use for one, since the whole flow is a redirect the server builds. Google's mark is inlined as SVG with their published colours and geometry. A hand-drawn approximation of somebody else's trademark is a compliance problem rather than a style choice, and a second origin on the sign-in path is a second thing that can be down. The button is a navigation rather than a fetch, which makes it unlike every other control on that form. The flow leaves the application entirely, so there is no promise to await and no error to catch — the callback decides and redirects. Where to return to is supplied by the caller, because only the caller knows. The route modal renders over a backdrop location and its own path is /login, so reading the current URL there would send the customer back to the form they just left; the router builds it from the backdrop instead. The cart prompt uses the page it interrupted. It cannot resume the interrupted action the way onSuccess does — the redirect leaves the app — so the customer lands back on the page and presses the button again. That value is validated on the server and not in the browser. It has to be, since anyone can type the URL, and doing it in one place beats doing it twice in two languages. The end-to-end test asserts the button is ABSENT, which is the behaviour local and QA actually have, and then signs in with the password form to show that its absence changes nothing. That is the point of putting the alternatives below rather than above. docs/ops/google-sign-in.md records what has to be true outside the repository: the seven sections of the Google Auth Platform, the three scopes that keep publishing out of a verification review, the cutover checklist for #313, and the production smoke test. It states plainly that QA on the Synology hostname is impossible rather than merely unconfigured, because Google will not accept a redirect URI whose domain nobody can prove they own — the same wall #285 hit with Cloudflare. The failure that document warns about hardest is leaving the consent screen in Testing. Only listed test users can then sign in, the refusal happens on Google's own page, and nothing reaches the storefront at all — so a customer reports a broken button and the logs are silent. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration and end-to-end suites need a database this machine has no Docker for. Closes #345 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
dcb3c7c91b |
feat(auth): the routes that assumed every customer has a password (#344)
The first accounts in this project's history have no password. Several things that were true stop being true, and one check written a month ago finally becomes reachable. Setting a first password and changing an existing one stay one route. A customer who signed up with Google cannot supply a value that was never set, so asking for one is a dead end; what authorises the change is the session they are already holding, which is what authorises every other setting on the account page. Two routes would be two places to get the guard wrong, and the one that would be forgotten is whichever is not on the path exercised by hand. The branch reads the stored hash rather than anything the caller sends, so a request cannot talk its way into the first-password case by omitting a field — there is a test for exactly that. Changing the email address is refused instead, and the asymmetry is the point. Setting a first password changes a credential the customer already controls. Changing the address changes where recovery goes, and whoever holds the new one can reset the password and own the account outright. That is why the route has always demanded more than a live session, and dropping the demand for the accounts that cannot meet it would remove the protection from exactly the ones that need it. The message says the real thing and names the way out, rather than claiming a password was wrong when there is none. Login is left exactly as it was. Answering "this account has no password" to a submitted address would turn the form into an oracle for which customers use Google, so it keeps the single refusal and the account page is where a signed-in customer learns what they have. Two tests pin that, including the one where both the supplied password and the stored hash are empty — the combination most tempting to call a match, and the one that would let anyone sign in as any Google-only customer. Deletion needed nothing, because it never asked for a password. That corrects what #332 recorded, and there is now a test so it stays true. The passkey lockout guard runs for the first time. It was written in #40 against the condition rather than the schema and has been unreachable ever since, because password_hash was NOT NULL. Three tests exercise it now: refused when it is the only way in, allowed when a second passkey remains, allowed once a password has been set. Two things about password reset were worth checking rather than assuming, and both turn out to be right as they stand. A customer who never had a password can still reset one, which is what somebody reaching for "forgot password" was asking for. And a reset still removes every passkey, per #42, because nothing about that path identifies who asked. What it does not do is sever the Google identity, and that asymmetry is deliberate: a passkey is a credential this shop issued and can revoke, while a Google identity is one Google holds, and cutting it would leave the customer unable to use the button they signed up with for no gain — whoever completed the reset controls the mailbox either way. The account page is told whether a password exists, and nothing more. Offering to change a password to somebody who has never had one is a dead end; saying nothing leaves them unable to see a credential they are entitled to manage. So the panel is titled for what it does for this customer, the current-password field is absent rather than disabled, and the confirmation says they can now sign in with it as well as with Google. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. Closes #344 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
44df0bd2d9 |
feat(auth): link a Google identity to an account that already exists (#343)
The smallest change in this feature and the one to read most carefully. It is the point where somebody who has proved nothing to this shop is handed an account belonging to somebody who did. The rule is one line at the top of linkIdentity.ts: link only when Google asserts the address is verified, and refuse otherwise. Everything below it is bookkeeping. That is defensible for Google specifically, and the reasoning is worth stating rather than assuming. Google asserting the address means whoever completed the sign-in demonstrably controls the mailbox, and that mailbox is already the root of trust for every other route into the account — it is where a password reset goes, and following a reset link takes the account over completely. So linking on it grants nothing that was not already reachable, and it spares the customer who came to Google precisely because they forgot their password. Never on an unverified address. That is not a weaker version of the same thing; it is an account takeover with extra steps, because the assertion would be one nobody checked. There is a test for the specific trap: the string "false" is truthy, and if that check ever becomes a truthiness test then every unverified Google account links to whatever account holds its address. The order matters and is an order rather than a set of independent checks. The identity lookup runs first and nothing else is consulted when it matches, which is why an identity that has signed in before keeps working after the address changes on either side. There is a test where a second customer has since taken the address the Google account reports, and the sign-in correctly reaches the first. Linking to a disabled account is refused, and the reason is not obvious. Linking and then refusing the session would leave the identity attached, so the next attempt would take the sign-in path instead — turning a disabled account into one that is merely inconvenient to reach. The refusal gets its own destination rather than the generic failure. It is the one refusal in this flow a customer can act on: they have an account and simply cannot reach it this way, so the login form now says to use the password they already have. That reveals nothing, because they arrived holding a Google account for that address — being told the address has an account here tells them only about themselves. Deciding this in newCustomer.ts, where the unique constraint already fires, was the shape to avoid. An account must never be handed over as a side effect of an INSERT failing, so that module reports the address is taken and stops, and the policy lives somewhere it can be read on its own. Automatic linking is defensible but it is not obvious, so the account page now shows it. A customer who signed up with a password and later used Google has had two credentials joined without being asked, and a silent link is indistinguishable from a bug when they later wonder why the password is no longer needed. It sits beside the passkeys for the reason that list exists at all: a customer cannot manage credentials they cannot see. The endpoint never returns the provider subject, which is the same reasoning that keeps credential ids out of the passkey list. No unlinking. Removing the only way into an account is the question #344 settles, and offering that button before the check runs would be the fastest possible way to lock somebody out of their own orders. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. Closes #343 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
25078417e5 |
feat(auth): create an account from a Google identity, then ask about consent (#342)
A Google account nobody here has seen now becomes a customer. The OAuth part of this was the easy half; the problem worth the issue is consent. Registration asks for two consents and stores their wording verbatim, and marketing consent must start unticked. Somebody arriving through Google has never seen those checkboxes and could not have, because the redirect happened before anyone knew whether they were new. Creating the account with both false is legally correct: nobody agreed to anything, and nothing is recorded as though they had. There is no stored wording either, because a wording saved against a false consent is a record of a conversation that never happened. But stopping there would mean a Google sign-up is never asked at all, and a silent no is still a decision made on somebody else's behalf. So the account is created, the customer is signed in, and they land on a step that shows the same two sentences with the same two unticked boxes. It saves through the endpoints registration already uses, which is what keeps the stored text byte-identical rather than merely similar. Not now is offered as an equal option, because consent has to be as easy to withhold as to give, and both can be changed later from the account page. The wording on that screen is imported from the shared constants rather than retyped. Three different wordings were already in circulation once before that was shared, and the record is meant to say what the customer actually saw. The return path is deliberately dropped for a new customer, who lands on the consent step instead. Carrying it through as a query parameter was the alternative and was rejected: the consent page would then redirect somewhere a URL told it to, which is the open-redirect question already answered on the server, asked a second time in a second language on a page an attacker can link to directly. One new customer occasionally landing on the storefront rather than back at their cart is much the cheaper of the two. The customer and the identity are inserted in one transaction. A customer row with no identity is an account nobody can sign in to and nobody can recover, because it has no password either. Signing up is refused when the address already belongs to a customer. Joining those two accounts is linking, it is the most security-sensitive decision in this project, and it belongs to the next issue rather than falling out of an INSERT here. Refusing is the safe half of that decision and the only half available until the policy is written down. The unique index rather than the preceding SELECT is what actually holds when two sign-ins race, so losing that race is treated as the address being taken rather than as an error. Google's assertion about the address is taken only when it is the boolean true. When it holds, the account is marked verified and no confirmation email is sent, because that email exists to prove the customer receives mail at the address and Google has just proved exactly that. When it does not, the account is unverified and goes through the ordinary confirmation, because an unverified assertion is worth nothing. Names from the profile are hints. Registration demands both because every email greets by first name, but Google may return neither and refusing a sign-in over it would be absurd — the greeting already has a fallback for exactly this case. The tests worth reading are the two about a returning customer. One signs in again and reaches the same account; the other changes their Google address first and still reaches it. That second one is the whole reason the identity is keyed on the subject claim: an email match would have created a second account there, and an address that had since been reassigned would have handed the first one to a stranger. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. Closes #342 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
90e372d6bd |
feat(admin): move a customer's account to an address they can reach (#337)
The third step of the only recovery route a customer who has lost their mailbox has. The first two are contacting the shop and being verified against order history. The third had no implementation, so the answer was a hand-written database edit that left no record of who did it or why. The thing to say plainly, because everything here follows from it: this operation and an account takeover are the same operation. They differ only in whether the verification was sound, and nothing in the software can check that. What the software can do is make the change recorded, announced, and complete in its effects. Recorded. The endpoint refuses without a written reason, and the reason is stored against the account. That row is the only thing that tells a genuine recovery from a takeover afterwards, which is why a hand edit was never acceptable and why the field is required by the server rather than merely collected by the form. It is never shown to the customer: it is a note about how somebody was verified and can name things the customer should not be handed back. There is no column for who did it. Admin access is one shared gate secret in front of a single operator, so such a column could only ever hold a constant, and a constant dressed up as an identity is worse than an honest absence. Announced, to the address being replaced. If the recovery was sound that reaches nobody and costs nothing. If it was not, it reaches the real owner, who is the only person in the world who can say so, and that is the only reason this endpoint is safe to have at all. Its own template rather than the self-service one, because that copy says to contact us if you did not make this change, and here somebody already did — the sentence would be addressed to the customer who just did the thing it asks for, while the person who needs to act on it did nothing. The new address is marked unverified and sent a confirmation link. Somebody reading an address out over the phone has not demonstrated they can receive mail at it, and that is the commonest way this goes wrong harmlessly. Complete in its effects. The move signs the customer out everywhere, removes every passkey, and cancels reset links already sent. That is the conclusion #42 reached for password reset, and it applies here with more force: somebody the system cannot identify asked for this change, so a session or a credential surviving it is one the new owner cannot see and cannot revoke, and a reset link sitting in the mailbox being taken away would let whoever still reads it take the account straight back. The password is left alone. What the customer lost was the mailbox, so demanding a new one adds a step for no gain. The verification-email helper moved out of the customers route into its own module, for the reason session creation moved out for passkeys: two implementations that agree today are two that can be changed one at a time, and the one that gets forgotten is whichever the manual testing does not exercise. This path runs perhaps once a year, so it is exactly the one that would rot. The admin drawer gains the action next to the address rather than among the account controls, because it is a thing done to that field by someone already looking at it. It leads with the warning instead of burying it. The history of moves sits on the same drawer and renders nothing at all for the overwhelming majority of customers, who have never been moved. Verified: backend tsc clean for src and tests, 526 unit tests pass, lint clean apart from warnings that predate this branch; frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. It also cannot be proven by CI right now — run 917 has been hung since it started and 24 runs are queued behind it, which is the same hang #154 identifies as the source of the leftover Postgres containers. Closes #337 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
36dbf18916 |
feat(passkeys): a password reset takes the passkeys with it (#42)
The last issue in the passkeys project, and the only one that adds no capability. It is the safety net, and it exists because this is the piece most likely to be skipped and most expensive to discover missing. A reset is the recovery path, and recovery has to be complete. The reset already deletes every session on the account, on the reasoning that a reset prompted by a compromise must not leave an intruder signed in for the remaining thirty days of their cookie. A passkey an intruder registered has no expiry at all. Leaving those behind would mean a customer can recover their password and still not have their account back. The obvious objection is that this hands whoever controls the mailbox a way to strip a customer's passkeys. It does, and it costs nothing: anyone who can complete a reset already controls the email address and therefore already controls the account. The passkeys were not protecting anything by that point. Anything in flight goes too. An intruder who pressed add a passkey moments before the reset could otherwise finish that ceremony afterwards and put a credential straight back onto the account the reset had just cleared. Changing a password deliberately does not do this, and the asymmetry is the point. A change requires the current password from someone already signed in, so nothing about it suggests a lockout or a compromise, and it already spares the current session for the same reason. A customer who suspects one particular device revokes that device by name from the account page, which is a better tool than deleting everything. A reset has no idea which credential is the problem, so it takes all of them. The customer is told twice. Before, in the reset email and on the reset form, unconditionally — that form has no session and is never told whether the account has passkeys, because answering that would make the reset page an oracle for it, so the wording has to read the same to someone who has none. After, with a count, and only when the count is not zero. That moment is the only one where the count can be reported: the rows are gone by the time anyone could go and look. A customer told two were removed who only remembers registering one has just learned something they could not otherwise find out. That notice is a panel that waits to be dismissed rather than a toast, because a toast dismisses itself and this is the message a customer needs to still be looking at while they decide what to do about it. The other question this issue asks — whether a reset ends a session established by a passkey — turns out to need no code, because #39 made both paths call one createSession. But "it falls out for free" is a claim, so there is now a test that establishes a session through that exact function and watches the reset end it. docs/ops/account-recovery.md records the whole policy, including the two answers that are not code. Losing an authenticator is not a lockout: the customer signs in with their password and revokes the lost credential themselves, which is why this change implements nothing for it. Losing the email address is a lockout, and there is deliberately no self-service route out — this shop holds no second proof of identity, and anything invented to fill that gap would be a weaker credential than the one it replaced. The manual route runs through the shop owner verifying against order history, and its third step, changing the address from the admin screen, does not exist yet. That is written down as a gap with its own notification and audit questions rather than smuggled in here. Verified: backend tsc clean for src and tests, 521 unit tests pass, lint clean apart from warnings that predate this branch; frontend tsc, lint and build clean. The integration and end-to-end suites need a database this machine has no Docker for, so CI is what proves those. Closes #42 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
72e8090fd1 |
feat(passkeys): offer passkey sign-in on the login form (#41)
The point at which passkeys become visible to customers. Everything before this was reachable only by knowing the endpoints existed. Below the password form rather than above it. Passwords are how every existing customer signs in and a passkey is the alternative, so putting it first would demote the path that works for everyone. Absent entirely where WebAuthn is unavailable, rather than shown disabled. A greyed-out control invites a customer to wonder what they are missing and offers nothing they can act on, and password login is the fallback in every case regardless. The check is read once at render because it decides whether the control exists, not whether pressing it works. The passkey button has its own loading flag rather than sharing the form's. The requirement is that a dismissed prompt leaves a usable password form behind it, and a shared flag would leave that form disabled and spinning while the browser's prompt is open. Dismissing the prompt is a cancellation and shows nothing. NotAllowedError and AbortError are the two the browser raises for it, and reporting either as a failure would tell a customer something went wrong when they changed their mind — leaving a red alert sitting above a form that is working perfectly. Everything else shows a message that says what to do next rather than only that something failed. That message says nothing about whether an account exists, which costs nothing to hold to here because the server already answers every refusal identically. There is also no email on this path at all, so there is nothing to be asked about. The end-to-end test covers the half of this issue that can be proven without an authenticator. The failure is injected at the first request, before the browser prompt, so it needs no credential and cannot hang waiting for a gesture nobody will make — and then the password form behind the error is used to sign in for real. That is the requirement: not a dead end. The other half cannot be tested anywhere but production, and this issue says so itself. Credentials bind to the Relying Party ID, so a passkey registered against QA will not work against production. QA proves the flow, the fallbacks and the copy; production needs its own smoke test with a real registration afterwards, and that is a standing property of the feature rather than a gap in this change. Verified: tsc clean for src and tests, lint clean with no warnings, frontend build green. Closes #41 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
1c8763f30f |
feat(passkeys): list, add and revoke from the account page (#40)
The issue calls this the smallest one in the project and the one that makes the rest usable, and that is right: registering a passkey with no way to see or remove it is worse than not offering passkeys at all. Revocation is the row going away. #39 looks a credential up by id on every sign-in, so a deleted one is refused immediately and by construction rather than by a flag something has to remember to check. The delete is scoped to the signed-in customer in the same statement that removes the row, because a credential id is not a secret and the WHERE clause is the only thing making this safe. Reading first and deleting after would leave a window. Both "no such credential" and "not yours" answer 404. The second is the interesting case, and saying so would confirm that some other customer holds that id. The lockout check is written even though it cannot fire. password_hash is NOT NULL, so every customer has a password and removing every passkey still leaves a way in. The issue asks for the check anyway and that is the right call, because it is written against the condition rather than against today's schema — it starts holding on its own the moment the condition changes. #332 is what changes it: social sign-in makes password_hash nullable and creates the first customers with no password, and at that point this branch starts running for real. The list returns name, added and last used, and nothing else. No public key, no credential id, no counter — the customer cannot act on any of them, and the credential id is the one value that identifies an authenticator to anyone holding it. Last used is what actually tells two entries apart when the names are similar: someone about to revoke one needs to know which device they are cutting off, and a creation date does not answer that. The whole ceremony lives in customerApi rather than the component, because it is one operation: options from the server, an attestation from the browser, verification back at the server. A component holding that intermediate state could leave a challenge issued and never answered. Dismissing the browser's prompt rejects, and that is a cancellation rather than a failure. Reporting it as an error would tell a customer something went wrong when they simply changed their mind, so NotAllowedError and AbortError are swallowed and everything else is shown. The server's message on a refused revoke is shown as it arrives too — it says what to do about the last way in, and a generic message would strand the customer on a button that just does not work. The section renders nothing where WebAuthn is unavailable, rather than offering a button that cannot work. Same rule #41 applies to the login form. Verified: tsc clean for src and tests in both workspaces, backend lint at the seven pre-existing warnings with none added, frontend lint clean, 521 unit tests across 36 suites, frontend build green. Not verified: the registration ceremony needs a browser and a real authenticator, which per #41 is a standing limitation of this feature. What CI can prove is the list and revoke endpoints, which are ordinary routes. Closes #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
6d320fd867 |
feat(passkeys): schema, dependency and per-environment Relying Party (#37)
Groundwork only. Nothing reads any of this yet, and no behaviour changes. The Relying Party ID is derived from PUBLIC_URL rather than written down, because it is the one value in this feature that cannot be corrected afterwards: a credential is bound to it permanently, and a wrong one surfaces only as a customer unable to sign in with a passkey that no longer matches anything. PUBLIC_URL is what every customer-facing link is already built from, so the ID is correct wherever those links are, and wrong only where they were already wrong. hostname rather than host, so a port cannot reach an ID that must not contain one. Local development is the exception the issue's table did not cover. envValidation requires PUBLIC_URL only when SMTP is configured, so a local setup that cannot send mail legitimately has none and falls back to localhost, which browsers treat as a secure context. Two origins there rather than one: the app is served by Vite on 5173 during development and by Express on 3000 once built, and those differ only by port, which is not part of the RP ID. The challenge table is separate from customer_tokens, and the reason is structural rather than preference. customer_tokens.customer_id is NOT NULL, and an authentication challenge is issued before anyone is identified — a discoverable-credential sign-in has no customer to attach to at the moment the challenge exists. Storing it there would mean making that column nullable for every other kind of token. Two of the issue's open decisions are deliberately not made here, because they belong to the ceremony that enforces them rather than to the schema. What to do when the signature counter fails to increase is #39's: many synced passkeys report zero forever, so treating a non-increase as cloning is wrong for them and right for a hardware key, and this only has to hold the value. Whether a disabled account can authenticate is also #39's, and the schema takes the position that it should not cost the customer their devices: credentials survive disabling and are refused at the ceremony, so re-enabling does not mean re-registering everything. Deletion is different and is settled here — credentials cascade with the customer, since one outliving its owner could authenticate as an account that no longer exists. signature_counter is BIGINT because the spec allows a 32-bit unsigned value, which overflows a signed INTEGER at half its range. That is the first bigint column in this schema, so the generated mirror gains the Int8 alias with it. Both tables are added to resetDb's TRUNCATE list and to REQUIRED_TABLES, and the schema mirror is updated by hand to match what kysely-codegen emits — placement and all, so a real regenerate produces no diff. Skipping either is how #56 turned a green local run into a red main; the mirror drift guard exists precisely to catch it, and schemaLoss's count moves from 18 to 20 with them. Verified: tsc clean for src and tests, lint 0 errors with no new warnings, 494 unit tests across 34 suites including nine new ones for the RP derivation, frontend build green, and the migration parses. Not verified: the migration has not been run against a database, and the integration suite needs one this machine cannot provide. Worth knowing before this goes further: #313 changes the domain, and every passkey registered before that cutover stops working at it. This code needs no change — it follows PUBLIC_URL — but the credentials do not survive. That is free while production is not live and nobody holds one, and it stops being free the day the shop opens. Closes #37 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
4b1ffffc25 |
fix(admin): refetch Inventory when its tab becomes visible (#327)
Two items published from the Review queue did not appear in Inventory. Publishing was never at fault: the items were published, the rows were right, and the API returned them. The Inventory tab was showing a list it had fetched earlier and never refreshed. antd keeps a tab pane mounted once it has been rendered, and Inventory is the default tab, so its pane mounts at page load whether or not anyone looks at it. Its only fetch runs from an effect depending on the filters, so it fires on mount and when a filter changes and at no other time. Open the admin, switch to the Review queue, publish, switch back, and nothing has re-run — the table still holds the list built before the submissions existed. A browser reload shows them, which is what makes this read as publishing being broken rather than as a stale table. The tab is controlled now and tells Inventory whether it is the one on screen, and Inventory refetches when that becomes true. Guarded on visibility rather than fetching unconditionally, because the pane lives for the life of the page and would otherwise keep refetching while hidden. destroyInactiveTabPane on the Tabs would also have fixed it, by remounting, and was rejected: it discards every tab's state on every switch — filters, scroll position, a half-filled form — and refetches all of them repeatedly, which is a much larger behavioural change than this bug is worth. The existing publish test passes against the broken behaviour, and that is the reason this went unnoticed. It asserts against GET /api/admin/items, and the API was always correct. The new test asserts through the UI and never reloads the page: Inventory is opened before publishing, so its list is fetched while the item still carries its submission-timestamp name, and the assertion afterwards looks for the name given at publish — which a stale list cannot contain. A reload anywhere in it would make it pass against the bug it exists to catch. AdminPage's comment claimed only the active panel is mounted. It is only the active panel that is visible; the rest stay in the DOM. That mistaken belief is the shape of this bug, so the comment now says which it is and why it matters. Not fixed here: Categories, Tags, Upload links and Customers are mounted-once children of the same Tabs and are all changeable from elsewhere, so they very likely share this. Recorded on the issue rather than assumed to be fine. Verified: tsc clean for src and tests, lint 0 errors, production build green. The new test needs CI to run — the e2e suite wants a browser and a database this machine cannot provide. Closes #327 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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>
|
||
 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
|
64f8efb617 |
fix(admin): offer Remove and Restore independently (#293)
One button whose label flipped on "is every photo cut out?" could not serve a partly cut-out item, which is not a hypothetical state: it is what a partial removal leaves behind, and it is also what happens when REMBG_URL goes away after some photos were already done. In that state the single button read "Remove backgrounds", so the cut-out photos the item already had could never be restored from this screen. Remove and Restore are now separately gated and can appear together, which is correct — Remove finishes the job on what is left, Restore undoes what is already done. Restore is deliberately not gated on the backgroundRemoval config flag. Gating it would strand cut-out photos with no way back in exactly the environment that most needs the undo. Remove stays gated, so an unconfigured environment shows no button rather than one that reports zero of four done every time. The emptiness check moves from `!== null` to `!= null`: original_image_path is optional on the shared Item type because the public storefront response omits it, so a stray undefined has to count as "not cut out" — `undefined !== null` is true, which would misread a public-shaped item as fully cut out. The modal now refreshes on a non-ok response too. A restore that fails partway can still have swapped some files back before it failed, so returning early left the thumbnails showing files that are no longer on the server. The warning text is now driven off whichever count the action reports, so a partial restore says how far it got the same way a partial removal already did. The e2e spec seeds its item into a category of its own and filters the table down to it. The inventory table paginates at 10 and the suite runs fullyParallel, so an unfiltered page one was never a reliable place to find the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
dbb63bc3d2 |
fix(admin): keep the active inventory filter and surface background-swap failures (#293)
handleBackgrounds re-read /api/admin/items unfiltered and called setItems(all) after every remove-backgrounds or restore-originals call, so an admin who had filtered Inventory to one category and opened an item from that filtered view saw the table silently repopulate with the entire unfiltered catalogue the moment the request resolved. Every other mutation in this file goes through load(), which respects the active filters; this one didn't, for no reason the spec required. The fix reuses load() instead: it now hands back the rows it fetched (previously discarded after setItems), and handleBackgrounds picks the edited item's fresh row out of that filtered result to refresh the open modal, rather than issuing a second unfiltered fetch. There is no GET /api/admin/items/:id route to fetch a single item directly, and the remove-backgrounds/restore-originals routes return only a summary, not the item, so load()'s own result is what's actually available. A background swap never touches the fields anything filters on, so the edited item stays in the filtered result whenever it was in it before. Also added a catch to handleBackgrounds, matching the message.error shape every sibling handler (handleDelete, handleDeleteImage, handleStatusChange) already uses — previously a network drop or a malformed JSON body became an unhandled rejection with no toast, silently different from how the rest of the file reports failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
385d5b89bf |
feat(admin): offer background removal where an item's photos are edited (#293)
One button per item in the inventory editor, beside the per-thumbnail delete buttons rather than on them, because an upload is one item and its photos are views of one thing. Its label is derived from the images rather than stored: Restore originals when every photo already carries an original, Remove backgrounds otherwise. The otherwise deliberately covers the mixed state a partial failure leaves behind — with two of four cut out it reads Remove backgrounds, which is the action that finishes the job, and pressing it skips the two that already worked. Rendering is gated on the feature being configured or every photo already being cut out, not on the flag alone. Gating on the flag would hide Restore originals the moment REMBG_URL is unset, stranding cut-out photos with no way back — the same reasoning the review queue's control already uses. The editor is a modal and this changes files on the server while it is open, so the item is re-read afterwards and the open modal updated. Without that the thumbnails keep showing the previous files and the button looks like it did nothing, which is the bug this was most likely to ship with. frontend/src/api.ts gains original_image_path on the shared Item type's images, since ADMIN_ITEM_SELECT's images aggregate carries it and the public catalogue's does not. It is added as optional rather than required because Item is the same type fetchItems() uses for the public storefront, and a required field the public response never sends would be a type that lies about what is actually there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
046937cdce |
test(e2e): assert a created link's address shows in the table (#260)
The spec's E2E section requires that a created link shows its address in the table, but the test that creates one filled a throwaway address and never asserted the cell. The address is now kept in a variable and asserted on the row after creation, using it to locate the row's own cell rather than any other row's. Not run: the local stack is down and Playwright was explicitly out of bounds for this pass, per the task instructions. This is written and verified by inspection and lint only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c12d40dc02 |
fix(admin): stop the upload links table claiming delivery it never recorded (#260)
The table headed its address column "Sent to", but contact_email records only the intent an admin typed in, never whether delivery happened — that outcome is shown once, at creation, and is not persisted. In QA, where every send is blocked by design, every row read "Sent to ..." for links that were never emailed, and the honest warning that appears at creation is guarded on `issued`, so it vanishes on refresh, leaving the false heading as the only surviving statement. Renamed to "Email", which is true of what the column actually stores. Separately, create() called setMailed(null) before every request, including one that would go on to 400. If an admin creates link A while mail is down (warning shown, token A still on screen) and then mistypes an address on a second attempt, the 400 path returned early — but the reset had already run, so the warning for link A disappeared while token A was still displayed on the same screen. setMailed is now only called after a successful create, alongside setIssued, so a request that never produces a new link can no longer clear a warning that belongs to the one still shown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
9c5935e8e0 |
feat(admin): ask for the contributor's address when creating a link (#260)
The address is now a required field beside the label, the links table shows where each link was sent, and the admin is told plainly when the mail did not go — with the link still on screen to copy, which is the case that matters in QA and in local development where there is no mail at all. Two specs in unrelated features created links with only a label and the route now refuses that, so they are updated here rather than left to go red on somebody else's branch. That is the cost of making the address required, and it is a small one: the compiler and the suite find every call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
1bd0024e6f |
test(storefront): cover paging, and say what the filter tests meant (#269)
Two assertions named a fixture and expected it visible in the unfiltered grid. No paginated catalogue can promise that — the item is on some page, not necessarily the first — so both would have started failing the moment paging landed. They were only ever proxies for "the result set got bigger", and the visible total lets them say that directly, which is what the issue predicted when it asked for a count. The new cases assert the control and the URL rather than which item is on which page, because the development database never truncates and which item lands where is not something a test may rely on. That is the same trap the two rewritten assertions had fallen into, and repeating it in new tests would have been worse than leaving them alone. Writing them found a real defect rather than just covering the feature. The control was rendering while the catalogue was still loading, showing "0 items" for a moment before the real count arrived — the empty-state early return only fires once loading has finished, so a mid-load render fell through to the grid branch with a total of zero. It is now suppressed until there is something to count, which is both true and what makes the count usable as a signal in a test. StorefrontPage.totalItems waits for the control for the same reason: reading during the load returned zero and quietly made "the result set shrank" compare against nothing. The conditional skips carry a file-level eslint exception with its reasoning rather than being left to add four warnings. They are honest about a real limit: against a catalogue of ten items or fewer these cases prove nothing, and if the e2e database is ever seeded that thinly they need fixtures of their own instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
f9073c515e |
feat(storefront): page the catalogue instead of rendering all of it (#269)
The page number joins the filters in the URL, which is already the single source of truth for what the storefront is showing. That is the whole reason numbered pages were chosen over infinite scroll: a page is a place you can send someone, and a scroll position is not. The page size deliberately does not go there — it is a preference belonging to one person, and putting it in the URL would mean sharing a link to an item also imposed your page size on whoever opened it. Changing a filter returns to page one, and it does so for free: filtersToSearchParams builds a fresh URLSearchParams, so applying filters drops the page parameter while goToPage copies the existing params and keeps the filters. That is behaviour worth having rather than an accident to tidy up — landing on page seven of a two-page result is a state a customer cannot get out of without understanding the URL. The control carries the total, because showing the count was a requirement in its own right and the only count that existed before this was on the filter drawer's "Show N items" button, which is hidden whenever the drawer is closed. It is therefore shown even when everything fits on one page: hiding the control on a single page would hide the count with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
013b6abb52 |
feat(storefront): the rules for paging the catalogue (#269)
Every decision paging needs, as pure functions: which page a URL is asking for, which page is actually showable given how much there is, which slice of the items that is, and what page size to use. Pure because that is the only thing this project can unit-test — vitest runs in a node environment with no jsdom and no testing-library, so a hook or a component is only reachable through Playwright. Keeping the rules here means the rules have tests and the React wrapper stays thin enough not to need any, which is the same split filters.ts already uses for the URL. An unrecognised page size is refused rather than clamped. A stored or hand-edited 5000 would render the entire catalogue in one page, which is the exact failure this issue exists to prevent, and clamping would quietly honour a value nobody offered. Storage access is guarded on both sides because localStorage is absent when there is no window and throws outright in some privacy modes, and neither is a reason for a customer to lose the catalogue — the worst acceptable outcome of a broken preference is the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
786996b7ac |
fix(admin): keep Restore original working after REMBG_URL is unset (#281)
The DraftQueue background-removal control gated both "Remove background" and "Restore original" on the same `backgroundRemoval` flag, which only reflects whether a sidecar is currently configured. Restoring is a pure database swap and never calls the sidecar, so once photos had already been cut out and REMBG_URL was later removed from the stack, the admin was left looking at a cut-out photo with no control at all and no way back to the original short of a hand-written SQL UPDATE — directly breaking the "the original is always restorable" invariant the feature is built on. DraftCard now computes `enabled` per photo as `backgroundRemoval || image.original_image_path !== null`, so Restore original stays available whenever a photo has an original regardless of whether the sidecar is configured, while Remove background still requires a configured sidecar. Also corrected the docstring on `DraftQueueResponse.backgroundRemoval` in draftsApi.ts, which claimed the flag hides "the control" generically — it only ever governed the remove-background control. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
09686d9f94 |
feat(admin): remove or restore a photo's background from the review queue (#281)
Adds the per-photo control that closes out the background-removal feature: each photo in the review queue now gets a "Remove background" or "Restore original" button, whichever matches its current state, and the button only appears when the server reports a sidecar is configured. The label is read from original_image_path alone rather than a second flag, so there is nothing that could disagree with what the button actually does.
draftsApi.ts's fetchDrafts now returns { drafts, backgroundRemoval } instead of a bare Draft[], matching the breaking change Task 6 made to GET /api/admin/item-drafts. DraftImage gains original_image_path, and a new setImageBackground(itemId, imageId, action) posts to the remove-background/restore-original endpoints, preferring the server's error message the same way publishDraft does.
Also updates docs/ops/image-background-removal-stack.md: the status line no longer says "evaluated, not adopted", since the feature is adopted here, and the closing "If this is adopted" section is replaced with "How the application uses it", describing the two real entry points (the drafting worker's default-on checkbox, and this per-photo control) and confirming that nothing in the feature deletes a file or a row.
Adds an e2e case asserting the button's label appears on a freshly submitted item's card, scoped to that card by the sender's note per #241. It is unrun in this environment — the local stack was not started, per standing instruction not to run start-local.ps1 or Playwright without the user's supervision.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
1902db6d04 |
feat(intake): offer background removal on the submission page, ticked (#281)
Adds a checkbox to the public submission page that lets a sender opt out of background removal, ticked by default because most items look better cut out and the reverse default would mean almost nobody got it. It only renders when the server reports the sidecar is configured, matching the intake link's new backgroundRemoval flag from Task 5 — an unconfigured environment gets no checkbox rather than one that would do nothing. submitItem now takes removeBackground as a required fourth parameter, sent as the multipart string 'true' or 'false' to match the backend's exact-string opt-out contract. Making the parameter required rather than optional was deliberate, so the compiler would catch any call site left unupdated; the frontend build (which also type-checks tests/ via tsconfig.test.json) confirmed the only call site, in Submit.tsx, was updated. scripts/start-local.ps1 now sets REMBG_URL for the local backend so the checkbox is visible during local and e2e runs; the value need not resolve, since no e2e submission reaches the sidecar without a configured drafting step. Adds two e2e cases to intake-submit.spec.ts: the checkbox appears ticked by default, and a sender can uncheck it and still submit successfully. Both are written per the task-7 brief but not run in this session, since running Playwright requires the full local stack (database, backend, frontend dev server) which was not started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
cd2676233c |
chore: clear the six code smells SonarQube reported (#181)
The list finally arrived from the reporting added earlier, and confirmed what #181 could only suspect: these are not the five eslint-plugin-sonarjs warnings that issue lists. Those were fixed under #261 and the count staying at five was a coincidence. It is six now, 25 minutes of debt, and one of them was mine. admin.ts imported '../utils' twice — I added readId in #207 without noticing the file already imported from there. One import now. filters.ts had a redundant `as ItemStatus[]`. TypeScript narrows an array through `.every()` with a type predicate from 5.5, and this project is on 5.9, so the assertion stopped telling the compiler anything. Removed, and the build confirms the narrowing holds without it. adminSettings.ts was the only CRITICAL: cognitive complexity 18 against a limit of 15, almost all of it three near-identical loops differing only in how they validated. Each validation is now a small pure reader returning a refusal rather than sending one, and the handler is one loop over a table. Adding a setting type means adding a row. That refactor is deliberately behaviour-preserving. Two things were left alone on purpose: the blanket rejection of empty text, which is wrong for the two settings whose documented default is empty and is filed as #280 rather than folded in where it would be invisible; and the absence of the `count` settings, which no caller submits and which the admin screen has no control for. I had started adding count validation and reverted it — widening behaviour under cover of a complexity fix is how a refactor stops being reviewable. The three S6478s are render props, not components defined during render. ErrorBoundary's `fallback` is typed `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)`, so React only ever sees returned elements and never a new component type — the subtree destruction the rule describes does not happen, and the rule's own message offers `allowAsProps` for this shape, which cannot be set from here. Hoisted rather than suppressed because none of them closes over anything local, so at module level each is one stable function instead of a new closure per render. That is a mild improvement, not a contortion. Verified: 402 backend unit, 358 backend integration, 30 frontend unit, 157 e2e, both lints clean, both builds clean. The e2e run matters most here — storefront-errors.spec.ts exercises all three hoisted fallbacks, and it was run on its own first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
06933aec75 |
fix(scripts): make the alias check fail closed, and correct three stale docs (#208)
The alias check was a negative match on an allowlist of English error strings, which returned True for empty output, for $null, and for "exit status 1: Access is denied." — so an alias switch producing nothing, or failing on the symlink permission error this file's own header warns about, was reported as success while the old version kept running. That is the bug #198 was filed about, narrowed rather than removed, and it also broke whenever nvm reworded an error. It is now a positive match on "Now using node v<what is actually running>". The floor check moves ahead of the switch and reads the constant rather than the result. Where it sat, $major was always whatever NODE_VERSION says, so it validated the switch it had just made instead of the pin it exists to guard, and could never fire. Use-NodeLatest is now Use-PinnedNode. In a change whose whole subject is that "latest" means something people do not expect, the name was an avoidable trap. The restore default moves beside NODE_VERSION. It deliberately is not a param default: a param block runs before the dot-source, so $script:DEFAULT_NODE_VERSION is still $null there and the restore would have quietly restored nothing — leaving the machine on the pinned version, which is the exact failure the restore exists to prevent. It is resolved after the dot-source instead, and an explicit -DefaultNodeVersion still wins. Three documents described behaviour the code no longer has: README's "both scripts run nvm use latest", run-tests.ps1's .DESCRIPTION, and project-context.md's instruction to agents. All corrected, and project-context.md now also says not to run these scripts from an agent shell, which is how this machine once ended up with no Node at all. Part 4 of the issue is partly stale: backend/package.json already declares engines >=20.9.0. frontend now matches it. The larger question — whether local should be pinned to the Node 20 that CI and the production image actually run — is a decision rather than an oversight and is left open on the issue. Verified by parsing all three scripts with the PowerShell AST parser, which does not execute them. They are deliberately never run from an agent shell. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
fdc92d8f19 |
test(intake): cover reviewing and publishing a submission (#225)
Seeded through the intake route rather than POST /api/admin/items, which writes no item_drafts row — an item created that way would never appear in a queue that joins that table. Going through the link is also the path a real submission takes, so the test exercises what actually ships. Two cases. The first edits the price and publishes, which confirms the number, so no dialog appears and the item reaches the storefront at what was typed. The second publishes an untouched price and asserts the confirmation names the figure and the fact that nobody chose it, then cancels and checks the item is still unconfirmed. That second case is the protection this whole screen exists to provide. Scoped to the card each test creates, located by the sender's note carrying the run id — the item's own name is a submission timestamp and not unique to the test. The dialog is matched on its accessible name because antd nests the confirm title in two elements and getByText resolves to both. Full suite 157 of 157, up from 155. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
2b06538ef9 |
feat(intake): review, edit and publish drafts from the admin (#225)
The screen that makes the intake pipeline usable. Until now a draft existed only in item_drafts and nothing rendered it, so a successful draft and a failed one looked identical from the admin — the item shows its placeholder submission-timestamp name either way, and telling them apart needed SQL.
The price carries the weight the schema no longer does. It is labelled with where the number came from, anything not set by a person is marked unconfirmed, and publishing at an unconfirmed price asks first rather than reporting afterwards. Editing the field is what confirms it, so opening the card and leaving the price alone is not recorded as approval — the same rule the server applies, which this only has to agree with.
Discard is offered rather than delete, and a discarded card offers Restore in its place.
The e2e page object's AdminTab union is extended alongside the tab itself. It is a closed union, so admin.open('Review queue') would not type-check without it — and the tab strip and that union have to be changed together or the next spec to use it fails to compile.
Both frontend lint and build clean, still at zero warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
fe28c97e0f |
chore(sonar): remove the rejected Tinqer spike, clear the lint debt, and report measures in CI (#261)
The standing cleanup, three features behind. Four changes. Report the measures in CI. This is the one that matters, because the rest was only findable by reading the tree. SonarQube here is 9.9 Community: no Bearer auth, so the official MCP cannot connect, and the host is a CI secret, so hotspots, duplication, debt and coverage existed only on a dashboard — which made "reduce the debt" an instruction nobody could act on without a browser open beside them. scripts/summarize-sonar.js queries the measures API with the secrets the workflow already holds and prints the result into the job log. The scanner masks the URL and token; measures are not secret. It polls the compute task before reading. The workflow does not set sonar.qualitygate.wait, so the scan step returns once the report is uploaded and the server computes measures afterwards — reading immediately would return the previous analysis, indistinguishable from this one and quietly wrong. When it cannot confirm, it says so in the output rather than presenting stale numbers as current. It is deliberately not guarded with continue-on-error: it exits 0 on every path, and guarding it would oblige it to appear in the final gate, whose job is to fail the build. Remove the Tinqer spike. #216 evaluated Drizzle against Tinqer and rejected Tinqer, and its closing comment said the throwaway src/db-tinqer/ probe must not reach main. The whole spike commit was merged, so it did. The probe is 71 lines imported by nothing, and @tinqerjs/tinqer, @tinqerjs/pg-promise-adapter and pg-promise were dependencies for a library nobody chose. The condition_note column that warning also named did not reach main. Clear the lint debt, both projects now at zero warnings from six and two. One of these was a real defect rather than tidiness: the third catch block in shippingAddresses.ts rolled back and returned 500 while discarding the error, so a failed default-address change left nothing behind to say why — the two catch blocks above it in the same file already logged, and this one had simply been missed. The Express namespace augmentation is a false positive and is disabled with the reason written beside it, because an interface that must merge into one Express declares inside a namespace has no ES module spelling. Dedupe the extension map. backfillImageReencode.ts kept its own .jpg/.png/.webp table whose comment named uploadTypes.ts as the source of truth, directly above duplicating it. That file rewrites stored images, so the two disagreeing would silently skip files it should re-encode. src/db-drizzle/ deliberately stays. #217 is open to promote exactly those files properly, with tablesFilter and the sql.param() array rule; deleting them here would be doing #217 badly in the wrong issue. Only their unused-symbol warnings are fixed, and if drizzle-kit pull regenerates schema.ts the table warning returns — worth #217 knowing. Hotspots and coverage are untouched because both numbers are still invisible. They are the next pass, once the step above has printed them once. Closes #261 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
94b2dfd186 |
fix(e2e): match the modal OK button exactly so a random suffix cannot collide (#253)
Playwright matches an accessible name case-insensitively and as a substring unless exact is passed, so `getByRole('button', { name: 'OK' })` matched any button whose name merely contained "ok".
Caught on a full serial run of main:
strict mode violation: getByRole('button', { name: 'OK' }) resolved to 2 elements:
1) aka getByRole('button', { name: 'Freed rmtiq9okg22k9e' })
2) aka getByRole('button', { name: 'OK', exact: true })
A leftover "Freed …" toast was still on screen and the random base36 suffix happened to contain "ok". Roughly one suffix in a few hundred does, which is the profile of a test that fails occasionally and reproduces for nobody.
This is not the contention #241 addressed. It reproduced with workers: 1, serially, on a fresh database — it needs only a stale toast and an unlucky suffix. It is at least part of what #245 skipped a real test to work around.
Also makes the taxonomy modal's OK and the sold-filter 'All' radio exact, the only other targets short enough to appear inside a random suffix. The dozen or so remaining short names are left alone deliberately: they would need a collision in deliberately-chosen test data rather than a random one, and substring matching is load-bearing in some of them.
Verified with two consecutive full e2e passes, 155 of 155 both times.
Closes #253
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
42412e8eeb |
fix(e2e): restore the frontend build by typing the responses findOrFail searches (#254)
main did not build, so every Portainer deploy failed with `npm run build` exit code 2. My regression from #241. findOrFail<T>(items: T[], predicate: (item: T) => boolean) infers T from both parameters. The call sites annotate the predicate as documentation, and the arrays come from .json(), which is any and offers no competing candidate — so T became the one-field shape written in the lambda and every caller failed on the field it actually wanted. Array.prototype.find has no such problem, which is why the code this replaced type-checked. The four responses are now typed at their call sites, so T is inferred from real data and the predicates need no annotation. findOrFail additionally takes NoInfer<T> on its predicate, so a stray annotation can never drive the element type again. The specs are better typed than before this change: `.json()` was plain any, and the annotations only ever documented a shape nothing enforced. I did not catch this because I verified with a bare `npx tsc --noEmit`, and tsconfig.json is `"include": ["src"]` — it structurally cannot see tests/. The specs are checked by the second command in `npm run build`, which is the step Docker runs and the step that failed. Verified this time with `npm run build` itself, plus lint, the unit suite, and two consecutive full e2e passes. Closes #254 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d27dcae62b |
feat(admin): choose the drafting model from Settings (#223)
The model was going to be an environment variable, which meant a redeploy to change it. It is now an admin setting, so it can be changed from the Settings page like the cart expiry and the greeting. A dropdown validated on the server, not a free-text field. The API only rejects an unknown model at the point of use, so a typo would be stored happily and then fail on every submission, surfacing as drafts quietly not appearing rather than as an error anybody could act on. The PUT refuses anything outside the offered set, and getSettings falls back rather than handing on a value that is no longer offered — drafting with the default beats drafting with a model the API will refuse. One catalogue rather than two lists. The dropdown needs the models, costMicros needs their rates, and the price shown beside a model in Admin has to be the price it is actually billed at, which it cannot be if the two are maintained separately. Rates were confirmed against the pricing page rather than recalled: Sonnet 5 $2/$10, Opus 5 $5/$25, Haiku 4.5 $1/$5 per million tokens. The unknown-model fallback is deliberately the most expensive rate and never zero, because a budget that reads as unspent however much was spent is the one failure a spend guard cannot have. Adding a third setting type pushed getSettings past the cognitive complexity limit, so the per-type resolution moved out into one small function each — the same shape the definitions block above it already argues for. The exhaustive assertion in the GET test gained the new field rather than being loosened. It exists to catch a setting silently vanishing from the response, and that is worth more than not having to touch it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
6489486de3 |
test(e2e): run the suite on one worker so a red run means something (#241)
The lever the plan deliberately held back, applied now that there is evidence it is needed. Better failure messages and a wider assertion timeout were not enough. Measured on this branch, same commit, same machine. Two parallel runs each failed 3 of 155, and not the same three: password-reset and admin-inventory-filters in one, admin-email-settings, admin-inventory-filters and resend-verification in the next. All 16 tests from those three specs then passed when run serially, and the full suite passed 155 of 155 twice in a row. Failures that move between runs of identical code are contention, not defects — specs asserting over tables other specs are concurrently writing to. The cost is about three minutes, roughly one minute parallel against 3.9 and 4.0 serially. A red run that means something is worth three minutes; the previous state was a suite whose result nobody could act on, which is what #245 had to skip a real test to work around. If that time is ever needed back, the cheaper fix is giving each worker its own database rather than raising this number and reopening #241. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
895c08d1a7 |
test(e2e): replace unchecked lookups, widen the assertion timeout, restore the skipped test (#241)
Tasks 2 to 4 of the plan. Nine `collection.find(...)` dereferences become findOrFail, so a missing row fails as a named assertion naming what was wanted and how many rows were searched, rather than "Cannot read properties of undefined" pointing at test plumbing. Where the old code followed the lookup with expect(x).toBeTruthy(), that assertion is dropped: findOrFail already guarantees it, and with a better message. The expect timeout goes from Playwright's default 5s to 10s. It costs nothing on a green run — it bounds how long a failing assertion waits, not how long a passing one takes — and #239 died reporting exactly Timeout: 5000ms on a runner that also builds, migrates and runs three other suites. The admin-save happy path comes back from the #245 skip. It is the only end-to-end check that adding an item reaches the database rather than merely firing a toast, and it passed both full parallel runs and in isolation. filters.spec.ts:216 is deliberately untouched: its .find() searches CSS class names on a string array, not test data, and has no missing-row failure mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
bcecda9122 |
fix(intake): stop a throttled sender being told their link is dead (#222)
Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for. One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it. Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message. Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want. The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions. Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store. The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole. Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings. Ref #222, #241 |
||
|
bermudalamb
|
9b4d7f2d03 |
feat(intake): manage upload links from the admin (#222)
An Upload links tab beside Tags: issue a named link, see how much of its allowance is spent, revoke it. Until now the only way to create one was curl, which is how the earlier tasks were exercised. The token is shown once, in an alert that says so plainly, because the server stores only a digest and genuinely cannot produce it again. A refresh loses it — that is the honest behaviour rather than a bug, so the copy says to revoke and reissue if it is lost instead of leaving somebody hunting for a reveal button. The cap field starts at 25 and unlimited is a checkbox rather than an empty field. Blank-means-unlimited would make the least deliberate action produce the least bounded link, and this screen sends all three cases explicitly so the server's default only ever has to cover callers that are not this screen. Two things came out of driving it in a browser rather than reading it. The revoke confirmation said "OK", and every other destructive confirm in this admin names its action — Delete, Disable, Re-enable — so it now says Revoke, in danger styling. A confirm button reading OK makes the reader go back and re-read the question to find out what they are agreeing to. And Popconfirm turned out to be a component nothing else here uses; the rest use Modal.confirm with an explicit okText. Keeping Popconfirm but matching its labelling to the established pattern seemed the smaller inconsistency, since the interaction is a row action rather than a page-level one. The load-on-mount effect carries the same eslint-disable and reasoning Tags and Categories already use, rather than a new shape. Verified in a browser: create shows the one-time reveal, the row lists as 0 of 25 and Active, revoke flips it to Revoked, and an explicitly unlimited link shows a bare count with no cap. The database then confirmed a default of 25, a null for the unlimited one, and a stamped revoked_at. Frontend: build clean, lint unchanged at 2 pre-existing warnings, 30 unit tests pass. Ref #222 |
||
|
bermudalamb
|
e7b01fdb36 |
feat(intake): add the public submission page (#222)
Where someone with no account sends in photos of one item. Route /submit/:token, outside the authentik gate by design: the token in the URL is the whole access control, which is what #222 chose deliberately over accounts. One state for every refusal, matching the server's single 404. Unknown, revoked and used-up links all render the same "this link is not active" card, because saying which kind of dead it was would tell a stranger whether a link they guessed at exists — the server is careful about that and the page must not undo it. `beforeUpload` returns false so antd keeps the files rather than uploading each one as it is picked. The submission is then a single request the server can accept or refuse as a unit, which is what makes the transaction on the other side meaningful. The accepted types and the six-file cap are stated here so the picker offers exactly what will be taken, but both are checked again server-side, because everything on this page is under the sender's control. The fetch effect guards against a late response from a previous token overwriting the current answer, which is reachable simply by editing the URL. TypeScript caught a real mistake rather than a stylistic one: `.filter((f): f is File => ...)` on antd's originFileObj does not narrow, because RcFile extends File and the predicate would widen rather than narrow. flatMap avoids the predicate entirely. Verified in a browser rather than by inspection: a throwaway Playwright run against the live stack confirmed the form renders for a good token, the inactive card renders for a bad one, and a photo can actually be sent and acknowledged. The database then showed the item at status pending with the default price, the draft carrying the note and its originating link, the image row written, the link's counter at one — and zero storefront-visible items, which is the property that matters most. Ref #222 |
||
|
bermudalamb
|
64324609e1 |
test(e2e): add a find-or-fail helper for collection lookups (#241)
Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal. The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase. It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures. Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails. Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean. Ref #241 |
||
|
bermudalamb
|
66c696ce40 |
test(e2e): skip the admin save happy-path test while #241 stands (#245)
`main` has been failing on one e2e test since the sold-filter fix landed, and it is a different test from the one #239 corrected: admin-save-failures' "saves an item successfully when the server accepts it". Skipped rather than fixed, deliberately. It fails in CI and passes locally, and which test fails moves around — a local parallel run of the whole suite on the same commit failed four *different* specs (admin-inventory-filters, auth, favorites-filter, resend-verification) and not this one. That is #241: fullyParallel against a single shared database. Fixing this test on its own would be guessing at a symptom that reappears somewhere else next run. Ruled out before disabling anything: the re-encoding from #226 is not involved. AdminInventory.addItem fills a name and a price and saves, attaching no files, so stripUploadedImages iterates an empty array and the image path is never entered. Checked rather than assumed, because this spec is on the admin save route and that is exactly where a regression of mine would surface. What this stops covering is not trivial, and the comment says so at the call site: it is the only end-to-end check that adding an item actually reaches the database rather than merely firing a toast. #245 exists so that it is un-skipped when #241 lands, rather than left behind. A skipped test on the core admin save path is worse than a red build, because a red build is at least visible. Ref #245, #241 |
||
|
bermudalamb
|
30227fb1e1 |
fix(test): correct the sold-filter tally assertion stranded by #188 (#239)
main has been red since 2026-08-25. Every SonarQube run reported 147 passed, 1 failed, and it was this test every time — expected "Filters", received "Filters (1)". The test is stale, not the code. It was last touched on 2026-08-23 in #137; the tally logic changed on 2026-08-25 in #188, which never touched the spec. #188 redefined the tally as the number of chips and moved the availability preset into the dimension system as a bar dimension — one that still emits a chip for any non-default status, deliberately, because without it `?status=reserved` is an empty grid with no Clear filters button and no way out but editing the URL. The test asserted the rule that held before that change. Counting bar chips differently from drawer chips would restore exactly the per-screen special-casing #188 removed, and the drift it fixed was the admin's tally disagreeing with the storefront's. So the assertion moves, not the tally. The replacement also checks the tally comes back down when the default is restored. The original only ever asserted one direction, which would pass against a count that incremented and never decremented — worth fixing while the test is open rather than leaving a second gap behind the first. There is a real wart left standing: `Filters (1)` opens a drawer with nothing selected in it, because the filter it is counting lives in the bar. That is a cost of #188's design rather than a defect in it, and the comment now says so rather than leaving the next reader to rediscover it. Verified by running the spec in isolation with a single worker: 6 passed. Closes #239 |
||
|
bermudalamb
|
44328d0b5c |
feat(admin): show the deployed commit and build time in the admin (#233)
There was no way to tell which build an environment was running. That is not hypothetical: minutes after #232 merged, `npm run backfill:images` in QA failed with `tsx: not found` because the container was still serving the pre-merge image, and the only thing that revealed it was npm echoing the old script line. Had the change been anywhere other than a package.json script, the container would have looked healthy while running the wrong code.
The header now reads something like `a5076cc · built 29 Aug 20:36`. The commit answers "is this the code I expect"; the build time answers "did my redeploy actually rebuild", which is a different question and the one that would have caught the case above.
The commit is read out of `.git` directly rather than by shelling out, because node:20-bookworm-slim has no git binary and adding an apt layer so the image can print seven characters is a poor trade. `.git` is copied into the build stage only — verified absent from the final image — so no repository history reaches a deployed container.
Resolution is pure and separately tested across every shape that actually occurs: a detached HEAD holding the object name, which is what a checkout of a ref produces; a symbolic HEAD followed to a loose ref file; the same followed to packed-refs, which is what a fresh clone commonly has; peeled `^` tag lines ignored so an annotated tag cannot yield the wrong commit; and every failure path returning `unknown`. That last part is the one that matters most — this runs during a Docker build, and a version stamp must never be the thing that stops a deploy.
Served from a gated /api/admin/version rather than folded into /api/config. That endpoint is public, and a commit hash there would tell any storefront visitor exactly which revision of a public repository is deployed. An integration test asserts the gate and asserts the public config does not carry it, because the boundary is the whole point rather than an implementation detail.
Verified in the built image rather than argued: the stamp inside it reads
|
||
|
bermudalambandClaude Opus 5
|
b897e99363 |
fix(cart): say what the demo button does rather than what the shop does (#203)
#195 added a notice reading "Demonstration only — This shop is not taking payments at the moment", gated on `demoMode` alone. That claim is false in a configuration the ops runbook actively steers towards. `demoMode` and `paypalClientId` are independent. `checkDemoMode` and `checkPayPal` only make the PayPal secrets *required* when `DEMO_MODE=false`; nothing forbids them while it is `true`. And `production-stack-cutover.md:65` says flipping to `false` without all three crash-loops the container — so the only safe order is to populate the secrets while demo mode is still on, verify, then flip. In that window `Cart.tsx` renders live PayPal buttons directly beneath a banner telling the customer the shop takes no payments, and it is precisely the window in which someone is clicking around production checking their work. That is the same failure #195 fixed, pointed the other way: silent where a warning was needed, then confidently wrong where a customer can actually be charged. Telling someone nothing will be shipped above a live PayPal button is worse than saying nothing. The notice now describes the button instead of the shop, which is true in both configurations and stays visible in the one with two controls that do different things — where a customer most needs to be told they differ. Gating it on `!paypalClientId` would also have removed the false claim, by hiding the notice exactly there, which is the worse trade. The test for it was also not testing it. "Says so before the customer commits" seeded an address with `isDefault: true`, and the cart auto-selects the default on load — so an address was already selected and the button already rendered when it asserted. It would have passed with the notice moved inside the `selectedAddressId` guard, which is the regression it exists to catch. It now seeds no address and asserts the notice is up while the checkout button is absent, which states the property directly. Mutation-tested rather than assumed: moving the Alert inside that guard fails the new test, and would not have failed the old one. Verified: 3 end-to-end tests pass against a browser, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #203 Refs #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |