Files

T

malkuthroandClaude Opus 4.7 8e489ad37a feat(snapshot): redesigned Save/Load dialogs + folder picker (#137 ) (#144 )

* feat(snapshot): row-augment max(periodic, pre_load_*) + pytest infra (#137)

Server-side preparation for the snapshot dialogs redesign. The Load
dialog's "newer auto-save available" row flag now reflects the freshest
recovery file regardless of which mechanism wrote it — before this we
only stat'd ``periodic.json``, missing cases where the user's most
recent Load wrote a fresher ``pre_load_*.json`` snapshot inside the
same autosave subfolder.

Extracts ``_latest_autosave_mtime(autosave_dir, named_mtime)`` from the
inline block in ``list_presets`` so the scan is unit-testable. The
helper walks ``periodic.json`` and every ``pre_load_*.json`` once,
returns the max mtime if strictly newer than the named file, else
``None``. Same shape as before, broader source set.

Introduces pytest as a first-class test runner:

  * ``tests/server/test_listing_autosave_mtime.py`` — 5 row-augment
    test cases (periodic-only, pre_load-only, max-of-both, no-newer,
    missing-dir).
  * ``[tool.pytest.ini_options]`` in ``pyproject.toml`` scopes
    discovery to ``tests/`` and uses ``importmode=importlib``.
  * ``[project.optional-dependencies] test = [...]`` defines the
    test extra (pytest + aiohttp, which is a runtime dep of
    ``koolook_routes`` that must be importable even when only a pure
    helper is under test).
  * ``.github/workflows/ci.yml`` gains a ``pytest`` job that installs
    via ``pip install .[test]`` and runs ``pytest -q``.

Guards ``__init__.py``'s relative imports in a ``try / except
ImportError / else`` block. Pytest's package collector walks up from
test files looking for ``__init__.py`` and tries to import the one at
the repo root — but the relative imports inside it only resolve under
ComfyUI's custom-node loader, not under a standalone Python import.
Without the guard, every pytest run aborts during collection with
``attempted relative import with no known parent package``. The
fallback emits a one-line diagnostic and registers empty mappings so
the package directory can still be imported by tooling. ComfyUI's
loader still hits the ``else`` branch and behaviour is unchanged for
real users.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(snapshot): folder picker + agent-driven visual harness (#137)

Commit 2 of the snapshot dialogs redesign. Introduces the navigate-
into folder picker from mockup section 6 — the picker that opens when
the user clicks Save to… / Load from… in the redesigned dialogs (and
that the Settings cog will be removed in favour of, once commit 6
lands). Path input is the source of truth: type/paste + Enter
navigates, clicking a folder row drills in, ↑ Up climbs one level.
Files appear greyed below the directories as a "yes, this is the
folder I expected" affordance.

End-visible path overflow: the path input uses ``direction: rtl`` +
``unicode-bidi: plaintext`` so when the path exceeds the input width
the leading ``/Users/…`` clips on the left, never the trailing folder
name. Per mockup section 6.

Visual harness — the path the issue called out as "visual verification
is a hard gate":

  * ``.claude/launch.json`` defines ``design-server`` (``python3 -m
    http.server 8765``). Agent / maintainer can spin this up via
    ``preview_start`` or directly from a terminal.
  * ``docs/designs/_harness/folder-picker.html`` mounts the real
    ``showFolderPicker`` from ``web/sidebar/modals.js`` against a
    canned filesystem, with one trigger button per mockup-illustrated
    state (default / new-folder-input / empty / error). Cache-busts
    module imports so editing modals.js while the harness is open
    picks up fresh code on reload.
  * ``docs/maintainers/visual-harness.md`` documents the workflow,
    when to use the harness vs. dev-sync, and the backtick-in-CSS-
    comments pitfall that broke this commit twice during development.
  * ``CLAUDE.md`` now lists the harness as the preferred agent-driven
    path; dev-sync stays the integration-level gate.

Server: ``/koolook/presets/browse`` gains an opt-in ``?files=1``
parameter that returns child JSON files alongside the directories.
Default response shape unchanged so the legacy Settings dialog
keeps working. ``_list_child_files`` mirrors ``_list_child_dirs``'s
swallow-OSError-per-entry policy so a broken symlink mid-listing
doesn't sink the whole response.

Tests: six new pytest cases in
``tests/server/test_browse_list_children.py`` covering directory
sorting, autosave-subfolder hiding, JSON-only filter, case-insensitive
extension match, and the empty-result paths. ``pytest`` runs all 11
tests cleanly under the existing CI job from commit 1.

What's still verified via dev-sync (not via the harness):
  * Real Comfy theme tokens (the harness fakes ``--comfy-menu-bg``
    etc. with dark-theme defaults).
  * End-to-end Save → Load round-trip across the new library path.
  * Picker interaction with ComfyUI's keyboard manager.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(snapshot): modal headers stay title-only; route gloss to tooltip (#137)

Follow-up to the folder picker commit. Maintainer flagged that the
"Where snapshots are saved and loaded from." subtitle below the title
was extra information the user doesn't need at first glance — if the
title isn't self-explanatory, fix the title, don't pad it with a
sentence. The mockup at docs/designs/snapshot-dialogs.html section 6
shows a title-only header for exactly this reason.

Locks the rule in as a project convention so every dialog box behaves
the same way going forward:

  * docs/maintainers/conventions.md — new "Modal headers" section
    spells out the rule, what counts as a "descriptive subtitle"
    (remove) vs. a "functional info row" (keep), and how to plumb
    explanatory copy via a hover tooltip on the title instead.
  * web/sidebar/modals.js — makeModalShell now accepts ``titleTooltip``,
    applied as the ``title`` attribute on the modal title element so
    the gloss is mouseover-discoverable but never visible by default.
    showFolderPicker drops its ``subtitle`` param (it was added one
    commit ago, no other callers) in favour of ``titleTooltip``.
  * docs/designs/_harness/folder-picker.html — harness re-points its
    explanatory text to ``titleTooltip``. Verified in the harness: the
    visible header is title-only, the tooltip surfaces on hover.

Applies prospectively to the Save / Load dialogs (commits 3 and 4) —
they will also be title-only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(snapshot): redesigned Save dialog with inline library row + Save to... + 4-state button (#137)

Commit 3 of the snapshot dialogs redesign. The Save dialog is now a
single modal (was three: three-way overwrite / input prompt / confirm)
that matches mockup section 2:

  * Inline library-path info row at the top of the body — "Saved to"
    label on the left, "Open folder ↗" link on the right, full path
    below. Pure information; every action lives in the bottom command
    bar.
  * Bottom command bar with split layout: Save to... on the left,
    Cancel + Save as new... + Save on the right (spacer in between).
  * Save to... opens the folder picker from commit 2. On Use this
    folder, the dialog calls saveSettings() to persist the new library
    path, refreshes its in-place library row, and flips the primary
    button into Dirty state.
  * The primary Save button cycles through the four states from
    mockup section 5:
      - Default ("Save") — clean state, primary blue.
      - Dirty ("Save to new folder") — folder differs from the one the
        dialog opened with; the label spells out the consequence.
      - In progress ("Saving...") — disabled, dimmed primary.
      - Done ("Saved") — disabled, subtle green, brief pause before
        the dialog closes.
  * Save as new... still routes through the existing showInputModal
    plus presetExists guard, but the prompt no longer carries a
    library-path subtitle — the parent dialog already shows the
    target. Convention from the previous commit applied.

tree.js now passes the new capability functions (saveSettings,
browseDirectories, createBrowseDirectory, revealPresetFolder) into
showSaveSnapshotDialog so the dialog can invoke the folder picker
end-to-end inside the Save flow.

Visual harness: docs/designs/_harness/save-dialog.html exercises all
five mockup states (default tracked, untracked Save-as-new, dirty,
in-progress, done) so the maintainer (or future agent) can re-verify
without dev-syncing. Verified all states render correctly in the
preview server.

Static guard against the recurring backtick-in-CSS-comments trap:
tests/sidebar/test_css_template_literal.py asserts that the CSS
template literal in web/sidebar/constants.js contains exactly two
backticks (open + close) and nothing in between. The trap bit this
work three times (commits 2 and 3) — a literal backtick inside a
comment closes the template early and silently bricks the entire
sidebar. The test runs under the same pytest job introduced in
commit 1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(snapshot): redesigned Load dialog with scoped recovery + inline delete (#137)

Commit 4 of the snapshot dialogs redesign. The Load dialog is now a
single modal matching mockup section 3:

  * Inline library row at the top, same shape as the Save dialog —
    "Loaded from" label + Open folder ↗ link + leaf folder name +
    full path. No 📂 emoji button (the link supersedes it).
  * Preset rows: click to load. The "Replace current state?" confirm
    modal is gone — clicking Load is the intent, the dialog title
    already says Load snapshot, no double-confirm.
  * Scoped recovery: when a preset has a newer autosave (per the
    server-side row-augment from commit 1), the first click DOES NOT
    load. It expands exactly ONE recovery row directly beneath the
    parent, showing the freshest entry (newest of periodic.json or
    pre_load_*.json) with its kind badge + meta line. The user then
    chooses: re-click the named row to load the named version, OR
    click the recovery row to restore the auto-save. Both options
    are surfaced inline; the user picks by clicking the one they
    want.
  * Inline delete confirm: × outlines the target row red, the bottom
    Close button transforms to 'Yes — delete "<name>"' (danger
    styling). A second click commits; Escape cancels and reverts.
    No second modal.
  * Bottom command bar matches mockup section 3 split layout:
    [Load from…] on the left, [Close] on the right. Load from…
    opens the folder picker from commit 2; on Use this folder it
    persists via saveSettings() and refreshes the listing.

The pre-#137 YES/NO choice modal (``promptApplyChoice``) and the
collapsible Recovery auto-saves <details> section are both removed —
the scoped-recovery inline row + the click flow above subsume their
job. (Commit 5 in the issue plan was specifically for dropping the
YES/NO modal, which happens here as a natural consequence of the
routing change; commit 5 will be a small follow-up if needed.)

Behavior preserved from the pre-#137 implementation:
  * doNamedLoad — pre-load auto-save backup chain, partial-failure
    tracker handling, identical toast messages.
  * doAutosaveRestore — same backup chain, same markStateAutosaved()
    semantics, same restored-name derivation from subdir.

Tree.js plumbs the three new caps (saveSettings, browseDirectories,
createBrowseDirectory) into showLoadSnapshotDialog so the dialog can
invoke the folder picker end-to-end.

Visual harness: docs/designs/_harness/load-dialog.html exercises
default, empty, scoped-recovery, and pending-delete states. Verified
all four render correctly in the preview server.

Net diff: -56 lines in the sidebar source (the redesign deletes more
than it adds because the YES/NO modal + collapsible recovery
disclosure + confirm modal for plain loads are all gone).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(designs): reconcile snapshot redesign sketch with shipped behaviour (#137)

Commit 5 of the snapshot dialogs redesign. The code-level removal of
the autosave-newer YES/NO modal happened in commit 4 as a natural
consequence of the scoped-recovery inline row taking over its job;
this commit reconciles the design docs so they describe what was
actually built.

docs/designs/snapshot-dialogs-redesign.md:
  * "What changes" item 4 reworded — the YES/NO modal is dropped;
    the affordance is now an inline scoped recovery row that opens
    directly beneath the clicked preset.
  * "What changes" gains items 2, 3, 5 covering the Save/Load dialog
    redesigns and the inline delete confirm that landed in commits 3
    and 4.
  * Mermaid flow rewritten to reflect the new routing (preset click
    → scoped recovery expand → user picks named or recovery row),
    matching commit 4's onPresetClick implementation.

docs/designs/snapshot-dialogs.html "Locked-in decisions":
  * Updated the autosave-modal item from "modal head reads X" to
    "modal — dropped" and explains the inline scoped recovery row
    replacement.
  * Added the title-only modal header convention (commit 2 follow-up)
    pointing to docs/maintainers/conventions.md.
  * Added a pointer to the visual harness workflow added in commit 2.

No code changes — the YES/NO modal was already gone after commit 4.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(snapshot): remove Settings cog + showSnapshotSettingsDialog + CHANGELOG (#137)

Final commit of the snapshot dialogs redesign. The Settings cog disappears
from the Snapshot toolbar — the snapshot row is now status indicator
| Load | Quick Save | Save, matching mockup section 1. Library-path
management now lives inside the Save dialog ("Save to…") and Load
dialog ("Load from…"), both of which open the folder picker introduced
in commit 2 and persist via the existing `saveSettings` route.

Removals:
  * `showSnapshotSettingsDialog` (~325 lines) — the dialog body, its
    inline directory browser, and the "save folder location" state
    machine. The same browse functionality lives in `showFolderPicker`
    now; the same persistence happens via `saveSettings()` called from
    inside Save / Load.
  * `showThreeWayDialog` (~45 lines) — only consumer was the pre-#137
    Save flow (overwrite / Save as new / Cancel), which the redesigned
    Save dialog absorbed into its primary action bar.
  * `renderLibraryLocation` (~17 lines) — replaced by inline rendering
    in the Save and Load dialogs' library rows.
  * Settings cog toolbar button + `openSettingsDialog` wiring in
    tree.js (~12 lines).
  * Orphan `.koolook-browse-*`, `.koolook-path-input-row`,
    `.koolook-load-library-row*`, `.koolook-settings-save-note` CSS
    rules in constants.js (~16 lines) — only consumed by the removed
    dialog. `.koolook-settings-folder-name` and `.koolook-settings-
    folder-path` stay; the new Save / Load library rows reuse them
    verbatim (git-blame continuity for the visual style).

Adds:
  * CHANGELOG.md [Unreleased] entry summarising the whole #137
    feature — picker, redesigned Save/Load dialogs, scoped recovery,
    inline delete, removed Settings cog, pytest infrastructure.

Net diff for this commit: -377 lines.

Cumulative PR diff (commits 1–6 + harness/convention follow-ups):
all six issue ACs satisfied. The maintainer should dev-sync once
before merging to confirm real-Comfy theme tokens render correctly
(harness fakes the dark-theme defaults) and end-to-end Save → Load
round-trips against the live preset folder behave.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ci): install pytest + aiohttp directly, not via pip install . (#137)

The Pytest job introduced in commit 1 of this PR was failing on CI
because ``pip install .[test]`` tries to build a wheel of the entire
project. setuptools's flat-layout discovery then trips on the
non-Python ``web/`` and ``forks/`` directories at the repo root and
errors out with "Multiple top-level packages discovered in a flat-
layout: ['web', 'forks']".

The project is a ComfyUI custom node loaded directly from its source
directory, not a pip-installable package. There's no wheel to build.
Install the test deps explicitly instead — the ``[project.optional-
dependencies] test`` block in pyproject.toml is the spec, CI mirrors
it by hand.

Worked locally because the venv install path went straight to
``pip install pytest aiohttp`` and never touched the project.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(snapshot): untracked dirty state + Load dialog Escape listener leak (#137)

Two findings from the external review on PR #144:

**Blocker — Save dialog's primary action with no tracked preset + folder
changed via Save to...**

``updatePrimaryLabel`` was entering the "dirty" state whenever the
library path diverged from the original, regardless of whether a
tracked preset existed. The primary click handler then routed "dirty"
to ``doOverwrite(current)`` — and with ``current === null``,
``sanitizeName(null)`` would throw / silently coerce to an empty
string instead of asking the user for a name.

Fix: ``isDirty()`` only matters when there is something to overwrite.
With no tracked preset, the primary stays in ``default-new``
regardless of folder divergence. The Save to... button still updates
the library row, so the user has feedback the destination changed; the
eventual primary click then routes to the Save-as-new name prompt and
writes into the new folder cleanly.

Verified in the visual harness: opening Save as untracked, switching
the library to ``/Volumes/Shared/different-library`` via the picker,
then inspecting the primary button:

  * Label stays "Save" (was "Save to new folder" pre-fix)
  * Tooltip "Choose a name and write the snapshot." (was "Write into
    /Volumes/Shared/different-library.")
  * Library row still reflects the new path (saveSettings DID run)

**Major — Load dialog leaked a document keydown listener per session**

The inline-delete Escape-to-cancel handler was registered with
``document.addEventListener`` but never removed. Each open/close cycle
of the Load dialog accumulated another stale handler; long sessions
ended up firing the cancelDelete check on every Escape press across
every dialog (no-op because ``pendingDelete`` was null in closed
dialogs, but the listener was still alive and called).

Fix: capture the handler by name, attach on open, tear down on close
by wrapping ``overlay.remove`` (mirrors the pattern the modal shell
already uses at ``modals.js:52-66`` for its own Escape handler).

Verified by instrumenting ``document.{add,remove}EventListener`` and
opening + closing the Load dialog five times in a row: every keydown
listener added during the cycle was removed before the next cycle
opened. No accumulation.

CI gates: ruff, compileall, bandit (via preflight equivalent), pytest
12/12, preflight 4/4 — all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

2026-05-14 21:00:34 +02:00

6.6 KiB

Raw Permalink Blame History

Visual harness — agent-driven UI verification

The in-tree visual harness mounts the real web/sidebar/ modules in isolation so an agent (or maintainer) can verify rendered UI against a docs/designs/ mockup without running a full ComfyUI install. Component-level visual parity becomes a self-contained loop: edit → reload → screenshot → diff.

This complements (does not replace) the dev-sync integration loop in dev-iteration-loop.md. See When to use the harness vs. dev-sync below.

Files

Path	Role
`.claude/launch.json`	Defines the `design-server` preview config — `python3 -m http.server 8765` from the worktree root. The Claude Code preview tool reads this; running by hand from a terminal works equivalently.
`docs/designs/_harness/`	One HTML file per dialog under verification. Each is a standalone page that mounts the corresponding component from `web/sidebar/`.
`web/sidebar/<module>.js`	Source-of-truth components. The harness imports these directly — no copies, no redefining.
`docs/designs/<feature>.html`	The locked design spec. Open side-by-side with the harness page for visual diff.

How a harness page is wired

Each page in docs/designs/_harness/:

Imports ensureStyle from web/sidebar/constants.js and calls it. That injects the live sidebar stylesheet so what renders is byte- for-byte the same CSS ComfyUI loads.
Imports the component under test from its real location (e.g. showFolderPicker from web/sidebar/modals.js). All transitive imports resolve naturally via the static server — no module stubbing needed for code that doesn't touch ComfyUI globals.
Stubs only the capability functions the component receives as constructor args (browseDirectories, createBrowseDirectory, etc.). The stubs use canned filesystem-shaped data so navigation round-trips end-to-end inside the harness with no Python beyond the static file host.
Auto-mounts the default state on page load. The page header carries buttons that cycle through the additional states the matching mockup card illustrates (default / new-folder-input / empty / error / …). One button per mockup-illustrated state.
Cache-busts the live-module imports with ?bust= + load time so editing modals.js while the harness is open picks up the fresh code on window.location.reload() instead of serving the browser's module cache. This is dev-only — the harness directory is never shipped (filtered out of the Comfy Registry publish, ignored by the sidebar loader).

Running

From the worktree root:

python3 -m http.server 8765

Then open both pages in the same browser tab group:

Harness: http://127.0.0.1:8765/docs/designs/_harness/<feature>.html
Mockup: http://127.0.0.1:8765/docs/designs/<feature>.html

Edit → save → switch to the harness tab → ⌘R / Ctrl-R. The harness imports re-fetch from the static server; the new code renders.

When invoked via the Claude Code preview tool, the equivalent flow is preview_start({ name: "design-server" }) followed by preview_eval (navigation) and preview_screenshot. Resize the viewport to at least 1000 × 700 via preview_resize so the picker / dialogs aren't collapsed into the harness panel's narrow default.

Adding a harness page for a new dialog

Pick the matching mockup card in docs/designs/<feature>.html. Read the surrounding annotations — they specify state names (default, hover, error, …) that become the harness buttons.
Copy the structure of an existing harness page (e.g. folder-picker.html) and adapt:
- Replace the imported component with yours.
- Replace the canned stubs with the capability surface your component expects.
- Add one trigger button per state the mockup shows. Auto-mount the default state on load.
Verify the harness renders matching the mockup before opening the PR. Note in the PR description "visual diff vs. mockup section N" for each cycle that touched rendered output.

When to use the harness vs. dev-sync

Use the harness for any change whose assertion lives at the rendered surface and can be exercised in isolation:

Single-dialog layouts, state transitions inside one component, CSS changes, copy changes, icon swaps.
Stubs cover all the integration surface the component touches.
Iteration loop is ~5 seconds (edit → reload → screenshot).

Use dev-sync for anything that needs a full ComfyUI environment:

Cross-dialog flows (Save dialog opens → Load dialog reflects the written file in the next session).
Theme variables that come from Comfy's frontend chrome (light/dark toggle, custom palette overrides).
End-to-end snapshot library round-trip against a real preset folder on disk.
Any sidebar interaction with ComfyUI's canvas, graph state, or keyboard manager.

A common pattern: harness during implementation to nail the layout, dev-sync once before opening the PR to confirm it still looks right under real Comfy chrome.

What the harness does NOT prove

That the component's integration with the rest of the sidebar still works. (Use dev-sync.)
That theme tokens resolve the same way under real Comfy theming. The harness fakes --comfy-menu-bg etc. with dark-theme defaults; Comfy's actual values may differ slightly.
That the component renders the same way at narrow widths or on mobile breakpoints. Test those explicitly via preview_resize.

If your change might touch any of the above, follow harness verification with a dev-sync pass before requesting review.

Pitfalls

Backticks in CSS comments break the entire sidebar. The CSS in web/sidebar/constants.js is a JS template literal. A literal backtick inside a comment closes the template early, the rest of the file becomes invalid syntax, and the whole sidebar fails to load. Use plain quotes in comments, never backticks. (There's a sentinel comment in constants.js calling this out — read it before adding new CSS.)
Stale module cache. Without the harness's cache-busting query string, the browser may serve the previous version of the module even after a reload. The harness handles this automatically; if you hand-write a one-off harness, mirror the ?bust= + Date.now() pattern.
The harness is dev-only. Do not import harness HTML or its stubs from any code under web/ or koolook_routes.py. The harness lives under docs/designs/_harness/ precisely so it's outside the Comfy Registry archive scanner's walk.

6.6 KiB Raw Permalink Blame History Unescape Escape