Split NotificationBell.svelte (316 lines) — extract dropdown + SSE stream module #200

New Issue

Closed

opened 2026-04-07 10:48:36 +02:00 by marcel · 7 comments

marcel commented 2026-04-07 10:48:36 +02:00

Owner

Copy Link

Context

NotificationBell.svelte is 316 lines with three concerns: the bell button with badge, the dropdown panel (header + notification list + footer), and the SSE EventSource stream + fetch logic.

Proposed Split

1. useNotificationStream.svelte.ts (~70 lines)

Extract SSE + fetch logic:

• fetchNotifications, fetchUnreadCount
• EventSource setup with parseNotificationEvent
• markRead, markAllRead
• State: notifications, unreadCount
• Lifecycle: onMount/onDestroy for EventSource

2. NotificationDropdown.svelte (~130 lines)

Extract the dropdown panel:

• Props: notifications, onMarkRead, onMarkAllRead, onClose, attachFirstFocusable
• Renders: header with "mark all read", notification list (or empty state), "view all" footer link
• Each notification row: type icon (REPLY/MENTION), text, time, unread dot

3. NotificationBell.svelte becomes orchestrator (~80 lines)

• Imports useNotificationStream + NotificationDropdown
• Renders: bell button with badge, conditional dropdown
• Handles: toggle, keyboard (Escape), click-outside, focus management

Acceptance Criteria

• NotificationBell.svelte under ~100 lines
• Real-time SSE notifications still arrive and increment badge
• Mark read/mark all read works
• Keyboard navigation (Escape to close) works
• Focus management (focus first item on open, return to bell on close) works
• npm run check passes

## Context `NotificationBell.svelte` is 316 lines with three concerns: the bell button with badge, the dropdown panel (header + notification list + footer), and the SSE EventSource stream + fetch logic. ## Proposed Split ### 1. `useNotificationStream.svelte.ts` (~70 lines) Extract SSE + fetch logic: - `fetchNotifications`, `fetchUnreadCount` - EventSource setup with `parseNotificationEvent` - `markRead`, `markAllRead` - State: `notifications`, `unreadCount` - Lifecycle: onMount/onDestroy for EventSource ### 2. `NotificationDropdown.svelte` (~130 lines) Extract the dropdown panel: - Props: `notifications`, `onMarkRead`, `onMarkAllRead`, `onClose`, `attachFirstFocusable` - Renders: header with "mark all read", notification list (or empty state), "view all" footer link - Each notification row: type icon (REPLY/MENTION), text, time, unread dot ### 3. `NotificationBell.svelte` becomes orchestrator (~80 lines) - Imports `useNotificationStream` + `NotificationDropdown` - Renders: bell button with badge, conditional dropdown - Handles: toggle, keyboard (Escape), click-outside, focus management ## Acceptance Criteria - [ ] NotificationBell.svelte under ~100 lines - [ ] Real-time SSE notifications still arrive and increment badge - [ ] Mark read/mark all read works - [ ] Keyboard navigation (Escape to close) works - [ ] Focus management (focus first item on open, return to bell on close) works - [ ] `npm run check` passes

marcel added the refactor label 2026-04-07 10:49:05 +02:00

marcel commented 2026-04-07 10:51:36 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Good decomposition along visual boundaries. The three-piece split (stream module, dropdown panel, orchestrator bell) maps cleanly to the "one nameable region per component" rule. A few things I'd want clarified before implementation:

Component Splitting

• The ~80-line orchestrator target for NotificationBell.svelte is reasonable, but toggle logic + keyboard handling + click-outside + focus management can grow quickly. If attachFirstFocusable involves a ref callback pattern, that alone can eat 10-15 lines. Is there a risk the orchestrator creeps past 100 lines once focus management is fully wired? Consider whether focus-trap logic belongs in a shared action or utility rather than inline.

• The issue mentions attachFirstFocusable as a prop on NotificationDropdown. This looks like an imperative callback pattern -- how does this interact with Svelte 5's $bindable and rune-based reactivity? A use:action directive on the first focusable element inside the dropdown might be cleaner than passing a callback prop.

Reactive State in useNotificationStream.svelte.ts

• The module exposes notifications and unreadCount as state. Will these be $state runes inside the .svelte.ts module? If so, the return type needs to be carefully designed -- returning raw $state objects from a module means consumers get reactive references, which is correct but worth documenting explicitly.

• markRead and markAllRead are commands that also update local state (decrement badge, update notification object). Are these optimistic updates, or do they await the server response before updating state? The acceptance criteria should specify this -- optimistic updates need rollback handling on failure.

Testing Strategy

• useNotificationStream.svelte.ts is pure logic with side effects (EventSource, fetch). This is highly testable in isolation with mocked EventSource and fetch. The issue should call out that this module gets unit tests before the visual components are touched.

• The {#each} block rendering notification rows in NotificationDropdown -- will these be keyed by notification ID? The acceptance criteria don't mention it, but it's required by project standards.

Naming

• useNotificationStream follows the React use* convention. In Svelte 5, the emerging pattern for .svelte.ts modules is either createX() or just a plain export. Consider createNotificationStream() or simply exporting the state and functions directly from the module without a wrapper function -- whichever matches the existing patterns in this codebase.

## 👨‍💻 Felix Brandt -- Senior Fullstack Developer Good decomposition along visual boundaries. The three-piece split (stream module, dropdown panel, orchestrator bell) maps cleanly to the "one nameable region per component" rule. A few things I'd want clarified before implementation: ### Component Splitting - The ~80-line orchestrator target for `NotificationBell.svelte` is reasonable, but toggle logic + keyboard handling + click-outside + focus management can grow quickly. If `attachFirstFocusable` involves a ref callback pattern, that alone can eat 10-15 lines. **Is there a risk the orchestrator creeps past 100 lines once focus management is fully wired?** Consider whether focus-trap logic belongs in a shared action or utility rather than inline. - The issue mentions `attachFirstFocusable` as a prop on `NotificationDropdown`. This looks like an imperative callback pattern -- how does this interact with Svelte 5's `$bindable` and rune-based reactivity? A `use:action` directive on the first focusable element inside the dropdown might be cleaner than passing a callback prop. ### Reactive State in `useNotificationStream.svelte.ts` - The module exposes `notifications` and `unreadCount` as state. Will these be `$state` runes inside the `.svelte.ts` module? If so, the return type needs to be carefully designed -- returning raw `$state` objects from a module means consumers get reactive references, which is correct but worth documenting explicitly. - `markRead` and `markAllRead` are commands that also update local state (decrement badge, update notification object). **Are these optimistic updates, or do they await the server response before updating state?** The acceptance criteria should specify this -- optimistic updates need rollback handling on failure. ### Testing Strategy - `useNotificationStream.svelte.ts` is pure logic with side effects (EventSource, fetch). This is highly testable in isolation with mocked `EventSource` and `fetch`. **The issue should call out that this module gets unit tests before the visual components are touched.** - The `{#each}` block rendering notification rows in `NotificationDropdown` -- will these be keyed by notification ID? The acceptance criteria don't mention it, but it's required by project standards. ### Naming - `useNotificationStream` follows the React `use*` convention. In Svelte 5, the emerging pattern for `.svelte.ts` modules is either `createX()` or just a plain export. Consider `createNotificationStream()` or simply exporting the state and functions directly from the module without a wrapper function -- whichever matches the existing patterns in this codebase.

marcel commented 2026-04-07 10:51:53 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller -- Senior Application Architect

The split is structurally sound. Extracting the SSE stream into a .svelte.ts module is the right call -- it separates transport from presentation and makes the EventSource lifecycle independently testable. A few architectural considerations:

Module Boundary Clarity

• The issue lists fetchNotifications, fetchUnreadCount, markRead, markAllRead, and EventSource setup all in useNotificationStream.svelte.ts. That's both read (SSE stream + polling) and write (mark read) in one module. Is this intentional? An alternative is to keep the stream module purely reactive (receives events, exposes state) and have the orchestrator call the mark-read API directly. The question is: does markAllRead need to update local state atomically with the server call? If yes, co-locating makes sense. If it's just a POST followed by a refetch, the orchestrator can own it.

• What happens when the SSE connection drops? The issue mentions EventSource setup but not reconnection strategy. EventSource has built-in reconnection, but the Last-Event-ID header and event deduplication need to be handled correctly on the backend. Does the backend SSE endpoint support Last-Event-ID? If not, reconnection will replay or miss events. This is worth a note in the acceptance criteria.

Transport Choice Validation

• SSE is the correct transport for server-to-client push notifications -- simpler than WebSockets, automatic reconnect, works through proxies. No concerns there. But confirm: is there only one SSE connection per browser tab, or could multiple components subscribe? If the notification stream is the only SSE consumer, the module is fine as a singleton. If other features will use SSE later, consider whether the EventSource connection should be shared at a higher level.

State Ownership

• notifications and unreadCount living in the stream module means the module is both a data source and a state store. This is fine for a single consumer (the bell), but if the notification count is ever needed elsewhere (e.g., a page title badge, a mobile nav indicator), the state would need to be lifted or the module imported in multiple places. Is there a known second consumer, or is single-consumer sufficient for now? Don't over-engineer, but worth asking.

Acceptance Criteria Gap

• The acceptance criteria don't mention SSE reconnection behavior or error states (backend down, 401 during stream). These are runtime edge cases that affect user experience. Suggest adding: "SSE reconnection works after temporary backend unavailability" and "expired session during SSE stream redirects to login or shows appropriate feedback."

## 🏗️ Markus Keller -- Senior Application Architect The split is structurally sound. Extracting the SSE stream into a `.svelte.ts` module is the right call -- it separates transport from presentation and makes the EventSource lifecycle independently testable. A few architectural considerations: ### Module Boundary Clarity - The issue lists `fetchNotifications`, `fetchUnreadCount`, `markRead`, `markAllRead`, and EventSource setup all in `useNotificationStream.svelte.ts`. That's both **read** (SSE stream + polling) and **write** (mark read) in one module. Is this intentional? An alternative is to keep the stream module purely reactive (receives events, exposes state) and have the orchestrator call the mark-read API directly. The question is: does `markAllRead` need to update local state atomically with the server call? If yes, co-locating makes sense. If it's just a POST followed by a refetch, the orchestrator can own it. - What happens when the SSE connection drops? The issue mentions `EventSource setup` but not reconnection strategy. `EventSource` has built-in reconnection, but the `Last-Event-ID` header and event deduplication need to be handled correctly on the backend. **Does the backend SSE endpoint support `Last-Event-ID`?** If not, reconnection will replay or miss events. This is worth a note in the acceptance criteria. ### Transport Choice Validation - SSE is the correct transport for server-to-client push notifications -- simpler than WebSockets, automatic reconnect, works through proxies. No concerns there. But confirm: is there only one SSE connection per browser tab, or could multiple components subscribe? If the notification stream is the only SSE consumer, the module is fine as a singleton. If other features will use SSE later, consider whether the EventSource connection should be shared at a higher level. ### State Ownership - `notifications` and `unreadCount` living in the stream module means the module is both a data source and a state store. This is fine for a single consumer (the bell), but if the notification count is ever needed elsewhere (e.g., a page title badge, a mobile nav indicator), the state would need to be lifted or the module imported in multiple places. **Is there a known second consumer, or is single-consumer sufficient for now?** Don't over-engineer, but worth asking. ### Acceptance Criteria Gap - The acceptance criteria don't mention SSE reconnection behavior or error states (backend down, 401 during stream). These are runtime edge cases that affect user experience. Suggest adding: "SSE reconnection works after temporary backend unavailability" and "expired session during SSE stream redirects to login or shows appropriate feedback."

marcel commented 2026-04-07 10:52:10 +02:00

Author

Owner

Copy Link

🧪 Sara Holt -- Senior QA Engineer

This is a pure refactor issue -- behavior must remain identical. That makes the test strategy straightforward but critical: existing behavior is the contract, and the tests must prove nothing changed.

Test Strategy for the Refactor

• Before writing any code, the existing NotificationBell should have tests that cover every acceptance criterion listed. If those tests don't exist yet, they must be written first against the current 316-line component. The refactor then runs under a green test suite that catches any regression.

• Specifically, I'd want these tests to exist before the split:

  • SSE events increment the badge count
  • markRead on a single notification updates its visual state
  • markAllRead clears all unread indicators
  • Escape key closes the dropdown
  • Focus moves to first item on open, returns to bell on close
  • Click outside closes the dropdown

Unit Tests for useNotificationStream.svelte.ts

• This module is the highest-value test target. It has pure logic with mockable boundaries (EventSource, fetch). Tests should cover:
  • Initial fetch populates notifications and unreadCount
  • SSE message event updates state correctly
  • parseNotificationEvent handles malformed events gracefully (doesn't throw, logs warning)
  • markRead updates local state (optimistic or after response -- whichever is chosen)
  • markAllRead resets unreadCount to 0
  • EventSource cleanup on destroy (no leaked connections)

Component Tests for NotificationDropdown.svelte

• Props-in, events-out pattern makes this easy to test in isolation:
  • Renders notification list when notifications are non-empty
  • Shows empty state when notifications array is empty
  • Calls onMarkRead with correct notification ID on click
  • Calls onMarkAllRead when header action is clicked
  • Calls onClose when "view all" link is clicked (if applicable)

Edge Cases to Cover

• What happens when notifications is empty and the user opens the dropdown? The issue mentions "empty state" but doesn't describe what it looks like. The test should assert specific empty-state text.
• What happens when SSE delivers a notification while the dropdown is open? Does the list update live, or only on next open? This is a UX decision that needs a test either way.
• Rapid toggle (open/close/open quickly) -- does focus management still land correctly?

Acceptance Criteria Suggestion

• Add: "npm run test passes with no new skipped or disabled tests" -- this ensures the refactor doesn't leave behind @Disabled or .skip() tests.

## 🧪 Sara Holt -- Senior QA Engineer This is a pure refactor issue -- behavior must remain identical. That makes the test strategy straightforward but critical: existing behavior is the contract, and the tests must prove nothing changed. ### Test Strategy for the Refactor - **Before writing any code**, the existing `NotificationBell` should have tests that cover every acceptance criterion listed. If those tests don't exist yet, they must be written first against the current 316-line component. The refactor then runs under a green test suite that catches any regression. - Specifically, I'd want these tests to exist before the split: - SSE events increment the badge count - `markRead` on a single notification updates its visual state - `markAllRead` clears all unread indicators - Escape key closes the dropdown - Focus moves to first item on open, returns to bell on close - Click outside closes the dropdown ### Unit Tests for `useNotificationStream.svelte.ts` - This module is the highest-value test target. It has pure logic with mockable boundaries (EventSource, fetch). Tests should cover: - Initial fetch populates `notifications` and `unreadCount` - SSE message event updates state correctly - `parseNotificationEvent` handles malformed events gracefully (doesn't throw, logs warning) - `markRead` updates local state (optimistic or after response -- whichever is chosen) - `markAllRead` resets `unreadCount` to 0 - EventSource cleanup on destroy (no leaked connections) ### Component Tests for `NotificationDropdown.svelte` - Props-in, events-out pattern makes this easy to test in isolation: - Renders notification list when notifications are non-empty - Shows empty state when notifications array is empty - Calls `onMarkRead` with correct notification ID on click - Calls `onMarkAllRead` when header action is clicked - Calls `onClose` when "view all" link is clicked (if applicable) ### Edge Cases to Cover - **What happens when `notifications` is empty and the user opens the dropdown?** The issue mentions "empty state" but doesn't describe what it looks like. The test should assert specific empty-state text. - **What happens when SSE delivers a notification while the dropdown is open?** Does the list update live, or only on next open? This is a UX decision that needs a test either way. - **Rapid toggle (open/close/open quickly)** -- does focus management still land correctly? ### Acceptance Criteria Suggestion - Add: "`npm run test` passes with no new skipped or disabled tests" -- this ensures the refactor doesn't leave behind `@Disabled` or `.skip()` tests.

marcel commented 2026-04-07 10:52:26 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner -- Application Security Engineer

Refactors don't change attack surface -- in theory. In practice, moving SSE and fetch logic into a separate module is a good moment to audit how that logic handles untrusted input. A few things to verify:

SSE Event Parsing

• parseNotificationEvent is called out explicitly. How does it handle unexpected or malicious event data? If the SSE backend is compromised or a MITM injects crafted events, the parser must not blindly trust the payload. Specifically:
  • Does it validate the event structure (expected fields, expected types) before updating state?
  • Does it use JSON.parse with a schema check, or does it spread raw parsed data into state?
  • If notification text is rendered as HTML anywhere in NotificationDropdown, that's a stored XSS vector via the SSE stream. Confirm all notification content is rendered as text, not {@html}.

Authentication on SSE Endpoint

• EventSource sends cookies automatically, which is correct for session-based auth. But:
  • What happens when the session expires mid-stream? Does the backend close the SSE connection with a specific event, or does EventSource silently reconnect and get a 401? If it reconnects to a 401, the browser retries indefinitely (EventSource default). The module should detect authentication failure and stop retrying.
  • Does the SSE endpoint enforce the same @RequirePermission checks as the REST notification endpoints?

Mark-Read API Calls

• markRead(notificationId) and markAllRead() presumably call REST endpoints. Confirm:
  • These endpoints validate that the authenticated user owns the notification (no IDOR -- user A marking user B's notifications as read)
  • The notification ID is a UUID, not a sequential integer (sequential IDs are enumerable)

CORS and CSP

• SSE connections are subject to CORS. If the frontend and backend are on different origins in development (port 3000 vs 8080), confirm the CORS config allows EventSource connections with credentials. This likely already works if the proxy is set up correctly, but worth a smoke test after the refactor.

Suggestion

• When extracting useNotificationStream.svelte.ts, add a guard in the EventSource onmessage handler that validates the event data shape before applying it to state. A simple type guard function (3-5 lines) prevents malformed events from corrupting the UI state.

## 🔒 Nora "NullX" Steiner -- Application Security Engineer Refactors don't change attack surface -- in theory. In practice, moving SSE and fetch logic into a separate module is a good moment to audit how that logic handles untrusted input. A few things to verify: ### SSE Event Parsing - `parseNotificationEvent` is called out explicitly. **How does it handle unexpected or malicious event data?** If the SSE backend is compromised or a MITM injects crafted events, the parser must not blindly trust the payload. Specifically: - Does it validate the event structure (expected fields, expected types) before updating state? - Does it use `JSON.parse` with a schema check, or does it spread raw parsed data into state? - If notification text is rendered as HTML anywhere in `NotificationDropdown`, that's a stored XSS vector via the SSE stream. Confirm all notification content is rendered as text, not `{@html}`. ### Authentication on SSE Endpoint - EventSource sends cookies automatically, which is correct for session-based auth. But: - **What happens when the session expires mid-stream?** Does the backend close the SSE connection with a specific event, or does EventSource silently reconnect and get a 401? If it reconnects to a 401, the browser retries indefinitely (EventSource default). The module should detect authentication failure and stop retrying. - Does the SSE endpoint enforce the same `@RequirePermission` checks as the REST notification endpoints? ### Mark-Read API Calls - `markRead(notificationId)` and `markAllRead()` presumably call REST endpoints. Confirm: - These endpoints validate that the authenticated user owns the notification (no IDOR -- user A marking user B's notifications as read) - The notification ID is a UUID, not a sequential integer (sequential IDs are enumerable) ### CORS and CSP - SSE connections are subject to CORS. If the frontend and backend are on different origins in development (port 3000 vs 8080), confirm the CORS config allows EventSource connections with credentials. This likely already works if the proxy is set up correctly, but worth a smoke test after the refactor. ### Suggestion - When extracting `useNotificationStream.svelte.ts`, add a guard in the EventSource `onmessage` handler that validates the event data shape before applying it to state. A simple type guard function (3-5 lines) prevents malformed events from corrupting the UI state.

marcel commented 2026-04-07 10:52:41 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss -- Senior UX Designer & Accessibility Strategist

The split itself is invisible to users -- good. But it's also an opportunity to audit the notification dropdown's accessibility and interaction design, since the components are being touched anyway.

Focus Management

• The acceptance criteria mention "focus first item on open, return to bell on close" -- this is the minimum. A few follow-up questions:
  • Is the dropdown a role="menu" with role="menuitem" children, or a role="listbox"? The correct ARIA role determines which keyboard interactions users expect (arrow keys for menu, different behavior for listbox).
  • Can the user navigate notification items with arrow keys inside the dropdown? If not, Tab is the only way to move through items, which is slower for keyboard users with many notifications.
  • Is there a role="status" or aria-live="polite" region that announces new notifications to screen readers? The badge count changing is a visual-only cue. Screen reader users need an announcement like "3 new notifications" when the count changes via SSE.

Touch Targets

• Each notification row in the dropdown needs a minimum 44x44px touch target. If rows contain both a "mark read" action and a "navigate to source" action, those two targets must be distinct and not overlap. How close are these interactive areas in the current design?

Empty State

• The issue mentions "empty state" but doesn't specify what it says. For the dual audience (millennials + seniors):
  • Avoid vague text like "Nothing here" -- use "Keine neuen Benachrichtigungen" (or the Paraglide equivalent)
  • Consider whether the empty state should include a subtle illustration or just text
  • The empty state must be announced to screen readers when the dropdown opens to an empty list

Dropdown Positioning

• On mobile viewports (< 768px), does the dropdown render as a full-width panel or a positioned popover? If it's a popover, it can easily overflow the viewport on small screens. The refactor is a good moment to confirm the dropdown has max-height with scroll and doesn't extend below the viewport fold.

Notification Type Icons

• The issue mentions "type icon (REPLY/MENTION)" per notification row. Confirm these icons have:
  • aria-hidden="true" if there's an adjacent text label conveying the type
  • A visible text label or tooltip for users who can't distinguish icon shapes
  • Sufficient contrast against the row background (both read and unread states)

## 🎨 Leonie Voss -- Senior UX Designer & Accessibility Strategist The split itself is invisible to users -- good. But it's also an opportunity to audit the notification dropdown's accessibility and interaction design, since the components are being touched anyway. ### Focus Management - The acceptance criteria mention "focus first item on open, return to bell on close" -- this is the minimum. A few follow-up questions: - **Is the dropdown a `role="menu"` with `role="menuitem"` children, or a `role="listbox"`?** The correct ARIA role determines which keyboard interactions users expect (arrow keys for menu, different behavior for listbox). - **Can the user navigate notification items with arrow keys inside the dropdown?** If not, Tab is the only way to move through items, which is slower for keyboard users with many notifications. - **Is there a `role="status"` or `aria-live="polite"` region that announces new notifications to screen readers?** The badge count changing is a visual-only cue. Screen reader users need an announcement like "3 new notifications" when the count changes via SSE. ### Touch Targets - Each notification row in the dropdown needs a minimum 44x44px touch target. If rows contain both a "mark read" action and a "navigate to source" action, those two targets must be distinct and not overlap. **How close are these interactive areas in the current design?** ### Empty State - The issue mentions "empty state" but doesn't specify what it says. For the dual audience (millennials + seniors): - Avoid vague text like "Nothing here" -- use "Keine neuen Benachrichtigungen" (or the Paraglide equivalent) - Consider whether the empty state should include a subtle illustration or just text - The empty state must be announced to screen readers when the dropdown opens to an empty list ### Dropdown Positioning - On mobile viewports (< 768px), does the dropdown render as a full-width panel or a positioned popover? If it's a popover, it can easily overflow the viewport on small screens. **The refactor is a good moment to confirm the dropdown has `max-height` with scroll and doesn't extend below the viewport fold.** ### Notification Type Icons - The issue mentions "type icon (REPLY/MENTION)" per notification row. Confirm these icons have: - `aria-hidden="true"` if there's an adjacent text label conveying the type - A visible text label or tooltip for users who can't distinguish icon shapes - Sufficient contrast against the row background (both read and unread states)

marcel commented 2026-04-07 10:52:56 +02:00

Author

Owner

Copy Link

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Pure frontend refactor -- no infrastructure changes needed. But SSE has operational characteristics worth considering, especially since the stream logic is being extracted into a standalone module.

SSE Connection Lifecycle

• EventSource connections are long-lived HTTP connections. Each open browser tab holds one connection to the backend. Does the Spring Boot backend use async/non-blocking SSE (e.g., SseEmitter with a virtual thread or reactive stream)? If it uses a servlet thread per connection, each open tab consumes a thread from the Jetty thread pool. With the default Jetty pool size (~200), this limits concurrent users with the notification panel.

• After the refactor, confirm that onDestroy in the stream module reliably calls eventSource.close(). A leaked EventSource connection per page navigation would accumulate connections on the backend and eventually exhaust resources.

Proxy Behavior

• SSE through Caddy (production) works out of the box -- Caddy handles HTTP/1.1 streaming correctly. But confirm the SvelteKit dev proxy (Vite) also passes SSE through without buffering. Vite's proxy sometimes buffers chunked responses. If SSE is proxied through the SvelteKit server in development, verify responseType and buffering settings.

Monitoring

• The stream module is now a clear boundary for adding observability. Consider:
  • Logging SSE reconnection events (helps diagnose intermittent backend issues)
  • Counting reconnections -- if a client reconnects more than N times in M seconds, something is wrong and it should back off
  • The backend SSE endpoint should be included in health/readiness checks if it's critical to user experience

CI Impact

• No new services or containers needed. The acceptance criterion "npm run check passes" is correct. Suggest also running npm run test in CI to catch regressions from the split. If there are existing E2E tests covering notification behavior, those are the highest-value regression signal here.

Bundle Size

• Extracting a .svelte.ts module and a new .svelte component shouldn't change the bundle size meaningfully, but worth a quick check with npm run build before and after to confirm no accidental dependency pulls.

## ⚙️ Tobias Wendt -- DevOps & Platform Engineer Pure frontend refactor -- no infrastructure changes needed. But SSE has operational characteristics worth considering, especially since the stream logic is being extracted into a standalone module. ### SSE Connection Lifecycle - EventSource connections are long-lived HTTP connections. Each open browser tab holds one connection to the backend. **Does the Spring Boot backend use async/non-blocking SSE (e.g., `SseEmitter` with a virtual thread or reactive stream)?** If it uses a servlet thread per connection, each open tab consumes a thread from the Jetty thread pool. With the default Jetty pool size (~200), this limits concurrent users with the notification panel. - After the refactor, confirm that `onDestroy` in the stream module reliably calls `eventSource.close()`. A leaked EventSource connection per page navigation would accumulate connections on the backend and eventually exhaust resources. ### Proxy Behavior - SSE through Caddy (production) works out of the box -- Caddy handles HTTP/1.1 streaming correctly. But confirm the SvelteKit dev proxy (Vite) also passes SSE through without buffering. Vite's proxy sometimes buffers chunked responses. If SSE is proxied through the SvelteKit server in development, verify `responseType` and buffering settings. ### Monitoring - The stream module is now a clear boundary for adding observability. Consider: - Logging SSE reconnection events (helps diagnose intermittent backend issues) - Counting reconnections -- if a client reconnects more than N times in M seconds, something is wrong and it should back off - The backend SSE endpoint should be included in health/readiness checks if it's critical to user experience ### CI Impact - No new services or containers needed. The acceptance criterion "`npm run check` passes" is correct. Suggest also running `npm run test` in CI to catch regressions from the split. If there are existing E2E tests covering notification behavior, those are the highest-value regression signal here. ### Bundle Size - Extracting a `.svelte.ts` module and a new `.svelte` component shouldn't change the bundle size meaningfully, but worth a quick check with `npm run build` before and after to confirm no accidental dependency pulls.

marcel commented 2026-04-07 11:06:01 +02:00

Author

Owner

Copy Link

Comprehensive Response to Review Comments

I read the full source of NotificationBell.svelte (316 lines), notifications.ts (42 lines), notifications.spec.ts (106 lines), NotificationController.java, SseEmitterRegistry.java, NotificationService.java, the notifications page, and vite.config.ts. Findings below, organized by persona.

---

Felix Brandt -- Component Splitting & Rune Design

Orchestrator line count risk: Confirmed realistic. The current orchestrator-scope code (toggle, close, keydown, attachBellButton, attachClickOutside, plus the bell button template) is ~75 lines. attachFirstFocusable is 6 lines (lines 106-111) -- it's a simple ref-capture attachment returning a cleanup function, not a complex ref callback. The 80-100 line target holds.

attachFirstFocusable pattern: Currently implemented as an attachment function (lines 106-111) that captures a DOM ref. The dropdown then uses {@attach attachFirstFocusable} on the "mark all read" button (line 203). This is already the Svelte-idiomatic approach -- it's equivalent to a use:action directive. No callback prop is needed; the attachment lives in the orchestrator and is passed down via {@attach}. After the split, NotificationDropdown would expose a slot or accept an attachment prop for its first focusable element. A use:action on the first focusable inside the dropdown is a clean alternative -- agreed.

$state runes in .svelte.ts module: This would be the first .svelte.ts module in the codebase (no existing ones found via glob). The return type should be explicit: { notifications: NotificationItem[], unreadCount: number, markRead: ..., markAllRead: ... } where the state fields are reactive $state values. Consumers get reactive references automatically -- this is correct Svelte 5 behavior but worth a brief inline comment since it's the first such module.

Optimistic updates: Both markRead (line 61-76) and markAllRead (line 78-88) currently do await-then-update -- they await fetch(...) and only update local state after a successful response. No rollback needed because state isn't changed optimistically. The catch blocks log errors but don't update state. This is the safe choice. Acceptance criteria should note: "mark-read updates are applied after server confirmation, not optimistically."

Naming: Agree that useNotificationStream follows React convention. Since no .svelte.ts modules exist yet, this sets the precedent. createNotificationStream() is more Svelte-idiomatic. Alternatively, a bare module export pattern (no wrapper function) works too.

Keyed {#each}: Already keyed by notification ID at line 236: {#each notifications as notification (notification.id)}. This carries over to NotificationDropdown.

---

Markus Keller -- Architecture & SSE Lifecycle

Read+write co-location: markRead and markAllRead both mutate local state atomically with the server response (decrement unreadCount, set notification.read = true). Splitting writes to the orchestrator would require the orchestrator to reach into the module's state to update it, breaking encapsulation. Co-location in the stream module is the correct choice here.

SSE reconnection / Last-Event-ID: The backend SseEmitterRegistry (line 18) creates emitters with new SseEmitter(0L) (no timeout). It does not send event IDs -- SseEmitter.event().name("notification").data(data) at line 30 has no .id() call. Therefore Last-Event-ID is not supported. EventSource will auto-reconnect on disconnect, but missed events during the gap are lost. The frontend mitigates this partially: toggleDropdown() calls fetchNotifications() every time the dropdown opens (line 47), so the user gets a fresh list on interaction. However, the badge count (unreadCount) could drift if events are missed during a reconnect gap. Recommendation for acceptance criteria: Add "on EventSource reconnect, re-fetch unreadCount to resync badge" -- this can be done in the onopen handler.

Singleton vs shared EventSource: SseEmitterRegistry stores one emitter per userId (line 19, ConcurrentHashMap<UUID, SseEmitter>). Opening a second tab replaces the previous emitter (line 19, emitters.put overwrites). This means only the most recently opened tab receives SSE events. This is a pre-existing limitation, not introduced by this refactor, but worth noting. For this issue, single-consumer (the bell) is sufficient.

Second consumer: The notifications page (/notifications/+page.svelte) does NOT use SSE -- it loads via server-side data and client-side fetch for pagination. No second SSE consumer exists. Single-consumer design is fine.

---

Sara Holt -- Test Coverage

Existing tests: frontend/src/lib/utils/notifications.spec.ts has 106 lines covering:

• relativeTime: 8 tests (all time buckets, default now parameter)
• parseNotificationEvent: 7 tests (valid payload, invalid JSON, missing id/documentId/actorName, unknown type, valid REPLY type)

Missing pre-refactor tests: No component-level tests exist for NotificationBell.svelte. The vitest config (vite.config.ts lines 56-69) has a browser-based test project for .svelte.test.ts files using Playwright, so component tests are supported. Before the refactor, these tests should be written:

• SSE event via mocked EventSource increments badge count
• markRead calls PATCH /api/notifications/{id}/read and updates visual state
• markAllRead calls POST /api/notifications/read-all and clears unread indicators
• Escape key closes dropdown
• Focus moves to first item on open (currently "mark all read" button, line 203)
• Focus returns to bell button on close (line 58)
• Click outside closes dropdown

Empty state: The empty state renders m.notification_empty() (line 232) with a bell icon. The Paraglide key exists. Test should assert this specific text appears when notifications array is empty.

Live update while open: Currently, SSE events prepend to notifications array (line 134) regardless of dropdown state. The list updates live if the dropdown is open. This should be tested.

Acceptance criteria addition: Agree -- add "npm run test passes with no .skip() or disabled tests."

---

Nora Steiner -- Security

parseNotificationEvent validation: Solid. Lines 29-34 of notifications.ts validate: id is string, documentId is string, actorName is string, type is one of ['REPLY', 'MENTION']. Returns null on any validation failure or JSON parse error. The onmessage handler (line 132-133) checks if (!notification) return before using the result. No blindly-spread raw data.

{@html} usage: None. Grep confirms zero {@html} in NotificationBell.svelte. All notification content is rendered as text via {notification.actorName} and Paraglide message functions. No XSS vector.

Session expiry mid-stream: The backend SSE endpoint (/api/notifications/stream) relies on Spring Security session auth. If the session expires, EventSource auto-reconnects and gets a 302 redirect to the login page (or a 401). The browser's EventSource implementation will retry indefinitely on non-2xx responses. The current code (lines 128-141) has no onerror handler on the EventSource. This is a gap. The stream module should add an onerror handler that detects repeated failures (e.g., 3 consecutive errors) and calls eventSource.close() to stop retrying. Alternatively, check eventSource.readyState and stop if the server consistently rejects.

IDOR on mark-read: Protected. NotificationService.markRead() (line 133-138 of NotificationService.java) checks notification.getRecipient().getId().equals(userId) and throws DomainException.forbidden() if the notification belongs to a different user. Notification IDs are UUIDs (not sequential).

CORS/CSP: In development, the Vite proxy at /api (vite.config.ts line 16-33) handles all API calls including SSE. The proxy injects the Authorization header from the auth_token cookie. SSE goes through the same proxy path, so no separate CORS issue. In production, frontend and backend are same-origin behind Caddy.

---

Leonie Voss -- Accessibility

ARIA roles: The dropdown uses role="dialog" with aria-modal="true" (line 191-193). The notification list uses role="list" (line 235). This is acceptable but role="dialog" with aria-modal="true" implies a focus trap, which is not implemented -- Tab can escape the dropdown. Two options: (a) implement a focus trap, or (b) change to role="menu" with role="menuitem" children, which better matches the interaction pattern (list of actionable items). Recommend option (b) since the dropdown behaves like a menu.

Arrow-key navigation: Not implemented. Users must Tab through items. With potentially 10 notification rows, this is slow. If switching to role="menu", arrow-key navigation between menuitem children is expected per WAI-ARIA. This should be added during the refactor since the dropdown component is being extracted anyway.

aria-live for badge: Present. Line 179: aria-live="polite" with aria-atomic="true" on the unread count badge. Screen readers will announce count changes. However, the badge is inside an {#if unreadCount > 0} block (line 177), so when the count goes from 0 to 1, the element is newly inserted into the DOM. Most screen readers will announce this via aria-live, but some may not catch the initial insertion. Moving aria-live to a persistent wrapper that always exists (even when count is 0, just visually hidden) would be more reliable.

Touch targets: Each notification row is a full-width button (line 239-298) with py-3 padding. At default font size, rows are approximately 56px tall -- above the 44px minimum. There's no separate "mark read" button per row; clicking the row both marks read and navigates. Single touch target per row -- no overlap concern.

Empty state: Renders m.notification_empty() with an icon (aria-hidden="true"). The text is announced. Accessible.

Dropdown positioning / mobile overflow: The dropdown is w-80 (320px) absolutely positioned right-0 (line 194). On viewports narrower than 320px + bell button offset, it will overflow left. No max-height with scroll is set on the notification list -- with 10 items, the dropdown could be ~500px tall. Recommend adding max-h-[24rem] overflow-y-auto to the <ul> or the dropdown body to prevent viewport overflow on mobile.

Type icons: Both icons (lines 248-278) are wrapped in <span aria-hidden="true"> (line 245). The type is conveyed via text in the adjacent <p> element using Paraglide messages (notification_type_reply / notification_type_mention). Icons are decorative. Correct.

---

Tobias Wendt -- DevOps & SSE Operations

SSE thread model: SseEmitterRegistry uses Spring's SseEmitter which is async -- it does NOT hold a servlet thread per connection. The SseEmitter is returned from the controller method, freeing the Jetty thread. Events are sent asynchronously via emitter.send() (line 30 of SseEmitterRegistry.java). The 0L timeout (line 18) means no server-side timeout. This is safe for thread pool exhaustion. Each open tab holds one HTTP connection (kernel socket) but no Jetty thread.

EventSource cleanup on navigation: onDestroy (lines 139-141) calls eventSource?.close(). In SvelteKit, onDestroy fires when the component is unmounted. Since NotificationBell lives in the global layout, it's only destroyed on full page unload (not client-side navigation). This is correct -- the bell persists across navigations, so the SSE connection stays open. No leaked connections from SPA navigation.

Vite proxy buffering: The Vite dev proxy config (vite.config.ts lines 16-33) uses http-proxy under the hood. By default, http-proxy does NOT buffer chunked/streaming responses -- it streams them through. SSE works through the Vite proxy without additional config. No ws: true or special streaming flags are needed for SSE (those are for WebSockets). If buffering issues arise, adding headers: { 'X-Accel-Buffering': 'no' } to the proxy config would fix it, but this is unlikely needed.

Monitoring: Agree that the extracted stream module is a good boundary for logging reconnection events. The SseEmitterRegistry already logs send failures at debug level (line 32). Client-side reconnection logging can be added in the onerror handler (which should be added anyway per Nora's session-expiry concern).

CI impact: npm run check and npm run test should both be in CI for this refactor. No new services needed. Bundle size impact is negligible -- splitting a single component into three files doesn't add imports.

---

Summary of Recommended Acceptance Criteria Additions

1. Add onerror handler on EventSource to detect auth failure / repeated errors and stop retrying
2. Re-fetch unreadCount on EventSource reconnect (onopen) to resync badge after missed events
3. Add max-h-[24rem] overflow-y-auto to dropdown list for mobile viewport safety
4. Pre-refactor component tests must be green before splitting (Sara's list above)
5. npm run test passes with no .skip() tests
6. Consider role="menu" + arrow-key navigation instead of role="dialog" (can be a follow-up issue)
7. Move aria-live region to a persistent wrapper outside the {#if} block for reliable screen reader announcement

## Comprehensive Response to Review Comments I read the full source of `NotificationBell.svelte` (316 lines), `notifications.ts` (42 lines), `notifications.spec.ts` (106 lines), `NotificationController.java`, `SseEmitterRegistry.java`, `NotificationService.java`, the notifications page, and `vite.config.ts`. Findings below, organized by persona. --- ### Felix Brandt -- Component Splitting & Rune Design **Orchestrator line count risk:** Confirmed realistic. The current orchestrator-scope code (toggle, close, keydown, `attachBellButton`, `attachClickOutside`, plus the bell button template) is ~75 lines. `attachFirstFocusable` is 6 lines (lines 106-111) -- it's a simple ref-capture attachment returning a cleanup function, not a complex ref callback. The 80-100 line target holds. **`attachFirstFocusable` pattern:** Currently implemented as an attachment function (lines 106-111) that captures a DOM ref. The dropdown then uses `{@attach attachFirstFocusable}` on the "mark all read" button (line 203). This is already the Svelte-idiomatic approach -- it's equivalent to a `use:action` directive. No callback prop is needed; the attachment lives in the orchestrator and is passed down via `{@attach}`. After the split, `NotificationDropdown` would expose a slot or accept an attachment prop for its first focusable element. A `use:action` on the first focusable inside the dropdown is a clean alternative -- agreed. **`$state` runes in `.svelte.ts` module:** This would be the first `.svelte.ts` module in the codebase (no existing ones found via glob). The return type should be explicit: `{ notifications: NotificationItem[], unreadCount: number, markRead: ..., markAllRead: ... }` where the state fields are reactive `$state` values. Consumers get reactive references automatically -- this is correct Svelte 5 behavior but worth a brief inline comment since it's the first such module. **Optimistic updates:** Both `markRead` (line 61-76) and `markAllRead` (line 78-88) currently do **await-then-update** -- they `await fetch(...)` and only update local state after a successful response. No rollback needed because state isn't changed optimistically. The `catch` blocks log errors but don't update state. This is the safe choice. Acceptance criteria should note: "mark-read updates are applied after server confirmation, not optimistically." **Naming:** Agree that `useNotificationStream` follows React convention. Since no `.svelte.ts` modules exist yet, this sets the precedent. `createNotificationStream()` is more Svelte-idiomatic. Alternatively, a bare module export pattern (no wrapper function) works too. **Keyed `{#each}`:** Already keyed by notification ID at line 236: `{#each notifications as notification (notification.id)}`. This carries over to `NotificationDropdown`. --- ### Markus Keller -- Architecture & SSE Lifecycle **Read+write co-location:** `markRead` and `markAllRead` both mutate local state atomically with the server response (decrement `unreadCount`, set `notification.read = true`). Splitting writes to the orchestrator would require the orchestrator to reach into the module's state to update it, breaking encapsulation. Co-location in the stream module is the correct choice here. **SSE reconnection / `Last-Event-ID`:** The backend `SseEmitterRegistry` (line 18) creates emitters with `new SseEmitter(0L)` (no timeout). It does **not** send event IDs -- `SseEmitter.event().name("notification").data(data)` at line 30 has no `.id()` call. Therefore `Last-Event-ID` is not supported. `EventSource` will auto-reconnect on disconnect, but missed events during the gap are lost. The frontend mitigates this partially: `toggleDropdown()` calls `fetchNotifications()` every time the dropdown opens (line 47), so the user gets a fresh list on interaction. However, the badge count (`unreadCount`) could drift if events are missed during a reconnect gap. **Recommendation for acceptance criteria:** Add "on EventSource reconnect, re-fetch `unreadCount` to resync badge" -- this can be done in the `onopen` handler. **Singleton vs shared EventSource:** `SseEmitterRegistry` stores one emitter per userId (line 19, `ConcurrentHashMap<UUID, SseEmitter>`). Opening a second tab replaces the previous emitter (line 19, `emitters.put` overwrites). This means only the most recently opened tab receives SSE events. This is a pre-existing limitation, not introduced by this refactor, but worth noting. For this issue, single-consumer (the bell) is sufficient. **Second consumer:** The notifications page (`/notifications/+page.svelte`) does NOT use SSE -- it loads via server-side `data` and client-side `fetch` for pagination. No second SSE consumer exists. Single-consumer design is fine. --- ### Sara Holt -- Test Coverage **Existing tests:** `frontend/src/lib/utils/notifications.spec.ts` has 106 lines covering: - `relativeTime`: 8 tests (all time buckets, default `now` parameter) - `parseNotificationEvent`: 7 tests (valid payload, invalid JSON, missing `id`/`documentId`/`actorName`, unknown type, valid REPLY type) **Missing pre-refactor tests:** No component-level tests exist for `NotificationBell.svelte`. The vitest config (vite.config.ts lines 56-69) has a browser-based test project for `.svelte.test.ts` files using Playwright, so component tests are supported. Before the refactor, these tests should be written: - SSE event via mocked EventSource increments badge count - `markRead` calls `PATCH /api/notifications/{id}/read` and updates visual state - `markAllRead` calls `POST /api/notifications/read-all` and clears unread indicators - Escape key closes dropdown - Focus moves to first item on open (currently "mark all read" button, line 203) - Focus returns to bell button on close (line 58) - Click outside closes dropdown **Empty state:** The empty state renders `m.notification_empty()` (line 232) with a bell icon. The Paraglide key exists. Test should assert this specific text appears when notifications array is empty. **Live update while open:** Currently, SSE events prepend to `notifications` array (line 134) regardless of dropdown state. The list updates live if the dropdown is open. This should be tested. **Acceptance criteria addition:** Agree -- add "`npm run test` passes with no `.skip()` or disabled tests." --- ### Nora Steiner -- Security **`parseNotificationEvent` validation:** Solid. Lines 29-34 of `notifications.ts` validate: `id` is string, `documentId` is string, `actorName` is string, `type` is one of `['REPLY', 'MENTION']`. Returns `null` on any validation failure or JSON parse error. The `onmessage` handler (line 132-133) checks `if (!notification) return` before using the result. No blindly-spread raw data. **`{@html}` usage:** None. Grep confirms zero `{@html}` in `NotificationBell.svelte`. All notification content is rendered as text via `{notification.actorName}` and Paraglide message functions. No XSS vector. **Session expiry mid-stream:** The backend SSE endpoint (`/api/notifications/stream`) relies on Spring Security session auth. If the session expires, `EventSource` auto-reconnects and gets a 302 redirect to the login page (or a 401). The browser's `EventSource` implementation will retry indefinitely on non-2xx responses. The current code (lines 128-141) has no `onerror` handler on the EventSource. **This is a gap.** The stream module should add an `onerror` handler that detects repeated failures (e.g., 3 consecutive errors) and calls `eventSource.close()` to stop retrying. Alternatively, check `eventSource.readyState` and stop if the server consistently rejects. **IDOR on mark-read:** Protected. `NotificationService.markRead()` (line 133-138 of NotificationService.java) checks `notification.getRecipient().getId().equals(userId)` and throws `DomainException.forbidden()` if the notification belongs to a different user. Notification IDs are UUIDs (not sequential). **CORS/CSP:** In development, the Vite proxy at `/api` (vite.config.ts line 16-33) handles all API calls including SSE. The proxy injects the `Authorization` header from the `auth_token` cookie. SSE goes through the same proxy path, so no separate CORS issue. In production, frontend and backend are same-origin behind Caddy. --- ### Leonie Voss -- Accessibility **ARIA roles:** The dropdown uses `role="dialog"` with `aria-modal="true"` (line 191-193). The notification list uses `role="list"` (line 235). This is acceptable but `role="dialog"` with `aria-modal="true"` implies a focus trap, which is not implemented -- Tab can escape the dropdown. Two options: (a) implement a focus trap, or (b) change to `role="menu"` with `role="menuitem"` children, which better matches the interaction pattern (list of actionable items). Recommend option (b) since the dropdown behaves like a menu. **Arrow-key navigation:** Not implemented. Users must Tab through items. With potentially 10 notification rows, this is slow. If switching to `role="menu"`, arrow-key navigation between `menuitem` children is expected per WAI-ARIA. This should be added during the refactor since the dropdown component is being extracted anyway. **`aria-live` for badge:** Present. Line 179: `aria-live="polite"` with `aria-atomic="true"` on the unread count badge. Screen readers will announce count changes. However, the badge is inside an `{#if unreadCount > 0}` block (line 177), so when the count goes from 0 to 1, the element is newly inserted into the DOM. Most screen readers will announce this via `aria-live`, but some may not catch the initial insertion. Moving `aria-live` to a persistent wrapper that always exists (even when count is 0, just visually hidden) would be more reliable. **Touch targets:** Each notification row is a full-width button (line 239-298) with `py-3` padding. At default font size, rows are approximately 56px tall -- above the 44px minimum. There's no separate "mark read" button per row; clicking the row both marks read and navigates. Single touch target per row -- no overlap concern. **Empty state:** Renders `m.notification_empty()` with an icon (`aria-hidden="true"`). The text is announced. Accessible. **Dropdown positioning / mobile overflow:** The dropdown is `w-80` (320px) absolutely positioned `right-0` (line 194). On viewports narrower than 320px + bell button offset, it will overflow left. No `max-height` with scroll is set on the notification list -- with 10 items, the dropdown could be ~500px tall. **Recommend adding `max-h-[24rem] overflow-y-auto`** to the `<ul>` or the dropdown body to prevent viewport overflow on mobile. **Type icons:** Both icons (lines 248-278) are wrapped in `<span aria-hidden="true">` (line 245). The type is conveyed via text in the adjacent `<p>` element using Paraglide messages (`notification_type_reply` / `notification_type_mention`). Icons are decorative. Correct. --- ### Tobias Wendt -- DevOps & SSE Operations **SSE thread model:** `SseEmitterRegistry` uses Spring's `SseEmitter` which is async -- it does NOT hold a servlet thread per connection. The `SseEmitter` is returned from the controller method, freeing the Jetty thread. Events are sent asynchronously via `emitter.send()` (line 30 of SseEmitterRegistry.java). The `0L` timeout (line 18) means no server-side timeout. This is safe for thread pool exhaustion. Each open tab holds one HTTP connection (kernel socket) but no Jetty thread. **EventSource cleanup on navigation:** `onDestroy` (lines 139-141) calls `eventSource?.close()`. In SvelteKit, `onDestroy` fires when the component is unmounted. Since `NotificationBell` lives in the global layout, it's only destroyed on full page unload (not client-side navigation). This is correct -- the bell persists across navigations, so the SSE connection stays open. No leaked connections from SPA navigation. **Vite proxy buffering:** The Vite dev proxy config (vite.config.ts lines 16-33) uses `http-proxy` under the hood. By default, `http-proxy` does NOT buffer chunked/streaming responses -- it streams them through. SSE works through the Vite proxy without additional config. No `ws: true` or special streaming flags are needed for SSE (those are for WebSockets). If buffering issues arise, adding `headers: { 'X-Accel-Buffering': 'no' }` to the proxy config would fix it, but this is unlikely needed. **Monitoring:** Agree that the extracted stream module is a good boundary for logging reconnection events. The `SseEmitterRegistry` already logs send failures at debug level (line 32). Client-side reconnection logging can be added in the `onerror` handler (which should be added anyway per Nora's session-expiry concern). **CI impact:** `npm run check` and `npm run test` should both be in CI for this refactor. No new services needed. Bundle size impact is negligible -- splitting a single component into three files doesn't add imports. --- ### Summary of Recommended Acceptance Criteria Additions 1. Add `onerror` handler on EventSource to detect auth failure / repeated errors and stop retrying 2. Re-fetch `unreadCount` on EventSource reconnect (`onopen`) to resync badge after missed events 3. Add `max-h-[24rem] overflow-y-auto` to dropdown list for mobile viewport safety 4. Pre-refactor component tests must be green before splitting (Sara's list above) 5. `npm run test` passes with no `.skip()` tests 6. Consider `role="menu"` + arrow-key navigation instead of `role="dialog"` (can be a follow-up issue) 7. Move `aria-live` region to a persistent wrapper outside the `{#if}` block for reliable screen reader announcement

marcel referenced this issue from a commit 2026-04-15 14:58:02 +02:00

refactor(notifications): extract useNotificationStream and NotificationDropdown from NotificationBell (#200)

marcel referenced this issue 2026-04-15 15:00:31 +02:00

refactor(frontend): utility dedup, component splits, dead code removal (#193–#200) #241

marcel closed this issue 2026-04-15 21:02:12 +02:00

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

feat/issue-533-mass-import-ux

worktree-feat+issue-557-upload-artifact-v3-pin

worktree-chore+issue-556-drop-client-branches-coverage-gate

fix/issue-514-prerender-crawl-bakes-redirects

fix/issue-472-prerender-entries

feat/issue-395-readme

feat/issue-345-bulk-mark-reviewed

feat/issue-344-bell-tooltip

feat/issue-341-annotieren-contrast

feat/issue-225-bulk-metadata-edit

feat/issue-317-bulk-upload

feat/issue-271-dashboard-redesign

docs/issue-240-mission-control-spec

refactor/issues-193-200

feat/issue-162-korrespondenz-redesign

feature/68-new-document-file-first

feat/81-discussion-discoverability

feat/62-document-bottom-panel

feature/56-backfill-file-hashes

feat/38-document-edit-history

fix/svelte5-test-delegation-and-login-auth

No results found.

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label refactor

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#200