Consolidate move_node API - Remove move_node_atomic pattern#545
Merged
Conversation
## Problem Issue #447/#536 changed schema loading from hardcoded to database-driven, but the GET /api/schemas endpoint was never implemented in dev-proxy. Frontend SchemaPluginLoader was failing with: - GET /api/schemas → 404 Not Found - [SchemaPluginLoader] Failed to initialize ## Solution Added GET /api/schemas endpoint to return all schema definitions: - Route: .route("/api/schemas", get(get_all_schemas)) - Handler: async fn get_all_schemas() -> ApiResult<Vec<SchemaDefinition>> - Converts SchemaService::get_all_schemas() tuple format to JSON array ## Testing - Endpoint returns empty array when no schemas exist - After schema initialization, returns all 7 core schemas - SchemaPluginLoader now initializes successfully - No more console errors on page load ## Files Changed - packages/desktop-app/src-tauri/src/bin/dev-proxy.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## Problem Issues #447/#536 changed schema loading from hardcoded to database-driven, but the dev-proxy was missing the GET /api/schemas endpoint. Frontend SchemaPluginLoader initialization was failing with 404 errors. ## Solution - Added route: .route("/api/schemas", get(get_all_schemas)) - Implemented handler that calls SchemaService::get_all_schemas() - Converts Vec<(String, SchemaDefinition)> to Vec<SchemaDefinition> - Returns JSON array of all schemas ## Testing - Endpoint now returns 200 OK with array of 7 core schemas - SchemaPluginLoader initializes successfully - All console and network errors resolved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…reation
## Issue
Schema nodes were duplicating data in both hub (node.properties) and spoke
(schema:id table), violating hub-and-spoke architecture pattern.
## Root Cause
The create_node() function was:
1. Storing properties in hub node table
2. Creating spoke record with same properties
3. Setting data link to spoke
But NOT clearing the hub properties after spoke creation.
## Fix
Update create_node() in surreal_store.rs line 916 to clear hub properties:
- Changed: `SET data = type::thing($type_table, $id);`
- To: `SET data = type::thing($type_table, $id), properties = {};`
## Architecture
Hub-and-spoke pattern for types with dedicated tables (task, schema):
- Hub (node table): Common metadata + data link to spoke
- Spoke (type table): Type-specific properties
- Properties stored ONLY in spoke, not duplicated in hub
## Testing
Requires database reset to apply:
1. `rm -rf ~/.nodespace/database/nodespace-dev`
2. `bun run dev:browser:db &`
3. `bun run dev:browser:proxy &`
4. `bun run dev:browser:init`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
… spoke creation" This reverts commit 51f8c5e.
## Backend Fixes - Fix virtual date node validation in operations/mod.rs - Call store.get_node() directly to check database state - Previously used node_service.get_node() which returns virtual nodes - Remove obsolete container validation preventing root node siblings - Remove unused can_be_container_type() function and tests (dead code cleanup) ## Frontend Fixes - Rename containerNodeId → parentId in Node interface (node.ts) - Update base-node-viewer.svelte to pass explicitParentId - Fix backend-adapter.ts to send parentId in HTTP requests - Update reactive-node-service.svelte.ts createNode() calls - Update all test files to use parentId instead of containerNodeId ## Test Fixes - test-helpers.ts: Updated createTestNode() signature and implementation - node-ordering.test.ts: Updated parent filtering logic - hierarchy-service.test.ts: Updated node creation All TypeScript errors resolved. All Rust tests passing. Quality checks passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fixed line 90 in test-helpers.ts: Changed 'containerNodeId' to 'parentId' in legacy positional parameter path This was causing all tests using positional parameters to create nodes with missing parent relationships - Updated JSDoc comment on line 37 to reference 'parentId' instead of 'containerNodeId' Both changes ensure the migration from containerNodeId → parentId is complete across all code paths. Severity: 🔴 Critical - This bug broke parent-child relationships in tests Addresses reviewer recommendations from PR #544 code review.
…parent-child changes ## Problem The indent operation was performing sequential non-atomic database operations: 1. updateNode() updated beforeSiblingId synchronously in frontend cache 2. setParent() called backend via HTTP/IPC asynchronously 3. During the gap between these operations, graph queries returned inconsistent state 4. Frontend cache refresh saw node as child of both old and new parent simultaneously 5. Caused Svelte "each_key_duplicate" errors with duplicate node IDs in render cache ## Solution Implemented atomic database transaction by: - Added Tauri command `move_node_atomic` exposing existing backend implementation - Added `moveNodeAtomic()` to BackendAdapter interface for both TauriAdapter and HttpAdapter - Updated `indentNode()` function to use single atomic operation instead of sequential calls - Removed deduplication band-aid (line 285-288) from getVisibleNodesRecursive() - no longer needed - Fixed unused variable in updated indentNode implementation - Added moveNodeAtomic() to MockBackendAdapter for tests ## Impact - Eliminates race condition window where system can be in inconsistent state - Removes defensive deduplication layer - fixes root cause rather than symptom - Tests: All 63 test files passing, no new failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit consolidates the node movement API across all layers (database, Tauri IPC, and TypeScript frontend) by eliminating the move_node_atomic/ moveNodeAtomic pattern and keeping only a single move_node/moveNode method that always performs atomic transactions. ## Changes Made ### Rust Backend (packages/core/) - **surreal_store.rs**: Deleted old non-atomic move_node method (took 2 params) - Kept only the atomic move_node method (takes 3 params: node_id, new_parent_id, new_before_sibling_id) - Updated 3 test calls to use the consolidated move_node method - All node movements now guaranteed to be atomic ### Tauri IPC Bridge (packages/desktop-app/src-tauri/) - **commands/nodes.rs**: - Deleted old move_node command that called NodeOperations - Kept single move_node command that calls SurrealStore atomic method - Updated documentation to reflect atomic-only behavior - Updated example from move_node_atomic to move_node ### TypeScript Frontend (packages/desktop-app/src/) - **backend-adapter.ts**: Renamed moveNodeAtomic → moveNode in interface and both implementations - **mock-backend-adapter.ts**: Renamed moveNodeAtomic → moveNode - **reactive-node-service.svelte.ts**: Updated call from moveNodeAtomic to moveNode ## Architecture Impact **Before**: Confusing pattern with both move_node() and move_node_atomic() - Implied that regular move_node was non-atomic (it wasn't) - Duplicated logic across layers - Unclear which version to use **After**: Single, clear API - moveNode/move_node ALWAYS performs atomic transactions - No ambiguity about atomicity guarantees - Simplified integration across all layers - All node movements are transactionally safe 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
malibio
added a commit
that referenced
this pull request
Feb 4, 2026
* Fix: Add missing GET /api/schemas endpoint to dev-proxy ## Problem Issue #447/#536 changed schema loading from hardcoded to database-driven, but the GET /api/schemas endpoint was never implemented in dev-proxy. Frontend SchemaPluginLoader was failing with: - GET /api/schemas → 404 Not Found - [SchemaPluginLoader] Failed to initialize ## Solution Added GET /api/schemas endpoint to return all schema definitions: - Route: .route("/api/schemas", get(get_all_schemas)) - Handler: async fn get_all_schemas() -> ApiResult<Vec<SchemaDefinition>> - Converts SchemaService::get_all_schemas() tuple format to JSON array ## Testing - Endpoint returns empty array when no schemas exist - After schema initialization, returns all 7 core schemas - SchemaPluginLoader now initializes successfully - No more console errors on page load ## Files Changed - packages/desktop-app/src-tauri/src/bin/dev-proxy.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix missing /api/schemas endpoint and add get_all_schemas handler ## Problem Issues #447/#536 changed schema loading from hardcoded to database-driven, but the dev-proxy was missing the GET /api/schemas endpoint. Frontend SchemaPluginLoader initialization was failing with 404 errors. ## Solution - Added route: .route("/api/schemas", get(get_all_schemas)) - Implemented handler that calls SchemaService::get_all_schemas() - Converts Vec<(String, SchemaDefinition)> to Vec<SchemaDefinition> - Returns JSON array of all schemas ## Testing - Endpoint now returns 200 OK with array of 7 core schemas - SchemaPluginLoader initializes successfully - All console and network errors resolved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix hub-and-spoke schema storage - Clear hub properties after spoke creation ## Issue Schema nodes were duplicating data in both hub (node.properties) and spoke (schema:id table), violating hub-and-spoke architecture pattern. ## Root Cause The create_node() function was: 1. Storing properties in hub node table 2. Creating spoke record with same properties 3. Setting data link to spoke But NOT clearing the hub properties after spoke creation. ## Fix Update create_node() in surreal_store.rs line 916 to clear hub properties: - Changed: `SET data = type::thing($type_table, $id);` - To: `SET data = type::thing($type_table, $id), properties = {};` ## Architecture Hub-and-spoke pattern for types with dedicated tables (task, schema): - Hub (node table): Common metadata + data link to spoke - Spoke (type table): Type-specific properties - Properties stored ONLY in spoke, not duplicated in hub ## Testing Requires database reset to apply: 1. `rm -rf ~/.nodespace/database/nodespace-dev` 2. `bun run dev:browser:db &` 3. `bun run dev:browser:proxy &` 4. `bun run dev:browser:init` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Revert "Fix hub-and-spoke schema storage - Clear hub properties after spoke creation" This reverts commit 51f8c5e. * Complete containerNodeId → parentId migration (Issue #535) ## Backend Fixes - Fix virtual date node validation in operations/mod.rs - Call store.get_node() directly to check database state - Previously used node_service.get_node() which returns virtual nodes - Remove obsolete container validation preventing root node siblings - Remove unused can_be_container_type() function and tests (dead code cleanup) ## Frontend Fixes - Rename containerNodeId → parentId in Node interface (node.ts) - Update base-node-viewer.svelte to pass explicitParentId - Fix backend-adapter.ts to send parentId in HTTP requests - Update reactive-node-service.svelte.ts createNode() calls - Update all test files to use parentId instead of containerNodeId ## Test Fixes - test-helpers.ts: Updated createTestNode() signature and implementation - node-ordering.test.ts: Updated parent filtering logic - hierarchy-service.test.ts: Updated node creation All TypeScript errors resolved. All Rust tests passing. Quality checks passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address review: Fix critical bug in test helper legacy path (Issue #535) - Fixed line 90 in test-helpers.ts: Changed 'containerNodeId' to 'parentId' in legacy positional parameter path This was causing all tests using positional parameters to create nodes with missing parent relationships - Updated JSDoc comment on line 37 to reference 'parentId' instead of 'containerNodeId' Both changes ensure the migration from containerNodeId → parentId is complete across all code paths. Severity: 🔴 Critical - This bug broke parent-child relationships in tests Addresses reviewer recommendations from PR #544 code review. * Fix indent operation atomicity - use atomic database transaction for parent-child changes ## Problem The indent operation was performing sequential non-atomic database operations: 1. updateNode() updated beforeSiblingId synchronously in frontend cache 2. setParent() called backend via HTTP/IPC asynchronously 3. During the gap between these operations, graph queries returned inconsistent state 4. Frontend cache refresh saw node as child of both old and new parent simultaneously 5. Caused Svelte "each_key_duplicate" errors with duplicate node IDs in render cache ## Solution Implemented atomic database transaction by: - Added Tauri command `move_node_atomic` exposing existing backend implementation - Added `moveNodeAtomic()` to BackendAdapter interface for both TauriAdapter and HttpAdapter - Updated `indentNode()` function to use single atomic operation instead of sequential calls - Removed deduplication band-aid (line 285-288) from getVisibleNodesRecursive() - no longer needed - Fixed unused variable in updated indentNode implementation - Added moveNodeAtomic() to MockBackendAdapter for tests ## Impact - Eliminates race condition window where system can be in inconsistent state - Removes defensive deduplication layer - fixes root cause rather than symptom - Tests: All 63 test files passing, no new failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Consolidate move_node API - Remove move_node_atomic pattern This commit consolidates the node movement API across all layers (database, Tauri IPC, and TypeScript frontend) by eliminating the move_node_atomic/ moveNodeAtomic pattern and keeping only a single move_node/moveNode method that always performs atomic transactions. ## Changes Made ### Rust Backend (packages/core/) - **surreal_store.rs**: Deleted old non-atomic move_node method (took 2 params) - Kept only the atomic move_node method (takes 3 params: node_id, new_parent_id, new_before_sibling_id) - Updated 3 test calls to use the consolidated move_node method - All node movements now guaranteed to be atomic ### Tauri IPC Bridge (packages/desktop-app/src-tauri/) - **commands/nodes.rs**: - Deleted old move_node command that called NodeOperations - Kept single move_node command that calls SurrealStore atomic method - Updated documentation to reflect atomic-only behavior - Updated example from move_node_atomic to move_node ### TypeScript Frontend (packages/desktop-app/src/) - **backend-adapter.ts**: Renamed moveNodeAtomic → moveNode in interface and both implementations - **mock-backend-adapter.ts**: Renamed moveNodeAtomic → moveNode - **reactive-node-service.svelte.ts**: Updated call from moveNodeAtomic to moveNode ## Architecture Impact **Before**: Confusing pattern with both move_node() and move_node_atomic() - Implied that regular move_node was non-atomic (it wasn't) - Duplicated logic across layers - Unclear which version to use **After**: Single, clear API - moveNode/move_node ALWAYS performs atomic transactions - No ambiguity about atomicity guarantees - Simplified integration across all layers - All node movements are transactionally safe 🤖 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
* Fix: Add missing GET /api/schemas endpoint to dev-proxy ## Problem Issue #447/#536 changed schema loading from hardcoded to database-driven, but the GET /api/schemas endpoint was never implemented in dev-proxy. Frontend SchemaPluginLoader was failing with: - GET /api/schemas → 404 Not Found - [SchemaPluginLoader] Failed to initialize ## Solution Added GET /api/schemas endpoint to return all schema definitions: - Route: .route("/api/schemas", get(get_all_schemas)) - Handler: async fn get_all_schemas() -> ApiResult<Vec<SchemaDefinition>> - Converts SchemaService::get_all_schemas() tuple format to JSON array ## Testing - Endpoint returns empty array when no schemas exist - After schema initialization, returns all 7 core schemas - SchemaPluginLoader now initializes successfully - No more console errors on page load ## Files Changed - packages/desktop-app/src-tauri/src/bin/dev-proxy.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix missing /api/schemas endpoint and add get_all_schemas handler ## Problem Issues #447/#536 changed schema loading from hardcoded to database-driven, but the dev-proxy was missing the GET /api/schemas endpoint. Frontend SchemaPluginLoader initialization was failing with 404 errors. ## Solution - Added route: .route("/api/schemas", get(get_all_schemas)) - Implemented handler that calls SchemaService::get_all_schemas() - Converts Vec<(String, SchemaDefinition)> to Vec<SchemaDefinition> - Returns JSON array of all schemas ## Testing - Endpoint now returns 200 OK with array of 7 core schemas - SchemaPluginLoader initializes successfully - All console and network errors resolved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix hub-and-spoke schema storage - Clear hub properties after spoke creation ## Issue Schema nodes were duplicating data in both hub (node.properties) and spoke (schema:id table), violating hub-and-spoke architecture pattern. ## Root Cause The create_node() function was: 1. Storing properties in hub node table 2. Creating spoke record with same properties 3. Setting data link to spoke But NOT clearing the hub properties after spoke creation. ## Fix Update create_node() in surreal_store.rs line 916 to clear hub properties: - Changed: `SET data = type::thing($type_table, $id);` - To: `SET data = type::thing($type_table, $id), properties = {};` ## Architecture Hub-and-spoke pattern for types with dedicated tables (task, schema): - Hub (node table): Common metadata + data link to spoke - Spoke (type table): Type-specific properties - Properties stored ONLY in spoke, not duplicated in hub ## Testing Requires database reset to apply: 1. `rm -rf ~/.nodespace/database/nodespace-dev` 2. `bun run dev:browser:db &` 3. `bun run dev:browser:proxy &` 4. `bun run dev:browser:init` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Revert "Fix hub-and-spoke schema storage - Clear hub properties after spoke creation" This reverts commit 51f8c5e. * Complete containerNodeId → parentId migration (Issue #535) ## Backend Fixes - Fix virtual date node validation in operations/mod.rs - Call store.get_node() directly to check database state - Previously used node_service.get_node() which returns virtual nodes - Remove obsolete container validation preventing root node siblings - Remove unused can_be_container_type() function and tests (dead code cleanup) ## Frontend Fixes - Rename containerNodeId → parentId in Node interface (node.ts) - Update base-node-viewer.svelte to pass explicitParentId - Fix backend-adapter.ts to send parentId in HTTP requests - Update reactive-node-service.svelte.ts createNode() calls - Update all test files to use parentId instead of containerNodeId ## Test Fixes - test-helpers.ts: Updated createTestNode() signature and implementation - node-ordering.test.ts: Updated parent filtering logic - hierarchy-service.test.ts: Updated node creation All TypeScript errors resolved. All Rust tests passing. Quality checks passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address review: Fix critical bug in test helper legacy path (Issue #535) - Fixed line 90 in test-helpers.ts: Changed 'containerNodeId' to 'parentId' in legacy positional parameter path This was causing all tests using positional parameters to create nodes with missing parent relationships - Updated JSDoc comment on line 37 to reference 'parentId' instead of 'containerNodeId' Both changes ensure the migration from containerNodeId → parentId is complete across all code paths. Severity: 🔴 Critical - This bug broke parent-child relationships in tests Addresses reviewer recommendations from PR #544 code review. * Fix indent operation atomicity - use atomic database transaction for parent-child changes ## Problem The indent operation was performing sequential non-atomic database operations: 1. updateNode() updated beforeSiblingId synchronously in frontend cache 2. setParent() called backend via HTTP/IPC asynchronously 3. During the gap between these operations, graph queries returned inconsistent state 4. Frontend cache refresh saw node as child of both old and new parent simultaneously 5. Caused Svelte "each_key_duplicate" errors with duplicate node IDs in render cache ## Solution Implemented atomic database transaction by: - Added Tauri command `move_node_atomic` exposing existing backend implementation - Added `moveNodeAtomic()` to BackendAdapter interface for both TauriAdapter and HttpAdapter - Updated `indentNode()` function to use single atomic operation instead of sequential calls - Removed deduplication band-aid (line 285-288) from getVisibleNodesRecursive() - no longer needed - Fixed unused variable in updated indentNode implementation - Added moveNodeAtomic() to MockBackendAdapter for tests ## Impact - Eliminates race condition window where system can be in inconsistent state - Removes defensive deduplication layer - fixes root cause rather than symptom - Tests: All 63 test files passing, no new failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Consolidate move_node API - Remove move_node_atomic pattern This commit consolidates the node movement API across all layers (database, Tauri IPC, and TypeScript frontend) by eliminating the move_node_atomic/ moveNodeAtomic pattern and keeping only a single move_node/moveNode method that always performs atomic transactions. ## Changes Made ### Rust Backend (packages/core/) - **surreal_store.rs**: Deleted old non-atomic move_node method (took 2 params) - Kept only the atomic move_node method (takes 3 params: node_id, new_parent_id, new_before_sibling_id) - Updated 3 test calls to use the consolidated move_node method - All node movements now guaranteed to be atomic ### Tauri IPC Bridge (packages/desktop-app/src-tauri/) - **commands/nodes.rs**: - Deleted old move_node command that called NodeOperations - Kept single move_node command that calls SurrealStore atomic method - Updated documentation to reflect atomic-only behavior - Updated example from move_node_atomic to move_node ### TypeScript Frontend (packages/desktop-app/src/) - **backend-adapter.ts**: Renamed moveNodeAtomic → moveNode in interface and both implementations - **mock-backend-adapter.ts**: Renamed moveNodeAtomic → moveNode - **reactive-node-service.svelte.ts**: Updated call from moveNodeAtomic to moveNode ## Architecture Impact **Before**: Confusing pattern with both move_node() and move_node_atomic() - Implied that regular move_node was non-atomic (it wasn't) - Duplicated logic across layers - Unclear which version to use **After**: Single, clear API - moveNode/move_node ALWAYS performs atomic transactions - No ambiguity about atomicity guarantees - Simplified integration across all layers - All node movements are transactionally safe 🤖 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
* Fix: Add missing GET /api/schemas endpoint to dev-proxy ## Problem Issue #447/#536 changed schema loading from hardcoded to database-driven, but the GET /api/schemas endpoint was never implemented in dev-proxy. Frontend SchemaPluginLoader was failing with: - GET /api/schemas → 404 Not Found - [SchemaPluginLoader] Failed to initialize ## Solution Added GET /api/schemas endpoint to return all schema definitions: - Route: .route("/api/schemas", get(get_all_schemas)) - Handler: async fn get_all_schemas() -> ApiResult<Vec<SchemaDefinition>> - Converts SchemaService::get_all_schemas() tuple format to JSON array ## Testing - Endpoint returns empty array when no schemas exist - After schema initialization, returns all 7 core schemas - SchemaPluginLoader now initializes successfully - No more console errors on page load ## Files Changed - packages/desktop-app/src-tauri/src/bin/dev-proxy.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix missing /api/schemas endpoint and add get_all_schemas handler ## Problem Issues #447/#536 changed schema loading from hardcoded to database-driven, but the dev-proxy was missing the GET /api/schemas endpoint. Frontend SchemaPluginLoader initialization was failing with 404 errors. ## Solution - Added route: .route("/api/schemas", get(get_all_schemas)) - Implemented handler that calls SchemaService::get_all_schemas() - Converts Vec<(String, SchemaDefinition)> to Vec<SchemaDefinition> - Returns JSON array of all schemas ## Testing - Endpoint now returns 200 OK with array of 7 core schemas - SchemaPluginLoader initializes successfully - All console and network errors resolved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix hub-and-spoke schema storage - Clear hub properties after spoke creation ## Issue Schema nodes were duplicating data in both hub (node.properties) and spoke (schema:id table), violating hub-and-spoke architecture pattern. ## Root Cause The create_node() function was: 1. Storing properties in hub node table 2. Creating spoke record with same properties 3. Setting data link to spoke But NOT clearing the hub properties after spoke creation. ## Fix Update create_node() in surreal_store.rs line 916 to clear hub properties: - Changed: `SET data = type::thing($type_table, $id);` - To: `SET data = type::thing($type_table, $id), properties = {};` ## Architecture Hub-and-spoke pattern for types with dedicated tables (task, schema): - Hub (node table): Common metadata + data link to spoke - Spoke (type table): Type-specific properties - Properties stored ONLY in spoke, not duplicated in hub ## Testing Requires database reset to apply: 1. `rm -rf ~/.nodespace/database/nodespace-dev` 2. `bun run dev:browser:db &` 3. `bun run dev:browser:proxy &` 4. `bun run dev:browser:init` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Revert "Fix hub-and-spoke schema storage - Clear hub properties after spoke creation" This reverts commit 5c492eab7c24289e52101d1bd94e53dd0e06441c. * Complete containerNodeId → parentId migration (Issue #535) ## Backend Fixes - Fix virtual date node validation in operations/mod.rs - Call store.get_node() directly to check database state - Previously used node_service.get_node() which returns virtual nodes - Remove obsolete container validation preventing root node siblings - Remove unused can_be_container_type() function and tests (dead code cleanup) ## Frontend Fixes - Rename containerNodeId → parentId in Node interface (node.ts) - Update base-node-viewer.svelte to pass explicitParentId - Fix backend-adapter.ts to send parentId in HTTP requests - Update reactive-node-service.svelte.ts createNode() calls - Update all test files to use parentId instead of containerNodeId ## Test Fixes - test-helpers.ts: Updated createTestNode() signature and implementation - node-ordering.test.ts: Updated parent filtering logic - hierarchy-service.test.ts: Updated node creation All TypeScript errors resolved. All Rust tests passing. Quality checks passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address review: Fix critical bug in test helper legacy path (Issue #535) - Fixed line 90 in test-helpers.ts: Changed 'containerNodeId' to 'parentId' in legacy positional parameter path This was causing all tests using positional parameters to create nodes with missing parent relationships - Updated JSDoc comment on line 37 to reference 'parentId' instead of 'containerNodeId' Both changes ensure the migration from containerNodeId → parentId is complete across all code paths. Severity: 🔴 Critical - This bug broke parent-child relationships in tests Addresses reviewer recommendations from PR #544 code review. * Fix indent operation atomicity - use atomic database transaction for parent-child changes ## Problem The indent operation was performing sequential non-atomic database operations: 1. updateNode() updated beforeSiblingId synchronously in frontend cache 2. setParent() called backend via HTTP/IPC asynchronously 3. During the gap between these operations, graph queries returned inconsistent state 4. Frontend cache refresh saw node as child of both old and new parent simultaneously 5. Caused Svelte "each_key_duplicate" errors with duplicate node IDs in render cache ## Solution Implemented atomic database transaction by: - Added Tauri command `move_node_atomic` exposing existing backend implementation - Added `moveNodeAtomic()` to BackendAdapter interface for both TauriAdapter and HttpAdapter - Updated `indentNode()` function to use single atomic operation instead of sequential calls - Removed deduplication band-aid (line 285-288) from getVisibleNodesRecursive() - no longer needed - Fixed unused variable in updated indentNode implementation - Added moveNodeAtomic() to MockBackendAdapter for tests ## Impact - Eliminates race condition window where system can be in inconsistent state - Removes defensive deduplication layer - fixes root cause rather than symptom - Tests: All 63 test files passing, no new failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Consolidate move_node API - Remove move_node_atomic pattern This commit consolidates the node movement API across all layers (database, Tauri IPC, and TypeScript frontend) by eliminating the move_node_atomic/ moveNodeAtomic pattern and keeping only a single move_node/moveNode method that always performs atomic transactions. ## Changes Made ### Rust Backend (packages/core/) - **surreal_store.rs**: Deleted old non-atomic move_node method (took 2 params) - Kept only the atomic move_node method (takes 3 params: node_id, new_parent_id, new_before_sibling_id) - Updated 3 test calls to use the consolidated move_node method - All node movements now guaranteed to be atomic ### Tauri IPC Bridge (packages/desktop-app/src-tauri/) - **commands/nodes.rs**: - Deleted old move_node command that called NodeOperations - Kept single move_node command that calls SurrealStore atomic method - Updated documentation to reflect atomic-only behavior - Updated example from move_node_atomic to move_node ### TypeScript Frontend (packages/desktop-app/src/) - **backend-adapter.ts**: Renamed moveNodeAtomic → moveNode in interface and both implementations - **mock-backend-adapter.ts**: Renamed moveNodeAtomic → moveNode - **reactive-node-service.svelte.ts**: Updated call from moveNodeAtomic to moveNode ## Architecture Impact **Before**: Confusing pattern with both move_node() and move_node_atomic() - Implied that regular move_node was non-atomic (it wasn't) - Duplicated logic across layers - Unclear which version to use **After**: Single, clear API - moveNode/move_node ALWAYS performs atomic transactions - No ambiguity about atomicity guarantees - Simplified integration across all layers - All node movements are transactionally safe 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <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.
Summary
This PR consolidates the node movement API by eliminating the architectural redundancy of having both
move_node()andmove_node_atomic()methods. As you correctly pointed out, we should have just modified themove_node()method to use atomic logic.Changes
Rust Backend
move_nodemethod from SurrealStoremove_nodemethod (takes 3 parameters: node_id, new_parent_id, new_before_sibling_id)Tauri IPC Bridge
move_nodecommand that used NodeOperationsmove_nodecommand that calls atomic SurrealStore methodmove_node_atomictomove_nodeTypeScript Frontend
moveNodeAtomic→moveNodeacross all layers:Call Sites
Quality Assurance
Architecture Benefit
This change creates a single, clear API where
moveNode/move_nodealways performs atomic transactions. No more confusion about which version to use—atomicity is guaranteed by design.Generated with Claude Code