Skip to content

Commit 4082941

Browse files
committed
feat: choose-action modal on Save in the legacy component editor
When a user edits a selected task's component definition and clicks Save, the editor now opens a small modal to choose what to do with the edit (Update this task / Import to library), showing what inputs/outputs changed. This replaces the silent in-place update with an explicit, informed choice and makes "import to library instead" reachable from the same flow. Shared groundwork (folded into this first branch, reused by later branches): - `src/utils/componentSpecDiff.ts`: portable input/output diff (`computeEntityDiff`, `diffComponentIO`, `hasIODiff`) lifted out of v2's `task.utils.ts`. v2 re-exports `EntityDiff` and delegates `computeDiffComponentSpecs` to it, so v2 imports are unchanged — and this removes the `Set.prototype.difference` (Node 20) dependency from that path. - `src/components/shared/ComponentDiff/DiffSection.tsx`: the lost/new/changed list lifted out of v2's `UpgradeCandidateDetail` so both editors share it (v1 must not import from v2). v2 call sites updated to the shared path. Save contract: - `ComponentEditorDialog` gains an optional `resolveSaveAction(hydrated) => Promise<SaveAction>`; `onComponentSaved` now receives the chosen `SaveAction`. `"cancel"` keeps the editor open, `"import"` runs the existing library path, `"update"`/`"place"` are forwarded to the caller. When no resolver is supplied, behavior is unchanged (in-place update or library import) — so the v2 caller and the sidebar import flow are unaffected. v1 wiring: - `ChooseSaveActionDialog` (built on the `Dialog` primitive) + a promise-based `useChooseSaveAction` hook (modeled on `useConfirmationDialog`). - `EditComponentButton` computes the diff via `diffComponentIO`, opens the modal, and applies `"update"` via the existing `replaceTaskComponentRef`. ("Place as a new task" is gated behind `allowPlace`, enabled in a later branch.) Tests: `componentSpecDiff` (lost/new/changed, empty, order; IO diff; hasIODiff) and `ComponentEditorDialog` (resolveSaveAction import/cancel paths; update forwards the action).
1 parent c7d24bc commit 4082941

12 files changed

Lines changed: 611 additions & 139 deletions

File tree

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { Icon } from "@/components/ui/icon";
2+
import { BlockStack, InlineStack } from "@/components/ui/layout";
3+
import { Text } from "@/components/ui/typography";
4+
import type { EntityDiff } from "@/utils/componentSpecDiff";
5+
6+
/**
7+
* Renders the lost / added / changed entities of an `EntityDiff` as a compact,
8+
* color-coded list. Shared by the legacy and v2 editors (upgrade flow, replace
9+
* confirmation, and the edit-component save modal). Only reads `name`, so it
10+
* accepts a diff of any name-keyed entity.
11+
*/
12+
export function DiffSection({
13+
label,
14+
diff,
15+
}: {
16+
label: string;
17+
diff: EntityDiff<{ name: string }>;
18+
}) {
19+
const hasChanges =
20+
diff.lostEntities.length > 0 ||
21+
diff.newEntities.length > 0 ||
22+
diff.changedEntities.length > 0;
23+
24+
if (!hasChanges) return null;
25+
26+
return (
27+
<BlockStack gap="1">
28+
<Text size="xs" weight="semibold" tone="subdued">
29+
{label} Changes
30+
</Text>
31+
<BlockStack className="gap-0.5">
32+
{diff.lostEntities.map((e) => (
33+
<DiffLine
34+
key={`lost-${e.name}`}
35+
icon="Minus"
36+
color="text-red-500"
37+
label={`Removed: ${e.name}`}
38+
/>
39+
))}
40+
{diff.newEntities.map((e) => (
41+
<DiffLine
42+
key={`new-${e.name}`}
43+
icon="Plus"
44+
color="text-green-600"
45+
label={`Added: ${e.name}`}
46+
/>
47+
))}
48+
{diff.changedEntities.map((e) => (
49+
<DiffLine
50+
key={`changed-${e.name}`}
51+
icon="RefreshCw"
52+
color="text-amber-500"
53+
label={`Changed: ${e.name}`}
54+
/>
55+
))}
56+
</BlockStack>
57+
</BlockStack>
58+
);
59+
}
60+
61+
function DiffLine({
62+
icon,
63+
color,
64+
label,
65+
}: {
66+
icon: "Minus" | "Plus" | "RefreshCw";
67+
color: string;
68+
label: string;
69+
}) {
70+
return (
71+
<InlineStack gap="1" blockAlign="start">
72+
<Icon name={icon} size="xs" className={`${color} mt-0.5 shrink-0`} />
73+
<Text size="xs">{label}</Text>
74+
</InlineStack>
75+
);
76+
}
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
import { DiffSection } from "@/components/shared/ComponentDiff/DiffSection";
2+
import { Button } from "@/components/ui/button";
3+
import {
4+
Dialog,
5+
DialogContent,
6+
DialogDescription,
7+
DialogHeader,
8+
DialogTitle,
9+
} from "@/components/ui/dialog";
10+
import { Icon, type IconName } from "@/components/ui/icon";
11+
import { BlockStack } from "@/components/ui/layout";
12+
import { Text } from "@/components/ui/typography";
13+
import type { EntityDiff } from "@/utils/componentSpecDiff";
14+
import { hasIODiff } from "@/utils/componentSpecDiff";
15+
16+
import type { SaveAction } from "./saveAction";
17+
18+
export interface ChooseSaveActionDialogProps {
19+
taskName: string;
20+
inputDiff: EntityDiff<{ name: string }>;
21+
outputDiff: EntityDiff<{ name: string }>;
22+
/** Whether to offer "Place as a new task" (enabled once placement ships). */
23+
allowPlace?: boolean;
24+
onChoose: (action: SaveAction) => void;
25+
}
26+
27+
export function ChooseSaveActionDialog({
28+
taskName,
29+
inputDiff,
30+
outputDiff,
31+
allowPlace = false,
32+
onChoose,
33+
}: ChooseSaveActionDialogProps) {
34+
const showDiff = hasIODiff(inputDiff, outputDiff);
35+
36+
return (
37+
<Dialog open onOpenChange={(open) => !open && onChoose("cancel")}>
38+
<DialogContent className="sm:max-w-md">
39+
<DialogHeader>
40+
<DialogTitle>Apply your edit</DialogTitle>
41+
<DialogDescription>
42+
Choose what to do with your changes to “{taskName}”.
43+
</DialogDescription>
44+
</DialogHeader>
45+
46+
{showDiff && (
47+
<BlockStack gap="2">
48+
<DiffSection label="Input" diff={inputDiff} />
49+
<DiffSection label="Output" diff={outputDiff} />
50+
</BlockStack>
51+
)}
52+
53+
<BlockStack gap="2">
54+
<ActionRow
55+
icon="RefreshCw"
56+
title="Update this task"
57+
description="Apply the edit to this task"
58+
onClick={() => onChoose("update")}
59+
autoFocus
60+
/>
61+
<ActionRow
62+
icon="Download"
63+
title="Import to library"
64+
description="Save as a new reusable component"
65+
onClick={() => onChoose("import")}
66+
/>
67+
{allowPlace && (
68+
<ActionRow
69+
icon="Plus"
70+
title="Place as a new task"
71+
description="Keep this task; add a new one nearby"
72+
onClick={() => onChoose("place")}
73+
/>
74+
)}
75+
</BlockStack>
76+
</DialogContent>
77+
</Dialog>
78+
);
79+
}
80+
81+
function ActionRow({
82+
icon,
83+
title,
84+
description,
85+
onClick,
86+
autoFocus,
87+
}: {
88+
icon: IconName;
89+
title: string;
90+
description: string;
91+
onClick: () => void;
92+
autoFocus?: boolean;
93+
}) {
94+
return (
95+
<Button
96+
variant="outline"
97+
className="h-auto w-full justify-start gap-3 p-3"
98+
onClick={onClick}
99+
autoFocus={autoFocus}
100+
>
101+
<Icon name={icon} size="sm" className="shrink-0 text-muted-foreground" />
102+
<span className="flex flex-col items-start gap-0.5 text-left">
103+
<Text size="sm" weight="semibold">
104+
{title}
105+
</Text>
106+
<Text size="xs" tone="subdued">
107+
{description}
108+
</Text>
109+
</span>
110+
</Button>
111+
);
112+
}

src/components/shared/ComponentEditor/ComponentEditorDialog.test.tsx

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,9 +372,10 @@ describe("<ComponentEditorDialog />", () => {
372372
fireEvent.click(screen.getByRole("button", { name: /Save/i }));
373373

374374
await waitFor(() => {
375-
// The edited component is handed to the caller to apply.
375+
// The edited component is handed to the caller to apply in place.
376376
expect(onComponentSavedMock).toHaveBeenCalledWith(
377377
mockHydratedComponent,
378+
"update",
378379
);
379380
expect(onCloseMock).toHaveBeenCalledTimes(1);
380381
});
@@ -386,5 +387,84 @@ describe("<ComponentEditorDialog />", () => {
386387
"success",
387388
);
388389
});
390+
391+
test("uses resolveSaveAction: 'import' runs the library path, not onComponentSaved", async () => {
392+
const onCloseMock = vi.fn();
393+
const onComponentSavedMock = vi.fn();
394+
const resolveSaveAction = vi.fn().mockResolvedValue("import");
395+
const mockHydratedComponent = {
396+
spec: { implementation: { container: { image: "test" } } },
397+
name: "test-component",
398+
digest: "abc123",
399+
text: "name: test-component",
400+
};
401+
vi.mocked(hydrateComponentReference).mockResolvedValue(
402+
mockHydratedComponent,
403+
);
404+
405+
renderWithProviders(
406+
<ComponentEditorDialog
407+
text="name: test-component"
408+
onClose={onCloseMock}
409+
onComponentSaved={onComponentSavedMock}
410+
resolveSaveAction={resolveSaveAction}
411+
/>,
412+
);
413+
414+
await waitFor(() => {
415+
expect(
416+
screen.getByRole("button", { name: /Save/i }),
417+
).toBeInTheDocument();
418+
});
419+
fireEvent.click(screen.getByRole("button", { name: /Save/i }));
420+
421+
await waitFor(() => {
422+
expect(resolveSaveAction).toHaveBeenCalledWith(mockHydratedComponent);
423+
expect(mockAddToComponentLibrary).toHaveBeenCalledWith(
424+
mockHydratedComponent,
425+
"editor_save",
426+
);
427+
expect(onCloseMock).toHaveBeenCalledTimes(1);
428+
});
429+
expect(onComponentSavedMock).not.toHaveBeenCalled();
430+
});
431+
432+
test("uses resolveSaveAction: 'cancel' keeps the editor open and applies nothing", async () => {
433+
const onCloseMock = vi.fn();
434+
const onComponentSavedMock = vi.fn();
435+
const resolveSaveAction = vi.fn().mockResolvedValue("cancel");
436+
const mockHydratedComponent = {
437+
spec: { implementation: { container: { image: "test" } } },
438+
name: "test-component",
439+
digest: "abc123",
440+
text: "name: test-component",
441+
};
442+
vi.mocked(hydrateComponentReference).mockResolvedValue(
443+
mockHydratedComponent,
444+
);
445+
446+
renderWithProviders(
447+
<ComponentEditorDialog
448+
text="name: test-component"
449+
onClose={onCloseMock}
450+
onComponentSaved={onComponentSavedMock}
451+
resolveSaveAction={resolveSaveAction}
452+
/>,
453+
);
454+
455+
await waitFor(() => {
456+
expect(
457+
screen.getByRole("button", { name: /Save/i }),
458+
).toBeInTheDocument();
459+
});
460+
fireEvent.click(screen.getByRole("button", { name: /Save/i }));
461+
462+
await waitFor(() => {
463+
expect(resolveSaveAction).toHaveBeenCalled();
464+
});
465+
expect(onCloseMock).not.toHaveBeenCalled();
466+
expect(onComponentSavedMock).not.toHaveBeenCalled();
467+
expect(mockAddToComponentLibrary).not.toHaveBeenCalled();
468+
});
389469
});
390470
});

src/components/shared/ComponentEditor/ComponentEditorDialog.tsx

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { FullscreenElement } from "../FullscreenElement";
2121
import { withSuspenseWrapper } from "../SuspenseWrapper";
2222
import { PythonComponentEditor } from "./components/PythonComponentEditor";
2323
import { YamlComponentEditor } from "./components/YamlComponentEditor";
24+
import type { SaveAction } from "./saveAction";
2425
import type { SupportedTemplate, YamlGeneratorOptions } from "./types";
2526
import { useTemplateCodeByName } from "./useTemplateCodeByName";
2627

@@ -76,6 +77,7 @@ export const ComponentEditorDialog = withSuspenseWrapper(
7677
templateName = "empty",
7778
onClose,
7879
onComponentSaved,
80+
resolveSaveAction,
7981
}: {
8082
text?: string;
8183
templateName?: SupportedTemplate;
@@ -84,12 +86,23 @@ export const ComponentEditorDialog = withSuspenseWrapper(
8486
* When provided, the editor is being used to edit an existing target (e.g.
8587
* a selected task's component) rather than to import a brand new component
8688
* into the library. The callback receives the hydrated, edited component
87-
* and is responsible for applying it (and any user feedback). When omitted,
88-
* the editor falls back to importing the component into the library.
89+
* and the chosen {@link SaveAction}, and is responsible for applying it
90+
* (and any user feedback). When omitted, the editor falls back to importing
91+
* the component into the library.
8992
*/
9093
onComponentSaved?: (
9194
hydratedComponent: HydratedComponentReference,
95+
action: SaveAction,
9296
) => void | Promise<void>;
97+
/**
98+
* When provided, the editor asks the caller which {@link SaveAction} to
99+
* take on Save (e.g. by opening a choose-action modal). `"cancel"` keeps
100+
* the editor open; `"import"` runs the library-import path; `"update"` /
101+
* `"place"` are forwarded to {@link onComponentSaved}.
102+
*/
103+
resolveSaveAction?: (
104+
hydratedComponent: HydratedComponentReference,
105+
) => Promise<SaveAction>;
93106
}) => {
94107
const notify = useToastNotification();
95108
const { track } = useAnalytics();
@@ -202,19 +215,52 @@ export const ComponentEditorDialog = withSuspenseWrapper(
202215
updatedAt: Date.now(),
203216
});
204217

205-
onClose();
206-
207-
if (onComponentSaved) {
208-
// Editing an existing target (e.g. a selected task): apply the edit to
209-
// that target instead of importing a new library component. The caller
210-
// owns the success/feedback messaging.
211-
await onComponentSaved(hydratedComponent);
212-
} else {
218+
const importToLibrary = async () => {
213219
await addToComponentLibrary(hydratedComponent, "editor_save");
214220
notify(
215221
`Component ${hydratedComponent.name} imported successfully`,
216222
"success",
217223
);
224+
};
225+
226+
// When a caller wants to choose what to do with the edit (e.g. update the
227+
// selected task vs. import to the library), it provides `resolveSaveAction`.
228+
if (resolveSaveAction) {
229+
const action = await resolveSaveAction(hydratedComponent);
230+
231+
if (action === "cancel") {
232+
// Keep the editor open so the user can keep editing.
233+
track("component_editor.save.cancelled", {
234+
mode,
235+
selected_template: templateName,
236+
});
237+
return;
238+
}
239+
240+
onClose();
241+
242+
if (action === "import") {
243+
await importToLibrary();
244+
} else {
245+
await onComponentSaved?.(hydratedComponent, action);
246+
}
247+
248+
track("component_editor.save.completed", {
249+
mode,
250+
selected_template: templateName,
251+
action,
252+
});
253+
return;
254+
}
255+
256+
// No action resolver: editing an existing target updates it in place;
257+
// otherwise (create/import flow) the component is imported to the library.
258+
onClose();
259+
260+
if (onComponentSaved) {
261+
await onComponentSaved(hydratedComponent, "update");
262+
} else {
263+
await importToLibrary();
218264
}
219265

220266
track("component_editor.save.completed", {
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
/**
2+
* What the user chose to do with an edited component definition on Save.
3+
*
4+
* - `update` — apply the edit to the selected task in place
5+
* - `import` — import the edited component into the library as a new component
6+
* - `place` — create a new task from the edit, placed near the selected task
7+
* - `cancel` — dismiss; keep the editor open
8+
*/
9+
export type SaveAction = "update" | "import" | "place" | "cancel";

0 commit comments

Comments
 (0)