Timeline: curator event create/edit forms (#781) #832

Open
marcel wants to merge 23 commits from feat/issue-781-timeline-curator-forms into main
Owner

Implements the curator timeline event editor (#781) — two WRITE_ALL-gated SvelteKit routes plus reused components. Pure frontend; backend #3 (TimelineEvent CRUD) is already merged.

What's in scope

  • /zeitstrahl/events/new — empty create form (?personId/?documentId prefill).
  • /zeitstrahl/events/[id]/edit — edit form seeded from GET /api/timeline/events/{id}, with the delete control.

REQ → test coverage (all green)

REQ Behavior Test
001 curator (WRITE_ALL) granted access new/page.server.spec.ts#allows a curator with WRITE_ALL
002 null user → 403 (guard before groups deref) both route specs #throws 403 for an unauthenticated (null) user
003 non-WRITE_ALL → 403 both route specs #throws 403 ... without WRITE_ALL
004 valid create → POST + redirect new#posts a TimelineEventRequest and redirects on success
005 valid edit → PUT + redirect edit#updates via PUT (with version) and redirects on success
006 confirmed delete → DELETE + redirect edit#deletes via DELETE and redirects ...
007 non-ok DELETE → error, no redirect edit#returns fail(status) ... when DELETE is not ok
008 RANGE → end-date visible EventForm#reveals the end-date field ..., WhoWhenSection#reveals the end-date field ...
009 non-RANGE → end-date hidden, eventDateEnd: null EventForm#hides the end-date field ..., new#sends eventDateEnd: null ...
010 blank title → error, no nav, pickers preserved EventForm#shows a required-field error ..., new#returns fail(400) with preserved picker arrays ...
011 blank title + date → both errors new#surfaces both title and date errors ...
012 unknown/derived id → 404, never blank form edit#throws 404 when the GET is not ok ...
013 409 → generic conflict, no redirect edit#maps a 409 conflict and does not redirect, new#... (incl. 409)
014 valid prefill pre-selected; unknown ignored new#preselects a valid person and ignores an unknown document, EventForm#preselects a person ...
015 non-UUID originPersonId → /zeitstrahl (CWE-601) new#defaults to /zeitstrahl when originPersonId is not a valid UUID
016 escaping via {...}, never {@html} grep -r '@html' src/lib/timeline/ → zero
017 labelled pickers, empty states, ≥44px chip targets PersonMultiSelect/DocumentMultiSelect/EventForm specs

Notable implementation choices

  • DatePrecisionField extracted into $lib/shared/primitives/ from WhoWhenSection's date+precision+RANGE region; both domains consume it without a cross-domain import. WhoWhenSection spec stays green across the refactor (RANGE-reveal regression fence added first).
  • One EventForm, two routes/new empty, /[id]/edit seeded. Type event as TimelineEventView, submit as TimelineEventRequest.
  • Reused hasWriteAll(locals) (no parallel helper). 403 error-page gating (the persons/new idiom), null-user guard first.
  • eslint boundaries extended to let timeline import person+document (mirrors the geschichte editor — the spec-mandated reuse).
  • ConfirmDialog got aria-modal="true"; both pickers' remove buttons padded to ≥44px; documentTypeahead.ts bare-fetch documented.
  • DocumentOption precision fields made optional so a DocumentRef (no precision) maps cleanly into a chip (graceful degradation, decision 2).

Tests

54 tests green across targeted single-file runs (35 client + 19 server). Thin Playwright spec written (create→200, logged-out→403, 320px no-overflow) but not executed — ci.yml does not run test:e2e; the component + server specs carry regression coverage. Production npm run build succeeds; svelte-check shows zero new errors in #781 files.

Docs

CLAUDE.md route tables, frontend C4 people-stories diagram, and .specify/rtm.md REQ-001…017 rows (feature timeline-curator-forms).

Closes #781

🤖 Generated with Claude Code

Implements the curator timeline event editor (#781) — two WRITE_ALL-gated SvelteKit routes plus reused components. Pure frontend; backend #3 (TimelineEvent CRUD) is already merged. ## What's in scope - `/zeitstrahl/events/new` — empty create form (`?personId`/`?documentId` prefill). - `/zeitstrahl/events/[id]/edit` — edit form seeded from `GET /api/timeline/events/{id}`, with the delete control. ## REQ → test coverage (all green) | REQ | Behavior | Test | |---|---|---| | 001 | curator (WRITE_ALL) granted access | `new/page.server.spec.ts#allows a curator with WRITE_ALL` | | 002 | null user → 403 (guard before groups deref) | both route specs `#throws 403 for an unauthenticated (null) user` | | 003 | non-WRITE_ALL → 403 | both route specs `#throws 403 ... without WRITE_ALL` | | 004 | valid create → POST + redirect | `new#posts a TimelineEventRequest and redirects on success` | | 005 | valid edit → PUT + redirect | `edit#updates via PUT (with version) and redirects on success` | | 006 | confirmed delete → DELETE + redirect | `edit#deletes via DELETE and redirects ...` | | 007 | non-ok DELETE → error, no redirect | `edit#returns fail(status) ... when DELETE is not ok` | | 008 | RANGE → end-date visible | `EventForm#reveals the end-date field ...`, `WhoWhenSection#reveals the end-date field ...` | | 009 | non-RANGE → end-date hidden, `eventDateEnd: null` | `EventForm#hides the end-date field ...`, `new#sends eventDateEnd: null ...` | | 010 | blank title → error, no nav, pickers preserved | `EventForm#shows a required-field error ...`, `new#returns fail(400) with preserved picker arrays ...` | | 011 | blank title + date → both errors | `new#surfaces both title and date errors ...` | | 012 | unknown/derived id → 404, never blank form | `edit#throws 404 when the GET is not ok ...` | | 013 | 409 → generic conflict, no redirect | `edit#maps a 409 conflict and does not redirect`, `new#... (incl. 409)` | | 014 | valid prefill pre-selected; unknown ignored | `new#preselects a valid person and ignores an unknown document`, `EventForm#preselects a person ...` | | 015 | non-UUID originPersonId → `/zeitstrahl` (CWE-601) | `new#defaults to /zeitstrahl when originPersonId is not a valid UUID` | | 016 | escaping via `{...}`, never `{@html}` | `grep -r '@html' src/lib/timeline/` → zero | | 017 | labelled pickers, empty states, ≥44px chip targets | `PersonMultiSelect`/`DocumentMultiSelect`/`EventForm` specs | ## Notable implementation choices - **`DatePrecisionField` extracted** into `$lib/shared/primitives/` from `WhoWhenSection`'s date+precision+RANGE region; both domains consume it without a cross-domain import. WhoWhenSection spec stays green across the refactor (RANGE-reveal regression fence added first). - **One `EventForm`, two routes** — `/new` empty, `/[id]/edit` seeded. Type `event` as `TimelineEventView`, submit as `TimelineEventRequest`. - **Reused `hasWriteAll(locals)`** (no parallel helper). 403 error-page gating (the `persons/new` idiom), null-user guard first. - **`eslint` boundaries** extended to let `timeline` import `person`+`document` (mirrors the geschichte editor — the spec-mandated reuse). - **`ConfirmDialog`** got `aria-modal="true"`; both pickers' remove buttons padded to ≥44px; `documentTypeahead.ts` bare-fetch documented. - **`DocumentOption`** precision fields made optional so a `DocumentRef` (no precision) maps cleanly into a chip (graceful degradation, decision 2). ## Tests 54 tests green across targeted single-file runs (35 client + 19 server). Thin Playwright spec written (create→200, logged-out→403, 320px no-overflow) but not executed — `ci.yml` does not run `test:e2e`; the component + server specs carry regression coverage. Production `npm run build` succeeds; `svelte-check` shows zero new errors in #781 files. ## Docs CLAUDE.md route tables, frontend C4 people-stories diagram, and `.specify/rtm.md` REQ-001…017 rows (feature `timeline-curator-forms`). Closes #781 🤖 Generated with [Claude Code](https://claude.com/claude-code)
marcel added 12 commits 2026-06-13 23:02:24 +02:00
NVDA+Chrome <2022 and VoiceOver iOS <16 need explicit aria-modal="true";
showModal() implicit modal semantics are not enough for older AT. One-line
patch benefits all dialog uses.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a RANGE-reveal regression test to WhoWhenSection's spec. The existing
spec covered only date pre-fill / hideDate / location, leaving the end-date
region without a red signal. This must stay green across the #781 extraction
of DatePrecisionField into $lib/shared/primitives/.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pulls the date + precision + RANGE-end-date region into a generic primitive in
$lib/shared/primitives/ so both document/ (WhoWhenSection) and timeline/
(EventForm, #781) can consume it without a cross-domain import. Preserves the
aria-live="polite" outer wrapper, onMount one-time seeding, $bindable
precision/endDateIso, the PRECISIONS array, and forwards data-testid attributes
so the existing WhoWhenSection spec selectors survive. WhoWhenSection spec stays
green (7/7).

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Labels, section headings, type options (PERSONAL/HISTORICAL), picker empty
states, required-field errors, delete-confirm and unsaved-changes copy for the
curator event create/edit forms. No new ErrorCode introduced — the feature
reuses existing TIMELINE_EVENT_* + CONFLICT codes from #3.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both PersonMultiSelect and DocumentMultiSelect remove buttons were ~12px tap
targets (below the 44px WCAG minimum) — pad them to min-h/min-w 44px with a
focus-visible ring (SVG stays 12px). Add an optional emptyLabel slot inside the
chip container and a hiddenInputName prop on PersonMultiSelect (mirroring
DocumentMultiSelect) so EventForm can wire personIds without touching
WhoWhenSection. Document the intentional bare typeahead fetch in
documentTypeahead.ts (same-origin in prod, Vite-proxied in dev).

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
grid-cols-2 segmented radio group modelled on PersonTypeSelector: role=radiogroup
with role=radio buttons, roving tabindex, radioGroupNav arrow-key support, and an
sr-only aria-live type-change announcement. Each option pairs a decorative
aria-hidden icon with a visible localized text label (icon is never the sole
differentiator), min-h-48px target. Emits a hidden input for form submission.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
One component for both routes: /new renders it empty, /[id]/edit seeds it from a
TimelineEventView. Composes EventTypeSelect, the shared DatePrecisionField, a
plain-textarea description, PersonMultiSelect and DocumentMultiSelect (personIds
/documentIds hidden inputs). lg:grid-cols-[2fr_1fr] collapsing to one column
below lg, sticky save bar, beforeNavigate unsaved-changes guard, submitting flag
via use:enhance (disabled submit), and a delete form gated by getConfirmService()
read lazily so the component mounts cleanly in isolation. Title/description/chip
labels render via default {...} escaping (CWE-79). Seeded DocumentRefs degrade
gracefully to DocumentOption (no precision fields). Pickers gain an inputId prop
so <label for> associates the control; eslint boundaries now lets timeline import
person+document (mirrors the geschichte editor). 6/6 component specs green.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Server load gates on hasWriteAll with a null-user guard first (403 error page,
the persons/new idiom — not a redirect); prefills ?personId/?documentId via
Promise.all, swallowing 404/403 so unknown ids never leak. The save action
parses the form, surfaces title+date required errors simultaneously via
fail(400) preserving picker arrays, builds a TimelineEventRequest (eventDateEnd
explicit null off RANGE), POSTs, maps API/409 errors via getErrorMessage without
redirecting, and redirects to a UUID-validated nav target (CWE-601). Shared
parse/validate/build/nav helpers live in eventFormServer.ts for reuse by the
edit route. 11/11 server specs green.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Load gates on hasWriteAll (null-user guard first, 403 error page) and seeds the
form from GET /api/timeline/events/{id}, failing closed with 404 on ANY non-ok
response so derived person-events (no UUID) and unknown ids never render a blank
create form. The save action PUTs with the optimistic-lock version (threaded via
a hidden input EventForm now emits), mapping 409 to the generic conflict message
without redirecting. The delete action DELETEs behind getConfirmService(),
returns fail(status) on a non-ok response (no redirect), and otherwise redirects
to the UUID-validated nav target. 8/8 server specs green; EventForm 6/6 green.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
One critical create journey (fill form with precision RANGE → HTTP 200 on
/zeitstrahl; the card assertion depends on #7), one security counterpart
(logged-out → 403), and one 320px no-overflow guarantee. Intentionally thin —
ci.yml does not run test:e2e today, so regression coverage lives in the
component + server specs that DO run in CI. Written, not executed locally.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the events/new + events/[id]/edit children to both CLAUDE.md route tables
and the frontend C4 people-stories diagram (new zeitstrahlEvents component +
backend relation), and traces REQ-001..017 (feature timeline-curator-forms) in
.specify/rtm.md mirroring the sibling #776/#777/#778/#779 timeline rows.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix(types): make DocumentOption precision fields optional; narrow spec data access
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 3m0s
CI / OCR Service Tests (pull_request) Successful in 23s
CI / Backend Unit Tests (pull_request) Successful in 4m54s
CI / fail2ban Regex (pull_request) Successful in 47s
SDD Gate / RTM Check (pull_request) Successful in 15s
SDD Gate / Contract Validate (pull_request) Successful in 22s
CI / Semgrep Security Scan (pull_request) Successful in 23s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m8s
SDD Gate / Constitution Impact (pull_request) Successful in 16s
94d7d8099f
DocumentOption's metaDatePrecision/metaDateEnd are now optional so a
TimelineEvent DocumentRef (id/title/documentDate only) maps cleanly into a
picker chip — formatDocumentOption already degrades gracefully when precision is
absent. The server specs read fail()'s union data via a small failData() cast
that TS cannot narrow. svelte-check shows zero new errors in the #781 files.

Refs #781
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
marcel added 3 commits 2026-06-13 23:22:55 +02:00
A TimelineEvent's DocumentRef carries documentDate but no precision, so
formatDocumentOption hit formatDocumentDate's undefined-precision path and
surfaced the UNKNOWN label instead of the date. Default a missing precision
to DAY so the chip shows the full date; add formatDocumentOption unit specs.

Addresses PR #832 review (#781).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both curator-event loaders repeated the same null-user + hasWriteAll block.
hasWriteAll already returns false for an anonymous user, so a single
requireWriteAll(locals) helper covers both REQ-002 (null user → 403) and
REQ-003 (no WRITE_ALL → 403) without the redundant pre-check.

Addresses PR #832 review (#781).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix(timeline): harden curator event precision field
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 2m51s
CI / OCR Service Tests (pull_request) Successful in 24s
CI / Backend Unit Tests (pull_request) Successful in 4m35s
CI / fail2ban Regex (pull_request) Successful in 47s
CI / Semgrep Security Scan (pull_request) Successful in 23s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m7s
SDD Gate / RTM Check (pull_request) Successful in 13s
SDD Gate / Contract Validate (pull_request) Successful in 22s
SDD Gate / Constitution Impact (pull_request) Successful in 17s
cd5649b96e
- Validate the submitted precision against the DatePrecision allow-list in
  parseEventForm (falls back to DAY) so an untrusted token can't flow into
  the request body — symmetric with the existing `type` narrowing.
- Parameterize the precision input name via DatePrecisionField's new
  precisionInputName prop; the timeline form now submits `precision` instead
  of the misleading document-domain `metaDatePrecision`. Document form keeps
  the default, so its behaviour is unchanged.
- Capture EventTypeSelect's onchange into EventForm's `type` state so it no
  longer goes stale (the submitted value was already correct via the hidden
  input; this keeps the local state in sync).

Addresses PR #832 review (#781).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
Owner

🧪 Tester — PR Review

Verdict: ⚠️ Approved with concerns

Blockers

  • None — every REQ has at least one real, behavior-asserting test at the right layer; the security boundary (403 null + 403 non-WRITE_ALL) and fail-closed 404 paths are covered on both routes.

Suggestions

  • frontend/src/lib/timeline/EventForm.svelte.spec.ts:63-69should_disable_submit_button_while_submitting is a named test in the spec's "## Tests" list ("assert the button gains disabled before the action completes"), but the implemented test only asserts the button is initially enabled (not.toBeDisabled()). That is a tautology against an empty form — it never exercises the submitting=truedisabled={submitting} path, so the double-submit guard (a named AC, Decision 8) is effectively untested. The submitting state is hard to drive without a real form action round-trip, so a thin E2E assertion (click Speichern, assert [aria-disabled]/disabled before navigation) would be the honest home for it — but the E2E file does not cover it either. Recommend either a component test that fires submit and asserts the disabled transition, or an explicit note that this AC is intentionally only smoke-covered. Not a blocker because the regression weight of #781 lives in the server specs, which are sound.
  • frontend/src/lib/timeline/EventForm.svelte.spec.ts:39-43 — the YEAR-hide test mixes a getByTestId('end-date-region').toBeInTheDocument() user-level assertion with a raw document.querySelector('#eventDateEnd') DOM probe. The querySelector is fine as the negative assertion, but consider page.getByLabelText('Enddatum') + .not.toBeInTheDocument() to stay symmetric with the RANGE-reveal test and avoid coupling to the #eventDateEnd id.
  • new/page.server.spec.ts:2001 and :2023 (eventDateEnd-null + unknown-precision→DAY) use a try/catch {} swallow around the redirect-throwing action, then assert on the captured mock call. This works but silently passes if the action stops throwing (e.g. a future regression where save no longer redirects) — the assertion would still run against a stale/empty mock. Prefer await expect(actions.save(...)).rejects.toMatchObject({ status: 303 }) and then assert the body, so a missing redirect fails loudly rather than being absorbed.
  • E2E (zeitstrahl-event-editor.spec.ts) is correctly documented as not-run-in-CI; the component+server specs do carry the regression weight as the spec requires. The 320px no-overflow and logged-out-403 checks are good thin coverage. No change needed, just confirming the documented constraint holds.
  • Newest-commit coverage is otherwise solid: the unknown-precision→DAY fallback (eventFormServer.ts:51) is asserted via the request body; documentTypeahead.spec.ts covers both the bare-title and precision-absent→full-date branches and explicitly fences the UNKNOWN-fallback regression; and the requireWriteAll extraction keeps both the null-user and non-WRITE_ALL 403 tests on each route, still asserting rejects.toMatchObject({ status: 403 }) — meaningful after the guard moved into the shared helper.
### 🧪 Tester — PR Review **Verdict: ⚠️ Approved with concerns** #### Blockers - None — every REQ has at least one real, behavior-asserting test at the right layer; the security boundary (403 null + 403 non-WRITE_ALL) and fail-closed 404 paths are covered on both routes. #### Suggestions - `frontend/src/lib/timeline/EventForm.svelte.spec.ts:63-69` — `should_disable_submit_button_while_submitting` is a **named test in the spec's "## Tests" list** ("assert the button gains `disabled` before the action completes"), but the implemented test only asserts the button is *initially* enabled (`not.toBeDisabled()`). That is a tautology against an empty form — it never exercises the `submitting=true` → `disabled={submitting}` path, so the double-submit guard (a named AC, Decision 8) is effectively untested. The submitting state is hard to drive without a real form action round-trip, so a thin E2E assertion (click Speichern, assert `[aria-disabled]`/`disabled` before navigation) would be the honest home for it — but the E2E file does not cover it either. Recommend either a component test that fires submit and asserts the disabled transition, or an explicit note that this AC is intentionally only smoke-covered. Not a blocker because the regression weight of #781 lives in the server specs, which are sound. - `frontend/src/lib/timeline/EventForm.svelte.spec.ts:39-43` — the YEAR-hide test mixes a `getByTestId('end-date-region').toBeInTheDocument()` user-level assertion with a raw `document.querySelector('#eventDateEnd')` DOM probe. The querySelector is fine as the negative assertion, but consider `page.getByLabelText('Enddatum')` + `.not.toBeInTheDocument()` to stay symmetric with the RANGE-reveal test and avoid coupling to the `#eventDateEnd` id. - `new/page.server.spec.ts:2001` and `:2023` (eventDateEnd-null + unknown-precision→DAY) use a `try/catch {}` swallow around the redirect-throwing action, then assert on the captured mock call. This works but silently passes if the action stops throwing (e.g. a future regression where save no longer redirects) — the assertion would still run against a stale/empty mock. Prefer `await expect(actions.save(...)).rejects.toMatchObject({ status: 303 })` *and then* assert the body, so a missing redirect fails loudly rather than being absorbed. - E2E (`zeitstrahl-event-editor.spec.ts`) is correctly documented as not-run-in-CI; the component+server specs do carry the regression weight as the spec requires. The 320px no-overflow and logged-out-403 checks are good thin coverage. No change needed, just confirming the documented constraint holds. - Newest-commit coverage is otherwise solid: the unknown-precision→DAY fallback (`eventFormServer.ts:51`) is asserted via the request body; `documentTypeahead.spec.ts` covers both the bare-title and precision-absent→full-date branches and explicitly fences the UNKNOWN-fallback regression; and the `requireWriteAll` extraction keeps both the null-user and non-WRITE_ALL 403 tests on each route, still asserting `rejects.toMatchObject({ status: 403 })` — meaningful after the guard moved into the shared helper.
Author
Owner

📋 Requirements Engineer — PR Review

Verdict: ⚠️ Approved with concerns

All 17 requirements are implemented, traced in .specify/rtm.md (timeline-curator-forms, all Done), and backed by real tests that exist in the diff. Two concerns are partial-coverage gaps on sub-aspects of REQ-011 and REQ-017, not missing requirements. The three follow-up commits check out: the precision field rename is consistent across DatePrecisionField.svelte (precisionInputName="precision"), eventFormServer.ts#parseEventForm (formData.get('precision')), and the server spec (precision: 'YEAR'); precision allow-list (VALID_PRECISIONS) is present with a dedicated hardening test; formatDocumentOption defaults a missing precision to DAY with a test; requireWriteAll is extracted and consumed by both route loads; REQ-009 still submits eventDateEnd: null (verified in parse + server spec).

Traceability table

REQ Implemented? Test? rtm Done? Notes
REQ-001 requireWriteAll in both loads new/...#allows a curator with WRITE_ALL, [id]/edit/...#seeds the form on ok GET
REQ-002 null-user guard inside hasWriteAll (false → 403) both specs #throws 403 for an unauthenticated (null) user
REQ-003 both specs #...without WRITE_ALL
REQ-004 POST + resolveNavTarget redirect new/...#posts a TimelineEventRequest and redirects
REQ-005 PUT w/ version [id]/edit/...#updates via PUT (with version) and redirects
REQ-006 DELETE + confirm via getConfirmService [id]/edit/...#deletes via DELETE and redirects
REQ-007 non-ok DELETE → fail, no redirect [id]/edit/...#returns fail(status) and does not redirect
REQ-008 showEndDate = precision==='RANGE' EventForm...#reveals the end-date field when precision is RANGE + WhoWhenSection regression
REQ-009 hidden input '' off-RANGE → parsed to null EventForm...#hides...when YEAR + new/...#sends eventDateEnd: null when precision is not RANGE
REQ-010 validateEventForm preserves picker arrays new/...#returns fail(400) with preserved picker arrays on blank title
REQ-011 ⚠️ partial — both error strings surfaced, but date error not wired to per-field aria-invalid ⚠️ test asserts both error strings, not per-field aria-invalid on the date field See Concern 1
REQ-012 any non-ok GET → 404 [id]/edit/...#throws 404 when the GET is not ok
REQ-013 409 mapped, no redirect both specs (#maps a 409 conflict...)
REQ-014 Promise.all prefill, swallow non-ok new/...#preselects a valid person and ignores an unknown document + EventForm...#preselects a person
REQ-015 strict UUID_RE in resolveNavTarget new/...#defaults to /zeitstrahl when ...not a valid UUID + valid-UUID case
REQ-016 all chip/title/desc via {...} grep '@html' frontend/src/lib/timeline/ → zero (verified)
REQ-017 ⚠️ impl present (min-h-[44px] min-w-[44px] on both pickers, <label for>, empty states) ⚠️ cited PersonMultiSelect/DocumentMultiSelect.svelte.spec.ts do not assert the ≥44px target See Concern 2

Blockers

  • None. Every REQ-001..017 is implemented and has at least one real, existing test; rtm is fully Done with valid pointers. No unrequited scope-creep found (the cross-domain eslint allowance, DatePrecisionField extraction, and ConfirmDialog aria-modal patch are all explicitly mandated by the issue body).

Suggestions

  • Concern 1 (REQ-011): the EARS text mandates "both errors via per-field aria-invalid." Today the title input carries aria-invalid, but the server-side blank-date dateError renders as a standalone <p> after the date card and is not wired to the date input's aria-invalid (the field's aria-invalid only reflects the client-side malformed-date dateInvalid). Wire the server dateError to the DatePrecisionField date input's aria-invalid, and add a component-layer assertion. The behavioral core (both errors shown) is covered, so this is a concern, not a blocker.
  • Concern 2 (REQ-017): the ≥44px remove-target fix is correctly in the production diff, but rtm cites PersonMultiSelect.svelte.spec.ts / DocumentMultiSelect.svelte.spec.ts "(green post-44px fix)" — those specs only cover render/select/remove and assert nothing about target size. Add a class/computed-size assertion for the ≥44px target (or repoint the rtm Test column at a test that truly exercises it) so the requirement is genuinely fenced.
  • Named-AC gap (not a REQ): the issue's test strategy lists should_disable_submit_button_while_submitting; EventForm.svelte.spec.ts only has renders an enabled submit button initially. The submitting/disabled wiring exists in the component but the disabled-while-submitting transition is unasserted. Optional to add.
  • Minor: rtm REQ-010 pointer #shows a required-field error when title is blank is a substring of the actual test name (...when title is blank and save is attempted) — matches fine, no action needed.
### 📋 Requirements Engineer — PR Review **Verdict: ⚠️ Approved with concerns** All 17 requirements are implemented, traced in `.specify/rtm.md` (`timeline-curator-forms`, all `Done`), and backed by real tests that exist in the diff. Two concerns are partial-coverage gaps on sub-aspects of REQ-011 and REQ-017, not missing requirements. The three follow-up commits check out: the `precision` field rename is consistent across `DatePrecisionField.svelte` (`precisionInputName="precision"`), `eventFormServer.ts#parseEventForm` (`formData.get('precision')`), and the server spec (`precision: 'YEAR'`); precision allow-list (`VALID_PRECISIONS`) is present with a dedicated hardening test; `formatDocumentOption` defaults a missing precision to `DAY` with a test; `requireWriteAll` is extracted and consumed by both route loads; REQ-009 still submits `eventDateEnd: null` (verified in parse + server spec). #### Traceability table | REQ | Implemented? | Test? | rtm Done? | Notes | |-----|---|---|---|---| | REQ-001 | ✅ `requireWriteAll` in both loads | ✅ `new/...#allows a curator with WRITE_ALL`, `[id]/edit/...#seeds the form on ok GET` | ✅ | | | REQ-002 | ✅ null-user guard inside `hasWriteAll` (false → 403) | ✅ both specs `#throws 403 for an unauthenticated (null) user` | ✅ | | | REQ-003 | ✅ | ✅ both specs `#...without WRITE_ALL` | ✅ | | | REQ-004 | ✅ POST + `resolveNavTarget` redirect | ✅ `new/...#posts a TimelineEventRequest and redirects` | ✅ | | | REQ-005 | ✅ PUT w/ version | ✅ `[id]/edit/...#updates via PUT (with version) and redirects` | ✅ | | | REQ-006 | ✅ DELETE + confirm via `getConfirmService` | ✅ `[id]/edit/...#deletes via DELETE and redirects` | ✅ | | | REQ-007 | ✅ non-ok DELETE → `fail`, no redirect | ✅ `[id]/edit/...#returns fail(status) and does not redirect` | ✅ | | | REQ-008 | ✅ `showEndDate = precision==='RANGE'` | ✅ `EventForm...#reveals the end-date field when precision is RANGE` + `WhoWhenSection` regression | ✅ | | | REQ-009 | ✅ hidden input `''` off-RANGE → parsed to `null` | ✅ `EventForm...#hides...when YEAR` + `new/...#sends eventDateEnd: null when precision is not RANGE` | ✅ | | | REQ-010 | ✅ `validateEventForm` preserves picker arrays | ✅ `new/...#returns fail(400) with preserved picker arrays on blank title` | ✅ | | | REQ-011 | ⚠️ partial — both error *strings* surfaced, but date error not wired to per-field `aria-invalid` | ⚠️ test asserts both error strings, not per-field `aria-invalid` on the date field | ✅ | See Concern 1 | | REQ-012 | ✅ any non-ok GET → 404 | ✅ `[id]/edit/...#throws 404 when the GET is not ok` | ✅ | | | REQ-013 | ✅ 409 mapped, no redirect | ✅ both specs (`#maps a 409 conflict...`) | ✅ | | | REQ-014 | ✅ `Promise.all` prefill, swallow non-ok | ✅ `new/...#preselects a valid person and ignores an unknown document` + `EventForm...#preselects a person` | ✅ | | | REQ-015 | ✅ strict `UUID_RE` in `resolveNavTarget` | ✅ `new/...#defaults to /zeitstrahl when ...not a valid UUID` + valid-UUID case | ✅ | | | REQ-016 | ✅ all chip/title/desc via `{...}` | ✅ `grep '@html' frontend/src/lib/timeline/` → zero (verified) | ✅ | | | REQ-017 | ⚠️ impl present (`min-h-[44px] min-w-[44px]` on both pickers, `<label for>`, empty states) | ⚠️ cited `PersonMultiSelect/DocumentMultiSelect.svelte.spec.ts` do **not** assert the ≥44px target | ✅ | See Concern 2 | #### Blockers - None. Every REQ-001..017 is implemented and has at least one real, existing test; rtm is fully `Done` with valid pointers. No unrequited scope-creep found (the cross-domain eslint allowance, `DatePrecisionField` extraction, and `ConfirmDialog aria-modal` patch are all explicitly mandated by the issue body). #### Suggestions - **Concern 1 (REQ-011):** the EARS text mandates "both errors via **per-field `aria-invalid`**." Today the title input carries `aria-invalid`, but the server-side blank-date `dateError` renders as a standalone `<p>` after the date card and is not wired to the date input's `aria-invalid` (the field's `aria-invalid` only reflects the client-side malformed-date `dateInvalid`). Wire the server `dateError` to the `DatePrecisionField` date input's `aria-invalid`, and add a component-layer assertion. The behavioral core (both errors shown) is covered, so this is a concern, not a blocker. - **Concern 2 (REQ-017):** the ≥44px remove-target fix is correctly in the production diff, but rtm cites `PersonMultiSelect.svelte.spec.ts` / `DocumentMultiSelect.svelte.spec.ts` "(green post-44px fix)" — those specs only cover render/select/remove and assert nothing about target size. Add a class/computed-size assertion for the ≥44px target (or repoint the rtm Test column at a test that truly exercises it) so the requirement is genuinely fenced. - **Named-AC gap (not a REQ):** the issue's test strategy lists `should_disable_submit_button_while_submitting`; `EventForm.svelte.spec.ts` only has `renders an enabled submit button initially`. The `submitting`/`disabled` wiring exists in the component but the disabled-while-submitting transition is unasserted. Optional to add. - Minor: rtm REQ-010 pointer `#shows a required-field error when title is blank` is a substring of the actual test name (`...when title is blank and save is attempted`) — matches fine, no action needed.
Author
Owner

⚙️ DevOps — PR Review

Verdict: Approved

Blockers

  • None

Constitution §4 Do-Not-Touch — all clear (verified against the full diff file list):

  • Generated frontend/src/lib/generated/api.ts not edited — only consumed via components['schemas'][...] type references.
  • No Flyway migration in the diff (pure-frontend feature, as issue #781 promises — no backend/.../db/migration/ file).
  • No Accepted ADR edited (no docs/adr/ change; issue correctly states no ADR is triggered).
  • No .gitea/workflows/ change at all — no artifact action bumped past @v3, no CI guard removed or weakened.
  • No docker-compose*.yml / .env* change — no new env var, no exposed port, no migration ordering concern.

Operational notes:

  • E2E-not-in-CI reality respected. The new frontend/e2e/zeitstrahl-event-editor.spec.ts documents in its header that ci.yml does not invoke test:e2e, and the real regression coverage lives in the component + server specs (which run in the "Unit & Component Tests" job). That matches the issue's DevOps caveat exactly — good.
  • The Playwright spec's const stamp = () => new Date().toISOString()... is fine: it only runs locally/manually against the Docker Compose stack, never in CI, so there is no nondeterminism risk in the pipeline.

Suggestions

  • frontend/eslint.config.js:150 widens the timeline → {shared, person, document} boundary. This is a domain-architecture config (the architect's lane), not a CI banned-pattern guard, and it is documented + justified per issue §1.4 — no DevOps objection. Flagging only so the architect persona confirms the boundary widening is intended.
  • No healthcheck / sidecar / observability surface touched — nothing to wire into Prometheus/Loki/Tempo for this change. Confirmed no-op for the observability stack.
### ⚙️ DevOps — PR Review **Verdict: ✅ Approved** #### Blockers - None Constitution §4 Do-Not-Touch — all clear (verified against the full diff file list): - Generated `frontend/src/lib/generated/api.ts` **not** edited — only consumed via `components['schemas'][...]` type references. ✅ - No Flyway migration in the diff (pure-frontend feature, as issue #781 promises — no `backend/.../db/migration/` file). ✅ - No `Accepted` ADR edited (no `docs/adr/` change; issue correctly states no ADR is triggered). ✅ - No `.gitea/workflows/` change at all — no artifact action bumped past `@v3`, no CI guard removed or weakened. ✅ - No `docker-compose*.yml` / `.env*` change — no new env var, no exposed port, no migration ordering concern. ✅ Operational notes: - **E2E-not-in-CI reality respected.** The new `frontend/e2e/zeitstrahl-event-editor.spec.ts` documents in its header that `ci.yml` does not invoke `test:e2e`, and the real regression coverage lives in the component + server specs (which run in the "Unit & Component Tests" job). That matches the issue's DevOps caveat exactly — good. - The Playwright spec's `const stamp = () => new Date().toISOString()...` is fine: it only runs locally/manually against the Docker Compose stack, never in CI, so there is no nondeterminism risk in the pipeline. #### Suggestions - `frontend/eslint.config.js:150` widens the `timeline → {shared, person, document}` boundary. This is a domain-architecture config (the architect's lane), not a CI banned-pattern guard, and it is documented + justified per issue §1.4 — no DevOps objection. Flagging only so the architect persona confirms the boundary widening is intended. - No healthcheck / sidecar / observability surface touched — nothing to wire into Prometheus/Loki/Tempo for this change. Confirmed no-op for the observability stack.
Author
Owner

🛠️ Developer (Felix Brandt) — PR Review

Verdict: ⚠️ Approved with concerns

Clean, well-decomposed work. DatePrecisionField extraction is textbook KISS reuse — shared primitive in $lib/shared/primitives/, $bindable props with no redundant $state mirror, onMount seeding preserved verbatim. The three review-fix commits all hold up:

  • Precision rename is consistent end-to-endDatePrecisionField select name={precisionInputName} (default 'metaDatePrecision'), EventForm overrides precisionInputName="precision", parseEventForm reads formData.get('precision'), specs assert precision:. WhoWhenSection omits the prop, so the document form still submits metaDatePrecision — unbroken (its regression test reveals the end-date field when precision is RANGE is green). The VALID_PRECISIONS allow-list with DAY fallback is sound and tested (falls back to DAY precision when an unknown precision token is submitted).
  • formatDocumentOption DAY default — sound. A DocumentRef chip carries documentDate but no precision; defaulting to DAY renders the full date rather than the UNKNOWN label. Unit-tested directly.
  • requireWriteAll(locals) — correct. hasWriteAll returns false for a null user (locals.user?.groups?... ?? false), so the single guard covers both REQ-002 (null user → 403) and REQ-003 (no WRITE_ALL → 403). Both server specs pass. No parallel hasPermission introduced (§3.2 / Decision 14 honored).

Pure frontend (no backend model/endpoint change → npm run generate:api not needed). No {@html} anywhere in the new code (REQ-016 ✓). {#each} keyed in both new components. No new ErrorCode. Layering/boundaries respected — the eslint timeline → person, document allow-entry is justified and scoped.

Blockers

  • None.

Suggestions

  • EventForm.svelte:58-71picker persistence on fail(400) is incomplete vs. REQ-010 / Decision 6. The server action correctly returns personIds/documentIds arrays in the fail payload, but EventForm seeds selectedPersons/selectedDocuments only from event?.persons / initialPersons — it never reads form?.personIds / form?.documentIds. Note the title/description/type fields do read form?.* first (line 51-53); the pickers are the odd ones out. In practice this is masked on the primary path because use:enhance's update() does not remount the component, so the in-memory chip selection survives a JS round-trip. But on the no-JS / full-reload fallback the chips are dropped — exactly the "preserved including pre-selected persons/documents" guarantee the AC names for the 60+ audience. Two gaps worth closing: (1) the returned bare-ID arrays can't rebuild chips anyway (chips need displayName/title, which aren't in the payload), so either the re-seed path is wired up properly or the comment at line 49 ("event > preserved fail payload") is misleading for pickers; (2) there is no component test asserting picker persistence after fail(400) — the spec covers the required-field error and preselect-from-initialPersons, but not the round-trip preservation that Decision 6 calls out as a "significant UX regression" if dropped. Recommend a test that pins the actual behavior.
  • EventForm.svelte:51 — the comment "re-mount with a different event to reset" documents a footgun (props are snapshotted into $state, so a parent re-render with new event won't update the form). Fine for two dedicated routes that always remount, but worth a one-line note that this component is intentionally single-shot.
### 🛠️ Developer (Felix Brandt) — PR Review **Verdict: ⚠️ Approved with concerns** Clean, well-decomposed work. `DatePrecisionField` extraction is textbook KISS reuse — shared primitive in `$lib/shared/primitives/`, `$bindable` props with no redundant `$state` mirror, `onMount` seeding preserved verbatim. The three review-fix commits all hold up: - **Precision rename is consistent end-to-end** — `DatePrecisionField` select `name={precisionInputName}` (default `'metaDatePrecision'`), `EventForm` overrides `precisionInputName="precision"`, `parseEventForm` reads `formData.get('precision')`, specs assert `precision:`. `WhoWhenSection` omits the prop, so the document form still submits `metaDatePrecision` — unbroken (its regression test `reveals the end-date field when precision is RANGE` is green). The `VALID_PRECISIONS` allow-list with `DAY` fallback is sound and tested (`falls back to DAY precision when an unknown precision token is submitted`). - **`formatDocumentOption` DAY default** — sound. A `DocumentRef` chip carries `documentDate` but no precision; defaulting to `DAY` renders the full date rather than the UNKNOWN label. Unit-tested directly. - **`requireWriteAll(locals)`** — correct. `hasWriteAll` returns `false` for a null user (`locals.user?.groups?... ?? false`), so the single guard covers both REQ-002 (null user → 403) and REQ-003 (no WRITE_ALL → 403). Both server specs pass. No parallel `hasPermission` introduced (§3.2 / Decision 14 honored). Pure frontend (no backend model/endpoint change → `npm run generate:api` not needed). No `{@html}` anywhere in the new code (REQ-016 ✓). `{#each}` keyed in both new components. No new `ErrorCode`. Layering/boundaries respected — the eslint `timeline → person, document` allow-entry is justified and scoped. #### Blockers - None. #### Suggestions - `EventForm.svelte:58-71` — **picker persistence on `fail(400)` is incomplete vs. REQ-010 / Decision 6.** The server action correctly returns `personIds`/`documentIds` arrays in the fail payload, but `EventForm` seeds `selectedPersons`/`selectedDocuments` only from `event?.persons` / `initialPersons` — it never reads `form?.personIds` / `form?.documentIds`. Note the title/description/type fields *do* read `form?.*` first (line 51-53); the pickers are the odd ones out. In practice this is masked on the primary path because `use:enhance`'s `update()` does not remount the component, so the in-memory chip selection survives a JS round-trip. But on the no-JS / full-reload fallback the chips are dropped — exactly the "preserved including pre-selected persons/documents" guarantee the AC names for the 60+ audience. Two gaps worth closing: (1) the returned bare-ID arrays can't rebuild chips anyway (chips need `displayName`/`title`, which aren't in the payload), so either the re-seed path is wired up properly or the comment at line 49 ("event > preserved fail payload") is misleading for pickers; (2) there is **no component test asserting picker persistence after `fail(400)`** — the spec covers the required-field error and preselect-from-`initialPersons`, but not the round-trip preservation that Decision 6 calls out as a "significant UX regression" if dropped. Recommend a test that pins the actual behavior. - `EventForm.svelte:51` — the comment "re-mount with a different `event` to reset" documents a footgun (props are snapshotted into `$state`, so a parent re-render with new `event` won't update the form). Fine for two dedicated routes that always remount, but worth a one-line note that this component is intentionally single-shot.
Author
Owner

🔐 Security (Nora "NullX") — PR Review

Verdict: Approved

Adversarial pass on #781's curator event forms. The frontend gate is correctly treated as UX; the real boundary remains the backend @RequirePermission(WRITE_ALL) on #3's CRUD endpoints. Every focus area checks out.

Blockers

  • None

Verified (the load-bearing claims)

1. Anonymous → 403 still holds after the guard refactor (REQ-002/003, §2.1). requireWriteAll(locals) delegates to hasWriteAll, whose body is locals.user?.groups?.some(...) ?? false. For a null user the optional chain short-circuits to undefined?? falsefalsethrow error(403). Dropping the explicit if (!locals.user) pre-check opened nothing: a null user never reaches a .groups deref. Both loaders (new/+page.server.ts:23, [id]/edit/+page.server.ts:23) call requireWriteAll(locals) as the first statement, before any API call. permissions.spec.ts pins hasWriteAll({}) and hasWriteAll({ user: {} })false, and both page.server.spec.ts files pin null-user → 403. The single existing caller pattern (documents/[id]/edit) is unchanged — no parallel predicate drift (§3.2 honored).

2. Open-redirect guard intact (REQ-015, CWE-601). resolveNavTarget (eventFormServer.ts:24) returns /persons/{id} only when UUID_RE.test(raw) passes; the regex is anchored both ends (^…$), so a crafted hidden originPersonId like ../evil or //evil.com fails and falls to /zeitstrahl (test: defaults to /zeitstrahl when originPersonId is not a valid UUID). The UUID branch can only emit a same-origin relative path with no scheme — no protocol-relative escape. Note (non-blocking): the regex is generic 8-4-4-4-12 hex, not strict RFC-4122 variant/version — but it matches the spec verbatim and is fully sufficient for the CWE-601 guard.

3. No untrusted value reaches the request body (mass-assignment, CWE-639). toEventRequest builds the body from title, type (narrowed to PERSONAL|HISTORICAL), eventDate, precision (allow-listed against VALID_PRECISIONS, else DAY — test pins the NOT_A_REAL_PRECISIONDAY fallback), eventDateEnd, description, personIds/documentIds, version. originPersonId is parsed for redirect only and is excluded from the body — it cannot escape into the write payload. createdBy/updatedBy are server-set in #3's service, never bound here (§2.4). person/document IDs are UUIDs validated by the backend, which is the authZ gate for IDOR.

4. Fail-closed on derived events (REQ-012). [id]/edit load does if (!result.response.ok) throw error(404) — covers all non-ok statuses (derived Geburt/Tod/Heirat 404, unknown id, 403). Never renders a blank create form that silently POSTs. Test pins it.

5. No existence leak on prefill (REQ-014). new load swallows 404/403 on the Promise.all person/document lookups (personResult && personResult.response.ok && personResult.data ? […] : []) — unknown id → empty picker, no thrown error. Test pins valid-person + unknown-document.

6. No {@html} / XSS (REQ-016, CWE-79, §2.5). grep -rn '@html' across lib/timeline/, DatePrecisionField.svelte, events/, PersonMultiSelect, DocumentMultiSelect → zero. Title, description (plain <textarea>), and OCR/import-derived chip labels ({person.displayName}, {formatDocumentOption(doc)}) all render via default {...} escaping. The formatDocumentOption default-to-DAY change feeds a label interpolated with {...} — no injection.

7. Intentional bare fetch (documentTypeahead.ts). Same-origin in prod (auth cookie travels), Vite-proxied in dev. Comment documents the threat-model reasoning. No new attack surface vs. the existing Geschichte editor; an internal proxy would add complexity with no security benefit. Accepted.

Suggestions

  • None blocking. Optional future hardening: if you ever want defense-in-depth on resolveNavTarget, swapping the generic hex pattern for an RFC-4122-strict regex would reject malformed-but-hex-shaped IDs earlier — but the backend already 404s those, so it's cosmetic.

Secrets/PII: no tokens, DSNs, or PII introduced; the i18n additions are static UI strings. Clean.

### 🔐 Security (Nora "NullX") — PR Review **Verdict: ✅ Approved** Adversarial pass on #781's curator event forms. The frontend gate is correctly treated as UX; the real boundary remains the backend `@RequirePermission(WRITE_ALL)` on #3's CRUD endpoints. Every focus area checks out. #### Blockers - None #### Verified (the load-bearing claims) **1. Anonymous → 403 still holds after the guard refactor (REQ-002/003, §2.1).** `requireWriteAll(locals)` delegates to `hasWriteAll`, whose body is `locals.user?.groups?.some(...) ?? false`. For a null user the optional chain short-circuits to `undefined` → `?? false` → `false` → `throw error(403)`. Dropping the explicit `if (!locals.user)` pre-check opened nothing: a null user never reaches a `.groups` deref. Both loaders (`new/+page.server.ts:23`, `[id]/edit/+page.server.ts:23`) call `requireWriteAll(locals)` as the **first statement**, before any API call. `permissions.spec.ts` pins `hasWriteAll({})` and `hasWriteAll({ user: {} })` → `false`, and both `page.server.spec.ts` files pin null-user → 403. The single existing caller pattern (`documents/[id]/edit`) is unchanged — no parallel predicate drift (§3.2 honored). **2. Open-redirect guard intact (REQ-015, CWE-601).** `resolveNavTarget` (`eventFormServer.ts:24`) returns `/persons/{id}` only when `UUID_RE.test(raw)` passes; the regex is anchored both ends (`^…$`), so a crafted hidden `originPersonId` like `../evil` or `//evil.com` fails and falls to `/zeitstrahl` (test: `defaults to /zeitstrahl when originPersonId is not a valid UUID`). The UUID branch can only emit a same-origin relative path with no scheme — no protocol-relative escape. Note (non-blocking): the regex is generic 8-4-4-4-12 hex, not strict RFC-4122 variant/version — but it matches the spec verbatim and is fully sufficient for the CWE-601 guard. **3. No untrusted value reaches the request body (mass-assignment, CWE-639).** `toEventRequest` builds the body from `title`, `type` (narrowed to `PERSONAL|HISTORICAL`), `eventDate`, `precision` (allow-listed against `VALID_PRECISIONS`, else `DAY` — test pins the `NOT_A_REAL_PRECISION` → `DAY` fallback), `eventDateEnd`, `description`, `personIds`/`documentIds`, `version`. `originPersonId` is parsed for redirect only and is **excluded** from the body — it cannot escape into the write payload. `createdBy`/`updatedBy` are server-set in #3's service, never bound here (§2.4). person/document IDs are UUIDs validated by the backend, which is the authZ gate for IDOR. **4. Fail-closed on derived events (REQ-012).** `[id]/edit` load does `if (!result.response.ok) throw error(404)` — covers all non-ok statuses (derived Geburt/Tod/Heirat 404, unknown id, 403). Never renders a blank create form that silently POSTs. Test pins it. **5. No existence leak on prefill (REQ-014).** `new` load swallows 404/403 on the `Promise.all` person/document lookups (`personResult && personResult.response.ok && personResult.data ? […] : []`) — unknown id → empty picker, no thrown error. Test pins valid-person + unknown-document. **6. No `{@html}` / XSS (REQ-016, CWE-79, §2.5).** `grep -rn '@html'` across `lib/timeline/`, `DatePrecisionField.svelte`, `events/`, `PersonMultiSelect`, `DocumentMultiSelect` → zero. Title, description (plain `<textarea>`), and OCR/import-derived chip labels (`{person.displayName}`, `{formatDocumentOption(doc)}`) all render via default `{...}` escaping. The `formatDocumentOption` default-to-DAY change feeds a label interpolated with `{...}` — no injection. **7. Intentional bare `fetch` (documentTypeahead.ts).** Same-origin in prod (auth cookie travels), Vite-proxied in dev. Comment documents the threat-model reasoning. No new attack surface vs. the existing Geschichte editor; an internal proxy would add complexity with no security benefit. Accepted. #### Suggestions - None blocking. Optional future hardening: if you ever want defense-in-depth on `resolveNavTarget`, swapping the generic hex pattern for an RFC-4122-strict regex would reject malformed-but-hex-shaped IDs earlier — but the backend already 404s those, so it's cosmetic. Secrets/PII: no tokens, DSNs, or PII introduced; the i18n additions are static UI strings. Clean.
Author
Owner

🏛️ Architect — PR Review

Verdict: Approved

Blockers

  • None

Notes (boundaries & decision history — all clear)

  • Domain boundaries (§1.4): timeline/EventForm.svelte imports only $lib/person/* and $lib/document/*, which is exactly what the extended eslint.config.js boundaries/dependencies rule now permits (timeline → shared, person, document). The extension mirrors the geschichte editor precedent and is scoped (no to: ['*']). eventFormServer.ts stays inside shared-only imports. No leak.
  • DatePrecisionField is genuinely domain-neutral. It lives in $lib/shared/primitives/ and imports only shared/utils/date, shared/utils/documentDate, and paraglide — zero person/document/timeline references. It is an extraction of logic that already existed verbatim in WhoWhenSection, now serving two real consumers (document/WhoWhenSection + timeline/EventForm). The new precisionInputName prop is a clean string parameterization (default metaDatePrecision; timeline overrides to precision) — pure config, not a domain hook. This is the correct way to avoid the cross-domain import (Decision 5). The shared-contract props interface is documented in the component header.
  • Decision 14 / §3.2 — requireWriteAll is a legitimate thin guard, NOT a parallel predicate. It is if (!hasWriteAll(locals)) throw error(403, 'Forbidden') — it delegates to the single hasWriteAll predicate rather than re-implementing the groups[].permissions.includes('WRITE_ALL') check. There is exactly one place that decides WRITE_ALL; requireWriteAll only composes the throw-403 behavior over it. The drift risk Decision 14 guards against (two predicates for one decision) does not exist here. It has two real callers in this PR (both new timeline route loads) and factors out the if (!hasWriteAll) throw 403 line that documents/[id]/edit already repeats — so it is reuse-positive, not speculative abstraction. Approved as composed-guard-over-predicate.
  • No ADR triggered, no superseded-ADR violation. Pure frontend: no new backend package/entity/migration/ErrorCode, no new runtime dependency. The eslint boundary extension follows the established geschichte pattern (not a novel architectural decision of lasting consequence). No Accepted ADR is contradicted. Issue body confirms (§Overview).
  • Doc currency met (blocker-class check): new SvelteKit routes → CLAUDE.md route table updated with the events/ child ✓, docs/architecture/c4/l3-frontend-3c-people-stories.puml updated with the zeitstrahlEvents component + backend Rel ✓, frontend/CLAUDE.md updated ✓. RTM rows REQ-001…017 added under feature timeline-curator-forms. No Permission/ErrorCode/GLOSSARY obligations incurred.

Suggestions

  • Minor (non-blocking): the JSDoc on requireWriteAll is good; consider a one-line note that documents/[id]/edit could later adopt it too, so the duplicate if (!hasWriteAll) throw error(403) doesn't quietly diverge. Not for this PR.
### 🏛️ Architect — PR Review **Verdict: ✅ Approved** #### Blockers - None #### Notes (boundaries & decision history — all clear) - **Domain boundaries (§1.4):** `timeline/EventForm.svelte` imports only `$lib/person/*` and `$lib/document/*`, which is exactly what the extended `eslint.config.js` `boundaries/dependencies` rule now permits (`timeline → shared, person, document`). The extension mirrors the geschichte editor precedent and is scoped (no `to: ['*']`). `eventFormServer.ts` stays inside `shared`-only imports. No leak. - **`DatePrecisionField` is genuinely domain-neutral.** It lives in `$lib/shared/primitives/` and imports only `shared/utils/date`, `shared/utils/documentDate`, and paraglide — zero person/document/timeline references. It is an *extraction* of logic that already existed verbatim in `WhoWhenSection`, now serving two real consumers (`document/WhoWhenSection` + `timeline/EventForm`). The new `precisionInputName` prop is a clean string parameterization (default `metaDatePrecision`; timeline overrides to `precision`) — pure config, not a domain hook. This is the correct way to avoid the cross-domain import (Decision 5). The shared-contract props interface is documented in the component header. - **Decision 14 / §3.2 — `requireWriteAll` is a legitimate thin guard, NOT a parallel predicate.** It is `if (!hasWriteAll(locals)) throw error(403, 'Forbidden')` — it *delegates* to the single `hasWriteAll` predicate rather than re-implementing the `groups[].permissions.includes('WRITE_ALL')` check. There is exactly one place that *decides* WRITE_ALL; `requireWriteAll` only *composes* the throw-403 behavior over it. The drift risk Decision 14 guards against (two predicates for one decision) does not exist here. It has two real callers in this PR (both new timeline route loads) and factors out the `if (!hasWriteAll) throw 403` line that `documents/[id]/edit` already repeats — so it is reuse-positive, not speculative abstraction. Approved as composed-guard-over-predicate. - **No ADR triggered, no superseded-ADR violation.** Pure frontend: no new backend package/entity/migration/ErrorCode, no new runtime dependency. The eslint boundary extension follows the established geschichte pattern (not a novel architectural decision of lasting consequence). No Accepted ADR is contradicted. Issue body confirms (§Overview). - **Doc currency met (blocker-class check):** new SvelteKit routes → `CLAUDE.md` route table updated with the `events/` child ✓, `docs/architecture/c4/l3-frontend-3c-people-stories.puml` updated with the `zeitstrahlEvents` component + backend Rel ✓, `frontend/CLAUDE.md` updated ✓. RTM rows REQ-001…017 added under feature `timeline-curator-forms`. No `Permission`/`ErrorCode`/`GLOSSARY` obligations incurred. #### Suggestions - Minor (non-blocking): the JSDoc on `requireWriteAll` is good; consider a one-line note that `documents/[id]/edit` could later adopt it too, so the duplicate `if (!hasWriteAll) throw error(403)` doesn't quietly diverge. Not for this PR.
Author
Owner

🎨 UI/UX — PR Review

Persona: Leonie Voss (@leonievoss) — independent SDD code review of PR #832 / closes #781.

I checked form states, i18n completeness (de/en/es), accessibility semantics, brand tokens, and the responsive collapse. Strong implementation — the senior-audience requirements are taken seriously throughout (48px targets, redundant ⚠ + text cues, aria-live, labelled pickers, card-per-section).

Verdict: Approved

Blockers

  • None.

What I verified (PASS)

  • Form states — loading/submitting (disabled={submitting} + disabled:opacity-50 visible cue), empty (picker emptyLabel empty states event_editor_persons_empty / event_editor_documents_empty), error (inline role="alert" mapped via getErrorMessage), and disabled-while-submitting are all present. beforeNavigate unsaved-changes guard is wired. REQ-007/010/011/013 satisfied.
  • i18n completeness — all 25 new event_editor_* / event_type_* keys present and correctly translated in all three messages/{de,en,es}.json; a11y_type_changed (used by EventTypeSelect) already exists in all three. No hard-coded literals in the components. (REQ-010/017 i18n)
  • EventTypeSelect — proper role="radiogroup" + two role="radio" buttons, grid-cols-2, aria-checked, roving tabindex, radioGroupNav keyboard action, sr-only aria-live announcement. Icon is aria-hidden="true" decorative + visible localized text label → non-color-only differentiation confirmed (Decision 4). The onchange refactor (onchange={(t) => { type = t; markDirty(); }}) is correct — it both syncs the bound state and marks the form dirty; no regression.
  • Accessibilityaria-required + aria-invalid + aria-describedby per field; end-date aria-live="polite" correctly on the outer wrapper of the RANGE region (REQ-008/009); both MultiSelect remove buttons upgraded from ~12px to min-h-[44px] min-w-[44px] with focus-visible ring (REQ-017); pickers now use real <label for=…> not placeholder-as-label; ConfirmDialog gains aria-modal="true". All focus rings use focus-visible:ring-focus-ring.
  • Brand/tokens — card pattern per section (rounded-sm border border-line bg-surface shadow-sm p-6), section titles text-xs font-bold uppercase tracking-widest text-ink-3, font-serif H1, <BackButton> (not <a>), semantic bg-primary/text-danger tokens. Title input/textarea use text-base (16px) — meets the senior-audience body-text floor.
  • Responsivelg:grid-cols-[2fr_1fr] collapsing to single column below lg is pinned; thin E2E asserts no 320px overflow (REQ-017 / Decision 17).
  • document-chip date label — the formatDocumentOption change (precision-less DocumentRef now defaults to DAY → full date instead of the "Unbekannt"/UNKNOWN label) is the better UX: a linked letter's chip shows its real date rather than a misleading "unknown" word. Unit-tested (documentTypeahead.spec.ts). Confirmed correct.

Suggestions (non-blocking)

  • Dark-mode error-text contrastDatePrecisionField.svelte (date-invalid + end-before-start) uses hard-coded text-red-600 (#dc2626) for error text. On the dark surface, #dc2626 is borderline against the 4.5:1 floor the issue explicitly flagged. EventForm.svelte already does the right thing by using the semantic text-danger token (which remaps to the lighter #e55347 in dark mode). This is pre-existing code lifted verbatim from WhoWhenSection during the extraction, not a regression — but since the primitive is now shared by two consumers, migrating its two text-red-600 (and border-red-400/ring-red-500) usages to the semantic text-danger token would close the dark-mode gap for both. Low effort, follow-up-able.
  • Icon size nit — EventTypeSelect uses an emoji glyph at text-lg rather than the spec's "~20×20px SVG icon". Functionally fine (decorative, aria-hidden, redundant with the text label), and emoji rendering varies by platform; worth noting only if brand consistency with other SVG-icon controls matters later.

Nice work — this clears the accessibility and brand bar I care about for the 60+ author audience. The single dark-mode contrast item is inherited debt, not introduced here, so it does not block.

— Leonie

### 🎨 UI/UX — PR Review *Persona: Leonie Voss (@leonievoss) — independent SDD code review of PR #832 / closes #781.* I checked form states, i18n completeness (de/en/es), accessibility semantics, brand tokens, and the responsive collapse. Strong implementation — the senior-audience requirements are taken seriously throughout (48px targets, redundant ⚠ + text cues, aria-live, labelled pickers, card-per-section). **Verdict: ✅ Approved** #### Blockers - None. #### What I verified (PASS) - **Form states** — loading/submitting (`disabled={submitting}` + `disabled:opacity-50` visible cue), empty (picker `emptyLabel` empty states `event_editor_persons_empty` / `event_editor_documents_empty`), error (inline `role="alert"` mapped via `getErrorMessage`), and disabled-while-submitting are all present. `beforeNavigate` unsaved-changes guard is wired. REQ-007/010/011/013 satisfied. - **i18n completeness** — all 25 new `event_editor_*` / `event_type_*` keys present and correctly translated in **all three** `messages/{de,en,es}.json`; `a11y_type_changed` (used by EventTypeSelect) already exists in all three. No hard-coded literals in the components. (REQ-010/017 i18n) - **EventTypeSelect** — proper `role="radiogroup"` + two `role="radio"` buttons, `grid-cols-2`, `aria-checked`, roving `tabindex`, `radioGroupNav` keyboard action, `sr-only` aria-live announcement. Icon is `aria-hidden="true"` decorative + visible localized text label → non-color-only differentiation confirmed (Decision 4). The `onchange` refactor (`onchange={(t) => { type = t; markDirty(); }}`) is correct — it both syncs the bound state and marks the form dirty; no regression. - **Accessibility** — `aria-required` + `aria-invalid` + `aria-describedby` per field; end-date `aria-live="polite"` correctly on the **outer** wrapper of the RANGE region (REQ-008/009); both MultiSelect remove buttons upgraded from ~12px to `min-h-[44px] min-w-[44px]` with focus-visible ring (REQ-017); pickers now use real `<label for=…>` not placeholder-as-label; `ConfirmDialog` gains `aria-modal="true"`. All focus rings use `focus-visible:ring-focus-ring`. - **Brand/tokens** — card pattern per section (`rounded-sm border border-line bg-surface shadow-sm p-6`), section titles `text-xs font-bold uppercase tracking-widest text-ink-3`, `font-serif` H1, `<BackButton>` (not `<a>`), semantic `bg-primary`/`text-danger` tokens. Title input/textarea use `text-base` (16px) — meets the senior-audience body-text floor. - **Responsive** — `lg:grid-cols-[2fr_1fr]` collapsing to single column below `lg` is pinned; thin E2E asserts no 320px overflow (REQ-017 / Decision 17). - **document-chip date label** — the `formatDocumentOption` change (precision-less `DocumentRef` now defaults to `DAY` → full date instead of the "Unbekannt"/UNKNOWN label) is the **better UX**: a linked letter's chip shows its real date rather than a misleading "unknown" word. Unit-tested (`documentTypeahead.spec.ts`). Confirmed correct. #### Suggestions (non-blocking) - **Dark-mode error-text contrast** — `DatePrecisionField.svelte` (date-invalid + end-before-start) uses hard-coded `text-red-600` (#dc2626) for error text. On the dark surface, #dc2626 is borderline against the 4.5:1 floor the issue explicitly flagged. `EventForm.svelte` already does the right thing by using the semantic `text-danger` token (which remaps to the lighter `#e55347` in dark mode). This is **pre-existing** code lifted verbatim from `WhoWhenSection` during the extraction, not a regression — but since the primitive is now shared by two consumers, migrating its two `text-red-600` (and `border-red-400`/`ring-red-500`) usages to the semantic `text-danger` token would close the dark-mode gap for both. Low effort, follow-up-able. - **Icon size nit** — EventTypeSelect uses an emoji glyph at `text-lg` rather than the spec's "~20×20px SVG icon". Functionally fine (decorative, aria-hidden, redundant with the text label), and emoji rendering varies by platform; worth noting only if brand consistency with other SVG-icon controls matters later. Nice work — this clears the accessibility and brand bar I care about for the 60+ author audience. The single dark-mode contrast item is inherited debt, not introduced here, so it does not block. — Leonie
marcel added 4 commits 2026-06-14 00:34:53 +02:00
Decision 6 / REQ-010 promised the pickers survive a fail(400) "including
pre-selected persons/documents", but EventForm only seeded them from
event/initialPersons — never from the fail payload — and the payload carried
only bare ids, which can't rebuild a chip (chips need displayName/title). On
the use:enhance path the in-memory selection survived; on a no-JS full reload
the chips were silently dropped.

Now the save action re-fetches the selected persons/documents by id
(lookupSelections, non-ok swallowed like the prefill path) and returns full
chip data; EventForm seeds the pickers from form.persons/documents ahead of
the seeded event. Extract preservedFormFields() to DRY the four fail payloads;
validateEventForm now returns the error pair and the route owns the fail().

Component test pins the rehydration; the server spec now asserts the fail
payload carries labelled chips, not just ids.

Addresses PR #832 review (Developer + Requirements Engineer concern, REQ-010).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
REQ-011 asks for both required-field errors via per-field aria-invalid, but the
blank-date error was rendered as a standalone <p> after the date card while the
date input's aria-invalid only reflected the client-side malformed-date cue.

DatePrecisionField gains a `dateError` prop: a server error now marks the field
aria-invalid and renders inline under the input (sharing the same error id), and
EventForm drops its detached <p>. While here, migrate the field's two error
texts from hard-coded text-red-600 to the semantic `text-danger` token so they
keep ≥4.5:1 contrast in dark mode (the token remaps; #dc2626 was borderline) —
this also fixes the contrast for WhoWhenSection, the other consumer.

Component test asserts the date input gains aria-invalid on a server date error.

Addresses PR #832 review (Requirements Engineer REQ-011; UI/UX dark-mode contrast).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tighten the review-flagged test gaps (no production change):
- submitting state: the old test only asserted the button was initially
  enabled (a tautology). Now stub a never-resolving fetch, click Speichern, and
  assert the button gains `disabled` — exercising the double-submit guard
  (Decision 8).
- the two redirect-throwing save tests now use `await expect(...).rejects` so a
  future missing redirect fails loudly instead of being swallowed by try/catch.
- the YEAR end-date-hide assertion uses getByLabelText('Enddatum') not-present,
  symmetric with the RANGE reveal, instead of a raw #eventDateEnd querySelector.
- PersonMultiSelect + DocumentMultiSelect: assert the chip remove button carries
  the min-h-[44px]/min-w-[44px] target the rtm cites for REQ-017.

Addresses PR #832 review (Tester + Requirements Engineer test-coverage concerns).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
docs(permissions): note requireWriteAll can replace the inline guard elsewhere
Some checks failed
CI / Semgrep Security Scan (pull_request) Successful in 25s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m6s
SDD Gate / RTM Check (pull_request) Successful in 14s
SDD Gate / Contract Validate (pull_request) Successful in 23s
SDD Gate / Constitution Impact (pull_request) Successful in 21s
CI / Unit & Component Tests (pull_request) Failing after 3m19s
CI / OCR Service Tests (pull_request) Successful in 22s
CI / Backend Unit Tests (pull_request) Successful in 4m49s
CI / fail2ban Regex (pull_request) Successful in 47s
719274ef88
Architect/Developer review suggestion: flag that other WRITE_ALL-gated author
loads (e.g. documents/[id]/edit) still inline the throw-403 guard and can adopt
requireWriteAll so it doesn't diverge. Comment-only.

Addresses PR #832 review (Architect suggestion).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
marcel added 1 commit 2026-06-14 00:40:00 +02:00
test(document): update WhoWhenSection.test ids after DatePrecisionField extraction
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 3m58s
CI / OCR Service Tests (pull_request) Successful in 25s
CI / Backend Unit Tests (pull_request) Successful in 5m32s
CI / fail2ban Regex (pull_request) Successful in 55s
CI / Semgrep Security Scan (pull_request) Successful in 32s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m8s
SDD Gate / RTM Check (pull_request) Successful in 16s
SDD Gate / Contract Validate (pull_request) Successful in 28s
SDD Gate / Constitution Impact (pull_request) Successful in 21s
d330510777
The DatePrecisionField extraction derives element ids from dateInputName, so the
document form's precision/end-date ids changed (metaDatePrecision →
documentDatePrecision, metaDateEnd → documentDateEnd, date-error →
documentDate-error, end-date-error → documentDate-end-error). The name attributes
are unchanged, so form submission is unaffected — but the pre-existing
WhoWhenSection.svelte.test.ts (a separate file from the .spec.ts) still queried
the old ids and was failing 5 assertions in CI's client project. It wasn't in
the PR diff, so the multi-persona review missed it. Re-point the selectors.

Addresses PR #832 review (round-1 clean-agent blocker).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
marcel added 2 commits 2026-06-14 00:45:05 +02:00
DatePrecisionField derives the precision select's id from dateInputName, so the
document form's id moved from #metaDatePrecision to #documentDatePrecision (the
name attribute is unchanged). This maintained E2E still queried the old id and
would fail when run. Not CI-gated, but a real silent breakage.

Addresses PR #832 review (round-2 clean-agent out-of-diff finding).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix(timeline): mark the event form dirty on date/precision/picker changes
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 4m53s
CI / OCR Service Tests (pull_request) Successful in 24s
CI / Backend Unit Tests (pull_request) Successful in 5m46s
CI / fail2ban Regex (pull_request) Failing after 46s
CI / Semgrep Security Scan (pull_request) Successful in 25s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m7s
SDD Gate / RTM Check (pull_request) Successful in 17s
SDD Gate / Contract Validate (pull_request) Successful in 24s
SDD Gate / Constitution Impact (pull_request) Successful in 19s
0862d43ba3
The beforeNavigate unsaved-changes guard only fired for title/type/description
(their oninput/onchange hooks set `dirty`). Editing only the date, precision,
end-date, or the linked persons/documents left `dirty` false, so a curator could
navigate away and silently lose those edits — defeating the guard for the senior
author audience. Add an $effect that watches those values and marks dirty on any
change after the initial prop snapshot (first run only arms the watcher).

Addresses PR #832 review (round-2 clean-agent concern).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
marcel added 1 commit 2026-06-14 00:55:58 +02:00
fix(timeline): track form dirtiness via change callbacks, not an $effect
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 4m17s
CI / OCR Service Tests (pull_request) Successful in 24s
CI / Backend Unit Tests (pull_request) Successful in 5m23s
CI / fail2ban Regex (pull_request) Successful in 47s
CI / Semgrep Security Scan (pull_request) Successful in 26s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m8s
SDD Gate / RTM Check (pull_request) Successful in 16s
SDD Gate / Contract Validate (pull_request) Successful in 25s
SDD Gate / Constitution Impact (pull_request) Successful in 17s
6150fc7be5
The round-2 dirty-guard used an $effect that both read and wrote its `dirtyArmed`
$state, so the self-write re-triggered the effect and `dirty` flipped true on
mount — the beforeNavigate confirm then fired on every navigation away from an
untouched form (caught by the round-3 clean-agent review + the Svelte autofixer,
which flags assigning state inside $effect).

Replace it with the component's existing idiomatic pattern: DatePrecisionField,
PersonMultiSelect, and DocumentMultiSelect each gain an optional `onchange`
callback fired on a real user edit, and EventForm passes `markDirty` to all
three. Now date/precision/end-date and picker add/remove mark the form dirty
exactly like title/type/description already did — no effect, no mount-timing
trap. The new props are optional, so the other consumers (WhoWhenSection, the
document forms) are unaffected. Svelte autofixer: clean.

Addresses PR #832 review (round-3 clean-agent concern).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 4m17s
CI / OCR Service Tests (pull_request) Successful in 24s
CI / Backend Unit Tests (pull_request) Successful in 5m23s
CI / fail2ban Regex (pull_request) Successful in 47s
CI / Semgrep Security Scan (pull_request) Successful in 26s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m8s
SDD Gate / RTM Check (pull_request) Successful in 16s
SDD Gate / Contract Validate (pull_request) Successful in 25s
SDD Gate / Constitution Impact (pull_request) Successful in 17s
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat/issue-781-timeline-curator-forms:feat/issue-781-timeline-curator-forms
git checkout feat/issue-781-timeline-curator-forms
Sign in to join this conversation.
No Reviewers
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#832