.gitea/workflows/ci.yml

@@ -37,17 +37,34 @@ jobs:
         working-directory: frontend
 
       - name: Run unit and component tests with coverage
-        run: npm run test:coverage
+        shell: bash
+        run: |
+          set -eo pipefail
+          npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}.log
         working-directory: frontend
         env:
           TZ: Europe/Berlin
 
+      # Diagnostic guard: covers the coverage run only. If `npm test` (above)
+      # exits 1 with a birpc error, the named pattern appears here — not there.
+      - name: Assert no birpc teardown race in coverage run
+        shell: bash
+        if: always()
+        run: |
+          if grep -qF "[birpc] rpc is closed" /tmp/coverage-test-${{ github.run_id }}.log 2>/dev/null; then
+            echo "FAIL: [birpc] rpc is closed teardown race detected in coverage run"
+            grep -F "[birpc] rpc is closed" /tmp/coverage-test-${{ github.run_id }}.log
+            exit 1
+          fi
+
       - name: Upload coverage reports
         if: always()
         uses: actions/upload-artifact@v4
         with:
           name: coverage-reports
-          path: frontend/coverage/
+          path: |
+            frontend/coverage/
+            /tmp/coverage-test-${{ github.run_id }}.log
 
       - name: Build frontend
         run: npm run build

docs/adr/012-browser-test-mocking-strategy.md

@@ -0,0 +1,90 @@
+# ADR 012 — Browser-Mode Test Mocking Strategy
+
+**Status:** Accepted  
+**Date:** 2026-05-11  
+**Issue:** [#535 — Unit & Component Tests job exits 1 from vitest-browser teardown race](https://git.raddatz.cloud/marcel/familienarchiv/issues/535)
+
+---
+
+## Context
+
+Vitest browser-mode tests (the `client` project, run with `@vitest/browser-playwright` / Chromium) use a different module resolution path than Node-environment tests. When a spec calls `vi.mock('some-module', factory)`, vitest registers a `ManualMockedModule`. At runtime, every time Chromium requests that module, a playwright route handler intercepts the request and calls the Node worker over **birpc** (`resolveManualMock`) to evaluate the factory and return the module body.
+
+This is safe for modules that are imported **statically** at spec module-eval time (e.g. `$app/navigation`, `$env/static/public`): those requests resolve before the first test runs and well before any teardown occurs.
+
+It is **unsafe** for modules that are imported **dynamically** (e.g. inside an `async onMount`, inside a lazy-loaded chunk): Chromium may fetch the module after the worker's birpc channel has already closed, producing:
+
+```
+Error: [birpc] rpc is closed, cannot call "resolveManualMock"
+  ❯ ManualMockedModule.factory   node_modules/@vitest/browser/dist/index.js:3221:34
+```
+
+This raises an unhandled rejection that exits the vitest process with code 1, even though every test in the run reported green.
+
+`pdfjs-dist` and `pdfjs-dist/build/pdf.worker.min.mjs?url` are loaded via `await Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')])` inside `usePdfRenderer.svelte.ts::init()`, which is called from `onMount`. These dynamic imports triggered the race.
+
+---
+
+## Decision
+
+**Prefer prop injection over `vi.mock(module, factory)` for any module that is loaded dynamically in browser-mode specs.**
+
+### The libLoader pattern (for external rendering libraries)
+
+When a component depends on a large external library loaded via dynamic import, extract the import into an injectable loader function with a production default:
+
+```typescript
+// usePdfRenderer.svelte.ts
+type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;
+
+const defaultLibLoader: LibLoader = () =>
+  Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]);
+
+export function createPdfRenderer(libLoader: LibLoader = defaultLibLoader) { ... }
+```
+
+The component threads the loader as an optional prop:
+
+```svelte
+<!-- PdfViewer.svelte -->
+let { url, ..., libLoader = undefined } = $props();
+const renderer = untrack(() => createPdfRenderer(libLoader));
+```
+
+Tests supply a synchronous fake — no `vi.mock` needed:
+
+```typescript
+const fakePdfjs = { GlobalWorkerOptions: ..., getDocument: vi.fn(), TextLayer: class {} };
+const fakeLoader = vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
+render(PdfViewer, { url: '...', libLoader: fakeLoader });
+```
+
+### The test-host pattern (for component behaviour)
+
+For components that fetch data or call services, the `*.test-host.svelte` pattern threads the dependency as a prop rather than mocking the module. See `PersonMentionEditor.test-host.svelte` for the canonical example.
+
+---
+
+## Residual exceptions
+
+The following `vi.mock(module, factory)` calls in browser specs are **acceptable** because the mocked modules are loaded statically at spec module-eval time and cannot produce a teardown race:
+
+| Module | Why it stays as vi.mock |
+|--------|------------------------|
+| `$app/navigation` | SvelteKit virtual module — no DI seam |
+| `$app/forms` | SvelteKit virtual module — no DI seam |
+| `$app/state` | SvelteKit virtual module — no DI seam |
+| `$app/stores` | SvelteKit virtual module — no DI seam |
+| `$env/static/public` | Vite env virtual module — no DI seam |
+
+These modules are resolved at static import time (before any test runs). Their `vi.mock` factories are served by birpc synchronously during module graph resolution, not after worker teardown.
+
+---
+
+## Consequences
+
+- New browser-mode specs that need to stub an external library **must not** use `vi.mock(externalLib, factory)`. Add a loader/factory parameter to the underlying hook or service instead.
+- The CI `unit-tests` job includes a permanent grep guard that fails the build if `rpc is closed` appears in any coverage run log. This catches regressions before they reach the acceptance criterion.
+- Acceptance criterion for #535: 60 consecutive green `workflow_dispatch` CI runs against `main` after the fix is merged, with zero `rpc is closed` lines in any log.
+- **Enforcement:** No automated lint rule is planned; the CI coverage guard is the regression backstop. If a lint rule is added later (e.g. an ESLint rule flagging `vi.mock` of non-virtual modules in browser-mode spec files), update this ADR.
+- **When to revisit the LibLoader home:** If three or more components adopt this pattern, consider extracting a shared `$lib/types/lib-loader.ts` or a generic `DynamicImportLoader<T>` type to avoid parallel type definitions across modules.

frontend/src/lib/document/viewer/PdfViewer.svelte

@@ -1,6 +1,6 @@
 <script lang="ts">
-import { onMount, setContext } from 'svelte';
-import { createPdfRenderer } from '$lib/document/viewer/usePdfRenderer.svelte';
+import { onMount, setContext, untrack } from 'svelte';
+import { createPdfRenderer, type LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte';
 import PdfControls from './PdfControls.svelte';
 import AnnotationLayer from '$lib/document/annotation/AnnotationLayer.svelte';
 import type { Annotation } from '$lib/shared/types';
@@ -21,7 +21,8 @@ let {
 	onDeleteAnnotationRequest,
 	documentFileHash,
 	annotationsDimmed = false,
-	flashAnnotationId = null
+	flashAnnotationId = null,
+	libLoader = undefined
 }: {
 	url: string;
 	documentId?: string;
@@ -35,9 +36,11 @@ let {
 	documentFileHash?: string | null;
 	annotationsDimmed?: boolean;
 	flashAnnotationId?: string | null;
+	libLoader?: LibLoader;
 } = $props();
 
-const renderer = createPdfRenderer();
+// untrack: libLoader prop change must not reinitialise the renderer
+const renderer = untrack(() => createPdfRenderer(libLoader));
 
 // Canvas and text layer container refs — bound via bind:this
 let canvasEl = $state<HTMLCanvasElement | null>(null);

frontend/src/lib/document/viewer/PdfViewer.svelte.spec.ts

@@ -1,14 +1,18 @@
 import { vi, describe, it, expect, afterEach } from 'vitest';
 import { cleanup, render } from 'vitest-browser-svelte';
 import { page } from 'vitest/browser';
+import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte';
+import PdfViewer from './PdfViewer.svelte';
 
-// pdfjs-dist is a rendering dependency — we mock it so unit tests don't need
-// a real browser PDF engine. The interesting behaviour under test here is the
-// component's own UI logic (controls, page counter), not pdfjs internals.
-vi.mock('pdfjs-dist', () => {
-	function TextLayerMock() {}
-	TextLayerMock.prototype.render = () => Promise.resolve();
-	TextLayerMock.prototype.cancel = () => {};
+afterEach(cleanup);
+
+function makeFakePdfjsLib() {
+	class TextLayerMock {
+		render() {
+			return Promise.resolve();
+		}
+		cancel() {}
+	}
 
 	return {
 		GlobalWorkerOptions: { workerSrc: '' },
@@ -23,31 +27,30 @@ vi.mock('pdfjs-dist', () => {
 			})
 		}),
 		TextLayer: TextLayerMock
-	};
-});
+	} as unknown as typeof import('pdfjs-dist');
+}
 
-vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', () => ({ default: '' }));
-
-import PdfViewer from './PdfViewer.svelte';
-
-afterEach(cleanup);
+function makeFakeLibLoader(): LibLoader {
+	const fakePdfjs = makeFakePdfjsLib();
+	return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
+}
 
 describe('PdfViewer', () => {
 	it('shows previous and next page navigation buttons', async () => {
-		render(PdfViewer, { url: '/api/documents/test-id/file' });
+		render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
 		await expect.element(page.getByRole('button', { name: /zurück/i })).toBeInTheDocument();
 		await expect.element(page.getByRole('button', { name: /weiter/i })).toBeInTheDocument();
 	});
 
 	it('shows zoom controls', async () => {
-		render(PdfViewer, { url: '/api/documents/test-id/file' });
+		render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
 		await expect.element(page.getByRole('button', { name: /vergrößern/i })).toBeInTheDocument();
 		await expect.element(page.getByRole('button', { name: /verkleinern/i })).toBeInTheDocument();
 	});
 
 	it('displays the page counter once the PDF has loaded', async () => {
-		render(PdfViewer, { url: '/api/documents/test-id/file' });
-		// Mock resolves synchronously, so "1 / 2" should appear quickly
+		render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
+		// Fake loader resolves synchronously, so "1 / 2" should appear quickly
 		await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument();
 	});
 });

frontend/src/lib/document/viewer/usePdfRenderer.svelte.test.ts

@@ -1,4 +1,4 @@
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, vi } from 'vitest';
 import { createPdfRenderer } from './usePdfRenderer.svelte';
 
 // Note: init() and loadDocument() require pdfjsLib (browser module).
@@ -173,4 +173,37 @@ describe('createPdfRenderer', () => {
 		r.goToPage(1);
 		expect(r.currentPage).toBe(1);
 	});
+
+	it('calls injected libLoader during init and sets pdfjsReady', async () => {
+		const fakePdfjs = {
+			GlobalWorkerOptions: { workerSrc: '' },
+			getDocument: vi.fn(),
+			TextLayer: class {}
+		} as unknown as typeof import('pdfjs-dist');
+		const fakeLoader = vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
+		const r = createPdfRenderer(fakeLoader);
+		await r.init();
+		expect(fakeLoader).toHaveBeenCalledOnce();
+		expect(r.pdfjsReady).toBe(true);
+	});
+
+	it('leaves pdfjsReady false when libLoader rejects', async () => {
+		const failingLoader = vi.fn().mockRejectedValue(new Error('load failed'));
+		const r = createPdfRenderer(failingLoader);
+		await expect(r.init()).rejects.toThrow('load failed');
+		expect(r.pdfjsReady).toBe(false);
+	});
+
+	it('init() is idempotent — libLoader called only once on repeated calls', async () => {
+		const fakePdfjs = {
+			GlobalWorkerOptions: { workerSrc: '' },
+			getDocument: vi.fn(),
+			TextLayer: class {}
+		} as unknown as typeof import('pdfjs-dist');
+		const fakeLoader = vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
+		const r = createPdfRenderer(fakeLoader);
+		await r.init();
+		await r.init();
+		expect(fakeLoader).toHaveBeenCalledOnce();
+	});
 });

frontend/src/lib/document/viewer/usePdfRenderer.svelte.ts

@@ -1,6 +1,11 @@
 import type { PDFDocumentProxy, RenderTask } from 'pdfjs-dist';
 
-export function createPdfRenderer() {
+export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;
+
+const defaultLibLoader: LibLoader = () =>
+	Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]);
+
+export function createPdfRenderer(libLoader: LibLoader = defaultLibLoader) {
 	// Reactive state — exposed via getters
 	let currentPage = $state(1);
 	let totalPages = $state(0);
@@ -18,10 +23,8 @@ export function createPdfRenderer() {
 	let pdfjsLib: typeof import('pdfjs-dist') | null = null;
 
 	async function init(): Promise<void> {
-		const [lib, { default: workerUrl }] = await Promise.all([
-			import('pdfjs-dist'),
-			import('pdfjs-dist/build/pdf.worker.min.mjs?url')
-		]);
+		if (pdfjsReady) return;
+		const [lib, { default: workerUrl }] = await libLoader();
 		lib.GlobalWorkerOptions.workerSrc = workerUrl;
 		pdfjsLib = lib;
 		pdfjsReady = true;