Skip to content

Commit c7d24bc

Browse files
committed
fix: wire Edit Component to update the task in the v2 editor
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.
1 parent b3561cb commit c7d24bc

4 files changed

Lines changed: 168 additions & 1 deletion

File tree

src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/TaskDetails.tsx

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { useEditorSession } from "@/routes/v2/pages/Editor/store/EditorSessionCo
1717
import { useSpec } from "@/routes/v2/shared/providers/SpecContext";
1818
import { useSharedStores } from "@/routes/v2/shared/store/SharedStoreContext";
1919
import { SYSTEM_ANNOTATIONS, ZINDEX_ANNOTATION } from "@/utils/annotations";
20+
import type { HydratedComponentReference } from "@/utils/componentSpec";
2021
import { tracking } from "@/utils/tracking";
2122

2223
import { getTaskYamlText } from "./components/actions/getTaskYamlText";
@@ -38,7 +39,7 @@ export const TaskDetails = observer(function TaskDetails({
3839
const { track } = useAnalytics();
3940
const { editor } = useSharedStores();
4041
const { undo } = useEditorSession();
41-
const { renameTask } = useTaskActions();
42+
const { renameTask, replaceTask } = useTaskActions();
4243
const notify = useToastNotification();
4344
const spec = useSpec();
4445
const task = useTask(entityId);
@@ -74,6 +75,23 @@ export const TaskDetails = observer(function TaskDetails({
7475

7576
const isSubgraphTask = task.subgraphSpec !== undefined;
7677

78+
const handleComponentSaved = (
79+
hydratedComponent: HydratedComponentReference,
80+
) => {
81+
const result = replaceTask(spec, task.$id, hydratedComponent);
82+
const lostInputs = result.inputDiff?.lostEntities ?? [];
83+
84+
if (lostInputs.length > 0) {
85+
const inputNames = lostInputs.map((input) => input.name).join(", ");
86+
notify(
87+
`Component updated. Removed ${lostInputs.length} input(s) no longer defined: ${inputNames}.`,
88+
"warning",
89+
);
90+
} else {
91+
notify("Component updated", "success");
92+
}
93+
};
94+
7795
const handleZIndexChange = (newZIndex: number) => {
7896
undo.withGroup("Update task z-index", () => {
7997
task.annotations.set(ZINDEX_ANNOTATION, newZIndex);
@@ -163,6 +181,7 @@ export const TaskDetails = observer(function TaskDetails({
163181
yamlText={yamlText}
164182
taskName={task.name}
165183
pythonCode={pythonCode}
184+
onComponentSaved={handleComponentSaved}
166185
/>
167186
</BlockStack>
168187

src/routes/v2/pages/Editor/nodes/TaskNode/context/TaskDetails/components/ComponentRefBar.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { Text } from "@/components/ui/typography";
2222
import useToastNotification from "@/hooks/useToastNotification";
2323
import type { ComponentReference as ModelComponentReference } from "@/models/componentSpec/entities/types";
2424
import { useAnalytics } from "@/providers/AnalyticsProvider";
25+
import type { HydratedComponentReference } from "@/utils/componentSpec";
2526
import { getComponentName } from "@/utils/getComponentName";
2627
import { isSubgraph } from "@/utils/subgraphUtils";
2728
import { tracking } from "@/utils/tracking";
@@ -35,13 +36,17 @@ interface ComponentRefBarProps {
3536
yamlText: string;
3637
taskName: string;
3738
pythonCode: string | undefined;
39+
onComponentSaved?: (
40+
hydratedComponent: HydratedComponentReference,
41+
) => void | Promise<void>;
3842
}
3943

4044
export function ComponentRefBar({
4145
componentRef,
4246
yamlText,
4347
taskName,
4448
pythonCode,
49+
onComponentSaved,
4550
}: ComponentRefBarProps) {
4651
const { track } = useAnalytics();
4752
const notify = useToastNotification();
@@ -195,6 +200,7 @@ export function ComponentRefBar({
195200
<ComponentEditorDialog
196201
text={yamlText}
197202
onClose={() => setIsEditDialogOpen(false)}
203+
onComponentSaved={onComponentSaved}
198204
/>
199205
)}
200206

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
3+
import { ComponentSpec, Task } from "@/models/componentSpec";
4+
import { replaceTask } from "@/routes/v2/pages/Editor/store/actions/task.actions";
5+
import type { ComponentReference, InputSpec } from "@/utils/componentSpec";
6+
7+
// task.actions imports the editor node registry barrel (used by other actions,
8+
// not by replaceTask). Loading the real barrel pulls in the node manifests and
9+
// trips a module cycle in the test environment, so stub it.
10+
vi.mock("@/routes/v2/pages/Editor/nodes", () => ({
11+
editorRegistry: { getByNodeId: () => undefined },
12+
}));
13+
14+
const noopUndo = {
15+
withGroup: <T>(_label: string, fn: () => T): T => fn(),
16+
};
17+
18+
const datasetRef = (
19+
digest: string,
20+
inputs: InputSpec[],
21+
): ComponentReference => ({
22+
name: "Chicago Taxi Trips dataset",
23+
digest,
24+
spec: {
25+
name: "Chicago Taxi Trips dataset",
26+
inputs,
27+
outputs: [{ name: "Table" }],
28+
implementation: {
29+
container: { image: "alpine/curl", command: ["sh", "-ec", "true"] },
30+
},
31+
},
32+
});
33+
34+
const LIMIT_AND_SELECT: InputSpec[] = [
35+
{ name: "Limit", type: "Integer", default: "1000" },
36+
{ name: "Select", type: "String" },
37+
];
38+
39+
const makeSpecWithDataset = () => {
40+
const spec = new ComponentSpec({ $id: "spec_1", name: "Pipeline" });
41+
spec.addTask(
42+
new Task({
43+
$id: "dataset",
44+
name: "dataset",
45+
componentRef: datasetRef("old-digest", LIMIT_AND_SELECT),
46+
arguments: [
47+
{ name: "Limit", value: "86" },
48+
{ name: "Select", value: "tips,trip_seconds" },
49+
],
50+
}),
51+
);
52+
return spec;
53+
};
54+
55+
describe("replaceTask (edit component definition, v2)", () => {
56+
it("swaps the componentRef in place, keeping task id/name and compatible arguments", () => {
57+
const spec = makeSpecWithDataset();
58+
59+
const result = replaceTask(
60+
noopUndo,
61+
spec,
62+
"dataset",
63+
datasetRef("new-digest", LIMIT_AND_SELECT),
64+
);
65+
66+
const task = spec.tasks.find((t) => t.$id === "dataset");
67+
68+
expect(spec.tasks).toHaveLength(1);
69+
expect(task?.name).toBe("dataset");
70+
expect(task?.componentRef.digest).toBe("new-digest");
71+
expect(task?.arguments.map((a) => a.name).sort()).toEqual([
72+
"Limit",
73+
"Select",
74+
]);
75+
expect(result.inputDiff?.lostEntities ?? []).toEqual([]);
76+
});
77+
78+
it("drops arguments and reports lostInputs for inputs the edit removed", () => {
79+
const spec = makeSpecWithDataset();
80+
81+
const result = replaceTask(
82+
noopUndo,
83+
spec,
84+
"dataset",
85+
datasetRef("new-digest", [{ name: "Limit", type: "Integer" }]),
86+
);
87+
88+
const task = spec.tasks.find((t) => t.$id === "dataset");
89+
90+
expect((result.inputDiff?.lostEntities ?? []).map((i) => i.name)).toEqual([
91+
"Select",
92+
]);
93+
expect(task?.arguments.map((a) => a.name)).toEqual(["Limit"]);
94+
});
95+
96+
it("adds arguments (with defaults) for inputs the edit introduced", () => {
97+
const spec = makeSpecWithDataset();
98+
99+
replaceTask(noopUndo, spec, "dataset", {
100+
name: "Chicago Taxi Trips dataset",
101+
digest: "new-digest",
102+
spec: {
103+
name: "Chicago Taxi Trips dataset",
104+
inputs: [
105+
...LIMIT_AND_SELECT,
106+
{ name: "Format", type: "String", default: "csv" },
107+
],
108+
outputs: [{ name: "Table" }],
109+
implementation: { container: { image: "alpine/curl" } },
110+
},
111+
});
112+
113+
const task = spec.tasks.find((t) => t.$id === "dataset");
114+
const formatArg = task?.arguments.find((a) => a.name === "Format");
115+
116+
expect(formatArg?.value).toBe("csv");
117+
});
118+
});

vitest-setup.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,25 @@
11
import "@testing-library/jest-dom/vitest";
2+
3+
// Polyfill ES2024 Set methods used by app code (e.g. computeDiff in
4+
// task.utils.ts). They ship in every browser we target, but CI runs the test
5+
// suite on Node 20, which predates them. Without this, any test that exercises
6+
// that code throws "Set.prototype.difference is not a function".
7+
if (typeof Set.prototype.difference !== "function") {
8+
Set.prototype.difference = function difference(other) {
9+
const result = new Set();
10+
for (const value of this) {
11+
if (!other.has(value)) result.add(value);
12+
}
13+
return result;
14+
};
15+
}
16+
17+
if (typeof Set.prototype.intersection !== "function") {
18+
Set.prototype.intersection = function intersection(other) {
19+
const result = new Set();
20+
for (const value of this) {
21+
if (other.has(value)) result.add(value);
22+
}
23+
return result;
24+
};
25+
}

0 commit comments

Comments
 (0)