From c7d24bccef5db005f33a4908fc2ec227cd14029d Mon Sep 17 00:00:00 2001 From: Morgan Wowk Date: Thu, 4 Jun 2026 15:30:53 -0700 Subject: [PATCH] fix: wire Edit Component to update the task in the v2 editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the legacy-editor fix. The v2 editor's "Edit Component" menu (`ComponentRefBar`, reached from `TaskDetails`) opened `ComponentEditorDialog` without an `onComponentSaved` handler, so editing a selected task's component only imported a library copy and the task itself was never updated — the same bug as the legacy path, on the v2 model/store. This passes a handler through `TaskDetails` → `ComponentRefBar` → the dialog that applies the edit to the selected task via the existing v2 `replaceTask` store action. `replaceTask` is already the correct in-place primitive here: it keys the task by its stable `$id` (no rename), reconciles arguments and bindings against the edited spec (dropping bindings for removed inputs/outputs, adding defaults for new inputs), and calls `task.setComponentRef`. So unlike the legacy path — which needed a new `replaceTaskComponentRef` helper to avoid `replaceTaskNode`'s rename — v2 just needed the wiring. Behavior is unchanged when no handler is supplied (the dialog still falls back to the library-import flow), so the sidebar "import new component" usage is unaffected. Tests: replaceTask covers in-place swap (id/name preserved), dropping arguments + reporting lostInputs for removed inputs, and adding defaults for new inputs. Test setup: the existing `computeDiff` (task.utils.ts) uses `Set.prototype` `.difference`/`.intersection` (ES2024). These ship in every browser we target, but CI runs the suite on Node 20, which predates them — so a feature-detected polyfill is added to `vitest-setup.js`. This is the first test to exercise that code path under Node, which is why it surfaced here. --- .../context/TaskDetails/TaskDetails.tsx | 21 +++- .../components/ComponentRefBar.tsx | 6 + .../task.actions.replaceTask.test.ts | 118 ++++++++++++++++++ vitest-setup.js | 24 ++++ 4 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 src/routes/v2/pages/Editor/store/actions/__tests__/task.actions.replaceTask.test.ts diff --git a/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx b/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx index 8eee7a41a..9dcdd8704 100644 --- a/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx +++ b/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx @@ -17,6 +17,7 @@ import { useEditorSession } from "@/routes/v2/pages/Editor/store/EditorSessionCo import { useSpec } from "@/routes/v2/shared/providers/SpecContext"; import { useSharedStores } from "@/routes/v2/shared/store/SharedStoreContext"; import { SYSTEM_ANNOTATIONS, ZINDEX_ANNOTATION } from "@/utils/annotations"; +import type { HydratedComponentReference } from "@/utils/componentSpec"; import { tracking } from "@/utils/tracking"; import { getTaskYamlText } from "./components/actions/getTaskYamlText"; @@ -38,7 +39,7 @@ export const TaskDetails = observer(function TaskDetails({ const { track } = useAnalytics(); const { editor } = useSharedStores(); const { undo } = useEditorSession(); - const { renameTask } = useTaskActions(); + const { renameTask, replaceTask } = useTaskActions(); const notify = useToastNotification(); const spec = useSpec(); const task = useTask(entityId); @@ -74,6 +75,23 @@ export const TaskDetails = observer(function TaskDetails({ const isSubgraphTask = task.subgraphSpec !== undefined; + const handleComponentSaved = ( + hydratedComponent: HydratedComponentReference, + ) => { + const result = replaceTask(spec, task.$id, hydratedComponent); + const lostInputs = result.inputDiff?.lostEntities ?? []; + + if (lostInputs.length > 0) { + const inputNames = lostInputs.map((input) => input.name).join(", "); + notify( + `Component updated. Removed ${lostInputs.length} input(s) no longer defined: ${inputNames}.`, + "warning", + ); + } else { + notify("Component updated", "success"); + } + }; + const handleZIndexChange = (newZIndex: number) => { undo.withGroup("Update task z-index", () => { task.annotations.set(ZINDEX_ANNOTATION, newZIndex); @@ -163,6 +181,7 @@ export const TaskDetails = observer(function TaskDetails({ yamlText={yamlText} taskName={task.name} pythonCode={pythonCode} + onComponentSaved={handleComponentSaved} /> diff --git a/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/components/ComponentRefBar.tsx b/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/components/ComponentRefBar.tsx index 2e807adf0..3dffbab27 100644 --- a/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/components/ComponentRefBar.tsx +++ b/src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/components/ComponentRefBar.tsx @@ -22,6 +22,7 @@ import { Text } from "@/components/ui/typography"; import useToastNotification from "@/hooks/useToastNotification"; import type { ComponentReference as ModelComponentReference } from "@/models/componentSpec/entities/types"; import { useAnalytics } from "@/providers/AnalyticsProvider"; +import type { HydratedComponentReference } from "@/utils/componentSpec"; import { getComponentName } from "@/utils/getComponentName"; import { isSubgraph } from "@/utils/subgraphUtils"; import { tracking } from "@/utils/tracking"; @@ -35,6 +36,9 @@ interface ComponentRefBarProps { yamlText: string; taskName: string; pythonCode: string | undefined; + onComponentSaved?: ( + hydratedComponent: HydratedComponentReference, + ) => void | Promise; } export function ComponentRefBar({ @@ -42,6 +46,7 @@ export function ComponentRefBar({ yamlText, taskName, pythonCode, + onComponentSaved, }: ComponentRefBarProps) { const { track } = useAnalytics(); const notify = useToastNotification(); @@ -195,6 +200,7 @@ export function ComponentRefBar({ setIsEditDialogOpen(false)} + onComponentSaved={onComponentSaved} /> )} diff --git a/src/routes/v2/pages/Editor/store/actions/__tests__/task.actions.replaceTask.test.ts b/src/routes/v2/pages/Editor/store/actions/__tests__/task.actions.replaceTask.test.ts new file mode 100644 index 000000000..d2920a3d9 --- /dev/null +++ b/src/routes/v2/pages/Editor/store/actions/__tests__/task.actions.replaceTask.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it, vi } from "vitest"; + +import { ComponentSpec, Task } from "@/models/componentSpec"; +import { replaceTask } from "@/routes/v2/pages/Editor/store/actions/task.actions"; +import type { ComponentReference, InputSpec } from "@/utils/componentSpec"; + +// task.actions imports the editor node registry barrel (used by other actions, +// not by replaceTask). Loading the real barrel pulls in the node manifests and +// trips a module cycle in the test environment, so stub it. +vi.mock("@/routes/v2/pages/Editor/nodes", () => ({ + editorRegistry: { getByNodeId: () => undefined }, +})); + +const noopUndo = { + withGroup: (_label: string, fn: () => T): T => fn(), +}; + +const datasetRef = ( + digest: string, + inputs: InputSpec[], +): ComponentReference => ({ + name: "Chicago Taxi Trips dataset", + digest, + spec: { + name: "Chicago Taxi Trips dataset", + inputs, + outputs: [{ name: "Table" }], + implementation: { + container: { image: "alpine/curl", command: ["sh", "-ec", "true"] }, + }, + }, +}); + +const LIMIT_AND_SELECT: InputSpec[] = [ + { name: "Limit", type: "Integer", default: "1000" }, + { name: "Select", type: "String" }, +]; + +const makeSpecWithDataset = () => { + const spec = new ComponentSpec({ $id: "spec_1", name: "Pipeline" }); + spec.addTask( + new Task({ + $id: "dataset", + name: "dataset", + componentRef: datasetRef("old-digest", LIMIT_AND_SELECT), + arguments: [ + { name: "Limit", value: "86" }, + { name: "Select", value: "tips,trip_seconds" }, + ], + }), + ); + return spec; +}; + +describe("replaceTask (edit component definition, v2)", () => { + it("swaps the componentRef in place, keeping task id/name and compatible arguments", () => { + const spec = makeSpecWithDataset(); + + const result = replaceTask( + noopUndo, + spec, + "dataset", + datasetRef("new-digest", LIMIT_AND_SELECT), + ); + + const task = spec.tasks.find((t) => t.$id === "dataset"); + + expect(spec.tasks).toHaveLength(1); + expect(task?.name).toBe("dataset"); + expect(task?.componentRef.digest).toBe("new-digest"); + expect(task?.arguments.map((a) => a.name).sort()).toEqual([ + "Limit", + "Select", + ]); + expect(result.inputDiff?.lostEntities ?? []).toEqual([]); + }); + + it("drops arguments and reports lostInputs for inputs the edit removed", () => { + const spec = makeSpecWithDataset(); + + const result = replaceTask( + noopUndo, + spec, + "dataset", + datasetRef("new-digest", [{ name: "Limit", type: "Integer" }]), + ); + + const task = spec.tasks.find((t) => t.$id === "dataset"); + + expect((result.inputDiff?.lostEntities ?? []).map((i) => i.name)).toEqual([ + "Select", + ]); + expect(task?.arguments.map((a) => a.name)).toEqual(["Limit"]); + }); + + it("adds arguments (with defaults) for inputs the edit introduced", () => { + const spec = makeSpecWithDataset(); + + replaceTask(noopUndo, spec, "dataset", { + name: "Chicago Taxi Trips dataset", + digest: "new-digest", + spec: { + name: "Chicago Taxi Trips dataset", + inputs: [ + ...LIMIT_AND_SELECT, + { name: "Format", type: "String", default: "csv" }, + ], + outputs: [{ name: "Table" }], + implementation: { container: { image: "alpine/curl" } }, + }, + }); + + const task = spec.tasks.find((t) => t.$id === "dataset"); + const formatArg = task?.arguments.find((a) => a.name === "Format"); + + expect(formatArg?.value).toBe("csv"); + }); +}); diff --git a/vitest-setup.js b/vitest-setup.js index f149f27ae..4c03d4484 100644 --- a/vitest-setup.js +++ b/vitest-setup.js @@ -1 +1,25 @@ import "@testing-library/jest-dom/vitest"; + +// Polyfill ES2024 Set methods used by app code (e.g. computeDiff in +// task.utils.ts). They ship in every browser we target, but CI runs the test +// suite on Node 20, which predates them. Without this, any test that exercises +// that code throws "Set.prototype.difference is not a function". +if (typeof Set.prototype.difference !== "function") { + Set.prototype.difference = function difference(other) { + const result = new Set(); + for (const value of this) { + if (!other.has(value)) result.add(value); + } + return result; + }; +} + +if (typeof Set.prototype.intersection !== "function") { + Set.prototype.intersection = function intersection(other) { + const result = new Set(); + for (const value of this) { + if (other.has(value)) result.add(value); + } + return result; + }; +}