diff --git a/CHANGELOG.md b/CHANGELOG.md index 852c98a..e491e12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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). diff --git a/web/sidebar/canvas_io.js b/web/sidebar/canvas_io.js index a0cb173..f370da9 100644 --- a/web/sidebar/canvas_io.js +++ b/web/sidebar/canvas_io.js @@ -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, + }, }; } diff --git a/web/sidebar/modals.js b/web/sidebar/modals.js index 8849f13..b7413bf 100644 --- a/web/sidebar/modals.js +++ b/web/sidebar/modals.js @@ -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 + // '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 )". - 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"; diff --git a/web/sidebar/picks_store.js b/web/sidebar/picks_store.js index bb14b9e..6008fa6 100644 --- a/web/sidebar/picks_store.js +++ b/web/sidebar/picks_store.js @@ -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; } } diff --git a/web/sidebar/tree.js b/web/sidebar/tree.js index 2f81a39..ec85c9a 100644 --- a/web/sidebar/tree.js +++ b/web/sidebar/tree.js @@ -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; 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), }); }, })); diff --git a/web/sidebar/workflows_store.js b/web/sidebar/workflows_store.js index 7482de1..256e960 100644 --- a/web/sidebar/workflows_store.js +++ b/web/sidebar/workflows_store.js @@ -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,