-
Notifications
You must be signed in to change notification settings - Fork 386
Fix/vue nodes snap to grid #5973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f81edc5
to
8a9b3f6
Compare
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/09/2025, 03:33:32 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/09/2025, 03:44:26 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/vue nodes snap to grid (#5973)
Impact: 133 additions, 15 deletions across 4 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 4
- Low: 2
Category Breakdown
- Architecture: 3 issues
- Security: 0 issues
- Performance: 2 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
The PR successfully integrates snap-to-grid functionality across Vue nodes and LiteGraph canvas. The new useNodeSnap composable follows Vue 3 composition API patterns well. However, the global event listener change in LGraphCanvas.ts poses integration risks by moving keydown listeners from canvas to document scope, which could interfere with other application components.
Security Considerations
No security vulnerabilities were identified. The code follows safe practices for DOM manipulation and event handling.
Performance Impact
Several performance optimizations could be implemented:
- Avoid redundant position mutations when snap positions haven't changed
- Consider caching intrinsic element sizes to reduce DOM manipulation during resize operations
- The addition of snap functionality adds minimal runtime overhead
Integration Points
The changes properly integrate with the existing layout mutation system and respect the Vue/LiteGraph boundary. The snap functionality correctly uses the existing settings system and maintains consistency with LiteGraph behavior.
Positive Observations
- Clean separation of concerns with dedicated useNodeSnap composable
- Proper integration with existing layout mutation system
- Good TypeScript typing and interface definitions
- Consistent code style and documentation
- Proper use of Vue 3 Composition API patterns
- Integration with settings system for user preferences
References
Next Steps
- Address the high priority global event listener scope change
- Consider implementing suggested performance optimizations
- Add error handling for edge cases in snap operations
- Consider adding unit tests for the new snap functionality
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
75af7e5
to
125743d
Compare
125743d
to
5669b0b
Compare
} | ||
|
||
shiftKeyState.value = isShiftPressed | ||
canvasEl.dispatchEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach mimics how other events get forwarded to litegraph such as mouseWheel. I think its better scoped and declarative as opposed to registering litegraph’s event listener on a parent element and filtering out view node input/textarea targets.
syncShiftState(initialEvent.shiftKey) | ||
|
||
// Listen for shift key press/release during the operation | ||
const handleKeyEvent = (e: KeyboardEvent) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lets us listen for the shift key event only when a vue node is being dragged. Also by managing its own listener we only need to use trackShiftKey() once in useNodeLayout.ts and useNodeResizer.ts and it will be auto cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
## What's Changed ### 🚀 Features - Implement DOMWidget for vue (#6006) - Implement drop-on-canvas + linkconnectoradapter consolidation (#5898) - Vuenodes/audio widgets (#5627) - Allow reordering of linked subgraph widgets (#5981) - Contextmenu Monkeypatch Standardization (#5977) - Fix/vue nodes snap to grid (#5973) - Select Vue Nodes After Drag (#5863) - fix Vue node widgets should be in disabled state if their slots are connected with a link (#5834) ### 🐛 Bug Fixes - [bugfix] Fix update-playwright-expectations workflow missing frontend build (#6005) - Fix: Reset size when collapsing (#6004) - fix: misc LOD polish (#6001) - Fix: Allow uncoloring Vue Nodes (#5991) - [ci] Fix detached HEAD state in Playwright update workflow (#5985) - Close zoom menu when toggling minimap visibility (#5974) ### 🔧 Maintenance - Devex: Improve dev server (#6002) - CI: Add concurrency checks to PR workflows (#6000) - [feat] Auto-remove New Browser Test Expectations label after workflow completes (#5998) - CI: Simplify update playwright expectations (maybe) (#5994) - Lint: Add tailwind linter (#5984) - [feat] Auto-remove claude-review label after CI review completes (#5983) - Fix CI: Remove explicit repository parameter causing non-reproducible test results (#5950) - refactor: Reorganize GitHub Actions for better reusability (#5949) - devex: Update CODEOWNERS (#5999) - Docs: Update agent instructions about style classes (#5990) - Style: Fix move cursors that should be grabs (#5989) - Workflow templates review (#5975) **Full Changelog**: v1.29.0...v1.29.1 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6012-1-29-1-2896d73d365081b08418f46934651c41) by [Unito](https://www.unito.io) Co-authored-by: arjansingh <[email protected]>
## Summary Enable node snap to grid in vue nodes mirroring the same behavior as litegraph. - Show node snap preview (semi transparent white box target behind node) - Resize snap to grid - Shift + drag / Auto snap - Multi select + group snap ## Changes - **What**: useNodeSnap.ts useShifyKeySync.ts setups the core hooks into both the vue node positioning/resizing system and the event forwarding technique for communicating to litegraph. ## Review Focus Both new composables and specifically the useNodeLayout modifications to batch the mutations when snapping. A key tradeoff/note is why we are using the useShifyKeySync.ts which dispatches a new shift event to the canvas layer. This approach is the cleaner / more declaritive method mimicking how other vue node -> litegraph realtime events are passed. <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5973-Fix-vue-nodes-snap-to-grid-2866d73d365081c1a058d223c8c52576) by [Unito](https://www.unito.io)
## What's Changed ### 🚀 Features - Implement DOMWidget for vue (#6006) - Implement drop-on-canvas + linkconnectoradapter consolidation (#5898) - Vuenodes/audio widgets (#5627) - Allow reordering of linked subgraph widgets (#5981) - Contextmenu Monkeypatch Standardization (#5977) - Fix/vue nodes snap to grid (#5973) - Select Vue Nodes After Drag (#5863) - fix Vue node widgets should be in disabled state if their slots are connected with a link (#5834) ### 🐛 Bug Fixes - [bugfix] Fix update-playwright-expectations workflow missing frontend build (#6005) - Fix: Reset size when collapsing (#6004) - fix: misc LOD polish (#6001) - Fix: Allow uncoloring Vue Nodes (#5991) - [ci] Fix detached HEAD state in Playwright update workflow (#5985) - Close zoom menu when toggling minimap visibility (#5974) ### 🔧 Maintenance - Devex: Improve dev server (#6002) - CI: Add concurrency checks to PR workflows (#6000) - [feat] Auto-remove New Browser Test Expectations label after workflow completes (#5998) - CI: Simplify update playwright expectations (maybe) (#5994) - Lint: Add tailwind linter (#5984) - [feat] Auto-remove claude-review label after CI review completes (#5983) - Fix CI: Remove explicit repository parameter causing non-reproducible test results (#5950) - refactor: Reorganize GitHub Actions for better reusability (#5949) - devex: Update CODEOWNERS (#5999) - Docs: Update agent instructions about style classes (#5990) - Style: Fix move cursors that should be grabs (#5989) - Workflow templates review (#5975) **Full Changelog**: v1.29.0...v1.29.1 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6012-1-29-1-2896d73d365081b08418f46934651c41) by [Unito](https://www.unito.io) Co-authored-by: arjansingh <[email protected]>
## Summary Enable node snap to grid in vue nodes mirroring the same behavior as litegraph. - Show node snap preview (semi transparent white box target behind node) - Resize snap to grid - Shift + drag / Auto snap - Multi select + group snap ## Changes - **What**: useNodeSnap.ts useShifyKeySync.ts setups the core hooks into both the vue node positioning/resizing system and the event forwarding technique for communicating to litegraph. ## Review Focus Both new composables and specifically the useNodeLayout modifications to batch the mutations when snapping. A key tradeoff/note is why we are using the useShifyKeySync.ts which dispatches a new shift event to the canvas layer. This approach is the cleaner / more declaritive method mimicking how other vue node -> litegraph realtime events are passed. <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5973-Fix-vue-nodes-snap-to-grid-2866d73d365081c1a058d223c8c52576) by [Unito](https://www.unito.io)
## What's Changed ### 🚀 Features - Implement DOMWidget for vue (#6006) - Implement drop-on-canvas + linkconnectoradapter consolidation (#5898) - Vuenodes/audio widgets (#5627) - Allow reordering of linked subgraph widgets (#5981) - Contextmenu Monkeypatch Standardization (#5977) - Fix/vue nodes snap to grid (#5973) - Select Vue Nodes After Drag (#5863) - fix Vue node widgets should be in disabled state if their slots are connected with a link (#5834) ### 🐛 Bug Fixes - [bugfix] Fix update-playwright-expectations workflow missing frontend build (#6005) - Fix: Reset size when collapsing (#6004) - fix: misc LOD polish (#6001) - Fix: Allow uncoloring Vue Nodes (#5991) - [ci] Fix detached HEAD state in Playwright update workflow (#5985) - Close zoom menu when toggling minimap visibility (#5974) ### 🔧 Maintenance - Devex: Improve dev server (#6002) - CI: Add concurrency checks to PR workflows (#6000) - [feat] Auto-remove New Browser Test Expectations label after workflow completes (#5998) - CI: Simplify update playwright expectations (maybe) (#5994) - Lint: Add tailwind linter (#5984) - [feat] Auto-remove claude-review label after CI review completes (#5983) - Fix CI: Remove explicit repository parameter causing non-reproducible test results (#5950) - refactor: Reorganize GitHub Actions for better reusability (#5949) - devex: Update CODEOWNERS (#5999) - Docs: Update agent instructions about style classes (#5990) - Style: Fix move cursors that should be grabs (#5989) - Workflow templates review (#5975) **Full Changelog**: v1.29.0...v1.29.1 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6012-1-29-1-2896d73d365081b08418f46934651c41) by [Unito](https://www.unito.io) Co-authored-by: arjansingh <[email protected]>
Summary
Enable node snap to grid in vue nodes mirroring the same behavior as litegraph.
Changes
Review Focus
Both new composables and specifically the useNodeLayout modifications to batch the mutations when snapping.
A key tradeoff/note is why we are using the useShifyKeySync.ts which dispatches a new shift event to the canvas layer. This approach is the cleaner / more declaritive method mimicking how other vue node -> litegraph realtime events are passed.
Screenshots (if applicable)
vue-nodes-feat-grid-snapping.mp4
┆Issue is synchronized with this Notion page by Unito