Timeline: curator event create/edit forms (#781) #832
Open
marcel
wants to merge 23 commits from
feat/issue-781-timeline-curator-forms into main
pull from: feat/issue-781-timeline-curator-forms
merge into: marcel:main
marcel:main
marcel:feat/issue-818-renovate-nightly-audit
marcel:feat/issue-803-geschichten-document-filter-chip
marcel:worktree-feat+issue-738-nl-search-backend
marcel:feat/issue-286-notification-bell-form-actions
marcel:feat/issue-580-sentry-backend
marcel:fix/issue-593-management-port-zero
marcel:worktree-feat+issue-557-upload-artifact-v3-pin
marcel:worktree-chore+issue-556-drop-client-branches-coverage-gate
marcel:fix/issue-514-prerender-crawl-bakes-redirects
marcel:fix/issue-472-prerender-entries
marcel:feat/issue-395-readme
marcel:feat/issue-345-bulk-mark-reviewed
marcel:feat/issue-344-bell-tooltip
marcel:feat/issue-341-annotieren-contrast
marcel:feat/issue-225-bulk-metadata-edit
marcel:feat/issue-317-bulk-upload
marcel:feat/issue-271-dashboard-redesign
marcel:docs/issue-240-mission-control-spec
marcel:refactor/issues-193-200
marcel:feat/issue-162-korrespondenz-redesign
marcel:feature/68-new-document-file-first
marcel:feat/81-discussion-discoverability
marcel:feat/62-document-bottom-panel
marcel:feature/56-backfill-file-hashes
marcel:feat/38-document-edit-history
marcel:fix/svelte5-test-delegation-and-login-auth
No Reviewers
Labels
Clear labels
P0-critical
P1-high
P2-medium
P3-later
audit
bug
cleanup
collaboration
conversation
descoped
devops
documentation
epic
feature
file-upload
legibility
needs-discussion
notification
person
phase-1: security
phase-2: container-images
phase-3: prod-compose
phase-4: spring-prod-profile
phase-5: backups
phase-6: deployment-docs
phase-7: monitoring
refactor
security
test
ui
user
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
Noticeable improvement; doesn't block anything; schedule opportunistically.
Cosmetic or parking-lot work; fine to stay open indefinitely.
Read-only audit / assessment work; produces a report rather than changing code
Something isn't working
Removal of dead code, vague comments, naming violations, and scratch artifacts
We want to extend the family archive to have more collaboration tools
We will do that later
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
Marker for epic-level issues that group multiple child issues
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
Has an open decision or design question that must be resolved before implementation can start.
Security hygiene — must be done first
Production-ready multi-stage Docker images
Production compose overlay + Caddy reverse proxy
Spring Boot production configuration profile
Database and object storage backup strategy
.env.example, DEPLOYMENT.md, runbook
Prometheus, Loki, Grafana, Alertmanager
Code restructuring without behaviour change
UI/UX and accessibility issues
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: marcel/familienarchiv#832
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "feat/issue-781-timeline-curator-forms"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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/?documentIdprefill)./zeitstrahl/events/[id]/edit— edit form seeded fromGET /api/timeline/events/{id}, with the delete control.REQ → test coverage (all green)
new/page.server.spec.ts#allows a curator with WRITE_ALL#throws 403 for an unauthenticated (null) user#throws 403 ... without WRITE_ALLnew#posts a TimelineEventRequest and redirects on successedit#updates via PUT (with version) and redirects on successedit#deletes via DELETE and redirects ...edit#returns fail(status) ... when DELETE is not okEventForm#reveals the end-date field ...,WhoWhenSection#reveals the end-date field ...eventDateEnd: nullEventForm#hides the end-date field ...,new#sends eventDateEnd: null ...EventForm#shows a required-field error ...,new#returns fail(400) with preserved picker arrays ...new#surfaces both title and date errors ...edit#throws 404 when the GET is not ok ...edit#maps a 409 conflict and does not redirect,new#... (incl. 409)new#preselects a valid person and ignores an unknown document,EventForm#preselects a person .../zeitstrahl(CWE-601)new#defaults to /zeitstrahl when originPersonId is not a valid UUID{...}, never{@html}grep -r '@html' src/lib/timeline/→ zeroPersonMultiSelect/DocumentMultiSelect/EventFormspecsNotable implementation choices
DatePrecisionFieldextracted into$lib/shared/primitives/fromWhoWhenSection'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).EventForm, two routes —/newempty,/[id]/editseeded. TypeeventasTimelineEventView, submit asTimelineEventRequest.hasWriteAll(locals)(no parallel helper). 403 error-page gating (thepersons/newidiom), null-user guard first.eslintboundaries extended to lettimelineimportperson+document(mirrors the geschichte editor — the spec-mandated reuse).ConfirmDialoggotaria-modal="true"; both pickers' remove buttons padded to ≥44px;documentTypeahead.tsbare-fetch documented.DocumentOptionprecision fields made optional so aDocumentRef(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.ymldoes not runtest:e2e; the component + server specs carry regression coverage. Productionnpm run buildsucceeds;svelte-checkshows zero new errors in #781 files.Docs
CLAUDE.md route tables, frontend C4 people-stories diagram, and
.specify/rtm.mdREQ-001…017 rows (featuretimeline-curator-forms).Closes #781
🤖 Generated with Claude Code
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>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>🧪 Tester — PR Review
Verdict: ⚠️ Approved with concerns
Blockers
Suggestions
frontend/src/lib/timeline/EventForm.svelte.spec.ts:63-69—should_disable_submit_button_while_submittingis a named test in the spec's "## Tests" list ("assert the button gainsdisabledbefore 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 thesubmitting=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]/disabledbefore 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 agetByTestId('end-date-region').toBeInTheDocument()user-level assertion with a rawdocument.querySelector('#eventDateEnd')DOM probe. The querySelector is fine as the negative assertion, but considerpage.getByLabelText('Enddatum')+.not.toBeInTheDocument()to stay symmetric with the RANGE-reveal test and avoid coupling to the#eventDateEndid.new/page.server.spec.ts:2001and:2023(eventDateEnd-null + unknown-precision→DAY) use atry/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. Preferawait expect(actions.save(...)).rejects.toMatchObject({ status: 303 })and then assert the body, so a missing redirect fails loudly rather than being absorbed.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.eventFormServer.ts:51) is asserted via the request body;documentTypeahead.spec.tscovers both the bare-title and precision-absent→full-date branches and explicitly fences the UNKNOWN-fallback regression; and therequireWriteAllextraction keeps both the null-user and non-WRITE_ALL 403 tests on each route, still assertingrejects.toMatchObject({ status: 403 })— meaningful after the guard moved into the shared helper.📋 Requirements Engineer — PR Review
Verdict: ⚠️ Approved with concerns
All 17 requirements are implemented, traced in
.specify/rtm.md(timeline-curator-forms, allDone), 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: theprecisionfield rename is consistent acrossDatePrecisionField.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;formatDocumentOptiondefaults a missing precision toDAYwith a test;requireWriteAllis extracted and consumed by both route loads; REQ-009 still submitseventDateEnd: null(verified in parse + server spec).Traceability table
requireWriteAllin both loadsnew/...#allows a curator with WRITE_ALL,[id]/edit/...#seeds the form on ok GEThasWriteAll(false → 403)#throws 403 for an unauthenticated (null) user#...without WRITE_ALLresolveNavTargetredirectnew/...#posts a TimelineEventRequest and redirects[id]/edit/...#updates via PUT (with version) and redirectsgetConfirmService[id]/edit/...#deletes via DELETE and redirectsfail, no redirect[id]/edit/...#returns fail(status) and does not redirectshowEndDate = precision==='RANGE'EventForm...#reveals the end-date field when precision is RANGE+WhoWhenSectionregression''off-RANGE → parsed tonullEventForm...#hides...when YEAR+new/...#sends eventDateEnd: null when precision is not RANGEvalidateEventFormpreserves picker arraysnew/...#returns fail(400) with preserved picker arrays on blank titlearia-invalidaria-invalidon the date field[id]/edit/...#throws 404 when the GET is not ok#maps a 409 conflict...)Promise.allprefill, swallow non-oknew/...#preselects a valid person and ignores an unknown document+EventForm...#preselects a personUUID_REinresolveNavTargetnew/...#defaults to /zeitstrahl when ...not a valid UUID+ valid-UUID case{...}grep '@html' frontend/src/lib/timeline/→ zero (verified)min-h-[44px] min-w-[44px]on both pickers,<label for>, empty states)PersonMultiSelect/DocumentMultiSelect.svelte.spec.tsdo not assert the ≥44px targetBlockers
Donewith valid pointers. No unrequited scope-creep found (the cross-domain eslint allowance,DatePrecisionFieldextraction, andConfirmDialog aria-modalpatch are all explicitly mandated by the issue body).Suggestions
aria-invalid." Today the title input carriesaria-invalid, but the server-side blank-datedateErrorrenders as a standalone<p>after the date card and is not wired to the date input'saria-invalid(the field'saria-invalidonly reflects the client-side malformed-datedateInvalid). Wire the serverdateErrorto theDatePrecisionFielddate input'saria-invalid, and add a component-layer assertion. The behavioral core (both errors shown) is covered, so this is a concern, not a blocker.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.should_disable_submit_button_while_submitting;EventForm.svelte.spec.tsonly hasrenders an enabled submit button initially. Thesubmitting/disabledwiring exists in the component but the disabled-while-submitting transition is unasserted. Optional to add.#shows a required-field error when title is blankis a substring of the actual test name (...when title is blank and save is attempted) — matches fine, no action needed.⚙️ DevOps — PR Review
Verdict: ✅ Approved
Blockers
Constitution §4 Do-Not-Touch — all clear (verified against the full diff file list):
frontend/src/lib/generated/api.tsnot edited — only consumed viacomponents['schemas'][...]type references. ✅backend/.../db/migration/file). ✅AcceptedADR edited (nodocs/adr/change; issue correctly states no ADR is triggered). ✅.gitea/workflows/change at all — no artifact action bumped past@v3, no CI guard removed or weakened. ✅docker-compose*.yml/.env*change — no new env var, no exposed port, no migration ordering concern. ✅Operational notes:
frontend/e2e/zeitstrahl-event-editor.spec.tsdocuments in its header thatci.ymldoes not invoketest: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.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:150widens thetimeline → {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.🛠️ Developer (Felix Brandt) — PR Review
Verdict: ⚠️ Approved with concerns
Clean, well-decomposed work.
DatePrecisionFieldextraction is textbook KISS reuse — shared primitive in$lib/shared/primitives/,$bindableprops with no redundant$statemirror,onMountseeding preserved verbatim. The three review-fix commits all hold up:DatePrecisionFieldselectname={precisionInputName}(default'metaDatePrecision'),EventFormoverridesprecisionInputName="precision",parseEventFormreadsformData.get('precision'), specs assertprecision:.WhoWhenSectionomits the prop, so the document form still submitsmetaDatePrecision— unbroken (its regression testreveals the end-date field when precision is RANGEis green). TheVALID_PRECISIONSallow-list withDAYfallback is sound and tested (falls back to DAY precision when an unknown precision token is submitted).formatDocumentOptionDAY default — sound. ADocumentRefchip carriesdocumentDatebut no precision; defaulting toDAYrenders the full date rather than the UNKNOWN label. Unit-tested directly.requireWriteAll(locals)— correct.hasWriteAllreturnsfalsefor 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 parallelhasPermissionintroduced (§3.2 / Decision 14 honored).Pure frontend (no backend model/endpoint change →
npm run generate:apinot needed). No{@html}anywhere in the new code (REQ-016 ✓).{#each}keyed in both new components. No newErrorCode. Layering/boundaries respected — the eslinttimeline → person, documentallow-entry is justified and scoped.Blockers
Suggestions
EventForm.svelte:58-71— picker persistence onfail(400)is incomplete vs. REQ-010 / Decision 6. The server action correctly returnspersonIds/documentIdsarrays in the fail payload, butEventFormseedsselectedPersons/selectedDocumentsonly fromevent?.persons/initialPersons— it never readsform?.personIds/form?.documentIds. Note the title/description/type fields do readform?.*first (line 51-53); the pickers are the odd ones out. In practice this is masked on the primary path becauseuse:enhance'supdate()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 needdisplayName/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 afterfail(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 differenteventto reset" documents a footgun (props are snapshotted into$state, so a parent re-render with neweventwon'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.🔐 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
Verified (the load-bearing claims)
1. Anonymous → 403 still holds after the guard refactor (REQ-002/003, §2.1).
requireWriteAll(locals)delegates tohasWriteAll, whose body islocals.user?.groups?.some(...) ?? false. For a null user the optional chain short-circuits toundefined→?? false→false→throw error(403). Dropping the explicitif (!locals.user)pre-check opened nothing: a null user never reaches a.groupsderef. Both loaders (new/+page.server.ts:23,[id]/edit/+page.server.ts:23) callrequireWriteAll(locals)as the first statement, before any API call.permissions.spec.tspinshasWriteAll({})andhasWriteAll({ user: {} })→false, and bothpage.server.spec.tsfiles 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 whenUUID_RE.test(raw)passes; the regex is anchored both ends (^…$), so a crafted hiddenoriginPersonIdlike../evilor//evil.comfails 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).
toEventRequestbuilds the body fromtitle,type(narrowed toPERSONAL|HISTORICAL),eventDate,precision(allow-listed againstVALID_PRECISIONS, elseDAY— test pins theNOT_A_REAL_PRECISION→DAYfallback),eventDateEnd,description,personIds/documentIds,version.originPersonIdis parsed for redirect only and is excluded from the body — it cannot escape into the write payload.createdBy/updatedByare 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]/editload doesif (!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).
newload swallows 404/403 on thePromise.allperson/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'acrosslib/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. TheformatDocumentOptiondefault-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
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.
🏛️ Architect — PR Review
Verdict: ✅ Approved
Blockers
Notes (boundaries & decision history — all clear)
timeline/EventForm.svelteimports only$lib/person/*and$lib/document/*, which is exactly what the extendedeslint.config.jsboundaries/dependenciesrule now permits (timeline → shared, person, document). The extension mirrors the geschichte editor precedent and is scoped (noto: ['*']).eventFormServer.tsstays insideshared-only imports. No leak.DatePrecisionFieldis genuinely domain-neutral. It lives in$lib/shared/primitives/and imports onlyshared/utils/date,shared/utils/documentDate, and paraglide — zero person/document/timeline references. It is an extraction of logic that already existed verbatim inWhoWhenSection, now serving two real consumers (document/WhoWhenSection+timeline/EventForm). The newprecisionInputNameprop is a clean string parameterization (defaultmetaDatePrecision; timeline overrides toprecision) — 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.requireWriteAllis a legitimate thin guard, NOT a parallel predicate. It isif (!hasWriteAll(locals)) throw error(403, 'Forbidden')— it delegates to the singlehasWriteAllpredicate rather than re-implementing thegroups[].permissions.includes('WRITE_ALL')check. There is exactly one place that decides WRITE_ALL;requireWriteAllonly 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 theif (!hasWriteAll) throw 403line thatdocuments/[id]/editalready repeats — so it is reuse-positive, not speculative abstraction. Approved as composed-guard-over-predicate.CLAUDE.mdroute table updated with theevents/child ✓,docs/architecture/c4/l3-frontend-3c-people-stories.pumlupdated with thezeitstrahlEventscomponent + backend Rel ✓,frontend/CLAUDE.mdupdated ✓. RTM rows REQ-001…017 added under featuretimeline-curator-forms. NoPermission/ErrorCode/GLOSSARYobligations incurred.Suggestions
requireWriteAllis good; consider a one-line note thatdocuments/[id]/editcould later adopt it too, so the duplicateif (!hasWriteAll) throw error(403)doesn't quietly diverge. Not for this PR.🎨 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
What I verified (PASS)
disabled={submitting}+disabled:opacity-50visible cue), empty (pickeremptyLabelempty statesevent_editor_persons_empty/event_editor_documents_empty), error (inlinerole="alert"mapped viagetErrorMessage), and disabled-while-submitting are all present.beforeNavigateunsaved-changes guard is wired. REQ-007/010/011/013 satisfied.event_editor_*/event_type_*keys present and correctly translated in all threemessages/{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)role="radiogroup"+ tworole="radio"buttons,grid-cols-2,aria-checked, rovingtabindex,radioGroupNavkeyboard action,sr-onlyaria-live announcement. Icon isaria-hidden="true"decorative + visible localized text label → non-color-only differentiation confirmed (Decision 4). Theonchangerefactor (onchange={(t) => { type = t; markDirty(); }}) is correct — it both syncs the bound state and marks the form dirty; no regression.aria-required+aria-invalid+aria-describedbyper field; end-datearia-live="polite"correctly on the outer wrapper of the RANGE region (REQ-008/009); both MultiSelect remove buttons upgraded from ~12px tomin-h-[44px] min-w-[44px]with focus-visible ring (REQ-017); pickers now use real<label for=…>not placeholder-as-label;ConfirmDialoggainsaria-modal="true". All focus rings usefocus-visible:ring-focus-ring.rounded-sm border border-line bg-surface shadow-sm p-6), section titlestext-xs font-bold uppercase tracking-widest text-ink-3,font-serifH1,<BackButton>(not<a>), semanticbg-primary/text-dangertokens. Title input/textarea usetext-base(16px) — meets the senior-audience body-text floor.lg:grid-cols-[2fr_1fr]collapsing to single column belowlgis pinned; thin E2E asserts no 320px overflow (REQ-017 / Decision 17).formatDocumentOptionchange (precision-lessDocumentRefnow defaults toDAY→ 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)
DatePrecisionField.svelte(date-invalid + end-before-start) uses hard-codedtext-red-600(#dc2626) for error text. On the dark surface, #dc2626 is borderline against the 4.5:1 floor the issue explicitly flagged.EventForm.sveltealready does the right thing by using the semantictext-dangertoken (which remaps to the lighter#e55347in dark mode). This is pre-existing code lifted verbatim fromWhoWhenSectionduring the extraction, not a regression — but since the primitive is now shared by two consumers, migrating its twotext-red-600(andborder-red-400/ring-red-500) usages to the semantictext-dangertoken would close the dark-mode gap for both. Low effort, follow-up-able.text-lgrather 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
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>View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.