docs: design for React error boundaries (#62) #83

Merged

bermudalamb merged 6 commits from feature/62-error-boundary into main 2026-08-20 17:46:27 -05:00

Conversation 0 Commits 6 Files Changed 10

+1294

bermudalamb commented 2026-08-20 16:22:03 -05:00

Owner

Copy Link

Copy Source

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

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 self-assigned this 2026-08-20 16:22:03 -05:00

bermudalamb added this to the Code Quality and Hardening project 2026-08-20 16:22:03 -05:00

bermudalamb added 1 commit 2026-08-20 16:22:03 -05:00

docs: design for React error boundaries (#62) ... e8374e391a

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 added 2 commits 2026-08-20 16:46:39 -05:00

docs: correct the error-boundary design's fallback actions (#62) ... 683139b8d8

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>

docs: implementation plan for React error boundaries (#62) ... 94e2267eaf

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>

bermudalamb added 3 commits 2026-08-20 17:44:42 -05:00

feat(backend): log client-side render errors to the server (#62) ... c693672051

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>

fix(backend): stop a client error report forging log lines (#62) ... 7d227507b0

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>

feat(frontend): add an error boundary, its fallback, and error reporting (#62) ... e0ae2dcbcd

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>

bermudalamb merged commit 9599387f34 into main 2026-08-20 17:46:27 -05:00

bermudalamb deleted branch feature/62-error-boundary 2026-08-20 17:46:57 -05:00

bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-20 18:39:02 -05:00

bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-21 09:46:11 -05:00

bermudalamb moved this to Released in Code Quality and Hardening on 2026-08-21 12:11:50 -05:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Code Quality and Hardening

Released

Assignees

Clear assignees

calamb (Chelsea Lamb) bermudalamb (Thom Lamb)

No Assignees bermudalamb

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#83