From 1f7f6bde15faa5efdca6c9c67c5e5e451da377ef Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 12 May 2026 12:17:25 +0200 Subject: [PATCH 1/4] =?UTF-8?q?test(pdf-viewer):=20port=20PdfViewer.svelte?= =?UTF-8?q?.test.ts=20to=20libLoader=20prop=20injection=20=E2=80=94=20remo?= =?UTF-8?q?ve=20vi.mock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../document/viewer/PdfViewer.svelte.test.ts | 94 ++++++++++++------- 1 file changed, 62 insertions(+), 32 deletions(-) diff --git a/frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts b/frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts index 12383c6d..a4b49ba9 100644 --- a/frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts +++ b/frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts @@ -1,11 +1,18 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { cleanup, render } from 'vitest-browser-svelte'; import { page } from 'vitest/browser'; +import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte'; +import PdfViewer from './PdfViewer.svelte'; -vi.mock('pdfjs-dist', () => { - function TextLayerMock() {} - TextLayerMock.prototype.render = () => Promise.resolve(); - TextLayerMock.prototype.cancel = () => {}; +afterEach(cleanup); + +function makeFakePdfjsLib() { + class TextLayerMock { + render() { + return Promise.resolve(); + } + cancel() {} + } return { GlobalWorkerOptions: { workerSrc: '' }, @@ -20,24 +27,23 @@ vi.mock('pdfjs-dist', () => { }) }), TextLayer: TextLayerMock - }; -}); + } as unknown as typeof import('pdfjs-dist'); +} -vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', () => ({ default: '' })); - -const { default: PdfViewer } = await import('./PdfViewer.svelte'); - -afterEach(cleanup); +function makeFakeLibLoader(): LibLoader { + const fakePdfjs = makeFakePdfjsLib(); + return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const); +} describe('PdfViewer — empty / error states', () => { it('renders the no-file placeholder when url is empty', async () => { - render(PdfViewer, { url: '' }); + render(PdfViewer, { url: '', libLoader: makeFakeLibLoader() }); await expect.element(page.getByText('Keine Datei vorhanden')).toBeVisible(); }); it('does not render the controls when url is empty', async () => { - render(PdfViewer, { url: '' }); + render(PdfViewer, { url: '', libLoader: makeFakeLibLoader() }); const buttons = document.querySelectorAll('button'); expect(buttons.length).toBe(0); @@ -49,10 +55,10 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - annotationReloadKey: 0 + annotationReloadKey: 0, + libLoader: makeFakeLibLoader() }); - // PdfControls renders its nav + zoom buttons once the document.promise resolves. await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); await expect.element(page.getByRole('button', { name: 'Weiter' })).toBeVisible(); await expect.element(page.getByRole('button', { name: 'Vergrößern' })).toBeVisible(); @@ -63,7 +69,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - annotationsDimmed: true + annotationsDimmed: true, + libLoader: makeFakeLibLoader() }); await vi.waitFor(() => { @@ -96,11 +103,10 @@ describe('PdfViewer — loaded state', () => { url: '/api/documents/test/file', documentId: 'test', transcribeMode: true, - documentFileHash: 'match' + documentFileHash: 'match', + libLoader: makeFakeLibLoader() }); - // transcribeMode forces showAnnotations=true; toggle button surfaces with "hide" label - // (only when annotationCount > 0). await expect .element(page.getByRole('button', { name: /annotierungen verbergen/i })) .toBeVisible(); @@ -113,7 +119,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - documentFileHash: 'abc123' + documentFileHash: 'abc123', + libLoader: makeFakeLibLoader() }); await vi.waitFor(() => { @@ -125,7 +132,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - flashAnnotationId: 'ann-flashing' + flashAnnotationId: 'ann-flashing', + libLoader: makeFakeLibLoader() }); await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); @@ -135,7 +143,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - blockNumbers: { 'ann-1': 1, 'ann-2': 2 } + blockNumbers: { 'ann-1': 1, 'ann-2': 2 }, + libLoader: makeFakeLibLoader() }); await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); @@ -145,7 +154,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - activeAnnotationId: 'ann-1' + activeAnnotationId: 'ann-1', + libLoader: makeFakeLibLoader() }); await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); @@ -156,10 +166,10 @@ describe('PdfViewer — loaded state', () => { url: '/api/documents/test/file', documentId: 'test', transcribeMode: true, - activeAnnotationId: 'ann-1' + activeAnnotationId: 'ann-1', + libLoader: makeFakeLibLoader() }); - // Without an annotations fetch, the visibility toggle is hidden — just assert the always-on nav. await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); await expect.element(page.getByRole('button', { name: 'Weiter' })).toBeVisible(); }); @@ -169,7 +179,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - onAnnotationClick + onAnnotationClick, + libLoader: makeFakeLibLoader() }); await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); @@ -199,7 +210,8 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - documentFileHash: 'new-hash' + documentFileHash: 'new-hash', + libLoader: makeFakeLibLoader() }); await vi.waitFor(() => { @@ -234,10 +246,10 @@ describe('PdfViewer — loaded state', () => { render(PdfViewer, { url: '/api/documents/test/file', documentId: 'test', - documentFileHash: 'matching-hash' + documentFileHash: 'matching-hash', + libLoader: makeFakeLibLoader() }); - // Controls finish mounting, and the outdated notice stays absent. await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); expect(document.querySelector('[data-testid="annotation-outdated-notice"]')).toBeNull(); } finally { @@ -250,10 +262,10 @@ describe('PdfViewer — loaded state', () => { try { render(PdfViewer, { url: '/api/documents/test/file', - documentId: 'test' + documentId: 'test', + libLoader: makeFakeLibLoader() }); - // PDF rendering does not depend on the annotations fetch — controls still appear. await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); expect(document.querySelector('[data-testid="annotation-outdated-notice"]')).toBeNull(); } finally { @@ -268,7 +280,8 @@ describe('PdfViewer — loaded state', () => { try { render(PdfViewer, { url: '/api/documents/test/file', - documentId: 'test' + documentId: 'test', + libLoader: makeFakeLibLoader() }); await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible(); @@ -277,4 +290,21 @@ describe('PdfViewer — loaded state', () => { fetchSpy.mockRestore(); } }); + + it('shows previous and next page navigation buttons', async () => { + render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() }); + await expect.element(page.getByRole('button', { name: /zurück/i })).toBeInTheDocument(); + await expect.element(page.getByRole('button', { name: /weiter/i })).toBeInTheDocument(); + }); + + it('shows zoom controls', async () => { + render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() }); + await expect.element(page.getByRole('button', { name: /vergrößern/i })).toBeInTheDocument(); + await expect.element(page.getByRole('button', { name: /verkleinern/i })).toBeInTheDocument(); + }); + + it('displays the page counter once the PDF has loaded', async () => { + render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() }); + await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument(); + }); }); -- 2.49.1 From 32d02b79f76d286c5645b8f77c5a58d6d31da2af Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 12 May 2026 12:17:56 +0200 Subject: [PATCH 2/4] =?UTF-8?q?test(pdf-viewer):=20consolidate=20spec.ts?= =?UTF-8?q?=20into=20test.ts=20=E2=80=94=20absorb=20page-counter=20test,?= =?UTF-8?q?=20delete=20spec?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../document/viewer/PdfViewer.svelte.spec.ts | 56 ------------------- 1 file changed, 56 deletions(-) delete mode 100644 frontend/src/lib/document/viewer/PdfViewer.svelte.spec.ts diff --git a/frontend/src/lib/document/viewer/PdfViewer.svelte.spec.ts b/frontend/src/lib/document/viewer/PdfViewer.svelte.spec.ts deleted file mode 100644 index 6067481d..00000000 --- a/frontend/src/lib/document/viewer/PdfViewer.svelte.spec.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { vi, describe, it, expect, afterEach } from 'vitest'; -import { cleanup, render } from 'vitest-browser-svelte'; -import { page } from 'vitest/browser'; -import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte'; -import PdfViewer from './PdfViewer.svelte'; - -afterEach(cleanup); - -function makeFakePdfjsLib() { - class TextLayerMock { - render() { - return Promise.resolve(); - } - cancel() {} - } - - return { - GlobalWorkerOptions: { workerSrc: '' }, - getDocument: vi.fn().mockReturnValue({ - promise: Promise.resolve({ - numPages: 2, - getPage: vi.fn().mockResolvedValue({ - getViewport: vi.fn().mockReturnValue({ width: 595, height: 842 }), - render: vi.fn().mockReturnValue({ promise: Promise.resolve() }), - streamTextContent: vi.fn().mockReturnValue(new ReadableStream()) - }) - }) - }), - TextLayer: TextLayerMock - } as unknown as typeof import('pdfjs-dist'); -} - -function makeFakeLibLoader(): LibLoader { - const fakePdfjs = makeFakePdfjsLib(); - return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const); -} - -describe('PdfViewer', () => { - it('shows previous and next page navigation buttons', async () => { - render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() }); - await expect.element(page.getByRole('button', { name: /zurück/i })).toBeInTheDocument(); - await expect.element(page.getByRole('button', { name: /weiter/i })).toBeInTheDocument(); - }); - - it('shows zoom controls', async () => { - render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() }); - await expect.element(page.getByRole('button', { name: /vergrößern/i })).toBeInTheDocument(); - await expect.element(page.getByRole('button', { name: /verkleinern/i })).toBeInTheDocument(); - }); - - it('displays the page counter once the PDF has loaded', async () => { - render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() }); - // Fake loader resolves synchronously, so "1 / 2" should appear quickly - await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument(); - }); -}); -- 2.49.1 From c1e5732fadb0bbf952db40cd60d07991ea3c6607 Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 12 May 2026 12:18:28 +0200 Subject: [PATCH 3/4] =?UTF-8?q?refactor(eslint):=20ban=20vi.mock('pdfjs-di?= =?UTF-8?q?st',=20...)=20in=20spec/test=20files=20=E2=80=94=20ADR=20012?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/012-browser-test-mocking-strategy.md | 2 +- frontend/eslint.config.js | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/docs/adr/012-browser-test-mocking-strategy.md b/docs/adr/012-browser-test-mocking-strategy.md index 763a7649..173ed63e 100644 --- a/docs/adr/012-browser-test-mocking-strategy.md +++ b/docs/adr/012-browser-test-mocking-strategy.md @@ -86,5 +86,5 @@ These modules are resolved at static import time (before any test runs). Their ` - New browser-mode specs that need to stub an external library **must not** use `vi.mock(externalLib, factory)`. Add a loader/factory parameter to the underlying hook or service instead. - The CI `unit-tests` job includes a permanent grep guard that fails the build if `rpc is closed` appears in any coverage run log. This catches regressions before they reach the acceptance criterion. - Acceptance criterion for #535: 60 consecutive green `workflow_dispatch` CI runs against `main` after the fix is merged, with zero `rpc is closed` lines in any log. -- **Enforcement:** No automated lint rule is planned; the CI coverage guard is the regression backstop. If a lint rule is added later (e.g. an ESLint rule flagging `vi.mock` of non-virtual modules in browser-mode spec files), update this ADR. +- **Enforcement:** An ESLint `no-restricted-syntax` rule in `eslint.config.js` (scoped to `**/*.spec.ts` and `**/*.test.ts`) flags any `vi.mock('pdfjs-dist', ...)` call with a message referencing this ADR. This turns a ~2-minute CI wait into an immediate lint error on save. The CI birpc grep guard remains as belt-and-suspenders for the coverage run. A CI static grep step (added in issue #546) also catches the pattern before the test suite launches. - **When to revisit the LibLoader home:** If three or more components adopt this pattern, consider extracting a shared `$lib/types/lib-loader.ts` or a generic `DynamicImportLoader` type to avoid parallel type definitions across modules. diff --git a/frontend/eslint.config.js b/frontend/eslint.config.js index a22043f6..a56c710b 100644 --- a/frontend/eslint.config.js +++ b/frontend/eslint.config.js @@ -72,6 +72,20 @@ export default defineConfig( ] } }, + { + files: ['**/*.spec.ts', '**/*.test.ts'], + rules: { + 'no-restricted-syntax': [ + 'error', + { + selector: + "CallExpression[callee.object.name='vi'][callee.property.name='mock'] > Literal[value=/^pdfjs-dist/]", + message: + "Banned: vi.mock('pdfjs-dist', factory) causes a birpc teardown race in browser-mode specs — see ADR 012. Use the libLoader prop injection pattern instead." + } + ] + } + }, { plugins: { boundaries }, settings: { -- 2.49.1 From f938971292a54e204a549faac378a5b17d75a55f Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 12 May 2026 12:19:01 +0200 Subject: [PATCH 4/4] ci(unit-tests): add early grep check for banned vi.mock pdfjs-dist pattern 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 --- .gitea/workflows/ci.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 0b27866c..aa65e565 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -36,6 +36,14 @@ jobs: run: npm run lint working-directory: frontend + - 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 + - name: Run unit and component tests with coverage shell: bash run: | -- 2.49.1