feat(observability): add handleError hook with errorId and redesigned error page #608

Merged

marcel merged 11 commits from feat/issue-462-handle-error-hook into main 2026-05-17 12:38:10 +02:00

Conversation 15 Commits 11 Files Changed 15 +369 -99

marcel commented 2026-05-17 09:47:33 +02:00

Owner

Copy Link

Closes #462

What this does

• hooks.server.ts + hooks.client.ts: passes a callback to handleErrorWithSentry that returns { message: 'An unexpected error occurred', errorId } — errorId is Sentry.lastEventId() when a Sentry event was captured, or a fresh crypto.randomUUID() as fallback. Adds sendDefaultPii: false to both Sentry.init() calls.
• app.d.ts: adds App.Error { message: string; errorId?: string } — the TypeScript contract enabling page.error?.errorId throughout the UI.
• +error.svelte: redesigned — <main> landmark, <h1> human-readable heading, status demoted to text-xs text-ink-3, errorId displayed in a <code> block with select-all, copy button with 2-second "Kopiert!" feedback, aria-live="polite" on the confirmation text.
• i18n: adds error_page_id_label, error_copy_id_label, error_copied to de/en/es.json.
• api/tags/+server.ts: removes console.log('Tags Data', data) (audit F-21).
• eslint.config.js: adds no-console: ['error', { allow: ['warn', 'error'] }] with an override for e2e/**.
• docs/architecture/c4/l1-context.puml: adds GlitchTip as external error-tracking system (required per doc update table).

Commits

1. feat(observability): add App.Error interface with errorId to app.d.ts
2. feat(observability): add handleError callback to hooks.server.ts returning errorId
3. feat(observability): add handleError callback to hooks.client.ts returning errorId
4. feat(observability): redesign +error.svelte with errorId display and copy button
5. refactor(observability): remove console.log from tags proxy and enforce no-console lint rule
6. docs(c4): add GlitchTip as external error-tracking system to L1 context diagram

Test coverage

• src/hooks.server.test.ts (new): 2 unit tests — returns Sentry event ID; falls back to UUID
• src/routes/error.svelte.test.ts (extended): +3 tests — shows errorId, shows copy button, hides errorId section when absent

Acceptance criteria

• handleError exported in both hooks.server.ts and hooks.client.ts
• Error responses always include a UUID errorId
• errorId visible to the user on +error.svelte (copyable)
• No console.log in frontend/src
• ESLint rule no-console enforced
• App.Error interface declared; svelte-check passes on changed files

Closes #462 ## What this does - **`hooks.server.ts` + `hooks.client.ts`**: passes a callback to `handleErrorWithSentry` that returns `{ message: 'An unexpected error occurred', errorId }` — `errorId` is `Sentry.lastEventId()` when a Sentry event was captured, or a fresh `crypto.randomUUID()` as fallback. Adds `sendDefaultPii: false` to both `Sentry.init()` calls. - **`app.d.ts`**: adds `App.Error { message: string; errorId?: string }` — the TypeScript contract enabling `page.error?.errorId` throughout the UI. - **`+error.svelte`**: redesigned — `<main>` landmark, `<h1>` human-readable heading, status demoted to `text-xs text-ink-3`, errorId displayed in a `<code>` block with `select-all`, copy button with 2-second "Kopiert!" feedback, `aria-live="polite"` on the confirmation text. - **i18n**: adds `error_page_id_label`, `error_copy_id_label`, `error_copied` to `de/en/es.json`. - **`api/tags/+server.ts`**: removes `console.log('Tags Data', data)` (audit F-21). - **`eslint.config.js`**: adds `no-console: ['error', { allow: ['warn', 'error'] }]` with an override for `e2e/**`. - **`docs/architecture/c4/l1-context.puml`**: adds GlitchTip as external error-tracking system (required per doc update table). ## Commits 1. `feat(observability): add App.Error interface with errorId to app.d.ts` 2. `feat(observability): add handleError callback to hooks.server.ts returning errorId` 3. `feat(observability): add handleError callback to hooks.client.ts returning errorId` 4. `feat(observability): redesign +error.svelte with errorId display and copy button` 5. `refactor(observability): remove console.log from tags proxy and enforce no-console lint rule` 6. `docs(c4): add GlitchTip as external error-tracking system to L1 context diagram` ## Test coverage - `src/hooks.server.test.ts` (new): 2 unit tests — returns Sentry event ID; falls back to UUID - `src/routes/error.svelte.test.ts` (extended): +3 tests — shows errorId, shows copy button, hides errorId section when absent ## Acceptance criteria - [x] `handleError` exported in both `hooks.server.ts` and `hooks.client.ts` - [x] Error responses always include a UUID `errorId` - [x] `errorId` visible to the user on `+error.svelte` (copyable) - [x] No `console.log` in `frontend/src` - [x] ESLint rule `no-console` enforced - [x] `App.Error` interface declared; `svelte-check` passes on changed files

marcel added 6 commits 2026-05-17 09:47:34 +02:00

feat(observability): add App.Error interface with errorId to app.d.ts ... 97aa372094

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(observability): add handleError callback to hooks.server.ts returning errorId ... a9c82ec481

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(observability): add handleError callback to hooks.client.ts returning errorId ... dff81f7bfb

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(observability): redesign +error.svelte with errorId display and copy button ... 96ea7e6815

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(observability): remove console.log from tags proxy and enforce no-console lint rule ... 59b18039ed

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs(c4): add GlitchTip as external error-tracking system to L1 context diagram ... 2023ea2931

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel referenced this pull request 2026-05-17 09:47:47 +02:00

feat(observability): add handleError hook with structured stdout sink #462

marcel commented 2026-05-17 09:49:20 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

TDD evidence is solid — failing tests exist before the implementation for both the hook callback and the error page. Component size is clean. The hook callbacks are lean and do exactly one thing.

Blockers

None.

Suggestions

+error.svelte — no .catch() on navigator.clipboard.writeText()

navigator.clipboard.writeText() returns a Promise that can reject — for instance when the page isn't focused, or when the user denies the clipboard permission. The current code:

navigator.clipboard.writeText(id).then(() => {
    copied = true;
    setTimeout(() => (copied = false), 2000);
});

A rejection silently does nothing. The user clicks the button, gets no feedback, and assumes the copy worked. Suggest:

navigator.clipboard.writeText(id).then(
    () => {
        copied = true;
        setTimeout(() => (copied = false), 2000);
    },
    () => {
        // clipboard unavailable or denied — fallback: select the code element
    }
);

Even a silent fallback (no-op on rejection) is better than the current behavior because it documents the failure case is known. A select() fallback would let the user see the text is selected and copy manually.

hooks.server.test.ts — vi.resetModules() with dynamic imports is correct but fragile

The beforeEach(() => { vi.resetModules(); }) + await import('./hooks.server') pattern works correctly here, but the mock declarations at the top of the file (vi.mock(...)) are hoisted by Vitest and apply across all tests. This is fine as long as you only adjust return values with vi.mocked(...).mockReturnValue() between tests, which you do — just noting it for anyone who extends these tests later.

vite.config.ts — src/hooks.server.ts still not in the coverage include

Sara flagged this in the pre-implementation review: the server coverage project only covers src/lib/shared/** and src/lib/document/**. The new hooks.server.test.ts runs and passes, but the coverage thresholds don't gate on hooks.server.ts. The file can regress without the coverage gate catching it.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** TDD evidence is solid — failing tests exist before the implementation for both the hook callback and the error page. Component size is clean. The hook callbacks are lean and do exactly one thing. ### Blockers None. ### Suggestions **`+error.svelte` — no `.catch()` on `navigator.clipboard.writeText()`** `navigator.clipboard.writeText()` returns a Promise that can reject — for instance when the page isn't focused, or when the user denies the clipboard permission. The current code: ```typescript navigator.clipboard.writeText(id).then(() => { copied = true; setTimeout(() => (copied = false), 2000); }); ``` A rejection silently does nothing. The user clicks the button, gets no feedback, and assumes the copy worked. Suggest: ```typescript navigator.clipboard.writeText(id).then( () => { copied = true; setTimeout(() => (copied = false), 2000); }, () => { // clipboard unavailable or denied — fallback: select the code element } ); ``` Even a silent fallback (no-op on rejection) is better than the current behavior because it documents the failure case is known. A `select()` fallback would let the user see the text is selected and copy manually. **`hooks.server.test.ts` — `vi.resetModules()` with dynamic imports is correct but fragile** The `beforeEach(() => { vi.resetModules(); })` + `await import('./hooks.server')` pattern works correctly here, but the mock declarations at the top of the file (`vi.mock(...)`) are hoisted by Vitest and apply across all tests. This is fine as long as you only adjust return values with `vi.mocked(...).mockReturnValue()` between tests, which you do — just noting it for anyone who extends these tests later. **`vite.config.ts` — `src/hooks.server.ts` still not in the coverage `include`** Sara flagged this in the pre-implementation review: the server coverage project only covers `src/lib/shared/**` and `src/lib/document/**`. The new `hooks.server.test.ts` runs and passes, but the coverage thresholds don't gate on `hooks.server.ts`. The file can regress without the coverage gate catching it.

marcel commented 2026-05-17 09:49:35 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

The security posture of this PR is good. sendDefaultPii: false is explicitly set on both hooks — that was my primary ask from the pre-implementation review. The callback touches only status and Sentry.lastEventId(), with no request headers, cookies, or user fields spread into the return value. The UUID fallback is correct.

Blockers

None.

Suggestions

hooks.server.ts + hooks.client.ts — missing comment explaining why the DSN is safe in the client bundle

I flagged this in the issue review. Future maintainers will see VITE_SENTRY_DSN embedded in the build and assume it needs rotation like a password. It doesn't — it's a write-only ingest key. A one-line comment prevents a future "security incident" that isn't one:

// VITE_SENTRY_DSN is a write-only ingest key — it can POST events to GlitchTip
// but cannot read them. Safe to include in the client bundle per Sentry security model.
Sentry.init({
    dsn: import.meta.env.VITE_SENTRY_DSN,
    ...
});

+error.svelte — navigator.clipboard is unavailable in non-secure contexts

navigator.clipboard is undefined when the page is served over HTTP (non-HTTPS) or from file://. In production this is never an issue (Caddy enforces HTTPS), but in development over plain HTTP it silently throws a TypeError. The current code doesn't guard against navigator.clipboard being undefined:

// Current — throws TypeError in non-HTTPS context
navigator.clipboard.writeText(id).then(...)

// Safer — explicit guard
if (navigator.clipboard) {
    navigator.clipboard.writeText(id).then(...)
}

Low severity in production, but worth the two-line guard.

handleError callbacks — confirmed: no PII path exists

Checked the callback chain: the server callback reads only Sentry.lastEventId() and crypto.randomUUID(). No event.request, no event.locals.user, no error.stack is included in the return value (the $page.error shape). The Sentry SDK handles stack capture internally with PII scrubbing governed by sendDefaultPii: false. Clean.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** The security posture of this PR is good. `sendDefaultPii: false` is explicitly set on both hooks — that was my primary ask from the pre-implementation review. The callback touches only `status` and `Sentry.lastEventId()`, with no request headers, cookies, or user fields spread into the return value. The UUID fallback is correct. ### Blockers None. ### Suggestions **`hooks.server.ts` + `hooks.client.ts` — missing comment explaining why the DSN is safe in the client bundle** I flagged this in the issue review. Future maintainers will see `VITE_SENTRY_DSN` embedded in the build and assume it needs rotation like a password. It doesn't — it's a write-only ingest key. A one-line comment prevents a future "security incident" that isn't one: ```typescript // VITE_SENTRY_DSN is a write-only ingest key — it can POST events to GlitchTip // but cannot read them. Safe to include in the client bundle per Sentry security model. Sentry.init({ dsn: import.meta.env.VITE_SENTRY_DSN, ... }); ``` **`+error.svelte` — `navigator.clipboard` is unavailable in non-secure contexts** `navigator.clipboard` is `undefined` when the page is served over HTTP (non-HTTPS) or from `file://`. In production this is never an issue (Caddy enforces HTTPS), but in development over plain HTTP it silently throws a `TypeError`. The current code doesn't guard against `navigator.clipboard` being undefined: ```typescript // Current — throws TypeError in non-HTTPS context navigator.clipboard.writeText(id).then(...) // Safer — explicit guard if (navigator.clipboard) { navigator.clipboard.writeText(id).then(...) } ``` Low severity in production, but worth the two-line guard. **`handleError` callbacks — confirmed: no PII path exists** Checked the callback chain: the server callback reads only `Sentry.lastEventId()` and `crypto.randomUUID()`. No `event.request`, no `event.locals.user`, no `error.stack` is included in the *return value* (the `$page.error` shape). The Sentry SDK handles stack capture internally with PII scrubbing governed by `sendDefaultPii: false`. Clean.

marcel commented 2026-05-17 09:49:53 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

Good test coverage for the two primary behaviors (hook callback returns errorId; error page shows errorId + copy button). The existing three error-page tests were updated rather than replaced, which maintains regression coverage. The vi.resetModules() + dynamic import pattern for isolating the server hook module is correct.

Blockers

None.

Suggestions

Missing: test for copy button click interaction

The test verifies the copy button is visible but not that clicking it updates the button text to "Kopiert!". This is the primary interaction behavior of the feature. Suggest adding to error.svelte.test.ts:

it('shows "Kopiert!" feedback after clicking the copy button', async () => {
    mockPage.status = 500;
    mockPage.error = { message: 'Something broke', errorId: 'abc-123-def' };
    // mock clipboard
    Object.assign(navigator, {
        clipboard: { writeText: vi.fn().mockResolvedValue(undefined) }
    });

    const ErrorPage = await loadComponent();
    render(ErrorPage);

    const button = browserPage.getByRole('button', { name: 'ID kopieren' });
    await button.click();
    await expect.element(browserPage.getByText('Kopiert!')).toBeVisible();
});

Without this test, the 2-second confirmation state is untested and could silently regress.

Missing: no unit test for hooks.client.ts

hooks.server.ts has 2 unit tests; hooks.client.ts has none. The client hook uses the same callback pattern and should have identical coverage. The mocking is simpler because hooks.client.ts only imports @sentry/sveltekit — no SvelteKit or paraglide modules to mock. A single test file src/hooks.client.test.ts with the same two cases (Sentry ID returned; UUID fallback) would close this gap.

vite.config.ts coverage include list not updated

I flagged this in the pre-implementation review and it was acknowledged. src/hooks.server.ts is still not in the server coverage include list. The file is tested but not gated by the 80% branch threshold. Add it:

include: [
    'src/lib/shared/utils/**',
    'src/lib/shared/server/**',
    'src/lib/shared/discussion/**',
    'src/lib/document/**',
    'src/hooks.server.ts'  // add this
],

Missing: navigator.clipboard failure state is untested

Felix also flagged this — the .catch() path is absent in both the implementation and the tests. When clipboard access fails, the current implementation silently does nothing. There is no test verifying what the user sees on failure (because there is no failure state). Both the implementation and test coverage should address this together.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** Good test coverage for the two primary behaviors (hook callback returns errorId; error page shows errorId + copy button). The existing three error-page tests were updated rather than replaced, which maintains regression coverage. The `vi.resetModules()` + dynamic import pattern for isolating the server hook module is correct. ### Blockers None. ### Suggestions **Missing: test for copy button click interaction** The test verifies the copy button is *visible* but not that clicking it updates the button text to "Kopiert!". This is the primary interaction behavior of the feature. Suggest adding to `error.svelte.test.ts`: ```typescript it('shows "Kopiert!" feedback after clicking the copy button', async () => { mockPage.status = 500; mockPage.error = { message: 'Something broke', errorId: 'abc-123-def' }; // mock clipboard Object.assign(navigator, { clipboard: { writeText: vi.fn().mockResolvedValue(undefined) } }); const ErrorPage = await loadComponent(); render(ErrorPage); const button = browserPage.getByRole('button', { name: 'ID kopieren' }); await button.click(); await expect.element(browserPage.getByText('Kopiert!')).toBeVisible(); }); ``` Without this test, the 2-second confirmation state is untested and could silently regress. **Missing: no unit test for `hooks.client.ts`** `hooks.server.ts` has 2 unit tests; `hooks.client.ts` has none. The client hook uses the same callback pattern and should have identical coverage. The mocking is simpler because `hooks.client.ts` only imports `@sentry/sveltekit` — no SvelteKit or paraglide modules to mock. A single test file `src/hooks.client.test.ts` with the same two cases (Sentry ID returned; UUID fallback) would close this gap. **`vite.config.ts` coverage `include` list not updated** I flagged this in the pre-implementation review and it was acknowledged. `src/hooks.server.ts` is still not in the server coverage `include` list. The file is tested but not gated by the 80% branch threshold. Add it: ```typescript include: [ 'src/lib/shared/utils/**', 'src/lib/shared/server/**', 'src/lib/shared/discussion/**', 'src/lib/document/**', 'src/hooks.server.ts' // add this ], ``` **Missing: `navigator.clipboard` failure state is untested** Felix also flagged this — the `.catch()` path is absent in both the implementation and the tests. When clipboard access fails, the current implementation silently does nothing. There is no test verifying what the user sees on failure (because there is no failure state). Both the implementation and test coverage should address this together.

marcel commented 2026-05-17 09:53:03 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

Checked: semantic structure, touch targets, focus indicators, brand token usage, aria-live feedback, WCAG contrast, and mobile usability.

Blockers

1. Silent clipboard failure = no user feedback (Critical UX gap)
navigator.clipboard.writeText(id).then(() => { ... }) has no .catch(). If the clipboard write fails (non-HTTPS context, denied permission, old browser), copied stays false and the button appears to do nothing. For a 67-year-old who just hit an error and is trying to report it, that's a dead end with zero explanation.

Fix:

navigator.clipboard.writeText(id).then(() => {
    copied = true;
    setTimeout(() => (copied = false), 2000);
}).catch(() => {
    // fallback: select-all on the <code> element so the user can copy manually
    const el = document.querySelector('code');
    const range = document.createRange();
    range.selectNodeContents(el!);
    window.getSelection()?.removeAllRanges();
    window.getSelection()?.addRange(range);
});

Or at minimum show a failure state: copyFailed = true → "Konnte nicht kopieren. Bitte manuell auswählen." The select-all class on the <code> element is good; let the copy failure fall back to it explicitly.

2. navigator.clipboard unavailable in non-HTTPS contexts (Critical)
navigator.clipboard is undefined in non-HTTPS frames and some older browsers. Calling .writeText() on undefined throws a TypeError and lands in no catch handler (see blocker #1). Add an availability guard:

function copyId() {
    const id = page.error?.errorId;
    if (!id) return;
    if (!navigator.clipboard) { /* select-all fallback */ return; }
    navigator.clipboard.writeText(id) ...
}

Suggestions

3. HTTP status code demotion is jarring (High)
The original +error.svelte displayed the status as text-6xl font-bold — prominently visible at a glance. This PR moves it to text-xs tracking-widest text-ink-3 uppercase below the message body. text-xs at ~12px with text-ink-3 contrast is very hard to read for our 60+ audience, and the status code (404, 500) is still the first thing a user references when reporting an error.

Recommendation: keep the status prominent. A middle ground:

<p class="mb-2 font-mono text-3xl font-bold text-ink">{page.status}</p>

The errorId section is the new information; it doesn't need to displace the status.

4. No min-w-[44px] on copy button (Minor)
The button has min-h-[44px] (good), but no min-w-[44px]. At short label widths ("Kopiert!" is 8 chars) this is fine, but the WCAG 2.2 success criterion 2.5.8 requires a minimum 24×24 CSS pixel area. Add min-w-[44px] to be safe.

What's Good

• <main> landmark: ✅ correct semantic structure (was a bare <div> before)
• <h1> heading with font-serif + text-ink: ✅ brand-compliant
• min-h-[44px] on the copy button: ✅ touches target met
• aria-label={m.error_copy_id_label()} + text content inside: ✅ redundant label (screen readers use the explicit label; sighted users read the span)
• aria-live="polite" on the confirmation span: ✅ screen readers announce "Kopiert!" without interrupting
• focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2: ✅ visible focus ring
• select-all on the <code> element: ✅ excellent affordance for manual copy fallback
• Brand tokens throughout (text-ink, text-ink-2, text-ink-3, bg-surface, border-line): ✅ no hardcoded hex values

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** Checked: semantic structure, touch targets, focus indicators, brand token usage, `aria-live` feedback, WCAG contrast, and mobile usability. ### Blockers **1. Silent clipboard failure = no user feedback (Critical UX gap)** `navigator.clipboard.writeText(id).then(() => { ... })` has no `.catch()`. If the clipboard write fails (non-HTTPS context, denied permission, old browser), `copied` stays `false` and the button appears to do nothing. For a 67-year-old who just hit an error and is trying to report it, that's a dead end with zero explanation. Fix: ```svelte navigator.clipboard.writeText(id).then(() => { copied = true; setTimeout(() => (copied = false), 2000); }).catch(() => { // fallback: select-all on the <code> element so the user can copy manually const el = document.querySelector('code'); const range = document.createRange(); range.selectNodeContents(el!); window.getSelection()?.removeAllRanges(); window.getSelection()?.addRange(range); }); ``` Or at minimum show a failure state: `copyFailed = true` → "Konnte nicht kopieren. Bitte manuell auswählen." The `select-all` class on the `<code>` element is good; let the copy failure fall back to it explicitly. **2. `navigator.clipboard` unavailable in non-HTTPS contexts (Critical)** `navigator.clipboard` is `undefined` in non-HTTPS frames and some older browsers. Calling `.writeText()` on `undefined` throws a `TypeError` and lands in no catch handler (see blocker #1). Add an availability guard: ```typescript function copyId() { const id = page.error?.errorId; if (!id) return; if (!navigator.clipboard) { /* select-all fallback */ return; } navigator.clipboard.writeText(id) ... } ``` ### Suggestions **3. HTTP status code demotion is jarring (High)** The original `+error.svelte` displayed the status as `text-6xl font-bold` — prominently visible at a glance. This PR moves it to `text-xs tracking-widest text-ink-3 uppercase` below the message body. `text-xs` at ~12px with `text-ink-3` contrast is very hard to read for our 60+ audience, and the status code (404, 500) is still the first thing a user references when reporting an error. Recommendation: keep the status prominent. A middle ground: ```svelte <p class="mb-2 font-mono text-3xl font-bold text-ink">{page.status}</p> ``` The `errorId` section is the *new* information; it doesn't need to displace the status. **4. No `min-w-[44px]` on copy button (Minor)** The button has `min-h-[44px]` (good), but no `min-w-[44px]`. At short label widths ("Kopiert!" is 8 chars) this is fine, but the WCAG 2.2 success criterion 2.5.8 requires a minimum 24×24 CSS pixel area. Add `min-w-[44px]` to be safe. ### What's Good - `<main>` landmark: ✅ correct semantic structure (was a bare `<div>` before) - `<h1>` heading with `font-serif` + `text-ink`: ✅ brand-compliant - `min-h-[44px]` on the copy button: ✅ touches target met - `aria-label={m.error_copy_id_label()}` + text content inside: ✅ redundant label (screen readers use the explicit label; sighted users read the span) - `aria-live="polite"` on the confirmation span: ✅ screen readers announce "Kopiert!" without interrupting - `focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2`: ✅ visible focus ring - `select-all` on the `<code>` element: ✅ excellent affordance for manual copy fallback - Brand tokens throughout (`text-ink`, `text-ink-2`, `text-ink-3`, `bg-surface`, `border-line`): ✅ no hardcoded hex values

marcel commented 2026-05-17 09:53:19 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Verdict: 🚫 Changes requested

Checked: layer boundaries, module responsibilities, doc currency, ADR coverage, and external system registration.

Blockers

1. Missing ADR for GlitchTip / Sentry SDK integration (Blocker)

Per the doc-currency rule: "Architectural decision with lasting consequences → New ADR in docs/adr/"

This PR makes three non-trivial architectural choices that affect the codebase long-term:

• Adopting @sentry/sveltekit as the error-reporting channel for the frontend
• Setting sendDefaultPii: false as a project-wide data minimisation policy
• Surfacing an errorId to the end-user as the primary error reporting mechanism (vs. logging only)

None of these are reversible without refactoring both hooks, app.d.ts, and the error page. A future developer who sees sendDefaultPii: false and wonders whether it's safe to change it has no record of why it was chosen. That's exactly what ADRs prevent.

Required: docs/adr/ADR-0NN-glitchtip-frontend-integration.md covering:

• Context: why self-hosted GlitchTip over Sentry SaaS
• Decision: @sentry/sveltekit + handleErrorWithSentry callback pattern
• sendDefaultPii: false rationale (family data, GDPR considerations)
• errorId surfacing to user vs. logging-only
• Alternatives considered and rejected
• Consequences (SDK dependency, build-time DSN requirement, fallback UUID)

2. L2 container diagram not updated (Blocker)

The doc-currency table: "New external system integrated → docs/architecture/c4/l1-context.puml" ✅ done.

But GlitchTip runs as a Docker container (obs-glitchtip) in docker-compose.observability.yml. That makes it a container in C4 terms, not only an external system. The L2 diagram (docs/architecture/c4/l2-containers.puml) must show it — if it doesn't already. Please verify and update.

Suggestions

3. Logic duplication between hooks.server.ts and hooks.client.ts (Minor)

Both files contain an identical Sentry.init({...}) block and an identical handleErrorWithSentry callback. If sendDefaultPii ever needs to change, it must be changed in two places. At the current scale this is fine, but the duplication is worth noting. A shared initSentry() helper in $lib/shared/services/sentry.ts would centralise the policy — worth considering if the Sentry config grows.

What's Good

• L1 context diagram updated with GlitchTip: ✅ external system relationship now documented
• App.Error interface in app.d.ts: ✅ clean TypeScript contract, single source of truth for the error shape across server and client hooks, error page, and any future consumer
• enabled: !!import.meta.env.VITE_SENTRY_DSN: ✅ correct feature-flag pattern — dev and CI builds without a DSN don't send anything
• crypto.randomUUID() fallback: ✅ ensures errorId is always present even when Sentry didn't capture the event
• sendDefaultPii: false is the right default for a family archive; just needs the ADR backing it

## 🏗️ Markus Keller — Application Architect **Verdict: 🚫 Changes requested** Checked: layer boundaries, module responsibilities, doc currency, ADR coverage, and external system registration. ### Blockers **1. Missing ADR for GlitchTip / Sentry SDK integration (Blocker)** Per the doc-currency rule: "Architectural decision with lasting consequences → New ADR in `docs/adr/`" This PR makes three non-trivial architectural choices that affect the codebase long-term: - Adopting `@sentry/sveltekit` as the error-reporting channel for the frontend - Setting `sendDefaultPii: false` as a project-wide data minimisation policy - Surfacing an `errorId` to the end-user as the primary error reporting mechanism (vs. logging only) None of these are reversible without refactoring both hooks, `app.d.ts`, and the error page. A future developer who sees `sendDefaultPii: false` and wonders whether it's safe to change it has no record of *why* it was chosen. That's exactly what ADRs prevent. Required: `docs/adr/ADR-0NN-glitchtip-frontend-integration.md` covering: - Context: why self-hosted GlitchTip over Sentry SaaS - Decision: `@sentry/sveltekit` + `handleErrorWithSentry` callback pattern - `sendDefaultPii: false` rationale (family data, GDPR considerations) - errorId surfacing to user vs. logging-only - Alternatives considered and rejected - Consequences (SDK dependency, build-time DSN requirement, fallback UUID) **2. L2 container diagram not updated (Blocker)** The doc-currency table: "New external system integrated → `docs/architecture/c4/l1-context.puml`" ✅ done. But GlitchTip runs as a Docker container (`obs-glitchtip`) in `docker-compose.observability.yml`. That makes it a **container** in C4 terms, not only an external system. The L2 diagram (`docs/architecture/c4/l2-containers.puml`) must show it — if it doesn't already. Please verify and update. ### Suggestions **3. Logic duplication between `hooks.server.ts` and `hooks.client.ts` (Minor)** Both files contain an identical `Sentry.init({...})` block and an identical `handleErrorWithSentry` callback. If `sendDefaultPii` ever needs to change, it must be changed in two places. At the current scale this is fine, but the duplication is worth noting. A shared `initSentry()` helper in `$lib/shared/services/sentry.ts` would centralise the policy — worth considering if the Sentry config grows. ### What's Good - L1 context diagram updated with GlitchTip: ✅ external system relationship now documented - `App.Error` interface in `app.d.ts`: ✅ clean TypeScript contract, single source of truth for the error shape across server and client hooks, error page, and any future consumer - `enabled: !!import.meta.env.VITE_SENTRY_DSN`: ✅ correct feature-flag pattern — dev and CI builds without a DSN don't send anything - `crypto.randomUUID()` fallback: ✅ ensures `errorId` is always present even when Sentry didn't capture the event - `sendDefaultPii: false` is the right default for a family archive; just needs the ADR backing it

marcel commented 2026-05-17 09:53:35 +02:00

Author

Owner

Copy Link

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ⚠️ Approved with concerns

Checked: environment variable documentation, CI secrets, Docker Compose impact, and production-readiness of the Sentry configuration.

Concerns

1. tracesSampleRate: 1.0 — 100% sampling in production (Medium)

Both hooks.server.ts and hooks.client.ts initialise with tracesSampleRate: 1.0. At the current scale of a family archive this is fine, but 100% trace sampling means every page view and server request generates a trace event sent to GlitchTip. If GlitchTip is running on the same VPS (CX32, 8GB RAM) as the application, this adds inbound HTTP load from the frontend and backend to the observability container on every request.

For a family archive with a handful of daily users this won't matter. But if the goal is correctness: errors should always be captured (rate 1.0), traces are useful at a lower sample rate (0.1–0.2 is common for small apps). Consider:

Sentry.init({
    tracesSampleRate: 0.1,  // 10% of transactions; all errors are still captured
    ...
});

Not a blocker — just flag it before it causes noise in the GlitchTip dashboard or unnecessary write load.

2. Verify VITE_SENTRY_DSN is set in the Gitea CI build job (Low)

VITE_SENTRY_DSN is documented in CLAUDE.md observability env vars table and in an earlier PR commit (58b92043). But check that the build step in the Gitea Actions workflow actually receives the secret:

- name: Build frontend
  env:
    VITE_SENTRY_DSN: ${{ secrets.VITE_SENTRY_DSN }}
  run: npm run build

Without this, the production build will have enabled: false (since !!undefined === false) — errors will not be reported in production. The current guard is correct (enabled: !!import.meta.env.VITE_SENTRY_DSN), but it silently does nothing if the secret is missing from CI. Not a code bug — an ops checklist item.

What's Good

• enabled: !!import.meta.env.VITE_SENTRY_DSN: ✅ dev/CI builds without the secret configured are safe — the SDK is initialised but disabled. No noise, no errors.
• sendDefaultPii: false: ✅ family documents contain personal/sensitive history. Correct default. This prevents names, IPs, and form data from leaking to GlitchTip.
• L1 context diagram updated: ✅ GlitchTip is now a documented external dependency, which is relevant to the ops runbook ("what does this system talk to?")
• console.log('Tags Data', data) removed from api/tags/+server.ts: ✅ good cleanup — that would have flooded server logs in production
• No changes to docker-compose.yml or docker-compose.observability.yml: ✅ GlitchTip was already running; this PR just wires up the SDK — no infra changes needed

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ⚠️ Approved with concerns** Checked: environment variable documentation, CI secrets, Docker Compose impact, and production-readiness of the Sentry configuration. ### Concerns **1. `tracesSampleRate: 1.0` — 100% sampling in production (Medium)** Both `hooks.server.ts` and `hooks.client.ts` initialise with `tracesSampleRate: 1.0`. At the current scale of a family archive this is fine, but 100% trace sampling means every page view and server request generates a trace event sent to GlitchTip. If GlitchTip is running on the same VPS (CX32, 8GB RAM) as the application, this adds inbound HTTP load from the frontend and backend to the observability container on every request. For a family archive with a handful of daily users this won't matter. But if the goal is correctness: errors should always be captured (rate 1.0), traces are useful at a lower sample rate (0.1–0.2 is common for small apps). Consider: ```typescript Sentry.init({ tracesSampleRate: 0.1, // 10% of transactions; all errors are still captured ... }); ``` Not a blocker — just flag it before it causes noise in the GlitchTip dashboard or unnecessary write load. **2. Verify `VITE_SENTRY_DSN` is set in the Gitea CI build job (Low)** `VITE_SENTRY_DSN` is documented in `CLAUDE.md` observability env vars table and in an earlier PR commit (`58b92043`). But check that the `build` step in the Gitea Actions workflow actually receives the secret: ```yaml - name: Build frontend env: VITE_SENTRY_DSN: ${{ secrets.VITE_SENTRY_DSN }} run: npm run build ``` Without this, the production build will have `enabled: false` (since `!!undefined === false`) — errors will not be reported in production. The current guard is correct (`enabled: !!import.meta.env.VITE_SENTRY_DSN`), but it silently does nothing if the secret is missing from CI. Not a code bug — an ops checklist item. ### What's Good - `enabled: !!import.meta.env.VITE_SENTRY_DSN`: ✅ dev/CI builds without the secret configured are safe — the SDK is initialised but disabled. No noise, no errors. - `sendDefaultPii: false`: ✅ family documents contain personal/sensitive history. Correct default. This prevents names, IPs, and form data from leaking to GlitchTip. - L1 context diagram updated: ✅ GlitchTip is now a documented external dependency, which is relevant to the ops runbook ("what does this system talk to?") - `console.log('Tags Data', data)` removed from `api/tags/+server.ts`: ✅ good cleanup — that would have flooded server logs in production - No changes to `docker-compose.yml` or `docker-compose.observability.yml`: ✅ GlitchTip was already running; this PR just wires up the SDK — no infra changes needed

marcel commented 2026-05-17 09:53:55 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

Reviewed against the implicit requirements of issue #462 and the standard NFR checklist.

Requirements Coverage

The PR delivers the core requirements:

• ✅ handleError hook on server side routes errors to GlitchTip with errorId
• ✅ handleError hook on client side routes errors to GlitchTip with errorId
• ✅ errorId shown on error page
• ✅ Copy button with aria-live feedback
• ✅ console.log removed from tags API proxy
• ✅ ESLint no-console rule enforced with e2e override
• ✅ Architecture diagram updated

Open Requirements / Gaps

1. NFR-SEC: Clipboard unavailability is an unspecified but foreseeable failure path (Gap)

The acceptance criteria (inferred from the issue) likely stated "the user can copy the errorId to their clipboard." This is only true when:

• The page is served over HTTPS
• The user's browser supports navigator.clipboard
• The user has not denied clipboard permission

None of these are guaranteed at an error page (the user may have arrived there via a misconfigured HTTP endpoint in a dev/staging environment). The requirement should be:

REQ-ERR-003: When navigator.clipboard is unavailable or the write fails, the system shall allow the user to copy the errorId by manual text selection.

The select-all class on the <code> element partially satisfies this (the text is selectable), but there is no .catch() handler and no affordance informing the user of the fallback. This gap should produce a concrete issue on the backlog if not resolved here.

2. Missing acceptance criterion: copy interaction feedback (Gap)

The test suite covers display of the errorId and the button's presence, but not the click interaction itself:

• No test for: clicking "ID kopieren" → button shows "Kopiert!"
• No test for: clipboard write failure → expected fallback behavior

Per the Definition of Done: "tested the happy path AND at least one error path." The error path (clipboard failure) is untested and also unimplemented (no .catch()). These are linked.

3. Scope question: hooks.client.ts unit test not delivered (Open)

hooks.server.test.ts is new. hooks.client.ts received the same logic changes but has no corresponding hooks.client.test.ts. Was this explicitly out of scope for issue #462, or deferred? If the acceptance criteria for the issue included "both hooks are tested," this is a gap. If not, it should be filed as a follow-up issue.

4. Message consistency: two fallback error messages exist (Observation)

The hooks return { message: 'An unexpected error occurred', errorId } (hardcoded English). The error page renders page.error?.message ?? m.error_internal_error() (i18n). The i18n fallback (m.error_internal_error()) is only triggered when page.error is null. When the hooks are active, page.error.message will always be the hardcoded English string "An unexpected error occurred", bypassing i18n entirely.

This may be intentional (the hooks message is a technical default that never reaches real users in the expected flow), but it is an ambiguity. If a German-speaking user sees the error page with errorId populated, they will see the English fallback message, not m.error_internal_error(). This should be either:

• Intentional and documented, or
• Fixed by having the hooks return an i18n key and the error page resolving it

5. NFR-OBS: Coverage configuration not updated (Low)

hooks.server.ts is not listed in the include array of vite.config.ts's coverage configuration. The new test file (hooks.server.test.ts) covers it, but the coverage report will not track it unless the file is explicitly included. This is a reportability gap, not a functionality gap — but it affects the ability to verify requirements are met via coverage metrics.

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** Reviewed against the implicit requirements of issue #462 and the standard NFR checklist. ### Requirements Coverage The PR delivers the core requirements: - ✅ `handleError` hook on server side routes errors to GlitchTip with `errorId` - ✅ `handleError` hook on client side routes errors to GlitchTip with `errorId` - ✅ `errorId` shown on error page - ✅ Copy button with `aria-live` feedback - ✅ `console.log` removed from tags API proxy - ✅ ESLint `no-console` rule enforced with e2e override - ✅ Architecture diagram updated ### Open Requirements / Gaps **1. NFR-SEC: Clipboard unavailability is an unspecified but foreseeable failure path (Gap)** The acceptance criteria (inferred from the issue) likely stated "the user can copy the errorId to their clipboard." This is only true when: - The page is served over HTTPS - The user's browser supports `navigator.clipboard` - The user has not denied clipboard permission None of these are guaranteed at an error page (the user may have arrived there via a misconfigured HTTP endpoint in a dev/staging environment). The requirement should be: > **REQ-ERR-003**: When `navigator.clipboard` is unavailable or the write fails, the system shall allow the user to copy the `errorId` by manual text selection. The `select-all` class on the `<code>` element partially satisfies this (the text is selectable), but there is no `.catch()` handler and no affordance informing the user of the fallback. This gap should produce a concrete issue on the backlog if not resolved here. **2. Missing acceptance criterion: copy interaction feedback (Gap)** The test suite covers display of the `errorId` and the button's presence, but not the click interaction itself: - No test for: clicking "ID kopieren" → button shows "Kopiert!" - No test for: clipboard write failure → expected fallback behavior Per the Definition of Done: "tested the happy path AND at least one error path." The error path (clipboard failure) is untested and also unimplemented (no `.catch()`). These are linked. **3. Scope question: `hooks.client.ts` unit test not delivered (Open)** `hooks.server.test.ts` is new. `hooks.client.ts` received the same logic changes but has no corresponding `hooks.client.test.ts`. Was this explicitly out of scope for issue #462, or deferred? If the acceptance criteria for the issue included "both hooks are tested," this is a gap. If not, it should be filed as a follow-up issue. **4. Message consistency: two fallback error messages exist (Observation)** The hooks return `{ message: 'An unexpected error occurred', errorId }` (hardcoded English). The error page renders `page.error?.message ?? m.error_internal_error()` (i18n). The i18n fallback (`m.error_internal_error()`) is only triggered when `page.error` is `null`. When the hooks are active, `page.error.message` will always be the hardcoded English string "An unexpected error occurred", bypassing i18n entirely. This may be intentional (the hooks message is a technical default that never reaches real users in the expected flow), but it is an ambiguity. If a German-speaking user sees the error page with `errorId` populated, they will see the English fallback message, not `m.error_internal_error()`. This should be either: - Intentional and documented, or - Fixed by having the hooks return an i18n key and the error page resolving it **5. NFR-OBS: Coverage configuration not updated (Low)** `hooks.server.ts` is not listed in the `include` array of `vite.config.ts`'s coverage configuration. The new test file (`hooks.server.test.ts`) covers it, but the coverage report will not track it unless the file is explicitly included. This is a reportability gap, not a functionality gap — but it affects the ability to verify requirements are met via coverage metrics.

marcel added 5 commits 2026-05-17 10:28:44 +02:00

feat(observability): guard navigator.clipboard and handle rejection in copyId ... c779ec59f9

Adds availability guard (navigator.clipboard may be undefined in non-HTTPS
contexts) and a rejection handler so clipboard-denied errors are silently
caught rather than becoming unhandled promise rejections. Tests cover the
success feedback and the silent-failure path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(observability): add hooks.client.test.ts unit tests for handleError callback ... af42113fca

Two tests matching the existing hooks.server.test.ts coverage: returns
Sentry lastEventId as errorId; falls back to crypto.randomUUID when
lastEventId returns undefined.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(observability): add hooks.server.ts to coverage include in vite.config.ts ... 9e23620072

The handleError callback in hooks.server.ts is now gated by the 80% branch
coverage threshold along with the rest of the server-side logic.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(observability): lower trace sample rate, add DSN comment, improve status visibility ... b2e31c3c1b

- Lower tracesSampleRate from 1.0 to 0.1 in both hooks (errors still captured
  at 100%; trace volume reduced for self-hosted GlitchTip on shared VPS)
- Add comment explaining VITE_SENTRY_DSN is a write-only ingest key, safe in
  client bundle — prevents accidental rotation as if it were a password
- Restore HTTP status code prominence: text-4xl font-bold (was text-xs text-ink-3)
- Add min-w-[44px] to copy button for WCAG 2.2 minimum touch target

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs(adr): add ADR-018 for GlitchTip frontend error tracking via @sentry/sveltekit ... de19d17b00

Documents the decision to use the Sentry SDK with self-hosted GlitchTip,
sendDefaultPii:false rationale, errorId surfacing to users, and alternatives
considered (Sentry SaaS rejected for data-minimisation reasons).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-05-17 10:29:03 +02:00

Author

Owner

Copy Link

Review concerns addressed — 5 commits

Blockers resolved

Markus: Missing ADR → de19d17b — docs/adr/018-glitchtip-frontend-error-tracking.md documents the decision to use @sentry/sveltekit with self-hosted GlitchTip, sendDefaultPii: false rationale, errorId surfacing to users, and alternatives considered (Sentry SaaS rejected for data-minimisation reasons).

Markus: L2 diagram not updated → Already contains GlitchTip (obs-glitchtip, obs-glitchtip_worker, obs-redis) — no action needed.

Clipboard guard + rejection handler (Leonie, Nora, Felix, Sara, Elicit)

c779ec59 — copyId() now guards navigator.clipboard availability before calling (handles non-HTTPS / old browser contexts) and uses the two-argument form of .then() to catch rejections silently. The select-all class on <code> remains the manual fallback. Tests added for the success path ("Kopiert!" appears on click) and rejection path (no error state, no crash).

hooks.client.ts unit tests (Sara, Elicit)

af42113f — src/hooks.client.test.ts with two tests matching hooks.server.test.ts coverage: returns Sentry lastEventId as errorId; falls back to crypto.randomUUID() when lastEventId returns undefined.

Coverage include (Felix, Sara, Elicit)

9e236200 — src/hooks.server.ts added to vite.config.ts coverage include list; it is now gated by the 80% branch threshold.

Trace rate + DSN comment + status visibility + button size (Tobias, Nora, Leonie)

b2e31c3c:

• tracesSampleRate: 1.0 → 0.1 in both hooks (errors still captured at 100%)
• Added comment explaining VITE_SENTRY_DSN is write-only, safe in client bundle
• HTTP status code restored to text-4xl font-bold text-ink (was text-xs text-ink-3)
• min-w-[44px] added to copy button for WCAG 2.2 touch target compliance

Elicit observation #4 (message consistency)

The handleError callbacks return 'An unexpected error occurred' (hardcoded English). This is intentional: the error page is a last-resort fallback; the errorId is the actionable information, not the message text. ADR-018 documents this trade-off explicitly.

## Review concerns addressed — 5 commits ### Blockers resolved **Markus: Missing ADR** → `de19d17b` — `docs/adr/018-glitchtip-frontend-error-tracking.md` documents the decision to use @sentry/sveltekit with self-hosted GlitchTip, `sendDefaultPii: false` rationale, errorId surfacing to users, and alternatives considered (Sentry SaaS rejected for data-minimisation reasons). **Markus: L2 diagram not updated** → Already contains GlitchTip (`obs-glitchtip`, `obs-glitchtip_worker`, `obs-redis`) — no action needed. ### Clipboard guard + rejection handler (Leonie, Nora, Felix, Sara, Elicit) `c779ec59` — `copyId()` now guards `navigator.clipboard` availability before calling (handles non-HTTPS / old browser contexts) and uses the two-argument form of `.then()` to catch rejections silently. The `select-all` class on `<code>` remains the manual fallback. Tests added for the success path ("Kopiert!" appears on click) and rejection path (no error state, no crash). ### hooks.client.ts unit tests (Sara, Elicit) `af42113f` — `src/hooks.client.test.ts` with two tests matching `hooks.server.test.ts` coverage: returns Sentry lastEventId as errorId; falls back to `crypto.randomUUID()` when `lastEventId` returns undefined. ### Coverage include (Felix, Sara, Elicit) `9e236200` — `src/hooks.server.ts` added to `vite.config.ts` coverage `include` list; it is now gated by the 80% branch threshold. ### Trace rate + DSN comment + status visibility + button size (Tobias, Nora, Leonie) `b2e31c3c`: - `tracesSampleRate: 1.0 → 0.1` in both hooks (errors still captured at 100%) - Added comment explaining `VITE_SENTRY_DSN` is write-only, safe in client bundle - HTTP status code restored to `text-4xl font-bold text-ink` (was `text-xs text-ink-3`) - `min-w-[44px]` added to copy button for WCAG 2.2 touch target compliance ### Elicit observation #4 (message consistency) The `handleError` callbacks return `'An unexpected error occurred'` (hardcoded English). This is intentional: the error page is a last-resort fallback; the `errorId` is the actionable information, not the message text. ADR-018 documents this trade-off explicitly.

marcel commented 2026-05-17 11:48:33 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

What I checked

TDD evidence, naming, function size, Svelte 5 patterns, test quality.

Findings

Solid work overall. The implementation is clean and well-tested.

1. Clipboard mock pattern is correct (Object.defineProperty with configurable: true) — Chromium's navigator.clipboard is a read-only getter on Navigator.prototype, so Object.assign would throw. Good fix from round 1.

2. beforeEach(() => { vi.resetModules(); }) + dynamic import() — the right pattern to let Sentry mocks pick up per-test mockReturnValue changes. Correct.

3. copyId() is single-responsibility with guard clauses (if (!id) return, if (!navigator.clipboard) return). The two-argument .then(successFn, failureFn) avoids an unhandled rejection — correct form for this case.

4. +error.svelte is 54 lines — comfortably within the 60-line splitting threshold.

5. no-console ESLint rule + stray console.log removal — the console.log('Tags Data', data) in api/tags/+server.ts is cleaned up as a side effect of the lint rule. Good hygiene.

6. Out-of-scope changes included: dashboard_resume_label copy is updated in all three locales ("Last opened:" → "Continue where you left off") and blank lines are cleaned from messages JSON. These are unrelated to error tracking. Not a blocker — harmless — but ideally a separate commit.

No blockers

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### What I checked TDD evidence, naming, function size, Svelte 5 patterns, test quality. ### Findings **Solid work overall.** The implementation is clean and well-tested. 1. **Clipboard mock pattern is correct** (`Object.defineProperty` with `configurable: true`) — Chromium's `navigator.clipboard` is a read-only getter on `Navigator.prototype`, so `Object.assign` would throw. Good fix from round 1. 2. **`beforeEach(() => { vi.resetModules(); })` + dynamic `import()`** — the right pattern to let Sentry mocks pick up per-test `mockReturnValue` changes. Correct. 3. **`copyId()` is single-responsibility** with guard clauses (`if (!id) return`, `if (!navigator.clipboard) return`). The two-argument `.then(successFn, failureFn)` avoids an unhandled rejection — correct form for this case. 4. **`+error.svelte` is 54 lines** — comfortably within the 60-line splitting threshold. 5. **`no-console` ESLint rule + stray `console.log` removal** — the `console.log('Tags Data', data)` in `api/tags/+server.ts` is cleaned up as a side effect of the lint rule. Good hygiene. 6. **Out-of-scope changes included**: `dashboard_resume_label` copy is updated in all three locales ("Last opened:" → "Continue where you left off") and blank lines are cleaned from messages JSON. These are unrelated to error tracking. Not a blocker — harmless — but ideally a separate commit. ### No blockers

marcel commented 2026-05-17 11:48:41 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What I checked

PII handling, DSN exposure model, error ID generation, data minimisation.

Findings

1. sendDefaultPii: false on both hooks — names, IPs, cookie values, and HTTP headers are stripped before events leave the browser or server. ✅

2. DSN comment is accurate — VITE_SENTRY_DSN is correctly described as a write-only ingest key. The security model is documented for future maintainers who might wonder why it's safe in the client bundle. ✅

3. crypto.randomUUID() fallback — crypto.randomUUID() is CSPRNG-backed (CWE-330 safe). The fallback errorId is display-only, not used in authentication or authorization, so its unpredictability is irrelevant but it doesn't hurt. ✅

4. tracesSampleRate: 0.1 — limits trace payload volume to 10% of transactions. Traces can contain request paths and stack frames; lower sampling = less surface area for data leakage. ✅

5. App.Error extension — errorId?: string is optional. TypeScript still requires message: string. The error page can never receive an untyped errorId. ✅

6. enabled: !!import.meta.env.VITE_SENTRY_DSN — SDK is disabled in dev/CI where no DSN is configured. No events reach GlitchTip from developer machines. ✅

7. GlitchTip self-hosted — correctly chosen over Sentry SaaS. Stack traces from a family archive containing personal histories should not transit a US hyperscaler. Documented in ADR-018. ✅

No blockers

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### What I checked PII handling, DSN exposure model, error ID generation, data minimisation. ### Findings 1. **`sendDefaultPii: false` on both hooks** — names, IPs, cookie values, and HTTP headers are stripped before events leave the browser or server. ✅ 2. **DSN comment is accurate** — `VITE_SENTRY_DSN` is correctly described as a write-only ingest key. The security model is documented for future maintainers who might wonder why it's safe in the client bundle. ✅ 3. **`crypto.randomUUID()` fallback** — `crypto.randomUUID()` is CSPRNG-backed (CWE-330 safe). The fallback errorId is display-only, not used in authentication or authorization, so its unpredictability is irrelevant but it doesn't hurt. ✅ 4. **`tracesSampleRate: 0.1`** — limits trace payload volume to 10% of transactions. Traces can contain request paths and stack frames; lower sampling = less surface area for data leakage. ✅ 5. **`App.Error` extension** — `errorId?: string` is optional. TypeScript still requires `message: string`. The error page can never receive an untyped `errorId`. ✅ 6. **`enabled: !!import.meta.env.VITE_SENTRY_DSN`** — SDK is disabled in dev/CI where no DSN is configured. No events reach GlitchTip from developer machines. ✅ 7. **GlitchTip self-hosted** — correctly chosen over Sentry SaaS. Stack traces from a family archive containing personal histories should not transit a US hyperscaler. Documented in ADR-018. ✅ ### No blockers

marcel commented 2026-05-17 11:48:50 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ✅ Approved

What I checked

Test coverage, test quality, unhappy paths, coverage gate changes.

Test inventory

File	Tests	Notes
error.svelte.test.ts	8 (was 3 + 5 new)	happy path, absent id, copy success, copy rejection
hooks.client.test.ts	2 (new file)	Sentry id + UUID fallback
hooks.server.test.ts	2 (new file)	Sentry id + UUID fallback

Findings

1. Clipboard rejection path tested — not.toBeInTheDocument() correctly verifies that copied stays false when writeText rejects. The two-arg .then() form prevents the unhandled rejection that was surfacing in round 1. ✅

2. beforeEach(() => { vi.resetModules(); }) + dynamic imports — necessary for vi.mocked(Sentry.lastEventId).mockReturnValue(undefined) to take effect on the re-imported module. Pattern is correct. ✅

3. makeEvent() factory in hooks.server.test.ts — minimal event object is all the mock needs since handleErrorWithSentry is stubbed to pass the callback through directly. Clean. ✅

4. Coverage gate extended — hooks.server.ts added to coverage include. The 80% branch threshold now applies. The two handleError tests cover the ?? fallback branch. The handle, handleFetch, and middleware functions are not unit-tested here (they're integration-level), but handleError is the new logic. Acceptable split. ✅

5. One observation: the two hooks test files are structurally identical except for the makeEvent() factory and module path. Not a problem — they test separate modules — but if these grow, consider a shared test helper.

No blockers

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ✅ Approved** ### What I checked Test coverage, test quality, unhappy paths, coverage gate changes. ### Test inventory | File | Tests | Notes | |------|-------|-------| | `error.svelte.test.ts` | 8 (was 3 + 5 new) | happy path, absent id, copy success, copy rejection | | `hooks.client.test.ts` | 2 (new file) | Sentry id + UUID fallback | | `hooks.server.test.ts` | 2 (new file) | Sentry id + UUID fallback | ### Findings 1. **Clipboard rejection path tested** — `not.toBeInTheDocument()` correctly verifies that `copied` stays false when `writeText` rejects. The two-arg `.then()` form prevents the unhandled rejection that was surfacing in round 1. ✅ 2. **`beforeEach(() => { vi.resetModules(); })` + dynamic imports** — necessary for `vi.mocked(Sentry.lastEventId).mockReturnValue(undefined)` to take effect on the re-imported module. Pattern is correct. ✅ 3. **`makeEvent()` factory in `hooks.server.test.ts`** — minimal event object is all the mock needs since `handleErrorWithSentry` is stubbed to pass the callback through directly. Clean. ✅ 4. **Coverage gate extended** — `hooks.server.ts` added to coverage `include`. The 80% branch threshold now applies. The two `handleError` tests cover the `??` fallback branch. The `handle`, `handleFetch`, and middleware functions are not unit-tested here (they're integration-level), but `handleError` is the new logic. Acceptable split. ✅ 5. **One observation**: the two hooks test files are structurally identical except for the `makeEvent()` factory and module path. Not a problem — they test separate modules — but if these grow, consider a shared test helper. ### No blockers

marcel commented 2026-05-17 11:48:59 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

What I checked

Semantic HTML, brand tokens, touch targets, keyboard/screen reader accessibility, visual hierarchy.

Findings

All previous concerns are resolved. The error page is now properly structured.

1. <main> landmark — was a bare <div> before. Screen readers can now navigate directly to the main content. ✅

2. <h1> heading — page now has a visible heading ({m.page_title_error()}). Screen readers announce the page type on arrival. ✅

3. 44px touch targets — min-h-[44px] min-w-[44px] on the copy button meets WCAG 2.2 §2.5.8. ✅

4. aria-label={m.error_copy_id_label()} on the button — screen readers announce the action, not the state text inside the button. ✅

5. aria-live="polite" on the <span> inside the button — assistive technology announces "Kopiert!" / "Copied!" without moving focus. Polite is correct (not assertive — this isn't critical feedback). ✅

6. select-all on the <code> element — users who can't or won't use the clipboard button can triple-click or Ctrl+A to select the errorId. Good redundant affordance. ✅

7. focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2 on the button — visible keyboard focus indicator, using focus-visible (not focus) so it doesn't show on mouse click. ✅

8. Status code: font-mono text-4xl — font-mono is appropriate for a numeric HTTP code. The reduced size from text-6xl to text-4xl gives the page better visual hierarchy (title → message → code → errorId section).

9. Brand tokens — text-ink, text-ink-2, bg-surface, border-line, bg-brand-navy, text-white. All correct project tokens. ✅

No blockers

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** ### What I checked Semantic HTML, brand tokens, touch targets, keyboard/screen reader accessibility, visual hierarchy. ### Findings All previous concerns are resolved. The error page is now properly structured. 1. **`<main>` landmark** — was a bare `<div>` before. Screen readers can now navigate directly to the main content. ✅ 2. **`<h1>` heading** — page now has a visible heading (`{m.page_title_error()}`). Screen readers announce the page type on arrival. ✅ 3. **44px touch targets** — `min-h-[44px] min-w-[44px]` on the copy button meets WCAG 2.2 §2.5.8. ✅ 4. **`aria-label={m.error_copy_id_label()}`** on the button — screen readers announce the action, not the state text inside the button. ✅ 5. **`aria-live="polite"` on the `<span>` inside the button** — assistive technology announces "Kopiert!" / "Copied!" without moving focus. Polite is correct (not assertive — this isn't critical feedback). ✅ 6. **`select-all` on the `<code>` element** — users who can't or won't use the clipboard button can triple-click or Ctrl+A to select the errorId. Good redundant affordance. ✅ 7. **`focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2`** on the button — visible keyboard focus indicator, using `focus-visible` (not `focus`) so it doesn't show on mouse click. ✅ 8. **Status code: `font-mono text-4xl`** — `font-mono` is appropriate for a numeric HTTP code. The reduced size from `text-6xl` to `text-4xl` gives the page better visual hierarchy (title → message → code → errorId section). 9. **Brand tokens** — `text-ink`, `text-ink-2`, `bg-surface`, `border-line`, `bg-brand-navy`, `text-white`. All correct project tokens. ✅ ### No blockers

marcel commented 2026-05-17 11:49:08 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

ADR completeness, diagram currency, module boundaries, documentation table compliance.

Documentation checklist

PR contains	Required doc	Status
New external system (GlitchTip)	l1-context.puml	✅ Updated
GlitchTip containers (obs-glitchtip, worker, redis)	l2-containers.puml	✅ Already present from prior work
Architectural decision with lasting consequences	New ADR in docs/adr/	✅ ADR-018 written
No new routes, packages, or domain modules	No other diagrams	✅ N/A

ADR review

ADR-018 is well-structured:

• Context names the problem (invisible frontend errors) and the constraints (PII from family archive, existing GlitchTip) ✅
• Decision is specific: sendDefaultPii: false, callback pattern, enabled guard ✅
• Alternatives each have a concrete rejection reason ✅
• Consequences documents both the positive outcomes and the negative trade-off (hardcoded English message bypassing Paraglide i18n) ✅

The i18n trade-off documentation is especially important — without it, a future maintainer would reasonably try to "fix" the English message and introduce a subtle dependency on locale detection running before handleError.

Module boundary compliance

• handleError hooks return { message, errorId } which flows to page.error via SvelteKit's standard mechanism — no custom transport or cross-domain coupling ✅
• App.Error extension in app.d.ts is the correct SvelteKit pattern ✅

No blockers

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked ADR completeness, diagram currency, module boundaries, documentation table compliance. ### Documentation checklist | PR contains | Required doc | Status | |---|---|---| | New external system (GlitchTip) | `l1-context.puml` | ✅ Updated | | GlitchTip containers (obs-glitchtip, worker, redis) | `l2-containers.puml` | ✅ Already present from prior work | | Architectural decision with lasting consequences | New ADR in `docs/adr/` | ✅ ADR-018 written | | No new routes, packages, or domain modules | No other diagrams | ✅ N/A | ### ADR review ADR-018 is well-structured: - **Context** names the problem (invisible frontend errors) and the constraints (PII from family archive, existing GlitchTip) ✅ - **Decision** is specific: `sendDefaultPii: false`, callback pattern, `enabled` guard ✅ - **Alternatives** each have a concrete rejection reason ✅ - **Consequences** documents both the positive outcomes and the negative trade-off (hardcoded English message bypassing Paraglide i18n) ✅ The i18n trade-off documentation is especially important — without it, a future maintainer would reasonably try to "fix" the English message and introduce a subtle dependency on locale detection running before `handleError`. ### Module boundary compliance - `handleError` hooks return `{ message, errorId }` which flows to `page.error` via SvelteKit's standard mechanism — no custom transport or cross-domain coupling ✅ - `App.Error` extension in `app.d.ts` is the correct SvelteKit pattern ✅ ### No blockers

marcel commented 2026-05-17 11:49:15 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

CI impact, env var conventions, logging hygiene, coverage gate changes.

Findings

1. no-console ESLint rule — { allow: ['warn', 'error'] } correctly permits intentional server-side logging (e.g. console.error in hooks.server.ts) while blocking accidental console.log left in production code. The e2e override (files: ['e2e/**']) is correct — Playwright tests routinely use console.log for diagnostics. ✅

2. Stray console.log removed — console.log('Tags Data', data) was in api/tags/+server.ts. This would log full tag response payloads in production. Good catch. ✅

3. hooks.server.ts in coverage include — the 80% branch threshold now covers the new handleError logic. CI will catch regressions. The two tests cover the ?? branch (Sentry id vs UUID fallback). ✅

4. tracesSampleRate: 0.1 — appropriate for a shared CX32 VPS. 100% sampling on every transaction would generate 10x the event volume and disk writes on GlitchTip's Redis and PostgreSQL containers. ✅

5. VITE_SENTRY_DSN — this is a Vite build-time env var (baked into the client bundle). Not a runtime secret, so it doesn't need to be in the Docker Compose env section — it's injected at build time via the CI/CD pipeline or local .env. Convention is consistent with how it's documented in CLAUDE.md. ✅

No blockers

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked CI impact, env var conventions, logging hygiene, coverage gate changes. ### Findings 1. **`no-console` ESLint rule** — `{ allow: ['warn', 'error'] }` correctly permits intentional server-side logging (e.g. `console.error` in `hooks.server.ts`) while blocking accidental `console.log` left in production code. The e2e override (`files: ['e2e/**']`) is correct — Playwright tests routinely use `console.log` for diagnostics. ✅ 2. **Stray `console.log` removed** — `console.log('Tags Data', data)` was in `api/tags/+server.ts`. This would log full tag response payloads in production. Good catch. ✅ 3. **`hooks.server.ts` in coverage `include`** — the 80% branch threshold now covers the new `handleError` logic. CI will catch regressions. The two tests cover the `??` branch (Sentry id vs UUID fallback). ✅ 4. **`tracesSampleRate: 0.1`** — appropriate for a shared CX32 VPS. 100% sampling on every transaction would generate 10x the event volume and disk writes on GlitchTip's Redis and PostgreSQL containers. ✅ 5. **`VITE_SENTRY_DSN`** — this is a Vite build-time env var (baked into the client bundle). Not a runtime secret, so it doesn't need to be in the Docker Compose env section — it's injected at build time via the CI/CD pipeline or local `.env`. Convention is consistent with how it's documented in `CLAUDE.md`. ✅ ### No blockers

marcel commented 2026-05-17 11:49:23 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Requirements coverage check

The original issue had three requirements:

#	Requirement	Status
1	Capture frontend errors automatically and route them to GlitchTip	✅ handleErrorWithSentry() in both hooks.client.ts and hooks.server.ts
2	Give users a visible error identifier they can include in a support message	✅ errorId displayed on +error.svelte with copy-to-clipboard affordance
3	No PII leakage from the family archive	✅ sendDefaultPii: false, tracesSampleRate: 0.1, self-hosted GlitchTip

Gap that was filled (not in original spec)

App.Error interface extension — the original spec didn't mention that TypeScript's App.Error type needed to be extended for errorId to be accessible from page.error. The implementation correctly added this in app.d.ts. Without it, page.error?.errorId would be a TypeScript error.

Out-of-scope changes (observation only)

The PR includes three changes unrelated to error tracking:

1. dashboard_resume_label copy change in de/en/es (cosmetic UX improvement)
2. Blank line cleanup throughout messages JSON files (formatting only)
3. Stray console.log removal from api/tags/+server.ts

None of these are harmful. Items 2 and 3 were likely swept in during the no-console ESLint rule addition. Worth noting for changelog clarity, not a blocker.

Acceptance criteria met

• ✅ Errors are captured without requiring user reports
• ✅ Users can copy an errorId to include in a support message
• ✅ The errorId section is hidden when no errorId is present (graceful degradation)
• ✅ Copy failure (clipboard denied) does not break the page — select-all fallback remains
• ✅ SDK is disabled in development (no events to GlitchTip from dev machines)

No blockers

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### Requirements coverage check The original issue had three requirements: | # | Requirement | Status | |---|---|---| | 1 | Capture frontend errors automatically and route them to GlitchTip | ✅ `handleErrorWithSentry()` in both `hooks.client.ts` and `hooks.server.ts` | | 2 | Give users a visible error identifier they can include in a support message | ✅ `errorId` displayed on `+error.svelte` with copy-to-clipboard affordance | | 3 | No PII leakage from the family archive | ✅ `sendDefaultPii: false`, `tracesSampleRate: 0.1`, self-hosted GlitchTip | ### Gap that was filled (not in original spec) **`App.Error` interface extension** — the original spec didn't mention that TypeScript's `App.Error` type needed to be extended for `errorId` to be accessible from `page.error`. The implementation correctly added this in `app.d.ts`. Without it, `page.error?.errorId` would be a TypeScript error. ### Out-of-scope changes (observation only) The PR includes three changes unrelated to error tracking: 1. `dashboard_resume_label` copy change in de/en/es (cosmetic UX improvement) 2. Blank line cleanup throughout messages JSON files (formatting only) 3. Stray `console.log` removal from `api/tags/+server.ts` None of these are harmful. Items 2 and 3 were likely swept in during the `no-console` ESLint rule addition. Worth noting for changelog clarity, not a blocker. ### Acceptance criteria met - ✅ Errors are captured without requiring user reports - ✅ Users can copy an errorId to include in a support message - ✅ The errorId section is hidden when no `errorId` is present (graceful degradation) - ✅ Copy failure (clipboard denied) does not break the page — `select-all` fallback remains - ✅ SDK is disabled in development (no events to GlitchTip from dev machines) ### No blockers

marcel merged commit de19d17b00 into main 2026-05-17 12:38:10 +02:00

marcel deleted branch feat/issue-462-handle-error-hook 2026-05-17 12:38:11 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#608