app: three ways the UI kept what it said it had thrown away

A module whose setup() threw was reported as hidden with its panel already on
screen -- appended one line too early. The file states the opposite guarantee.

A redraw destroyed every control and then rebuilt, so a row revealed by another
value -- one the first build never rendered and so never checked -- stripped
the panel and threw on the way to refilling it. Building first makes a redraw
as safe as the first build already was.

A page loaded at a narrow width recorded the panel its own one-at-a-time rule
had just closed as one the user wanted closed, so widening brought back only
the left. What the user chose is now kept apart from what is on screen.

The redraw test counted rows and passed against the bug; a control removed from
a row leaves the row behind. It counts controls.

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

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) {

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 }),

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;
+});

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,

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");
+});

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);
+});