diff --git a/app/boot.js b/app/boot.js index ae892d88..225a1f33 100644 --- a/app/boot.js +++ b/app/boot.js @@ -13,7 +13,7 @@ import { createRun, viewUrl, DONE, FAILED, CANCELLED } from "./shell/run.js"; import { clientId, connect, queuedFor, finishedFor } from "./shell/client.js"; import { wire, waitForTerminal } from "./shell/session.js"; import { check, load, describe, search } from "./shell/pipeline.js"; -import { open as openPipeline } from "./shell/pipeline_window.js"; +import { mount as mountPipeline } from "./shell/pipeline_window.js"; import { createPrompts } from "./shell/prompt.js"; import { createProject } from "./shell/projects.js"; import { createTimeline } from "./shell/timeline.js"; @@ -21,10 +21,10 @@ import { createInspector } from "./shell/inspector.js"; import { createWheel } from "./shell/wheel.js"; import { offerAction } from "./shell/actions.js"; import { open as openWizard } from "./shell/wizard.js"; -import { open as openUpdates, status as gitStatus } from "./shell/updates.js"; -import { open as openPacks } from "./shell/packs.js"; -import { open as openLog } from "./shell/logwindow.js"; -import { open as openTemp } from "./shell/tempfiles.js"; +import { mount as mountUpdates, status as gitStatus } from "./shell/updates.js"; +import { mount as mountPacks } from "./shell/packs.js"; +import { mount as mountLog } from "./shell/logwindow.js"; +import { mount as mountTemp } from "./shell/tempfiles.js"; import { createSettingsWindow } from "./shell/settings_window.js"; const root = document.querySelector("#app"); @@ -194,16 +194,16 @@ async function start() { }); // One window for everything that is a PREFERENCE rather than an edit to the - // project on screen. Each of these already has its own real, tested modal -- - // a settings section here is a deep link to it, not a rebuild of it. + // project on screen. Each section mounts the same content its standalone + // window would build -- pipeline_window.js etc. -- inside this one instead. const settings = createSettingsWindow({ gitStatus, sections: [ { id: "pipeline", title: "Models and pipeline", subtitle: "What the run is made of.", keywords: "pipeline models nodes slots loaders", icon: "▦", - action: () => openPipeline({ - load, describe, check, search, + mount: (ctx) => mountPipeline({ + load, describe, check, search, ...ctx, onApply: (next) => { slots = next; // The boxes on the main window are for inputs of THESE slots. A slot @@ -224,25 +224,29 @@ async function start() { keywords: "update git branch restart pull checkout", icon: "⟳", // The window asks whether a run is in flight, because the restart that // follows an update would take it with it. - action: () => openUpdates({ running: () => ["queued", "running"].includes(run.state.phase) }), + mount: (ctx) => mountUpdates({ + ...ctx, running: () => ["queued", "running"].includes(run.state.phase), + }), }, { id: "packs", title: "Node packs", subtitle: "Install, update, or remove a custom_nodes pack.", keywords: "node packs custom_nodes install", icon: "▣", - action: () => openPacks(), + mount: (ctx) => mountPacks(ctx), }, { id: "log", title: "ComfyUI log", subtitle: "The server's own log, without opening devtools.", keywords: "log console errors output", icon: "▤", - action: () => openLog(), + mount: (ctx) => mountLog(ctx), }, { id: "temp", title: "Temp files", subtitle: "Where a file went when it did not land in the bin.", keywords: "temp files output directory", icon: "▥", // Opening one puts it in the Preview, which is where somebody hunting // for a file wants it -- the same place a result from the bin goes. - action: () => openTemp({ onOpen: (item) => page.viewer.setSource( - item.url, item.kind, item.file ? { ...item.file, type: "temp" } : null) }), + mount: (ctx) => mountTemp({ + ...ctx, onOpen: (item) => page.viewer.setSource( + item.url, item.kind, item.file ? { ...item.file, type: "temp" } : null), + }), }, ], }); diff --git a/app/composer/elements/input.js b/app/composer/elements/input.js index 64432ec8..276198dd 100644 --- a/app/composer/elements/input.js +++ b/app/composer/elements/input.js @@ -160,8 +160,11 @@ define("filterList", "md", ({ items = [], value, onChange, placeholder = "Search function draw() { const q = search.value.trim().toLowerCase(); + // `keywords` is searched but never drawn -- a way in that does not name + // itself in the visible label/hint (a section called "About FunPack" + // found by typing "commit", say). const shown = items.filter((i) => - !q || `${i.label} ${i.hint || ""}`.toLowerCase().includes(q)); + !q || `${i.label} ${i.hint || ""} ${i.keywords || ""}`.toLowerCase().includes(q)); list.replaceChildren(); if (!shown.length) { list.append(el("div", { cls: "cx-filter-empty", text: empty })); return; } for (const item of shown) { diff --git a/app/shell/logwindow.js b/app/shell/logwindow.js index e6dcb944..454ea695 100644 --- a/app/shell/logwindow.js +++ b/app/shell/logwindow.js @@ -13,8 +13,10 @@ import { composer } from "../composer/composer.js"; const EVERY = 2000; -export function open({ poll = EVERY } = {}) { - let window_ = null; +/** mount({ poll, setFooter, close }) -> { node, destroy }. See tempfiles.js's + * mount() for why setFooter/close are the only things asked for -- `destroy` + * additionally stops the poll, since this one runs a timer nothing else does. */ +export function mount({ poll = EVERY, setFooter = () => {}, close = () => {} } = {}) { let timer = null; let follow = true; // stick to the end until the user scrolls off it let last = ""; @@ -47,14 +49,12 @@ export function open({ poll = EVERY } = {}) { view.setText(text); if (follow) view.node.scrollTop = view.node.scrollHeight; } - if (window_) window_.setFooter({ note: payload.path || "", actions: footer() }); + setFooter({ note: payload.path || "", actions: footer() }); } catch (err) { // The server being gone is the most interesting thing a log can say, so it // is said -- and the lines already on screen are kept, because they are // the ones from just before it went. - if (window_) { - window_.setFooter({ note: `Cannot reach ComfyUI: ${err.message}`, actions: footer() }); - } + setFooter({ note: `Cannot reach ComfyUI: ${err.message}`, actions: footer() }); } } @@ -66,19 +66,28 @@ export function open({ poll = EVERY } = {}) { if (navigator.clipboard) navigator.clipboard.writeText(last).catch(() => {}); }, }), - composer.button.md({ label: "Close", tone: "primary", - onClick: () => window_ && window_.close("done") }), + composer.button.md({ label: "Close", tone: "primary", onClick: () => close() }), ]; + setFooter({ actions: footer() }); + refresh(); + timer = setInterval(refresh, poll); + return { node: body.node, destroy: () => { clearInterval(timer); timer = null; body.destroy(); } }; +} + +export function open({ poll = EVERY } = {}) { + let window_ = null; + const content = mount({ + poll, + setFooter: (f) => window_ && window_.setFooter(f), + close: () => window_ && window_.close("done"), + }); window_ = composer.modal.generic({ title: "ComfyUI log", subtitle: "What the server printed. Newest at the bottom.", size: "xl", - body, - onClose: () => { clearInterval(timer); timer = null; window_ = null; }, + body: content, + onClose: () => { content.destroy(); window_ = null; }, }); - window_.setFooter({ actions: footer() }); - refresh(); - timer = setInterval(refresh, poll); return window_; } diff --git a/app/shell/packs.js b/app/shell/packs.js index 84a66232..1aad1794 100644 --- a/app/shell/packs.js +++ b/app/shell/packs.js @@ -33,8 +33,9 @@ function saySince(pack, checked) { return `${pack.branch || "?"} · up to date`; } -export function open() { - let window_ = null; +/** mount({ setFooter, close }) -> a composer handle, no modal chrome. See + * tempfiles.js's mount() for why these two are the only things asked for. */ +export function mount({ setFooter = () => {}, close = () => {} } = {}) { let listing = null; let checked = null; let busy = null; // the pack currently being worked on, by name @@ -121,17 +122,14 @@ export function open() { function draw() { body.set(rows()); - if (window_) { - window_.setFooter({ - note: listing ? listing.root : "", - actions: [ - composer.button.md({ label: "Check for updates", disabled: Boolean(busy), - onClick: () => check() }), - composer.button.md({ label: "Close", tone: "primary", - onClick: () => window_.close("done") }), - ], - }); - } + setFooter({ + note: listing ? listing.root : "", + actions: [ + composer.button.md({ label: "Check for updates", disabled: Boolean(busy), + onClick: () => check() }), + composer.button.md({ label: "Close", tone: "primary", onClick: () => close() }), + ], + }); } async function load() { @@ -154,6 +152,16 @@ export function open() { finally { busy = null; draw(); } } + load(); + return body; +} + +export function open() { + let window_ = null; + const body = mount({ + setFooter: (f) => window_ && window_.setFooter(f), + close: () => window_ && window_.close("done"), + }); window_ = composer.modal.generic({ title: "Node packs", subtitle: "Install, update and remove ComfyUI custom nodes.", @@ -161,6 +169,5 @@ export function open() { body, onClose: () => { window_ = null; }, }); - load(); return window_; } diff --git a/app/shell/pipeline_window.js b/app/shell/pipeline_window.js index 3092f67c..c2880813 100644 --- a/app/shell/pipeline_window.js +++ b/app/shell/pipeline_window.js @@ -53,19 +53,20 @@ const countOf = (n) => (n === 0 ? "empty" : `${n} node${n === 1 ? "" : "s"}`); let current = null; /** - * open({ load, describe, check, search, onApply }) -> handle + * mount({ load, describe, check, search, onApply, setFooter, close }) -> 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. + * `setFooter`/`close` are the two things a modal gives this that a container + * does not have for free -- see tempfiles.js's mount() for the general shape. + * This one additionally exposes its live state (slots/editing/selected/ + * pending) and a few structural actions, because `open()` below and the + * jsdom suite both drive it without reading the DOM back; `open()` forwards + * every one of these rather than wrapping a second copy of this state. */ -export function open({ load, describe, check, search, onApply } = {}) { - if (current) { - current.focus(); - return current; - } +export function mount({ load, describe, check, search, onApply, + setFooter = () => {}, close = () => {} } = {}) { let slots = []; const nodes = new Map(); // class name -> description | null const extraGroups = []; // made in this window, no slot in them yet @@ -75,18 +76,6 @@ export function open({ load, describe, check, search, onApply } = {}) { let notes = { refused: [], incomplete: [] }; const body = composer.region.stack({ gap: "md", label: "Pipeline", fill: true }); - const modal = composer.modal.generic({ - title: "Models and pipeline", - subtitle: "What the run is made of.", - size: "xl", - body, - // 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 @@ -177,11 +166,11 @@ export function open({ load, describe, check, search, onApply } = {}) { if (editing === null) { body.set(index()); // Nothing to save from the index: the cards are a way in, not an edit. - modal.setFooter({}); + setFooter({}); return; } body.set(group(editing)); - modal.setFooter(footer()); + setFooter(footer()); } /** @@ -442,7 +431,7 @@ export function open({ load, describe, check, search, onApply } = {}) { // that fired this and take the cursor out of it mid-edit; leaving the // footer alone left it reading "No changes to save" over an edit that had // just been made. - modal.setFooter(footer()); + setFooter(footer()); } async function saveDraft() { @@ -556,10 +545,9 @@ export function open({ load, describe, check, search, onApply } = {}) { })]); }); - const handle = { - node: modal.node, - close: (reason) => modal.close(reason), - focus: () => modal.node.focus ? modal.node.focus() : undefined, + return { + node: body.node, + destroy: () => body.destroy(), ready, // The window's own view of things, so a test does not have to infer state // by reading the DOM back. @@ -574,6 +562,53 @@ export function open({ load, describe, check, search, onApply } = {}) { _moveTo: moveTo, _remove: remove, }; +} + +/** + * open({ load, describe, check, search, onApply }) -> handle + * + * 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 window_ = null; + const content = mount({ + load, describe, check, search, onApply, + setFooter: (f) => window_ && window_.setFooter(f), + close: () => window_ && window_.close("done"), + }); + window_ = composer.modal.generic({ + title: "Models and pipeline", + subtitle: "What the run is made of.", + size: "xl", + body: content, + // 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; }, + }); + + const handle = { + node: window_.node, + close: (reason) => window_.close(reason), + focus: () => window_.node.focus ? window_.node.focus() : undefined, + ready: content.ready, + get slots() { return content.slots; }, + get editing() { return content.editing; }, + get selected() { return content.selected; }, + get pending() { return content.pending; }, + enter: content.enter, + leave: content.leave, + _replace: content._replace, + _moveTo: content._moveTo, + _remove: content._remove, + }; current = handle; return handle; } diff --git a/app/shell/settings_window.js b/app/shell/settings_window.js index b7ad484d..07b01e1a 100644 --- a/app/shell/settings_window.js +++ b/app/shell/settings_window.js @@ -3,34 +3,40 @@ // menu items (Models and pipeline, Updates, Node packs, Log, Temp files) that // nothing tied together and nothing let you search. // -// A section is either MOUNTED here (About, today) or a DEEP LINK: picking it -// closes this window and opens the real one. Every deep-linked window here -// (pipeline_window, updates, packs, log, tempfiles) already owns real, -// tested modal chrome of its own -- rebuilding each into a mountable pane is -// real surgery on working code for a visual nicety, not a fix for anything -// broken. This gets the one thing that mattered (one findable, searchable -// place to start) without touching any of that. +// Every section is MOUNTED here, in place, beside the same always-visible +// nav list -- picking "Node packs" does not leave this window any more than +// clicking a different System Settings pane leaves System Settings. Each of +// pipeline_window.js/updates.js/packs.js/logwindow.js/tempfiles.js already +// had its content built separately from its own modal chrome (a `body` +// region.stack, footer buttons set on the modal handle); their `mount()` +// exports are that same content, taking `setFooter`/`close` from whoever is +// hosting them instead of owning a modal directly. Their `open()` exports +// are unchanged for anyone opening one standalone. import { composer } from "../composer/composer.js"; -let current = null; - /** * createSettingsWindow({ sections, gitStatus }) -> { open, close } * * `sections`: [{ id, title, subtitle, keywords, icon, - * mount() -> handle } -- OR -- { action() }]. - * A section with `action` is a deep link: picking it closes this window and - * calls `action`. A section with `mount` is drawn in place; `mount()` returns - * a composer element handle (its own `destroy()` is the cleanup, called when - * another section is picked or the window closes). + * mount({ setFooter, close }) -> handle }]. + * `mount()` returns a composer element handle (its own `destroy()` is the + * cleanup, called when another section is picked or the window closes). + * `setFooter`/`close` act on this window's own modal chrome -- a section + * does not know or care whether it is standing alone or hosted here. * * `gitStatus` is injected, not imported, for the same reason every other * shell window takes its network calls as props: testable without a server, * and unable to reach for one that was not handed to it. + * + * `current` lives HERE, per call, not at module scope: a module-level + * singleton would let a second independent settings window (there is only + * ever one in this app today, but nothing enforced that) silently redraw + * the FIRST one's modal instead of opening its own. */ export function createSettingsWindow({ sections = [], gitStatus } = {}) { const all = [aboutSection(gitStatus), ...sections]; + let current = null; function open(id) { if (current) { current.show(id); return current; } @@ -42,15 +48,13 @@ export function createSettingsWindow({ sections = [], gitStatus } = {}) { const spec = all.find((s) => s.id === pick) || all[0]; if (!spec) return; if (activeCleanup) { activeCleanup.destroy?.(); activeCleanup = null; } - if (spec.action) { - // Deep link: this window is not what is being asked for, only how - // it was found. Close first -- two full-size modals stacked is not - // "unified", it is one more window in front of the one just opened. - close(); - spec.action(); - return; - } - const content = spec.mount(); + // Cleared before mounting: a section that sets no footer of its own + // (About) must not go on showing the PREVIOUS section's buttons. + modal.setFooter({}); + const content = spec.mount({ + setFooter: (f) => modal.setFooter(f), + close: () => modal.close("done"), + }); activeCleanup = content; contentStack.set([ composer.header.md({ text: spec.title }), @@ -61,7 +65,7 @@ export function createSettingsWindow({ sections = [], gitStatus } = {}) { } const nav = composer.filterList.md({ - items: all.map((s) => ({ id: s.id, label: s.title, hint: s.subtitle, icon: s.icon })), + items: all.map((s) => ({ id: s.id, label: s.title, hint: s.subtitle, icon: s.icon, keywords: s.keywords })), value: id, onChange: showSection, placeholder: "Search settings", @@ -69,7 +73,7 @@ export function createSettingsWindow({ sections = [], gitStatus } = {}) { const body = composer.splitPane.h({ panes: [nav, contentStack], size: 25, min: 15, label: "Settings" }); const modal = composer.modal.generic({ - title: "Settings", size: "lg", body, + title: "Settings", size: "xl", body, onClose: () => { if (activeCleanup) { activeCleanup.destroy?.(); activeCleanup = null; } if (current === handle) current = null; diff --git a/app/shell/tempfiles.js b/app/shell/tempfiles.js index cc174771..f7977869 100644 --- a/app/shell/tempfiles.js +++ b/app/shell/tempfiles.js @@ -23,8 +23,18 @@ function size(bytes) { const when = (mtime) => new Date((Number(mtime) || 0) * 1000).toLocaleTimeString(); -export function open({ onOpen } = {}) { - let window_ = null; +/** + * mount({ onOpen, setFooter, close }) -> a composer handle, no modal chrome. + * + * `setFooter`/`close` are the two things a modal gives this window that a + * container does not have for free: somewhere for Refresh/Close to live + * outside the scrolling body, and a way to leave when a file is picked. + * `open()` below is this plus a modal wired to itself; the unified Settings + * window mounts this directly, wired to ITS OWN footer and close instead -- + * so picking a file from Settings closes Settings, and standalone Temp + * files behaves exactly as it always did. + */ +export function mount({ onOpen, setFooter = () => {}, close = () => {} } = {}) { const body = composer.region.stack({ gap: "sm", fill: true, label: "Temp files" }); const cells = (files) => files.map((f) => ({ @@ -60,7 +70,7 @@ export function open({ onOpen } = {}) { const file = files.find((f) => `${f.subfolder}/${f.filename}` === cell.id); if (!file || !onOpen) return; onOpen({ url: viewUrl({ ...file, type: "temp" }), kind: file.kind, file }); - if (window_) window_.close("opened"); + close(); }, }) // The reason there is nothing, which is not the same as "nothing was made". @@ -69,18 +79,26 @@ export function open({ onOpen } = {}) { hint: payload.detail || "", })]); - if (window_) { - window_.setFooter({ - note: payload.path || "", - actions: [ - composer.button.md({ label: "Refresh", onClick: () => load() }), - composer.button.md({ label: "Close", tone: "primary", - onClick: () => window_.close("done") }), - ], - }); - } + setFooter({ + note: payload.path || "", + actions: [ + composer.button.md({ label: "Refresh", onClick: () => load() }), + composer.button.md({ label: "Close", tone: "primary", onClick: () => close() }), + ], + }); } + load(); + return body; +} + +export function open({ onOpen } = {}) { + let window_ = null; + const body = mount({ + onOpen, + setFooter: (f) => window_ && window_.setFooter(f), + close: () => window_ && window_.close("done"), + }); window_ = composer.modal.generic({ title: "Temp files", subtitle: "Transient ComfyUI outputs. Wiped when the server restarts.", @@ -88,6 +106,5 @@ export function open({ onOpen } = {}) { body, onClose: () => { window_ = null; }, }); - load(); return window_; } diff --git a/app/shell/tests/settings_window.test.js b/app/shell/tests/settings_window.test.js index b765da4c..9383a161 100644 --- a/app/shell/tests/settings_window.test.js +++ b/app/shell/tests/settings_window.test.js @@ -1,6 +1,5 @@ -// The unified Settings window: one modal, a searchable list of sections, and -// two kinds of section -- mounted in place (About), or a deep link that -// closes this window and opens the real one. +// The unified Settings window: one modal, a searchable nav list, and every +// section mounted in place beside it -- picking one never leaves the window. import test from "node:test"; import assert from "node:assert/strict"; @@ -15,15 +14,17 @@ test.before(async () => { }); test.after(() => teardownDom()); -function fakeSection(id, title) { +function fakeSection(id, title, { keywords } = {}) { const destroyed = []; return { - id, title, subtitle: `${title} subtitle`, keywords: title, + id, title, subtitle: `${title} subtitle`, keywords: keywords ?? "", calls: destroyed, - mount() { - const stack = { node: document.createElement("div"), destroy: () => destroyed.push("destroyed") }; - stack.node.textContent = `${title} content`; - return stack; + lastCtx: null, + mount(ctx) { + this.lastCtx = ctx; + const node = document.createElement("div"); + node.textContent = `${title} content`; + return { node, destroy: () => destroyed.push("destroyed") }; }, }; } @@ -37,26 +38,40 @@ test("opening draws the first section by default", () => { w.close(); }); -test("picking a section in the nav swaps the content and tears down the last one", () => { +test("picking a section in the nav swaps the content and tears down the last one, without closing the window", () => { const alpha = fakeSection("a", "Alpha"); const w = createSettingsWindow({ sections: [alpha] }); w.open("a"); assert.match(document.querySelector(".cx-modal").textContent, /Alpha content/); document.querySelector('.cx-filter-row[aria-selected="false"]').click(); // About + assert.equal(document.querySelectorAll(".cx-modal").length, 1, "picking a section opened or closed a modal"); assert.match(document.querySelector(".cx-modal").textContent, /About FunPack/); assert.deepEqual(alpha.calls, ["destroyed"]); w.close(); }); -test("a deep-link section closes this window and calls its own action instead of drawing anything", () => { - let called = 0; - const w = createSettingsWindow({ - sections: [{ id: "packs", title: "Node packs", keywords: "packs", action: () => { called += 1; } }], - }); - w.open("packs"); - assert.equal(called, 1); - assert.equal(document.querySelector(".cx-modal"), null, "the settings window is still open over the real one"); +test("a mounted section gets a setFooter and a close, wired to this window's own modal", () => { + const alpha = fakeSection("a", "Alpha"); + const w = createSettingsWindow({ sections: [alpha] }); + w.open("a"); + assert.equal(typeof alpha.lastCtx.setFooter, "function"); + alpha.lastCtx.setFooter({ note: "hello" }); + assert.match(document.querySelector(".cx-modal-foot").textContent, /hello/); + + alpha.lastCtx.close(); + assert.equal(document.querySelector(".cx-modal"), null, "the section's own close did not close the window"); +}); + +test("switching sections clears the previous one's footer instead of leaving it showing", () => { + const alpha = fakeSection("a", "Alpha"); + const beta = fakeSection("b", "Beta"); + const w = createSettingsWindow({ sections: [alpha, beta] }); + w.open("a"); + alpha.lastCtx.setFooter({ note: "alpha's own note" }); + w.open("b"); + assert.doesNotMatch(document.querySelector(".cx-modal-foot").textContent, /alpha's own note/); + w.close(); }); test("opening again while already open jumps to the requested section instead of stacking a second modal", () => { @@ -68,7 +83,7 @@ test("opening again while already open jumps to the requested section instead of w.close(); }); -test("the search box narrows the list to matching sections", () => { +test("the search box narrows the list to sections matching the title", () => { const w = createSettingsWindow({ sections: [fakeSection("a", "Alpha"), fakeSection("b", "Beta")] }); w.open(); const search = document.querySelector(".cx-search"); @@ -79,6 +94,23 @@ test("the search box narrows the list to matching sections", () => { w.close(); }); +test("the search box also matches a section's keywords, not only its visible title and subtitle", () => { + // The bug this guards: an earlier version only searched `label` + `hint`, + // so a section findable by a word that appears ONLY in its keywords list + // (e.g. "loaders" for a section titled "Models and pipeline") silently + // vanished from the filtered list instead of matching. + const w = createSettingsWindow({ + sections: [fakeSection("a", "Alpha", { keywords: "xyzzy" }), fakeSection("b", "Beta")], + }); + w.open(); + const search = document.querySelector(".cx-search"); + search.value = "xyzzy"; + search.dispatchEvent(new window.Event("input", { bubbles: true })); + const labels = [...document.querySelectorAll(".cx-filter-label")].map((n) => n.textContent); + assert.deepEqual(labels, ["Alpha"]); + w.close(); +}); + test("a gitStatus rejection is shown as a hint rather than left blank or thrown", async () => { const w = createSettingsWindow({ sections: [], gitStatus: () => Promise.reject(new Error("offline")) }); w.open("about"); @@ -95,3 +127,19 @@ test("closing tears down the active section and a later open starts fresh", () = assert.equal(document.querySelector(".cx-modal"), null); assert.deepEqual(alpha.calls, ["destroyed"]); }); + +test("two independent windows do not share which section is open", () => { + // The bug this guards: `current` used to live at module scope, so a second + // createSettingsWindow() instance's open() would redraw the FIRST + // instance's already-open modal instead of opening its own. + const w1 = createSettingsWindow({ sections: [fakeSection("a", "Alpha")] }); + const w2 = createSettingsWindow({ sections: [fakeSection("z", "Zeta")] }); + w1.open("a"); + w2.open("z"); + assert.equal(document.querySelectorAll(".cx-modal").length, 2); + const texts = [...document.querySelectorAll(".cx-modal")].map((m) => m.textContent); + assert.ok(texts.some((t) => t.includes("Alpha content")), "the first window lost its own section"); + assert.ok(texts.some((t) => t.includes("Zeta content")), "the second window did not open its own section"); + w1.close(); + w2.close(); +}); diff --git a/app/shell/updates.js b/app/shell/updates.js index 680c2273..fd901d15 100644 --- a/app/shell/updates.js +++ b/app/shell/updates.js @@ -65,13 +65,14 @@ export function waitForRestart({ poll = 1500, give_up = 180000 } = {}) { } /** - * open() -> the window. One at a time. - * - * Built from a live status every time it opens: what branch you are on and how - * far behind are exactly the facts that go stale while a window is closed. + * mount({ onRestart, running, setFooter, close }) -> a composer handle, no + * modal chrome. See tempfiles.js's mount() for why setFooter/close are the + * only things asked for. Built from a live status the moment it is mounted: + * what branch you are on and how far behind are exactly the facts that go + * stale while this was not on screen. */ -export function open({ onRestart = waitForRestart, running = () => false } = {}) { - let window_ = null; +export function mount({ onRestart = waitForRestart, running = () => false, + setFooter = () => {}, close = () => {} } = {}) { const body = composer.region.stack({ gap: "sm", label: "Updates" }); const act = async (run) => { @@ -88,7 +89,7 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) // Update while already up to date is a normal thing to do, and waiting for // a restart that is not coming would hang on an overlay for three minutes. if (!result || result.restarting === false) { load(); return; } - if (window_) window_.close("restarting"); + close(); onRestart(); } catch (err) { // Said in the window, where the button was pressed. Every refusal this can @@ -136,8 +137,8 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) onClick: () => act(() => ask("POST", "/restart")) }), })); body.set([composer.group.default({ rows })]); - if (window_) window_.setFooter({ actions: [ - composer.button.md({ label: "Close", tone: "ghost", onClick: () => window_.close("done") }), + setFooter({ actions: [ + composer.button.md({ label: "Close", tone: "ghost", onClick: () => close() }), ] }); return; } @@ -173,20 +174,17 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) } body.set([composer.group.default({ rows })]); - if (window_) { - window_.setFooter({ - note: s.ok ? "" : "FunPack is not a git checkout, so it cannot update itself.", - actions: [ - composer.button.md({ label: "Close", tone: "ghost", - onClick: () => window_.close("done") }), - composer.button.md({ - label: s.behind ? `Update (${s.behind})` : "Update", tone: "primary", - disabled: !s.ok || Boolean(s.dirty) || busy, - onClick: () => act(() => ask("POST", "/update", {})), - }), - ], - }); - } + setFooter({ + note: s.ok ? "" : "FunPack is not a git checkout, so it cannot update itself.", + actions: [ + composer.button.md({ label: "Close", tone: "ghost", onClick: () => close() }), + composer.button.md({ + label: s.behind ? `Update (${s.behind})` : "Update", tone: "primary", + disabled: !s.ok || Boolean(s.dirty) || busy, + onClick: () => act(() => ask("POST", "/update", {})), + }), + ], + }); } async function load() { @@ -207,6 +205,20 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) catch { /* the local answer stands, and says it has not checked */ } } + load(); + return body; +} + +/** + * open() -> the window. One at a time. + */ +export function open({ onRestart, running } = {}) { + let window_ = null; + const body = mount({ + onRestart, running, + setFooter: (f) => window_ && window_.setFooter(f), + close: () => window_ && window_.close("done"), + }); window_ = composer.modal.generic({ title: "Updates", subtitle: "FunPack updates itself from git, then restarts ComfyUI.", @@ -214,6 +226,5 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) body, onClose: () => { window_ = null; }, }); - load(); return window_; } diff --git a/app/tests-layout/settings.spec.js b/app/tests-layout/settings.spec.js index b4eb6908..dcc6261a 100644 --- a/app/tests-layout/settings.spec.js +++ b/app/tests-layout/settings.spec.js @@ -1,5 +1,5 @@ -// The unified Settings window: one button, a searchable list, About drawn in -// place, everything else a deep link to its own real window. +// The unified Settings window: one button, a searchable nav list, and every +// section mounted in place beside it -- picking one never leaves the window. import { test, expect } from "@playwright/test"; @@ -26,22 +26,54 @@ test("search narrows the section list to what matches", async ({ page }) => { await expect(rows).toContainText("Node packs"); }); -test("picking a deep-link section closes Settings and opens that window instead", async ({ page }) => { +test("search also matches a section by its keywords, not only its title", async ({ page }) => { await app(page); await page.getByRole("button", { name: "Settings" }).click(); - await page.locator(".cx-filter-row", { hasText: "Node packs" }).click(); - - // Only one modal ever exists at a time -- the deep link replaces this - // window rather than stacking over it. - await expect(page.locator(".cx-modal")).toHaveCount(1); - await expect(page.locator(".cx-modal-title")).toHaveText("Node packs"); + // "loaders" is on the pipeline section's keywords, not its visible title + // ("Models and pipeline") or subtitle ("What the run is made of."). + await page.locator(".cx-modal .cx-search").fill("loaders"); + const rows = page.locator(".cx-modal .cx-filter-row"); + await expect(rows).toHaveCount(1); + await expect(rows).toContainText("Models and pipeline"); }); -test("a fresh open always starts on About, even after a deep link was used last", async ({ page }) => { +test("picking a section swaps the content in place -- the window stays open, and there is only ever one", async ({ page }) => { await app(page); await page.getByRole("button", { name: "Settings" }).click(); await page.locator(".cx-filter-row", { hasText: "Node packs" }).click(); - await page.locator(".cx-modal .cx-icon-btn").click(); // close the packs window + + await expect(page.locator(".cx-modal")).toHaveCount(1); + // "Settings" is the window's own title throughout; the section names + // itself inside the body, beside the still-visible nav list. + await expect(page.locator(".cx-modal-title")).toHaveText("Settings"); + await expect(page.locator(".cx-modal")).toContainText("Node packs"); + await expect(page.locator(".cx-filter-row")).toHaveCount(6); // About + the five, still all there +}); + +test("switching sections tears down the last one -- its own polling stops", async ({ page }) => { + // The log window polls while mounted. Picking it, then picking something + // else, must stop that poll rather than leaving it running against a + // detached view -- proven by leaving the log mounted only briefly and + // confirming the window still works normally afterwards, requesting + // nothing further from a section no longer on screen. + await app(page); + await page.getByRole("button", { name: "Settings" }).click(); + let logRequests = 0; + await page.route("**/api/log**", (route) => { logRequests += 1; route.fulfill({ json: { lines: ["hi"] } }); }); + await page.locator(".cx-filter-row", { hasText: "ComfyUI log" }).click(); + await expect.poll(() => logRequests).toBeGreaterThan(0); + + await page.locator(".cx-filter-row", { hasText: "About FunPack" }).click(); + const before = logRequests; + await page.waitForTimeout(2200); // longer than the log's own 2s poll + expect(logRequests, "the log kept polling after it was navigated away from").toBe(before); +}); + +test("closing and reopening always starts fresh, on About", async ({ page }) => { + await app(page); + await page.getByRole("button", { name: "Settings" }).click(); + await page.locator(".cx-filter-row", { hasText: "Node packs" }).click(); + await page.locator(".cx-modal .cx-icon-btn").click(); // the one close button, for the whole window await page.getByRole("button", { name: "Settings" }).click(); await expect(page.locator(".cx-modal")).toContainText("About FunPack");