bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

No React error boundary — one render error blanks the entire storefront #62

New Issue
Closed
opened 2026-08-19 11:05:27 -05:00 by bermudalamb · 1 comment
bermudalamb commented 2026-08-19 11:05:27 -05:00
Owner
Copy Link
Copy Source

Found during a Microsoft/React/SonarQube best-practices review.

Problem

There is no error boundary anywhere in frontend/src — no componentDidCatch, no getDerivedStateFromError, no boundary component.

In React 18, an error thrown during render that no boundary catches unmounts the entire component tree. The customer gets a blank white page. No message, no retry, nothing in the UI indicating anything went wrong.

Why this one matters here specifically

This project has repeatedly been bitten by failures that present as silence rather than as errors, and has deliberately 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, so a signed-out visitor is not told they have no favorites.
  • Logout reports failure rather than appearing to succeed.

A missing error boundary is the same class of problem one layer up, and it defeats those protections: 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 rather than any of the considered failure states.

The modal-route arrangement raises the stakes slightly. /account, /login and the rest now render the storefront as a backdrop, so a throw in the storefront takes the modal with it.

Suggested approach

A boundary near the root, inside the providers so it can render themed chrome, showing a readable message and a reload action — and, for the storefront specifically, one boundary around the item grid so a bad item cannot take down the header and navigation with it.

Worth deciding:

  • How many boundaries, and where. One at the root is the minimum. Per-route or per-region boundaries degrade more gracefully but multiply the fallback UI to maintain.
  • Whether errors get reported anywhere. A boundary that only shows a message still leaves nobody knowing it happened. There is no error reporting in this project at all today, so "the customer sees a message and we never find out" is only a partial fix.
  • What the fallback offers. Reload is the obvious action; returning to the storefront may be better for a failure inside a modal route.

Severity

Medium-high. Low likelihood per-render, but the failure is total and silent, and it undoes protections this project has already paid for once.

Found during a Microsoft/React/SonarQube best-practices review. ## Problem There is no error boundary anywhere in `frontend/src` — no `componentDidCatch`, no `getDerivedStateFromError`, no boundary component. In React 18, an error thrown during render that no boundary catches unmounts the **entire component tree**. The customer gets a blank white page. No message, no retry, nothing in the UI indicating anything went wrong. ## Why this one matters here specifically This project has repeatedly been bitten by failures that present as silence rather than as errors, and has deliberately 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, so a signed-out visitor is not told they have no favorites. - Logout reports failure rather than appearing to succeed. A missing error boundary is the same class of problem one layer up, and it defeats those protections: 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 rather than any of the considered failure states. The modal-route arrangement raises the stakes slightly. `/account`, `/login` and the rest now render the storefront as a backdrop, so a throw in the storefront takes the modal with it. ## Suggested approach A boundary near the root, inside the providers so it can render themed chrome, showing a readable message and a reload action — and, for the storefront specifically, one boundary around the item grid so a bad item cannot take down the header and navigation with it. Worth deciding: - **How many boundaries, and where.** One at the root is the minimum. Per-route or per-region boundaries degrade more gracefully but multiply the fallback UI to maintain. - **Whether errors get reported anywhere.** A boundary that only shows a message still leaves nobody knowing it happened. There is no error reporting in this project at all today, so "the customer sees a message and we never find out" is only a partial fix. - **What the fallback offers.** Reload is the obvious action; returning to the storefront may be better for a failure inside a modal route. ## Severity Medium-high. Low likelihood per-render, but the failure is total and silent, and it undoes protections this project has already paid for once.
bermudalamb added this to the Code Quality and Hardening project 2026-08-19 11:37:56 -05:00
bermudalamb moved this to In Progress in Code Quality and Hardening on 2026-08-20 15:44:54 -05:00
bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-20 17:46:59 -05:00
bermudalamb commented 2026-08-20 18:28:44 -05:00
Author
Owner
Copy Link
Copy Source

Implemented on feature/62-error-boundary — 10 commits, not pushed

Design in docs/superpowers/specs/2026-08-20-error-boundary-design.md, plan in docs/superpowers/plans/2026-08-20-error-boundary.md. Both are updated in place where implementation proved them wrong, which happened three times — recorded below rather than quietly corrected.

What landed

Three boundaries, one ErrorBoundary class, one ErrorFallback in three containers.

Mount Catches Fallback
Root, inside BrowserRouter anything the inner two miss Full page: "Something went wrong", Reload, Back to the shop
Around <Catalogue> in App.tsx a throw rendering the item grid Inline: "The item list didn't load", Reload — header, cart badge and filters stay alive
Around the modal block in main.tsx a throw in /account, /login and the rest antd Modal: "Couldn't open that", Close

POST /api/client-errors logs each caught error with a [client-error] prefix and returns 204. No storage — the container log is where this project's operational visibility already lives.

DevThrow throws on ?boom=<scope>, mounted only behind import.meta.env.DEV. Checked in both directions, as #61 checked its instrumentation gate: the dev server serves it, and a production bundle greps to zero occurrences of its marker.

The three things the design got wrong

antd's Result renders its title as a plain <div>. The design specified both a Result with a string title and end-to-end assertions using getByRole('heading') — two requirements that contradict each other, so those tests could never have passed. Fixed by giving the title real heading semantics rather than by loosening the assertion: a page whose entire content is an error message, with nothing carrying a heading, gives a screen-reader user navigating by headings nothing at all to find. The assertion was right; the component was wrong.

A boundary does not reset on client-side navigation — caught twice, the second time seriously. The first draft justified the root boundary's placement by the fallback needing router context to link, which would have left the fallback on screen after the URL changed. Corrected to hard navigations before implementation. But the same mistake survived one level down: the catalogue boundary keeps Clear filters alive, and clicking it only changed the URL, so the fallback went on rendering over a catalogue that would by then have loaded fine. The controls the boundary exists to preserve could not actually recover the page. ErrorBoundary now takes an optional resetKey; the catalogue boundary passes the filter key. The other two deliberately do not take one, recorded on the prop so nobody completes the pattern by symmetry.

import.meta.env had no ambient type declaration. Nothing in the app had used it before — the only Vite gating lived in vite.config.ts — so tsc rejected the DEV check outright. vite-env.d.ts added.

Three defects review caught that would have shipped

Log forging. The endpoint is unauthenticated and reachable without the frontend, and truncating fields is not enough: a newline embedded in any field forged what read as a second [client-error] record. Every field is now stripped of C0 control characters and DEL. Sanitising happens before truncation, because the substitution is 1-for-1 and so cannot push the result past the limit — an escaping scheme that expanded characters would need the opposite order, which is noted next to the code rather than left to be rediscovered by reversing it.

The reporter could throw. Its comment promised it could not, and JSON.stringify(report) ran synchronously as an argument to fetch, before the promise carrying the .catch existed. Since React does not guarantee the value handed to componentDidCatch is a real Error, an object with a circular message would have thrown out of the one thing that exists to stop throws.

A falsy throw defeated the boundary entirely. throw null is legal. Branching on the error object alone treated it as no error, re-rendered the children, threw again, and would eventually have taken the root down — a blank page, the exact outcome this issue exists to prevent. Now tracks hasError separately.

One thing needing your hand outside this repo

Each accepted report wrote about 8.7 KB, the endpoint is unauthenticated, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as intended, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, and docker-compose.qa.yml caps and rotates the log.

Production is a Portainer stack outside this repository and does not get that change. It needs max-size and max-file set on the app service directly, or the mitigation is only half applied.

Verification

Every number observed, not assumed, against a freshly created database:

  • Backend build clean, lint 0 errors, 79 unit, 144 integration
  • Frontend build clean, lint 0 errors / 31 warnings — unchanged from the branch baseline
  • 87 end-to-end, 4 of them new: each boundary catches rather than blanking, only the nearest boundary fires, 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
  • Dev-only trigger: absent from the production bundle, present on the dev server

Deliberately not done

An outermost boundary around the providers — their render bodies are state and JSX with no data mapping, and it would sit outside ConfigProvider needing a second hand-styled fallback. Flagged in the design to revisit if that stops being true. No error reporting to an external service, no persistence, no alerting. No frontend unit suite — that is #72's.

Not pushed, per the usual arrangement.

## Implemented on `feature/62-error-boundary` — 10 commits, not pushed Design in `docs/superpowers/specs/2026-08-20-error-boundary-design.md`, plan in `docs/superpowers/plans/2026-08-20-error-boundary.md`. Both are updated in place where implementation proved them wrong, which happened three times — recorded below rather than quietly corrected. ## What landed Three boundaries, one `ErrorBoundary` class, one `ErrorFallback` in three containers. | Mount | Catches | Fallback | | --- | --- | --- | | Root, inside `BrowserRouter` | anything the inner two miss | Full page: "Something went wrong", Reload, Back to the shop | | Around `<Catalogue>` in `App.tsx` | a throw rendering the item grid | Inline: "The item list didn't load", Reload — header, cart badge and filters stay alive | | Around the modal block in `main.tsx` | a throw in `/account`, `/login` and the rest | antd `Modal`: "Couldn't open that", Close | `POST /api/client-errors` logs each caught error with a `[client-error]` prefix and returns 204. No storage — the container log is where this project's operational visibility already lives. `DevThrow` throws on `?boom=<scope>`, mounted only behind `import.meta.env.DEV`. Checked in both directions, as #61 checked its instrumentation gate: the dev server serves it, and a production bundle greps to **zero** occurrences of its marker. ## The three things the design got wrong **antd's `Result` renders its title as a plain `<div>`.** The design specified both a `Result` with a string title *and* end-to-end assertions using `getByRole('heading')` — two requirements that contradict each other, so those tests could never have passed. Fixed by giving the title real heading semantics rather than by loosening the assertion: a page whose entire content is an error message, with nothing carrying a heading, gives a screen-reader user navigating by headings nothing at all to find. The assertion was right; the component was wrong. **A boundary does not reset on client-side navigation** — caught twice, the second time seriously. The first draft justified the root boundary's placement by the fallback needing router context to link, which would have left the fallback on screen after the URL changed. Corrected to hard navigations before implementation. But the same mistake survived one level down: the catalogue boundary keeps **Clear filters** alive, and clicking it only changed the URL, so the fallback went on rendering over a catalogue that would by then have loaded fine. The controls the boundary exists to preserve could not actually recover the page. `ErrorBoundary` now takes an optional `resetKey`; the catalogue boundary passes the filter key. The other two deliberately do not take one, recorded on the prop so nobody completes the pattern by symmetry. **`import.meta.env` had no ambient type declaration.** Nothing in the app had used it before — the only Vite gating lived in `vite.config.ts` — so `tsc` rejected the `DEV` check outright. `vite-env.d.ts` added. ## Three defects review caught that would have shipped **Log forging.** The endpoint is unauthenticated and reachable without the frontend, and truncating fields is not enough: a newline embedded in any field forged what read as a second `[client-error]` record. Every field is now stripped of C0 control characters and DEL. Sanitising happens *before* truncation, because the substitution is 1-for-1 and so cannot push the result past the limit — an escaping scheme that expanded characters would need the opposite order, which is noted next to the code rather than left to be rediscovered by reversing it. **The reporter could throw.** Its comment promised it could not, and `JSON.stringify(report)` ran synchronously as an argument to `fetch`, before the promise carrying the `.catch` existed. Since React does not guarantee the value handed to `componentDidCatch` is a real `Error`, an object with a circular `message` would have thrown out of the one thing that exists to stop throws. **A falsy throw defeated the boundary entirely.** `throw null` is legal. Branching on the error object alone treated it as no error, re-rendered the children, threw again, and would eventually have taken the root down — a blank page, the exact outcome this issue exists to prevent. Now tracks `hasError` separately. ## One thing needing your hand outside this repo Each accepted report wrote about 8.7 KB, the endpoint is unauthenticated, and Docker's default `json-file` driver has no size cap — so the rate limiter bounded a render loop, as intended, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, and `docker-compose.qa.yml` caps and rotates the log. **Production is a Portainer stack outside this repository and does not get that change.** It needs `max-size` and `max-file` set on the app service directly, or the mitigation is only half applied. ## Verification Every number observed, not assumed, against a freshly created database: - Backend build clean, lint 0 errors, **79 unit**, **144 integration** - Frontend build clean, lint **0 errors / 31 warnings** — unchanged from the branch baseline - **87 end-to-end**, 4 of them new: each boundary catches rather than blanking, only the *nearest* boundary fires, 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 - Dev-only trigger: absent from the production bundle, present on the dev server ## Deliberately not done An outermost boundary around the providers — their render bodies are state and JSX with no data mapping, and it would sit outside `ConfigProvider` needing a second hand-styled fallback. Flagged in the design to revisit if that stops being true. No error reporting to an external service, no persistence, no alerting. No frontend unit suite — that is #72's. Not pushed, per the usual arrangement.
bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-21 09:46:14 -05:00
bermudalamb moved this to Released in Code Quality and Hardening on 2026-08-21 12:11:48 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#62