From 71cbd142c3a0ef3da6e8d2b62f5786891655a96e Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 18:26:37 -0500 Subject: [PATCH] fix: address the final review of the error boundaries (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/routes/clientErrors.ts | 10 +++- .../clientErrors.integration.test.ts | 4 +- docker-compose.qa.yml | 10 ++++ .../specs/2026-08-20-error-boundary-design.md | 12 ++-- frontend/src/App.tsx | 7 +++ frontend/src/components/ErrorBoundary.tsx | 60 ++++++++++++++++--- frontend/src/main.tsx | 4 +- 7 files changed, 90 insertions(+), 17 deletions(-) diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts index 645a7f7..0acda30 100644 --- a/backend/src/routes/clientErrors.ts +++ b/backend/src/routes/clientErrors.ts @@ -10,8 +10,14 @@ const router = Router(); const CONTEXTS: readonly string[] = ['page', 'catalogue', 'modal']; const MAX_MESSAGE = 500; -const MAX_STACK = 4000; -const MAX_COMPONENT_STACK = 4000; +// Kept well short of the 4000 this endpoint originally used. The endpoint is +// unauthenticated and the rate limiter is per-address, so a distributed +// writer sending a few hundred cheap requests a day was still tens of +// megabytes against Docker's default json-file log driver, which has no size +// cap on its own. 1000 characters is roughly fifteen stack frames — enough to +// identify a throw — and keeps the worst-case record under 3 KB. +const MAX_STACK = 1000; +const MAX_COMPONENT_STACK = 1000; const MAX_PATH = 200; // True for CR, LF and every other C0 control character, plus DEL (the diff --git a/backend/tests/integration/clientErrors.integration.test.ts b/backend/tests/integration/clientErrors.integration.test.ts index 45bf76e..204520c 100644 --- a/backend/tests/integration/clientErrors.integration.test.ts +++ b/backend/tests/integration/clientErrors.integration.test.ts @@ -95,7 +95,7 @@ describe('POST /api/client-errors', () => { const res = await request(app).post('/api/client-errors').send({ context: 'page', message: 'short', - stack: 'x'.repeat(4001) + stack: 'x'.repeat(1001) }); expect(res.status).toBe(204); @@ -108,7 +108,7 @@ describe('POST /api/client-errors', () => { const res = await request(app).post('/api/client-errors').send({ context: 'page', message: 'short', - componentStack: 'x'.repeat(4001) + componentStack: 'x'.repeat(1001) }); expect(res.status).toBe(204); diff --git a/docker-compose.qa.yml b/docker-compose.qa.yml index bdd759d..26376e0 100644 --- a/docker-compose.qa.yml +++ b/docker-compose.qa.yml @@ -84,6 +84,16 @@ services: # happening. `unless-stopped` would silently bring it back after every NAS # reboot and leave it running indefinitely. restart: "no" + # Docker's default json-file driver has no size cap. POST /api/client-errors + # is unauthenticated, so an unrotated log is a disk-filling vector on its + # own — see the error-boundary design doc's backend section. This does not + # cover production, which is a separate Portainer stack outside this repo; + # the same logging options need to be added there directly. + logging: + driver: json-file + options: + max-size: 10m + max-file: "3" redefined-designs-qa-db-syn: image: postgres:16 diff --git a/docs/superpowers/specs/2026-08-20-error-boundary-design.md b/docs/superpowers/specs/2026-08-20-error-boundary-design.md index 0e358b0..1898b94 100644 --- a/docs/superpowers/specs/2026-08-20-error-boundary-design.md +++ b/docs/superpowers/specs/2026-08-20-error-boundary-design.md @@ -70,7 +70,7 @@ props: { children, context: 'page' | 'catalogue' | 'modal', fallback: (error: Er ### `components/ErrorFallback.tsx` -One component, three containers. It renders a title, a set of actions, and — only under `import.meta.env.DEV` — the error message and component stack. +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 | | --- | --- | --- | --- | @@ -80,7 +80,7 @@ One component, three containers. It renders a title, a set of actions, and — o 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 a stack trace lives, rather than three that can drift apart. +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. @@ -111,10 +111,14 @@ A new router mounted in `app.ts`, taking `{ context, message, stack, componentSt | Field | Limit | | --- | --- | | `message` | 500 | -| `stack` | 4000 | -| `componentStack` | 4000 | +| `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. diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 5b7c3b3..76d40ae 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -256,6 +256,13 @@ export default function App() { {loading && !items.length && !failed ? : null} ( React.ReactNode; children: React.ReactNode; + // Lets a boundary recover without a hard navigation, for the one case where + // the fallback's own surrounding controls can make the error go away on + // their own — the catalogue boundary's "Clear filters". Pass a value that + // changes whenever the underlying condition the boundary is guarding + // against has changed; when it does and the boundary is currently showing + // an error, the error clears and children render again. + // + // Do NOT add this to the page or modal boundaries. Their escapes + // (`window.location.reload()` / `window.location.href = '/'`) are hard + // navigations by design — see the design doc — which already remount the + // tree and clear the error. Giving them a resetKey too would just be a + // second, redundant reset mechanism for a boundary that does not need one. + resetKey?: string; }>; interface ErrorBoundaryState { + // Tracked separately from `error` because a thrown value is not guaranteed + // to be truthy — `throw null` and `throw ''` are both legal JavaScript, and + // React does not stop them. Branching on `error` alone would treat a falsy + // caught value as "no error", render the children again, throw again, and + // eventually take the whole root down with it — the exact blank page this + // boundary exists to prevent. + hasError: boolean; error: Error | null; } +// Not every thrown value is an Error, but the rest of this boundary — and the +// fallback it hands the error to — needs a real `.message` (and ideally a +// `.stack`) to report and to render. Synthesize one rather than passing the +// raw value through. +function toError(value: unknown): Error { + if (value instanceof Error) { + return value; + } + return new Error(`Non-Error value thrown: ${String(value)}`); +} + // The only class component in the codebase. getDerivedStateFromError and // componentDidCatch have no hook equivalent, so a boundary cannot be written as // a function component. @@ -19,25 +50,38 @@ interface ErrorBoundaryState { // server. The fallback is the caller's business, which is what lets one // boundary serve a full page, an inline region and a modal. export default class ErrorBoundary extends React.Component { - state: ErrorBoundaryState = { error: null }; + state: ErrorBoundaryState = { hasError: false, error: null }; - static getDerivedStateFromError(error: Error): ErrorBoundaryState { - return { error }; + static getDerivedStateFromError(error: unknown): ErrorBoundaryState { + return { hasError: true, error: toError(error) }; } - componentDidCatch(error: Error, info: React.ErrorInfo): void { + componentDidUpdate(prevProps: ErrorBoundaryProps): void { + // Only ever fires for the catalogue boundary, which is the only caller + // that passes resetKey — see the comment on the prop. + if ( + this.state.hasError && + this.props.resetKey !== undefined && + this.props.resetKey !== prevProps.resetKey + ) { + this.setState({ hasError: false, error: null }); + } + } + + componentDidCatch(error: unknown, info: React.ErrorInfo): void { + const normalized = toError(error); reportClientError({ context: this.props.context, - message: error.message, - stack: error.stack, + message: normalized.message, + stack: normalized.stack, componentStack: info.componentStack ?? undefined, path: `${window.location.pathname}${window.location.search}` }); } render(): React.ReactNode { - if (this.state.error) { - return this.props.fallback(this.state.error); + if (this.state.hasError) { + return this.props.fallback(this.state.error ?? new Error('Unknown error')); } return this.props.children; } diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index b0a078b..c59e085 100755 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -107,7 +107,9 @@ function AppRoutes() { fallback={(error) => ( , and antd would otherwise announce the dialog's accessible + // name and then the identical heading right after it. footer={null} onCancel={() => { window.location.href = '/';