From b7251ca624dcd19eec473485e562ea6934f98a26 Mon Sep 17 00:00:00 2001
From: Guilhem Lemouel <guilhemlemouel@gmail.com>
Date: Thu, 12 Mar 2026 16:59:15 +0100
Subject: [PATCH] refactor: replace group module_ids with start_id/end_id,
 compute members dynamically

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 .../flows/map/FlowModuleSchemaMap.svelte      |   9 --
 .../lib/components/graph/FlowGraphV2.svelte   |  71 +++++++--
 .../components/graph/GroupActionBar.svelte    |  18 ++-
 .../lib/components/graph/GroupOverlay.svelte  |  19 +--
 .../components/graph/NodeContextMenu.svelte   |  14 +-
 .../graph/SelectionBoundingBox.svelte         |  15 +-
 .../components/graph/graphBuilder.svelte.ts   |  13 +-
 .../src/lib/components/graph/graphContext.ts  |   7 +
 .../components/graph/groupDetectionUtils.ts   | 138 +++++++++++++++-
 .../components/graph/groupEditor.svelte.ts    | 148 ++++++------------
 frontend/src/lib/gen/types.gen.ts             |   8 +-
 openflow.openapi.yaml                         |  16 +-
 12 files changed, 322 insertions(+), 154 deletions(-)

diff --git a/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte b/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte
index 2053017639..43cc6790e8 100644
--- a/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte
+++ b/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte
@@ -632,11 +632,6 @@
 								}
 								targetModules.splice(insertIndex, 0, ...removedModules)
 								selectionManager.selectByIds(removedModules.map((m) => m.id))
-								for (const m of removedModules) {
-									groupEditorContext?.groupEditor.handleNodeMoved(
-										m.id, detail.sourceId, detail.targetId
-									)
-								}
 							} else {
 								let indexToRemove = originalModules.findIndex((m) => moveManager.movingModuleId == m.id)
 								let [removedModule] = originalModules.splice(indexToRemove, 1)
@@ -647,9 +642,6 @@
 								}
 								targetModules.splice(insertIndex, 0, removedModule)
 								selectionManager.selectId(removedModule.id)
-								groupEditorContext?.groupEditor.handleNodeMoved(
-									removedModule.id, detail.sourceId, detail.targetId
-								)
 							}
 							moveManager.clearMoving()
 						} else {
@@ -687,7 +679,6 @@
 									toolKind
 								)
 								const id = targetModules[index].id
-								groupEditorContext?.groupEditor.addInsertedNode(id, detail.sourceId, detail.targetId)
 								selectionManager.selectId(id)
 
 								if (detail.inlineScript?.instructions) {
diff --git a/frontend/src/lib/components/graph/FlowGraphV2.svelte b/frontend/src/lib/components/graph/FlowGraphV2.svelte
index 9978bcd40b..262da4665d 100644
--- a/frontend/src/lib/components/graph/FlowGraphV2.svelte
+++ b/frontend/src/lib/components/graph/FlowGraphV2.svelte
@@ -66,8 +66,10 @@
 	import {
 		getGroupEditorContext,
 		GROUP_HEADER_HEIGHT,
-		GROUP_TOP_MARGIN
+		GROUP_TOP_MARGIN,
+		type FlowGroup
 	} from './groupEditor.svelte'
+	import { computeGroupMembers, type GroupMembership } from './groupDetectionUtils'
 	import SelectionTool from './SelectionTool.svelte'
 	import PaneContextMenu from './PaneContextMenu.svelte'
 	import { SelectionManager } from './selectionUtils.svelte'
@@ -329,7 +331,10 @@
 		moveManager: untrack(() => moveManager),
 		clearFlowSelection,
 		yOffset,
-		diffManager
+		diffManager,
+		getFlowNodes: () => currentGraphNodeDeps,
+		getContainerDescendants: () => currentContainerDescendants,
+		getGroupMemberships: () => currentGroupMemberships
 	} as any)
 
 	if (triggerContext && untrack(() => allowSimplifiedPoll)) {
@@ -370,6 +375,33 @@
 		| [NodeDep[], Map<string, { top: number; bottom: number }> | undefined, (NodeDep & NodePos)[]]
 		| undefined = undefined
 	let currentContainerDescendants: Map<string, string[]> = $state(new Map())
+	let currentGraphNodeDeps: { id: string; parentIds?: string[] }[] = $state([])
+
+	/** Compute group memberships from start_id/end_id for a given set of flow nodes.
+	 *  Collapsed groups preserve their previous membership (their member nodes
+	 *  aren't in the graph — they've been replaced by a placeholder). */
+	function computeAllGroupMemberships(
+		groups: FlowGroup[],
+		flowNodes: { id: string; parentIds?: string[] }[],
+		contDescendants: Map<string, string[]>
+	): Map<string, GroupMembership> {
+		const map = new Map<string, GroupMembership>()
+		for (const group of groups) {
+			if (groupEditorContext?.groupEditor.isRuntimeCollapsed(group.id)) {
+				const prev = currentGroupMemberships.get(group.id)
+				if (prev) map.set(group.id, prev)
+				continue
+			}
+			map.set(
+				group.id,
+				computeGroupMembers(group.start_id, group.end_id, flowNodes, contDescendants)
+			)
+		}
+		return map
+	}
+
+	// Current group memberships — recomputed when groups, nodes, or containerDescendants change
+	let currentGroupMemberships: Map<string, GroupMembership> = $state(new Map())
 
 	const MAX_TOOLS_PER_ROW = 2
 
@@ -458,7 +490,7 @@
 			// Need to find topmost node per group - use topological sort
 			const sortedNodes = topologicalSort(graphNodes).reverse()
 			for (const group of groups) {
-				if (group.module_ids.length === 0) continue
+				const memberIds = currentGroupMemberships.get(group.id)?.memberIds ?? []
 				// Check if it's collapsed
 				const collapsedNodeId = `collapsed-group:${group.id}`
 				const hasCollapsedNode = graphNodes.some((n) => n.id === collapsedNodeId)
@@ -468,8 +500,8 @@
 				if (hasCollapsedNode) {
 					topmostNodeId = collapsedNodeId
 					isCollapsed = true
-				} else {
-					topmostNodeId = sortedNodes.find((node) => group.module_ids.includes(node.id))?.id
+				} else if (memberIds.length > 0) {
+					topmostNodeId = sortedNodes.find((node) => memberIds.includes(node.id))?.id
 				}
 
 				if (topmostNodeId) {
@@ -490,7 +522,7 @@
 					if (!isCollapsed) {
 						const bottommostNodeId = [...sortedNodes]
 							.reverse()
-							.find((node) => group.module_ids.includes(node.id))?.id
+							.find((node) => memberIds.includes(node.id))?.id
 						if (bottommostNodeId) {
 							const prevBottom = extraSpace.get(bottommostNodeId) ?? {
 								top: 0,
@@ -789,12 +821,20 @@
 			parentIds: n.parentIds,
 			data: { assets: (n.data as any).assets, module: (n.data as any).module }
 		}))
+		currentGraphNodeDeps = graphNodeDeps
 
-		// Clean up groups: remove stale IDs, complete paths, split disconnected components
+		// Clean up groups: check start_id/end_id still exist
 		if (editMode && groupEditorContext?.groupEditor?.isAvailable()) {
 			groupEditorContext.groupEditor.cleanupGroups(graphNodeDeps)
 		}
 
+		// Compute group memberships from start_id/end_id
+		currentGroupMemberships = computeAllGroupMemberships(
+			groupEditorContext?.groupEditor.getGroups() ?? [],
+			graphNodeDeps,
+			currentContainerDescendants
+		)
+
 		// Pre-compute extra space per node for assets, AI tools, group notes, group headers
 		const nodeExtraSpace = computeNodeExtraSpace(graphNodeDeps)
 
@@ -855,7 +895,7 @@
 				: {}
 			const sortedNodes = topologicalSort(graphNodeDeps).reverse()
 			for (const group of groups) {
-				if (group.module_ids.length === 0) continue
+				const memberIds = currentGroupMemberships.get(group.id)?.memberIds ?? []
 				const collapsedNodeId = `collapsed-group:${group.id}`
 				const hasCollapsedNode = graphNodeDeps.some((n) => n.id === collapsedNodeId)
 				let topNodeId: string | undefined
@@ -864,8 +904,8 @@
 				if (hasCollapsedNode) {
 					topNodeId = collapsedNodeId
 					isCollapsed = true
-				} else {
-					topNodeId = sortedNodes.find((node) => group.module_ids.includes(node.id))?.id
+				} else if (memberIds.length > 0) {
+					topNodeId = sortedNodes.find((node) => memberIds.includes(node.id))?.id
 				}
 
 				if (topNodeId) {
@@ -880,7 +920,7 @@
 				if (!isCollapsed) {
 					const bottomNodeId = [...sortedNodes]
 						.reverse()
-						.find((node) => group.module_ids.includes(node.id))?.id
+						.find((node) => memberIds.includes(node.id))?.id
 					if (bottomNodeId) {
 						const groupPadding = 16
 						groupBottomOffsets[bottomNodeId] = Math.max(
@@ -995,7 +1035,12 @@
 		effectiveModuleActions
 		currentGroups
 
-		const collapsedGroups = groupEditorContext?.groupEditor.getCollapsedGroups() ?? []
+		const collapsedGroups = (groupEditorContext?.groupEditor.getCollapsedGroups() ?? []).map(
+			(g) => ({
+				...g,
+				memberIds: untrack(() => currentGroupMemberships.get(g.id)?.memberIds ?? [])
+			})
+		)
 
 		return graphBuilder(
 			untrack(() => effectiveModules),
@@ -1298,7 +1343,7 @@
 					allNodes={nodesWithOffset as (Node & { type: string })[]}
 					{editMode}
 					{showNotes}
-					containerDescendants={currentContainerDescendants}
+					groupMemberships={currentGroupMemberships}
 				/>
 
 				<!-- SelectionTool for handling selection changes and filtering -->
diff --git a/frontend/src/lib/components/graph/GroupActionBar.svelte b/frontend/src/lib/components/graph/GroupActionBar.svelte
index ac571ccb98..ef540beb8e 100644
--- a/frontend/src/lib/components/graph/GroupActionBar.svelte
+++ b/frontend/src/lib/components/graph/GroupActionBar.svelte
@@ -64,7 +64,9 @@
 			</button>
 		{/snippet}
 		{#snippet menu()}
-			<div class="bg-surface-tertiary dark:border w-56 origin-top-right rounded-lg shadow-lg focus:outline-none py-1">
+			<div
+				class="bg-surface-tertiary dark:border w-56 origin-top-right rounded-lg shadow-lg focus:outline-none py-1"
+			>
 				<!-- Color picker -->
 				<div class="px-4 py-2">
 					<div class="grid grid-cols-5 gap-1">
@@ -95,10 +97,15 @@
 				<!-- Add / Remove note -->
 				<button
 					class="px-4 py-2 text-primary font-normal hover:bg-surface-hover cursor-pointer text-xs w-full flex flex-row gap-2 items-center rounded-sm"
-					onclick={() => { note == null ? onAddNote() : onRemoveNote(); menuOpen = false }}
+					onclick={() => {
+						note == null ? onAddNote() : onRemoveNote()
+						menuOpen = false
+					}}
 				>
 					<StickyNote size={14} class="shrink-0" />
-					<p class="truncate grow min-w-0 whitespace-nowrap text-left">{note == null ? 'Add note' : 'Remove note'}</p>
+					<p class="truncate grow min-w-0 whitespace-nowrap text-left"
+						>{note == null ? 'Add note' : 'Remove note'}</p
+					>
 				</button>
 
 				{#if onDeleteGroup}
@@ -107,7 +114,10 @@
 					<!-- Ungroup -->
 					<button
 						class="px-4 py-2 font-normal hover:bg-red-500/10 cursor-pointer text-xs w-full flex flex-row gap-2 items-center rounded-sm text-red-600 dark:text-red-400"
-						onclick={() => { onDeleteGroup?.(); menuOpen = false }}
+						onclick={() => {
+							onDeleteGroup?.()
+							menuOpen = false
+						}}
 					>
 						<Ungroup size={14} class="shrink-0" />
 						<p class="truncate grow min-w-0 whitespace-nowrap text-left">Ungroup</p>
diff --git a/frontend/src/lib/components/graph/GroupOverlay.svelte b/frontend/src/lib/components/graph/GroupOverlay.svelte
index 7955307c09..90a9be5f99 100644
--- a/frontend/src/lib/components/graph/GroupOverlay.svelte
+++ b/frontend/src/lib/components/graph/GroupOverlay.svelte
@@ -1,7 +1,8 @@
 <script lang="ts">
 	import { ViewportPortal, type Node } from '@xyflow/svelte'
-	import { calculateNodesBoundsWithOffset, expandWithContainerDescendants } from './util'
+	import { calculateNodesBoundsWithOffset } from './util'
 	import { getGroupEditorContext, GROUP_HEADER_HEIGHT, GROUP_TOP_MARGIN, type FlowGroup } from './groupEditor.svelte'
+	import type { GroupMembership } from './groupDetectionUtils'
 	import { NoteColor, NOTE_COLORS } from './noteColors'
 	import GroupActionBar from './GroupActionBar.svelte'
 	import GroupHeader from './GroupHeader.svelte'
@@ -13,10 +14,10 @@
 		allNodes: (Node & { type: string })[]
 		editMode: boolean
 		showNotes: boolean
-		containerDescendants: Map<string, string[]>
+		groupMemberships: Map<string, GroupMembership>
 	}
 
-	let { hoveredNodeId, allNodes, editMode, showNotes, containerDescendants }: Props = $props()
+	let { hoveredNodeId, allNodes, editMode, showNotes, groupMemberships }: Props = $props()
 
 	const groupEditorContext = getGroupEditorContext()
 
@@ -39,7 +40,7 @@
 			return allGroups.find((g) => g.id === headerHoveredGroupId)
 		}
 		if (!hoveredNodeId || !groupEditorContext?.groupEditor) return undefined
-		return groupEditorContext.groupEditor.getClosestGroup(hoveredNodeId)
+		return groupEditorContext.groupEditor.getClosestGroup(hoveredNodeId, groupMemberships)
 	})
 
 	// Manage visible group with delay to prevent flickering
@@ -71,7 +72,8 @@
 	let groupBoundsMap = $derived.by(() => {
 		const map: Record<string, { x: number; y: number; width: number; height: number; headerY: number } | null> = {}
 		for (const group of allGroups) {
-			if (group.module_ids.length === 0) {
+			const memberIds = groupMemberships.get(group.id)?.memberIds ?? []
+			if (memberIds.length === 0) {
 				map[group.id] = null
 				continue
 			}
@@ -95,9 +97,8 @@
 					headerY: node.position.y - headerTotal
 				}
 			} else {
-				// Expanded group bounds — expand module_ids to include container descendants
-				const expandedIds = expandWithContainerDescendants(group.module_ids, containerDescendants)
-				const { minX, minY, maxX, maxY } = calculateNodesBoundsWithOffset(expandedIds, allNodes)
+				// Expanded group bounds — memberIds already include container descendants
+				const { minX, minY, maxX, maxY } = calculateNodesBoundsWithOffset(memberIds, allNodes)
 				const padding = 16
 				const noteHeight = getGroupNoteHeight(group.id)
 				const topPadding = GROUP_HEADER_HEIGHT + noteHeight + GROUP_TOP_MARGIN
@@ -105,7 +106,7 @@
 				// Find the topmost node's center x to symmetrize the bounding box
 				let topNodeCenterX = (minX + maxX) / 2
 				let topNodeY = Infinity
-				for (const id of group.module_ids) {
+				for (const id of memberIds) {
 					const node = allNodes.find((n) => n.id === id)
 					if (node && node.position.y < topNodeY) {
 						topNodeY = node.position.y
diff --git a/frontend/src/lib/components/graph/NodeContextMenu.svelte b/frontend/src/lib/components/graph/NodeContextMenu.svelte
index a811c7d2d3..5c2fb2d90f 100644
--- a/frontend/src/lib/components/graph/NodeContextMenu.svelte
+++ b/frontend/src/lib/components/graph/NodeContextMenu.svelte
@@ -4,6 +4,7 @@
 	import type { Snippet } from 'svelte'
 	import { getNoteEditorContext } from './noteEditor.svelte'
 	import { getGroupEditorContext } from './groupEditor.svelte'
+	import { canFormValidGroup } from './groupDetectionUtils'
 	import { getGraphContext } from './graphContext'
 	import { tick } from 'svelte'
 
@@ -21,15 +22,24 @@
 	// Get Graph context for clearFlowSelection function
 	const graphContext = getGraphContext()
 
+	let canCreateGroup = $derived.by(() => {
+		if (selectedNodeIds.length < 1 || !groupEditorContext?.groupEditor || !graphContext) return false
+		const flowNodes = graphContext.getFlowNodes?.() ?? []
+		const contDesc = graphContext.getContainerDescendants?.() ?? new Map()
+		return canFormValidGroup(selectedNodeIds, flowNodes, contDesc).valid
+	})
+
 	const menuItems: ContextMenuItem[] = $derived([
 		{
 			id: 'create-group',
 			label: `Create group (${selectedNodeIds.length} nodes)`,
 			icon: Group,
-			disabled: selectedNodeIds.length === 0 || !groupEditorContext?.groupEditor,
+			disabled: selectedNodeIds.length === 0 || !groupEditorContext?.groupEditor || !canCreateGroup,
 			onClick: () => {
 				if (selectedNodeIds.length > 0 && groupEditorContext?.groupEditor && graphContext) {
-					groupEditorContext.groupEditor.createGroup(selectedNodeIds)
+					const flowNodes = graphContext.getFlowNodes?.() ?? []
+					const contDesc = graphContext.getContainerDescendants?.() ?? new Map()
+					groupEditorContext.groupEditor.createGroup(selectedNodeIds, flowNodes, contDesc)
 
 					tick().then(() => {
 						graphContext?.clearFlowSelection?.()
diff --git a/frontend/src/lib/components/graph/SelectionBoundingBox.svelte b/frontend/src/lib/components/graph/SelectionBoundingBox.svelte
index 666b0d117a..c39697b3fc 100644
--- a/frontend/src/lib/components/graph/SelectionBoundingBox.svelte
+++ b/frontend/src/lib/components/graph/SelectionBoundingBox.svelte
@@ -6,6 +6,7 @@
 	import DropdownV2 from '../DropdownV2.svelte'
 	import { getNoteEditorContext } from './noteEditor.svelte'
 	import { getGroupEditorContext } from './groupEditor.svelte'
+	import { canFormValidGroup } from './groupDetectionUtils'
 	import { getGraphContext } from './graphContext'
 	import MoveHandleButton from './MoveHandleButton.svelte'
 	import { tick } from 'svelte'
@@ -56,9 +57,18 @@
 		}
 	}
 
+	let canCreateGroup = $derived.by(() => {
+		if (selectedNodes.length < 1 || !groupEditorContext?.groupEditor || !graphContext) return false
+		const flowNodes = graphContext.getFlowNodes?.() ?? []
+		const contDesc = graphContext.getContainerDescendants?.() ?? new Map()
+		return canFormValidGroup(selectedNodes, flowNodes, contDesc).valid
+	})
+
 	function handleAddGroup() {
 		if (selectedNodes.length > 0 && groupEditorContext?.groupEditor && graphContext) {
-			groupEditorContext.groupEditor.createGroup(selectedNodes)
+			const flowNodes = graphContext.getFlowNodes?.() ?? []
+			const contDesc = graphContext.getContainerDescendants?.() ?? new Map()
+			groupEditorContext.groupEditor.createGroup(selectedNodes, flowNodes, contDesc)
 
 			tick().then(() => {
 				graphContext?.clearFlowSelection?.()
@@ -101,7 +111,8 @@
 					{
 						displayName: 'Create group',
 						icon: Group,
-						action: handleAddGroup
+						action: handleAddGroup,
+						disabled: !canCreateGroup
 					}
 				]
 			: [])
diff --git a/frontend/src/lib/components/graph/graphBuilder.svelte.ts b/frontend/src/lib/components/graph/graphBuilder.svelte.ts
index 1a0d52a0d4..74f7391357 100644
--- a/frontend/src/lib/components/graph/graphBuilder.svelte.ts
+++ b/frontend/src/lib/components/graph/graphBuilder.svelte.ts
@@ -347,7 +347,8 @@ export function topologicalSort(
 		if (visited.has(id)) return
 		visited.add(id)
 
-		const node = nodeMap.get(id)!
+		const node = nodeMap.get(id)
+		if (!node) return
 		node.parentIds?.forEach(visit)
 		result.push(node)
 	}
@@ -410,7 +411,9 @@ export function graphBuilder(
 		note?: string
 		color?: string
 		collapsed?: boolean
-		module_ids: string[]
+		start_id: string
+		end_id: string
+		memberIds: string[]
 	}>,
 	collapsedContainers: Set<string>,
 	showNotes: boolean
@@ -433,7 +436,7 @@ export function graphBuilder(
 		// Build a map: module_id -> collapsed group (only for collapsed groups)
 		const moduleToCollapsedGroup = new Map<string, (typeof collapsedGroups)[number]>()
 		for (const group of collapsedGroups) {
-			for (const moduleId of group.module_ids) {
+			for (const moduleId of group.memberIds) {
 				moduleToCollapsedGroup.set(moduleId, group)
 			}
 		}
@@ -696,8 +699,8 @@ export function graphBuilder(
 								summary: collapsedGroup.summary,
 								note: collapsedGroup.note,
 								color: collapsedGroup.color,
-								stepCount: collapsedGroup.module_ids.length,
-								modules: collapsedGroup.module_ids
+								stepCount: collapsedGroup.memberIds.length,
+								modules: collapsedGroup.memberIds
 									.map((id) => getAllModules(modules).find((m) => m.id === id))
 									.filter((m): m is FlowModule => m != null),
 								showNotes,
diff --git a/frontend/src/lib/components/graph/graphContext.ts b/frontend/src/lib/components/graph/graphContext.ts
index 8540982366..a219d30f07 100644
--- a/frontend/src/lib/components/graph/graphContext.ts
+++ b/frontend/src/lib/components/graph/graphContext.ts
@@ -4,6 +4,7 @@ import type { NoteManager } from './noteManager.svelte'
 import type { MoveManager } from './moveManager.svelte'
 import type { Writable } from 'svelte/store'
 import type { FlowDiffManager } from '../flows/flowDiffManager.svelte'
+import type { GroupMembership } from './groupDetectionUtils'
 
 export type GraphContext = {
 	selectionManager: SelectionManager
@@ -14,6 +15,12 @@ export type GraphContext = {
 	clearFlowSelection?: () => void
 	yOffset?: number
 	diffManager: FlowDiffManager
+	/** Current flow nodes for group validation (set by FlowGraphV2) */
+	getFlowNodes?: () => { id: string; parentIds?: string[] }[]
+	/** Current container descendants map (set by FlowGraphV2) */
+	getContainerDescendants?: () => Map<string, string[]>
+	/** Current group memberships (set by FlowGraphV2) */
+	getGroupMemberships?: () => Map<string, GroupMembership>
 }
 
 const graphContextKey = 'FlowGraphContext'
diff --git a/frontend/src/lib/components/graph/groupDetectionUtils.ts b/frontend/src/lib/components/graph/groupDetectionUtils.ts
index e2c81c4d98..2639e41c18 100644
--- a/frontend/src/lib/components/graph/groupDetectionUtils.ts
+++ b/frontend/src/lib/components/graph/groupDetectionUtils.ts
@@ -1,7 +1,143 @@
+import { expandWithContainerDescendants } from './util'
+
 type FlowNode = { id: string; parentIds?: string[] }
 
+export type GroupMembership = {
+	memberIds: string[]
+	valid: boolean
+	error?: string
+}
+
 /**
- * Use a simple algorithm to complete a group and split it into connected components
+ * Compute the set of nodes that belong to a group defined by start_id and end_id.
+ *
+ * Algorithm:
+ * 1. Build a childrenMap from parentIds
+ * 2. Walk forward from start_id collecting all reachable nodes (stop at end_id, don't go past)
+ * 3. Expand with containerDescendants
+ */
+export function computeGroupMembers(
+	startId: string,
+	endId: string,
+	flowNodes: FlowNode[],
+	containerDescendants: Map<string, string[]>
+): GroupMembership {
+	const nodeMap = new Map<string, FlowNode>()
+	for (const n of flowNodes) nodeMap.set(n.id, n)
+
+	if (!nodeMap.has(startId) || !nodeMap.has(endId)) {
+		return { memberIds: [], valid: false, error: 'Start or end node not found' }
+	}
+
+	// Single node group
+	if (startId === endId) {
+		const members = expandWithContainerDescendants([startId], containerDescendants)
+		return { memberIds: members, valid: true }
+	}
+
+	// Build children map (node → children that list it as parent)
+	const childrenMap = new Map<string, string[]>()
+	for (const node of flowNodes) {
+		for (const pid of node.parentIds ?? []) {
+			const children = childrenMap.get(pid)
+			if (children) {
+				children.push(node.id)
+			} else {
+				childrenMap.set(pid, [node.id])
+			}
+		}
+	}
+
+	// Forward walk from start_id: collect all nodes reachable without going past end_id
+	const reachable = new Set<string>()
+	const queue = [startId]
+	reachable.add(startId)
+	let qi = 0
+	while (qi < queue.length) {
+		const current = queue[qi++]
+		if (current === endId) continue // don't go past end
+		for (const child of childrenMap.get(current) ?? []) {
+			if (!reachable.has(child)) {
+				reachable.add(child)
+				queue.push(child)
+			}
+		}
+	}
+
+	if (!reachable.has(endId)) {
+		return { memberIds: [], valid: false, error: 'End node is not reachable from start node' }
+	}
+
+	// Expand with container descendants
+	const members = expandWithContainerDescendants(Array.from(reachable), containerDescendants)
+	return { memberIds: members, valid: true }
+}
+
+/**
+ * Check whether a set of selected node IDs can form a valid group.
+ * Uses topological ordering to find start (no in-group parents) and end (no in-group children).
+ */
+export function canFormValidGroup(
+	selectedIds: string[],
+	flowNodes: FlowNode[],
+	containerDescendants: Map<string, string[]>
+): { valid: true; startId: string; endId: string } | { valid: false } {
+	if (selectedIds.length === 0) return { valid: false }
+
+	const selectedSet = new Set(selectedIds)
+	const nodeMap = new Map<string, FlowNode>()
+	for (const n of flowNodes) nodeMap.set(n.id, n)
+
+	// Build children map
+	const childrenMap = new Map<string, string[]>()
+	for (const node of flowNodes) {
+		for (const pid of node.parentIds ?? []) {
+			const children = childrenMap.get(pid)
+			if (children) {
+				children.push(node.id)
+			} else {
+				childrenMap.set(pid, [node.id])
+			}
+		}
+	}
+
+	// Find entries (no in-selection parents) and exits (no in-selection children)
+	const entries: string[] = []
+	const exits: string[] = []
+
+	for (const id of selectedIds) {
+		const node = nodeMap.get(id)
+		if (!node) return { valid: false }
+
+		const hasInternalParent = (node.parentIds ?? []).some((pid) => selectedSet.has(pid))
+		if (!hasInternalParent) entries.push(id)
+
+		const hasInternalChild = (childrenMap.get(id) ?? []).some((cid) => selectedSet.has(cid))
+		if (!hasInternalChild) exits.push(id)
+	}
+
+	// Valid group needs exactly one entry and one exit
+	if (entries.length !== 1 || exits.length !== 1) return { valid: false }
+
+	const startId = entries[0]
+	const endId = exits[0]
+
+	// Validate that the computed members match the selection
+	const membership = computeGroupMembers(startId, endId, flowNodes, containerDescendants)
+	if (!membership.valid) return { valid: false }
+
+	// Check that all selected ids are in the computed members
+	const memberSet = new Set(membership.memberIds)
+	for (const id of selectedIds) {
+		if (!memberSet.has(id)) return { valid: false }
+	}
+
+	return { valid: true, startId, endId }
+}
+
+/**
+ * Legacy utility: complete a group and split it into connected components.
+ * Still used by NoteEditor for FlowNote group notes (contained_node_ids).
  */
 export function completeAndSplitGroup(groupNodes: string[], flowNodes: FlowNode[]): string[][] {
 	if (groupNodes.length <= 1) {
diff --git a/frontend/src/lib/components/graph/groupEditor.svelte.ts b/frontend/src/lib/components/graph/groupEditor.svelte.ts
index e516c12e35..4bdce73d10 100644
--- a/frontend/src/lib/components/graph/groupEditor.svelte.ts
+++ b/frontend/src/lib/components/graph/groupEditor.svelte.ts
@@ -1,20 +1,22 @@
 import type { StateStore } from '$lib/utils'
 import type { ExtendedOpenFlow } from '../flows/types'
-import { completeAndSplitGroup } from './groupDetectionUtils'
+import { canFormValidGroup, type GroupMembership } from './groupDetectionUtils'
 import type { NoteColor } from './noteColors'
 import { DEFAULT_GROUP_NOTE_COLOR, getNextAvailableColor } from './noteColors'
 import { generateId } from './util'
 import { getContext, setContext } from 'svelte'
 
 /**
- * Type for a flow group (matches the generated type from OpenAPI)
+ * Type for a flow group (matches the generated type from OpenAPI).
+ * Members are computed dynamically from all nodes on paths between start_id and end_id.
  */
 export type FlowGroup = {
 	id: string
 	summary?: string
 	note?: string
 	collapsed_by_default?: boolean
-	module_ids: Array<string>
+	start_id: string
+	end_id: string
 	color?: string
 }
 
@@ -102,10 +104,15 @@ export class GroupEditor {
 	}
 
 	/**
-	 * Create a new group containing the specified module IDs.
+	 * Create a new group from selected node IDs.
+	 * Uses canFormValidGroup to determine start_id and end_id.
 	 * Returns the generated group ID.
 	 */
-	createGroup(moduleIds: string[]): string {
+	createGroup(
+		moduleIds: string[],
+		flowNodes: { id: string; parentIds?: string[] }[],
+		containerDescendants: Map<string, string[]>
+	): string | undefined {
 		// Filter subflow node IDs (same logic as NoteEditor.createGroupNote)
 		let filteredIds = [...moduleIds]
 		const subflowIds: string[] = []
@@ -122,6 +129,9 @@ export class GroupEditor {
 			filteredIds = [...filteredIds, ...subflowIds]
 		}
 
+		const result = canFormValidGroup(filteredIds, flowNodes, containerDescendants)
+		if (!result.valid) return undefined
+
 		const groups = this.getGroups()
 		const usedColors = new Set<NoteColor>()
 		for (const group of groups) {
@@ -133,7 +143,8 @@ export class GroupEditor {
 
 		const newGroup: FlowGroup = {
 			id: generateId(),
-			module_ids: filteredIds,
+			start_id: result.startId,
+			end_id: result.endId,
 			color
 		}
 		this.setGroups([...groups, newGroup])
@@ -177,10 +188,14 @@ export class GroupEditor {
 	}
 
 	/**
-	 * Returns the smallest group (by module_ids.length) that contains the given module ID.
+	 * Returns the smallest group (by member count) that contains the given module ID.
 	 * Also matches collapsed group node IDs (collapsed-group:{groupId}).
+	 * Requires pre-computed membership map.
 	 */
-	getClosestGroup(moduleId: string): FlowGroup | undefined {
+	getClosestGroup(
+		moduleId: string,
+		groupMemberships: Map<string, GroupMembership>
+	): FlowGroup | undefined {
 		const groups = this.getGroups()
 
 		// Check if this is a collapsed group node ID
@@ -190,10 +205,13 @@ export class GroupEditor {
 		}
 
 		let closest: FlowGroup | undefined = undefined
+		let closestSize = Infinity
 		for (const group of groups) {
-			if (group.module_ids.includes(moduleId)) {
-				if (!closest || group.module_ids.length < closest.module_ids.length) {
+			const membership = groupMemberships.get(group.id)
+			if (membership && membership.memberIds.includes(moduleId)) {
+				if (membership.memberIds.length < closestSize) {
 					closest = group
+					closestSize = membership.memberIds.length
 				}
 			}
 		}
@@ -204,32 +222,32 @@ export class GroupEditor {
 		return !!this.flowStore.val.value
 	}
 
-	/** Remove a deleted node from all groups. Removes empty groups. */
+	/**
+	 * Remove a deleted node from groups.
+	 * If nodeId === start_id or end_id: delete the group (cleanupGroups on next render will
+	 * catch any edge cases). If between: no-op (membership recomputes dynamically).
+	 */
 	removeNode(nodeId: string): void {
 		const groups = this.getGroups()
-		let changed = false
-		for (const group of groups) {
-			const idx = group.module_ids.indexOf(nodeId)
-			if (idx !== -1) {
-				group.module_ids.splice(idx, 1)
-				changed = true
-			}
-		}
-		if (changed) {
-			this.setGroups(groups.filter((g) => g.module_ids.length > 0))
+		const newGroups = groups.filter((g) => g.start_id !== nodeId && g.end_id !== nodeId)
+		if (newGroups.length !== groups.length) {
+			this.setGroups(newGroups)
 		}
 	}
 
-	/** Clean up groups: remove stale node IDs, complete paths, and split disconnected components. */
+	/**
+	 * Clean up groups: check start_id and end_id still exist, delete if not.
+	 * Much simpler than the old module_ids-based cleanup.
+	 */
 	cleanupGroups(flowNodes: { id: string; parentIds?: string[] }[]): void {
 		if (!this.isAvailable()) return
 
 		const groups = this.getGroups()
 		if (groups.length === 0) return
 
-		let hasChanges = false
 		const nodeSet = new Set(flowNodes.map((n) => n.id))
 		const newGroups: FlowGroup[] = []
+		let hasChanges = false
 
 		for (const group of groups) {
 			// Skip collapsed groups — their module nodes are replaced by a single
@@ -239,50 +257,10 @@ export class GroupEditor {
 				continue
 			}
 
-			// Step 1: Remove stale module_ids
-			const validIds = group.module_ids.filter((id) => nodeSet.has(id))
-			if (validIds.length !== group.module_ids.length) {
-				group.module_ids = validIds
-				hasChanges = true
-			}
-
-			if (group.module_ids.length === 0) {
-				hasChanges = true
-				continue
-			}
-
-			// Step 2: Complete paths and split disconnected components
-			const components = completeAndSplitGroup(group.module_ids, flowNodes)
-
-			if (components.length <= 1) {
-				const completed = components.length > 0 ? components[0] : []
-				const sortedCompleted = [...completed].sort()
-				const sortedOriginal = [...group.module_ids].sort()
-
-				if (
-					sortedCompleted.length !== sortedOriginal.length ||
-					!sortedCompleted.every((id, i) => id === sortedOriginal[i])
-				) {
-					group.module_ids = completed
-					hasChanges = true
-				}
-				if (group.module_ids.length > 0) {
-					newGroups.push(group)
-				} else {
-					hasChanges = true
-				}
+			if (nodeSet.has(group.start_id) && nodeSet.has(group.end_id)) {
+				newGroups.push(group)
 			} else {
-				// Split into multiple groups
 				hasChanges = true
-				for (const component of components) {
-					if (component.length === 0) continue
-					newGroups.push({
-						...group,
-						id: generateId(),
-						module_ids: component,
-						summary: group.summary ? `${group.summary}` : undefined
-					})
-				}
 			}
 		}
 
@@ -290,39 +268,6 @@ export class GroupEditor {
 			this.setGroups(newGroups)
 		}
 	}
-
-	/** Add a newly inserted node to the group that contains both its neighbors. */
-	addInsertedNode(newNodeId: string, sourceId?: string, targetId?: string): void {
-		if (!sourceId || !targetId) return
-		const groups = this.getGroups()
-		for (const group of groups) {
-			if (group.module_ids.includes(sourceId) && group.module_ids.includes(targetId)) {
-				group.module_ids.push(newNodeId)
-				this.setGroups(groups)
-				return
-			}
-		}
-	}
-
-	/** Handle a node that was moved to a new position in the flow. */
-	handleNodeMoved(movedId: string, sourceId?: string, targetId?: string): void {
-		const groups = this.getGroups()
-		const currentGroup = groups.find((g) => g.module_ids.includes(movedId))
-
-		if (currentGroup) {
-			// Was in a group — only keep if BOTH neighbors are in the same group
-			const sourceInGroup = sourceId ? currentGroup.module_ids.includes(sourceId) : false
-			const targetInGroup = targetId ? currentGroup.module_ids.includes(targetId) : false
-			if (!(sourceInGroup && targetInGroup)) {
-				// Moved to boundary or outside the group — remove
-				currentGroup.module_ids = currentGroup.module_ids.filter((id) => id !== movedId)
-				this.setGroups(groups.filter((g) => g.module_ids.length > 0))
-			}
-		} else {
-			// Wasn't in a group — check if moved into one
-			this.addInsertedNode(movedId, sourceId, targetId)
-		}
-	}
 }
 
 export type GroupEditorContext = {
@@ -393,9 +338,11 @@ export function computeCollapsedGroupNoteSpacing(
 /**
  * Compute adjusted node positions that account for group label spacing.
  * Follows the same push-down pattern as computeNoteNodes in noteUtils.
+ * Accepts a membership map to look up computed members per group.
  */
 export function computeGroupSpacing(
 	groups: FlowGroup[],
+	groupMemberships: Map<string, GroupMembership>,
 	nodes: Array<{ id: string; position: { x: number; y: number } }>,
 	noteHeights?: Record<string, number>
 ): Record<string, { x: number; y: number }> {
@@ -407,7 +354,8 @@ export function computeGroupSpacing(
 	const yPosMap: Record<number, number> = {}
 
 	for (const group of groups) {
-		if (group.module_ids.length === 0) continue
+		const memberIds = groupMemberships.get(group.id)?.memberIds ?? []
+		if (memberIds.length === 0) continue
 
 		// Find topmost node Y position in this group
 		// Check both member nodes (uncollapsed) and collapsed-group node (collapsed)
@@ -418,7 +366,7 @@ export function computeGroupSpacing(
 			if (node.id === collapsedNodeId && node.position.y < topY) {
 				topY = node.position.y
 				isCollapsed = true
-			} else if (group.module_ids.includes(node.id) && node.position.y < topY) {
+			} else if (memberIds.includes(node.id) && node.position.y < topY) {
 				topY = node.position.y
 				isCollapsed = false
 			}
diff --git a/frontend/src/lib/gen/types.gen.ts b/frontend/src/lib/gen/types.gen.ts
index 0eade4c5a8..e638b705b0 100644
--- a/frontend/src/lib/gen/types.gen.ts
+++ b/frontend/src/lib/gen/types.gen.ts
@@ -129,9 +129,13 @@ export type FlowValue = {
          */
         collapsed_by_default?: boolean;
         /**
-         * IDs of the flow modules belonging to this group. Must reference valid FlowModule ids from the flow's modules array.
+         * ID of the first flow module in this group (topological entry point)
          */
-        module_ids: Array<(string)>;
+        start_id: string;
+        /**
+         * ID of the last flow module in this group (topological exit point)
+         */
+        end_id: string;
         /**
          * Color for the group in the flow editor
          */
diff --git a/openflow.openapi.yaml b/openflow.openapi.yaml
index 77f85fd591..cffd12d2e4 100644
--- a/openflow.openapi.yaml
+++ b/openflow.openapi.yaml
@@ -216,7 +216,7 @@ components:
 
     FlowGroup:
       type: object
-      description: A semantic group of flow modules for organizational purposes. Does not affect execution — modules remain in their original position in the flow. Groups provide naming and collapsibility in the editor.
+      description: A semantic group of flow modules for organizational purposes. Does not affect execution — modules remain in their original position in the flow. Groups provide naming and collapsibility in the editor. Members are computed dynamically from all nodes on paths between start_id and end_id.
       properties:
         id:
           type: string
@@ -231,17 +231,19 @@ components:
           type: boolean
           default: false
           description: If true, this group is collapsed by default in the flow editor. UI hint only.
-        module_ids:
-          type: array
-          description: IDs of the flow modules belonging to this group. Must reference valid FlowModule ids from the flow's modules array.
-          items:
-            type: string
+        start_id:
+          type: string
+          description: ID of the first flow module in this group (topological entry point)
+        end_id:
+          type: string
+          description: ID of the last flow module in this group (topological exit point)
         color:
           type: string
           description: Color for the group in the flow editor
       required:
         - id
-        - module_ids
+        - start_id
+        - end_id
 
     RetryIf:
       type: object