From f0adb4c9d3eeab635b1f2d89781098b8a3c420e9 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sun, 17 Aug 2025 11:14:53 -0700 Subject: [PATCH] [bugfix] Allow removeInput/removeOutput on nodes without graph reference (#5053) - Modified removeInput/removeOutput to skip disconnect operations when node has no graph - Both methods now safely handle nodes that aren't part of a graph - Added comprehensive tests for the new behavior - Fixes #5037 --- src/lib/litegraph/src/LGraphNode.ts | 29 ++++-- src/lib/litegraph/test/LGraphNode.test.ts | 115 ++++++++++++++++++++++ 2 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/lib/litegraph/src/LGraphNode.ts b/src/lib/litegraph/src/LGraphNode.ts index 14bfee485..6d8b202b4 100644 --- a/src/lib/litegraph/src/LGraphNode.ts +++ b/src/lib/litegraph/src/LGraphNode.ts @@ -1574,7 +1574,10 @@ export class LGraphNode * remove an existing output slot */ removeOutput(slot: number): void { - this.disconnectOutput(slot) + // Only disconnect if node is part of a graph + if (this.graph) { + this.disconnectOutput(slot) + } const { outputs } = this outputs.splice(slot, 1) @@ -1582,11 +1585,12 @@ export class LGraphNode const output = outputs[i] if (!output || !output.links) continue - for (const linkId of output.links) { - if (!this.graph) throw new NullGraphError() - - const link = this.graph._links.get(linkId) - if (link) link.origin_slot-- + // Only update link indices if node is part of a graph + if (this.graph) { + for (const linkId of output.links) { + const link = this.graph._links.get(linkId) + if (link) link.origin_slot-- + } } } @@ -1626,7 +1630,10 @@ export class LGraphNode * remove an existing input slot */ removeInput(slot: number): void { - this.disconnectInput(slot, true) + // Only disconnect if node is part of a graph + if (this.graph) { + this.disconnectInput(slot, true) + } const { inputs } = this const slot_info = inputs.splice(slot, 1) @@ -1634,9 +1641,11 @@ export class LGraphNode const input = inputs[i] if (!input?.link) continue - if (!this.graph) throw new NullGraphError() - const link = this.graph._links.get(input.link) - if (link) link.target_slot-- + // Only update link indices if node is part of a graph + if (this.graph) { + const link = this.graph._links.get(input.link) + if (link) link.target_slot-- + } } this.onInputRemoved?.(slot, slot_info[0]) this.setDirtyCanvas(true, true) diff --git a/src/lib/litegraph/test/LGraphNode.test.ts b/src/lib/litegraph/test/LGraphNode.test.ts index 6dd36cd8d..ed7d15329 100644 --- a/src/lib/litegraph/test/LGraphNode.test.ts +++ b/src/lib/litegraph/test/LGraphNode.test.ts @@ -656,4 +656,119 @@ describe('LGraphNode', () => { spy.mockRestore() }) }) + + describe('removeInput/removeOutput on copied nodes', () => { + beforeEach(() => { + // Register a test node type so clone() can work + LiteGraph.registerNodeType('TestNode', LGraphNode) + }) + + test('should NOT throw error when calling removeInput on a copied node without graph', () => { + // Create a node with an input + const originalNode = new LGraphNode('Test Node') + originalNode.type = 'TestNode' + originalNode.addInput('input1', 'number') + + // Clone the node (which creates a node without graph reference) + const copiedNode = originalNode.clone() + + // This should NOT throw anymore - we can remove inputs on nodes without graph + expect(() => copiedNode!.removeInput(0)).not.toThrow() + expect(copiedNode!.inputs).toHaveLength(0) + }) + + test('should NOT throw error when calling removeOutput on a copied node without graph', () => { + // Create a node with an output + const originalNode = new LGraphNode('Test Node') + originalNode.type = 'TestNode' + originalNode.addOutput('output1', 'number') + + // Clone the node (which creates a node without graph reference) + const copiedNode = originalNode.clone() + + // This should NOT throw anymore - we can remove outputs on nodes without graph + expect(() => copiedNode!.removeOutput(0)).not.toThrow() + expect(copiedNode!.outputs).toHaveLength(0) + }) + + test('should skip disconnectInput/disconnectOutput when node has no graph', () => { + // Create nodes with input/output + const nodeWithInput = new LGraphNode('Test Node') + nodeWithInput.type = 'TestNode' + nodeWithInput.addInput('input1', 'number') + + const nodeWithOutput = new LGraphNode('Test Node') + nodeWithOutput.type = 'TestNode' + nodeWithOutput.addOutput('output1', 'number') + + // Clone nodes (no graph reference) + const clonedInput = nodeWithInput.clone() + const clonedOutput = nodeWithOutput.clone() + + // Mock disconnect methods to verify they're not called + clonedInput!.disconnectInput = vi.fn() + clonedOutput!.disconnectOutput = vi.fn() + + // Remove input/output - disconnect methods should NOT be called + clonedInput!.removeInput(0) + clonedOutput!.removeOutput(0) + + expect(clonedInput!.disconnectInput).not.toHaveBeenCalled() + expect(clonedOutput!.disconnectOutput).not.toHaveBeenCalled() + }) + + test('should be able to removeInput on a copied node after adding to graph', () => { + // Create a graph and a node with an input + const graph = new LGraph() + const originalNode = new LGraphNode('Test Node') + originalNode.type = 'TestNode' + originalNode.addInput('input1', 'number') + + // Clone the node and add to graph + const copiedNode = originalNode.clone() + expect(copiedNode).not.toBeNull() + graph.add(copiedNode!) + + // This should work now that the node has a graph reference + expect(() => copiedNode!.removeInput(0)).not.toThrow() + expect(copiedNode!.inputs).toHaveLength(0) + }) + + test('should be able to removeOutput on a copied node after adding to graph', () => { + // Create a graph and a node with an output + const graph = new LGraph() + const originalNode = new LGraphNode('Test Node') + originalNode.type = 'TestNode' + originalNode.addOutput('output1', 'number') + + // Clone the node and add to graph + const copiedNode = originalNode.clone() + expect(copiedNode).not.toBeNull() + graph.add(copiedNode!) + + // This should work now that the node has a graph reference + expect(() => copiedNode!.removeOutput(0)).not.toThrow() + expect(copiedNode!.outputs).toHaveLength(0) + }) + + test('RerouteNode clone scenario - should be able to removeOutput and addOutput on cloned node', () => { + // This simulates the RerouteNode clone method behavior + const originalNode = new LGraphNode('Reroute') + originalNode.type = 'TestNode' + originalNode.addOutput('*', '*') + + // Clone the node (simulating RerouteNode.clone) + const clonedNode = originalNode.clone() + expect(clonedNode).not.toBeNull() + + // This should not throw - we should be able to modify outputs on a cloned node + expect(() => { + clonedNode!.removeOutput(0) + clonedNode!.addOutput('renamed', '*') + }).not.toThrow() + + expect(clonedNode!.outputs).toHaveLength(1) + expect(clonedNode!.outputs[0].name).toBe('renamed') + }) + }) })