From b3561cb51e1f931d8e3aaac2d93105cc1741cf96 Mon Sep 17 00:00:00 2001 From: Morgan Wowk Date: Thu, 4 Jun 2026 15:21:16 -0700 Subject: [PATCH] fix: Edit Component Definition now updates the selected task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The bug "Edit Component Definition" (the pencil action on a selected task) saved the edited component to the user's component **library** but never updated the **task** the user had selected. The save handler in `ComponentEditorDialog` only called `saveComponent` (localforage, keyed by the new digest) + `addToComponentLibrary`, then closed. The task's embedded `componentRef` was left untouched. Because the editor re-reads from that same embedded `componentRef.text` when it reopens, the edit appeared to vanish — even though the toast said "Component … imported successfully". ### Reproduction (negative scenario) 1. Add the "Chicago Taxi Trips dataset" component to a pipeline. Its command pipes the API response through `sed -E 's/"([^",\]*)"/\1/g'` to strip the quotes Socrata wraps every field in. 2. Select the task and click "Edit Component Definition". 3. Fix a bug in the command — e.g. move the `\1` backreference back inside the single quotes (a stray `'\1'` makes sed emit the literal `1`, which turns every value, header included, into `1`). 4. Click Save → toast: "Component Chicago Taxi Trips dataset imported successfully". 5. Reopen "Edit Component Definition" → the old, broken command is back. Running the pipeline still produces the broken output. ## The fix When the editor is opened for a selected task, the dialog now applies the edit to that task instead of only importing a library copy: - `ComponentEditorDialog` gains an optional `onComponentSaved(hydrated)` callback. When provided, it is invoked with the hydrated edited component (and the library-import + "imported successfully" path is skipped). When omitted (e.g. the sidebar "import new component" flow), behavior is unchanged. - `EditComponentButton` receives the `taskId` and supplies that callback, which swaps the task's `componentRef` via a new `replaceTaskComponentRef` helper and shows "Component updated". - `replaceTaskComponentRef` updates the task **in place** — same task id, position, annotations, execution options — and preserves the task's argument bindings. It only drops bindings that can no longer be valid: arguments for inputs the edited component removed (reported back as `lostInputs`), and downstream bindings to outputs it no longer produces. ## Why this is safe - **Scoped behavior change.** The only behavioral change is on the task-edit path (an `onComponentSaved` callback is now passed). The create/import flows (`ImportComponent`) pass no callback and are byte-for- byte unchanged. - **Matches user intent.** If you edit a component's definition *after selecting a task*, you mean to update that task. We preserve everything you'd expect to keep — wired inputs, set values, position, display name — rather than renaming or re-keying the task. - **Reuses proven semantics.** The preserve-wiring / lost-input handling mirrors the existing "Upgrade" flow (`replaceTaskNode`). Unlike upgrade, this keeps the same task id, because an in-place edit shouldn't rename `dataset` to `dataset 2`. - **Pure + immutable.** `replaceTaskComponentRef` deep-clones and never mutates the input graph; the swap goes through the standard `updateGraphSpec` path, so undo/redo and validation work as usual. - **No data loss surprises.** If an edit removes an input or output that was wired up, the affected binding is dropped (it would be invalid otherwise) and the user is told via a warning toast listing the removed inputs. ## Tests - `replaceTaskComponentRef.test.ts`: in-place swap without renaming, preservation of arguments/annotations/downstream wiring, immutability of the input graph, dropping of arguments for removed inputs (with `lostInputs`), dropping of downstream bindings to removed outputs, and the missing-task no-op. - `ComponentEditorDialog.test.tsx`: when `onComponentSaved` is provided, it receives the hydrated component and the library-import path is not taken. ## Follow-up The v2 editor's "Edit Component" menu (`ComponentRefBar`) has the same latent gap but uses the v2 model/store; it's intentionally out of scope for this PR and can be addressed as a stacked follow-up. --- .../ComponentEditorDialog.test.tsx | 49 ++++++ .../ComponentEditor/ComponentEditorDialog.tsx | 33 +++- .../utils/replaceTaskComponentRef.test.ts | 163 ++++++++++++++++++ .../utils/replaceTaskComponentRef.ts | 100 +++++++++++ src/components/shared/TaskDetails/Actions.tsx | 2 +- .../Actions/EditComponentButton.tsx | 38 ++++ 6 files changed, 378 insertions(+), 7 deletions(-) create mode 100644 src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.test.ts create mode 100644 src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.ts diff --git a/src/components/shared/ComponentEditor/ComponentEditorDialog.test.tsx b/src/components/shared/ComponentEditor/ComponentEditorDialog.test.tsx index 397892ffd..d873891f3 100644 --- a/src/components/shared/ComponentEditor/ComponentEditorDialog.test.tsx +++ b/src/components/shared/ComponentEditor/ComponentEditorDialog.test.tsx @@ -337,5 +337,54 @@ describe("", () => { expect(onCloseMock).toHaveBeenCalledTimes(1); }); }); + + test("calls onComponentSaved instead of importing to the library when provided", async () => { + const onCloseMock = vi.fn(); + const onComponentSavedMock = vi.fn(); + const mockHydratedComponent = { + spec: { + implementation: { container: { image: "test" } }, + metadata: { annotations: {} }, + }, + name: "test-component", + digest: "abc123", + text: "name: test-component", + }; + + vi.mocked(hydrateComponentReference).mockResolvedValue( + mockHydratedComponent, + ); + + renderWithProviders( + , + ); + + await waitFor(() => { + expect( + screen.getByRole("button", { name: /Save/i }), + ).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByRole("button", { name: /Save/i })); + + await waitFor(() => { + // The edited component is handed to the caller to apply. + expect(onComponentSavedMock).toHaveBeenCalledWith( + mockHydratedComponent, + ); + expect(onCloseMock).toHaveBeenCalledTimes(1); + }); + + // It must NOT fall back to the library-import behavior. + expect(mockAddToComponentLibrary).not.toHaveBeenCalled(); + expect(mockToast).not.toHaveBeenCalledWith( + expect.stringContaining("imported successfully"), + "success", + ); + }); }); }); diff --git a/src/components/shared/ComponentEditor/ComponentEditorDialog.tsx b/src/components/shared/ComponentEditor/ComponentEditorDialog.tsx index 3579bf65a..933087ece 100644 --- a/src/components/shared/ComponentEditor/ComponentEditorDialog.tsx +++ b/src/components/shared/ComponentEditor/ComponentEditorDialog.tsx @@ -11,7 +11,10 @@ import useToastNotification from "@/hooks/useToastNotification"; import { useAnalytics } from "@/providers/AnalyticsProvider"; import { useComponentLibrary } from "@/providers/ComponentLibraryProvider"; import { hydrateComponentReference } from "@/services/componentService"; -import { isContainerImplementation } from "@/utils/componentSpec"; +import { + type HydratedComponentReference, + isContainerImplementation, +} from "@/utils/componentSpec"; import { saveComponent } from "@/utils/localforage"; import { FullscreenElement } from "../FullscreenElement"; @@ -72,10 +75,21 @@ export const ComponentEditorDialog = withSuspenseWrapper( text, templateName = "empty", onClose, + onComponentSaved, }: { text?: string; templateName?: SupportedTemplate; onClose: () => void; + /** + * When provided, the editor is being used to edit an existing target (e.g. + * a selected task's component) rather than to import a brand new component + * into the library. The callback receives the hydrated, edited component + * and is responsible for applying it (and any user feedback). When omitted, + * the editor falls back to importing the component into the library. + */ + onComponentSaved?: ( + hydratedComponent: HydratedComponentReference, + ) => void | Promise; }) => { const notify = useToastNotification(); const { track } = useAnalytics(); @@ -190,16 +204,23 @@ export const ComponentEditorDialog = withSuspenseWrapper( onClose(); - await addToComponentLibrary(hydratedComponent, "editor_save"); + if (onComponentSaved) { + // Editing an existing target (e.g. a selected task): apply the edit to + // that target instead of importing a new library component. The caller + // owns the success/feedback messaging. + await onComponentSaved(hydratedComponent); + } else { + await addToComponentLibrary(hydratedComponent, "editor_save"); + notify( + `Component ${hydratedComponent.name} imported successfully`, + "success", + ); + } track("component_editor.save.completed", { mode, selected_template: templateName, }); - notify( - `Component ${hydratedComponent.name} imported successfully`, - "success", - ); }; const handleClose = () => { diff --git a/src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.test.ts b/src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.test.ts new file mode 100644 index 000000000..58dc1555a --- /dev/null +++ b/src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, it } from "vitest"; + +import type { ComponentReference, GraphSpec } from "@/utils/componentSpec"; + +import { replaceTaskComponentRef } from "./replaceTaskComponentRef"; + +const taxiRef = (command: string): ComponentReference => ({ + name: "Chicago Taxi Trips dataset", + digest: "old-digest", + spec: { + name: "Chicago Taxi Trips dataset", + inputs: [ + { name: "Limit", type: "Integer", default: "1000" }, + { name: "Select", type: "String" }, + ], + outputs: [{ name: "Table" }], + implementation: { container: { image: "alpine/curl", command: [command] } }, + }, +}); + +const baseGraphSpec = (): GraphSpec => ({ + tasks: { + dataset: { + componentRef: taxiRef("sed -E 's/x/'\\1'/g'"), + annotations: { "editor.position": '{"x":10,"y":20}' }, + arguments: { + Limit: { graphInput: { inputName: "Input" } }, + Select: "tips,trip_seconds", + }, + }, + train: { + componentRef: { name: "Train" }, + arguments: { + training_data: { + taskOutput: { taskId: "dataset", outputName: "Table" }, + }, + }, + }, + }, +}); + +describe("replaceTaskComponentRef", () => { + it("swaps the componentRef in place without renaming the task", () => { + const graphSpec = baseGraphSpec(); + const fixedRef: ComponentReference = { + ...taxiRef("sed -E 's/x/\\1/g'"), + digest: "new-digest", + }; + + const { updatedGraphSpec, lostInputs } = replaceTaskComponentRef( + "dataset", + fixedRef, + graphSpec, + ); + + // Same key, no "dataset 2". + expect(Object.keys(updatedGraphSpec.tasks)).toEqual(["dataset", "train"]); + expect(updatedGraphSpec.tasks.dataset.componentRef.digest).toBe( + "new-digest", + ); + expect(lostInputs).toEqual([]); + }); + + it("preserves the task's arguments, annotations and downstream wiring", () => { + const graphSpec = baseGraphSpec(); + const fixedRef = { ...taxiRef("fixed"), digest: "new-digest" }; + + const { updatedGraphSpec } = replaceTaskComponentRef( + "dataset", + fixedRef, + graphSpec, + ); + + expect(updatedGraphSpec.tasks.dataset.arguments).toEqual({ + Limit: { graphInput: { inputName: "Input" } }, + Select: "tips,trip_seconds", + }); + expect(updatedGraphSpec.tasks.dataset.annotations).toEqual({ + "editor.position": '{"x":10,"y":20}', + }); + // Downstream consumer still wired to the (unchanged) output/taskId. + expect(updatedGraphSpec.tasks.train.arguments).toEqual({ + training_data: { + taskOutput: { taskId: "dataset", outputName: "Table" }, + }, + }); + }); + + it("does not mutate the original graph spec", () => { + const graphSpec = baseGraphSpec(); + const fixedRef = { ...taxiRef("fixed"), digest: "new-digest" }; + + replaceTaskComponentRef("dataset", fixedRef, graphSpec); + + expect(graphSpec.tasks.dataset.componentRef.digest).toBe("old-digest"); + }); + + it("drops arguments for inputs the edited component no longer defines", () => { + const graphSpec = baseGraphSpec(); + const refWithoutSelect: ComponentReference = { + name: "Chicago Taxi Trips dataset", + digest: "new-digest", + spec: { + name: "Chicago Taxi Trips dataset", + inputs: [{ name: "Limit", type: "Integer" }], + outputs: [{ name: "Table" }], + implementation: { container: { image: "alpine/curl" } }, + }, + }; + + const { updatedGraphSpec, lostInputs } = replaceTaskComponentRef( + "dataset", + refWithoutSelect, + graphSpec, + ); + + expect(lostInputs.map((input) => input.name)).toEqual(["Select"]); + expect(updatedGraphSpec.tasks.dataset.arguments).toEqual({ + Limit: { graphInput: { inputName: "Input" } }, + }); + }); + + it("drops downstream bindings to outputs the edited component no longer produces", () => { + const graphSpec = baseGraphSpec(); + const refWithoutTable: ComponentReference = { + name: "Chicago Taxi Trips dataset", + digest: "new-digest", + spec: { + name: "Chicago Taxi Trips dataset", + inputs: [ + { name: "Limit", type: "Integer" }, + { name: "Select", type: "String" }, + ], + outputs: [{ name: "RenamedTable" }], + implementation: { container: { image: "alpine/curl" } }, + }, + }; + + const { updatedGraphSpec } = replaceTaskComponentRef( + "dataset", + refWithoutTable, + graphSpec, + ); + + expect(updatedGraphSpec.tasks.train.arguments).toEqual({}); + }); + + it("returns the graph unchanged when the task does not exist", () => { + const graphSpec = baseGraphSpec(); + const fixedRef = { ...taxiRef("fixed"), digest: "new-digest" }; + + const { updatedGraphSpec, lostInputs } = replaceTaskComponentRef( + "missing", + fixedRef, + graphSpec, + ); + + expect(lostInputs).toEqual([]); + expect(updatedGraphSpec.tasks.dataset.componentRef.digest).toBe( + "old-digest", + ); + }); +}); diff --git a/src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.ts b/src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.ts new file mode 100644 index 000000000..5a448cac7 --- /dev/null +++ b/src/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef.ts @@ -0,0 +1,100 @@ +import type { + ArgumentType, + ComponentReference, + GraphSpec, + InputSpec, +} from "@/utils/componentSpec"; +import { deepClone } from "@/utils/deepClone"; + +/** + * Replaces a task's componentRef in place (keeping the same taskId, position, + * annotations and execution options) while preserving as much of the task's + * existing wiring as possible. + * + * This is the right primitive for "Edit Component Definition": the user has + * selected a task and edited its component, so their intent is to update that + * specific task and keep everything they already configured. + * + * Unlike `replaceTaskNode` (used by "Upgrade"), this does NOT rename the task: + * renaming would re-key the task to a new unique id derived from the component + * name and is surprising for an in-place edit. + * + * Wiring is preserved with two safe exceptions, mirroring the upgrade flow: + * - argument bindings on this task for inputs that no longer exist in the + * edited component are dropped (and reported via `lostInputs`); + * - argument bindings on downstream tasks that consume an output this + * component no longer produces are dropped, to avoid dangling references. + * + * In the common case (editing the command/image, no input/output changes) + * nothing is dropped and every argument and default is preserved. + */ +export const replaceTaskComponentRef = ( + taskId: string, + newComponentRef: ComponentReference, + graphSpec: GraphSpec, +) => { + const updatedGraphSpec = deepClone(graphSpec); + const task = updatedGraphSpec.tasks[taskId]; + + if (!task) { + return { updatedGraphSpec, lostInputs: [] as InputSpec[] }; + } + + const oldInputs = task.componentRef.spec?.inputs ?? []; + const newInputNames = new Set( + (newComponentRef.spec?.inputs ?? []).map((input) => input.name), + ); + const newOutputNames = new Set( + (newComponentRef.spec?.outputs ?? []).map((output) => output.name), + ); + + // Inputs present on the old component but missing from the edited one. + const lostInputs = oldInputs.filter( + (input) => !newInputNames.has(input.name), + ); + + task.componentRef = newComponentRef; + + // Drop this task's argument bindings for inputs that were removed. + if (task.arguments) { + task.arguments = Object.entries(task.arguments).reduce( + (acc, [inputName, argument]) => { + if (newInputNames.has(inputName)) { + acc[inputName] = argument; + } + return acc; + }, + {} as Record, + ); + } + + // Drop downstream argument bindings that consume an output this component no + // longer produces. Outputs that still exist keep working because the taskId + // is unchanged. + Object.values(updatedGraphSpec.tasks).forEach((downstreamTask) => { + if (!downstreamTask.arguments) { + return; + } + + downstreamTask.arguments = Object.entries(downstreamTask.arguments).reduce( + (acc, [inputName, argument]) => { + if ( + typeof argument === "object" && + argument !== null && + "taskOutput" in argument && + argument.taskOutput && + argument.taskOutput.taskId === taskId && + !newOutputNames.has(argument.taskOutput.outputName) + ) { + return acc; + } + + acc[inputName] = argument; + return acc; + }, + {} as Record, + ); + }); + + return { updatedGraphSpec, lostInputs } as const; +}; diff --git a/src/components/shared/TaskDetails/Actions.tsx b/src/components/shared/TaskDetails/Actions.tsx index 88de44bc3..a64abc96b 100644 --- a/src/components/shared/TaskDetails/Actions.tsx +++ b/src/components/shared/TaskDetails/Actions.tsx @@ -56,7 +56,7 @@ const TaskActions = ({ /> ); const editComponent = !readOnly && ( - + ); // Canvas Actions diff --git a/src/components/shared/TaskDetails/Actions/EditComponentButton.tsx b/src/components/shared/TaskDetails/Actions/EditComponentButton.tsx index 5b6d241b1..c3d1d4a46 100644 --- a/src/components/shared/TaskDetails/Actions/EditComponentButton.tsx +++ b/src/components/shared/TaskDetails/Actions/EditComponentButton.tsx @@ -1,5 +1,8 @@ import { useState } from "react"; +import { replaceTaskComponentRef } from "@/components/shared/ReactFlow/FlowCanvas/utils/replaceTaskComponentRef"; +import useToastNotification from "@/hooks/useToastNotification"; +import { useComponentSpec } from "@/providers/ComponentSpecProvider"; import type { HydratedComponentReference } from "@/utils/componentSpec"; import { tracking } from "@/utils/tracking"; @@ -8,12 +11,46 @@ import { ComponentEditorDialog } from "../../ComponentEditor/ComponentEditorDial interface EditComponentButtonProps { componentRef: HydratedComponentReference; + taskId?: string; } export const EditComponentButton = ({ componentRef, + taskId, }: EditComponentButtonProps) => { const [isEditDialogOpen, setIsEditDialogOpen] = useState(false); + const notify = useToastNotification(); + const { currentGraphSpec, updateGraphSpec } = useComponentSpec(); + + const handleComponentSaved = ( + hydratedComponent: HydratedComponentReference, + ) => { + if (!taskId || !currentGraphSpec?.tasks[taskId]) { + notify( + "Could not update the component: the edited task was not found.", + "error", + ); + return; + } + + const { updatedGraphSpec, lostInputs } = replaceTaskComponentRef( + taskId, + hydratedComponent, + currentGraphSpec, + ); + + updateGraphSpec(updatedGraphSpec); + + 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"); + } + }; return ( <> @@ -27,6 +64,7 @@ export const EditComponentButton = ({ setIsEditDialogOpen(false)} + onComponentSaved={taskId ? handleComponentSaved : undefined} /> )}