fix(assets): fall back to bare-tag grouping for legacy tags in model_type mode - #14217
Closed
claude[bot] wants to merge 64 commits into
Closed
fix(assets): fall back to bare-tag grouping for legacy tags in model_type mode#14217claude[bot] wants to merge 64 commits into
claude[bot] wants to merge 64 commits into
Conversation
Group the asset-mode Model Library by namespaced model_type:* tags. When an asset carries model_type:* tags, the stripped values are the authoritative categories and all other tags are disregarded; otherwise fall back to the legacy bare-tag top-level grouping. A shared getAssetCategories helper keeps the "By type" nav and category filtering in agreement. Resolve node-provider lookup from the same tags via getAssetNodeCategory: prefer the most specific (deepest parent/child) candidate so a flat model_type:LLM does not shadow a resolvable hierarchical tag, and strip the model_type: prefix before the modelNodeMappings lookup.
Drive the model_type: grouping/resolution off the backend-reported supports_model_type_tags capability (getServerFeature) instead of sniffing tags per asset. Default false preserves the legacy bare-tag grouping, so a backend that does not declare the capability is unchanged. When the flag is true, assets group by their model_type: values and an uncovered asset (no model_type: tag) still routes by its bare tags. The thread-through is a modelTypeMode flag on getAssetCategories / getAssetNodeCategory / filterByCategory (keeping them pure); call sites read the capability via useFeatureFlags.
Unwrap the if (isCloud) gate in assetsStore getModelState so the asset-mode Model Library works on non-cloud distributions (it previously returned an empty no-op store off-cloud, leaving the library blank even with the asset API enabled). Scoped to the model-asset store only; the rest of the isCloud forks are out of scope here.
The card type badge derived from the first raw non-models tag, so in model_type mode it rendered the literal `model_type:checkpoints`. Route it through a model_type-aware getAssetTypeBadge and key the display cache on the flag so a late-arriving feature flag re-derives the badge. Also adds docstrings to the tag helpers to satisfy coverage.
Also realigns the card type badge to the same rule: keep the existing "first non-models tag" selection and only strip the model_type: namespace prefix, rather than reordering to prefer the model_type: value. So an asset like [models, foo, model_type:bar] still shows foo, while [models, model_type:checkpoints] shows checkpoints. Title (primaryCategoryTag) strips the same prefix when the flag is on.
…NodeFromAsset main (#12765) renamed createModelNodeFromAsset -> resolveModelNodeFromAsset. Re-apply FE-1076's flag-gated node-category resolution there: use getAssetNodeCategory(asset, flags.supportsModelTypeTags) instead of the inline first-non-reserved-tag lookup, so a model_type:* tag resolves the loader when the supports_model_type_tags flag is on.
Importing MODELS_TAG/MISSING_TAG from assetService made this leaf util pull the assetService -> i18n (createI18n) chain. Test suites that import assetMetadataUtils (e.g. useAssetFilterOptions, AssetsSidebarListView) with a minimal vue-i18n mock then failed to load. Define the two reserved-tag literals locally so the util has no heavy transitive dependency.
Add afterEach(vi.restoreAllMocks) so the console.error spy doesn't persist across tests (CodeRabbit nit).
An incidental hierarchical tag (e.g. a user-added foo/bar) could outrank an authoritative model_type:vae in node-category resolution, targeting the wrong provider or failing with NO_PROVIDER. Resolution now tries category candidates most-specific-first and takes the first that maps to a provider, so the deepest resolvable path still wins but unresolvable tags fall through.
In modelTypeMode the card badge and modal title now key off the same model_type: value the asset groups under, so they can no longer diverge from the grouping (review consensus on FE-1076). Loader-node resolution demotes bare tags unrelated to any model_type: value to trailing fallbacks: they can no longer pre-empt an authoritative resolvable model_type: however deep they are, while related hierarchical twins (LLM/Qwen-VL over flat model_type:LLM) keep winning. Also: getAssetModelType can no longer return a raw model_type: literal (kills the prefixed round-trip through edit widgets), modelTypeMode is now a required parameter so new call sites must choose explicitly, malformed empty model_type: tags produce no badge, per-asset flag reads hoisted, and a test pins the local reserved-tag mirrors to assetService's canonical values.
A refresh that fails before its first batch commits now mirrors the error onto the committed state consumers actually read, instead of dying silently on the discarded pending state. Pagination stops only when a full page repeats the previous page verbatim (ignored-offset signature); an all-duplicate page whose content merely shifted keeps walking so later pages still load. Teardown of a stale request can no longer evict a newer request's single-flight entries after an interleaved invalidation.
Adversarial review follow-ups: an offset-ignoring backend that reorders its responses could alternate two page orderings and defeat the repeated-page check forever, so the walk now also remembers every served page signature and treats a run of no-new-ID full pages as exhausted. Modal titles for uncovered assets keep their legacy verbatim tag (only covered assets retitle to their model_type value), restoring the pre-alignment behavior the batch accidentally changed, and namespace residue like a malformed empty model_type: can no longer leak into titles, nav items, or categories.
A shared-root asset tagged with several model_type values groups under each of them, but its badge and a node-typed picker's title keyed off only the first value, so viewing it under its second category showed the first category's label. Cards now badge every authoritative model_type value, and a node-typed picker titles off the requested node's category instead of guessing from the first asset.
An asset tagged with both `model_type:LLM` and a bare `LLM` yielded the same category twice in the candidate list; deduplicate the tier-1 values so a provider is only ever tried once per distinct category. Flat output pagination stopped on the first all-duplicate page, so a pagination window shifted by concurrent writes could hide later unseen assets. Reuse the model-category walk's no-progress tolerance for both walks instead of duplicating the counter, letting a bounded run of all-duplicate pages be traversed while still guaranteeing termination.
Prefix the discovery folder_name with model_type: on model import and on model-type edits, gated on the supports_model_type_tags backend capability. Default off writes the legacy bare directory-path tag unchanged, so this is a no-op until the backend declares the cutover; the flag is the same authoritative signal FE-1076 grouping reads. The FE writes only the single selected folder_name's model_type: form; the backend keeps the bare directory-path twin in sync (and forward-expands shared paths), so the edit path manages just the model_type: tags and the resolver read path is untouched. Mirrors the read side: an uncovered asset with no model_type: tag still routes by its bare tag.
Assert the import-by-URL path namespaces only the tag (model_type:<folder_name>) while user_metadata.model_type stays the bare folder_name, since that value still feeds download tracking, category refresh, and the progress label.
The required-type guard and imported-type label compared the bare folder_name against raw asset tags, so a namespaced model_type:<folder> tag was treated as a type mismatch (and leaked the raw prefix into the mismatch label). Strip the model_type: prefix before comparing/labelling so a namespaced tag satisfies the required type. Not reachable in production (the dual-write keeps the bare twin), but hardens the import path against a namespaced-only asset.
Matches the read-path helpers: new call sites must choose the tag mode explicitly instead of silently inheriting legacy behavior.
Re-typing a multi-type asset previously replaced its entire model_type: tag set, silently destroying the other type memberships. Replace only the primary membership (its model_type tag and stale bare twin), keeping siblings intact. Primary is the lexicographically-first model_type value so the choice is deterministic regardless of backend tag order, and the edit dropdown displays that same primary. Extract the selection into one shared helper and privatize the unused tag-prefix export.
Retagging a model rewrites its asset tags, but core is filesystem-backed and does not yet move the file to match the new type, so an edit would leave the tag and the on-disk location diverged. Gate the model type dropdown to cloud builds; on core the field renders the existing read-only label. Revert once core implements move-on-retag (or the asset system no longer requires the file move).
Toasts teleport to body but were missing from the reka outside-layer allowlist, so a non-modal dialog (e.g. Settings) dismissed itself when focus recovery landed on a toast's close button. This happens whenever an action button disables itself while its request is in flight and the request fails: the browser drops focus from the disabled button and it can resurface on the error toast. Latent on main; deterministic once the sidebar rework shifted chunk timing enough for the loading state to render before the failure response.
…(FE-1142) Add a hidden, experimental Comfy.Assets.ModelLibraryAssetView setting that controls whether the model library opens the asset browser or the sidebar, defaulting to the asset browser on Cloud and the sidebar on Core. The model-library toggle now reads this setting instead of Comfy.Assets.UseAssetAPI, decoupling the sidebar-vs-asset-browser view from Asset API enablement so the asset-backed view stays reachable on Core for testing without forcing the cutover.
Derive folders from model_type: tags and use loader_path for the tree, widget value, and viewMetadata. Gate local-disk metadata/preview reads on !isCloud. The models tag is walked once and bucketed per category (multi-category assets land in each), memoized behind a request id so an in-flight walk cannot repopulate an invalidated cache. Assets with a null loader_path are unloadable (orphans) and are dropped with a warning rather than minting a widget value from the deprecated name field; bare-tag backends that predate loader_path keep the name fallback.
Manual refresh now kicks off POST /assets/seed (fire-and-forget, 409 tolerated, skipped on Cloud and on the legacy listing path) alongside the immediate re-load of known server state. The scan's fast phase already persists the category tags and loader filenames the sidebar needs, and the backend announces it with the assets.seed.fast_complete websocket event, so the store re-loads previously loaded folders on that event instead of polling /assets/seed/status (which cannot distinguish the fast phase from enrichment).
In asset mode every folder resolves from one cached walk, so load all folders on sidebar mount: search and badge counts are complete from the start and the load-all button becomes redundant (hidden; it remains for the request-per-folder legacy path, where AutoLoadAll stays the opt-in). Log when a scan's fast phase discovered new files, since the sidebar now updates itself and the console note is the only trace of why.
Search results were a snapshot filled inside handleSearch, so a reload (e.g. a completed scan discovering new files) updated the store without the filtered tree ever re-running. Filter via a computed over the store instead, and expand results from a watch so late-arriving matches unfold too.
…ions /experiment/models returns each folder's raw registered allowlist; an empty array means match-all. The asset walk filters folder contents by the registered set verbatim, substituting an FE-owned copy of core's default supported_pt_extensions when the allowlist is empty or the field is absent (older backends). This reproduces the legacy sidebar's global-set display behavior for match-all folders and extensions-less backends — nothing that was hidden starts showing — while registered allowlists let exotic folders (e.g. .gguf) surface their actual models. The legacy listing keeps its historical server-side filter untouched.
- assert negative membership, no model_type: label leak, flag-absent
default, and orphan (loader_path: null) exclusion
- pin the flag-on/no-loader_path cutover window (BE-4728) with a
dedicated fixture and empty-tree spec so the cross-repo ordering
dependency fails visibly in CI
- swap snapshot-then-assert sites to expect.poll and replace positional
folder assertions with an ordered-text assertion per flake rules
- synchronize the degraded-walk expansion retry on its 500 response
- share the assets.seed.fast_complete wire constant between service and
specs; default createCoreModelAsset overrides to {}
Specs authored against mocked routes; not fully verified locally (app
boot exceeds the fixture timeout under load). Routing spec passed 3/3;
lint, typecheck, knip, and unit tests pass.
…load Search expansion ran only when the query committed, so a folder whose first match arrived from a later reload (a scan completing while the search was active) rendered collapsed and hid the result. Expand result folders once per key instead: newly appearing folders open, folders the user collapsed stay collapsed, and an unchanged tree costs no expand pass.
…type mode modelFolderFromTag returned undefined for any non-namespaced tag once a backend advertised supports_model_type_tags, so buildModelBuckets dropped the asset entirely (with a console.warn) instead of falling back to the legacy bare-tag grouping used on older backends. A legacy bare-tagged model would silently vanish from the sidebar. Add e2e coverage in modelLibraryAssetMode.spec.ts and a unit test for the underlying bucketing behavior.
dosubot
Bot
added
the
size:S
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1759 passed, 0 failed · 1 flaky📊 Browser Reports
📦 Bundle: 8.15 MB gzip 🟢 -132 BDetailsSummary
Category Glance App Entry Points — 3.63 kB (baseline 3.63 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.28 MB (baseline 1.28 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed / 1 unchanged Views & Navigation — 112 kB (baseline 112 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 12 added / 12 removed / 4 unchanged Panels & Settings — 551 kB (baseline 551 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed / 15 unchanged User & Accounts — 29.1 kB (baseline 29.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed / 3 unchanged Editors & Dialogs — 121 kB (baseline 121 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 5 added / 5 removed / 1 unchanged UI Components — 64.8 kB (baseline 64.8 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed / 8 unchanged Data & Services — 3.42 MB (baseline 3.42 MB) • 🟢 -525 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 369 kB (baseline 369 kB) • 🟢 -28 BHelpers, composables, and utility bundles
Status: 16 added / 16 removed / 19 unchanged Vendor & Third-Party — 15.7 MB (baseline 15.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 12.5 MB (baseline 12.5 MB) • ⚪ 0 BBundles that do not match a named category
Status: 70 added / 70 removed / 207 unchanged ⚡ Performance Report
Show regressions
All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-07-29T02:07:46.112Z",
"gitSha": "e8d08005a210d3c86e7c881a9eaa960f756a1455",
"branch": "fix/assets-bare-tag-model-type-fallback",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2084.935999999999,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.214,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 517.808,
"heapDeltaBytes": 3584188,
"heapUsedBytes": 71561536,
"domNodes": 18,
"jsHeapTotalBytes": 20312064,
"scriptDurationMs": 17.616,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-idle",
"durationMs": 2036.1649999999827,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.883000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 490.059,
"heapDeltaBytes": -22105320,
"heapUsedBytes": 45804328,
"domNodes": -271,
"jsHeapTotalBytes": 20176896,
"scriptDurationMs": 14.93,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2030.0860000000114,
"styleRecalcs": 79,
"styleRecalcDurationMs": 45.969,
"layouts": 12,
"layoutDurationMs": 3.6929999999999996,
"taskDurationMs": 1000.1630000000001,
"heapDeltaBytes": -17445392,
"heapUsedBytes": 50449756,
"domNodes": -275,
"jsHeapTotalBytes": 21749760,
"scriptDurationMs": 130.416,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1840.9350000000018,
"styleRecalcs": 76,
"styleRecalcDurationMs": 38.82,
"layouts": 12,
"layoutDurationMs": 3.764,
"taskDurationMs": 881.857,
"heapDeltaBytes": -15868460,
"heapUsedBytes": 52080500,
"domNodes": -275,
"jsHeapTotalBytes": 20176896,
"scriptDurationMs": 123.73299999999998,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1707.505000000026,
"styleRecalcs": 32,
"styleRecalcDurationMs": 16.161,
"layouts": 6,
"layoutDurationMs": 0.63,
"taskDurationMs": 365.00600000000003,
"heapDeltaBytes": 7245772,
"heapUsedBytes": 75158048,
"domNodes": 79,
"jsHeapTotalBytes": 20049920,
"scriptDurationMs": 17.781,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1739.180000000033,
"styleRecalcs": 32,
"styleRecalcDurationMs": 17.575,
"layouts": 6,
"layoutDurationMs": 0.6949999999999998,
"taskDurationMs": 372.66200000000003,
"heapDeltaBytes": 7246072,
"heapUsedBytes": 75172992,
"domNodes": 77,
"jsHeapTotalBytes": 20312064,
"scriptDurationMs": 21.135999999999996,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 594.7680000000162,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.641,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 376.609,
"heapDeltaBytes": -12075916,
"heapUsedBytes": 55819516,
"domNodes": 18,
"jsHeapTotalBytes": 20836352,
"scriptDurationMs": 61.323,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 568.6820000000239,
"styleRecalcs": 11,
"styleRecalcDurationMs": 7.146000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 373.205,
"heapDeltaBytes": -12136620,
"heapUsedBytes": 55844816,
"domNodes": 18,
"jsHeapTotalBytes": 20836352,
"scriptDurationMs": 57.599000000000004,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2023.2469999999694,
"styleRecalcs": 8,
"styleRecalcDurationMs": 8.043000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 730.998,
"heapDeltaBytes": 5625508,
"heapUsedBytes": 64629928,
"domNodes": -276,
"jsHeapTotalBytes": 4255744,
"scriptDurationMs": 124.71099999999998,
"eventListeners": -142,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2040.862000000061,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.548999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 659.403,
"heapDeltaBytes": 6729076,
"heapUsedBytes": 65563860,
"domNodes": -274,
"jsHeapTotalBytes": 3731456,
"scriptDurationMs": 102.94,
"eventListeners": -144,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2177.7819999999792,
"styleRecalcs": 70,
"styleRecalcDurationMs": 14.249999999999995,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1243.039,
"heapDeltaBytes": 5152372,
"heapUsedBytes": 65259160,
"domNodes": -278,
"jsHeapTotalBytes": 4722688,
"scriptDurationMs": 419.209,
"eventListeners": -142,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2135.6799999999794,
"styleRecalcs": 68,
"styleRecalcDurationMs": 13.461999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1302.799,
"heapDeltaBytes": 2316680,
"heapUsedBytes": 62023928,
"domNodes": -277,
"jsHeapTotalBytes": 4984832,
"scriptDurationMs": 428.535,
"eventListeners": -144,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3198.4600000000114,
"styleRecalcs": 65,
"styleRecalcDurationMs": 14.493999999999996,
"layouts": 60,
"layoutDurationMs": 7.063,
"taskDurationMs": 1468.934,
"heapDeltaBytes": -773904,
"heapUsedBytes": 60203888,
"domNodes": -280,
"jsHeapTotalBytes": 7663616,
"scriptDurationMs": 513.547,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3178.058000000078,
"styleRecalcs": 64,
"styleRecalcDurationMs": 13.664999999999997,
"layouts": 60,
"layoutDurationMs": 7.28,
"taskDurationMs": 1516.395,
"heapDeltaBytes": -1632616,
"heapUsedBytes": 59936556,
"domNodes": -283,
"jsHeapTotalBytes": 7401472,
"scriptDurationMs": 532.054,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "minimap-idle",
"durationMs": 2040.0119999999902,
"styleRecalcs": 7,
"styleRecalcDurationMs": 5.54,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 650.3079999999999,
"heapDeltaBytes": 7398968,
"heapUsedBytes": 69648904,
"domNodes": -274,
"jsHeapTotalBytes": 4517888,
"scriptDurationMs": 103.359,
"eventListeners": -144,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2036.004000000048,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.481000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 682.1210000000001,
"heapDeltaBytes": 6752164,
"heapUsedBytes": 67191216,
"domNodes": -276,
"jsHeapTotalBytes": 3993600,
"scriptDurationMs": 109.09699999999998,
"eventListeners": -144,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 550.0109999999836,
"styleRecalcs": 47,
"styleRecalcDurationMs": 9.729,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 381.106,
"heapDeltaBytes": -11839064,
"heapUsedBytes": 56139760,
"domNodes": 20,
"jsHeapTotalBytes": 22147072,
"scriptDurationMs": 112.14699999999999,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 613.2840000000215,
"styleRecalcs": 49,
"styleRecalcDurationMs": 11.790000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 407.604,
"heapDeltaBytes": -11769808,
"heapUsedBytes": 56180956,
"domNodes": 24,
"jsHeapTotalBytes": 22147072,
"scriptDurationMs": 118.632,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2004.92399999996,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.169999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 433.57700000000006,
"heapDeltaBytes": 3507828,
"heapUsedBytes": 71494328,
"domNodes": 18,
"jsHeapTotalBytes": 21098496,
"scriptDurationMs": 12.033999999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 1998.97500000003,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.666,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 482.52200000000005,
"heapDeltaBytes": 3592528,
"heapUsedBytes": 71736988,
"domNodes": 20,
"jsHeapTotalBytes": 20574208,
"scriptDurationMs": 15.632,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1688.2209999999986,
"styleRecalcs": 77,
"styleRecalcDurationMs": 35.664,
"layouts": 16,
"layoutDurationMs": 3.9180000000000006,
"taskDurationMs": 730.7790000000001,
"heapDeltaBytes": -5707512,
"heapUsedBytes": 62431012,
"domNodes": 66,
"jsHeapTotalBytes": 20836352,
"scriptDurationMs": 87.03800000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1713.9139999999315,
"styleRecalcs": 76,
"styleRecalcDurationMs": 34.991,
"layouts": 16,
"layoutDurationMs": 3.723,
"taskDurationMs": 792.6569999999999,
"heapDeltaBytes": -19588912,
"heapUsedBytes": 48472368,
"domNodes": -273,
"jsHeapTotalBytes": 20176896,
"scriptDurationMs": 90.235,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 1402.778000000012,
"styleRecalcs": 19,
"styleRecalcDurationMs": 30.789000000000012,
"layouts": 15,
"layoutDurationMs": 12.54,
"taskDurationMs": 994.5779999999997,
"heapDeltaBytes": 9014888,
"heapUsedBytes": 101072920,
"domNodes": 13673,
"jsHeapTotalBytes": 11010048,
"scriptDurationMs": 37.11699999999998,
"eventListeners": 2371,
"totalBlockingTimeMs": 130,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "viewport-pan-sweep",
"durationMs": 8289.405999999984,
"styleRecalcs": 251,
"styleRecalcDurationMs": 37.759,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4459.450000000001,
"heapDeltaBytes": 27500964,
"heapUsedBytes": 85944176,
"domNodes": -274,
"jsHeapTotalBytes": 5771264,
"scriptDurationMs": 1396.595,
"eventListeners": -128,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8348.428000000013,
"styleRecalcs": 250,
"styleRecalcDurationMs": 37.87799999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4532.459999999999,
"heapDeltaBytes": 1140496,
"heapUsedBytes": 59999976,
"domNodes": -275,
"jsHeapTotalBytes": 6090752,
"scriptDurationMs": 1380.646,
"eventListeners": -158,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 16738.545000000046,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 16696.127,
"heapDeltaBytes": -41072752,
"heapUsedBytes": 167046316,
"domNodes": -8312,
"jsHeapTotalBytes": -14356480,
"scriptDurationMs": 566.375,
"eventListeners": -16387,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 16992.929000000004,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 16969.359,
"heapDeltaBytes": -53853812,
"heapUsedBytes": 167255664,
"domNodes": -8312,
"jsHeapTotalBytes": -13307904,
"scriptDurationMs": 568.723,
"eventListeners": -16385,
"totalBlockingTimeMs": 5,
"frameDurationMs": 17.779999999999927,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 20288.93199999999,
"styleRecalcs": 135,
"styleRecalcDurationMs": 15.757999999999994,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 20231.894999999997,
"heapDeltaBytes": -62098276,
"heapUsedBytes": 155947028,
"domNodes": -8312,
"jsHeapTotalBytes": -13832192,
"scriptDurationMs": 878.0570000000001,
"eventListeners": -16379,
"totalBlockingTimeMs": 136,
"frameDurationMs": 17.776666666666763,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 20178.45299999999,
"styleRecalcs": 137,
"styleRecalcDurationMs": 15.981999999999996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 20124.743,
"heapDeltaBytes": -27650612,
"heapUsedBytes": 180203644,
"domNodes": -8312,
"jsHeapTotalBytes": -12607488,
"scriptDurationMs": 849.33,
"eventListeners": -16385,
"totalBlockingTimeMs": 96,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 453.2029999999736,
"styleRecalcs": 14,
"styleRecalcDurationMs": 18.833999999999996,
"layouts": 2,
"layoutDurationMs": 0.38199999999999995,
"taskDurationMs": 111.23199999999999,
"heapDeltaBytes": -15849452,
"heapUsedBytes": 51122788,
"domNodes": 130,
"jsHeapTotalBytes": 7991296,
"scriptDurationMs": 9.908000000000001,
"eventListeners": 67,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "workflow-execution",
"durationMs": 461.8849999999384,
"styleRecalcs": 13,
"styleRecalcDurationMs": 18.58,
"layouts": 2,
"layoutDurationMs": 0.30100000000000005,
"taskDurationMs": 110.99699999999999,
"heapDeltaBytes": -15981416,
"heapUsedBytes": 51145768,
"domNodes": 119,
"jsHeapTotalBytes": 7729152,
"scriptDurationMs": 9.611999999999998,
"eventListeners": 65,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
}
]
} |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #14217 +/- ##
==========================================
+ Coverage 79.44% 79.67% +0.22%
==========================================
Files 1754 1755 +1
Lines 111080 98054 -13026
Branches 37281 31270 -6011
==========================================
- Hits 88250 78123 -10127
+ Misses 22282 19552 -2730
+ Partials 548 379 -169
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 207 files with indirect coverage changes 🚀 New features to boost your workflow:
|
synap5e
added
the
cursor-review
synap5e
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @synap5e.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟢 Low | 1 |
Panel: 8/8 reviewers contributed findings.
| return tag.startsWith(MODEL_TYPE_TAG_PREFIX) | ||
| ? tag.slice(MODEL_TYPE_TAG_PREFIX.length) | ||
| : undefined | ||
| if (modelTypeMode && tag.startsWith(MODEL_TYPE_TAG_PREFIX)) { |
There was a problem hiding this comment.
🟠 High — The bare-tag fallback now fires per-tag without checking whether the asset already carries authoritative model_type: tags. During the partial re-tagging this change targets, an asset carrying both model_type:checkpoints and a bare checkpoints twin resolves to checkpoints twice — and since buildModelBuckets does not de-duplicate the folder list, the asset is pushed into the same bucket twice (duplicate sidebar rows / duplicate Vue keys and duplicate getAssetModels entries); separately, an unrelated bare tag that matches another folder cross-lists a covered asset into the wrong loader (risking loading a different same-named model). Only fall back to bare-tag grouping when the asset has no model_type: tag, and de-duplicate the resolved folders as defense-in-depth. Raised by 6 of 8 reviewers (gpt-5.6-sol-max edge-case, gemini-3.1-pro edge-case, gpt-5.6-sol-max adversarial, claude-opus-4-8-thinking-max edge-case, claude-opus-4-8-thinking-max adversarial, kimi-k2.7-code edge-case).
Contributor
Author
There was a problem hiding this comment.
Thanks for the review. Both findings addressed in 80b587e (pushed to this branch):
- High: replaced the per-tag
modelFolderFromTagwalk with the existing, already-tested asset-level resolvergetAssetCategories(asset, modelTypeMode), de-duped viaSet. An asset with both amodel_type:tag and a bare twin no longer double-buckets or cross-lists. - Low: left as a documented known limitation, narrowed to only affect assets with no
model_type:tag at all and only junk bare tags. There's no static known-folder-categories list in the codebase to restrict against without a larger restructure, so fixing this fully is out of scope here.
Generated by Claude Code
| return tag.slice(MODEL_TYPE_TAG_PREFIX.length) | ||
| } | ||
| if (tag === MODELS_TAG || tag.length === 0) return undefined | ||
| return tag.split('/')[0] |
There was a problem hiding this comment.
🟢 Low — Previously a non-namespaced tag returned undefined in model-type mode, so an asset with only junk/descriptive bare tags (no real category) was dropped and surfaced the 'has no model category; skipping' diagnostic. Now every bare tag mints a phantom folder bucket, so genuinely mis-tagged assets are silently filed under stray keys and the warning is lost. Consider restricting the fallback to known folder categories or preserving the diagnostic path. Raised by 2 of 8 reviewers (claude-opus-4-8-thinking-max adversarial, claude-opus-4-8-thinking-max edge-case).
… alone modelFolderFromTag resolved a bare-tag fallback per-tag with no visibility into the rest of an asset's tags, so an asset caught mid-retag (carrying both model_type:checkpoints and its pre-migration checkpoints twin) resolved to 'checkpoints' twice and buildModelBuckets pushed it into that bucket twice (duplicate sidebar rows). A leftover unrelated bare tag could also cross-list an already-covered asset into a folder it does not belong in. Replace the per-tag walk with getAssetCategories, which already resolves an asset's categories correctly at the asset level elsewhere in the codebase (useAssetBrowser, assetFilterUtils): a model_type-covered asset groups by its model_type: tags alone, bare tags only apply as a fallback for uncovered assets. Also de-dupe the resolved folder list as defense in depth.
dosubot
Bot
added
size:M
Asset mode hides sidebar folders that load with zero models (modelStore's visibleModelFolders), so an empty 'loras' folder never renders a row at all. The new mid-retag-twin test relied on clicking that row, which timed out on every CI run since the fix under test correctly keeps the mid-retag asset's loras bare tag from populating it. Add a real model_type:loras asset so the folder renders, then assert the mid-retag asset's leaf is absent from it.
The whole-tree locator counted the leaf still legitimately rendered under the still-expanded checkpoints folder, so the assertion that it is absent from loras failed deterministically (received 1, not 0) even though the app correctly excludes it. Scope the query to the loras folder's own subtree instead.
dosubot
Bot
added
size:L
…odel-type-fallback # Conflicts: # browser_tests/fixtures/data/assetFixtures.ts # browser_tests/tests/sidebar/modelLibraryAssetMode.spec.ts
dosubot
Bot
added
size:M
The three new getAssetModels tests (bare-tag fallback, mid-retag de-dupe, cross-list guard) re-assert the exact scenarios already proven end-to-end by modelLibraryAssetMode.spec.ts, and the underlying category-resolution logic they exercise is already pinned directly against getAssetCategories in assetMetadataUtils.test.ts. buildModelBuckets/getAssetModels now just delegate to getAssetCategories, so these tests added no coverage beyond confirming that delegation, which the pre-existing tests in this same describe block (walks the models tag once, buckets by bare tags when unsupported) already establish.
4 tasks
Contributor
|
Recreated as #14848 under my own account for tracking. |
pull Bot
pushed a commit
to Penguinjanator/ComfyUI_frontend
that referenced
this pull request
Aug 12, 2026
…type mode (Comfy-Org#14848) ## Summary Fixes a bug where a legacy bare-tagged model asset silently disappears from the model library sidebar once the backend advertises `model_type:` support, instead of falling back to legacy bare-tag grouping. _Recreated from Comfy-Org#14217 — same change, opened directly under my own account for tracking. The High finding from Cursor's panel on that PR is already folded into this diff._ ## Changes - **What**: `modelFolderFromTag` returned `undefined` for any tag that didn't carry the `model_type:` prefix as soon as `modelTypeMode` was `true`. `buildModelBuckets` treats an asset whose tags resolve to no folder as "uncategorized" and drops it with a `console.warn`, so a model still carrying a legacy bare tag (e.g. `checkpoints` instead of `model_type:checkpoints`) vanished from the sidebar entirely on a `model_type:`-capable backend. Changed in `src/platform/assets/services/assetService.ts`. - **Breaking**: none ## How The fix deletes the local `modelFolderFromTag` helper and buckets via `getAssetCategories` from `assetMetadataUtils`, which already implements exactly the intended semantics and is already used by the other asset surfaces: `model_type:*` values are authoritative when present, and an asset with no `model_type:` tag still routes by its bare tags (with namespace residue filtered out). This makes the sidebar consistent with the rest of the asset code rather than carrying a second, subtly different grouping rule. A side effect worth calling out: because `getAssetCategories` returns `model_type:` values *alone* when an asset has any, an asset covered by `model_type:` tags can no longer be cross-listed into a second folder by a leftover bare-tag twin from a partial re-tagging. The e2e suite covers that case (the "mid-retag twin" scenario). Added an e2e scenario, `Model library sidebar - asset mode with a legacy bare tag`, in `browser_tests/tests/sidebar/modelLibraryAssetMode.spec.ts`, plus a matching fixture (`MODEL_TYPE_CHECKPOINT_LEGACY_TAG` in `browser_tests/fixtures/data/assetFixtures.ts`): a bare-tagged asset that still carries a `loader_path`, walked with `supports_model_type_tags: true`, asserting it still renders in the `checkpoints` folder rather than being dropped. ## Review Focus - This targets `main` directly. The stack it was originally written on top of (Comfy-Org#13574) has since merged, so the diff here is just the three files. - Coverage for the regression lives in the e2e spec rather than a unit test — the unit-level version was dropped as duplicating it. Flagging that explicitly since it is a judgement call about test placement. - Verified against `main` that the bug is still live: `modelFolderFromTag` on `main` still returns `undefined` for bare tags whenever `modelTypeMode` is true. - Checked that the existing `assetService.test.ts` cases on `main` remain valid under the new path: `getBareTagCategories` filters out the reserved `models` tag, so the "drops uncategorized model assets with a warning" case still drops its `tags: ['models']` asset; the bare-tag grouping cases all run with `supports_model_type_tags = false`, an unchanged path. ## Test plan - [ ] `pnpm typecheck` — **not run**: no Node/pnpm toolchain provisioned on the host I authored this on. Relying on CI. - [ ] `pnpm exec vitest run src/platform/assets/services/assetService.test.ts` — **not run**, same reason. Compatibility with the existing cases was verified by reading them (see Review Focus). - [x] Static check that the refactor is complete: `MODEL_TYPE_TAG_PREFIX` import removed with no remaining uses in the file, `modelFolderFromTag` has zero references repo-wide, `getAssetCategories` is exported from `assetMetadataUtils` on `main`. - [x] The identical three-file diff passed a full CI run on Comfy-Org#14217 (Playwright 1759 passed / 0 failed / 1 flaky; Codecov reported all modified lines covered). --- **Review coverage note:** no Cursor panel has run on this PR — verified by querying for the panel review itself rather than inferring from an empty findings list. That is a *did-not-run*, not a clean result; the checks API reports success either way. CodeRabbit was rate-limited. Codex did review it, and its one finding was addressed in `f16560c`. _(An earlier version of this note blamed an org-wide Cursor outage running to 2026-08-22. That attribution was wrong and is withdrawn — panels have since been seen running normally elsewhere. The measured fact, that none ran here, is unchanged.)_ --------- Co-authored-by: Austin Mroz <austin@comfy.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Simon P · Slack thread
Summary
Fixes a bug where a legacy bare-tagged model asset would silently disappear from the model library sidebar once the backend advertised
model_type:support, instead of falling back to legacy bare-tag grouping.Changes
modelFolderFromTagreturnedundefinedfor any tag that didn't carry themodel_type:prefix as soon asmodelTypeModewastrue.buildModelBucketstreats a tag list with no resolved folder as "uncategorized" and drops the asset with aconsole.warn, so a model still carrying a legacy bare tag (e.g.checkpointsinstead ofmodel_type:checkpoints) vanished from the sidebar entirely on amodel_type:-capable backend. The fix makesmodelFolderFromTagfall back to the existing legacy bare-tag grouping logic (same as the non-modelTypeModepath) whenever a tag doesn't carry themodel_type:prefix, regardless ofmodelTypeMode. Changed insrc/platform/assets/services/assetService.ts.How
modelFolderFromTag(tag, modelTypeMode)insrc/platform/assets/services/assetService.tsnow only special-cases tags that actually start withMODEL_TYPE_TAG_PREFIX; any other tag (bare or namespaced-but-unrecognized) falls through to the same bare-tag/top-level-segment grouping used whenmodelTypeModeisfalse.Model library sidebar - asset mode with a legacy bare tag, inbrowser_tests/tests/sidebar/modelLibraryAssetMode.spec.ts, plus a matching fixture (MODEL_TYPE_CHECKPOINT_LEGACY_TAGinbrowser_tests/fixtures/data/assetFixtures.ts): a bare-tagged asset that still carries aloader_path, walked withsupports_model_type_tags: true, asserting it still renders in thecheckpointsfolder rather than being dropped.src/platform/assets/services/assetService.test.tswith a case for the same regression (verified it fails against the pre-fix code, returning[]instead of the expected bucketed asset), since that file already exercisesbuildModelBuckets/modelFolderFromTagdirectly.Review Focus
synap5e/test/assets-sidebar-e2e) rather thanmain.modelFolderFromTag(tag, modelTypeMode)returnsundefinedfor any non-namespaced tag oncemodelTypeModeis true...buildModelBucketsthen... drops the asset with aconsole.warn."http://localhost:5173fortest:browser:local). Verified instead with:pnpm typecheck(clean), eslint on changed files (clean),pnpm exec playwright test ... --list(new test resolves correctly, no syntax errors), and the full Vitest unit suite forassetService.test.ts(66/66 passing, including a manual revert of the fix to confirm the new unit test fails on pre-fix code).Test plan
pnpm typecheckcleanpnpm exec eslinton changed files cleanpnpm exec vitest run src/platform/assets/services/assetService.test.ts— 66/66 passingpnpm exec playwright test browser_tests/tests/sidebar/modelLibraryAssetMode.spec.ts --listresolves the new e2e scenario with no errorsGenerated by Claude Code