feat(stammbaum): family network — graph, badge, edit card, /stammbaum page (#358) #360
Reference in New Issue
Block a user
Delete Branch "feat/stammbaum-issue-358"
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 #358.
Summary
End-to-end implementation of the Stammbaum feature: a family-relationship graph with a typed REST API, a tree page at
/stammbaum, an inferred-relationship badge in the document drawer, and aStammbaum & Beziehungencard on/persons/{id}/edit.Scope
RelationTypeenum,PersonRelationshipentity + repository,RelationshipInferenceService(BFS over the family-graph subset),RelationshipService(write-side rules),RelationshipController(7 endpoints under/api/networkand/api/persons/{id}/...), 3 newErrorCodes,Person.familyMemberflag.RelationshipService, 7@DataJpaTestintegration tests against Postgres-Testcontainers covering the unique constraint, circular-PARENT check, ownership rule, and ON DELETE CASCADE.RelationshipBadge,StammbaumCard(edit page),StammbaumTree+StammbaumSidePanel(/stammbaum),PersonRelationshipsCard(person detail),relationshipLabels.tshelper. Nav swap from/briefwechselto/stammbaum(the/briefwechselroute stays intact). Year-range validation, self-relationship prevention, and direction-aware chip text via the newrelation_child_ofkey.de/en/es(relation types, inferred labels, page strings, form errors).npm run generate:apire-run; existing test fixtures gotfamilyMember: falseto match the now-required field.Constraints honoured
Controller → Service → Repositorylayering:RelationshipServiceorchestratesPersonService; never reaches intoPersonRepository(Markus / Felix).saveAndFlushsounique_rel+CIRCULAR_RELATIONSHIPviolations surface inside the@Transactionalscope.SIBLING_OFenforced by a partial unique index overLEAST/GREATEST(person_id, related_person_id).SIBLING_OFrow required.Verification
cd backend && ./mvnw test— 1406 / 1406 green (includes the 18 inference tests, 8 service tests, 7 integration tests).cd frontend && npm run check— same 251-error baseline asmain; no new errors introduced. The pre-existing fixture errors that gained one extra "missing property" entry (familyMember) are out of scope.cd frontend && npm run test— 383 / 385 pass; the 2 failures are pre-existing time-sensitivedate-buckets.spec.tscases unrelated to this work.npx playwright test e2e/stammbaum.spec.ts— committed; not run locally because Playwright browser deps are unavailable in this environment.Test plan
Als Familienmitgliedon a person → see "Erscheint im Stammbaum" banner appear.PARENT_OFrelationship → it shows as a chip on both sides (Elternteil von/Kind von) and the child appears one row below the parent on/stammbaum.Verwandtschaftrow appears in the metadata drawer./stammbaum?focus={id}deep-link selects the requested node./stammbaumshows the empty state and the link returns to/persons./briefwechselstill resolves (route preserved).Von 1935,Bis 1920shows the inline error and disables submit.Notes for review
frontend/CLAUDE.mdandfrontend/e2e/CLAUDE.mdwere untracked when I started; they got picked up while staging Part L. Happy to split off if preferred.archive-frontendcontainer; in both cases Playwright's bundled chromium binary won't launch without sudo /apt-get. Tests should run in CI.🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid feature delivery. TDD evidence is strong (18 inference tests, 8 service tests, 7 integration tests — all written in proper red→green style). The layering and naming are mostly clean. Two issues need fixing before merge.
Blockers
1.
RelationshipInferenceServicedirectly injectsPersonRepositoryRelationshipInferenceService.javahasprivate final PersonRepository personRepositoryinjected directly. This is a cross-domain repository access that CLAUDE.md and the Architecture persona explicitly prohibit:The
findAllFor()method callspersonRepository.findAllById(ids)to resolve names for BFS results. It should instead callpersonService.findAllById(ids)—PersonServicealready exposes this method. SinceRelationshipService(the orchestrator) already injectsPersonService, the inference service should receive the resolved persons passed in from the orchestrator, orPersonServiceshould be injected here instead ofPersonRepository.Quickest fix: replace
PersonRepositoryinjection withPersonServiceinjection, and callpersonService.findAllById(ids).2. Hardcoded German strings in
StammbaumCard.svelteThese are not i18n'd. The project uses Paraglide throughout. These strings need translation keys (e.g.
relation_year_from,relation_year_to) in all three message files.Suggestions
3.
StammbaumCard.svelteat 365 lines is over the 60-line splitting thresholdThe chip rendering, the add-form logic, the year-range helpers, and the toggle section are distinct visual regions that could become:
RelationshipChip.svelte— the individual chip rowAddRelationshipForm.svelte— the add-form sub-componentNot a blocker but worth splitting post-merge.
4.
RelationshipController.getRelationshipBetweenusesResponseStatusExceptionfor the 404 caseThis should be
DomainException.notFound(ErrorCode.RELATIONSHIP_NOT_FOUND, ...)so the frontend gets a structured error code it can map to i18n. The frontend currently swallows404silently for this endpoint (if (result.response.status === 404) return null) so it's not user-visible, but it's inconsistent with the rest of the codebase.5.
parseType()is duplicated between controller and serviceRelationshipController.validateRelationType()andRelationshipService.parseType()both parseRelationType.valueOf(typeName). One of these can be removed — since the service already validates and throws aDomainException, the controller's early-exit guard is defensive but redundant.6. Unkeyed
{#each}inStammbaumCard— actually fineChecked: the
{#each topDerived as derived (derived.person.id)}block is correctly keyed. ✅🏗️ Markus Keller — Senior Application Architect
Verdict: 🚫 Changes requested
The feature is architecturally well-scoped: a dedicated
relationship/package with its own entity, repository, services, DTOs, and controller. The V54 migration is clean and uses the right constraints. The BFS design is simple and correct.One boundary violation must be fixed before merge. One controller inconsistency should be fixed.
Blockers
1.
RelationshipInferenceServicereaches directly intoPersonRepositoryRelationshipInferenceServiceinjectsPersonRepositoryfrom a different domain:This violates the cross-domain access rule: services in the
relationshipdomain must not inject repositories from thepersondomain. The owning service forPersonRepositoryisPersonService, which already exposesfindAllById().Fix: remove
PersonRepositoryfromRelationshipInferenceService. Either:PersonServicedirectly intoRelationshipInferenceServiceand callpersonService.findAllById(ids), orRelationshipService.getInferredRelationships()and pass the resolved persons down to the inference service.The second option keeps
RelationshipInferenceServicepurely about graph traversal (which is its stated purpose) and gives it no external dependencies. That's the cleaner split.Suggestions
2.
RelationshipController.getRelationshipBetweenthrowsResponseStatusExceptioninstead ofDomainExceptionResponseStatusExceptionbypasses the structured error response and doesn't carry anErrorCode. Since the frontend already handles the 404 silently for this endpoint, it's low-impact — but it's inconsistent with every other endpoint in the codebase. UseDomainException.notFound(ErrorCode.RELATIONSHIP_NOT_FOUND, ...).3.
getFamilyNetwork()loads all family-graph edges then filters in JavaFor a family archive this is almost certainly fine forever. But if the edge list grows (non-family colleagues, friends), this loads unnecessary rows. A future improvement would be a query that joins to
persons.family_member = trueat the DB level. Not a blocker for now.4. Migration V54 is correct
ON DELETE CASCADEon both FK columns: ✅ deleting a person cascades relationship rowsCHECK (person_id <> related_person_id): ✅ self-relation guard at DB levelUNIQUE (person_id, related_person_id, relation_type): ✅ deduplicationSIBLING_OFfor unordered pair: ✅ symmetric constraint handled properly5. New package structure is correct
relationship/is a proper feature package with its own entity, repository, services, DTOs, and controller. Cross-domain calls (RelationshipService→PersonService) go through the published interface. The BFS adjacency-list construction is time-ignorant as stated in the PR description — this is an intentional design decision that makes sense for a historical archive where relationship dates are usually unknown.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
The write-side security is solid. Authorization controls are in the right places. The ownership check in
deleteRelationshipis correctly implemented and has regression tests (the "Nora blocker" comments in the test file are a nice gesture). No injection vulnerabilities found.One gap in test coverage for the authorization layer, and one architectural note about client-side API access in the side panel.
Concerns (non-blocking)
1. No
@WebMvcTesttest forRelationshipController— authorization is untested at the HTTP boundaryThe PR has unit tests for
RelationshipServiceand integration tests for the repository layer, but there is no@WebMvcTest(RelationshipController.class)test class. This means:POST /api/persons/{id}/relationshipsreturns403for a user with onlyREAD_ALLDELETE /api/persons/{id}/relationships/{relId}returns401for unauthenticated requests@RequirePermission(Permission.WRITE_ALL)) on the controller are assumed to work because they work elsewhere — but this specific controller has never been testedPer the security standard: every write endpoint needs a test proving it rejects unauthorized users. This is not a runtime vulnerability (the AOP enforcement is consistent across the codebase), but it's a regression risk if someone later adds
@PermitAllor restructures the controller.Recommended follow-up (acceptable as a separate issue):
2.
StammbaumSidePanelmakesfetch('/api/...')calls client-sideThis exposes the
/api/persons/*/relationshipsroutes to the browser. The routes are protected by session-based auth so the auth cookie is forwarded automatically by the browser — this is fine from an auth perspective for this app's architecture. The risk here is more DX and reliability than security: the client-side fetch bypasses the typed API client and error code mapping.Not a blocker given the app's current architecture, but worth noting for future consistency.
3. Write-side authorization coverage is correct
POST /api/persons/{id}/relationships→@RequirePermission(WRITE_ALL)✅DELETE /api/persons/{id}/relationships/{relId}→@RequirePermission(WRITE_ALL)+ ownership check ✅PATCH /api/persons/{id}/family-member→@RequirePermission(WRITE_ALL)✅GET /api/networkand read endpoints → no explicit permission (authenticated only) — consistent with the rest of the read API ✅4. Circular relationship guard is limited but intentional
The only cycle-prevention check is: "does a reverse PARENT_OF edge already exist?" This prevents direct A→B→A PARENT_OF cycles. Longer cycles (A is parent of B, B is parent of C, C is parent of A) are not prevented. Given this is a historical family archive — not a trust hierarchy — this is an acceptable scope limit. The BFS inference will still terminate because
MAX_DEPTH = 8and visited-node tracking prevents infinite loops. No security issue here.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Test coverage for this feature is substantially better than average for a feature of this scope. The 18 BFS unit tests are a textbook red→green implementation. The
@DataJpaTestintegration tests with Testcontainers hit real PostgreSQL constraints. Factory helpers (person(),parentOf()) are clean and readable.Three gaps I'd like to see addressed.
Concerns
1. No
@WebMvcTestforRelationshipController— HTTP boundary is untestedThere is no controller test class for
RelationshipController. This means:validateRelationType()is not tested at the HTTP layerrelationTypehas no testPATCH /api/persons/{id}/family-memberhappy path has no HTTP-layer testThe existing
RelationshipServiceTestcovers business logic well, but the controller boundary needs its own@WebMvcTestslice. This is in line with the test pyramid that@WebMvcTestis the appropriate slice for controller tests (not@SpringBootTest).Minimum recommended tests for a follow-up issue:
2.
PersonRelationshipsCard.svelte.test.tsandStammbaumTree.svelte.test.tsare present, butStammbaumSidePanelhas no component testStammbaumSidePanel(325 lines) contains the most complex client-side logic in the PR: it fetches data, manages form state, validates year inputs, and handles errors. Yet there is no component test for it. Given the loading/error/empty/populated state patterns, this is exactly where component tests pay off.The fetching is done via plain
fetch()(not the typed API client), so the easiest test approach would be to mockglobalThis.fetchin the test setup.3. E2E test is not verified to run
The
stammbaum.spec.tsfile is committed but the PR notes that Playwright browsers couldn't launch in the dev environment. The spec is present but its correctness is unverified. This is acceptable for merge given the note, but should be tracked as a follow-up to verify in CI once Playwright is set up.What's working well
RelationshipServiceIntegrationTestuses@DataJpaTest+ Testcontainers correctly — unique constraint, circular check, cascade delete, and ownership rule are all covered with real PostgreSQLverb_condition_expectedOutcomepattern consistentlyperson(),parentOf()) are clean — one method per test object, no@BeforeEachbloatDocumentMetadataDrawer.svelte.spec.tscorrectly adds a test for the new relationship pill featurefamilyMember: false) in existing tests are handled correctly without skipping🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
The overall visual direction is coherent with the brand. The side panel layout, card pattern, and chip styles follow the design system. Three accessibility issues must be fixed, one is a blocker.
Blockers
1. SVG year-label text is 10px — below the 12px minimum
In
StammbaumTree.svelte:10px text is unreadable for the 60+ audience and fails WCAG 2.1 practical accessibility. Use 11px minimum; 12px preferred. The name label at
font-size="14"is acceptable.High Priority
2. SVG tree nodes are not keyboard-accessible
The
StammbaumTreerenders<g>groups withonclickhandlers but notabindex,role, oraria-label. Keyboard users cannot navigate the tree at all. Screen readers get a blank SVG.Minimum fix:
3. Zoom buttons are 32×32px — below the 44px touch target minimum
h-8 w-8= 32px. WCAG 2.2 requires 24×24px minimum; our design system targets 44px for the senior audience. Useh-11 w-11(44px) or pad the hit area.Medium Priority
4.
text-[10px]on inferred relationship chips inStammbaumSidePanel10pxuppercase tracking-widest — this is decorative but the text conveys information (the relationship type label). Minimum 11px for any visible text; prefer 12px. Usetext-[11px]ortext-xs(12px).Low Priority / Suggestions
5. The side panel appears over the tree canvas on all screen sizes
The layout
flex flex-rowfor the tree+panel split means on a 375px phone the side panel would take the full width. There's nomd:flex-rowbreakpoint guard. The transcribers are on laptops/tablets so this may be acceptable, but consider:6. Empty state is well designed ✅
The empty-state card in
/stammbaumis clean, uses the right card pattern, and includes a helpful link to/persons. Good work.7. Focus rings are present on interactive elements ✅
The nav link active state, zoom button hover states, and form inputs use the project's focus utilities consistently. No missing focus outlines found.
🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure, CI, or Docker Compose changes in this PR. The only things I care about here are the Flyway migration and any new runtime dependencies.
What I checked
V54 migration is correct
ALTER TABLE persons ADD COLUMN family_member BOOLEAN NOT NULL DEFAULT FALSE— non-breaking, safe to run on an existing dataset ✅ON DELETE CASCADEon both FKs — person deletion cleans up relationship rows automatically; no orphan rows ✅CHECK (person_id <> related_person_id)— self-relation guard at DB level, not application level ✅UNIQUE (person_id, related_person_id, relation_type)constraint — deduplication enforced at DB layer ✅SIBLING_OFsymmetric pairs:LEAST/GREATESTpattern is correct PostgreSQL for unordered pair uniqueness ✅No new infrastructure
No new Docker services, no new external dependencies, no changes to
docker-compose.ymlor CI workflows. Thefrontend/CLAUDE.mdandfrontend/e2e/CLAUDE.mdbeing included is noted in the PR description as accidental staging — doesn't affect deployment.No operational concerns
The BFS runs in-memory over the family graph. For a family archive the graph size is bounded (hundreds of nodes at most). No caching, background jobs, or async processing needed. No new MinIO buckets or S3 objects. The
canWriteflag read frompage.dataon the frontend is server-side — no new client-accessible auth surface.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
The core feature (storing and displaying family relationships) is well-specified and implemented. A few requirements gaps and a potentially surprising UX change for existing users.
Concerns
1. Navigation swap removes
Briefwechselfrom the main nav without user communicationAppNav.sveltereplaces the/briefwechselnav link with/stammbaum. The route itself is preserved, but any user who had bookmarked or habitually clicked "Konversationen / Briefwechsel" will find it gone from the nav without explanation.This is a deliberate product decision (Stammbaum is higher value), but it's worth acknowledging in any changelog or user-facing release note. No code change needed — just noting it as a UX consideration.
2. Relationship badge only shows for exactly one receiver — constraint is invisible to users
The badge that shows the inferred relationship between sender and receiver only appears when there is exactly one receiver (and both must be family members). A document with two family-member receivers gets no badge. Users have no indication why the badge appears on some documents and not others.
Suggestion: either document this constraint in a tooltip on the badge, or consider showing the badge for the first receiver when multiple exist (the code already handles
labelFromBfori === 0).3. Non-family relationship types (FRIEND, COLLEAGUE, etc.) are stored but don't appear in the Stammbaum
Users can add a
FRIENDorCOLLEAGUErelationship between two people, but the/stammbaumpage only showsPARENT_OF,SPOUSE_OF,SIBLING_OFedges. The non-family relationships appear in the person edit card (StammbaumCard) and person detail page (PersonRelationshipsCard), but a user who adds aDOCTORrelationship might wonder why it doesn't affect the Stammbaum tree.This is an intentional design decision per the PR description. It should be communicated to users — either through UI labeling ("Family relationships" vs "Other relationships") or through a tooltip explaining the tree scope.
4. The
relation_child_ofi18n key is present inde.jsonbut the UX implication is worth validatingWhen viewpoint is the subject of a
PARENT_OFrow, the chip readsm.relation_parent_of()("Elternteil von [other]"). When the viewpoint is the object, it readsm.relation_child_of()("Kind von [other]"). This direction-aware label is technically correct but relies onrel.personId === personIdcomparison — which works becausepersonIdis the storage subject. This logic is replicated in bothStammbaumCardandStammbaumSidePanel(chipLabelfunction). Worth extracting to a shared helper to avoid drift.5. What I verified meets the issue requirements
/stammbaum— implementedStammbaum & Beziehungencard on/persons/{id}/edit— implementedPersonRelationshipsCardon person detail view — implementedCHECK), service level, and frontend/stammbaum?focus={id}deep-link — implemented/briefwechselroute preserved — confirmedReview concerns addressed
All 5 open reviewer concerns resolved. Full test suites green: 1409/1409 backend, 1111/1111 frontend.
✅ Cross-domain layering violation —
RelationshipInferenceServiceinjectedPersonRepositorydirectlyRelationshipInferenceServicenow injectsPersonServiceand callspersonService.getAllById(ids)instead of reaching intoPersonRepositorydirectly.Commit:
dcdf270b— fix(stammbaum): resolve persons via PersonService in RelationshipInferenceServiceTest added:
RelationshipInferenceServiceTest#findAllFor_resolves_persons_via_PersonService(test 19 of 19).✅
RelationshipControllererror handling —ResponseStatusExceptioninstead ofDomainExceptiongetRelationshipBetween()now throwsDomainException.notFound(ErrorCode.RELATIONSHIP_NOT_FOUND, ...), producing a structured{"code":"RELATIONSHIP_NOT_FOUND", ...}JSON body.Also removed the redundant
validateRelationType()private method —RelationshipService.parseType()already throwsDomainException.badRequest(VALIDATION_ERROR, ...)for invalid types.Commit:
d6a5cdc4— fix(stammbaum): structured error codes in RelationshipControllerTests added:
RelationshipControllerTest(2 tests — 404 withRELATIONSHIP_NOT_FOUNDcode, and 403 for READ-only user).✅ Hardcoded German strings in
StammbaumCard.yearRange()Added
relation_year_from(ab {year}/from {year}/desde {year}) andrelation_year_to(bis {year}/until {year}/hasta {year}) to all three message files (de/en/es).yearRange()now usesm.relation_year_from/to({year}).Commit:
9a33d1ae— fix(stammbaum): i18n for year-range labels in StammbaumCard✅ WCAG font size and touch targets
StammbaumTree.svelte: SVGfont-size="10"→font-size="12"StammbaumSidePanel.svelte:text-[10px]→text-xs(×2)StammbaumCard.svelte:text-[10px]→text-xs(×2)/stammbaum/+page.sveltezoom buttons:h-8 w-8(32 px) →h-11 w-11(44 px)Commit:
1f62f9ed— fix(stammbaum): WCAG min font-size and 44px touch targets✅
StammbaumCard.sveltetoo large — extract sub-componentsStammbaumCard.sveltereduced from 366 → 196 lines by extracting:RelationshipChip.svelte— single relationship row (chip label + person name + optional year + delete button). 6 browser-mode spec tests.AddRelationshipForm.svelte— self-contained open/close form with alladdType/addRelatedPersonIdstate internal. 4 browser-mode spec tests.StammbaumCardis now a pure orchestrator with no form state of its own.Commit:
707b6e00— refactor(stammbaum): extract RelationshipChip and AddRelationshipForm🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
End-to-end implementation is solid. Architecture decisions are well-reasoned. A few observations worth calling out.
What I checked
Feature packaging — All new classes live under
org.raddatz.familienarchiv.relationshipwith their owndto/sub-package. This follows the feature-package-first convention. The new domain does not bleed into existing packages. ✅Layer boundaries —
RelationshipServicecallsPersonService.getById()/PersonService.findAllFamilyMembers()— never touchesPersonRepositorydirectly. The javadoc on the class makes this explicit: "Always orchestrates PersonService for cross-domain access — never touches PersonRepository." ✅Database-layer integrity — V54 migration gets the critical constraints right:
CHECK (person_id <> related_person_id)— self-relationship prevention at the DB layer, not just in Java.UNIQUE(person_id, related_person_id, relation_type)— atomic uniqueness, no race window.UNIQUE INDEX unique_sibling_pair ON (...LEAST(person_id,related_person_id), GREATEST(...)) WHERE relation_type = 'SIBLING_OF'— clean PostgreSQL-specific solution for unordered pairs.ON DELETE CASCADEon both FK columns — referential integrity without orphan rows.The
saveAndFlushcomment explaining why (synchronous constraint violation inside the@Transactionalscope) is exactly the kind of non-obvious WHY comment that belongs in the codebase. ✅Transport choice — HTTP/REST throughout, no unnecessary complexity introduced. ✅
Suggestions (non-blocking)
Adjacency rebuild per call —
buildAdjacency()inRelationshipInferenceServiceruns one DB query + in-memory graph construction on everyinfer()andfindAllFor()call. For the current family size (likely tens of nodes) this is O(|edges|) and perfectly fine. Worth keeping in mind that/stammbaumpage load + document badge fetch in a document with two family-member persons triggers two independent graph builds from the same underlying data. If the family grows to hundreds of nodes, a request-scoped cache or pre-built adjacency snapshot would be the natural next step — no action needed now.patchFamilyMemberreturnsPersonentity — consistent with the existing project pattern (most controllers return entities directly), but this endpoint returns the fullPersonincludingnotes, which may contain private biographical information. Not a regression from today's pattern, just worth noting if a response-DTO layer is ever introduced.GET
/api/network— no@RequirePermission— the endpoint is protected by Spring Security'sanyRequest().authenticated()so unauthenticated access is blocked. All authenticated users can query the full family graph regardless of their permission group. For a family archive where all authenticated users are family members, this is likely the intended design. A one-line comment in the controller (// intentionally open to all authenticated users) would make this explicit for future reviewers rather than leaving them wondering if the annotation was forgotten.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Strong TDD evidence throughout. The BFS tests are well-constructed and the red-then-green discipline is visible. Two DRY violations I'd call blockers.
Blockers
chipLabel()andotherName()duplicated verbatim — Both functions appear identically inStammbaumCard.svelteandStammbaumSidePanel.svelte. ThechipLabel()function is ~22 lines of switch logic mapping RelationType + viewpoint direction to an i18n string.otherName()is 2 lines. Both belong in a shared location.RelationshipChip.sveltealready exists — the chip component is the natural owner. Either:RelationshipChip.svelteas an internal$derived(passrel: RelationshipDTOandperspectivePersonId: stringas props), orchipLabel(rel, perspectivePersonId)helper fromsrc/lib/utils/relationshipLabel.ts.Option 1 is cleaner — the chip knows its own label. Option 2 if the chip component can't receive those prop types.
Until this is fixed, adding the same RelationType in the future requires three parallel edits to stay in sync.
Suggestions
StammbaumTree.svelteat 531 lines — The layout algorithm justifies some size, but the SVG rendering section (node rectangles, edge lines, generation rows) contains 3–4 nameable visual regions that could be extracted.StammbaumNodeRect.svelteandStammbaumEdgeLine.sveltewould each be under 40 lines of markup and independently testable. Not blocking, but worth a follow-up issue.$effectfetch inStammbaumSidePanel.sveltehas no cancellation — The pattern:If
node.idchanges while a fetch is in flight (user clicks another tree node quickly), both requests are outstanding and the last-to-resolve overwrites the state. The$effectcleanup function can set an abort flag:Not a data-corruption risk at current load speeds, but correct cancellation is the right pattern.
yearRange()inStammbaumCard.svelte— Good use of i18n keys (m.relation_year_from(),m.relation_year_to()). Clean.TDD — The comment in
RelationshipServiceTest.javanaming the Nora blockers being addressed is exactly the right discipline. Tests 1 and 2 prove the security rules hold at the service layer before the controller layer is even tested.Svelte 5 patterns —
$derived.by()for multi-step layout inStammbaumTree.svelte,$state+ event callbacks for selection, keyed{#each}with(rel.id)throughout. All correct.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
No injection vectors, no auth bypasses, no critical vulnerabilities. Two patterns worth documenting and one design question to answer explicitly.
Write-side authorization ✅
All mutating endpoints carry
@RequirePermission(Permission.WRITE_ALL):POST /api/persons/{id}/relationshipsDELETE /api/persons/{id}/relationships/{relId}PATCH /api/persons/{id}/family-memberThe permission enum is type-checked at compile time — no magic string typo risk. ✅
Ownership enforcement ✅
deleteRelationshipcorrectly verifies the path person is one of the two parties in the relationship before deleting:This prevents URL manipulation: a user with
WRITE_ALLcannot deletealice↔bobby callingDELETE /api/persons/charlie/relationships/{aliceBobRelId}. ✅Atomic constraint enforcement ✅
saveAndFlush+ catchDataIntegrityViolationExceptionis the correct pattern. The circular-PARENT_OF check (existsByPersonIdAndRelatedPersonIdAndRelationType) is a race-aware pre-check backed by the uniqueness constraint at the DB layer. No window between check and insert can corrupt data. ✅Concerns
GET endpoints — intention should be documented —
/api/network,/api/persons/{id}/relationships,/api/persons/{id}/inferred-relationships, and/api/persons/{aId}/relationship-to/{bId}carry no@RequirePermission. They are protected by Spring Security'sanyRequest().authenticated()— unauthenticated requests are rejected. However, all authenticated users can read the full family tree regardless of their assigned permissions.This is likely intentional (family archive = all family members can see the family), but the pattern is ambiguous to a future reviewer who might wonder if
@RequirePermissionwas forgotten. I'd add an inline comment or a class-level comment:CWE-284 (Improper Access Control) doesn't apply here — the design is intentional, but it should be explicit.
StammbaumSidePanel.svelte— client-side fetch pattern — The sidebar doesfetch('/api/persons/${id}/relationships')andfetch('/api/persons/${id}/inferred-relationships')directly from the browser. Session cookie is forwarded, so auth is maintained. However, this deviates from the project's documented SvelteKit SSR pattern (data flows from+page.server.ts, API routes are not exposed to the browser).This is consistent with how other interactive panels work in the app (person merge panel, etc.), so it's not a regression. The risk is that these API routes are now part of the client-visible surface. Noting it for awareness rather than as a blocker.
patchFamilyMemberreturns fullPersonentity — The response includes allPersonfields (notes,alias,birthYear,deathYear, etc.). For a toggle endpoint that only changes one flag, returning the full entity is more data than the client needs. Not exploitable in the current threat model, but worth noting if sensitive biographical notes are stored on historical persons.No issues found
@Paramnamed parameters)@CrossOrigin(origins = "*")on authenticated endpoints@ValidonCreateRelationshipRequestin the controller ✅@Size(max = 2000)on notes field matching the DBVARCHAR(2000)✅🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The backend test suite is the strongest part of this PR. The frontend component tests are solid. The controller test slice has a notable gap.
What's strong ✅
Backend test pyramid is well-constructed:
RelationshipInferenceServiceTest— one per LABEL_MAP entry plus the no-path case, each asserting exactRelationTokensequences. These tests verify the BFS algorithm in isolation from the DB.RelationshipServiceTest— cover all domain rule violations: FORBIDDEN ownership, CIRCULAR_RELATIONSHIP, DUPLICATE_RELATIONSHIP, self-relationship, year-range validation, happy-path persistence, and ownership permitted from either side.@DataJpaTestintegration tests inRelationshipServiceIntegrationTest— hit real Postgres via Testcontainers, verifying thatunique_rel,ON DELETE CASCADE,family_memberflag, and the circular parent check work at the database layer.@AutoConfigureTestDatabase(replace = NONE)+ Testcontainers = real PostgreSQL, not H2. This is the correct setup. ✅Factory helpers —
person("Alice")andparentOf(alice, bob, relId)helpers keep test setup readable. ✅Descriptive test names —
deleteRelationship_throws_FORBIDDEN_when_rel_belongs_to_different_person,addRelationship_throws_CIRCULAR_when_reverse_PARENT_OF_exists— each name is a behavior statement. ✅Frontend —
DocumentMetadataDrawer.svelte.spec.tsupdated withinferredRelationshipprop coverage.StammbaumTree.svelte.test.tsandPersonRelationshipsCard.svelte.test.tsare present.Blocker
RelationshipControllerTesthas only 2 tests — for a controller with 7 endpoints this is thin. The current tests cover:getRelationshipBetween→ 404 with correct error codeaddRelationship→ 403 for READ_ALL-only userMissing critical coverage:
The 401 tests in particular prove that the endpoints are not publicly accessible — without them, a future refactor to the Spring Security config could accidentally open these endpoints and no test would catch it.
Suggestions
E2E spec committed but untestable locally — noted in the PR description. The spec structure is there for CI. Once Playwright browser deps are available, the E2E suite should cover the
/stammbaum?focus={id}deep-link and the empty-state path as first priority.@Disablednot used — no tests are skipped without reason. ✅🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Clean migration, correct indexing strategy, no infrastructure changes needed.
Migration (V54) ✅
Column addition —
ALTER TABLE persons ADD COLUMN family_member BOOLEAN NOT NULL DEFAULT FALSE— safe on a live table: existing rows getfalse, new column is non-null without requiring a backfill. ✅Table design —
UUID PRIMARY KEY DEFAULT gen_random_uuid()(no sequence, no integer PK — correct for distributed consistency),TIMESTAMPTZ(notTIMESTAMP— timezone-aware),VARCHAR(2000)for notes matching application-level@Size(max=2000). ✅Constraints —
CHECK (person_id <> related_person_id)andUNIQUE(person_id, related_person_id, relation_type)at the DB layer. These fire atomically — no application-layer race can circumvent them. ✅Indexes — Both FK columns are indexed:
idx_person_rel_person_id ON person_relationships(person_id)idx_person_rel_related_person_id ON person_relationships(related_person_id)The JPQL queries in
PersonRelationshipRepositoryfilter byperson_id OR related_person_id. Both indexes will be used. ✅Partial unique index for symmetric SIBLING_OF —
UNIQUE INDEX unique_sibling_pair ... LEAST(person_id, related_person_id), GREATEST(person_id, related_person_id) WHERE relation_type = 'SIBLING_OF'— this is the correct PostgreSQL approach for unordered pair uniqueness without a trigger. Clean. ✅ON DELETE CASCADE— both FK columns cascade on person delete. This prevents orphan relationship rows when a Person is deleted. The integration tests verify this actually fires against Postgres (not H2). ✅Infrastructure impact
No changes to
docker-compose.yml, Caddy config, CI pipeline, or environment variables. The migration runs automatically via Flyway on next startup. ✅Minor notes
VARCHAR(30)forrelation_typeis sufficient — longest enum value (COLLEAGUE) is 9 chars. If the enum grows, a future migration can widen it with zero downtime on PostgreSQL.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
All stated acceptance criteria from the issue are met. I walked through the test plan line by line against the implementation.
Test plan coverage
Als Familienmitglied→ see "Erscheint im Stammbaum" bannerStammbaumCard.sveltetoggle withuse:enhance, conditional banner below headingElternteil von/Kind von)chipLabel()usesviewpointIsSubjectflag; both edit page and detail page render the perspective-correct labelVerwandtschaftrow when sender + single receiver are family members with a pathDocumentMetadataDrawer.sveltereceivesinferredRelationshipprop;RelationshipPillrendered inline/stammbaum?focus={id}selects requested node$effectreadspage.url.searchParams.get('focus')and pre-setsselectedId/stammbaumshows empty state + link to /persons{#if data.nodes.length === 0}branch with SVG icon, heading, body, and<a href="/persons">/briefwechselstill resolvessrc/routes/briefwechsel/is not modified; only the nav link target changedVon 1935, Bis 1920shows error and disables submityearError$derivedinStammbaumSidePanel.svelteandAddRelationshipForm.svelte;submitDisabledgates the buttonObservations
Feature scope was respected — The PR description's "Scope" section matches the issue. No out-of-scope features were introduced silently.
Non-family relation types (FRIEND, COLLEAGUE, EMPLOYER, DOCTOR, NEIGHBOR, OTHER) are stored and rendered in relationship chips but are deliberately excluded from the family-graph BFS (inference only walks PARENT_OF, SPOUSE_OF, SIBLING_OF). This aligns with the "family-graph subset" decision noted in the PR.
/briefwechselnav-swap caveat — The nav link now points to/stammbaum, which means the nav label says "Stammbaum" where it previously said "Briefwechsel". If any users have the/briefwechselURL bookmarked, they can still reach it directly — it's just no longer surfaced in nav. This is a deliberate product decision, worth confirming is intentional.Open question (non-blocking)
The document badge shows the relationship only when the sender and single receiver are both family members and a path exists. If there are multiple receivers and the sender is a family member, no badge is shown. Is this the intended behavior, or should the badge show for the first receiver who has a path? Worth tracking as a follow-up issue if the latter is desired.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The visual design follows the brand system. One WCAG violation that needs fixing before merge. One medium accessibility gap for the tree itself.
Blocker
Zoom buttons have no focus-visible ring —
StammbaumCard.svelte(and the/stammbaumpage header) zoom buttons:There is no
focus-visible:ring-*class. Keyboard users navigating with Tab cannot see where they are. This violates WCAG 2.4.7 (Focus Visible, Level AA).Fix:
This is the pattern used elsewhere in the app (e.g. nav links use
focus-visible:ring-2 focus-visible:ring-focus-ring). ✅ Apply the same here.Medium concern
SVG tree nodes are not keyboard-reachable — The family tree is an SVG canvas where
<rect>and<text>elements handleonclickfor node selection. SVG elements have no implicit tab-stop role. A keyboard user (or screen reader user) cannot navigate to individual persons in the tree.For the 60+ transcriber audience who may use keyboard navigation: this is a usability gap. Suggested approach for a follow-up issue:
<button>elements at each node position (absolute-positioned via CSS), ortabindex="0"+role="button"+onkeydownon each SVG group element.Not blocking merge (the feature is new and the gap is tracked here), but should be filed as a separate accessibility issue.
What's good ✅
Touch targets — Zoom buttons are
h-11 w-11(44×44px) ✅. The toggle button inStammbaumCard.sveltehasmin-h-[44px] min-w-[44px]✅. WCAG 2.2 requirement met.Section heading pattern —
text-xs font-bold tracking-widest text-ink-3 uppercaseinStammbaumCard.sveltematches the CLAUDE.md card pattern. ✅ARIA on interactive elements — Zoom buttons have
aria-labelvia i18n keys ✅. Toggle usesrole="switch"+aria-checked={familyMember}✅. Empty-state SVG hasaria-hidden="true"✅. Delete buttons in chips havearia-label✅.Responsive fix in drawer —
min-w-0 truncateadded to person name inDocumentMetadataDrawer.svelteprevents overflow on narrow viewports. ✅Empty state — Follows the standard card pattern (
rounded-sm border border-line bg-surface p-10 shadow-sm). The SVG illustration is decorative (aria-hidden). Heading + body + link hierarchy is correct. ✅Token usage —
text-ink-2,text-ink-3appear in other existing components in the codebase, so they're established tokens.bg-surface,border-line,text-inkare all used consistently. ✅Round 2 concerns addressed
All 5 open reviewer concerns resolved. Backend: 1413/1413 green. Frontend: 889/889+ pass (3 pre-existing HelpPopover browser-mode flakes unrelated to this branch).
✅
chipLabel()andotherName()duplicated in StammbaumCard + StammbaumSidePanel — @felix blockerBoth functions (~22-line switch + 2-liner) were copied verbatim between the two components. Extracted to the shared
$lib/relationshipLabels.tsmodule as exportedchipLabel(rel, perspectivePersonId)andotherName(rel, perspectivePersonId). Both components now import from the shared helper.8 unit tests added to
src/lib/relationshipLabels.test.ts(red→green), covering direction-sensitive PARENT_OF, symmetric SPOUSE_OF, and the name-flip logic.Commits:
ec938a7a— refactor(stammbaum): extract chipLabel/otherName to shared relationshipLabels helperb24dc75c— refactor(stammbaum): use shared chipLabel/otherName from relationshipLabels in both components✅
RelationshipControllerTesthad only 2 tests — missing 401/403 coverage — @sara blockerAdded 4 tests to
RelationshipControllerTest:getRelationships_returns401_whenUnauthenticated— provesGET /api/persons/{id}/relationshipsrejects unauthenticated requestsgetNetwork_returns401_whenUnauthenticated— provesGET /api/networkrejects unauthenticated requestsdeleteRelationship_returns403_for_READ_ALL_only_user— provesDELETE .../relationships/{relId}enforcesWRITE_ALLpatchFamilyMember_returns403_for_READ_ALL_only_user— provesPATCH .../family-memberenforcesWRITE_ALLController test count: 2 → 6.
Commits:
7cbf5176— test(stammbaum): prove GET endpoints reject unauthenticated requests (401)eec1b9d1— test(stammbaum): prove DELETE and PATCH /family-member return 403 for READ_ALL-only users✅ Zoom buttons missing
focus-visiblering — WCAG 2.4.7 — @leonie blockerBoth zoom buttons in
/stammbaum/+page.sveltenow carryfocus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:outline-none, matching the pattern used on nav links throughout the app.Commit:
9996017e— fix(stammbaum): add focus-visible ring to zoom buttons — WCAG 2.4.7✅ GET endpoints missing auth-intent comment — @markus/@nora suggestion
Added a two-line comment above the read endpoint group in
RelationshipController.javaexplaining that the missing@RequirePermissionis intentional: all authenticated family members can read the family graph; unauthenticated access remains blocked by Spring Security'sanyRequest().authenticated().Commit:
9b750a06— docs(stammbaum): document intentional auth design on RelationshipController GET endpoints👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1. DRY violation:
PersonRelationshipsCard.sveltere-implementschipLabel()andotherName()locallyPersonRelationshipsCard.svelte(lines ~21–57) defines its ownchipLabel(rel)andotherName(rel)functions with identical logic to what is already exported from$lib/relationshipLabels.tsand used inStammbaumCard.svelte. Two copies of the same switch statement will diverge. The fix is one line:Then delete the duplicate local functions.
Suggestions
2.
StammbaumTree.svelteis 531 lines — the layout algorithm inbuildLayout()alone is ~200 lines. This is still a single visual region (the SVG canvas), so it doesn't strictly violate the "one nameable region per file" rule, but the algorithm is pure TypeScript and has no Svelte dependency. Extracting it tostammbaumLayout.tswould make it independently testable without a browser context and reduce the component file to its template responsibilities.3. Hardcoded German heading in
StammbaumCard.svelte— the<h2>readsStammbaum & Beziehungendirectly in the template instead of a Paraglide message key. Every other heading in this PR usesm.xxx(). This will break theen/eslocales.4.
$effectforselectedIdinitialisation instammbaum/+page.svelte— the$effectthat copiesfocusIdintoselectedIdis correct, but$effectruns after render (the node is briefly unselected on first paint). An immediate initialiser avoids the flicker:The
$effectcan then be deleted. If future URL-driven selection changes are needed it can be re-introduced. (ThefocusIddeep-link is currently a one-time load behaviour, not a live reactive URL watcher.)5.
{#each}keys — all{#each}blocks in the new components use key expressions. ✅6.
@Transactionalplacement — write methods inRelationshipServiceare annotated, read methods are not. ✅7. Guard clauses and
DomainExceptionfactories — consistently used throughoutRelationshipService. ✅8.
saveAndFlush— catches theunique_relconstraint inside the@Transactionalboundary as intended. ✅🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Layer boundaries
RelationshipService→PersonService.getById()/findAllFamilyMembers()/getAllById()— never touchesPersonRepositorydirectly. ✅RelationshipInferenceService→PersonService.getAllById()for name resolution after BFS. ✅relationship/package holds entity, repository, service, inference service, controller, and all its DTOs — true feature packaging. ✅Schema
The V54 migration is solid:
no_self_relCHECK constraint at DB level (not just application). ✅unique_relUNIQUE constraint prevents duplicates at commit time;saveAndFlushsurfaces the violation synchronously inside the@Transactionalboundary. ✅LEAST/GREATEST(person_id, related_person_id)for SIBLING_OF correctly handles symmetric storage. ✅ON DELETE CASCADEon both FKs keeps the table clean when a person is deleted. ✅Suggestions (non-blocking)
1.
buildAdjacency()is called once per service call — no cachingRelationshipInferenceService.buildAdjacency()issues afindAllByRelationTypeIn(PARENT_OF, SPOUSE_OF, SIBLING_OF)query on every call.getInferredRelationships(id)→findAllFor(id)→buildAdjacency()is one full table scan. For a family archive with ~50–200 members this is fine and keeping it simple is the right call. If the graph grows beyond a few hundred nodes a request-scoped cache would be the natural next step, but that's a future concern.2.
getFamilyNetwork()filters edges in memoryThe method loads all family-type edges, then filters to those where both endpoints are in
familyIds. A JOIN query would avoid loading edges for persons who are not family members, but again — for family archive scale, this is correct and not worth optimising prematurely.3. Controller split across two
@RequestMappingrootsRelationshipControllermaps both/api/networkand/api/persons/{id}/...without a class-level@RequestMapping. This is unconventional but intentional (documented in the Javadoc). It avoids touchingPersonControlleras noted in the PR. Acceptable for the feature's scope.4.
RelationshipInferenceService.MAX_DEPTH = 8This is an undocumented product decision. Why 8 and not 6 or 10? A brief comment on the constant would prevent the next developer from tuning it blindly. Suggestion:
🔒 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
What I checked
Authentication on read endpoints (no
@RequirePermission)The controller comment is explicit: "Unauthenticated requests are rejected by Spring Security's
anyRequest().authenticated()rule." The test suite confirms this:RelationshipControllerTesthas@Testmethods proving GET/api/networkand GET/api/persons/{id}/relationshipsreturn 401 for anonymous requests. The design is intentional and correctly documented. ✅IDOR on
deleteRelationshipThe service checks that
personId(from the URL path, validated as a UUID by Spring MVC) is either thepersonorrelatedPersonof the relationship row before deleting. A user cannot delete someone else's relationship by guessing arelId. ✅Self-relationship prevention
Both at the DB layer (
no_self_rel CHECK (person_id <> related_person_id)) and in the service (if (personId.equals(dto.relatedPersonId())) throw ...). Defense in depth. ✅Enum parsing
parseType(String typeName)catchesIllegalArgumentExceptionfromRelationType.valueOf()and re-throws a structuredDomainException.badRequest. No injection vector. ✅Frontend error handling
All form action error paths use
getErrorMessage(code)— no raw backend messages reach the UI. ✅JPQL / Spring Data
All queries go through Spring Data JPA repository methods (
findAllByPersonIdOrRelatedPersonId,existsByPersonIdAndRelatedPersonIdAndRelationType, etc.). No string concatenation in queries. ✅Suggestions (non-blocking)
1. UUID enumeration via
relationship-toendpointGET /api/persons/{aId}/relationship-to/{bId}returns 404 when no path is found. An authenticated attacker can probe with known UUIDs: a 200 confirms both persons exist and are connected; a 404 is ambiguous (no path, or person doesn't exist). Since UUIDs are not guessable and all authenticated users can already query/api/persons/{id}directly, this is a smell, not a vulnerability. Worth noting for the threat model.2.
patchFamilyMemberexposes aPersonentity in the responseThe PATCH endpoint returns the full
Personentity (including all fields). For this single-tenant family archive this is fine, but if multi-tenancy is ever added the response should be narrowed to a dedicated DTO.3. Year range validation is on the server only when
notesfield is absentThe server-side
validateYears()is called unconditionally inaddRelationship(). The client-sideyearErrorderived state inAddRelationshipForm.sveltealso prevents submission. Both layers are in place. ✅Overall: the security posture of this feature is sound. The read/write split is clear and enforced at the framework level.
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
What's there (and looks good)
RelationshipInferenceServiceTestRelationshipServiceTestRelationshipServiceIntegrationTestRelationshipControllerTestRelationshipChip.spec.ts(6),PersonRelationshipsCard.test.ts(2),StammbaumTree.svelte.test.tsThe inference tests are thorough — they test at the path level (exact token sequences), not just label strings. This is the right granularity. ✅
Blockers
1. E2E test (
stammbaum.spec.ts) is committed but not verified to run in CIThe PR description states: "Tests should run in CI." But there is no evidence of a Gitea Actions workflow that installs Playwright browser binaries. The test file is committed, but if CI never executes it, it provides no safety net. Before merge, one of the following must be true:
npx playwright install chromium && npx playwright test e2e/stammbaum.spec.tsexists, OR@skipor quarantine label with a linked Gitea issue and deadline (Sara's rule:@Disabledonly with ticket + deadline).Suggestions
2.
deleteRelationshipownership check has no unit testRelationshipService.deleteRelationship()throwsDomainException.forbiddenwhen the caller'spersonIdis not a participant. The integration tests cover the CASCADE behaviour, but I don't see a unit test asserting the 403-path for ownership. Suggest adding:3.
getFamilyNetwork()has no unit testOnly covered implicitly through the load function test in the frontend. Worth a unit test asserting that edges where one endpoint is not in
familyIdsare excluded.4. The two pre-existing
date-buckets.spec.tsfailuresThe PR description calls them "pre-existing time-sensitive cases unrelated to this work." Fine — but they should have a Gitea issue with a deadline. Time-sensitive tests that fail intermittently erode trust in the suite over time.
5.
PersonRelationshipsCardtests cover deduplication but not thechipLabeldirection logicThe card shows
Elternteil vonorKind vondepending onrel.personId === personId. This direction-sensitive branch is not tested. One test for each direction would be a quick add.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
This PR adds no changes to
docker-compose.yml, CI workflow files,Dockerfile, or infrastructure configuration. My review is limited to implications for the existing environment.Observations
New migration V54 —
ALTER TABLE persons ADD COLUMN family_member BOOLEAN NOT NULL DEFAULT FALSEwith a column default means this migration applies safely to an existing populated table: no backfill required, no lock escalation risk on small PostgreSQL tables. ✅No new Docker services, volumes, or port exposure — clean from a platform perspective. ✅
Frontend CLAUDE.md files committed —
frontend/CLAUDE.mdandfrontend/e2e/CLAUDE.mdwere untracked and got staged. These are documentation files that belong in the repo; no issue from a DevOps angle.One concern (not a blocker for merge)
Playwright E2E in CI needs attention
The PR notes that
npx playwright test e2e/stammbaum.spec.tscould not be run locally due to missing Chromium deps. If the Gitea Actions runner doesn't haveplaywright install chromiumin its setup, the committed spec will never execute in CI. The fix is a one-liner in the workflow:This is not a merge blocker if the E2E suite isn't gated in CI yet, but it should be tracked so the spec doesn't rot silently.
🎨 Leonie Voss — UX Design & Accessibility
Verdict: ⚠️ Approved with concerns
Critical (WCAG blocker)
1. Delete button touch target: 32px — fails WCAG 2.2 SC 2.5.8
RelationshipChip.svelteline ~38:h-8 w-8= 32 × 32 px. WCAG 2.2 requires a minimum 24×24 CSS target and recommends 44×44 for the senior audience (60+). This directly affects the core authoring workflow. Fix:High
2. Hardcoded German string in
StammbaumCard.svelteThis heading will stay German even when the UI language is switched to English or Spanish. Replace with a Paraglide message key.
3. Derived relationship names in
StammbaumCard.svelteare not linksThe derived relationships section uses
<span>for the person name:PersonRelationshipsCard.sveltecorrectly wraps these in<a href="/persons/{derived.person.id}">. Users on the edit page should be able to navigate to a related person as easily as from the detail page. This inconsistency will confuse users.Medium
4. SVG node focus ring may not render correctly
The node
<g>elements haveclass="cursor-pointer focus:outline-none focus-visible:ring-2 focus-visible:ring-primary". SVG<g>elements don't reliably support CSSoutlineorbox-shadowrings across all browsers — the ring may be invisible. Keyboard users navigating the tree with Tab/Enter need a clearly visible focus indicator. Consider adding a<rect>as a focus highlight that is conditionally shown:5. SVG node font size: 14px
14px is at or below the readable floor for the senior audience (60+). 16px is preferred. At the current
NODE_W = 160px, there is room for 16px text.Low / Positive notes
h-11 w-11(44px) ✅role="switch"+aria-checked✅aria-label="{m.btn_delete()} — {otherName}"✅role="img"+aria-label="Stammbaum"✅aria-hidden="true"✅role="alert"✅var(--c-primary),bg-surface, etc.) is consistent throughout ✅📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Requirements coverage
The PR closes #358 and the implementation aligns closely with the feature scope described in the PR body. Checking the stated test plan items:
familyMemberprop drives the in-tree banner inStammbaumCard.sveltechipLabel()inrelationshipLabels.tsloadInferredRelationship()in/documents/[id]/+page.server.ts/stammbaum?focus={id}deep-linkfocusIdderived frompage.url.searchParamsinstammbaum/+page.svelte/persons/briefwechselstill resolvesVon 1935, Bis 1920→ erroryearError+ server-sidevalidateYears()Observations
1. The relationship badge on document detail requires exactly 1 receiver who is a family member
The comment in
loadInferredRelationship()is clear:This means a document with a sender and 2 family-member receivers shows no badge. This is a conscious scope decision and reasonable for a first implementation. It should be documented as a known limitation in issue #358 so it doesn't generate a future "bug" report.
2.
MAX_DEPTH = 8is an undocumented product decisionThe constant controls how many hops the BFS traverses before classifying a path as
"distant". 8 hops covers great-great-grandparents and second cousins. This was presumably chosen intentionally for the 1899–1950 archive, but there is no requirement in #358 that specifies it. A comment on the constant (suggested also by Markus) preserves the rationale.3. The
LABEL_DISTANTfallback label maps to what i18n key?RelationshipInferenceService.LABEL_DISTANT = "distant"is passed as thelabelfield inInferredRelationshipWithPersonDTO. The frontendinferredRelationshipLabel("distant")inrelationshipLabels.tsshould map this to a translated string. I didn't see the completerelationshipLabels.tsfile but assuming"distant"has a corresponding key inde.json/en.json/es.json— worth confirming.4. "Colleague", "Employer", "Doctor", "Neighbor" relation types are stored but excluded from the BFS inference graph
RelationshipInferenceService.buildAdjacency()only processesPARENT_OF,SPOUSE_OF,SIBLING_OF. The other types (FRIEND, COLLEAGUE, etc.) are stored in the table and shown in the chip list, but inferred relationship paths do not traverse them. This is the correct product decision for a family archive, but it's implicit. If a user adds aFRIENDrelationship and wonders why it doesn't appear in the inferred list for third parties, this will seem like a bug. Consider surfacing this distinction in the UI (e.g., labelling the inferred section "Familiäre Verbindungen" rather than just "Abgeleitete Beziehungen").Round 3 concerns addressed
All 10 open reviewer concerns resolved. Backend: 1414/1414 green. Frontend: 24/24 stammbaum-related tests green (2 pre-existing unrelated flakes remain in the wider suite).
✅
PersonRelationshipsCardre-implementedchipLabel()/otherName()locally — @felix blocker #1Deleted the local 22-line switch duplicate and
otherName()inPersonRelationshipsCard.svelte. Now importschipLabel(rel, personId)andotherName(rel, personId)from$lib/relationshipLabels.ts— the same shared helper already used byStammbaumCardandStammbaumSidePanel.Two direction-sensitive regression tests added to
PersonRelationshipsCard.svelte.test.tsprovingElternteil von/Kind vonrender correctly from each viewpoint.Commits:
e0e23750— fix(stammbaum): import chipLabel/otherName from shared relationshipLabels in PersonRelationshipsCard✅ Hardcoded
Stammbaum & Beziehungenheading — @felix blocker #3 / @leonie high #2Added
stammbaum_relationships_headingkey to all three message files (de/en/es).StammbaumCard.svelteheading now uses{m.stammbaum_relationships_heading()}.Commit:
ada98e80— fix(stammbaum): i18n the StammbaumCard heading (de/en/es)✅ Delete button 32px — WCAG 2.2 SC 2.5.8 — @leonie WCAG blocker
RelationshipChip.sveltedelete button:h-8 w-8(32px) →h-11 w-11(44px). Class-presence test added to prevent regression.Commit:
4e73486b— fix(stammbaum): WCAG 2.2 SC 2.5.8 — delete button 32px → 44px in RelationshipChip✅ Derived relationship
<span>→<a href>inStammbaumCard— @leonie high #3The derived-relationships section of
StammbaumCard.svelteused a<span>for the person name wherePersonRelationshipsCardcorrectly used an<a href>. Both are now consistent:<a href="/persons/{derived.person.id}" class="... hover:underline">.Commit:
8f9d08b1— fix(stammbaum): derived relationship names link to person page in StammbaumCard✅ E2E spec committed but unverifiable in CI — @sara blocker
Created issue #363 (
devops: add Playwright E2E job to CI for stammbaum spec) with labelsdevops,test,P2-medium. All four tests instammbaum.spec.tsare now skipped with a comment referencing #363. Remove thetest.skip()once the CI job is wired up.Commit:
97660b4c— test(stammbaum): skip E2E spec until CI Playwright job exists (#363)✅
MAX_DEPTH = 8undocumented — @markus / @elicit suggestionAdded a three-line comment explaining that 8 hops covers great-grandparents ↔ great-great-grandchildren and second cousins — the practical horizon for the 1899–1950 archive.
Commit:
4b151681— docs(stammbaum): explain MAX_DEPTH=8 rationale on RelationshipInferenceService✅
$effectselectedId initialiser — @felix suggestionRemoved
$derived(page.url.searchParams.get('focus'))+$effectpattern.selectedIdis now initialised directly from the URL parameter in$state(), eliminating the deferred write that left the focused node unselected on first paint.Commit:
3510be26— refactor(stammbaum): initialise selectedId directly from focusId, drop $effect✅ SVG node focus ring unreliable + font size too small — @leonie medium #4 / #5
Two changes in
StammbaumTree.svelte:focus-visible:ring-*(box-shadow) replaced with a conditional<rect x="-3" y="-3" stroke="var(--c-focus-ring)" fill="none" />drawn inside each node<g>, which renders correctly in all browsers.Commit:
656c93ca— fix(stammbaum): SVG node font 14→16px and reliable keyboard focus ring✅
getFamilyNetwork()has no unit test — @sara suggestionAdded
getFamilyNetwork_excludes_edges_where_one_endpoint_is_not_a_family_membertoRelationshipServiceTest. Proves that an edge between a family member and a non-family member is filtered out before it reaches the DTO.Commit:
32622b9b— test(stammbaum): getFamilyNetwork excludes edges with non-family endpoints🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
This is architecturally sound work. The new
relationship/package is properly feature-first, service boundaries are clean, and database integrity is enforced at the right layer. I have two structural concerns worth addressing before this ships.Blockers
1.
SPOUSE_OFhas no symmetric uniqueness constraintV54__add_family_network.sqladds a partial unique index for symmetricSIBLING_OFpairs:But
SPOUSE_OFhas no equivalent. Theunique_relconstraint (UNIQUE (person_id, related_person_id, relation_type)) only blocks exact duplicates. Nothing prevents a user from inserting both(A, B, SPOUSE_OF)and(B, A, SPOUSE_OF)as two separate rows.buildAdjacency()adds both directions forSPOUSE_OFinto the adjacency graph anyway, so the BFS would find duplicate SPOUSE edges and theRelationshipDTOlist on a person's page would render the same marriage twice from opposite angles. The fix is straightforward:Suggestions
2.
buildAdjacency()loads the entire family-graph edge set on every inference callRelationshipInferenceService.buildAdjacency()callsfindAllByRelationTypeIn(PARENT_OF, SPOUSE_OF, SIBLING_OF)— this fetches every family relationship row on every call toinfer(),findAllFor(), andgetFamilyNetwork(). For a 1899–1950 family archive with perhaps 40–100 family members this is fine, but the pattern will degrade silently as data grows. Not a blocker, but worth aTODO: cache adjacency graph on first build, invalidate on writecomment in the service so a future maintainer doesn't have to rediscover the hot path.3.
patchFamilyMemberreturns the fullPersonentity — not a blocker but a consistency smellRelationshipController.patchFamilyMemberreturnsPerson(the JPA entity) while every other new endpoint in this PR returns purpose-built record DTOs. This is consistent with the existing pattern inPersonController, so not worth fixing now — but worth a note in the backlog to align when PersonController gets its DTO pass.What's done well
relationship/is exactly right. The package is self-contained, independently testable, and could be extracted later if needed.RelationshipServiceorchestratesPersonServicefor all person lookups — never reaches intoPersonRepositorydirectly. Clean domain boundary.saveAndFlushinaddRelationship()ensuresDataIntegrityViolationExceptionis caught inside the@Transactionalboundary rather than at commit time. The comment explaining why is exactly the kind of comment that should exist.LEAST/GREATESTis the correct PostgreSQL idiom. Would appreciate the same for SPOUSE_OF.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The TDD evidence is solid — 33+ tests across BFS unit, service unit, and integration layers. The inference service is clean and readable. One blocker that breaks the EN/ES translations.
Blockers
1. Hardcoded German strings in
AddRelationshipForm.svelteThe form contains several user-visible strings that bypass Paraglide and will always display in German regardless of the user's language setting:
The
messages/*.jsonfiles have keys for all the relation types and year fields — but the grouping labels and field names aren't i18n'd. A user with their language set to English sees"Familie","Typ","Von Jahr"throughout. These need Paraglide keys added and wired up. I'd add:Suggestions
2.
RelationshipInferenceService.buildAdjacency()is 50+ lines doing three thingsThe method builds the adjacency map, derives the sibling edges from shared parents, and handles the switch on relation type — all in one pass. The logic is correct, but splitting it into
buildEdgesFromRepository()(the switch) andderiveSiblingEdges(parentToChildren, adj)would make each step independently readable and testable without any functional change.3.
RelationshipChip/RelationshipPill— two components with similar namesBoth
RelationshipChip.svelteandRelationshipPill.svelteexist. From the diff,RelationshipChipis used in the side panel (shows the stored type with a delete button) andRelationshipPillis the inline badge in the metadata drawer (shows an inferred label). The names are close enough that a new contributor would have to open both files to understand which to use. ConsiderStoredRelationshipChip/InferredRelationshipBadgeor at minimum JSDoc on each file explaining when to use it.4.
{#each displayedReceivers as receiver, i (receiver.id)}— badge only on first receiverDocumentMetadataDrawer.svelterenders the inferred relationship pill only oni === 0. If a document has multiple receivers and the inferred relationship is with receiver index 1+, the badge appears on the wrong person. The current behaviour is documented in the PR description ("single receiver" scenario), but there's no guard in the component — it silently applies the badge to the wrong person when there are multiple receivers. Worth adding a comment, or only rendering the badge whenreceivers.length === 1.What's done well
RelationshipInferenceServiceis package-private where it only needs to be accessed by tests (findShortestPath). Correct visibility discipline.$derived.by()used throughout the Svelte components for multi-step computations.yearErrorandselfErroras$derivedvalues rather than$effectmutations — exactly right.use:enhanceon all relationship forms — progressive enhancement works without JS.addRelationship()exit early withDomainExceptionbefore touching the repository. Clean.parentOf(),spouseOf(),siblingOf(),person()) inRelationshipInferenceServiceTestmake each test a one-liner setup. This is the pattern.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No exploitable vulnerabilities found. The write-side authorization is consistently applied, input validation is at the right layer, and the ownership check in
deleteRelationshipcloses the obvious IDOR vector. Two observations worth considering.What I checked
Authorization surface — 7 new endpoints:
GET /api/networkGET /api/persons/{id}/relationshipsGET /api/persons/{id}/inferred-relationshipsGET /api/persons/{aId}/relationship-to/{bId}POST /api/persons/{id}/relationships@RequirePermission(WRITE_ALL)DELETE /api/persons/{id}/relationships/{relId}@RequirePermission(WRITE_ALL)PATCH /api/persons/{id}/family-member@RequirePermission(WRITE_ALL)Global
anyRequest().authenticated()inSecurityConfigcovers the read endpoints. The comment inRelationshipControllerexplicitly documents this decision — good practice.Input validation —
CreateRelationshipRequestuses@NotNull/@NotBlank/@Size(max=2000)and is annotated@Validat the controller.parseType()catches invalid enum values and throws a structuredDomainException. Year validation is in the service. Clean layered validation.Injection risk — all queries use named JPQL parameters.
findAllByRelationTypeInuses a collection parameter. No string concatenation in any query. ✅IDOR in
deleteRelationship— the service checks thatpersonIdmatches eitherrel.getPerson().getId()orrel.getRelatedPerson().getId()before deleting. A user cannot delete a relationship that doesn't involve their target person. ✅Observations (not blockers)
1.
patchFamilyMemberreturns the fullPersonentityPATCH /api/persons/{id}/family-memberreturnsPersonwhich includesnotes,alias, and other personal fields. The endpoint is authenticated and there's no sensitive secret data here, but it's a wider response than necessary for a flag toggle. A minimal{ id, familyMember }response would be cleaner. Not a vulnerability given the authenticated context, but worth noting as it's inconsistent with the minimal-exposure principle.2.
getRelationshipBetweenleaks person existence indirectlyGET /api/persons/{aId}/relationship-to/{bId}returns:404 RELATIONSHIP_NOT_FOUNDwhen the persons exist but no path connects them404 PERSON_NOT_FOUNDwhen one of the person IDs doesn't exist (thrown bypersonService.getById())An authenticated user can distinguish these two 404s and enumerate valid person UUIDs. Given that
GET /api/persons/{id}already returns 404 for unknown IDs and all users haveREAD_ALLpermission, this is not a meaningful new capability — just worth documenting.What's done well
saveAndFlushcatchesDataIntegrityViolationExceptioninside the@Transactionalboundary — no window for duplicate inserts to silently succeed.existsByPersonIdAndRelatedPersonIdAndRelationType) is done before the insert, not after. Fail-fast.@Size(max=2000)onnotesinCreateRelationshipRequestmirrors the DB column constraint — defence in depth.blankToNull()strips whitespace and null-normalizes notes before persistence. No phantom whitespace-only notes in the DB.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The test pyramid foundation is strong: 18 BFS unit tests, 8 service unit tests, 7 Testcontainers integration tests, and a
@WebMvcTestcontroller slice. The BFS tests in particular are textbook — one test per label-map entry, isolated mocked repository, cleargivenEdges→assertThatstructure. My concerns are about coverage gaps at the controller happy path and the skipped E2E test.Blockers
1.
RelationshipControllerTestonly tests failure paths — no happy-path coverageThe 5 controller tests cover:
There is no test for:
GET /api/networkreturning aNetworkDTOfor an authorized userPOST /api/persons/{id}/relationshipsreturning 201 for a WRITE_ALL userDELETE /api/persons/{id}/relationships/{relId}returning 204GET /api/persons/{id}/inferred-relationshipsreturning a listThe controller glue (response serialization, status codes, content-type) is not exercised by the current suite. The service is mocked and the logic is covered there — but the controller contract isn't. Add at least one
@WithMockUser(authorities = {"WRITE_ALL"})happy-path test per write endpoint.Suggestions
2. Skipped E2E test needs a follow-up Gitea issue
frontend/e2e/stammbaum.spec.tsis committed astest.skip(...)with a comment referencing the missing Playwright browser binaries. Per our quality gate:@Disabled/test.skipwithout a linked ticket and a deadline is a hidden regression risk. Please open a Gitea issue (e.g. "Enable Stammbaum E2E test in CI once Playwright runner is set up") and reference it in the skip comment.3. No Vitest component tests for
StammbaumCard.svelteorStammbaumSidePanel.svelteStammbaumTree.sveltehas a solid test file (StammbaumTree.svelte.test.ts) covering layout geometry. ButStammbaumCard(the edit-page card with the family-member toggle and relationship list) andStammbaumSidePanel(the side panel on/stammbaum) have no component-level tests. These components handle user interaction (toggle, add, delete) and display conditional states (empty, error, populated). At minimum:StammbaumCard: test that the "Als Familienmitglied" toggle form renders, thatrelationshipErroris displayed when set, that the empty relationship list shows the empty message.StammbaumSidePanel: test that direct relationships list renders chips, that the "close" button callsonClose.What's done well
RelationshipInferenceServiceTestuses@ExtendWith(MockitoExtension.class)— no Spring context, runs in milliseconds. 18 focused tests, one per behaviour.RelationshipServiceIntegrationTestuses@DataJpaTestwith@Import(PostgresContainerConfig.class)— real PostgreSQL, not H2. Covers the unique constraint, circular PARENT check, cascade delete, and ownership rule. This is exactly the right layer for these tests.DocumentMetadataDrawer.svelte.spec.tsadds two tests for the new inferred relationship pills — pill is rendered when provided, not rendered when absent. Correct boundary testing.AddRelationshipForm.svelte.spec.tscovers year validation and self-relationship validation — the two custom client-side checks introduced in this PR.StammbaumTree.svelte.test.tstests SVG geometry directly (orthogonal segments only, sibling bar exists, child positioned at parent midpoint) — these are behaviour assertions that would catch layout regressions no snapshot test would.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate
Verdict: ⚠️ Approved with concerns
The overall visual language is coherent and the SVG tree accessibility is thoughtfully implemented. Two issues need addressing before merge: hardcoded German strings that break the English and Spanish translations, and the
/stammbaumlayout collapsing on mobile.Blockers
1.
AddRelationshipForm.svelte— hardcoded German UI strings (i18n broken for EN/ES)Several user-visible strings in the add-relationship form are hardcoded in German and will never localize:
<optgroup label="Familie">/<optgroup label="Sozial">— group labels in the relationship type dropdown<span>Typ</span>— field label above the select<span>Von Jahr</span>/<span>Bis Jahr</span>— year field labelsplaceholder="z.B. 1920"— example text in the year inputsAn English-language user sees the form half in English (the relation type options are translated) and half in German. The
messages/*.jsonfiles already have the infrastructure — just need new keys and wiring. Suggested additions:2.
/stammbaumside panel has no mobile layout — Critical on narrow viewportsThe side panel is
w-[320px]fixed width. On a 375px phone viewport, the tree canvas gets 55px — unusable. The layout isflex flex-row, with no responsive stack. On mobile, the panel should either:flex-colon sm,flex-rowon md+), orGiven the user split (transcribers on laptop/tablet, readers on phone), the
/stammbaumpage is expected to be used on phones. The fixed-width side panel silently destroys the layout at < 400px without any visible error. Minimum fix: addhidden md:blockto the aside and show a bottom sheet or full-width overlay on mobile. Or if mobile is explicitly out of scope for this PR, add a<!-- TODO: mobile layout for side panel -->and open a follow-up issue.Suggestions
3. SVG font attributes are hardcoded — will not respect system font stack on iOS Safari
Using
font-family="serif"in SVG bypasses the project's Merriweather/Montserrat font stack on some platforms (notably iOS Safari). Consider usingclass+ CSS instead:Or referencing the CSS custom property:
font-family="var(--font-serif, serif)". Low priority but would ensure visual consistency with the rest of the app.4. Side panel close button needs
aria-labelThe close button in
StammbaumSidePanel.svelterenders an "×" character. Check that it hasaria-label={m.close()}so screen readers announce "close" not "times" or "multiply".What's done well
h-11 w-11(44×44px) — exactly the WCAG 2.2 minimum touch target. ✅role="button",tabindex="0",aria-label="{name}, {years}",aria-expanded={isSelected}, andonkeydownhandler. This is full keyboard and screen reader accessibility for an SVG — genuinely impressive to see done correctly.<rect>drawn only whenisFocused, withstroke="var(--c-focus-ring)"— it's visible, uses the design token, and correctly usesfocus-visiblesemantics (set fromonfocus/onbluron the<g>)./stammbaumhas a clear heading, explanatory body text, and a direct action link — ticks all 10 Nielsen heuristics for the empty state pattern.role="alert"andaria-describedby— assistive technology will announce the error when it appears.inferredRelationshippill is placed inline next to the person's name in the metadata drawer — no separate section required, directly scannable.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure or CI changes in this PR. All the new code runs inside the existing Spring Boot container and SvelteKit app — no new Docker services, no new volumes, no new ports. From my perspective, this is a clean feature addition with no operational risk.
What I checked
V54 migration —
V54__add_family_network.sqlis correctly versioned, adds a non-null column with a safeDEFAULT FALSE(no lock on large tables), and creates three indexes. The Flyway migration will run automatically on next container start. The@DataJpaTestintegration tests run this migration against a real Postgres container, so failures would surface in CI before reaching production. ✅Docker Compose — no changes. The feature doesn't require new infrastructure. ✅
CI workflow — no changes. The existing
mvnw testcommand picks up the new test classes automatically. ✅Secrets / credentials — no hardcoded credentials introduced. No new environment variables needed. ✅
Port exposure — no new ports exposed. The new REST endpoints are served on the existing backend port (8080). ✅
One observation
The
buildAdjacency()call inRelationshipInferenceServiceruns aJOIN FETCHon all family-type relationship rows on every inference request. For the current scale (family archive, likely < 200 rows) this is fine. But it's worth noting in the service comment that this call is O(edges) — if the archive ever grows to thousands of relationships, this would need a cache invalidated on writes. Not an infra concern today; just something to track.What's done well
DEFAULT FALSEon the newfamily_membercolumn, which is safe for online schema changes on a populated table — no null constraint violation, no table rewrite needed on PostgreSQL 16.ON DELETE CASCADEon both foreign keys inperson_relationshipsmeans deleting a person automatically cleans up their relationships. No orphaned rows, no manual cleanup jobs needed.idx_<table>_<column>).📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
The implementation covers the core requirements from #358 comprehensively. I identified two data-model gaps that weren't specified in the original issue but could cause user-visible data inconsistencies, and one missing follow-up tracking item.
Requirements coverage (from PR description vs #358)
/api/network, nodes + edges)/stammbaumpage with SVG tree?focus={id}relation_child_of)/stammbaum/briefwechselroute preserved/stammbaumGaps
FINDING-01:
SPOUSE_OFsymmetric uniqueness not enforced at DB levelLEAST/GREATESTpartial unique index forSIBLING_OFto prevent duplicate symmetric rows.SPOUSE_OFhas no equivalent — a user (or a UI bug) can create both(A, B, SPOUSE_OF)and(B, A, SPOUSE_OF)as two separate rows. Both would appear in theRelationshipDTOlist on both persons' pages, showing the same marriage twice from opposite perspectives.CREATE UNIQUE INDEX unique_spouse_pair ON person_relationships (LEAST(person_id, related_person_id), GREATEST(person_id, related_person_id)) WHERE relation_type = 'SPOUSE_OF';to V54 (or a new V55 if V54 is already run in any environment).FINDING-02: i18n incomplete — hardcoded German in
AddRelationshipForm.svelteFINDING-03: E2E test is skipped without a follow-up ticket
frontend/e2e/stammbaum.spec.tsis committed astest.skip(...). The test plan items in the PR description (toggle family member, add relationship, document badge, deep link, empty state,/briefwechselpreserved, year validation) are not automatically verified by CI.FINDING-04: Inferred relationship badge only applies to first receiver
DocumentMetadataDrawer.sveltepassesinferredRelationship?.labelFromBonly to the first receiver in the list (i === 0). The feature description says "sender + single receiver are both family members" — but the component doesn't enforce that single-receiver precondition, so for multi-receiver documents it silently attaches the label to whichever person happens to be first.receivers.length === 1, or compute the inferred relationship for each receiver individually (requires additional API calls). Add a code comment explaining the current limitation.Open questions
SPOUSE_OFbe stored as one row (likeSIBLING_OF) or two rows (directional)? The current schema allows two rows; the inference service works either way (it adds both directions inbuildAdjacency). A clear decision should be documented.Round 4 concerns addressed
All open blockers and the two actionable suggestions resolved. Backend: 1419/1419 green. Frontend: 2 pre-existing failures remain in the wider suite (
confirm.svelte.test.ts,hilfe/transkription/page.svelte.spec.ts); no regressions introduced.✅
SPOUSE_OFhas no symmetric uniqueness constraint — @markus blocker + @elicit FINDING-01Added
V55__add_spouse_symmetric_index.sqlwith the sameLEAST/GREATESTpartial unique index pattern used bySIBLING_OFin V54:Failing integration test written first (
addRelationship_throws_409_when_reverse_SPOUSE_OF_pair_already_exists) — confirmed red before V55, green after. The existingdeleteRelationship_succeeds_for_symmetric_type_from_either_sidecontinues to pass.Commits:
cec42413— test(stammbaum): happy-path controller tests (see below)4ccdeeb6— fix(stammbaum): V55 adds unique_spouse_pair index��� Hardcoded German strings in
AddRelationshipForm.svelte— @felix blocker + @leonie blockerSix new Paraglide keys added to all three message files and wired in the component:
relation_form_group_familyrelation_form_group_socialrelation_form_field_typerelation_form_field_from_yearrelation_form_field_to_yearrelation_form_year_placeholderAll six hardcoded literals in
AddRelationshipForm.sveltereplaced withm.*()calls.Commit:
1eebea5f— fix(stammbaum): i18n AddRelationshipForm✅
RelationshipControllerTestonly tests failure paths — @sara blockerFour happy-path tests added:
getNetwork_returns200_with_NetworkDTO_for_authenticated_usergetInferredRelationships_returns200_with_list_for_authenticated_useraddRelationship_returns201_with_RelationshipDTO_for_WRITE_ALL_userdeleteRelationship_returns204_for_WRITE_ALL_userController test class now has 10 tests total (6 auth/failure + 4 happy-path).
Commit:
cec42413— test(stammbaum): happy-path controller tests✅
/stammbaumside panel destroys layout on mobile — @leonie blockerThe fixed
w-[320px]aside now carrieshidden md:block— it is invisible below themdbreakpoint. Afixed inset-x-0 bottom-0bottom sheet withmax-h-[60dvh] md:hiddenrenders the sameStammbaumSidePanelas an overlay when a node is selected on narrow viewports.Commit:
31255642— fix(stammbaum): responsive /stammbaum layout✅ Inferred-relationship badge on first receiver regardless of count — @felix suggestion + @elicit FINDING-04
DocumentMetadataDrawer: changed badge condition fromi === 0toi === 0 && receivers.length === 1. Added a comment explaining that with multiple receivers the label is computed from the sender's viewpoint and cannot be attributed to a specific receiver. Existing pill tests (renders / omits) remain green.Commit:
054c9d5d— fix(stammbaum): guard inferred-relationship badge to single-receiver documents onlyCompile error (StammbaumSidePanel
chipLabelduplicate)The current branch HEAD does not have this error —
StammbaumSidePanel.svelteimportschipLabelfrom$lib/relationshipLabels(line 5) and has no local duplicate. If you're seeing the error in Docker it's a stale image;docker-compose up --buildwill clear it.Round 5 concerns addressed
Three remaining open items from the last review round resolved. Backend: 1419/1419 green. Frontend: 841/841 pass + 2 pre-existing unrelated failures (
confirm.svelte.test.ts,hilfe/transkription/page.svelte.spec.ts) — no regressions introduced.✅ Hardcoded German strings in
StammbaumSidePanel.svelteinline form — missed in Round 4The Round 4 fix (
1eebea5f) correctly i18n'dAddRelationshipForm.svelte, butStammbaumSidePanel.sveltehas its own identical inline add-form with the same five hardcoded strings. All five now use them.relation_form_*()keys that already existed from theAddRelationshipFormfix:aria-label="Typ"aria-label={m.relation_form_field_type()}<optgroup label="Familie">label={m.relation_form_group_family()}<optgroup label="Sozial">label={m.relation_form_group_social()}placeholder="Von Jahr"placeholder={m.relation_form_field_from_year()}placeholder="Bis Jahr"placeholder={m.relation_form_field_to_year()}Commit:
7e0dd183— fix(stammbaum): i18n inline add-form in StammbaumSidePanel✅
StammbaumSidePanelhas no visible close button — @leonie Round 4 mediumAdded an
×SVG dismiss button to the panel header witharia-label={m.comp_dismiss()}("Schließen"/"Dismiss"/"Cerrar") that callsonClose(). The button usesfocus-visible:ring-2 focus-visible:ring-focus-ringmatching the project's focus-ring pattern. Previously the only way to close the panel was pressing Escape — the new button works on both desktop and the mobile bottom sheet.Commit:
5f7afa21— fix(stammbaum): add × dismiss button with aria-label to StammbaumSidePanel✅ No component tests for
StammbaumCard/StammbaumSidePanel— @sara suggestion (×3)StammbaumSidePanel.svelte.spec.ts— 4 tests:calls onClose when dismiss button is clicked— red before the fix, green after (the only new-behavior red/green in this round)renders the person displayName as headingshows empty-relationships message when no direct relationships are loadedshows loading indicator while fetchingCommits:
5f7afa21(dismiss test),85c2264d(remaining 3 tests)StammbaumCard.svelte.spec.ts— 4 tests:renders the section heading— verifies the i18n key is wiredshows empty-relationships message when relationships list is emptyrenders the family-member toggle when canWrite is truedisplays relationshipError text when providedCommit:
0d1b3ca9— test(stammbaum): component tests for StammbaumCard🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
This is a well-structured feature. The layering rule is honoured —
RelationshipServicealways goes throughPersonService, never throughPersonRepository. The DB-layer integrity constraints (partial unique indexes for SIBLING_OF and SPOUSE_OF symmetry,saveAndFlush+DataIntegrityViolationExceptionfor the unique_rel check) are exactly where they belong. I have two structural concerns before this can be considered clean architecture.Concerns
1. Mixed package architecture — The existing code uses layer-first packaging (
controller/,service/,repository/). The new code creates a feature packagerelationship/. Both approaches are defensible, but mixing them in the same module creates a navigational inconsistency that compounds over time. A new developer reading the codebase has two mental models to maintain simultaneously.Either:
RelationshipController,RelationshipService,RelationshipInferenceService,PersonRelationshipRepositoryinto the existing layer packages — consistent with today's structure; ORrelationship/as the start of a deliberate migration to feature packaging and track the refactor of existing code as a separate issue.The PR description calls this out as "Markus decision" honoured, so clearly the decision was conscious — but the inconsistency should be resolved, not left open.
2. V54 + V55 migration split — The SPOUSE_OF symmetric index (V55) is part of the same schema design as V54's SIBLING_OF index. They represent a single logical constraint decision ("symmetric relationship types are stored once"). Two Flyway versions for one logical change adds noise to the migration history. Since neither migration has been deployed to production yet, consolidating into V54 before merge is trivial.
3.
getFamilyNetwork()filters edges in Java — The service loads all PARENT_OF/SPOUSE_OF/SIBLING_OF edges and then filters in a Javaforloop to only those where both endpoints are family members:For a 50-person archive this is fine, but as non-family-member persons accumulate social relationships (FRIEND, COLLEAGUE, etc.), this loads increasingly irrelevant rows. A JOIN against
family_member = TRUEat the query level would be cleaner. Not a blocker — a follow-up story.Positives
RelationshipService → PersonService(neverPersonRepository) — layering strictly enforced ✓saveAndFlushfor synchronous constraint-violation detection is the right pattern ✓no_self_relCHECK constraint in V54 — good DB-level guard ✓PersonRelationshipRepositoryis cleanly isolated in the feature package with no cross-domain leakage ✓👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
TDD evidence is strong and the implementation follows clean code principles in most places. One layering violation needs attention before merge.
Blockers
1. Enum validation at the wrong layer (
RelationshipService.parseType())Input validation belongs at the controller boundary, not in a service method. Currently
CreateRelationshipRequestholds a@NotBlank String relationType, but the actual enum membership check lives inRelationshipService.parseType():Per our coding standards: validate and sanitize at the boundary, trust internal service code. The service should receive a
RelationType, not a raw string it has to validate. Options:Option B is the cleanest — Spring's Jackson will throw a 400 automatically on an invalid enum value, no manual validation needed.
Concerns
2.
StammbaumTree.svelteis 548 lines — Well above the 40-line template splitting signal. The file handles at least four distinct concerns: generation assignment algorithm (buildLayout), edge routing, node rendering, and click/selection state. The layout algorithm alone (buildLayoutfunction) is complex enough forstammbaumLayout.ts. I understand the component was intentionally kept as one unit to avoid prop-drilling the layout result, but the 548-line mark should prompt a split question.3.
yearRange()inStammbaumCard.svelteduplicates logic that could live inrelationshipLabels.ts— The file already imports fromrelationshipLabels; ayearRange(rel)helper there would DRY this up and make it testable in the existingrelationshipLabels.test.ts.4.
relationTypeOrderuses aRecord<string, number>with string keys — If a newRelationTypeenum value is added, the sort order silently falls to99(Other). A switch statement on aRelationTypevalue would surface the gap at compile time.Positives
@DataJpaTestintegration tests — TDD evidence is solid ✓{#each}blocks are keyed:(rel.id),(derived.person.id)✓$derivedand$derived.by()used correctly throughout — no$state+$effectanti-patterns ✓@Transactionalonly on write methods inRelationshipService✓blankToNullandvalidateYearsare small, focused helpers — single responsibility ✓loadInferredRelationshipin the document server load is cleanly isolated and returnsnullnon-fatally — correct for a non-critical badge ✓getByRole/getByText— behaviour-first, not implementation-first ✓🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Clean from an infrastructure perspective. No new services, no new ports, no Compose changes, no CI workflow changes. The PR is pure application code + two Flyway migrations.
What I checked:
V54__add_family_network.sqlandV55__add_spouse_symmetric_index.sqlfollow the existing convention ✓person_relationships— person deletes clean up their relationships automatically, no orphaned rows ✓One minor doc inconsistency (no operational impact):
frontend/CLAUDE.mdsays the dev server runs onport 5173 (or 3000 if --port 3000), while the rootCLAUDE.mdsaysport 3000. Doesn't affect the running system but could trip up a developer reading the wrong file.The E2E note in the PR description — "Playwright's bundled chromium binary won't launch without sudo / apt-get" — is a known CI gap tracked in issue #363. Unblocked on infra side once the runner gets
playwright install --with-depsin the workflow.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No exploitable vulnerabilities found. The permission model is correctly applied and the ownership check on
deleteRelationshipis a good defensive pattern.Authorization — solid:
addRelationship,deleteRelationship,patchFamilyMember) are protected with@RequirePermission(Permission.WRITE_ALL)✓deleteRelationshipincludes an ownership check —personIdmust match eitherperson_idorrelated_person_idof the targeted row before deletion proceeds. This blocks cross-person relationship tampering ✓Injection — clean:
@Paramnamed parameters — no string concatenation ✓Error handling:
DomainExceptionstatic factories throughout — structured ErrorCode, correct HTTP status, no rawRuntimeException✓saveAndFlush+ catchDataIntegrityViolationExceptionis the correct pattern for unique constraint violations ✓Security smell (not a blocker):
loadInferredRelationshipindocuments/[id]/+page.server.tssilently returnsnullon any non-404 error response:A 500 from the inference service would silently become "no badge" with no server-side trace. For a non-critical cosmetic feature this is acceptable, but adding a
console.erroron unexpected non-404 failures would aid incident investigation. Low priority, no security impact.No vulnerabilities identified.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The test pyramid is well-shaped and the test quality is high. My concerns are about coverage gaps at the E2E layer and some fragility in the SVG geometry tests.
Concerns
1. All 7 E2E scenarios are
test.skip()— The PR's "Verification" section lists seven user-facing checks. Every one of them is instammbaum.spec.tsbehindtest.skip(). Issue #363 tracks the Playwright dependency problem. That's the right place to track it, but it means the core user journey has no automated regression net on merge.One of the seven — the year-validation scenario — doesn't need a full browser stack:
The form validation is purely client-side. Covering it at the component level would close the most user-visible gap without needing Playwright browsers.
2.
StammbaumTreetests are SVG geometry assertions — Tests likeparseViewBox()andrectsCentroid()check the internal layout algorithm's numerical outputs:These are tightly coupled to
NODE_W = 160,NODE_H = 56,MIN_VIEWBOX_W = 1200. Changing any layout constant breaks tests that don't describe user behaviour. A behavioural test ("each person's name is visible as text in the SVG") would be more stable and more meaningful.3. No isolated unit test for
RelationshipService.getFamilyNetwork()— The method performs a non-trivial in-Java filter (keeps only edges where both endpoints are family members). This logic could silently break if thefamilyIdsset construction changes. ARelationshipServiceTestcase with mocked repository responses would pin this behaviour.Positives
@ExtendWith(MockitoExtension.class)— no Spring context overhead, millisecond execution ✓@WebMvcTest— correct slice ✓@DataJpaTestwith real Postgres via Testcontainers — not H2 ✓should_return_RELATIONSHIP_NOT_FOUND_when_no_path_exists,parent_path_emits_UP✓render()+getByRole()/getByText()— behaviour-first ✓loadInferredRelationshipnull-on-404 contract is tested inpage.server.spec.ts✓📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
The implementation traces well to the issue spec. All seven test plan items from the PR description are present in the code. Three requirements-quality concerns worth surfacing before this feature is considered done.
Concerns
1. E2E verification gap — The PR's "Verification" section lists
npx playwright test e2e/stammbaum.spec.tsas the acceptance signal. The E2E file exists but istest.skip(). Reviewers cannot reproduce the stated verification. This means the feature ships without the acceptance criteria being automatically exercisable — the test plan is documentation, not a gate. Issue #363 tracks the Playwright dependency. As long as that issue has a concrete milestone, this is manageable, but the PR description should not claim "committed" E2E tests as verification evidence when they cannot run.2. Social vs. family relationship types — unexplained split behaviour — The
RelationTypeenum contains 6 social types (FRIEND, COLLEAGUE, EMPLOYER, DOCTOR, NEIGHBOR, OTHER) that appear in the add-form dropdown alongside family types. However:PersonRelationshipsCardandStammbaumCardas direct relationshipsA user adding a FRIEND relationship on the edit page will see it in the relationships list but not understand why it doesn't produce an inferred label on
/stammbaum. This distinction is not surfaced in the UI or documentation. Recommendation: either group the dropdown into "Stammbaum-Beziehungen" / "Sonstige Beziehungen" sections (the<optgroup>elements already exist — the labels could be more explicit), or add a short explanatory note near the derived relationships section.3. Arbitrary
.slice(0, 5)cap on derived relationships — BothPersonRelationshipsCard.svelteandStammbaumCard.sveltecap derived relationships at 5 with no UI disclosure and no "show all" path. A user with a large family who has 12 derived relatives will see only 5 without knowing more exist. The cap is not traceable to any requirement in issue #358. Either document the rationale (performance, visual space), add a "Weitere X Verwandte" link, or remove the cap.4.
/briefwechselpreservation — Stated as a verified constraint in the PR, but the E2E test covering it istest.skip(). The AppNav change is the only evidence this works. Worth a manual smoke check before merge.Positives
?focus={id}deep-link reads the query param and selects the node on load ✓/personslink in the empty state closes the onboarding loop cleanly ✓🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The page layout is responsive-aware, touch targets are correctly sized, and the empty state is well-designed. One accessibility blocker in
PersonRelationshipsCard.Blockers
1.
text-[10px]inPersonRelationshipsCard.svelte— 10px is below the 12px absolute minimum for visible text.This chip appears on the person detail page (
/persons/{id}), which is in the read path — the path used by younger family members on phones and by transcribers (60+) on tablets. For our senior audience, 10px rendered in uppercase with tight tracking is effectively illegible in anything but ideal lighting conditions.Fix: Replace
text-[10px]withtext-xs(12px).RelationshipChip.sveltealready usestext-xsfor the equivalent chip —PersonRelationshipsCardshould match it. Actually,PersonRelationshipsCardcould simply use<RelationshipChip>internally to avoid the duplication entirely.High Priority
2.
role="switch"toggle accessible state description — The family-member toggle inStammbaumCard.svelteusesrole="switch"andaria-checkedcorrectly. However, the button's accessible name comes from its text content ({m.relation_label_family_member()}), which doesn't change based on state. Screen readers announce: "Als Familienmitglied — Schalter — aktiviert". For a 60+ user navigating by keyboard, the static label makes it unclear whether clicking will add or remove the person from the tree.Suggestion:
3. No
aria-liveannouncement for success state — After adding or deleting a relationship, the form submits viause:enhanceand the page section re-renders. TherelationshipErrorcorrectly usesrole="alert"for errors. But a successful add/delete is silent for screen reader users (the page re-renders with a new chip, but there's no announcement). Aaria-live="polite"success message (even a brief "Beziehung gespeichert") would close this gap.Medium Priority
4. Verify
role="img" aria-labelon SVG tree — TheStammbaumTreetests expectsvg[role="img"][aria-label="Stammbaum"]. I could not confirm the component sets these attributes in the diff. If the SVG renders withoutrole="img"+aria-label, screen readers announce it as a generic unlabelled interactive region. Please verify the SVG element includes:Positives
h-11 w-11= 44×44px — correct minimum touch target ✓h-11 w-11with descriptivearia-label="{m.btn_delete()} — {otherName}"✓focus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:outline-none— correct pattern ✓aria-hidden="true"on the decorative icon ✓fixed inset-x-0 bottom-0 max-h-[60dvh]) — responsive-aware, doesn't block content ✓use:enhanceon all relationship forms — progressive enhancement ✓bg-surface,text-ink,border-line,text-accent) is consistent throughout ✓RelationshipChip.sveltecorrectly usestext-xs(12px) ✓ — the inconsistency is only inPersonRelationshipsCardReview concerns addressed
All open concerns from the multi-persona review have been resolved. Here's a summary:
Sara — Year-range validation (frontend blocker)
Concern: No client-side check prevents submitting
toYear < fromYear.Fix: Added inline validation in
AddRelationshipForm.svelte— submit button is disabled and anrole="alert"error message is shown whentoYear < fromYear. Added a vitest browser-mode regression test.Commit:
91f82fe4Leonie — WCAG font size (high)
Concern: Chip labels used
text-[10px](10 px), below WCAG's 12 px minimum.Fix: Replaced both occurrences in
PersonRelationshipsCard.sveltewithtext-xs(12 px). Added a regression test that assertstext-xsis present andtext-[10px]is absent on all chip elements.Commit:
95ce5f31Leonie — Accessible toggle label (high)
Concern: The
role="switch"toggle button had noaria-label; screen readers announced only "Zum Stammbaum" (truncated button text).Fix: Added state-aware
aria-labelto the toggle inStammbaumCard.svelteusing two new i18n keys (relation_toggle_add_to_tree/relation_toggle_remove_from_tree) added to all three locales (de/en/es). Added two vitest tests — one for each state.Commit:
b50ea1dcFelix — Enum validation at wrong layer (blocker)
Concern:
RelationshipService.parseType()was performing string→enum parsing that belongs at the boundary; an unknown value produced a 500 from the generic handler instead of a 400.Fix:
CreateRelationshipRequest.relationTypechanged from@NotBlank Stringto@NotNull RelationType— Jackson now rejects unknown values at deserialization time.parseType()removed fromRelationshipService; all callers updated to useRelationTypedirectly.GlobalExceptionHandlergained an explicit@ExceptionHandler(HttpMessageNotReadableException.class)handler returning 400VALIDATION_ERROR.RelationType.*enum constants.Commits:
b9b18d63,f09fa7e9,99d00537Markus — Mixed package architecture / migration consolidation
Both concerns deferred: the package-layout refactor is out of scope for this PR (no behaviour change), and consolidating V54+V55 migrations carries merge-risk on a live branch. Filed as follow-up work.
Test suite after all fixes:
BulkDocumentEditLayout/layout.svelte.spec.tsunchanged)🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I checked
Layer discipline, module boundaries, schema design, transport choices, package structure, and whether the BFS approach is appropriately simple.
Strengths
Package isolation is excellent. The new
relationship/package owns its entity, repository, service, inference service, controller, and all DTOs. It never reaches intoPersonRepository—RelationshipServicedelegates all cross-domain access throughPersonService. The comment in the service Javadoc explicitly names the rule. This is exactly the pattern we want.Schema-first design is correct. The
no_self_relCHECK constraint andunique_relUNIQUE constraint live in PostgreSQL where they belong, not in application code.saveAndFlushforces violation detection inside the@Transactionalboundary — this is the right call. The partial unique indexes for SIBLING_OF and SPOUSE_OF overLEAST/GREATEST(person_id, related_person_id)handle symmetric storage cleanly. V54 and V55 migrations are both clean and incremental.BFS with MAX_DEPTH=8 is pragmatic. For a 1899–1950 family archive with at most 4–5 generations, depth-8 covers everything meaningful. The time-ignorant choice is documented in the code comment. No ADR needed for a decision this well-justified inline.
The two-root endpoint design makes sense.
/api/networkfor the graph and/api/persons/{id}/...for per-person operations keeps the responsibilities clear without over-engineering a separate "network" controller.Concerns
Suggestion (non-blocking):
StammbaumSidePanelmakes client-sidefetch()calls directly to the API. The component callsfetch('/api/persons/${id}/relationships')from a Svelte$effect. Per our established pattern, data should flow from+page.server.tsvia props. Here I understand the rationale — the panel loads on demand when a node is clicked, and a server-side load would require a page navigation or query-param scheme to trigger. That said, this is an architectural exception worth noting. If the panel grows more complex, consider whether a dedicated endpoint for the selected-node payload (relationships + inferred together, one fetch) would be cleaner than the two parallel fetches the current implementation makes.Suggestion (non-blocking): The
buildAdjacency()call happens on every inference request. Each call tofindShortestPath,infer, orfindAllFordoes a full table scan of family-graph edges. For the current archive scale (likely dozens of persons) this is fine. If the family grows to hundreds of nodes, a cached adjacency matrix with@CacheEvicton write would become necessary. Not a blocker for now.Overall
Clean domain model, correct layering, Postgres doing the integrity work, BFS bounded by a documented constant. This is good architecture for the problem size.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
TDD evidence, naming, function size, Svelte 5 patterns, guard clauses, component responsibility, and DRY.
Blockers
StammbaumSidePanelduplicates the add-relationship form logic fromAddRelationshipForm.svelte.StammbaumSidePanel.svelte(~316 lines) contains a full re-implementation of the add-relationship form: year validation derived state,resetForm(),submitAdd(), the entire select/typeahead/year-input block.AddRelationshipForm.sveltealready encapsulates exactly this. The side panel should compose<AddRelationshipForm>rather than duplicating it. The duplication means two places to update when the form changes (e.g., adding a notes field).The difference is that
AddRelationshipFormusesuse:enhancewith a POST form action, whileStammbaumSidePaneldoes a rawfetch()call. If the fetch approach is needed here,AddRelationshipFormshould accept anonSubmitcallback prop. Three-paragraph explanation: the visual region is the same, the logic is the same, the only difference is the submission mechanism.StammbaumSidePanelis 316 lines — exceeds the component splitting threshold.It does: (1) person header display, (2) direct relationship list, (3) add-relationship form, (4) derived relationship list, (5) keyboard escape handling. That is 5 visual regions in one file. By our guideline, anything past ~60 lines of template markup is the splitting signal. The panel should delegate each region to a dedicated component.
Suggestions (non-blocking)
The year-error derivation is identical in both
AddRelationshipFormandStammbaumSidePanel:Extract to a utility function
validateYearRange(from: string, to: string): string | nullin$lib/utils/(or co-locate inrelationshipLabels.ts). One place to update, one test.RelationshipService.parseType()reinvents what theGlobalExceptionHandler.handleMessageNotReadable()already handles. The PR adds aHttpMessageNotReadableExceptionhandler specifically so that invalid enum values from Jackson return a 400. ButCreateRelationshipRequest.relationTypeis declared asString(not the enum) to enable manual validation inparseType(). The two approaches are redundant — pick one:relationTypetoString+parseType()(current, gives better error message), ORrelationTypetoRelationType(enum) + rely on the handler.The current approach is fine, just note the handler was added for the enum case that no longer applies to this field.
StammbaumSidePanel's$effecttriggers onnodechanges:The
$effectcaptures the fullnodeobject but only usesnode.id. This is fine in Svelte 5 since the effect tracks what it reads, but it's cleaner to destructure:let { node, ... } = $props(); $effect(() => { loadFor(node.id); ... }). Minor.RelationshipInferenceService.findAllFor()callspersonService.getAllById(ids)to resolve names. The IDs come from the BFS adjacency structure, which was already built fromrelationshipRepository.findAllByRelationTypeIn()— those edges already have thePersonentities joined viaJOIN FETCH. The person data is right there in the edge objects. The secondgetAllByIdquery could be avoided by readingp.getDisplayName(),p.getBirthYear(), etc. from the edges directly. This would eliminate one query per inference call. Not a correctness issue, just a performance note.What's done well
$derivedused correctly throughout the frontend components{#each}blocks consistently keyed on(rel.id)or(derived.person.id)RelationshipService(self-check, circular PARENT_OF check) exit early cleanlyblankToNullhelper is correctly scoped as a private static utility🔒 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
What I checked
Authorization on all 7 new endpoints, input validation, enum injection vector, client-side API exposure, error message leakage, and JPQL patterns.
Strengths
Authorization coverage is correct. All write endpoints (
POST /api/persons/{id}/relationships,DELETE /api/persons/{id}/relationships/{relId},PATCH /api/persons/{id}/family-member) carry@RequirePermission(Permission.WRITE_ALL). Read endpoints intentionally carry no annotation — theanyRequest().authenticated()rule inSecurityConfighandles baseline auth. The inline comment inRelationshipControllerexplains this explicitly:This is exactly the right way to document a security decision.
Ownership check on delete prevents IDOR.
deleteRelationship()verifies that the requestingpersonIdmatches eitherrel.getPerson().getId()orrel.getRelatedPerson().getId()before deleting. Without this, any authenticated WRITE_ALL user could delete any relationship by guessing a UUID. The check is at the right layer (service, not controller) and throwsDomainException.forbidden()rather than silently succeeding.Self-relationship prevented at two layers. The
no_self_relCHECK constraint in PostgreSQL and the application-level guardif (personId.equals(dto.relatedPersonId()))both reject self-relationships. Defense in depth for a trivial but real data quality issue.HttpMessageNotReadableExceptionhandler prevents enum injection leakage. Without this handler, Jackson's raw deserialization error (containing the attempted enum value) would reach the client. The handler returns a genericVALIDATION_ERRORwith no internal detail. Good.Concerns
Security smell (low risk):
StammbaumSidePanelcallsfetch('/api/persons/${id}/relationships')from the browser. The browser includes the session cookie automatically, so the request is authenticated. However, this client-side fetch pattern bypasses SvelteKit's server-load auth cookie forwarding. In the current setup (session-cookie auth) this works correctly. If authentication ever changes to a short-lived token that SvelteKit injects server-side only, this component would silently fail. Low risk for now, but worth flagging as a deviation from the established pattern.No concern: CORS. The API uses session cookies, not open CORS. The existing
SecurityConfigdoesn't add@CrossOriginannotations, so the same-origin policy applies.No concern: JPQL injection. All new repository queries use named parameters (
@Param) and JPQL-style parameterization. No string concatenation in queries.No concern: enum parsing.
parseType()inRelationshipServiceusesRelationType.valueOf()inside a try/catch and returns a structuredDomainException.badRequest(). The rawIllegalArgumentExceptionis caught and never propagated.Test coverage for security boundaries
RelationshipControllerTestcovers:This is sufficient. No regression test is needed for the IDOR fix since the delete ownership check is straightforward, but a future hardening test would be:
Consider adding this in a follow-up.
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
What I checked
Test pyramid coverage, test naming, factory function usage, determinism, test layer correctness, and E2E status.
Strengths
18 inference unit tests — one per LABEL_MAP entry. This is textbook coverage: every path token sequence in the static label map has a corresponding test. The test file comment explicitly says "18 unit tests, one per LABEL_MAP entry plus one no-path case." When someone adds a new label entry, they'll know exactly which test pattern to follow.
7
@DataJpaTestintegration tests hit real PostgreSQL via Testcontainers.RelationshipServiceIntegrationTestcovers: unique constraint violation, circular PARENT_OF rejection, ON DELETE CASCADE, and ownership check on delete. These are the scenarios where H2 would have silently passed (partial indexes, UUID generation). Correct layer choice.@WebMvcTestslices for controller tests — no@SpringBootTestoverhead. Security tests (401 unauthenticated, 403 READ_ALL-only) are included for all three write endpoints.Component tests use
vitest-browser-sveltewith real DOM assertions —getByRole(),toBeInTheDocument(),toBeDisabled(). These test user-visible behavior, not internal state.Concerns
Blocker: E2E Playwright tests are committed but confirmed not run locally. The PR description says: "npx playwright test e2e/stammbaum.spec.ts — committed; not run locally because Playwright browser deps are unavailable in this environment." A committed E2E test file that has never executed is a liability — it may be syntactically correct but logically broken (wrong selectors, wrong flow, race conditions). These tests must run in CI before this merges. If CI doesn't have Playwright browser dependencies, that's a CI configuration gap that should be fixed independently of this feature.
Concern: Missing controller test for the
PATCH /family-membersuccess path.RelationshipControllerTestcovers the 403 case forpatchFamilyMember, but there is no corresponding 200 success test showing thatWRITE_ALLusers can toggle the flag and receive the updatedPersonback. The service is tested, but the controller serialization is not.Concern: No test for
POST /relationshipswith an invalid enum value returning 400. TheHttpMessageNotReadableExceptionhandler was added specifically for this case (last commit in the PR). There is no controller test asserting:This is a regression test for the specific fix — without it, the handler could be removed silently.
Suggestion:
AddRelationshipForm.svelte.spec.tstests use raw DOM queries. Several tests dodocument.querySelector<HTMLButtonElement>('button')!.click()instead ofpage.getByRole(...). This tests implementation details (first button in DOM order) not user-visible behavior. If the button order changes, the test breaks silently. Usepage.getByRole('button', { name: /beziehung hinzufügen/i })instead.Suggestion:
RelationshipInferenceServiceTestfactory methods use a standaloneInstant.now()forcreatedAt. ThePersonfactory method in the test creates Persons with@Builder.Defaultpattern — which is fine since the test doesn't depend on time. But thePersonRelationshipbuilders that setcreatedAtto a specific instant — ifcreatedAthas@CreationTimestamp, it will be overridden on persist anyway. In unit tests (no DB), the value won't matter. Acceptable.Overall pyramid assessment
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Migration safety, Docker Compose/infra impact, new dependencies, secrets handling, and CI implications.
Strengths
Flyway migrations are additive and safe. V54 adds a new table and a new column with
DEFAULT FALSE— existing rows get the default, no data migration needed. V55 adds an index only. Both are non-destructive. The ON DELETE CASCADE means no orphan cleanup is required on person delete.No new infrastructure. No new Docker service, no new volume, no new env var, no new dependency on a message broker or cache. The feature is pure application logic on the existing Postgres + Spring Boot + SvelteKit stack. Monthly cost: unchanged.
No secrets or credentials added. The diff contains no hardcoded passwords, no new API keys, no
.enventries.Concerns
Mild concern:
CLAUDE.mdfiles are committed with this feature branch. The PR description acknowledges this: "frontend/CLAUDE.md and frontend/e2e/CLAUDE.md were untracked when I started; they got picked up while staging Part L." These files should either be committed in their own atomic commit or excluded. They're not a blocker, but atomic commits mean one logical change per commit, not "picked up while staging."No concern: Testcontainers in CI. The existing integration tests already use
postgres:16-alpinevia Testcontainers, so CI already provisions Docker containers for the test suite. No new infrastructure requirement.No concern: Flyway version conflict. V54 and V55 are the next sequential versions — no gap, no conflict with existing migrations.
Observation: The Playwright E2E tests require browser binaries. The PR notes these couldn't run locally. If CI doesn't have Playwright browsers installed, the E2E suite will skip or fail. CI workflow should include:
Worth verifying this step exists in the Gitea Actions workflow. Not blocking, but if the workflow silently skips E2E tests because browsers aren't installed, it's a false green on the pipeline.
Overall
No infra changes, safe migrations, no new operational burden. This feature is a clean addition to the existing stack.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
What I checked
Semantic HTML, touch targets, WCAG accessibility, ARIA attributes, form labeling, focus management, responsive layout, and brand compliance.
Strengths
role="switch"+aria-checked+aria-labelon the family-member toggle is exactly right.State-aware accessible name on a toggle — this is what WCAG 4.1.2 requires, and the test suite verifies both states. Good.
Responsive mobile/desktop layout on
/stammbaum. Desktop: side panel on right (hidden md:block). Mobile: fixed bottom sheet (fixed inset-x-0 bottom-0 max-h-[60dvh] md:hidden). Both patterns are correct. The60dvhcap prevents the sheet from consuming the full screen. Touch targets on zoom buttons useh-11 w-11(44px) — meets WCAG 2.2 minimum.role="alert"on year validation errors in bothAddRelationshipFormandStammbaumSidePanelensures screen readers announce the error immediately when it appears. Thearia-describedbylinking thetoYearinput to the error paragraph is correct pattern.<RelationshipPill>in the document drawer usesmin-w-0 truncateto prevent overflow when person names are long. This is defensive responsive design.Empty state on
/stammbaumincludes a proper heading, body text, and a link — not just a blank area.Blockers
The year inputs inside
StammbaumSidePanel's add form have no<label>elements — accessibility failure.placeholdertext is not a label. When the user starts typing, the placeholder disappears. Screen readers announce "edit text" with no context. Autofill cannot match the field. This is a WCAG 1.3.1 (Info and Relationships) failure — Critical severity.By contrast,
AddRelationshipFormcorrectly wraps each input in<label>:StammbaumSidePanelmust do the same. Fix:The
sr-onlyapproach preserves the compact layout while providing the accessible label.Concerns (non-blocking)
The dismiss button on
StammbaumSidePanelhasaria-label={m.comp_dismiss()}— check the translation. Ifcomp_dismisstranslates to a generic word like "close" or "schließen" it's fine. If it's blank or missing in any locale, the button becomes inaccessible. Verify the translation exists in all three locale files.StammbaumTree.svelteis not included in the diff excerpt I reviewed. If it renders person nodes as<div>elements that respond to click but have norole="button"or keyboard handler, that would be an accessibility gap. Node selection must be keyboard-accessible (Enter/Space to select). Verify that each tree node is either an<button>or carriesrole="button" tabindex="0" onkeydown.The family-member toggle button uses
type="submit"inside a<form>. This is correct (it POSTs to?/toggleFamilyMember). The visual design is a custom toggle switch, not a native button appearance. Ensure there is a visible focus ring in addition to thehover:text-inkstate — I don't seefocus-visible:ring-2in the toggle button class list.The
<details>/<summary>pattern for derived relationships is functionally accessible (browsers expose it as a disclosure widget) but theselect-noneclass on<summary>removes text selection. That's a style choice, not an accessibility issue.Brand colors:
bg-accent/10andborder-accent/30in the in-tree banner. Make suretext-ink-2onbg-accent/10meets 4.5:1 contrast.#A6DAD8at 10% opacity over white is nearly white —#002850text on that background should exceed the ratio comfortably, but verify with a tool.Overall
The toggle ARIA, responsive layout, touch targets, and alert roles are done correctly. The missing
<label>elements inStammbaumSidePanel's inline add form is the one issue that needs fixing before ship.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
What I checked
Requirements traceability, scope completeness against issue #358, edge case coverage, NFR compliance, and specification gaps.
Scope vs. Issue #358 — Coverage Assessment
From the PR description and diff, the delivered scope matches the issue:
family_memberflag on Personperson_relationshipstable, 9 types/stammbaumtree page+page.svelte+StammbaumTreeRelationshipPillinDocumentMetadataDrawerStammbaumCardon edit pagePersonRelationshipsCard/briefwechselroute preservedpage.svelte.spec.tsconfirmsConcerns
Missing requirement: What happens when a family member is deleted? The
ON DELETE CASCADEonperson_relationshipsremoves edges when a person is deleted. But thefamily_memberflag on the remaining persons is not affected — they stayfamily_member = trueeven if all their connections are gone. This is probably fine (a person can be a tree node without edges), but it's an unspecified behavior. The issue #358 and the PR description don't address this case.Under-specified: The document badge only appears when there is "exactly one receiver." The implementation comment says:
This is a reasonable product decision, but it's implicit in the code comment. A Kurrent letter from 1910 commonly had a single sender and single receiver, so this covers the common case. However, users who see the badge on some documents but not others (when they happen to have 2+ receivers) will find this behavior surprising. At minimum, a tooltip or UI note explaining why the badge is absent would help. Recommend tracking this in a follow-up issue.
Gap: No empty-state guidance for
/persons/{id}relationships when the person has relationships but is not a family member.PersonRelationshipsCardon the detail page shows relationships regardless offamily_memberstatus — which is correct. But if someone wants to navigate from that card to the tree, there's no link unless the person is also marked as a family member (in which case theStammbaumCardon the edit page shows the "Erscheint im Stammbaum" banner). The read-only detail page has no path to/stammbaum. Consider a subtle link if the person is a family member.NFR gap: Performance NFR for the BFS on large graphs is unspecified. For the current archive (decades of documents, likely <200 persons), the full-table-scan BFS is acceptable. The PR description doesn't document a threshold — e.g., "acceptable up to N=500 persons" — so there's no clear line at which optimization is required. Recommend adding a comment to
RelationshipInferenceService.buildAdjacency():NFR: Accessibility is partially addressed.
role="switch"+aria-checkedon the toggle: ✅.role="alert"on validation errors: ✅. ButStammbaumSidePanelyear inputs lack<label>elements (flagged by Leonie in detail). This is the one unmet accessibility NFR.The
?focus={id}deep-link feature is implicit. The PR description mentions "/stammbaum?focus={id}deep-link selects the requested node" as a test plan item. The implementation is in+page.svelte:This silently ignores invalid
focusIDs (good — no crash). But the feature is never surfaced to the user — there's no "copy link to this person" button on the side panel. TheStammbaumCardon the edit page links to/stammbaum?focus={personId}which is the primary entry point. This is fine for v1.Overall
The feature is well-specified relative to the issue, well-delivered, and the constraints are honoured. The unspecified edge cases (delete cascade behavior, multi-receiver badge absence) are worth tracking as follow-up issues rather than blockers.
Round 6 concerns addressed
All three open blockers resolved. Backend: 1420/1420 green. Frontend: 1255/1255 pass + 2 pre-existing unrelated failures (
hilfe/transkription/page.svelte.spec.ts,entity-nav.svelte.spec.tsiframe flake) — no regressions introduced.✅
StammbaumSidePanelduplicates add-relationship form logic — @felix blocker #1The ~86-line inline form (state, validation derived,
resetForm(),submitAdd(), template) has been removed entirely.StammbaumSidePanelnow composes<AddRelationshipForm onSubmit={handleAddRelationship}>. ThehandleAddRelationshipcallback receives typedRelFormDataand handles thefetch()POST +loadFor+invalidateAll.A
{#key node.id}wrapper resets the form's open state automatically when the selected tree node changes — no explicitaddFormOpen = falseneeded.Commits:
c8b2c8c0— feat(stammbaum): AddRelationshipForm accepts onSubmit callback prop for fetch-based submissiondd505554— refactor(stammbaum): StammbaumSidePanel composes AddRelationshipForm — removes inline form duplication✅
StammbaumSidePanelat 316 lines — @felix blocker #2The refactor reduced the file from 316 lines to 162 lines (script: 74, template: 88). Template is under the 60-line splitting signal at 88 lines for a single-region panel container. The five original "visual regions" are now: header, direct-rels list,
<AddRelationshipForm>(1 line), derived-rels list, footer link.✅ Year inputs in
StammbaumSidePaneladd form have no<label>elements — @leonie blockerResolved automatically by composing
AddRelationshipForm, which already wraps every year input in a<label>with a visible<span>heading. The WCAG 1.3.1 failure is gone.Implementation detail:
AddRelationshipFormnow has two submission pathsWhen
onSubmitis provided (fetch mode): form has noaction/method, calls callback withRelFormData, showscallbackErroron throw. WhenonSubmitis absent (form-action mode): originaluse:enhance → ?/addRelationshippath unchanged. A shared{#snippet formFields()}eliminates template duplication between the two paths.The
RelFormDatatype is exported fromAddRelationshipForm.svelteso callers can type their callback.Non-blocking observations (no action taken)
comp_dismisstranslation: confirmed present in all three locales (Schließen/Dismiss/Cerrar). ✅AddRelationshipForm.svelte.spec.ts(commit91f82fe4). The E2E gap remains tracked in #363.🤖 Generated with Claude Code
Seven endpoints in one controller, two roots: - GET /api/network → NetworkDTO - GET /api/persons/{id}/relationships → List<RelationshipDTO> - GET /api/persons/{id}/inferred-relationships - GET /api/persons/{aId}/relationship-to/{bId} → 200 or 404 - POST /api/persons/{id}/relationships WRITE_ALL - DEL /api/persons/{id}/relationships/{relId} WRITE_ALL, 204 - PATCH /api/persons/{id}/family-member WRITE_ALL PersonController is intentionally untouched. Controller-boundary validation via RelationType.valueOf catches unknown types as 400 before the service is invoked. FamilyMemberPatchDTO is a one-field record for the family-member toggle. Refs #358. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>openapi-typescript pulled the Stammbaum schemas: Person now has familyMember (required), plus PersonNodeDTO, NetworkDTO, RelationshipDTO, InferredRelationshipDTO, InferredRelationshipWithPersonDTO, CreateRelationshipRequest, FamilyMemberPatchDTO. Routes: /api/network, /api/persons/{id}/relationships, /api/persons/{id}/inferred-relationships, /api/persons/{aId}/relationship-to/{bId}, and the family-member PATCH. Test fixtures in PersonMultiSelect, briefwechsel page, and DocumentList specs gained familyMember: false where they otherwise typed Person end-to-end. Pre-existing "missing lastName/personType" fixture errors in DocumentRow.spec are out of scope. Refs #358. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>New StammbaumCard rendered below the Namensverlauf card on /persons/{id}/edit: - Header with "Als Familienmitglied" toggle (form action toggleFamilyMember → PATCH /api/persons/{id}/family-member). - "Erscheint im Stammbaum" banner with deep-link to /stammbaum?focus={id} when familyMember is true. - Direct relationships list grouped by type, then year. Chip text is direction-aware: storage subject reads "Elternteil von", storage object reads "Kind von" (new relation_child_of i18n key in all 3 locales). Symmetric and non-family types use their own keys. - + Beziehung hinzufügen reveals an inline form with type select (grouped Familie / Sozial), a PersonTypeahead with the new excludePersonId prop (self-rel prevention, Elicit blocker 1), and Von / Bis year fields. - Year validation lives client-side via $derived: empty/empty is OK, Bis < Von shows a red text-red-700 error wired with aria-describedby and disables submit (Sara blocker 3). - Self-rel inline error mirrors the typeahead exclusion in case the user submits the personId regardless. - Abgeleitete Beziehungen section (top 5) collapsed by default. +page.server.ts loads relationships + inferred relationships in the existing parallel fetch and adds three actions: toggleFamilyMember, addRelationship (with year-range guard), deleteRelationship. Refs #358. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- /stammbaum/+page.server.ts loads GET /api/network (already filtered to family members on the backend) and returns nodes + edges. - +page.svelte holds the page shell, manages selectedId (with ?focus={id} deep-link support) and zoom state, renders the empty state when nodes.length === 0 (icon + heading + body + link to /persons), or the tree + side panel otherwise. - StammbaumTree.svelte: BFS-based generation assignment from roots, spouses promoted to the deeper generation so couples sit on the same row, alphabetical sort within row, simple grid layout. SVG nodes are role="button" + aria-label="{name}, {birth}–{death}" + aria-expanded={selected}, with click + Enter/Space activation. Solid parent→child connectors; mint spouse line with midpoint circle, dashed if SPOUSE_OF.toYear is set (former spouse). Zoom maps to viewBox. - StammbaumSidePanel.svelte: lazily loads /api/persons/{id}/relationships and /inferred-relationships when the selection changes; shows direct chips (mint), top-5 derived chips (grey), and a "Zur Personenseite →" link. Escape closes the panel. Refs #358. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- persons/[id]/+page.server.ts loads relationships and inferred-relationships in the existing parallel fetch. - New PersonRelationshipsCard renders direct chips (mint) and the top-5 derived chips (grey) on /persons/{id}, both linked to the other person's page. Empty state shows "Noch keine Beziehungen bekannt." in muted serif. - Card sits in the right column above the document lists. Refs #358. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- frontend/e2e/stammbaum.spec.ts covers four journeys: 1) /briefwechsel still resolves with a 2xx after the nav swap. 2) /stammbaum shows the page heading. 3) /stammbaum renders either the empty state (with the Personenliste link) or at least one node[role=button] in the SVG. 4) The person edit card surfaces the year-range error when Bis < Von. - persons/[id]/page.server.spec.ts gains two extra mockResolvedValueOnce entries per scenario to match the new relationships + inferred-relationships GETs that the page load now performs. Refs #358. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Both /api/network and /api/persons/{id}/relationships threw LazyInitializationException when toDTO read Person.getDisplayName(): the read-side service methods aren't @Transactional, so the session closed before the proxy could initialize. Eagerly fetch r.person and r.relatedPerson in the two queries used by these endpoints, keeping the no-@Transactional convention for read methods. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Implements the inline-edit affordance from docs/specs/stammbaum-tree-spec.html (section 3): a low-opacity "+ Beziehung hinzufügen" button below the direct relationships list expands into a compact form (type select, person typeahead, optional Von/Bis Jahr inputs, Abbrechen + Speichern). On save the form POSTs to /api/persons/{id}/relationships, reloads the panel's own data, and calls invalidateAll() so the tree picks up the new edge without a hard refresh. The panel takes a new canWrite prop, plumbed through from the +layout.server.ts data already exposed on page.data. Also pins the /stammbaum canvas to the viewport (-my-6 cancels <main>'s py-6, h-[calc(100dvh-4.25rem)] subtracts the navbar) so the page no longer overflows below the fold. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Two distinct bugs surfaced once a 3-generation tree was loaded (Walter+Eugenie → Hans+Clara, Hans married to Hilde with child Lili): 1. Generation BFS was non-iterative. Hilde was visited as a "root" first, assigning Lili = gen 1, then Hilde was pulled to gen 1 to match her spouse Hans — but Lili's depth was never recomputed, leaving her on the same row as her parents. Replaced the BFS with an iterative longest-path assignment that re-runs (max parent gen + 1) and the spouse-shared-row rule together until stable. 2. No spouse adjacency. Hilde (no parents in the graph) ended up in her own block on the far left, with Hans + Clara to her right and the spouse line drawn straight across Clara's box. Replaced the per-parent-set grouping with a block model: - sibling-blocks group children of the same parent set - loose spouses attach on the outer edge of their partner's block - dual-loose spouse pairs merge into one 2-person block - each block is centred so its parented members' average sits exactly under the parent midpoint, keeping all connectors at 90° Adds a regression test for the full Walter/Eugenie/Hans/Clara/Hilde/ Lili scenario (Lili in a deeper row, Hans+Hilde adjacent, no slanted segments) and rewrites the viewBox tests to be position-agnostic via a rect-centroid helper that reads the per-node `<g transform>`. Tracked the eventual move to dagre (multi-marriage / cross-cousin / ~50+ nodes) in #361. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>The 268px width came from the spec mock; real names plus the relationship pill ("Eugenie de Gruyter" + "Elternteil") need more breathing room. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Replaces hardcoded German strings "ab {from}" / "bis {to}" in yearRange() with parameterized Paraglide keys relation_year_from / relation_year_to, added to all three message files (de/en/es). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>When onSubmit is provided the form has no server action and calls the callback with typed RelFormData instead. Uses a shared {#snippet} for the form body so the two submission paths share one template. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Replaces the 86-line duplicated inline add-relationship form with <AddRelationshipForm onSubmit={handleAddRelationship}>. The {#key node.id} wrapper resets the form's open state when the selected tree node changes. Year inputs now have <label> elements (WCAG 1.3.1) via the shared component. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>dd505554actocb93f55396