Skip to content

Refactor BaseNodeViewer to use SharedNodeStore for all database operations #237

Description

@malibio

Problem

BaseNodeViewer bypasses SharedNodeStore and directly calls database service methods, creating dual persistence paths that cause conflicts and violate single-source-of-truth principle.

Current Architecture (Broken)

BaseNodeViewer → databaseService → database
ReactiveNodeService → SharedNodeStore → tauriNodeService → database

Direct Database Calls in BaseNodeViewer

  • databaseService.getNode() - checking node existence
  • databaseService.deleteNode() - deleting nodes
  • databaseService.updateNode() - updating structural changes
  • databaseService.getNodesByOriginId() - loading children
  • databaseService.saveNodeWithParent() - saving content (debounced, placeholder-aware)

Issues Caused

  1. Duplicate INSERTs - Both paths try to create nodes (temporarily fixed with source.type !== 'viewer' workaround)
  2. Conflicting deletes - Not covered by workaround
  3. Conflicting structural updates - Not covered by workaround
  4. Tests can't catch bugs - Tests use HttpAdapter, real app uses TauriNodeService, SharedNodeStore persistence never tested
  5. Inconsistent state - SharedNodeStore.persistedNodeIds tracking doesn't know about BaseNodeViewer's direct saves

Temporary Workaround (Added in #232)

Added && source.type !== 'viewer' check in SharedNodeStore to skip persisting viewer-sourced updates. This prevents duplicate INSERTs but doesn't address all conflicts.

IMPORTANT: This workaround must be removed as part of this refactoring.

Solution

Refactor BaseNodeViewer to use SharedNodeStore for ALL database operations.

Target Architecture

BaseNodeViewer → ReactiveNodeService → SharedNodeStore → tauriNodeService → database

Changes Needed

  1. Remove direct database calls from BaseNodeViewer

    • Replace databaseService.getNodesByOriginId() with SharedNodeStore query
    • Replace databaseService.saveNodeWithParent() with sharedNodeStore.updateNode()
    • Replace databaseService.deleteNode() with sharedNodeStore.deleteNode()
    • Replace databaseService.updateNode() with sharedNodeStore.updateNode()
  2. Move debouncing to SharedNodeStore

    • Transfer 500ms debounce logic from BaseNodeViewer
    • Make SharedNodeStore placeholder-aware (don't persist empty nodes)
    • Preserve FOREIGN KEY coordination (wait for parent saves before child structural updates)
  3. Remove temporary workaround

    • Delete && source.type !== 'viewer' check from SharedNodeStore.updateNode() (line ~284)
    • Delete && source.type !== 'viewer' check from SharedNodeStore.setNode() (line ~367)
    • SharedNodeStore should handle ALL persistence for viewer-sourced updates
  4. Update tests

    • SharedNodeStore persistence will now be exercised by integration tests
    • Tests will catch dual persistence bugs automatically

Complexity

  • Medium-High
  • Requires careful migration of BaseNodeViewer's sophisticated save logic
  • Must preserve placeholder handling and FOREIGN KEY coordination
  • Need comprehensive testing to ensure no regressions

Related Issues

Acceptance Criteria

  • All databaseService.* calls removed from BaseNodeViewer
  • SharedNodeStore handles all persistence with proper debouncing
  • Placeholder nodes not persisted to database
  • FOREIGN KEY coordination preserved
  • Temporary workaround removed from SharedNodeStore
  • All integration tests pass
  • Manual testing confirms no regressions

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions