mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-21 21:09:00 +00:00
fix: clear media upload errors via widget change (#12212)
## Summary Clear missing media validation errors after paste/drop media uploads by emitting the existing widget-change event path. ## Changes - **What**: Emit `node.onWidgetChanged` after image/video upload completion updates the file combo widget. - **What**: Emit the same widget-change path after Load Audio upload completion. - **What**: Add unit coverage for upload completion emitting `onWidgetChanged` and for missing media clearing through that existing hook path. - **What**: Add E2E coverage for Load Image drag/drop and paste clearing validation rings, with red/green verified from a fresh `main` base. - **Dependencies**: None. ## Review Focus Please check that paste/drop upload paths now reuse the existing widget-change error-clearing path instead of expanding `widget.callback` patching. Also check the Load Image E2E helper path for synthetic paste/drop behavior. Supersedes #12207. Ref: FE-687 ## Screenshots Before https://github.com/user-attachments/assets/2cee52bc-b1c8-4dff-8a02-5b18a69ae639 After https://github.com/user-attachments/assets/e1ecd147-1d8a-470e-b77d-13345d473ef3 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-12212-fix-clear-media-upload-errors-via-widget-change-35f6d73d365081bcb1a0dfc042d417eb) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -6,6 +6,71 @@ import type { Locator, Page } from '@playwright/test'
|
||||
import type { KeyboardHelper } from '@e2e/fixtures/helpers/KeyboardHelper'
|
||||
import { getMimeType } from '@e2e/fixtures/utils/mimeTypeUtil'
|
||||
|
||||
function readFilePayload(filePath: string) {
|
||||
const buffer = readFileSync(filePath)
|
||||
const bufferArray = [...new Uint8Array(buffer)]
|
||||
const fileName = basename(filePath)
|
||||
const fileType = getMimeType(fileName)
|
||||
|
||||
return { bufferArray, fileName, fileType }
|
||||
}
|
||||
|
||||
async function dispatchFilePaste(
|
||||
page: Page,
|
||||
payload: ReturnType<typeof readFilePayload>
|
||||
): Promise<void> {
|
||||
await page.evaluate(({ bufferArray, fileName, fileType }) => {
|
||||
const file = new File([new Uint8Array(bufferArray)], fileName, {
|
||||
type: fileType
|
||||
})
|
||||
const dataTransfer = new DataTransfer()
|
||||
dataTransfer.items.add(file)
|
||||
|
||||
const target = document.activeElement ?? document
|
||||
target.dispatchEvent(
|
||||
new ClipboardEvent('paste', {
|
||||
clipboardData: dataTransfer,
|
||||
bubbles: true,
|
||||
cancelable: true
|
||||
})
|
||||
)
|
||||
}, payload)
|
||||
}
|
||||
|
||||
async function interceptNextFilePaste(
|
||||
page: Page,
|
||||
payload: ReturnType<typeof readFilePayload>
|
||||
): Promise<void> {
|
||||
await page.evaluate(({ bufferArray, fileName, fileType }) => {
|
||||
document.addEventListener(
|
||||
'paste',
|
||||
(e: ClipboardEvent) => {
|
||||
e.preventDefault()
|
||||
e.stopImmediatePropagation()
|
||||
|
||||
const file = new File([new Uint8Array(bufferArray)], fileName, {
|
||||
type: fileType
|
||||
})
|
||||
const dataTransfer = new DataTransfer()
|
||||
dataTransfer.items.add(file)
|
||||
|
||||
document.dispatchEvent(
|
||||
new ClipboardEvent('paste', {
|
||||
clipboardData: dataTransfer,
|
||||
bubbles: true,
|
||||
cancelable: true
|
||||
})
|
||||
)
|
||||
},
|
||||
{ capture: true, once: true }
|
||||
)
|
||||
}, payload)
|
||||
}
|
||||
|
||||
type PasteFileOptions = {
|
||||
mode?: 'keyboard' | 'direct'
|
||||
}
|
||||
|
||||
export class ClipboardHelper {
|
||||
constructor(
|
||||
private readonly keyboard: KeyboardHelper,
|
||||
@@ -20,43 +85,20 @@ export class ClipboardHelper {
|
||||
await this.keyboard.ctrlSend('KeyV', locator ?? null)
|
||||
}
|
||||
|
||||
async pasteFile(filePath: string): Promise<void> {
|
||||
const buffer = readFileSync(filePath)
|
||||
const bufferArray = [...new Uint8Array(buffer)]
|
||||
const fileName = basename(filePath)
|
||||
const fileType = getMimeType(fileName)
|
||||
async pasteFile(
|
||||
filePath: string,
|
||||
{ mode = 'keyboard' }: PasteFileOptions = {}
|
||||
): Promise<void> {
|
||||
const payload = readFilePayload(filePath)
|
||||
|
||||
// Register a one-time capturing-phase listener that intercepts the next
|
||||
// paste event and injects file data onto clipboardData.
|
||||
await this.page.evaluate(
|
||||
({ bufferArray, fileName, fileType }) => {
|
||||
document.addEventListener(
|
||||
'paste',
|
||||
(e: ClipboardEvent) => {
|
||||
e.preventDefault()
|
||||
e.stopImmediatePropagation()
|
||||
if (mode === 'keyboard') {
|
||||
await interceptNextFilePaste(this.page, payload)
|
||||
await this.paste()
|
||||
return
|
||||
}
|
||||
|
||||
const file = new File([new Uint8Array(bufferArray)], fileName, {
|
||||
type: fileType
|
||||
})
|
||||
const dataTransfer = new DataTransfer()
|
||||
dataTransfer.items.add(file)
|
||||
|
||||
const syntheticEvent = new ClipboardEvent('paste', {
|
||||
clipboardData: dataTransfer,
|
||||
bubbles: true,
|
||||
cancelable: true
|
||||
})
|
||||
document.dispatchEvent(syntheticEvent)
|
||||
},
|
||||
{ capture: true, once: true }
|
||||
)
|
||||
},
|
||||
{ bufferArray, fileName, fileType }
|
||||
)
|
||||
|
||||
// Trigger a real Ctrl+V keystroke — the capturing listener above will
|
||||
// intercept it and re-dispatch with file data attached.
|
||||
await this.paste()
|
||||
// Browser clipboard APIs cannot reliably seed arbitrary files in tests.
|
||||
// Dispatch the app-level paste event with file clipboardData directly.
|
||||
await dispatchFilePaste(this.page, payload)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ import {
|
||||
comfyExpect as expect,
|
||||
comfyPageFixture
|
||||
} from '@e2e/fixtures/ComfyPage'
|
||||
import type { ComfyPage } from '@e2e/fixtures/ComfyPage'
|
||||
import {
|
||||
cleanupFakeModel,
|
||||
dismissErrorOverlay,
|
||||
@@ -13,7 +14,9 @@ import {
|
||||
ExecutionHelper,
|
||||
buildKSamplerError
|
||||
} from '@e2e/fixtures/helpers/ExecutionHelper'
|
||||
import type { NodeError } from '@/schemas/apiSchema'
|
||||
import { fitToViewInstant } from '@e2e/fixtures/utils/fitToView'
|
||||
import { assetPath } from '@e2e/fixtures/utils/paths'
|
||||
import { webSocketFixture } from '@e2e/fixtures/ws'
|
||||
|
||||
const test = mergeTests(comfyPageFixture, webSocketFixture)
|
||||
@@ -22,6 +25,61 @@ const ERROR_CLASS = /ring-destructive-background/
|
||||
const UNKNOWN_NODE_ID = '1'
|
||||
const INNER_EXECUTION_ID = '2:1'
|
||||
const KSAMPLER_MODEL_INPUT_NAME = 'model'
|
||||
const LOAD_IMAGE_INPUT_NAME = 'image'
|
||||
const LOAD_IMAGE_UPLOAD_FILE = 'test_upload_image.png'
|
||||
|
||||
function buildLoadImageRequiredInputError(): NodeError {
|
||||
return {
|
||||
class_type: 'LoadImage',
|
||||
dependent_outputs: [],
|
||||
errors: [
|
||||
{
|
||||
type: 'required_input_missing',
|
||||
message: `Required input is missing: ${LOAD_IMAGE_INPUT_NAME}`,
|
||||
details: '',
|
||||
extra_info: { input_name: LOAD_IMAGE_INPUT_NAME }
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
async function surfaceLoadImageMissingInputError(
|
||||
comfyPage: ComfyPage,
|
||||
loadImageId: string
|
||||
): Promise<void> {
|
||||
const exec = new ExecutionHelper(comfyPage)
|
||||
await exec.mockValidationFailure({
|
||||
[loadImageId]: buildLoadImageRequiredInputError()
|
||||
})
|
||||
await comfyPage.runButton.click()
|
||||
await dismissErrorOverlay(comfyPage)
|
||||
}
|
||||
|
||||
async function selectLoadImageNodeForPaste(
|
||||
comfyPage: ComfyPage,
|
||||
loadImageId: string
|
||||
): Promise<void> {
|
||||
await comfyPage.page.evaluate((nodeId) => {
|
||||
const node = window.app!.graph.getNodeById(Number(nodeId))
|
||||
if (!node) throw new Error(`Load Image node ${nodeId} not found`)
|
||||
window.app!.canvas.selectNode(node)
|
||||
window.app!.canvas.current_node = node
|
||||
}, loadImageId)
|
||||
}
|
||||
|
||||
async function setupLoadImageErrorScenario(comfyPage: ComfyPage) {
|
||||
await comfyPage.workflow.loadWorkflow('widgets/load_image_widget')
|
||||
const loadImageNode = (
|
||||
await comfyPage.nodeOps.getNodeRefsByType('LoadImage')
|
||||
)[0]
|
||||
const loadImageId = String(loadImageNode.id)
|
||||
|
||||
return {
|
||||
loadImageId,
|
||||
innerWrapper: comfyPage.vueNodes.getNodeInnerWrapper(loadImageId),
|
||||
imageWidget: await loadImageNode.getWidgetByName(LOAD_IMAGE_INPUT_NAME)
|
||||
}
|
||||
}
|
||||
|
||||
test.describe('Vue Node Error', { tag: '@vue-nodes' }, () => {
|
||||
test('should display error state when node is missing (node from workflow is not installed)', async ({
|
||||
@@ -191,6 +249,74 @@ test.describe('Vue Node Error', { tag: '@vue-nodes' }, () => {
|
||||
|
||||
await expect(innerWrapper).not.toHaveClass(ERROR_CLASS)
|
||||
})
|
||||
|
||||
test('clears error ring when user drops an image file onto Load Image', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
const { loadImageId, innerWrapper, imageWidget } =
|
||||
await setupLoadImageErrorScenario(comfyPage)
|
||||
|
||||
await test.step('queue with missing image input to surface the error', async () => {
|
||||
await surfaceLoadImageMissingInputError(comfyPage, loadImageId)
|
||||
await expect(innerWrapper).toHaveClass(ERROR_CLASS)
|
||||
})
|
||||
|
||||
await test.step('drop an image onto the Load Image node', async () => {
|
||||
const dropPosition =
|
||||
await comfyPage.canvasOps.getNodeCenterByTitle('Load Image')
|
||||
if (!dropPosition) {
|
||||
throw new Error('Load Image node center must be available for drop')
|
||||
}
|
||||
|
||||
await comfyPage.dragDrop.dragAndDropFile(LOAD_IMAGE_UPLOAD_FILE, {
|
||||
dropPosition,
|
||||
waitForUpload: true
|
||||
})
|
||||
await expect
|
||||
.poll(() => imageWidget.getValue())
|
||||
.toContain(LOAD_IMAGE_UPLOAD_FILE)
|
||||
})
|
||||
|
||||
await expect(innerWrapper).not.toHaveClass(ERROR_CLASS)
|
||||
})
|
||||
|
||||
test('clears error ring when user pastes an image file onto Load Image', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
const { loadImageId, innerWrapper, imageWidget } =
|
||||
await setupLoadImageErrorScenario(comfyPage)
|
||||
|
||||
await test.step('queue with missing image input to surface the error', async () => {
|
||||
await surfaceLoadImageMissingInputError(comfyPage, loadImageId)
|
||||
await expect(innerWrapper).toHaveClass(ERROR_CLASS)
|
||||
})
|
||||
|
||||
await test.step('paste an image while Load Image is selected', async () => {
|
||||
await comfyPage.canvas.focus()
|
||||
await selectLoadImageNodeForPaste(comfyPage, loadImageId)
|
||||
await expect
|
||||
.poll(() =>
|
||||
comfyPage.page.evaluate(() => window.app!.canvas.current_node?.type)
|
||||
)
|
||||
.toBe('LoadImage')
|
||||
|
||||
const uploadResponse = comfyPage.page.waitForResponse(
|
||||
(resp) => resp.url().includes('/upload/') && resp.status() === 200,
|
||||
{ timeout: 10_000 }
|
||||
)
|
||||
// File clipboard contents cannot be seeded reliably in Playwright;
|
||||
// use the direct document paste mode to exercise usePaste.
|
||||
await comfyPage.clipboard.pasteFile(assetPath(LOAD_IMAGE_UPLOAD_FILE), {
|
||||
mode: 'direct'
|
||||
})
|
||||
await uploadResponse
|
||||
await expect
|
||||
.poll(() => imageWidget.getValue())
|
||||
.toContain(LOAD_IMAGE_UPLOAD_FILE)
|
||||
})
|
||||
|
||||
await expect(innerWrapper).not.toHaveClass(ERROR_CLASS)
|
||||
})
|
||||
})
|
||||
|
||||
test.describe('subgraph propagation', { tag: '@subgraph' }, () => {
|
||||
|
||||
@@ -21,6 +21,7 @@ import { useMissingNodesErrorStore } from '@/platform/nodeReplacement/missingNod
|
||||
import { app } from '@/scripts/app'
|
||||
import { useExecutionErrorStore } from '@/stores/executionErrorStore'
|
||||
import { seedRequiredInputMissingNodeError } from '@/utils/__tests__/executionErrorTestUtils'
|
||||
import type { MissingMediaCandidate } from '@/platform/missingMedia/types'
|
||||
import type { MissingModelCandidate } from '@/platform/missingModel/types'
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -210,6 +211,47 @@ describe('Widget change error clearing via onWidgetChanged', () => {
|
||||
expect(store.lastNodeErrors).not.toBeNull()
|
||||
})
|
||||
|
||||
it('clears missing media when an upload emits onWidgetChanged', () => {
|
||||
const graph = new LGraph()
|
||||
const node = new LGraphNode('LoadImage')
|
||||
node.type = 'LoadImage'
|
||||
const widget = node.addWidget(
|
||||
'combo',
|
||||
'image',
|
||||
'missing.png',
|
||||
() => undefined,
|
||||
{ values: [] }
|
||||
)
|
||||
graph.add(node)
|
||||
installErrorClearingHooks(graph)
|
||||
|
||||
const store = useExecutionErrorStore()
|
||||
const mediaStore = useMissingMediaStore()
|
||||
vi.spyOn(app, 'rootGraph', 'get').mockReturnValue(graph)
|
||||
seedRequiredInputMissingNodeError(store, String(node.id), 'image')
|
||||
mediaStore.setMissingMedia([
|
||||
{
|
||||
nodeId: String(node.id),
|
||||
nodeType: 'LoadImage',
|
||||
widgetName: 'image',
|
||||
mediaType: 'image',
|
||||
name: 'missing.png',
|
||||
isMissing: true
|
||||
} satisfies MissingMediaCandidate
|
||||
])
|
||||
|
||||
node.onWidgetChanged!.call(
|
||||
node,
|
||||
'image',
|
||||
'uploaded.png',
|
||||
'missing.png',
|
||||
widget
|
||||
)
|
||||
|
||||
expect(store.lastNodeErrors).toBeNull()
|
||||
expect(mediaStore.missingMediaCandidates).toBeNull()
|
||||
})
|
||||
|
||||
it('uses interior node execution ID for promoted widget error clearing', () => {
|
||||
const subgraph = createTestSubgraph({
|
||||
inputs: [{ name: 'ckpt_input', type: '*' }]
|
||||
|
||||
@@ -130,7 +130,8 @@ function createAudioNode() {
|
||||
widgets: [audioWidget, audioUIWidget],
|
||||
isUploading: false,
|
||||
graph: { setDirtyCanvas: vi.fn() },
|
||||
addWidget: vi.fn(() => uploadWidget)
|
||||
addWidget: vi.fn(() => uploadWidget),
|
||||
onWidgetChanged: vi.fn()
|
||||
})
|
||||
|
||||
return { audioUIWidget, audioWidget, node, uploadWidget }
|
||||
@@ -180,6 +181,12 @@ describe('Comfy.UploadAudio AUDIOUPLOAD widget', () => {
|
||||
expect(node.isUploading).toBe(false)
|
||||
expect(audioWidget.value).toBe('pasted/uploaded.mp3')
|
||||
expect(audioWidget.options.values).toContain('pasted/uploaded.mp3')
|
||||
expect(node.onWidgetChanged).toHaveBeenCalledWith(
|
||||
'audio',
|
||||
'pasted/uploaded.mp3',
|
||||
'clip.mp3',
|
||||
audioWidget
|
||||
)
|
||||
expect(node.graph?.setDirtyCanvas).toHaveBeenCalledWith(true)
|
||||
})
|
||||
|
||||
|
||||
@@ -38,6 +38,7 @@ function updateUIWidget(
|
||||
}
|
||||
|
||||
async function uploadFile(
|
||||
node: LGraphNode,
|
||||
audioWidget: IStringWidget,
|
||||
audioUIWidget: DOMWidget<HTMLAudioElement, string>,
|
||||
file: File,
|
||||
@@ -67,6 +68,7 @@ async function uploadFile(
|
||||
}
|
||||
|
||||
if (updateNode) {
|
||||
const oldValue = audioWidget.value
|
||||
updateUIWidget(
|
||||
audioUIWidget,
|
||||
api.apiURL(getResourceURL(...splitFilePath(path)))
|
||||
@@ -75,6 +77,7 @@ async function uploadFile(
|
||||
audioWidget.value = path
|
||||
// Manually trigger the callback to update VueNodes
|
||||
audioWidget.callback?.(path)
|
||||
node.onWidgetChanged?.(audioWidget.name, path, oldValue, audioWidget)
|
||||
}
|
||||
return true
|
||||
} else {
|
||||
@@ -247,6 +250,7 @@ app.registerExtension({
|
||||
audioWidget.value = files[0].name
|
||||
try {
|
||||
const success = await uploadFile(
|
||||
node,
|
||||
audioWidget,
|
||||
audioUIWidget,
|
||||
files[0],
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
import { fromPartial } from '@total-typescript/shoehorn'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { useImageUploadWidget } from '@/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget'
|
||||
import { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
import type { IComboWidget } from '@/lib/litegraph/src/types/widgets'
|
||||
import type { ResultItem, ResultItemType } from '@/schemas/apiSchema'
|
||||
import type { InputSpec } from '@/schemas/nodeDefSchema'
|
||||
|
||||
type CapturedImageUploadOptions = {
|
||||
onUploadComplete: (paths: (string | ResultItem)[]) => void
|
||||
allow_batch?: boolean
|
||||
folder?: ResultItemType
|
||||
onUploadStart?: (files: File[]) => void
|
||||
onUploadError?: () => void
|
||||
}
|
||||
|
||||
const mocks = vi.hoisted(() => ({
|
||||
capturedUploadOptions: undefined as CapturedImageUploadOptions | undefined,
|
||||
openFileSelection: vi.fn(),
|
||||
setNodeOutputs: vi.fn(),
|
||||
showPreview: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('@/composables/node/useNodeImage', () => ({
|
||||
useNodeImage: () => ({ showPreview: mocks.showPreview }),
|
||||
useNodeVideo: () => ({ showPreview: mocks.showPreview })
|
||||
}))
|
||||
|
||||
vi.mock('@/composables/node/useNodeImageUpload', () => ({
|
||||
useNodeImageUpload: (
|
||||
_node: LGraphNode,
|
||||
options: CapturedImageUploadOptions
|
||||
) => {
|
||||
mocks.capturedUploadOptions = options
|
||||
return { openFileSelection: mocks.openFileSelection }
|
||||
}
|
||||
}))
|
||||
|
||||
vi.mock('@/i18n', () => ({
|
||||
t: (key: string) => key
|
||||
}))
|
||||
|
||||
vi.mock('@/stores/nodeOutputStore', () => ({
|
||||
useNodeOutputStore: () => ({
|
||||
setNodeOutputs: mocks.setNodeOutputs
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/utils/litegraphUtil', () => ({
|
||||
addToComboValues: (widget: IComboWidget, value: string) => {
|
||||
const values = widget.options?.values
|
||||
if (Array.isArray(values) && !values.includes(value)) {
|
||||
values.push(value)
|
||||
}
|
||||
}
|
||||
}))
|
||||
|
||||
function createUploadNode() {
|
||||
const onWidgetChanged = vi.fn()
|
||||
const node = new LGraphNode('LoadImage')
|
||||
node.type = 'LoadImage'
|
||||
node.onWidgetChanged = onWidgetChanged
|
||||
const fileComboWidget = node.addWidget(
|
||||
'combo',
|
||||
'image',
|
||||
'missing.png',
|
||||
() => undefined,
|
||||
{ values: ['missing.png'] }
|
||||
) as IComboWidget
|
||||
|
||||
return { fileComboWidget, node, onWidgetChanged }
|
||||
}
|
||||
|
||||
describe('useImageUploadWidget', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
mocks.capturedUploadOptions = undefined
|
||||
vi.stubGlobal('requestAnimationFrame', vi.fn())
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals()
|
||||
})
|
||||
|
||||
it('emits onWidgetChanged after upload changes the combo widget value', () => {
|
||||
const { fileComboWidget, node, onWidgetChanged } = createUploadNode()
|
||||
const constructor = useImageUploadWidget()
|
||||
|
||||
constructor(
|
||||
node,
|
||||
'upload',
|
||||
[
|
||||
'IMAGEUPLOAD',
|
||||
{ imageInputName: 'image', image_upload: true }
|
||||
] as InputSpec,
|
||||
fromPartial({})
|
||||
)
|
||||
|
||||
mocks.capturedUploadOptions?.onUploadComplete(['uploaded.png'])
|
||||
|
||||
expect(fileComboWidget.value).toBe('uploaded.png')
|
||||
expect(mocks.setNodeOutputs).toHaveBeenCalledWith(node, 'uploaded.png', {
|
||||
isAnimated: false
|
||||
})
|
||||
expect(onWidgetChanged).toHaveBeenCalledWith(
|
||||
'image',
|
||||
'uploaded.png',
|
||||
'missing.png',
|
||||
fileComboWidget
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -83,10 +83,17 @@ export const useImageUploadWidget = () => {
|
||||
})
|
||||
|
||||
const newValue = allow_batch ? annotated : annotated[0]
|
||||
const oldValue = fileComboWidget.value
|
||||
|
||||
// @ts-expect-error litegraph combo value type does not support arrays yet
|
||||
fileComboWidget.value = newValue
|
||||
fileComboWidget.callback?.(newValue)
|
||||
node.onWidgetChanged?.(
|
||||
fileComboWidget.name,
|
||||
newValue,
|
||||
oldValue,
|
||||
fileComboWidget
|
||||
)
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user