feat: dashboard enrichment-list-block after batch upload (#296) #298
Reference in New Issue
Block a user
Delete Branch "feat/issue-296-enrichment-list-block"
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 #296.
Summary
Restores the enrichment list block on the dashboard so seniors have a visible next step after a batch upload. Re-adds the deleted
/api/documents/incompleteendpoint, gates it (and the two existing/incomplete*siblings) behindWRITE_ALL, drops a post-upload banner with a CTA into/enrich, and wires the whole thing into+page.sveltebetween Resume strip and Mission Control.Changes
GET /api/documents/incomplete(deleted inddd811c6), capped server-side at 200, retrofitted@RequirePermission(WRITE_ALL)on/incomplete-count,/incomplete, and/incomplete/next(CWE-285 gap Nora flagged).IncompleteDocumentDTOnow carriesuploadedAtfor the relative-time meta line.EnrichmentBlock,UploadSuccessBanner, and$lib/relativeTime.tshelper; redesignedDashboardNeedsMetadatawithtopDocs+totalCountprops + chevron row anatomy; DropZone emitsonUploadComplete(count)so the dashboard can lift banner state without a store.upload_banner_singular/plural/cta/close,dashboard_needs_metadata_show_all_count).DocumentControllerTest/DocumentServiceTest(6 new backend tests), new Playwright spec covering upload → banner →/enrichplus axe sweep at 320/768/1440 × light/dark.Decisions honoured (#3723)
WRITE_ALLon all three/incomplete*endpoints.$navigatingto prevent layout shift.Deviations (documented in plan)
uploadedAtisLocalDateTime(notInstant) to match existing DTO convention (NotificationDTO,DocumentVersionSummary,InviteListItemDTO).Test plan
./mvnw test(1194/1194 green).src/lib/components/{DashboardNeedsMetadata,UploadSuccessBanner,EnrichmentBlock}+src/lib/relativeTime+src/routes/DropZone+src/routes/page.server— all green.dashboard-enrichment-block.spec.tswritten and queued for CI. Local run blocked by a pre-existingauth.setup.tsbug (looks for "Benutzername" label, login form uses "E-Mail-Adresse") — unrelated to this PR.🤖 Generated with Claude Code
Mapper populates uploadedAt from Document.createdAt so the dashboard enrichment block can show a relative-time meta line ("vor 2 Min.") per issue #296. LocalDateTime matches the convention used by NotificationDTO, DocumentVersionSummary and InviteListItemDTO. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I checked
EnrichmentBlockcomposes cleanly without leaking state into+page.svelteObservations
DocumentController.getIncompletedelegates straight todocumentService.findIncompleteDocuments(...); no repository touch from the controller, and the cap (Math.min(size, 200)) is at the edge where it belongs. The service mapper stays where it already lived (DocumentService.java:545), so the DTO extension is a one-line touch — no wider ripple.IncompleteDocumentDTOstays a record with@Schema(requiredMode = REQUIRED)on every field, which is the codebase convention that drives the TypeScript codegen. I was going to push back onLocalDateTimevs.Instant, but the PR description explicitly calls out the deviation and the reasoning (NotificationDTO,DocumentVersionSummary,InviteListItemDTOall useLocalDateTime). Consistency wins. Good.EnrichmentBlocksplit is exactly what I asked for on the issue. The list-row component stays focused on one visual region; the wrapper owns the skeleton/banner composition. Two nameable regions, two components. Parent+page.svelteremains an orchestrator — no business logic, justbannerCountstate and two prop bindings.Promise.allSettled. Still fine — we're nowhere near connection-pool pressure — but this is roughly the limit I'd want before we start thinking about a dashboard-aggregator endpoint. Worth noting for future PRs; not blocking here.Suggestions (non-blocking)
frontend/src/routes/+page.server.ts:86usesincompleteCountResult.value.data?.count as number | undefined. The generated type for/incomplete-count(fromapi.ts) isMap<String, Long>→{ [key: string]: number }, which openapi-typescript flattens to a loosely-typed object. Theas number | undefinedcast is pragmatic but brittle if the backend shape ever changes to e.g.{ count, lastUpdatedAt }. A small helper likeextractCount(data): numberwith a type guard would make this more future-proof. Nit — not blocking.No boundary violations, no premature abstractions. Ship it.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I checked
/incomplete-count,/incomplete, and/incomplete/nextdoc.title,doc.uploadedAt,m.upload_banner_plural)Observations
/incomplete*endpoints now have@RequirePermission(Permission.WRITE_ALL), and the 401 + 403 tests prove both unauthenticated and reader-only paths are denied (DocumentControllerTest.java:395–399,:417–421,:466–471). This is a real security improvement;/incomplete/nextpreviously returned the fullDocumententity (sender, tags, filename) to anyone withREAD_ALL. That's fixed.DocumentControllerTest.java:424–432). The client can't force the server into a large page fetch. Good defense in depth.doc.titleis injected into Svelte templates, which auto-escape; same for the banner count inm.upload_banner_plural({ count }). No{@html ...}anywhere in the new markup, noinnerHTML, no CSS-class routing from user input (mime-type badge was dropped per correction #3721 — good, it would have been the only risky bit).aria-label={m.upload_banner_close()}(UploadSuccessBanner.svelte:44). Banner disappears on Enter/Space via standard button semantics. My a11y-meets-security note from the issue is honoured.+page.server.ts:101-102returns a generic'Daten konnten nicht geladen werden.'on load failure, no backend stack trace reaches the browser.Suggestions (non-blocking)
frontend/src/lib/components/DashboardNeedsMetadata.svelte:21doesnew Date(doc.uploadedAt)without validating the shape. If a malformed ISO string ever slipped through (e.g. from a manually-written SQL insert during migration),relativeTimeDereturnsNaNin the Paraglide key. Not a security issue — a UX papercut. ANumber.isFinite(minutes)guard inrelativeTime.tswould make it robust; up to Felix whether that's worth the test cost.@Testthat asserts the three endpoints are decorated with@RequirePermissionat the reflection level, so a future refactor that accidentally removes the annotation trips a build failure, not a pen-test report. Same idea as SecurityConfigTest in some codebases. Optional.No OWASP Top 10 concerns. No dangerous primitives introduced. Shipping this makes the app more secure than it is on
main.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
What I checked
setTimeoutsleeps, clock assumptions)What I like
relativeTimeDeis covered — minutes, hours, days, rounding edge, zero-gap (relativeTime.spec.ts:6-33). Injectednowmeans no clock flake. This is the pattern.verify(documentService).findIncompleteDocuments(200)(DocumentControllerTest.java:432). Locks the security decision.UploadSuccessBanner.svelte.spec.ts:49-55). No CI burn, no flakes.DashboardNeedsMetadatacovers the 5/6 boundary and the "topDocs ≠ totalCount" case (DashboardNeedsMetadata.svelte.spec.ts:45-60) — exactly the nuance I asked for on the issue.Concerns
DropZone.svelte.spec.tsuses thesetTimeout(50)anti-pattern in two places (lines 66 and 80):This is the exact flake pattern I warned about. On a slow CI runner the microtask chain may not have resolved in 50ms; on a fast runner it resolves in 5ms and a race can falsely-pass. Please replace with
vi.waitFor({ timeout, interval })for the negative assertion, and eithervi.waitForor (preferably) an awaitedinvalidateAllpromise resolution for the last one. The happy-path test (line 46) already usesvi.waitForcorrectly — mirror that everywhere.EnrichmentBlock.svelte.spec.tsnever exercises the skeleton branch. The mock at line 7-10 createswritable(null)for$navigatingand never writes to it, soshowSkeletonis alwaysfalsethroughout every test. The skeleton was specifically decision #3 on the issue (Leonie's B) and warrants a test:Without this, a future refactor could break the skeleton logic silently and no test would catch it.
E2E spec was not executed locally. The PR description acknowledges this (pre-existing
auth.setup.tsbug). I'll take the commitment to CI-validate, but please flag on merge if CI also red-flags the auth setup so this doesn't land in a "green commit, red CI" state.Suggestions
DropZone.svelte.spec.tstest 2 ("does not invoke callback when no files were created") would be stronger as an assertion at a deterministic trigger point — e.g. chaininvalidateAll's promise resolution and assert after that resolves. Then nosetTimeoutat all.Summary
The TDD commit discipline is excellent (one red/green pair per commit, visible in the log). Coverage is ~95% of what I'd want. Fix the two flake patterns in
DropZone.svelte.spec.tsand add the skeleton-branch test and this is a full green check.🎨 Leonie Voss — UX & Accessibility Lead
Verdict: ⚠️ Approved with concerns
What I checked
DashboardNeedsMetadatavs. the issue spec (icon · title · time · chevron)role="status",aria-live="polite", keyboard-reachable dismiss<ul>/<li>list,<a>as the full-row hit target)motion-reduce:animate-none)What I like
<a>around icon · title · time · chevron — that's the pattern I specified, keeps the hit target large enough for a senior's thumb on 320px.motion-reduce:animate-noneon the skeleton pulse — respectsprefers-reduced-motion. Nice catch.role="status"+aria-live="polite"— screen readers will announce "2 Dokumente hochgeladen. Jetzt ergänzen" without interrupting the user mid-action.upload_banner_singularandupload_banner_plural), not with a conditional inside the component. German grammar is correct.Concerns
Contrast not verified locally. The PR description notes E2E axe was not run locally (pre-existing auth issue). That means the dark-mode contrast assertions in
dashboard-enrichment-block.spec.tslines 57-80 haven't been exercised. Thetext-ink-3used for the time meta line and chevron — was that spot-checked againstbg-surfacein dark mode? If CI surfaces a contrast failure, please don't just add the class to axe'sdisableRuleslist — fix the token instead.Chevron on
.group-hover:translate-x-0.5usestransition-transform, but nomotion-reduce:transition-none. Small but consistent: if you addedmotion-reduce:animate-noneto the skeleton, add the same guard to the chevron too. Users with vestibular disorders notice sub-pixel movement.Dismiss button hit target is 24×24px (
UploadSuccessBanner.svelte:45→h-6 w-6). WCAG 2.2 AA minimum is 24×24 for non-essential controls, so technically fine — but our touch-target baseline everywhere else on the dashboard is 48×48 (and the issue spec explicitly calls that out for seniors). Please expand the button's clickable area toh-8 w-8or add padding, while keeping the icon visually small. This is the single most-used control on the banner for users who don't want the CTA; it should be trivially tappable.Row icon on
DashboardNeedsMetadata.svelte:20is a generic copy-item SVG. That's fine post correction #3721 (PDF-only), but the alt-text is empty and the icon is aria-hidden — which means the row currently announces only the title + relative time, no "document" affordance, to screen readers. Consider a visually-hidden label like<span class="sr-only">PDF: </span>before the title so the row reads "PDF: Sterbeurkunde 1930, vor 2 Minuten" to assistive tech. Nit, not a blocker.Suggestions
EnrichmentBlock.svelte:32, consideraria-busy="true"on the parent instead of justaria-hidden="true". Screen readers will know the region is loading rather than treating the absence as permanent.Summary
Looks visually correct against the spec on inspection of the diff; I'm flagging dark-mode contrast and the dismiss-button hit-target explicitly because seniors are the primary user. If CI's axe sweep comes back green at all six viewport-scheme combinations, concern #1 auto-resolves.
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
What I checked
What I like
afterEach, notafterAll(dashboard-enrichment-block.spec.ts:24-27). Good — means if a test fails mid-suite, the DB doesn't accumulate orphans across runs.Concerns
dashboard-enrichment-block.spec.ts:21-23hardcodesarchive-dbas the container name:This matches our
docker-compose.ymltoday. But if we ever run the E2E job in a CI workflow that uses Kubernetes pods, GitHub Actions services, or a differently-named container, this spec silently fails inafterEach(the cleanup throws, but the test's actual assertion may have already passed or failed). Same pattern exists indashboard-screenshots.spec.ts:22— so it's not new debt, but this PR extends the blast radius. Two paths:$DB_CONTAINER) with the current hardcoded value as the default, so CI can override. 3 lines, future-proof.My lean: (a) for this PR, (b) as a follow-up cleanup issue I'll file.
E2E spec adds 7 tests (1 happy-path + 6 matrix cells). Each matrix cell opens a fresh browser context, navigates, and runs axe — probably ~3-5s each. That's 20-35s added to the E2E suite. Still well under our 8-minute SLO, but it's worth noting because the axe matrix is a pattern that other persona-review PRs will want to replicate. If two more PRs add the same matrix, we're at +60-100s and the SLO gets tight. Not blocking; flagging for awareness.
No visual-regression retention pin. The issue mentioned capping artifact retention to the default 30 days, which I'd still like to see in the workflow — but that's a CI-workflow change, not a PR-scope change. Filing as a follow-up.
Suggestions
Summary
Clean application-layer change with no operational surface. Ship it. I'll file two follow-up issues (container-name env var, artifact retention) so the debt isn't lost.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
What I like
758c7087 test(documents): lock /incomplete size cap at 200— the test was written first, the cap was restored second. Every step has a clear red/green pair. This is the pattern.relativeTime.tsis 7 lines of pure function with an injectednow. No class, no wrapper, no configuration object. That's KISS done right.DashboardNeedsMetadata.svelteuses$derived(totalCount > 5)for the footer flag, keeping the template dumb. No business logic in markup.<a>wrapping everything. No nested interactive elements — accessibility is correct by construction, not by audit.LocalDateTimedeviation from the issue is documented in the PR description with reasoning (consistency with existing DTOs). That's exactly how pragmatic deviations should be surfaced.onUploadCompleteis an optional callback prop, not a store. Lifts state to the orchestrator, keeps DropZone focused. The "props over stores" principle.Concerns
EnrichmentBlock.svelte.spec.tsnever exercises the skeleton branch (overlaps with Sara's finding). The$navigatingmock is hardcoded tonull, soshowSkeletonis dead code in the test world. Add a test that swaps the store to a truthy value and asserts the pulsing div is rendered. ~15 lines, closes a real gap.DropZone.svelte.spec.tslines 66 and 80 useawait new Promise(r => setTimeout(r, 50)). Flake magnet under CI load. Replace withvi.waitForor await the mockedinvalidateAllpromise directly. The happy-path test on line 46 already usesvi.waitForcorrectly — mirror that pattern.DashboardNeedsMetadata.svelte:38-47's inline chevron SVG is 8 lines of markup. No reuse cost here (only one call site), but we've got a pattern in this codebase of extracting inline SVGs into shared$lib/icons/components once they appear in 3+ places. Not a blocker on this PR; flag for future refactor if chevron-right shows up anywhere else.Suggestions
interface Propspattern is used correctly four times in the new components — consistent and explicit. Keep doing this.EnrichmentBlock.svelte:11—type IncompleteDocis redeclared inside the component instead of importing from$lib/generated/api.ts. The generatedIncompleteDocumentDTO['schemas']has exactly these three fields; a single import would avoid the drift risk if the DTO gains a field. Small nit.relativeTimeDe-style relative-time component into a<RelativeTime from={date} />wrapper eventually, once a second caller appears (activity feed? conversation timeline?). Pre-emptive extraction would be YAGNI.Summary
The TDD was clean and the code reads well. Fix the two test-pattern issues (skeleton branch + setTimeout flake) and this is a 💯. Everything else is cosmetic.
🔁 Review round 1 — fixes pushed
All concerns from the six persona reviews have been addressed in four new commits:
EnrichmentBlock.svelte.spec.tsskeleton branch untested30ea1f0dEnrichmentBlock.svelte{,.spec.ts},__mocks__/navigatingStore.tsDropZone.svelte.spec.tssetTimeout(50)flake pattern97e8e4fcDropZone.svelte.spec.ts35303831UploadSuccessBanner.svelte(now 40×40)motion-reduce:transition-none35303831DashboardNeedsMetadata.sveltearia-busy="true"30ea1f0dEnrichmentBlock.svelte35303831DashboardNeedsMetadata.svelte(<span class="sr-only">PDF: </span>)relativeTimeDecan render"vor NaN Tag(en)"on malformed input6f3aa056relativeTime.ts+ specDeliberately deferred
countfrom/incomplete-countinto a type-guarded helper. Deferred: the response shape is a simple{ count: number }, openapi-typescript models it as a loose record, and a 3-line helper would be heavier than the current inline cast. Revisit when a second caller appears or when the endpoint shape grows.IncompleteDocumentDTOfrom$lib/generated/api.tsinEnrichmentBlock.svelteinstead of redeclaring the inline type. The generated type is{ id: string; title: string; uploadedAt: string }— the same fields. Drift risk is real but small; leaving local type declarations in three components (DashboardNeedsMetadata,EnrichmentBlock, spec) matches the prevailing pattern in the codebase today. Worth a broader cleanup PR.archive-dbcontainer name in the Playwright spec. Filed as follow-up issue noted in the review comment; leaving the existing convention that matchesdashboard-screenshots.spec.ts.Verification
vitest runon the six touched specs: 36/36 passing (DashboardNeedsMetadata 6, UploadSuccessBanner 6, EnrichmentBlock 6, relativeTime 6, DropZone 3, page.server 9).Ready for another pass.
🏛️ Markus Keller — Senior Application Architect (Round 2)
Verdict: ✅ Approved
What I re-checked
countextractor suggestion got deferred with sound reasoningObservations
__mocks__/navigatingStore.tsextraction is correct — vi.mock hoisting constraints aren't a personal preference, they're a tooling fact. Putting the shared store in its own module is the cleanest resolution.countextractor suggestion being deferred with the stated reasoning ("revisit when a second caller appears or the endpoint shape grows") is exactly the pragmatism I look for. I'd have done the same.No boundary violations. Ship it.
🔐 Nora "NullX" Steiner — Application Security Engineer (Round 2)
Verdict: ✅ Approved
What I re-checked
relativeTime.ts+ its testObservations
relativeTime.ts:5-7now guards withNumber.isFinite(minutes)and returnsm.comment_time_minutes({ count: 0 }). The fallback is in the allowlist (a Paraglide key with a numeric parameter), so no arbitrary string ever reaches the template. Robustness without widening the attack surface — exactly right.'falls back to 0 minutes when the input Date is invalid'is concise and uses the injectablenowpattern, so it's deterministic.No OWASP concerns. Clean round-2 closeout.
🧪 Sara Holt — Senior QA Engineer (Round 2)
Verdict: ✅ Approved
What I re-checked
EnrichmentBlock.svelte.spec.tsvi.waitFor+ hoisted mock replaced the sleeps correctlyrelativeTime.spec.tsObservations
EnrichmentBlock.svelte.spec.ts:67-78+:79-88exercise both the true branch (navigating active + topDocs empty → skeleton visible) and the false branch (navigating active + topDocs non-empty → skeleton hidden). The__mocks__/navigatingStore.tsextraction +afterEach(navigatingStore.set(null))is clean test hygiene — no leaked state across tests.vi.waitFor(() => expect(invalidateAllMock).toHaveBeenCalled())replaces the 50ms sleep with a deterministic anchor. The "no prop supplied" test now relies purely onvitest-browser-svelte's polling (await expect.element(...).toBeInTheDocument()), which is the right tool. Both sleeps are gone.Invalid Dateinput, two assertions, deterministicnow. Exactly what I'd have written.Summary
All three of my concerns from round 1 are closed cleanly. Coverage is 100% of what I asked for. Flip to green.
🎨 Leonie Voss — UX & Accessibility Lead (Round 2)
Verdict: ✅ Approved
What I re-checked
motion-reduceguardaria-busyObservations
h-4 w-4) inside a 40px button — visually subtle, easily tappable. Exceeds WCAG 2.2 AA 24px minimum with margin, and lines up with the senior-first baseline. ✓motion-reduce:transition-none motion-reduce:group-hover:translate-x-0cancels both the transition and the translate for users with vestibular sensitivity. ✓aria-busy="true"plusaria-label="Lade aktualisierte Liste"— screen readers will announce the loading state instead of treating the region as empty. ✓Note
auth.setup.tsbug blocks a local run. That's the only thing I can't tick off myself — but the assertions are in the spec and will surface in the CI run.Summary
Every visual + a11y concern from round 1 is addressed. UX-wise, this is a very polished dashboard increment. Ship it.
🔧 Tobias Wendt — DevOps & Platform Engineer (Round 2)
Verdict: ✅ Approved
What I re-checked
Observations
archive-dbenv-var suggestion was explicitly deferred to a follow-up with a clear rationale (matching existingdashboard-screenshots.spec.tsconvention). That's the right call for this PR's scope.Nothing new from my angle. Green.
👨💻 Felix Brandt — Senior Fullstack Developer (Round 2)
Verdict: ✅ Approved
What I re-checked
vi.hoistedusage inDropZone.svelte.spec.tsrelativeTime.tsand whether the test properly drove the implementationObservations
writablestore. The__mocks__/navigatingStore.tsextraction is the right escape hatch for vi.mock's hoisting constraint. Clean factoring — the store lives in one place, the tests and the mock factory both import it.vi.hoistedinDropZone.svelte.spec.ts:7is the idiomatic way to share a mock function across a hoisted factory and the test body. The comment explaining why it exists is exactly the kind of "why" comment I want to see — the mechanism is non-obvious without context.6f3aa056has the test failing with'vor NaN Tag(en)'followed by theNumber.isFinite(minutes)guard. Comment in the code explains the threat model (malformed backend string), not the mechanism.Summary
Every concern from round 1 is closed or consciously deferred with a documented reason. TDD discipline held through the fix cycle. 💯.