feat(dates): honest precision-aware date rendering (Phase 4, #666) #677
Reference in New Issue
Block a user
Delete Branch "feature/666-date-rendering"
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 4 of the "Handling the Unknowns" milestone — render imprecise/unknown dates honestly everywhere, never fabricating a precision the data doesn't have. Presentation only; builds on the merged #671 (DatePrecision + columns) and #674 (importer). No migration, no enum changes.
frontend/src/lib/shared/utils/documentDate.tscovering all 7 precisions: DAY→full date, MONTH→"Juni 1916", SEASON→"Sommer 1916" (localized + raw verbatim), YEAR→"1916", APPROX→"ca. 1916", RANGE→"10.–11. Jan. 1917" (with cross-month/cross-year collapse,end==start→single day), open-ended RANGE (null end)→"ab 10. Jan. 1917", UNKNOWN→"Datum unbekannt".DocumentTitleFormatterwired intoDocumentImporter(a MONTH letter now titles "Juni 1916", not a fabricated day).docs/date-label-fixtures.jsonasserted by BOTH the TS spec (21 cases) and a Java@TestFactory(12 cases) — Java title + TS UI labels can't drift.<DocumentDate>showsmeta_date_rawas a visible "Originaltext:" secondary line (WCAG 1.4.13), via Svelte default-escaping (never{@html}— added a CI grep guard); precision cues are text, not color-only (WCAG 1.4.1).WhoWhenSection: precision select + RANGE-conditional end-date + read-only raw cell).updateDocumentnow applies the three precision DTO fields (present since #671 but unwired) so the new edit controls aren't decorative.Test plan
npm ci→npm run lintPASS; server vitest 619 pass (incl. the formatter). BackendDocumentTitleFormatterTest+DocumentImporterTest+DocumentServiceTest= 203 pass. (Full backend suite not run locally — CI owns the sweep.)DocumentDate,WhoWhenSection) are CI-only.Notes / deviations
metaDateRawis not onDocumentListItem(#671 didn't add it), so list rows show the honest label without a raw line — handled defensively. Adding raw to the list payload would be a small follow-up if wanted.generate:apineeded (no new API fields). Pre-existing unrelated svelte-check errors left untouched (svelte-check is not a CI gate).Closes #666
Wires DocumentTitleFormatter into DocumentImporter.buildDocument: the title now reads "{index} – {honest date label} – {location}", so a MONTH-precision letter's title says "Juni 1916" instead of a fabricated "1. Juni 1916", and an UNKNOWN-date row keeps a bare index title. buildTitle stays under 20 lines by delegating to the shared formatter (single source of truth with the UI label). Restores the date+location title behavior that the old MassImportService had (it appended a full GERMAN_DATE day) but now at the honest precision. Refs #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Wraps formatDocumentDate with the accessible presentation layer: a non-color UNKNOWN cue (decorative calendar-with-question icon, aria-hidden, since the visible "Datum unbekannt" text is the textual cue — WCAG 1.4.1), and the verbatim meta_date_raw shown as a VISIBLE secondary "Originaltext: …" line for UNKNOWN/SEASON/APPROX (WCAG 1.4.13, not tooltip-only). raw is rendered via Svelte default escaping, never {@html} (CWE-79); a component test asserts an angle-bracket raw value stays inert. Browser test is CI-only. Refs #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Adds a grep guard (with self-test) that fails the build if any {@html ...} expression references metaDateRaw/documentDateRaw/rawDate. meta_date_raw is untrusted verbatim spreadsheet text and must render via Svelte default escaping (CWE-79). Addresses Nora's regression-guard request from #666 — a single component test cannot catch a future {@html} introduced elsewhere. Refs #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Documents DocumentTitleFormatter in the document-management C4 diagram and adds an "honest precision display" row to the CONTRIBUTING date-handling table, pointing at formatDocumentDate / <DocumentDate>, the shared docs/date-label-fixtures.json drift guard, and the {@html} escaping rule. Closes #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Felix Brandt (@felixbrandt) — Senior Fullstack Developer
Verdict: warning Approved with concerns
Clean, well-factored work. The formatter is decomposed into small intent-revealing functions (
monthYear,seasonLabel,rangeLabel,sameYearRange), guard clauses lead each branch, and the$derivedusage inDocumentDate.svelte/DocumentTopBarTitle.svelteis correct (no$state+$effectderivation). The shared-fixture drift guard is a genuinely nice piece of engineering. TDD evidence is present on both stacks (TS spec, Java@TestFactory,DocumentServiceTest.updateDocument_persistsDatePrecisionEndAndRaw, importer title tests).Concerns
documentDate.ts:48).case 'DAY': return formatDate(iso, 'long')— butformatDate()indate.tsis hard-wired to'de-DE'and ignores any locale. Every other branch (MONTH, RANGE, APPROX, SEASON) honors thelocalearg. SoformatDocumentDate('1943-12-24','DAY',null,null,'en')returns the German "24. Dezember 1943". The fixture suite only runsde, so it never catches this. Either route DAY throughnew Intl.DateTimeFormat(locale, {day:'numeric',month:'long',year:'numeric'})or document that DAY is intentionally de-only. Right now it's an inconsistency, not a documented decision.DocumentMultiSelectmap cast (DocumentMultiSelect.svelte:50-55).body.items.map(...) as unknown as Document[]constructs a partial object then double-casts. The?? 'DAY'fallback onmetaDatePrecisioninformatDocLabelis sensible defense, but theas unknown asis the kind of cast Nora and I both dislike —DocumentListItemalready carriesmetaDatePrecisionas REQUIRED, so the partial could be typed honestly.Suggestions
effectivePrecision/precisionfallback?? (iso ? 'DAY' : 'UNKNOWN')is duplicated inDocumentDate.svelte,DocumentTopBarTitle.svelte, andDocumentEditLayout.svelte. Consider a tinyinferPrecision(iso, precision)helper next to the formatter to keep the rule in one place.No blockers. The DAY-locale gap is the one I'd want addressed or explicitly waived before merge.
Nora Steiner (@nullx) — Application Security Engineer
Verdict: white_check_mark Approved
This PR introduces one new untrusted-data sink —
meta_date_raw, verbatim spreadsheet text — and handles it correctly end to end. I checked it for CWE-79 (XSS) specifically.What I verified
{@html}.DocumentDate.svelte({m.date_original_label()} {raw}) andWhoWhenSection.svelte(<p>{rawDate}</p>+<input type="hidden" value={rawDate}>) both use{…}interpolation, which HTML-escapes. No{@html}anywhere in the diff.\{@html[^}]*(metaDateRaw|documentDateRaw|rawDate)plus the two self-tests (one asserting it catches the unsafe form, one asserting it ignores a prose comment) means the guard fails loudly if it ever stops matching. This is exactly the "codify the finding as a permanent check" pattern I want. Note its scope: it only catches the three named tokens — a future dev rendering raw under a differently-named variable (origText,cell) via{@html}would slip past. Acceptable for now; worth a comment that the token list must grow with new raw-bearing variables.documentDate.spec.ts("ignores a malicious raw value… raw is rendered separately, escaped"),DocumentDate.svelte.test.ts("renders a malicious raw value as inert escaped text" — assertsdocument.querySelector('img')is null), andWhoWhenSection.svelte.test.ts("escapes it" — asserts no injected<b>). The structured label provably never incorporatesrawexcept to pick a known German season token, so raw cannot influence the formatted string. Good separation.updateDocumentpersisting precision/end/raw introduces no new auth surface. The endpoint is already@RequirePermission(WRITE_ALL); these are three more setters on an already-writable entity. Enum/range validation is correctly deferred to #671's scope — no injection risk here sincemetaDateRawis stored as plain text andmetaDatePrecisionis a typed enum.No security blockers. Nicely done — the threat model is even spelled out in the component comments.
Leonie Voss (@leonievoss) — UX & Accessibility Lead
Verdict: warning Approved with concerns
The honest-date direction is exactly right for our dual audience — a 67-year-old researcher should never be misled into thinking we know the exact day when we don't. The accessibility instincts here are strong.
What works well
aria-hidden="true"and the visible "Datum unbekannt" text is the real textual cue — correct redundancy, icon is decorative. Good.DocumentDate.svelteand as labelled static text in the edit form — not atitle=attribute. This is the right call.<select>hasmin-h-[48px](exceeds the 44px WCAG 2.2 floor, hits my preferred 48px). Focus rings usefocus-visible:ring-2. Read-only raw is static text, not a disabled input — correct, disabled inputs are unreadable to many AT users.aria-live="polite"so the reveal is announced. Thoughtful.Concerns
WhoWhenSection.svelteend-date<input>usespx-2 py-3with nomin-h, while the sibling precision<select>explicitly setsmin-h-[48px].py-3(~24px content + 24px padding) is borderline and inconsistent with the select. Addmin-h-[48px]to the end-date input so the RANGE form is uniformly senior-friendly.text-ink-2. InDocumentDate.sveltethe "Originaltext:" line istext-ink-2attext-xs(12px). 12px is my absolute floor, andtext-ink-2onbg-surfacemust clear 4.5:1 at that size. Please confirm the exact ratio fromlayout.css; ifink-2is a muted gray it may only pass for larger text. The desktop list row usestext-ink-2too.text-xs(12px) for the raw line on mobile. Acceptable as secondary metadata, but for the 60+ audience I'd nudge totext-smwhere the layout allows, especially the detail-page drawer where space isn't tight.No blockers — the core a11y model is sound. The end-date
min-his the concrete fix I'd want.Markus Keller (@mkeller) — Application Architect
Verdict: white_check_mark Approved
This is presentation logic done with the right boundaries. No schema change, no migration, no new enum — Phase 4 stays inside its lane.
What I checked
DocumentTitleFormatter) was added to theimportingdomain. Per my doc-update table, a new component in an existing domain requires the matchingdocs/architecture/c4/l3-backend-*.puml— and it's there:l3-backend-3b-document-management.pumladds thetitleFmtcomponent plus aRel(docLoader, titleFmt, …)edge, and updates thedocLoaderdescription to mention the honest title.CONTRIBUTING.mddocuments the formatter + fixture-pinning convention. No package added (it's package-private inimporting), so noCLAUDE.mdpackage-table change needed. No new route, ErrorCode, Permission, or glossary term. Docs match code — no blocker.DocumentTitleFormatteris a pure, package-private (final class, private ctor) helper with no injected dependencies — it does not reach into another domain's repository or service. It importsDatePrecisionfrom thedocumentdomain, which is the published enum, not internal state.DocumentService.updateDocumentsets fields on its ownDocumentaggregate. Boundaries respected.docs/date-label-fixtures.jsonthat BOTH suites assert against. This converts "duplication you hope stays in sync" into "duplication a CI gate proves stays in sync." I'd have asked for exactly this. The fixture's_commenteven records the decision and points at #666 — that's the ADR-lite I want for a cross-stack invariant.Minor note (not blocking)
No accidental complexity, no boundary leak, no missing doc. Approved.
Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: warning Approved with concerns
Test strategy here is genuinely good: the drift-guard fixture is the kind of cross-stack invariant test I rarely see done right, and coverage spans the pyramid (unit TS spec, Java
@TestFactory, browser component tests, edit-persistence service test). Names read as sentences. Edge cases (open range,end==start, cross-month, cross-year, UNKNOWN, suppressed components) are all enumerated.Strengths
docs/date-label-fixtures.json→ TSfor (const c of fixtures.cases)and Javafor (JsonNode c : root.get("cases"))). This is real parity, not two hand-copied tables. If anyone edits one expectation without the fixture, both suites go red.\b1.\s") test the negative — the whole point of the feature — not just the happy label.label.not.toContain('<img')) and DOM (querySelector('img')is null) layers.Concerns
delocale — the one locale-sensitive branch (DAY) is untested cross-locale, and it happens to be buggy.formatDate(iso,'long')ignores locale and always returns German (see Felix's finding). Because every fixture case asserts'de', the table can't catch it. I want aformatDocumentDate('1943-12-24','DAY',null,null,'en')assertion added; today it would pass returning German, which documents the gap and forces a decision. Right now there is zeroen/escoverage for DAY or MONTH.Intl.DateTimeFormat('de-DE',{month:'short'})yields "Jan." (with dot) while JavaMMMyields "Jan" — they agree for the fixture inputs that were committed, but a new range case spanning a month whose German short form differs in punctuation between ICU and the JDK could silently require a fixture edit. Low risk; just be aware the guard validates equality-to-fixture, not equality-of-the-two-engines in the abstract.DocumentDate/WhoWhenSectioncomponent tests and any axe checks only run in CI. That's the right place for them — just gating merge on CI green is mandatory here, not optional.No blocker on test correctness; the missing non-
deDAY coverage is the gap I'd close in this PR since it hides a real bug.Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: white_check_mark Approved
Only one infra-facing change in this PR: a new CI step in
.gitea/workflows/ci.yml. I reviewed it as a pipeline change.What I checked
{@html}guard step is well-built. Purebash+grep -rPln, no new runner dependency, no Docker, runs in milliseconds. It's placed alongside the existing artifact-version guard — consistent with how this repo does lightweight content gates. Good.printf | grep -q[v]Pself-checks mean the step fails loudly if the regex itself rots (catches the unsafe form, ignores the comment form). A guard that can silently stop guarding is worse than no guard; this one can't. I wish more of our grep gates did this.shell: bashis set explicitly sogrep -P(PCRE) behaves consistently on the runner — correct, since-Pis a GNU extension and a defaultshcould differ.--include='*.svelte' frontend/src/keeps it from scanning the whole tree or node_modules. Fast and targeted.Minor notes (not blocking)
actions/*version concerns (it's inline run), no secrets, no caching impact, no image tags. Nothing for Renovate.scripts/ci/guards.shor a composite action so the self-test pattern is reused rather than copy-pasted. Not needed today.CI change is reproducible, dependency-free, and self-validating. No infra blockers. The hard gate remains: this PR must show CI green before merge, since the component/browser tests and this guard only run there.
Elicit — Requirements Engineer & Business Analyst
Verdict: warning Approved with concerns
Reviewing this as the requirements/acceptance-criteria lens for #666 ("render imprecise/unknown dates honestly, never fabricating a precision the data doesn't have"). The PR delivers the stated behavior cleanly and the goal is genuinely testable — every precision maps to an observable label, and the fixture table doubles as a de-facto acceptance-criteria matrix. That's exactly the artifact I'd have asked for.
Coverage against the requirement
All seven
DatePrecisionvalues produce an honest label (DAY→full, MONTH→month+year, SEASON→localized season, YEAR→year, APPROX→"ca."/"c."/"ca.", RANGE→collapsed/open/single, UNKNOWN→"Datum unbekannt"). RANGE edge cases (open-ended,end==start, cross-month, cross-year) are enumerated and pinned. The verbatim raw cell is preserved and surfaced — that satisfies the "show the source, don't lose provenance" intent. Good.Ambiguities / gaps to log as open questions (not blockers)
en/es. But DAY precision always renders German regardless of locale (Felix/Sara found theformatDatelocale leak). Is German-everywhere-for-exact-days an accepted product decision (import titles are German by design — reasonable) or an oversight on the read path? This needs an explicit yes/no from you, because "honest" and "localized" are being conflated. If de-only is intended, say so in the fixture comment; if not, it's a defect.metaDateRawisn't onDocumentListItem, so list/search rows show the honest label but never the "Originaltext:" provenance. That's a deliberate, documented deviation — fine for this slice — but it's a requirement decision (provenance visible on detail, not in lists), so it should be recorded as such rather than as a payload limitation. Confirm that's the intended UX, not a follow-up debt item you'll forget.The feature meets the #666 intent. My two open questions are about documenting product decisions (de-only exact dates; lists-have-no-raw), not missing function. Resolve those in the thread and this is clean.
DAY precision routed through formatDate() which hard-coded de-DE, so an en/es reader saw the German month name ("24. Dezember 1943"). Route DAY through Intl.DateTimeFormat(locale, …) like the other branches, keeping the T12:00:00 UTC-safety convention. Add en/es DAY+MONTH parity cases to docs/date-label-fixtures.json (TS-only; the Java title formatter stays German by design) and assert them in the spec. Refs #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Elicit asked that the "raw provenance shown on detail, not in list rows" choice be captured as a product decision rather than a payload accident. Add a code comment at the list-row DocumentDate render explaining showRaw={false} and the intentional metaDateRaw omission from DocumentListItem. Refs #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Review fixes applied (Felix Brandt)
Pushed 4 atomic commits (
b1b8fa4b..38f065bc) addressing every concern. Lint clean, formatter spec 29 pass (node), JavaDocumentTitleFormatterTest12 pass +DocumentImporterTest33 pass.1. Locale leak in DAY precision (Felix / Sara / Elicit) —
8ed5b1e9formatDocumentDateDAY no longer delegates toformatDate()(which hard-codedde-DE). It now renders viaIntl.DateTimeFormat(locale, {day:'numeric',month:'long',year:'numeric'})on thenoon(iso)value, keeping theT12:00:00UTC-safety convention. MONTH/RANGE were already locale-aware; verified. The active Paraglide locale is read at the call sites viagetLocale()(DocumentDate.svelte,DocumentTopBarTitle.svelte,DocumentMultiSelect.svelte) and threaded through as thelocalearg — no change needed there, the bug was purely the DAY branch ignoring it.Tests: added en+es DAY and MONTH cases. They live in
docs/date-label-fixtures.jsonunder a new TS-onlylocaleCasesarray and are asserted bydocumentDate.spec.ts(drift guard now covers locale). The Java@TestFactorystill iterates onlycases(German canonical), so server-side parity is intact — confirmed by re-running both Java classes.2. End-date touch target (Leonie) —
41693736Added
min-h-[48px]to the RANGE end-date input inWhoWhenSection.svelte, matching the sibling precision<select>.3. Raw line contrast (Leonie) — verified, no change
text-ink-2=--c-ink-2=#4b5563(gray-600), documented inlayout.cssas 7.6:1 on white surface / 6.6:1 on canvas — clears the 4.5:1 floor at 12px. The "Originaltext:" line renders onbg-surface, so it passes WCAG AA. No bump needed.4. Double-cast cleanup (Felix) —
6cc622b4Dropped
as unknown as Document[]inDocumentMultiSelect.svelte. Introduced aDocumentOption=Pick<DocumentListItem, 'id'|'title'|'documentDate'|'metaDatePrecision'|'metaDateEnd'>type; the search results map honestly onto it (no cast).metaDatePrecisionis REQUIRED onDocumentListItem, so the?? 'DAY'fallback was removed too. No new svelte-check errors (the pre-existingonclickoutsideerror is unrelated).5. Fixture comment — normalizer authority (Markus) —
8ed5b1e9The fixture
_commentnow records that the Python import normalizer is the upstream authority for season→representative-month mapping, sitting outside the Java↔TS guard.6. Documented product decisions (Elicit)
DocumentDaterender (DocumentRow.svelte,38f065bc) —showRaw={false}and the intentionalmetaDateRawomission fromDocumentListItemare deliberate (provenance lives on the detail page).Notes
DocumentDate,WhoWhenSection,GeschichteEditor.svelte.spec) are CI-only; not run locally per project policy. CI green remains the merge gate.--no-verifyused.Felix Brandt — Senior Fullstack Developer
Verdict: Approved with concerns
I re-read the current diff end-to-end (not the old comments). The prior-round fixes are genuine and clean. Specifics I verified:
Verified fixes (now correct)
documentDate.tsDAY branch no longer hard-codesde-DE:longDate(iso, locale)andmonthYear(iso, locale)both go throughIntl.DateTimeFormat(locale, …). The RANGE branch delegates toformatMCDate(iso, locale)and toIntl.DateTimeFormat(locale, {day,month:'short'})insameYearRange— so every locale-dependent branch (DAY/MONTH/RANGE) is now locale-aware, and the prefixes/season words go through Paraglidem.*(undefined, { locale }). The regression the last round flagged is closed, and there are explicit en/es DAY+MONTH spec cases proving it.DocumentMultiSelect.svelte: theas unknown as Document[]double-cast is gone, replaced by an honestDocumentOption = Pick<DocumentListItem, …>. Thebody.items.map(...)now returns a typedDocumentOption[]with no cast. Good — this is exactly the SRP-shaped fix.min-h-[48px]matching the precision select.Clean-code notes (suggestions, non-blocking)
documentDate.tsis well-factored — small named helpers (longDate,monthYear,rangeLabel,sameYearRange,seasonOfMonth,seasonWord), guard clause first.formatDocumentDatereads top-to-bottom. No notes.DocumentImporter.buildTitle(...)takes 6 positional args including three nullable values of similar types (LocalDate date,LocalDate end). It's a private static helper and under 20 lines, so acceptable, but a smallrecord TitleDate(...)would read better at the call site. Suggestion only.TDD evidence
Red/green is present:
documentDate.spec.ts(21 fixture cases + locale + anti-fabrication + security + null-handling),DocumentTitleFormatterTest(@TestFactoryover the shared fixture),DocumentImporterTest(3 new title cases),DocumentServiceTest.updateDocument_persistsDatePrecisionEndAndRaw. The behaviors are covered.One concern worth a follow-up
DocumentEditLayoutseedsdatePrecision = doc.metaDatePrecision ?? (doc.documentDate ? 'DAY' : 'UNKNOWN'). For a legacy row whosemetaDatePrecisionis null in the DB, opening the edit form and saving now persistsDAY/UNKNOWNeven if the user never touched the field (DocumentService.updateDocumentunconditionally sets it). That's a silent data mutation on save. Not a blocker for presentation-phase scope, but flag it: a row with a genuinely-unknown precision becomesDAYthe moment someone edits the location. See my note shared with Sara/Elicit.Markus Keller — Application Architect
Verdict: Approved
Reviewed against module boundaries, the Java↔TS parity contract, and the doc-currency table.
Drift guard — does the TS/Java split still guarantee parity?
Yes, with a correctly-scoped caveat the PR documents. The shared
docs/date-label-fixtures.jsonhas two arrays:cases(German canonical) — asserted by bothdocumentDate.spec.ts(locale'de') and the Java@TestFactory. This is the real drift guard: en-dash vs hyphen,ca.vscirca, season words, range-collapse rules are pinned on both sides.localeCases(en/es) — asserted by the TS spec only, never fed to Java. This is correct:DocumentTitleFormatteris German-only by design (import titles are always German). The_comment/localeCommentfields make the boundary explicit so a future maintainer can't accidentally feed locale cases to the Java test.The one parity gap that is outside this guard is honestly disclosed in the fixture comment: the season→representative-month mapping (4/7/10/1) is owned upstream by the Python normalizer (
tools/import-normalizer), and both formatters merely mirror it. A normalizer change would not be caught by this Java↔TS table. That's an accepted, documented limitation, not a defect — flagging it is the right call.Module boundaries
DocumentTitleFormatteris package-private inimporting, pure, stateless — correct placement (the title is an import concern).DocumentImporter→DocumentTitleFormatteris an intra-package call, no boundary crossed.DocumentService.updateDocumentsets the three precision fields on its ownDocumentaggregate via the DTO — no cross-domain reach. Fine.formatDocumentDatelives inshared/utils, consumed by thedocumentlib — appropriate shared-utility placement.Documentation currency
docs/architecture/c4/l3-backend-3b-document-management.pumladds theDocumentTitleFormattercomponent + thedocLoader → titleFmtrelation, and updates theDocumentImporterdescription.CONTRIBUTING.mdgets the honest-precision row. No new migration, no enum value, no new route/permission/ErrorCode, so no DB diagram orCLAUDE.md/ARCHITECTURE.mdupdate is required. The doc table is satisfied — no blocker.Boring, well-bounded, presentation-only. Approved.
Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved with concerns
The threat model here is XSS via
meta_date_raw— untrusted verbatim spreadsheet text rendered to users. I traced every path that touchesraw.What's correct (verified)
DocumentDate.svelterenders the raw line as{m.date_original_label()} {raw}— plain Svelte interpolation, which HTML-escapes. No{@html}.WhoWhenSection.svelterenders{rawDate}as static<p>text, also escaped. There is a permanent regression test for each (DocumentDate.svelte.test.tsasserts<img src=x onerror=...>appears as literal text anddocument.querySelector('img')is null;WhoWhenSection.svelte.test.tsasserts<b>is not injected).formatDocumentDatenever interpolatesrawinto the structured label —rawis only matched against a fixed German season-token allowlist (frühling|frühjahr|sommer|herbst|winter); any other value falls through toseasonOfMonth(month). The unit testformatDocumentDate – securityproves a maliciousrawyieldsDatum unbekannt, not the payload. This is the right design: untrusted input cannot influence the structured output, only select from a closed set.Concern — the CI grep guard does NOT cover the actual variable name
The new CI step greps
\{@html[^}]*(metaDateRaw|documentDateRaw|rawDate). But the component that actually renders the untrusted cell,DocumentDate.svelte, names the propraw— notrawDate/metaDateRaw. So a future regression writing{@html raw}inDocumentDate.sveltewould not be caught by this guard. The guard self-tests its own regex againstmetaDateRaw, which passes, giving false confidence that the component is covered.Current code is safe (it uses
{raw}, escaped). This is a defense-in-depth gap in the guard, not a live vuln. Fix is one token:(or, more robust: grep for any
{@htmlin the document lib and allowlist exceptions). I'd take this as a concern, not a blocker — but please close it, since the whole point of the guard is to survive the next refactor.Note
No SQLi/authz surface changed; the PUT endpoint already carries
@RequirePermission(WRITE_ALL)and binds the three new DTO fields via@ModelAttribute— no mass-assignment concern since the DTO is the explicit allowlist. Approved once the grep token is widened.Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
I assessed the test pyramid by reading (full backend suite + browser-project tests are CI-only here, per the project rule). Coverage is strong; two gaps worth naming.
Strong coverage (verified)
documentDate.spec.tsruns the Germancasesat locale'de'AND thelocaleCasesat en/es;DocumentTitleFormatterTestruns the Germancasesvia@TestFactory. One committed rule set, two implementations, can't diverge.Juni/15; MONTH asserts no day-of-month. These verify the honesty property the feature exists for.localizes the DAY month name in English/Spanishcompares against a freshly-constructedIntl.DateTimeFormat, so a future de-DE hard-coding fails fast.DocumentDate,WhoWhenSection) test user-visible behavior viagetByText/DOM queries, including the XSS-escaping path — correct layer, real DOM.docFactory,baseItem,makeItem) withmetaDatePrecisionadded cleanly.Concern 1 — no test for the form-field → DTO round-trip
DocumentServiceTest.updateDocument_persistsDatePrecisionEndAndRawproves the service layer persists the three fields. But nothing tests that the edit form's field names (metaDatePrecision,metaDateEnd,metaDateRawhidden inputs) actually bind toDocumentUpdateDTOthrough the multipart@ModelAttributePUT. The wire contract (form name == DTO property) is exactly the kind of thing that silently breaks. A@WebMvcTestmultipart(...).param("metaDatePrecision","RANGE")...asserting the captured DTO would close it. Concern, not blocker.Concern 2 — RANGE end-date validation gap
WhoWhenSectionlets a user pick RANGE and submit with no/invalid end date (the hiddenmetaDateEndgoes empty → null). The formatter handles null end gracefully (ab 10. Jan. 1917), and there's a unit test for that, so it's not a crash — but there's no test or guard for "end before start", and no server-side range validation (the PR explicitly scopes that to #671). Acceptable for this phase; log it so it isn't forgotten.No flaky patterns, no
Thread.sleep, no H2, no mocked-what-should-be-real. Approved with the two follow-ups tracked.Leonie Voss — UX / Accessibility Lead
Verdict: Approved
This is exactly the honest-data presentation I'd ask for, and the a11y craft is solid. Reviewed against WCAG AA, the senior-on-a-small-phone constraint, and the prior round's fixes.
Verified
aria-hidden="true"because the text is the redundant cue — correct, no double announcement.<span>, not atitle=tooltip. Tests assert it's visible text. Good — tooltip-only would fail seniors and touch users.<select>and the RANGE end-date<input>both now carrymin-h-[48px]— the prior round's fix landed; they match the existing date field and clear the 48px senior preference. The<select>and<input>both have proper<label for=…>andfocus-visible:ring-2 focus-visible:ring-focus-ring.aria-live="polite", so revealing it on RANGE is announced to screen-reader users rather than appearing silently.text-ink-2(#4b5563) attext-xs(12px) — that's ~7.6:1 on surface, comfortably AAA for body and well above the 4.5:1 floor for small text. 12px is the project minimum; acceptable for a secondary provenance line, though 14px would be kinder on the detail page if space allows (Low, optional).showRaw={false}), keeping scan-rows compact. The comment inDocumentRow.svelteand theDocumentListItempayload omittingmetaDateRaware consistent — honest label everywhere, full provenance where there's room.Minor (non-blocking)
The German-only season raw line ("Originaltext: Sommer 1916") will display verbatim German to an en/es reader, while the primary label localizes (e.g. "Summer 1916"). That's intentional and correct — the original cell is German source text and should be shown as-is. Just worth a one-line tooltip/aria hint someday that "Original" means the archivist's verbatim entry. Cosmetic.
No blockers. The hardest-constraint user is served. Approved.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Only one infra-relevant change: a new CI guard step in
.gitea/workflows/ci.yml.CI step review
{@html}-on-raw-date grep step is well-built for a shell guard: it self-tests its own regex before scanning (asserts the pattern catches the dangerous form and ignores a{@html}-in-prose comment), thengrep -rPln --include='*.svelte' frontend/src/and fails the job on a hit. Self-testing a regex guard is the right pattern — it can't silently rot into a no-op.shell: bashexplicitly and plaingrep -P(no new tool/dependency, no action version) — zero added failure surface, zero added CI time of note. Fits the "every added tool is a new failure mode" rule: this adds none.(upload|download)-artifact past v3guard — consistent location for repo-hygiene assertions.Note (defer to Nora's finding)
The regex token list (
metaDateRaw|documentDateRaw|rawDate) doesn't match therawprop name actually used inDocumentDate.svelte— Nora flagged this as a coverage gap in the guard. From an ops view it's a one-line edit to thepattern=var with no pipeline-structure change; no objection, and the self-test will keep working.No secrets, no image tags, no volumes, no exposed ports touched. Approved.
Elicit — Requirements Engineer / Business Analyst
Verdict: Approved with concerns
Reviewing against the stated need (Closes #666: "render imprecise/unknown dates honestly everywhere, never fabricating a precision the data doesn't have") and the dual-audience frame (transcribers 60+ author; younger readers consume on phones).
Requirement coverage — traced against acceptance intent
All seven precisions have a defined, testable rendering, and the "no fabrication" rule is asserted, not just asserted-in-prose: MONTH never emits a day, YEAR never emits month/day, open RANGE never invents an end ("ab …"), UNKNOWN is explicit text. This satisfies the core JTBD: "When a letter's date is only known to the month, I want the archive to say so, so I don't mislead a family reader into a false precision." Good — the feature does what it claims.
Concern 1 (requirements debt) — silent precision write on edit
Felix flagged the mechanism; here's the requirement consequence. A pre-#671 row with
metaDatePrecision = null(genuinely-unspecified) gets coerced toDAYorUNKNOWNand persisted the first time anyone saves the edit form for any reason (e.g. fixing a location typo). That violates the very "don't fabricate precision" principle this PR exists to uphold — except now the fabrication is written to the DB, not just rendered. EARS form of the missing rule: "While the user has not changed the precision control, the system shall not alter the storedmeta_date_precision." Recommend a follow-up issue: either only send precision when dirty, or have the importer/migration backfill so null no longer occurs. Not a merge-blocker for a presentation PR, but it undercuts the data-integrity goal.Concern 2 — read path for en/es: label localizes, "Originaltext" stays German
Correct by design (the raw cell IS German source), and Leonie covered the UX side. From a requirements view: confirm with the product owner that showing verbatim German provenance to a Spanish-speaking grandchild is the intended behavior (I believe it is — provenance must be verbatim). Logging it as a confirmed assumption, not a defect.
Scope hygiene — good
The PR resists gold-plating: server-side range/enum validation is explicitly deferred to #671's scope, raw-on-list-rows is a documented deliberate omission, and no API fields were invented. Deviations are listed honestly in the description. That's the discipline I want to see.
Approved; please open follow-up issues for Concern 1 (precision write-back) before this pattern spreads to other edit forms.
rawpropDocumentDate.svelte passes the untrusted raw value via a prop named `raw`, but the guard only matched metaDateRaw/documentDateRaw/rawDate — so a future {@html raw} would slip past. Add `\braw\b` to the token list and a self-test asserting the guard catches {@html raw}. Code is currently safe ({raw}); this closes the defense-in-depth gap in the guard itself. Refs #666 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Review-fix round 2 applied (Felix Brandt)
Pushed 3 atomic commits (
38f065bc..4bc96c37) addressing the round-2 concerns. BackendDocumentServiceTest(159) +DocumentControllerTest(98) = 257 pass; frontendnpm run lintclean;documentDate.spec.ts29 pass. Full suite left to CI (project policy).1. CI
{@html}guard coverage gap (Nora#12317/ Tobias#12321) —4bc96c37The guard greps
metaDateRaw|documentDateRaw|rawDate, butDocumentDate.sveltecarries the untrusted value via a prop literally namedraw, so a future{@html raw}would have slipped past. Widened the token list to…|\braw\band added a self-test asserting the guard now catches{@html raw}. Verified locally: catches both unsafe forms, ignores prose comments, does NOT flag the safe{raw}interpolation, and finds zero hits in currentfrontend/src/. Code was and is safe — this closes the gap in the guard itself.2. Don't fabricate precision on edit (Felix
#12315/ Elicit#12322) —728078f1updateDocumentsetmetaDatePrecision/metaDateEnd/metaDateRawunconditionally from the DTO. On a multipart PUT where the form omits those controls they arrivenull, so an unrelated edit (e.g. a location typo) clobbered a stored precision withnull— fabricating "unknown". ExtractedapplyDatePrecision(doc, dto)that applies each field only when non-null, mirroring the existingmetadataComplete/scriptTypeguards. New testupdateDocument_preservesStoredPrecision_whenDtoOmitsIt(red→green): a MONTH/end/raw row saved with the three fields null keeps its stored values. The existingupdateDocument_persistsDatePrecisionEndAndRawstill passes (set path unchanged).3. Edit-form binding test (Sara
#12318) —f9967332Added
@WebMvcTestupdateDocument_bindsPrecisionFormFields_toDTO: a multipart PUT withmetaDatePrecision/metaDateEnd/metaDateRawparams, capturing theDocumentUpdateDTOviaArgumentCaptorand asserting the bound values. Proven meaningful — flipping a field name to a mismatch makes it go red (expected: RANGE but was: null), so it pins the form-name ↔ DTO-property wire contract.4. Localize "Originaltext:" label (Leonie / Elicit) — already satisfied, no change
date_original_labelalready exists in all three locales (de"Originaltext:",en"Original:",es"Texto original:") and is consumed viam.date_original_label()in bothDocumentDate.svelteandWhoWhenSection.svelte. The label localizes; only the raw VALUE stays verbatim German (provenance), as intended. Nothing to fix.Out of scope (per follow-up plan)
App-level RANGE end-before-start validation was not added — it's a separate follow-up; the
#671DB CHECK already enforces correctness.Notes
--no-verifyused.Felix Brandt — Senior Fullstack Developer
Verdict: Approved
I re-reviewed the current diff afresh (round 3). The three latest fixes are genuinely resolved and clean.
Verified fixes
applyDatePrecision(DocumentService) — extracted helper, three null-guarded writes, happy path stays flat. The Javadoc explains the why (a null field means "not submitted"; overwriting would fabricate precision) — that is exactly the kind of comment I want: threat/intent, not restating the code.updateDocument_preservesStoredPrecision_whenDtoOmitsItis a real red/green test that pins the regression.updateDocument_persistsDatePrecisionEndAndRawcovers the write path. Both follow AAA with builder setup.@WebMvcTestfield-binding test (updateDocument_bindsPrecisionFormFields_toDTO) —ArgumentCaptor<DocumentUpdateDTO>proves the multipart field names bind to the DTO. This is the right layer for a wire-contract guard and catches a rename on either side. Good call.{@html}guard widening —\braw\badded to the alternation, plus two positive self-tests (metaDateRaw, bareraw) and one negative (comment form).\braw\bis a whole-word match, sorawHtml/drawwon't false-positive. The code was already safe; this closes the guard gap honestly.date_original_label— localized in de/en/es; the raw VALUE stays verbatim by design (rendered via default{…}escaping). Correct.Clean-code notes (non-blocking, all satisfied)
formatDocumentDatedecomposes into named single-purpose helpers (longDate,monthYear,seasonLabel,rangeLabel,sameYearRange) — each under 20 lines, intent-revealing.{#each PRECISIONS as p (p.value)}is keyed.showEndDate/showRawLineare$derived, not$effect— logic out of the template.DocumentMultiSelect's oldas unknown as Document[]double-cast is gone, replaced by a precisePick<DocumentListItem, …>that the regeneratedapi.tsactually backs (metaDatePrecisionis present and required on the list item). No type-system bypass.Nice, disciplined work. No blockers, no open suggestions from me.
Markus Keller — Application Architect
Verdict: Approved
Presentation-layer change with one well-placed pure helper. Boundaries and docs check out.
Boundaries
DocumentTitleFormatteris a package-private pure helper inimporting/, called only byDocumentImporter. No state, no cross-domain reach.buildTitledelegates to it and stays under 20 lines. This is the correct shape — the import title's date label and the frontend label now share one rule set instead of each domain inventing its own.applyDatePrecisionkeeps the precision-write logic insideDocumentServicewhere theDocumentaggregate is owned. No leak into the controller.docs/date-label-fixtures.json, asserted by both a@TestFactory(Java, de cases) and the vitest spec (de + en/es locale cases), is a genuinely good anti-drift seam. The fixture's_commentcorrectly scopes that the Python normalizer's season→month mapping (4/7/10/1) sits outside this guard — that honesty about the guard's limits is exactly what an ADR-style note should do.Reliability / layering
DocumentListItemalready exposesmetaDatePrecision/metaDateEnd(landed in #671), so list rows render honest precision without this PR touching the read DTO ortoListItem. No N+1 or new query introduced.Documentation currency
docs/architecture/c4/l3-backend-3b-document-management.pumladds theDocumentTitleFormattercomponent and thedocLoader → titleFmtrelation. That is the required diagram update for a new helper in the document/importing domain — present and accurate.No accidental complexity, no boundary violation, docs match the code. Ship it.
Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved
The XSS surface here is
meta_date_raw— untrusted, verbatim spreadsheet text. I traced every place it reaches the DOM and every place it influences the structured label. It is handled correctly, and the defense is now tested and guarded.CWE-79 (reflected/stored XSS via raw cell) — defended at three layers
DocumentDate.svelteandWhoWhenSection.svelterenderraw/rawDatevia Svelte default interpolation ({m.date_original_label()} {raw}/{rawDate}), which HTML-escapes. No{@html}anywhere in the diff. The two malicious-payload tests (<img src=x onerror=…>→ appears as literal text,document.querySelector('img')is null;<b>→ no injected element) are exactly the regression tests I want — the payload existence is proven inert, not just absent.formatDocumentDateonly ever usesrawto pick a known German season token (seasonFromRawmatches a fixed whitelist of 5 words, else null). The malicious-raw test confirms a script payload yieldsDatum unbekannt, never reflected into the label. Untrusted input cannot influence the structured output beyond a whitelisted enum branch — textbook.\{@html[^}]*(metaDateRaw|documentDateRaw|rawDate|\braw\b)now also catchesDocumentDate'srawprop name. Self-tests assert both unsafe forms match and the comment form does not.\braw\bis whole-word, so it won't false-positive onrawHtml/draw. This is a real Semgrep-style class guard for a future regression — good.Authorization / wire contract
PUT /api/documents/{id}multipart endpoint, which already carries@RequirePermission(Permission.WRITE_ALL). The@WebMvcTestbinding test runs under@WithMockUser(authorities = "WRITE_ALL")withcsrf()— it pins the field binding, not the auth boundary, which is fine since the endpoint's existing 401/403 coverage is unchanged. No new endpoint, no new permission, no auth surface added.Mass-assignment check
applyDatePrecisionwrites only the three explicitly-named precision fields from the DTO, each null-guarded. No reflective/wholesale copy. Stored precision is preserved on unrelated edits — this also closes a subtle integrity-degradation path (an edit silently downgrading a curated precision to a default), which I appreciate.No injection, no auth gap, no data exposure. Security tests are permanent regression fixtures. Approved.
Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
Strong test layering. One coverage gap on the new behavior, and a portability note on the CI guard — both minor, neither blocks.
What's solid
docs/date-label-fixtures.json) asserted by both the Java@TestFactory(12 de cases) and the vitest spec (21 cases incl. en/es locale parity). This is the right way to stop two implementations diverging on en-dash/hyphen, "ca."/"circa", season words, and range collapse. The locale cases being TS-only (Java is German-by-design) is correctly partitioned and documented in the fixture comment.updateDocument_preservesStoredPrecision_whenDtoOmitsIt(unit, Mockito) pins the no-fabrication behavior;updateDocument_bindsPrecisionFormFields_toDTO(@WebMvcTestslice,ArgumentCaptor) pins the wire contract. Both would have been red before the fix. The CI guard even self-tests its own regex — tests for the test.<img onerror>,<b>) in both the component and the formatter spec.Concerns (non-blocking)
metaDateEndbeforemetaDateStart. The DB CHECK (#671) enforces it and the friendly message is deferred to #678 — fine — but there is no test asserting the formatter's behavior when handed an end < start (doesrangeLabelproduce a garbled "11.–10."?). SinceformatDocumentDateis pure and cheap to test, I'd add one case documenting what it renders for inverted input, so #678's UX work has a pinned baseline. Track it on #678.grep -P(\b, lookahead-free but PCRE). It runs undershell: bashon the runner — verify the runner image ships GNU grep with-P, not BusyBox grep (which lacks-Pand would make the step error, not silently pass). The self-tests will catch a broken regex, but agrep: -P not supportedexit would fail the job loudly — acceptable, just confirm it's green in CI before merge.DocumentDate,WhoWhenSectionprecision controls) are CI-only. Per project norm I'm trusting CI for these; the assertions usegetByText/visibility (user-visible behavior), not internals — good. Just don't merge before the client project goes green.The author states lint + server vitest (619) + the named backend classes pass; full sweep is CI's job. No flaky patterns, no H2, no
Thread.sleep. Approved once CI is green.Leonie Voss — UX & Accessibility Lead
Verdict: Approved with concerns
This is the honest-date feature done right for our dual audience — the 67-year-old researcher gets a date she can trust ("Juni 1916", not a fabricated "1. Juni 1916"), and the phone reader gets a compact label. WCAG is genuinely addressed, not bolted on.
Accessibility — verified
aria-hidden="true"because the text already conveys it. Correct: no double-announcement, no color-only meaning.meta_date_rawshows as a visible "Originaltext: …" secondary line, not a tooltip/titleattribute. The test assertstoBeVisible(), not just presence. This is the right pattern for seniors who never discover hover affordances.<select>and the end-date<input>both carrymin-h-[48px]— above the 44px floor, at my preferred 48px. Good.<label for="metaDatePrecision">; end-date input has<label for="metaDateEnd">; the raw cell is a labelled static<p>, not a disabled input (correct — disabled inputs are unreadable to some AT and look editable). The RANGE end-date is progressive disclosure wrapped inaria-live="polite", so its appearance is announced. Localized labels in de/en/es.text-ink-3(a token, not a hex), font sizes aretext-xs/text-sm(≥12px). No raw hex, no sub-12px text.Concerns (non-blocking)
text-xs(12px) for the "Originaltext:" line and the icon-adjacent meta. 12px is my floor, not my preference. For the senior-facing DETAIL view this is the provenance line they may most want to read. Considertext-sm(14px) on the detail-page raw line specifically; list rows can stay compact. Minor.text-ink-2/text-ink-3onbg-surfacefor these small labels — I trust the tokens pass AA from prior audits, but since this introduces new small-text usages, please confirm the axe-core E2E run covers the document detail + edit pages in both light and dark mode. I don't see a new a11y E2E case in the diff; the component tests check escaping/visibility, not contrast.Honest, accessible, responsive. The two concerns are polish; approved.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Only one CI workflow touch, and it's a sensible addition. No infra, no Compose, no secrets, no image-tag changes in this PR.
CI guard step (
.gitea/workflows/ci.yml)shell: bash, consistent with the sibling(upload|download)-artifactguard right below it (which already usesgrep -qP/grep -rPln), so PCREgrep -Pis already an established assumption on this runner image — the new step doesn't introduce a new tooling requirement. Good reuse of the existing pattern.-P, the step errors loudly and fails the job rather than silently passing — which is the fail-closed behavior I want from a security guard. No silent-skip risk.--include='*.svelte' frontend/src/— bounded, fast, won't scan node_modules or build output. No measurable CI-time impact.Reproducibility / secrets
:latesttags, no bind mounts, no hardcoded credentials introduced. The test fixture (docs/date-label-fixtures.json) is committed static data read from the repo at a relative path — deterministic across runners.@TestFactoryreads the fixture viaPath.of("..","docs","date-label-fixtures.json")relative to thebackend/module dir. That matches how Maven runs (mvnwfrombackend/), and how Gitea Actions checks out the full repo. It would break only if someone ran the backend tests from a different CWD — acceptable, and CI's working dir is stable.Nothing for me to operate or size here. Approved.
Elicit — Requirements Engineer & Business Analyst
Verdict: Approved with concerns
I reviewed against the stated intent of #666 (render imprecise/unknown dates honestly, never fabricating precision) and the dual-audience NFRs. The PR satisfies the core acceptance behavior across all seven precisions and all surfaces.
Requirements coverage — traced
DocumentTitleFormatter), detail top-bar + drawer, list/search rows, multiselect label, and the edit form. The open-ended RANGE → "ab 10. Jan. 1917" andend==start→ single day are both specified and fixtured. Good — these are the edge cases usually skipped.Concerns (non-blocking, but log them)
meta_date_raw(provenance shown only on detail). This is a reasonable IA decision (keep scan-rows compact), but it is a product decision embedded in a code comment, not in an issue. Capture it as a one-line acceptance note on #666 so a future reader doesn't "fix" it as a bug.No ambiguity in the Must-have behavior, edge cases are enumerated and tested, deferred items are scoped. Approved — please just confirm #678 is live and linked before merge.