Refactor BaseNodeViewer to use SharedNodeStore for all database operations#238
Merged
malibio merged 2 commits intoOct 12, 2025
Merged
Conversation
…tions (#237) ## Summary Eliminates dual persistence paths by routing all BaseNodeViewer database operations through SharedNodeStore, resolving conflicts and race conditions while preserving sophisticated FOREIGN KEY coordination logic. ## Changes ### SharedNodeStore Enhancements - Added `pendingContentSaves` tracking for FOREIGN KEY coordination - Implemented `waitForNodeSaves()` with timeout and grace period handling - Added `ensureAncestorChainPersisted()` for recursive placeholder parent persistence - Added `validateNodeReferences()` for FOREIGN KEY validation before structural updates - Added `updateStructuralChangesValidated()` for serial processing with validation - Added `loadChildrenForParent()` wrapping database queries - Implemented hybrid debouncing: 500ms for content, immediate for structural updates - Added placeholder-awareness: skips persistence when `isPlaceholder === true` ### BaseNodeViewer Refactoring - Replaced all direct `databaseService` calls with `sharedNodeStore` methods - Refactored `waitForNodeSavesIfPending()` to delegate to SharedNodeStore - Refactored `ensureAncestorsPersisted()` to delegate with placeholder checker callback - Updated content save watcher to use `saveNodeImmediately()` and `updateNodeContentDebounced()` - Updated deletion watcher to use `sharedNodeStore.deleteNode()` - Updated structural watcher to use `updateStructuralChangesValidated()` with proper coordination - Refactored `loadChildrenForParent()` to use SharedNodeStore method - Removed `saveNode()` and `debounceSave()` functions (replaced by SharedNodeStore) - Simplified `saveHierarchyChange()` to use SharedNodeStore - Removed `queueDatabaseWrite` import (no longer needed) ### Architecture - **BaseNodeViewer** remains the "conductor": decides WHEN and WHAT ORDER to persist - **SharedNodeStore** is the "performer": executes database operations with coordination - Preserved all FOREIGN KEY logic: waiting for saves, validating references, ordering operations - Maintained sophisticated save/delete coordination for data integrity ## Test Notes All quality checks pass. Some pre-existing test failures in node-ordering tests are unrelated to this refactoring (tests use service layer directly, not BaseNodeViewer). ## Related Issues Closes #237 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## Changes Implemented ### 1. Component Unmount Safety (🟡 Important) - Added `isDestroyed` flag to BaseNodeViewer to prevent writes after unmount - Checks flag before all save operations in content watcher - Prevents race conditions and stale writes ### 2. Grace Period Logic Fix (🟡 Important) - Fixed `waitForNodeSaves()` to properly detect in-flight vs failed saves - Now checks if promise actually rejected vs still pending - Prevents false positive failures for saves that complete during grace period ### 3. Single Source of Truth (🟢 Nice-to-Have) - Added `hasPendingSave()` accessor method to SharedNodeStore - Eliminates dual promise tracking between components - Provides clean API for checking pending save status ### 4. Error Event Construction (🟢 Nice-to-Have) - Extracted `buildPersistenceErrorEvent()` helper function - Reduces code duplication in error handling paths - Improves maintainability and consistency ### 5. Comprehensive Test Coverage (🟡 Important) - Added 34 unit tests for 7 new SharedNodeStore methods: - `loadChildrenForParent()` (3 tests) - `updateNodeContentDebounced()` (4 tests) - `saveNodeImmediately()` (5 tests) - `hasPendingSave()` (3 tests) - `waitForNodeSaves()` (5 tests) - `ensureAncestorChainPersisted()` (4 tests) - `validateNodeReferences()` (5 tests) - `updateStructuralChangesValidated()` (5 tests) - All 65 tests passing (100% pass rate) - Tests cover FOREIGN KEY coordination, debouncing, validation, error handling ## Test Results ✅ 65/65 tests passing ✅ quality:fix passes with 0 errors, 0 warnings ✅ All lint issues resolved ## Files Modified - `packages/desktop-app/src/lib/design/components/base-node-viewer.svelte` - Added isDestroyed flag and checks - Added buildPersistenceErrorEvent() helper - `packages/desktop-app/src/lib/services/shared-node-store.ts` - Fixed waitForNodeSaves() grace period logic - Added hasPendingSave() method - `packages/desktop-app/src/tests/services/shared-node-store.test.ts` - Added 34 comprehensive unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
Code Review Recommendations Addressed ✅I've completed all the recommendations from the pragmatic code review. Here's what was implemented: 🟡 Important Issues Fixed1. Component Unmount Race ConditionIssue: Async save operations could complete after component unmounts, causing stale writes
Files: 2. Grace Period Logic False PositivesIssue:
Files: 3. Comprehensive Test CoverageIssue: 7 new SharedNodeStore methods had no unit tests
All tests cover complex business logic: FOREIGN KEY coordination, debouncing, validation, error handling Files: 🟢 Nice-to-Have Improvements4. Single Source of TruthIssue: Dual promise tracking between BaseNodeViewer and SharedNodeStore Files: 5. Error Event Construction HelperIssue: Duplicated error event construction logic Files: Test Results✅ 65/65 tests passing (100% pass rate) Impact
Ready for final review and merge! 🚀 |
malibio
deleted the
feature/issue-237-refactor-basenodeviewer-sharedstore
branch
October 12, 2025 00:01
5 tasks
malibio
added a commit
that referenced
this pull request
Feb 4, 2026
…tions (#238) * Refactor BaseNodeViewer to use SharedNodeStore for all database operations (#237) ## Summary Eliminates dual persistence paths by routing all BaseNodeViewer database operations through SharedNodeStore, resolving conflicts and race conditions while preserving sophisticated FOREIGN KEY coordination logic. ## Changes ### SharedNodeStore Enhancements - Added `pendingContentSaves` tracking for FOREIGN KEY coordination - Implemented `waitForNodeSaves()` with timeout and grace period handling - Added `ensureAncestorChainPersisted()` for recursive placeholder parent persistence - Added `validateNodeReferences()` for FOREIGN KEY validation before structural updates - Added `updateStructuralChangesValidated()` for serial processing with validation - Added `loadChildrenForParent()` wrapping database queries - Implemented hybrid debouncing: 500ms for content, immediate for structural updates - Added placeholder-awareness: skips persistence when `isPlaceholder === true` ### BaseNodeViewer Refactoring - Replaced all direct `databaseService` calls with `sharedNodeStore` methods - Refactored `waitForNodeSavesIfPending()` to delegate to SharedNodeStore - Refactored `ensureAncestorsPersisted()` to delegate with placeholder checker callback - Updated content save watcher to use `saveNodeImmediately()` and `updateNodeContentDebounced()` - Updated deletion watcher to use `sharedNodeStore.deleteNode()` - Updated structural watcher to use `updateStructuralChangesValidated()` with proper coordination - Refactored `loadChildrenForParent()` to use SharedNodeStore method - Removed `saveNode()` and `debounceSave()` functions (replaced by SharedNodeStore) - Simplified `saveHierarchyChange()` to use SharedNodeStore - Removed `queueDatabaseWrite` import (no longer needed) ### Architecture - **BaseNodeViewer** remains the "conductor": decides WHEN and WHAT ORDER to persist - **SharedNodeStore** is the "performer": executes database operations with coordination - Preserved all FOREIGN KEY logic: waiting for saves, validating references, ordering operations - Maintained sophisticated save/delete coordination for data integrity ## Test Notes All quality checks pass. Some pre-existing test failures in node-ordering tests are unrelated to this refactoring (tests use service layer directly, not BaseNodeViewer). ## Related Issues Closes #237 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address code review recommendations for Issue #237 ## Changes Implemented ### 1. Component Unmount Safety (🟡 Important) - Added `isDestroyed` flag to BaseNodeViewer to prevent writes after unmount - Checks flag before all save operations in content watcher - Prevents race conditions and stale writes ### 2. Grace Period Logic Fix (🟡 Important) - Fixed `waitForNodeSaves()` to properly detect in-flight vs failed saves - Now checks if promise actually rejected vs still pending - Prevents false positive failures for saves that complete during grace period ### 3. Single Source of Truth (🟢 Nice-to-Have) - Added `hasPendingSave()` accessor method to SharedNodeStore - Eliminates dual promise tracking between components - Provides clean API for checking pending save status ### 4. Error Event Construction (🟢 Nice-to-Have) - Extracted `buildPersistenceErrorEvent()` helper function - Reduces code duplication in error handling paths - Improves maintainability and consistency ### 5. Comprehensive Test Coverage (🟡 Important) - Added 34 unit tests for 7 new SharedNodeStore methods: - `loadChildrenForParent()` (3 tests) - `updateNodeContentDebounced()` (4 tests) - `saveNodeImmediately()` (5 tests) - `hasPendingSave()` (3 tests) - `waitForNodeSaves()` (5 tests) - `ensureAncestorChainPersisted()` (4 tests) - `validateNodeReferences()` (5 tests) - `updateStructuralChangesValidated()` (5 tests) - All 65 tests passing (100% pass rate) - Tests cover FOREIGN KEY coordination, debouncing, validation, error handling ## Test Results ✅ 65/65 tests passing ✅ quality:fix passes with 0 errors, 0 warnings ✅ All lint issues resolved ## Files Modified - `packages/desktop-app/src/lib/design/components/base-node-viewer.svelte` - Added isDestroyed flag and checks - Added buildPersistenceErrorEvent() helper - `packages/desktop-app/src/lib/services/shared-node-store.ts` - Fixed waitForNodeSaves() grace period logic - Added hasPendingSave() method - `packages/desktop-app/src/tests/services/shared-node-store.test.ts` - Added 34 comprehensive unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
malibio
added a commit
that referenced
this pull request
Feb 5, 2026
…tions (#238) * Refactor BaseNodeViewer to use SharedNodeStore for all database operations (#237) ## Summary Eliminates dual persistence paths by routing all BaseNodeViewer database operations through SharedNodeStore, resolving conflicts and race conditions while preserving sophisticated FOREIGN KEY coordination logic. ## Changes ### SharedNodeStore Enhancements - Added `pendingContentSaves` tracking for FOREIGN KEY coordination - Implemented `waitForNodeSaves()` with timeout and grace period handling - Added `ensureAncestorChainPersisted()` for recursive placeholder parent persistence - Added `validateNodeReferences()` for FOREIGN KEY validation before structural updates - Added `updateStructuralChangesValidated()` for serial processing with validation - Added `loadChildrenForParent()` wrapping database queries - Implemented hybrid debouncing: 500ms for content, immediate for structural updates - Added placeholder-awareness: skips persistence when `isPlaceholder === true` ### BaseNodeViewer Refactoring - Replaced all direct `databaseService` calls with `sharedNodeStore` methods - Refactored `waitForNodeSavesIfPending()` to delegate to SharedNodeStore - Refactored `ensureAncestorsPersisted()` to delegate with placeholder checker callback - Updated content save watcher to use `saveNodeImmediately()` and `updateNodeContentDebounced()` - Updated deletion watcher to use `sharedNodeStore.deleteNode()` - Updated structural watcher to use `updateStructuralChangesValidated()` with proper coordination - Refactored `loadChildrenForParent()` to use SharedNodeStore method - Removed `saveNode()` and `debounceSave()` functions (replaced by SharedNodeStore) - Simplified `saveHierarchyChange()` to use SharedNodeStore - Removed `queueDatabaseWrite` import (no longer needed) ### Architecture - **BaseNodeViewer** remains the "conductor": decides WHEN and WHAT ORDER to persist - **SharedNodeStore** is the "performer": executes database operations with coordination - Preserved all FOREIGN KEY logic: waiting for saves, validating references, ordering operations - Maintained sophisticated save/delete coordination for data integrity ## Test Notes All quality checks pass. Some pre-existing test failures in node-ordering tests are unrelated to this refactoring (tests use service layer directly, not BaseNodeViewer). ## Related Issues Closes #237 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address code review recommendations for Issue #237 ## Changes Implemented ### 1. Component Unmount Safety (🟡 Important) - Added `isDestroyed` flag to BaseNodeViewer to prevent writes after unmount - Checks flag before all save operations in content watcher - Prevents race conditions and stale writes ### 2. Grace Period Logic Fix (🟡 Important) - Fixed `waitForNodeSaves()` to properly detect in-flight vs failed saves - Now checks if promise actually rejected vs still pending - Prevents false positive failures for saves that complete during grace period ### 3. Single Source of Truth (🟢 Nice-to-Have) - Added `hasPendingSave()` accessor method to SharedNodeStore - Eliminates dual promise tracking between components - Provides clean API for checking pending save status ### 4. Error Event Construction (🟢 Nice-to-Have) - Extracted `buildPersistenceErrorEvent()` helper function - Reduces code duplication in error handling paths - Improves maintainability and consistency ### 5. Comprehensive Test Coverage (🟡 Important) - Added 34 unit tests for 7 new SharedNodeStore methods: - `loadChildrenForParent()` (3 tests) - `updateNodeContentDebounced()` (4 tests) - `saveNodeImmediately()` (5 tests) - `hasPendingSave()` (3 tests) - `waitForNodeSaves()` (5 tests) - `ensureAncestorChainPersisted()` (4 tests) - `validateNodeReferences()` (5 tests) - `updateStructuralChangesValidated()` (5 tests) - All 65 tests passing (100% pass rate) - Tests cover FOREIGN KEY coordination, debouncing, validation, error handling ## Test Results ✅ 65/65 tests passing ✅ quality:fix passes with 0 errors, 0 warnings ✅ All lint issues resolved ## Files Modified - `packages/desktop-app/src/lib/design/components/base-node-viewer.svelte` - Added isDestroyed flag and checks - Added buildPersistenceErrorEvent() helper - `packages/desktop-app/src/lib/services/shared-node-store.ts` - Fixed waitForNodeSaves() grace period logic - Added hasPendingSave() method - `packages/desktop-app/src/tests/services/shared-node-store.test.ts` - Added 34 comprehensive unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
malibio
added a commit
that referenced
this pull request
Feb 26, 2026
…tions (#238) * Refactor BaseNodeViewer to use SharedNodeStore for all database operations (#237) ## Summary Eliminates dual persistence paths by routing all BaseNodeViewer database operations through SharedNodeStore, resolving conflicts and race conditions while preserving sophisticated FOREIGN KEY coordination logic. ## Changes ### SharedNodeStore Enhancements - Added `pendingContentSaves` tracking for FOREIGN KEY coordination - Implemented `waitForNodeSaves()` with timeout and grace period handling - Added `ensureAncestorChainPersisted()` for recursive placeholder parent persistence - Added `validateNodeReferences()` for FOREIGN KEY validation before structural updates - Added `updateStructuralChangesValidated()` for serial processing with validation - Added `loadChildrenForParent()` wrapping database queries - Implemented hybrid debouncing: 500ms for content, immediate for structural updates - Added placeholder-awareness: skips persistence when `isPlaceholder === true` ### BaseNodeViewer Refactoring - Replaced all direct `databaseService` calls with `sharedNodeStore` methods - Refactored `waitForNodeSavesIfPending()` to delegate to SharedNodeStore - Refactored `ensureAncestorsPersisted()` to delegate with placeholder checker callback - Updated content save watcher to use `saveNodeImmediately()` and `updateNodeContentDebounced()` - Updated deletion watcher to use `sharedNodeStore.deleteNode()` - Updated structural watcher to use `updateStructuralChangesValidated()` with proper coordination - Refactored `loadChildrenForParent()` to use SharedNodeStore method - Removed `saveNode()` and `debounceSave()` functions (replaced by SharedNodeStore) - Simplified `saveHierarchyChange()` to use SharedNodeStore - Removed `queueDatabaseWrite` import (no longer needed) ### Architecture - **BaseNodeViewer** remains the "conductor": decides WHEN and WHAT ORDER to persist - **SharedNodeStore** is the "performer": executes database operations with coordination - Preserved all FOREIGN KEY logic: waiting for saves, validating references, ordering operations - Maintained sophisticated save/delete coordination for data integrity ## Test Notes All quality checks pass. Some pre-existing test failures in node-ordering tests are unrelated to this refactoring (tests use service layer directly, not BaseNodeViewer). ## Related Issues Closes #237 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address code review recommendations for Issue #237 ## Changes Implemented ### 1. Component Unmount Safety (🟡 Important) - Added `isDestroyed` flag to BaseNodeViewer to prevent writes after unmount - Checks flag before all save operations in content watcher - Prevents race conditions and stale writes ### 2. Grace Period Logic Fix (🟡 Important) - Fixed `waitForNodeSaves()` to properly detect in-flight vs failed saves - Now checks if promise actually rejected vs still pending - Prevents false positive failures for saves that complete during grace period ### 3. Single Source of Truth (🟢 Nice-to-Have) - Added `hasPendingSave()` accessor method to SharedNodeStore - Eliminates dual promise tracking between components - Provides clean API for checking pending save status ### 4. Error Event Construction (🟢 Nice-to-Have) - Extracted `buildPersistenceErrorEvent()` helper function - Reduces code duplication in error handling paths - Improves maintainability and consistency ### 5. Comprehensive Test Coverage (🟡 Important) - Added 34 unit tests for 7 new SharedNodeStore methods: - `loadChildrenForParent()` (3 tests) - `updateNodeContentDebounced()` (4 tests) - `saveNodeImmediately()` (5 tests) - `hasPendingSave()` (3 tests) - `waitForNodeSaves()` (5 tests) - `ensureAncestorChainPersisted()` (4 tests) - `validateNodeReferences()` (5 tests) - `updateStructuralChangesValidated()` (5 tests) - All 65 tests passing (100% pass rate) - Tests cover FOREIGN KEY coordination, debouncing, validation, error handling ## Test Results ✅ 65/65 tests passing ✅ quality:fix passes with 0 errors, 0 warnings ✅ All lint issues resolved ## Files Modified - `packages/desktop-app/src/lib/design/components/base-node-viewer.svelte` - Added isDestroyed flag and checks - Added buildPersistenceErrorEvent() helper - `packages/desktop-app/src/lib/services/shared-node-store.ts` - Fixed waitForNodeSaves() grace period logic - Added hasPendingSave() method - `packages/desktop-app/src/tests/services/shared-node-store.test.ts` - Added 34 comprehensive unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
mstomar125
pushed a commit
to mstomar125/nodespace-core
that referenced
this pull request
Jul 2, 2026
…paceAI#238 review) Address the S1 review: only the read path (loadCollection / ensureIdentity) was Pro-gated; the 9 mutations called the pro_* commands unconditionally, breaking the store's own "every method is inert in community mode" contract — and acceptInvite / requestJoin are the S5 onboarding entry points. - Void mutations early-return in community mode; the value-returning createInvite / acceptInvite / requestJoin throw via a requirePro() guard (never a fake-success value) so none reach the service when tier !== 'pro'. - Test pins the invariant: a void mutation no-ops and the value-returning ones throw, none touching the service, when isPro === false. Also from review (non-blocking): - ensureIdentity retries while person_id is empty, so a mid-session identity bind is picked up instead of leaving currentUserRole null all session. - loadCollection commits the roster before the admin-only invite/request listings, so a listing failure no longer discards an already-fetched roster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mstomar125
added a commit
that referenced
this pull request
Jul 2, 2026
…238) (#1473) * Add membership service + store foundations for the Collaboration UI (#238) Groundwork (slice S1) for the collection-membership UI on top of the complete #143 backend. No visible surface yet — this is the service + reactive store the Collaboration view (S3/S4) and onboarding inbox (S5) build on. - membership-service.ts: typed wrappers over the eight pro_* membership commands plus S2's three (pro_list_invites / pro_list_requests / pro_revoke_invite) and pro_current_person. Maps the commands' snake_case DTOs to camelCase interfaces at the boundary so the rest of the frontend never sees the wire shape. - membership.svelte.ts: a Svelte 5 runes store caching per-collection roster + pending invites/requests + the caller's own identity. Derives currentUserRole(collectionId) by matching the caller's person id against the roster, so admin controls gate correctly; only fetches the admin-gated invite/request listings when the caller is an admin. Mutations refresh the affected collection. Pro-gated: every method is inert when proSync.tier !== 'pro', so the community build is behaviorally unchanged. Tests: 16 new (service arg/mapping + store role-derivation, admin gating, community-inert, mutation refresh, reset). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Enforce community-inert contract on membership store mutations (#238 review) Address the S1 review: only the read path (loadCollection / ensureIdentity) was Pro-gated; the 9 mutations called the pro_* commands unconditionally, breaking the store's own "every method is inert in community mode" contract — and acceptInvite / requestJoin are the S5 onboarding entry points. - Void mutations early-return in community mode; the value-returning createInvite / acceptInvite / requestJoin throw via a requirePro() guard (never a fake-success value) so none reach the service when tier !== 'pro'. - Test pins the invariant: a void mutation no-ops and the value-returning ones throw, none touching the service, when isPro === false. Also from review (non-blocking): - ensureIdentity retries while person_id is empty, so a mid-session identity bind is picked up instead of leaving currentUserRole null all session. - loadCollection commits the roster before the admin-only invite/request listings, so a listing failure no longer discards an already-fetched roster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Mayank Tomar <mayanktomar@Michaels-Mac-mini.local> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #237