refactor(notification): use SvelteKit form actions instead of raw fetch #630

marcel · 2026-05-19T23:26:59+02:00

marcel commented

2026-05-19 23:26:59 +02:00

$(cat <<'EOF'
Closes #286

Summary

Adds dismiss-notification and mark-all-read form actions to aktivitaeten/+page.server.ts so notification writes go through SvelteKit instead of raw fetch()
Renames store methods markRead/markAllRead → optimisticMarkRead(id)/optimisticMarkAllRead() — pure local state mutations, no network calls
Rewrites NotificationDropdown.svelte and ChronikFuerDichBox.svelte to use <form method="POST" use:enhance> for dismiss and mark-all-read; JS-off degradation now works via native form POST
Removes handleMarkRead wrapper from NotificationBell.svelte; aktivitaeten/+page.svelte passes store helpers directly

Test plan

Server action tests: npx vitest run src/routes/aktivitaeten/page.server.spec.ts — 6 new tests (dismiss + mark-all, happy path + API error + missing field)
Store unit tests: notifications.svelte.spec.ts — renamed helper tests confirm no fetch calls fire
Component tests: NotificationDropdown.svelte.test.ts, NotificationBell.svelte.spec.ts, ChronikFuerDichBox.svelte.spec.ts, ChronikFuerDichBox.svelte.test.ts — all updated for form-based API
CI green on full browser suite
Manual: open bell → click a notification row → unread dot disappears, navigates to comment
Manual: open bell → "Alle gelesen" → badge clears
Manual: Chronik page → dismiss (X) → row disappears; mark-all → all dots clear

🤖 Generated with Claude Code
EOF
)

$(cat <<'EOF' Closes #286 ## Summary - Adds `dismiss-notification` and `mark-all-read` form actions to `aktivitaeten/+page.server.ts` so notification writes go through SvelteKit instead of raw `fetch()` - Renames store methods `markRead`/`markAllRead` → `optimisticMarkRead(id)`/`optimisticMarkAllRead()` — pure local state mutations, no network calls - Rewrites `NotificationDropdown.svelte` and `ChronikFuerDichBox.svelte` to use `<form method="POST" use:enhance>` for dismiss and mark-all-read; JS-off degradation now works via native form POST - Removes `handleMarkRead` wrapper from `NotificationBell.svelte`; `aktivitaeten/+page.svelte` passes store helpers directly ## Test plan - [ ] Server action tests: `npx vitest run src/routes/aktivitaeten/page.server.spec.ts` — 6 new tests (dismiss + mark-all, happy path + API error + missing field) - [ ] Store unit tests: `notifications.svelte.spec.ts` — renamed helper tests confirm no fetch calls fire - [ ] Component tests: `NotificationDropdown.svelte.test.ts`, `NotificationBell.svelte.spec.ts`, `ChronikFuerDichBox.svelte.spec.ts`, `ChronikFuerDichBox.svelte.test.ts` — all updated for form-based API - [ ] CI green on full browser suite - [ ] Manual: open bell → click a notification row → unread dot disappears, navigates to comment - [ ] Manual: open bell → "Alle gelesen" → badge clears - [ ] Manual: Chronik page → dismiss (X) → row disappears; mark-all → all dots clear 🤖 Generated with [Claude Code](https://claude.com/claude-code) EOF )

marcel added 4 commits 2026-05-19 23:27:00 +02:00

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

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 c0a7408ef4

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 2c6b59d0c7

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

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

Details

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

Details

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

Details

CI / fail2ban Regex (pull_request) Successful in 46s

Details

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

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 58s

Details

23439e581a

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>

marcel commented

2026-05-19 23:28:58 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Good refactor. The split between server-side HTTP calls and client-side optimistic state is clean, the naming is mostly improved, and the use:enhance pattern matches the project standard. A few things need attention.

Blockers

1. SubmitFunction fires before the POST — optimistic update runs on network failure too

In NotificationDropdown.svelte (lines 85–97) and ChronikFuerDichBox.svelte, the use:enhance callback runs synchronously on submit, unconditionally calling optimisticMarkRead(notification.id), onClose(), and goto(...) before the server responds. If the action returns fail(), the optimistic mark-read has already fired, the dropdown is already closed, and the user has already been navigated away. The server-side state is out of sync with what the UI shows, with no recovery path and no error surfaced to the user.

The correct sequence for a dismiss action that navigates is:

use:enhance={() => {
    // Fire optimistic update immediately — intentional UX trade-off
    optimisticMarkRead(notification.id);
    return async ({ result, update }) => {
        if (result.type === 'failure') {
            // Roll back: un-mark the notification
            // OR show a toast with the error
        }
        onClose();
        goto(buildCommentHref(...));
        await update({ reset: false, invalidateAll: false });
    };
}}

Right now onClose() and goto() are inside the SubmitFunction body, not the callback — they execute before the server responds, making it impossible to handle failures. This is the key correctness issue in the whole PR.

2. Missing notificationId validation in the dismiss-notification action

+page.server.ts line 74: const notificationId = data.get('notificationId') as string;

formData.get() returns FormDataEntryValue | null. The as string cast bypasses TypeScript's null check. If the field is missing or empty, notificationId will be null cast to string, and createApiClient(fetch).PATCH('/api/notifications/{id}/read', { params: { path: { id: null } } }) will make a request to /api/notifications/null/read. The backend will return 404 or 400, which is handled — but the frontend should reject missing IDs before making the network call:

const notificationId = data.get('notificationId');
if (!notificationId || typeof notificationId !== 'string') {
    return fail(400, { error: 'Missing notificationId' });
}

Suggestions

3. use:enhance mock doesn't simulate the async callback — update() never runs

The mock in all five test files (ChronikFuerDichBox.svelte.spec.ts, NotificationDropdown.svelte.test.ts, etc.):

enhance(node: HTMLFormElement, submit?: (opts: { formData: FormData }) => unknown) {
    const handler = (e: Event) => {
        e.preventDefault();
        submit?.({ formData: new FormData(node) } as never);
    };
    node.addEventListener('submit', handler);
    return { destroy: () => node.removeEventListener('submit', handler) };
}

The real enhance passes a SubmitFunction that returns a callback ({ result, update }) => Promise<void>. The mock calls the outer function synchronously but never calls the returned callback, so update({ reset: false }) never executes in tests. This means the mock doesn't verify the async path. It's acceptable for testing the optimistic call, but it means there are no tests proving the update() branch works correctly — particularly relevant given blocker #1.

4. fail() response is never displayed to the user

Both actions can return fail(status, { error: string }), but no component reads $page.form or form.error to display the message. The user gets silent failure — the optimistic update fires, the dropdown closes, and if the server returned an error, nothing tells the user. Even a simple error toast or reverting the optimistic state would be sufficient.

5. Prop naming — optimisticMarkRead / optimisticMarkAllRead are accurate but verbose

The names clearly signal client-only intent, which is good. A minor concern: they're now passed as props through two component layers (Bell → Dropdown, Chronik Page → ChronikFuerDichBox). Consider whether a callback with a clearer action-oriented name (onDismiss, onDismissAll) would read more naturally at the call site, with the "optimistic" implementation detail staying internal to the store. That said, the current naming is not wrong — it's a readability trade-off.

6. Dead code: deleted handleMarkRead tests in NotificationBell.svelte.spec.ts

The two tests that verified navigation with/without annotationId were deleted rather than migrated. The navigation logic now lives inside NotificationDropdown.svelte's use:enhance callback (line 88–94). There is no test asserting that clicking a notification row navigates to buildCommentHref(documentId, referenceId, annotationId). That's a coverage gap on a user-visible behavior.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** Good refactor. The split between server-side HTTP calls and client-side optimistic state is clean, the naming is mostly improved, and the `use:enhance` pattern matches the project standard. A few things need attention. --- ### Blockers **1. `SubmitFunction` fires before the POST — optimistic update runs on network failure too** In `NotificationDropdown.svelte` (lines 85–97) and `ChronikFuerDichBox.svelte`, the `use:enhance` callback runs synchronously on submit, unconditionally calling `optimisticMarkRead(notification.id)`, `onClose()`, and `goto(...)` *before* the server responds. If the action returns `fail()`, the optimistic mark-read has already fired, the dropdown is already closed, and the user has already been navigated away. The server-side state is out of sync with what the UI shows, with no recovery path and no error surfaced to the user. The correct sequence for a dismiss action that navigates is: ```ts use:enhance={() => { // Fire optimistic update immediately — intentional UX trade-off optimisticMarkRead(notification.id); return async ({ result, update }) => { if (result.type === 'failure') { // Roll back: un-mark the notification // OR show a toast with the error } onClose(); goto(buildCommentHref(...)); await update({ reset: false, invalidateAll: false }); }; }} ``` Right now `onClose()` and `goto()` are inside the SubmitFunction body, not the callback — they execute before the server responds, making it impossible to handle failures. This is the key correctness issue in the whole PR. **2. Missing `notificationId` validation in the `dismiss-notification` action** `+page.server.ts` line 74: `const notificationId = data.get('notificationId') as string;` `formData.get()` returns `FormDataEntryValue | null`. The `as string` cast bypasses TypeScript's null check. If the field is missing or empty, `notificationId` will be `null` cast to `string`, and `createApiClient(fetch).PATCH('/api/notifications/{id}/read', { params: { path: { id: null } } })` will make a request to `/api/notifications/null/read`. The backend will return 404 or 400, which is handled — but the frontend should reject missing IDs before making the network call: ```typescript const notificationId = data.get('notificationId'); if (!notificationId || typeof notificationId !== 'string') { return fail(400, { error: 'Missing notificationId' }); } ``` --- ### Suggestions **3. `use:enhance` mock doesn't simulate the async callback — `update()` never runs** The mock in all five test files (`ChronikFuerDichBox.svelte.spec.ts`, `NotificationDropdown.svelte.test.ts`, etc.): ```ts enhance(node: HTMLFormElement, submit?: (opts: { formData: FormData }) => unknown) { const handler = (e: Event) => { e.preventDefault(); submit?.({ formData: new FormData(node) } as never); }; node.addEventListener('submit', handler); return { destroy: () => node.removeEventListener('submit', handler) }; } ``` The real `enhance` passes a `SubmitFunction` that returns a callback `({ result, update }) => Promise<void>`. The mock calls the outer function synchronously but never calls the returned callback, so `update({ reset: false })` never executes in tests. This means the mock doesn't verify the async path. It's acceptable for testing the optimistic call, but it means there are no tests proving the `update()` branch works correctly — particularly relevant given blocker #1. **4. `fail()` response is never displayed to the user** Both actions can return `fail(status, { error: string })`, but no component reads `$page.form` or `form.error` to display the message. The user gets silent failure — the optimistic update fires, the dropdown closes, and if the server returned an error, nothing tells the user. Even a simple error toast or reverting the optimistic state would be sufficient. **5. Prop naming — `optimisticMarkRead` / `optimisticMarkAllRead` are accurate but verbose** The names clearly signal client-only intent, which is good. A minor concern: they're now passed as props through two component layers (Bell → Dropdown, Chronik Page → ChronikFuerDichBox). Consider whether a callback with a clearer action-oriented name (`onDismiss`, `onDismissAll`) would read more naturally at the call site, with the "optimistic" implementation detail staying internal to the store. That said, the current naming is not wrong — it's a readability trade-off. **6. Dead code: deleted `handleMarkRead` tests in `NotificationBell.svelte.spec.ts`** The two tests that verified navigation with/without `annotationId` were deleted rather than migrated. The navigation logic now lives inside `NotificationDropdown.svelte`'s `use:enhance` callback (line 88–94). There is no test asserting that clicking a notification row navigates to `buildCommentHref(documentId, referenceId, annotationId)`. That's a coverage gap on a user-visible behavior.

marcel commented

2026-05-19 23:29:19 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

This PR is primarily a frontend refactor, so the attack surface is small. The backend IDOR protection is unchanged and the session-based auth cookie forwarding through SvelteKit's fetch is correct. A few things to flag.

Blockers

1. Unvalidated form input reaches the API path parameter

+page.server.ts line 74:

const notificationId = data.get('notificationId') as string;

formData.get() returns FormDataEntryValue | null. The as string cast removes the null guard. An attacker with JS disabled who manually crafts a form POST to /aktivitaeten?/dismiss-notification without a notificationId field will cause this to be null cast to string, which then gets interpolated into the API path: /api/notifications/null/read. The backend will 404 or 400 (so no exploitation here), but the missing guard is a code smell that can mask worse bugs in similar actions added later.

Fix:

const notificationId = data.get('notificationId');
if (!notificationId || typeof notificationId !== 'string' || notificationId.trim() === '') {
    return fail(400, { error: 'Missing notificationId' });
}

Security note on IDOR: The backend PATCH /api/notifications/{id}/read enforces ownership server-side (authenticated session + notification belongs to calling user), so there is no IDOR vulnerability introduced here. The notificationId comes from the store, not from user input, under JS-on flow. Under JS-off flow it comes from the hidden input, but the backend validates ownership either way. This is correctly handled.

Suggestions

2. The notificationId in the hidden <input> is visible in the DOM

<input type="hidden" name="notificationId" value={notification.id} />

type="hidden" does not prevent a user from viewing or modifying the value via browser DevTools. Under JS-on flow with use:enhance, this is harmless because the backend validates ownership. Under JS-off flow (native form POST), the same backend ownership check applies. This is not a vulnerability, but it's worth noting for completeness: the notification.id (a UUID) is exposed in the HTML source. That's acceptable given UUIDs are not a secret, but teams sometimes have concerns about ID enumeration.

3. fail() response includes a localized error string

return fail(result.response.status, { error: getErrorMessage(code) });

getErrorMessage() maps error codes to i18n strings. This is correct — no raw backend error messages, no stack traces, no implementation details. This is the right pattern and matches the project's error handling standard. Good.

4. No CSRF exposure from the use:enhance approach

SvelteKit form actions use the session cookie (HttpOnly, SameSite) for auth. The use:enhance intercept does not change the CSRF properties — form POSTs to same-origin actions are SameSite-protected. No concern here.

Nit

5. makeActionEvent in page.server.spec.ts uses the global fetch

function makeActionEvent(formData: FormData): any {
    return {
        request: new Request('http://localhost/aktivitaeten', { method: 'POST', body: formData }),
        fetch  // <-- global fetch, not the mocked API client fetch
    };
}

The fetch here is the real global fetch, but the test mocks createApiClient so it doesn't actually use this fetch. Functionally fine in tests, but the // eslint-disable-next-line @typescript-eslint/no-explicit-any above the function and the bare fetch reference look like implementation shortcuts that could cause confusion when the test infra changes. Worth documenting with a comment.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** This PR is primarily a frontend refactor, so the attack surface is small. The backend IDOR protection is unchanged and the session-based auth cookie forwarding through SvelteKit's `fetch` is correct. A few things to flag. --- ### Blockers **1. Unvalidated form input reaches the API path parameter** `+page.server.ts` line 74: ```typescript const notificationId = data.get('notificationId') as string; ``` `formData.get()` returns `FormDataEntryValue | null`. The `as string` cast removes the null guard. An attacker with JS disabled who manually crafts a form POST to `/aktivitaeten?/dismiss-notification` without a `notificationId` field will cause this to be `null` cast to `string`, which then gets interpolated into the API path: `/api/notifications/null/read`. The backend will 404 or 400 (so no exploitation here), but the missing guard is a code smell that can mask worse bugs in similar actions added later. Fix: ```typescript const notificationId = data.get('notificationId'); if (!notificationId || typeof notificationId !== 'string' || notificationId.trim() === '') { return fail(400, { error: 'Missing notificationId' }); } ``` **Security note on IDOR:** The backend `PATCH /api/notifications/{id}/read` enforces ownership server-side (authenticated session + notification belongs to calling user), so there is no IDOR vulnerability introduced here. The `notificationId` comes from the store, not from user input, under JS-on flow. Under JS-off flow it comes from the hidden input, but the backend validates ownership either way. This is correctly handled. --- ### Suggestions **2. The `notificationId` in the hidden `<input>` is visible in the DOM** ```svelte <input type="hidden" name="notificationId" value={notification.id} /> ``` `type="hidden"` does not prevent a user from viewing or modifying the value via browser DevTools. Under JS-on flow with `use:enhance`, this is harmless because the backend validates ownership. Under JS-off flow (native form POST), the same backend ownership check applies. This is not a vulnerability, but it's worth noting for completeness: the `notification.id` (a UUID) is exposed in the HTML source. That's acceptable given UUIDs are not a secret, but teams sometimes have concerns about ID enumeration. **3. `fail()` response includes a localized error string** ```typescript return fail(result.response.status, { error: getErrorMessage(code) }); ``` `getErrorMessage()` maps error codes to i18n strings. This is correct — no raw backend error messages, no stack traces, no implementation details. This is the right pattern and matches the project's error handling standard. Good. **4. No CSRF exposure from the `use:enhance` approach** SvelteKit form actions use the session cookie (HttpOnly, SameSite) for auth. The `use:enhance` intercept does not change the CSRF properties — form POSTs to same-origin actions are SameSite-protected. No concern here. --- ### Nit **5. `makeActionEvent` in `page.server.spec.ts` uses the global `fetch`** ```typescript function makeActionEvent(formData: FormData): any { return { request: new Request('http://localhost/aktivitaeten', { method: 'POST', body: formData }), fetch // <-- global fetch, not the mocked API client fetch }; } ``` The `fetch` here is the real global `fetch`, but the test mocks `createApiClient` so it doesn't actually use this `fetch`. Functionally fine in tests, but the `// eslint-disable-next-line @typescript-eslint/no-explicit-any` above the function and the bare `fetch` reference look like implementation shortcuts that could cause confusion when the test infra changes. Worth documenting with a comment.

marcel commented

2026-05-19 23:29:37 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

This refactor brings notification writes into the SvelteKit form action layer — the right place for server-side writes in this stack. The boundary between server action (HTTP) and client store (optimistic UI) is now explicit and correct. No doc updates are required: no new routes, no new backend packages, no new DB migrations, no new Docker services. The existing aktivitaeten route just gains actions.

Observations

1. The use:enhance SubmitFunction ordering is architecturally concerning

As Felix correctly notes, onClose() and goto() currently execute in the SubmitFunction body before the server responds. From an architecture perspective, this is a layering issue: the UI state transition (close + navigate) is being triggered optimistically without a rollback path. This is fine if the product decision is "we accept that failed server calls don't undo the navigation", but that decision should be explicit. The current implementation makes it accidental rather than intentional.

2. Hardcoded action path /aktivitaeten?/mark-all-read in components outside the route

NotificationDropdown.svelte and ChronikFuerDichBox.svelte are in $lib/notification/ and $lib/activity/ respectively — library components, not route-specific components. They now contain a hardcoded absolute path /aktivitaeten?/mark-all-read. This creates a coupling between a library component and a specific route.

If the route is ever renamed or if these components are ever reused in a different context, the hardcoded paths will silently break (the native form POST goes to the wrong place; use:enhance intercepts it, so JS-on users won't notice). The correct approach is to pass the action URL as a prop, or accept that these components are route-bound and move them into the route folder:

<!-- Option A: prop -->
<ChronikFuerDichBox
    unread={unread}
    markAllReadAction="/aktivitaeten?/mark-all-read"
    dismissNotificationAction="/aktivitaeten?/dismiss-notification"
    optimisticMarkRead={...}
    optimisticMarkAllRead={...}
/>

This is a design smell, not a blocker, but it should be addressed before the components are ever reused.

3. Module structure is clean

The separation of optimisticMarkRead/optimisticMarkAllRead (pure state mutation, no network) from the server actions (network, session auth) is the correct split. The store is now a pure UI state manager; the SvelteKit action is the API gateway. This is a better architecture than the original fetch() calls inside the store.

4. No doc updates triggered by this PR

No new SvelteKit routes (only new actions on existing route) → no CLAUDE.md route table update needed
No new backend packages → no architecture diagram update needed
No Flyway migrations → no DB diagram update needed
No new Docker services → no deployment doc update needed

✅ This PR correctly requires no documentation updates.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** This refactor brings notification writes into the SvelteKit form action layer — the right place for server-side writes in this stack. The boundary between server action (HTTP) and client store (optimistic UI) is now explicit and correct. No doc updates are required: no new routes, no new backend packages, no new DB migrations, no new Docker services. The existing `aktivitaeten` route just gains actions. --- ### Observations **1. The `use:enhance` SubmitFunction ordering is architecturally concerning** As Felix correctly notes, `onClose()` and `goto()` currently execute in the SubmitFunction body before the server responds. From an architecture perspective, this is a layering issue: the UI state transition (close + navigate) is being triggered optimistically without a rollback path. This is fine if the product decision is "we accept that failed server calls don't undo the navigation", but that decision should be explicit. The current implementation makes it accidental rather than intentional. **2. Hardcoded action path `/aktivitaeten?/mark-all-read` in components outside the route** `NotificationDropdown.svelte` and `ChronikFuerDichBox.svelte` are in `$lib/notification/` and `$lib/activity/` respectively — library components, not route-specific components. They now contain a hardcoded absolute path `/aktivitaeten?/mark-all-read`. This creates a coupling between a library component and a specific route. If the route is ever renamed or if these components are ever reused in a different context, the hardcoded paths will silently break (the native form POST goes to the wrong place; `use:enhance` intercepts it, so JS-on users won't notice). The correct approach is to pass the action URL as a prop, or accept that these components are route-bound and move them into the route folder: ```svelte  <ChronikFuerDichBox unread={unread} markAllReadAction="/aktivitaeten?/mark-all-read" dismissNotificationAction="/aktivitaeten?/dismiss-notification" optimisticMarkRead={...} optimisticMarkAllRead={...} /> ``` This is a design smell, not a blocker, but it should be addressed before the components are ever reused. **3. Module structure is clean** The separation of `optimisticMarkRead`/`optimisticMarkAllRead` (pure state mutation, no network) from the server actions (network, session auth) is the correct split. The store is now a pure UI state manager; the SvelteKit action is the API gateway. This is a better architecture than the original `fetch()` calls inside the store. **4. No doc updates triggered by this PR** - No new SvelteKit routes (only new actions on existing route) → no `CLAUDE.md` route table update needed - No new backend packages → no architecture diagram update needed - No Flyway migrations → no DB diagram update needed - No new Docker services → no deployment doc update needed ✅ This PR correctly requires no documentation updates.

marcel commented

2026-05-19 23:29:59 +02:00

🧪 Sara Holt — QA Engineer & Test Automation Specialist

Verdict: ⚠️ Approved with concerns

The new server action tests (page.server.spec.ts) are solid — three cases per action (happy path, API error, missing field implied by error test). The store tests for optimisticMarkRead/optimisticMarkAllRead correctly verify no fetch is called. The component test updates are mechanical and correct. Two gaps worth addressing.

Blockers

1. Navigation behavior after dismiss is untested

The previous NotificationBell.svelte.spec.ts had two tests verifying that goto() was called with the correct URL (with and without annotationId). These tests were deleted. The navigation now happens inside NotificationDropdown.svelte's use:enhance SubmitFunction — but the $app/forms mock doesn't invoke the returned callback, so goto() is never called in tests.

Concretely: there is no test asserting that clicking a notification row in the dropdown navigates to /documents/{documentId}?commentId={referenceId} or /documents/{documentId}?commentId={referenceId}&annotationId={annotationId}. This was a user-visible behavior that was tested and is now untested. It should be restored in NotificationDropdown.svelte.test.ts.

2. fail() response path is never asserted in component tests

The server action tests verify that fail(status, { error }) is returned. But no component test verifies that when the enhance callback receives result.type === 'failure', the error is surfaced to the user (e.g., a toast, an inline message, or at least that update() is called). Since the current code silently swallows failures (the update() call with invalidateAll: false effectively discards the fail() payload), this is a behavior gap — but it's also untested, so no test currently documents that the failure is intentionally silent.

Suggestions

3. use:enhance mock is duplicated in 5 files

The identical mock block appears verbatim in:

ChronikFuerDichBox.svelte.spec.ts
ChronikFuerDichBox.svelte.test.ts
NotificationBell.svelte.spec.ts
NotificationDropdown.svelte.test.ts

Extract to a shared test helper:

// src/test-utils/mockEnhance.ts
import { vi } from 'vitest';

export function mockEnhance() {
    vi.mock('$app/forms', () => ({
        enhance(node: HTMLFormElement, submit?: (opts: { formData: FormData }) => unknown) {
            const handler = (e: Event) => {
                e.preventDefault();
                submit?.({ formData: new FormData(node) } as never);
            };
            node.addEventListener('submit', handler);
            return { destroy: () => node.removeEventListener('submit', handler) };
        }
    }));
}

DRY violation in test code is as costly as in production code — when the mock needs to change (e.g., to test the result callback), you'll have to update five files.

4. page.server.spec.ts missing test: dismiss-notification with missing notificationId

The PR description says there are tests for "missing field", but looking at the actual tests:

calls PATCH ... with the form-supplied notificationId ✅
returns { success: true } when the API responds ok ✅
returns fail(status, { error }) when the API responds non-ok ✅

There is no test for when notificationId is absent from the FormData (no fd.set('notificationId', ...) call). Given the as string null cast issue, this is a gap. Add:

it('returns fail(400) when notificationId is missing from form data', async () => {
    const result = await actions['dismiss-notification'](makeActionEvent(new FormData()));
    expect(result).toMatchObject({ status: 400 });
});

Nit

5. Test name inconsistency between the two spec files for ChronikFuerDichBox

There are two test files for the same component: ChronikFuerDichBox.svelte.spec.ts and ChronikFuerDichBox.svelte.test.ts. Both cover overlapping behaviors (dismiss button, mark-all-read). This is a pre-existing issue, not introduced by this PR — but worth noting that duplicate coverage of the same component across two files makes it harder to know where to add new tests.

## 🧪 Sara Holt — QA Engineer & Test Automation Specialist **Verdict: ⚠️ Approved with concerns** The new server action tests (`page.server.spec.ts`) are solid — three cases per action (happy path, API error, missing field implied by error test). The store tests for `optimisticMarkRead`/`optimisticMarkAllRead` correctly verify no fetch is called. The component test updates are mechanical and correct. Two gaps worth addressing. --- ### Blockers **1. Navigation behavior after dismiss is untested** The previous `NotificationBell.svelte.spec.ts` had two tests verifying that `goto()` was called with the correct URL (with and without `annotationId`). These tests were deleted. The navigation now happens inside `NotificationDropdown.svelte`'s `use:enhance` SubmitFunction — but the `$app/forms` mock doesn't invoke the returned callback, so `goto()` is never called in tests. Concretely: there is no test asserting that clicking a notification row in the dropdown navigates to `/documents/{documentId}?commentId={referenceId}` or `/documents/{documentId}?commentId={referenceId}&annotationId={annotationId}`. This was a user-visible behavior that was tested and is now untested. It should be restored in `NotificationDropdown.svelte.test.ts`. **2. `fail()` response path is never asserted in component tests** The server action tests verify that `fail(status, { error })` is returned. But no component test verifies that when the enhance callback receives `result.type === 'failure'`, the error is surfaced to the user (e.g., a toast, an inline message, or at least that `update()` is called). Since the current code silently swallows failures (the `update()` call with `invalidateAll: false` effectively discards the `fail()` payload), this is a behavior gap — but it's also untested, so no test currently documents that the failure is intentionally silent. --- ### Suggestions **3. `use:enhance` mock is duplicated in 5 files** The identical mock block appears verbatim in: - `ChronikFuerDichBox.svelte.spec.ts` - `ChronikFuerDichBox.svelte.test.ts` - `NotificationBell.svelte.spec.ts` - `NotificationDropdown.svelte.test.ts` Extract to a shared test helper: ```typescript // src/test-utils/mockEnhance.ts import { vi } from 'vitest'; export function mockEnhance() { vi.mock('$app/forms', () => ({ enhance(node: HTMLFormElement, submit?: (opts: { formData: FormData }) => unknown) { const handler = (e: Event) => { e.preventDefault(); submit?.({ formData: new FormData(node) } as never); }; node.addEventListener('submit', handler); return { destroy: () => node.removeEventListener('submit', handler) }; } })); } ``` DRY violation in test code is as costly as in production code — when the mock needs to change (e.g., to test the `result` callback), you'll have to update five files. **4. `page.server.spec.ts` missing test: `dismiss-notification` with missing `notificationId`** The PR description says there are tests for "missing field", but looking at the actual tests: - `calls PATCH ... with the form-supplied notificationId` ✅ - `returns { success: true } when the API responds ok` ✅ - `returns fail(status, { error }) when the API responds non-ok` ✅ There is no test for when `notificationId` is absent from the FormData (no `fd.set('notificationId', ...)` call). Given the `as string` null cast issue, this is a gap. Add: ```typescript it('returns fail(400) when notificationId is missing from form data', async () => { const result = await actions['dismiss-notification'](makeActionEvent(new FormData())); expect(result).toMatchObject({ status: 400 }); }); ``` --- ### Nit **5. Test name inconsistency between the two spec files for `ChronikFuerDichBox`** There are two test files for the same component: `ChronikFuerDichBox.svelte.spec.ts` and `ChronikFuerDichBox.svelte.test.ts`. Both cover overlapping behaviors (dismiss button, mark-all-read). This is a pre-existing issue, not introduced by this PR — but worth noting that duplicate coverage of the same component across two files makes it harder to know where to add new tests.

marcel commented

2026-05-19 23:30:24 +02:00

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

Verdict: ⚠️ Approved with concerns

The structural changes from <button onclick> to <form use:enhance> are sound and improve JS-off degradation. Most of the visual output is unchanged. A few accessibility regressions and concerns to flag.

Blockers

1. <form class="contents"> breaks focus ring and hover state on the notification row

NotificationDropdown.svelte line 84: class="contents" on the <form> removes it from layout. The <button type="submit"> inside then carries all the visual classes (flex, border, hover, etc.) — that part is fine. However, class="contents" means the <form> itself has no visual presence, but assistive technologies (particularly older screen readers) may still announce the form as a group. More importantly, the <form> doesn't receive focus, so there's no accessible group role communicating that this is a "dismiss notification" unit.

This is not catastrophically broken — the button inside still has its text content read aloud — but it's an unusual pattern. I'd check with axe-core whether <form class="contents"> with use:enhance inside a <ul role="list"> produces any ARIA tree issues. The alternative is to not use class="contents" and instead apply the layout classes to the <form> itself (making it display: flex).

2. Dismiss action errors are never communicated to the user

When dismiss-notification returns fail(), there is no visible error state. The user clicks dismiss, the unread dot disappears (optimistic update), and if the server fails, there is no recovery, no toast, no inline error. For senior users (60+) who rely on visual confirmation that their action succeeded, silent failure violates Nielsen heuristic #1 (visibility of system status). This is a design problem, not just an engineering one — the UX requirement for failure feedback was not specified in the original issue.

Minimum viable: a toast notification using the existing notification infrastructure or a simple aria-live region on the page.

Suggestions

3. mark-all-read form in the dropdown header has no label or accessible name

NotificationDropdown.svelte lines 36–52: the <form> wrapping "Alle gelesen" contains only a <button>. The form itself has no aria-label. While the button text provides its own label, some screen readers announce the form as a container before the button. Adding aria-label to the form (or aria-hidden="true" if the form element itself is noise) would clarify intent for AT users.

4. The ChronikFuerDichBox dismiss form is also missing an accessible name for the form grouping

ChronikFuerDichBox.svelte: each dismiss button is now inside a <form>. The button has aria-label={m.chronik_mark_read_aria()} — that's correct. But the wrapping <form> is an anonymous interactive region. Given the form wraps both the hidden input and the dismiss button, AT may or may not announce the form boundary. In practice this is minor since the button's aria-label is sufficient, but adding aria-hidden="true" to the <form> element itself (keeping only the button accessible) would make the AT tree cleaner.

5. Positive: py-3 → py-3.5 on notification rows

The diff shows px-4 py-3 changed to px-4 py-3.5 on the notification button rows. Going from 12px to 14px vertical padding slightly increases the touch target — positive for the senior audience. Small but appreciated.

Nit

6. The unicode escape ↩ replaced with literal ↩

The diff shows {n.type === 'MENTION' ? '@' : '↩'} changed to {n.type === 'MENTION' ? '@' : '↩'}. Both render identically. The literal ↩ is more readable in source. No issue.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** The structural changes from `<button onclick>` to `<form use:enhance>` are sound and improve JS-off degradation. Most of the visual output is unchanged. A few accessibility regressions and concerns to flag. --- ### Blockers **1. `<form class="contents">` breaks focus ring and hover state on the notification row** `NotificationDropdown.svelte` line 84: `class="contents"` on the `<form>` removes it from layout. The `<button type="submit">` inside then carries all the visual classes (flex, border, hover, etc.) — that part is fine. However, `class="contents"` means the `<form>` itself has no visual presence, but assistive technologies (particularly older screen readers) may still announce the form as a group. More importantly, the `<form>` doesn't receive focus, so there's no accessible group role communicating that this is a "dismiss notification" unit. This is not catastrophically broken — the button inside still has its text content read aloud — but it's an unusual pattern. I'd check with axe-core whether `<form class="contents">` with `use:enhance` inside a `<ul role="list">` produces any ARIA tree issues. The alternative is to not use `class="contents"` and instead apply the layout classes to the `<form>` itself (making it `display: flex`). **2. Dismiss action errors are never communicated to the user** When `dismiss-notification` returns `fail()`, there is no visible error state. The user clicks dismiss, the unread dot disappears (optimistic update), and if the server fails, there is no recovery, no toast, no inline error. For senior users (60+) who rely on visual confirmation that their action succeeded, silent failure violates Nielsen heuristic #1 (visibility of system status). This is a design problem, not just an engineering one — the UX requirement for failure feedback was not specified in the original issue. Minimum viable: a toast notification using the existing notification infrastructure or a simple `aria-live` region on the page. --- ### Suggestions **3. `mark-all-read` form in the dropdown header has no label or accessible name** `NotificationDropdown.svelte` lines 36–52: the `<form>` wrapping "Alle gelesen" contains only a `<button>`. The form itself has no `aria-label`. While the button text provides its own label, some screen readers announce the form as a container before the button. Adding `aria-label` to the form (or `aria-hidden="true"` if the form element itself is noise) would clarify intent for AT users. **4. The `ChronikFuerDichBox` dismiss form is also missing an accessible name for the form grouping** `ChronikFuerDichBox.svelte`: each dismiss button is now inside a `<form>`. The button has `aria-label={m.chronik_mark_read_aria()}` — that's correct. But the wrapping `<form>` is an anonymous interactive region. Given the form wraps both the hidden input and the dismiss button, AT may or may not announce the form boundary. In practice this is minor since the button's `aria-label` is sufficient, but adding `aria-hidden="true"` to the `<form>` element itself (keeping only the button accessible) would make the AT tree cleaner. **5. Positive: `py-3` → `py-3.5` on notification rows** The diff shows `px-4 py-3` changed to `px-4 py-3.5` on the notification button rows. Going from 12px to 14px vertical padding slightly increases the touch target — positive for the senior audience. Small but appreciated. --- ### Nit **6. The unicode escape `↩` replaced with literal `↩`** The diff shows `{n.type === 'MENTION' ? '@' : '↩'}` changed to `{n.type === 'MENTION' ? '@' : '↩'}`. Both render identically. The literal `↩` is more readable in source. No issue.

marcel commented

2026-05-19 23:30:43 +02:00

📋 Elicit (Requirements Engineer)

Verdict: ⚠️ Approved with concerns

Reviewing from a requirements completeness perspective — specifically whether the stated goal of "progressive enhancement / JS-off degradation" is actually achieved, and whether edge cases are covered.

Blockers

1. JS-off degradation for the bell dropdown is not achievable — acceptance criterion cannot be met

The PR description states: "JS-off degradation now works via native form POST." This is true for ChronikFuerDichBox on the /aktivitaeten page. However, NotificationDropdown.svelte renders inside NotificationBell.svelte, which is opened by a <button> with a JavaScript onclick handler. Without JS, the dropdown never opens, so the form actions inside it are never reachable.

This is an inherent architectural constraint — a JavaScript-gated dropdown cannot provide JS-off form degradation for its contents. The PR should not claim JS-off degradation for the bell path; only for the Chronik page path. The acceptance criterion as written is partially misleading.

2. Race condition: what happens if the user clicks dismiss on two notifications in rapid succession?

Both optimisticMarkRead calls fire immediately. Both use:enhance form POSTs are submitted to the server. There is no loading/disabled state on the button after submission, so the user can submit the same notification's form multiple times. PATCH /api/notifications/{id}/read is presumably idempotent (marking an already-read notification as read is a no-op), but this is not verified by any test. The UI does not prevent double-submission.

Minimum: disable the submit button after first click (standard use:enhance pattern with a submitting flag).

Suggestions

3. The requirement "mark-all-read clears the badge" is tested server-side but the badge update path is indirect

The badge count lives in notificationStore.unreadCount, which is updated by optimisticMarkAllRead(). The form action itself returns { success: true } and calls update({ reset: false, invalidateAll: false }). This means the server's reloaded page data (which would have unreadNotifications: []) is not re-fetched after mark-all-read. The badge clears only via the optimistic store update, not via a server round-trip.

This is intentional and correct — the PR's intent is optimistic updates. But it creates an undocumented invariant: if the optimistic update fires but the server call fails, the badge shows 0 while the server still has unread notifications. This is an accepted trade-off, but it should be documented in a comment next to invalidateAll: false.

4. Missing requirement: what should happen when dismiss-notification is called from the bell dropdown on a page other than /aktivitaeten?

The bell dropdown exists on every page (it's in the global layout). The form action target is hardcoded to /aktivitaeten?/dismiss-notification. If a user dismisses a notification from the bell while on /documents/123:

Under JS-on: use:enhance intercepts the form, calls the action, and update() runs. The user sees no navigation to /aktivitaeten. ✅
Under JS-off: the native form POST submits to /aktivitaeten?/dismiss-notification, and SvelteKit redirects back — but there's no redirect in the action, so the user lands on /aktivitaeten. ❌

This is the JS-off degradation gap for the bell path (related to blocker #1). Worth documenting as an explicit non-goal: "JS-off dismiss from bell always navigates to Chronik page."

## 📋 Elicit (Requirements Engineer) **Verdict: ⚠️ Approved with concerns** Reviewing from a requirements completeness perspective — specifically whether the stated goal of "progressive enhancement / JS-off degradation" is actually achieved, and whether edge cases are covered. --- ### Blockers **1. JS-off degradation for the bell dropdown is not achievable — acceptance criterion cannot be met** The PR description states: "JS-off degradation now works via native form POST." This is true for `ChronikFuerDichBox` on the `/aktivitaeten` page. However, `NotificationDropdown.svelte` renders inside `NotificationBell.svelte`, which is opened by a `<button>` with a JavaScript `onclick` handler. Without JS, the dropdown never opens, so the form actions inside it are never reachable. This is an inherent architectural constraint — a JavaScript-gated dropdown cannot provide JS-off form degradation for its contents. The PR should not claim JS-off degradation for the bell path; only for the Chronik page path. The acceptance criterion as written is partially misleading. **2. Race condition: what happens if the user clicks dismiss on two notifications in rapid succession?** Both `optimisticMarkRead` calls fire immediately. Both `use:enhance` form POSTs are submitted to the server. There is no loading/disabled state on the button after submission, so the user can submit the same notification's form multiple times. `PATCH /api/notifications/{id}/read` is presumably idempotent (marking an already-read notification as read is a no-op), but this is not verified by any test. The UI does not prevent double-submission. Minimum: disable the submit button after first click (standard `use:enhance` pattern with a `submitting` flag). --- ### Suggestions **3. The requirement "mark-all-read clears the badge" is tested server-side but the badge update path is indirect** The badge count lives in `notificationStore.unreadCount`, which is updated by `optimisticMarkAllRead()`. The form action itself returns `{ success: true }` and calls `update({ reset: false, invalidateAll: false })`. This means the server's reloaded page data (which would have `unreadNotifications: []`) is **not** re-fetched after mark-all-read. The badge clears only via the optimistic store update, not via a server round-trip. This is intentional and correct — the PR's intent is optimistic updates. But it creates an undocumented invariant: if the optimistic update fires but the server call fails, the badge shows 0 while the server still has unread notifications. This is an accepted trade-off, but it should be documented in a comment next to `invalidateAll: false`. **4. Missing requirement: what should happen when `dismiss-notification` is called from the bell dropdown on a page other than `/aktivitaeten`?** The bell dropdown exists on every page (it's in the global layout). The form action target is hardcoded to `/aktivitaeten?/dismiss-notification`. If a user dismisses a notification from the bell while on `/documents/123`: - Under JS-on: `use:enhance` intercepts the form, calls the action, and `update()` runs. The user sees no navigation to `/aktivitaeten`. ✅ - Under JS-off: the native form POST submits to `/aktivitaeten?/dismiss-notification`, and SvelteKit redirects back — but there's no redirect in the action, so the user lands on `/aktivitaeten`. ❌ This is the JS-off degradation gap for the bell path (related to blocker #1). Worth documenting as an explicit non-goal: "JS-off dismiss from bell always navigates to Chronik page."

marcel commented

2026-05-19 23:30:54 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

This is a pure frontend refactor — no Docker Compose changes, no CI pipeline changes, no new services, no environment variables. Nothing to flag from an infrastructure perspective.

Observations

1. No infrastructure impact

No new environment variables introduced
No new Docker services or compose changes
No new ports exposed
No new secrets or credentials
CI pipeline is unaffected — the new tests run in the existing Vitest browser suite

2. The server action pattern is SSR-friendly

+page.server.ts actions run server-side in the Node adapter. The createApiClient(fetch) call uses the SvelteKit-provided fetch (which forwards cookies and session context), not the browser's fetch. This is correct for the production deployment topology (Caddy → SvelteKit Node → Spring Boot).

3. No invalidateAll: true — avoids unnecessary backend round-trips

update({ reset: false, invalidateAll: false }) means after a dismiss or mark-all-read, the page's load() function is not re-executed. This is correct — it avoids an extra round-trip to the backend on every notification interaction. The load function fetches unreadNotifications from the backend; skipping that re-fetch and relying on the optimistic store update is the right call for this interaction pattern.

4. What's done well

The refactor eliminates two raw fetch() calls from the client store and routes them through the SvelteKit server layer. This is operationally better: the server-side calls use the authenticated session cookie, are logged server-side (if logging is enabled), and don't expose API endpoints to browser network inspection in the same way. Good hygiene.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** This is a pure frontend refactor — no Docker Compose changes, no CI pipeline changes, no new services, no environment variables. Nothing to flag from an infrastructure perspective. --- ### Observations **1. No infrastructure impact** - No new environment variables introduced - No new Docker services or compose changes - No new ports exposed - No new secrets or credentials - CI pipeline is unaffected — the new tests run in the existing Vitest browser suite **2. The server action pattern is SSR-friendly** `+page.server.ts` actions run server-side in the Node adapter. The `createApiClient(fetch)` call uses the SvelteKit-provided `fetch` (which forwards cookies and session context), not the browser's `fetch`. This is correct for the production deployment topology (Caddy → SvelteKit Node → Spring Boot). **3. No `invalidateAll: true` — avoids unnecessary backend round-trips** `update({ reset: false, invalidateAll: false })` means after a dismiss or mark-all-read, the page's `load()` function is **not** re-executed. This is correct — it avoids an extra round-trip to the backend on every notification interaction. The load function fetches `unreadNotifications` from the backend; skipping that re-fetch and relying on the optimistic store update is the right call for this interaction pattern. **4. What's done well** The refactor eliminates two raw `fetch()` calls from the client store and routes them through the SvelteKit server layer. This is operationally better: the server-side calls use the authenticated session cookie, are logged server-side (if logging is enabled), and don't expose API endpoints to browser network inspection in the same way. Good hygiene.

marcel added 3 commits 2026-05-19 23:57:25 +02:00

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

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 f9340366d1

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

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

Details

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

Details

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

Details

CI / fail2ban Regex (pull_request) Successful in 39s

Details

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

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 58s

Details

7fe8842b57

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>

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

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

- 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 2adb98895d

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

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

Details

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

Details

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

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

f2bb58e294

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>

marcel added 1 commit 2026-05-20 07:31:45 +02:00

fix(notification): address review suggestions

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

Details

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

Details

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

Details

CI / fail2ban Regex (pull_request) Successful in 40s

Details

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

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 58s

Details

b5239f515f

- 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>

marcel closed this pull request

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

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

Details

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

Details

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

Details

CI / fail2ban Regex (pull_request) Successful in 40s

Details

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

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 58s

Details

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#630