From ad4060aa92797b51468d96931306111a2d287090 Mon Sep 17 00:00:00 2001 From: Adam Gough <77861281+aadamgough@users.noreply.github.com> Date: Mon, 26 May 2025 12:05:23 -0700 Subject: [PATCH] fit(loop-ui): fixed some UI issues, added tests for parallel and loop nodes (#421) * basic change * fixed drag * testing and UI fixes * ran bun * added greptile comments --------- Co-authored-by: Adam Gough --- .../components/loop-node/loop-node.test.tsx | 463 +++++++++++++ .../w/[id]/components/loop-node/loop-node.tsx | 13 +- .../parallel-node/parallel-node.test.tsx | 616 ++++++++++++++++++ .../parallel-node/parallel-node.tsx | 11 - .../sub-block/components/long-input.tsx | 4 + .../workflow-block/workflow-block.tsx | 15 +- apps/sim/app/w/[id]/utils.ts | 85 ++- apps/sim/app/w/[id]/workflow.tsx | 164 ++--- 8 files changed, 1198 insertions(+), 173 deletions(-) create mode 100644 apps/sim/app/w/[id]/components/loop-node/loop-node.test.tsx create mode 100644 apps/sim/app/w/[id]/components/parallel-node/parallel-node.test.tsx diff --git a/apps/sim/app/w/[id]/components/loop-node/loop-node.test.tsx b/apps/sim/app/w/[id]/components/loop-node/loop-node.test.tsx new file mode 100644 index 0000000000..9625cb5f61 --- /dev/null +++ b/apps/sim/app/w/[id]/components/loop-node/loop-node.test.tsx @@ -0,0 +1,463 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { useWorkflowStore } from '@/stores/workflows/workflow/store' +import { LoopNodeComponent } from './loop-node' + +// Mock dependencies that don't need DOM +vi.mock('@/stores/workflows/workflow/store', () => ({ + useWorkflowStore: vi.fn(), +})) + +vi.mock('@/lib/logs/console-logger', () => ({ + createLogger: vi.fn(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + })), +})) + +// Mock ReactFlow components and hooks +vi.mock('reactflow', () => ({ + Handle: ({ id, type, position }: any) => ({ id, type, position }), + Position: { + Top: 'top', + Bottom: 'bottom', + Left: 'left', + Right: 'right', + }, + useReactFlow: () => ({ + getNodes: vi.fn(() => []), + }), + NodeResizer: ({ isVisible }: any) => ({ isVisible }), + memo: (component: any) => component, +})) + +// Mock React hooks +vi.mock('react', async () => { + const actual = await vi.importActual('react') + return { + ...actual, + memo: (component: any) => component, + useMemo: (fn: any) => fn(), + useRef: () => ({ current: null }), + } +}) + +// Mock UI components +vi.mock('@/components/ui/button', () => ({ + Button: ({ children, onClick, ...props }: any) => ({ children, onClick, ...props }), +})) + +vi.mock('@/components/ui/card', () => ({ + Card: ({ children, ...props }: any) => ({ children, ...props }), +})) + +vi.mock('@/components/icons', () => ({ + StartIcon: ({ className }: any) => ({ className }), +})) + +vi.mock('@/lib/utils', () => ({ + cn: (...classes: any[]) => classes.filter(Boolean).join(' '), +})) + +// Mock the LoopBadges component +vi.mock('./components/loop-badges', () => ({ + LoopBadges: ({ loopId }: any) => ({ loopId }), +})) + +describe('LoopNodeComponent', () => { + const mockRemoveBlock = vi.fn() + const mockGetNodes = vi.fn() + const defaultProps = { + id: 'loop-1', + type: 'loopNode', + data: { + width: 500, + height: 300, + state: 'valid', + }, + selected: false, + zIndex: 1, + isConnectable: true, + xPos: 0, + yPos: 0, + dragging: false, + } + + beforeEach(() => { + vi.clearAllMocks() + + // Mock useWorkflowStore + + ;(useWorkflowStore as any).mockImplementation((selector: any) => { + const state = { + removeBlock: mockRemoveBlock, + } + return selector(state) + }) + + // Mock getNodes + mockGetNodes.mockReturnValue([]) + }) + + describe('Component Definition and Structure', () => { + it('should be defined as a function component', () => { + expect(LoopNodeComponent).toBeDefined() + expect(typeof LoopNodeComponent).toBe('function') + }) + + it('should have correct display name', () => { + expect(LoopNodeComponent.displayName).toBe('LoopNodeComponent') + }) + + it('should be a memoized component', () => { + // Since we mocked memo to return the component as-is, we can verify it exists + expect(LoopNodeComponent).toBeDefined() + }) + }) + + describe('Props Validation and Type Safety', () => { + it('should accept NodeProps interface', () => { + // Test that the component accepts the correct prop types + const validProps = { + id: 'test-id', + type: 'loopNode' as const, + data: { + width: 400, + height: 300, + state: 'valid' as const, + }, + selected: false, + zIndex: 1, + isConnectable: true, + xPos: 0, + yPos: 0, + dragging: false, + } + + // This tests that TypeScript compilation succeeds with these props + expect(() => { + // We're not calling the component, just verifying the types + const _component: typeof LoopNodeComponent = LoopNodeComponent + expect(_component).toBeDefined() + }).not.toThrow() + }) + + it('should handle different data configurations', () => { + const configurations = [ + { width: 500, height: 300, state: 'valid' }, + { width: 800, height: 600, state: 'invalid' }, + { width: 0, height: 0, state: 'pending' }, + {}, + ] + + configurations.forEach((data) => { + const props = { ...defaultProps, data } + expect(() => { + const _component: typeof LoopNodeComponent = LoopNodeComponent + expect(_component).toBeDefined() + }).not.toThrow() + }) + }) + }) + + describe('Store Integration', () => { + it('should integrate with workflow store', () => { + // Test that the component uses the store correctly + expect(useWorkflowStore).toBeDefined() + + // Verify the store selector function works + const mockState = { removeBlock: mockRemoveBlock } + const selector = vi.fn((state) => state.removeBlock) + + expect(() => { + selector(mockState) + }).not.toThrow() + + expect(selector(mockState)).toBe(mockRemoveBlock) + }) + + it('should handle removeBlock function', () => { + expect(mockRemoveBlock).toBeDefined() + expect(typeof mockRemoveBlock).toBe('function') + + // Test calling removeBlock + mockRemoveBlock('test-id') + expect(mockRemoveBlock).toHaveBeenCalledWith('test-id') + }) + }) + + describe('Component Logic Tests', () => { + it('should handle nesting level calculation logic', () => { + // Test the nesting level calculation logic + const testCases = [ + { nodes: [], parentId: undefined, expectedLevel: 0 }, + { nodes: [{ id: 'parent', data: {} }], parentId: 'parent', expectedLevel: 1 }, + { + nodes: [ + { id: 'parent', data: { parentId: 'grandparent' } }, + { id: 'grandparent', data: {} }, + ], + parentId: 'parent', + expectedLevel: 2, + }, + ] + + testCases.forEach(({ nodes, parentId, expectedLevel }) => { + mockGetNodes.mockReturnValue(nodes) + + // Simulate the nesting level calculation logic + let level = 0 + let currentParentId = parentId + + while (currentParentId) { + level++ + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + currentParentId = parentNode.data?.parentId + } + + expect(level).toBe(expectedLevel) + }) + }) + + it('should handle nested styles generation', () => { + // Test the nested styles logic + const testCases = [ + { nestingLevel: 0, state: 'valid', expectedBg: 'rgba(34,197,94,0.05)' }, + { nestingLevel: 0, state: 'invalid', expectedBg: 'transparent' }, + { nestingLevel: 1, state: 'valid', expectedBg: '#e2e8f030' }, + { nestingLevel: 2, state: 'valid', expectedBg: '#cbd5e130' }, + ] + + testCases.forEach(({ nestingLevel, state, expectedBg }) => { + // Simulate the getNestedStyles logic + const styles: Record = { + backgroundColor: state === 'valid' ? 'rgba(34,197,94,0.05)' : 'transparent', + } + + if (nestingLevel > 0) { + const colors = ['#e2e8f0', '#cbd5e1', '#94a3b8', '#64748b', '#475569'] + const colorIndex = (nestingLevel - 1) % colors.length + styles.backgroundColor = `${colors[colorIndex]}30` + } + + expect(styles.backgroundColor).toBe(expectedBg) + }) + }) + }) + + describe('Component Configuration', () => { + it('should handle different dimensions', () => { + const dimensionTests = [ + { width: 500, height: 300 }, + { width: 800, height: 600 }, + { width: 0, height: 0 }, + { width: 10000, height: 10000 }, + ] + + dimensionTests.forEach(({ width, height }) => { + const data = { width, height, state: 'valid' } + expect(data.width).toBe(width) + expect(data.height).toBe(height) + }) + }) + + it('should handle different states', () => { + const stateTests = ['valid', 'invalid', 'pending', 'executing'] + + stateTests.forEach((state) => { + const data = { width: 500, height: 300, state } + expect(data.state).toBe(state) + }) + }) + }) + + describe('Event Handling Logic', () => { + it('should handle delete button click logic', () => { + const mockEvent = { + stopPropagation: vi.fn(), + } + + // Simulate the delete button click handler + const handleDelete = (e: any, nodeId: string) => { + e.stopPropagation() + mockRemoveBlock(nodeId) + } + + handleDelete(mockEvent, 'test-id') + + expect(mockEvent.stopPropagation).toHaveBeenCalled() + expect(mockRemoveBlock).toHaveBeenCalledWith('test-id') + }) + + it('should handle event propagation prevention', () => { + const mockEvent = { + stopPropagation: vi.fn(), + } + + // Test that stopPropagation is called + mockEvent.stopPropagation() + expect(mockEvent.stopPropagation).toHaveBeenCalled() + }) + }) + + describe('Component Data Handling', () => { + it('should handle missing data properties gracefully', () => { + const testCases = [ + undefined, + {}, + { width: 500 }, + { height: 300 }, + { state: 'valid' }, + { width: 500, height: 300 }, + ] + + testCases.forEach((data) => { + const props = { ...defaultProps, data } + + // Test default values logic + const width = Math.max(0, data?.width || 500) + const height = Math.max(0, data?.height || 300) + + expect(width).toBeGreaterThanOrEqual(0) + expect(height).toBeGreaterThanOrEqual(0) + }) + }) + + it('should handle parent ID relationships', () => { + const testCases = [ + { parentId: undefined, hasParent: false }, + { parentId: 'parent-1', hasParent: true }, + { parentId: '', hasParent: false }, + ] + + testCases.forEach(({ parentId, hasParent }) => { + const data = { ...defaultProps.data, parentId } + expect(Boolean(data.parentId)).toBe(hasParent) + }) + }) + }) + + describe('Edge Cases and Error Handling', () => { + it('should handle circular parent references', () => { + // Test circular reference prevention + const nodes = [ + { id: 'node1', data: { parentId: 'node2' } }, + { id: 'node2', data: { parentId: 'node1' } }, + ] + + mockGetNodes.mockReturnValue(nodes) + + // Test the actual component's nesting level calculation logic + // This simulates the real useMemo logic from the component + let level = 0 + let currentParentId = 'node1' + const visited = new Set() + + // This is the actual logic pattern used in the component + while (currentParentId) { + // If we've seen this parent before, we have a cycle - break immediately + if (visited.has(currentParentId)) { + break + } + + visited.add(currentParentId) + level++ + + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + + currentParentId = parentNode.data?.parentId + } + + // With proper circular reference detection, we should stop at level 2 + // (node1 -> node2, then detect cycle when trying to go back to node1) + expect(level).toBe(2) + expect(visited.has('node1')).toBe(true) + expect(visited.has('node2')).toBe(true) + }) + + it('should handle complex circular reference chains', () => { + // Test more complex circular reference scenarios + const nodes = [ + { id: 'node1', data: { parentId: 'node2' } }, + { id: 'node2', data: { parentId: 'node3' } }, + { id: 'node3', data: { parentId: 'node1' } }, // Creates a 3-node cycle + ] + + mockGetNodes.mockReturnValue(nodes) + + let level = 0 + let currentParentId = 'node1' + const visited = new Set() + + while (currentParentId) { + if (visited.has(currentParentId)) { + break // Cycle detected + } + + visited.add(currentParentId) + level++ + + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + + currentParentId = parentNode.data?.parentId + } + + // Should traverse node1 -> node2 -> node3, then detect cycle + expect(level).toBe(3) + expect(visited.size).toBe(3) + }) + + it('should handle self-referencing nodes', () => { + // Test node that references itself + const nodes = [ + { id: 'node1', data: { parentId: 'node1' } }, // Self-reference + ] + + mockGetNodes.mockReturnValue(nodes) + + let level = 0 + let currentParentId = 'node1' + const visited = new Set() + + while (currentParentId) { + if (visited.has(currentParentId)) { + break // Cycle detected immediately + } + + visited.add(currentParentId) + level++ + + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + + currentParentId = parentNode.data?.parentId + } + + // Should detect self-reference immediately after first iteration + expect(level).toBe(1) + expect(visited.has('node1')).toBe(true) + }) + + it('should handle extreme values', () => { + const extremeValues = [ + { width: Number.MAX_SAFE_INTEGER, height: Number.MAX_SAFE_INTEGER }, + { width: -1, height: -1 }, + { width: 0, height: 0 }, + { width: null, height: null }, + ] + + extremeValues.forEach((data) => { + expect(() => { + const width = data.width || 500 + const height = data.height || 300 + expect(typeof width).toBe('number') + expect(typeof height).toBe('number') + }).not.toThrow() + }) + }) + }) +}) diff --git a/apps/sim/app/w/[id]/components/loop-node/loop-node.tsx b/apps/sim/app/w/[id]/components/loop-node/loop-node.tsx index f90ce40a50..9190c1a33f 100644 --- a/apps/sim/app/w/[id]/components/loop-node/loop-node.tsx +++ b/apps/sim/app/w/[id]/components/loop-node/loop-node.tsx @@ -26,12 +26,6 @@ const LoopNodeStyles: React.FC = () => { box-shadow: 0 0 0 8px rgba(47, 179, 255, 0.1); } - /* Make resizer handles more visible */ - .react-flow__resize-control { - z-index: 10; - pointer-events: all !important; - } - /* Ensure parent borders are visible when hovering over resize controls */ .react-flow__node-group:hover, .hover-highlight { @@ -43,12 +37,7 @@ const LoopNodeStyles: React.FC = () => { opacity: 1 !important; visibility: visible !important; } - - /* React Flow position transitions within loops */ - .react-flow__node[data-parent-node-id] { - transition: transform 0.05s ease; - pointer-events: all; - } + /* Prevent jumpy drag behavior */ .loop-drop-container .react-flow__node { diff --git a/apps/sim/app/w/[id]/components/parallel-node/parallel-node.test.tsx b/apps/sim/app/w/[id]/components/parallel-node/parallel-node.test.tsx new file mode 100644 index 0000000000..260a691795 --- /dev/null +++ b/apps/sim/app/w/[id]/components/parallel-node/parallel-node.test.tsx @@ -0,0 +1,616 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { useWorkflowStore } from '@/stores/workflows/workflow/store' +import { ParallelNodeComponent } from './parallel-node' + +// Mock dependencies that don't need DOM +vi.mock('@/stores/workflows/workflow/store', () => ({ + useWorkflowStore: vi.fn(), +})) + +vi.mock('@/lib/logs/console-logger', () => ({ + createLogger: vi.fn(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + })), +})) + +// Mock ReactFlow components and hooks +vi.mock('reactflow', () => ({ + Handle: ({ id, type, position }: any) => ({ id, type, position }), + Position: { + Top: 'top', + Bottom: 'bottom', + Left: 'left', + Right: 'right', + }, + useReactFlow: () => ({ + getNodes: vi.fn(() => []), + }), + NodeResizer: ({ isVisible }: any) => ({ isVisible }), + memo: (component: any) => component, +})) + +// Mock React hooks +vi.mock('react', async () => { + const actual = await vi.importActual('react') + return { + ...actual, + memo: (component: any) => component, + useMemo: (fn: any) => fn(), + useRef: () => ({ current: null }), + } +}) + +// Mock UI components +vi.mock('@/components/ui/button', () => ({ + Button: ({ children, onClick, ...props }: any) => ({ children, onClick, ...props }), +})) + +vi.mock('@/components/ui/card', () => ({ + Card: ({ children, ...props }: any) => ({ children, ...props }), +})) + +vi.mock('@/components/icons', () => ({ + StartIcon: ({ className }: any) => ({ className }), +})) + +vi.mock('@/lib/utils', () => ({ + cn: (...classes: any[]) => classes.filter(Boolean).join(' '), +})) + +// Mock the ParallelBadges component +vi.mock('./components/parallel-badges', () => ({ + ParallelBadges: ({ parallelId }: any) => ({ parallelId }), +})) + +describe('ParallelNodeComponent', () => { + const mockRemoveBlock = vi.fn() + const mockGetNodes = vi.fn() + const defaultProps = { + id: 'parallel-1', + type: 'parallelNode', + data: { + width: 500, + height: 300, + state: 'valid', + }, + selected: false, + zIndex: 1, + isConnectable: true, + xPos: 0, + yPos: 0, + dragging: false, + } + + beforeEach(() => { + vi.clearAllMocks() + + // Mock useWorkflowStore + + ;(useWorkflowStore as any).mockImplementation((selector: any) => { + const state = { + removeBlock: mockRemoveBlock, + } + return selector(state) + }) + + // Mock getNodes + mockGetNodes.mockReturnValue([]) + }) + + describe('Component Definition and Structure', () => { + it('should be defined as a function component', () => { + expect(ParallelNodeComponent).toBeDefined() + expect(typeof ParallelNodeComponent).toBe('function') + }) + + it('should have correct display name', () => { + expect(ParallelNodeComponent.displayName).toBe('ParallelNodeComponent') + }) + + it('should be a memoized component', () => { + // Since we mocked memo to return the component as-is, we can verify it exists + expect(ParallelNodeComponent).toBeDefined() + }) + }) + + describe('Props Validation and Type Safety', () => { + it('should accept NodeProps interface', () => { + // Test that the component accepts the correct prop types + const validProps = { + id: 'test-id', + type: 'parallelNode' as const, + data: { + width: 400, + height: 300, + state: 'valid' as const, + }, + selected: false, + zIndex: 1, + isConnectable: true, + xPos: 0, + yPos: 0, + dragging: false, + } + + // This tests that TypeScript compilation succeeds with these props + expect(() => { + // We're not calling the component, just verifying the types + const _component: typeof ParallelNodeComponent = ParallelNodeComponent + expect(_component).toBeDefined() + }).not.toThrow() + }) + + it('should handle different data configurations', () => { + const configurations = [ + { width: 500, height: 300, state: 'valid' }, + { width: 800, height: 600, state: 'invalid' }, + { width: 0, height: 0, state: 'pending' }, + {}, + ] + + configurations.forEach((data) => { + const props = { ...defaultProps, data } + expect(() => { + const _component: typeof ParallelNodeComponent = ParallelNodeComponent + expect(_component).toBeDefined() + }).not.toThrow() + }) + }) + }) + + describe('Store Integration', () => { + it('should integrate with workflow store', () => { + // Test that the component uses the store correctly + expect(useWorkflowStore).toBeDefined() + + // Verify the store selector function works + const mockState = { removeBlock: mockRemoveBlock } + const selector = vi.fn((state) => state.removeBlock) + + expect(() => { + selector(mockState) + }).not.toThrow() + + expect(selector(mockState)).toBe(mockRemoveBlock) + }) + + it('should handle removeBlock function', () => { + expect(mockRemoveBlock).toBeDefined() + expect(typeof mockRemoveBlock).toBe('function') + + // Test calling removeBlock + mockRemoveBlock('test-id') + expect(mockRemoveBlock).toHaveBeenCalledWith('test-id') + }) + }) + + describe('Component Logic Tests', () => { + it('should handle nesting level calculation logic', () => { + // Test the nesting level calculation logic (same as loop node) + const testCases = [ + { nodes: [], parentId: undefined, expectedLevel: 0 }, + { nodes: [{ id: 'parent', data: {} }], parentId: 'parent', expectedLevel: 1 }, + { + nodes: [ + { id: 'parent', data: { parentId: 'grandparent' } }, + { id: 'grandparent', data: {} }, + ], + parentId: 'parent', + expectedLevel: 2, + }, + ] + + testCases.forEach(({ nodes, parentId, expectedLevel }) => { + mockGetNodes.mockReturnValue(nodes) + + // Simulate the nesting level calculation logic + let level = 0 + let currentParentId = parentId + + while (currentParentId) { + level++ + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + currentParentId = parentNode.data?.parentId + } + + expect(level).toBe(expectedLevel) + }) + }) + + it('should handle nested styles generation for parallel nodes', () => { + // Test the nested styles logic with parallel-specific colors + const testCases = [ + { nestingLevel: 0, state: 'valid', expectedBg: 'rgba(254,225,43,0.05)' }, + { nestingLevel: 0, state: 'invalid', expectedBg: 'transparent' }, + { nestingLevel: 1, state: 'valid', expectedBg: '#e2e8f030' }, + { nestingLevel: 2, state: 'valid', expectedBg: '#cbd5e130' }, + ] + + testCases.forEach(({ nestingLevel, state, expectedBg }) => { + // Simulate the getNestedStyles logic for parallel nodes + const styles: Record = { + backgroundColor: state === 'valid' ? 'rgba(254,225,43,0.05)' : 'transparent', + } + + if (nestingLevel > 0) { + const colors = ['#e2e8f0', '#cbd5e1', '#94a3b8', '#64748b', '#475569'] + const colorIndex = (nestingLevel - 1) % colors.length + styles.backgroundColor = `${colors[colorIndex]}30` + } + + expect(styles.backgroundColor).toBe(expectedBg) + }) + }) + }) + + describe('Parallel-Specific Features', () => { + it('should handle parallel execution states', () => { + const parallelStates = ['valid', 'invalid', 'executing', 'completed', 'pending'] + + parallelStates.forEach((state) => { + const data = { width: 500, height: 300, state } + expect(data.state).toBe(state) + + // Test parallel-specific state handling + const isExecuting = state === 'executing' + const isCompleted = state === 'completed' + + expect(typeof isExecuting).toBe('boolean') + expect(typeof isCompleted).toBe('boolean') + }) + }) + + it('should handle parallel node color scheme', () => { + // Test that parallel nodes use yellow color scheme + const parallelColors = { + background: 'rgba(254,225,43,0.05)', + ring: '#FEE12B', + startIcon: '#FEE12B', + } + + expect(parallelColors.background).toContain('254,225,43') + expect(parallelColors.ring).toBe('#FEE12B') + expect(parallelColors.startIcon).toBe('#FEE12B') + }) + + it('should differentiate from loop node styling', () => { + // Ensure parallel nodes have different styling than loop nodes + const loopColors = { + background: 'rgba(34,197,94,0.05)', + ring: '#2FB3FF', + startIcon: '#2FB3FF', + } + + const parallelColors = { + background: 'rgba(254,225,43,0.05)', + ring: '#FEE12B', + startIcon: '#FEE12B', + } + + expect(parallelColors.background).not.toBe(loopColors.background) + expect(parallelColors.ring).not.toBe(loopColors.ring) + expect(parallelColors.startIcon).not.toBe(loopColors.startIcon) + }) + }) + + describe('Component Configuration', () => { + it('should handle different dimensions', () => { + const dimensionTests = [ + { width: 500, height: 300 }, + { width: 800, height: 600 }, + { width: 0, height: 0 }, + { width: 10000, height: 10000 }, + ] + + dimensionTests.forEach(({ width, height }) => { + const data = { width, height, state: 'valid' } + expect(data.width).toBe(width) + expect(data.height).toBe(height) + }) + }) + + it('should handle different states', () => { + const stateTests = ['valid', 'invalid', 'pending', 'executing', 'completed'] + + stateTests.forEach((state) => { + const data = { width: 500, height: 300, state } + expect(data.state).toBe(state) + }) + }) + }) + + describe('Event Handling Logic', () => { + it('should handle delete button click logic', () => { + const mockEvent = { + stopPropagation: vi.fn(), + } + + // Simulate the delete button click handler + const handleDelete = (e: any, nodeId: string) => { + e.stopPropagation() + mockRemoveBlock(nodeId) + } + + handleDelete(mockEvent, 'test-id') + + expect(mockEvent.stopPropagation).toHaveBeenCalled() + expect(mockRemoveBlock).toHaveBeenCalledWith('test-id') + }) + + it('should handle event propagation prevention', () => { + const mockEvent = { + stopPropagation: vi.fn(), + } + + // Test that stopPropagation is called + mockEvent.stopPropagation() + expect(mockEvent.stopPropagation).toHaveBeenCalled() + }) + }) + + describe('Component Data Handling', () => { + it('should handle missing data properties gracefully', () => { + const testCases = [ + undefined, + {}, + { width: 500 }, + { height: 300 }, + { state: 'valid' }, + { width: 500, height: 300 }, + ] + + testCases.forEach((data) => { + const props = { ...defaultProps, data } + + // Test default values logic + const width = data?.width || 500 + const height = data?.height || 300 + + expect(width).toBeGreaterThanOrEqual(0) + expect(height).toBeGreaterThanOrEqual(0) + }) + }) + + it('should handle parent ID relationships', () => { + const testCases = [ + { parentId: undefined, hasParent: false }, + { parentId: 'parent-1', hasParent: true }, + { parentId: '', hasParent: false }, + ] + + testCases.forEach(({ parentId, hasParent }) => { + const data = { ...defaultProps.data, parentId } + expect(Boolean(data.parentId)).toBe(hasParent) + }) + }) + }) + + describe('Handle Configuration', () => { + it('should have correct handle IDs for parallel nodes', () => { + const handleIds = { + startSource: 'parallel-start-source', + endSource: 'parallel-end-source', + } + + expect(handleIds.startSource).toContain('parallel') + expect(handleIds.endSource).toContain('parallel') + expect(handleIds.startSource).not.toContain('loop') + expect(handleIds.endSource).not.toContain('loop') + }) + + it('should handle different handle positions', () => { + const positions = { + left: 'left', + right: 'right', + top: 'top', + bottom: 'bottom', + } + + Object.values(positions).forEach((position) => { + expect(typeof position).toBe('string') + expect(position.length).toBeGreaterThan(0) + }) + }) + }) + + describe('Edge Cases and Error Handling', () => { + it('should handle circular parent references', () => { + // Test circular reference prevention + const nodes = [ + { id: 'node1', data: { parentId: 'node2' } }, + { id: 'node2', data: { parentId: 'node1' } }, + ] + + mockGetNodes.mockReturnValue(nodes) + + // Test the actual component's nesting level calculation logic + // This simulates the real useMemo logic from the component + let level = 0 + let currentParentId = 'node1' + const visited = new Set() + + // This is the actual logic pattern used in the component + while (currentParentId) { + // If we've seen this parent before, we have a cycle - break immediately + if (visited.has(currentParentId)) { + break + } + + visited.add(currentParentId) + level++ + + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + + currentParentId = parentNode.data?.parentId + } + + // With proper circular reference detection, we should stop at level 2 + // (node1 -> node2, then detect cycle when trying to go back to node1) + expect(level).toBe(2) + expect(visited.has('node1')).toBe(true) + expect(visited.has('node2')).toBe(true) + }) + + it('should handle complex circular reference chains', () => { + // Test more complex circular reference scenarios + const nodes = [ + { id: 'node1', data: { parentId: 'node2' } }, + { id: 'node2', data: { parentId: 'node3' } }, + { id: 'node3', data: { parentId: 'node1' } }, // Creates a 3-node cycle + ] + + mockGetNodes.mockReturnValue(nodes) + + let level = 0 + let currentParentId = 'node1' + const visited = new Set() + + while (currentParentId) { + if (visited.has(currentParentId)) { + break // Cycle detected + } + + visited.add(currentParentId) + level++ + + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + + currentParentId = parentNode.data?.parentId + } + + // Should traverse node1 -> node2 -> node3, then detect cycle + expect(level).toBe(3) + expect(visited.size).toBe(3) + }) + + it('should handle self-referencing nodes', () => { + // Test node that references itself + const nodes = [ + { id: 'node1', data: { parentId: 'node1' } }, // Self-reference + ] + + mockGetNodes.mockReturnValue(nodes) + + let level = 0 + let currentParentId = 'node1' + const visited = new Set() + + while (currentParentId) { + if (visited.has(currentParentId)) { + break // Cycle detected immediately + } + + visited.add(currentParentId) + level++ + + const parentNode = nodes.find((n) => n.id === currentParentId) + if (!parentNode) break + + currentParentId = parentNode.data?.parentId + } + + // Should detect self-reference immediately after first iteration + expect(level).toBe(1) + expect(visited.has('node1')).toBe(true) + }) + + it('should handle extreme values', () => { + const extremeValues = [ + { width: Number.MAX_SAFE_INTEGER, height: Number.MAX_SAFE_INTEGER }, + { width: -1, height: -1 }, + { width: 0, height: 0 }, + { width: null, height: null }, + ] + + extremeValues.forEach((data) => { + expect(() => { + const width = data.width || 500 + const height = data.height || 300 + expect(typeof width).toBe('number') + expect(typeof height).toBe('number') + }).not.toThrow() + }) + }) + + it('should handle negative position values', () => { + const positions = [ + { xPos: -100, yPos: -200 }, + { xPos: 0, yPos: 0 }, + { xPos: 1000, yPos: 2000 }, + ] + + positions.forEach(({ xPos, yPos }) => { + const props = { ...defaultProps, xPos, yPos } + expect(props.xPos).toBe(xPos) + expect(props.yPos).toBe(yPos) + expect(typeof props.xPos).toBe('number') + expect(typeof props.yPos).toBe('number') + }) + }) + }) + + describe('Component Comparison with Loop Node', () => { + it('should have similar structure to loop node but different type', () => { + expect(defaultProps.type).toBe('parallelNode') + expect(defaultProps.id).toContain('parallel') + + // Should not be a loop node + expect(defaultProps.type).not.toBe('loopNode') + expect(defaultProps.id).not.toContain('loop') + }) + + it('should handle the same prop structure as loop node', () => { + // Test that parallel node accepts the same prop structure as loop node + const sharedPropStructure = { + id: 'test-parallel', + type: 'parallelNode' as const, + data: { + width: 400, + height: 300, + state: 'valid' as const, + }, + selected: false, + zIndex: 1, + isConnectable: true, + xPos: 0, + yPos: 0, + dragging: false, + } + + expect(() => { + const _component: typeof ParallelNodeComponent = ParallelNodeComponent + expect(_component).toBeDefined() + }).not.toThrow() + + // Verify the structure + expect(sharedPropStructure.type).toBe('parallelNode') + expect(sharedPropStructure.data.width).toBe(400) + expect(sharedPropStructure.data.height).toBe(300) + }) + + it('should maintain consistency with loop node interface', () => { + // Both components should accept the same base props + const baseProps = [ + 'id', + 'type', + 'data', + 'selected', + 'zIndex', + 'isConnectable', + 'xPos', + 'yPos', + 'dragging', + ] + + baseProps.forEach((prop) => { + expect(defaultProps).toHaveProperty(prop) + }) + }) + }) +}) diff --git a/apps/sim/app/w/[id]/components/parallel-node/parallel-node.tsx b/apps/sim/app/w/[id]/components/parallel-node/parallel-node.tsx index 85f7e47d52..f0ff171038 100644 --- a/apps/sim/app/w/[id]/components/parallel-node/parallel-node.tsx +++ b/apps/sim/app/w/[id]/components/parallel-node/parallel-node.tsx @@ -25,12 +25,6 @@ const ParallelNodeStyles: React.FC = () => { box-shadow: 0 0 0 8px rgba(254, 225, 43, 0.1); } - /* Make resizer handles more visible */ - .react-flow__resize-control { - z-index: 10; - pointer-events: all !important; - } - /* Ensure parent borders are visible when hovering over resize controls */ .react-flow__node-group:hover, .hover-highlight { @@ -43,11 +37,6 @@ const ParallelNodeStyles: React.FC = () => { visibility: visible !important; } - /* React Flow position transitions within parallel blocks */ - .react-flow__node[data-parent-node-id] { - transition: transform 0.05s ease; - pointer-events: all; - } /* Prevent jumpy drag behavior */ .parallel-drop-container .react-flow__node { diff --git a/apps/sim/app/w/[id]/components/workflow-block/components/sub-block/components/long-input.tsx b/apps/sim/app/w/[id]/components/workflow-block/components/sub-block/components/long-input.tsx index 26eee1ca35..bd66e8d5da 100644 --- a/apps/sim/app/w/[id]/components/workflow-block/components/sub-block/components/long-input.tsx +++ b/apps/sim/app/w/[id]/components/workflow-block/components/sub-block/components/long-input.tsx @@ -105,6 +105,7 @@ export function LongInput({ // Handle resize functionality const startResize = (e: React.MouseEvent) => { e.preventDefault() + e.stopPropagation() isResizing.current = true const startY = e.clientY @@ -290,6 +291,9 @@ export function LongInput({
{ + e.preventDefault() + }} >
diff --git a/apps/sim/app/w/[id]/components/workflow-block/workflow-block.tsx b/apps/sim/app/w/[id]/components/workflow-block/workflow-block.tsx index bad9940317..76541345a6 100644 --- a/apps/sim/app/w/[id]/components/workflow-block/workflow-block.tsx +++ b/apps/sim/app/w/[id]/components/workflow-block/workflow-block.tsx @@ -426,7 +426,12 @@ export function WorkflowBlock({ id, data }: NodeProps) { )} {/* Block Header */} -
+
{ + e.stopPropagation() + }} + >
) {
{/* Block Content */} -
+
{ + e.stopPropagation() + }} + > {subBlockRows.length > 0 ? subBlockRows.map((row, rowIndex) => (
diff --git a/apps/sim/app/w/[id]/utils.ts b/apps/sim/app/w/[id]/utils.ts index 28afd6cc6c..d16f092581 100644 --- a/apps/sim/app/w/[id]/utils.ts +++ b/apps/sim/app/w/[id]/utils.ts @@ -151,12 +151,6 @@ export const updateNodeParent = ( // Update both position and parent updateBlockPosition(nodeId, relativePosition) updateParentId(nodeId, newParentId, 'parent') - - logger.info('Updated node parent', { - nodeId, - newParentId, - relativePosition, - }) } else if (currentParentId) { // Removing parent - convert to absolute position const absolutePosition = getNodeAbsolutePosition(nodeId, getNodes) @@ -164,12 +158,6 @@ export const updateNodeParent = ( // Update position to absolute coordinates and remove parent updateBlockPosition(nodeId, absolutePosition) // Note: updateParentId function signature needs to handle null case - - logger.info('Removed node parent', { - nodeId, - previousParentId: currentParentId, - absolutePosition, - }) } // Resize affected loops @@ -262,45 +250,51 @@ export const calculateLoopDimensions = ( let nodeHeight if (node.type === 'loopNode' || node.type === 'parallelNode') { - // For nested containers, don't add excessive padding to the parent - // Use actual dimensions without additional padding to prevent cascading expansion + // For nested containers, use their actual dimensions nodeWidth = node.data?.width || DEFAULT_CONTAINER_WIDTH nodeHeight = node.data?.height || DEFAULT_CONTAINER_HEIGHT } else if (node.type === 'workflowBlock') { - // Handle all workflowBlock types appropriately - const blockType = node.data?.type + // Use actual dynamic dimensions from the node data if available + // Fall back to block type defaults only if no dynamic dimensions exist + nodeWidth = node.data?.width || node.width + nodeHeight = node.data?.height || node.height - switch (blockType) { - case 'agent': - case 'api': - // Tall blocks - nodeWidth = 350 - nodeHeight = 650 - break - case 'condition': - case 'function': - nodeWidth = 250 - nodeHeight = 200 - break - case 'router': - nodeWidth = 250 - nodeHeight = 350 - break - default: - // Default dimensions for other block types - nodeWidth = 200 - nodeHeight = 200 + // If still no dimensions, use block type defaults as last resort + if (!nodeWidth || !nodeHeight) { + const blockType = node.data?.type + + switch (blockType) { + case 'agent': + case 'api': + // Tall blocks + nodeWidth = nodeWidth || 350 + nodeHeight = nodeHeight || 650 + break + case 'condition': + case 'function': + nodeWidth = nodeWidth || 250 + nodeHeight = nodeHeight || 200 + break + case 'router': + nodeWidth = nodeWidth || 250 + nodeHeight = nodeHeight || 350 + break + default: + // Default dimensions for other block types + nodeWidth = nodeWidth || 200 + nodeHeight = nodeHeight || 200 + } } } else { - // Default dimensions for any other node types - nodeWidth = 200 - nodeHeight = 200 + // For any other node types, try to get actual dimensions first + nodeWidth = node.data?.width || node.width || 200 + nodeHeight = node.data?.height || node.height || 200 } - minX = Math.min(minX, node.position.x) - minY = Math.min(minY, node.position.y) + minX = Math.min(minX, node.position.x + nodeWidth) + minY = Math.min(minY, node.position.y + nodeHeight) maxX = Math.max(maxX, node.position.x + nodeWidth) - maxY = Math.max(maxY, node.position.y + nodeHeight) + maxY = Math.max(maxY, node.position.y + nodeHeight + 50) }) // Add buffer padding to all sides (20px buffer before edges) @@ -313,9 +307,12 @@ export const calculateLoopDimensions = ( // Reduce the excessive padding that was causing parent containers to be too large const sidePadding = hasNestedContainers ? 150 : 120 // Reduced padding for containers containing other containers + // Add extra padding to the right side to prevent sidebar from covering the right handle + const extraPadding = 50 + // Ensure the width and height are never less than the minimums - // Apply padding to all sides (left/right and top/bottom) - const width = Math.max(minWidth, maxX + sidePadding) + // Apply padding to all sides (left/right and top/bottom) with extra right padding + const width = Math.max(minWidth, maxX + sidePadding + extraPadding) const height = Math.max(minHeight, maxY + sidePadding) return { width, height } diff --git a/apps/sim/app/w/[id]/workflow.tsx b/apps/sim/app/w/[id]/workflow.tsx index 3c4b046db9..ae86ad3ff2 100644 --- a/apps/sim/app/w/[id]/workflow.tsx +++ b/apps/sim/app/w/[id]/workflow.tsx @@ -223,6 +223,12 @@ function WorkflowContent() { } } } + // For loop and parallel nodes, use their end source handle + else if (block.type === 'loop') { + sourceHandle = 'loop-end-source' + } else if (block.type === 'parallel') { + sourceHandle = 'parallel-end-source' + } return sourceHandle }, []) @@ -376,12 +382,6 @@ function WorkflowContent() { extent: 'parent', }) - logger.info(`Added nested ${data.type} inside parent container`, { - nodeId: id, - parentId: containerInfo.loopId, - relativePosition, - }) - // Auto-connect the nested container to nodes inside the parent container const isAutoConnectEnabled = useGeneralStore.getState().isAutoConnectEnabled if (isAutoConnectEnabled) { @@ -485,13 +485,6 @@ function WorkflowContent() { extent: 'parent', }) - logger.info('Added block inside container', { - blockId: id, - blockType: data.type, - containerId: containerInfo.loopId, - relativePosition, - }) - // Resize the container node to fit the new block // Immediate resize without delay debouncedResizeLoopNodes() @@ -499,42 +492,61 @@ function WorkflowContent() { // Auto-connect logic for blocks inside containers const isAutoConnectEnabled = useGeneralStore.getState().isAutoConnectEnabled if (isAutoConnectEnabled && data.type !== 'starter') { - // Try to find other nodes in the container to connect to - const containerNodes = getNodes().filter((n) => n.parentId === containerInfo.loopId) + // First priority: Connect to the container's start node + const containerNode = getNodes().find((n) => n.id === containerInfo.loopId) + const containerType = containerNode?.type - if (containerNodes.length > 0) { - // Connect to the closest node in the container - const closestNode = containerNodes - .map((n) => ({ - id: n.id, - distance: Math.sqrt( - (n.position.x - relativePosition.x) ** 2 + - (n.position.y - relativePosition.y) ** 2 - ), - })) - .sort((a, b) => a.distance - b.distance)[0] + if (containerType === 'loopNode' || containerType === 'parallelNode') { + // Connect from the container's start node to the new block + const startSourceHandle = + containerType === 'loopNode' ? 'loop-start-source' : 'parallel-start-source' - if (closestNode) { - // Get appropriate source handle - const sourceNode = getNodes().find((n) => n.id === closestNode.id) - const sourceType = sourceNode?.data?.type + addEdge({ + id: crypto.randomUUID(), + source: containerInfo.loopId, + target: id, + sourceHandle: startSourceHandle, + targetHandle: 'target', + type: 'workflowEdge', + }) + } else { + // Fallback: Try to find other nodes in the container to connect to + const containerNodes = getNodes().filter((n) => n.parentId === containerInfo.loopId) - // Default source handle - let sourceHandle = 'source' + if (containerNodes.length > 0) { + // Connect to the closest node in the container + const closestNode = containerNodes + .map((n) => ({ + id: n.id, + distance: Math.sqrt( + (n.position.x - relativePosition.x) ** 2 + + (n.position.y - relativePosition.y) ** 2 + ), + })) + .sort((a, b) => a.distance - b.distance)[0] - // For condition blocks, use the condition-true handle - if (sourceType === 'condition') { - sourceHandle = 'condition-true' + if (closestNode) { + // Get appropriate source handle + const sourceNode = getNodes().find((n) => n.id === closestNode.id) + const sourceType = sourceNode?.data?.type + + // Default source handle + let sourceHandle = 'source' + + // For condition blocks, use the condition-true handle + if (sourceType === 'condition') { + sourceHandle = 'condition-true' + } + + addEdge({ + id: crypto.randomUUID(), + source: closestNode.id, + target: id, + sourceHandle, + targetHandle: 'target', + type: 'workflowEdge', + }) } - - addEdge({ - id: crypto.randomUUID(), - source: closestNode.id, - target: id, - sourceHandle, - targetHandle: 'target', - type: 'workflowEdge', - }) } } } @@ -744,6 +756,7 @@ function WorkflowContent() { type: 'workflowBlock', position, parentId: block.data?.parentId, + dragHandle: '.workflow-drag-handle', extent: block.data?.extent || undefined, data: { type: block.type, @@ -752,6 +765,9 @@ function WorkflowContent() { isActive, isPending, }, + // Include dynamic dimensions for container resizing calculations + width: block.isWide ? 450 : 350, // Standard width based on isWide state + height: Math.max(block.height || 100, 100), // Use actual height with minimum }) }) @@ -815,9 +831,6 @@ function WorkflowContent() { (changes: any) => { changes.forEach((change: any) => { if (change.type === 'remove') { - logger.info('Edge removal requested via ReactFlow:', { - edgeId: change.id, - }) removeEdge(change.id) } }) @@ -831,9 +844,6 @@ function WorkflowContent() { if (connection.source && connection.target) { // Prevent self-connections if (connection.source === connection.target) { - logger.info('Rejected self-connection:', { - nodeId: connection.source, - }) return } @@ -862,12 +872,6 @@ function WorkflowContent() { targetNode.parentId === sourceNode.id ) { // This is a connection from container start to a node inside the container - always allow - logger.info('Creating container start connection:', { - edgeId, - sourceId: connection.source, - targetId: connection.target, - parentId: sourceNode.id, - }) addEdge({ ...connection, @@ -888,12 +892,6 @@ function WorkflowContent() { (!sourceParentId && targetParentId) || (sourceParentId && targetParentId && sourceParentId !== targetParentId) ) { - logger.info('Rejected cross-boundary connection:', { - sourceId: connection.source, - targetId: connection.target, - sourceParentId, - targetParentId, - }) return } @@ -901,14 +899,6 @@ function WorkflowContent() { const isInsideContainer = Boolean(sourceParentId) || Boolean(targetParentId) const parentId = sourceParentId || targetParentId - logger.info('Creating connection:', { - edgeId, - sourceId: connection.source, - targetId: connection.target, - isInsideContainer, - parentId, - }) - // Add appropriate metadata for container context addEdge({ ...connection, @@ -1087,12 +1077,6 @@ function WorkflowContent() { // Store the original parent ID when starting to drag const currentParentId = node.parentId || blocks[node.id]?.data?.parentId || null setDragStartParentId(currentParentId) - - logger.info('Node drag started', { - nodeId: node.id, - startParentId: currentParentId, - nodeType: node.type, - }) }, [blocks] ) @@ -1109,13 +1093,6 @@ function WorkflowContent() { // Don't process if the node hasn't actually changed parent or is being moved within same parent if (potentialParentId === dragStartParentId) return - logger.info('Node drag stopped', { - nodeId: node.id, - dragStartParentId, - potentialParentId, - nodeType: node.type, - }) - // Check if this is a starter block - starter blocks should never be in containers const isStarterBlock = node.data?.type === 'starter' if (isStarterBlock) { @@ -1180,16 +1157,6 @@ function WorkflowContent() { // Create a unique identifier that combines edge ID and parent context const contextId = `${edge.id}${parentLoopId ? `-${parentLoopId}` : ''}` - logger.info('Edge selected:', { - edgeId: edge.id, - sourceId: edge.source, - targetId: edge.target, - sourceNodeParent: sourceNode?.parentId, - targetNodeParent: targetNode?.parentId, - parentLoopId, - contextId, - }) - setSelectedEdgeInfo({ id: edge.id, parentLoopId, @@ -1223,11 +1190,6 @@ function WorkflowContent() { parentLoopId, onDelete: (edgeId: string) => { // Log deletion for debugging - logger.info('Deleting edge:', { - edgeId, - fromSelection: selectedEdgeInfo?.id === edgeId, - contextId: edgeContextId, - }) // Only delete this specific edge removeEdge(edgeId) @@ -1245,12 +1207,6 @@ function WorkflowContent() { useEffect(() => { const handleKeyDown = (event: KeyboardEvent) => { if ((event.key === 'Delete' || event.key === 'Backspace') && selectedEdgeInfo) { - logger.info('Keyboard shortcut edge deletion:', { - edgeId: selectedEdgeInfo.id, - parentLoopId: selectedEdgeInfo.parentLoopId, - contextId: selectedEdgeInfo.contextId, - }) - // Only delete the specific selected edge removeEdge(selectedEdgeInfo.id) setSelectedEdgeInfo(null)