chore(sidebar): finish #47 tidiness — UX touches + magic-string lift + small refactors (#64)

Closes the remaining checkboxes on #47 (Sidebar follow-ups deferred
from PR #45). Bundles seven items in one PR per the new "default to
bundling small slices" policy: two user-visible UX improvements + five
internal hygiene fixes.

User-visible (Changed)
----------------------
- **Save selection toast differentiates "no selection" from "selection
  points at deleted nodes."** `serializeSelection` returns a
  discriminated `{ kind: "empty" | "stale" | "ok", graph? }` instead
  of plain `null` / graph. The "stale" case (selection set non-empty
  but every id refers to a removed node — common after undo/redo)
  now toasts "Selected node(s) no longer exist. Click a node on the
  canvas to re-select." instead of the misleading generic message.
- **Curated-defaults seed failure now toasts.** Previously
  console-only; users without DevTools never saw it. The most common
  cause (localStorage quota / private-mode rejection) is something
  the user can act on.

Internal (no behavior change)
-----------------------------
- Lifted save-modal action sentinels (`new` / `use_existing` /
  `modify_existing`) into `ACTION_*` named constants at the top of
  `showSaveWorkflowModal`, alongside the pre-existing `NEW_TOP` /
  `SAVE_HERE` cascade sentinels.
- Section-id constants (`SECTION_ID_NODES`, `SECTION_ID_WORKFLOWS`,
  `SECTION_ID_TAGS`) lifted to module scope in `tree.js`. `pinExpanded`
  in the save flow now references the constant instead of the literal
  `"workflows"` string, so a future section-id rename only needs the
  constant updated.
- `modalLabel(text)` factory in `modals.js` replaces 7× repetition of
  the four-line `createElement("label")` + `className` + `textContent`
  + `appendChild` pattern.
- `USERDATA_OVERWRITE_QUERY = "?overwrite=true"` named constant in
  `workflows_store.js`. ComfyUI's userdata API requires this flag to
  allow POST over an existing file; pinning the contract as a
  constant makes it visible at a glance.
- `buildFolder` no longer round-trips folder-expansion state through
  `wrapper.dataset.expanded`. State lives in a closure variable;
  `pathStates` remains the canonical cross-render store.
- `subcategoryFor` fallback path now logs `console.warn` with the
  offending category + `categoryRoot`. Reachable only via a logic
  error (stale `REPOS` config); previously silent.
- Save-modal `onSave` callback contract renamed `dir` → `dirPath` to
  match codebase-wide convention (`dirPath` for arrays of segments,
  `dirName` for single segments, `dir` for resolved DirNode objects).
  Caller updates in `tree.js` paired.

Skipped from #47 (with rationale)
---------------------------------
- The broader `el(tag, props, children)` DOM helper from the issue
  body. Adopting it consistently would touch every modal/folder/leaf
  row construction site (~40+ replacements) for marginal readability
  win. The narrower `modalLabel(text)` is the meaningful slice
  because it replaces an exact 4-line repetition. If `el()` becomes
  desirable later, file as a focused refactor PR.

Verification
------------
- `python tools/preflight_release.py --skip manager-meta -v` PASS.
- `node --input-type=module --check` on every modified JS file —
  clean.
- Synced to live ComfyUI install via `scripts/sync_to_dev.py` for
  smoke-test on Mac.

Closes #47.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
malkuthro
2026-05-05 09:19:48 +02:00
committed by GitHub
co-authored by Claude Opus 4.7
parent bebc6f618d
commit 2f370a962c
6 changed files with 194 additions and 67 deletions
+54
View File
@@ -6,7 +6,61 @@ The format is inspired by Keep a Changelog and SemVer.
## [Unreleased]
### Changed
- **Save selection toast distinguishes "no selection" from "selection
points at deleted nodes."** Previously both produced the generic
"Select one or more nodes on the canvas first." Now the deleted-node
case (selection set non-empty but every id refers to a removed node,
common after undo/redo) shows "Selected node(s) no longer exist.
Click a node on the canvas to re-select." `serializeSelection`
returns a discriminated `{ kind: "empty" | "stale" | "ok", graph? }`
result so the caller can route messaging precisely.
- **Curated-defaults seed failure now toasts.** When
`seedDefaultsIfNeeded` writes the seed list to localStorage and the
write is rejected (full disk, browser quota, private mode), users
see a toast pointing at the console instead of getting a silent
console-only warning.
### Internal (sidebar tidiness pass — no behavior change)
- **Lifted save-modal action sentinels and the cascade picker
sentinels into named constants** at the top of `showSaveWorkflowModal`
in `web/sidebar/modals.js` (`ACTION_NEW`, `ACTION_USE_EXISTING`,
`ACTION_MODIFY_EXISTING`, alongside the existing `NEW_TOP` /
`SAVE_HERE`). Magic strings (`"new"`, `"use_existing"`,
`"modify_existing"`) are gone from the function body.
- **Section-id constants** (`SECTION_ID_NODES`, `SECTION_ID_WORKFLOWS`,
`SECTION_ID_TAGS`) lifted to module scope in `web/sidebar/tree.js`.
The `pinExpanded` save-flow callsite now references the constant
instead of the literal `"workflows"` string, so a future section-id
rename only needs the constant updated.
- **`modalLabel(text)`** factory in `web/sidebar/modals.js` replaces
the seven repetitions of the four-line `createElement("label")` +
`className` + `textContent` + `appendChild` pattern.
- **`USERDATA_OVERWRITE_QUERY = "?overwrite=true"`** named constant in
`web/sidebar/workflows_store.js`. ComfyUI's userdata API requires
this flag to allow POST over an existing file; pinning it as a
constant makes the contract visible at a glance.
- **`buildFolder` no longer round-trips folder-expansion state through
`wrapper.dataset.expanded`.** The DOM dataset forces every value
to a string (and led to the slightly awkward `!== "false"` read);
expansion state now lives in a closure variable. `pathStates` is
the canonical store for cross-render persistence — the dataset was
just mirroring it for reads.
- **`subcategoryFor` fallback path now logs a `console.warn`** with
the offending category and `categoryRoot`. Reachable only via a
logic error (typically a stale `REPOS` entry whose `categoryRoot`
no longer matches the upstream node category prefix); previously
it silently rewrote the category, masking the misconfig.
- **Save-modal `onSave` callback contract renamed `dir` → `dirPath`**
to match the codebase-wide convention (`dirPath` for arrays of
segments, `dirName` for single segments, `dir` for resolved
DirNode objects). Caller updates in `tree.js` paired.
- **`makeToolbarButton({iconClass, title, onClick})`** factory in
`web/sidebar/tree.js` replaces the four near-identical button-construction
blocks in `renderPanel` (export, new-dir, save-canvas, save-selection).
Each block was 5–7 lines of `createElement` / `className` / `innerHTML`
/ `title` / `addEventListener`; calls collapse to 4-line factory invocations.
No behavior change. Closes the "extract `makeToolbarButton`" item on #47.
- **`makeToolbarButton({iconClass, title, onClick})`** factory in
`web/sidebar/tree.js` replaces the four near-identical button-construction
blocks in `renderPanel` (export, new-dir, save-canvas, save-selection).
+27 -11
View File
@@ -33,11 +33,24 @@ function getSelectedNodeIds() {
}
}
// Returns a discriminated result so callers can distinguish "user has not
// selected anything yet" from "user has a selection but every node in it is
// gone" — those produce different toasts in the UI.
//
// Shapes:
// { kind: "empty" } — `selectedIds.size === 0` OR `serializeFullCanvas`
// failed (canvas not ready / threw); caller should
// prompt the user to select something.
// { kind: "stale" } — `selectedIds.size > 0` but none of the IDs match
// a live node on the graph (selection points at
// deleted nodes — common after undo/redo). Caller
// should prompt the user to re-select live nodes.
// { kind: "ok", graph } — success.
export function serializeSelection() {
const selectedIds = getSelectedNodeIds();
if (selectedIds.size === 0) return null;
if (selectedIds.size === 0) return { kind: "empty" };
const full = serializeFullCanvas();
if (!full) return null;
if (!full) return { kind: "empty" };
const internalLinks = (full.links || []).filter(link =>
Array.isArray(link) && selectedIds.has(link[1]) && selectedIds.has(link[3])
@@ -66,17 +79,20 @@ export function serializeSelection() {
return clone;
});
if (nodes.length === 0) return null;
if (nodes.length === 0) return { kind: "stale" };
return {
last_node_id: full.last_node_id,
last_link_id: full.last_link_id,
nodes,
links: internalLinks,
groups: [],
config: full.config || {},
extra: full.extra || {},
version: full.version || 0.4,
kind: "ok",
graph: {
last_node_id: full.last_node_id,
last_link_id: full.last_link_id,
nodes,
links: internalLinks,
groups: [],
config: full.config || {},
extra: full.extra || {},
version: full.version || 0.4,
},
};
}
+46 -38
View File
@@ -67,13 +67,20 @@ function makeModalButton({ label, primary, danger, onClick }) {
return btn;
}
// Tiny factory for the small-caps label rows above modal inputs/selects.
// Replaces the four-line `createElement` + `className` + `textContent` +
// `appendChild` block that appears 7× across the modal helpers below.
function modalLabel(text) {
const lbl = document.createElement("label");
lbl.className = "koolook-modal-label";
lbl.textContent = text;
return lbl;
}
export function showInputModal({ title, label, defaultValue, placeholder, confirmLabel, onSubmit }) {
const body = document.createElement("div");
const lbl = document.createElement("label");
lbl.className = "koolook-modal-label";
lbl.textContent = label || "Name";
body.appendChild(lbl);
body.appendChild(modalLabel(label || "Name"));
const input = document.createElement("input");
input.className = "koolook-modal-input";
@@ -130,14 +137,22 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
// sentinel so the user can stop drilling at any level. Subdirectories
// are created via right-click; "+ New directory…" at the top level
// creates a NEW top-level directory only.
//
// The leading double-underscores on cascade sentinels (`__new__`,
// `__here__`) are deliberate: real directory names cannot start with
// double-underscore in any current store, so the sentinel can never
// collide with a user-created directory's name when stored as a
// <select>'s `value`. The action values are bare lowercase strings
// because they're only ever compared against the action <select>'s
// own `value`, which we control.
const NEW_TOP = "__new__";
const SAVE_HERE = "__here__";
const ACTION_NEW = "new";
const ACTION_USE_EXISTING = "use_existing";
const ACTION_MODIFY_EXISTING = "modify_existing";
const topNames = listDirectoryNames([]);
const dirLbl = document.createElement("label");
dirLbl.className = "koolook-modal-label";
dirLbl.textContent = "Directory";
body.appendChild(dirLbl);
body.appendChild(modalLabel("Directory"));
const cascadeContainer = document.createElement("div");
body.appendChild(cascadeContainer);
@@ -282,9 +297,7 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
if (topNames.length === 0) newDirInput.style.display = "";
// ---- Base on existing (only shown when the chosen directory has workflows) ----
const baseLbl = document.createElement("label");
baseLbl.className = "koolook-modal-label";
baseLbl.textContent = "Base on existing";
const baseLbl = modalLabel("Base on existing");
body.appendChild(baseLbl);
const baseSelect = document.createElement("select");
@@ -292,17 +305,15 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
body.appendChild(baseSelect);
// ---- Action ----
const actionLbl = document.createElement("label");
actionLbl.className = "koolook-modal-label";
actionLbl.textContent = "Action";
const actionLbl = modalLabel("Action");
body.appendChild(actionLbl);
const actionSelect = document.createElement("select");
actionSelect.className = "koolook-modal-select";
[
{ value: "new", label: "New name" },
{ value: "use_existing", label: "Use existing name (archive previous)" },
{ value: "modify_existing", label: "Modify existing name" },
{ value: ACTION_NEW, label: "New name" },
{ value: ACTION_USE_EXISTING, label: "Use existing name (archive previous)" },
{ value: ACTION_MODIFY_EXISTING, label: "Modify existing name" },
].forEach(a => {
const opt = document.createElement("option");
opt.value = a.value;
@@ -312,9 +323,7 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
body.appendChild(actionSelect);
// ---- Workflow name (only shown when needed) ----
const nameLbl = document.createElement("label");
nameLbl.className = "koolook-modal-label";
nameLbl.textContent = "Workflow name";
const nameLbl = modalLabel("Workflow name");
body.appendChild(nameLbl);
const nameInput = document.createElement("input");
@@ -407,7 +416,7 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
actionLbl.style.display = "";
actionSelect.style.display = "";
for (const opt of actionSelect.options) {
if (opt.value === "use_existing" || opt.value === "modify_existing") {
if (opt.value === ACTION_USE_EXISTING || opt.value === ACTION_MODIFY_EXISTING) {
opt.disabled = false;
}
}
@@ -416,19 +425,19 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
actionSelect.style.display = "none";
// Pin the underlying value to "new" so submit takes the Workflow
// Name path.
if (actionSelect.value !== "new") actionSelect.value = "new";
if (actionSelect.value !== ACTION_NEW) actionSelect.value = ACTION_NEW;
}
// Workflow name — visible only when needed.
const action = actionSelect.value;
if (action === "use_existing") {
if (action === ACTION_USE_EXISTING) {
// Name comes from the base; field is irrelevant.
nameLbl.style.display = "none";
nameInput.style.display = "none";
} else {
nameLbl.style.display = "";
nameInput.style.display = "";
if (action === "modify_existing" && baseSelect.value) {
if (action === ACTION_MODIFY_EXISTING && baseSelect.value) {
nameInput.value = baseSelect.value;
nameInput.readOnly = false;
if (refocusName) {
@@ -447,7 +456,7 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
actionSelect.addEventListener("change", () => applyState({ refocusName: true }));
baseSelect.addEventListener("change", () => {
if (actionSelect.value === "modify_existing") {
if (actionSelect.value === ACTION_MODIFY_EXISTING) {
nameInput.value = baseSelect.value;
}
});
@@ -465,23 +474,28 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
// yields a new top-level (single-segment) path once the user types
// a name; existing-dir selections yield the deepest path they
// drilled to before stopping at "(save in <path>)".
const dir = getSelectedPath();
if (!dir || dir.length === 0) {
const dirPath = getSelectedPath();
if (!dirPath || dirPath.length === 0) {
// Top is "__new__" with empty input — prompt for a name.
if (cascadeSelects[0].value === NEW_TOP) newDirInput.focus();
return;
}
const action = actionSelect.value;
let name;
if (action === "use_existing") {
if (action === ACTION_USE_EXISTING) {
name = (baseSelect.value || "").trim();
if (!name) { actionSelect.value = "new"; applyState(); nameInput.focus(); return; }
if (!name) { actionSelect.value = ACTION_NEW; applyState(); nameInput.focus(); return; }
} else {
name = nameInput.value.trim();
if (!name) { nameInput.focus(); return; }
}
overlay.remove();
await onSave({ name, dir });
// `dirPath` is a string[] of segment names (`["UP-scale", "Type-A"]`).
// Renamed from the historical `dir` field on this contract so callers
// match the codebase-wide convention (`dirPath` for arrays of
// segments, `dirName` for single-segment strings, `dir` for resolved
// DirNode objects).
await onSave({ name, dirPath });
};
nameInput.addEventListener("keydown", (e) => { if (e.key === "Enter") submit(); });
@@ -517,10 +531,7 @@ export function showSaveWorkflowModal({ titleSuffix, defaultName, defaultDir, on
export function showTagsModal({ wfName, getCurrentTags, onAddTag, onRemoveTag }) {
const body = document.createElement("div");
const lbl = document.createElement("label");
lbl.className = "koolook-modal-label";
lbl.textContent = "Current tags";
body.appendChild(lbl);
body.appendChild(modalLabel("Current tags"));
const chipsContainer = document.createElement("div");
chipsContainer.className = "koolook-tags-chips";
@@ -572,10 +583,7 @@ export function showTagsModal({ wfName, getCurrentTags, onAddTag, onRemoveTag })
}
}
const addLbl = document.createElement("label");
addLbl.className = "koolook-modal-label";
addLbl.textContent = "Add tag";
body.appendChild(addLbl);
body.appendChild(modalLabel("Add tag"));
const addRow = document.createElement("div");
addRow.className = "koolook-tag-add-row";
+5
View File
@@ -73,6 +73,11 @@ export async function seedDefaultsIfNeeded() {
// page load retries instead of being permanently blocked by the flag.
if (!saveUserPicks(picks)) {
console.warn("[Koolook] seed save failed; will retry on next load");
// Surface the failure in the UI too — the console-only path
// is invisible to anyone who hasn't opened DevTools, and the
// most common cause (full or quota-restricted localStorage)
// is something the user can act on.
toast("Could not seed default favorites — localStorage write rejected. See console.");
return;
}
}
+55 -17
View File
@@ -65,6 +65,18 @@ import {
showTagsModal,
} from "./modals.js";
// =============================================================================
// Built-in section IDs. These are also pathState key prefixes — every
// `pathStates` entry begins with one of these strings followed by `/`. The
// engine handles the prefixing for paths emitted via `ctx.folder({path: …})`,
// but `renderPanel`'s save flow has to construct pin keys directly, so the
// constants are exported here (module-private) instead of duplicated as
// literals.
// =============================================================================
const SECTION_ID_NODES = "nodes";
const SECTION_ID_WORKFLOWS = "workflows";
const SECTION_ID_TAGS = "tags";
// =============================================================================
// Folder expansion state — Map<path, boolean>; truthy = expanded.
// Path keys are auto-prefixed by the engine with the owning section's id, so
@@ -395,6 +407,17 @@ function subcategoryFor(category, categoryRoot) {
if (categoryRoot && category.startsWith(categoryRoot + "/")) {
return category.slice(categoryRoot.length + 1);
}
// Fallback: the node's category neither equals the repo's `categoryRoot`
// nor lives under it. Reachable only via a logic error — typically a
// `REPOS` entry whose `categoryRoot` no longer matches the upstream
// node's actual category prefix (upstream renamed; repo config is
// stale). Surface the misconfig in the console so it's debuggable
// instead of producing silently mis-grouped sidebar entries.
console.warn(
`[Koolook] subcategoryFor fallback fired: category="${category}" did not match ` +
`categoryRoot="${categoryRoot}". The repo config may need updating to track an ` +
`upstream category rename.`
);
const parts = category.split("/");
return parts.length > 1 ? parts.slice(1).join("/") : category;
}
@@ -771,12 +794,19 @@ function buildFolder({ name, count, iconKind, childrenBuilder, onContextMenu, st
initiallyExpanded = startExpanded;
}
// Track the live expanded state in a closure variable instead of
// round-tripping through `wrapper.dataset.expanded`. The DOM dataset
// forces every value to a string, which led to the slightly awkward
// `!== "false"` read; closure-locality keeps the toggle a plain boolean
// and removes one source of truth that wasn't the canonical one
// (`pathStates` is the canonical store; the DOM dataset just mirrored
// it for reads).
let isExpanded = initiallyExpanded;
const wrapper = document.createElement("div");
wrapper.dataset.expanded = initiallyExpanded ? "true" : "false";
const children = document.createElement("div");
children.className = "koolook-children";
if (!initiallyExpanded) children.style.display = "none";
if (!isExpanded) children.style.display = "none";
const { row, chevron } = makeFolderRow({
name,
@@ -786,12 +816,10 @@ function buildFolder({ name, count, iconKind, childrenBuilder, onContextMenu, st
draggablePayload,
dropTarget,
onToggle: () => {
const expanded = wrapper.dataset.expanded !== "false";
const next = !expanded;
wrapper.dataset.expanded = next ? "true" : "false";
chevron.textContent = next ? "▾" : "▸";
children.style.display = next ? "" : "none";
pathStates.set(path, next);
isExpanded = !isExpanded;
chevron.textContent = isExpanded ? "▾" : "▸";
children.style.display = isExpanded ? "" : "none";
pathStates.set(path, isExpanded);
},
});
if (!initiallyExpanded) chevron.textContent = "▸";
@@ -1117,7 +1145,7 @@ function directoryRowContextMenu(event, dirPath) {
// Built-in section descriptors. Order here = render order in the panel.
// =============================================================================
addSection({
id: "nodes",
id: SECTION_ID_NODES,
label: ROOT_GROUP_LABEL,
iconKind: "favorites",
emptyMessage: "No curated nodes yet. Click + above (with a canvas node selected) or right-click a node on the canvas → Add to Kforge Labs.",
@@ -1163,7 +1191,7 @@ addSection({
});
addSection({
id: "workflows",
id: SECTION_ID_WORKFLOWS,
label: WORKFLOWS_GROUP_LABEL,
iconKind: "workflows",
// No emptyMessage — when the workflow store is empty and no search is
@@ -1184,7 +1212,7 @@ addSection({
// canonical location, so renames/moves originating here update the same
// underlying entry the Workflows section shows.
addSection({
id: "tags",
id: SECTION_ID_TAGS,
label: "Tags",
iconKind: "folder",
// No emptyMessage — until any workflow carries a tag, the section stays
@@ -1396,8 +1424,8 @@ export function renderPanel(container) {
// Pin the workflows section + every ancestor of the destination dir
// so the resulting re-render leaves the whole path open. Pins are
// consumed on the next render.
const pinKeys = ["workflows"];
let cur = "workflows";
const pinKeys = [SECTION_ID_WORKFLOWS];
let cur = SECTION_ID_WORKFLOWS;
for (const seg of dirPath) {
cur = `${cur}/${seg}`;
pinKeys.push(cur);
@@ -1435,7 +1463,7 @@ export function renderPanel(container) {
}
showSaveWorkflowModal({
titleSuffix: "entire canvas",
onSave: ({ name, dir }) => saveAndToast(graph, name, dir),
onSave: ({ name, dirPath }) => saveAndToast(graph, name, dirPath),
});
},
}));
@@ -1444,14 +1472,24 @@ export function renderPanel(container) {
iconClass: "pi pi-objects-column",
title: "Save current selection as a workflow",
onClick: () => {
const graph = serializeSelection();
if (!graph) {
const result = serializeSelection();
if (result.kind === "empty") {
toast("Select one or more nodes on the canvas first.");
return;
}
if (result.kind === "stale") {
// Selection set is non-empty but every id in it points at a
// deleted node — typically after an undo or a delete-then-
// change-tab sequence. Generic "select one or more" was
// misleading because the user *had* selected something; the
// canvas just doesn't have it anymore.
toast("Selected node(s) no longer exist. Click a node on the canvas to re-select.");
return;
}
const { graph } = result;
showSaveWorkflowModal({
titleSuffix: `${graph.nodes.length} selected node${graph.nodes.length === 1 ? "" : "s"}`,
onSave: ({ name, dir }) => saveAndToast(graph, name, dir),
onSave: ({ name, dirPath }) => saveAndToast(graph, name, dirPath),
});
},
}));
+7 -1
View File
@@ -134,6 +134,12 @@ async function fetchWorkflowsFromServer() {
}
}
// ComfyUI's /userdata endpoint requires `?overwrite=true` to allow POST over
// an existing file; without it the server 409s on every save after the
// first. The flag is part of the endpoint contract — pinned here as a named
// constant so the contract is visible at a glance.
const USERDATA_OVERWRITE_QUERY = "?overwrite=true";
// Returns "server" | "fallback" | false. Callers that care about durability
// (specifically the seeders) should only treat "server" as a success — a
// "fallback"-only write is per-browser and won't reach future page loads if
@@ -141,7 +147,7 @@ async function fetchWorkflowsFromServer() {
async function persistWorkflowsToServer(store) {
const json = JSON.stringify(store, null, 2);
try {
const resp = await fetch(`/userdata/${WORKFLOWS_USERDATA_PATH}?overwrite=true`, {
const resp = await fetch(`/userdata/${WORKFLOWS_USERDATA_PATH}${USERDATA_OVERWRITE_QUERY}`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: json,