Files
redefined-designs/backend/tests/integration/clientErrors.integration.test.ts
T
bermudalambandClaude Opus 5 71cbd142c3 fix: address the final review of the error boundaries (#62)
Five findings from a whole-branch review, two of which mattered.

The catalogue boundary could not be recovered by the very controls it exists to keep alive. When the item grid threw, the header, filter chips and Clear filters stayed mounted — that was the point — but clicking Clear filters only changed the URL. A boundary does not reset on a client-side state change, so the fallback went on rendering over a catalogue that would by then have loaded perfectly well, and the only offered action reloaded the same failing URL. The shop read as permanently broken, which is the failure this whole change exists to prevent, reintroduced one level down. ErrorBoundary now takes an optional resetKey and clears itself when it changes; the catalogue boundary passes the filter key. The page and modal boundaries deliberately do not take one, because their escapes are hard navigations that remount the tree already — recorded on the prop so nobody completes the pattern by symmetry.

The client-error endpoint could fill the disk. It is unauthenticated, each accepted report wrote about 8.7 KB, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as its comment claims, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, which is still around fifteen frames and cuts the worst case to under 3 KB, and the QA compose file caps and rotates the log. Production is a Portainer stack outside this repository and needs the same option applied there; noted in the design doc rather than left implied.

Three smaller things. A falsy thrown value defeated the boundary entirely: throw null is legal, and branching on the error object alone treated it as no error, re-rendered the children, threw again, and would have taken the root down — a blank page, the one outcome this is all here to avoid. The boundary now tracks hasError separately and synthesises a real Error for non-Error throws. The modal fallback announced its title twice to a screen reader, once as the dialog's name and once as the heading inside it, so the redundant dialog title is gone. And the design doc claimed the development-only detail shows the component stack when it only ever showed the message; corrected, with a note that the stack still reaches the server log, which is where it is useful.

Verified after all five: backend lint 0 errors, 144 integration tests, frontend lint 0 errors and 31 warnings, 87 end-to-end tests, all against a freshly created database.

Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-20 18:32:27 -05:00

154 lines
5.6 KiB
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);
});
// 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(1001)
});
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(1001)
});
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');
});
});