Let the relationships modal's size follow the node's data (closes #2385)

[malibio]

Closes #2385.

The problem

The panel rendered one full section per schema-declared relationship, so its size tracked the schema rather than the node's data. An adr node with seven declared relationships and no edges rendered seven headers all reading "0 items", seven identical "+ Add" buttons, and nothing that was actually data — most overwhelming exactly when it had least to say.

What changed

Empty types no longer get sections. Only groups with edges render as sections. Empty outbound relationships collapse behind a single "Add relationship" chooser; empty inbound groups are dropped entirely, since they have no Add of their own to justify standing open. partitionGroups owns that rule in the pure module so it is unit-testable rather than inline in the template.

Inbound groups are read-only. An inbound group is the same physical edge seen from the other end, declared on and owned by the other node's schema. Offering Add there misrepresented ownership, silently inverted the picker's semantics (pick a source, not a target), and could not work at all when the other type doesn't declare the forward relationship. They now carry an incoming · read-only badge and a dashed rule — distinguishable without reading the arrow direction — and their rows navigate to the node that owns the edge, where it can be edited properly.

Controls are per row, not behind a panel-wide mode. editMode is deleted, along with the Done/Close footer — the dialog's ✕ is the single dismiss control. Edge values render read-only in their columns at all times; ✏️ opens an editor for that one edge and appears only where the schema declares edge_fields, so a relationship with none shows ❌ alone rather than an editor with nothing to edit. Editing one row leaves every other row — and the panel behind it — untouched.

Why a dialog for edge properties, rather than inline cells or a popover: each relationship declares its own edge_fields, so the form is per-relationship and variable in length, and it needs room to grow for the collection↔person RBAC fields. Its input helpers moved to edge-field-input.ts so the add form and the row editor render a given field type identically.

Verification

Driven in dev:browser against an isolated daemon (NODESPACE_HOME pointed at a throwaway copy, per ADR-053), using an adr schema with four outbound relationships — one carrying edge_fields — and two reverse names:

• Seven declared groups, zero edges → 0 sections, 1 Add control, no scrolling
• Add from the chooser stages the declared edge fields, persists, and the section appears
• ✏️ opens scoped to one row, pre-filled; Save persists and closes; panel behind unchanged
• depends_on (no edge_fields) shows ❌ and no ✏️
• Inbound Superseded By shows no Add/edit/remove; its row navigates to the owning ADR
• Exactly one dismiss control

Tests: 233 files / 4886 passing (baseline 232 / 4863 — 23 new tests covering the partition rule, the edit-affordance predicate, and the edge-field input helpers). Lint and svelte-check clean.

Notes for review

• The modal is still ~1080 lines. The issue cited 1044 as evidence of structural weight, but what remains is mostly the per-group view-settings popover (~85 lines of columns/sort/filter), which is unrelated to this issue. Splitting it out would be a reasonable follow-up.
• window.confirm on removing a required relationship's last edge is unchanged, but is now reachable without first entering an edit mode — same behavior, more exposure.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

Pragmatic Code Review — PR #2426

Review Type: Initial Review
Scope: origin/main...HEAD — 1 feature commit, 6 files (+863 / −320)
Issue: #2385 — Relationships modal renders a full section per declared type

---

Summary

This is a strong, well-reasoned change and a clear net positive. The core insight — the panel's size should track the node's DATA, not its schema's declared relationship count — is correct, and it is implemented in the right place: partitionGroups is a pure, unit-tested function rather than an inline .filter() buried in the template. Deleting the modal-wide editMode in favour of per-row controls removes a genuine state-machine (three dismiss controls, a mode flag that gated both remove and edit, a Done button whose meaning depended on the flag) and replaces it with a simpler one.

The commit message is exemplary — it explains why inbound is read-only (ownership, picker inversion, cross-type failure modes) rather than restating the diff. The extraction of edge-field-input.ts is well-motivated: the add form and the row editor previously duplicated the type→input mapping, and DRY here is real duplication of knowledge, not incidental similarity.

Findings below are two 🟡 correctness/lifecycle issues worth addressing and several 🟢 observations. Nothing is a blocker.

---

Requirements Check

Against the acceptance criteria in issue #2385:

#	Criterion	Status
1	A node with no relationships does not render one section per declared type	✅ partitionGroups — populated requires rows.length > 0
2	Adding a first edge reachable in at most one extra interaction	✅ Chooser popover → type → picker. One extra click vs. before
3	Per-row remove without a separate edit mode	✅ 📁 relationship-viewer-modal.svelte:822-831 (table), :869-877 (chips)
4	Per-row edit of edge properties preserved (RBAC prerequisite)	✅ EdgePropertiesModal, wired at :1072-1084
5	Editing one row does not switch the whole modal into a mode	✅ editing is a single-row {group,row} handle; panel behind is untouched
6	Rows for relationships declaring no edge_fields show no edit control	✅ groupSupportsEdgeEditing gates :810-821
7	Current property values visible without entering edit mode	✅ :796-804 — value columns always render read-only
8	Inbound groups read-only — no Add, no edit, no remove	✅ hasControls = !inbound :758; Add gated by {#if !inbound} :884
9	Inbound visually distinguishable at a glance, not only by arrow	✅ dashed left border :633 + "incoming · read-only" badge :651-656
10	Inbound rows navigate to the owning node	✅ navigateToOwner :276-279, wired at :782-791 and :858-865
11	Empty inbound groups follow the same collapsing rule	✅ partitionGroups excludes them from both lists; test in relationship-grouping.test.ts covers it explicitly
12	Exactly one dismiss control remains	✅ Dialog.Footer/Close and the Edit/Done header button both deleted; the built-in ✕ is sole
13	Modal fits without scrolling for the ADR benchmark	✅ 0 sections + 1 Add control; verified manually and by the benchmark unit test
14	Immediate persistence, per-row drafts survive another row's add, error surfacing	⚠️ Mostly — see 🟡 #1. confirmAdd correctly closes only its own picker and runMutation's docblock states the invariant, but the new editing handle introduces a staleness path the old code did not have

14 / 14 addressed, one with a caveat.

---

Code Review Findings

🟡 Important: editing holds pre-reload object references, so a concurrent mutation can silently strand the open editor

📁 packages/desktop-app/src/lib/components/relationships/relationship-viewer-modal.svelte:143, :1072-1084

let editing = $state<{ group: RelationshipGroupView; row: RelationshipRowView } | null>(null);

editing stores the object identities from the view that was current when ✏️ was clicked. Every mutation routes through runMutation → reloadAfterMutation, which does view = result — a wholly new object graph. The editing handle is never reconciled against it.

Two consequences:

1. Stale displayed values. valueFor={(name) => currentEdgeValue(target.group, target.row, name)} (:1078) falls through to row.edgeValues[fieldName] when no draft covers the field. After a reload, target.row is the old row object, so the editor shows pre-reload stored values while the table behind it shows post-reload ones.
2. A save against a removed edge. If the row being edited is deleted by another path while the editor is open, editing still points at it; saveRow then calls updateEdgeProperties for an edge that no longer exists. That surfaces as a daemon error rather than data corruption — so this is a UX/robustness issue, not a data-integrity one — but the failure is opaque.

In practice the window is narrow: busy disables the row controls and both EdgePropertiesModal buttons, and the editor is modal over the panel, so a user cannot easily trigger a competing mutation. That is why this is 🟡 and not 🔴. But the safety is incidental — it rests on the overlay, not on an invariant the code states.

Recommendation: key editing by identity rather than by reference — store { groupKey, rowId } and derive the live group/row from view, closing the editor when the lookup misses:

let editingKey = $state<{ groupKey: string; rowId: string } | null>(null);
const editing = $derived.by(() => {
  if (!editingKey) return null;
  const group = view?.groups.find((g) => g.key === editingKey.groupKey);
  const row = group?.rows.find((r) => r.id === editingKey.rowId);
  return group && row ? { group, row } : null;
});

This makes {#if editing} self-healing: a reload that removes the row closes the editor rather than leaving it pointed at a ghost, and displayed values always track the live view. It also makes the {@const target = editing} capture at :1073 unconditionally correct.

Engineering Principle: Single source of truth. Derived UI state should reference authoritative state by identity, not snapshot it. A snapshot creates a second source of truth that can silently diverge from the first.

---

🟡 Important: addGroup has the same staleness, and its stale rows weakens the duplicate-target filter

📁 packages/desktop-app/src/lib/components/relationships/relationship-viewer-modal.svelte:148, :326, :912-913

{#if addGroup && addableGroups.some((g) => g.key === addGroup?.key)}
  {@render targetPicker(addGroup)}

The membership test here is by key, which is correct and deliberate — good. But addGroup itself is still the pre-reload object, and the picker's own logic reads through it:

• :326 — const existing = new Set(group.rows.map((r) => r.id)) filters already-linked targets out of search results using the stale rows. After a failed add (the path where the picker stays open), the search list can still offer a node that a prior successful add already linked.
• :971 — {#each group.edgeFields ...} renders from stale edgeFields. Benign in practice, since edgeFields comes from the schema and does not change under a relationship mutation.

There is also a subtler interaction worth noting: on a successful add from the chooser, the group moves from addable to populated. confirmAdd calls closeAdd() so the picker is dismissed — correct — but had it not, :912's membership test would have evaluated false and the picker would have vanished mid-interaction with no explanation. The current behaviour is right; it is right for a reason that is not stated anywhere. A one-line comment at :912 noting that the membership test is what keeps a group's picker from surviving its promotion into populated would protect that.

Recommendation: same fix shape as #1 — hold addGroupKey: string | null and derive the live group. That resolves :326's stale-rows filter as a side effect.

Engineering Principle: Single source of truth again — this is the same defect class in a second place, which is itself the argument for fixing it structurally rather than case by case.

---

🟢 Suggestion: losing free-text editing of undeclared edge keys is the right call, but it is an undocumented capability removal

📁 packages/desktop-app/src/lib/components/relationships/relationship-viewer-modal.svelte:796-804 (was :820-832 on main)

The old editMode had a branch rendering an <Input type="text"> for an edge column with no matching edgeFields entry — i.e. ad-hoc keys present on stored edges but never declared. The new code renders those read-only, because groupSupportsEdgeEditing requires edgeFields.length > 0.

I agree with the removal. An undeclared key has no type, so the editor could only ever guess text; saving one through saveRow's wholesale {...row.edgeValues, ...draft} replace would coerce a stored number or boolean into a string. That is a real data-shape bug the old code carried. Dropping it is a fix, not a regression.

But note the asymmetry it leaves: groupEdgeColumns (relationship-grouping.ts:167-188) still surfaces ad-hoc keys as columns, and the test at relationship-grouping.test.ts ("is false for ad-hoc edge keys…") correctly pins layout === 'table' alongside groupSupportsEdgeEditing === false. So ad-hoc edge data is visible-but-unreachable through the UI — which is defensible, since the CLI's --edge-data is the tool that wrote it. Worth a sentence in groupSupportsEdgeEditing's docblock making that the stated design rather than a consequence. The docblock already says "offers no editor, since there is no field definition to render an input from" — adding "and the values remain visible as read-only columns" closes the loop for the next reader.

Engineering Principle: Make the design decision legible. A deliberate capability removal that a reader could mistake for an oversight should say so where they will look.

---

🟢 Suggestion: formatEdgeFieldLabel vs humanizeName — the duplication is warranted, but nearly invisible

📁 packages/desktop-app/src/lib/services/edge-field-input.ts:84-86 and relationship-grouping.ts:119

export function formatEdgeFieldLabel(name: string): string {
  return name.replace(/[_-]+/g, ' ');
}

humanizeName does a fuller job (title-casing); formatEdgeFieldLabel deliberately does not, because every call site pairs it with the capitalize Tailwind class (:89, :974). So the two are not redundant — they target different rendering contexts.

That said, the distinction rests entirely on a CSS class sitting in a different file, and the name formatEdgeFieldLabel does not hint at it. A reader doing a DRY sweep would plausibly "consolidate" these and produce Due By where Due by was intended. Suggest the docblock state the coupling: "Lower-case by design — call sites apply capitalize, so title-casing here would double-capitalize."

Engineering Principle: DRY applies to knowledge, not to character sequences. Two functions that look alike but encode different decisions should say what separates them.

---

🟢 Suggestion: inbound chip rows nest an interactive <button> inside a title-bearing <span>

📁 packages/desktop-app/src/lib/components/relationships/relationship-viewer-modal.svelte:853-865

The chip wrapper <span> carries title={row.nodeType} and the inner <button> repeats row.nodeType as visible text. The title is therefore redundant on inbound chips and, on some screen readers, is announced as the button's accessible description — the type gets read twice.

Semantics and keyboard behaviour are otherwise fine: real <button type="button"> elements, so Tab/Enter/Space work natively, and the same holds for the table variant at :782-791 (a button inside a <td> is valid). No role/tabindex hacks anywhere — good.

Two small polish items:

• Drop title on the inbound branch, or move it to the outbound-only branch where it is the sole carrier of the type.
• The navigating buttons have no accessible name beyond their text; aria-label={Open ${row.label}} would make the action explicit, since "Ada / person" alone does not convey that activating it navigates. Compare the explicit aria-labels already present on the ✏️/❌ controls (:816, :827) — the navigation affordance deserves the same treatment.

Engineering Principle: An accessible name should describe the action, not just the object.

---

🟢 Observation: EdgePropertiesModal's lifecycle is sound — no stuck-open or double-close hazard

📁 packages/desktop-app/src/lib/components/relationships/edge-properties-modal.svelte:68-75, relationship-viewer-modal.svelte:1072-1084

I looked specifically for the failure modes flagged for scrutiny and did not find them:

• open={true} non-bindable + {#if editing} mount guard is the right pattern. There is exactly one source of truth for "which row is open" (editing), and the comment at :62-67 says so. A second open flag could disagree with editing; this arrangement makes that unrepresentable.
• No double-close. onOpenChange fires onCancel only for !next. cancelEdit (:468-470) guards on if (!editing) return, so a second invocation is a no-op — and the {#if} unmounts the component before Bits UI could emit a duplicate anyway.
• busy guards a save in flight correctly. Both footer buttons are disabled={busy} (:126-127), and runMutation sets busy = true for the whole mutate + reload window with a finally reset — so an in-flight save cannot be double-submitted or cancelled out from under itself. ✕/Esc/overlay are not blocked by busy, but dismissing mid-save routes to cancelEdit, which only drops the draft; the already-issued updateEdgeProperties still completes and reloadAfterMutation still runs. That is benign.
• {@const target = editing} at :1073 captures correctly. Inside {#if editing}, Svelte 5 re-evaluates the block when editing changes, so target is never stale relative to editing. The staleness in 🟡 Set up Tauri + Svelte project structure #1 is editing vs. view, not target vs. editing.
• saveRow's ordering is right: clearRowDraft then editing = null, both gated on ok. On failure the editor stays open with the draft intact (:237-238 comment), which is the correct retry affordance.

🟢 Observation: partitionGroups expresses exactly the stated rule

📁 packages/desktop-app/src/lib/services/relationship-grouping.ts:290-296

populated: groups.filter((group) => group.rows.length > 0),
addable: groups.filter((group) => group.direction === 'out' && group.rows.length === 0)

populated is direction-agnostic, so an inbound group with rows still renders (read-only) — which is criterion #10's prerequisite. Only empty inbound groups fall out of both lists. That is precisely the specified semantics, and the two filters are disjoint and jointly exhaustive over outbound groups. The test "gives a section only to groups that have edges, in either direction" pins the direction-agnostic half explicitly, which is the subtle part.

Nit: the two .filter() passes could be one reduce, but at the size of a schema's relationship list that would trade clarity for nothing measurable. YAGNI — leave it.

---

Testing Assessment

23 new tests, 0 regressions (233/4886 vs. a 232/4863 baseline). The new tests are well-targeted:

• partitionGroups coverage is genuinely behavioural, not tautological — it pins direction-agnostic populated, empty-outbound folding, empty-inbound dropping, and the mixed case. The "renders the six-declared-relationships-no-edges case" test encodes the issue's own benchmark as an executable assertion, which is exactly the right instinct: the acceptance criterion now has a regression guard rather than a manual check.
• groupSupportsEdgeEditing covers all four quadrants (outbound+fields, outbound+none, inbound+fields, ad-hoc-only). The ad-hoc case asserting layout === 'table' alongside the false result is a nice touch — it proves the group still renders, so the test would catch a fix that accidentally hid the group instead of just its editor.
• toDateTimeLocalString's round-trip test ('2026-03-04T09:30' → itself) is the one that matters most, since a save→reload drift bug there would be silent and cumulative.

Gap worth flagging 🟢: there is no component-level test for the modal, and the two 🟡 findings above live precisely in the layer the pure-module tests cannot reach — editing/addGroup lifecycle across a reload. Both the draft-isolation invariant (criterion #14) and the editor-close-on-save path are asserted today only by manual dev:browser verification.

I would not block on this, and I would not ask for a broad component suite — the project has no such test for this modal today, and adding one is a larger commitment than this PR should carry. But if the lifecycle fix in 🟡 #1 is made, a focused test is then cheap and high-value, because the $derived version is testable without mounting: extract resolveEditing(view, editingKey) into relationship-grouping.ts and assert that a removed row yields null. That converts an untestable component concern into a pure-function one — the same move this PR already made twice, applied once more.

---

Standards Compliance

• ✅ createLogger throughout; no raw console.*
• ✅ No any; unknown used correctly for edge values and narrowed at the boundaries
• ✅ No {@html}
• ✅ No lint suppressions; eslint + svelte-check clean (0/0)
• ✅ No backward-compat shims, dead-code allowances, or migration paths — the old editMode was deleted, not deprecated, and the stale issue reference in relationship-grouping.ts's header docblock was removed rather than left to rot. Both are exactly right for greenfield.
• ✅ No issue numbers introduced into code comments (the one at relationship-grouping.ts:1 was removed by this PR; the pre-existing #1918 reference at :122 is untouched and out of scope)
• ✅ New pure modules kept free of Svelte/DOM imports and explicitly documented as such — good separation of concerns, and the reason the tests are as cheap as they are

---

Recommendation

✅ APPROVE

The change is a clear net improvement in code health and delivers all 14 acceptance criteria. The architecture is right: pure decisions live in testable modules, the component keeps only orchestration, and the deleted editMode removes more complexity than the new EdgePropertiesModal adds.

The two 🟡 findings share one root cause — snapshotting view objects into editing/addGroup instead of referencing them by key — and are both narrow in practice because busy and the modal overlay close the window. They are worth a follow-up rather than a merge block, ideally as one fix since a single $derived-by-key pattern resolves both. The 🟢 items are optional polish.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review feedback addressed — commit 6aac7786

6 of 6 recommendations addressed; 0 skipped.

🟡 #1 + #2 — one root cause, one fix

Both findings were the same bug wearing two hats: editing and addGroup captured view objects, while every mutation reloads view into a wholly new object graph. Anything outliving a mutation was therefore rendering — and writing from — a pre-reload snapshot.

The modal now holds editingKey / addGroupKey and derives the live group and row through two new resolvers in the pure module:

• findGroupByKey(groups, key) — relationship-grouping.ts
• findRowByKey(groups, groupKey, rowId) — returns null when either half is gone

Consequences: the editor always shows post-reload values; it collapses to null (closing itself) once a reload reveals the edge is gone; and runSearch now re-reads the group after its await, so the duplicate-target filter can no longer offer an already-linked node from stale rows.

Putting the resolvers in the pure module also answers the review's testing note — the staleness lived in the one layer pure-module tests couldn't reach. 7 new tests cover it, including:

• a resolver returns the post-reload row, asserted by identity (not.toBe the pre-reload object), not just by value
• a deleted row resolves to null
• same-named groups facing opposite directions never cross-resolve

Also documented the picker hand-off the review flagged as working "for an unstated reason": once a group's first edge lands it moves addable → populated, and confirmAdd's closeAdd() means that hand-off is never seen mid-interaction — it matters only on the failure path, where the picker stays put.

🟢 #3 — ad-hoc edge keys

Documented in groupSupportsEdgeEditing why these are read-only rather than free-text editable: an undeclared key has no type, and the wholesale-replace update path would coerce a stored number or boolean into a string on first save. groupEdgeColumns still surfaces them as read-only columns, so the data stays visible; declaring the field on the schema is how it becomes editable.

🟢 #4 — label helper coupling

Documented why formatEdgeFieldLabel deliberately is not humanizeName: callers pair it with a capitalize CSS class, so routing through humanizeName would render Due By where Due by is intended. The near-duplication is the point.

🟢 #5 — inbound row a11y

Dropped the redundant title that duplicated visible type text, and gave both navigating controls an action-describing accessible name — "Open <node> to edit this relationship" — matching the explicit labels already on ✏️/❌.

---

One correction to the original framing

I'd described the keyed resolver as making the editor "self-close if the edge disappears." Verification showed that is true on reload, not on any deletion. Deleting an edge out of band and then hitting Save still fails at the daemon, with the error surfaced and the editor kept open with its draft — which is correct, and is the pre-existing "error surfacing / drafts not lost" requirement. Worth stating precisely rather than overclaiming.

Verification

Driven in dev:browser against an isolated daemon, with two edges in one group (the case that proves per-row isolation):

• Editing row 2 changed only row 2 — in the UI and in storage; row 1 untouched
• Editor resolved to the correct row's values (ADR-3, not ADR-2)
• Deleting an edge out of band while its editor was open → save fails cleanly with Relationship 'supersedes' ... does not exist surfaced, no resurrection, and the row drops on the next reload
• Cancel closes without disturbing the panel behind it
• Inbound row announces "Open ADR-1 Use libsql for local storage to edit this relationship"; still read-only

Tests: 233 files / 4893 passing (was 4886 — +7 resolver tests, no regressions). eslint + svelte-check 0/0. Full pre-push gate passed.

Re-review

Requested. The 🟡 fix is an architectural change to how the modal tracks open-editor/open-picker state — object references → keys plus derived resolution — touching the state declarations, four writers, runSearch's async path and two template call sites. Worth confirming there's no reactivity gap in the derived resolution.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2426 (Re-Review)

Review Type: Re-Review
Scope: git diff 3feaab30..6aac7786 — 1 commit, 4 files (+230 / −32)
Prior review: comment IC_kwDOPYIGPs8AAAABTDaNgw (verdict APPROVE, 2 🟡 + 4 🟢)

---

Previous Review Summary

• Number of previous recommendations: 6 (2 🟡 correctness findings, 3 actionable 🟢 suggestions, 1 🟢 testing note)
• Addressed: 6
• Not addressed: 0
• New issues found: 3 (1 🟡, 2 🟢)

---

Verification of Previous Findings

#	Finding	Status	Evidence
🟡 1	editing snapshots pre-reload objects	✅ Addressed	editingKey (:151) + $derived resolution (:179-181); {@const target} removed, call sites read editing.group/editing.row directly (:1103-1108)
🟡 2	addGroup same staleness; stale rows weakens duplicate filter	✅ Addressed	addGroupKey (:156) + $derived (:182); runSearch re-reads the group after its await (:565-567)
🟢 3	Ad-hoc edge keys read-only is undocumented	✅ Addressed	relationship-grouping.ts:253-266 — states the type-less/wholesale-replace coercion rationale and that groupEdgeColumns keeps the values visible
🟢 4	formatEdgeFieldLabel vs humanizeName coupling invisible	✅ Addressed	edge-field-input.ts:83-92. Claim verified: the capitalize class is confirmed present at edge-properties-modal.svelte:89 and relationship-viewer-modal.svelte:1003. The docblock is factually correct, not aspirational
🟢 5	Inbound chip redundant title + no action-describing name	✅ Addressed	title dropped from the chip wrapper (:875-878); aria-label="Open {row.label} to edit this relationship" on both navigating controls (:807, :883)
🟢 6	Testing gap — lifecycle untestable inside the component	✅ Addressed structurally	Resolvers moved into the pure module; 7 tests added. This is precisely the move the note asked for

The 🟢 #5 resolution went slightly beyond the suggestion: title was dropped from both chip branches rather than moved to the outbound one. That is correct — row.nodeType renders as visible text in both branches, so the attribute was redundant on outbound too.

---

Scrutiny Items — Assessment

Reactivity of the $derived resolution: sound

📁 relationship-viewer-modal.svelte:179-182

No reactivity gap and no loop hazard. Both deriveds read only view.groups and their key state, and neither writes reactive state, so there is no $derived → $state → $derived cycle. The resolvers are Array.prototype.find over a list the size of a schema's relationship count — re-running per render costs nothing measurable.

On the narrowing question specifically: {#if editing} followed by editing.group is safe, and {@const target = editing} was correctly removable. svelte-check confirms it (I re-ran quality:check: 2501 files, 0 errors, 0 warnings), and at runtime Svelte 5 re-reads the derived getter, so the props track the live view rather than a capture. Removing the {@const} also dissolves the prior review's residual worry that the capture could drift from view — there is now no capture at all.

EdgePropertiesModal does not remount across a truthy→truthy editing transition, so a reload while the editor is open updates props in place: focus and cursor position survive, and valueFor re-resolves to post-reload edgeValues with the draft still layered over it. That is exactly the intended behaviour.

runSearch guard order: correct

📁 relationship-viewer-modal.svelte:548-574

The order is right, and deliberately so: requestedKey is captured as a string, and the post-await staleness check compares addGroupKey !== requestedKey — key against key, never object against object. This matters for the promotion case: when a group's first edge lands and it moves addable → populated, the group object is replaced but its key is unchanged, so a legitimately in-flight search is not discarded. Comparing objects would have thrown those results away. The if (!current) return then covers the group leaving the schema entirely. There is no window in which addResults is set for the wrong group.

Save-against-deleted-edge behaviour: right, and correctly re-characterised

The main agent's correction to its own framing is accurate and worth crediting: the keyed resolver self-closes the editor on reload, not on any out-of-band deletion. A Save against an already-deleted edge still fails at the daemon, surfaces the error, and keeps the editor open with its draft.

That is the correct behaviour, for the reason saveRow's comment gives (:470-471): the user typed something, and the retry affordance must not eat it. Auto-closing on the failure would discard work in response to a condition the user cannot see. The narrower claim is also the honest one — the resolver can only close when the view says the edge is gone, which is the only moment the client actually knows.

Resolver placement in relationship-grouping.ts: acceptable, mild tension

The module header documents it as pure grouping/normalization, and findGroupByKey/findRowByKey are lookups rather than grouping decisions. But they are pure, they own the key/id identity scheme that buildGroupView (:210) mints, and co-locating a key's producer with its consumer is the stronger cohesion argument. A third module for 20 lines would be YAGNI. Leave them.

The 7 new tests: they pin what they claim, with one caveat

📁 relationship-grouping.test.ts:435-545

Genuinely behavioural, not tautological:

• The identity assertion is constructed correctly. before and after come from separate buildRelationshipsView calls with a differing reason value, and the test asserts both resolved?.row.edgeValues.reason === 'edited' and not.toBe(before[0].rows[0]). A value-only assertion would pass against a snapshot resolver whenever the values happened to coincide; not.toBe closes that. expect(resolved?.group).toBe(after[0]) is the stronger half — it pins which object comes back, not merely which one doesn't.
• The deleted-row → null case is the direct regression guard for the self-closing editor.
• The opposite-direction test is the best of the seven: asserting outbound.key !== inbound.key and both cross-resolutions returning null pins the key scheme itself (direction:name:targetType), so a future change dropping direction from the key fails here rather than silently cross-resolving.

Caveat 🟢: the tests pin the resolvers; nothing pins that the modal uses them, or that editingKey is what's stored. That is the untestable-without-mounting residue — and the new 🟡 below lives in exactly that residue, which is why it is worth naming.

---

New Findings

🟡 Important: a self-closed editor leaves editingKey set, so re-adding the same target silently re-opens it with the stale draft

📁 relationship-viewer-modal.svelte:179-181, :482-486, :1101

The new design makes editing self-heal, but only in the read direction. Nothing reconciles editingKey when its resolution collapses to null:

• cancelEdit clears it — but a self-close never routes through cancelEdit. When editing goes null, the {#if editing} block unmounts EdgePropertiesModal directly; Dialog.Root's onOpenChange does not fire on unmount, so onCancel is never called.
• No $effect watches for the collapse — editingKey is assigned only in resetTransient, saveRow, openEdit, and cancelEdit.

So after a self-close, editingKey is a dangling {groupKey, rowId} and edgeDrafts['<groupKey>::<rowId>'] a dangling draft. Both keys are fully reproducible, which is what turns this from untidiness into behaviour:

• group.key is `${direction}:${relationshipName}:${targetType}` (relationship-grouping.ts:210) — derived from the schema, identical on every load.
• row.id is the target node's id (:204), not an edge id.

Therefore: open the editor on row X → a reload drops X (out-of-band delete, then any mutation) → the editor closes while editingKey still points at X → the user re-adds the same target to the same relationship → confirmAdd's reload rebuilds a row with the same id in a group with the same key → findRowByKey resolves again → the editor pops open unbidden, pre-populated from the stale edgeDrafts entry, over a freshly created edge the user never asked to edit. A subsequent Save writes pre-delete draft values onto the new edge.

This is narrow — it needs a delete-then-recreate of the same pair while the editor was open — and it is not data corruption: every write is a legitimate updateEdgeProperties against a real edge. But it is a new failure mode. The object-reference version could not do this, because a dropped object reference was permanently unresolvable. Keying by reproducible identity is the right fix; it just needs the write side to match the read side.

On the draft specifically: the brief asks whether the un-cleared edgeDrafts entry is a real leak given resetTransient clears all drafts on close. As a memory leak, no. As a correctness leak within a single panel session, yes — the surviving draft is what makes the spurious re-open write stale values rather than merely present an unwanted dialog.

Recommendation: reconcile the key when resolution misses. One effect covers both keys and both symptoms:

// A reload can remove the edge or group an open editor/picker refers to.
// Resolution already collapses to null (closing them); drop the keys too, so a
// later reload cannot resurrect them — group keys and row ids are both derived
// from stable schema/node identity, so re-adding the same pair reproduces them.
$effect(() => {
  if (editingKey && !editing) {
    clearRowDraftByKey(`${editingKey.groupKey}::${editingKey.rowId}`);
    editingKey = null;
  }
  if (addGroupKey && !addGroup) closeAdd();
});

addGroupKey has the identical shape of the bug — a picker whose group leaves the schema keeps its key and would re-open on a later reload — though it is harder still to reach, since a group only leaves on a schema change.

Engineering Principle: Self-healing state must heal in both directions. Deriving the read from authoritative state removes the stale-render bug, but a key outliving its referent is itself a second source of truth. Resolution to null is the signal that the key is dead; something has to act on that signal.

🟢 Suggestion: the new comments overstate what self-closing guarantees

📁 relationship-viewer-modal.svelte:148-150, :176-178, relationship-grouping.ts:294-297

Three comments say the resolver closes the editor "if the edge it points at disappears" / "so an editor left open over a vanished edge closes instead of saving against it". The main agent has already corrected exactly this overclaim — but the correction landed in the PR discussion, and the code is where the next reader looks.

The precise statement is "closes once a reload shows the edge is gone". That distinction is the whole substance of the correction: a Save against an edge deleted out of band does not close — it fails at the daemon and deliberately keeps the editor open with the draft. As written, relationship-grouping.ts:296's "instead of saving against it" reads as a guarantee the code does not make, and a future reader could reasonably delete the daemon-error path as dead code on its strength.

Engineering Principle: A comment that overstates an invariant is worse than none — it invites removal of the very code handling the case the invariant doesn't cover.

🟢 Nit: runSearch's finally and catch act on superseded requests

📁 relationship-viewer-modal.svelte:568-573

Both are pre-existing (this commit only reordered the guards above them); I note them only because the commit put the surrounding lines under review:

• finally { addSearching = false; } runs on the discard path too, so a superseded response clears the spinner belonging to the current picker.
• catch sets addResults = [] unconditionally, so a failed superseded request blanks the current picker's results.

Both self-correct on the next keystroke (200 ms debounce), and neither can produce wrong data — only a briefly wrong spinner or an empty list. Moving both inside the addGroupKey === requestedKey guard would close it. YAGNI territory; noted for completeness, not requested.

---

Verification Assessment: adequate

I re-ran the checks rather than taking them on trust:

• ✅ quality:check (eslint + svelte-check): 2501 files, 0 errors, 0 warnings — independently settles the narrowing question.
• ✅ relationship-grouping.test.ts (32) + edge-field-input.test.ts (14): 46 passed. The +7 accounting checks out.
• ✅ Verified the 🟢 Enhanced BaseNode with Hierarchical Circle Indicators + TextNode Implementation #4 docblock's factual claim (the capitalize class at both call sites), since a docblock asserting a coupling in another file is exactly the kind of claim that rots.

The manual dev:browser protocol is the right one and reaches what unit tests cannot — two edges in one group is the case that actually proves per-row isolation, and checking storage as well as the UI is what makes it meaningful. The out-of-band-deletion scenario was tested and the result reported honestly rather than smoothed over, which is how the overclaim got caught.

The one gap: the protocol stopped at the delete. The new 🟡 needs delete-then-recreate of the same target while the editor is open.

---

Standards Compliance

• ✅ No lint suppressions; 0 errors / 0 warnings
• ✅ No any; string | null resolver params, correctly narrowed
• ✅ New resolvers stay free of Svelte/DOM imports — the pure-module boundary held
• ✅ No backward-compat shims; the object-reference state was deleted, not kept alongside
• ✅ No issue numbers introduced into comments (the pre-existing #1918 at :124 is untouched)
• ✅ Comments explain why (the reload/object-graph rationale), not what — with the one overstatement noted above

---

Recommendation

✅ APPROVE

The commit does what it set out to do, and in the right place: both 🟡 findings had one root cause, and the fix addressed the cause rather than the two symptoms. Moving the resolvers into the pure module — which also answered the prior review's testing note — is the third instance of the same good instinct this PR has shown throughout: push the decision into a testable function and leave the component holding only orchestration.

The new 🟡 is a genuine consequence of the fix — keying by reproducible identity makes a dangling key resurrectable in a way a dangling object reference was not — but it is narrower than the bug it replaced, it cannot corrupt data, and it wants a ~5-line effect rather than a rethink. Net code health is clearly up. Worth a follow-up, ideally paired with the 🟢 comment correction, since both concern the same "what does self-closing actually guarantee" boundary.

Particular credit for the self-correction on the overclaim. Catching that the resolver closes on reload and not on deletion, and saying so unprompted, is the difference between a fix that is understood and one that merely passes.

🤖 Generated with Claude Code

[malibio]

Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review feedback addressed — commit 620d7bd2

3 of 3 addressed (1 🟡, 2 🟢); 0 skipped.

🟡 Self-closed editor left editingKey set

Correctly diagnosed, and a real catch. Resolving by key self-healed in the read direction only: a self-close never routes through cancelEdit (the {#if} unmounts the child directly and onOpenChange doesn't fire on unmount), so editingKey and its edgeDrafts entry dangled — and neither key is unique to an edge (group.key is direction:name:targetType, row.id is the target node's id), so a re-add reproduced a resolvable key.

Two effects now drop the key the moment resolution misses — one for the editor, clearing its draft with it, one for the picker. rowKey is now built from a shared draftKey(groupKey, rowId) so the key-only path and the row-object path can't drift on how a draft is addressed.

🟢 Comments overstated what self-closing guarantees

Fixed in the code, where it belongs — the earlier correction only lived in the PR discussion. Both the findRowByKey docblock and the editingKey comment now state the limit outright: self-closing is a reaction to a reload, not a save-time check, so a save racing an out-of-band delete still reaches the daemon and still relies on its error being surfaced. Explicitly flagged so the daemon-error path isn't later deleted as dead code on the strength of a comment.

🟢 runSearch's catch/finally acting on superseded requests

Fixed rather than deferred — the reviewer called it YAGNI, but it was two lines on code this PR already touched. Both now confirm the response still belongs to the open picker before clearing results or the spinner.

---

Verification — the scenario the previous pass missed

The re-review noted my manual protocol "stopped at the delete, not the recreate." Ran exactly that, in one component instance:

1. Draft typed on row B, editor left open
2. B's edge deleted out of band
3. Row C removed through the modal → in-place reload → editor self-closes here (the effect firing)
4. B's target re-added, reproducing group key + row id
5. Another modal-driven reload

Result: editor did not reopen (dialogCount: 1), GHOST DRAFT appears nowhere in the DOM, and the recreated edge kept RECREATED CLEAN.

One thing that looks like a bug and isn't

While testing I found that keeping the editor open across a delete/re-add and then pressing Save does write the typed draft onto the recreated edge. No reload intervenes, so nothing resolves to null and no effect fires. I judged this correct — pressing Save is asking to write what you typed, and the endpoints are identical so the daemon accepts it — and recorded the reasoning in the commit message so it isn't later read as an oversight. Making it behave otherwise would need a save-time identity check on the edge.

Tests: 233 files / 4895 passing (+2 pinning the reproducible-key fact the fix rests on: a stale key resolves again after a re-add, to a different row object; and a group key survives the empty→populated promotion). eslint + svelte-check 0/0. Full pre-push gate passed.

Re-review

Requested. The fix adds two $effects that write reactive state (editingKey, edgeDrafts) while reading a $derived that depends on it — the classic shape for a reactivity loop. svelte-check is clean and the behaviour verified correctly in the browser, but that's a self-interested judgement on my own change, so it's worth a second pair of eyes rather than an assertion.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2426 (Re-Review, round 2)

Review Type: Re-Review (round 2)
Scope: git diff 6aac7786..620d7bd2 — 1 commit, 3 files (+115 / −9)
Prior reviews: initial (APPROVE, 2 🟡 + 4 🟢) → re-review round 1 (APPROVE, 1 🟡 + 2 🟢)

---

Previous Review Summary

• Number of previous recommendations: 3 (1 🟡, 2 🟢)
• Addressed: 3
• Not addressed: 0
• New issues found: 0 (2 🟢 observations, neither actionable before merge)

---

Verification of Previous Findings

#	Finding	Status	Evidence
🟡 1	Self-closed editor left editingKey set; reproducible keys mean a re-add reopens the editor with a stale draft	✅ Addressed	Two $effects at :193-205 drop the key when resolution misses; the editor's also clears the orphaned draft
🟢 2	Comments overstated what self-closing guarantees	✅ Addressed	relationship-grouping.ts:329-338 and relationship-viewer-modal.svelte:149-152 now state the limit outright
🟢 3	Nit: runSearch's catch/finally acted on superseded requests	✅ Addressed	:605-612 — both branches now check addGroupKey === requestedKey

The 🟡 diagnosis in the round-1 review was correct and the fix targets the actual root cause rather than the symptom. Worth crediting the specific insight: the reason this was a real bug and not a theoretical one is that neither key is unique to an edge — group.key is direction:name:targetType (minted at relationship-grouping.ts:210, confirmed) and row.id is the target node's id. Both halves are reproducible, so "the key will just never resolve again" was never a safe assumption. The fix rests on the right fact.

---

Primary Scrutiny Item — the two new $effects: sound, no loop

📁 relationship-viewer-modal.svelte:193-205

This is the shape that usually deserves suspicion, so I traced the dependency graph edge by edge rather than trusting svelte-check's silence.

Editor effect. Reads editingKey and editing. editing reads editingKey and view. The effect writes editingKey and (via clearDraftKey) edgeDrafts. So editingKey is genuinely both read and written by the same effect — the cycle exists in the graph. It terminates because the write drives the guard monotonically to false:

• Pass 1: editingKey truthy, editing null → write editingKey = null.
• Pass 2 (re-triggered by that write): editingKey is null, so if (editingKey && ...) short-circuits. No write. Fixed point reached.

One pass, one re-run, done — nowhere near effect_update_depth_exceeded (Svelte's threshold is far higher). The write is also idempotent: editingKey = null when already null is a no-op assignment Svelte's equality check drops. And edgeDrafts is not read by editing or by either effect, so writing it closes no loop at all; clearDraftKey's own if (!(key in edgeDrafts)) return early-exit makes it idempotent independently. The guard is sufficient.

Picker effect. Same shape, and closeAdd()'s six writes are the thing to check, since one of them feeding addGroup would be the loop. addGroup derives from view and addGroupKey only. Of the six — addGroupKey, addQuery, addResults, addSearching, addStaged, addEdgeDraft — only addGroupKey is in that derived's dependency set, and it is set to null, driving the guard false exactly as above. The other five are read only by the picker template (:1026-1128), which unmounts anyway. I checked the other four effects in the component (:207, :239, :258) for a cascade: none reads any add* state. No cascade.

state_unsafe_mutation: not applicable. That error fires when state is mutated during a derived's evaluation; these writes happen inside $effect, which is the sanctioned place for them.

The suspected mid-load bug does not exist. This was the sharpest hypothesis and worth stating why it fails, because the reasoning is non-obvious. load() does blank the view:

async function load(id: string) {
  phase = 'loading';
  view = null;          // :400 — editing would resolve null here
  errorMessage = null;
  resetTransient();     // :402 — but this nulls editingKey and addGroupKey

view = null and resetTransient() are in the same synchronous block, and resetTransient() (:222-236) sets editingKey = null and addGroupKey = null. Svelte 5 effects run after the synchronous block completes, so by the time either effect evaluates, both guards are already false. There is no interleaving in which the effect observes editingKey set while view is null-because-loading. The effect cannot wrongly clear a legitimately-open editor mid-load.

I confirmed view is written in only three places (:400, :407, :422) and no other path blanks it. reloadAfterMutation (:413) deliberately never blanks — its docblock says so — which is what makes the in-place reload the only trigger, and that is precisely the intended one.

Would $derived alone have been simpler? No — and this is the substantive design question. Letting {#if editing} handle it is exactly what round 1 found insufficient: hiding the editor is a read-side concern, but the bug was that the write-side state (editingKey, edgeDrafts) outlived it. A derived cannot clear state by definition; that's the point of a derived. The state has to be forgotten, and forgetting state in response to external change is what $effect is for. Using effects here is correct, not a workaround.

The one genuinely simpler alternative would be pushing the self-close through a single explicit path so no reconciliation is needed — but that means routing an unmount through cancelEdit, which round 1 established is impossible (onOpenChange doesn't fire on unmount). Two guarded effects is the right shape given that constraint.

---

Other Verification

draftKey / rowKey refactor: correct, no call site missed. I grepped every construction and consumption site. All key building now funnels through draftKey(groupKey, rowId): rowKey delegates to it (:459-460), and the effect calls it directly (:200). Every edgeDrafts read and write (:468, :479, :485, :501) goes through rowKey. Single source of truth for the key format — this is the DRY that matters, since the whole bug class here is two paths disagreeing on how a draft is addressed. Making the key-only path and the row-object path structurally incapable of drifting is the correct fix, not just a tidier one.

runSearch's guarded finally: correct, and addSearching cannot stick true. I checked the specific worry. If the picker closes mid-flight, the finally guard fails and addSearching is not reset there — but closeAdd() (:573) and openAdd() (:563) both set addSearching = false unconditionally, and resetTransient() (:225) does too. Every path that changes addGroupKey out from under an in-flight search also clears the spinner. So the guard cannot orphan it, and guarding is strictly better than not: an unguarded finally would kill the spinner belonging to the replacement search. Same reasoning for catch.

The two new tests pin the behaviour they claim. 📁 relationship-grouping.test.ts:547-607. The first test is the load-bearing one and it is constructed honestly: it walks the full delete → re-add cycle through separate buildRelationshipsView calls and asserts both that the stale key resolves again (not.toBeNull()) and that it resolves to a different row object (not.toBe(before[0].rows[0])). That is the exact fact the fix depends on, asserted by identity rather than by value — a value assertion would have passed even if the resolver had returned the stale object. The second test pins group-key stability across the empty→populated promotion, which is what keeps the round-1 requestedKey comparison correct. Neither test is tautological.

Corrected comments are accurate, not overstated. Both now say self-closing is a reaction to a reload, not a save-time check, and findRowByKey's docblock explicitly says "do not read this as making that path unreachable." That last clause is doing real work: it prevents a future reader from deleting the daemon-error handling as dead code on the strength of a comment. Documenting the limit of a guarantee is more useful than documenting the guarantee, and this is the right instinct.

Verification adequacy: adequate, and I reproduced the mechanical parts. I independently re-ran bun run test (233 files / 4895 tests passing — matches the claim, +2) and quality:check (2501 files, 0 errors, 0 warnings — matches). The browser verification is the part I can't re-run, but the protocol described is the right one: it exercises both keys reproducing in a single component instance, which is the only way the dangling-key bug manifests. Round 1 correctly noted the prior pass "stopped at the delete, not the recreate"; this one goes through the recreate and checks the two things that distinguish a fix from a coincidence (dialogCount: 1, and GHOST DRAFT absent from the DOM). Asserting the absence of the ghost draft is what makes it a real test of the fix.

---

On the deliberate non-fix (Save across a delete/re-add)

I agree with the judgement, and a save-time identity check would be over-engineering.

The reasoning holds on its own terms: Save is an explicit user instruction to persist what is on screen, and the user typed those values against a target that is, by node id and relationship, the same target. The write lands on an edge with identical endpoints. Nothing is corrupted and nothing silently disagrees with what the user saw.

The stronger argument against the check is what it would cost. An edge identity token would have to be minted by the daemon, threaded through the view model, held in editingKey, and compared at save time — new surface across the pure module, the component state, and the wire format. In exchange, it would convert a case where the user gets what they asked for into an error message they cannot act on ("this edge was recreated, retype your value"). That fails the YAGNI test on the cost side and arguably makes the UX worse on the benefit side. It also does not generalise: no client-side check can close the true race, because the delete can always land after the check and before the write — which is exactly why the daemon-error path exists and why this commit was right to document that path as live rather than prune it.

Recording it in the commit message rather than "fixing" it is the correct call. The failure mode of a non-obvious deliberate choice is a future reader silently reverting it; a commit-message rationale is the cheapest durable defence.

---

New Findings

Two observations, neither actionable before merge.

🟢 The two effects are near-identical in shape and invite a shared abstraction — resist it

📁 relationship-viewer-modal.svelte:193-205

Both effects are "if the key no longer resolves, forget it." The pull toward a generic forgetWhenUnresolved(getKey, getResolved, onForget) helper is real, and I want to name it only to say don't. The two differ in what forgetting means (one clears a draft under a reconstructed key; the other clears six fields via closeAdd), and the abstraction would need a callback per site — leaving the same number of lines plus an indirection. Two instances is not a pattern (Rule of Three). The current form is more readable precisely because each effect states its own consequence inline.

🟢 Nit: the editor effect reconstructs a key it could read from the resolved handle

📁 relationship-viewer-modal.svelte:200

draftKey(editingKey.groupKey, editingKey.rowId) rebuilds the composite key from parts. That is necessary here — this is the branch where editing is null, so there is no row object to hand to rowKey, which is the whole reason clearDraftKey was extracted. So the code is right. The nit is only that stale is computed before editingKey = null out of ordering necessity, which reads as incidental; a one-clause comment ("read the key before clearing it") would make the ordering constraint explicit rather than something the next reader has to re-derive. Genuinely optional.

---

Assessment

The round-1 🟡 was a real bug with a correct diagnosis, and this commit fixes the root cause rather than the symptom. The reactivity concern that prompted this re-review is unfounded on analysis: both effects reach a fixed point in one re-run because each write drives its own guard monotonically false, the written state is outside the deriveds' dependency sets except for the one key that terminates the loop, and the suspected mid-load clearing is impossible because resetTransient() runs in the same synchronous block as view = null. Requesting a second pair of eyes on one's own reactivity change was the right instinct even though the answer came back clean — that is the correct default for this code shape.

The comment corrections are the quietly valuable part of the commit: they replace a guarantee the code doesn't make with an explicit statement of the limit, which protects a live error path from being mistaken for dead code.

Net effect on code health: clearly positive. No blockers, no 🟡, nothing outstanding from any of the three rounds.

Recommendation

APPROVE — merge as is. Both 🟢 items above are optional; neither warrants another round.

🤖 Generated with Claude Code

[malibio]

Automated pragmatic-code-review (re-review round 2): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Round-2 feedback addressed — commit ac92bc13

Both round-2 items were 🟢 and optional; the cycle is complete.

• "Resist a shared abstraction" for the two effects — followed by leaving them alone. Two instances isn't a pattern, and they differ in what forgetting means, so a helper would need a callback per site.
• The stale-before-null ordering nit — addressed. Comments only, no logic change: a one-clause note on the ordering constraint, plus the two facts that took tracing to establish (the effect terminates on its first run because nulling the key falsifies its own guard; and load() blanking view can't trip it, since resetTransient() runs in the same synchronous block and has already cleared both keys).

No further re-review requested. A comment-only change to just-reviewed code is the trivial/low-risk case — re-reviewing it would be ceremony.

Cycle summary

Commit	Round	Verdict
3feaab30	The redesign	APPROVE — 2 🟡, 4 🟢
6aac7786	All 6 addressed	APPROVE — 1 new 🟡, 2 🟢
620d7bd2	All 3 addressed	APPROVE, merge as is — 0 new
ac92bc13	Optional 🟢 nit	—

14/14 acceptance criteria delivered and browser-verified. 233 files / 4895 tests passing (baseline 232 / 4863; +32 new). eslint + svelte-check 0/0 across every round; full pre-push gate green on all four pushes.

Follow-ups worth their own issues, not this PR

1. Save across a delete/re-add writes the open draft to the recreated edge. Judged correct — Save means "persist this" — and the reviewer agreed a save-time identity check would be over-engineering, since closing the true race needs daemon-minted edge identity threaded through the view model and wire format.
2. The modal is still ~1100 lines. The issue cited 1044 as evidence of structural weight, but what remains is largely the per-group view-settings popover (~85 lines of columns/sort/filter), unrelated to Relationships modal renders a full section per declared type, so an empty node shows six 'Add' buttons and no data #2385. Splitting it out would be a clean separate change.

🤖 Generated with Claude Code

[malibio]

Merged main — resolved a real overlap with #2387

main had moved on and this went CONFLICTING. Worth recording what the conflict was, because it wasn't mechanical.

#2387 ("closed enum value set for edge fields") touched the same two files at the same lines. It added an enum edge-field kind: a Select picker in both edit paths, plus formatEdgeValue so a read-only cell shows an enum's declared label instead of its stored key. This PR had meanwhile moved edgeInputKind into edge-field-input.ts and replaced both of those edit paths.

Resolved by keeping this PR's structure and carrying #2387's feature into it:

• enum kind ported into edge-field-input.ts, where that logic now lives
• formatEdgeValue kept, wired into the new read-only edge cells
• Enum Select added to both of this PR's edit surfaces — the targetPicker snippet and EdgePropertiesModal — where EdgeField cannot declare enum values, so RBAC roles on a relationship would be free text #2387 had two inline copies
• Dropped EdgeField cannot declare enum values, so RBAC roles on a relationship would be free text #2387's formatColumn; formatEdgeFieldLabel supersedes it

Taking either side wholesale would have lost enum support or reverted the redesign.

Verified the combination, not just that it compiles

Used the collection↔person RBAC schema this issue cites as the reason per-row edge editing had to survive — a shared_with relationship with an access enum (Owner / Can modify / Read only) and a granted_on date:

• Read-only cell renders Read only — the label, not the stored read_only
• Per-row ✏️ opens the editor with an enum picker (not a text box) offering all three declared labels
• Selecting "Can modify" persists the key "modify" while the cell displays "Can modify"

So the RBAC case the issue was protecting now works end to end on the merged code.

Tests: 233 files / 4900 passing (4895 mine + #2387's enum tests). eslint + svelte-check 0/0. Full pre-push gate green on the merged tree. mergeStateStatus: CLEAN.

Also merged 5ddcf4e3, which documents the six status values the board accepts — the CLAUDE.md discrepancy I hit earlier (gh:status rejects "Ready for Review"; the correct value is "In Review").

🤖 Generated with Claude Code