fix(test): NotificationDropdown iframe navigation crash + Tailwind CI noise #548
Reference in New Issue
Block a user
Delete Branch "feat/issue-545-notification-dropdown-iframe-fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #545
Summary
NotificationDropdown.svelte: the "view all" footer link usedonclick={onClose}, which let the browser follow thehreffor real. Inside the vitest-browser Playwright iframe there is no SvelteKit router, so the navigation disconnected the orchestrator iframe and crashed all 13 tests in the file. Fix:e.preventDefault(); onClose(); goto('/aktivitaeten')— dropdown closes first (UX), then client-side navigation.hrefis preserved for right-click / open-in-new-tab.$app/navigationwith{ goto: vi.fn() }, tighten thegetByRole('link')selector to{ name: /alle aktivitäten|view all/i }, and addexpect(goto).toHaveBeenCalledWith('/aktivitaeten')to cover the new behavioural guarantee.ChronikFuerDichBox.svelteandhilfe/transkription/+page.sveltewere the only two Svelte components with non-empty<style>blocks. The Svelte vite plugin creates virtual CSS modules for these, and@tailwindcss/vite:generate:servetried to process them, logging pre-transform errors on every CI run. Fix: extract both<style>blocks intolayout.css(animation renamedfade-in→chronik-fade-infor global scope safety), removing the virtual CSS modules.Commits
f6a0d7aatest(notification): add goto mock and tighten selector in NotificationDropdown spec0387d51efix(notification): prevent iframe navigation — use goto instead of href follow97bb1ee6fix(style): move ChronikFuerDichBox animation to global CSS to suppress Tailwind noise0a6a3fd0fix(style): move transkription print styles to global CSS to suppress Tailwind noiseTest plan
npm run test— all 13 NotificationDropdown tests pass, no iframe crash,gotospy asserted@tailwindcss/vite:generate:servepre-transform errors in CI output🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I checked
TDD evidence, naming, function size/responsibility, Svelte 5 rules, dead code.
Suggestions (no blockers)
NotificationDropdown.svelte— inline handler doing three thingsThis is borderline for template markup. Three sequential side effects in an anonymous arrow function. A named handler would make the intent auditable at a glance:
Not a blocker — the current form is readable enough at this scale — but worth noting if the component grows.
Test:
hrefattribute preservation is untestedThe PR description correctly calls out that
hrefis preserved for right-click / open-in-new-tab. That's a deliberate UX behaviour. There is no assertion verifying it. Consider adding:This costs one line and codifies the guarantee.
Test:
onClosecalled-once assertionexpect(onClose).toHaveBeenCalledOnce()is already present — good. The newexpect(goto).toHaveBeenCalledWith('/aktivitaeten')assertion correctly covers the new behaviour.CSS migration
Animation rename
fade-in→chronik-fade-inis correct and necessary for global scope. The@media (prefers-reduced-motion: reduce)block is preserved — well done.Overall clean work. The TDD order (test commit first, then fix commit) is clear from the commit log. Red/green discipline visible in the history.
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
What I checked
Module boundary compliance, doc update requirements (per the matrix in my persona), transport protocol choices, abstraction fitness.
Doc update matrix — nothing triggered
CLAUDE.mdroute table + C4 frontend diagramCLAUDE.mdpackage tabledocs/DEPLOYMENT.md+ C4 L2One question worth documenting
The decision to use
goto()instead ofwindow.location.hreffollow for dropdown links is a repeatable pattern — any dropdown or overlay that contains navigation links will face the same problem in a vitest-browser iframe context. This is small enough that an ADR is overkill, but a one-liner inCONTRIBUTING.mdunder "Component Testing Patterns" or in the existing ADR-012 would save the next person from having to rediscover it.Not a blocker — the commit message body already documents the reasoning well — but worth considering.
Architecture observations
goto('/aktivitaeten')is the correct SvelteKit navigation primitive for client-side navigation inside an overlay. Usinghreffollow would bypass the router. The fix is architecturally sound.<style>blocks tolayout.csswhen they either (a) target global selectors like.app-navor (b) define keyframe animations that need global scope is the correct call. Svelte's style scoping does not extend to@keyframes, so the original component-scoped animation was already technically global in effect — making it explicitly global inlayout.cssis more honest.🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
CI configuration, Docker Compose, image tags, secrets, deprecated actions, infrastructure side-effects of the change.
LGTM — nothing to flag
This is a pure frontend fix with no infrastructure footprint. Specific checks:
Positive note
The primary motivation for the
<style>block extraction is suppressing@tailwindcss/vite:generate:servepre-transform errors on every CI run. Removing those spurious log lines is a real improvement — noisy CI logs cause engineers to start ignoring output, which is how real errors get missed. Good housekeeping.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I checked
Requirement coverage for issue #545, acceptance criteria completeness, edge cases, NFR gaps.
Issue #545 coverage
The PR description lists three explicit test-plan items:
npm run test— 13 NotificationDropdown tests pass, no iframe crash@tailwindcss/vite:generate:servepre-transform errors in CI output (verified by observation, not assertion)All three are verifiable. Items 2 and 3 are process/CI observations rather than automated assertions, which is fine for this class of fix.
One undocumented behaviour present in the code but absent from the test suite
The PR description states:
This is a stated requirement of the fix. It is not tested. I'd frame it as:
This is testable with a single attribute assertion. Not a blocker, but a gap between stated intent and codified guarantee.
NFR check
gotomock prevents iframe navigationonClose()called beforegoto()hrefpreserved@media (prefers-reduced-motion)retained in layout.cssOverall the requirements from issue #545 are met.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I checked
Open redirect risk, XSS surface, CSRF implications, input handling in the new handler, global CSS scope leakage.
No security concerns found
goto('/aktivitaeten')— hardcoded path, zero open-redirect riskThe navigation target is a compile-time string literal. There is no user-controlled input in the path. An open redirect would require something like
goto(userInput)orgoto(new URL(location.href).searchParams.get('next')). None of that is present here.e.preventDefault()— correct patternCalling
preventDefault()before the SvelteKit router takes over is the right approach. It prevents the browser from issuing a full-page GET request, which could expose the session cookie unnecessarily in a cross-origin context (though given the same-origin anchor, risk is negligible here). Still, correct by construction.Global CSS — no security implication
Moving
@keyframes chronik-fade-inand print styles tolayout.csshas no security surface. The animation name is scoped by its unique prefix. The print styles target.app-navwhich is a layout-level class with no auth or data boundary significance.Test mock —
vi.mock('$app/navigation', () => ({ goto: vi.fn() }))— correct isolationMocking at the module boundary (not mocking the Svelte component internals) is the correct pattern. The mock does not introduce any test-time security bypass.
One observation (smell, not vulnerability)
The commit log shows
goto('/aktivitaeten')as a hardcoded path repeated in both the component and the test. If this path were ever renamed and only one side was updated, the test would catch it via thetoHaveBeenCalledWithassertion — good. No action needed.Clean change.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
What I checked
Test naming, factory function usage, selector quality, coverage gaps, assertion completeness, correct layer placement, mock boundary.
What's done well
vi.mock('$app/navigation', () => ({ goto: vi.fn() }))— mocked at module boundary, not inside test body. Correct.getByRole('link', { name: /alle aktivitäten|view all/i })— role + accessible name selector. Robust against DOM restructuring.expect(goto).toHaveBeenCalledWith('/aktivitaeten')— behavioural assertion covering the new guarantee.afterEach(cleanup)— present and correct.makeNotification()factory function — used throughout, keeps tests readable.Concerns
1.
hrefattribute not asserted — gap between stated behaviour and test coverageThe PR description explicitly states that
hrefis preserved for right-click / open-in-new-tab. This is a user-visible behaviour. It is not tested. If someone later changes the anchor to a<button>or removeshref, no test fails.This is a suggestion — the PR is shippable without it, but the stated behaviour should be covered.
2.
e.preventDefault()is not directly assertedThe test verifies
gotowas called, but does not verify that the default browser navigation was suppressed. In a real browser (not the vitest-browser iframe), the absence ofpreventDefault()would cause a full-page navigation and agotocall — the test would still pass. The only thing provingpreventDefault()works is the absence of an iframe crash, which is an environmental side-effect rather than an assertion.This is a minor gap — the vitest-browser iframe context partially covers it by definition, but it's worth noting.
3. Test for the original bug (regression guard)
There is no test named like
it('does not trigger full-page navigation')that explicitly guards against the iframe crash regression. The current tests would catch it only because they'd crash — which is the original symptom, not an assertion.Summary
The test improvements are real and meaningful. The
gotospy assertion is the right call. The gaps above are suggestions, not blockers. Ship it.🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist
Verdict: ✅ Approved
What I checked
Semantic HTML integrity, accessibility of the changed link, animation and reduced-motion compliance, print style preservation, brand token usage.
Positive findings
Anchor semantics preserved — critical for accessibility
The
<a href="/aktivitaeten">remains an anchor element with a realhref. This is important for:Using
e.preventDefault()+goto()instead of converting this to a<button>is the correct accessibility-preserving choice. Well done.@media (prefers-reduced-motion: reduce)preserved in layout.cssThis is retained correctly in the extraction. Users with vestibular disorders or motion sensitivity are not affected. ✅
Print styles preserved functionally
The
.app-nav { display: none }print rule is correctly moved tolayout.cssand will continue to suppress navigation on printed pages. ✅One observation
The animation rename from
.fade-into.chronik-fade-inis correct for global scope safety. However,.fade-inis an extremely generic class name — if it was ever used by accident on other elements (not in this PR), those would silently stop animating. Worth checking with a quick grep, though this is outside the scope of this PR.No accessibility regressions found
The change is net positive for UX: the dropdown closes first (expected behaviour for an overlay), then navigation happens. No visual jank, no focus trap issues introduced. The animation extraction is transparent to the user.
Review concerns addressed
Three suggestions from the team review have been resolved:
22704a14— test(notification): assert href is preserved on view-all linkAddresses @Felix, @Elicit, @Sara: the PR description states that
href="/aktivitaeten"is preserved for right-click / open-in-new-tab, but that behaviour had no test assertion. The existing "closes and navigates" test now stores the link in a variable and assertstoHaveAttribute('href', '/aktivitaeten')before clicking, codifying the guarantee.7aa12997— refactor(notification): extract handleViewAll named functionAddresses @Felix: the three-side-effect inline
onclickarrow function is extracted into a namedhandleViewAll(e: MouseEvent)function in the script block, making the intent auditable at a glance without changing any behaviour.c8d052d3— docs(adr-012): add overlay navigation pattern noteAddresses @Markus: ADR-012 now carries a "Pattern note" under the Residual Exceptions section documenting that navigation links inside overlays/dropdowns must use
e.preventDefault()+goto(path)rather than letting the browser follow the href — the reason (vitest-browser Playwright iframe has no SvelteKit router) and thehref-preservation requirement are both noted.All 3109 tests still pass. No new type errors introduced (pre-existing errors in
users/[id]/page.svelte.test.tsandadmin/users/+layout.svelteare unrelated to this PR).👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
TDD Evidence
The commit order in the PR body (
f6a0d7aa test(notification): add goto mock...→0387d51e fix(notification): prevent iframe navigation) confirms test-first discipline. Red before green — exactly right.What's Clean
handleViewAllis well-named, single-responsibility, and short (3 lines). No issues there.hrefkept on the<a>tag while navigation is handled client-side — correct progressive-enhancement thinking.fade-in→chronik-fade-inis the right call when moving to global scope; prevents future silent class name collision.@media (prefers-reduced-motion)correctly transferred tolayout.csswith the animation.Minor Suggestion
The test verifies that
onCloseandgotoare each called, but doesn't assert their call order. Since the intended UX is "close first, then navigate," a future refactor could swap them and no test would catch it:This is a
Couldnot a blocker — the functional behavior is already covered.Summary
Focused change, no dead code, test-first,
hrefsemantics preserved. Ship it.🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Doc Audit
Per the documentation update table, I check what this PR touches:
docs/adr/012-browser-test-mocking-strategy.mdupdatedThe ADR addition is exactly right. The pattern note added to ADR-012 captures why
e.preventDefault() + goto()is needed in a vitest-browser iframe context — this is precisely the kind of "non-obvious constraint" that belongs in an ADR. Future developers will encounter this same iframe-crash problem and the ADR now documents the solution before they have to rediscover it.CSS Architecture
Moving component-scoped
<style>blocks tolayout.cssis a reasonable pragmatic call when the Tailwind vite plugin struggles with virtual CSS modules. Thechronik-fade-inrename is correct namespace discipline for global scope — a component-scoped.fade-inliving in global CSS would be an accident waiting to happen.One observation (not a blocker): if this pattern grows — more components needing animations extracted to avoid Tailwind noise — it's worth noting in CLAUDE.md that component
<style>blocks should be avoided when@tailwindcss/viteis in use. For now, two instances is too few to generalize.Module Boundaries
Frontend-only change. No layering concerns, no cross-domain access patterns to flag.
goto('/aktivitaeten')hardcoded in the component is acceptable here — this is a navigation shortcut, not business logic.LGTM
Clean fix, ADR updated, no structural regressions.
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
CI Impact
This PR is the fix for a CI failure, not a source of one. The described symptoms match what you'd expect:
e.preventDefault() + goto()@tailwindcss/vite:generate:servepre-transform error noise on every CI run → fixed by removing the two<style>blocksNo pipeline changes in this PR. No new services, no Docker changes, no workflow files touched. My checklist comes back clean:
One Thing Done Well
Removing the Tailwind noise logs improves CI signal quality — less noise in the output means real errors stand out. That's a small but real operational win.
Summary
No infrastructure concerns. This is a frontend fix with clean CI implications.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Attack Surface Review
This PR touches client-side navigation behavior. My checklist:
handleViewAllfunction —goto('/aktivitaeten')The path
'/aktivitaeten'is a hardcoded string literal, not user-controlled input. No injection risk. No open redirect vector — SvelteKit'sgoto()does client-side navigation within the same origin; it does not accept arbitrary external URLs here.e.preventDefault()Standard event handling. No security implications.
CSS changes
Animation keyframe + print style moved to global CSS. Zero security surface.
$app/navigationmock in testsvi.mock('$app/navigation', () => ({ goto: vi.fn() }))is test-only code, correctly isolated. No production impact.What I Checked and Found Clean
innerHTML,eval, or unsanitized user content introducedfetch()calls or API endpointsNo Findings
Clean PR from a security perspective. Nothing to flag.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Test Quality Assessment
What improved:
Selector specificity —
page.getByRole('link')was grabbing the first link in the component, which is fragile if the markup changes.page.getByRole('link', { name: /alle aktivitäten|view all/i })is role + accessible name — the gold standard for resilient selectors. ✅$app/navigationmock at module boundary —vi.mock('$app/navigation', () => ({ goto: vi.fn() }))is exactly the right pattern for SvelteKit navigation mocking in vitest-browser context. The mock is at the module import level, not stubbing internal component state. ✅Behavioral assertion added —
expect(goto).toHaveBeenCalledWith('/aktivitaeten')pins the navigation target. If someone refactorshandleViewAlland changes the path, this test catches it. ✅hrefpreservation asserted —toHaveAttribute('href', '/aktivitaeten')confirms the anchor still has itshreffor right-click / open-in-new-tab semantics — a behavioral guarantee that was implicit before. ✅One suggestion (non-blocker):
The test doesn't cover the case where
onClosethrows — ifonClose()throws an exception,goto()is never called and the dropdown never navigates. Given these are mocks, the failure mode is unlikely in practice, but if you ever want to be thorough:This is a
Could— the core scenarios are well covered.Coverage Assessment
The 13 previously-crashing tests are now passing. The new assertion covers the behavioral guarantee introduced by this fix. No gaps in the happy path,
hrefattribute,onClosecall, orgotocall.LGTM
Tight test changes, focused assertions, correct mock pattern.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved
Requirement Traceability
e.preventDefault() + goto()). The problem statement is precise and testable. ✅Acceptance Criteria Coverage
The PR test plan reads:
npm run test— all 13 NotificationDropdown tests pass@tailwindcss/vite:generate:servepre-transform errorsAll three are observable, binary conditions. No ambiguous "should feel better" language.
Scope Discipline
This PR fixes exactly two things: the iframe navigation crash and the Tailwind noise. Both are tightly scoped to the described issue. No scope creep, no added features, no refactoring beyond what the fix requires (
handleViewAllextraction is directly required by the fix).No Requirements Gaps Found
The fix is complete with respect to the stated issue. No missing edge cases or deferred unhappy paths that would need a follow-up issue.
🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist
Verdict: ✅ Approved
UX Impact of the Navigation Change
Before: clicking "Alle Aktivitäten" / "View all" called
onCloseand then let the browser follow thehref. The dropdown closed, the page navigated. Visually this worked — in a real browser.After:
e.preventDefault()+onClose()+goto('/aktivitaeten'). The dropdown closes first (good UX — no lingering overlay during navigation), then SvelteKit handles the transition client-side (faster, no full-page reload).The
href="/aktivitaeten"is preserved on the<a>element. This means:/aktivitaetenAccessibility: no regressions. The element remains a semantic
<a>with anhref. Nothing in this change affects focus management, ARIA roles, or visible labels.Animation Preservation
The
chronik-fade-inanimation moved from scoped<style>tolayout.css. What matters to me:@media (prefers-reduced-motion: reduce)block is present inlayout.css— the senior audience that experiences vestibular discomfort from animations is protected160ms ease-out,translateY(-4px) → 0) are preserved identically — no visual regressionfade-in → chronik-fade-inin the markup correctly matches the new global class name — nothing breaks visuallyPrint Styles
Print styles for the transcription help page moved to
layout.css. The.app-nav { display: none }and@page { margin: 1.5cm }rules are unchanged in behavior. The transcription help page is the primary print target in this app (older users printing the Kurrent/Sütterlin guide), so preserving these is important. ✅No UX or Accessibility Concerns
Clean changes from my perspective.