Increase browser component test coverage to ≥ 80% on all metrics (statements, lines, branches, functions) #496

New Issue

marcel · 2026-05-09T18:43:54+02:00

marcel commented

2026-05-09 18:43:54 +02:00

Context

The vitest.client-coverage.config.ts currently enforces only a branches: 80 threshold for the browser (client) Vitest project. All four Istanbul metrics must reach ≥ 80 %. The current numbers are:

Metric	Covered	Total	% Now	Target
Statements	—	—	—	≥ 80 %
Lines	5 468	8 006	68.3%	≥ 80 %
Branches	2 303	4 267	54.0%	≥ 80 %
Functions	2 374	3 390	70.0%	≥ 80 %

Statements were not captured in the original snapshot; run npm run test:coverage on the client project to fill in that row.

Branches are the hardest metric to move (26 pp gap); lines and functions each need ~10–12 pp. A test that exercises a branch almost always covers the corresponding lines, statements, and function entry — so the branch-coverage work plan below drives all four metrics simultaneously.

Quick Win — Delete the dev demo route

src/routes/demo/ contains two scaffolding pages (+page.svelte, paraglide/+page.svelte) that exist only as a Paraglide i18n smoke-check from initial project setup. They are never navigated to in production or staging, carry no user-facing behaviour, and are already uncovered (0 %). Delete both files and the demo/ directory.

Files to delete:

src/routes/demo/+page.svelte
src/routes/demo/paraglide/+page.svelte

Main Work — Component tests for 0 % branch files

38 source files have branch coverage of exactly 0 %. They are the fastest path to the threshold because every branch hit counts from a zero baseline.

Priority order by branch count (largest first — most leverage):

File	Branches	Est. new hits at 70%
`src/lib/document/DocumentTopBar.svelte`	83	~58
`src/routes/persons/[id]/edit/PersonEditForm.svelte`	52	~36
`src/routes/admin/invites/+page.svelte`	52	~36
`src/routes/persons/[id]/PersonCard.svelte`	34	~24
`src/lib/ocr/SegmentationTrainingCard.svelte`	34	~24
`src/routes/persons/[id]/PersonDocumentList.svelte`	32	~22
`src/routes/geschichten/[id]/+page.svelte`	33	~23
`src/routes/persons/new/+page.svelte`	28	~20
`src/routes/aktivitaeten/+page.svelte`	20	~14
`src/routes/persons/[id]/CoCorrespondentsList.svelte`	12	~8
`src/lib/ocr/OcrTrigger.svelte`	12	~8
`src/routes/documents/new/FileSectionNew.svelte`	6	~4
`src/routes/enrich/+page.svelte`	16	~11
`src/routes/enrich/[id]/+page.svelte`	6	~4
`src/routes/forgot-password/+page.svelte`	4	~3
`src/routes/reset-password/+page.svelte`	8	~6
`src/routes/stammbaum/+page.svelte`	14	~10
`src/routes/profile/+page.svelte`	16	~11
`src/routes/profile/PasswordChangeForm.svelte`	6	~4
`src/routes/profile/PersonalInfoForm.svelte`	16	~11
`src/routes/users/[id]/+page.svelte`	18	~13
`src/routes/documents/bulk-edit/+page.svelte`	12	~8
`src/lib/document/DocumentStatusChip.svelte`	2	~1
`src/lib/document/DocumentViewer.svelte`	16	~11
`src/lib/person/PersonChip.svelte`	6	~4
`src/lib/person/PersonChipRow.svelte`	16	~11
`src/lib/person/PersonTypeBadge.svelte`	11	~8
`src/lib/shared/primitives/ExpandableText.svelte`	12	~8
`src/routes/+error.svelte`	4	~3
`src/routes/briefwechsel/CorrespondentSuggestionsDropdown.svelte`	18	~13
`src/routes/geschichten/[id]/edit/+page.svelte`	8	~6
`src/routes/geschichten/new/+page.svelte`	4	~3
`src/routes/admin/groups/+layout.svelte`	4	~3
`src/routes/admin/groups/new/+page.svelte`	16	~11
`src/routes/admin/ocr/global/+page.svelte`	4	~3
`src/routes/admin/tags/+layout.svelte`	4	~3
`src/routes/admin/users/+layout.svelte`	4	~3

Covering the top ~20 files at ~70 % branch hit rate yields roughly 400–450 additional branches. Combined with improvements on partially-covered files already above 0 %, all four thresholds should be reachable. If coverage progress stalls, revisit the exclude list in vitest.client-coverage.config.ts for any remaining dev-only artefacts.

Config change required

vitest.client-coverage.config.ts must be updated from:

thresholds: {
  branches: 80
}

to:

thresholds: {
  statements: 80,
  lines: 80,
  branches: 80,
  functions: 80
}

Testing approach

Use vitest-browser-svelte with render() from vitest-browser-svelte — not JSDOM — as established in the existing .svelte.test.ts files. Mock only what cannot run in a headless Chromium (server-side load functions, network calls via vi.mock). Test observable behaviour (rendered text, ARIA roles, slot/prop variants) rather than internal state.

Each test file lives alongside its component: DocumentTopBar.svelte.test.ts next to DocumentTopBar.svelte.

Acceptance Criteria

Given the client browser project is run with coverage,
When `npm run test:coverage` completes (or the CI coverage step),
Then statement, line, branch, and function coverage for the client project are each ≥ 80 %
  AND the vitest threshold block enforces all four metrics at 80
  AND the vitest threshold does not throw.

Given src/routes/demo/ files exist before this issue,
When the branch is merged,
Then src/routes/demo/+page.svelte and src/routes/demo/paraglide/+page.svelte are deleted
  AND no import anywhere in the codebase references the demo route.

Given a newly added .svelte.test.ts file,
When it is run in isolation,
Then it passes without touching the real backend or a real database
  AND it uses render() from vitest-browser-svelte (not @testing-library/svelte).

Out of scope

Server (Node) project coverage — tracked separately
E2E Playwright tests — those live in frontend/e2e/ and are not counted here
Raising any threshold above 80 % — that is a future decision

## Context The `vitest.client-coverage.config.ts` currently enforces only a `branches: 80` threshold for the browser (client) Vitest project. All four Istanbul metrics must reach ≥ 80 %. The current numbers are: | Metric | Covered | Total | % Now | Target | |--------------|---------|-------|-------|---------| | **Statements** | — | — | — | **≥ 80 %** | | **Lines** | 5 468 | 8 006 | 68.3% | **≥ 80 %** | | **Branches** | 2 303 | 4 267 | 54.0% | **≥ 80 %** | | **Functions**| 2 374 | 3 390 | 70.0% | **≥ 80 %** | > Statements were not captured in the original snapshot; run `npm run test:coverage` on the client project to fill in that row. Branches are the hardest metric to move (26 pp gap); lines and functions each need ~10–12 pp. A test that exercises a branch almost always covers the corresponding lines, statements, and function entry — so the branch-coverage work plan below drives all four metrics simultaneously. --- ## Quick Win — Delete the dev demo route `src/routes/demo/` contains two scaffolding pages (`+page.svelte`, `paraglide/+page.svelte`) that exist only as a Paraglide i18n smoke-check from initial project setup. They are never navigated to in production or staging, carry no user-facing behaviour, and are already uncovered (0 %). Delete both files and the `demo/` directory. **Files to delete:** - `src/routes/demo/+page.svelte` - `src/routes/demo/paraglide/+page.svelte` --- ## Main Work — Component tests for 0 % branch files 38 source files have branch coverage of exactly 0 %. They are the fastest path to the threshold because every branch hit counts from a zero baseline. Priority order by branch count (largest first — most leverage): | File | Branches | Est. new hits at 70% | |------|----------|----------------------| | `src/lib/document/DocumentTopBar.svelte` | 83 | ~58 | | `src/routes/persons/[id]/edit/PersonEditForm.svelte` | 52 | ~36 | | `src/routes/admin/invites/+page.svelte` | 52 | ~36 | | `src/routes/persons/[id]/PersonCard.svelte` | 34 | ~24 | | `src/lib/ocr/SegmentationTrainingCard.svelte` | 34 | ~24 | | `src/routes/persons/[id]/PersonDocumentList.svelte` | 32 | ~22 | | `src/routes/geschichten/[id]/+page.svelte` | 33 | ~23 | | `src/routes/persons/new/+page.svelte` | 28 | ~20 | | `src/routes/aktivitaeten/+page.svelte` | 20 | ~14 | | `src/routes/persons/[id]/CoCorrespondentsList.svelte` | 12 | ~8 | | `src/lib/ocr/OcrTrigger.svelte` | 12 | ~8 | | `src/routes/documents/new/FileSectionNew.svelte` | 6 | ~4 | | `src/routes/enrich/+page.svelte` | 16 | ~11 | | `src/routes/enrich/[id]/+page.svelte` | 6 | ~4 | | `src/routes/forgot-password/+page.svelte` | 4 | ~3 | | `src/routes/reset-password/+page.svelte` | 8 | ~6 | | `src/routes/stammbaum/+page.svelte` | 14 | ~10 | | `src/routes/profile/+page.svelte` | 16 | ~11 | | `src/routes/profile/PasswordChangeForm.svelte` | 6 | ~4 | | `src/routes/profile/PersonalInfoForm.svelte` | 16 | ~11 | | `src/routes/users/[id]/+page.svelte` | 18 | ~13 | | `src/routes/documents/bulk-edit/+page.svelte` | 12 | ~8 | | `src/lib/document/DocumentStatusChip.svelte` | 2 | ~1 | | `src/lib/document/DocumentViewer.svelte` | 16 | ~11 | | `src/lib/person/PersonChip.svelte` | 6 | ~4 | | `src/lib/person/PersonChipRow.svelte` | 16 | ~11 | | `src/lib/person/PersonTypeBadge.svelte` | 11 | ~8 | | `src/lib/shared/primitives/ExpandableText.svelte` | 12 | ~8 | | `src/routes/+error.svelte` | 4 | ~3 | | `src/routes/briefwechsel/CorrespondentSuggestionsDropdown.svelte` | 18 | ~13 | | `src/routes/geschichten/[id]/edit/+page.svelte` | 8 | ~6 | | `src/routes/geschichten/new/+page.svelte` | 4 | ~3 | | `src/routes/admin/groups/+layout.svelte` | 4 | ~3 | | `src/routes/admin/groups/new/+page.svelte` | 16 | ~11 | | `src/routes/admin/ocr/global/+page.svelte` | 4 | ~3 | | `src/routes/admin/tags/+layout.svelte` | 4 | ~3 | | `src/routes/admin/users/+layout.svelte` | 4 | ~3 | Covering the top ~20 files at ~70 % branch hit rate yields roughly 400–450 additional branches. Combined with improvements on partially-covered files already above 0 %, all four thresholds should be reachable. If coverage progress stalls, revisit the exclude list in `vitest.client-coverage.config.ts` for any remaining dev-only artefacts. --- ## Config change required `vitest.client-coverage.config.ts` must be updated from: ```ts thresholds: { branches: 80 } ``` to: ```ts thresholds: { statements: 80, lines: 80, branches: 80, functions: 80 } ``` --- ## Testing approach Use `vitest-browser-svelte` with `render()` from `vitest-browser-svelte` — not JSDOM — as established in the existing `.svelte.test.ts` files. Mock only what cannot run in a headless Chromium (server-side `load` functions, network calls via `vi.mock`). Test observable behaviour (rendered text, ARIA roles, slot/prop variants) rather than internal state. Each test file lives alongside its component: `DocumentTopBar.svelte.test.ts` next to `DocumentTopBar.svelte`. --- ## Acceptance Criteria ``` Given the client browser project is run with coverage, When `npm run test:coverage` completes (or the CI coverage step), Then statement, line, branch, and function coverage for the client project are each ≥ 80 % AND the vitest threshold block enforces all four metrics at 80 AND the vitest threshold does not throw. Given src/routes/demo/ files exist before this issue, When the branch is merged, Then src/routes/demo/+page.svelte and src/routes/demo/paraglide/+page.svelte are deleted AND no import anywhere in the codebase references the demo route. Given a newly added .svelte.test.ts file, When it is run in isolation, Then it passes without touching the real backend or a real database AND it uses render() from vitest-browser-svelte (not @testing-library/svelte). ``` --- ## Out of scope - Server (Node) project coverage — tracked separately - E2E Playwright tests — those live in `frontend/e2e/` and are not counted here - Raising any threshold above 80 % — that is a future decision

marcel added the P1-high cleanup test labels 2026-05-09 18:44:01 +02:00

marcel changed title from ~~Increase browser component test coverage to meet 80% branch threshold~~ to Increase browser component test coverage to ≥ 80% on all metrics (statements, lines, branches, functions)

2026-05-09 18:57:10 +02:00

marcel commented

2026-05-09 19:07:24 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Config change is already done. Commit 80ccc0f3 on this branch already expanded vitest.client-coverage.config.ts to enforce all four metrics at 80. The "Config change required" section of the issue spec is complete — the remaining work is purely writing the test files.
18 test files exist as established patterns. UploadZone.svelte.test.ts and TranscriptionPanelHeader.svelte.test.ts are the clearest models to follow. The patterns are sound: render() from vitest-browser-svelte, page.getByRole() / page.getByText() for assertions, vi.fn() for callback props.
afterEach(cleanup) is inconsistent. TranscriptionPanelHeader.svelte.test.ts calls cleanup after each test; UploadZone.svelte.test.ts does not. For complex components that modify global DOM state, missing cleanup causes test pollution. Pick one pattern and stick to it — I'd go with always including it.
data-testid crept into TranscriptionPanelHeader.svelte.test.ts (document.querySelector('[data-testid="mode-read"]')). This directly conflicts with the "test observable behaviour via ARIA roles" principle stated in the issue. Use getByRole('button', { name: /lesen/i }) instead — it tests what a screen reader announces and doesn't require production code to carry test attributes.
DocumentTopBar.svelte (303 lines, 83 branches) is the hardest file on the list. At 303 lines it already violates the 60-line splitting signal. Before writing tests, consider whether any branches can be moved into smaller child components — that would both simplify the test and improve the component design. If splitting is out of scope for this issue, at minimum write separate describe blocks per visual state: drawer open/closed, metadata filled/empty, mobile vs. desktop overflow.
requireAssertions: true is set in the config — good guard against accidentally empty tests. Make sure every test body has at least one await expect.element(...) call, not just a render().

Recommendations

Follow the UploadZone.svelte.test.ts pattern as the canonical template: describe per visual state, one it per observable outcome, vi.fn() for callbacks, getByRole / getByText for assertions. Avoid querySelector and data-testid.
Start with the smallest-branch files (4–6 branches: DocumentStatusChip, +error.svelte, PasswordChangeForm, FileSectionNew) to build momentum and confirm the test harness works before tackling DocumentTopBar (83 branches).
Add afterEach(() => cleanup()) to every new test file, and backfill it into UploadZone.svelte.test.ts.
For components with a canWrite prop (e.g. PersonCard.svelte), write separate tests for the authorized and unauthorized render — both branches count.
Mock vi.mock('$app/navigation') and vi.mock('$lib/api.server') at the top of any test file that renders a component that imports those modules — they are the most common import-time failures in browser tests here.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - **Config change is already done.** Commit `80ccc0f3` on this branch already expanded `vitest.client-coverage.config.ts` to enforce all four metrics at 80. The "Config change required" section of the issue spec is complete — the remaining work is purely writing the test files. - **18 test files exist as established patterns.** `UploadZone.svelte.test.ts` and `TranscriptionPanelHeader.svelte.test.ts` are the clearest models to follow. The patterns are sound: `render()` from `vitest-browser-svelte`, `page.getByRole()` / `page.getByText()` for assertions, `vi.fn()` for callback props. - **`afterEach(cleanup)` is inconsistent.** `TranscriptionPanelHeader.svelte.test.ts` calls `cleanup` after each test; `UploadZone.svelte.test.ts` does not. For complex components that modify global DOM state, missing cleanup causes test pollution. Pick one pattern and stick to it — I'd go with always including it. - **`data-testid` crept into `TranscriptionPanelHeader.svelte.test.ts`** (`document.querySelector('[data-testid="mode-read"]')`). This directly conflicts with the "test observable behaviour via ARIA roles" principle stated in the issue. Use `getByRole('button', { name: /lesen/i })` instead — it tests what a screen reader announces and doesn't require production code to carry test attributes. - **`DocumentTopBar.svelte` (303 lines, 83 branches) is the hardest file** on the list. At 303 lines it already violates the 60-line splitting signal. Before writing tests, consider whether any branches can be moved into smaller child components — that would both simplify the test and improve the component design. If splitting is out of scope for this issue, at minimum write separate `describe` blocks per visual state: drawer open/closed, metadata filled/empty, mobile vs. desktop overflow. - **`requireAssertions: true` is set in the config** — good guard against accidentally empty tests. Make sure every test body has at least one `await expect.element(...)` call, not just a `render()`. ### Recommendations - Follow the `UploadZone.svelte.test.ts` pattern as the canonical template: `describe` per visual state, one `it` per observable outcome, `vi.fn()` for callbacks, `getByRole` / `getByText` for assertions. Avoid `querySelector` and `data-testid`. - Start with the smallest-branch files (4–6 branches: `DocumentStatusChip`, `+error.svelte`, `PasswordChangeForm`, `FileSectionNew`) to build momentum and confirm the test harness works before tackling `DocumentTopBar` (83 branches). - Add `afterEach(() => cleanup())` to every new test file, and backfill it into `UploadZone.svelte.test.ts`. - For components with a `canWrite` prop (e.g. `PersonCard.svelte`), write separate tests for the authorized and unauthorized render — both branches count. - Mock `vi.mock('$app/navigation')` and `vi.mock('$lib/api.server')` at the top of any test file that renders a component that imports those modules — they are the most common import-time failures in browser tests here.

marcel commented

2026-05-09 19:07:38 +02:00

🏗️ Markus Keller — Application Architect

Observations

No architectural concerns. This issue is entirely test infrastructure — no new routes, no new domain modules, no new packages. No ADR or diagram update is required.
The standalone vitest.client-coverage.config.ts pattern is the right call. The comment in the file itself explains why it exists: Vitest 4 ignores per-project coverage overrides, so a root-level config block is the only reliable way to gate on all four Istanbul metrics for the browser project. This is a pragmatic workaround for a framework limitation, not an architectural smell.
The exclusion list is sound. src/lib/paraglide/**, src/lib/generated/**, src/hooks/**, and **/__mocks__/** are all correctly excluded — generated code, i18n outputs, and test infrastructure should not count toward coverage metrics.
Demo route deletion is a clean-up. src/routes/demo/ contains scaffolding pages with no user-facing behaviour. Deleting them reduces the measured surface area (fewer uncovered lines in the denominator), but more importantly removes dead code from the production bundle. No routing change, no redirect needed — the pages are navigated to by nothing.
The "38 zero-coverage files" list maps entirely to the view/presentation layer (routes and $lib components). There is no server-side logic in scope here. The test pyramid for this issue is: browser component tests only. That's appropriate — the server load functions and API calls are either already covered by backend tests or excluded by the src/lib/server/** exclude rule.

Recommendations

No structural changes needed before implementation begins. The test infrastructure (config, CI step, Istanbul provider) is already in place on this branch.
Confirm the exclude list in vitest.client-coverage.config.ts doesn't need updating after the demo route deletion — it currently matches on file patterns, not specific paths, so deletion alone won't affect it.
If the demo route deletion is done as a separate commit from the test file additions, add a brief commit message note (chore: delete dev-only demo route — reduces uncovered denominator in browser coverage) so the coverage number jump is traceable in git history and not mistaken for a test regression.

## 🏗️ Markus Keller — Application Architect ### Observations - **No architectural concerns.** This issue is entirely test infrastructure — no new routes, no new domain modules, no new packages. No ADR or diagram update is required. - **The standalone `vitest.client-coverage.config.ts` pattern is the right call.** The comment in the file itself explains why it exists: Vitest 4 ignores per-project `coverage` overrides, so a root-level config block is the only reliable way to gate on all four Istanbul metrics for the browser project. This is a pragmatic workaround for a framework limitation, not an architectural smell. - **The exclusion list is sound.** `src/lib/paraglide/**`, `src/lib/generated/**`, `src/hooks/**`, and `**/__mocks__/**` are all correctly excluded — generated code, i18n outputs, and test infrastructure should not count toward coverage metrics. - **Demo route deletion is a clean-up.** `src/routes/demo/` contains scaffolding pages with no user-facing behaviour. Deleting them reduces the measured surface area (fewer uncovered lines in the denominator), but more importantly removes dead code from the production bundle. No routing change, no redirect needed — the pages are navigated to by nothing. - **The "38 zero-coverage files" list maps entirely to the view/presentation layer** (routes and `$lib` components). There is no server-side logic in scope here. The test pyramid for this issue is: browser component tests only. That's appropriate — the server load functions and API calls are either already covered by backend tests or excluded by the `src/lib/server/**` exclude rule. ### Recommendations - No structural changes needed before implementation begins. The test infrastructure (config, CI step, Istanbul provider) is already in place on this branch. - Confirm the exclude list in `vitest.client-coverage.config.ts` doesn't need updating after the demo route deletion — it currently matches on file patterns, not specific paths, so deletion alone won't affect it. - If the demo route deletion is done as a separate commit from the test file additions, add a brief commit message note (`chore: delete dev-only demo route — reduces uncovered denominator in browser coverage`) so the coverage number jump is traceable in git history and not mistaken for a test regression.

marcel commented

2026-05-09 19:07:58 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

CI is currently failing on coverage thresholds. The branch already enforces all four metrics at 80%, but the current numbers (lines: 68.3%, branches: 54%, functions: 70%) are below those gates. That means npm run test:coverage fails, and the CI unit-tests job is red. This is the right pressure to have — but it means nothing can merge until all four thresholds are cleared.
The priority order by branch count is the correct strategy. Branches are the hardest metric to move (26pp gap vs ~10–12pp for lines and functions). A branch hit almost always co-moves lines and statements, so optimising for branches is the right leverage point.
Starting with the largest files carries hidden risk. DocumentTopBar.svelte has 83 branches in 303 lines. Writing tests for it first risks getting stuck on a complex component while easier wins exist. I'd invert the order: start with the smallest files (4–6 branches each) to validate the test setup, build commit momentum, and make the coverage numbers move visibly — then tackle the large ones when the approach is proven.
requireAssertions: true is a good safety net — it will fail any test that forgets to assert, preventing the empty-test anti-pattern. Make sure every new test makes at least one await expect.element(...) call (not just render()).
Some files on the list need mocking strategies that aren't obvious. OcrTrigger.svelte, SegmentationTrainingCard.svelte, and src/routes/admin/ocr/global/+page.svelte likely import server-only modules or call APIs directly. Before writing tests for these, confirm what needs to be mocked (vi.mock('$lib/api.server'), vi.mock('$app/navigation'), etc.) — otherwise the test file will compile but fail at import time.
No E2E coverage is claimed here — explicitly out of scope. But for the admin/OCR pages especially, note that component tests can only verify rendered state given props; they cannot verify the server-side data loading. That's an acceptable gap given the scope.

Recommendations

Sequencing: tackle files in this order: (1) pure display components with few branches (DocumentStatusChip, PersonTypeBadge, PersonChip, ExpandableText, +error.svelte), (2) medium-complexity route pages (forgot-password, reset-password, geschichten/new, geschichten/[id]/edit), (3) large complex components (DocumentTopBar, PersonEditForm, admin/invites). Each batch should visibly move the coverage numbers.
For each test file, write tests covering: (a) default/empty prop state, (b) populated state, (c) conditional UI triggered by boolean props (e.g. canWrite, loading flags, error states). These three cases together will hit 60–70% of branches in most components.
Add a note in the PR description about the baseline coverage numbers before and after — it makes the review faster and gives a clear picture of progress.
Accept criteria gap: the issue doesn't specify what "passes without touching the real backend" means in practice — a test that renders a component which makes no API calls trivially passes. For components that accept API data via props, the pattern of passing mock data directly is correct and already established in the existing tests.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - **CI is currently failing on coverage thresholds.** The branch already enforces all four metrics at 80%, but the current numbers (lines: 68.3%, branches: 54%, functions: 70%) are below those gates. That means `npm run test:coverage` fails, and the CI `unit-tests` job is red. This is the right pressure to have — but it means nothing can merge until all four thresholds are cleared. - **The priority order by branch count is the correct strategy.** Branches are the hardest metric to move (26pp gap vs ~10–12pp for lines and functions). A branch hit almost always co-moves lines and statements, so optimising for branches is the right leverage point. - **Starting with the largest files carries hidden risk.** `DocumentTopBar.svelte` has 83 branches in 303 lines. Writing tests for it first risks getting stuck on a complex component while easier wins exist. I'd invert the order: **start with the smallest files** (4–6 branches each) to validate the test setup, build commit momentum, and make the coverage numbers move visibly — then tackle the large ones when the approach is proven. - **`requireAssertions: true` is a good safety net** — it will fail any test that forgets to assert, preventing the empty-test anti-pattern. Make sure every new test makes at least one `await expect.element(...)` call (not just `render()`). - **Some files on the list need mocking strategies that aren't obvious.** `OcrTrigger.svelte`, `SegmentationTrainingCard.svelte`, and `src/routes/admin/ocr/global/+page.svelte` likely import server-only modules or call APIs directly. Before writing tests for these, confirm what needs to be mocked (`vi.mock('$lib/api.server')`, `vi.mock('$app/navigation')`, etc.) — otherwise the test file will compile but fail at import time. - **No E2E coverage is claimed here** — explicitly out of scope. But for the admin/OCR pages especially, note that component tests can only verify rendered state given props; they cannot verify the server-side data loading. That's an acceptable gap given the scope. ### Recommendations - **Sequencing**: tackle files in this order: (1) pure display components with few branches (`DocumentStatusChip`, `PersonTypeBadge`, `PersonChip`, `ExpandableText`, `+error.svelte`), (2) medium-complexity route pages (`forgot-password`, `reset-password`, `geschichten/new`, `geschichten/[id]/edit`), (3) large complex components (`DocumentTopBar`, `PersonEditForm`, `admin/invites`). Each batch should visibly move the coverage numbers. - For each test file, write tests covering: (a) default/empty prop state, (b) populated state, (c) conditional UI triggered by boolean props (e.g. `canWrite`, loading flags, error states). These three cases together will hit 60–70% of branches in most components. - Add a note in the PR description about the **baseline coverage numbers before and after** — it makes the review faster and gives a clear picture of progress. - **Accept criteria gap**: the issue doesn't specify what "passes without touching the real backend" means in practice — a test that renders a component which makes no API calls trivially passes. For components that accept API data via props, the pattern of passing mock data directly is correct and already established in the existing tests.

marcel commented

2026-05-09 19:08:10 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

CI infrastructure is already in place. The unit-tests job in ci.yml runs npm run test:coverage and uploads coverage reports as artifacts using actions/upload-artifact@v4 (current version — good, not the deprecated v3). The Playwright container is pinned to mcr.microsoft.com/playwright:v1.58.2-noble — reproducible and specific. No new infrastructure is needed for this issue.
The coverage step correctly blocks the job. npm run test:coverage calls vitest run -c vitest.client-coverage.config.ts --coverage, and that config now enforces all four thresholds at 80%. When any threshold is unmet, Vitest exits non-zero, the step fails, and the job fails. CI is currently red on this branch for that reason. This is the right gate — the branch cannot be merged until all thresholds are cleared.
Coverage artifacts are uploaded with if: always() — this is correct. Even when the coverage step fails, the reports are preserved and downloadable. Reviewers can inspect the lcov and text reports in the Gitea artifacts to see exactly which files and branches are still uncovered. This is a useful debugging tool during the implementation phase.
No duplicate CI config needed. The standalone vitest.client-coverage.config.ts is invoked explicitly by test:coverage — it does not conflict with the main vitest.config.ts used by npm test. The two configs run separate test phases with no overlap.

Recommendations

When the implementation is complete and coverage has crossed 80% on all four metrics, do a final npm run test:coverage locally and confirm the exit code is 0 before opening the PR. The CI will confirm it, but catching it locally saves a round-trip.
The lcov reporter output in coverage/client/ is the one most useful for inspecting uncovered lines. Download the artifact from the CI run, open lcov-report/index.html in a browser, and use it to identify which specific branches in each file are still uncovered — much faster than reading the text reporter output in CI logs.
No changes to docker-compose.yml, Caddyfile, or production infrastructure are required. This is a pure dev/CI concern.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - **CI infrastructure is already in place.** The `unit-tests` job in `ci.yml` runs `npm run test:coverage` and uploads coverage reports as artifacts using `actions/upload-artifact@v4` (current version — good, not the deprecated v3). The Playwright container is pinned to `mcr.microsoft.com/playwright:v1.58.2-noble` — reproducible and specific. No new infrastructure is needed for this issue. - **The coverage step correctly blocks the job.** `npm run test:coverage` calls `vitest run -c vitest.client-coverage.config.ts --coverage`, and that config now enforces all four thresholds at 80%. When any threshold is unmet, Vitest exits non-zero, the step fails, and the job fails. CI is currently red on this branch for that reason. This is the right gate — the branch cannot be merged until all thresholds are cleared. - **Coverage artifacts are uploaded with `if: always()`** — this is correct. Even when the coverage step fails, the reports are preserved and downloadable. Reviewers can inspect the `lcov` and `text` reports in the Gitea artifacts to see exactly which files and branches are still uncovered. This is a useful debugging tool during the implementation phase. - **No duplicate CI config needed.** The standalone `vitest.client-coverage.config.ts` is invoked explicitly by `test:coverage` — it does not conflict with the main `vitest.config.ts` used by `npm test`. The two configs run separate test phases with no overlap. ### Recommendations - When the implementation is complete and coverage has crossed 80% on all four metrics, do a final `npm run test:coverage` locally and confirm the exit code is 0 before opening the PR. The CI will confirm it, but catching it locally saves a round-trip. - The `lcov` reporter output in `coverage/client/` is the one most useful for inspecting uncovered lines. Download the artifact from the CI run, open `lcov-report/index.html` in a browser, and use it to identify which specific branches in each file are still uncovered — much faster than reading the text reporter output in CI logs. - No changes to `docker-compose.yml`, `Caddyfile`, or production infrastructure are required. This is a pure dev/CI concern.

marcel commented

2026-05-09 19:08:22 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Observations

No security vulnerabilities introduced or remediated. This issue is test infrastructure and dead-code cleanup — no new endpoints, no authentication changes, no new data flows. No OWASP Top 10 concerns apply.
Demo route deletion is a minor positive. src/routes/demo/ pages are in the production build (SvelteKit includes all routes by default). They serve no user-facing purpose and represent dead code in the attack surface. Deleting them is the right call — less code, less exposure, smaller bundle.
One security-relevant observation for the test authoring phase: several files on the list render UI conditionally based on authorization state. PersonCard.svelte takes a canWrite prop; DocumentTopBar.svelte likely does too. When writing tests for these components, test both the authorized and unauthorized rendering paths explicitly. This validates that write-action controls (edit buttons, delete links, etc.) are not rendered for users without the appropriate permission — a pattern that matters even at the component level, since a rendered button that POSTs to a @RequirePermission-protected endpoint is a potential CSRF surface if the permission check fails.
No test credentials or secrets are introduced by this work — tests mock API calls via vi.mock() and pass data as props. No real backend is touched. This is the correct pattern.

Recommendations

For components with authorization-conditional rendering, the test should explicitly assert that the write-control (button, link, form) is absent in the unauthorized render, not just present in the authorized one. Example:
```
it('does not render edit button when canWrite is false', async () => {
    render(PersonCard, { props: { person: makePerson(), canWrite: false } });
    expect(document.querySelector('a[href*="/edit"]')).toBeNull();
});
```
This catches future regressions where a conditional {#if canWrite} is accidentally removed.
No security review is needed for this PR. From my angle, implementation can proceed.

## 🔐 Nora "NullX" Steiner — Application Security Engineer ### Observations - **No security vulnerabilities introduced or remediated.** This issue is test infrastructure and dead-code cleanup — no new endpoints, no authentication changes, no new data flows. No OWASP Top 10 concerns apply. - **Demo route deletion is a minor positive.** `src/routes/demo/` pages are in the production build (SvelteKit includes all routes by default). They serve no user-facing purpose and represent dead code in the attack surface. Deleting them is the right call — less code, less exposure, smaller bundle. - **One security-relevant observation for the test authoring phase:** several files on the list render UI conditionally based on authorization state. `PersonCard.svelte` takes a `canWrite` prop; `DocumentTopBar.svelte` likely does too. When writing tests for these components, **test both the authorized and unauthorized rendering paths explicitly**. This validates that write-action controls (edit buttons, delete links, etc.) are not rendered for users without the appropriate permission — a pattern that matters even at the component level, since a rendered button that POSTs to a `@RequirePermission`-protected endpoint is a potential CSRF surface if the permission check fails. - **No test credentials or secrets are introduced** by this work — tests mock API calls via `vi.mock()` and pass data as props. No real backend is touched. This is the correct pattern. ### Recommendations - For components with authorization-conditional rendering, the test should explicitly assert that the write-control (button, link, form) is **absent** in the unauthorized render, not just present in the authorized one. Example: ```typescript it('does not render edit button when canWrite is false', async () => { render(PersonCard, { props: { person: makePerson(), canWrite: false } }); expect(document.querySelector('a[href*="/edit"]')).toBeNull(); }); ``` This catches future regressions where a conditional `{#if canWrite}` is accidentally removed. - No security review is needed for this PR. From my angle, implementation can proceed.

marcel commented

2026-05-09 19:08:42 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue spec is unusually well-written for an internal engineering ticket — quantified baseline numbers, a prioritised file list with branch estimates, working test commands, and Gherkin acceptance criteria. Most of what a developer needs is here. A few gaps are worth flagging before work starts.

Gap 1 — Statements baseline is missing. The issue explicitly says "Statements were not captured in the original snapshot; run npm run test:coverage on the client project to fill in that row." This means the starting statements percentage is unknown. Since statements is now a gated threshold (≥ 80%), implementation could finish all branch work and still fail CI if statements happen to land below 80. The statements number should be captured once at the start of the issue and noted so there's no surprise at the end.

Gap 2 — The file list is a point-in-time snapshot. The 38 zero-coverage files were identified at a specific moment. If new .svelte files were added to the project after that snapshot (or are added during this issue's implementation), they won't be on the list — but they will count against the threshold. The acceptance criteria should read: "All four thresholds pass as measured by npm run test:coverage at the time of the PR review", not "all 38 files have tests". (The issue's Gherkin AC already expresses this correctly — this is just a reminder not to treat the file list as exhaustive.)

Gap 3 — "If coverage progress stalls, revisit the exclude list" is undefined. The fallback strategy is mentioned but not specified. What would a legitimate addition to the exclude list look like? (Admin/OCR pages with complex server dependencies? Generated component wrappers?) Without a criterion, this becomes an open door to padding the threshold by excluding legitimate source code. A concrete rule would be: "Files may be added to the exclude list only if they contain no user-observable behaviour independent of server data".

Gap 4 — No definition of "done" for the demo route deletion. The AC says the files are deleted and no import references them. A grep -r "routes/demo" check would confirm this. Consider adding it as an explicit verification step.

Recommendations

Capture the statements baseline number before writing any tests and add it to the issue body.
Treat the 38-file list as a priority guide, not a complete checklist — the AC (threshold passes) is the authoritative "done" condition.
Decide up-front whether the admin/OCR pages (SegmentationTrainingCard, src/routes/admin/ocr/global/+page.svelte) are in scope for this issue or whether they should be excluded from coverage measurement due to dependency complexity. Document that decision in the issue or PR description.

## 📋 Elicit — Requirements Engineer ### Observations The issue spec is unusually well-written for an internal engineering ticket — quantified baseline numbers, a prioritised file list with branch estimates, working test commands, and Gherkin acceptance criteria. Most of what a developer needs is here. A few gaps are worth flagging before work starts. **Gap 1 — Statements baseline is missing.** The issue explicitly says "Statements were not captured in the original snapshot; run `npm run test:coverage` on the client project to fill in that row." This means the starting statements percentage is unknown. Since statements is now a gated threshold (≥ 80%), implementation could finish all branch work and still fail CI if statements happen to land below 80. The statements number should be captured once at the start of the issue and noted so there's no surprise at the end. **Gap 2 — The file list is a point-in-time snapshot.** The 38 zero-coverage files were identified at a specific moment. If new `.svelte` files were added to the project after that snapshot (or are added during this issue's implementation), they won't be on the list — but they will count against the threshold. The acceptance criteria should read: *"All four thresholds pass as measured by `npm run test:coverage` at the time of the PR review"*, not "all 38 files have tests". (The issue's Gherkin AC already expresses this correctly — this is just a reminder not to treat the file list as exhaustive.) **Gap 3 — "If coverage progress stalls, revisit the exclude list" is undefined.** The fallback strategy is mentioned but not specified. What would a legitimate addition to the exclude list look like? (Admin/OCR pages with complex server dependencies? Generated component wrappers?) Without a criterion, this becomes an open door to padding the threshold by excluding legitimate source code. A concrete rule would be: _"Files may be added to the exclude list only if they contain no user-observable behaviour independent of server data"_. **Gap 4 — No definition of "done" for the demo route deletion.** The AC says the files are deleted and no import references them. A `grep -r "routes/demo"` check would confirm this. Consider adding it as an explicit verification step. ### Recommendations - Capture the statements baseline number before writing any tests and add it to the issue body. - Treat the 38-file list as a priority guide, not a complete checklist — the AC (threshold passes) is the authoritative "done" condition. - Decide up-front whether the admin/OCR pages (`SegmentationTrainingCard`, `src/routes/admin/ocr/global/+page.svelte`) are in scope for this issue or whether they should be excluded from coverage measurement due to dependency complexity. Document that decision in the issue or PR description.

marcel commented

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

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

No UI or visual changes are introduced — this issue is entirely about test coverage. No design review of rendered output is required.
One meaningful UX opportunity in the new test files. When writing component tests using getByRole() and getByText(), each test implicitly verifies that the component has the correct semantic structure to be found by role. A test that uses getByRole('heading'), getByRole('button', { name: /save/i }), or getByRole('status') is both a behavior test and an implicit accessibility check — the component must have the correct ARIA role for the assertion to pass. This is a free accessibility audit layered into coverage work.
Avoid getByText() for non-translatable strings. Several components use Paraglide i18n (m.save(), m.edit(), etc.). In browser tests, Paraglide renders the actual German string at test time. Tests like getByText('Speichern') work today but break if the German copy changes. Prefer getByRole('button', { name: /speichern/i }) with a loose regex, or use getByTestId as a last resort for interactive elements with no unique accessible name.
Senior audience concern with the existing components under test. Several zero-coverage files render UI for the 60+ audience (profile forms, person detail, document top bar). While tests verify branch logic, they don't verify font sizes, touch target sizes, or contrast. That's expected for unit tests — but flag these components for an accessibility E2E check when the E2E suite is next extended.

Recommendations

In new test files, prefer getByRole assertions that verify meaningful ARIA structure over getByText for UI strings. Example:

// Prefer this — role check is stable across copy changes
await expect.element(page.getByRole('button', { name: /bearbeiten/i })).toBeVisible();

// Avoid this — breaks if the German copy is corrected or expanded
await expect.element(page.getByText('Bearbeiten')).toBeVisible();

No design changes, no accessibility blockers for this issue. From my angle, implementation can proceed.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - **No UI or visual changes are introduced** — this issue is entirely about test coverage. No design review of rendered output is required. - **One meaningful UX opportunity in the new test files.** When writing component tests using `getByRole()` and `getByText()`, each test implicitly verifies that the component has the correct semantic structure to be found by role. A test that uses `getByRole('heading')`, `getByRole('button', { name: /save/i })`, or `getByRole('status')` is both a behavior test and an implicit accessibility check — the component must have the correct ARIA role for the assertion to pass. This is a free accessibility audit layered into coverage work. - **Avoid `getByText()` for non-translatable strings.** Several components use Paraglide i18n (`m.save()`, `m.edit()`, etc.). In browser tests, Paraglide renders the actual German string at test time. Tests like `getByText('Speichern')` work today but break if the German copy changes. Prefer `getByRole('button', { name: /speichern/i })` with a loose regex, or use `getByTestId` as a last resort for interactive elements with no unique accessible name. - **Senior audience concern with the existing components under test.** Several zero-coverage files render UI for the 60+ audience (profile forms, person detail, document top bar). While tests verify branch logic, they don't verify font sizes, touch target sizes, or contrast. That's expected for unit tests — but flag these components for an accessibility E2E check when the E2E suite is next extended. ### Recommendations - In new test files, prefer `getByRole` assertions that verify meaningful ARIA structure over `getByText` for UI strings. Example: ```typescript // Prefer this — role check is stable across copy changes await expect.element(page.getByRole('button', { name: /bearbeiten/i })).toBeVisible(); // Avoid this — breaks if the German copy is corrected or expanded await expect.element(page.getByText('Bearbeiten')).toBeVisible(); ``` - No design changes, no accessibility blockers for this issue. From my angle, implementation can proceed.

marcel referenced this issue from a commit

2026-05-11 15:14:57 +02:00

test(geschichten): cover authorName + publishedAt branches

marcel referenced this issue from a commit

2026-05-11 15:14:57 +02:00

test(persons): cover personType icon paths + life-date branches

marcel referenced this issue from a commit

2026-05-11 15:14:57 +02:00

test(annotation): expand AnnotationLayer prop-combo coverage

marcel referenced this issue from a commit

2026-05-11 15:14:57 +02:00

test(discussion): expand CommentThread coverage further