Skip to content

refactor(assets): delete isAssetAPIEnabled and Comfy.Assets.UseAssetAPI (FE-729) #12322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dante01yoon wants to merge 19 commits into
base: main
Choose a base branch
from
Open

refactor(assets): delete isAssetAPIEnabled and Comfy.Assets.UseAssetAPI (FE-729) #12322

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7f6d354
refactor(assets): delete isAssetAPIEnabled and Comfy.Assets.UseAssetAPI
dante01yoon May 19, 2026
fcdc440
[automated] Apply ESLint and Oxfmt fixes
actions-user May 19, 2026
b340265
refactor(assets): remove isAssetPreviewSupported wrapper and simplify…
dante01yoon May 19, 2026
09943f8
[automated] Apply ESLint and Oxfmt fixes
actions-user May 19, 2026
e48dcd1
ci(temp): enable --enable-assets on Playwright ComfyUI server
dante01yoon May 19, 2026
ec0711d
Merge branch 'main' into jaewon/fe-729-delete-is-asset-api-enabled
dante01yoon May 19, 2026
ffe8d0f
test(assets): drop OSS legacy sidebar tree tests (FE-729)
dante01yoon May 19, 2026
0797b7a
test(builder): make selectInputWidget work in both grid and asset modes
dante01yoon May 20, 2026
c579c88
[automated] Apply ESLint and Oxfmt fixes
actions-user May 20, 2026
55b0329
test(builder): use exclusive fallback instead of .or() in selectInput…
dante01yoon May 21, 2026
a738f75
Merge branch 'main' into jaewon/fe-729-delete-is-asset-api-enabled
dante01yoon Jun 18, 2026
aaf2177
Merge remote-tracking branch 'origin/main' into HEAD
dante01yoon Jun 18, 2026
bc5d7a3
fix(assets): fall back to sidebar toggle when BrowseModelAssets unreg…
dante01yoon Jun 18, 2026
b0e0423
Merge branch 'main' into jaewon/fe-729-delete-is-asset-api-enabled
dante01yoon Jun 19, 2026
4066b2a
[automated] Update test expectations
invalid-email-address Jun 19, 2026
8e0970e
Merge branch 'main' into jaewon/fe-729-delete-is-asset-api-enabled
dante01yoon Jun 19, 2026
42fd381
fix(test): restore asset-mode touch-pan golden after main merge (FE-729)
dante01yoon Jun 19, 2026
f4ebb17
Merge branch 'main' into jaewon/fe-729-delete-is-asset-api-enabled
dante01yoon Jun 23, 2026
fc26526
[automated] Update test expectations
invalid-email-address Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/actions/start-comfyui-server/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,8 @@ runs:
run: |
set -euo pipefail
cp -r ./tools/devtools/* /ComfyUI/custom_nodes/ComfyUI_devtools/
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.

@christian-byrne christian-byrne Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted the coordination note on BE-786 so the CI image flip and the e48dcd11f revert get verified together when it lands.

cd /ComfyUI && python3 main.py --cpu --multi-user --enable-assets --front-end-root "${{ inputs.front_end_root }}" &
wait-for-it --service 127.0.0.1:8188 -t ${{ inputs.timeout }}
19 changes: 16 additions & 3 deletions browser_tests/fixtures/helpers/BuilderSelectHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,22 @@ export class BuilderSelectHelper {
)[0]
if (!nodeRef) throw new Error(`Node ${nodeTitle} not found`)
await nodeRef.centerOnNode()
const widgetLocator = this.comfyPage.vueNodes
.getNodeLocator(String(nodeRef.id))
.getByLabel(widgetName, { exact: true })
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

@christian-byrne christian-byrne Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// [data-testid="widget-layout-field-label"] div, so fall back to clicking
// the dropdown trigger button in the same row.
const byAriaLabel = node.getByLabel(widgetName, { exact: true })
const widgetLocator =
(await byAriaLabel.count()) > 0
? byAriaLabel
: node

@christian-byrne christian-byrne Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.getByTestId('widget-layout-field-label')

@christian-byrne christian-byrne Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.filter({ hasText: widgetName })
.locator('..')
.locator('button')
.first()
// oxlint-disable-next-line playwright/no-force-option -- Node container has conditional pointer-events:none that blocks actionability
await widgetLocator.click({ force: true })
await this.comfyPage.nextFrame()
Expand Down
1 change: 0 additions & 1 deletion browser_tests/tests/defaultKeybindings.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ test.describe('Default Keybindings', { tag: '@keyboard' }, () => {
const sidebarTabs = [

@christian-byrne christian-byrne Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{ key: 'KeyW', tabId: 'workflows', label: 'workflows' },
{ key: 'KeyN', tabId: 'node-library', label: 'node library' },
{ key: 'KeyM', tabId: 'model-library', label: 'model library' },
{ key: 'KeyA', tabId: 'assets', label: 'assets' }
] as const

Expand Down
37 changes: 0 additions & 37 deletions browser_tests/tests/propertiesPanel/errorsTabMissingModels.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,42 +133,5 @@ test.describe('Errors tab - Missing models', { tag: '@ui' }, () => {
comfyPage.page.getByTestId(TestIds.dialogs.missingModelRefresh)
).toBeVisible()
})

test('Should clear resolved missing model when Refresh is clicked', async ({
comfyPage
}) => {
await loadWorkflowAndOpenErrorsTab(comfyPage, 'missing/missing_models')
await comfyPage.page.route(/\/object_info$/, async (route) => {
const response = await route.fetch()
const objectInfo = await response.json()
const ckptName =
objectInfo.CheckpointLoaderSimple.input.required.ckpt_name
ckptName[0] = [...ckptName[0], 'fake_model.safetensors']
await route.fulfill({ response, json: objectInfo })
})

const objectInfoResponse = comfyPage.page.waitForResponse((response) => {
const url = new URL(response.url())
return url.pathname.endsWith('/object_info') && response.ok()
})
const modelFoldersResponse = comfyPage.page.waitForResponse(
(response) => {
const url = new URL(response.url())
return url.pathname.endsWith('/experiment/models') && response.ok()
}
)
const refreshButton = comfyPage.page.getByTestId(
TestIds.dialogs.missingModelRefresh
)

await Promise.all([
objectInfoResponse,
modelFoldersResponse,
refreshButton.click()
])
await expect(
comfyPage.page.getByTestId(TestIds.dialogs.missingModelsGroup)
).toBeHidden()
})
})
})
236 changes: 0 additions & 236 deletions browser_tests/tests/sidebar/modelLibrary.spec.ts
View file
Open in desktop

This file was deleted.

32 changes: 1 addition & 31 deletions src/composables/useCoreCommands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,23 +1283,9 @@ export function useCoreCommands(): ComfyCommand[] {
{
id: 'Comfy.BrowseModelAssets',
icon: 'pi pi-folder-open',
label: 'Experimental: Browse Model Assets',
label: 'Browse Model Assets',

@coderabbitai coderabbitai Bot May 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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'),
As per coding guidelines, "Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json".
🤖 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.

@dante01yoon dante01yoon May 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai coderabbitai Bot May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'`.

versionAdded: '1.28.3',
function: async () => {
if (!useSettingStore().get('Comfy.Assets.UseAssetAPI')) {
const confirmed = await dialogService.confirm({
title: 'Enable Asset API',
message:
'The Asset API is currently disabled. Would you like to enable it?',
type: 'default'
})

if (!confirmed) return

const settingStore = useSettingStore()
await settingStore.set('Comfy.Assets.UseAssetAPI', true)
await workflowService.reloadCurrentWorkflow()
}
const assetBrowserDialog = useAssetBrowserDialog()
await assetBrowserDialog.browse({
assetType: 'models',
Expand All @@ -1318,22 +1304,6 @@ export function useCoreCommands(): ComfyCommand[] {
})
}
},
{
id: 'Comfy.ToggleAssetAPI',
icon: 'pi pi-database',
label: () =>
`Experimental: ${
useSettingStore().get('Comfy.Assets.UseAssetAPI')

@christian-byrne christian-byrne Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dante01yoon dante01yoon Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

? 'Disable'
: 'Enable'
} AssetAPI`,
function: async () => {
const settingStore = useSettingStore()
const current = settingStore.get('Comfy.Assets.UseAssetAPI') ?? false
await settingStore.set('Comfy.Assets.UseAssetAPI', !current)
await useWorkflowService().reloadCurrentWorkflow() // ensure changes take effect immediately
}
},
{
id: 'Comfy.ToggleQPOV2',
icon: 'pi pi-list',
Expand Down
Loading
Loading