[malibio]

Summary

• waitForNodeSaves returns the set of failed/timed-out nodes instead of throwing; the createNode child-transfer block ignored it and still called persistChildTransfer against a parent the daemon may not have.
• Now throws when the new node is in the failed set, so the existing rollback path reverts the on-screen move and emits child-transfer-failure.

Tests

• New test: waitForNodeSaves reports the new node failed → moveChildrenToParent never called, optimistic move reversed, notification emitted. Verified it fails with the fix disabled.
• reactive-node-service.test.ts: 115 passed. Baseline: 5392 frontend tests passed.
• Pushed with --no-verify at the user's request (the pre-push gate was taking too long on a cold worktree); quality:fix is clean.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3081

Review Type: Initial Review

Summary

This is a small, well-scoped fix. waitForNodeSaves reports failures through its return value and never throws, so the createNode child-transfer block now checks the returned set and throws into the rollback path that already existed. As a result, a failed or timed-out save of the new node never reaches persistChildTransfer. The on-screen move is reverted and the child-transfer-failure notification is shown. This is the smallest correct fix (KISS). It reuses the existing rollback and notification path and adds no second one (DRY).

I ran the test file locally: reactive-node-service.test.ts passed 115/115.

Requirements Check

• ✅ A failed or timed-out save of the new node skips the child transfer and rolls back the on-screen move. waitForNodeSaves → settleNodes puts the node in the failed set both when its save rejects and when it loses the timeout race. The fix throws before persistChildTransfer. The catch block then reverses every moveInMemoryRelationship and calls notifyChildTransferFailure.
• ✅ A test covers this failure path. The new test checks that the optimistic move happened, that the reverse move happened, that moveChildrenToParent was never called, and that the child-transfer-failure notification was shown. The author confirmed the test fails when the fix is disabled.

Code Review Findings

Critical Issues

None.

Suggested Improvements

None that block merge.

Nitpicks

• 🟢 Nit: 📁 packages/desktop-app/src/lib/services/reactive-node-service.svelte.ts:370. When the save fails, the shared catch block logs Failed to transfer children to database, rolling back: Error: Save of new node … failed or timed out. The thrown message still points to the real cause, so debugging is not harmed. However, the prefix says "transfer" when no transfer was attempted. A more neutral prefix such as Child transfer aborted or failed, rolling back: would describe both cases accurately.
• 🟢 Nit: 📁 packages/desktop-app/src/tests/services/reactive-node-service.test.ts (new test). The spies are restored after the vi.waitFor assertion, so a failing assertion skips mockRestore(). This does not leak into other tests, because beforeEach calls SharedNodeStore.resetInstance() and so each test spies on a new instance. moveInMemoryMock.mockClear() only resets call history on the module-level vi.fn, so no implementation leaks either. Test isolation is sound. Moving the restores into try/finally (or relying on the reset alone) would make that clearer, but it is optional.
• 🟢 Nit: The test name says "fails or times out", but only one input is simulated: a returned failed set. That is the right level to test at, because settleNodes merges failure and timeout into one set by design. No change is needed.

Interaction check

• Compared with outdentNode (~L1180): that path detects a failed CREATE through isNodePersisted after flushAllPendingSaves, not through the returned set. The two approaches differ but both work: outdentNode has to handle the case where the node was moved, not created. The two paths do not share state, and this change leaves outdentNode untouched.
• Races: the check happens inside the existing microtask, before any RPC runs. The rollback still reverses exactly the children snapshot taken synchronously. The change adds no new window.
• Project standards: it uses createLogger (log.error), the comment explains why and cites no issue number, and nothing suppresses lint.

Recommendation

APPROVE. The change clearly improves correctness: a child can no longer end up with a parent that the daemon may not have, and the test covers exactly the gap described in the issue.

[malibio]

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

[malibio]

Review addressed (79e6a79)

• ✅ Log prefix → Child transfer aborted or failed, rolling back: — accurate for both the save-failure and RPC-failure paths.
• ✅ New test restores its spies in try/finally.
• ⏭️ Test-name nit: no change — faking the failed set is the right level since settleNodes reports failure and timeout identically.

Tests: reactive-node-service.test.ts 115/115; quality:fix clean. Pushed with --no-verify per user request.

Re-review: not needed — log string and test-cleanup changes only, no logic change.