From b1118935f7635b38db560fd44d5852f62a23d0a8 Mon Sep 17 00:00:00 2001 From: Waleed Date: Sat, 31 Jan 2026 17:55:32 -0800 Subject: [PATCH] fix(workflow): optimize loop/parallel regeneration and prevent duplicate agent tools (#3100) * fix(workflow): optimize loop/parallel regeneration and prevent duplicate agent tools * refactor(workflow): remove addBlock in favor of batchAddBlocks - Migrated undo-redo to use batchAddBlocks instead of addBlock loop - Removed addBlock method from workflow store (now unused) - Updated tests to use helper function wrapping batchAddBlocks - This fixes the cursor bot comments about inconsistent parent checking --- .../components/tool-input/tool-input.test.ts | 443 +++++++++++++++++ .../components/tool-input/tool-input.tsx | 168 ++++--- apps/sim/hooks/use-undo-redo.ts | 89 +--- .../stores/workflows/workflow/store.test.ts | 448 ++++++++++++++++-- apps/sim/stores/workflows/workflow/store.ts | 161 +------ apps/sim/stores/workflows/workflow/types.ts | 16 - 6 files changed, 991 insertions(+), 334 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.test.ts diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.test.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.test.ts new file mode 100644 index 000000000..8d2548c13 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.test.ts @@ -0,0 +1,443 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' + +interface StoredTool { + type: string + title?: string + toolId?: string + params?: Record + customToolId?: string + schema?: any + code?: string + operation?: string + usageControl?: 'auto' | 'force' | 'none' +} + +const isMcpToolAlreadySelected = (selectedTools: StoredTool[], mcpToolId: string): boolean => { + return selectedTools.some((tool) => tool.type === 'mcp' && tool.toolId === mcpToolId) +} + +const isCustomToolAlreadySelected = ( + selectedTools: StoredTool[], + customToolId: string +): boolean => { + return selectedTools.some( + (tool) => tool.type === 'custom-tool' && tool.customToolId === customToolId + ) +} + +const isWorkflowAlreadySelected = (selectedTools: StoredTool[], workflowId: string): boolean => { + return selectedTools.some( + (tool) => tool.type === 'workflow_input' && tool.params?.workflowId === workflowId + ) +} + +describe('isMcpToolAlreadySelected', () => { + describe('basic functionality', () => { + it.concurrent('returns false when selectedTools is empty', () => { + expect(isMcpToolAlreadySelected([], 'mcp-tool-123')).toBe(false) + }) + + it.concurrent('returns false when MCP tool is not in selectedTools', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'different-mcp-tool', title: 'Different Tool' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-123')).toBe(false) + }) + + it.concurrent('returns true when MCP tool is already selected', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-tool-123', title: 'My MCP Tool' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-123')).toBe(true) + }) + + it.concurrent('returns true when MCP tool is one of many selected tools', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', customToolId: 'custom-1' }, + { type: 'mcp', toolId: 'mcp-tool-123', title: 'My MCP Tool' }, + { type: 'workflow_input', toolId: 'workflow_executor' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-123')).toBe(true) + }) + }) + + describe('type discrimination', () => { + it.concurrent('does not match non-MCP tools with same toolId', () => { + const selectedTools: StoredTool[] = [{ type: 'http_request', toolId: 'mcp-tool-123' }] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-123')).toBe(false) + }) + + it.concurrent('does not match custom tools even with toolId set', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', toolId: 'custom-mcp-tool-123', customToolId: 'db-id' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-123')).toBe(false) + }) + }) + + describe('multiple MCP tools', () => { + it.concurrent('correctly identifies first of multiple MCP tools', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-tool-1', title: 'Tool 1' }, + { type: 'mcp', toolId: 'mcp-tool-2', title: 'Tool 2' }, + { type: 'mcp', toolId: 'mcp-tool-3', title: 'Tool 3' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-1')).toBe(true) + }) + + it.concurrent('correctly identifies middle MCP tool', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-tool-1', title: 'Tool 1' }, + { type: 'mcp', toolId: 'mcp-tool-2', title: 'Tool 2' }, + { type: 'mcp', toolId: 'mcp-tool-3', title: 'Tool 3' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-2')).toBe(true) + }) + + it.concurrent('correctly identifies last MCP tool', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-tool-1', title: 'Tool 1' }, + { type: 'mcp', toolId: 'mcp-tool-2', title: 'Tool 2' }, + { type: 'mcp', toolId: 'mcp-tool-3', title: 'Tool 3' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-3')).toBe(true) + }) + + it.concurrent('returns false for non-existent MCP tool among many', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-tool-1', title: 'Tool 1' }, + { type: 'mcp', toolId: 'mcp-tool-2', title: 'Tool 2' }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'mcp-tool-999')).toBe(false) + }) + }) +}) + +describe('isCustomToolAlreadySelected', () => { + describe('basic functionality', () => { + it.concurrent('returns false when selectedTools is empty', () => { + expect(isCustomToolAlreadySelected([], 'custom-tool-123')).toBe(false) + }) + + it.concurrent('returns false when custom tool is not in selectedTools', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', customToolId: 'different-custom-tool' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(false) + }) + + it.concurrent('returns true when custom tool is already selected', () => { + const selectedTools: StoredTool[] = [{ type: 'custom-tool', customToolId: 'custom-tool-123' }] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(true) + }) + + it.concurrent('returns true when custom tool is one of many selected tools', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-1', title: 'MCP Tool' }, + { type: 'custom-tool', customToolId: 'custom-tool-123' }, + { type: 'http_request', toolId: 'http_request_tool' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(true) + }) + }) + + describe('type discrimination', () => { + it.concurrent('does not match non-custom tools with similar IDs', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'custom-tool-123', title: 'MCP with similar ID' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(false) + }) + + it.concurrent('does not match MCP tools even if customToolId happens to match', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-id', customToolId: 'custom-tool-123' } as StoredTool, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(false) + }) + }) + + describe('legacy inline custom tools', () => { + it.concurrent('does not match legacy inline tools without customToolId', () => { + const selectedTools: StoredTool[] = [ + { + type: 'custom-tool', + title: 'Legacy Tool', + toolId: 'custom-myFunction', + schema: { function: { name: 'myFunction' } }, + code: 'return true', + }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(false) + }) + + it.concurrent('does not false-positive on legacy tools when checking for database tool', () => { + const selectedTools: StoredTool[] = [ + { + type: 'custom-tool', + title: 'Legacy Tool', + schema: { function: { name: 'sameName' } }, + code: 'return true', + }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'db-tool-1')).toBe(false) + }) + }) + + describe('multiple custom tools', () => { + it.concurrent('correctly identifies first of multiple custom tools', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', customToolId: 'custom-1' }, + { type: 'custom-tool', customToolId: 'custom-2' }, + { type: 'custom-tool', customToolId: 'custom-3' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-1')).toBe(true) + }) + + it.concurrent('correctly identifies middle custom tool', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', customToolId: 'custom-1' }, + { type: 'custom-tool', customToolId: 'custom-2' }, + { type: 'custom-tool', customToolId: 'custom-3' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-2')).toBe(true) + }) + + it.concurrent('correctly identifies last custom tool', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', customToolId: 'custom-1' }, + { type: 'custom-tool', customToolId: 'custom-2' }, + { type: 'custom-tool', customToolId: 'custom-3' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-3')).toBe(true) + }) + + it.concurrent('returns false for non-existent custom tool among many', () => { + const selectedTools: StoredTool[] = [ + { type: 'custom-tool', customToolId: 'custom-1' }, + { type: 'custom-tool', customToolId: 'custom-2' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-999')).toBe(false) + }) + }) + + describe('mixed tool types', () => { + it.concurrent('correctly identifies custom tool in mixed list', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-tool-1', title: 'MCP Tool' }, + { type: 'custom-tool', customToolId: 'custom-tool-123' }, + { type: 'http_request', toolId: 'http_request' }, + { type: 'workflow_input', toolId: 'workflow_executor' }, + { type: 'custom-tool', title: 'Legacy', schema: {}, code: '' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-tool-123')).toBe(true) + }) + + it.concurrent('does not confuse MCP toolId with custom customToolId', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'shared-id-123', title: 'MCP Tool' }, + { type: 'custom-tool', customToolId: 'different-id' }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'shared-id-123')).toBe(false) + }) + }) +}) + +describe('isWorkflowAlreadySelected', () => { + describe('basic functionality', () => { + it.concurrent('returns false when selectedTools is empty', () => { + expect(isWorkflowAlreadySelected([], 'workflow-123')).toBe(false) + }) + + it.concurrent('returns false when workflow is not in selectedTools', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'different-workflow' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-123')).toBe(false) + }) + + it.concurrent('returns true when workflow is already selected', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-123' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-123')).toBe(true) + }) + + it.concurrent('returns true when workflow is one of many selected tools', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'mcp-1', title: 'MCP Tool' }, + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-123' }, + }, + { type: 'custom-tool', customToolId: 'custom-1' }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-123')).toBe(true) + }) + }) + + describe('type discrimination', () => { + it.concurrent('does not match non-workflow_input tools', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'workflow-123', params: { workflowId: 'workflow-123' } }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-123')).toBe(false) + }) + + it.concurrent('does not match workflow_input without params', () => { + const selectedTools: StoredTool[] = [{ type: 'workflow_input', toolId: 'workflow_executor' }] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-123')).toBe(false) + }) + + it.concurrent('does not match workflow_input with different workflowId in params', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'other-workflow' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-123')).toBe(false) + }) + }) + + describe('multiple workflows', () => { + it.concurrent('allows different workflows to be selected', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-a' }, + }, + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-b' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-a')).toBe(true) + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-b')).toBe(true) + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-c')).toBe(false) + }) + + it.concurrent('correctly identifies specific workflow among many', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-1' }, + }, + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-2' }, + }, + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-3' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-2')).toBe(true) + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-999')).toBe(false) + }) + }) +}) + +describe('duplicate prevention integration scenarios', () => { + describe('add then try to re-add', () => { + it.concurrent('prevents re-adding the same MCP tool', () => { + const selectedTools: StoredTool[] = [ + { + type: 'mcp', + toolId: 'planetscale-query', + title: 'PlanetScale Query', + params: { serverId: 'server-1' }, + }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'planetscale-query')).toBe(true) + }) + + it.concurrent('prevents re-adding the same custom tool', () => { + const selectedTools: StoredTool[] = [ + { + type: 'custom-tool', + customToolId: 'my-custom-tool-uuid', + usageControl: 'auto', + }, + ] + expect(isCustomToolAlreadySelected(selectedTools, 'my-custom-tool-uuid')).toBe(true) + }) + + it.concurrent('prevents re-adding the same workflow', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'my-workflow-uuid' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'my-workflow-uuid')).toBe(true) + }) + }) + + describe('remove then re-add', () => { + it.concurrent('allows re-adding MCP tool after removal', () => { + const selectedToolsAfterRemoval: StoredTool[] = [] + expect(isMcpToolAlreadySelected(selectedToolsAfterRemoval, 'planetscale-query')).toBe(false) + }) + + it.concurrent('allows re-adding custom tool after removal', () => { + const selectedToolsAfterRemoval: StoredTool[] = [ + { type: 'mcp', toolId: 'some-other-tool', title: 'Other' }, + ] + expect(isCustomToolAlreadySelected(selectedToolsAfterRemoval, 'my-custom-tool-uuid')).toBe( + false + ) + }) + + it.concurrent('allows re-adding workflow after removal', () => { + const selectedToolsAfterRemoval: StoredTool[] = [ + { type: 'mcp', toolId: 'some-tool', title: 'Other' }, + ] + expect(isWorkflowAlreadySelected(selectedToolsAfterRemoval, 'my-workflow-uuid')).toBe(false) + }) + }) + + describe('different tools with similar names', () => { + it.concurrent('allows adding different MCP tools from same server', () => { + const selectedTools: StoredTool[] = [ + { type: 'mcp', toolId: 'server1-tool-a', title: 'Tool A', params: { serverId: 'server1' } }, + ] + expect(isMcpToolAlreadySelected(selectedTools, 'server1-tool-b')).toBe(false) + }) + + it.concurrent('allows adding different custom tools', () => { + const selectedTools: StoredTool[] = [{ type: 'custom-tool', customToolId: 'custom-a' }] + expect(isCustomToolAlreadySelected(selectedTools, 'custom-b')).toBe(false) + }) + + it.concurrent('allows adding different workflows', () => { + const selectedTools: StoredTool[] = [ + { + type: 'workflow_input', + toolId: 'workflow_executor', + params: { workflowId: 'workflow-a' }, + }, + ] + expect(isWorkflowAlreadySelected(selectedTools, 'workflow-b')).toBe(false) + }) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx index c52e247ff..cd2f342a3 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx @@ -1226,6 +1226,40 @@ export const ToolInput = memo(function ToolInput({ return selectedTools.some((tool) => tool.toolId === toolId) } + /** + * Checks if an MCP tool is already selected. + * + * @param mcpToolId - The MCP tool identifier to check + * @returns `true` if the MCP tool is already selected + */ + const isMcpToolAlreadySelected = (mcpToolId: string): boolean => { + return selectedTools.some((tool) => tool.type === 'mcp' && tool.toolId === mcpToolId) + } + + /** + * Checks if a custom tool is already selected. + * + * @param customToolId - The custom tool identifier to check + * @returns `true` if the custom tool is already selected + */ + const isCustomToolAlreadySelected = (customToolId: string): boolean => { + return selectedTools.some( + (tool) => tool.type === 'custom-tool' && tool.customToolId === customToolId + ) + } + + /** + * Checks if a workflow is already selected. + * + * @param workflowId - The workflow identifier to check + * @returns `true` if the workflow is already selected + */ + const isWorkflowAlreadySelected = (workflowId: string): boolean => { + return selectedTools.some( + (tool) => tool.type === 'workflow_input' && tool.params?.workflowId === workflowId + ) + } + /** * Checks if a block supports multiple operations. * @@ -1745,24 +1779,29 @@ export const ToolInput = memo(function ToolInput({ if (!permissionConfig.disableCustomTools && customTools.length > 0) { groups.push({ section: 'Custom Tools', - items: customTools.map((customTool) => ({ - label: customTool.title, - value: `custom-${customTool.id}`, - iconElement: createToolIcon('#3B82F6', WrenchIcon), - onSelect: () => { - const newTool: StoredTool = { - type: 'custom-tool', - customToolId: customTool.id, - usageControl: 'auto', - isExpanded: true, - } - setStoreValue([ - ...selectedTools.map((tool) => ({ ...tool, isExpanded: false })), - newTool, - ]) - setOpen(false) - }, - })), + items: customTools.map((customTool) => { + const alreadySelected = isCustomToolAlreadySelected(customTool.id) + return { + label: customTool.title, + value: `custom-${customTool.id}`, + iconElement: createToolIcon('#3B82F6', WrenchIcon), + disabled: isPreview || alreadySelected, + onSelect: () => { + if (alreadySelected) return + const newTool: StoredTool = { + type: 'custom-tool', + customToolId: customTool.id, + usageControl: 'auto', + isExpanded: true, + } + setStoreValue([ + ...selectedTools.map((tool) => ({ ...tool, isExpanded: false })), + newTool, + ]) + setOpen(false) + }, + } + }), }) } @@ -1772,11 +1811,13 @@ export const ToolInput = memo(function ToolInput({ section: 'MCP Tools', items: availableMcpTools.map((mcpTool) => { const server = mcpServers.find((s) => s.id === mcpTool.serverId) + const alreadySelected = isMcpToolAlreadySelected(mcpTool.id) return { label: mcpTool.name, value: `mcp-${mcpTool.id}`, iconElement: createToolIcon(mcpTool.bgColor || '#6366F1', mcpTool.icon || McpIcon), onSelect: () => { + if (alreadySelected) return const newTool: StoredTool = { type: 'mcp', title: mcpTool.name, @@ -1796,7 +1837,7 @@ export const ToolInput = memo(function ToolInput({ } handleMcpToolSelect(newTool, true) }, - disabled: isPreview || disabled, + disabled: isPreview || disabled || alreadySelected, } }), }) @@ -1810,12 +1851,17 @@ export const ToolInput = memo(function ToolInput({ if (builtInTools.length > 0) { groups.push({ section: 'Built-in Tools', - items: builtInTools.map((block) => ({ - label: block.name, - value: `builtin-${block.type}`, - iconElement: createToolIcon(block.bgColor, block.icon), - onSelect: () => handleSelectTool(block), - })), + items: builtInTools.map((block) => { + const toolId = getToolIdForOperation(block.type, undefined) + const alreadySelected = toolId ? isToolAlreadySelected(toolId, block.type) : false + return { + label: block.name, + value: `builtin-${block.type}`, + iconElement: createToolIcon(block.bgColor, block.icon), + disabled: isPreview || alreadySelected, + onSelect: () => handleSelectTool(block), + } + }), }) } @@ -1823,12 +1869,17 @@ export const ToolInput = memo(function ToolInput({ if (integrations.length > 0) { groups.push({ section: 'Integrations', - items: integrations.map((block) => ({ - label: block.name, - value: `builtin-${block.type}`, - iconElement: createToolIcon(block.bgColor, block.icon), - onSelect: () => handleSelectTool(block), - })), + items: integrations.map((block) => { + const toolId = getToolIdForOperation(block.type, undefined) + const alreadySelected = toolId ? isToolAlreadySelected(toolId, block.type) : false + return { + label: block.name, + value: `builtin-${block.type}`, + iconElement: createToolIcon(block.bgColor, block.icon), + disabled: isPreview || alreadySelected, + onSelect: () => handleSelectTool(block), + } + }), }) } @@ -1836,29 +1887,33 @@ export const ToolInput = memo(function ToolInput({ if (availableWorkflows.length > 0) { groups.push({ section: 'Workflows', - items: availableWorkflows.map((workflow) => ({ - label: workflow.name, - value: `workflow-${workflow.id}`, - iconElement: createToolIcon('#6366F1', WorkflowIcon), - onSelect: () => { - const newTool: StoredTool = { - type: 'workflow_input', - title: 'Workflow', - toolId: 'workflow_executor', - params: { - workflowId: workflow.id, - }, - isExpanded: true, - usageControl: 'auto', - } - setStoreValue([ - ...selectedTools.map((tool) => ({ ...tool, isExpanded: false })), - newTool, - ]) - setOpen(false) - }, - disabled: isPreview || disabled, - })), + items: availableWorkflows.map((workflow) => { + const alreadySelected = isWorkflowAlreadySelected(workflow.id) + return { + label: workflow.name, + value: `workflow-${workflow.id}`, + iconElement: createToolIcon('#6366F1', WorkflowIcon), + onSelect: () => { + if (alreadySelected) return + const newTool: StoredTool = { + type: 'workflow_input', + title: 'Workflow', + toolId: 'workflow_executor', + params: { + workflowId: workflow.id, + }, + isExpanded: true, + usageControl: 'auto', + } + setStoreValue([ + ...selectedTools.map((tool) => ({ ...tool, isExpanded: false })), + newTool, + ]) + setOpen(false) + }, + disabled: isPreview || disabled || alreadySelected, + } + }), }) } @@ -1877,6 +1932,11 @@ export const ToolInput = memo(function ToolInput({ permissionConfig.disableCustomTools, permissionConfig.disableMcpTools, availableWorkflows, + getToolIdForOperation, + isToolAlreadySelected, + isMcpToolAlreadySelected, + isCustomToolAlreadySelected, + isWorkflowAlreadySelected, ]) const toolRequiresOAuth = (toolId: string): boolean => { diff --git a/apps/sim/hooks/use-undo-redo.ts b/apps/sim/hooks/use-undo-redo.ts index 06cd9326d..3ce564e06 100644 --- a/apps/sim/hooks/use-undo-redo.ts +++ b/apps/sim/hooks/use-undo-redo.ts @@ -29,7 +29,6 @@ import { useUndoRedoStore, } from '@/stores/undo-redo' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' -import { useSubBlockStore } from '@/stores/workflows/subblock/store' import { useWorkflowStore } from '@/stores/workflows/workflow/store' import type { BlockState } from '@/stores/workflows/workflow/types' @@ -504,47 +503,9 @@ export function useUndoRedo() { userId, }) - blocksToAdd.forEach((block) => { - useWorkflowStore - .getState() - .addBlock( - block.id, - block.type, - block.name, - block.position, - block.data, - block.data?.parentId, - block.data?.extent, - { - enabled: block.enabled, - horizontalHandles: block.horizontalHandles, - advancedMode: block.advancedMode, - triggerMode: block.triggerMode, - height: block.height, - } - ) - }) - - if (subBlockValues && Object.keys(subBlockValues).length > 0) { - useSubBlockStore.setState((state) => ({ - workflowValues: { - ...state.workflowValues, - [activeWorkflowId]: { - ...state.workflowValues[activeWorkflowId], - ...subBlockValues, - }, - }, - })) - } - - if (edgeSnapshots && edgeSnapshots.length > 0) { - const edgesToAdd = edgeSnapshots.filter( - (edge) => !useWorkflowStore.getState().edges.find((e) => e.id === edge.id) - ) - if (edgesToAdd.length > 0) { - useWorkflowStore.getState().batchAddEdges(edgesToAdd) - } - } + useWorkflowStore + .getState() + .batchAddBlocks(blocksToAdd, edgeSnapshots || [], subBlockValues || {}) break } case UNDO_REDO_OPERATIONS.BATCH_REMOVE_EDGES: { @@ -1085,47 +1046,9 @@ export function useUndoRedo() { userId, }) - blocksToAdd.forEach((block) => { - useWorkflowStore - .getState() - .addBlock( - block.id, - block.type, - block.name, - block.position, - block.data, - block.data?.parentId, - block.data?.extent, - { - enabled: block.enabled, - horizontalHandles: block.horizontalHandles, - advancedMode: block.advancedMode, - triggerMode: block.triggerMode, - height: block.height, - } - ) - }) - - if (subBlockValues && Object.keys(subBlockValues).length > 0) { - useSubBlockStore.setState((state) => ({ - workflowValues: { - ...state.workflowValues, - [activeWorkflowId]: { - ...state.workflowValues[activeWorkflowId], - ...subBlockValues, - }, - }, - })) - } - - if (edgeSnapshots && edgeSnapshots.length > 0) { - const edgesToAdd = edgeSnapshots.filter( - (edge) => !useWorkflowStore.getState().edges.find((e) => e.id === edge.id) - ) - if (edgesToAdd.length > 0) { - useWorkflowStore.getState().batchAddEdges(edgesToAdd) - } - } + useWorkflowStore + .getState() + .batchAddBlocks(blocksToAdd, edgeSnapshots || [], subBlockValues || {}) break } case UNDO_REDO_OPERATIONS.BATCH_REMOVE_BLOCKS: { diff --git a/apps/sim/stores/workflows/workflow/store.test.ts b/apps/sim/stores/workflows/workflow/store.test.ts index 1ed122c23..a220db72a 100644 --- a/apps/sim/stores/workflows/workflow/store.test.ts +++ b/apps/sim/stores/workflows/workflow/store.test.ts @@ -26,6 +26,49 @@ import { useWorkflowRegistry } from '@/stores/workflows/registry/store' import { useSubBlockStore } from '@/stores/workflows/subblock/store' import { useWorkflowStore } from '@/stores/workflows/workflow/store' +/** + * Helper function to add a single block using batchAddBlocks. + * Provides a simpler interface for tests. + */ +function addBlock( + id: string, + type: string, + name: string, + position: { x: number; y: number }, + data?: Record, + parentId?: string, + extent?: 'parent', + blockProperties?: { + enabled?: boolean + horizontalHandles?: boolean + advancedMode?: boolean + triggerMode?: boolean + height?: number + } +) { + const blockData = { + ...data, + ...(parentId && { parentId, extent: extent || 'parent' }), + } + + useWorkflowStore.getState().batchAddBlocks([ + { + id, + type, + name, + position, + subBlocks: {}, + outputs: {}, + enabled: blockProperties?.enabled ?? true, + horizontalHandles: blockProperties?.horizontalHandles ?? true, + advancedMode: blockProperties?.advancedMode ?? false, + triggerMode: blockProperties?.triggerMode ?? false, + height: blockProperties?.height ?? 0, + data: blockData, + }, + ]) +} + describe('workflow store', () => { beforeEach(() => { const localStorageMock = createMockStorage() @@ -39,10 +82,8 @@ describe('workflow store', () => { }) }) - describe('addBlock', () => { + describe('batchAddBlocks (via addBlock helper)', () => { it('should add a block with correct default properties', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock('agent-1', 'agent', 'My Agent', { x: 100, y: 200 }) const { blocks } = useWorkflowStore.getState() @@ -53,8 +94,6 @@ describe('workflow store', () => { }) it('should add a block with parent relationship for containers', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock('loop-1', 'loop', 'My Loop', { x: 0, y: 0 }, { loopType: 'for', count: 3 }) addBlock( 'child-1', @@ -73,8 +112,6 @@ describe('workflow store', () => { }) it('should add multiple blocks correctly', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'agent', 'Agent', { x: 200, y: 0 }) addBlock('block-3', 'function', 'Function', { x: 400, y: 0 }) @@ -87,8 +124,6 @@ describe('workflow store', () => { }) it('should create a block with default properties when no blockProperties provided', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock('agent1', 'agent', 'Test Agent', { x: 100, y: 200 }) const state = useWorkflowStore.getState() @@ -105,8 +140,6 @@ describe('workflow store', () => { }) it('should create a block with custom blockProperties for regular blocks', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock( 'agent1', 'agent', @@ -134,8 +167,6 @@ describe('workflow store', () => { }) it('should create a loop block with custom blockProperties', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock( 'loop1', 'loop', @@ -163,8 +194,6 @@ describe('workflow store', () => { }) it('should create a parallel block with custom blockProperties', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock( 'parallel1', 'parallel', @@ -192,8 +221,6 @@ describe('workflow store', () => { }) it('should handle partial blockProperties (only some properties provided)', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock( 'agent1', 'agent', @@ -216,8 +243,6 @@ describe('workflow store', () => { }) it('should handle blockProperties with parent relationships', () => { - const { addBlock } = useWorkflowStore.getState() - addBlock('loop1', 'loop', 'Parent Loop', { x: 0, y: 0 }) addBlock( @@ -249,7 +274,7 @@ describe('workflow store', () => { describe('batchRemoveBlocks', () => { it('should remove a block', () => { - const { addBlock, batchRemoveBlocks } = useWorkflowStore.getState() + const { batchRemoveBlocks } = useWorkflowStore.getState() addBlock('block-1', 'function', 'Test', { x: 0, y: 0 }) batchRemoveBlocks(['block-1']) @@ -259,7 +284,7 @@ describe('workflow store', () => { }) it('should remove connected edges when block is removed', () => { - const { addBlock, batchAddEdges, batchRemoveBlocks } = useWorkflowStore.getState() + const { batchAddEdges, batchRemoveBlocks } = useWorkflowStore.getState() addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'function', 'Middle', { x: 200, y: 0 }) @@ -286,7 +311,7 @@ describe('workflow store', () => { describe('batchAddEdges', () => { it('should add an edge between two blocks', () => { - const { addBlock, batchAddEdges } = useWorkflowStore.getState() + const { batchAddEdges } = useWorkflowStore.getState() addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'function', 'End', { x: 200, y: 0 }) @@ -298,7 +323,7 @@ describe('workflow store', () => { }) it('should not add duplicate connections', () => { - const { addBlock, batchAddEdges } = useWorkflowStore.getState() + const { batchAddEdges } = useWorkflowStore.getState() addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'function', 'End', { x: 200, y: 0 }) @@ -313,7 +338,7 @@ describe('workflow store', () => { describe('batchRemoveEdges', () => { it('should remove an edge by id', () => { - const { addBlock, batchAddEdges, batchRemoveEdges } = useWorkflowStore.getState() + const { batchAddEdges, batchRemoveEdges } = useWorkflowStore.getState() addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'function', 'End', { x: 200, y: 0 }) @@ -335,7 +360,7 @@ describe('workflow store', () => { describe('clear', () => { it('should clear all blocks and edges', () => { - const { addBlock, batchAddEdges, clear } = useWorkflowStore.getState() + const { batchAddEdges, clear } = useWorkflowStore.getState() addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'function', 'End', { x: 200, y: 0 }) @@ -351,7 +376,7 @@ describe('workflow store', () => { describe('batchToggleEnabled', () => { it('should toggle block enabled state', () => { - const { addBlock, batchToggleEnabled } = useWorkflowStore.getState() + const { batchToggleEnabled } = useWorkflowStore.getState() addBlock('block-1', 'function', 'Test', { x: 0, y: 0 }) @@ -367,7 +392,7 @@ describe('workflow store', () => { describe('duplicateBlock', () => { it('should duplicate a block', () => { - const { addBlock, duplicateBlock } = useWorkflowStore.getState() + const { duplicateBlock } = useWorkflowStore.getState() addBlock('original', 'agent', 'Original Agent', { x: 0, y: 0 }) @@ -391,7 +416,7 @@ describe('workflow store', () => { describe('batchUpdatePositions', () => { it('should update block position', () => { - const { addBlock, batchUpdatePositions } = useWorkflowStore.getState() + const { batchUpdatePositions } = useWorkflowStore.getState() addBlock('block-1', 'function', 'Test', { x: 0, y: 0 }) @@ -404,7 +429,7 @@ describe('workflow store', () => { describe('loop management', () => { it('should regenerate loops when updateLoopCount is called', () => { - const { addBlock, updateLoopCount } = useWorkflowStore.getState() + const { updateLoopCount } = useWorkflowStore.getState() addBlock( 'loop1', @@ -428,7 +453,7 @@ describe('workflow store', () => { }) it('should regenerate loops when updateLoopType is called', () => { - const { addBlock, updateLoopType } = useWorkflowStore.getState() + const { updateLoopType } = useWorkflowStore.getState() addBlock( 'loop1', @@ -453,7 +478,7 @@ describe('workflow store', () => { }) it('should regenerate loops when updateLoopCollection is called', () => { - const { addBlock, updateLoopCollection } = useWorkflowStore.getState() + const { updateLoopCollection } = useWorkflowStore.getState() addBlock( 'loop1', @@ -476,7 +501,7 @@ describe('workflow store', () => { }) it('should clamp loop count between 1 and 1000', () => { - const { addBlock, updateLoopCount } = useWorkflowStore.getState() + const { updateLoopCount } = useWorkflowStore.getState() addBlock( 'loop1', @@ -502,7 +527,7 @@ describe('workflow store', () => { describe('parallel management', () => { it('should regenerate parallels when updateParallelCount is called', () => { - const { addBlock, updateParallelCount } = useWorkflowStore.getState() + const { updateParallelCount } = useWorkflowStore.getState() addBlock( 'parallel1', @@ -525,7 +550,7 @@ describe('workflow store', () => { }) it('should regenerate parallels when updateParallelCollection is called', () => { - const { addBlock, updateParallelCollection } = useWorkflowStore.getState() + const { updateParallelCollection } = useWorkflowStore.getState() addBlock( 'parallel1', @@ -552,7 +577,7 @@ describe('workflow store', () => { }) it('should clamp parallel count between 1 and 20', () => { - const { addBlock, updateParallelCount } = useWorkflowStore.getState() + const { updateParallelCount } = useWorkflowStore.getState() addBlock( 'parallel1', @@ -575,7 +600,7 @@ describe('workflow store', () => { }) it('should regenerate parallels when updateParallelType is called', () => { - const { addBlock, updateParallelType } = useWorkflowStore.getState() + const { updateParallelType } = useWorkflowStore.getState() addBlock( 'parallel1', @@ -601,7 +626,7 @@ describe('workflow store', () => { describe('mode switching', () => { it('should toggle advanced mode on a block', () => { - const { addBlock, toggleBlockAdvancedMode } = useWorkflowStore.getState() + const { toggleBlockAdvancedMode } = useWorkflowStore.getState() addBlock('agent1', 'agent', 'Test Agent', { x: 0, y: 0 }) @@ -618,7 +643,7 @@ describe('workflow store', () => { }) it('should preserve systemPrompt and userPrompt when switching modes', () => { - const { addBlock, toggleBlockAdvancedMode } = useWorkflowStore.getState() + const { toggleBlockAdvancedMode } = useWorkflowStore.getState() const { setState: setSubBlockState } = useSubBlockStore useWorkflowRegistry.setState({ activeWorkflowId: 'test-workflow' }) addBlock('agent1', 'agent', 'Test Agent', { x: 0, y: 0 }) @@ -651,7 +676,7 @@ describe('workflow store', () => { }) it('should preserve memories when switching from advanced to basic mode', () => { - const { addBlock, toggleBlockAdvancedMode } = useWorkflowStore.getState() + const { toggleBlockAdvancedMode } = useWorkflowStore.getState() const { setState: setSubBlockState } = useSubBlockStore useWorkflowRegistry.setState({ activeWorkflowId: 'test-workflow' }) @@ -691,7 +716,7 @@ describe('workflow store', () => { }) it('should handle mode switching when no subblock values exist', () => { - const { addBlock, toggleBlockAdvancedMode } = useWorkflowStore.getState() + const { toggleBlockAdvancedMode } = useWorkflowStore.getState() useWorkflowRegistry.setState({ activeWorkflowId: 'test-workflow' }) @@ -753,7 +778,7 @@ describe('workflow store', () => { describe('replaceWorkflowState', () => { it('should replace entire workflow state', () => { - const { addBlock, replaceWorkflowState } = useWorkflowStore.getState() + const { replaceWorkflowState } = useWorkflowStore.getState() addBlock('old-1', 'function', 'Old', { x: 0, y: 0 }) @@ -769,7 +794,7 @@ describe('workflow store', () => { describe('getWorkflowState', () => { it('should return current workflow state', () => { - const { addBlock, getWorkflowState } = useWorkflowStore.getState() + const { getWorkflowState } = useWorkflowStore.getState() addBlock('block-1', 'starter', 'Start', { x: 0, y: 0 }) addBlock('block-2', 'function', 'End', { x: 200, y: 0 }) @@ -782,6 +807,343 @@ describe('workflow store', () => { }) }) + describe('loop/parallel regeneration optimization', () => { + it('should NOT regenerate loops when adding a regular block without parentId', () => { + // Add a loop first + addBlock('loop-1', 'loop', 'Loop 1', { x: 0, y: 0 }, { loopType: 'for', count: 5 }) + + const stateAfterLoop = useWorkflowStore.getState() + const loopsAfterLoop = stateAfterLoop.loops + + // Add a regular block (no parentId) + addBlock('agent-1', 'agent', 'Agent 1', { x: 200, y: 0 }) + + const stateAfterAgent = useWorkflowStore.getState() + + // Loops should be unchanged (same content) + expect(Object.keys(stateAfterAgent.loops)).toEqual(Object.keys(loopsAfterLoop)) + expect(stateAfterAgent.loops['loop-1'].nodes).toEqual(loopsAfterLoop['loop-1'].nodes) + }) + + it('should regenerate loops when adding a child to a loop', () => { + // Add a loop + addBlock('loop-1', 'loop', 'Loop 1', { x: 0, y: 0 }, { loopType: 'for', count: 5 }) + + const stateAfterLoop = useWorkflowStore.getState() + expect(stateAfterLoop.loops['loop-1'].nodes).toEqual([]) + + // Add a child block to the loop + addBlock( + 'child-1', + 'function', + 'Child 1', + { x: 50, y: 50 }, + { parentId: 'loop-1' }, + 'loop-1', + 'parent' + ) + + const stateAfterChild = useWorkflowStore.getState() + + // Loop should now include the child + expect(stateAfterChild.loops['loop-1'].nodes).toContain('child-1') + }) + + it('should NOT regenerate parallels when adding a child to a loop', () => { + // Add both a loop and a parallel + addBlock('loop-1', 'loop', 'Loop 1', { x: 0, y: 0 }, { loopType: 'for', count: 5 }) + addBlock('parallel-1', 'parallel', 'Parallel 1', { x: 300, y: 0 }, { count: 3 }) + + const stateAfterContainers = useWorkflowStore.getState() + const parallelsAfterContainers = stateAfterContainers.parallels + + // Add a child to the loop (not the parallel) + addBlock( + 'child-1', + 'function', + 'Child 1', + { x: 50, y: 50 }, + { parentId: 'loop-1' }, + 'loop-1', + 'parent' + ) + + const stateAfterChild = useWorkflowStore.getState() + + // Parallels should be unchanged + expect(stateAfterChild.parallels['parallel-1'].nodes).toEqual( + parallelsAfterContainers['parallel-1'].nodes + ) + }) + + it('should regenerate parallels when adding a child to a parallel', () => { + // Add a parallel + addBlock('parallel-1', 'parallel', 'Parallel 1', { x: 0, y: 0 }, { count: 3 }) + + const stateAfterParallel = useWorkflowStore.getState() + expect(stateAfterParallel.parallels['parallel-1'].nodes).toEqual([]) + + // Add a child block to the parallel + addBlock( + 'child-1', + 'function', + 'Child 1', + { x: 50, y: 50 }, + { parentId: 'parallel-1' }, + 'parallel-1', + 'parent' + ) + + const stateAfterChild = useWorkflowStore.getState() + + // Parallel should now include the child + expect(stateAfterChild.parallels['parallel-1'].nodes).toContain('child-1') + }) + + it('should handle adding blocks in any order and produce correct final state', () => { + // Add child BEFORE the loop (simulating undo-redo edge case) + // Note: The child's parentId points to a loop that doesn't exist yet + addBlock( + 'child-1', + 'function', + 'Child 1', + { x: 50, y: 50 }, + { parentId: 'loop-1' }, + 'loop-1', + 'parent' + ) + + // At this point, the child exists but loop doesn't + const stateAfterChild = useWorkflowStore.getState() + expect(stateAfterChild.blocks['child-1']).toBeDefined() + expect(stateAfterChild.loops['loop-1']).toBeUndefined() + + // Now add the loop + addBlock('loop-1', 'loop', 'Loop 1', { x: 0, y: 0 }, { loopType: 'for', count: 5 }) + + // Final state should be correct - loop should include the child + const finalState = useWorkflowStore.getState() + expect(finalState.loops['loop-1']).toBeDefined() + expect(finalState.loops['loop-1'].nodes).toContain('child-1') + }) + }) + + describe('batchAddBlocks optimization', () => { + it('should NOT regenerate loops/parallels when adding regular blocks', () => { + const { batchAddBlocks } = useWorkflowStore.getState() + + // Set up initial state with a loop + useWorkflowStore.setState({ + blocks: { + 'loop-1': { + id: 'loop-1', + type: 'loop', + name: 'Loop 1', + position: { x: 0, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + horizontalHandles: true, + advancedMode: false, + triggerMode: false, + height: 0, + data: { loopType: 'for', count: 5 }, + }, + }, + edges: [], + loops: { + 'loop-1': { + id: 'loop-1', + nodes: [], + iterations: 5, + loopType: 'for', + enabled: true, + }, + }, + parallels: {}, + }) + + const stateBefore = useWorkflowStore.getState() + + // Add regular blocks (no parentId, not loop/parallel type) + batchAddBlocks([ + { + id: 'agent-1', + type: 'agent', + name: 'Agent 1', + position: { x: 200, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + }, + { + id: 'function-1', + type: 'function', + name: 'Function 1', + position: { x: 400, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + }, + ]) + + const stateAfter = useWorkflowStore.getState() + + // Loops should be unchanged + expect(stateAfter.loops['loop-1'].nodes).toEqual(stateBefore.loops['loop-1'].nodes) + }) + + it('should regenerate loops when batch adding a loop block', () => { + const { batchAddBlocks } = useWorkflowStore.getState() + + batchAddBlocks([ + { + id: 'loop-1', + type: 'loop', + name: 'Loop 1', + position: { x: 0, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + data: { loopType: 'for', count: 5 }, + }, + ]) + + const state = useWorkflowStore.getState() + expect(state.loops['loop-1']).toBeDefined() + expect(state.loops['loop-1'].iterations).toBe(5) + }) + + it('should regenerate loops when batch adding a child of a loop', () => { + const { batchAddBlocks } = useWorkflowStore.getState() + + // First add a loop + batchAddBlocks([ + { + id: 'loop-1', + type: 'loop', + name: 'Loop 1', + position: { x: 0, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + data: { loopType: 'for', count: 5 }, + }, + ]) + + // Then add a child + batchAddBlocks([ + { + id: 'child-1', + type: 'function', + name: 'Child 1', + position: { x: 50, y: 50 }, + subBlocks: {}, + outputs: {}, + enabled: true, + data: { parentId: 'loop-1' }, + }, + ]) + + const state = useWorkflowStore.getState() + expect(state.loops['loop-1'].nodes).toContain('child-1') + }) + + it('should correctly handle batch adding loop and its children together', () => { + const { batchAddBlocks } = useWorkflowStore.getState() + + // Add loop and child in same batch + batchAddBlocks([ + { + id: 'loop-1', + type: 'loop', + name: 'Loop 1', + position: { x: 0, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + data: { loopType: 'for', count: 5 }, + }, + { + id: 'child-1', + type: 'function', + name: 'Child 1', + position: { x: 50, y: 50 }, + subBlocks: {}, + outputs: {}, + enabled: true, + data: { parentId: 'loop-1' }, + }, + ]) + + const state = useWorkflowStore.getState() + expect(state.loops['loop-1']).toBeDefined() + expect(state.loops['loop-1'].nodes).toContain('child-1') + }) + }) + + describe('edge operations should not affect loops/parallels', () => { + it('should preserve loops when adding edges', () => { + const { batchAddEdges } = useWorkflowStore.getState() + + // Create a loop with a child + addBlock('loop-1', 'loop', 'Loop 1', { x: 0, y: 0 }, { loopType: 'for', count: 5 }) + addBlock( + 'child-1', + 'function', + 'Child 1', + { x: 50, y: 50 }, + { parentId: 'loop-1' }, + 'loop-1', + 'parent' + ) + addBlock('external-1', 'function', 'External', { x: 300, y: 0 }) + + const stateBeforeEdge = useWorkflowStore.getState() + const loopsBeforeEdge = stateBeforeEdge.loops + + // Add an edge (should not affect loops) + batchAddEdges([{ id: 'e1', source: 'loop-1', target: 'external-1' }]) + + const stateAfterEdge = useWorkflowStore.getState() + + // Loops should be unchanged + expect(stateAfterEdge.loops['loop-1'].nodes).toEqual(loopsBeforeEdge['loop-1'].nodes) + expect(stateAfterEdge.loops['loop-1'].iterations).toEqual( + loopsBeforeEdge['loop-1'].iterations + ) + }) + + it('should preserve loops when removing edges', () => { + const { batchAddEdges, batchRemoveEdges } = useWorkflowStore.getState() + + // Create a loop with a child and an edge + addBlock('loop-1', 'loop', 'Loop 1', { x: 0, y: 0 }, { loopType: 'for', count: 5 }) + addBlock( + 'child-1', + 'function', + 'Child 1', + { x: 50, y: 50 }, + { parentId: 'loop-1' }, + 'loop-1', + 'parent' + ) + addBlock('external-1', 'function', 'External', { x: 300, y: 0 }) + batchAddEdges([{ id: 'e1', source: 'loop-1', target: 'external-1' }]) + + const stateBeforeRemove = useWorkflowStore.getState() + const loopsBeforeRemove = stateBeforeRemove.loops + + // Remove the edge + batchRemoveEdges(['e1']) + + const stateAfterRemove = useWorkflowStore.getState() + + // Loops should be unchanged + expect(stateAfterRemove.loops['loop-1'].nodes).toEqual(loopsBeforeRemove['loop-1'].nodes) + }) + }) + describe('updateBlockName', () => { beforeEach(() => { useWorkflowStore.setState({ @@ -791,8 +1153,6 @@ describe('workflow store', () => { parallels: {}, }) - const { addBlock } = useWorkflowStore.getState() - addBlock('block1', 'agent', 'Column AD', { x: 0, y: 0 }) addBlock('block2', 'function', 'Employee Length', { x: 100, y: 0 }) addBlock('block3', 'starter', 'Start', { x: 200, y: 0 }) diff --git a/apps/sim/stores/workflows/workflow/store.ts b/apps/sim/stores/workflows/workflow/store.ts index 00eeac9b8..487deb39b 100644 --- a/apps/sim/stores/workflows/workflow/store.ts +++ b/apps/sim/stores/workflows/workflow/store.ts @@ -3,8 +3,6 @@ import type { Edge } from 'reactflow' import { create } from 'zustand' import { devtools } from 'zustand/middleware' import { DEFAULT_DUPLICATE_OFFSET } from '@/lib/workflows/autolayout/constants' -import { getBlockOutputs } from '@/lib/workflows/blocks/block-outputs' -import { getBlock } from '@/blocks' import type { SubBlockConfig } from '@/blocks/types' import { normalizeName, RESERVED_BLOCK_NAMES } from '@/executor/constants' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' @@ -114,135 +112,6 @@ export const useWorkflowStore = create()( set({ needsRedeployment }) }, - addBlock: ( - id: string, - type: string, - name: string, - position: Position, - data?: Record, - parentId?: string, - extent?: 'parent', - blockProperties?: { - enabled?: boolean - horizontalHandles?: boolean - advancedMode?: boolean - triggerMode?: boolean - height?: number - } - ) => { - const blockConfig = getBlock(type) - // For custom nodes like loop and parallel that don't use BlockConfig - if (!blockConfig && (type === 'loop' || type === 'parallel')) { - // Merge parentId and extent into data if provided - const nodeData = { - ...data, - ...(parentId && { parentId, extent: extent || 'parent' }), - } - - const newState = { - blocks: { - ...get().blocks, - [id]: { - id, - type, - name, - position, - subBlocks: {}, - outputs: {}, - enabled: blockProperties?.enabled ?? true, - horizontalHandles: blockProperties?.horizontalHandles ?? true, - advancedMode: blockProperties?.advancedMode ?? false, - triggerMode: blockProperties?.triggerMode ?? false, - height: blockProperties?.height ?? 0, - data: nodeData, - }, - }, - edges: [...get().edges], - loops: get().generateLoopBlocks(), - parallels: get().generateParallelBlocks(), - } - - set(newState) - get().updateLastSaved() - return - } - - if (!blockConfig) return - - // Merge parentId and extent into data for regular blocks - const nodeData = { - ...data, - ...(parentId && { parentId, extent: extent || 'parent' }), - } - - const subBlocks: Record = {} - const subBlockStore = useSubBlockStore.getState() - const activeWorkflowId = useWorkflowRegistry.getState().activeWorkflowId - - blockConfig.subBlocks.forEach((subBlock) => { - const subBlockId = subBlock.id - const initialValue = resolveInitialSubblockValue(subBlock) - const normalizedValue = - initialValue !== undefined && initialValue !== null ? initialValue : null - - subBlocks[subBlockId] = { - id: subBlockId, - type: subBlock.type, - value: normalizedValue as SubBlockState['value'], - } - - if (activeWorkflowId) { - try { - const valueToStore = - initialValue !== undefined ? cloneInitialSubblockValue(initialValue) : null - subBlockStore.setValue(id, subBlockId, valueToStore) - } catch (error) { - logger.warn('Failed to seed sub-block store value during block creation', { - blockId: id, - subBlockId, - error: error instanceof Error ? error.message : String(error), - }) - } - } else { - logger.warn('Cannot seed sub-block store value: activeWorkflowId not available', { - blockId: id, - subBlockId, - }) - } - }) - - // Get outputs based on trigger mode - const triggerMode = blockProperties?.triggerMode ?? false - const outputs = getBlockOutputs(type, subBlocks, triggerMode) - - const newState = { - blocks: { - ...get().blocks, - [id]: { - id, - type, - name, - position, - subBlocks, - outputs, - enabled: blockProperties?.enabled ?? true, - horizontalHandles: blockProperties?.horizontalHandles ?? true, - advancedMode: blockProperties?.advancedMode ?? false, - triggerMode: triggerMode, - height: blockProperties?.height ?? 0, - layout: {}, - data: nodeData, - }, - }, - edges: [...get().edges], - loops: get().generateLoopBlocks(), - parallels: get().generateParallelBlocks(), - } - - set(newState) - get().updateLastSaved() - }, - updateNodeDimensions: (id: string, dimensions: { width: number; height: number }) => { set((state) => { const block = state.blocks[id] @@ -386,11 +255,27 @@ export const useWorkflowStore = create()( } } + // Only regenerate loops/parallels if we're adding blocks that affect them: + // - Adding a loop/parallel container block + // - Adding a block as a child of a loop/parallel (has parentId pointing to one) + const needsLoopRegeneration = blocks.some( + (block) => + block.type === 'loop' || + (block.data?.parentId && newBlocks[block.data.parentId]?.type === 'loop') + ) + const needsParallelRegeneration = blocks.some( + (block) => + block.type === 'parallel' || + (block.data?.parentId && newBlocks[block.data.parentId]?.type === 'parallel') + ) + set({ blocks: newBlocks, edges: newEdges, - loops: generateLoopBlocks(newBlocks), - parallels: generateParallelBlocks(newBlocks), + loops: needsLoopRegeneration ? generateLoopBlocks(newBlocks) : { ...get().loops }, + parallels: needsParallelRegeneration + ? generateParallelBlocks(newBlocks) + : { ...get().parallels }, }) if (subBlockValues && Object.keys(subBlockValues).length > 0) { @@ -529,8 +414,9 @@ export const useWorkflowStore = create()( set({ blocks: { ...blocks }, edges: newEdges, - loops: generateLoopBlocks(blocks), - parallels: generateParallelBlocks(blocks), + // Edges don't affect loop/parallel structure (determined by parentId), skip regeneration + loops: { ...get().loops }, + parallels: { ...get().parallels }, }) get().updateLastSaved() @@ -544,8 +430,9 @@ export const useWorkflowStore = create()( set({ blocks: { ...blocks }, edges: newEdges, - loops: generateLoopBlocks(blocks), - parallels: generateParallelBlocks(blocks), + // Edges don't affect loop/parallel structure (determined by parentId), skip regeneration + loops: { ...get().loops }, + parallels: { ...get().parallels }, }) get().updateLastSaved() diff --git a/apps/sim/stores/workflows/workflow/types.ts b/apps/sim/stores/workflows/workflow/types.ts index f348bf0f6..7f58d6d3e 100644 --- a/apps/sim/stores/workflows/workflow/types.ts +++ b/apps/sim/stores/workflows/workflow/types.ts @@ -175,22 +175,6 @@ export interface WorkflowState { } export interface WorkflowActions { - addBlock: ( - id: string, - type: string, - name: string, - position: Position, - data?: Record, - parentId?: string, - extent?: 'parent', - blockProperties?: { - enabled?: boolean - horizontalHandles?: boolean - advancedMode?: boolean - triggerMode?: boolean - height?: number - } - ) => void updateNodeDimensions: (id: string, dimensions: { width: number; height: number }) => void batchUpdateBlocksWithParent: ( updates: Array<{