diff --git a/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts b/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts index e78df5fb5..204ea8d31 100644 --- a/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts +++ b/packages/desktop-app/src/lib/services/shared-node-store.svelte.ts @@ -58,13 +58,19 @@ interface PendingOperation { const coordLog = createLogger('PersistenceCoordinator'); +/** Pending operation to run after current execution completes */ +interface QueuedOperation { + operation: () => Promise; + options: { mode: 'immediate' | 'debounce'; dependencies?: Array Promise)> }; +} + class SimplePersistenceCoordinator { private static instance: SimplePersistenceCoordinator | null = null; private pendingOperations = new Map(); private executingOperations = new Set(); // Track in-flight operations // Track nodes that need re-persistence after current operation completes - // This prevents scheduling multiple operations while one is executing - private needsRerun = new Set(); + // Stores the QUEUED operation so DELETE isn't overwritten by UPDATE re-run + private queuedOperations = new Map(); private readonly DEBOUNCE_MS = 500; private operationCounter = 0; // For tracking operation IDs @@ -96,15 +102,17 @@ class SimplePersistenceCoordinator { `hasPending=${hasPending}, isExecuting=${isExecuting}` ); - // If an operation is already executing for this node, just mark it for re-run + // If an operation is already executing for this node, queue the new operation // This prevents multiple concurrent database operations and OCC conflicts + // CRITICAL: We store the NEW operation so DELETE isn't lost when UPDATE is executing if (isExecuting) { - this.needsRerun.add(nodeId); + // Queue the new operation - it will supersede any previously queued operation + this.queuedOperations.set(nodeId, { operation, options }); coordLog.debug( - `[op#${opId}] operation already executing for ${shortNodeId}, marked for re-run` + `[op#${opId}] operation already executing for ${shortNodeId}, queued new operation (mode=${options.mode})` ); // Return a promise that resolves when the current operation completes - // The re-run will happen automatically after completion + // The queued operation will run automatically after completion const existingPending = this.pendingOperations.get(nodeId); if (existingPending) { return { promise: existingPending.promise }; @@ -154,14 +162,17 @@ class SimplePersistenceCoordinator { this.pendingOperations.delete(nodeId); this.executingOperations.delete(nodeId); - // Check if we need to re-run for this node (content changed while executing) - if (this.needsRerun.has(nodeId)) { - this.needsRerun.delete(nodeId); - coordLog.debug(`[op#${opId}] re-running persistence for ${shortNodeId} (content changed during execution)`); - // Schedule a new debounced operation - it will read current state - // Use setTimeout to avoid stack overflow from immediate recursion + // Check if there's a queued operation for this node (e.g., DELETE queued during UPDATE) + const queued = this.queuedOperations.get(nodeId); + if (queued) { + this.queuedOperations.delete(nodeId); + coordLog.debug( + `[op#${opId}] executing queued operation for ${shortNodeId} (mode=${queued.options.mode})` + ); + // Execute the queued operation - use setTimeout to avoid stack overflow + // CRITICAL: Use the QUEUED operation, not the original one setTimeout(() => { - this.persist(nodeId, operation, { mode: 'debounce' }); + this.persist(nodeId, queued.operation, queued.options); }, 0); } } @@ -1154,8 +1165,14 @@ export class SharedNodeStore { // This prevents version conflicts on subsequent updates const createdNode = await tauriCommands.getNode(nodeId); if (createdNode) { - currentNode.version = createdNode.version; - this.nodes.set(nodeId, currentNode); // Update local node with backend version + // BUG FIX: Only update the VERSION, not the entire node! + // The user may have continued typing while createNode was in flight. + // We must preserve their local changes and only take the version from backend. + const latestLocalNode = this.nodes.get(nodeId); + if (latestLocalNode) { + latestLocalNode.version = createdNode.version; + this.nodes.set(nodeId, latestLocalNode); + } } } } catch (dbError) { @@ -2494,8 +2511,14 @@ export class SharedNodeStore { // This prevents version conflicts on subsequent updates const createdNode = await tauriCommands.getNode(nodeId); if (createdNode) { - finalNode.version = createdNode.version; - this.nodes.set(nodeId, finalNode); // Update local node with backend version + // BUG FIX: Only update the VERSION, not the entire node! + // The user may have continued typing while createNode was in flight. + // We must preserve their local changes and only take the version from backend. + const latestLocalNode = this.nodes.get(nodeId); + if (latestLocalNode) { + latestLocalNode.version = createdNode.version; + this.nodes.set(nodeId, latestLocalNode); + } } } catch (createError) { // If CREATE fails (node already exists from race), try UPDATE with batched changes diff --git a/packages/desktop-app/src/tests/services/shared-node-store-coverage.test.ts b/packages/desktop-app/src/tests/services/shared-node-store-coverage.test.ts index 2c80f48bd..117a3ed39 100644 --- a/packages/desktop-app/src/tests/services/shared-node-store-coverage.test.ts +++ b/packages/desktop-app/src/tests/services/shared-node-store-coverage.test.ts @@ -621,6 +621,195 @@ describe('SharedNodeStore - Coverage Completion', () => { }); }); + // ======================================================================== + // CREATE Operation Race Condition (Issue: content overwritten mid-typing) + // ======================================================================== + + describe('CREATE Operation - User Typing Race Condition', () => { + /** + * REGRESSION TEST: Content overwritten during CREATE 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. BUG: nodes.set() was called with stale `currentNode` captured at step 1 + * 7. User's typing ("Hello World") was overwritten with stale content ("## ") + * + * Fix: Only update the VERSION from backend, preserve local content + */ + it('should preserve user typing when CREATE completes (setNode path)', async () => { + let createNodeResolve: (value: string) => void; + const createNodePromise = new Promise((resolve) => { + createNodeResolve = resolve; + }); + + // Mock createNode to delay so we can simulate user typing during the operation + vi.spyOn(tauriCommands, 'createNode').mockImplementation(async () => { + return createNodePromise; + }); + + vi.spyOn(tauriCommands, 'getNode').mockResolvedValue({ + ...mockNode, + id: 'typing-race-node', + content: '## ', // Backend has OLD content + nodeType: 'header', + version: 1 + }); + + // Step 1: Create new node with initial content (NOT persisted yet) + // The node is new because persistedNodeIds doesn't have it + const newNode = { + ...mockNode, + id: 'typing-race-node', + content: '## ', + nodeType: 'header' as const, + version: 0 + }; + store.setNode(newNode, viewerSource); // Will trigger CREATE persistence + + // Step 2: Simulate user typing WHILE createNode is in flight + // This updates the store directly (as the UI would via updateNode) + // The store update happens immediately, but CREATE is still pending + await new Promise(resolve => setTimeout(resolve, 10)); + + // Directly modify the store's node to simulate what the UI does + // (updateNode would queue its own persistence which complicates the test) + const nodeInStore = store.getNode('typing-race-node'); + if (nodeInStore) { + nodeInStore.content = 'Hello World'; + nodeInStore.nodeType = 'text'; + store.nodes.set('typing-race-node', nodeInStore); + } + + // Verify store has user's typed content + const beforeCreate = store.getNode('typing-race-node'); + expect(beforeCreate?.content).toBe('Hello World'); + expect(beforeCreate?.nodeType).toBe('text'); + + // Step 3: Now complete the CREATE operation + createNodeResolve!('typing-race-node'); + + // Wait for persistence to complete (debounce is 500ms for new viewer nodes) + await new Promise(resolve => setTimeout(resolve, 700)); + + // Step 4: CRITICAL CHECK - User's content MUST be preserved! + const afterCreate = store.getNode('typing-race-node'); + expect(afterCreate?.content).toBe('Hello World'); // NOT '## '! + expect(afterCreate?.nodeType).toBe('text'); // NOT 'header'! + expect(afterCreate?.version).toBe(1); // Version updated from backend + }); + + it('should preserve user typing when batch CREATE completes (commitBatch path)', async () => { + let createNodeResolve: (value: string) => void; + const createNodePromise = new Promise((resolve) => { + createNodeResolve = resolve; + }); + + vi.spyOn(tauriCommands, 'createNode').mockImplementation(async () => { + return createNodePromise; + }); + + vi.spyOn(tauriCommands, 'getNode').mockResolvedValue({ + ...mockNode, + id: 'batch-typing-race', + content: '> Quote', // Backend has OLD content from batch + nodeType: 'quote-block', + version: 1 + }); + + // Step 1: Create node with batch mode (quote-block uses batching) + const quoteNode = { + ...mockNode, + id: 'batch-typing-race', + content: '> Quote', + nodeType: 'quote-block' as const, + version: 0 + }; + store.setNode(quoteNode, viewerSource, true); + + // Start a batch operation + store.startBatch('batch-typing-race'); + store.addToBatch('batch-typing-race', { content: '> Original batch content' }); + store.commitBatch('batch-typing-race'); + + // Step 2: User types more content while batch is processing + await new Promise(resolve => setTimeout(resolve, 10)); + store.updateNode('batch-typing-race', { + content: '> User typed this after batch' + }, viewerSource); + + // Verify store has user's typed content + const beforeComplete = store.getNode('batch-typing-race'); + expect(beforeComplete?.content).toBe('> User typed this after batch'); + + // Step 3: Complete the CREATE operation + createNodeResolve!('batch-typing-race'); + + // Wait for persistence to complete + await new Promise(resolve => setTimeout(resolve, 100)); + + // Step 4: CRITICAL CHECK - User's content MUST be preserved! + const afterComplete = store.getNode('batch-typing-race'); + expect(afterComplete?.content).toBe('> User typed this after batch'); + expect(afterComplete?.version).toBe(1); + }); + + it('should handle rapid typing with multiple content changes during CREATE', async () => { + let createNodeResolve: (value: string) => void; + const createNodePromise = new Promise((resolve) => { + createNodeResolve = resolve; + }); + + vi.spyOn(tauriCommands, 'createNode').mockImplementation(async () => { + return createNodePromise; + }); + + vi.spyOn(tauriCommands, 'getNode').mockResolvedValue({ + ...mockNode, + id: 'rapid-typing', + content: 'A', + version: 1 + }); + + // Create initial node (will trigger CREATE persistence) + const node = { ...mockNode, id: 'rapid-typing', content: 'A', version: 0 }; + store.setNode(node, viewerSource); + + // Simulate rapid typing - directly modify store content + // This simulates what happens when user types faster than persistence + await new Promise(resolve => setTimeout(resolve, 5)); + const nodeInStore = store.getNode('rapid-typing'); + if (nodeInStore) { + nodeInStore.content = 'AB'; + store.nodes.set('rapid-typing', nodeInStore); + await new Promise(resolve => setTimeout(resolve, 5)); + nodeInStore.content = 'ABC'; + store.nodes.set('rapid-typing', nodeInStore); + await new Promise(resolve => setTimeout(resolve, 5)); + nodeInStore.content = 'ABCD'; + store.nodes.set('rapid-typing', nodeInStore); + await new Promise(resolve => setTimeout(resolve, 5)); + nodeInStore.content = 'ABCDE'; + store.nodes.set('rapid-typing', nodeInStore); + } + + // Verify latest content + expect(store.getNode('rapid-typing')?.content).toBe('ABCDE'); + + // Complete CREATE + createNodeResolve!('rapid-typing'); + await new Promise(resolve => setTimeout(resolve, 700)); // debounce is 500ms + + // Final content must be the LATEST user typed, not initial 'A' + const final = store.getNode('rapid-typing'); + expect(final?.content).toBe('ABCDE'); + expect(final?.version).toBe(1); + }); + }); + // ======================================================================== // Concurrent Resync Prevention // ========================================================================