refactor(notification-bell): use SvelteKit form actions instead of raw fetch #286

New Issue

marcel · 2026-04-20T12:28:53+02:00

marcel commented

2026-04-20 12:28:53 +02:00

Motivation

While discussing issue #285 (Chronik — unified activity + notifications page), we decided Chronik's Dismiss and "Alle gelesen" actions will use SvelteKit form actions (<form method="POST" action="?/…" use:enhance>) — consistent with Nora's CSRF guidance and the existing admin patterns (admin/users, admin/tags, etc.).

The existing NotificationBell + NotificationDropdown components call the same two endpoints (PATCH /api/notifications/{id}/read, POST /api/notifications/read-all) via raw fetch() from inside useNotificationStream.svelte.ts. Shipping Chronik without touching the Bell leaves us with two code paths for the same operations:

Chronik: form action → +page.server.ts action → /api/notifications/{id}/read
Bell dropdown: raw fetch() from the browser → /api/notifications/{id}/read

The inconsistency is cosmetic today (both work, both carry the auth cookie, both are SameSite=Lax-protected) but will confuse the next developer who wonders "why does Chronik use a form action and the Bell doesn't?"

Proposed change

Replace the raw fetch() calls in NotificationDropdown.svelte with SvelteKit form actions, matching the pattern Chronik introduces in #285.

Affected files

frontend/src/lib/components/NotificationDropdown.svelte — per-notification Mark-read button wraps in a <form method="POST" action="/notifications/dismiss?id={n.id}" use:enhance>; Mark-all wraps similarly.
frontend/src/lib/hooks/useNotificationStream.svelte.ts — remove markRead and markAllRead from the exported API (or keep them as the mutation path called by the form action submit handlers). Update the optimistic-update logic to run inside the use:enhance callback instead of inline after fetch().
A new frontend/src/routes/notifications/dismiss/+page.server.ts (or a server action on an appropriate existing route) holding the form actions — alternatively, refactor to POST-only SvelteKit routes.
Any Vitest specs that exercise the Bell's read-path need updating.

Questions to resolve during implementation

Best place for the form action route. Options: a dedicated /actions/notifications route group, a server endpoint under /api/ wrapped in a SvelteKit proxy, or inline on the current page via +page.server.ts on every route that renders the header. The third is ugly; one of the first two is right.
Whether useNotificationStream should expose a single optimistic-update helper (notifications.optimisticMarkRead(id)) that the form action's enhancement callback calls, decoupling "mutate the API" from "update the store."

Acceptance

Bell dropdown's per-row mark-read uses a SvelteKit form action (<form method="POST" … use:enhance>).
Bell dropdown's "Mark all as read" uses a SvelteKit form action.
The underlying API endpoints (PATCH /api/notifications/{id}/read, POST /api/notifications/read-all) are unchanged — this is a frontend refactor only.
Optimistic UI updates (list mutation, unread count decrement) still happen — just inside the use:enhance callback instead of immediately after fetch().
Existing Bell tests (NotificationBell and NotificationDropdown specs) updated and passing.
Behavior parity: clicking a notification row still marks it read and navigates to the target document.
No change to /api/notifications/* endpoints themselves.

Blocked by

#285 (Chronik unified page). This refactor adopts the pattern Chronik introduces. It doesn't make sense to do this before #285 establishes the form-action convention for these endpoints.

References

Bell implementation: src/lib/components/NotificationBell.svelte
Dropdown: src/lib/components/NotificationDropdown.svelte
Stream hook: src/lib/hooks/useNotificationStream.svelte.ts
Chronik issue where the form-action convention is established: #285
Discussion context: #285 item 2 comment (#issuecomment-3568)

## Motivation While discussing issue #285 (Chronik — unified activity + notifications page), we decided Chronik's Dismiss and "Alle gelesen" actions will use SvelteKit form actions (`<form method="POST" action="?/…" use:enhance>`) — consistent with Nora's CSRF guidance and the existing admin patterns (`admin/users`, `admin/tags`, etc.). The existing `NotificationBell` + `NotificationDropdown` components call the **same two endpoints** (`PATCH /api/notifications/{id}/read`, `POST /api/notifications/read-all`) via raw `fetch()` from inside `useNotificationStream.svelte.ts`. Shipping Chronik without touching the Bell leaves us with two code paths for the same operations: - Chronik: form action → `+page.server.ts` action → `/api/notifications/{id}/read` - Bell dropdown: raw `fetch()` from the browser → `/api/notifications/{id}/read` The inconsistency is cosmetic today (both work, both carry the auth cookie, both are `SameSite=Lax`-protected) but will confuse the next developer who wonders "why does Chronik use a form action and the Bell doesn't?" ## Proposed change Replace the raw `fetch()` calls in `NotificationDropdown.svelte` with SvelteKit form actions, matching the pattern Chronik introduces in #285. ### Affected files - `frontend/src/lib/components/NotificationDropdown.svelte` — per-notification Mark-read button wraps in a `<form method="POST" action="/notifications/dismiss?id={n.id}" use:enhance>`; Mark-all wraps similarly. - `frontend/src/lib/hooks/useNotificationStream.svelte.ts` — remove `markRead` and `markAllRead` from the exported API (or keep them as the mutation path called by the form action submit handlers). Update the optimistic-update logic to run inside the `use:enhance` callback instead of inline after `fetch()`. - A new `frontend/src/routes/notifications/dismiss/+page.server.ts` (or a server action on an appropriate existing route) holding the form actions — alternatively, refactor to POST-only SvelteKit routes. - Any Vitest specs that exercise the Bell's read-path need updating. ### Questions to resolve during implementation - Best place for the form action route. Options: a dedicated `/actions/notifications` route group, a server endpoint under `/api/` wrapped in a SvelteKit proxy, or inline on the current page via `+page.server.ts` on *every* route that renders the header. The third is ugly; one of the first two is right. - Whether `useNotificationStream` should expose a single optimistic-update helper (`notifications.optimisticMarkRead(id)`) that the form action's enhancement callback calls, decoupling "mutate the API" from "update the store." ## Acceptance - [ ] Bell dropdown's per-row mark-read uses a SvelteKit form action (`<form method="POST" … use:enhance>`). - [ ] Bell dropdown's "Mark all as read" uses a SvelteKit form action. - [ ] The underlying API endpoints (`PATCH /api/notifications/{id}/read`, `POST /api/notifications/read-all`) are **unchanged** — this is a frontend refactor only. - [ ] Optimistic UI updates (list mutation, unread count decrement) still happen — just inside the `use:enhance` callback instead of immediately after `fetch()`. - [ ] Existing Bell tests (`NotificationBell` and `NotificationDropdown` specs) updated and passing. - [ ] Behavior parity: clicking a notification row still marks it read and navigates to the target document. - [ ] No change to `/api/notifications/*` endpoints themselves. ## Blocked by - **#285** (Chronik unified page). This refactor adopts the pattern Chronik introduces. It doesn't make sense to do this before #285 establishes the form-action convention for these endpoints. ## References - Bell implementation: [`src/lib/components/NotificationBell.svelte`](../src/branch/main/frontend/src/lib/components/NotificationBell.svelte) - Dropdown: [`src/lib/components/NotificationDropdown.svelte`](../src/branch/main/frontend/src/lib/components/NotificationDropdown.svelte) - Stream hook: [`src/lib/hooks/useNotificationStream.svelte.ts`](../src/branch/main/frontend/src/lib/hooks/useNotificationStream.svelte.ts) - Chronik issue where the form-action convention is established: #285 - Discussion context: #285 item 2 comment ([#issuecomment-3568](../issues/285#issuecomment-3568))

marcel added the notification refactor ui labels 2026-04-20 12:28:58 +02:00

marcel referenced this issue

2026-04-20 12:36:21 +02:00

feat: unify /notifications and dashboard activity feed into a /chronik page #285

marcel commented

2026-04-20 12:48:02 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The acceptance criterion "Existing Bell tests… updated and passing" is factually wrong — there are no test files for NotificationBell.svelte or NotificationDropdown.svelte today. The only adjacent test is frontend/src/routes/notifications/page.server.spec.ts, which only covers the page route. The refactor is effectively writing these tests from scratch, not updating them.
useNotificationStream.svelte.ts currently does three things: SSE subscription, REST fetches, and mutation. The proposed split (mutations move out, stream stays) is the right SRP cleanup — the hook becomes "subscribe + read", and writes go through form actions.
Current markRead does fetch → mutate → return; NotificationBell.handleMarkRead then goto(...). The user waits for the network round-trip before the navigation starts. Switching to a form action is the right time to fix that (optimistic mutate + goto immediately, fire-and-forget submission).
The notifications/+page.server.ts already wires a mark-all action through api.POST('/api/notifications/read-all') and redirects — the new bell action should mirror that exactly so we have one pattern, not two.

Recommendations

TDD this, red first: write NotificationDropdown.spec.ts (vitest-browser-svelte) asserting that clicking a row submits a form to the dismiss action; write dismiss/+page.server.spec.ts mocking createApiClient, mirroring the existing mark-all test. No production change before those are red.
Yes, expose optimisticMarkRead(id) and optimisticMarkAllRead() on the stream. The form action's use:enhance callback calls those synchronously before submit — that gives the existing instant UI without keeping fetch() in the hook. Removes the notification.read = true mutation-via-prop pattern, which is fragile.
For the route home: do not invent /notifications/dismiss/+page.server.ts. The /notifications route is being 301-redirected to /chronik by #285 (per its plan: "hooks.server.ts: 301 redirect /notifications → /chronik"). A POST to a 301-redirected path becomes a GET on the target — the form action breaks. Put the actions on /chronik/+page.server.ts (where mark-all will logically live anyway after #285) and submit absolute: <form action="/chronik?/dismiss-notification" method="POST" use:enhance>. One canonical home, lives next to its peer mutations.

use:enhance with explicit callback to keep navigation fast:

<form action="/chronik?/dismiss-notification" method="POST" use:enhance={({ formData }) => {
    stream.optimisticMarkRead(notification.id);
    goto(targetUrl);
    return async ({ update }) => { await update({ reset: false, invalidateAll: false }); };
}}>

Optimistic update + navigation kicks off immediately; the server response is awaited but doesn't block the user.

Hidden inputs for ids, not query params on the action URL — ?id={n.id} in the action attribute (as the issue body suggests) is brittle and inconsistent with the admin/tags pattern. Use <input type="hidden" name="notificationId" value={notification.id}>.

Open Decisions

(none — sequencing under #285 resolves the route-home question)

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The acceptance criterion "Existing Bell tests… updated and passing" is **factually wrong** — there are no test files for `NotificationBell.svelte` or `NotificationDropdown.svelte` today. The only adjacent test is `frontend/src/routes/notifications/page.server.spec.ts`, which only covers the page route. The refactor is effectively writing these tests from scratch, not updating them. - `useNotificationStream.svelte.ts` currently does three things: SSE subscription, REST fetches, and mutation. The proposed split (mutations move out, stream stays) is the right SRP cleanup — the hook becomes "subscribe + read", and writes go through form actions. - Current `markRead` does `fetch` → mutate → return; `NotificationBell.handleMarkRead` then `goto(...)`. The user waits for the network round-trip *before* the navigation starts. Switching to a form action is the right time to fix that (optimistic mutate + `goto` immediately, fire-and-forget submission). - The `notifications/+page.server.ts` already wires a `mark-all` action through `api.POST('/api/notifications/read-all')` and redirects — the new bell action should mirror that exactly so we have one pattern, not two. ### Recommendations - **TDD this, red first**: write `NotificationDropdown.spec.ts` (vitest-browser-svelte) asserting that clicking a row submits a form to the dismiss action; write `dismiss/+page.server.spec.ts` mocking `createApiClient`, mirroring the existing `mark-all` test. No production change before those are red. - **Yes, expose `optimisticMarkRead(id)` and `optimisticMarkAllRead()` on the stream.** The form action's `use:enhance` callback calls those synchronously before submit — that gives the existing instant UI without keeping `fetch()` in the hook. Removes the `notification.read = true` mutation-via-prop pattern, which is fragile. - **For the route home**: do *not* invent `/notifications/dismiss/+page.server.ts`. The `/notifications` route is being 301-redirected to `/chronik` by #285 (per its plan: "hooks.server.ts: 301 redirect /notifications → /chronik"). A POST to a 301-redirected path becomes a GET on the target — the form action breaks. Put the actions on `/chronik/+page.server.ts` (where `mark-all` will logically live anyway after #285) and submit absolute: `<form action="/chronik?/dismiss-notification" method="POST" use:enhance>`. One canonical home, lives next to its peer mutations. - **`use:enhance` with explicit callback** to keep navigation fast: ```svelte <form action="/chronik?/dismiss-notification" method="POST" use:enhance={({ formData }) => { stream.optimisticMarkRead(notification.id); goto(targetUrl); return async ({ update }) => { await update({ reset: false, invalidateAll: false }); }; }}> ``` Optimistic update + navigation kicks off immediately; the server response is awaited but doesn't block the user. - **Hidden inputs for ids**, not query params on the action URL — `?id={n.id}` in the action attribute (as the issue body suggests) is brittle and inconsistent with the admin/tags pattern. Use `<input type="hidden" name="notificationId" value={notification.id}>`. ### Open Decisions _(none — sequencing under #285 resolves the route-home question)_

marcel commented

2026-04-20 12:48:20 +02:00

🏛️ Markus Keller — Senior Application Architect

Observations

The motivation here is honest: the inconsistency is cosmetic, not functional. That's a legitimate refactor target — two paths for one operation is exactly the silent coupling that costs the next developer an hour of "wait, why?" Worth doing, but only after #285 lands so we're consolidating toward the new convention, not introducing a fork.
The proposed frontend/src/routes/notifications/dismiss/+page.server.ts route conflicts with #285's plan to 301 /notifications → /chronik. POST through a 301 becomes GET on the target — the action breaks. The route home isn't a stylistic choice; the dependency forces it onto /chronik.
useNotificationStream currently owns: (a) SSE subscription, (b) REST queries, (c) mutations. (a) and (b) are read concerns — the stream is the projection of unread state. (c) is a write concern. This refactor is a good moment to honor that boundary: the hook stops emitting writes, the form actions own them. The hook gains an optimisticMarkRead(id) helper — that's a projection update, conceptually a read-side operation. Boundary intact.
A note on conflict with #285's locked decisions: that issue says "Bell icon. Unchanged." That refers to user-visible behavior (count, dropdown content, layout). This refactor preserves all of that and changes only the wire mechanism. Document this explicitly in the PR description so reviewers don't think we're contradicting the spec.

Recommendations

Form actions live on /chronik after #285 ships. Names: dismiss-notification (single id via hidden input), mark-all-read (no body). They join the existing mark-all action — net result: one route owns all notification write operations, and the SSE stream is read-only.
Make the dependency explicit and ordered: do not start #286 until #285 is merged to main. The blocked-by line is correct; honor it strictly. If we start in parallel, the route-home question gets re-litigated.
Keep useNotificationStream as a read projection only. Public API after this refactor:
```
return {
    get notifications, get unreadCount,
    fetchNotifications, fetchUnreadCount,
    optimisticMarkRead, optimisticMarkAllRead,  // local mutation only
    init, destroy
};
```
No markRead/markAllRead — those names imply a network call and would mislead future readers. Optimistic updates are a UI concern, not a transport concern.
One ADR entry (short — 5 lines) capturing the decision: "Notification mutations use SvelteKit form actions submitted to /chronik. SSE stream is read-only. Optimistic UI lives in the stream hook as separately named helpers." This is the kind of structural decision that gets reverted by a future contributor "to simplify" if we don't write it down.

Open Decisions

(none — the dependency on #285 sequences the route-home choice, and the hook API split is straightforward SRP)

## 🏛️ Markus Keller — Senior Application Architect ### Observations - The motivation here is honest: the inconsistency is cosmetic, not functional. That's a legitimate refactor target — two paths for one operation is exactly the silent coupling that costs the next developer an hour of "wait, why?" Worth doing, but only after #285 lands so we're consolidating *toward* the new convention, not introducing a fork. - The proposed `frontend/src/routes/notifications/dismiss/+page.server.ts` route conflicts with #285's plan to 301 `/notifications` → `/chronik`. POST through a 301 becomes GET on the target — the action breaks. The route home isn't a stylistic choice; the dependency forces it onto `/chronik`. - `useNotificationStream` currently owns: (a) SSE subscription, (b) REST queries, (c) mutations. (a) and (b) are read concerns — the stream is the *projection* of unread state. (c) is a write concern. This refactor is a good moment to honor that boundary: the hook stops emitting writes, the form actions own them. The hook gains an `optimisticMarkRead(id)` helper — that's a *projection update*, conceptually a read-side operation. Boundary intact. - A note on conflict with #285's locked decisions: that issue says "Bell icon. **Unchanged.**" That refers to *user-visible behavior* (count, dropdown content, layout). This refactor preserves all of that and changes only the wire mechanism. Document this explicitly in the PR description so reviewers don't think we're contradicting the spec. ### Recommendations - **Form actions live on `/chronik`** after #285 ships. Names: `dismiss-notification` (single id via hidden input), `mark-all-read` (no body). They join the existing `mark-all` action — net result: one route owns *all* notification write operations, and the SSE stream is read-only. - **Make the dependency explicit and ordered**: do not start #286 until #285 is merged to main. The blocked-by line is correct; honor it strictly. If we start in parallel, the route-home question gets re-litigated. - **Keep `useNotificationStream` as a read projection only.** Public API after this refactor: ```typescript return { get notifications, get unreadCount, fetchNotifications, fetchUnreadCount, optimisticMarkRead, optimisticMarkAllRead, // local mutation only init, destroy }; ``` No `markRead`/`markAllRead` — those names imply a network call and would mislead future readers. Optimistic updates are a UI concern, not a transport concern. - **One ADR entry** (short — 5 lines) capturing the decision: "Notification mutations use SvelteKit form actions submitted to `/chronik`. SSE stream is read-only. Optimistic UI lives in the stream hook as separately named helpers." This is the kind of structural decision that gets reverted by a future contributor "to simplify" if we don't write it down. ### Open Decisions _(none — the dependency on #285 sequences the route-home choice, and the hook API split is straightforward SRP)_

marcel commented

2026-04-20 12:48:38 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Observations

The motivation cites "Nora's CSRF guidance" but the issue doesn't restate the threat model. For the next reader (and for the PR review), the rationale should be explicit: today's raw fetch('/api/notifications/{id}/read', { method: 'PATCH' }) with cookie auth is already protected — PATCH is not a form-submittable method, so a cross-origin attacker page cannot mint that request via a <form>. The benefit of moving to a form action is convention consistency, not a CSRF fix. Calling this a "security improvement" would overstate it.
That said: SvelteKit form actions submit POST to a SvelteKit route, then the SvelteKit Node server proxies the PATCH with cookie forwarding. The cross-origin attack surface for the new path is POST /chronik?/dismiss-notification — which is form-submittable. So the question becomes: what protects that path?
Looked up SecurityConfig.java: CSRF is disabled at the Spring side because of header-based session auth on the API. The SvelteKit form-action endpoint isn't a Spring endpoint — it's a Node SSR handler. SvelteKit's default protection here is the Origin header check on actions (built into the framework when csrf.checkOrigin is enabled, which it is by default). So we are not regressing — just shifting the protective layer.

Recommendations

Add a one-line code comment at the new form action explaining the threat model: // CSRF: SvelteKit checks Origin header by default for form-action POSTs; backend session cookies are SameSite=Lax. Future-you in 6 months will thank present-you.
Write three permission/auth tests (dismiss/+page.server.spec.ts or wherever the action lands) — these are cheap and they document the boundary:
1. Action returns redirect-to-login (or 401) when the SvelteKit session cookie is absent.
2. Action returns the backend's 403 when the user tries to dismiss someone else's notification (verify the backend enforces this — if it doesn't, that's a separate finding for the backend, not this issue).
3. Action returns 400/422 when notificationId is missing or malformed.
Verify the backend ownership check exists before relying on it. Grep the NotificationController.markRead path: confirm it asserts notification.recipient.id == currentUser.id. If not, that's a pre-existing IDOR — flag a separate issue, do not let this refactor mask it.
Do not log the notification id at INFO in the new server action. It's not sensitive per se, but logs are a soft attack surface; debug-level only, parameterized: log.debug("Dismissing notification {}", notificationId).

Open Decisions

(none — the security posture is preserved as long as the recommendations above are followed)

## 🔐 Nora "NullX" Steiner — Application Security Engineer ### Observations - The motivation cites "Nora's CSRF guidance" but the issue doesn't restate the threat model. For the next reader (and for the PR review), the rationale should be explicit: today's raw `fetch('/api/notifications/{id}/read', { method: 'PATCH' })` with cookie auth is *already* protected — `PATCH` is not a form-submittable method, so a cross-origin attacker page cannot mint that request via a `<form>`. The benefit of moving to a form action is **convention consistency**, not a CSRF fix. Calling this a "security improvement" would overstate it. - That said: SvelteKit form actions submit `POST` to a SvelteKit route, then the SvelteKit Node server proxies the `PATCH` with cookie forwarding. The cross-origin attack surface for the new path is `POST /chronik?/dismiss-notification` — which is form-submittable. So the question becomes: what protects *that* path? - Looked up `SecurityConfig.java`: CSRF is disabled at the Spring side because of header-based session auth on the API. The SvelteKit form-action endpoint isn't a Spring endpoint — it's a Node SSR handler. SvelteKit's default protection here is the `Origin` header check on actions (built into the framework when `csrf.checkOrigin` is enabled, which it is by default). So we are not regressing — just shifting the protective layer. ### Recommendations - **Add a one-line code comment at the new form action** explaining the threat model: `// CSRF: SvelteKit checks Origin header by default for form-action POSTs; backend session cookies are SameSite=Lax`. Future-you in 6 months will thank present-you. - **Write three permission/auth tests** (`dismiss/+page.server.spec.ts` or wherever the action lands) — these are cheap and they document the boundary: 1. Action returns redirect-to-login (or 401) when the SvelteKit session cookie is absent. 2. Action returns the backend's 403 when the user tries to dismiss someone else's notification (verify the backend enforces this — if it doesn't, that's a separate finding for the backend, not this issue). 3. Action returns 400/422 when `notificationId` is missing or malformed. - **Verify the backend ownership check exists** before relying on it. Grep the `NotificationController.markRead` path: confirm it asserts `notification.recipient.id == currentUser.id`. If not, that's a pre-existing IDOR — flag a separate issue, do not let this refactor mask it. - **Do not log the notification id at INFO** in the new server action. It's not sensitive per se, but logs are a soft attack surface; debug-level only, parameterized: `log.debug("Dismissing notification {}", notificationId)`. ### Open Decisions _(none — the security posture is preserved as long as the recommendations above are followed)_

marcel commented

2026-04-20 12:49:00 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

Acceptance criterion "Existing Bell tests (NotificationBell and NotificationDropdown specs) updated and passing" is incorrect — those test files do not exist today. I checked: Glob frontend/**/Notification*.test.ts returns nothing. The only related spec is frontend/src/routes/notifications/page.server.spec.ts, covering the page route only. This refactor is adding test coverage, not updating it. Reword the acceptance line so the implementer doesn't waste 20 minutes hunting for files that aren't there.
The existing mark-all action spec in notifications/page.server.spec.ts is a clean template: mocks createApiClient, asserts the API was called once, asserts the redirect. Mirror that shape for the new actions and we get consistent coverage.
No E2E coverage exists for the bell flow today. Adding a Playwright happy-path here would be high-value — the bell sits in the global header, regressions hit every page.
The optimistic-update logic is the part most likely to silently regress. A test that submits a form action with a deliberately delayed mock and asserts the UI updated before the response resolved is exactly the kind of thing we'd miss in code review and catch in CI.

Recommendations

Test pyramid for this refactor — write all of these, in this order, red-first:
1. Server action unit (vitest) — chronik/page.server.spec.ts (or wherever the actions land):
  - dismiss-notification calls api.PATCH('/api/notifications/{id}/read', { params: { path: { id }}}) exactly once with the form-supplied id.
  - dismiss-notification returns the SvelteKit form-action success shape (not a redirect — single dismiss should not navigate server-side; client navigates after).
  - mark-all-read calls api.POST('/api/notifications/read-all') exactly once and returns success.
  - Both return fail(401) / fail(403) when the API does. Three failure paths, three tests.
2. Component (vitest-browser-svelte) — NotificationDropdown.spec.ts:
  - Renders forms (not bare buttons) for each row when notifications are present.
  - Form submission triggers stream.optimisticMarkRead(id) synchronously before the server response resolves (use a delayed mock, assert the unread dot disappears immediately).
  - "Mark all" button is wrapped in a form posting to /chronik?/mark-all-read.
  - Empty state still renders without forms.
3. E2E (Playwright) — bell.spec.ts:
  - Login → click bell → click first unread row → asserts navigation to /documents/{id}?commentId=... AND that on return, the row appears as read in the bell list.
  - "Mark all" button clears the count badge.
  - No-JS variant: launch the same flow with javaScriptEnabled: false, verify the forms still submit and the page navigates correctly. This is the whole point of moving to form actions — if it doesn't work without JS, the refactor failed.
Quality gate: CI must run npm run test (vitest covers the unit + component layers) AND the new E2E spec. Don't ship without the no-JS test — it's the test that proves the refactor delivered its claimed benefit.
Visual regression: take an axe-playwright snapshot of the open dropdown at 320 / 768 / 1440px. The form wrapping per row may shift focus styles or padding by 1-2px — easier to verify by snapshot than by code review.

Open Decisions

(none — the test plan is straightforward; only requires the implementer to follow TDD)

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - Acceptance criterion "Existing Bell tests (`NotificationBell` and `NotificationDropdown` specs) updated and passing" is incorrect — those test files **do not exist today**. I checked: `Glob frontend/**/Notification*.test.ts` returns nothing. The only related spec is `frontend/src/routes/notifications/page.server.spec.ts`, covering the page route only. This refactor is *adding* test coverage, not updating it. Reword the acceptance line so the implementer doesn't waste 20 minutes hunting for files that aren't there. - The existing `mark-all` action spec in `notifications/page.server.spec.ts` is a clean template: mocks `createApiClient`, asserts the API was called once, asserts the redirect. Mirror that shape for the new actions and we get consistent coverage. - No E2E coverage exists for the bell flow today. Adding a Playwright happy-path here would be high-value — the bell sits in the global header, regressions hit *every* page. - The optimistic-update logic is the part most likely to silently regress. A test that submits a form action with a deliberately delayed mock and asserts the UI updated *before* the response resolved is exactly the kind of thing we'd miss in code review and catch in CI. ### Recommendations - **Test pyramid for this refactor** — write all of these, in this order, red-first: 1. **Server action unit (`vitest`)** — `chronik/page.server.spec.ts` (or wherever the actions land): - `dismiss-notification` calls `api.PATCH('/api/notifications/{id}/read', { params: { path: { id }}})` exactly once with the form-supplied id. - `dismiss-notification` returns the SvelteKit form-action success shape (not a redirect — single dismiss should not navigate server-side; client navigates after). - `mark-all-read` calls `api.POST('/api/notifications/read-all')` exactly once and returns success. - Both return `fail(401)` / `fail(403)` when the API does. Three failure paths, three tests. 2. **Component (`vitest-browser-svelte`)** — `NotificationDropdown.spec.ts`: - Renders forms (not bare buttons) for each row when notifications are present. - Form submission triggers `stream.optimisticMarkRead(id)` *synchronously* before the server response resolves (use a delayed mock, assert the unread dot disappears immediately). - "Mark all" button is wrapped in a form posting to `/chronik?/mark-all-read`. - Empty state still renders without forms. 3. **E2E (`Playwright`)** — `bell.spec.ts`: - Login → click bell → click first unread row → asserts navigation to `/documents/{id}?commentId=...` AND that on return, the row appears as read in the bell list. - "Mark all" button clears the count badge. - **No-JS variant**: launch the same flow with `javaScriptEnabled: false`, verify the forms still submit and the page navigates correctly. This is the *whole point* of moving to form actions — if it doesn't work without JS, the refactor failed. - **Quality gate**: CI must run `npm run test` (vitest covers the unit + component layers) AND the new E2E spec. Don't ship without the no-JS test — it's the test that proves the refactor delivered its claimed benefit. - **Visual regression**: take an axe-playwright snapshot of the open dropdown at 320 / 768 / 1440px. The form wrapping per row may shift focus styles or padding by 1-2px — easier to verify by snapshot than by code review. ### Open Decisions _(none — the test plan is straightforward; only requires the implementer to follow TDD)_

marcel commented

2026-04-20 12:49:09 +02:00

🛠️ Tobias Wendt — DevOps & Platform Engineer

Observations

Pure frontend refactor. No Compose, no env vars, no secrets, no CI workflow changes. Backend endpoints unchanged per the acceptance criteria.
One CI knock-on: the proposed Playwright no-JS test (which Sara will recommend) needs javaScriptEnabled: false context support — already supported by Playwright out of the box, no infra change.
Form action POSTs add a hop through SvelteKit SSR (Node) before reaching Spring Boot. In dev that's localhost-to-localhost — invisible. In prod (Caddy → SvelteKit Node adapter → Spring on 8080) it's still one extra in-VPS hop. Sub-millisecond. Not a concern.

Recommendations

No infrastructure work needed. Let the frontend folks ship this.
One small ask: when the new action lives on /chronik, make sure the existing Caddy config doesn't have any path-specific rules on /notifications/* that would break the 301 from #285 — verify in docs/infrastructure/production-compose.md (or whichever Caddyfile is canonical) when reviewing the #285 PR, before this one starts.

Open Decisions

(none — no infra impact)

## 🛠️ Tobias Wendt — DevOps & Platform Engineer ### Observations - Pure frontend refactor. No Compose, no env vars, no secrets, no CI workflow changes. Backend endpoints unchanged per the acceptance criteria. - One CI knock-on: the proposed Playwright no-JS test (which Sara will recommend) needs `javaScriptEnabled: false` context support — already supported by Playwright out of the box, no infra change. - Form action POSTs add a hop through SvelteKit SSR (Node) before reaching Spring Boot. In dev that's localhost-to-localhost — invisible. In prod (Caddy → SvelteKit Node adapter → Spring on 8080) it's still one extra in-VPS hop. Sub-millisecond. Not a concern. ### Recommendations - No infrastructure work needed. Let the frontend folks ship this. - One small ask: when the new action lives on `/chronik`, make sure the existing Caddy config doesn't have any path-specific rules on `/notifications/*` that would break the 301 from #285 — verify in `docs/infrastructure/production-compose.md` (or whichever Caddyfile is canonical) when reviewing the #285 PR, before this one starts. ### Open Decisions _(none — no infra impact)_

marcel commented

2026-04-20 12:49:31 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

Today the dropdown row is <button onclick={() => onMarkRead(notification)}> — a single accessible focusable element with a clean accessible name (the notification text). Wrapping it in <form><button type="submit">…</button></form> adds a <form> ancestor inside <li> inside [role="dialog"]. This is structurally fine for screen readers but worth verifying — axe and a manual VoiceOver/NVDA pass should run on the open dropdown.
The current row is a single-target click area (flex w-full cursor-pointer). The form wrapping must not break that — the <button> must remain w-full, must keep the existing min-h-[44px]-ish padding (it currently uses px-4 py-3 which yields ~50px — borderline acceptable for the senior audience), and the form itself must have display: contents or no padding/margin so it doesn't introduce a visible gap.
"Mark all as read" today is an unstyled-looking text button at the top-right. Wrapping it in a form changes nothing visually if the form is display: contents, but it changes the focus order if the form introduces an extra tab stop. Verify keyboard tab order is unchanged.
Optimistic UI matters for perceived speed. Today: click row → 100-300ms wait → close dropdown → navigate. After this refactor (per Felix's recommendation): click → instant close + navigate, mutation in-flight. That's a real UX win — make sure it actually ships, not "we'll keep current behavior to be safe."
The bell badge uses aria-live="polite". The optimistic decrement of unreadCount will still announce. Verify the form-action path doesn't cause a double-announce (one optimistic, one on response).

Recommendations

Use display: contents on the form so it has no layout impact:

<li>
  <form action="/chronik?/dismiss-notification" method="POST" use:enhance={...} class="contents">
    <input type="hidden" name="notificationId" value={notification.id} />
    <button type="submit" class="flex w-full ...">…</button>
  </form>
</li>

This keeps the existing visual + interaction design intact. Touch target stays 100% width × ~50px.

Bump padding from py-3 to py-3.5 (14px) while we're here — gets us to 52px row height, comfortably above the 44px WCAG 2.2 minimum and friendlier for the 60+ audience. This is a one-line change; do it as part of the refactor or skip it, but don't half-do it.
Run axe-playwright on the open dropdown at 320 / 768 / 1440 in both light and dark mode. The form wrapping is the kind of change that occasionally adds an "interactive controls must not be nested" violation if a button inside the form receives role="button" from Tailwind reset — verify, don't assume.
Keyboard test (manual, 30 seconds): open dropdown with bell → Tab through rows → Enter on a row → confirm focus returns to the bell after navigation. Today this works because the button has direct focus; with forms, browsers may move focus to the body during submit. The closeDropdown() → bellButtonEl?.focus() path in NotificationBell.svelte must run reliably from the use:enhance callback.
Preserve aria-label on the bell button — the unread-count branch (stream.unreadCount > 0 ? unread label : default) must still flip correctly when the optimistic update runs. Add a test that asserts the aria-label changes after the optimistic mutation, before the server response.

Open Decisions

(none — all changes are concrete a11y/UX preservation work)

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - Today the dropdown row is `<button onclick={() => onMarkRead(notification)}>` — a single accessible focusable element with a clean accessible name (the notification text). Wrapping it in `<form><button type="submit">…</button></form>` adds a `<form>` ancestor inside `<li>` inside `[role="dialog"]`. This is structurally fine for screen readers but worth verifying — axe and a manual VoiceOver/NVDA pass should run on the open dropdown. - The current row is a single-target click area (`flex w-full cursor-pointer`). The form wrapping must not break that — the `<button>` must remain `w-full`, must keep the existing `min-h-[44px]`-ish padding (it currently uses `px-4 py-3` which yields ~50px — borderline acceptable for the senior audience), and the form itself must have `display: contents` or no padding/margin so it doesn't introduce a visible gap. - "Mark all as read" today is an unstyled-looking text button at the top-right. Wrapping it in a form changes nothing visually if the form is `display: contents`, but it changes the focus order if the form introduces an extra tab stop. Verify keyboard tab order is unchanged. - Optimistic UI matters for perceived speed. Today: click row → 100-300ms wait → close dropdown → navigate. After this refactor (per Felix's recommendation): click → instant close + navigate, mutation in-flight. That's a real UX win — make sure it actually ships, not "we'll keep current behavior to be safe." - The bell badge uses `aria-live="polite"`. The optimistic decrement of `unreadCount` will still announce. Verify the form-action path doesn't cause a double-announce (one optimistic, one on response). ### Recommendations - **Use `display: contents` on the form** so it has no layout impact: ```svelte <li> <form action="/chronik?/dismiss-notification" method="POST" use:enhance={...} class="contents"> <input type="hidden" name="notificationId" value={notification.id} /> <button type="submit" class="flex w-full ...">…</button> </form> </li> ``` This keeps the existing visual + interaction design intact. Touch target stays 100% width × ~50px. - **Bump padding from `py-3` to `py-3.5` (14px)** while we're here — gets us to 52px row height, comfortably above the 44px WCAG 2.2 minimum and friendlier for the 60+ audience. This is a one-line change; do it as part of the refactor or skip it, but don't half-do it. - **Run axe-playwright on the open dropdown** at 320 / 768 / 1440 in both light and dark mode. The form wrapping is the kind of change that occasionally adds an "interactive controls must not be nested" violation if a button inside the form receives `role="button"` from Tailwind reset — verify, don't assume. - **Keyboard test (manual, 30 seconds)**: open dropdown with bell → Tab through rows → Enter on a row → confirm focus returns to the bell after navigation. Today this works because the button has direct focus; with forms, browsers may move focus to the body during submit. The `closeDropdown()` → `bellButtonEl?.focus()` path in `NotificationBell.svelte` must run reliably from the `use:enhance` callback. - **Preserve `aria-label` on the bell button** — the unread-count branch (`stream.unreadCount > 0 ? unread label : default`) must still flip correctly when the optimistic update runs. Add a test that asserts the aria-label changes after the optimistic mutation, before the server response. ### Open Decisions _(none — all changes are concrete a11y/UX preservation work)_

marcel referenced this issue

2026-04-20 17:34:31 +02:00

feat: unify /notifications and dashboard activity feed into a /chronik page #288

marcel referenced this issue

2026-04-20 17:37:59 +02:00

feat: unify /notifications and dashboard activity feed into a /chronik page #288

marcel referenced this issue from a commit

2026-04-20 18:23:34 +02:00

refactor(chronik): remove unused form actions and broken pagination UI

marcel referenced this issue

2026-04-20 18:23:59 +02:00

feat: unify /notifications and dashboard activity feed into a /chronik page #288