diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index b575e83c83f..44609815540 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3838,6 +3838,19 @@ "canvas.selection.toolbar.group": "Group nodes", "canvas.selection.toolbar.ungroup": "Ungroup", "canvas.nodeGroup.defaultTitle": "Group", + "canvas.nodeGroup.connectionAddBlocked.title": "Connection not added", + "canvas.nodeGroup.connectionRemoveBlocked.title": "Connection not removed", + "canvas.nodeGroup.connectionChangeBlocked.message": "This change would break '{group}'. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.nonMainBoundary": "The sub-node connection between '{source}' and '{target}' crosses the boundary of '{group}'. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.multipleInputBranches": "'{group}' can have only one main input branch. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.multipleOutputBranches": "'{group}' can have only one main output branch. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.multipleInputNodes": "'{group}' can have only one entry point. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.inputEdgeToNonRoot": "Connections into '{group}' must go to its first node. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.multipleOutputNodes": "'{group}' can have only one exit point. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.outputEdgeFromNonLeaf": "Connections out of '{group}' must come from its last node. You can make this change after ungrouping it.", + "canvas.nodeGroup.connectionChangeBlocked.noContinuousPathFromRootToLeaf": "'{group}' must stay connected from first node to last node. You can make this change after ungrouping it.", + "canvas.nodeGroup.autoExtended.title": "'{group}' extended", + "canvas.nodeGroup.autoExtended.message": "Added '{node}' to keep '{group}' valid with the new connection.", "canvas.nodeGroup.titlePlaceholder": "Group name", "workflowExtraction.error.failure": "Sub-workflow conversion failed", "workflowExtraction.error.selectionGraph.inputEdgeToNonRoot": "Non-input node '{node}' has a connection from a node outside the current selection.", diff --git a/packages/frontend/editor-ui/src/app/composables/canvasConnectionReplacement.utils.ts b/packages/frontend/editor-ui/src/app/composables/canvasConnectionReplacement.utils.ts new file mode 100644 index 00000000000..121837771c6 --- /dev/null +++ b/packages/frontend/editor-ui/src/app/composables/canvasConnectionReplacement.utils.ts @@ -0,0 +1,278 @@ +import type { Connection } from '@vue-flow/core'; +import uniq from 'lodash/uniq'; +import type { IConnection, IConnections, NodeConnectionType } from 'n8n-workflow'; +import { NodeConnectionTypes } from 'n8n-workflow'; + +import type { INodeUi } from '@/Interface'; +import { CanvasConnectionMode } from '@/features/workflows/canvas/canvas.types'; +import { + createCanvasConnectionHandleString, + mapCanvasConnectionToLegacyConnection, + mapLegacyConnectionToCanvasConnection, + parseCanvasConnectionHandleString, +} from '@/features/workflows/canvas/canvas.utils'; + +type WorkflowDocumentConnectionAccess = { + connectionsBySourceNode: IConnections; + getNodeById: (id: string) => INodeUi | undefined; +}; + +type MappedCanvasConnection = { + sourceNode: INodeUi; + targetNode: INodeUi; + mappedConnection: [IConnection, IConnection]; +}; + +export type CanvasConnectionReplacement = { + connectionToRemove: Connection; + addBeforeRemoval: Connection[]; + addAfterRemoval: Connection[]; + trackHistory?: boolean; +}; + +export type NodeConnectionReplacements = { + connectionsToRemove: Connection[]; + connectionsToAdd: Connection[]; +}; + +type CanvasConnectionReplacementDependencies = { + workflowDocumentStore: WorkflowDocumentConnectionAccess; + createConnection: ( + connection: Connection, + options?: { + trackHistory?: boolean; + keepPristine?: boolean; + validateNodeGroups?: boolean; + }, + ) => void; + deleteConnection: ( + connection: Connection, + options?: { + trackHistory?: boolean; + trackBulk?: boolean; + validateNodeGroups?: boolean; + }, + ) => void; + isConnectionAllowed: ( + sourceNode: INodeUi, + targetNode: INodeUi, + sourceConnection: IConnection, + targetConnection: IConnection, + ) => boolean; + isConnectionReplacementAllowedForNodeGroups: (replacement: { + nodeIds: string[]; + connectionsToRemove: Array<[IConnection, IConnection]>; + connectionsToAdd: Array<[IConnection, IConnection]>; + connectionsBySourceNode: IConnections; + }) => boolean; +}; + +type ReplaceCanvasConnectionInput = CanvasConnectionReplacement & + CanvasConnectionReplacementDependencies; + +type GetNodeConnectionReplacementsInput = { + previousNode: INodeUi; + newNode: INodeUi; + connectionPairs: Array<[IConnection, IConnection]>; + getNodeByName: (name: string) => INodeUi | null | undefined; + isConnectionAllowed: ( + sourceNode: INodeUi, + targetNode: INodeUi, + sourceConnection: IConnection, + targetConnection: IConnection, + ) => boolean; +}; + +export function createInputConnectionHandle(type: NodeConnectionType) { + return createCanvasConnectionHandleString({ + type, + index: 0, + mode: CanvasConnectionMode.Input, + }); +} + +export function createMainOutputConnectionHandle() { + return createCanvasConnectionHandleString({ + type: NodeConnectionTypes.Main, + index: 0, + mode: CanvasConnectionMode.Output, + }); +} + +export function getPrimaryConnectionForNewNode( + node: INodeUi, + lastInteractedWithNodeId: string, + lastInteractedWithNodeHandle: string | null, +): Connection { + if (!lastInteractedWithNodeHandle) { + return { + source: lastInteractedWithNodeId, + sourceHandle: createMainOutputConnectionHandle(), + target: node.id, + targetHandle: createInputConnectionHandle(NodeConnectionTypes.Main), + }; + } + + const { type: connectionType, mode } = parseCanvasConnectionHandleString( + lastInteractedWithNodeHandle, + ); + const nodeHandle = createInputConnectionHandle(connectionType); + + if (mode === CanvasConnectionMode.Input) { + return { + source: node.id, + sourceHandle: nodeHandle, + target: lastInteractedWithNodeId, + targetHandle: lastInteractedWithNodeHandle, + }; + } + + return { + source: lastInteractedWithNodeId, + sourceHandle: lastInteractedWithNodeHandle, + target: node.id, + targetHandle: nodeHandle, + }; +} + +export function getNodeConnectionReplacements({ + previousNode, + newNode, + connectionPairs, + getNodeByName, + isConnectionAllowed, +}: GetNodeConnectionReplacementsInput): NodeConnectionReplacements { + const connectionsToRemove: Connection[] = []; + const connectionsToAdd: Connection[] = []; + + for (const pair of connectionPairs) { + const sourceNode = getNodeByName(pair[0].node); + const targetNode = getNodeByName(pair[1].node); + if (!sourceNode || !targetNode) continue; + + connectionsToRemove.push(mapLegacyConnectionToCanvasConnection(sourceNode, targetNode, pair)); + + const newSourceIConnection = { + ...pair[0], + node: pair[0].node === previousNode.name ? newNode.name : pair[0].node, + }; + const newTargetIConnection = { + ...pair[1], + node: pair[1].node === previousNode.name ? newNode.name : pair[1].node, + }; + + const newSourceNode = sourceNode.name === previousNode.name ? newNode : sourceNode; + const newTargetNode = targetNode.name === previousNode.name ? newNode : targetNode; + + if ( + !isConnectionAllowed(newSourceNode, newTargetNode, newSourceIConnection, newTargetIConnection) + ) { + continue; + } + + connectionsToAdd.push( + mapLegacyConnectionToCanvasConnection(newSourceNode, newTargetNode, [ + newSourceIConnection, + newTargetIConnection, + ]), + ); + } + + return { connectionsToRemove, connectionsToAdd }; +} + +export function mapCanvasConnectionsToLegacyConnections( + connections: Connection[], + workflowDocumentStore: WorkflowDocumentConnectionAccess, +): Array<[IConnection, IConnection]> | undefined { + const mappedConnections = getMappedCanvasConnections(connections, workflowDocumentStore); + return mappedConnections?.map(({ mappedConnection }) => mappedConnection); +} + +export function replaceCanvasConnection({ + workflowDocumentStore, + createConnection, + deleteConnection, + isConnectionAllowed, + isConnectionReplacementAllowedForNodeGroups, + connectionToRemove, + addBeforeRemoval, + addAfterRemoval, + trackHistory = false, +}: ReplaceCanvasConnectionInput): boolean { + const removal = getMappedCanvasConnection(connectionToRemove, workflowDocumentStore); + if (!removal) return false; + + const additions = getMappedCanvasConnections( + [...addBeforeRemoval, ...addAfterRemoval], + workflowDocumentStore, + ); + if (!additions) return false; + + const areAdditionsAllowed = additions.every( + ({ sourceNode, targetNode, mappedConnection: [sourceConnection, targetConnection] }) => + isConnectionAllowed(sourceNode, targetNode, sourceConnection, targetConnection), + ); + if (!areAdditionsAllowed) return false; + + const nodeIds = uniq([ + removal.sourceNode.id, + removal.targetNode.id, + ...additions.flatMap(({ sourceNode, targetNode }) => [sourceNode.id, targetNode.id]), + ]); + + const isReplacementAllowed = isConnectionReplacementAllowedForNodeGroups({ + nodeIds, + connectionsToRemove: [removal.mappedConnection], + connectionsToAdd: additions.map(({ mappedConnection }) => mappedConnection), + connectionsBySourceNode: workflowDocumentStore.connectionsBySourceNode, + }); + if (!isReplacementAllowed) return false; + + for (const connection of addBeforeRemoval) { + createConnection(connection, { trackHistory, validateNodeGroups: false }); + } + + deleteConnection(connectionToRemove, { + trackHistory, + trackBulk: false, + validateNodeGroups: false, + }); + + for (const connection of addAfterRemoval) { + createConnection(connection, { trackHistory, validateNodeGroups: false }); + } + + return true; +} + +function getMappedCanvasConnection( + connection: Connection, + workflowDocumentStore: WorkflowDocumentConnectionAccess, +): MappedCanvasConnection | undefined { + const sourceNode = workflowDocumentStore.getNodeById(connection.source); + const targetNode = workflowDocumentStore.getNodeById(connection.target); + if (!sourceNode || !targetNode) return undefined; + + return { + sourceNode, + targetNode, + mappedConnection: mapCanvasConnectionToLegacyConnection(sourceNode, targetNode, connection), + }; +} + +function getMappedCanvasConnections( + connections: Connection[], + workflowDocumentStore: WorkflowDocumentConnectionAccess, +): MappedCanvasConnection[] | undefined { + const mappedConnections: MappedCanvasConnection[] = []; + + for (const connection of connections) { + const mappedConnection = getMappedCanvasConnection(connection, workflowDocumentStore); + if (!mappedConnection) return undefined; + + mappedConnections.push(mappedConnection); + } + + return mappedConnections; +} diff --git a/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts b/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts index f67baac731f..150073ecc7c 100644 --- a/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts @@ -8,6 +8,7 @@ import type { INodeConnections, WorkflowExecuteMode, Workflow, + IWorkflowGroup, } from 'n8n-workflow'; import { NodeConnectionTypes, NodeHelpers, UserError, TelemetryHelpers } from 'n8n-workflow'; import type { CanvasConnection, CanvasNode } from '@/features/workflows/canvas/canvas.types'; @@ -36,6 +37,7 @@ import { useCredentialsStore } from '@/features/credentials/credentials.store'; import { useExecutionsStore } from '@/features/execution/executions/executions.store'; import { useNodeCreatorStore } from '@/features/shared/nodeCreator/nodeCreator.store'; import { useProjectsStore } from '@/features/collaboration/projects/projects.store'; +import { usePostHog } from '@/app/stores/posthog.store'; import { waitFor } from '@testing-library/vue'; import { createTestingPinia } from '@pinia/testing'; import { mockedStore } from '@/__tests__/utils'; @@ -55,7 +57,7 @@ import { STORES } from '@n8n/stores'; import type { Connection } from '@vue-flow/core'; import { useClipboard } from '@vueuse/core'; import { createCanvasConnectionHandleString } from '@/features/workflows/canvas/canvas.utils'; -import { nextTick, reactive, ref } from 'vue'; +import { isVNode, nextTick, reactive, ref } from 'vue'; import type { CanvasLayoutEvent } from '@/features/workflows/canvas/composables/useCanvasLayout'; import { useTelemetry } from './useTelemetry'; import { useToast } from '@/app/composables/useToast'; @@ -234,6 +236,8 @@ describe('useCanvasOperations', () => { createWorkflowDocumentId(workflowsStore.workflowId), ) as WritableDocumentStore; + mockedStore(usePostHog).isFeatureEnabled.mockReturnValue(true); + // These actions are stubbed by createTestingPinia, so provide safe defaults. // Tests that need custom behavior can override via vi.spyOn. vi.mocked(workflowDocumentStoreInstance.getParentNodesByDepth).mockReturnValue([]); @@ -2164,6 +2168,167 @@ describe('useCanvasOperations', () => { }); }); + type TestNode = ReturnType; + type TestConnectionOptions = { + sourceType?: IConnection['type']; + targetType?: IConnection['type']; + sourceIndex?: number; + targetIndex?: number; + }; + + function createGroupedNode(id: string, name: string, position?: TestNode['position']): TestNode { + return createTestNode({ id, name, type: SET_NODE_TYPE, ...(position ? { position } : {}) }); + } + + function nodeEndpoint( + node: TestNode, + type: IConnection['type'] = NodeConnectionTypes.Main, + index = 0, + ): IConnection { + return { index, node: node.name, type }; + } + + function workflowConnection( + sourceNode: TestNode, + targetNode: TestNode, + options: TestConnectionOptions = {}, + ): [IConnection, IConnection] { + const { + sourceType = NodeConnectionTypes.Main, + targetType = sourceType, + sourceIndex = 0, + targetIndex = 0, + } = options; + + return [ + nodeEndpoint(sourceNode, sourceType, sourceIndex), + nodeEndpoint(targetNode, targetType, targetIndex), + ]; + } + + function canvasConnection( + sourceNode: TestNode, + targetNode: TestNode, + options: TestConnectionOptions = {}, + ): Connection { + const { + sourceType = NodeConnectionTypes.Main, + targetType = sourceType, + sourceIndex = 0, + targetIndex = 0, + } = options; + + return { + source: sourceNode.id, + sourceHandle: `outputs/${sourceType}/${sourceIndex}`, + target: targetNode.id, + targetHandle: `inputs/${targetType}/${targetIndex}`, + }; + } + + function createConnectionsBySource( + ...connections: Array<[IConnection, IConnection]> + ): IConnections { + const connectionsBySource: IConnections = {}; + + for (const [source, target] of connections) { + const sourceConnections = (connectionsBySource[source.node] ??= {}); + const typeConnections = (sourceConnections[source.type] ??= []); + const outputConnections = (typeConnections[source.index] ??= []); + outputConnections.push(target); + } + + return connectionsBySource; + } + + function createGroupedNodeTypeDescription( + connectionTypes: IConnection['type'][] = [NodeConnectionTypes.Main], + ): INodeTypeDescription { + return mockNodeTypeDescription({ + name: SET_NODE_TYPE, + inputs: connectionTypes, + outputs: connectionTypes, + }); + } + + function mockWorkflowObjectAccessors(nodes: TestNode[], connections: IConnections) { + const workflowObject = createTestWorkflowObject({ + nodes, + connections, + }); + + vi.mocked(workflowDocumentStoreInstance.getParentNodes).mockImplementation((...args) => + workflowObject.getParentNodes(...args), + ); + vi.mocked(workflowDocumentStoreInstance.getChildNodes).mockImplementation((...args) => + workflowObject.getChildNodes(...args), + ); + vi.mocked(workflowDocumentStoreInstance.getConnectionsBetweenNodes).mockImplementation( + (...args) => workflowObject.getConnectionsBetweenNodes(...args), + ); + } + + function setupGroupedCanvas({ + nodes, + lookupNodes = nodes, + connections = {}, + groups = [], + nodeTypeDescription = createGroupedNodeTypeDescription(), + mockWorkflowObject = false, + }: { + nodes: TestNode[]; + lookupNodes?: TestNode[]; + connections?: IConnections; + groups?: IWorkflowGroup[]; + nodeTypeDescription?: INodeTypeDescription; + mockWorkflowObject?: boolean; + }) { + const nodeTypesStore = mockedStore(useNodeTypesStore); + const nodesById = new Map(lookupNodes.map((node) => [node.id, node])); + const nodesByName = new Map(lookupNodes.map((node) => [node.name, node])); + + nodeTypesStore.nodeTypes = { [SET_NODE_TYPE]: { 1: nodeTypeDescription } }; + nodeTypesStore.getNodeType = vi.fn().mockReturnValue(nodeTypeDescription); + workflowDocumentStoreInstance.allNodes = nodes; + workflowDocumentStoreInstance.connectionsBySourceNode = connections; + vi.spyOn(workflowDocumentStoreInstance, 'getNodeById').mockImplementation((id) => + nodesById.get(id), + ); + vi.spyOn(workflowDocumentStoreInstance, 'getNodeByName').mockImplementation( + (name) => nodesByName.get(name) ?? null, + ); + vi.spyOn(workflowDocumentStoreInstance, 'allGroups', 'get').mockReturnValue(groups); + vi.spyOn(workflowDocumentStoreInstance, 'getGroupForNode').mockImplementation((nodeId) => + groups.find((group) => group.nodeIds.includes(nodeId)), + ); + + if (mockWorkflowObject) { + mockWorkflowObjectAccessors(nodes, connections); + } + + return { workflowDocumentStore: workflowDocumentStoreInstance }; + } + + function expectConnectionAdded( + sourceNode: TestNode, + targetNode: TestNode, + options: TestConnectionOptions = {}, + ) { + expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledWith({ + connection: workflowConnection(sourceNode, targetNode, options), + }); + } + + function expectConnectionRemoved( + sourceNode: TestNode, + targetNode: TestNode, + options: TestConnectionOptions = {}, + ) { + expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalledWith({ + connection: workflowConnection(sourceNode, targetNode, options), + }); + } + describe('createConnection', () => { it('should not create a connection if source node does not exist', () => { const uiStore = mockedStore(useUIStore); @@ -2305,6 +2470,173 @@ describe('useCanvasOperations', () => { expect(uiStore.markStateDirty).not.toHaveBeenCalled(); }); + + it('adds the off-group endpoint to the group when that keeps the group valid', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const nodeD = createGroupedNode('d', 'D'); + const group = { id: 'group', nodeIds: [nodeB.id, nodeC.id], name: 'Group 1' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC, nodeD], + connections: createConnectionsBySource( + workflowConnection(nodeA, nodeB), + workflowConnection(nodeB, nodeC), + workflowConnection(nodeC, nodeD), + ), + groups: [group], + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { createConnection } = useCanvasOperations(); + createConnection(canvasConnection(nodeA, nodeC)); + + expect(addNodesToGroupSpy).toHaveBeenCalledWith(group.id, [nodeA.id]); + expectConnectionAdded(nodeA, nodeC); + expect(toast.showToast).toHaveBeenCalledWith( + expect.objectContaining({ + title: "'Group 1' extended", + message: "Added 'A' to keep 'Group 1' valid with the new connection.", + type: 'info', + }), + ); + expect(toast.showToast).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })); + }); + + it('should skip node group validation when the grouping feature flag is disabled', () => { + mockedStore(usePostHog).isFeatureEnabled.mockReturnValue(false); + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const nodeD = createGroupedNode('d', 'D'); + const group = { id: 'group', nodeIds: [nodeB.id, nodeC.id], name: 'Group 1' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC, nodeD], + connections: createConnectionsBySource( + workflowConnection(nodeA, nodeB), + workflowConnection(nodeB, nodeC), + workflowConnection(nodeC, nodeD), + ), + groups: [group], + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { createConnection } = useCanvasOperations(); + createConnection(canvasConnection(nodeA, nodeC)); + + expect(addNodesToGroupSpy).not.toHaveBeenCalled(); + expectConnectionAdded(nodeA, nodeC); + expect(toast.showToast).not.toHaveBeenCalled(); + }); + + it('allows a main connection across the group boundary when the group stays valid', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const group = { id: 'group', nodeIds: [nodeA.id, nodeB.id], name: 'Group' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC], + connections: createConnectionsBySource( + workflowConnection(nodeA, nodeB), + workflowConnection(nodeB, nodeC), + ), + groups: [group], + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { createConnection } = useCanvasOperations(); + createConnection(canvasConnection(nodeC, nodeA)); + + expect(addNodesToGroupSpy).not.toHaveBeenCalled(); + expectConnectionAdded(nodeC, nodeA); + expect(toast.showToast).not.toHaveBeenCalled(); + }); + + it('auto-extends the group when a non-main connection brings the sub-node inside', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const group = { id: 'group', nodeIds: [nodeA.id, nodeB.id], name: 'Group 1' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC], + connections: createConnectionsBySource(workflowConnection(nodeA, nodeB)), + groups: [group], + nodeTypeDescription: createGroupedNodeTypeDescription([ + NodeConnectionTypes.Main, + NodeConnectionTypes.AiTool, + ]), + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { createConnection } = useCanvasOperations(); + createConnection(canvasConnection(nodeC, nodeB, { sourceType: NodeConnectionTypes.AiTool })); + + expect(addNodesToGroupSpy).toHaveBeenCalledWith(group.id, [nodeC.id]); + expectConnectionAdded(nodeC, nodeB, { sourceType: NodeConnectionTypes.AiTool }); + expect(toast.showToast).toHaveBeenCalledWith( + expect.objectContaining({ + title: "'Group 1' extended", + type: 'info', + }), + ); + expect(toast.showToast).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })); + }); + + it('does not auto-extend when a non-main connection stays inside the group', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const group = { id: 'group', nodeIds: [nodeA.id, nodeB.id, nodeC.id], name: 'Group' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC], + connections: createConnectionsBySource(workflowConnection(nodeA, nodeB)), + groups: [group], + nodeTypeDescription: createGroupedNodeTypeDescription([ + NodeConnectionTypes.Main, + NodeConnectionTypes.AiTool, + ]), + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { createConnection } = useCanvasOperations(); + createConnection(canvasConnection(nodeC, nodeB, { sourceType: NodeConnectionTypes.AiTool })); + + expect(addNodesToGroupSpy).not.toHaveBeenCalled(); + expectConnectionAdded(nodeC, nodeB, { sourceType: NodeConnectionTypes.AiTool }); + expect(toast.showToast).not.toHaveBeenCalled(); + }); + + it('does not auto-extend when the off-group endpoint already belongs to another group', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const nodeD = createGroupedNode('d', 'D'); + const groupOne = { id: 'one', nodeIds: [nodeA.id], name: 'Group 1' }; + const groupTwo = { id: 'two', nodeIds: [nodeB.id, nodeC.id], name: 'Group 2' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC, nodeD], + connections: createConnectionsBySource( + workflowConnection(nodeA, nodeB), + workflowConnection(nodeB, nodeC), + workflowConnection(nodeC, nodeD), + ), + groups: [groupOne, groupTwo], + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { createConnection } = useCanvasOperations(); + createConnection(canvasConnection(nodeA, nodeC)); + + expect(addNodesToGroupSpy).not.toHaveBeenCalled(); + expect(workflowDocumentStoreInstance.addConnection).not.toHaveBeenCalled(); + expect(toast.showToast).toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })); + }); }); describe('revertCreateConnection', () => { @@ -2323,6 +2655,32 @@ describe('useCanvasOperations', () => { expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalled(); }); + + it('bypasses group validation when undoing an auto-extended connection add', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const group = { id: 'group', nodeIds: [nodeA.id, nodeB.id, nodeC.id], name: 'Group 1' }; + const connection = workflowConnection(nodeA, nodeC); + + setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC], + connections: createConnectionsBySource( + workflowConnection(nodeA, nodeB), + workflowConnection(nodeA, nodeC), + ), + groups: [group], + }); + + const { revertCreateConnection } = useCanvasOperations(); + revertCreateConnection(connection); + + expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalledWith({ + connection, + }); + expect(toast.showToast).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })); + }); }); describe('isConnectionAllowed', () => { @@ -3144,6 +3502,66 @@ describe('useCanvasOperations', () => { }); }); + it('should not delete a connection that would make an affected node group invalid', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const group = { id: 'group', nodeIds: [nodeA.id, nodeB.id], name: 'Group' }; + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB], + connections: createConnectionsBySource(workflowConnection(nodeA, nodeB)), + groups: [group], + }); + const deleteGroupSpy = vi.spyOn(workflowDocumentStore, 'deleteGroup'); + + const { deleteConnection } = useCanvasOperations(); + deleteConnection(canvasConnection(nodeA, nodeB)); + + expect(workflowDocumentStoreInstance.removeConnection).not.toHaveBeenCalled(); + expect(toast.showToast).toHaveBeenCalledWith( + expect.objectContaining({ + title: 'Connection not removed', + type: 'error', + }), + ); + + const toastConfig = vi.mocked(toast.showToast).mock.calls.at(-1)?.[0]; + expect(toastConfig).toBeDefined(); + expect(isVNode(toastConfig?.message)).toBe(true); + if (!toastConfig || !isVNode(toastConfig.message)) { + throw new Error('Expected toast message to be a vnode'); + } + + const messageChildren = toastConfig.message.children; + if (!Array.isArray(messageChildren)) { + throw new Error('Expected toast message children to be an array'); + } + + expect(messageChildren[0]).toBe( + "'Group' must stay connected from first node to last node. You can make this change after ungrouping it.", + ); + const ungroupAction = messageChildren[2]; + expect(isVNode(ungroupAction)).toBe(true); + if (!isVNode(ungroupAction)) { + throw new Error('Expected ungroup action to be a vnode'); + } + + expect(ungroupAction.children).toBe('Ungroup'); + expect(ungroupAction.props).toEqual( + expect.objectContaining({ href: '#', class: 'primary-color' }), + ); + + const event = { + preventDefault: vi.fn(), + stopPropagation: vi.fn(), + } as unknown as MouseEvent; + ungroupAction.props?.onClick(event); + + expect(event.preventDefault).toHaveBeenCalled(); + expect(event.stopPropagation).toHaveBeenCalled(); + expect(deleteGroupSpy).toHaveBeenCalledWith(group.id); + }); + it('should update node input issues for both nodes after deleting connection', async () => { const nodeA = createTestNode({ id: 'a', @@ -3187,16 +3605,77 @@ describe('useCanvasOperations', () => { describe('revertDeleteConnection', () => { it('should revert delete connection', () => { + const nodeTypesStore = mockedStore(useNodeTypesStore); + const sourceNode = createTestNode({ + id: 'source', + name: 'sourceNode', + type: SET_NODE_TYPE, + }); + const targetNode = createTestNode({ + id: 'target', + name: 'targetNode', + type: SET_NODE_TYPE, + }); const connection: [IConnection, IConnection] = [ - { node: 'sourceNode', type: NodeConnectionTypes.Main, index: 1 }, - { node: 'targetNode', type: NodeConnectionTypes.Main, index: 2 }, + { node: sourceNode.name, type: NodeConnectionTypes.Main, index: 1 }, + { node: targetNode.name, type: NodeConnectionTypes.Main, index: 2 }, ]; + nodeTypesStore.getNodeType = vi.fn().mockReturnValue( + mockNodeTypeDescription({ + name: SET_NODE_TYPE, + inputs: [NodeConnectionTypes.Main, NodeConnectionTypes.Main, NodeConnectionTypes.Main], + outputs: [NodeConnectionTypes.Main, NodeConnectionTypes.Main, NodeConnectionTypes.Main], + }), + ); + workflowDocumentStoreInstance.allNodes = [sourceNode, targetNode]; + vi.spyOn(workflowDocumentStoreInstance, 'getNodeByName').mockImplementation( + (name) => + ({ + [sourceNode.name]: sourceNode, + [targetNode.name]: targetNode, + })[name] ?? null, + ); + vi.spyOn(workflowDocumentStoreInstance, 'getNodeById').mockImplementation( + (id) => + ({ + [sourceNode.id]: sourceNode, + [targetNode.id]: targetNode, + })[id], + ); + const { revertDeleteConnection } = useCanvasOperations(); revertDeleteConnection(connection); expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledWith({ connection }); }); + + it('replays group auto-extension when redoing a connection add', () => { + const toast = useToast(); + const nodeA = createGroupedNode('a', 'A'); + const nodeB = createGroupedNode('b', 'B'); + const nodeC = createGroupedNode('c', 'C'); + const nodeD = createGroupedNode('d', 'D'); + const group = { id: 'group', nodeIds: [nodeB.id, nodeC.id], name: 'Group 1' }; + const connection = workflowConnection(nodeA, nodeC); + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [nodeA, nodeB, nodeC, nodeD], + connections: createConnectionsBySource( + workflowConnection(nodeA, nodeB), + workflowConnection(nodeB, nodeC), + workflowConnection(nodeC, nodeD), + ), + groups: [group], + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + const { revertDeleteConnection } = useCanvasOperations(); + revertDeleteConnection(connection); + + expect(addNodesToGroupSpy).toHaveBeenCalledWith(group.id, [nodeA.id]); + expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledWith({ connection }); + expect(toast.showToast).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })); + }); }); describe('revalidateNodeInputConnections', () => { @@ -5972,6 +6451,87 @@ describe('useCanvasOperations', () => { expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalledTimes(2); expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledTimes(2); }); + + describe('node group validation', () => { + const firstNode = createTestNode({ + id: 'first', + name: 'First Node', + type: SET_NODE_TYPE, + }); + const selectedStartNode = createTestNode({ + id: 'selected-start', + name: 'Selected Start Node', + type: SET_NODE_TYPE, + }); + const selectedEndNode = createTestNode({ + id: 'selected-end', + name: 'Selected End Node', + type: SET_NODE_TYPE, + }); + const lastNode = createTestNode({ + id: 'last', + name: 'Last Node', + type: SET_NODE_TYPE, + }); + const replacementNode = createTestNode({ + id: 'replacement', + name: 'Replacement Node', + type: SET_NODE_TYPE, + }); + const group = { + id: 'group', + nodeIds: [firstNode.id, selectedStartNode.id, selectedEndNode.id, lastNode.id], + name: 'Group 1', + }; + + beforeEach(() => { + const nodes = [firstNode, selectedStartNode, selectedEndNode, lastNode, replacementNode]; + + setupGroupedCanvas({ + nodes, + connections: createConnectionsBySource( + workflowConnection(firstNode, selectedStartNode), + workflowConnection(selectedStartNode, selectedEndNode), + workflowConnection(selectedEndNode, lastNode), + ), + groups: [group], + mockWorkflowObject: true, + }); + }); + + it('should not add duplicate replacement connections when a grouped replacement is blocked', () => { + const toast = useToast(); + const { replaceNodeConnections } = useCanvasOperations(); + + replaceNodeConnections(selectedEndNode.id, replacementNode.id, { + replaceInputs: false, + }); + + expect(workflowDocumentStoreInstance.removeConnection).not.toHaveBeenCalled(); + expect(workflowDocumentStoreInstance.addConnection).not.toHaveBeenCalled(); + expect(toast.showToast).toHaveBeenCalledWith( + expect.objectContaining({ title: 'Connection not added', type: 'error' }), + ); + }); + + it('should replace grouped connections without per-edge validation for graph rewrites', () => { + const toast = useToast(); + const { replaceNodeConnections } = useCanvasOperations(); + + replaceNodeConnections(selectedEndNode.id, replacementNode.id, { + replaceInputs: false, + validateNodeGroups: false, + }); + + expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalledTimes(1); + expectConnectionRemoved(selectedEndNode, lastNode); + expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledTimes(1); + expectConnectionAdded(replacementNode, lastNode); + expect(toast.showToast).not.toHaveBeenCalledWith( + expect.objectContaining({ type: 'error' }), + ); + }); + }); }); describe('replaceNode', () => { const sourceNode = createTestNode({ id: 'source', name: 'Source Node' }); @@ -6149,6 +6709,122 @@ describe('useCanvasOperations', () => { expect(historyStore.stopRecordingUndo).toHaveBeenCalled(); }); }); + describe('node group validation', () => { + it('should replace a grouped node against the final graph and transfer group membership', () => { + const toast = useToast(); + const group = { + id: 'group', + nodeIds: [sourceNode.id, targetNode.id, nextNode.id], + name: 'Group 1', + }; + const connections: IConnections = { + [sourceNode.name]: { + [NodeConnectionTypes.Main]: [[connectionTargetMain0]], + }, + [targetNode.name]: { + [NodeConnectionTypes.Main]: [[connectionNextMain0]], + }, + }; + const nodes = [sourceNode, targetNode, replacementNode, nextNode]; + + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes, + connections, + groups: [group], + nodeTypeDescription: createGroupedNodeTypeDescription([ + NodeConnectionTypes.Main, + NodeConnectionTypes.Main, + ]), + mockWorkflowObject: true, + }); + const replaceNodeInGroupSpy = vi + .spyOn(workflowDocumentStore, 'replaceNodeInGroup') + .mockImplementation((id, previousNodeId, newNodeId) => { + if (id !== group.id) return; + group.nodeIds = group.nodeIds.map((nodeId) => + nodeId === previousNodeId ? newNodeId : nodeId, + ); + }); + + vi.mocked(workflowDocumentStoreInstance.incomingConnectionsByNodeName).mockImplementation( + (name) => { + const sourceConnections = connections[sourceNode.name]?.[NodeConnectionTypes.Main]; + const hasSourceToTarget = sourceConnections?.[0]?.some( + (connection) => connection.node === targetNode.name, + ); + if (name === targetNode.name && hasSourceToTarget) { + return { [NodeConnectionTypes.Main]: [[connectionSourceMain0]] }; + } + + return {} as INodeConnections; + }, + ); + vi.mocked(workflowDocumentStoreInstance.outgoingConnectionsByNodeName).mockImplementation( + (name) => { + const targetConnections = connections[targetNode.name]?.[NodeConnectionTypes.Main]; + const hasTargetToNext = targetConnections?.[0]?.some( + (connection) => connection.node === nextNode.name, + ); + if (name === targetNode.name && hasTargetToNext) { + return { [NodeConnectionTypes.Main]: [[connectionNextMain0]] }; + } + + return {} as INodeConnections; + }, + ); + vi.mocked(workflowDocumentStoreInstance.removeConnection).mockImplementation( + ({ connection }: { connection: IConnection[] }) => { + const sourceData = connection[0]; + const destinationData = connection[1]; + if (!sourceData || !destinationData) return; + const sourceConnections = + connections[sourceData.node]?.[sourceData.type]?.[sourceData.index]; + if (!sourceConnections) return; + + connections[sourceData.node][sourceData.type][sourceData.index] = + sourceConnections.filter( + (connectionData) => + connectionData.node !== destinationData.node || + connectionData.type !== destinationData.type || + connectionData.index !== destinationData.index, + ); + }, + ); + vi.mocked(workflowDocumentStoreInstance.addConnection).mockImplementation( + ({ connection }: { connection: IConnection[] }) => { + const sourceData = connection[0]; + const destinationData = connection[1]; + if (!sourceData || !destinationData) return; + connections[sourceData.node] = connections[sourceData.node] ?? {}; + connections[sourceData.node][sourceData.type] = + connections[sourceData.node][sourceData.type] ?? []; + connections[sourceData.node][sourceData.type][sourceData.index] = + connections[sourceData.node][sourceData.type][sourceData.index] ?? []; + connections[sourceData.node][sourceData.type][sourceData.index]?.push(destinationData); + }, + ); + + const { replaceNode } = useCanvasOperations(); + replaceNode(targetNode.id, replacementNode.id, { trackHistory: true }); + + expect(replaceNodeInGroupSpy).toHaveBeenCalledWith( + group.id, + targetNode.id, + replacementNode.id, + ); + expect(group.nodeIds).toEqual([sourceNode.id, replacementNode.id, nextNode.id]); + expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalledTimes(2); + expectConnectionRemoved(sourceNode, targetNode); + expectConnectionRemoved(targetNode, nextNode); + expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledTimes(2); + expectConnectionAdded(sourceNode, replacementNode); + expectConnectionAdded(replacementNode, nextNode); + expect(workflowDocumentStoreInstance.removeNodeById).toHaveBeenCalledWith(targetNode.id); + expect(toast.showToast).not.toHaveBeenCalledWith( + expect.objectContaining({ type: 'error' }), + ); + }); + }); it('should not track history if flag is false', () => { const { replaceNode } = useCanvasOperations(); replaceNode(targetNode.id, replacementNode.id, { trackHistory: false }); @@ -6939,6 +7615,54 @@ describe('useCanvasOperations', () => { }); }); + describe('createConnectionToLastInteractedWithNode - grouped edge insertion', () => { + it('should replace the grouped edge and extend the group when inserting a node between grouped nodes', () => { + const uiStore = mockedStore(useUIStore); + const toast = useToast(); + const nodeTypeDescription = createGroupedNodeTypeDescription(); + const sourceNode = createGroupedNode('source', 'Source', [0, 0]); + const targetNode = createGroupedNode('target', 'Target', [500, 0]); + const insertedNode = createGroupedNode('inserted', 'Inserted', [250, 0]); + const group = { + id: 'group', + nodeIds: [sourceNode.id, targetNode.id], + name: 'Group 1', + }; + + const { workflowDocumentStore } = setupGroupedCanvas({ + nodes: [sourceNode, targetNode], + lookupNodes: [sourceNode, targetNode, insertedNode], + connections: createConnectionsBySource(workflowConnection(sourceNode, targetNode)), + groups: [group], + nodeTypeDescription, + }); + const addNodesToGroupSpy = vi.spyOn(workflowDocumentStore, 'addNodesToGroup'); + + uiStore.lastInteractedWithNodeId = sourceNode.id; + uiStore.lastInteractedWithNodeConnection = canvasConnection(sourceNode, targetNode); + uiStore.lastInteractedWithNodeHandle = `outputs/${NodeConnectionTypes.Main}/0`; + + const { addNode } = useCanvasOperations(); + addNode(insertedNode, nodeTypeDescription, { isAutoAdd: false }); + + expect(addNodesToGroupSpy).toHaveBeenCalledWith(group.id, [insertedNode.id]); + expect(workflowDocumentStoreInstance.removeConnection).toHaveBeenCalledTimes(1); + expectConnectionRemoved(sourceNode, targetNode); + expect(workflowDocumentStoreInstance.addConnection).toHaveBeenCalledTimes(2); + expectConnectionAdded(sourceNode, insertedNode); + expectConnectionAdded(insertedNode, targetNode); + expect(toast.showToast).toHaveBeenCalledWith( + expect.objectContaining({ + title: "'Group 1' extended", + type: 'info', + }), + ); + expect(toast.showToast).not.toHaveBeenCalledWith( + expect.objectContaining({ title: 'Connection not removed' }), + ); + }); + }); + describe('tryToOpenSubworkflowInNewTab', () => { let windowOpenSpy: ReturnType; diff --git a/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.ts b/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.ts index 50b6ec16b4c..e2a6ced4b52 100644 --- a/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.ts +++ b/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.ts @@ -17,6 +17,14 @@ import type { IUsedCredential } from '@/features/credentials/credentials.types'; import type { ITag } from '@n8n/rest-api-client/api/tags'; import type { IWorkflowTemplate } from '@n8n/rest-api-client/api/templates'; import type { WorkflowData, WorkflowDataUpdate } from '@n8n/rest-api-client/api/workflows'; +import { + type CanvasConnectionReplacement, + createInputConnectionHandle, + getNodeConnectionReplacements, + getPrimaryConnectionForNewNode, + mapCanvasConnectionsToLegacyConnections, + replaceCanvasConnection, +} from '@/app/composables/canvasConnectionReplacement.utils'; import { useDataSchema } from '@/app/composables/useDataSchema'; import { useExternalHooks } from '@/app/composables/useExternalHooks'; import { useI18n } from '@n8n/i18n'; @@ -131,6 +139,7 @@ import cloneDeep from 'lodash/cloneDeep'; import uniq from 'lodash/uniq'; import { useExperimentalNdvStore } from '@/features/workflows/canvas/experimental/experimentalNdv.store'; import { canvasEventBus } from '@/features/workflows/canvas/canvas.eventBus'; +import { useCanvasNodeGroupOperationGuards } from '@/features/workflows/canvas/composables/useCanvasNodeGroupOperationGuards'; import { useFocusPanelStore } from '@/app/stores/focusPanel.store'; import type { TelemetryNdvSource, TelemetryNdvType } from '@/app/types/telemetry'; import { useRoute, useRouter } from 'vue-router'; @@ -205,6 +214,11 @@ export function useCanvasOperations() { const externalHooks = useExternalHooks(); const clipboard = useClipboard(); const { uniqueNodeName } = useUniqueNodeName(); + const { + isConnectionChangeAllowedForNodeGroups, + isConnectionReplacementAllowedForNodeGroups, + isNodeReplacementAllowedForNodeGroups, + } = useCanvasNodeGroupOperationGuards(); const router = useRouter(); const route = useRoute(); @@ -407,7 +421,10 @@ export function useCanvasOperations() { await renameNode(currentName, previousName); } - function connectAdjacentNodes(id: string, { trackHistory = false } = {}) { + function connectAdjacentNodes( + id: string, + { trackHistory = false, validateNodeGroups = true } = {}, + ) { const node = workflowDocumentStore.value.getNodeById(id); if (!node) { @@ -461,20 +478,23 @@ export function useCanvasOperations() { ); } - createConnection({ - source: incomingNodeId, - sourceHandle: createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Output, - type, - index: incomingConnection.index, - }), - target: outgoingNodeId, - targetHandle: createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Input, - type, - index: outgoingConnection.index, - }), - }); + createConnection( + { + source: incomingNodeId, + sourceHandle: createCanvasConnectionHandleString({ + mode: CanvasConnectionMode.Output, + type, + index: incomingConnection.index, + }), + target: outgoingNodeId, + targetHandle: createCanvasConnectionHandleString({ + mode: CanvasConnectionMode.Input, + type, + index: outgoingConnection.index, + }), + }, + { validateNodeGroups }, + ); } } } @@ -496,7 +516,7 @@ export function useCanvasOperations() { uiStore.lastInteractedWithNodeId = undefined; } - connectAdjacentNodes(id, { trackHistory }); + connectAdjacentNodes(id, { trackHistory, validateNodeGroups: false }); deleteConnectionsByNodeId(id, { trackHistory, trackBulk: false }); workflowsStore.clearNodeExecutionData(node.name); @@ -555,25 +575,17 @@ export function useCanvasOperations() { } } - function replaceNodeConnections( - previousId: string, - newId: string, - { trackHistory = false, trackBulk = true, replaceInputs = true, replaceOutputs = true } = {}, - ) { - const previousNode = workflowDocumentStore.value.getNodeById(previousId); - const newNode = workflowDocumentStore.value.getNodeById(newId); - - if (!previousNode || !newNode) { - return; - } - + function getNodeConnectionPairs( + previousNode: INodeUi, + { replaceInputs = true, replaceOutputs = true } = {}, + ): Array<[IConnection, IConnection]> { const inputNodeNames = replaceInputs ? uniq(workflowDocumentStore.value.getParentNodes(previousNode.name, 'ALL', 1)) : []; const outputNodeNames = replaceOutputs ? uniq(workflowDocumentStore.value.getChildNodes(previousNode.name, 'ALL', 1)) : []; - const connectionPairs = [ + return [ ...(workflowDocumentStore.value.getConnectionsBetweenNodes(inputNodeNames, [ previousNode.name, ]) ?? []), @@ -582,6 +594,27 @@ export function useCanvasOperations() { outputNodeNames, ) ?? []), ]; + } + + function replaceNodeConnections( + previousId: string, + newId: string, + { + trackHistory = false, + trackBulk = true, + replaceInputs = true, + replaceOutputs = true, + validateNodeGroups = true, + } = {}, + ) { + const previousNode = workflowDocumentStore.value.getNodeById(previousId); + const newNode = workflowDocumentStore.value.getNodeById(newId); + + if (!previousNode || !newNode) { + return; + } + + const connectionPairs = getNodeConnectionPairs(previousNode, { replaceInputs, replaceOutputs }); if (trackHistory && trackBulk) { historyStore.startRecordingUndo(); @@ -595,7 +628,6 @@ export function useCanvasOperations() { targetNode, pair, ); - deleteConnection(oldCanvasConnection, { trackHistory, trackBulk: false }); const newSourceIConnection = { ...pair[0], @@ -616,8 +648,14 @@ export function useCanvasOperations() { newSourceIConnection, newTargetIConnection, ) - ) + ) { + deleteConnection(oldCanvasConnection, { + trackHistory, + trackBulk: false, + validateNodeGroups, + }); continue; + } const newCanvasConnection = mapLegacyConnectionToCanvasConnection( newSourceNode, @@ -625,7 +663,29 @@ export function useCanvasOperations() { [newSourceIConnection, newTargetIConnection], ); - createConnection(newCanvasConnection, { trackHistory }); + let didReplace = false; + if (validateNodeGroups) { + didReplace = replaceConnectionWithConnections({ + connectionToRemove: oldCanvasConnection, + addBeforeRemoval: [], + addAfterRemoval: [newCanvasConnection], + trackHistory, + }); + } else { + deleteConnection(oldCanvasConnection, { + trackHistory, + trackBulk: false, + validateNodeGroups: false, + }); + createConnection(newCanvasConnection, { + trackHistory, + validateNodeGroups: false, + }); + didReplace = true; + } + + if (!didReplace) continue; + revalidateNodeInputConnections(newCanvasConnection.target); revalidateNodeOutputConnections(newCanvasConnection.source); } @@ -635,6 +695,70 @@ export function useCanvasOperations() { } } + function replaceGroupedNodeConnections( + previousNode: INodeUi, + newNode: INodeUi, + { trackHistory = false } = {}, + ): boolean { + const group = workflowDocumentStore.value.getGroupForNode(previousNode.id); + if (!group) return false; + + const replacement = getNodeConnectionReplacements({ + previousNode, + newNode, + connectionPairs: getNodeConnectionPairs(previousNode), + getNodeByName: workflowDocumentStore.value.getNodeByName, + isConnectionAllowed, + }); + const connectionsToRemove = mapCanvasConnectionsToLegacyConnections( + replacement.connectionsToRemove, + workflowDocumentStore.value, + ); + const connectionsToAdd = mapCanvasConnectionsToLegacyConnections( + replacement.connectionsToAdd, + workflowDocumentStore.value, + ); + if (!connectionsToRemove || !connectionsToAdd) return false; + + const nodeIds = uniq([ + previousNode.id, + newNode.id, + ...replacement.connectionsToRemove.flatMap(({ source, target }) => [source, target]), + ...replacement.connectionsToAdd.flatMap(({ source, target }) => [source, target]), + ]); + + const isReplacementAllowed = isNodeReplacementAllowedForNodeGroups({ + previousNodeId: previousNode.id, + newNodeId: newNode.id, + nodeIds, + connectionsToRemove, + connectionsToAdd, + connectionsBySourceNode: workflowDocumentStore.value.connectionsBySourceNode, + }); + if (!isReplacementAllowed) return false; + + workflowDocumentStore.value.replaceNodeInGroup(group.id, previousNode.id, newNode.id); + + for (const connection of replacement.connectionsToRemove) { + deleteConnection(connection, { + trackHistory, + trackBulk: false, + validateNodeGroups: false, + }); + } + + for (const connection of replacement.connectionsToAdd) { + createConnection(connection, { + trackHistory, + validateNodeGroups: false, + }); + revalidateNodeInputConnections(connection.target); + revalidateNodeOutputConnections(connection.source); + } + + return true; + } + function setNodeActive(id: string, source: TelemetryNdvSource) { const node = workflowDocumentStore.value.getNodeById(id); if (!node) { @@ -973,6 +1097,17 @@ export function useCanvasOperations() { deleteNode(node.id); } + function replaceConnectionWithConnections(replacement: CanvasConnectionReplacement): boolean { + return replaceCanvasConnection({ + ...replacement, + workflowDocumentStore: workflowDocumentStore.value, + createConnection, + deleteConnection, + isConnectionAllowed, + isConnectionReplacementAllowedForNodeGroups, + }); + } + function createConnectionToLastInteractedWithNode(node: INodeUi, options: AddNodeOptions = {}) { if (!lastInteractedWithNode.value) { return; @@ -986,30 +1121,26 @@ export function useCanvasOperations() { lastInteractedWithNodeHandle, ); const nodeId = node.id; - const nodeHandle = createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Input, - type: connectionType, - index: 0, - }); - // create connection from master(e.g. agent) node to hitl node - const connectionFromHitl: Connection = { + const nodeHandle = createInputConnectionHandle(connectionType); + const connectionFromHitlToTarget: Connection = { target: lastInteractedWithNodeConnection.target, targetHandle: lastInteractedWithNodeConnection.targetHandle, source: nodeId, sourceHandle: nodeHandle, }; - createConnection(connectionFromHitl); - // delete existing connection from agent node to tool node - deleteConnection(lastInteractedWithNodeConnection); - - const connection: Connection = { + const connectionFromSourceToHitl: Connection = { source: lastInteractedWithNodeConnection.source, sourceHandle: lastInteractedWithNodeConnection.sourceHandle, target: nodeId, targetHandle: nodeHandle, }; - createConnection(connection); + replaceConnectionWithConnections({ + connectionToRemove: lastInteractedWithNodeConnection, + addBeforeRemoval: [connectionFromHitlToTarget], + addAfterRemoval: [connectionFromSourceToHitl], + trackHistory: options.trackHistory, + }); return; } @@ -1019,84 +1150,35 @@ export function useCanvasOperations() { trackBulk: false, }; - // If we have a specific endpoint to connect to - if (lastInteractedWithNodeHandle) { - const { type: connectionType, mode } = parseCanvasConnectionHandleString( - lastInteractedWithNodeHandle, - ); - - const nodeId = node.id; - const nodeHandle = createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Input, - type: connectionType, - index: 0, - }); - - if (mode === CanvasConnectionMode.Input) { - createConnection( - { - source: nodeId, - sourceHandle: nodeHandle, - target: lastInteractedWithNodeId, - targetHandle: lastInteractedWithNodeHandle, - }, - trackOptions, - ); - } else { - createConnection( - { - source: lastInteractedWithNodeId, - sourceHandle: lastInteractedWithNodeHandle, - target: nodeId, - targetHandle: nodeHandle, - }, - trackOptions, - ); - } - } else { - // If a node is last selected then connect between the active and its child ones - // Connect active node to the newly created one - createConnection( - { - source: lastInteractedWithNodeId, - sourceHandle: createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Output, - type: NodeConnectionTypes.Main, - index: 0, - }), - target: node.id, - targetHandle: createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Input, - type: NodeConnectionTypes.Main, - index: 0, - }), - }, - trackOptions, - ); - } + const primaryConnection = getPrimaryConnectionForNewNode( + node, + lastInteractedWithNodeId, + lastInteractedWithNodeHandle, + ); if (lastInteractedWithNodeConnection) { - deleteConnection(lastInteractedWithNodeConnection, trackOptions); - const targetNode = workflowDocumentStore.value.getNodeById( lastInteractedWithNodeConnection.target, ); if (targetNode) { - createConnection( - { - source: node.id, - sourceHandle: createCanvasConnectionHandleString({ - mode: CanvasConnectionMode.Input, - type: NodeConnectionTypes.Main, - index: 0, - }), - target: lastInteractedWithNodeConnection.target, - targetHandle: lastInteractedWithNodeConnection.targetHandle, - }, - trackOptions, - ); + replaceConnectionWithConnections({ + connectionToRemove: lastInteractedWithNodeConnection, + addBeforeRemoval: [primaryConnection], + addAfterRemoval: [ + { + source: node.id, + sourceHandle: createInputConnectionHandle(NodeConnectionTypes.Main), + target: lastInteractedWithNodeConnection.target, + targetHandle: lastInteractedWithNodeConnection.targetHandle, + }, + ], + trackHistory: trackOptions.trackHistory, + }); + return; } } + + createConnection(primaryConnection, trackOptions); } function trackAddNode(nodeData: INodeUi, options: AddNodeOptions, nextView?: TelemetryNdvType) { @@ -1948,7 +2030,7 @@ export function useCanvasOperations() { function createConnection( connection: Connection, - { trackHistory = false, keepPristine = false } = {}, + { trackHistory = false, keepPristine = false, validateNodeGroups = true } = {}, ) { const sourceNode = workflowDocumentStore.value.getNodeById(connection.source); const targetNode = workflowDocumentStore.value.getNodeById(connection.target); @@ -1956,15 +2038,6 @@ export function useCanvasOperations() { return; } - if (trackHistory) { - historyStore.pushCommandToUndo( - new AddConnectionCommand( - mapCanvasConnectionToLegacyConnection(sourceNode, targetNode, connection), - Date.now(), - ), - ); - } - const mappedConnection = mapCanvasConnectionToLegacyConnection( sourceNode, targetNode, @@ -1975,6 +2048,22 @@ export function useCanvasOperations() { return; } + if ( + validateNodeGroups && + !isConnectionChangeAllowedForNodeGroups({ + nodeIds: [sourceNode.id, targetNode.id], + connection: mappedConnection, + connectionsBySourceNode: workflowDocumentStore.value.connectionsBySourceNode, + action: 'add', + }) + ) { + return; + } + + if (trackHistory) { + historyStore.pushCommandToUndo(new AddConnectionCommand(mappedConnection, Date.now())); + } + workflowDocumentStore.value.addConnection({ connection: mappedConnection, }); @@ -1999,7 +2088,9 @@ export function useCanvasOperations() { return; } - deleteConnection(mapLegacyConnectionToCanvasConnection(sourceNode, targetNode, connection)); + deleteConnection(mapLegacyConnectionToCanvasConnection(sourceNode, targetNode, connection), { + validateNodeGroups: false, + }); } function deleteConnectionsByNodeId( @@ -2053,7 +2144,7 @@ export function useCanvasOperations() { index: connectionData.index, }), }, - { trackHistory, trackBulk: false }, + { trackHistory, trackBulk: false, validateNodeGroups: false }, ); } } @@ -2070,7 +2161,7 @@ export function useCanvasOperations() { function deleteConnection( connection: Connection, - { trackHistory = false, trackBulk = true } = {}, + { trackHistory = false, trackBulk = true, validateNodeGroups = true } = {}, ) { const sourceNode = workflowDocumentStore.value.getNodeById(connection.source); const targetNode = workflowDocumentStore.value.getNodeById(connection.target); @@ -2084,6 +2175,18 @@ export function useCanvasOperations() { connection, ); + if ( + validateNodeGroups && + !isConnectionChangeAllowedForNodeGroups({ + nodeIds: [sourceNode.id, targetNode.id], + connection: mappedConnection, + connectionsBySourceNode: workflowDocumentStore.value.connectionsBySourceNode, + action: 'remove', + }) + ) { + return; + } + if (trackHistory && trackBulk) { historyStore.startRecordingUndo(); } @@ -2107,9 +2210,14 @@ export function useCanvasOperations() { } function revertDeleteConnection(connection: [IConnection, IConnection]) { - workflowDocumentStore.value.addConnection({ - connection, - }); + const sourceNode = workflowDocumentStore.value.getNodeByName(connection[0].node); + const targetNode = workflowDocumentStore.value.getNodeByName(connection[1].node); + + if (!sourceNode || !targetNode) { + return; + } + + createConnection(mapLegacyConnectionToCanvasConnection(sourceNode, targetNode, connection)); } function revalidateNodeConnections(id: string, connectionMode: CanvasConnectionMode) { @@ -2150,7 +2258,7 @@ export function useCanvasOperations() { connection.data.target, ) ) { - void nextTick(() => deleteConnection(connection)); + void nextTick(() => deleteConnection(connection, { validateNodeGroups: false })); } } }); @@ -3216,12 +3324,31 @@ export function useCanvasOperations() { historyStore.startRecordingUndo(); } - const [x, y] = previousNode.position; - updateNodePosition(newId, { x, y }, { trackHistory }); - replaceNodeConnections(previousId, newId, { - trackBulk: false, - trackHistory, - }); + const moveNewNodeToPreviousPosition = () => { + const [x, y] = previousNode.position; + updateNodePosition(newId, { x, y }, { trackHistory }); + }; + + const previousGroup = workflowDocumentStore.value.getGroupForNode(previousId); + if (previousGroup) { + const didReplaceConnections = replaceGroupedNodeConnections(previousNode, newNode, { + trackHistory, + }); + if (!didReplaceConnections) { + if (trackHistory && trackBulk) { + historyStore.stopRecordingUndo(); + } + return; + } + moveNewNodeToPreviousPosition(); + } else { + moveNewNodeToPreviousPosition(); + replaceNodeConnections(previousId, newId, { + trackBulk: false, + trackHistory, + }); + } + deleteNode(previousId, { trackHistory, trackBulk: false }); uiStore.markStateDirty(); diff --git a/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.test.ts b/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.test.ts index 678517c6eb7..25f97083d1a 100644 --- a/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.test.ts @@ -168,18 +168,6 @@ describe('useSelectionValidation', () => { expect(result.valid).toBe(true); }); - it('returns too-few-nodes for a single-node grouping selection', () => { - const graph = makeLinearGraph(); - setupGraph(graph, { - 'n8n-nodes-base.set': makeNodeType({ name: 'n8n-nodes-base.set' }), - }); - - const { isSelectionGroupable } = useSelectionValidation(); - const result = isSelectionGroupable(['a']); - - expect(result).toEqual({ valid: false, reason: 'too-few-nodes' }); - }); - it('returns node-already-grouped when a selection id belongs to an existing group', () => { const graph = makeLinearGraph(); const workflowDocumentStore = setupGraph(graph, { diff --git a/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.ts b/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.ts index c6b58faa1a7..79fc1862afc 100644 --- a/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.ts +++ b/packages/frontend/editor-ui/src/app/composables/useSelectionValidation.ts @@ -1,5 +1,6 @@ import type { IConnections, + IWorkflowGroup, INodeTypeDescription, NodeGroupValidationResult, NodeSelectionValidationResult, @@ -17,6 +18,10 @@ import type { INodeUi } from '@/Interface'; export type SelectionValidationResult = NodeSelectionValidationResult; export type GroupValidationResult = NodeGroupValidationResult; +type GroupValidationOptions = { + ignoredNodeGroupIds?: string[]; +}; + export function useSelectionValidation() { const nodeTypesStore = useNodeTypesStore(); const workflowDocumentStore = injectWorkflowDocumentStore(); @@ -56,11 +61,17 @@ export function useSelectionValidation() { function isSelectionGroupable( nodeIds: string[], connectionsBySourceNode?: IConnections, + options: GroupValidationOptions = {}, ): GroupValidationResult { const store = workflowDocumentStore.value; + const ignoredNodeGroupIds = new Set(options.ignoredNodeGroupIds ?? []); + const existingNodeGroups: IWorkflowGroup[] = (store?.allGroups ?? []).filter( + (group) => !ignoredNodeGroupIds.has(group.id), + ); + return validateNodeSelectionForGrouping({ ...getValidationInput(nodeIds, connectionsBySourceNode), - existingNodeGroups: store?.allGroups ?? [], + existingNodeGroups, }); } diff --git a/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.test.ts b/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.test.ts index e1fc36e503d..94ce672e311 100644 --- a/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.test.ts @@ -26,6 +26,8 @@ const { getNodeByName: vi.fn(), getChildNodes: vi.fn().mockReturnValue([]), getExpressionHandler: vi.fn().mockReturnValue({}), + getGroupForNode: vi.fn(), + addNodesToGroup: vi.fn(), }, mockNodeTypesStore: { getNodeType: vi.fn().mockReturnValue({ @@ -127,6 +129,8 @@ describe('useWorkflowExtraction', () => { mockCanvasOperations.replaceNodeParameters.mockClear(); mockTelemetry.track.mockClear(); mockWorkflowDocumentStore.getChildNodes.mockReturnValue([]); + mockWorkflowDocumentStore.getGroupForNode.mockReset(); + mockWorkflowDocumentStore.addNodesToGroup.mockReset(); mockWorkflowDocumentStore.allNodes = []; mockWorkflowDocumentStore.connectionsBySourceNode = {}; }); diff --git a/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.ts b/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.ts index 2f1758f33f8..4035fc4172f 100644 --- a/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.ts +++ b/packages/frontend/editor-ui/src/app/composables/useWorkflowExtraction.ts @@ -27,6 +27,7 @@ import { useNodeTypesStore } from '@/app/stores/nodeTypes.store'; import { useTelemetry } from './useTelemetry'; import { checkExhaustive } from '@/app/utils/typeGuards'; import isEqual from 'lodash/isEqual'; +import uniq from 'lodash/uniq'; import { v4 as uuidv4 } from 'uuid'; import { sanitizeConnections } from '../utils/workflowUtils'; @@ -322,16 +323,23 @@ export function useWorkflowExtraction() { }) )[0]; + addReplacementNodeToSelectionGroup( + selection.map((node) => node.id), + executeWorkflowNode.id, + ); + if (endId) canvasOperations.replaceNodeConnections(endId, executeWorkflowNode.id, { ...CANVAS_HISTORY_OPTIONS, replaceInputs: false, + validateNodeGroups: false, }); if (startId) canvasOperations.replaceNodeConnections(startId, executeWorkflowNode.id, { ...CANVAS_HISTORY_OPTIONS, replaceOutputs: false, + validateNodeGroups: false, }); canvasOperations.deleteNodes( @@ -356,6 +364,18 @@ export function useWorkflowExtraction() { historyStore.stopRecordingUndo(); } + function addReplacementNodeToSelectionGroup(selectionIds: string[], replacementNodeId: string) { + const affectedGroupIds = uniq( + selectionIds + .map((nodeId) => workflowDocumentStore.value.getGroupForNode(nodeId)?.id) + .filter((id): id is string => id !== undefined), + ); + + if (affectedGroupIds.length !== 1) return; + + workflowDocumentStore.value.addNodesToGroup(affectedGroupIds[0], [replacementNodeId]); + } + function tryExtractNodesIntoSubworkflow(nodeIds: string[]): boolean { const result = isSelectionExtractable(nodeIds); diff --git a/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts b/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts index 8c43a7a8f90..931137faca2 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts @@ -74,6 +74,7 @@ describe('workflowDocument.store orchestration', () => { expect(workflowDocumentStore.allNodes).toHaveLength(0); expect(workflowDocumentStore.connectionsBySourceNode).toEqual({}); expect(workflowDocumentStore.pinnedDataByNodeName).toEqual({}); + expect(workflowDocumentStore.allGroups).toHaveLength(0); }); it('disposeWorkflowDocumentStore disposes the instance and clears scoped state', () => { @@ -383,6 +384,9 @@ describe('workflowDocument.store orchestration', () => { expect(store.allNodes).toHaveLength(2); expect(store.connectionsBySourceNode).toHaveProperty('A'); expect(store.pinnedDataByNodeName).toEqual({ A: [{ json: { foo: 'bar' } }] }); + expect(store.allGroups).toEqual([ + { id: 'group-1', name: 'Group A', nodeIds: ['node-a', 'node-b'] }, + ]); }); it('applies safe defaults for missing optional fields', () => { @@ -674,6 +678,7 @@ describe('workflowDocument.store orchestration', () => { expect(store.allNodes).toHaveLength(0); expect(store.connectionsBySourceNode).toEqual({}); expect(store.pinnedDataByNodeName).toEqual({}); + expect(store.allGroups).toEqual([]); expect(store.viewport).toBeNull(); }); }); diff --git a/packages/frontend/editor-ui/src/features/workflows/canvas/components/elements/selection/CanvasSelectionToolbar.test.ts b/packages/frontend/editor-ui/src/features/workflows/canvas/components/elements/selection/CanvasSelectionToolbar.test.ts index cd5f36ebd0f..64bbedc3c93 100644 --- a/packages/frontend/editor-ui/src/features/workflows/canvas/components/elements/selection/CanvasSelectionToolbar.test.ts +++ b/packages/frontend/editor-ui/src/features/workflows/canvas/components/elements/selection/CanvasSelectionToolbar.test.ts @@ -72,7 +72,7 @@ describe('CanvasSelectionToolbar', () => { isSelectionGroupableMock.mockImplementation(getDefaultGroupableResult); isSelectionExtractableMock.mockImplementation((nodeIds: string[]) => nodeIds.length < 2 - ? { valid: false, reason: 'too-few-nodes' } + ? { valid: false, reason: 'invalid-subgraph' } : { valid: true, subGraphData: { start: 'A', end: 'B' } }, ); expandSelectionWithSubNodesMock.mockImplementation((nodeIds: string[]) => nodeIds); diff --git a/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.test.ts b/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.test.ts index f7b78213d4e..e9c624210ef 100644 --- a/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.test.ts +++ b/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.test.ts @@ -95,7 +95,7 @@ describe('useCanvasNodeGroupActions', () => { }); it('returns null when canGroup is false', () => { - isSelectionGroupableMock.mockReturnValue({ valid: false, reason: 'too-few-nodes' }); + isSelectionGroupableMock.mockReturnValue({ valid: false, reason: 'invalid-subgraph' }); const { groupSelection } = useCanvasNodeGroupActions(computed(() => [makeNode('a')])); expect(groupSelection()).toBeNull(); }); diff --git a/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.ts b/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.ts index 8bc295279d9..d16406b82d1 100644 --- a/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.ts +++ b/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupActions.ts @@ -18,9 +18,7 @@ export function useCanvasNodeGroupActions( const isReadOnly = computed(() => toValue(options?.readOnly) ?? false); const expandedSelectionIds = computed(() => { - const nodes = toValue(selectedNodes); - if (isReadOnly.value || nodes.length < 2) return []; - return expandSelectionWithSubNodes(nodes.map((n) => n.id)); + return expandSelectionWithSubNodes(toValue(selectedNodes).map((n) => n.id)); }); const canGroup = computed(() => { diff --git a/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupOperationGuards.ts b/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupOperationGuards.ts new file mode 100644 index 00000000000..4ac7f39644f --- /dev/null +++ b/packages/frontend/editor-ui/src/features/workflows/canvas/composables/useCanvasNodeGroupOperationGuards.ts @@ -0,0 +1,423 @@ +import type { IConnection, IConnections, IWorkflowGroup } from 'n8n-workflow'; +import { computed, h } from 'vue'; +import type { NotificationHandle } from 'element-plus'; +import cloneDeep from 'lodash/cloneDeep'; +import uniq from 'lodash/uniq'; + +import { CANVAS_NODES_GROUPING_EXPERIMENT } from '@/app/constants'; +import { useI18n, type BaseTextKey } from '@n8n/i18n'; +import { + useSelectionValidation, + type GroupValidationResult, +} from '@/app/composables/useSelectionValidation'; +import { usePostHog } from '@/app/stores/posthog.store'; +import { useToast } from '@/app/composables/useToast'; +import { + createWorkflowDocumentId, + useWorkflowDocumentStore, +} from '@/app/stores/workflowDocument.store'; +import { useWorkflowsStore } from '@/app/stores/workflows.store'; + +type ConnectionChangeAction = 'add' | 'remove'; +type InvalidGroupValidationResult = Extract; +type InvalidAffectedGroup = { group: IWorkflowGroup; result: InvalidGroupValidationResult }; +type ExtractableErrorCode = NonNullable< + Extract< + InvalidGroupValidationResult, + { reason: 'invalid-subgraph' } + >['errors'][number]['errorCode'] +>; + +const BLOCKED_TITLE_KEY: Record = { + add: 'canvas.nodeGroup.connectionAddBlocked.title', + remove: 'canvas.nodeGroup.connectionRemoveBlocked.title', +}; + +const MESSAGE_KEY_BY_REASON: Partial> = + { + 'multiple-input-branches': 'canvas.nodeGroup.connectionChangeBlocked.multipleInputBranches', + 'multiple-output-branches': 'canvas.nodeGroup.connectionChangeBlocked.multipleOutputBranches', + }; + +const MESSAGE_KEY_BY_ERROR_CODE: Record = { + 'Multiple Input Nodes': 'canvas.nodeGroup.connectionChangeBlocked.multipleInputNodes', + 'Input Edge To Non-Root Node': 'canvas.nodeGroup.connectionChangeBlocked.inputEdgeToNonRoot', + 'Multiple Output Nodes': 'canvas.nodeGroup.connectionChangeBlocked.multipleOutputNodes', + 'Output Edge From Non-Leaf Node': + 'canvas.nodeGroup.connectionChangeBlocked.outputEdgeFromNonLeaf', + 'No Continuous Path From Root To Leaf In Selection': + 'canvas.nodeGroup.connectionChangeBlocked.noContinuousPathFromRootToLeaf', +}; + +const FALLBACK_MESSAGE_KEY: BaseTextKey = 'canvas.nodeGroup.connectionChangeBlocked.message'; + +export function useCanvasNodeGroupOperationGuards() { + const workflowsStore = useWorkflowsStore(); + const posthogStore = usePostHog(); + const workflowDocumentStore = computed(() => + useWorkflowDocumentStore(createWorkflowDocumentId(workflowsStore.workflowId)), + ); + const isCanvasNodeGroupingEnabled = computed(() => + posthogStore.isFeatureEnabled(CANVAS_NODES_GROUPING_EXPERIMENT.name), + ); + + const i18n = useI18n(); + const toast = useToast(); + const { isSelectionGroupable } = useSelectionValidation(); + + function applyAddConnection( + candidateConnections: IConnections, + connection: [IConnection, IConnection], + ): void { + const [sourceData, destinationData] = connection; + + candidateConnections[sourceData.node] = candidateConnections[sourceData.node] ?? {}; + const sourceNodeConnections = candidateConnections[sourceData.node]; + + sourceNodeConnections[sourceData.type] = sourceNodeConnections[sourceData.type] ?? []; + const outputConnections = sourceNodeConnections[sourceData.type]; + + while (outputConnections.length <= sourceData.index) { + outputConnections.push([]); + } + + outputConnections[sourceData.index] = outputConnections[sourceData.index] ?? []; + outputConnections[sourceData.index]?.push(destinationData); + } + + function applyRemoveConnection( + candidateConnections: IConnections, + connection: [IConnection, IConnection], + ): void { + const [sourceData, destinationData] = connection; + const outputConnections = candidateConnections[sourceData.node]?.[sourceData.type]; + const targetConnections = outputConnections?.[sourceData.index]; + if (!outputConnections || !targetConnections) return; + + outputConnections[sourceData.index] = targetConnections.filter( + (connectionData) => + connectionData.node !== destinationData.node || + connectionData.type !== destinationData.type || + connectionData.index !== destinationData.index, + ); + } + + function applyConnectionChangesToCandidate({ + connectionsBySourceNode, + connectionsToRemove = [], + connectionsToAdd = [], + }: { + connectionsBySourceNode: IConnections; + connectionsToRemove?: Array<[IConnection, IConnection]>; + connectionsToAdd?: Array<[IConnection, IConnection]>; + }): IConnections { + const candidateConnections = cloneDeep(connectionsBySourceNode); + + for (const connection of connectionsToRemove) { + applyRemoveConnection(candidateConnections, connection); + } + + for (const connection of connectionsToAdd) { + applyAddConnection(candidateConnections, connection); + } + + return candidateConnections; + } + + function getAffectedNodeGroups(nodeIds: string[]): IWorkflowGroup[] { + const affectedGroups = new Map(); + for (const nodeId of nodeIds) { + const group = workflowDocumentStore.value.getGroupForNode(nodeId); + if (group) { + affectedGroups.set(group.id, group); + } + } + + return Array.from(affectedGroups.values()); + } + + function findInvalidGroup( + affectedGroups: IWorkflowGroup[], + connectionsBySourceNode: IConnections, + getNodeIdsForGroup: (group: IWorkflowGroup) => string[] = (group) => group.nodeIds, + ): { group: IWorkflowGroup; result: InvalidGroupValidationResult } | undefined { + for (const group of affectedGroups) { + const result = isSelectionGroupable(getNodeIdsForGroup(group), connectionsBySourceNode, { + ignoredNodeGroupIds: [group.id], + }); + if (!result.valid) return { group, result }; + } + return undefined; + } + + function getConnectionChangeBlockedMessage( + group: IWorkflowGroup, + result: InvalidGroupValidationResult, + ): string { + const groupInterpolation = { group: group.name }; + + if (result.reason === 'non-main-boundary') { + return i18n.baseText('canvas.nodeGroup.connectionChangeBlocked.nonMainBoundary', { + interpolate: { + ...groupInterpolation, + source: result.connection.source, + target: result.connection.target, + }, + }); + } + + const errorCodeKey = + result.reason === 'invalid-subgraph' && result.errors[0] + ? MESSAGE_KEY_BY_ERROR_CODE[result.errors[0].errorCode] + : undefined; + + const key: BaseTextKey = + errorCodeKey ?? MESSAGE_KEY_BY_REASON[result.reason] ?? FALLBACK_MESSAGE_KEY; + return i18n.baseText(key, { interpolate: groupInterpolation }); + } + + function getConnectionChangeBlockedMessageWithAction( + group: IWorkflowGroup, + result: InvalidGroupValidationResult, + ) { + let notification: NotificationHandle | undefined; + const message = getConnectionChangeBlockedMessage(group, result); + const ungroupAction = h( + 'a', + { + href: '#', + class: 'primary-color', + onClick: (event: MouseEvent) => { + event.preventDefault(); + event.stopPropagation(); + workflowDocumentStore.value.deleteGroup(group.id); + notification?.close(); + }, + }, + i18n.baseText('canvas.selection.toolbar.ungroup'), + ); + + return { + message: h('span', [message, ' ', ungroupAction]), + setNotification: (value: NotificationHandle) => { + notification = value; + }, + }; + } + + function showConnectionChangeBlockedToast( + titleKey: BaseTextKey, + invalidAffectedGroup: InvalidAffectedGroup, + ) { + const { message, setNotification } = getConnectionChangeBlockedMessageWithAction( + invalidAffectedGroup.group, + invalidAffectedGroup.result, + ); + + const notification = toast.showToast({ + title: i18n.baseText(titleKey), + message, + type: 'error', + duration: 5000, + }); + setNotification(notification); + } + + function getAutoExtendCandidate({ + failingGroup, + endpointIds, + connectionsBySourceNode, + }: { + failingGroup: IWorkflowGroup; + endpointIds: string[]; + connectionsBySourceNode: IConnections; + }): string | undefined { + const memberSet = new Set(failingGroup.nodeIds); + const offGroupEndpoints = endpointIds.filter((id) => !memberSet.has(id)); + if (offGroupEndpoints.length !== 1) return undefined; + + const [candidateId] = offGroupEndpoints; + if (workflowDocumentStore.value.getGroupForNode(candidateId)) return undefined; + + const result = isSelectionGroupable( + [...failingGroup.nodeIds, candidateId], + connectionsBySourceNode, + { ignoredNodeGroupIds: [failingGroup.id] }, + ); + if (!result.valid) return undefined; + + return candidateId; + } + + function showAutoExtendedToast(group: IWorkflowGroup, candidateId: string) { + const candidateName = workflowDocumentStore.value.getNodeById(candidateId)?.name ?? candidateId; + + toast.showToast({ + title: i18n.baseText('canvas.nodeGroup.autoExtended.title', { + interpolate: { group: group.name }, + }), + message: i18n.baseText('canvas.nodeGroup.autoExtended.message', { + interpolate: { + node: candidateName, + group: group.name, + }, + }), + type: 'info', + duration: 5000, + }); + } + + function tryAutoExtendInvalidGroup({ + invalidAffectedGroup, + endpointIds, + connectionsBySourceNode, + }: { + invalidAffectedGroup: InvalidAffectedGroup; + endpointIds: string[]; + connectionsBySourceNode: IConnections; + }): boolean { + const candidateId = getAutoExtendCandidate({ + failingGroup: invalidAffectedGroup.group, + endpointIds, + connectionsBySourceNode, + }); + + if (candidateId === undefined) return false; + + workflowDocumentStore.value.addNodesToGroup(invalidAffectedGroup.group.id, [candidateId]); + showAutoExtendedToast(invalidAffectedGroup.group, candidateId); + + return true; + } + + function isConnectionReplacementAllowedForNodeGroups({ + nodeIds, + connectionsToRemove, + connectionsToAdd, + connectionsBySourceNode, + allowAutoExtend = true, + blockedTitleKey = BLOCKED_TITLE_KEY.add, + }: { + nodeIds: string[]; + connectionsToRemove: Array<[IConnection, IConnection]>; + connectionsToAdd: Array<[IConnection, IConnection]>; + connectionsBySourceNode: IConnections; + allowAutoExtend?: boolean; + blockedTitleKey?: BaseTextKey; + }): boolean { + if (!isCanvasNodeGroupingEnabled.value) return true; + + const affectedGroups = getAffectedNodeGroups(nodeIds); + if (affectedGroups.length === 0) return true; + + const candidateConnections = applyConnectionChangesToCandidate({ + connectionsBySourceNode, + connectionsToRemove, + connectionsToAdd, + }); + + const invalidAffectedGroup = findInvalidGroup(affectedGroups, candidateConnections); + if (!invalidAffectedGroup) return true; + + if ( + allowAutoExtend && + tryAutoExtendInvalidGroup({ + invalidAffectedGroup, + endpointIds: nodeIds, + connectionsBySourceNode: candidateConnections, + }) + ) { + return true; + } + + showConnectionChangeBlockedToast(blockedTitleKey, invalidAffectedGroup); + + return false; + } + + function isConnectionChangeAllowedForNodeGroups({ + nodeIds, + connection, + connectionsBySourceNode, + action, + }: { + nodeIds: string[]; + connection: [IConnection, IConnection]; + connectionsBySourceNode: IConnections; + action: ConnectionChangeAction; + }): boolean { + return isConnectionReplacementAllowedForNodeGroups({ + nodeIds, + connectionsToRemove: action === 'remove' ? [connection] : [], + connectionsToAdd: action === 'add' ? [connection] : [], + connectionsBySourceNode, + allowAutoExtend: action === 'add', + blockedTitleKey: BLOCKED_TITLE_KEY[action], + }); + } + + function isNodeReplacementAllowedForNodeGroups({ + previousNodeId, + newNodeId, + nodeIds, + connectionsToRemove, + connectionsToAdd, + connectionsBySourceNode, + }: { + previousNodeId: string; + newNodeId: string; + nodeIds: string[]; + connectionsToRemove: Array<[IConnection, IConnection]>; + connectionsToAdd: Array<[IConnection, IConnection]>; + connectionsBySourceNode: IConnections; + }): boolean { + if (!isCanvasNodeGroupingEnabled.value) return true; + + const previousGroup = workflowDocumentStore.value.getGroupForNode(previousNodeId); + if (!previousGroup) return true; + + const newNodeGroup = workflowDocumentStore.value.getGroupForNode(newNodeId); + if (newNodeGroup && newNodeGroup.id !== previousGroup.id) { + showConnectionChangeBlockedToast(BLOCKED_TITLE_KEY.add, { + group: previousGroup, + result: { + valid: false, + reason: 'invalid-subgraph', + errors: [], + }, + }); + return false; + } + + const affectedGroups = getAffectedNodeGroups([...nodeIds, previousNodeId, newNodeId]); + if (affectedGroups.length === 0) return true; + + const candidateConnections = applyConnectionChangesToCandidate({ + connectionsBySourceNode, + connectionsToRemove, + connectionsToAdd, + }); + const swappedPreviousGroupNodeIds = uniq( + previousGroup.nodeIds.map((nodeId) => (nodeId === previousNodeId ? newNodeId : nodeId)), + ); + const getNodeIdsForGroup = (group: IWorkflowGroup) => + group.id === previousGroup.id ? swappedPreviousGroupNodeIds : group.nodeIds; + + const invalidAffectedGroup = findInvalidGroup( + affectedGroups, + candidateConnections, + getNodeIdsForGroup, + ); + if (!invalidAffectedGroup) return true; + + showConnectionChangeBlockedToast(BLOCKED_TITLE_KEY.add, invalidAffectedGroup); + + return false; + } + + return { + isConnectionChangeAllowedForNodeGroups, + isConnectionReplacementAllowedForNodeGroups, + isNodeReplacementAllowedForNodeGroups, + }; +} diff --git a/packages/testing/playwright/tests/e2e/workflows/editor/canvas/node-groups.spec.ts b/packages/testing/playwright/tests/e2e/workflows/editor/canvas/node-groups.spec.ts index 436cd9e8a8b..77d3db13729 100644 --- a/packages/testing/playwright/tests/e2e/workflows/editor/canvas/node-groups.spec.ts +++ b/packages/testing/playwright/tests/e2e/workflows/editor/canvas/node-groups.spec.ts @@ -199,6 +199,27 @@ test.describe( await n8n.canvas.selectNodes([TRIGGER, 'Set A']); await expect(n8n.canvas.selectionToolbar.root()).toBeHidden(); }); + + test('auto-extends the group when a new connection would otherwise invalidate it', async ({ + n8n, + }) => { + await n8n.canvas.selectNodes(['Set B', 'Set C']); + await n8n.canvas.selectionToolbar.groupButton().click(); + await n8n.canvas.deselectAll(); + await expect(n8n.canvas.getNodeGroupByTitle(DEFAULT_GROUP_TITLE)).toBeVisible(); + + const before = await n8n.canvas.getNodeGroupBoundingBox(DEFAULT_GROUP_TITLE); + + await n8n.canvas.connectNodesByDrag('Set A', 'Set C'); + + await expect( + n8n.notifications.getNotificationByTitle(`'${DEFAULT_GROUP_TITLE}' extended`), + ).toBeVisible(); + await expect(n8n.canvas.connectionBetweenNodes('Set A', 'Set C')).toHaveCount(1); + + const after = await n8n.canvas.getNodeGroupBoundingBox(DEFAULT_GROUP_TITLE); + expect(after.width).toBeGreaterThan(before.width); + }); }, ); diff --git a/packages/workflow/src/node-grouping-validation.ts b/packages/workflow/src/node-grouping-validation.ts index 2985982caeb..28fd85346f6 100644 --- a/packages/workflow/src/node-grouping-validation.ts +++ b/packages/workflow/src/node-grouping-validation.ts @@ -44,7 +44,6 @@ export type NodeSelectionValidationResult = export type NodeGroupValidationResult = | NodeSelectionValidationResult - | { valid: false; reason: 'too-few-nodes' } | { valid: false; reason: 'node-already-grouped'; nodeIds: string[] } | { valid: false; @@ -110,10 +109,6 @@ export function validateNodeSelectionForExtraction({ export function validateNodeSelectionForGrouping( input: NodeGroupingValidationInput, ): NodeGroupValidationResult { - if (input.nodes.length < 2) { - return { valid: false, reason: 'too-few-nodes' }; - } - const alreadyGroupedNodeIds = findAlreadyGroupedNodeIds( input.nodes.map((node) => node.id), input.existingNodeGroups ?? [], diff --git a/packages/workflow/test/node-grouping-validation.test.ts b/packages/workflow/test/node-grouping-validation.test.ts index 8d60b5475ca..5531fe102fa 100644 --- a/packages/workflow/test/node-grouping-validation.test.ts +++ b/packages/workflow/test/node-grouping-validation.test.ts @@ -97,17 +97,6 @@ describe('node grouping validation', () => { } }); - it('returns too-few-nodes for a single-node grouping selection', () => { - const graph = makeLinearGraph(); - - const result = validateGrouping({ - nodes: [graph.nodes[0]], - connectionsBySourceNode: graph.connections, - }); - - expect(result).toEqual({ valid: false, reason: 'too-few-nodes' }); - }); - it('returns node-already-grouped when a selection id belongs to an existing group', () => { const graph = makeLinearGraph();