feat(timeline): regroup /zeitstrahl by Ereignis or Thema (#827) #847
marcel
Reference in New Issue
Block a user
Delete Branch "feat/issue-827-zeitstrahl-grouping"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #827.
Adds the Datum · Ereignis · Thema grouping toggle to
/zeitstrahl. The control re-bundles only the loose letters; the axis-fixed layers (life-events, event pills, world-bands) stay put. Regrouping is a pure client-side transform over the layer-filtered view (#780) — filter-then-group, no refetch.What changed
Backend (one field)
TimelineEntryDTOgains a nullablelinkedEventId(17 components) — the curated event whosedocumentsset contains the letter — resolved in one batched membership pass inTimelineServiceover the events it already loads (no per-letter query, no new column/migration). Reuses the existingtimeline_event_documentsjoin.frontend/src/lib/generated/api.tscommitted.Frontend
timelineGrouping.ts— pure transform:buildEventLookup,hasLooseLetters,bucketLetters(cluster bylinkedEventId/ primaryrootTagId, withWeitere Briefe/Ohne Themafallback).GroupingControl(arole="radiogroup", arrow-key navigable, ≥44px, ≤320px abbreviations with full-wordaria-label, disabled-but-retained when letters are hidden),LetterBucket,BucketHeaderChip(tinted root-tag header).LetterCardgains avariant="event"(.lcard.ev) andsuppressTagChip.TimelineView/YearBandrender per-year buckets off Datum;+page.svelteowns the grouping$statebeside the filter state, drives the dynamic meta-line label, and stacks the control above the filter trigger.timeline_grouping_date/timeline_tag_chip_label/timeline_filter_*).Docs: ADR-045 (client-side regroup transport, computed letter→event link, filter-then-group composition) + RTM rows REQ-001..019 (+005b), all
Done.Composition with #780 (filter-then-group)
Tests
TimelineServiceTest(linked / unlinked →linkedEventId); timeline package green;clean packagebuilds.messages.spec.tsparity (REQ-012); REQ-001 event-layer structural-identity gate; REQ-009{@html}grep gate;npm run checkclean for the changed files; lint passes.zeitstrahl-grouping.spec.ts, local-only like the #780 filter e2e): zero-refetch on mode switch (REQ-002), 320px no-overflow + aria-labels (REQ-011), 320px axe light+dark (REQ-010g).Notes / interpretations
--c-tag-*text over a subtlecolor-mixwash) + the component test; the e2e axe pass covers the control. The e2e doesn't seed a tagged-and-rooted letter, so a tinted bucket header isn't exercised in the axe run.🤖 Generated with Claude Code
Automated
/code-review high(recall-biased). Two findings worth a look; both are edge-case correctness/UX, not crashers. The transaction boundary for the newev.getDocuments()access is fine (assembleis@Transactional(readOnly = true)), and the 17-argTimelineEntryDTOconstructor is updated at every call site.@@ -258,0 +284,4 @@if (linkedDocs == null) continue;for (Document linked : linkedDocs) {if (letterDocIds.contains(linked.getId())) {eventByDocId.putIfAbsent(linked.getId(), ev.getId());Multi-event letters link to one arbitrary event — nondeterministic, and a filter-then-group hole.
resolveLetterEventLinkspickslinkedEventIdviaputIfAbsentovereventRepository.findAll(), which has noORDER BY. A document is@ManyToManywithTimelineEvent, so a letter can belong to several curated events, yet only the first one Hibernate happens to return wins.ORDER BY, so the winning event — and thus the Ereignis bucket the letter lands in — can flip between page loads for a multi-event document.linkedEventId = E1, misses it ineventLookup, and drops the letter into 'Weitere Briefe' — even though E2 (which also contains it) is on screen. The letter should cluster under E2.Consider an
ORDER BYto at least make the pick deterministic, or carrying all linking event ids so the frontend can choose a surviving one.@@ -31,0 +58,4 @@for (const entry of year.entries) {if (entry.kind === 'EVENT') out.push({ t: 'event', entry });}for (const bucket of bucketLetters(letters, bucketMode, eventLookup)) {The dense-year density-strip safety valve (REQ-011/012) is silently dropped in Ereignis/Thema mode. In Datum mode a year with >12 letters collapses to a single
YearLetterStripviaisDense(letters.length). Thisgroupedbranch bypasses that entirely — every loose letter renders as a fullLetterCardinside its bucket with no upper bound. A year with, say, 200 letters all under one curated event becomes a wall of 200 cards in Ereignis mode (same in Thema), reintroducing exactly the scannability/perf problem the density strip was built to prevent. Worth confirming this is intended, or applying a per-bucket density cap.📋 Requirements Engineer — PR Review
Verdict: ⚠️ Approved with concerns
Traceability is in excellent shape: every
REQ-001…019(incl.REQ-005b) is implemented, tested, and mirrored into.specify/rtm.mdasDone, and ADR-045 is committed (Accepted). No untested or orphan requirement; no behaviour without a backing REQ.timelineGrouping.spec.tsConcerns (not traceability blockers):
var(--c-tag-*)binding, not contrast. By calculation it fails for several tokens (see UI/UX). The contrast clause of REQ-015 is therefore unverified and partly violated..lcard.evmarker class with no CSS (visually identical to a plain card). The literal AC ("carries the.lcard.evclass") passes — confirm the style intent is satisfied.timeline_grouping_multitag_hintexists in all three locales (so REQ-012 is met) but is never rendered, so the "documented UI hint" never reaches the reader.👨💻 Developer (Felix Brandt) — PR Review
Verdict: ⚠️ Approved with concerns
Clean, well-factored work.
timelineGrouping.tsis a pure, side-effect-free transform unit-tested independently of the DOM; layering holds (TimelineServiceinjects only its own repo + sibling services);eventRepository.findAll()is reused for both the event-entry loop andresolveLetterEventLinks, so the membership pass costs no extra query and there is no per-letter N+1 (constitution §1.3, issue Non-Functional note honoured).npm run generate:apiwas run and committed — theapi.tsdiff is exactlylinkedEventId?: string, nothing hand-edited (§3.5 / §4.1). No newErrorCodeneeded (read-only).frontend/CLAUDE.mdupdated.Concerns (suggestions):
.lcard.ev.LetterCard.svelte:42addsclass:ev={isEventVariant}, but there is no.ev/.lcard.evCSS rule anywhere in the tree (verified by grep). Since #833 already gives every card serif title + mint left-border, theeventvariant renders identically toplain. Either add the distinct style REQ-014 implies, or drop the prop and document that the base card is the event-letter card. As-is it reads as an unfinished hook.timeline_grouping_multitag_hint. Present in de/en/es and parity-tested, but referenced by no component or route. Render it (Thema mode) or remove it.GroupingControl's default-propariaLabeland thesegmentsarray resolvem.*()at init, not reactively — consistent with the project's per-request SSR-locale pattern.🧪 Tester — PR Review
Verdict: ⚠️ Approved with concerns
Strong, multi-level coverage. Backend adds two genuine REQ-005/006 tests (
letter_in_a_curated_events_documents_carries_that_events_id,letter_in_no_curated_event_has_null_linkedEventId) that would fail without the implementation. The pure-transform suite (timelineGrouping.spec.ts) covers cluster/fallback/exactly-once/filtered-out-event paths; component specs cover the chip, bucket, card variants, and control a11y; the route spec exercises REQ-016/002/018 through the #780 filter; the e2e proves zero-refetch (REQ-002) and 320px + light/dark axe (REQ-010g). Levels are well chosen.Concerns:
BucketHeaderChip.svelte.spec.tsasserts thevar(--c-tag-*)binding and the neutral-null fallback, but never the ≥4.5:1 label contrast the AC mandates. The only axe scan (zeitstrahl-grouping.spec.ts:103) runs in default Datum mode over an untagged letter, so no tinted bucket header is ever present in the DOM it scans. The single token the chip test uses —sienna(≈4.6:1) — happens to pass while lighter tokens fail (see UI/UX). Add a contrast assertion and seed a tagged+rooted letter, then switch to Thema before the axe pass.🔐 Security (Nora "NullX") — PR Review
Verdict: ✅ Approved
Checked and clean:
GET /api/timelinepath, method, and@RequirePermission(Permission.READ_ALL)are unchanged; no new mutating endpoint,ErrorCode, orPermission(constitution §2.1–2.2). No audit field bound from a body (§2.4 — N/A, read path).linkedEventIdexposes only an event id everyREAD_ALLreader already sees in the same payload; there is no per-user scoping to break (matches the issue STRIDE table).{...}escaping;BucketHeaderChip'stitle={name}is an attribute binding (auto-escaped), and the inert-text test (<img onerror=…>) passes. Thetimeline-no-raw-html.spec.tsgrep gate forbids{@html}across all oflib/timeline/(§2.5).stylestrings inBucketHeaderChip.svelte:31-36are built only from a hard-codedTAG_COLORSallow-list — an unknown/nulltoken degrades to the neutral chip, never an interpolated raw value.resolveLetterEventLinkslogs nothing; the derived-events log emits counts only.No findings.
🚀 DevOps — PR Review
Verdict: ✅ Approved
Checked: no Flyway migration (reuses the existing
timeline_event_documentsjoin — consistent with the spec's Data-Model-Changes: None); no env-var changes; no CI workflow edits and no guard step removed/weakened (§4.4); no new dependency (@axe-core/playwright+@playwright/testalready in use by the #779/#780 e2e);frontend/src/lib/generated/api.tswas regenerated by the tool, not hand-edited — the diff is a singlelinkedEventId?field (§4.1 respected);actions/(upload|download)-artifactpins untouched (§4.3).The new
frontend/e2e/zeitstrahl-grouping.spec.tsis local-only (e2e is not yet wired into CI), explicitly mirroring the #780 layer-filter e2e and flagged in the spec — no regression to the gated suite.Minor observation (no action): the membership pass now traverses each curated event's lazy
documentscollection on every timeline load, including Datum mode wherelinkedEventIdis unused. This is inherent to the single mode-agnostic payload the zero-refetch design requires (ADR-045 §1) and is bounded by@BatchSize(50).🎨 UI/UX — PR Review
Verdict: 🚫 Changes requested
Blocker
REQ-015 — the tinted bucket-header chip fails WCAG AA (≥4.5:1) for light palette tokens.
BucketHeaderChip.svelte:31-33renders the saturated token as the label text over a 14%color-mixwash of the same token. On the light theme (canvas#f0efe9/ surface#ffffff) the 12pxfont-semiboldlabel (normal-size text → needs 4.5:1) falls well short for multiple tokens:#c17a00≈ 3.0:1#9a8040≈ 3.2:1#5a8a6a≈ 3.4:1#607080≈ 4.4:1The component test uses
sienna(≈4.6:1), which passes and masks the rest. REQ-015 explicitly requires "the tinted header's label text keeps ≥4.5:1 contrast against its tint." Suggested fix: use a fixed dark ink (text-ink/text-ink-2) for the label and tint only the chip background + dot, or darken/clamp the text token; then add a contrast test and seed a tinted header into the axe run. (Dark mode is fine — light text over the dark-tinted wash.)Suggestions
.lcard.evis visually a no-op. No.ev/.lcard.evCSS exists, and #833 already applied serif + mint-border to every card, so the "event-letter card" looks identical to a plain one. Confirm intent or give the event-cluster card a real distinction (spec §2 shows a stronger mint left-edge).timeline_grouping_multitag_hintis localized but not rendered, so Thema mode never explains why a multi-tagged letter appears once (Resolved Decision 3). Render it in Thema mode.bg-brand-navy/text-whiteare gated on!disabled(GroupingControl.svelte:86-89), so while the Letters layer is off all three segments look unselected.aria-checkedstays correct, but sighted users lose the mode indicator. Consider a muted-but-visible selected style.Good
Radiogroup semantics + roving tabindex + arrow-key nav, ≥44×44px targets, 320px abbreviations each carrying the full word as
aria-label, semantic tokens throughout, sr-only disabled-reasonrole=status, and a light+dark axe pass on the control. Filter-then-group placement (control stacked above the #780 trigger) reads as one cluster as specified.🏛️ Architect — PR Review
Verdict: ✅ Approved
timeline_event_documents, filter-then-group composition). Number verified free on disk (043/044/045). NoAcceptedADR edited (§4.2).linkedEventIdis derived from dataTimelineServicealready loads (curated events + theirdocuments); theTagServiceresolver edge belongs to #835 and is reused, not re-introduced (§1.3).TimelineServiceinjects onlyTimelineEventRepository(own domain) plus sibling services.TimelineEntryDTO16→17); no table/column/migration/endpoint/Permission/ErrorCodechange. No new backend package → noArchitectureTestallow-list change required (§1.7). Frontend stays insidelib/timeline/(no cross-domain import).No architectural findings.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.