From e8374e391acb8a73b28fbb3116abe9939ff46392 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 16:17:29 -0500 Subject: [PATCH 1/6] docs: design for React error boundaries (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../specs/2026-08-20-error-boundary-design.md | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-20-error-boundary-design.md diff --git a/docs/superpowers/specs/2026-08-20-error-boundary-design.md b/docs/superpowers/specs/2026-08-20-error-boundary-design.md new file mode 100644 index 0000000..0264449 --- /dev/null +++ b/docs/superpowers/specs/2026-08-20-error-boundary-design.md @@ -0,0 +1,140 @@ +# 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:** Approved + +## 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 + + App + [EB "catalogue"] inline fallback + + [EB "modal"] antd Modal fallback + {modalPath === '/account' && } … +``` + +**The root boundary sits inside `BrowserRouter`, not outside it.** The fallback offers a way back to the storefront, which needs router context. Placing it outside would mean the fallback could not link anywhere, and would only additionally catch a throw from the router itself — which is not a realistic failure here. + +**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` — the error message and component stack. + +| Mount point | Container | Actions | +| --- | --- | --- | +| `page` | Full-page antd `Result` | Reload, Back to the shop | +| `catalogue` | Inline, in place of the grid | Reload | +| `modal` | antd `Modal` | Close, returning to the storefront | + +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. + +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=` matching its own scope. Mounted inside each boundary as `{import.meta.env.DEV && }`, so Rollup eliminates both the element and its import from a production build. + +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` | 4000 | +| `componentStack` | 4000 | +| `path` | 200 | + +**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 header, the wordmark and the cart badge remain visible — the specific claim that a bad item no longer takes down navigation. +3. `?boom=modal` on `/account` leaves the storefront rendered behind, and Close returns to it. + +**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. + +## 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. From 683139b8d86fa05460095dda33c1f78628e841a4 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 16:25:48 -0500 Subject: [PATCH 2/6] docs: correct the error-boundary design's fallback actions (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../specs/2026-08-20-error-boundary-design.md | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) 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 0264449..1b325db 100644 --- a/docs/superpowers/specs/2026-08-20-error-boundary-design.md +++ b/docs/superpowers/specs/2026-08-20-error-boundary-design.md @@ -42,7 +42,9 @@ StrictMode {modalPath === '/account' && } … ``` -**The root boundary sits inside `BrowserRouter`, not outside it.** The fallback offers a way back to the storefront, which needs router context. Placing it outside would mean the fallback could not link anywhere, and would only additionally catch a throw from the router itself — which is not a realistic failure here. +**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 ``.** This is worth stating because the obvious implementation is wrong: a React error boundary does not reset when the route changes. A fallback offering `` 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. @@ -70,11 +72,13 @@ props: { children, context: 'page' | 'catalogue' | 'modal', fallback: (error: Er 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. -| Mount point | Container | Actions | -| --- | --- | --- | -| `page` | Full-page antd `Result` | Reload, Back to the shop | -| `catalogue` | Inline, in place of the grid | Reload | -| `modal` | antd `Modal` | Close, returning to the storefront | +| 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 a stack trace lives, rather than three that can drift apart. @@ -92,6 +96,8 @@ Posts to `/api/client-errors` and ignores the outcome — `.catch(() => undefine Throws when the current URL carries `?boom=` matching its own scope. Mounted inside each boundary as `{import.meta.env.DEV && }`, 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` @@ -121,9 +127,12 @@ A new router mounted in `app.ts`, taking `{ context, message, stack, componentSt **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 header, the wordmark and the cart badge remain visible — the specific claim that a bad item no longer takes down navigation. -3. `?boom=modal` on `/account` leaves the storefront rendered behind, and Close returns to it. +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. @@ -135,6 +144,10 @@ A new router mounted in `app.ts`, taking `{ context, message, stack, componentSt **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. + ## 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. From 94e2267eaf557450def7055fa32b87a6c4d31d86 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 16:28:38 -0500 Subject: [PATCH 3/6] 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 --- .../plans/2026-08-20-error-boundary.md | 787 ++++++++++++++++++ 1 file changed, 787 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-20-error-boundary.md diff --git a/docs/superpowers/plans/2026-08-20-error-boundary.md b/docs/superpowers/plans/2026-08-20-error-boundary.md new file mode 100644 index 0000000..400ba69 --- /dev/null +++ b/docs/superpowers/plans/2026-08-20-error-boundary.md @@ -0,0 +1,787 @@ +# React Error Boundaries Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop a single render error from unmounting the whole storefront into a blank white page, and make sure that when it happens it leaves a trace in the server log. + +**Architecture:** One `ErrorBoundary` class component mounted at three points — the root, the item grid, and the modal block — each rendering the same parameterized `ErrorFallback` in a different container. `componentDidCatch` fires a fire-and-forget POST to a new `/api/client-errors` endpoint that logs and returns 204. A development-only `DevThrow` component, gated behind `import.meta.env.DEV` so it cannot reach production, lets the end-to-end suite fire each boundary deliberately. + +**Tech Stack:** React 18.3 + TypeScript, antd 5.20, react-router-dom 6.26, Vite 5, Express + `express-rate-limit`, Jest + supertest (backend), Playwright (end-to-end). + +**Spec:** `docs/superpowers/specs/2026-08-20-error-boundary-design.md` + +## Global Constraints + +- **Branch:** `feature/62-error-boundary`. Every commit subject ends `(#62)`. Do not push — the repository owner pushes. +- **Node:** the dev machine defaults to Node 18, which cannot run this project's tooling. Prefix every command with `export PATH="/c/Users/tlamb/AppData/Local/nvm/v24.13.1:$PATH"`. +- **Integration tests need Postgres.** Port 55432 is Hyper-V-reserved on this machine. Start the container on another port and pass `TEST_PGPORT` rather than editing `docker-compose.test.yml`. +- **antd imports in new files:** deep imports from `antd/es/*`. Never `antd/lib/*` — #65 records that it loads a second React context and breaks `ConfigProvider`. +- **Contexts are exactly** `'page' | 'catalogue' | 'modal'`, spelled identically in the frontend type and the server's allowlist. +- **Truncation limits:** message 500, stack 4000, componentStack 4000, path 200. +- **Rate limit:** 30 requests per 15 minutes, keyed on the caller only. +- **Fallback titles**, used verbatim as test locators: page `Something went wrong`, catalogue `The item list didn't load`, modal `Couldn't open that`. +- **Every escape action is a hard navigation** (`window.location.reload()` / `window.location.href = '/'`). A boundary does not reset on client-side navigation, so a `` would leave the fallback on screen. +- **Arrays of JSX elements need `key` props.** #81 fixed exactly this bug; the `Result` `extra` array is an array of elements. +- **Lint must stay at 0 errors.** `sonarjs/prefer-read-only-props` is on as a warning — declare component props as `Readonly<{…}>` from the start. + +--- + +### Task 1: Backend endpoint — `POST /api/client-errors` + +**Files:** +- Modify: `backend/src/rateLimit.ts` (append a second limiter) +- Create: `backend/src/routes/clientErrors.ts` +- Modify: `backend/src/app.ts` (import near the other route imports, mount beside the other `app.use('/api/...')` lines) +- Test: `backend/tests/integration/clientErrors.integration.test.ts` + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `POST /api/client-errors` accepting `{ context: 'page'|'catalogue'|'modal', message?: string, stack?: string, componentStack?: string, path?: string }` → `204` on success, `400` on an unrecognised `context`. Task 2's `reportClientError` posts this exact shape. + +- [ ] **Step 1: Start a test database and write the failing test** + +Start Postgres first (once per session): + +```bash +MSYS_NO_PATHCONV=1 docker run -d --name rd-eb-db \ + -e POSTGRES_USER=redefined_test -e POSTGRES_PASSWORD=redefined_test -e POSTGRES_DB=redefined_test \ + -p 55435:5432 --tmpfs /var/lib/postgresql/data postgres:16 +``` + +Create `backend/tests/integration/clientErrors.integration.test.ts`: + +```typescript +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { closeDb } from './setup/testDb'; + +// No resetDb: this endpoint never touches the database. The pool still has to +// be closed or Jest reports an open handle. +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +describe('POST /api/client-errors', () => { + let errorSpy: jest.SpyInstance; + + beforeEach(() => { + errorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => { + errorSpy.mockRestore(); + }); + + it('accepts a well-formed report and logs it', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'catalogue', + message: 'Cannot read properties of undefined', + stack: 'Error: Cannot read properties of undefined\n at ItemCard', + componentStack: '\n at Catalogue\n at App', + path: '/?category=3' + }); + + expect(res.status).toBe(204); + expect(errorSpy).toHaveBeenCalledTimes(1); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain('[client-error]'); + expect(logged).toContain('context=catalogue'); + expect(logged).toContain('Cannot read properties of undefined'); + }); + + it('refuses a context it does not recognise rather than logging under a guess', async () => { + const res = await request(app) + .post('/api/client-errors') + .send({ context: 'checkout', message: 'boom' }); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid context'); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('refuses a report with no context at all', async () => { + const res = await request(app).post('/api/client-errors').send({ message: 'boom' }); + + expect(res.status).toBe(400); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + // Truncated rather than refused: an over-long report is still the only record + // of the failure, and dropping it would lose the thing worth having. + it('truncates an oversized message instead of refusing it', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'x'.repeat(5000) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain('[truncated]'); + expect(logged.length).toBeLessThan(2000); + }); +}); +``` + +- [ ] **Step 2: Run the test and confirm it fails for the right reason** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v24.13.1:$PATH" +cd backend && TEST_PGPORT=55435 npx jest -c jest.integration.config.js --runInBand clientErrors +``` + +Expected: all four fail with `404` responses, because nothing is mounted at that path yet. A failure for any other reason means the harness is wrong — fix that before writing the route. + +- [ ] **Step 3: Add the limiter** + +Append to `backend/src/rateLimit.ts`: + +```typescript +// Client error reports carry no email, so this one is keyed on the caller +// alone — deliberately not reusing passwordResetRequestLimiter, whose comment +// above explains why its key is wrong for an endpoint without an email. +// +// `trust proxy` is set in app.ts, so `req.ip` is the real client address from +// X-Forwarded-For rather than Nginx Proxy Manager's, making this a per-customer +// allowance rather than one shared by everybody behind the proxy. +// +// Generous, because hitting the limit is harmless: the reporter ignores the +// response either way. It exists so a render loop cannot fill the log. +const CLIENT_ERROR_WINDOW_MS = 15 * 60 * 1000; +const CLIENT_ERROR_MAX_REQUESTS = 30; + +export const clientErrorLimiter = rateLimit({ + windowMs: CLIENT_ERROR_WINDOW_MS, + limit: CLIENT_ERROR_MAX_REQUESTS, + standardHeaders: 'draft-7', + legacyHeaders: false, + message: { error: 'too many reports' } +}); +``` + +Note it uses the default key generator rather than a custom one. That is deliberate: the custom `keyByCallerAndEmail` is what triggers the `ERR_ERL_KEY_GEN_IPV6` warning already visible in the backend log, and the default handles IPv6 correctly. + +- [ ] **Step 4: Write the route** + +Create `backend/src/routes/clientErrors.ts`: + +```typescript +import { Router, Request, Response } from 'express'; +import { clientErrorLimiter } from '../rateLimit'; + +const router = Router(); + +// The three error boundaries in the frontend. An unrecognised context means the +// client and the server disagree about something, which is worth surfacing +// rather than logging under a guessed label — the same reasoning as +// parseItemFilters refusing a malformed filter instead of coercing it. +const CONTEXTS: readonly string[] = ['page', 'catalogue', 'modal']; + +const MAX_MESSAGE = 500; +const MAX_STACK = 4000; +const MAX_COMPONENT_STACK = 4000; +const MAX_PATH = 200; + +// Anything that is not a string becomes empty rather than 'undefined' or +// '[object Object]', so a malformed field cannot dress itself up as content. +function clip(value: unknown, max: number): string { + if (typeof value !== 'string') { + return ''; + } + return value.length > max ? `${value.slice(0, max)}… [truncated]` : value; +} + +// No asyncRoute: this handler is synchronous, so there is no promise for the +// error middleware to miss. +router.post('/', clientErrorLimiter, (req: Request, res: Response) => { + const context: unknown = req.body?.context; + if (typeof context !== 'string' || !CONTEXTS.includes(context)) { + return res.status(400).json({ error: 'invalid context' }); + } + + console.error( + `[client-error] context=${context} path=${clip(req.body?.path, MAX_PATH)}\n` + + ` message: ${clip(req.body?.message, MAX_MESSAGE)}\n` + + ` stack: ${clip(req.body?.stack, MAX_STACK)}\n` + + ` componentStack: ${clip(req.body?.componentStack, MAX_COMPONENT_STACK)}` + ); + + res.status(204).end(); +}); + +export default router; +``` + +- [ ] **Step 5: Mount it** + +In `backend/src/app.ts`, add the import beside the other route imports: + +```typescript +import clientErrorsRouter from './routes/clientErrors'; +``` + +and the mount beside the other `app.use('/api/...')` lines: + +```typescript +app.use('/api/client-errors', clientErrorsRouter); +``` + +- [ ] **Step 6: Run the tests and confirm they pass** + +```bash +cd backend && TEST_PGPORT=55435 npx jest -c jest.integration.config.js --runInBand clientErrors +``` + +Expected: 4 passed. + +- [ ] **Step 7: Run the whole backend suite plus build and lint** + +```bash +cd backend && npm run build && npm run lint && npm run test:unit +cd backend && TEST_PGPORT=55435 npm run test:integration +``` + +Expected: build clean, lint 0 errors, 78 unit pass, and **138 integration pass** — 134 existing plus the 4 new. Confirm the total actually rose by 4 rather than trusting the number here. `routesAreWrapped.test.ts` must still pass; it scans for unwrapped async handlers, and the new handler is synchronous. + +- [ ] **Step 8: Commit** + +```bash +git add backend/src/rateLimit.ts backend/src/routes/clientErrors.ts backend/src/app.ts backend/tests/integration/clientErrors.integration.test.ts +git commit -m "feat(backend): log client-side render errors to the server (#62)" +``` + +--- + +### Task 2: The boundary, the fallback, and the reporter + +**Files:** +- Create: `frontend/src/errorReporting.ts` +- Create: `frontend/src/components/ErrorBoundary.tsx` +- Create: `frontend/src/components/ErrorFallback.tsx` + +**Interfaces:** +- Consumes: `POST /api/client-errors` from Task 1. +- Produces: + - `type ErrorContext = 'page' | 'catalogue' | 'modal'` + - `reportClientError(report: ClientErrorReport): void` + - ` React.ReactNode}>` — default export of `components/ErrorBoundary` + - `` — default export of `components/ErrorFallback` + + Task 3 mounts all three of these. + +Nothing here is independently testable — the frontend has no unit suite, which is #72's business. The gate for this task is that both files compile and lint clean; the behaviour is proved in Task 3. + +- [ ] **Step 1: Write the reporter** + +Create `frontend/src/errorReporting.ts`: + +```typescript +// Where a caught render error came from. Kept in step with the allowlist in +// backend/src/routes/clientErrors.ts, which refuses anything else rather than +// logging under a guess — change one and you must change the other. +export type ErrorContext = 'page' | 'catalogue' | 'modal'; + +export interface ClientErrorReport { + context: ErrorContext; + message: string; + stack?: string; + componentStack?: string; + path: string; +} + +// Fire and forget, and deliberately swallowing — the one place in this change +// where swallowing is correct. This runs inside componentDidCatch, so a +// reporter that rejected would throw from the very thing that exists to stop +// throws, and there would be nothing left to catch it. +export function reportClientError(report: ClientErrorReport): void { + void fetch('/api/client-errors', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(report) + }).catch(() => undefined); +} +``` + +- [ ] **Step 2: Write the boundary** + +Create `frontend/src/components/ErrorBoundary.tsx`: + +```tsx +import React from 'react'; +import { ErrorContext, reportClientError } from '../errorReporting'; + +type ErrorBoundaryProps = Readonly<{ + context: ErrorContext; + fallback: (error: Error) => React.ReactNode; + children: React.ReactNode; +}>; + +interface ErrorBoundaryState { + error: Error | null; +} + +// The only class component in the codebase. getDerivedStateFromError and +// componentDidCatch have no hook equivalent, so a boundary cannot be written as +// a function component. +// +// It knows nothing about antd and nothing about how reporting reaches the +// 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 }; + + static getDerivedStateFromError(error: Error): ErrorBoundaryState { + return { error }; + } + + componentDidCatch(error: Error, info: React.ErrorInfo): void { + reportClientError({ + context: this.props.context, + message: error.message, + stack: error.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); + } + return this.props.children; + } +} +``` + +- [ ] **Step 3: Write the fallback** + +Create `frontend/src/components/ErrorFallback.tsx`: + +```tsx +import React from 'react'; +import Result from 'antd/es/result'; +import Typography from 'antd/es/typography'; + +const { Paragraph, Text } = Typography; + +type ErrorFallbackProps = Readonly<{ + error: Error; + title: string; + actions: React.ReactNode; + fullPage?: boolean; +}>; + +// The single place that decides whether a customer is shown a stack trace. +// Gated on 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. +export default function ErrorFallback({ error, title, actions, fullPage = false }: ErrorFallbackProps) { + return ( + + {import.meta.env.DEV ? ( + + {error.message} + + ) : null} + + ); +} +``` + +- [ ] **Step 4: Build and lint** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v24.13.1:$PATH" +cd frontend && npm run build && npm run lint +``` + +Expected: build clean. Lint 0 errors and **31 warnings** — the count on `main`. If it rose, the new files introduced a warning; fix it rather than accepting it. The likeliest is `sonarjs/prefer-read-only-props`, which the `Readonly<{…}>` props above are written to avoid. + +- [ ] **Step 5: Commit** + +```bash +git add frontend/src/errorReporting.ts frontend/src/components/ErrorBoundary.tsx frontend/src/components/ErrorFallback.tsx +git commit -m "feat(frontend): add an error boundary, its fallback, and error reporting (#62)" +``` + +--- + +### Task 3: Mount the three boundaries and prove each one catches + +**Files:** +- Create: `frontend/src/components/DevThrow.tsx` +- Modify: `frontend/src/main.tsx` (the `Root` function's `BrowserRouter` block, and the end of `AppRoutes`' returned fragment) +- Modify: `frontend/src/App.tsx` (around the `` element) +- Test: `frontend/tests/e2e/error-boundary.spec.ts` + +**Interfaces:** +- Consumes: `ErrorBoundary`, `ErrorFallback`, `ErrorContext` from Task 2; `POST /api/client-errors` from Task 1. +- Produces: nothing later tasks build on. + +- [ ] **Step 1: Write the failing end-to-end spec** + +Create `frontend/tests/e2e/error-boundary.spec.ts`: + +```typescript +import { test, expect } from './fixtures'; + +// The ?boom= trigger only exists on the dev server, which is what Playwright +// runs against. A production build drops it entirely — verified separately by +// grepping dist for the marker. +test.describe('Error boundaries', () => { + test('a throw below the root shows a message rather than a blank page', async ({ page }) => { + await page.goto('/?boom=page'); + + await expect(page.getByRole('heading', { name: 'Something went wrong' })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Reload' })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Back to the shop' })).toBeVisible(); + }); + + test('a throw in the item grid leaves the header and theme switch usable', async ({ page }) => { + await page.goto('/?boom=catalogue'); + + await expect(page.getByRole('heading', { name: "The item list didn't load" })).toBeVisible(); + + // The claim this boundary exists to make: a bad item no longer takes + // navigation down with it. + await expect(page.getByRole('heading', { name: 'Redefined Designs' })).toBeVisible(); + await expect(page.getByRole('switch')).toBeVisible(); + + // And the root boundary did not also fire — only the nearest one should. + await expect(page.getByRole('heading', { name: 'Something went wrong' })).toHaveCount(0); + }); + + test('a throw in the modal block leaves the storefront behind it intact', async ({ page }) => { + await page.goto('/?boom=modal'); + + await expect(page.getByRole('heading', { name: "Couldn't open that" })).toBeVisible(); + await expect(page.getByRole('heading', { name: 'Redefined Designs' })).toBeVisible(); + }); + + test('a caught error is reported to the server', async ({ page }) => { + const reports: string[] = []; + page.on('request', (request) => { + if (request.url().includes('/api/client-errors')) { + reports.push(request.postData() ?? ''); + } + }); + + await page.goto('/?boom=catalogue'); + await expect(page.getByRole('heading', { name: "The item list didn't load" })).toBeVisible(); + + // Observed on the wire rather than trusting that the reporter was called. + // + // Greater-than-zero, not exactly one: StrictMode double-invokes render in + // development, so the dev server this runs against may report twice where + // production reports once. Asserting an exact count would make the test + // fail for a reason that has nothing to do with the boundary. + await expect.poll(() => reports.length).toBeGreaterThan(0); + expect(reports[0]).toContain('"context":"catalogue"'); + }); +}); +``` + +- [ ] **Step 2: Run it and confirm it fails** + +The backend and a test database must be running. If they are not: + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v24.13.1:$PATH" +cd backend && PGHOST=localhost PGPORT=55435 PGUSER=redefined_test PGPASSWORD=redefined_test \ + PGDATABASE=redefined_test PORT=3000 DEMO_MODE=true UPLOADS_DIR=/tmp/redefined-uploads \ + node migrate.js up && node dist/server.js & +``` + +Then: + +```bash +cd frontend && TEST_PGPORT=55435 npx playwright test tests/e2e/error-boundary.spec.ts +``` + +Expected: 4 failed. The `?boom=` parameter does nothing yet, so the storefront renders normally and no fallback heading is found. + +- [ ] **Step 3: Write the development-only trigger** + +Create `frontend/src/components/DevThrow.tsx`: + +```tsx +import { ErrorContext } from '../errorReporting'; + +// A deliberate throw, reachable only from the dev server. Every mount site +// guards it with `import.meta.env.DEV &&`, which Vite replaces with `false` in +// a production build so Rollup drops both the element and this module. +// +// The marker is in the thrown message so a production bundle can be grepped for +// it. A gate that is silently always-off looks identical to one that works, so +// this is checked rather than trusted — the same reasoning as #61's coverage +// instrumentation gate. +export const DEV_THROW_MARKER = '__DEV_THROW_BOUNDARY__'; + +export default function DevThrow({ scope }: Readonly<{ scope: ErrorContext }>) { + const requested = new URLSearchParams(window.location.search).get('boom'); + if (requested === scope) { + throw new Error(`${DEV_THROW_MARKER} deliberate throw in ${scope}`); + } + return null; +} +``` + +- [ ] **Step 4: Mount the root and modal boundaries in `main.tsx`** + +Add to the imports at the top of `frontend/src/main.tsx`: + +```typescript +import Button from 'antd/es/button'; +import ModalDialog from 'antd/es/modal'; +import ErrorBoundary from './components/ErrorBoundary'; +import ErrorFallback from './components/ErrorFallback'; +import DevThrow from './components/DevThrow'; +``` + +`ModalDialog` rather than `Modal`, because `main.tsx` is one of the nine files still importing from the `antd` barrel and a bare `Modal` risks colliding with a future barrel import there. Renaming at the import keeps this change from depending on #65 landing first. + +In `Root`, replace: + +```tsx + + + +``` + +with: + +```tsx + + ( + window.location.reload()}> + Reload + , + + ]} + /> + )} + > + + + +``` + +Then in `AppRoutes`, wrap the modal block. Replace the section that currently begins with the `{/* Rendered outside the Routes above… */}` comment and ends with the closing of the `/reset-password` branch, so that the whole run of `{modalPath === … && …}` expressions sits inside a boundary: + +```tsx + {/* Rendered outside the Routes above, which are showing the backdrop. */} + ( + { + window.location.href = '/'; + }} + > + { + window.location.href = '/'; + }} + > + Close + + } + /> + + )} + > + {/* Unconditional, so /?boom=modal fires this boundary with the + storefront rendered behind it — no session needed. */} + {import.meta.env.DEV && } + {modalPath === '/account' && } + {modalPath === '/login' && ( + + )} + {modalPath === '/register' && ( + + )} + {modalPath === '/forgot-password' && ( + goWithinAuth('/login')} /> + )} + {modalPath === '/reset-password' && ( + goWithinAuth('/forgot-password')} + onBackToSignIn={() => goWithinAuth('/login')} + /> + )} + +``` + +The `ModalDialog` carries the title and the `ErrorFallback` repeats it. That is intentional: the antd `Modal` header is what a customer reads, and the `Result` title is what the test locates and what keeps the three fallbacks consistent. + +Finally, add the page-scope trigger just inside the `<>` that `AppRoutes` returns, immediately before ``: + +```tsx + {import.meta.env.DEV && } +``` + +- [ ] **Step 5: Mount the catalogue boundary in `App.tsx`** + +Add to the imports at the top of `frontend/src/App.tsx`: + +```typescript +import ErrorBoundary from './components/ErrorBoundary'; +import ErrorFallback from './components/ErrorFallback'; +import DevThrow from './components/DevThrow'; +``` + +Replace the `` element with: + +```tsx + ( + window.location.reload()}> + Reload + + } + /> + )} + > + {import.meta.env.DEV && } + + +``` + +`Button` is already imported in `App.tsx` from the `antd` barrel, so no new import is needed for it. + +- [ ] **Step 6: Run the end-to-end spec and confirm it passes** + +```bash +cd frontend && TEST_PGPORT=55435 npx playwright test tests/e2e/error-boundary.spec.ts +``` + +Expected: 4 passed. + +If the tests fail with clicks timing out or the page obscured, check whether Vite's error overlay is rendering over the app — the spec flags this as a known risk. The fix is to disable the overlay for the dev server (`server: { hmr: { overlay: false } }` in `vite.config.ts`), not to work around it in the tests. + +- [ ] **Step 7: Run the full frontend suite** + +```bash +cd frontend && npm run build && npm run lint && TEST_PGPORT=55435 npm run test:e2e +``` + +Expected: build clean, lint 0 errors and 31 warnings, **87 e2e passing** (83 existing plus 4 new). If the database has already served an earlier run, recreate it first — the suite is not idempotent against a reused database, which produces failures that look like regressions in whatever you just changed. + +- [ ] **Step 8: Commit** + +```bash +git add frontend/src/components/DevThrow.tsx frontend/src/main.tsx frontend/src/App.tsx frontend/tests/e2e/error-boundary.spec.ts +git commit -m "feat(frontend): mount error boundaries at the root, the item grid and the modals (#62)" +``` + +--- + +### Task 4: Prove the trigger cannot reach production, then close out + +**Files:** +- Modify: `docs/superpowers/specs/2026-08-20-error-boundary-design.md` (Status line) + +**Interfaces:** +- Consumes: everything from Tasks 1–3. +- Produces: nothing. + +- [ ] **Step 1: Confirm the development gate is on in development** + +```bash +cd frontend && npm run dev & +curl -s "http://localhost:5173/src/components/DevThrow.tsx" | grep -c "__DEV_THROW_BOUNDARY__" +``` + +Expected: `1` or more. The module is served by the dev server. Stop the dev server afterwards. + +This direction matters as much as the other: a gate that is always off looks identical to a gate that works, and only checking the production side would not tell them apart. + +- [ ] **Step 2: Confirm it is absent from a production build** + +```bash +cd frontend && npm run build +grep -c "__DEV_THROW_BOUNDARY__" dist/assets/*.js +grep -c "boom" dist/assets/*.js +``` + +Expected: `0` for the marker. The `boom` count is informational — a coincidental match in a minified bundle is possible, so the marker is the assertion and `boom` is a sanity check. + +If the marker is present, `import.meta.env.DEV` is not being treated as statically false. Do not ship it; fix the gate. + +- [ ] **Step 3: Run everything, both workspaces** + +```bash +export PATH="/c/Users/tlamb/AppData/Local/nvm/v24.13.1:$PATH" +cd backend && npm run build && npm run lint && npm run test:unit +cd backend && TEST_PGPORT=55435 npm run test:integration +cd frontend && npm run build && npm run lint +cd frontend && TEST_PGPORT=55435 npm run test:e2e +``` + +Record the actual numbers rather than asserting success. Expected: backend build and lint clean, 78 unit, 138 integration; frontend build clean, lint 0 errors / 31 warnings, 87 e2e. + +- [ ] **Step 4: Tear down the test environment** + +```bash +docker rm -f rd-eb-db +``` + +and stop the backend `node dist/server.js` process. + +- [ ] **Step 5: Mark the spec implemented and commit** + +Change the spec's `**Status:** Approved` to `**Status:** Implemented`. If anything in the design turned out to be wrong during implementation, correct the spec in place — it is the durable record, and a spec that documents a decision nobody followed is worse than none. + +```bash +git add docs/superpowers/specs/2026-08-20-error-boundary-design.md +git commit -m "docs: mark the error-boundary design implemented (#62)" +``` + +- [ ] **Step 6: Report on the issue** + +Comment on #62 with what landed, the verification numbers actually observed, and anything the implementation contradicted in the design. Do not push — the repository owner pushes. From c6936720510d60bb08e0f26ceb382b8c59c67ffa Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 17:23:18 -0500 Subject: [PATCH 4/6] feat(backend): log client-side render errors to the server (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/app.ts | 2 + backend/src/rateLimit.ts | 21 ++++++ backend/src/routes/clientErrors.ts | 44 ++++++++++++ .../clientErrors.integration.test.ts | 71 +++++++++++++++++++ 4 files changed, 138 insertions(+) create mode 100644 backend/src/routes/clientErrors.ts create mode 100644 backend/tests/integration/clientErrors.integration.test.ts diff --git a/backend/src/app.ts b/backend/src/app.ts index 9172962..e0fcea5 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -13,6 +13,7 @@ import customersRouter from './routes/customers'; import publicRouter from './routes/public'; import cartRouter from './routes/cart'; import shippingAddressesRouter from './routes/shippingAddresses'; +import clientErrorsRouter from './routes/clientErrors'; import { attachCustomer } from './middleware/customerAuth'; import { asyncRoute } from './asyncRoute'; @@ -52,6 +53,7 @@ app.use('/api/admin/tags', adminTagsRouter); app.use('/api/admin', adminRouter); app.use('/api/customers/me/addresses', shippingAddressesRouter); app.use('/api/customers', customersRouter); +app.use('/api/client-errors', clientErrorsRouter); app.use('/', publicRouter); if (process.env.NODE_ENV !== 'test') { diff --git a/backend/src/rateLimit.ts b/backend/src/rateLimit.ts index 2cfa2ca..3a190e3 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -33,3 +33,24 @@ export const passwordResetRequestLimiter = rateLimit({ legacyHeaders: false, message: { error: 'too many attempts, please try again later' } }); + +// Client error reports carry no email, so this one is keyed on the caller +// alone — deliberately not reusing passwordResetRequestLimiter, whose comment +// above explains why its key is wrong for an endpoint without an email. +// +// `trust proxy` is set in app.ts, so `req.ip` is the real client address from +// X-Forwarded-For rather than Nginx Proxy Manager's, making this a per-customer +// allowance rather than one shared by everybody behind the proxy. +// +// Generous, because hitting the limit is harmless: the reporter ignores the +// response either way. It exists so a render loop cannot fill the log. +const CLIENT_ERROR_WINDOW_MS = 15 * 60 * 1000; +const CLIENT_ERROR_MAX_REQUESTS = 30; + +export const clientErrorLimiter = rateLimit({ + windowMs: CLIENT_ERROR_WINDOW_MS, + limit: CLIENT_ERROR_MAX_REQUESTS, + standardHeaders: 'draft-7', + legacyHeaders: false, + message: { error: 'too many reports' } +}); diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts new file mode 100644 index 0000000..cd07400 --- /dev/null +++ b/backend/src/routes/clientErrors.ts @@ -0,0 +1,44 @@ +import { Router, Request, Response } from 'express'; +import { clientErrorLimiter } from '../rateLimit'; + +const router = Router(); + +// The three error boundaries in the frontend. An unrecognised context means the +// client and the server disagree about something, which is worth surfacing +// rather than logging under a guessed label — the same reasoning as +// parseItemFilters refusing a malformed filter instead of coercing it. +const CONTEXTS: readonly string[] = ['page', 'catalogue', 'modal']; + +const MAX_MESSAGE = 500; +const MAX_STACK = 4000; +const MAX_COMPONENT_STACK = 4000; +const MAX_PATH = 200; + +// Anything that is not a string becomes empty rather than 'undefined' or +// '[object Object]', so a malformed field cannot dress itself up as content. +function clip(value: unknown, max: number): string { + if (typeof value !== 'string') { + return ''; + } + return value.length > max ? `${value.slice(0, max)}… [truncated]` : value; +} + +// No asyncRoute: this handler is synchronous, so there is no promise for the +// error middleware to miss. +router.post('/', clientErrorLimiter, (req: Request, res: Response) => { + const context: unknown = req.body?.context; + if (typeof context !== 'string' || !CONTEXTS.includes(context)) { + return res.status(400).json({ error: 'invalid context' }); + } + + console.error( + `[client-error] context=${context} path=${clip(req.body?.path, MAX_PATH)}\n` + + ` message: ${clip(req.body?.message, MAX_MESSAGE)}\n` + + ` stack: ${clip(req.body?.stack, MAX_STACK)}\n` + + ` componentStack: ${clip(req.body?.componentStack, MAX_COMPONENT_STACK)}` + ); + + res.status(204).end(); +}); + +export default router; diff --git a/backend/tests/integration/clientErrors.integration.test.ts b/backend/tests/integration/clientErrors.integration.test.ts new file mode 100644 index 0000000..18e7509 --- /dev/null +++ b/backend/tests/integration/clientErrors.integration.test.ts @@ -0,0 +1,71 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { closeDb } from './setup/testDb'; + +// No resetDb: this endpoint never touches the database. The pool still has to +// be closed or Jest reports an open handle. +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +describe('POST /api/client-errors', () => { + let errorSpy: jest.SpyInstance; + + beforeEach(() => { + errorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => { + errorSpy.mockRestore(); + }); + + it('accepts a well-formed report and logs it', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'catalogue', + message: 'Cannot read properties of undefined', + stack: 'Error: Cannot read properties of undefined\n at ItemCard', + componentStack: '\n at Catalogue\n at App', + path: '/?category=3' + }); + + expect(res.status).toBe(204); + expect(errorSpy).toHaveBeenCalledTimes(1); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain('[client-error]'); + expect(logged).toContain('context=catalogue'); + expect(logged).toContain('Cannot read properties of undefined'); + }); + + it('refuses a context it does not recognise rather than logging under a guess', async () => { + const res = await request(app) + .post('/api/client-errors') + .send({ context: 'checkout', message: 'boom' }); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid context'); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('refuses a report with no context at all', async () => { + const res = await request(app).post('/api/client-errors').send({ message: 'boom' }); + + expect(res.status).toBe(400); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + // Truncated rather than refused: an over-long report is still the only record + // of the failure, and dropping it would lose the thing worth having. + it('truncates an oversized message instead of refusing it', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'x'.repeat(5000) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain('[truncated]'); + expect(logged.length).toBeLessThan(2000); + }); +}); From 7d227507b01945d96262203cbdc48eb105598004 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 17:32:45 -0500 Subject: [PATCH 5/6] fix(backend): stop a client error report forging log lines (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/routes/clientErrors.ts | 33 +++++++- .../clientErrors.integration.test.ts | 82 +++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts index cd07400..645a7f7 100644 --- a/backend/src/routes/clientErrors.ts +++ b/backend/src/routes/clientErrors.ts @@ -14,13 +14,44 @@ const MAX_STACK = 4000; const MAX_COMPONENT_STACK = 4000; const MAX_PATH = 200; +// True for CR, LF and every other C0 control character, plus DEL (the +// C0 range is code points 0 through 31; DEL is 127). Written as a numeric +// comparison rather than a control-character regex literal so the source +// never has to embed a raw control character or an escape sequence for one. +const LAST_C0_CODE = 31; +const DEL_CODE = 127; +function isControlCharCode(code: number): boolean { + return code <= LAST_C0_CODE || code === DEL_CODE; +} + +// Strips CR, LF and other control characters from a string, replacing each +// with a single space. The endpoint is unauthenticated, so without this a +// caller could embed a newline in any field to forge what looks like a +// second [client-error] line in the shared server log. The replacement is +// 1-for-1 (one control character becomes one space), so it cannot change +// the string's length either way. +function sanitize(value: string): string { + let result = ''; + for (const char of value) { + result += isControlCharCode(char.codePointAt(0) ?? 0) ? ' ' : char; + } + return result; +} + // Anything that is not a string becomes empty rather than 'undefined' or // '[object Object]', so a malformed field cannot dress itself up as content. function clip(value: unknown, max: number): string { if (typeof value !== 'string') { return ''; } - return value.length > max ? `${value.slice(0, max)}… [truncated]` : value; + // Sanitize before truncating, not after. Because the substitution above is + // 1-for-1, sanitizing first cannot push the stored length past `max` — an + // escaping scheme that expanded a control character into multiple visible + // characters would need the opposite order to keep that same guarantee, so + // the two are not interchangeable and must not be reordered without + // re-checking this. + const sanitized = sanitize(value); + return sanitized.length > max ? `${sanitized.slice(0, max)}… [truncated]` : sanitized; } // No asyncRoute: this handler is synchronous, so there is no promise for the diff --git a/backend/tests/integration/clientErrors.integration.test.ts b/backend/tests/integration/clientErrors.integration.test.ts index 18e7509..45bf76e 100644 --- a/backend/tests/integration/clientErrors.integration.test.ts +++ b/backend/tests/integration/clientErrors.integration.test.ts @@ -68,4 +68,86 @@ describe('POST /api/client-errors', () => { expect(logged).toContain('[truncated]'); expect(logged.length).toBeLessThan(2000); }); + + // The boundary itself, not just "way over the limit": a message of exactly + // MAX_MESSAGE (500) must survive untouched. + it('leaves a message of exactly the length limit unmodified', async () => { + const message = 'a'.repeat(500); + const res = await request(app).post('/api/client-errors').send({ context: 'page', message }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain(`message: ${message}\n`); + expect(logged).not.toContain('[truncated]'); + }); + + // One character past the boundary must truncate. + it('truncates a message one character past the length limit', async () => { + const message = 'a'.repeat(501); + const res = await request(app).post('/api/client-errors').send({ context: 'page', message }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain(`message: ${'a'.repeat(500)}… [truncated]`); + }); + + it('truncates an oversized stack independently of message', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'short', + stack: 'x'.repeat(4001) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + const stackLine = logged.split('\n').find((line) => line.trim().startsWith('stack:')); + expect(stackLine).toContain('[truncated]'); + }); + + it('truncates an oversized componentStack independently of message', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'short', + componentStack: 'x'.repeat(4001) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + const componentStackLine = logged + .split('\n') + .find((line) => line.trim().startsWith('componentStack:')); + expect(componentStackLine).toContain('[truncated]'); + }); + + it('truncates an oversized path independently of message', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'short', + path: '/'.concat('x'.repeat(201)) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + const pathLine = logged.split('\n')[0]; + expect(pathLine).toContain('[truncated]'); + }); + + // The endpoint is unauthenticated, so nothing stops a caller from sending a + // message crafted to look like a second [client-error] line. This is what + // Finding 1 closes: an embedded CR/LF must not survive into the log. + it('collapses embedded newlines so a report cannot forge a second log line', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'modal', + message: 'real error\n[client-error] context=page path=/fake\r\n message: forged entry' + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + + // The template itself joins four fixed lines with three newlines; that + // count must not grow no matter what the caller sends. + expect(logged.split('\n')).toHaveLength(4); + expect(logged).not.toContain('\n[client-error]'); + expect(logged).not.toContain('\r'); + }); }); From e0ae2dcbcda2a79c42bfd8e610b9a3bcb90af000 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 17:40:16 -0500 Subject: [PATCH 6/6] feat(frontend): add an error boundary, its fallback, and error reporting (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- frontend/src/components/ErrorBoundary.tsx | 44 +++++++++++++++++++++++ frontend/src/components/ErrorFallback.tsx | 34 ++++++++++++++++++ frontend/src/errorReporting.ts | 24 +++++++++++++ frontend/src/vite-env.d.ts | 1 + 4 files changed, 103 insertions(+) create mode 100644 frontend/src/components/ErrorBoundary.tsx create mode 100644 frontend/src/components/ErrorFallback.tsx create mode 100644 frontend/src/errorReporting.ts create mode 100644 frontend/src/vite-env.d.ts diff --git a/frontend/src/components/ErrorBoundary.tsx b/frontend/src/components/ErrorBoundary.tsx new file mode 100644 index 0000000..b88389e --- /dev/null +++ b/frontend/src/components/ErrorBoundary.tsx @@ -0,0 +1,44 @@ +import React from 'react'; +import { ErrorContext, reportClientError } from '../errorReporting'; + +type ErrorBoundaryProps = Readonly<{ + context: ErrorContext; + fallback: (error: Error) => React.ReactNode; + children: React.ReactNode; +}>; + +interface ErrorBoundaryState { + error: Error | null; +} + +// The only class component in the codebase. getDerivedStateFromError and +// componentDidCatch have no hook equivalent, so a boundary cannot be written as +// a function component. +// +// It knows nothing about antd and nothing about how reporting reaches the +// 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 }; + + static getDerivedStateFromError(error: Error): ErrorBoundaryState { + return { error }; + } + + componentDidCatch(error: Error, info: React.ErrorInfo): void { + reportClientError({ + context: this.props.context, + message: error.message, + stack: error.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); + } + return this.props.children; + } +} diff --git a/frontend/src/components/ErrorFallback.tsx b/frontend/src/components/ErrorFallback.tsx new file mode 100644 index 0000000..729308b --- /dev/null +++ b/frontend/src/components/ErrorFallback.tsx @@ -0,0 +1,34 @@ +import React from 'react'; +import Result from 'antd/es/result'; +import Typography from 'antd/es/typography'; + +const { Paragraph, Text } = Typography; + +type ErrorFallbackProps = Readonly<{ + error: Error; + title: string; + actions: React.ReactNode; + fullPage?: boolean; +}>; + +// The single place that decides whether a customer is shown a stack trace. +// Gated on 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. +export default function ErrorFallback({ error, title, actions, fullPage = false }: ErrorFallbackProps) { + return ( + + {import.meta.env.DEV ? ( + + {error.message} + + ) : null} + + ); +} diff --git a/frontend/src/errorReporting.ts b/frontend/src/errorReporting.ts new file mode 100644 index 0000000..eb9ea66 --- /dev/null +++ b/frontend/src/errorReporting.ts @@ -0,0 +1,24 @@ +// Where a caught render error came from. Kept in step with the allowlist in +// backend/src/routes/clientErrors.ts, which refuses anything else rather than +// logging under a guess — change one and you must change the other. +export type ErrorContext = 'page' | 'catalogue' | 'modal'; + +export interface ClientErrorReport { + context: ErrorContext; + message: string; + stack?: string; + componentStack?: string; + path: string; +} + +// Fire and forget, and deliberately swallowing — the one place in this change +// where swallowing is correct. This runs inside componentDidCatch, so a +// reporter that rejected would throw from the very thing that exists to stop +// throws, and there would be nothing left to catch it. +export function reportClientError(report: ClientErrorReport): void { + void fetch('/api/client-errors', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(report) + }).catch(() => undefined); +} diff --git a/frontend/src/vite-env.d.ts b/frontend/src/vite-env.d.ts new file mode 100644 index 0000000..11f02fe --- /dev/null +++ b/frontend/src/vite-env.d.ts @@ -0,0 +1 @@ +///