test(legibility): verify e2e suite covers every critical user journey #405
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
Part of Epic #402 — Pre-flight. This is TEST-3: confirm that the Playwright e2e suite has at least one test per critical user journey, so the big-bang refactor (Epic 4) can be validated end-to-end and not just at the unit level.
Per the Legibility Rubric, this addresses C8.2 (Major).
Critical user journeys (must each have ≥1 e2e)
These are the journeys Anja and Tobias will mentally test when they evaluate the codebase. Each must work post-refactor.
/register,/login,/logout/documents/new,/documents/documents/[id]/edit/admin/tags,/documents/persons/new,/persons/[id]/documentswith query params/documents/[id]transcribe panel/geschichten/new,/geschichten/[id]/briefwechsel?personA=...&personB=.../admin/*/admin/usersas non-admin/admin/systemimport pathMethod
For each journey, search
frontend/e2e/for an existing test that exercises it. Use grep, not memory.Required output
Add
docs/audits/e2e-coverage-report.mdcontaining:Plus a summary: how many journeys are covered today vs needed before refactor.
Acceptance criteria
docs/audits/e2e-coverage-report.mdexists with all 12 journeys mappedcd frontend && npx playwright testruns green (excluding any documented quarantined tests)Dependency
Soft dependency on AUDIT-1 (#388) for findings about existing e2e suite shape.
Definition of Done
PR merged; closing comment summarizes the final coverage table.
Dispatch
Best done by an agent with full file-edit access. Memory: dev admin credentials are
admin@familyarchive.local/admin123.👨💻 Felix Brandt — Senior Fullstack Developer
Observations
I grepped every spec file in
frontend/e2e/against the 12 journeys. Here's what I found before writing a single line:auth.spec.ts/registernever appears in any specdocuments.spec.ts"Document creation"documents.spec.ts"Document editing"admin.spec.ts"tag management"persons.spec.ts"Person creation"stammbaum.spec.tsare entirelytest.skip()documents.spec.tstranscription.spec.tsgeschichten.spec.tskorrespondenz.spec.ts/briefwechsel— the route is actually/korrespondenz; the spec covers the real URL finenotification-deep-link.spec.ts/admin/*permissions.spec.tsCritical finding: The CI workflow (
.gitea/workflows/ci.yml) runs unit tests, backend tests, and OCR tests but never runs Playwright. The e2e suite produces zero signal on every push. This is confirmed infrontend/e2e/CLAUDE.md: "E2E tests are not currently run in CI."Recommendations
/admin→ System tab → upload a fixture ODS → poll for status message → verify at least one imported document appears in the list. Create a minimal 2-row ODS fixture ine2e/fixtures/.PersonTypeaheadand adds a tag viaTagInput, then verifies both appear on the detail page after save./?tag=....stammbaum.spec.tsis fully skipped withtest.skip()and references issue #363 for "Playwright Chromium not installed in CI." That issue predates the current CI setup which already usesmcr.microsoft.com/playwright:v1.58.2-noble. Unskip those tests; Chromium is available.ci.ymlthat brings up Docker Compose (db + minio + backend), starts the frontend, runsnpx playwright test, and uploads the HTML report as an artifact. Without this, even a fully written suite provides zero refactor safety net./registeris not a public route in this project (users are created by admin), remove journey 1's register step from the issue table or note it explicitly as "admin-only user creation via/admin/users/new" — the test exists for that path inadmin.spec.ts.Open Decisions
on:trigger inci.yml.🏛️ Markus Keller — Application Architect
Observations
The issue frames this as a pre-flight for a big-bang refactor (Epic 4). From an architecture standpoint, the value of this ticket is entirely determined by whether the resulting e2e suite actually runs in CI — and right now it does not.
The current CI pipeline (
.gitea/workflows/ci.yml) has three jobs: unit tests (frontend/Playwright container), OCR tests, and backend unit tests. Zero Playwright e2e jobs. Zero Docker Compose startup. Thefrontend/e2e/CLAUDE.mddocuments this explicitly: "E2E tests are not currently run in CI." This means the suite is tested locally by one developer on one machine. That is not a safety net for a refactor.The missing journeys (grepped, not from memory):
admin.spec.tsrenames a tag but never creates one or assigns it to a document via the edit form.documents.spec.tschanges the title but never touches thePersonTypeaheadorTagInput.stammbaum.spec.tsis entirely skipped viatest.skip().notification-deep-link.spec.tscovers the deep-link scroll after arrival but never triggers an event, observes the bell badge increment, or clicks the dropdown.Route name discrepancy: The issue table says journey 9 uses
/briefwechsel. The actual route in production is/korrespondenz(confirmed inkorrespondenz.spec.tsand the frontend CLAUDE.md route structure). This needs to be corrected in the issue table — it is not a test gap, but it will confuse the implementor.Recommendations
docker compose up -d db minio, backend start,npm run dev,npx playwright test. Without this, the acceptance criteria "Playwright runs green" only means "it runs green on one developer's laptop."stammbaum.spec.tsis a signal, not a gap. The skip comment says "Playwright Chromium not installed in CI — see issue #363." The current CI job usesmcr.microsoft.com/playwright:v1.58.2-noblewhich pre-installs Chromium. Issue #363 is stale. The tests should be unskipped and validated before this issue closes./briefwechsel— update to/korrespondenz. Small thing, prevents future confusion.Open Decisions
on:stanza inci.ymland whether the suite genuinely gates merges.🔍 Sara Holt — QA Engineer & Test Strategist
Observations
I read every spec file. The existing suite has strong bones — auth, transcription, admin lifecycle, and geschichten are well-structured with API-seeded data in
beforeAll. But several journeys are incompletely covered and the suite is not running in CI.Coverage audit against the 12 journeys:
documents.spec.ts"Document editing" only changes the title. No test touches the sender typeahead or tag chips.admin.spec.tstag tests rename an existing tag but never create, never assign to a document, never verify that searching by tag filter returns that document.stammbaum.spec.tsusestest.skip()on the entire describe block. The skip comment references issue #363 ("Playwright Chromium not installed in CI") — but the current CI container ismcr.microsoft.com/playwright:v1.58.2-noblewhich ships Chromium. These tests can be unskipped now.notification-deep-link.spec.tsverifies that a pre-existing deep link scrolls to the correct comment, but never tests the bell badge incrementing or the dropdown opening. The trigger → bell update half is untested.geschichten.spec.tscovers create + publish + multi-person filter. Missing: actually linking a document or person during story edit and verifying the link appears on the detail page.Structural concern — test isolation: Several describe blocks in
documents.spec.tsdepend on data created by previous describes (e.g. "Document editing" searches for "E2E Testbrief" created by "Document creation"). This is implicit inter-test coupling. It works when tests run sequentially, but makes individual test runs and debugging fragile. ThebeforeAll+ API-seed pattern used intranscription.spec.tsandannotations.spec.tsis the right model — it should be applied todocuments.spec.tsfor the edit journey.CI gap: E2E is not in the pipeline. The acceptance criterion says "
cd frontend && npx playwright testruns green" — this is a manual verification requirement, not a CI gate. That is not a valid Definition of Done for a pre-refactor safety net.Recommendations
Titel,Datum,Absendercolumns), upload it via the admin system tab, wait for the status message "Import abgeschlossen" (or equivalent), then assert at least one of the fixture persons appears in the persons list. Seed data must be deterministic — use unique title strings likeE2E-Import-+ timestamp.admin.spec.tsto create a new tag ("E2E Schlagwort"), then in a separate test edit a document and assign that tag, then verify/?tag=E2E+Schlagwortreturns that document. Clean up by deleting the tag at the end.requestfixture to POST a comment as admin, then refresh the admin page and check that the bell badge count is > 0. The dropdown interaction is a second test.stammbaum.spec.ts: Removetest.skip(). The referenced CI blocker is resolved by the current container image. Run the tests and fix any failures before closing this issue.documents.spec.tsedit tests: Move the "Document editing" test to its ownbeforeAllthat seeds a known document via API. Remove the dependency on the "Document creation" describe having run first.ci.yml. Without it, the acceptance criterion "runs green" is not machine-verifiable and cannot gate the Epic 4 refactor.Open Decisions
workflow_dispatchjob or a nightly schedule, with the PR gate covering journeys 1–11 only? This is a scheduling choice with a real cost tradeoff.🔐 Nora "NullX" Steiner — Security Engineer
Observations
This is a test coverage issue, not a direct security implementation ticket — but writing new e2e tests for these journeys creates permanent opportunities to embed security assertions. Here's what I see from a security testing perspective after reading the existing specs:
Existing security test coverage (good):
permissions.spec.ts: confirms non-admin users cannot access/documents/newdirectly (redirect away) — this is the right pattern.auth.spec.ts: verifies unauthenticated users are redirected to/loginfor protected routes.storageStatefor read-only user, confirming the Annotieren button is absent for users lackingANNOTATE_ALL.Missing security coverage in the planned new tests:
Journey 11 (Permission gate): The issue says "non-admin user blocked from
/admin/*."permissions.spec.tscurrently tests/documents/newredirect for read-only users and write controls visibility, but it does not test that a read-only user navigating directly to/admin/usersis blocked. This is the explicit journey-11 requirement. I searched — no test hits/admin/userswith a non-admin session. Add it.Journey 12 (Mass import): The ODS upload endpoint is an admin-only operation. When writing the mass import test, add a companion test: attempt the ODS upload as the
readeruser (no ADMIN permission) and verify it returns a 403 or redirects away. This prevents a future permission regression where the import endpoint loses its@RequirePermissionguard.Journey 10 (Notifications): Notifications are per-user. When testing the bell, add a check that user A cannot read user B's notifications by direct API call:
GET /api/notifications/{id}as the wrong user should return 403, not 200. This is a basic IDOR check for the notification domain.The dev admin credentials
admin@familyarchive.local / admin123appear in the issue body as plaintext. They also appear infrontend/e2e/CLAUDE.mdand the project memory file. For a family archive with private documents this is acceptable for a local dev seed — but these credentials must never be used in the production environment. Flagging for awareness, not a blocker.upload-artifact@v3inci.yml(line 45): This is the deprecated version.@v3has had security patches stopped. It should be@v4. Not directly related to this issue's scope, but worth fixing in the same PR since you'll be touchingci.ymlto add the Playwright job.Recommendations
permissions.spec.ts: a test that navigates areadersession directly to/admin/usersand asserts it does NOT load the admin panel (redirect or error).readeruser cannot trigger the import.GET /api/notifications/{someId}using therequestfixture with reader session returns 403.upload-artifact@v3→@v4in the same CI PR.No open decisions from the security side — these are all concrete, unambiguous additions.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Observations
My angle on this issue is: when new e2e tests are written for these journeys, they should include axe accessibility assertions on the pages they visit. The existing suite does this in several specs —
transcription.spec.ts,korrespondenz.spec.ts,geschichten.spec.ts— but inconsistently.What the existing specs do well:
transcription.spec.tshas a dedicated axe test at the end of the describe block — good pattern.geschichten.spec.tsrunsAxeBuilderwithwcag2a+wcag2aatags on the index.korrespondenz.spec.tsruns inline axe after every major state transition (empty, single-person, bilateral).notification-deep-link.spec.tsruns axe after the deep-link resolves.Where new tests should follow suit:
PersonTypeaheaddropdown interaction has no accessibility test. Typeahead comboboxes are a common source of ARIA attribute errors (missingrole="combobox", incorrectaria-expanded,aria-activedescendantnot set). Scanning this state is important.stammbaum.spec.tstests are skipped entirely, meaning the relationship add-form has never been axe-scanned in CI.Viewport coverage:
notification-deep-link.spec.tsis the only spec that tests at both 320px and 1440px. The senior-focused personas (Anja) will evaluate the refactored codebase on tablet — the critical journeys (document create, document edit, person detail) should be spot-checked at 768px and 320px in at least one test each.Recommendations
AxeBuilder.withTags(['wcag2a', 'wcag2aa']).analyze()assertion after the main interaction lands the user on the result page. Copy thegeschichten.spec.tspattern.aria-liveregions) are checked by axe.{ width: 768, height: 1024 }to thenotification-deep-link.spec.tsviewport loop (it currently only tests 320 and 1440 — tablet is the most likely device for the transcriber audience and is missing).No open decisions from the UX side — these are standard practice additions that cost one extra assertion per new test.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Observations
The biggest infrastructure finding for this issue is confirmed in two places:
frontend/e2e/CLAUDE.mdsays "E2E tests are not currently run in CI" and.gitea/workflows/ci.ymlhas no Playwright job. The acceptance criterion says the suite "runs green" — but without CI integration, "green" only means "green on the implementor's laptop the day they close the issue."Specific CI issues I found:
upload-artifact@v3(line 45 inci.yml): Deprecated.@v3is end-of-life and no longer receives security patches. Should be@v4. This should be fixed in the same PR.No Playwright e2e job in
ci.yml. Adding one requires:dbandminioas service containers (or viadocker compose up -d)npx playwright testThe Playwright container image
mcr.microsoft.com/playwright:v1.58.2-nobleis already used in the unit-tests job, so Chromium is available — no additional browser install step needed.The NAS runner constraint: The backend unit test job sets
DOCKER_API_VERSION: "1.43"specifically because "NAS runner runs Docker 24.x (max API 1.43)." The Playwright e2e job will also need this env var if it uses Testcontainers or Docker Compose directly. Worth noting in the new job.Sequential e2e tests (workers: 1):
playwright.config.tssetsfullyParallel: falseandworkers: 1because tests share a session cookie. This is the right choice for session-dependent tests, but it means the entire suite runs serially. CI pipeline time impact: typically 5–10 minutes for a suite this size on a single worker.test-results/artifacts: The existing unit test job uploadstest-results/screenshots/. The Playwright e2e job should uploadtest-results/(the Playwright HTML report and all screenshots) usingupload-artifact@v4withretention-days: 7.Recommendations
ci.ymlin this PR — not as a follow-up. Without it, the refactor safety net is a fiction. Sample structure:upload-artifact@v3→@v4in the existing unit-tests job in the same PR.Open Decisions
docker compose -f docker-compose.ci.yml up -dif the runner supports Docker-in-Docker (which requiresprivileged: true— check NAS runner capabilities). The simpler path is Java + service containers without DinD. Which approach is available on the NAS runner?📋 Elicit — Requirements Engineer
Observations
Reading this issue as a requirements document, the acceptance criteria are mostly clear and testable — but there are gaps between the journey table and what the codebase actually exposes.
Requirements quality issues:
Journey 1 mentions "register" but no
/registerroute exists. Users in this system are created by admins via/admin/users/new— there is no self-registration flow. The journey table either needs to remove "register" or acknowledge that user creation is admin-driven. As written, an implementor would search for/registertests, find none, and be unsure whether this is a gap or an intentional omission.admin.spec.tsalready covers admin user creation — that is the equivalent of "register" in this system.Journey 9 route is wrong. The table says
/briefwechsel?personA=...&personB=.... The actual route uses/korrespondenz?senderId=...&receiverId=...(confirmed inkorrespondenz.spec.tsand the frontend CLAUDE.md). The parameter names also differ. This is a stale reference — the route was likely renamed at some point. The tests cover the real route correctly, but the discrepancy will cause confusion when the implementor cross-references the issue.Journey 10 is vague about "trigger event." The acceptance criterion implies the full bell notification cycle: trigger → bell increments → click → read detail. But "trigger event" is undefined. What event? A transcription block comment? A document upload? Without knowing which event to trigger, the implementor cannot write the test. The
notification-deep-link.spec.tsalready seeds data via API — the same approach should work for the bell test, but the issue should specify which notification event to use.Journey 5 says "add relationship to existing person." The
stammbaum.spec.tstests (currently fully skipped) cover relationship addition viapersons/[id]/edit. The issue table should reference this path explicitly — the relationship UI is on the person edit page, not a separate route.The "Required output" section asks for
docs/audits/e2e-coverage-report.md. The issue's own CLAUDE.md project instructions say "NEVER create documentation files (*.md) or README files unless explicitly requested by the User." This is explicitly requested, so it is fine — but the implementor should be aware that this file will be untracked by the project's file creation rules and is expected as a deliberate deliverable.The acceptance criteria do not specify what "flaky/skipped test" treatment looks like. It says "either fix or schedule (note in report)." The
stammbaum.spec.tsskip is documented with a reference to issue #363. Does that count as "scheduled"? The issue should clarify whether an existing open issue is sufficient or whether a new P1-labeled issue is required.Recommendations
/admin/users/newas the route. The coverage exists already./korrespondenz?senderId=...&receiverId=..../persons/[id]/edit(not a separate route) and clarify thatstammbaum.spec.tsis the target file.No open decisions — these are all specification clarifications that can be applied as quick edits to the issue body before implementation starts.
🗳️ Decision Queue — Action Required
3 decisions need your input before implementation starts.
Infrastructure
E2E in CI: every push or PRs to main only? Running the full Docker Compose stack (db + minio + backend + SvelteKit dev server) adds ~5–10 minutes to the pipeline. For a solo project on a NAS runner this may be acceptable on every push, or you may prefer PR-only gating to keep push feedback fast. The answer determines the
on:stanza inci.ymland whether the e2e suite genuinely gates merges. (Raised by: Felix, Markus, Tobias)CI stack approach: Java service containers vs. Docker-in-Docker? Running the backend inside the Playwright container requires Java to be installed in that image (or a multi-stage job). Using
docker compose uprequires DinD andprivileged: trueon the NAS runner. The simpler path is service containers (postgres + minio) + Java install + JAR, without DinD — but only you know whether the NAS runner supportsprivileged: true. (Raised by: Tobias)Scope
TEST-3 Complete ✅
All 12 critical journeys are now covered.
Coverage before this issue
New tests added
auth.spec.ts— Admin creates invite via API, new browser context registers at/register?code=…, new user can log indocuments.spec.ts— Opens edit page, adds existing "Familie" tag via TagInput suggestion, saves, verifies chip on detail pagedocuments.spec.ts— Types a brand-new tag name, presses Enter to create it, verifies chip persists after savepersons.spec.ts— Creates two persons via API, opens first person's edit page, addsSPOUSE_OFrelationship via AddRelationshipFormdocuments.spec.ts— Verifies text + tagId filter combination and text + date range combination in URL and resultsnotification-deep-link.spec.ts— Bell button opens notification dropdown, Escape closes itpermissions.spec.ts— Reader user navigating to/admingets 403 error, not the admin paneladmin.spec.ts— Admin opens System tab, clicks import trigger, verifies status response appearsCoverage report:
docs/audits/e2e-coverage-report.mdPR: #430