feat(persons): surface personType + title in forms and detail card #333
Reference in New Issue
Block a user
Delete Branch "feat/issue-218-person-title-type-fields"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #218
Summary
personTypeis now@NotNullinPersonUpdateDTO;title(@Size max=50) added. Both are persisted on create/update. SKIP is rejected withINVALID_PERSON_TYPE.firstNameis only required for PERSON type.PersonTypeSelectorsegmented control (4 types, keyboard nav, ARIA). Edit and new-person forms show the selector at the top, a title field (PERSON only), and hide firstName/alias/years for non-PERSON types.PersonCardshows title in small-caps above the display name.What changed
Backend
PersonUpdateDTO—personType(@NotNull),title(@Size max=50)PersonService— persists both fields; SKIP guard on create + updatePersonController—validatePersonNames()helper; firstName required only for PERSONErrorCode.INVALID_PERSON_TYPE+ i18n (de/en/es)Frontend
radioGroupNav.ts— ArrowLeft/Right keyboard navigation action (7 browser tests)PersonTypeSelector.svelte— 4-segment radiogroup, 2×2 mobile grid, min-h-[48px], aria-livePersonEditForm.svelte— type selector, title input, conditional fields per spec §4 matrix+page.server.ts— extracts personType + title; type-aware firstName validation+page.svelte++page.server.ts— same structure as edit formPersonCard.svelte— title in[font-variant:small-caps]above displayName (PERSON only)Test results
🏗️ Markus Keller — Software Architect
Verdict: ⚠️ Approved with concerns
Blockers
None — the changes are safe to merge.
Concerns
1. Breaking API contract on
personType@NotNullonPersonUpdateDTO.personTypemakes omittingpersonTypein a JSON body a Bean Validation violation. The error response produced by Spring's constraint handler is the default Spring format ({ "errors": [{ "field": "personType", "message": "..." }] }), not the project's{ "code": "..." }format. The frontendgetErrorMessage()can't map it. In practice, the frontend always sendspersonTypenow, so there's no user-visible bug. But: any other caller (import scripts, API tests) that previously sent{ "firstName": "...", "lastName": "..." }will silently break. ThePersonControllerTesttests now all includepersonType— good — but confirm no integration caller is omitting it before deploying.2. SKIP guard ordering in
PersonService.updatePersonThe SKIP check runs before
personRepository.findById(id). If a caller submits SKIP for a non-existent ID, they get a 400 instead of a 404. Minor semantic inversion — usually "does the resource exist" comes before "is the input valid".Suggestions
3. PR scope creep — FTS fix bundled in
The
to_tsquery('simple')change inDocumentRepositoryis a good fix, but it has no relation to person-type surfacing. Makes bisecting harder if the FTS change ever causes a regression. Ideally a separate PR, but pragmatically acceptable since the regression test is included.4. Layering is correct
validatePersonNamesin the controller handles input validation (controller concern). SKIP guard inPersonServiceenforces a domain invariant (service concern). That's the right split per CLAUDE.md layering rules.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1.
page.server.test.tstests a local copy, not production codenormalizePersonTypeis defined inside the test file. The actual production code in+page.server.tsinlines the same logic:These tests pass because they test a locally-defined helper that matches the production intent — but if someone later changes the inline expression in
+page.server.ts, the test file keeps passing. The tests provide documentation value but no regression protection.Fix: Extract
normalizePersonTypefrom+page.server.tsas a named export, then import it in the test file. One line change in production code, and the test binding becomes real.Suggestions
2. Focus ring token inconsistency
PersonTypeSelector.svelteusesfocus-visible:ring-brand-navywhile every input inPersonEditForm.svelteusesfocus-visible:ring-focus-ring. These may resolve to the same color via CSS variables, but the tokens should be consistent. Swapring-brand-navy→ring-focus-ringin PersonTypeSelector.3.
untrack()pattern — correctUsed properly in all three components (
PersonTypeSelector,PersonEditForm, new-person page) for one-time initialization from props without establishing a reactive dependency. This is the right Svelte 5 idiom.4.
labelsrecord — good callUsing
Record<PersonType, () => string>instead of dynamicm[person_type_${selected}]()keeps TypeScript happy without type assertions. Clean.5.
radioGroupNavtests — excellent7 tests covering ArrowRight/ArrowLeft, wrap-around in both directions, aria-checked updates, destroy cleanup, and ignoring non-arrow keys. Nothing missing here.
6.
@NotNullon DTO is a breaking changeFlagged by the architect — same concern here from a code perspective. All
PersonControllerTesttest bodies have been updated to includepersonType. Make sure the same is done in any*.httpAPI test files.🔐 Nora "NullX" Steiner — Security Expert
Verdict: ✅ Approved
What I checked
Findings
1. Defense in depth on SKIP — correct
SKIP is blocked at two layers: the service throws
DomainException.badRequest(INVALID_PERSON_TYPE, ...)for both create and update paths. The@NotNullconstraint onpersonTypeprevents null from reaching either path. Neither layer alone is sufficient (null ≠ SKIP), but together they cover the full rejection surface.2. Bean Validation error format leaks field names
When
personTypeis null, Spring returns a constraint violation response that includes the field name and constraint type (e.g."field": "personType", "defaultMessage": "must not be null"). This is mild information disclosure — it reveals DTO structure. For a family-only internal tool this is acceptable, but worth noting as a pattern to avoid on public-facing APIs.3. Title not trimmed server-side
titleis trimmed in the frontend action (formData.get('title')?.toString().trim()) but there's no equivalent trim in the controller or service layer. An API caller submitting"title": " Prof. "would persist the whitespace. Not a security issue, just a data quality gap.4. FTS change is safe
to_tsquery('simple', regexp_replace(...))continues to use parameterized values — no SQL injection surface introduced. Thesimpledictionary change is a correctness fix, not a security-relevant one.5. RBAC unchanged
@RequirePermission(WRITE_ALL)guards oncreatePersonandupdatePersonare untouched. ✅🧪 Sara Holt — QA / Tester
Verdict: ⚠️ Approved with concerns
Blockers
1.
page.server.test.tsdoesn't test production codeThe test file defines its own copy of
normalizePersonTypeand tests that. The actual production logic in+page.server.tsis the inline ternaryperson.personType === 'SKIP' ? 'UNKNOWN' : person.personType. These tests would stay green even if that production line were deleted or corrupted. This is a phantom test — it gives false confidence in coverage.Missing coverage
2. No controller-level SKIP rejection test
PersonServiceTesttests the SKIP guard in the service.PersonControllerTestadds a test for INSTITUTION without firstName, but no test for submittingpersonType: "SKIP"via the HTTP endpoint. A controller integration test likecreatePerson_returns400_whenPersonTypeIsSkipwould confirm the full stack rejects it (not just the service in isolation).3. Edit form action validation untested
The server action in
edit/+page.server.tsnow has type-aware validation (personType === 'PERSON' && !firstName). There are no unit or integration tests for this path. The analogous path innew/+page.server.tsis also untested. The frontend validates before submit, but the server-side is still an untested code path.What's well-covered ✅
radioGroupNav.svelte.spec.ts— 7 tests, all keyboard edge cases including destroy and non-arrow keysDocumentFtsTest— new stop-word test is well-named, self-documenting, and confirms the root causePersonServiceTest— new tests forpersistsPersonType,persistsTitle, and both SKIP guardsPersonControllerTest— updated all existing tests to includepersonType, added INSTITUTION without firstName test🎨 Leonie Voss — UI/UX Expert
Verdict: ✅ Approved
What I checked
Findings
Accessibility — strong implementation
role="radiogroup"on the container +role="radio"on each button +aria-checked+ roving tabindex (tabindex={selected === type ? 0 : -1}) — this is textbook WAI-ARIA 1.2 radio group pattern.radioGroupNavaction handles wrap-around correctly in both directions.aria-live="polite"announces type changes to screen readers without interrupting. The region only fires on changes, not on page load. ✅Touch targets — correct
min-h-[48px]on each button exceeds WCAG 2.5.5 (44px). ✅Title typography — nice
[font-variant:small-caps]for honorifics above the name in PersonCard is the right visual treatment for honorific prefixes like "Dr." or "Prof.". Gated correctly onpersonType === 'PERSON' && person.title. ✅Adaptive labels — correct
lastNameLabelswitches between "Name" (INSTITUTION/GROUP) and "Nachname" (PERSON/UNKNOWN). This prevents institutions from being asked for a "surname". ✅Form repopulation — complete
The new-person server action now echoes back
personType,title,firstName,lastName,aliason validation failure. Users don't lose their input. ✅Minor: focus ring token mismatch
PersonTypeSelectorusesfocus-visible:ring-brand-navywhile inputs in the same form usefocus-visible:ring-focus-ring. Visually they may match depending on how the CSS variable resolves, but using a different token is semantically inconsistent. Suggest aligning tofocus-visible:ring-focus-ring.⚙️ Tobias Wendt — DevOps / Infrastructure
Verdict: ✅ Approved
What I checked
Findings
No migration needed
titleandpersonTypecolumns already exist on thepersonstable. NoV{n}__*.sqlmigration required. ✅Breaking API change — check before deploying
@NotNullonPersonUpdateDTO.personTypemeans any existing caller that omitspersonTypefrom aPOST /api/personsorPUT /api/persons/{id}body will receive a 400 after this deploys. Key callers to verify:MassImportService— calls the three-argumentcreatePerson(String, String, String)overload, not the DTO variant. ✅ Safe.backend/api_tests/— may needpersonTypeadded to request bodies.FTS fix is low-risk
to_tsquery('simple', ...)relaxes the query builder to avoid German stop-word collisions. TheDocumentFtsTestregression test confirms correctness. No index rebuild needed —simpleandgermanuse the same underlying GIN index.No infra surface changes
Docker Compose, MinIO config, Nginx/reverse proxy config — all untouched. Standard
./mvnw clean package && docker-compose up -ddeployment applies.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Issue #218 checklist
personTypein new-person form/persons/newpersonTypein edit formPersonEditFormtitlein new-person form (PERSON only)isPersontitlein edit form (PERSON only)isPersontitlein PersonCard (PERSON only)+page.server.tsload functiontitle,firstName,alias,birthYear,deathYearall hiddenPersonServicesetspersonTypefrom DTOPersonServicesetstitlefrom DTOObservations
1. FTS fix is out of scope
The
DocumentRepositoryto_tsquery('simple')fix addresses a search correctness bug unrelated to issue #218. It's a good fix, but it's not mentioned in the PR description, which makes it invisible in the issue's change history. Recommend noting it explicitly in the PR description or linking to a separate issue so it's traceable.2. UNKNOWN type behavior
The spec §4 matrix treats UNKNOWN the same as PERSON for the
lastNamelabel ("Nachname"), but the same as non-PERSON for hidingtitle/firstName/alias/birth/death fields. The implementation matches this:lastNameLabeluses "Nachname" for UNKNOWN, andisPersonis false for UNKNOWN (so fields are hidden). ✅3. Backend
titlepersistence on updateThe
PersonService.updatePersonnow setsperson.setTitle(dto.getTitle())— this means sending a null title will clear an existing one. This is the correct behavior for a PUT (full replacement), but worth confirming with the product owner that title erasure is intentional rather than a no-op.Dark mode fix applied ✅
Concern addressed:
PersonTypeSelectorbuttons did not adapt to dark mode — hardcodedbrand-navy/brand-sand/whiteclasses are invisible on dark surfaces.What changed —
d6f91d66fix(persons): use semantic color tokens in PersonTypeSelector for dark modebg-brand-navy text-white border-brand-navybg-primary text-primary-fg border-primarybg-white text-brand-navy border-brand-sandbg-surface text-ink border-linehover:border-brand-navy/50hover:border-primary/50ring-brand-navyring-focus-ringIn dark mode
bg-primaryresolves to mint (#a1dcd8) with navy foreground — the same swap the rest of the UI already uses. All other components introduced in this PR (PersonCard,PersonEditForm,PersonEditSaveBar, new-person form) were already using semantic tokens correctly.Tests added:
PersonTypeSelector.svelte.spec.ts— 3 browser-mode assertions verifyingbg-primary,bg-surface/text-ink/border-line, andring-focus-ringare present in the rendered DOM. All 3 were red before the fix, green after.Review concerns addressed ✅
All open reviewer concerns have been resolved in 3 commits on this branch.
✅ Nora #3 —
titlenot trimmed server-sideCommit:
fe981336—fix(persons): trim title server-side and add SKIP controller testPersonController.createPersonandupdatePersonnow calldto.setTitle(dto.getTitle().trim())when title is non-null, consistent with the existingfirstName/lastNametrim pattern. Covered by a newArgumentCaptor-based test that verifies the trimmed value reaches the service.✅ Sara Missing #2 — No HTTP-level SKIP rejection test
Commit:
fe981336— same commitAdded
PersonControllerTest.createPerson_returns400_whenPersonTypeIsSkip: sendspersonType: "SKIP"viaMockMvc, mocks the service to throwDomainException.badRequest(INVALID_PERSON_TYPE, ...), and asserts the controller returns 400. Confirms full controller →GlobalExceptionHandlererror propagation for SKIP.✅ Felix / Sara Blocker #1 —
normalizePersonTypetests bound to local copy, not production codeCommit:
84986e7e—refactor(persons): export normalizePersonType from edit server moduleExtracted the inline ternary in
+page.server.tsload as a named exported function. Updatedpage.server.test.tstoimport { normalizePersonType } from './+page.server'— the 6 tests now bind directly to production code. If the inline logic ever changes, the tests will catch it.✅ Sara Missing #3 — Server action validation untested in both edit and new forms
Commit:
8295ca83—test(persons): extract validatePersonFields and cover validation branchessrc/lib/person-validation.tsexportsvalidatePersonFields(personType, firstName, lastName): string | nulledit/+page.server.tsandnew/+page.server.tsnow call this shared helper instead of duplicated inlineif-chainssrc/lib/person-validation.test.ts— 8 unit tests covering: valid PERSON,lastNamemissing,lastNameundefined,firstNamemissing for PERSON,firstNameundefined for PERSON, INSTITUTION/GROUP/UNKNOWN without firstName (all valid)Test results after all changes
./mvnw test)npm run test --project=server)./mvnw clean package -DskipTests)Not actioned
findById— minor semantic inversion): no code change requested, noted as advisory onlybackend/api_tests/): noPerson.httpfile exists in the directory; no update required👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1. Keyboard navigation doesn't update form state — silent wrong-value submission
radioGroupNav.tsmanipulatesaria-checkeddirectly on the DOM, butPersonTypeSelector.svelteuses a reactive$state<PersonType>variable calledselectedwhich drives the hidden<input type="hidden" name={name} value={selected} />. When the user navigates with ArrowLeft/ArrowRight, the DOM shows the new button as checked, butselectedis never updated — so the form submits the value that was last set by a click, not the keyboard-navigated value.The fix:
radioGroupNavneeds to dispatch a custom DOM event (e.g.radiogroup-change) thatPersonTypeSelectorlistens to and callsselect(type)with. Or the action needs aonchangecallback parameter.The 7 action tests pass because they only assert DOM state, not reactive state integration. This is a test coverage gap too (Sara will expand on this).
Suggestions
2.
PersonTypeunion type defined in four places —PersonTypeSelector.svelte,PersonEditForm.svelte,new/+page.svelte, andedit/+page.server.tsall independently declaretype PersonType = 'PERSON' | 'INSTITUTION' | 'GROUP' | 'UNKNOWN'. Extract to$lib/types/person.tsor export fromPersonTypeSelector.svelte. When a new type is added, four files need to change.3.
TYPES as constalso duplicated — same array declared inPersonTypeSelector.svelte,PersonEditForm.svelte, andnew/+page.svelte. Import fromPersonTypeSelectoror a shared module.4. Comment in PersonCard.svelte —
<!-- Title (academic/honorific) — PERSON type only -->explains what the code does, not why. The conditional{#if person.personType === 'PERSON' && person.title}already reads like the comment. Per clean code rules: if the code needs a "what" comment, the code should speak for itself; if there's a non-obvious why, explain that instead. Remove it.5.
to_tsquery('simple', ...)bugfix inDocumentRepository— this is a correct fix with a clear test (should_find_document_whose_transcription_contains_word_that_stems_to_german_stop_word) but it is a separate concern from person type + title. It should have been its own atomic commit and possibly its own issue. Bundling it here makes the commit history harder to bisect. Not a blocker, but worth noting.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No security vulnerabilities identified. Full audit summary:
What I checked
Input validation & injection surface
@NotNull PersonType personTypeon the DTO means Jackson deserialization rejects unknown string values with a 400 before business logic runs — no string-to-enum injection possible. ✅@Size(max=50) String titleis enforced by Bean Validation before the service layer. ✅personTypebeing an enum in the DTO means the type system guarantees only valid enum values reachPersonService. ✅SKIP guard placement
PersonService.createPerson/updatePerson), which is the correct defence-in-depth position: even if the controller boundary is bypassed, the service still rejects it. ✅Authorization
createPersonandupdatePersonretain@RequirePermission(Permission.WRITE_ALL). The SKIP guard does not accidentally change who can call these endpoints. ✅Error information exposure
DomainException.badRequest(ErrorCode.INVALID_PERSON_TYPE, ...)returns a structured error code. The frontend maps this toerror_invalid_person_typei18n key viagetErrorMessage()— no implementation detail leaks to the user. ✅Suggestion (not a blocker)
validatePersonNames()inPersonControllerthrowsResponseStatusException(CLAUDE.md notes this is acceptable for simple controller validation), but this bypasses theDomainException→ErrorCodepipeline. The frontend cannot map these messages to localized strings — the message"Nachname ist Pflichtfeld"/"Vorname ist Pflichtfeld"appears raw in HTTP responses. This is not a security issue but a localization leak worth addressing (Elicit's review will cover this from a requirements angle).CWE summary: No findings. CWE-20 (improper input validation) addressed. CWE-285 (improper authorization) unchanged/correct.
🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
No structural blockers
Layer boundaries are correct throughout: controller validates → service persists → repository is never crossed from the wrong domain.
PersonServiceowns its repository;PersonControllerdelegates cleanly.validatePersonNames()in the controller is appropriate controller-boundary validation. ✅Concerns
1.
normalizePersonTypeis in the wrong modulenormalizePersonTypeis exported fromfrontend/src/routes/persons/[id]/edit/+page.server.ts. This function is presentation/domain logic (mapping an import-only enum value to a safe default) and already has a sibling inperson-validation.tswhich is the right home for it. Exporting a utility function from a route server module couples the tests (page.server.test.tsimports from a route) to the route location. When the route moves, the tests break.Move it to
$lib/person-validation.tswhere it belongs alongsidevalidatePersonFields.2.
PersonTypeunion type fragmented across 4 filestype PersonType = 'PERSON' | 'INSTITUTION' | 'GROUP' | 'UNKNOWN'is declared independently in:PersonTypeSelector.sveltePersonEditForm.sveltenew/+page.svelteedit/+page.server.tsThe authoritative set of user-facing types should be one exported constant/type. Adding
FAMILYor renamingGROUPin the future will require hunting through 4 files.3. Unrelated FTS bugfix bundled into this PR
DocumentRepository.javahas theto_tsquery('german', ...)→to_tsquery('simple', ...)change for prefix queries. This is a correct fix (proven by theDocumentFtsTestregression test) but it belongs in its own commit and ideally its own issue/PR. A developer bisecting a person-type regression would have to mentally filter this out. Atomic commits exist precisely to prevent this.What's done well
person_type) with a CHECK constraint enforced at the PostgreSQL level — application code can't write arbitrary strings to it. ✅@Transactionalonly on write methods. ✅🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Test coverage is genuinely good here — multiple layers covered, factory patterns used correctly, test names read as sentences. One blocker: the keyboard navigation bug Felix identified is undetected by tests.
Blockers
1.
PersonTypeSelectorhas no behavioral test for hidden-input value after keyboard navigationThe three
PersonTypeSelector.svelte.spec.tstests check CSS classes on buttons — not that the hidden<input name="personType">value reflects what the user navigated to with the keyboard. TheradioGroupNavaction mutatesaria-checkeddirectly without callingselect(), so the hidden input keeps its old value. A test that:ArrowRighton the active buttoncontainer.querySelector('input[type="hidden"]').value === 'INSTITUTION'...would fail today and prove the bug. Add this test before merging.
Suggestions
2.
PersonTypeSelector.svelte.spec.ts— CSS class tests are fragile assertionsTests like
expect(selected!.className).toContain('bg-primary')are snapshot-class-name tests. They'll fail on any Tailwind class rename or theme token change. What matters is that the selected button is visually distinguishable and the form value is correct. Consider testing:aria-checkedattribute values (behavior)onclick(behavior)tabindexvalues (keyboard accessibility)3.
createPerson_returns400_whenPersonTypeIsSkipinPersonControllerTestThis test configures the mock to throw
INVALID_PERSON_TYPEand then checks the controller returns 400. It's testing mockito plumbing, not actual controller behavior — the SKIP guard lives in the service, so the controller has nothing to do here. The meaningful test isPersonServiceTest.createPerson_dto_throwsInvalidPersonType_whenSkip. The controller test adds noise. Remove or replace with a test that verifies the 400 response shape (errorCode field).4.
person-validation.test.tsasserts on hardcoded German stringsIf the error message text changes, every assertion in this file breaks. Extract the expected strings to a constant or test the return type / key instead. This also reveals the localization bug (Elicit will detail it).
5. No test for type-change behavior during edit form submission
There's no test for the edit action when
personTypechanges fromINSTITUTIONtoPERSON— specifically that firstName validation is now enforced where it wasn't before. A unit test for theupdateaction covering this transition would be valuable.What's done well
personType, which proves existing tests still exercise the right behaviors. ✅createPerson_trimsTitle_beforePersistingwithArgumentCaptor— this is exactly the right way to test controller-level trimming. ✅DocumentFtsTest.should_find_document_whose_transcription_contains_word_that_stems_to_german_stop_word— regression test exists before the production fix. Correct TDD order. ✅page.server.test.tstests all 5 pass-through cases plus null — complete boundary coverage fornormalizePersonType. ✅🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The PersonTypeSelector is well-built from a UI/UX foundation: ARIA roles, keyboard navigation, focus rings, and touch targets all meet or exceed requirements. Two concerns worth addressing.
Concerns
1.
aria-liveregion announces on initial page loadThis region always contains the current type label text. Most screen readers don't announce the initial content of an
aria-liveregion, but some (NVDA + Firefox) do on focus entry to the region. More importantly: this renders"Typ geändert zu Person"immediately on page load, which is confusing — the user hasn't changed anything yet. The announcement should only fire on actual user-initiated change.Fix: use an
aria-atomic="true"region that starts empty and is populated only after the first user interaction:2. Verify
text-ink-3token exists inlayout.cssPersonCard.svelteusestext-ink-3for the title text andPersonTypeSelector.svelteuses it for the type label. If this token is not defined inlayout.css, it will fall back silently tocolor: inheritin Tailwind v4, making the text indistinguishable from the surrounding content in dark mode. Check that--color-ink-3is in the design token set with sufficient contrast againstbg-white(≥4.5:1 for body text).What's done well
min-h-[48px]on all radio buttons — exceeds the 44px touch target minimum for the 60+ audience. ✅focus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:outline-none— correctfocus-visibleusage, invisible on mouse click, visible on keyboard. ✅grid-cols-2 sm:grid-cols-4— 2×2 on small screens, single row on desktop. Correct approach. ✅[font-variant:small-caps]for the title inPersonCard— typographically appropriate for honorifics, consistent with the serif archival tone. ✅role="radiogroup"+role="radio"+aria-checked— correct ARIA pattern for a segmented control. ✅lastNameLabelderived value ("Name"for INSTITUTION/GROUP,"Nachname"for PERSON/UNKNOWN) is a small but meaningful UX detail that matches how these entities are actually named in real use. ✅⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure-related changes in this PR. LGTM from a platform perspective.
What I checked
personTypeandtitlefields onPersonalready have database columns from a prior migration. This PR only adds them to the DTO and persists them via the service layer. ✅to_tsquery('simple', ...)change: This is a query change, not a schema change. No migration needed. The existing FTS indexes cover thesimpledictionary. ✅.envor application profiles. ✅One note for future work: when
PersonTypeenum values are added or removed, the PostgreSQL CHECK constraint on theperson_typecolumn will need a matching Flyway migration (constraint update). This is not an issue in this PR but worth knowing for whenFAMILYor similar types are eventually added.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
The implementation covers the core functional requirements for issue #218. One requirements gap that creates a localization defect.
Blocker
Validation error messages are not localized — German hardcoded in a multilingual app
person-validation.tsreturns hardcoded German strings:These strings are returned from
+page.server.tsactions insidefail(400, { error: validationError })and rendered directly in the UI asform.error/form.updateError. The application supports three locales (de/en/es). An English-locale user who tries to save a PERSON without a first name will see "Vorname ist Pflichtfeld." — German, untranslated.The fix is to return an error key rather than a message, and map it through Paraglide on the frontend, or to call
m.xxx()from within the server action where the locale is available. Sinceperson-validation.tsruns in server context where locale resolution is available via SvelteKit'slocals, the messages could use the Paraglide server-side API.Alternatively, return an error code key (
'FIRST_NAME_REQUIRED','LAST_NAME_REQUIRED') and map it to localized strings in the component like the rest of the error handling does.Requirements gaps from spec §4
UNKNOWN type label: When
selectedType === 'UNKNOWN', thelastNameLabelresolves tom.form_label_last_name()(i.e., "Nachname"). For an unknown/unidentified person, "Nachname" may not be semantically accurate — the user might enter a description like "Unbekannter Mann auf Foto 1923". This is a UX judgment call worth reviewing with the original issue: does the spec define what the lastName field represents for UNKNOWN-type persons?What's correctly covered
titlefield (max 50 chars, PERSON only in UI) — implementation matches spec. ✅8295ca83afto0c47c22185Adds jsonPath("$.code").value("INVALID_PERSON_TYPE") to verify the full error response shape, not just the HTTP status. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review concerns addressed ✅
All open reviewer concerns from the second round have been resolved in 5 commits.
✅ Felix/Sara Blocker #1 — Keyboard navigation doesn't update form state
Commit:
842ab28f—fix(persons): keyboard navigation now updates PersonTypeSelector reactive stateradioGroupNav.tsnow accepts an optionalonChangecallback (+update/destroylifecycle).PersonTypeSelectorpassesselect()as the callback soArrowLeft/ArrowRightnavigation updates bothselectedstate and the hidden<input>value. Addedvalue={type}attribute to each radio button so the callback can read the navigated-to value. New test inPersonTypeSelector.svelte.spec.tsconfirms: render withvalue='PERSON', dispatchArrowRight, assert hidden input value is'INSTITUTION'.✅ Leonie Concern #1 —
aria-liveregion announces on initial page loadCommit:
842ab28f— same commitannouncement = $state('')starts empty. Theselect()function now setsannouncement = m.a11y_type_changed(...)before callingonchange. Thearia-liveregion renders{announcement}so it is silent on page load and announces only on user-initiated type changes. Addedaria-atomic="true".✅ Markus Concern #1 —
normalizePersonTypein wrong module✅ Felix/Markus Concern #2 —
PersonTypeunion type in 4 files✅ Felix Suggestion #3 —
TYPESarray duplicatedCommit:
8e1733ab—refactor(persons): centralise PersonType, PERSON_TYPES and normalizePersonType in person-validation$lib/person-validation.tsnow exportsPERSON_TYPES,PersonType, andnormalizePersonType. All four consumer files (PersonTypeSelector.svelte,PersonEditForm.svelte,new/+page.svelte,edit/+page.server.ts) import from there.page.server.test.tsnow importsnormalizePersonTypefrom$lib/person-validationinstead of the route module. Both server actions now usenormalizePersonType()forpersonTypeextraction instead of an inline type cast.✅ Elicit Blocker — Validation error messages hardcoded German strings
✅ Sara Suggestion #4 — Tests assert on German strings
Commit:
f9a982db—fix(persons): localize validation error messages via Paraglide i18nvalidatePersonFieldsnow returnsPersonValidationKey | null('validation_last_name_required'/'validation_first_name_required').resolveValidationMessage(key)inperson-validation.tstranslates via Paraglide. Both server actions callresolveValidationMessage()before passing tofail(). Addedvalidation_last_name_required/validation_first_name_requiredtode.json,en.json,es.json. Tests now assert on keys, not German strings.✅ Felix Suggestion #4 — What-comment in PersonCard
Commit:
00af9765—refactor(persons): remove what-comment from PersonCard title blockRemoved
<!-- Title (academic/honorific) — PERSON type only -->from line 70.✅ Sara Suggestion #3 — SKIP controller test only checks HTTP status
Commit:
f5eb14a7—test(persons): assert error code in createPerson_returns400_whenPersonTypeIsSkipAdded
andExpect(jsonPath("$.code").value("INVALID_PERSON_TYPE"))— now verifies the fullGlobalExceptionHandler→ structured error response pipeline, not just the HTTP status.Not actioned
text-ink-3token) — verified:--color-ink-3is defined inlayout.csswith 4.8:1 contrast (WCAG AA ✓). No change needed.validatePersonNamesin controller usesResponseStatusException) — CLAUDE.md explicitly allows this for simple controller validation. Frontend form actions catch it before it reaches users in the UI.Test results after all changes
./mvnw test)vitest run --project=server)DocumentList,RichtlinienRuleCard,hilfe/transkription(unrelated to this PR, none in files touched by this branch)👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
The implementation is clean and the key structural concerns from the previous review are fully resolved.
What's right
person-validation.tsas the single source of truth forPersonType,PERSON_TYPES,normalizePersonType, andvalidatePersonFieldseliminates the type fragmentation that was a blocker. Every consumer now imports from one place.radioGroupNavcallback pattern correctly solves the Svelte 5 custom event naming constraint. Returning{ destroy, update }follows the Svelte action contract precisely.validatePersonFieldsreturning a typedPersonValidationKeyinstead of hardcoded German strings is the right separation of concerns — validation logic is decoupled from display.$derived(selectedType === 'PERSON')forisPersonis idiomatic Svelte 5.untrack()to initialize state from props prevents reactive cycles — appropriate use.hiddenInput.value === 'INSTITUTION'after ArrowRight from PERSON) directly tests the bug that existed before — good regression anchor.Suggestions (non-blocking)
new/+page.svelteextracts shared class strings toinputCls/labelClsconstants, butPersonEditForm.sveltestill inlines them. Minor inconsistency worth aligning in a follow-up.bg-primary,text-primary-fg,bg-surface, etc.) are coupling to implementation details — a token rename would silently break them. Prefer asserting onaria-checkedstate or computed behavior over class name string matching.🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Layer boundaries are maintained throughout. No architectural violations.
What's right
PersonControllerdelegates toPersonServicewhich owns the validation logic. ✅person-validation.tsis a pure shared module — no framework imports, no side effects, safe to import from anywhere in the frontend including server-side SvelteKit actions. Paraglide'sm.xxx()calls are stateless; this is fine.normalizePersonTypefrom+page.server.tsto$lib/person-validation.tsbreaks the test's dependency on a specific route file path — the tests now test the function in isolation, not indirectly through a route.new/+page.server.tsandedit/+page.server.tsimport from the same validation module — consistent cross-route behavior guaranteed.PersonEditFormis scoped to the edit context,+page.sveltehandles the new-person context. The component boundary is reasonable.Suggestions (non-blocking)
PersonEditForm.sveltereceives apersonprop with a large inline type annotation (personType,title,firstName,lastName,alias,birthYear,deathYear,notes). As this form grows, an explicit named type or interface would make prop contracts visible at the call site rather than requiring the reader to open the component.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
Test coverage for this PR is solid and addresses the gaps from the previous review round.
What's covered
person-validation.test.ts— 8 test cases covering all branches ofvalidatePersonFields(null, empty string, PERSON/INSTITUTION/GROUP/UNKNOWN type differences) ✅page.server.test.ts— 6 test cases coveringnormalizePersonTypeincluding null default and SKIP→UNKNOWN normalization ✅PersonTypeSelector.svelte.spec.ts— 4 browser tests including the keyboard navigation regression test ✅PersonControllerTest.java— SKIP rejection test now assertsjsonPath("$.code").value("INVALID_PERSON_TYPE")— confirms the error envelope matches what the frontend expects ✅Suggestions (non-blocking)
Test file name misleads:
page.server.test.tsonly testsnormalizePersonTypeimported from$lib/person-validation. The actualload()and formactionsin+page.server.tsare not covered. Consider renaming tonormalizePersonType.test.tsto avoid implying server action coverage that isn't there.Keyboard navigation coverage gap: The
ArrowRightfrom PERSON → INSTITUTION case is tested. AnArrowLeft(boundary check) and wrap-around test would close the remaining keyboard path coverage.Class-name assertions are fragile: Tests that check
btn.className.includes('bg-primary')will silently break on a token rename. Prefer asserting onaria-checked="true"state to verify which button is selected, and let visual regression tests handle styling.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No new security risks introduced. Several security-relevant details are handled correctly.
What's right
personType: "SKIP"is rejected server-side withINVALID_PERSON_TYPE. The backend does not silently accept unknown enum values — this is the correct approach. ✅titleis trimmed before persistence. Prevents whitespace-only values from bypassing@Sizeor display checks. ✅maxlength="50"on title input: Consistent with any backend-side length constraint. Reduces oversized payload in the normal case. ✅@RequirePermission(Permission.WRITE_ALL)still guards create/update endpoints — no permission regression. ✅PersonControllerTestnow asserts the error code in the response body, confirming that the error envelope format is contract-tested. If the error structure changes, the test breaks — good defensive move. ✅Nothing to flag
No injection vectors, no new data exposure surface, no authentication bypass paths. Clean.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Nothing to block.
What's checked
Observation
Translation keys (
form_label_person_type,form_label_name,a11y_type_changed,error_invalid_person_type,validation_last_name_required,validation_first_name_required) are added to all three locale files (de, en, es) in a single commit. No missing-locale runtime error risk. ✅LGTM from a platform perspective.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Checking the implementation against the expected acceptance criteria for this feature.
Acceptance criteria — all fulfilled
Legacy data handling
normalizePersonTypemapsSKIP → UNKNOWNwhen loading existing records. Legacy persons withpersonType = SKIPdisplay as "Unbekannt" rather than breaking the UI or being silently dropped. This is the correct migration path — no data loss, clean user-visible representation.Future consideration (out of scope for this PR)
The UNKNOWN type is always visible in the selector. From a user perspective, deliberately choosing "Unknown" is a valid action (e.g. "I found a letter but don't know the sender type"), but could be confusing to new users. A future UX iteration might consider hiding UNKNOWN unless the current record already has that value, and surfacing it as "can't determine type." Not a blocker — the current implementation is correct and complete for the stated requirements.
🎨 Leonie Voss — UI/UX & Accessibility
Verdict: ⚠️ Approved with concerns
The accessibility architecture of
PersonTypeSelectoris correct and the previous blockers are resolved. One non-blocking concern about screen reader announcement quality.What's right
role="radiogroup"+role="radio"+aria-checked— correct WAI-ARIA radiogroup pattern ✅aria-live="polite"+aria-atomic="true"region for keyboard navigation announcements — correct implementation ✅radioGroupNavnow updates Svelte state via callback — the disconnect bug is fixed ✅bg-primary,text-primary-fg,bg-surface,text-ink,border-line,ring-focus-ring) — consistent with the design system ✅maxlength="50"on the title input — prevents overlong entries without a confusing error message ✅Concern: Screen reader announcement uses raw type value
File:
PersonTypeSelector.svelte— theselect()function setsannouncementusing the raw type string (e.g."INSTITUTION"), so a screen reader announces "Typ geändert zu INSTITUTION" instead of "Typ geändert zu Institution".The localized type labels already exist as
m.person_type_PERSON(),m.person_type_INSTITUTION(), etc. The fix is one line in theselect()function:This is a quality-of-life issue for screen reader users, not a functionality blocker. Suggest addressing before merge or opening a follow-up issue.
Minor
forattribute on the "Typ" label (<p class={labelCls}>) is a<p>tag, not a<label>. It has noforattribute because the radiogroup doesn't have a single focusable target. This is semantically correct — the<div role="radiogroup">should havearia-labelledbypointing to this label paragraph. Consider addingid="persontype-label"to the<p>andaria-labelledby="persontype-label"to the radiogroup<div>for a complete accessible name chain. Non-blocking.Review concerns addressed ✅
All non-blocking suggestions from the third review round resolved in 4 commits.
✅ Leonie minor —
aria-labelmissing on radiogroup divCommit:
c154dcd4—a11y(persons): add aria-label to PersonTypeSelector radiogroupAdded
aria-label={m.form_label_person_type()}directly to<div role="radiogroup">inPersonTypeSelector.svelte. Screen readers now announce the group name without requiring anaria-labelledbyID coordinate between parent and child. Test added:radiogroup has an accessible name via aria-label.Note: Leonie's concern about the announcement using a raw type value (
"INSTITUTION") was already fixed in the previous round —select()useslabels[type]()which resolves to the localized label. No change needed there.✅ Sara/Felix — CSS class assertions replaced with
aria-checkedbehavior tests✅ Sara — ArrowLeft keyboard navigation test added
Commit:
409a8fc8—test(persons): replace fragile CSS class tests with aria-checked behavior testsRemoved 3 tests asserting
className.includes('bg-primary')etc. Replaced with:exactly one button is aria-checked=true for the initial valuearia-checked=true moves to clicked button on clickselected button has tabindex=0, unselected buttons have tabindex=-1Also added:
hidden input value updates when user navigates with ArrowLeft (wraps around)— confirms PERSON wraps to UNKNOWN on ArrowLeft.✅ Sara —
page.server.test.tsrenamed tonormalizePersonType.test.tsCommit:
da1270c4—refactor(persons): rename page.server.test.ts to normalizePersonType.test.ts✅ Felix —
inputCls/labelClsextracted inPersonEditForm.svelte✅ Markus —
PersonFormDatanamed type exported fromperson-validation.tsCommit:
1f750941—refactor(persons): extract inputCls/labelCls and PersonFormData typePersonFormDatais now exported from$lib/person-validation.tsand used as the prop type inPersonEditForm.svelte, replacing the 8-field inline type annotation.inputClsandlabelClsconstants are extracted inPersonEditForm.svelte, consistent withnew/+page.svelte.Test results