mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-03 14:54:37 +00:00
[feat] Add containerNode property for DOM widget positioning in subgraphs (#1128)
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
36
CLAUDE.md
36
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 }
|
||||
}
|
||||
```
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -187,8 +187,13 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
}
|
||||
|
||||
#setWidget(subgraphInput: Readonly<SubgraphInput>, input: INodeInputSlot, widget: Readonly<IBaseWidget>) {
|
||||
// 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()
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -56,6 +56,12 @@ export abstract class BaseWidget<TWidget extends IBaseWidget = IBaseWidget> 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<TWidget extends IBaseWidget = IBaseWidget> 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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
|
||||
279
test/subgraph/SubgraphWidgetPromotion.test.ts
Normal file
279
test/subgraph/SubgraphWidgetPromotion.test.ts
Normal file
@@ -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)
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user