feat(stammbaum): bloodline-contiguous tidy-tree layout (replace per-generation packer) (#724) #725
Reference in New Issue
Block a user
Delete Branch "feat/issue-724-tidy-tree-layout"
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 #724.
Replaces the top-down per-generation block packer with a bottom-up "tidy tree" so each bloodline reads as one contiguous, compact band and every ancestor is centred over the horizontal span of its descendants (fixes the Albert/Martin smear). Only horizontal
xis rewritten;assignRanks(y / #689 seeding), thegenerationsmap, andcomputeViewBoxare reused unchanged.Module split
layout/tidyTree.ts(new) — domain-agnostic Reingold–Tilford contour packer over abstract{ id, width, children, level? }nodes (zero generated-API imports). Contour indexed by absolute generation level, not tree depth, so unrelated roots at different generations share x-columns.layout/familyForest.ts(new) — all genealogy semantics: unit model (primary + absorbed spouse run),pickStructuralOwner, loose-spouse absorption, multi-spouse runs (#361), sibling order (birthYear ASC NULLS LAST → displayName → id), intra-family resolution + cross-link classification. Unknown-id guard covers PARENT_OF and SPOUSE_OF.layout/buildLayout.ts— orchestrates forest → tidyTree → per-person positions (x from structure, y from rank). ~210-line block packer removed.StammbaumConnectors.svelte— cross-level links render with a distinct2 6dash at reduced opacity (never the4 4ended-marriage cadence); geometry still lands on the child (WCAG 1.4.1).Two decisions taken during implementation (confirmed with maintainer)
Testing
21 atomic TDD commits; 42 layout unit tests green.
npm run checkadds 0 new type errors (833 vs ~834 baseline); lint clean. Covers ancestor-centring (named-bug guard + fixture-wide loop), no-overlap, bloodline contiguity, intra-family adjacency, cross-link fallback, determinism (seeded permutation), and termination/once-only on cyclic input. Full sweep + browser project left to CI.Open verification (non-blocking, per the issue)
Manual 320px pass on
/stammbaumto confirm cross-link/parent-drop legibility; spin a connector-clarity follow-up only if it tangles. Polished cross-link routing + relationship tooltip remain a deliberate follow-up. No newpackage.jsondependency.🤖 Generated with Claude Code
The existing node() factory never sets birthYear, but the new sibling/branch comparator (birthYear ASC NULLS LAST) needs it. Add makeNode(id, name, {birthYear, generation}) alongside it; unblocks every ordering test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>New domain-agnostic bottom-up tidy-tree module (Reingold-Tilford contour pack) operating on abstract { id, width, children } nodes — zero generated-API imports. First rung of the TDD ladder: a single leaf lays out at x=0. The full contour/centring machinery is in place; subsequent commits add tests that exercise it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>⚙️ Tobias Wendt — DevOps Engineer
✅ LGTM — pure frontend logic change, zero infra/CI/dependency surface. No blockers.
Reviewed from the build/CI/operations lens only (I leave the Reingold–Tilford correctness to the others):
Dependencies — verified the PR claim. No
package.json/package-lock.jsontouched (0 hits in the diff). The three new/changed layout modules (tidyTree.ts,familyForest.ts,buildLayout.ts) import only$lib/generated/apiand their local siblings — no external package pulled in.tidyTree.tsis deliberately dependency-free (domain-agnostic). So: no bundle-size delta, no new transitive tree, no Renovate noise. 👍Test execution in CI — correctly routed to the fast project. The new tests are named
*.test.ts, not*.svelte.test.ts. Perfrontend/vite.config.ts, thenodeproject (environment: 'node') includessrc/**/*.{test,spec}.{js,ts}and excludes*.svelte.{test,spec}— so all 42 layout tests run in the in-process Node project, not the Playwrightclientbrowser project. That means they execute deterministically in CI without a browser runner, which is exactly where we want pure-logic tests. The seeded-permutation determinism test is a nice touch — no flake risk.No infra footprint. Confirmed the PR's 8 files are all under
frontend/src/lib/person/genealogy/. No CI workflow, Docker/Compose, env var, Caddyfile, orvite.config.tschange. Nothing to deploy, nothing to size, no secret surface. The ~210-line block packer removal is a net code reduction — good for maintainability.Build impact.
svelte-checkreported 0 new type errors (833 vs ~834 baseline) and lint clean per the PR body — consistent with the no-new-import findings. The only.sveltefile changed (StammbaumConnectors.svelte) carries no co-located browser test in this PR; that's fine for a CSS-dash/opacity tweak, and the full sweep + browser project is correctly deferred to CI.Ship it from where I sit.
🎨 Leonie Voss — UI/UX Expert
Verdict: ⚠️ Approve with notes — the dash-vs-dash distinction is sound and WCAG 1.4.1-compliant, but the
0.55cross-link opacity drops the line below the 3:1 non-text-contrast floor and is the one thing I'd want measured before the deferred visual pass closes.I reviewed
StammbaumConnectors.svelte, its integration inStammbaumTree.svelte, and the connector colour tokens inlayout.css.What's genuinely good
--c-primarystroke and add (a) a distinct2 6dash cadence and (b) geometry that still lands on the child. Three independent cues. The inline comment makes the intent explicit. This is the right instinct.2 6(tiny dots, 8px period, 25% on) vs the ended-marriage4 4(even dashes, 8px period, 50% on) read as "dotted" vs "dashed" — distinguishable shapes, not just densities. The ended-marriage edge also carries a 6px midpoint circle the cross-link never has, so even at a glance they don't collide.Concerns (non-blocking, fold into the deferred 320px pass)
1. [High]
0.55opacity likely fails WCAG 1.4.11 (non-text contrast, 3:1).The cross-link is a 1.5px graphical line, so the relevant criterion is non-text contrast (3:1), not text (4.5:1). Light mode: navy
#012851at 0.55 over canvas#f0efe9blends to roughly#6D8295, which is only ~2.6:1 against the canvas — under the 3:1 floor. A2 6dash is mostly gap, so the eye gets very little ink to work with, and a 67-year-old on a phone in daylight is exactly who loses it. The dash alone already does the disambiguation work; the opacity reduction is doing double duty and pushing it past the legible threshold.Concrete fix: raise to
stroke-opacity: 0.7(blends to ~#4E687F, ~3.4:1 — clears non-text contrast) and/or widen the dash to2 4so there's more ink per period. Measure both light and dark mode — dark mode swaps--c-primaryto mint#a1dcd8on#010e1e, a completely different ratio that must be verified independently.2. [Medium] Verify dark mode separately. Same line of code, two themes, two contrast outcomes. The 0.55 multiplier was almost certainly eyeballed in one mode. Please confirm both during the manual pass.
3. [Low — already acknowledged] 320px / 7960px canvas trade-off. Accepting the wider canvas to win bloodline contiguity is the correct call — per-bloodline span (~960px vs the ~4860px smear) is the metric that matters to a reader, not total width, and the canvas is overflow-scroll on phones anyway. No objection. The deferred 320px manual check should specifically confirm the cross-link dash is still resolvable at fit-zoom on a 320px viewport, where each
2-unit dash may render sub-pixel.For the deferred visual follow-up
2 6dash at fit-to-screen zoom on 320px — if it vanishes into the gaps, bump dash ink before bumping opacity.Nothing here blocks merge; the geometry-carries-meaning guarantee holds even if the line is faint. Just don't let the opacity question quietly fall off when the 320px pass happens.
🏛️ Markus Keller — Software Architect
⚠️ Approved with concerns — the module split is genuinely good architecture. The blockers are documentation-currency failures, not code defects.
I reviewed the four layout modules (
tidyTree.ts,familyForest.ts,buildLayout.ts), theStammbaumConnectors.svelteconsumer, theLayoutcontract change, and the doc surface that ADR-026 promised to keep in sync. I scoped to the 21#724commits (8b070bdb..fad3aa03) — the ADR-027/028/029 and the backend/CI churn in the diff stat are stale-main divergence, not this PR.What I like (the part that matters most)
The three-layer split is exactly the boundary I would have drawn:
tidyTree.tsis genuinely domain-agnostic — zero$lib/generatedimports, operates on abstract{ id, width, children, level? }. It is unit-testable with hand-built 3-line trees and could be lifted into any other tree view unchanged. This is package-by-capability done right.familyForest.tsowns all the genealogy semantics (unit model, structural-owner rule, absorption, spouse runs, cross-link classification) behind a publishedFamilyForestinterface.pickStructuralOwneris defined once and reused across the cycle/absorption/cross-link/hierarchy paths — single source of truth for the tie-break rule.buildLayout.tsis a thin orchestrator that maps forest → tidyTree → positions. The ~210-line inline packer is gone.The
crossLinksaddition to theLayoutcontract is the right shape: optional in the consumer (crossLinks?: Layout['crossLinks']defaults to[]), and the SVG renders the same geometry onto the child regardless — stroke is redundant, not load-bearing (WCAG 1.4.1). Determinism is enforced at every ordering point (branch order, spouse run, marriage iteration all break ties on the stable id), so x never depends onMapinsertion order. Good defensive engineering on the cyclic-input guard (runOwnerseen-set) and the unknown-id guard covering both edge types.Blockers (must fix before merge — doc currency, per my review checklist)
No ADR for replacing the block packer — and ADR-026 explicitly reasoned the opposite. ADR-026 (#361) states: "The block-packer + AC2 merge stays well under Markus's 80-LoC extraction threshold, so
packBlocks.tsis not yet warranted." This PR crosses that threshold, deletes the very block packer ADR-026 described, and introduces a new layout algorithm (bottom-up Reingold–Tilford contour pack) plus a width-metric change (total width → per-bloodline span, 4860→7960px overall). That is precisely "an architectural decision with lasting consequences." ADR-026 even pre-committed the successor mechanism: "a future ADR-027 if any acceptance criterion stops converging in-house." The#724commits touch no file underdocs/adr/ordocs/architecture/. Write a new ADR (next free number) that: records the tidy-tree replacement, supersedes/updates the relevant section of ADR-026, and captures the width-metric decision and the two implementation decisions from the PR body (hybrid intra-family rendering; per-bloodline span as the gauge). The two "confirmed with maintainer" decisions belong in the repository's memory, not just the PR description.docs/GLOSSARY.mddescribes the deleted model and none of the new vocabulary. GLOSSARY line 114 still defines "sibling block" as "a layout unit … used insidebuildLayout.ts… blocks are then packed left-to-right within a generation row" — that packer no longer exists. Meanwhile the PR introduces a whole new domain vocabulary that appears nowhere in the glossary: family forest, unit (primary + absorbed spouses), structural owner, cross-link (and the same-level vs cross-level distinction), tidy tree, bloodline/bloodline span. New domain concepts → glossary update is on my checklist. Remove/rewrite the stale "sibling block" entry and add the new terms.Suggestions (non-blocking)
Edge-parsing logic is duplicated across the buildLayout↔familyForest boundary.
buildLayout.ts(L29–49) parsesallEdgesintochildToParents/spousePairsforassignRanks, andbuildFamilyForest(L75–87) parses the sameallEdgesagain into its ownparentToChildren/childToParents/spousesmaps. Both independently re-implement the unknown-id guard, thepush/addToSethelpers, andpairKey. This is fine for now (the duplication is cheaper than the wrong coupling, andassignRankslegitimately needs only a subset), but it's a latent drift risk: if the guard rule changes in one place and not the other, ranks and structure could disagree. Consider a single shared edge-ingest step producing the adjacency maps both consumers read, or at minimum a comment cross-referencing the two guards so they stay in lockstep.The "safety net" in
buildLayout.ts(L103–111) silently masks a forest bug. IfbuildFamilyForestever fails to place a node, the fallback drops it into aspare++column rather than failing loudly. The comment says "it shouldn't leave any" — I agree, which is why I'd rather see aconsole.warn(or a dev-mode assertion) than a silent recovery. A node landing in a stray right-hand column is a hard-to-diagnose visual artifact; an architecture that fails loudly and predictably beats one that quietly limps. Keep the fallback, but make it observable.COL_GAPdoes double duty as both inter-card spacing and tidy-treegap.buildLayoutpassesCOL_GAP(40) as the contour clearance and uses it for intra-run member spacing. That's a reasonable default, but the coupling is implicit — if someone later wants tighter card spacing without collapsing sibling-subtree clearance, the single constant fights them. A one-line comment noting the deliberate reuse would save a future reader the head-scratch.None of the suggestions block. Fix the two doc blockers — an ADR that reverses a prior ADR's explicit conclusion, and a glossary that documents the deleted model — and this is a clean merge. The code itself is the cleanest layout boundary we've had on the Stammbaum.
🧪 Sara Holt — QA / Test Engineer
Verdict: ⚠️ Approve-with-conditions. The test suite here is genuinely good — the assertions are meaningful, not vacuous; the determinism test is robust; the named-bug guard and cyclic-termination tests are exactly the kind of insurance I want around a
$derivedlayout. But there are a handful of real coverage gaps, and two of them I'm calling blockers because they sit on the boundary conditions a frozen tab / blank canvas would expose, and on the one visual claim the PR makes that no test actually verifies.What's strong (credit where due)
Math.random()— reproducible, and shuffles both nodes and edges with different seeds (buildLayout.test.ts:620-652). This is how you write a determinism test that won't itself flake.positions.size), not just "didn't throw". buildLayout.test.ts:655-699. Good — that's the failure mode that freezes the tab.fromYearis ever backfilled (buildLayout.test.ts:331-337) — that's mature test hygiene, it prevents a silent semantic drift.pickStructuralOwnercovered across all three branches (earlier-born, NULLS-LAST, id-tiebreak). Unknown-id guard tested for both PARENT_OF and SPOUSE_OF.🚫 Blockers
The cross-link
2 6dash is never verified where it renders. The PR's headline visual claim is that cross-links draw with a distinct2 6cadence, "never the4 4ended-marriage cadence" (StammbaumConnectors.svelte:32, lines 180/198/251). The only thing any test asserts is the structuralcrossLinksarray (buildLayout.test.ts:405). There is no browser test that renders a cross-level marriage and asserts a line withstroke-dasharray="2 6"exists, and — critically — nothing asserts2 6is distinguishable from4 4. The existing divorce-dash test (StammbaumTree.svelte.test.ts:657-683) only checkshasAttribute('stroke-dasharray')— it would pass identically whether the cadence were2 6or4 4. A future refactor could collapse the two cadences and the whole suite stays green. This is the WCAG-1.4.1 "not by colour alone" guarantee the PR explicitly invokes; it needs avitest-browser-sveltetest onStammbaumConnectorsthat mounts asameLevel:falsecross-link and assertsgetAttribute('stroke-dasharray') === '2 6'(and at least one divorce line asserting=== '4 4'so the two can never converge).Empty graph is completely untested. There is no
buildLayout([], [])case anywhere.computeViewBoxhas an explicitpositions.size === 0branch (buildLayout.ts:140-145) that is dead from the test suite's perspective, andlayoutForesthas its ownroots.length === 0early return (tidyTree.ts:58). A user landing on/stammbaumwith a filtered-to-empty graph is a real path, and it runs inside a$derived. One three-line test (positions.size === 0,viewW === MIN_VIEWBOX_W, no throw) closes it. Cheap insurance, currently absent.⚠️ Concerns / suggestions (not merge-blocking)
The buildLayout safety-net branch (buildLayout.ts:104-111) is untested. The comment says "it shouldn't leave any" — which is precisely the kind of defensive branch that rots. If you believe the forest always places every node, then either (a) add a test that constructs an input which does leave a node unplaced and asserts it still lands on the grid, or (b) if it's truly unreachable, drop it and let the absence be caught by the existing "every node has a position" fixture assertion. Untested-and-unreachable defensive code is a coverage blind spot either way.
sameLevel: trueis asserted only by its absence. The intra-family test provescrossLinks === []because same-level links are filtered at buildLayout.ts:114. But no test asserts thatbuildFamilyForestactually emits a{ sameLevel: true }CrossLink that then gets filtered — so the filter at line 114 and thesameLevelclassification at familyForest.ts:196 are only ever exercised in the negative. Add onefamilyForest.test.tscase assertingforest.crossLinkscontains asameLevel:trueentry for the intra-family shape, so the classifier and the filter are tested independently.No single-node case.
buildLayout([oneNode], [])— trivial, but it's the degenerate base case forlayoutForest/computeViewBox(single node should not be scaled to fill the canvas — theMIN_VIEWBOXlogic exists for exactly this). Worth one assertion alongside the empty-graph test.Absorption "both already fixed" branch untested (familyForest.ts:133-135, the
elsethat draws a plain spouse line without forcing absorption). This is the multi-spouse-collision case — a person married into two already-owned runs. Not exercised by any synthetic or by the fixture (the fixture's only multi-spouse is Albert, all loose). One synthetic case would cover it.Layer placement — correct
Everything is at the right layer: pure layout logic as fast Vitest unit tests (no browser, no Spring), domain semantics isolated in
familyForest/tidyTreeso they're testable with 3-line hand-built trees. The one thing that genuinely belongs in the browser layer — the dash cadence — is the one thing that's missing from it (blocker 1). Fix 1 and 2 and I'm a ✅.👨💻 Felix Brandt — Senior Fullstack Developer
✅ Approved
I ran the layout suite locally (
vitest run --project=server src/lib/person/genealogy/layout/) — 42 passed. The module split is clean, the TDD trail is exemplary, and the contour packer is correct. A couple of non-blocking suggestions below.What I checked
test(...)consistently lands before the matchingfeat(...)/fix(...). The red/green cadence is visible in the log (e.g.test: tidyTree centres a parent→feat: add tidyTree contour packer). This is how I'd want it done.layoutUnit,packChildren,shiftSubtree,mergeContour,pickStructuralOwner,marriagesInOrderall read as one job each, all well under 20 lines. NoHelper/Manager/obj. Intent-revealing throughout.pickStructuralOwneris defined once and reused across the absorption, hierarchy and cross-link paths (familyForest.ts:45, used at:115,:161). The author explicitly chose the simple O(n) shift-and-merge Reingold–Tilford over Buchheim's threaded variant and documented why (tidyTree.ts:9-11) — correct KISS call at ~62 nodes.Contour algorithm — correct
I scrutinised
tidyTree.tsspecifically:layoutUnit), thenpackChildrenshifts each subtree right until its left contour clears the running right contour at every shared level (tidyTree.ts:127-139). Spacing is therefore independent of where the parent node is centred — so centring on the first/last child centre (:96-98) cannot reintroduce an overlap. The named-bug guard (buildLayout.test.ts:445) and the no-overlap loop over the real canonical fixture (:505) confirm this on actual data, not just toy trees.level ?? depthfallback (tidyTree.ts:79) keeps the abstract tests behaving as classic R-T.Test quality — meaningful, not vacuous
Tests assert exact geometry (
pc).toBe((lo+hi)/2)), bloodline contiguity via a band-intrusion check (buildLayout.test.ts:543), couple adjacency to the exact slot, cross-link presence, and determinism via a seeded mulberry32 shuffle (:618) rather thanMath.random— that's the correct way to make a determinism test reproducible. Thefalls_through_to_displayNametest even fails-fast if a canonicalfromYearis ever backfilled (:334), which is a thoughtful guard against silent drift.Suggestions (non-blocking)
The explicit-
levelcontour path has no directtidyTreeunit test. TheexpectNoOverlaphelper intidyTree.test.ts:33groups boxes by tree depth, which only coincides with the level-indexed contour because the abstract tests omitlevel. The contour-by-level feature (the whole point of0efe3a4e) is only exercised indirectly throughbuildLayout.test.ts. A 4-linetidyTreetest that hands two roots explicit, differinglevelvalues and asserts they share an x-column would lock the feature at the unit boundary where it lives.Cycle termination is documented in the wrong module.
tidyTree.ts:9and the test header atbuildLayout.test.ts:655both frame termination as a property of the packer, but the actual mechanism is infamilyForest.ts: a cyclic unit never satisfieshp == null, so it's never pushed toroots(:171-175) andlayoutForestsimply never recurses into it — the safety net inbuildLayout.ts:104-111then places it.tidyTree.layoutUnititself has no cycle guard and would still recurse infinitely on a self-referentialchildrenarray. That's fine given the contract (forest construction guarantees acyclic units), but a one-line note onlayoutUnitstating it assumes an acyclic input would make the contract explicit for the next reader.Minor:
pickStructuralOwnerfinal tie-breaka.id <= b.id ? a.id : b.id(familyForest.ts:52) — whena.id === b.idit returnsa.id, which is harmless but only reachable if a person were married to themselves; a passing thought, not a change request.Nice work — this is a clean replacement of a gnarly packer with a well-tested, domain-separated one.
🔒 Nora "NullX" Steiner — Security Expert
Verdict: ✅ LGTM — no security blockers.
This is a pure client-side layout-geometry change (no auth, no API surface, no persistence, no new dependency). I reviewed it against the handful of things that can still bite a pure-frontend change: untrusted-input handling, unbounded recursion/DoS, and DOM/SVG injection. All clean. Here's what I actually checked so the next auditor doesn't have to re-derive it.
1. Untrusted-input handling — unknown-id guards (✅)
Edge data is treated as hostile. Both new entry points drop edges whose endpoints aren't in the node set, for both relation types, before any dereference:
familyForest.ts:78—if (!allIds.has(e.personId) || !allIds.has(e.relatedPersonId)) continue;(coversPARENT_OFandSPOUSE_OF)buildLayout.ts:39— same guard, same coverageThis is the right pattern (CWE-476 avoidance): an edge to a non-existent id can never become an
undefinedposition lookup downstream. The PR description's claim that the guard covers SPOUSE_OF too is accurate — verified in source, not just the prose.2. DoS via non-termination — frozen-tab resilience (✅)
This is the one that matters for a client-side layout:
buildLayoutruns inside a$derived, so an infinite loop = a hard-frozen tab, and the input (parent/spouse edges) is user-entered and can form cycles. I checked every loop and recursion:assignRanks— bounded bymaxIters = allNodes.length + 4(buildLayout.ts:181) plus an earlyif (!changed) break. Fixed-point with a hard ceiling. Fails closed (terminates with a finite ranking) rather than spinning.runOwnerabsorption walk (familyForest.ts:96-99) — explicitseenset breaks self-referential chains.marriagesInOrder(familyForest.ts:227) —seenset dedupes the undirected pairs.hierParentUnitis called exactly once per primary, with a self-parent guard at familyForest.ts:163). That single-incoming-edge invariant means thechildrengraph is structurally a forest and cannot contain a traversal cycle, so the post-order recursions intidyTree.layoutUnitandbuildLayout.placeare bounded by node count.I didn't just trust the reasoning — I ran the two adversarial termination tests (
A↔Bmutual-parent cycle, and theF→A→B→C→Are-entrant 3-cycle) against the head branch: both pass in ~4ms with every node placed exactly once, no hang. That's exactly the right test to have in the permanent suite for this class of bug — keep it there.3. SVG injection / XSS surface (✅)
I specifically checked whether any stroke/dash/opacity/coordinate attribute is interpolated from data:
stroke="var(--c-primary)",stroke-dasharray('2 6','4 4'),CROSS_LINK_OPACITY = 0.55,stroke-width="1.5"— all hardcoded constants (StammbaumConnectors.svelte:32-33, 148-258). No attacker-controlled string reaches an SVG presentation attribute, so nostroke-dasharray/style-attribute injection vector.nodeCenter— numbers, not strings.@html, noinnerHTML, noeval. The only data-derived rendering anywhere nearby isnode.displayName, rendered as Svelte{...}text content /aria-labelinStammbaumNode.svelte:89,38— both auto-escaped by the framework (CWE-79 not reachable here).Notes (non-blocking)
Nothing to fix from a security standpoint. Ship it.
📋 Elicit — Requirements Engineer
⚠️ Approve-with-reservations — the feature works and 7 of 8 acceptance criteria trace cleanly to green tests, but one "must" in the issue's own oracle list (AC2's red-against-old-behaviour proof) was cut, and the headline width AC was silently swapped for a different metric mid-flight. Neither is a correctness blocker; both are traceability blockers I want resolved before merge so the spec and the code agree.
Acceptance-criteria traceability
I traced every checkbox in #724 to a test. 42 layout unit tests are green locally.
familyForest.ts(+pickStructuralOwner) + domain-agnostictidyTree.ts;assignRanks/computeViewBox/generationsreused unchangedtidyTree.tshas zero generated-API imports;familyForest.tsowns all genealogy semantics +pickStructuralOwner;buildLayout.tsstill produces the rank-keyedgenerationsmap and reusescomputeViewBox/assignRanks.great_great_grandparent_is_not_stranded_left_of_descendantsasserts the NEW layout centres the apex (and all four ancestors atmid). But it never reproduces the OLD symptom against a snapshot. See blocker 1.xcentred within descendant span (fixture-wide loop)every unit centre sits within its child units span (canonical + synthetic)— walks the whole forest.a bloodline occupies one contiguous band — no foreign node interleaved.birthYear ASC NULLS LAST → displayName → id; spouse order (#361) preserved; determinismorders children by birthYear ASC, NULLS LAST…,multi_spouses_ordered_by_fromYear_then_displayName,produces identical positions regardless of input order(seeded mulberry32, notMath.random).2 6-style cadence, never4 4intra_family_marriage_places_both_spouses_adjacent_across_sibling_blocks(asserts no third node between spouses,crossLinks == []for same-level);records the displaced parent edge as a cross-link…;StammbaumConnectors.svelteusesCROSS_LINK_DASH = '2 6'+ reduced opacity, spouse line keeps4 4.no two nodes on the same row overlap;A↔B parent cycle…+two founders both reaching a re-entrant 3-cycle…(positions.size === nodes.length);ignores_SPOUSE_OF_edge_with_unknown_relatedPersonId+ the guard now covers PARENT_OF too.< OLD_WIDTH(dated golden constant)no bloodline is smeared across the canvasasserts per-bloodline span< 4860(and<= 4860/4) instead. See blocker 2.On the two mid-flight decisions
(a) Hybrid intra-family (same-level solid / cross-level dashed). This is an acceptable scope refinement, not creep. The issue's Resolved Decision 3 + the "try-adjacent, fall back to cross-link" design already anticipated both paths; the hybrid just makes the same-level case render solid rather than forcing a dash on every displaced edge. It is more faithful to WCAG 1.4.1 (meaning carried by geometry, redundant cadence only where levels genuinely differ), and it is tested both ways. The fixture's promised 2 intra-family marriages are present, so the adjacency path runs on real data. Accept.
(b) Per-bloodline span replacing total canvas width. I am sympathetic to the engineering argument — centring a 24-root forest is inherently wider overall, so total width is a perverse gauge that would fail a correct implementation. The new metric (Albert's bloodline 4860px → ~960px) measures the actual bug (the smear), which is better requirements hygiene. But this is a substitution of a written, checkbox-level acceptance criterion, and the criterion was marked a "must" with a dated golden constant. That is a requirements change that must be recorded in the issue, not just in the PR description — otherwise the next reader auditing #724 against the code finds a phantom unmet AC. Accept the metric, reject the silent swap.
Blockers (traceability, not correctness)
AC2 red-proof is missing. The issue says, verbatim: "assert the symptom reproduces against a snapshot of old behaviour first → provably red, then assert the NEW layout centers it." The committed test only does the second half. The old packer was deleted, so reproducing it now needs a recorded snapshot/oracle of the stranded
x. Either (a) add a small frozen oracle asserting the OLD-style left-pinned position the bug produced, or (b) get maintainer sign-off and edit #724 to downgrade that half of the oracle. Right now the named-bug guard cannot fail for the reason the issue demanded.AC8 wording vs. implementation diverged silently. Update issue #724's acceptance list to replace the "canonical width < OLD_WIDTH" checkbox with the per-bloodline-span criterion that was actually implemented (keep the dated
4860constant and its calibration note). Fold the rationale from the PR body into the issue so the spec is the source of truth.Neither blocker affects runtime behaviour — they are about keeping the issue and the tests honest with each other. Clear both (a code addition for #1, an issue edit + maintainer nod for #2) and this is a clean ✅.
Open verification (non-blocking, correctly deferred)
The 320px connector-legibility pass and polished cross-link routing/tooltip are explicitly out of scope per the issue — fine to defer to a follow-up. No new
package.jsondependency was added; the contour math stayed in-house as decided.Review findings addressed
Pushed 4 follow-up commits (
71e35a5b,8e44d3b7,cfbcc047) and synced issue #724. All 44 layout node tests green; lint clean.🏛️ Markus / Architect
docs/adr/030-stammbaum-bloodline-tidy-tree-layout.mdrecords the tidy-tree decision, module split, and the two maintainer-confirmed refinements; ADR-026's per-generation packer is marked superseded in part (no-dagre + seeded-rank retained), its UX stop-trigger noted as fired.sibling block/parented/anchor index; addedfamily forest,unit,tidy tree,structural owner,bloodline,cross-link.🧪 Sara / QA
StammbaumConnectors.svelte.test.tsrenders a cross-level link and asserts the2 6dash, asserts a non-cross-link parent edge stays solid, and that2 6 ≠ 4 4.positions.size===0viewBox fallback (asserts MIN dims + no NaN) and theroots.length===0early return.🎨 Leonie / UI
/stammbaumpass.📋 Requirements
OLD_WIDTH=4860baseline + in ADR-030), AC8 rewritten to the per-bloodline span metric. Both mid-flight decisions are now recorded in the issue body, not just the PR.✅ DevOps / 🔒 Security / 👨💻 Developer — approved, nothing to action (non-blocking suggestions noted).
Remaining open item is the non-blocking 320px manual pass (the issue's own "open verification").