From 8f3bafdf310b2d65e5a3059ec8cee8e948010a3c Mon Sep 17 00:00:00 2001 From: Subagent 5 Date: Wed, 28 Jan 2026 23:56:01 -0800 Subject: [PATCH] refactor: extract createAssetWidget to shared factory, remove # privates - Extract createAssetWidget factory to src/platform/assets/utils/ - Refactor useComboWidget.ts to use the shared factory - Simplify PrimitiveNode to use shared factory - Convert JS # privates to underscore convention - Add knip ignore for isAssetWidget (litegraph public API) Amp-Thread-ID: https://ampcode.com/threads/T-019c0839-bbdc-754a-9d3b-151417058ded Co-authored-by: Amp --- src/extensions/core/widgetInputs.ts | 117 ++++++------------ src/lib/litegraph/src/litegraph.ts | 4 +- src/lib/litegraph/src/widgets/widgetMap.ts | 5 +- .../assets/utils/createAssetWidget.ts | 88 +++++++++++++ .../widgets/composables/useComboWidget.ts | 85 ++----------- todo.md | 34 +++++ 6 files changed, 180 insertions(+), 153 deletions(-) create mode 100644 src/platform/assets/utils/createAssetWidget.ts create mode 100644 todo.md diff --git a/src/extensions/core/widgetInputs.ts b/src/extensions/core/widgetInputs.ts index 0c15a9b7a..e440d145b 100644 --- a/src/extensions/core/widgetInputs.ts +++ b/src/extensions/core/widgetInputs.ts @@ -10,16 +10,10 @@ import { NodeSlot } from '@/lib/litegraph/src/node/NodeSlot' import type { CanvasPointerEvent } from '@/lib/litegraph/src/types/events' import type { IBaseWidget, - IWidgetAssetOptions, TWidgetValue } from '@/lib/litegraph/src/types/widgets' -import { useAssetBrowserDialog } from '@/platform/assets/composables/useAssetBrowserDialog' -import { - assetFilenameSchema, - assetItemSchema -} from '@/platform/assets/schemas/assetSchema' 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 { InputSpec } from '@/schemas/nodeDefSchema' import { app } from '@/scripts/app' @@ -112,7 +106,7 @@ export class PrimitiveNode extends LGraphNode { override onAfterGraphConfigured() { if (this.outputs[0].links?.length && !this.widgets?.length) { - this.#onFirstConnection() + this._onFirstConnection() // Populate widget values from config data if (this.widgets && this.widgets_values) { @@ -125,7 +119,7 @@ export class PrimitiveNode extends LGraphNode { } // Merge values if required - this.#mergeWidgetConfig() + this._mergeWidgetConfig() } } @@ -142,11 +136,11 @@ export class PrimitiveNode extends LGraphNode { const links = this.outputs[0].links if (connected) { if (links?.length && !this.widgets?.length) { - this.#onFirstConnection() + this._onFirstConnection() } } else { // We may have removed a link that caused the constraints to change - this.#mergeWidgetConfig() + this._mergeWidgetConfig() if (!links?.length) { this.onLastDisconnect() @@ -168,7 +162,7 @@ export class PrimitiveNode extends LGraphNode { } if (this.outputs[slot].links?.length) { - const valid = this.#isValidConnection(input) + const valid = this._isValidConnection(input) if (valid) { // On connect of additional outputs, copy our value to their widget this.applyToGraph([{ target_id: target_node.id, target_slot } as LLink]) @@ -179,7 +173,7 @@ export class PrimitiveNode extends LGraphNode { return true } - #onFirstConnection(recreating?: boolean) { + private _onFirstConnection(recreating?: boolean) { // First connection can fire before the graph is ready on initial load so random things can be missing if (!this.outputs[0].links || !this.graph) { this.onLastDisconnect() @@ -213,7 +207,7 @@ export class PrimitiveNode extends LGraphNode { this.outputs[0].name = type this.outputs[0].widget = widget - this.#createWidget( + this._createWidget( widget[CONFIG] ?? config, theirNode, widget.name, @@ -222,7 +216,7 @@ export class PrimitiveNode extends LGraphNode { ) } - #createWidget( + private _createWidget( inputData: InputSpec, node: LGraphNode, widgetName: string, @@ -245,8 +239,8 @@ export class PrimitiveNode extends LGraphNode { widgetName ) if (isEligible) { - widget = this.#createAssetWidget(node, widgetName, inputData) - this.#finalizeWidget(widget, oldWidth, oldHeight, recreating) + widget = this._createAssetWidget(node, widgetName, inputData) + this._finalizeWidget(widget, oldWidth, oldHeight, recreating) return } } @@ -300,69 +294,34 @@ export class PrimitiveNode extends LGraphNode { } } - this.#finalizeWidget(widget, oldWidth, oldHeight, recreating) + this._finalizeWidget(widget, oldWidth, oldHeight, recreating) } - #createAssetWidget( + private _createAssetWidget( targetNode: LGraphNode, - widgetName: string, + _widgetName: string, inputData: InputSpec ): IBaseWidget { const defaultValue = inputData[1]?.default as string | undefined - const assetBrowserDialog = useAssetBrowserDialog() - - const openModal = async (widget: IBaseWidget) => { - await assetBrowserDialog.show({ - nodeType: targetNode.comfyClass ?? '', - inputName: widgetName, - currentValue: widget.value as string, - onAssetSelected: (asset) => { - const validatedAsset = assetItemSchema.safeParse(asset) - if (!validatedAsset.success) { - console.error('Invalid asset item:', validatedAsset.error.errors) - return - } - - const filename = getAssetFilename(validatedAsset.data) - const validatedFilename = assetFilenameSchema.safeParse(filename) - if (!validatedFilename.success) { - console.error( - 'Invalid asset filename:', - validatedFilename.error.errors - ) - return - } - - const oldValue = widget.value - widget.value = validatedFilename.data - widget.callback?.( - widget.value, - app.canvas, - this, - app.canvas.graph_mouse, - {} as CanvasPointerEvent - ) - this.onWidgetChanged?.( - widget.name, - validatedFilename.data, - oldValue, - widget - ) - } - }) - } - - const options: IWidgetAssetOptions = { openModal } - return this.addWidget( - 'asset', - 'value', - defaultValue ?? '', - () => {}, - options - ) + return createAssetWidget({ + node: this, + widgetName: 'value', + nodeTypeForBrowser: targetNode.comfyClass ?? '', + defaultValue, + onValueChange: (widget, newValue, oldValue) => { + widget.callback?.( + widget.value, + app.canvas, + this, + app.canvas.graph_mouse, + {} as CanvasPointerEvent + ) + this.onWidgetChanged?.(widget.name, newValue, oldValue, widget) + } + }) } - #finalizeWidget( + private _finalizeWidget( widget: IBaseWidget, oldWidth: number, oldHeight: number, @@ -394,8 +353,8 @@ export class PrimitiveNode extends LGraphNode { recreateWidget() { const values = this.widgets?.map((w) => w.value) - this.#removeWidgets() - this.#onFirstConnection(true) + this._removeWidgets() + this._onFirstConnection(true) if (values?.length && this.widgets) { for (let i = 0; i < this.widgets.length; i++) this.widgets[i].value = values[i] @@ -403,7 +362,7 @@ export class PrimitiveNode extends LGraphNode { return this.widgets?.[0] } - #mergeWidgetConfig() { + private _mergeWidgetConfig() { // Merge widget configs if the node has multiple outputs const output = this.outputs[0] const links = output.links ?? [] @@ -435,11 +394,11 @@ export class PrimitiveNode extends LGraphNode { const theirInput = theirNode.inputs[link.target_slot] // Call is valid connection so it can merge the configs when validating - this.#isValidConnection(theirInput, hasConfig) + this._isValidConnection(theirInput, hasConfig) } } - #isValidConnection(input: INodeInputSlot, forceUpdate?: boolean) { + private _isValidConnection(input: INodeInputSlot, forceUpdate?: boolean) { // Only allow connections where the configs match const output = this.outputs?.[0] const config2 = (input.widget?.[GET_CONFIG] as () => InputSpec)?.() @@ -454,7 +413,7 @@ export class PrimitiveNode extends LGraphNode { ) } - #removeWidgets() { + private _removeWidgets() { if (this.widgets) { // Allow widgets to cleanup for (const w of this.widgets) { @@ -485,7 +444,7 @@ export class PrimitiveNode extends LGraphNode { this.outputs[0].name = 'connect to widget input' delete this.outputs[0].widget - this.#removeWidgets() + this._removeWidgets() } } diff --git a/src/lib/litegraph/src/litegraph.ts b/src/lib/litegraph/src/litegraph.ts index 59d62a84f..960dc190b 100644 --- a/src/lib/litegraph/src/litegraph.ts +++ b/src/lib/litegraph/src/litegraph.ts @@ -150,7 +150,9 @@ export { BaseWidget } from './widgets/BaseWidget' export { LegacyWidget } from './widgets/LegacyWidget' -export { isComboWidget, isAssetWidget } from './widgets/widgetMap' +export { isComboWidget } from './widgets/widgetMap' +/** @knipIgnoreUnusedButUsedByCustomNodes */ +export { isAssetWidget } from './widgets/widgetMap' // Additional test-specific exports export { LGraphButton } from './LGraphButton' export { MovingOutputLink } from './canvas/MovingOutputLink' diff --git a/src/lib/litegraph/src/widgets/widgetMap.ts b/src/lib/litegraph/src/widgets/widgetMap.ts index 37b906efb..7b2eaea6a 100644 --- a/src/lib/litegraph/src/widgets/widgetMap.ts +++ b/src/lib/litegraph/src/widgets/widgetMap.ts @@ -141,7 +141,10 @@ export function isComboWidget(widget: IBaseWidget): widget is IComboWidget { return widget.type === 'combo' } -/** Type guard: Narrow **from {@link IBaseWidget}** to {@link IAssetWidget}. */ +/** + * Type guard: Narrow **from {@link IBaseWidget}** to {@link IAssetWidget}. + * @knipIgnoreUnusedButUsedByCustomNodes + */ export function isAssetWidget(widget: IBaseWidget): widget is IAssetWidget { return widget.type === 'asset' } diff --git a/src/platform/assets/utils/createAssetWidget.ts b/src/platform/assets/utils/createAssetWidget.ts new file mode 100644 index 000000000..d63c82e70 --- /dev/null +++ b/src/platform/assets/utils/createAssetWidget.ts @@ -0,0 +1,88 @@ +import { t } from '@/i18n' +import type { LGraphNode } from '@/lib/litegraph/src/litegraph' +import type { + IBaseWidget, + IWidgetAssetOptions +} from '@/lib/litegraph/src/types/widgets' +import { useAssetBrowserDialog } from '@/platform/assets/composables/useAssetBrowserDialog' +import { + assetFilenameSchema, + assetItemSchema +} from '@/platform/assets/schemas/assetSchema' +import { getAssetFilename } from '@/platform/assets/utils/assetMetadataUtils' + +interface CreateAssetWidgetParams { + /** The node to add the widget to */ + node: LGraphNode + /** The widget name */ + widgetName: string + /** The node type to show in asset browser (may differ from node.comfyClass for PrimitiveNode) */ + nodeTypeForBrowser: string + /** Default value for the widget */ + defaultValue?: string + /** Callback when widget value changes */ + onValueChange?: ( + widget: IBaseWidget, + newValue: string, + oldValue: unknown + ) => void +} + +/** + * Creates an asset widget that opens the Asset Browser dialog for model selection. + * Used by both regular nodes (via useComboWidget) and PrimitiveNode. + * + * @param params - Configuration for the asset widget + * @returns The created asset widget + */ +export function createAssetWidget( + params: CreateAssetWidgetParams +): IBaseWidget { + const { node, widgetName, nodeTypeForBrowser, defaultValue, onValueChange } = + params + + const displayLabel = defaultValue ?? t('widgets.selectModel') + const assetBrowserDialog = useAssetBrowserDialog() + + async function openModal(widget: IBaseWidget) { + await assetBrowserDialog.show({ + nodeType: nodeTypeForBrowser, + inputName: widgetName, + currentValue: widget.value as string, + onAssetSelected: (asset) => { + const validatedAsset = assetItemSchema.safeParse(asset) + + if (!validatedAsset.success) { + console.error( + 'Invalid asset item:', + validatedAsset.error.errors, + 'Received:', + asset + ) + return + } + + const filename = getAssetFilename(validatedAsset.data) + const validatedFilename = assetFilenameSchema.safeParse(filename) + + if (!validatedFilename.success) { + console.error( + 'Invalid asset filename:', + validatedFilename.error.errors, + 'for asset:', + validatedAsset.data.id + ) + return + } + + const oldValue = widget.value + widget.value = validatedFilename.data + onValueChange?.(widget, validatedFilename.data, oldValue) + } + }) + } + + const options: IWidgetAssetOptions = { openModal } + + return node.addWidget('asset', widgetName, displayLabel, () => {}, options) +} diff --git a/src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts b/src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts index 75cf47fec..c10be3d81 100644 --- a/src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts +++ b/src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts @@ -3,18 +3,10 @@ import { ref } from 'vue' import MultiSelectWidget from '@/components/graph/widgets/MultiSelectWidget.vue' import { t } from '@/i18n' import type { LGraphNode } from '@/lib/litegraph/src/litegraph' -import { isAssetWidget, isComboWidget } from '@/lib/litegraph/src/litegraph' -import type { - IBaseWidget, - IWidgetAssetOptions -} from '@/lib/litegraph/src/types/widgets' -import { useAssetBrowserDialog } from '@/platform/assets/composables/useAssetBrowserDialog' -import { - assetFilenameSchema, - assetItemSchema -} from '@/platform/assets/schemas/assetSchema' -import { getAssetFilename } from '@/platform/assets/utils/assetMetadataUtils' +import { isComboWidget } from '@/lib/litegraph/src/litegraph' +import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets' import { assetService } from '@/platform/assets/services/assetService' +import { createAssetWidget } from '@/platform/assets/utils/createAssetWidget' import { isCloud } from '@/platform/distribution/types' import { useSettingStore } from '@/platform/settings/settingStore' import type { @@ -86,71 +78,20 @@ const addMultiSelectWidget = ( return widget } -const createAssetBrowserWidget = ( +function createAssetBrowserWidget( node: LGraphNode, inputSpec: ComboInputSpec, defaultValue: string | undefined -): IBaseWidget => { - const currentValue = defaultValue - const displayLabel = currentValue ?? t('widgets.selectModel') - const assetBrowserDialog = useAssetBrowserDialog() - - async function openModal(widget: IBaseWidget) { - if (!isAssetWidget(widget)) { - throw new Error(`Expected asset widget but received ${widget.type}`) +): IBaseWidget { + return createAssetWidget({ + node, + widgetName: inputSpec.name, + nodeTypeForBrowser: node.comfyClass ?? '', + defaultValue, + onValueChange: (widget, newValue, oldValue) => { + node.onWidgetChanged?.(widget.name, newValue, oldValue, widget) } - await assetBrowserDialog.show({ - nodeType: node.comfyClass || '', - inputName: inputSpec.name, - currentValue: widget.value, - onAssetSelected: (asset) => { - const validatedAsset = assetItemSchema.safeParse(asset) - - if (!validatedAsset.success) { - console.error( - 'Invalid asset item:', - validatedAsset.error.errors, - 'Received:', - asset - ) - return - } - - const filename = getAssetFilename(validatedAsset.data) - const validatedFilename = assetFilenameSchema.safeParse(filename) - - if (!validatedFilename.success) { - console.error( - 'Invalid asset filename:', - validatedFilename.error.errors, - 'for asset:', - validatedAsset.data.id - ) - return - } - - const oldValue = widget.value - widget.value = validatedFilename.data - node.onWidgetChanged?.( - widget.name, - validatedFilename.data, - oldValue, - widget - ) - } - }) - } - const options: IWidgetAssetOptions = { openModal } - - const widget = node.addWidget( - 'asset', - inputSpec.name, - displayLabel, - () => undefined, - options - ) - - return widget + }) } const createInputMappingWidget = ( diff --git a/todo.md b/todo.md new file mode 100644 index 000000000..efa1111e0 --- /dev/null +++ b/todo.md @@ -0,0 +1,34 @@ +# Bug Description + +When using a primitive node to select a model, this causes an issue specifically on Comfy Cloud. The issue does not appear to occur in the local version. + +## Context + +This bug was reported in the #frontend-bug-dump channel with reference links to Slack thread discussions: + +- https://comfy-organization.slack.com/archives/C07RCREPL67/p1767751867519549?thread_ts=1767751129.592359&cid=C07RCREPL67 +- https://comfy-organization.slack.com/archives/C07RCREPL67/p1767752100115359?thread_ts=1767751129.592359&cid=C07RCREPL67 + +## Reproduction + +The issue is triggered when a model is selected by a primitive node in workflows running on the cloud. + +## Expected Behavior + +Model selection via primitive nodes should work consistently between local and cloud environments. + +## Actual Behavior + +An issue occurs on cloud when using primitive nodes for model selection. + +## Additional Information + +### Related Bug Ticket + +Another bug ticket was created here: https://comfy-organization.slack.com/archives/C09FY39CC3V/p1767753991406309?thread_ts=1767734275.051999&cid=C09FY39CC3V + +### Updated Reproduction Details + +**Important:** This workflow didn't include any primitive nodes, but still caused the same issue. This indicates the problem is not limited to primitive nodes as originally thought. + +**Affected Workflow File:** video*ltx2*t2v_distilled.json (provided in Slack thread)