From 585d46d4fbd9b394f1caa30f4256999e25839850 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sun, 14 Dec 2025 18:37:29 -0800 Subject: [PATCH] fix: inner groups being moved double when moving outer group (in vue mode) (#7447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes issue when dragging a group that had inner groups when in vue mode. When dragging the outer group in Vue mode: 1. getAllNestedItems(selected) returns ALL items: outer group + inner groups + nodes 2. moveChildNodesInGroupVueMode loops through all items 3. For outer group G1: calls G1.move(delta, true) then moveGroupChildren(G1, ...) 4. moveGroupChildren calls G2.move(delta) (no skipChildren) - this moves G2 AND G2's children! 5. Then the loop reaches G2: calls G2.move(delta, true) - moves G2 again 6. Plus moveGroupChildren(G2, ...) processes G2's children again This PR fixes it by adding `skipChildren=true` to the `move` call. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7447-fix-inner-groups-being-moved-double-when-moving-outer-group-in-vue-mode-2c86d73d365081ce97abec682f2a8518) by [Unito](https://www.unito.io) --- .../groups/nested-groups-1-inner-node.json | 92 +++++++++++++++++++ browser_tests/fixtures/ComfyPage.ts | 49 ++++++++++ .../tests/vueNodes/groups/groups.spec.ts | 38 ++++++++ src/lib/litegraph/src/LGraphCanvas.ts | 8 +- 4 files changed, 184 insertions(+), 3 deletions(-) create mode 100644 browser_tests/assets/groups/nested-groups-1-inner-node.json diff --git a/browser_tests/assets/groups/nested-groups-1-inner-node.json b/browser_tests/assets/groups/nested-groups-1-inner-node.json new file mode 100644 index 000000000..3a2a1c111 --- /dev/null +++ b/browser_tests/assets/groups/nested-groups-1-inner-node.json @@ -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 +} \ No newline at end of file diff --git a/browser_tests/fixtures/ComfyPage.ts b/browser_tests/fixtures/ComfyPage.ts index 782af9921..2d8ece5d2 100644 --- a/browser_tests/fixtures/ComfyPage.ts +++ b/browser_tests/fixtures/ComfyPage.ts @@ -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 { + 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 { + 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 + }) + } } export const testComfySnapToGridGridSize = 50 diff --git a/browser_tests/tests/vueNodes/groups/groups.spec.ts b/browser_tests/tests/vueNodes/groups/groups.spec.ts index fcfb0efad..c12c1772b 100644 --- a/browser_tests/tests/vueNodes/groups/groups.spec.ts +++ b/browser_tests/tests/vueNodes/groups/groups.spec.ts @@ -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 }) + }) }) diff --git a/src/lib/litegraph/src/LGraphCanvas.ts b/src/lib/litegraph/src/LGraphCanvas.ts index c1c726d9e..246bcf50f 100644 --- a/src/lib/litegraph/src/LGraphCanvas.ts +++ b/src/lib/litegraph/src/LGraphCanvas.ts @@ -8564,9 +8564,11 @@ export class LGraphCanvas implements CustomEventDispatcher 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) } } }