Consolidate shared vi.mock bodies + migrate confirm/notification specs (#560) #719
Reference in New Issue
Block a user
Delete Branch "feat/issue-560-shared-vimock-mocks"
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 #560.
What this PR delivers (CI-green path)
Groundwork
admin/tags/[id]/+page.svelte:$app/stores→$app/state; its 2$app/storesspec mocks replaced with sync-factory$app/statemocks (AC#3 ✅).@vitest/browser-playwrightexact-pinned to4.1.6(drop caret) sopatches/@vitest+browser-playwright+4.1.6.patchkeeps applying; TODO recorded via a top-level"//"key (AC#9 ✅).src/__meta__/no-factory-ban.test.ts— Node-mode scan banning no-factoryvi.mock(virtualModule)(AC#6 ✅, TDD red→green). Still valuable independent of Phase 1.Phase 2 — fixtures / context (the substantive work)
confirm.sveltenow inject a realcreateConfirmService()viarendercontext (mirroringadmin/tags/[id]/page.svelte.spec.ts). Zeroconfirm.sveltevi.mocks remain. The genericconfirm.test-fixture.sveltecan't wrap a page, and none of the 8 triggerconfirm(), so AC#4 is met as "real provided service, novi.mock".notifications.sveltesingleton →createNotificationStore()+provideNotificationStore()/getNotificationStore()/NOTIFICATION_KEY, provided once in root+layout.NotificationBell+ the Chronik page read it via context.notification.test-fixture.sveltewraps the bell and exposessetNotifications()viaonReady;NotificationBell.svelte.spec.tsasserts the announced unread count across empty / single / many / error states.notifications.svelte.spec.tsrewritten to drive a fresh factory per test. (Maintainer-approved scope: adds production code beyond the issue's "test-infra only" sign-off.)Reverted / not delivered
$app/forms/$app/navigationshared mocks) — infeasible, reverted to inline factories.$mocksalias) — removed (it only existed to serve the shared mocks).coverage-flake-probe.ymlare CI-only.Verification
npm run build✓ (production refactor compiles + SSR).npm run lint✓ (pre-commit, every commit).__meta__tests ✓ (no-factory-ban, no-duplicate-mock-ids, no-async-mock-factories).svelte-check: no new errors from these changes.Decisions taken during implementation (all maintainer-confirmed)
ConfirmService.Follow-up (not in this PR): delete the dead
feat/issue-560-vimock-migrationremote branch from closed PR #657.🤖 Generated with Claude Code
Declares $mocks -> src/__mocks__ in both vite.config.ts and vitest.client-coverage.config.ts so shared mock modules resolve in the client test run and the coverage job alike. Enables the sync-factory dedup pattern from ADR-012 (vi.mock('$app/forms', () => ({ ...formsMock }))). Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>A vi.mock('$app/navigation') with no factory does not auto-resolve to a __mocks__ file for SvelteKit virtual modules — it substitutes some exports and leaves others (replaceState) bound to the live router, which is exactly the PR #657 failure. This Node-mode source scan, mirroring no-async-mock-factories and no-duplicate-mock-ids, fails at every vitest invocation if any *.svelte.{spec,test}.ts reintroduces the pattern, and forecloses ADR-012's rejected Option C. Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Single home for the non-trivial form-interceptor enhance() shared by the four complex consumers: it intercepts submit, invokes the SubmitFunction, and fires the returned callback with a configurable result. setFormResult() drives the success/failure branch; an embedded beforeEach resets it before every test so isolation is structural. Consumed via vi.mock('$app/forms', () => ({ ...formsMock })) through the $mocks alias. Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Load-bearing first migration (ADR-012): this is the hardest case — its enhance submit callback actually fires and reads the form result. Replaces the duplicated 23-line interceptor factory with vi.mock('$app/forms', () => ({ ...formsMock })) via $mocks, and the per-test mockFormResult mutation with formsMock.setFormResult({ type: 'failure' }). The reset now comes from the shared module's embedded beforeEach. The existing optimisticMarkRead/optimisticMarkAllRead-on-submit assertions remain as the positive proof the callback fired. Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Replaces the vi.mock('$lib/shared/services/confirm.svelte') stub with a real createConfirmService() provided through render's context map, mirroring the existing admin/tags/[id]/page.svelte.spec.ts pattern. The generic confirm.test-fixture.svelte renders only ConfirmDialog and cannot wrap an arbitrary page; none of these specs trigger confirm(), so the children's getConfirmService() simply reads the provided context instead of a module mock. No vi.mock of confirm.svelte remains in these 5 specs. Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>🔒 Nora "NullX" Steiner — Security
✅ LGTM — no auth/session/redirect regression. This is a clean singleton→factory refactor; the security-relevant code paths are byte-for-byte identical.
What I checked
1. The 401 →
/loginredirect path is unchanged.The
eventSource.onerrorhandler increateNotificationStore()is identical to the old top-level body — both branches (CLOSED readyState, and theerrorCount >= 3reconnect-failure branch) still callfetch('/api/notifications/unread-count')andnavigate('/login')only onres.status === 401. No condition was loosened, no branch dropped. The store moving from module singleton to a per-tree context instance does not change when or whether the session-expiry redirect fires. The defaultnavigatestill hard-navigates viawindow.location.href, which correctly forces a full reload (clearing in-memory state) on session loss — good fail-closed behavior.2.
setNavigate()test seam is not a production risk.It replaces the old module-level
__setNavigateForTest. I grepped the wholefrontend/srctree: the only non-test caller ofsetNavigateisnotifications.svelte.spec.ts:55. No.sveltecomponent, no route, no layout invokes it. Production code never reaches the seam, so the redirect target cannot be hijacked at runtime. Minor nit (non-blocking): it now sits on the publicNotificationStoreinterface rather than being a clearly-quarantined__-prefixed export, so it's technically callable by any context consumer. The JSDoc/** Test-only */mitigates this, and there's no injection vector (the store is provided once in root+layout, all consumers are first-party). I'd accept it as-is; if you want belt-and-suspenders, a__-prefix or a dev-only guard would make the test-only contract enforced rather than documented.3. No data exposure / context-scoping concern.
provideNotificationStore()is called once in the root+layout.svelte; consumers (NotificationBell,aktivitaeten/+page.svelte) read viagetNotificationStore(), which fails closed with a thrown error if the context is missing — no silent null-store fallback. Per-tree instancing actually reduces cross-session bleed risk vs. a module singleton (relevant in SSR, though this store is client-driven). Notification payloads are unchanged and still gated by the same/api/notifications*endpoints server-side.4. Supply chain: the
@vitest/browser-playwrightexact-pin is benign and well-documented.^4.0.10→ exact4.1.6(no caret) sopatches/@vitest+browser-playwright+4.1.6.patchkeeps applying. The patch content is unchanged in this PR. Pinning a dev-only test dependency to an exact version is a positive supply-chain hygiene move — it removes the floating-range window where a malicious/broken minor could slip in, and the inlinepackage.jsoncomment + ADR-012 reference document the TODO to unpin once vitest PR #10267 ships. Dev-only (devDependencies), so zero production attack surface. The accompanyingvi.mocksynchronous-factory rule (ADR-012) is a test-correctness constraint, not a security control.Blockers
None.
Suggestions
__-prefixingsetNavigate/setNotificationsor gating them behind a dev-mode check so the test-only contract is enforced by the type surface rather than only by JSDoc convention.Verdict: ship it. No injection, no auth bypass, no IDOR, no session-handling regression, no risky dependency drift.
🏛️ Markus Keller (@mkeller) — Architect
⚠️ Approved with concerns — the context-provider design is sound and the SSE refcount semantics survive the refactor, but the production
NotificationStoreinterface carries two test-only methods, which the very same PR proves is avoidable.I reviewed the module/layer boundaries, the context-provider lifecycle, the
$mocksalias topology, and ADR-012 accuracy. The mock-dedup infra itself is clean and well-fenced; my comments are almost entirely about the notification refactor that landed alongside it.What I checked and like
provideNotificationStore()runs once in the root+layout.svelte;NotificationBelland the Chronik page (aktivitaeten/+page.svelte) both read viagetNotificationStore(). The singleton-in-module-scope was an implicit global; moving it onto Svelte context makes the dependency explicit and per-tree, which is the right boundary. This also mirrors the existingprovideConfirmService()pattern already in the same layout — consistency wins.refCount/errorCount/eventSourceare now per-store closure state instead of module globals, but both consumers still pairinit()on mount withdestroy()on destroy (NotificationBell.svelteonMount/onDestroy;aktivitaeten/+page.sveltelines 37–42). TherefCount > 1guard and therefCount === 0teardown are unchanged. With a single store instance provided at root and N consumers calling init/destroy, the refcount still collapses to one EventSource and tears it down only when the last consumer unmounts. No regression in the lifecycle — verified the balance on both call sites.$mocksalias declared in 3 places is the right number, not one too many. Each declaration serves a distinct resolver:svelte.config.jskit.alias(generates thepathsentry in.svelte-kit/tsconfig.jsonsotsc/svelte-check resolve it — confirmed it's emitted),vite.config.tsresolve.alias(main browser-test runtime), andvitest.client-coverage.config.tsresolve.alias(the coverage job, which is a standalone config that does not extend the base). Collapsing these would require the coverage config to import the base, which it deliberately doesn't. ADR-012 only documents "both vite configs" and omits the svelte.config.js one — see the ADR nit below.Blockers
None. The boundaries are intact and the docs largely track the code.
Suggestions (nice-to-have)
1.
setNotifications/setNavigateon the productionNotificationStoreinterface — the same PR shows the clean alternative.notifications.svelte.ts:17-20puts two/** Test-only */methods on the shipped interface. This is a leaked seam: production code can now mutate the store's internal state and reroute its 401 redirect, and nothing at the type level stops it. The counter-example is in this very diff — the confirm-mock migration (e.g.admin/tags/[id]/page.svelte.test.ts) injects the realcreateConfirmService()through context with zero test-only methods onConfirmService(verified: it exposes onlyconfirm()). The notification specs already addednotification.test-fixture.sveltecallingprovideNotificationStore()— that fixture is the proper seam. I'd push the two setters off the interface and have the fixture/specs reach the seed-and-navigate behaviour another way (a test-only subtype, or threading an injectablefetch/navigateas acreateNotificationStore(opts)param, which is also howADR-012's own libLoader pattern argues for prop injection over post-hoc setters). I'm not blocking because the SSE/EventSource surface genuinely is harder to fake than ConfirmService — but the asymmetry within one PR is exactly the kind of thing a future reader will cargo-cult the wrong direction.2.
getNotificationStore()try/catch aroundgetContextis dead defence.notifications.svelte.ts:148-160wrapsgetContextin try/catch and then also null-checks, throwing the identical message from both paths.getContextdoesn't throw when a key is absent — it returnsundefined. The catch arm is unreachable; collapse to the null-check alone. Minor, but duplicated error strings rot independently.3. ADR-012 §"Sanctioned dedup" undercounts the alias. Line 157 states the
$mocksalias "is declared in bothvite.config.tsandvitest.client-coverage.config.ts." It's actually three places —svelte.config.jskit.aliasis load-bearing for type resolution and is the one a contributor is most likely to forget when adding a new resolver. ADRs are the codebase's memory; this one should name all three and say why each exists (TS resolution / test runtime / coverage runtime). One-line fix, but it's the difference between the ADR preventing the next "why doesn't$mocksresolve in svelte-check" hour-long detour or not.4. ADR-012 scope note. The ADR is titled and framed as a test-mocking strategy, but this PR's largest blast radius is a production refactor (module singleton → context provider for notifications). That's a deliberate maintainer call and I support the direction — but the ADR doesn't mention it at all. Either add a short paragraph ("the notification store was migrated to the same context-provider shape as ConfirmService to make it injectable without test-only setters") or drop a one-liner in the PR description pointing at where that decision is recorded. Right now the architectural decision with the most lasting consequence in this PR has no ADR footprint.
None of these block merge. Fix #1 and #3 before the pattern propagates; #2 and #4 are housekeeping.
🎨 Leonie Voss — UX / Accessibility Review
⚠️ Solid a11y assertions, but the "single" case has a tautology gap and the error state deserves a second look.
This PR is overwhelmingly test-infra. No visual or layout changes reach production DOM, and the
__mocks__/fixture files never render, so they're a11y-neutral. I focused on the newNotificationBella11y assertions (AC#5) and the two production touches.What I checked
NotificationBell.svelteproduction change — purenotificationStore→getNotificationStore()import swap. The live region is unchanged and well-built: persistent<span aria-live="polite" aria-atomic="true">that stays mounted and just toggles.hidden, so SR live-region history is preserved across count changes.titleandaria-labelare both bound to the samebellLabelderived — good, consistent announcement. No regression here. ✅admin/tags/[id]$app/stores→$app/state—$page.url.pathname→page.url.pathnameonly, inside a non-renderingreplaceStateeffect. No a11y surface. ✅aktivitaeten/+page.svelte— import + comment churn only; thearia-live="polite"/aria-busyfeed region is untouched. ✅title === aria-labelin all four states and verifies the badge.hiddentoggle + exact count text. That's the right shape.Blockers
None. Nothing here blocks merge.
Suggestions
The "single" test is tautological and hides a real grammar bug (High). The spec asserts:
This compares the rendered title against the same message function with the same arg — it can never fail regardless of grammar, so it does not actually verify a correct singular announcement. And there is a defect to catch:
notification_bell_unread_labelis a flat interpolation with no Paraglide plural matcher, socount: 1renders "1 unread notifications" / "1 ungelesene Benachrichtigungen" / "1 notificaciones sin leer" — plural noun for a single item. Screen-reader users hear ungrammatical output, and it reads as broken to the senior audience.Fix (two parts): (a) give the message a plural form, e.g. Paraglide match:
{count, plural, one {1 unread notification} other {# unread notifications}}acrossde/en/es.json; (b) assert the literal expected string in the test (toBe('1 unread notification')) instead of round-tripping the message fn, so the singular form is genuinely pinned.The "error" state announces a silent recovery, not the failure — confirm that's intended (Medium). On a failed unread-count load the bell degrades to count 0 and hides the badge (
aria-liveannounces nothing). For this widget that's defensible: a notification count is non-critical, and shouting "couldn't load notifications" on every transient SSE/fetch blip via a live region would be noise —aria-live="polite"on a count badge is the wrong channel for an error anyway. So the behavior is fine, but the test comment ("announces a valid, non-urgent state") slightly oversells it — nothing is announced; the badge is simply absent. If product ever wants the failure surfaced, it belongs in the dropdown body (arole="status"empty/error line on open), not the bell badge. No change required now; just naming the boundary so it's a conscious choice.Unverified a11y gap:
aria-expanded/ focus-return on the bell (Low). AC#5 covers the announced count, but the bell's other a11y contract —aria-expandedtoggling withopen, focus returning to the bell button on Escape/close — isn't asserted in the new spec. Not in scope for this PR, but worth a follow-up test so the keyboard/SR open-close loop is regression-guarded alongside the count.Net: the assertions test the right surface, but suggestion 1 is worth folding in before merge — the singular case currently green-lights a real grammatical defect.
👨💻 Felix Brandt — Senior Fullstack Developer
⚠️ Approved with concerns — clean, well-factored dedup with real TDD evidence (the no-factory ban is genuine red→green). The shared-mock module design is exactly the right altitude. Two things I want addressed before merge, and a handful of suggestions.
Blockers
1. The
no-factory-banregex has a false-negative gap it advertises as a guarantee.frontend/src/__meta__/no-factory-ban.test.ts:35—NO_FACTORY_VI_MOCK = /vi\.mock\(\s*['"][^'"]+['"]\s*\)/. The whole point of this meta-test is to foreclose the PR #657 class. But it only matches when the closing paren immediately follows the string. A trailing-comma no-factory call —vi.mock('$app/navigation', )— slips straight through, and so doesvi.mock('$app/navigation' /* note */). Either is a one-keystroke regression that reintroduces the exact partial-mock crash the ADR documents. Add a red test for the trailing-comma form first (it should currently pass = prove the hole), then widen the pattern to treat "string arg followed by)with only whitespace/comma/comment between" as an offender. The persona rule here: a guard test that can be trivially bypassed gives false confidence, which is worse than no guard.Suggestions
2. The NotificationBell a11y "error" state test does not test what its name claims — and the green is partly accidental.
NotificationBell.svelte.spec.ts:123— "degrades to a zero announced count when the unread-count load fails". In the success tests,await tick()(asetTimeout(0)macrotask) flushesinit()'sfetchUnreadCountmicrotask chain (await fetch→await res.json()→unreadCount = 0) beforesetNotifications(...)runs, so the ordering is race-free for those — good. But the error test assertstitle === notification_bell_label()and the badge ishidden, which is the exact same assertion as the empty state (line 82). The count is 0 because the store initializes to 0, not because the rejected fetch was observed —fetchUnreadCount'scatchjust logs and leavesunreadCountuntouched. So this test would still pass if the error path were deleted entirely. It is not modelling "error" as a distinct a11y state; it is re-asserting the initial state with a thrown fetch that nothing waits on. Either (a) drive a real non-zero count first, then trigger an error and assert it degrades to a meaningful state, or (b) be honest and rename it — "error" as graceful-degradation-to-0 is indistinguishable from "empty" and the test name oversells it.3. Context-map boilerplate is now copy-pasted across ~17 spec files.
context: new Map([[CONFIRM_KEY, createConfirmService()]])repeats on nearly everyrender(...)call in the migrated confirm specs (e.g.admin/users/[id]/page.svelte.test.ts, 8+ times in one file). This PR's whole thesis is "extract the duplicated test setup into a shared helper" — yet the confirm migration introduces a new duplication of the same shape it's removing for$app/forms. A one-linerrenderWithConfirm(Component, props)(or a sharedconfirmContext()returning the Map) would apply the same DRY discipline you applied to the mock bodies. Not a blocker, but it's inconsistent to dedup one pattern while hand-rolling another in the same changeset.4.
getNotificationStore()swallows the real error in its catch.notifications.svelte.ts:147-160—getContextis wrapped intry/catchthat discards the caught value and throws a hardcoded message, and then there's a second identical throw for the!storecase. Two copies of the same error string.getContextdoesn't throw when a key is absent (it returnsundefined), so thetry/catchis dead defensive scaffolding — theif (!store)branch is the one that actually fires. Drop the try/catch, keep the singleif (!store) throw. Less code, one source of truth for the message.What I liked
createNotificationStore()+provide/getis the correct Svelte 5 context idiom; killing the module-level singleton removes cross-test state bleed at the root. ThesetNavigateseam for the 401 side-effect is a clean test-injection point rather than a global stub.simulateNavigatein the shared nav mock is genuinely consumed (useUnsavedWarning.svelte.test.ts) — no dead helper shipped.hasNoFactoryViMockscan has its own positive/negative unit cases before the file-glob assertion. That's the discipline I want to see.CI caught that vi.mock('$app/forms', () => ({ ...formsMock })) with a static `import * as formsMock` fails: vitest hoists vi.mock above the import, so the factory references an uninitialised binding ("no top level variables inside"). Load the shared mock module via `const formsMock = await vi.hoisted(() => import('$mocks/...'))` instead — the factory may reference a vi.hoisted binding, and the dynamic import runs at collection time (not in the lazily-invoked factory), so it stays clear of ADR-012's birpc race and the no-async-mock-factories guard. Applies to all 5 shared-mock consumers ($app/forms x4, $app/navigation x1). Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>CI proved cross-file sharing of a virtual-module mock body cannot work in @vitest/browser-playwright 4.1.6: the static-import spread fails the hoist ("no top level variables"), and the await-vi.hoisted-import form fails to parse ("Unexpected identifier 'vi'"). vi.hoisted has the same hoist constraint as vi.mock, so there is no way to thread an external module's body into the factory here. Reverts Phase 1: restores the 4 $app/forms/$app/navigation specs to their inline factories, inlines NotificationBell.spec's forms stub, deletes the src/__mocks__/$app/* modules and the $mocks alias (vite, vitest-coverage, kit). The no-factory-ban meta-test stays (no-factory vi.mock is still banned). ADR-012 amended to record the infeasibility. Everything else ($app/state migration, confirm context-inject, notification refactor, the pin, the meta-test) is unaffected. Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>⚙️ Reproducible, the pin is wired correctly — DevOps
Verdict: ✅ — The exact-pin is consistent end to end and CI will reliably re-apply the patch. The Phase-1 alias revert left the configs clean. Ship it.
I reviewed this purely from the "will builds be reproducible and will CI apply the patch" angle, since that's the only DevOps-relevant change that survived the revert.
What's correct (the load-bearing stuff)
Version triple is consistent — patch will always apply.
frontend/package.json:51→"@vitest/browser-playwright": "4.1.6"(caret dropped, exact pin)frontend/package-lock.json:4787→ resolved"version": "4.1.6"with a real integrity hashfrontend/patches/@vitest+browser-playwright+4.1.6.patch→ filename target matchespatch-package matches a patch file to the installed version by filename. Before this PR, a caret range meant a 4.1.7+ release would silently install, the
4.1.6.patchwould no longer match, patch-package would skip it (warn, not fail), and the duplicate-mock-id birpc race would resurface in CI with no hard signal. The exact pin closes that gap. This is the right fix.npm ciapplies the patch in CI.patch-packageruns via"postinstall": "patch-package"(package.json:13), and npm firespostinstallonnpm ci. Both consumers —.gitea/workflows/ci.yml:29and.gitea/workflows/coverage-flake-probe.yml:33— usenpm ci, andpatch-packageis a devDependency (package.json:61), so it's present in a fresh install. Belt-and-suspenders:ci.yml:148-154greps the coverage log for[birpc] rpc is closedand fails the job if the race reappears — i.e. CI actively proves the patch is effective, not just that it applied. Nice.Configs are clean after the alias revert. No
$mocksalias and no danglingfileURLToPathimport invite.config.ts,vitest.client-coverage.config.ts, orsvelte.config.js. The remaining**/__mocks__/**entries are just coverageexcludeglobs — a glob matching a now-nonexistent dir is a harmless no-op, not a leftover of the reverted alias.The
"//"comment key is safe. npm treats any key it doesn't recognize as inert, and"//"(-prefixed) keys are the documented convention for package.json comments."//@vitest/browser-playwright"is read by nobody and serializes fine. It correctly records the unpin TODO + the ADR pointer.Blockers
None.
Suggestions (non-blocking)
@vitest/browser-playwright. Consider apackageRulesentry pinning/ignoring this package until PR #10267 ships upstream, so the TODO doesn't get auto-bumped out from under the patch. The"//"note helps a human, but automation won't read it.--error-on-failto the postinstall ("postinstall": "patch-package --error-on-fail") turns a silently-unapplied security/correctness patch into a failednpm ciin CI — fail loud, fail early. Low effort, removes a whole class of silent drift.Neither blocks merge.
📋 PR #719 — Requirements Engineer
Verdict: ✅ Approve. The residual scope is coherent, the AC#1/#2/#8 reversal is CI-proven and properly recorded (ADR-012 revision + PR body), and the two substantive pieces (confirm injection, notification context refactor) are fully traceable to the issue's intent. Delivered ≈ promised once the four maintainer-confirmed decisions are accepted as the governing contract.
I assessed this as a brownfield requirements audit: did each acceptance criterion get Met / Reinterpreted / Reverted-infeasible / Deferred, is each deviation justified and recorded, and is what ships worth shipping.
AC-by-AC traceability
__mocks__/$app/forms.tsshared interceptor +setFormResult+$mocksimportvi.hoistedimport) fail in @vitest/browser-playwright 4.1.6. Recorded in ADR-012 rev 2026-06-02 ("third false premise") + PR body. Specs keep inline factories. Justified.__mocks__/$app/navigation.ts+simulateNavigatevia$mocks__mocks__/dir exists, no$mocksref remains (verified). Justified.admin/tags/[id]/+page.svelte→$app/state; 2$app/storesmocks gone+page.svelteswapped; zero$app/storesrefs underadmin/tags/(verified); the 2 spec mocks now sync-factory$app/state.confirm.test-fixture.svelteused by every ex-confirm.sveltemocker; novi.mockleftcreateConfirmService()via rendercontext: new Map([[CONFIRM_KEY, …]]). Zeroconfirm.sveltevi.mocks remain (verified). The fixture can't wrap a page and none of the 8 callconfirm(), so "real provided service, novi.mock" satisfies the AC's intent (kill the mock, use the real seam). Defensible and arguably cleaner than the fixture.notification.test-fixture.svelte(option-bsetNotifications); both/all consumers use it AND assert announced unread count across empty/single/many/errorNotificationBell, exposessetNotificationsviaonReady.NotificationBell.svelte.spec.tsasserts title/aria-label/aria-livebadge across all four named states. Both production consumers (NotificationBell,aktivitaeten/+page.svelte) migrated togetNotificationStore()context.vi.mock(virtualModule)src/__meta__/no-factory-ban.test.tswith the spec'd regex + unit cases + full-tree scan; TDD red→green. Tree scan returns zero offenders (verified).*.svelte.{spec,test}.ts= the 8 confirm specs + 2 notification specs + aktivitaeten + the admin/tags prerequisite. No stray factory bodies.$mocksalias invite.config.ts+vitest.client-coverage.config.ts@vitest/browser-playwrightpinned 4.1.6 + TODOpackage.jsonexact-pinned4.1.6(caret dropped) with a"//"-key TODO; patch renamed…+4.1.6.patch(file present).coverage-flake-probe.yml20× cleanworkflow_dispatchjob; cannot run locally. Appropriate.Net assessment: delivered vs promised
vi.mockof the confirm service — use the real injectable seam." That requirement is fully met. This is solution-language in the AC giving way to the actual need — exactly the right call.aria-livecount-change path that the issue specifically flagged as otherwise-unverified.Blockers
None.
Suggestions (non-blocking)
__mocks__/" — the headline names the reverted mechanism. On close, retitle to reflect what shipped (confirm/notification fixture+context migration + no-factory guard) so the closed issue isn't misleading in search.feat/issue-560-vimock-migrationremote branch (PR #657) is noted but out-of-PR — make sure it doesn't get lost when this merges.Requirements Engineer review — traceability + scope-coherence focus. No code-quality or security judgement implied.
🧪 Sara Holt — QA
Verdict: ⚠️ Approve with one substantive test-quality fix. The context refactor genuinely improves isolation (fresh store per test beats the old singleton
__resetForTest()), the confirm migration preserves coverage, and the meta-test is a sound permanent guard. But the new "error" a11y test does not test an error — it's a duplicate of the empty-state assertion. Fix that and this is a clean ✅.Blockers
None. Nothing here breaks the suite or regresses coverage. The item below is a quality defect in a new test, not a merge-stopper, but I'd want it addressed before this lands as the canonical pattern others will copy.
Concerns
1.
NotificationBell.svelte.spec.ts:131— the "error" test is vacuous; it asserts the empty state, not an error path.The three assertions are byte-for-byte identical to the
empty:test (title === bell_label,aria-label === title, badge hidden). The count is 0 here because the store initialisesunreadCount = $state(0)(notifications.svelte.ts), not because the rejected fetch was observed.fetchUnreadCount()'scatchblock logs and returns, leaving the already-zero count untouched. Swap the rejecting fetch for a resolving{count: 0}and every assertion still passes — so the test cannot distinguish "error path exercised" from "store never loaded." The comment claims the bell "degrades... from a broken count", but no non-zero starting state is ever established, so there is nothing to degrade from.What it actually proves: the bell doesn't crash on a rejected fetch. That's a real (if thin) guarantee, but the name oversells it. To make it test the error path meaningfully, the store must start non-zero and then fail a refetch — e.g. seed a successful
{count: 3}oninit(), settle, assert badge shows 3, then trigger an onerror/refetch that rejects and assert it holds (or degrades) deterministically. As written, rename it todoes not crash when the unread-count load failsso the name matches the assertion, or beef it up. Right now it's coverage padding dressed as an error test.2.
NotificationBell.svelte.spec.tstick()race — works today, but on a hidden coupling.The ordering invariant ("init()'s async
fetchUnreadCountsettles to 0 beforesetNotifications()runs") holds only because (a) the stubbed fetch resolves purely on the microtask queue (mockResolvedValue+Response.json()— no macrotask), so a singlesetTimeout(0)drains it, and (b)NoopEventSourcenever firesonopen, which would queue a secondfetchUnreadCount()that could land aftersetNotificationsand clobber the count back to 0. That second condition is an undocumented dependency between the stub and the timing. If anyone givesNoopEventSource.onopena call, orfetchUnreadCountgains a real timer/retry,single/manyflake intermittently. Not flaky now — but it's exactly the "passes 90% of the time" trap. Cheap hardening: assert the post-init baseline (expect(unreadBadge()).toHaveClass('hidden')right after the firsttick()) so a regression surfaces as a clear failure rather than a race, and/or usevi.waitForon the final count instead of a baretick().What's good (and I checked)
notifications.svelte.spec.tsrewrite is sound. FreshcreateNotificationStore()inbeforeEach+setNavigate(spy)injection is strictly better isolation than the old singleton__resetForTest()— no cross-test leakage ofrefCount/errorCount/eventSource. All 15 behaviours (ref-counting, SSE prepend, optimistic marks, the full onerror/401 matrix) are preserved verbatim, just retargeted to the instance. No coverage lost.getConfirmService: () => ({ confirm: async () => false })mock is replaced by a real context-injectedcreateConfirmService(). I verified none of the 8 migrated specs exercise aconfirm()-gated branch: they assert rendering (headings/banners/buttons/links). The two clicking tests inTranscriptionEditView(toggles training chip,handleMarkAllReviewed) don't touchconfirm()—handleMarkAllReviewedcallsonMarkAllReviewed()directly with no confirm gate. The injection exists solely to satisfy the init-timegetConfirmService()context lookup. Correct and minimal. (Note: the realconfirm()now returns a pending promise in-browser rather thanfalse; harmless here only because no test awaits a confirm outcome — worth a one-line comment so a future author doesn't add a confirm-gated assertion expecting auto-decline.)$app/forms/$app/navigationfactories vs. sharing viasrc/__mocks__is a structural/DRY change only; the mock bodies and every assertion are unchanged. Theno-factory-ban.test.tsmeta-test correctly forecloses the partial-auto-mock failure class (PR #657 /replaceStatebefore router init) with good self-tests for the scanner. Regex is fine for the named anti-pattern (won't catch backtick-quoted ids, acceptable for a belt-and-braces guard layered behind ESLint + CI grep).admin/tags/[id]$app/stores→$app/statemigration is correct; the getter-basedvi.mock('$app/state')matches the ADR-012 sync-factory rule.Bottom line: ship-ready once the
error:test is renamed or made to actually observe a failed refetch from a non-zero baseline. The isolation story is a real improvement — this is the kind of refactor that pays for itself.CI showed the single/many a11y tests failing with count 0: init()'s async fetchUnreadCount resolved to {count:0} AFTER setNotifications() ran, clobbering the seeded count (the flake Sara predicted in review). Stub fetch to never settle so the announced count is driven solely by setNotifications — deterministic, no race. Also rewrites the 'error' test to seed a count then fail the load and assert the count SURVIVES, so it is a meaningful state distinct from 'empty' (was byte-identical, flagged by Felix/Sara/Leonie). Part of #560. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>