Timeline: curator event create/edit forms #781

New Issue

marcel · 2026-06-07T19:29:32+02:00

marcel commented

2026-06-07 19:29:32 +02:00

Milestone: Zeitstrahl — Family Timeline
Spec: docs/superpowers/specs/2026-06-07-family-timeline-design.md § "Frontend"
Depends on: #3 (TimelineEvent CRUD API — @RequirePermission(WRITE_ALL) on POST/PUT/DELETE), #6 (shared TimelineEvent types + dateLabel.ts precision rendering).
Build-time prerequisite: npm run generate:api must have run after #3 so TimelineEvent/TimelineEventRequest types exist before this work starts. If #9 is merged before #3, CI's generate:api step will fail — #3 must merge first.

Overview

Curator UI to create/edit/delete hand-curated timeline events, gated to WRITE_ALL. Pure frontend: two SvelteKit routes plus reused components. No new backend package, entity, migration, or ErrorCode — therefore no ADR and no DB-diagram update is triggered here (the timeline/ domain ADR belongs to #2/#3).

Doc updates triggered by this issue: add the events/new and events/[id]/edit children to the CLAUDE.md route table and the frontend C4 diagram (alongside /zeitstrahl from #7).

Scope

/zeitstrahl/events/new — empty create form.
/zeitstrahl/events/[id]/edit — edit form seeded from GET /api/timeline/events/{id}; includes the delete control.

Architecture & reuse

One component, two routes. Build EventForm.svelte taking optional event + initialPersons/initialDocuments props. /new renders it empty; /[id]/edit renders it seeded. Do not fork the markup.
Extract a shared DatePrecisionField.svelte into $lib/shared/primitives/ (not $lib/timeline/). It is a generic date-input primitive shared by two domains (document/ via WhoWhenSection and timeline/ via EventForm); placing it in either consumer's domain would create a cross-domain import. No new lib-level diagram entry is needed. WhoWhenSection already implements the exact logic (German dd.mm.yyyy input via handleGermanDateInput + hidden ISO input, dateDirty tracking, endBeforeStart guard, aria-live="polite" on the revealed end-date). Refactor that region into the shared primitive and have both WhoWhenSection and EventForm consume it. Do NOT import WhoWhenSection wholesale (it carries sender/receiver/location concerns that do not belong on a timeline event).
- The extracted component must preserve the aria-live="polite" wrapper around the entire {#if showEndDate} region (not just the error text) — do not move it to a child element.
- Expose a data-testid="end-date-region" attribute on the RANGE-revealed end-date container so that WhoWhenSection.svelte.spec.ts's existing data-testid="who-when-end-date" and data-testid="who-when-precision" selectors survive the refactor (coordinate naming with the existing spec).
- precision and endDateIso must remain $bindable props so the parent can read values (the WhoWhenSection bindable pattern must be preserved).
EventTypeSelect.svelte — its own tiny component. Follow the PersonTypeSelector.svelte reference verbatim: role="radiogroup" with role="radio" buttons, aria-checked, tabindex roving, radioGroupNav action from $lib/shared/actions/. Two options: PERSONAL / HISTORICAL. Rendered as a segmented radio group (not a <select>): each option pairs a localized text label with a decorative icon (aria-hidden="true") for the person/world accent — no color-only differentiation.
Pickers are wiring, not new components: PersonMultiSelect.svelte (already exists at $lib/person/PersonMultiSelect.svelte, already consumed by WhoWhenSection — just wire it into EventForm) and DocumentMultiSelect.svelte (emits a documentIds hidden input, form-actions-ready). Each needs an associated <label> (not placeholder-as-label), a visible empty state ("Noch keine Person verknüpft" / "Noch kein Dokument verknüpft" / localized), and ≥44px remove targets on chips (add min-h-[44px] min-w-[44px] or equivalent padding expansion on the remove button — the current DocumentMultiSelect remove button is ~12px and does not meet this requirement).
DocumentMultiSelect typeahead auth: the existing component fires fetch('/api/documents/search?q=...') as a bare browser call — the same pattern as the Geschichte editor. This is intentional: in dev the Vite proxy forwards /api; in prod the app is same-origin. Add a code comment to the component making this explicit. Do not refactor to an internal +server.ts proxy (no practical security benefit, adds complexity).
All data flows through +page.server.ts load + form action (SSR) — never a client fetch('/api/...') in the page or form layer. Do not use the geschichten/new csrfFetch + goto() pattern; use SvelteKit form actions.
Delete confirmation: use getConfirmService() (globally mounted ConfirmDialog in the root layout) + use:enhance cancel pattern as shown in the confirm service JSDoc. Do not build a bespoke dialog — ConfirmDialog already handles aria-modal, keyboard trap, and the destructive button variant.

Routes & gating

Gating idiom: 403 error page (throw error(403, 'Forbidden'), the persons/new idiom). This is the project convention for permission-gated author routes; it is more honest about why the user was stopped than a silent redirect. The load function reads locals.user.groups server-side to make the decision — never gate route access on a client-derived data.canWrite flag (that flag is only for hiding entry-point buttons elsewhere). The groups check is a string comparison against 'WRITE_ALL' (mirroring Permission.WRITE_ALL exactly) — add a comment to that effect.
The frontend gate is UX, not security. The real authorization boundary is the backend @RequirePermission(WRITE_ALL) on the CRUD endpoints (#3). A hand-crafted POST is stopped by the backend, not by this code. The AC "non-curators cannot reach the forms" is satisfied by the load guard returning 403.
Edit route must fail closed. Derived person-events (Geburt/Tod/Heirat) are not persisted and have no UUID, so GET /api/timeline/events/{id} 404s for them. Guard the load: !result.response.ok → throw error(404). This covers all non-ok responses (not just 404). Never render a blank editable form that silently POSTs a new event.
Prefill via ?personId=/?documentId= query params (mirror geschichten/new): pre-selects that person/document for the "Ereignis hinzufügen" flow from a Person's Lebensweg. Silently swallow 404/403 on prefill lookups — check personResult && personResult.response.ok && personResult.data and return initialPersons: [] on any failure; do not throw. This avoids leaking entity existence on unknown IDs (copy geschichten/new's comment-documented behavior verbatim).

Form fields & validation

Fields: title (required), type (PERSONAL/HISTORICAL, segmented radio), eventDate + precision selector (required), eventDateEnd (shown only when precision = RANGE), description (optional), person picker (optional), document picker (optional).

Use the conditional-spread idiom for optionals. Send eventDateEnd explicitly as null when precision flips away from RANGE on an edit — the DatePrecisionField does this via value={showEndDate ? endDateIso : ''} — so a stale end-date does not persist. The form action must convert the empty string to null in the TimelineEventRequest body.
API result handling: check !result.response.ok; use result.data! after the ok-check; map errors via getErrorMessage(extractErrorCode(result.error as unknown as { code?: string })). Never read result.error directly.
Required fields (title, eventDate) marked with * + aria-required; on error use icon + text + aria-invalid, never red border alone. RANGE end-date-before-start surfaces the same inline ⚠ cue as WhoWhenSection; the backend (#3) owns the hard rejection.
Submitting state: add a submitting reactive flag via use:enhance and apply disabled={submitting} on the submit button. This prevents double-submit and provides essential feedback for the senior audience on slower connections (authors on 60+ laptop/tablet). The use:enhance callback sets submitting = true on start and resets it on complete.
Senior-audience a11y (author path skews 60+ on laptop/tablet): 16px+ body, 48px targets, redundant non-color cues, no timed interactions, aria-live="polite" on the revealed end-date. Inherit WhoWhenSection's treatment; do not regress it. Card pattern per section (rounded-sm border border-line bg-surface shadow-sm p-6), not one flat stack. <h2 class="text-xs font-bold uppercase tracking-widest text-ink-3 mb-5"> section titles. Run axe on the route in both light and dark mode (dark mode remaps all color tokens — verify end-date error text contrast).
Back navigation: shared <BackButton>, never a static <a href>.
Keyed {#each} on precision options and picker chips ((p.value), (person.id)).
i18n keys for all labels/errors/empty-states in de/en/es. Includes document picker empty state ("Noch kein Dokument verknüpft"). No new ErrorCode / errors.ts work needed.

Delete

Placement: a "Löschen" button on the /[id]/edit form, behind getConfirmService() confirmation → DELETE /api/timeline/events/{id} → redirect. Keeps this issue self-contained, fulfills the AC, and matches the CRUD-form mental model. Card-level delete in the #7 timeline view is a separate, additive concern.
The confirm dialog uses ConfirmDialog's destructive button variant; the dialog already handles keyboard trapping and aria-modal.
ConfirmDialog.svelte patch: add aria-modal="true" to the <dialog> element (one-line patch, benefits all uses). The HTML <dialog showModal()> has implicit modal semantics in modern browsers but aria-modal="true" is best practice for older AT.
Delete action failure: if the DELETE returns !ok, return fail(status, { error: getErrorMessage(...) }) and surface the error in the form — do not redirect.

Context-aware redirect. If the form was launched via ?personId=, return to that person's page (/persons/{personId}); otherwise redirect to /zeitstrahl. Thread the originating context through a hidden <input type="hidden" name="originPersonId"> field. Validate originPersonId: in the action, accept the value only if it is a non-empty UUID-format string; default to /zeitstrahl on empty or malformed values to prevent an open redirect on a crafted hidden field.

Validation failure re-render (picker persistence)

When the form action returns fail(400, { ... }), include personIds and documentIds arrays in the payload so the pickers re-populate on re-render. The load function seeds initialPersons/initialDocuments from these IDs on re-render (same path as the prefill lookup). This matches the "re-rendered with entered values preserved" AC for all fields, not just scalars.

Acceptance criteria (Given-When-Then)

Given a curator, when they use /new or /[id]/edit, then they can create, edit, and delete events. Given a non-curator (logged out or READ_ALL only), when they navigate to either route, then a 403 error page is shown.
Given precision = RANGE, when the form renders, then the end-date field is visible (and required); given precision ≠ RANGE, then it is hidden and eventDateEnd is submitted as null (not a stale date or empty string).
Given precision is changed from RANGE to YEAR on an edit, when the form is submitted, then eventDateEnd is sent as null.
Given a blank title, when the curator submits, then the form shows a localized required-field error, does not navigate away, and re-renders with all entered values preserved — including pre-selected persons/documents.
Given an unknown or derived event id, when the edit route loads, then a 404 is shown — never a blank create form.
Given a curator deletes an event and confirms, when the DELETE succeeds, then the event is removed and they return to /zeitstrahl (or the originating person page if launched via ?personId); when the DELETE fails, then the error is surfaced in the form and no redirect occurs.
Given initialPersons/initialDocuments (prefill), when the form renders, then those persons/documents are pre-selected; given an unknown prefill id, then it is silently ignored (no "not found" leak).
Linking persons/documents works; pickers have labelled controls, visible empty states, and ≥44px remove targets.

Tests

Component (*.svelte.spec.ts, vitest-browser, single-file local runs):

should_reveal_end_date_field_when_precision_is_RANGE and should_hide_end_date_field_when_precision_is_YEAR (headline AC). Assert via getByLabelText/getByRole + toBeVisible()/not-present, not internal state.
should_preselect_person_when_initialPersons_provided.
should_show_required_error_when_title_is_blank.
should_submit_null_for_eventDateEnd_when_precision_changed_from_RANGE_to_YEAR.
RANGE end-date-before-start surfaces the inline cue (component layer); the hard rejection is owned by #3's backend test — ensure neither layer leaves the constraint unasserted.
Factory makeEvent(overrides) returning the TimelineEvent shape — do not repeat the builder per test. Document the minimal required shape in a comment in the spec file once npm run generate:api has run after #3.

Server (page.server.spec.ts, import load/actions, mock fetch/api client):

load throws 403 for a non-curator (mirror persons/new/page.server.spec.ts).
load on /[id]/edit throws 404 when the API returns !ok (covers derived-event / unknown-id / any non-ok status — regression test).
action maps fail(400, { ..., personIds, documentIds }) on blank title with all values preserved (including picker arrays).
action maps API error via getErrorMessage and redirect(303) on success.
delete action redirects to the resolved context target on DELETE success.
delete action returns fail(status, { error: getErrorMessage(...) }) when DELETE returns !ok.
originPersonId redirect validation: action defaults to /zeitstrahl when originPersonId is empty, non-UUID, or absent.

Regression (WhoWhenSection.svelte.spec.ts):

Run green after DatePrecisionField extraction — verify existing data-testid selectors (who-when-end-date, who-when-precision) are preserved and no behavior regresses.

E2E (Playwright — keep thin, one critical journey + security counterpart):

Curator logs in → creates an event with precision RANGE → verifies the route returns HTTP 200 on /zeitstrahl. (Full "sees the event card" assertion depends on #7 — if this issue ships before #7, scope to HTTP 200 only, not card presence.)
Logged-out / READ_ALL-only user navigating to /zeitstrahl/events/new is blocked (403). Do not push field/precision permutations to E2E — those belong at the component layer.

Task checklist

Patch ConfirmDialog.svelte: add aria-modal="true" to the <dialog> element (one-line patch).
Extract DatePrecisionField.svelte into $lib/shared/primitives/ from WhoWhenSection's date+precision+RANGE-end-date region; preserve aria-live="polite" around the full {#if showEndDate} block, $bindable props for precision/endDateIso, and data-testid attributes compatible with the existing WhoWhenSection spec. Refactor WhoWhenSection to consume it.
Run WhoWhenSection.svelte.spec.ts green after extraction — verify no data-testid selectors or behavior regressed.
EventTypeSelect.svelte — segmented radio group modelled on PersonTypeSelector.svelte (role="radiogroup", radioGroupNav, tabindex roving); icon (aria-hidden="true") + text label per option; two options: PERSONAL / HISTORICAL.
EventForm.svelte — title, type, DatePrecisionField, description, PersonMultiSelect, DocumentMultiSelect; optional event/initialPersons/initialDocuments props; conditional-spread optionals; explicit null for eventDateEnd off RANGE; submitting flag + disabled={submitting} on submit.
Route /zeitstrahl/events/new (+page.server.ts load + action) with ?personId/?documentId prefill (404/403 swallowed, return initialPersons: []).
Route /zeitstrahl/events/[id]/edit (load seeds form, throw error(404) on any !ok response; action for update + delete; delete uses getConfirmService()).
Delete action: fail(status, { error }) on DELETE !ok; redirect on success.
originPersonId validation in action: UUID-format check; default to /zeitstrahl on invalid/empty.
fail(400) payload includes personIds/documentIds arrays so pickers re-populate on re-render.
Permission-gate both routes server-side (403 via locals.user.groups; string comparison against 'WRITE_ALL'; add comment mirroring Permission.WRITE_ALL); hide entry-point buttons for non-curators.
DocumentMultiSelect remove button: add min-h-[44px] min-w-[44px] (or equivalent padded hit area). Add code comment that bare fetch('/api/...') is intentional (same-origin in prod, Vite-proxied in dev — matches Geschichte editor).
PersonMultiSelect remove button: verify ≥44px target; fix if needed.
Picker empty states: "Noch keine Person verknüpft" and "Noch kein Dokument verknüpft" (localized).
Labelled pickers; <BackButton>; card-pattern sections; senior-a11y treatment; axe light+dark.
i18n keys (de/en/es) — including document picker empty state.
Update CLAUDE.md route table + frontend C4 diagram with events/new and events/[id]/edit children.
Tests per the strategy above (component, server, regression, E2E thin).

Decisions resolved (Round 1)

Delete control → Option (a): "Löschen" button on the edit form behind getConfirmService() confirmation, DELETE then redirect. Rationale: keeps the issue self-contained, fulfills the AC's "delete" now rather than deferring to #7, and matches the standard CRUD-form mental model.
Post-save / post-delete navigation → Context-aware: return to the originating person page when launched via ?personId=, else /zeitstrahl. Thread via hidden originPersonId field; validate as UUID before using in redirect. Rationale: friendliest for the "add event from a person" flow at trivial cost, with a safe default and open-redirect protection.
Route-gating idiom → 403 error page (the persons/new idiom), adopted as the project convention for permission-gated author routes. Rationale: honest about why access was denied; consistent with a server-side load guard; gentler redirects are appropriate for read paths, not author tools.
EventType control → Segmented radio group (icon + text per option), modelled on PersonTypeSelector.svelte. Rationale: keyboard-trivial for a binary choice, provides redundant non-color cues out of the box.
DatePrecisionField.svelte location → $lib/shared/primitives/. Rationale: it is a generic date-input primitive shared by two domains; placing it in either consumer's domain would create a cross-domain import; no new diagram entry needed.
initialPersons/initialDocuments in fail(400) payload → Include personIds/documentIds arrays in the fail payload and re-seed pickers on re-render. Rationale: consistent with "all entered values preserved" AC; dropping picker state on a required-field error is a significant UX regression for the senior audience.
ConfirmDialog aria-modal patch → Add aria-modal="true" to the <dialog> element now (one-line patch). Rationale: zero downside, benefits all dialog uses, best practice for AT compatibility across browsers.
Submit/submitting state → Add submitting flag + disabled={submitting} via use:enhance. Rationale: prevents double-submit and provides essential feedback for the senior audience on slower connections; trivial to implement with use:enhance.
DocumentMultiSelect typeahead auth path → Keep bare fetch('/api/...') as-is (intentional, matches Geschichte editor), add a code comment. Rationale: already works in dev (Vite proxy) and prod (same-origin); a +server.ts proxy adds complexity with no security benefit.

**Milestone:** Zeitstrahl — Family Timeline **Spec:** `docs/superpowers/specs/2026-06-07-family-timeline-design.md` § "Frontend" **Depends on:** #3 (TimelineEvent CRUD API — `@RequirePermission(WRITE_ALL)` on POST/PUT/DELETE), #6 (shared `TimelineEvent` types + `dateLabel.ts` precision rendering). **Build-time prerequisite:** `npm run generate:api` must have run after #3 so `TimelineEvent`/`TimelineEventRequest` types exist before this work starts. If #9 is merged before #3, CI's `generate:api` step will fail — #3 must merge first. ## Overview Curator UI to create/edit/delete hand-curated timeline events, gated to `WRITE_ALL`. Pure frontend: two SvelteKit routes plus reused components. No new backend package, entity, migration, or `ErrorCode` — therefore **no ADR and no DB-diagram update** is triggered here (the `timeline/` domain ADR belongs to #2/#3). Doc updates triggered by this issue: add the `events/new` and `events/[id]/edit` children to the `CLAUDE.md` route table and the frontend C4 diagram (alongside `/zeitstrahl` from #7). ## Scope - `/zeitstrahl/events/new` — empty create form. - `/zeitstrahl/events/[id]/edit` — edit form seeded from `GET /api/timeline/events/{id}`; includes the delete control. ## Architecture & reuse - **One component, two routes.** Build `EventForm.svelte` taking optional `event` + `initialPersons`/`initialDocuments` props. `/new` renders it empty; `/[id]/edit` renders it seeded. Do not fork the markup. - **Extract a shared `DatePrecisionField.svelte`** into **`$lib/shared/primitives/`** (not `$lib/timeline/`). It is a generic date-input primitive shared by two domains (`document/` via `WhoWhenSection` and `timeline/` via `EventForm`); placing it in either consumer's domain would create a cross-domain import. No new lib-level diagram entry is needed. `WhoWhenSection` already implements the exact logic (German `dd.mm.yyyy` input via `handleGermanDateInput` + hidden ISO input, `dateDirty` tracking, `endBeforeStart` guard, `aria-live="polite"` on the revealed end-date). Refactor that region into the shared primitive and have **both** `WhoWhenSection` and `EventForm` consume it. Do NOT import `WhoWhenSection` wholesale (it carries sender/receiver/location concerns that do not belong on a timeline event). - The extracted component must preserve the `aria-live="polite"` wrapper around the entire `{#if showEndDate}` region (not just the error text) — do not move it to a child element. - Expose a `data-testid="end-date-region"` attribute on the RANGE-revealed end-date container so that `WhoWhenSection.svelte.spec.ts`'s existing `data-testid="who-when-end-date"` and `data-testid="who-when-precision"` selectors survive the refactor (coordinate naming with the existing spec). - `precision` and `endDateIso` must remain `$bindable` props so the parent can read values (the `WhoWhenSection` bindable pattern must be preserved). - **`EventTypeSelect.svelte`** — its own tiny component. Follow the `PersonTypeSelector.svelte` reference verbatim: `role="radiogroup"` with `role="radio"` buttons, `aria-checked`, `tabindex` roving, `radioGroupNav` action from `$lib/shared/actions/`. Two options: PERSONAL / HISTORICAL. Rendered as a **segmented radio group** (not a `<select>`): each option pairs a localized text label with a decorative icon (`aria-hidden="true"`) for the person/world accent — no color-only differentiation. - **Pickers are wiring, not new components:** `PersonMultiSelect.svelte` (already exists at `$lib/person/PersonMultiSelect.svelte`, already consumed by `WhoWhenSection` — just wire it into `EventForm`) and `DocumentMultiSelect.svelte` (emits a `documentIds` hidden input, form-actions-ready). Each needs an associated `<label>` (not placeholder-as-label), a visible empty state ("Noch keine Person verknüpft" / "Noch kein Dokument verknüpft" / localized), and ≥44px remove targets on chips (add `min-h-[44px] min-w-[44px]` or equivalent padding expansion on the remove button — the current `DocumentMultiSelect` remove button is ~12px and does not meet this requirement). - **`DocumentMultiSelect` typeahead auth:** the existing component fires `fetch('/api/documents/search?q=...')` as a bare browser call — the same pattern as the Geschichte editor. This is intentional: in dev the Vite proxy forwards `/api`; in prod the app is same-origin. Add a code comment to the component making this explicit. Do **not** refactor to an internal `+server.ts` proxy (no practical security benefit, adds complexity). - All data flows through `+page.server.ts` `load` + form `action` (SSR) — never a client `fetch('/api/...')` in the page or form layer. Do not use the `geschichten/new` `csrfFetch` + `goto()` pattern; use SvelteKit form actions. - **Delete confirmation:** use `getConfirmService()` (globally mounted `ConfirmDialog` in the root layout) + `use:enhance` cancel pattern as shown in the confirm service JSDoc. Do **not** build a bespoke dialog — `ConfirmDialog` already handles `aria-modal`, keyboard trap, and the destructive button variant. ## Routes & gating - **Gating idiom: 403 error page** (`throw error(403, 'Forbidden')`, the `persons/new` idiom). This is the project convention for permission-gated author routes; it is more honest about *why* the user was stopped than a silent redirect. The `load` function reads `locals.user.groups` **server-side** to make the decision — never gate route access on a client-derived `data.canWrite` flag (that flag is only for hiding entry-point buttons elsewhere). The `groups` check is a string comparison against `'WRITE_ALL'` (mirroring `Permission.WRITE_ALL` exactly) — add a comment to that effect. - **The frontend gate is UX, not security.** The real authorization boundary is the backend `@RequirePermission(WRITE_ALL)` on the CRUD endpoints (#3). A hand-crafted POST is stopped by the backend, not by this code. The AC "non-curators cannot reach the forms" is satisfied by the `load` guard returning 403. - **Edit route must fail closed.** Derived person-events (Geburt/Tod/Heirat) are not persisted and have no UUID, so `GET /api/timeline/events/{id}` 404s for them. Guard the load: `!result.response.ok` → `throw error(404)`. This covers all non-ok responses (not just 404). Never render a blank editable form that silently POSTs a new event. - **Prefill** via `?personId=`/`?documentId=` query params (mirror `geschichten/new`): pre-selects that person/document for the "Ereignis hinzufügen" flow from a Person's Lebensweg. Silently swallow 404/403 on prefill lookups — check `personResult && personResult.response.ok && personResult.data` and return `initialPersons: []` on any failure; do not throw. This avoids leaking entity existence on unknown IDs (copy `geschichten/new`'s comment-documented behavior verbatim). ## Form fields & validation Fields: title (required), type (PERSONAL/HISTORICAL, segmented radio), eventDate + precision selector (required), eventDateEnd (shown only when precision = RANGE), description (optional), person picker (optional), document picker (optional). - Use the conditional-spread idiom for optionals. **Send `eventDateEnd` explicitly as `null`** when precision flips away from RANGE on an edit — the `DatePrecisionField` does this via `value={showEndDate ? endDateIso : ''}` — so a stale end-date does not persist. The form action must convert the empty string to `null` in the `TimelineEventRequest` body. - API result handling: check `!result.response.ok`; use `result.data!` after the ok-check; map errors via `getErrorMessage(extractErrorCode(result.error as unknown as { code?: string }))`. Never read `result.error` directly. - Required fields (`title`, `eventDate`) marked with `*` + `aria-required`; on error use icon + text + `aria-invalid`, never red border alone. RANGE end-date-before-start surfaces the same inline ⚠ cue as `WhoWhenSection`; the backend (#3) owns the hard rejection. - **Submitting state:** add a `submitting` reactive flag via `use:enhance` and apply `disabled={submitting}` on the submit button. This prevents double-submit and provides essential feedback for the senior audience on slower connections (authors on 60+ laptop/tablet). The `use:enhance` callback sets `submitting = true` on start and resets it on complete. - Senior-audience a11y (author path skews 60+ on laptop/tablet): 16px+ body, 48px targets, redundant non-color cues, no timed interactions, `aria-live="polite"` on the revealed end-date. Inherit `WhoWhenSection`'s treatment; do not regress it. Card pattern per section (`rounded-sm border border-line bg-surface shadow-sm p-6`), not one flat stack. `<h2 class="text-xs font-bold uppercase tracking-widest text-ink-3 mb-5">` section titles. Run `axe` on the route in both light and dark mode (dark mode remaps all color tokens — verify end-date error text contrast). - Back navigation: shared `<BackButton>`, never a static `<a href>`. - Keyed `{#each}` on precision options and picker chips (`(p.value)`, `(person.id)`). - i18n keys for all labels/errors/empty-states in de/en/es. Includes document picker empty state ("Noch kein Dokument verknüpft"). No new `ErrorCode` / `errors.ts` work needed. ## Delete - **Placement:** a "Löschen" button on the `/[id]/edit` form, behind `getConfirmService()` confirmation → `DELETE /api/timeline/events/{id}` → redirect. Keeps this issue self-contained, fulfills the AC, and matches the CRUD-form mental model. Card-level delete in the #7 timeline view is a separate, additive concern. - The confirm dialog uses `ConfirmDialog`'s destructive button variant; the dialog already handles keyboard trapping and `aria-modal`. - **`ConfirmDialog.svelte` patch:** add `aria-modal="true"` to the `<dialog>` element (one-line patch, benefits all uses). The HTML `<dialog showModal()>` has implicit modal semantics in modern browsers but `aria-modal="true"` is best practice for older AT. - **Delete action failure:** if the DELETE returns `!ok`, return `fail(status, { error: getErrorMessage(...) })` and surface the error in the form — do not redirect. ## Navigation target (post-save / post-delete) Context-aware redirect. If the form was launched via `?personId=`, return to that person's page (`/persons/{personId}`); otherwise redirect to `/zeitstrahl`. Thread the originating context through a hidden `<input type="hidden" name="originPersonId">` field. **Validate `originPersonId`:** in the action, accept the value only if it is a non-empty UUID-format string; default to `/zeitstrahl` on empty or malformed values to prevent an open redirect on a crafted hidden field. ## Validation failure re-render (picker persistence) When the form action returns `fail(400, { ... })`, include `personIds` and `documentIds` arrays in the payload so the pickers re-populate on re-render. The `load` function seeds `initialPersons`/`initialDocuments` from these IDs on re-render (same path as the prefill lookup). This matches the "re-rendered with entered values preserved" AC for all fields, not just scalars. ## Acceptance criteria (Given-When-Then) - *Given* a curator, *when* they use `/new` or `/[id]/edit`, *then* they can create, edit, and delete events. *Given* a non-curator (logged out or READ_ALL only), *when* they navigate to either route, *then* a 403 error page is shown. - *Given* precision = RANGE, *when* the form renders, *then* the end-date field is visible (and required); *given* precision ≠ RANGE, *then* it is hidden and `eventDateEnd` is submitted as `null` (not a stale date or empty string). - *Given* precision is changed from RANGE to YEAR on an edit, *when* the form is submitted, *then* `eventDateEnd` is sent as `null`. - *Given* a blank title, *when* the curator submits, *then* the form shows a localized required-field error, does not navigate away, and re-renders with all entered values preserved — including pre-selected persons/documents. - *Given* an unknown or derived event id, *when* the edit route loads, *then* a 404 is shown — never a blank create form. - *Given* a curator deletes an event and confirms, *when* the DELETE succeeds, *then* the event is removed and they return to `/zeitstrahl` (or the originating person page if launched via `?personId`); *when* the DELETE fails, *then* the error is surfaced in the form and no redirect occurs. - *Given* `initialPersons`/`initialDocuments` (prefill), *when* the form renders, *then* those persons/documents are pre-selected; *given* an unknown prefill id, *then* it is silently ignored (no "not found" leak). - Linking persons/documents works; pickers have labelled controls, visible empty states, and ≥44px remove targets. ## Tests **Component (`*.svelte.spec.ts`, vitest-browser, single-file local runs):** - `should_reveal_end_date_field_when_precision_is_RANGE` and `should_hide_end_date_field_when_precision_is_YEAR` (headline AC). Assert via `getByLabelText`/`getByRole` + `toBeVisible()`/not-present, not internal state. - `should_preselect_person_when_initialPersons_provided`. - `should_show_required_error_when_title_is_blank`. - `should_submit_null_for_eventDateEnd_when_precision_changed_from_RANGE_to_YEAR`. - RANGE end-date-before-start surfaces the inline cue (component layer); the hard rejection is owned by #3's backend test — ensure neither layer leaves the constraint unasserted. - Factory `makeEvent(overrides)` returning the `TimelineEvent` shape — do not repeat the builder per test. Document the minimal required shape in a comment in the spec file once `npm run generate:api` has run after #3. **Server (`page.server.spec.ts`, import `load`/`actions`, mock fetch/api client):** - `load` throws 403 for a non-curator (mirror `persons/new/page.server.spec.ts`). - `load` on `/[id]/edit` throws 404 when the API returns `!ok` (covers derived-event / unknown-id / any non-ok status — regression test). - action maps `fail(400, { ..., personIds, documentIds })` on blank title with all values preserved (including picker arrays). - action maps API error via `getErrorMessage` and `redirect(303)` on success. - delete action redirects to the resolved context target on DELETE success. - delete action returns `fail(status, { error: getErrorMessage(...) })` when DELETE returns `!ok`. - `originPersonId` redirect validation: action defaults to `/zeitstrahl` when `originPersonId` is empty, non-UUID, or absent. **Regression (`WhoWhenSection.svelte.spec.ts`):** - Run green after `DatePrecisionField` extraction — verify existing `data-testid` selectors (`who-when-end-date`, `who-when-precision`) are preserved and no behavior regresses. **E2E (Playwright — keep thin, one critical journey + security counterpart):** - Curator logs in → creates an event with precision RANGE → verifies the route returns HTTP 200 on `/zeitstrahl`. (Full "sees the event card" assertion depends on #7 — if this issue ships before #7, scope to HTTP 200 only, not card presence.) - Logged-out / READ_ALL-only user navigating to `/zeitstrahl/events/new` is blocked (403). Do not push field/precision permutations to E2E — those belong at the component layer. ## Task checklist - [ ] **Patch `ConfirmDialog.svelte`:** add `aria-modal="true"` to the `<dialog>` element (one-line patch). - [ ] **Extract `DatePrecisionField.svelte`** into `$lib/shared/primitives/` from `WhoWhenSection`'s date+precision+RANGE-end-date region; preserve `aria-live="polite"` around the full `{#if showEndDate}` block, `$bindable` props for `precision`/`endDateIso`, and `data-testid` attributes compatible with the existing `WhoWhenSection` spec. Refactor `WhoWhenSection` to consume it. - [ ] **Run `WhoWhenSection.svelte.spec.ts` green** after extraction — verify no data-testid selectors or behavior regressed. - [ ] `EventTypeSelect.svelte` — segmented radio group modelled on `PersonTypeSelector.svelte` (`role="radiogroup"`, `radioGroupNav`, `tabindex` roving); icon (`aria-hidden="true"`) + text label per option; two options: PERSONAL / HISTORICAL. - [ ] `EventForm.svelte` — title, type, `DatePrecisionField`, description, `PersonMultiSelect`, `DocumentMultiSelect`; optional `event`/`initialPersons`/`initialDocuments` props; conditional-spread optionals; explicit `null` for `eventDateEnd` off RANGE; `submitting` flag + `disabled={submitting}` on submit. - [ ] Route `/zeitstrahl/events/new` (`+page.server.ts` load + action) with `?personId`/`?documentId` prefill (404/403 swallowed, return `initialPersons: []`). - [ ] Route `/zeitstrahl/events/[id]/edit` (load seeds form, `throw error(404)` on any `!ok` response; action for update + delete; delete uses `getConfirmService()`). - [ ] Delete action: `fail(status, { error })` on DELETE `!ok`; redirect on success. - [ ] `originPersonId` validation in action: UUID-format check; default to `/zeitstrahl` on invalid/empty. - [ ] `fail(400)` payload includes `personIds`/`documentIds` arrays so pickers re-populate on re-render. - [ ] Permission-gate both routes server-side (403 via `locals.user.groups`; string comparison against `'WRITE_ALL'`; add comment mirroring `Permission.WRITE_ALL`); hide entry-point buttons for non-curators. - [ ] `DocumentMultiSelect` remove button: add `min-h-[44px] min-w-[44px]` (or equivalent padded hit area). Add code comment that bare `fetch('/api/...')` is intentional (same-origin in prod, Vite-proxied in dev — matches Geschichte editor). - [ ] `PersonMultiSelect` remove button: verify ≥44px target; fix if needed. - [ ] Picker empty states: "Noch keine Person verknüpft" and "Noch kein Dokument verknüpft" (localized). - [ ] Labelled pickers; `<BackButton>`; card-pattern sections; senior-a11y treatment; `axe` light+dark. - [ ] i18n keys (de/en/es) — including document picker empty state. - [ ] Update `CLAUDE.md` route table + frontend C4 diagram with `events/new` and `events/[id]/edit` children. - [ ] Tests per the strategy above (component, server, regression, E2E thin). ## Decisions resolved (Round 1) 1. **Delete control** → Option (a): "Löschen" button on the edit form behind `getConfirmService()` confirmation, DELETE then redirect. *Rationale:* keeps the issue self-contained, fulfills the AC's "delete" now rather than deferring to #7, and matches the standard CRUD-form mental model. 2. **Post-save / post-delete navigation** → Context-aware: return to the originating person page when launched via `?personId=`, else `/zeitstrahl`. Thread via hidden `originPersonId` field; validate as UUID before using in redirect. *Rationale:* friendliest for the "add event from a person" flow at trivial cost, with a safe default and open-redirect protection. 3. **Route-gating idiom** → 403 error page (the `persons/new` idiom), adopted as the project convention for permission-gated author routes. *Rationale:* honest about why access was denied; consistent with a server-side `load` guard; gentler redirects are appropriate for read paths, not author tools. 4. **EventType control** → Segmented radio group (icon + text per option), modelled on `PersonTypeSelector.svelte`. *Rationale:* keyboard-trivial for a binary choice, provides redundant non-color cues out of the box. 5. **`DatePrecisionField.svelte` location** → `$lib/shared/primitives/`. *Rationale:* it is a generic date-input primitive shared by two domains; placing it in either consumer's domain would create a cross-domain import; no new diagram entry needed. 6. **`initialPersons`/`initialDocuments` in `fail(400)` payload** → Include `personIds`/`documentIds` arrays in the fail payload and re-seed pickers on re-render. *Rationale:* consistent with "all entered values preserved" AC; dropping picker state on a required-field error is a significant UX regression for the senior audience. 7. **`ConfirmDialog` `aria-modal` patch** → Add `aria-modal="true"` to the `<dialog>` element now (one-line patch). *Rationale:* zero downside, benefits all dialog uses, best practice for AT compatibility across browsers. 8. **Submit/submitting state** → Add `submitting` flag + `disabled={submitting}` via `use:enhance`. *Rationale:* prevents double-submit and provides essential feedback for the senior audience on slower connections; trivial to implement with `use:enhance`. 9. **`DocumentMultiSelect` typeahead auth path** → Keep bare `fetch('/api/...')` as-is (intentional, matches Geschichte editor), add a code comment. *Rationale:* already works in dev (Vite proxy) and prod (same-origin); a `+server.ts` proxy adds complexity with no security benefit.

marcel added this to the Zeitstrahl — Family Timeline milestone 2026-06-07 19:29:32 +02:00

marcel added the P2-medium feature ui labels 2026-06-07 19:30:05 +02:00

marcel commented

2026-06-07 19:53:47 +02:00

🏗️ Markus Keller — Application Architect

Observations

geschichten/new gating inconsistency — do not copy that pattern. The issue correctly says to follow persons/new (throw error(403, 'Forbidden')). Good. I verified geschichten/new uses redirect(303, '/geschichten') for non-writers — that's a weaker precedent that obscures why access was denied. The issue has already made the right call here; implementors should not reach for geschichten/new as the gating template.

canWrite from layout data vs. locals.user check. The layout server exposes canWrite (from +layout.server.ts). The issue correctly directs the load function to read locals.user.groups server-side and throw 403 — not to derive the gate from data.canWrite. data.canWrite flows through the page props and could be stale on a navigation without a full load. The server-side locals check is the right approach; the comment mirroring Permission.WRITE_ALL is a good documentation touch.

Doc update list is correct but incomplete in one area. The issue calls out CLAUDE.md route table and frontend C4 diagram. The timeline/ frontend lib directory ($lib/timeline/) belongs in the CLAUDE.md lib structure table as well. For this issue specifically: EventForm.svelte, EventTypeSelect.svelte, and DatePrecisionField.svelte land in two directories. DatePrecisionField in $lib/shared/primitives/ needs no new entry (it's a primitive). EventForm and EventTypeSelect should land in $lib/timeline/ per the spec — that directory needs a CLAUDE.md lib entry when it is first created (that's issue #7's territory, but if this issue creates the directory, it triggers the update).

No new DB migration, no backend package, no ADR. Correct. The issue is purely frontend. DatePrecision already exists in the document/ package — the extracted DatePrecisionField.svelte reuses the existing frontend DatePrecision type from $lib/shared/utils/documentDate. No cross-domain backend concern.

originPersonId open-redirect protection. The UUID format check before using originPersonId in a redirect is correct and necessary. The form posts a hidden field — even though it's a same-origin redirect target, a crafted form submission could set it to an arbitrary path. Validating as UUID-format before constructing /persons/{originPersonId} is the right gate. Document the threat model in a comment in the action.

WhoWhenSection refactor scope. The DatePrecisionField extraction touches WhoWhenSection.svelte, which is consumed by the document edit/upload forms. The regression test requirement is correctly identified. The aria-live="polite" wrapper must stay on the outer <div> around {#if showEndDate}, not be moved inside the extracted component — the spec is explicit on this and it's the right call (the live region must announce when the region appears, not just the content inside it).

Recommendations

Add a comment in the route load function: // WRITE_ALL check mirrors Permission.WRITE_ALL — server-side gate; frontend canWrite flag is for hiding entry-point buttons only. This prevents a future developer from thinking the data.canWrite flag is the real security boundary.
When creating $lib/timeline/ as a directory (even if just for EventForm.svelte), add the entry to CLAUDE.md lib structure table in the same commit.
The fail(400) payload must include personIds and documentIds as arrays (not objects). Use formData.getAll('personIds') to collect the multi-value hidden inputs. The issue specifies this; make sure the load re-seeding path uses the same array-of-strings format the pickers expect.

## 🏗️ Markus Keller — Application Architect ### Observations **`geschichten/new` gating inconsistency — do not copy that pattern.** The issue correctly says to follow `persons/new` (`throw error(403, 'Forbidden')`). Good. I verified `geschichten/new` uses `redirect(303, '/geschichten')` for non-writers — that's a weaker precedent that obscures why access was denied. The issue has already made the right call here; implementors should not reach for `geschichten/new` as the gating template. **`canWrite` from layout data vs. `locals.user` check.** The layout server exposes `canWrite` (from `+layout.server.ts`). The issue correctly directs the `load` function to read `locals.user.groups` server-side and throw 403 — not to derive the gate from `data.canWrite`. `data.canWrite` flows through the page props and could be stale on a navigation without a full load. The server-side `locals` check is the right approach; the comment mirroring `Permission.WRITE_ALL` is a good documentation touch. **Doc update list is correct but incomplete in one area.** The issue calls out `CLAUDE.md` route table and frontend C4 diagram. The `timeline/` frontend lib directory (`$lib/timeline/`) belongs in the `CLAUDE.md` lib structure table as well. For this issue specifically: `EventForm.svelte`, `EventTypeSelect.svelte`, and `DatePrecisionField.svelte` land in two directories. `DatePrecisionField` in `$lib/shared/primitives/` needs no new entry (it's a primitive). `EventForm` and `EventTypeSelect` should land in `$lib/timeline/` per the spec — that directory needs a `CLAUDE.md` lib entry when it is first created (that's issue #7's territory, but if this issue creates the directory, it triggers the update). **No new DB migration, no backend package, no ADR.** Correct. The issue is purely frontend. `DatePrecision` already exists in the `document/` package — the extracted `DatePrecisionField.svelte` reuses the existing frontend `DatePrecision` type from `$lib/shared/utils/documentDate`. No cross-domain backend concern. **`originPersonId` open-redirect protection.** The UUID format check before using `originPersonId` in a redirect is correct and necessary. The form posts a hidden field — even though it's a same-origin redirect target, a crafted form submission could set it to an arbitrary path. Validating as UUID-format before constructing `/persons/{originPersonId}` is the right gate. Document the threat model in a comment in the action. **`WhoWhenSection` refactor scope.** The `DatePrecisionField` extraction touches `WhoWhenSection.svelte`, which is consumed by the document edit/upload forms. The regression test requirement is correctly identified. The `aria-live="polite"` wrapper must stay on the outer `<div>` around `{#if showEndDate}`, not be moved inside the extracted component — the spec is explicit on this and it's the right call (the live region must announce when the region appears, not just the content inside it). ### Recommendations - Add a comment in the route `load` function: `// WRITE_ALL check mirrors Permission.WRITE_ALL — server-side gate; frontend canWrite flag is for hiding entry-point buttons only.` This prevents a future developer from thinking the `data.canWrite` flag is the real security boundary. - When creating `$lib/timeline/` as a directory (even if just for `EventForm.svelte`), add the entry to `CLAUDE.md` lib structure table in the same commit. - The `fail(400)` payload must include `personIds` and `documentIds` as arrays (not objects). Use `formData.getAll('personIds')` to collect the multi-value hidden inputs. The issue specifies this; make sure the `load` re-seeding path uses the same array-of-strings format the pickers expect.

marcel commented

2026-06-07 19:54:13 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

DatePrecisionField extraction — the onMount seeding pattern must be preserved. WhoWhenSection uses an onMount to seed dateDisplay from dateIso || initialDateIso exactly once, so later prop changes don't stomp the user's input. This logic must transfer to DatePrecisionField unchanged. The existing WhoWhenSection.svelte.spec.ts doesn't test end-date behavior or RANGE precision at all — it only tests the date field and location. The regression spec referenced in the issue will be the first test of the extracted end-date region's behavior.

EventTypeSelect.svelte — two options, not four. PersonTypeSelector.svelte has four types rendered in a grid-cols-2 sm:grid-cols-4 grid. EventTypeSelect needs only PERSONAL / HISTORICAL. The radioGroupNav action takes a callback with the selected value — wire it identically. The sr-only aria-live announcement div (the announcement state + a11y_type_changed message) should be replicated in EventTypeSelect for the same screen-reader UX.

EventForm props interface — use specific types. When TimelineEvent and TimelineEventRequest types are generated from the OpenAPI spec (after #3 merges), the event prop should be typed as TimelineEvent | undefined, not object or any. The factory function makeEvent(overrides) in the component spec file should document the minimal shape needed in a comment, as the issue directs.

DocumentMultiSelect remove button — the hit-area fix is concrete. The current remove button class is "ml-0.5 text-ink/50 hover:text-red-500 focus:outline-none" with a 12×12px SVG inside. Adding min-h-[44px] min-w-[44px] to the button and flex items-center justify-center will expand the hit area without changing visual size. The SVG stays 12×12; the padding absorbs the target. Don't forget PersonMultiSelect's remove button too — verify it before assuming it passes.

Conditional spread for eventDateEnd. The issue specifies sending eventDateEnd: null when precision leaves RANGE. The DatePrecisionField hides the end-date input when !showEndDate and the hidden <input> emits value={showEndDate ? endDateIso : ''}. The form action must convert empty string to null in the TimelineEventRequest body. This is a one-liner: const eventDateEnd = precision === 'RANGE' && endDateIsoRaw ? endDateIsoRaw : null. Getting this wrong leaves stale end-dates in the database from edit round-trips.

$bindable on precision and endDateIso. These must be $bindable so EventForm can read the current values when assembling the form body (even though the form also has hidden inputs). The issue is correct that the WhoWhenSection bindable pattern must be preserved. Don't add $state redundantly alongside $bindable — the parent's binding IS the state.

submitting flag pattern. The use:enhance callback form: use:enhance={() => { submitting = true; return async ({ update }) => { submitting = false; await update(); }; }}. The disabled={submitting} on the submit button should also visually reflect the state — add a spinner or opacity-50 class while submitting for the senior audience.

Recommendations

Write the DatePrecisionField component spec first (red), extract the component second (green). The should_reveal_end_date_field_when_precision_is_RANGE test gives the red signal before any extraction happens.
Name the hidden ISO input for event date name="eventDate" to match the API field directly — avoids a translation step in the form action.
In EventForm, extract a $derived for showEndDate = $derived(precision === 'RANGE') rather than repeating precision === 'RANGE' in multiple places.
After the DatePrecisionField extraction, run WhoWhenSection.svelte.spec.ts immediately in isolation (--project=client src/lib/document/WhoWhenSection.svelte.spec.ts) before touching EventForm — ensures the regression fence is green before adding new surface.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations **`DatePrecisionField` extraction — the `onMount` seeding pattern must be preserved.** `WhoWhenSection` uses an `onMount` to seed `dateDisplay` from `dateIso || initialDateIso` exactly once, so later prop changes don't stomp the user's input. This logic must transfer to `DatePrecisionField` unchanged. The existing `WhoWhenSection.svelte.spec.ts` doesn't test end-date behavior or RANGE precision at all — it only tests the date field and location. The regression spec referenced in the issue will be the first test of the extracted end-date region's behavior. **`EventTypeSelect.svelte` — two options, not four.** `PersonTypeSelector.svelte` has four types rendered in a `grid-cols-2 sm:grid-cols-4` grid. `EventTypeSelect` needs only `PERSONAL` / `HISTORICAL`. The `radioGroupNav` action takes a callback with the selected value — wire it identically. The `sr-only` `aria-live` announcement div (the `announcement` state + `a11y_type_changed` message) should be replicated in `EventTypeSelect` for the same screen-reader UX. **`EventForm` props interface — use specific types.** When `TimelineEvent` and `TimelineEventRequest` types are generated from the OpenAPI spec (after #3 merges), the `event` prop should be typed as `TimelineEvent | undefined`, not `object` or `any`. The factory function `makeEvent(overrides)` in the component spec file should document the minimal shape needed in a comment, as the issue directs. **`DocumentMultiSelect` remove button — the hit-area fix is concrete.** The current remove button class is `"ml-0.5 text-ink/50 hover:text-red-500 focus:outline-none"` with a 12×12px SVG inside. Adding `min-h-[44px] min-w-[44px]` to the button and `flex items-center justify-center` will expand the hit area without changing visual size. The SVG stays 12×12; the padding absorbs the target. Don't forget `PersonMultiSelect`'s remove button too — verify it before assuming it passes. **Conditional spread for `eventDateEnd`.** The issue specifies sending `eventDateEnd: null` when precision leaves RANGE. The `DatePrecisionField` hides the end-date input when `!showEndDate` and the hidden `<input>` emits `value={showEndDate ? endDateIso : ''}`. The form action must convert empty string to `null` in the `TimelineEventRequest` body. This is a one-liner: `const eventDateEnd = precision === 'RANGE' && endDateIsoRaw ? endDateIsoRaw : null`. Getting this wrong leaves stale end-dates in the database from edit round-trips. **`$bindable` on `precision` and `endDateIso`.** These must be `$bindable` so `EventForm` can read the current values when assembling the form body (even though the form also has hidden inputs). The issue is correct that the `WhoWhenSection` bindable pattern must be preserved. Don't add `$state` redundantly alongside `$bindable` — the parent's binding IS the state. **`submitting` flag pattern.** The `use:enhance` callback form: `use:enhance={() => { submitting = true; return async ({ update }) => { submitting = false; await update(); }; }}`. The `disabled={submitting}` on the submit button should also visually reflect the state — add a spinner or `opacity-50` class while submitting for the senior audience. ### Recommendations - Write the `DatePrecisionField` component spec first (red), extract the component second (green). The `should_reveal_end_date_field_when_precision_is_RANGE` test gives the red signal before any extraction happens. - Name the hidden ISO input for event date `name="eventDate"` to match the API field directly — avoids a translation step in the form action. - In `EventForm`, extract a `$derived` for `showEndDate = $derived(precision === 'RANGE')` rather than repeating `precision === 'RANGE'` in multiple places. - After the `DatePrecisionField` extraction, run `WhoWhenSection.svelte.spec.ts` immediately in isolation (`--project=client src/lib/document/WhoWhenSection.svelte.spec.ts`) before touching `EventForm` — ensures the regression fence is green before adding new surface.

marcel commented

2026-06-07 19:54:37 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Observations

originPersonId open-redirect — the threat model is real. The hidden <input type="hidden" name="originPersonId"> is user-controlled in the sense that a crafted form POST can set it to any string. The action's UUID format validation before using it in redirect(303, '/persons/{originPersonId}') is correct. I'd add: use a strict UUID regex, not just a non-empty check. In TypeScript: const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; const safeOrigin = UUID_RE.test(originPersonIdRaw) ? \/persons/${originPersonIdRaw}` : '/zeitstrahl';`. This closes the redirect regardless of what a crafted form submits.

DocumentMultiSelect bare fetch('/api/...') — confirmed intentional, confirmed safe. The component already does this pattern and the issue explicitly calls it intentional (same-origin in prod, Vite-proxied in dev). The auth cookie travels automatically on same-origin requests. Adding the code comment is the right call. No action needed beyond the comment.

Frontend permission gate is UX, not a security boundary — correctly stated. The load guard returning 403 stops casual users; the backend @RequirePermission(WRITE_ALL) on the CRUD endpoints stops crafted requests. The issue is clear on this layering. One thing to verify during implementation: the GET /api/timeline/events/{id} endpoint (for the edit form load) is protected by READ_ALL per the spec. A logged-out user navigating to /zeitstrahl/events/123/edit should hit the server-side load which checks locals.user — if locals.user is null, the canWrite check will throw 403 before the API call is even made. Confirm the load function handles the unauthenticated case (null user) as 403, not as an unhandled TypeError.

Server-side permission check implementation — string comparison risk. The issue directs using a string comparison against 'WRITE_ALL'. Looking at persons/new, the pattern is: locals.user?.groups?.some((g) => g.permissions.includes('WRITE_ALL')) ?? false. This is safe because includes() is exact-match on strings. The comment directing developers to mirror Permission.WRITE_ALL is good hygiene — it prevents drift if the permission string ever changes. Recommend extracting a helper hasPermission(locals, 'WRITE_ALL') in $lib/shared/server/ so the check is one line and centrally testable.

fail(400) payload with personIds/documentIds — no injection risk. These are UUIDs collected from hidden <input> elements. They're re-used as GET /api/persons/{id} path params in the load re-seeding path. Verify that the load function validates each ID before passing to the API (the API will 404/403 on invalid IDs and the spec says to swallow those — so even a malformed ID just results in an empty picker, which is fine).

ConfirmDialog aria-modal patch — confirmed missing. I checked the source: the <dialog> element has aria-labelledby="confirm-title" but no aria-modal="true". The one-line patch is correct. No security implication, but it is the right AT practice.

Recommendations

Add a strict UUID regex constant in the form action for originPersonId validation. Document the threat: // Prevents open redirect: validate before constructing /persons/{id}. See OWASP: CWE-601.
In the load function, add an explicit null-user guard before the canWrite check: if (!locals.user) throw error(403, 'Forbidden'); — this avoids a potential TypeError on locals.user.groups when an unauthenticated request reaches a route that wasn't caught by the global auth hook.
The extractErrorCode pattern (result.error as unknown as { code?: string }) is correct per project conventions — do not use raw result.error.message anywhere in the form or action.

## 🔐 Nora "NullX" Steiner — Application Security Engineer ### Observations **`originPersonId` open-redirect — the threat model is real.** The hidden `<input type="hidden" name="originPersonId">` is user-controlled in the sense that a crafted form POST can set it to any string. The action's UUID format validation before using it in `redirect(303, '/persons/{originPersonId}')` is correct. I'd add: use a strict UUID regex, not just a non-empty check. In TypeScript: `const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; const safeOrigin = UUID_RE.test(originPersonIdRaw) ? \`/persons/\${originPersonIdRaw}\` : '/zeitstrahl';`. This closes the redirect regardless of what a crafted form submits. **`DocumentMultiSelect` bare `fetch('/api/...')` — confirmed intentional, confirmed safe.** The component already does this pattern and the issue explicitly calls it intentional (same-origin in prod, Vite-proxied in dev). The auth cookie travels automatically on same-origin requests. Adding the code comment is the right call. No action needed beyond the comment. **Frontend permission gate is UX, not a security boundary — correctly stated.** The `load` guard returning 403 stops casual users; the backend `@RequirePermission(WRITE_ALL)` on the CRUD endpoints stops crafted requests. The issue is clear on this layering. One thing to verify during implementation: the `GET /api/timeline/events/{id}` endpoint (for the edit form `load`) is protected by `READ_ALL` per the spec. A logged-out user navigating to `/zeitstrahl/events/123/edit` should hit the server-side `load` which checks `locals.user` — if `locals.user` is null, the `canWrite` check will throw 403 before the API call is even made. Confirm the `load` function handles the unauthenticated case (null user) as 403, not as an unhandled TypeError. **Server-side permission check implementation — string comparison risk.** The issue directs using a string comparison against `'WRITE_ALL'`. Looking at `persons/new`, the pattern is: `locals.user?.groups?.some((g) => g.permissions.includes('WRITE_ALL')) ?? false`. This is safe because `includes()` is exact-match on strings. The comment directing developers to mirror `Permission.WRITE_ALL` is good hygiene — it prevents drift if the permission string ever changes. Recommend extracting a helper `hasPermission(locals, 'WRITE_ALL')` in `$lib/shared/server/` so the check is one line and centrally testable. **`fail(400)` payload with `personIds`/`documentIds` — no injection risk.** These are UUIDs collected from hidden `<input>` elements. They're re-used as `GET /api/persons/{id}` path params in the `load` re-seeding path. Verify that the `load` function validates each ID before passing to the API (the API will 404/403 on invalid IDs and the spec says to swallow those — so even a malformed ID just results in an empty picker, which is fine). **`ConfirmDialog` `aria-modal` patch — confirmed missing.** I checked the source: the `<dialog>` element has `aria-labelledby="confirm-title"` but no `aria-modal="true"`. The one-line patch is correct. No security implication, but it is the right AT practice. ### Recommendations - Add a strict UUID regex constant in the form action for `originPersonId` validation. Document the threat: `// Prevents open redirect: validate before constructing /persons/{id}. See OWASP: CWE-601.` - In the `load` function, add an explicit null-user guard before the `canWrite` check: `if (!locals.user) throw error(403, 'Forbidden');` — this avoids a potential TypeError on `locals.user.groups` when an unauthenticated request reaches a route that wasn't caught by the global auth hook. - The `extractErrorCode` pattern (`result.error as unknown as { code?: string }`) is correct per project conventions — do not use raw `result.error.message` anywhere in the form or action.

marcel commented

2026-06-07 19:55:03 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

WhoWhenSection.svelte.spec.ts regression scope is narrow — and that's a risk. The existing spec tests only: date pre-fill from initialDateIso, hideDate mode, editMode location field, and initialLocation pre-fill. It does NOT test: precision selector behavior, RANGE end-date reveal, or endDateIso binding. After the DatePrecisionField extraction, those behaviors live in the new primitive — but there are no existing tests for them that could go red. The regression fence for the extraction is therefore weaker than the issue implies. Recommendation: before extracting, write a test in WhoWhenSection.svelte.spec.ts that asserts RANGE end-date reveals when precision = 'RANGE'. This gives a real red/green signal for the extraction.

Component spec coverage gaps. The issue lists five component tests for DatePrecisionField/EventForm. Missing from the list:

should_disable_submit_button_while_submitting — the submitting flag is a named AC but has no named test.
should_show_error_inline_when_api_returns_error_on_save — the form action maps API errors, but no component test verifies the error renders in the form.
should_not_submit_eventDateEnd_when_precision_changes_from_RANGE_to_YEAR is listed but needs clarification: at the component layer, this is asserting the hidden input's value, not the network payload. At the server test layer, the form action conversion of '' → null is what matters.

Server spec coverage — load on /[id]/edit with fail payload re-seeding. The issue specifies that load re-seeds pickers from personIds/documentIds in the fail payload. But this is only triggered when the action returns fail(400, { personIds, documentIds }) and SvelteKit re-renders the page with data.form. The load function itself doesn't participate in re-seeding — it's the form data in the page that provides picker re-population. Verify whether the issue intends the load to actively re-fetch from the form.personIds or whether the page component reads form.personIds directly to pre-populate picker state. This affects whether a server spec for load re-seeding is even the right test location.

E2E scope is correctly thin. The two E2E scenarios (curator creates event + non-curator blocked) are appropriate. The conditional scope note about #7 ("HTTP 200 only if #7 hasn't shipped") is good — it prevents a test that passes only in the combined state. Implement the E2E assertion as: await expect(response.status()).toBe(200) on the /zeitstrahl redirect target, not a DOM assertion about card presence.

delete action returns fail(status, { error }) on !ok — need explicit test. The spec lists this as a server test. The test should verify: (a) DELETE returns 500 from API → action returns fail(500, { error: getErrorMessage(...) }) and (b) no redirect occurs. Use expect(result.status).toBe(500) on the action return value and assert no Location header.

WhoWhenSection.svelte.spec.ts data-testid compatibility. After extraction, the spec uses page.getByTestId('who-when-end-date') and page.getByTestId('who-when-precision'). These data-testid attributes live on the wrapping divs inside WhoWhenSection, not inside DatePrecisionField itself. As long as DatePrecisionField is rendered inside those same wrapper divs in WhoWhenSection, the selectors survive. If the extraction moves those divs INTO DatePrecisionField, the testids must be exposed as props or via data-testid forwarding. The issue anticipates data-testid="end-date-region" on the range-revealed container — confirm this is on the DatePrecisionField's outer wrapper, not the inner content div.

Recommendations

Add one test to WhoWhenSection.svelte.spec.ts NOW (before extraction) that asserts RANGE end-date reveals: render(WhoWhenSection, { precision: 'RANGE' }) → expect(getByLabelText('Enddatum')).toBeVisible(). This gives a true red before the extraction and green after.
Clarify the picker re-population test location: if it's form.personIds read directly in the component, write a component spec. If the load re-fetches from those IDs, write a server spec. Don't write both without clarifying what each is testing.
Add a dedicated test for the submitting flag: render EventForm, trigger submit, assert button has disabled attribute before the action completes.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations **`WhoWhenSection.svelte.spec.ts` regression scope is narrow — and that's a risk.** The existing spec tests only: date pre-fill from `initialDateIso`, `hideDate` mode, `editMode` location field, and `initialLocation` pre-fill. It does NOT test: precision selector behavior, RANGE end-date reveal, or `endDateIso` binding. After the `DatePrecisionField` extraction, those behaviors live in the new primitive — but there are no existing tests for them that could go red. The regression fence for the extraction is therefore weaker than the issue implies. **Recommendation:** before extracting, write a test in `WhoWhenSection.svelte.spec.ts` that asserts RANGE end-date reveals when precision = 'RANGE'. This gives a real red/green signal for the extraction. **Component spec coverage gaps.** The issue lists five component tests for `DatePrecisionField`/`EventForm`. Missing from the list: - `should_disable_submit_button_while_submitting` — the `submitting` flag is a named AC but has no named test. - `should_show_error_inline_when_api_returns_error_on_save` — the form action maps API errors, but no component test verifies the error renders in the form. - `should_not_submit_eventDateEnd_when_precision_changes_from_RANGE_to_YEAR` is listed but needs clarification: at the component layer, this is asserting the hidden input's value, not the network payload. At the server test layer, the form action conversion of `''` → `null` is what matters. **Server spec coverage — `load` on `/[id]/edit` with `fail` payload re-seeding.** The issue specifies that `load` re-seeds pickers from `personIds`/`documentIds` in the fail payload. But this is only triggered when the action returns `fail(400, { personIds, documentIds })` and SvelteKit re-renders the page with `data.form`. The `load` function itself doesn't participate in re-seeding — it's the `form` data in the page that provides picker re-population. Verify whether the issue intends the `load` to actively re-fetch from the `form.personIds` or whether the page component reads `form.personIds` directly to pre-populate picker state. This affects whether a server spec for `load` re-seeding is even the right test location. **E2E scope is correctly thin.** The two E2E scenarios (curator creates event + non-curator blocked) are appropriate. The conditional scope note about #7 ("HTTP 200 only if #7 hasn't shipped") is good — it prevents a test that passes only in the combined state. Implement the E2E assertion as: `await expect(response.status()).toBe(200)` on the `/zeitstrahl` redirect target, not a DOM assertion about card presence. **`delete action returns fail(status, { error })` on `!ok` — need explicit test.** The spec lists this as a server test. The test should verify: (a) DELETE returns 500 from API → action returns `fail(500, { error: getErrorMessage(...) })` and (b) no redirect occurs. Use `expect(result.status).toBe(500)` on the action return value and assert no `Location` header. **`WhoWhenSection.svelte.spec.ts` `data-testid` compatibility.** After extraction, the spec uses `page.getByTestId('who-when-end-date')` and `page.getByTestId('who-when-precision')`. These `data-testid` attributes live on the wrapping divs inside `WhoWhenSection`, not inside `DatePrecisionField` itself. As long as `DatePrecisionField` is rendered inside those same wrapper divs in `WhoWhenSection`, the selectors survive. If the extraction moves those divs INTO `DatePrecisionField`, the testids must be exposed as props or via `data-testid` forwarding. The issue anticipates `data-testid="end-date-region"` on the range-revealed container — confirm this is on the `DatePrecisionField`'s outer wrapper, not the inner content div. ### Recommendations - Add one test to `WhoWhenSection.svelte.spec.ts` NOW (before extraction) that asserts RANGE end-date reveals: `render(WhoWhenSection, { precision: 'RANGE' })` → `expect(getByLabelText('Enddatum')).toBeVisible()`. This gives a true red before the extraction and green after. - Clarify the picker re-population test location: if it's `form.personIds` read directly in the component, write a component spec. If the `load` re-fetches from those IDs, write a server spec. Don't write both without clarifying what each is testing. - Add a dedicated test for the `submitting` flag: render `EventForm`, trigger submit, assert button has `disabled` attribute before the action completes.

marcel commented

2026-06-07 19:55:33 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

ConfirmDialog.svelte — aria-modal confirmed missing, but the <dialog> element's native modal semantics are already partly there. The dialog uses showModal() via the $effect, which sets the native inert attribute on the rest of the DOM in modern browsers. However, aria-modal="true" is still required for older AT (NVDA + Chrome < 2022, VoiceOver on iOS < 16) that don't honor native dialog semantics. The one-line patch is correct and low risk.

EventTypeSelect — icon + text per option. The issue specifies a "decorative icon (aria-hidden='true') for the person/world accent." These icons must be truly decorative (not the only differentiator between PERSONAL and HISTORICAL). The text labels ("Persönlich" / "Historisch" or equivalent i18n keys) must be visible alongside the icon — this is the redundant non-color cue requirement. The segmented radio group layout from PersonTypeSelector already does this well; replicate the exact button classes including min-h-[48px].

DocumentMultiSelect remove button — the 12×12px SVG is the entire tap target. The current markup is <button class="ml-0.5 text-ink/50 hover:text-red-500 focus:outline-none">. Tapping a 12px target on a 60+ author's laptop/tablet is genuinely difficult. The fix is class="ml-0.5 min-h-[44px] min-w-[44px] inline-flex items-center justify-center text-ink/50 hover:text-red-500 focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring". Also add rounded-sm so the focus ring looks intentional, not clipped.

Picker empty states — placement matters. "Noch keine Person verknüpft" and "Noch kein Dokument verknüpft" should render inside the picker's chip container when it has zero selected items, not below or outside. This makes the empty state feel like part of the field. Use font-sans text-sm text-ink-3 italic to visually distinguish it from chip text. Don't use the same style as error text.

aria-live="polite" wrapper position. The issue is explicit: the aria-live="polite" wrapper must surround the entire {#if showEndDate} block, not just the error text. Looking at the existing WhoWhenSection.svelte: line 149 has <div aria-live="polite"> wrapping the {#if showEndDate} block, with the data-testid="who-when-end-date" div inside it. This is the correct structure. When extracting DatePrecisionField, the aria-live div must be on the component's outer wrapper element for the end-date region, not on a child. If the component emits a fragment, the aria-live will be misplaced.

Card pattern — use it for all sections. The issue specifies "card pattern per section." EventForm should wrap: (1) Wann / date section, (2) Wer / persons section, (3) Dokumente / documents section, (4) Beschreibung / description section — each in their own rounded-sm border border-line bg-surface shadow-sm p-6 card. Do not flatten all fields into one card. This matches how the document edit form structures its sections and aids the senior audience's spatial orientation.

BackButton — already in $lib/shared/primitives/. Use it. The issue calls this out. Looking at the component, it handles back navigation via browser history or a fallback href. Use it on both routes.

Dark mode — verify end-date error text contrast. The text-red-600 error color is used in WhoWhenSection for the ⚠ endBeforeStart cue. In dark mode, text-red-600 on dark backgrounds may not meet 4.5:1. The issue specifically calls for an axe check in dark mode. Run axe on the edit form in dark mode before marking the issue done. If contrast fails, use text-red-400 in dark mode via a conditional or a semantic error token.

Recommendations

For EventTypeSelect, use exactly two buttons in a grid-cols-2 layout (not sm:grid-cols-4 like PersonTypeSelector). Each button: icon (aria-hidden="true", ~20×20px) + text label side by side with flex items-center gap-2.
Add focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring to the DocumentMultiSelect remove button fix so keyboard users see a clear focus indicator after expanding the hit area.
The <label> for each picker must use <label for="..."> or wrap the picker in a <label>. Using <p class="..."> as the label (which is the current WhoWhenSection pattern for receivers) is not semantically correct — it won't be announced as the field label by screen readers. Switch to <label> for the new pickers.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations **`ConfirmDialog.svelte` — `aria-modal` confirmed missing, but the `<dialog>` element's native modal semantics are already partly there.** The dialog uses `showModal()` via the `$effect`, which sets the native `inert` attribute on the rest of the DOM in modern browsers. However, `aria-modal="true"` is still required for older AT (NVDA + Chrome < 2022, VoiceOver on iOS < 16) that don't honor native dialog semantics. The one-line patch is correct and low risk. **`EventTypeSelect` — icon + text per option.** The issue specifies a "decorative icon (`aria-hidden='true'`) for the person/world accent." These icons must be truly decorative (not the only differentiator between PERSONAL and HISTORICAL). The text labels ("Persönlich" / "Historisch" or equivalent i18n keys) must be visible alongside the icon — this is the redundant non-color cue requirement. The segmented radio group layout from `PersonTypeSelector` already does this well; replicate the exact button classes including `min-h-[48px]`. **`DocumentMultiSelect` remove button — the 12×12px SVG is the entire tap target.** The current markup is `<button class="ml-0.5 text-ink/50 hover:text-red-500 focus:outline-none">`. Tapping a 12px target on a 60+ author's laptop/tablet is genuinely difficult. The fix is `class="ml-0.5 min-h-[44px] min-w-[44px] inline-flex items-center justify-center text-ink/50 hover:text-red-500 focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring"`. Also add `rounded-sm` so the focus ring looks intentional, not clipped. **Picker empty states — placement matters.** "Noch keine Person verknüpft" and "Noch kein Dokument verknüpft" should render inside the picker's chip container when it has zero selected items, not below or outside. This makes the empty state feel like part of the field. Use `font-sans text-sm text-ink-3 italic` to visually distinguish it from chip text. Don't use the same style as error text. **`aria-live="polite"` wrapper position.** The issue is explicit: the `aria-live="polite"` wrapper must surround the entire `{#if showEndDate}` block, not just the error text. Looking at the existing `WhoWhenSection.svelte`: line 149 has `<div aria-live="polite">` wrapping the `{#if showEndDate}` block, with the `data-testid="who-when-end-date"` div inside it. This is the correct structure. When extracting `DatePrecisionField`, the `aria-live` div must be on the component's outer wrapper element for the end-date region, not on a child. If the component emits a fragment, the `aria-live` will be misplaced. **Card pattern — use it for all sections.** The issue specifies "card pattern per section." `EventForm` should wrap: (1) Wann / date section, (2) Wer / persons section, (3) Dokumente / documents section, (4) Beschreibung / description section — each in their own `rounded-sm border border-line bg-surface shadow-sm p-6` card. Do not flatten all fields into one card. This matches how the document edit form structures its sections and aids the senior audience's spatial orientation. **`BackButton` — already in `$lib/shared/primitives/`. Use it.** The issue calls this out. Looking at the component, it handles back navigation via browser history or a fallback href. Use it on both routes. **Dark mode — verify end-date error text contrast.** The `text-red-600` error color is used in `WhoWhenSection` for the `⚠ endBeforeStart` cue. In dark mode, `text-red-600` on dark backgrounds may not meet 4.5:1. The issue specifically calls for an `axe` check in dark mode. Run `axe` on the edit form in dark mode before marking the issue done. If contrast fails, use `text-red-400` in dark mode via a conditional or a semantic error token. ### Recommendations - For `EventTypeSelect`, use exactly two buttons in a `grid-cols-2` layout (not `sm:grid-cols-4` like `PersonTypeSelector`). Each button: icon (`aria-hidden="true"`, ~20×20px) + text label side by side with `flex items-center gap-2`. - Add `focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring` to the `DocumentMultiSelect` remove button fix so keyboard users see a clear focus indicator after expanding the hit area. - The `<label>` for each picker must use `<label for="...">` or wrap the picker in a `<label>`. Using `<p class="...">` as the label (which is the current `WhoWhenSection` pattern for receivers) is not semantically correct — it won't be announced as the field label by screen readers. Switch to `<label>` for the new pickers.

marcel commented

2026-06-07 19:55:42 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

No infrastructure, CI pipeline, Docker, or deployment concerns are introduced by this issue. The new routes are pure SvelteKit SSR — no new services, no new containers, no new environment variables, no new infrastructure entries in the Compose file.

What I verified:

No new Docker service or infrastructure component → no docker-compose.yml or docs/DEPLOYMENT.md change needed.
No new external system → no C4 L1 context update.
No new Flyway migration in this issue (migrations belong to #2/#3) → no db-orm.puml or db-relationships.puml change needed for this PR.
CI impact: the E2E tests added by this issue (playwright) need --project=client for component specs (fast, local-safe) and the full E2E suite for Playwright scenarios. Confirm the thin E2E scenarios don't require the full Docker Compose stack to be up — if they do, they must run in the CI e2e job, not the unit job. Based on the spec (curator logs in → creates event), they need the full stack.
The npm run generate:api prerequisite (after #3 merges) is a build-time requirement documented in the issue. Make sure CI's generate:api step runs in the right job order — the issue already flags this dependency.

No concerns from my angle — clean separation from infrastructure.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer No infrastructure, CI pipeline, Docker, or deployment concerns are introduced by this issue. The new routes are pure SvelteKit SSR — no new services, no new containers, no new environment variables, no new infrastructure entries in the Compose file. **What I verified:** - No new Docker service or infrastructure component → no `docker-compose.yml` or `docs/DEPLOYMENT.md` change needed. - No new external system → no C4 L1 context update. - No new Flyway migration in this issue (migrations belong to #2/#3) → no `db-orm.puml` or `db-relationships.puml` change needed for this PR. - CI impact: the E2E tests added by this issue (`playwright`) need `--project=client` for component specs (fast, local-safe) and the full E2E suite for Playwright scenarios. Confirm the thin E2E scenarios don't require the full Docker Compose stack to be up — if they do, they must run in the CI e2e job, not the unit job. Based on the spec (curator logs in → creates event), they need the full stack. - The `npm run generate:api` prerequisite (after #3 merges) is a build-time requirement documented in the issue. Make sure CI's `generate:api` step runs in the right job order — the issue already flags this dependency. No concerns from my angle — clean separation from infrastructure.

marcel commented

2026-06-07 19:56:05 +02:00

📋 Elicit — Requirements Engineer

Observations

All ACs are in Given-When-Then format — good. Three edge cases are underspecified:

Concurrent edit collision. What happens if curator A saves an event while curator B is editing the same event? The TimelineEvent entity has a version field (per the spec's audit section), suggesting optimistic locking. But the frontend issue doesn't mention handling a 409 Conflict response. If the backend returns 409 on a stale version, the form action should surface it — but there's no AC for this. Either add an AC or explicitly note "optimistic locking conflict → generic error message, no special handling."
Empty title with type="submit" and keyboard nav. The AC says "form shows a localized required-field error, does not navigate away." But there's no AC for what happens when the title field is blank AND eventDate is also blank. The form has two required fields. The issue covers blank title alone, but does not specify whether both errors render simultaneously or sequentially. Spring Boot form actions return fail(400) with a single error string currently — are we surfacing per-field errors or a single summary error? Make this explicit.
Prefill with both ?personId= and ?documentId=. The issue states the /new route accepts both params. The spec says both are looked up in parallel via Promise.all. If both are valid, both should be pre-selected. If one is 404/403, that one is silently ignored and the other is used. This is implied but not stated as an explicit AC. Add: Given ?personId=A&documentId=B where A is valid and B is 404, when /new loads, then person A is pre-selected and the document picker is empty.

Scope boundary around DatePrecisionField — cross-domain import concern. The issue explicitly places DatePrecisionField in $lib/shared/primitives/ to avoid cross-domain imports. But DatePrecisionField needs the DatePrecision type from $lib/shared/utils/documentDate and the PRECISIONS array (currently defined inline in WhoWhenSection). The PRECISIONS array uses i18n keys from $lib/paraglide/messages.js. Moving it to $lib/shared/primitives/ is fine — both of those imports are already in shared/. No cross-domain issue. However, the AC for the extraction should explicitly state which props DatePrecisionField exposes, since this is now a shared contract between two consumers (WhoWhenSection and EventForm). Consider adding a props interface to the task checklist.

RANGE end-date null on submit — the AC is precise but the test is not. The AC says "eventDateEnd is submitted as null" but null is a JSON concept. A form action receives '' (empty string) from the hidden <input value="">. The mapping '' → null happens in the action. The test should_submit_null_for_eventDateEnd_when_precision_changed_from_RANGE_to_YEAR should be a server spec (testing the form action), not a component spec — the component can only assert the hidden input's value is '', not that the API receives null. The issue lists this as a component test, which is the wrong layer.

Recommendations

Add an AC: Given ?personId=A&documentId=B where A is valid and B does not exist, when the form loads, then person A is pre-selected and the document picker shows its empty state.
Clarify in the issue whether multi-required-field validation errors are shown simultaneously or the first failing field only. This determines whether the form action returns a single error string or a per-field error map.
Move should_submit_null_for_eventDateEnd_when_precision_changed_from_RANGE_to_YEAR to the server spec layer. At the component layer, assert the hidden input value is ''; at the server layer, assert the API body has eventDateEnd: null.

## 📋 Elicit — Requirements Engineer ### Observations **All ACs are in Given-When-Then format — good. Three edge cases are underspecified:** 1. **Concurrent edit collision.** What happens if curator A saves an event while curator B is editing the same event? The `TimelineEvent` entity has a `version` field (per the spec's audit section), suggesting optimistic locking. But the frontend issue doesn't mention handling a `409 Conflict` response. If the backend returns 409 on a stale version, the form action should surface it — but there's no AC for this. Either add an AC or explicitly note "optimistic locking conflict → generic error message, no special handling." 2. **Empty `title` with `type="submit"` and keyboard nav.** The AC says "form shows a localized required-field error, does not navigate away." But there's no AC for what happens when the title field is blank AND `eventDate` is also blank. The form has two required fields. The issue covers blank title alone, but does not specify whether both errors render simultaneously or sequentially. Spring Boot form actions return `fail(400)` with a single error string currently — are we surfacing per-field errors or a single summary error? Make this explicit. 3. **Prefill with both `?personId=` and `?documentId=`.** The issue states the `/new` route accepts both params. The spec says both are looked up in parallel via `Promise.all`. If both are valid, both should be pre-selected. If one is 404/403, that one is silently ignored and the other is used. This is implied but not stated as an explicit AC. Add: *Given `?personId=A&documentId=B` where A is valid and B is 404, when `/new` loads, then person A is pre-selected and the document picker is empty.* **Scope boundary around `DatePrecisionField` — cross-domain import concern.** The issue explicitly places `DatePrecisionField` in `$lib/shared/primitives/` to avoid cross-domain imports. But `DatePrecisionField` needs the `DatePrecision` type from `$lib/shared/utils/documentDate` and the `PRECISIONS` array (currently defined inline in `WhoWhenSection`). The PRECISIONS array uses i18n keys from `$lib/paraglide/messages.js`. Moving it to `$lib/shared/primitives/` is fine — both of those imports are already in `shared/`. No cross-domain issue. However, the AC for the extraction should explicitly state which props `DatePrecisionField` exposes, since this is now a shared contract between two consumers (`WhoWhenSection` and `EventForm`). Consider adding a props interface to the task checklist. **`RANGE` end-date `null` on submit — the AC is precise but the test is not.** The AC says "*eventDateEnd is submitted as `null`*" but `null` is a JSON concept. A form action receives `''` (empty string) from the hidden `<input value="">`. The mapping `'' → null` happens in the action. The test `should_submit_null_for_eventDateEnd_when_precision_changed_from_RANGE_to_YEAR` should be a server spec (testing the form action), not a component spec — the component can only assert the hidden input's value is `''`, not that the API receives `null`. The issue lists this as a component test, which is the wrong layer. ### Recommendations - Add an AC: *Given `?personId=A&documentId=B` where A is valid and B does not exist, when the form loads, then person A is pre-selected and the document picker shows its empty state.* - Clarify in the issue whether multi-required-field validation errors are shown simultaneously or the first failing field only. This determines whether the form action returns a single error string or a per-field error map. - Move `should_submit_null_for_eventDateEnd_when_precision_changed_from_RANGE_to_YEAR` to the server spec layer. At the component layer, assert the hidden input value is `''`; at the server layer, assert the API body has `eventDateEnd: null`.

marcel referenced this issue

2026-06-08 08:58:12 +02:00

Timeline: "events as context" overlay in document search #784

marcel referenced this issue

2026-06-08 12:03:44 +02:00

Timeline × Lesereisen: cross-epic synergies — promotion seam + mutual reference (avoid duplicate curation) #785

marcel commented

2026-06-08 16:28:25 +02:00

Visual spec for the curator event editor (on main, commit ddb1ec4d):

docs/specs/zeitstrahl-event-editor-spec.html — mirrors GeschichteEditor (2fr/1fr, sticky save bar, beforeNavigate guard): title, Typ (PERSONAL/HISTORICAL segmented), Datum + Präzision (shared DatePrecisionField, RANGE reveals end date), optional plain-text description; sidebar = Verknüpfte Briefe via DocumentMultiSelect (the letter-grouping control, §5/⑤) + Beteiligte Personen via PersonMultiSelect. Aligns with the decisions already in this body. Also covers the document-detail quick-action (Details-drawer "Zeitstrahl" column mirroring the Geschichten column + top-bar button) and the typeahead states — that surface isn't a separate issue yet, so flagging it here.

**Visual spec** for the curator event editor (on `main`, commit `ddb1ec4d`): [`docs/specs/zeitstrahl-event-editor-spec.html`](https://git.raddatz.cloud/marcel/familienarchiv/src/branch/main/docs/specs/zeitstrahl-event-editor-spec.html) — mirrors `GeschichteEditor` (`2fr/1fr`, sticky save bar, `beforeNavigate` guard): title, **Typ** (PERSONAL/HISTORICAL segmented), **Datum + Präzision** (shared `DatePrecisionField`, RANGE reveals end date), optional plain-text description; sidebar = **Verknüpfte Briefe** via `DocumentMultiSelect` (the letter-grouping control, §5/⑤) + **Beteiligte Personen** via `PersonMultiSelect`. Aligns with the decisions already in this body. Also covers the **document-detail quick-action** (Details-drawer "Zeitstrahl" column mirroring the Geschichten column + top-bar button) and the typeahead states — that surface isn't a separate issue yet, so flagging it here.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#781