Merge branch 'main' into feature/84-ipv6-rate-limit-key

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

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);

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

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 `<div>`, with no heading semantics.** This design specified both a `Result` with a string title and end-to-end assertions using `getByRole('heading')` — two requirements that contradict each other, so the tests could never have passed as written. The fix was to give the title a real heading rather than to loosen the assertion: a page whose entire content is an error message, with nothing carrying heading semantics, offers a screen-reader user navigating by headings nothing at all to find. `ErrorFallback` now wraps the title in `Typography.Title`, and the assertion stands as it was.
+
+**`import.meta.env` had no ambient type declaration.** Nothing in this application had used `import.meta.env` before — the only Vite environment gating lived in `vite.config.ts` — so `tsc` rejected the `DEV` check outright with TS2339. `frontend/src/vite-env.d.ts` was added, which is the standard one-line Vite reference and pulls in no new dependency.
+
+One thing the design flagged as a risk did not materialise: Vite's error overlay never interfered with the Playwright run, so `vite.config.ts` was left alone.
+
 ## Out of scope
 
 Error reporting to an external service, persisting errors, alerting, and a frontend unit-test suite — the last belongs to #72, which already owns it.

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() {
         </div>
 
         {loading && !items.length && !failed ? <Spin /> : null}
-        <Catalogue
-          failed={failed}
-          loading={loading}
-          items={items}
-          filters={filters}
-          needsFavoritesAuth={needsFavoritesAuth}
-          onRetry={handleRetry}
-          onSignIn={openAuthModal}
-          onClearFilters={clearFilters}
-          onChanged={reload}
-        />
+        <ErrorBoundary
+          context="catalogue"
+          // Lets Clear filters (and any other filter change) recover the grid
+          // without a reload: filterKey changes whenever the filters change,
+          // which resets the boundary the next time it renders. `?boom=catalogue`
+          // is not itself a known filter, so filterKey is unaffected by it and
+          // the existing end-to-end assertion that the fallback renders still
+          // holds.
+          resetKey={filterKey}
+          fallback={(error) => (
+            <ErrorFallback
+              error={error}
+              title="The item list didn't load"
+              actions={
+                <Button type="primary" onClick={() => window.location.reload()}>
+                  Reload
+                </Button>
+              }
+            />
+          )}
+        >
+          {import.meta.env.DEV && <DevThrow scope="catalogue" />}
+          <Catalogue
+            failed={failed}
+            loading={loading}
+            items={items}
+            filters={filters}
+            needsFavoritesAuth={needsFavoritesAuth}
+            onRetry={handleRetry}
+            onSignIn={openAuthModal}
+            onClearFilters={clearFilters}
+            onChanged={reload}
+          />
+        </ErrorBoundary>
       </Content>
       <Footer style={{ textAlign: 'center', background: token.colorBgContainer }}>
         <Link to="/privacy">Privacy Policy</Link>

frontend/src/components/DevThrow.tsx

@@ -0,0 +1,19 @@
+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;
+}

frontend/src/components/ErrorBoundary.tsx

@@ -5,12 +5,43 @@ type ErrorBoundaryProps = Readonly<{
   context: ErrorContext;
   fallback: (error: Error) => 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<ErrorBoundaryProps, ErrorBoundaryState> {
-  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;
   }

frontend/src/components/ErrorFallback.tsx

@@ -2,7 +2,7 @@ import React from 'react';
 import Result from 'antd/es/result';
 import Typography from 'antd/es/typography';
 
-const { Paragraph, Text } = Typography;
+const { Title, Paragraph, Text } = Typography;
 
 type ErrorFallbackProps = Readonly<{
   error: Error;
@@ -19,7 +19,16 @@ export default function ErrorFallback({ error, title, actions, fullPage = false
   return (
     <Result
       status="error"
-      title={title}
+      // antd's Result renders `title` as a plain div, with no heading
+      // semantics — a screen-reader user navigating by headings would find
+      // nothing on a page whose entire content is this error. Wrapped in
+      // Title so the fallback has a real heading; do not simplify this back
+      // to a bare string.
+      title={
+        <Title level={3} style={{ marginBottom: 0 }}>
+          {title}
+        </Title>
+      }
       subTitle="This has been reported. Nothing you did caused it."
       style={{ paddingBlock: fullPage ? 64 : 24 }}
       extra={actions}

frontend/src/errorReporting.ts

@@ -13,12 +13,24 @@ export interface ClientErrorReport {
 
 // 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.
+// reporter that threw — or rejected — would throw from the very thing that
+// exists to stop throws, and there would be nothing left to catch it.
+//
+// The try/catch guards the synchronous part: JSON.stringify(report) and the
+// call to fetch() itself both run before any promise exists, so React does
+// not guarantee componentDidCatch is handed a real Error — code can throw
+// anything, and an object with a circular message or stack makes
+// JSON.stringify throw. The .catch on the returned promise guards the async
+// part, the rejection once fetch has actually started. Neither covers the
+// other; both are required.
 export function reportClientError(report: ClientErrorReport): void {
-  void fetch('/api/client-errors', {
-    method: 'POST',
-    headers: { 'Content-Type': 'application/json' },
-    body: JSON.stringify(report)
-  }).catch(() => undefined);
+  try {
+    void fetch('/api/client-errors', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify(report)
+    }).catch(() => undefined);
+  } catch {
+    // Nothing to do — see the comment above.
+  }
 }

frontend/src/main.tsx

@@ -4,7 +4,12 @@ import { BrowserRouter, Routes, Route, useLocation, useNavigate } from 'react-ro
 import type { Location } from 'react-router-dom';
 import { ConfigProvider, theme as antdTheme } from 'antd';
 import 'antd/dist/reset.css';
+import Button from 'antd/es/button';
+import ModalDialog from 'antd/es/modal';
 import App from './App';
+import ErrorBoundary from './components/ErrorBoundary';
+import ErrorFallback from './components/ErrorFallback';
+import DevThrow from './components/DevThrow';
 import Admin from './admin/Admin';
 import AuthRouteModal from './customer/AuthRouteModal';
 import Account from './customer/Account';
@@ -88,6 +93,7 @@ function AppRoutes() {
 
   return (
     <>
+      {import.meta.env.DEV && <DevThrow scope="page" />}
       <Routes location={backdrop}>
         <Route path="/" element={<App />} />
         <Route path="/admin" element={<Admin />} />
@@ -96,23 +102,57 @@ function AppRoutes() {
         <Route path="/verify-email" element={<VerifyEmail />} />
       </Routes>
       {/* Rendered outside the Routes above, which are showing the backdrop. */}
-      {modalPath === '/account' && <Account onClose={closeModal} />}
-      {modalPath === '/login' && (
-        <AuthRouteModal mode="login" onClose={closeModal} onNavigate={goWithinAuth} />
-      )}
-      {modalPath === '/register' && (
-        <AuthRouteModal mode="register" onClose={closeModal} onNavigate={goWithinAuth} />
-      )}
-      {modalPath === '/forgot-password' && (
-        <ForgotPassword onClose={closeModal} onBackToSignIn={() => goWithinAuth('/login')} />
-      )}
-      {modalPath === '/reset-password' && (
-        <ResetPassword
-          onClose={closeModal}
-          onRequestNewLink={() => goWithinAuth('/forgot-password')}
-          onBackToSignIn={() => goWithinAuth('/login')}
-        />
-      )}
+      <ErrorBoundary
+        context="modal"
+        fallback={(error) => (
+          <ModalDialog
+            open
+            // No `title` here: ErrorFallback renders the same string as an
+            // <h3>, and antd would otherwise announce the dialog's accessible
+            // name and then the identical heading right after it.
+            footer={null}
+            onCancel={() => {
+              window.location.href = '/';
+            }}
+          >
+            <ErrorFallback
+              error={error}
+              title="Couldn't open that"
+              actions={
+                <Button
+                  type="primary"
+                  onClick={() => {
+                    window.location.href = '/';
+                  }}
+                >
+                  Close
+                </Button>
+              }
+            />
+          </ModalDialog>
+        )}
+      >
+        {/* Unconditional, so /?boom=modal fires this boundary with the
+            storefront rendered behind it — no session needed. */}
+        {import.meta.env.DEV && <DevThrow scope="modal" />}
+        {modalPath === '/account' && <Account onClose={closeModal} />}
+        {modalPath === '/login' && (
+          <AuthRouteModal mode="login" onClose={closeModal} onNavigate={goWithinAuth} />
+        )}
+        {modalPath === '/register' && (
+          <AuthRouteModal mode="register" onClose={closeModal} onNavigate={goWithinAuth} />
+        )}
+        {modalPath === '/forgot-password' && (
+          <ForgotPassword onClose={closeModal} onBackToSignIn={() => goWithinAuth('/login')} />
+        )}
+        {modalPath === '/reset-password' && (
+          <ResetPassword
+            onClose={closeModal}
+            onRequestNewLink={() => goWithinAuth('/forgot-password')}
+            onBackToSignIn={() => goWithinAuth('/login')}
+          />
+        )}
+      </ErrorBoundary>
     </>
   );
 }
@@ -138,7 +178,31 @@ function Root() {
       }}
     >
       <BrowserRouter>
-        <AppRoutes />
+        <ErrorBoundary
+          context="page"
+          fallback={(error) => (
+            <ErrorFallback
+              error={error}
+              title="Something went wrong"
+              fullPage
+              actions={[
+                <Button key="reload" type="primary" onClick={() => window.location.reload()}>
+                  Reload
+                </Button>,
+                <Button
+                  key="home"
+                  onClick={() => {
+                    window.location.href = '/';
+                  }}
+                >
+                  Back to the shop
+                </Button>
+              ]}
+            />
+          )}
+        >
+          <AppRoutes />
+        </ErrorBoundary>
       </BrowserRouter>
     </ConfigProvider>
   );

frontend/tests/e2e/error-boundary.spec.ts

@@ -0,0 +1,56 @@
+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"');
+  });
+});