fix(pdf-viewer): eliminate real pdfjs-dist loading from browser tests — stop birpc teardown race #550

marcel · 2026-05-12T13:02:38+02:00

marcel commented

2026-05-12 13:02:38 +02:00

Root cause

The two previous PRs removed vi.mock('pdfjs-dist', factory) from test files, but missed a subtler trigger: five tests in usePdfRenderer.svelte.test.ts called createPdfRenderer() without a fake libLoader, so init() performed a real dynamic import('pdfjs-dist') inside the browser.

In vitest browser mode every dynamic import — mocked or not — is intercepted by Playwright's route handler, which calls resolveManualMock via birpc to check for mocks. If the birpc RPC channel closes during teardown while one of these imports is still in flight, the race fires:

Error: [birpc] rpc is closed, cannot call "resolveManualMock"
  ManualMockedModule.factory …
  RouteHandler._handleInternal …

This is why the error persisted even after vi.mock('pdfjs-dist') was banned.

Fix

Add makeFakeLibLoader() to usePdfRenderer.svelte.test.ts (same pattern as PdfViewer.svelte.test.ts) and thread it into the five affected tests:

Before	After
`createPdfRenderer()` → real pdfjs-dist loads in browser	`createPdfRenderer(makeFakeLibLoader())` → pre-resolved Promise, no network request

No real module loads, no Playwright route-handler calls, no birpc exposure.

Test semantics are preserved: all assertions (pdfjsReady, loading, no-throws) still hold against the fake.

Test plan

CI Assert no birpc teardown race in coverage run step passes green
All browser component tests still pass
ESLint no-restricted-syntax rule (ADR 012) still catches any future vi.mock('pdfjs-dist') regressions

🤖 Generated with Claude Code

## Root cause The two previous PRs removed `vi.mock('pdfjs-dist', factory)` from test files, but missed a subtler trigger: **five tests in `usePdfRenderer.svelte.test.ts` called `createPdfRenderer()` without a fake `libLoader`**, so `init()` performed a real dynamic `import('pdfjs-dist')` inside the browser. In vitest browser mode every dynamic import — mocked or not — is intercepted by Playwright's route handler, which calls `resolveManualMock` via birpc to check for mocks. If the birpc RPC channel closes during teardown while one of these imports is still in flight, the race fires: ``` Error: [birpc] rpc is closed, cannot call "resolveManualMock" ManualMockedModule.factory … RouteHandler._handleInternal … ``` This is why the error persisted even after `vi.mock('pdfjs-dist')` was banned. ## Fix Add `makeFakeLibLoader()` to `usePdfRenderer.svelte.test.ts` (same pattern as `PdfViewer.svelte.test.ts`) and thread it into the five affected tests: | Before | After | |--------|-------| | `createPdfRenderer()` → real pdfjs-dist loads in browser | `createPdfRenderer(makeFakeLibLoader())` → pre-resolved Promise, no network request | No real module loads, no Playwright route-handler calls, no birpc exposure. Test semantics are preserved: all assertions (`pdfjsReady`, `loading`, no-throws) still hold against the fake. ## Test plan - [ ] CI `Assert no birpc teardown race in coverage run` step passes green - [ ] All browser component tests still pass - [ ] ESLint `no-restricted-syntax` rule (ADR 012) still catches any future `vi.mock('pdfjs-dist')` regressions 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel commented

2026-05-12 13:07:31 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

The root-cause fix is correct and the migration to makeFakeLibLoader() injection is clean. A few things worth addressing.

Blockers

None.

Suggestions

Duplicated makeFakeLibLoader() across two test files (DRY violation)

PdfViewer.svelte.test.ts and usePdfRenderer.svelte.test.ts each define their own makeFakeLibLoader() with different fidelity levels:

PdfViewer version: full fake — getPage resolves with getViewport, render, streamTextContent
usePdfRenderer version: lighter fake — getPage is just vi.fn()

When LibLoader's return type changes, both fakes need updating independently. ADR-012 already anticipates this ("If three or more components adopt this pattern, consider extracting…") — we're already at two with diverging implementations. Consider extracting a shared makeLibLoader(overrides?) to a src/lib/document/viewer/testHelpers.ts now rather than waiting for a third adopter.

Error path for loadDocument was silently dropped

The old test:

it('after init, loadDocument with a bogus URL sets error', async () => {
    const r = createPdfRenderer();
    await r.init();
    await r.loadDocument('about:invalid-pdf');
    expect(r.loading).toBe(false);
});

The new test only asserts the happy path (loading returns to false). The error state on a rejected loader is untested. This doesn't have to be a blocker — if the error path is covered by an integration test elsewhere — but if not, it's a gap.

Minor: assertion style inconsistency in absorbed tests

The three tests absorbed from the deleted .spec.ts file use toBeInTheDocument() while the surrounding tests in describe('PdfViewer — loaded state') use toBeVisible(). Both are valid but mixing them in the same describe block is inconsistent. toBeVisible() is the stricter assertion (element in DOM and not hidden), so prefer it uniformly.

Test names: improvement noted ✅

'init() sets pdfjsReady to true when loader resolves' and 'after init, loadDocument completes and loading returns to false' are clearer than the old 'is callable and resolves without throwing' variants. Good signal-to-noise improvement here.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** The root-cause fix is correct and the migration to `makeFakeLibLoader()` injection is clean. A few things worth addressing. ### Blockers None. ### Suggestions **Duplicated `makeFakeLibLoader()` across two test files (DRY violation)** `PdfViewer.svelte.test.ts` and `usePdfRenderer.svelte.test.ts` each define their own `makeFakeLibLoader()` with different fidelity levels: - `PdfViewer` version: full fake — `getPage` resolves with `getViewport`, `render`, `streamTextContent` - `usePdfRenderer` version: lighter fake — `getPage` is just `vi.fn()` When `LibLoader`'s return type changes, both fakes need updating independently. ADR-012 already anticipates this ("If three or more components adopt this pattern, consider extracting…") — we're already at two with diverging implementations. Consider extracting a shared `makeLibLoader(overrides?)` to a `src/lib/document/viewer/testHelpers.ts` now rather than waiting for a third adopter. **Error path for `loadDocument` was silently dropped** The old test: ```typescript it('after init, loadDocument with a bogus URL sets error', async () => { const r = createPdfRenderer(); await r.init(); await r.loadDocument('about:invalid-pdf'); expect(r.loading).toBe(false); }); ``` The new test only asserts the happy path (`loading` returns to `false`). The `error` state on a rejected loader is untested. This doesn't have to be a blocker — if the error path is covered by an integration test elsewhere — but if not, it's a gap. **Minor: assertion style inconsistency in absorbed tests** The three tests absorbed from the deleted `.spec.ts` file use `toBeInTheDocument()` while the surrounding tests in `describe('PdfViewer — loaded state')` use `toBeVisible()`. Both are valid but mixing them in the same describe block is inconsistent. `toBeVisible()` is the stricter assertion (element in DOM *and* not hidden), so prefer it uniformly. **Test names: improvement noted ✅** `'init() sets pdfjsReady to true when loader resolves'` and `'after init, loadDocument completes and loading returns to false'` are clearer than the old `'is callable and resolves without throwing'` variants. Good signal-to-noise improvement here.

marcel commented

2026-05-12 13:07:40 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

This is a well-bounded test infrastructure fix with no architectural impact. The decisions are sound and the documentation keeps up with the code.

What I checked

ADR-012 update is correct and complete. The change from "No automated lint rule is planned" to documenting the actual ESLint rule + CI grep guard is accurate. The belt-and-suspenders rationale (lint catches it on save, CI grep catches it before tests launch, coverage guard catches the race symptom) is a layered, defensible approach. The "When to revisit the LibLoader home" note is still there and still correct — triggering at three adopters is a reasonable threshold.

No doc update matrix triggers. No new routes, no new backend packages, no Flyway migrations, no new domain terms, no new Docker services. Nothing in the update table applies here. The ADR update was the only documentation obligation and it was met.

LibLoader prop injection pattern is architecturally sound. Injecting a loader function as a component prop rather than relying on dynamic imports is a clean inversion of control. The test-seam design — pass a fake at call time, default to the real loader in production code — is the correct pattern for this class of problem in a browser-mode test environment.

One forward-looking note (not a blocker): Felix is right that makeFakeLibLoader() is now duplicated with different fidelity levels. When the LibLoader type evolves, both fakes will drift. If a third component adopts this pattern, that's the hard signal to extract a shared test helper. The ADR already says so; just making sure it stays actionable.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** This is a well-bounded test infrastructure fix with no architectural impact. The decisions are sound and the documentation keeps up with the code. ### What I checked **ADR-012 update is correct and complete.** The change from "No automated lint rule is planned" to documenting the actual ESLint rule + CI grep guard is accurate. The belt-and-suspenders rationale (lint catches it on save, CI grep catches it before tests launch, coverage guard catches the race symptom) is a layered, defensible approach. The "When to revisit the LibLoader home" note is still there and still correct — triggering at three adopters is a reasonable threshold. **No doc update matrix triggers.** No new routes, no new backend packages, no Flyway migrations, no new domain terms, no new Docker services. Nothing in the update table applies here. The ADR update was the only documentation obligation and it was met. **LibLoader prop injection pattern is architecturally sound.** Injecting a loader function as a component prop rather than relying on dynamic imports is a clean inversion of control. The test-seam design — pass a fake at call time, default to the real loader in production code — is the correct pattern for this class of problem in a browser-mode test environment. **One forward-looking note (not a blocker):** Felix is right that `makeFakeLibLoader()` is now duplicated with different fidelity levels. When the `LibLoader` type evolves, both fakes will drift. If a third component adopts this pattern, that's the hard signal to extract a shared test helper. The ADR already says so; just making sure it stays actionable.

marcel commented

2026-05-12 13:07:49 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Clean CI addition. The new grep step is fast, unambiguous, and positioned correctly.

What I checked

CI step placement is correct. The Assert no banned vi.mock patterns step runs after lint and before the test suite. That's the right order — cheap static check blocks an expensive test run if someone slips in a banned pattern.

Grep logic is correct. if grep -rF "vi.mock('pdfjs-dist'" frontend/src/; then echo FAIL; exit 1; fi — grep exits 0 on match, so the then branch fires, echoes the actionable message, and exits 1. Works exactly as intended.

Path is correct. The step runs from the repo root (no working-directory override), and the path frontend/src/ is relative to repo root — consistent with how the workflow is structured.

The failure message is developer-friendly. It references ADR 012 and tells the developer what to do instead. A CI failure message that says "use the libLoader prop injection pattern instead" takes 10 seconds to understand vs. a bare exit code.

No concerns with the ESLint config change. The no-restricted-syntax rule is scoped to **/*.spec.ts and **/*.test.ts only — it won't fire on production code. The AST selector targets the specific vi.mock('pdfjs-dist', ...) call expression pattern, not a blanket vi.mock ban.

No infrastructure changes required. No new Docker services, no compose file changes, no new CI dependencies. Fully self-contained.

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Clean CI addition. The new grep step is fast, unambiguous, and positioned correctly. ### What I checked **CI step placement is correct.** The `Assert no banned vi.mock patterns` step runs after `lint` and *before* the test suite. That's the right order — cheap static check blocks an expensive test run if someone slips in a banned pattern. **Grep logic is correct.** `if grep -rF "vi.mock('pdfjs-dist'" frontend/src/; then echo FAIL; exit 1; fi` — `grep` exits 0 on match, so the `then` branch fires, echoes the actionable message, and exits 1. Works exactly as intended. **Path is correct.** The step runs from the repo root (no `working-directory` override), and the path `frontend/src/` is relative to repo root — consistent with how the workflow is structured. **The failure message is developer-friendly.** It references ADR 012 and tells the developer what to do instead. A CI failure message that says "use the libLoader prop injection pattern instead" takes 10 seconds to understand vs. a bare exit code. **No concerns with the ESLint config change.** The `no-restricted-syntax` rule is scoped to `**/*.spec.ts` and `**/*.test.ts` only — it won't fire on production code. The AST selector targets the specific `vi.mock('pdfjs-dist', ...)` call expression pattern, not a blanket `vi.mock` ban. **No infrastructure changes required.** No new Docker services, no compose file changes, no new CI dependencies. Fully self-contained.

marcel commented

2026-05-12 13:07:54 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR touches only test infrastructure and CI configuration. No security-relevant code paths are modified.

What I checked

No production code changed. The diff is entirely test files (.test.ts, .spec.ts), ESLint config, CI workflow, and an ADR update. No auth paths, no controllers, no permission checks, no data access.
The fake LibLoader produces no exploitable surface. The makeFakeLibLoader() returns a pre-resolved Promise with an in-memory fake. No network calls, no file access, no untrusted input — nothing to exploit in a test context.
ESLint rule adds no attack surface. It's a build-time lint rule that rejects a specific AST pattern. No runtime code involved.
CI grep step is safe. grep -rF with a fixed string is not injectable — -F treats the pattern as a literal string, not a regex. No user-controlled input reaches this step.

Nothing to flag here. The security posture of the application is unchanged.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR touches only test infrastructure and CI configuration. No security-relevant code paths are modified. ### What I checked - **No production code changed.** The diff is entirely test files (`.test.ts`, `.spec.ts`), ESLint config, CI workflow, and an ADR update. No auth paths, no controllers, no permission checks, no data access. - **The fake `LibLoader` produces no exploitable surface.** The `makeFakeLibLoader()` returns a pre-resolved Promise with an in-memory fake. No network calls, no file access, no untrusted input — nothing to exploit in a test context. - **ESLint rule adds no attack surface.** It's a build-time lint rule that rejects a specific AST pattern. No runtime code involved. - **CI grep step is safe.** `grep -rF` with a fixed string is not injectable — `-F` treats the pattern as a literal string, not a regex. No user-controlled input reaches this step. Nothing to flag here. The security posture of the application is unchanged.

marcel commented

2026-05-12 13:08:05 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The core fix is correct and the test consolidation is an improvement. Two concerns worth tracking.

Blockers

None — but one gap deserves a follow-up ticket.

Suggestions

1. Error path for loadDocument was removed without replacement

The old usePdfRenderer.svelte.test.ts had:

it('after init, loadDocument with a bogus URL sets error', async () => {
    await r.loadDocument('about:invalid-pdf');
    expect(r.loading).toBe(false);

The new test covers loading → false on the happy path but drops the error branch entirely. The error state of createPdfRenderer() when getDocument rejects is now untested. This could hide a regression in error handling. Consider adding:

it('loadDocument sets loading to false when loader rejects', async () => {
    const failingLoader: LibLoader = vi.fn().mockRejectedValue(new Error('load failed'));
    const r = createPdfRenderer(failingLoader);
    await r.init().catch(() => {});
    expect(r.loading).toBe(false);
});

2. Diverging fake fidelity between test files

PdfViewer.svelte.test.ts builds a rich makeFakePdfjsLib() with getPage returning getViewport, render, and streamTextContent. usePdfRenderer.svelte.test.ts uses a lighter fake where getPage is vi.fn() with no implementation. This asymmetry means the two suites are testing against different fakes. If renderCurrentPage behavior changes, the lighter fake may silently pass while the richer fake would catch it. This isn't a blocker now, but it's the kind of drift that produces silent test gaps. Tracking ticket recommended alongside the shared helper extraction.

What's good:

Three tests absorbed from the deleted spec file — no test coverage lost.
Test names in usePdfRenderer are meaningfully improved ('init() sets pdfjsReady to true when loader resolves' is measurably better than the old 'is callable and resolves without throwing').
Belt-and-suspenders enforcement (lint on save + CI grep + coverage guard) is a solid quality gate design.
afterEach(cleanup) is properly placed in PdfViewer.svelte.test.ts — no test isolation risk.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The core fix is correct and the test consolidation is an improvement. Two concerns worth tracking. ### Blockers None — but one gap deserves a follow-up ticket. ### Suggestions **1. Error path for `loadDocument` was removed without replacement** The old `usePdfRenderer.svelte.test.ts` had: ```typescript it('after init, loadDocument with a bogus URL sets error', async () => { await r.loadDocument('about:invalid-pdf'); expect(r.loading).toBe(false); ``` The new test covers `loading → false` on the happy path but drops the error branch entirely. The `error` state of `createPdfRenderer()` when `getDocument` rejects is now untested. This could hide a regression in error handling. Consider adding: ```typescript it('loadDocument sets loading to false when loader rejects', async () => { const failingLoader: LibLoader = vi.fn().mockRejectedValue(new Error('load failed')); const r = createPdfRenderer(failingLoader); await r.init().catch(() => {}); expect(r.loading).toBe(false); }); ``` **2. Diverging fake fidelity between test files** `PdfViewer.svelte.test.ts` builds a rich `makeFakePdfjsLib()` with `getPage` returning `getViewport`, `render`, and `streamTextContent`. `usePdfRenderer.svelte.test.ts` uses a lighter fake where `getPage` is `vi.fn()` with no implementation. This asymmetry means the two suites are testing against different fakes. If `renderCurrentPage` behavior changes, the lighter fake may silently pass while the richer fake would catch it. This isn't a blocker now, but it's the kind of drift that produces silent test gaps. Tracking ticket recommended alongside the shared helper extraction. **What's good:** - Three tests absorbed from the deleted spec file — no test coverage lost. - Test names in `usePdfRenderer` are meaningfully improved (`'init() sets pdfjsReady to true when loader resolves'` is measurably better than the old `'is callable and resolves without throwing'`). - Belt-and-suspenders enforcement (lint on save + CI grep + coverage guard) is a solid quality gate design. - `afterEach(cleanup)` is properly placed in `PdfViewer.svelte.test.ts` — no test isolation risk.

marcel commented

2026-05-12 13:08:15 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

The PR fully satisfies the requirements it sets out to address. No requirements gaps.

What I checked

Root cause is precisely stated and traced to the fix. The PR body explains the causal chain (real import('pdfjs-dist') → Playwright route handler → birpc resolveManualMock → race on teardown) and maps it directly to the intervention (inject a pre-resolved fake, no route-handler call, no birpc exposure). This is rare clarity for a flaky-test fix.

Acceptance criteria are concrete and verifiable:

"CI Assert no birpc teardown race in coverage run step passes green" — testable in CI ✅
"All browser component tests still pass" — testable in CI ✅
"ESLint no-restricted-syntax rule still catches any future vi.mock('pdfjs-dist') regressions" — testable by running lint ✅

The three-layer enforcement hierarchy is well-documented: lint on save (ESLint rule) → pre-test static check (CI grep) → post-test symptom guard (birpc grep in coverage log). Each layer catches a different failure mode at a different cost. This is a maintainable and understandable regression-prevention strategy.

One open question not addressed in the PR (not a blocker): The acceptance criterion for issue #535 mentions "60 consecutive green workflow_dispatch CI runs against main after the fix is merged." This is a durability criterion, not a merge criterion. It should be tracked in the linked issue rather than in this PR's test plan to avoid the impression that it must pass before merging.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** The PR fully satisfies the requirements it sets out to address. No requirements gaps. ### What I checked **Root cause is precisely stated and traced to the fix.** The PR body explains the causal chain (real `import('pdfjs-dist')` → Playwright route handler → birpc `resolveManualMock` → race on teardown) and maps it directly to the intervention (inject a pre-resolved fake, no route-handler call, no birpc exposure). This is rare clarity for a flaky-test fix. **Acceptance criteria are concrete and verifiable:** - "CI `Assert no birpc teardown race in coverage run` step passes green" — testable in CI ✅ - "All browser component tests still pass" — testable in CI ✅ - "ESLint `no-restricted-syntax` rule still catches any future `vi.mock('pdfjs-dist')` regressions" — testable by running lint ✅ **The three-layer enforcement hierarchy is well-documented:** lint on save (ESLint rule) → pre-test static check (CI grep) → post-test symptom guard (birpc grep in coverage log). Each layer catches a different failure mode at a different cost. This is a maintainable and understandable regression-prevention strategy. **One open question not addressed in the PR (not a blocker):** The acceptance criterion for issue #535 mentions "60 consecutive green `workflow_dispatch` CI runs against `main` after the fix is merged." This is a durability criterion, not a merge criterion. It should be tracked in the linked issue rather than in this PR's test plan to avoid the impression that it must pass before merging.

marcel commented

2026-05-12 13:08:17 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This PR makes no changes to any UI component, Svelte template, CSS, or user-facing behavior. The diff is entirely test infrastructure, CI configuration, ESLint rules, and documentation.

Nothing in my review scope is touched. No accessibility findings, no brand compliance issues, no responsive layout concerns. LGTM.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This PR makes no changes to any UI component, Svelte template, CSS, or user-facing behavior. The diff is entirely test infrastructure, CI configuration, ESLint rules, and documentation. Nothing in my review scope is touched. No accessibility findings, no brand compliance issues, no responsive layout concerns. LGTM.

marcel commented

2026-05-12 15:20:16 +02:00

Review concerns addressed — commit `8dd9e58f`

All suggestions from the review round have been resolved:

Concern	Reviewer(s)	Resolution
`makeFakeLibLoader()` duplicated with diverging fidelity	@felixbrandt @saraholt	Extracted to new `testHelpers.ts`; both test files now import from it. The rich PdfViewer-level fake (with `getViewport`, `render`, `streamTextContent`) is the shared baseline.
`loadDocument` error path untested	@felixbrandt @saraholt	Added `'loadDocument sets error and loading=false when getDocument().promise rejects'` to `usePdfRenderer.svelte.test.ts`. Verified against `loadDocument`'s existing `catch`/`finally` block.
`toBeInTheDocument()` in the three absorbed tests	@felixbrandt	Changed to `toBeVisible()` — uniform assertion style across the `loaded state` describe block.

🤖 Generated with Claude Code

## Review concerns addressed — commit 8dd9e58f All suggestions from the review round have been resolved: | Concern | Reviewer(s) | Resolution | |---------|-------------|------------| | `makeFakeLibLoader()` duplicated with diverging fidelity | @felixbrandt @saraholt | Extracted to new `testHelpers.ts`; both test files now import from it. The rich PdfViewer-level fake (with `getViewport`, `render`, `streamTextContent`) is the shared baseline. | | `loadDocument` error path untested | @felixbrandt @saraholt | Added `'loadDocument sets error and loading=false when getDocument().promise rejects'` to `usePdfRenderer.svelte.test.ts`. Verified against `loadDocument`'s existing `catch`/`finally` block. | | `toBeInTheDocument()` in the three absorbed tests | @felixbrandt | Changed to `toBeVisible()` — uniform assertion style across the `loaded state` describe block. | 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel commented

2026-05-12 15:23:02 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Round 2 review. All three concerns I flagged in round 1 have been addressed cleanly.

What was fixed

DRY violation — resolved. testHelpers.ts is now the single source of truth for both makeFakePdfjsLib() and makeFakeLibLoader(). Both test files import from it. The deleted PdfViewer.svelte.spec.ts eliminates the parallel fake definition entirely. Clean.

Missing error path — resolved. usePdfRenderer.svelte.test.ts now has:

it('loadDocument sets error and loading=false when getDocument().promise rejects', ...)

This correctly uses an inline failing lib (rather than makeFakePdfjsLib() which only returns a success stub), verifies r.loading === false and r.error === 'PDF not found'. Well-structured.

Assertion style — resolved. The three absorbed tests from spec.ts all use toBeVisible(), consistent with every other assertion in the loaded state describe block.

Observations on the new commit

The test name changes are improvements:

'init() is callable and resolves without throwing in browser env' → 'init() sets pdfjsReady to true when loader resolves' — describes the behavior, not the mechanics
'after init, loadDocument with a bogus URL sets error' → 'after init, loadDocument completes and loading returns to false' — accurate now that the fake always resolves successfully

The ESLint no-restricted-syntax selector:

CallExpression[callee.object.name='vi'][callee.property.name='mock'] > Literal[value=/^pdfjs-dist/]

This is AST-precise and scoped correctly to **/*.spec.ts and **/*.test.ts. No broader than it needs to be.

No blockers

testHelpers.ts is well-structured. If a third component adopts LibLoader, ADR-012 already calls out the extraction point. Nothing premature here.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Round 2 review. All three concerns I flagged in round 1 have been addressed cleanly. ### What was fixed **DRY violation — resolved.** `testHelpers.ts` is now the single source of truth for both `makeFakePdfjsLib()` and `makeFakeLibLoader()`. Both test files import from it. The deleted `PdfViewer.svelte.spec.ts` eliminates the parallel fake definition entirely. Clean. **Missing error path — resolved.** `usePdfRenderer.svelte.test.ts` now has: ```typescript it('loadDocument sets error and loading=false when getDocument().promise rejects', ...) ``` This correctly uses an inline failing lib (rather than `makeFakePdfjsLib()` which only returns a success stub), verifies `r.loading === false` and `r.error === 'PDF not found'`. Well-structured. **Assertion style — resolved.** The three absorbed tests from `spec.ts` all use `toBeVisible()`, consistent with every other assertion in the `loaded state` describe block. ### Observations on the new commit The test name changes are improvements: - `'init() is callable and resolves without throwing in browser env'` → `'init() sets pdfjsReady to true when loader resolves'` — describes the behavior, not the mechanics - `'after init, loadDocument with a bogus URL sets error'` → `'after init, loadDocument completes and loading returns to false'` — accurate now that the fake always resolves successfully The ESLint `no-restricted-syntax` selector: ``` CallExpression[callee.object.name='vi'][callee.property.name='mock'] > Literal[value=/^pdfjs-dist/] ``` This is AST-precise and scoped correctly to `**/*.spec.ts` and `**/*.test.ts`. No broader than it needs to be. ### No blockers `testHelpers.ts` is well-structured. If a third component adopts `LibLoader`, ADR-012 already calls out the extraction point. Nothing premature here.

marcel commented

2026-05-12 15:23:11 +02:00

🧪 Sara Holt — Senior QA Engineer

Verdict: ✅ Approved

Round 2 review. Both issues I raised in round 1 are resolved.

What was fixed

Fidelity divergence — eliminated. PdfViewer.svelte.spec.ts is deleted. There is now exactly one fake implementation: testHelpers.ts. Both PdfViewer.svelte.test.ts and usePdfRenderer.svelte.test.ts import from it. No drift possible.

loadDocument error path — covered. The new test in usePdfRenderer.svelte.test.ts:

it('loadDocument sets error and loading=false when getDocument().promise rejects', async () => {
    // ...
    expect(r.loading).toBe(false);
    expect(r.error).toBe('PDF not found');
});

Two assertions — one per observable state change. Each assertion has exactly one reason to fail. This is the right pattern.

Test quality notes

The test correctly uses an inline failingLib rather than repurposing makeFakePdfjsLib(). The shared helper only models the success path; the error path test needs a custom stub where getDocument().promise rejects. That's the right call — don't bend a shared fixture to serve a single test's edge case.

Test names are now behaviorally accurate throughout. "init() sets pdfjsReady to true when loader resolves" — you know exactly what breaks if this fails.

The afterEach(cleanup) in PdfViewer.svelte.test.ts handles DOM teardown correctly. usePdfRenderer.svelte.test.ts tests pure state logic, no DOM to clean up — correct omission.

No blockers

Test suite structure is solid. Fake layer is DRY. Error paths are covered. Three-layer enforcement (ESLint + CI grep + birpc grep guard) gives me high confidence this regression class won't return.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ✅ Approved** Round 2 review. Both issues I raised in round 1 are resolved. ### What was fixed **Fidelity divergence — eliminated.** `PdfViewer.svelte.spec.ts` is deleted. There is now exactly one fake implementation: `testHelpers.ts`. Both `PdfViewer.svelte.test.ts` and `usePdfRenderer.svelte.test.ts` import from it. No drift possible. **`loadDocument` error path — covered.** The new test in `usePdfRenderer.svelte.test.ts`: ```typescript it('loadDocument sets error and loading=false when getDocument().promise rejects', async () => { // ... expect(r.loading).toBe(false); expect(r.error).toBe('PDF not found'); }); ``` Two assertions — one per observable state change. Each assertion has exactly one reason to fail. This is the right pattern. ### Test quality notes The test correctly uses an inline `failingLib` rather than repurposing `makeFakePdfjsLib()`. The shared helper only models the success path; the error path test needs a custom stub where `getDocument().promise` rejects. That's the right call — don't bend a shared fixture to serve a single test's edge case. Test names are now behaviorally accurate throughout. "init() sets pdfjsReady to true when loader resolves" — you know exactly what breaks if this fails. The `afterEach(cleanup)` in `PdfViewer.svelte.test.ts` handles DOM teardown correctly. `usePdfRenderer.svelte.test.ts` tests pure state logic, no DOM to clean up — correct omission. ### No blockers Test suite structure is solid. Fake layer is DRY. Error paths are covered. Three-layer enforcement (ESLint + CI grep + birpc grep guard) gives me high confidence this regression class won't return.

marcel commented

2026-05-12 15:23:18 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

Round 2 review. No architectural concerns in this PR; my round 1 assessment stands and the new commit reinforces the right patterns.

What I checked

testHelpers.ts placement. Co-located in src/lib/document/viewer/ — correct. This is test-layer infrastructure scoped to the PDF viewer domain. It does not leak into production code paths.

LibLoader type ownership. Still lives in usePdfRenderer.svelte.ts, exported from there. Appropriate for the current scope. ADR-012 carries the right note:

"If three or more components adopt this pattern, consider extracting a shared $lib/types/lib-loader.ts..."
Belt-and-suspenders decision log. No premature extraction, no premature coupling.

Three-layer enforcement. ESLint (lint-time) + CI grep (pre-test) + birpc coverage grep (post-test) is a good defense-in-depth strategy for a pattern that's hard to catch otherwise. Each layer catches it at a different point in the developer feedback loop.

ADR-012 update. The enforcement section now accurately reflects what's actually implemented. ADRs must stay current — this one does.

No blockers

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** Round 2 review. No architectural concerns in this PR; my round 1 assessment stands and the new commit reinforces the right patterns. ### What I checked **`testHelpers.ts` placement.** Co-located in `src/lib/document/viewer/` — correct. This is test-layer infrastructure scoped to the PDF viewer domain. It does not leak into production code paths. **`LibLoader` type ownership.** Still lives in `usePdfRenderer.svelte.ts`, exported from there. Appropriate for the current scope. ADR-012 carries the right note: > "If three or more components adopt this pattern, consider extracting a shared `$lib/types/lib-loader.ts`..." Belt-and-suspenders decision log. No premature extraction, no premature coupling. **Three-layer enforcement.** ESLint (lint-time) + CI grep (pre-test) + birpc coverage grep (post-test) is a good defense-in-depth strategy for a pattern that's hard to catch otherwise. Each layer catches it at a different point in the developer feedback loop. **ADR-012 update.** The enforcement section now accurately reflects what's actually implemented. ADRs must stay current — this one does. ### No blockers

marcel commented

2026-05-12 15:23:26 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Round 2 review. CI changes are solid. Let me walk through what the new step does.

CI grep step

- name: Assert no banned vi.mock patterns
  shell: bash
  run: |
    if grep -rF "vi.mock('pdfjs-dist'" frontend/src/; then
      echo "FAIL: banned vi.mock('pdfjs-dist') pattern found — see ADR 012. Use the libLoader prop injection pattern instead."
      exit 1
    fi

-F (fixed string) is the right flag. No regex, no risk of accidental shell meta-character expansion. Fast on large trees.

Positioned before the test suite. This fails fast — the grep runs in seconds before Playwright/Chromium spins up. Developer gets immediate feedback without waiting for the full browser test run.

Covers both quote styles? The pattern vi.mock('pdfjs-dist' (single quotes) is what the spec used. The ESLint AST rule catches the syntactic form regardless of quote style. Together they have complementary coverage — the grep catches the most common human-typed form, the ESLint rule catches it via the AST parser.

Three-layer regression backstop is well-structured

ESLint on save — fastest, catches during development
CI grep before tests — catches if lint was skipped
birpc coverage grep post-test — catches if somehow both above were bypassed

Each layer is cheap to run and independent. Good belt-and-suspenders.

No blockers

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Round 2 review. CI changes are solid. Let me walk through what the new step does. ### CI grep step ```yaml - name: Assert no banned vi.mock patterns shell: bash run: | if grep -rF "vi.mock('pdfjs-dist'" frontend/src/; then echo "FAIL: banned vi.mock('pdfjs-dist') pattern found — see ADR 012. Use the libLoader prop injection pattern instead." exit 1 fi ``` **`-F` (fixed string) is the right flag.** No regex, no risk of accidental shell meta-character expansion. Fast on large trees. **Positioned before the test suite.** This fails fast — the grep runs in seconds before Playwright/Chromium spins up. Developer gets immediate feedback without waiting for the full browser test run. **Covers both quote styles?** The pattern `vi.mock('pdfjs-dist'` (single quotes) is what the spec used. The ESLint AST rule catches the syntactic form regardless of quote style. Together they have complementary coverage — the grep catches the most common human-typed form, the ESLint rule catches it via the AST parser. ### Three-layer regression backstop is well-structured 1. ESLint on save — fastest, catches during development 2. CI grep before tests — catches if lint was skipped 3. birpc coverage grep post-test — catches if somehow both above were bypassed Each layer is cheap to run and independent. Good belt-and-suspenders. ### No blockers

marcel commented

2026-05-12 15:23:31 +02:00

🔒 Nora Steiner — Application Security Engineer

Verdict: ✅ Approved

Round 2 review. This PR is pure test infrastructure — no production code paths, no API surface, no auth logic changed.

What I checked

The vi.fn().mockResolvedValue([fakePdfjs, { default: '' }]) pattern creates no real network activity. The fake getDocument() returns a pre-resolved promise in memory. No SSRF vectors, no actual fetch calls to mock intercept issues.

The fetchSpy pattern used in the annotation tests (vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(...))) is correctly scoped with try/finally { fetchSpy.mockRestore() }. No test-global mock leakage.

The new testHelpers.ts exports vi from vitest — test-only dependency, not bundled in production.

No security findings

## 🔒 Nora Steiner — Application Security Engineer **Verdict: ✅ Approved** Round 2 review. This PR is pure test infrastructure — no production code paths, no API surface, no auth logic changed. ### What I checked The `vi.fn().mockResolvedValue([fakePdfjs, { default: '' }])` pattern creates no real network activity. The fake `getDocument()` returns a pre-resolved promise in memory. No SSRF vectors, no actual fetch calls to mock intercept issues. The `fetchSpy` pattern used in the annotation tests (`vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(...))`) is correctly scoped with `try/finally { fetchSpy.mockRestore() }`. No test-global mock leakage. The new `testHelpers.ts` exports `vi` from vitest — test-only dependency, not bundled in production. ### No security findings

marcel commented

2026-05-12 15:23:38 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Round 2 review. This PR closes issue #546 (fix PDF viewer test flakiness) and in the process makes the ADR and enforcement machinery match the implementation.

Requirements traceability

The original issue (#535 birpc race → #546 follow-up) had a clear acceptance criterion: 60 consecutive green CI runs with zero rpc is closed lines. The root cause was confirmed as vi.mock('pdfjs-dist', factory) triggering Playwright's route handler during teardown.

This PR addresses that in three layers:

Detection — ESLint rule flags the anti-pattern immediately on save
Prevention — CI grep blocks merges that contain the pattern
Regression — birpc coverage grep catches if tests produce the race condition anyway

The acceptance criterion in ADR-012 (60 consecutive green runs) remains unchanged. That's correct — the enforcement machinery helps prevent regressions, but the acceptance criterion is a runtime measurement that stands on its own.

ADR-012 enforcement section

The updated text accurately describes all three layers and their rationale. It also carries a forward-looking note about extracting a shared LibLoader type if the pattern spreads — appropriate scope management.

No requirements gaps found

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Round 2 review. This PR closes issue #546 (fix PDF viewer test flakiness) and in the process makes the ADR and enforcement machinery match the implementation. ### Requirements traceability The original issue (#535 birpc race → #546 follow-up) had a clear acceptance criterion: 60 consecutive green CI runs with zero `rpc is closed` lines. The root cause was confirmed as `vi.mock('pdfjs-dist', factory)` triggering Playwright's route handler during teardown. This PR addresses that in three layers: 1. **Detection** — ESLint rule flags the anti-pattern immediately on save 2. **Prevention** — CI grep blocks merges that contain the pattern 3. **Regression** — birpc coverage grep catches if tests produce the race condition anyway The acceptance criterion in ADR-012 (60 consecutive green runs) remains unchanged. That's correct — the enforcement machinery helps prevent regressions, but the acceptance criterion is a runtime measurement that stands on its own. ### ADR-012 enforcement section The updated text accurately describes all three layers and their rationale. It also carries a forward-looking note about extracting a shared `LibLoader` type if the pattern spreads — appropriate scope management. ### No requirements gaps found

marcel commented

2026-05-12 15:23:39 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

Round 2 review. This PR contains no user-facing changes — it's entirely test infrastructure and CI configuration. No UI components, no CSS, no Svelte template markup, no accessibility implications.

LGTM from the design and accessibility perspective.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** Round 2 review. This PR contains no user-facing changes — it's entirely test infrastructure and CI configuration. No UI components, no CSS, no Svelte template markup, no accessibility implications. LGTM from the design and accessibility perspective.

marcel added 2 commits 2026-05-12 16:19:49 +02:00

test(pdf-renderer): eliminate real pdfjs-dist loading from browser tests — use fake libLoader for all init() calls 23cbb6be22

Five tests in usePdfRenderer.svelte.test.ts called createPdfRenderer() without
a libLoader, causing init() to dynamically import pdfjs-dist in the browser.
Every dynamic import goes through Playwright's route handler, which calls
resolveManualMock via birpc to check for mocks. If the RPC closes during
teardown while one of these imports is in flight, the birpc race fires —
even though pdfjs-dist was never explicitly vi.mock()-ed.

Replace all bare createPdfRenderer() calls that invoke init() with
createPdfRenderer(makeFakeLibLoader()), identical to the pattern already
used in PdfViewer.svelte.test.ts. No real module loads, no route-handler
calls, no birpc exposure.

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

refactor(pdf-viewer-tests): extract shared fake, add loadDocument error path, fix assertions

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

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

Details

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

Details

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

Details

CI / fail2ban Regex (push) Successful in 38s

Details

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

Details

d21ba8fed2

- Extract makeFakePdfjsLib / makeFakeLibLoader to testHelpers.ts — single
  source of truth used by both PdfViewer.svelte.test.ts and
  usePdfRenderer.svelte.test.ts; removes the diverging-fidelity DRY violation
  flagged by @felixbrandt and @saraholt in the PR review
- Add 'loadDocument sets error and loading=false when getDocument().promise
  rejects' test to usePdfRenderer.svelte.test.ts — closes the error-path gap
  flagged by @felixbrandt and @saraholt
- Replace toBeInTheDocument() with toBeVisible() in the three absorbed
  spec-file tests — uniform assertion style across the loaded-state describe
  block, as flagged by @felixbrandt

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

marcel force-pushed worktree-feat+issue-546-pdf-viewer-test-fix from 8dd9e58fa4 to d21ba8fed2

2026-05-12 16:19:49 +02:00

Compare

marcel merged commit d21ba8fed2 into main

2026-05-12 16:20:29 +02:00

marcel deleted branch worktree-feat+issue-546-pdf-viewer-test-fix

2026-05-12 16:20:29 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#550