diff --git a/app/shell/pipeline_window.js b/app/shell/pipeline_window.js index e217b9d3..3092f67c 100644 --- a/app/shell/pipeline_window.js +++ b/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; } diff --git a/app/shell/tests/pipeline_window.test.js b/app/shell/tests/pipeline_window.test.js index c04c2c6c..269d608a 100644 --- a/app/shell/tests/pipeline_window.test.js +++ b/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(); +}); diff --git a/app/tests-layout/pipeline.spec.js b/app/tests-layout/pipeline.spec.js index 3cbd34a4..2cafffa4 100644 --- a/app/tests-layout/pipeline.spec.js +++ b/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); +});