Increase vue slot/link functionality #5710
Conversation
🎭 Playwright Test Results⏰ Completed at: 09/26/2025, 02:56:06 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
the two failing tests are screenshot expectations that I added that were bugged from the overlapping dirty workflow dot indicator and the close workflow button |
dosubot
bot
added
the
size:XXL
There was a problem hiding this comment.
question (non-blocking): Can you explain these changes briefly?
There was a problem hiding this comment.
this was for testing/iteration, and was supposed to be removed in the 'those who know' commit, but wasn't cleaned up fully
it didn't have any callers, I just removed it now
| type InputConnectableLink = RenderLink & { | ||
| toType: 'input' | ||
| canConnectToInput: (node: LGraphNode, input: INodeInputSlot) => boolean | ||
| } | ||
|
|
||
| type OutputConnectableLink = RenderLink & { | ||
| toType: 'output' | ||
| canConnectToOutput: (node: LGraphNode, output: INodeOutputSlot) => boolean | ||
| } | ||
|
|
||
| function isInputConnectableLink( | ||
| link: RenderLink | ||
| ): link is InputConnectableLink { | ||
| return ( | ||
| link.toType === 'input' && | ||
| typeof (link as { canConnectToInput?: unknown }).canConnectToInput === | ||
| 'function' | ||
| ) | ||
| } | ||
|
|
||
| function isOutputConnectableLink( | ||
| link: RenderLink | ||
| ): link is OutputConnectableLink { | ||
| return ( | ||
| link.toType === 'output' && | ||
| typeof (link as { canConnectToOutput?: unknown }).canConnectToOutput === | ||
| 'function' | ||
| ) | ||
| } |
There was a problem hiding this comment.
suggestion (non-blocking): We could consider using more robust type guards and/or discriminated unions for link types.
There was a problem hiding this comment.
true, changed this to intersection typed, and I think a true discriminated union for RenderLink can be done in another PR
| const adapter = ensureActiveAdapter() | ||
| if (graph && adapter) { | ||
| if (layout.type === 'input') { | ||
| candidate.compatible = adapter.isInputValidDrop( | ||
| layout.nodeId, | ||
| layout.index | ||
| ) | ||
| } else if (layout.type === 'output') { | ||
| candidate.compatible = adapter.isOutputValidDrop( | ||
| layout.nodeId, | ||
| layout.index | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
question: What is the lifecycle of the adapter like? Is it necessary to have explicit cleanup?
There was a problem hiding this comment.
ignoring the quoted code, the adapter is a weakmap per graph
we need the explicit cleanup because the adapter has its own linkconnector, so the regular linkconnector won't reset for us, and reset is used for things like _dragging flags and anything using listenUntilReset
| outputNode: LGraphNode, | ||
| output: INodeOutputSlot, | ||
| events: CustomEventTarget<LinkConnectorEventMap> | ||
| ): LLink | null | undefined { |
There was a problem hiding this comment.
question: Is it possible to make the meaning of the return types more explicit?
The API contract might be somewhat confusing if undefined and null have different semantics. If they are the same, would it be possible to only return null or only return undefined?
There was a problem hiding this comment.
this code was removed after addressing other review comments
| // This avoids relying on an exact path hit-test position. | ||
| await comfyPage.page.evaluate( | ||
| ([targetNodeId, targetSlot, clientPoint]) => { | ||
| const app = (window as any)['app'] |
There was a problem hiding this comment.
nit (non-blocking): could we use "expect-error" directive so this is easier to cleanup/refactor/audit later on?
There was a problem hiding this comment.
wouldn't that get flagged when linting? also, I think the PR that will switch off of as any will have a simple script to prepend with expect error
| const colour = resolveConnectingLinkColor(sourceSlot?.type) | ||
| const adapter = createLinkConnectorAdapter() | ||
| const renderLinks = adapter?.renderLinks | ||
| if (!adapter || !renderLinks || renderLinks.length === 0) return |
There was a problem hiding this comment.
nit: if adapter is falsy here, then so is renderLinks by necessity.
Summary
Increase functionality for slots and links, covered with playwright tests.
Features
Tests Added
Playwright
Notes
The double rendering system for links being dragged, it works right now, maybe they can be coalesced later.
Edit: As in the adapter, can be removed in a followup PR
Also, it's known that more features will arrive in smaller PRs, this PR actually should've been much smaller.
The next ones coming up are drop on canvas support, snap to node, type compatibility highlighting, and working with subgraphs.
┆Issue is synchronized with this Notion page by Unito