Skip to content

Carousel layout redesign - #71

Open
Psylenceo wants to merge 6 commits into
ethanfel:mainfrom
Psylenceo:carousel-layout-redesign
Open

Psylenceo wants to merge 6 commits into
ethanfel:mainfrom
Psylenceo:carousel-layout-redesign

Conversation

@Psylenceo

Copy link
Copy Markdown

Summary

Reshapes the Project Asset Carousel's DOM widget from a flat top-controls-plus-card-strip layout into a source-tree layout that surfaces two kinds of asset relationships that previously had no visual representation: crop/edit lineage and tag-based version families.

Layout

  • Run-name/import/upload/duplicate/refresh controls and the filter tabs stay as full-width rows at the top, as before.
  • Below that, a two-column layout: a resizable source-tree column on the left, and the preview + asset-detail panel side by side on the right (unchanged from before, aside from reflowing).
  • A new full-width row at the bottom shows the currently selected asset's related versions/edits as a row of larger cards.

Crop/edit lineage tree

Cropping or editing an asset already created a new catalog entry carrying a parent_asset_id back-reference (register_derived_image() in project_assets.py) — a backward pointer only, with no forward index anywhere. The left column now builds that forward index client-side (lineageChildren()) and nests a source asset's crops/edits under it with an expand/collapse toggle, instead of listing every asset as a flat strip.

Tag-family version stacking

Adds recognition of the -v<N> tag suffix (@char_bob, @char_bob-v1, @char_bob-v2, ...) as a "version family":

  • Multiple versions of the same tag collapse into a single card showing the latest version, with a caption noting how many versions exist.
  • The family's stack renders at whatever tree position is actually correct — at the root if the base version has no parent, or nested under a shared ancestor if the family's members are themselves crops of something else (rather than always being hoisted to the top of the tree).
  • This is a separate relationship from crop lineage (a crop's parent/child link and a tag-family's version link don't imply each other), and both can be surfaced for the same asset — the bottom detail row unions both when present (e.g. "2 versions, 1 edit").

Backend: -v<N> capture-tag suffix

Review Gate's frame-capture flow already had a "re-use the same tag as an updated take" behavior (_capture_family_tag() in project_assets.py) for exactly this use case, but it used a bare trailing digit (@char_bob1, @char_bob2). Switched to -v<N> and made the ordinal scan match the suffix pattern exactly rather than via startswith, so a tag that merely ends in a number or the letter v@vehicle-van, @char-venessa, @char_bob_house — can no longer be misread as a version of an unrelated family. This only affects newly-created capture tags; no migration of existing data.

Other fixes bundled in

  • Thumbnails: left-column cards are now a fixed 168×126 size with a 92px-tall, center-cropped image — matching the bottom detail row's card size, instead of stretching to the column's full width.
  • Node resize floor: the node previously had no minimum width it could be resized to (only a minimum height). Traced this to ComfyUI's DOM widget always reporting minWidth: 0 from its default computeLayoutSize() — overrode it directly on this widget to report an actual floor derived from the layout's column minimums.

Testing

  • node --test tests/*.mjs — full JS suite passes (53 tests on this branch; literal-string assertions in tests/_project_asset_manager_js_test.mjs etc. are unaffected).
  • PYTHONPATH=. python tests/_project_asset_store_unit_test.py / tests/_asset_store_unit_test.py — pass, including new coverage for the -v<N> capture-tag scheme and a regression guard against the @vehicle-van/@char_bob_house false-match case.
  • Manually verified in a live ComfyUI instance across several rounds: crop lineage nesting/toggle, tag-family stacking at both root and nested attach points, thumbnail sizing, and node width resize floor.

Known limitations

  • Tag-family grouping is purely a naming convention (-v<N> suffix match) — it can't detect a typo (@human_herm-v1 vs @human-herm-v1 won't match) and has no relation to the underlying asset content.
  • Family stacking and folder grouping don't compose: an asset inside a folder is not considered for family stacking.

Psylenceo added a commit to Psylenceo/ComfyUI-MiniMaxH3-Context-Loop that referenced this pull request Sep 15, 2026
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ethanfel

Copy link
Copy Markdown
Owner

Thanks for working on this. I like the direction of the source tree and the separate preview/edit area. I checked commit a316f38faecc62ac82e7c63de22919469c7d897b, and there are a few things to correct before we merge it, especially around keeping existing projects usable.

The UI cases below were reproduced using the actual rendering/grouping functions from this commit with a small mocked DOM and synthetic catalogs. These are code-level reproductions, not a full live-ComfyUI browser test. No existing project was modified.

1. A child assigned to another folder is displayed under its parent's folder

Relevant code: renderCarousel, root detection and folder rendering

Reproduction:

  1. Put an original image in folder References.
  2. Create a crop with parent_asset_id pointing to that image.
  3. Assign the crop to folder Characters.
  4. Expand both folders and the original's edits.

The crop is displayed beneath the original in References, even though its saved folder_id is Characters. With only that crop assigned to Characters, the latter is rendered as an empty folder with a count of zero.

byParent and byId use the complete catalog. The child is skipped at line 2082 before its folder is considered, then rendered through the parent's children without checking its folder. The fallback at line 2110 gives the skipped folder a hardcoded zero count.

Please keep folder membership authoritative for placement and counts. A child whose parent belongs to another folder should remain visible as a root item in its own folder. The related-edits strip can still show its relationship to the original; that relationship should not override where the user put it.

2. Role tabs can hide matching children and show nonmatching ones

Relevant code: full-catalog lineage lookup

Reproduction:

  1. Have a parent with role picture and a derived child with role semantic_anchor.
  2. Switch to the Semantic tab.

The parent is filtered out, but the child is also skipped because the parent still exists in the unfiltered catalog. The result is an empty browser despite having a matching semantic asset. Switching to Images shows the semantic child beneath the image parent instead.

Please build the main tree relative to the active filter as well as folder membership. A matching child whose parent is outside that visible scope needs its own visible entry. Expanding a parent should not bypass the active role filter. This filtering rule can be separate from the intentionally cross-role related-assets strip.

3. Version stacks disappear when their parent is deleted

Relevant code: familyAttachPoint

Reproduction:

  1. Have two unfiled children tagged hero and hero-v1, both pointing to the same parent.
  2. Delete the parent while keeping the children.

Both remaining assets disappear from the main tree. They are still in the catalog; this is a display problem, not deletion of their files.

familyAttachPoint returns the missing parent's ID without checking that it exists. Both members are then marked as hoisted, so neither gets a normal card, and the stack has no existing parent under which to render. The current backend deletion path leaves surviving children's parent references in place, so this is a reachable state.

Please fall back to displaying the stack at the appropriate visible root when its attachment point is absent or outside the current scope. A stale parent reference must not make surviving assets inaccessible.

4. Version naming needs a compatibility policy, and names alone are not enough to identify a family

Backend naming / frontend grouping

hero-v1 is a valid tag, and the delimiter is clearer than assuming every trailing digit means a version. The problem is the transition and the grouping logic:

  • Existing captures hero, hero1, hero2 are not recognized as one version family. Adding a new hero-v1 groups it with hero, while hero1 and hero2 remain separate.
  • assetFamilies groups purely by tag text, without checking any actual capture/version relationship. Two unrelated assets named hero and hero-v1 are grouped, even when one is an image and the other is an audio reference. I reproduced that case too.

Please preserve existing tags and prompt references. Explicit family/version metadata would be safer than inferring identity entirely from a naming convention. For old assets where the relationship cannot be established reliably, keeping them separate and accessible is preferable to guessing or renaming them automatically. Please document how existing captured takes are handled and test mixed old/new catalogs.

To be clear: the PR does not rename existing tags, and the three failing capture tests are not evidence of broken prompt resolution. They currently assert the old naming contract. Those expectations need to be reconciled with the chosen behavior, while retaining their collision, concurrency, and capture-integrity checks.

5. Please separate the unrelated motion-decoding change

Relevant code: _decode_lazy_motion_video EOF handling

This PR also changes early-EOF handling from a bounded last-frame display interval to repeating the final frame through the entire remaining request.

I tested the decoder with a synthetic 12-frame, 24 fps video and a requested window of 0:240. It returned 240 frames, with the additional 228 frames all repeating the last source frame. That is a substantial behavior change, not just frame-rate rounding tolerance. It can conceal insufficient or unexpectedly truncated source media at the decoder level.

Please remove this from the carousel PR and discuss it separately, with an explicit policy and tests for short references versus unexpected early EOF. This reproduction calls the decoder directly; it does not establish that every higher-level preflight path allows such a request.

Also please remove the fork-specific status section added to the upstream README.

Verification and requested regression coverage

Current results in the isolated PR checkout:

  • node --test tests/*.mjs: 53 passed. The existing tests do not cover the new tree/family cases above.
  • tests/_project_asset_store_unit_test.py: passed.
  • tests/_lazy_motion_path_unit_test.py: passed.
  • tests/_review_frame_capture_test.py: 9 passed, 3 failed on tag expectations (test_capture_pixels_and_numbered_takes, test_capture_tag_does_not_collide_with_disabled_asset, and test_concurrent_captures_preserve_all_takes).

Please add regression tests for moving a child between folders, a folder containing only derived children, parent/child roles across tabs, missing/deleted family parents, mixed legacy/new capture tags, and unrelated assets with version-looking names. Include a save/reload check so placement and visibility remain correct after reopening a workflow.

The layout can stay; the important part is that organizing or filtering assets must not make them appear in the wrong place or disappear. A small, scope-aware grouping fix with tests is enough here—we do not need a storage migration or automatic project rewrite.

Psylenceo added a commit to Psylenceo/ComfyUI-MiniMaxH3-Context-Loop that referenced this pull request Sep 17, 2026
isLineageRoot() only considered parent_asset_id, so a crop moved to a
different folder than its parent still rendered nested under it, and
the crop's real folder showed as empty with a hardcoded zero count.
isLineageRoot() and collectChildItems() now also check that a child
shares its parent's folder_id before treating it as nested; a folder
mismatch surfaces the child as its own root item in its real folder
instead. The crop/edit relationship itself is unaffected and still
shown in the bottom detail row.

Reported by ethanfel on PR ethanfel#71.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Psylenceo added a commit to Psylenceo/ComfyUI-MiniMaxH3-Context-Loop that referenced this pull request Sep 17, 2026
assetFamilies() grouped purely by tag text, so an image and an
unrelated audio reference sharing a coincidental -vN tag pattern
(e.g. @hero and @hero-v1) were treated as versions of each other.
Groups (and looks them up) by tag *and* kind together via a new
familyGroupKey(), so a family can never mix images/video/audio.

No change to the "no migration" behavior for older bare-digit capture
tags (@char_bob, @char_bob1, @char_bob2) — they intentionally stay
separate and untouched, since a naming pattern alone can't prove two
old assets are actually related. Documented that explicitly in
docs/PROJECT_ASSETS.md.

Also updates tests/_review_frame_capture_test.py's three assertions
that hard-coded the old bare-digit suffix contract to expect -vN
instead; their actual purpose (collision handling, concurrent-capture
uniqueness, disabled-asset non-collision) is unchanged, only the
literal expected tag strings were stale. One assertion's input was
also changed to request the exact prior "-v1" tag rather than the
old scheme's coincidental "hero1", so it still exercises a genuine
collision against a "-vN" tag.

Reported by ethanfel on PR ethanfel#71.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Psylenceo and others added 3 commits September 17, 2026 00:31
…rees

Reshapes the Asset Carousel DOM widget into a left source-tree column
(crops/edits nested via parent_asset_id, tag-family "-vN" versions
grouped and nested at their correct attach point) alongside a
preview+editor stage, with a bottom panel showing the selected asset's
combined crop and version relationships. Also gives the node an actual
horizontal resize floor (ComfyUI's DOM widgets only report one for
height by default) and switches Review Gate's capture-family tag
suffix from a bare digit to "-vN" to avoid false-positive matches like
@vehicle-van or @char-venessa.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
isLineageRoot() only considered parent_asset_id, so a crop moved to a
different folder than its parent still rendered nested under it, and
the crop's real folder showed as empty with a hardcoded zero count.
isLineageRoot() and collectChildItems() now also check that a child
shares its parent's folder_id before treating it as nested; a folder
mismatch surfaces the child as its own root item in its real folder
instead. The crop/edit relationship itself is unaffected and still
shown in the bottom detail row.

Reported by ethanfel on PR ethanfel#71.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
assetFamilies() grouped purely by tag text, so an image and an
unrelated audio reference sharing a coincidental -vN tag pattern
(e.g. @hero and @hero-v1) were treated as versions of each other.
Groups (and looks them up) by tag *and* kind together via a new
familyGroupKey(), so a family can never mix images/video/audio.

No change to the "no migration" behavior for older bare-digit capture
tags (@char_bob, @char_bob1, @char_bob2) — they intentionally stay
separate and untouched, since a naming pattern alone can't prove two
old assets are actually related. Documented that explicitly in
docs/PROJECT_ASSETS.md.

Also updates tests/_review_frame_capture_test.py's three assertions
that hard-coded the old bare-digit suffix contract to expect -vN
instead; their actual purpose (collision handling, concurrent-capture
uniqueness, disabled-asset non-collision) is unchanged, only the
literal expected tag strings were stale. One assertion's input was
also changed to request the exact prior "-v1" tag rather than the
old scheme's coincidental "hero1", so it still exercises a genuine
collision against a "-vN" tag.

Reported by ethanfel on PR ethanfel#71.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Psylenceo
Psylenceo force-pushed the carousel-layout-redesign branch from 84fcc3f to 291e433 Compare September 17, 2026 04:33
The tree/family placement logic (isLineageRoot, assetFamilies,
familyAttachPoint, collectChildItems, and the whole grouping decision
in renderCarousel) lived only as closures nested inside mount(node),
so nothing outside a live ComfyUI widget could exercise it directly —
this is exactly why several of the placement bugs fixed earlier in
this PR went unnoticed until manual testing.

Moves all of it, unchanged in behavior, into a new
web/h3_project_asset_carousel_core.mjs following this codebase's
existing "_core.mjs" pattern (h3_dom_wheel.mjs, etc.): plain exported
functions taking explicit catalog data, no `state`/DOM dependency.
renderCarousel() now calls one new pure function,
computeCarouselPlacement(), which returns a plain placement plan
(which assets are plain root items, which collapse into a family
stack and where it attaches, which are grouped under a folder), and
just walks that plan to build DOM — no behavior change, confirmed by
the full test suite passing identically before and after.

Adds tests/_project_asset_carousel_placement_test.mjs with direct,
DOM-free regression coverage for every scenario raised in review:
moving a child between folders, a folder containing only derived
children, parent/child roles split across tabs, a deleted/missing
family parent, mixed legacy (hero1/hero2) and new (-vN) capture tags
coexisting without cross-grouping, unrelated same-tag different-kind
assets, and a JSON save/reload round-trip producing identical
placement.

Reported by ethanfel on PR ethanfel#71.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Psylenceo

Copy link
Copy Markdown
Author

Update: folder placement, tab scoping, and naming-policy fixes

Addressed the review feedback with four targeted fixes plus a testability
refactor. Reasoning below for the three with real design decisions behind
them; the other two are more mechanical and covered by their commit
messages.

1. Folder placement is now authoritative over crop lineage

isLineageRoot() only considered parent_asset_id, so a crop moved to a
different folder than its source still rendered nested under the source
(and the crop's real folder showed up empty with a hardcoded zero count).
It now also treats a folder mismatch between a child and its crop-parent as
making the child a root for display purposes — the crop/edit relationship
itself isn't lost, it's still shown in the bottom detail row. Wherever you
put an asset is what the tree now respects; the crop lineage is additional
information, not an override.

2. Tree/tab scoping fixed at the root cause

The tree's parent_asset_id index was built from the whole catalog, not
the active tab's filtered list. That caused two symptoms from the same bug:
a child matching the active filter but whose parent didn't (or vice versa)
would either vanish entirely or bleed into a tab it shouldn't be in. Fixed
by building that index from the tab-filtered asset list instead — a child
whose parent isn't currently visible now gets its own top-level entry
rather than disappearing, and a parent's nested children are naturally
limited to whatever also passes the active filter. Expanding a parent can
no longer bypass the active role/kind filter. The full-catalog view of this
same relationship still drives the bottom detail row intentionally, since
that's meant to be cross-tab.

4. Naming convention kept as-is; explicit media-kind guard added

Agreed with the review's core point that a naming pattern alone can't prove
two assets are related, and deliberately chose not to add any automatic
migration or grouping of older bare-digit capture tags (hero, hero1,
hero2) — they stay exactly as they are, untouched, rather than risk an
automatic scheme reinterpreting something just because it happens to look
like the old pattern. A fresh capture reusing that base tag only ever
groups with another asset already using the exact -vN pattern.

What was a real bug, fixed separately: assetFamilies() grouped purely by
tag text with no check on media kind, so an image tagged @hero and an
unrelated audio reference tagged @hero-v1 were being treated as versions
of each other. Grouping now requires matching tag and kind together, so a
family can never mix images/video/audio. This is documented explicitly in
docs/PROJECT_ASSETS.md, including the no-migration behavior for older
catalogs.

3 & 5, briefly

  • A family whose shared crop-parent is missing/deleted (or filtered out of
    the current tab, per Decode ffmpeg/ffprobe output as UTF-8, not the locale codepage #2's fix) now falls back to rendering at the tree
    root instead of becoming unreachable.
  • Rebased the branch directly onto ethanfel/main to drop several
    unrelated commits that had leaked into the diff from how the branch was
    originally cut (the lazy-motion-decoding EOF tolerance change, and the
    fork-specific README section) — this PR is now scoped to only the
    carousel changes (currently 4 commits / 7 files).

On the regression-coverage request

Added tests/_project_asset_carousel_placement_test.mjs, covering every
scenario requested: moving a child between folders, a folder containing
only derived children, parent/child roles split across tabs, a
deleted/missing family parent, mixed legacy/new capture tags coexisting
without cross-grouping, unrelated same-tag different-kind assets, and a
JSON save/reload round-trip. These needed the placement logic pulled out of
the widget's mount() closure into a standalone module
(web/h3_project_asset_carousel_core.mjs) to be testable at all outside a
live ComfyUI instance — done with no behavior change (verified against the
full suite before/after).

One thing I'm intentionally not doing: adding Chrome/headless-browser-based
test coverage. I don't want a Chrome install on this machine, and the two
existing *_browser_test.mjs failures in the suite are pre-existing
upstream tests unrelated to this PR (they fail here for the same reason —
no local Chrome — not because of anything in this diff). If browser-level
coverage for this change specifically is wanted, it'd need to be run/added
on a machine that already has that environment.

@ethanfel

Copy link
Copy Markdown
Owner

Thanks, this is much closer. I checked f2d628d: the ordinary cross-folder child case, tab filtering, and missing/deleted-parent fallback now pass. The unrelated decoder/README changes are gone too.

One folder-placement case still slips through when the children form a version stack:

  1. Put a source image in the References folder.
  2. Give it two image children tagged hero and hero-v1, both with parent_asset_id pointing to that source.
  3. Leave both children unfiled (folder_id: "").
  4. Expand References and the source's children.

Expected: the two unfiled assets appear as a top-level stack outside References.

Actual: the stack is nested under the source inside References. There is no top-level entry for either child, while the folder's count correctly remains 1. With just the single hero child, the new folder rule works; adding hero-v1 triggers the wrong placement.

I reproduced this with the actual computeCarouselPlacement() and collectChildItems() functions. familyAttachPoint() checks that the parent exists but not that it shares the members' folder. The members are then marked as hoisted, bypassing the ordinary root check. collectChildItems() applies the folder rule to plain children but appends family stacks without that check.

Please apply the same folder/scope rule to stack attachment and fall back to a visible root when the parent is in another folder. Don't just filter the stack out after hoisting, as that would hide it instead. A regression comparing the single-child case with the two-member stack, including a JSON save/reload, should cover this without requiring Chrome.

The existing JS tests pass here, including the browser tests on my machine; the asset-store and all 12 capture tests also pass. This is a remaining display-placement bug, not deletion or movement of the underlying files. I used synthetic catalogs and did not modify an existing project.

@Psylenceo

Psylenceo commented Sep 17, 2026

Copy link
Copy Markdown
Author

that last issue or rather two issues are chrome based, and I'm trying to restrict how much google has access to my stuff. That includes preventing playwright from using chromium modules.

edit: I tried to have claude come up something to test for the issue but it still seems majority of it is chrome based. Otherwise I've included some additional testing to try and validate without having chromium module.

…hildren

familyAttachPoint() nested a version-family stack under its crop-parent
whenever the parent existed, without checking that the members actually
share the parent's folder. A source in one folder with unfiled -vN version
children hoisted the stack out of the plain per-child folder path but then
attached it back under the parent anyway, leaving it invisible: not in the
parent's folder listing (folder rule excludes it) and not surfaced at the
root either.

Fall back to the root attach point when any member's folder_id differs from
the parent's, matching the folder-authority rule already applied to plain
children in isLineageRoot/collectChildItems. Adds a regression comparing the
single-child case (unaffected) against the two-member stack case, plus a
save/reload round-trip.
…g carousel fixes

Extract the wheel-forwarding decision in bindNodeWheel into a pure
resolveWheelRouting() (h3_dom_wheel_core.mjs), mirroring the DOM-free core
pattern already used for the carousel placement logic. Behavior is
unchanged; this only lets the routing matrix (inactive vs. active panel,
already-handled gestures, read-only override) run under plain Node without
needing the Chrome binary the existing _dom_wheel_browser_test.mjs requires
for verifying real native-scroll physics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants