refactor(assets): delete isAssetAPIEnabled and Comfy.Assets.UseAssetAPI (FE-729)#12322
Open
dante01yoon wants to merge 17 commits into
Open
refactor(assets): delete isAssetAPIEnabled and Comfy.Assets.UseAssetAPI (FE-729)#12322dante01yoon wants to merge 17 commits into
dante01yoon wants to merge 17 commits into
Conversation
FE-729 (L1.2 — L1 central). Asset API will be always available post-BE-786 (`--enable-assets` removed from OSS). Delete the gating function, the user-facing setting, the experimental warning, and the toggle command. Simplify all caller sites to assume the asset API is available. Changes: - Delete isAssetAPIEnabled() from assetService.ts; simplify shouldUseAssetBrowser() to call isAssetBrowserEligible() directly. - Remove the EXPERIMENTAL_WARNING prefix from four asset-load error messages. - Drop the Comfy.Assets.UseAssetAPI setting from coreSettings.ts and its schema entry in apiSchema.ts. - Remove the Comfy.ToggleAssetAPI command outright. Keep Comfy.BrowseModelAssets but drop its setting-check / setting-set preamble and the "Experimental:" label prefix; the command now just opens the asset browser dialog. - modelStore: drop the useAssetAPI fork in createGetModelsFunc and loadModelFolders; always route through assetService. - sidebarTabStore: model-library tab click always routes through Comfy.BrowseModelAssets (no more setting check). - assetPreviewUtil.isAssetPreviewSupported() returns true unconditionally; caller-site simplification deferred. - WidgetSelect.vue: simplify isAssetMode to drop the isAssetAPIEnabled() arm of the disjunction. - Test mocks/expectations updated. Blocked-merge by BE-786 (OSS `--enable-assets` removal). PR opened as draft. Also auto-fixed unrelated tailwind class-order lint errors in four files (VirtualGrid, RightSidePanel, Textarea, ModelInfoPanel, WidgetSelectDefault) to keep CI green.
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe ChangesAsset API Toggle Deprecation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/composables/useCoreCommands.ts`:
- Line 1286: The label "Browse Model Assets" in the command definition inside
useCoreCommands.ts should be localized: import and use the composition API
useI18n() and replace the raw label value with a call to
t('main.browseModelAssets') (or similar key) in the command object where label
is defined, and add the corresponding "browseModelAssets" entry under the main
namespace in src/locales/en/main.json; ensure the key name matches exactly
between the t(...) call and the JSON entry.
In `@src/platform/assets/services/assetService.ts`:
- Line 229: Replace direct schema.parse()/throw of Zod error details with
schema.safeParse() in the asset service locations that currently throw "Invalid
asset response against zod schema" (the occurrences at the existing throw sites
around the message at lines 229 and 448); on failure, log the full Zod parse
error to your internal logger (e.g., processLogger or the module logger) for
debugging, but throw a generic, user-safe error message (e.g., "Invalid asset
response" or a localized equivalent) instead. Ensure the code path that
previously used schema.parse() now checks result.success and uses result.error
for logging only, and update any callers of those parse sites to expect the same
generic Error type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c90d715-72a7-4165-b3d1-83259c1091ea
📒 Files selected for processing (18)
src/components/common/VirtualGrid.vuesrc/components/rightSidePanel/RightSidePanel.vuesrc/components/ui/textarea/Textarea.vuesrc/composables/useCoreCommands.tssrc/platform/assets/components/modelInfo/ModelInfoPanel.vuesrc/platform/assets/services/assetService.test.tssrc/platform/assets/services/assetService.tssrc/platform/assets/utils/assetPreviewUtil.test.tssrc/platform/assets/utils/assetPreviewUtil.tssrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.asset-mode.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetSelectDefault.vuesrc/schemas/apiSchema.tssrc/stores/modelStore.test.tssrc/stores/modelStore.tssrc/stores/workspace/sidebarTabStore.ts
💤 Files with no reviewable changes (2)
- src/schemas/apiSchema.ts
- src/platform/settings/constants/coreSettings.ts
| id: 'Comfy.BrowseModelAssets', | ||
| icon: 'pi pi-folder-open', | ||
| label: 'Experimental: Browse Model Assets', | ||
| label: 'Browse Model Assets', |
Contributor
There was a problem hiding this comment.
Localize this command label via vue-i18n.
This user-facing label is a raw string and should go through i18n.
💡 Suggested change
- label: 'Browse Model Assets',
+ label: () => t('commands.browseModelAssets'),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/composables/useCoreCommands.ts` at line 1286, The label "Browse Model
Assets" in the command definition inside useCoreCommands.ts should be localized:
import and use the composition API useI18n() and replace the raw label value
with a call to t('main.browseModelAssets') (or similar key) in the command
object where label is defined, and add the corresponding "browseModelAssets"
entry under the main namespace in src/locales/en/main.json; ensure the key name
matches exactly between the t(...) call and the JSON entry.
Collaborator
Author
There was a problem hiding this comment.
Valid finding, but the raw-string label is a pre-existing pattern on main — this PR only removed the Experimental: prefix, not the i18n gap. Full i18n migration for command labels is out of scope for FE-729 (which targets isAssetAPIEnabled() / Comfy.Assets.UseAssetAPI removal). Tracking as a follow-up.
Contributor
There was a problem hiding this comment.
The PR description image shows the ComfyUI Model Library dialog — a dark-themed modal with a left sidebar listing model categories (BEN, BiRefNet, CogVideo, etc.) and a main grid displaying LORA model cards with thumbnails, file names, dates, and "Use" buttons, along with search and filter controls.
@dante01yoon Fair enough — the i18n gap is pre-existing and FE-729 is rightly scoped to the isAssetAPIEnabled / Comfy.Assets.UseAssetAPI removal. Would you like me to open a follow-up GitHub issue to track the full i18n migration for command labels in useCoreCommands.ts?
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8248
File: src/platform/assets/schemas/assetSchema.ts:109-118
Timestamp: 2026-01-22T23:44:59.888Z
Learning: Do not export declarations (e.g., schemas, constants, types) unless they are actually used elsewhere in the codebase. Keep the public API surface minimal by exporting only items with concrete usage. This reduces coupling and keeps imports explicit. In src/platform/assets/schemas/assetSchema.ts, ensure only necessary exports are exposed; remove unused exports or switch to non-exported/internal where appropriate.
Learnt from: JemiloII
Repo: Comfy-Org/ComfyUI_frontend PR: 8282
File: src/composables/usePaste.test.ts:372-383
Timestamp: 2026-01-23T21:06:15.240Z
Learning: In paste/clipboard handling code, do not rely on ephemeral DataTransfer data across async boundaries. If an operation awaits async tasks (such as node creation) during a paste, clone the DataTransfer (e.g., using cloneDataTransfer) to persist files and string data before the paste event completes. This ensures clipboard contents remain available for subsequent processing.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8722
File: src/composables/maskeditor/useBrushDrawing.ts:5-5
Timestamp: 2026-02-08T03:19:12.009Z
Learning: Treat unresolved import warnings caused by pnpm workspace catalog mappings (pnpm-workspace.yaml) and unplugin-typegpu in vite.config.mts as false positives. Do not flag these in PR reviews; they are expected in sandboxes without node_modules and are not indicative of actual missing dependencies in the TS codebase.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/load3d.ts:427-430
Timestamp: 2026-02-19T02:06:23.468Z
Learning: In TypeScript, you can use typeof SomeClass in type annotation positions (e.g., param: typeof LGraphNode) even when SomeClass is imported with import type. This is a type query that only exists at compile time and is erased in runtime, so it is safe to combine with type-only imports. Apply this pattern to TS files broadly when you need a type that references the shape of a class or constructor function without importing the value at runtime.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/uploadAudio.ts:91-94
Timestamp: 2026-02-19T02:06:38.395Z
Learning: In TypeScript files, you can use a type annotation like 'nodeType: typeof MyClass' even if MyClass is imported via 'import type'. Both the type-only import and 'typeof' operate at the type level and are erased at compile time. This pattern is commonly used for constructor types (e.g., 'nodeType: typeof LGraphNode'). Apply this pattern across TypeScript files in the repository (src/**/*.ts) as appropriate, ensuring the imported symbol is a type-only import when possible for clarity and to avoid runtime imports.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8992
File: src/lib/litegraph/src/widgets/GradientSliderWidget.ts:18-18
Timestamp: 2026-02-20T21:08:19.814Z
Learning: When drawing with CanvasRenderingContext2D in TypeScript/JavaScript, wrap the drawing logic with ctx.save() at the start and ctx.restore() at the end to preserve and restore the canvas state. Do not manually destructure and restore individual properties (e.g., fillStyle, strokeStyle); rely on save/restore to manage state changes in a scoped manner. This should be applied to all TS files that perform canvas drawing.
Learnt from: dante01yoon
Repo: Comfy-Org/ComfyUI_frontend PR: 9075
File: src/scripts/api.featureFlags.test.ts:237-268
Timestamp: 2026-02-22T04:27:33.379Z
Learning: In Vite/Vitest, import.meta.env.DEV is true for any mode that is not 'production' (i.e., DEV is the opposite of PROD, and can be true in 'test', 'development', etc.). Do not assume DEV implies only 'development' mode. When reviewing code and tests, treat DEV as a non-production flag and verify environment-specific logic accordingly. Reference: https://vite.dev/guide/env-and-mode#modes
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 9427
File: src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuFilter.vue:33-33
Timestamp: 2026-03-06T00:53:28.835Z
Learning: When reviewing code, note that the enforce-canonical-classes (better-tailwindcss) rule may auto-fix Tailwind v3 !class-name syntax by converting it to v4 class-name! syntax. Do not treat these auto-fixed class-name! instances as newly introduced issues; the perceived change is in syntax placement, not in usage or intent. This guidance applies across all .vue and .ts files in the repository.
Learnt from: sonnybox
Repo: Comfy-Org/ComfyUI_frontend PR: 9446
File: src/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vue:45-45
Timestamp: 2026-03-06T01:55:00.013Z
Learning: Treat wrap-break-word as a valid Tailwind CSS utility for overflow-wrap: break-word in Tailwind v4+ projects. Do not flag this class as invalid in any Vue (.vue) or TypeScript (.ts/.tsx) files within the repository (e.g., Comfy-Org/ComfyUI_frontend) or other Tailwind v4+ projects. When reviewing, verify that the class is used to enable word breaking in long text content and reference the Tailwind docs: https://tailwindcss.com/docs/overflow-wrap.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 11420
File: src/composables/useCanvasScheduler.ts:20-20
Timestamp: 2026-04-19T22:12:36.981Z
Learning: In the ComfyUI frontend Vue/TypeScript codebase, when implementing singleton composables inside `src/composables/**`, prefer `createSharedComposable` from VueUse over module-scoped `let` variables. Module-scoped `let` can cause SSR cross-request state leakage, unpredictable HMR behavior, and requires manual lifecycle/cleanup management for watchers/computeds/effects. `createSharedComposable` ties the singleton to Vue lifecycle: it disposes watchers/computeds and effect-scope reactivity automatically when the last consumer unmounts, improving testability. Only use the module-scoped `let` pattern if the singleton must intentionally outlive all Vue component trees or if there is a clearly documented reason why `createSharedComposable` is inferior for that specific composable.
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 11531
File: src/composables/maskeditor/brushDrawingUtils.ts:14-14
Timestamp: 2026-04-22T04:06:01.353Z
Learning: In the Comfy-Org/ComfyUI_frontend codebase, `knip` is used to detect unused exports. When reviewing TypeScript/TSX code, avoid recommending or introducing exported types/functions/constants that have no concrete external consumers (e.g., they’re only used within the same module). Only recommend exporting when there is an actual external usage outside the module. If something might be needed externally later, keep it non-exported for now and defer exporting until an external consumer is added.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 12197
File: src/renderer/extensions/linearMode/AppInput.vue:23-23
Timestamp: 2026-05-14T21:10:45.382Z
Learning: In the ComfyUI_frontend repo, `WidgetEntityId` is a branded template-literal type (e.g., `${UUID}:${NodeId}:${string}`) and should be treated as structurally non-empty when valid—so an empty string is invalid and must be treated as “absent”. When handling optional `entityId?: WidgetEntityId` values (e.g., for Vue props or TS function parameters), guard missing/invalid IDs with a truthy check (e.g., `if (!entityId) return`) rather than `if (entityId === undefined)`. Rationale: `=== undefined` would allow empty-string values through into stores such as `appModeStore.selectedInputs`, creating rows that can’t resolve back to a widget.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 12197
File: src/core/schemas/parseNodePropertyArray.ts:1-1
Timestamp: 2026-05-18T21:34:41.153Z
Learning: In TypeScript, it is correct to use `import type { z } from 'zod'` when the imported symbol `z` is used exclusively in type-annotation/type-only positions (e.g., `schema: z.ZodType<T[]>`, `z.infer<typeof schema>`, `type X = z.AnyZodObject`, etc.). Because `import type` is fully erased at compile time and requires no runtime value, code reviewers should not flag `import type { z }` as an error (e.g., as an unused value import or incorrect runtime import) in TypeScript/React projects when `z` is only referenced in type positions. If `z` is referenced in a value/runtime position, it should instead be imported with a normal `import { z } from 'zod'`.
| throw new Error( | ||
| `${EXPERIMENTAL_WARNING}Invalid asset response against zod schema:\n${error}` | ||
| ) | ||
| throw new Error(`Invalid asset response against zod schema:\n${error}`) |
Contributor
There was a problem hiding this comment.
Avoid throwing raw schema validation details to callers.
Line 229 and Line 448 include full Zod validation output in thrown errors. Keep detailed parse errors in logs, but throw a generic/localized user-safe message.
💡 Suggested change
- const error = fromZodError(result.error)
- throw new Error(`Invalid asset response against zod schema:\n${error}`)
+ const error = fromZodError(result.error)
+ console.error('Invalid asset response:', error)
+ throw new Error(
+ st(
+ 'assetBrowser.errorInvalidAssetResponse',
+ 'Received invalid asset data from the server. Please try again.'
+ )
+ )- const error = result.error
- ? fromZodError(result.error)
- : 'Unknown validation error'
- throw new Error(`Invalid asset response against zod schema:\n${error}`)
+ const error = result.error
+ ? fromZodError(result.error)
+ : 'Unknown validation error'
+ console.error(`Invalid asset response for ${id}:`, error)
+ throw new Error(
+ st(
+ 'assetBrowser.errorInvalidAssetResponse',
+ 'Received invalid asset data from the server. Please try again.'
+ )
+ )Also applies to: 448-448
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/assets/services/assetService.ts` at line 229, Replace direct
schema.parse()/throw of Zod error details with schema.safeParse() in the asset
service locations that currently throw "Invalid asset response against zod
schema" (the occurrences at the existing throw sites around the message at lines
229 and 448); on failure, log the full Zod parse error to your internal logger
(e.g., processLogger or the module logger) for debugging, but throw a generic,
user-safe error message (e.g., "Invalid asset response" or a localized
equivalent) instead. Ensure the code path that previously used schema.parse()
now checks result.success and uses result.error for logging only, and update any
callers of those parse sites to expect the same generic Error type.
Collaborator
Author
There was a problem hiding this comment.
Valid finding, but the fromZodError(...) throw pattern is pre-existing on main — this PR only removed the EXPERIMENTAL_WARNING prefix from the messages, not the parse / log / throw shape. Sanitizing zod detail at the throw site (log full detail, throw a user-safe message) is a worthwhile improvement but out of scope for FE-729. Tracking as a follow-up.
Contributor
There was a problem hiding this comment.
@dante01yoon, understood — the diff only strips the EXPERIMENTAL_WARNING prefix and doesn't touch the parse/log/throw shape, which is indeed pre-existing. Happy to track the sanitization improvement separately.
Would you like me to open a follow-up GitHub issue for: "Sanitize Zod validation error details at throw sites in assetService — log full detail, throw user-safe message"? I can link back to this PR and comment automatically.
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7746
File: src/platform/assets/services/assetService.ts:484-491
Timestamp: 2026-01-06T19:20:52.000Z
Learning: In asset service files under src/platform/assets/services, prefer using schema.safeParse() over schema.parse() for API response validation. Follow the established pattern: call safeParse(), check result.success, and if failed, convert the error with fromZodError(result.error) for logging while throwing user-friendly errors that do not reveal internal schema details.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8248
File: src/platform/assets/schemas/assetSchema.ts:109-118
Timestamp: 2026-01-22T23:44:59.888Z
Learning: Do not export declarations (e.g., schemas, constants, types) unless they are actually used elsewhere in the codebase. Keep the public API surface minimal by exporting only items with concrete usage. This reduces coupling and keeps imports explicit. In src/platform/assets/schemas/assetSchema.ts, ensure only necessary exports are exposed; remove unused exports or switch to non-exported/internal where appropriate.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8722
File: src/composables/maskeditor/useBrushDrawing.ts:5-5
Timestamp: 2026-02-08T03:19:12.009Z
Learning: Treat unresolved import warnings caused by pnpm workspace catalog mappings (pnpm-workspace.yaml) and unplugin-typegpu in vite.config.mts as false positives. Do not flag these in PR reviews; they are expected in sandboxes without node_modules and are not indicative of actual missing dependencies in the TS codebase.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/load3d.ts:427-430
Timestamp: 2026-02-19T02:06:23.468Z
Learning: In TypeScript, you can use typeof SomeClass in type annotation positions (e.g., param: typeof LGraphNode) even when SomeClass is imported with import type. This is a type query that only exists at compile time and is erased in runtime, so it is safe to combine with type-only imports. Apply this pattern to TS files broadly when you need a type that references the shape of a class or constructor function without importing the value at runtime.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/uploadAudio.ts:91-94
Timestamp: 2026-02-19T02:06:38.395Z
Learning: In TypeScript files, you can use a type annotation like 'nodeType: typeof MyClass' even if MyClass is imported via 'import type'. Both the type-only import and 'typeof' operate at the type level and are erased at compile time. This pattern is commonly used for constructor types (e.g., 'nodeType: typeof LGraphNode'). Apply this pattern across TypeScript files in the repository (src/**/*.ts) as appropriate, ensuring the imported symbol is a type-only import when possible for clarity and to avoid runtime imports.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8992
File: src/lib/litegraph/src/widgets/GradientSliderWidget.ts:18-18
Timestamp: 2026-02-20T21:08:19.814Z
Learning: When drawing with CanvasRenderingContext2D in TypeScript/JavaScript, wrap the drawing logic with ctx.save() at the start and ctx.restore() at the end to preserve and restore the canvas state. Do not manually destructure and restore individual properties (e.g., fillStyle, strokeStyle); rely on save/restore to manage state changes in a scoped manner. This should be applied to all TS files that perform canvas drawing.
Learnt from: dante01yoon
Repo: Comfy-Org/ComfyUI_frontend PR: 9075
File: src/scripts/api.featureFlags.test.ts:237-268
Timestamp: 2026-02-22T04:27:33.379Z
Learning: In Vite/Vitest, import.meta.env.DEV is true for any mode that is not 'production' (i.e., DEV is the opposite of PROD, and can be true in 'test', 'development', etc.). Do not assume DEV implies only 'development' mode. When reviewing code and tests, treat DEV as a non-production flag and verify environment-specific logic accordingly. Reference: https://vite.dev/guide/env-and-mode#modes
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 9427
File: src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuFilter.vue:33-33
Timestamp: 2026-03-06T00:53:28.835Z
Learning: When reviewing code, note that the enforce-canonical-classes (better-tailwindcss) rule may auto-fix Tailwind v3 !class-name syntax by converting it to v4 class-name! syntax. Do not treat these auto-fixed class-name! instances as newly introduced issues; the perceived change is in syntax placement, not in usage or intent. This guidance applies across all .vue and .ts files in the repository.
Learnt from: sonnybox
Repo: Comfy-Org/ComfyUI_frontend PR: 9446
File: src/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vue:45-45
Timestamp: 2026-03-06T01:55:00.013Z
Learning: Treat wrap-break-word as a valid Tailwind CSS utility for overflow-wrap: break-word in Tailwind v4+ projects. Do not flag this class as invalid in any Vue (.vue) or TypeScript (.ts/.tsx) files within the repository (e.g., Comfy-Org/ComfyUI_frontend) or other Tailwind v4+ projects. When reviewing, verify that the class is used to enable word breaking in long text content and reference the Tailwind docs: https://tailwindcss.com/docs/overflow-wrap.
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 11531
File: src/composables/maskeditor/brushDrawingUtils.ts:14-14
Timestamp: 2026-04-22T04:06:01.353Z
Learning: In the Comfy-Org/ComfyUI_frontend codebase, `knip` is used to detect unused exports. When reviewing TypeScript/TSX code, avoid recommending or introducing exported types/functions/constants that have no concrete external consumers (e.g., they’re only used within the same module). Only recommend exporting when there is an actual external usage outside the module. If something might be needed externally later, keep it non-exported for now and defer exporting until an external consumer is added.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 12197
File: src/renderer/extensions/linearMode/AppInput.vue:23-23
Timestamp: 2026-05-14T21:10:45.382Z
Learning: In the ComfyUI_frontend repo, `WidgetEntityId` is a branded template-literal type (e.g., `${UUID}:${NodeId}:${string}`) and should be treated as structurally non-empty when valid—so an empty string is invalid and must be treated as “absent”. When handling optional `entityId?: WidgetEntityId` values (e.g., for Vue props or TS function parameters), guard missing/invalid IDs with a truthy check (e.g., `if (!entityId) return`) rather than `if (entityId === undefined)`. Rationale: `=== undefined` would allow empty-string values through into stores such as `appModeStore.selectedInputs`, creating rows that can’t resolve back to a widget.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 12197
File: src/core/schemas/parseNodePropertyArray.ts:1-1
Timestamp: 2026-05-18T21:34:41.153Z
Learning: In TypeScript, it is correct to use `import type { z } from 'zod'` when the imported symbol `z` is used exclusively in type-annotation/type-only positions (e.g., `schema: z.ZodType<T[]>`, `z.infer<typeof schema>`, `type X = z.AnyZodObject`, etc.). Because `import type` is fully erased at compile time and requires no runtime value, code reviewers should not flag `import type { z }` as an error (e.g., as an unused value import or incorrect runtime import) in TypeScript/React projects when `z` is only referenced in type positions. If `z` is referenced in a value/runtime position, it should instead be imported with a normal `import { z } from 'zod'`.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #12322 +/- ##
===========================================
- Coverage 76.76% 63.03% -13.73%
===========================================
Files 1573 1461 -112
Lines 107148 74602 -32546
Branches 33135 20983 -12152
===========================================
- Hits 82248 47025 -35223
- Misses 23982 27231 +3249
+ Partials 918 346 -572
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1167 files with indirect coverage changes 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:M
… callers
Follow-up to the previous FE-729 commit. After deleting
isAssetAPIEnabled, isAssetPreviewSupported() became a wrapper that
always returned true. Remove the function and simplify all callers.
Changes:
- Delete isAssetPreviewSupported() from assetPreviewUtil.ts.
- Media3DTop.vue: drop the isAssetPreviewSupported() arm of the
loadThumbnail guard (asset.name check is still required).
- saveMesh.ts: unwrap two `if (isAssetPreviewSupported()) { ... }`
blocks in applySaveGLBOutput and the SaveGLB beforeRegisterNodeDef
extension callback.
- FormDropdownMenuItem.vue: drop the early return from
resolveMeshPreview.
- useLoad3d.ts: drop the isAssetPreviewSupported() arm of the
modelReady guard.
- Tests: remove the dead "asset preview is unsupported" branches
(useLoad3d, Media3DTop, FormDropdownMenuItem) and clean up the
associated mocks and hoisted state.
Auto-fixed unrelated tailwind class-order lint errors in five files
(VirtualGrid, RightSidePanel, Textarea, ModelInfoPanel,
WidgetSelectDefault) to keep CI green.
dosubot
Bot
added
size:L
Collaborator
Author
2026-05-19 — Cloud verificationVerified locally against Smoke result on Cloud:
Cloud-side (Screenshot attached separately — see comments.) |
dante01yoon
changed the title
7 tasks
Temporary unblock for FE-729 e2e while BE-786 is in flight. FE-729 removes the legacy /api/models fallback in modelStore, so the assets endpoint must be reachable in CI. BE-786 will make assets always-on in OSS core; once that ships in the CI ComfyUI image, this flag (and this commit) MUST be reverted before merging FE-729 to main.
After FE-729 the model-library sidebar tab routes to the Asset Browser dialog and never renders ModelLibrarySidebarTab.vue. The legacy sidebar-tree tests now validate dead code paths: - delete browser_tests/tests/sidebar/modelLibrary.spec.ts entirely (tab/folders/search/refresh/empty-state — all dead) - drop the model-library entry from the defaultKeybindings sidebar toggle table (KeyM no longer toggles a panel) - remove the Refresh-clears-resolved-missing-model case from errorsTabMissingModels.spec.ts — it waited for the legacy /experiment/models endpoint that modelStore no longer hits Dialog-flow coverage for the new entry point belongs to FE-732.
FE-729 routes model widgets (ckpt_name, lora_name, etc.) through WidgetSelectDropdown on OSS as well as Cloud. That dropdown's trigger button has no aria-label — the widget name lives in a sibling label div ([data-testid="widget-layout-field-label"]). Grid-mode widgets (WidgetSelectDefault) still carry aria-label on the input directly. Update the helper to .or() the two strategies so existing aria-label lookups keep working while asset-mode dropdowns are reached via the visible label text + sibling trigger button.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/fixtures/helpers/BuilderSelectHelper.ts`:
- Around line 131-133: The fallback label selector uses filter({ hasText:
widgetName }) which does substring matching and can pick the wrong label; change
that call on the locator returned by getByTestId("widget-layout-field-label") to
use an anchored regular expression that matches the entire widgetName (e.g., new
RegExp('^' + escapeRegExp(widgetName) + '$')) so the match is exact; ensure you
add or use an escapeRegExp helper to safely escape any regex metacharacters in
widgetName and keep the final .locator('..') chain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42a1c44a-0c3e-4976-a9ef-f8d23e3a9c05
📒 Files selected for processing (1)
browser_tests/fixtures/helpers/BuilderSelectHelper.ts
Comment on lines
+131
to
+133
| .getByTestId("widget-layout-field-label") | ||
| .filter({ hasText: widgetName }) | ||
| .locator('..') |
Contributor
There was a problem hiding this comment.
Use exact text matching in the fallback label selector
hasText: widgetName is substring-based and can match the wrong field label when names overlap, causing flaky/misdirected clicks. Please make this match exact.
Suggested fix
+function escapeRegExp(value: string): string {
+ return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+}
+
const widgetLocator = node
.getByLabel(widgetName, { exact: true })
.or(
node
.getByTestId("widget-layout-field-label")
- .filter({ hasText: widgetName })
+ .filter({ hasText: new RegExp(`^${escapeRegExp(widgetName)}$`) })
.locator('..')
.locator('button')
.first()
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .getByTestId("widget-layout-field-label") | |
| .filter({ hasText: widgetName }) | |
| .locator('..') | |
| function escapeRegExp(value: string): string { | |
| return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') | |
| } | |
| const widgetLocator = node | |
| .getByLabel(widgetName, { exact: true }) | |
| .or( | |
| node | |
| .getByTestId("widget-layout-field-label") | |
| .filter({ hasText: new RegExp(`^${escapeRegExp(widgetName)}$`) }) | |
| .locator('..') | |
| .locator('button') | |
| .first() | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser_tests/fixtures/helpers/BuilderSelectHelper.ts` around lines 131 -
133, The fallback label selector uses filter({ hasText: widgetName }) which does
substring matching and can pick the wrong label; change that call on the locator
returned by getByTestId("widget-layout-field-label") to use an anchored regular
expression that matches the entire widgetName (e.g., new RegExp('^' +
escapeRegExp(widgetName) + '$')) so the match is exact; ensure you add or use an
escapeRegExp helper to safely escape any regex metacharacters in widgetName and
keep the final .locator('..') chain unchanged.
6 tasks
…Widget Previous .or() implementation acted as a union — both branches resolved for number widgets (e.g. 'seed' has aria-label on wrapper + a Decrement button under widget-layout-field-label), causing Playwright strict mode violations on tests that previously passed. Switch to a count-based check: prefer the aria-label match when it exists, otherwise fall back to the dropdown-trigger button under the visible label row. This restores grid/number widget behaviour while keeping asset-mode dropdown support.
dante01yoon
added a commit
that referenced
this pull request
May 26, 2026
Align with the L1 stack pattern (FE-729 #12322 / FE-730 #12335 / FE-731 #12375 / FE-732 #12417): the Generated tab now resolves to `useFlatOutputAssetsGrouped()` unconditionally. OSS reaches the same asset-API path as cloud once BE-786 lands and `_ASSETS_ENABLED` is no longer a gate; no transitional fallback is kept, in line with FE-730's "no temporary isCloud fallback" stance. FE-740
8 tasks
dante01yoon
added a commit
that referenced
this pull request
Jun 2, 2026
…ed feature-flag test infra Per review (DrJKL): mockFeatureFlags and the exported FeatureFlags type have no live consumer in the open stack — FE-729~732 (#12322/#12335/#12375/#12417) don't use them, and FE-780/781 (#12485/#12486) still hand-roll inline vi.hoisted mocks. Shipping them here adds a public surface with no caller, so split them out; they'll be reintroduced bundled with the first PR that actually adopts the util. This PR is now scoped to the getAssetStoredFilename extraction, which is consumed at its two call sites in useMediaAssetActions. Retained-code review fixes: - M3: spread importOriginal in the distribution/types mock so isDesktop and isNightly survive the wholesale replacement. - L2: trim getAssetStoredFilename JSDoc; demote the BE-933/934 collapse to a one-line TODO. Removed: src/test-utils/mockFeatureFlags.ts, the FeatureFlags type export, and its knip ignore entry. Pre-split state preserved on local branch jaewon/fe-733-presplit-snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dante01yoon
added a commit
that referenced
this pull request
Jun 2, 2026
…ed feature-flag test infra Per review (DrJKL): mockFeatureFlags and the exported FeatureFlags type have no live consumer in the open stack — FE-729~732 (#12322/#12335/#12375/#12417) don't use them, and FE-780/781 (#12485/#12486) still hand-roll inline vi.hoisted mocks. Shipping them here adds a public surface with no caller, so split them out; they'll be reintroduced bundled with the first PR that actually adopts the util. This PR is now scoped to the getAssetStoredFilename extraction, which is consumed at its two call sites in useMediaAssetActions. Retained-code review fixes: - M3: spread importOriginal in the distribution/types mock so isDesktop and isNightly survive the wholesale replacement. - L2: trim getAssetStoredFilename JSDoc; demote the BE-933/934 collapse to a one-line TODO. Removed: src/test-utils/mockFeatureFlags.ts, the FeatureFlags type export, and its knip ignore entry. Pre-split state preserved on local branch jaewon/fe-733-presplit-snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mathtone
pushed a commit
to sketchyplans/ComfyUI-frontend-fork1
that referenced
this pull request
Jun 17, 2026
…fy-Org#12287) ## Summary L1 prerequisite cleanup, scoped to a single type-preserving refactor: extract `getAssetStoredFilename(asset)` to collapse the duplicated `isCloud && asset.asset_hash ? asset.asset_hash : asset.name` branch from `useMediaAssetActions.ts` into one helper in `assetMetadataUtils.ts`. No behavior change. Once BE-933/934 emit `file_path` and the cloud spec sync brings the field into generated types, only the helper internals change (collapse to `asset.file_path ?? asset.name`). ## Scope change (per review) The `mockFeatureFlags` test util and the exported `FeatureFlags` type that this PR originally also added have been **split out**. They had no live consumer in the open stack — FE-729~732 (Comfy-Org#12322 / Comfy-Org#12335 / Comfy-Org#12375 / Comfy-Org#12417) don't use them, and FE-780 / FE-781 (Comfy-Org#12485 / Comfy-Org#12486) still hand-roll inline `vi.hoisted` mocks — so shipping them here would add a public surface with no caller. They will be reintroduced bundled with the first PR that actually adopts the util, where `featureFlag`'s return type and the "all flags off vs. production defaults" semantics can be validated against a real consumer. ## Review fixes carried in this PR - Mock `@/platform/distribution/types` via an `importOriginal` spread so `isDesktop` / `isNightly` survive the wholesale replacement (only `isCloud` was re-hoisted before). - Trimmed the `getAssetStoredFilename` JSDoc; the BE-933/934 future-collapse is now a one-line `TODO` rather than a design-doc paragraph. ## Review Focus - The helper is intentionally named `getAssetStoredFilename` to disambiguate from the existing `getAssetFilename` (which targets `user_metadata.filename` / `metadata.filename` for serialized-identifier contexts — missing-model matching, filename schema validation) and `getAssetDisplayFilename` (UI labels). Folding the `isCloud && asset_hash` fallback into either of those would regress display/identifier sites where the cloud hash is never meant to surface. - Fixes FE-733 - Parent: FE-601 (L1 umbrella) - RFC: [Asset Identity Semantics](https://www.notion.so/comfy-org/RFC-Asset-Identity-Semantics-35a6d73d365080e59d59c98cebae779b) - Survey: [Asset FE Divergence Survey M1 Scope](https://www.notion.so/comfy-org/Assets-FE-Divergence-Survey-M1-Scope-3616d73d365080d0a9cbf5f2394c12f8) - Slack thread: https://comfy-organization.slack.com/archives/C0AUUTS2RQV/p1778815571949519 ## Screenshots (if applicable) N/A — no UI change. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-12287-refactor-assets-extract-getAssetStoredFilename-helper-add-mockFeatureFlags-test-util-3616d73d365081c9a1c6e1982728a38a) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Matt Miller <matt@miller-media.com>
🎭 Playwright: ❌ 1653 passed, 1 failed · 3 flaky❌ Failed Tests📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 7.45 MB gzip 🟢 -660 BDetailsSummary
Category Glance App Entry Points — 46.7 kB (baseline 46.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.25 MB (baseline 1.25 MB) • 🟢 -1.07 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 95.3 kB (baseline 95.3 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 3 unchanged Panels & Settings — 525 kB (baseline 525 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed / 15 unchanged User & Accounts — 19.9 kB (baseline 19.9 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed / 3 unchanged Editors & Dialogs — 112 kB (baseline 112 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 4 added / 4 removed / 1 unchanged UI Components — 57.2 kB (baseline 57.2 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 8 unchanged Data & Services — 268 kB (baseline 268 kB) • 🟢 -44 BStores, services, APIs, and repositories
Status: 13 added / 13 removed / 3 unchanged Utilities & Hooks — 3.32 MB (baseline 3.32 MB) • 🟢 -1 kBHelpers, composables, and utility bundles
Status: 14 added / 14 removed / 16 unchanged Vendor & Third-Party — 15.3 MB (baseline 15.3 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 10.4 MB (baseline 10.4 MB) • 🟢 -286 BBundles that do not match a named category
Status: 61 added / 61 removed / 89 unchanged ⚡ Performance Report
✅ No regressions detected. All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-06-20T00:09:13.893Z",
"gitSha": "545635ae1c7ea0b6539cebb8deb31c230caa0d51",
"branch": "jaewon/fe-729-delete-is-asset-api-enabled",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2018.0959999999857,
"styleRecalcs": 7,
"styleRecalcDurationMs": 6.255999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 373.24999999999994,
"heapDeltaBytes": -2381580,
"heapUsedBytes": 56705284,
"domNodes": 14,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 18.733000000000004,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2024.3470000000343,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.591,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 371.71700000000004,
"heapDeltaBytes": -2219596,
"heapUsedBytes": 56636700,
"domNodes": 22,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 21.153,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1840.2949999999976,
"styleRecalcs": 74,
"styleRecalcDurationMs": 44.43,
"layouts": 12,
"layoutDurationMs": 3.284,
"taskDurationMs": 812.696,
"heapDeltaBytes": -6966940,
"heapUsedBytes": 51749596,
"domNodes": 60,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 116.729,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1791.0469999999918,
"styleRecalcs": 71,
"styleRecalcDurationMs": 37.029999999999994,
"layouts": 12,
"layoutDurationMs": 3.371,
"taskDurationMs": 756.722,
"heapDeltaBytes": -9310380,
"heapUsedBytes": 59725068,
"domNodes": -229,
"jsHeapTotalBytes": 16429056,
"scriptDurationMs": 109.85099999999998,
"eventListeners": -193,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1741.6190000000142,
"styleRecalcs": 30,
"styleRecalcDurationMs": 15.953000000000001,
"layouts": 6,
"layoutDurationMs": 0.5549999999999999,
"taskDurationMs": 358.56499999999994,
"heapDeltaBytes": 1859444,
"heapUsedBytes": 60608348,
"domNodes": 78,
"jsHeapTotalBytes": 26214400,
"scriptDurationMs": 27.278999999999996,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1722.8709999999978,
"styleRecalcs": 32,
"styleRecalcDurationMs": 18.369,
"layouts": 6,
"layoutDurationMs": 0.7470000000000002,
"taskDurationMs": 338.805,
"heapDeltaBytes": 7394904,
"heapUsedBytes": 71041856,
"domNodes": 78,
"jsHeapTotalBytes": 15466496,
"scriptDurationMs": 30.518000000000004,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 574.9250000000075,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.325,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 351.178,
"heapDeltaBytes": 7390156,
"heapUsedBytes": 66022388,
"domNodes": 20,
"jsHeapTotalBytes": 19136512,
"scriptDurationMs": 59.459,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "dom-widget-clipping",
"durationMs": 527.4779999999737,
"styleRecalcs": 12,
"styleRecalcDurationMs": 7.378000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 333.59499999999997,
"heapDeltaBytes": 7262412,
"heapUsedBytes": 65903432,
"domNodes": 20,
"jsHeapTotalBytes": 19136512,
"scriptDurationMs": 55.36800000000001,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666682,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 1993.471999999997,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.818000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 521.6750000000001,
"heapDeltaBytes": -8027944,
"heapUsedBytes": 62103384,
"domNodes": 20,
"jsHeapTotalBytes": 10891264,
"scriptDurationMs": 89.44,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2020.5060000000117,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.243000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 541.823,
"heapDeltaBytes": -7996560,
"heapUsedBytes": 62638712,
"domNodes": 20,
"jsHeapTotalBytes": 10366976,
"scriptDurationMs": 99.683,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-pan",
"durationMs": 2100.611000000015,
"styleRecalcs": 70,
"styleRecalcDurationMs": 20.365000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1097.379,
"heapDeltaBytes": 9927812,
"heapUsedBytes": 83079868,
"domNodes": 18,
"jsHeapTotalBytes": 9232384,
"scriptDurationMs": 397.253,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-pan",
"durationMs": 2071.673999999973,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.280999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1096.002,
"heapDeltaBytes": 27984460,
"heapUsedBytes": 83607788,
"domNodes": 6,
"jsHeapTotalBytes": 2301952,
"scriptDurationMs": 395.326,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3113.4739999999965,
"styleRecalcs": 66,
"styleRecalcDurationMs": 19.636,
"layouts": 60,
"layoutDurationMs": 8.02,
"taskDurationMs": 1282.201,
"heapDeltaBytes": 12929076,
"heapUsedBytes": 68475636,
"domNodes": 14,
"jsHeapTotalBytes": 5767168,
"scriptDurationMs": 453.288,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3170.1189999999997,
"styleRecalcs": 65,
"styleRecalcDurationMs": 19.654999999999998,
"layouts": 60,
"layoutDurationMs": 8.084000000000001,
"taskDurationMs": 1347.702,
"heapDeltaBytes": 13129704,
"heapUsedBytes": 68848828,
"domNodes": 12,
"jsHeapTotalBytes": 6029312,
"scriptDurationMs": 489.057,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "minimap-idle",
"durationMs": 2002.6860000000397,
"styleRecalcs": 5,
"styleRecalcDurationMs": 4.598000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 545.534,
"heapDeltaBytes": 6308720,
"heapUsedBytes": 60189764,
"domNodes": 10,
"jsHeapTotalBytes": 786432,
"scriptDurationMs": 95.92500000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "minimap-idle",
"durationMs": 2002.016000000026,
"styleRecalcs": 10,
"styleRecalcDurationMs": 9.053000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 545.2060000000001,
"heapDeltaBytes": -7951808,
"heapUsedBytes": 64355076,
"domNodes": 20,
"jsHeapTotalBytes": 11415552,
"scriptDurationMs": 91.09500000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 579.7460000000001,
"styleRecalcs": 47,
"styleRecalcDurationMs": 10.916,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 373.595,
"heapDeltaBytes": 7616660,
"heapUsedBytes": 66444888,
"domNodes": 20,
"jsHeapTotalBytes": 19922944,
"scriptDurationMs": 122.291,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 553.9589999999635,
"styleRecalcs": 48,
"styleRecalcDurationMs": 11.184000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 362.61199999999997,
"heapDeltaBytes": 7281872,
"heapUsedBytes": 66222856,
"domNodes": 22,
"jsHeapTotalBytes": 18087936,
"scriptDurationMs": 122.94699999999999,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "subgraph-idle",
"durationMs": 2051.7329999999847,
"styleRecalcs": 9,
"styleRecalcDurationMs": 11.621,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 487.208,
"heapDeltaBytes": 16108324,
"heapUsedBytes": 70801688,
"domNodes": -320,
"jsHeapTotalBytes": 15380480,
"scriptDurationMs": 17.013,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-idle",
"durationMs": 2026.7049999999927,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.552000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 371.614,
"heapDeltaBytes": -2435452,
"heapUsedBytes": 56637528,
"domNodes": 20,
"jsHeapTotalBytes": 25427968,
"scriptDurationMs": 19.814,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1705.5890000000034,
"styleRecalcs": 77,
"styleRecalcDurationMs": 34.995,
"layouts": 16,
"layoutDurationMs": 4.361000000000001,
"taskDurationMs": 695.0849999999999,
"heapDeltaBytes": -10780304,
"heapUsedBytes": 48285760,
"domNodes": 65,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 84.782,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1691.7440000000283,
"styleRecalcs": 76,
"styleRecalcDurationMs": 35.867000000000004,
"layouts": 16,
"layoutDurationMs": 4.086,
"taskDurationMs": 684.1440000000001,
"heapDeltaBytes": -11323916,
"heapUsedBytes": 47591648,
"domNodes": 62,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 86.77499999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 948.3589999999822,
"styleRecalcs": 17,
"styleRecalcDurationMs": 25.054999999999993,
"layouts": 4,
"layoutDurationMs": 11.822,
"taskDurationMs": 713.9710000000001,
"heapDeltaBytes": 4388132,
"heapUsedBytes": 79826672,
"domNodes": 13833,
"jsHeapTotalBytes": 17825792,
"scriptDurationMs": 28.695999999999998,
"eventListeners": 2527,
"totalBlockingTimeMs": 153,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8173.467999999957,
"styleRecalcs": 251,
"styleRecalcDurationMs": 57.861,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3848.8,
"heapDeltaBytes": 12693340,
"heapUsedBytes": 75297496,
"domNodes": -260,
"jsHeapTotalBytes": 6250496,
"scriptDurationMs": 1229.8870000000002,
"eventListeners": -114,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8168.706000000043,
"styleRecalcs": 252,
"styleRecalcDurationMs": 58.06400000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4100.1539999999995,
"heapDeltaBytes": 2353540,
"heapUsedBytes": 72745884,
"domNodes": 22,
"jsHeapTotalBytes": 22339584,
"scriptDurationMs": 1385.8110000000001,
"eventListeners": 20,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "vue-large-graph-idle",
"durationMs": 11124.616999999944,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 11108.032,
"heapDeltaBytes": -21918112,
"heapUsedBytes": 169897336,
"domNodes": -3302,
"jsHeapTotalBytes": 19922944,
"scriptDurationMs": 539.626,
"eventListeners": -16467,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333326,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 11847.328000000061,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 11830.583999999999,
"heapDeltaBytes": -36559664,
"heapUsedBytes": 170343600,
"domNodes": -3308,
"jsHeapTotalBytes": 22515712,
"scriptDurationMs": 549.67,
"eventListeners": -16474,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.220000000000073,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 14410.539000000028,
"styleRecalcs": 66,
"styleRecalcDurationMs": 19.712000000000007,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14389.104,
"heapDeltaBytes": -55354144,
"heapUsedBytes": 154891192,
"domNodes": -3308,
"jsHeapTotalBytes": 15437824,
"scriptDurationMs": 910.1109999999999,
"eventListeners": -16470,
"totalBlockingTimeMs": 69,
"frameDurationMs": 17.219999999999953,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 13368.910000000029,
"styleRecalcs": 65,
"styleRecalcDurationMs": 19.95099999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 13348.078999999998,
"heapDeltaBytes": -33280172,
"heapUsedBytes": 170612916,
"domNodes": -3306,
"jsHeapTotalBytes": 19632128,
"scriptDurationMs": 940.0090000000001,
"eventListeners": -16470,
"totalBlockingTimeMs": 29,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 451.0200000000282,
"styleRecalcs": 20,
"styleRecalcDurationMs": 28.351000000000003,
"layouts": 6,
"layoutDurationMs": 2.02,
"taskDurationMs": 134.957,
"heapDeltaBytes": 5474692,
"heapUsedBytes": 65594264,
"domNodes": 170,
"jsHeapTotalBytes": 3407872,
"scriptDurationMs": 21.540999999999997,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "workflow-execution",
"durationMs": 453.9839999999913,
"styleRecalcs": 18,
"styleRecalcDurationMs": 24.715,
"layouts": 6,
"layoutDurationMs": 1.6090000000000002,
"taskDurationMs": 122.89399999999999,
"heapDeltaBytes": 5248064,
"heapUsedBytes": 65335612,
"domNodes": 157,
"jsHeapTotalBytes": 3407872,
"scriptDurationMs": 21.426000000000002,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
}
]
} |
Collaborator
Author
|
Blocker status — 2026-06-18 BE-786 ("assets always-on", the merge gate for this PR) is confirmed still required — not done elsewhere (Matt checked: Slack). 3 of its 4 blockers are now cleared; the only one left is BE-933 (Core: emit Until then the red e2e is expected: it is the temporary |
| icon: 'pi pi-database', | ||
| label: () => | ||
| `Experimental: ${ | ||
| useSettingStore().get('Comfy.Assets.UseAssetAPI') |
Contributor
There was a problem hiding this comment.
issue: (non-blocking) Any user with a persisted keybinding to Comfy.ToggleAssetAPI will hit an unhandled rejection on keypress -- commandStore.execute throws "Command not found" and the keybinding service has no try/catch around execute(). A silent no-op would be preferable; consider whether the keybinding service should guard missing commands gracefully, or add a one-time startup migration that drops orphaned bindings pointing to unregistered command IDs.
Collaborator
Author
There was a problem hiding this comment.
Confirmed the path: keybindHandler is attached via useEventListener(window, "keydown", ...) in GraphView.vue, and commandStore.execute throws Command ... not found for an unregistered id, so a floating rejection is possible. Scope is narrow though — no default keybinding ever pointed at Comfy.ToggleAssetAPI (KeyM maps to Workspace.ToggleSidebarTab.model-library), so only users who manually bound a key are affected.
The clean fix is a graceful guard in the keybinding service (skip + warn on unregistered command ids), which also covers future removed commands and orphaned extension bindings. That touches shared keybinding infra beyond this delete-only PR, so I'll handle it as a separate follow-up rather than widen this one.
Collaborator
Author
There was a problem hiding this comment.
Tracked as FE-1100 — the keybinding service will skip unregistered command ids gracefully (warn + no-op), covering this removal and any future ones plus orphaned extension bindings. https://linear.app/comfyorg/issue/FE-1100
|
|
||
| if ( | ||
| tab.id === 'model-library' && | ||
| settingStore.get('Comfy.Assets.UseAssetAPI') |
Contributor
There was a problem hiding this comment.
suggestion: (non-blocking) Previously, if Comfy.Assets.UseAssetAPI was false the if-block was skipped and normal tab toggle behavior continued. Now if (tab.id === 'model-library') always fires and ?.function?.() silently no-ops if Comfy.BrowseModelAssets is not yet registered (load-ordering race). The tab becomes a dead click with no feedback. Worth adding a fallback or asserting the command exists before relying on optional chaining.
Collaborator
Author
There was a problem hiding this comment.
Fixed in bc5d7a3. If Comfy.BrowseModelAssets isn't registered we now fall back to toggleSidebarTab(tab.id) instead of a silent no-op, so the click is never dead. Added regression coverage in sidebarTabStore.test.ts: command-present opens the browser; command-absent toggles the tab.
| cd /ComfyUI && python3 main.py --cpu --multi-user --front-end-root "${{ inputs.front_end_root }}" & | ||
| # TODO(FE-729): remove --enable-assets once BE-786 lands in the CI ComfyUI image | ||
| # (BE-786 removes the gate so /api/assets is always on). Until then, FE-729 | ||
| # routes modelStore through assetService, which 503s without this flag. |
Contributor
There was a problem hiding this comment.
note: The TODO comment is clear, but there is no machine enforcement that catches if --enable-assets survives into a non-draft merge. Consider a CI lint step (a grep gate) or a note in the BE-786 ticket so the revert is verified automatically rather than relying on reviewer memory.
Collaborator
Author
There was a problem hiding this comment.
Agreed it shouldn't rely on reviewer memory. The --enable-assets line lives in commit e48dcd1, which is itself reverted before this PR leaves draft-gating on BE-786 (PR checklist item). I'll add a note on BE-786 so the CI image flip and this revert are verified together.
A standing grep gate isn't durable here since the whole temp commit is removed at merge, but I can add a transient CI check if you'd prefer belt-and-suspenders.
Collaborator
Author
There was a problem hiding this comment.
Posted the coordination note on BE-786 so the CI image flip and the e48dcd11f revert get verified together when it lands.
| @@ -1,236 +0,0 @@ | |||
| import { expect } from '@playwright/test' | |||
Contributor
There was a problem hiding this comment.
issue: (non-blocking) All 5 describe blocks covering the model-library sidebar (open/close, folder expand, search, refresh, empty state -- roughly 15 test cases) are removed and the tab now opens the Asset Browser dialog. No equivalent E2E tests for that dialog appear in this PR. Is Asset Browser dialog coverage tracked as a follow-up (FE-732?), or is there an existing suite I'm missing?
Collaborator
Author
There was a problem hiding this comment.
Those blocks exercised the legacy model-library sidebar panel, which no longer renders — the tab now opens the Asset Browser dialog. The dialog's content/fetch and its E2E are the next layers: FE-730 (store fetcher) and FE-732 (dialog content). They're out of this routing-only PR, and on OSS the dialog data path is gated on BE-786, so there's no green dialog suite to add here yet. New-behavior coverage is owned by FE-732.
| @@ -27,7 +27,6 @@ test.describe('Default Keybindings', { tag: '@keyboard' }, () => { | |||
| const sidebarTabs = [ | |||
Contributor
There was a problem hiding this comment.
question: The KeyM -> model-library keybinding test entry is removed, but I don't see the keybinding definition file in the diff. Does the keybinding still exist with changed behavior (in which case it should be re-tested under the new behavior), or was the binding itself removed entirely?
Collaborator
Author
There was a problem hiding this comment.
The binding still exists — defaults.ts keeps KeyM -> Workspace.ToggleSidebarTab.model-library. What changed is its behavior: that command now opens the Asset Browser dialog instead of toggling the sidebar panel. I dropped the entry from this data-driven Sidebar Toggle Shortcuts test because its assertion (.model-library-tab-button.side-bar-button-selected shows/hides) no longer holds — there's no selectable sidebar button to toggle. A test for the new behavior (KeyM opens the dialog) belongs with the dialog work in FE-732, same as the modelLibrary.spec.ts removal above.
| const node = this.comfyPage.vueNodes.getNodeLocator(String(nodeRef.id)) | ||
| // Grid-mode widgets (WidgetSelectDefault) and number widgets expose | ||
| // aria-label on a wrapper/input. Asset-mode widgets (WidgetSelectDropdown) | ||
| // do not — the widget name lives in a sibling |
Contributor
There was a problem hiding this comment.
suggestion: (non-blocking) byAriaLabel.count() resolves against the current DOM immediately with no wait -- if the widget is still mounting it returns 0 falsely and routes to the fallback even when the aria-label element is about to appear. Consider waitFor({ state: 'attached' }) wrapped in a try/catch before branching.
Collaborator
Author
There was a problem hiding this comment.
Valid — count() is a point-in-time read. The preceding await nodeRef.centerOnNode() settles a frame first so the window is small, but not airtight. I held off on a blanket waitFor({ state: "attached" }) because asset-mode widgets legitimately have no aria-label, so every asset-widget selection would eat the full timeout on the fallback branch.
The durable fix is the data-testid on the dropdown trigger you suggest in the next two comments — that removes the aria-label/fallback branch and the race with it. Tracking that as a follow-up; happy to drop in a short-timeout waitFor here in the meantime if you'd rather.
| (await byAriaLabel.count()) > 0 | ||
| ? byAriaLabel | ||
| : node | ||
| .getByTestId('widget-layout-field-label') |
Contributor
There was a problem hiding this comment.
nitpick: (non-blocking) .locator('button').first() picks by DOM order. If the widget row has multiple buttons (clear, expand, trigger), this may click the wrong one. A data-testid on the dropdown trigger button would be more reliable than positional selection.
Collaborator
Author
There was a problem hiding this comment.
Agreed — positional .first() is fragile. A data-testid on the dropdown trigger is the right call; it supersedes both this positional select and the .. traversal below. Tracking as a follow-up with the widget-helper cleanup.
| const widgetLocator = | ||
| (await byAriaLabel.count()) > 0 | ||
| ? byAriaLabel | ||
| : node |
Contributor
There was a problem hiding this comment.
nitpick: (non-blocking) locator('..') is XPath parent-axis traversal -- Playwright docs discourage it since any added wrapper div silently breaks the path. A shared data-testid on the row ancestor would be more maintainable.
Collaborator
Author
There was a problem hiding this comment.
Agreed — XPath parent-axis is brittle to wrapper changes. Same fix as the previous comment: a data-testid on the row/trigger removes the .. hop. Folding both into the follow-up.
Contributor
|
suggestion: (non-blocking) The locale files (e.g. |
…istered (FE-729) If Comfy.BrowseModelAssets is not yet registered (load-ordering race), the model-library tab click was a silent no-op via optional chaining. Fall back to toggleSidebarTab so the click is never dead, and add regression coverage.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dante01yoon
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
github-actions
Bot
removed
the
New Browser Test Expectations
|
Deployment failed with the following error: Learn More: https://vercel.com/uy-tieu-s-projects?upgradeToPro=build-rate-limit |
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
3 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.
Summary
FE-729 (L1.2 — L1 central). Asset API will be always available post-BE-786 (
--enable-assetsremoved from OSS). Delete the gating function, the user-facing setting, the experimental warning, and the toggle command. Simplify caller sites to assume the asset API is available.Draft — do not merge until BE-786 lands. This PR assumes OSS asset routes are unconditionally registered (which is BE-786's deliverable).
Scope clarification
This PR is the routing-layer half of bringing the Asset Browser dialog to OSS:
sidebarTabStore.ts:81) — Model Library sidebar tab → Asset Browser dialog (always, not gated on a setting)assetService.ts:385) —isAssetAPIEnabled()deleted;shouldUseAssetBrowsersimplifiedComfy.Assets.UseAssetAPIsetting +Comfy.ToggleAssetAPIcommand +EXPERIMENTAL_WARNINGremovedassetsStore.ts:121, 324Cloud-OSS fork): FE-730 (L1.3)After this PR + BE-786 land, an OSS user sees the dialog when they click the Model Library sidebar tab, but model content will not be correct until FE-730 + FE-732 + BE-933 also merge. See the scope clarification comment on FE-729 for the full breakdown.
This PR maps directly to the Notion v3 Setting retirement plan (sites #2–#5 +
EXPERIMENTAL_WARNINGrewording) — see Asset FE Divergence Survey M1 Scope.What OSS users gain immediately after BE-786 + this PR
Once OSS asset routes are unconditionally registered (BE-786) and this PR merges, OSS users see — without any opt-in — the same baseline asset surface Cloud has today:
GET /api/assets)GET /api/assets/{id})PUT /api/assets/{id},DELETE /api/assets/{id})POST /api/assets)POST/DELETE /api/assets/{id}/tags)/api/assets/seed), prune, content streaming for local filesCloud-specific features remain absent on OSS and are not delivered by this PR:
/api/assets/download,/api/assets/remote-metadata,/api/assets/import-url; OSS has skeleton only/api/assets/export; OSS lacks the task system for backgrounded archivingThese three Cloud-only capabilities are described in Notion v3 §L2 as
asset_url_import_enabled,asset_bulk_export_enabled,asset_model_wizard_enabledand tracked separately — they depend on additional BE work outside this PR's scope.Changes
Two commits:
1.
7f6d354a8— delete isAssetAPIEnabled + Comfy.Assets.UseAssetAPIisAssetAPIEnabled()fromassetService.ts; simplifyshouldUseAssetBrowser()to callisAssetBrowserEligible()directly.EXPERIMENTAL_WARNINGprefix from four asset-load error messages.Comfy.Assets.UseAssetAPIsetting (coreSettings.ts,apiSchema.ts).Comfy.ToggleAssetAPIcommand outright. KeepComfy.BrowseModelAssetsbut drop its setting-check / setting-set preamble and theExperimental:label prefix; the command now just opens the asset browser dialog.modelStore,sidebarTabStore,assetPreviewUtil,WidgetSelect.vuecaller-site simplifications.assetService.test.tsdrops the two now-meaningless cloud/setting branch cases.2.
b34026527— remove isAssetPreviewSupported wrapper + simplify callersisAssetPreviewSupported()fromassetPreviewUtil.ts(was areturn truewrapper after commit 1).Media3DTop.vue,saveMesh.ts(two blocks),FormDropdownMenuItem.vue,useLoad3d.ts.Auto-fixed unrelated tailwind class-order lint errors in several files via
pnpm lint:fixacross both commits to keep CI green.Review Focus
--enable-assets, noasset_api_enabledflag.Comfy.BrowseModelAssetsis not fully removed: it survives as the entry point thatsidebarTabStoreinvokes when the model-library sidebar tab is clicked, and as a Command Palette entry. Removing it would require inlining the dialog-open logic intosidebarTabStore(couples a store to UI logic). Kept as a small command.useCoreCommands.ts:1287and:1330-1331are the setting-check lines named in the FE-729 description.isAssetPreviewSupportedcleanup (commit 2) is follow-up tightening that was originally listed as a follow-up PR; rolled into this PR since the wrapper was newly dead and the caller surface was small.Done when (status)
isAssetAPIEnabled()function deletedComfy.Assets.UseAssetAPIsetting + schema entry removedComfy.ToggleAssetAPIcommand removed;Comfy.BrowseModelAssetsbody simplifiedEXPERIMENTAL_WARNINGremoved + 4 caller-site prefixesisAssetAPIEnabled/Comfy.Assets.UseAssetAPIcaller sites simplifiede48dcd11f(--enable-assetsin CI action) before mergeDependencies
--enable-assetsremoval. Asset routes must be unconditionally registered before this PR can merge.Refs
Screenshots (if applicable)
User-visible behavior changes after BE-786 + this PR merge:
Browse Model Assetscommand label no longer says "Experimental:".Comfy.Assets.UseAssetAPIhidden setting and theComfy.ToggleAssetAPIcommand are no longer present in the command palette.See attached cloud-side verification screenshot in the comment thread.