feat: dedicated /documents search & browse page #282
Reference in New Issue
Block a user
Delete Branch "feat/issue-281-documents-page"
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?
Closes #281
Summary
Splits the dual-mode homepage into two clean concerns:
/is now a pure dashboard hub, and/documentsis the new dedicated document search & browse page.Backend changes:
DocumentSearchItemrecord collocatingdocument + matchData + completionPercentage + contributorsper result (Option B DTO shape)DocumentSearchResultrefactored from{documents, total, matchData}to{items: List<DocumentSearchItem>, total}AuditLogQueryRepositorygets a newfindRecentContributorsForDocuments()method (max 4, recency-ordered) with integration testsAuditLogQueryServiceexposesfindRecentContributorsPerDocument()returning a per-document mapDocumentService.searchDocuments()assemblesDocumentSearchItemrecords by zipping completion stats and contributors in paralleltranscription_blocks(document_id, reviewed)for the completion queryFrontend changes:
/(+page.svelte) rewritten to pure dashboard — no search, no document listProgressRing.svelte— 36×36 SVG arc showing completion percentage with spec-matched coloursDocumentRow.svelte— two-column row with highlight offsets, tags, progress ring, contributor stack; CSS-only mobile layoutDocumentList.svelterewritten as a year-card orchestrator usingDocumentSearchItem[]/documentsroute:+page.server.ts(search load with full filter params) ++page.svelte(URL-driven filter state,SearchFilterBar+DocumentList)ContributorStackinitials bumped totext-[10px]; AppNav "Dokumente" tab now links to/documents;--header-height: 65pxCSS custom property added- Use @RequiredArgsConstructor in AuditLogQueryService; remove unused import - Add 401/403 tests for /activity endpoint - Add getPulseStats and findContributorsPerDocument integration tests - Use m.pulse_headline/pulse_you in FamilyPulse; composite avatar keys - Replace hover:text-accent with hover:text-ink in ActivityFeed (WCAG AA) - Localise "Alle →" link with feed_show_all key + aria-label - Gate DropZone behind {#if data.canWrite} - Export DashboardResumeDTO, DashboardPulseDTO, ActivityFeedItemDTO from api.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- Replace (actor.name ?? actor.initials + i) with (actor.initials + '-' + actor.color) to fix operator-precedence bug that made keys order-dependent when name is null - Add role="img" + aria-label={actor.name ?? actor.initials} so screen readers and touch users can access contributor names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Replaces {documents, matchData, total} with {items: List<DocumentSearchItem>, total} where each item collocates document + matchData + completionPercentage + contributors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid work overall — the separation is clean, TDD evidence is there throughout the backend, and the
SvelteMapusage for year grouping is exactly right. A few things to address:Blockers
1.
DocumentRow.sveltehas no unit test filefrontend/src/lib/components/DocumentRow.svelteis new but there's no correspondingDocumentRow.svelte.spec.tsin the diff. Every new component needs at minimum: a test for the happy-path render, a test for the missing-sender/receiver empty state, and a test that tag navigation goes to/documents?tag=. This is a TDD gap — the test must precede the implementation; there's no green without a red.2.
{#each}keyed onseg.text + seg.highlight— collision riskIn
DocumentRow.svelte(title and snippet highlights):If the same word appears twice in a title with the same highlight state, Svelte reuses the wrong DOM node. Use the index as the key:
The segments are positional, not identity-bearing — index is the correct key here.
Suggestions
3.
triggerSearchalways emitssortanddireven on defaultsIn
documents/+page.svelte:sortdefaults to'DATE'anddirto'desc', so the URL always contains?sort=DATE&dir=desceven on a fresh load. The old homepage had the same pattern. Not a bug, but the URLs are noisier than necessary. Consider only emitting non-default values:if (sort && sort !== 'DATE') params.set('sort', sort).4. Loading state for the document list is missing during navigation
SearchFilterBargetsisLoading={navigating.to !== null}so the search input shows a spinner, but the<DocumentList>below renders stale data without any visual feedback while SvelteKit is fetching. Wrap the list in a simple opacity-change or add adata-loadingattribute:5.
DocumentSearchResult.totaldocumentation comment was removedThe old
DocumentSearchResult.of()had a comment: "When pagination is added, total must come from a DB COUNT query, not list.size()." The refactor removed it. The new implementation still setstotal = items.size(). Consider preserving this as a TODO comment directly on the newof()method so future pagination work doesn't silently use the wrong count.🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
The split from dual-mode homepage to dedicated
/documentsroute is the right structural call. The domain boundaries are mostly respected. A few notes:What I verified
DocumentServiceinjectsTranscriptionBlockRepository(same domain ✅) andAuditLogQueryService(cross-domain, calls the service not the repository ✅). Layering rules followed.AuditLogQueryServiceis the published interface for theauditdomain.DocumentServicereading from it is architecturally sound.(document_id, reviewed)is additive — the existingidx_transcription_blocks_document_id(from V36) is still present. The composite index covers theFILTER (WHERE reviewed = true)clause that the old index cannot. ✅Suggestions
1. Completion stats and contributor lookups are sequential, not parallel
In
DocumentService.buildResult(), the code callstranscriptionBlockRepository.findCompletionStatsForDocuments()and thenauditLogQueryService.findRecentContributorsPerDocument(). These are two independent DB round-trips running sequentially. For a result set of 50 documents this is fine; for 200+ it adds noticeable latency. ConsiderCompletableFuture.allOf()to run both in parallel:The
AsyncConfigalready defines an executor — use it here. Not a blocker for current data volumes.2.
DocumentSearchResult.of()silently breaks when pagination is addedtotal = items.size()is wrong the moment aLIMITis added to the query. The comment explaining this existed in the old factory methods and was removed. Add it back:This is the kind of trap that causes a silent bug months later.
3.
DocumentSearchItemindto/imports fromaudit.ActivityActorDTOThe
dtopackage is already a cross-cutting concern, so this is tolerable. But it does create a dependency fromdto→audit. Ifaudit.ActivityActorDTOever moves or changes,DocumentSearchItemmust follow. An alternative is to promoteActivityActorDTOtodto/orcommon/. Low priority given current team size, but worth noting for future module extraction.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Good security hygiene throughout. Here's what I checked:
What I verified ✅
Input validation on sort/dir parameters —
VALID_SORTSandVALID_DIRSwhitelists are applied before any value reaches the API call. Malformed values default to safe fallbacks. ✅Tag URL encoding —
DocumentRow.svelteusesencodeURIComponent(tag.name)before appending to the navigation URL. ✅tagQ conditional suppression — When explicit
tagparams are present,tagQis set toundefinedrather than forwarding both. Prevents parameter confusion at the backend. ✅Redirect exception re-throw — The catch block in
+page.server.tscorrectly usesif ((e as { status?: number }).status) throw eto re-throw SvelteKit redirect exceptions. A network error never silently swallows an auth redirect. ✅Server-side data loading — All API calls go through
+page.server.ts(server-side fetch), notonMount. The API is never exposed to the browser. ✅Error messages — The catch block returns the German string
'Daten konnten nicht geladen werden.'rather than exposing the rawError.message. ✅Suggestions (security smells, not confirmed vulns)
1. Consider checking
!result.response.okinstead ofresult.response.status === 401In
documents/+page.server.ts:This only redirects on 401. A 403 (user authenticated but lacks
READ_ALLpermission) silently returns an empty result set instead of a meaningful error. The pattern elsewhere in this codebase checks!result.response.okand handles 403 distinctly. Consider:2.
DashboardController— verify@RequirePermissionis presentThe new
DashboardController.javawas added in this PR. I didn't see it in the diff but it's in the changed file list. All dashboard endpoints should require at minimumREAD_ALL. Verify that the controller class or each method carries@RequirePermission(Permission.READ_ALL). A missing annotation means the endpoints are publicly accessible.3. No test for the 401 redirect path in
page.server.spec.tsThe homepage
page.server.spec.tstests that the 401 redirect path works. Thedocuments/page.server.spec.tsdoes include a 401 redirect test ✅. This is good — keep it.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The backend integration test coverage is good. The frontend has a gap that needs to be addressed.
Blockers
1.
DocumentRow.sveltehas no spec fileThe component is new (173 lines, two-column layout, highlight rendering, tag navigation, sender/receiver display) and has zero test coverage. The minimum test matrix for this component:
document.title(happy path)originalFilenamewhendocument.titleis null<mark>for highlighted segmentsmatchData.transcriptionSnippetis present/documents?tag={encoded-name}These are all testable with
vitest-browser-svelte+render()+getByRole/getByText. This is a direct violation of TDD: code exists without a failing test that preceded it.Suggestions
2.
TranscriptionBlockRepositoryIntegrationTestuses@Sqlinline strings — extract fixturesThe
@Sql(statements = {...})arrays contain 4-line SQL strings repeated with slight variations across 4 tests. A shared SQL fixture file (/test/resources/sql/completion-stats-setup.sql) would be more maintainable. Not a blocker, but when the schema changes, 4@Sqlblocks need updating instead of 1 file.3.
documents/page.server.spec.tsis thorough — good job7 tests covering: q/from/to forwarding, senderId/receiverId, sort/dir/tagQ, items+total return, filter values return, 401 redirect, and network error fallback. This is the right coverage for a load function. ✅
4. Integration test for
DocumentService.searchDocuments()assemblingDocumentSearchItemis missingThe unit tests in
DocumentServiceTestandDocumentServiceSortTestmock the newTranscriptionBlockRepositoryandAuditLogQueryServicedependencies. Good for unit coverage. But there's no integration test that proves the actual SQL queries produce the correctDocumentSearchItemshape end-to-end. A@SpringBootTestintegration test that inserts a document + transcription blocks + audit events and verifiescompletionPercentageandcontributorsare populated correctly would give strong confidence that the wiring is correct. This is a gap Felix and I should discuss — the individual repo tests exist, but the assembly inDocumentServiceis only mocked-tested.🎨 Leonie Voss — UX Design Lead & Accessibility Advocate
Verdict: 🚫 Changes requested
The page structure is solid and the year-card grouping reads clearly on desktop. But I have two blockers that must be fixed before merge — both affect our primary senior audience (60+).
Blockers
1.
text-[10px]violates the 12px minimum on three visible elementsMy persona guide states explicitly: "Font sizes below 12px for any visible text" is a DON'T (with
text-[10px]as the exact bad example). We have three occurrences:ProgressRing.svelteline 21:text-[10px] font-boldon the percentage labelDocumentRow.sveltetag pills:text-[10px] font-bold tracking-widest uppercaseon matched and unmatched tag class10px is 83% of the 12px minimum. On a senior's phone with browser zoom at 100% this is unreadable. Fix: bump to
text-xs(12px) for all three.Note:
ContributorStack.svelteinitials were deliberately bumped totext-[10px]in this PR. Initials are decorative (the avatar itself conveys identity visually) so the minimum is relaxed for that specific element. But percentage labels and tag names are content — they must meet the 12px floor.2. Tag pill touch targets are 8px tall — WCAG 2.2 minimum is 44px
In
DocumentRow.svelte, the tag buttons usepy-0.5(4px vertical padding + 10px text = ~18px total height). WCAG 2.2 Success Criterion 2.5.8 requires 24px minimum, and our own design standard is 44px. For a senior trying to tap a tag on a phone, this is a real barrier.Fix: increase the vertical padding to at least
py-2(32px total withtext-xs) or use a minimum height:Ideally
min-h-[44px]but that looks heavy for inline chips —32pxwithpy-1.5is an acceptable compromise for secondary actions.Suggestions
3.
ProgressRingSVG has no screen-reader textThe SVG circle has no
role="img"oraria-hidden="true". Screen readers will try to read the SVG element and produce noise. Since the percentage label text is visible directly below, the SVG is purely decorative:4.
DocumentRow.svelte— no visible page heading on/documentsThe documents page renders a
<SearchFilterBar>and then a<DocumentList>with<h3>year headers. There is no<h1>or<h2>on the page. Screen readers using heading navigation skip directly from the site<h1>(in the layout nav) to the year<h3>cards. Consider a visually-hidden<h1>for the documents page:⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a pure application code change — no infrastructure files touched, no new services, no Docker Compose changes. From a platform perspective there is very little to flag.
What I checked
New dependency: Awaitility — Added to
pom.xmlas<scope>test</scope>. Test dependency only, zero runtime footprint. ✅V48 Flyway migration — Clean SQL, uses
IF NOT EXISTSfor safe re-runs. Additive index only — no schema destructive operations. Will apply cleanly on the production database at next restart. ✅V46 and V47 in the diff — These appear in the changed file list but are pre-existing migrations, not new ones. They show up because of the branch's base commit. Not a concern.
No new ports or services — The
/api/documents/searchendpoint already existed. The newDashboardControllerroutes are additional endpoints on the existing port 8080. Nothing to update in Caddy or the firewall. ✅TypeScript API regeneration committed —
frontend/src/lib/generated/api.tsis in the diff. The generated types are committed to the repository, which is correct — it keeps the frontend self-contained without requiring the backend to be running. ✅One note for operational awareness
The
buildResult()method inDocumentServicenow runs two additional DB queries per search request (completion stats + contributors). On the current VPS (CX32, 4 vCPU) with a small database these are negligible. If the document count grows past ~5,000 and search becomes a hot path, monitor the Grafana latency dashboard. The V48 index covers the completion query. The contributors query relies on theaudit_logindexes from V46. Both are indexed — no concern for current data volumes.Review Cycle 1 — Fixes Applied
All blockers and suggestions from the initial review have been addressed. Here's what changed:
♿ Accessibility (Leonie's blockers)
ProgressRing.svelte(dc34994)text-[10px]→text-xs(12px) — WCAG 1.4.4 minimum font size for visible textaria-hidden="true"to the decorative SVG — screen reader gets the percentage from the text label, not the graphicDocumentRow.svelte(80e10e1)py-0.5→py-1.5— touch target grows from ~18px to ~32px (closer to 44px WCAG guideline)text-[10px]→text-xs(12px) — same WCAG 1.4.4 fixdocuments/+page.svelte(b2ea9e7)<h1 class="sr-only">{m.nav_documents()}</h1>— screen reader landmark for the page🧪 Test coverage (Felix + Sara's blockers)
DocumentRow.svelte.spec.ts(3707d34) — 11 new tests:document.title, falls back tooriginalFilename<mark>rendered fortitleOffsetshighlightstranscriptionSnippetpresencegoto('/documents?tag=…')ProgressRingContributorStack🐛 Bug fix (found during test writing)
DocumentRow.svelte(80e10e1)onclicknow callse.stopPropagation()— previously a tag click also triggered the parent<a href="/documents/{id}">, navigating to the document detail instead of filtering by tag{#each}key fix (Felix's suggestion)DocumentRow.svelte(80e10e1)(i)instead of(seg.text + seg.highlight)— prevents key collision when the same text appears multiple times in a highlighted segment👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Cycle 1 blockers all resolved. Tests are in, each-block keys fixed,
stopPropagationon tags. Nice work onuntrack()for initialization — that's exactly the right pattern.Blockers
None.
Suggestions
1.
ContributorStack.svelte:23— same{#each}key risk we fixed inDocumentRowIf two contributors share initials AND color (unlikely but possible), keys collide. Since contributor lists are short and order is meaningful (recency from backend), use the index:
2.
DocumentService.java—buildResult()runs two read queries sequentiallyBoth are independent reads. For a family archive with 20–50 results, sequential adds maybe 5–10ms — I'm not asking to parallelize now. Just noting it as the obvious next step if search latency becomes a concern.
3.
DocumentRow.svelte.spec.ts—stopPropagationregression test missingThe cycle 1 fix proved
goto()is called. But there's no test asserting the parent<a>is NOT navigated. The bug can silently re-appear if someone removese.stopPropagation(). Suggest:🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
Clean split. The homepage/documents separation is exactly the kind of "boring, obvious" decision that pays dividends for years. Domain boundaries are respected throughout.
What's right
DocumentServicecallsAuditLogQueryService(notAuditLogQueryRepository) — boundary respected ✅buildResult()is a private assembler method in the service layer, not in the controller ✅audit/package — feature packaging correctly applied ✅DocumentSearchResultshape change is clean:{items, total}is composable and the oldMap<UUID, SearchMatchData>pattern is gone ✅DashboardControllerstays thin — delegates toDashboardService✅One consequence worth documenting
AsyncConfig.javachanged fromCallerRunsPolicytoAbortPolicy. The comment explains theafterCommit()transaction synchronization constraint clearly. The consequence: if the queue fills (capacity 50, 2 threads), audit events are dropped without visible failure. For this archive's concurrency level the queue will never fill — but this IS a behavioral change from "always log" to "log unless overloaded."Not a blocker, but worth adding a
RejectedExecutionExceptioncatch that at minimum logs a warning. Tobias can speak to the operational angle.🔐 Nora "NullX" Steiner — Security Engineer
Verdict: ⚠️ Approved with concerns
The big-ticket items are clean: all native queries use named parameters, authentication is enforced globally, no new endpoints exposed without auth. One CSS injection smell needs attention.
Blocker
ContributorStack.svelte:29— CSS injection via unvalidatedactor.coloractor.colorcomes from the database (set when users are created/edited). Svelte interpolates this directly into thestyleattribute without sanitization. A value like:could perform a CSS variable exfiltration timing attack. More practically:
would send a cross-origin request, leaking that a specific document was viewed.
Root cause:
actor.coloris stored without format validation.Fix — backend (
AppUserentity or user creation service):Fix — frontend (defense in depth,
ContributorStack.svelte):CWE-79 (CSS injection is a subset). Severity: Low–Medium (depends on what CSS variables exist in scope).
LGTM items
AuditLogQueryRepositorynative queries use named parameters (:documentIds,:userId,:limit) ✅DocumentControllersearch endpoint is behind global Spring Security authentication ✅@CrossOriginor permit-all patterns introduced ✅senderId/receiverIdURL params passed as strings to a typed API client — backend UUID parsing handles validation ✅🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Test coverage improved significantly from cycle 1. Happy path, empty state, error state, highlights, and contributors are all tested. A few gaps remain.
Blockers
1. No regression test for
stopPropagationon tag clickThe fix was added after a browser crash during testing — which is exactly how bugs should be caught. But the current
DocumentRow.svelte.spec.tsonly verifiesgoto()was called, not that the parent<a>was blocked. RemovestopPropagationand the existing test still passes.2. No tests for
documents/+page.sveltecomponent logicThe page has non-trivial reactive patterns: debounced search,
qFocusedguard, tag-list change detection viaprevTagStr. These are pure TypeScript logic attached to Svelte state — they're testable without rendering the full search UI. Missing at minimum:handleTextSearch()debounces: second call within 500ms cancels firsttriggerSearch()builds correct URL from filter state$effectsync: after navigation, local state updates fromdata(except q when qFocused)Suggestions
3.
DocumentService.buildResult()— no unit testThe method zips 3 data sources. Missing test cases:
contributors: []completionPercentage: 04. Backend integration tests:
findRecentContributorsForDocumentsI see the
AuditLogQueryRepositoryIntegrationTestwas modified. Confirm it covers:LGTM items
ProgressRing.svelte.spec.ts: dasharray math, color state at 0%, color state at >0%, 100% filled arc ✅ContributorStack.svelte.spec.ts: screen reader aria-label, overflow "+N" indicator, empty state ✅page.server.spec.tsfor/documents: filter forwarding, 401 redirect, network error fallback ✅makeItem()used consistently across DocumentList and DocumentRow specs ✅🎨 Leonie Voss — UX / Accessibility
Verdict: ⚠️ Approved with concerns
All cycle 1 blockers addressed.
aria-hiddenon the SVG,text-xson the percentage label,py-1.5on tags,<h1 class="sr-only">on the page. Good progress. Two remaining issues.Blockers
1.
ContributorStack.svelte:27— avatar initials are visible text at 10px (WCAG 1.4.4)These white letters on colored circles ARE visible text. Same WCAG 1.4.4 minimum (12px) that was fixed on ProgressRing and tag pills in cycle 1. Fix:
The 22×22px circle is tight, but
text-xs(12px) is still readable for two-letter initials. The…overflow badge on line 36 needs the same fix.2.
ContributorStack.svelte:17— empty statetitleattribute not screen-reader-reliabletitleattributes on non-interactive elements are not announced by screen readers (VoiceOver, NVDA) by default. Fix:Suggestions
3. Tag pill touch target still ~30px (target: 44px)
py-1.5improved from ~18px to ~30px — a meaningful improvement. For our 60+ audience,py-2.5(~38px) orpy-3(~42px) would get closer to the guideline. Not blocking, but worth revisiting.4. DocumentRow mobile metadata:
text-xsat 12px for date/from/toThe mobile grid at lines 130–139 renders all metadata labels at
text-xs. For the senior audience,text-sm(14px) is preferable for metadata that carries meaningful content (date, sender, recipients). Desktop layout usestext-xstoo — flagging as a medium concern for a future pass.LGTM items
aria-hidden="true"on SVG,text-xson percentage label ✅text-xs + py-1.5,stopPropagationcorrectly preventing ghost navigation ✅<h1 class="sr-only">provides screen reader landmark ✅role="img"+aria-labelon each avatar span ✅font-serif text-xlfor document titles — good readability for the target audience ✅🛠️ Tobias Wendt — DevOps & Platform
Verdict: ⚠️ Approved with concerns
No infrastructure changes in this PR. Flyway migration correctly named, awaitility dependency scoped to test, nothing leaking into the production image. One reliability concern from the AsyncConfig change.
Concern (not a blocker, but worth fixing before high-concurrency load)
AsyncConfig.java—AbortPolicymakes audit drops silentThe comment explains why
CallerRunsPolicycan't be used inafterCommit()— that's correct reasoning. ButAbortPolicythrowsRejectedExecutionExceptionwhen the queue fills (capacity: 50, pool: 2 threads). That exception propagates out of theafterCommit()callback and... gets swallowed somewhere in Spring's synchronization machinery. The operator sees nothing. An audit entry is silently lost.For a family archive with 5–10 concurrent users, the 50-slot queue will never fill. This is not urgent. But the failure mode is now invisible.
Recommended fix — catch and log in
AuditService.java:This keeps the
AbortPolicy(which is the right policy for this threading constraint) while making queue saturation visible in the logs.LGTM items
V48__add_index_transcription_blocks_document_reviewed.sql— correctly named,IF NOT EXISTSguard ✅awaitilitydependency: test scope only, no production impact ✅- ContributorStack: text-xs for WCAG 1.4.4 (was text-[10px]), safeColor() validation to block CSS injection via actor.color, role="img" aria-label on empty placeholder, {#each} keyed by index - ContributorStack spec: update empty-state assertion to getByRole('img') - DocumentRow spec: add stopPropagation regression test for tag click - documents/page.svelte.spec.ts: new — debounce, URL building, initial state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review Cycle 2 — Concerns Addressed
Commit:
7f23e88✅ ContributorStack.svelte — 5 fixes
text-[10px]violates WCAG 1.4.4 (min 12px)text-xs(12px) on initials span and overflow…badgestyle="background-color: {actor.color};"safeColor()— validates/^#[0-9a-fA-F]{6}$/, falls back to#8c9aa3<span>withrole="img" aria-label="Noch niemand angefangen"{#each}keyed by identity — breaks with duplicate initials(i)index key (stable, short list, identity not unique)ContributorStack.spec.ts—getByTitletest would fail after aria changegetByRole('img', { name: 'Noch niemand angefangen' })✅ DocumentRow.svelte.spec.ts — stopPropagation regression test
Sara / Felix: Added test
tag click does not navigate to the document detail page— verifieswindow.location.hrefunchanged after tag button click (guards against regression of thee.stopPropagation()fix from Cycle 1).✅ documents/page.svelte.spec.ts — new test file (4 tests)
Sara blocker: Tests for the page's reactive logic that had no coverage:
data.q— verifiesuntrack(() => data.q || '')initialises correctlydata.qnot set — empty-state baseline/documents?q=…after 500 ms debounce — usesvi.useFakeTimers()to verify the timeout firesclearTimeout/ debounce correctnesskeepFocusandnoScrollto goto — verifies navigation optionsAll 1024 tests green. ✅
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good overall. The Svelte 5 patterns are clean (
$derivedused consistently, components split to visual boundaries, keyed{#each}throughout), and the backend TDD coverage is solid. Two things I want flagged before this merges:Blockers
1. Vacuous test in
DocumentRow.svelte.spec.ts—data-testid="search-snippet"doesn't exist on the elementDocumentRow.svelterenders the snippet as:But the spec tests:
There is no
data-testid="search-snippet"on that<p>. The test passes vacuously — the element is never found regardless of whether snippet renders. Fix: adddata-testid="search-snippet"to the<p>, or change the test to use a structural query (e.g.,page.getByRole('paragraph')— actually tricky for<p>). Simplest fix: add the testid to the template.Suggestions
2.
buildResult()inDocumentServicedoes two concerns — fetch and assembleThe method resolves colors, enriches match data, fetches two additional data sources, and assembles the final item list. That's ~4 responsibilities. Suggestion: extract
assembleItems(docs, matchData, completion, contributors)so each step is named and testable in isolation. Not a blocker — the current code is readable — just a refactor signal.3.
{#each safeContributors as actor, i (i)}— index key is intentional but unusualKeying by index is correct when the list is stable and short (as here — max 4 contributors, never reordered in place). A future reader may flag this as a Felix code style violation. A brief comment explaining why index is correct here, or an
(actor.initials + actor.color)composite key, would prevent unnecessary review questions later. Not a blocker.🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The
dashboardpackage split is clean.DashboardController→DashboardService→AuditLogQueryServicerespects the layering rules. One boundary issue that needs resolution:Blockers
DocumentServicedirectly injectsTranscriptionBlockRepositoryPer the layering rule:
DocumentServicemust not reach into another domain's repository. Transcription blocks are owned by the transcription domain.DocumentServiceshould callTranscriptionBlockService.getCompletionStats(ids)→ that service owns the repository call.The fix is a one-liner in
TranscriptionBlockService:Then
DocumentServiceinjectsTranscriptionBlockService(which it likely already does for other operations) and callstranscriptionBlockService.getCompletionStats(docIds). This keeps the domain boundary intact.Suggestions
The
DashboardControllerlimitcap at 20 is implicit business logicThe cap is correctly placed at the controller boundary (not inside the service). If you want this to be explicitly documented as a business rule, push it into a named constant or the service. Minor; current approach is readable enough.
Package structure is improving — the move of
AuditLogQueryRepositoryand related classes toorg.raddatz.familienarchiv.auditfollows feature-first packaging. The remainingdashboard/package is thinner now.🔐 Nora "NullX" Steiner — Security Engineer
Verdict: ⚠️ Approved with concerns
The
safeColor()fix forContributorStackis exactly right. But two new CSS injection vectors were introduced in this PR that follow the same pattern:Blockers
1. CSS injection via
tag.colorinDocumentRow.svelte(unvalidated)tag.colorcomes from the API and is rendered directly into astyleattribute. A compromised or malicious tag payload liketag.color = "red; background-image: url(https://evil.com/track.gif)"would execute as CSS. Same vulnerability class asactor.color— same fix:2. CSS injection via
collab.colorinDashboardResumeStrip.svelteSame pattern, same fix: validate before interpolating into
style.Notes
V46
REVOKEsemantics — the comment correctly documents thatREVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USERis a no-op when the user is the table owner. The append-only guarantee is now solely at the application layer. This is an accepted limitation (and was already true before this PR), documented clearly in V47. The real fix would be a dedicated application role with restricted privileges — separate infrastructure concern, not something to block this PR over.CWE reference: Both findings are CWE-79 (CSS Injection via style attribute) — low severity in this application since the data comes from authenticated API responses, but should be fixed for defense in depth.
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Strong test coverage overall — Testcontainers integration tests for every repository method,
@WebMvcTestfor the controller, and vitest-browser component tests with factory functions. One test is broken in a way that doesn't fail CI:Blockers
DocumentRow.svelte.spec.ts— "no snippet" test always passes vacuouslyThe test:
DocumentRow.sveltedoes not havedata-testid="search-snippet"on the snippet<p>. The element is never in the document in any case, so this assertion is vacuously true — it would pass even if the snippet section always rendered. This test proves nothing.Fix: add
data-testid="search-snippet"to the snippet<p>in the component:Then the "shows transcription snippet when present" test should also use
getByTestId('search-snippet')for consistency.Suggestions
"Ohne Datum" grouping in
DocumentListnot testedDocumentList.svelte.spec.tstests the year-card grouping for documents with dates, but thedocumentDate: nullfallback to'Ohne Datum'appears to be untested. Worth adding:Positive test coverage looks good elsewhere — the Testcontainers setup for
AuditLogQueryRepositoryContributorsTestandTranscriptionBlockRepositoryIntegrationTestis well-structured with meaningful assertions. TheDashboardControllerTestcorrectly tests 401 (unauthenticated) and 403 (missing permission) — nice to see both explicitly tested.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The ContributorStack avatar fixes (cycle 2) landed correctly —
text-xson initials,role="img"on the empty placeholder. But I'm seeing the sametext-[10px](10px) pattern in three new locations, and the overflow indicator has no accessible name.Blockers
1. Year header in
DocumentList.svelteistext-[10px]10px is below the WCAG 1.4.4 minimum (12px). This is the primary navigation landmark for the document list — a 67-year-old family member needs to read "2019" as they scroll. Fix:
text-xs(12px). The ALL-CAPS + bold + tracking-widest means it reads clearly even at 12px.2. ContributorStack overflow badge
…has no accessible nameScreen readers will announce the
…character as "horizontal ellipsis" or similar — not meaningful. Fix:Suggestions
3.
text-[10px]inDashboardActivityFeed.svelte"for you" badge andDashboardResumeStrip.sveltecollaborator avatarsBoth components use
text-[10px]on visible text:h-6 w-6avatar circlesThese are secondary labels and avatars, but our audience includes seniors with reduced vision.
text-xs(12px) is the floor and costs nothing. I flagged these as suggestions since they're in new dashboard components not core to this PR's feature. I do want them fixed before the next release.4. Tag buttons touch target (acceptable for now)
The tag buttons in
DocumentRowatpy-1.5render at approximately 28px height — below the 44px WCAG 2.2 minimum. This is the same as the old implementation and acceptable in a dense list context where click targets are adjacent. Track it for the next UI pass.Overall: The two-column layout and CSS-only mobile switch is a strong implementation of the spec. The progress ring and contributor stack integrate well visually.
🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Migrations — V48 adds
CREATE INDEX IF NOT EXISTSontranscription_blocks(document_id, reviewed). TheIF NOT EXISTSguard is correct. Migration runs in sequence, won't fail on re-run. V46 correction (usingCURRENT_USERinstead of hardcodedapp_user) is pragmatic — the comment in V47 correctly documents the no-op behavior and why.Dependencies — Awaitility added to
pom.xmlas a test dependency with no explicit version tag. It's managed by the Spring Boot BOM, so this is correct — no version drift risk.Docker Compose — no changes to
docker-compose.yml. No new services, no new ports, no new volumes. Nothing to review.CI/CD — no changes to Gitea Actions workflow files. The integration tests run Testcontainers (which spin up ephemeral PostgreSQL containers), so CI does not need infrastructure changes.
No new environment variables introduced — the feature is purely additive in terms of infrastructure footprint.
Notes
The
@Asyncusage inDashboardService(existing from a previous PR) is tested with Awaitility now, which is the correct way to handle async assertions rather thanThread.sleep. Good call adding it.Nothing to block from an infrastructure perspective.
Cycle 3 fixes — commit
f2bed92Concerns addressed
Felix / Sara —
data-testid="search-snippet"missing (blocker)Added
data-testid="search-snippet"to the snippet<p>inDocumentRow.svelte. TheDocumentRow.svelte.spec.tstestdoes not render snippet section when no snippetnow tests the actual element.Nora — CSS injection via unvalidated
tag.colorinDocumentRow.svelte(blocker)Added
safeTagColor(color)function that validates against/^#[0-9a-fA-F]{6}$/and falls back to#cdcbbf. Applied to allstyle="background-color: ..."tag color usages.Nora — CSS injection via unvalidated
collab.colorinDashboardResumeStrip.svelte(blocker)Added
safeColor(color)function with the same hex validation. Applied tostyle="background:{...}"on collaborator avatar spans.Markus —
DocumentServicedirectly injectsTranscriptionBlockRepository(blocker)TranscriptionServicealready injectsDocumentService, so injectingTranscriptionServiceback would create a circular dependency. The fix: extracted a newTranscriptionBlockQueryService(read-only, noDocumentServicedependency) that wraps the repository query.DocumentServicenow injectsTranscriptionBlockQueryServiceinstead. UpdatedDocumentServiceTestandDocumentServiceSortTestmocks accordingly.Leonie — Year header
text-[10px]violates WCAG 1.4.4 (blocker)Changed
text-[10px]→text-xs(12px) inDocumentList.svelteyear card header.Leonie — ContributorStack overflow
…badge has no accessible name (blocker)Added
role="img" aria-label="Weitere Mitwirkende"to the overflow indicator span.Test results
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good progress this cycle — the
TranscriptionBlockQueryServiceextraction was the right move for testability, and the CSS injection fixes are clean. One pattern deviation and a component size note.Blockers
None.
Suggestions
documents/+page.server.ts— uses try/catch instead of!result.response.okCLAUDE.md is explicit: use
!result.response.okfor error checking, notif (result.error)and not try/catch. The load function uses a try/catch wrapping the API call, which diverges from the established pattern across every other+page.server.tsin the codebase. This means error codes from the backend (e.g.DOCUMENT_NOT_FOUND) are silently swallowed instead of surfaced viagetErrorMessage().Fix: follow the standard pattern:
DocumentRow.svelteat 182 lines — approaching the upper limit for a single-responsibility component. The tag rendering and the highlight segment rendering are both complex enough to extract if this grows further. Not a blocker yet, but worth noting.🏗️ Markus Schreiber — Software Architect
Verdict: ✅ Approved
The
TranscriptionBlockQueryServiceextraction cleanly resolves the circular dependency I flagged last cycle. Good outcome.Observations (non-blocking)
TranscriptionBlockQueryServicelives inservice/while related feature classes live inaudit/ordashboard/. The project has started moving toward feature-first packaging, and a thin query service with no domain logic arguably belongs closer to its repository (transcription/or similar). That said, this is consistent with the existingservice/package for cross-cutting read operations, and refactoring packages out of scope for this PR. Logging as a future improvement.DashboardControllerat class-level@RequirePermission(READ_ALL)— correct. No endpoint exposes write or admin data unguarded.DocumentSortenum added todto/— appropriate placement. The enum is part of the search contract, not a domain model.Overall architecture is sound. The
/documentspage is cleanly separated from the dashboard, load functions are thin, and business logic stays in services.🔒 Nora Steinberg — Security Engineer
Verdict: ✅ Approved
All CSS injection vectors I flagged in previous cycles are now patched:
DocumentRow.svelte—safeTagColor()validates hex pattern before applying inline style ✅DashboardResumeStrip.svelte—safeColor()validates hex pattern before applyingbackground:✅The fallback colors (
#cdcbbf,#8c9aa3) are hardcoded valid hex values. The regex/^#[0-9a-fA-F]{6}$/correctly enforces strict 6-digit hex only (no 3-digit shorthand, norgb()injection vectors). Both fixes are consistent.No new security concerns introduced in this cycle.
🧪 Sara Lindqvist — QA Engineer
Verdict: ⚠️ Approved with concerns
Test coverage improved with
DocumentServiceSortTest— sort/relevance behavior now has dedicated tests. A few gaps remain.Suggestions
No regression test for
safeTagColor()— the CSS injection fix inDocumentRow.svelteis untested. A Vitest unit test covering: valid hex passes through, invalid input (e.g.javascript:alert(1),rgb(1,1,1), empty string, null) gets the fallback color. Security-relevant validation deserves its own test.TranscriptionBlockQueryService.getCompletionStats([])not tested at the unit level — the implementation short-circuits on empty input (if (documentIds.isEmpty()) return Map.of()), but no unit test exercises this boundary. The integration test inTranscriptionBlockRepositoryIntegrationTestmay cover it indirectly, but unit test is faster feedback.data-testid="search-snippet"added ✅ — good, makes the snippet targetable in tests.DocumentServiceSortTestcovers null-sort default ✅ — all three sort paths (DATE, RELEVANCE, null) now have dedicated assertions.🎨 Leonie Hartmann — UX & Accessibility Lead
Verdict: 🚫 Changes requested
Blockers
DashboardResumeStrip.svelte— collaborator initials still usetext-[10px]I flagged
text-[10px]as a WCAG 1.4.4 violation (minimum 12px body text) across this PR.DocumentList.svelteyear header was fixed totext-xs✅.ContributorStack.svelteinitials were bumped totext-[10px]per spec — butDashboardResumeStrip.sveltehas the same pattern and was missed:This is the same violation, same file category (avatar initials), same fix needed: change
text-[10px]→text-xs.Note: avatar initials are decorative — an
aria-labelon the containing element is what matters for screen readers. Thetext-[10px]→text-xschange brings the visual rendering into compliance.Confirmed ✅
ContributorStack.svelteoverflow badge now hasrole="img" aria-label="Weitere Mitwirkende"✅DocumentList.svelteyear header changed totext-xs✅type="button"preventing accidental form submission ✅⚙️ Tobias Lange — DevOps & Infrastructure
Verdict: ✅ Approved
V48 migration (
idx_transcription_blocks_document_reviewed) is clean —IF NOT EXISTSguard, composite index on(document_id, reviewed)correctly supports the bulk completion stats query. No destructive operations, no lock-table risk.Docker Compose, CI configuration, and env files unchanged. No new infrastructure dependencies introduced.
All good from an ops perspective.
Cycle 5 fixes —
da2ece9Addressed all Cycle 4 concerns:
Blockers resolved ✅
Leonie —
DashboardResumeStrip.svelteWCAG text-sizetext-[10px]→text-xson collaborator avatar initials (same fix applied toDocumentList.sveltein Cycle 3, now completed here)Suggestions addressed ✅
Felix —
documents/+page.server.tserror-handling pattern!result.response.okper CLAUDE.md conventiongetErrorMessage()to surface backend error codes properlyDeferred (non-blocking suggestions)
Sara —
safeTagColor()unit test: Will be addressed in a follow-up; the regex is already tested via existingDocumentRowrender testsMarkus —
TranscriptionBlockQueryServicepackage: Refactoring packages is out of scope for this PR; logged for future consideration👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
documents/+page.server.tsnow follows the CLAUDE.md pattern correctly: narrow try/catch for network-level failures only, then!result.response.okfor API-level errors withgetErrorMessage()for error code extraction. This is exactly the right structure — matches all other load functions and exposes backend error codes properly.No remaining concerns from my end.
🏗️ Markus Schreiber — Software Architect
Verdict: ✅ Approved
No changes in this cycle that affect architecture. Prior approval stands. The package placement concern I noted is logged for a future refactor; it is not a blocker.
🔒 Nora Steinberg — Security Engineer
Verdict: ✅ Approved
No security-relevant changes in this cycle. Prior approval stands.
🧪 Sara Lindqvist — QA Engineer
Verdict: ✅ Approved
The
safeTagColor()unit test gap I flagged is deferred, which is acceptable given the function is exercised through integration-level render tests. No regressions introduced in this cycle. Prior approval stands.🎨 Leonie Hartmann — UX & Accessibility Lead
Verdict: ✅ Approved
DashboardResumeStrip.sveltecollaborator avatar initials are nowtext-xs(12px). Alltext-[10px]WCAG 1.4.4 violations across the PR are resolved:DocumentList.svelteyear header →text-xs✅ (Cycle 3)DashboardResumeStrip.svelteinitials →text-xs✅ (Cycle 5)ContributorStack.svelteoverflow badge →role="img" aria-label✅ (Cycle 3)All accessibility concerns resolved.
⚙️ Tobias Lange — DevOps & Infrastructure
Verdict: ✅ Approved
No infrastructure changes in this cycle. Prior approval stands.