fix(test): NotificationDropdown view-all click navigates iframe — breaks vitest coverage #551

New Issue

marcel · 2026-05-12T16:16:25+02:00

marcel commented

2026-05-12 16:16:25 +02:00

Problem

NotificationDropdown.svelte.test.ts contains a test that clicks the "View All" <a href="/aktivitaeten"> link:

// test line 172
await viewAllLink.click();

Despite the component correctly calling e.preventDefault() in handleViewAll, SvelteKit's own global link interceptor (running inside the vitest-browser dev server) fires before the component handler and initiates a client-side navigation to /aktivitaeten. The iframe leaves the vitest session URL, and vitest reports:

Caused by: Error: Cannot connect to the iframe. Did you change the location or submitted a form?
Received URL: http://localhost:63315/aktivitaeten
Expected: http://localhost:63315/?sessionId=...&iframeId=.../NotificationDropdown.svelte.test.ts

Impact

Every test file that runs after NotificationDropdown.svelte.test.ts in the same browser session loses its iframe connection and reports 0 % coverage. This drops the Istanbul client-coverage from the expected > 80 % down to ≈ 58 %, causing the CI Unit & Component Tests job to fail on the coverage threshold gate.

The birpc guard does not catch this — the failure mode is a stale iframe URL, not a closed RPC channel.

Root cause

handleViewAll calls e.preventDefault() on the component's own click event, but SvelteKit registers a document-level click listener that intercepts <a> clicks before the component handler runs and pushes a history entry / kicks off the SvelteKit router. The mock vi.mock('$app/navigation', ...) only replaces the imported goto symbol — it does not disable SvelteKit's internal router.

Fix options

Option A — Change `<a>` to `<button>` in the component (recommended)

Replace the anchor with a button that calls goto('/aktivitaeten') programmatically. No href attribute → no default navigation → SvelteKit's link interceptor never fires → goto mock is sufficient.

<!-- NotificationDropdown.svelte -->
<button
  type="button"
  onclick={handleViewAll}
  class="text-xs font-medium text-ink-2 transition-colors hover:text-ink"
>
  {m.chronik_view_all()}
</button>

function handleViewAll() {
  onClose();
  goto('/aktivitaeten');
}

The test becomes clean: click the button, assert onClose and goto were called, no navigation happens.

Trade-off: Loses native link semantics (right-click → "Open in new tab", browser history prefetch). Acceptable since this is a UI widget button inside a dropdown dialog, not a standalone navigation link.

Option B — Keep `<a>`, intercept navigation in the test

Use the underlying Playwright page object to abort the /aktivitaeten route before clicking:

import { page } from '@vitest/browser/context';

it('calls onClose when the view-all link is clicked', async () => {
  const onClose = vi.fn();
  render(NotificationDropdown, { props: { ..., onClose } });

  await page.playwright.route('**/aktivitaeten', route => route.abort());

  const viewAllLink = page.getByRole('link', { name: /alle aktivitäten/i });
  await expect.element(viewAllLink).toHaveAttribute('href', '/aktivitaeten');
  await viewAllLink.click();

  expect(onClose).toHaveBeenCalledOnce();
  expect(goto).toHaveBeenCalledWith('/aktivitaeten');

  await page.playwright.unroute('**/aktivitaeten');
});

Trade-off: Keeps the semantic <a> in the component. Test is more complex and couples to Playwright internals. The page.playwright escape hatch may break on vitest-browser upgrades.

Option C — Split the test, skip the click

The href assertion already proves the link points to the right URL. Replace the click + goto assertion with a direct event dispatch that bypasses SvelteKit's interceptor:

it('calls onClose when the view-all link is clicked', async () => {
  const onClose = vi.fn();
  render(NotificationDropdown, { props: { ..., onClose } });

  const link = document.querySelector('a[href="/aktivitaeten"]')!;
  // Dispatch a cancelable event — handleViewAll calls e.preventDefault(), so no navigation
  link.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }));

  expect(onClose).toHaveBeenCalledOnce();
  expect(goto).toHaveBeenCalledWith('/aktivitaeten');
});

Trade-off: dispatchEvent bypasses Playwright's auto-wait and real user gesture simulation. Tests the handler but not the full browser-click path.

Recommendation

Option A. The "View All" link lives inside a dropdown dialog (role="dialog"). Buttons inside dialogs that trigger programmatic navigation are a normal pattern. Losing "open in new tab" for a notification bell item is an acceptable trade-off for test stability. The component already uses goto() everywhere else for in-app navigation.

Acceptance criteria

NotificationDropdown.svelte.test.ts passes without any iframe navigation error
npm run test (browser project) reports no Cannot connect to the iframe errors
Client-coverage lines/functions/statements are ≥ 80 % in CI

## Problem `NotificationDropdown.svelte.test.ts` contains a test that clicks the "View All" `<a href="/aktivitaeten">` link: ```typescript // test line 172 await viewAllLink.click(); ``` Despite the component correctly calling `e.preventDefault()` in `handleViewAll`, SvelteKit's own global link interceptor (running inside the vitest-browser dev server) fires **before** the component handler and initiates a client-side navigation to `/aktivitaeten`. The iframe leaves the vitest session URL, and vitest reports: ``` Caused by: Error: Cannot connect to the iframe. Did you change the location or submitted a form? Received URL: http://localhost:63315/aktivitaeten Expected: http://localhost:63315/?sessionId=...&iframeId=.../NotificationDropdown.svelte.test.ts ``` ### Impact Every test file that runs **after** `NotificationDropdown.svelte.test.ts` in the same browser session loses its iframe connection and reports 0 % coverage. This drops the Istanbul client-coverage from the expected > 80 % down to ≈ 58 %, causing the CI `Unit & Component Tests` job to fail on the coverage threshold gate. The birpc guard does **not** catch this — the failure mode is a stale iframe URL, not a closed RPC channel. --- ## Root cause `handleViewAll` calls `e.preventDefault()` on the component's own click event, but SvelteKit registers a document-level click listener that intercepts `<a>` clicks before the component handler runs and pushes a history entry / kicks off the SvelteKit router. The mock `vi.mock('$app/navigation', ...)` only replaces the imported `goto` symbol — it does not disable SvelteKit's internal router. --- ## Fix options ### Option A — Change `<a>` to `<button>` in the component (recommended) Replace the anchor with a button that calls `goto('/aktivitaeten')` programmatically. No `href` attribute → no default navigation → SvelteKit's link interceptor never fires → `goto` mock is sufficient. ```svelte  <button type="button" onclick={handleViewAll} class="text-xs font-medium text-ink-2 transition-colors hover:text-ink" > {m.chronik_view_all()} </button> ``` ```typescript function handleViewAll() { onClose(); goto('/aktivitaeten'); } ``` The test becomes clean: click the button, assert `onClose` and `goto` were called, no navigation happens. **Trade-off:** Loses native link semantics (right-click → "Open in new tab", browser history prefetch). Acceptable since this is a UI widget button inside a dropdown dialog, not a standalone navigation link. --- ### Option B — Keep `<a>`, intercept navigation in the test Use the underlying Playwright `page` object to abort the `/aktivitaeten` route before clicking: ```typescript import { page } from '@vitest/browser/context'; it('calls onClose when the view-all link is clicked', async () => { const onClose = vi.fn(); render(NotificationDropdown, { props: { ..., onClose } }); await page.playwright.route('**/aktivitaeten', route => route.abort()); const viewAllLink = page.getByRole('link', { name: /alle aktivitäten/i }); await expect.element(viewAllLink).toHaveAttribute('href', '/aktivitaeten'); await viewAllLink.click(); expect(onClose).toHaveBeenCalledOnce(); expect(goto).toHaveBeenCalledWith('/aktivitaeten'); await page.playwright.unroute('**/aktivitaeten'); }); ``` **Trade-off:** Keeps the semantic `<a>` in the component. Test is more complex and couples to Playwright internals. The `page.playwright` escape hatch may break on vitest-browser upgrades. --- ### Option C — Split the test, skip the click The href assertion already proves the link points to the right URL. Replace the click + goto assertion with a direct event dispatch that bypasses SvelteKit's interceptor: ```typescript it('calls onClose when the view-all link is clicked', async () => { const onClose = vi.fn(); render(NotificationDropdown, { props: { ..., onClose } }); const link = document.querySelector('a[href="/aktivitaeten"]')!; // Dispatch a cancelable event — handleViewAll calls e.preventDefault(), so no navigation link.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); expect(onClose).toHaveBeenCalledOnce(); expect(goto).toHaveBeenCalledWith('/aktivitaeten'); }); ``` **Trade-off:** `dispatchEvent` bypasses Playwright's auto-wait and real user gesture simulation. Tests the handler but not the full browser-click path. --- ## Recommendation **Option A.** The "View All" link lives inside a dropdown dialog (`role="dialog"`). Buttons inside dialogs that trigger programmatic navigation are a normal pattern. Losing "open in new tab" for a notification bell item is an acceptable trade-off for test stability. The component already uses `goto()` everywhere else for in-app navigation. --- ## Acceptance criteria - `NotificationDropdown.svelte.test.ts` passes without any iframe navigation error - `npm run test` (browser project) reports no `Cannot connect to the iframe` errors - Client-coverage lines/functions/statements are ≥ 80 % in CI

marcel added the P1-high bug notification test labels 2026-05-12 16:16:32 +02:00

marcel commented

2026-05-12 16:21:09 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The root cause is a semantic conflict baked into the component: <a href="/aktivitaeten" onclick={handleViewAll}> declares a navigation destination but immediately overrides it. In the production SPA this works because SvelteKit's own router handles the pushState gracefully — but in the vitest-browser environment SvelteKit registers its link interceptor as a capture-phase document listener, which fires before the component's onclick handler (bubble phase). By the time handleViewAll calls e.preventDefault(), the router has already initiated navigation. The current e.preventDefault() is structurally too late.
Option A is the right call. The established pattern in this codebase for "button that calls goto()" is already used in NotificationBell.svelte:41 (goto(url) from a plain function), BulkSelectionBar.svelte:12, and BulkDocumentEditLayout.svelte:146–211. There is no <a href onclick=e.preventDefault()> pattern anywhere else. This change makes NotificationDropdown consistent.
The handleViewAll(e: MouseEvent) signature needs to change: drop the e parameter entirely since e.preventDefault() is no longer needed. The function becomes a plain zero-arg handler.
Test line 171 — toHaveAttribute('href', '/aktivitaeten') — becomes dead code after Option A. Remove it. The navigation destination is already covered by expect(goto).toHaveBeenCalledWith('/aktivitaeten') on line 175.
The single it() block at line 159–176 asserts two behaviors (onClose + goto). Per the one-reason-to-fail rule these should be two separate tests — but this is a refactor concern, not a blocker for the fix.

Recommendations

Implement Option A. Change the anchor to:

<button
  type="button"
  onclick={handleViewAll}
  class="text-xs font-medium text-ink-2 transition-colors hover:text-ink"
>
  {m.chronik_view_all()}
</button>

And simplify handleViewAll:

function handleViewAll() {
  onClose();
  goto('/aktivitaeten');
}

Remove toHaveAttribute('href', '/aktivitaeten') from the test — it no longer applies. The goto mock assertion already covers the navigation target.
Optionally split the view-all test into two it() blocks: 'calls onClose when view-all is clicked' and 'navigates to /aktivitaeten when view-all is clicked'. This keeps the one-assertion-per-test discipline without adding test surface.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The root cause is a semantic conflict baked into the component: `<a href="/aktivitaeten" onclick={handleViewAll}>` declares a navigation destination but immediately overrides it. In the production SPA this works because SvelteKit's own router handles the pushState gracefully — but in the `vitest-browser` environment SvelteKit registers its link interceptor as a **capture-phase** document listener, which fires before the component's `onclick` handler (bubble phase). By the time `handleViewAll` calls `e.preventDefault()`, the router has already initiated navigation. The current `e.preventDefault()` is structurally too late. - Option A is the right call. The established pattern in this codebase for "button that calls `goto()`" is already used in `NotificationBell.svelte:41` (`goto(url)` from a plain function), `BulkSelectionBar.svelte:12`, and `BulkDocumentEditLayout.svelte:146–211`. There is no `<a href onclick=e.preventDefault()>` pattern anywhere else. This change makes `NotificationDropdown` consistent. - The `handleViewAll(e: MouseEvent)` signature needs to change: drop the `e` parameter entirely since `e.preventDefault()` is no longer needed. The function becomes a plain zero-arg handler. - Test line 171 — `toHaveAttribute('href', '/aktivitaeten')` — becomes dead code after Option A. Remove it. The navigation destination is already covered by `expect(goto).toHaveBeenCalledWith('/aktivitaeten')` on line 175. - The single `it()` block at line 159–176 asserts two behaviors (onClose + goto). Per the one-reason-to-fail rule these should be two separate tests — but this is a refactor concern, not a blocker for the fix. ### Recommendations - **Implement Option A.** Change the anchor to: ```svelte <button type="button" onclick={handleViewAll} class="text-xs font-medium text-ink-2 transition-colors hover:text-ink" > {m.chronik_view_all()} </button> ``` And simplify `handleViewAll`: ```typescript function handleViewAll() { onClose(); goto('/aktivitaeten'); } ``` - Remove `toHaveAttribute('href', '/aktivitaeten')` from the test — it no longer applies. The `goto` mock assertion already covers the navigation target. - Optionally split the view-all test into two `it()` blocks: `'calls onClose when view-all is clicked'` and `'navigates to /aktivitaeten when view-all is clicked'`. This keeps the one-assertion-per-test discipline without adding test surface.

marcel commented

2026-05-12 16:21:20 +02:00

🏛️ Markus Keller — Application Architect

Observations

The real architectural problem here is not a test configuration gap — it is a semantic mismatch in the component. An <a href> element carries a contract with the browser: "this is a navigation link to the given URL." Overriding that contract with e.preventDefault() → goto() is a known fragile pattern, particularly in environments where the router registers a capture-phase link interceptor (SvelteKit's current design). Using e.preventDefault() to suppress anchor navigation while calling goto() instead only works reliably when the component's handler runs before the interceptor — which isn't guaranteed across all rendering environments.
The role="dialog" context is the architectural signal. Dialog widgets contain actions, not standalone navigation links. The distinction matters:
- Navigation link (<a href>): user should be able to right-click, open in new tab, bookmark, share URL. The link is the destination.
- Widget action (<button>): triggers a behavior (close + navigate). The behavior is the point; the navigation is a side effect. "View All" inside a notification dropdown is clearly the latter.
Option A resolves the mismatch by using the semantically correct element. This is not a test workaround — it is fixing the component to say what it means.
No ADR needed. This is a local component decision. The pattern is already established in NotificationBell.svelte:41 and BulkSelectionBar.svelte:12.
Options B and C both paper over the semantic mismatch rather than fixing it: B patches the test environment, C bypasses real browser interactions. Neither addresses the root cause. If the team ever changes test runners or upgrades vitest-browser, B and C would need to be revisited. Option A does not.

Recommendations

Option A is the architecturally correct fix. The component's intent is "close dialog + navigate," which is a button action. Implement it as one.
As a general rule for this codebase: any "close dialog + navigate" pattern should use <button type="button" onclick={handler}> + goto() inside the handler. An <a href onclick=e.preventDefault()> is a smell that signals this pattern was used incorrectly.

## 🏛️ Markus Keller — Application Architect ### Observations - The real architectural problem here is not a test configuration gap — it is a semantic mismatch in the component. An `<a href>` element carries a contract with the browser: "this is a navigation link to the given URL." Overriding that contract with `e.preventDefault()` → `goto()` is a known fragile pattern, particularly in environments where the router registers a capture-phase link interceptor (SvelteKit's current design). Using `e.preventDefault()` to suppress anchor navigation while calling `goto()` instead only works reliably when the component's handler runs before the interceptor — which isn't guaranteed across all rendering environments. - The `role="dialog"` context is the architectural signal. Dialog widgets contain **actions**, not standalone navigation links. The distinction matters: - **Navigation link** (`<a href>`): user should be able to right-click, open in new tab, bookmark, share URL. The link is the destination. - **Widget action** (`<button>`): triggers a behavior (close + navigate). The behavior is the point; the navigation is a side effect. "View All" inside a notification dropdown is clearly the latter. - Option A resolves the mismatch by using the semantically correct element. This is not a test workaround — it is fixing the component to say what it means. - No ADR needed. This is a local component decision. The pattern is already established in `NotificationBell.svelte:41` and `BulkSelectionBar.svelte:12`. - Options B and C both paper over the semantic mismatch rather than fixing it: B patches the test environment, C bypasses real browser interactions. Neither addresses the root cause. If the team ever changes test runners or upgrades `vitest-browser`, B and C would need to be revisited. Option A does not. ### Recommendations - **Option A is the architecturally correct fix.** The component's intent is "close dialog + navigate," which is a button action. Implement it as one. - As a general rule for this codebase: any "close dialog + navigate" pattern should use `<button type="button" onclick={handler}>` + `goto()` inside the handler. An `<a href onclick=e.preventDefault()>` is a smell that signals this pattern was used incorrectly.

marcel commented

2026-05-12 16:21:35 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The failure mode here is a P1 test contamination bug: one test file navigates the shared browser iframe, causing all subsequently-run test files to report 0% coverage. The coverage gate then sees ≈58% instead of >80% and blocks the entire CI run. This is not a "flaky test" — it is a deterministic failure that affects every PR touching any frontend component tested after NotificationDropdown.svelte.test.ts in the browser project's execution order.

Evaluating each option against the test quality criteria:

Criterion	Option A	Option B	Option C
Tests real browser behavior	✅	✅	❌ (`dispatchEvent` bypasses Playwright auto-wait)
Test is isolated from env internals	✅	❌ (`page.playwright` escape hatch)	⚠️
Works after vitest-browser upgrades	✅	⚠️	⚠️
Root cause fixed	✅	❌	❌
Test stays clean	✅	More boilerplate	Less reliable

Option B introduces a page.playwright.route() / page.playwright.unroute() pair that is implementation-coupled to vitest-browser-svelte's internal page object. The unroute cleanup is easy to forget and creates inter-test state if omitted. This is the kind of test debt that accumulates silently.
Option C's element.dispatchEvent(new MouseEvent(...)) bypasses Playwright's real user gesture simulation — which is the entire value proposition of vitest-browser-svelte over JSDOM. Using it here would defeat the purpose of browser-mode testing for this specific interaction.
After Option A, the test at line 159–176 should be split into two separate it() blocks per the one-logical-assertion rule. Right now a single failure would give you "calls onClose when view-all link is clicked" failing, but you wouldn't immediately know whether it was the onClose call or the goto call that broke.
The href assertion (toHaveAttribute('href', '/aktivitaeten')) must be removed — the button has no href. The navigation intent is now solely expressed by expect(goto).toHaveBeenCalledWith('/aktivitaeten'), which is already present.

Recommendations

Option A is the correct fix. It resolves the root cause, keeps tests clean, and requires no test-environment workarounds.

After implementing, update the test:

it('calls onClose when the view-all button is clicked', async () => {
  const onClose = vi.fn();
  render(NotificationDropdown, { props: { notifications: [], onMarkRead: () => {}, onMarkAllRead: () => {}, onClose } });
  await page.getByRole('button', { name: /alle aktivitäten|view all/i }).click();
  expect(onClose).toHaveBeenCalledOnce();
});

it('navigates to /aktivitaeten when the view-all button is clicked', async () => {
  render(NotificationDropdown, { props: { notifications: [], onMarkRead: () => {}, onMarkAllRead: () => {}, onClose: () => {} } });
  await page.getByRole('button', { name: /alle aktivitäten|view all/i }).click();
  expect(goto).toHaveBeenCalledWith('/aktivitaeten');
});

Post-fix: run npm run test with the browser project and confirm (a) no iframe errors and (b) coverage ≥ 80% across the full suite.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - The failure mode here is a **P1 test contamination bug**: one test file navigates the shared browser iframe, causing all subsequently-run test files to report 0% coverage. The coverage gate then sees ≈58% instead of >80% and blocks the entire CI run. This is not a "flaky test" — it is a deterministic failure that affects every PR touching any frontend component tested after `NotificationDropdown.svelte.test.ts` in the browser project's execution order. - Evaluating each option against the test quality criteria: | Criterion | Option A | Option B | Option C | |---|---|---|---| | Tests real browser behavior | ✅ | ✅ | ❌ (`dispatchEvent` bypasses Playwright auto-wait) | | Test is isolated from env internals | ✅ | ❌ (`page.playwright` escape hatch) | ⚠️ | | Works after vitest-browser upgrades | ✅ | ⚠️ | ⚠️ | | Root cause fixed | ✅ | ❌ | ❌ | | Test stays clean | ✅ | More boilerplate | Less reliable | - Option B introduces a `page.playwright.route()` / `page.playwright.unroute()` pair that is implementation-coupled to `vitest-browser-svelte`'s internal `page` object. The `unroute` cleanup is easy to forget and creates inter-test state if omitted. This is the kind of test debt that accumulates silently. - Option C's `element.dispatchEvent(new MouseEvent(...))` bypasses Playwright's real user gesture simulation — which is the entire value proposition of `vitest-browser-svelte` over JSDOM. Using it here would defeat the purpose of browser-mode testing for this specific interaction. - After Option A, the test at line 159–176 should be split into two separate `it()` blocks per the one-logical-assertion rule. Right now a single failure would give you "calls onClose when view-all link is clicked" failing, but you wouldn't immediately know whether it was the `onClose` call or the `goto` call that broke. - The `href` assertion (`toHaveAttribute('href', '/aktivitaeten')`) must be removed — the button has no `href`. The navigation intent is now solely expressed by `expect(goto).toHaveBeenCalledWith('/aktivitaeten')`, which is already present. ### Recommendations - **Option A is the correct fix.** It resolves the root cause, keeps tests clean, and requires no test-environment workarounds. - After implementing, update the test: ```typescript it('calls onClose when the view-all button is clicked', async () => { const onClose = vi.fn(); render(NotificationDropdown, { props: { notifications: [], onMarkRead: () => {}, onMarkAllRead: () => {}, onClose } }); await page.getByRole('button', { name: /alle aktivitäten|view all/i }).click(); expect(onClose).toHaveBeenCalledOnce(); }); it('navigates to /aktivitaeten when the view-all button is clicked', async () => { render(NotificationDropdown, { props: { notifications: [], onMarkRead: () => {}, onMarkAllRead: () => {}, onClose: () => {} } }); await page.getByRole('button', { name: /alle aktivitäten|view all/i }).click(); expect(goto).toHaveBeenCalledWith('/aktivitaeten'); }); ``` - Post-fix: run `npm run test` with the browser project and confirm (a) no iframe errors and (b) coverage ≥ 80% across the full suite.

marcel commented

2026-05-12 16:21:49 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

The <a> vs <button> decision has real UX consequences. An anchor element gives users browser-native affordances: right-click → "Open in new tab", middle-click, address bar preview on hover. A button gives none of these. For a standalone navigation link (e.g. a link in a list of documents), losing these affordances would be a regression. But this is not a standalone link — it is an action inside a role="dialog" notification dropdown.
The expected user interaction is: click bell → see notifications → click "View All" → dialog closes → user arrives at /aktivitaeten. No user in this flow would right-click "View All" expecting to open Aktivitäten in a new tab while keeping the notification dropdown open. The action model is correct for a <button>.
Accessibility impact of Option A: None negative.
- The button already has accessible text via m.chronik_view_all() (German: "Alle Aktivitäten" or similar).
- type="button" prevents accidental form submission if the dropdown is ever nested inside a form.
- NotificationBell.svelte:20 queries [role="dialog"] button, [role="dialog"] a for focus management — this still works with a button.
- WCAG 2.1 Success Criterion 4.1.2 (Name, Role, Value): <button> has an implicit role of button, which is correct for a widget action. ✅
Touch target: The existing Tailwind classes on the anchor (px-4 py-2 from the wrapping <div class="px-4 py-2">) should ensure the button meets the 44×44px minimum. Verify this holds after the change, since the button itself would need the padding, not just its container.
Styling note: Browsers apply default button styles (background, border, padding). Ensure the button gets appearance-none or equivalent, or that the existing utility classes (text-xs font-medium text-ink-2 transition-colors hover:text-ink) are sufficient to suppress browser defaults. In Tailwind CSS 4 with @layer base { button { ... } }, this is typically already handled.

Recommendations

Option A is correct from a UX and accessibility standpoint. "View All" inside a notification dialog is a widget action. Use <button type="button">.
Verify touch target height on the rendered button (DevTools or Playwright screenshot) — the button itself needs min-h-[44px] or equivalent, not just its container.
No ARIA changes needed. The button's text content provides the accessible name.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - The `<a>` vs `<button>` decision has real UX consequences. An anchor element gives users browser-native affordances: right-click → "Open in new tab", middle-click, address bar preview on hover. A button gives none of these. For a standalone navigation link (e.g. a link in a list of documents), losing these affordances would be a regression. But this is **not** a standalone link — it is an action inside a `role="dialog"` notification dropdown. - The expected user interaction is: click bell → see notifications → click "View All" → dialog closes → user arrives at /aktivitaeten. No user in this flow would right-click "View All" expecting to open Aktivitäten in a new tab while keeping the notification dropdown open. The action model is correct for a `<button>`. - **Accessibility impact of Option A:** None negative. - The button already has accessible text via `m.chronik_view_all()` (German: "Alle Aktivitäten" or similar). - `type="button"` prevents accidental form submission if the dropdown is ever nested inside a form. - `NotificationBell.svelte:20` queries `[role="dialog"] button, [role="dialog"] a` for focus management — this still works with a button. - WCAG 2.1 Success Criterion 4.1.2 (Name, Role, Value): `<button>` has an implicit role of `button`, which is correct for a widget action. ✅ - **Touch target:** The existing Tailwind classes on the anchor (`px-4 py-2` from the wrapping `<div class="px-4 py-2">`) should ensure the button meets the 44×44px minimum. Verify this holds after the change, since the button itself would need the padding, not just its container. - **Styling note:** Browsers apply default button styles (background, border, padding). Ensure the button gets `appearance-none` or equivalent, or that the existing utility classes (`text-xs font-medium text-ink-2 transition-colors hover:text-ink`) are sufficient to suppress browser defaults. In Tailwind CSS 4 with `@layer base { button { ... } }`, this is typically already handled. ### Recommendations - Option A is correct from a UX and accessibility standpoint. "View All" inside a notification dialog is a widget action. Use `<button type="button">`. - Verify touch target height on the rendered button (DevTools or Playwright screenshot) — the button itself needs `min-h-[44px]` or equivalent, not just its container. - No ARIA changes needed. The button's text content provides the accessible name.

marcel commented

2026-05-12 16:21:57 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Observations

I reviewed all three options for security implications. This is a test infrastructure and component semantics issue with no attack surface. Specific checks:
- Open redirect risk: The navigation target /aktivitaeten is a hardcoded string literal — not user-controlled, not derived from a prop, not read from a URL parameter or API response. No open redirect risk under any option.
- XSS risk: No user-controlled string is rendered into HTML in the affected code path. m.chronik_view_all() is a compile-time i18n call. Safe.
- goto() mock scope: The vi.mock('$app/navigation') in tests correctly scopes the mock to the test module. The mock does not leak into production bundles.
- Event handler security: The handleViewAll function calls onClose() (a prop callback) and goto() (mocked in tests, SvelteKit router in production). Neither has an injection surface.
Option B's page.playwright.route() intercept aborts the navigation at the network layer — this is a test-only mechanism and carries no production security implications.

Recommendations

No security objections to Option A, B, or C. Option A is preferred on technical grounds (already well-argued above), with no security trade-offs either way.
No concerns from my angle on this issue.

## 🔐 Nora "NullX" Steiner — Application Security Engineer ### Observations - I reviewed all three options for security implications. This is a test infrastructure and component semantics issue with no attack surface. Specific checks: - **Open redirect risk:** The navigation target `/aktivitaeten` is a hardcoded string literal — not user-controlled, not derived from a prop, not read from a URL parameter or API response. No open redirect risk under any option. - **XSS risk:** No user-controlled string is rendered into HTML in the affected code path. `m.chronik_view_all()` is a compile-time i18n call. Safe. - **`goto()` mock scope:** The `vi.mock('$app/navigation')` in tests correctly scopes the mock to the test module. The mock does not leak into production bundles. - **Event handler security:** The `handleViewAll` function calls `onClose()` (a prop callback) and `goto()` (mocked in tests, SvelteKit router in production). Neither has an injection surface. - Option B's `page.playwright.route()` intercept aborts the navigation at the network layer — this is a test-only mechanism and carries no production security implications. ### Recommendations - No security objections to Option A, B, or C. Option A is preferred on technical grounds (already well-argued above), with no security trade-offs either way. - No concerns from my angle on this issue.

marcel commented

2026-05-12 16:22:07 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

The primary CI impact is clear: one test file's iframe navigation contaminates the entire browser project's coverage run, dropping measured coverage from the expected >80% to ≈58%. This blocks the Unit & Component Tests job and therefore every PR's merge gate — regardless of what the PR touches.
The birpc guard does not catch this note in the issue is important from an observability standpoint. The error surfaces as Cannot connect to the iframe — a runtime error in the test runner, not a test assertion failure. This means the coverage failure is a secondary symptom of a primary connection error, which makes root-cause diagnosis harder from CI logs alone.
Scope of the underlying risk: vitest-browser-svelte by default runs all test files in the same browser session (unless isolate: true is set in the browser project config). Any <a href> element in a test-rendered component that triggers SvelteKit's link interceptor could reproduce this failure for other test files. After fixing this issue, it's worth checking the vitest config's browser.isolate setting.
Option A is a code-only fix — no CI pipeline, Compose, or runner changes needed. It's the cleanest resolution from an infrastructure perspective.

Recommendations

Option A resolves the CI failure cleanly. No pipeline changes required.
After merging, confirm in CI that the Unit & Component Tests job passes end-to-end and that coverage reports ≥ 80% for the browser project.
Check the vitest config for browser: { isolate: ... }. If not set or set to false, similar <a href onclick> patterns in other components under test could trigger the same failure. Setting isolate: true provides an extra safety net — each test file gets its own browser context — at the cost of slightly slower CI (each context has cold-start overhead). Worth evaluating after this fix lands.
No concerns about Docker Compose, Caddy, or production infrastructure for this issue.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - The primary CI impact is clear: one test file's iframe navigation contaminates the entire browser project's coverage run, dropping measured coverage from the expected >80% to ≈58%. This blocks the `Unit & Component Tests` job and therefore every PR's merge gate — regardless of what the PR touches. - The `birpc guard does not catch this` note in the issue is important from an observability standpoint. The error surfaces as `Cannot connect to the iframe` — a runtime error in the test runner, not a test assertion failure. This means the coverage failure is a secondary symptom of a primary connection error, which makes root-cause diagnosis harder from CI logs alone. - **Scope of the underlying risk:** `vitest-browser-svelte` by default runs all test files in the same browser session (unless `isolate: true` is set in the browser project config). Any `<a href>` element in a test-rendered component that triggers SvelteKit's link interceptor could reproduce this failure for other test files. After fixing this issue, it's worth checking the vitest config's `browser.isolate` setting. - Option A is a code-only fix — no CI pipeline, Compose, or runner changes needed. It's the cleanest resolution from an infrastructure perspective. ### Recommendations - **Option A resolves the CI failure cleanly.** No pipeline changes required. - After merging, confirm in CI that the `Unit & Component Tests` job passes end-to-end and that coverage reports ≥ 80% for the browser project. - Check the vitest config for `browser: { isolate: ... }`. If not set or set to `false`, similar `<a href onclick>` patterns in other components under test could trigger the same failure. Setting `isolate: true` provides an extra safety net — each test file gets its own browser context — at the cost of slightly slower CI (each context has cold-start overhead). Worth evaluating after this fix lands. - No concerns about Docker Compose, Caddy, or production infrastructure for this issue.

marcel commented

2026-05-12 16:22:19 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is exceptionally well-specified: problem statement → root cause analysis → three options with explicit trade-offs → a clear recommendation → measurable acceptance criteria. This is above average quality for a bug report — it's implementation-ready as written.
AC completeness check:
- AC1 ("NotificationDropdown.svelte.test.ts passes without iframe navigation error") — satisfied by all three options.
- AC2 ("npm run test (browser project) reports no Cannot connect to the iframe errors") — satisfied cleanly by Option A; partially by Option B (route abort prevents navigation but the interceptor still fires); uncertain for Option C (whether SvelteKit's capture-phase interceptor still fires for dispatchEvent events is not explicitly addressed).
- AC3 ("Client-coverage lines/functions/statements ≥ 80% in CI") — the coverage threshold is phrased as lines/functions/statements but the gate in the project config is likely branches. Verify that the coverage gate in vitest.config.ts matches the AC wording. If it's branch coverage (as referenced in the issue body: "Istanbul client-coverage"), then all three metrics should be explicitly stated in the AC.
One ambiguity: the test at line 171 currently asserts toHaveAttribute('href', '/aktivitaeten'). With Option A this assertion disappears (no href on a button). The navigation intent is then covered only by expect(goto).toHaveBeenCalledWith('/aktivitaeten'). This is functionally equivalent, but the AC doesn't mention that the test must be updated — it only says the test must pass. The implementation plan should include "update test assertions" as an explicit step.
Non-functional requirement gap: No performance or loading-time NFR applies here. No i18n change — m.chronik_view_all() is reused unchanged. No accessibility regression (confirmed by Leonie above).

Recommendations

No requirements changes needed. The issue can proceed as specified.
Suggest adding one line to the AC: "The view-all click test no longer asserts href attribute; the navigation target is verified via expect(goto).toHaveBeenCalledWith('/aktivitaeten')." This makes the test update part of the definition of done rather than an implementation detail.

## 📋 Elicit — Requirements Engineer ### Observations - The issue is exceptionally well-specified: problem statement → root cause analysis → three options with explicit trade-offs → a clear recommendation → measurable acceptance criteria. This is above average quality for a bug report — it's implementation-ready as written. - **AC completeness check:** - AC1 ("NotificationDropdown.svelte.test.ts passes without iframe navigation error") — satisfied by all three options. - AC2 ("npm run test (browser project) reports no `Cannot connect to the iframe` errors") — satisfied cleanly by Option A; partially by Option B (route abort prevents navigation but the interceptor still fires); uncertain for Option C (whether SvelteKit's capture-phase interceptor still fires for `dispatchEvent` events is not explicitly addressed). - AC3 ("Client-coverage lines/functions/statements ≥ 80% in CI") — the coverage threshold is phrased as lines/functions/statements but the gate in the project config is likely branches. Verify that the coverage gate in `vitest.config.ts` matches the AC wording. If it's branch coverage (as referenced in the issue body: "Istanbul client-coverage"), then all three metrics should be explicitly stated in the AC. - **One ambiguity:** the test at line 171 currently asserts `toHaveAttribute('href', '/aktivitaeten')`. With Option A this assertion disappears (no href on a button). The navigation intent is then covered only by `expect(goto).toHaveBeenCalledWith('/aktivitaeten')`. This is functionally equivalent, but the AC doesn't mention that the test must be updated — it only says the test must pass. The implementation plan should include "update test assertions" as an explicit step. - **Non-functional requirement gap:** No performance or loading-time NFR applies here. No i18n change — `m.chronik_view_all()` is reused unchanged. No accessibility regression (confirmed by Leonie above). ### Recommendations - No requirements changes needed. The issue can proceed as specified. - Suggest adding one line to the AC: "The view-all click test no longer asserts `href` attribute; the navigation target is verified via `expect(goto).toHaveBeenCalledWith('/aktivitaeten')`." This makes the test update part of the definition of done rather than an implementation detail.

marcel commented

2026-05-12 16:38:38 +02:00

✅ Implemented — Felix Brandt

Option A implemented on branch feat/issue-545-notification-dropdown-iframe-fix.

What changed

NotificationDropdown.svelte — commit f0971a23

Replaced <a href="/aktivitaeten" onclick={handleViewAll}> with <button type="button" onclick={handleViewAll}>
Simplified handleViewAll: dropped the e: MouseEvent parameter and e.preventDefault() — no longer needed since there is no href for the link interceptor to intercept

NotificationDropdown.svelte.test.ts — same commit

Added vi.clearAllMocks() to afterEach to prevent mock state leaking across tests
Split the single 'calls onClose when the view-all link is clicked' test into two focused it() blocks:
- 'calls onClose when the view-all button is clicked' — asserts onClose only
- 'navigates to /aktivitaeten when the view-all button is clicked' — asserts goto('/aktivitaeten') only
Changed selector from getByRole('link', ...) to getByRole('button', ...) — the element is now a button
Removed the stale toHaveAttribute('href', '/aktivitaeten') assertion (buttons have no href)

Verification

npm run test --project=client NotificationDropdown: 14/14 tests pass, no iframe navigation errors
npm run test: 3110/3110 tests pass across all 326 test files — no Cannot connect to the iframe contamination
npm run check: no new type errors in the changed files

## ✅ Implemented — Felix Brandt **Option A implemented** on branch `feat/issue-545-notification-dropdown-iframe-fix`. ### What changed **`NotificationDropdown.svelte`** — commit `f0971a23` - Replaced `<a href="/aktivitaeten" onclick={handleViewAll}>` with `<button type="button" onclick={handleViewAll}>` - Simplified `handleViewAll`: dropped the `e: MouseEvent` parameter and `e.preventDefault()` — no longer needed since there is no `href` for the link interceptor to intercept **`NotificationDropdown.svelte.test.ts`** — same commit - Added `vi.clearAllMocks()` to `afterEach` to prevent mock state leaking across tests - Split the single `'calls onClose when the view-all link is clicked'` test into two focused `it()` blocks: - `'calls onClose when the view-all button is clicked'` — asserts `onClose` only - `'navigates to /aktivitaeten when the view-all button is clicked'` — asserts `goto('/aktivitaeten')` only - Changed selector from `getByRole('link', ...)` to `getByRole('button', ...)` — the element is now a button - Removed the stale `toHaveAttribute('href', '/aktivitaeten')` assertion (buttons have no href) ### Verification - `npm run test --project=client NotificationDropdown`: **14/14 tests pass**, no iframe navigation errors - `npm run test`: **3110/3110 tests pass** across all 326 test files — no `Cannot connect to the iframe` contamination - `npm run check`: no new type errors in the changed files

marcel closed this issue

2026-05-12 16:38:41 +02:00

marcel referenced this issue from a commit

2026-05-12 16:40:18 +02:00

fix(notification): replace view-all anchor with button to prevent iframe navigation

marcel referenced a pull request that will close this issue

2026-05-12 16:40:27 +02:00

fix(notification): replace view-all anchor with button to prevent iframe navigation #552

marcel referenced this issue

2026-05-12 16:41:32 +02:00

fix(notification): replace view-all anchor with button to prevent iframe navigation #552

marcel referenced this issue