fix(pdf-viewer): remove banned vi.mock('pdfjs-dist') — ADR 012 enforcement (issue #546) #549

Merged

marcel merged 4 commits from worktree-feat+issue-546-pdf-viewer-test-fix into main 2026-05-12 12:32:24 +02:00

Conversation 7 Commits 4 Files Changed 5 +85 -89

marcel commented 2026-05-12 12:19:58 +02:00

Owner

Copy Link

Summary

• Ports PdfViewer.svelte.test.ts from vi.mock('pdfjs-dist', …) to the libLoader prop injection pattern — eliminates the birpc teardown race documented in ADR 012
• Consolidates PdfViewer.svelte.spec.ts into test.ts (absorbs nav/zoom/page-counter tests, deletes the spec file)
• Adds an ESLint no-restricted-syntax rule scoped to *.spec.ts / *.test.ts that flags the banned pattern at save time
• Adds an early CI grep step that fails in ~1 s if the banned pattern reappears in frontend/src/, before Playwright even spins up

Commits

• 1f7f6bde test(pdf-viewer): port PdfViewer.svelte.test.ts to libLoader prop injection — remove vi.mock
• 32d02b79 test(pdf-viewer): consolidate spec.ts into test.ts — absorb page-counter test, delete spec
• c1e5732f refactor(eslint): ban vi.mock('pdfjs-dist', ...) in spec/test files — ADR 012
• f9389712 ci(unit-tests): add early grep check for banned vi.mock pdfjs-dist pattern

Test plan

• CI unit-tests job passes green (no birpc teardown race in coverage log)
• npm run lint clean — ESLint rule fires if you manually add vi.mock('pdfjs-dist' to any spec/test file
• PdfViewer.svelte.spec.ts no longer exists; all 18 tests live in test.ts

Closes #546

🤖 Generated with Claude Code

## Summary - Ports `PdfViewer.svelte.test.ts` from `vi.mock('pdfjs-dist', …)` to the libLoader prop injection pattern — eliminates the birpc teardown race documented in ADR 012 - Consolidates `PdfViewer.svelte.spec.ts` into `test.ts` (absorbs nav/zoom/page-counter tests, deletes the spec file) - Adds an ESLint `no-restricted-syntax` rule scoped to `*.spec.ts` / `*.test.ts` that flags the banned pattern at save time - Adds an early CI grep step that fails in ~1 s if the banned pattern reappears in `frontend/src/`, before Playwright even spins up ## Commits - `1f7f6bde` test(pdf-viewer): port PdfViewer.svelte.test.ts to libLoader prop injection — remove vi.mock - `32d02b79` test(pdf-viewer): consolidate spec.ts into test.ts — absorb page-counter test, delete spec - `c1e5732f` refactor(eslint): ban vi.mock('pdfjs-dist', ...) in spec/test files — ADR 012 - `f9389712` ci(unit-tests): add early grep check for banned vi.mock pdfjs-dist pattern ## Test plan - [ ] CI unit-tests job passes green (no birpc teardown race in coverage log) - [ ] `npm run lint` clean — ESLint rule fires if you manually add `vi.mock('pdfjs-dist'` to any spec/test file - [ ] `PdfViewer.svelte.spec.ts` no longer exists; all 18 tests live in `test.ts` Closes #546 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 4 commits 2026-05-12 12:19:58 +02:00

test(pdf-viewer): port PdfViewer.svelte.test.ts to libLoader prop injection — remove vi.mock ... 1f7f6bde15

Removes both vi.mock('pdfjs-dist', …) calls that caused the birpc teardown
race (ADR 012). Replaces with static import + makeFakeLibLoader() helper
injected via the libLoader prop on every render() call.

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

test(pdf-viewer): consolidate spec.ts into test.ts — absorb page-counter test, delete spec ... 32d02b79f7

Absorbs the three tests from PdfViewer.svelte.spec.ts (nav buttons, zoom
controls, page counter) into the loaded-state describe in test.ts, then
deletes the now-empty spec file. One spec file per component.

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

refactor(eslint): ban vi.mock('pdfjs-dist', ...) in spec/test files — ADR 012 ... c1e5732fad

Adds a no-restricted-syntax rule scoped to *.spec.ts / *.test.ts that
flags any vi.mock call whose first argument starts with 'pdfjs-dist'.
Turns the ~2-min CI wait into an immediate lint error on save.
Updates ADR 012 Enforcement section to document the rule.

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

ci(unit-tests): add early grep check for banned vi.mock pdfjs-dist pattern ... f938971292

Adds a static grep step that runs after Lint and before the test suite.
Fails in ~1 s if any file under frontend/src/ contains the banned
vi.mock('pdfjs-dist' pattern, catching the regression before Playwright
spins up. Belt-and-suspenders with the ESLint rule (ADR 012).

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

marcel commented 2026-05-12 12:26:00 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

This is clean, disciplined test migration. The pattern is correct and consistently applied across all render calls.

What I checked

• Factory functions: makeFakePdfjsLib() and makeFakeLibLoader() are well-named, return fresh mocks per call (each render() gets its own vi.fn()), and follow the factory pattern correctly. No shared mock state between tests.
• Cleanup: afterEach(cleanup) is correctly present — was previously only in spec.ts.
• Comments removed: The removed inline comments ("PdfControls renders its nav + zoom buttons once the document.promise resolves", "transcribeMode forces showAnnotations=true...", "Without an annotations fetch, the visibility toggle is hidden", "PDF rendering does not depend on the annotations fetch") were all "what" comments. The test names and assertions carry the intent. Good removal.
• Test placement: The three tests absorbed from spec.ts (shows previous and next page navigation buttons, shows zoom controls, displays the page counter) sit correctly inside describe('PdfViewer — loaded state') — they require a resolved fake library to assert on controls.
• TypeScript hygiene: [fakePdfjs, { default: '' }] as const gives the tuple its exact type, matching the LibLoader return type contract.
• ESLint selector: "CallExpression[callee.object.name='vi'][callee.property.name='mock'] > Literal[value=/^pdfjs-dist/]" — the /^pdfjs-dist/ regex covers both pdfjs-dist and pdfjs-dist/build/pdf.worker.min.mjs?url. The > (direct child combinator) is correct since the first argument of vi.mock() is a direct child of the CallExpression.

Minor observation (not a blocker)

The three absorbed tests call render() without a documentId prop. This is intentionally minimal — they only care about controls appearing when a PDF loads. The omission is correct for their scope, but the absence is slightly jarring in a describe block where most peers pass documentId. Not a blocker.

All 18 tests in one file. Consolidation reduces cognitive overhead when reading this suite.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** This is clean, disciplined test migration. The pattern is correct and consistently applied across all render calls. ### What I checked - **Factory functions**: `makeFakePdfjsLib()` and `makeFakeLibLoader()` are well-named, return fresh mocks per call (each `render()` gets its own `vi.fn()`), and follow the factory pattern correctly. No shared mock state between tests. - **Cleanup**: `afterEach(cleanup)` is correctly present — was previously only in spec.ts. - **Comments removed**: The removed inline comments (`"PdfControls renders its nav + zoom buttons once the document.promise resolves"`, `"transcribeMode forces showAnnotations=true..."`, `"Without an annotations fetch, the visibility toggle is hidden"`, `"PDF rendering does not depend on the annotations fetch"`) were all "what" comments. The test names and assertions carry the intent. Good removal. - **Test placement**: The three tests absorbed from spec.ts (`shows previous and next page navigation buttons`, `shows zoom controls`, `displays the page counter`) sit correctly inside `describe('PdfViewer — loaded state')` — they require a resolved fake library to assert on controls. - **TypeScript hygiene**: `[fakePdfjs, { default: '' }] as const` gives the tuple its exact type, matching the `LibLoader` return type contract. - **ESLint selector**: `"CallExpression[callee.object.name='vi'][callee.property.name='mock'] > Literal[value=/^pdfjs-dist/]"` — the `/^pdfjs-dist/` regex covers both `pdfjs-dist` and `pdfjs-dist/build/pdf.worker.min.mjs?url`. The `>` (direct child combinator) is correct since the first argument of `vi.mock()` is a direct child of the CallExpression. ### Minor observation (not a blocker) The three absorbed tests call `render()` without a `documentId` prop. This is intentionally minimal — they only care about controls appearing when a PDF loads. The omission is correct for their scope, but the absence is slightly jarring in a describe block where most peers pass `documentId`. Not a blocker. All 18 tests in one file. Consolidation reduces cognitive overhead when reading this suite.

marcel commented 2026-05-12 12:26:06 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

The structural changes are correct. ADR-012 is the authoritative record of the mocking decision, and this PR correctly updates it to reflect the enforcement reality that now exists.

Documentation audit

Per the doc update matrix:

Change	Required doc update	Status
Architectural enforcement mechanism changed	Update ADR in docs/adr/	✅ Done — ADR-012 enforcement note updated

No other doc updates required: no new routes, no new backend packages, no DB schema changes, no new Docker services, no new ErrorCode or Permission values.

Pattern assessment

The libLoader prop injection pattern is the right call architecturally. It:

• Pushes the external dependency inward (through a prop) rather than sideways (via module-level mock hoisting)
• Makes the component's external library dependency explicit in its prop interface
• Allows test doubles without coupling to the module system's hoisting lifecycle

The ESLint rule + CI grep combination gives three enforcement layers: save-time (ESLint), pre-test CI (grep), and coverage CI (birpc guard). Belt-and-suspenders for a failure mode that cost real CI time. Proportionate response.

ADR-012 update quality

The updated enforcement note accurately describes what was added and what remains. The revisit signal ("If three or more components adopt this pattern, consider extracting a shared type...") is intact — good. That's the right moment to revisit, not before.

No architectural concerns.

## 🏗️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** The structural changes are correct. ADR-012 is the authoritative record of the mocking decision, and this PR correctly updates it to reflect the enforcement reality that now exists. ### Documentation audit Per the doc update matrix: | Change | Required doc update | Status | |---|---|---| | Architectural enforcement mechanism changed | Update ADR in `docs/adr/` | ✅ Done — ADR-012 enforcement note updated | No other doc updates required: no new routes, no new backend packages, no DB schema changes, no new Docker services, no new `ErrorCode` or `Permission` values. ### Pattern assessment The libLoader prop injection pattern is the right call architecturally. It: - Pushes the external dependency inward (through a prop) rather than sideways (via module-level mock hoisting) - Makes the component's external library dependency explicit in its prop interface - Allows test doubles without coupling to the module system's hoisting lifecycle The ESLint rule + CI grep combination gives three enforcement layers: save-time (ESLint), pre-test CI (grep), and coverage CI (birpc guard). Belt-and-suspenders for a failure mode that cost real CI time. Proportionate response. ### ADR-012 update quality The updated enforcement note accurately describes what was added and what remains. The revisit signal ("If three or more components adopt this pattern, consider extracting a shared type...") is intact — good. That's the right moment to revisit, not before. No architectural concerns.

marcel commented 2026-05-12 12:26:14 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

CI change assessment

New step inserted between npm run lint and Run unit and component tests with coverage:

- 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

What's correct

• Ordering: Runs after lint, before the test suite. Grep exits in ~1 s — no point burning 2+ minutes of test time before catching a banned pattern that ESLint missed at save time.
• shell: bash: The if/then/fi construct is bash-specific. Explicit declaration is correct — some runners default to sh.
• -F flag: Fixed-string matching, not regex. More efficient and unambiguous for a literal string check.
• Path: frontend/src/ is relative to the repo root, which is the default working directory. No working-directory: override needed.
• Error message: Actionable. Names the banned pattern, references the ADR, names the replacement. An engineer reading this in a failed CI run can act immediately.

One suggestion (not a blocker)

grep exits with code 2 on error (path doesn't exist, permission issue). The current if grep ... treats exit code 2 the same as exit code 1 (no match), so a broken grep invocation silently passes. For a safety check this is mildly ironic.

Hardening option, if desired:

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
  # grep exits 1 (no match) → step passes; grep exits 2 (error) → also passes silently

Could set set -o pipefail or check [ -d frontend/src ] first. For this codebase and runner environment the current form is pragmatically fine — just worth being aware of.

No blockers.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### CI change assessment New step inserted between `npm run lint` and `Run unit and component tests with coverage`: ```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 ``` ### What's correct - **Ordering**: Runs after lint, before the test suite. Grep exits in ~1 s — no point burning 2+ minutes of test time before catching a banned pattern that ESLint missed at save time. - **`shell: bash`**: The `if/then/fi` construct is bash-specific. Explicit declaration is correct — some runners default to `sh`. - **`-F` flag**: Fixed-string matching, not regex. More efficient and unambiguous for a literal string check. - **Path**: `frontend/src/` is relative to the repo root, which is the default working directory. No `working-directory:` override needed. - **Error message**: Actionable. Names the banned pattern, references the ADR, names the replacement. An engineer reading this in a failed CI run can act immediately. ### One suggestion (not a blocker) `grep` exits with code 2 on error (path doesn't exist, permission issue). The current `if grep ...` treats exit code 2 the same as exit code 1 (no match), so a broken grep invocation silently passes. For a safety check this is mildly ironic. Hardening option, if desired: ```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 # grep exits 1 (no match) → step passes; grep exits 2 (error) → also passes silently ``` Could set `set -o pipefail` or check `[ -d frontend/src ]` first. For this codebase and runner environment the current form is pragmatically fine — just worth being aware of. No blockers.

marcel commented 2026-05-12 12:26:20 +02:00

Author

Owner

Copy Link

🔒 Nora Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR modifies test infrastructure and enforcement tooling exclusively. No production code is touched. My security review finds nothing to flag.

What I checked

1. CI grep command — grep -rF "vi.mock('pdfjs-dist'" frontend/src/ uses fixed-string matching, targets a known source directory, operates only on committed source files. No user input in the pipeline, no injection risk.
2. ESLint rule — The no-restricted-syntax selector operates on parsed AST nodes, not on user-controlled data. The regex /^pdfjs-dist/ is a static literal embedded in the config, not constructed from external input. No security concern.
3. ADR-012 update — Documentation change only. Zero runtime security impact.
4. Test file changes — Factory functions use vi.fn().mockResolvedValue() for prop-based fake injection. No auth handling, no secrets, no input validation bypass, no XSS surface.

Indirect security note

Eliminating a flaky CI pattern is good for security posture. When CI suites produce non-deterministic failures, teams learn to dismiss red builds — and that's when real security regressions slip through unnoticed. A reliable CI is a prerequisite for trusting the suite as a security gate. This PR moves in the right direction.

No findings.

## 🔒 Nora Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR modifies test infrastructure and enforcement tooling exclusively. No production code is touched. My security review finds nothing to flag. ### What I checked 1. **CI grep command** — `grep -rF "vi.mock('pdfjs-dist'" frontend/src/` uses fixed-string matching, targets a known source directory, operates only on committed source files. No user input in the pipeline, no injection risk. 2. **ESLint rule** — The `no-restricted-syntax` selector operates on parsed AST nodes, not on user-controlled data. The regex `/^pdfjs-dist/` is a static literal embedded in the config, not constructed from external input. No security concern. 3. **ADR-012 update** — Documentation change only. Zero runtime security impact. 4. **Test file changes** — Factory functions use `vi.fn().mockResolvedValue()` for prop-based fake injection. No auth handling, no secrets, no input validation bypass, no XSS surface. ### Indirect security note Eliminating a flaky CI pattern is good for security posture. When CI suites produce non-deterministic failures, teams learn to dismiss red builds — and that's when real security regressions slip through unnoticed. A reliable CI is a prerequisite for trusting the suite as a security gate. This PR moves in the right direction. No findings.

marcel commented 2026-05-12 12:26:31 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ✅ Approved

This is a solid test consolidation and regression-prevention investment. Let me work through it systematically.

Test quality

Factory pattern ✅ — makeFakePdfjsLib() and makeFakeLibLoader() are correct factory functions. Each render() call passes libLoader: makeFakeLibLoader(), creating a fresh vi.fn() per test. No shared mock state across tests — isolation is correct.

Cleanup ✅ — afterEach(cleanup) is in place. It was previously only in spec.ts; test.ts now owns it correctly.

Test naming ✅ — Sentences describing behavior. The absorbed tests maintain the same naming style as the existing suite ('shows previous and next page navigation buttons', 'shows zoom controls', 'displays the page counter once the PDF has loaded').

Behavioral assertions ✅ — All assertions use getByRole, getByText, toBeVisible(), toBeInTheDocument(). Testing what users see, not component internals.

Test placement ✅ — The three absorbed tests from spec.ts belong inside describe('PdfViewer — loaded state'): they require the fake library to resolve before asserting on controls. Correct grouping.

Flakiness elimination

The birpc teardown race was non-deterministic by nature — it depended on GC and IPC timing after each test. The libLoader prop injection doesn't touch the module system's hoisting or teardown lifecycle at all. The race is architecturally eliminated, not suppressed. This is the right fix.

Enforcement layers

Layer	Mechanism	Trigger time
1	ESLint no-restricted-syntax	On save in editor
2	CI grep step	~1 s into CI, before tests
3	Existing birpc grep guard	After coverage run

Three layers for one failure mode that cost real time. Proportionate and maintainable.

Minor observation

The three absorbed tests at the bottom call render() without documentId. This is intentionally minimal — they only assert on basic controls appearing when any PDF loads. The behavior is correct. If this suite grows and those tests need annotation context, they'll need documentId added then. Not a blocker today.

No blockers.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ✅ Approved** This is a solid test consolidation and regression-prevention investment. Let me work through it systematically. ### Test quality **Factory pattern** ✅ — `makeFakePdfjsLib()` and `makeFakeLibLoader()` are correct factory functions. Each `render()` call passes `libLoader: makeFakeLibLoader()`, creating a fresh `vi.fn()` per test. No shared mock state across tests — isolation is correct. **Cleanup** ✅ — `afterEach(cleanup)` is in place. It was previously only in spec.ts; test.ts now owns it correctly. **Test naming** ✅ — Sentences describing behavior. The absorbed tests maintain the same naming style as the existing suite (`'shows previous and next page navigation buttons'`, `'shows zoom controls'`, `'displays the page counter once the PDF has loaded'`). **Behavioral assertions** ✅ — All assertions use `getByRole`, `getByText`, `toBeVisible()`, `toBeInTheDocument()`. Testing what users see, not component internals. **Test placement** ✅ — The three absorbed tests from spec.ts belong inside `describe('PdfViewer — loaded state')`: they require the fake library to resolve before asserting on controls. Correct grouping. ### Flakiness elimination The birpc teardown race was non-deterministic by nature — it depended on GC and IPC timing after each test. The libLoader prop injection doesn't touch the module system's hoisting or teardown lifecycle at all. The race is architecturally eliminated, not suppressed. This is the right fix. ### Enforcement layers | Layer | Mechanism | Trigger time | |---|---|---| | 1 | ESLint `no-restricted-syntax` | On save in editor | | 2 | CI grep step | ~1 s into CI, before tests | | 3 | Existing birpc grep guard | After coverage run | Three layers for one failure mode that cost real time. Proportionate and maintainable. ### Minor observation The three absorbed tests at the bottom call `render()` without `documentId`. This is intentionally minimal — they only assert on basic controls appearing when any PDF loads. The behavior is correct. If this suite grows and those tests need annotation context, they'll need `documentId` added then. Not a blocker today. No blockers.

marcel commented 2026-05-12 12:26:38 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This PR contains no UI component changes, no template changes, no CSS changes, and no user-facing modifications. My review scope is minimal but I do check one thing that lives at the intersection of tests and accessibility.

Accessibility-aligned test assertions

The test assertions use role-based selectors with German localized labels matching the production component:

Assertion	Type	Accessible name verified
getByRole('button', { name: 'Zurück' })	Exact	✅
getByRole('button', { name: /zurück/i })	Case-insensitive	✅
getByRole('button', { name: 'Weiter' })	Exact	✅
getByRole('button', { name: 'Vergrößern' })	Exact	✅
getByRole('button', { name: 'Verkleinern' })	Exact	✅
getByRole('button', { name: /vergrößern/i })	Case-insensitive	✅
getByRole('button', { name: /verkleinern/i })	Case-insensitive	✅

Using getByRole('button', { name: ... }) to locate buttons means these tests will fail if the buttons lose their accessible name (e.g., if aria-label is removed or the visible text disappears). This is the correct testing pattern — it verifies accessibility correctness as a side effect of verifying behavior. Nothing to flag here.

LGTM.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This PR contains no UI component changes, no template changes, no CSS changes, and no user-facing modifications. My review scope is minimal but I do check one thing that lives at the intersection of tests and accessibility. ### Accessibility-aligned test assertions The test assertions use role-based selectors with German localized labels matching the production component: | Assertion | Type | Accessible name verified | |---|---|---| | `getByRole('button', { name: 'Zurück' })` | Exact | ✅ | | `getByRole('button', { name: /zurück/i })` | Case-insensitive | ✅ | | `getByRole('button', { name: 'Weiter' })` | Exact | ✅ | | `getByRole('button', { name: 'Vergrößern' })` | Exact | ✅ | | `getByRole('button', { name: 'Verkleinern' })` | Exact | ✅ | | `getByRole('button', { name: /vergrößern/i })` | Case-insensitive | ✅ | | `getByRole('button', { name: /verkleinern/i })` | Case-insensitive | ✅ | Using `getByRole('button', { name: ... })` to locate buttons means these tests will **fail** if the buttons lose their accessible name (e.g., if `aria-label` is removed or the visible text disappears). This is the correct testing pattern — it verifies accessibility correctness as a side effect of verifying behavior. Nothing to flag here. LGTM.

marcel commented 2026-05-12 12:26:46 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

This PR delivers on issue #546 clearly and completely. I review it for requirements coverage and traceability.

Issue-to-delivery mapping

PR claim	Verifiable in diff	Status
Remove vi.mock('pdfjs-dist', …) from test.ts	vi.mock block deleted, replaced with prop injection	✅
ESLint rule flags banned pattern at save time	eslint.config.js — no-restricted-syntax scoped to spec/test files	✅
CI grep fails in ~1 s if pattern reappears	ci.yml — grep step before test suite	✅
spec.ts deleted; 3 tests absorbed into test.ts	spec.ts shown as deleted; 3 tests visible at bottom of test.ts diff	✅
ADR-012 enforcement note updated	docs/adr/012-browser-test-mocking-strategy.md updated	✅

Test plan verifiability

The PR's test plan checklist is well-formed and each item is independently verifiable:

• CI unit-tests job passes green → observable in CI logs
• npm run lint clean → observable locally and in CI lint step
• PdfViewer.svelte.spec.ts no longer exists → observable in repo file tree

Scope discipline

The PR delivers both the remediation (remove banned pattern) and the prevention mechanism (ESLint + CI grep), without adding unrelated changes. This is tight scope. The issue is fully closed without feature creep.

No open ambiguities

The PR description references ADR-012 and issue #546. The rationale (birpc teardown race) is documented in the ADR. The replacement pattern (libLoader prop injection) is documented in both the ADR and demonstrated in the test changes. No requirements gaps.

No findings.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** This PR delivers on issue #546 clearly and completely. I review it for requirements coverage and traceability. ### Issue-to-delivery mapping | PR claim | Verifiable in diff | Status | |---|---|---| | Remove `vi.mock('pdfjs-dist', …)` from test.ts | `vi.mock` block deleted, replaced with prop injection | ✅ | | ESLint rule flags banned pattern at save time | `eslint.config.js` — `no-restricted-syntax` scoped to spec/test files | ✅ | | CI grep fails in ~1 s if pattern reappears | `ci.yml` — grep step before test suite | ✅ | | `spec.ts` deleted; 3 tests absorbed into `test.ts` | `spec.ts` shown as deleted; 3 tests visible at bottom of `test.ts` diff | ✅ | | ADR-012 enforcement note updated | `docs/adr/012-browser-test-mocking-strategy.md` updated | ✅ | ### Test plan verifiability The PR's test plan checklist is well-formed and each item is independently verifiable: - CI unit-tests job passes green → observable in CI logs - `npm run lint` clean → observable locally and in CI lint step - `PdfViewer.svelte.spec.ts` no longer exists → observable in repo file tree ### Scope discipline The PR delivers both the remediation (remove banned pattern) and the prevention mechanism (ESLint + CI grep), without adding unrelated changes. This is tight scope. The issue is fully closed without feature creep. ### No open ambiguities The PR description references ADR-012 and issue #546. The rationale (birpc teardown race) is documented in the ADR. The replacement pattern (libLoader prop injection) is documented in both the ADR and demonstrated in the test changes. No requirements gaps. No findings.

marcel merged commit 9260866f47 into main 2026-05-12 12:32:24 +02:00

marcel deleted branch worktree-feat+issue-546-pdf-viewer-test-fix 2026-05-12 12:32:25 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#549