diff --git a/CLAUDE.md b/CLAUDE.md index cebc67dac..c04edd40c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,3 +24,39 @@ - Be sure to typecheck when you’re done making a series of code changes - Prefer running single tests, and not the whole test suite, for performance + +# Testing Guidelines + +## Avoiding Circular Dependencies in Tests + +**CRITICAL**: When writing tests for subgraph-related code, always import from the barrel export to avoid circular dependency issues: + +```typescript +// ✅ CORRECT - Use barrel import +import { LGraph, Subgraph, SubgraphNode } from "@/litegraph" + +// ❌ WRONG - Direct imports cause circular dependency +import { LGraph } from "@/LGraph" +import { Subgraph } from "@/subgraph/Subgraph" +import { SubgraphNode } from "@/subgraph/SubgraphNode" +``` + +**Root cause**: `LGraph` and `Subgraph` have a circular dependency: +- `LGraph.ts` imports `Subgraph` (creates instances with `new Subgraph()`) +- `Subgraph.ts` extends `LGraph` + +The barrel export (`@/litegraph`) handles this properly, but direct imports cause module loading failures. + +## Test Setup for Subgraphs + +Use the provided test helpers for consistent setup: + +```typescript +import { createTestSubgraph, createTestSubgraphNode } from "./fixtures/subgraphHelpers" + +function createTestSetup() { + const subgraph = createTestSubgraph() + const subgraphNode = createTestSubgraphNode(subgraph) + return { subgraph, subgraphNode } +} +``` diff --git a/src/infrastructure/SubgraphEventMap.ts b/src/infrastructure/SubgraphEventMap.ts index 080ddca95..48c7072be 100644 --- a/src/infrastructure/SubgraphEventMap.ts +++ b/src/infrastructure/SubgraphEventMap.ts @@ -1,6 +1,8 @@ import type { LGraphEventMap } from "./LGraphEventMap" import type { SubgraphInput } from "@/subgraph/SubgraphInput" +import type { SubgraphNode } from "@/subgraph/SubgraphNode" import type { SubgraphOutput } from "@/subgraph/SubgraphOutput" +import type { IBaseWidget } from "@/types/widgets" export interface SubgraphEventMap extends LGraphEventMap { "adding-input": { @@ -40,4 +42,13 @@ export interface SubgraphEventMap extends LGraphEventMap { oldName: string newName: string } + + "widget-promoted": { + widget: IBaseWidget + subgraphNode: SubgraphNode + } + "widget-unpromoted": { + widget: IBaseWidget + subgraphNode: SubgraphNode + } } diff --git a/src/subgraph/SubgraphNode.ts b/src/subgraph/SubgraphNode.ts index a5295b94e..0ba651842 100644 --- a/src/subgraph/SubgraphNode.ts +++ b/src/subgraph/SubgraphNode.ts @@ -187,8 +187,13 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph { } #setWidget(subgraphInput: Readonly, input: INodeInputSlot, widget: Readonly) { - // Use the first matching widget - const promotedWidget = toConcreteWidget(widget, this).createCopyForNode(this) + const concreteWidget = toConcreteWidget(widget, this) + + const promotedWidget = concreteWidget.createCopyForNode(this) + + // Set parentSubgraphNode for all promoted widgets to track their origin + promotedWidget.parentSubgraphNode = this + Object.assign(promotedWidget, { get name() { return subgraphInput.name @@ -212,6 +217,9 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph { this.widgets.push(promotedWidget) + // Dispatch widget-promoted event + this.subgraph.events.dispatch("widget-promoted", { widget: promotedWidget, subgraphNode: this }) + input.widget = { name: subgraphInput.name } input._widget = promotedWidget } @@ -307,7 +315,28 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph { return nodes } + override removeWidgetByName(name: string): void { + const widget = this.widgets.find(w => w.name === name) + if (widget) { + this.subgraph.events.dispatch("widget-unpromoted", { widget, subgraphNode: this }) + } + super.removeWidgetByName(name) + } + + override ensureWidgetRemoved(widget: IBaseWidget): void { + if (this.widgets.includes(widget)) { + this.subgraph.events.dispatch("widget-unpromoted", { widget, subgraphNode: this }) + } + super.ensureWidgetRemoved(widget) + } + override onRemoved(): void { + // Clean up all promoted widgets + for (const widget of this.widgets) { + widget.parentSubgraphNode = undefined + this.subgraph.events.dispatch("widget-unpromoted", { widget, subgraphNode: this }) + } + for (const input of this.inputs) { input._listenerController?.abort() } diff --git a/src/types/widgets.ts b/src/types/widgets.ts index fb397abb3..b01a97bee 100644 --- a/src/types/widgets.ts +++ b/src/types/widgets.ts @@ -201,6 +201,12 @@ export interface IBaseWidget< tooltip?: string + /** + * Reference to the subgraph container node when this widget is promoted from a subgraph. + * This allows the widget to know which SubgraphNode it belongs to in the parent graph. + */ + parentSubgraphNode?: LGraphNode + // TODO: Confirm this format callback?( value: any, diff --git a/src/widgets/BaseWidget.ts b/src/widgets/BaseWidget.ts index ba0d9cf41..9cd66a729 100644 --- a/src/widgets/BaseWidget.ts +++ b/src/widgets/BaseWidget.ts @@ -56,6 +56,12 @@ export abstract class BaseWidget impl return this.#node } + /** + * Reference to the subgraph container node when this widget is promoted from a subgraph. + * This allows the widget to know which SubgraphNode it belongs to in the parent graph. + */ + parentSubgraphNode?: LGraphNode + linkedWidgets?: IBaseWidget[] name: string options: TWidget["options"] @@ -297,4 +303,12 @@ export abstract class BaseWidget impl cloned.value = this.value return cloned } + + /** + * Type guard to check if this widget has a DOM element. + * @returns True if the widget has a DOM element attached + */ + isDOMWidget(): this is this & { element: HTMLElement } { + return this.element instanceof HTMLElement + } } diff --git a/test/subgraph/SubgraphEdgeCases.test.ts b/test/subgraph/SubgraphEdgeCases.test.ts index a7c4157df..c6ee6156f 100644 --- a/test/subgraph/SubgraphEdgeCases.test.ts +++ b/test/subgraph/SubgraphEdgeCases.test.ts @@ -67,9 +67,9 @@ describe("SubgraphEdgeCases - Recursion Detection", () => { it("should respect MAX_NESTED_SUBGRAPHS constant", () => { // Verify the constant exists and is a reasonable positive number expect(Subgraph.MAX_NESTED_SUBGRAPHS).toBeDefined() - expect(typeof Subgraph.MAX_NESTED_SUBGRAPHS).toBe('number') + expect(typeof Subgraph.MAX_NESTED_SUBGRAPHS).toBe("number") expect(Subgraph.MAX_NESTED_SUBGRAPHS).toBeGreaterThan(0) - expect(Subgraph.MAX_NESTED_SUBGRAPHS).toBeLessThanOrEqual(10000) // Reasonable upper bound + expect(Subgraph.MAX_NESTED_SUBGRAPHS).toBeLessThanOrEqual(10_000) // Reasonable upper bound // Note: Currently not enforced in implementation // This test documents the intended behavior @@ -313,7 +313,7 @@ describe("SubgraphEdgeCases - Performance and Scale", () => { const flattened = subgraphNode.getInnerNodes(executableNodes) expect(flattened).toHaveLength(50) - + // Performance is acceptable for 50 nodes (typically < 1ms) }) diff --git a/test/subgraph/SubgraphWidgetPromotion.test.ts b/test/subgraph/SubgraphWidgetPromotion.test.ts new file mode 100644 index 000000000..7598c8fb6 --- /dev/null +++ b/test/subgraph/SubgraphWidgetPromotion.test.ts @@ -0,0 +1,279 @@ +import type { ISlotType } from "@/interfaces" +import type { TWidgetType } from "@/types/widgets" + +import { describe, expect, it } from "vitest" + +import { LGraphNode, Subgraph } from "@/litegraph" +import { BaseWidget } from "@/widgets/BaseWidget" + +import { createEventCapture, createTestSubgraph, createTestSubgraphNode } from "./fixtures/subgraphHelpers" + +// Helper to create a node with a widget +function createNodeWithWidget( + title: string, + widgetType: TWidgetType = "number", + widgetValue: any = 42, + slotType: ISlotType = "number", +) { + const node = new LGraphNode(title) + const input = node.addInput("value", slotType) + node.addOutput("out", slotType) + + const widget = new BaseWidget({ + name: "widget", + type: widgetType, + value: widgetValue, + y: 0, + options: widgetType === "number" ? { min: 0, max: 100, step: 1 } : {}, + node, + }) + node.widgets = [widget] + input.widget = { name: widget.name } + + return { node, widget, input } +} + +// Helper to connect subgraph input to node and create SubgraphNode +function setupPromotedWidget(subgraph: Subgraph, node: LGraphNode, slotIndex = 0) { + subgraph.add(node) + subgraph.inputNode.slots[slotIndex].connect(node.inputs[slotIndex], node) + return createTestSubgraphNode(subgraph) +} + +describe("SubgraphWidgetPromotion", () => { + describe("Widget Promotion Functionality", () => { + it("should promote widgets when connecting node to subgraph input", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "value", type: "number" }], + }) + + const { node } = createNodeWithWidget("Test Node") + const subgraphNode = setupPromotedWidget(subgraph, node) + + // The widget should be promoted to the subgraph node + expect(subgraphNode.widgets).toHaveLength(1) + expect(subgraphNode.widgets[0].name).toBe("value") // Uses subgraph input name + expect(subgraphNode.widgets[0].type).toBe("number") + expect(subgraphNode.widgets[0].value).toBe(42) + expect(subgraphNode.widgets[0].parentSubgraphNode).toBe(subgraphNode) + }) + + it("should set parentSubgraphNode for all promoted widget types", () => { + const subgraph = createTestSubgraph({ + inputs: [ + { name: "numberInput", type: "number" }, + { name: "stringInput", type: "string" }, + { name: "toggleInput", type: "boolean" }, + ], + }) + + // Create nodes with different widget types + const { node: numberNode } = createNodeWithWidget("Number Node", "number", 100) + const { node: stringNode } = createNodeWithWidget("String Node", "string", "test", "string") + const { node: toggleNode } = createNodeWithWidget("Toggle Node", "toggle", true, "boolean") + + // Setup all nodes + subgraph.add(numberNode) + subgraph.add(stringNode) + subgraph.add(toggleNode) + + subgraph.inputNode.slots[0].connect(numberNode.inputs[0], numberNode) + subgraph.inputNode.slots[1].connect(stringNode.inputs[0], stringNode) + subgraph.inputNode.slots[2].connect(toggleNode.inputs[0], toggleNode) + + const subgraphNode = createTestSubgraphNode(subgraph) + + // All widgets should be promoted with parentSubgraphNode set + expect(subgraphNode.widgets).toHaveLength(3) + + for (const widget of subgraphNode.widgets) { + expect(widget.parentSubgraphNode).toBe(subgraphNode) + } + + // Check specific widget values + expect(subgraphNode.widgets[0].value).toBe(100) + expect(subgraphNode.widgets[1].value).toBe("test") + expect(subgraphNode.widgets[2].value).toBe(true) + }) + + it("should fire widget-promoted event when widget is promoted", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "input", type: "number" }], + }) + + const eventCapture = createEventCapture(subgraph.events, [ + "widget-promoted", + "widget-unpromoted", + ]) + + const { node } = createNodeWithWidget("Test Node") + const subgraphNode = setupPromotedWidget(subgraph, node) + + // Check event was fired + const promotedEvents = eventCapture.getEventsByType("widget-promoted") + expect(promotedEvents).toHaveLength(1) + expect(promotedEvents[0].detail.widget).toBeDefined() + expect(promotedEvents[0].detail.widget.parentSubgraphNode).toBe(subgraphNode) + expect(promotedEvents[0].detail.subgraphNode).toBe(subgraphNode) + + eventCapture.cleanup() + }) + + it("should fire widget-unpromoted event when removing promoted widget", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "input", type: "number" }], + }) + + const { node } = createNodeWithWidget("Test Node") + const subgraphNode = setupPromotedWidget(subgraph, node) + expect(subgraphNode.widgets).toHaveLength(1) + + const eventCapture = createEventCapture(subgraph.events, ["widget-unpromoted"]) + + // Remove the widget + subgraphNode.removeWidgetByName("input") + + // Check event was fired + const unpromotedEvents = eventCapture.getEventsByType("widget-unpromoted") + expect(unpromotedEvents).toHaveLength(1) + expect(unpromotedEvents[0].detail.widget).toBeDefined() + expect(unpromotedEvents[0].detail.subgraphNode).toBe(subgraphNode) + + // Widget should be removed + expect(subgraphNode.widgets).toHaveLength(0) + + eventCapture.cleanup() + }) + + it("should handle multiple widgets on same node", () => { + const subgraph = createTestSubgraph({ + inputs: [ + { name: "input1", type: "number" }, + { name: "input2", type: "string" }, + ], + }) + + // Create node with multiple widgets + const multiWidgetNode = new LGraphNode("Multi Widget Node") + const numInput = multiWidgetNode.addInput("num", "number") + const strInput = multiWidgetNode.addInput("str", "string") + + const widget1 = new BaseWidget({ + name: "widget1", + type: "number", + value: 10, + y: 0, + options: {}, + node: multiWidgetNode, + }) + + const widget2 = new BaseWidget({ + name: "widget2", + type: "string", + value: "hello", + y: 40, + options: {}, + node: multiWidgetNode, + }) + + multiWidgetNode.widgets = [widget1, widget2] + numInput.widget = { name: widget1.name } + strInput.widget = { name: widget2.name } + subgraph.add(multiWidgetNode) + + // Connect both inputs + subgraph.inputNode.slots[0].connect(multiWidgetNode.inputs[0], multiWidgetNode) + subgraph.inputNode.slots[1].connect(multiWidgetNode.inputs[1], multiWidgetNode) + + // Create SubgraphNode + const subgraphNode = createTestSubgraphNode(subgraph) + + // Both widgets should be promoted + expect(subgraphNode.widgets).toHaveLength(2) + expect(subgraphNode.widgets[0].name).toBe("input1") + expect(subgraphNode.widgets[0].value).toBe(10) + expect(subgraphNode.widgets[0].parentSubgraphNode).toBe(subgraphNode) + + expect(subgraphNode.widgets[1].name).toBe("input2") + expect(subgraphNode.widgets[1].value).toBe("hello") + expect(subgraphNode.widgets[1].parentSubgraphNode).toBe(subgraphNode) + }) + + it("should clean up parentSubgraphNode on node removal", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "input", type: "number" }], + }) + + const { node } = createNodeWithWidget("Test Node") + const subgraphNode = setupPromotedWidget(subgraph, node) + const promotedWidget = subgraphNode.widgets[0] + + expect(promotedWidget.parentSubgraphNode).toBe(subgraphNode) + + const eventCapture = createEventCapture(subgraph.events, ["widget-unpromoted"]) + + // Remove the subgraph node + subgraphNode.onRemoved() + + // parentSubgraphNode should be cleared + expect(promotedWidget.parentSubgraphNode).toBeUndefined() + + // Should fire unpromoted events for all widgets + const unpromotedEvents = eventCapture.getEventsByType("widget-unpromoted") + expect(unpromotedEvents).toHaveLength(1) + + eventCapture.cleanup() + }) + + it("should handle DOM widgets with parentSubgraphNode", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "domInput", type: "custom" }], + }) + + const { node, widget } = createNodeWithWidget("DOM Node", "custom", "custom value", "custom") + + // Make it a DOM widget + widget.element = document.createElement("div") + + const subgraphNode = setupPromotedWidget(subgraph, node) + + // DOM widget should be promoted with parentSubgraphNode + expect(subgraphNode.widgets).toHaveLength(1) + const promotedWidget = subgraphNode.widgets[0] + expect(promotedWidget.isDOMWidget()).toBe(true) + expect(promotedWidget.parentSubgraphNode).toBe(subgraphNode) + expect(promotedWidget.name).toBe("domInput") + }) + + it("should not promote widget if input is not connected", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "input", type: "number" }], + }) + + const { node } = createNodeWithWidget("Test Node") + subgraph.add(node) + + // Don't connect - just create SubgraphNode + const subgraphNode = createTestSubgraphNode(subgraph) + + // No widgets should be promoted + expect(subgraphNode.widgets).toHaveLength(0) + }) + + it("should handle disconnection of promoted widget", () => { + const subgraph = createTestSubgraph({ + inputs: [{ name: "input", type: "number" }], + }) + + const { node } = createNodeWithWidget("Test Node") + const subgraphNode = setupPromotedWidget(subgraph, node) + expect(subgraphNode.widgets).toHaveLength(1) + + // Disconnect the link + subgraph.inputNode.slots[0].disconnect() + + // Widget should be removed (through event listeners) + expect(subgraphNode.widgets).toHaveLength(0) + }) + }) +})