Browse relationships in two panes instead of stacked tables (closes #2432)

[malibio]

Phase 2 of #2432 — the panel itself. Stacked on #2441 (phase 1), which it depends on for focusOrOpenNode. Review #2441 first; this PR's diff will shrink to just the panel once that merges.

Closes #2432.

The problem

#2426 stopped the panel scaling with the schema — declared-but-empty relationships no longer each got a section. It still scaled with the node's data: every populated relationship rendered its own full-width table, stacked vertically. Six populated relationships meant six tables and a scrollbar.

Two panes

Left — a rail of the relationships the node actually has, one row each with its edge count. Right — every edge of the selected relationship, each row expanding in place to that edge's properties.

Two panes rather than three because expansion does the third pane's job: the rail stays short whatever the node's connectivity, properties stay one click away, and a single-target relationship needs no special case — it shows its one edge.

That also retires edge-properties-modal.svelte. It existed because a table cell couldn't hold a variable-length form; a nested dialog over the panel isn't worth keeping once the row itself can.

The rail encodes the ownership boundary

Order is: relationships this node owns → + Add → divider → INCOMING · READ-ONLY. The control's position says what the user may create, so no per-item badge has to. The detail pane carries no "read-only" badge either — the rail heading above the selection already said it, and the absent controls are what actually hold the rule.

+ Add is the single creation path, listing only what's still addable (outbound, and not a cardinality: one that's already filled).

What was deleted, and why that was a decision

The per-group view-settings subsystem from #1918 — selectable columns, sort, filter, persisted per (nodeType, relationshipName, direction) — is removed: two services, three test files, and the two service functions that existed only to feed it (fetchTargetSchemaFields, fetchNodesProperties).

The two-pane design's answer to "see edge data" is expanding a row, not configuring columns; a right pane offering both would be two mechanisms fighting over one job. Left in place they'd have been unreachable code. This was an explicit product call, not incidental cleanup — flagging it because it removes shipped functionality, and reversing it later means rebuilding the popover against the new layout.

Net −1457 lines.

One bug found in the browser, and its test

Removing a relationship's last edge left the detail pane showing a stale header with no rows.

The re-selection effect asked whether the selected key still resolved — and it does. Removing the last edge doesn't remove the group; it stays declared, which is exactly what makes it addable again. The rail is built from populated, so that's what the check has to be against. Fixed, with a unit test that pins the distinction rather than the symptom.

Verification

Tests: 230 files / 4877 passed. The drop from 4916 is the 40 view-settings tests deleted above. eslint + svelte-check: 0 errors, 0 warnings. Full pre-push gate green (frontend + skill + Rust + e2e).

Browser (dev:browser, isolated NODESPACE_HOME, real DB untouched) against purpose-built conference / speaker / venue schemas — one relationship with enum + string edge fields, one cardinality: one with none:

• Rail selection; first relationship auto-selected on open
• Expansion collapsed by default, independent toggles (expanding one left the other closed), and absent entirely on the relationship declaring no edge fields
• Enum renders its label ("Keynote"), not the stored key
• Auto-save: edited a topic field, closed the panel, reopened — value persisted. No Save button
• Inbound read-only: from the speaker's side the same edge shows ROLE: Keynote / TOPIC: Knowledge Graphs — the value edited from the conference side, confirming it's one physical edge, not a copy — with zero inputs and zero remove buttons, and the hint "Edit these from the node that owns this relationship"
• Target links open via phase 1's focusOrOpenNode, reusing a tab
• Removal, and the resulting re-selection (the bug above)
• Full + Add → chooser → search → add flow, with the chooser correctly empty when every relationship is populated

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

Review Type

Pragmatic code review of phase 2 only — commit 6305e72 (origin/issue-2432-focus-or-open-node...HEAD). Phase 1 (navigation-service.ts + the sidebar/search/collection/query-viewer migrations) belongs to #2441 and was not re-reviewed.

Scope: 9 files, +618 / −2075.

Requirements Check — #2432 Phase 2

Acceptance criterion	Status
Rail lists only relationships with ≥1 edge; no 0 rows	✅ populatedGroups = partitionGroups().populated = rows.length > 0
Rail order: owned → + Add → divider → incoming	✅ relationship-viewer-modal.svelte:610-926 in exactly that order
Selecting a relationship shows its edges	✅ selectedGroup derived from findGroupByKey
No edge_fields → plain rows, no arrow, no empty form	✅ expandable = fields.length > 0; spacer <span> keeps alignment
edge_fields → expandable rows, several at once	✅ expandedRows is a per-key record, not a single value
Edge properties edited in the expanded row; modal deleted	✅ edge-properties-modal.svelte deleted, zero remaining references
Edits persist on change; no Save button	✅ commitEdge on onValueChange / onchange / onblur
enum renders labels, persists keys (#2387)	✅ Select.Item value={option.value} + enumValueLabel in trigger and read-only path
+ Add is the only creation path	✅ no add control in the right pane
+ Add omits inbound and filled cardinality: one	✅ inherited from partitionGroups().addable (direction === 'out' && rows.length === 0)
Inbound shows edge properties read-only, same values	✅ editable = groupSupportsEdgeEditing(group) is false for in → value spans, not inputs
Inbound offers no add/edit/delete; one action opens owner	✅ remove button gated on {#if !inbound}; only the open button remains
No read-only badge in the detail header	✅ header carries label / targetType / cardinality / required only
Target links use the phase-1 method	✅ openTarget → focusOrOpenNode
Node with no relationships → empty rail with + Add	⚠️ partially — see 🟢 #2
Panel fits without scrolling for several relationships	✅ max-h-[55vh] on both panes, rail height independent of connectivity
Panes collapse below a narrow breakpoint	✅ grid gap-4 sm:grid-cols-[…] — single column below sm
Immediate persistence, per-edge draft isolation, error surfacing	✅ drafts keyed per row; clearRowDraft scoped; mutationError banner

sm:max-w-4xl container as specified. Enum-label rendering, findGroupByKey/findRowByKey, groupSupportsEdgeEditing and edge-field-input.ts all survive from #2426 unrewritten, per the "do not rewrite" list.

Code Review Findings

🟡 1. The deletion left layout and edgeColumns behind as dead code — and with them, a silent regression

📁 packages/desktop-app/src/lib/services/relationship-grouping.ts:26, 105-107, 160-196, 209-227

The deletion of the view-settings subsystem is otherwise clean and complete: grep for relationship-view-settings, edge-properties-modal, EdgePropertiesModal, fetchTargetSchemaFields, fetchNodesProperties across packages/ returns nothing, and isUserVisibleField retains four legitimate consumers (task-schema-form.svelte, generic-schema-form.svelte, table-view.svelte, plus the fixture doc reference). No orphaned imports, svelte-check is at 0/0.

But two view-model members are now unreachable from production code. Their only remaining callers are their own tests:

• RelationshipLayout / groupLayout / RelationshipGroupView.layout — the old modal branched on group.layout === 'table' at lines 744 and 836 (verified against origin/issue-2432-focus-or-open-node). The new panel branches on expandableFields(group).length > 0 instead. Zero production reads of .layout remain.
• groupEdgeColumns / RelationshipGroupView.edgeColumns — the old modal read group.edgeColumns at line 297; the deleted relationship-view-settings.ts was the other consumer. Zero production reads remain.

CLAUDE.md is explicit: "Delete unused code, don't suppress warnings" and "no dead code left behind". buildGroupView still calls both on every group build, so this is also work done per group per reload for a value nobody reads. The tests at relationship-grouping.test.ts:76-126, 170-171, 184-185, 232, 461 are pinning behavior no user can observe — they will keep the code alive against future cleanup, which is the specific failure mode the greenfield rule exists to prevent.

The regression hiding inside this. groupEdgeColumns was the mechanism that surfaced ad-hoc undeclared edge keys — keys present on a stored edge but absent from edgeFields. groupSupportsEdgeEditing's own docstring (:267-274) still describes the contract that no longer holds:

"A group carrying only AD-HOC edge keys … is deliberately not editable either, even though groupEdgeColumns still surfaces those keys as read-only columns so the data stays visible."

It does not surface them any more. expandableFields(group) returns group.edgeFields, so a group whose edges carry only undeclared properties now renders expandable === false — no disclosure arrow, no way to see the values at all. formatEdgeValue's field: RawEdgeField | undefined signature and its comment about "an undeclared edge key, where field is undefined" describe a call that can no longer occur, since fields is sourced from edgeFields where every entry is defined.

This may well be an acceptable product call — the same call as dropping configurable columns. But it is currently undocumented and contradicted by a docstring in the file next door, which is the worst of the three states. Decide and make the code say it:

• If ad-hoc keys should stay visible: expandableFields should merge edgeFields with the row's undeclared edgeValues keys (per-row, not per-group, since groupEdgeColumns computed a group-wide union). formatEdgeValue's undefined branch then earns its keep.
• If they are intentionally dropped: delete RelationshipLayout, groupLayout, groupEdgeColumns, both view-model fields and their tests; correct groupSupportsEdgeEditing's docstring; and narrow formatEdgeValue to field: RawEdgeField.

Principle: YAGNI / dead-code elimination. Unreferenced code that tests keep alive is worse than deleted code, because the tests make it look load-bearing.

🟢 2. + Add disappears entirely when nothing is addable, including on a node with no relationships

📁 relationship-viewer-modal.svelte:672 and :634-636

{#if addableGroups.length > 0} hides the whole + Add popover. Combined with the empty-state branch at :634 (populatedGroups.length === 0 && addableGroups.length === 0 → "This node has no typed relationships."), the acceptance criterion "A node with no relationships shows an empty rail with + Add" is met only when the node's schema declares at least one outbound relationship. A node whose type declares none gets the text block with no rail and no + Add — which is arguably the more honest UI (there is nothing to add), but it is not literally what the criterion says.

Worth an explicit confirmation from the issue author rather than a code change; if the current behavior is intended, the criterion should be reworded. Not a blocker either way — hiding a control that can only ever open an empty list is the right instinct.

🟢 3. Auto-save: draft isolation is correct, but the enum picker leaves a concurrent-write window open

📁 relationship-viewer-modal.svelte:667-676 (commitEdge), :589 (runMutation), :791 (Select.Root)

The per-row draft model is sound. edgeDrafts is keyed by ${group.key}::${row.id}, setEdgeDraft merges into just that row's bag, clearRowDraft deletes just that key, and the failure path deliberately keeps the draft so the surfaced error is actionable without retyping. That directly satisfies the "per-edge draft isolation" and "error surfacing" criteria, and the comments explain the why rather than the what.

Two narrow concerns:

Out-of-order writes. runMutation has no request sequencing — nothing cancels or supersedes an in-flight write. Two commitEdge calls for the same row could resolve out of order, and since updateEdgeProperties replaces edge properties wholesale, the loser would clobber the winner. In practice busy disables the Input (:809) and the checkbox (:826) while a write is in flight, which prevents this for those two. But Select.Root at :791 carries no disabled={busy}, unlike its siblings. Two quick enum picks on the same row are therefore two concurrent overlapping writes racing over a wholesale replace. One-line fix: add disabled={busy} to the Select.Trigger.

Blur-during-busy is benign but non-obvious. commitEdge reads row.edgeValues as the merge base; row comes from the current render of selectedGroup, which reloadAfterMutation replaces wholesale, so the base is fresh rather than a pre-mutation snapshot — correct. Note also that applying disabled={busy} to a focused <input> fires blur, re-entering commitEdge; on the success path clearRowDraft has already run, so the !draft || Object.keys(draft).length === 0 guard absorbs it. Worth a one-line comment on that guard saying it is also what absorbs the disable-induced blur — otherwise it reads as merely defensive.

Principle: last-write-wins over a wholesale-replace API needs either sequencing or input gating; the gating is here for two of three input kinds.

🟢 4. splitRowKey is sound, and the pruning effect's stated hazard is real

📁 relationship-viewer-modal.svelte:395-405, :207-217

splitRowKey uses lastIndexOf('::') against a composite of ${direction}:${relationshipName}:${targetType ?? '*'} + '::' + rowId. Checked against the actual key shape at relationship-grouping.ts:218:

• The group key's own separators are single colons, and none of its three parts can contain :: — direction is the literal 'out'/'in', targetType is a schema type name or '*', and relationshipName is a schema identifier. Even if a relationship name did somehow contain ::, lastIndexOf still splits at the joining separator, because rowId is a node id (UUID) containing no colons at all. Choosing lastIndexOf over indexOf is what makes it robust, and the comment explains exactly that.
• The at === -1 fallback returns [key, ''], which findRowByKey resolves to null, so a malformed key is pruned rather than throwing. Correct degradation.

The pruning effect's documented hazard is genuine and I confirmed the mechanism: RelationshipRowView.id is node.id (relationship-grouping.ts:212) — the target node's id, not the edge's. Remove an edge and re-add the same target to the same relationship, and ${groupKey}::${rowId} reconstructs identically. A lazy resolve-on-read would leave the dangling true in the record and the rebuilt row would spring open pre-expanded. The active prune is the right call, and the comment earns its length by explaining a hazard invisible from the code.

🟢 5. Effect termination is sound across all four effects

📁 relationship-viewer-modal.svelte:183-190, 207-217, 219-224, 226-235

Each was checked for the write-triggers-own-read loop:

1. Re-selection (:183) — reads populatedGroups and selectedKey, writes selectedKey. Writes only when the selection is absent from the rail; the assigned value is populatedGroups[0].key, present by construction. Second run finds stillInRail === true and no-ops. Terminates. The bug fix is correct and for the right reason: the check is populatedGroups.some(g => g.key === selectedKey), not findGroupByKey(...) !== null. Since partitionGroups keeps an emptied group in groups (so it can appear in addable), key-resolvability and rail-membership genuinely diverge — checking the former is what stranded the header.
2. Expansion pruning (:207) — reads and writes expandedRows, but only when stale.length > 0, and the write removes exactly the stale keys. Second run computes stale = [] and returns. Terminates.
3. Add-picker cleanup (:219) — if (addGroupKey && !addGroup) closeAdd(); closeAdd sets addGroupKey = null, so the guard is false on re-run. Terminates.
4. Load-on-open (:226) — guarded by loadedKey === nodeId, set before the async call. Terminates, and the loadedKey !== id re-check after each await in load / reloadAfterMutation correctly discards a stale response for a superseded node.

With view === null (loading, error, or the !open reset): populatedGroups is [] → effect 1 clears selectedKey; effect 2 prunes against [], correct because resetTransient has already emptied expandedRows; effect 3 sees addGroup === null and closes a picker whose group vanished mid-interaction — the right behavior. load() calls resetTransient() before awaiting, so a mid-interaction reload cannot leave a picker open over a blanked view.

runSearch also re-reads addGroup after the await (:487-489) specifically so the already-linked filter uses post-mutation rows. That is careful work and the comment says why.

🟢 6. Inbound read-only is structurally enforced, not merely styled

📁 relationship-viewer-modal.svelte:703, 764-775, 789-844

Verified this is real enforcement, not visual suppression:

• editable = groupSupportsEdgeEditing(group) returns false for any direction === 'in' group (relationship-grouping.ts:276-278), independent of edgeFields. The {#if !editable} branch at :789 renders a <span> — there is no disabled input in the DOM to re-enable.
• The remove button is inside {#if !inbound} (:764), so it is absent, not disabled.
• addableGroups filters direction === 'out', so + Add can never target an inbound relationship — and even if it could, resolveEdgeEndpoints would flip the endpoints correctly and the daemon would arbitrate.

Three independent layers, each structural. The hint text at :846-850 distinguishes the two cases correctly.

🟢 7. Accessibility is good, with two gaps

📁 relationship-viewer-modal.svelte:611, 642, 727, 745, 759, 583-590, 778

Solid: the rail is a <nav aria-label="Relationships"> wrapping <ul>/<li> of buttons; aria-current={selected ? 'true' : undefined} is applied on both the owned and incoming rails, correctly omitted rather than set to "false"; expanders carry aria-expanded={open} plus a state-dependent aria-label (Expand/Collapse ${row.label}); icon-only open/remove buttons have both title and aria-label; the alignment spacer is aria-hidden="true"; dialog semantics come from Dialog.Root/Header/Title/Description. focus-visible:ring-1 is present throughout.

Two gaps:

• The expander does not control an identified region. aria-expanded without aria-controls tells a screen-reader user that something expanded, with no programmatic path to it. Add an id on the expanded <div> at :778 derived from rowKey(group, row), and aria-controls on the button.
• The mutationError banner is not announced. At :583-590 it appears in the DOM with no role="alert" / aria-live, so a failed auto-save — the one failure mode a Save-button-less UI cannot signal any other way — is silent to assistive tech. Given "error surfacing" is an explicit acceptance criterion, role="alert" here is worth adding.

🟢 8. Nits

• Nit: 📁 :381 — openTarget calls focusOrOpenNode(row.id, { nodeType: row.nodeType }) without a title, so a newly opened tab reads Loading... until the viewer mounts. row.label is right there and FocusOrOpenOptions.title exists precisely for callers that already know it (navigation-service.ts:65-68).
• Nit: 📁 :666-668 — expandableFields(group) is a one-line passthrough returning group.edgeFields. It earns its keep only if it grows the ad-hoc-key merge from finding Set up Tauri + Svelte project structure #1; if that finding resolves the other way, inline it.
• Nit: 📁 :701 — {@const open = expandable && isExpanded(group, row)} shadows the component's open prop (:100). Legal in Svelte and scoped to the {#each}, but in a file where open = false is the modal-closing idiom (:379), a name like expanded reads with less friction.
• Nit: 📁 relationship-viewer-service.ts:2 — the module docstring still opens "Relationship viewer + editor service (issue Relationship viewer: view and manage typed relationships between nodes #1918)". CLAUDE.md: "Do not embed GitHub issue numbers in documentation content or code comments." Pre-existing, not introduced here, but this PR is the one deleting Relationship viewer: view and manage typed relationships between nodes #1918's subsystem — a natural moment to drop the reference.

Standards Compliance

• ✅ No lint suppression; svelte-check reports 0 errors / 0 warnings (independently reproduced)
• ✅ No any; no {@html}
• ✅ createLogger('RelationshipViewerModal') — no raw console.* anywhere in the diff
• ✅ No backward-compat shims, no #[deprecated], no feature flags
• ⚠️ One dead-code exception — finding Set up Tauri + Svelte project structure #1
• ✅ Bun only
• ✅ relationship-grouping.test.ts passes (35/35, reproduced locally)

Verdict: APPROVE

This is a clear net improvement and the design reasoning holds up under scrutiny. The layout change does what the issue asked — the rail's height is now a function of the schema's shape rather than the node's connectivity — and it removes 1457 lines net while doing it. The deletion of the #1918 view-settings subsystem is complete and correct at the module level, with isUserVisibleField verified to retain four legitimate consumers elsewhere.

The bug fix is right for the right reason, and its test pins the actual invariant — that a group survives losing its last edge, so key-resolvability and rail-membership diverge — rather than the symptom. That is the harder and more durable test to write.

All four effects terminate, splitRowKey is sound for every realistic key shape, inbound read-only is enforced structurally across three layers rather than styled, and the async paths consistently re-check loadedKey / addGroupKey after every await.

Before merge, two small things worth doing:

1. Finding Set up Tauri + Svelte project structure #1 — resolve the layout / edgeColumns dead code one way or the other, and either restore ad-hoc edge-key visibility or correct groupSupportsEdgeEditing's now-false docstring. The docstring contradicting the code is the part that will mislead the next reader.
2. disabled={busy} on the enum Select.Trigger (:791) — one line, closes the only concurrent-write window against a wholesale-replace API.

Neither blocks the architecture. Everything else above is a nit.

[malibio]

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

[malibio]

Address Review — all findings addressed

✅ 🟡 1. Ad-hoc edge keys had become invisible — a real regression, not just dead code

You were right that this was the worst of the three possible states, and the diagnosis was exact.

Row expansion keyed off group.edgeFields — the declared fields — so an edge carrying only undeclared properties rendered unexpandable, with its values unreachable. The old modal surfaced them via groupEdgeColumns, and groupSupportsEdgeEditing's docstring still promised exactly that while the code no longer delivered it.

Expansion now runs over edgeColumns (declared ∪ stored). Editing still keys off the declaration, which is the asymmetry the docstring always described: an undeclared key has no type, and the update path replaces properties wholesale, so an editor could only guess a free-text input and would coerce a stored number or boolean to a string on first save.

A second bug fell out of fixing this. The read-only hint keys off editable, which is false for two unrelated reasons, and said "Edit these from the node that owns this relationship" — actively misleading for an outbound relationship this node already owns, where the real answer is to declare the field. Now split:

• inbound → "Edit these from the node that owns this relationship."
• undeclared → "These properties aren't declared on the schema, so they're shown as stored. Declare them as edge fields to make them editable."

Verified in the browser with a purpose-built schema declaring no edge fields, given an edge storing one anyway: the row expands, shows FITTING: snug, renders zero inputs, and explains what would make it editable.

✅ 🟡 1b. layout / groupLayout / RelationshipLayout deleted

With edgeColumns back in production use, groupLayout had one caller left: a guard inside groupEdgeColumns returning [] for a "chips" group — redundant, since the union is empty in exactly that case. Removed the guard, then the function, the type, and the view-model member.

Its three unit tests are replaced by three pinning the union rule instead — including the undeclared-key case that was broken.

✅ 🟢 disabled={busy} on the enum Select.Root

Its checkbox and text siblings had it; the enum did not. Real concurrent-write window against a wholesale-replace API.

✅ 🟢 Accessibility

aria-controls linking each expander to the panel it toggles (verified in the DOM), and role="alert" on both error banners.

⏭️ Not changed: the + Add criterion wording

You flagged this as wanting author confirmation rather than a code change. The behaviour is correct — a node with no edges shows + Add and nothing else — so this is a question about the issue's phrasing, not the implementation. Leaving it.

---

Verification

• 230 files / 4877 tests passed
• eslint + svelte-check: 0 errors, 0 warnings
• Full pre-push gate green (frontend + skill + Rust + e2e)
• Browser-verified the ad-hoc case end to end, in an isolated NODESPACE_HOME

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: This is more than the two small pre-merge items you flagged. Restoring ad-hoc visibility changed what drives expansion (edgeFields → edgeColumns), introduced a new EdgeRow type through the render path, split a user-facing message on a new condition, and deleted a type plus a function with three tests rewritten. That is interconnected enough to warrant another pass — particularly on whether the edgeColumns union is the right expansion driver in every case, and whether the rewritten tests pin the rule as well as the ones they replaced.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

Re-Review — commit 08b72b41 only

Scope: the single commit since my last review (6305e72e..HEAD), 3 files, +106 / −68. Phase-2 panel commit 6305e72e and phase 1 (#2441) were not re-reviewed.

Previous Review Summary

• Recommendations: 4 (finding 1 dead code + regression; finding 3 disabled={busy}; finding 7 a11y ×2; plus 4 nits and 1 author-confirmation item)
• Addressed: 4 — all four substantive items, and the diagnosis was carried further than I asked
• Not addressed: 1 — the + Add criterion wording (🟢 Create design system foundation #2), correctly declined as an issue-phrasing question, plus the four nits, which were left as nits
• New issues: 1 (🟢, a behavioural consequence worth documenting, not fixing)

The response to finding #1 is better than the fix I proposed. I offered two ways out and mildly framed them as equivalent product calls; the author correctly identified that only one of them was defensible, because a docstring in the file next door already promised ad-hoc visibility as deliberate design, not as an accident of the old renderer. Restoring the behaviour and then deleting layout is the right order of operations — deleting first would have cemented the regression as intent.

Finding the read-only hint bug while in there is the kind of thing a careful fix surfaces and a mechanical one does not. 'Edit these from the node that owns this relationship.' shown on an outbound relationship the node already owns was actively misdirecting, and it was invisible before this commit only because the case it fires on was itself unreachable. Fixing the regression is what made the second bug observable.

Verification of the specific claims

I checked the four things I said I would rather than taking the commit message's word for them.

The groupLayout guard removal is a true no-op. groupLayout returned 'chips' iff !hasDeclaredFields && !hasEdgeData — i.e. edgeFields empty and no related node carries a non-empty edgeProperties. Under exactly that condition groupEdgeColumns' first loop iterates an empty list and its second contributes no keys, so the union is [] regardless. The old guard returned [] in precisely the case the function already returned []. Equivalence confirmed for every input, not asserted.

The EdgeRow threading is complete and type-safe. field.name → entry.name at all five call sites (the {#each} key, formatEdgeFieldLabel, and the three setEdgeDraft calls); the key is (entry.name), which is correct since edgeColumns is deduplicated by construction via its seen set. No possibly-undefined field reaches a function requiring one: edgeInputKind is guarded by field ? … : null, getEnumValues/enumValueLabel sit inside the kind === 'enum' branch which is only reachable past !field || in the guard, and formatEdgeValue already declared RawEdgeField | undefined — that parameter's undefined branch, which I flagged last time as describing an impossible call, now earns its keep. svelte-check: 2492 files, 0 errors / 0 warnings. ESLint on all three changed files: clean.

The {#if editable}{:else if inbound}{:else} chain is exhaustive and correctly ordered. The third branch is reachable only when !editable && !inbound — outbound with zero declared fields — and since expansion requires edgeColumns.length > 0, that state implies at least one undeclared key. So the message is always true when shown. On the overlap you asked about: an inbound group can carry undeclared keys, and there inbound wins. That is the right precedence — "declare the field on the schema" is not actionable advice for an edge this node does not own, whereas "open the owning node" remains true and leads somewhere useful. Worth noting the ordering is load-bearing rather than incidental; flipping the two branches would reintroduce a variant of the bug just fixed.

The docstring now matches the code. groupSupportsEdgeEditing's contract reads "the panel expands a row over that union" — which is now literally what expandableFields does. The false promise is gone.

Dead code is genuinely gone. grep across packages/ for groupLayout / RelationshipLayout / .layout returns zero hits in source and tests; the only edgeColumns matches are the live production path plus four test assertions (the sole remaining hits are in packages/desktop-app/coverage/, a build artifact). relationship-grouping.test.ts: 35/35 pass.

Findings

🟢 1. NEW: edgeColumns is group-wide, so a bare row inherits its siblings' ad-hoc keys

📁 relationship-viewer-modal.svelte:592-594, relationship-grouping.ts:162-179

You asked me to scrutinise exactly this, so I verified it empirically rather than by reading. With a group declaring no edgeFields and two related nodes — one with edgeProperties: { note: 'x' }, one with {} — buildRelationshipsView yields edgeColumns: ['note'] for the group and edgeValues: {} for the second row. The bare row therefore renders an expander, and expanding it shows NOTE —.

This is acceptable, and I would not change it. Three reasons, in order of weight:

1. The alternative is worse. Per-row expansion (Object.keys(row.edgeValues)) would make the disclosure arrow appear and disappear down a list of otherwise-identical rows, and — more importantly — it would make an edge's absent property invisible rather than shown as empty. In a group where three of four edges carry note, the fourth's missing note is information; — conveys it, no-arrow does not.
2. It is the pre-existing contract, restored. groupEdgeColumns was always a group-wide union — that is what made it columns. The old stacked table rendered exactly this: a NOTE column with an empty cell for the bare row. This commit restores that semantic in a new presentation rather than inventing one.
3. The declared case already behaves this way and always did. A group declaring role shows ROLE — on every edge that has not set it. Ad-hoc keys now behave consistently with declared ones, which is the less surprising rule.

The only cost is a slightly noisy expander on a row with nothing stored. That is a real but minor cost, and it buys consistency.

Suggestion, not a change request: expandableFields' docstring explains why edgeColumns beats edgeFields but does not mention that the union is group-wide, which is the non-obvious half. One clause — noting that a row shows a sibling's ad-hoc key as —, deliberately, so a missing value reads as missing rather than as absent-column — would pre-empt the next reader filing this as a bug. The reasoning is good; it just is not written down.

Principle: least astonishment is served by one consistent rule (columns are a group property) over a locally-tidier per-row rule that hides the difference between "empty" and "not applicable".

🟢 2. The replacement tests pin the union rule well, and cover more than they replaced

📁 relationship-grouping.test.ts:75-111, 236-240, 465-469

You asked whether anything meaningful lost coverage. I compared the deleted and added assertions case by case.

The three deleted groupLayout tests covered: declared-fields → table, edge-data-without-declaration → table, neither → chips. The three replacements cover: undeclared-key-only → ['role'], declared-then-undeclared → ['role', 'note'] (ordering, which nothing previously pinned), neither → []. The two conversions at :236-240 and :465-469 swap layout assertions for edgeColumns ones over the same fixtures.

Net assessment: coverage improved. Every input partition the old tests distinguished is still distinguished, because layout === 'table' was exactly edgeColumns.length > 0 — the new assertions are strictly more specific over the same partitions, checking which columns rather than a boolean derived from whether there were any. The declared-before-undeclared ordering test is genuinely new, and ordering now matters more than it did: it is the visual order of rows in the expanded panel, not just column order in a table nobody sees any more.

The one thing not covered at this level is expandableFields itself — the edgeColumns → EdgeRow[] mapping, specifically that field is undefined for an undeclared name. It is a four-line pure function over a Map, it is exercised through the component, and the author browser-verified the end-to-end case (FITTING: snug, zero inputs, correct hint). I would not require a unit test for it. Noting it only because it is the one seam between the tested service and the untested component, and it is where a future regression of this exact bug would land.

🟢 3. disabled={busy} and the a11y items are correctly placed

📁 relationship-viewer-modal.svelte:609, 624, 762, 806-808, 833

• disabled={busy} went on Select.Root (:833), not Select.Trigger as I suggested. The author's placement is the correct one — bits-ui propagates disabled state from the root to the trigger and closes the content, whereas disabling only the trigger would leave an already-open dropdown interactive. This closes the concurrent-write window properly.
• aria-controls={edge-props-${rowKey(group, row)}} (:762) matches the id on the panel (:806-808), and rowKey is ${group.key}::${row.id} — unique within the rendered list, since only one group renders at a time. Correct.
• role="alert" on both banners (:609, :624). Both, not just the mutation one — right call, since the load-failure banner is equally unannounced otherwise.

🟢 4. Nits from the previous review, still open

All four remain, all still nits, none worth blocking on. Restating only because they were not mentioned in the response and I do not want them silently lost:

• Nit: :381 — openTarget still omits title: row.label, so a newly opened tab reads Loading... briefly.
• Nit: :592 — expandableFields has now earned its keep and outgrown its name; it returns EdgeRow[], not fields. edgeRowsFor(group) or expandedRowsFor(group) would read truer, especially with fields as the call-site binding at :752 while entry.field is the actual field inside.
• Nit: :753 — open still shadows the open prop.
• Nit: relationship-viewer-service.ts:2 — module docstring still carries (issue #1918), against CLAUDE.md's no-issue-numbers-in-comments rule.

The naming one is marginally more worth doing than it was before this commit, since the type/return mismatch is new.

Standards Compliance

• ✅ No lint suppression — ESLint clean on all three changed files
• ✅ No any — EdgeRow.field is RawEdgeField | undefined, narrowed at every use
• ✅ No {@html}; the new hint text is plain template content
• ✅ No raw console.* in the diff
• ✅ No dead code left behind — layout fully retired, verified by grep across packages/
• ✅ No backward-compat shims; the type was deleted outright rather than deprecated
• ✅ Bun only
• ✅ svelte-check 0/0 across 2492 files; relationship-grouping.test.ts 35/35

Verdict: APPROVE — no re-review needed

Both pre-merge items from the previous review are closed, and the first was closed more thoroughly than requested: the author treated the regression as primary, restored the behaviour, and only then deleted the concept whose removal would otherwise have made the regression permanent. The equivalence claim for the groupEdgeColumns guard removal holds for every input, the EdgeRow threading is complete with no unguarded undefined reaching a function requiring a field, the hint chain is exhaustive with load-bearing branch ordering, and the replacement tests cover strictly more than the ones they retired.

The one new finding is a behavioural consequence of the group-wide union, not a defect — it restores the old table's semantics and is the better of the two available rules. My only suggestion is one clause of documentation on expandableFields recording why the union is group-wide, since that is the half a future reader is most likely to mistake for a bug. Not a blocker, and fine to fold in whenever this file is next touched.

The four outstanding nits are unchanged in status and remain optional.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

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

[malibio]

Address Re-Review — 3 of 4 nits taken

You marked all four as optional. Three were worth taking; one I'm declining with reasoning.

✅ (issue #1918) in a doc comment

A straight CLAUDE.md violation — issue numbers don't belong in code comments; constraints get described directly or referenced by ADR. Removed, and grepped the rest of the relationship files for others (none).

✅ openTarget was dropping a label it already had

Better catch than a nit. It called focusOrOpenNode with only nodeType, so every target opened as "Loading..." until its viewer mounted — while row.label sat right there, already resolved. title is precisely the option phase 1 added for a caller in this position, and collection-node-viewer.svelte already uses it the same way. So this was the panel not using an affordance built for it.

✅ The group-wide union is now documented

Your finding #1 was right that this is the half a reader would file as a bug. The docstring now says the union spans the whole group, and why per-row columns would be worse — the expander flickering down a list of near-identical rows, and losing the distinction between "this edge has no note" and "no edge here has a note", which is information when three of four carry one.

✅ {@const open} shadowing → rowOpen

Checked whether it was a live bug first: it is not. open = false in openTarget sits outside the each-block and correctly assigns the prop; the shadowed name is only read within the block. Renamed anyway — a collision with a $bindable prop is a trap for the next editor, and it costs nothing.

⏭️ Declined: renaming expandableFields

It returns EdgeRow[] rather than fields, so the name is imprecise — but "field" is the domain's word here, shared by the docstring, edgeFields, RawEdgeField, and edge-field-input.ts. A more literally accurate name would read less like the code around it. Worth revisiting if EdgeRow ever grows beyond a name plus an optional declaration.

---

On your new finding (🟢 1)

Agreed on all counts, including that it shouldn't change. Your reasoning for why the group-wide rule is the better of the two is the reasoning now in the docstring — I took the documentation suggestion and left the behaviour alone.

Verification

• 230 files / 4877 tests passed; eslint + svelte-check 0/0
• Pre-push gate green (frontend + skill + Rust + e2e)

One note for the record: the first push attempt was rejected by the gate, but its captured output was truncated to the tail and I never saw which test failed. I then ran each stage separately — test:all exit 0 with 61 Rust suites green, test:e2e exit 0 with 26 passed — and the retry succeeded with no code change in between. Reporting it as an unattributed flake rather than claiming it was nothing; a review agent was running tests against this repo concurrently, which is the contention pattern noted on #2385.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: One doc comment, one docstring, one added title argument, and a local rename verified not to be a live bug. No logic changed, no types moved, no tests touched. Your re-review already cleared the substantive commit.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

Post-approval commit: DESIGN.md conformance

One more commit after the re-review approval (b6203af1), from checking the panel against nodespace-docs/DESIGN.md rather than against the token names alone. Flagging it because it landed after the APPROVE.

Colors were already fully tokenized — no hex, rgb(), or Tailwind palette literals anywhere in the panel. But four things diverged from the documented system:

1. The rail's selected row used bg-accent. In this palette --accent is 176 100% 24% — a saturated dark teal — paired with a near-black --accent-foreground. That is a brand color and a legibility problem, not a selection surface. DESIGN.md's Sidebar spec is explicit: "navigation items use active-nav-background when active, muted on hover", and the rail is a navigation list.

Verified by reading computed styles back in the browser: the selected row is now rgb(234,234,236) in light and rgb(34,34,37) in dark — the cool-neutral hue-240 surface the sidebar uses. With bg-accent it would have been a teal row.

Worth naming why bg-accent wasn't simply wrong: nine other components use it (dropdown focus, autocomplete selection, calendar cells). The distinction is that those are all transient highlight; a rail selection is persistent structural state, which is what active-nav-background is for.

2. Hover was hardcoded as bg-muted/60. DESIGN.md: "derive hover states from --hover-background / --hover-foreground — never hardcode hover colors."

Both tokens are read via hsl(var(...)) in a scoped style block rather than Tailwind classes, because neither is mapped in the Tailwind config. The navigation sidebar reads them the same way, so this follows the existing consumer instead of adding a second access path.

3. The "Back" button used variant="ghost". DESIGN.md: "no outline/ghost variants in the current system — use secondary for low-emphasis actions." Switched. Note ghost is defined as hover:bg-accent, so it was a second route to the same teal.

4. text-[0.6875rem] (5 uses) was an arbitrary value where the scale has text-xs (0.75rem — the documented body-xs role). It was the only file in the codebase using that size; other components style the same uppercase micro-label with text-xs. Inherited from the modal this replaced, not introduced here.

Verified in both light and dark mode with computed colors read back rather than eyeballed. Tests: 230 files / 4877 passed; eslint + svelte-check clean; full pre-push gate green.

---

Separately filed: #2454

While comparing against DESIGN.md I noticed the dialog close button renders two focus indicators — the browser's default blue outline nested inside the component's grey ring.

Cause: dialog-content.svelte uses focus:outline-hidden, a Tailwind v4 class, on a v3 project (tailwindcss: ^3.4.17), where the equivalent is outline-none. An unknown utility compiles to nothing silently, so the browser outline is never suppressed. Five shared UI components are affected; thirteen others correctly use outline-none.

Not caused by this PR — dialog-content.svelte dates to the original repo-structure commit and is untouched here. Filed as #2454 because it is shared UI-kit styling affecting every dialog and dropdown in the app, and it also surfaces a contradiction in DESIGN.md worth settling (line 218/334 say focus rings are teal primary; --ring is defined as neutral grey).

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK