app: two findings from the adversarial pass on the Settings rewrite
An outside click on the Settings window closed it unconditionally -- pipeline_window.js's own standalone modal has always refused to close that way (a group edit's draft is the only copy of it), and hosting it inside Settings silently dropped that guarantee, with no way left for a real user to reach the protected version (boot.js has no other caller of pipeline_window.js any more). Settings now holds the door the same way, for every section: an accidental backdrop click during any half-finished edit no longer costs it. filterList's icon `tone` claimed to be a closed set, the same rule el()'s style refusal already holds -- but nothing checked it, so a typo would have rendered a silent, unstyled chip instead of failing loudly the way button.js's own tone check already does for the same mistake. Now it throws, matching that sibling pattern.
This commit is contained in:
@@ -152,6 +152,8 @@ define("textarea", "md", ({ value = "", rows = 4, autoGrow = false, placeholder,
|
||||
* Items are DATA -- {id, label, hint, icon} -- never a render callback, so a
|
||||
* module cannot smuggle markup in through a "just render this" escape hatch.
|
||||
*/
|
||||
const FILTER_ICON_TONES = new Set(["accent", "danger", "warn", "good", "neutral"]);
|
||||
|
||||
define("filterList", "md", ({ items = [], value, onChange, placeholder = "Search", empty = "Nothing matches" } = {}) => {
|
||||
let selected = value;
|
||||
const search = el("input", { cls: ["cx-input", "cx-input-sm", "cx-search", "cx-focusable"],
|
||||
@@ -168,13 +170,18 @@ define("filterList", "md", ({ items = [], value, onChange, placeholder = "Search
|
||||
list.replaceChildren();
|
||||
if (!shown.length) { list.append(el("div", { cls: "cx-filter-empty", text: empty })); return; }
|
||||
for (const item of shown) {
|
||||
// `tone` picks one of a FIXED set of chip colours -- never a raw
|
||||
// colour from data, which is the rule `el()`'s style refusal exists to
|
||||
// hold everywhere else too. Enforced, not just claimed: an unknown
|
||||
// tone throws, the same as button.js's own tone check, rather than
|
||||
// silently rendering an unstyled chip.
|
||||
if (item.tone && !FILTER_ICON_TONES.has(item.tone)) {
|
||||
throw new RangeError(`Unknown filterList icon tone "${item.tone}". Known: ${[...FILTER_ICON_TONES].join(", ")}.`);
|
||||
}
|
||||
const row = el("button", {
|
||||
cls: ["cx-filter-row", "cx-focusable", item.id === selected ? "cx-on" : null],
|
||||
attrs: { type: "button", role: "option", "aria-selected": String(item.id === selected) },
|
||||
children: [
|
||||
// `tone` picks one of a FIXED set of chip colours -- never a raw
|
||||
// colour from data, which is the rule `el()`'s style refusal exists
|
||||
// to hold everywhere else too.
|
||||
item.icon ? el("span", {
|
||||
cls: ["cx-filter-icon", item.tone ? `cx-filter-icon-${item.tone}` : null],
|
||||
text: item.icon,
|
||||
|
||||
@@ -238,6 +238,12 @@ test("filterList gives an icon a coloured chip only from a fixed tone, never fro
|
||||
assert.ok(icon.classList.contains("cx-filter-icon-good"));
|
||||
});
|
||||
|
||||
test("an unknown filterList icon tone is refused rather than rendered as a blank chip", () => {
|
||||
assert.throws(() => mount(composer.filterList.md({
|
||||
items: [{ id: "a", label: "Node packs", icon: "▣", tone: "sparkly-typo" }],
|
||||
})), RangeError);
|
||||
});
|
||||
|
||||
test("filterList says so when nothing matches", () => {
|
||||
const f = mount(composer.filterList.md({ items: [{ id: "a", label: "one" }] }));
|
||||
const search = f.node.querySelector("input");
|
||||
|
||||
@@ -83,6 +83,14 @@ 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: "xl", body,
|
||||
// Mirrors pipeline_window.js's own standalone modal: a half-finished
|
||||
// edit (Models and pipeline's draft, most concretely) must not vanish
|
||||
// because a click landed on the backdrop. This window has no way to
|
||||
// ask "does the section on screen have unsaved work" per section, so
|
||||
// it holds the door for all of them, the same conservative call
|
||||
// pipeline_window.js already made for itself -- the X button and every
|
||||
// section's own "Close" still work.
|
||||
closeOnOutside: false,
|
||||
onClose: () => {
|
||||
if (activeCleanup) { activeCleanup.destroy?.(); activeCleanup = null; }
|
||||
if (current === handle) current = null;
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import { setupDom, teardownDom } from "../../composer/tests/_dom.js";
|
||||
import { setupDom, teardownDom, fire } from "../../composer/tests/_dom.js";
|
||||
|
||||
let createSettingsWindow;
|
||||
test.before(async () => {
|
||||
@@ -119,6 +119,20 @@ test("a gitStatus rejection is shown as a hint rather than left blank or thrown"
|
||||
w.close();
|
||||
});
|
||||
|
||||
test("a click outside the window does not close it -- a half-finished edit in any section is not a click away from gone", () => {
|
||||
// The bug this guards: pipeline_window.js's own standalone modal has
|
||||
// ALWAYS refused to close on an outside click (a group edit's draft is
|
||||
// the only copy of it), but Settings' own modal was built without
|
||||
// carrying that over, so hosting Models and pipeline in here silently
|
||||
// lost the protection its own file was written to guarantee.
|
||||
const alpha = fakeSection("a", "Alpha");
|
||||
const w = createSettingsWindow({ sections: [alpha] });
|
||||
w.open("a");
|
||||
fire(document.body, "pointerdown");
|
||||
assert.ok(document.querySelector(".cx-modal"), "an outside click closed the window");
|
||||
w.close();
|
||||
});
|
||||
|
||||
test("closing tears down the active section and a later open starts fresh", () => {
|
||||
const alpha = fakeSection("a", "Alpha");
|
||||
const w = createSettingsWindow({ sections: [alpha] });
|
||||
|
||||
Reference in New Issue
Block a user