diff --git a/frontend/src/lib/components/graph/FlowGraphV2.svelte b/frontend/src/lib/components/graph/FlowGraphV2.svelte index 262da4665d..e42488b36b 100644 --- a/frontend/src/lib/components/graph/FlowGraphV2.svelte +++ b/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 | undefined, (NodeDep & NodePos)[]] | undefined = undefined - let currentContainerDescendants: Map = $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 + flowNodes: { id: string; parentIds?: string[] }[] ): Map { const map = new Map() 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 = $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 ?? []) + ) + ) }) ) diff --git a/frontend/src/lib/components/graph/NodeContextMenu.svelte b/frontend/src/lib/components/graph/NodeContextMenu.svelte index 5c2fb2d90f..aa3f05805e 100644 --- a/frontend/src/lib/components/graph/NodeContextMenu.svelte +++ b/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?.() diff --git a/frontend/src/lib/components/graph/SelectionBoundingBox.svelte b/frontend/src/lib/components/graph/SelectionBoundingBox.svelte index c39697b3fc..befd9cd2d7 100644 --- a/frontend/src/lib/components/graph/SelectionBoundingBox.svelte +++ b/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?.() diff --git a/frontend/src/lib/components/graph/graphBuilder.svelte.ts b/frontend/src/lib/components/graph/graphBuilder.svelte.ts index 74f7391357..bf74bc7887 100644 --- a/frontend/src/lib/components/graph/graphBuilder.svelte.ts +++ b/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, showNotes: boolean @@ -436,7 +437,7 @@ export function graphBuilder( // Build a map: module_id -> collapsed group (only for collapsed groups) const moduleToCollapsedGroup = new Map() 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, diff --git a/frontend/src/lib/components/graph/graphContext.ts b/frontend/src/lib/components/graph/graphContext.ts index a219d30f07..599b53ae51 100644 --- a/frontend/src/lib/components/graph/graphContext.ts +++ b/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 /** Current group memberships (set by FlowGraphV2) */ getGroupMemberships?: () => Map } diff --git a/frontend/src/lib/components/graph/groupDetectionUtils.ts b/frontend/src/lib/components/graph/groupDetectionUtils.ts index 22cc02e4b6..bd0a667410 100644 --- a/frontend/src/lib/components/graph/groupDetectionUtils.ts +++ b/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 + flowNodes: FlowNode[] ): GroupMembership { - const nodeMap = new Map() - 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() - 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() - 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 + 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 diff --git a/frontend/src/lib/components/graph/groupEditor.svelte.ts b/frontend/src/lib/components/graph/groupEditor.svelte.ts index 4bdce73d10..26d56fb4ae 100644 --- a/frontend/src/lib/components/graph/groupEditor.svelte.ts +++ b/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 + 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()