bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity
Files
dbe6a0cf8f01351b7d87e9d5925459cd1b44db59
redefined-designs/docs/superpowers/specs/2026-08-20-error-boundary-design.md
T
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>
2026-08-20 18:32:27 -05:00

168 lines
14 KiB
Markdown
Raw Blame History

# React Error Boundaries — Design
**Issue:** [#62 — No React error boundary](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/62)
**Date:** 2026-08-20
**Status:** Implemented
## Goal
Stop a single render error from unmounting the whole storefront and leaving a blank white page, and make sure that when it happens somebody can find out.
## Why this one matters here
This project has been bitten repeatedly by failures that present as silence, and has designed against it each time: the storefront distinguishes "request failed" from "no items" because rendering an outage as an empty shop hid a production incident; the favorites filter answers 401 rather than an empty list; logout reports failure rather than appearing to succeed.
A missing error boundary defeats all of it one layer up. The storefront's careful "Couldn't load items" alert cannot render if the component that would render it has already thrown. A malformed API response, an unexpected `null`, or a bad `.map` anywhere in the tree produces a white page instead of any of the considered failure states.
## Decisions
Four decisions were open in the issue. All four are settled.
| Question | Decision |
| --- | --- |
| How many boundaries, and where | Three mount points: root, catalogue, modal block |
| Whether errors get reported anywhere | Yes — a server log endpoint, no storage |
| What the fallback offers | An action appropriate to each context; error detail in development only |
| How it gets verified | A development-gated throw trigger, exercised by end-to-end tests |
## Placement
```
StrictMode
ThemeModeProvider → CustomerAuthProvider → CartProvider → FavoritesProvider
Root → ConfigProvider
BrowserRouter
[EB "page"] full-page fallback
AppRoutes
<Routes location={backdrop}>
App
[EB "catalogue"] inline fallback
<Catalogue/>
[EB "modal"] antd Modal fallback
{modalPath === '/account' && <Account/>} …
```
**The root boundary sits inside `BrowserRouter`**, because `AppRoutes` is what it guards. Placing it outside would only additionally catch a throw from the router itself, which is not a realistic failure here.
**Every escape action is a hard navigation, not a `<Link>`.** This is worth stating because the obvious implementation is wrong: a React error boundary does not reset when the route changes. A fallback offering `<Link to="/">` would change the URL and go on rendering the fallback, which reads as the app being permanently broken. So Reload calls `window.location.reload()` and the two "leave this page" actions set `window.location.href`, both of which remount the tree and clear the error. An earlier draft of this design justified the boundary's placement by the fallback needing router context to link; that reasoning was wrong and the placement is justified above instead.
**The catalogue boundary is the one that earns its keep.** The likeliest throw in this application is a component rendering data from the API, and the item grid is where the most API data is rendered per page. Containing it there keeps the header, the cart badge, the filters and the footer alive, so the customer can still navigate rather than being handed one dead page.
**The modal boundary exists because the modal-route arrangement couples two independent trees.** `/account`, `/login`, `/register`, `/forgot-password` and `/reset-password` render as modals over the storefront as a backdrop. Without a boundary between them, a throw in `Account` blanks the storefront behind it and a throw in the storefront takes the open modal down with it. One boundary around the modal block separates the two in both directions.
### Deliberately excluded: an outermost boundary around the providers
An additional boundary outside `ThemeModeProvider` would catch a throw from the providers themselves. It is not included. Their render bodies are state, `useCallback`, `useMemo` and JSX, with no data mapping and no array indexing — the throw risk is remote. It would also sit outside `ConfigProvider`, so its fallback could not use antd and would need a second, hand-styled fallback to maintain for a case that is unlikely to occur.
If a provider does start doing real work later, this decision should be revisited rather than assumed still valid.
## Components
### `components/ErrorBoundary.tsx`
The only class component in the codebase — `getDerivedStateFromError` and `componentDidCatch` have no hook equivalent, so this cannot be a function component.
```
props: { children, context: 'page' | 'catalogue' | 'modal', fallback: (error: Error) => ReactNode }
```
`getDerivedStateFromError` records the error; `componentDidCatch` hands it to the reporter along with the component stack. The boundary itself knows nothing about antd and nothing about how reporting works, so it stays testable and the fallback stays the caller's business.
### `components/ErrorFallback.tsx`
One component, three containers. It renders a title, a set of actions, and — only under `import.meta.env.DEV` — `error.message`. It does not render the component stack; that still reaches the server log via `componentDidCatch`, which is where it is actually useful, so the omission from the fallback is a decision rather than a gap.
| Mount point | Title | Container | Actions |
| --- | --- | --- | --- |
| `page` | Something went wrong | Full-page antd `Result` | Reload, Back to the shop |
| `catalogue` | The item list didn't load | Inline, in place of the grid | Reload |
| `modal` | Couldn't open that | antd `Modal` | Close, returning to the storefront |
The three titles are deliberately distinct rather than one shared string. They tell a customer which part failed — the difference between "the shop is broken" and "the list didn't load but everything else works" — and they give the end-to-end tests an unambiguous locator for *which* boundary caught, which a shared title could not.
Keeping the development-only detail in one component means there is exactly one place where a decision about showing customers error detail lives, rather than three that can drift apart.
The modal fallback is a `Modal` rather than inline markup because the modal block renders after the routed content in the DOM. An inline fallback there would appear at the foot of the page, disconnected from the thing that failed.
### `errorReporting.ts`
```
reportClientError({ context, message, stack, componentStack, path })
```
Posts to `/api/client-errors` and ignores the outcome — `.catch(() => undefined)`. A reporter that throws inside `componentDidCatch` is the one failure this whole change must not introduce, so it swallows deliberately and that is the single place in this design where swallowing is correct.
### `components/DevThrow.tsx`
Throws when the current URL carries `?boom=<scope>` matching its own scope. Mounted inside each boundary as `{import.meta.env.DEV && <DevThrow scope="…" />}`, so Rollup eliminates both the element and its import from a production build.
The modal trigger is mounted as an unconditional sibling inside the modal boundary rather than inside one of the `modalPath === …` branches. That way `/?boom=modal` exercises it with the storefront rendered behind, which is the exact assertion the test needs to make, without depending on `/account` first resolving a session.
This is test-only code adjacent to a production path, which is exactly what the project's test/production boundary rule exists for. The gate is therefore verified rather than trusted: the production build is grepped for a distinctive marker string and must contain zero occurrences.
## Backend: `POST /api/client-errors`
A new router mounted in `app.ts`, taking `{ context, message, stack, componentStack, path }`.
**Validation.** `context` must be one of `page`, `catalogue`, `modal`; anything else is a 400. This follows `parseItemFilters`, which refuses a malformed filter rather than coercing it — a report from an unknown context means the client and server disagree about something, and that is worth surfacing rather than logging under a guessed label.
**Truncation**, applied before logging rather than rejecting, because a report that is too long is still worth having:
| Field | Limit |
| --- | --- |
| `message` | 500 |
| `stack` | 1000 |
| `componentStack` | 1000 |
| `path` | 200 |
`stack` and `componentStack` were originally 4000 each. The endpoint is unauthenticated and its rate limiter is keyed per address, which bounds a single render loop but not a distributed writer: a few hundred cheap source addresses sending accepted reports is tens of megabytes a day against Docker's default `json-file` log driver, which has no size cap of its own — a full disk takes the database down with it. 1000 characters is roughly fifteen stack frames, enough to identify a throw, and keeps the worst-case record under 3 KB.
**Log rotation.** `docker-compose.qa.yml` now sets a `logging` stanza on the app service (`json-file` driver, `max-size: 10m`, `max-file: "3"`), which caps this risk for the QA stack. Production is deployed from a Portainer stack that does not live in this repository, so this change does not reach it — the production stack needs the same logging options added directly in Portainer, or this mitigation is only half applied.
**Rate limiting needs its own limiter, not the existing one.** `rateLimit.ts` documents that `passwordResetRequestLimiter` is keyed on caller *and* email, and that "applying the same limiter to an endpoint without one collapses every caller into a single `ip:` bucket". This endpoint has no email, so it gets a separate limiter keyed on `req.ip` alone at 30 requests per 15 minutes. `app.set('trust proxy', 1)` is already in place, so `req.ip` is the real client address from `X-Forwarded-For` rather than the proxy's — per-customer, not per-deployment. Hitting the limit is harmless: the client ignores the response either way.
**Logs** with a `[client-error]` prefix and returns **204**. No body, because the client ignores the response.
**Unauthenticated**, because render errors happen to signed-out visitors and an error report that requires a session would miss exactly the cases worth knowing about.
**No storage.** The container log is where this project's operational visibility already lives. A table, a retention policy and an admin screen are a subsystem, and were rejected as larger than the rest of this issue.
## Verification
**End-to-end**, in a new `error-boundary.spec.ts`:
1. `/?boom=page` renders the full-page fallback rather than a blank document.
2. `/?boom=catalogue` renders the inline fallback while the wordmark and the theme switch remain visible — the specific claim that a bad item no longer takes down navigation.
3. `/?boom=modal` leaves the storefront rendered behind the modal fallback.
4. A caught error actually reaches `/api/client-errors`, asserted by observing the request from the page rather than by trusting that the reporter was called.
Assertions are on the three distinct fallback titles, so a test cannot pass because *some* boundary caught when the wrong one did.
**Backend integration**, for the new route: a well-formed report returns 204, an unknown `context` is a 400, and an oversized message is truncated rather than rejected. The rate limiter is deliberately **not** asserted in the integration suite — its store is in-memory and process-wide, so a test that exhausts the allowance leaks that state into every later test keyed on the same address, and the order-dependent failure it produces later would cost more than the assertion is worth. The limit is verified by reading the configuration, the way the other limiter is.
**Production safety**, both directions, as #61 did for its instrumentation gate: a normal `npm run build` produces a bundle containing zero occurrences of the `DevThrow` marker, and the development server does throw when the parameter is present. A gate that is silently always-off looks identical to a gate that works.
## Risks to confirm during implementation
**React 18 StrictMode double-invokes render in development**, so a caught error may be reported twice locally. Harmless in production, where StrictMode's double render does not apply, but the end-to-end assertions must not depend on a single report.
**Vite's error overlay** may intercept runtime errors in the development server that Playwright drives. If it renders over the page it will block clicks and the tests will fail in a way that looks like the boundary not working. If that happens, the overlay is disabled for the test run rather than the tests worked around.
## A note on antd import style
The new frontend files use deep imports from `antd/es/*`, which is this project's documented convention and the style every recently-added file follows. Not `antd/lib/*`: #65 records that as the mistake which loads a second React context and breaks `ConfigProvider`. The nine files still using the `antd` barrel are #65's business, not this change's.
## Corrections found during implementation
Two things this design got wrong, recorded here rather than left for the next reader to rediscover.
**antd's `Result` renders its title as a plain `<div>`, with no heading semantics.** This design specified both a `Result` with a string title and end-to-end assertions using `getByRole('heading')` — two requirements that contradict each other, so the tests could never have passed as written. The fix was to give the title a real heading rather than to loosen the assertion: a page whose entire content is an error message, with nothing carrying heading semantics, offers a screen-reader user navigating by headings nothing at all to find. `ErrorFallback` now wraps the title in `Typography.Title`, and the assertion stands as it was.
**`import.meta.env` had no ambient type declaration.** Nothing in this application had used `import.meta.env` before — the only Vite environment gating lived in `vite.config.ts` — so `tsc` rejected the `DEV` check outright with TS2339. `frontend/src/vite-env.d.ts` was added, which is the standard one-line Vite reference and pulls in no new dependency.
One thing the design flagged as a risk did not materialise: Vite's error overlay never interfered with the Playwright run, so `vite.config.ts` was left alone.
## Out of scope
Error reporting to an external service, persisting errors, alerting, and a frontend unit-test suite — the last belongs to #72, which already owns it.
Reference in New Issue View Git Blame Copy Permalink