5c907fcf9a86641ffcf24eee94a36859055fc1d2
187
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
984d91f00a |
Merge pull request 'Feature/63 admin gate' (#94) from feature/63-admin-gate into main
Reviewed-on: #94 |
||
|
bermudalamb
|
89fc7c5c1b |
feat(backend): add an application-layer gate to the admin API (#63)
Authorization for the admin panel and the admin API has lived entirely in one auth_request regex in an Nginx Proxy Manager config outside this repository. That control is real and it works — nothing is publicly exposed today — but it is invisible from the code, untested here, and not reviewed when this code changes. Three things follow from that, and the first is the one worth the change. An admin route added at a path the regex does not match is unprotected the moment it is written, and nothing in Express indicates that. Anything reaching the published container port directly bypasses authentik entirely. And locally there is no gate at all, so no developer ever sees the boundary being enforced. requireAdminGate is attached to each admin router rather than to a path prefix, which is what makes it useful rather than merely redundant with the proxy. An admin router added later at some other path inherits the gate; because the proxy only injects the header on paths its regex matches, that router refuses on its first request instead of being quietly public. A 403 in that situation is the boundary reporting that it has drifted. The gate is optional, and unset means exactly today's behaviour. That keeps local development and all 113 existing admin test call sites working untouched, and means shipping the image before configuring the proxy cannot take the admin panel down. What it does not do is stay silent about it: the server warns at boot when the gate is inactive, naming what is unprotected. This project has been bitten repeatedly by controls that report success while doing nothing, and an unconfigured gate should be a visible choice rather than an invisible one. An empty value is treated as unset rather than as a secret, because enforcing an empty secret would admit any caller sending an empty header. Comparison is timing-safe over SHA-256 digests of both sides: timingSafeEqual throws on buffers of unequal length, so comparing raw values would turn a short header into a 500 rather than a 403, and a length check first would leak the secret's length. Turning it on requires the secret in two places at once — the stack environment and a proxy_set_header line on the gated location in NPM. Setting only one gives 403s until the other catches up. That coupling, and the three consequences above, are now written into the README beside the deployment section, since none of it is visible from the code. Verified over real HTTP as well as in tests. Booting without the secret logs the warning and serves admin normally; booting with it returns 403 for a missing header, 403 for a wrong one, 200 for the right one, and leaves the public storefront at 200 throughout, with each refusal logged distinguishably and without echoing the value it was sent. 8 new unit tests, 9 new integration tests covering every admin router separately — a correct middleware nobody mounted would pass the unit tests and protect nothing. 106 unit and 153 integration passing, lint 0 errors and 8 warnings unchanged. Refs #63 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
105bf141f5 |
Merge pull request 'feat: stage new items as pending until an admin publishes them (#90)' (#93) from feature/90-pending-status into main
Reviewed-on: #93 |
||
 bermudalambandClaude Opus 5
|
ecc2219fa5 |
feat: stage new items as pending until an admin publishes them (#90)
An item used to be live on the storefront the instant it was created. Now it starts pending, and a customer sees it only once it is published. The migration changes the column default and nothing else. Backfilling would un-publish the entire live catalogue, which is the one thing it must not do. Hiding a pending item took four separate changes, not one, and that is the part worth knowing. The storefront's item routes had no status filter at all — sold items are listed and rendered with a Sold badge deliberately — so pending could not be expressed as one more optional filter. GET /api/items now carries an exclusion the caller cannot opt out of; GET /api/items/:id carries the same, because hiding an item from the list while still serving it by id would leave it reachable to anyone who kept a link; and GET /api/filters excludes pending from both aggregates it computes. That last one is the least obvious: a pending item would have inflated its tags' counts, so a customer would read "Rare (1)", filter by it, and be told nothing matches — and its price would have stretched the slider to a range no visible item occupies. The tag count is computed over the joined items rather than filtered with a WHERE. A WHERE would have dropped the row for a tag whose only item is pending, and the tag would have vanished from the drawer instead of showing zero. There is a test for exactly that, because the first version of this query had that bug. parseItemFilters is shared by the storefront and admin routes, so 'pending' parses on both. The public route refuses it explicitly rather than answering with an empty list, which would read as "no items match" instead of "you may not ask that". The storefront's URL reader is deliberately left not accepting it either, with a comment saying so, since a request guaranteed to fail is not worth constructing. Publishing is the existing mark-available: same transition, same UPDATE, so the admin UI labels that button "Publish" when the item is pending rather than adding a second endpoint that does the same thing. Unpublish is new and is not symmetrical — it is refused for a reserved item, which someone is holding in their cart right now, and for a sold one, which is a record of something that happened rather than a draft. Both refusals name their reason, and the buttons are hidden in those states so the refusal is not how you find out. Changing a column default has reach, and it surfaced eight test fixtures that silently depended on it. Each is now explicit about the status it wants rather than inheriting one — better practice regardless, and immune to the next default change. Two tests also used 'pending' as their example of an *unknown* status; both would have quietly become tautologies, so they now use one that is genuinely unknown. Verified: 98 unit, 160 integration and 94 end-to-end passing, the last on a freshly created container. One earlier run showed a single failure in favorites.spec.ts; it passes in isolation and on a clean container, and is the cross-spec interference already recorded against the suite rather than anything from this change. Refs #90 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
adf480faf6 |
Merge pull request 'feat(frontend): preview an inventory item as a customer sees it (#89)' (#91) from feature/89-item-preview-panel into main
Reviewed-on: #91 |
||
|
bermudalambandClaude Opus 5
|
03f08074d1 |
feat(frontend): preview an inventory item as a customer sees it (#89)
Clicking an item's name in the admin Inventory opens a drawer rendering the real storefront ItemCard for it, so the way a listing will look can be checked without publishing it and going to see. The name cell is a link-styled button rather than a clickable cell, so it stays reachable by keyboard and announces itself as an action. Admin already imports Item from ../api, the same type the storefront uses, so the row object goes straight into the card with no adapter and nothing to drift. The part that needed care is that ItemCard is not a passive component. It wires into the cart and favorites contexts and has working buttons, and both providers wrap the whole app — so a naive preview would have been fully functional, and an admin browsing inventory could have added their own stock to their own cart. On a one-of-a-kind catalogue that reserves the item and takes it off sale. ItemCard therefore takes an optional preview prop that short-circuits its two click handlers. Those two are the only entry points, so guarding them also covers the shared auth modal and the favorite-alerts consent prompt hanging off them. Deliberately not `disabled` on the buttons. A disabled antd button renders in a different colour with a different cursor and no hover, and the whole point of this panel is to show what a customer will actually see. The controls keep their normal appearance and their correct state for the item's status; only the handlers stop. The comment on the prop says so, because "simplifying" this to a disabled button would quietly defeat the feature while appearing to implement it. Four end-to-end tests, two of which are the ones worth having. Clicking Add to Cart in the preview must do nothing — asserted by the sign-in prompt never appearing, which a real click on a signed-out card always raises, so its absence proves the handler stopped before doing any work. And the storefront card must still be live where it is actually used, or this change would have quietly broken buying things. ItemCard's props are now Readonly, which was an existing lint warning on a file this change already touches: frontend warnings drop from 31 to 30. Verified: build clean, lint 0 errors, 91 end-to-end tests passing against a freshly created database. Refs #89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
e3d5475fa6 |
Merge pull request 'feat: let QA send real email, guarded by a recipient allowlist (#87)' (#88) from feature/87-qa-mail-allowlist into main
Reviewed-on: #88 |
||
|
bermudalambandClaude Opus 5
|
0c90e18205 |
feat: let QA send real email, guarded by a recipient allowlist (#87)
QA has never been able to send mail. The compose file set no SMTP variables and the mailer skips sending when it finds none, which was deliberate — a QA run must not be able to email a real customer if a fixture ever holds a real address. The cost is that four customer-facing flows have never been exercised anywhere but production: verification, password reset, favorite-sold alerts, and the cart-reminder cron that already has a known silent failure mode. MAIL_ALLOWLIST replaces the blanket mute. Unset means unrestricted, which is production and must stay so. Set means only matching recipients are delivered to; anything else is skipped with a [mail-blocked] warning naming the address and subject. An entry is either a full address, which also covers its plus-suffixed variants, or @domain for every mailbox there — plus-addressing is how these tests get written, and nobody should have to edit an allowlist to invent a new suffix mid-run. The guard sits in the mailer, not at the four call sites, so every sender is covered by construction and a fifth added later cannot bypass it by forgetting. It skips rather than throws: three callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 on the signup path. The flow under test finishes and the log says why no mail arrived, which is exactly what was missing when QA was simply muted. Two details are load-bearing enough to state. Comparison is exact equality on both halves of the address rather than a suffix test, so a lookalike domain ending in an allowed one cannot get through — there is a test for that specifically. And a present-but-empty value refuses everyone rather than allowing everyone: writing MAIL_ALLOWLIST= expresses an intent to restrict, and reading it as "no restriction" would turn a typo into an outbound mail incident. This inverts the failure mode, so the allowlist is hardcoded in docker-compose.qa.yml rather than read from a stack variable. The safety property must not depend on remembering to set something in Portainer, where an omission would mean unrestricted sending from an environment full of fixtures. The comment says removing the line disables the restriction rather than the mail. QA points at Brevo, reusing the existing account rather than a separate QA sender — a deliberate choice that puts QA volume behind production's sending reputation and quota, acceptable for now. Host, port and secure are pinned in the compose because the mailer's fallbacks are Gmail's and Brevo needs 587 with STARTTLS; that mismatch fails at send time rather than at boot, which is #64's territory. Verified: 12 new unit tests on the matching function, which is where a mistake would actually be dangerous — 98 unit and 144 integration passing, lint 0 errors and 8 warnings unchanged, and the compose renders the expected values under docker compose config. Refs #87 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
d4d602da3e |
Merge pull request 'Feature/84 ipv6 rate limit key' (#86) from feature/84-ipv6-rate-limit-key into main
Reviewed-on: #86 |
||
|
bermudalamb
|
2f6e855596 |
fix(backend): stop IPv6 callers bypassing the password-reset rate limit (#84)
The QA stack has been logging ERR_ERL_KEY_GEN_IPV6 at every boot, and express-rate-limit was right to complain. keyByCallerAndEmail built its key from req.ip raw. For an IPv4 caller that is one address and the limiter worked as intended. For an IPv6 caller it is the full 128 bits — and a residential IPv6 customer is delegated an entire prefix and can source every request from a different address inside it at no cost. Keyed that way, each request counted as a new caller and the allowance of five per fifteen minutes never bound at all. That matters more here than it would elsewhere, because of what this limiter is for. Its own comment says it: without one, anyone can make the server send unlimited mail to any address they choose. For IPv6 clients there effectively was no limiter, while the code read as though there were. The caller half of the key now goes through express-rate-limit's ipKeyGenerator, which groups IPv6 by prefix and returns IPv4 unchanged. The helper's default is /56 rather than /64, and that default is kept deliberately: /56 covers a whole delegated site, so an attacker cannot escape their bucket by moving within their own allocation. It does mean several households behind one delegation share an allowance — acceptable only because the key also carries the email address, so they collide just when targeting the same account. The reasoning sits next to the code, because a future reader tightening it to /64 would silently reopen the hole. keyByCallerAndEmail is now exported so it can be tested directly. The limiter's allowance is still not asserted anywhere, and should not be: its store is process-wide, so a test that exhausts it leaks into every later test from the same address and fails something unrelated later. The key function is pure, and it is where the bug was. Verified by firing the guard rather than reasoning about it: building main and loading the module reproduces the ValidationError, and the same load with this change is silent. Seven new unit tests cover an IPv4 caller unchanged, two addresses in one delegation collapsing to a single key, separate delegations staying apart, an IPv4-mapped address keying the same as the plain IPv4 one, email normalisation, a non-string email, and a request with no address at all. 86 unit tests pass, 144 integration, lint 0 errors and 8 warnings — unchanged. Closes #84 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
cf45b7a8eb |
Merge pull request 'Feature/62 error boundary' (#85) from feature/62-error-boundary into main
Reviewed-on: #85 |
||
|
bermudalambandClaude Opus 5
|
71cbd142c3 |
fix: address the final review of the error boundaries (#62)
Five findings from a whole-branch review, two of which mattered. The catalogue boundary could not be recovered by the very controls it exists to keep alive. When the item grid threw, the header, filter chips and Clear filters stayed mounted — that was the point — but clicking Clear filters only changed the URL. A boundary does not reset on a client-side state change, so the fallback went on rendering over a catalogue that would by then have loaded perfectly well, and the only offered action reloaded the same failing URL. The shop read as permanently broken, which is the failure this whole change exists to prevent, reintroduced one level down. ErrorBoundary now takes an optional resetKey and clears itself when it changes; the catalogue boundary passes the filter key. The page and modal boundaries deliberately do not take one, because their escapes are hard navigations that remount the tree already — recorded on the prop so nobody completes the pattern by symmetry. The client-error endpoint could fill the disk. It is unauthenticated, each accepted report wrote about 8.7 KB, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as its comment claims, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, which is still around fifteen frames and cuts the worst case to under 3 KB, and the QA compose file caps and rotates the log. Production is a Portainer stack outside this repository and needs the same option applied there; noted in the design doc rather than left implied. Three smaller things. A falsy thrown value defeated the boundary entirely: throw null is legal, and branching on the error object alone treated it as no error, re-rendered the children, threw again, and would have taken the root down — a blank page, the one outcome this is all here to avoid. The boundary now tracks hasError separately and synthesises a real Error for non-Error throws. The modal fallback announced its title twice to a screen reader, once as the dialog's name and once as the heading inside it, so the redundant dialog title is gone. And the design doc claimed the development-only detail shows the component stack when it only ever showed the message; corrected, with a note that the stack still reaches the server log, which is where it is useful. Verified after all five: backend lint 0 errors, 144 integration tests, frontend lint 0 errors and 31 warnings, 87 end-to-end tests, all against a freshly created database. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
557703f86d |
docs: mark the error-boundary design implemented (#62)
Records the two things the design got wrong. antd's Result renders its title as a plain div, so the design's Result usage and its getByRole('heading') assertions contradicted each other and the tests could never have passed as written — resolved by giving the title real heading semantics rather than by loosening the assertion, because an error page with no heading leaves a screen-reader user navigating by headings nothing to find. And import.meta.env had no ambient declaration anywhere in the app, so the DEV gate did not type-check until vite-env.d.ts was added.
The Vite error overlay risk the design flagged did not materialise.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
f156cecd87 |
feat(frontend): mount error boundaries at the root, the item grid and the modals (#62)
Three mount points, so a render error costs the smallest part of the page it can. The catalogue boundary is the one that earns its keep. The likeliest throw in this app is a component rendering data from the API, and the item grid renders the most of it per page — contained there, the header, cart badge, filters and footer all survive, so a customer can still navigate instead of being handed one dead page. The modal boundary exists because the modal-route arrangement couples two independent trees. /account, /login and the rest render as modals over the storefront as a backdrop, so without a boundary between them a throw in Account blanks the storefront behind it and a throw in the storefront takes the open modal with it. One boundary separates them in both directions. Every escape action is a hard navigation rather than a Link. This is worth stating because the obvious implementation is wrong: a boundary does not reset when the route changes, so a Link would change the URL and go on rendering the fallback, which reads as the app being permanently broken. ErrorFallback changed too, outside this change's original scope and for a reason worth recording. antd's Result renders its title as a plain div with no heading semantics, so a page whose entire content is an error message offered a screen-reader user navigating by headings nothing at all to find. The title is now wrapped in Typography.Title. The tests assert a heading role and were right to; the component was what needed fixing, not the assertion. DevThrow throws on ?boom=<scope> and is mounted only behind import.meta.env.DEV, so Rollup drops it from a production build. Checked in both directions rather than trusted: the dev server serves it, and a production bundle greps to zero occurrences of its marker. A gate that is silently always-off looks identical to one that works. Verified: 87 end-to-end tests pass, 4 of them new — each boundary catches rather than blanking, the header survives a catalogue throw, the storefront survives a modal throw, and the report is observed reaching /api/client-errors on the wire rather than assumed. Build clean, lint 0 errors and 31 warnings. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3e9d9a57c0 |
fix(frontend): make the error reporter genuinely unable to throw (#62)
The comment claimed the reporter could not throw and the code did not deliver it. JSON.stringify(report) and the call to fetch both run synchronously, as arguments, before the promise carrying the .catch exists — so a throw from either escaped straight out of componentDidCatch, where nothing remains to catch it. The boundary that exists to stop errors would itself have been the thing that crashed. Not merely theoretical: React does not guarantee the value handed to componentDidCatch is a real Error despite the parameter's type, because code can throw anything. An object whose message or stack is circular makes JSON.stringify throw. The body is now wrapped in try/catch for the synchronous part, and the existing .catch still covers rejection once the request is in flight. Neither covers the other, so both are kept, and the comment now says so rather than asserting a guarantee the code did not make. Verified: build clean, lint 0 errors and 31 warnings, unchanged. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
9599387f34 |
Merge pull request 'docs: design for React error boundaries (#62)' (#83) from feature/62-error-boundary into main
Reviewed-on: #83 |
||
|
bermudalambandClaude Opus 5
|
e0ae2dcbcd |
feat(frontend): add an error boundary, its fallback, and error reporting (#62)
Three pieces, none of them mounted yet — the next change wires them into the tree. ErrorBoundary is the only class component in the codebase, because getDerivedStateFromError and componentDidCatch have no hook equivalent. It knows nothing about antd and nothing about how reporting reaches the server: the fallback arrives as a render prop, which is what lets one boundary serve a full page, an inline region and a modal without knowing which it is. ErrorFallback is the single place that decides whether a customer is shown a stack trace. The detail is gated on import.meta.env.DEV so a developer sees the throw immediately while a production bundle cannot render it at all — one decision in one file rather than the same judgement repeated at three mount points, where they would drift apart. reportClientError posts to the endpoint added earlier and deliberately swallows its outcome. That is the one place in this feature where swallowing is correct: it runs inside componentDidCatch, so a reporter that rejected would throw from the very thing that exists to stop throws, with nothing left to catch it. vite-env.d.ts was not in the plan and is needed. Nothing in this app had used import.meta.env before, so there was no ambient declaration for it and tsc rejected the DEV check outright. The standard one-line Vite reference fixes it, adds no dependency, and would have been needed by the next change regardless. Verified: build clean, lint 0 errors and 31 warnings, unchanged from the branch baseline. No unit tests, because the frontend has no unit suite — that gap belongs to #72, and these components are covered end to end by the next change. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
7d227507b0 |
fix(backend): stop a client error report forging log lines (#62)
Review of the endpoint found that truncating the report's fields is not enough. It is unauthenticated and reachable without the frontend, so a caller could embed a newline in any field and forge what reads as a second [client-error] record in the shared server log. Every field is now stripped of CR, LF and the other C0 control characters, plus DEL, each replaced by a single space, so one report is always exactly one log record. Sanitising happens before truncation rather than after. The substitution is 1-for-1, so it cannot change the string's length and clipping the sanitised value still guarantees the stored result never exceeds the limit. An escaping scheme that expanded a control character into several visible ones would need the opposite order to keep that guarantee, so the two are not interchangeable — recorded in a comment next to the code rather than left for someone to rediscover by reversing it. The check is a numeric code-point comparison rather than a regex over a control-character class. That is not style: the first attempt used one, and the hex escapes were corrupted into raw control bytes on the way into the file. Written this way the source never has to contain an escape sequence or a raw control character at all, and the file is verified free of both. The review also found the truncation boundary was never exercised — the only test sent 5000 characters against a 500 limit. Tests now cover a string of exactly the limit passing through untouched, one character over truncating, truncation of stack and componentStack rather than message alone, and a report full of newlines producing a single log line. Verified: 10 integration tests pass, up from 4, and lint reports no new warnings. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c693672051 |
feat(backend): log client-side render errors to the server (#62)
The frontend's error boundaries need somewhere to report to. A boundary that only shows a customer a message leaves nobody knowing it happened, which is the failure shape this project has designed against three times already. POST /api/client-errors takes a report, truncates its fields, logs it with a [client-error] prefix and returns 204. No storage: the container log is where this project's operational visibility already lives, and a table with a retention policy and an admin screen is a subsystem larger than the issue. An unrecognised context is a 400 rather than a log line under a guessed label, following parseItemFilters, which refuses a malformed filter instead of coercing it. Oversized fields go the other way and are truncated rather than refused, because an over-long report is still the only record of the failure. The endpoint gets its own rate limiter rather than reusing passwordResetRequestLimiter, whose comment already warns that its caller-and-email key collapses every caller into one shared bucket on an endpoint without an email. The new one takes the default key generator, which also avoids the ERR_ERL_KEY_GEN_IPV6 warning the custom key produces. Verified: 138 integration tests pass, 4 of them new, and 79 unit. The unit count rose by one without a test being written — routesAreWrapped.test.ts runs describe.each over the files in src/routes, so a new route file generates a case. The handler is synchronous and needs no asyncRoute wrapper. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
94e2267eaf |
docs: implementation plan for React error boundaries (#62)
Four tasks, each ending in an independently testable deliverable: the backend endpoint with its own rate limiter, the boundary and fallback components, the three mount points with end-to-end coverage, and the production-gate verification. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
683139b8d8 |
docs: correct the error-boundary design's fallback actions (#62)
Found while working the design into a plan: the obvious implementation of the fallback's escape actions is wrong, and the spec was recommending it. A React error boundary does not reset when the route changes. The original spec justified placing the root boundary inside BrowserRouter on the grounds that the fallback needed router context to offer a way back — but a fallback offering a Link would change the URL and go on rendering the fallback, which reads as the app being permanently broken rather than recovering. Every escape action is therefore a hard navigation: reload, or setting window.location.href. The placement is unchanged, but it is now justified by what the boundary guards rather than by reasoning that does not hold. Three consequences recorded while there. The three fallbacks get distinct titles rather than one shared string, so a customer learns which part failed and the tests get an unambiguous locator for which boundary caught. The modal throw trigger mounts as an unconditional sibling inside its boundary, so /?boom=modal exercises it with the storefront behind rather than depending on /account resolving a session first. And a fourth end-to-end test asserts the report actually reaches /api/client-errors by observing the request, rather than trusting the reporter was called. Also recorded: new files use antd/es deep imports, this project's documented convention — not antd/lib, which #65 notes loads a second React context and breaks ConfigProvider. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
e8374e391a |
docs: design for React error boundaries (#62)
Settles the four questions #62 left open, and records why each alternative lost. Three mount points rather than one: root, the catalogue, and the modal block. The catalogue boundary is the one that earns its keep, because the likeliest throw in this app is a component rendering API data and the item grid renders the most of it per page — containing it there keeps the header, cart badge and filters alive instead of handing the customer one dead page. The modal boundary exists because the modal-route arrangement couples two independent trees: without a boundary between them a throw in Account blanks the storefront behind it, and a throw in the storefront takes the open modal with it. Errors get reported to a new POST /api/client-errors that logs and returns 204, with no storage. A boundary that only shows a message leaves nobody knowing it happened, which is the exact failure shape this project has designed against three times already. A persisted store with an admin screen was rejected as a subsystem larger than the rest of the issue. Rate limiting needs its own limiter rather than the existing one. rateLimit.ts already documents that passwordResetRequestLimiter is keyed on caller and email, and that reusing it where there is no email collapses every caller into one shared bucket — so this endpoint gets a separate limiter keyed on req.ip, which is the real client address because trust proxy is already set. Recorded as rejected: an outermost boundary around the providers, which would sit outside ConfigProvider and need a second hand-styled fallback for a case that is remote — their render bodies are state and JSX with no data mapping. Flagged for revisiting if that stops being true. Also recorded: the rate limiter is deliberately not asserted in the integration suite, because its store is process-wide and a test that exhausts the allowance leaks into every later test keyed on the same address. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
8fef669358 |
Merge pull request 'Feature/81 technical debt' (#82) from feature/81-technical-debt into main
Reviewed-on: #82 |
||
|
bermudalamb
|
4bad40868c |
refactor(frontend): actually reduce Customers' complexity rather than relocate it (#81)
The previous commit claimed this one fixed. It was not: hoisting the confirm dialog to module level moved the reported line from 12 to 60, and I read that as the finding having moved to the hoisted function. It had not — line 60 was Customers() itself, still scoring exactly 16. The scan is what caught it, which is the argument for scanning rather than reasoning about what a rule will say. Two further attempts also failed to move the number, and both are worth recording because they were wrong about what cognitive complexity counts. Collapsing five branches on `disabling` into one copy object fixed the hoisted function but left Customers() at 16. Extracting ten ternaries out of the table's cell renderers into module-level components left it at 16 as well — the ternaries inside a render callback were never the weight. What actually carried the score was the drawer and the reserved-items dialog: two JSX bodies whose loading, empty and populated states are each a branch nested several levels inside the component. Extracting them as CustomerDetailPanel and ReservedItemsBody takes Customers() under the limit. The cell-renderer extraction is kept even though it did not move the metric. NameAndEmail, BooleanTag, ReservedCell and ToggleDisabledButton read better than the inline callbacks did, and BooleanTag removes a repetition the Status column was open-coding differently from Verified and Subscribed. Verified on a scan rather than by argument: technical debt 85 minutes to 5, code smells 14 to 1, and the one that remains is the S6478 false positive. ESLint holds at 31 warnings against a baseline of 35. End-to-end 83 pass. One thing found on the way: the e2e suite is not idempotent against a persistent database. Two runs against a database that had already served three produced two different pairs of failures; recreating it produced a clean 83. CI is unaffected because its Postgres is fresh per run, but locally the suite needs a new database rather than a repeated one. Refs #81 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5f9172512c |
refactor: clear the 85 minutes of technical debt (#81)
Thirteen of SonarQube's fourteen code smells, with the fourteenth argued as a false positive rather than coded around. Four of these were not stylistic. The Remove button in Cart's List.Item actions array carried no key, so React could not match it across renders and rebuilt it on every cart render. The cart, customer-auth and favorites providers each passed a freshly allocated object as their context value, which re-renders every consumer whenever the provider renders, cart or session unchanged — and these three wrap the entire storefront, so the fan-out was the whole tree. Two of them also rebuilt a Set on every render for good measure. All four now memoized on the values they actually derive from. The four cognitive-complexity findings wanted real restructuring rather than suppression. parseItemFilters splits into one helper per field, leaving the function with the order those helpers run in and the single rule that spans two fields; the order is preserved exactly, because a query wrong in two ways reports the first field and rearranging the calls would change which error a caller sees. adminCategories' PUT extracts the parent resolution — the existence check and the cycle check — into a resolver that returns the refusal rather than sending it. App's four-way render chain becomes a Catalogue component, which also removes two of the nested ternaries. Customers' confirm dialog moves to module level, since a function's cognitive complexity counts everything declared inside it and that dialog branches on `disabling` five times. The rest were mechanical: two more nested ternaries — a status-colour lookup and a pluralisation helper — and one type assertion that asserted the type the expression already had. Left alone: S6478 on CategoryTreeSelect's popupRender. That is antd's render prop, called as a function and spliced in, never mounted as a component type, so the destroy-the-subtree failure the rule describes cannot happen. Marked false positive in SonarQube with that reasoning rather than contorting the component around a rule that misread it. Verified rather than assumed. Backend unit 78 pass, integration 134 pass, end-to-end 83 pass, both workspaces build clean. ESLint warnings drop from 35 to 31 with no new file warning — the same React and SonarJS rules #60 turned on are what surfaced this backlog in the first place. Refs #81 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
931d8f8167 |
Merge pull request 'chore(ci): declare sonar.projectVersion so the new-code period means something (#79)' (#80) from feature/79-sonar-project-version into main
Reviewed-on: #80 |
||
|
bermudalamb
|
e6d9079097 |
chore(ci): declare sonar.projectVersion so the new-code period means something (#79)
The quality gate has been grading the entire codebase rather than what changed. The server's new-code period is PREVIOUS_VERSION, but sonar.projectVersion was never set anywhere — not here, not in the workflow — so every analysis recorded "not provided" and there was no previous version to diff against. SonarQube's fallback is to treat everything as new. The tell was visible in the measures all along: new_lines read 9460 against a total ncloc of 5496, and new_coverage tracked overall coverage to within two points. Both are what you would expect if "new code" meant "all code", and neither is surprising enough to notice unless you go looking. This is the fourth time the project has hit the same shape — a tool reporting a plausible number for something other than what was asked. Verified with a scan against the scratch key rather than by reasoning about the config. The analysis now records version 1.0.0, ncloc holds at 5557 so nothing was silently dropped, and new_lines falls from the whole codebase to 151 — the window is now the diff. One consequence worth expecting rather than discovering: a narrow window makes new_coverage volatile. The verification scan reported 0.0% on two lines to cover, because two uncovered lines is all it takes. The number will settle as commits accumulate and the window widens, but the gate will be jumpy for the first few merges, and it will not simply turn green on its own. Left at 1.0.0 to match both package.json files. Nothing enforces that they stay in step, so the comment says to move all three together. Refs #79 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
c2964482e0 |
Merge pull request 'docs: require an issue behind every branch and PR (#77)' (#78) from chore/77-branch-issue-convention into main
Reviewed-on: #78 |
||
|
bermudalamb
|
0b8419dffb |
docs: require an issue behind every branch and PR (#77)
The documented convention allowed dropping the issue number when no issue existed. The clause looks harmless — it was meant for trivial work — but in practice it turns "there is no issue yet" into "there is no issue ever", and two branches went that way this week: the app icon, merged as #70, and the header mark, which had to be filed retroactively as #76. Both were legitimate work that the tracker never heard about, and the convention is what permitted it. The cost is not bookkeeping. The issue is where the reasoning, the rejected alternatives, and the verification end up — the record that outlives the conversation that produced it. A branch with nothing behind it leaves that nowhere but a commit message. Everything else about the convention is unchanged: the type prefixes, the (#N) subject, the Closes #N body line, and the point that Gitea builds the link from the commit rather than the branch name. Closes #77 |
||
|
bermudalamb
|
95d36b0dab |
Merge pull request 'feat(frontend): show the RD mark beside the storefront wordmark' (#75) from feature/header-brand-mark into main
Reviewed-on: #75 |
||
|
bermudalamb
|
7b89023d5e |
feat(frontend): show the RD mark beside the storefront wordmark
Puts the monogram to the left of "Redefined Designs" in the site header. Inline SVG rather than an <img src="/favicon.svg"> for two reasons. The tile is drawn in currentColor so it follows the antd text token and inverts with the app's own theme switch — the favicon file inverts on prefers-color-scheme, which tracks the operating system and would disagree with the app whenever someone toggles the theme themselves. And the letters are knocked out with a mask rather than painted in the background colour, so the mark sits correctly on any surface instead of carrying a backdrop that only matches this header. The mask id is generated per instance, since two marks on one page would otherwise share an id and the second would reference the first. It is stripped to alphanumerics because React's generated ids contain colons. Marked aria-hidden: the wordmark beside it already says the name, so announcing it again would be noise. Verified by screenshotting the header in both themes rather than by reasoning about the colours. |
||
|
bermudalamb
|
0587c18307 |
Merge pull request 'Feature/61 coverage import' (#73) from feature/61-coverage-import into main
Reviewed-on: #73 |
||
|
bermudalamb
|
261d087a9c |
docs(ci): record the coverage pipeline contract and the CI identity gap
Two standing documents rather than one, because they are different kinds of thing: one is a contract the pipeline must keep, the other is work not yet done. The coverage contract names the seven requirements that keep SonarQube's number real, and what specifically breaks if each lapses. It exists because coverage does not fail loudly — it reports a smaller number, which looks exactly like tests covering less. That is the third time this project has met a tool that succeeds while measuring nothing, after #67 and #60, so the failure mode is written down alongside how to check the guard still fires. The identity document covers CI authenticating to SonarQube as admin rather than a restricted account, raised as a "Related" note in #61 and split out so a permissions change is not buried in a CI-config commit. It spells out the revoke step explicitly, since the workflow goes green one step earlier and stopping there leaves the old credential valid. |
||
|
bermudalamb
|
332c1e7cd0 |
feat(ci): import test coverage into SonarQube (#61)
SonarQube reported 0% coverage for 78 unit, 134 integration and 83 end-to-end tests, so the coverage-on-new-code gate — the most useful thing SonarQube offers a project this size — has been failing permanently while looking configured. It now reports 69.6%, verified by a real scan. Backend coverage comes from both suites, written to separate directories because jest writes coverage/lcov.info by default and the second run would silently overwrite the first. Both are needed rather than just the fast one: the unit suite alone reports 11%, because everything in src/routes is exercised by the integration suite. That suite is manual-only after hanging for 3h12m post-run, so it runs here with --forceExit and the job carries a hard timeout; jest confirmed during testing that it would otherwise have hung. The frontend had no unit tests at all, so its coverage comes from Playwright driving an istanbul-instrumented dev server, collected per test by an auto-fixture and merged with nyc. The 17 specs now import from a local fixtures module that re-exports @playwright/test, which is what lets the fixture attach without touching each test body. Instrumentation is gated behind COVERAGE=true and loaded by dynamic import, since vite-plugin-istanbul is ESM-only while vite.config.ts evaluates as CommonJS. Both directions were checked rather than assumed: a normal build contains no instrumentation, and the dev server instruments nested modules as well as top-level ones — the first attempt used an include glob of src/* which would have silently missed everything under src/admin and src/cart. coverage:report fails when nothing was collected instead of writing an empty report, and that guard was fired deliberately to confirm it works. This project has been bitten twice by tools succeeding while measuring nothing — SonarQube skipping the whole frontend and still exiting EXECUTION SUCCESS in #67, and an ESLint matcher silently matching no files during #60 — and coverage has exactly that shape: an uninstrumented dev server lets every test pass while gathering nothing, and the 0% that follows reads as lost coverage rather than broken collection. Worth knowing when reading the numbers: end-to-end coverage flatters. Istanbul marks a line covered when the browser ran it, so a component rendered during a test counts as covered with nothing asserting anything about it. Recorded in the design doc and the project context rather than left to be discovered. Also declares sonar.tests so test files are analysed under the test rule set rather than as production code. Closes #61 |
||
|
bermudalamb
|
7e4084a65f |
docs: design for importing test coverage into SonarQube (#61)
Records what #61 still needs after #67 delivered two of its four asks, and the two constraints that shape the rest: backend route logic is covered only by the integration suite, which is manual-only because of a post-run hang, and the frontend has no unit tests at all so its coverage has to come from instrumenting the app and collecting from Playwright. Also records the thing most likely to mislead later — end-to-end coverage marks a line covered when the browser merely ran it, so the frontend number will read considerably better than the testing behind it, and the 80% gate will be easier to clear on frontend changes than backend ones. Accepted deliberately, because the alternative leaves every frontend pull request failing a gate it cannot satisfy. Refs #61 |
||
|
bermudalamb
|
0e7fc83ea9 |
Merge pull request 'feat(frontend): add an RD monogram app icon' (#70) from feature/app-icon into main
Reviewed-on: #70 |
||
|
bermudalamb
|
b677378533 |
feat(frontend): add an RD monogram app icon
The app had no favicon at all, so every tab showed the browser's default globe. Adds an RD monogram as frontend/public/favicon.svg plus a 180x180 apple-touch-icon.png, and links both from index.html. The mark is the letters knocked out of a filled tile rather than drawn on their own. At 16px — which is the only size that really decides a favicon — a solid shape with a bright interior stays findable in a strip of twenty tabs, where bare strokes turn to grey fuzz. Every candidate was rendered at 16, 32, 64 and 128 in headless Chromium and looked at rather than trusted from the path data, which is what caught the two drafts that did not survive: one had the D's bowl extending past the viewBox and clipping flat, because a stroked arc reaches half the stroke width beyond its centreline; another shared a stem between a mirrored R and the D and read as a Cyrillic Я. Colours are the accent already defined in src/main.tsx, and the SVG inverts with the theme for the same reason that accent does. Chrome ignores media queries inside a favicon and keeps the light rendering; that is acceptable here rather than worked around, because the pale letters still carry the mark against a dark tab strip. The Apple icon is full-bleed with no inner corner radius, since iOS applies its own mask and would otherwise show a small rounded tile floating inside a larger rounded tile. Also sets theme-color for both schemes so the browser chrome on Android and in an installed PWA matches the app rather than fighting it. Verified: build clean, lint clean, and both files land in dist/, which the Dockerfile copies wholesale into the image's public directory. |
||
|
bermudalamb
|
733fcf3c2d |
Merge pull request 'fix(ci): make SonarQube actually analyse the frontend (#67)' (#69) from bugfix/67-sonar-frontend-skipped into main
Reviewed-on: #69 |
||
|
bermudalamb
|
ae8f0eb12c |
fix(ci): make SonarQube actually analyse the frontend (#67)
SonarQube was analysing none of frontend/src. All 34 files were indexed and reported ncloc 0, so the quality gate had been measuring roughly a third of the codebase while appearing to cover it. The scanner log gives the cause: frontend/tsconfig.json sets "moduleResolution": "bundler", which is correct for Vite, and the TypeScript bundled with SonarQube 9.9 predates 5.0 and rejects it. Building the frontend program throws, every frontend file is dropped, and the scan still exits EXECUTION SUCCESS — which is why a green job hid this indefinitely. Adds frontend/tsconfig.sonar.json, an analysis-only mirror that differs only in using "node", and points the scan at it via sonar.typescript.tsconfigPaths. The app's own tsconfig is deliberately untouched: "bundler" is right for the build, and changing it to satisfy an old analyser would let the tool dictate the build. The mirror cannot use `extends` — the old compiler validates the base file while reading it, so the error just moves to pointing at tsconfig.json. Verified locally against a scratch project: 59/59 files analysed, no skips. Analysed lines go from 2,069 to 5,481, code smells from 2 to 14, security hotspots from 3 to 4, and technical debt from 21 to 85 minutes. The frontend had been hiding twelve code smells and a hotspot, which is part of why the React problems behind #60 and #62 had to be found by hand. A standalone copy drifts, and drift here does not fail anything — it silently returns to skipping the frontend while reporting success. scripts/check-sonar-tsconfig.js compares the two and fails when they diverge in anything but moduleResolution, and runs before the scan so the scan is never what discovers it. Confirmed it catches drift by introducing some. Moves scan settings into sonar-project.properties at the repo root so a local scan and the CI scan analyse the same thing, leaving only the host and token in secrets. Adds scripts/scan-local.sh, which runs the scanner in Docker because it needs Java 11+ and the dev machine has Java 8, and which defaults to a scratch project key: the server is Community edition with no branch analysis, so any scan overwrites the single main analysis of whichever key it is given. Closes #67 |
||
|
bermudalamb
|
29e97f60d3 |
Merge pull request 'Feature/60 eslint' (#68) from feature/60-eslint into main
Reviewed-on: #68 |
||
|
bermudalamb
|
c058b3ed2e |
feat(ci): add ESLint to both workspaces with a defect-only CI gate (#60)
TypeScript's strict mode checks types and nothing else, so nothing enforced the React hook rules, the SonarJS rules, or unhandled-promise detection. Adds a flat config per workspace, a lint script in each, and a lint job in tests.yml. The rule selection is the substance of this change and is measured rather than guessed. A full-strength config reports 435 violations across 50 files, but 325 of those are the no-unsafe-* family from recommendedTypeChecked, every one downstream of pool.query() returning any rows and untyped fetch responses. Typing those boundaries is the whole of #65, so enabling the rules here would ship a linter whose output is three-quarters another issue's backlog — the reliable way to teach everyone to ignore lint output. This enables recommended plus the two type-aware rules that catch defects rather than describe type debt, which leaves 110 findings. Both configs downgrade every preset to a warning and then list the error rules explicitly at the bottom, so the CI gate is readable in one place instead of inferred from four presets' defaults. Errors are no-floating-promises, no-misused-promises, rules-of-hooks, exhaustive-deps and jsx-a11y/alt-text; everything else warns. No --max-warnings flag is needed because ESLint already exits non-zero on errors and zero on warnings. no-misused-promises runs with checksVoidReturn.attributes false, since onClick={async () => ...} is idiomatic React and safe when the handler catches its own errors — at the default it flags every antd button in the admin screens, 25 of its 28 hits, and a rule that is 89% noise gets switched off within a week. The 37 errors this surfaced were mostly not the mechanical fix they looked like. The plan assumed the 30 floating promises were fire-and-forget loaders that already handled their own failures, which was true of the one sampled when writing the design and false for most of the rest: Admin, Categories, Customers, Tags, Settings, Account and CustomerAuthContext all had no rejection handling at all, so `void` on them would have hidden real failures rather than annotated deliberate ones. Each of those loaders now catches and surfaces the failure before the call site voids it. The CustomerAuthContext one was a live bug — a rejected fetchMe left loading true forever, rendering as a permanent spinner instead of a signed-out page. Admin's load became a useCallback so its effect can name it honestly rather than suppress the dependency, Categories' drop handler was split so the function antd receives returns void as its type says, and Cart's effect now names refreshCartContext, which is a useCallback with an empty dependency list and so cannot re-run it. The only disable added is in asyncRoute, where returning a promise where Express expects void is the entire point of the wrapper and the promise cannot reject. Two of the issue's premises did not survive measurement, both recorded in the spec: exhaustive-deps flags 2 cases rather than the 10 inferred from empty dependency arrays, and the backend was already clean on the defect rules because #59 wrapped every async route. Verified: lint, build, 78 unit, 134 integration and 83 e2e all pass in both workspaces, and the CI gate was confirmed to fail by introducing a deliberate violation rather than by assuming the job is wired correctly. Closes #60 |
||
|
bermudalamb
|
3cb6a42fb3 |
docs: unwrap the ESLint design doc (#60)
Hard-wrapped at 100 columns, which assumes a viewer width neither Gitea's web UI nor the VS Code markdown preview has, so the wrap points landed mid-sentence for the person reading it. Paragraphs and list items are now one line each; tables, code fences and the header block keep their own breaks because those are structure rather than wrapped prose. Refs #60 |
||
|
bermudalamb
|
8e859e58ec |
docs: design for adding ESLint to both workspaces (#60)
Records the measurement the design rests on — 435 violations from a full-strength config, of which 325 are the no-unsafe-* family — and the decision not to enable recommendedTypeChecked, since those 325 all trace to untyped pool.query rows and fetch responses, which is the whole of #65. Also records two of the issue's premises that measurement contradicts: exhaustive-deps flags 2 rather than the 10 the issue inferred from empty dependency arrays, and the backend is already clean on the defect rules because #59 wrapped every async route. Refs #60 |
||
|
bermudalamb
|
259b3779c6 |
Merge pull request 'Feature/59 wrap async routes' (#66) from feature/59-wrap-async-routes into main
Reviewed-on: #66 |
||
|
bermudalamb
|
e58c8b4009 |
docs: the asyncRoute guarantee now holds everywhere, and is enforced (#59)
The note said the older route files were still unwrapped, which stopped being true with #59. Records where the guarantee now reaches — including the second `webhookRouter` and the globally-mounted `attachCustomer`, both of which an audit grepping for `router.` misses — and points at the test that enforces it, so the next person adding a route learns it from a failing build rather than from this file. |
||
|
bermudalambandClaude Opus 5
|
1d7aba2d60 |
fix(backend): route every async handler through the error middleware (#59)
Express 4 does not forward a rejected promise from an async handler, so an unwrapped async route never responds at all — the request hangs until the client gives up, nothing reaches the error middleware, and monitoring sees an open connection rather than a 500. That silence is the shape of the 2026-08-17 incident, where an unhandled rejection left every item query hanging and the storefront rendered it as an empty shop. `asyncRoute` was written in response, but it was only applied to some routes: 30 handlers added afterwards were still bare, including register, login, the whole cart, and PayPal checkout. Wraps all 30, plus two the issue's inventory missed. `attachCustomer` is a bare async middleware mounted globally in app.ts, so a rejection in its session lookup would hang every request in the application — including the 25 handlers that were already wrapped correctly, which meant the guarantee did not actually hold anywhere. The PayPal webhook registers on a second router named `webhookRouter`, so an audit grepping for `router.` walked straight past it. Adds a unit test that scans the route sources and fails on any registration whose handler is not wrapped. A convention already half-forgotten once will be forgotten again, and enforcement is what the issue asked for; ESLint would be the better home for it but there is no ESLint in this repo yet (#60). The test walks parens rather than lines, so it also catches a handler whose `async` sits on its own line, and it matches any `*Router` name rather than just `router` — the two ways the existing bare handlers escaped notice. It is deleted along with `asyncRoute` if the project moves to Express 5, which forwards rejections natively. No behaviour changes on the success path; the failure path turns a hung request into a logged 500. Closes #59 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
07247caab4 |
Merge pull request 'Feature/50 auth modal routes' (#58) from feature/50-auth-modal-routes into main
Reviewed-on: #58 |
||
|
bermudalamb
|
b5e3f8fc4a |
docs: record the shared auth form, its consent coupling, and the Node switch
Notes that sign-in now has one implementation shared by the auth routes and the cart prompt, and where to change it. Records the marketing consent wording as a cross-boundary duplication in the same class as TAG_COLORS, but with a sharper failure mode: the server stores its copy verbatim so the record says what the customer saw, and a drifted label defeats that silently rather than loudly. An end-to-end test now spans the two sides. Also records that switching the active Node version for a test run is fine provided it is switched back to 18.16.1 afterwards, and expands the modal-route entry to the full route list now that four auth routes use it. |
||
|
bermudalamb
|
5ebb366074 |
feat(auth): open sign-in and registration as modals over the page behind (#50)
/login, /register and /forgot-password rendered bare cards with no site header. They linked to each other and nowhere else, so a customer who clicked Log in from the storefront and changed their mind had no way back except the browser's back button. All four auth routes are now modals over the page the customer was already on, reusing the backdrop-location arrangement from #51: a direct visit or a link from an email opens over the storefront, so closing always lands somewhere real. They remain real routes, because /reset-password links to /login and customers may have bookmarks. Signing in or registering now returns the customer to the page behind, signed in, rather than moving them to /account. Someone who signs in while browsing wants to carry on browsing, and this is already how the cart and favorites prompts behave when they resume an interrupted action. The larger half of this is removing the duplication. Signing in existed twice — as these routes and again inside the prompt shown when a signed-out visitor adds to the cart or favorites something — and the two had already drifted. There were three different wordings of the marketing consent in circulation: the register page's, a shorter one in the prompt, and the string the server actually stores. The server keeps that text verbatim so the consent record says what the customer saw, which none of the three did. Both callers now render one shared AuthForm whose checkbox is the exact string the server records, and a test asserts that wording so it cannot drift again silently. Steps within the auth flow replace rather than push, so switching between tabs or stepping to password recovery leaves the whole detour as a single history entry and closing returns to where it started instead of walking back through every tab that was looked at. The privacy policy link opens in a new tab: following it in place would discard a part-filled signup form, and /privacy still has no way back of its own until #52. Test changes follow from the destination change rather than being incidental. Nineteen assertions across seven specs waited for /account after signing in; they now assert the header shows a signed-in customer, which is the condition actually being waited for. Modal submits are scoped to their dialog, because the storefront behind now offers a Log in button of its own and an unscoped locator matched both. Assertions that follow a server round-trip were given a realistic timeout — the 5s default is too tight for a bcrypt hash plus re-rendering the storefront behind the modal. Verified with 83 end-to-end tests, all passing, and type checking clean. No backend changes. Closes #50 |
||
|
bermudalamb
|
7e26acace3 |
Merge pull request '48 my account modal' (#57) from 48-my-account-modal into main
Reviewed-on: #57 |