diff --git a/src/components/shared/ReactFlow/FlowCanvas/utils/addTask.test.ts b/src/components/shared/ReactFlow/FlowCanvas/utils/addTask.test.ts index e1c825e44..0274e04f8 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/utils/addTask.test.ts +++ b/src/components/shared/ReactFlow/FlowCanvas/utils/addTask.test.ts @@ -123,7 +123,7 @@ describe("addTask", () => { ? Object.keys(newComponentSpec.implementation.graph.tasks) : []; expect(taskIds.length).toBe(2); - expect(taskIds).toContain("TestTask 2"); + expect(taskIds).toContain("TestTask (2)"); }); it("should create unique names for inputs when duplicates exist", () => { @@ -139,7 +139,7 @@ describe("addTask", () => { }; expect(newComponentSpec.inputs.length).toBe(2); - expect(newComponentSpec.inputs[1].name).toBe("Input 2"); + expect(newComponentSpec.inputs[1].name).toBe("Input (2)"); }); it("should create unique names for outputs when duplicates exist", () => { @@ -155,6 +155,6 @@ describe("addTask", () => { }; expect(newComponentSpec.outputs.length).toBe(2); - expect(newComponentSpec.outputs[1].name).toBe("Output 2"); + expect(newComponentSpec.outputs[1].name).toBe("Output (2)"); }); }); diff --git a/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.test.ts b/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.test.ts index a38b7fcdc..493701dc7 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.test.ts +++ b/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.test.ts @@ -3,12 +3,13 @@ import { describe, expect, it, vi } from "vitest"; import { NodeManager } from "@/nodeManager"; import type { TaskNodeData } from "@/types/nodes"; -import type { - ComponentSpec, - InputSpec, - OutputSpec, - TaskOutputArgument, - TaskSpec, +import { + type ComponentSpec, + type InputSpec, + isGraphImplementation, + type OutputSpec, + type TaskOutputArgument, + type TaskSpec, } from "@/utils/componentSpec"; import { duplicateNodes } from "./duplicateNodes"; @@ -63,7 +64,7 @@ const createMockComponentSpecWithOutputs = ( acc[output.name] = { taskOutput: { taskId: "task1", - outputName: "result", + outputName: output.name, }, }; return acc; @@ -124,7 +125,7 @@ const createMockInputNode = ( position, data: { label: inputName, - inputSpec: { ...mockInputSpec, name: inputName }, + spec: { ...mockInputSpec, name: inputName }, }, selected: false, dragging: false, @@ -145,7 +146,7 @@ const createMockOutputNode = ( position, data: { label: outputName, - outputSpec: { ...mockOutputSpec, name: outputName }, + spec: { ...mockOutputSpec, name: outputName }, }, selected: false, dragging: false, @@ -211,7 +212,7 @@ describe("duplicateNodes", () => { if ("graph" in result.updatedComponentSpec.implementation!) { expect( result.updatedComponentSpec.implementation.graph.tasks, - ).toHaveProperty("original-task 2"); + ).toHaveProperty("original-task (2)"); } }); @@ -237,14 +238,14 @@ describe("duplicateNodes", () => { expect(result.newNodes).toHaveLength(1); expect(result.newNodes[0].type).toBe("input"); expect(result.newNodes[0].id).toBe( - nodeManager.getNodeId("original-input 2", "input"), + nodeManager.getNodeId("original-input (2)", "input"), ); expect(result.newNodes[0].position).toEqual({ x: 60, y: 60 }); expect(result.updatedComponentSpec.inputs).toHaveLength(2); expect( result.updatedComponentSpec.inputs?.some( - (input) => input.name === "original-input 2", + (input) => input.name === "original-input (2)", ), ).toBe(true); }); @@ -275,14 +276,14 @@ describe("duplicateNodes", () => { expect(result.newNodes).toHaveLength(1); expect(result.newNodes[0].type).toBe("output"); expect(result.newNodes[0].id).toBe( - nodeManager.getNodeId("original-output 2", "output"), + nodeManager.getNodeId("original-output (2)", "output"), ); expect(result.newNodes[0].position).toEqual({ x: 310, y: 310 }); expect(result.updatedComponentSpec.outputs).toHaveLength(2); expect( result.updatedComponentSpec.outputs?.some( - (output) => output.name === "original-output 2", + (output) => output.name === "original-output (2)", ), ).toBe(true); }); @@ -305,13 +306,13 @@ describe("duplicateNodes", () => { const result = duplicateNodes(componentSpec, nodes, nodeManager); expect(result.newNodes).toHaveLength(2); - if ("graph" in result.updatedComponentSpec.implementation!) { + if (isGraphImplementation(result.updatedComponentSpec.implementation)) { expect( result.updatedComponentSpec.implementation.graph.tasks, - ).toHaveProperty("task1 2"); + ).toHaveProperty("task1 (2)"); expect( result.updatedComponentSpec.implementation.graph.tasks, - ).toHaveProperty("task2 2"); + ).toHaveProperty("task2 (2)"); } }); }); @@ -419,7 +420,7 @@ describe("duplicateNodes", () => { if ("graph" in result.updatedComponentSpec.implementation!) { const duplicatedTask2 = - result.updatedComponentSpec.implementation.graph.tasks["task2 2"]; + result.updatedComponentSpec.implementation.graph.tasks["task2 (2)"]; expect(duplicatedTask2.arguments).toEqual({}); } }); @@ -443,10 +444,10 @@ describe("duplicateNodes", () => { if ("graph" in result.updatedComponentSpec.implementation!) { const duplicatedTask2 = - result.updatedComponentSpec.implementation.graph.tasks["task2 2"]; + result.updatedComponentSpec.implementation.graph.tasks["task2 (2)"]; expect(duplicatedTask2.arguments?.input1).toEqual({ taskOutput: { - taskId: "task1 2", + taskId: "task1 (2)", outputName: "output1", }, }); @@ -496,7 +497,7 @@ describe("duplicateNodes", () => { if ("graph" in result.updatedComponentSpec.implementation!) { const duplicatedTask2 = - result.updatedComponentSpec.implementation.graph.tasks["task2 2"]; + result.updatedComponentSpec.implementation.graph.tasks["task2 (2)"]; // Should remove internal connection to task1 (since task1 is being duplicated) expect(duplicatedTask2.arguments?.input1).toBeUndefined(); @@ -530,10 +531,10 @@ describe("duplicateNodes", () => { if ("graph" in result.updatedComponentSpec.implementation!) { const duplicatedTask2 = - result.updatedComponentSpec.implementation.graph.tasks["task2 2"]; + result.updatedComponentSpec.implementation.graph.tasks["task2 (2)"]; expect(duplicatedTask2.arguments?.input1).toEqual({ taskOutput: { - taskId: "task1 2", + taskId: "task1 (2)", outputName: "output1", }, }); @@ -571,26 +572,45 @@ describe("duplicateNodes", () => { connection: "all", }); - if ("graph" in result.updatedComponentSpec.implementation!) { + if (isGraphImplementation(result.updatedComponentSpec.implementation)) { const duplicatedTask = - result.updatedComponentSpec.implementation.graph.tasks["task1 2"]; + result.updatedComponentSpec.implementation.graph.tasks["task1 (2)"]; expect(duplicatedTask.arguments?.input1).toEqual({ graphInput: { - inputName: "graph-input 2", + inputName: "graph-input (2)", }, }); } }); it("should handle graph output connections", () => { + const outputSpec: OutputSpec = { + ...mockOutputSpec, + name: "graph-output-node", + }; + + const taskComponentSpec: ComponentSpec = { + name: "task-component", + inputs: [], + outputs: [ + { + name: "graph-output", + type: "String", + annotations: {}, + }, + ], + implementation: { + container: { image: "task-image" }, + }, + }; + const taskSpec: TaskSpec = { ...mockTaskSpec, arguments: {}, - }; - - const outputSpec: OutputSpec = { - ...mockOutputSpec, - name: "graph-output", + componentRef: { + name: "task-component", + spec: taskComponentSpec, + }, }; const componentSpec = createMockComponentSpecWithOutputs( @@ -602,7 +622,7 @@ describe("duplicateNodes", () => { const nodeManager = createMockNodeManager(); const nodes = [ createMockTaskNode("task1", taskSpec, nodeManager), - createMockOutputNode("graph-output", nodeManager), + createMockOutputNode("graph-output-node", nodeManager), ]; const result = duplicateNodes(componentSpec, nodes, nodeManager, { @@ -610,15 +630,15 @@ describe("duplicateNodes", () => { }); // Check that outputValues are updated for duplicated outputs - if ("graph" in result.updatedComponentSpec.implementation!) { + if (isGraphImplementation(result.updatedComponentSpec.implementation)) { const outputValues = result.updatedComponentSpec.implementation.graph.outputValues; // Duplicated output should reference duplicated task - expect(outputValues?.["graph-output 2"]).toEqual({ + expect(outputValues?.["graph-output-node (2)"]).toEqual({ taskOutput: { - taskId: "task1 2", - outputName: "result", + taskId: "task1 (2)", + outputName: "graph-output-node", }, }); } @@ -656,7 +676,7 @@ describe("duplicateNodes", () => { it("should handle nodes without position annotations", () => { const taskSpecWithoutPosition = { ...mockTaskSpec, - annotations: {}, // No position annotation + annotations: {}, }; const componentSpec = createMockComponentSpec({ diff --git a/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.ts b/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.ts index 914b9bdb6..fc160146b 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.ts +++ b/src/components/shared/ReactFlow/FlowCanvas/utils/duplicateNodes.ts @@ -65,18 +65,17 @@ export const duplicateNodes = ( const selected = config?.selected ?? true; const connection = config?.connection ?? "all"; + // Temporary specs used to ensure newly generated names & ids are truly unique + const tempGraphSpec = { ...graphSpec }; + const tempComponentSpec = { ...componentSpec }; + /* Create new Nodes and map old Task IDs to new Task IDs */ nodesToDuplicate.forEach((node) => { const oldNodeId = node.id; if (isTaskNode(node)) { - const oldTaskId = nodeManager.getRefId(oldNodeId); - if (!oldTaskId) { - console.warn("Could not find taskId for node:", node); - return; - } - - const newTaskId = getUniqueTaskId(graphSpec, oldTaskId); + const oldTaskId = node.data.taskId; + const newTaskId = getUniqueTaskId(tempGraphSpec, oldTaskId); const newNodeId = nodeManager.getNodeId(newTaskId, "task"); nodeIdMap[oldNodeId] = newNodeId; @@ -93,18 +92,24 @@ export const duplicateNodes = ( ...taskSpec, annotations: updatedAnnotations, }; + newTasks[newTaskId] = newTaskSpec; + tempGraphSpec.tasks = { + ...tempGraphSpec.tasks, + [newTaskId]: newTaskSpec, + }; } else if (isInputNode(node)) { - const inputSpec = componentSpec.inputs?.find( - (input) => input.name === node.data.label, - ); + const inputSpec = node.data.spec; - const newInputName = getUniqueInputName(componentSpec, inputSpec?.name); + const newInputName = getUniqueInputName( + tempComponentSpec, + inputSpec.name, + ); const newNodeId = nodeManager.getNodeId(newInputName, "input"); nodeIdMap[oldNodeId] = newNodeId; - const annotations = inputSpec?.annotations || {}; + const annotations = inputSpec.annotations || {}; const updatedAnnotations = setPositionInAnnotations(annotations, { x: node.position.x + OFFSET, @@ -118,20 +123,22 @@ export const duplicateNodes = ( }; newInputs[newInputName] = newInputSpec; + tempComponentSpec.inputs = [ + ...(tempComponentSpec.inputs || []), + newInputSpec, + ]; } else if (isOutputNode(node)) { - const outputSpec = componentSpec.outputs?.find( - (output) => output.name === node.data.label, - ); + const outputSpec = node.data.spec; const newOutputName = getUniqueOutputName( - componentSpec, - outputSpec?.name, + tempComponentSpec, + outputSpec.name, ); const newNodeId = nodeManager.getNodeId(newOutputName, "output"); nodeIdMap[oldNodeId] = newNodeId; - const annotations = outputSpec?.annotations || {}; + const annotations = outputSpec.annotations || {}; const updatedAnnotations = setPositionInAnnotations(annotations, { x: node.position.x + OFFSET, @@ -145,6 +152,10 @@ export const duplicateNodes = ( }; newOutputs[newOutputName] = newOutputSpec; + tempComponentSpec.outputs = [ + ...(tempComponentSpec.outputs || []), + newOutputSpec, + ]; } }); @@ -186,10 +197,11 @@ export const duplicateNodes = ( } }); - // Outputs are defined in the graph spec const updatedGraphOutputs = { ...graphSpec.outputValues }; - if (connection !== "none") { - /* Reconfigure Outputs */ + + // Output Nodes can only have one input, so the "external" connection mode does not apply to them. + if (connection !== "none" && connection !== "external") { + /* Reconfigure Output Nodes */ Object.entries(newOutputs).forEach((output) => { const [outputName] = output; const newNodeId = nodeManager.getNodeId(outputName, "output"); @@ -201,57 +213,62 @@ export const duplicateNodes = ( return; } - const oldOutputName = nodeManager.getRefId(oldNodeId); + const originalOutputNode = nodesToDuplicate.find( + (node) => node.id === oldNodeId && isOutputNode(node), + ); - if (!graphSpec.outputValues || !oldOutputName) { + if (!originalOutputNode || !isOutputNode(originalOutputNode)) { return; } - const outputValue = graphSpec.outputValues[oldOutputName]; + const oldOutputName = originalOutputNode.data.spec.name; + + const originalOutputValue = graphSpec.outputValues?.[oldOutputName]; - if (!outputValue) { + if (!originalOutputValue) { + // Todo: Handle cross-instance copy + paste for output nodes (we don't have the original graphSpec available so we can't look up the output value) + console.warn( + `No output value found for output ${oldOutputName} in graph spec.`, + ); return; } - const updatedOutputValue = { ...outputValue }; - - // If the outputvalue references a task that was also duplicated (internal connection), we need to update it to refer to the duplicated task id - let isInternal = false; - if ( - typeof updatedOutputValue === "object" && - updatedOutputValue !== null && - connection !== "external" - ) { - if ("taskOutput" in updatedOutputValue) { - const oldTaskId = updatedOutputValue.taskOutput.taskId; - const oldTaskNodeId = nodeManager.getNodeId(oldTaskId, "task"); - if (oldTaskNodeId in nodeIdMap) { - const newTaskId = nodeManager.getRefId(nodeIdMap[oldTaskNodeId]); - if (!newTaskId) { - return; - } - - updatedOutputValue.taskOutput = { - ...updatedOutputValue.taskOutput, - taskId: newTaskId, - }; - - isInternal = true; - } - } + if (!("taskOutput" in originalOutputValue)) { + console.warn( + `Output ${oldOutputName} is not connected to a task output`, + ); + return; + } + + const connectedTaskId = originalOutputValue.taskOutput.taskId; + const connectedOutputName = originalOutputValue.taskOutput.outputName; + + const originalNodeId = nodeManager.getNodeId(connectedTaskId, "task"); + + const newTaskNodeId = nodeIdMap[originalNodeId]; + if (!newTaskNodeId) { + console.warn(`No mapping found for task node ${originalNodeId}`); + return; } - if ( - (isInternal && connection === "internal") || - (!isInternal && connection === "external") || - connection === "all" - ) { - updatedGraphOutputs[outputName] = updatedOutputValue; + const newTaskId = nodeManager.getRefId(newTaskNodeId); + if (!newTaskId) { + console.warn(`Could not get new task ID for node ${newTaskNodeId}`); + return; } + + const outputValue: TaskOutputArgument = { + taskOutput: { + taskId: newTaskId, + outputName: connectedOutputName, + }, + }; + + updatedGraphOutputs[outputName] = outputValue; }); } - /* Update the Graph Spec & Inputs */ + /* Update the Graph Spec */ const updatedTasks = { ...graphSpec.tasks, ...newTasks }; const updatedGraphSpec = { ...graphSpec, @@ -507,43 +524,61 @@ function reconfigureConnections( if ("taskOutput" in argument) { const oldTaskId = argument.taskOutput.taskId; - oldNodeId = nodeManager.getNodeId(oldTaskId, "task"); - if (!isGraphImplementation(componentSpec.implementation)) { - throw new Error("ComponentSpec does not contain a graph implementation."); + const taskNode = nodes.find( + (node) => isTaskNode(node) && node.data.taskId === oldTaskId, + ); + + if (taskNode) { + oldNodeId = taskNode.id; + isExternal = false; + } else { + if (!isGraphImplementation(componentSpec.implementation)) { + throw new Error( + "ComponentSpec does not contain a graph implementation.", + ); + } + + const graphSpec = componentSpec.implementation.graph; + isExternal = oldTaskId in graphSpec.tasks; } - const graphSpec = componentSpec.implementation.graph; - isExternal = oldTaskId in graphSpec.tasks; + if (!oldNodeId) { + return reconfigureExternalConnection(taskSpec, argKey, mode); + } const newNodeId = nodeIdMap[oldNodeId]; - if (!newNodeId) { return reconfigureExternalConnection(taskSpec, argKey, mode); } const newTaskId = nodeManager.getRefId(newNodeId); - newArgId = newTaskId; } else if ("graphInput" in argument) { const oldInputName = argument.graphInput.inputName; - oldNodeId = nodeManager.getNodeId(oldInputName, "input"); - if (!("inputs" in componentSpec)) { - throw new Error("ComponentSpec does not contain inputs."); + const inputNode = nodes.find( + (node) => isInputNode(node) && node.data.spec.name === oldInputName, + ); + + if (inputNode) { + oldNodeId = inputNode.id; + isExternal = false; + } else { + const inputs = componentSpec.inputs || []; + isExternal = inputs.some((input) => input.name === oldInputName); } - const inputs = componentSpec.inputs || []; - isExternal = inputs.some((input) => input.name === oldInputName); + if (!oldNodeId) { + return reconfigureExternalConnection(taskSpec, argKey, mode); + } const newNodeId = nodeIdMap[oldNodeId]; - if (!newNodeId) { return reconfigureExternalConnection(taskSpec, argKey, mode); } const newInputName = nodeManager.getRefId(newNodeId); - newArgId = newInputName; } diff --git a/src/utils/unique.ts b/src/utils/unique.ts index d353ab240..639e39b04 100644 --- a/src/utils/unique.ts +++ b/src/utils/unique.ts @@ -4,11 +4,12 @@ const makeNameUniqueByAddingIndex = ( name: string, existingNames: Set, ): string => { - let finalName = name; + const baseName = name.replace(/ \(\d+\)$/, ""); + let finalName = baseName; let index = 1; while (existingNames.has(finalName)) { index++; - finalName = name + " " + index.toString(); + finalName = baseName + " (" + index.toString() + ")"; } return finalName; };