diff --git a/packages/dashboard/app/components/WorkflowAddStepModal.tsx b/packages/dashboard/app/components/WorkflowAddStepModal.tsx index 0b00014d67..167342c753 100644 --- a/packages/dashboard/app/components/WorkflowAddStepModal.tsx +++ b/packages/dashboard/app/components/WorkflowAddStepModal.tsx @@ -17,7 +17,11 @@ The simplified workflow view's add-step surface. Requirements: registry, because the flat 16-button advanced palette is the main thing users found hard to use. - When inserting INSIDE a container (foreach/loop/optional-group child - edge), container kinds are hidden — containers cannot nest. + edge), container kinds are hidden — containers cannot nest — and + FRAGMENTS are hidden too (PR #2006 review): fragments expand to top-level + subgraphs, so splicing one into a template-child edge would create + cross-boundary edges into the container. Step templates stay available + (they materialize a single prompt/script node, valid as a sibling child). */ export interface AddStepPaletteEntry { @@ -108,8 +112,8 @@ export function WorkflowAddStepModal({ }, [palette, disallowContainers, q, t]); const filteredFragments = useMemo( - () => fragments.filter((f) => !q || f.name.toLowerCase().includes(q)), - [fragments, q], + () => (disallowContainers ? [] : fragments.filter((f) => !q || f.name.toLowerCase().includes(q))), + [fragments, q, disallowContainers], ); const filteredStepTemplates = useMemo( () => stepTemplates.filter((s) => !q || s.name.toLowerCase().includes(q)), diff --git a/packages/dashboard/app/components/__tests__/WorkflowAddStepModal.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowAddStepModal.test.tsx new file mode 100644 index 0000000000..79ae5ae48a --- /dev/null +++ b/packages/dashboard/app/components/__tests__/WorkflowAddStepModal.test.tsx @@ -0,0 +1,72 @@ +import { render, screen, cleanup } from "@testing-library/react"; +import { describe, expect, it, vi, afterEach } from "vitest"; +import { MessageSquare, Repeat } from "lucide-react"; +import type { WorkflowDefinition, WorkflowStepTemplate } from "@fusion/core"; +import { WorkflowAddStepModal, type AddStepPaletteEntry } from "../WorkflowAddStepModal"; + +/* +FNXC:WorkflowSimpleView 2026-07-12-14:30: +PR #2006 review coverage: when the add-step dialog targets an edge INSIDE a +container (disallowContainers), it must hide container palette kinds, +fragments (top-level subgraphs cannot splice into a template-child edge), +and the "as optional group" template variant — while keeping plain step +templates, which materialize a single sibling-safe node. +*/ + +const palette: AddStepPaletteEntry[] = [ + { kind: "prompt", label: "Prompt", icon: MessageSquare }, + { kind: "loop", label: "Loop", icon: Repeat }, +]; + +const fragment = { + id: "WF-FRAG", + kind: "fragment", + name: "Lint fragment", + description: "", + ir: { version: "v1", name: "Lint fragment", nodes: [], edges: [] }, + layout: {}, + createdAt: "2026-06-03T00:00:00.000Z", + updatedAt: "2026-06-03T00:00:00.000Z", +} as WorkflowDefinition; + +const stepTemplate = { id: "tpl-1", name: "Security review" } as WorkflowStepTemplate; + +function renderModal(disallowContainers: boolean) { + return render( + {}} + palette={palette} + disallowContainers={disallowContainers} + fragments={[fragment]} + stepTemplates={[stepTemplate]} + pluginTemplates={[]} + onPickPalette={vi.fn()} + onPickFragment={vi.fn()} + onPickStepTemplate={vi.fn()} + onPickStepTemplateAsOptionalGroup={vi.fn()} + />, + ); +} + +describe("WorkflowAddStepModal", () => { + afterEach(() => cleanup()); + + it("offers containers, fragments, and optional-group inserts for top-level targets", () => { + renderModal(false); + expect(screen.getByTestId("wf-add-step-loop-loop")).toBeInTheDocument(); + expect(screen.getByTestId("wf-add-step-fragment-WF-FRAG")).toBeInTheDocument(); + expect(screen.getByTestId("wf-add-step-tpl-tpl-1")).toBeInTheDocument(); + expect(screen.getByTestId("wf-add-step-tpl-tpl-1-optional-group")).toBeInTheDocument(); + }); + + it("hides containers, fragments, and optional-group inserts for container-internal targets", () => { + renderModal(true); + expect(screen.getByTestId("wf-add-step-prompt-prompt")).toBeInTheDocument(); + expect(screen.queryByTestId("wf-add-step-loop-loop")).not.toBeInTheDocument(); + expect(screen.queryByTestId("wf-add-step-fragment-WF-FRAG")).not.toBeInTheDocument(); + // Plain step templates remain — they insert a single sibling-safe node. + expect(screen.getByTestId("wf-add-step-tpl-tpl-1")).toBeInTheDocument(); + expect(screen.queryByTestId("wf-add-step-tpl-tpl-1-optional-group")).not.toBeInTheDocument(); + }); +}); diff --git a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx index 44d6492878..1b5a2f48c3 100644 --- a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx +++ b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx @@ -4187,6 +4187,34 @@ describe("WorkflowNodeEditor simplified view modes", () => { (workflow-simple-layout.test.ts) and the toolbar-pick test above, which exercises the same insertFromAddStep path end-to-end. */ + it("splices an edge-targeted 'as optional group' pick into the targeted edge", async () => { + // FNXC:WorkflowSimpleView 2026-07-12-14:30: PR #2006 review coverage — + // the optional-group template variant must wire into the targeted edge, + // not land free-floating. + vi.mocked(fetchWorkflowStepTemplates).mockResolvedValue({ + templates: [{ id: "tpl-sec", name: "Security review", prompt: "Review security", defaultOn: true }], + }); + vi.mocked(updateWorkflow).mockImplementation(async (_id, updates) => ({ ...def(), ...(updates as object) })); + render( {}} addToast={() => {}} />); + await screen.findByTestId("wf-simple-canvas"); + + fireEvent.click(screen.getByTestId("wf-simple-toolbar-add-step")); + const dialog = await screen.findByTestId("wf-add-step-modal"); + fireEvent.click(within(dialog).getByTestId("wf-add-step-tpl-tpl-sec-optional-group")); + await waitFor(() => expect(screen.queryByTestId("wf-add-step-modal")).not.toBeInTheDocument()); + + fireEvent.click(screen.getByText("Save").closest("button")!); + await waitFor(() => expect(updateWorkflow).toHaveBeenCalledTimes(1)); + const [, updates] = vi.mocked(updateWorkflow).mock.calls[0]; + const ir = (updates as { ir: { nodes: Array<{ id: string; kind: string }>; edges: Array<{ from: string; to: string }> } }).ir; + const group = ir.nodes.find((n) => n.kind === "optional-group"); + expect(group).toBeDefined(); + // def()'s single edge into end was the target: merge → group → end. + expect(ir.edges.some((e) => e.from === "merge" && e.to === "end")).toBe(false); + expect(ir.edges.some((e) => e.from === "merge" && e.to === group!.id)).toBe(true); + expect(ir.edges.some((e) => e.from === group!.id && e.to === "end")).toBe(true); + }); + it("keeps built-in workflows read-only in the simplified view", async () => { vi.mocked(fetchWorkflows).mockResolvedValue([builtinDef()]); render( {}} addToast={() => {}} />); diff --git a/packages/dashboard/app/components/__tests__/workflow-lifecycle-autofix.test.ts b/packages/dashboard/app/components/__tests__/workflow-lifecycle-autofix.test.ts index f00ae115dd..c57a67bf52 100644 --- a/packages/dashboard/app/components/__tests__/workflow-lifecycle-autofix.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-lifecycle-autofix.test.ts @@ -50,6 +50,24 @@ describe("lifecycleFixTargetEdgeId", () => { expect(lifecycleFixTargetEdgeId(nodes, edges, "missing-merge-region")).toBe("e3"); }); + it("falls back to the append edge when the merge node has multiple inbound edges", () => { + const nodes = [ + node("start", "start"), + node("a", "prompt", 300, 0), + node("b", "prompt", 300, 200), + node("m", "merge", 600, 0), + node("end", "end", 900, 0), + ]; + const edges = [ + edge("e1", "start", "a"), + edge("e2", "start", "b"), + edge("e3", "a", "m"), + edge("e4", "b", "m"), + edge("e5", "m", "end"), + ]; + expect(lifecycleFixTargetEdgeId(nodes, edges, "missing-completion-summary")).toBe("e5"); + }); + it("falls back to the edge into end when no merge node exists", () => { const nodes = [node("start", "start"), node("end", "end", 360, 0)]; const edges = [edge("e1", "start", "end")]; diff --git a/packages/dashboard/app/components/__tests__/workflow-simple-layout.test.ts b/packages/dashboard/app/components/__tests__/workflow-simple-layout.test.ts index 6a67af72b8..d67daf5962 100644 --- a/packages/dashboard/app/components/__tests__/workflow-simple-layout.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-simple-layout.test.ts @@ -260,6 +260,48 @@ describe("spliceInsertedSubgraphOnEdge", () => { expect(child.position).toEqual({ x: 30, y: 56 }); }); + it("refuses to splice into a container-internal (template child) edge", () => { + // FNXC:WorkflowSimpleView 2026-07-12-14:30: PR #2006 review — subgraphs + // are top-level; splicing into a template-child edge would create + // cross-boundary edges into the container. + const nodes = [ + ...baseNodes(), + node("grp", "foreach", 240, 700, { style: { width: 560, height: 220 } }), + node("c1", "prompt", 30, 56, { parentId: "grp" }), + node("c2", "prompt", 300, 56, { parentId: "grp" }), + node("f1", "gate", 240, 900), + ]; + const edges = [...baseEdges(), edge("t1", "c1", "c2")]; + expect(spliceInsertedSubgraphOnEdge(nodes, edges, "t1", ["f1"])).toBeNull(); + }); + + it("fans out to multiple entries and exits, preserving the inbound condition on each entry", () => { + const nodes = [ + ...baseNodes(), + node("in1", "gate", 200, 700), + node("in2", "script", 500, 700), + node("out", "prompt", 350, 900), + ]; + // Diamond: in1/in2 are entries (no internal inbound), out is the exit. + const edges = [...baseEdges(), edge("i1", "in1", "out"), edge("i2", "in2", "out")]; + const result = spliceInsertedSubgraphOnEdge(nodes, edges, "e2", ["in1", "in2", "out"]); + expect(result).not.toBeNull(); + const inbound = result!.edges.filter((e) => e.source === "a" && ["in1", "in2"].includes(e.target)); + expect(inbound).toHaveLength(2); + expect(inbound.every((e) => e.data?.condition === "failure")).toBe(true); + expect(result!.edges.some((e) => e.source === "out" && e.target === "end")).toBe(true); + }); + + it("falls back to all inserted nodes when the subgraph is an internal cycle (no entries/exits)", () => { + const nodes = [...baseNodes(), node("x", "prompt", 200, 700), node("y", "prompt", 500, 700)]; + const edges = [...baseEdges(), edge("c1", "x", "y"), edge("c2", "y", "x")]; + const result = spliceInsertedSubgraphOnEdge(nodes, edges, "e2", ["x", "y"]); + expect(result).not.toBeNull(); + // Every inserted node is treated as both entry and exit. + expect(result!.edges.filter((e) => e.source === "a" && ["x", "y"].includes(e.target))).toHaveLength(2); + expect(result!.edges.filter((e) => e.target === "end" && ["x", "y"].includes(e.source))).toHaveLength(2); + }); + it("returns null when the target edge is gone or ineligible", () => { const nodes = [...baseNodes(), node("f1", "gate", 240, 700)]; expect(spliceInsertedSubgraphOnEdge(nodes, baseEdges(), "missing", ["f1"])).toBeNull(); diff --git a/packages/dashboard/app/components/workflow-simple-layout.ts b/packages/dashboard/app/components/workflow-simple-layout.ts index 72c234759d..a18d6defad 100644 --- a/packages/dashboard/app/components/workflow-simple-layout.ts +++ b/packages/dashboard/app/components/workflow-simple-layout.ts @@ -299,6 +299,12 @@ export function spliceInsertedSubgraphOnEdge( const source = nodes.find((n) => n.id === edge.source); const target = nodes.find((n) => n.id === edge.target); if (!source || !target) return null; + // FNXC:WorkflowSimpleView 2026-07-12-14:30: PR #2006 review — subgraphs + // insert as TOP-LEVEL nodes, so splicing one into a container-internal + // (template child) edge would wire cross-boundary edges into the container. + // Refuse; the caller falls back to the fixed-position insert, and the + // add-step dialog hides fragments for container-edge targets anyway. + if (source.parentId || target.parentId) return null; const insertedSet = new Set(insertedNodeIds); const insertedTop = nodes.filter(