Compare commits

...

18 Commits

Author SHA1 Message Date
Marcel
3d9e10f3e7 docs(adr-012): add enforcement note and libLoader revisit signal
Some checks failed
CI / fail2ban Regex (push) Successful in 38s
CI / Compose Bucket Idempotency (push) Successful in 56s
CI / Unit & Component Tests (push) Failing after 2m46s
CI / OCR Service Tests (push) Successful in 16s
CI / Backend Unit Tests (push) Successful in 4m12s
CI / OCR Service Tests (pull_request) Has been cancelled
CI / Backend Unit Tests (pull_request) Has been cancelled
CI / fail2ban Regex (pull_request) Has been cancelled
CI / Compose Bucket Idempotency (pull_request) Has been cancelled
CI / Unit & Component Tests (pull_request) Has been cancelled
- Explicitly states no lint rule is planned; CI guard is the backstop
  (addresses Elicit OQ-001 from PR #536 round 4)
- Adds a "when to revisit" note: extract shared DynamicImportLoader<T>
  if 3+ components adopt the libLoader pattern
  (addresses Markus Keller round-4 observation on PR #536)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 09:38:49 +02:00
Marcel
8396d82cb4 ci(coverage): document that birpc guard covers coverage run only
Adds a comment above the assertion step so a future developer diagnosing
a birpc-related failure in `npm test` knows where to find the diagnostic.

Addresses Sara Holt + Tobias Wendt round-4 observation on PR #536.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 09:37:50 +02:00
Marcel
185c754eee refactor(test): convert TextLayerMock to class syntax in PdfViewer spec
Prototype-style assignment was a vi.mock hoisting artifact from the old
version of the file. Rest of the codebase uses class syntax — aligning.

Addresses Felix Brandt round-4 suggestion on PR #536.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 09:36:47 +02:00
Marcel
0b3ce838f4 test(pdf-renderer): guard init() against repeated calls — libLoader must fire once
Adds idempotency test: calling init() twice must invoke libLoader only once.
Adds `if (pdfjsReady) return;` guard to satisfy the contract.

Addresses Felix Brandt round-4 suggestion on PR #536.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 09:25:13 +02:00
Marcel
48b7e0f3a2 ci(coverage): use grep -F for birpc guard to avoid BRE escaping
Some checks failed
CI / Unit & Component Tests (push) Failing after 2m47s
CI / OCR Service Tests (push) Successful in 16s
CI / Backend Unit Tests (push) Successful in 4m12s
CI / fail2ban Regex (push) Successful in 38s
CI / Compose Bucket Idempotency (push) Successful in 55s
CI / Unit & Component Tests (pull_request) Failing after 2m46s
CI / OCR Service Tests (pull_request) Successful in 15s
CI / Backend Unit Tests (pull_request) Successful in 4m9s
CI / fail2ban Regex (pull_request) Successful in 38s
CI / Compose Bucket Idempotency (pull_request) Successful in 56s
-F (fixed string) matches the literal pattern [birpc] rpc is closed
without relying on BRE bracket escaping, making the intent explicit
and immune to accidental regex interpretation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 07:56:28 +02:00
Marcel
017b404f8f ci(coverage): include coverage log in artifact upload
The birpc guard step writes to /tmp/coverage-test-<run_id>.log and exits 1
when a race is detected. Without this file in the artifact, the evidence
disappears when the runner tears down — only the exit code remained visible.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 07:55:49 +02:00
Marcel
58eb6e27d3 refactor(test): remove what-comment from makeFakePdfjsLib
The name already implies it's a fake; the comment described what, not why.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 07:55:11 +02:00
Marcel
2163cd9634 test(pdf-renderer): assert init() re-throws when libLoader rejects
.catch(()=>{}) swallowed the rejection, so the test passed vacuously even
if a future refactor silently caught the error. rejects.toThrow() proves
the propagation contract holds before asserting pdfjsReady stays false.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-12 07:54:33 +02:00
Marcel
e418e884b5 ci(coverage): harden coverage guard step
Some checks failed
CI / Unit & Component Tests (push) Failing after 2m47s
CI / OCR Service Tests (push) Successful in 15s
CI / Backend Unit Tests (push) Successful in 4m4s
CI / fail2ban Regex (push) Successful in 38s
CI / Compose Bucket Idempotency (push) Successful in 55s
CI / Unit & Component Tests (pull_request) Failing after 2m49s
CI / OCR Service Tests (pull_request) Successful in 16s
CI / Backend Unit Tests (pull_request) Successful in 4m9s
CI / fail2ban Regex (pull_request) Successful in 39s
CI / Compose Bucket Idempotency (pull_request) Successful in 56s
- Add explicit set -eo pipefail so npm test:coverage exit code
  propagates through the pipe (not just tee's always-0 exit)
- Scope log file to github.run_id to prevent stale-log false positives
  on retried steps sharing the same runner /tmp
- Tighten grep pattern to \[birpc\] rpc is closed to avoid matching
  unrelated log lines that happen to contain "rpc is closed"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 23:18:22 +02:00
Marcel
d8496498ea test(pdf-renderer): document libLoader rejection leaves pdfjsReady false
Regression-protection test: init() propagates the loader rejection
before pdfjsReady is set, so the renderer stays in a safe unready state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 23:17:30 +02:00
Marcel
b8d9c0e9d5 docs(pdf-viewer): comment untrack invariant on renderer init
Without untrack, a reactive libLoader prop reference change would
reinitialise the whole renderer and lose all loaded state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 23:13:41 +02:00
Marcel
e529f9f7d1 refactor(pdf-viewer): export LibLoader type and update callers
Exporting LibLoader gives the type a stable, named identity.
PdfViewer.svelte and PdfViewer.svelte.spec.ts now import it directly
instead of using Parameters<typeof createPdfRenderer>[0].

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 23:12:55 +02:00
Marcel
11c61c8a77 ci(coverage): simplify coverage step and pin shell to bash
Some checks failed
CI / Unit & Component Tests (push) Failing after 2m47s
CI / OCR Service Tests (push) Successful in 16s
CI / Backend Unit Tests (push) Successful in 4m8s
CI / fail2ban Regex (push) Successful in 38s
CI / Compose Bucket Idempotency (push) Has been cancelled
CI / Unit & Component Tests (pull_request) Failing after 2m48s
CI / OCR Service Tests (pull_request) Successful in 15s
CI / Backend Unit Tests (pull_request) Successful in 4m9s
CI / fail2ban Regex (pull_request) Successful in 38s
CI / Compose Bucket Idempotency (pull_request) Successful in 23s
- removes unreachable `; exit ${PIPESTATUS[0]}` — already covered by pipefail (Tobias)
- adds explicit `shell: bash` to both new steps for clarity (Tobias)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 22:52:33 +02:00
Marcel
3bb940a2c2 refactor(test): move PdfViewer import to top and annotate partial fake cast
- import PdfViewer left mid-file from vi.mock hoisting — no longer needed (Sara/Felix)
- adds one-line comment explaining as unknown as cast is an intentional partial fake (Felix)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 22:49:34 +02:00
Marcel
b9e2ed4af8 docs(adr): 012 — browser-mode test mocking strategy
Some checks failed
CI / Unit & Component Tests (push) Failing after 2m49s
CI / OCR Service Tests (push) Successful in 17s
CI / Backend Unit Tests (push) Successful in 4m13s
CI / fail2ban Regex (push) Successful in 37s
CI / Compose Bucket Idempotency (push) Successful in 56s
CI / Unit & Component Tests (pull_request) Failing after 2m47s
CI / OCR Service Tests (pull_request) Successful in 16s
CI / Backend Unit Tests (pull_request) Successful in 4m16s
CI / fail2ban Regex (pull_request) Successful in 38s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s
Documents why vi.mock(module, factory) races with birpc teardown for
dynamically-imported modules, the libLoader injection pattern used to fix
#535, and the residual exceptions ($app/*, $env/*) that are safe to keep
as vi.mock because they are resolved statically before any test runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 21:32:12 +02:00
Marcel
67f53fcc58 ci(guard): fail unit-tests job if [birpc] rpc is closed appears in coverage run
Captures npm run test:coverage output with tee and adds an always-run step
that greps for the teardown-race fingerprint. Any future regression where a
vi.mock factory races with birpc teardown will now surface as an explicit CI
failure rather than a silent exit-1 after all tests report green (#535).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 21:31:03 +02:00
Marcel
7a4295d403 fix(pdf-viewer): replace vi.mock(pdfjs-dist) with injected libLoader prop
Removes both vi.mock('pdfjs-dist', factory) and
vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', factory) from
PdfViewer.svelte.spec.ts — the ManualMockedModule registrations that were
racing with vitest-browser-playwright's birpc teardown channel.

PdfViewer.svelte now accepts an optional libLoader prop (typed as
Parameters<typeof createPdfRenderer>[0]) that is passed untracked to
createPdfRenderer(). Tests supply a vi.fn() fake loader directly as a prop;
production code uses the default loader that imports the real pdfjs-dist.
The birpc route handler for pdfjs-dist is never registered, so no teardown
race is possible. Fixes #535.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 21:30:15 +02:00
Marcel
49171e596d test(pdf-renderer): inject libLoader into createPdfRenderer to eliminate vi.mock factories
Adds an optional LibLoader parameter (defaults to the real pdfjs-dist dynamic
imports) and a failing test that verified the loader is called during init().
This is the first step toward removing ManualMockedModule registrations that
race with vitest-browser-playwright's birpc teardown (#535).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-11 21:25:38 +02:00
6 changed files with 179 additions and 30 deletions

View File

@@ -43,17 +43,34 @@ jobs:
TZ: Europe/Berlin
- name: Run coverage (server + client)
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

View File

@@ -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.

View File

@@ -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);

View File

@@ -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();
});
});

View File

@@ -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).
@@ -66,4 +66,37 @@ describe('createPdfRenderer', () => {
expect(r.error).toBeNull();
expect(r.loading).toBe(false);
});
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();
});
});

View File

@@ -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;