From 9c7c7b6f1e9f4738f13b18be21db4009f7275318 Mon Sep 17 00:00:00 2001 From: centdix Date: Mon, 13 Apr 2026 11:23:28 +0200 Subject: [PATCH] refactor: stabilize flow delete execution Co-Authored-By: Claude Opus 4.5 --- .../flows/flowDeleteController.test.ts | 142 ++++++++++++++++++ .../components/flows/flowDeleteController.ts | 135 +++++++++++++++++ .../components/flows/flowDeleteUtils.test.ts | 24 ++- .../lib/components/flows/flowDeleteUtils.ts | 41 ++--- .../flows/map/FlowModuleSchemaMap.svelte | 54 +++---- .../graph/groupedModulesProxy.svelte.ts | 4 - 6 files changed, 340 insertions(+), 60 deletions(-) create mode 100644 frontend/src/lib/components/flows/flowDeleteController.test.ts create mode 100644 frontend/src/lib/components/flows/flowDeleteController.ts diff --git a/frontend/src/lib/components/flows/flowDeleteController.test.ts b/frontend/src/lib/components/flows/flowDeleteController.test.ts new file mode 100644 index 0000000000..263190b5c4 --- /dev/null +++ b/frontend/src/lib/components/flows/flowDeleteController.test.ts @@ -0,0 +1,142 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('../aiProviderStorage', () => ({ + loadStoredConfig: () => undefined +})) + +vi.mock('./flowInfers', () => ({ + AI_AGENT_SCHEMA: { properties: {} } +})) + +import { GroupDisplayState } from '$lib/components/graph/groupEditor.svelte' +import { + GroupedModulesProxy, + type ExtendedOpenFlow +} from '$lib/components/graph/groupedModulesProxy.svelte' +import type { FlowModule, OpenFlow } from '$lib/gen' +import type { StateStore } from '$lib/utils' +import { executeDeletePlan, prepareDeleteRequest } from './flowDeleteController' + +function makeRawModule(id: string, expr?: string): FlowModule { + return { + id, + summary: id, + value: { + type: 'rawscript', + content: '', + language: 'python3', + input_transforms: expr + ? { + user_input: { + type: 'javascript', + expr + } + } + : {} + } as any + } as FlowModule +} + +function makeAiAgent(id: string, tools: any[]): FlowModule { + return { + id, + summary: id, + value: { + type: 'aiagent', + tools, + input_transforms: {} + } as any + } as FlowModule +} + +function makeFlowModuleTool(module: FlowModule) { + return { + id: module.id, + summary: module.summary, + value: { + tool_type: 'flowmodule', + ...module.value + } + } +} + +describe('flowDeleteController', () => { + it('prepares confirmations and executes grouped ai-agent deletes end to end', () => { + const agent = makeAiAgent('agent_step', [ + makeFlowModuleTool(makeRawModule('lookup_user')), + makeFlowModuleTool( + makeAiAgent('support_agent', [makeFlowModuleTool(makeRawModule('create_ticket'))]) + ) + ]) + const dependent = makeRawModule('dependent_step', 'results.create_ticket?.value') + const flowStore = { + val: { + summary: 'Flow', + value: { + modules: [agent, dependent], + groups: [ + { + summary: 'Agent group', + start_id: 'agent_step', + end_id: 'agent_step' + } + ] + } + } as OpenFlow + } satisfies StateStore + const proxy = new GroupedModulesProxy(flowStore as unknown as StateStore) + const displayState = new GroupDisplayState(() => flowStore.val.value.groups ?? []) + + const request = prepareDeleteRequest({ + ids: ['agent_step'], + flow: flowStore.val, + tree: proxy.items, + proxy, + displayState + }) + + expect(request?.needsDependencyConfirmation).toBe(true) + expect(request?.plan.affectedGroups).toHaveLength(1) + expect(request?.plan.affectedGroups[0]).toMatchObject({ + summary: 'Agent group', + start_id: 'agent_step', + end_id: 'agent_step' + }) + + const selectionManager = { + clearSelection: vi.fn(), + selectId: vi.fn() + } + const flowStateStore = { + val: { + agent_step: {}, + lookup_user: {}, + support_agent: {}, + create_ticket: {}, + dependent_step: {} + } + } as StateStore + const onDelete = vi.fn() + + const result = executeDeletePlan(request!.plan, { + flowStore, + flowStateStore, + selectionManager, + proxy, + displayState, + onDelete + }) + + expect(result.removedStateIds).toEqual([ + 'agent_step', + 'lookup_user', + 'support_agent', + 'create_ticket' + ]) + expect(flowStore.val.value.modules.map((module) => module.id)).toEqual(['dependent_step']) + expect(flowStore.val.value.groups ?? []).toEqual([]) + expect(Object.keys(flowStateStore.val)).toEqual(['dependent_step']) + expect(selectionManager.selectId).toHaveBeenCalledWith('dependent_step') + expect(onDelete).toHaveBeenCalledWith('agent_step') + }) +}) diff --git a/frontend/src/lib/components/flows/flowDeleteController.ts b/frontend/src/lib/components/flows/flowDeleteController.ts new file mode 100644 index 0000000000..677b9c0d60 --- /dev/null +++ b/frontend/src/lib/components/flows/flowDeleteController.ts @@ -0,0 +1,135 @@ +import type { GroupDisplayState } from '$lib/components/graph/groupEditor.svelte' +import type { GroupedModulesProxy } from '$lib/components/graph/groupedModulesProxy.svelte' +import type { SelectionManager } from '$lib/components/graph/selectionUtils.svelte' +import type { FlowStructureNode } from '$lib/components/graph/flowStructure' +import type { FlowModule, OpenFlow } from '$lib/gen' +import { push, type History } from '$lib/history.svelte' +import { refreshStateStore } from '$lib/svelte5Utils.svelte' +import type { StateStore } from '$lib/utils' +import { collectFlowNodeIds } from './agentToolTree' +import { + createDeletePlan, + removeDeletePlanTools, + type DeletePlan, + type DeleteTarget +} from './flowDeleteUtils' +import type { FlowState } from './flowState' +import { deleteFlowStateById } from './flowStateUtils.svelte' +import { dfsByModule } from './previousResults' + +export type PreparedDeleteRequest = { + plan: DeletePlan + needsDependencyConfirmation: boolean +} + +export type DeletePlanExecutionContext = { + history?: History + flowStore: StateStore + flowStateStore: StateStore + selectionManager: Pick + proxy: GroupedModulesProxy + displayState: GroupDisplayState + onDelete?: (id: string) => void +} + +export type DeletePlanExecutionResult = { + removedStateIds: string[] + removedToolStateIds: string[] +} + +export function prepareDeleteRequest(args: { + ids: string[] + flow: OpenFlow + tree: FlowStructureNode[] + proxy: GroupedModulesProxy + displayState: GroupDisplayState +}): PreparedDeleteRequest | undefined { + const plan = createDeletePlan(args) + if (!plan) { + return undefined + } + + return { + plan, + needsDependencyConfirmation: Object.keys(plan.dependents).length > 0 + } +} + +export function executeDeletePlan( + plan: DeletePlan, + args: DeletePlanExecutionContext +): DeletePlanExecutionResult { + push(args.history, args.flowStore.val) + const hasPreprocessor = Boolean(args.flowStore.val.value.preprocessor_module) + const removedStructureStateIds = resolveLiveStructureStateIds( + plan.targets, + args.flowStore.val.value.modules, + hasPreprocessor + ) + + if (plan.selection.kind === 'clear') { + args.selectionManager.clearSelection() + } else { + args.selectionManager.selectId(plan.selection.id) + } + + if (plan.targets.some((target) => target.kind === 'preprocessor')) { + args.flowStore.val.value.preprocessor_module = undefined + } + + if (plan.structureIds.length > 0) { + const structureDelete = args.proxy.prepareDelete(plan.structureIds, { + displayState: args.displayState + }) + structureDelete.commit({ removeDuplicates: plan.removeDuplicates }) + } + + const removedToolStateIds = removeDeletePlanTools(plan.targets, args.flowStore.val.value.modules) + const removedStateIds = uniqueIds([...removedStructureStateIds, ...removedToolStateIds]) + + for (const id of removedStateIds) { + deleteFlowStateById(id, args.flowStateStore) + } + + refreshStateStore(args.flowStore) + + if (plan.inputIds.length === 1) { + args.onDelete?.(plan.targets[0]?.id ?? plan.inputIds[0]) + } + + return { + removedStateIds, + removedToolStateIds + } +} + +function resolveLiveStructureStateIds( + targets: DeleteTarget[], + modules: FlowModule[], + hasPreprocessor: boolean +): string[] { + const removedIds: string[] = [] + + for (const target of targets) { + if (target.kind === 'preprocessor') { + if (hasPreprocessor) { + removedIds.push(target.id) + } + continue + } + if (target.kind !== 'structure_node') { + continue + } + + const module = dfsByModule(target.id, modules)[0] + if (module) { + removedIds.push(...collectFlowNodeIds(module)) + } + } + + return uniqueIds(removedIds) +} + +function uniqueIds(ids: string[]): string[] { + return [...new Set(ids)] +} diff --git a/frontend/src/lib/components/flows/flowDeleteUtils.test.ts b/frontend/src/lib/components/flows/flowDeleteUtils.test.ts index f878d3f47f..fb215b3b0f 100644 --- a/frontend/src/lib/components/flows/flowDeleteUtils.test.ts +++ b/frontend/src/lib/components/flows/flowDeleteUtils.test.ts @@ -103,7 +103,6 @@ describe('createDeletePlan', () => { ] const commit = vi.fn() const prepareDelete = vi.fn(() => ({ - removedIds: ['agent_step'], affectedGroups: [], duplicateGroups: [], commit @@ -121,7 +120,9 @@ describe('createDeletePlan', () => { }) expect(prepareDelete).toHaveBeenCalledWith(['agent_step'], expect.any(Object)) - expect(plan?.stateIds).toEqual(['agent_step', 'lookup_user']) + expect(plan?.plannedStateIds).toEqual(['agent_step', 'lookup_user']) + expect(plan?.structureIds).toEqual(['agent_step']) + expect(plan?.affectedGroups).toEqual([]) expect(plan?.dependents).toEqual({ dependent_step: ['results.lookup_user?.value'] }) @@ -142,7 +143,24 @@ describe('removeDeletePlanTools', () => { false ) - expect(removeDeletePlanTools(targets)).toEqual(['support_agent', 'create_ticket']) + expect(removeDeletePlanTools(targets, [rootAgent])).toEqual(['support_agent', 'create_ticket']) expect((rootAgent.value as any).tools).toEqual([]) }) + + it('re-resolves live owners before removing planned tool targets', () => { + const rootAgent = makeAiAgent('root_agent', [ + makeFlowModuleTool(makeRawModule('lookup_user')), + makeFlowModuleTool(makeAiAgent('support_agent', [makeFlowModuleTool(makeRawModule('create_ticket'))])) + ]) + const tree: FlowStructureNode[] = [] + const { targets } = resolveDeleteTargets(tree, [rootAgent], ['support_agent'], false) + + ;(rootAgent.value as any).tools.unshift(makeFlowModuleTool(makeRawModule('new_lookup'))) + + expect(removeDeletePlanTools(targets, [rootAgent])).toEqual(['support_agent', 'create_ticket']) + expect(((rootAgent.value as any).tools as any[]).map((tool) => tool.id)).toEqual([ + 'new_lookup', + 'lookup_user' + ]) + }) }) diff --git a/frontend/src/lib/components/flows/flowDeleteUtils.ts b/frontend/src/lib/components/flows/flowDeleteUtils.ts index b9534631f1..2b45f068bd 100644 --- a/frontend/src/lib/components/flows/flowDeleteUtils.ts +++ b/frontend/src/lib/components/flows/flowDeleteUtils.ts @@ -1,8 +1,5 @@ -import type { GroupDisplayState } from '$lib/components/graph/groupEditor.svelte' -import type { - GroupedModulesProxy, - PreparedStructureDelete -} from '$lib/components/graph/groupedModulesProxy.svelte' +import type { FlowGroup, GroupDisplayState } from '$lib/components/graph/groupEditor.svelte' +import type { GroupedModulesProxy } from '$lib/components/graph/groupedModulesProxy.svelte' import { findInStructure, type FlowStructureNode } from '$lib/components/graph/flowStructure' import type { FlowModule, OpenFlow } from '$lib/gen' import { @@ -39,7 +36,7 @@ export type StructureDeleteTarget = DeleteTargetBase & { export type AgentToolDeleteTarget = DeleteTargetBase & { kind: 'agent_tool' - owner: AgentToolOwner + expectedOwnerAgentId: string } export type DeleteTarget = @@ -50,10 +47,11 @@ export type DeleteTarget = export type DeletePlan = { inputIds: string[] targets: DeleteTarget[] - stateIds: string[] + plannedStateIds: string[] dependents: Record selection: DeleteSelection - structureDelete?: PreparedStructureDelete + structureIds: string[] + affectedGroups: FlowGroup[] removeDuplicates: boolean } @@ -104,7 +102,7 @@ export function resolveDeleteTargets( targets.push({ kind: 'agent_tool', id, - owner, + expectedOwnerAgentId: owner.agentId, stateIds: collectAgentToolIds(owner.tool) }) continue @@ -146,26 +144,35 @@ export function createDeletePlan(args: { ? args.proxy.prepareDelete(structureIds, { displayState: args.displayState }) : undefined - const stateIds = uniqueIds(targets.flatMap((target) => target.stateIds)) + const plannedStateIds = uniqueIds(targets.flatMap((target) => target.stateIds)) return { inputIds: args.ids, targets, - stateIds, - dependents: collectDeleteDependents(stateIds, args.flow), - selection: getDeleteSelection(args.ids, stateIds, args.flow.value.modules), - structureDelete, - removeDuplicates: Boolean(structureDelete && structureDelete.duplicateGroups.length > 0) + plannedStateIds, + dependents: collectDeleteDependents(plannedStateIds, args.flow), + selection: getDeleteSelection(args.ids, plannedStateIds, args.flow.value.modules), + structureIds, + affectedGroups: structureDelete?.affectedGroups ?? [], + removeDuplicates: Boolean(structureDelete?.duplicateGroups.length) } } export function removeDeletePlanTools( targets: DeleteTarget[], + modules: FlowModule[], onRemove?: (tool: AgentTool) => void ): string[] { const removedIds = new Set() const toolTargets = targets .filter((target): target is AgentToolDeleteTarget => target.kind === 'agent_tool') + .map((target) => { + const owner = findAgentToolOwner(modules, target.id) + return owner ? { owner, target } : undefined + }) + .filter((entry): entry is { owner: AgentToolOwner; target: AgentToolDeleteTarget } => + Boolean(entry) + ) .sort((left, right) => { if (left.owner.depth !== right.owner.depth) { return right.owner.depth - left.owner.depth @@ -176,8 +183,8 @@ export function removeDeletePlanTools( return 0 }) - for (const target of toolTargets) { - const removed = removeAgentToolOwner(target.owner) + for (const { owner } of toolTargets) { + const removed = removeAgentToolOwner(owner) if (!removed) { continue } diff --git a/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte b/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte index 95ff9ae83f..bced95048d 100644 --- a/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte +++ b/frontend/src/lib/components/flows/map/FlowModuleSchemaMap.svelte @@ -53,11 +53,8 @@ agentToolToFlowModule } from '../agentToolUtils' import { loadFlowModuleState } from '../flowStateUtils.svelte' - import { - createDeletePlan, - removeDeletePlanTools, - type DeletePlan - } from '../flowDeleteUtils' + import type { DeletePlan } from '../flowDeleteUtils' + import { executeDeletePlan, prepareDeleteRequest } from '../flowDeleteController' import { getNoteEditorContext } from '$lib/components/graph/noteEditor.svelte' import { GroupedModulesProxy, @@ -340,55 +337,40 @@ } function applyDeletePlan(plan: DeletePlan) { - push(history, flowStore.val) - - if (plan.selection.kind === 'clear') { - selectionManager.clearSelection() - } else { - selectionManager.selectId(plan.selection.id) - } - - if (plan.targets.some((target) => target.kind === 'preprocessor')) { - flowStore.val.value.preprocessor_module = undefined - } - - plan.structureDelete?.commit({ removeDuplicates: plan.removeDuplicates }) - removeDeletePlanTools(plan.targets) - - for (const id of plan.stateIds) { - deleteFlowStateById(id, flowStateStore) - } - - refreshStateStore(flowStore) - - if (plan.inputIds.length === 1) { - onDelete?.(plan.targets[0]?.id ?? plan.inputIds[0]) - } + executeDeletePlan(plan, { + history, + flowStore, + flowStateStore, + selectionManager, + proxy, + displayState: groupDisplayState, + onDelete + }) } function requestDelete(ids: string[]) { - const plan = createDeletePlan({ + const request = prepareDeleteRequest({ ids, flow: flowStore.val, tree: proxy.items, proxy, displayState: groupDisplayState }) - if (!plan) { + if (!request) { return } const proceed = () => { - if (Object.keys(plan.dependents).length > 0) { - pendingDeleteConfirmation = { plan } + if (request.needsDependencyConfirmation) { + pendingDeleteConfirmation = { plan: request.plan } } else { - applyDeletePlan(plan) + applyDeletePlan(request.plan) } } - if ((plan.structureDelete?.affectedGroups.length ?? 0) > 0) { + if (request.plan.affectedGroups.length > 0) { pendingGroupAction = { - groups: plan.structureDelete!.affectedGroups, + groups: request.plan.affectedGroups, label: 'delete', confirm: proceed } diff --git a/frontend/src/lib/components/graph/groupedModulesProxy.svelte.ts b/frontend/src/lib/components/graph/groupedModulesProxy.svelte.ts index 4fdcd2a2f9..27fa54463c 100644 --- a/frontend/src/lib/components/graph/groupedModulesProxy.svelte.ts +++ b/frontend/src/lib/components/graph/groupedModulesProxy.svelte.ts @@ -27,7 +27,6 @@ export type ExtendedOpenFlow = { } export type PreparedStructureDelete = { - removedIds: string[] affectedGroups: FlowGroup[] duplicateGroups: FlowGroup[] commit: (commitOpts?: { removeDuplicates?: boolean }) => void @@ -126,7 +125,6 @@ export class GroupedModulesProxy { displayState?: import('./groupEditor.svelte').GroupDisplayState } ): PreparedStructureDelete { - const removedIds: string[] = [] const { emptiedGroups, duplicateGroups, commit } = this.prepareMutation((tree) => { for (const id of ids) { const found = findInStructure(tree, id) @@ -135,12 +133,10 @@ export class GroupedModulesProxy { } found.parentChildren.splice(found.index, 1) - removedIds.push(id) } }, opts) return { - removedIds, affectedGroups: [...emptiedGroups, ...duplicateGroups], duplicateGroups, commit