Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions browser_tests/assets/groups/nested-groups-1-inner-node.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
{
"id": "2ba0b800-2f13-4f21-b8d6-c6cdb0152cae",
"revision": 0,
"last_node_id": 17,
"last_link_id": 9,
"nodes": [
{
"id": 17,
"type": "VAEDecode",
"pos": [
318.8446183157076,
355.3961392345528
],
"size": [
225,
102
],
"flags": {},
"order": 0,
"mode": 0,
"inputs": [
{
"name": "samples",
"type": "LATENT",
"link": null
},
{
"name": "vae",
"type": "VAE",
"link": null
}
],
"outputs": [
{
"name": "IMAGE",
"type": "IMAGE",
"links": null
}
],
"properties": {
"Node name for S&R": "VAEDecode"
},
"widgets_values": []
}
],
"links": [],
"groups": [
{
"id": 4,
"title": "Outer Group",
"bounding": [
-46.25245366331014,
-150.82497138023245,
1034.4034361963616,
1007.338460439933
],
"color": "#3f789e",
"font_size": 24,
"flags": {}
},
{
"id": 3,
"title": "Inner Group",
"bounding": [
80.96059074101554,
28.123757436778178,
718.286373661183,
691.2397164539732
],
"color": "#3f789e",
"font_size": 24,
"flags": {}
}
],
"config": {},
"extra": {
"ds": {
"scale": 0.7121393732101533,
"offset": [
289.18242848011835,
367.0747755524199
]
},
"frontendVersion": "1.35.5",
"VHS_latentpreview": false,
"VHS_latentpreviewrate": 0,
"VHS_MetadataImage": true,
"VHS_KeepIntermediate": true,
"workflowRendererVersion": "Vue"
},
"version": 0.4
}
49 changes: 49 additions & 0 deletions browser_tests/fixtures/ComfyPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1653,6 +1653,55 @@ export class ComfyPage {
}, focusMode)
await this.nextFrame()
}

/**
* Get the position of a group by title.
* @param title The title of the group to find
* @returns The group's canvas position
* @throws Error if group not found
*/
async getGroupPosition(title: string): Promise<Position> {
const pos = await this.page.evaluate((title) => {
const groups = window['app'].graph.groups
const group = groups.find((g: { title: string }) => g.title === title)
if (!group) return null
return { x: group.pos[0], y: group.pos[1] }
}, title)
if (!pos) throw new Error(`Group "${title}" not found`)
return pos
}

/**
* Drag a group by its title.
* @param options.name The title of the group to drag
* @param options.deltaX Horizontal drag distance in screen pixels
* @param options.deltaY Vertical drag distance in screen pixels
*/
async dragGroup(options: {
name: string
deltaX: number
deltaY: number
}): Promise<void> {
const { name, deltaX, deltaY } = options
const screenPos = await this.page.evaluate((title) => {
const app = window['app']
const groups = app.graph.groups
const group = groups.find((g: { title: string }) => g.title === title)
if (!group) return null
// Position in the title area of the group
const clientPos = app.canvasPosToClientPos([
group.pos[0] + 50,
group.pos[1] + 15
])
return { x: clientPos[0], y: clientPos[1] }
}, name)
if (!screenPos) throw new Error(`Group "${name}" not found`)

await this.dragAndDrop(screenPos, {
x: screenPos.x + deltaX,
y: screenPos.y + deltaY
})
}
Comment on lines +1674 to +1704
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

LGTM! Consider parameter naming consistency.

The method correctly locates the group, converts coordinates, and performs the drag operation. The magic numbers for the title area offset are appropriately documented.

Minor optional suggestion: For consistency with getGroupPosition, consider using title instead of name for the group identifier parameter. Both methods refer to the same concept (the group's title property), so using the same terminology would improve API consistency.

🤖 Prompt for AI Agents
In browser_tests/fixtures/ComfyPage.ts around lines 1674 to 1704, rename the
method parameter and all internal uses from name to title for consistency with
getGroupPosition: update the JSDoc param, the options type ({ title: string ...
}), the destructuring const { title, deltaX, deltaY }, the page.evaluate
parameter and usage, and the thrown error message to reference title; ensure
callers are updated to pass title instead of name and run tests to confirm no
call sites break.

}

export const testComfySnapToGridGridSize = 50
Expand Down
38 changes: 38 additions & 0 deletions browser_tests/tests/vueNodes/groups/groups.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,42 @@ test.describe('Vue Node Groups', () => {
'vue-groups-fit-to-contents.png'
)
})

test('should move nested groups together when dragging outer group', async ({
comfyPage
}) => {
await comfyPage.loadWorkflow('groups/nested-groups-1-inner-node')

// Get initial positions with null guards
const outerInitial = await comfyPage.getGroupPosition('Outer Group')
const innerInitial = await comfyPage.getGroupPosition('Inner Group')

const initialOffsetX = innerInitial.x - outerInitial.x
const initialOffsetY = innerInitial.y - outerInitial.y

// Drag the outer group
const dragDelta = { x: 100, y: 80 }
await comfyPage.dragGroup({
name: 'Outer Group',
deltaX: dragDelta.x,
deltaY: dragDelta.y
})

// Use retrying assertion to wait for positions to update
await expect(async () => {
const outerFinal = await comfyPage.getGroupPosition('Outer Group')
const innerFinal = await comfyPage.getGroupPosition('Inner Group')

const finalOffsetX = innerFinal.x - outerFinal.x
const finalOffsetY = innerFinal.y - outerFinal.y

// Both groups should have moved
expect(outerFinal.x).not.toBe(outerInitial.x)
expect(innerFinal.x).not.toBe(innerInitial.x)

// The relative offset should be maintained (inner group moved with outer)
expect(finalOffsetX).toBeCloseTo(initialOffsetX, 0)
expect(finalOffsetY).toBeCloseTo(initialOffsetY, 0)
}).toPass({ timeout: 5000 })
Comment on lines +36 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add Y-axis movement assertions (currently only X is checked).

You drag by { x: 100, y: 80 } but only assert *.x changed (Line 64-67). Add y checks so a “no vertical move” regression can’t slip through.

@@
       // Both groups should have moved
       expect(outerFinal.x).not.toBe(outerInitial.x)
+      expect(outerFinal.y).not.toBe(outerInitial.y)
       expect(innerFinal.x).not.toBe(innerInitial.x)
+      expect(innerFinal.y).not.toBe(innerInitial.y)
🤖 Prompt for AI Agents
In browser_tests/tests/vueNodes/groups/groups.spec.ts around lines 36 to 71, the
test drags the outer group by { x: 100, y: 80 } but only asserts that the x
positions changed; add assertions that the y positions changed as well: after
computing outerFinal and innerFinal, add
expect(outerFinal.y).not.toBe(outerInitial.y) and
expect(innerFinal.y).not.toBe(innerInitial.y) so vertical movement regressions
are caught (keep the existing relative-offset assertions intact).

})
})
8 changes: 5 additions & 3 deletions src/lib/litegraph/src/LGraphCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8564,9 +8564,11 @@ export class LGraphCanvas implements CustomEventDispatcher<LGraphCanvasEventMap>
node,
newPos: this.calculateNewPosition(node, deltaX, deltaY)
})
} else {
// Non-node children (nested groups, reroutes)
child.move(deltaX, deltaY)
} else if (!(child instanceof LGraphGroup)) {
// Non-node, non-group children (reroutes, etc.)
// Skip groups here - they're already in allItems and will be
// processed in the main loop of moveChildNodesInGroupVueMode
child.move(deltaX, deltaY, true)
}
}
}
Expand Down
Loading