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 1b325db..1898b94 100644 --- a/docs/superpowers/specs/2026-08-20-error-boundary-design.md +++ b/docs/superpowers/specs/2026-08-20-error-boundary-design.md @@ -2,7 +2,7 @@ **Issue:** [#62 — No React error boundary](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/62) **Date:** 2026-08-20 -**Status:** Approved +**Status:** Implemented ## Goal @@ -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. @@ -148,6 +152,16 @@ Assertions are on the three distinct fallback titles, so a test cannot pass beca The new frontend files use deep imports from `antd/es/*`, which is this project's documented convention and the style every recently-added file follows. Not `antd/lib/*`: #65 records that as the mistake which loads a second React context and breaks `ConfigProvider`. The nine files still using the `antd` barrel are #65's business, not this change's. +## Corrections found during implementation + +Two things this design got wrong, recorded here rather than left for the next reader to rediscover. + +**antd's `Result` renders its title as a plain `
`, with no heading semantics.** This design specified both a `Result` with a string title and end-to-end assertions using `getByRole('heading')` — two requirements that contradict each other, so the tests could never have passed as written. The fix was to give the title a real heading rather than to loosen the assertion: a page whose entire content is an error message, with nothing carrying heading semantics, offers a screen-reader user navigating by headings nothing at all to find. `ErrorFallback` now wraps the title in `Typography.Title`, and the assertion stands as it was. + +**`import.meta.env` had no ambient type declaration.** Nothing in this application had used `import.meta.env` before — the only Vite environment gating lived in `vite.config.ts` — so `tsc` rejected the `DEV` check outright with TS2339. `frontend/src/vite-env.d.ts` was added, which is the standard one-line Vite reference and pulls in no new dependency. + +One thing the design flagged as a risk did not materialise: Vite's error overlay never interfered with the Playwright run, so `vite.config.ts` was left alone. + ## Out of scope Error reporting to an external service, persisting errors, alerting, and a frontend unit-test suite — the last belongs to #72, which already owns it. diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 8d210d4..76d40ae 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -18,6 +18,9 @@ import AuthPromptModal from './customer/AuthPromptModal'; import { useThemeMode } from './theme/ThemeContext'; import { useCustomerAuth } from './customer/CustomerAuthContext'; import { useCart } from './cart/CartContext'; +import ErrorBoundary from './components/ErrorBoundary'; +import ErrorFallback from './components/ErrorFallback'; +import DevThrow from './components/DevThrow'; const { Header, Content, Footer } = Layout; const { Title } = Typography; @@ -251,17 +254,40 @@ export default function App() {
{loading && !items.length && !failed ? : null} - + ( + window.location.reload()}> + Reload + + } + /> + )} + > + {import.meta.env.DEV && } + +