-
Notifications
You must be signed in to change notification settings - Fork 377
Fix reroute links not snapping correctly #5903
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
base: bl-linkslot-refactor
Are you sure you want to change the base?
Conversation
…\n\n- Route getInputPos/getInputSlotPos/getOutputPos through getSlotPosition(), which prefers layoutStore DOM-tracked positions and falls back to geometry.\n- Ensures reroute-origin drags snap to visually tracked node slots when hovering compatible nodes in Vue nodes mode.\n- No change for non-Vue nodes mode or when no tracked slot layout is present.
🎭 Playwright Test Results⏰ Completed at: 10/03/2025, 02:00:41 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
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 reroute links not snapping correctly (#5903)
Impact: 27 additions, 8 deletions across 2 files
Files Modified:
- src/lib/litegraph/src/LGraphCanvas.ts
- src/lib/litegraph/src/LGraphNode.ts
Issue Distribution
- Critical: 0
- High: 0
- Medium: 0
- Low: 0
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 0 issues
- Code Quality: 0 issues
Key Findings
Architecture & Design
✅ Excellent architectural consistency: The PR correctly centralizes slot position calculations through the existing getSlotPosition function, which follows the established pattern of consulting the layout store first before falling back to geometry calculations.
✅ Proper abstraction layer: The changes maintain clean separation between Vue node layout (DOM-tracked) and LiteGraph geometry calculations, with appropriate fallback behavior.
✅ Consistent implementation: All six modification points follow the same conditional pattern: LiteGraph.vueNodesMode ? getSlotPosition(...) : legacyMethod(...)
Security Considerations
✅ No security implications: The changes are purely computational and don't introduce any security vectors. All inputs are properly typed and validated through existing mechanisms.
Performance Impact
✅ Performance neutral: The changes add minimal conditional checks (LiteGraph.vueNodesMode) but don't introduce expensive operations. The getSlotPosition function itself is already optimized with layoutStore caching.
✅ Bundle impact: No additional dependencies or imports, changes only routing of existing function calls.
Integration Points
✅ Backward compatibility maintained: Non-Vue nodes mode continues to use legacy calculation methods, ensuring no breaking changes for existing workflows.
✅ Extension compatibility: Changes are internal to LiteGraph slot position calculations and don't affect the extension API surface.
Positive Observations
🎯 Problem-focused solution: The PR directly addresses the stated issue of reroute-origin drags not snapping to the same visual slot centers as Vue nodes, with a surgical approach that touches only the necessary code paths.
🏗️ Code quality:
- Proper TypeScript typing with explicit Point annotations
- Consistent conditional logic pattern across all changes
- Meaningful variable names and clear intent
- Maintains existing error handling and fallback mechanisms
🔧 Implementation details:
- Smart use of indexOf to convert slot objects to indices where needed
- Proper handling of the result.index from findInputByType/findOutputByType methods
- Clean parameter renaming in getOutputPos (slot → outputSlotIndex) improves code clarity
Technical Analysis
The core change routes LiteGraph slot position queries through getSlotPosition(), which:
- First attempts to retrieve DOM-tracked positions from layoutStore.getSlotLayout()
- Falls back to node layout tree calculations if available
- Finally falls back to original LiteGraph geometry calculations
This creates a single source of truth for slot positioning that prioritizes accuracy from DOM measurements while maintaining all existing fallback behavior.
Next Steps
✅ Ready for merge: This PR is well-implemented with no issues identified. The changes are minimal, focused, and follow established architectural patterns.
📋 Testing: Verify that reroute-origin drag snapping now matches Vue node hover behavior in manual testing.
This is a comprehensive automated review. The high code quality and surgical approach of this PR made it an exemplary implementation with no issues requiring correction.
## Summary Snap link preview to the nearest compatible slot while dragging in Vue Nodes mode, and complete the connection on drop using the snapped target. Mirrors LiteGraph’s first-compatible-slot logic for node-level snapping and reuses the computed candidate for performance. ## Changes - Snap preview end to compatible slot - slot under cursor via `data-slot-key` fast-path - node under cursor via `findInputByType` / `findOutputByType` - Render path - `slotLinkPreviewRenderer.ts` now renders to `state.candidate.layout.position` - Complete on drop - Prefer `state.candidate` (no re-hit-testing) - Fallbacks: DOM slot → node first-compatible → reroute - Disconnects moving input link when dropped on canvas ## Review Focus - UX feel of snapping and drop completion (both directions) - Performance on large graphs (mousemove path is O(1) with dataset + single validation) - Edge cases: reroutes, moving existing links, collapsed nodes ## Screenshots (if applicable) https://github.com/user-attachments/assets/fbed0ae2-2231-473b-a05a-9aaf68e3f820 #5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5780-feat-vue-nodes-snap-link-preview-connect-on-drop-27a6d73d365081d89c8cf570e2049c89) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <[email protected]>
Summary
What changed
Why
Impact
#5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping)
┆Issue is synchronized with this Notion page by Unito