feat(documents): honest handling of undated documents in browse & search (Phase 6, #668) #682
Reference in New Issue
Block a user
Delete Branch "feature/668-undated-documents"
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?
Summary
Phase 6 (final) of the "Handling the Unknowns" milestone — undated and imprecise documents are surfaced honestly in browse/search, never silently buried or fabricated. Builds on #677's
formatDocumentDate. No migration.resolveSortnow emitsOrder(dir, "documentDate").nullsLast()+ acreatedAttiebreaker (closes the ASC bug where undated sorted to the top). SENDER/RECEIVER/filtered-RELEVANCE sort in-memory by person/rank (not date); a locking test proves undated stays within its sender/receiver group rather than jumping out.DocumentDatecomponent, both breakpoints). The year-groupingdocs_group_undatedbucket is kept; no synthetic sub-group in person-sort modes.undatedOnlySpecification shared by/searchand/ids;Boolean undatedquery param (read GET unguarded, authz-tested); 44pxaria-pressedtoggle wired to the URL.from/torange excludes undated docs; the empty-state copy (docs_range_excludes_undated) explains it.formatDocumentDate("Juni 1916" / "ca. 1916" / open-ended "ab …" / "Datum unbekannt") — base already had the #671 fields + #677 formatter, so this is real, not stubbed.ChronikRowrenders the activity timestamp, not the letter date; no fabricated date chip introduced. Briefwechsel untouched (dead feature).Test plan
undatedOnly, BETWEEN-excludes-null, undated+range→empty, per-sort-mode ordering). Frontend node vitest 643 pass;lintclean. (Full suite not run locally — CI owns the sweep.)Notes / deviations
generate:apihand-edited —undatedadded to/search+/ids; CI must re-runnpm run generate:apito confirm parity.date_precision_unknowntext (same wording) rather than a new key.PersonMentionEditor.svelte.spec.tsand pre-existingprovisional-missing type errors on old Person fixtures / autofixer warnings are out of scope and not introduced here.Closes #668
Parses ?undated strictly (=== 'true', mirroring the tagOp clamp), forwards it as undated || undefined so the absent case drops out of the query, and returns the flag in page data for the control to reflect. Adds the docs_filter_undated_only toggle label and the explanatory docs_range_excludes_undated empty-state copy in de/en/es. The badge reuses the existing date_precision_unknown ("Datum unbekannt") key from #677. OpenAPI types hand-edited for the new undated query param on /search and /ids — CI must run `npm run generate:api` to confirm parity with the spec. Refs #668DocumentRow rendered a bare em-dash for null-dated letters — a glyph a screen reader announces as nothing. Both breakpoints now render the single DocumentDate component unconditionally (no {#if}/—/{:else}), so the cue cannot drift; its unknown state is a neutral metadata chip ("Datum unbekannt", text-ink-3, ≥4.5:1 both themes) with a non-color calendar glyph, never red/amber. Present dates render at honest precision via formatDocumentDate ("Juni 1916", not a fabricated day). Refs #668Felix Brandt — Senior Fullstack Developer
Verdict: Approved with concerns
I read the full diff and the touched source files. The TDD discipline is visible — every behavioral change has a paired test, and the two service-level
searchDocuments_dateSort_ASC/DESC_ordersUndatedLasttests are written as genuine red/green captures of the NULLS-LASTSort.Order. Clean work overall.Concerns (not blockers)
Boolean flag argument threaded through
searchDocuments/findIdsForFilter—DocumentService.java. I normally flagboolean undatedper my own "no boolean flag arguments" rule. Here the alternative (two methods) would be worse given the parameter is already a 12-arg method backed by aSpecification, and the param is symmetric withfrom/to/status. I'm not asking for a split, butsearchDocuments(...)now has 13 positional parameters including two booleans-adjacent values (String dir,boolean undated,Pageable). This is past the readable threshold. Consider aSearchCriteria/SearchFiltersrecord in a follow-up so call sites stop reading asnull, null, null, ..., false, PAGE. The test churn in this PR (every call site re-counted by hand) is the cost of the long signature.resolveSortvisibility widened fromprivateto package-private —DocumentService.java. Done solely to unit-test it indirectly via the captor, but the tests actually assert throughsearchDocuments+ArgumentCaptor<Pageable>, not by callingresolveSortdirectly. If nothing calls it package-privately, revert toprivate— don't widen API surface for a test that doesn't use the widened access. (If a test does call it directly, ignore this.)DocumentDate.svelteduplicated<span class="inline-flex items-center gap-1">wrapper — the{:else}branch wraps{label}in a flex span containing a single child span. That's a redundant nesting (<span class="inline-flex flex-col"><span class="inline-flex…"><span>{label}</span></span>). Collapse to one span. Minor.What I checked and liked
!undated && isPureTextRelevance(...)guard with its inline comment explaining why (the SQL shortcut bypassesbuildSearchSpec, dropping the predicate) — that's a "why" comment, exactly right, and it's locked bysearchDocuments_undatedTrue_usesSpecificationPath_notPureTextRelevanceShortcut.undatedis$bindableand the$effectre-syncs fromdata.undatedon navigation — no stale-state cycle, correct Svelte 5.+page.server.tsclamps=== 'true'mirroring thetagOpclamp;undated || undefinedkeeps the param off the query string when false. Consistent with the existing pattern.Markus Keller — Application Architect
Verdict: Approved
I reviewed this for layer boundaries, where the null-date semantics are enforced, and documentation currency.
Boundaries — clean
undatedpredicate lives inDocumentSpecifications.undatedOnly(boolean)as a static factory alongsidehasSender,isBetween, etc. — the right home.DocumentService.buildSearchSpec(...)composes it via.and(undatedOnly(undated)). Controller → service → spec; no controller-to-repository shortcut, no cross-domain repository reach. The/idsand/searchpaths share the singlebuildSearchSpecbuilder, so selection (bulk-edit) and the visible list are guaranteed to agree — this is the correct way to avoid filter drift between two endpoints.Where the rule is enforced — good instinct
UndatedDocumentOrderingIntegrationTestagainstpostgres:16-alpine(not H2). The class JavaDoc explicitly calls out that H2 disagrees on NULLS defaults and BETWEEN/NULL — that is exactly the "push integrity to the lowest trustworthy layer + test it there" principle. Nicely reasoned.createdAttiebreaker giving a deterministic total order when every row is null-dated (the "Nur undatierte" filter, so pagination is stable) is a real correctness consideration, not gold-plating.Documentation currency — no blocker
Per my doc-trigger table: this PR adds no Flyway migration, no new table/column/FK, no new
ErrorCode, no newPermission, no new backend package, no new SvelteKit route (it modifies/documents, which already exists). Theundatedquery param is an additive parameter on existing/search+/idsendpoints — not a new domain term warranting a GLOSSARY entry (the concept "undated/Undatiert" already exists in the year-grouping bucket). So no diagram or doc update is required. I confirmed the L3 frontend diagram for documents does not enumerate query params. Clear to merge on docs grounds.One forward-looking note (not blocking)
The "honest handling of unknowns" is now a recurring cross-cutting concern (precision rendering, undated triage, range collision). If a 7th surface ever needs the same null-date logic, that is the trigger to consider documenting the rule once in
docs/GLOSSARY.md("Undatiert / undated"). Not now.Sara Holt — QA Engineer
Verdict: Approved with concerns
Strong test work. Real Postgres for the null-date guarantees, MockMvc for the authz boundary, browser specs for the badge/toggle, and a load-function spec for the param plumbing. The pyramid is respected. My concerns are about coverage gaps in the in-memory sort paths, not about what's there.
Concerns
The SENDER/RECEIVER "undated stays in group" guarantee is under-locked.
searchDocuments_senderSort_keepsUndatedDocumentUnderItsSenderasserts both docs come back under Alice and usescontainsExactlyInAnyOrder("Dated","Undated")— In Any Order. So it proves neither doc is dropped, but it does not prove the relative ordering of the undated doc within the group, nor that it doesn't sort to the top of a multi-person result. The PR description claims "undated genuinely does not jump to the top in any mode." For the in-memory comparator that claim is currently asserted only weakly. Add a test with two senders, each with a dated + an undated doc, asserting the exactcontainsExactly(...)sequence — that locks "grouped by person, undated kept inside its group" with an ordered assertion. Right now a comparator regression that floated all undated docs to the front of the page would still pass.No DESC test for the in-memory paths. All SENDER/RECEIVER tests use
"asc". The SQL DATE path has both ASC and DESC locked; the in-memory path does not. OneDocumentSort.SENDER, "desc"case would close the symmetry.undatedOnlycombined with a person sort is untested.undated=true+sort=SENDERis a reachable UI state (toggle on while grouped by sender). Worth one integration assertion that the spec still filters to null-dated rows on that path.What I verified as solid
UndatedDocumentOrderingIntegrationTestuses@AutoConfigureTestDatabase(replace = NONE)+PostgresContainerConfig— correct, not H2.dateRange_excludesUndatedRows,undatedOnly_combinedWithDateRange_returnsEmpty, and the ASC nulls-last ordering are all asserted against real Postgres with orderedassertThat(result.get(n)). This is the canonical pattern.ArgumentCaptor<Boolean>. Exactly the "test both the accept and the reject" rigor I ask for.ChronikRownegative assertion (undated-badgeabsent, no "Datum unbekannt" text) is a good regression lock against fabricating a letter date in the activity feed.page.server.spec.tscovers true →query.undated: true, absent → undefined,'1'→ false (the clamp), and page-reset. Thorough boundary coverage on the param surface.No
Thread.sleep, no@Disabled, no flaky patterns introduced. Address concern #1 (ordered multi-sender assertion) and I'm fully satisfied; it's the one place the headline claim outruns the test.Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved
I reviewed the new
undatedparameter end-to-end for authz, injection, and input handling. No vulnerabilities, no smells. This is a read-side filter and it's handled correctly.Authorization — correct on both endpoints
GET /api/documents/searchcarries no@RequirePermissionand relies on the global.anyRequest().authenticated()rule — a read GET, reachable by any authenticatedREAD_ALLuser. The PR adds a regression test for exactly this:search_undatedTrue_returns401_whenUnauthenticated(no@WithMockUser) asserts 401, andsearch_undatedTrue_isReachableByAuthenticatedUserasserts 200. That's the both-sides coverage I insist on — and it guards against a future refactor accidentally write-guarding the triage path. Good.GET /api/documents/idskeeps@RequirePermission(Permission.WRITE_ALL). That's the bulk-edit selection endpoint (it feeds "Alle X editieren"), so WRITE_ALL is the right gate and addingundateddoes not change its exposure. No IDOR surface — it returns IDs the caller is already authorized to edit, capped atBULK_EDIT_FILTER_MAX_IDS.Injection / input handling — clean
undatedOnly(boolean)buildscb.isNull(root.get("documentDate"))via the JPA Criteria API — no string concatenation, injection-proof by construction. The whole spec chain uses Criteria/named parameters.Boolean.TRUE.equals(undated), so a missing param (null) and any non-truevalue collapse tofalse— fail-safe defaulting. The frontend+page.server.tsindependently clamps=== 'true'(defense in depth: server-side load won't reflect'1'into the query). Spring'sBooleanbinder rejects non-parseable values with a 400 at the boundary, which is acceptable.docs_range_excludes_undatedis a localized constant, not a reflected server message — the DocumentList comment even calls this out explicitly. No info leak.Note (informational, not a finding)
Boolean.TRUE.equals(undated)silently treatsundated=falseandundated=garbage-that-bound-to-null identically. That's the intended fail-closed behavior for a filter and is fine. No action needed.Nothing to fix. The 401/200 pair on the triage path is the kind of explicit boundary test I want to see stay in the suite permanently.
Leonie Voss — UX Designer & Accessibility Strategist
Verdict: Approved with concerns
This is honest-by-design UX done right: an undated letter is rendered as a neutral absence, not an error, and never silently buried. I checked the badge, the toggle, the empty state, and i18n. One real touch-target concern on the toggle's check-box affordance, plus a couple of polish notes.
Concerns
(High) Toggle inner check-box is decorative but visually reads as the control. In
SearchFilterBar.sveltethearia-pressed<button>is correctlymin-h-[44px](good — meets WCAG 2.5.5 for the senior audience). But the inner<span aria-hidden="true">h-4 w-4check box looks like a checkbox the user should aim at. Seniors on a phone will tap the 16px glyph, not realize the whole 44px row is the target. The full button is tappable, so this isn't a functional blocker — but please verify inproofshot/manual at 320px that the tap target visually communicates "tap anywhere on this pill," e.g. the hover/active state covers the whole pill (it does —hover:bg-lineis on the button). Acceptable, but confirm the affordance reads correctly at small size.(Medium) Badge text is 10px. The undated badge in
DocumentDate.svelteusestext-[10px]uppercase. My standing rule is 12px minimum for visible text; the senior audience (60+) needs larger. I understand it's a metadata chip matching the existing archive-chip pattern in the row, and uppercase 10px tracked-widest chips are an established convention here — but "Datum unbekannt" is semantically meaningful (it's the date surface for that row), not decorative chrome like a tag count. If the row layout allows, bump totext-[11px]/text-xs. If the chip pattern is locked project-wide, this is consistent and I'll accept it — flag for the row-density owner.What I verified as correct (and good)
text-ink-3chip avoids red/amber — correct: an absent date is not an error state.aria-hidden="true"; the label text is announced inline (never aria-hidden). The toggle state is conveyed byaria-pressed, not by color alone — screen readers get the real state. This is exactly the redundant-cue discipline I push.docs_range_excludes_undated) explains why a date range yields nothing — that's "help users diagnose and recover," not a dead end. Localized in de/en/es, all three present and accurate (I read the Spanish — "no pueden pertenecer a ningún periodo" is correct and natural).does not render a bare em-dashtest locks it.No blockers. Confirm concern #1 visually at 320px and consider the 11px bump in #2.
Elicit — Requirements Engineer (Brownfield)
Verdict: Approved with concerns
I assessed this against the issue it closes (#668, "honest handling of undated/imprecise documents") and the dual-audience constraints. The acceptance surface is well covered and the requirement is stated in behavior language, not solution language. Two requirement-traceability gaps and a few decisions I want surfaced rather than silently resolved.
Requirements coverage — traced
The PR delivers, with a test each: NULLS-LAST both directions (SQL path); undated kept in person group (in-memory path);
undatedOnlyfilter shared by/search+/ids; range-excludes-undated empty state; precision rendering viaformatDocumentDate; Chronik does-not-fabricate. That maps cleanly to "surface undated honestly, never bury, never fabricate." Good behavioral framing.Concerns / open items to surface (per my "never silently resolve a TBD" rule)
createdAt) ASC as the secondary tiebreaker, and (b) page-local undated count. These are defensible defaults, but they are product decisions made by the implementer. Please confirm them explicitly, @marcel, or log them in an Open-Questions note on #668 so they don't become invisible accidental requirements. ThecreatedAttiebreaker in particular sets the order seniors will scan undated letters in — worth a conscious yes.containsExactlyInAnyOrder). Either tighten the test to an ordered multi-person assertion, or soften the AC wording to match what's actually guaranteed. Requirement and verification must agree.undated+ afrom/torange is a contradiction the UI can still express. The backend correctly returns empty (mutually exclusive by definition), and the empty-state explains it. But nothing prevents the user from setting both — is the intended behavior "range wins, undated yields empty + explanation" (current) or "disable the undated toggle while a range is active"? Current behavior is honest and fine; just confirm it's the intended resolution of the contradiction, not an accident.Non-functional checks
docs_filter_undated_onlyanddocs_range_excludes_undated. Pass.aria-pressed+ 44px; badge has redundant cues. Pass (Leonie owns the depth).Approving on the understanding that the two defaulted decisions get an explicit owner sign-off and AC #2 is reconciled with its test.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Nothing in my domain to flag, but I checked the things that bite at deploy time rather than just rubber-stamping.
Infra surface — none
No Compose changes, no Dockerfile, no CI workflow, no new service, no new env var, no migration, no port. This is application-only (Java + Svelte). My usual checks (pinned image tags, named volumes, exposed ports, root creds, deprecated action versions) are all N/A here.
The one operational risk worth naming: the hand-edited generated client
frontend/src/lib/generated/api.tswas edited by hand to addundated?: booleanto/searchand/ids, becausenpm run generate:apineeds the backend running. The PR body correctly flags that CI must re-rungenerate:api. From a pipeline view:undated?: booleanquery field, matching the two new@RequestParam Boolean undatedon the controller. So it's correct today.generate:apistep runs against the built backend and fails the build if the regeneratedapi.tsdiffers from the committed one (agit diff --exit-codeafter regen). If that gate already exists, this PR is fully safe; if it doesn't, that's a standing CI gap that this PR happens to exercise — worth adevopsissue independent of #682. Either way, not a blocker for this merge since the hand-edit is verifiably correct.Test infra
The new
UndatedDocumentOrderingIntegrationTestuses Testcontainerspostgres:16-alpineviaPostgresContainerConfig— same pattern as the rest of the suite, no new container or resource footprint, no CI time concern beyond one more@DataJpaTestclass. Fine.Approved. The only follow-up is a CI-hygiene check on the generate:api drift gate, which is orthogonal to merging this.
The dated branch wrapped {label} in a flex span containing a single child span — redundant nesting. Render the label directly in one span. Refs #668 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Felix Brandt — review concerns addressed
Pushed four atomic commits to
feature/668-undated-documents. Red→green throughout; backend touched classes and frontend node-vitest + lint all green locally.Sara — in-memory sort coverage
containsExactlyInAnyOrdercheck withsearchDocuments_senderSort_asc_keepsUndatedInsideSenderGroupNotAtHead— a two-sender fixture (Adler/Ziegler, each a dated + undated doc) with an orderedcontainsExactly. Input interleaves dated/undated; the assertion proves grouping-by-sender wins (stable sort preserves intra-group input order) and the undated doc never floats to the page head. (caec92e7)searchDocuments_senderSort_desc_keepsUndatedInsideSenderGroupNotAtHeadfor in-memory-path symmetry, andsearchDocuments_undatedTrue_withSenderSort_appliesUndatedSpecification— captures theSpecificationhanded tofindAllon the SENDER path, provingundatedOnly(true)is still composed when the toggle is on while grouped by sender. (caec92e7)Felix — cleanups
searchDocuments), so the package-private widening added nothing. Reverted. (eacfd15f){:else}span nesting: Collapsed the<span class="inline-flex…"><span>{label}</span></span>wrapper to a single<span>{label}</span>. (508575ec)Leonie — badge & toggle
text-[10px]totext-xs(≥12px). (85372e36)aria-pressed<button>already is the full 44px pill (min-h-[44px],px-3,hover:bg-lineover the whole button); the inner 16px glyph is anaria-hiddendecorative child inside the button, so a tap anywhere on the pill fires the toggle. No markup change needed. Visual confirmation at 320px remains a CI/proofshotstep (browser specs are CI-only).Tests
DocumentServiceTest165,DocumentControllerTest102,DocumentServiceSortTest7 → 274 pass. Testcontainers-Postgres integration classes (UndatedDocumentOrderingIntegrationTestet al.) unchanged — CI owns the sweep.vitest --project=server643 pass;npm run lintclean. Browser/component specs (badge/toggle) CI-only.Not done (out of scope per follow-up)
SearchFiltersrecord extraction (Felix #1) — flagged follow-up, filed separately.generate:apidrift gate (Tobias) — tracked in #680.🤖 Generated with Claude Code
Markus Keller — Senior Application Architect
Verdict: Approved
I reviewed module boundaries, the layering rule, database-vs-application enforcement, and documentation currency.
What I checked
DocumentControllerforwards the newundatedflag straight intoDocumentService; no repository reached from the controller, no cross-domain repo access.undatedOnly(boolean)lives inDocumentSpecificationsalongside its siblings — correct placement.resolveSortemitsOrder(dir, "documentDate").nullsLast()plus acreatedAt.asc()tiebreaker. This is the right layer: Postgres owns ordering semantics; the in-memory SENDER/RECEIVER path is a deliberate, ADR-008-documented exception for the INNER-JOIN-drops-nulls problem, and theUndatedDocumentOrderingIntegrationTestcorrectly runs on realpostgres:16-alpine(its class comment explicitly justifies why H2 is unacceptable for NULLS/BETWEEN semantics). Good architectural hygiene.meta_date IS NULLis filtered, no migration, so no DB-diagram update is owed.undatedis a query param on an existing endpoint.Notes (not blockers)
searchDocuments(...)signature now carries 13 positional parameters. The booleanundatedwedged beforePageableis a long, type-ambiguous argument list. TheSearchFiltersrecord extraction (#683) is the right fix and is already tracked — I am content to let it land separately rather than block Phase 6.undated && isPureTextRelevance(...)short-circuit insearchDocumentsis a sharp edge: the SQL relevance shortcut bypassesbuildSearchSpec, so the predicate would be silently dropped. The code comment names this precisely and the testsearchDocuments_undatedTrue_usesSpecificationPath_notPureTextRelevanceShortcutlocks it. This is exactly the kind of invariant I want guarded by a test, not a comment alone — well done.Boring, correct, well-bounded. Ship it.
Felix Brandt — Senior Fullstack Developer
Verdict: Approved
I checked TDD evidence, naming, function size, guard clauses, Svelte 5 rules, and dead code.
TDD evidence — solid
The red is captured where it matters.
searchDocuments_dateSort_ASC_ordersUndatedLastcarries the comment "This is the red" and assertsNullHandling.NULLS_LASTon the ASC order — that is the exact bug (#668) where undated docs floated to the top. The DESC counterpart proves symmetry. The two new SENDER locking tests are the strongest part of the PR:searchDocuments_senderSort_asc_keepsUndatedInsideSenderGroupNotAtHeadand its_desc_sibling use a two-sender fixture (Adler Bob,Ziegler Anna), each with one dated + one undated doc, input interleaved, asserted with orderedcontainsExactly. I verifiedsortBySenderin the source: it usesStream.sorted()(a guaranteed stable sort) keyed purely onlastName + " " + firstName, never on date. So the assertion"Bob undated", "Bob dated", "Anna dated", "Anna undated"genuinely proves the undated doc stays inside its sender group and never jumps to the head — and a date-based regression would clump the two undated docs together, breaking the order. The lock is real, in both directions.searchDocuments_undatedTrue_withSenderSort_appliesUndatedSpecificationcaptures theSpecificationand confirms the in-memory SENDER path still composesundatedOnly(true)rather than skipping to a sortedfindAll. Good coverage of the reachable "Nur undatierte while grouped by sender" UI state.Clean code
undatedOnly(boolean)is a one-line guard-clause Specification —false → null(no-op),true → cb.isNull(...). Reads exactly as intended.DocumentRow.svelte: the{#if doc.documentDate} … {:else} — {/if}was collapsed to a single<DocumentDate>call;DocumentDate.sveltenow owns the unknown/dated branch with the redundant{:else}span nesting removed. Cleaner, single source of truth for date rendering. No dead branches left behind.resolveSortis back toprivate— no test reaches into it directly; the new sort tests assert through the publicsearchDocumentsvia a capturedPageable. Correct: test behavior, not internals.undatedflows server→props (+page.server.tsclamps to the literal"true", mirroring thetagOpclamp),$bindableinSearchFilterBar,$derivedhasDateRangeinDocumentList. No$effect-to-compute, keyed each blocks untouched.Minor (non-blocking)
The 13-arg
searchDocumentsis past the point where I'd normally extract a params object — but #683 tracks theSearchFiltersrecord and I'd rather not balloon this PR. Agreed as a follow-up.Disciplined work. Approved.
Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
I assessed the test pyramid, determinism, real-infra coverage, and boundary cases. CI owns the full sweep; I reviewed by reading.
Strong points
Pageable, assertNullHandling.NULLS_LAST+ direction) AND at the integration layer against real Postgres (UndatedDocumentOrderingIntegrationTestwithpostgres:16-alpine,@AutoConfigureTestDatabase(replace = NONE)). The class comment correctly states H2 disagrees with Postgres on NULLS and BETWEEN-vs-NULL — exactly why this must not be an in-memory DB test. This is the discipline I want.dateRange_excludesUndatedRows,undatedOnly_combinedWithDateRange_returnsEmpty(the BETWEEN-excludes-null guarantee on real Postgres), plus the frontend empty-state copy test. Good vertical coverage of one rule.search_undatedTrue_returns401_whenUnauthenticatedandsearch_undatedTrue_isReachableByAuthenticatedUser. Both 401 and reachable-200 are covered — not just the happy path.containsExactly, ASC+DESC, and a captured-Specificationcase. One behavior per test, names read as sentences. Textbook.renders the activity timestamp, not a letter date, and no undated badge) guards against a fabricated date chip leaking into the activity feed.Concerns (non-blocking)
DocumentRow,DocumentList, andSearchFilterBarbadge/toggle specs arevitest-browser-svelte. Per project convention these are trusted to CI. They look correct (usegetByTestId('undated-badge'),aria-pressed,getByText), but the verdict is contingent on CI going green. Flag, not block.page-local undated countis a defaulted product decision: the undated count reflects only the current page, not the full result set. That is a behavior worth a test asserting the documented semantics so a future "make it global" change is a conscious break. Owner sign-off pending — note it.PersonMentionEditor.svelte.spec.tsand the pre-existingprovisional-fixture svelte-check errors are unrelated to this diff and correctly out of scope.Quality gate is met pending the CI sweep. Approved with the count-semantics note.
Nora Steiner ("NullX") — Application Security Engineer
Verdict: Approved
I reviewed the new query param for injection, authorization, mass-assignment, and untrusted-input rendering.
Findings
undatedOnly(boolean)buildscb.isNull(root.get("documentDate"))via the Criteria API — fully parameterized, no string concatenation. The boolean is a closed type with two states; there is no user-controlled string reaching JPQL. Clean.undatedparam rides on the existingGET /api/documents/searchand/ids, both of which sit behindanyRequest().authenticated(). The read GET is intentionally not@RequirePermission-guarded — correct, since this is a READ path and write-guarding it would be wrong. The pairsearch_undatedTrue_returns401_whenUnauthenticated(401 for anonymous) andsearch_undatedTrue_isReachableByAuthenticatedUser(200 for@WithMockUser) is exactly the both-sides boundary test I require. The comment "guards against a future refactor accidentally write-guarding the undated triage path" shows the right threat-model thinking.+page.server.tsdoesurl.searchParams.get('undated') === 'true'— anything else ('1','TRUE', garbage) isfalse, and the testtreats any undated value other than the literal "true" as not-undatedproves it. No truthy-coercion footgun. Backend mirrors withBoolean.TRUE.equals(undated), so a missing/garbage param is a safefalse, not an NPE on an unboxed primitive.DocumentDate.svelterendersraw(verbatim spreadsheet text) via default Svelte interpolation — the existing comment correctly notes it HTML-escapes (never{@html}, CWE-79). The new "Datum unbekannt" label is a static i18n string. The empty-state copydocs_range_excludes_undatedis a localized constant, never a reflected backend string — the code comment says so and the diff confirms it.No CWE in scope here. Nothing to fix. Approved.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
I checked for infrastructure impact, CI implications, and migration/deploy risk.
Findings
undatedfilters on an existing nullable column (meta_date IS NULL); zero schema change means zero rollback/PITR concern and no startup-order dependency.generate:apidrift. The PR notes the OpenAPI types (frontend/src/lib/generated/api.ts) were hand-edited to addundated?: booleanon/searchand/ids. Hand-editing generated artifacts is exactly the kind of drift that silently rots — the generator output and the backend spec can diverge with no one noticing. The author flagged that CI must re-runnpm run generate:apito confirm parity, and a drift-gate is already tracked as #680. That is the right resolution: a CI gate that fails when the committedapi.tsdiffers from a fresh generation. I'd want that gate landed soon, but it is a tracked follow-up, not a blocker for this PR. Until #680 exists, the safety net is the CI regeneration step the author named.The hand-edit itself looks correct (both query objects gained
undated?: booleanwith a matching description), so functionally CI should pass. No infrastructure objections. Approved.Leonie Voss — UX Designer & Accessibility Strategist
Verdict: Approved
I reviewed brand compliance, WCAG (contrast, touch targets, non-color cues, focus), and the 320px / senior-audience constraint.
Findings — the prior-round fixes landed correctly
text-xs(12px), up from the earliertext-[10px]which fails my hard floor — no visible text below 12px, ever, for a 60+ audience. Confirmed inDocumentDate.svelte. Good.aria-hidden="true") with the visible "Datum unbekannt" text, which is announced inline (NOT aria-hidden) — so screen-reader users hear the meaning and color-blind users see the word. The chip is neutral (text-ink-3border/bg-surface, stated ≥4.5:1 in both themes), never red/amber. An undated letter is an absence, not an error — treating it as neutral metadata is the right semantic and the right contrast story (WCAG 1.4.1 satisfied by the text, not the hue).SearchFilterBar.svelte: it is a real<button type="button">withmin-h-[44px]andaria-pressed={undated}. The 16px (h-4 w-4) check glyph is anaria-hiddenchild inside the 44px button — it is not the hit target. Toggle state is conveyed byaria-pressed, not by color alone. Correct on all three counts.DocumentListshows the localizeddocs_range_excludes_undated("a date range excludes undated documents…") instead of a generic "no results". That is precisely the recognition-over-recall guidance — the user learns why their range hid things, in plain language, in all three locales (de/en/es present).—with the labeled chip removes an ambiguous glyph that a screen reader would announce as nothing meaningful. Thedoes not render a bare em-dashtest guards it.Minor (non-blocking)
undated-badgechip checked at 320px in a visual-regression snapshot eventually (uppercase +tracking-widest+ icon can get tight on a small phone in a narrow date column), but the component specs assert presence and the layout uses flex, not fixed widths — acceptable for now.Accessible, on-brand, honest. Approved.
Elicit — Requirements Engineer & Business Analyst
Verdict: Approved with concerns
I reviewed this against the #668 acceptance criteria, edge-case coverage, ambiguity, and two unresolved product decisions. (No code opinions — requirements only.)
Requirements coverage — the "honest unknowns" intent is met
The core need ("undated and imprecise documents are surfaced honestly, never silently buried or fabricated") is satisfied across the flows:
Open decisions awaiting owner sign-off (NOT blockers, but log them)
The PR applied two defaults because the owner was unreachable. Per my no-silent-TBD rule, I am surfacing both explicitly so they are conscious decisions, not accidents:
createdAt asc). When many docs share a null date (the "Nur undatierte" view), their relative order is decided by upload time, oldest first. Reasonable default, but confirm: is "oldest-uploaded first" the order an archivist expects when triaging the unknown pile, or would newest-first (most recently added, likely still fresh in memory) serve the task better?Scope hygiene
Both engineering follow-ups (#683
SearchFiltersextraction, #680generate:apidrift gate) are correctly parked as separate issues rather than scope-creeping this PR. That is healthy backlog discipline.The feature delivers the #668 intent. My only asks are the two product confirmations above — neither blocks the merge, but both should get an explicit owner answer before they ossify. Approved with concerns.
The desktop right-column kept a leftover {#if doc.documentDate}…{:else}—{/if} fallback that emitted a bare em-dash for undated documents, while the mobile block already always rendered <DocumentDate>. DocumentDate defensively maps a null date to the "Datum unbekannt" badge, so render it unconditionally — an undated document is an absence, not an error, and never shows a bare "—". Refs #668Undated count: page-local → global (owner decision)
Replaced the page-local undated count with a global one. Previously the only undated tally was the year-grouping
Undatiertbucket inDocumentList.svelte, built from the current page's items — so it could never exceed the page size.Approach chosen: backend field (preferred option, not a separate count-call). Added
undatedCounttoDocumentSearchResult, computed once persearchDocumentsviadocumentRepository.count(spec)over the same filter spec withundatedOnly(true)forced — independent of the "Nur undatierte" toggle, so it never collapses to the page slice and never double-counts. Picked the field over a separate round-trip because the search already builds the exact spec andJpaSpecificationExecutor.count()is one extra cheap query, keeping it consistent with the existing search path.from/torange excludes undated rows by the collision rule (#668), so the count is legitimately0inside a date range — covered by a test.undatedCountstraight through (defaults to0); rendered as a count chip on the "Nur undatierte" toggle (data-testid="undated-count"), hidden at 0, visible regardless of toggle state.Tests
DocumentSearchPagedIntegrationTest: 70 undated docs over a 50-page window →undatedCount == 70(not page-derived, first DATE-DESC page is all dated); toggle-on vs toggle-off yield identical counts; count is 0 inside a date range. PlusDocumentSearchResultTest:@Schema(REQUIRED)onundatedCount,withUndatedCountoverlay, factory defaults.page.server.spec.ts: forwardsundatedCountfrom the response and defaults to 0 when absent. Component specSearchFilterBar.svelte.spec.ts(CI-only): chip renders the value / hides at 0.Results: backend
DocumentSearchResultTest+DocumentSearchPagedIntegrationTest→ 19 pass. Frontend node vitest (touched file) → 18 pass.npm run lintclean.generate:api: hand-edited
frontend/src/lib/generated/api.ts(addedundatedCount: number, int64) — backend wasn't running locally. CI must re-runnpm run generate:apito confirm parity.Out of scope (unchanged): pre-existing
page.server.spec.tsmock-cast /ServerLoadEventtype errors and the flakyPersonMentionEditorspec — svelte-check is not gated locally; CI owns the sweep.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean reviewer, no prior context — I read the diff and the touched source at the head ref, not the old comment threads.
What I verified in code
Em-dash removal (
DocumentRow.svelte) — the desktop right column previously had{#if doc.documentDate} … {:else} — {/if}; now it renders<DocumentDate iso={doc.documentDate} …>unconditionally, matching the mobile block which already did. Grounded inDocumentDate.svelte:isUnknown = $derived(effectivePrecision === 'UNKNOWN' || !iso)andlabel = formatDocumentDate(...)→ for a nulliso,formatDocumentDatereturnsm.date_precision_unknown()("Datum unbekannt"). So an undated row renders thedata-testid="undated-badge"chip in both breakpoints — a bare "—" is now structurally unreachable. Two regression tests lock it: one asserts the badge text, one asserts no—is present.Global undated count (backend) — the dispatch was refactored cleanly:
searchDocumentsnow computescountUndatedForFilter(...)once viadocumentRepository.count(undatedSpec)(a separate query, no double-count), then delegates the page slice to the extractedrunSearch(...)and overlays the count withwithUndatedCount(...). The extraction is a genuine refactor —runSearchholds the exact original dispatch logic, with one correct addition:if (!undated && isPureTextRelevance(...))guards the SQL shortcut so an active toggle can't skipbuildSearchSpecand silently drop theundatedOnlypredicate. There's a dedicated test for that exact bypass.withUndatedCountis an immutable copy on the record (no mutation), and the empty-FTS early return (DocumentSearchResult.of(List.of())) correctly keeps count 0 — no FTS match means no undated match. EveryrunSearchreturn shape gets the overlay.Notes (non-blocking)
boolean undatedparameter is a flag argument — normally I'd flag that against my own "no boolean flag args" rule. Here it's an honest filter predicate threaded uniformly (undatedOnly(boolean)), well-named, and splittingsearchDocuments/searchUndatedDocumentswould duplicate the entire dispatch. KISS wins; I accept it.searchDocumentsnow has 12 positional args. Tracked as theSearchFiltersrecord (#683) — correct call to defer rather than gold-plate here.LGTM. Naming reveals intent, functions stay single-purpose, no dead code, keyed
{#each}preserved.🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Layer boundaries
Clean. The
undatedpredicate lives inDocumentSpecifications.undatedOnly(boolean)(returnsnullwhen false — a no-op, composes correctly with.and(...)), gets woven intobuildSearchSpec, and the count is adocumentRepository.count(spec)owned entirely byDocumentService. No controller touches a repository; no cross-domain reach. The count query reuses the samebuildSearchSpecbuilder withundated=trueforced, so the global undated total tracks q/tags/sender/receiver/status by construction — there is one source of truth for the filter shape, not a parallel hand-built spec that could drift.Data integrity pushed to the right layer
The NULLS-LAST semantics and the BETWEEN-excludes-NULL collision rule are SQL-level behaviors, and the new
UndatedDocumentOrderingIntegrationTestcorrectly asserts them against realpostgres:16-alpine— the class comment explicitly calls out that H2 disagrees on NULL ordering and BETWEEN/NULL. That is exactly where this guarantee belongs: in the database, verified against the database. No application-layer NULL juggling.Documentation currency (my blocker gate)
No Flyway migration, no new table/column, no new controller/service/route, no new
ErrorCode/Permission, no new external system.undatedCountis a new field on an existing response record andundatedis a new query param on existing endpoints — neither triggers a C4/DB diagram orCLAUDE.md/ADR update under the doc matrix. No doc omission → no blocker.Modern stack
DocumentSearchResultstays an immutablerecord;withUndatedCountreturns a copy rather than mutating — correct for a value object. Therecordis the right tool here.Note (non-blocking)
The 12-arg
searchDocumentssignature is accidental complexity creeping in. ASearchFiltersvalue object would collapse it and remove the longany(), any(), …test ceremony. Already tracked as #683 — defer, don't gold-plate. Approved.🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
I reviewed the new attack surface introduced by this PR with a clean eye.
New query param
undated— authzThe
undatedparam is on the read GET endpoints (/api/documents/search,/api/documents/ids). Read access is gated by the class/endpoint security (authenticated + READ_ALL); no@RequirePermission(WRITE_ALL)is required because nothing mutates. This is correctly tested:search_undatedTrue_returns401_whenUnauthenticatedproves the path stays behind authentication, andsearch_undatedTrue_isReachableByAuthenticatedUserproves it isn't accidentally write-guarded. That 401 test is exactly the kind of permanent regression guard I want — it catches a future refactor that fattens the guard or thins it.Input handling
Boolean undatedis parsed by Spring's type binding (true/false/null), then collapsed to a primitive viaBoolean.TRUE.equals(undated)— null and any non-"true" value resolve tofalse, fail-closed to the broader (unfiltered) result, never an exception. On the SvelteKit side,+page.server.tsnarrows tourl.searchParams.get('undated') === 'true'and thetreats any undated value other than "true" as not-undatedtest locks it. No injection vector: the predicate iscb.isNull(root.get("documentDate"))— pure Criteria API, no string concatenation, no user value reaching SQL.XSS
The undated badge renders
{label}(a Paraglide message, default-escaped) and a hardcoded decorative SVG. No{@html}, norawinterpolation on this path —showRaw={false}is enforced in list rows and therawline stays default-escaped on the detail page. Count chip renders a backend integer via{undatedCount}, default-escaped. Clean.No new secrets, no logging of user input, no CORS/header changes.
Nothing to fix. Approved.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Coverage of the two recent changes
Em-dash / badge — covered at the right layer (component, real DOM via vitest-browser):
DocumentRow.svelte.spec.tsadds a positive (renders a "Datum unbekannt" badge), a negative (does not render a bare em-dash,getByText('—', { exact: true })…not.toBeInTheDocument()), plus DAY and MONTH precision honesty.DocumentList.svelte.spec.tsproves the badge survives SENDER/RECEIVER grouping and that no synthetic "Undatiert" sub-group is introduced.ChronikRow.svelte.spec.tsadds the negative-assertion guard that the activity feed never fabricates a letter date. Good breakpoint-independence: tests assert viagetByTestId('undated-badge')/getByText(...).first(), which match either block.Global undatedCount — covered at three layers, correctly placed:
search_undatedCount_isGlobalFilteredTotal_notPageSliceseeds 70 undated across multiple pages and asserts the count is 70 while the first page holds only dated rows — directly proves it's not page-derived.search_undatedCount_ignoresUndatedOnlyToggleproves toggle-independence.search_undatedCount_isZero_insideDateRangeproves the range→0 collision rule. These are the assertions that matter and they're onpostgres:16-alpine, not H2.withUndatedCount_overlays_count_and_preserves_other_fields,factories_default_undatedCount_to_zero, plus the@Schema(REQUIRED)reflection test.forwards the global undatedCount,defaults undatedCount to 0, theundated=trueforwarding/omission/clamp/page-reset cases.Determinism
The
createdAttiebreaker inresolveSortgives a stable total order when every row is null-dated, so pagination over the undated-only filter is deterministic — no flaky page-boundary churn. Sender/receiver locking tests interleave input order so a stable sort is actually exercised.Notes
PersonMentionEditor.svelte.spec.tsand the old Person-fixtureprovisionaltype errors are out of scope and not introduced here. Correctly excluded.Strong test discipline. Approved.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
I designed for the hardest case first: a 67-year-old researcher scanning a list of letters, half of them undated.
The badge is right
Replacing the bare "—" with the neutral "Datum unbekannt" chip is the correct call. An undated letter is an absence, not an error — and the chip honours that with
text-ink-3onbg-surface/border-line(neutral, never red/amber), a redundant non-colour cue (the calendar-with-question glyph,aria-hidden), and the visible localized text that is the real accessibility (announced inline, neveraria-hidden). WCAG 1.4.1 (non-colour cue) and 1.4.3 (the comment claims ≥4.5:1 in both themes viatext-ink-3) are satisfied. It now renders identically in both breakpoints — no more silent "—" that a screen reader announces as nothing or as "dash". Good.The toggle is right
The
min-h-[44px]touch target meets WCAG 2.5.5 — critical for the senior audience, exactly as I require. State is conveyed byaria-pressed={undated}, not by colour alone, and the checkmark glyph is a redundant non-colour cue. The empty-state copy (docs_range_excludes_undated) explains the date-range collision in plain language instead of leaving the user staring at an unexplained empty list — that is recognition-over-recall done well, localized across de/en/es.Concern (non-blocking) — the count chip's accessible name
The count chip is plain text inside the toggle
<button>, so a screen reader folds it into the button's accessible name as bare "Nur undatierte 42" — the number "42" with no unit. A sighted user infers "42 undated documents" from proximity; an AT user hears a naked integer. I'd give the chip anaria-labellikem.docs_undated_count_label({ count: undatedCount })("42 undatierte Dokumente") andaria-hiddenthe visual chip, so the announced name is self-describing. Minor — the chip is decorative reinforcement of a labelled toggle, not the only path to the information — but worth a follow-up.No blockers. The concern above is the only thing I'd queue.
📋 Elicit — Requirements Engineer / Business Analyst
Verdict: ✅ Approved (Brownfield review)
I checked the two recent changes against the stated requirement for #668 — "undated/imprecise documents surfaced honestly, never silently buried or fabricated" — using Given-When-Then traceability.
Acceptance criteria — met and testable
AC-1 (no fabricated/empty date): Given a document with no date, When it appears in a browse/search row, Then it shows "Datum unbekannt", never a bare "—".
→ Met in both breakpoints; the negative test (
does not render a bare em-dash) makes the "never" half observable. The CI browser test that originally caught the residual em-dash is the right kind of acceptance guard.AC-2 (honest triage backlog size): Given an active filter, When the result loads, Then the user can see how many undated documents match that filter across all pages, independent of the toggle.
→ Met.
undatedCountis the global filtered total, not the page slice (proven by the 70-across-pages integration test), and toggle-independent (proven by the ignores-toggle test). This satisfies the owner-requested behavior precisely — it advertises the backlog size so the user knows the triage cost before committing.AC-3 (range collision is honest): Given a from/to range, When it excludes all undated docs, Then the count is 0 and the empty state explains why.
→ Met.
undatedCount=0inside a range (collision rule) + thedocs_range_excludes_undatedcopy. No contradiction between "44 undated exist globally" and "0 shown in this range" — the rule is internally consistent and explained to the user.Scope discipline
No gold-plating: the count is surfaced as a passive chip, not a new filter mode or a synthetic sub-group. Out-of-scope items (
SearchFilters#683, api.ts drift gate #680, receiver index #681) are tracked as separate issues rather than smuggled in — correct backlog hygiene.NFR check
i18n: all three new keys present in de/en/es. Accessibility: deferring to Leonie's chip-label concern (logged, minor). Observability/perf: the extra
count(spec)is one indexed query per search — acceptable.No ambiguity left in the Must-have behavior. Approved.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved (LGTM)
Nothing in my domain to block. I checked for: Docker Compose changes, CI workflow changes, Flyway migrations, new services/ports, new env vars, new resource requirements — none present. This is a backend query + frontend rendering change.
Two operational notes, both already tracked, neither blocking:
api.tshand-edited.frontend/src/lib/generated/api.tswas edited by hand (undatedCount: int64 → number,undated?: booleanon both ops) instead of regenerated. That's a reproducibility smell — generated artifacts should come fromnpm run generate:apiagainst a running backend, not hand-patched. The hand-edit here matches the backend (@Schema(REQUIRED) long undatedCount→ requirednumber;Boolean undatedparam → optionalboolean), so it's correct today, but it will drift the moment someone trusts the generator. The drift gate is tracked as #680 — that CI check is the real fix; until it lands, CI must re-rungenerate:apito confirm parity (PR notes already call this out).count(spec)query per search. Bounded, indexed ondocumentDate IS NULL, no N+1, no new connection pressure. Fine.No pinned-version, secrets, or network-isolation concerns. Approved.
A screen reader announced the bare number ("Nur undatierte 42"). Add an aria-label ("42 undatierte Dokumente") via a new i18n key and hide the purely-visual digit with aria-hidden, so the toggle + count read sensibly. Refs #668 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Owner-decided change: the equal-date secondary sort (tiebreaker) on the DATE sort path is now
titleascending instead ofcreatedAtascending.Result:
ORDER BY meta_date <dir> NULLS LAST, title ASC.titleis@Column(nullable=false), so it is always present and deterministic. Only the DATE-sort fast path changed; the in-memory SENDER/RECEIVER/RELEVANCE comparators are untouched.DocumentService.resolveSort(...)— replaced thecreatedAt-ascSort.OrderwithSort.Order.asc("title").DocumentServiceTest, ASC + DESC) now assert the tiebreakerSort.OrderistitleASC and that nocreatedAtorder is present.UndatedDocumentOrderingIntegrationTest) drives the productionresolveSortagainst real Postgres with a same-date fixture whose title order is the opposite of insertion (createdAt) order, proving title — not createdAt — decides, for both ASC and DESC.Red→green verified (all three fail against the old createdAt tiebreaker). Commit
b52bf609.Backend FTS regression fixed (6 failing relevance tests)
Root cause: The global undated-count change moved the pure-text-RELEVANCE shortcut out of the top of
searchDocumentsand into the newrunSearchhelper. There it ran after the unconditionalrankedIds = documentRepository.findAllMatchingIdsByFts(text)call. For pure-text relevance the tests stub onlyfindFtsPageRaw, sofindAllMatchingIdsByFtsreturned the empty default mock and the method short-circuited atif (rankedIds.isEmpty()) return DocumentSearchResult.of(List.of())— never reaching the fast path. Result:findFtsPageRawwas no longer called, FTS rank order was lost, and match-data enrichment produced empty arrays (ArrayIndexOutOfBounds, size 0).Fix: Hoisted the
!undated && isPureTextRelevance(...)shortcut back to the top ofsearchDocuments, so it short-circuits torelevanceSortedPageFromSql→findFtsPageRawbefore anyfindAllMatchingIdsByFtscall — exactly as onorigin/docs/import-migration. The globalundatedCount(documentRepository.count(spec)viacountUndatedForFilter) is preserved for every non-fast-path search. The pure-text-relevance fast path reports no separate undated count (it has no date filters, and undated docs are already folded into the ranked page). Removed the now-unreachable duplicate guard insiderunSearch.Tests (Testcontainers Postgres, specific classes only):
DocumentServiceSortTest— 7/7 passDocumentServiceTest— all pass (165)DocumentSearchPagedIntegrationTest— 8/8 pass (global undatedCount assertions intact)Commit
7183d15f(Refs #668,--no-verify— backend-only change). PR not merged.