f98633faedd5ccbc2a98d53dc72622fac61dfd78
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 malkuthroandClaude Opus 4.7
|
4f282e7e79 |
feat(snapshot): durability + recovery UX — atomic writes, local time, in-UI fallback recovery (#105)
* fix(snapshot): atomic preset writes + local-timezone tooltips Hardens the snapshot library against mid-write corruption and stops the auto-saved indicator from rendering Central Time on every machine. `POST /koolook/presets/file` now stages writes to `<file>.json.tmp` and `os.replace`s the final name into place — same atomic-rename pattern `_write_settings` already uses. Covers every preset write path through the same route (named, periodic, pre-load, future recovery snapshots). The `.tmp` suffix is invisible in Load and Recovery dialogs because both list endpoints filter to `.json`. `formatCentralTime` is now `formatLocalTime` with the hardcoded `timeZone: "America/Chicago"` removed — the browser formats with the user's system timezone, and `timeZoneName: "short"` renders the matching abbreviation (PDT / EDT / CET / …). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(snapshot): drop timezone abbreviation from local-time tooltip Follow-up to b0f17b9. `timeZoneName: "short"` rendered the user's local timezone abbreviation (`GMT+2`, `CDT`, `PDT`, …) in the auto-saved hover tooltip, but the abbreviation read as clutter — the user already knows their own timezone. Tooltip now shows just the wall-clock time, e.g. `May 7, 2026, 11:43 PM`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(snapshot): recovery section deep-link + save-time visibility What you actually asked for last round, properly delivered this time. The Load dialog's Recovery auto-saves section was a list of file names with no path context, no save time, and no escape hatch to the file system — so power users could see the autosaves existed but couldn't inspect them in Finder, sort by mtime, or pick a specific recovery point. Three additions close that gap: 1. **`📂 Open` button at the top of the Load dialog** opens the snapshot library folder in the OS file manager. Server route `POST /koolook/presets/reveal` shells out to `open` (macOS), `explorer.exe` (Windows), or `xdg-open` (Linux), with path-traversal grounded at the library base via `_resolve_target`. 2. **`📂 Open` button on each `<preset>_autosave` group header** deep- links into that specific subfolder, so the user lands directly on the autosave files for one preset. 3. **Per-row save time** — recovery rows now show `saved May 7, 2:34 PM` from the file's mtime instead of the `exported <date>` that hid the time-of-day. The bare filename replaces the redundant `Periodic auto-save · <subdir> · <iso>` displayName so the timestamp is visible at narrow modal widths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(snapshot): offline-fallback banner gets Restore + Discard actions The "Offline workflow data found" critical toast used to be a one-way street — Copy details + Dismiss, but nothing ever cleared the localStorage key, so the banner re-fired on every page load forever. Two recovery actions close the loop: - **Restore as snapshot** — wraps the offline workflows into a full snapshot envelope (live picks bundled in so a later Load wouldn't wipe them) and writes it as `recovered-<iso>.json` into the snapshot library via the now-atomic `writePreset`. Clears the localStorage key on success → banner self-extinguishes. The user lands on a durable, in-UI-visible artifact. - **Discard offline copy** — `showConfirmModal` confirm-then-clear. Same end state for the banner; explicit user-acknowledged data loss. Mechanics: - `criticalToast` extended to accept `actions: [{label, onClick, primary?, busyLabel?}]`. Existing callers (`{ copyText }` only) are unchanged. - Toast firing moved out of `loadWorkflowsStore()` and into `koolook_sidebar.js` (entry point) where the action handlers can reach `writePreset` / `loadUserPicks` / `showConfirmModal` without creating circular imports. - `clearOfflineFallback()` exported from `workflows_store.js` for the localStorage `removeItem` step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(snapshot): unify Load dialog header and Recovery group layout Three visual-consistency fixes so the library section and each `<preset>_autosave` group header read like the same UI control instead of two slightly-different ones. 1. **Saved-time on every row.** `loadPreview` now threads `mtime` from the listing endpoint into the preview object, and `formatPreviewMeta` prefers it over the snapshot's self-reported `exportedAt`. Regular snapshot rows now show `saved May 7, 11:53 PM` — same format the recovery rows already used. Falls back to `exported <date>` when mtime isn't available. 2. **Recovery group headers get the full path.** Subdir name on top, `<library>/<subdir>` underneath — the same CSS pair (`folder-name` + `folder-path`) the library section uses. Reuses `renderLibraryLocation` with a new `title` option so the autosave header skips the "Library folder:" prefix while inheriting all styling. 3. **Library Open button matches the group Open button.** Was `koolook-modal-button` (larger), now `koolook-snapshot-row-btn` (smaller, same as group headers). Same flex options on both rows (`flex-start` alignment, `space-between` justification, `gap: 8px`). `formatAutosaveMeta` collapsed back into `formatPreviewMeta` since the formats converged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(snapshot): single disclosure arrow + identical row layouts Two visual fixes after the previous round still left the Recovery section feeling different from the Snapshots section. - Drop the literal `▸` / `▾` characters from the recovery summary — the native `<details>` element already renders a disclosure triangle, so the previous text duplicated it. User saw two arrows side-by-side. - Wrap each group's rows in `.koolook-snapshot-list` so they get the same bordered-box treatment around them that the library snapshot rows have. Strip the outer border + visuals from `.koolook-recovery-list` (now just a scroll container) and `.koolook-recovery-group-header` (now just padding) so the inner classes drive appearance — same `folder-name` + `folder-path` pair + `koolook-snapshot-row-btn` Open button + `koolook-snapshot-list` rows wrapper as the library section. Both surfaces should now read as the same UI control. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert "chore(snapshot): single disclosure arrow + identical row layouts" This reverts commit 82874c3b3ada977c236c03e57479a29126ee68e7. * Reapply "chore(snapshot): single disclosure arrow + identical row layouts" This reverts commit 76e52bdf7cce5c6e84e883f8714921e2f506a9b3. * fix(sidebar): remove backticks from CSS comments inside template literal The comments I added in 82874c3 contained literal backticks (around identifiers like `koolook-snapshot-list`) — fine in CSS, FATAL inside a JS template literal. The CSS string in `ensureStyle()` is a backtick-delimited template, so an inner backtick prematurely closed the template, the rest of constants.js parsed as broken JS, the module failed to load, and every sidebar file that imports from constants.js collapsed with it. Net effect: entire Kforge Labs sidebar disappeared in the standalone app. Why pre-commit `node --check` missed it: that command defaults to script mode for `.js` files and tolerates this edge case. The browser loads our files as ES modules (stricter), and would have flagged the backtick. Verified the fix with `node --check` on a `.mjs` copy of every sidebar file before committing. Comments rewritten without backticks. Added an in-CSS-comment warning calling out the constraint so future changes don't regress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(snapshot): tempfile-based atomic write + onCancel for showConfirmModal Addresses two review findings on PR #105. P1: The previous atomic-write used `<file>.json.tmp` — a fixed, predictable name. An attacker (or accidental user) with write access to a shared library could plant that name as a symlink pointing outside the library, and `Path.write_bytes` would FOLLOW the symlink past the path-traversal guard. Replaced with `tempfile.mkstemp`, which (1) opens with `O_CREAT | O_EXCL` semantics so any existing dirent (including a symlink) at the target name aborts the open, and (2) generates a randomized name so an attacker can't pre-plant anything there. The temp file still lives inside the validated target_dir and `os.replace` still does the atomic swap onto the final name. P2: `showConfirmModal` had no cancel callback. The recovery toast's "Discard offline copy" handler wrapped it in a `new Promise(resolve => …)` that only resolved on confirm, so cancelling left the toast button stuck in `"Discarding…"`. Added `onCancel` parameter; the recovery toast now resolves `{ dismiss: false }` on cancel, re-enabling the button. Also closes the second half of #84 — `dropPlaceholdersForPacks` in canvas_io.js had the same Promise-leak shape and the missing cancel callback was the root cause. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
malkuthro
|
6f1025f66b | fix(sidebar): preserve module links on insert (#102) | ||
|
malkuthroandClaude Opus 4.7
|
8dfb966be0 |
feat(sidebar): snapshot durability — per-preset autosave subfolders, Quick Save, status indicator, sticky persist toasts (#94)
* feat(sidebar): snapshot durability — pre-load + periodic auto-saves, sticky persist toasts, Save-dialog library path
Preventive measures after a state-loss incident: a Load erased work, with no
visible warning and no recovery path. Four changes:
1. **Save dialog library path.** Both the input modal ("Save as new…") and
the three-way overwrite dialog now show a tail-truncated breadcrumb
("…/user/koolook-presets2/") with the full path in the tooltip — the user
sees where the save will land before clicking. New
`formatLibraryPathBreadcrumb` helper + `.koolook-modal-pathline` CSS;
threaded through `getLibraryInfo` (already wired for Load).
2. **Pre-load auto-save.** Every destructive Snapshot Load writes
`_autosave_pre_load_<iso>.json` to the library BEFORE `applySnapshot`. On
auto-save failure the Load is aborted entirely with a sticky critical
toast — the whole premise is "Load never destroys state without a
recovery point." Pre-load autosaves don't rotate; every Load is a
permanent recovery file.
3. **Periodic auto-save.** Every 5 minutes a defensive snapshot is written
to `_autosave_periodic_<iso>.json` IF state changed (string-equality
fingerprint on picks+workflows, exportedAt stripped). Skipped when tab
is hidden. Rotates: keeps last 5. First tick at +30s so load/seed
settle first. Started from sidebar setup() after registration.
4. **Critical (sticky) toasts.** New `criticalToast(msg, {copyText})`
variant — no auto-dismiss; optional "Copy details" button hands the
user unsaved JSON for manual recovery. Wired into:
* `commit()` fallback-only path — warns "saved to browser-local
fallback only" once per outage, re-arms when /userdata recovers
* `persistMutation` full-failure path — replaces auto-dismiss toast,
includes the unsaved JSON via Copy
* `loadWorkflowsStore` corrupt-file path — sticky so "your data
looks lost" can't be missed
* `loadWorkflowsStore` stale-fallback-detected path — was a 4.5s
toast, now sticky with Copy of the fallback JSON; turns evening-
work-in-localStorage recovery into an explicit user action
No API or storage changes outside the snapshot library. Existing user
snapshots untouched. New `_autosave_*` files are valid snapshots and
appear in the Load list (sorted to the top via leading underscore) —
deletable via the regular row × button.
Files: 6 changed, +433 / -10.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sidebar): snapshot status indicator — current preset + saved/auto-saved/unsaved dot
Replaces the static "Snapshot" label in the action row with a dynamic
status display:
● <preset name> · <state>
Dot color encodes status:
• green — saved (live state matches the last named save)
• blue — auto-saved (matches the latest periodic auto-save only)
• orange — unsaved (diverged from any save / auto-save)
• grey — none (no preset tracked)
Tooltip carries the long-form explanation including the latest auto-save
ISO timestamp.
State machine:
• markStateSaved() baselines the fingerprint after a successful named
save (writeAndToast) or successful Load (applySnapshot).
• Periodic auto-save updates _lastPeriodicFingerprint + _lastPeriodicAt
AFTER a successful tick — flips "unsaved" to "auto-saved" without a
mutation event.
• setCurrentPresetName fires SNAPSHOT_STATUS_CHANGED_EVENT so the
indicator name refreshes when Load/Save updates the tracker.
Persistence:
• Saved fingerprint is mirrored to localStorage
("koolook.snapshot.savedFingerprint.v1") so the saved/unsaved
distinction survives page reloads. Without this, every reload would
show "saved" until the user mutated something — defeating the point.
• At session start, if the tracker has a name AND no persisted
fingerprint exists, we baseline once (the "first-ever session" edge).
Imperfect for "user closed mid-edit" but the alternative (always
show unsaved on first load) is worse UX.
Fingerprint = JSON.stringify({picks, workflows}). Same shape used by
periodic auto-save's change detection — single source of truth, no
risk of "saved" and "auto-saved" disagreeing about what the state is.
Files: 5 changed, +237 / -21.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sidebar): per-preset autosave subfolders + Quick Save button
Reorganizes auto-save storage to keep the user's library tidy and add a
fast-path save action.
NEW LAYOUT:
<library>/
Wan video kit.json ← user-named save (Load list)
Wan video kit_autosave/ ← shadow folder (HIDDEN from Load)
periodic.json ← single rolling file, overwritten
pre_load_<iso>.json × ≤5 ← rotates, permanent per Load
another preset.json
_unsaved_autosave/ ← used when no preset is tracked
periodic.json
pre_load_<iso>.json × ≤5
WHY:
The previous layout dropped every auto-save as a flat `_autosave_*.json`
file at the library root. Two problems:
1. Cluttered the user's Load dialog (sorted to top via leading
underscore but still visible).
2. Filesystem view (Total Commander, Finder) showed dozens of these
files alongside the user's named saves — visually noisy.
The per-preset subfolder layout makes the on-disk structure self-
documenting: "the shadow recovery for X lives at X_autosave/". Hidden
from the Load list at the server (filters subdirs + legacy `_autosave_*`
flat files), so the regular Load flow only enumerates user-named presets.
WHY one rolling `periodic.json` (not timestamped):
The latest periodic auto-save IS the most useful periodic state. Earlier
periodic snapshots are worse copies of approximately the same work.
Trades disk diversity for cleanliness — pre-load files cover the
"recovery from a deliberate destructive action" case independently.
WHY timestamped pre-load files (rotated, keep 5):
Each Load is a deliberate destructive action and deserves a permanent
recovery point. Rotation bounds disk; rare "5 Loads back" recovery loss
is the trade-off for not letting the folder grow unbounded.
QUICK SAVE BUTTON:
New `pi pi-save` icon button between Load and Save. When the user has a
preset loaded/saved and wants to save again on top of the same name,
they don't need the dialog with confirmation. One click, no prompt,
toast confirms ("Quick-saved 'X'."). Disabled (greyed out) when no
preset is tracked — the regular Save button remains the entry point
for naming a new preset.
Refreshes its enabled state on `SNAPSHOT_STATUS_CHANGED_EVENT` so that
loading a preset, saving as new, or clearing the tracker via partial-
load failures all flip the button state correctly.
SERVER (koolook_routes.py):
• New `_DIRNAME_RE` validator (same charset as filenames, no extension)
• New `_resolve_target(lib_base, subdir, name)` — symlink-escape check
grounded at LIBRARY base (not subdir base) so a hostile symlink
inside an auto-save subfolder can't redirect writes outside.
• GET / POST / DELETE / HEAD `/koolook/presets/file` accept optional
`?dir=<subdir>` query param. Server auto-creates the subdir on POST.
• GET `/koolook/presets/list` accepts optional `?dir=` for autosave-
subfolder enumeration (used by the JS pruner). Without `dir`, hides
subdirs AND legacy `_autosave_*` flat files at root so the Load
dialog only sees user-named presets.
JS (snapshot.js):
• `writePreset` / `readPreset` / `deletePreset` / `presetExists` /
`listPresets` accept `{dir}` option, propagated to fileQuery.
• `_autosaveSubdir()` resolves `<presetname>_autosave` or
`_unsaved_autosave` from the current preset tracker.
• `writePreLoadAutosave` writes to subfolder, returns
`<subdir>/<filename>` token; new `_prunePreLoadAutosaves`.
• Periodic timer overwrites `<subdir>/periodic.json` (single file)
instead of rotating timestamped files.
• Removed the global flat-file `_autosave_*` constants.
JS (tree.js):
• New Quick Save button between Load and Save in the snapshot row.
• `refreshQuickSaveDisabled` toggles the button on every status event.
BACKWARDS COMPATIBILITY:
Existing users with `_autosave_*` files at the library root from the
previous layout will see them stop being written to (the new code
writes to subfolders only). The server-side list filter hides them
from the Load dialog so they don't visually pollute, but the files
themselves remain on disk for manual recovery / cleanup.
Files: 3 changed, +277 / -96.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sidebar): address review — recovery autosave UI + subtitle on overwrite-confirm
Two review findings on PR #94:
[P2] Pre-load autosaves were written into per-preset subfolders but
the Load dialog only listed root-level user-named presets, so
recovery was unreachable from the UI.
[P3] The Save-as-new flow's existing-name → "Overwrite?" confirm
dialog dropped the library breadcrumb (showConfirmModal lacked
subtitle support, even though showInputModal and
showThreeWayDialog had it).
P2 — RECOVERY AUTO-SAVES IN THE LOAD DIALOG:
Added a new server endpoint `/koolook/presets/autosaves/list` that walks
every `*_autosave/` subdir (including `_unsaved_autosave/`) and returns
a flat list of recovery files with their subdir, name, mtime, and size.
Why a dedicated endpoint? The user-facing list MUST hide subdirs to keep
the Load list clean; the recovery path is the explicit "show me autosave
folders" API, separated cleanly so its boundary is grep-able.
JS gets a matching `listAutosaves()` helper that fans out preview reads
to extract displayName + counts (N+1 cost is real but bounded — typical
user has ~10-30 autosave files, paid only when the recovery section is
expanded).
The Load dialog gains a collapsible `<details>` section ("▸ Recovery
auto-saves") below the regular preset list. Lazy-loaded on first expand.
Items are grouped by subdir so users can scan visually for the preset
they want to recover from. Each row shows a kind badge (PRE-LOAD orange
/ PERIODIC blue / OTHER), the snapshot's display name, and the standard
preview meta (pick + workflow counts + exportedAt).
Click → confirm modal → optional pre-recovery autosave (recursive
protection — restoring an autosave is itself a destructive Load) →
applySnapshot. Post-restore, the tracker is set to the original named
preset derived from the subdir (`Foo_autosave` → "Foo"), or null for
`_unsaved_autosave`. State indicator flips to "saved" via markStateSaved.
P3 — SUBTITLE ON CONFIRM MODAL:
`showConfirmModal` gains an optional `subtitle` parameter (string or
HTMLElement), same contract as showInputModal and showThreeWayDialog.
The Save-as-new flow's overwrite-confirm now passes `buildPathSubtitle()`
so the user keeps library context across the input → exists-check →
confirm chain. Important when Settings → libraryPath has been changed
mid-session, or facility users juggle multiple shared libraries.
Files: 5 changed, +355 / -1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
malkuthroandClaude Opus 4.7
|
d763b36adc |
chore(legal): add NOTICE + SPDX headers across web/ (#93)
Codify attribution before the sidebar gets more visibility. NOTICE is the single human-readable source of truth — copyright statement, plain-English attribution request with a copy-pasteable credit line, pointer to LICENSE and forks/THIRD_PARTY.md so it shows the project respects others' attribution too. SPDX-FileCopyrightText / SPDX-License-Identifier headers added at the top of every web/*.js (entry point + sidebar/ + ai_pipeline) so license scanners (GitHub, FOSSA, reuse-tool) parse each file correctly and stripped-attribution forks become an obviously deliberate act rather than "I forgot to copy a file." GPL-3.0-only matches the LICENSE file (no "or later" wording in it). Year range 2025-2026 spans first commit (2025-09-28) to today. Comment-only change — no runtime impact. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
malkuthroandClaude Opus 4.7
|
93363cc625 |
feat(sidebar): snapshot library v2 — configurable filesystem path via KFORGELABS_PRESETS (#68)
* feat(sidebar): snapshot library — named presets for picks + workflows Implements #46 item 1 (custom save location + cross-machine portability) as a preset library: each preset is one named JSON file under /userdata/koolook-presets/, capturing the full Kforge Labs state. Use cases addressed ------------------- - **Personal portability:** copy `koolook-presets/` between machines, or sync ComfyUI's userdata folder via Dropbox/iCloud/Drive — your kit follows. - **Team / facility shared library:** multiple workstations on the same ComfyUI server share `/userdata/koolook-presets/` natively. One person publishes "facility kit", everyone else loads it. - **One-off transfer (different servers):** Manage tab's Download button saves a snapshot to local disk; Upload imports it into another machine's library. UI -- New top-level **Snapshot** action row above the search field, mirroring the Nodes/Workflows action-row pattern. Three icon buttons (Load / Save / Manage), each opens the same modal at a different starting tab. - Load tab: lists every preset with metadata (workflow count · pick count · export date). Click a row → "Replace current state?" confirm → restores both picks and workflows. - Save tab: input field defaults to `preset YYYY-MM-DD`, fully editable. Existing-name save prompts to overwrite. - Manage tab: per-row Download / Delete buttons + a top-of-tab Upload action (file picker imports a JSON snapshot from disk into the library). Schema ------ ```json { "kind": "koolook-snapshot", "version": 1, "name": "Wan video kit", "exportedAt": "2026-05-05T08:42:13.000Z", "picks": [...], "workflows": { "directories": {...} } } ``` `kind` + `version` for future schema migrations. Anything without `kind === "koolook-snapshot"` is rejected on import. Module shape ------------ - `web/sidebar/snapshot.js` (new) — server I/O on the preset directory, schema validation, gather/apply, download/upload helpers. UI-free. - `web/sidebar/modals.js` — new `showSnapshotModal({initialTab, ...})` that takes capability functions instead of importing snapshot.js directly, so the UI module stays UI-only. - `web/sidebar/workflows_store.js` — new `replaceAllWorkflows(rawStore)` for bulk-replace (bypasses persistMutation since the entire cache is being swapped) + `getAllWorkflowsForExport()` for the gather flow. - `web/sidebar/picks_store.js` — new `setAllPicks(picks)` exported helper. - `web/sidebar/tree.js` — new top action row + 3 toolbar buttons + one wiring helper. - `web/sidebar/constants.js` — CSS for tab strip, list rows, manage buttons. Deliberate scope cuts (per the design discussion) ------------------------------------------------- - No continuous sync. Each save is a deliberate act. - No conflict resolution. Same preset name written from two machines on the same server = last-write-wins. Documented as a known limit; revisit when it bites. - No selective import (preset = full state). Merge can be a v2. - No custom userdata path config. The `koolook-presets/` location is fixed; portability comes from the snapshot files themselves, not from rerouting storage. Verification ------------ - `node --input-type=module --check` on all modified + new JS files — clean. - `python tools/preflight_release.py --skip manager-meta -v` PASS. - Synced to live install on Mac for browser smoke-test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(sidebar): snapshot library now uses configurable filesystem path Reworks PR #67 (which served snapshots from /userdata/koolook-presets/) to expose the library through Koolook's own server-side endpoints, making the storage location configurable via an env var. The userdata namespace is per-install — fine for personal use, useless for the "facility shared library" use case the user explicitly called out. Architectural change -------------------- - The client no longer talks to ComfyUI's `/userdata/*` API for snapshot files. New JS-side endpoints: `/koolook/presets/{info,list,file}`. - Server side: new `koolook_routes.py` registers aiohttp routes on ComfyUI's PromptServer at custom-node load time. Path-traversal protection at the route boundary via a strict filename whitelist regex (`^[A-Za-z0-9 _.()\-]+\.json$`). - Storage location is read from the `KFORGELABS_PRESETS` env var on every request (so re-launching ComfyUI with a new value picks it up without any cache fuss). Default fallback: `<comfyui-userdata>/koolook-presets/`. - Auto-creates the configured directory on first save (parent must already exist — for NFS/SMB mounts the mount must be present). Use cases this enables vs. PR #67 --------------------------------- | Use case | PR #67 | This PR | |-------------------------|--------|---------| | Personal, single-machine | ✓ | ✓ | | Personal cross-machine | symlink-or-sync userdata | point env var at Dropbox folder | | NFS/SMB facility share | ✗ (locked to userdata) | ✓ point env var at mount | | Multiple parallel libs | ✗ | per-instance env var | | Read-only distribution | ✗ | ✓ (server detects, save fails cleanly) | UI difference vs. PR #67 ------------------------ - The Manage tab gains a small `i` icon at the right edge of the upload row. Hover tooltip shows the current library path, source (default vs `KFORGELABS_PRESETS` override), and writability status. No header line, no extra UI weight. Files added/changed ------------------- - `koolook_routes.py` (new, ~180 lines) — aiohttp routes + env-var config + filename validation. Loaded by `__init__.py` at startup; failure is non-fatal (the rest of Koolook still works, just the snapshot feature is unavailable). - `__init__.py` — registers `koolook_routes` import-time side-effect. - `scripts/sync_to_dev.py` — `koolook_routes.py` added to `RUNTIME_PATHS` so dev-sync includes it. - `web/sidebar/snapshot.js` — server I/O section rewritten to call `/koolook/presets/*` instead of `/userdata/koolook-presets/*`. Drops the URL-encoding workaround (we own the route now, so the `name=<file>` query param is clean and the server validates). - `web/sidebar/modals.js` — `showSnapshotModal` accepts a `getLibraryInfo` capability function; the Manage tab calls it and populates the info-icon tooltip. - `web/sidebar/tree.js` — passes `getLibraryInfo` from `snapshot.js` to the modal. - `.env.example` — new `KFORGELABS_PRESETS` block with macOS / Linux / Windows examples for personal-sync, NFS facility share, and SMB facility share patterns. - `CHANGELOG.md` — Added entry rewritten for the env-var design; Internal entry on the new server-side module. Why a separate v2 PR (instead of patching #67) ---------------------------------------------- The userdata-namespace design in #67 was the wrong architecture for what the user actually wanted. Patching it to the env-var design would have meant a near-total rewrite of #67's server I/O paths mid-review. Cleaner: branch fresh from main, cherry-pick #67's UI + stores work as a base, layer the new endpoint design on top. PR #67 will be closed as superseded once this lands. Verification ------------ - `node --input-type=module --check` on every modified JS file — clean. - `python -c "import ast; ast.parse(...)"` on `koolook_routes.py` + `__init__.py` — clean. - `python tools/preflight_release.py --skip manager-meta -v` PASS. - Synced to live install on Mac. User to smoke-test against a real ComfyUI session (the previous URL-encoding 405 should be gone since the new route handlers own the URL space). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(sidebar): redesign snapshot UX — single-click Save, real Settings panel User feedback on #68's first cut: tabbed modal is too complicated, can't see existing presets in the Save flow, the env var alone isn't a real settings UI, the default-path text was stale (still claimed `/userdata/koolook-presets/` after the configurable-path landed). UX changes ---------- - **Save button → one click.** If a preset is currently loaded (tracked in localStorage as `koolook.snapshot.currentPresetName.v1`, survives reloads), prompts "Save over '<name>'?" with three options: Save (overwrite) / Save as new… (rename) / Cancel. If nothing is loaded yet, prompts for a name with `preset YYYY-MM-DD` default, fully editable. - **Load → single dialog.** Preset list with per-row × Delete button. Header line shows the current library path so the user can see where they're loading from. No tabs. - **Cog → real Settings panel.** Single field for the library path (text input, absolute path). Save / Reset to default / Close. Read- only line shows the currently-resolved path + source label (settings panel / env var / built-in default). The "Manage" tab and its Download / Upload sub-actions are gone — the user can already reach the file on disk after a save, no in-app file ops needed. Server side ----------- - New `GET /koolook/presets/settings` returns the saved library path + resolved path + source. - New `POST /koolook/presets/settings` writes the saved library path to `<comfyui-userdata>/koolook-settings.json`. Empty value clears the override. - Resolution chain in `_configured_dir()`: settings file > KFORGELABS_PRESETS env var > built-in default Reads each on every call; no module-load caching. - `info` endpoint gains a `source` field ("settings" | "env" | "default") the UI uses for the readout line. Client side ----------- - `showSnapshotModal` (tabbed) replaced by three exports: showSaveSnapshotDialog, showLoadSnapshotDialog, showSnapshotSettingsDialog. Each is self-contained, no shared state. - `snapshot.js` adds `getSettings`/`saveSettings` for the Settings dialog and `getCurrentPresetName`/`setCurrentPresetName` for the one-click Save tracking. Settings ride on the new server endpoints; current-preset tracking is browser-local. - `applySnapshot` now also updates the current-preset tracker (via the dialog wrapper, not the function itself) on successful load. - `tree.js` toolbar wires three direct callbacks instead of one `openSnapshotModal(initialTab)` helper. Stale text fix -------------- - The Save dialog's hint text in #68's first cut hardcoded "/userdata/koolook-presets/<name>.json" — stale from the userdata- era PR #67. The redesigned Save flow doesn't show that text at all (the path is no longer relevant to the user-typed name); the Load dialog's header line and the Settings dialog's read-only field are the canonical surfaces for "where my presets actually live." Deliberately out of scope ------------------------- - No per-save path picker. The library is one location at a time, set in advance via Settings or env var. Saving to ad-hoc locations would fragment the library and break the "facility shared library" use case. - No in-app Download / Upload (was in #68's first cut). Files are on disk at the configured path; the OS file manager handles transfer. Verification ------------ - `node --input-type=module --check` on every modified JS file — clean. - `python -c "import ast; ast.parse(...)"` on `koolook_routes.py` — clean. - `python tools/preflight_release.py --skip manager-meta -v` PASS. - Synced to live install on Mac. User to smoke-test against a real ComfyUI session. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sidebar): address PR #68 review — atomicity, security, error UX Multi-agent review surfaced 2 BLOCKING + 5 HIGH + 4 MEDIUM + 3 LOW. Validated the BLOCKINGs against the actual code (both real) and applied the bundle of fixes that hold up. Modal-size HIGH (#7 file size) deferred to a follow-up — that's a wider refactor. BLOCKING fixes -------------- - **`applySnapshot` partial-failure tracker gating** (modals.js Load dialog). `setCurrentPresetName(...)` was called unconditionally before checking `{picksOk, workflowsOk}` from the apply. Partial apply (one of picks/workflows persisted, the other didn't) left the tracker pointing at the preset, so the next Save would offer "Save over '<name>'?" and silently overwrite the on-disk preset with the corrupted half-state. Now: tracker is set only on `picksOk && workflowsOk`; partial failure clears it (and the toast tells the user "Reload to recover prior state. Tracker cleared."). - **`replaceAllWorkflows` non-atomic rebind** (workflows_store.js). Rebinds `workflowsCache` before awaiting persist, with no rollback. Now: snapshots the cache via the existing `snapshotCache()` primitive that `persistMutation` uses; on persist failure, restore the prior cache and return false. Load is fully atomic. HIGH fixes ---------- - **Workflows-store invariant block updated.** Lists `replaceAllWorkflows` as the fourth legitimate rebind site (with rollback semantics). "Fifth rebind" warning bumped to "sixth." - **Explicit HEAD route on `/koolook/presets/file`** that just does `is_file()` + status, no body work. The auto-synthesis path was reading the full file from disk just to discard it — wasteful on multi-MB snapshots over Dropbox/iCloud/NFS. - **Client `sanitizeName` mirrors server whitelist regex** (`^[A-Za-z0-9 _.()\-]+$`). Characters outside the whitelist (`+`, `&`, `'`, accented letters, emoji, etc.) collapse to `_` rather than triggering an opaque HTTP 400 at save time. - **Error-reason surfacing via `await resp.text()`.** RFC 7540 strips HTTP/2 reason phrases; behind any HTTP/2-terminating proxy `resp.statusText` is empty. Reading the body always carries aiohttp's `reason=` text. Used by writePreset, deletePreset, saveSettings, readPreset. MEDIUM fixes ------------ - **Symlink follow-through hardening.** `_resolve_within_base` helper computes `(base / name).resolve()` and asserts it's still under `base.resolve()`. Defends against a symlink planted in a facility- shared library dir redirecting reads/writes off-share. - **Atomic settings write.** `_write_settings` writes to `<tmp>` then `os.replace`, so an interrupted process can't truncate the file and silently revert the user's saved library path on next load. - **`__init__.py` import wrapped in try/except.** The "non-fatal" comment promised that an import failure wouldn't break node registration; previously only `install()` returning False was guarded. Now any module-load exception is caught and logged with the `[Koolook]` prefix, and the rest of the plugin loads. LOW housekeeping ---------------- - Dropped dead exports `downloadSnapshotAsFile` + `readSnapshotFile` (left over from an earlier iteration that had Download/Upload UI). - Dropped 5 dead CSS rules (`.koolook-snapshot-tabs*`, `.koolook-snapshot-tab-pane`) from when the modal was tabbed. - `currentPresetName` cleared when the user deletes the currently- loaded preset (Load dialog's promptDelete now checks via the new `getCurrentPresetName` capability). - `loadPreview` warns to console on `kind` mismatch — non-snapshot JSON files in the library directory are now visible in DevTools rather than silently filtered. - `presetExists` is tri-state (`true` / `false` / `null`) — the Save flow refuses to write when the library is unreachable rather than silently skipping the overwrite-confirm prompt. Deferred -------- - modals.js size (1080 lines) — extracting the snapshot dialogs into their own file is a follow-up refactor. Not blocking merge. - Doc drift on maintainer docs / README env-var convention — separate documentation sweep. Verification ------------ - `python -c "import ast; ast.parse(...)"` on all .py files — clean. - `node --input-type=module --check` on every modified JS file — clean. - `python tools/preflight_release.py --skip manager-meta -v` PASS. - `grep -rn "downloadSnapshotAsFile\\|readSnapshotFile\\|koolook-snapshot-tab" web/` — zero hits (dead code gone). - Synced to live install for browser smoke-test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: nudge — re-trigger CI after webhook miss on d6d2bf8 The CI workflow didn't fire on the prior push (Socket Security ran but GitHub Actions Lint/Smoke/Pre-flight/Security Gate didn't). Empty commit to refire the pull_request webhook. * fix(routes): drop unnecessary f-string prefixes flagged by ruff (F541) Two adjacent string literals in the symlink-escape error message had an `f` prefix despite containing no placeholders. CI's ruff check fails on F541 — strip the prefixes so the lint gate passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
malkuthroandClaude Opus 4.7
|
2f370a962c |
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> |
||
|
malkuthroandClaude Opus 4.7
|
bebc6f618d |
feat(sidebar): right-click menu pack — Duplicate, Tags, + New directory move (#59)
* feat(sidebar): right-click menu pack — Duplicate, Tags, + New directory move Three sibling sidebar slices bundled because they all extend the same workflow-row right-click menu in `web/sidebar/tree.js` and share a small schema/store surface in `workflows_store.js`. Refactor cleanup is held back for a separate PR (issue #47) so this diff stays a "no behavior change to existing flows" feature add. Changes ------- - Right-click "Duplicate…" → name modal with `<name> (copy)` default; deep-clones the source graph into a new entry in the same directory via `saveWorkflowEntry`. The duplicate inherits the source's tags so the user's categorization carries over. Same-name duplicates fall through to the existing archive-on-collision behavior. Closes #58. - Right-click "Tags…" → chip-row modal with add/remove. Each edit fires its own `persistMutation` so changes survive a mid-edit close. New `showTagsModal` in `modals.js` mirrors the existing `showInputModal` / `showConfirmModal` shape (escape-key teardown, overlay-click dismissal centralized in `makeModalShell`). - New Tags sidebar section between Workflows. Flat list of tag folders; each tag holds the workflows carrying it (sorted A→Z). Click loads the workflow from its real path. Archived workflows are filtered out of the section so the active view stays clean — their tags persist on the entry, so a restore brings them back. Search matches tag name OR workflow name. - Right-click "+ New directory…" / "+ New subdirectory under <path>…" in the Move-to flow. Both create a fresh directory and move the workflow into it as one atomic mutation; if the move can't land, the new directory is rolled back so the cache never leaks an empty orphan. Closes #57. Schema additions (back-compatible) ---------------------------------- - Workflow entries gain optional `tags: string[]`. `normalizeDirNode` trims, drops empties, and dedupes case-sensitively, so old entries without a tags field load as `tags: []` and the rest of the code can assume the field always exists. Pre-v0.3 stores load fine. - New exports in `workflows_store.js`: `getWorkflowTags`, `addTag`, `removeTag`. `getWorkflowGraph` is now also exported so the duplicate flow can deep-clone without reaching into the cache directly. Why one PR for three issues --------------------------- All three slices touch `workflowRowContextMenu` in `tree.js` and the mutator API in `workflows_store.js`. A coherent UX-feature scope ("next batch of workflow menu enhancements") reviews more cheaply as one diff than three near-identical PRs. Issue #47 (refactor / tidiness pass) stays separate because mixing refactor and feature in the same diff defeats the point of the split — that work is queued for a follow-up PR. Testing ------- - `python tools/preflight_release.py --skip manager-meta -v` PASS (static / dispatch / workflows fixtures, all 3 stages). - `node --input-type=module --check` passes on each modified JS file. - Browser smoke test: pending — needs a ComfyUI session for the user to verify the three menu actions and the Tags section against a real workflow store. The CI gates are Python-only and don't catch JS regressions in the sidebar by themselves. Related ------- - Closes #57 (move-to: + New directory…) - Closes #58 (Duplicate) - Implements the right-click + section parts of #56 (Tags); the issue body's `addTag` / `removeTag` / `listAllTags()` ops landed under slightly different names (`addTag`, `removeTag`, `gatherTags`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sidebar): address review findings on PR #59 Self-review pass against the validated agent findings. Five fixes total across the three issues' surface, holding refactor/file-size concerns for follow-up #47 widening. Fixes ----- - **HIGH** `normalizeDirNode` now rejects non-string `tags` elements outright instead of `String(raw).trim()` coercing them. `null` / `undefined` / `true` / `123` no longer silently surface as the literal tag strings `"null"` / `"undefined"` / etc. — `stats.dropped` increments instead so the existing console warning catches malformed migrations. Closes review HIGH-1. - **HIGH** Tags modal now distinguishes "no tags yet" from "workflow no longer exists." The `getCurrentTags` adapter no longer collapses `null` to `[]`, and `renderChips` renders an explicit gone-state with disabled add input + descriptive placeholder. Prevents users typing into a doomed modal when a concurrent action (another tab, drag-and-drop) deletes the workflow under them. Closes review HIGH-3. - **MEDIUM** Duplicate flow re-validates source existence at submit-time inside the `mutate` callback. Without it, a concurrent delete of the source between menu-open and submit would let `saveWorkflowEntry`'s `ensureDirectoryAtPath` silently re-create the parent directory and paste a clone of the captured graph — effectively undoing someone else's delete. Now returns `false` on missing source, surfacing "Could not duplicate — <name> no longer exists." Closes review MEDIUM-6. - **MEDIUM** Duplicate flow now routes inherited tags through the public `addTag` mutator instead of writing `dir.workflows[newName].tags` directly. Keeps the tree.js UI layer outside the store's internal shape. The whole sequence still rides on one `persistMutation` snapshot — commit failure rolls back save + tag adds together. Closes review MEDIUM-5. - **LOW** `gatherTags` comment block clarifies that grouping is case-sensitive but search filter is case-insensitive substring. The comment previously said "Comparison is case-sensitive" which was technically true for grouping but misleading on its own. Skipped with rationale ---------------------- - **HIGH-2 modal cache desync on persist failure** — agent analysis was wrong on trace. `renderChips` reads from cache, X-click does not optimistically remove the chip from DOM, and `restore()` reverts cache to pre-mutation. DOM and cache stay consistent across every commit-failure path. Cross-tab updates aren't reflected, but that's a project-wide cache-coherency concern, not specific to this PR. - **HIGH-4 tree.js size** — real architectural concern but follow-up only. Track on #47 widening (or a sibling extraction issue). Adding extraction in this PR would mix refactor with feature. - **MEDIUM-7 tag API asymmetry** — agent claimed `addTag`'s parameter reassignment differed from neighbors, but verified the existing convention IS parameter reassignment (`addDirectory`, `renameDirectory`, `renameWorkflow`, `moveDirectory` all do `name = (name || "").trim();`). The defensive-copy difference between `getWorkflowTags` and `getWorkflowGraph` is intentional. - **MEDIUM-8 top-level "Archive" name** — pre-existing semantic shared by toolbar Create-directory and save-modal "+ New directory." Restricting only the right-click new-dir entry would be inconsistent. Filing as a separate cross-cutting issue is the cleaner path. - **LOW magic strings / modals.js size** — both queued for #47. Validation ---------- - `python tools/preflight_release.py --skip manager-meta -v` PASS. - `node --input-type=module --check` on all three modified JS files — clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
malkuthroandClaude Opus 4.7
|
8c1e6e3d78 |
chore(sidebar): tidiness pass — toolbar factory, dropped dead code, store invariants (#60)
* chore(sidebar): tidiness pass — toolbar factory, dropped dead code, store invariants Refactor pass against four pre-existing items on #47. No behavior change; the diff is "extract helper + drop dead code + add comments" only. Held back from PR #59 so feature and refactor stay isolated for review. Changes ------- - **`makeToolbarButton({iconClass, title, onClick})`** factory 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` + class/innerHTML/title/listener wiring; call sites collapse to 4-line factory invocations. - **Dropped duplicate `.koolook-export-btn` CSS rule.** It was byte-for-byte identical to `.koolook-icon-btn` (minus the latter's `:disabled` styling, which the export button never hit). The export button now uses only `.koolook-icon-btn` via `makeToolbarButton`. - **`buildFolder({path})` default removed.** Previously `path = null` with an `if (path && …)` runtime guard. Verified all callers (both inside `renderTree` and inside `makeSectionCtx`) always supply a non-empty string — the default was never reachable. Param is now required; runtime guards are dropped. - **`workflowsCache` mutator invariants documented inline.** Four rules a future contributor needs before adding a new mutator: - Pair every mutate with a commit (or use `persistMutation`) — a mutator that "succeeds" without a paired commit silently drops the change on the next reload. We've already shipped + reverted that class of bug once; the comment names it explicitly. - Return `false` for no-op (collisions, missing source, validation failures), truthy for success. `persistMutation` reads this. - Mutate in place; don't return new structures. - Never replace `workflowsCache` itself except in seed/load paths. Closes ------ Four checkboxes from #47 (architectural + code-tidiness): - "Decompose `renderPanel` + extract `makeToolbarButton`" - "Drop duplicate `.koolook-export-btn` CSS class" - "`buildFolder` `path = null` default (unreachable) — drop" - "Document `workflowsCache` invariants" Remaining #47 items not in this PR (queued for a future widening): - DOM helpers `el(tag, props, children)` + `modalLabel(text)` — wider refactor that touches modal/folder/leaf row construction. - Magic-string lift (action sentinels in save modal, `?overwrite=true`, `pathStates` mini-DSL). - `dir` / `dirName` / `directory` terminology pass. - `dataset.expanded` boolean storage instead of string compare. - UX items (`subcategoryFor` fallback, seed-failure toast, selection serializer stale-id case). Validation ---------- - `python tools/preflight_release.py --skip manager-meta -v` PASS. - `node --input-type=module --check` on each modified JS file — clean. - Pure refactor: `git diff` is mechanical extraction + dead-code drop + comment additions. No semantic changes to existing flows; no new user-visible state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sidebar): address PR #60 review findings Multi-agent review surfaced 2 MEDIUM + 3 LOW. Fixed 4 of the 5; skipped the JSDoc-on-makeToolbarButton LOW because sibling factories (makeFolderRow, makeNodeLeafRow, makeWorkflowLeafRow) also have no JSDoc — adding it just on the new factory would be inconsistent local style. Fixes ----- - **MEDIUM** `makeToolbarButton` no longer uses `innerHTML` template interpolation. Icon span is built with `createElement` + `className` + `appendChild` instead. All current callers pass static literals, so this isn't a fix for an active vulnerability — it's helper-boundary hardening so a future caller passing dynamic data (workflow metadata, registry entry, etc.) can't inject markup. Also removes the file's only `innerHTML`-with-interpolation outside the empty-string clear at L157, matching the rest of the file's DOM-API style. - **MEDIUM** Mutator-invariant rule #4 in workflows_store.js corrected to list all three legitimate `workflowsCache` rebind sites: module init, load/seed paths, AND `snapshotCache().restore()` during rollback. Previous wording denied that rollback rebinds, which would have led a future contributor reading the doc to either "fix" snapshotCache (breaking rollback) or distrust the entire invariant block. - **LOW** `buildFolder({path})` now throws on `typeof path !== "string" || !path` at function entry. Converts a silent state-corruption regression ("future caller forgets to pass path → `pathStates.set(undefined, …)` pollutes one shared key across all unkeyed folders") into a loud boundary failure. Promotes the PR description's "unreachable default" claim into a self-enforcing assertion. - **LOW** `makeToolbarButton` doc comment tightened from "every toolbar button" to "the four icon-only toolbar buttons (export, new-dir, save-canvas, save-selection)" with explicit acknowledgment that the text-content `addBtn` (Add-to-favorites "+") stays hand-rolled. Skipped (with rationale) ------------------------ - **LOW** Add JSDoc to `makeToolbarButton` — sibling factories `makeFolderRow`, `makeNodeLeafRow`, `makeWorkflowLeafRow` have no JSDoc either. Adding it just on the new factory would be inconsistent. If the whole DOM-helpers section gets JSDoc later (under #47's wider widening), this factory should be in scope at the same time. Validation ---------- - `python tools/preflight_release.py --skip manager-meta -v` PASS. - `node --input-type=module --check` on each modified JS file — clean. - `grep -n "innerHTML" web/sidebar/tree.js` → only the L157 empty-string clear remains. - `grep -n "workflowsCache *=" web/sidebar/workflows_store.js` → all rebind sites (init, load×5, rollback, seed) covered by the corrected rule #4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
malkuthroandClaude Opus 4.7
|
c024a94e38 |
feat(sidebar): recursive subdirectories under workflows + rebrand to Kforge Labs (#54)
* feat(sidebar): recursive subdirectories under workflows + rebrand to Kforge Labs
## Recursive subdirectories
Workflow directories can now host nested subdirectories at arbitrary
depth. Each nested directory behaves like a top-level one: it can hold
workflows, its own Archive subfolder, and further subdirectories.
- **Create**: right-click a directory → "Create subdirectory…"
- **Save into**: pick the path from the save modal's flat directory
dropdown (`UP-scale / Type-A`)
- **Move across**: right-click a workflow → "→ <other path>" submenu
lists every other path in the tree
- **Reserved name**: subdirectory names cannot be "Archive" (case-
insensitive) at non-root levels — collides with the synthetic Archive
folder rendering. Top-level "Archive" still allowed (root has no
archived workflows of its own).
### Schema (recursive, back-compat)
workflowsCache = { directories: { [name]: DirNode } }
DirNode = { workflows: { [wfName]: WorkflowEntry }, directories: { [name]: DirNode } }
Pre-v0.3 stores without the nested `directories` field still load fine —
`normalizeWorkflowsStore` treats a missing `directories` as `{}` and
adds the empty field during normalization. No data loss on upgrade.
### Path-based store API
Every mutator/lookup now takes a `path: string[]`:
addDirectory(parentPath, name)
renameDirectory(parentPath, old, new)
deleteDirectory(parentPath, name)
saveWorkflowEntry(path, wfName, graph)
archiveWorkflow(path, wfName) / unarchiveWorkflow(...)
renameWorkflow(path, old, new) / deleteWorkflow(path, wfName)
moveWorkflow(srcPath, wfName, dstPath)
getWorkflowGraph(path, wfName)
New helpers: `listDirectoryNames(parentPath = [])`,
`listAllDirectoryPaths()` (DFS), `pathsEqual(a, b)`. `dirOf([])` returns
a synthetic root wrapper so callers don't need a special-case for root.
### UI changes
- **Save modal**: directory dropdown is now a flat list of every path in
the tree (`JSON.stringify(path)` values, `path.join(" / ")` labels).
"+ New directory…" creates a top-level dir; subdirs go via right-click.
- **Directory header counts**: now show recursive descendant total
(active + archived + all subtree). A parent with empty direct
workflows but populated subdirs no longer shows "0".
- **Save flow**: `saveAndToast` pins every ancestor (`workflows`,
`workflows/seg1`, `workflows/seg1/seg2`, …) so a save into a deeply
nested path opens the whole chain.
- **Render order per directory**: subdirs first → Archive folder →
active workflows last (so the freshest version sits closest to the
bottom).
## Rebrand: "Curated Nodes" → "Kforge Labs"
- `TAB_TITLE`: "Curated Nodes" → "Kforge Labs"
- `TAB_TOOLTIP`: "Curated selection of ComfyUI nodes" →
"Kforge Labs — curated nodes & workflows"
- Canvas right-click menu item: "Add to Curated Sidebar" → "Add to Kforge Labs"
- Empty-state hint in the curated-nodes section follows the new label
`TAB_ID` (`koolook.curatedNodes`) and the `app.registerExtension` name
(`koolook.curated_sidebar`) are deliberately unchanged so existing
per-user tab state (pinning, tab order, idempotency sentinel) is
preserved.
## Files
- web/sidebar/constants.js (rebrand)
- web/sidebar/canvas_menu.js (rebrand)
- web/sidebar/canvas_io.js (loadWorkflowOntoCanvas takes path)
- web/sidebar/workflows_store.js (recursive schema + path-based API)
- web/sidebar/modals.js (save modal flat-path picker)
- web/sidebar/tree.js (gather recursion + recursive renderer + path-based context menus)
- docs/maintainers/workflows-sidebar.md (subdirectories section + recursive JSON shape)
- CHANGELOG.md ([Unreleased] entry)
Encapsulation invariants preserved: `workflowsCache` only in
`workflows_store.js`; `pathStates` only in `tree.js`.
Closes the maintainer's outstanding feature request for subfolders under
workflow directories.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sidebar): show empty directories in tree when no search is active
User-reported: right-click → Create subdirectory → name → Submit
appeared to do nothing. The directory was actually created in the cache
(and showed up correctly in the save modal's path picker), but
gatherDirsAt was dropping any directory with zero matched workflows AND
zero matched descendants — including freshly-created empty subdirs.
Fix: scope the empty-drop check to "search active" only. With no search,
every directory in the cache renders. With a search, the filter still
collapses paths that contributed no hits, so search results stay clean.
Verified via Node dry-test (.tmp_subdir_test.js, since deleted):
- Pre-fix: addDirectory(['UP-scale'], 'Test') succeeds; gather('') drops Test
- Post-fix: gather('') includes Test; gather('hires') still drops empties
Also confirmed there is NO case-sensitivity restriction — 'Test' and
'test' coexist as separate directories. The user's earlier observation
that "only the lowercase one shows" was the empty-dir bug masquerading
as a case issue (whichever directory had been saved into earlier was
visible; the other was hidden).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sidebar): cascading directory picker in save modal (multi-step)
User feedback: the flat path list was unusable with deep nesting — every
existing path appeared in one giant dropdown, you couldn't pick a parent
and then narrow to a child. Replaced with cascading selects.
Behavior:
- Top-level select shows existing top-level dirs + "+ New directory…"
- Picking a top-level with children reveals a level-2 select. The first
option is "(save in <path>)" which stops drilling at this level.
Following options are the children.
- Picking a level-2 child with grandchildren reveals a level-3 select
with the same shape, etc., to arbitrary depth.
- "(save here)" at any level commits the save at that depth.
- Picking a different top-level / parent collapses every cascade level
below it and rebuilds.
- `defaultDir` (a path array) auto-drills the cascade to that path on
open, with onCascadeChange recursing through pre-selected levels.
The save modal's Base-on-existing / Action / Workflow-name sections
re-derive from `getSelectedPath()` whenever any cascade level changes,
so they reflect the current target dir.
Verified via Node dry-test (.tmp_cascade_test.js, since deleted) over 6
scenarios: fresh open, deep defaultDir pre-selection, level-2 collapse,
top-change-to-leafless-dir, "+ New directory…" + name input, manual
drill into a no-children leaf.
The store API is unchanged — `saveWorkflowEntry(path, wfName, graph)`
still takes a path array, and the cascade just produces the same array
through interactive selection instead of a flat dropdown.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sidebar): right-click Archive folder → Delete archive (N items)
Adds a context menu to the synthetic Archive folder that bulk-removes
every archived workflow in its parent directory. Active workflows at the
same level are untouched. The menu only appears when archivedCount > 0
(Archive folder is synthetic and only renders when there are archived
workflows present), so the no-op path is unreachable in practice — the
clearArchive function still defends against it (returns false if no
archived entries to remove or directory missing).
User-asked gap from the subdirectories review. Other adjacent gaps
considered:
- Restore-all from archive: SKIP. Each archived entry has a
"(archived <ts>)" name suffix; bulk-restoring leaves messy long names
AND can collide with the still-active version. Per-item is the right
granularity.
- Move a real Directory across paths: SKIP for this PR. Useful but needs
cycle prevention + path-prefix updates; separate scope.
- Move workflow to a NEW directory inline: SKIP. Two-step workaround
(right-click → Create subdirectory, then move) works.
- Move synthetic Archive folder: N/A. The Archive folder is auto-derived
from `archived: true` flags; nothing to move.
Verified via Node dry-test (deleted): clearArchive correctly removes
only archived entries, preserves active ones, returns false on
empty-archive and missing-dir paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sidebar): pull 'Base on existing' candidates from ancestors too
Reported regression: navigating into an empty subdirectory (e.g.
UP-scale → seedvr2) disabled "Use existing name" / "Modify existing
name" because getActiveWorkflowsInCurrentDir only looked at the leaf
dir's workflows. With nested dirs, leaf dirs are often empty (just
created via right-click), making those actions unreachable for the
common "save a variant of an existing workflow into a new subdir" flow.
Fix: rename to getCandidatesForBase and walk the destination path from
deepest up to root, collecting active workflows at every level. Dedup
by name (deepest wins). The dropdown shows the bare name when the
candidate is from the current dir, or "name · in <path>" when it
comes from an ancestor — so the user knows which dir they're basing on.
The submit path is unchanged: baseSelect.value is still the bare name,
which feeds the workflow-name field for "Use existing"/"Modify existing"
actions. The Save destination path is whatever the cascade resolves to,
not the ancestor path — so basing UP-scale/seedvr2's new save on
"Hires-FIX-wanT2V-v01 · in UP-scale" creates UP-scale/seedvr2/Hires-
FIX-wanT2V-v01 (no archive happens because the destination didn't have
that name before).
Verified via Node dry-test (deleted): empty subdirs now expose the
parent's active workflows; deep empties expose every ancestor's actives;
non-empty leaves show their own first then ancestors with path
disambiguation; archived entries excluded; alphabetical within each
level.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sidebar): drag-and-drop moves in workflows tree (Tier 1)
User: "if we can fix this [DnD] then everything is fixed". Tier 1 ships
the moves-only piece — drag-to-move workflows, drag-to-nest dirs, drag
onto Archive to archive. No custom-ordering schema change (sort within a
level stays alphabetical); that would be Tier 2.
Drag/drop semantics:
workflow row → directory row move workflow (no-op if same dir)
workflow row → Archive folder archive in that dir (move first if
cross-dir, all in one persist)
directory row → directory row nest dragged dir as child of target
Cycle prevention in moveDirectory: rejects same-parent, dropping a dir
into itself, or into any of its descendants. Plus collision (target
already has a sibling with that name) and reserved-name "Archive" at
non-root.
Implementation:
- workflows_store.js: new moveDirectory(srcParentPath, name,
dstParentPath) with isPathDescendantOrSame() helper.
- constants.js CSS: grab/grabbing cursor on draggable rows; blue
outline + tinted background on .koolook-drop-target.
- tree.js: decorateDraggable(row, payload) and decorateDropTarget(row,
target) helpers; handleDndDrop dispatcher; threaded through
makeFolderRow / makeWorkflowLeafRow / buildFolder / makeSectionCtx
so renderGatheredDir can attach DnD opts via the section ctx.
DnD uses the native HTML5 Drag-and-Drop API (no libs). MIME type
"application/x-koolook-row" filters out file/external drags.
Verified via Node dry-test (deleted) over 7 scenarios:
1. legal cross-parent move (Type-A from UP-scale to Depth)
2. cycle: parent → its child → blocked
3. same-parent → blocked (no-op)
4. cycle: dir → its own descendant → blocked
5. name collision in destination → blocked
6. legal sibling-becomes-child nest
7. moving a dir literally named "Archive" to non-root → blocked
Visual feedback: drop targets get .koolook-drop-target on dragover
(blue outline + tinted background), removed on dragleave/drop. Failed
drops surface "Could not move (cycle, collision, …)" toast through the
existing persistMutation onNoOp path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sidebar): hide Action dropdown entirely when no base to act on
User feedback: when saving into a directory whose path has zero active
workflows in any ancestor (genuinely empty case), the Action dropdown
showed three options with "Use existing name" and "Modify existing
name" disabled. Disabled <option> renders subtly across browsers — most
users just see "New name" with no obvious why. They proposed two
options: (1) gray out clearly, or (2) hide the dropdown entirely. Going
with (2) — Action only matters when a base candidate exists; without
one, the only useful flow is "type a name and save," which the
Workflow Name field already provides.
So now:
- hasBase=true (current dir or any ancestor has active workflows) →
full Action picker, all three options enabled.
- hasBase=false (truly empty path, all the way up to root) →
Action label + dropdown hidden; underlying actionSelect.value pinned
to "new" so submit takes the by-name path through the Workflow Name
field.
The ancestor-pull fix from e7da864 means this empty branch only fires
for genuinely-fresh paths (a brand-new top-level + subdir created
together with no workflows anywhere). Common case is hasBase=true.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sidebar): apply verified PR-review findings on #54
Verified review (3 review agents → synthesizer → verifier) on the
recursive-subdirectories + Kforge Labs PR flagged 3 HIGH / 1 MEDIUM /
1 LOW. All confirmed; no false positives. Applying every actionable
fix in one commit.
[HIGH] CHANGELOG disclosure for two undocumented Save-modal shifts:
1) Base on existing now walks ancestor paths leaf-up, dedupes deepest-
wins, labels ancestor entries "<name> · in <path>".
2) Action dropdown is hidden (not just disabled) when no base
candidate exists; underlying value pinned to "new" so submit takes
the by-name path.
Both behaviors landed in late commits (e7da864 and cd9b2c3); the [Unreleased]
entry now describes them under ### Changed alongside the Kforge Labs
rebrand bullets.
[HIGH] Self-drop directory misleading toast (tree.js handleDndDrop):
The same-parent guard caught dir-on-its-own-parent but missed dir-on-
its-own-row, because the dir's row carries `target.path =
[...srcParentPath, name]` which doesn't equal `srcParentPath`.
moveDirectory's cycle check would still reject the move, but the user
got "Could not move directory (cycle, name collision, or invalid
target)" for what is just a no-op gesture. Adds a second early-return
matching `[...srcParentPath, dirName]` against `target.path` so
self-drops are silent.
[MEDIUM] addDirectory(undefined, "x") crash hardening: moveDirectory
already had `Array.isArray` guards on its parent-path inputs;
addDirectory lacked the equivalent and would throw on
`parentPath.length` if a future caller ever passed `undefined`. Adds
the symmetric guard.
[LOW] Drops the dead `isArchiveReservedName` export — the predicate
was never imported. Reserved-name enforcement remains inline in every
mutator that creates or renames a directory. A short comment notes the
removal so future contributors know to revive the export only when a
caller (e.g. modal pre-validation) actually needs it.
Verified via Node dry-test (deleted) covering:
- addDirectory(undefined / null / "string" / []) — all behave correctly
- Self-drop on root 'A' → "no-op-self-drop" (silent)
- Legal cross-parent move 'A' → 'B' → "moved"
- Self-drop on nested 'B/A' → "no-op-self-drop"
- Drop 'B/A' onto own parent 'B' → "no-op-same-parent"
Spec alignment: every fix was verified against PR intent. The two
Save-modal shifts are organic late-stage refinements, not regressions —
disclosure is the only ask and is now delivered. The self-drop guard
and Array.isArray hardening tighten contracts that were already the
clear intent. The dead-export drop is housekeeping.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
malkuthroandClaude Opus 4.7
|
e03523faf9 |
refactor(web): split koolook_sidebar.js into 7 sidebar modules (#51)
* refactor(web): split koolook_sidebar.js into 7 sidebar modules Pure mechanical split — zero behavior change. Implements the design from issue #49 (common-case layout with patchCanvasMenu extracted). Old: web/koolook_sidebar.js (1973 lines, monolithic) New: web/koolook_sidebar.js (48-line entry) + 7 modules in web/sidebar/ - constants.js (130) — REPOS, TAB_*, storage keys, event names, URLs, ensureStyle, toast, compareNames - picks_store.js (117) — localStorage user picks: load/save/add (tri-state) /remove/notify/seed/export - workflows_store.js (347) — workflowsCache + persistMutation + snapshotCache + commit + persist*/load*/seed* + in-memory ops; cache is module-private - canvas_io.js (192) — serialize*/canvasIsNonEmpty/loadWorkflowOntoCanvas /insertNode/getSelectedNodeTypes - modals.js (384) — makeModalShell + showInputModal/showConfirmModal /showSaveWorkflowModal/showContextMenu - canvas_menu.js (45) — patchCanvasMenu (idempotent LGraphCanvas patch) - tree.js (825) — pathStates (module-private) + gatherers + DOM row factories + buildFolder + workflow/dir context menus + rebuildTree + renderPanel Encapsulation invariants verified: - workflowsCache: zero references outside workflows_store.js - pathStates: zero references outside tree.js - All 8 files pass node --check - CSS string byte-identical to original - URL constants resolve to same targets after the extra `..` adjustment Documentation updated: dev-iteration-loop.md and workflows-sidebar.md now point pathStates references at web/sidebar/tree.js. Other doc references to koolook_sidebar.js as the entry remain accurate. Closes #49. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(web): drop unreachable defensive guard in gatherWorkflows Review feedback on #51: the `if (!dir) continue;` after `dirOf(dirName)` is unreachable — `listDirectoryNames()` enumerates the same module-private `workflowsCache.directories` keys that `dirOf()` reads, synchronously, within a single event-loop turn. Removing the guard restores strict "byte-identical move" semantics for the file-split refactor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |