fix(#535): eliminate vi.mock(pdfjs-dist) birpc teardown race via libLoader injection #536

marcel · 2026-05-11T21:33:27+02:00

marcel commented

2026-05-11 21:33:27 +02:00

Summary

Replaces vi.mock('pdfjs-dist', factory) and vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', factory) in PdfViewer.svelte.spec.ts with prop injection — no ManualMockedModule is registered, so the birpc route handler that races with worker teardown is never installed
Adds optional libLoader parameter to createPdfRenderer() (defaults to the real dynamic imports) and threads it as an optional libLoader prop on PdfViewer.svelte
Adds a permanent CI guard step that captures coverage output and fails the build if [birpc] rpc is closed appears
Adds ADR 012 documenting the rule (prop injection over vi.mock factories for dynamically-imported modules) and the residual exceptions ($app/*, $env/*)

Fixes #535.

Root cause

usePdfRenderer.svelte.ts::init() loads pdfjs-dist and its worker URL via await Promise.all([import('pdfjs-dist'), import(...?url)]) inside onMount. In vitest browser mode, each vi.mock(module, factory) registers a ManualMockedModule served via a playwright route handler over birpc. When Chromium fetches the dynamically-imported module after the worker channel has closed → [birpc] rpc is closed, cannot call "resolveManualMock" → unhandled rejection → exit 1.

Test plan

usePdfRenderer.svelte.test.ts — new test verifies libLoader is called during init() and pdfjsReady becomes true
PdfViewer.svelte.spec.ts — all 3 existing tests pass with injected fake loader, no vi.mock
CI guard step present in .gitea/workflows/ci.yml
60 consecutive green workflow_dispatch CI runs against main after merge (per user decision on quality gate)

🤖 Generated with Claude Code

## Summary - Replaces `vi.mock('pdfjs-dist', factory)` and `vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', factory)` in `PdfViewer.svelte.spec.ts` with prop injection — no `ManualMockedModule` is registered, so the birpc route handler that races with worker teardown is never installed - Adds optional `libLoader` parameter to `createPdfRenderer()` (defaults to the real dynamic imports) and threads it as an optional `libLoader` prop on `PdfViewer.svelte` - Adds a permanent CI guard step that captures coverage output and fails the build if `[birpc] rpc is closed` appears - Adds ADR 012 documenting the rule (prop injection over vi.mock factories for dynamically-imported modules) and the residual exceptions (`$app/*`, `$env/*`) Fixes #535. ## Root cause `usePdfRenderer.svelte.ts::init()` loads `pdfjs-dist` and its worker URL via `await Promise.all([import('pdfjs-dist'), import(...?url)])` inside `onMount`. In vitest browser mode, each `vi.mock(module, factory)` registers a `ManualMockedModule` served via a playwright route handler over birpc. When Chromium fetches the dynamically-imported module after the worker channel has closed → `[birpc] rpc is closed, cannot call "resolveManualMock"` → unhandled rejection → `exit 1`. ## Test plan - [x] `usePdfRenderer.svelte.test.ts` — new test verifies `libLoader` is called during `init()` and `pdfjsReady` becomes `true` - [x] `PdfViewer.svelte.spec.ts` — all 3 existing tests pass with injected fake loader, no `vi.mock` - [x] CI guard step present in `.gitea/workflows/ci.yml` - [ ] 60 consecutive green `workflow_dispatch` CI runs against `main` after merge (per user decision on quality gate) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel commented

2026-05-11 22:04:50 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

The birpc race is fixed cleanly. The LibLoader type + defaultLibLoader + optional override is the right seam: minimal, typed, and only touches the boundary where the problem actually lived. The untrack(() => createPdfRenderer(libLoader)) is correct — without it, Svelte could re-run the factory if libLoader ever became reactive. Good instinct.

Suggestions (no blockers)

1. Misplaced import in PdfViewer.svelte.spec.ts

import PdfViewer from './PdfViewer.svelte' sits after the factory function declarations (~line 35 in the new file). This was a vi.mock hoisting artifact: vitest had to process vi.mock() calls before the static import, so the component import was pushed down. Now that there are no vi.mock calls, the constraint is gone. Leaving it mid-file will confuse contributors who wonder if the position is intentional.

 import type { createPdfRenderer } from '$lib/document/viewer/usePdfRenderer.svelte';
+import PdfViewer from './PdfViewer.svelte';

 afterEach(cleanup);

 function makeFakePdfjsLib() { ... }
 function makeFakeLibLoader() { ... }
-
-import PdfViewer from './PdfViewer.svelte';

2. as unknown as typeof import('pdfjs-dist') double cast

The cast is correct — the fake only implements the methods usePdfRenderer actually calls. Worth a one-liner above it so a future reader doesn't try to "fix" it:

// Partial fake: only the methods used by usePdfRenderer are implemented
} as unknown as typeof import('pdfjs-dist');

3. No unhappy-path test for libLoader rejection

usePdfRenderer.svelte.test.ts now tests the happy path (loader resolves → pdfjsReady === true). There's no test for libLoader rejecting (network failure, missing bundle). The error state presumably gets set — a follow-up issue would close this gap, but it's out of scope for #535.

What's well done: factory functions makeFakePdfjsLib / makeFakeLibLoader are clean and readable; afterEach(cleanup) is correctly placed; test name reads as a sentence; vi.fn().mockResolvedValue is the right tool for an async fake.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** The birpc race is fixed cleanly. The `LibLoader` type + `defaultLibLoader` + optional override is the right seam: minimal, typed, and only touches the boundary where the problem actually lived. The `untrack(() => createPdfRenderer(libLoader))` is correct — without it, Svelte could re-run the factory if `libLoader` ever became reactive. Good instinct. ### Suggestions (no blockers) **1. Misplaced import in `PdfViewer.svelte.spec.ts`** `import PdfViewer from './PdfViewer.svelte'` sits after the factory function declarations (~line 35 in the new file). This was a `vi.mock` hoisting artifact: vitest had to process `vi.mock()` calls before the static import, so the component import was pushed down. Now that there are no `vi.mock` calls, the constraint is gone. Leaving it mid-file will confuse contributors who wonder if the position is intentional. ```diff import type { createPdfRenderer } from '$lib/document/viewer/usePdfRenderer.svelte'; +import PdfViewer from './PdfViewer.svelte'; afterEach(cleanup); function makeFakePdfjsLib() { ... } function makeFakeLibLoader() { ... } - -import PdfViewer from './PdfViewer.svelte'; ``` **2. `as unknown as typeof import('pdfjs-dist')` double cast** The cast is correct — the fake only implements the methods `usePdfRenderer` actually calls. Worth a one-liner above it so a future reader doesn't try to "fix" it: ```typescript // Partial fake: only the methods used by usePdfRenderer are implemented } as unknown as typeof import('pdfjs-dist'); ``` **3. No unhappy-path test for `libLoader` rejection** `usePdfRenderer.svelte.test.ts` now tests the happy path (loader resolves → `pdfjsReady === true`). There's no test for `libLoader` rejecting (network failure, missing bundle). The `error` state presumably gets set — a follow-up issue would close this gap, but it's out of scope for #535. **What's well done:** factory functions `makeFakePdfjsLib` / `makeFakeLibLoader` are clean and readable; `afterEach(cleanup)` is correctly placed; test name reads as a sentence; `vi.fn().mockResolvedValue` is the right tool for an async fake.

marcel commented

2026-05-11 22:05:00 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

ADR 012 is exactly right. Root cause documented, decision stated, residual exceptions table for $app/* / $env/*, consequences including the CI guard. This is the kind of ADR that prevents a future developer from "simplifying" the code back into the broken state.

Architecture assessment

The LibLoader type is placed at the correct abstraction level — it wraps exactly the two dynamic imports that caused the race, not the whole PDF rendering pipeline. The defaultLibLoader stays with usePdfRenderer.svelte.ts, which owns the dependency. PdfViewer.svelte passes it through as an optional prop using Parameters<typeof createPdfRenderer>[0] — this keeps the type in sync with the actual parameter automatically without a separate type alias.

untrack(() => createPdfRenderer(libLoader)) is the correct Svelte 5 pattern here. Without it, if a parent ever passes a reactive expression as libLoader, Svelte would re-run the factory on each reactive update, creating a new renderer instance and losing all loaded state. Good defensive use of untrack.

Documentation checklist

Trigger	Required update	Status
New architectural decision with lasting consequences	New ADR in `docs/adr/`	✅ ADR 012 added
No new Flyway migration	DB diagrams — n/a	✅
No new backend package/controller	C4 backend diagrams — n/a	✅
No new SvelteKit route	Route table — n/a	✅

All satisfied.

One note (non-blocking)

libLoader is a test-only escape hatch surfaced as a public prop. This is intentional and documented in ADR 012's "libLoader pattern" section. If the component API ever gets an explicit JSDoc or prop documentation pass, a @internal or @testing annotation on this prop would make the intent visible at the call site without needing to read the ADR. Not needed now — just flagging for if the API surface ever grows.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ADR 012 is exactly right. Root cause documented, decision stated, residual exceptions table for `$app/*` / `$env/*`, consequences including the CI guard. This is the kind of ADR that prevents a future developer from "simplifying" the code back into the broken state. ### Architecture assessment The `LibLoader` type is placed at the correct abstraction level — it wraps exactly the two dynamic imports that caused the race, not the whole PDF rendering pipeline. The `defaultLibLoader` stays with `usePdfRenderer.svelte.ts`, which owns the dependency. `PdfViewer.svelte` passes it through as an optional prop using `Parameters<typeof createPdfRenderer>[0]` — this keeps the type in sync with the actual parameter automatically without a separate type alias. `untrack(() => createPdfRenderer(libLoader))` is the correct Svelte 5 pattern here. Without it, if a parent ever passes a reactive expression as `libLoader`, Svelte would re-run the factory on each reactive update, creating a new renderer instance and losing all loaded state. Good defensive use of `untrack`. ### Documentation checklist | Trigger | Required update | Status | |---|---|---| | New architectural decision with lasting consequences | New ADR in `docs/adr/` | ✅ ADR 012 added | | No new Flyway migration | DB diagrams — n/a | ✅ | | No new backend package/controller | C4 backend diagrams — n/a | ✅ | | No new SvelteKit route | Route table — n/a | ✅ | All satisfied. ### One note (non-blocking) `libLoader` is a test-only escape hatch surfaced as a public prop. This is intentional and documented in ADR 012's "libLoader pattern" section. If the component API ever gets an explicit JSDoc or prop documentation pass, a `@internal` or `@testing` annotation on this prop would make the intent visible at the call site without needing to read the ADR. Not needed now — just flagging for if the API surface ever grows.

marcel commented

2026-05-11 22:05:08 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ⚠️ Approved with concerns

The CI guard is exactly the right move. Adding a permanent grep check for rpc is closed means regressions don't silently re-introduce the flaky exit-1 pattern. if: always() ensures the assertion runs even when tests fail — correct.

Concern — `exit ${PIPESTATUS[0]}` is unreachable under pipefail

Gitea Actions (mirroring GitHub Actions) runs bash steps with set -eo pipefail by default. Under these settings:

npm run test:coverage 2>&1 | tee /tmp/coverage-test.log; exit ${PIPESTATUS[0]}

If npm run test:coverage exits non-zero, pipefail causes the whole pipe to fail and bash exits immediately — the ; exit ${PIPESTATUS[0]} is never reached. tee still writes the log because it ran concurrently, so the capture works correctly. The explicit exit is harmless but will confuse future maintainers wondering "why is PIPESTATUS here?"

Simpler and equivalent under pipefail:

- name: Run coverage (server + client)
  run: npm run test:coverage 2>&1 | tee /tmp/coverage-test.log
  working-directory: frontend
  env:
    TZ: Europe/Berlin

Under pipefail, the step exit code is the maximum of all pipe segments. No need to re-read PIPESTATUS explicitly.

Minor — add `shell: bash` for explicitness

The default shell on Linux Gitea runners is bash, but making it explicit removes ambiguity:

- name: Run coverage (server + client)
  shell: bash
  run: npm run test:coverage 2>&1 | tee /tmp/coverage-test.log

- name: Assert no birpc teardown race in coverage run
  shell: bash
  if: always()
  run: |
    if grep -q "rpc is closed" /tmp/coverage-test.log 2>/dev/null; then

What's good

if: always() on the guard step is correct — it must run even when tests fail ✅
2>/dev/null in the grep guards against the log file not existing (e.g., if the tee step itself failed) ✅
The guard is a permanent fixture, not a one-off check ✅

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ⚠️ Approved with concerns** The CI guard is exactly the right move. Adding a permanent grep check for `rpc is closed` means regressions don't silently re-introduce the flaky exit-1 pattern. `if: always()` ensures the assertion runs even when tests fail — correct. ### Concern — `exit ${PIPESTATUS[0]}` is unreachable under pipefail Gitea Actions (mirroring GitHub Actions) runs bash steps with `set -eo pipefail` by default. Under these settings: ```bash npm run test:coverage 2>&1 | tee /tmp/coverage-test.log; exit ${PIPESTATUS[0]} ``` If `npm run test:coverage` exits non-zero, `pipefail` causes the whole pipe to fail and bash exits immediately — the `; exit ${PIPESTATUS[0]}` is never reached. `tee` still writes the log because it ran concurrently, so the capture works correctly. The explicit `exit` is harmless but will confuse future maintainers wondering "why is `PIPESTATUS` here?" Simpler and equivalent under pipefail: ```yaml - name: Run coverage (server + client) run: npm run test:coverage 2>&1 | tee /tmp/coverage-test.log working-directory: frontend env: TZ: Europe/Berlin ``` Under `pipefail`, the step exit code is the maximum of all pipe segments. No need to re-read `PIPESTATUS` explicitly. ### Minor — add `shell: bash` for explicitness The default shell on Linux Gitea runners is bash, but making it explicit removes ambiguity: ```yaml - name: Run coverage (server + client) shell: bash run: npm run test:coverage 2>&1 | tee /tmp/coverage-test.log - name: Assert no birpc teardown race in coverage run shell: bash if: always() run: | if grep -q "rpc is closed" /tmp/coverage-test.log 2>/dev/null; then ``` ### What's good - `if: always()` on the guard step is correct — it must run even when tests fail ✅ - `2>/dev/null` in the grep guards against the log file not existing (e.g., if the tee step itself failed) ✅ - The guard is a permanent fixture, not a one-off check ✅

marcel commented

2026-05-11 22:05:14 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

No security-sensitive code paths touched in this PR. Brief audit of each changed surface:

usePdfRenderer.svelte.ts — defaultLibLoader
Constructs hardcoded static import paths (import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')). No user input is interpolated into import paths. No SSRF vector.

PdfViewer.svelte — libLoader prop
Accepts a function reference. In production, this is always undefined (falls through to defaultLibLoader). In tests, it's a synchronous fake returning in-memory objects. No user-controlled data flows through this prop. No eval, no dynamic string construction, no HTTP requests constructed from external input.

ci.yml — grep -q "rpc is closed"
Pattern is hardcoded — no shell injection possible. The 2>/dev/null silently handles a missing log file rather than expanding untrusted input.

docs/adr/012-browser-test-mocking-strategy.md
Documentation only. No executable code.

PdfViewer.svelte.spec.ts / usePdfRenderer.svelte.test.ts
Test-only code, never shipped to production.

Nothing to flag. Clean PR from a security standpoint.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** No security-sensitive code paths touched in this PR. Brief audit of each changed surface: **`usePdfRenderer.svelte.ts` — `defaultLibLoader`** Constructs hardcoded static import paths (`import('pdfjs-dist')`, `import('pdfjs-dist/build/pdf.worker.min.mjs?url')`). No user input is interpolated into import paths. No SSRF vector. **`PdfViewer.svelte` — `libLoader` prop** Accepts a function reference. In production, this is always `undefined` (falls through to `defaultLibLoader`). In tests, it's a synchronous fake returning in-memory objects. No user-controlled data flows through this prop. No eval, no dynamic string construction, no HTTP requests constructed from external input. **`ci.yml` — `grep -q "rpc is closed"`** Pattern is hardcoded — no shell injection possible. The `2>/dev/null` silently handles a missing log file rather than expanding untrusted input. **`docs/adr/012-browser-test-mocking-strategy.md`** Documentation only. No executable code. **`PdfViewer.svelte.spec.ts` / `usePdfRenderer.svelte.test.ts`** Test-only code, never shipped to production. Nothing to flag. Clean PR from a security standpoint.

marcel commented

2026-05-11 22:05:23 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The root fix is right: removing vi.mock(externalLib, factory) from browser-mode specs eliminates the birpc race at the source. The prop injection pattern is the correct level of abstraction and the factory functions are clean.

Blocker — misplaced `import PdfViewer` in `PdfViewer.svelte.spec.ts`

This is a vi.mock hoisting artifact that should be cleaned up now that vi.mock is gone. The file currently reads:

import type { createPdfRenderer } from '...';   // line ~4
afterEach(cleanup);                              // line 6
function makeFakePdfjsLib() { ... }              // lines 8–28
function makeFakeLibLoader() { ... }             // lines 30–33
import PdfViewer from './PdfViewer.svelte';      // ← mid-file import

vi.mock() calls had to precede the static component import for vitest to hoist them correctly. With vi.mock removed, this constraint is gone. A mid-file import will mislead future contributors into thinking the placement is intentional and required.

Fix: move import PdfViewer from './PdfViewer.svelte' to the top of the file with the other imports.

Suggestion — add error-path test for `libLoader` rejection

usePdfRenderer.svelte.test.ts now covers: loader resolves → pdfjsReady === true. Not covered: loader rejects. What does the renderer expose in that case? error state? Silent failure? This gap is out of scope for #535 (the issue is about exit-1 from the birpc race, not error handling), but a follow-up issue would close it.

What's done well

makeFakePdfjsLib() / makeFakeLibLoader() — clean factory functions, single-responsibility, readable ✅
afterEach(cleanup) — prevents DOM state bleed between tests ✅
New test name 'calls injected libLoader during init and sets pdfjsReady' — reads as a sentence describing an observable behavior ✅
CI guard with if: always() — runs even when tests fail, catches regressions before they accumulate ✅
vi.fn().mockResolvedValue(...) — correct tool for an async DI fake in vitest ✅
Three existing PdfViewer tests updated to pass libLoader — all behaviors still covered ✅

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The root fix is right: removing `vi.mock(externalLib, factory)` from browser-mode specs eliminates the birpc race at the source. The prop injection pattern is the correct level of abstraction and the factory functions are clean. ### Blocker — misplaced `import PdfViewer` in `PdfViewer.svelte.spec.ts` This is a `vi.mock` hoisting artifact that should be cleaned up now that `vi.mock` is gone. The file currently reads: ```typescript import type { createPdfRenderer } from '...'; // line ~4 afterEach(cleanup); // line 6 function makeFakePdfjsLib() { ... } // lines 8–28 function makeFakeLibLoader() { ... } // lines 30–33 import PdfViewer from './PdfViewer.svelte'; // ← mid-file import ``` `vi.mock()` calls had to precede the static component import for vitest to hoist them correctly. With `vi.mock` removed, this constraint is gone. A mid-file import will mislead future contributors into thinking the placement is intentional and required. Fix: move `import PdfViewer from './PdfViewer.svelte'` to the top of the file with the other imports. ### Suggestion — add error-path test for `libLoader` rejection `usePdfRenderer.svelte.test.ts` now covers: loader resolves → `pdfjsReady === true`. Not covered: loader rejects. What does the renderer expose in that case? `error` state? Silent failure? This gap is out of scope for #535 (the issue is about exit-1 from the birpc race, not error handling), but a follow-up issue would close it. ### What's done well - `makeFakePdfjsLib()` / `makeFakeLibLoader()` — clean factory functions, single-responsibility, readable ✅ - `afterEach(cleanup)` — prevents DOM state bleed between tests ✅ - New test name `'calls injected libLoader during init and sets pdfjsReady'` — reads as a sentence describing an observable behavior ✅ - CI guard with `if: always()` — runs even when tests fail, catches regressions before they accumulate ✅ - `vi.fn().mockResolvedValue(...)` — correct tool for an async DI fake in vitest ✅ - Three existing `PdfViewer` tests updated to pass `libLoader` — all behaviors still covered ✅

marcel commented

2026-05-11 22:05:28 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This PR makes no changes to rendered UI, visual layout, component structure, accessibility markup, or design tokens. Nothing in scope for a UI/UX review.

What I verified:

PdfViewer.svelte: the only template change is libLoader = undefined added to the props destructuring and untrack(() => createPdfRenderer(libLoader)) replacing createPdfRenderer(). Zero changes to markup, ARIA attributes, Tailwind classes, focus management, or event handlers.
No new Svelte components created.
No changes to layout.css, design tokens, or typography.
The libLoader prop is an internal testing seam — it has no user-visible effect in production (it defaults to undefined, which routes to the real library).

LGTM. The PDF viewer's rendered output and accessibility behaviour are unchanged.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This PR makes no changes to rendered UI, visual layout, component structure, accessibility markup, or design tokens. Nothing in scope for a UI/UX review. **What I verified:** - `PdfViewer.svelte`: the only template change is `libLoader = undefined` added to the props destructuring and `untrack(() => createPdfRenderer(libLoader))` replacing `createPdfRenderer()`. Zero changes to markup, ARIA attributes, Tailwind classes, focus management, or event handlers. - No new Svelte components created. - No changes to `layout.css`, design tokens, or typography. - The `libLoader` prop is an internal testing seam — it has no user-visible effect in production (it defaults to `undefined`, which routes to the real library). LGTM. The PDF viewer's rendered output and accessibility behaviour are unchanged.

marcel commented

2026-05-11 22:05:40 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing from a requirements traceability and acceptance-criteria standpoint.

Issue #535 traceability

The PR description traces back to issue #535 with specificity: which module (usePdfRenderer.svelte.ts::init()), which dynamic imports triggered the race, the exact error ([birpc] rpc is closed, cannot call "resolveManualMock"), why it happened (late module fetch after worker teardown), and what the fix does. This is good requirements discipline — it's clear what was broken and that the fix addresses the root cause, not a symptom.

Test plan coverage

Acceptance criterion	Status in this PR
`usePdfRenderer` — new test verifies `libLoader` is called during `init()` and `pdfjsReady` becomes `true`	✅ `usePdfRenderer.svelte.test.ts`
`PdfViewer.svelte.spec.ts` — all 3 existing tests pass with injected fake loader, no `vi.mock`	✅ spec updated, `vi.mock` removed
CI guard step present in `.gitea/workflows/ci.yml`	✅ `Assert no birpc teardown race` step added
60 consecutive green `workflow_dispatch` CI runs post-merge	⬜ Explicitly open — deferred to post-merge verification

The deferred 60-run criterion is honest and appropriate. Marking it open rather than claiming it done prematurely is good requirements hygiene.

One observation (non-blocking)

The 60-run quality gate lives only in the PR description's open checkbox. For a solo project, this is pragmatic. If you want to ensure it's not forgotten after merge, consider adding a follow-up issue on #535 (or a comment on the issue itself) as a reminder to close the issue only after the run count is met. The PR description will still be visible in the merge commit, so it won't disappear — this is just a process note, not a requirement.

ADR 012 quality

Context, decision, pattern with code examples, residual exceptions table with justifications, and consequences — all present. The acceptance criterion for #535 is stated in the Consequences section, which is the right place for it.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing from a requirements traceability and acceptance-criteria standpoint. ### Issue #535 traceability The PR description traces back to issue #535 with specificity: which module (`usePdfRenderer.svelte.ts::init()`), which dynamic imports triggered the race, the exact error (`[birpc] rpc is closed, cannot call "resolveManualMock"`), why it happened (late module fetch after worker teardown), and what the fix does. This is good requirements discipline — it's clear what was broken and that the fix addresses the root cause, not a symptom. ### Test plan coverage | Acceptance criterion | Status in this PR | |---|---| | `usePdfRenderer` — new test verifies `libLoader` is called during `init()` and `pdfjsReady` becomes `true` | ✅ `usePdfRenderer.svelte.test.ts` | | `PdfViewer.svelte.spec.ts` — all 3 existing tests pass with injected fake loader, no `vi.mock` | ✅ spec updated, `vi.mock` removed | | CI guard step present in `.gitea/workflows/ci.yml` | ✅ `Assert no birpc teardown race` step added | | 60 consecutive green `workflow_dispatch` CI runs post-merge | ⬜ Explicitly open — deferred to post-merge verification | The deferred 60-run criterion is honest and appropriate. Marking it open rather than claiming it done prematurely is good requirements hygiene. ### One observation (non-blocking) The 60-run quality gate lives only in the PR description's open checkbox. For a solo project, this is pragmatic. If you want to ensure it's not forgotten after merge, consider adding a follow-up issue on #535 (or a comment on the issue itself) as a reminder to close the issue only after the run count is met. The PR description will still be visible in the merge commit, so it won't disappear — this is just a process note, not a requirement. ### ADR 012 quality Context, decision, pattern with code examples, residual exceptions table with justifications, and consequences — all present. The acceptance criterion for #535 is stated in the Consequences section, which is the right place for it.

marcel referenced this pull request

2026-05-11 22:46:07 +02:00

bug(test): flaky browser-mode test — admin edit-user unsaved-changes guard #538

marcel commented

2026-05-11 22:55:27 +02:00

Review feedback addressed

Two commits pushed to this branch:

`3bb940a2` — refactor(test): move PdfViewer import to top and annotate partial fake cast

Sara (blocker) + Felix (suggestion): import PdfViewer from './PdfViewer.svelte' moved from line 34 to line 5 with the other imports — the vi.mock hoisting constraint that forced it down no longer exists
Felix (suggestion): one-line comment added above as unknown as typeof import('pdfjs-dist') explaining the cast is an intentional partial fake

`11c61c8a` — ci(coverage): simplify coverage step and pin shell to bash

Tobias (suggestion): removed unreachable ; exit ${PIPESTATUS[0]} — under bash's default pipefail, the pipeline itself propagates the exit code and the explicit exit is never reached
Tobias (minor): added shell: bash to both new CI steps (coverage run and birpc assertion) for explicitness

Deferred (out of scope for #535):

Unhappy-path test for libLoader rejection (when the loader rejects, does error state get set?) — filed as #538 for a follow-up

Pre-existing unrelated failures:

src/routes/admin/users/[id]/page.svelte.spec.ts — "Admin edit user page – unsaved-changes guard" intermittently fails in browser mode (goto mock records 0 calls after a discard button click) — filed as #538 for a fix

Wait, I made an error above — the flaky test and the libLoader unhappy path are two different issues. The flaky test was filed as #538. The libLoader unhappy path was called out by Felix and Sara as a suggestion for a follow-up but no separate issue was filed yet — it can be added to the #535 follow-up or filed separately if needed.

## Review feedback addressed Two commits pushed to this branch: ### `3bb940a2` — refactor(test): move PdfViewer import to top and annotate partial fake cast - **Sara (blocker) + Felix (suggestion):** `import PdfViewer from './PdfViewer.svelte'` moved from line 34 to line 5 with the other imports — the `vi.mock` hoisting constraint that forced it down no longer exists - **Felix (suggestion):** one-line comment added above `as unknown as typeof import('pdfjs-dist')` explaining the cast is an intentional partial fake ### `11c61c8a` — ci(coverage): simplify coverage step and pin shell to bash - **Tobias (suggestion):** removed unreachable `; exit ${PIPESTATUS[0]}` — under bash's default `pipefail`, the pipeline itself propagates the exit code and the explicit `exit` is never reached - **Tobias (minor):** added `shell: bash` to both new CI steps (coverage run and birpc assertion) for explicitness --- **Deferred (out of scope for #535):** - Unhappy-path test for `libLoader` rejection (when the loader rejects, does `error` state get set?) — filed as **#538** for a follow-up **Pre-existing unrelated failures:** - `src/routes/admin/users/[id]/page.svelte.spec.ts` — "Admin edit user page – unsaved-changes guard" intermittently fails in browser mode (goto mock records 0 calls after a discard button click) — filed as **#538** for a fix Wait, I made an error above — the flaky test and the libLoader unhappy path are two different issues. The flaky test was filed as #538. The libLoader unhappy path was called out by Felix and Sara as a suggestion for a follow-up but no separate issue was filed yet — it can be added to the #535 follow-up or filed separately if needed.

marcel commented

2026-05-11 23:02:19 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

This is clean, well-motivated work. The LibLoader injection pattern is idiomatic, the factory functions in the spec are correct, and the untrack(() => createPdfRenderer(libLoader)) guard is exactly right — it prevents Svelte from treating libLoader as a reactive dependency and recreating the renderer if the prop reference ever changes.

Blockers

None.

Suggestions

1. Export LibLoader so PdfViewer.svelte doesn't need Parameters<…>[0]

PdfViewer.svelte currently types the prop as:

libLoader?: Parameters<typeof createPdfRenderer>[0];

This works, but Parameters<typeof createPdfRenderer>[0] is an indirect inference that breaks if the signature changes shape. Since LibLoader is already defined in usePdfRenderer.svelte.ts, just export it:

// usePdfRenderer.svelte.ts
export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;

Then in PdfViewer.svelte:

import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte';
// …
libLoader?: LibLoader;

This makes the intent explicit and keeps the type definition in one place.

2. Add an error-path test for init() with a rejecting loader

The new usePdfRenderer.svelte.test.ts test verifies the happy path. A failing loader is a real edge case (network, import error in tests):

it('sets error state when libLoader rejects', async () => {
    const fakeLoader = vi.fn().mockRejectedValue(new Error('load failed'));
    const r = createPdfRenderer(fakeLoader);
    await r.init().catch(() => {});
    // verify the renderer exposes the error / pdfjsReady stays false
    expect(r.pdfjsReady).toBe(false);
});

Whether createPdfRenderer exposes an error state or simply propagates the rejection determines the exact assertion — but the path should be covered.

3. Minor: afterEach(cleanup) placement

Moving afterEach(cleanup) above the factory functions (as done here) is fine, but the standard pattern in this codebase places it inside the describe block so it's scoped explicitly. Either way is correct — noting it for consistency.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** This is clean, well-motivated work. The `LibLoader` injection pattern is idiomatic, the factory functions in the spec are correct, and the `untrack(() => createPdfRenderer(libLoader))` guard is exactly right — it prevents Svelte from treating `libLoader` as a reactive dependency and recreating the renderer if the prop reference ever changes. ### Blockers None. ### Suggestions **1. Export `LibLoader` so `PdfViewer.svelte` doesn't need `Parameters<…>[0]`** `PdfViewer.svelte` currently types the prop as: ```svelte libLoader?: Parameters<typeof createPdfRenderer>[0]; ``` This works, but `Parameters<typeof createPdfRenderer>[0]` is an indirect inference that breaks if the signature changes shape. Since `LibLoader` is already defined in `usePdfRenderer.svelte.ts`, just export it: ```typescript // usePdfRenderer.svelte.ts export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>; ``` Then in `PdfViewer.svelte`: ```svelte import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte'; // … libLoader?: LibLoader; ``` This makes the intent explicit and keeps the type definition in one place. **2. Add an error-path test for `init()` with a rejecting loader** The new `usePdfRenderer.svelte.test.ts` test verifies the happy path. A failing loader is a real edge case (network, import error in tests): ```typescript it('sets error state when libLoader rejects', async () => { const fakeLoader = vi.fn().mockRejectedValue(new Error('load failed')); const r = createPdfRenderer(fakeLoader); await r.init().catch(() => {}); // verify the renderer exposes the error / pdfjsReady stays false expect(r.pdfjsReady).toBe(false); }); ``` Whether `createPdfRenderer` exposes an error state or simply propagates the rejection determines the exact assertion — but the path should be covered. **3. Minor: `afterEach(cleanup)` placement** Moving `afterEach(cleanup)` above the factory functions (as done here) is fine, but the standard pattern in this codebase places it inside the `describe` block so it's scoped explicitly. Either way is correct — noting it for consistency.

marcel commented

2026-05-11 23:02:26 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR is entirely in the test infrastructure layer — no new endpoints, no authentication changes, no data flows modified, no user input processed. From a security perspective, there is nothing to flag.

What I checked

New attack surface: None. libLoader is an optional DI prop used only in test environments; the production code path uses the hard-coded defaultLibLoader which calls import('pdfjs-dist') directly — same as before this PR.
Secrets / credentials in CI: The new CI step Assert no birpc teardown race in coverage run uses grep on a local log file. No credentials, no external calls, no injected inputs.
CI step injection risk: The guard step is a static string — grep -q "rpc is closed" — with no user-controlled inputs. Safe.
as unknown as typeof import('pdfjs-dist'): This cast is confined to test code and carries no runtime security implication. Acceptable.

The CI guard (shell: bash, if: always()) is a good defensive posture — it catches regressions without any security cost.

LGTM.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR is entirely in the test infrastructure layer — no new endpoints, no authentication changes, no data flows modified, no user input processed. From a security perspective, there is nothing to flag. ### What I checked - **New attack surface**: None. `libLoader` is an optional DI prop used only in test environments; the production code path uses the hard-coded `defaultLibLoader` which calls `import('pdfjs-dist')` directly — same as before this PR. - **Secrets / credentials in CI**: The new CI step `Assert no birpc teardown race in coverage run` uses `grep` on a local log file. No credentials, no external calls, no injected inputs. - **CI step injection risk**: The guard step is a static string — `grep -q "rpc is closed"` — with no user-controlled inputs. Safe. - **`as unknown as typeof import('pdfjs-dist')`**: This cast is confined to test code and carries no runtime security implication. Acceptable. The CI guard (`shell: bash`, `if: always()`) is a good defensive posture — it catches regressions without any security cost. LGTM.

marcel commented

2026-05-11 23:02:40 +02:00

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

The core fix is well-tested. The new usePdfRenderer unit test proves the injected loader is called and pdfjsReady becomes true. The three PdfViewer browser-mode specs now pass a makeFakeLibLoader() per test, which is the correct shape. The CI guard (if: always(), tee to capture output, 2>/dev/null on the grep) is defensively written.

Blockers

None.

Suggestions

1. PdfViewer specs don't assert libLoader was actually called

The three existing spec cases verify DOM output (buttons present, counter shows "1 / 2"). The third test implicitly exercises the loader path (counter appears only after init() resolves), but the first two tests pass even if the loader is never invoked. Consider adding a targeted assertion to at least one spec:

it('calls the injected libLoader during mount', async () => {
    const loader = makeFakeLibLoader();
    render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: loader });
    // wait for any async init — the page counter is the ready signal
    await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument();
    expect(loader).toHaveBeenCalledOnce();
});

This would catch a regression where createPdfRenderer(libLoader) stops forwarding the loader without breaking the DOM.

2. No test for the libLoader rejection path

init() is async and awaits the loader. If the loader rejects, the renderer currently propagates the exception. There's no test proving what the component does in that state (error boundary? silent fail?). At minimum a unit test in usePdfRenderer.svelte.test.ts covering the rejection path would prevent a silent regression.

3. CI guard: log file path is not cleaned up between re-runs

/tmp/coverage-test.log persists across retried job steps on the same runner. If the coverage step is retried (e.g., a transient infra failure), the second run's log would append to the first and the guard could flag a stale rpc is closed from a previous attempt. Consider:

run: npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}.log

And update the grep step accordingly. Low probability in practice but worth the one-liner fix.

4. Acceptance criterion tracking

The PR body correctly notes "60 consecutive green workflow_dispatch CI runs" as an unchecked post-merge gate. I'd suggest creating a Gitea issue to track that criterion so it doesn't get forgotten once this PR is merged and out of view.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** The core fix is well-tested. The new `usePdfRenderer` unit test proves the injected loader is called and `pdfjsReady` becomes `true`. The three `PdfViewer` browser-mode specs now pass a `makeFakeLibLoader()` per test, which is the correct shape. The CI guard (`if: always()`, `tee` to capture output, `2>/dev/null` on the grep) is defensively written. ### Blockers None. ### Suggestions **1. PdfViewer specs don't assert `libLoader` was actually called** The three existing spec cases verify DOM output (buttons present, counter shows "1 / 2"). The third test implicitly exercises the loader path (counter appears only after `init()` resolves), but the first two tests pass even if the loader is never invoked. Consider adding a targeted assertion to at least one spec: ```typescript it('calls the injected libLoader during mount', async () => { const loader = makeFakeLibLoader(); render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: loader }); // wait for any async init — the page counter is the ready signal await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument(); expect(loader).toHaveBeenCalledOnce(); }); ``` This would catch a regression where `createPdfRenderer(libLoader)` stops forwarding the loader without breaking the DOM. **2. No test for the `libLoader` rejection path** `init()` is `async` and awaits the loader. If the loader rejects, the renderer currently propagates the exception. There's no test proving what the component does in that state (error boundary? silent fail?). At minimum a unit test in `usePdfRenderer.svelte.test.ts` covering the rejection path would prevent a silent regression. **3. CI guard: log file path is not cleaned up between re-runs** `/tmp/coverage-test.log` persists across retried job steps on the same runner. If the coverage step is retried (e.g., a transient infra failure), the second run's log would append to the first and the guard could flag a stale `rpc is closed` from a previous attempt. Consider: ```yaml run: npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}.log ``` And update the grep step accordingly. Low probability in practice but worth the one-liner fix. **4. Acceptance criterion tracking** The PR body correctly notes "60 consecutive green `workflow_dispatch` CI runs" as an unchecked post-merge gate. I'd suggest creating a Gitea issue to track that criterion so it doesn't get forgotten once this PR is merged and out of view.

marcel commented

2026-05-11 23:02:43 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This PR makes zero changes to user-facing UI, templates, styles, or interaction patterns. No visual regions were added, removed, or restyled. No accessibility attributes were touched. No responsive layout changes occurred.

What I verified

PdfViewer.svelte: only the <script> block changed (added libLoader prop and untrack import). The template markup is untouched.
No new Svelte components introduced.
No Tailwind classes, CSS tokens, or layout structure modified.
No i18n strings added or removed.

Nothing to flag from a UI/UX or accessibility perspective.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This PR makes zero changes to user-facing UI, templates, styles, or interaction patterns. No visual regions were added, removed, or restyled. No accessibility attributes were touched. No responsive layout changes occurred. ### What I verified - `PdfViewer.svelte`: only the `<script>` block changed (added `libLoader` prop and `untrack` import). The template markup is untouched. - No new Svelte components introduced. - No Tailwind classes, CSS tokens, or layout structure modified. - No i18n strings added or removed. Nothing to flag from a UI/UX or accessibility perspective.

marcel commented

2026-05-11 23:02:54 +02:00

🏗️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

The design is architecturally clean. The LibLoader abstraction is a textbook dependency-inversion seam: the module owns the type, exposes a default, and accepts an override at construction time. Nothing leaks across domain boundaries.

What I checked against the doc-update table

PR contains	Required doc update	Status
Architectural decision with lasting consequences (new pattern)	New ADR in `docs/adr/`	✅ ADR 012 present
New SvelteKit route	`CLAUDE.md` route table	N/A — no new route
New backend domain / controller / service	C4 diagram update	N/A — frontend only

ADR 012 is well-structured: context explains the birpc mechanism, decision names the rule precisely, residual exceptions table documents what intentionally stays as vi.mock, consequences are explicit. No issues.

Suggestions (not blockers)

1. Export LibLoader from usePdfRenderer.svelte.ts

The type is defined but not exported. PdfViewer.svelte works around this with Parameters<typeof createPdfRenderer>[0], which couples the prop type to the function signature shape rather than the named concept. Exporting LibLoader gives the type a stable, referenceable identity:

export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;

This is consistent with how the ADR describes the pattern — the type is a named concept worth surfacing.

2. ADR residual-exceptions table: consider adding a "safe because" column

The current table has "Why it stays as vi.mock" which is good. One future reader's question is: "how do we know $app/navigation is resolved statically?" Adding a brief note — e.g., "resolved at spec module-eval time, before any test runs" — inline in that column would make the ADR self-sufficient without needing to trace back to the context section.

## 🏗️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** The design is architecturally clean. The `LibLoader` abstraction is a textbook dependency-inversion seam: the module owns the type, exposes a default, and accepts an override at construction time. Nothing leaks across domain boundaries. ### What I checked against the doc-update table | PR contains | Required doc update | Status | |---|---|---| | Architectural decision with lasting consequences (new pattern) | New ADR in `docs/adr/` | ✅ ADR 012 present | | New SvelteKit route | `CLAUDE.md` route table | N/A — no new route | | New backend domain / controller / service | C4 diagram update | N/A — frontend only | ADR 012 is well-structured: context explains the birpc mechanism, decision names the rule precisely, residual exceptions table documents what intentionally stays as `vi.mock`, consequences are explicit. No issues. ### Suggestions (not blockers) **1. Export `LibLoader` from `usePdfRenderer.svelte.ts`** The type is defined but not exported. `PdfViewer.svelte` works around this with `Parameters<typeof createPdfRenderer>[0]`, which couples the prop type to the function signature shape rather than the named concept. Exporting `LibLoader` gives the type a stable, referenceable identity: ```typescript export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>; ``` This is consistent with how the ADR describes the pattern — the type is a named concept worth surfacing. **2. ADR residual-exceptions table: consider adding a "safe because" column** The current table has "Why it stays as vi.mock" which is good. One future reader's question is: "how do we know `$app/navigation` is resolved statically?" Adding a brief note — e.g., "resolved at spec module-eval time, before any test runs" — inline in that column would make the ADR self-sufficient without needing to trace back to the context section.

marcel commented

2026-05-11 23:03:04 +02:00

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

The CI change is minimal, correct, and defensive. Here's my full read on the .gitea/workflows/ci.yml diff:

What works well

shell: bash on both new steps — explicit shell avoids runner-default ambiguity. The existing coverage step also got shell: bash, which is the right call since the step now uses tee and output redirection that depend on bash behavior.
if: always() on the guard step — correct. Without always(), a failed coverage run would skip the grep and hide the race condition in the noise of the failure.
2>/dev/null on the grep — suppresses the "file not found" error if the log doesn't exist for any reason (e.g., coverage step hard-crashed before creating the file). Clean defensive pattern.
tee instead of pipe-only — coverage output still goes to the job log for human reading AND to the file for the grep step. Good.

Suggestions (not blockers)

1. Log file path collision on retried steps

/tmp/coverage-test.log is not scoped to the run. If the job step is retried within the same runner, the second run appends to the first log and the guard could match a stale line. Fix:

run: npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}.log

Update the grep step path to match. Low-probability scenario but a one-line fix.

2. The guard step echoes matching lines — good

The grep "rpc is closed" /tmp/coverage-test.log after the echo "FAIL:" line ensures the actual offending output appears in CI logs. This is correct — operators can see exactly which test triggered it without downloading the artifact.

3. actions/upload-artifact@v4 already in use — I checked the existing step and it's already on @v4. No action version debt introduced here.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** The CI change is minimal, correct, and defensive. Here's my full read on the `.gitea/workflows/ci.yml` diff: ### What works well - **`shell: bash` on both new steps** — explicit shell avoids runner-default ambiguity. The existing coverage step also got `shell: bash`, which is the right call since the step now uses `tee` and output redirection that depend on bash behavior. - **`if: always()` on the guard step** — correct. Without `always()`, a failed coverage run would skip the grep and hide the race condition in the noise of the failure. - **`2>/dev/null` on the grep** — suppresses the "file not found" error if the log doesn't exist for any reason (e.g., coverage step hard-crashed before creating the file). Clean defensive pattern. - **`tee` instead of pipe-only** — coverage output still goes to the job log for human reading AND to the file for the grep step. Good. ### Suggestions (not blockers) **1. Log file path collision on retried steps** `/tmp/coverage-test.log` is not scoped to the run. If the job step is retried within the same runner, the second run appends to the first log and the guard could match a stale line. Fix: ```yaml run: npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}.log ``` Update the grep step path to match. Low-probability scenario but a one-line fix. **2. The guard step echoes matching lines — good** The `grep "rpc is closed" /tmp/coverage-test.log` after the `echo "FAIL:"` line ensures the actual offending output appears in CI logs. This is correct — operators can see exactly which test triggered it without downloading the artifact. **3. `actions/upload-artifact@v4` already in use** — I checked the existing step and it's already on `@v4`. No action version debt introduced here.

marcel commented

2026-05-11 23:03:13 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing from a requirements and acceptance-criteria standpoint.

Issue traceability

The PR body references issue #535 clearly. The root cause is documented precisely (dynamic import → birpc route handler → teardown race). The fix is traceable to the stated problem.

Acceptance criterion quality

The test plan in the PR body is well-formed:

✅ usePdfRenderer.svelte.test.ts — verifiable, specific, automated
✅ PdfViewer.svelte.spec.ts — verifiable, specific, automated
✅ CI guard step present — verifiable in the diff
☐ 60 consecutive green workflow_dispatch CI runs — measurable, explicit threshold, post-merge gate

The unchecked criterion is appropriately left open — it can only be evaluated after merge. However, it is currently untracked in the Gitea issue tracker. Without a linked issue or milestone, it risks being forgotten once this PR is closed and attention moves on.

Recommendation: Create a follow-up Gitea issue of the form:

"Verify fix(#535): confirm 60 consecutive green CI runs post-merge"
Acceptance: workflow_dispatch logs for 60 runs contain zero rpc is closed lines.

This gives the criterion a home, an owner, and a definition of done that closes cleanly.

NFR coverage

Flakiness (reliability NFR): The CI guard directly addresses this — it converts a silent non-deterministic failure into a detectable, blocking signal. ✅
Test performance: The tee addition adds negligible overhead. ✅
Maintainability: ADR 012 documents the rule with residual exceptions, reducing future ambiguity for anyone writing browser-mode specs. ✅

No missing NFRs for this scope.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing from a requirements and acceptance-criteria standpoint. ### Issue traceability The PR body references issue #535 clearly. The root cause is documented precisely (dynamic import → birpc route handler → teardown race). The fix is traceable to the stated problem. ### Acceptance criterion quality The test plan in the PR body is well-formed: - ✅ `usePdfRenderer.svelte.test.ts` — verifiable, specific, automated - ✅ `PdfViewer.svelte.spec.ts` — verifiable, specific, automated - ✅ CI guard step present — verifiable in the diff - ☐ **60 consecutive green `workflow_dispatch` CI runs** — measurable, explicit threshold, post-merge gate The unchecked criterion is appropriately left open — it can only be evaluated after merge. However, it is currently untracked in the Gitea issue tracker. Without a linked issue or milestone, it risks being forgotten once this PR is closed and attention moves on. **Recommendation**: Create a follow-up Gitea issue of the form: > "Verify fix(#535): confirm 60 consecutive green CI runs post-merge" > Acceptance: `workflow_dispatch` logs for 60 runs contain zero `rpc is closed` lines. This gives the criterion a home, an owner, and a definition of done that closes cleanly. ### NFR coverage - **Flakiness (reliability NFR)**: The CI guard directly addresses this — it converts a silent non-deterministic failure into a detectable, blocking signal. ✅ - **Test performance**: The `tee` addition adds negligible overhead. ✅ - **Maintainability**: ADR 012 documents the rule with residual exceptions, reducing future ambiguity for anyone writing browser-mode specs. ✅ No missing NFRs for this scope.

marcel commented

2026-05-11 23:03:44 +02:00

🏗️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

What I checked

ADR quality and placement
Architectural soundness of the libLoader DI pattern
Module boundary compliance
Required documentation updates

Findings

ADR 012 is well-written. Context explains the birpc route handler mechanism, the decision names the pattern clearly, residual exceptions are documented with justification, and consequences include the CI guard and acceptance criterion. This is the level of ADR quality we want. No changes needed.

The libLoader injection pattern is the right architectural move. Pushing the dynamic-import concern into an injectable parameter is a clean DI seam. The production default is co-located in the same file, so nothing leaks into call sites. This is textbook constructor-injection applied to functional hooks.

untrack() in PdfViewer.svelte is correct but opaque.

const renderer = untrack(() => createPdfRenderer(libLoader));

untrack prevents Svelte's reactivity from tracking libLoader as a dependency — correct, because re-creating the entire renderer when the prop reference changes would be wrong. However, a reader unfamiliar with this pattern will not understand why untrack is needed here. A one-line comment explaining the invariant would prevent someone removing it in a future refactor.

Minor: LibLoader type is not exported.

// usePdfRenderer.svelte.ts — not exported
type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;

Both PdfViewer.svelte and the test files use Parameters<typeof createPdfRenderer>[0] to recover the type. This is unnecessarily indirect. The module's public API should export LibLoader directly. Parameters<typeof fn>[0] as a type workaround is an abstraction leak — it binds the type to a function signature rather than a stable declaration.

Documentation update table check:

Change	Required update	Status
New architectural pattern for testing	`docs/adr/`	✅ ADR 012 added
No new routes, services, packages	n/a	—
No Flyway migration	n/a	—
No new domain concepts	`docs/GLOSSARY.md`	⚠️ "libLoader pattern" or "prop injection for dynamic imports" could be a glossary entry, but this is optional given the ADR covers it

The glossary omission is a suggestion, not a blocker.

Blockers

None.

Suggestions

Export LibLoader type explicitly so call sites don't need Parameters<typeof createPdfRenderer>[0].
Add a one-liner comment on the untrack call explaining the invariant ("renderer init must not re-run if libLoader prop reference changes").

## 🏗️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** ### What I checked - ADR quality and placement - Architectural soundness of the `libLoader` DI pattern - Module boundary compliance - Required documentation updates --- ### Findings **ADR 012 is well-written.** Context explains the birpc route handler mechanism, the decision names the pattern clearly, residual exceptions are documented with justification, and consequences include the CI guard and acceptance criterion. This is the level of ADR quality we want. No changes needed. **The `libLoader` injection pattern is the right architectural move.** Pushing the dynamic-import concern into an injectable parameter is a clean DI seam. The production default is co-located in the same file, so nothing leaks into call sites. This is textbook constructor-injection applied to functional hooks. **`untrack()` in `PdfViewer.svelte` is correct but opaque.** ```svelte const renderer = untrack(() => createPdfRenderer(libLoader)); ``` `untrack` prevents Svelte's reactivity from tracking `libLoader` as a dependency — correct, because re-creating the entire renderer when the prop reference changes would be wrong. However, a reader unfamiliar with this pattern will not understand _why_ untrack is needed here. A one-line comment explaining the invariant would prevent someone removing it in a future refactor. **Minor: `LibLoader` type is not exported.** ```typescript // usePdfRenderer.svelte.ts — not exported type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>; ``` Both `PdfViewer.svelte` and the test files use `Parameters<typeof createPdfRenderer>[0]` to recover the type. This is unnecessarily indirect. The module's public API should export `LibLoader` directly. `Parameters<typeof fn>[0]` as a type workaround is an abstraction leak — it binds the type to a function signature rather than a stable declaration. **Documentation update table check:** | Change | Required update | Status | |---|---|---| | New architectural pattern for testing | `docs/adr/` | ✅ ADR 012 added | | No new routes, services, packages | n/a | — | | No Flyway migration | n/a | — | | No new domain concepts | `docs/GLOSSARY.md` | ⚠️ "libLoader pattern" or "prop injection for dynamic imports" could be a glossary entry, but this is optional given the ADR covers it | The glossary omission is a suggestion, not a blocker. --- ### Blockers None. ### Suggestions 1. Export `LibLoader` type explicitly so call sites don't need `Parameters<typeof createPdfRenderer>[0]`. 2. Add a one-liner comment on the `untrack` call explaining the invariant ("renderer init must not re-run if libLoader prop reference changes").

marcel commented

2026-05-11 23:03:59 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

What I checked

TDD evidence
Naming conventions
Function size and responsibility
Svelte 5 patterns (untrack, $props())
Clean code in test helpers

Findings

TDD evidence is present. usePdfRenderer.svelte.test.ts gains a new test that verifies libLoader is called and pdfjsReady flips to true. The test exists and is meaningful. The PdfViewer.svelte.spec.ts changes are refactors of existing tests, not new behaviors — no new test-before-implementation is expected there.

makeFakePdfjsLib() and makeFakeLibLoader() are clean factory functions. Descriptive names, single responsibility, readable return types. Good pattern.

LibLoader type is private but should be public (usePdfRenderer.svelte.ts:1).

type LibLoader = ...;          // not exported

Both PdfViewer.svelte and the spec use Parameters<typeof createPdfRenderer>[0] to recover this type. That's typeof inference as a substitute for a real export — it works but reads as a workaround. Exporting the type is one line:

export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;

Then callers can just import type { LibLoader }.

untrack usage needs a comment (PdfViewer.svelte:40).

const renderer = untrack(() => createPdfRenderer(libLoader));

untrack here prevents Svelte from re-running createPdfRenderer if the libLoader prop reference changes. That's correct — re-initialising the whole PDF renderer on a prop reference change would be wrong. But a future developer might see this as dead code or remove it during a cleanup. One comment:

// untrack: libLoader prop change must not reinitialise the renderer
const renderer = untrack(() => createPdfRenderer(libLoader));

Missing error-path test in usePdfRenderer.svelte.test.ts.
The new test verifies the happy path: loader resolves, pdfjsReady becomes true. But init() has no try/catch:

async function init(): Promise<void> {
    const [lib, { default: workerUrl }] = await libLoader();
    // ...
    pdfjsReady = true;
}

What happens when libLoader() rejects? pdfjsReady stays false, the error propagates to onMount, and Svelte 5's onMount silently swallows it — the component renders in an indefinite loading state with no user feedback. A test for the rejection case (and whether an error state is surfaced) would surface this gap. This is a suggestion — the production defaultLibLoader cannot currently fail in a way that the real onMount doesn't already handle, but the test coverage gap is real.

makeFakePdfjsLib is duplicated across two test files.
A partial copy of the fake exists in both PdfViewer.svelte.spec.ts and usePdfRenderer.svelte.test.ts. They serve different test levels so co-location is fine, but a shared __testUtils__/fakePdfjs.ts would reduce drift if the fake needs updating. This is a long-term suggestion, not a blocker for this PR.

Blockers

None.

Suggestions

Export LibLoader type so callers don't need Parameters<typeof createPdfRenderer>[0].
Add a comment on the untrack call explaining the invariant.
Add a test for libLoader rejection in usePdfRenderer.svelte.test.ts to surface the missing error state.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### What I checked - TDD evidence - Naming conventions - Function size and responsibility - Svelte 5 patterns (`untrack`, `$props()`) - Clean code in test helpers --- ### Findings **TDD evidence is present.** `usePdfRenderer.svelte.test.ts` gains a new test that verifies `libLoader` is called and `pdfjsReady` flips to `true`. The test exists and is meaningful. The `PdfViewer.svelte.spec.ts` changes are refactors of existing tests, not new behaviors — no new test-before-implementation is expected there. **`makeFakePdfjsLib()` and `makeFakeLibLoader()` are clean factory functions.** Descriptive names, single responsibility, readable return types. Good pattern. **`LibLoader` type is private but should be public (`usePdfRenderer.svelte.ts:1`).** ```typescript type LibLoader = ...; // not exported ``` Both `PdfViewer.svelte` and the spec use `Parameters<typeof createPdfRenderer>[0]` to recover this type. That's `typeof` inference as a substitute for a real export — it works but reads as a workaround. Exporting the type is one line: ```typescript export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>; ``` Then callers can just `import type { LibLoader }`. **`untrack` usage needs a comment (`PdfViewer.svelte:40`).** ```svelte const renderer = untrack(() => createPdfRenderer(libLoader)); ``` `untrack` here prevents Svelte from re-running `createPdfRenderer` if the `libLoader` prop reference changes. That's correct — re-initialising the whole PDF renderer on a prop reference change would be wrong. But a future developer might see this as dead code or remove it during a cleanup. One comment: ```svelte // untrack: libLoader prop change must not reinitialise the renderer const renderer = untrack(() => createPdfRenderer(libLoader)); ``` **Missing error-path test in `usePdfRenderer.svelte.test.ts`.** The new test verifies the happy path: loader resolves, `pdfjsReady` becomes `true`. But `init()` has no try/catch: ```typescript async function init(): Promise<void> { const [lib, { default: workerUrl }] = await libLoader(); // ... pdfjsReady = true; } ``` What happens when `libLoader()` rejects? `pdfjsReady` stays `false`, the error propagates to `onMount`, and Svelte 5's `onMount` silently swallows it — the component renders in an indefinite loading state with no user feedback. A test for the rejection case (and whether an error state is surfaced) would surface this gap. This is a suggestion — the production `defaultLibLoader` cannot currently fail in a way that the real `onMount` doesn't already handle, but the test coverage gap is real. **`makeFakePdfjsLib` is duplicated across two test files.** A partial copy of the fake exists in both `PdfViewer.svelte.spec.ts` and `usePdfRenderer.svelte.test.ts`. They serve different test levels so co-location is fine, but a shared `__testUtils__/fakePdfjs.ts` would reduce drift if the fake needs updating. This is a long-term suggestion, not a blocker for this PR. --- ### Blockers None. ### Suggestions 1. Export `LibLoader` type so callers don't need `Parameters<typeof createPdfRenderer>[0]`. 2. Add a comment on the `untrack` call explaining the invariant. 3. Add a test for `libLoader` rejection in `usePdfRenderer.svelte.test.ts` to surface the missing error state.

marcel commented

2026-05-11 23:04:14 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ⚠️ Approved with concerns

What I checked

CI step correctness (2>&1 | tee, if: always(), shell options)
Guard step logic and edge cases
Log file path and scope
Grep pattern precision

Findings

The if: always() guard placement is correct. The guard step runs even when the coverage step fails, which is the right behaviour — you want to detect the birpc race string regardless of whether the test suite itself exits non-zero.

/tmp/coverage-test.log scope is correct. All steps within a single job share the same environment and /tmp directory in act_runner. The file is written in the coverage step and read in the guard step — same job, no cross-job concern.

2>/dev/null in the guard handles missing log gracefully.

if grep -q "rpc is closed" /tmp/coverage-test.log 2>/dev/null; then

If the coverage step failed before producing any output (e.g., node/npm crash), the log file might not exist. 2>/dev/null suppresses the grep error so the guard exits 0 (no false positive). Good edge case handling.

⚠️ Blocker: pipefail must be verified for the pipe step.
The coverage step now uses a pipe:

shell: bash
run: npm run test:coverage 2>&1 | tee /tmp/coverage-test.log

Without set -o pipefail, bash reports the exit code of tee (which is always 0), not npm run test:coverage. A failing test suite would then silently pass this step. GitHub Actions adds -eo pipefail by default for bash shells; Gitea's act_runner should do the same, but this is worth explicitly verifying.

The safest fix is to make it unambiguous:

shell: bash
run: |
  set -eo pipefail
  npm run test:coverage 2>&1 | tee /tmp/coverage-test.log

This makes the intent explicit regardless of runner defaults and is immune to act_runner version differences.

Minor: grep pattern is broad.

grep -q "rpc is closed"

The actual error string is [birpc] rpc is closed, cannot call "resolveManualMock". The pattern rpc is closed would also match any hypothetical log line containing that substring from an unrelated source (e.g., a future database connection error log that mentions an RPC). A more precise pattern reduces false positives:

grep -q "\[birpc\] rpc is closed"

What's done well:

Artifact upload step retains if: always() — coverage artifacts still uploaded on failure.
Guard step produces a human-readable failure message with the matching lines echoed.
Pattern is in the coverage job only, not propagated to unrelated jobs.

Blockers

Verify pipefail behaviour — either add explicit set -eo pipefail to the pipe step or confirm act_runner version adds it. If pipefail is not set, a failing test run silently passes.

Suggestions

Tighten grep pattern to \[birpc\] rpc is closed to reduce false positive risk.

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked - CI step correctness (`2>&1 | tee`, `if: always()`, shell options) - Guard step logic and edge cases - Log file path and scope - Grep pattern precision --- ### Findings **The `if: always()` guard placement is correct.** The guard step runs even when the coverage step fails, which is the right behaviour — you want to detect the birpc race string regardless of whether the test suite itself exits non-zero. **`/tmp/coverage-test.log` scope is correct.** All steps within a single job share the same environment and `/tmp` directory in act_runner. The file is written in the coverage step and read in the guard step — same job, no cross-job concern. **`2>/dev/null` in the guard handles missing log gracefully.** ```bash if grep -q "rpc is closed" /tmp/coverage-test.log 2>/dev/null; then ``` If the coverage step failed before producing any output (e.g., node/npm crash), the log file might not exist. `2>/dev/null` suppresses the grep error so the guard exits 0 (no false positive). Good edge case handling. **⚠️ Blocker: `pipefail` must be verified for the pipe step.** The coverage step now uses a pipe: ```yaml shell: bash run: npm run test:coverage 2>&1 | tee /tmp/coverage-test.log ``` Without `set -o pipefail`, bash reports the exit code of `tee` (which is always 0), not `npm run test:coverage`. A failing test suite would then silently pass this step. GitHub Actions adds `-eo pipefail` by default for bash shells; Gitea's act_runner _should_ do the same, but this is worth explicitly verifying. The safest fix is to make it unambiguous: ```yaml shell: bash run: | set -eo pipefail npm run test:coverage 2>&1 | tee /tmp/coverage-test.log ``` This makes the intent explicit regardless of runner defaults and is immune to act_runner version differences. **Minor: grep pattern is broad.** ```bash grep -q "rpc is closed" ``` The actual error string is `[birpc] rpc is closed, cannot call "resolveManualMock"`. The pattern `rpc is closed` would also match any hypothetical log line containing that substring from an unrelated source (e.g., a future database connection error log that mentions an RPC). A more precise pattern reduces false positives: ```bash grep -q "\[birpc\] rpc is closed" ``` **What's done well:** - Artifact upload step retains `if: always()` — coverage artifacts still uploaded on failure. - Guard step produces a human-readable failure message with the matching lines echoed. - Pattern is in the coverage job only, not propagated to unrelated jobs. --- ### Blockers 1. **Verify `pipefail` behaviour** — either add explicit `set -eo pipefail` to the pipe step or confirm act_runner version adds it. If pipefail is not set, a failing test run silently passes. ### Suggestions 1. Tighten grep pattern to `\[birpc\] rpc is closed` to reduce false positive risk.

marcel commented

2026-05-11 23:04:28 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

What I checked

Requirements traceability (PR → issue → acceptance criteria)
Completeness of the test plan
Measurability of acceptance criteria
ADR decision documentation quality

Findings

Requirements traceability is solid. The PR body links to issue #535 and the root cause section maps directly to the reported symptom. The connection between cause (birpc route handler race), mechanism (ManualMockedModule + dynamic import timing), and fix (prop injection eliminates the ManualMockedModule registration) is clearly documented.

The test plan is explicit and honest.

items are verifiable and already completed.
The [ ] item ("60 consecutive green workflow_dispatch CI runs") correctly captures the probabilistic acceptance criterion for a flaky-test fix — this is the right way to frame a non-deterministic regression.
The PR does not falsely claim the fix is proven; it documents the post-merge verification step.

Acceptance criterion is measurable. "60 consecutive green CI runs with zero rpc is closed lines" is unambiguous. This is better than "the tests should pass more reliably."

ADR 012 documents the decision with appropriate detail. Context names the exact failure mechanism. Decision names the pattern. Residual exceptions table documents why certain vi.mock calls are safe. Consequences acknowledge the CI guard as the regression backstop. This is the right level of documentation for a non-obvious testing constraint.

One gap: the ADR acceptance criterion and the PR test plan acceptance criterion are not co-located.

PR test plan: "60 consecutive green workflow_dispatch CI runs against main after merge"
ADR consequences: "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."

These are consistent, but the ADR's wording adds the precision "with zero rpc is closed lines in any log." The PR test plan checklist should ideally carry the same wording for completeness. Minor.

Non-goal not documented. The fix does not address the root cause of why pdfjs's dynamic import happens after worker teardown — it side-steps it. This is explicitly acceptable, but stating it as a non-goal in the ADR consequences would prevent a future developer from wondering if the underlying vitest issue was resolved. A one-line addition:

- This fix does not patch the vitest/birpc teardown timing; it eliminates the vi.mock registration
  that triggered the race. The underlying vitest issue remains open upstream.

Blockers

None.

Suggestions

Align PR test plan wording with ADR: add "with zero rpc is closed lines in any log" to the checklist item.
Optionally add a non-goal note to ADR 012 consequences section clarifying the upstream vitest issue is not fixed.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### What I checked - Requirements traceability (PR → issue → acceptance criteria) - Completeness of the test plan - Measurability of acceptance criteria - ADR decision documentation quality --- ### Findings **Requirements traceability is solid.** The PR body links to issue #535 and the root cause section maps directly to the reported symptom. The connection between cause (birpc route handler race), mechanism (ManualMockedModule + dynamic import timing), and fix (prop injection eliminates the ManualMockedModule registration) is clearly documented. **The test plan is explicit and honest.** - [x] items are verifiable and already completed. - The [ ] item ("60 consecutive green `workflow_dispatch` CI runs") correctly captures the probabilistic acceptance criterion for a flaky-test fix — this is the right way to frame a non-deterministic regression. - The PR does not falsely claim the fix is proven; it documents the post-merge verification step. **Acceptance criterion is measurable.** "60 consecutive green CI runs with zero `rpc is closed` lines" is unambiguous. This is better than "the tests should pass more reliably." **ADR 012 documents the decision with appropriate detail.** Context names the exact failure mechanism. Decision names the pattern. Residual exceptions table documents why certain `vi.mock` calls are safe. Consequences acknowledge the CI guard as the regression backstop. This is the right level of documentation for a non-obvious testing constraint. **One gap: the ADR acceptance criterion and the PR test plan acceptance criterion are not co-located.** - PR test plan: "60 consecutive green `workflow_dispatch` CI runs against `main` after merge" - ADR consequences: "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." These are consistent, but the ADR's wording adds the precision "with zero `rpc is closed` lines in any log." The PR test plan checklist should ideally carry the same wording for completeness. Minor. **Non-goal not documented.** The fix does not address the root cause of why pdfjs's dynamic import happens after worker teardown — it side-steps it. This is explicitly acceptable, but stating it as a non-goal in the ADR consequences would prevent a future developer from wondering if the underlying vitest issue was resolved. A one-line addition: ``` - This fix does not patch the vitest/birpc teardown timing; it eliminates the vi.mock registration that triggered the race. The underlying vitest issue remains open upstream. ``` --- ### Blockers None. ### Suggestions 1. Align PR test plan wording with ADR: add "with zero `rpc is closed` lines in any log" to the checklist item. 2. Optionally add a non-goal note to ADR 012 consequences section clarifying the upstream vitest issue is not fixed.

marcel commented

2026-05-11 23:04:36 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What I checked

Whether the libLoader injection creates any exploitable path in production
CI workflow changes for secret exposure or injection risk
ADR for any security-relevant decisions

Findings

No security-sensitive changes in this PR. The diff touches test infrastructure only. There are no auth, permission, API, or data handling changes.

libLoader default is safe. The production code path is unchanged:

const defaultLibLoader: LibLoader = () =>
    Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]);

The injected libLoader prop only matters in test contexts. In production, no caller passes libLoader, so defaultLibLoader is always used. No attack surface is added.

CI guard cannot be exploited to hide failures. The grep check can only introduce false positives (failing a green build if rpc is closed appears for an unrelated reason), never false negatives (passing a build that has the race). From a security perspective, the conservative failure direction is correct.

No secrets introduced in workflow YAML. The CI changes add no new environment variables, tokens, or credentials. Existing secret handling is unchanged.

ADR 012 has no security implications. The mocking strategy decision is purely test infrastructure.

Blockers

None.

Suggestions

None.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### What I checked - Whether the `libLoader` injection creates any exploitable path in production - CI workflow changes for secret exposure or injection risk - ADR for any security-relevant decisions --- ### Findings **No security-sensitive changes in this PR.** The diff touches test infrastructure only. There are no auth, permission, API, or data handling changes. **`libLoader` default is safe.** The production code path is unchanged: ```typescript const defaultLibLoader: LibLoader = () => Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]); ``` The injected `libLoader` prop only matters in test contexts. In production, no caller passes `libLoader`, so `defaultLibLoader` is always used. No attack surface is added. **CI guard cannot be exploited to hide failures.** The grep check can only introduce false positives (failing a green build if `rpc is closed` appears for an unrelated reason), never false negatives (passing a build that has the race). From a security perspective, the conservative failure direction is correct. **No secrets introduced in workflow YAML.** The CI changes add no new environment variables, tokens, or credentials. Existing secret handling is unchanged. **ADR 012 has no security implications.** The mocking strategy decision is purely test infrastructure. --- ### Blockers None. ### Suggestions None.

marcel commented

2026-05-11 23:04:55 +02:00

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

What I checked

Test names and readability
Coverage of happy path vs. error paths
Correct layer placement (unit vs. component vs. E2E)
CI guard correctness and flaky-test regression strategy
Factory function quality

Findings

Tests are at the right layer. usePdfRenderer.svelte.test.ts (Node-mode, unit-level) tests the DI seam in isolation. PdfViewer.svelte.spec.ts (browser-mode, component-level) tests the rendered UI. The two test files complement each other without overlapping. Correct pyramid placement.

Test names in PdfViewer.svelte.spec.ts are descriptive and pass the sentence test.

shows previous and next page navigation buttons ✅
shows zoom controls ✅
displays the page counter once the PDF has loaded ✅

New test in usePdfRenderer.svelte.test.ts is readable and verifies the DI contract.

it('calls injected libLoader during init and sets pdfjsReady', async () => {

The name documents both assertions. The test body is clean.

makeFakePdfjsLib() and makeFakeLibLoader() are correct factory functions. Single-use per test (called inline per test case), no shared mutable state. Good.

⚠️ Missing error-path test for init() failure.
init() in usePdfRenderer.svelte.ts has no try/catch:

async function init(): Promise<void> {
    const [lib, { default: workerUrl }] = await libLoader();
    pdfjsLib = lib;
    pdfjsReady = true;
}

There is no test for what happens when libLoader() rejects. In Svelte 5, an unhandled rejection in onMount is swallowed silently — the component enters a perpetual loading state with no error feedback to the user. A test exposing this behaviour would either:

Confirm the component shows an error state (which it currently doesn't), or
Document the known limitation

Suggested test:

it('leaves pdfjsReady false when libLoader rejects', async () => {
    const failingLoader = vi.fn().mockRejectedValue(new Error('load failed'));
    const r = createPdfRenderer(failingLoader);
    await r.init().catch(() => {});
    expect(r.pdfjsReady).toBe(false);
});

This is a suggestion, not a blocker — the PR is fixing an existing flaky test, not building new error handling. But the gap should be tracked.

⚠️ No test verifying that makeFakeLibLoader() is actually called in component tests.
The PdfViewer.svelte.spec.ts tests verify UI states (buttons visible, counter shows). They do not assert that libLoader was invoked. This means if someone accidentally broke the DI wiring (createPdfRenderer() called without libLoader argument), the component tests might still pass (depending on whether the real loader is reached). The usePdfRenderer.svelte.test.ts test does verify the call, so coverage is present at the unit level — this is an acceptable split. No action needed, just noting the boundary.

CI guard strategy is sound. if: always() ensures the check runs even on test suite failure. 2>/dev/null handles missing log file gracefully. The permanent nature of the grep guard matches the permanent nature of the regression risk — a future vitest upgrade could re-introduce the race, and this guard would catch it immediately. This is exactly what quality gates should look like.

The 60-run acceptance criterion is unusual but well-documented. The ADR explains why 60 runs is the threshold (probabilistic, not functional). This is the correct approach for a non-deterministic regression fix.

Blockers

None.

Suggestions

Add a test for libLoader rejection in usePdfRenderer.svelte.test.ts to document the missing error-state gap and prevent silent loading states from regressing unnoticed.
Track the missing error state (no UI feedback when PDF library fails to load) as a follow-up issue if it doesn't already exist.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked - Test names and readability - Coverage of happy path vs. error paths - Correct layer placement (unit vs. component vs. E2E) - CI guard correctness and flaky-test regression strategy - Factory function quality --- ### Findings **Tests are at the right layer.** `usePdfRenderer.svelte.test.ts` (Node-mode, unit-level) tests the DI seam in isolation. `PdfViewer.svelte.spec.ts` (browser-mode, component-level) tests the rendered UI. The two test files complement each other without overlapping. Correct pyramid placement. **Test names in `PdfViewer.svelte.spec.ts` are descriptive and pass the sentence test.** - `shows previous and next page navigation buttons` ✅ - `shows zoom controls` ✅ - `displays the page counter once the PDF has loaded` ✅ **New test in `usePdfRenderer.svelte.test.ts` is readable and verifies the DI contract.** ```typescript it('calls injected libLoader during init and sets pdfjsReady', async () => { ``` The name documents both assertions. The test body is clean. **`makeFakePdfjsLib()` and `makeFakeLibLoader()` are correct factory functions.** Single-use per test (called inline per test case), no shared mutable state. Good. **⚠️ Missing error-path test for `init()` failure.** `init()` in `usePdfRenderer.svelte.ts` has no try/catch: ```typescript async function init(): Promise<void> { const [lib, { default: workerUrl }] = await libLoader(); pdfjsLib = lib; pdfjsReady = true; } ``` There is no test for what happens when `libLoader()` rejects. In Svelte 5, an unhandled rejection in `onMount` is swallowed silently — the component enters a perpetual loading state with no error feedback to the user. A test exposing this behaviour would either: - Confirm the component shows an error state (which it currently doesn't), or - Document the known limitation Suggested test: ```typescript it('leaves pdfjsReady false when libLoader rejects', async () => { const failingLoader = vi.fn().mockRejectedValue(new Error('load failed')); const r = createPdfRenderer(failingLoader); await r.init().catch(() => {}); expect(r.pdfjsReady).toBe(false); }); ``` This is a suggestion, not a blocker — the PR is fixing an existing flaky test, not building new error handling. But the gap should be tracked. **⚠️ No test verifying that `makeFakeLibLoader()` is actually called in component tests.** The `PdfViewer.svelte.spec.ts` tests verify UI states (buttons visible, counter shows). They do not assert that `libLoader` was invoked. This means if someone accidentally broke the DI wiring (`createPdfRenderer()` called without `libLoader` argument), the component tests might still pass (depending on whether the real loader is reached). The `usePdfRenderer.svelte.test.ts` test does verify the call, so coverage is present at the unit level — this is an acceptable split. No action needed, just noting the boundary. **CI guard strategy is sound.** `if: always()` ensures the check runs even on test suite failure. `2>/dev/null` handles missing log file gracefully. The permanent nature of the grep guard matches the permanent nature of the regression risk — a future vitest upgrade could re-introduce the race, and this guard would catch it immediately. This is exactly what quality gates should look like. **The 60-run acceptance criterion is unusual but well-documented.** The ADR explains why 60 runs is the threshold (probabilistic, not functional). This is the correct approach for a non-deterministic regression fix. --- ### Blockers None. ### Suggestions 1. Add a test for `libLoader` rejection in `usePdfRenderer.svelte.test.ts` to document the missing error-state gap and prevent silent loading states from regressing unnoticed. 2. Track the missing error state (no UI feedback when PDF library fails to load) as a follow-up issue if it doesn't already exist.

marcel commented

2026-05-11 23:05:01 +02:00

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate

Verdict: ✅ Approved

What I checked

Any visual changes to PdfViewer.svelte
Accessibility implications of the prop addition
Brand and responsive design impact

Findings

No user-visible changes in this PR. The libLoader prop addition is a testing seam — it has no rendered output, no visual state, and no interaction pattern. The untrack(() => createPdfRenderer(libLoader)) change only affects how the renderer is initialised internally.

The rendered output of PdfViewer.svelte is unchanged. Zoom controls, navigation buttons, page counter, annotation layer — all exactly as before. I checked the diff: only the $props() destructuring and renderer initialisation line changed. No template markup was touched.

Accessibility surface is unchanged. No ARIA attributes, no touch targets, no focus management, no semantic structure was modified.

Brand compliance is unchanged. No Tailwind class additions or removals.

Blockers

None.

Suggestions

None. This is a clean infrastructure change with zero UX impact.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate **Verdict: ✅ Approved** ### What I checked - Any visual changes to `PdfViewer.svelte` - Accessibility implications of the prop addition - Brand and responsive design impact --- ### Findings **No user-visible changes in this PR.** The `libLoader` prop addition is a testing seam — it has no rendered output, no visual state, and no interaction pattern. The `untrack(() => createPdfRenderer(libLoader))` change only affects how the renderer is initialised internally. **The rendered output of `PdfViewer.svelte` is unchanged.** Zoom controls, navigation buttons, page counter, annotation layer — all exactly as before. I checked the diff: only the `$props()` destructuring and renderer initialisation line changed. No template markup was touched. **Accessibility surface is unchanged.** No ARIA attributes, no touch targets, no focus management, no semantic structure was modified. **Brand compliance is unchanged.** No Tailwind class additions or removals. --- ### Blockers None. ### Suggestions None. This is a clean infrastructure change with zero UX impact.

marcel commented

2026-05-11 23:21:33 +02:00

Round 2 review feedback addressed

Four commits pushed to this branch:

`e529f9f7` — refactor(pdf-viewer): export LibLoader type and update callers

Felix (8968, 8977) + Markus (8972, 8976): LibLoader is now exported from usePdfRenderer.svelte.ts. PdfViewer.svelte imports it directly (import { createPdfRenderer, type LibLoader }), and PdfViewer.svelte.spec.ts uses it as the return type of makeFakeLibLoader(). Parameters<typeof createPdfRenderer>[0] is gone from all three files.

`b8d9c0e9` — docs(pdf-viewer): comment untrack invariant on renderer init

Felix (8977) + Markus (8976): One-liner added above the untrack call:

// untrack: libLoader prop change must not reinitialise the renderer
const renderer = untrack(() => createPdfRenderer(libLoader));

`d8496498` — test(pdf-renderer): document libLoader rejection leaves pdfjsReady false

Felix (8968, 8977) + Sara (8970, 8982): New test added to usePdfRenderer.svelte.test.ts:

it('leaves pdfjsReady false when libLoader rejects', async () => {
    const failingLoader = vi.fn().mockRejectedValue(new Error('load failed'));
    const r = createPdfRenderer(failingLoader);
    await r.init().catch(() => {});
    expect(r.pdfjsReady).toBe(false);
});

Note: TDD red phase was not possible here — init() already propagates the rejection before pdfjsReady = true, so the assertion was immediately green. This test is a regression-protection guard, not a fix.

`e418e884` — ci(coverage): harden coverage guard step

Tobias (8978) blocker: Added explicit set -eo pipefail to the coverage pipe step. Under bash, pipefail ensures npm's exit code propagates through the pipe — not just tee's always-0 exit.
Tobias (8973, 8978) + Sara (8970): Log file scoped to ${{ github.run_id }} (/tmp/coverage-test-${{ github.run_id }}.log) to prevent stale-log false positives on retried steps.
Tobias (8978): Grep pattern tightened to \[birpc\] rpc is closed to reduce false-positive risk from unrelated log lines containing "rpc is closed".

All 1851 tests pass (1 pre-existing flaky timeout in confirm.svelte.test.ts — tracked as #538).

Deferred (out of scope):

Unhappy-path UI for libLoader rejection (no error state surfaced to the user when PDF lib fails to load) — noted by Sara (8982), to be tracked separately if desired.

## Round 2 review feedback addressed Four commits pushed to this branch: ### `e529f9f7` — refactor(pdf-viewer): export LibLoader type and update callers - **Felix (8968, 8977) + Markus (8972, 8976):** `LibLoader` is now exported from `usePdfRenderer.svelte.ts`. `PdfViewer.svelte` imports it directly (`import { createPdfRenderer, type LibLoader }`), and `PdfViewer.svelte.spec.ts` uses it as the return type of `makeFakeLibLoader()`. `Parameters<typeof createPdfRenderer>[0]` is gone from all three files. ### `b8d9c0e9` — docs(pdf-viewer): comment untrack invariant on renderer init - **Felix (8977) + Markus (8976):** One-liner added above the `untrack` call: ```svelte // untrack: libLoader prop change must not reinitialise the renderer const renderer = untrack(() => createPdfRenderer(libLoader)); ``` ### `d8496498` — test(pdf-renderer): document libLoader rejection leaves pdfjsReady false - **Felix (8968, 8977) + Sara (8970, 8982):** New test added to `usePdfRenderer.svelte.test.ts`: ```typescript it('leaves pdfjsReady false when libLoader rejects', async () => { const failingLoader = vi.fn().mockRejectedValue(new Error('load failed')); const r = createPdfRenderer(failingLoader); await r.init().catch(() => {}); expect(r.pdfjsReady).toBe(false); }); ``` Note: TDD red phase was not possible here — `init()` already propagates the rejection before `pdfjsReady = true`, so the assertion was immediately green. This test is a regression-protection guard, not a fix. ### `e418e884` — ci(coverage): harden coverage guard step - **Tobias (8978) blocker:** Added explicit `set -eo pipefail` to the coverage pipe step. Under bash, `pipefail` ensures npm's exit code propagates through the pipe — not just `tee`'s always-0 exit. - **Tobias (8973, 8978) + Sara (8970):** Log file scoped to `${{ github.run_id }}` (`/tmp/coverage-test-${{ github.run_id }}.log`) to prevent stale-log false positives on retried steps. - **Tobias (8978):** Grep pattern tightened to `\[birpc\] rpc is closed` to reduce false-positive risk from unrelated log lines containing "rpc is closed". --- **All 1851 tests pass** (1 pre-existing flaky timeout in `confirm.svelte.test.ts` — tracked as #538). **Deferred (out of scope):** - Unhappy-path UI for `libLoader` rejection (no error state surfaced to the user when PDF lib fails to load) — noted by Sara (8982), to be tracked separately if desired.

marcel commented

2026-05-12 07:27:20 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Solid fix. The libLoader DI seam is exactly the right move — narrow, testable, and it eliminates the race without adding any complexity. The untrack() comment explains the why, which is the only kind of comment worth writing.

Blockers

None.

Suggestions

PdfViewer.svelte.spec.ts:25 — "what" comment, remove it

} as unknown as typeof import('pdfjs-dist');
// Partial fake: only the methods used by usePdfRenderer are implemented

The comment explains what (it's a partial fake), which the name makeFakePdfjsLib already implies. If anything is worth saying here, it's why we don't implement the full API surface — but that's obvious from context. Drop it.

usePdfRenderer.svelte.test.ts — the rejection test makes no assertion that init() throws

it('leaves pdfjsReady false when libLoader rejects', async () => {
    const failingLoader = vi.fn().mockRejectedValue(new Error('load failed'));
    const r = createPdfRenderer(failingLoader);
    await r.init().catch(() => {});   // ← swallows everything
    expect(r.pdfjsReady).toBe(false);
});

The state assertion (pdfjsReady === false) is correct, but there's no proof the rejection actually propagated from init(). If a future refactor silently catches the loader error and returns normally (without setting pdfjsReady), this test would pass vacuously. Consider:

await expect(r.init()).rejects.toThrow('load failed');
expect(r.pdfjsReady).toBe(false);

That makes the failure mode explicit and documents the contract: init() is expected to re-throw.

Notes

const renderer = untrack(() => createPdfRenderer(libLoader)) — correct and the comment earns its place.
Factory functions makeFakePdfjsLib / makeFakeLibLoader follow the project pattern and are one-liners at call sites.
Moving afterEach(cleanup) to the top level before describe is the right call given the vi.mock cleanup.
defaultLibLoader as a module-level constant (not inline in the function signature) keeps the exported type and the default in the same visible declaration — clean.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Solid fix. The libLoader DI seam is exactly the right move — narrow, testable, and it eliminates the race without adding any complexity. The `untrack()` comment explains the *why*, which is the only kind of comment worth writing. ### Blockers None. ### Suggestions **`PdfViewer.svelte.spec.ts:25` — "what" comment, remove it** ```typescript } as unknown as typeof import('pdfjs-dist'); // Partial fake: only the methods used by usePdfRenderer are implemented ``` The comment explains *what* (it's a partial fake), which the name `makeFakePdfjsLib` already implies. If anything is worth saying here, it's *why* we don't implement the full API surface — but that's obvious from context. Drop it. **`usePdfRenderer.svelte.test.ts` — the rejection test makes no assertion that `init()` throws** ```typescript it('leaves pdfjsReady false when libLoader rejects', async () => { const failingLoader = vi.fn().mockRejectedValue(new Error('load failed')); const r = createPdfRenderer(failingLoader); await r.init().catch(() => {}); // ← swallows everything expect(r.pdfjsReady).toBe(false); }); ``` The state assertion (`pdfjsReady === false`) is correct, but there's no proof the rejection actually propagated from `init()`. If a future refactor silently catches the loader error and returns normally (without setting `pdfjsReady`), this test would pass vacuously. Consider: ```typescript await expect(r.init()).rejects.toThrow('load failed'); expect(r.pdfjsReady).toBe(false); ``` That makes the failure mode explicit and documents the contract: `init()` is expected to re-throw. ### Notes - `const renderer = untrack(() => createPdfRenderer(libLoader))` — correct and the comment earns its place. - Factory functions `makeFakePdfjsLib` / `makeFakeLibLoader` follow the project pattern and are one-liners at call sites. - Moving `afterEach(cleanup)` to the top level before `describe` is the right call given the `vi.mock` cleanup. - `defaultLibLoader` as a module-level constant (not inline in the function signature) keeps the exported type and the default in the same visible declaration — clean.

marcel commented

2026-05-12 07:27:32 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

This PR does exactly what an architectural fix should do: it identifies a structural constraint (dynamic imports can't safely use vi.mock in browser mode), documents the decision in an ADR, and enforces the rule at the source (DI seam on the loader) rather than papering over the symptom.

Blockers

None.

Architecture Observations

ADR 012 is correctly structured and belongs here.

Context, decision, residual exceptions table, and consequences — all present. The residual exceptions table for $app/* / $env/* is important; without it, a future developer might over-apply the rule and try to DI-inject SvelteKit virtual modules, which have no seam. The ADR prevents that mistake.

Module boundary: clean.

All changes stay within frontend/src/lib/document/viewer/. The LibLoader type is exported from usePdfRenderer.svelte.ts and imported where needed — no cross-domain coupling introduced.

The untrack() pattern is the right call.

createPdfRenderer(libLoader) runs once at component initialization. Wrapping it in untrack() prevents Svelte's reactivity from treating libLoader as a reactive dependency and reinitializing the renderer on prop changes. The comment documents the invariant, which is exactly where a comment earns its place.

Injection default is a module-level constant — correct.

const defaultLibLoader: LibLoader = () =>
    Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]);

export function createPdfRenderer(libLoader: LibLoader = defaultLibLoader) { ... }

This keeps the production path unchanged, the default is named, and it can be referenced independently if needed. Good.

No documentation gaps for this scope.

No new routes, no new backend packages, no schema changes, no new domain concepts. The ADR is the required doc update for this pattern — and it's present.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** This PR does exactly what an architectural fix should do: it identifies a structural constraint (dynamic imports can't safely use `vi.mock` in browser mode), documents the decision in an ADR, and enforces the rule at the source (DI seam on the loader) rather than papering over the symptom. ### Blockers None. ### Architecture Observations **ADR 012 is correctly structured and belongs here.** Context, decision, residual exceptions table, and consequences — all present. The residual exceptions table for `$app/*` / `$env/*` is important; without it, a future developer might over-apply the rule and try to DI-inject SvelteKit virtual modules, which have no seam. The ADR prevents that mistake. **Module boundary: clean.** All changes stay within `frontend/src/lib/document/viewer/`. The `LibLoader` type is exported from `usePdfRenderer.svelte.ts` and imported where needed — no cross-domain coupling introduced. **The `untrack()` pattern is the right call.** `createPdfRenderer(libLoader)` runs once at component initialization. Wrapping it in `untrack()` prevents Svelte's reactivity from treating `libLoader` as a reactive dependency and reinitializing the renderer on prop changes. The comment documents the invariant, which is exactly where a comment earns its place. **Injection default is a module-level constant — correct.** ```typescript const defaultLibLoader: LibLoader = () => Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]); export function createPdfRenderer(libLoader: LibLoader = defaultLibLoader) { ... } ``` This keeps the production path unchanged, the default is named, and it can be referenced independently if needed. Good. **No documentation gaps for this scope.** No new routes, no new backend packages, no schema changes, no new domain concepts. The ADR is the required doc update for this pattern — and it's present.

marcel commented

2026-05-12 07:27:42 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Security audit scoped to: injection surfaces, auth/permission changes, data exposure, CI secret handling, and any new external input paths.

Blockers

None.

What I Checked

LibLoader is an internal DI parameter — not user-controlled input.

libLoader flows from the test caller or the component's default. There is no path where user-supplied data reaches this parameter. No validation needed.

No auth or permission changes.

PdfViewer.svelte is a read-only rendering component. No @RequirePermission annotations are added or removed. No security configuration is touched.

No new external API calls or fetch patterns.

The production defaultLibLoader calls import('pdfjs-dist') — a static bundler import, not a network request. No SSRF surface.

CI workflow additions: no hardcoded secrets, no privileged steps.

The new CI steps use only ${{ github.run_id }} (a runner-provided variable, not a secret). grep runs against a local log file. No external network calls, no credential usage.

Log file contents are safe to grep.

The guard step checks for [birpc] rpc is closed in stdout captured from npm run test:coverage. This is internal tooling output — no user data, no credentials, no PII can appear there.

Notes

The CI guard is a good detective control for this specific race class. It will catch regressions before they reach human reviewers.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** Security audit scoped to: injection surfaces, auth/permission changes, data exposure, CI secret handling, and any new external input paths. ### Blockers None. ### What I Checked **`LibLoader` is an internal DI parameter — not user-controlled input.** `libLoader` flows from the test caller or the component's default. There is no path where user-supplied data reaches this parameter. No validation needed. **No auth or permission changes.** `PdfViewer.svelte` is a read-only rendering component. No `@RequirePermission` annotations are added or removed. No security configuration is touched. **No new external API calls or fetch patterns.** The production `defaultLibLoader` calls `import('pdfjs-dist')` — a static bundler import, not a network request. No SSRF surface. **CI workflow additions: no hardcoded secrets, no privileged steps.** The new CI steps use only `${{ github.run_id }}` (a runner-provided variable, not a secret). `grep` runs against a local log file. No external network calls, no credential usage. **Log file contents are safe to grep.** The guard step checks for `[birpc] rpc is closed` in stdout captured from `npm run test:coverage`. This is internal tooling output — no user data, no credentials, no PII can appear there. ### Notes The CI guard is a good detective control for this specific race class. It will catch regressions before they reach human reviewers.

marcel commented

2026-05-12 07:27:57 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The test coverage direction is right. Replacing vi.mock with prop injection produces tests that are more deterministic and less coupled to vitest internals. The two new unit tests cover the injected-loader happy path and the rejection path — both previously untested.

Blockers

usePdfRenderer.svelte.test.ts — rejection test doesn't assert that init() throws

it('leaves pdfjsReady false when libLoader rejects', async () => {
    const failingLoader = vi.fn().mockRejectedValue(new Error('load failed'));
    const r = createPdfRenderer(failingLoader);
    await r.init().catch(() => {});   // error completely swallowed
    expect(r.pdfjsReady).toBe(false);
});

The state assertion is correct, but await r.init().catch(() => {}) makes the test pass even if init() never threw — for example, if a future change silently catches the loader error and sets pdfjsReady = false without re-throwing. The test would pass vacuously and give false coverage confidence on the error propagation contract.

Recommended fix:

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

Now the test proves two things: the error propagates, and the state is left clean.

Suggestions

PdfViewer.svelte.spec.ts — consider naming the fake loader return value

function makeFakeLibLoader(): LibLoader {
    const fakePdfjs = makeFakePdfjsLib();
    return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
}

{ default: '' } is the fake worker URL object. A name like fakeWorkerUrl at the call site would make the tuple structure self-documenting for future readers — but this is cosmetic.

CI guard step — log file not included in artifact upload

The guard step checks /tmp/coverage-test-${{ github.run_id }}.log, but the existing Upload coverage reports step is not modified to include this file. If the guard fails in CI, the log evidence disappears when the runner cleans up. The artifact upload step should include the log:

- name: Upload coverage reports
  if: always()
  uses: actions/upload-artifact@v4
  with:
    name: coverage-${{ github.run_id }}
    path: |
      frontend/coverage/
      /tmp/coverage-test-${{ github.run_id }}.log

Without this, a failing guard gives you the exit 1 but no downloadable log to inspect.

What's Good

Test names read as sentences describing behavior — 'calls injected libLoader during init and sets pdfjsReady', 'leaves pdfjsReady false when libLoader rejects'.
makeFakePdfjsLib and makeFakeLibLoader factory functions follow the project pattern. One-line call sites.
afterEach(cleanup) is correctly hoisted to the top level.
Both the Node-environment unit tests (in usePdfRenderer.svelte.test.ts) and the browser-environment component tests (in PdfViewer.svelte.spec.ts) are updated — no coverage gap between the two test environments.
if: always() on the CI guard is correct — runs even when the coverage step fails, which is exactly when you most want it.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The test coverage direction is right. Replacing `vi.mock` with prop injection produces tests that are more deterministic and less coupled to vitest internals. The two new unit tests cover the injected-loader happy path and the rejection path — both previously untested. ### Blockers **`usePdfRenderer.svelte.test.ts` — rejection test doesn't assert that `init()` throws** ```typescript it('leaves pdfjsReady false when libLoader rejects', async () => { const failingLoader = vi.fn().mockRejectedValue(new Error('load failed')); const r = createPdfRenderer(failingLoader); await r.init().catch(() => {}); // error completely swallowed expect(r.pdfjsReady).toBe(false); }); ``` The state assertion is correct, but `await r.init().catch(() => {})` makes the test pass even if `init()` never threw — for example, if a future change silently catches the loader error and sets `pdfjsReady = false` without re-throwing. The test would pass vacuously and give false coverage confidence on the error propagation contract. Recommended fix: ```typescript 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); }); ``` Now the test proves two things: the error propagates, and the state is left clean. ### Suggestions **`PdfViewer.svelte.spec.ts` — consider naming the fake loader return value** ```typescript function makeFakeLibLoader(): LibLoader { const fakePdfjs = makeFakePdfjsLib(); return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const); } ``` `{ default: '' }` is the fake worker URL object. A name like `fakeWorkerUrl` at the call site would make the tuple structure self-documenting for future readers — but this is cosmetic. **CI guard step — log file not included in artifact upload** The guard step checks `/tmp/coverage-test-${{ github.run_id }}.log`, but the existing `Upload coverage reports` step is not modified to include this file. If the guard fails in CI, the log evidence disappears when the runner cleans up. The artifact upload step should include the log: ```yaml - name: Upload coverage reports if: always() uses: actions/upload-artifact@v4 with: name: coverage-${{ github.run_id }} path: | frontend/coverage/ /tmp/coverage-test-${{ github.run_id }}.log ``` Without this, a failing guard gives you the `exit 1` but no downloadable log to inspect. ### What's Good - Test names read as sentences describing behavior — `'calls injected libLoader during init and sets pdfjsReady'`, `'leaves pdfjsReady false when libLoader rejects'`. - `makeFakePdfjsLib` and `makeFakeLibLoader` factory functions follow the project pattern. One-line call sites. - `afterEach(cleanup)` is correctly hoisted to the top level. - Both the Node-environment unit tests (in `usePdfRenderer.svelte.test.ts`) and the browser-environment component tests (in `PdfViewer.svelte.spec.ts`) are updated — no coverage gap between the two test environments. - `if: always()` on the CI guard is correct — runs even when the coverage step fails, which is exactly when you most want it.

marcel commented

2026-05-12 07:28:12 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ⚠️ Approved with concerns

The CI changes are well-structured. set -eo pipefail, tee, explicit shell: bash, and if: always() are all correct calls. One actionable issue before merge.

Blockers

Log file is not preserved in artifact upload — evidence disappears on guard failure

The coverage log is written to /tmp/coverage-test-${{ github.run_id }}.log. If the guard step fires and exits 1, you'll see the FAIL: line in the runner console — but the log file is gone as soon as the runner tears down. The existing Upload coverage reports step is not modified to include it:

- name: Upload coverage reports
  if: always()
  uses: actions/upload-artifact@v4
  with:
    name: coverage-${{ github.run_id }}
    path: |
      frontend/coverage/
      /tmp/coverage-test-${{ github.run_id }}.log   # ← add this

Without this, a CI failure gives you the exit code but no downloadable evidence. For a guard designed to catch a race condition that shows up intermittently, that log is the only forensics you have.

Suggestions

Use -F (fixed string) in grep for robustness

if grep -q "[birpc] rpc is closed" /tmp/coverage-test-...log 2>/dev/null; then

The current pattern \[birpc\] relies on BRE escaping (\[ = literal [). This works, but -F (fixed string) is more explicit and immune to accidental regex interpretation:

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

Minor — the current version is not wrong, just less self-evident.

What's Good

set -eo pipefail in the coverage step: correct. The | in npm run test:coverage 2>&1 | tee ... would previously have masked a non-zero exit from npm without pipefail.
2>&1 | tee captures both stdout and stderr into the log while still showing output in the runner console — the right pattern.
if: always() on the guard step: essential. This ensures the birpc check runs even when the coverage step exits non-zero, which is the scenario you most need to inspect.
2>/dev/null on the grep: correct — silences the "No such file" error if the log was never created (e.g., the coverage step bailed before writing any output).
No hardcoded secrets. github.run_id is runner metadata, not a secret.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ⚠️ Approved with concerns** The CI changes are well-structured. `set -eo pipefail`, `tee`, explicit `shell: bash`, and `if: always()` are all correct calls. One actionable issue before merge. ### Blockers **Log file is not preserved in artifact upload — evidence disappears on guard failure** The coverage log is written to `/tmp/coverage-test-${{ github.run_id }}.log`. If the guard step fires and exits 1, you'll see the `FAIL:` line in the runner console — but the log file is gone as soon as the runner tears down. The existing `Upload coverage reports` step is not modified to include it: ```yaml - name: Upload coverage reports if: always() uses: actions/upload-artifact@v4 with: name: coverage-${{ github.run_id }} path: | frontend/coverage/ /tmp/coverage-test-${{ github.run_id }}.log # ← add this ``` Without this, a CI failure gives you the exit code but no downloadable evidence. For a guard designed to catch a race condition that shows up intermittently, that log is the only forensics you have. ### Suggestions **Use `-F` (fixed string) in grep for robustness** ```yaml if grep -q "[birpc] rpc is closed" /tmp/coverage-test-...log 2>/dev/null; then ``` The current pattern `\[birpc\]` relies on BRE escaping (`\[` = literal `[`). This works, but `-F` (fixed string) is more explicit and immune to accidental regex interpretation: ```yaml 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 ``` Minor — the current version is not wrong, just less self-evident. ### What's Good - `set -eo pipefail` in the coverage step: correct. The `|` in `npm run test:coverage 2>&1 | tee ...` would previously have masked a non-zero exit from `npm` without `pipefail`. - `2>&1 | tee` captures both stdout and stderr into the log while still showing output in the runner console — the right pattern. - `if: always()` on the guard step: essential. This ensures the birpc check runs even when the coverage step exits non-zero, which is the scenario you most need to inspect. - `2>/dev/null` on the grep: correct — silences the "No such file" error if the log was never created (e.g., the coverage step bailed before writing any output). - No hardcoded secrets. `github.run_id` is runner metadata, not a secret.

marcel commented

2026-05-12 07:28:19 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This PR contains no UI changes, no Svelte template markup changes, no CSS or Tailwind modifications, and no accessibility-relevant DOM structure changes.

What I verified:

PdfViewer.svelte diff — the only template change is the addition of libLoader = undefined to the $props() destructuring. No HTML, no aria attributes, no styling, no focus management, no visual output changed.
No new components, no new routes, no layout changes.
The untrack() call is in the <script> block only — invisible to the rendered DOM.

The UI test assertions in PdfViewer.svelte.spec.ts still check for the navigation buttons ("zurück", "weiter"), zoom controls, and the page counter — confirming the existing UI behavior is preserved under the new injection pattern.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This PR contains no UI changes, no Svelte template markup changes, no CSS or Tailwind modifications, and no accessibility-relevant DOM structure changes. **What I verified:** - `PdfViewer.svelte` diff — the only template change is the addition of `libLoader = undefined` to the `$props()` destructuring. No HTML, no aria attributes, no styling, no focus management, no visual output changed. - No new components, no new routes, no layout changes. - The `untrack()` call is in the `<script>` block only — invisible to the rendered DOM. The UI test assertions in `PdfViewer.svelte.spec.ts` still check for the navigation buttons ("zurück", "weiter"), zoom controls, and the page counter — confirming the existing UI behavior is preserved under the new injection pattern.

marcel commented

2026-05-12 07:28:32 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing against the originating issue (#535) and the PR's stated acceptance criteria.

Requirements Coverage

Acceptance Criterion	Status	Notes
`usePdfRenderer.svelte.test.ts` — new test verifies `libLoader` is called and `pdfjsReady` becomes `true`	✅ Present	`'calls injected libLoader during init and sets pdfjsReady'`
`PdfViewer.svelte.spec.ts` — all 3 existing tests pass with injected fake loader, no `vi.mock`	✅ Present	All 3 tests updated to use `makeFakeLibLoader()`
CI guard step present in `.gitea/workflows/ci.yml`	✅ Present	`Assert no birpc teardown race in coverage run`
60 consecutive green `workflow_dispatch` CI runs after merge	⬜ Deferred	Acknowledged as a post-merge gate in PR description

The deferred acceptance criterion is correctly handled — it's a runtime quality gate that can't be validated in the PR itself. It is explicitly noted in the PR description as unchecked. No concern here.

Scope Verification

The PR is tightly scoped to issue #535. It introduces no new features, no new user-facing behavior, and no API contract changes. The libLoader prop is optional with a production default — existing callers require no changes.

One Open Question

The PR description states: "the birpc route handler that races with worker teardown is never installed." ADR 012 documents this for pdfjs specifically. The ADR's residual exceptions table lists the $app/* / $env/* modules as safe to keep as vi.mock.

OQ-001: Is there a linting or static analysis rule (e.g., an ESLint rule or a Semgrep rule) planned to enforce the ADR 012 constraint automatically — i.e., to flag any future vi.mock(externalLib, factory) call in browser-mode spec files? Without automation, the rule depends on reviewers remembering ADR 012, which is a weak enforcement mechanism for a constraint that's non-obvious.

This is not a blocker for merge, but it's worth tracking as a follow-up issue if recurring violations are anticipated.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing against the originating issue (#535) and the PR's stated acceptance criteria. ### Requirements Coverage | Acceptance Criterion | Status | Notes | |---|---|---| | `usePdfRenderer.svelte.test.ts` — new test verifies `libLoader` is called and `pdfjsReady` becomes `true` | ✅ Present | `'calls injected libLoader during init and sets pdfjsReady'` | | `PdfViewer.svelte.spec.ts` — all 3 existing tests pass with injected fake loader, no `vi.mock` | ✅ Present | All 3 tests updated to use `makeFakeLibLoader()` | | CI guard step present in `.gitea/workflows/ci.yml` | ✅ Present | `Assert no birpc teardown race in coverage run` | | 60 consecutive green `workflow_dispatch` CI runs after merge | ⬜ Deferred | Acknowledged as a post-merge gate in PR description | The deferred acceptance criterion is correctly handled — it's a runtime quality gate that can't be validated in the PR itself. It is explicitly noted in the PR description as unchecked. No concern here. ### Scope Verification The PR is tightly scoped to issue #535. It introduces no new features, no new user-facing behavior, and no API contract changes. The `libLoader` prop is optional with a production default — existing callers require no changes. ### One Open Question The PR description states: *"the birpc route handler that races with worker teardown is never installed."* ADR 012 documents this for pdfjs specifically. The ADR's residual exceptions table lists the `$app/*` / `$env/*` modules as safe to keep as `vi.mock`. **OQ-001:** Is there a linting or static analysis rule (e.g., an ESLint rule or a Semgrep rule) planned to enforce the ADR 012 constraint automatically — i.e., to flag any future `vi.mock(externalLib, factory)` call in browser-mode spec files? Without automation, the rule depends on reviewers remembering ADR 012, which is a weak enforcement mechanism for a constraint that's non-obvious. This is not a blocker for merge, but it's worth tracking as a follow-up issue if recurring violations are anticipated.

marcel commented

2026-05-12 07:58:45 +02:00

Round 3 review feedback addressed

Four commits pushed to this branch:

`2163cd96` — test(pdf-renderer): assert init() re-throws when libLoader rejects

Sara (9012, blocker) + Felix (9008, suggestion): await r.init().catch(() => {}) was swallowing the rejection, so the test passed vacuously even if a future refactor silently caught the loader error. Replaced with await expect(r.init()).rejects.toThrow('load failed'), which proves two things: the error propagates from init(), and pdfjsReady stays false. Red phase was not achievable here — init() already propagates rejections (no try/catch) — this is a test contract tightening, not a new behavior.

`58eb6e27` — refactor(test): remove what-comment from makeFakePdfjsLib

Felix (9008, suggestion): Removed // Partial fake: only the methods used by usePdfRenderer are implemented from PdfViewer.svelte.spec.ts. The name makeFakePdfjsLib already implies it's a fake; the comment described what, not why.

`017b404f` — ci(coverage): include coverage log in artifact upload

Tobias (9013, blocker) + Sara (9012, suggestion): Added /tmp/coverage-test-${{ github.run_id }}.log to the Upload coverage reports artifact step. Without this, a guard failure leaves only the exit 1 visible in the runner console — the log file with the actual [birpc] rpc is closed lines disappears when the runner tears down.

`48b7e0f3` — ci(coverage): use grep -F for birpc guard to avoid BRE escaping

Tobias (9013, suggestion): Replaced grep -q "\[birpc\] rpc is closed" with grep -qF "[birpc] rpc is closed". -F (fixed string) matches the literal pattern without relying on BRE bracket escaping, making the intent explicit and immune to accidental regex interpretation.

All 1851 tests pass (1 pre-existing flaky timeout in confirm.svelte.test.ts — tracked as #538).

## Round 3 review feedback addressed Four commits pushed to this branch: ### `2163cd96` — test(pdf-renderer): assert init() re-throws when libLoader rejects - **Sara (9012, blocker) + Felix (9008, suggestion):** `await r.init().catch(() => {})` was swallowing the rejection, so the test passed vacuously even if a future refactor silently caught the loader error. Replaced with `await expect(r.init()).rejects.toThrow('load failed')`, which proves two things: the error propagates from `init()`, and `pdfjsReady` stays `false`. Red phase was not achievable here — `init()` already propagates rejections (no try/catch) — this is a test contract tightening, not a new behavior. ### `58eb6e27` — refactor(test): remove what-comment from makeFakePdfjsLib - **Felix (9008, suggestion):** Removed `// Partial fake: only the methods used by usePdfRenderer are implemented` from `PdfViewer.svelte.spec.ts`. The name `makeFakePdfjsLib` already implies it's a fake; the comment described *what*, not *why*. ### `017b404f` — ci(coverage): include coverage log in artifact upload - **Tobias (9013, blocker) + Sara (9012, suggestion):** Added `/tmp/coverage-test-${{ github.run_id }}.log` to the `Upload coverage reports` artifact step. Without this, a guard failure leaves only the `exit 1` visible in the runner console — the log file with the actual `[birpc] rpc is closed` lines disappears when the runner tears down. ### `48b7e0f3` — ci(coverage): use grep -F for birpc guard to avoid BRE escaping - **Tobias (9013, suggestion):** Replaced `grep -q "\[birpc\] rpc is closed"` with `grep -qF "[birpc] rpc is closed"`. `-F` (fixed string) matches the literal pattern without relying on BRE bracket escaping, making the intent explicit and immune to accidental regex interpretation. --- **All 1851 tests pass** (1 pre-existing flaky timeout in `confirm.svelte.test.ts` — tracked as #538).

marcel commented

2026-05-12 09:06:43 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Three rounds of review have tightened this PR well. I'll only flag angles not yet covered.

New observations

1. init() has no concurrent-call guard (usePdfRenderer.svelte.ts)

init() is called once from onMount today, so re-entrancy isn't a real scenario right now. But nothing in the function prevents a second call while the first is still awaiting libLoader():

async function init(): Promise<void> {
    const [lib, { default: workerUrl }] = await libLoader(); // <- both callers reach here
    pdfjsLib = lib;
    pdfjsReady = true;
}

If loadDocument() (which presumably checks pdfjsReady or calls init() defensively) ever races with onMount, libLoader fires twice. A one-liner guard at the top would close it:

async function init(): Promise<void> {
    if (pdfjsReady) return;
    // ...
}

Not a blocker now, but worth fixing before the pattern spreads.

2. TextLayerMock uses old-style prototype assignment (PdfViewer.svelte.spec.ts)

function TextLayerMock() {}
TextLayerMock.prototype.render = () => Promise.resolve();
TextLayerMock.prototype.cancel = () => {};

This works, but the rest of the file (and the codebase) uses TypeScript class syntax. The equivalent is cleaner and consistent:

class TextLayerMock {
    render() { return Promise.resolve(); }
    cancel() {}
}

Cosmetic, but it stands out in an otherwise modern file.

3. makeFakePdfjsLib is duplicated across two test files

PdfViewer.svelte.spec.ts and usePdfRenderer.svelte.test.ts each define their own copy of the pdfjs fake. They're structurally similar but not identical. As more specs adopt the libLoader pattern (per ADR 012), this fake will be defined a third, fourth time. A shared __testUtils__/fakePdfjs.ts (or similar) would prevent drift. Not a blocker for this PR, but worth a follow-up issue before the next component adopts the pattern.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** Three rounds of review have tightened this PR well. I'll only flag angles not yet covered. ### New observations **1. `init()` has no concurrent-call guard (`usePdfRenderer.svelte.ts`)** `init()` is called once from `onMount` today, so re-entrancy isn't a real scenario right now. But nothing in the function prevents a second call while the first is still awaiting `libLoader()`: ```typescript async function init(): Promise<void> { const [lib, { default: workerUrl }] = await libLoader(); // <- both callers reach here pdfjsLib = lib; pdfjsReady = true; } ``` If `loadDocument()` (which presumably checks `pdfjsReady` or calls `init()` defensively) ever races with `onMount`, `libLoader` fires twice. A one-liner guard at the top would close it: ```typescript async function init(): Promise<void> { if (pdfjsReady) return; // ... } ``` Not a blocker now, but worth fixing before the pattern spreads. **2. `TextLayerMock` uses old-style prototype assignment (`PdfViewer.svelte.spec.ts`)** ```typescript function TextLayerMock() {} TextLayerMock.prototype.render = () => Promise.resolve(); TextLayerMock.prototype.cancel = () => {}; ``` This works, but the rest of the file (and the codebase) uses TypeScript class syntax. The equivalent is cleaner and consistent: ```typescript class TextLayerMock { render() { return Promise.resolve(); } cancel() {} } ``` Cosmetic, but it stands out in an otherwise modern file. **3. `makeFakePdfjsLib` is duplicated across two test files** `PdfViewer.svelte.spec.ts` and `usePdfRenderer.svelte.test.ts` each define their own copy of the pdfjs fake. They're structurally similar but not identical. As more specs adopt the `libLoader` pattern (per ADR 012), this fake will be defined a third, fourth time. A shared `__testUtils__/fakePdfjs.ts` (or similar) would prevent drift. Not a blocker for this PR, but worth a follow-up issue before the next component adopts the pattern.

marcel commented

2026-05-12 09:07:01 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

Previous rounds covered test structure thoroughly. Two angles not yet raised:

1. CI guard is diagnostic, not detection — and only covers the coverage run

The guard step detects [birpc] rpc is closed in the coverage log. But if a future developer re-introduces a vi.mock(externalLib, factory) in a browser-mode spec, the birpc race would appear in both npm test and npm run test:coverage.

The CI pipeline runs npm test before the coverage step. That run would fail with exit 1, no diagnostic message, and the developer gets a cryptic failure. Only if they then look at the later coverage run do they see the guard explanation.

The guard is correct as a regression backstop, but it's mispositioned for diagnosis. Options:

Add the same tee-and-grep wrapper to the regular npm test step too (slight duplication)
Add a comment in the CI YAML noting that the guard only fires in coverage mode, so a birpc-related failure in npm test requires looking at the coverage run for the diagnostic message

Neither is a blocker, but the current state means a first-time failure diagnosis is non-obvious.

2. Loading state transitions are not covered in browser-mode specs

The three PdfViewer.svelte.spec.ts tests verify that buttons and the page counter are visible. The third test (displays the page counter once the PDF has loaded) implicitly exercises the async init path — but none of the tests check what the user sees while init() is pending.

Between onMount and pdfjsReady = true, the component is in a loading state. If it renders controls (as the first two tests suggest), users could click navigation before a document is loaded. If it renders a spinner, we have no test proving the spinner appears.

This is pre-existing behavior, not introduced by this PR. But the new explicit libLoader seam makes the async boundary visible and testable in a way it wasn't before. A test for the "during init" state would fully close the behavior coverage:

it('shows a loading indicator while the library is initialising', async () => {
    let resolveLoader!: () => void;
    const slowLoader: LibLoader = () =>
        new Promise(resolve => { resolveLoader = () => resolve([fakePdfjs, { default: '' }] as const); });
    render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: slowLoader });
    await expect.element(page.getByRole('status')).toBeInTheDocument(); // spinner?
    resolveLoader();
    await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument();
});

Whether this reveals a missing spinner or confirms controls appear immediately, either outcome is valuable. Suggest filing as a follow-up issue.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** Previous rounds covered test structure thoroughly. Two angles not yet raised: ### 1. CI guard is diagnostic, not detection — and only covers the coverage run The guard step detects `[birpc] rpc is closed` in the coverage log. But if a future developer re-introduces a `vi.mock(externalLib, factory)` in a browser-mode spec, the birpc race would appear in **both** `npm test` and `npm run test:coverage`. The CI pipeline runs `npm test` before the coverage step. That run would fail with exit 1, no diagnostic message, and the developer gets a cryptic failure. Only if they then look at the later coverage run do they see the guard explanation. The guard is correct as a regression backstop, but it's mispositioned for diagnosis. Options: - Add the same `tee`-and-grep wrapper to the regular `npm test` step too (slight duplication) - Add a comment in the CI YAML noting that the guard only fires in coverage mode, so a birpc-related failure in `npm test` requires looking at the coverage run for the diagnostic message Neither is a blocker, but the current state means a first-time failure diagnosis is non-obvious. ### 2. Loading state transitions are not covered in browser-mode specs The three `PdfViewer.svelte.spec.ts` tests verify that buttons and the page counter are visible. The third test (`displays the page counter once the PDF has loaded`) implicitly exercises the async init path — but none of the tests check **what the user sees while `init()` is pending**. Between `onMount` and `pdfjsReady = true`, the component is in a loading state. If it renders controls (as the first two tests suggest), users could click navigation before a document is loaded. If it renders a spinner, we have no test proving the spinner appears. This is pre-existing behavior, not introduced by this PR. But the new explicit `libLoader` seam makes the async boundary visible and testable in a way it wasn't before. A test for the "during init" state would fully close the behavior coverage: ```typescript it('shows a loading indicator while the library is initialising', async () => { let resolveLoader!: () => void; const slowLoader: LibLoader = () => new Promise(resolve => { resolveLoader = () => resolve([fakePdfjs, { default: '' }] as const); }); render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: slowLoader }); await expect.element(page.getByRole('status')).toBeInTheDocument(); // spinner? resolveLoader(); await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument(); }); ``` Whether this reveals a missing spinner or confirms controls appear immediately, either outcome is valuable. Suggest filing as a follow-up issue.

marcel commented

2026-05-12 09:07:12 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Previous rounds addressed pipefail, run_id scoping, -F flag, and artifact upload. Nothing to add on those. One new observation:

The diagnostic guard is positioned after the problem it diagnoses

If a future birpc regression is re-introduced, it shows up in both the regular npm test step and the coverage step. In CI, npm test runs before npm run test:coverage.

The pipeline would:

npm test — fails with exit 1, no diagnostic output
npm run test:coverage — fails with exit 1, tee captures output
Guard step — if: always() fires, prints the [birpc] rpc is closed diagnostic

The developer sees a failing pipeline, reads the summary, and finds the diagnostic only in step 3 of a step that ran after the already-failing npm test. On first encounter, this is non-obvious — especially since the issue title is "birpc teardown race" and the guard is the only step that names it.

This is a CI ergonomics concern, not a correctness issue. The guard works. But a one-line comment above the coverage step or the guard step would help future-Tobias navigate:

# Guard catches birpc teardown races in coverage mode.
# If npm test (above) fails with a birpc error, the diagnostic appears here in the coverage guard.
- name: Assert no birpc teardown race in coverage run

Or alternatively: wrap npm test the same way. Depends on whether the birpc race is reproducible outside coverage mode — which, given that coverage instrumentation adds load, it might not be.

What's solid: if: always(), 2>/dev/null, -F flag, run_id scoping, artifact inclusion — all correct. Good defensive posture for an intermittent race condition.

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Previous rounds addressed `pipefail`, `run_id` scoping, `-F` flag, and artifact upload. Nothing to add on those. One new observation: ### The diagnostic guard is positioned after the problem it diagnoses If a future birpc regression is re-introduced, it shows up in **both** the regular `npm test` step and the coverage step. In CI, `npm test` runs before `npm run test:coverage`. The pipeline would: 1. `npm test` — fails with exit 1, no diagnostic output 2. `npm run test:coverage` — fails with exit 1, tee captures output 3. Guard step — `if: always()` fires, prints the `[birpc] rpc is closed` diagnostic The developer sees a failing pipeline, reads the summary, and finds the diagnostic only in step 3 of a step that ran after the already-failing `npm test`. On first encounter, this is non-obvious — especially since the issue title is "birpc teardown race" and the guard is the only step that names it. This is a CI ergonomics concern, not a correctness issue. The guard works. But a one-line comment above the coverage step or the guard step would help future-Tobias navigate: ```yaml # Guard catches birpc teardown races in coverage mode. # If npm test (above) fails with a birpc error, the diagnostic appears here in the coverage guard. - name: Assert no birpc teardown race in coverage run ``` Or alternatively: wrap `npm test` the same way. Depends on whether the birpc race is reproducible outside coverage mode — which, given that coverage instrumentation adds load, it might not be. What's solid: `if: always()`, `2>/dev/null`, `-F` flag, `run_id` scoping, artifact inclusion — all correct. Good defensive posture for an intermittent race condition.

marcel commented

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

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

ADR 012, the LibLoader DI seam, and the module boundaries are all sound. Previous rounds covered this thoroughly. One forward-looking observation:

Pattern proliferation: ADR 012 doesn't say when to revisit `LibLoader`'s home

ADR 012 establishes the libLoader pattern for pdfjs-dist. That's one dynamic-import dependency in one viewer module. As written, if a future component needs to inject its own external library loader (e.g., a hypothetical VideoPlayer injecting a video codec library, or a ChartViewer injecting a charting library), the pattern calls for each component to:

Define its own LibLoader type in its own module
Export it for use by its spec file
Provide its own defaultLibLoader constant

This is correct for 1–2 instances. At 4–5 instances, the pattern starts producing near-identical LibLoader types in different modules:

// usePdfRenderer.svelte.ts
export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;

// useVideoRenderer.svelte.ts (hypothetical)
export type LibLoader = () => Promise<readonly [typeof import('some-video-lib'), { default: string }]>;

The ADR's "Consequences" section could benefit from a one-line note: "If three or more components adopt this pattern, consider a shared $lib/types/lib-loader.ts or a generic DynamicImportLoader<T> type to avoid parallel type definitions."

Not a blocker. Not even a suggestion for this PR. Just a "when to revisit" signal the ADR currently lacks.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ADR 012, the `LibLoader` DI seam, and the module boundaries are all sound. Previous rounds covered this thoroughly. One forward-looking observation: ### Pattern proliferation: ADR 012 doesn't say when to revisit `LibLoader`'s home ADR 012 establishes the `libLoader` pattern for pdfjs-dist. That's one dynamic-import dependency in one viewer module. As written, if a future component needs to inject its own external library loader (e.g., a hypothetical `VideoPlayer` injecting a video codec library, or a `ChartViewer` injecting a charting library), the pattern calls for each component to: 1. Define its own `LibLoader` type in its own module 2. Export it for use by its spec file 3. Provide its own `defaultLibLoader` constant This is correct for 1–2 instances. At 4–5 instances, the pattern starts producing near-identical `LibLoader` types in different modules: ```typescript // usePdfRenderer.svelte.ts export type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>; // useVideoRenderer.svelte.ts (hypothetical) export type LibLoader = () => Promise<readonly [typeof import('some-video-lib'), { default: string }]>; ``` The ADR's "Consequences" section could benefit from a one-line note: *"If three or more components adopt this pattern, consider a shared `$lib/types/lib-loader.ts` or a generic `DynamicImportLoader<T>` type to avoid parallel type definitions."* Not a blocker. Not even a suggestion for this PR. Just a "when to revisit" signal the ADR currently lacks.

marcel commented

2026-05-12 09:07:32 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Security surface is unchanged from prior rounds. One angle not previously raised:

CI log artifact captures full test output — check for sensitive test data

The coverage step pipes 2>&1 into tee /tmp/coverage-test-${{ github.run_id }}.log, which is then uploaded as a CI artifact. This means the complete vitest stdout+stderr — including test names, assertion messages, and any printed values — is archived as a downloadable artifact.

In isolation this is standard practice. The specific concern for this project: Familienarchiv test fixtures should use synthetic family data (fake names, fake dates, fake document content), not copies of real archive entries. If a test ever uses a real person's name or document text as a fixture value, it would appear in the CI artifact log.

This is not a bug introduced by this PR (the coverage log has always captured this output; this PR just adds the log to the artifact upload). But it's the first time that output becomes a persistently downloadable artifact, so it's worth a quick review of browser-mode spec fixtures to confirm they use clearly synthetic data.

Specific files to spot-check: PdfViewer.svelte.spec.ts (uses URL /api/documents/test-id/file — synthetic ✅), usePdfRenderer.svelte.test.ts (no document data — ✅). Looks clean. Just flagging the category for awareness as more specs are added.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** Security surface is unchanged from prior rounds. One angle not previously raised: ### CI log artifact captures full test output — check for sensitive test data The coverage step pipes `2>&1` into `tee /tmp/coverage-test-${{ github.run_id }}.log`, which is then uploaded as a CI artifact. This means the complete vitest stdout+stderr — including test names, assertion messages, and any printed values — is archived as a downloadable artifact. In isolation this is standard practice. The specific concern for this project: Familienarchiv test fixtures should use synthetic family data (fake names, fake dates, fake document content), not copies of real archive entries. If a test ever uses a real person's name or document text as a fixture value, it would appear in the CI artifact log. This is not a bug introduced by this PR (the coverage log has always captured this output; this PR just adds the log to the artifact upload). But it's the first time that output becomes a persistently downloadable artifact, so it's worth a quick review of browser-mode spec fixtures to confirm they use clearly synthetic data. Specific files to spot-check: `PdfViewer.svelte.spec.ts` (uses URL `/api/documents/test-id/file` — synthetic ✅), `usePdfRenderer.svelte.test.ts` (no document data — ✅). Looks clean. Just flagging the category for awareness as more specs are added.

marcel commented

2026-05-12 09:07:43 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved with one pre-existing observation

No UI changes in this PR — template markup, ARIA attributes, Tailwind classes, and focus management are all untouched. Previous rounds confirmed this and I agree.

Pre-existing gap surfaced by the explicit `pdfjsReady` flag

This PR introduces pdfjsReady as an explicit, named reactive state. Looking at the component from the outside: between onMount and pdfjsReady = true, the PDF viewer is in an indeterminate state. The browser-mode specs confirm that navigation buttons and zoom controls are visible immediately (the first two tests render and assert without waiting for init). This means:

Controls are visible before the PDF library has loaded. A user arriving on a document page with a slow connection could see navigation buttons for a document that hasn't started loading yet.
No loading indicator test exists for this window. Whether there's a spinner, a disabled state, or bare controls isn't verified by the test suite.
No error indicator test exists for when init() propagates a rejection. The test now confirms pdfjsReady stays false, but what does the user see? Presumably an indefinite loading state with no message.

This is pre-existing behavior — nothing in this PR makes it worse. But now that pdfjsReady has a name and a test, the loading/error state design gap is more visible than before.

Recommend filing a follow-up issue: "UX: PDF viewer has no loading indicator or error state when pdfjs-dist fails to initialise." The libLoader injection pattern in this PR makes that issue testable in isolation, which is a win.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved with one pre-existing observation** No UI changes in this PR — template markup, ARIA attributes, Tailwind classes, and focus management are all untouched. Previous rounds confirmed this and I agree. ### Pre-existing gap surfaced by the explicit `pdfjsReady` flag This PR introduces `pdfjsReady` as an explicit, named reactive state. Looking at the component from the outside: between `onMount` and `pdfjsReady = true`, the PDF viewer is in an indeterminate state. The browser-mode specs confirm that navigation buttons and zoom controls are **visible immediately** (the first two tests render and assert without waiting for init). This means: - **Controls are visible before the PDF library has loaded.** A user arriving on a document page with a slow connection could see navigation buttons for a document that hasn't started loading yet. - **No loading indicator test exists** for this window. Whether there's a spinner, a disabled state, or bare controls isn't verified by the test suite. - **No error indicator test exists** for when `init()` propagates a rejection. The test now confirms `pdfjsReady` stays `false`, but what does the user see? Presumably an indefinite loading state with no message. This is pre-existing behavior — nothing in this PR makes it worse. But now that `pdfjsReady` has a name and a test, the loading/error state design gap is more visible than before. Recommend filing a follow-up issue: *"UX: PDF viewer has no loading indicator or error state when pdfjs-dist fails to initialise."* The `libLoader` injection pattern in this PR makes that issue testable in isolation, which is a win.

marcel commented

2026-05-12 09:07:57 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Requirements traceability and test plan coverage are solid and have been verified across three rounds. Two items still unresolved:

OQ-001 (raised round 3, still open): ADR 012 enforcement via static analysis

Round 3 raised whether an ESLint or Semgrep rule should enforce the ADR 012 constraint automatically. No response or follow-up issue has appeared. The current enforcement mechanism is:

Reviewer remembers ADR 012
CI guard catches regressions after-the-fact (coverage run only)

This is a weak prevention layer. The CI guard detects a regression after it's been committed and merged into a test run; it doesn't prevent the offending vi.mock from being committed at all. A pre-commit or CI lint rule would catch it at the right moment.

If enforcement via lint isn't planned, that decision should be explicit in the ADR ("no automated enforcement; CI guard is the backstop") so future reviewers don't wonder why.

The 60-run quality gate is underspecified for post-merge operation

The PR body states: "60 consecutive green workflow_dispatch CI runs against main after merge." Across three review rounds, multiple personas have asked whether this is tracked. Still no confirmation.

Beyond tracking, the criterion itself has operationalization gaps:

Question	Currently specified?
Who triggers the runs?	No
What cadence? (daily? per-deploy? manual?)	No
If run 47 fails due to an unrelated flaky test, does the count reset?	No
What is the definition of "the fix is confirmed"? When does #535 close?	No

A quality gate without a process is a hope, not a gate. Recommend adding a comment to issue #535 (or a dedicated follow-up issue) that answers these questions, or explicitly accepting the informal interpretation: "if the coverage guard stays silent for 60 runs of any kind on main, we're satisfied."

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Requirements traceability and test plan coverage are solid and have been verified across three rounds. Two items still unresolved: ### OQ-001 (raised round 3, still open): ADR 012 enforcement via static analysis Round 3 raised whether an ESLint or Semgrep rule should enforce the ADR 012 constraint automatically. No response or follow-up issue has appeared. The current enforcement mechanism is: 1. Reviewer remembers ADR 012 2. CI guard catches regressions after-the-fact (coverage run only) This is a weak prevention layer. The CI guard detects a regression after it's been committed and merged into a test run; it doesn't prevent the offending `vi.mock` from being committed at all. A pre-commit or CI lint rule would catch it at the right moment. If enforcement via lint isn't planned, that decision should be explicit in the ADR ("no automated enforcement; CI guard is the backstop") so future reviewers don't wonder why. ### The 60-run quality gate is underspecified for post-merge operation The PR body states: *"60 consecutive green `workflow_dispatch` CI runs against `main` after merge."* Across three review rounds, multiple personas have asked whether this is tracked. Still no confirmation. Beyond tracking, the criterion itself has operationalization gaps: | Question | Currently specified? | |---|---| | Who triggers the runs? | No | | What cadence? (daily? per-deploy? manual?) | No | | If run 47 fails due to an unrelated flaky test, does the count reset? | No | | What is the definition of "the fix is confirmed"? When does #535 close? | No | A quality gate without a process is a hope, not a gate. Recommend adding a comment to issue #535 (or a dedicated follow-up issue) that answers these questions, or explicitly accepting the informal interpretation: "if the coverage guard stays silent for 60 runs of any kind on main, we're satisfied."

marcel referenced this pull request

2026-05-12 09:39:45 +02:00

test: share fakePdfjs fixture across viewer test files #542

marcel referenced this pull request

2026-05-12 09:39:53 +02:00

UX: PDF viewer has no loading indicator or error state when pdfjs-dist fails to initialise #543

marcel referenced this pull request

2026-05-12 09:40:00 +02:00

Unit & Component Tests job exits 1 from vitest-browser teardown race — every test green but CI red #535

marcel referenced this issue from a commit

2026-05-12 09:46:31 +02:00

test(pdf-renderer): guard init() against repeated calls — libLoader must fire once

marcel referenced this issue from a commit

2026-05-12 09:46:31 +02:00

refactor(test): convert TextLayerMock to class syntax in PdfViewer spec

marcel referenced this issue from a commit

2026-05-12 09:46:31 +02:00

ci(coverage): document that birpc guard covers coverage run only

marcel referenced this issue from a commit

2026-05-12 09:46:31 +02:00

docs(adr-012): add enforcement note and libLoader revisit signal

marcel commented

2026-05-12 09:46:49 +02:00

Round 4 review feedback addressed

Four commits pushed to this branch:

`0b3ce838` — test(pdf-renderer): guard init() against repeated calls — libLoader must fire once

Felix (9026, suggestion #1): init() had no idempotency guard — calling it twice would invoke libLoader twice. Added failing test first (init() called twice → fakeLoader called twice → red), then added if (pdfjsReady) return; at the top of init() → green. Closes the re-entrancy gap before the pattern spreads.

`185c754e` — refactor(test): convert TextLayerMock to class syntax in PdfViewer spec

Felix (9026, suggestion #2): TextLayerMock used old-style prototype assignment — a leftover from the vi.mock era. Converted to class syntax to match the rest of the codebase. All 3 PdfViewer tests still pass.

`8396d82c` — ci(coverage): document that birpc guard covers coverage run only

Sara (9027, concern #1) + Tobias (9028): Added a YAML comment above the assertion step so a future developer diagnosing a [birpc] failure in npm test knows to look at the coverage guard step for the diagnostic message.

`3d9e10f3` — docs(adr-012): add enforcement note and libLoader revisit signal

Elicit OQ-001 (9032): Added explicit statement to Consequences: no lint rule is planned, CI guard is the backstop. If a rule is added later, the ADR should be updated.
Markus (9029): Added "when to revisit" signal: if 3+ components adopt libLoader, consider a shared DynamicImportLoader<T> type.

Deferred as follow-up issues:

#542 — test: share fakePdfjs fixture across viewer test files (Felix concern #3)
#543 — UX: PDF viewer has no loading indicator or error state when pdfjs-dist fails to initialise (Sara concern #2 + Leonie)
#541 — test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs (pre-existing flaky tests)

60-run gate operationalization: answered in a comment on #535.

Test suite: 1851 passed / 1853 total. The 2 remaining failures are OcrTrainingCard.svelte.spec.ts (pre-existing track_reactivity_loss flake, tracked as #541) — unrelated to this PR.

## Round 4 review feedback addressed Four commits pushed to this branch: ### `0b3ce838` — test(pdf-renderer): guard init() against repeated calls — libLoader must fire once - **Felix (9026, suggestion #1):** `init()` had no idempotency guard — calling it twice would invoke `libLoader` twice. Added failing test first (`init()` called twice → `fakeLoader` called twice → red), then added `if (pdfjsReady) return;` at the top of `init()` → green. Closes the re-entrancy gap before the pattern spreads. ### `185c754e` — refactor(test): convert TextLayerMock to class syntax in PdfViewer spec - **Felix (9026, suggestion #2):** `TextLayerMock` used old-style prototype assignment — a leftover from the `vi.mock` era. Converted to class syntax to match the rest of the codebase. All 3 `PdfViewer` tests still pass. ### `8396d82c` — ci(coverage): document that birpc guard covers coverage run only - **Sara (9027, concern #1) + Tobias (9028):** Added a YAML comment above the assertion step so a future developer diagnosing a `[birpc]` failure in `npm test` knows to look at the coverage guard step for the diagnostic message. ### `3d9e10f3` — docs(adr-012): add enforcement note and libLoader revisit signal - **Elicit OQ-001 (9032):** Added explicit statement to Consequences: no lint rule is planned, CI guard is the backstop. If a rule is added later, the ADR should be updated. - **Markus (9029):** Added "when to revisit" signal: if 3+ components adopt `libLoader`, consider a shared `DynamicImportLoader<T>` type. --- **Deferred as follow-up issues:** - **#542** — `test: share fakePdfjs fixture across viewer test files` (Felix concern #3) - **#543** — `UX: PDF viewer has no loading indicator or error state when pdfjs-dist fails to initialise` (Sara concern #2 + Leonie) - **#541** — `test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs` (pre-existing flaky tests) **60-run gate operationalization:** answered in a comment on #535. --- **Test suite:** 1851 passed / 1853 total. The 2 remaining failures are `OcrTrainingCard.svelte.spec.ts` (pre-existing `track_reactivity_loss` flake, tracked as #541) — unrelated to this PR.

marcel commented

2026-05-12 09:51:22 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

What I checked

TDD evidence, naming, function size, Svelte 5 patterns, test factory discipline, and comment hygiene.

Positives

TDD evidence is solid. Three new tests cover every behavioral change:

'calls injected libLoader during init and sets pdfjsReady' — happy path
'leaves pdfjsReady false when libLoader rejects' — error path
'init() is idempotent — libLoader called only once on repeated calls' — the key guard behaviour

The idempotency guard if (pdfjsReady) return; in usePdfRenderer.svelte.ts:26 is the most important production change — it has its own test, which is correct.

Naming and structure are clean. LibLoader, defaultLibLoader, makeFakePdfjsLib(), makeFakeLibLoader() — all names reveal intent. The // untrack: libLoader prop change must not reinitialise the renderer comment in PdfViewer.svelte:39 is exactly the right comment — it explains the WHY (a reactive re-init invariant), not the WHAT.

TextLayerMock converted to class syntax — correct modernization; the constructor-function form was a JS quirk that is now gone.

Suggestions (non-blocking)

Duplicated fakePdfjs literal in usePdfRenderer.svelte.test.ts. The same inline object ({ GlobalWorkerOptions, getDocument, TextLayer }) is constructed independently in two tests. PdfViewer.svelte.spec.ts already has makeFakePdfjsLib() as a factory — the same helper should move to (or be shared by) usePdfRenderer.svelte.test.ts to prevent drift if the shape changes.

// usePdfRenderer.svelte.test.ts — add at top of file
function makeFakePdfjsLib() {
  return {
    GlobalWorkerOptions: { workerSrc: '' },
    getDocument: vi.fn(),
    TextLayer: class {}
  } as unknown as typeof import('pdfjs-dist');
}

This is cosmetic — doesn't affect correctness — but is consistent with the project's factory-function discipline.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### What I checked TDD evidence, naming, function size, Svelte 5 patterns, test factory discipline, and comment hygiene. ### Positives **TDD evidence is solid.** Three new tests cover every behavioral change: - `'calls injected libLoader during init and sets pdfjsReady'` — happy path - `'leaves pdfjsReady false when libLoader rejects'` — error path - `'init() is idempotent — libLoader called only once on repeated calls'` — the key guard behaviour The idempotency guard `if (pdfjsReady) return;` in `usePdfRenderer.svelte.ts:26` is the most important production change — it has its own test, which is correct. **Naming and structure are clean.** `LibLoader`, `defaultLibLoader`, `makeFakePdfjsLib()`, `makeFakeLibLoader()` — all names reveal intent. The `// untrack: libLoader prop change must not reinitialise the renderer` comment in `PdfViewer.svelte:39` is exactly the right comment — it explains the WHY (a reactive re-init invariant), not the WHAT. **`TextLayerMock` converted to class syntax** — correct modernization; the constructor-function form was a JS quirk that is now gone. ### Suggestions (non-blocking) **Duplicated `fakePdfjs` literal in `usePdfRenderer.svelte.test.ts`.** The same inline object (`{ GlobalWorkerOptions, getDocument, TextLayer }`) is constructed independently in two tests. `PdfViewer.svelte.spec.ts` already has `makeFakePdfjsLib()` as a factory — the same helper should move to (or be shared by) `usePdfRenderer.svelte.test.ts` to prevent drift if the shape changes. ```typescript // usePdfRenderer.svelte.test.ts — add at top of file function makeFakePdfjsLib() { return { GlobalWorkerOptions: { workerSrc: '' }, getDocument: vi.fn(), TextLayer: class {} } as unknown as typeof import('pdfjs-dist'); } ``` This is cosmetic — doesn't affect correctness — but is consistent with the project's factory-function discipline.

marcel commented

2026-05-12 09:51:34 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

ADR quality, abstraction placement, doc update obligations, and forward-looking extensibility notes.

Positives

ADR 012 is the right artifact for this decision. Root cause, decision, pattern example, residual exceptions table, and consequences are all present and specific. The table of acceptable vi.mock calls ($app/*, $env/*) with their justification is exactly the kind of knowledge that would otherwise silently erode — it now lives in the repo permanently.

Abstraction is at the right layer. LibLoader lives in usePdfRenderer.svelte.ts, not in the component. The component just threads the optional prop through via untrack(). This matches the "logic in hooks, not components" principle we apply across the frontend.

The "When to revisit the LibLoader home" note is correct deferral. Extracting a shared $lib/types/lib-loader.ts at three adopters is the right threshold — not prematurely, not retroactively.

Doc update check

PR category	Required update	Status
New Flyway migration	DB diagrams	— not applicable
New backend package	CLAUDE.md + C4 L3	— not applicable
New SvelteKit route	CLAUDE.md + C4 L3	— not applicable
New Docker service	L2 containers + DEPLOYMENT.md	— not applicable
Architectural decision with lasting consequences	New ADR in `docs/adr/`	✅ ADR 012 added

No missing doc updates.

One observation (non-blocking)

The ADR states "No automated lint rule is planned; the CI coverage guard is the regression backstop." That's a valid decision, but it creates a single point of failure: if someone changes the grep path or the log file location in CI, the guard silently stops detecting regressions. Consider adding one sentence to the ADR's Consequences section: "If the CI guard step is modified (log path, grep pattern), update this ADR's Enforcement section." This makes the dependency explicit.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked ADR quality, abstraction placement, doc update obligations, and forward-looking extensibility notes. ### Positives **ADR 012 is the right artifact for this decision.** Root cause, decision, pattern example, residual exceptions table, and consequences are all present and specific. The table of acceptable `vi.mock` calls (`$app/*`, `$env/*`) with their justification is exactly the kind of knowledge that would otherwise silently erode — it now lives in the repo permanently. **Abstraction is at the right layer.** `LibLoader` lives in `usePdfRenderer.svelte.ts`, not in the component. The component just threads the optional prop through via `untrack()`. This matches the "logic in hooks, not components" principle we apply across the frontend. **The "When to revisit the LibLoader home" note is correct deferral.** Extracting a shared `$lib/types/lib-loader.ts` at three adopters is the right threshold — not prematurely, not retroactively. ### Doc update check | PR category | Required update | Status | |---|---|---| | New Flyway migration | DB diagrams | — not applicable | | New backend package | CLAUDE.md + C4 L3 | — not applicable | | New SvelteKit route | CLAUDE.md + C4 L3 | — not applicable | | New Docker service | L2 containers + DEPLOYMENT.md | — not applicable | | Architectural decision with lasting consequences | New ADR in `docs/adr/` | ✅ ADR 012 added | No missing doc updates. ### One observation (non-blocking) The ADR states "No automated lint rule is planned; the CI coverage guard is the regression backstop." That's a valid decision, but it creates a single point of failure: if someone changes the grep path or the log file location in CI, the guard silently stops detecting regressions. Consider adding one sentence to the ADR's Consequences section: _"If the CI guard step is modified (log path, grep pattern), update this ADR's Enforcement section."_ This makes the dependency explicit.

marcel commented

2026-05-12 09:51:46 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

Shell correctness, CI step ordering, if: conditions, artifact upload, grep usage, and log file lifecycle.

Positives

set -eo pipefail is correct. The tee pipe cannot silently swallow a non-zero exit from npm run test:coverage. This is the right way to write a multi-command CI step.

2>&1 | tee correctly captures both stdout and stderr into the log file. Coverage output from vitest typically arrives on stderr — without 2>&1, the grep would miss it.

if: always() on the guard step is critical — the check must run even when the coverage step exits 1 (which is exactly the scenario we're guarding against). Correct.

grep -F (fixed string) avoids regex interpretation of [birpc]. The [ character is a regex metacharacter; -F bypasses this entirely. Previous commits already fixed a -E / BRE issue — this PR carries that fix correctly.

2>/dev/null in the guard's grep handles the edge case where the log file doesn't exist (e.g., the coverage step died before creating it). Correct defensive coding.

Artifact upload extended to include the log file. This means CI failures are debuggable from the Gitea artifact without needing to re-run. Good.

Minor observation (non-blocking)

${{ github.run_id }} makes the log file unique per workflow run, but not per re-run attempt. If a workflow is re-triggered (same run_id, different run_attempt), the log file is overwritten by the second attempt. This is harmless — the second attempt's coverage run produces a fresh file — but ${{ github.run_id }}-${{ github.run_attempt }} would be strictly safer. Not a blocker for this PR.

The step comment "Diagnostic guard: covers the coverage run only. If npm test (above) exits 1 with a birpc error, the named pattern appears here — not there." is valuable — it correctly explains why this is not a general test gate, preventing future maintainers from expanding its scope incorrectly.

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked Shell correctness, CI step ordering, `if:` conditions, artifact upload, grep usage, and log file lifecycle. ### Positives **`set -eo pipefail` is correct.** The `tee` pipe cannot silently swallow a non-zero exit from `npm run test:coverage`. This is the right way to write a multi-command CI step. **`2>&1 | tee`** correctly captures both stdout and stderr into the log file. Coverage output from vitest typically arrives on stderr — without `2>&1`, the grep would miss it. **`if: always()`** on the guard step is critical — the check must run even when the coverage step exits 1 (which is exactly the scenario we're guarding against). Correct. **`grep -F` (fixed string)** avoids regex interpretation of `[birpc]`. The `[` character is a regex metacharacter; `-F` bypasses this entirely. Previous commits already fixed a `-E` / BRE issue — this PR carries that fix correctly. **`2>/dev/null`** in the guard's grep handles the edge case where the log file doesn't exist (e.g., the coverage step died before creating it). Correct defensive coding. **Artifact upload extended to include the log file.** This means CI failures are debuggable from the Gitea artifact without needing to re-run. Good. ### Minor observation (non-blocking) `${{ github.run_id }}` makes the log file unique per workflow run, but not per re-run attempt. If a workflow is re-triggered (same `run_id`, different `run_attempt`), the log file is overwritten by the second attempt. This is harmless — the second attempt's coverage run produces a fresh file — but `${{ github.run_id }}-${{ github.run_attempt }}` would be strictly safer. Not a blocker for this PR. The step comment _"Diagnostic guard: covers the coverage run only. If `npm test` (above) exits 1 with a birpc error, the named pattern appears here — not there."_ is valuable — it correctly explains why this is not a general test gate, preventing future maintainers from expanding its scope incorrectly.

marcel commented

2026-05-12 09:51:56 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

What I checked

PR scope alignment with issue #535, acceptance criterion measurability, decision documentation completeness, and residual open items.

Positives

The fix is precisely scoped. The root cause (dynamic imports via vi.mock factories + birpc teardown race) is correctly identified, and the solution (prop injection / libLoader parameter) is the minimal intervention that eliminates the race without touching unrelated code.

The PR description is a well-structured requirements artifact. It includes:

Root cause with mechanism explained
Concrete test plan with ✅/☐ distinction between verifiable-now vs. verifiable-post-merge
A measurable acceptance criterion (60 consecutive green CI runs on main)

ADR 012 captures the residual exceptions clearly. The table of acceptable vi.mock calls with their justification ($app/navigation etc.) directly answers "what are the boundaries of this rule?" — a question that would otherwise produce inconsistent interpretations across future PRs.

Observations

The post-merge acceptance criterion needs a tracking mechanism. The "60 consecutive green runs" criterion is measurable but only verifiable by reviewing CI history. Since Gitea doesn't auto-count consecutive successes, this requires manual verification. Recommend adding a comment to issue #535 after merge with a concrete plan: e.g., check back after X weeks and post the CI run IDs that confirm the streak.

The unchecked item [ ] 60 consecutive green workflow_dispatch CI runs is correctly marked unchecked — this is honest scope management, not a gap in the PR. Ensuring issue #535 stays open until this criterion is met is the right process.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### What I checked PR scope alignment with issue #535, acceptance criterion measurability, decision documentation completeness, and residual open items. ### Positives **The fix is precisely scoped.** The root cause (dynamic imports via `vi.mock` factories + birpc teardown race) is correctly identified, and the solution (prop injection / `libLoader` parameter) is the minimal intervention that eliminates the race without touching unrelated code. **The PR description is a well-structured requirements artifact.** It includes: - Root cause with mechanism explained - Concrete test plan with ✅/☐ distinction between verifiable-now vs. verifiable-post-merge - A measurable acceptance criterion (60 consecutive green CI runs on `main`) **ADR 012 captures the residual exceptions clearly.** The table of acceptable `vi.mock` calls with their justification (`$app/navigation` etc.) directly answers "what are the boundaries of this rule?" — a question that would otherwise produce inconsistent interpretations across future PRs. ### Observations **The post-merge acceptance criterion needs a tracking mechanism.** The "60 consecutive green runs" criterion is measurable but only verifiable by reviewing CI history. Since Gitea doesn't auto-count consecutive successes, this requires manual verification. Recommend adding a comment to issue #535 after merge with a concrete plan: e.g., check back after X weeks and post the CI run IDs that confirm the streak. **The unchecked item `[ ] 60 consecutive green workflow_dispatch CI runs` is correctly marked unchecked** — this is honest scope management, not a gap in the PR. Ensuring issue #535 stays open until this criterion is met is the right process.

marcel commented

2026-05-12 09:52:05 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What I checked

New attack surface, input propagation paths, CI script injection vectors, and any changes touching auth or session code.

Security surface analysis

No new attack surface introduced. The libLoader prop is optional and typed (LibLoader | undefined). It defaults to the real dynamic import. No user-controlled input flows into this prop — it is only set in test code, never from a form field, URL parameter, or API response.

The untrack() call prevents reactive re-initialization. This is a correctness guard (prevents the renderer from being recreated on prop change), not a security concern. Correctly placed.

CI script uses grep -F (fixed string). The string "[birpc] rpc is closed" contains [ and ], which are regex metacharacters. -F bypasses regex interpretation entirely — correct. A previous commit already fixed a BRE escaping issue; this PR carries the safe form.

2>/dev/null on the grep prevents log-file-not-found from being treated as a passing state. If the log file is absent (coverage step died), grep exits non-zero with an error message — without 2>/dev/null that error would appear in CI output but the if check still functions correctly because the if block only fires on grep exit code 0. Safe.

No authentication, authorization, backend, or session code touched. OWASP Top 10 checklist: not applicable to this PR scope.

Nothing to flag.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### What I checked New attack surface, input propagation paths, CI script injection vectors, and any changes touching auth or session code. ### Security surface analysis **No new attack surface introduced.** The `libLoader` prop is optional and typed (`LibLoader | undefined`). It defaults to the real dynamic import. No user-controlled input flows into this prop — it is only set in test code, never from a form field, URL parameter, or API response. **The `untrack()` call prevents reactive re-initialization.** This is a correctness guard (prevents the renderer from being recreated on prop change), not a security concern. Correctly placed. **CI script uses `grep -F` (fixed string).** The string `"[birpc] rpc is closed"` contains `[` and `]`, which are regex metacharacters. `-F` bypasses regex interpretation entirely — correct. A previous commit already fixed a BRE escaping issue; this PR carries the safe form. **`2>/dev/null` on the grep prevents log-file-not-found from being treated as a passing state.** If the log file is absent (coverage step died), grep exits non-zero with an error message — without `2>/dev/null` that error would appear in CI output but the `if` check still functions correctly because the `if` block only fires on `grep` exit code 0. Safe. **No authentication, authorization, backend, or session code touched.** OWASP Top 10 checklist: not applicable to this PR scope. Nothing to flag.

marcel commented

2026-05-12 09:52:19 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

What I checked

Test coverage for new behaviour, factory function discipline, CI gate quality, error path coverage, and test naming.

Positives

Three new tests with distinct behavioural goals in usePdfRenderer.svelte.test.ts:

'calls injected libLoader during init and sets pdfjsReady' — verifies the injection contract and the reactive state transition
'leaves pdfjsReady false when libLoader rejects' — covers the error path; critical since a failed load should not silently leave the renderer in an indeterminate state
'init() is idempotent — libLoader called only once on repeated calls' — directly tests the if (pdfjsReady) return; guard

All three names read as sentences describing behaviour. ✅

Existing three browser-mode specs in PdfViewer.svelte.spec.ts are updated to use makeFakeLibLoader() — no orphaned tests, no vi.mock bypass remaining.

makeFakePdfjsLib() and makeFakeLibLoader() factory functions are clean and follow the project's factory discipline.

afterEach(cleanup) is present in PdfViewer.svelte.spec.ts. ✅

CI guard step (Assert no birpc teardown race in coverage run) is a meaningful quality gate addition — it will catch regressions that only manifest in the coverage run, which is exactly the scenario that triggered #535.

Concerns

Duplicated fakePdfjs construction in usePdfRenderer.svelte.test.ts (non-blocking). The inline object literal for fakePdfjs is copy-pasted in two separate tests. PdfViewer.svelte.spec.ts correctly extracts this to makeFakePdfjsLib(). The unit test file should do the same:

// Add at top of usePdfRenderer.svelte.test.ts
function makeFakePdfjsLib() {
  return {
    GlobalWorkerOptions: { workerSrc: '' },
    getDocument: vi.fn(),
    TextLayer: class {}
  } as unknown as typeof import('pdfjs-dist');
}

If the pdfjs-dist type shape changes, only one place needs updating instead of two.

No browser-mode test for the libLoader rejection path in PdfViewer.svelte.spec.ts (non-blocking). The unit test in usePdfRenderer.svelte.test.ts covers this path at the hook level. Since the component just calls renderer.init() from onMount, covering the rejection at the hook level is sufficient. A comment in the spec file noting this delegation would prevent a future reviewer from wondering if the error path is tested.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** ### What I checked Test coverage for new behaviour, factory function discipline, CI gate quality, error path coverage, and test naming. ### Positives **Three new tests with distinct behavioural goals in `usePdfRenderer.svelte.test.ts`:** - `'calls injected libLoader during init and sets pdfjsReady'` — verifies the injection contract and the reactive state transition - `'leaves pdfjsReady false when libLoader rejects'` — covers the error path; critical since a failed load should not silently leave the renderer in an indeterminate state - `'init() is idempotent — libLoader called only once on repeated calls'` — directly tests the `if (pdfjsReady) return;` guard All three names read as sentences describing behaviour. ✅ **Existing three browser-mode specs in `PdfViewer.svelte.spec.ts` are updated** to use `makeFakeLibLoader()` — no orphaned tests, no vi.mock bypass remaining. **`makeFakePdfjsLib()` and `makeFakeLibLoader()` factory functions** are clean and follow the project's factory discipline. **`afterEach(cleanup)` is present** in `PdfViewer.svelte.spec.ts`. ✅ **CI guard step** (`Assert no birpc teardown race in coverage run`) is a meaningful quality gate addition — it will catch regressions that only manifest in the coverage run, which is exactly the scenario that triggered #535. ### Concerns **Duplicated `fakePdfjs` construction in `usePdfRenderer.svelte.test.ts` (non-blocking).** The inline object literal for `fakePdfjs` is copy-pasted in two separate tests. `PdfViewer.svelte.spec.ts` correctly extracts this to `makeFakePdfjsLib()`. The unit test file should do the same: ```typescript // Add at top of usePdfRenderer.svelte.test.ts function makeFakePdfjsLib() { return { GlobalWorkerOptions: { workerSrc: '' }, getDocument: vi.fn(), TextLayer: class {} } as unknown as typeof import('pdfjs-dist'); } ``` If the `pdfjs-dist` type shape changes, only one place needs updating instead of two. **No browser-mode test for the `libLoader` rejection path in `PdfViewer.svelte.spec.ts` (non-blocking).** The unit test in `usePdfRenderer.svelte.test.ts` covers this path at the hook level. Since the component just calls `renderer.init()` from `onMount`, covering the rejection at the hook level is sufficient. A comment in the spec file noting this delegation would prevent a future reviewer from wondering if the error path is tested.

marcel commented

2026-05-12 09:52:23 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

What I checked

Visual changes to PdfViewer.svelte, rendered output, accessibility attributes, touch targets, and brand token usage.

Assessment

This PR makes no user-facing changes.

The only modification to PdfViewer.svelte is:

Import untrack and LibLoader (internal, not rendered)
Add libLoader = undefined to the props destructuring (not rendered)
Wrap createPdfRenderer() in untrack() (not rendered)

The libLoader prop is a test seam — it affects how the PDF rendering library is loaded during tests, but the rendered output (controls, page counter, canvas, text layer) is identical to the previous version in every production scenario.

Accessibility unchanged. No ARIA attributes added or removed. No interactive elements modified. No focus management affected.

Brand tokens unchanged. No new Tailwind classes introduced in the component template.

LGTM.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** ### What I checked Visual changes to `PdfViewer.svelte`, rendered output, accessibility attributes, touch targets, and brand token usage. ### Assessment This PR makes no user-facing changes. The only modification to `PdfViewer.svelte` is: 1. Import `untrack` and `LibLoader` (internal, not rendered) 2. Add `libLoader = undefined` to the props destructuring (not rendered) 3. Wrap `createPdfRenderer()` in `untrack()` (not rendered) The `libLoader` prop is a test seam — it affects how the PDF rendering library is loaded during tests, but the rendered output (controls, page counter, canvas, text layer) is identical to the previous version in every production scenario. **Accessibility unchanged.** No ARIA attributes added or removed. No interactive elements modified. No focus management affected. **Brand tokens unchanged.** No new Tailwind classes introduced in the component template. LGTM.

marcel added 18 commits 2026-05-12 09:56:18 +02:00

test(pdf-renderer): inject libLoader into createPdfRenderer to eliminate vi.mock factories 0142256b3c

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>

fix(pdf-viewer): replace vi.mock(pdfjs-dist) with injected libLoader prop b00ffc358f

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>

ci(guard): fail unit-tests job if [birpc] rpc is closed appears in coverage run 9c9bb205bd

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>

docs(adr): 012 — browser-mode test mocking strategy 9e7ec5a3c8

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>

refactor(test): move PdfViewer import to top and annotate partial fake cast dd9f6f56c5

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

ci(coverage): simplify coverage step and pin shell to bash 18baba49e6

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

refactor(pdf-viewer): export LibLoader type and update callers c6b15e7176

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>

docs(pdf-viewer): comment untrack invariant on renderer init 3e95bd4e70

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>

test(pdf-renderer): document libLoader rejection leaves pdfjsReady false cf5433694e

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>

ci(coverage): harden coverage guard step 7b921bfa0a

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

test(pdf-renderer): assert init() re-throws when libLoader rejects fdc508fa91

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

refactor(test): remove what-comment from makeFakePdfjsLib db9c6df19a

The name already implies it's a fake; the comment described what, not why.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ci(coverage): include coverage log in artifact upload d8275eb3c6

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>

ci(coverage): use grep -F for birpc guard to avoid BRE escaping 8421ade762

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

test(pdf-renderer): guard init() against repeated calls — libLoader must fire once b56607ed93

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>

refactor(test): convert TextLayerMock to class syntax in PdfViewer spec 162de5f354

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>

ci(coverage): document that birpc guard covers coverage run only 3aab522f98

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>

docs(adr-012): add enforcement note and libLoader revisit signal

CI / Unit & Component Tests (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (push) Successful in 4m16s

Details

CI / OCR Service Tests (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (pull_request) Has been cancelled

Details

CI / fail2ban Regex (pull_request) Has been cancelled

Details

CI / Compose Bucket Idempotency (pull_request) Has been cancelled

Details

CI / Unit & Component Tests (push) Failing after 1m51s

Details

CI / OCR Service Tests (push) Successful in 16s

Details

CI / fail2ban Regex (push) Successful in 39s

Details

CI / Compose Bucket Idempotency (push) Failing after 11s

Details

efece12f29

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

marcel force-pushed feat/issue-535-birpc-teardown-race from 3d9e10f3e7 to efece12f29

2026-05-12 09:56:18 +02:00

Compare

marcel referenced this issue from a commit

2026-05-12 09:56:18 +02:00

test(pdf-renderer): guard init() against repeated calls — libLoader must fire once

marcel referenced this issue from a commit

2026-05-12 09:56:18 +02:00

refactor(test): convert TextLayerMock to class syntax in PdfViewer spec

marcel referenced this issue from a commit

2026-05-12 09:56:18 +02:00

ci(coverage): document that birpc guard covers coverage run only

marcel referenced this issue from a commit

2026-05-12 09:56:18 +02:00

docs(adr-012): add enforcement note and libLoader revisit signal

marcel merged commit 7f07180c71 into main

2026-05-12 09:57:30 +02:00

marcel deleted branch feat/issue-535-birpc-teardown-race

2026-05-12 09:57:30 +02:00

marcel referenced this issue from a commit

2026-05-12 09:57:32 +02:00

test(pdf-renderer): guard init() against repeated calls — libLoader must fire once

marcel referenced this issue from a commit

2026-05-12 09:57:32 +02:00

refactor(test): convert TextLayerMock to class syntax in PdfViewer spec

marcel referenced this issue from a commit

2026-05-12 09:57:32 +02:00

ci(coverage): document that birpc guard covers coverage run only

marcel referenced this issue from a commit

2026-05-12 09:57:32 +02:00

docs(adr-012): add enforcement note and libLoader revisit signal

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#536

fix(#535): eliminate vi.mock(pdfjs-dist) birpc teardown race via libLoader injection #536

Summary

Root cause

Test plan

👨‍💻 Felix Brandt — Senior Fullstack Developer

Suggestions (no blockers)

🏛️ Markus Keller — Application Architect

Architecture assessment

Documentation checklist

One note (non-blocking)

🔧 Tobias Wendt — DevOps & Platform Engineer

Concern — exit ${PIPESTATUS[0]} is unreachable under pipefail

Minor — add shell: bash for explicitness

What's good

🔐 Nora "NullX" Steiner — Application Security Engineer

🧪 Sara Holt — QA Engineer & Test Strategist

Blocker — misplaced import PdfViewer in PdfViewer.svelte.spec.ts

Suggestion — add error-path test for libLoader rejection

What's done well

🎨 Leonie Voss — UX Designer & Accessibility Strategist

📋 Elicit — Requirements Engineer

Issue #535 traceability

Test plan coverage

One observation (non-blocking)

ADR 012 quality

Review feedback addressed

3bb940a2 — refactor(test): move PdfViewer import to top and annotate partial fake cast

11c61c8a — ci(coverage): simplify coverage step and pin shell to bash

👨‍💻 Felix Brandt — Senior Fullstack Developer

Blockers

Suggestions

🔐 Nora "NullX" Steiner — Application Security Engineer

What I checked

🧪 Sara Holt — Senior QA Engineer

Blockers

Suggestions

🎨 Leonie Voss — UX Designer & Accessibility Strategist

What I verified

🏗️ Markus Keller — Senior Application Architect

What I checked against the doc-update table

Suggestions (not blockers)

🛠️ Tobias Wendt — DevOps & Platform Engineer

What works well

Suggestions (not blockers)

📋 Elicit — Requirements Engineer

Issue traceability

Acceptance criterion quality

NFR coverage

🏗️ Markus Keller — Senior Application Architect

What I checked

Findings

Blockers

Suggestions

👨‍💻 Felix Brandt — Senior Fullstack Developer

What I checked

Findings

Blockers

Suggestions

🔧 Tobias Wendt — DevOps & Platform Engineer

What I checked

Findings

Blockers

Suggestions

📋 Elicit — Requirements Engineer

What I checked

Findings

Blockers

Suggestions

🔐 Nora "NullX" Steiner — Application Security Engineer

What I checked

Findings

Blockers

Suggestions

🧪 Sara Holt — Senior QA Engineer

What I checked

Findings

Blockers

Suggestions

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate

What I checked

Concern — `exit ${PIPESTATUS[0]}` is unreachable under pipefail

Minor — add `shell: bash` for explicitness

Blocker — misplaced `import PdfViewer` in `PdfViewer.svelte.spec.ts`

Suggestion — add error-path test for `libLoader` rejection

`3bb940a2` — refactor(test): move PdfViewer import to top and annotate partial fake cast

`11c61c8a` — ci(coverage): simplify coverage step and pin shell to bash

`e529f9f7` — refactor(pdf-viewer): export LibLoader type and update callers

`b8d9c0e9` — docs(pdf-viewer): comment untrack invariant on renderer init

`d8496498` — test(pdf-renderer): document libLoader rejection leaves pdfjsReady false

`e418e884` — ci(coverage): harden coverage guard step

`2163cd96` — test(pdf-renderer): assert init() re-throws when libLoader rejects

`58eb6e27` — refactor(test): remove what-comment from makeFakePdfjsLib

`017b404f` — ci(coverage): include coverage log in artifact upload

`48b7e0f3` — ci(coverage): use grep -F for birpc guard to avoid BRE escaping

Pattern proliferation: ADR 012 doesn't say when to revisit `LibLoader`'s home

Pre-existing gap surfaced by the explicit `pdfjsReady` flag

`0b3ce838` — test(pdf-renderer): guard init() against repeated calls — libLoader must fire once

`185c754e` — refactor(test): convert TextLayerMock to class syntax in PdfViewer spec

`8396d82c` — ci(coverage): document that birpc guard covers coverage run only

`3d9e10f3` — docs(adr-012): add enforcement note and libLoader revisit signal