fix: entity graph gaps, ANNOTATE_ALL on transcription blocks, CSRF on client fetch #648

marcel · 2026-05-20T20:11:02+02:00

marcel commented

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

Summary

Three production bugs found and fixed during a systematic debugging session.

1. `Document.list` entity graph missing `receivers` and `trainingLabels`

Document.list only loaded sender and tags. With open-in-view: false, accessing any unloaded lazy association after the transaction closes throws LazyInitializationException.

receivers was causing an immediate crash when sorting the document list by receiver (sortByFirstReceiver → doc.getReceivers() after session closed).
trainingLabels is a latent crash — Jackson serializes the full Document entity in every list response, so any document with OCR training labels assigned would fail.
Document.full was also missing trainingLabels (same serialization risk on the detail page).

2. `TranscriptionBlockController` blocked `ANNOTATE_ALL` users with 403

All write endpoints required WRITE_ALL exclusively. Users with ANNOTATE_ALL (the transcriber role) could not save, review, delete, or reorder transcription blocks. AnnotationController and CommentController already accepted {ANNOTATE_ALL, WRITE_ALL} — this brings TranscriptionBlockController in line with that pattern.

3. Client-side fetch calls missing `X-XSRF-TOKEN` header

hooks.server.ts forwards the CSRF cookie as a header for server-side fetch (form actions, load functions). Client-side XHR calls — transcription block saves, reorder, review, training label toggle, bulk upload, OCR training triggers — bypassed this and never sent X-XSRF-TOKEN. Spring Security's CSRF filter rejected them with 403 before PermissionAspect even ran.

Fix: Added getCsrfToken / withCsrf / makeCsrfFetch to cookies.ts. useTranscriptionBlocks wraps its injectable fetchImpl with makeCsrfFetch (covers all block mutations and saveBlockWithConflictRetry). The remaining direct fetch calls in useBlockAutoSave, TranscriptionEditView, BulkDocumentEditLayout, OcrTrainingCard, and SegmentationTrainingCard apply withCsrf inline.

Test plan

Log in as a user with only ANNOTATE_ALL (no WRITE_ALL) — verify transcription blocks can be saved, reviewed, and deleted
Log in as a user with WRITE_ALL — verify transcription blocks still work
Sort document list by receiver — verify no LazyInitializationException in backend logs
Open document list — verify no crash for documents with training labels
Open document detail — verify trainingLabels field is present in response
Drag-to-reorder transcription blocks — verify reorder PUT succeeds (no 403)
Bulk upload files — verify POST succeeds (no 403)

🤖 Generated with Claude Code

## Summary Three production bugs found and fixed during a systematic debugging session. ### 1. `Document.list` entity graph missing `receivers` and `trainingLabels` `Document.list` only loaded `sender` and `tags`. With `open-in-view: false`, accessing any unloaded lazy association after the transaction closes throws `LazyInitializationException`. - **`receivers`** was causing an immediate crash when sorting the document list by receiver (`sortByFirstReceiver` → `doc.getReceivers()` after session closed). - **`trainingLabels`** is a latent crash — Jackson serializes the full `Document` entity in every list response, so any document with OCR training labels assigned would fail. - `Document.full` was also missing `trainingLabels` (same serialization risk on the detail page). ### 2. `TranscriptionBlockController` blocked `ANNOTATE_ALL` users with 403 All write endpoints required `WRITE_ALL` exclusively. Users with `ANNOTATE_ALL` (the transcriber role) could not save, review, delete, or reorder transcription blocks. `AnnotationController` and `CommentController` already accepted `{ANNOTATE_ALL, WRITE_ALL}` — this brings `TranscriptionBlockController` in line with that pattern. ### 3. Client-side fetch calls missing `X-XSRF-TOKEN` header `hooks.server.ts` forwards the CSRF cookie as a header for server-side fetch (form actions, load functions). Client-side XHR calls — transcription block saves, reorder, review, training label toggle, bulk upload, OCR training triggers — bypassed this and never sent `X-XSRF-TOKEN`. Spring Security's CSRF filter rejected them with 403 before `PermissionAspect` even ran. **Fix:** Added `getCsrfToken` / `withCsrf` / `makeCsrfFetch` to `cookies.ts`. `useTranscriptionBlocks` wraps its injectable `fetchImpl` with `makeCsrfFetch` (covers all block mutations and `saveBlockWithConflictRetry`). The remaining direct `fetch` calls in `useBlockAutoSave`, `TranscriptionEditView`, `BulkDocumentEditLayout`, `OcrTrainingCard`, and `SegmentationTrainingCard` apply `withCsrf` inline. ## Test plan - [ ] Log in as a user with only `ANNOTATE_ALL` (no `WRITE_ALL`) — verify transcription blocks can be saved, reviewed, and deleted - [ ] Log in as a user with `WRITE_ALL` — verify transcription blocks still work - [ ] Sort document list by receiver — verify no `LazyInitializationException` in backend logs - [ ] Open document list — verify no crash for documents with training labels - [ ] Open document detail — verify `trainingLabels` field is present in response - [ ] Drag-to-reorder transcription blocks — verify reorder PUT succeeds (no 403) - [ ] Bulk upload files — verify POST succeeds (no 403) 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel commented

2026-05-20 20:18:06 +02:00

🏗️ Markus Keller — Software Architect

Verdict: ✅ Approved

What I checked

Entity graph integrity, permission system correctness, CSRF utility placement, form action architecture.

Entity graphs — correct fixes

Adding receivers to Document.list is the right fix: sortByFirstReceiver() in DocumentService accesses the collection after the JPA session closes (OSIV=false). Adding trainingLabels prevents a latent Jackson serialization crash from an uninitialized @ElementCollection. A tracking issue has been created to optimize the list graph later — the defensive inclusion is acceptable in the meantime.

Permission change — follows established pattern

Expanding @RequirePermission(WRITE_ALL) to @RequirePermission({ANNOTATE_ALL, WRITE_ALL}) in TranscriptionBlockController matches AnnotationController and CommentController. The permission aspect uses OR semantics. This is correct and consistent.

CSRF utilities — correct placement

getCsrfToken(), withCsrf(), and makeCsrfFetch() in cookies.ts is a reasonable location — the file already reads cookies. The makeCsrfFetch wrapper preserves the injectable fetchImpl contract in hooks, keeping unit tests unaffected (no cookie = no header added, existing assertions pass).

Form action architecture — correct direction

Moving notification mutations from client-side fetch (requiring manual CSRF injection) to SvelteKit form actions (server-side fetch, CSRF bypassed entirely) is architecturally correct. The aktivitaeten/+page.server.ts actions follow the project's established form action pattern.

One note: NotificationDropdown.svelte hardcodes action="/aktivitaeten?/dismiss-notification". This component lives in the global nav bell and renders on any page. The absolute URL is technically correct — the form always POSTs to the aktivitaeten page server — but it creates an invisible coupling between the component and that route. If the route path ever changes, this breaks silently. A shared constant (NOTIFICATION_BASE_URL) would make the coupling explicit. Not a blocker.

Summary

Three independent fixes (entity graph, permissions, CSRF) are cleanly separated and correctly implemented. The notification bell refactor to SvelteKit form actions is architecturally sound and the right long-term direction.

## 🏗️ Markus Keller — Software Architect **Verdict: ✅ Approved** ### What I checked Entity graph integrity, permission system correctness, CSRF utility placement, form action architecture. --- ### Entity graphs — correct fixes Adding `receivers` to `Document.list` is the right fix: `sortByFirstReceiver()` in `DocumentService` accesses the collection after the JPA session closes (OSIV=false). Adding `trainingLabels` prevents a latent Jackson serialization crash from an uninitialized `@ElementCollection`. A tracking issue has been created to optimize the list graph later — the defensive inclusion is acceptable in the meantime. ### Permission change — follows established pattern Expanding `@RequirePermission(WRITE_ALL)` to `@RequirePermission({ANNOTATE_ALL, WRITE_ALL})` in `TranscriptionBlockController` matches `AnnotationController` and `CommentController`. The permission aspect uses OR semantics. This is correct and consistent. ### CSRF utilities — correct placement `getCsrfToken()`, `withCsrf()`, and `makeCsrfFetch()` in `cookies.ts` is a reasonable location — the file already reads cookies. The `makeCsrfFetch` wrapper preserves the injectable `fetchImpl` contract in hooks, keeping unit tests unaffected (no cookie = no header added, existing assertions pass). ### Form action architecture — correct direction Moving notification mutations from client-side fetch (requiring manual CSRF injection) to SvelteKit form actions (server-side fetch, CSRF bypassed entirely) is architecturally correct. The `aktivitaeten/+page.server.ts` actions follow the project's established form action pattern. **One note:** `NotificationDropdown.svelte` hardcodes `action="/aktivitaeten?/dismiss-notification"`. This component lives in the global nav bell and renders on any page. The absolute URL is technically correct — the form always POSTs to the aktivitaeten page server — but it creates an invisible coupling between the component and that route. If the route path ever changes, this breaks silently. A shared constant (`NOTIFICATION_BASE_URL`) would make the coupling explicit. Not a blocker. ### Summary Three independent fixes (entity graph, permissions, CSRF) are cleanly separated and correctly implemented. The notification bell refactor to SvelteKit form actions is architecturally sound and the right long-term direction.

marcel commented

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

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

What I checked

Code quality, TDD evidence, type safety, component structure, edge cases in the implementation.

Tests are thorough and well-structured

The vi.hoisted(() => ({ type: 'success' as string })) + afterEach(() => { mockFormResult.type = 'success' }) pattern for controlling the enhance mock result is clean and easy to reason about. Error-path tests (mockFormResult.type = 'failure') exist for both dismiss-notification and mark-all-read in both NotificationDropdown and ChronikFuerDichBox.

The aktivitaeten/page.server.spec.ts addition covers: missing input, correct API call, success response, and API failure for each action. Good coverage.

`makeCsrfFetch` — correct composition

The wrapper respects the injectable fetchImpl contract. In tests, getCsrfToken() returns null (no browser cookie in jsdom), so the header is never added and existing test expectations remain valid. The new Headers(init?.headers) approach handles both plain object headers and pre-existing Headers instances correctly.

`optimisticMarkRead/optimisticMarkAllRead` — synchronous is the right call

The old markRead/markAllRead were async fetch calls in the store. Making them synchronous state mutations and delegating the API call to the form action is the correct split. The store no longer needs to know about HTTP — it only manages local state. SSE events still flow into the store for initial population.

Minor: duplicate `$app/forms` mock across four test files

ChronikFuerDichBox.svelte.spec.ts, ChronikFuerDichBox.svelte.test.ts, NotificationDropdown.svelte.test.ts, and NotificationBell.svelte.spec.ts each define identical $app/forms mocks. A shared vitest setup file would remove the duplication. Not a blocker — tech debt worth a follow-up.

Pre-existing gap in `SegmentationTrainingCard` (not introduced here)

startTraining() uses try/finally but has no errorMessage state (unlike OcrTrainingCard). Silent failure on network error. Worth a separate issue — not part of this PR.

Summary

Clean implementation. Tests cover success and failure branches. The synchronous-store + server-action split is the right pattern for this stack.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### What I checked Code quality, TDD evidence, type safety, component structure, edge cases in the implementation. --- ### Tests are thorough and well-structured The `vi.hoisted(() => ({ type: 'success' as string }))` + `afterEach(() => { mockFormResult.type = 'success' })` pattern for controlling the enhance mock result is clean and easy to reason about. Error-path tests (`mockFormResult.type = 'failure'`) exist for both `dismiss-notification` and `mark-all-read` in both `NotificationDropdown` and `ChronikFuerDichBox`. The `aktivitaeten/page.server.spec.ts` addition covers: missing input, correct API call, success response, and API failure for each action. Good coverage. ### `makeCsrfFetch` — correct composition The wrapper respects the injectable `fetchImpl` contract. In tests, `getCsrfToken()` returns `null` (no browser cookie in jsdom), so the header is never added and existing test expectations remain valid. The `new Headers(init?.headers)` approach handles both plain object headers and pre-existing `Headers` instances correctly. ### `optimisticMarkRead/optimisticMarkAllRead` — synchronous is the right call The old `markRead`/`markAllRead` were async fetch calls in the store. Making them synchronous state mutations and delegating the API call to the form action is the correct split. The store no longer needs to know about HTTP — it only manages local state. SSE events still flow into the store for initial population. ### Minor: duplicate `$app/forms` mock across four test files `ChronikFuerDichBox.svelte.spec.ts`, `ChronikFuerDichBox.svelte.test.ts`, `NotificationDropdown.svelte.test.ts`, and `NotificationBell.svelte.spec.ts` each define identical `$app/forms` mocks. A shared vitest setup file would remove the duplication. Not a blocker — tech debt worth a follow-up. ### Pre-existing gap in `SegmentationTrainingCard` (not introduced here) `startTraining()` uses `try/finally` but has no `errorMessage` state (unlike `OcrTrainingCard`). Silent failure on network error. Worth a separate issue — not part of this PR. ### Summary Clean implementation. Tests cover success and failure branches. The synchronous-store + server-action split is the right pattern for this stack.

marcel commented

2026-05-20 20:18:32 +02:00

🔐 Nora "NullX" Steiner — Security Expert

Verdict: ✅ Approved

What I checked

CSRF coverage completeness, permission model correctness, server-side input validation, token extraction implementation.

CSRF coverage — comprehensive

All client-side state-mutating fetches now include X-XSRF-TOKEN:

Location	Method	Status
`useTranscriptionBlocks.svelte.ts` — all mutations	PUT/DELETE via `makeCsrfFetch`	✅
`useBlockAutoSave.svelte.ts` — `flushOnUnload` beacon	PUT (keepalive)	✅
`TranscriptionEditView.svelte` — `reorder`	PUT	✅
`BulkDocumentEditLayout.svelte` — `saveUpload`	POST (multipart)	✅
`OcrTrainingCard.svelte`	POST	✅
`SegmentationTrainingCard.svelte`	POST	✅
Notification mark-read / mark-all	Moved to server-side form actions (no client CSRF needed)	✅

`getCsrfToken()` implementation — correct

The regex /(?:^|;\s*)XSRF-TOKEN=([^;]+)/ is correct: handles both first-cookie and mid-string positions, URL-decodes the value, returns null server-side (no document global). Safe.

Permission change — appropriate scope

{ANNOTATE_ALL, WRITE_ALL} in TranscriptionBlockController matches the established pattern in AnnotationController and CommentController. OR semantics in the aspect. Users with only ANNOTATE_ALL can transcribe but cannot access write-only operations outside their scope.

Input validation — adequate

dismiss-notification action validates that notificationId is a non-empty string before calling the API (typeof raw === 'string' ? raw : null). Empty-string and null are rejected with fail(400). ✅

One item to confirm — backend ownership check

The dismiss-notification action passes the user-supplied notificationId directly to PATCH /api/notifications/{id}/read. There is no ownership check in the SvelteKit action layer. If NotificationService.markRead() does not verify the notification belongs to the authenticated user, any authenticated user could mark another user's notification as read by crafting a POST with an arbitrary UUID.

This is almost certainly enforced in the backend service, but confirming it is worth a quick check in NotificationService. Not a blocker — just a verification step.

Summary

CSRF remediation is thorough and covers all identified client-side mutation sites. The token extraction and header injection are correct. Confirm backend ownership enforcement for the notification mark-read endpoint.

## 🔐 Nora "NullX" Steiner — Security Expert **Verdict: ✅ Approved** ### What I checked CSRF coverage completeness, permission model correctness, server-side input validation, token extraction implementation. --- ### CSRF coverage — comprehensive All client-side state-mutating fetches now include `X-XSRF-TOKEN`: | Location | Method | Status | |---|---|---| | `useTranscriptionBlocks.svelte.ts` — all mutations | PUT/DELETE via `makeCsrfFetch` | ✅ | | `useBlockAutoSave.svelte.ts` — `flushOnUnload` beacon | PUT (keepalive) | ✅ | | `TranscriptionEditView.svelte` — `reorder` | PUT | ✅ | | `BulkDocumentEditLayout.svelte` — `saveUpload` | POST (multipart) | ✅ | | `OcrTrainingCard.svelte` | POST | ✅ | | `SegmentationTrainingCard.svelte` | POST | ✅ | | Notification mark-read / mark-all | Moved to server-side form actions (no client CSRF needed) | ✅ | ### `getCsrfToken()` implementation — correct The regex `/(?:^|;\s*)XSRF-TOKEN=([^;]+)/` is correct: handles both first-cookie and mid-string positions, URL-decodes the value, returns `null` server-side (no `document` global). Safe. ### Permission change — appropriate scope `{ANNOTATE_ALL, WRITE_ALL}` in `TranscriptionBlockController` matches the established pattern in `AnnotationController` and `CommentController`. OR semantics in the aspect. Users with only `ANNOTATE_ALL` can transcribe but cannot access write-only operations outside their scope. ### Input validation — adequate `dismiss-notification` action validates that `notificationId` is a non-empty string before calling the API (`typeof raw === 'string' ? raw : null`). Empty-string and null are rejected with `fail(400)`. ✅ ### One item to confirm — backend ownership check The `dismiss-notification` action passes the user-supplied `notificationId` directly to `PATCH /api/notifications/{id}/read`. There is no ownership check in the SvelteKit action layer. If `NotificationService.markRead()` does not verify the notification belongs to the authenticated user, any authenticated user could mark another user's notification as read by crafting a POST with an arbitrary UUID. This is almost certainly enforced in the backend service, but confirming it is worth a quick check in `NotificationService`. Not a blocker — just a verification step. ### Summary CSRF remediation is thorough and covers all identified client-side mutation sites. The token extraction and header injection are correct. Confirm backend ownership enforcement for the notification mark-read endpoint.

marcel commented

2026-05-20 20:18:47 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ✅ Approved

What I checked

Test completeness, error scenario coverage, mock quality, test isolation, edge cases.

Coverage is solid

notifications.svelte.spec.ts: optimisticMarkRead is tested for state mutation, guard against double-decrement on already-read items, and optimisticMarkAllRead for bulk state reset. All tested without mocking fetch — the functions are synchronous, so tests are simple and reliable.

aktivitaeten/page.server.spec.ts: Both new actions are tested for: missing input → 400, correct API call, success → { success: true }, API failure → fail(status). Good shape.

NotificationDropdown.svelte.test.ts: Tests for form structure (action URL, method, hidden notificationId input), optimisticMarkRead called with ID, success navigation (goto + onClose), failure isolation (no goto/onClose on error), error banner presence, and annotationId deep-link. Very thorough.

ChronikFuerDichBox.svelte.spec.ts: Error banner test added (mockFormResult.type = 'failure' → [role="alert"] appears). ✅

Concern: duplicate test files for `ChronikFuerDichBox`

There are two test files for the same component:

ChronikFuerDichBox.svelte.spec.ts — uses vitest/browser
ChronikFuerDichBox.svelte.test.ts — also uses vitest/browser

Both cover similar scenarios with different assertion styles. The error banner test exists only in .spec.ts, not in .test.ts. Having two files risks coverage gaps and confuses future contributors about which file to add new tests to. Suggest consolidating into one file in a follow-up.

Minor: `mockFormResult.type` typed as `string`

The mock result type field is { type: 'success' as string }. If it were typed as 'success' | 'failure' | 'error', tests that set mockFormResult.type = 'other' would be caught at compile time. Not a blocker.

Missing edge case (low priority)

No test for notificationId being an empty string (as opposed to missing entirely). The action handles !notificationId correctly, but a test would confirm the guard works for empty string too. The current typeof raw === 'string' ? raw : null check does NOT protect against empty string — '' is a string and would pass through to the API call. Worth checking.

Summary

Test coverage is strong across all changed files. The dual test file situation for ChronikFuerDichBox is a maintenance concern. Empty-string notificationId is a minor gap in both the action guard and test coverage.

## 🧪 Sara Holt — QA Engineer **Verdict: ✅ Approved** ### What I checked Test completeness, error scenario coverage, mock quality, test isolation, edge cases. --- ### Coverage is solid **`notifications.svelte.spec.ts`**: `optimisticMarkRead` is tested for state mutation, guard against double-decrement on already-read items, and `optimisticMarkAllRead` for bulk state reset. All tested without mocking fetch — the functions are synchronous, so tests are simple and reliable. **`aktivitaeten/page.server.spec.ts`**: Both new actions are tested for: missing input → 400, correct API call, success → `{ success: true }`, API failure → `fail(status)`. Good shape. **`NotificationDropdown.svelte.test.ts`**: Tests for form structure (action URL, method, hidden `notificationId` input), `optimisticMarkRead` called with ID, success navigation (`goto` + `onClose`), failure isolation (no `goto`/`onClose` on error), error banner presence, and annotationId deep-link. Very thorough. **`ChronikFuerDichBox.svelte.spec.ts`**: Error banner test added (`mockFormResult.type = 'failure'` → `[role="alert"]` appears). ✅ ### Concern: duplicate test files for `ChronikFuerDichBox` There are two test files for the same component: - `ChronikFuerDichBox.svelte.spec.ts` — uses `vitest/browser` - `ChronikFuerDichBox.svelte.test.ts` — also uses `vitest/browser` Both cover similar scenarios with different assertion styles. The error banner test exists only in `.spec.ts`, not in `.test.ts`. Having two files risks coverage gaps and confuses future contributors about which file to add new tests to. Suggest consolidating into one file in a follow-up. ### Minor: `mockFormResult.type` typed as `string` The mock result type field is `{ type: 'success' as string }`. If it were typed as `'success' | 'failure' | 'error'`, tests that set `mockFormResult.type = 'other'` would be caught at compile time. Not a blocker. ### Missing edge case (low priority) No test for `notificationId` being an empty string (as opposed to missing entirely). The action handles `!notificationId` correctly, but a test would confirm the guard works for empty string too. The current `typeof raw === 'string' ? raw : null` check does NOT protect against empty string — `''` is a string and would pass through to the API call. Worth checking. ### Summary Test coverage is strong across all changed files. The dual test file situation for `ChronikFuerDichBox` is a maintenance concern. Empty-string `notificationId` is a minor gap in both the action guard and test coverage.

marcel commented

2026-05-20 20:18:55 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

Infrastructure impact, deployment requirements, CSRF token delivery at the proxy layer, runtime behavior of the form actions.

No infrastructure changes required

This PR is purely application code. No Compose file changes, no CI workflow changes, no database migrations. Flyway is unaffected — entity graph annotations are compile-time metadata, not schema changes.

CSRF token delivery — verify Caddy config

Spring Security's CookieCsrfTokenRepository sets XSRF-TOKEN as a response cookie. For the client-side getCsrfToken() implementation to work, Caddy must not strip this cookie before it reaches the browser.

One-time check: confirm the production Caddyfile has no header directive that removes XSRF-TOKEN or matches Set-Cookie headers containing it. If Caddy is configured to strip cookies for caching or privacy reasons, this breaks silently and every client-side mutation returns 403 again.

Form actions → no new routing rules needed

action="/aktivitaeten?/dismiss-notification" and action="/aktivitaeten?/mark-all-read" are SvelteKit server-side form actions. They route through the Node adapter to the SvelteKit server process. The existing reverse_proxy directive in Caddy handles them — no additional rules needed.

No new npm dependencies

getCsrfToken(), withCsrf(), makeCsrfFetch() are pure TypeScript using the standard Web Fetch API. Zero new packages.

Summary

No operational changes needed. Single verification item: confirm Caddy does not strip the XSRF-TOKEN cookie in the production Caddyfile.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked Infrastructure impact, deployment requirements, CSRF token delivery at the proxy layer, runtime behavior of the form actions. --- ### No infrastructure changes required This PR is purely application code. No Compose file changes, no CI workflow changes, no database migrations. Flyway is unaffected — entity graph annotations are compile-time metadata, not schema changes. ### CSRF token delivery — verify Caddy config Spring Security's `CookieCsrfTokenRepository` sets `XSRF-TOKEN` as a response cookie. For the client-side `getCsrfToken()` implementation to work, Caddy must not strip this cookie before it reaches the browser. One-time check: confirm the production Caddyfile has no `header` directive that removes `XSRF-TOKEN` or matches `Set-Cookie` headers containing it. If Caddy is configured to strip cookies for caching or privacy reasons, this breaks silently and every client-side mutation returns 403 again. ### Form actions → no new routing rules needed `action="/aktivitaeten?/dismiss-notification"` and `action="/aktivitaeten?/mark-all-read"` are SvelteKit server-side form actions. They route through the Node adapter to the SvelteKit server process. The existing `reverse_proxy` directive in Caddy handles them — no additional rules needed. ### No new npm dependencies `getCsrfToken()`, `withCsrf()`, `makeCsrfFetch()` are pure TypeScript using the standard Web Fetch API. Zero new packages. ### Summary No operational changes needed. Single verification item: confirm Caddy does not strip the `XSRF-TOKEN` cookie in the production Caddyfile.

marcel commented

2026-05-20 20:19:13 +02:00

🎨 Leonie Voss — UI/UX Design Lead

Verdict: ⚠️ Approved with concerns

What I checked

Error banner accessibility, touch targets, brand compliance, senior user experience (60+ primary audience).

Concern 1 — Error banner has no dismiss button (Medium priority)

Both NotificationDropdown.svelte and ChronikFuerDichBox.svelte render this error banner:

<p role="alert" class="px-4 py-2 text-sm text-red-600">{errorMessage}</p>

role="alert" is correct — screen readers announce it immediately. But there is no manual dismiss button. The message disappears only when the user triggers the next action (errorMessage = null at the top of the next use:enhance callback). For a senior user who sees "Aktion fehlgeschlagen. Bitte versuche es erneut." with no X button, this is confusing — they see an error but don't know how to clear it.

Suggested fix:

{#if errorMessage}
  <p role="alert" class="flex items-center gap-2 px-4 py-2 text-sm text-red-600">
    {errorMessage}
    <button
      type="button"
      class="ml-auto rounded-sm p-1 hover:bg-muted focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-none"
      aria-label={m.comp_dismiss()}
      onclick={() => (errorMessage = null)}
    >×</button>
  </p>
{/if}

Concern 2 — FuerDich dismiss button touch target is too small (Medium priority)

The per-item dismiss button in ChronikFuerDichBox:

class="mt-0.5 shrink-0 rounded-sm p-1 ..."

p-1 = 4px all sides. The icon is h-4 w-4 (16px). Total clickable area: ~24×24px. This is below the 44px minimum required for senior users and WCAG 2.2. At minimum use p-2 (8px padding → ~32×32px); ideally p-3 for 40px.

Touch targets — notification list rows are acceptable

NotificationDropdown notification row button: px-4 py-3.5 (14px top/bottom) + content height easily reaches 44px. ✅

Brand compliance

text-red-600 on bg-surface: ~4.5:1 contrast ratio — meets WCAG AA minimum for body text. ✅
text-sm (14px) for error text: borderline for seniors (prefer 16px), but consistent with existing error text patterns in the app. Acceptable.
form class="contents" for the form wrapper around notification rows: correct pattern, no layout disruption. ✅

No loading state during form action

When a user clicks a notification in the dropdown, there is a brief network round-trip before navigation. No loading indicator is shown during this time. For seniors on slow connections, the delayed navigation could feel like a broken button. A disabled attribute on the submit button during the enhance callback would help.

Summary

The error banner's role="alert" is correct, but the missing dismiss button and the too-small FuerDich dismiss touch target are usability issues for the senior audience. Neither is a hard blocker, but both should be addressed before this reaches the primary transcriber audience.

## 🎨 Leonie Voss — UI/UX Design Lead **Verdict: ⚠️ Approved with concerns** ### What I checked Error banner accessibility, touch targets, brand compliance, senior user experience (60+ primary audience). --- ### Concern 1 — Error banner has no dismiss button (Medium priority) Both `NotificationDropdown.svelte` and `ChronikFuerDichBox.svelte` render this error banner: ```svelte <p role="alert" class="px-4 py-2 text-sm text-red-600">{errorMessage}</p> ``` `role="alert"` is correct — screen readers announce it immediately. But there is **no manual dismiss button**. The message disappears only when the user triggers the next action (`errorMessage = null` at the top of the next `use:enhance` callback). For a senior user who sees "Aktion fehlgeschlagen. Bitte versuche es erneut." with no X button, this is confusing — they see an error but don't know how to clear it. **Suggested fix:** ```svelte {#if errorMessage} <p role="alert" class="flex items-center gap-2 px-4 py-2 text-sm text-red-600"> {errorMessage} <button type="button" class="ml-auto rounded-sm p-1 hover:bg-muted focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-none" aria-label={m.comp_dismiss()} onclick={() => (errorMessage = null)} >×</button> </p> {/if} ``` ### Concern 2 — FuerDich dismiss button touch target is too small (Medium priority) The per-item dismiss button in `ChronikFuerDichBox`: ```svelte class="mt-0.5 shrink-0 rounded-sm p-1 ..." ``` `p-1` = 4px all sides. The icon is `h-4 w-4` (16px). Total clickable area: ~24×24px. This is **below the 44px minimum** required for senior users and WCAG 2.2. At minimum use `p-2` (8px padding → ~32×32px); ideally `p-3` for 40px. ### Touch targets — notification list rows are acceptable `NotificationDropdown` notification row button: `px-4 py-3.5` (14px top/bottom) + content height easily reaches 44px. ✅ ### Brand compliance - `text-red-600` on `bg-surface`: ~4.5:1 contrast ratio — meets WCAG AA minimum for body text. ✅ - `text-sm` (14px) for error text: borderline for seniors (prefer 16px), but consistent with existing error text patterns in the app. Acceptable. - `form class="contents"` for the form wrapper around notification rows: correct pattern, no layout disruption. ✅ ### No loading state during form action When a user clicks a notification in the dropdown, there is a brief network round-trip before navigation. No loading indicator is shown during this time. For seniors on slow connections, the delayed navigation could feel like a broken button. A `disabled` attribute on the submit button during the enhance callback would help. ### Summary The error banner's `role="alert"` is correct, but the missing dismiss button and the too-small FuerDich dismiss touch target are usability issues for the senior audience. Neither is a hard blocker, but both should be addressed before this reaches the primary transcriber audience.

marcel commented

2026-05-20 20:19:33 +02:00

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

What I checked

Requirements traceability, bug fix completeness, user-facing behavior changes, permission model semantics.

Bug #1 — LazyInitializationException in the list view ✅

Root cause: Document.list entity graph was missing receivers, which sortByFirstReceiver() accesses after the JPA session closes (OSIV=false). Fix correctly adds receivers to Document.list. Traceable, complete.

Bug #2 — 403 on transcription block saves ✅

Two independent root causes, both addressed:

CSRF headers missing on 6+ client-side mutation sites → fixed comprehensively via withCsrf/makeCsrfFetch utilities
ANNOTATE_ALL permission not accepted by TranscriptionBlockController → fixed to match the established pattern

Both fixes are correctly targeted at root causes, not symptoms.

Behavior change: notification bell interaction model

This is the most significant user-facing change in the PR. The interaction model changes from:

Before: Click notification → immediate client-side PATCH → navigate away

After: Click notification → optimistic state update → form action fires → on success: navigate; on failure: show error banner, stay on page

This is a meaningful UX change. Two questions for the product owner:

Is the user expected to retry manually after a failure? The error message says "Bitte versuche es erneut" but the notification item has already been optimistically removed from the visible list (via optimisticMarkRead). The user has no obvious item to click again to retry. What is the intended retry path?
Is there a loading state between click and navigation? On slow connections (relevant for the senior audience), the form action takes time. Currently nothing indicates the click was registered. A brief disabled state or spinner on the notification row during submission would communicate that the action is in progress.

`ANNOTATE_ALL` permission for transcription blocks — semantic ambiguity

The ANNOTATE_ALL permission is used in three controllers: AnnotationController, CommentController, and now TranscriptionBlockController. The permission name suggests annotation-specific access (PDF region overlays), but it now also grants the ability to create, update, delete, reorder, and review transcription text blocks.

This works correctly because of OR semantics. However, a user reading "this user has ANNOTATE_ALL permission" cannot infer they can also modify transcription blocks. If ANNOTATE_ALL scope ever needs to be narrowed (e.g., to read-only PDF annotations), transcription block access would be inadvertently revoked.

This is a pre-existing architecture decision (matching AnnotationController/CommentController). A future TRANSCRIBE_ALL permission would be cleaner and would decouple annotation access from transcription access. Worth a backlog issue.

`trainingLabels` in `Document.list` — requirement gap

No user-facing requirement exists for training labels in the document list view. They are included in Document.list defensively to prevent a latent Jackson serialization crash. A tracking issue exists. The current state is acceptable as a temporary measure. The optimization issue should capture the requirement: "document list view does not need training labels; remove from list entity graph once a safe decoupling strategy is identified."

Summary

Both production bugs are correctly fixed. The notification bell behavior change raises two open questions (retry path after optimistic removal, loading state). The ANNOTATE_ALL semantic mismatch for transcription is a pre-existing design debt worth a backlog issue.

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked Requirements traceability, bug fix completeness, user-facing behavior changes, permission model semantics. --- ### Bug #1 — LazyInitializationException in the list view ✅ Root cause: `Document.list` entity graph was missing `receivers`, which `sortByFirstReceiver()` accesses after the JPA session closes (OSIV=false). Fix correctly adds `receivers` to `Document.list`. Traceable, complete. ### Bug #2 — 403 on transcription block saves ✅ Two independent root causes, both addressed: 1. CSRF headers missing on 6+ client-side mutation sites → fixed comprehensively via `withCsrf`/`makeCsrfFetch` utilities 2. `ANNOTATE_ALL` permission not accepted by `TranscriptionBlockController` → fixed to match the established pattern Both fixes are correctly targeted at root causes, not symptoms. ### Behavior change: notification bell interaction model This is the most significant user-facing change in the PR. The interaction model changes from: **Before:** Click notification → immediate client-side PATCH → navigate away **After:** Click notification → optimistic state update → form action fires → on success: navigate; on failure: show error banner, stay on page This is a meaningful UX change. Two questions for the product owner: 1. **Is the user expected to retry manually after a failure?** The error message says "Bitte versuche es erneut" but the notification item has already been optimistically removed from the visible list (via `optimisticMarkRead`). The user has no obvious item to click again to retry. What is the intended retry path? 2. **Is there a loading state between click and navigation?** On slow connections (relevant for the senior audience), the form action takes time. Currently nothing indicates the click was registered. A brief disabled state or spinner on the notification row during submission would communicate that the action is in progress. ### `ANNOTATE_ALL` permission for transcription blocks — semantic ambiguity The `ANNOTATE_ALL` permission is used in three controllers: `AnnotationController`, `CommentController`, and now `TranscriptionBlockController`. The permission name suggests annotation-specific access (PDF region overlays), but it now also grants the ability to create, update, delete, reorder, and review transcription text blocks. This works correctly because of OR semantics. However, a user reading "this user has ANNOTATE_ALL permission" cannot infer they can also modify transcription blocks. If `ANNOTATE_ALL` scope ever needs to be narrowed (e.g., to read-only PDF annotations), transcription block access would be inadvertently revoked. This is a pre-existing architecture decision (matching `AnnotationController`/`CommentController`). A future `TRANSCRIBE_ALL` permission would be cleaner and would decouple annotation access from transcription access. Worth a backlog issue. ### `trainingLabels` in `Document.list` — requirement gap No user-facing requirement exists for training labels in the document list view. They are included in `Document.list` defensively to prevent a latent Jackson serialization crash. A tracking issue exists. The current state is acceptable as a temporary measure. The optimization issue should capture the requirement: "document list view does not need training labels; remove from list entity graph once a safe decoupling strategy is identified." ### Summary Both production bugs are correctly fixed. The notification bell behavior change raises two open questions (retry path after optimistic removal, loading state). The `ANNOTATE_ALL` semantic mismatch for transcription is a pre-existing design debt worth a backlog issue.

marcel changed target branch from main to feat/issue-380-decouple-mention-search

2026-05-20 20:34:34 +02:00

marcel changed target branch from feat/issue-380-decouple-mention-search to feat/issue-286-notification-bell-form-actions

2026-05-20 20:34:48 +02:00

marcel changed target branch from feat/issue-286-notification-bell-form-actions to main

2026-05-20 20:35:43 +02:00

marcel added 14 commits 2026-05-20 20:35:53 +02:00

feat(notification): add dismiss-notification and mark-all-read form actions to aktivitaeten 85c13b3d46

Adds two SvelteKit form actions to /aktivitaeten/+page.server.ts so the
notification bell can POST there instead of calling the backend directly
from the browser.

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

refactor(notification): rename markRead/markAllRead to optimistic helpers without fetch cdd5bfa318

Removes raw fetch() calls from the store. optimisticMarkRead(id) and
optimisticMarkAllRead() now only mutate local $state — the actual API
calls move to SvelteKit form actions on /aktivitaeten.

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

refactor(notification): replace callback props with form actions in Dropdown and Bell 3d3c111c2b

NotificationDropdown now wraps each row in a <form action="/aktivitaeten?/dismiss-notification">
and the mark-all control in <form action="/aktivitaeten?/mark-all-read">, wired via use:enhance
for optimistic UI. Props renamed onMarkRead/onMarkAllRead → optimisticMarkRead/optimisticMarkAllRead
to match the simplified store API. NotificationBell passes the store helpers directly; handleMarkRead
is removed.

Test mocks updated: $app/forms enhance mock fires SubmitFunction synchronously on form submit so
callback assertions work without a real HTTP round-trip.

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

refactor(chronik): replace callback props with form actions in ChronikFuerDichBox 6f862243fd

Dismiss (X) button and mark-all-read button now submit forms to
/aktivitaeten?/dismiss-notification and /aktivitaeten?/mark-all-read respectively.
Props renamed onMarkRead/onMarkAllRead → optimisticMarkRead/optimisticMarkAllRead.

aktivitaeten/+page.svelte drops the now-deleted onMarkRead/onMarkAllRead wrapper functions
and passes notificationStore.optimisticMarkRead/optimisticMarkAllRead directly to the box.

Tests: $app/forms enhance mock added to both spec files so dismiss and mark-all assertions
work synchronously against form-submit events.

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

fix(notifications): guard against null notificationId in dismiss action 261cbbd867

Casting null to string caused PATCH to fire against /api/notifications/null/read
when the field was absent. Added an early-return fail(400) and a test that
submitting an empty form returns 400 without calling the API.

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

fix(notifications): move onClose/goto into enhance result callback dbf74cb91a

onClose() and goto() were firing before the server responded, making it
impossible for a fail() response to cancel navigation. Moved them inside
the result callback behind a result.type !== 'failure' guard.

Updated the $app/forms enhance mock to always invoke the returned async
callback with a configurable mockFormResult, and added three tests:
- success path calls onClose + goto with the correct deep-link URL
- failure path skips onClose and goto
- annotationId is appended to the URL when present

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

fix(notifications): surface action failures as an error banner 30efb54aac

When dismiss-notification or mark-all-read returns a failure the dropdown
now shows a localised error message above the list. Added
notification_error_generic key (de/en/es) as the fallback when the
action response carries no explicit error string.

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

fix(notification-dropdown): handle error result type, add role=alert, fix update ordering 978a2b3cdb

- Add role="alert" to error banner so screen-reader users hear failures
- Handle result.type === 'error' (network failure) alongside 'failure' in both enhance callbacks
- Clear errorMessage at the start of each submit so stale errors don't persist on retry
- On dismiss success: skip update() entirely since goto() navigates away from the page
- On dismiss failure: await update() then set error message
- On mark-all success: skip update() (optimistic state already applied)

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

fix(aktivitaeten): narrow File cast and use null payload for missing notificationId 35fbaf8154

Replace 'as string | null' cast (which silently accepts File values) with an explicit
typeof check. Use error: null instead of hardcoded German so the client falls through
to the generic i18n-keyed error banner.

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

fix(chronik): surface action failures in ChronikFuerDichBox with accessible error banner 728f9cd1b0

Add $state errorMessage + role=alert banner to ChronikFuerDichBox. Both enhance callbacks
now inspect result.type and set the error message on 'failure' or 'error'; errorMessage
is cleared on each new submit attempt.

Upgrade both test files to the mockFormResult pattern (via vi.hoisted) so the result
callback is exercised. Add a failing-action test in each file that asserts role=alert
appears after a form submit with type='failure'.

Fix bare Function cast → explicit typed cast to satisfy @typescript-eslint/no-unsafe-function-type.

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

fix(notification): address review suggestions 392097287c

- ChronikFuerDichBox: move update() inside the failure branch so success
  path skips it, matching NotificationDropdown's pattern
- NotificationDropdown test: add role=alert assertion for mark-all-read
  failure to match existing dismiss-failure coverage in ChronikFuerDichBox
- +page.server.ts: use getErrorMessage(undefined) instead of null so the
  missing-notificationId 400 goes through the same i18n pipeline as other errors

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

fix(document): add receivers+trainingLabels to Document.list entity graph 7b282f699d

Document.list was missing receivers (caused LazyInitializationException
when sorting by receiver) and trainingLabels (latent crash for any
document with OCR training labels assigned). Document.full was missing
trainingLabels for the same reason. OSIV is disabled so every lazy
association used after the transaction closes must be in the graph.

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

fix(transcription): allow ANNOTATE_ALL on block write endpoints 909f960b2e

TranscriptionBlockController required WRITE_ALL exclusively, blocking
users with only ANNOTATE_ALL from saving, reviewing, or deleting blocks.
All write endpoints now accept {ANNOTATE_ALL, WRITE_ALL}, matching the
pattern already established in AnnotationController and CommentController.

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

fix(csrf): send X-XSRF-TOKEN on all client-side mutating fetch calls

CI / fail2ban Regex (pull_request) Has been cancelled

Details

CI / Semgrep Security Scan (pull_request) Has been cancelled

Details

CI / Compose Bucket Idempotency (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (pull_request) Has been cancelled

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

CI / OCR Service Tests (push) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / fail2ban Regex (push) Has been cancelled

Details

CI / Semgrep Security Scan (push) Has been cancelled

Details

CI / Compose Bucket Idempotency (push) Has been cancelled

Details

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

Details

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

Details

19e2f65a21

hooks.server.ts already forwards the CSRF token for server-side fetch
(form actions, load). Client-side XHR calls bypassed it, causing Spring
Security to return 403 before PermissionAspect even ran.

Adds getCsrfToken/withCsrf/makeCsrfFetch to cookies.ts.
useTranscriptionBlocks wraps its injectable fetchImpl with makeCsrfFetch
(covers all block mutations and saveBlockWithConflictRetry).
useBlockAutoSave, TranscriptionEditView, BulkDocumentEditLayout,
OcrTrainingCard, and SegmentationTrainingCard apply withCsrf inline.

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

marcel force-pushed worktree-feat+issue-286-notification-bell-form-actions from 902f855bd0 to 19e2f65a21

2026-05-20 20:35:53 +02:00

Compare

marcel merged commit 19e2f65a21 into main

2026-05-20 20:36:16 +02:00

marcel deleted branch worktree-feat+issue-286-notification-bell-form-actions

2026-05-20 20:36:17 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#648

fix: entity graph gaps, ANNOTATE_ALL on transcription blocks, CSRF on client fetch #648

Summary

1. Document.list entity graph missing receivers and trainingLabels

2. TranscriptionBlockController blocked ANNOTATE_ALL users with 403

3. Client-side fetch calls missing X-XSRF-TOKEN header

Test plan

🏗️ Markus Keller — Software Architect

What I checked

Entity graphs — correct fixes

Permission change — follows established pattern

CSRF utilities — correct placement

Form action architecture — correct direction

Summary

👨‍💻 Felix Brandt — Senior Fullstack Developer

What I checked

Tests are thorough and well-structured

makeCsrfFetch — correct composition

optimisticMarkRead/optimisticMarkAllRead — synchronous is the right call

Minor: duplicate $app/forms mock across four test files

Pre-existing gap in SegmentationTrainingCard (not introduced here)

Summary

🔐 Nora "NullX" Steiner — Security Expert

What I checked

CSRF coverage — comprehensive

getCsrfToken() implementation — correct

Permission change — appropriate scope

Input validation — adequate

One item to confirm — backend ownership check

Summary

🧪 Sara Holt — QA Engineer

What I checked

Coverage is solid

Concern: duplicate test files for ChronikFuerDichBox

Minor: mockFormResult.type typed as string

Missing edge case (low priority)

Summary

⚙️ Tobias Wendt — DevOps & Platform Engineer

What I checked

No infrastructure changes required

CSRF token delivery — verify Caddy config

Form actions → no new routing rules needed

No new npm dependencies

Summary

🎨 Leonie Voss — UI/UX Design Lead

What I checked

Concern 1 — Error banner has no dismiss button (Medium priority)

Concern 2 — FuerDich dismiss button touch target is too small (Medium priority)

Touch targets — notification list rows are acceptable

Brand compliance

No loading state during form action

Summary

📋 Elicit — Requirements Engineer

What I checked

Bug #1 — LazyInitializationException in the list view ✅

Bug #2 — 403 on transcription block saves ✅

Behavior change: notification bell interaction model

ANNOTATE_ALL permission for transcription blocks — semantic ambiguity

trainingLabels in Document.list — requirement gap

Summary

1. `Document.list` entity graph missing `receivers` and `trainingLabels`

2. `TranscriptionBlockController` blocked `ANNOTATE_ALL` users with 403

3. Client-side fetch calls missing `X-XSRF-TOKEN` header

`makeCsrfFetch` — correct composition

`optimisticMarkRead/optimisticMarkAllRead` — synchronous is the right call

Minor: duplicate `$app/forms` mock across four test files

Pre-existing gap in `SegmentationTrainingCard` (not introduced here)

`getCsrfToken()` implementation — correct

Concern: duplicate test files for `ChronikFuerDichBox`

Minor: `mockFormResult.type` typed as `string`

`ANNOTATE_ALL` permission for transcription blocks — semantic ambiguity

`trainingLabels` in `Document.list` — requirement gap