feat: notification history page (/notifications) #153

New Issue

marcel · 2026-03-29T12:44:52+02:00

marcel commented

2026-03-29 12:44:52 +02:00

Summary

Users currently have a bell dropdown (latest 10) and a dashboard widget (latest 5). There is no way to browse older notifications. This issue tracks the implementation of a dedicated /notifications route providing a full, paginated notification history.

Design Decisions

Location: `/notifications` — not the profile page

The profile page serves identity management and notification preferences. Adding a history feed there would mix three distinct concerns. A dedicated route keeps the separation clean and matches the user's mental model: "I come here to catch up, not to change settings."

Entry points

Bell dropdown → "Alle anzeigen →" link at the bottom (new)
Profile page → "Benachrichtigungsverlauf ansehen →" link below the preference toggles (new)

Screen Design

Mobile layout (320px baseline)

← Zurück zum Dashboard

BENACHRICHTIGUNGEN                    [Alle als gelesen markieren]

[Alle] [Ungelesen ②] [Erwähnung] [Antwort]   ← horizontally scrollable filter pills

┌──────────────────────────────────────────┐
│ ● Anna Müller hat Sie erwähnt            │  ← UNREAD: left-border accent + filled dot
│   „Geburtsurkunde Opa Karl"              │
│   Erwähnung · vor 2 Stunden              │
├──────────────────────────────────────────┤
│   Peter Raddatz hat geantwortet          │  ← READ: no accent border, muted bg
│   „Hochzeitsfoto 1962"                   │
│   Antwort · vor 1 Tag                    │
└──────────────────────────────────────────┘

           [  Ältere laden  ]

Desktop (≥768px): same layout, max-w-2xl mx-auto — no second column (linear reading content).

Layout & Responsive Behaviour

Follows the project's standard shell pattern:

<div class="mx-auto max-w-2xl px-4 py-8 sm:px-6 lg:px-8">

max-w-2xl — focused, linear reading; consistent with how max-w-4xl is used on person detail
px-4 sm:px-6 lg:px-8 — standard horizontal padding progression (16px → 24px → 32px)
No second column at any breakpoint — notification history is linear, not a grid

Component Spec

Notification row

Min height: 56px (mobile), 64px preferred �� exceeds 44px touch target requirement
Padding: px-4 py-4 → px-6 py-5 (md+)
Unread: border-l-[3px] border-accent + filled dot indicator (two cues, never color alone per WCAG 1.4.1)
Read: border-l-[3px] border-transparent + bg-canvas background
Full row is a <a> → /documents/{documentId}?commentId={referenceId}&annotationId={annotationId}
Clicking marks the notification as read then navigates

Row content

Line 1: actor name (font-serif font-semibold text-ink) + type badge ("Erwähnung" / "Antwort", font-sans text-xs uppercase tracking-wide bg-muted text-ink-2 px-2 py-0.5 rounded-sm)
Line 2: document title link (font-serif text-ink, hover underline decoration-accent)
Line 3: relative time (font-sans text-sm text-ink-3)
aria-label on each <a>: "{actor} hat {type} auf „{title}" — {time} — {read/unread}"

Filter pills

role="radiogroup" + role="radio" + aria-checked on each pill
Unselected: bg-muted text-ink font-sans text-sm font-medium px-3 py-2 rounded-full
Selected: bg-primary text-primary-fg font-sans text-sm font-medium px-3 py-2 rounded-full
Focus: focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2

"Alle als gelesen markieren"

Only rendered when unreadCount > 0 (no orphaned controls)
aria-label="Alle Benachrichtigungen als gelesen markieren"
Style: text-sm font-medium text-ink-2 hover:text-ink transition-colors underline decoration-accent/60 hover:decoration-accent

Empty state

Outlined bell + checkmark icon (decorative, aria-hidden)
"Keine Benachrichtigungen" heading + explanatory sentence
No CTA — nothing to do here

Pagination

Explicit "Ältere laden" button (no infinite scroll — predictable for all ages)

Theming

⚠️ All colors must use the project's existing semantic token utilities — never hardcode hex values.

The project uses semantic Tailwind utilities backed by CSS custom properties in layout.css, which swap automatically between light and dark mode.

Role	Tailwind utility	Notes
Page background	`bg-canvas`	outermost bg
Card / surface	`bg-surface`	white in light, elevated in dark
Muted background (read rows)	`bg-canvas`	sand tint in light
Badge background	`bg-muted`	subtle grey
Accent border + dot (unread)	`border-accent` / `bg-accent`	brand-mint
Primary text	`text-ink`	brand-navy
Secondary text	`text-ink-2`	muted label text
Tertiary text (timestamps)	`text-ink-3`	faintest readable level
Badge text	`text-ink-2`
Row divider	`border-line`
Selected pill bg	`bg-primary`
Selected pill text	`text-primary-fg`

Files to create / modify

File	Change
`frontend/src/routes/notifications/+page.svelte`	New — notification list UI
`frontend/src/routes/notifications/+page.server.ts`	New — load notifications (paginated), mark-all action
`frontend/src/lib/components/NotificationBell.svelte`	Add "Alle anzeigen →" link at dropdown bottom
`frontend/src/routes/profile/+page.svelte`	Add "Benachrichtigungsverlauf ansehen →" cross-link

Accessibility requirements

WCAG 1.4.1: Unread state conveyed by border-accent + dot (not color alone)
WCAG 1.4.3: All text combinations pass AA (4.5:1 minimum)
WCAG 2.4.3: Focus order: filter pills → mark-all button → notification rows
WCAG 4.1.2: Filter pills use proper ARIA radio pattern
Touch targets ≥ 44px on all interactive elements

Out of scope

Mark individual notification as unread (no use case)
Date groupings (Today / Yesterday) — relative timestamps are sufficient at family archive scale
Notification settings on this page — those stay on /profile

## Summary Users currently have a bell dropdown (latest 10) and a dashboard widget (latest 5). There is no way to browse older notifications. This issue tracks the implementation of a dedicated `/notifications` route providing a full, paginated notification history. --- ## Design Decisions ### Location: `/notifications` — not the profile page The profile page serves identity management and notification preferences. Adding a history feed there would mix three distinct concerns. A dedicated route keeps the separation clean and matches the user's mental model: "I come here to catch up, not to change settings." ### Entry points - Bell dropdown → **"Alle anzeigen →"** link at the bottom (new) - Profile page → **"Benachrichtigungsverlauf ansehen →"** link below the preference toggles (new) --- ## Screen Design ### Mobile layout (320px baseline) ``` ← Zurück zum Dashboard BENACHRICHTIGUNGEN [Alle als gelesen markieren] [Alle] [Ungelesen ②] [Erwähnung] [Antwort] ← horizontally scrollable filter pills ┌──────────────────────────────────────────┐ │ ● Anna Müller hat Sie erwähnt │ ← UNREAD: left-border accent + filled dot │ „Geburtsurkunde Opa Karl" │ │ Erwähnung · vor 2 Stunden │ ├──────────────────────────────────────────┤ │ Peter Raddatz hat geantwortet │ ← READ: no accent border, muted bg │ „Hochzeitsfoto 1962" │ │ Antwort · vor 1 Tag │ └──────────────────────────────────────────┘ [ Ältere laden ] ``` Desktop (≥768px): same layout, `max-w-2xl mx-auto` — no second column (linear reading content). --- ## Layout & Responsive Behaviour Follows the project's standard shell pattern: ```svelte <div class="mx-auto max-w-2xl px-4 py-8 sm:px-6 lg:px-8"> ``` - `max-w-2xl` — focused, linear reading; consistent with how `max-w-4xl` is used on person detail - `px-4 sm:px-6 lg:px-8` — standard horizontal padding progression (16px → 24px → 32px) - No second column at any breakpoint — notification history is linear, not a grid --- ## Component Spec ### Notification row - Min height: **56px** (mobile), 64px preferred �� exceeds 44px touch target requirement - Padding: `px-4 py-4` → `px-6 py-5` (md+) - **Unread**: `border-l-[3px] border-accent` + filled dot indicator (two cues, never color alone per WCAG 1.4.1) - **Read**: `border-l-[3px] border-transparent` + `bg-canvas` background - Full row is a `<a>` → `/documents/{documentId}?commentId={referenceId}&annotationId={annotationId}` - Clicking marks the notification as read then navigates ### Row content - Line 1: actor name (`font-serif font-semibold text-ink`) + type badge ("Erwähnung" / "Antwort", `font-sans text-xs uppercase tracking-wide bg-muted text-ink-2 px-2 py-0.5 rounded-sm`) - Line 2: document title link (`font-serif text-ink`, hover underline `decoration-accent`) - Line 3: relative time (`font-sans text-sm text-ink-3`) - `aria-label` on each `<a>`: `"{actor} hat {type} auf „{title}" — {time} — {read/unread}"` ### Filter pills - `role="radiogroup"` + `role="radio"` + `aria-checked` on each pill - Unselected: `bg-muted text-ink font-sans text-sm font-medium px-3 py-2 rounded-full` - Selected: `bg-primary text-primary-fg font-sans text-sm font-medium px-3 py-2 rounded-full` - Focus: `focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2` ### "Alle als gelesen markieren" - Only rendered when `unreadCount > 0` (no orphaned controls) - `aria-label="Alle Benachrichtigungen als gelesen markieren"` - Style: `text-sm font-medium text-ink-2 hover:text-ink transition-colors underline decoration-accent/60 hover:decoration-accent` ### Empty state - Outlined bell + checkmark icon (decorative, `aria-hidden`) - "Keine Benachrichtigungen" heading + explanatory sentence - No CTA — nothing to do here ### Pagination - Explicit **"Ältere laden"** button (no infinite scroll — predictable for all ages) --- ## Theming > ⚠️ All colors must use the project's existing semantic token utilities — never hardcode hex values. The project uses semantic Tailwind utilities backed by CSS custom properties in `layout.css`, which swap automatically between light and dark mode. | Role | Tailwind utility | Notes | |---|---|---| | Page background | `bg-canvas` | outermost bg | | Card / surface | `bg-surface` | white in light, elevated in dark | | Muted background (read rows) | `bg-canvas` | sand tint in light | | Badge background | `bg-muted` | subtle grey | | Accent border + dot (unread) | `border-accent` / `bg-accent` | brand-mint | | Primary text | `text-ink` | brand-navy | | Secondary text | `text-ink-2` | muted label text | | Tertiary text (timestamps) | `text-ink-3` | faintest readable level | | Badge text | `text-ink-2` | | | Row divider | `border-line` | | | Selected pill bg | `bg-primary` | | | Selected pill text | `text-primary-fg` | | --- ## Files to create / modify | File | Change | |---|---| | `frontend/src/routes/notifications/+page.svelte` | New — notification list UI | | `frontend/src/routes/notifications/+page.server.ts` | New — load notifications (paginated), mark-all action | | `frontend/src/lib/components/NotificationBell.svelte` | Add "Alle anzeigen →" link at dropdown bottom | | `frontend/src/routes/profile/+page.svelte` | Add "Benachrichtigungsverlauf ansehen →" cross-link | --- ## Accessibility requirements - WCAG 1.4.1: Unread state conveyed by `border-accent` + dot (not color alone) - WCAG 1.4.3: All text combinations pass AA (4.5:1 minimum) - WCAG 2.4.3: Focus order: filter pills → mark-all button → notification rows - WCAG 4.1.2: Filter pills use proper ARIA radio pattern - Touch targets ≥ 44px on all interactive elements --- ## Out of scope - Mark individual notification as unread (no use case) - Date groupings (Today / Yesterday) — relative timestamps are sufficient at family archive scale - Notification settings on this page — those stay on `/profile`

marcel added the feature notification ui labels 2026-03-29 12:44:59 +02:00

marcel commented

2026-03-29 13:09:43 +02:00

Architecture Review — @mkeller

Backend: already done (mostly)

GET /api/notifications already accepts page, size, type, and read params. POST /api/notifications/read-all and PATCH /api/notifications/{id}/read both exist. No new endpoints needed.

The only gap: NotificationDTO has no documentTitle. The row design in the spec explicitly shows it — that field needs to be added to the record and populated in NotificationService (one join to documents). That's the only backend change.

Filter pills → URL search params, not client state

The natural SvelteKit approach: filter changes call goto('?type=MENTION') or goto('?read=false'). The +page.server.ts load function reads url.searchParams and passes them straight to the API. Benefits: bookmarkable, back button works, SSR delivers the correct initial state. No $state variable needed for the active filter — derive the active pill from $page.url.searchParams.

// +page.server.ts
export const load: PageServerLoad = async ({ fetch, url }) => {
    const type = url.searchParams.get('type') ?? undefined;
    const read = url.searchParams.has('read') ? url.searchParams.get('read') === 'true' : undefined;
    const result = await api.GET('/api/notifications', {
        params: { query: { type, read, page: 0, size: 20 } }
    });
    return { notifications: result.data! };
};

"Ältere laden" → hybrid: SSR first page, client-side append

Server renders page 0. The button does a client-side fetch and appends to a local $state array. SSR for the initial view (fast, no layout shift); append UX for subsequent loads. No full navigation on load-more.

When a filter changes, goto() triggers a full server re-render — page resets to 0 automatically. No stale pagination state to manage.

Extract shared notification utilities before starting

relativeTime() and the NotificationItem type currently live inside NotificationBell.svelte. The history page needs both. Extract to $lib/notifications.ts first — otherwise you end up with a second copy and a cleanup issue later.

Unread count for the "Alle als gelesen markieren" button

The button should only render when unreadCount > 0. That count should come from the server-loaded page data, not a separate API call. Either derive it from the loaded notifications or add unreadCount to the load return. Don't add a third round-trip just to conditionally render a button.

Actual scope

Area	Change	Size
`NotificationDTO.java`	Add `documentTitle` field	Trivial
`NotificationService.java`	Populate `documentTitle` in mapping	Small
`$lib/notifications.ts`	Extract `relativeTime()` + type	Small
`NotificationBell.svelte`	Import from shared util + "Alle anzeigen →" link	Trivial
`notifications/+page.server.ts`	Load function with URL param filters	Small
`notifications/+page.svelte`	List UI, filter pills, load-more	Main work
`profile/+page.svelte`	Add cross-link	Trivial

The component spec and accessibility requirements are solid — no concerns there. The role="radiogroup" / role="radio" / aria-checked pattern for filter pills is correct; binding aria-checked to $page.url.searchParams rather than a click handler means SSR and client navigation stay in sync automatically.

## Architecture Review — @mkeller ### Backend: already done (mostly) `GET /api/notifications` already accepts `page`, `size`, `type`, and `read` params. `POST /api/notifications/read-all` and `PATCH /api/notifications/{id}/read` both exist. **No new endpoints needed.** The only gap: `NotificationDTO` has no `documentTitle`. The row design in the spec explicitly shows it — that field needs to be added to the record and populated in `NotificationService` (one join to documents). That's the only backend change. --- ### Filter pills → URL search params, not client state The natural SvelteKit approach: filter changes call `goto('?type=MENTION')` or `goto('?read=false')`. The `+page.server.ts` load function reads `url.searchParams` and passes them straight to the API. Benefits: bookmarkable, back button works, SSR delivers the correct initial state. No `$state` variable needed for the active filter — derive the active pill from `$page.url.searchParams`. ```typescript // +page.server.ts export const load: PageServerLoad = async ({ fetch, url }) => { const type = url.searchParams.get('type') ?? undefined; const read = url.searchParams.has('read') ? url.searchParams.get('read') === 'true' : undefined; const result = await api.GET('/api/notifications', { params: { query: { type, read, page: 0, size: 20 } } }); return { notifications: result.data! }; }; ``` --- ### "Ältere laden" → hybrid: SSR first page, client-side append Server renders page 0. The button does a client-side `fetch` and appends to a local `$state` array. SSR for the initial view (fast, no layout shift); append UX for subsequent loads. No full navigation on load-more. When a filter changes, `goto()` triggers a full server re-render — page resets to 0 automatically. No stale pagination state to manage. --- ### Extract shared notification utilities before starting `relativeTime()` and the `NotificationItem` type currently live inside `NotificationBell.svelte`. The history page needs both. Extract to `$lib/notifications.ts` first — otherwise you end up with a second copy and a cleanup issue later. --- ### Unread count for the "Alle als gelesen markieren" button The button should only render when `unreadCount > 0`. That count should come from the **server-loaded page data**, not a separate API call. Either derive it from the loaded notifications or add `unreadCount` to the load return. Don't add a third round-trip just to conditionally render a button. --- ### Actual scope | Area | Change | Size | |---|---|---| | `NotificationDTO.java` | Add `documentTitle` field | Trivial | | `NotificationService.java` | Populate `documentTitle` in mapping | Small | | `$lib/notifications.ts` | Extract `relativeTime()` + type | Small | | `NotificationBell.svelte` | Import from shared util + "Alle anzeigen →" link | Trivial | | `notifications/+page.server.ts` | Load function with URL param filters | Small | | `notifications/+page.svelte` | List UI, filter pills, load-more | Main work | | `profile/+page.svelte` | Add cross-link | Trivial | The component spec and accessibility requirements are solid — no concerns there. The `role="radiogroup"` / `role="radio"` / `aria-checked` pattern for filter pills is correct; binding `aria-checked` to `$page.url.searchParams` rather than a click handler means SSR and client navigation stay in sync automatically.

marcel commented

2026-03-29 13:13:08 +02:00

Test Scope — @saraholt

What changed — and what needs tests

Two backend changes, one shared frontend utility extraction, three frontend files modified, two new frontend files. Here is the full coverage plan.

Layer 1 — Static Analysis (zero cost, already running)

No new configuration needed. TypeScript strict mode will catch the documentTitle field addition automatically once npm run generate:api is re-run after the backend change. Verify:

NotificationDTO type in the generated client includes documentTitle: string
No any casts in the new $lib/notifications.ts

Layer 2 — Unit Tests

Backend: `NotificationServiceTest.java` (new test cases)

NotificationService gains a join to populate documentTitle. This is pure business logic and belongs in a unit test with Mockito.

New test cases to add:

should map documentTitle from joined document when document exists
should map documentTitle as null when notification has no linked document
should return all fields of NotificationDTO (regression — full mapping test)

Use @ExtendWith(MockitoExtension.class) — no Spring context. Mock NotificationRepository and DocumentRepository. Assert on the returned DTO with AssertJ.

Frontend: `$lib/notifications.ts` (new test file: `notifications.test.ts`)

relativeTime() is a pure function being extracted from NotificationBell.svelte. Zero network dependencies — perfect unit test target.

Test cases:

should return "gerade eben" for timestamps under 60 seconds ago
should return minutes for timestamps 1–59 minutes ago
should return hours for timestamps 1–23 hours ago
should return "gestern" for timestamps 24–48 hours ago
should return days for timestamps 2–6 days ago
should return weeks for older timestamps
should handle edge case: exactly 60 seconds
should handle edge case: exactly 24 hours

⚠️ Testability concern: these tests are deterministic only if relativeTime() accepts a now parameter. If the implementation uses Date.now() internally, it will be flaky. Ensure the function signature is relativeTime(timestamp: string, now?: Date): string — passing now in tests, defaulting to new Date() in production. Get this right before writing any production code.

Layer 3 — Integration Tests

Backend: `NotificationControllerTest.java` (add to existing `@WebMvcTest` slice)

GET /api/notifications — verify documentTitle is present in response body
GET /api/notifications?type=MENTION — verify filtering by type works
GET /api/notifications?read=false — verify filtering by read status works
GET /api/notifications?page=1&size=5 — verify pagination parameters are passed through
GET /api/notifications — verify 401 when unauthenticated

Use @WebMvcTest(NotificationController.class) + MockMvc. Mock NotificationService — persistence is tested at the repository layer.

Backend: `NotificationRepositoryTest.java` (add only if JOIN lives in the repository)

If documentTitle is fetched via a JPQL/native query on the repository, add @DataJpaTest cases with Testcontainers (PostgreSQL 16, never H2):

should return documentTitle via join for notifications with a linked document
should return null documentTitle for notifications without a linked document

Frontend: `notifications/+page.server.ts` load function (Vitest + MSW)

should pass type param to API when ?type=MENTION is in URL
should pass read=false to API when ?read=false is in URL
should pass no filter params when no search params present
should return notifications and unreadCount from API response
should throw 401 error when API returns 401
should call the API exactly once (no separate round-trip for unreadCount)

That last case enforces Markus's architecture decision: no third API call just to conditionally render the mark-all button.

Frontend: `NotificationBell.svelte` regression (Vitest + @testing-library/svelte)

After the refactor to import from $lib/notifications.ts:

should render "Alle anzeigen →" link pointing to /notifications
should render "Alle anzeigen →" link even when notification list is empty

Layer 4 — E2E Tests (Playwright)

File: frontend/e2e/notifications.spec.ts — critical journeys only.

user can navigate to notifications from bell dropdown → /notifications
checkA11y on landing

Journey 2: Filter by unread

clicking "Ungelesen" pill sets ?read=false in URL
only unread rows rendered (border-accent visible)
checkA11y with filter active

Journey 3: Mark all as read

"Alle als gelesen markieren" button visible when unreadCount > 0
clicking it removes unread indicators and hides the button
"Alle als gelesen markieren" NOT rendered when unreadCount is 0

Journey 4: Load more

"Ältere laden" button appends rows without full navigation (URL stays the same)
checkA11y after append

Journey 5: Row click → navigate and mark read

clicking an unread row navigates to /documents/{id}?commentId=...
returning to /notifications shows that row as read

Journey 6: Empty state

"Keine Benachrichtigungen" heading visible when inbox is empty
"Ältere laden" and "Alle als gelesen markieren" NOT present
checkA11y on empty state

Journey 7: Profile cross-link

/profile shows "Benachrichtigungsverlauf ansehen →" link
clicking it navigates to /notifications
checkA11y on profile page

Accessibility — runtime assertions (axe won't catch these)

Filter pill container has role="radiogroup"; each pill has role="radio" and aria-checked matches the active URL param
Each notification row <a> has aria-label in the format: "{actor} hat {type} auf „{title}" — {time} — {read/unread}"
"Alle als gelesen markieren" button has aria-label="Alle Benachrichtigungen als gelesen markieren"
Empty state bell icon has aria-hidden="true"

Visual regression

Capture at 320px, 768px, 1440px in both light and dark mode:

Default list (mixed read/unread)
Unread filter active
Empty state
After "Ältere laden" (rows appended below fold)

CI time impact

Layer	New tests	Estimated added time
Unit — frontend	~8 cases	+1–2s
Unit — backend	~3 cases	+1s
Integration — backend	~5–7 cases	+15–20s (Testcontainers reuse)
Integration — frontend	~6 cases	+3–5s
E2E	~7 journeys	+60–90s

Within budget for all layers. No load test changes needed — /notifications calls the existing GET /api/notifications which is already covered by the nightly k6 suite.

## Test Scope — @saraholt --- ## What changed — and what needs tests Two backend changes, one shared frontend utility extraction, three frontend files modified, two new frontend files. Here is the full coverage plan. --- ## Layer 1 — Static Analysis (zero cost, already running) No new configuration needed. TypeScript strict mode will catch the `documentTitle` field addition automatically once `npm run generate:api` is re-run after the backend change. Verify: - `NotificationDTO` type in the generated client includes `documentTitle: string` - No `any` casts in the new `$lib/notifications.ts` --- ## Layer 2 — Unit Tests ### Backend: `NotificationServiceTest.java` (new test cases) `NotificationService` gains a join to populate `documentTitle`. This is pure business logic and belongs in a unit test with Mockito. **New test cases to add:** ``` should map documentTitle from joined document when document exists should map documentTitle as null when notification has no linked document should return all fields of NotificationDTO (regression — full mapping test) ``` Use `@ExtendWith(MockitoExtension.class)` — no Spring context. Mock `NotificationRepository` and `DocumentRepository`. Assert on the returned DTO with AssertJ. ### Frontend: `$lib/notifications.ts` (new test file: `notifications.test.ts`) `relativeTime()` is a pure function being extracted from `NotificationBell.svelte`. Zero network dependencies — perfect unit test target. **Test cases:** ``` should return "gerade eben" for timestamps under 60 seconds ago should return minutes for timestamps 1–59 minutes ago should return hours for timestamps 1–23 hours ago should return "gestern" for timestamps 24–48 hours ago should return days for timestamps 2–6 days ago should return weeks for older timestamps should handle edge case: exactly 60 seconds should handle edge case: exactly 24 hours ``` > ⚠️ **Testability concern:** these tests are deterministic only if `relativeTime()` accepts a `now` parameter. If the implementation uses `Date.now()` internally, it will be flaky. Ensure the function signature is `relativeTime(timestamp: string, now?: Date): string` — passing `now` in tests, defaulting to `new Date()` in production. **Get this right before writing any production code.** --- ## Layer 3 — Integration Tests ### Backend: `NotificationControllerTest.java` (add to existing `@WebMvcTest` slice) ``` GET /api/notifications — verify documentTitle is present in response body GET /api/notifications?type=MENTION — verify filtering by type works GET /api/notifications?read=false — verify filtering by read status works GET /api/notifications?page=1&size=5 — verify pagination parameters are passed through GET /api/notifications — verify 401 when unauthenticated ``` Use `@WebMvcTest(NotificationController.class)` + MockMvc. Mock `NotificationService` — persistence is tested at the repository layer. ### Backend: `NotificationRepositoryTest.java` (add only if JOIN lives in the repository) If `documentTitle` is fetched via a JPQL/native query on the repository, add `@DataJpaTest` cases with Testcontainers (PostgreSQL 16, never H2): ``` should return documentTitle via join for notifications with a linked document should return null documentTitle for notifications without a linked document ``` ### Frontend: `notifications/+page.server.ts` load function (Vitest + MSW) ``` should pass type param to API when ?type=MENTION is in URL should pass read=false to API when ?read=false is in URL should pass no filter params when no search params present should return notifications and unreadCount from API response should throw 401 error when API returns 401 should call the API exactly once (no separate round-trip for unreadCount) ``` That last case enforces Markus's architecture decision: no third API call just to conditionally render the mark-all button. ### Frontend: `NotificationBell.svelte` regression (Vitest + @testing-library/svelte) After the refactor to import from `$lib/notifications.ts`: ``` should render "Alle anzeigen →" link pointing to /notifications should render "Alle anzeigen →" link even when notification list is empty ``` --- ## Layer 4 — E2E Tests (Playwright) File: `frontend/e2e/notifications.spec.ts` — critical journeys only. ### Journey 1: Navigate from bell dropdown ``` user can navigate to notifications from bell dropdown → /notifications checkA11y on landing ``` ### Journey 2: Filter by unread ``` clicking "Ungelesen" pill sets ?read=false in URL only unread rows rendered (border-accent visible) checkA11y with filter active ``` ### Journey 3: Mark all as read ``` "Alle als gelesen markieren" button visible when unreadCount > 0 clicking it removes unread indicators and hides the button "Alle als gelesen markieren" NOT rendered when unreadCount is 0 ``` ### Journey 4: Load more ``` "Ältere laden" button appends rows without full navigation (URL stays the same) checkA11y after append ``` ### Journey 5: Row click → navigate and mark read ``` clicking an unread row navigates to /documents/{id}?commentId=... returning to /notifications shows that row as read ``` ### Journey 6: Empty state ``` "Keine Benachrichtigungen" heading visible when inbox is empty "Ältere laden" and "Alle als gelesen markieren" NOT present checkA11y on empty state ``` ### Journey 7: Profile cross-link ``` /profile shows "Benachrichtigungsverlauf ansehen →" link clicking it navigates to /notifications checkA11y on profile page ``` ### Accessibility — runtime assertions (axe won't catch these) - Filter pill container has `role="radiogroup"`; each pill has `role="radio"` and `aria-checked` matches the active URL param - Each notification row `<a>` has `aria-label` in the format: `"{actor} hat {type} auf „{title}" — {time} — {read/unread}"` - "Alle als gelesen markieren" button has `aria-label="Alle Benachrichtigungen als gelesen markieren"` - Empty state bell icon has `aria-hidden="true"` ### Visual regression Capture at `320px`, `768px`, `1440px` in both light and dark mode: - Default list (mixed read/unread) - Unread filter active - Empty state - After "Ältere laden" (rows appended below fold) --- ## CI time impact | Layer | New tests | Estimated added time | |---|---|---| | Unit — frontend | ~8 cases | +1–2s | | Unit — backend | ~3 cases | +1s | | Integration — backend | ~5–7 cases | +15–20s (Testcontainers reuse) | | Integration — frontend | ~6 cases | +3–5s | | E2E | ~7 journeys | +60–90s | Within budget for all layers. No load test changes needed — `/notifications` calls the existing `GET /api/notifications` which is already covered by the nightly k6 suite.

marcel commented

2026-03-29 13:30:19 +02:00

Security Review — @marcel

Nora "NullX" Steiner · App Sec · OSWE/BSCP

FINDING 1 — Medium | CWE-807 | Filter logic gap silently drops `read` param

Location: NotificationService.java:96–107

// ❌ BAD — current logic
public Page<NotificationDTO> getNotifications(UUID userId, NotificationType type, Boolean read, Pageable pageable) {
    if (type != null && Boolean.FALSE.equals(read)) {
        return ...findByRecipientIdAndTypeAndReadFalseOrderByCreatedAtDesc(...)  // type + unread
    }
    if (type != null) {
        return ...findByRecipientIdAndTypeOrderByCreatedAtDesc(...)              // type only
    }
    return ...findByRecipientIdOrderByCreatedAtDesc(userId, pageable);           // ALL — read=false silently ignored
    // ← when type == null, the `read` param is completely ignored
}

The "Ungelesen" filter pill calls ?read=false with no type. That request falls through to the last branch and returns every notification instead of only unread ones. This isn't a confidentiality breach — but it's a logic integrity failure: the UI presents a filtered view while delivering unfiltered data.

Fix: Add the missing repository method and two missing service branches:

// In NotificationRepository:
Page<Notification> findByRecipientIdAndReadFalseOrderByCreatedAtDesc(UUID recipientId, Pageable pageable);

// In NotificationService.getNotifications():
if (type != null && Boolean.FALSE.equals(read)) {
    return notificationRepository.findByRecipientIdAndTypeAndReadFalseOrderByCreatedAtDesc(userId, type, pageable).map(this::toDTO);
}
if (type != null) {
    return notificationRepository.findByRecipientIdAndTypeOrderByCreatedAtDesc(userId, type, pageable).map(this::toDTO);
}
if (Boolean.FALSE.equals(read)) {
    return notificationRepository.findByRecipientIdAndReadFalseOrderByCreatedAtDesc(userId, pageable).map(this::toDTO);
}
return notificationRepository.findByRecipientIdOrderByCreatedAtDesc(userId, pageable).map(this::toDTO);

Detection: NotificationServiceTest — add should_return_only_unread_when_type_is_null_and_read_is_false. The test scope Sarah outlined already exercises ?read=false — just make sure it asserts the result count, not only that the call succeeds.

FINDING 2 — Low | CWE-770 | Unbounded `size` pagination parameter

Location: NotificationController.java:47–48

// ❌ BAD
@RequestParam(defaultValue = "10") int size,

// ✅ FIX — add @Validated on the controller class too
@RequestParam(defaultValue = "10") @Max(100) @Min(1) int size,

Without a cap, an authenticated user can request ?size=1000000, forcing a full table scan. Low blast radius for a family archive, but it costs nothing to add and closes the door before the history page makes this endpoint more attractive to hammer.

Detection: @WebMvcTest — add GET /api/notifications?size=200 → 400 as a negative case.

FINDING 3 — Low | CWE-20 | SSE payload parsed without runtime validation

Location: NotificationBell.svelte:150

// ❌ BAD — TypeScript cast is compile-time only; no runtime shape check
const notification = JSON.parse(e.data) as NotificationItem;
notifications = [notification, ...notifications];

If the backend emits a malformed event (serialization bug, schema evolution, future field rename), this silently inserts a broken object into notifications. A null documentId fed into goto() without a null check would produce an open redirect from backend-sourced data.

Fix: Validate the shape at the boundary. Since $lib/notifications.ts is being created as part of this issue anyway, put this guard in there:

export function parseNotificationEvent(raw: string): NotificationItem | null {
    try {
        const parsed = JSON.parse(raw);
        if (
            typeof parsed.id !== 'string' ||
            typeof parsed.documentId !== 'string' ||
            typeof parsed.actorName !== 'string' ||
            !['REPLY', 'MENTION'].includes(parsed.type)
        ) {
            console.warn('Unexpected SSE payload shape:', parsed);
            return null;
        }
        return parsed as NotificationItem;
    } catch {
        return null;
    }
}

Apply the same pattern to the history page's client-side "load more" fetch response.

FINDING 4 — Informational | CWE-79 risk in implementation — `documentTitle` field

The new documentTitle field is sourced from the documents table — user-controlled input. Current actorName is rendered safely via Svelte text interpolation ({...} auto-escapes). As long as the new history page never uses {@html documentTitle}, you're safe. The XSS surface opens the moment someone "optimises" the title rendering with {@html}.

Make this an explicit PR review gate: no {@html} on any field sourced from the notifications API.

FINDING 5 — Informational | CSRF justification — verify client-side auth path

SecurityConfig.java disables CSRF with the note that every request carries a Basic Auth header injected by hooks.server.ts. This holds for server-side load functions. But NotificationBell.svelte makes client-side fetches that don't go through hooks.server.ts:

fetch('/api/notifications?size=10')
fetch(`/api/notifications/${notification.id}/read`, { method: 'PATCH' })
fetch('/api/notifications/read-all', { method: 'POST' })
new EventSource('/api/notifications/stream')  // ← can never send custom headers

The new history page adds another client-side fetch for "load more". Before writing it: confirm whether the Vite dev server (and production SvelteKit server) proxies /api/* to the backend. If there's a same-origin proxy, all of this is fine and the CSRF comment holds. If client-side requests hit the backend cross-origin, EventSource is unauthenticated and the other fetches rely on browser Basic Auth caching (fragile). Not a new bug from this issue — but worth settling before more client-side API calls are added.

What's already correct — no action needed

Area	Status
`markRead` ownership check (`userId` must match `notification.recipient.id`)	✅ Correct
URL construction uses UUIDs only — no injection vectors in email links or navigation URLs	✅ Safe
Filter pills → URL params → typed API client → enum validation rejects unknown `type` values	✅ Safe
`markAllRead` scoped to authenticated user's ID only — no cross-user escalation	✅ Safe
`/api/notifications/stream` covered by `auth.anyRequest().authenticated()` in SecurityConfig	✅ Confirmed

Priority order for implementation

Fix Finding 1 first — the architecture comment describes URL params driving filter state, but the backend silently ignores read=false without a type. Fix the backend before building the filter pill UI.
Add @Max(100) to size — 5-minute fix, do it while touching the controller for documentTitle.
Add SSE payload validation when extracting $lib/notifications.ts — the extraction is already planned, so the guard goes in at the same time.
Verify Vite proxy config before the load-more client fetch is written.

## Security Review — @marcel *Nora "NullX" Steiner · App Sec · OSWE/BSCP* --- ### FINDING 1 — **Medium** | CWE-807 | Filter logic gap silently drops `read` param **Location:** `NotificationService.java:96–107` ```java // ❌ BAD — current logic public Page<NotificationDTO> getNotifications(UUID userId, NotificationType type, Boolean read, Pageable pageable) { if (type != null && Boolean.FALSE.equals(read)) { return ...findByRecipientIdAndTypeAndReadFalseOrderByCreatedAtDesc(...) // type + unread } if (type != null) { return ...findByRecipientIdAndTypeOrderByCreatedAtDesc(...) // type only } return ...findByRecipientIdOrderByCreatedAtDesc(userId, pageable); // ALL — read=false silently ignored // ← when type == null, the `read` param is completely ignored } ``` The "Ungelesen" filter pill calls `?read=false` with **no type**. That request falls through to the last branch and returns every notification instead of only unread ones. This isn't a confidentiality breach — but it's a **logic integrity failure**: the UI presents a filtered view while delivering unfiltered data. **Fix:** Add the missing repository method and two missing service branches: ```java // In NotificationRepository: Page<Notification> findByRecipientIdAndReadFalseOrderByCreatedAtDesc(UUID recipientId, Pageable pageable); // In NotificationService.getNotifications(): if (type != null && Boolean.FALSE.equals(read)) { return notificationRepository.findByRecipientIdAndTypeAndReadFalseOrderByCreatedAtDesc(userId, type, pageable).map(this::toDTO); } if (type != null) { return notificationRepository.findByRecipientIdAndTypeOrderByCreatedAtDesc(userId, type, pageable).map(this::toDTO); } if (Boolean.FALSE.equals(read)) { return notificationRepository.findByRecipientIdAndReadFalseOrderByCreatedAtDesc(userId, pageable).map(this::toDTO); } return notificationRepository.findByRecipientIdOrderByCreatedAtDesc(userId, pageable).map(this::toDTO); ``` **Detection:** `NotificationServiceTest` — add `should_return_only_unread_when_type_is_null_and_read_is_false`. The test scope Sarah outlined already exercises `?read=false` — just make sure it asserts the result count, not only that the call succeeds. --- ### FINDING 2 — **Low** | CWE-770 | Unbounded `size` pagination parameter **Location:** `NotificationController.java:47–48` ```java // ❌ BAD @RequestParam(defaultValue = "10") int size, // ✅ FIX — add @Validated on the controller class too @RequestParam(defaultValue = "10") @Max(100) @Min(1) int size, ``` Without a cap, an authenticated user can request `?size=1000000`, forcing a full table scan. Low blast radius for a family archive, but it costs nothing to add and closes the door before the history page makes this endpoint more attractive to hammer. **Detection:** `@WebMvcTest` — add `GET /api/notifications?size=200 → 400` as a negative case. --- ### FINDING 3 — **Low** | CWE-20 | SSE payload parsed without runtime validation **Location:** `NotificationBell.svelte:150` ```typescript // ❌ BAD — TypeScript cast is compile-time only; no runtime shape check const notification = JSON.parse(e.data) as NotificationItem; notifications = [notification, ...notifications]; ``` If the backend emits a malformed event (serialization bug, schema evolution, future field rename), this silently inserts a broken object into `notifications`. A `null` `documentId` fed into `goto()` without a null check would produce an open redirect from backend-sourced data. **Fix:** Validate the shape at the boundary. Since `$lib/notifications.ts` is being created as part of this issue anyway, put this guard in there: ```typescript export function parseNotificationEvent(raw: string): NotificationItem | null { try { const parsed = JSON.parse(raw); if ( typeof parsed.id !== 'string' || typeof parsed.documentId !== 'string' || typeof parsed.actorName !== 'string' || !['REPLY', 'MENTION'].includes(parsed.type) ) { console.warn('Unexpected SSE payload shape:', parsed); return null; } return parsed as NotificationItem; } catch { return null; } } ``` Apply the same pattern to the history page's client-side "load more" fetch response. --- ### FINDING 4 — **Informational** | CWE-79 risk in implementation — `documentTitle` field The new `documentTitle` field is sourced from the `documents` table — user-controlled input. Current `actorName` is rendered safely via Svelte text interpolation (`{...}` auto-escapes). **As long as the new history page never uses `{@html documentTitle}`, you're safe.** The XSS surface opens the moment someone "optimises" the title rendering with `{@html}`. Make this an explicit PR review gate: no `{@html}` on any field sourced from the notifications API. --- ### FINDING 5 — **Informational** | CSRF justification — verify client-side auth path `SecurityConfig.java` disables CSRF with the note that every request carries a Basic Auth header injected by `hooks.server.ts`. This holds for **server-side** load functions. But `NotificationBell.svelte` makes client-side fetches that don't go through `hooks.server.ts`: ```typescript fetch('/api/notifications?size=10') fetch(`/api/notifications/${notification.id}/read`, { method: 'PATCH' }) fetch('/api/notifications/read-all', { method: 'POST' }) new EventSource('/api/notifications/stream') // ← can never send custom headers ``` The new history page adds another client-side fetch for "load more". Before writing it: **confirm whether the Vite dev server (and production SvelteKit server) proxies `/api/*` to the backend**. If there's a same-origin proxy, all of this is fine and the CSRF comment holds. If client-side requests hit the backend cross-origin, `EventSource` is unauthenticated and the other fetches rely on browser Basic Auth caching (fragile). Not a new bug from this issue — but worth settling before more client-side API calls are added. --- ### What's already correct — no action needed | Area | Status | |---|---| | `markRead` ownership check (`userId` must match `notification.recipient.id`) | ✅ Correct | | URL construction uses UUIDs only — no injection vectors in email links or navigation URLs | ✅ Safe | | Filter pills → URL params → typed API client → enum validation rejects unknown `type` values | ✅ Safe | | `markAllRead` scoped to authenticated user's ID only — no cross-user escalation | ✅ Safe | | `/api/notifications/stream` covered by `auth.anyRequest().authenticated()` in SecurityConfig | ✅ Confirmed | --- ### Priority order for implementation 1. **Fix Finding 1 first** — the architecture comment describes URL params driving filter state, but the backend silently ignores `read=false` without a type. Fix the backend before building the filter pill UI. 2. **Add `@Max(100)` to `size`** — 5-minute fix, do it while touching the controller for `documentTitle`. 3. **Add SSE payload validation** when extracting `$lib/notifications.ts` — the extraction is already planned, so the guard goes in at the same time. 4. **Verify Vite proxy config** before the load-more client fetch is written.

marcel referenced a pull request that will close this issue

2026-03-29 14:13:22 +02:00

feat(#153): notification history page (/notifications) #155

marcel referenced this issue from a commit

2026-03-29 14:38:16 +02:00

chore(screenshots): add /notifications page proofshots for #153

marcel referenced this issue from a commit

2026-03-29 14:53:48 +02:00

chore(screenshots): fix empty-state proofshots for #153

marcel closed this issue

2026-03-29 19:12:17 +02:00

marcel referenced this issue from a commit