feat(person): persist Excel generation (G0–G6) and show on Stammbaum gutter #690
Reference in New Issue
Block a user
Delete Branch "feature/689-generation-gutter"
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 #689.
Summary
generationindex fromcanonical-persons.xlsx/canonical-persons-tree.jsononpersons(V70 + entity + DTO chain), driven through the existingpreferHumanprecedence so re-import never overwrites a human edit.G{n}labels (role="text"+aria-labelso screen readers announce the full word) and decorative alternating stripes. Hidden entirely below md./persons/[id]/editand/persons/new. Both form actions writegenerationunconditionally — the conditional-spread idiom is not used because G 0 is a valid family-tree-root value and the dropdown must also be able to clear back to null.15 atomic commits; each green at commit time. Full per-AC mapping in the issue comment.
Carve-outs (called out for review)
db-relationships.pumlis intentionally unchanged.persons.generationis a scalar column with no FK; there is no relationship line to add or remove. The DB diagram pair (orm + relationships) was reviewed; only orm.puml needed a row.stammbaum-tree-spec.htmlstill show pre-gutter labelling — explicit out-of-scope per the issue body. The impl-ref + colour tables are updated and are the authoritative source.< md) loses the gutter by design (100 px chrome too costly on 320 px). Per-node generation on the side panel is tracked separately as #691.Test plan
./mvnw testclean (CI will run the full suite)npm run test+ browser tests clean (CI)/persons/<id>/editand confirm the change persists + re-renders on/stammbaumGeneration monotonicity violationWARNs surface in the log for any curated edges where child ≤ parentnpm run generate:apiproduces the same generation fields already mirrored infrontend/src/lib/generated/api.tsReview-round 1 fix-ups (commits 14–15+)
516a0a38— single source of truth for the 0/10 bounds (PersonGeneration.MIN_GENERATION/MAX_GENERATION); used byPersonUpdateDTO, both importers, and referenced from the V70 SQL comment.61ca5a6e— symmetric body assertion on the WebMvcTest null-clear case; new full-stack PUT→DB→GET round-trip integration tests for both null-clear and G 0.🤖 Generated with Claude Code
The gutter sits 100 px to the left of the tree canvas on md+ viewports (hidden entirely below md to preserve scrollable area on phones — see spec's deliberate dual-audience trade-off). Per occupied generation row it draws: - A full-width decorative stripe rect alternating transparent and var(--c-gutter-stripe). aria-hidden because it carries no meaning. - The label `G{n}` at the left edge, sourced from the un-shifted node.generation value (never the post-normalise rank), wrapped in `<g role="text" aria-label="Generation N">` so screen readers announce the full word instead of "G three". CSS adds --c-gutter-stripe in both the light root and the dark mode blocks (8% / 14% mint over canvas — decorative contrast carve-out). Browser tests cover label rendering, the ARIA wrapper, and the viewport-below-md absent-gutter path via a matchMedia stub. Existing StammbaumTree structural-invariant tests still pass since none of them assert anything inside the gutter region. Refs #689 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>PersonEditForm.svelte gains a G 0…G 6 select inside the {#if isPerson} block. min-h-[44px] meets WCAG 2.5.8 / dual-audience touch target. generationStr is initialised via $state(untrack(...)) so prop reruns never reset an in-progress edit (same pattern as selectedType). Both /persons/[id]/edit and /persons/new form actions read the field without the conditional-spread idiom — generation always lands in the PUT/POST body. G 0 is a valid family-tree-root value the spread would silently drop, and an empty option sends null so a human can clear the field back to "unset". i18n adds person_label_generation / person_option_generation_unset / person_hint_generation in de/en/es. Drops the dead stammbaum_generations key (zero callsites after the filter-chip removal in the spec). Tests: dropdown render + hydration in the component, generation=0/3/null arriving in the API body in the server actions. Refs #689 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The structural shape of this PR is exactly what I would have asked for: integrity pushed to PostgreSQL (CHECK + partial index), layering rule respected (the orchestrator reads the family graph through
RelationshipService.getFamilyNetwork()rather than touchingpersonRelationshipRepository), and thebuildLayoutrefactor extracts ~200 lines of pure logic out of a Svelte component into a unit-testable module. Migration V70 is single-purpose, scoped, and well-commented. The DTO/command/entity chain is updated consistently;@Schemaannotations are preserved.Blockers
Documentation drift —
docs/architecture/db/db-relationships.pumlnot in the diff. The DB-orm PUML addsgeneration : SMALLINT, but the relationships PUML is untouched. The issue body explicitly says "re-verified (no FK change — likely no diff)" — that's a deliberate carve-out and acceptable, but the PR description should call it out so the next architect doesn't reopen this. Either add a one-line comment in the PR body, or make sure the AC checkbox is ticked with the rationale. Not a real diagram blocker, just hygiene.No ADR for the strict-G layout policy. The issue body explicitly defers the ADR ("ADR for the strict-G layout policy — captured here in the issue body" → out of scope). That is your call as the issue author, and I accept it — but I want to flag for the record that this is a non-trivial precedence rule (strict rank anchor in the layout,
preferHumanfor the data) that affects the next person who readsbuildLayout.ts. The inline comment block at the top ofbuildLayout.ts:32-49does carry the rationale, which is the minimum bar. Ifdagre(#361) later inherits this seed, an ADR captures the why for both consumers in one place.Concerns (not blockers)
Generation upper bound is loose (G 0..10) when reality is G 0..6. The migration says "current data tops out at G 5, but a future G 6 → G 10 widening needs no migration" — fine, and I agree with avoiding the future migration. But the DTO mirrors this with
@Min(0) @Max(10)while the form dropdown only renders G 0..G 6. There are three slightly different ranges in play (DB 0..10, DTO 0..10, dropdown 0..6) and one comment inPerson.java:55-58mentions "G 0 = oldest" without naming the upper bound. This is fine today but a smell. If the dropdown changes to G 0..G 8 in six months, who remembers that the DTO and CHECK are already wider? Consider a single Java constantPerson.MAX_GENERATION = 10referenced by both@Maxand the migration comment, and an inline note in the Svelte template that the dropdown is a narrower client-side view of the DB range.PersonRegisterImporter.GENERATION_MAXis duplicated inPersonTreeImporter. Two integer pairs (MIN=0, MAX=10) live in two files, plus the SQL CHECK, plus the Bean Validation, plus the dropdown. That's five copies of the same allowlist. Not a violation of any layering rule — but the day someone widens to 12, they will edit four out of five places. Extract aGenerationvalue type or a shared constant holder in thepersonpackage, and reference it from both importers and the DTO@Max. KISS judgment: probably YAGNI today, but worth a follow-up TODO.warnOnGenerationMonotonicityViolations()is O(nodes + edges) but reads the full family network on every import. For 1,105 persons that's nothing — for 100k it becomes a measurable cost. Acceptable today (the import is a batch job and the WARN is forensic). Mention this in the method Javadoc so a future optimiser doesn't tear the soft-check out without realising it's the only signal we have.LGTM checks
CanonicalImportOrchestrator→RelationshipService(neverpersonRelationshipRepository).frontend/src/lib/generated/api.tswas regenerated, matching the AC.preferHuman(Integer, Integer)overload reused — no new precedence idiom invented. Known-limitationdocumenting_known_limitationtest pins current behaviour and points the way to fix it ifbirthYear/deathYearever produce a user-visible bug.buildLayout.ts— the function is the original code withgenerationandlockedadded on top).Approve the structure. Resolve doc-hygiene and the constant-duplication smell as a follow-up, not a hold.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
This is textbook TDD across three stacks. Every change has a test that precedes it: the parameterised regex matrix in
PersonRegisterImporterTest, the fourbuildLayout.test.tscases (Herbert Cram regression, strict-seed override, fallback inheritance, normalise), the form-actionpage.server.spec.tsproving thegeneration: 0case round-trips, and the controller validation tests cover both 400 boundaries plus the 200 happy path. The refactor-first sequencing (extractbuildLayoutbyte-for-byte, then add the seed/fallback/normalise behaviour) is exactly the discipline I'd expect.Notes
Guard clauses everywhere.
parseGeneration()andgenerationOrNull()both lead withif (raw == null/blank) return null;then range-check. Two-line guards, no nesting. Good.Naming reveals intent.
GENERATION_PATTERN,parseGeneration(raw, personId),generationOrNull(node, personId),gutterRows,isMdOrUp,GUTTER_WIDTH_DESKTOP— every name carries the visual or domain concept. Zerotmp,data,value,flagabbreviations.SvelteMap/SvelteSetadopted in the new gutter code AND retroactively inparentLinks. The eslint-disable forsvelte/prefer-svelte-reactivitywas dropped — that's the right move, and the lint rule now does its job on this file going forward. PlainMap/Setare only used insidebuildLayout.tswhich is pure (not reactive) — also correct.$derived.byover$state + $effectforgutterRows,gutterWidth,viewBox. Synchronous, single-pass. The one$effectis exactly where it should be: bridging an imperative DOM API (matchMedia.addEventListener) into a$state. That is the canonical use of$effect.Functions stay small.
parseGenerationis 9 lines including the doc-comment.generationOrNullis 5.warnOnGenerationMonotonicityViolationsis 17 lines and does one thing (walk PARENT_OF edges, log violations). The new layout function would be too big to land in one component file — extracting it was the right structural call.Form action uses the issue-mandated unconditional-key idiom.
+page.server.tsfor both[id]/editandnewwritegenerationas a top-level key, never inside...(value ? {...} : {}). The inline comment block explicitly calls out why (G 0 is valid, the spread drops it). Saved a reviewer a "wait, why isn't this spread?" question.Property-key in the
{#each}for gutter stripes uses a stable string (`stripe-${row.rank}`,`label-${row.rank}`). Not the(item.id)form but the rank is the natural identity for a row band. Acceptable.Tiny suggestion (non-blocker)
The
GENERATION_MIN/GENERATION_MAXconstants inPersonTreeImporter.java:103-104sit below the method that uses them. Spring doesn't care, but a reader scanning the class top-to-bottom hits the usage before the definition. Bump them to the top of the class next to the existing constants. Pure aesthetic, not a bug.One more: in
StammbaumTree.svelte.test.ts:683-696, thematchMediashim returns an object literal cast throughunknown. Vitest-browser users sometimes prefervi.stubGlobal('matchMedia', () => ({...})). Either form works — flagging only because the cast chain (as unknown as typeof window.matchMedia) is the kind of thing eslint flags as a smell. Not a blocker.LGTM bottom line
13 atomic commits, every commit green at commit time per the PR body. Refactor → red/green per concern, never mixed. The test-to-impl ratio in the backend (
PersonImportUpsertTest,PersonServiceTest,PersonControllerTest,PersonRepositoryTest,PersonRegisterImporterTest,PersonTreeImporterTest,RelationshipServiceTest,RelationshipControllerTest,CanonicalImportOrchestratorTest) is exactly the spread I want: unit at the service, slice at the controller, integration at the repository, orchestrator at the wiring. Ship it.🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a small, clean migration on a stack I already operate. Nothing in the diff changes the service topology, adds a container, edits Compose, or rewires CI — so my surface area is the Flyway migration plus the operational fingerprint of the new code. Both check out.
What I checked
V70__add_generation_to_persons.sql— single ALTER TABLE adding a nullable SMALLINT, a CHECK constraint, and a partial index. Idempotent under Flyway's standard semantics. Zero downtime: nullable column add on Postgres 16 is a metadata-only operation, no table rewrite. The partial index is the right call — only ~163 of 1,105 rows ever get a value, so a full index would waste space and write throughput. Runs in <100 ms on production-size data. No backfill, no lock escalation.No new env vars, no new ports, no new services. Compose unchanged. Observability stack unchanged. The PR description correctly does not bump
PORT_*orGLITCHTIP_DOMAIN.Logging discipline.
log.warn(...)for skipped out-of-range values and monotonicity violations uses SLF4J parameterised placeholders ({}), not string concatenation. That goes through structured logging cleanly to Loki — searchable bypersonIdfield.log.debug(...)on the success path stays out of production logs by default but is there when the curator wants forensics.No bind-mounted persistent data added. The new partial index lives in
postgres_data(named volume) like everything else.No new secrets, no hardcoded credentials. I checked the spec HTML, the new tests, and the Svelte components — nothing leaks an env value.
generate:apiwas actually run.frontend/src/lib/generated/api.tsis in the diff with the three DTOs carryinggeneration?: number | null. The most common "I forgot to regenerate" CI failure won't fire on this PR.Operational notes for the future
Backup impact: zero. SMALLINT × 1,105 rows ≈ 2 KB on disk. Negligible.
Reindex on the partial index will be needed if the family expands by an order of magnitude. Today: ~163 indexed rows. Threshold to revisit: ~5k. Not in scope here.
Monotonicity WARN logs are forensic-only. They appear in the import job's log stream after every canonical re-import. If they ever fire in production, the curator opens the canonical xlsx and fixes the data. No alerting, no PagerDuty. That is exactly the right operational posture for a curated dataset.
Concern — barely worth flagging
The
node_modulescache key in CI (actions/cache@v4) doesn't need a bump for this PR —package-lock.jsonis unchanged — but if the regeneratedapi.tsever triggers a TypeScript compile that takes minutes to validate, that's the place to look. Not asking for any change here, just naming it in case CI gets weird.LGTM from the platform side. Ship it whenever the application-side reviewers are done.
📐 Elicit — Senior Requirements Engineer
Verdict: ⚠️ Approved with concerns
Issue #689 is one of the densest, most rigorous specs I've reviewed on this project — it would be a strong example to point new contributors at. The two-policies-two-layers framing (layout = strict rank anchor, data =
preferHuman) is exactly the kind of distinction that prevents three-month-later rework. The AC blocks (Schema, Importer, Layout, UI, Validation, Docs, Contrast) are MoSCoW-clean Must-haves with measurable outcomes. Out-of-scope is explicit and generous.Requirements coverage — what mapped, what didn't
I walked every AC checkbox against the diff. Result: all 27 ACs covered in code or test, including the trickier ones:
load_succeeds_andLeavesGenerationNull_whenArtifactHasNoGenerationColumncovers itIf preferHuman gains explicit-null-vs-unset semantics, delete this test" comment present (mergeCanonical_overwrites_human_null_with_canonical_value_documenting_known_limitation`)page.server.spec.tsfor both/persons/newand/persons/[id]/editassertgeneration: 0survives the round-tripstrict-seed override: imported generation pins rank even when parent edges imply deeper)RelationshipService.getFamilyNetwork()— not the repositorynode.generation, not the post-normalise rank<rect aria-hidden="true">, meaningful<g role="text" aria-label="Generation N">min-h-[44px]on the new<select>untrack(() => ...)initialisation pattern matchesselectedTypecreatePerson(PersonUpdateDTO)writesgeneration(controller test verifies the create path)stammbaum_generationsremoved from all three localesConcerns
Three AC checkboxes are not actually checked off in the PR description's Test plan. The Test plan reads as "[ ] mvnw test clean (CI will run...)" etc. — every box still empty. CI status isn't in the PR body. For a 13-commit feature PR this is reviewable but not yet demonstrably complete. Either tick the boxes after CI passes or add a Co-authored "CI green" comment.
"Contrast verification (manual, not axe)" — no evidence on the PR. Three contrast checks (
G3on light canvas,G3on dark canvas, gutter stripe vs canvas) are explicit Must-haves and the issue body specifically excludes them from axe automation. I have no way to verify these from the diff alone. Recommend: attach a screenshot or contrast-tool output (e.g. Stark, axe DevTools manual mode) as a PR comment."Spot-check that the gutter disappears on a phone-width viewport (≤ 320 px)" — no manual evidence. There's a unit test asserting
matchMediafalse → no labels, which I'll accept as "the JavaScript branches correctly" — but no screenshot of the actual rendered tree at 320 px. The Svelte test is necessary coverage, not sufficient — it doesn't catch a CSS regression where the gutter rect still renders despitegutterWidth === 0. Recommend Leonie sign off on a 320 px screenshot.AC: "
db-relationships.pumlre-verified (no FK change — likely no diff)" is in the issue but not surfaced in the PR body or commits. As a requirements artifact, "verified, no diff needed" should leave an artifact (a Co-authored comment, a checked AC). Otherwise the next person doesn't know if it was deliberately skipped or forgotten.Out-of-scope policing — clean
I re-read the Out-of-scope list against the diff and found zero scope creep. No filter chip resurrection, no semantic generation names, no median-birth-year-on-gutter, no
birthYear/deathYearspread idiom fix, nopreferHumanrework. The PR did exactly what was scoped, no more.Backlog hygiene
When this lands, file the follow-ups the issue carved out:
preferHumanexplicit-null-vs-unset semantics (when the limitation produces a real user-visible bug)birthYear/deathYearconditional-spread idiom fixLGTM as a requirements-met delivery. Tick the boxes, attach the contrast and 320 px proofs, then merge.
🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Read the full diff against the OWASP Top 10 plus the project-specific concerns (mass assignment, SSRF, permission boundary leaks, log injection, validation bypass). Findings: nothing exploitable, nothing risky enough to block. Some defense-in-depth observations follow.
What I checked
Mass assignment (CWE-915) on the new
generationfield. The DTO addsgenerationand the service writes it. There's no@JsonIgnoreexposure of the field via a different route (e.g. a controller that takes a Person directly). ThePersonUpdateDTOroute is the only write path from HTTP, and@RequirePermission(Permission.WRITE_ALL)already guards it (checked the existingPersonControllerannotations in tests).provisional,personType, andfamilyMembercontinue to use their dedicated, permission-guarded endpoints. No new bypass.Input validation.
@Min(0) @Max(10)onPersonUpdateDTO.generationenforces the range at the controller boundary via Bean Validation.MethodArgumentNotValidException→ErrorCode.VALIDATION_ERRORis handled by the existingGlobalExceptionHandler(confirmed by the newupdatePerson_returns400_whenGenerationAboveRangeand_BelowRangetests asserting the JSON$.code). No rawRuntimeException, noResponseStatusExceptionfor domain errors in the new code.Database-layer defense.
V70addsCHECK (generation IS NULL OR generation BETWEEN 0 AND 10). So even if the DTO validation ever gets weakened, the database holds the line. Belt + suspenders, exactly what I want. The CHECK explicitly permits NULL, so the form-clear path works.Importer regex.
^\s*G?\s*(-?\d+)is anchored at the start but intentionally not at the end (the comment explains: tolerates trailing curator commentary like"G 2 de Gruyter").Integer.parseIntof a string captured by\d+is safe — noNumberFormatExceptionpath; max digit count is unbounded which could in principle parse9999999999→ throws — but the range guard< 0 || > 10catches any sane numeric and the parameterised test covers it. Tiny smell, not a bug: an attacker-controlled xlsx with"G 99999999999999999999"would throw aNumberFormatExceptionbefore reaching the range check. The importer runs as a trusted batch (admin-triggered, source artifact under SCM), so the threat model is "data curator pastes weird Excel cell" not "attacker uploads xlsx". Acceptable. If you want belt + suspenders, wrapparseIntin try/catch and treat the exception as "skip + warn".Log injection (CWE-117). All new
log.warn(...)andlog.debug(...)calls use SLF4J{}placeholders, never string concatenation.Logger.warn("Skipping out-of-range generation '{}' for row {}", raw, personId)does not evaluate JNDI lookups in the parameters. Clean.SVG content / XSS. The new gutter SVG renders
G{row.label}whererow.labelis anumber | nullpopulated fromnode.generation(which isIntegerin the backend,number | nullin TS). No user-controlled string flows into SVG attributes. Svelte's{}expressions are escaped by default — even ifrow.labelwere ever a string it'd be text-encoded. Safe.Server-only data flow.
+page.server.ts(both/persons/newand/persons/[id]/edit) doesformData.get('generation')and posts toapi.PUT/POST. There's nofetch('/api/persons/...')inonMountor client-side code. Auth cookie flow unchanged.Permission boundary tests. I checked: existing
PersonControllerTestalready hasWITHOUT WRITE_ALL → 403coverage; the new tests add WRITE_ALL-authenticated 400/200 cases. They do not weaken any existing permission assertion. Good — many features regress permission tests by adding new methods that lack@WithMockUser(authorities = "WRITE_ALL").PII / data exposure.
generationis a small integer per family member. No new sensitive surface. TheRelationshipControllergetNetworkendpoint already requiresREAD_ALL; the new field rides along with the existing PersonNodeDTO authorisation envelope.Non-blocker for the next pass
If you ever revisit the regex, anchor it
^\s*G?\s*(-?\d{1,4})\s*(cap digit count, optionally anchor at end). Belt + suspenders against future weird-cell inputs. Not blocking this PR.LGTM. Standards met, regression tests for the validator stay in the suite, no new attack surface.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The test coverage on this PR is the strongest I've seen on the project in a while. Every layer of the pyramid is represented — parameterised unit tests for the regex, repository integration via Testcontainers, service-level unit + behaviour pinning, slice tests at the controller, in-process form-action tests, browser-based component tests for the dropdown and the gutter. Naming reads as sentences. Factory helpers reused. AAA structure clean. I want to be picky on a few CI-shaped risks below; none are merge blockers.
What I love
load_parsesGeneration_perRegexis the textbook parameterised test. 10 cases, one per regex branch,@CsvSourcewithnullValues = "null". When this test fails in CI the developer sees exactly which input broke. Compare to thetestParse()anti-pattern that runs 10 asserts in sequence.mergeCanonical_overwrites_human_null_with_canonical_value_documenting_known_limitation. Name documents the behaviour AND the exit condition (the inline commentIf preferHuman gains explicit-null-vs-unset semantics, delete this test). This is exactly how to handle a tracked-limitation regression test without it rotting into noise.buildLayout.test.ts— Herbert Cram is named in the test. That regression has a story, and the story is in the test. Future maintainers will know why these four cases exist.expect(body).toHaveProperty('generation', 0)— not "is defined", not "matches some object" — the literal0. Same fornull. That is the failing-test-first pattern done right.entityManager.flush()+clear()between save and find so the assertion crosses a real Postgres round-trip, not a Hibernate first-level cache hit. Catches column-mapping bugs.Concerns (none are merge blockers)
PersonControllerTest.updatePerson_returns200_whenGenerationNulldoes not assert the response body has"generation": null. It checksstatus().isOk()and stops. That passes if the controller accepts the request but silently drops the field somewhere. Add.andExpect(jsonPath("$.generation").isEmpty())or similar. Compare to the in-range test which does assert the value:.andExpect(jsonPath("$.generation").value(3)). Asymmetric coverage.No "full-clear round-trip" integration test against a real DB. The AC explicitly says: "PUT
{"generation": null}after a prior{"generation": 3}reads back null from the DB." The repository test covers null-save + null-read in isolation; the service test covers clear-via-DTO. But nothing exercises the full PUT → service → DB → GET path end-to-end with a real Postgres clearing a previously-set value.@SpringBootTestwith Testcontainers would close this. Not catastrophic — the unit + slice tests catch most of it — but the AC has an explicit checkbox that's not technically met by what's in the diff.StammbaumTree.svelte.test.ts:683-696mockswindow.matchMedia— needed for the JSDOM/browser-test environment to simulate a narrow viewport. The mock is shaped right (matches: false, all the listener methods are no-ops). But it's brittle: if Svelte starts relying onmatchMedia.removeEventListenerreturning anything specific, the mock breaks silently. Considervi.stubGlobalto make the cleanup automatic, and assert the cleanup ran. Tiny.PersonImportUpsertTest.mergeCanonical_overwrites_human_null_with_canonical_value_documenting_known_limitationwould benefit from an assertion on alog.warnevent — to prove the limitation is visible during operation, not silent. Not strictly necessary but it strengthens the documenting nature of the test.No visual regression test for the gutter at 320 px and at 1440 px. Component-level unit tests cover the DOM branches. Playwright visual regression at the route level (
/stammbaumviewport ∈ {320, 768, 1440}) would catch a CSS regression where the rect renders with wrong width or the labels overlap a node. I know vitest-browser is the project's preferred component test stack and full Playwright visual-regression is heavier — flag for the Playwright workflow when the team gets to it.CI evidence not yet in the PR body. Test plan checkboxes are still all unticked. Once CI is green, tick them — that's the contract.
LGTM checklist passed
personOf,node,parentEdge,spouseEdge,makeFormData)@Disabled, noThread.sleep, no flakiness fingerprintsMark the body checkboxes after CI, address the asymmetric assertion in #1, and this is shippable.
🎨 Leonie Voss — UI/UX & Accessibility Lead
Verdict: ⚠️ Approved with concerns
This is a thoughtful brand- and a11y-aware implementation. The gutter is the right pattern (decorative stripe + meaningful label, separately handled for screen readers), the dropdown ships at-spec on touch targets (
min-h-[44px]), the i18n carries the meaning of G 0 ("älteste Generation"), and the spec HTML was updated alongside the code. The thing I am genuinely happy to see:<g role="text" aria-label="Generation N">so VoiceOver/NVDA announce "Generation three" instead of "G three". That detail was named in the issue body and it's present in the code. Many implementations would have dropped it.Concerns
Contrast verification (Critical → upgrade to evidence-required). The AC explicitly lists three manual contrast checks (
var(--c-ink-2)G3 label on light canvas, same on dark canvas, gutter stripe vs canvas). No screenshot or contrast-tool output is on the PR. The colours look right on paper:--c-ink-2 = #4b5563on--c-canvas(sand). I get ~6.6:1 — AAA for normal text.--c-ink-2 = #9ca3afon#010e1e(canvas dark). I get ~5.9:1 — AA for normal text.rgba(161, 220, 216, 0.08)light /0.14dark — decorative, WCAG 1.4.11 ≥3:1 is the bar for non-text contrast, and decorative carve-outs are allowed for purely aesthetic underlays. The comment inlayout.css:163-166("~1.04:1 vs canvas — decorative carve-out") names this explicitly.But "trust my math" is not the project standard. Attach a contrast-tool screenshot (Stark, axe DevTools manual, or
https://webaim.org/resources/contrastchecker/) for each of the three checks, ideally as a PR comment.No 320 px screenshot. The gutter is hidden below
md(a 100 px column would consume 30% of a 320 px viewport — correct call). The Svelte test verifies thematchMediaJS branch. But I want to see the rendered tree at 320 px and confirm:width=0still rendering somewhere).This is a 30-second Playwright shot or a manual screenshot. Don't skip it for a UI PR.
Trade-off needs to be visible to users. The Out-of-scope says "tree readers on phones do not see G{n} labels. Deliberate trade-off." Fine — but the senior audience (60+) and the millennial audience both use phones. If a phone reader wonders "how do I know which generation Walter is in?", we've stripped a meaningful affordance. Two thoughts:
G3text, no stripes) might fit even at 320 px. Out of scope here; capture as a follow-up if user signal appears.Hint text under the dropdown — colour check.
text-xs text-ink-3≈ 12 px.--c-ink-3is the lightest text token. Verify the hint clears AA on light AND dark. The persona's standing rule: 16 px minimum body, 12 px floor for ANY visible text. 12 px AA at the lightest ink shade is right at the edge — make sure it's not below 4.5:1 in dark mode (which historically has thinner contrast headroom for--c-ink-3).Touch target chain — dropdown wins, but its hint is unselectable. The
<select>itself ismin-h-[44px]✓. The label and hint are read-only text, so no target concern. Good. (Aside: the issue body called out that the other form inputs are below 44 px today and that's a known gap, intentionally not fixed here. Agree — but file it as a separate issue with the fix bundled across the form.)Spec HTML — visual mockup not updated. Out-of-scope per the issue body ("still show pre-gutter design; visual refresh is a separate concern"). The impl-ref tables ARE updated, which is the authoritative source. I accept the call, but flag for future hygiene: the spec HTML is increasingly the source of truth for "what should this look like" — drift between mockup and table will cost a future contributor a confused hour.
Things I am specifically pleased with
aria-hidden="true"on the stripe<rect>. A screen reader doesn't announce "rectangle" 7 times per scroll. Often missed.var(--c-ink-2)chosen over--c-ink-3for the gutter label. The comment instammbaum-tree-spec.htmlnotes "--c-ink-3fails AA dark." That's a hand-picked token, not a default.font-family: var(--font-sans)for the gutter label. Brand-correct — labels and UI chrome are Montserrat (sans), body and titles are Tinos (serif). The gutterG3is chrome.Bottom line
Implementation is right. Approve once you attach the contrast screenshots and a 320 px shot, and file the "generation in side panel on phone" follow-up.
CI flagged two browser tests: - "renders a G{n} label per occupied generation row …" - "wraps the visible G3 text inside an aria-labelled group …" Both queried g[role="text"] and got an empty array. Root cause: isMdOrUp was initialised to false and only flipped to true inside a $effect — but $effect runs after the first render, so the test's post-render DOM scan saw the pre-effect (gutter-absent) state. Seed the rune synchronously from window.matchMedia(...).matches when window is available; SSR still picks the false branch and hydrates without a layout flash. The effect now only attaches the change listener for subsequent resizes. Refs #689 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Review round 1 — addressed
Pushed three fix-up commits + one CI fix:
516a0a38GENERATION_MIN/MAXintoPersonGeneration(DTO + both importers + V70 comment all read from it)61ca5a6ef124529eisMdOrUpwas initialised to false and only flipped inside$effect(post-render), so the test saw the pre-effect DOMc0b500b6)PR body updated with explicit carve-outs (db-relationships.puml unchanged, visual mockup SVGs out-of-scope, mobile gutter trade-off → follow-up #691).
Contrast verification (Leonie + Elicit AC)
Computed via WCAG 2.x relative-luminance formula against the actual canvas colours from
layout.css:#f0efe9#4b5563(--c-ink-2)#010e1e#9ca3af(--c-ink-2)#f0efe9#e9ede7(mint @ 8 % over canvas)#010e1e#172a38(mint @ 14 % over canvas)Stripe carve-out: WCAG 1.4.11 applies to "user interface components" carrying meaning. The stripe is
aria-hidden, purely visual flavour to band the rows; the meaningful affordance is the labelled<g role="text" aria-label="Generation N">adjacent to it, which clears AA comfortably in both modes. Raising the opacity to hit 3:1 would force the stripe to compete with node fills (the comment on--c-gutter-stripealready documents this trade-off). Recommend keeping 8 % / 14 % and dropping the 3:1 bullet from the AC, or open a follow-up if you want a louder banding choice.Follow-ups
node.generationper-node in the side panel for phone readers (Leonie's "mobile loses the affordance entirely" concern).Remaining test-plan checkboxes
Test-plan ticks left for after CI runs green on the fix-up commits + a manual smoke pass on the dev stack. The CI fail was a flake of my own making (the
$effect-vs-render-order bug), not a regression — fixed inf124529eand pushed.