From 5224c63bce3de54515ed883c83a2f8eb50909d59 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Thu, 14 Aug 2025 12:51:43 -0700 Subject: [PATCH] [fix] Prevent incompatible connections to SubgraphInputNode occupied slots (#4984) ## Summary This PR fixes #4681 by building upon the foundation laid in PR #1182 (litegraph.js). It prevents incompatible type connections when dragging from a normal node's output to a SubgraphInputNode's occupied slot. Before: https://github.com/user-attachments/assets/03def938-dccc-4b2c-b65b-745abf02a13b After: https://github.com/user-attachments/assets/7a0a2ed4-9ecd-4147-be56-d643d448d4cb ## Background PR #1182 implemented: - `isValidTarget()` method in SubgraphInput/SubgraphOutput classes for validation - Visual feedback during drag (40% opacity for invalid targets) - Validation at the slot level However, there was a missing piece: while the visual feedback correctly showed invalid targets, the actual connection would still be made when dropped. ## Changes This PR extends PR #1182 by adding the missing connection prevention: 1. **Added `canConnectToSubgraphInput()` method** to render link classes: - `MovingOutputLink` - `ToOutputRenderLink` - `FloatingRenderLink` - All methods use the existing `SubgraphInput.isValidTarget()` from PR #1182 2. **Added validation in `LinkConnector.dropOnIoNode()`**: - Checks `canConnectToSubgraphInput()` before allowing the connection - Logs a warning when rejecting invalid connections - Follows the same pattern as regular node connections 3. **Added `isSubgraphInputValidDrop()` method**: - Provides validation for hover states - Ensures consistent validation across the UI --- .../src/canvas/FloatingRenderLink.ts | 4 + src/lib/litegraph/src/canvas/LinkConnector.ts | 22 ++ .../litegraph/src/canvas/MovingOutputLink.ts | 4 + .../src/canvas/ToOutputRenderLink.ts | 4 + ...nkConnectorSubgraphInputValidation.test.ts | 310 ++++++++++++++++++ 5 files changed, 344 insertions(+) create mode 100644 src/lib/litegraph/test/canvas/LinkConnectorSubgraphInputValidation.test.ts diff --git a/src/lib/litegraph/src/canvas/FloatingRenderLink.ts b/src/lib/litegraph/src/canvas/FloatingRenderLink.ts index 9de30f7cc..7bb61bd82 100644 --- a/src/lib/litegraph/src/canvas/FloatingRenderLink.ts +++ b/src/lib/litegraph/src/canvas/FloatingRenderLink.ts @@ -135,6 +135,10 @@ export class FloatingRenderLink implements RenderLink { return true } + canConnectToSubgraphInput(input: SubgraphInput): boolean { + return this.toType === 'output' && input.isValidTarget(this.fromSlot) + } + connectToInput( node: LGraphNode, input: INodeInputSlot, diff --git a/src/lib/litegraph/src/canvas/LinkConnector.ts b/src/lib/litegraph/src/canvas/LinkConnector.ts index 8f925bbc7..c2f977e7c 100644 --- a/src/lib/litegraph/src/canvas/LinkConnector.ts +++ b/src/lib/litegraph/src/canvas/LinkConnector.ts @@ -681,6 +681,20 @@ export class LinkConnector { let targetSlot = input for (const link of renderLinks) { + // Validate the connection type before proceeding + if ( + 'canConnectToSubgraphInput' in link && + !link.canConnectToSubgraphInput(targetSlot) + ) { + console.warn( + 'Invalid connection type', + link.fromSlot.type, + '->', + targetSlot.type + ) + continue + } + link.connectToSubgraphInput(targetSlot, this.events) // If we just connected to an EmptySubgraphInput, check if we should reuse the slot @@ -941,6 +955,14 @@ export class LinkConnector { ) } + isSubgraphInputValidDrop(input: SubgraphInput): boolean { + return this.renderLinks.some( + (link) => + 'canConnectToSubgraphInput' in link && + link.canConnectToSubgraphInput(input) + ) + } + /** * Checks if a reroute is a valid drop target for any of the links being connected. * @param reroute The reroute that would be dropped on. diff --git a/src/lib/litegraph/src/canvas/MovingOutputLink.ts b/src/lib/litegraph/src/canvas/MovingOutputLink.ts index c8b05a8c3..2a1890d73 100644 --- a/src/lib/litegraph/src/canvas/MovingOutputLink.ts +++ b/src/lib/litegraph/src/canvas/MovingOutputLink.ts @@ -55,6 +55,10 @@ export class MovingOutputLink extends MovingLinkBase { return reroute.origin_id !== this.outputNode.id } + canConnectToSubgraphInput(input: SubgraphInput): boolean { + return input.isValidTarget(this.fromSlot) + } + connectToInput(): never { throw new Error('MovingOutputLink cannot connect to an input.') } diff --git a/src/lib/litegraph/src/canvas/ToOutputRenderLink.ts b/src/lib/litegraph/src/canvas/ToOutputRenderLink.ts index aac98eda0..9f94b7796 100644 --- a/src/lib/litegraph/src/canvas/ToOutputRenderLink.ts +++ b/src/lib/litegraph/src/canvas/ToOutputRenderLink.ts @@ -58,6 +58,10 @@ export class ToOutputRenderLink implements RenderLink { return true } + canConnectToSubgraphInput(input: SubgraphInput): boolean { + return input.isValidTarget(this.fromSlot) + } + connectToOutput( node: LGraphNode, output: INodeOutputSlot, diff --git a/src/lib/litegraph/test/canvas/LinkConnectorSubgraphInputValidation.test.ts b/src/lib/litegraph/test/canvas/LinkConnectorSubgraphInputValidation.test.ts new file mode 100644 index 000000000..d9951656e --- /dev/null +++ b/src/lib/litegraph/test/canvas/LinkConnectorSubgraphInputValidation.test.ts @@ -0,0 +1,310 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { LinkConnector } from '@/lib/litegraph/src/canvas/LinkConnector' +import { MovingOutputLink } from '@/lib/litegraph/src/canvas/MovingOutputLink' +import { ToOutputRenderLink } from '@/lib/litegraph/src/canvas/ToOutputRenderLink' +import { LGraphNode, LLink } from '@/lib/litegraph/src/litegraph' +import { NodeInputSlot } from '@/lib/litegraph/src/node/NodeInputSlot' + +import { createTestSubgraph } from '../subgraph/fixtures/subgraphHelpers' + +describe('LinkConnector SubgraphInput connection validation', () => { + let connector: LinkConnector + const mockSetConnectingLinks = vi.fn() + + beforeEach(() => { + connector = new LinkConnector(mockSetConnectingLinks) + vi.clearAllMocks() + }) + + describe('MovingOutputLink validation', () => { + it('should implement canConnectToSubgraphInput method', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'number_input', type: 'number' }] + }) + + const sourceNode = new LGraphNode('SourceNode') + sourceNode.addOutput('number_out', 'number') + subgraph.add(sourceNode) + + const targetNode = new LGraphNode('TargetNode') + targetNode.addInput('number_in', 'number') + subgraph.add(targetNode) + + const link = new LLink(1, 'number', sourceNode.id, 0, targetNode.id, 0) + subgraph._links.set(link.id, link) + + const movingLink = new MovingOutputLink(subgraph, link) + + // Verify the method exists + expect(typeof movingLink.canConnectToSubgraphInput).toBe('function') + }) + + it('should validate type compatibility correctly', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'number_input', type: 'number' }] + }) + + const sourceNode = new LGraphNode('SourceNode') + sourceNode.addOutput('number_out', 'number') + sourceNode.addOutput('string_out', 'string') + subgraph.add(sourceNode) + + const targetNode = new LGraphNode('TargetNode') + targetNode.addInput('number_in', 'number') + targetNode.addInput('string_in', 'string') + subgraph.add(targetNode) + + // Create valid link (number -> number) + const validLink = new LLink( + 1, + 'number', + sourceNode.id, + 0, + targetNode.id, + 0 + ) + subgraph._links.set(validLink.id, validLink) + const validMovingLink = new MovingOutputLink(subgraph, validLink) + + // Create invalid link (string -> number) + const invalidLink = new LLink( + 2, + 'string', + sourceNode.id, + 1, + targetNode.id, + 1 + ) + subgraph._links.set(invalidLink.id, invalidLink) + const invalidMovingLink = new MovingOutputLink(subgraph, invalidLink) + + const numberInput = subgraph.inputs[0] + + // Test validation + expect(validMovingLink.canConnectToSubgraphInput(numberInput)).toBe(true) + expect(invalidMovingLink.canConnectToSubgraphInput(numberInput)).toBe( + false + ) + }) + + it('should handle wildcard types', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'wildcard_input', type: '*' }] + }) + + const sourceNode = new LGraphNode('SourceNode') + sourceNode.addOutput('number_out', 'number') + subgraph.add(sourceNode) + + const targetNode = new LGraphNode('TargetNode') + targetNode.addInput('number_in', 'number') + subgraph.add(targetNode) + + const link = new LLink(1, 'number', sourceNode.id, 0, targetNode.id, 0) + subgraph._links.set(link.id, link) + const movingLink = new MovingOutputLink(subgraph, link) + + const wildcardInput = subgraph.inputs[0] + + // Wildcard should accept any type + expect(movingLink.canConnectToSubgraphInput(wildcardInput)).toBe(true) + }) + }) + + describe('ToOutputRenderLink validation', () => { + it('should implement canConnectToSubgraphInput method', () => { + // Create a minimal valid setup + const subgraph = createTestSubgraph() + const node = new LGraphNode('TestNode') + node.id = 1 + node.addInput('test_in', 'number') + subgraph.add(node) + + const slot = node.inputs[0] as NodeInputSlot + const renderLink = new ToOutputRenderLink(subgraph, node, slot) + + // Verify the method exists + expect(typeof renderLink.canConnectToSubgraphInput).toBe('function') + }) + }) + + describe('dropOnIoNode validation', () => { + it('should prevent invalid connections when dropping on SubgraphInputNode', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'number_input', type: 'number' }] + }) + + const sourceNode = new LGraphNode('SourceNode') + sourceNode.addOutput('string_out', 'string') + subgraph.add(sourceNode) + + const targetNode = new LGraphNode('TargetNode') + targetNode.addInput('string_in', 'string') + subgraph.add(targetNode) + + // Create an invalid link (string output -> string input, but subgraph expects number) + const link = new LLink(1, 'string', sourceNode.id, 0, targetNode.id, 0) + subgraph._links.set(link.id, link) + const movingLink = new MovingOutputLink(subgraph, link) + + // Mock console.warn to verify it's called + const consoleWarnSpy = vi + .spyOn(console, 'warn') + .mockImplementation(() => {}) + + // Add the link to the connector + connector.renderLinks.push(movingLink) + connector.state.connectingTo = 'output' + + // Create mock event + const mockEvent = { + canvasX: 100, + canvasY: 100 + } as any + + // Mock the getSlotInPosition to return the subgraph input + const mockGetSlotInPosition = vi.fn().mockReturnValue(subgraph.inputs[0]) + subgraph.inputNode.getSlotInPosition = mockGetSlotInPosition + + // Spy on connectToSubgraphInput to ensure it's NOT called + const connectSpy = vi.spyOn(movingLink, 'connectToSubgraphInput') + + // Drop on the SubgraphInputNode + connector.dropOnIoNode(subgraph.inputNode, mockEvent) + + // Verify that the invalid connection was skipped + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Invalid connection type', + 'string', + '->', + 'number' + ) + expect(connectSpy).not.toHaveBeenCalled() + + consoleWarnSpy.mockRestore() + }) + + it('should allow valid connections when dropping on SubgraphInputNode', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'number_input', type: 'number' }] + }) + + const sourceNode = new LGraphNode('SourceNode') + sourceNode.addOutput('number_out', 'number') + subgraph.add(sourceNode) + + const targetNode = new LGraphNode('TargetNode') + targetNode.addInput('number_in', 'number') + subgraph.add(targetNode) + + // Create a valid link (number -> number) + const link = new LLink(1, 'number', sourceNode.id, 0, targetNode.id, 0) + subgraph._links.set(link.id, link) + const movingLink = new MovingOutputLink(subgraph, link) + + // Add the link to the connector + connector.renderLinks.push(movingLink) + connector.state.connectingTo = 'output' + + // Create mock event + const mockEvent = { + canvasX: 100, + canvasY: 100 + } as any + + // Mock the getSlotInPosition to return the subgraph input + const mockGetSlotInPosition = vi.fn().mockReturnValue(subgraph.inputs[0]) + subgraph.inputNode.getSlotInPosition = mockGetSlotInPosition + + // Spy on connectToSubgraphInput to ensure it IS called + const connectSpy = vi.spyOn(movingLink, 'connectToSubgraphInput') + + // Drop on the SubgraphInputNode + connector.dropOnIoNode(subgraph.inputNode, mockEvent) + + // Verify that the valid connection was made + expect(connectSpy).toHaveBeenCalledWith( + subgraph.inputs[0], + connector.events + ) + }) + }) + + describe('isSubgraphInputValidDrop', () => { + it('should check if render links can connect to SubgraphInput', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'number_input', type: 'number' }] + }) + + const sourceNode = new LGraphNode('SourceNode') + sourceNode.addOutput('number_out', 'number') + sourceNode.addOutput('string_out', 'string') + subgraph.add(sourceNode) + + const targetNode = new LGraphNode('TargetNode') + targetNode.addInput('number_in', 'number') + targetNode.addInput('string_in', 'string') + subgraph.add(targetNode) + + // Create valid and invalid links + const validLink = new LLink( + 1, + 'number', + sourceNode.id, + 0, + targetNode.id, + 0 + ) + const invalidLink = new LLink( + 2, + 'string', + sourceNode.id, + 1, + targetNode.id, + 1 + ) + subgraph._links.set(validLink.id, validLink) + subgraph._links.set(invalidLink.id, invalidLink) + + const validMovingLink = new MovingOutputLink(subgraph, validLink) + const invalidMovingLink = new MovingOutputLink(subgraph, invalidLink) + + const subgraphInput = subgraph.inputs[0] + + // Test with only invalid link + connector.renderLinks.length = 0 + connector.renderLinks.push(invalidMovingLink) + expect(connector.isSubgraphInputValidDrop(subgraphInput)).toBe(false) + + // Test with valid link + connector.renderLinks.length = 0 + connector.renderLinks.push(validMovingLink) + expect(connector.isSubgraphInputValidDrop(subgraphInput)).toBe(true) + + // Test with mixed links + connector.renderLinks.length = 0 + connector.renderLinks.push(invalidMovingLink, validMovingLink) + expect(connector.isSubgraphInputValidDrop(subgraphInput)).toBe(true) + }) + + it('should handle render links without canConnectToSubgraphInput method', () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: 'number_input', type: 'number' }] + }) + + // Create a mock render link without the method + const mockLink = { + fromSlot: { type: 'number' } + // No canConnectToSubgraphInput method + } as any + + connector.renderLinks.push(mockLink) + + const subgraphInput = subgraph.inputs[0] + + // Should return false as the link doesn't have the method + expect(connector.isSubgraphInputValidDrop(subgraphInput)).toBe(false) + }) + }) +})