mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
fix: use cloud assets for asset widget default value (#10983)
## Summary In cloud mode, asset-supported nodes (e.g. CheckpointLoaderSimple) used the server's `object_info` combo options as their default widget value. These options list local files on the backend which may not exist in the user's cloud asset library. When the missing-model pipeline runs (on undo, reload, or tab switch), it checks widget values against cloud assets and correctly flags these local-only files as missing — producing errors that appear to be false positives but are actually valid detections of unusable defaults. This PR changes the default value source from server combo options to the cloud assets store. ## Default Value Behavior (Before → After) ### Cloud + asset-supported widgets (changed) | Condition | Before | After | |-----------|--------|-------| | Assets cached, `inputSpec.default` in assets | `inputSpec.default` | `inputSpec.default` | | Assets cached, `inputSpec.default` not in assets | `inputSpec.default` | `assets[0]` | | Assets cached, no `inputSpec.default`, `options` exist | `options[0]` | `assets[0]` | | Assets not cached, `inputSpec.default` exists | `inputSpec.default` | `undefined` → "Select model" | | Assets not cached, no `inputSpec.default`, `options` exist | `options[0]` | `undefined` → "Select model" | | Assets not cached, no `inputSpec.default`, no `options` | `undefined` → "Select model" | `undefined` → "Select model" | ### Cloud + non-asset widgets (unchanged) | Condition | Behavior | |-----------|----------| | `inputSpec.default` exists | `inputSpec.default` | | `options` exist | `options[0]` | | `remote` input | `"Loading..."` | | None | `undefined` | ### OSS (unchanged) | Condition | Behavior | |-----------|----------| | `inputSpec.default` exists | `inputSpec.default` | | `options` exist | `options[0]` | | `remote` input | `"Loading..."` | | None | `undefined` | ## Root Cause 1. `addComboWidget` called `getDefaultValue(inputSpec)` which returns `inputSpec.options[0]` — a local file from `object_info` 2. In cloud mode, `shouldUseAssetBrowser()` creates an asset widget with this local filename as default 3. The model (e.g. `dynamicrafter/controlnet/dc-sketch_encoder_fp16.safetensors`) exists on the server but not in the user's cloud asset library 4. On undo/reload, `verifyAssetSupportedCandidates()` checks the widget value against cloud assets → not found → marked as missing ## Changes ### Production (`useComboWidget.ts`) - New `resolveCloudDefault(nodeType, specDefault)` function encapsulates cloud default resolution - Default priority: `inputSpec.default` (if found in cloud assets) → first cloud asset → `undefined` (shows "Select model" placeholder) - Edge case guards: `!= null` check for falsy defaults, `|| undefined` for empty `getAssetFilename` return - Server combo options (`object_info`) are no longer used as defaults for asset widgets ### Unit Tests (`useComboWidget.test.ts`) - 6 scenarios covering all default value paths: - Cloud assets loaded, no `inputSpec.default` → `assets[0]` - Cloud assets loaded, `inputSpec.default` in assets → uses default - Cloud assets loaded, `inputSpec.default` not in assets → `assets[0]` - No cloud assets, with `inputSpec.default` → placeholder - No cloud assets, with server options → placeholder - Asset widget creation verification - Test helper refactored: assertions moved from helper to each test for clarity ### E2E Test (`cloud-asset-default.spec.ts`) - New `@cloud` tagged test verifying CheckpointLoaderSimple uses first cloud asset, not server default - Fixture extension stubs `/api/assets` before app loads (local backend returns 503 for this endpoint) - Uses typed mock data from existing `assetFixtures.ts` ## Scope - **Cloud only**: All changes gated behind `isCloud` + `shouldUseAssetBrowser()` - **OSS impact**: None — code path is not entered in non-cloud builds - **Breaking changes**: None — `useComboWidget` export signature unchanged ## Review Focus - Should the `/api/assets` stub in the E2E fixture extension be moved into `ComfyPage` for all `@cloud` tests? ## Record Before https://github.com/user-attachments/assets/994162a0-b56a-4e84-9b1c-d0f0068196d5 After https://github.com/user-attachments/assets/ba299990-9bd3-4565-bd09-bffac3db60a9
This commit is contained in:
82
browser_tests/tests/cloud-asset-default.spec.ts
Normal file
82
browser_tests/tests/cloud-asset-default.spec.ts
Normal file
@@ -0,0 +1,82 @@
|
||||
import { expect } from '@playwright/test'
|
||||
|
||||
import type { Asset, ListAssetsResponse } from '@comfyorg/ingest-types'
|
||||
import { comfyPageFixture } from '@e2e/fixtures/ComfyPage'
|
||||
import {
|
||||
STABLE_CHECKPOINT,
|
||||
STABLE_CHECKPOINT_2
|
||||
} from '@e2e/fixtures/data/assetFixtures'
|
||||
|
||||
function makeAssetsResponse(assets: Asset[]): ListAssetsResponse {
|
||||
return { assets, total: assets.length, has_more: false }
|
||||
}
|
||||
|
||||
const CLOUD_ASSETS: Asset[] = [STABLE_CHECKPOINT, STABLE_CHECKPOINT_2]
|
||||
|
||||
// Stub /api/assets before the app loads. The local ComfyUI backend has no
|
||||
// /api/assets endpoint (returns 503), which poisons the assets store on
|
||||
// first load. Narrow pattern avoids intercepting static /assets/*.js bundles.
|
||||
//
|
||||
// TODO: Consider moving this stub into ComfyPage fixture for all @cloud tests.
|
||||
const test = comfyPageFixture.extend<{ stubCloudAssets: void }>({
|
||||
stubCloudAssets: [
|
||||
async ({ page }, use) => {
|
||||
const pattern = '**/api/assets?*'
|
||||
await page.route(pattern, (route) =>
|
||||
route.fulfill({
|
||||
status: 200,
|
||||
contentType: 'application/json',
|
||||
body: JSON.stringify(makeAssetsResponse(CLOUD_ASSETS))
|
||||
})
|
||||
)
|
||||
await use()
|
||||
await page.unroute(pattern)
|
||||
},
|
||||
{ auto: true }
|
||||
]
|
||||
})
|
||||
|
||||
test.describe('Asset-supported node default value', { tag: '@cloud' }, () => {
|
||||
test.afterEach(async ({ comfyPage }) => {
|
||||
await comfyPage.nodeOps.clearGraph()
|
||||
})
|
||||
|
||||
test('should use first cloud asset when server default is not in assets', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
// The default workflow contains a CheckpointLoaderSimple node whose
|
||||
// server default (from object_info) is a local file not in cloud assets.
|
||||
// Wait for the existing node's asset widget to mount, confirming the
|
||||
// assets store has been populated from the stub before adding a new node.
|
||||
await expect
|
||||
.poll(
|
||||
() =>
|
||||
comfyPage.page.evaluate(() => {
|
||||
const node = window.app!.graph.nodes.find(
|
||||
(n: { type: string }) => n.type === 'CheckpointLoaderSimple'
|
||||
)
|
||||
return node?.widgets?.find(
|
||||
(w: { name: string }) => w.name === 'ckpt_name'
|
||||
)?.type
|
||||
}),
|
||||
{ timeout: 10_000 }
|
||||
)
|
||||
.toBe('asset')
|
||||
|
||||
// Add a new CheckpointLoaderSimple — should use first cloud asset,
|
||||
// not the server's object_info default.
|
||||
const widgetValue = await comfyPage.page.evaluate(() => {
|
||||
const node = window.LiteGraph!.createNode('CheckpointLoaderSimple')
|
||||
window.app!.graph.add(node!)
|
||||
const widget = node!.widgets?.find(
|
||||
(w: { name: string }) => w.name === 'ckpt_name'
|
||||
)
|
||||
return String(widget?.value ?? '')
|
||||
})
|
||||
|
||||
// Production resolves via getAssetFilename (user_metadata.filename →
|
||||
// metadata.filename → asset.name). Test fixtures have no metadata
|
||||
// filename, so asset.name is the resolved value.
|
||||
expect(widgetValue).toBe(CLOUD_ASSETS[0].name)
|
||||
})
|
||||
})
|
||||
@@ -28,6 +28,7 @@ function createMockAssetItem(overrides: Partial<AssetItem> = {}): AssetItem {
|
||||
const mockDistributionState = vi.hoisted(() => ({ isCloud: false }))
|
||||
const mockUpdateInputs = vi.hoisted(() => vi.fn(() => Promise.resolve()))
|
||||
const mockGetInputName = vi.hoisted(() => vi.fn((hash: string) => hash))
|
||||
const mockGetAssets = vi.hoisted(() => vi.fn(() => [] as AssetItem[]))
|
||||
const mockAssetsStoreState = vi.hoisted(() => {
|
||||
const inputAssets: AssetItem[] = []
|
||||
return {
|
||||
@@ -55,7 +56,8 @@ vi.mock('@/stores/assetsStore', () => ({
|
||||
return mockAssetsStoreState.inputLoading
|
||||
},
|
||||
updateInputs: mockUpdateInputs,
|
||||
getInputName: mockGetInputName
|
||||
getInputName: mockGetInputName,
|
||||
getAssets: mockGetAssets
|
||||
}))
|
||||
}))
|
||||
|
||||
@@ -199,67 +201,117 @@ describe('useComboWidget', () => {
|
||||
expect(widget).toBe(mockWidget)
|
||||
})
|
||||
|
||||
it('should create asset browser widget when API enabled', () => {
|
||||
mockDistributionState.isCloud = true
|
||||
vi.mocked(assetService.shouldUseAssetBrowser).mockReturnValue(true)
|
||||
describe('cloud asset browser widget', () => {
|
||||
// "Select model" is the fallback from t('widgets.selectModel')
|
||||
// in createAssetWidget when defaultValue is undefined.
|
||||
const PLACEHOLDER = 'Select model'
|
||||
|
||||
const constructor = useComboWidget()
|
||||
const mockWidget = createMockWidget({
|
||||
type: 'asset',
|
||||
name: 'ckpt_name',
|
||||
value: 'model1.safetensors'
|
||||
})
|
||||
const mockNode = createMockNode('CheckpointLoaderSimple')
|
||||
vi.mocked(mockNode.addWidget).mockReturnValue(mockWidget)
|
||||
const inputSpec = createMockInputSpec({
|
||||
name: 'ckpt_name',
|
||||
options: ['model1.safetensors', 'model2.safetensors']
|
||||
function setupCloudAssetWidget(
|
||||
inputSpecOverrides: Partial<InputSpec> = {}
|
||||
) {
|
||||
mockDistributionState.isCloud = true
|
||||
vi.mocked(assetService.shouldUseAssetBrowser).mockReturnValue(true)
|
||||
|
||||
const constructor = useComboWidget()
|
||||
const mockWidget = createMockWidget({
|
||||
type: 'asset',
|
||||
name: 'ckpt_name',
|
||||
value: ''
|
||||
})
|
||||
const mockNode = createMockNode('CheckpointLoaderSimple')
|
||||
vi.mocked(mockNode.addWidget).mockReturnValue(mockWidget)
|
||||
const inputSpec = createMockInputSpec({
|
||||
name: 'ckpt_name',
|
||||
...inputSpecOverrides
|
||||
})
|
||||
|
||||
constructor(mockNode, inputSpec)
|
||||
return { mockNode }
|
||||
}
|
||||
|
||||
function getWidgetDefault(mockNode: ReturnType<typeof createMockNode>) {
|
||||
return vi.mocked(mockNode.addWidget).mock.calls[0]?.[2]
|
||||
}
|
||||
|
||||
it('should create asset browser widget when API enabled', () => {
|
||||
mockGetAssets.mockReturnValue([
|
||||
createMockAssetItem({ name: 'cloud_model.safetensors' })
|
||||
])
|
||||
|
||||
const { mockNode } = setupCloudAssetWidget({
|
||||
options: ['model1.safetensors', 'model2.safetensors']
|
||||
})
|
||||
|
||||
expect(
|
||||
vi.mocked(assetService.shouldUseAssetBrowser)
|
||||
).toHaveBeenCalledWith('CheckpointLoaderSimple', 'ckpt_name')
|
||||
expect(mockNode.addWidget).toHaveBeenCalledWith(
|
||||
'asset',
|
||||
'ckpt_name',
|
||||
expect.anything(),
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
})
|
||||
|
||||
const widget = constructor(mockNode, inputSpec)
|
||||
it('should use first cloud asset as default instead of server combo options', () => {
|
||||
mockGetAssets.mockReturnValue([
|
||||
createMockAssetItem({ name: 'cloud_model.safetensors' })
|
||||
])
|
||||
|
||||
expect(mockNode.addWidget).toHaveBeenCalledWith(
|
||||
'asset',
|
||||
'ckpt_name',
|
||||
'model1.safetensors',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(vi.mocked(assetService.shouldUseAssetBrowser)).toHaveBeenCalledWith(
|
||||
'CheckpointLoaderSimple',
|
||||
'ckpt_name'
|
||||
)
|
||||
expect(widget).toBe(mockWidget)
|
||||
})
|
||||
const { mockNode } = setupCloudAssetWidget({
|
||||
options: ['local_only_model.safetensors']
|
||||
})
|
||||
|
||||
it('should create asset browser widget when default value provided without options', () => {
|
||||
mockDistributionState.isCloud = true
|
||||
vi.mocked(assetService.shouldUseAssetBrowser).mockReturnValue(true)
|
||||
|
||||
const constructor = useComboWidget()
|
||||
const mockWidget = createMockWidget({
|
||||
type: 'asset',
|
||||
name: 'ckpt_name',
|
||||
value: 'fallback.safetensors'
|
||||
})
|
||||
const mockNode = createMockNode('CheckpointLoaderSimple')
|
||||
vi.mocked(mockNode.addWidget).mockReturnValue(mockWidget)
|
||||
const inputSpec = createMockInputSpec({
|
||||
name: 'ckpt_name',
|
||||
default: 'fallback.safetensors'
|
||||
// Note: no options array provided
|
||||
expect(getWidgetDefault(mockNode)).toBe('cloud_model.safetensors')
|
||||
})
|
||||
|
||||
const widget = constructor(mockNode, inputSpec)
|
||||
it('should fallback to assets[0] when inputSpec.default not in cloud assets', () => {
|
||||
mockGetAssets.mockReturnValue([
|
||||
createMockAssetItem({ name: 'cloud_model.safetensors' })
|
||||
])
|
||||
|
||||
expect(mockNode.addWidget).toHaveBeenCalledWith(
|
||||
'asset',
|
||||
'ckpt_name',
|
||||
'fallback.safetensors',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(widget).toBe(mockWidget)
|
||||
const { mockNode } = setupCloudAssetWidget({
|
||||
default: 'not_in_cloud.safetensors'
|
||||
})
|
||||
|
||||
expect(getWidgetDefault(mockNode)).toBe('cloud_model.safetensors')
|
||||
})
|
||||
|
||||
it('should prefer inputSpec.default when it exists in cloud assets', () => {
|
||||
mockGetAssets.mockReturnValue([
|
||||
createMockAssetItem({ name: 'other_model.safetensors' }),
|
||||
createMockAssetItem({ name: 'fallback.safetensors' })
|
||||
])
|
||||
|
||||
const { mockNode } = setupCloudAssetWidget({
|
||||
// Note: no options array provided
|
||||
default: 'fallback.safetensors'
|
||||
})
|
||||
|
||||
expect(getWidgetDefault(mockNode)).toBe('fallback.safetensors')
|
||||
})
|
||||
|
||||
it('should create asset browser widget when default value provided without options', () => {
|
||||
mockGetAssets.mockReturnValue([])
|
||||
|
||||
const { mockNode } = setupCloudAssetWidget({
|
||||
// Note: no options array provided
|
||||
default: 'fallback.safetensors'
|
||||
})
|
||||
|
||||
expect(getWidgetDefault(mockNode)).toBe(PLACEHOLDER)
|
||||
})
|
||||
|
||||
it('should fallback to placeholder when cloud assets not loaded', () => {
|
||||
mockGetAssets.mockReturnValue([])
|
||||
|
||||
const { mockNode } = setupCloudAssetWidget({
|
||||
options: ['local_model.safetensors']
|
||||
})
|
||||
|
||||
expect(getWidgetDefault(mockNode)).toBe(PLACEHOLDER)
|
||||
})
|
||||
})
|
||||
|
||||
it('should show Select model when asset widget has undefined current value', () => {
|
||||
|
||||
@@ -6,6 +6,7 @@ import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
import { isComboWidget } from '@/lib/litegraph/src/litegraph'
|
||||
import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets'
|
||||
import { assetService } from '@/platform/assets/services/assetService'
|
||||
import { getAssetFilename } from '@/platform/assets/utils/assetMetadataUtils'
|
||||
import { createAssetWidget } from '@/platform/assets/utils/createAssetWidget'
|
||||
import { isCloud } from '@/platform/distribution/types'
|
||||
import type {
|
||||
@@ -104,6 +105,25 @@ const addMultiSelectWidget = (
|
||||
return widget
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the default value for a cloud asset widget.
|
||||
* Priority: inputSpec.default (if present in cloud assets) → first cloud
|
||||
* asset → undefined (shows placeholder).
|
||||
*/
|
||||
function resolveCloudDefault(
|
||||
nodeType: string,
|
||||
specDefault: string | undefined
|
||||
): string | undefined {
|
||||
const assets = useAssetsStore().getAssets(nodeType)
|
||||
if (specDefault != null) {
|
||||
const inAssets = assets.some((a) => getAssetFilename(a) === specDefault)
|
||||
if (inAssets) return specDefault
|
||||
}
|
||||
// empty filename → undefined (shows placeholder)
|
||||
const filename = assets.length ? getAssetFilename(assets[0]) : undefined
|
||||
return filename || undefined
|
||||
}
|
||||
|
||||
function createAssetBrowserWidget(
|
||||
node: LGraphNode,
|
||||
inputSpec: ComboInputSpec,
|
||||
@@ -195,7 +215,14 @@ const addComboWidget = (
|
||||
|
||||
if (isCloud) {
|
||||
if (assetService.shouldUseAssetBrowser(node.comfyClass, inputSpec.name)) {
|
||||
return createAssetBrowserWidget(node, inputSpec, defaultValue)
|
||||
// Default from cloud assets, not from server combo options.
|
||||
// Server options list local files that may not exist in the user's
|
||||
// cloud asset library, leading to missing-model errors on undo/reload.
|
||||
const cloudDefault = resolveCloudDefault(
|
||||
node.comfyClass ?? '',
|
||||
inputSpec.default
|
||||
)
|
||||
return createAssetBrowserWidget(node, inputSpec, cloudDefault)
|
||||
}
|
||||
|
||||
if (NODE_MEDIA_TYPE_MAP[node.comfyClass ?? '']) {
|
||||
|
||||
Reference in New Issue
Block a user