Merge: sibling Y-order + reorder-branches + clean fan-out extraction

Author: DylanMerigaud

components/workflow-graph.tsx

@@ -290,6 +290,25 @@ const layout = (
     childrenOf.set(e.source, [...(childrenOf.get(e.source) ?? []), e.target]);
     parentsOf.set(e.target, [...(parentsOf.get(e.target) ?? []), e.source]);
   }
+
+  // SIBLING ORDER (top→bottom) follows the parent's `next` order, NOT dagre's
+  // crossing-minimisation (which can reshuffle). We keep the exact Y-slots dagre
+  // computed for a parent's children (so spacing + measured heights are respected),
+  // but RE-ASSIGN those slots to the children in `next` order. Only for siblings
+  // that belong to a single parent (true fan-out branches) — a shared join node
+  // like the post isn't reordered.
+  for (const [, children] of childrenOf) {
+    const branches = children.filter(
+      (c) => (parentsOf.get(c) ?? []).length === 1,
+    );
+    if (branches.length < 2) continue;
+    // The Y-slots these branches currently occupy, top→bottom.
+    const slots = branches.map((c) => cy.get(c) ?? 0).sort((a, b) => a - b);
+    // `branches` is already in `next` order (childrenOf is built from edge order),
+    // so assign the i-th branch to the i-th slot from the top.
+    branches.forEach((c, i) => cy.set(c, slots[i] ?? cy.get(c) ?? 0));
+  }
+
   // Parents centered on their children (right→left so children settle first).
   for (const id of [...g.nodes()].sort((a, b) => g.node(b).x - g.node(a).x)) {
     const kids = childrenOf.get(id) ?? [];

lib/workflow-edit.test.ts

@@ -266,6 +266,29 @@ test("add-parallel-after that would cycle is rejected (workflow unchanged)", ()
   assert.equal(JSON.stringify(next), before, "cycle guard kept it unchanged");
 });
 
+test("reorder-branches: sets the parent's next order, keeps all edges", () => {
+  // base: manager → [director, post]. Reorder to [post, director].
+  const next = applyEditOp(base, {
+    op: "reorder-branches",
+    parentStepId: "manager",
+    order: ["post", "director"],
+  });
+  const mgr = next.steps.find((s) => s.id === "manager");
+  assert.deepEqual(mgr?.next, ["post", "director"], "order applied");
+});
+
+test("reorder-branches: omitted children are kept (no edge dropped)", () => {
+  const next = applyEditOp(base, {
+    op: "reorder-branches",
+    parentStepId: "manager",
+    order: ["post"], // director omitted
+  });
+  const mgr = next.steps.find((s) => s.id === "manager");
+  assert.ok(mgr?.next.includes("director"), "omitted child still present");
+  assert.ok(mgr?.next.includes("post"));
+  assert.equal(mgr?.next.length, 2, "no edge dropped or duplicated");
+});
+
 test("rename-step: changes only the label, nothing else", () => {
   const next = applyEditOp(base, {
     op: "rename-step",
@@ -299,10 +322,8 @@ test("duplicate-step: makes a parallel twin with the same successors", () => {
   assert.doesNotThrow(() => ApprovalWorkflow.parse(next));
 });
 
-test("move-step: relocates a step after the anchor, predecessors bypass it", () => {
-  // base: manager → {director, post}; director → post. Move director after post?
-  // post is terminal — moving director after post: post → director → (post's old
-  // next = []). And manager bypasses director to director's old next (post).
+test("move-step: relocates a step after the anchor", () => {
+  // base: manager → {director, post}; director → post. Move director after post.
   const next = applyEditOp(base, {
     op: "move-step",
     stepId: "director",
@@ -316,14 +337,119 @@ test("move-step: relocates a step after the anchor, predecessors bypass it", ()
     !mgr.next.includes("director"),
     "manager no longer points at director",
   );
-  assert.ok(
-    mgr.next.includes("post"),
-    "manager bypasses to director's old next",
-  );
   assert.ok(post.next.includes("director"), "post → director (new position)");
   assert.doesNotThrow(() => ApprovalWorkflow.parse(next));
 });
 
+test("move-step: extracting a parallel branch into sequence leaves NO stray edge", () => {
+  // manager → {dir, dept} (parallel), both → post. Move dept after dir ⇒ a clean
+  // chain manager → dir → dept → post, with NO leftover manager → post bypass.
+  const wf: TWorkflow = {
+    name: "par",
+    roots: ["mgr"],
+    steps: [
+      {
+        id: "mgr",
+        kind: "approval",
+        label: "Manager",
+        when: { kind: "always" },
+        approverTitle: "M",
+        approverName: "x",
+        next: ["dir", "dept"],
+      },
+      {
+        id: "dir",
+        kind: "approval",
+        label: "Director",
+        when: { kind: "always" },
+        approverTitle: "D",
+        approverName: "y",
+        next: ["post"],
+      },
+      {
+        id: "dept",
+        kind: "approval",
+        label: "Dept",
+        when: { kind: "always" },
+        approverTitle: "H",
+        approverName: "z",
+        next: ["post"],
+      },
+      {
+        id: "post",
+        kind: "integration",
+        label: "Post",
+        when: { kind: "always" },
+        integration: "netsuite",
+        next: [],
+      },
+    ],
+  };
+  const next = applyEditOp(wf, {
+    op: "move-step",
+    stepId: "dept",
+    afterStepId: "dir",
+  });
+  const get = (id: string) => next.steps.find((s) => s.id === id);
+  assert.deepEqual(
+    get("mgr")?.next,
+    ["dir"],
+    "manager → dir only (no stray post)",
+  );
+  assert.deepEqual(get("dir")?.next, ["dept"], "dir → dept");
+  assert.deepEqual(get("dept")?.next, ["post"], "dept → post");
+  assert.doesNotThrow(() => ApprovalWorkflow.parse(next));
+});
+
+test("move-step: a linear-chain move still bypasses (keeps flow)", () => {
+  // a → b → c. Move b after c: a had ONLY b, so a must bypass to b's old next (c).
+  const wf: TWorkflow = {
+    name: "lin",
+    roots: ["a"],
+    steps: [
+      {
+        id: "a",
+        kind: "approval",
+        label: "A",
+        when: { kind: "always" },
+        approverTitle: "A",
+        approverName: "x",
+        next: ["b"],
+      },
+      {
+        id: "b",
+        kind: "approval",
+        label: "B",
+        when: { kind: "always" },
+        approverTitle: "B",
+        approverName: "y",
+        next: ["c"],
+      },
+      {
+        id: "c",
+        kind: "integration",
+        label: "C",
+        when: { kind: "always" },
+        integration: "netsuite",
+        next: [],
+      },
+    ],
+  };
+  const next = applyEditOp(wf, {
+    op: "move-step",
+    stepId: "b",
+    afterStepId: "c",
+  });
+  const get = (id: string) => next.steps.find((s) => s.id === id);
+  assert.deepEqual(
+    get("a")?.next,
+    ["c"],
+    "a bypasses to c (would be orphaned otherwise)",
+  );
+  assert.deepEqual(get("c")?.next, ["b"], "c → b (new position)");
+  assert.doesNotThrow(() => ApprovalWorkflow.parse(next));
+});
+
 test("move-step keeps the graph acyclic (it unhooks before re-inserting)", () => {
   // move-step always detaches the step (predecessors bypass it) before re-parenting
   // it under the anchor, so it can't introduce a back-edge — the result stays a DAG

lib/workflow-edit.ts

@@ -94,6 +94,15 @@ export const WorkflowEditOp = z.discriminatedUnion("op", [
       afterStepId: z.string(),
     })
     .strict(),
+  z
+    .object({
+      op: z.literal("reorder-branches"),
+      /** The parent whose parallel branches to reorder, top→bottom. */
+      parentStepId: z.string(),
+      /** The branch step ids in the desired top→bottom order. */
+      order: z.array(z.string()),
+    })
+    .strict(),
   z
     .object({
       op: z.literal("insert-approval-after"),
@@ -241,14 +250,19 @@ export const applyEditOp = (wf: TWorkflow, op: WorkflowEditOp): TWorkflow => {
       const anchor = next.steps.find((s) => s.id === op.afterStepId);
       if (!moved || !anchor || moved.id === anchor.id) return next;
       const movedNext = [...moved.next];
-      // 1. Unhook the moved step: its old predecessors bypass it to its successors.
+      // 1. Unhook the moved step from its old predecessors. Only BYPASS (re-point the
+      //    predecessor at the moved step's successors) when that predecessor would
+      //    otherwise be left with NO way forward — i.e. it relied solely on the moved
+      //    step (a linear chain). When the predecessor has OTHER branches (extracting
+      //    one branch out of a fan-out), drop the edge cleanly with no bypass, so
+      //    "2 parallel → sequential" yields a clean chain, not a stray edge.
       for (const s of next.steps) {
-        if (s.id === moved.id) continue;
-        if (s.next.includes(moved.id)) {
-          s.next = [
-            ...new Set([...s.next.filter((n) => n !== moved.id), ...movedNext]),
-          ];
-        }
+        if (s.id === moved.id || !s.next.includes(moved.id)) continue;
+        const withoutMoved = s.next.filter((n) => n !== moved.id);
+        s.next =
+          withoutMoved.length === 0
+            ? [...new Set(movedNext)] // sole path forward → bypass to keep flow
+            : withoutMoved; // had other branches → clean drop
       }
       next.roots = next.roots.filter((r) => r !== moved.id);
       // 2. Re-insert after the anchor: anchor → moved → anchor's old successors.
@@ -257,6 +271,19 @@ export const applyEditOp = (wf: TWorkflow, op: WorkflowEditOp): TWorkflow => {
       return wouldCycle(next) ? wf : next;
     }
 
+    case "reorder-branches": {
+      const parent = next.steps.find((s) => s.id === op.parentStepId);
+      if (!parent) return next;
+      // Reorder `next` to match the requested top→bottom order. Keep only ids that
+      // are actually current children; append any current child the model omitted so
+      // we never silently drop an edge.
+      const current = new Set(parent.next);
+      const ordered = op.order.filter((id) => current.has(id));
+      const rest = parent.next.filter((id) => !ordered.includes(id));
+      parent.next = [...ordered, ...rest];
+      return next;
+    }
+
     case "add-approval": {
       const id = uniqueId(next, slug(op.label));
       const post = postStepId(next);
@@ -436,6 +463,7 @@ export const WORKFLOW_EDIT_SYSTEM_PROMPT = `You translate a plain-language instr
 - rename-step: change a step's title/label (by "stepId", set "label"). Use for "rename X to Y" / "call it Z".
 - duplicate-step: copy an existing step (by "stepId", set "label" for the copy). Use for "duplicate X" / "add another like X".
 - move-step: relocate an existing step to immediately AFTER another (by "stepId" + "afterStepId"). Use for "move X after Y" / "put X before the post".
+- reorder-branches: change the TOP→BOTTOM order of a parent's parallel branches (by "parentStepId" + "order": the branch step ids in the wanted order). Use for "swap the two reviews" / "put the IT review above the director review". This only changes the visual order of parallel branches, nothing structural.
 - insert-approval-after: insert a new approval gate IMMEDIATELY AFTER one existing step (use "afterStepId"). Use this for "add a step between X and Y" or "after the manager, add …". Same label/approverTitle/amountOver/department fields as add-approval.
 - add-parallel-after: a new approval gate that runs only once ALL of the given steps have been approved (use "afterStepIds": a list). Use this for "after the two reviews, require a final sign-off" / "a step that waits for both X and Y". Same label/approverTitle/amountOver/department fields.
 - none: if the instruction doesn't map to any of the above, or asks for something already true — give a short "reason".
@@ -484,6 +512,7 @@ Return { "ops": [ ... ] } where each op is one of:
 - rename-step { stepId, label }: change a step's title.
 - duplicate-step { stepId, label }: copy an existing step.
 - move-step { stepId, afterStepId }: relocate an existing step to after another.
+- reorder-branches { parentStepId, order: [..] }: set the top→bottom order of a parent's parallel branches ("swap the two reviews").
 - insert-approval-after { afterStepId, label, approverTitle, amountOver|null, department|null }: insert a gate immediately AFTER one step.
 - add-parallel-after { afterStepIds: [..], label, approverTitle, amountOver|null, department|null }: a gate that runs once ALL the listed steps are approved (use for "after X and Y, a step that waits on both").
 - none { reason }: only if NOTHING in the instruction maps to a supported edit.