Add focusOrOpenNode to the navigation service (#2432 phase 1)

[malibio]

Phase 1 of #2432. The two-pane relationships panel needs to open the node at the other end of an edge without stacking a duplicate tab — and no existing navigation method does that. This PR adds it and migrates the call sites that had each grown their own copy.

The problem

Six call sites hand-rolled the same behaviour: scan state.tabs for one whose content.nodeId matches, setActiveTab if found, otherwise mint a uuid and addTab into a pane resolved by a locally-duplicated getTargetPaneId. Three carried their own copy of that pane helper; two spelled its fallback as the bare literal 'pane-1'.

Neither existing method covers it:

• navigateToNode re-points the current tab, replacing what the user was looking at
• navigateToNodeInOtherPane always calls addTab, so clicking the same node twice stacks duplicates

Tab reuse existed only as copies inside components.

The API

focusOrOpenNode(nodeId, options). The three options are the axes the call sites genuinely differ on, not generality for its own sake:

Option	Why it exists
nodeType	Written into tab content to select a viewer. Usually the node's own type, but deliberately an override where a tab routes elsewhere — a schema id opens as 'query' to reach QueryNodeViewer, a chat as 'ai-chat'
title	Defaults to 'Loading...', which viewers replace on mount. Only the collection viewer passes one, because its member row already holds the text
matchNodeType	Off by default: a node id identifies one node, so any tab showing it is the tab the user means. Daily Journal needs it because a date id is a date string, not a real node id — nothing stops an unrelated tab from carrying the same string

Unlike the other two methods it does not resolve to a navigation ancestor. These callers pass ids they mean to open as tabs — a schema, a chat, a collection member — not child nodes to scroll to.

The one call site that doesn't fit

QueryNodeViewer's row click opens its "not yet open" case in the other pane, so the result list stays visible beside the node. Rather than bend the signature around it, the focus half is exposed separately as focusExistingNodeTab, and both share one private lookup so they can't drift on what counts as "already open". #2432 explicitly says not to force every site into one signature.

Deliberately not migrated

• openSearchTab — opens a non-node tab (type: 'search', no content), so the node lookup doesn't apply
• pane-content.svelte:95 — matches one specific tab by id as a staleness guard, not to focus it
• rerouteTab (query-node-viewer) — re-points a tab it identifies by pane; that's updateTabContent, not focus-or-open

Verification

Unit: 16 new cases — open, reuse, cross-pane focus, title defaulting and preservation on reuse, the nodeType override, both matchNodeType directions, and that no ancestor resolution occurs. Suite 233 files / 4916 passed (baseline 4900). Pre-push gate green: frontend + Rust + e2e.

Browser (dev:browser against an isolated NODESPACE_HOME, real DB untouched):

• Schema click opens one tab; clicking again refocuses it without duplicating, and the resolved title survives
• Daily Journal opens "Today"; clicking again refocuses
• Query row opens in a new second pane; clicking the same row again refocuses it instead of stacking — the exact regression focusExistingNodeTab prevents
• From pane-2, clicking a node whose tab is in pane-1 switches the active pane and focuses it there
• A genuinely new node opens into the active pane

Search's openResult is covered by unit tests only: search-pane.svelte calls Tauri invoke directly for search_roots, which doesn't work in browser proxy mode. That limitation is pre-existing on main and untouched here.

Net effect

−176 lines across five components, replaced by one service method. Behaviour is unchanged at every migrated site; the sidebar's 'pane-1' fallback is now DEFAULT_PANE_ID, which is that same value under its name.

Landed separately from the panel itself because verifying tab and pane behaviour across the sidebar, search and two viewers is a different review job from reviewing a relationships panel.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

Pragmatic Code Review — PR #2441

Review Type: Initial Review (no prior reviews on this PR)

---

Requirements Check (phase 1 scope only)

#2432's phase-1 section asks for one extracted navigation-service method plus migration of the listed call sites. Measured against that table:

#2432 call site	Status
navigation-sidebar.svelte — 3 plain sites (node, schema, chat)	✅ migrated to focusOrOpenNode
navigation-sidebar.svelte:228 — variant, also matches nodeType === 'date'	✅ migrated via matchNodeType: true
search-pane.svelte	✅ migrated
collection-node-viewer.svelte	✅ migrated (+ local getTargetPaneId deleted)
query-node-viewer.svelte:527	✅ migrated via focusExistingNodeTab
query-node-viewer.svelte:310 (rerouteTab) — variant, pane-constrained	✅ correctly left alone
pane-content.svelte:95	✅ correctly excluded

The issue's explicit instruction — "The two variants need an optional nodeType filter and pane constraint, or they keep their own logic deliberately. Decide when implementing; don't force all of them into one signature" — was followed, and both halves of it were exercised: variant one got the optional filter (matchNodeType), variant two kept its own logic. That is the right split, and I verified rerouteTab independently: it locates a tab by (nodeId, paneId) in order to call updateTabContent on it, never setActiveTab. It is a re-point, not a focus. Folding it into focusOrOpenNode would have required a pane parameter that no other caller wants, to serve a method that does not focus anything.

I also verified the two non-migrated exclusions rather than taking the PR body's word:

• openSearchTab (navigation-sidebar) creates { id: 'search', type: 'search' } with no content field at all. focusNodeTab matches on tab.content?.nodeId, which is structurally undefined here — the lookup genuinely does not apply.
• pane-content.svelte:95 is inside hydrateNode: it finds a tab by the tabId it was handed and checks tab?.content?.nodeId === nodeId as a staleness guard before closeTab. Opposite direction of lookup, opposite intent.

Both exclusions are correct, not oversights.

Phase 2 (the panel itself) is explicitly out of scope for this PR, per #2432's own "Land this as its own PR" instruction. Nothing here anticipates phase 2 speculatively, which is the right call — YAGNI applies to the seam as much as to the feature.

Verification: I ran bun run --cwd packages/desktop-app test src/tests/services/navigation-service.test.ts — 56 passed, no failures.

---

Code Review Findings

🟡 Improvements

📁 packages/desktop-app/src/lib/components/layout/navigation-sidebar.svelte:188-197 — the last getTargetPaneId copy survives, still with a hardcoded 'pane-1'

The PR body and commit message both state: "the sidebar's 'pane-1' fallback is now DEFAULT_PANE_ID, which is that same value under its name." That is not what the code does. The sidebar's local helper is still present verbatim, fallback and all:

function getTargetPaneId(): string {
  const currentState = navigationStore.state;
  const paneExists = currentState.panes.some((p) => p.id === currentState.activePaneId);
  if (paneExists) {
    return currentState.activePaneId;
  }
  return currentState.panes[0]?.id ?? 'pane-1';
}

I grepped the whole of packages/desktop-app/src/lib for pane-1: exactly two hits remain, the DEFAULT_PANE_ID definition itself (navigation.svelte.ts:42) and this line. So this is now the sole surviving magic-literal copy of a constant the codebase already names — the PR removed the other two and left the one it claims to have fixed.

To be fair on severity: the helper itself cannot simply be deleted, because openSearchTab (correctly not migrated) still calls it. It is a genuine remaining caller, not dead code. But the fix is small and available: import { DEFAULT_PANE_ID } and swap the literal, which is what the commit message already promises. Better still, since NavigationService.targetPaneId() is now the canonical version of this exact logic, consider exposing it (a public targetPaneId() on the service) so openSearchTab uses the one implementation too — that would retire the last copy rather than merely correcting its constant.

Principle: DRY, and — more sharply here — the commit message is part of the change. A reviewer or future archaeologist reading "the fallback is now DEFAULT_PANE_ID" will not re-grep to check. A claim in a commit message that the code does not honor is a small correctness debt in the project's own history, and it is the kind that compounds because nobody re-verifies it.

📁 packages/desktop-app/src/lib/services/navigation-service.ts:396-401 — focusExistingNodeTab is a pass-through wrapper that adds a second public name for one behavior

focusExistingNodeTab(nodeId: string): boolean {
  return this.focusNodeTab(nodeId);
}

The task brief asks specifically whether exposing two public methods is justified. My read: the split is justified, the wrapper is not.

The split is justified because QueryNodeViewer's fall-through is navigateToNodeInOtherPane, not addTab into the active pane. Modelling that as an option on focusOrOpenNode would mean a boolean like openInOtherPane that changes not just where the tab lands but which method runs — including ancestor resolution, pane creation, and scrollToNode, none of which focusOrOpenNode does. That is a flag argument controlling two genuinely different code paths, which is worse than two names. Good judgment here.

What is unnecessary is the third method. focusExistingNodeTab and the private focusNodeTab are the same function; the private one exists only because the public one drops the nodeType parameter. Two options, both simpler than three methods:

1. Make focusNodeTab public with its optional second parameter and delete the wrapper — the parameter is already optional, so the QueryNodeViewer call site is unchanged.
2. Keep the wrapper but give the private one a name that says it is the shared primitive (findAndFocusTab), so a reader hitting focusNodeTab vs focusExistingNodeTab in autocomplete can tell which is which.

As it stands, three near-identically-named methods (focusNodeTab, focusExistingNodeTab, focusOrOpenNode) sit in one class, and the difference between the first two is invisible at the call site. The doc comments carry the distinction, which means the names are not carrying it.

Principle: KISS / naming as the primary documentation. When two symbols differ only by a word that does not describe the difference, the reader must consult prose — a cost paid on every future read.

🟢 Nitpicks

📁 packages/desktop-app/src/lib/services/navigation-service.ts:438-454 — matchNodeType is correct but is a workaround for a modelling problem worth naming

Daily Journal's use of it is correct and necessary, and I want to be explicit about that since the brief asks. I confirmed the mechanism: createInitialTabState() (navigation.svelte.ts:47-73) seeds a tab whose content.nodeId is getTodayDateId() — a bare YYYY-MM-DD string. Date ids are not UUIDs and share a namespace with nothing that guarantees uniqueness, so an unqualified id match genuinely could focus a non-date tab carrying the same string. The old findTodayDateTab filtered on nodeType === 'date' for exactly this reason, and dropping that filter would have been a silent behavior change. Preserving it is right.

The nit is that matchNodeType frames a date-id collision hazard as a general option on a general method. Every future caller now has to decide about a flag that exists for one caller's identifier-namespace quirk, and the honest answer for all of them is "no". The option's own doc comment says as much. Not worth reworking in this PR — the alternative (fixing date-node identity) is a much larger change and rightly out of scope — but it is worth a note that this flag is a symptom, not a feature.

Nit — 📁 packages/desktop-app/src/tests/services/navigation-service.test.ts:829 — stray double blank line before describe('NavigationService - focusOrOpenNode'). (Note: prettier --check flags four of the changed files, but I verified the same four are already flagged on origin/main — this is pre-existing config drift, not something this PR introduced. The blank line is a manual observation, not a lint failure.)

---

Areas verified clean

I checked these specifically and found no issues worth filing:

• Behavioral equivalence at each migrated site. Walked all six. Pane targeting, setActiveTab(id, paneId) arguments, type: 'node', closeable: true, and the addTab(tab, true) make-active flag are identical to what each site did inline. The sidebar node/schema/chat sites and search-pane all previously used the unqualified content?.nodeId match and still do. Collection viewer's member.content || 'Untitled' title is preserved through the new title option. The one site with extra logic (Daily Journal) kept it. No silent changes found.
• focusOrOpenNode skipping ancestor resolution — correct, not a latent bug. This deserved the scrutiny the brief gave it. resolveWithNavigationAncestor exists to walk a primitive child node up to the ancestor that owns a viewer, then scrollToNode the child. Every focusOrOpenNode caller passes an id that is already a viewer-owning root: a schema, an ai-chat, a date, a collection member, or a search result. Resolution would be a no-op at best; at worst it would be wrong for the schema case, where the id is deliberately opened under a different nodeType ('query') than the node's own — an ancestor walk would fight that override. Notably, the search-pane and collection sites also did not resolve ancestors before this PR, so nothing regressed. It is also synchronous, where both other methods are async — a real ergonomic win for click handlers, and only possible because resolution is skipped. There is a test asserting the non-resolution ('does not resolve to a navigation ancestor'), which pins the decision so a future refactor cannot quietly add it.
• Test coverage. 16 new cases, and the assertions are meaningful rather than tautological — several would catch real regressions. The strongest: cross-pane focus asserts three things at once (no duplicate tab, activePaneId switched, correct activeTabIds entry), which is the behavior that would break if setActiveTab's second argument were dropped; 'does not overwrite the existing tab title when reusing it' pins reuse semantics that a naive "update the tab on focus" refactor would violate; and both matchNodeType directions are covered, not just the positive. The ancestor test builds real store + structure-tree state rather than mocking the resolver, so it tests the actual path.
• Dead imports. Verified clean at every touched site: uuid, addTab, setActiveTab, navigationStore are each removed exactly where they became unused and retained exactly where a remaining caller needs them (navigationStore + setActiveTab + addTab in the sidebar for openSearchTab; navigationStore + updateTabContent in query-node-viewer for rerouteTab). No orphaned helpers except the getTargetPaneId case above, which has a live caller.
• Standards. No lint suppressions, no any, no {@html}, no raw console.* in the changed files. No backward-compat shims, no deprecation markers, no dead code left behind. Bun-only throughout.
• Net effect. −176 lines across five components for one service method, with a genuine reduction in duplicated state-reading logic. This is the shape a good extraction has.

---

Recommendation

APPROVE

This is a clean, well-scoped extraction that does exactly what #2432 phase 1 asks and stops there. The design judgment on the hard call — refusing to force QueryNodeViewer's cross-pane fall-through into a boolean flag — is correct, and the documentation of why each option exists is unusually good; it is the kind of comment that survives contact with a future reader.

Neither finding is a blocker. The 'pane-1' literal is a one-line fix that would make the commit message true, and I would take it before merge since the claim is already written. The focusExistingNodeTab wrapper is a naming question that can be settled either way without changing behavior. Both are net-positive polish on a change that is already a clear improvement to code health.

[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 three findings addressed

✅ 🟡 The commit message was wrong about 'pane-1'

You were right, and the claim was mine: the previous commit message said "the sidebar's 'pane-1' fallback is now DEFAULT_PANE_ID", and it wasn't — the sidebar's local getTargetPaneId survived untouched. The PR removed two copies of that helper and left the third, which happened to be the one the message described.

Took the stronger option you offered rather than the literal swap: NavigationService.targetPaneId() is now public, and openSearchTab calls it. That retires the last copy instead of correcting its constant.

$ grep -rn "'pane-1'" packages/desktop-app/src/lib/
packages/desktop-app/src/lib/stores/navigation.svelte.ts:42:export const DEFAULT_PANE_ID = 'pane-1';

One occurrence left in the frontend — its own definition.

✅ 🟡 focusExistingNodeTab wrapper removed

Agreed on both halves of your read. The split stays for the reason you gave: QueryNodeViewer's fall-through is navigateToNodeInOtherPane, and modelling that as an option would mean a boolean selecting which method runs — including ancestor resolution, pane creation and scrollToNode.

The third method is gone. focusNodeTab is now public with its already-optional nodeType parameter, so the QueryNodeViewer call site changed by name only. Two methods, not three, and the remaining names differ by what they actually do.

✅ 🟢 matchNodeType documented as a symptom

Kept as-is — the real fix is date-node identity, well outside this PR. But the option's doc comment now says so explicitly, including that it should be deleted along with its only caller if date ids stop being bare YYYY-MM-DD strings. Your point was that the flag reads as general when it isn't, and the doc comment is where a future caller actually meets it.

Also collapsed the stray double blank line.

---

Additional commit: repairing a break on main

Not part of #2432. Included here only because it blocked every push, including this branch's.

After rebasing onto main (to pick up #2437), the pre-push gate failed:

Relationship 'held_at' (entry 0) is missing "reverseName" and "reverseCardinality".
error: test failed, to rerun pass `-p nodespace-core --test object_valued_property_roundtrip_test`

Two PRs that were individually correct and jointly broken: #2436 added object_valued_property_roundtrip_test with a Concert schema declaring held_at → venue and no reverse half; #2437 then made those fields required. Each passed its own gate against the main it branched from; neither ran against the other.

Confirmed it isn't mine before touching it — this branch changes six frontend files and zero Rust.

The fixture now names the edge from both ends: read from a venue the concerts held there are "concerts", and a venue may host many, so reverseCardinality: "many" while the forward stays "one". That's a modeling choice, which is precisely why #2437 requires an author to make it.

Scoped deliberately: I scanned every relationship declaration across the Rust test, bench and CLI-test suites for the same omission. held_at is the only one, so this is the whole repair.

---

Verification

• Frontend: 233 files / 4916 tests passed
• eslint + svelte-check: 0 errors, 0 warnings
• Pre-push gate green end to end: frontend + skill + Rust + e2e
• Rebased onto main (2f664ab2, includes Require reverseName and reverseCardinality so a relationship edge is named from both ends (closes #2433) #2437) with no conflicts

One note for the record: an earlier full-suite run showed a single failure I was unable to attribute — it didn't reproduce, and five consecutive clean runs followed. It coincided with the review agent running its own vitest against this same worktree, which is the cross-run contention pattern documented on #2385 (shared sharedNodeStore / structureTree singletons under vitest's forks pool). I'm flagging it rather than claiming certainty, since I never captured which test it was.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED — for the #2432 changes

Rationale: All three findings were mechanical and behavior-preserving. Making a private method public, deleting a pass-through wrapper, and adding a doc comment change no call-site semantics; the one caller rename is covered by the existing 56 tests, which all pass.

One caveat worth your judgment: the held_at fixture commit is a Rust change on a PR that otherwise touches only frontend files. It's a three-line test-fixture edit verified by the full gate, but if you'd rather it land under its own issue with its own attribution, say so and I'll split it out.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PtzaT8FoAP9ut43eveHUdK

[malibio]

Note on overlap with #2442: that PR fixes the same held_at fixture, and its change is character-identical to the one here (reverseName: "concerts", reverseCardinality: "many") — we reached the same modeling answer independently.

Leaving my commit in place per instruction. Once #2442 lands, the identical hunk resolves cleanly on rebase and my commit becomes an effective no-op; no conflict to hand-resolve.

#2442 also fixes the root cause of something I hit here: the post-checkout hook running bun install mid-rebase, which rewrites bun.lock (the configVersion key) and dirties the tree before the first commit replays — "cannot rebase: You have unstaged changes". I worked around it manually twice in this session by reverting the lockfile; that hook fix removes the need.

[malibio]

Correction — the held_at commit has been dropped from this branch.

My two previous comments described carrying that fixture fix here. That was wrong: #2442 is landing with the identical change plus the post-checkout hook fix, so this PR should not have carried a redundant copy. Removed.

This PR is now frontend-only again — six files, zero Rust:

packages/desktop-app/src/lib/components/layout/navigation-sidebar.svelte
packages/desktop-app/src/lib/components/search/search-pane.svelte
packages/desktop-app/src/lib/components/viewers/collection-node-viewer.svelte
packages/desktop-app/src/lib/components/viewers/query-node-viewer.svelte
packages/desktop-app/src/lib/services/navigation-service.ts
packages/desktop-app/src/tests/services/navigation-service.test.ts

The three review findings remain addressed as described above; that part is unchanged.

Consequence: this branch can't be pushed again until #2442 lands, because the pre-push gate runs the full Rust suite against a main that is currently red. The remote is already at the reviewed state, so nothing is pending — this only matters if further commits are needed here.

Disregard the "Additional commit: repairing a break on main" section of my address-review summary and the follow-up comment about #2442 overlap. #2442 owns that fix.