Cluster event letters inline in the chronological /zeitstrahl (no grouping toggle) #851
Reference in New Issue
Block a user
Delete Branch "feat/issue-850-inline-event-clustering"
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 #850
Summary
On
/zeitstrahl, a curated event that has letters linked to it now renders as a contained event card — the event is the card header (accent glyph, title,{date} · {kuratiert|abgeleitet}subtitle, count, and a curator edit link), with its linked letters listed inside (first 5, then a keyboard-operable show-more/less toggle). Letters in a year other than the event's band get a lighter cross-year✉ titlecard. Every other letter stays a plain, alternating, density-folding chronological letter. There is no grouping control — clustering is automatic and always on. The meta-line drops itsGruppierung: Datumsegment.This supersedes #827: it keeps that branch's event-card clustering and the computed
linkedEventId, and drops the toggle, the Thema mode, and the "Weitere Briefe" drawer.What changed
Backend
TimelineEntryDTOgains a nullablelinkedEventId(UUID; not@Schema(REQUIRED)).TimelineService.resolveLetterEventLinksresolves each letter's curated event in one batched pass over the events it already loads — no per-letter query, no new column, no Flyway migration.linkedEventId?field inapi.ts.Frontend
eventClustering.ts(buildEventLookup,splitYearLetters,CLUSTER_PREVIEW=5) — filter-then-cluster: a letter clusters only if itslinkedEventIdis set AND present in the lookup, otherwise it stays loose.EventCluster.svelte— the contained event card (same-year event header + edit link, or cross-year ✉ text header; first-5 + show-more).LetterCard.sveltegainscompact+variant='event'(the.lcard.evin-card letter).YearBand.svelterebuilt to render event clusters inline; loose letters keep the alternating layout and density strip, and the strip counts only loose letters (no duplication).TimelineView.sveltebuilds the event lookup once and threads it +canWriteto each band.+page.sveltedrops the grouping meta segment; the unusedtimeline_grouping_datekey removed from de/en/es.timeline_bucket_show_more/_lesskeys in all locales.{@html}grep gate overlib/timeline/.Tests (real runs)
TimelineServiceTest: 30 passed (incl. the 2 newlinkedEventIdtests);DerivedEventsAssemblyTest: 17 passed; backend main sources compile.LetterCard,EventCluster,YearBand,TimelineView,zeitstrahl/page): 81 passed (5 files).eventClustering,messages,timeline-no-raw-html): 18 passed (3 files).svelte-check: no new errors in the touched files (pre-existing baseline noise elsewhere unchanged).RTM: thirteen
REQ-001..013rows added for #850 (featureinline-event-clustering), Status Done.🤖 Generated with Claude Code
Automated
/code-review high(recall-biased, 7 finder angles → verify). 10 findings below, most-severe first. Several are behavior/UX judgement calls rather than crashes — flagging for you to confirm intent. The backend lazy-access is safe (assemble()is@Transactional(readOnly=true)), all 16-arg DTO constructor sites compile, the removedtimeline_grouping_datekey has no dangling refs, and i18n{count}parity is correct.@@ -258,0 +279,4 @@if (letterDocIds.isEmpty()) return Map.of();Map<UUID, UUID> eventByDocId = new HashMap<>();for (TimelineEvent ev : events) {[Efficiency]
resolveLetterEventLinkshydratesev.getDocuments()for ALL events, including those filtered out of the timeline. It iterates the fullallEventsset and touches the lazydocumentscollection on each, even events skipped by the type/person/generation/year predicates in the event loop.@BatchSize(50)caps this at ceil(N/50) queries (no N+1) but still transfers every event's join rows. On a large archive (e.g. 1000 events, 50 visible) it fires ~20 batch queries to load collections that are mostly discarded. Building the map only over events that survived filtering — or only those with ≥1 document — would bound the I/O to what's shown.@@ -258,0 +284,4 @@if (linkedDocs == null) continue;for (Document linked : linkedDocs) {if (letterDocIds.contains(linked.getId())) {eventByDocId.putIfAbsent(linked.getId(), ev.getId());[Determinism] Multi-event letter link is non-deterministic. When a document is in the
documentsset of two curated events,putIfAbsentoverevents(fromeventRepository.findAll(), whose iteration order JPA does not guarantee) keeps whichever event is seen first. Across re-seeds / VACUUM / deploys the same letter can cluster under a different event with no data change — it visibly jumps between event cards. The javadoc acknowledges 'first by repository iteration order wins', but that order is undefined; consider an explicit, stable tiebreak (e.g. lowest eventId, or earliest event date).@@ -0,0 +42,4 @@// Event-as-header: a same-year curated event renders as this card's header, mirroring EventPill —// glyph + title + date · provenance + an edit pencil for a curator. The title is never repeated// as a separate floating pill (REQ-002).const accent = $derived(event ? getAccentConfig(event) : null);[Reuse / DRY] EventCluster's same-year header re-implements EventPill's visual + logic contract. The
accent/provenance/eventSubtitle/canEditderiveds (and thecanWrite && !derived && eventId != nulledit gate) plus the glyph-circle markup, serif title, subtitle, anddata-testid="event-edit"anchor to/zeitstrahl/events/{eventId}/editare copied near-verbatim fromEventPill.svelte. The curator-affordance edit gate is called out as load-bearing in CLAUDE.md's DTO contract — having it stated in two components invites drift. Consider extracting a shared<EventHeader>(or aneventHeaderModel(entry, canWrite)helper alongsidegetAccentConfig) and usingGlyphLabelfor the glyph/sr-only pairs.@@ -0,0 +107,4 @@class="flex items-center gap-2 border-b border-line px-3 py-2"><span class="font-serif text-sm font-bold whitespace-pre-line text-ink"><span aria-hidden="true">✉</span>[Accessibility] The cross-year
✉glyph isaria-hiddenwith nosr-onlylabel, unlike the same-year header. The same-year header pairs<span aria-hidden>{accent.glyph}</span>with<span class="sr-only">{accent.label}</span>(lines 80–81), but this cross-year branch emits a barearia-hidden✉. A screen-reader user hears only the title (Briefe von der Front) with no cue that this is a letter group. ReuseGlyphLabelhere (asLetterCardalready does:glyph="✉" label={m.timeline_letter_glyph_label()}). Same gap on the bare· {count}spans (lines 88, 113) —· 2announces with no semantic context.@@ -19,0 +35,4 @@// Inside an event card the year frames the time, and these archive titles already// embed the date — so the compact in-card letter drops the redundant date chip when a// title is present, halving the row height and killing the duplicate date (#850).const showDate = $derived(!compact || !entry.title);[Correctness — info loss]
showDate = !compact || !entry.titledrops the date for any titled compact letter, even when the title has no date in it. The comment assumes archive titles always embed the date (H-0023 – 6. Juli 1916), buttitleis free-form OCR/import text. A clustered letter titled e.g.Brief an Mutterrenders with no date chip at all — and inside an event card the year band only frames the year, so the reader loses the letter's month/day entirely. The component can't infer date-redundancy from title presence alone; consider a explicit flag from the caller, or only suppress when the title actually contains the formatted date.@@ -42,0 +67,4 @@// A curated event whose letters live in THIS band becomes the contained card's// header — its title reads once, no separate pill (REQ-002). Otherwise it stays a// plain pill/world-band (REQ-005).const cluster = entry.eventId ? clusters.find((c) => c.eventId === entry.eventId) : undefined;[Correctness — possible REQ-005 regression] A HISTORICAL curated event with ≥1 linked letter silently loses its WorldBand treatment.
buildEventLookupkeys onkind === 'EVENT' && eventIdwithout checkingtype, so HISTORICAL curated events enter the lookup and produce clusters. Hereentry.eventId ? clusters.find(...)matches regardless of type and pushes{ t: 'eventcard' }, which always renders<EventCluster>. Thetype === 'HISTORICAL' → <WorldBand>branch (line 121) is only reachable on the plain{ t: 'event' }path.So the moment any letter links to a world event, it downgrades from the full-width world band to a mint family-style card (and EventCluster's header has no HISTORICAL-specific styling beyond the navy/mint glyph swap). If world events were meant to keep WorldBand chrome, this is a regression — confirm intent or gate clustering to
type !== 'HISTORICAL'.@@ -42,0 +74,4 @@} else {out.push({ t: 'event', entry });}} else if (loose.includes(entry)) {[Efficiency]
loose.includes(entry)inside the loop overyear.entriesmakes row assembly O(L²) in loose letters.looseis an Array, so.includesis O(L), run once per LETTER entry. A dense year band (the >12 path already exists) with a few hundred loose letters does hundreds×hundreds reference comparisons every time therows$derived.byrecomputes (e.g. on any layer-filter change that re-renders bands). Build aSetfromsplit.looseonce and test membership in O(1). (Same array-scan smell onclusters.findat line 72 andconsumed.includesat line 94, both lower-impact.)@@ -42,0 +90,4 @@// Cross-year clusters: a cluster whose event is NOT a same-year EVENT entry renders as a// text-header card (no pill, no edit link) holding this year's linked letters (REQ-004).for (const cluster of clusters) {[Correctness — chronology, REQ-003] Cross-year clusters are appended after every event and loose letter, breaking within-band time order. The backend sorts each band by
WITHIN_BAND_ORDER(chronological), and the main loop emits events + loose letters in that order — but this second pass pushes every cross-yeareventcardto the end ofout, regardless of its letters' dates.Scenario: 1917 band has a loose letter dated 1917-11 plus two letters dated 1917-02 linked to a 1916 event. The Feb letters render in a
✉card placed below the November letter — earlier-dated content sits visually below later-dated content, contradicting the strict-time reading the old YearBand guaranteed. Consider interleaving cross-year cards at the position of their earliest letter instead of appending.@@ -0,0 +32,4 @@const lookup = new Map<string, string>();const collect = (entries: TimelineEntryDTO[]) => {for (const entry of entries) {if (entry.kind === 'EVENT' && entry.eventId) lookup.set(entry.eventId, entry.title ?? '');[Edge case] An empty/whitespace event title produces a bare
✉cross-year card.lookup.set(entry.eventId, entry.title ?? '')stores''for a titleless event; insplitYearLettersthetitle !== undefinedcheck is still true for'', so the letters cluster, and a cross-year EventCluster renders✉with no label — an unlabeled mystery card. Curated event titles are normally required, so low-probability, but worth a guard (skip empty titles, or fall back to a placeholder).@@ -0,0 +36,4 @@}};for (const band of timeline.years) collect(band.entries);collect(timeline.undated);[UX — orphaned presentation] Undated curated events drive cross-year cards.
buildEventLookupcollectstimeline.undatedtoo, so an undated event (which renders as anEventPillin the undated bucket, explicitly out of clustering scope per TimelineView's comment) still seeds clusters: its dated linked letters scatter into year bands and each collapses into a✉ {title}cross-year card with no edit link and no spatial tie to the pill. The reader sees the event title twice with no relationship, and the letters are detached from their event. Consider excluding undated-bucket events from the lookup, or otherwise linking the two.📋 Requirements Engineer — PR Review
Verdict: ⚠️ Approved with concerns
Traceability against the #850 issue body (the spec). Every
REQ-NNNis implemented, tested, and the RTM rows are added withStatus: Doneand in sync with the code.+page.sveltedrops segment; GroupingControl never portedpage.svelte.spec.tsEventCluster.svelteEventCluster/YearBandspecsCLUSTER_PREVIEW=5EventClusterelse-branchYearBandspecYearBandspecsplitYearLetterseventClustering.speceventClustering.specTimelineServiceTest{...}escaping, no{@html}page.svelte.spec.tsmessages.spec.tsNothing is unimplemented or untested, so this is not a traceability blocker. Two spec gaps the code surfaced (behavior exists that no REQ pins down — decide and add a REQ, or accept):
Minor: REQ-013 carries no new test on this branch — fine as a regression req, but note it.
Blockers: none (traceability complete).
Concerns: two under-specified interactions above.
💻 Developer — PR Review (Felix Brandt)
Verdict: 🚫 Changes requested
Confirmed clean:
new TimelineEntryDTO(...)sites updated (3×TimelineEventService, 2×TimelineService, 3× test helpers) — backend compiles,TimelineServiceTest30 /DerivedEventsAssemblyTest17 green.api.tscarries only the regeneratedlinkedEventId?— agenerate:apiregen, not a hand-edit, so §3.5 satisfied and §4.1 (Do-Not-Touch) not violated.linkedEventIdnullable and deliberately not@Schema(REQUIRED)— correct.ErrorCode, endpoint, or migration.eventClustering.tsis small, pure, well-named.Blockers
LetterCard.svelte:showDate = !compact || !entry.titledrops the date for any titled compact letter.entry.titleis free-form OCR/import text — the comment's assumption that archive titles always embed the date (H-0023 – 6. Juli 1916) does not hold for a letter titled e.g.Brief an Mutter. Inside an event card the band frames only the year, so the reader loses month/day entirely. Suppress only when the title actually contains the formatted date, or take an explicithideDateprop from the caller. (matches automated finding #4)WorldBandto a mintEventClustercard.buildEventLookupkeys onkind==='EVENT' && eventIdwith notypecheck, so thetype==='HISTORICAL' → WorldBandbranch inYearBand.svelte:121becomes unreachable once any letter links to a world event. Gate clustering totype !== 'HISTORICAL', or confirm this is intended (it contradicts #779 REQ-009). (finding #2)Suggestions
loose.includes(entry)runs once per LETTER inside theyear.entriesloop; build aSet(split.loose)once (clusters.find/consumed.includesare lower-impact variants of the same smell). (finding #3)EventCluster's same-year header re-implementsEventPill'saccent/provenance/canEditgate and edit anchor near-verbatim — third caller, time to extract.🧪 Tester — PR Review (Sara Holt)
Verdict: 🚫 Changes requested
Strong baseline: each REQ has a behavior-named test, factory-driven setup (
makeEntry/makeEvent/linkedLetters), real-DOM browser specs, a pure-logiceventClustering.spec.ts, and a self-testing{@html}grep gate. 81 client + 18 server + 30+17 backend green.Blockers
LetterCard.svelte.spec.ts#drops the redundant date line in the compact variant when a title is presentasserts the date disappears for any title — it locks in the info-loss the Developer flagged (finding #4). The correct test is the inverse: the date must survive when the title does not embed a date.YearBand/EventCluster) renders a band of only linked letters, so the append-at-end ordering bug is structurally invisible. Add a band mixing loose + cross-year-linked letters in interleaved dates and assert DOM order.Suggestions
splitYearLettersunit level. AYearBand/TimelineViewtest that toggles a layer off through the real #780 path would prove filter-then-cluster end-to-end (the user-visible contract).TagServiceIntegrationTestproving the@BatchSizebatched pass on real Postgres (Testcontainers). One here would also pin the multi-event determinism gap (finding #9), which currently has no test.🔐 Security — PR Review (Nora "NullX" Steiner)
Verdict: ✅ Approved
What I checked, adversarially:
GET /api/timelinekeepsREAD_ALL; no new mutating endpoint, so no@RequirePermissiongap. The 401/403 paths from #777 are untouched.linkedEventIdis computed server-side inresolveLetterEventLinks, never bound from a request body. Audit fields untouched.buildEventLookupis built from the response the reader already receives).linkedEventIddiscloses only an event id aREAD_ALLreader can already see; there is no cross-tenant model here.{@html}appears nowhere inlib/timeline/— enforced by the newtimeline-no-raw-html.spec.tsgrep gate, andEventCluster.svelte.spec.tsrenders<img src=x onerror=alert(1)>as inert text. Letter/sender/receiver/tag titles all go through{...}.resolveLetterEventLinkslogs nothing; no names/ids enter logs.Non-security note: an empty/whitespace event title produces a bare
✉card (finding #8) — a UX edge case, not a vuln.🚀 DevOps — PR Review (Tobias Wendt)
Verdict: ✅ Approved
Deployability is clean:
timeline_event_documentsjoin (ADR-040) and computes the link on read — correct, nothing to roll back, noV<n>to verify.act_runnerto choke on; no CI guard weakened (§4.4 intact).api.tschange is agenerate:apiregen, not a hand-edit — consistent with the single backend field added./actuator/*exposure change.Note (operational, minor):
resolveLetterEventLinksiterates the fulleventRepository.findAll()and touches the lazyev.getDocuments()on every event, including those filtered out of the response.@BatchSize(50)caps this at ⌈N/50⌉ queries (no N+1), but it still transfers join rows for events that are never shown. On a large archive (e.g. 1000 events, 50 visible) that's ~20 batch queries loading collections that get discarded. Bounding the map to events that survived the filter (or have ≥1 document) keeps the I/O proportional to what's rendered. (finding #10)🎨 UI/UX — PR Review (Leonie Voss)
Verdict: ⚠️ Approved with concerns
Good work: semantic tokens throughout (
bg-surface,border-line,border-l-brand-mint,text-ink-3,text-brand-navy— no raw hex); the show-more toggle hasaria-expanded+ a real 44pxmin-height(REQ-003); new strings added to de/en/es with{count}placeholders (REQ-012); the same-year edit ✎ pairs anaria-hiddenglyph with ansr-onlylabel.Concerns
✉header glyph isaria-hiddenwith nosr-onlylabel — unlike the same-year header (which pairs glyph +sr-only) andLetterCard(which usesGlyphLabel). A screen-reader user hears only the title with no cue that this is a letter group. ReuseGlyphLabel glyph="✉" label={m.timeline_letter_glyph_label()}. The bare· {count}spans likewise announce as "· 2" with no context. (finding #6)Minor: in-card letter text drops to
text-xs(12px) — exactly the accessibility floor for this audience; acceptable but at the edge.Biggest gap in one line: the cross-year card is the only timeline group with no screen-reader cue for its glyph.
🏛️ Architect — PR Review (Markus Keller)
Verdict: ⚠️ Approved with concerns
Systemic fit is good:
EventClusterlives inlib/timeline/,linkedEventIdis a computed DTO field (no DB column, reusestimeline_event_documents/ ADR-040), no new domain, no controller→repository or cross-domain repository access, and the view is assembled inside the@Transactional(readOnly=true)assemble()(§1.6). No Accepted ADR is contradicted and none is required. No doc-table update is triggered — no new route, domain, migration,ErrorCode, orPermission.Concerns
EventClusterre-implementsEventPill's header model. Theaccent/provenance/eventSubtitlederiveds and — load-bearing — the edit gatecanWrite && !event.derived && event.eventId != nullplus the/zeitstrahl/events/{eventId}/editanchor are copied near-verbatim. That gate is the curator-affordance contract documented in CLAUDE.md'sTimelineEntryDTOrow; it now lives inEventPill,WorldBand, andEventCluster— the third caller. §3.2 (KISS-over-DRY, extract on the third real caller) now favours a sharedEventHeader/eventHeaderModel(entry, canWrite)so the security-relevant gate has one source of truth. (finding #5)type !== 'HISTORICAL'if world events should never cluster) so the boundary stays legible. (finding #2)eventRepository.findAll()iteration order (JPA does not guarantee it) viaputIfAbsent. The javadoc acknowledges "first by repository iteration order wins," but that order is undefined — a letter can jump between event cards across re-seeds/VACUUM with no data change. Add a stable tiebreak (lowesteventIdor earliest event date) so the link is a deterministic property of the data. (finding #9)No ADR is owed; these are refinements within the chosen design.
Review concerns addressed (pushed
bf73d8d..bcf95e43, 13 atomic commits)All 10 automated findings and the persona blockers/concerns are resolved. Four were product/scope calls — decided with the author before implementing:
EventHeadernow.REQ-014/REQ-015 were added to the #850 issue body (the spec) + RTM (
179ada13).Blockers
a9027ceabuildEventLookupexcludes HISTORICAL → world events keep WorldBand, letters stay loose (REQ-014);eventClustering+TimelineViewtests0fc7ef5dYearBandinterleave test (loose Nov before/after Feb cluster)81e0dfb9Suggestions / concerns
putIfAbsentoverfindAll()order) (Arch-3)eventId; determinism unit test (order-independent)5a21843cgetDocuments()for all events (DevOps)b53198768cc11aec✉card4e704ae4✉+· {count}unlabeled for screen readers (UX-1)GlyphLabelfor the glyph; sr-only{count} Briefevia a new Paraglide key (de/en/es)30384fa5,a68f7ee5loose.includes()O(L²)splitYearLettersreturns itsbyEventmap; O(1) membership/lookup in row assembly70a76904EventClusterre-implementsEventPill's header + edit gate (Arch-1)canEditEvent()single-sources the security gate (EventPill/WorldBand/EventCluster); sharedEventHeadercomponentd450f97b,bcf95e43Verification (real runs)
TimelineServiceTest: 32 passed (was 30; +determinism, +filtered-out-event), clean compile / BUILD SUCCESS.svelte-check: no new errors in touched files; new.sveltevalidated through the Svelte MCP autofixer;npm run lintgreen (pre-commit) on every commit.Done; REQ-009 row now cites the determinism test.Consciously deferred (Tester suggestions, not blockers)
splitYearLettersunit level (lookup-presence gate). Happy to add theTimelineView-level test if you'd like it in this PR.@BatchSizepass like #835'sTagServiceIntegrationTest.A curated event with linked letters renders as one contained card: the event is the header (accent glyph, title, date · provenance, count, and a curator edit link), its letters sit inside as compact .lcard.ev cards. The first CLUSTER_PREVIEW (5) show, then a keyboard-operable show-more/less toggle reveals the rest. A cross-year card (no event prop) gets a plain '✉ title' text header with no edit link. Titles render through default {...} escaping. Adds timeline_bucket_show_more/less keys to de/en/es. Refs #850The bare "· {count}" spans in both the same-year and cross-year headers announced as "· 2" with no context. Each now pairs the aria-hidden visible count with an sr-only "{count} Briefe" via a new Paraglide key (timeline_cluster_letter_count, present in de/en/es). Fixes review finding #6 (count half). Refs #850 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>EventCluster's same-year header re-implemented EventPill's glyph circle, serif title, provenance subtitle, and the curator edit anchor near- verbatim — the third copy of that markup. They now share a single EventHeader component (glyph via GlyphLabel, title, `{date} · provenance` subtitle, optional sr-only letter count, and the canEditEvent-gated edit pencil); EventPill keeps only its pill border, EventCluster only its card chrome. Second half of review finding #5 (Architect-1). No behavior change — EventPill/EventCluster/YearBand/TimelineView specs stay green. Refs #850 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>bcf95e4399to621248f941