fix: emit layout change for batch node bounds #5939
Conversation
🎭 Playwright Test Results⏰ Completed at: 10/07/2025, 05:58:46 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/07/2025, 05:48:51 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
christian-byrne
force-pushed
the
vue-node/fix-layout-mutation
branch
from
5f682ef to
1a14c1c
Compare
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| operation: BatchUpdateBoundsOperation, | ||
| change: LayoutChange | ||
| ): void { | ||
| for (const nodeId of operation.nodeIds) { |
There was a problem hiding this comment.
[quality] medium Priority\n\nIssue: Missing null check for operation.nodeIds could cause runtime error\nContext: If nodeIds is undefined or null, the for loop will throw TypeError\nSuggestion: Add validation: if (!operation.nodeIds?.length) return at the start of the method
| const ynode = this.ynodes.get(nodeId) | ||
| if (!ynode || !data) continue | ||
|
|
||
| this.spatialIndex.update(nodeId, data.bounds) |
There was a problem hiding this comment.
[performance] medium Priority\n\nIssue: Spatial index updates in loop may cause performance degradation for large batch operations\nContext: Each spatialIndex.update() call in the loop could trigger expensive rebalancing operations\nSuggestion: Consider batching spatial index updates or adding a batch update method to the spatial index if available
| change.nodeIds.push(nodeId) | ||
| } | ||
|
|
||
| if (change.nodeIds.length) { |
There was a problem hiding this comment.
[quality] low Priority\n\nIssue: Change type is only set when nodes exist, but change object is still passed to observers\nContext: If all nodes are invalid/missing, change.type remains undefined but still gets processed\nSuggestion: Set change.type = 'batch' unconditionally at the start, or return early if no valid nodes
|
|
||
| boundsRecord[nodeId] = { | ||
| bounds, | ||
| previousBounds: currentLayout.bounds |
There was a problem hiding this comment.
[performance] low Priority\n\nIssue: Storing previousBounds for every node in batch operations increases memory footprint\nContext: For large batch operations (hundreds of nodes), this could create significant temporary memory usage\nSuggestion: Consider if previousBounds is actually needed for operation replay/undo, or if it can be computed on-demand
| type: 'batchUpdate' | ||
| updates: Partial<NodeLayout> | ||
| previousValues: Partial<NodeLayout> | ||
| export interface BatchUpdateBoundsOperation extends OperationMeta { |
There was a problem hiding this comment.
[quality] medium Priority\n\nIssue: Interface extends OperationMeta but adds duplicate fields that may already exist\nContext: OperationMeta likely contains timestamp, source, actor fields that are being redefined here\nSuggestion: Verify OperationMeta interface and remove duplicate fields, or use intersection types properly
| const newBounds = { x: 40, y: 60, width: 220, height: 120 } | ||
| layoutStore.batchUpdateNodeBounds([{ nodeId, bounds: newBounds }]) | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 50)) |
There was a problem hiding this comment.
[quality] low Priority\n\nIssue: Test uses arbitrary timeout without proper synchronization\nContext: setTimeout(50) is brittle and may cause flaky tests on slower systems\nSuggestion: Use waitFor() or proper async synchronization instead of arbitrary timeouts
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: fix: emit layout change for batch node bounds (#5939)
Impact: 113 additions, 16 deletions across 4 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 3
- Low: 3
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 2 issues
- Code Quality: 4 issues
Key Findings
Architecture & Design
The implementation follows good CRDT patterns by routing DOM-driven node bounds updates through a single operation type. The new BatchUpdateBoundsOperation properly integrates with the existing Yjs-based layout store architecture and maintains consistency with other operation handlers.
Security Considerations
No security issues identified. The implementation properly validates node existence and doesn't expose any attack vectors.
Performance Impact
Two performance considerations:
- Spatial index updates in loops may cause performance degradation for large batch operations
- Storing previousBounds increases memory footprint during batch operations
Integration Points
The integration with useLinkLayoutSync correctly handles link recomputation for updated nodes. The test coverage adequately validates the new functionality.
Positive Observations
- Good separation of concerns with dedicated operation type
- Proper integration with existing CRDT pipeline
- Comprehensive test coverage for the new functionality
- Consistent error handling patterns with existing codebase
- Clean type definitions for the new operation
Next Steps
- Address medium priority quality issues (null checks, type safety)
- Consider performance optimizations for large batch operations
- Replace arbitrary timeout in test with proper synchronization
- Validate OperationMeta interface usage in type definitions
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
Summary
Fixes issue where node size changes are not serialized by routing DOM-driven node bounds updates through a single CRDT operation so Vue node geometry stays synchronized with LiteGraph.
Changes
BatchUpdateBoundsOperationto the layout store, applied it via the existing Yjs pipeline, notified link sync to recompute touched nodes, and covered the path with a regression testReview Focus
Correctness of the new batch operation when multiple nodes update simultaneously, especially remote replay/undo scenarios and link geometry recomputation.
┆Issue is synchronized with this Notion page by Unito