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

New Issue

marcel · 2026-05-07T17:23:16+02:00

marcel commented

2026-05-07 17:23:16 +02:00

Context

frontend/src/hooks.server.ts exports handle and handleFetch only. There is no handleError hook. As a result, every unhandled SSR error renders SvelteKit's default 500 page and disappears — no error sink, no Sentry, no log aggregation.

Operationally this means:

A user reports "the page is white" — there's no trace, no error ID, no way to correlate to logs.
The transcriber audience (60+, on laptops/tablets per memory) is the most likely to hit edge-case errors and the least likely to be able to describe what they saw.
Combined with the absence of distributed tracing (#140 + audit F-08), production incidents are essentially undebuggable.

Approach

Implement handleError in both hooks.server.ts and hooks.client.ts. Forward to structured stdout for now (Loki picks it up after #140). When a real error sink is chosen (Sentry, GlitchTip, etc.), swap the destination — the contract stays the same.

`frontend/src/hooks.server.ts`

import type { HandleServerError } from '@sveltejs/kit';

export const handleError: HandleServerError = ({ error, event, status, message }) => {
    const errorId = crypto.randomUUID();
    console.error(JSON.stringify({
        ts: new Date().toISOString(),
        level: 'error',
        errorId,
        status,
        message,
        url: event.url.pathname,
        userId: event.locals.user?.id ?? null,
        // serialize error including stack but exclude any message that might contain PII
        error: error instanceof Error ? { name: error.name, message: error.message, stack: error.stack } : String(error),
    }));
    return { message: 'An unexpected error occurred', errorId };
};

The returned object becomes $page.error on the rendered error page — surface errorId to the user so they can quote it in a bug report.

`frontend/src/hooks.client.ts` (new file)

import type { HandleClientError } from '@sveltejs/kit';

export const handleError: HandleClientError = ({ error, event, status, message }) => {
    const errorId = crypto.randomUUID();
    console.error('[client-error]', { errorId, status, message, url: event.url.pathname, error });
    return { message: 'An unexpected error occurred', errorId };
};

`frontend/src/routes/+error.svelte`

Show errorId to the user, with a "copy to clipboard" affordance:

<script>
  import { page } from '$app/state';
</script>
<h1>Es ist etwas schiefgelaufen.</h1>
<p>{$page.error?.message}</p>
{#if $page.error?.errorId}
  <p>Fehler-ID: <code>{$page.error.errorId}</code></p>
{/if}

i18n keys: error_page_title, error_page_id_label (de/en/es).

Add to frontend/eslint.config.js:

'no-console': ['error', { allow: ['warn', 'error'] }]

This catches the stray console.log the audit found at frontend/src/routes/api/tags/+server.ts:2 and prevents new ones (closes audit F-21).

Critical files

frontend/src/hooks.server.ts — add handleError
frontend/src/hooks.client.ts — new file
frontend/src/routes/+error.svelte — surface error ID
frontend/messages/{de,en,es}.json — 2 new keys
frontend/eslint.config.js — no-console rule
frontend/src/routes/api/tags/+server.ts:2 — remove the existing console.log

Verification

npm run dev, intentionally throw inside a +page.server.ts load → server stdout shows JSON line with errorId; user-facing page shows "Es ist etwas schiefgelaufen" + the same errorId.
npm run dev, throw inside an in-browser action → browser console shows [client-error] with same shape; user sees same error UI.
npm run lint fails if a new console.log is added; passes after removing the existing one.
Locale switcher → error UI translates to en/es.

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 (existing one removed).
ESLint rule no-console enforced.
Stdout output is single-line JSON suitable for Loki ingestion.
No PII (passwords, tokens, raw SQL) appears in the stdout payload — verified by grepping a sample crash dump.

Effort

S — half a day.

Risk if not addressed

Production incidents are undebuggable. Unhappy-path bugs accumulate without telemetry to find them.

Tracked in audit doc as F-02 (Critical) + F-21 (Medium, folded into this PR).

## Context `frontend/src/hooks.server.ts` exports `handle` and `handleFetch` only. There is no `handleError` hook. As a result, every unhandled SSR error renders SvelteKit's default 500 page and disappears — no error sink, no Sentry, no log aggregation. Operationally this means: - A user reports "the page is white" — there's no trace, no error ID, no way to correlate to logs. - The transcriber audience (60+, on laptops/tablets per memory) is the most likely to hit edge-case errors and the least likely to be able to describe what they saw. - Combined with the absence of distributed tracing (#140 + audit F-08), production incidents are essentially undebuggable. ## Approach Implement `handleError` in both `hooks.server.ts` and `hooks.client.ts`. Forward to structured stdout for now (Loki picks it up after #140). When a real error sink is chosen (Sentry, GlitchTip, etc.), swap the destination — the contract stays the same. ### `frontend/src/hooks.server.ts` ```typescript import type { HandleServerError } from '@sveltejs/kit'; export const handleError: HandleServerError = ({ error, event, status, message }) => { const errorId = crypto.randomUUID(); console.error(JSON.stringify({ ts: new Date().toISOString(), level: 'error', errorId, status, message, url: event.url.pathname, userId: event.locals.user?.id ?? null, // serialize error including stack but exclude any message that might contain PII error: error instanceof Error ? { name: error.name, message: error.message, stack: error.stack } : String(error), })); return { message: 'An unexpected error occurred', errorId }; }; ``` The returned object becomes `$page.error` on the rendered error page — surface `errorId` to the user so they can quote it in a bug report. ### `frontend/src/hooks.client.ts` (new file) ```typescript import type { HandleClientError } from '@sveltejs/kit'; export const handleError: HandleClientError = ({ error, event, status, message }) => { const errorId = crypto.randomUUID(); console.error('[client-error]', { errorId, status, message, url: event.url.pathname, error }); return { message: 'An unexpected error occurred', errorId }; }; ``` ### `frontend/src/routes/+error.svelte` Show `errorId` to the user, with a "copy to clipboard" affordance: ```svelte <script> import { page } from '$app/state'; </script> <h1>Es ist etwas schiefgelaufen.</h1> <p>{$page.error?.message}</p> {#if $page.error?.errorId} <p>Fehler-ID: <code>{$page.error.errorId}</code></p> {/if} ``` i18n keys: `error_page_title`, `error_page_id_label` (de/en/es). ### Lint-time enforcement of related cleanup Add to `frontend/eslint.config.js`: ```javascript 'no-console': ['error', { allow: ['warn', 'error'] }] ``` This catches the stray `console.log` the audit found at `frontend/src/routes/api/tags/+server.ts:2` and prevents new ones (closes audit F-21). ## Critical files - `frontend/src/hooks.server.ts` — add `handleError` - `frontend/src/hooks.client.ts` — **new** file - `frontend/src/routes/+error.svelte` — surface error ID - `frontend/messages/{de,en,es}.json` — 2 new keys - `frontend/eslint.config.js` — `no-console` rule - `frontend/src/routes/api/tags/+server.ts:2` — remove the existing `console.log` ## Verification 1. `npm run dev`, intentionally throw inside a `+page.server.ts` load → server stdout shows JSON line with `errorId`; user-facing page shows "Es ist etwas schiefgelaufen" + the same `errorId`. 2. `npm run dev`, throw inside an in-browser action → browser console shows `[client-error]` with same shape; user sees same error UI. 3. `npm run lint` fails if a new `console.log` is added; passes after removing the existing one. 4. Locale switcher → error UI translates to en/es. ## 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` (existing one removed). - [ ] ESLint rule `no-console` enforced. - [ ] Stdout output is single-line JSON suitable for Loki ingestion. - [ ] No PII (passwords, tokens, raw SQL) appears in the stdout payload — verified by grepping a sample crash dump. ## Effort S — half a day. ## Risk if not addressed Production incidents are undebuggable. Unhappy-path bugs accumulate without telemetry to find them. Tracked in audit doc as **F-02** (Critical) + **F-21** (Medium, folded into this PR).

marcel added this to the Production v1 milestone 2026-05-07 17:23:16 +02:00

marcel added the P0-critical feature labels 2026-05-07 17:23:35 +02:00

marcel commented

2026-05-07 17:36:48 +02:00

👨‍💻 Markus Keller — Application Architect

Observations

The issue correctly identifies a gap in the observability layer: no handleError hook means every unhandled SSR exception is a silent production black hole. This is a straightforward, well-scoped fix.
The proposed JSON stdout format is the right approach for the current stack. Loki ingests Docker stdout via promtail with no configuration change — the JSON shape just needs to stay consistent.
Returning { message, errorId } from handleError is architecturally sound: it extends App.Error cleanly. The issue does not mention that App.Error must be augmented in src/app.d.ts to declare the errorId field — without this TypeScript will complain when +error.svelte reads page.error?.errorId. Add:
```
// src/app.d.ts
interface Error {
  message: string;
  errorId?: string;
}
```
The no-console ESLint rule addition is the right lint-time enforcement mechanism. However, the existing hooks.server.ts already uses console.error(...) in the userGroup handler (line 56). That call is legitimate — it should survive the new rule because no-console allows error and warn by default in the proposed config. Confirm this carefully during implementation.
Doc update checklist (from the architect table): this PR adds a new hook file (hooks.client.ts) and modifies the error page. Neither touches a backend domain, Flyway migration, or Docker service — no C4 or DB diagram update needed. However, if the team adds a future Sentry integration, that's a new external system requiring l1-context.puml update. The issue correctly calls this out as a future swap point.
The issue references audit items F-02 and F-21. If there is a docs/audits/ file tracking these, that file should be updated to mark them as addressed once the PR merges. I see docs/audits/2026-05-07-pre-prod-architectural-review.md in the git status — the implementing PR should include a pass over that file to close the relevant findings.

Recommendations

Add App.Error augmentation to src/app.d.ts as the very first step — it is the TypeScript contract everything else depends on.
Use crypto.randomUUID() on the server (Node 21, available natively). On the client, crypto.randomUUID() is also available in all modern browsers — no polyfill needed. The issue's code is correct; just make this explicit in the PR description.
Keep the stdout payload lean: ts, level, errorId, status, url, userId, error.name, error.message, error.stack. Do not log request headers, query parameters, or cookie values in the initial implementation — they may contain tokens. The issue's PII note is good; make it a concrete exclusion list in the implementation.
Mark the console.log in api/tags/+server.ts:27 for removal in the same PR — the issue already calls this out as F-21. Bundle it; don't leave it for a follow-up.
Do not create a separate ErrorService or abstraction layer. The two hook files are the right abstraction level for this scale. Keep it boring.

Open Decisions

Client-side errorId sourcing: the client hook generates its own UUID independently. This means a client-side error and a server-side re-render of the error page will have different IDs. For correlating a client crash report to a server log, the IDs will never match. Whether this matters depends on whether client errors are ever expected to have a server-side counterpart — for this app's SSR-first architecture, probably not. No action needed now, but worth keeping in mind when Sentry is added.

## 👨‍💻 Markus Keller — Application Architect ### Observations - The issue correctly identifies a gap in the observability layer: no `handleError` hook means every unhandled SSR exception is a silent production black hole. This is a straightforward, well-scoped fix. - The proposed JSON stdout format is the right approach for the current stack. Loki ingests Docker stdout via promtail with no configuration change — the JSON shape just needs to stay consistent. - Returning `{ message, errorId }` from `handleError` is architecturally sound: it extends `App.Error` cleanly. The issue **does not mention** that `App.Error` must be augmented in `src/app.d.ts` to declare the `errorId` field — without this TypeScript will complain when `+error.svelte` reads `page.error?.errorId`. Add: ```ts // src/app.d.ts interface Error { message: string; errorId?: string; } ``` - The `no-console` ESLint rule addition is the right lint-time enforcement mechanism. However, the existing `hooks.server.ts` already uses `console.error(...)` in the `userGroup` handler (line 56). That call is legitimate — it should survive the new rule because `no-console` allows `error` and `warn` by default in the proposed config. Confirm this carefully during implementation. - **Doc update checklist (from the architect table):** this PR adds a new hook file (`hooks.client.ts`) and modifies the error page. Neither touches a backend domain, Flyway migration, or Docker service — no C4 or DB diagram update needed. However, if the team adds a future Sentry integration, that's a new external system requiring `l1-context.puml` update. The issue correctly calls this out as a future swap point. - The issue references audit items F-02 and F-21. If there is a `docs/audits/` file tracking these, that file should be updated to mark them as addressed once the PR merges. I see `docs/audits/2026-05-07-pre-prod-architectural-review.md` in the git status — the implementing PR should include a pass over that file to close the relevant findings. ### Recommendations 1. Add `App.Error` augmentation to `src/app.d.ts` as the very first step — it is the TypeScript contract everything else depends on. 2. Use `crypto.randomUUID()` on the server (Node 21, available natively). On the client, `crypto.randomUUID()` is also available in all modern browsers — no polyfill needed. The issue's code is correct; just make this explicit in the PR description. 3. Keep the stdout payload lean: `ts`, `level`, `errorId`, `status`, `url`, `userId`, `error.name`, `error.message`, `error.stack`. Do **not** log request headers, query parameters, or cookie values in the initial implementation — they may contain tokens. The issue's PII note is good; make it a concrete exclusion list in the implementation. 4. Mark the `console.log` in `api/tags/+server.ts:27` for removal in the same PR — the issue already calls this out as F-21. Bundle it; don't leave it for a follow-up. 5. Do not create a separate `ErrorService` or abstraction layer. The two hook files are the right abstraction level for this scale. Keep it boring. ### Open Decisions - **Client-side `errorId` sourcing:** the client hook generates its own UUID independently. This means a client-side error and a server-side re-render of the error page will have different IDs. For correlating a client crash report to a server log, the IDs will never match. Whether this matters depends on whether client errors are ever expected to have a server-side counterpart — for this app's SSR-first architecture, probably not. No action needed now, but worth keeping in mind when Sentry is added.

marcel commented

2026-05-07 17:37:08 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Observations

The current +error.svelte (14 lines) displays page.status and page.error?.message but has no errorId surface at all — this is the primary gap the issue addresses.
hooks.client.ts does not exist. hooks.server.ts exports handle and handleFetch only — handleError is missing. Both are confirmed by reading the file.
The stray console.log('Tags Data', data) at frontend/src/routes/api/tags/+server.ts:27 is confirmed. It is the only console.log in frontend/src/ — easy to remove.
The issue's proposed handleError implementations are clean and correct. One naming note: the returned object uses message (matches SvelteKit's default App.Error.message field) and errorId (new field). This is consistent with the project's error-handling idiom.
The +error.svelte snippet in the issue uses $page.error?.errorId — this reads fine in Svelte 5 with $app/state's page rune (the existing file already uses import { page } from '$app/state'). No store migration needed.
The "copy to clipboard" affordance mentioned in the issue is not fully specified. The snippet shows the errorId in a <code> block but no button. This should be implemented as a small <button> that calls navigator.clipboard.writeText(page.error.errorId) — consistent with how other copy actions in the codebase work.

Recommendations

TDD order for this feature:
- Write a Vitest test for the server load function in a test page that verifies a thrown error produces a response with errorId in $page.error — or at minimum write a unit test that the handleError function returns { message, errorId } where errorId matches a UUID regex.
- For the ESLint rule: add a fixture file with a console.log statement, verify npm run lint fails, then add the rule and verify it passes.
Component split in +error.svelte: the issue's snippet is minimal (good). Don't create a child component — at 15-20 lines it doesn't need splitting. Keep the copy button inline.
The handleError on the server should serialize error.stack only when status >= 500. For 4xx errors (which can also reach the error hook via error() from load functions), stack traces add noise and may expose path info. Add a conditional:
```
stack: status >= 500 && error instanceof Error ? error.stack : undefined
```
Remove the console.log in api/tags/+server.ts in the same commit as the ESLint rule — otherwise the rule will immediately fail CI on the existing codebase. The issue already groups these; enforce the commit order.
The i18n keys error_page_title and error_page_id_label should be added to all three locale files in the same commit. The existing page_title_error key is already present — error_page_title would be a duplicate concept. Reuse the existing key for the title; only add error_page_id_label as a new key.

Open Decisions

Stack trace in client hook: the browser console already shows the stack, so logging it to console.error in hooks.client.ts is redundant and potentially verbose. Omit error.stack from the client hook payload — the browser DevTools already surface it. This is a recommendation, not a blocker.

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations - The current `+error.svelte` (14 lines) displays `page.status` and `page.error?.message` but has no `errorId` surface at all — this is the primary gap the issue addresses. - `hooks.client.ts` does not exist. `hooks.server.ts` exports `handle` and `handleFetch` only — `handleError` is missing. Both are confirmed by reading the file. - The stray `console.log('Tags Data', data)` at `frontend/src/routes/api/tags/+server.ts:27` is confirmed. It is the only `console.log` in `frontend/src/` — easy to remove. - The issue's proposed `handleError` implementations are clean and correct. One naming note: the returned object uses `message` (matches SvelteKit's default `App.Error.message` field) and `errorId` (new field). This is consistent with the project's error-handling idiom. - The `+error.svelte` snippet in the issue uses `$page.error?.errorId` — this reads fine in Svelte 5 with `$app/state`'s `page` rune (the existing file already uses `import { page } from '$app/state'`). No store migration needed. - The "copy to clipboard" affordance mentioned in the issue is not fully specified. The snippet shows the `errorId` in a `<code>` block but no button. This should be implemented as a small `<button>` that calls `navigator.clipboard.writeText(page.error.errorId)` — consistent with how other copy actions in the codebase work. ### Recommendations 1. **TDD order for this feature:** - Write a Vitest test for the server `load` function in a test page that verifies a thrown error produces a response with `errorId` in `$page.error` — or at minimum write a unit test that the `handleError` function returns `{ message, errorId }` where `errorId` matches a UUID regex. - For the ESLint rule: add a fixture file with a `console.log` statement, verify `npm run lint` fails, then add the rule and verify it passes. 2. **Component split in `+error.svelte`:** the issue's snippet is minimal (good). Don't create a child component — at 15-20 lines it doesn't need splitting. Keep the copy button inline. 3. The `handleError` on the server should serialize `error.stack` only when `status >= 500`. For 4xx errors (which can also reach the error hook via `error()` from load functions), stack traces add noise and may expose path info. Add a conditional: ```ts stack: status >= 500 && error instanceof Error ? error.stack : undefined ``` 4. **Remove the `console.log` in `api/tags/+server.ts` in the same commit as the ESLint rule** — otherwise the rule will immediately fail CI on the existing codebase. The issue already groups these; enforce the commit order. 5. The i18n keys `error_page_title` and `error_page_id_label` should be added to all three locale files in the same commit. The existing `page_title_error` key is already present — `error_page_title` would be a duplicate concept. Reuse the existing key for the title; only add `error_page_id_label` as a new key. ### Open Decisions - **Stack trace in client hook:** the browser console already shows the stack, so logging it to `console.error` in `hooks.client.ts` is redundant and potentially verbose. Omit `error.stack` from the client hook payload — the browser DevTools already surface it. This is a recommendation, not a blocker.

marcel commented

2026-05-07 17:37:28 +02:00

👨‍💻 Tobias Wendt — DevOps & Platform Engineer

Observations

The issue's stdout-first approach is exactly right for this stack. The Docker Compose setup means every console.error(JSON.stringify({...})) line lands in docker logs frontend and is automatically ingested by Loki via promtail with zero configuration change. No additional infrastructure needed for phase 1.
The JSON shape the issue proposes (ts, level, errorId, status, url, userId, error) is clean. The ts field uses ISO 8601, which Loki's log pipeline can parse natively for time ordering.
The docker-compose.yml is listed as modified in git status. I have not seen the diff, but if the frontend service definition changes there, verify the Node adapter's startup command still uses stdout (not a log file) — @sveltejs/adapter-node does by default, so no change expected.
The issue references a future Sentry/GlitchTip swap. GlitchTip is already in the self-hosted catalogue (docs/infrastructure/self-hosted-catalogue.md) — it's the natural fit when a real error sink is needed, and it runs on the VPS at zero additional cost. The code contract (return { message, errorId } from the hook, swap the destination) is the right abstraction.
The ESLint no-console rule addition (no-console: ['error', { allow: ['warn', 'error'] }]) is a CI-time control — it gates every PR. This is the correct enforcement layer.

Recommendations

Log format consistency: the server hook should output a single-line JSON string (as the issue specifies). Confirm the JSON.stringify(...) call has no line breaks in the error object — a multi-line stack trace will break Loki's line-based ingestion. Truncate or replace newlines in the stack:
```
stack: error instanceof Error ? error.stack?.replace(/\n/g, '\\n') : undefined
```
Loki label suggestion: when promtail is configured (issue #140 context), add a source="frontend-ssr" label in the log line so Loki queries can filter frontend errors from backend logs in a single dashboard panel. Add this field to the JSON payload now — it costs nothing and saves a schema migration later.
No new Docker service required for this issue — confirm by ensuring no changes to docker-compose.yml are part of this PR (beyond what was already modified). The stdout sink is handled by the existing container runtime.
CI check: after the ESLint rule is added, the CI run for this branch must pass npm run lint green. The only pre-existing console.log is at api/tags/+server.ts:27 — remove it in the same PR. If npm run lint was not part of CI before, add it now as a step before the build step.

Open Decisions

When to add GlitchTip: the issue deliberately defers the real error sink. The right trigger is "first production incident where the errorId in the log cannot be found fast enough." At that point, stand up GlitchTip from the self-hosted catalogue, add the Sentry SDK (GlitchTip is Sentry-compatible), and swap the hook body. The current stdout approach is good enough until then.

## 👨‍💻 Tobias Wendt — DevOps & Platform Engineer ### Observations - The issue's stdout-first approach is exactly right for this stack. The Docker Compose setup means every `console.error(JSON.stringify({...}))` line lands in `docker logs frontend` and is automatically ingested by Loki via promtail with zero configuration change. No additional infrastructure needed for phase 1. - The JSON shape the issue proposes (`ts`, `level`, `errorId`, `status`, `url`, `userId`, `error`) is clean. The `ts` field uses ISO 8601, which Loki's log pipeline can parse natively for time ordering. - The `docker-compose.yml` is listed as modified in `git status`. I have not seen the diff, but if the frontend service definition changes there, verify the Node adapter's startup command still uses stdout (not a log file) — `@sveltejs/adapter-node` does by default, so no change expected. - The issue references a future Sentry/GlitchTip swap. GlitchTip is already in the self-hosted catalogue (`docs/infrastructure/self-hosted-catalogue.md`) — it's the natural fit when a real error sink is needed, and it runs on the VPS at zero additional cost. The code contract (return `{ message, errorId }` from the hook, swap the destination) is the right abstraction. - The ESLint `no-console` rule addition (`no-console: ['error', { allow: ['warn', 'error'] }]`) is a CI-time control — it gates every PR. This is the correct enforcement layer. ### Recommendations 1. **Log format consistency:** the server hook should output a single-line JSON string (as the issue specifies). Confirm the `JSON.stringify(...)` call has no line breaks in the error object — a multi-line stack trace will break Loki's line-based ingestion. Truncate or replace newlines in the stack: ```ts stack: error instanceof Error ? error.stack?.replace(/\n/g, '\\n') : undefined ``` 2. **Loki label suggestion:** when promtail is configured (issue #140 context), add a `source="frontend-ssr"` label in the log line so Loki queries can filter frontend errors from backend logs in a single dashboard panel. Add this field to the JSON payload now — it costs nothing and saves a schema migration later. 3. **No new Docker service required** for this issue — confirm by ensuring no changes to `docker-compose.yml` are part of this PR (beyond what was already modified). The stdout sink is handled by the existing container runtime. 4. **CI check:** after the ESLint rule is added, the CI run for this branch must pass `npm run lint` green. The only pre-existing `console.log` is at `api/tags/+server.ts:27` — remove it in the same PR. If `npm run lint` was not part of CI before, add it now as a step before the build step. ### Open Decisions - **When to add GlitchTip:** the issue deliberately defers the real error sink. The right trigger is "first production incident where the `errorId` in the log cannot be found fast enough." At that point, stand up GlitchTip from the self-hosted catalogue, add the Sentry SDK (GlitchTip is Sentry-compatible), and swap the hook body. The current stdout approach is good enough until then.

marcel commented

2026-05-07 17:37:47 +02:00

👨‍💻 Elicit — Requirements Engineer

Observations

The issue is exceptionally well-specified for a P0 observability fix: it has context, approach, code snippets, critical files, verification steps, and acceptance criteria. It passes the Definition of Ready checklist on all counts.
The acceptance criteria are clear and testable, though two are worth tightening:
- "No PII appears in the stdout payload — verified by grepping a sample crash dump" is a manual verification step, not an automated criterion. For a solo developer this is pragmatic, but it should be an automated check if possible (see Recommendations).
- "Stdout output is single-line JSON suitable for Loki ingestion" — this is testable: JSON.stringify(...) always produces single-line output. The acceptance criterion should clarify that stack traces with embedded newlines must be escaped (see DevOps note).
The issue folds two audit findings (F-02 and F-21) into one PR. This is a sensible bundling — they share the same file (hooks.server.ts context) and the same enforcement mechanism (no-console rule). No scope creep concern.
The effort estimate of "S — half a day" is accurate for a developer familiar with the codebase. The App.Error augmentation (missing from the spec) adds ~15 minutes.
The issue mentions "Locale switcher → error UI translates to en/es" as a verification step. This implies the two new i18n keys (error_page_title, error_page_id_label) are needed in all three locale files. The issue lists this in Critical files but the acceptance criteria do not include a localization check. Minor gap.

Recommendations

Add one acceptance criterion: "The App.Error interface in src/app.d.ts declares { message: string; errorId?: string } and svelte-check passes." This catches the TypeScript contract gap before it reaches runtime.
Tighten the PII criterion: restate as "The server hook does not log request headers, query parameters, cookie values, or form body content." This is auditable in code review without requiring a crash dump.
Reuse the existing page_title_error i18n key for the error page title — it already exists in all three locales. Only error_page_id_label is a net-new key. This reduces the i18n work to 3 lines (one per locale).
Add a non-functional requirement for the error page: the errorId must be copyable with a single interaction (button or click-to-select). The issue mentions "copy to clipboard" but the snippet only shows a <code> block. Specify the exact interaction so the implementer doesn't ship a non-copyable code block.
The verification step "intentionally throw inside a +page.server.ts load" is good manual QA. For a P0 item, consider whether a Playwright smoke test that triggers a 500 and verifies the error page renders an errorId should be part of the acceptance criteria.

## 👨‍💻 Elicit — Requirements Engineer ### Observations - The issue is exceptionally well-specified for a P0 observability fix: it has context, approach, code snippets, critical files, verification steps, and acceptance criteria. It passes the Definition of Ready checklist on all counts. - The acceptance criteria are clear and testable, though two are worth tightening: - "No PII appears in the stdout payload — verified by grepping a sample crash dump" is a manual verification step, not an automated criterion. For a solo developer this is pragmatic, but it should be an automated check if possible (see Recommendations). - "Stdout output is single-line JSON suitable for Loki ingestion" — this is testable: `JSON.stringify(...)` always produces single-line output. The acceptance criterion should clarify that stack traces with embedded newlines must be escaped (see DevOps note). - The issue folds two audit findings (F-02 and F-21) into one PR. This is a sensible bundling — they share the same file (`hooks.server.ts` context) and the same enforcement mechanism (`no-console` rule). No scope creep concern. - The effort estimate of "S — half a day" is accurate for a developer familiar with the codebase. The `App.Error` augmentation (missing from the spec) adds ~15 minutes. - The issue mentions "Locale switcher → error UI translates to en/es" as a verification step. This implies the two new i18n keys (`error_page_title`, `error_page_id_label`) are needed in all three locale files. The issue lists this in Critical files but the acceptance criteria do not include a localization check. Minor gap. ### Recommendations 1. **Add one acceptance criterion:** "The `App.Error` interface in `src/app.d.ts` declares `{ message: string; errorId?: string }` and `svelte-check` passes." This catches the TypeScript contract gap before it reaches runtime. 2. **Tighten the PII criterion:** restate as "The server hook does not log request headers, query parameters, cookie values, or form body content." This is auditable in code review without requiring a crash dump. 3. **Reuse the existing `page_title_error` i18n key** for the error page title — it already exists in all three locales. Only `error_page_id_label` is a net-new key. This reduces the i18n work to 3 lines (one per locale). 4. **Add a non-functional requirement for the error page:** the `errorId` must be copyable with a single interaction (button or click-to-select). The issue mentions "copy to clipboard" but the snippet only shows a `<code>` block. Specify the exact interaction so the implementer doesn't ship a non-copyable code block. 5. The verification step "intentionally throw inside a `+page.server.ts` load" is good manual QA. For a P0 item, consider whether a Playwright smoke test that triggers a 500 and verifies the error page renders an `errorId` should be part of the acceptance criteria.

marcel commented

2026-05-07 17:38:10 +02:00

👨‍💻 Nora "NullX" Steiner — Security Engineer

Observations

The PII concern flagged in the issue is the right instinct. The proposed server hook logs error.message — in the Familienarchiv context, a DomainException message may include entity IDs ("Document not found: <UUID>") or usernames ("Unknown user: admin@family.local"). UUIDs are low-risk; usernames are borderline. The current approach is acceptable but deserves explicit attention.
Stack trace leakage vector: error.stack in the stdout payload can include file system paths from the Node.js process (e.g., /home/marcel/Desktop/familienarchiv/frontend/.svelte-kit/...). In the dev environment this is fine; in production, these paths could aid an attacker in understanding the server's directory layout if logs are ever misconfigured to be publicly accessible. Recommendation: strip or hash path components in production.
The errorId returned to the user in $page.error is a UUID — this is safe. UUIDs reveal nothing about the system. Good design.
Client-side hook: the browser console log in hooks.client.ts is visible to any user who opens DevTools. { errorId, status, message, url, error } is acceptable — the error object in the browser will include stack traces from the client bundle. Since the production bundle is minified, path leakage is minimal. No concern here.
The no-console ESLint rule targets console.log (disallowed) while allowing console.warn and console.error. The existing console.error(...) call in hooks.server.ts line 56 (userGroup handler) and the new handleError body both use console.error — these survive the rule correctly.
Log injection / log forging concern (CWE-117): the url: event.url.pathname field in the server log is user-controlled. A crafted URL could inject JSON characters (", \n, {, }) into the log line, potentially breaking Loki's parser or injecting false log entries. JSON.stringify({...}) at the top level handles this correctly because the entire object is stringified — the pathname is a JSON string value and its special characters are escaped automatically. This is safe as written.
The issue correctly notes "no PII (passwords, tokens, raw SQL)" — the hook as specified does not log form bodies, request headers, or cookies. Verify during implementation that event.request.body is never read or logged in the hook.

Recommendations

Add a production guard on error.stack: log the full stack in development, truncate to the first 3 lines in production. This is a one-liner:

const stack = error instanceof Error ? error.stack : undefined;
const safeStack = process.env.NODE_ENV === 'production'
  ? stack?.split('\n').slice(0, 3).join('\n')
  : stack;

This is a hardening recommendation, not a blocker for merging.

Exclude error.message from the client hook (or document explicitly that it may be user-visible). In the browser, console.error output is visible to the end user in DevTools. For this family app with trusted users, this is not a meaningful threat, but it establishes a good habit for any future public-facing derivative.
Write a security regression test: add a test (Vitest, server environment) that calls a mock handleError with a crafted event.url.pathname containing "}\n{"level":"admin" and verifies the output is a valid single-line JSON object. This proves the log injection path is closed.

Open Decisions

userId in the server log: the issue logs event.locals.user?.id ?? null. User IDs (UUIDs) are pseudonymous — they identify an account without revealing a name. For a private family app this is appropriate and useful for incident correlation. For any future GDPR-scope deployment, user IDs in logs are personal data requiring a retention policy. Not a blocker now; flag for the production hardening checklist.

## 👨‍💻 Nora "NullX" Steiner — Security Engineer ### Observations - The PII concern flagged in the issue is the right instinct. The proposed server hook logs `error.message` — in the Familienarchiv context, a `DomainException` message may include entity IDs (`"Document not found: <UUID>"`) or usernames (`"Unknown user: admin@family.local"`). UUIDs are low-risk; usernames are borderline. The current approach is acceptable but deserves explicit attention. - **Stack trace leakage vector:** `error.stack` in the stdout payload can include file system paths from the Node.js process (e.g., `/home/marcel/Desktop/familienarchiv/frontend/.svelte-kit/...`). In the dev environment this is fine; in production, these paths could aid an attacker in understanding the server's directory layout if logs are ever misconfigured to be publicly accessible. Recommendation: strip or hash path components in production. - The `errorId` returned to the user in `$page.error` is a UUID — this is safe. UUIDs reveal nothing about the system. Good design. - **Client-side hook:** the browser console log in `hooks.client.ts` is visible to any user who opens DevTools. `{ errorId, status, message, url, error }` is acceptable — the `error` object in the browser will include stack traces from the client bundle. Since the production bundle is minified, path leakage is minimal. No concern here. - The `no-console` ESLint rule targets `console.log` (disallowed) while allowing `console.warn` and `console.error`. The existing `console.error(...)` call in `hooks.server.ts` line 56 (userGroup handler) and the new `handleError` body both use `console.error` — these survive the rule correctly. - **Log injection / log forging concern (CWE-117):** the `url: event.url.pathname` field in the server log is user-controlled. A crafted URL could inject JSON characters (`"`, `\n`, `{`, `}`) into the log line, potentially breaking Loki's parser or injecting false log entries. `JSON.stringify({...})` at the top level handles this correctly because the entire object is stringified — the pathname is a JSON string value and its special characters are escaped automatically. This is safe as written. - The issue correctly notes "no PII (passwords, tokens, raw SQL)" — the hook as specified does not log form bodies, request headers, or cookies. Verify during implementation that `event.request.body` is never read or logged in the hook. ### Recommendations 1. **Add a production guard on `error.stack`:** log the full stack in development, truncate to the first 3 lines in production. This is a one-liner: ```ts const stack = error instanceof Error ? error.stack : undefined; const safeStack = process.env.NODE_ENV === 'production' ? stack?.split('\n').slice(0, 3).join('\n') : stack; ``` This is a hardening recommendation, not a blocker for merging. 2. **Exclude `error.message` from the client hook** (or document explicitly that it may be user-visible). In the browser, `console.error` output is visible to the end user in DevTools. For this family app with trusted users, this is not a meaningful threat, but it establishes a good habit for any future public-facing derivative. 3. **Write a security regression test:** add a test (Vitest, server environment) that calls a mock `handleError` with a crafted `event.url.pathname` containing `"}\n{"level":"admin"` and verifies the output is a valid single-line JSON object. This proves the log injection path is closed. ### Open Decisions - **`userId` in the server log:** the issue logs `event.locals.user?.id ?? null`. User IDs (UUIDs) are pseudonymous — they identify an account without revealing a name. For a private family app this is appropriate and useful for incident correlation. For any future GDPR-scope deployment, user IDs in logs are personal data requiring a retention policy. Not a blocker now; flag for the production hardening checklist.

marcel commented

2026-05-07 17:38:30 +02:00

👨‍💻 Sara Holt — QA Engineer

Observations

The issue's verification section describes manual testing steps only. For a P0 observability feature, this is the minimum — but the test coverage question for the hook itself deserves attention.
handleError is a plain TypeScript function that takes { error, event, status, message } and returns { message: string, errorId: string }. It is unit-testable without any SvelteKit runtime. A Vitest test can import it directly, call it with a mock event, and assert on the returned shape and the stdout output.
The existing test suite uses vitest-browser-svelte for component tests and Vitest (Node environment) for server-side logic. The handleError function fits naturally into the Node environment project.
The +error.svelte change (adding the errorId display and copy button) should have a vitest-browser-svelte component test that:
- Renders the component with a mocked page.error including errorId
- Asserts the errorId text is visible
- Asserts the copy button is present and labeled correctly
The ESLint rule change (no-console) is tested implicitly by npm run lint in CI. No separate unit test needed — but CI must run lint as a gate.
The console.log removal at api/tags/+server.ts:27 has an existing unit test implication: if there were tests for this route's handler, they would be unaffected by this removal. The console.log is a debug artifact, not load-bearing behavior.

Recommendations

Write a unit test for handleError (server):

// hooks.server.test.ts
it('handleError returns a UUID errorId', () => {
  const result = handleError({
    error: new Error('boom'),
    event: { url: { pathname: '/documents/123' }, locals: { user: null } } as any,
    status: 500,
    message: 'Internal Error'
  });
  expect(result.errorId).toMatch(/^[0-9a-f-]{36}$/);
  expect(result.message).toBe('An unexpected error occurred');
});

Write a component test for +error.svelte: use vitest-browser-svelte to render it with page.error = { message: 'test error', errorId: 'abc-123' } mocked via a store override, and assert getByText('abc-123') is visible.
Add a Playwright E2E test that navigates to a deliberate 500 route (or mocks one) and asserts:
- The error page renders with a heading
- An errorId element is present
- The page title is localized (not "Internal Error")
  This covers the full SSR error path that unit tests cannot reach.
Lint gate in CI: confirm npm run lint is a CI step that runs before the build. If it is not currently in the Gitea Actions workflow, add it as part of this PR.
Test the no-console rule itself: add a fixture file (under src/lib/**/__fixtures__/) with a console.log call and verify npm run lint reports an error on it. The existing eslint.config.js already excludes __fixtures__ from the regular lint pass — this makes it a safe test location.

Open Decisions

Coverage gate for hooks files: hooks.server.ts and hooks.client.ts are currently not covered by the Vitest coverage report (if one exists). Whether to include them in the coverage threshold or keep them as manually-verified integration points is a team decision. Given the P0 severity, recommend including them in coverage.

## 👨‍💻 Sara Holt — QA Engineer ### Observations - The issue's verification section describes manual testing steps only. For a P0 observability feature, this is the minimum — but the test coverage question for the hook itself deserves attention. - `handleError` is a plain TypeScript function that takes `{ error, event, status, message }` and returns `{ message: string, errorId: string }`. It is unit-testable without any SvelteKit runtime. A Vitest test can import it directly, call it with a mock event, and assert on the returned shape and the stdout output. - The existing test suite uses `vitest-browser-svelte` for component tests and Vitest (Node environment) for server-side logic. The `handleError` function fits naturally into the Node environment project. - The `+error.svelte` change (adding the `errorId` display and copy button) should have a `vitest-browser-svelte` component test that: - Renders the component with a mocked `page.error` including `errorId` - Asserts the `errorId` text is visible - Asserts the copy button is present and labeled correctly - The ESLint rule change (`no-console`) is tested implicitly by `npm run lint` in CI. No separate unit test needed — but CI must run lint as a gate. - The `console.log` removal at `api/tags/+server.ts:27` has an existing unit test implication: if there were tests for this route's handler, they would be unaffected by this removal. The `console.log` is a debug artifact, not load-bearing behavior. ### Recommendations 1. **Write a unit test for `handleError` (server):** ```typescript // hooks.server.test.ts it('handleError returns a UUID errorId', () => { const result = handleError({ error: new Error('boom'), event: { url: { pathname: '/documents/123' }, locals: { user: null } } as any, status: 500, message: 'Internal Error' }); expect(result.errorId).toMatch(/^[0-9a-f-]{36}$/); expect(result.message).toBe('An unexpected error occurred'); }); ``` 2. **Write a component test for `+error.svelte`:** use `vitest-browser-svelte` to render it with `page.error = { message: 'test error', errorId: 'abc-123' }` mocked via a store override, and assert `getByText('abc-123')` is visible. 3. **Add a Playwright E2E test** that navigates to a deliberate 500 route (or mocks one) and asserts: - The error page renders with a heading - An `errorId` element is present - The page title is localized (not "Internal Error") This covers the full SSR error path that unit tests cannot reach. 4. **Lint gate in CI:** confirm `npm run lint` is a CI step that runs before the build. If it is not currently in the Gitea Actions workflow, add it as part of this PR. 5. **Test the `no-console` rule itself:** add a fixture file (under `src/lib/**/__fixtures__/`) with a `console.log` call and verify `npm run lint` reports an error on it. The existing `eslint.config.js` already excludes `__fixtures__` from the regular lint pass — this makes it a safe test location. ### Open Decisions - **Coverage gate for hooks files:** `hooks.server.ts` and `hooks.client.ts` are currently not covered by the Vitest coverage report (if one exists). Whether to include them in the coverage threshold or keep them as manually-verified integration points is a team decision. Given the P0 severity, recommend including them in coverage.

marcel commented

2026-05-07 17:38:56 +02:00

👨‍💻 Leonie Voss — UI/UX Design Lead

Observations

The current +error.svelte (14 lines) is minimal but functional: it shows page.status (large, bold, text-6xl) and page.error?.message (small, text-ink-2). It uses the correct font-sans and text-ink / text-ink-2 tokens. Brand compliance is already correct.
What's missing: the errorId display and the copy button. The issue's snippet shows the ID in a <code> block but does not specify the copy interaction, touch target size, or focus state. For the transcriber audience (60+, laptop/tablet), this matters.
The error page has no localized heading. The page.status number (500, 404) is not meaningful to a 65-year-old transcriber. A human-readable headline in German ("Es ist etwas schiefgelaufen" as the issue proposes) is essential.
The <code> element for errorId will render in the browser's monospace fallback font, which may have poor contrast depending on the system font. The project uses Tinos (serif) and Montserrat (sans) — neither is applied to <code> by default.
The copy button needs: visible label or icon, accessible aria-label, minimum 44px touch target, and a confirmation state ("Kopiert!" for 2 seconds) so seniors know the action succeeded.

Recommendations

Add a human-readable headline before the status code. The issue proposes "Es ist etwas schiefgelaufen" — use the error_page_title i18n key already present in all three locales (check the key name matches). Place it as an <h1> with font-serif text-2xl text-ink.

Style the errorId display to brand standards:

{#if page.error?.errorId}
  <div class="mt-4 flex flex-col items-center gap-2">
    <p class="text-xs font-sans text-ink-2 uppercase tracking-widest">
      {m.error_page_id_label()}
    </p>
    <code class="font-mono text-sm bg-surface border border-line rounded px-3 py-1 text-ink select-all">
      {page.error.errorId}
    </code>
    <button
      class="mt-1 min-h-[44px] px-4 py-2 text-sm font-sans bg-brand-navy text-white rounded-sm
             hover:opacity-90 focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2"
      onclick={() => navigator.clipboard.writeText(page.error!.errorId!)}
      aria-label={m.error_copy_id_label()}
    >
      {m.error_copy_id_label()}
    </button>
  </div>
{/if}

The select-all class on the <code> element means users can also triple-click to select the ID without using the button — essential fallback for seniors who may not trust or find the button.

Add a confirmation state to the copy button: a 2-second "Kopiert!" feedback (swap the button label using a $state boolean) prevents seniors from pressing the button multiple times uncertain whether it worked.
Check the error page at 320px: at small viewport widths, text-6xl for the status code (96px) will dominate the screen. Consider reducing to text-4xl or text-5xl on mobile, scaling up on desktop:
```
<p class="font-sans text-4xl sm:text-6xl font-bold text-ink">{page.status}</p>
```
Verify the existing page.error?.message display: with the new handleError, the message will always be "An unexpected error occurred" (localized). The hardcoded fallback 'Internal Error' in the current template (?? 'Internal Error') should be replaced with a Paraglide key: ?? m.error_internal_error().

Open Decisions

Show the status code to users? A 500 is not useful to a senior transcriber but is useful to a developer helping them. Consider hiding the status code on production and showing only the error ID, or making the status code visually smaller and the message more prominent. This is a UX product decision, not a technical one.

## 👨‍💻 Leonie Voss — UI/UX Design Lead ### Observations - The current `+error.svelte` (14 lines) is minimal but functional: it shows `page.status` (large, bold, `text-6xl`) and `page.error?.message` (small, `text-ink-2`). It uses the correct `font-sans` and `text-ink` / `text-ink-2` tokens. Brand compliance is already correct. - **What's missing:** the `errorId` display and the copy button. The issue's snippet shows the ID in a `<code>` block but does not specify the copy interaction, touch target size, or focus state. For the transcriber audience (60+, laptop/tablet), this matters. - The error page has no localized heading. The `page.status` number (500, 404) is not meaningful to a 65-year-old transcriber. A human-readable headline in German ("Es ist etwas schiefgelaufen" as the issue proposes) is essential. - The `<code>` element for `errorId` will render in the browser's monospace fallback font, which may have poor contrast depending on the system font. The project uses Tinos (serif) and Montserrat (sans) — neither is applied to `<code>` by default. - The copy button needs: visible label or icon, accessible `aria-label`, minimum 44px touch target, and a confirmation state ("Kopiert!" for 2 seconds) so seniors know the action succeeded. ### Recommendations 1. **Add a human-readable headline** before the status code. The issue proposes "Es ist etwas schiefgelaufen" — use the `error_page_title` i18n key already present in all three locales (check the key name matches). Place it as an `<h1>` with `font-serif text-2xl text-ink`. 2. **Style the `errorId` display to brand standards:** ```svelte {#if page.error?.errorId} <div class="mt-4 flex flex-col items-center gap-2"> <p class="text-xs font-sans text-ink-2 uppercase tracking-widest"> {m.error_page_id_label()} </p> <code class="font-mono text-sm bg-surface border border-line rounded px-3 py-1 text-ink select-all"> {page.error.errorId} </code> <button class="mt-1 min-h-[44px] px-4 py-2 text-sm font-sans bg-brand-navy text-white rounded-sm hover:opacity-90 focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2" onclick={() => navigator.clipboard.writeText(page.error!.errorId!)} aria-label={m.error_copy_id_label()} > {m.error_copy_id_label()} </button> </div> {/if} ``` The `select-all` class on the `<code>` element means users can also triple-click to select the ID without using the button — essential fallback for seniors who may not trust or find the button. 3. **Add a confirmation state to the copy button:** a 2-second "Kopiert!" feedback (swap the button label using a `$state` boolean) prevents seniors from pressing the button multiple times uncertain whether it worked. 4. **Check the error page at 320px:** at small viewport widths, `text-6xl` for the status code (96px) will dominate the screen. Consider reducing to `text-4xl` or `text-5xl` on mobile, scaling up on desktop: ```svelte <p class="font-sans text-4xl sm:text-6xl font-bold text-ink">{page.status}</p> ``` 5. **Verify the existing `page.error?.message` display:** with the new `handleError`, the message will always be `"An unexpected error occurred"` (localized). The hardcoded fallback `'Internal Error'` in the current template (`?? 'Internal Error'`) should be replaced with a Paraglide key: `?? m.error_internal_error()`. ### Open Decisions - **Show the status code to users?** A 500 is not useful to a senior transcriber but is useful to a developer helping them. Consider hiding the status code on production and showing only the error ID, or making the status code visually smaller and the message more prominent. This is a UX product decision, not a technical one.

marcel commented

2026-05-07 17:39:27 +02:00

Decision Queue — Open Decisions for #462

Consolidated from all persona reviews. Each item requires a judgment call before or during implementation.

Theme 1: Error ID Correlation (Server vs. Client)

Raised by: Markus Keller

The server handleError and the client handleError each generate independent UUIDs. A client-side crash that triggers a page re-render on the server will produce two unrelated IDs. For this app's SSR-first architecture this is probably acceptable — client errors are mostly post-hydration UI bugs, not server failures.

Decision needed: Accept independent IDs for now (deferred until Sentry/GlitchTip is added), or thread a server-generated errorId through the initial SSR response for the client to reuse?

Recommendation: Accept independent IDs now. Revisit when a real error sink is connected.

Theme 2: Stack Traces in Client Hook

Raised by: Felix Brandt

Browser DevTools already show the full stack for client-side errors. Logging error.stack in the hooks.client.ts console.error call is redundant and adds noise.

Decision needed: Include or omit error.stack from the client hook's console.error payload?

Recommendation: Omit from the client hook. The browser surfaces it automatically.

Theme 3: Stack Traces in Production (Security + DevOps)

Raised by: Nora Steiner, Tobias Wendt

error.stack in stdout may include absolute file system paths (e.g., /home/marcel/Desktop/familienarchiv/...). In development, fine. In production with proper log access controls, acceptable. If logs are ever accidentally exposed, path information aids attackers.

Decision needed: Log full stack traces in production, truncate to N lines, or strip path components?

Recommendation: Truncate to first 3 lines in production using process.env.NODE_ENV === 'production'. Zero infrastructure cost, reduces leakage surface.

Theme 4: Show Status Code to End Users

Raised by: Leonie Voss

500 and 404 are not meaningful to a 65-year-old transcriber. The current +error.svelte shows the status code prominently (text-6xl). With the new error page, the errorId is the user-facing artifact — the status code is developer-facing.

Decision needed: Keep the large status code, make it smaller, or hide it entirely for production users?

Recommendation: Keep it but reduce visual weight — text-2xl or text-xl in muted text-ink-3, and make the human-readable message (h1) more prominent. A senior's first read should be "something went wrong" + the ID, not "500".

Theme 5: `userId` in Server Logs and GDPR

Raised by: Nora Steiner

The server hook logs event.locals.user?.id (a UUID). For a private family app, this is useful for incident correlation and low risk. For any future deployment with external users, user IDs in logs are personal data under GDPR, requiring a documented retention policy.

Decision needed: Log userId as proposed, or log a hashed/pseudonymized value?

Recommendation: Log the raw UUID for now — it is already pseudonymous and the app is family-only. Add a note in the PR that a retention policy is required before any public launch.

## Decision Queue — Open Decisions for #462 Consolidated from all persona reviews. Each item requires a judgment call before or during implementation. --- ### Theme 1: Error ID Correlation (Server vs. Client) **Raised by:** Markus Keller The server `handleError` and the client `handleError` each generate independent UUIDs. A client-side crash that triggers a page re-render on the server will produce two unrelated IDs. For this app's SSR-first architecture this is probably acceptable — client errors are mostly post-hydration UI bugs, not server failures. **Decision needed:** Accept independent IDs for now (deferred until Sentry/GlitchTip is added), or thread a server-generated `errorId` through the initial SSR response for the client to reuse? **Recommendation:** Accept independent IDs now. Revisit when a real error sink is connected. --- ### Theme 2: Stack Traces in Client Hook **Raised by:** Felix Brandt Browser DevTools already show the full stack for client-side errors. Logging `error.stack` in the `hooks.client.ts` `console.error` call is redundant and adds noise. **Decision needed:** Include or omit `error.stack` from the client hook's `console.error` payload? **Recommendation:** Omit from the client hook. The browser surfaces it automatically. --- ### Theme 3: Stack Traces in Production (Security + DevOps) **Raised by:** Nora Steiner, Tobias Wendt `error.stack` in stdout may include absolute file system paths (e.g., `/home/marcel/Desktop/familienarchiv/...`). In development, fine. In production with proper log access controls, acceptable. If logs are ever accidentally exposed, path information aids attackers. **Decision needed:** Log full stack traces in production, truncate to N lines, or strip path components? **Recommendation:** Truncate to first 3 lines in production using `process.env.NODE_ENV === 'production'`. Zero infrastructure cost, reduces leakage surface. --- ### Theme 4: Show Status Code to End Users **Raised by:** Leonie Voss `500` and `404` are not meaningful to a 65-year-old transcriber. The current `+error.svelte` shows the status code prominently (`text-6xl`). With the new error page, the `errorId` is the user-facing artifact — the status code is developer-facing. **Decision needed:** Keep the large status code, make it smaller, or hide it entirely for production users? **Recommendation:** Keep it but reduce visual weight — `text-2xl` or `text-xl` in muted `text-ink-3`, and make the human-readable message (`h1`) more prominent. A senior's first read should be "something went wrong" + the ID, not "500". --- ### Theme 5: `userId` in Server Logs and GDPR **Raised by:** Nora Steiner The server hook logs `event.locals.user?.id` (a UUID). For a private family app, this is useful for incident correlation and low risk. For any future deployment with external users, user IDs in logs are personal data under GDPR, requiring a documented retention policy. **Decision needed:** Log `userId` as proposed, or log a hashed/pseudonymized value? **Recommendation:** Log the raw UUID for now — it is already pseudonymous and the app is family-only. Add a note in the PR that a retention policy is required before any public launch.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#462

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

Context

Approach

frontend/src/hooks.server.ts

frontend/src/hooks.client.ts (new file)

frontend/src/routes/+error.svelte

Lint-time enforcement of related cleanup

Critical files

Verification

Acceptance criteria

Effort

Risk if not addressed

👨‍💻 Markus Keller — Application Architect

Observations

Recommendations

Open Decisions

👨‍💻 Felix Brandt — Fullstack Developer

Observations

Recommendations

Open Decisions

👨‍💻 Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

👨‍💻 Elicit — Requirements Engineer

Observations

Recommendations

👨‍💻 Nora "NullX" Steiner — Security Engineer

Observations

Recommendations

Open Decisions

👨‍💻 Sara Holt — QA Engineer

Observations

Recommendations

Open Decisions

👨‍💻 Leonie Voss — UI/UX Design Lead

Observations

Recommendations

Open Decisions

Decision Queue — Open Decisions for #462

Theme 1: Error ID Correlation (Server vs. Client)

Theme 2: Stack Traces in Client Hook

Theme 3: Stack Traces in Production (Security + DevOps)

Theme 4: Show Status Code to End Users

Theme 5: userId in Server Logs and GDPR

`frontend/src/hooks.server.ts`

`frontend/src/hooks.client.ts` (new file)

`frontend/src/routes/+error.svelte`

Theme 5: `userId` in Server Logs and GDPR