refactor: separate group module membership from graph node membership

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

frontend/src/lib/components/graph/FlowGraphV2.svelte

@@ -69,7 +69,12 @@
 		GROUP_TOP_MARGIN,
 		type FlowGroup
 	} from './groupEditor.svelte'
-	import { computeGroupMembers, type GroupMembership } from './groupDetectionUtils'
+	import {
+		computeGroupNodeIds,
+		computeGroupModuleIds,
+		type GroupMembership
+	} from './groupDetectionUtils'
+	import { getAllModules } from '../flows/flowExplorer'
 	import SelectionTool from './SelectionTool.svelte'
 	import PaneContextMenu from './PaneContextMenu.svelte'
 	import { SelectionManager } from './selectionUtils.svelte'
@@ -333,7 +338,6 @@
 		yOffset,
 		diffManager,
 		getFlowNodes: () => currentGraphNodeDeps,
-		getContainerDescendants: () => currentContainerDescendants,
 		getGroupMemberships: () => currentGroupMemberships
 	} as any)
 
@@ -374,7 +378,6 @@
 	let lastNodes:
 		| [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.
@@ -382,8 +385,7 @@
 	 *  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[]>
+		flowNodes: { id: string; parentIds?: string[] }[]
 	): Map<string, GroupMembership> {
 		const map = new Map<string, GroupMembership>()
 		for (const group of groups) {
@@ -394,13 +396,13 @@
 			}
 			map.set(
 				group.id,
-				computeGroupMembers(group.start_id, group.end_id, flowNodes, contDescendants)
+				computeGroupNodeIds(group.start_id, group.end_id, flowNodes)
 			)
 		}
 		return map
 	}
 
-	// Current group memberships — recomputed when groups, nodes, or containerDescendants change
+	// Current group memberships — recomputed when groups or nodes change
 	let currentGroupMemberships: Map<string, GroupMembership> = $state(new Map())
 
 	const MAX_TOOLS_PER_ROW = 2
@@ -566,11 +568,7 @@
 		}
 
 		// Run recursive compound layout with pre-computed extra space
-		const {
-			positions,
-			bbox,
-			containerDescendants: layoutContainerDescendants
-		} = compoundLayout(
+		const { positions, bbox } = compoundLayout(
 			nodes,
 			{
 				nodeWidth: NODE.width,
@@ -591,7 +589,6 @@
 			}
 		}))
 
-		currentContainerDescendants = layoutContainerDescendants ?? new Map()
 		lastNodes = [nodes, nodeExtraSpace, newNodes]
 		return newNodes
 	}
@@ -831,8 +828,7 @@
 		// Compute group memberships from start_id/end_id
 		currentGroupMemberships = computeAllGroupMemberships(
 			groupEditorContext?.groupEditor.getGroups() ?? [],
-			graphNodeDeps,
-			currentContainerDescendants
+			graphNodeDeps
 		)
 
 		// Pre-compute extra space per node for assets, AI tools, group notes, group headers
@@ -1038,7 +1034,14 @@
 		const collapsedGroups = (groupEditorContext?.groupEditor.getCollapsedGroups() ?? []).map(
 			(g) => ({
 				...g,
-				memberIds: untrack(() => currentGroupMemberships.get(g.id)?.memberIds ?? [])
+				memberIds: untrack(() => currentGroupMemberships.get(g.id)?.memberIds ?? []),
+				moduleIds: untrack(() =>
+					computeGroupModuleIds(
+						g.start_id,
+						g.end_id,
+						getAllModules(effectiveModules ?? [])
+					)
+				)
 			})
 		)
 

frontend/src/lib/components/graph/NodeContextMenu.svelte

@@ -25,8 +25,7 @@
 	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
+		return canFormValidGroup(selectedNodeIds, flowNodes).valid
 	})
 
 	const menuItems: ContextMenuItem[] = $derived([
@@ -38,8 +37,7 @@
 			onClick: () => {
 				if (selectedNodeIds.length > 0 && groupEditorContext?.groupEditor && graphContext) {
 					const flowNodes = graphContext.getFlowNodes?.() ?? []
-					const contDesc = graphContext.getContainerDescendants?.() ?? new Map()
-					groupEditorContext.groupEditor.createGroup(selectedNodeIds, flowNodes, contDesc)
+					groupEditorContext.groupEditor.createGroup(selectedNodeIds, flowNodes)
 
 					tick().then(() => {
 						graphContext?.clearFlowSelection?.()

frontend/src/lib/components/graph/SelectionBoundingBox.svelte

@@ -60,15 +60,13 @@
 	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
+		return canFormValidGroup(selectedNodes, flowNodes).valid
 	})
 
 	function handleAddGroup() {
 		if (selectedNodes.length > 0 && groupEditorContext?.groupEditor && graphContext) {
 			const flowNodes = graphContext.getFlowNodes?.() ?? []
-			const contDesc = graphContext.getContainerDescendants?.() ?? new Map()
-			groupEditorContext.groupEditor.createGroup(selectedNodes, flowNodes, contDesc)
+			groupEditorContext.groupEditor.createGroup(selectedNodes, flowNodes)
 
 			tick().then(() => {
 				graphContext?.clearFlowSelection?.()

frontend/src/lib/components/graph/graphBuilder.svelte.ts

@@ -414,6 +414,7 @@ export function graphBuilder(
 		start_id: string
 		end_id: string
 		memberIds: string[]
+		moduleIds: string[]
 	}>,
 	collapsedContainers: Set<string>,
 	showNotes: boolean
@@ -436,7 +437,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.memberIds) {
+			for (const moduleId of group.moduleIds) {
 				moduleToCollapsedGroup.set(moduleId, group)
 			}
 		}
@@ -699,8 +700,8 @@ export function graphBuilder(
 								summary: collapsedGroup.summary,
 								note: collapsedGroup.note,
 								color: collapsedGroup.color,
-								stepCount: collapsedGroup.memberIds.length,
-								modules: collapsedGroup.memberIds
+								stepCount: collapsedGroup.moduleIds.length,
+								modules: collapsedGroup.moduleIds
 									.map((id) => getAllModules(modules).find((m) => m.id === id))
 									.filter((m): m is FlowModule => m != null),
 								showNotes,

frontend/src/lib/components/graph/graphContext.ts

@@ -17,8 +17,6 @@ export type GraphContext = {
 	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>
 }

frontend/src/lib/components/graph/groupDetectionUtils.ts

@@ -1,5 +1,4 @@
 import { topologicalSort } from './graphBuilder.svelte'
-import { expandWithContainerDescendants } from './util'
 
 type FlowNode = { id: string; parentIds?: string[] }
 
@@ -10,79 +9,79 @@ export type GroupMembership = {
 }
 
 /**
- * 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
+ * Compute the set of graph nodes that belong to a group defined by start_id and end_id.
+ * Uses topological sort and slices between start and end — valid because windmill flows
+ * are series-parallel (branches always converge at explicit end nodes).
  */
-export function computeGroupMembers(
+export function computeGroupNodeIds(
 	startId: string,
 	endId: string,
-	flowNodes: FlowNode[],
-	containerDescendants: Map<string, string[]>
+	flowNodes: FlowNode[]
 ): GroupMembership {
-	const nodeMap = new Map<string, FlowNode>()
-	for (const n of flowNodes) nodeMap.set(n.id, n)
+	if (startId === endId) {
+		const exists = flowNodes.some((n) => n.id === startId)
+		return exists
+			? { memberIds: [startId], valid: true }
+			: { memberIds: [], valid: false, error: 'Start node not found' }
+	}
 
-	if (!nodeMap.has(startId) || !nodeMap.has(endId)) {
+	const sorted = topologicalSort(flowNodes)
+	// Topo sort puts bottom-of-graph nodes first, top-of-graph nodes last.
+	// start_id = top of group (visually) = topo-last, end_id = bottom (visually) = topo-first.
+	const endIdx = sorted.findIndex((n) => n.id === endId)
+
+	// If end_id is a container (branchall, forloop, etc.), its last graph node
+	// is `${endId}-end`. Use that as the actual start of the slice.
+	const endNodeId = sorted.some((n) => n.id === `${endId}-end`) ? `${endId}-end` : endId
+	const firstIdx = endNodeId === endId ? endIdx : sorted.findIndex((n) => n.id === endNodeId)
+
+	const lastIdx = sorted.findIndex((n) => n.id === startId)
+
+	if (firstIdx === -1 || lastIdx === -1) {
 		return { memberIds: [], valid: false, error: 'Start or end node not found' }
 	}
+	if (firstIdx > lastIdx) {
+		return { memberIds: [], valid: false, error: 'end_id must be topologically before start_id' }
+	}
 
-	// Single node group
+	return {
+		memberIds: sorted.slice(firstIdx, lastIdx + 1).map((n) => n.id),
+		valid: true
+	}
+}
+
+/**
+ * Compute the set of module IDs that belong to a group defined by start_id and end_id.
+ * Uses the flattened module list (from getAllModules) and slices between start and end.
+ * Used for collapsed group icons, step count, and moduleToCollapsedGroup mapping.
+ */
+export function computeGroupModuleIds(
+	startId: string,
+	endId: string,
+	allModules: { id: string }[]
+): string[] {
 	if (startId === endId) {
-		const members = expandWithContainerDescendants([startId], containerDescendants)
-		return { memberIds: members, valid: true }
+		return allModules.some((m) => m.id === startId) ? [startId] : []
 	}
 
-	// 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])
-			}
-		}
+	const startIdx = allModules.findIndex((m) => m.id === startId)
+	const endIdx = allModules.findIndex((m) => m.id === endId)
+
+	if (startIdx === -1 || endIdx === -1 || startIdx > endIdx) {
+		return []
 	}
 
-	// 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 }
+	return allModules.slice(startIdx, endIdx + 1).map((m) => m.id)
 }
 
 /**
  * Check whether a set of selected node IDs can form a valid group.
  * Uses topologicalSort to find start (first) and end (last) of selection,
- * then validates with computeGroupMembers.
+ * then validates with computeGroupNodeIds.
  */
 export function canFormValidGroup(
 	selectedIds: string[],
-	flowNodes: FlowNode[],
-	containerDescendants: Map<string, string[]>
+	flowNodes: FlowNode[]
 ): { valid: true; startId: string; endId: string } | { valid: false } {
 	if (selectedIds.length === 0) return { valid: false }
 
@@ -94,11 +93,13 @@ export function canFormValidGroup(
 
 	if (selectedSorted.length === 0) return { valid: false }
 
-	const startId = selectedSorted[0].id
-	const endId = selectedSorted[selectedSorted.length - 1].id
+	// Topo sort: index 0 = bottom of flow, last index = top of flow
+	// start_id = top (visually), end_id = bottom (visually)
+	const startId = selectedSorted[selectedSorted.length - 1].id
+	const endId = selectedSorted[0].id
 
 	// Validate that the computed members match the selection
-	const membership = computeGroupMembers(startId, endId, flowNodes, containerDescendants)
+	const membership = computeGroupNodeIds(startId, endId, flowNodes)
 	if (!membership.valid) return { valid: false }
 
 	// Check that all selected ids are in the computed members

frontend/src/lib/components/graph/groupEditor.svelte.ts

@@ -110,8 +110,7 @@ export class GroupEditor {
 	 */
 	createGroup(
 		moduleIds: string[],
-		flowNodes: { id: string; parentIds?: string[] }[],
-		containerDescendants: Map<string, string[]>
+		flowNodes: { id: string; parentIds?: string[] }[]
 	): string | undefined {
 		// Filter subflow node IDs (same logic as NoteEditor.createGroupNote)
 		let filteredIds = [...moduleIds]
@@ -129,7 +128,7 @@ export class GroupEditor {
 			filteredIds = [...filteredIds, ...subflowIds]
 		}
 
-		const result = canFormValidGroup(filteredIds, flowNodes, containerDescendants)
+		const result = canFormValidGroup(filteredIds, flowNodes)
 		if (!result.valid) return undefined
 
 		const groups = this.getGroups()