diff --git a/app/composer/elements/layout.js b/app/composer/elements/layout.js index 8788e323..837780f2 100644 --- a/app/composer/elements/layout.js +++ b/app/composer/elements/layout.js @@ -299,6 +299,15 @@ define("workspace", "docked", ({ }; const state = { left: recall("left", leftOpen), right: recall("right", rightOpen) }; + // What the user last chose, kept apart from what is currently on screen. + // + // `state` is mutated by the one-at-a-time rule below, so it stops being a + // record of anyone's preference the moment a window is narrow. Snapshotting + // it there recorded a panel the RULE had just closed as a panel the user + // wanted closed -- so a page loaded at a narrow width came back from being + // widened with the right panel shut, and nothing the user did would explain + // why. + const wanted = { ...state }; const panels = {}; const toggles = {}; @@ -345,14 +354,17 @@ define("workspace", "docked", ({ panels[which].setAttribute("aria-hidden", String(!state[which])); toggles[which].setAttribute("aria-expanded", String(state[which])); // A window being small is not the user changing their mind, so an automatic - // close does not overwrite what they last chose. - if (keep) remember(which, state[which]); + // close does not overwrite what they last chose -- in storage or here. + if (keep) { + wanted[which] = state[which]; + remember(which, state[which]); + } if (onToggle) onToggle(which, state[which]); } function fit() { if (isNarrow()) { - if (wide === null) wide = { ...state }; + if (wide === null) wide = { ...wanted }; set("left", false, { remember: false }); set("right", false, { remember: false }); } else if (wide) { diff --git a/app/composer/panel.js b/app/composer/panel.js index 7a33d9ed..e2b354db 100644 --- a/app/composer/panel.js +++ b/app/composer/panel.js @@ -118,12 +118,13 @@ export function renderPanel(spec, { values: initial, onChange } = {}) { const values = { ...defaultsOf(spec), ...(initial || {}) }; const rows = el("div", { cls: "cx-group-rows" }); - const handles = new Map(); + let handles = new Map(); function build() { - // Into a fragment first: a row that throws must not leave the previous - // half of the panel on screen. + // Into a fragment, and into a map of its OWN, so a row that throws leaves + // nothing half-made behind and touches nothing already on screen. const built = frag(); + const made = new Map(); for (const [key, setting] of Object.entries(settings)) { if (!visible(setting, values)) continue; const render = rendererFor(setting); @@ -133,14 +134,14 @@ export function renderPanel(spec, { values: initial, onChange } = {}) { // Another setting's visibility may depend on this one. if (dependents.has(key)) redraw(); }); - handles.set(key, control); + made.set(key, control); built.appendChild( SELF_LABELLING.has(rendererNameFor(setting)) ? control.node : composer.settingsRow.default({ label: setting.label, hint: setting.hint, control }).node ); } - return built; + return { built, made }; } // Which keys other rows watch, so a change only redraws when it can matter. @@ -150,12 +151,21 @@ export function renderPanel(spec, { values: initial, onChange } = {}) { } function redraw() { + // Built BEFORE anything is torn down. Destroying first and building second + // meant a row that only appears once another value is set -- so a row the + // first build never rendered and never checked -- emptied the panel and + // then threw, leaving it permanently blank with no way back. + const { built, made } = build(); for (const handle of handles.values()) if (handle.destroy) handle.destroy(); - handles.clear(); - rows.replaceChildren(build()); + handles = made; + rows.replaceChildren(built); } - rows.appendChild(build()); + { + const first = build(); + handles = first.made; + rows.appendChild(first.built); + } const node = el("section", { cls: "cx-group", children: [ el("div", { cls: "cx-eyebrow", text: spec.title }), diff --git a/app/composer/tests/panel_schema.test.js b/app/composer/tests/panel_schema.test.js index 0f0f5a6d..de68b415 100644 --- a/app/composer/tests/panel_schema.test.js +++ b/app/composer/tests/panel_schema.test.js @@ -179,3 +179,59 @@ test("every renderer named by the contract exists", async () => { assert.doesNotThrow(() => rendererFor({ type, default: null }), `${type} default`); } }); + +// --- a redraw is as safe as the first build -------------------------------- + +test("a row that only appears later cannot empty the panel when it throws", () => { + // The initial build is protected by a detached fragment, and the redraw was + // not: it destroyed every control and then built, so a setting revealed by + // another value -- one the first build never rendered and so never checked -- + // emptied the panel and threw on the way to refilling it. A blank panel with + // no way back, from a typo in a `ui` name. + const spec = { + id: "late", title: "Late", + settings: { + mode: { type: "enum", label: "Mode", default: "simple", + options: [{ value: "simple", label: "Simple" }, { value: "full", label: "Full" }] }, + extra: { type: "int", label: "Extra", default: 1, ui: "noSuchRenderer", + when: { mode: "full" } }, + }, + }; + + const panel = renderPanel(spec, {}); + document.body.replaceChildren(panel.node); + // The CONTROLS, not the rows. A control's destroy() takes the control out of + // its settings row and leaves the row behind, so counting rows sees a panel + // that is intact and is looking at labels with nothing under them -- the + // first version of this test did exactly that and passed against the bug. + const controls = () => panel.node.querySelectorAll("input, select, textarea").length; + const before = controls(); + assert.ok(before >= 1, "nothing was rendered to begin with"); + + assert.throws(() => panel.setValue("mode", "full"), /noSuchRenderer/); + assert.equal(controls(), before, + "a redraw that could not finish stripped the controls out of the panel"); +}); + +test("a redraw that succeeds destroys the controls it replaced", () => { + // The fix builds before it tears down, which is exactly where a leak would + // hide: keeping both maps and forgetting to destroy the old one. + const destroyed = []; + const spec = { + id: "swap", title: "Swap", + settings: { + mode: { type: "enum", label: "Mode", default: "a", + options: [{ value: "a", label: "A" }, { value: "b", label: "B" }] }, + only_a: { type: "int", label: "Only A", default: 1, when: { mode: "a" } }, + }, + }; + const panel = renderPanel(spec, {}); + document.body.replaceChildren(panel.node); + const was = panel.node.querySelector('[aria-label="Only A"]'); + assert.ok(was, "the gated row was not rendered"); + + panel.setValue("mode", "b"); + assert.equal(was.isConnected, false, "the replaced control stayed in the document"); + assert.equal(panel.node.querySelector('[aria-label="Only A"]'), null); + void destroyed; +}); diff --git a/app/shell/panels.js b/app/shell/panels.js index 595fba75..e8809eec 100644 --- a/app/shell/panels.js +++ b/app/shell/panels.js @@ -63,9 +63,6 @@ export async function mountAll(manifest, { load = (path) => import(path) } = {}) onChange: (key, value) => values.set(spec.id, key, value), }); - // Only now, with a complete panel in hand, does anything reach the DOM. - host.appendChild(panel.node); - if (ui && typeof ui.setup === "function") { // Kept, not discarded: setup() returns the unsubscribe from on(), and // dropping it means every re-mount leaves another live listener behind. @@ -87,6 +84,12 @@ export async function mountAll(manifest, { load = (path) => import(path) } = {}) }); } + // Only now, with a complete panel AND a setup() that returned, does + // anything reach the DOM. Appending before setup ran left a module whose + // setup threw reported as hidden with its panel still on screen -- which + // is precisely the guarantee this file states it keeps. + host.appendChild(panel.node); + mounted.push({ id: spec.id, panel, diff --git a/app/shell/tests/panels.test.js b/app/shell/tests/panels.test.js new file mode 100644 index 00000000..fca21e97 --- /dev/null +++ b/app/shell/tests/panels.test.js @@ -0,0 +1,80 @@ +// Mounting a module's panel. +// +// The guarantee this file exists to check is stated in panels.js: a module that +// failed is indistinguishable from one nobody installed. No placeholder, no +// greyed row, nothing on screen. It had never been tested against a module that +// fails AFTER its panel is built, which is the only case where there is +// something on screen to leave behind. + +import test from "node:test"; +import assert from "node:assert/strict"; + +import { setupDom, teardownDom } from "../../composer/tests/_dom.js"; + +let mountAll, offer, resetMounts, resetValues; +test.before(async () => { + setupDom(); + await import("../../composer/composer.js"); + ({ mountAll } = await import("../panels.js")); + ({ offer, _reset: resetMounts } = await import("../mounts.js")); + ({ _reset: resetValues } = await import("../values.js")); +}); +test.after(() => teardownDom()); + +function region() { + resetMounts(); + resetValues(); + const host = document.createElement("div"); + document.body.replaceChildren(host); + offer("generation.model", host); + return host; +} + +const spec = (over = {}) => ({ + id: "demo", title: "Demo", mount: "generation.model", + settings: { steps: { type: "int", label: "Steps", default: 20 } }, + ...over, +}); + +test("a module whose setup throws leaves nothing on screen", async () => { + const host = region(); + const { mounted, hidden } = await mountAll( + { modules: [spec({ ui: "./boom.js" })] }, + { load: async () => ({ setup: () => { throw new Error("no"); } }) }); + + assert.deepEqual(mounted, []); + assert.equal(hidden.length, 1); + assert.equal(host.childElementCount, 0, + "a hidden module left its panel in the document"); +}); + +test("a module whose ui.js will not import leaves nothing on screen", async () => { + const host = region(); + const { hidden } = await mountAll( + { modules: [spec({ ui: "./missing.js" })] }, + { load: async () => { throw new Error("404"); } }); + + assert.equal(hidden.length, 1); + assert.equal(host.childElementCount, 0); +}); + +test("a module that mounts is on screen, so the checks above can fail", async () => { + // Without this the two tests above pass on a mountAll that mounts nothing. + const host = region(); + const { mounted, hidden } = await mountAll({ modules: [spec()] }); + assert.deepEqual(hidden, []); + assert.equal(mounted.length, 1); + assert.equal(host.childElementCount, 1); +}); + +test("a module's setup still runs, and its teardown is kept", async () => { + const host = region(); + let torn = 0; + const { mounted } = await mountAll( + { modules: [spec({ ui: "./ok.js" })] }, + { load: async () => ({ setup: () => () => { torn += 1; } }) }); + + assert.equal(host.childElementCount, 1); + mounted[0].destroy(); + assert.equal(torn, 1, "the unsubscribe setup() returned was dropped"); +}); diff --git a/app/tests-layout/workspace.spec.js b/app/tests-layout/workspace.spec.js index f7eaa1af..3fd527e3 100644 --- a/app/tests-layout/workspace.spec.js +++ b/app/tests-layout/workspace.spec.js @@ -195,3 +195,33 @@ test("a settings label is not squeezed into three lines at a square window", asy const right = (await widths(page)).right; expect(right, "the properties column is too narrow to read").toBeGreaterThanOrEqual(260); }); + +test("a page loaded narrow gives both panels back when there is room", async ({ page }) => { + // The asymmetry nothing caught. Loading narrow with both panels remembered + // open runs the one-at-a-time rule during construction, which closes the + // right one -- and the snapshot of "what to restore" was taken AFTER that, so + // it recorded a panel the rule had just closed as a panel the user wanted + // closed. Widening then brought back only the left, permanently, with nothing + // the user did to explain it. + await page.addInitScript(() => { + window.localStorage.setItem("cx.ws.main.left", "1"); + window.localStorage.setItem("cx.ws.main.right", "1"); + }); + await page.setViewportSize({ width: 420, height: 780 }); + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + + const narrow = await widths(page); + expect(narrow.left, "a panel was docked in a window too narrow for it").toBe(0); + expect(narrow.right).toBe(0); + + await page.setViewportSize({ width: 1280, height: 900 }); + await page.waitForFunction(() => { + const el = document.querySelector(".cx-workspace-right"); + return el && el.getBoundingClientRect().width > 0; + }, null, { timeout: 3000 }).catch(() => {}); + + const wide = await widths(page); + expect(wide.left, "the left panel did not come back").toBeGreaterThan(0); + expect(wide.right, "the right panel never came back").toBeGreaterThan(0); +});