Fix CREATE operation race condition that overwrote user typing

[malibio]

Summary

• Fixed race condition where user content was overwritten when CREATE operation completed
• Added regression tests to prevent this bug from recurring

Problem

When a user typed content while a CREATE operation was in flight, the persistence code would overwrite their new content with stale data captured at the start of the operation.

Bug scenario:

1. User creates new node with content ## (header)
2. User deletes ## (becomes text node)
3. User presses Tab to indent
4. User starts typing "Hello World"
5. CREATE operation completes and fetches version from backend
6. nodes.set() was called with stale currentNode from step 1
7. User's typing ("Hello World") was overwritten with ##

Solution

After CREATE completes, read fresh node state from store and only update the version field, preserving user's local content changes.

Applied fix to two code paths:

• setNode path for new nodes (lines ~1172-1176)
• commitBatch path (lines ~2518-2521)

Also includes refactoring of SimplePersistenceCoordinator to queue operations properly, ensuring DELETE operations aren't lost when queued during an executing UPDATE.

Test plan

• Added 3 regression tests for the race condition
• All 3472 tests pass
• Manually verified fix works in the app

🤖 Generated with Claude Code

[malibio]

Code Review: PR #840 - Fix CREATE Operation Race Condition

Review Type

Initial Review (No prior reviews found)

---

Summary Assessment

This PR addresses a critical data loss bug where user-typed content was being overwritten when CREATE operations completed asynchronously. The fix is well-targeted, correctly preserves user content during race conditions, and includes comprehensive regression tests.

Overall Verdict: APPROVE - This is a high-quality bug fix that solves a real user-facing issue with minimal, focused changes.

---

Requirements Validation

Requirement	Status
Fix race condition where content is overwritten	Passed
Applied to setNode path (lines ~1172-1176)	Passed
Applied to commitBatch path (lines ~2518-2521)	Passed
3 regression tests added	Passed
All tests pass	Passed (claimed 3472)

---

Code Review Findings

Architectural Design & Integrity

[APPROVE] The fix demonstrates excellent understanding of the existing architecture:

1. Minimal, surgical change - Only modifies the specific code paths where the bug occurs
2. Consistent pattern - The same fix is applied identically in both affected locations (setNode and commitBatch paths)
3. Preserves existing abstractions - Works within the existing persistence coordinator pattern

The refactoring of SimplePersistenceCoordinator from needsRerun (Set) to queuedOperations (Map storing full operation context) is the right architectural choice - it preserves operation semantics instead of blindly re-running potentially stale operations.

Functionality & Correctness

[APPROVE] The fix correctly addresses the root cause:

Before (Bug):

currentNode.version = createdNode.version;
this.nodes.set(nodeId, currentNode); // Overwrites with stale snapshot

After (Fix):

const latestLocalNode = this.nodes.get(nodeId);
if (latestLocalNode) {
  latestLocalNode.version = createdNode.version;
  this.nodes.set(nodeId, latestLocalNode);
}

The fix correctly:

• Reads the current state from the store at completion time (not the stale captured state)
• Only updates the version field (which came from backend)
• Preserves all local changes (content, nodeType, etc.)
• Handles the null case defensively

Security

No security concerns - this is an internal state management fix that doesn't introduce any new attack vectors, data exposure, or authentication concerns.

Maintainability & Readability

Suggestions:

1. [Nit] /Users/malibio/nodespace/nodespace-core-dev2/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:1132 - There's a double blank line after return; that appears to be accidental:

                return;
              }


              // Check if node has been persisted...

2. The BUG FIX: comments are excellent - they explain the why clearly and will help future developers understand the non-obvious nature of this fix.

Testing Strategy & Robustness

[APPROVE] The test coverage is comprehensive and well-structured:

1. should preserve user typing when CREATE completes (setNode path) - Tests the primary bug scenario with delayed createNode
2. should preserve user typing when batch CREATE completes (commitBatch path) - Tests the batch persistence path
3. should handle rapid typing with multiple content changes during CREATE - Tests the edge case of many rapid changes

The tests correctly:

• Use controlled promises to simulate timing
• Verify intermediate states
• Check final content preservation
• Verify version is still updated from backend

Performance & Scalability

No performance concerns. The fix adds one additional Map.get() call per CREATE completion, which is O(1) and negligible.

---

Minor Observations

Extra Blank Line

[Nit] Line 1132 has an extra blank line that should be removed for consistency.

Test Timeout Considerations

The tests use setTimeout delays (5ms, 10ms, 700ms) which could be flaky in CI under load. However, given the nature of testing async race conditions, this is acceptable and the delays are reasonable.

---

Final Recommendation

APPROVE

This PR:

• Correctly identifies and fixes a critical data loss bug
• Makes minimal, focused changes
• Applies the fix consistently across all affected code paths
• Includes comprehensive regression tests
• Improves the persistence coordinator's operation queueing semantics

The fix demonstrates solid engineering judgment - it solves the immediate problem while also improving the underlying infrastructure (operation queueing) to prevent similar issues in the future.

---

Reviewed by: Principal Engineer AI Reviewer
Review Framework: Pragmatic Quality

[malibio]

Code review complete - recommending APPROVE. See full review comment above.

[malibio]

Review Recommendations Addressed

Summary

Status	Count	Description
✅ Addressed	1	Nit-level formatting fix
⏭️ Skipped	0	None

Changes Made

🟢 Nit: Extra blank line at line 1132

• Removed accidental double blank line in shared-node-store.svelte.ts
• Commit: 34c493bd

Verification

• ✅ All 3472 tests passing
• ✅ Quality checks (ESLint, svelte-check, cargo clippy) passing
• ✅ No regressions introduced

---

Addressed by: Implementation Agent