Compare commits

...

8 Commits

Author SHA1 Message Date
bymyself
93bb2d0d6f fix: restore positionBatchNodes/selectItems for multi-image and deduplicate getFileType
- Add back positionBatchNodes and selectItems calls in multi-image branch
- Extract duplicate getFileType into a module-level function
2026-03-12 08:06:30 -07:00
bymyself
f04809d124 test: fix undo test to call changeTracker.undo() directly
Amp-Thread-ID: https://ampcode.com/threads/T-019cbb7e-7e09-7668-bac9-ff06ee44a8e4
2026-03-04 21:55:12 -08:00
bymyself
6171bd9ac4 test: fix batch image import E2E tests for CI reliability
Amp-Thread-ID: https://ampcode.com/threads/T-019cbb7e-7e09-7668-bac9-ff06ee44a8e4
2026-03-04 21:45:36 -08:00
GitHub Action
218cf60f5f [automated] Apply ESLint and Oxfmt fixes 2026-03-05 05:26:04 +00:00
bymyself
fc56f7ee85 test: add E2E tests for batch image import and undo
Amp-Thread-ID: https://ampcode.com/threads/T-019cbb7e-7e09-7668-bac9-ff06ee44a8e4
2026-03-04 21:23:34 -08:00
bymyself
c664b5bc38 fix: update test mock canvas with emitBeforeChange/emitAfterChange and correct selectItems assertion
Amp-Thread-ID: https://ampcode.com/threads/T-019cbb71-dfcd-772a-8995-5fc69ae59f4b
2026-03-04 16:45:13 -08:00
GitHub Action
724d60822a [automated] Apply ESLint and Oxfmt fixes 2026-03-04 21:22:34 +00:00
bymyself
a15d3ce49b fix: batch image import into single undo entry
Wrap handleFileList in emitBeforeChange()/emitAfterChange() so
dropping multiple images creates one undo entry instead of one per
node. Add JSDoc to the undocumented batching APIs and unit tests
for ChangeTracker batching.
2026-03-04 13:20:17 -08:00
7 changed files with 336 additions and 31 deletions

View File

@@ -4,6 +4,17 @@ import type { Page } from '@playwright/test'
import type { Position } from '../types'
function getFileType(fileName: string): string {
if (fileName.endsWith('.png')) return 'image/png'
if (fileName.endsWith('.svg')) return 'image/svg+xml'
if (fileName.endsWith('.webp')) return 'image/webp'
if (fileName.endsWith('.webm')) return 'video/webm'
if (fileName.endsWith('.json')) return 'application/json'
if (fileName.endsWith('.glb')) return 'model/gltf-binary'
if (fileName.endsWith('.avif')) return 'image/avif'
return 'application/octet-stream'
}
export class DragDropHelper {
constructor(
private readonly page: Page,
@@ -48,17 +59,6 @@ export class DragDropHelper {
const filePath = this.assetPath(fileName)
const buffer = readFileSync(filePath)
const getFileType = (fileName: string) => {
if (fileName.endsWith('.png')) return 'image/png'
if (fileName.endsWith('.svg')) return 'image/svg+xml'
if (fileName.endsWith('.webp')) return 'image/webp'
if (fileName.endsWith('.webm')) return 'video/webm'
if (fileName.endsWith('.json')) return 'application/json'
if (fileName.endsWith('.glb')) return 'model/gltf-binary'
if (fileName.endsWith('.avif')) return 'image/avif'
return 'application/octet-stream'
}
evaluateParams.fileName = fileName
evaluateParams.fileType = getFileType(fileName)
evaluateParams.buffer = [...new Uint8Array(buffer)]
@@ -155,6 +155,104 @@ export class DragDropHelper {
await this.nextFrame()
}
async dragAndDropFiles(
fileNames: string[],
options: {
dropPosition?: Position
waitForUploadCount?: number
} = {}
): Promise<void> {
const { dropPosition = { x: 100, y: 100 }, waitForUploadCount = 0 } =
options
const files = fileNames.map((fileName) => {
const filePath = this.assetPath(fileName)
const buffer = readFileSync(filePath)
return {
fileName,
fileType: getFileType(fileName),
buffer: [...new Uint8Array(buffer)]
}
})
let uploadResponsePromise: Promise<unknown> | null = null
if (waitForUploadCount > 0) {
let uploadCount = 0
uploadResponsePromise = new Promise<void>((resolve) => {
const handler = (resp: { url(): string; status(): number }) => {
if (resp.url().includes('/upload/') && resp.status() === 200) {
uploadCount++
if (uploadCount >= waitForUploadCount) {
this.page.off('response', handler)
resolve()
}
}
}
this.page.on('response', handler)
})
}
await this.page.evaluate(
async (params) => {
const dataTransfer = new DataTransfer()
for (const f of params.files) {
const file = new File([new Uint8Array(f.buffer)], f.fileName, {
type: f.fileType
})
dataTransfer.items.add(file)
}
const targetElement = document.elementFromPoint(
params.dropPosition.x,
params.dropPosition.y
)
if (!targetElement) {
throw new Error(
`No element found at drop position: (${params.dropPosition.x}, ${params.dropPosition.y}).`
)
}
const eventOptions = {
bubbles: true,
cancelable: true,
dataTransfer,
clientX: params.dropPosition.x,
clientY: params.dropPosition.y
}
const graphCanvasElement = document.querySelector('#graph-canvas')
if (graphCanvasElement && !graphCanvasElement.contains(targetElement)) {
graphCanvasElement.dispatchEvent(
new DragEvent('dragover', eventOptions)
)
}
const dropEvent = new DragEvent('drop', eventOptions)
Object.defineProperty(dropEvent, 'preventDefault', {
value: () => {},
writable: false
})
Object.defineProperty(dropEvent, 'stopPropagation', {
value: () => {},
writable: false
})
targetElement.dispatchEvent(new DragEvent('dragover', eventOptions))
targetElement.dispatchEvent(dropEvent)
},
{ files, dropPosition }
)
if (uploadResponsePromise) {
await uploadResponsePromise
}
await this.nextFrame()
}
async dragAndDropFile(
fileName: string,
options: { dropPosition?: Position; waitForUpload?: boolean } = {}

View File

@@ -0,0 +1,90 @@
import { expect } from '@playwright/test'
import { comfyPageFixture as test } from '../fixtures/ComfyPage'
import type { WorkspaceStore } from '../types/globals'
test.describe('Batch Image Import', () => {
test('Dropping multiple images creates LoadImage nodes and a BatchImagesNode', async ({
comfyPage
}) => {
const initialCount = await comfyPage.nodeOps.getGraphNodesCount()
await comfyPage.dragDrop.dragAndDropFiles(
['image32x32.webp', 'image64x64.webp'],
{ waitForUploadCount: 2 }
)
await expect
.poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 10000 })
.toBe(initialCount + 3)
const batchNodes =
await comfyPage.nodeOps.getNodeRefsByType('BatchImagesNode')
expect(batchNodes).toHaveLength(1)
})
test('Dropping a single image does not create a BatchImagesNode', async ({
comfyPage
}) => {
const initialCount = await comfyPage.nodeOps.getGraphNodesCount()
await comfyPage.dragDrop.dragAndDropFile('image32x32.webp', {
waitForUpload: true
})
await expect
.poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 10000 })
.toBe(initialCount + 1)
const batchNodes =
await comfyPage.nodeOps.getNodeRefsByType('BatchImagesNode')
expect(batchNodes).toHaveLength(0)
})
test('Batch image import produces a single undo entry', async ({
comfyPage
}) => {
const initialCount = await comfyPage.nodeOps.getGraphNodesCount()
const initialUndoSize = await comfyPage.workflow.getUndoQueueSize()
await comfyPage.dragDrop.dragAndDropFiles(
['image32x32.webp', 'image64x64.webp'],
{ waitForUploadCount: 2 }
)
await expect
.poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 10000 })
.toBe(initialCount + 3)
await expect
.poll(() => comfyPage.workflow.getUndoQueueSize(), { timeout: 5000 })
.toBe((initialUndoSize ?? 0) + 1)
})
test('Batch image import can be undone as a single action', async ({
comfyPage
}) => {
const initialCount = await comfyPage.nodeOps.getGraphNodesCount()
await comfyPage.dragDrop.dragAndDropFiles(
['image32x32.webp', 'image64x64.webp'],
{ waitForUploadCount: 2 }
)
await expect
.poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 10000 })
.toBe(initialCount + 3)
// Call undo directly on the change tracker to avoid keyboard focus issues
await comfyPage.page.evaluate(async () => {
const workflow = (window.app!.extensionManager as WorkspaceStore).workflow
.activeWorkflow
await workflow?.changeTracker.undo()
})
await comfyPage.nextFrame()
await expect
.poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 10000 })
.toBe(initialCount)
})
})

View File

@@ -3972,14 +3972,41 @@ export class LGraphCanvas implements CustomEventDispatcher<LGraphCanvasEventMap>
)
}
/** @todo Refactor to where it belongs - e.g. Deleting / creating nodes is not actually canvas event. */
/**
* Signals the start of a compound graph operation. All graph mutations
* between this call and the matching {@link emitAfterChange} are treated
* as a single undoable action by the change tracking system.
*
* Emits a `litegraph:canvas` DOM event with `subType: 'before-change'`,
* which `ChangeTracker` listens for to suppress intermediate state
* snapshots. Calls are nestable — only the outermost pair triggers a
* state check.
*
* Always pair with {@link emitAfterChange} in a `try/finally` block.
*
* @example
* ```ts
* canvas.emitBeforeChange()
* try {
* // multiple graph mutations...
* } finally {
* canvas.emitAfterChange()
* }
* ```
*/
emitBeforeChange(): void {
this.emitEvent({
subType: 'before-change'
})
}
/** @todo See {@link emitBeforeChange} */
/**
* Signals the end of a compound graph operation started by
* {@link emitBeforeChange}. When the outermost pair completes, the
* change tracking system takes a single state snapshot and records
* one undo entry for all mutations since the matching
* `emitBeforeChange`.
*/
emitAfterChange(): void {
this.emitEvent({
subType: 'after-change'

View File

@@ -66,7 +66,9 @@ function createMockCanvas(): Partial<LGraphCanvas> {
return {
graph: mockGraph as LGraph,
selectItems: vi.fn()
selectItems: vi.fn(),
emitBeforeChange: vi.fn(),
emitAfterChange: vi.fn()
}
}
@@ -102,13 +104,9 @@ describe('ComfyApp', () => {
expect(pasteImageNodes).toHaveBeenCalledWith(mockCanvas, files)
expect(createNode).toHaveBeenCalledWith(mockCanvas, 'BatchImagesNode')
expect(mockCanvas.selectItems).toHaveBeenCalledWith([
mockNode1,
mockNode2,
mockBatchNode
])
expect(mockNode1.connect).toHaveBeenCalledWith(0, mockBatchNode, 0)
expect(mockNode2.connect).toHaveBeenCalledWith(0, mockBatchNode, 1)
expect(mockCanvas.selectItems).not.toHaveBeenCalled()
})
it('should select single image node without batch node', async () => {

View File

@@ -1720,21 +1720,26 @@ export class ComfyApp {
if (fileList.length === 0) return
if (!fileList[0].type.startsWith('image')) return
const imageNodes = await pasteImageNodes(this.canvas, fileList)
if (imageNodes.length === 0) return
this.canvas.emitBeforeChange()
try {
const imageNodes = await pasteImageNodes(this.canvas, fileList)
if (imageNodes.length === 0) return
if (imageNodes.length > 1) {
const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode')
if (!batchImagesNode) return
if (imageNodes.length > 1) {
const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode')
if (!batchImagesNode) return
this.positionBatchNodes(imageNodes, batchImagesNode)
this.canvas.selectItems([...imageNodes, batchImagesNode])
imageNodes.forEach((imageNode, index) => {
imageNode.connect(0, batchImagesNode, index)
})
imageNodes.forEach((imageNode, index) => {
imageNode.connect(0, batchImagesNode, index)
})
} else {
this.canvas.selectItems(imageNodes)
this.positionBatchNodes(imageNodes, batchImagesNode)
this.canvas.selectItems([...imageNodes, batchImagesNode])
} else {
this.canvas.selectItems(imageNodes)
}
} finally {
this.canvas.emitAfterChange()
}
}

View File

@@ -0,0 +1,66 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { ComfyWorkflow } from '@/platform/workflow/management/stores/workflowStore'
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
import { ChangeTracker } from './changeTracker'
vi.mock('@/scripts/app', () => ({
app: {
graph: {},
rootGraph: { serialize: () => ({ nodes: [], links: [] }) },
canvas: { ds: { scale: 1, offset: [0, 0] } }
}
}))
vi.mock('@/scripts/api', () => ({
api: {
dispatchCustomEvent: vi.fn(),
apiURL: vi.fn((path: string) => path)
}
}))
vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({
useWorkflowStore: () => ({ getWorkflowByPath: () => null })
}))
vi.mock('@/stores/subgraphNavigationStore', () => ({
useSubgraphNavigationStore: () => ({ exportState: () => [] })
}))
describe('ChangeTracker', () => {
let tracker: ChangeTracker
beforeEach(() => {
const mockWorkflow = { path: 'test.json' } as unknown as ComfyWorkflow
const initialState = {
version: 1,
nodes: [],
links: [],
last_node_id: 0,
last_link_id: 0
} as unknown as ComfyWorkflowJSON
tracker = new ChangeTracker(mockWorkflow, initialState)
})
describe('beforeChange / afterChange batching', () => {
it('calls checkState only when outermost afterChange completes', () => {
const checkStateSpy = vi.spyOn(tracker, 'checkState')
tracker.beforeChange()
tracker.afterChange()
expect(checkStateSpy).toHaveBeenCalledOnce()
})
it('suppresses checkState for nested calls until fully unwound', () => {
const checkStateSpy = vi.spyOn(tracker, 'checkState')
tracker.beforeChange()
tracker.beforeChange()
tracker.afterChange()
expect(checkStateSpy).not.toHaveBeenCalled()
tracker.afterChange()
expect(checkStateSpy).toHaveBeenCalledOnce()
})
})
})

View File

@@ -34,6 +34,13 @@ export class ChangeTracker {
activeState: ComfyWorkflowJSON
undoQueue: ComfyWorkflowJSON[] = []
redoQueue: ComfyWorkflowJSON[] = []
/**
* Nesting counter for compound operations. While greater than zero,
* {@link checkState} is suppressed. Incremented by {@link beforeChange},
* decremented by {@link afterChange}. When it returns to zero,
* `checkState()` runs and captures a single undo entry for all mutations
* since the first `beforeChange`.
*/
changeCount: number = 0
/**
* Whether the redo/undo restoring is in progress.
@@ -203,10 +210,24 @@ export class ChangeTracker {
}
}
/**
* Marks the start of a compound operation. Increments the nesting
* counter to suppress {@link checkState} until the matching
* {@link afterChange} call completes.
*
* Typically called via `LGraphCanvas.emitBeforeChange()` through the
* `litegraph:canvas` DOM event, rather than directly.
*/
beforeChange() {
this.changeCount++
}
/**
* Marks the end of a compound operation. Decrements the nesting
* counter; when it reaches zero, calls {@link checkState} to capture
* a single undo entry for all mutations since the first
* {@link beforeChange}.
*/
afterChange() {
if (!--this.changeCount) {
this.checkState()