marcel/familienarchiv
1
0
Fork 0
Code Issues 119 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Timeline: per-person Lebensweg on Person detail #782

New Issue
Open
opened 2026-06-07 19:29:37 +02:00 by marcel · 8 comments
marcel commented 2026-06-07 19:29:37 +02:00
Owner
Copy Link

Milestone: Zeitstrahl — Family Timeline
Spec: docs/superpowers/specs/2026-06-07-family-timeline-design.md § "Concept & UX" / "Frontend"
Depends on (hard, blocked-by):

  • #1 Person date+precision migration — without birthDate/deathDate + precision, derived person-events (Geburt/Tod) are empty and the Lebensweg is empty.
  • #5 assembly endpoint with ?personId= filter — defines what "their events" means (see Data scope).
  • #7 <TimelineView> component — this issue only embeds it. #7 MUST ship with explicit ACs for both modes: personId-self-load AND pre-loaded timeline-prop embedding. The pre-loaded prop is required for SSR embedding here. Do not start this issue until #7's spec confirms both signatures are in its acceptance criteria.

Scope

Embed a Lebensweg timeline on the Person detail page by reusing the existing <TimelineView> filtered to that person. This is pure frontend composition — no new backend, no new endpoint, no new entity, no migration, no new route, no new env/infra. Per-person is just GET /api/timeline?personId=…; do not add a /api/persons/{id}/timeline shim. All timeline business logic stays in lib/timeline/; the Person domain depends on Timeline's published component, never the reverse.

Resolved Decisions

  1. Data flow: SSR prop (Option A). The timeline is loaded in persons/[id]/+page.server.ts as an additional parallel api.GET('/api/timeline', { params: { query: { personId: id } } }) call and passed down as a timeline prop. Rationale: consistent with every other card on the page (all data flows server → prop), fully SSR (no loading spinner, no client-side API exposure), and preserves the authenticated createApiClient(fetch) auth-cookie forwarding. Do NOT onMount(fetch('/api/timeline')) client-side — that breaks SSR auth-cookie forwarding (documented anti-pattern). The corresponding dual-input contract for <TimelineView> (accept a personId for self-load on the global route or pre-loaded data when embedded) is locked at #7 — design it there now, do not retrofit.

  2. Empty state: hide the card (default). For a person with zero dated items, the Lebensweg card is omitted entirely — mirrors the existing Geschichten {#if data.geschichten.length > 0} precedent. Rationale: cleanest page for the many persons who'll have only a birth year; zero cost; consistent UX. Never render an empty bordered card.

  3. English translation for person_lebensweg_heading: "Life Journey". Rationale: closest to the German literal meaning ("life's path/journey"), distinct from Geschichten (stories) above it, more personal than "Timeline" (too generic), less confusing than "Life Story" (too close to Geschichten). Spanish: "Camino de Vida". German primary: "Lebensweg".

  4. {#if} emptiness check: inspect actual entry counts, not just years.length. The assembly endpoint (#5) may legitimately return year bands with empty entries: [] arrays (e.g. during incremental assembly). The visibility guard must count real entries: {#if data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0}. Never years.length > 0 alone — that would show an empty bordered card when all year bands are empty.

  5. LebenswegCard.svelte wrapper: inline unless page exceeds ~100 template lines. The card chrome is 4 lines of Tailwind + a heading — not worth a separate file unless +page.svelte becomes unwieldy. Check actual line count when implementing; only extract if the template portion exceeds ~100 lines.

  6. No canWrite prop on the Lebensweg card. The card is read-only by spec. "Edit at source" links point to the Person/relationship edit screens which already enforce WRITE_ALL. Do not copy data.canBlogWrite (which is a pre-existing latent bug in line 107 of +page.svelte — canWrite={data.canBlogWrite ?? false} reads a non-existent field).

Data scope (asserted against the rendered embed; contract owned by #5)

A person's Lebensweg includes:

  • their derived events: Geburt, Tod, and Heirat (marriage is derived once per SPOUSE_OF edge and must surface on both spouses' Lebenswege);
  • hand-linked events where the person is in TimelineEvent.persons;
  • letters they sent or received (sender or receiver).

Entries are year-banded and use precision-aware date labels.

Cross-issue note for #5: TimelineService.getForPerson(UUID personId) should call PersonService.getById(personId) first (throwing 404 if absent), not silently return empty. This prevents non-existent personId values from returning an empty timeline to direct API callers. From the Person detail page this is harmless (page already 404s), but correct for the API contract.

Cross-issue note for #5 (AC3 symmetry): Add to #5's acceptance criteria: "Given persons A and B with a SPOUSE_OF relationship, GET /api/timeline?personId=A and GET /api/timeline?personId=B both return the Heirat event." If #5 doesn't test symmetry, this embed cannot reliably verify AC3.

UI / placement

  • Place the card in the right (65%) column, last — after Geschichten; it's the narrative payoff and the year bands need the wider column. On phones the grid stacks to one column.
  • Use the standard card chrome: rounded-sm border border-line bg-surface shadow-sm p-6, wrapped in the sibling-consistent <div class="mt-6">.
  • Section title "LEBENSWEG" via the standard section-title classes (text-xs font-bold uppercase tracking-widest text-ink-3 mb-5), rendered as the section <h2>.
  • A thin section wrapper component (card chrome + heading) may live in persons/[id]/ only if +page.svelte template exceeds ~100 lines after the addition — name it LebenswegCard.svelte (visible region name), not …Wrapper. Do not put timeline logic in it; entries render inside <TimelineView>. Default to inlining in +page.svelte.
  • Pass domain-named props only: <TimelineView personId={person.id} timeline={data.timeline} />. Never data/item/props.
  • No nested scroll container — the embedded timeline extends the page flow; do not make it an inner overflow-y-auto box (scroll-within-scroll traps 60+ phone users).
  • Typography: Person names and event titles within the timeline use font-serif (Tinos). Date label strings produced by dateLabel.ts (ca. 1914, Sommer 1914, Ohne Datum) are UI metadata and use font-sans (Montserrat). The "LEBENSWEG" heading is UI chrome — font-sans. Never apply font-serif to date labels or section headings.
  • A11y (heading hierarchy is this issue's responsibility since it owns the section wrapper): <h2> Lebensweg → year bands as <h3> under it; semantic list markup for entries; focus-visible:ring-2 focus-visible:ring-brand-navy on any "edit at source" links (do not rely on browser defaults). Within the timeline, person names and event titles use font-serif per the spec.
  • Date labels must use the precision-aware formatter from #6 (dateLabel.ts): ca. 1914, Sommer 1914, Ohne Datum — never a fabricated 01.01.1914 for a year-only date. The embed must not bypass it.
  • Any "edit at source" link on a derived event points to the Person / relationship edit screens (which already enforce WRITE_ALL), not a timeline-event editor. Curator event edit/delete is out of scope here (that's #9).
  • Long-name overflow: Letter rows showing "sender → receiver + snippet" can overflow with long German names. When testing at 375px, verify the LetterCard component (from #7) truncates with truncate or line-clamp. If it doesn't, flag against #7.

i18n

Key de en es
person_lebensweg_heading Lebensweg Life Journey Camino de Vida

German is primary per project convention.

Security

Read-only re-projection of data already authorized for display on this page (birth/death dates, letter sender/receiver names already appear via PersonCard, PersonDocumentList, CoCorrespondentsList). Permissions are global (READ_ALL), not per-record — no new authorization boundary, no IDOR concern, no @RequirePermission work. A 403/503 on the timeline sub-fetch must not surface a raw backend message — fall back to an empty timeline (?? { years: [], undated: [] }), never result.data!. The graceful-degradation fallback correctly hides the failure from the user.

Acceptance criteria

  1. Given a person with ≥1 dated item, when I open their detail page, then a "Lebensweg" card appears showing only that person's birth/death/marriage derived events, hand-linked events, and sent+received letters, year-banded.
  2. Given a person with no dated items (or all year bands having empty entry arrays), then the Lebensweg card is hidden — never an empty bordered card.
  3. Given a marriage between A and B, then the same Heirat event appears on both A's and B's Lebensweg. (Depends on #5 verifying symmetric assembly — see cross-issue note above.)
  4. The section renders at 375px without horizontal scroll; year-band labels, date labels and letter rows (sender → receiver + snippet) do not overflow. Long names are truncated, not clipped by viewport.
  5. Heading uses i18n key person_lebensweg_heading in de/en/es with translations "Lebensweg" / "Life Journey" / "Camino de Vida".
  6. A failed/forbidden/slow timeline sub-fetch degrades only the Lebensweg section (hidden/empty), never the whole page; the rest of the person page renders normally.

Tasks

  • Add the timeline call to persons/[id]/+page.server.ts Promise.all (8th parallel api.GET('/api/timeline', { params: { query: { personId: id } } })); default to { years: [], undated: [] } on !response.ok; expose as data.timeline.
  • Add the Lebensweg card section to persons/[id]/+page.svelte (right column, last), conditionally rendered with {#if data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0}, embedding <TimelineView personId={person.id} timeline={data.timeline} />. Only extract LebenswegCard.svelte if template exceeds ~100 lines.
  • Add i18n key person_lebensweg_heading in messages/{de,en,es}.json: de="Lebensweg", en="Life Journey", es="Camino de Vida".
  • Ensure heading hierarchy (h2 Lebensweg → h3 year) and no nested scroll container.
  • Apply focus-visible:ring-2 focus-visible:ring-brand-navy to any "edit at source" links.
  • Verify dateLabel.ts output strings render in font-sans, not font-serif.

Tests

The previously-cited "TimelineView in personId mode renders only that person's entries" is a unit test of TimelineView's filtering and belongs to #7 (or #5's filter), not here. This issue tests the embed + load integration.

  • Server load (persons/[id]/+page.server.spec.ts, Vitest node):

    • The timeline call is included in Promise.all and data.timeline is returned on success (assert shape).
    • personResult failure still 404s the page (existing behavior unchanged by the new call).
    • Timeline sub-fetch failure (response.ok = false) → page still loads, data.timeline deep-equals { years: [], undated: [] } (assert the exact shape, not just that no exception was thrown).
    • Important: The existing page.server.spec.ts uses 7 mockResolvedValueOnce chains. Adding an 8th means every existing test in that file must get an additional .mockResolvedValueOnce({ response: { ok: true }, data: { years: [], undated: [] } }) for the timeline call. Failing to do this will cause the 8th slot to return undefined and silently mask regressions. Do a systematic pass over all existing tests and update every chain. Flag this in the PR description.

  • Component (persons/[id]/page.svelte.test.ts, vitest-browser-svelte, real DOM):

    • Lebensweg section present (getByRole('heading', { name: /lebensweg/i })) when data.timeline has entries.
    • Lebensweg section absent when timeline is empty or all year bands have empty entry arrays (mirror the Geschichten {#if length > 0} pattern).
    • <TimelineView> receives personId={person.id} — assert via a rendered person-scoped entry, not by inspecting props.

  • Test factories:

    • makeTimeline(overrides?) — defaults to { years: [], undated: [] } (empty state).
    • makeTimelineWithEntries() — returns one populated year band with at least one entry, so populated-state tests stay one-liners without constructing a 10-field DTO inline.

  • No new Playwright E2E — the per-person timeline is a re-render of the global timeline (covered by #7/#11). The axe/dark-mode a11y pass on /persons/[id] belongs to #11's polish.

  • Targeted single-file runs locally (browser specs ~3s); full sweep to CI.

Ops note

The 8th parallel call rides the existing Spring Boot trace/metrics instrumentation — no new observability work. If, once #5 lands, GET /api/timeline?personId= p95 is materially slower than the other person sub-fetches (it can be the heaviest read on the page and gates first paint when slowest), surface it back to #5 as an indexing problem (documents.document_date, timeline_event_persons.person_id) — do not paper over it with a client-side lazy-load here.

Post-merge monitoring: After #5 + #10 land, check the Spring Boot request-duration histogram: http_server_requests_seconds{uri="/api/timeline", quantile="0.95"}. The p95 for ?personId= should stay under 500ms (matching the existing load test gate). If it exceeds it, that is an index ticket for #5, not a lazy-load workaround here.

**Milestone:** Zeitstrahl — Family Timeline **Spec:** `docs/superpowers/specs/2026-06-07-family-timeline-design.md` § "Concept & UX" / "Frontend" **Depends on (hard, blocked-by):** - **#1** Person date+precision migration — without `birthDate`/`deathDate` + precision, derived person-events (Geburt/Tod) are empty and the Lebensweg is empty. - **#5** assembly endpoint with `?personId=` filter — defines what "their events" means (see Data scope). - **#7** `<TimelineView>` component — this issue only embeds it. **#7 MUST ship with explicit ACs for both modes: `personId`-self-load AND pre-loaded `timeline`-prop embedding. The pre-loaded prop is required for SSR embedding here. Do not start this issue until #7's spec confirms both signatures are in its acceptance criteria.** ## Scope Embed a **Lebensweg** timeline on the Person detail page by reusing the existing `<TimelineView>` filtered to that person. This is **pure frontend composition** — no new backend, no new endpoint, no new entity, no migration, no new route, no new env/infra. Per-person is just `GET /api/timeline?personId=…`; do **not** add a `/api/persons/{id}/timeline` shim. All timeline business logic stays in `lib/timeline/`; the Person domain depends on Timeline's published component, never the reverse. ## Resolved Decisions 1. **Data flow: SSR prop (Option A).** The timeline is loaded in `persons/[id]/+page.server.ts` as an additional parallel `api.GET('/api/timeline', { params: { query: { personId: id } } })` call and passed down as a `timeline` prop. Rationale: consistent with every other card on the page (all data flows server → prop), fully SSR (no loading spinner, no client-side API exposure), and preserves the authenticated `createApiClient(fetch)` auth-cookie forwarding. **Do NOT** `onMount(fetch('/api/timeline'))` client-side — that breaks SSR auth-cookie forwarding (documented anti-pattern). The corresponding dual-input contract for `<TimelineView>` (accept a `personId` for self-load on the global route **or** pre-loaded data when embedded) is locked at **#7** — design it there now, do not retrofit. 2. **Empty state: hide the card (default).** For a person with zero dated items, the Lebensweg card is omitted entirely — mirrors the existing Geschichten `{#if data.geschichten.length > 0}` precedent. Rationale: cleanest page for the many persons who'll have only a birth year; zero cost; consistent UX. Never render an empty bordered card. 3. **English translation for `person_lebensweg_heading`: "Life Journey".** Rationale: closest to the German literal meaning ("life's path/journey"), distinct from Geschichten (stories) above it, more personal than "Timeline" (too generic), less confusing than "Life Story" (too close to Geschichten). Spanish: "Camino de Vida". German primary: "Lebensweg". 4. **`{#if}` emptiness check: inspect actual entry counts, not just `years.length`.** The assembly endpoint (#5) may legitimately return year bands with empty `entries: []` arrays (e.g. during incremental assembly). The visibility guard must count real entries: `{#if data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0}`. Never `years.length > 0` alone — that would show an empty bordered card when all year bands are empty. 5. **`LebenswegCard.svelte` wrapper: inline unless page exceeds ~100 template lines.** The card chrome is 4 lines of Tailwind + a heading — not worth a separate file unless `+page.svelte` becomes unwieldy. Check actual line count when implementing; only extract if the template portion exceeds ~100 lines. 6. **No `canWrite` prop on the Lebensweg card.** The card is read-only by spec. "Edit at source" links point to the Person/relationship edit screens which already enforce `WRITE_ALL`. Do not copy `data.canBlogWrite` (which is a pre-existing latent bug in line 107 of `+page.svelte` — `canWrite={data.canBlogWrite ?? false}` reads a non-existent field). ## Data scope (asserted against the rendered embed; contract owned by #5) A person's Lebensweg includes: - their **derived** events: Geburt, Tod, and Heirat (marriage is derived once per `SPOUSE_OF` edge and must surface on **both** spouses' Lebenswege); - **hand-linked** events where the person is in `TimelineEvent.persons`; - **letters** they **sent or received** (sender or receiver). Entries are year-banded and use precision-aware date labels. **Cross-issue note for #5:** `TimelineService.getForPerson(UUID personId)` should call `PersonService.getById(personId)` first (throwing 404 if absent), not silently return empty. This prevents non-existent `personId` values from returning an empty timeline to direct API callers. From the Person detail page this is harmless (page already 404s), but correct for the API contract. **Cross-issue note for #5 (AC3 symmetry):** Add to #5's acceptance criteria: "Given persons A and B with a `SPOUSE_OF` relationship, `GET /api/timeline?personId=A` and `GET /api/timeline?personId=B` both return the Heirat event." If #5 doesn't test symmetry, this embed cannot reliably verify AC3. ## UI / placement - Place the card in the **right (65%) column, last** — after Geschichten; it's the narrative payoff and the year bands need the wider column. On phones the grid stacks to one column. - Use the standard card chrome: `rounded-sm border border-line bg-surface shadow-sm p-6`, wrapped in the sibling-consistent `<div class="mt-6">`. - Section title **"LEBENSWEG"** via the standard section-title classes (`text-xs font-bold uppercase tracking-widest text-ink-3 mb-5`), rendered as the section `<h2>`. - A thin section wrapper component (card chrome + heading) may live in `persons/[id]/` **only if** `+page.svelte` template exceeds ~100 lines after the addition — name it `LebenswegCard.svelte` (visible region name), **not** `…Wrapper`. Do **not** put timeline logic in it; entries render inside `<TimelineView>`. Default to inlining in `+page.svelte`. - Pass domain-named props only: `<TimelineView personId={person.id} timeline={data.timeline} />`. Never `data`/`item`/`props`. - **No nested scroll container** — the embedded timeline extends the page flow; do not make it an inner `overflow-y-auto` box (scroll-within-scroll traps 60+ phone users). - **Typography:** Person names and event titles within the timeline use `font-serif` (Tinos). Date label strings produced by `dateLabel.ts` (`ca. 1914`, `Sommer 1914`, `Ohne Datum`) are UI metadata and use `font-sans` (Montserrat). The "LEBENSWEG" heading is UI chrome — `font-sans`. Never apply `font-serif` to date labels or section headings. - **A11y (heading hierarchy is this issue's responsibility since it owns the section wrapper):** `<h2>` Lebensweg → year bands as `<h3>` under it; semantic list markup for entries; `focus-visible:ring-2 focus-visible:ring-brand-navy` on any "edit at source" links (do not rely on browser defaults). Within the timeline, person names and event titles use `font-serif` per the spec. - **Date labels** must use the precision-aware formatter from #6 (`dateLabel.ts`): `ca. 1914`, `Sommer 1914`, `Ohne Datum` — never a fabricated `01.01.1914` for a year-only date. The embed must not bypass it. - Any "edit at source" link on a derived event points to the Person / relationship edit screens (which already enforce `WRITE_ALL`), not a timeline-event editor. Curator event edit/delete is out of scope here (that's #9). - **Long-name overflow:** Letter rows showing "sender → receiver + snippet" can overflow with long German names. When testing at 375px, verify the `LetterCard` component (from #7) truncates with `truncate` or `line-clamp`. If it doesn't, flag against #7. ## i18n | Key | de | en | es | |---|---|---|---| | `person_lebensweg_heading` | `Lebensweg` | `Life Journey` | `Camino de Vida` | German is primary per project convention. ## Security Read-only re-projection of data already authorized for display on this page (birth/death dates, letter sender/receiver names already appear via `PersonCard`, `PersonDocumentList`, `CoCorrespondentsList`). Permissions are global (`READ_ALL`), not per-record — no new authorization boundary, no IDOR concern, no `@RequirePermission` work. A 403/503 on the timeline sub-fetch must **not** surface a raw backend message — fall back to an empty timeline (`?? { years: [], undated: [] }`), never `result.data!`. The graceful-degradation fallback correctly hides the failure from the user. ## Acceptance criteria 1. Given a person with ≥1 dated item, when I open their detail page, then a "Lebensweg" card appears showing only that person's birth/death/marriage derived events, hand-linked events, and sent+received letters, year-banded. 2. Given a person with no dated items (or all year bands having empty entry arrays), then the Lebensweg card is **hidden** — never an empty bordered card. 3. Given a marriage between A and B, then the same Heirat event appears on **both** A's and B's Lebensweg. _(Depends on #5 verifying symmetric assembly — see cross-issue note above.)_ 4. The section renders at **375px** without horizontal scroll; year-band labels, date labels and letter rows (sender → receiver + snippet) do not overflow. Long names are truncated, not clipped by viewport. 5. Heading uses i18n key `person_lebensweg_heading` in de/en/es with translations "Lebensweg" / "Life Journey" / "Camino de Vida". 6. A failed/forbidden/slow timeline sub-fetch degrades only the Lebensweg section (hidden/empty), never the whole page; the rest of the person page renders normally. ## Tasks - [ ] Add the timeline call to `persons/[id]/+page.server.ts` `Promise.all` (8th parallel `api.GET('/api/timeline', { params: { query: { personId: id } } })`); default to `{ years: [], undated: [] }` on `!response.ok`; expose as `data.timeline`. - [ ] Add the **Lebensweg** card section to `persons/[id]/+page.svelte` (right column, last), conditionally rendered with `{#if data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0}`, embedding `<TimelineView personId={person.id} timeline={data.timeline} />`. Only extract `LebenswegCard.svelte` if template exceeds ~100 lines. - [ ] Add i18n key `person_lebensweg_heading` in `messages/{de,en,es}.json`: de="Lebensweg", en="Life Journey", es="Camino de Vida". - [ ] Ensure heading hierarchy (`h2` Lebensweg → `h3` year) and no nested scroll container. - [ ] Apply `focus-visible:ring-2 focus-visible:ring-brand-navy` to any "edit at source" links. - [ ] Verify `dateLabel.ts` output strings render in `font-sans`, not `font-serif`. ## Tests > The previously-cited "`TimelineView` in `personId` mode renders only that person's entries" is a unit test of `TimelineView`'s filtering and belongs to **#7** (or #5's filter), **not** here. This issue tests the **embed + load integration**. - **Server load (`persons/[id]/+page.server.spec.ts`, Vitest node):** - The timeline call is included in `Promise.all` and `data.timeline` is returned on success (assert shape). - `personResult` failure still 404s the page (existing behavior unchanged by the new call). - Timeline sub-fetch failure (`response.ok = false`) → page still loads, `data.timeline` deep-equals `{ years: [], undated: [] }` (assert the exact shape, not just that no exception was thrown). - **Important:** The existing `page.server.spec.ts` uses 7 `mockResolvedValueOnce` chains. Adding an 8th means **every existing test** in that file must get an additional `.mockResolvedValueOnce({ response: { ok: true }, data: { years: [], undated: [] } })` for the timeline call. Failing to do this will cause the 8th slot to return `undefined` and silently mask regressions. Do a systematic pass over all existing tests and update every chain. Flag this in the PR description. - **Component (`persons/[id]/page.svelte.test.ts`, `vitest-browser-svelte`, real DOM):** - Lebensweg section present (`getByRole('heading', { name: /lebensweg/i })`) when `data.timeline` has entries. - Lebensweg section **absent** when timeline is empty or all year bands have empty entry arrays (mirror the Geschichten `{#if length > 0}` pattern). - `<TimelineView>` receives `personId={person.id}` — assert via a rendered person-scoped entry, not by inspecting props. - **Test factories:** - `makeTimeline(overrides?)` — defaults to `{ years: [], undated: [] }` (empty state). - `makeTimelineWithEntries()` — returns one populated year band with at least one entry, so populated-state tests stay one-liners without constructing a 10-field DTO inline. - **No new Playwright E2E** — the per-person timeline is a re-render of the global timeline (covered by #7/#11). The axe/dark-mode a11y pass on `/persons/[id]` belongs to #11's polish. - Targeted single-file runs locally (browser specs ~3s); full sweep to CI. ## Ops note The 8th parallel call rides the existing Spring Boot trace/metrics instrumentation — no new observability work. If, once #5 lands, `GET /api/timeline?personId=` p95 is materially slower than the other person sub-fetches (it can be the heaviest read on the page and gates first paint when slowest), surface it back to **#5** as an indexing problem (`documents.document_date`, `timeline_event_persons.person_id`) — do **not** paper over it with a client-side lazy-load here. **Post-merge monitoring:** After #5 + #10 land, check the Spring Boot request-duration histogram: `http_server_requests_seconds{uri="/api/timeline", quantile="0.95"}`. The p95 for `?personId=` should stay under 500ms (matching the existing load test gate). If it exceeds it, that is an index ticket for #5, not a lazy-load workaround here.
marcel added this to the Zeitstrahl — Family Timeline milestone 2026-06-07 19:29:37 +02:00
marcel added the P3-laterfeaturepersonui labels 2026-06-07 19:30:06 +02:00
marcel commented 2026-06-07 19:52:39 +02:00
Author
Owner
Copy Link

🏛️ Markus Keller — Application Architect

Observations

This issue is frontend composition only — and it is well-scoped.
The "no new backend, no new endpoint" constraint is correct and should not be relaxed. The per-person view is GET /api/timeline?personId=…, which is already the right endpoint shape given that GET /api/timeline accepts personId as an optional filter.

ADR gap — but this issue may not be the right place to write it.
The milestone spec (§ "An ADR may be warranted for the new timeline/ domain + entity") flags that an ADR belongs somewhere in this milestone. Issue #10 (this issue) is a pure frontend composition task; the ADR belongs in issues #2/#3 where the timeline/ backend package is introduced. Confirm that the ADR is in-flight there. If neither #2 nor #3 have opened that ADR, it must happen before #10 is merged — the new package is already a structural fact by the time #10 lands.

Domain dependency direction is correct.
The spec states: "All timeline business logic stays in lib/timeline/; the Person domain depends on Timeline's published component, never the reverse." This aligns with the project's domain boundary rules. However, <TimelineView> receiving personId={person.id} must not import anything from $lib/person/ — direction must stay Person → Timeline, not bidirectional. Verify in the PR that TimelineView.svelte and its sub-components have zero imports from $lib/person/.

Documentation requirements triggered by this issue:
Per the doc-update table, this issue adds a new card section (not a new route), but it touches persons/[id]/+page.svelte and embeds a component from a new domain (lib/timeline/). Required updates:

  • docs/architecture/c4/l3-frontend-3c-people-stories.puml — add the Lebensweg card + TimelineView reference
  • CLAUDE.md — the route table entry for persons/[id] should note the timeline embed once the /zeitstrahl route is also added (that belongs to #7, but the person page reference should be consistent)
  • docs/GLOSSARY.md — "Lebensweg" is a new domain term; add it alongside "Zeitstrahl"

No new Docker service, no new ErrorCode, no new Permission — those parts of the doc table are silent for this issue.

The canBlogWrite latent bug.
The issue body correctly identifies the canWrite={data.canBlogWrite ?? false} on line 107 of +page.svelte as a pre-existing bug — canBlogWrite is not exposed by +page.server.ts. The Lebensweg card is read-only so it does not need a canWrite prop at all, which is the right call. Do not silently propagate the bug; the PR description should note it explicitly (it is not the issue's responsibility to fix it, but it should not be made worse).

The 8th Promise.all slot and test chains.
The issue body already flags this in detail — every existing mockResolvedValueOnce chain in page.server.spec.ts gains an 8th slot. I counted 5 test cases in that file, each with a 7-chain mock; the issue correctly requires updating all 5. This is a genuine regression risk if missed. The PR description must call this out explicitly so the reviewer checks.

Recommendations

  • Before implementing, confirm that ADR-035 (or next sequential number) has been drafted in issues #2/#3 covering the timeline/ domain. Flag it in the PR if absent.
  • Add a docs/GLOSSARY.md entry for "Lebensweg" (German: life's path; the per-person chronological view on the Person detail page) alongside the "Zeitstrahl" entry that will come from #7.
  • Update l3-frontend-3c-people-stories.puml to add the TimelineView embed reference on the persons/[id] page.
  • In the PR, add a checklist item: "Confirmed TimelineView imports zero symbols from $lib/person/."
## 🏛️ Markus Keller — Application Architect ### Observations **This issue is frontend composition only — and it is well-scoped.** The "no new backend, no new endpoint" constraint is correct and should not be relaxed. The per-person view is `GET /api/timeline?personId=…`, which is already the right endpoint shape given that `GET /api/timeline` accepts `personId` as an optional filter. **ADR gap — but this issue may not be the right place to write it.** The milestone spec (§ "An ADR may be warranted for the new `timeline/` domain + entity") flags that an ADR belongs somewhere in this milestone. Issue #10 (this issue) is a pure frontend composition task; the ADR belongs in issues #2/#3 where the `timeline/` backend package is introduced. Confirm that the ADR is in-flight there. If neither #2 nor #3 have opened that ADR, it must happen before #10 is merged — the new package is already a structural fact by the time #10 lands. **Domain dependency direction is correct.** The spec states: "All timeline business logic stays in `lib/timeline/`; the Person domain depends on Timeline's published component, never the reverse." This aligns with the project's domain boundary rules. However, `<TimelineView>` receiving `personId={person.id}` must **not** import anything from `$lib/person/` — direction must stay Person → Timeline, not bidirectional. Verify in the PR that `TimelineView.svelte` and its sub-components have zero imports from `$lib/person/`. **Documentation requirements triggered by this issue:** Per the doc-update table, this issue adds a new card section (not a new route), but it touches `persons/[id]/+page.svelte` and embeds a component from a new domain (`lib/timeline/`). Required updates: - `docs/architecture/c4/l3-frontend-3c-people-stories.puml` — add the Lebensweg card + TimelineView reference - `CLAUDE.md` — the route table entry for `persons/[id]` should note the timeline embed once the `/zeitstrahl` route is also added (that belongs to #7, but the person page reference should be consistent) - `docs/GLOSSARY.md` — "Lebensweg" is a new domain term; add it alongside "Zeitstrahl" No new Docker service, no new `ErrorCode`, no new `Permission` — those parts of the doc table are silent for this issue. **The `canBlogWrite` latent bug.** The issue body correctly identifies the `canWrite={data.canBlogWrite ?? false}` on line 107 of `+page.svelte` as a pre-existing bug — `canBlogWrite` is not exposed by `+page.server.ts`. The Lebensweg card is read-only so it does not need a `canWrite` prop at all, which is the right call. Do not silently propagate the bug; the PR description should note it explicitly (it is not the issue's responsibility to fix it, but it should not be made worse). **The 8th `Promise.all` slot and test chains.** The issue body already flags this in detail — every existing `mockResolvedValueOnce` chain in `page.server.spec.ts` gains an 8th slot. I counted 5 test cases in that file, each with a 7-chain mock; the issue correctly requires updating all 5. This is a genuine regression risk if missed. The PR description must call this out explicitly so the reviewer checks. ### Recommendations - Before implementing, confirm that ADR-035 (or next sequential number) has been drafted in issues #2/#3 covering the `timeline/` domain. Flag it in the PR if absent. - Add a `docs/GLOSSARY.md` entry for "Lebensweg" (German: life's path; the per-person chronological view on the Person detail page) alongside the "Zeitstrahl" entry that will come from #7. - Update `l3-frontend-3c-people-stories.puml` to add the `TimelineView` embed reference on the persons/[id] page. - In the PR, add a checklist item: "Confirmed `TimelineView` imports zero symbols from `$lib/person/`."
marcel commented 2026-06-07 19:53:02 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The existing +page.server.ts has 7 parallel calls; this adds an 8th.
I read the actual file. The current Promise.all destructures 7 results. The issue correctly mandates adding the timeline call as the 8th. The key implementation point: the timeline result must use ?? { years: [], undated: [] } (not ?? []), because the TypeScript types from #6 will type data.timeline as TimelineDTO, not an array. Using the wrong fallback will cause a type error at the {#if data.timeline.years.some(...)} call site.

The emptiness guard expression is non-trivial — extract it.
The issue mandates: {#if data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0}. This is business logic in template markup (rule violation from my style guide). Extract it as a $derived in +page.svelte:

const hasLebenswegEntries = $derived(
  data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0
);

Then the template reads {#if hasLebenswegEntries}. This also makes the component test straightforward to read.

Inline vs. LebenswegCard.svelte threshold.
The current +page.svelte is 113 lines. Adding the Lebensweg card section will bring it to approximately 123-130 lines depending on the card markup. The issue sets the threshold at ~100 template lines. Since the script block is ~50 lines and the template is ~63 lines, adding 12-15 lines of template puts the template portion at ~75-78 lines — under the 100-line split threshold. Keep it inline; do not extract. Recheck at PR time: if the hasLebenswegEntries derived and the card markup push template past 100, extract then.

Component test: assert behavior, not props.
The issue correctly states: "<TimelineView> receives personId={person.id} — assert via a rendered person-scoped entry, not by inspecting props." This is the right approach. In the page.svelte.test.ts, the test should render with a data.timeline that has a populated entry and assert the heading appears — that confirms the composition works. Do not try to inspect Svelte internal props.

The baseData factory in page.svelte.test.ts needs updating.
The existing baseData() factory (line 16-37) does not include a timeline field. Every existing test will need timeline: { years: [], undated: [] } added to baseData's defaults so existing tests do not break when the prop is required. This is the component-test parallel of the 8th mock-slot problem.

Test factory naming.
The issue mandates makeTimeline() and makeTimelineWithEntries(). Good names. Place them in persons/[id]/ alongside the page test, or in a shared $lib/timeline/testUtils.ts if they're reused by TimelineView tests in #7. Do not duplicate.

i18n key spelling precision.
The key is person_lebensweg_heading. The value in de.json is "Lebensweg" (capital L, German noun). Confirm en.json gets "Life Journey" and es.json gets "Camino de Vida" — all three must be added atomically in one commit.

Recommendations

  • Extract the emptiness check to const hasLebenswegEntries = $derived(...) before using it in the template.
  • Update baseData() defaults in page.svelte.test.ts to include timeline: { years: [], undated: [] }.
  • Place test factories in persons/[id]/ unless #7's TimelineView tests also need them — in that case, create $lib/timeline/testUtils.ts.
  • Confirm data.timeline fallback uses { years: [], undated: [] } not [] or null.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations **The existing `+page.server.ts` has 7 parallel calls; this adds an 8th.** I read the actual file. The current `Promise.all` destructures 7 results. The issue correctly mandates adding the timeline call as the 8th. The key implementation point: the timeline result must use `?? { years: [], undated: [] }` (not `?? []`), because the TypeScript types from `#6` will type `data.timeline` as `TimelineDTO`, not an array. Using the wrong fallback will cause a type error at the `{#if data.timeline.years.some(...)}` call site. **The emptiness guard expression is non-trivial — extract it.** The issue mandates: `{#if data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0}`. This is business logic in template markup (rule violation from my style guide). Extract it as a `$derived` in `+page.svelte`: ```svelte const hasLebenswegEntries = $derived( data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0 ); ``` Then the template reads `{#if hasLebenswegEntries}`. This also makes the component test straightforward to read. **Inline vs. `LebenswegCard.svelte` threshold.** The current `+page.svelte` is 113 lines. Adding the Lebensweg card section will bring it to approximately 123-130 lines depending on the card markup. The issue sets the threshold at ~100 template lines. Since the script block is ~50 lines and the template is ~63 lines, adding 12-15 lines of template puts the template portion at ~75-78 lines — under the 100-line split threshold. Keep it inline; do not extract. Recheck at PR time: if the `hasLebenswegEntries` derived and the card markup push template past 100, extract then. **Component test: assert behavior, not props.** The issue correctly states: "`<TimelineView>` receives `personId={person.id}` — assert via a rendered person-scoped entry, not by inspecting props." This is the right approach. In the `page.svelte.test.ts`, the test should render with a `data.timeline` that has a populated entry and assert the heading appears — that confirms the composition works. Do not try to inspect Svelte internal props. **The `baseData` factory in `page.svelte.test.ts` needs updating.** The existing `baseData()` factory (line 16-37) does not include a `timeline` field. Every existing test will need `timeline: { years: [], undated: [] }` added to `baseData`'s defaults so existing tests do not break when the prop is required. This is the component-test parallel of the 8th mock-slot problem. **Test factory naming.** The issue mandates `makeTimeline()` and `makeTimelineWithEntries()`. Good names. Place them in `persons/[id]/` alongside the page test, or in a shared `$lib/timeline/testUtils.ts` if they're reused by `TimelineView` tests in #7. Do not duplicate. **i18n key spelling precision.** The key is `person_lebensweg_heading`. The value in `de.json` is `"Lebensweg"` (capital L, German noun). Confirm `en.json` gets `"Life Journey"` and `es.json` gets `"Camino de Vida"` — all three must be added atomically in one commit. ### Recommendations - Extract the emptiness check to `const hasLebenswegEntries = $derived(...)` before using it in the template. - Update `baseData()` defaults in `page.svelte.test.ts` to include `timeline: { years: [], undated: [] }`. - Place test factories in `persons/[id]/` unless #7's `TimelineView` tests also need them — in that case, create `$lib/timeline/testUtils.ts`. - Confirm `data.timeline` fallback uses `{ years: [], undated: [] }` not `[]` or `null`.
marcel commented 2026-06-07 19:53:21 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

No new attack surface — the security model here is correct.
The issue correctly identifies this as "read-only re-projection of data already authorized for display on this page." The timeline data is fetched in +page.server.ts using createApiClient(fetch), which forwards the auth cookie server-side. This is the established, correct pattern — it is not a new API exposure to the browser.

The personId query parameter is user-controlled input.
GET /api/timeline?personId={person.id} — the personId is the UUID from the already-validated person record (it comes from personResult.data!.id after the personResult.response.ok guard). This is fine: the UUID is not user-typed input, it is the server's own response. No injection risk here.

The graceful-degradation fallback is secure.
The issue mandates: a 403/503 on the timeline sub-fetch must fall back to { years: [], undated: [] }, never result.data!. This is correct — result.data! after a non-ok response would be undefined, forcing a runtime crash. The ?? { years: [], undated: [] } pattern is both safe and correct. Confirm the implementation uses timelineResult.data ?? { years: [], undated: [] } with the !result.response.ok check, not the result.error check (which breaks when the spec has no error responses defined).

No IDOR risk in this issue.
Permissions are global (READ_ALL), not per-record. There is no per-person access control on timeline data — if you can see the person page, you can see their timeline. The 403 fallback handles the edge case where the timeline endpoint returns a 403 (e.g., misconfigured role) without exposing a raw backend error message to the user.

Cross-issue note for #5's security posture.
The issue documents: "TimelineService.getForPerson(UUID personId) should call PersonService.getById(personId) first (throwing 404 if absent)." From this issue's perspective, that 404 is harmless because the person page already 404s before the timeline fetch is attempted. However, for direct API callers (someone manually calling GET /api/timeline?personId=<nonexistent-uuid>), silently returning empty instead of 404 is an information disclosure concern — an attacker could enumerate valid UUIDs by checking whether they get an empty or 404 response. The issue already flagged this correctly. Ensure #5 implements the getById pre-check.

No new @RequirePermission annotation is needed here.
The issue is correct: this is a GET that reuses the existing READ_ALL permission on the timeline endpoint. No additional authorization work belongs in this issue.

Recommendations

  • Confirm implementation uses timelineResult.data ?? { years: [], undated: [] } (not result.error) to guard the fallback.
  • Add a page.server.spec.ts test asserting the exact shape { years: [], undated: [] } when the timeline sub-fetch returns ok: false — the issue already mandates this, but make it explicit that the assertion is toEqual({ years: [], undated: [] }), not just toBeDefined().
  • No security blockers in this issue. The implementation inherits the correct security model from the existing page.
## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations **No new attack surface — the security model here is correct.** The issue correctly identifies this as "read-only re-projection of data already authorized for display on this page." The timeline data is fetched in `+page.server.ts` using `createApiClient(fetch)`, which forwards the auth cookie server-side. This is the established, correct pattern — it is not a new API exposure to the browser. **The `personId` query parameter is user-controlled input.** `GET /api/timeline?personId={person.id}` — the `personId` is the UUID from the already-validated person record (it comes from `personResult.data!.id` after the `personResult.response.ok` guard). This is fine: the UUID is not user-typed input, it is the server's own response. No injection risk here. **The graceful-degradation fallback is secure.** The issue mandates: a 403/503 on the timeline sub-fetch must fall back to `{ years: [], undated: [] }`, never `result.data!`. This is correct — `result.data!` after a non-ok response would be `undefined`, forcing a runtime crash. The `?? { years: [], undated: [] }` pattern is both safe and correct. Confirm the implementation uses `timelineResult.data ?? { years: [], undated: [] }` with the `!result.response.ok` check, not the `result.error` check (which breaks when the spec has no error responses defined). **No IDOR risk in this issue.** Permissions are global (`READ_ALL`), not per-record. There is no per-person access control on timeline data — if you can see the person page, you can see their timeline. The 403 fallback handles the edge case where the timeline endpoint returns a 403 (e.g., misconfigured role) without exposing a raw backend error message to the user. **Cross-issue note for #5's security posture.** The issue documents: "`TimelineService.getForPerson(UUID personId)` should call `PersonService.getById(personId)` first (throwing 404 if absent)." From this issue's perspective, that 404 is harmless because the person page already 404s before the timeline fetch is attempted. However, for direct API callers (someone manually calling `GET /api/timeline?personId=<nonexistent-uuid>`), silently returning empty instead of 404 is an information disclosure concern — an attacker could enumerate valid UUIDs by checking whether they get an empty or 404 response. The issue already flagged this correctly. Ensure #5 implements the `getById` pre-check. **No new `@RequirePermission` annotation is needed here.** The issue is correct: this is a GET that reuses the existing `READ_ALL` permission on the timeline endpoint. No additional authorization work belongs in this issue. ### Recommendations - Confirm implementation uses `timelineResult.data ?? { years: [], undated: [] }` (not `result.error`) to guard the fallback. - Add a `page.server.spec.ts` test asserting the exact shape `{ years: [], undated: [] }` when the timeline sub-fetch returns `ok: false` — the issue already mandates this, but make it explicit that the assertion is `toEqual({ years: [], undated: [] })`, not just `toBeDefined()`. - No security blockers in this issue. The implementation inherits the correct security model from the existing page.
marcel commented 2026-06-07 19:53:43 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The test plan is detailed and correct — but has execution traps.

Trap 1: The 8th mock slot is a silent failure risk.
I read page.server.spec.ts. It has 5 test cases, each using a mockResolvedValueOnce chain of exactly 7 calls. When you add the 8th Promise.all slot, each existing chain returns undefined for the 8th call. The current +page.server.ts does ?? [] / ?? {} fallbacks, so the tests still pass — but data.timeline will be undefined in all 5 existing tests, masking the regression. This is the exact failure mode the issue warns about.

The fix is not just adding a chain item to every test — you must verify the test is actually asserting data.timeline:

  • Tests that only assert result.person.firstName or result.sentDocuments will pass even if data.timeline is broken.
  • Add at least one assertion per happy-path test that result.timeline deep-equals the mock value.
  • The "timeline sub-fetch failure" test must assert result.timeline equals { years: [], undated: [] } exactly (not just that no exception is thrown).

Trap 2: baseData() in page.svelte.test.ts needs timeline in defaults.
I read the current page.svelte.test.ts. The baseData() factory (line 16-37) does not include timeline. When the component prop is added, TypeScript will reject tests that use baseData() without timeline. Every existing test breaks at the type level. Fix: add timeline: { years: [], undated: [] } to baseData() defaults.

Component test for "absent when empty" is non-trivial.
The Lebensweg section must be absent when data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0 is false. The component test must cover three subcases:

  1. timeline: { years: [], undated: [] } — fully empty.
  2. timeline: { years: [{ year: 1920, entries: [] }], undated: [] } — year band with empty entries.
  3. timeline: { years: [], undated: [{ ... }] } — undated entries only (should render the card).

The issue spec calls out subcase 2 explicitly as the "fabricated year bands" edge case. Do not skip it.

The makeTimelineWithEntries() factory needs a defined shape.
The issue's TimelineDTO shape from the spec is { years: TimelineYearDTO[], undated: TimelineEntryDTO[] }. The factory needs at least one TimelineYearDTO with entries containing at least one TimelineEntryDTO. Define a minimal makeTimelineEntry() factory too, so both the year-band and undated entry tests can stay one-liners.

No new Playwright E2E — correct.
The decision to skip new E2E and rely on #7/#11 is sound. The per-person timeline is a composition of existing, independently-tested components. The axe/dark-mode pass on /persons/[id] correctly belongs to #11.

Targeted single-file test runs — respected.
The project rule is to run only page.server.spec.ts and page.svelte.test.ts locally, not the full suite. The issue respects this. Flag in the PR: "I ran npx vitest run persons/[id]/page.server.spec.ts and npx vitest run persons/[id]/page.svelte.test.ts locally."

Recommendations

  • Add result.timeline assertions to all existing happy-path tests in page.server.spec.ts to catch silent regressions from the 8th slot.
  • Cover the three "absent when empty" subcases explicitly in page.svelte.test.ts.
  • Define a minimal makeTimelineEntry() factory alongside makeTimeline() and makeTimelineWithEntries().
  • Update baseData() factory to include timeline: { years: [], undated: [] } before any other changes — do this as the first commit to make the failing-type-check visible immediately.
## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations **The test plan is detailed and correct — but has execution traps.** **Trap 1: The 8th mock slot is a silent failure risk.** I read `page.server.spec.ts`. It has 5 test cases, each using a `mockResolvedValueOnce` chain of exactly 7 calls. When you add the 8th `Promise.all` slot, each existing chain returns `undefined` for the 8th call. The current `+page.server.ts` does `?? []` / `?? {}` fallbacks, so the tests still pass — but `data.timeline` will be `undefined` in all 5 existing tests, masking the regression. This is the exact failure mode the issue warns about. **The fix is not just adding a chain item to every test — you must verify the test is actually asserting `data.timeline`:** - Tests that only assert `result.person.firstName` or `result.sentDocuments` will pass even if `data.timeline` is broken. - Add at least one assertion per happy-path test that `result.timeline` deep-equals the mock value. - The "timeline sub-fetch failure" test must assert `result.timeline` equals `{ years: [], undated: [] }` exactly (not just that no exception is thrown). **Trap 2: `baseData()` in `page.svelte.test.ts` needs `timeline` in defaults.** I read the current `page.svelte.test.ts`. The `baseData()` factory (line 16-37) does not include `timeline`. When the component prop is added, TypeScript will reject tests that use `baseData()` without `timeline`. Every existing test breaks at the type level. Fix: add `timeline: { years: [], undated: [] }` to `baseData()` defaults. **Component test for "absent when empty" is non-trivial.** The Lebensweg section must be absent when `data.timeline.years.some(y => y.entries.length > 0) || data.timeline.undated.length > 0` is false. The component test must cover three subcases: 1. `timeline: { years: [], undated: [] }` — fully empty. 2. `timeline: { years: [{ year: 1920, entries: [] }], undated: [] }` — year band with empty entries. 3. `timeline: { years: [], undated: [{ ... }] }` — undated entries only (should render the card). The issue spec calls out subcase 2 explicitly as the "fabricated year bands" edge case. Do not skip it. **The `makeTimelineWithEntries()` factory needs a defined shape.** The issue's `TimelineDTO` shape from the spec is `{ years: TimelineYearDTO[], undated: TimelineEntryDTO[] }`. The factory needs at least one `TimelineYearDTO` with `entries` containing at least one `TimelineEntryDTO`. Define a minimal `makeTimelineEntry()` factory too, so both the year-band and undated entry tests can stay one-liners. **No new Playwright E2E — correct.** The decision to skip new E2E and rely on #7/#11 is sound. The per-person timeline is a composition of existing, independently-tested components. The axe/dark-mode pass on `/persons/[id]` correctly belongs to #11. **Targeted single-file test runs — respected.** The project rule is to run only `page.server.spec.ts` and `page.svelte.test.ts` locally, not the full suite. The issue respects this. Flag in the PR: "I ran `npx vitest run persons/[id]/page.server.spec.ts` and `npx vitest run persons/[id]/page.svelte.test.ts` locally." ### Recommendations - Add `result.timeline` assertions to all existing happy-path tests in `page.server.spec.ts` to catch silent regressions from the 8th slot. - Cover the three "absent when empty" subcases explicitly in `page.svelte.test.ts`. - Define a minimal `makeTimelineEntry()` factory alongside `makeTimeline()` and `makeTimelineWithEntries()`. - Update `baseData()` factory to include `timeline: { years: [], undated: [] }` before any other changes — do this as the first commit to make the failing-type-check visible immediately.
marcel commented 2026-06-07 19:54:09 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

The placement and visual spec are correct — one gap to close.

Heading hierarchy is correctly specified.
The issue mandates <h2> for "LEBENSWEG" → <h3> for year bands. I read +page.svelte: the current page uses h2 headings inside PersonCard, NameHistoryCard, CoCorrespondentsList, PersonDocumentList (×2), and GeschichtenCard. The Lebensweg <h2> adds to this list. This is consistent — all sibling sections share the h2 level. Confirm year bands inside <TimelineView> use <h3> (not another <h2>), since <TimelineView> is being embedded inside an already-<h2>-headed section.

The "no nested scroll" constraint is critical for the 60+ audience.
"No overflow-y-auto inner container" — this is the right call. The global timeline page (/zeitstrahl) may have an inner scroll container for filtering UX reasons; when embedded here, the timeline must extend the page flow. Verify that <TimelineView> does not apply overflow-y-auto or max-h-* to itself when rendered in embed mode. If <TimelineView> conditionally applies scroll based on context, that is a prop the personId vs. prop-pass mode should communicate. Flag this against #7 if TimelineView does not have a standalone prop to disable scroll containment.

Typography enforcement: date labels must not use font-serif.
The spec states: date label strings (ca. 1914, Sommer 1914, Ohne Datum) use font-sans; person names and event titles use font-serif. This rule applies both inside <TimelineView> and on any "edit at source" link text. Since #7 owns EventCard.svelte and LetterCard.svelte, the font assignments are <TimelineView>'s responsibility, not this issue's — but this issue is the first consumer of those components on a real page, so if the typography is wrong, it will be visible here first.

Long-name overflow at 375px.
The issue calls out: "When testing at 375px, verify the LetterCard component truncates with truncate or line-clamp." This is a manual test that belongs in the PR checklist. Add it explicitly: "Tested at 375px: letter row sender → receiver → snippet truncates without horizontal overflow."

Touch targets on "edit at source" links.
"Edit at source" links point to Person/relationship edit screens. These must meet the 44×44px minimum (48px preferred for the 60+ audience). At 375px, inline text links inside a timeline card are at risk of being below the touch target minimum if they are pure text anchors with default padding. Verify focus-visible:ring-2 focus-visible:ring-brand-navy is applied (already in the issue's task list) and that the link has enough vertical padding to hit 44px.

Dark-mode tokens.
The card uses bg-surface border-line — these are semantic tokens that remap in dark mode. Correct. The section title uses text-ink-3 mb-5 — also semantic. The <TimelineView> sub-components must also use semantic tokens, not raw Tailwind colors. This is a #7 concern, but flag it in the PR review of this issue's embed.

The <h2> renders via i18n (m.person_lebensweg_heading()), not hardcoded.
This is good — but verify that svelte-check does not produce a type error for the new i18n key before it exists in the generated paraglide output. The key must be added to all three messages/*.json files before the component references it, otherwise npm run check will fail.

Recommendations

  • Add a PR checklist item: "Tested at 375px with DevTools device emulation — no horizontal overflow on letter rows or year band labels."
  • Flag against #7 if <TimelineView> applies overflow-y-auto without a way to disable it for embedded mode.
  • Confirm that the <h2> rendering uses m.person_lebensweg_heading() and that the i18n file changes are in the same commit as the component changes.
  • Verify <TimelineView>'s sub-components use semantic color tokens only — flag against #7 if they use raw Tailwind palette colors.
## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations **The placement and visual spec are correct — one gap to close.** **Heading hierarchy is correctly specified.** The issue mandates `<h2>` for "LEBENSWEG" → `<h3>` for year bands. I read `+page.svelte`: the current page uses `h2` headings inside `PersonCard`, `NameHistoryCard`, `CoCorrespondentsList`, `PersonDocumentList` (×2), and `GeschichtenCard`. The Lebensweg `<h2>` adds to this list. This is consistent — all sibling sections share the `h2` level. Confirm year bands inside `<TimelineView>` use `<h3>` (not another `<h2>`), since `<TimelineView>` is being embedded inside an already-`<h2>`-headed section. **The "no nested scroll" constraint is critical for the 60+ audience.** "No `overflow-y-auto` inner container" — this is the right call. The global timeline page (`/zeitstrahl`) may have an inner scroll container for filtering UX reasons; when embedded here, the timeline must extend the page flow. Verify that `<TimelineView>` does not apply `overflow-y-auto` or `max-h-*` to itself when rendered in embed mode. If `<TimelineView>` conditionally applies scroll based on context, that is a prop the `personId` vs. prop-pass mode should communicate. Flag this against #7 if `TimelineView` does not have a `standalone` prop to disable scroll containment. **Typography enforcement: date labels must not use `font-serif`.** The spec states: date label strings (`ca. 1914`, `Sommer 1914`, `Ohne Datum`) use `font-sans`; person names and event titles use `font-serif`. This rule applies both inside `<TimelineView>` and on any "edit at source" link text. Since #7 owns `EventCard.svelte` and `LetterCard.svelte`, the font assignments are `<TimelineView>`'s responsibility, not this issue's — but this issue is the **first consumer** of those components on a real page, so if the typography is wrong, it will be visible here first. **Long-name overflow at 375px.** The issue calls out: "When testing at 375px, verify the `LetterCard` component truncates with `truncate` or `line-clamp`." This is a manual test that belongs in the PR checklist. Add it explicitly: "Tested at 375px: letter row sender → receiver → snippet truncates without horizontal overflow." **Touch targets on "edit at source" links.** "Edit at source" links point to Person/relationship edit screens. These must meet the 44×44px minimum (48px preferred for the 60+ audience). At 375px, inline text links inside a timeline card are at risk of being below the touch target minimum if they are pure text anchors with default padding. Verify `focus-visible:ring-2 focus-visible:ring-brand-navy` is applied (already in the issue's task list) and that the link has enough vertical padding to hit 44px. **Dark-mode tokens.** The card uses `bg-surface border-line` — these are semantic tokens that remap in dark mode. Correct. The section title uses `text-ink-3 mb-5` — also semantic. The `<TimelineView>` sub-components must also use semantic tokens, not raw Tailwind colors. This is a #7 concern, but flag it in the PR review of this issue's embed. **The `<h2>` renders via i18n (`m.person_lebensweg_heading()`), not hardcoded.** This is good — but verify that `svelte-check` does not produce a type error for the new i18n key before it exists in the generated paraglide output. The key must be added to all three `messages/*.json` files **before** the component references it, otherwise `npm run check` will fail. ### Recommendations - Add a PR checklist item: "Tested at 375px with DevTools device emulation — no horizontal overflow on letter rows or year band labels." - Flag against #7 if `<TimelineView>` applies `overflow-y-auto` without a way to disable it for embedded mode. - Confirm that the `<h2>` rendering uses `m.person_lebensweg_heading()` and that the i18n file changes are in the same commit as the component changes. - Verify `<TimelineView>`'s sub-components use semantic color tokens only — flag against #7 if they use raw Tailwind palette colors.
marcel commented 2026-06-07 19:54:25 +02:00
Author
Owner
Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

Zero infrastructure footprint — no new services, ports, volumes, or env vars.
This issue is a pure frontend composition task riding on GET /api/timeline?personId=…, which is a new endpoint from #5. My only infrastructure concern is what happens when #5 is not deployed (i.e., this issue lands in staging before #5 lands in production).

The graceful-degradation path handles the pre-#5 case correctly.
timelineResult.data ?? { years: [], undated: [] } — when GET /api/timeline returns 404 (endpoint doesn't exist yet), the HTTP status is 404, response.ok is false, so the fallback kicks in and the Lebensweg card is hidden. This is correct behavior during a phased rollout where #5 has not landed yet.

The p95 monitoring note is operationally sound.
The issue flags: "After #5 + #10 land, check http_server_requests_seconds{uri="/api/timeline", quantile="0.95"} — should stay under 500ms." The Prometheus metric is already instrumented by Spring Boot's Actuator (this is the existing micrometer-based request duration histogram). No new instrumentation needed. Verify on the Grafana dashboard that the /api/timeline URI is correctly captured — Spring Boot's default Micrometer tag may group it as /api/timeline with the personId query param stripped (query params are stripped by default in Micrometer's URI tagging). This is correct behavior; the same histogram covers both the global and per-person timeline.

The 8th parallel call adds one more outbound HTTP request per person-page load.
The existing 7 parallel calls are already the load profile. Adding an 8th is a minor increase. At current traffic volumes (family project), this is noise. If GET /api/timeline?personId= turns out to be slow (the spec acknowledges it can be the heaviest read), it is observable via the Prometheus histogram. The issue correctly says to surface this as an indexing problem to #5 rather than papering over it with a client-side lazy-load. That is the right call — a lazy-load would break SSR and auth-cookie forwarding.

No new Docker service, no new Compose file changes, no new Caddy config.
Nothing to review on the infrastructure side.

Recommendations

  • After this issue lands alongside #5, add a Grafana alert on http_server_requests_seconds{uri="/api/timeline", quantile="0.95"} > 0.5 with a runbook pointing to #5's index documentation.
  • No blocking infrastructure concerns. This issue is safe to proceed.
## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations **Zero infrastructure footprint — no new services, ports, volumes, or env vars.** This issue is a pure frontend composition task riding on `GET /api/timeline?personId=…`, which is a new endpoint from #5. My only infrastructure concern is what happens when #5 is not deployed (i.e., this issue lands in staging before #5 lands in production). **The graceful-degradation path handles the pre-#5 case correctly.** `timelineResult.data ?? { years: [], undated: [] }` — when `GET /api/timeline` returns 404 (endpoint doesn't exist yet), the HTTP status is 404, `response.ok` is false, so the fallback kicks in and the Lebensweg card is hidden. This is correct behavior during a phased rollout where #5 has not landed yet. **The p95 monitoring note is operationally sound.** The issue flags: "After #5 + #10 land, check `http_server_requests_seconds{uri="/api/timeline", quantile="0.95"}` — should stay under 500ms." The Prometheus metric is already instrumented by Spring Boot's Actuator (this is the existing micrometer-based request duration histogram). No new instrumentation needed. Verify on the Grafana dashboard that the `/api/timeline` URI is correctly captured — Spring Boot's default Micrometer tag may group it as `/api/timeline` with the `personId` query param stripped (query params are stripped by default in Micrometer's URI tagging). This is correct behavior; the same histogram covers both the global and per-person timeline. **The 8th parallel call adds one more outbound HTTP request per person-page load.** The existing 7 parallel calls are already the load profile. Adding an 8th is a minor increase. At current traffic volumes (family project), this is noise. If `GET /api/timeline?personId=` turns out to be slow (the spec acknowledges it can be the heaviest read), it is observable via the Prometheus histogram. The issue correctly says to surface this as an indexing problem to #5 rather than papering over it with a client-side lazy-load. That is the right call — a lazy-load would break SSR and auth-cookie forwarding. **No new Docker service, no new Compose file changes, no new Caddy config.** Nothing to review on the infrastructure side. ### Recommendations - After this issue lands alongside #5, add a Grafana alert on `http_server_requests_seconds{uri="/api/timeline", quantile="0.95"} > 0.5` with a runbook pointing to #5's index documentation. - No blocking infrastructure concerns. This issue is safe to proceed.
marcel commented 2026-06-07 19:54:46 +02:00
Author
Owner
Copy Link

📋 Elicit — Requirements Engineer

Observations

The acceptance criteria are well-formed and verifiable. All 6 ACs are Given-When-Then testable, with no vague qualifiers. AC2 correctly distinguishes "no dated items" from "year bands with empty entries" — a non-trivial distinction that most requirements miss. AC4 ("375px without horizontal scroll") is measurable. AC6 (graceful degradation) is observable and has an implementation path.

One AC is blocked by another issue and should be marked as such.
AC3 ("Given a marriage between A and B, then the same Heirat event appears on both A's and B's Lebensweg") is explicitly dependent on #5 verifying symmetric assembly. The AC is correct, but it cannot be verified by this issue's implementation alone — it requires #5 to have added the symmetry test first. The issue body notes this, but the AC itself should carry a tag like "(Verifiability depends on #5 AC3 symmetry)" to prevent it from being rubber-stamped as passing when the backend hasn't been tested.

The "hard depends-on" list is clear, but the sequencing signal is buried.
The issue states: "Do not start this issue until #7's spec confirms both signatures are in its acceptance criteria." This is a hard prerequisite, not just a soft dependency. It should be reflected in the issue's blocked-by links, not only in prose. Confirm that the Gitea issue tracker has the blocked-by relationship set.

Missing: a rollback acceptance criterion.
If GET /api/timeline starts returning 500 (backend error, not 403/404), the current spec only mandates hiding the Lebensweg card. A 500 on a sub-fetch would cause response.ok to be false, so the fallback activates. This is the correct behavior. However, the ACs do not cover the 500 case explicitly — only 403/503 are named in the Security section. Consider adding: "Given the timeline endpoint returns a 5xx error, the Lebensweg card is hidden and the rest of the person page renders normally." This is a superset of AC6 but makes the test coverage intent explicit.

The i18n requirement (AC5) is verifiable but needs a concrete test.
AC5 says "Heading uses i18n key person_lebensweg_heading in de/en/es." The component test asserts getByRole('heading', { name: /lebensweg/i }) — this matches the German (default) translation. The test does not verify English or Spanish. For a feature this explicitly i18n-specified, add a test that switches locale and verifies the heading changes. This is low-cost in Paraglide: set document.documentElement.lang or use the Paraglide test utility to switch the active language.

Data scope is clear — one implicit assumption worth surfacing.
The scope states letters they "sent or received" are included. The +page.server.ts already fetches sentDocuments and receivedDocuments separately. The timeline assembly endpoint (#5) does its own query for the same data. This means the person page will fetch some letter data twice: once for PersonDocumentList and once for the timeline. This is not a bug (they serve different presentation purposes), but it is worth noting in the issue as an explicit, accepted trade-off so it is not "fixed" by someone trying to DRY up the calls.

Recommendations

  • Add "(Verifiability depends on #5 AC3 symmetry)" annotation to AC3.
  • Add AC7: "Given the timeline endpoint returns a 5xx error, the Lebensweg card is hidden and the rest of the person page renders normally." (This tests !response.ok for 5xx, matching the existing pattern.)
  • Add a locale-switch test to verify AC5 in English and/or Spanish, not just German.
  • Confirm the Gitea blocked-by relationship with #7 is set in the issue tracker.
## 📋 Elicit — Requirements Engineer ### Observations **The acceptance criteria are well-formed and verifiable.** All 6 ACs are Given-When-Then testable, with no vague qualifiers. AC2 correctly distinguishes "no dated items" from "year bands with empty entries" — a non-trivial distinction that most requirements miss. AC4 ("375px without horizontal scroll") is measurable. AC6 (graceful degradation) is observable and has an implementation path. **One AC is blocked by another issue and should be marked as such.** AC3 ("Given a marriage between A and B, then the same Heirat event appears on both A's and B's Lebensweg") is explicitly dependent on #5 verifying symmetric assembly. The AC is correct, but it **cannot be verified by this issue's implementation alone** — it requires #5 to have added the symmetry test first. The issue body notes this, but the AC itself should carry a tag like "(Verifiability depends on #5 AC3 symmetry)" to prevent it from being rubber-stamped as passing when the backend hasn't been tested. **The "hard depends-on" list is clear, but the sequencing signal is buried.** The issue states: "Do not start this issue until #7's spec confirms both signatures are in its acceptance criteria." This is a hard prerequisite, not just a soft dependency. It should be reflected in the issue's blocked-by links, not only in prose. Confirm that the Gitea issue tracker has the blocked-by relationship set. **Missing: a rollback acceptance criterion.** If `GET /api/timeline` starts returning 500 (backend error, not 403/404), the current spec only mandates hiding the Lebensweg card. A 500 on a sub-fetch would cause `response.ok` to be false, so the fallback activates. This is the correct behavior. However, the ACs do not cover the 500 case explicitly — only 403/503 are named in the Security section. Consider adding: "Given the timeline endpoint returns a 5xx error, the Lebensweg card is hidden and the rest of the person page renders normally." This is a superset of AC6 but makes the test coverage intent explicit. **The i18n requirement (AC5) is verifiable but needs a concrete test.** AC5 says "Heading uses i18n key `person_lebensweg_heading` in de/en/es." The component test asserts `getByRole('heading', { name: /lebensweg/i })` — this matches the German (default) translation. The test does not verify English or Spanish. For a feature this explicitly i18n-specified, add a test that switches locale and verifies the heading changes. This is low-cost in Paraglide: set `document.documentElement.lang` or use the Paraglide test utility to switch the active language. **Data scope is clear — one implicit assumption worth surfacing.** The scope states letters they "sent or received" are included. The `+page.server.ts` already fetches `sentDocuments` and `receivedDocuments` separately. The timeline assembly endpoint (#5) does its own query for the same data. This means the person page will fetch some letter data twice: once for `PersonDocumentList` and once for the timeline. This is not a bug (they serve different presentation purposes), but it is worth noting in the issue as an explicit, accepted trade-off so it is not "fixed" by someone trying to DRY up the calls. ### Recommendations - Add "(Verifiability depends on #5 AC3 symmetry)" annotation to AC3. - Add AC7: "Given the timeline endpoint returns a 5xx error, the Lebensweg card is hidden and the rest of the person page renders normally." (This tests `!response.ok` for 5xx, matching the existing pattern.) - Add a locale-switch test to verify AC5 in English and/or Spanish, not just German. - Confirm the Gitea blocked-by relationship with #7 is set in the issue tracker.
marcel commented 2026-06-08 16:28:20 +02:00
Author
Owner
Copy Link

Visual spec for the per-person Lebensweg (on main, commit ddb1ec4d):

docs/specs/zeitstrahl-final-spec.html — §4 shows the reuse: the same TimelineView with a personId prop placed in the 35 % left rail under PersonCard on the Person detail page (persons/[id]/+page.svelte is lg:grid-cols-[35%_65%]), filtered to one person (own Geburt/Heirat/Tod, involved world-events as context, own letter clusters). §5 covers the responsive story — same component, narrow rail = the phone left-axis layout (a real argument for Concept A: one idiom from rail → phone → desktop).

**Visual spec** for the per-person Lebensweg (on `main`, commit `ddb1ec4d`): [`docs/specs/zeitstrahl-final-spec.html`](https://git.raddatz.cloud/marcel/familienarchiv/src/branch/main/docs/specs/zeitstrahl-final-spec.html) — **§4** shows the reuse: the same `TimelineView` with a `personId` prop placed in the **35 % left rail under `PersonCard`** on the Person detail page (`persons/[id]/+page.svelte` is `lg:grid-cols-[35%_65%]`), filtered to one person (own Geburt/Heirat/Tod, involved world-events as context, own letter clusters). **§5** covers the responsive story — same component, narrow rail = the phone left-axis layout (a real argument for Concept A: one idiom from rail → phone → desktop).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 needs-discussion
Has an open decision or design question that must be resolved before implementation can start.
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label P3-later feature person ui
Milestone
No items
No Milestone Zeitstrahl — Family Timeline
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#782
Delete Branch "%!s()"

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?