fix mask editor bug under vueNodes #5953
Conversation
dosubot
bot
added
the
size:M
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/08/2025, 01:43:15 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/08/2025, 01:53:22 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
Yes this PR fixed the #5816 Screen.Recording.2025-10-07.at.18.06.18.mov |
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (node) { | ||
| node.imageIndex = currentIndex.value | ||
| node.imgs = await Promise.all( |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Promise rejection not handled - Image loading could fail silently
Context: If an image fails to load in the Promise.all, the entire operation fails but there's no error handling
Suggestion: Add proper error handling with try-catch or Promise.allSettled to handle individual failures gracefully
| if (node) { | ||
| node.imageIndex = currentIndex.value | ||
| node.imgs = await Promise.all( |
There was a problem hiding this comment.
[performance] medium Priority
Issue: Blocking image loading may freeze UI
Context: Promise.all waits for all images to load synchronously, potentially causing UI freezes with large images or slow networks
Suggestion: Consider loading images asynchronously or showing a loading indicator during this operation
| if (props.nodeId) { | ||
| const node = app.rootGraph?.getNodeById(props.nodeId) | ||
| if (node) { | ||
| node.imageIndex = currentIndex.value |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Direct mutation of node object properties
Context: Directly setting node.imageIndex and node.imgs bypasses Vue reactivity and violates immutability principles
Suggestion: Use a reactive store or emit events to parent components to handle state changes properly
| if (ComfyApp.clipspace.imgs && node.imgs) { | ||
| if (node.images && ComfyApp.clipspace.images) { | ||
| // Update node.images even if it's initially undefined (vueNodes mode) | ||
| if (ComfyApp.clipspace.images) { |
There was a problem hiding this comment.
[quality] high Priority
Issue: Logic condition removed may cause null reference errors
Context: Removed check for node.images existence before accessing ClipSpace.images could cause undefined/null references
Suggestion: Restore defensive check or add null safety: if (ComfyApp.clipspace.images && node.images)
| app.graph.setDirtyCanvas(true) | ||
|
|
||
| if (node.images && app.nodeOutputs[node.id]) { | ||
| useNodeOutputStore().nodeOutputs[node.id] = { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Potential race condition with nodeOutputs access
Context: Accessing app.nodeOutputs[node.id] and useNodeOutputStore().nodeOutputs[node.id] without proper synchronization could lead to race conditions
Suggestion: Add proper locks or use atomic operations when modifying shared state
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 mask editor bug under vueNodes (#5953)
Impact: 39 additions, 4 deletions across 2 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 4
- Low: 1
Category Breakdown
- Architecture: 1 issue
- Security: 1 issue
- Performance: 1 issue
- Code Quality: 3 issues
Key Findings
Architecture & Design
The PR introduces direct state mutations on node objects, which bypasses Vue's reactivity system. This pattern could lead to inconsistent state management. Consider using proper reactive stores or event-driven communication instead.
Security Considerations
The addition of crossOrigin = 'anonymous' enables cross-origin access to images. While likely necessary for canvas operations, ensure this doesn't expose sensitive images to unauthorized access.
Performance Impact
The synchronous Promise.all for image loading could block the UI thread, especially with large images or slow networks. Consider asynchronous loading patterns or loading indicators.
Integration Points
The changes improve vueNodes integration with the mask editor by properly setting up node state and synchronizing with the node output store. However, removed safety checks could introduce runtime errors.
Positive Observations
- Good use of TypeScript with proper type annotations
- Proper async/await pattern for the mask editor function
- Icon updates follow the project's icon naming conventions (i-lucide:* format)
- Added proper fallback logic for vueNodes mode
References
Next Steps
- Address the high-priority logic issue in app.ts to prevent potential null reference errors
- Consider architectural feedback for proper state management patterns
- Add error handling for image loading operations
- Consider performance optimizations for image loading
- Validate CORS security implications
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
|
Accessing nodeOutputs by nodeId instead of nodeLocatorId will cause the code to break when run inside of subgraphs. But from looking into it, it's a decent amount of work to cleanup the surrounding code to use locatorIds instead of assuming the node resides in the active graph which is kinda out of scope. I can handle it in a separate PR after this one is merged. |
Summary
fix mask editor issues on vueNodes
┆Issue is synchronized with this Notion page by Unito