feat(documents): timeline date-range filter with density bars (#385) #478
Reference in New Issue
Block a user
Delete Branch "feat/issue-385-timeline-density-filter"
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 #385.
Adds a horizontal density timeline above
/documentsshowing per-month document counts. Click, drag, zoom, and filter — all reflected in the URL so views are sharable and bookmarkable. Hidden on mobile and tablet (below lg, 1024px) and in calendar view (no fetch cost paid where the widget isn't shown).What ships
Backend —
GET /api/documents/densityDocumentDensityResult+MonthBucketrecords.q,senderId,receiverId,tag,tagQ,status,tagOpand returns buckets matching the same predicates the document list uses (excludingfrom/to, since those are what the user is selecting from the chart).Cache-Control: private, max-age=300.DocumentService.getDensityJavadoc).Frontend —
TimelineDensityFilter.sveltetimeline.ts: month-sequence builder, gap-filler, year-aggregator, range-clipper, tick-index picker, label formatter.+page.tsso the chart never blocks the SSR'd document list.Test plan
cd backend && ./mvnw test— 1540 tests, all greencd backend && ./mvnw clean package -DskipTests— clean buildcd frontend && npm run generate:api—DocumentDensityResult+MonthBucketregeneratedcd frontend && npm run check— no new TypeScript errors in touched filescd frontend && npm run test—TimelineDensityFilter.svelte.spec.ts35/35 ✓,timeline.spec.ts44/44 ✓,routes/documents/page.svelte.spec.ts12/12 ✓cd frontend && npm run lint— passesJan Feb Mär…axis labels?view=calendarboth hide the widget and skip the density fetchFollow-up issues
🤖 Generated with Claude Code
The +page.ts client-side load now forwards the active /documents URL filters (q, senderId, receiverId, tag, tagQ, status, tagOp) to /api/documents/density so the bars recompute when the user narrows the search. Date bounds (from/to) are deliberately omitted — the chart is the surface for picking those. - New `DensityFilters` type and `buildDensityUrl(filters)` helper. - `fetchDensity` accepts a filter snapshot (defaulting to {} for back-compat in tests). - 6 new unit tests cover URL building, multi-tag repetition, AND/OR forwarding, the explicit-no-from/to invariant, and filter-aware fetch. - Generated API types refreshed against the new backend signature. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
Solid endpoint, clean records, correct documentation hygiene (the C4 PUMLs for both backend and frontend are updated, which is exactly the table-driven discipline I expect). Two architectural concerns worth flagging — one structural, one schema-related.
Concerns (not blockers)
In-memory aggregation in
DocumentService.getDensity—backend/.../DocumentService.java:163-172This pulls every matching
Documentrow (with its eagerly fetched joins for sender/receivers/tags through the Specification) into the JVM, then groups in Java. The Javadoc explicitly justifies this against the 200ms target with "≈5k docs". That's fine today. The right shape is a SQL aggregation:The Specification API doesn't compose with raw SQL aggregations cleanly, so I accept the trade-off — but please add a TODO with a row-count threshold (e.g.
// revisit when documents > 50k) so it surfaces during the next perf audit. The newidx_documents_meta_dateis a good defensive move regardless.The endpoint shape duplicates filter-parameter plumbing across
/searchand/density— same seven query params, parsed twice (controller + service) into the sameSpecification<Document>viabuildSearchSpec. The duplication is bounded today, but if a third aggregation endpoint ever lands, extract aDocumentSearchCriteriarecord before adding it.LGTM
DocumentDensityResultandMonthBucketare flat records in the document package — correct, no over-engineered DTO layering.V61__add_document_date_index.sqlcorrectly usesIF NOT EXISTSand includes a header comment explaining thedocumentDate → meta_datemapping. That comment will save the next archaeologist 20 minutes.tagOpstring parsing in the controller mirrors the existing/searchendpoint — consistency wins over surface elegance.docsListPageTsandtimelineFiltercomponents with their relationships. No ADR needed — this is a feature, not an architectural decision with lasting consequences.Doc completeness check
+page.ts+TimelineDensityFilter)ErrorCode/Permission— Markus
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The TDD evidence is strong — backend tests by layer (service unit, controller slice, integration with Testcontainers), frontend tests at three altitudes (timeline.ts pure helpers, component spec, page spec). Names read as sentences. Factory functions used. I'd like a few cleanups before merge.
Concerns (not blockers, but please address)
TimelineDensityFilter.svelteis 377 lines — too many visual concerns in one file —frontend/src/lib/document/TimelineDensityFilter.svelteThe component owns: bar rendering, Y-axis, X-axis ticks, drag-window overlay, drag/click pointer choreography, year-aggregation fallback, zoom-reset and clear controls. By the rule of "one nameable visual region per
.sveltefile," that's at least four:The pointer-choreography logic (
handleDocumentMove,indexFromClientX,cleanupDragListeners) is genuinely tricky and should stay in the orchestrator. Splitting will also let you delete severaldata-testids in favor ofgetByRole-style queries. Not a blocker because the code works and is well-tested, but the next non-trivial change here will hurt.Five i18n keys added but never used —
frontend/messages/{de,en,es}.jsongit grep -E 'timeline_(count_label|loading|filtered_count|zoom_in|dragging_aria_live)' frontend/srcreturns zero matches. Either wire them up or delete them — three locales × five keys = 15 dead strings to translate the next time we refresh i18n.aria-label="1915-08 · 5"on each bar is not localized —TimelineDensityFilter.svelte:263Screen readers will announce a bare YYYY-MM string and a number. Coordinate with Leonie's review — at minimum format with
formatTickLabel(bucket.month, getLocale())and an i18n template likem.timeline_bar_aria({ month, count }).triggerSearch(zoomOverride?: …)adds optional behavior to a function with two callersThe
+page.svelteonchangehandler callstriggerSearch({...})for drag (filter+zoom atomic) andtriggerSearch()for click (filter only). The branching reads "depends on whether the source event happened to includezoomFrom/zoomTo," which is implicit. Two named functions (triggerSearchKeepZoom()/triggerSearchWithZoom(from, to)) would make the contract explicit at the call site. Minor — defer if you want.LGTM
getDensityis short, has guard clauses, usesOptional-equivalent nullity handling cleanly. The FTS short-circuit (if (rankedIds.isEmpty()) return …) is exactly the right early-return shape.timeline.tsis pure functions only — every helper is independently testable, and the spec exercises 42 cases.aggregateToYearsandtickIndicesForstrategy comments explain why, not what. Good.clipBucketsToRangereturns the input array unchanged when bounds are null (return buckets, identity-equal in the test) — tiny detail, but the test pinning that withtoBerather thantoEqualis the right discipline.+page.tscorrectly usesbrowserto gate the desktop-only fetch, andfetchDensitydegrades toEMPTYon non-ok / network failure rather than throwing — the document list keeps rendering. ✓api.tsnow hasDocumentDensityResult+MonthBucketschemas and thedensityoperation).— Felix
🛡️ Nora Steiner ("NullX") — Application Security Engineer
Verdict: ✅ Approved
I checked: authn/authz on the new endpoint, injection surface, sensitive-data exposure, cache leakage, the
tagOpstring parameter, and the user-controlled URL params. Clean.What I checked
Authentication —
GET /api/documents/densityhas no@RequirePermissionannotation. That's correct: per the project convention@RequirePermissionis required only on POST/PUT/PATCH/DELETE, andSecurityConfig.anyRequest().authenticated()enforces auth on all/api/**GETs. The controller testdensity_returns401_whenUnauthenticatedpins this — anonymous requests get 401. ✓SQL injection — every filter goes through the existing
buildSearchSpecwhich uses JPASpecificationpredicates with bound parameters. No string concatenation reaches Hibernate. The newtagOpparameter isString-typed but only used in"OR".equalsIgnoreCase(tagOp)— no shape where a malicious value can reach the query. ✓Information disclosure — the response (
buckets,minDate,maxDate) reveals only what the user could already see by paging through/api/documents/searchwith the same filters. No new data exposure. ✓Cache leakage —
Cache-Control: private, max-age=300is correct for an authenticated, per-user-filtered response.privatekeeps shared proxies (Caddy, any future CDN) from caching one user's response and returning it to another. The controller test verifies bothprivateandmax-age=300headers are emitted. ✓CSRF / SSRF — GET endpoint, no state mutation, no outbound requests. N/A.
Logging — no user input is logged in the new code path.
Frontend
fetchDensity— uses thefetchinjected by SvelteKit's load function, notonMount+ rawfetch. Auth cookie forwarding works correctly (handled by the SvelteKit hook layer). The graceful failure mode (return EMPTYon non-ok) does not leak status codes or backend error shapes to the user. ✓Suggestion (defer-OK)
The
aria-pressedandaria-labelattributes on the bar buttons are user-visible state. Make sure XSS through a hypothetical future custom locale formatter isn't possible — currentlyformatTickLabelusesIntl.DateTimeFormatwhich is safe, and bucket months come from server-validatedYYYY-MMstrings. No action needed today.— Nora
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Small, well-scoped change. No infra surface affected (no new services, ports, volumes, or env vars). The migration is safe and the cache header is correct. A single low-priority observation below.
Reviewed
V61__add_document_date_index.sql—CREATE INDEX IF NOT EXISTS idx_documents_meta_date ON documents (meta_date)IF NOT EXISTSis correct — re-runnable.CONCURRENTLY. For the current archive (~5k rows) the lock is microseconds; even at 100k rows on a CX32 this is sub-second. The Flyway transaction wrapping would rejectCONCURRENTLYanyway. ✓documentDate→meta_date). I'd file this under "things that save the on-call engineer at 2 a.m." — keep doing this.Cache-Control: private, max-age=300—privatecorrectly prevents Caddy or any future CDN from caching one user's filtered response and serving it to another.max-age=300matches the rough cadence of how often a user changes filters. ✓Observation (FYI only)
The
Cache-Controldirective is set per-response in the controller, not via a sharedWebMvcConfigureror@CacheControlinterceptor. That's fine for one endpoint, but if a second cacheable read endpoint lands, prefer extracting it (e.g. anHttpCachePolicyconstant or aCache-Control: private, max-age=…advice inWebConfig) so the policy lives in one place.Test footprint
The new
DocumentDensityIntegrationTestuses@SpringBootTest+PostgresContainerConfig— adds one Testcontainers boot. Acceptable; the existing suite already does this pattern. Watch the CI total — if the full backend test phase creeps past ~3 min, this is one to consolidate by widening an existing integration test class instead of adding a new one.— Tobi
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Coverage is broad and well-layered. Backend has unit (
@ExtendWith(MockitoExtension)), controller slice (@WebMvcTest), and integration (@SpringBootTest+ Testcontainers Postgres) — exactly the right pyramid. Frontend has 42 pure-helper tests, 25 component tests in vitest-browser, and 12 page-level wiring tests. Naming is descriptive throughout. Two things to address before this regresses.Concerns
No E2E coverage of the timeline interaction at all —
frontend/e2e/The whole zoom + drag + filter feedback loop is the user-facing claim of issue #385, and we have zero Playwright proof it works end-to-end against a real backend. At minimum:
The component spec uses synthesized
PointerEvents, which is fine for the unit layer but does not exercise the document-levelpointermovelistener path that the implementation uses for off-bar drags (indexFromClientX+rowEl.getBoundingClientRect()). That code path is currently untested.Pending manual checks in the PR description should be automated where they affect critical journeys
The five unchecked checkboxes ("drag across years on production archive… click a single year… add a sender filter… ↩ reset… mobile width hides the widget…") are exactly the regressions that will quietly break in three months. The first four belong in Playwright; the fifth ("mobile hides the widget") is a one-line viewport test that already has the test infrastructure in
page.svelte.spec.ts(thedensity: nullcase). Codify them.LGTM
DocumentDensityIntegrationTestis the right shape: real Postgres via Testcontainers,@DirtiesContextper method (acceptable trade-off for a clean test surface here),@MockitoBean S3Client(avoids MinIO requirement). Seven scenarios cover the filter-reactive contract, including the explicit "excludes documents with null date" edge case.density_returns401_whenUnauthenticated(negative auth path) anddensity_emitsPrivateCacheControlHeader(header contract). Both will catch regressions that swap the header strategy.getDensity_shortCircuits_whenFtsReturnsNoMatchesassertsverify(documentRepository, never()).findAll(any())— that's the right way to test the optimization, not a fragile timing assertion.makeProps,makeData) is consistent and overrideable. ✓Quality gate
./mvnw verifykeeps the 88% gate green for the touched files.npm run checkpasses per PR. Confirmnpm run test:coveragedoesn't drop the project total.— Sara
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: 🚫 Changes requested
Visually this is lovely — the Graylog-style drag selector is a smart pattern, the year-mode collapse is the right mental model for a 1873→2023 archive, and the brand mint accent is correctly used. But there are several accessibility regressions that block merge for me, plus a few mobile and dark-mode gaps to address.
Blockers
X-axis tick labels are below my 12px font floor —
TimelineDensityFilter.svelte:288, 244text-[10px]fails our minimum (12px floor; 16px body; 18px preferred for the senior audience). The contrast ratio oftext-ink-3onbg-surfaceat 10px also won't pass WCAG 1.4.4 (resize) for users who don't use browser zoom. Fix:text-[11px]is still under floor — please usetext-xs(12px) and increase the axis row toh-4to accommodate the line height.Bar
aria-labelis a raw machine string —TimelineDensityFilter.svelte:263A screen reader hears "1915 dash 08 middle dot 5" — meaningless. Already drafted the i18n key (
timeline_bar_aria?). Pipebucket.monththroughformatTickLabel(bucket.month, getLocale())and use a templated message:WCAG 1.3.1 (Info and Relationships) + 4.1.2 (Name, Role, Value).
No keyboard equivalent for the drag-to-zoom gesture
Mouse / touch users get drag-to-zoom; keyboard users can only single-click a bar (filter only). That's a 2.1.1 (Keyboard) failure for the zoom feature. Even a Shift+Click "extend selection from previous click to this one with zoom" would close it. Felix and I should pair on the interaction model — drag is genuinely hard to keyboard-equivalent, but the feature it enables (range zoom) cannot be keyboard-only-blocked.
aria-livemessage is in i18n but no live region is rendered —messages/{de,en,es}.jsontimeline_dragging_aria_liveis defined ("Range {from} to {to} selected") but there's no<div aria-live="polite">{drag preview text}</div>in the markup. Without it, screen readers cannot follow the drag preview. Either render an off-screen live region duringisDragging, or delete the unused key.Concerns (not blockers)
No
prefers-reduced-motionhandling —TimelineDensityFilter.svelte:347100ms is short, but combined with the drag-window cursor follow it can be uncomfortable for users with vestibular sensitivity. Wrap in:
Touch targets fail 44×44 on tablet at long ranges — drag the production archive, get 240 monthly bars across an 800px tablet column → ~3.3px per bar. Year-mode (1809-month archive collapses to ~150 year bars) is also ~5px per year bar at 800px. The widget is hidden on
<sm(640px) but tablet (640–1024px) is exactly the transcriber audience on iPads. Either widen the breakpoint tolg:block(1024+) or make tablet bars taller and offer a "page through decades" mode.Dark-mode contrast for idle bars —
TimelineDensityFilter.svelte:341#0d3358against--c-surfacein dark mode is going to come in low — please verify with axe in dark mode (Sara's E2E covers this if extended) and bump if it fails 3:1 (large element).LGTM
--palette-mint) used correctly for selection and drag window — on-brand, semantically consistent with hover-underline use elsewhere.↩) and clear (×) buttons are real<button>elements witharia-labelandtitle— keyboard reachable, screen-reader announceable.hidden sm:blockis the right call for the senior-on-phone reader audience — saves vertical real estate on the read path.{#if density !== null}) — no broken-state UI.rounded-sm border border-line bg-surface shadow-sm) matches the project's section card convention. ✓I'd like another pass after items 1–4 land. Items 5–7 can ship as a follow-up issue if you prefer.
— Leonie
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
Reviewed in BROWNFIELD mode against the acceptance criteria implicit in issue #385 and the PR description. The functional shape matches the issue's intent — density bars above the document list, click-to-filter, drag-to-zoom-and-filter, filter-reactive density, hidden on mobile/calendar. A few requirements-quality observations below.
Concerns
Five of ten test-plan items are unchecked manual verifications
By our Definition of Done ("I've tested the happy path AND at least one error path"), these are blocking checks for the feature acceptance — every one is a critical user journey. Either tick them off after a manual pass, or — better, per Sara — codify the deterministic ones in Playwright. The current PR ships with the promise of correctness, not the proof.
NFRs not explicitly addressed in the PR description
/api/documents/densityat 50k rows? Markus already flagged the in-memory aggregation. Pin a measurable threshold (p95 < 200ms at 50k rows) so the next perf audit knows when to refactor.<sm. Document this as a deliberate scope decision — preferably as a comment in the issue, so future readers don't treat it as a bug.The "drag-to-zoom-AND-filter atomically" behavior is non-obvious to first-time users
This is gold-plating territory but worth flagging: the click-vs-drag distinction (click → filter only; click on year → filter + zoom; drag → filter + zoom) is a clever interaction model, but it has no on-screen affordance. A short tooltip on the widget ("Klicke einen Balken zum Filtern · Ziehe zum Vergrößern") or a one-time hint would close the gap for the senior audience. Track as a separate enhancement issue rather than expanding this PR.
LGTM (requirements quality)
from/tofrom the density endpoint ("the chart is the surface for picking those") is clearly stated in code comments — that's a non-trivial design decision properly documented at the right altitude.Suggested follow-up issues (do not block this PR)
feat(documents): widen timeline visibility to tablet (640–1024px)— Leonie's tablet touch-target concern.feat(documents): keyboard-accessible range zoom for timeline— Leonie's WCAG 2.1.1 finding.chore(documents): SQL aggregation for density at scale— Markus's row-count concern, gated ondocuments > 50k.— Elicit
Replaces the raw "1915-08 · 5" aria-label, which a screen reader announces as "1915 dash 08 middle dot 5", with the i18n template timeline_bar_aria("{when}, {count} ...") and a getLocale-formatted month/year string. Closes Leonie's WCAG 1.3.1 / 4.1.2 finding and Felix's localisation flag. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Splits the bar row + drag-window overlay + bar styling out of the 377-line orchestrator into a single-purpose component. The pointer choreography (handle{PointerDown,DocumentMove,DocumentUp}, indexFromClientX, cleanupDragListeners) stays in the orchestrator per Felix's note. Closes part 1 of Felix's component-split concern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>👨💻 Felix Brandt — Review concerns addressed
Pushed 12 atomic commits resolving the open concerns from this PR's review round. Branch:
feat/issue-385-timeline-density-filter.✅ Resolved in this PR
6aceafdatimeline_count_label,timeline_loading,timeline_filtered_count,timeline_zoom_in)c9fb6774aria-labelviam.timeline_bar_aria+formatTickLabel(getLocale())dfdcacdbtext-[10px]totext-xs(12px floor) and X-axis row toh-4ea106e94isDragging(uses the previously-orphanedtimeline_dragging_aria_live)5028082dprefers-reduced-motion: reducedisables the bar transitionc06987da--timeline-bar-idle#0d3358(1.44:1) →#3a6e8c(3.33:1) — clears WCAG 1.4.1161d1c179sm→lg(now hidden on tablet to protect 44×44 touch target) — both+page.svelteand the+page.tsdensity-fetch gate52827ccctriggerSearch(zoomOverride?)intotriggerSearchKeepZoom()+triggerSearchWithZoom(from, to)77d282bbTimelineBars.svelte(bar row + drag-window overlay + bar styling)00682bacTimelineYAxis.svelte+TimelineXAxis.svelte(Y and X are not adjacent in the DOM, so two single-purpose components beats a discriminator-prop one)219d9a81TimelineControls.svelte(reset-zoom + clear buttons)e5739d7fThe orchestrator
TimelineDensityFilter.svelteis now 279 lines (down from 377), composing four single-purpose children and keeping only the pointer choreography (per @Felix's note that the document-level pointer listeners andindexFromClientXbelong in the orchestrator).🔁 Deferred to follow-up issues (per discussion with the maintainer)
feat(documents): keyboard-accessible range zoom for timelinetest(documents): timeline density Playwright coverageDocumentSearchCriteriarecordCache-ControlpolicyVerification
cd backend && ./mvnw clean package -DskipTests— clean build ✓cd backend && ./mvnw test -Dtest=DocumentControllerTest,DocumentDensityIntegrationTest,DocumentServiceTest— 257/257 ✓cd frontend && npx vitest run src/lib/document src/routes/documents— 543/543 ✓cd frontend && npx svelte-check— no new type errors in any touched file (261 pre-existing errors in unrelated modules)cd frontend && npx prettier --check .∧ ESLint — clean— Felix
🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Solid feature partition (records as DTOs, helpers extracted to
timeline.ts, components split by visual region) and the docs were updated alongside the code, which is what I want to see. Two architecture-level concerns before merge.Blockers
The migration index is dead code.
V61__add_document_date_index.sqladdsidx_documents_meta_dateand the comment justifies it as keeping the GROUP BY cheap — butDocumentService.getDensitydoes not issue aGROUP BY date_trunc('month', meta_date). It callsdocumentRepository.findAll(spec).stream()...and groups the entire result set in Java memory (seeDocumentService.java:163-170). The index will not be touched by the current query plan. Either:pg_dumpsize, autovacuum bookkeeping), orJpaSpecificationExecutorprojection, where the index actually pays.Mismatched rationale and implementation ages badly — six months from now nobody will remember why
idx_documents_meta_dateexists.C4 diagram is stale relative to the code.
docs/architecture/c4/l3-frontend-3b-document-workflows.puml:11says the client loader is gated bymatchMedia('(min-width: 640px)')and labels it "tablet/desktop only". The actual+page.ts:7uses(min-width: 1024px)(Tailwindlg) — that excludes nearly every tablet. Update the diagram and the description.Suggestions
DocumentService.getDensity(revisit at >50k docs, move to native query) is appropriately noted. Consider also flagging it indocs/audits/2026-05-07-pre-prod-architectural-review.mdso it doesn't get lost.DocumentDensityResult.minDate/maxDatedeliberately omit@Schema(REQUIRED)so they're optional in TS — that's correct given the empty-result case (return new DocumentDensityResult(List.of(), null, null)). Worth a one-line comment on the record so future contributors don't "fix" it.@GetMapping("/density")without@RequirePermission, consistent with the rest of/api/documents/search. Confirm with Nora that "anyAuthenticated" is the intended read-access policy across the document domain.LGTM
DomainExceptionnot needed here — pure read endpoint with no domain failure modes.Cache-Control: private, max-age=300on the controller layer.TimelineDensityFilterintoBars/XAxis/YAxis/Controlsfollows feature-package layering cleanly.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
The Svelte 5 idioms are clean —
$derivedeverywhere, keyed{#each}, components named after visible regions, clear prop discipline. The backend follows our service patterns. But there's one bug inTimelineDensityFilterthat will leak listeners and crash on unmount, and one quietly-swallowed error path I'm not OK shipping.Blockers
Document-level pointer listeners leak on mid-drag unmount.
TimelineDensityFilter.svelte:191-195:If the component unmounts during a drag — route change,
view=calendartoggle, viewport drops belowlg, anything —handleDocumentUpis never called, the listeners stay attached, andhandleDocumentMovewill keep trying to write todragEndIndexon a torn-down state cell. Wrap with an$effectcleanup, or trackmountedand bail out:This needs a regression test too — render, dispatch pointerdown, unmount, assert
documentlisteners are gone.fetchDensityswallows errors silently (timeline.ts:198-216). Thetry { ... } catch { return EMPTY }form means a network blip, a 5xx, or a JSON parse error all collapse into the same "no buckets" rendering. Graceful degradation is fine for this widget — but at minimumconsole.warnso it surfaces in browser devtools and Sentry. As written, an outage is invisible to ops.Suggestions
String.format("%04d-%02d", d.getYear(), d.getMonthValue())inDocumentService.getDensityreads more cleanly asYearMonth.from(d).toString()— same output, name reveals intent.DocumentDensityIntegrationTest) uses@DirtiesContext(AFTER_EACH_TEST_METHOD)which restarts the full Spring context per test.@Transactionalrollback would be ~5x faster — Sara will probably push on this too.clipBucketsToRangeis exported and tested but the only call site is insideTimelineDensityFilter. Either drop the export or note it's part of the public helper surface.LGTM
density_returns401_whenUnauthenticated,getDensity_excludesDocumentsWithNullDate). Service unit tests, controller@WebMvcTest, and a real-Postgres integration test — all three layers.$derived.byfor multi-step computations,$statefor the drag indices, no$effectfor derived values.TimelineBars/TimelineXAxis/TimelineYAxis/TimelineControls/TimelineDensityFilterorchestrator. None over 60 lines except the orchestrator (279), which is justified by the drag state machine.DomainExceptionnot needed — this is a pure read with no domain failure modes; returning emptyDocumentDensityResultis correct.buildMonthSequence,tickIndicesFor,aggregateToYears) extracted and unit-tested intimeline.spec.ts(42 cases).🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infra surface change here — no new services, no Compose edits, no CI workflow changes, no new secrets, no port exposures. Read-through with my usual checklist.
What I checked
/api/documents/densityis behind the sameanyRequest().authenticated()policy as the rest of/api/documents/*.Cache-Control: private, max-age=300is correct —privatekeeps the response out of any shared cache (Caddy / future CDN),max-age=300lines up with the 5-minute browse-pattern stated in the PR description, and the controller test pins it (density_emitsPrivateCacheControlHeader). Good.V61__add_document_date_index.sqlis a singleCREATE INDEX IF NOT EXISTS— idempotent, safe to re-run, won't lock writers (CONCURRENTLYnot needed at our archive size; ~5k rows). Flyway will run it on next boot of every environment. No backfill, no data movement.Minor
documents.meta_dateis cheap and doesn't block this PR from a DevOps angle. If the architecture decision is to drop it (option (a) in Markus's comment), the right move is a follow-upV62that drops it, not amending V61.+page.tsis a universal load that doesbrowser && window.matchMedia(...)— correctly skips the fetch on SSR. The widget never blocks first paint of the document list. ✓LGTM
./mvnw test,npm run check,npm run lint, full vitest suite).:latestimage tags introduced.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Closes #385 with a feature that maps cleanly to the original brief — sharable URL state, atomic drag-zoom + filter, year fallback for long ranges, hidden on mobile and in calendar view. The acceptance criteria implied by the PR description are testable and largely covered. Two requirements-level gaps to resolve.
Concerns
i18n strings have no plural form. All three locales encode the count without ICU plural rules:
For
count = 1this reads as "1 documents" / "1 Dokumente" / "1 documentos" to a screen reader. Convert to ICU MessageFormat plural — Paraglide supports this. NFR: usability + accessibility.Documented breakpoint differs from implementation. The PR description, the C4 diagram, and the code disagree:
l3-frontend-3b):matchMedia('(min-width: 640px)'), "tablet/desktop only"+page.ts:7):(min-width: 1024px)— the Tailwindlgbreakpoint1024px excludes most tablets in portrait (iPad portrait is 768px) and even some 13-inch laptops in split-screen. Decide: is this mobile-and-tablet hidden or mobile only, then align the three sources. This is a real product question because the senior persona uses tablets heavily.
Suggestions
density === nullnothing renders. That's fine on first paint (the document list is already there) but worth stating in the issue: "no skeleton / no progress indicator for the timeline; absence is the loading state." Otherwise a future reviewer will file it as a bug.DocumentService.getDensity("revisit when documents > 50k") is a valid NFR boundary. Capture it as a follow-up issue with a label liketech-debtso it doesn't disappear into code-comment limbo.LGTM
from,to,zoomFrom,zoomTo) — preserves bookmarkability, which #385 calls out.🛡️ Nora "NullX" Steiner — Application Security
Verdict: ✅ Approved
Read-only endpoint, parameterized queries throughout, structured filter input that flows through the existing
Specificationbuilder, and the auth boundary is tested. Nothing here that I'd hold up.What I checked
density_returns401_whenUnauthenticatedproves the endpoint is gated bySecurityConfig'sanyRequest().authenticated(). NopermitAll()slipped in.@RequirePermissionon the endpoint — consistent with the rest of/api/documents/search, the read-access policy is "any authenticated user". The project doesn't enforceREAD_ALLat the controller layer for document reads, so this is policy-aligned. (If we ever introduce per-document RLS, this endpoint andsearchneed the same treatment together — flag for the architecture follow-up.)DocumentService.getDensityreusesbuildSearchSpecwhich builds a JPASpecificationwith parameterized predicates. No JPQL string concatenation.tagQ,q, sender/receiver IDs all flow through typed params. Clean.Cache-Control: privateprevents the response from being cached by intermediaries. Important here because the bucket counts vary by user-visible filter combinations and could theoretically leak existence of documents to a shared cache. Correctly set.fetchDensitycatch block is silent. Not a security issue per se, but it does mean a server 5xx is invisible to operators. Felix flagged this from the dev side — supporting his point.aria-label,formatTickLabel, and date strings render as text via Svelte's default escaping. NoinnerHTML, noeval. Tag/sender filter values reach the URL viaURLSearchParams.set(auto-encodes). Clean.Educational note
The
DocumentDensityResult.minDate/maxDateare not@Schema(REQUIRED)and are nullable in the empty-result case. Worth knowing that this means the TypeScript type has them as optional — frontend code should never assume they're present (which+page.tscorrectly handles via?? null).LGTM
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
The pyramid is well-balanced for this feature: 4 service unit tests, 5 controller
@WebMvcTesttests including auth and headers, 7 Testcontainer integration tests on real Postgres, plus 25 component tests and 42 pure-function tests on the frontend. That's coverage I trust. Three things to fix or follow up.Blockers
TimelineDensityFilterunmounts mid-drag, document-levelpointermove/pointerup/pointercancellisteners stay attached. Add a vitest-browser-svelte test:Concerns
@DirtiesContext(classMode = AFTER_EACH_TEST_METHOD)inDocumentDensityIntegrationTestrestarts the full Spring context per test method — for 7 tests in this class that's 7× full app boot (≈10–15s each in our config). The whole class runs alone in the suite when it doesn't need to. Replace with:Each test rolls back its own writes, no context restart. CI time drops from ~90s to ~15s for this class alone.
Accessibility regression coverage missing. The widget exposes
role="group",aria-pressed, anaria-live="polite"region, andaria-labelon each bar — but none of that is asserted with axe. Add to the documents page E2E:If the widget hides below
lg(1024px), setviewport: { width: 1280, height: 900 }.Suggestions
onchangefires with the rightfrom/to. Aria-pressed is set; let's prove it works.formatTickLabelis locale-dependent. The test attimeline.spec.ts:330ishlikely hardcodes English month abbreviations — make sure CI's locale matches or pass an explicit locale to keep the test deterministic across runners.Thread.sleep, no global timers, no real network — appreciated.LGTM
density_emitsPrivateCacheControlHeader).verify(documentService).getDensity(eq(...), ...)— proves the controller passes params through, doesn't just smoke-test.🎨 Leonie Voss — UX & Accessibility
Verdict: 🚫 Changes requested
Lots to like — the widget uses our brand tokens, respects
prefers-reduced-motion, computes a 3.33:1 dark-mode bar contrast that clears WCAG 1.4.11, exposes an aria-live drag preview, and hasaria-label+aria-pressedon every bar. But two accessibility blockers and one broken-promise breakpoint that I can't approve through.Blockers
Touch targets in
TimelineControlsfail WCAG 2.5.8.TimelineControls.svelte:18and:25:h-6is 24px — that's the WCAG 2.2 floor, but our project standard for the senior audience (60+) is 44×44 minimum, prefer 48×48. Even though the widget hides belowlg, seniors absolutely use desktop. Fix:No keyboard alternative to drag-zoom. Single-bar selection works (the bar is a
<button>, Enter/Space firesonclick,suppressClickonly suppresses post-pointerup synthesized clicks). But range selection is mouse-only — there is no keyboard path to select a range. WCAG 2.1.1 ("Keyboard") makes this a Level A failure. Two reasonable options:from, second Enter on a later bar setstoand commits. Show the in-progress state via the existingaria-liveregion.Either path needs a vitest-browser test asserting the keyboard journey. Pure mouse drag is not enough.
Documented breakpoint contradicts the implementation. The l3-frontend C4 diagram and the description say "tablet/desktop only" /
(min-width: 640px), but the code uses(min-width: 1024px)(thelgtoken). 1024px excludes iPad portrait (768px) and 13" laptops in side-by-side. This affects the user persona split — readers on tablets will lose this feature silently. Decide and align.Concerns
No focus-visible style on the bar buttons. They render with
bg-transparent p-0— so the default browser outline is what keyboard users get, and our project standard isfocus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2. Add the rings to.barso keyboard users can see where they are inside the timeline.timeline_bar_ariaplurals are wrong. All three locales hardcode the plural form:"{count} documents","{count} Dokumente","{count} documentos". Forcount = 1a screen reader announces "August 1915, 1 documents". Use Paraglide's ICU plural form. NFR: a11y + i18n quality.:rootselector in a scoped Svelte<style>block (TimelineBars.svelte:69) is a code smell. It defines--timeline-bar-idleglobally even though the style block is scoped — it works because:rootis a global selector, but it spreads component CSS contracts into the global namespace. Move the variable definitions intolayout.cssnext to the other--palette-*tokens, or scope them to the timeline root with a class selector.Suggestions
<small>text hint above the timeline on first interaction: "Drag across bars to zoom into a range." Discoverability of the drag affordance is a known weak point in Graylog-style selectors.count === 0for many consecutive months — the 2px sentinel bars will read as a near-empty row. That's correct information, but a subtle muted background stripe (already mostly there viaborder-line) helps the user perceive "yes, this is the selected/visible area, not a render bug."density.length === 0after filtering: the widget renders{#if density !== null}but if all buckets are filtered out,filledis empty and you get an invisible bar row plus the controls. A small message ("No documents in this filter") would help.LGTM
bg-surface,border-line,text-ink-3,--palette-mint).prefers-reduced-motionrespected.aria-live="polite"for drag preview, polite-tone (correct vs assertive).↩) and clear (×) icons havearia-label+title.Bars,XAxis,YAxis,Controls).YearMonth.from(d).toString() emits the same canonical YYYY-MM string as the previous String.format("%04d-%02d", …) call but reads as a single intent-revealing expression. Existing assertions on "1915-08", "1916-01", … pin the output format unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>The flat "{count} Dokumente / documents / documentos" keys read as "1 Dokumente" / "1 documents" / "1 documentos" to a screen reader when only one document falls in the month bucket. Splits each locale into _singular + _plural keys and picks the form by count in TimelineBars, mirroring the existing upload_banner_singular / _plural pattern in this project. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Round-2 review concerns addressed
Pushed 12 atomic commits resolving the open round-2 concerns. Branch:
feat/issue-385-timeline-density-filter.✅ Resolved in this round
DocumentService.getDensity360db1aeString.format("%04d-%02d", …)→YearMonth.from(d).toString()47841b91DocumentDensityResultexplaining whyminDate/maxDatedeliberately omit@Schema(REQUIRED)ffe617dbDocumentDensityIntegrationTest:@DirtiesContext(AFTER_EACH_TEST_METHOD)→@Transactionalrollback. Class wall time dropped from ~35s to ~18s locallyc9be6cc1fetchDensitynowconsole.warns on non-ok status and on caught error so outages aren't invisible to ops/Sentry2e9ce8e1$effectreturn on unmount mid-drag — with a regression test that pins the behaviour3b6b117cTimelineControlsreset-zoom + clear buttons bumped fromh-6toh-11/min-w-[44px]/w-11— clears WCAG 2.5.8 (AAA prefer-48 will be revisited if Leonie pushes)153752a9focus-visible:ring-2 ring-brand-navy ring-offset-2on bar buttons so keyboard users can see focus48da819atimeline_bar_ariasplit into_singular+_pluralper project convention (mirrorsupload_banner_*);TimelineBarspicks the form by count. No more "1 documents/Dokumente/documentos"1d6016cb--timeline-bar-idle/--timeline-bar-outsidemoved out ofTimelineBars's scoped<style> :rootand intolayout.cssnext to the rest of the design tokens5d749b24clipBucketsToRangecarries an@internalJSDoc note explaining why it is exported (sotimeline.spec.tscan pin boundaries directly)bf501b7dl3-frontend-3b-document-workflows.pumlupdated:'(min-width: 640px)'→'(min-width: 1024px)', "tablet/desktop only" → "desktop only (≥1024px)". PR description also aligned.🔁 Deferred (per the round-1 discussion + Markus's defer agreement)
DocumentSearchCriteriarecordCache-ControlpolicyformatTickLabeltestgetLocale()so no flake observedVerification
cd backend && ./mvnw test— 1540/1540 ✓ (BUILD SUCCESS)cd frontend && npx vitest run src/lib/document src/routes/documents— 551/551 ✓ across 54 filescd frontend && npm run lint— cleancd frontend && npx svelte-check— 0 new errors in any touched file (261 pre-existing in unrelated modules)— Felix
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
A clean, boring, well-bounded slice. The chart sits inside the existing
document/package, the controller delegates to the service, the service composes the existingSpecificationinstead of introducing a parallel query path. No new modules, no new transport protocols, no new infrastructure. This is exactly how I want feature work to land.What I checked
DocumentController.density()→DocumentService.getDensity()→documentRepository.findAll(spec). No cross-domain reach.tagService.expandTagNamesToDescendantIdSets(...)(visible in the unit-test mock) goes throughTagService, which is the boundary I expect. ✅docs/architecture/c4/l3-backend-3b-document-management.pumladds the density description;docs/architecture/c4/l3-frontend-3b-document-workflows.pumladdsdocsListPageTsandtimelineFiltercomponents plus the new HTTP edge). This is the part most PRs miss; well done. ✅DocumentDensityResultandMonthBucketlive inorg.raddatz.familienarchiv.document. Not shared. ✅Suggestions (non-blocking)
DocumentService.getDensityJavadoc, line ~98). The trade-off is correctly named and the threshold is concrete. Consider opening a follow-up issue now taggedperfso it isn't lost; comments age into invisibility. The migration target is also clear (GROUP BY TO_CHAR(meta_date, 'YYYY-MM')), which means future-me has a starting point.*/*content type on the OpenAPI spec fordensity(frontend/src/lib/generated/api.ts:2485) — Spring is producing the generic media-type because the controller doesn't declareproduces = MediaType.APPLICATION_JSON_VALUE. The other read endpoints in this controller likely have the same shape, so this is a project-wide tidy, not a blocker for this PR. Worth a one-lineproduces = ...when someone next touches the controller signature.from/to" is a non-obvious design rule encoded in three places (Javadoc, integration test class comment,buildDensityUrltest). That's load-bearing context. If anyone reaches for "let's just forward all filters" in 6 months, an ADR ordocs/specs/note would prevent a regression. Not a blocker — the code comments are unusually good — but it's the kind of thing future-Markus would thank present-Markus for writing down.What I did NOT need to flag
Cache-Control: private, max-age=300does the work browser-side without standing up Redis.+page.tsCSR loader is the right call for a chart that the SSR-friendly main list shouldn't wait on. Justified deviation from SSR-default. ✅Boring, readable, scoped. Ship it.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
TDD evidence is strong (44 timeline-helper specs + 35 component specs + 7 service unit/integration tests, all in front of meaningful implementations), guard clauses replace nesting, names reveal intent. Two readability concerns in the orchestrator, a few minor nits, nothing blocking.
Concerns
1.
TimelineDensityFilter.svelteis 286 lines (frontend/src/lib/document/TimelineDensityFilter.svelte). The bar/axis/controls children were extracted — good — but the orchestrator now carries five concerns: drag state machine, selection-vs-zoom routing, drag-window math, aria-live message derivation, and click-vs-drag suppression. Each is justified individually, but together they tip past the "one nameable region" rule.If you want to split later, the cleanest cleavage is the drag state machine —
dragStartIndex,dragEndIndex,suppressClick,handleDocumentMove/Up/Cancel,cleanupDragListeners,finalizeDrag,indexFromClientX,dragWindowLeftPct/RightPct,dragLiveMessageform a coherent unit that could live in auseTimelineDrag.svelte.tsrune-class. That would leave the component as a thin choreographer (~140 lines), and the drag state would be unit-testable without a DOM. Not a blocker for this PR — flag for the perf/cleanup pass.2.
DocumentService.getDensitydoes three things (backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java:109): FTS short-circuit, spec build + load, in-memory grouping. That's at the edge of "20 lines, one thing". A small refactor would be:Each helper is testable in isolation. Right now the unit-test pyramid leans on heavier
findAll(any(Specification.class))mocks than necessary. Again, non-blocking — the existing tests do work.Suggestions (nice-to-have)
getDensity— 7 positional args (text, sender, receiver, tags, tagQ, status, tagOperator) is the kind of signature where the next caller swaps twoUUIDs by accident. ADensityFiltersrecord (mirroring the TSDensityFilterstype) would lock the order at compile time.@SuppressWarnings("rawtypes")on theSpecification.classmatchers —any(Specification.class)triggers an unchecked-warning for the diamond.any(Specification.class)cast orArgumentMatchers.<Specification<Document>>any()reads cleaner. Cosmetic.monthBoundaryTocleverness (frontend/src/lib/document/timeline.ts:43) —new Date(Date.UTC(year, month, 0)).getUTCDate()exploits that day-0 of next month equals the last day of current month. Correct for leap years (the test suite proves it:1916-02 → 29), but the trick is invisible to a future reader. A one-line "day 0ofmonth+1≡ last day ofmonth" comment would help. The "no comments unless non-obvious" rule applies here: this is non-obvious.console.warnon density failure (fetchDensity) is the right call for graceful degradation — the chart is an enhancement, the list still renders. The codepath is intentionally distinct fromgetErrorMessage()because there's no user-facing error to localise. ✅ Confirming this is the right pattern, not flagging it as wrong.What's done well — explicitly
timeline.tsare exemplary. Twelve standalone functions, each one expression or a single loop, each with a focused test block. This is the "make business logic testable by extracting it from the component" rule applied perfectly.@derived/@derived.byeverywhere instead of$effectfor computed values.monthBuckets,filled,isZoomed,maxCount,hasSelection,dragLowIndex,dragHighIndex,dragWindowLeftPct/RightPct,dragLiveMessage— every reactive value is$derived. No effects-as-derivation anti-patterns.{#each}on bars ((bucket.month)). ✅SvelteMapnot used — and not needed; the data is plain arrays. ✅$effect(() => cleanupDragListeners)for unmount mid-drag. The test ('removes document pointer listeners when unmounted mid-drag') proves it. ✅DomainExceptionnot raw exceptions — the controller delegates to a service that doesn't throw; the empty-result path returns an emptyDocumentDensityResult. NoOptional.get(), noRuntimeException. ✅Verdict
Component-size flag is a "next pass" concern, not a blocker for this PR. Ship it; come back for the drag-state extraction in the next density iteration.
🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved (LGTM — nothing to operate)
No infrastructure surface in this PR. No Compose changes, no CI workflow edits, no new service, no new env var, no new secret, no new image, no new volume, no new exposed port. The 25-file diff lives entirely in
backend/,frontend/, anddocs/. From an ops chair this is a code-only change — there's literally nothing for me to size, monitor, or alert on.What I checked anyway
docker-compose*.ymluntouched. ✅application*.yamland.env*untouched. ✅pom.xmlandpackage.jsonuntouched, so no Renovate config update needed. ✅/api/documents/densityis under/api/*like the rest of the application traffic. Nothing to block at Caddy. ✅console.warnon density-fetch failure (degraded path). Won't flood Loki. ✅One thing worth noting (not a finding, just situational awareness)
Cache-Control: private, max-age=300on the response means the browser caches per-user. We do not terminate at a shared CDN/Caddy cache for/api/*, soprivatehere is belt-and-braces — the right kind of belt-and-braces. If we ever introduce a shared cache in front of the API (we won't, until metrics demand it), this header already forbids it from picking up the response and serving it cross-tenant. Defensive default — keep it.What I'd flag if it were here, and isn't
:latestimage tag? No.Nothing to do on my side. Merging this doesn't touch the ops bill (still ~23 EUR/month) or the runbook.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Read endpoint, parameterised query path, no string concatenation, authenticated-only,
Cache-Control: private. Threat surface is small and the implementation didn't widen it. I went looking for the usual suspects and didn't find them.What I checked
1. AuthN gate is enforced (
DocumentControllerTest.density_returns401_whenUnauthenticated, line 1244-1247) —mockMvc.perform(get("/api/documents/density")).andExpect(status().isUnauthorized()). SecurityConfig'sanyRequest().authenticated()covers it; explicit regression test pins the behaviour so a futurepermitAll()slip would break the build.2. No
@RequirePermissionon the endpoint — checked the project rule (CLAUDE.md:@RequirePermissionrequired on POST/PUT/PATCH/DELETE). GET is not required by the convention. The endpoint exposes per-month aggregates of metadata the user already has read access to via/api/documents/search— no privilege boundary is being crossed by exposing density. Consistent with the existing read endpoints inDocumentController. Not a finding.3. SQL/JPQL injection — the controller passes user input straight into
DocumentService.getDensity(...), which composes aSpecification<Document>via the samebuildSearchSpec(...)the search endpoint already uses. No string concatenation, no native query, noem.createQuery("..." + userInput).qis forwarded todocumentRepository.findRankedIdsByFts(text)— a JPA repository method, parameterised by definition. ✅4. Mass assignment / parameter pollution — request params are typed (
UUID,DocumentStatusenum).tagOpis whitelisted:"OR".equalsIgnoreCase(tagOp) ? OR : AND. Anything not "OR" defaults to AND — no unbounded enum lookup. ✅5. Authorization-via-filter sidechannel (IDOR-ish) — the endpoint accepts
senderId/receiverIdand returns counts. If we had per-document ACLs, an attacker could enumerate counts for documents they cannot read. We don't have per-document ACLs (READ_ALL is global), so this isn't a sidechannel today. Worth flagging in the threat model if document-level ACLs are ever introduced — the density endpoint would need the same predicate as the list. (Pin indocs/threats/or wherever you keep this.) Non-blocking.6. Cache-Control —
private, max-age=300(DocumentController.java:39). Prevents shared-cache poisoning across users. The right header for an authenticated, filter-personalised response. ✅7. CORS / CSRF / SameSite — endpoint is read-only (GET), no state change, no body. CSRF doesn't apply. CORS is project-wide config and untouched. ✅
8. Frontend output encoding —
aria-label, button text, and tick labels go throughformatTickLabel(...)which usesIntl.DateTimeFormat. NoinnerHTML, no{@html}, noeval. Theconsole.warnon fetch failure stringifies aTypeErrorand a numeric status — no sensitive payload. ✅9. Logging — controller uses no
logger.info(...)with user input. Service-level logging unchanged. No Log4Shell-shaped concatenation. ✅10. Secrets — none introduced.
Educational note (no fix required)
Density endpoints are a classic info-disclosure helper for any system that does have row-level ACLs — they leak existence-by-count even when the documents themselves are filtered out of search results. Worth keeping in mind if the access model ever evolves (e.g. private documents per family member). Today it's not exploitable; the symmetry between density and search predicates means anything visible in the chart is visible in the list.
Static-analysis suggestion (one for
tobi's CI list, not this PR)If/when we add a Semgrep rule for "JPA repository method invoked with concatenated string", the codebase already passes — but pinning the rule prevents a future regression. Filing this as a low-pri gate-hardening, not a blocker here.
Ship it.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The pyramid is well-shaped. Static + unit + integration + component layers all have meaningful coverage with real Postgres at the integration tier. Two missing layers and a handful of test-style nits — none block merge given the follow-up issues already filed for the gaps.
Coverage I verified
DocumentServiceTest.getDensity_*DocumentControllerTest.density_*@WebMvcTest-style sliceDocumentDensityIntegrationTest@Import(PostgresContainerConfig.class)— real PG 16 ✅timeline.spec.tsTimelineDensityFilter.svelte.spec.tsvitest-browser-svelte(real DOM via Playwright Chromium) ✅routes/documents/page.svelte.spec.tsPR description claims
1540 backend tests, all greenand the new specs pass — the test counts inTest planmatch the file-level counts I see in the diff.Concerns (none blocking, all addressed by follow-ups or stylistic)
1. No E2E for the timeline interactions yet. The drag-to-zoom, click-filter, click-year-zoom, reset-zoom flow is the marquee feature of this PR. Component tests prove the callback fires; they don't prove the URL round-trip and rerender wire it back through the page loader correctly under real network conditions. #480 is filed and adequate — the component tests are dense enough that I'm comfortable letting E2E follow rather than gate. Ship pattern: feature lands behind component coverage; #480 closes the loop within one cycle.
2. No axe accessibility check in CI for the timeline route. Component tests assert WCAG-shaped properties (touch-target classes
h-11/min-w-[44px]/w-11,focus-visible:ring-*,aria-pressed,aria-labelcontent, singular/plural pluralisation,aria-live="polite"text content during drag). That's solid behavioural coverage — but it doesn't run axe against the rendered page. #480 covers it. When that lands, it should run axe in both light and dark mode (dark-mode contrast on--timeline-bar-idle: #3a6e8cagainst--c-surface: #011526is asserted in CSS comments at 3.33:1 — axe will verify it independently).3. Multiple assertions per test in a few places (style nit). Examples:
getDensity_returnsAllMonths_whenNoFiltersApplied(DocumentDensityIntegrationTest) assertsbuckets.monthandbuckets.countandminDateandmaxDatein one method.density_returns200_withResultBody_whenAuthenticatedchains 5jsonPathexpectations.When one fails in CI, the developer can still tell which dimension broke from the assertion message — but a
should_return_correct_min_date/should_return_correct_max_datesplit would name the regression on a one-line CI summary instead of "expected … but got …". Non-blocking; flag for the next refactor pass.4. Integration coverage gap: tag OR operator.
getDensity_filtersByTagonly exercisesTagOperator.AND. The controller test forwardstagOp=ORcorrectly, but no integration assertion proves the OR predicate produces the right buckets against real PG. ExistingsearchDocumentsintegration tests probably already pin OR semantics on the samebuildSearchSpec— if so, defensible. Worth a one-line add when convenient.5. Integration coverage gap:
qparameter (FTS path). The unit testgetDensity_shortCircuits_whenFtsReturnsNoMatchescovers the empty-FTS case. No integration test seeds documents with searchable titles and proves the FTS-filtered density buckets. Same logic applies — covered transitively by existing search integration tests, defensible. Add when convenient.6.
@MockitoBean S3Client s3Clientin the integration test — pragmatic (avoids needing a MinIO container for a read-only metadata test), andS3Clientisn't exercised bygetDensity, so mocking it is correct. Just noting it as the right call, not a finding.Confirmed strengths
makeProps) instead of repeating the bucket array inline. ✅'dragging from a later bar to an earlier bar still emits ascending boundaries'reads as documentation. ✅@Transactionalon integration test class for automatic rollback. ✅Thread.sleep()anywhere. The component tests usetick()for Svelte reactivity, not arbitrary timeouts. ✅'removes document pointer listeners when unmounted mid-drag') — exactly the kind of regression test that catches a future "I'll just remove that effect" refactor. ✅(?:documents|Dokumente|documentos)with explicit singular/plural assertions. ✅Ship criteria
Backend pyramid: green. Frontend pyramid: green at the layers below E2E. E2E gap is acknowledged and tracked. Axe gap is acknowledged and tracked.
Approve with the assumption #480 lands in the next iteration. If it slips two cycles, raise it.
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This is the most accessibility-conscious feature PR I've reviewed in months. WCAG 2.5.8 (touch targets), 2.4.7 (focus visible), 1.4.11 (non-text contrast), 4.1.3 (status messages), and 1.4.12 (text spacing via
text-xs/12px floor) are all met with explicit tests — not just hoped for. Two minor surface concerns and one observation about the senior audience, neither blocking.What I checked, with exact findings
Touch targets — WCAG 2.5.8 (44×44 minimum):
h-11 min-w-[44px] px-3(TimelineControls.svelte:13). 44px height × ≥44px width. ✅ Test pins it ('reset-zoom button is at least 44×44 (WCAG 2.5.8)').h-11 w-11(TimelineControls.svelte:23). 44 × 44. ✅ Tested.min-w-pxfloor and the year-aggregation fallback at >240 months keep them clickable. ✅ Acceptable.Focus indicators — WCAG 2.4.7:
focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2 focus-visible:outline-none(TimelineBars.svelte:67). Brand-navy on sand surface = high contrast. ✅ Tested.hover:bg-canvas hover:text-ink-1— visible without the explicitfocus-visible:ring-*pattern though. Minor: prefer adding the samefocus-visible:ring-2 focus-visible:ring-brand-navyto the reset-zoom and clear-selection buttons for visual consistency with the bars. Not a regression — current state is still keyboard-discoverable via the hover styling — but the bar/control parity matters for the senior keyboard user. Non-blocking.Non-text contrast — WCAG 1.4.11 (3:1 minimum for UI components):
--timeline-bar-idle: rgba(161, 220, 216, 0.35)over--c-surface(sand) — needs verification with a tool, the alpha makes static math non-trivial.--timeline-bar-idle: #3a6e8cover--c-surface: #011526— comment inlayout.css:233-234claims 3.33:1, which is the right calculation for non-text UI controls. ✅--palette-mint #a1dcd8over both surfaces. Mint is a brand colour, ratio against white likely passes; against sand needs verification. Recommendation: add a one-line comment to layout.css confirming the light-mode ratio with the actual sampled value (rgba(161, 220, 216, 0.35) over #f5efe6 ≈ X:1) so the regression test against a future palette tweak is explicit. Non-blocking.Status messages — WCAG 4.1.3:
aria-live="polite"region (TimelineDensityFilter.svelte:1038) populated only during drag, empty otherwise so AT doesn't announce residuals. ✅ The test ('renders a polite aria-live region whose text reflects the dragged range') verifies the empty→populated→empty cycle.m.timeline_dragging_aria_live({from, to})— three locales. ✅aria-pressedon bars to convey selection state to AT. ✅Pluralisation — i18n correctness:
timeline_bar_aria_singular/timeline_bar_aria_pluraldistinguished withcount === 1branch (TimelineBars.svelte:33-39). Tested across de/en/es. ✅ This is the correct way to do pluralisation — Paraglide doesn't auto-pluralise, so explicit is right.Reduced motion — WCAG 2.3.3:
@media (prefers-reduced-motion: reduce)disables bartransition: background-color(TimelineBars.svelte:78-82). ✅Typography floor — 12px minimum:
text-xs= 12px (TimelineYAxis.svelte:13). ✅text-xs h-4= 12px on a 16px line. ✅'Y-axis labels meet the 12px minimum font floor (Tailwind text-xs)'and'X-axis row uses text-xs and h-4 to fit the 12px line height'. The negative assertion (not.toMatch(/text-\[10px\]/)) is exactly the kind of regression-blocker test I want to see.Concerns (minor)
1. Hover-stuck on touch devices. Bars have
hover:styles applied (bar:hover .bar-fill { background-color: var(--palette-mint) }). On touch devices that fire emulated mouse events, a tap can leave a bar in:hoveruntil the next tap elsewhere. This widget is hidden belowlg(1024px), so most touch users never see it — but a Surface Pro or iPad Pro in landscape ≥1024px will. Easy fix when convenient: wrap hover in@media (hover: hover). Non-blocking because the visual regression is cosmetic, not functional.2. Mobile/tablet exclusion under
lg. The+page.tsloader skips the fetch and the wrapper hashidden lg:block. Reasoning is correct (sub-pixel bar widths fail the 44×44 floor, drag gesture is hard on small screens). I want to confirm the replacement affordance below 1024px is clear: the date-range filter inSearchFilterBaris the answer. Worth an explicit visual note in the/documentsmobile screenshot in the next design spec — "below this breakpoint, use the date pickers" — so users don't wonder why the chart vanished. Non-blocking; cosmetic doc nit.3. Drag gesture has no keyboard equivalent (#479 filed). Confirmed. WCAG 2.1.1 (Keyboard) applies. The bar buttons are keyboard-focusable and clickable (Enter/Space → single-month filter — verified by the click-handler path that preserves
suppressClick = falsefor keyboard events). What's missing is the range selection — drag has no keyboard analogue. #479 is the right scope: shift-click for range, or numeric date inputs that mirror the chart's selection. Tracked, not blocking.Senior-audience review (60+ persona)
text-xs(12px) for axis labels which is at the floor. The aria-labels are read aloud by AT, so visual size is less critical for tactile/screen-reader users.aria-pressed+class:selected. ✅Brand compliance
brand-navyfor focus rings,brand-mintfor selection/preview,--c-surfacefor the card. All semantic tokens, no hardcoded hex outsidelayout.css. ✅rounded-sm border border-line bg-surface shadow-sm) matches the project standard. ✅Verdict
This is what an accessibility-aware feature looks like. The follow-ups (#479 keyboard range, #480 axe in CI) close the remaining gaps. Ship it.
📋 Elicit — Senior Requirements Engineer
Verdict: ✅ Approved
I'm reviewing this against the spec implied by issue #385 and the surrounding NFR landscape. Acceptance coverage is strong, follow-ups are filed for the known gaps, the design rule "filter-reactive but range-independent density" is documented in three load-bearing places (Javadoc, integration test class doc, helper-test comment) instead of being implicit. From a requirements-engineering chair, this is a feature that won't decay.
Acceptance coverage (against #385 inferred AC)
TimelineDensityFilter.sveltemounts indocuments/+page.svelteabove the result counthandleClick→emitSelection(idx, idx, false)→onchange({from, to})→ URL params'clicking a bar emits the boundary dates of that month'finalizeDragemitsonchange({from, to, zoomFrom, zoomTo})in one call'dragging from bar A to bar B emits a single onchange with filter + zoom (atomic)'clearSelection→onchange({from: '', to: ''})'clicking the clear button emits empty dates'resetZoom→onzoomchange(null)(nofrom/tochange)'clicking reset-zoom emits onzoomchange(null)'+ page wiring test'clicking reset-zoom drops zoomFrom/zoomTo from the URL'aggregateToYearstriggered whenmonthBuckets.length > 240'collapses to year buckets when the month sequence exceeds the limit'isYearLabel(label)→ emit zoom alongside filter'clicking a year bar zooms into that year (filter + zoom atomic)'hidden lg:blockwrapper ++page.tsskips fetch'hides the timeline widget when density is null (mobile / calendar view)'view === 'calendar'short-circuit infetchDensity'skips fetch when view is calendar'buildDensityUrlexcludesfrom/toby type design'does not forward from/to even if a caller mistakenly adds them'buildSearchParamswriteszoomFrom/zoomToprivate, max-age=300'density_emitsPrivateCacheControlHeader'Every AC I'd write for this feature has implementation + test evidence in the diff. ✅
NFR coverage
getDensityJavadoc; current load (<5k docs) keeps p95 under target with in-memory aggregation; browser cache absorbs repeated loads (5min)GROUP BY TO_CHAR(meta_date, 'YYYY-MM')" — concrete next step, not "we'll figure it out"fetchDensitycatches network + non-ok responses, returnsEMPTYstate. List page keeps rendering.Cache-Control: privateIntl.DateTimeFormat,URLSearchParams,matchMedia— all baseline browser APIslg(1024px) breakpoint as cut-off, fetch skipped belowtimeline.tsseparated from component DOM logicconsole.warnon density failureOpen questions / TBD register (from this PR)
perfissue alongside this mergelgbreakpoint?SearchFilterBardate pickers fill this role; worth one screenshot in the specdocs/specs/noteScope discipline
PR holds tight to #385's scope. No gold plating — no chart-type toggle, no custom date input replacing the bars, no animation library. The drag-to-zoom is a replacement for an originally-planned standalone zoom-in button (visible in the negative test
'does not show the zoom-in button (drag replaces it as the zoom gesture)'), which is a justified UX simplification, not feature creep.Backlog hygiene observation
The fact that #479 and #480 were filed alongside the PR (not after) is the workflow I want every feature to follow. Known gaps documented at landing-time, with explicit owners (Leonie for WCAG; Sara for E2E + axe). This is what "Definition of Done — known gaps tracked, not silently deferred" looks like.
Verdict
Approve. Merge. File OQ-385-1 as a
perfissue before closing #385, and the requirements trail is clean.👨💻 Felix Brandt — Round-3 review concerns addressed
Pushed 9 atomic commits resolving the open round-3 suggestions. All seven reviewers approved on the previous round; this round picks up the explicitly non-blocking suggestions (no blockers were outstanding).
✅ Resolved in this round
monthBoundaryToday-0-of-month+1 idiom intimeline.tsf68d16effocus-visible:ring-2 ring-brand-navy ring-offset-2onTimelineControlsreset and clear buttons (visual parity with bars)dd0a77a5:hoverin@media (hover: hover)so a tap on a Surface Pro / iPad Pro at ≥1024px doesn't leave a bar stuck in:hover18aaf1f3--timeline-bar-idlecontrast (~1.13:1 vs#ffffff) as a decorative-fill carve-out alongside--c-accent7f99c64dproduces = MediaType.APPLICATION_JSON_VALUEon the density endpoint + a controller test that pins refusal of non-JSON Accept headersc0a1f04dperffollow-up) for the >50k row SQL aggregation TODO; updated the Javadoc to crosslink the issue00f35ab6getDensity(...)positional args into aDensityFiltersrecord so callers can't swap twoUUIDs by accident86de118dDocumentService.getDensityintoresolveFtsIds/loadFilteredDates/aggregateByMonthprivate helpers; orchestrator now reads as 6 lines5cd6ecc6useTimelineDrag.svelte.tsrune-class factory; orchestrator drops to ~150 lines and the drag state is unit-testable without a DOM (13 new specs)9fd1f3cd🔁 Carrying forward / out of scope
DocumentSearchCriteriarecordCache-ControlpolicysearchDocumentsintegration tests*/*content type in the OpenAPI emitproduces=APPLICATION_JSON_VALUE(#5 above). The api.ts regen will pick this up on the next backend restart; runtime is already locked by the controller test.Verification
cd backend && ./mvnw test— 1541 / 1541 ✓ (BUILD SUCCESS)cd backend && ./mvnw clean package -DskipTests— clean build ✓cd frontend && npx vitest run src/lib/document src/routes/documents— 567 / 567 ✓ across 55 files (includes the new 13useTimelineDragspecs)cd frontend && npm run lint— clean (Prettier + ESLint)cd frontend && npx svelte-check— 0 new errors in any touched file (261 pre-existing in unrelated modules)— Felix
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Solid, boring, well-justified change. The new endpoint reuses the existing
Specificationmachinery, the in-memory aggregation tradeoff is documented with a concrete trigger to revisit it, and the documentation update wasn't an afterthought.What I checked
DocumentServicecallsdocumentRepositorydirectly (its own domain) andtagService(cross-domain via service, not repository). Clean.DensityFilters,MonthBucket, andDocumentDensityResultall live inside thedocumentpackage. No coupling to other modules.Cache-Control: private, max-age=300. Right tool. No new infra.DensityFiltersandDocumentDensityResultuse Java records, which is the right call for immutable value bundles.MonthBucketlikewise.docs/architecture/c4/l3-backend-3b-document-management.puml—DocumentControllerdescription amended to mention density. ✓docs/architecture/c4/l3-frontend-3b-document-workflows.puml— addsdocsListPageTsandtimelineFiltercomponents plus theRelarrows. ✓Suggestions (non-blocking)
resolveFtsIdsnullvs empty list semantics (DocumentService.java:158) — usingnullto mean "no FTS query" andList.of()to mean "FTS ran but matched nothing" is correct but subtle. Each caller has to remember the convention. IfgetDensityever grows a third caller, consider anOptional<List<UUID>>or a smallFtsResultrecord. Not a blocker — the current single-caller use is fine./api/documents/densityp95 latency) so the trigger fires on data, not on someone remembering. Tobias's call.Cache-Control choice is right:
private(per-user filter combinations) + 5 min (browse-tab refresh window). Don't move it to a shared cache without thinking about whether different users' filter results could collide on the same URL.🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Tests precede implementation, naming is intent-revealing, methods stay under 20 lines, and the Svelte 5 idioms are clean. Real TDD evidence across all three layers.
What I checked
DocumentServiceTest(4 new),DocumentControllerTest(6 new),DocumentDensityIntegrationTest(7 new),timeline.spec.ts(44),TimelineDensityFilter.svelte.spec.ts(35),useTimelineDrag.svelte.test.ts(13),page.svelte.spec.ts(5 new). The test-first signal is unmistakable — backend behavior is described before the implementation reads it.getDensityis 8 lines and delegates to three named private helpers (resolveFtsIds,loadFilteredDates,aggregateByMonth). Each helper does one thing. This is the pattern.MonthBucket,DensityFilters,DocumentDensityResult,tickIndicesFor,aggregateToYears,selectionBoundaryFrom/To— every name reads as the question it answers. NoHelper,Manager,Util.$derivedused everywhere computed values are needed;$derived.byfor multi-step ones (monthBuckets,dragWindowLeftPct,dragLiveMessage). No$state + $effectderive antipatterns. ✓{#each filled as bucket, i (bucket.month)}. ✓$bindable()for the row element so the orchestrator can measure it forindexFromClientX. ✓+page.tsusesfetchand gracefully degrades toEMPTYon non-ok / rejection (timeline.ts:2400-2415).console.warnfor visibility. Good.Suggestions (non-blocking)
if ('zoomFrom' in event)discriminator (+page.svelte:2916) — keying on optional property presence works but a literal kind tag ({kind: 'select' | 'rangeZoom', ...}) would be self-documenting and survive future refactors. Not worth changing now; flag it if you ever add a third selection variant.{ from: '', to: '' }clear sentinel (TimelineDensityFilter.svelte:921) — empty string is falsy so URL building drops it, which is consistent with the existing convention. Considernullif you ever need to distinguish "explicitly cleared" from "never set". Today they collapse to the same URL, which is fine.TimelineDensityFilter.svelteat 204 lines — over the 60-line splitting signal, but each chunk maps to a distinct concern (props, derivations, selection helpers, drag wiring, render). It's a true orchestrator withTimelineBars/TimelineYAxis/TimelineXAxis/TimelineControlsalready extracted. Keeping it intact reads better than slicing further.The drag rune class (
useTimelineDrag.svelte.ts) is exemplary — pure state machine with injected dependencies, document listeners attached onpointerDownand torn down viacleanup. The unit test coverage of the state machine in isolation is exactly the right shape.🤖 Generated with Claude Code
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved (LGTM)
Pure feature change — no
docker-compose.yml, no Caddyfile, no CI workflow, no env vars, no new image, no new port, no new volume. The infra surface is unchanged.What I checked
docker-compose.yml,docker-compose.ci.yml,Caddyfile,.gitea/workflows/*,runner-config.yaml: all clean.private, max-age=300.privateis correct: per-user filter combinations could collide on the same URL key (differentsenderId/tagselections per user are personal browse state). Don't ever flip this topublicin Caddy without thinking it through.max-age=300is a reasonable browser-tab refresh window.console.warnonly on the degraded path.One thing to wire up later
The Javadoc on
DocumentService.getDensitysays "revisit at >50k rows" with #481 tracking the SQL aggregation rewrite. To make that trigger fire on data, not on someone remembering it, ping me when the Grafana board is set up — adding adocuments.density.duration_secondshistogram + a count gauge for the documents table is ~30 lines of Micrometer wiring. Not a blocker for this PR, but I'd rather we know before the p95 climbs than after.Monthly cost impact: zero. The endpoint adds no infra, fits in current CX32 headroom, and the 5-minute private cache absorbs the repeated browse loads on the same view.
🤖 Generated with Claude Code
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Functional requirements are well-specified and traceable to issue #385. Acceptance is testable across all three layers. The two NFR gaps (keyboard accessibility, E2E coverage) are explicitly tracked in #479 and #480, which is the right answer — but those follow-ups must actually ship before the next release closes, not drift.
Requirements coverage assessment
Functional requirements (covered):
/documentsq,senderId,receiverId,tag,tagQ,status,tagOpchange (intentionally NOT onfrom/to— the chart is the surface for picking those, well-justified in service Javadoc and test pinned)lgbreakpoint (1024 px) and in?view=calendarNFRs verified:
Cache-Control: privateon the endpoint; in-memory aggregation acceptable below 50k docs (#481 tracks the SQL rewrite trigger)de/en/esall updated, with proper singular/plural forms in ARIA labels(hover: hover)to prevent touch hover-stickNFRs deferred (must not be forgotten):
<button>s with focus rings, but a keyboard user cannot pick a range. Tracked in #479 → WCAG 2.1.1 (Keyboard) compliance. Priority should be P1: this blocks an actual user goal for assistive-tech users on the desktop archive (the senior researcher persona explicitly uses keyboard navigation).Open questions
?view=calendarswitches back to list view, does the density refetch trigger? CSR load function returnsSKIP(density null), so the chart should mount and refetch whenviewchanges — but I don't see a test pinning the back-and-forth transition.+page.ts?density_returns401_whenUnauthenticatedproves the endpoint requires auth, but the frontend graceful-degradationEMPTYwould silently swallow a 401 and render an empty chart. If the rest of the page is also auth-gated, this is fine; if there's any way to land on/documentsunauthenticated, the chart would show as empty without explanation.Definition-of-Ready check on the follow-ups
#479 — has acceptance criteria for keyboard semantics? If not, draft them now: which keys move focus between bars (
←/→), how is range selection triggered (Shift+arrow? Space-drag?), how is zoom invoked (Enter on year bar? a separate "Zoom" button when range is selected?). The drag UX has implicit semantics that need explicit keyboard mappings before implementation starts.#480 — should specify which scenarios from this PR's manual test plan migrate into Playwright. Recommend: drag across years, single click filter, reset-zoom preserves filter, mobile-hide via viewport, axe-clean in light + dark mode (Leonie's standard).
🤖 Generated with Claude Code
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No new attack surface. Authentication is enforced and pinned by test, parameters are type-safe, no raw user input touches a query string or a log statement, and the cache header is correctly scoped private.
What I audited
DocumentControllerTest.density_returns401_whenUnauthenticatedproves the default Spring Security.anyRequest().authenticated()rule covers the new endpoint. ✓GET, no@RequirePermissionneeded (per project convention, onlyPOST/PUT/PATCH/DELETErequire the annotation). The test pins200for@WithMockUser(any authenticated user can read). Consistent with the rest of the document read endpoints. ✓senderId/receiverIdtyped asUUID— Spring rejects malformed input with 400 before it reaches the service.statustyped asDocumentStatusenum — same rejection path.tagOpaccepted asStringthen mapped to enum ("OR".equalsIgnoreCase→TagOperator.OR, anything else →TagOperator.AND). No injection vector — the enum value is what reaches the spec builder.q,tagQ,tag[]are forwarded toSpecificationvia named parameters and JPQL — no string concatenation. ✓console.warnincludes status code or caught error object, no user input. ✓findRankedIdsByFts(text)(existing parameterized query) and the existingbuildSearchSpec(criteria API). Zero SQL/JPQL string concatenation in the new code. ✓Cache-Control: private, max-age=300.privateis critical here: filter combinations are personal browse state, and a shared cache (CDN, intermediate proxy) keying on the URL alone would serve user A's filtered density to user B. Pinned by test (density_emitsPrivateCacheControlHeader). Don't flip this topublic.Notes (not findings)
api.tsshows"*/*"for the response body even though the controller hasproduces=APPLICATION_JSON_VALUE. The testdensity_declaresApplicationJsonContentTypeproves the runtime correctly rejectsAccept: application/xmlwith 4xx, so this is a SpringDoc emit cosmetic, not a security issue. Worth filing if you care about the generated spec being precise — flagging Sara.fetchDensityswallows non-ok withconsole.warnand returns empty buckets. If the page is otherwise unauthenticated-accessible, this would silently render an empty chart instead of redirecting. Probably fine since the rest of/documentsis auth-gated, but worth confirming.🤖 Generated with Claude Code
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Test pyramid coverage at every layer except the top. Unit, component, integration, and controller tests are thorough and well-named. The E2E gap is acknowledged via #480, which is the right call — but it must close before this feature is "done" by my Definition of Done.
Pyramid coverage
DocumentServiceTest+4 (getDensityempty / groups / null-date filter / FTS short-circuit)timeline.spec.ts44 /useTimelineDrag.svelte.test.ts13TimelineDensityFilter.svelte.spec.ts35 (visibility, axes, bars, selection, year fallback, zoom, touch targets, a11y, aria-live, drag-to-select-range, listener cleanup)DocumentControllerTest+6 (401, 200, content-type, cache header, sender+tag forwarding, status+q forwarding)DocumentDensityIntegrationTest7 against real Postgres (@Import(PostgresContainerConfig.class),@Transactionalrollback)page.svelte.spec.ts+5 (visibility, click navigation, zoom-reset URL clearing)What's done well
@WithMockUseron the protected endpoint tests, plus an explicit unauthenticated test pinning 401. Both auth states covered, not just the happy path.PostgresContainerConfig). No H2 substitution.getByTestId/aria-label/ class assertions that survive refactoring. No$stateintrospection.h-11 min-w-[44px]for touch targets,focus-visible:ring-*for keyboard focus,@media (hover: hover)for touch-device hover-stick prevention, singular/plural ARIA forms — these are tested as code, not as a checklist.Gaps (must close before release)
?view=calendarboth hide the widget AND skip the density fetch.axe-core/playwrightrun on/documentswith the timeline mounted, light AND dark mode. Dark mode contrast must be tested independently — the comment on--timeline-bar-idlein dark mode claims 3.33:1; light mode acknowledges decorative-only contrast. Verify with a tool, both modes.Smells
*/*in generatedapi.tsdespiteproduces=APPLICATION_JSON_VALUEon the controller. Runtime is correct (density_declaresApplicationJsonContentTypetest pins XML rejection), but SpringDoc isn't emittingapplication/jsonto the spec. If you regenerate after every backend change as policy, the type may drift. Worth a separate check — possibly a SpringDoc config tweak.fetchDensitygraceful-degrade swallows 401 — returns empty bucket withconsole.warn. If the entire page is auth-gated this is fine, but if a future PR makes any path to/documentsreachable unauthenticated, the chart would silently render empty. A test for the 401 path returningEMPTYand emitting the warn would pin this.Recommend before merge
Add at least one Playwright smoke test in this PR (one drag, one reset-zoom, one mobile-hide assertion) so the most critical journeys aren't entirely deferred to #480. Five minutes of
axe-core/playwrightoutput on/documentsin light + dark mode would close the visual a11y gap too.🤖 Generated with Claude Code
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Visually polished, brand-token-correct, and unusually accessibility-aware for a chart component — touch targets, focus rings, reduced motion, hover gating, singular/plural ARIA, and an aria-live region for drag are all here in code, pinned by tests. But two concerns block this from being a "ship it" rather than "ship it with follow-up": keyboard inaccessibility for range selection, and dark-mode contrast for the selected/preview state.
Brand & visual
--timeline-bar-idle,--timeline-bar-outside,--palette-mint,border-line,bg-surface). No raw hex in the components.rounded-sm border border-line bg-surface shadow-smwithpx-3 pt-3 pb-2.font-sans text-xs leading-nonefor axes — meets the 12 px floor (test pinsnot.toMatch(/text-\[10px\]/)). Good.@media (prefers-reduced-motion: reduce)strips the100ms easecolor transition. WCAG 2.3.3.Accessibility — strengths
h-11 min-w-[44px]on reset-zoom,h-11 w-11on clear. WCAG 2.5.8. Test-pinned.focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2on every interactive element (bars, reset, clear). WCAG 2.4.7. Test-pinned and consistent.<button>s witharia-pressed,aria-label, and a localized template that distinguishes singular ("1 Dokument") from plural ("5 Dokumente"). Three locales. The aria-label test explicitly forbids the rawYYYY-MMmachine string from leaking. Excellent.role="group"+aria-labelon the widget container. WCAG 4.1.2.aria-live="polite"region during drag with localized "Range X to Y selected" — assistive tech actually narrates the in-progress range. Rare and well-done.@media (hover: hover)— prevents touch-tap hover-stick. WCAG 2.5.7 spirit.aria-hidden="true"on the X/Y axes (decorative — bararia-labelcarries the count).Concerns
🟡 Critical: Keyboard inaccessibility of range zoom (WCAG 2.1.1, A)
Tracked in #479. Repeating it here because severity matters: the drag-to-zoom gesture is the primary high-information interaction in this widget, and there is currently no keyboard equivalent. A keyboard or switch user can
Tabto a bar andEnterto filter that single month, but cannot select a range.This is a P1 in my book — Familienarchiv's senior persona (60+, tablet/laptop, often using keyboard or voice control rather than precise pointing) is the exact user who can't drag with a mouse. WCAG 2.1.1 is Level A — the floor, not a stretch goal. #479 must close in the next release, with explicit keyboard mappings spec'd before implementation:
🟡 Major: Dark-mode contrast for the selected/preview state
--timeline-bar-idlein dark mode is#3a6e8c, claimed at 3.33:1 vs--c-surface (#011526). That's correct math and clears WCAG 1.4.11 for non-text UI. ✓The selected and in-drag-preview state, however, falls back to
var(--palette-mint, #a1dcd8). Against dark--c-surface (#011526),#a1dcd8is ~10:1 — that's fine. But against the adjacent idle bar#3a6e8cthe contrast between selected and idle is only ~2.4:1, which means a low-vision user can struggle to tell which bars are in the selection. The visible cue should be more than just the mint-vs-mid-tone-blue difference.Fix recommendation (light + dark, both modes):
border-topon the selected bar<span>, ORoutline: 2px solid var(--palette-mint)on the selected bar's button (re-using the focus token).Color alone is not the only cue today (
aria-pressed="true"plus opacity 0.7 on in-drag-preview), but the visual signal for sighted-but-low-vision users is fragile. Tracked under #480 if you want to roll it into the axe pass; otherwise file as a small follow-up.🟢 Minor
rgba(161, 220, 216, 0.35)over white ≈ #DEF3F1, ~1.13:1) is in the WCAG 1.4.11 carve-out for "decorative content". Comment inlayout.css:3063correctly acknowledges this. The selected state (mint at 1.52:1 over white) is also in that carve-out — same fix as above would solve it.aria-pressedon the bars is correct usage (toggle state). Some screen readers handle this differently across the months when the user moves between selected/unselected — usually fine, but worth observing in the dark-mode + screen-reader pass for #480.What I'd ship right now if I had pen tablet open
That solves the dark-mode adjacency contrast and reinforces the selection visually for everyone.
🤖 Generated with Claude Code