[malibio]

Root cause

NavigationService.resolveNodeTarget() puts a fetched node into the store with setNode(node, { type: 'database' }, true). In determinePersistenceBehavior, skipPersistence (priority 2) ranked above the database source (priority 4) and returned shouldMarkAsPersisted: false. The node landed in the store but never in persistedNodeIds.

doLoadChildrenTree then skips a root that is already cached, so nothing marked it later either. The first edit took the create path, and the daemon rejected the duplicate insert (POST /api/nodes → 500).

Fix

skipPersistence now only suppresses the write. A database source still marks the node persisted.

The same database + skipPersistence=true pairing is used by five other ingress paths, all of which had the same latent defect:

• browser-sync-service.ts (SSE sync)
• tauri-sync-listener.ts (domain events)
• ai-chat-node-viewer.svelte (poll)
• node-card-inline.svelte
• navigation-service.ts

Fixing the precedence covers all of them in one place, with no per-write existence query.

Tauri mode: also affected. navigation-service doesn't depend on the transport, and tauri-sync-listener uses the same pairing.

Verification

• New navigated-root-persisted-regression.test.ts: navigation fetch → loadChildrenTree → root is in persistedNodeIds → updateNode calls backendAdapter.updateNode, never createNode. Fails without the fix (expected false to be true), passes with it.
• Browser mode against an isolated daemon, following the issue's repro: after editing Last name, the request is PATCH /api/nodes/<id> → 200, with no POST and no "Database write failed". GET /api/nodes/<id> returns last_name: "Byron" at version 2.
• Frontend suite: 5287 tests pass. Two timing checks in architecture-benchmarks failed under load on the first run and passed 13/13 in isolation. The pre-push gate passed.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #2944 (closes #2940)

Review Type: Initial review. The PR had no prior reviews or comments.

Requirements Check

• ✅ Load path that leaves the root untracked is identified and fixed. NavigationService.resolveNodeTarget() calls setNode(node, {type:'database'}, true), and skipPersistence (priority 2) outranked the database source (priority 4). doLoadChildrenTree skips a root that is already cached, so nothing marked the root later. The fix is in determinePersistenceBehavior, the one place that decides this, so all six ingress paths using this pairing are fixed together.
• ✅ Browser-mode edit issues an update and survives a reload. Verified against an isolated daemon: PATCH /api/nodes/<id> returned 200, there was no POST, and GET returned the new value at version 2.
• ✅ Tauri mode checked. This was checked by analysis, not a runtime run. The analysis holds: the defect is in the transport-independent store and navigation-service, and tauri-sync-listener uses the same pairing. So Tauri is affected, and this fix covers it.
• ✅ Regression test. navigated-root-persisted-regression.test.ts drives the navigation fetch, then loadChildrenTree, then asserts isNodePersisted, then checks that updateNode calls backendAdapter.updateNode and never createNode. It fails without the fix.
• ✅ quality:fix. Passed as part of the pre-push gate.
• ✅ Tests pass. 5287 frontend tests pass and the pre-push gate passed. The architecture-benchmarks timing flake under load is a known issue unrelated to this PR.

Code Review Findings

I checked whether marking any of the affected ingress paths as persisted could be wrong. I found no case where it is.

• Blast radius is limited to setNode and batchSetNodes. shouldMarkAsPersisted is only read at shared-node-store.svelte.ts:2412 (setNode) and :2827 (batchSetNodes). updateNode (:1646) and deleteNode (:2900) read only shouldPersist, which is still false under skipPersistence. The isComputedField → skipPersistence path in updateNode is therefore unaffected. batchSetNodes has one caller (:3696), and it does not pass skipPersistence.
• Virtual date nodes stay consistent. For a YYYY-MM-DD id the backend's getNode returns a virtual date. The other ingress paths already mark virtual dates as persisted through a database source without skip: ensureNode/fetchAndCacheNode (:1463, synthesized at :1485) and doLoadChildrenTree (:3632). Navigation was the one path that did not, so this change removes an inconsistency rather than adding one.
• Deleted or tombstoned nodes are not made worse. Every sync ingress calls setNode only when getNode returns a row. The Tauri flush loop also skips tombstonedDuringFlush (tauri-sync-listener.ts:161). The remaining window, a delete landing between getNode resolving and setNode running, already existed and already resurrected the node in the store. With this change, an edit to such a node goes down the update path and fails loudly (404). Before, it would have silently recreated a deleted row. That is safer.
• The viewer placeholder contract is kept. viewer + skipPersistence (the initial placeholder in reactive-node-service.svelte.ts:262 and :1642) still returns shouldMarkAsPersisted: false. shared-node-store.test.ts:2407 still asserts this.
• isNewNode and the skip-while-editing guard don't interact badly. isNewNode (:2318) is computed before the mark, and it only gates the shouldPersist branch, which is false for database sources anyway. The guard-skip branch (:2395) already added the node to persistedNodeIds for database events. That branch and the apply branch now agree, where before they disagreed.

🟡 Suggested Improvements

• 🟡 The rationale comment in the ai-chat workaround is now wrong.
  📁 packages/desktop-app/src/lib/stores/ai-chats.svelte.ts:130-141
  The comment says the domain-event echo "sets skipPersistence: true and therefore never marks it persisted". After this PR, that echo does mark the node persisted. The explicit setNode(created, {type:'database'}) seed is still worth keeping, because it marks the node before the tab opens instead of waiting for the echo, which removes a race. But the stated reason is now false, and a future reader could draw the wrong conclusion from it: either delete a still-useful seed, or copy the workaround to new call sites. Reword it to the real reason: seed synchronously so the first write doesn't race the echo. (Principle: comments must explain the current why; a stale rationale is worse than none.)

🟢 Nitpicks

• 🟢 Nit: The determinePersistenceBehavior JSDoc is out of date.
  📁 packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:1216-1227
  Priority item 2 still reads "options.skipPersistence - Skip persistence", which is exactly the ambiguity that caused this bug. Make it say "suppresses the write; a database source still marks persisted". The header's "backward compatibility with the legacy source.type === 'database' checks" wording also goes against the project's no-back-compat framing. This was already there before the PR, but it is a one-line fix while you're in the function.
• 🟢 Nit: The navigation comment could name persisted tracking.
  📁 packages/desktop-app/src/lib/services/navigation-service.ts:141-145
  "already in backend or virtual" is now accurate. Consider adding that the database source is what puts the node in persistedNodeIds, so nobody "simplifies" it to a viewer source later.
• 🟢 Nit: A direct unit test would pin the contract more cheaply.
  📁 packages/desktop-app/src/tests/services/navigated-root-persisted-regression.test.ts
  The integration-style test is the right regression guard for the reported path. A short unit assertion next to it would pin the precedence contract itself: setNode(n, {type:'database'}, true) leads to isNodePersisted === true, next to the existing viewer+skip → false case in shared-node-store.test.ts. It needs no navigation or tree mocks and would catch a precedence regression coming from any of the six callers.

Recommendation

APPROVE

The fix is a single-line precedence correction in the one function that owns the decision. It restores the documented persistedNodeIds contract (a backend-existing node is tracked as persisted) for all six ingress paths, adds no per-write existence query, and is covered by a regression test that fails without it. I checked the risky interactions (virtual dates, tombstones, updateNode/deleteNode, viewer placeholders, the isNewNode and skip-while-editing guards) and found no case where marking a node persisted is wrong. The stale ai-chat comment is worth fixing, but it does not block the merge.

[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 (81c0d15)

✅ Addressed (4/4):

• 🟡 ai-chats.svelte.ts: rewrote the stale rationale. The domain-event echo now marks the node persisted. The seed stays because the viewer's first write can race ahead of that echo.
• 🟢 determinePersistenceBehavior JSDoc: priority 2 now says it suppresses the write and that a database source is still marked persisted. Removed the "backward compatibility / legacy" wording.
• 🟢 navigation-service.ts: the comment now says the database source is what records the node in persistedNodeIds.
• 🟢 shared-node-store.test.ts: added a unit test pinning the contract directly (database source + skipPersistence → isNodePersisted === true), next to the existing viewer + skip → false case.

⏭️ Skipped: none.

Tests: the three affected files pass (218 tests). quality:fix is clean and the pre-push gate passed.

Re-review decision: not needed. The changes are comments plus one additive unit test, with no logic changes.