mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-05 05:32:02 +00:00
*PR Created by the Glary-Bot Agent* --- ## Summary Fixes the "choose video to upload" button becoming unresponsive after running a workflow with a subgraph a few times. **Root cause**: The detached input element in `useNodeFileInput` never resets its `value`. The browser's `onchange` only fires when the value *changes* — re-selecting the same file silently drops the event. A page refresh recreates the input with an empty value, which is why refreshing fixes it. ## Changes - `useNodeFileInput.ts`: Reset `fileInput.value` before invoking callbacks so value is cleared even if a callback throws - `useNodeDragAndDrop.ts`: Add `onRemoved` cleanup for installed handlers (only clears own handlers; preserves replacements from extensions) - `useNodePaste.ts`: Add `onRemoved` cleanup for installed `pasteFiles` handler (same reference-safe pattern) - 3 new colocated test files with 26 test cases covering all branches ## Codebase Audit Audited all 11 file upload implementations across the codebase. Found 5 using the ghost/virtual input pattern — 3 with the same missing value-reset bug: - `useNodeFileInput.ts` — fixed in this PR - `scripts/utils.ts` (`uploadFile()`) — one-shot pattern, lower risk - `extensions/core/load3d.ts` — partial reset only The 4 Vue component implementations already reset correctly. ## Future Work VueUse `useFileDialog` composable handles same-file reselection via `reset: true` and provides automatic lifecycle cleanup. A follow-up PR could migrate the ghost input patterns for a centralized solution. ## Test Plan - 26 unit tests across 3 new test files (all pass) - 9 existing useNodeImageUpload tests still pass - Pre-commit hooks pass (oxfmt, oxlint, eslint, typecheck) - Oracle code review addressed ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11417-fix-reset-file-input-value-after-selection-to-allow-same-file-reupload-3476d73d3650814d95efdab602a3852d) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com>
244 lines
7.2 KiB
TypeScript
244 lines
7.2 KiB
TypeScript
import { fromAny } from '@total-typescript/shoehorn'
|
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
|
|
import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
|
import { useNodeDragAndDrop } from './useNodeDragAndDrop'
|
|
|
|
function createNode(overrides: Record<string, unknown> = {}): LGraphNode {
|
|
return fromAny<LGraphNode, unknown>({
|
|
...overrides
|
|
})
|
|
}
|
|
|
|
function createFile(name: string, type = 'image/png'): File {
|
|
return new File(['data'], name, { type })
|
|
}
|
|
|
|
function createDragEvent(options: {
|
|
items?: Array<{ kind: string }>
|
|
files?: File[]
|
|
types?: string[]
|
|
uri?: string
|
|
}): DragEvent {
|
|
const { items = [], files = [], types = [], uri = '' } = options
|
|
return fromAny<DragEvent, unknown>({
|
|
dataTransfer: {
|
|
items: fromAny<DataTransferItemList, unknown>(items),
|
|
files: fromAny<FileList, unknown>(files),
|
|
types,
|
|
getData: vi.fn((format: string) =>
|
|
format === 'text/uri-list' ? uri : ''
|
|
)
|
|
}
|
|
})
|
|
}
|
|
|
|
describe('useNodeDragAndDrop', () => {
|
|
beforeEach(() => {
|
|
vi.restoreAllMocks()
|
|
})
|
|
|
|
it('onDragOver detects file items by default', () => {
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop: vi.fn().mockResolvedValue([]) })
|
|
|
|
const isDragging = node.onDragOver?.(
|
|
createDragEvent({ items: [{ kind: 'file' }] })
|
|
)
|
|
|
|
expect(isDragging).toBe(true)
|
|
})
|
|
|
|
it('onDragOver delegates to custom handler result', () => {
|
|
const node = createNode()
|
|
const onDragOver = vi.fn().mockReturnValue(false)
|
|
|
|
useNodeDragAndDrop(node, {
|
|
onDrop: vi.fn().mockResolvedValue([]),
|
|
onDragOver
|
|
})
|
|
|
|
const isDragging = node.onDragOver?.(
|
|
createDragEvent({ items: [{ kind: 'file' }] })
|
|
)
|
|
|
|
expect(onDragOver).toHaveBeenCalledTimes(1)
|
|
expect(isDragging).toBe(false)
|
|
})
|
|
|
|
it('onDragOver returns true for uri list drops without file items', () => {
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop: vi.fn().mockResolvedValue([]) })
|
|
|
|
const isDragging = node.onDragOver?.(
|
|
createDragEvent({ items: [{ kind: 'string' }], types: ['text/uri-list'] })
|
|
)
|
|
|
|
expect(isDragging).toBe(true)
|
|
})
|
|
|
|
it('onDragOver returns false when drag event has no items', () => {
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop: vi.fn().mockResolvedValue([]) })
|
|
|
|
const isDragging = node.onDragOver?.(fromAny<DragEvent, unknown>({}))
|
|
|
|
expect(isDragging).toBe(false)
|
|
})
|
|
|
|
it('onDragDrop calls onDrop with filtered files', async () => {
|
|
const onDrop = vi.fn().mockResolvedValue([])
|
|
const node = createNode()
|
|
const keep = createFile('keep.png')
|
|
const skip = createFile('skip.jpg', 'image/jpeg')
|
|
|
|
useNodeDragAndDrop(node, {
|
|
onDrop,
|
|
fileFilter: (file) => file.type === 'image/png'
|
|
})
|
|
|
|
const result = await node.onDragDrop?.(
|
|
createDragEvent({ files: [keep, skip], items: [{ kind: 'file' }] })
|
|
)
|
|
|
|
expect(result).toBe(true)
|
|
expect(onDrop).toHaveBeenCalledWith([keep])
|
|
})
|
|
|
|
it('onDragDrop returns false for invalid drops', async () => {
|
|
const onDrop = vi.fn().mockResolvedValue([])
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop })
|
|
|
|
const result = await node.onDragDrop?.(createDragEvent({}))
|
|
|
|
expect(result).toBe(false)
|
|
expect(onDrop).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('onDragDrop handles same-origin uri drops', async () => {
|
|
const onDrop = vi.fn().mockResolvedValue([])
|
|
const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(
|
|
fromAny<Response, unknown>({
|
|
ok: true,
|
|
blob: vi
|
|
.fn()
|
|
.mockResolvedValue(new Blob(['uri'], { type: 'image/png' }))
|
|
})
|
|
)
|
|
const uri = `${location.origin}/api/file?filename=uri.png`
|
|
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop })
|
|
|
|
const result = await node.onDragDrop?.(
|
|
createDragEvent({ uri, types: ['text/uri-list'] })
|
|
)
|
|
|
|
expect(result).toBe(true)
|
|
expect(fetchSpy).toHaveBeenCalledWith(new URL(uri))
|
|
expect(onDrop).toHaveBeenCalledTimes(1)
|
|
expect(onDrop.mock.calls[0][0][0]).toBeInstanceOf(File)
|
|
expect(onDrop.mock.calls[0][0][0].name).toBe('uri.png')
|
|
})
|
|
|
|
it('onDragDrop returns false for cross-origin uri drops', async () => {
|
|
const node = createNode()
|
|
const onDrop = vi.fn().mockResolvedValue([])
|
|
const fetchSpy = vi.spyOn(globalThis, 'fetch')
|
|
useNodeDragAndDrop(node, { onDrop })
|
|
|
|
const result = await node.onDragDrop?.(
|
|
createDragEvent({
|
|
uri: 'https://example.com/api/file?filename=uri.png',
|
|
types: ['text/uri-list']
|
|
})
|
|
)
|
|
|
|
expect(result).toBe(false)
|
|
expect(fetchSpy).not.toHaveBeenCalled()
|
|
expect(onDrop).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('onDragDrop returns false when uri fetch throws', async () => {
|
|
const onDrop = vi.fn().mockResolvedValue([])
|
|
vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('network'))
|
|
const uri = `${location.origin}/api/file?filename=uri.png`
|
|
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop })
|
|
|
|
const result = await node.onDragDrop?.(
|
|
createDragEvent({ uri, types: ['text/uri-list'] })
|
|
)
|
|
|
|
expect(result).toBe(false)
|
|
expect(onDrop).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('onDragDrop returns false when uri response is invalid or filtered out', async () => {
|
|
const onDrop = vi.fn().mockResolvedValue([])
|
|
const uri = `${location.origin}/api/file?filename=uri.jpg`
|
|
|
|
const nodeA = createNode()
|
|
vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
|
|
fromAny<Response, unknown>({ ok: false })
|
|
)
|
|
useNodeDragAndDrop(nodeA, { onDrop })
|
|
const badResponseResult = await nodeA.onDragDrop?.(
|
|
createDragEvent({ uri, types: ['text/uri-list'] })
|
|
)
|
|
expect(badResponseResult).toBe(false)
|
|
|
|
const nodeB = createNode()
|
|
vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
|
|
fromAny<Response, unknown>({
|
|
ok: true,
|
|
blob: vi
|
|
.fn()
|
|
.mockResolvedValue(new Blob(['uri'], { type: 'image/jpeg' }))
|
|
})
|
|
)
|
|
useNodeDragAndDrop(nodeB, {
|
|
onDrop,
|
|
fileFilter: (file) => file.type === 'image/png'
|
|
})
|
|
const filteredOutResult = await nodeB.onDragDrop?.(
|
|
createDragEvent({ uri, types: ['text/uri-list'] })
|
|
)
|
|
|
|
expect(filteredOutResult).toBe(false)
|
|
expect(onDrop).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('onRemoved clears handlers and chains existing onRemoved', () => {
|
|
const previousOnRemoved = vi.fn()
|
|
const node = createNode({ onRemoved: previousOnRemoved })
|
|
|
|
useNodeDragAndDrop(node, { onDrop: vi.fn().mockResolvedValue([]) })
|
|
expect(node.onDragOver).toBeTypeOf('function')
|
|
expect(node.onDragDrop).toBeTypeOf('function')
|
|
|
|
node.onRemoved?.call(node)
|
|
|
|
expect(previousOnRemoved).toHaveBeenCalledTimes(1)
|
|
expect(node.onDragOver).toBeUndefined()
|
|
expect(node.onDragDrop).toBeUndefined()
|
|
})
|
|
|
|
it('onRemoved preserves handlers replaced by another extension', () => {
|
|
const node = createNode()
|
|
useNodeDragAndDrop(node, { onDrop: vi.fn().mockResolvedValue([]) })
|
|
|
|
const replacementDragOver = vi.fn()
|
|
const replacementDragDrop = vi.fn()
|
|
node.onDragOver = replacementDragOver
|
|
node.onDragDrop = replacementDragDrop
|
|
|
|
node.onRemoved?.call(node)
|
|
|
|
expect(node.onDragOver).toBe(replacementDragOver)
|
|
expect(node.onDragDrop).toBe(replacementDragDrop)
|
|
})
|
|
})
|