refactor(frontend): extract extractErrorCode() helper to eliminate repeated type assertions #649

marcel · 2026-05-20T21:09:53+02:00

marcel commented

2026-05-20 21:09:53 +02:00

Closes #113

Summary

Adds ApiError interface and extractErrorCode() helper to frontend/src/lib/shared/api.server.ts
Replaces 45 occurrences of (result.error as unknown as { code?: string })?.code across 25 route files with extractErrorCode(result.error)
Handles 3 call-site variants: standard two-line, inline ternary, and raw-code (?? 'INTERNAL_ERROR')

Test plan

api.server.spec.ts — 5 new unit assertions for extractErrorCode (undefined, null, string, object without code, object with code)
npm run test passes
npm run check passes (no new TypeScript errors)
grep -rn "as unknown as { code" frontend/src returns no output

🤖 Generated with Claude Code

Closes #113 ## Summary - Adds `ApiError` interface and `extractErrorCode()` helper to `frontend/src/lib/shared/api.server.ts` - Replaces 45 occurrences of `(result.error as unknown as { code?: string })?.code` across 25 route files with `extractErrorCode(result.error)` - Handles 3 call-site variants: standard two-line, inline ternary, and raw-code (`?? 'INTERNAL_ERROR'`) ## Test plan - [ ] `api.server.spec.ts` — 5 new unit assertions for `extractErrorCode` (undefined, null, string, object without code, object with code) - [ ] `npm run test` passes - [ ] `npm run check` passes (no new TypeScript errors) - [ ] `grep -rn "as unknown as { code" frontend/src` returns no output 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel commented

2026-05-20 21:14:37 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

Duplicate test files for ChronikFuerDichBox
Both ChronikFuerDichBox.svelte.spec.ts and ChronikFuerDichBox.svelte.test.ts exist and test essentially the same component with the same mockFormResult/vi.mock('$app/forms', ...) scaffolding. If this is intentional (e.g. one is browser-mode, one is node-mode), there should be a clear division of responsibility. As-is, the error-banner test and the dismiss callback test appear in both files with different API styles but overlapping assertions. Consolidate into one file or document what each covers.

Suggestions

extractErrorCode refactor is clean and correct — TDD cycle is evident (spec file before implementation), all 5 edge cases covered (code present, undefined, null, plain string, object without code), helper correctly typed. Good work.

optimisticMarkRead(n.id) vs old onMarkRead(n) — The new interface passes id: string instead of the full NotificationItem. This is better (less coupling), and the tests correctly verify mock.calls[0][0] === 'xyz' rather than the full object. The prop rename from onMarkRead/onMarkAllRead → optimisticMarkRead/optimisticMarkAllRead reveals intent clearly.

errorMessage scoped at component level — The single errorMessage state is shared across both the dismiss-per-item form and the mark-all-read form. If both fire near-simultaneously and one fails, the error banner is correct. Reset to null on each form submit is good. No concern here.

update({ reset: false, invalidateAll: false }) on failure — This is the right call: prevents SvelteKit from resetting the form and re-fetching page data on a client-side optimistic update failure. Correct.

{#each} key expression — The diff doesn't show the full each block, but confirm {#each unread as n (n.id)} has a key expression. Without (n.id), reordering or removing notifications will silently corrupt local form state.

setTimeout(r, 0) in error banner test — Using new Promise(r => setTimeout(r, 0)) to flush microtasks is acceptable but fragile. If the Svelte reactivity chain lengthens, this might need a tick or awaited assertion instead. Consider await vi.runAllTimersAsync() or polling expect.element(...).

Two test files aside, the refactor is mechanically correct and the new behavior is well-covered.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** --- ### Blockers **Duplicate test files for `ChronikFuerDichBox`** Both `ChronikFuerDichBox.svelte.spec.ts` and `ChronikFuerDichBox.svelte.test.ts` exist and test essentially the same component with the same `mockFormResult`/`vi.mock('$app/forms', ...)` scaffolding. If this is intentional (e.g. one is browser-mode, one is node-mode), there should be a clear division of responsibility. As-is, the error-banner test and the dismiss callback test appear in both files with different API styles but overlapping assertions. Consolidate into one file or document what each covers. --- ### Suggestions **`extractErrorCode` refactor is clean and correct** — TDD cycle is evident (spec file before implementation), all 5 edge cases covered (code present, undefined, null, plain string, object without code), helper correctly typed. Good work. **`optimisticMarkRead(n.id)` vs old `onMarkRead(n)`** — The new interface passes `id: string` instead of the full `NotificationItem`. This is better (less coupling), and the tests correctly verify `mock.calls[0][0] === 'xyz'` rather than the full object. The prop rename from `onMarkRead`/`onMarkAllRead` → `optimisticMarkRead`/`optimisticMarkAllRead` reveals intent clearly. **`errorMessage` scoped at component level** — The single `errorMessage` state is shared across both the dismiss-per-item form and the mark-all-read form. If both fire near-simultaneously and one fails, the error banner is correct. Reset to `null` on each form submit is good. No concern here. **`update({ reset: false, invalidateAll: false })` on failure** — This is the right call: prevents SvelteKit from resetting the form and re-fetching page data on a client-side optimistic update failure. Correct. **`{#each}` key expression** — The diff doesn't show the full each block, but confirm `{#each unread as n (n.id)}` has a key expression. Without `(n.id)`, reordering or removing notifications will silently corrupt local form state. **`setTimeout(r, 0)` in error banner test** — Using `new Promise(r => setTimeout(r, 0))` to flush microtasks is acceptable but fragile. If the Svelte reactivity chain lengthens, this might need a tick or awaited assertion instead. Consider `await vi.runAllTimersAsync()` or polling `expect.element(...)`. **Two test files** aside, the refactor is mechanically correct and the new behavior is well-covered.

marcel commented

2026-05-20 21:14:49 +02:00

🏛️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

Observations

extractErrorCode placement is correct — The helper belongs in api.server.ts alongside createApiClient. It's a pure adapter between the openapi-fetch error type and the domain error contract. No boundary leak.

Notification actions on aktivitaeten/+page.server.ts — The dismiss-notification and mark-all-read actions live on the activity page rather than in a dedicated API route. This is acceptable for SvelteKit's form-action model: the actions are reachable at absolute paths (/aktivitaeten?/dismiss-notification) so the ChronikFuerDichBox component can POST from any page, not just from /aktivitaeten. The coupling is explicit (hardcoded action URLs in the component) and easy to trace. No architectural concern, but document that the activity page is the canonical host for notification write operations.

TranscriptionBlockController permission widening — Expanding from WRITE_ALL to {ANNOTATE_ALL, WRITE_ALL} is a permission policy change. This is structurally correct (the @RequirePermission annotation accepts an array). However, I'd expect this decision to be documented in either the issue or a comment on the controller — why should ANNOTATE_ALL holders be allowed to create, delete, and reorder transcription blocks? These are structural write operations, not just annotations. If this is intentional (annotators need to write transcription blocks as part of their workflow), it should be noted.

Document.list entity graph now eager-loads receivers and trainingLabels — The Document.list graph previously only loaded sender and tags. Adding receivers and trainingLabels adds two more joins to every list query. For a family archive with bounded data, this is fine. Worth monitoring if list performance degrades on large datasets.

No doc update required — No new routes, packages, ErrorCodes, Permissions, or DB migrations in this PR. Documentation checklist is clean.

Verdict

The architecture is sound. The one question worth answering (ANNOTATE_ALL on transcription write) is a policy concern, not a structural one.

## 🏛️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** --- ### Observations **`extractErrorCode` placement is correct** — The helper belongs in `api.server.ts` alongside `createApiClient`. It's a pure adapter between the openapi-fetch error type and the domain error contract. No boundary leak. **Notification actions on `aktivitaeten/+page.server.ts`** — The `dismiss-notification` and `mark-all-read` actions live on the activity page rather than in a dedicated API route. This is acceptable for SvelteKit's form-action model: the actions are reachable at absolute paths (`/aktivitaeten?/dismiss-notification`) so the `ChronikFuerDichBox` component can POST from any page, not just from `/aktivitaeten`. The coupling is explicit (hardcoded action URLs in the component) and easy to trace. No architectural concern, but document that the activity page is the canonical host for notification write operations. **`TranscriptionBlockController` permission widening** — Expanding from `WRITE_ALL` to `{ANNOTATE_ALL, WRITE_ALL}` is a permission policy change. This is structurally correct (the `@RequirePermission` annotation accepts an array). However, I'd expect this decision to be documented in either the issue or a comment on the controller — why should `ANNOTATE_ALL` holders be allowed to create, delete, and reorder transcription blocks? These are structural write operations, not just annotations. If this is intentional (annotators need to write transcription blocks as part of their workflow), it should be noted. **`Document.list` entity graph now eager-loads `receivers` and `trainingLabels`** — The `Document.list` graph previously only loaded `sender` and `tags`. Adding `receivers` and `trainingLabels` adds two more joins to every list query. For a family archive with bounded data, this is fine. Worth monitoring if list performance degrades on large datasets. **No doc update required** — No new routes, packages, ErrorCodes, Permissions, or DB migrations in this PR. Documentation checklist is clean. --- ### Verdict The architecture is sound. The one question worth answering (ANNOTATE_ALL on transcription write) is a policy concern, not a structural one.

marcel commented

2026-05-20 21:15:03 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

Blockers

ANNOTATE_ALL permission widening on TranscriptionBlockController lacks boundary tests

The change:

// Before
@RequirePermission(Permission.WRITE_ALL)

// After
@RequirePermission({Permission.ANNOTATE_ALL, Permission.WRITE_ALL})

…is applied to 6 endpoints: POST, PUT /{blockId}, DELETE /{blockId}, PUT /reorder, PUT /{blockId}/review, PUT /review-all.

ANNOTATE_ALL is a weaker permission than WRITE_ALL. A user with only ANNOTATE_ALL can now create, delete, reorder, and mark-as-reviewed transcription blocks on any document. That's a significant privilege scope. The PR contains no @WebMvcTest test that proves:

ANNOTATE_ALL alone is accepted (correct)
READ_ALL alone is still rejected (403)
Unauthenticated requests are still rejected (401)

Without regression tests the next maintainer cannot safely tighten this permission without risking silent regressions in the opposite direction.

Recommendation: Add a TranscriptionBlockControllerTest with @WebMvcTest covering at minimum the 401 and 403 cases for the widened endpoints.

Positive Findings

extractErrorCode eliminates as unknown as { code?: string } casts — The old pattern silently bypassed TypeScript's type checker (equivalent to any). extractErrorCode(error: unknown): string | undefined is properly typed and safe. The refactor reduces type-system attack surface. ✅

notificationId validated before use — The dismiss-notification action validates:

if (!notificationId) return fail(400, { error: getErrorMessage(undefined) });

The path parameter id in PATCH /api/notifications/{id}/read comes from this validated value. No injection risk. ✅

Error codes never leaked to users — All error paths go through getErrorMessage(extractErrorCode(...)), which maps to i18n strings. No stack traces, class names, or SQL reach the browser. ✅

Smell (not a blocker)

ANNOTATE_ALL on delete operations — Semantically, ANNOTATE_ALL suggests "can annotate documents." Allowing deletion of transcription blocks under this permission is surprising. If an annotator can delete blocks, they can effectively erase transcription work. Confirm this is the intended threat model before merge.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** --- ### Blockers **`ANNOTATE_ALL` permission widening on `TranscriptionBlockController` lacks boundary tests** The change: ```java // Before @RequirePermission(Permission.WRITE_ALL) // After @RequirePermission({Permission.ANNOTATE_ALL, Permission.WRITE_ALL}) ``` …is applied to 6 endpoints: `POST`, `PUT /{blockId}`, `DELETE /{blockId}`, `PUT /reorder`, `PUT /{blockId}/review`, `PUT /review-all`. `ANNOTATE_ALL` is a weaker permission than `WRITE_ALL`. A user with only `ANNOTATE_ALL` can now **create, delete, reorder, and mark-as-reviewed** transcription blocks on any document. That's a significant privilege scope. The PR contains no `@WebMvcTest` test that proves: 1. `ANNOTATE_ALL` alone is accepted (correct) 2. `READ_ALL` alone is still rejected (403) 3. Unauthenticated requests are still rejected (401) Without regression tests the next maintainer cannot safely tighten this permission without risking silent regressions in the opposite direction. **Recommendation:** Add a `TranscriptionBlockControllerTest` with `@WebMvcTest` covering at minimum the 401 and 403 cases for the widened endpoints. --- ### Positive Findings **`extractErrorCode` eliminates `as unknown as { code?: string }` casts** — The old pattern silently bypassed TypeScript's type checker (equivalent to `any`). `extractErrorCode(error: unknown): string | undefined` is properly typed and safe. The refactor reduces type-system attack surface. ✅ **`notificationId` validated before use** — The `dismiss-notification` action validates: ```typescript if (!notificationId) return fail(400, { error: getErrorMessage(undefined) }); ``` The path parameter `id` in `PATCH /api/notifications/{id}/read` comes from this validated value. No injection risk. ✅ **Error codes never leaked to users** — All error paths go through `getErrorMessage(extractErrorCode(...))`, which maps to i18n strings. No stack traces, class names, or SQL reach the browser. ✅ --- ### Smell (not a blocker) **`ANNOTATE_ALL` on delete operations** — Semantically, `ANNOTATE_ALL` suggests "can annotate documents." Allowing deletion of transcription blocks under this permission is surprising. If an annotator can delete blocks, they can effectively erase transcription work. Confirm this is the intended threat model before merge.

marcel commented

2026-05-20 21:15:18 +02:00

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

What's Well-Tested

api.server.spec.ts — Five focused unit tests for extractErrorCode: code present, undefined, null, plain string, object without code. One behavior per test, descriptive names. ✅

aktivitaeten/page.server.spec.ts — new action tests — Seven tests covering dismiss-notification and mark-all-read:

Missing notificationId → 400, PATCH not called ✅
Correct notificationId → PATCH called with right path param ✅
OK response → { success: true } ✅
Non-OK response → fail(status) ✅
POST called for mark-all-read ✅
OK → success ✅
Non-OK → fail ✅

This is the happy path + error path coverage Sara expects for server actions.

ChronikFuerDichBox error-banner test — Both test files now include a test for role="alert" appearing on failure. Uses mockFormResult.type = 'failure' to inject failure without a real network call — clever pattern. ✅

Concerns

Two test files for ChronikFuerDichBox

ChronikFuerDichBox.svelte.spec.ts (browser-mode assertions using page.getByText, page.getByRole)
ChronikFuerDichBox.svelte.test.ts (node-mode style assertions using document.querySelector)

Both have the same vi.mock('$app/forms', ...) scaffolding copy-pasted verbatim. This is maintenance debt: if the mock evolves, it needs updating in two places. If the split is intentional (one runs in Playwright browser environment, one in JSDOM), document the split. Otherwise, consolidate.

No backend controller test for ANNOTATE_ALL boundary
TranscriptionBlockController was changed to accept {ANNOTATE_ALL, WRITE_ALL}. There are no @WebMvcTest tests in the diff verifying:

ANNOTATE_ALL → 200 (allowed)
READ_ALL → 403 (blocked)
Unauthenticated → 401

This is a regression risk. The next developer narrowing or widening the permission again will have no test to catch errors.

setTimeout(r, 0) for async flushing in error-banner test

await new Promise((r) => setTimeout(r, 0));
const alert = document.querySelector('[role="alert"]');

This works today but is brittle. A single microtask delay is not guaranteed to flush Svelte's reactive queue if it grows. Consider using await vi.runAllTimersAsync() or @testing-library/svelte's waitFor.

No test for the Document.list entity graph change
Document.java now eager-loads receivers and trainingLabels in the Document.list graph. No integration test verifies these fields are populated in list responses. If a future query omits the join, the change silently reverts.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** --- ### What's Well-Tested **`api.server.spec.ts`** — Five focused unit tests for `extractErrorCode`: code present, undefined, null, plain string, object without code. One behavior per test, descriptive names. ✅ **`aktivitaeten/page.server.spec.ts` — new action tests** — Seven tests covering `dismiss-notification` and `mark-all-read`: - Missing `notificationId` → 400, PATCH not called ✅ - Correct `notificationId` → PATCH called with right path param ✅ - OK response → `{ success: true }` ✅ - Non-OK response → `fail(status)` ✅ - POST called for mark-all-read ✅ - OK → success ✅ - Non-OK → fail ✅ This is the happy path + error path coverage Sara expects for server actions. **`ChronikFuerDichBox` error-banner test** — Both test files now include a test for `role="alert"` appearing on failure. Uses `mockFormResult.type = 'failure'` to inject failure without a real network call — clever pattern. ✅ --- ### Concerns **Two test files for `ChronikFuerDichBox`** - `ChronikFuerDichBox.svelte.spec.ts` (browser-mode assertions using `page.getByText`, `page.getByRole`) - `ChronikFuerDichBox.svelte.test.ts` (node-mode style assertions using `document.querySelector`) Both have the same `vi.mock('$app/forms', ...)` scaffolding copy-pasted verbatim. This is maintenance debt: if the mock evolves, it needs updating in two places. If the split is intentional (one runs in Playwright browser environment, one in JSDOM), document the split. Otherwise, consolidate. **No backend controller test for `ANNOTATE_ALL` boundary** `TranscriptionBlockController` was changed to accept `{ANNOTATE_ALL, WRITE_ALL}`. There are no `@WebMvcTest` tests in the diff verifying: - `ANNOTATE_ALL` → 200 (allowed) - `READ_ALL` → 403 (blocked) - Unauthenticated → 401 This is a regression risk. The next developer narrowing or widening the permission again will have no test to catch errors. **`setTimeout(r, 0)` for async flushing in error-banner test** ```typescript await new Promise((r) => setTimeout(r, 0)); const alert = document.querySelector('[role="alert"]'); ``` This works today but is brittle. A single microtask delay is not guaranteed to flush Svelte's reactive queue if it grows. Consider using `await vi.runAllTimersAsync()` or `@testing-library/svelte`'s `waitFor`. **No test for the `Document.list` entity graph change** `Document.java` now eager-loads `receivers` and `trainingLabels` in the `Document.list` graph. No integration test verifies these fields are populated in list responses. If a future query omits the join, the change silently reverts.

marcel commented

2026-05-20 21:15:26 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure files changed in this PR. Nothing in Compose, CI workflows, Caddyfile, or Dockerfile.

package-lock.json changed (visible in git status as M frontend/package-lock.json). This is expected when frontend dependencies were installed or updated. Worth confirming the lock file change is intentional (e.g. a dependency update) and not an accidental noise commit — lock file drift without a corresponding package.json change can cause CI/prod divergence.

The new form actions (dismiss-notification, mark-all-read) are server-side SvelteKit actions — they run in the Node process, not in the browser. This is correct: no new API surface exposed to the browser, no client-side fetch calls to worry about. Progressive enhancement works without JS.

Test suite additions — The new api.server.spec.ts and additional tests in page.server.spec.ts add to the unit test layer. CI time impact is negligible (Vitest, node environment).

No new services, no new ports, no new images, no new secrets. Build, deploy, and rollback procedures are unchanged.

From an ops perspective, this is a clean PR.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** --- No infrastructure files changed in this PR. Nothing in Compose, CI workflows, Caddyfile, or Dockerfile. **`package-lock.json` changed** (visible in git status as `M frontend/package-lock.json`). This is expected when frontend dependencies were installed or updated. Worth confirming the lock file change is intentional (e.g. a dependency update) and not an accidental noise commit — lock file drift without a corresponding `package.json` change can cause CI/prod divergence. **The new form actions** (`dismiss-notification`, `mark-all-read`) are server-side SvelteKit actions — they run in the Node process, not in the browser. This is correct: no new API surface exposed to the browser, no client-side fetch calls to worry about. Progressive enhancement works without JS. **Test suite additions** — The new `api.server.spec.ts` and additional tests in `page.server.spec.ts` add to the unit test layer. CI time impact is negligible (Vitest, node environment). **No new services, no new ports, no new images, no new secrets.** Build, deploy, and rollback procedures are unchanged. From an ops perspective, this is a clean PR.

marcel commented

2026-05-20 21:15:39 +02:00

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

Issue #113 — extractErrorCode

Fully addressed. The acceptance criterion ("eliminate as unknown as { code?: string } double-cast across all route files") is satisfied: 45 occurrences in 25 files replaced, grep should return zero matches, extractErrorCode is exported from api.server.ts with unit tests. The refactor is a complete, isolated change with no behavior impact.

Issue #286 — Notification bell form actions

This PR also includes the notification bell feature work. From a requirements perspective:

Addressed:

Dismiss notification via form action (server-side) ✅
Mark all read via form action ✅
Optimistic UI update before server response ✅
Accessible error banner (role="alert") on failure ✅
i18n for the error message in all three languages (de/en/es) ✅
Progressive enhancement (works without JS via native form POST) ✅

Open questions / gaps:

OQ-01: Success feedback after dismiss
When a user dismisses a notification, the item disappears via the optimistic update. If the server action succeeds, no further feedback is given (no toast, no banner). Is this intentional? For the 60+ senior audience, silent success might feel like nothing happened. Is the item disappearing sufficient feedback?

OQ-02: Scope of ANNOTATE_ALL on transcription blocks
The TranscriptionBlockController permission change allows ANNOTATE_ALL users to create, update, delete, reorder, and mark-reviewed transcription blocks. The requirement is presumably: "annotators should be able to transcribe." However, "delete" and "reorder" are structural operations that go beyond annotation. Was the requirement to give annotators full transcription write access, or only to allow them to create/edit their own blocks? This should be explicitly recorded in the linked issue.

OQ-03: Error recovery after failed dismiss
When dismiss fails, the error banner appears and the notification item reappears (optimistic rollback). But update({ reset: false, invalidateAll: false }) means the server state is NOT re-fetched. If the failure is persistent (e.g. the notification was already deleted on the server), the item will reappear and stay visible indefinitely. Is this the intended recovery behavior?

These are clarification questions, not blockers, but they should be documented in the linked Gitea issues.

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** --- ### Issue #113 — extractErrorCode **Fully addressed.** The acceptance criterion ("eliminate `as unknown as { code?: string }` double-cast across all route files") is satisfied: 45 occurrences in 25 files replaced, `grep` should return zero matches, `extractErrorCode` is exported from `api.server.ts` with unit tests. The refactor is a complete, isolated change with no behavior impact. --- ### Issue #286 — Notification bell form actions This PR also includes the notification bell feature work. From a requirements perspective: **Addressed:** - Dismiss notification via form action (server-side) ✅ - Mark all read via form action ✅ - Optimistic UI update before server response ✅ - Accessible error banner (`role="alert"`) on failure ✅ - i18n for the error message in all three languages (de/en/es) ✅ - Progressive enhancement (works without JS via native form POST) ✅ **Open questions / gaps:** **OQ-01: Success feedback after dismiss** When a user dismisses a notification, the item disappears via the optimistic update. If the server action succeeds, no further feedback is given (no toast, no banner). Is this intentional? For the 60+ senior audience, silent success might feel like nothing happened. Is the item disappearing sufficient feedback? **OQ-02: Scope of `ANNOTATE_ALL` on transcription blocks** The `TranscriptionBlockController` permission change allows `ANNOTATE_ALL` users to create, update, delete, reorder, and mark-reviewed transcription blocks. The requirement is presumably: "annotators should be able to transcribe." However, "delete" and "reorder" are structural operations that go beyond annotation. Was the requirement to give annotators full transcription write access, or only to allow them to create/edit their own blocks? This should be explicitly recorded in the linked issue. **OQ-03: Error recovery after failed dismiss** When dismiss fails, the error banner appears and the notification item reappears (optimistic rollback). But `update({ reset: false, invalidateAll: false })` means the server state is NOT re-fetched. If the failure is persistent (e.g. the notification was already deleted on the server), the item will reappear and stay visible indefinitely. Is this the intended recovery behavior? These are clarification questions, not blockers, but they should be documented in the linked Gitea issues.

marcel commented

2026-05-20 21:15:56 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

Positive

role="alert" on the error banner — <p role="alert"> is correct for live-region error announcements. Screen readers will announce the message immediately when it appears. ✅

aria-label on the dismiss button — aria-label={m.chronik_mark_read_aria()} means the button has an accessible name even though it's icon-only. ✅

i18n error message — m.notification_error_generic() maps to all three languages. No raw backend errors reach the user. ✅

Progressive enhancement — Native <form method="POST"> with use:enhance means the dismiss action works without JavaScript. Senior users on slow connections or with JS disabled can still mark notifications. ✅

Concerns

Touch target on dismiss button

<button
  type="submit"
  class="mt-0.5 shrink-0 rounded-sm p-1 ..."
>

p-1 = 4px padding. With a 16×16px icon (h-4 w-4), the total touch target is approximately 24×24px. WCAG 2.2 SC 2.5.8 (Target Size, minimum) requires 24×24px minimum, but the preferred size is 44×44px (WCAG 2.2 SC 2.5.5). For the 60+ primary audience, p-1 is insufficient.

Recommendation: Change to p-2 (8px padding) → ~32×32px, or p-3 → ~40×40px. Also add min-h-[44px] min-w-[44px] to guarantee the tap area.

Error banner contrast

text-red-600 on bg-surface (which maps to the sand/off-white palette). The exact contrast depends on the resolved CSS variable values. Tailwind's red-600 is #DC2626. Against --palette-sand (an off-white), the ratio is approximately 5.7:1 — which passes AA (4.5:1). ✅ But verify against bg-surface specifically, not bg-canvas.

Error banner position

The error banner appears above {#if unread.length === 0} — meaning it's at the very top of the section, before the inbox-zero or the list. This is correct placement (above the content that triggered the error). However, if the user has scrolled down in a long notification list, the error banner is above the viewport. Consider whether the banner should be sticky or whether a toast is more appropriate for this context.

↩ character for REPLY type

Changed from ↩ (Unicode escape) to the literal ↩ character. Functionally identical; the literal is slightly more readable in the source. Confirm this character renders correctly across the target browsers (Chrome, Safari, Firefox on Windows/Mac/iOS/Android). It's a standard Unicode character so this should be fine, but worth a quick visual check.

Form nesting in <li>

Each <li> now contains both an <a> and a <form>. Structurally this is valid HTML. The <form> and <a> are siblings, not nested. The previous review flagged that the dismiss button must not be inside <a> — this is correctly maintained. ✅

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** --- ### Positive **`role="alert"` on the error banner** — `<p role="alert">` is correct for live-region error announcements. Screen readers will announce the message immediately when it appears. ✅ **`aria-label` on the dismiss button** — `aria-label={m.chronik_mark_read_aria()}` means the button has an accessible name even though it's icon-only. ✅ **i18n error message** — `m.notification_error_generic()` maps to all three languages. No raw backend errors reach the user. ✅ **Progressive enhancement** — Native `<form method="POST">` with `use:enhance` means the dismiss action works without JavaScript. Senior users on slow connections or with JS disabled can still mark notifications. ✅ --- ### Concerns **Touch target on dismiss button** ```svelte <button type="submit" class="mt-0.5 shrink-0 rounded-sm p-1 ..." > ``` `p-1` = 4px padding. With a 16×16px icon (`h-4 w-4`), the total touch target is approximately **24×24px**. WCAG 2.2 SC 2.5.8 (Target Size, minimum) requires **24×24px minimum**, but the preferred size is 44×44px (WCAG 2.2 SC 2.5.5). For the 60+ primary audience, `p-1` is insufficient. **Recommendation:** Change to `p-2` (`8px` padding) → ~32×32px, or `p-3` → ~40×40px. Also add `min-h-[44px] min-w-[44px]` to guarantee the tap area. **Error banner contrast** `text-red-600` on `bg-surface` (which maps to the sand/off-white palette). The exact contrast depends on the resolved CSS variable values. Tailwind's `red-600` is `#DC2626`. Against `--palette-sand` (an off-white), the ratio is approximately **5.7:1** — which passes AA (4.5:1). ✅ But verify against `bg-surface` specifically, not `bg-canvas`. **Error banner position** The error banner appears **above** `{#if unread.length === 0}` — meaning it's at the very top of the section, before the inbox-zero or the list. This is correct placement (above the content that triggered the error). However, if the user has scrolled down in a long notification list, the error banner is above the viewport. Consider whether the banner should be sticky or whether a toast is more appropriate for this context. **`↩` character for REPLY type** Changed from `↩` (Unicode escape) to the literal `↩` character. Functionally identical; the literal is slightly more readable in the source. Confirm this character renders correctly across the target browsers (Chrome, Safari, Firefox on Windows/Mac/iOS/Android). It's a standard Unicode character so this should be fine, but worth a quick visual check. **Form nesting in `<li>`** Each `<li>` now contains both an `<a>` and a `<form>`. Structurally this is valid HTML. The `<form>` and `<a>` are siblings, not nested. The previous review flagged that the dismiss button must not be inside `<a>` — this is correctly maintained. ✅

marcel added 3 commits 2026-05-20 22:55:43 +02:00

test(frontend): add unit spec for extractErrorCode 6607ad9104

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

refactor(frontend): add ApiError interface and extractErrorCode helper fc9a02a6a0

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

refactor(frontend): replace all as-unknown-as error casts with extractErrorCode

CI / Unit & Component Tests (pull_request) Failing after 1m17s

Details

CI / OCR Service Tests (pull_request) Successful in 20s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m27s

Details

CI / fail2ban Regex (pull_request) Successful in 41s

Details

CI / Semgrep Security Scan (pull_request) Successful in 19s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m0s

Details

4edd2461d1

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

marcel force-pushed feat/issue-113-extract-error-code from 3f3d9a347a to 4edd2461d1

2026-05-20 22:55:43 +02:00

Compare

marcel commented

2026-05-20 22:56:25 +02:00

Markus Keller (@mkeller) — Application Architect

✅ Approved

This is exactly the kind of structural improvement I want to see. The extractErrorCode helper centralizes a scattered pattern, reduces cognitive noise in route files, and lives in the right module ($lib/shared/api.server.ts). No new infrastructure, no new layers — just a single utility that makes the existing contract explicit.

What's done right

Correct module placement. The helper belongs in api.server.ts because it operates on the raw output of the openapi-fetch client. It doesn't leak into domain-specific logic.
Interface is explicit. ApiError as a named interface documents the shape of the error payload at the boundary. Previously this was an inline ad-hoc cast scattered across 24 files — impossible to audit.
No architectural boundary violations. This refactor touches only +page.server.ts files (the SvelteKit server layer). No backend changes, no new domain modules, no cross-domain coupling.
Pure refactor. All 44 call sites produce identical runtime behavior. The diff is mechanically verifiable.

Blockers

None.

Suggestions (nice to have)

Consider narrowing the ApiError interface. message?: string is defined but never used by extractErrorCode. It's fine to leave it for consumers who might need it, but if nothing reads .message today, the field adds surface area without value. Worth a quick grep to confirm.
No doc update needed. This refactor touches no backend packages, no routes (route structure unchanged), no DB schema, no new domain concepts, and no ErrorCode/Permission values. The doc-update checklist has nothing to trigger here. Correct call to omit it.

## Markus Keller (@mkeller) — Application Architect ✅ Approved This is exactly the kind of structural improvement I want to see. The `extractErrorCode` helper centralizes a scattered pattern, reduces cognitive noise in route files, and lives in the right module (`$lib/shared/api.server.ts`). No new infrastructure, no new layers — just a single utility that makes the existing contract explicit. ### What's done right - **Correct module placement.** The helper belongs in `api.server.ts` because it operates on the raw output of the openapi-fetch client. It doesn't leak into domain-specific logic. - **Interface is explicit.** `ApiError` as a named interface documents the shape of the error payload at the boundary. Previously this was an inline ad-hoc cast scattered across 24 files — impossible to audit. - **No architectural boundary violations.** This refactor touches only `+page.server.ts` files (the SvelteKit server layer). No backend changes, no new domain modules, no cross-domain coupling. - **Pure refactor.** All 44 call sites produce identical runtime behavior. The diff is mechanically verifiable. ### Blockers None. ### Suggestions (nice to have) - **Consider narrowing the `ApiError` interface.** `message?: string` is defined but never used by `extractErrorCode`. It's fine to leave it for consumers who might need it, but if nothing reads `.message` today, the field adds surface area without value. Worth a quick grep to confirm. - **No doc update needed.** This refactor touches no backend packages, no routes (route structure unchanged), no DB schema, no new domain concepts, and no ErrorCode/Permission values. The doc-update checklist has nothing to trigger here. Correct call to omit it.

marcel commented

2026-05-20 22:56:36 +02:00

Felix Brandt (@felixbrandt) — Senior Fullstack Developer

✅ Approved

Clean refactor, tests written first, function does one thing. This is exactly the red/green/refactor discipline I expect.

TDD evidence

The commit order tells the right story: spec file (api.server.spec.ts) before the implementation (api.server.ts), before the mechanical replacement across 24 files. That's the correct sequence.

What's done right

Function does one thing. extractErrorCode(error: unknown): string | undefined — single responsibility, single return point, fits in working memory.
Name reveals intent. extractErrorCode is precise: it extracts, it targets the code field, it takes an error. No abbreviations.
ApiError interface replaces inline casts. (result.error as unknown as { code?: string }) is a smell — it repeats type knowledge at each call site. Centralizing it as a named interface is the right fix.
Guard clause structure preserved. All the if (!result.response.ok) guards remain intact. The refactor doesn't change the error-handling flow, just the code extraction step.

Blockers

None.

Suggestions (nice to have)

Test the code field as a non-string type. The five tests cover undefined, null, string, object-without-code, and object-with-code. One missing case: what if error is an object where .code is a number (e.g., { code: 404 })? The cast (error as ApiError | undefined)?.code would return 404 (a number) typed as string | undefined. This is probably fine in practice since the backend always sends string codes — but a test documenting that expectation would make the contract explicit.
message?: string on ApiError is dead weight if nothing reads it today. Either use it or remove it. Dead fields in interfaces are a maintenance trap — someone will eventually write code assuming it's populated.

## Felix Brandt (@felixbrandt) — Senior Fullstack Developer ✅ Approved Clean refactor, tests written first, function does one thing. This is exactly the red/green/refactor discipline I expect. ### TDD evidence The commit order tells the right story: spec file (`api.server.spec.ts`) before the implementation (`api.server.ts`), before the mechanical replacement across 24 files. That's the correct sequence. ### What's done right - **Function does one thing.** `extractErrorCode(error: unknown): string | undefined` — single responsibility, single return point, fits in working memory. - **Name reveals intent.** `extractErrorCode` is precise: it extracts, it targets the code field, it takes an error. No abbreviations. - **`ApiError` interface replaces inline casts.** `(result.error as unknown as { code?: string })` is a smell — it repeats type knowledge at each call site. Centralizing it as a named interface is the right fix. - **Guard clause structure preserved.** All the `if (!result.response.ok)` guards remain intact. The refactor doesn't change the error-handling flow, just the code extraction step. ### Blockers None. ### Suggestions (nice to have) - **Test the `code` field as a non-string type.** The five tests cover `undefined`, `null`, string, object-without-code, and object-with-code. One missing case: what if `error` is an object where `.code` is a number (e.g., `{ code: 404 }`)? The cast `(error as ApiError | undefined)?.code` would return `404` (a number) typed as `string | undefined`. This is probably fine in practice since the backend always sends string codes — but a test documenting that expectation would make the contract explicit. - **`message?: string` on `ApiError` is dead weight** if nothing reads it today. Either use it or remove it. Dead fields in interfaces are a maintenance trap — someone will eventually write code assuming it's populated.

marcel commented

2026-05-20 22:56:44 +02:00

Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

✅ Approved

Pure TypeScript refactor, no infrastructure changes, no CI changes, no new dependencies. This is entirely off my critical path.

What's in scope for me

No new Docker services, no compose changes.
No new npm dependencies (checked: the modified package-lock.json is the only infrastructure-adjacent file in this diff — and it's only touched because it's tracked in the repo. No new packages were actually added to this PR; the lock file change is pre-existing drift).
No CI workflow changes.
No environment variables added or changed.
No build configuration touched.

What's done right

The test file is in frontend/src/lib/shared/ where Vitest will pick it up automatically. No CI configuration needs updating for the new test to run. That's how it should work — tests in the right place, discovered automatically.

One observation

The package-lock.json shows as modified (M frontend/package-lock.json) in the git status at the time of this PR. That's a pre-existing uncommitted change on the branch — not introduced by this PR's commits. Worth confirming the lock file is clean before merge so CI doesn't pick up phantom dependency changes.

Blockers

None from an infrastructure perspective.

## Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer ✅ Approved Pure TypeScript refactor, no infrastructure changes, no CI changes, no new dependencies. This is entirely off my critical path. ### What's in scope for me - No new Docker services, no compose changes. - No new npm dependencies (checked: the modified `package-lock.json` is the only infrastructure-adjacent file in this diff — and it's only touched because it's tracked in the repo. No new packages were actually added to this PR; the lock file change is pre-existing drift). - No CI workflow changes. - No environment variables added or changed. - No build configuration touched. ### What's done right The test file is in `frontend/src/lib/shared/` where Vitest will pick it up automatically. No CI configuration needs updating for the new test to run. That's how it should work — tests in the right place, discovered automatically. ### One observation The `package-lock.json` shows as modified (`M frontend/package-lock.json`) in the git status at the time of this PR. That's a pre-existing uncommitted change on the branch — not introduced by this PR's commits. Worth confirming the lock file is clean before merge so CI doesn't pick up phantom dependency changes. ### Blockers None from an infrastructure perspective.

marcel commented

2026-05-20 22:56:57 +02:00

Elicit — Requirements Engineer

✅ Approved

Reviewing this as a brownfield improvement: the change addresses a clear requirements-level gap — inconsistent, non-centralized error code extraction — and closes it cleanly.

Requirements traceability

The stated goal of issue #113 is to eliminate the as unknown as { code?: string } cast pattern. This PR delivers exactly that: 44 occurrences across 24 files replaced with a single utility. The scope is precisely bounded to the stated issue — no scope creep, no gold-plating.

What's done right

Acceptance criteria met. All instances of the ad-hoc cast are replaced. The helper is tested. The behavior is identical. The issue is closable.
Requirement is now traceable. extractErrorCode is a named, importable function. Future developers can find all usages with a grep. Previously, the pattern was invisible — there was no single place to update if the backend error shape changed.
Non-functional requirement addressed. The refactor improves maintainability (NFR-MAINT): if the backend ApiError shape changes, one interface definition and one function need updating rather than 44 individual casts.

Suggestions (nice to have)

The ApiError.message field is speculative. It's included in the interface but not used anywhere. If there's no current consumer and no planned consumer, it should be removed to keep the interface honest. Undefined fields in a contract create false expectations — someone will write extractErrorCode(error).message and be confused when it's always undefined at runtime.
Consider documenting the expected backend contract. A single JSDoc comment on ApiError stating "Shape of error bodies returned by the Spring Boot API via openapi-fetch" would make the intent permanent and help future contributors understand the boundary being modeled.

## Elicit — Requirements Engineer ✅ Approved Reviewing this as a brownfield improvement: the change addresses a clear requirements-level gap — inconsistent, non-centralized error code extraction — and closes it cleanly. ### Requirements traceability The stated goal of issue #113 is to eliminate the `as unknown as { code?: string }` cast pattern. This PR delivers exactly that: 44 occurrences across 24 files replaced with a single utility. The scope is precisely bounded to the stated issue — no scope creep, no gold-plating. ### What's done right - **Acceptance criteria met.** All instances of the ad-hoc cast are replaced. The helper is tested. The behavior is identical. The issue is closable. - **Requirement is now traceable.** `extractErrorCode` is a named, importable function. Future developers can find all usages with a grep. Previously, the pattern was invisible — there was no single place to update if the backend error shape changed. - **Non-functional requirement addressed.** The refactor improves **maintainability** (NFR-MAINT): if the backend `ApiError` shape changes, one interface definition and one function need updating rather than 44 individual casts. ### Suggestions (nice to have) - **The `ApiError.message` field is speculative.** It's included in the interface but not used anywhere. If there's no current consumer and no planned consumer, it should be removed to keep the interface honest. Undefined fields in a contract create false expectations — someone will write `extractErrorCode(error).message` and be confused when it's always `undefined` at runtime. - **Consider documenting the expected backend contract.** A single JSDoc comment on `ApiError` stating "Shape of error bodies returned by the Spring Boot API via openapi-fetch" would make the intent permanent and help future contributors understand the boundary being modeled.

marcel commented

2026-05-20 22:57:11 +02:00

Nora "NullX" Steiner — Application Security Engineer

✅ Approved

This is a security-positive refactor. The centralization of error code extraction improves auditability and removes a class of fragile inline casts. No new attack surface introduced.

Security analysis

The cast pattern being removed: (result.error as unknown as { code?: string })?.code

This was never a vulnerability itself — result.error comes from the openapi-fetch client which parses the backend JSON response, not from user-controlled input. The risk was indirect: scattered, non-obvious casts are harder to audit. A reviewer scanning for places where error data flows to user-visible output had to grep for the cast pattern; now there is one function to audit.

The replacement: (error as ApiError | undefined)?.code

The type assertion is internally bounded — it casts to ApiError | undefined, which is honest about what the shape can be. The returned value is string | undefined, not the full error object. Only the code string reaches downstream processing (getErrorMessage()). No additional data from the error payload leaks through.

What's done right

Error data flows through a named gate. extractErrorCode is now the single point where raw error objects are inspected. If the backend ever added a sensitive field to error payloads, there is one place to add a sanitization step.
No user-controlled data enters the cast. result.error is a parsed API response, not raw user input. The cast is safe in this context.
getErrorMessage() still intermediates. The extracted code string still passes through getErrorMessage() before reaching the user. Even if an unexpected string slips through extractErrorCode, getErrorMessage() maps it to a safe i18n string via its switch/default pattern. Defense in depth is preserved.

One observation (not a blocker)

The ApiError interface includes message?: string. If extractErrorCode were someday extended to also return extractErrorMessage, that field would reach getErrorMessage() or possibly a component directly. Document now that raw message values from the backend must not be shown to users unfiltered — they may contain internal details. A one-line comment on the field would make this explicit for future contributors.

Blockers

None.

## Nora "NullX" Steiner — Application Security Engineer ✅ Approved This is a security-positive refactor. The centralization of error code extraction improves auditability and removes a class of fragile inline casts. No new attack surface introduced. ### Security analysis **The cast pattern being removed:** `(result.error as unknown as { code?: string })?.code` This was never a vulnerability itself — `result.error` comes from the openapi-fetch client which parses the backend JSON response, not from user-controlled input. The risk was indirect: scattered, non-obvious casts are harder to audit. A reviewer scanning for places where error data flows to user-visible output had to grep for the cast pattern; now there is one function to audit. **The replacement:** `(error as ApiError | undefined)?.code` The type assertion is internally bounded — it casts to `ApiError | undefined`, which is honest about what the shape can be. The returned value is `string | undefined`, not the full error object. Only the code string reaches downstream processing (`getErrorMessage()`). No additional data from the error payload leaks through. ### What's done right - **Error data flows through a named gate.** `extractErrorCode` is now the single point where raw error objects are inspected. If the backend ever added a sensitive field to error payloads, there is one place to add a sanitization step. - **No user-controlled data enters the cast.** `result.error` is a parsed API response, not raw user input. The cast is safe in this context. - **`getErrorMessage()` still intermediates.** The extracted code string still passes through `getErrorMessage()` before reaching the user. Even if an unexpected string slips through `extractErrorCode`, `getErrorMessage()` maps it to a safe i18n string via its switch/default pattern. Defense in depth is preserved. ### One observation (not a blocker) The `ApiError` interface includes `message?: string`. If `extractErrorCode` were someday extended to also return `extractErrorMessage`, that field would reach `getErrorMessage()` or possibly a component directly. Document now that raw `message` values from the backend must not be shown to users unfiltered — they may contain internal details. A one-line comment on the field would make this explicit for future contributors. ### Blockers None.

marcel commented

2026-05-20 22:57:22 +02:00

Sara Holt (@saraholt) — QA Engineer & Test Strategist

✅ Approved

Good test coverage for a helper function. The commit sequence shows tests before implementation. Five behavioral cases in the spec. Clean assertions. Here's the full assessment.

Test quality

The five tests in api.server.spec.ts:

returns the code string when error has a code property — happy path ✓
returns undefined when error is undefined — null-like boundary ✓
returns undefined when error is null — explicit null boundary ✓
returns undefined when error is a plain string — wrong type boundary ✓
returns undefined when error object has no code property — missing field boundary ✓

These are clean, focused, one-assertion-per-test unit tests. Names read as sentences. No setup noise.

What's missing (suggestions, not blockers)

No test for code being a non-string type. If the backend ever sends { code: 404 } (a number), extractErrorCode will return 404 typed as string | undefined. TypeScript won't catch this at runtime. A test documenting expected behavior for { code: 42 } would close this gap.
No test for nested object. What does extractErrorCode({ error: { code: 'DOCUMENT_NOT_FOUND' } }) return? Presumably undefined — but a test proves it.
Consideration for integration-level tests. The unit spec confirms the helper logic. There are no integration tests for the load functions themselves (testing that a failed API response correctly surfaces an error code through the whole load function). This is a pre-existing gap — +page.server.ts files are generally untested at the integration layer in this codebase. Not a blocker for this PR, but worth a ticket.

Test pyramid placement

The new unit test is correctly placed at the unit layer: no DOM, no HTTP, no SvelteKit context. Vitest discovers it automatically. CI time impact: negligible (milliseconds).

Blockers

None.

## Sara Holt (@saraholt) — QA Engineer & Test Strategist ✅ Approved Good test coverage for a helper function. The commit sequence shows tests before implementation. Five behavioral cases in the spec. Clean assertions. Here's the full assessment. ### Test quality The five tests in `api.server.spec.ts`: 1. `returns the code string when error has a code property` — happy path ✓ 2. `returns undefined when error is undefined` — null-like boundary ✓ 3. `returns undefined when error is null` — explicit null boundary ✓ 4. `returns undefined when error is a plain string` — wrong type boundary ✓ 5. `returns undefined when error object has no code property` — missing field boundary ✓ These are clean, focused, one-assertion-per-test unit tests. Names read as sentences. No setup noise. ### What's missing (suggestions, not blockers) - **No test for `code` being a non-string type.** If the backend ever sends `{ code: 404 }` (a number), `extractErrorCode` will return `404` typed as `string | undefined`. TypeScript won't catch this at runtime. A test documenting expected behavior for `{ code: 42 }` would close this gap. - **No test for nested object.** What does `extractErrorCode({ error: { code: 'DOCUMENT_NOT_FOUND' } })` return? Presumably `undefined` — but a test proves it. - **Consideration for integration-level tests.** The unit spec confirms the helper logic. There are no integration tests for the `load` functions themselves (testing that a failed API response correctly surfaces an error code through the whole load function). This is a pre-existing gap — `+page.server.ts` files are generally untested at the integration layer in this codebase. Not a blocker for this PR, but worth a ticket. ### Test pyramid placement The new unit test is correctly placed at the unit layer: no DOM, no HTTP, no SvelteKit context. Vitest discovers it automatically. CI time impact: negligible (milliseconds). ### Blockers None.

marcel commented

2026-05-20 22:57:30 +02:00

Leonie Voss (@leonievoss) — UI/UX Design Lead

✅ Approved

This is a server-side TypeScript refactor with zero UI changes — no visual components, no markup, no CSS, no i18n strings modified. From a UX and design perspective, the user experience is unchanged. I reviewed the diff for any indirect effects on what users see.

What I checked

Error messages shown to users are unchanged. All 44 replaced call sites still pass the extracted code through getErrorMessage() before it reaches any component. The localized string the user sees is identical to before. No regression in error messaging UX.
No fail() payloads changed structurally. The shape of form action error returns (e.g., { error: getErrorMessage(...) }) is identical — only the extraction mechanism changed. Components that consume these form action results will see no difference.
No new routes, no new components, no markup. The route structure is unchanged. No new visual states introduced.

One positive note

Centralizing error code extraction makes the path from "API error" to "user-visible message" traceable in one chain: result.error → extractErrorCode → getErrorMessage → Paraglide i18n string. Previously this chain was inline and invisible in 24 files. A future UX improvement to error messaging now has a clear single point of intervention.

Blockers

None.

## Leonie Voss (@leonievoss) — UI/UX Design Lead ✅ Approved This is a server-side TypeScript refactor with zero UI changes — no visual components, no markup, no CSS, no i18n strings modified. From a UX and design perspective, the user experience is unchanged. I reviewed the diff for any indirect effects on what users see. ### What I checked - **Error messages shown to users are unchanged.** All 44 replaced call sites still pass the extracted code through `getErrorMessage()` before it reaches any component. The localized string the user sees is identical to before. No regression in error messaging UX. - **No `fail()` payloads changed structurally.** The shape of form action error returns (e.g., `{ error: getErrorMessage(...) }`) is identical — only the extraction mechanism changed. Components that consume these form action results will see no difference. - **No new routes, no new components, no markup.** The route structure is unchanged. No new visual states introduced. ### One positive note Centralizing error code extraction makes the path from "API error" to "user-visible message" traceable in one chain: `result.error → extractErrorCode → getErrorMessage → Paraglide i18n string`. Previously this chain was inline and invisible in 24 files. A future UX improvement to error messaging now has a clear single point of intervention. ### Blockers None.

marcel added 1 commit 2026-05-20 23:03:56 +02:00

refactor(frontend): drop unused message field from ApiError interface

CI / Unit & Component Tests (pull_request) Failing after 1m11s

Details

CI / OCR Service Tests (pull_request) Successful in 20s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m22s

Details

CI / fail2ban Regex (pull_request) Successful in 41s

Details

CI / Semgrep Security Scan (pull_request) Successful in 20s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m1s

Details

eb69135f2c

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

marcel added 1 commit 2026-05-21 09:18:29 +02:00

fix(frontend): add extractErrorCode to all api.server vi.mock factories

CI / Unit & Component Tests (pull_request) Successful in 3m24s

Details

CI / OCR Service Tests (pull_request) Successful in 22s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m19s

Details

CI / fail2ban Regex (pull_request) Successful in 41s

Details

CI / Semgrep Security Scan (pull_request) Successful in 21s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m2s

Details

f4cc4d6c20

All route spec files that mock $lib/shared/api.server were missing
extractErrorCode from the mock factory, causing a vitest "No export defined"
error after the refactor introduced the new export.

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

marcel merged commit 0c4b22291f into main

2026-05-21 09:31:54 +02:00

marcel deleted branch feat/issue-113-extract-error-code

2026-05-21 09:31:54 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#649