-
Notifications
You must be signed in to change notification settings - Fork 376
feat(vue-nodes): snap link preview; connect on drop #5780
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: main
Are you sure you want to change the base?
Conversation
@codex review |
Codex Review: Something went wrong. Try again later by commenting “@codex review”.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/03/2025, 10:31:41 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
regenerating after the canvasonly flag was added, widgets moved around, links stayed the same though |
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
When vue nodes is enabled, switch to the new getSlotPosition for subgraph nodes as well. This aligns links to slots in subgraphs. Do note, getSlotPosition is actually the new standard, but we fallback to inputNode.getInputPos since it's so core, idea is to switch over fully to getSlotPosition without the vue flag once things are more stable. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5876-Align-links-to-slots-in-subgraphs-27f6d73d365081148b92f312a0af84a0) by [Unito](https://www.unito.io)
- Snap preview target: use snapped candidate position when compatible - Vue nodes interaction: track hovered candidate via setCandidate - Keep PR browser test snapshots (ours) for link interactions
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 seems like a lot of state to keep synchronized 🫤
reset: () => { | ||
state.compatCache = new Map() | ||
state.nodePreferred = new Map() | ||
state.lastHoverSlotKey = null | ||
state.lastHoverNodeId = null | ||
state.lastCandidateKey = null | ||
state.pendingMove = null | ||
}, | ||
dispose: () => { | ||
state.reset() | ||
} |
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.
Why would this persist and be reset instead of just being dropped and garbage collected?
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.
I think it's because it will be alive for the lifetime of the composable
it's called as const dragSession = createSlotLinkDragSession()
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.
It seems like a microoptimization in advance to me. Did you see performance issues without it?
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.
I don't think this was meant for performance, just to save like a few lines from both files
const snappedCandidate = state.candidate?.compatible | ||
? state.candidate | ||
: null | ||
|
||
let connected = tryConnectToCandidate(snappedCandidate) | ||
|
||
// Fallback to DOM slot under pointer (if any), then node fallback, then reroute | ||
if (!connected) { | ||
const domCandidate = candidateFromTarget(event.target) | ||
connected = tryConnectToCandidate(domCandidate) | ||
} | ||
|
||
if (!connected) { | ||
const nodeCandidate = candidateFromNodeTarget(event.target) | ||
connected = tryConnectToCandidate(nodeCandidate) | ||
} | ||
|
||
if (!connected) connected = tryConnectViaRerouteAtPointer() || connected |
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.
nit: This kind of long chain of optional assignment to a mutable variable is a great case for extraction to a more pure function.
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.
there's a lot of scuffed code that I want to fix next week, this can be one of them
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
data-slot-key
fast-pathfindInputByType
/findOutputByType
slotLinkPreviewRenderer.ts
now renders tostate.candidate.layout.position
state.candidate
(no re-hit-testing)Review Focus
Screenshots (if applicable)
Recording.2025-09-25.172342.mp4
┆Issue is synchronized with this Notion page by Unito