app: two of the same window, and an edit undone by an older one

The button opened another window on every press -- reported from a real
session as two of the same modal. Two windows over one pipeline are two drafts
of it: whichever is saved last wins, and the other was edited against a
pipeline that had already moved. One window now, focused rather than stacked,
and a fresh one after it is closed.

commit() took whatever answer arrived. Moving a node carries the whole slot
list and removing one carries an id, so the second answers first and the first
then describes a pipeline that no longer exists -- putting a removed node back
with no refusal and nothing said. The picker guarded this already; the path
that decides what gets queued did not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

app/shell/pipeline_window.js

@@ -44,13 +44,28 @@ const isLink = (value) => Array.isArray(value) && value.length === 2
 
 const countOf = (n) => (n === 0 ? "empty" : `${n} node${n === 1 ? "" : "s"}`);
 
+// The one that is open, if one is.
+//
+// There is a single pipeline, so there is a single window onto it. Without this
+// the button opened another one on every press, and two windows over the same
+// pipeline are two drafts of it: whichever was saved last wins, and the other
+// was edited against a pipeline that had already moved.
+let current = null;
+
 /**
  * open({ load, describe, check, search, onApply }) -> handle
  *
  * Every server call is injected. The window is then testable without a server
  * and -- more to the point -- cannot reach for one that is not there.
+ *
+ * Opening while one is already open returns THAT one and focuses it, rather
+ * than stacking a second over it.
  */
 export function open({ load, describe, check, search, onApply } = {}) {
+  if (current) {
+    current.focus();
+    return current;
+  }
   let slots = [];
   const nodes = new Map();          // class name -> description | null
   const extraGroups = [];           // made in this window, no slot in them yet
@@ -68,6 +83,9 @@ export function open({ load, describe, check, search, onApply } = {}) {
     // A half-finished group edit must not vanish because a click landed on the
     // backdrop: the draft is the only copy of it.
     closeOnOutside: false,
+    // However it was closed -- the button, Escape, destroy -- the next press
+    // opens a fresh one rather than finding this one and refusing.
+    onClose: () => { if (current === handle) current = null; },
   });
 
   // ---------------------------------------------------------------- server
@@ -90,16 +108,29 @@ export function open({ load, describe, check, search, onApply } = {}) {
     for (const name of unknown) nodes.set(name, described[name] ?? null);
   }
 
+  // Which edit is the current one. Two structural edits can be in flight at
+  // once -- moving a node carries the whole slot list and a remove carries one
+  // id, so the second can easily answer first -- and the older answer was built
+  // from the pipeline as it stood BEFORE the newer edit. Taking it put the
+  // removed node back with no refusal and nothing said. The picker's search
+  // already guards this; the path that decides what gets QUEUED did not.
+  let edits = 0;
+
   /** Send the pipeline as it now stands and keep what the server says of it. */
   async function commit(next, action = {}) {
+    const mine = ++edits;
     let answer;
     try {
       answer = await check({ slots: next, ...action });
     } catch (err) {
+      if (mine !== edits) return false;
       notes = { refused: [err.message], incomplete: notes.incomplete };
       draw();
       return false;
     }
+    // A newer edit has been sent since. Its answer is the one that describes
+    // the pipeline; this one describes a pipeline that no longer exists.
+    if (mine !== edits) return false;
     // A refused edit did not happen, so what is on screen must stay what it
     // was: showing the refusal beside the change it refused is how someone
     // comes to believe an edit landed when it did not.
@@ -525,9 +556,10 @@ export function open({ load, describe, check, search, onApply } = {}) {
     })]);
   });
 
-  return {
+  const handle = {
     node: modal.node,
     close: (reason) => modal.close(reason),
+    focus: () => modal.node.focus ? modal.node.focus() : undefined,
     ready,
     // The window's own view of things, so a test does not have to infer state
     // by reading the DOM back.
@@ -539,5 +571,15 @@ export function open({ load, describe, check, search, onApply } = {}) {
     leave,
     _replace: (slotId, className) =>
       commit(slots, { action: "replace", slot: slotId, node: className }),
+    _moveTo: moveTo,
+    _remove: remove,
   };
+  current = handle;
+  return handle;
+}
+
+/** For tests, which open and close many windows in one process. */
+export function _reset() {
+  if (current) current.close("reset");
+  current = null;
 }

app/shell/tests/pipeline_window.test.js

@@ -11,12 +11,15 @@ import assert from "node:assert/strict";
 
 import { setupDom, teardownDom, fire } from "../../composer/tests/_dom.js";
 
-let openWindow, groupsOf;
+let openWindow, groupsOf, resetWindow;
 test.before(async () => {
   setupDom();
   await import("../../composer/composer.js");
-  ({ open: openWindow, groupsOf } = await import("../pipeline_window.js"));
+  ({ open: openWindow, groupsOf, _reset: resetWindow } = await import("../pipeline_window.js"));
 });
+// There is one pipeline and so one window onto it; a test that leaves one open
+// would otherwise hand it to the next test instead of opening a fresh one.
+test.beforeEach(() => resetWindow());
 test.after(() => teardownDom());
 
 // --- the fake server -------------------------------------------------------
@@ -480,3 +483,60 @@ test("a value edit is not mistaken for a structural one", async () => {
   assert.equal(api.held.find((s) => s.id === "sampler").inputs.steps, 35);
   win.close();
 });
+
+// --- one window, and one edit at a time -----------------------------------
+
+test("pressing the button twice does not stack a second window", async () => {
+  // Two windows over one pipeline are two drafts of it: whichever is saved
+  // last wins and the other was edited against a pipeline that had moved.
+  const api = server();
+  const first = openWindow(api);
+  await first.ready;
+  const second = openWindow(api);
+
+  assert.equal(second, first, "a second window was opened over the first");
+  assert.equal(document.querySelectorAll(".cx-modal").length, 1);
+  first.close();
+  assert.equal(document.querySelectorAll(".cx-modal").length, 0);
+});
+
+test("closing one lets the next press open a fresh one", async () => {
+  const api = server();
+  const first = openWindow(api);
+  await first.ready;
+  first.close();
+
+  const second = openWindow(api);
+  await second.ready;
+  assert.notEqual(second, first, "the button stopped working after one close");
+  second.close();
+});
+
+test("a slow edit answering late does not undo the edit made after it", async () => {
+  // Moving a node carries the whole slot list; removing one carries an id. The
+  // second answers first, and the first's answer describes a pipeline that no
+  // longer exists -- taking it put the removed node back with no refusal and
+  // nothing said.
+  const api = server();
+  const held = [];
+  const slow = { ...api, check: (body) => new Promise((resolve) => {
+    held.push(() => resolve(api.check(body)));
+  }) };
+
+  const win = openWindow(slow);
+  await win.ready;                     // the load does not go through check()
+  win.enter("Sampling");
+
+  const moving = win._moveTo("model", "Sampling");
+  const removing = win._remove("sampler");
+  assert.equal(held.length, 2, "both edits should be in flight");
+
+  held[1]();                           // the remove answers first
+  await removing;
+  held[0]();                           // the move answers second, and is stale
+  await moving;
+
+  assert.equal(win.slots.some((s) => s.id === "sampler"), false,
+    "a removed node came back when an older edit answered late");
+  win.close();
+});

app/tests-layout/pipeline.spec.js

@@ -96,3 +96,23 @@ test("Save and Cancel hold the bottom instead of scrolling with the settings", a
   const after = await barIn.boundingBox();
   expect(after.y).toBeCloseTo(bar.y, 0);
 });
+
+test("the button opens one window, however many times it is pressed", async ({ page }) => {
+  // Reported from a real session: two of the same modal, one behind the other.
+  // Two windows over one pipeline are two drafts of it -- whichever is saved
+  // last wins, and the other was edited against a pipeline that had moved.
+  await page.goto("/funpack/");
+  await page.waitForFunction(() => window.FunPack !== undefined);
+
+  const button = page.getByRole("button", { name: "Models and pipeline" });
+  await button.click();
+  await expect(page.locator(".cx-modal")).toHaveCount(1);
+  await button.click({ force: true });
+  await button.click({ force: true });
+  await expect(page.locator(".cx-modal")).toHaveCount(1);
+
+  await page.locator(".cx-modal .cx-icon-btn").click();
+  await expect(page.locator(".cx-modal")).toHaveCount(0);
+  await button.click();
+  await expect(page.locator(".cx-modal")).toHaveCount(1);
+});