Batch drag and drop #8246
Closed
Batch drag and drop #8246
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
the
size:L
Contributor
📝 WalkthroughWalkthroughConverted image paste flow to async, added a reusable Changes
Sequence DiagramsequenceDiagram
actor User
participant DragDrop as Drag/Drop Handler
participant EventUtils as eventUtils
participant App as ComfyApp
participant UsePaste as usePaste
participant NodeUtils as litegraphUtil
participant Graph as LiteGraph
User->>DragDrop: Drop multiple images
DragDrop->>EventUtils: extractFileFromDragEvent(event)
EventUtils-->>DragDrop: Return FileList
DragDrop->>App: handleFileList(fileList)
loop per file
App->>UsePaste: pasteImageNode(canvas, items)
UsePaste->>NodeUtils: createNode(canvas, "LoadImage")
NodeUtils->>Graph: LiteGraph.createNode(...)
Graph-->>NodeUtils: node instance
NodeUtils->>Graph: add node / redraw
NodeUtils-->>UsePaste: Promise<LGraphNode>
UsePaste-->>App: LGraphNode
end
App->>App: positionNodes(nodes, batchNode)
App->>Graph: update node positions
Graph-->>User: Render positioned nodes
Possibly related PRs
Suggested reviewers
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook Build Status
⏰ Started at: 01/22/2026, 10:46:03 PM UTC 🚀 Building Storybook
⏱️ Please wait while the Storybook build is in progress... |
🎭 Playwright Tests: ⏳ Running...Tests started at 01/22/2026, 10:45:48 PM UTC 📊 Browser Tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/composables/usePaste.test.ts`:
- Around line 107-121: Replace unsafe casts like "as unknown as LGraphNode" used
for test mocks with explicit partial casts "Partial<LGraphNode> as LGraphNode";
update occurrences where createMockNode returns mockNode and anywhere tests call
createNode and assert mockNode.pasteFile (including the other test blocks
referenced) so the mocks are typed as Partial<LGraphNode> as LGraphNode (e.g.,
change the mockNode declaration/return and any similar casts around
createMockNode, mockNode, createNode, and pasteImageNode uses).
In `@src/composables/usePaste.ts`:
- Line 127: Awaiting pasteImageNode can throw and must be wrapped in try/catch;
modify the call in the function that currently does "await pasteImageNode(canvas
as LGraphCanvas, items, imageNode)" to catch errors, send them to the existing
UI error reporting function (use the project's error reporting helper) and
present a user-visible message via i18n (use the current i18n instance/keys).
Ensure the catch logs/debug-reports the original error and shows a localized
message to the user.
In `@src/scripts/app.ts`:
- Around line 1554-1561: Remove the per-node debug logging and avoid calling
graph.change() inside the loop: in the nodes.forEach callback (where node.pos is
set) delete the console.log('index:', index, 'pos:', node.pos) and move the
this.canvas.graph?.change() call to run once after the forEach completes so the
graph is updated only once after all node positions are set.
- Around line 1531-1552: The code accesses nodes[0] in positionNodes without
guarding for empty arrays which can throw when pasteImageNodes returns no nodes
or fileList is empty; update handleFileList to check fileList.length and that
imageNodes is non-empty (and batchImagesNode exists) before calling
positionNodes, and add an early-return at the start of positionNodes to return
if nodes is falsy or nodes.length === 0; reference functions/variables:
handleFileList, pasteImageNodes, createNode, imageNodes, batchImagesNode, and
positionNodes.
In `@src/utils/eventUtils.ts`:
- Around line 6-11: The helper `hasImageType` is currently declared as a const
after its use which risks TDZ in circular-import scenarios and uses the
unnecessary Boolean wrapper; change `const hasImageType = ...` into a hoisted
function declaration `function hasImageType(...) : boolean` that returns a
boolean directly, update any other occurrence (the second use noted later in the
file) to rely on that hoisted function, and remove any `Boolean(...)` wrapping
so callers like the Array.from(...).every(hasImageType) invoke a plain
boolean-returning function.
In `@src/utils/litegraphUtil.test.ts`:
- Around line 31-33: The tests use untyped casts (mockGraph: any, as unknown as
LGraphNode, options: any, null as any, and { addAlert: mockAddAlert } as any);
replace these with proper typed partial mocks using TypeScript generics (e.g.,
mockCanvas: Partial<LGraphCanvas>, mockGraph: Partial<LGraphNode> or a concrete
interface for the mocked shape, options: Partial<YourOptionsType>, null values
typed as null as unknown as ExpectedType when necessary, and { addAlert:
mockAddAlert } as Partial<AlertService>), updating all occurrences (including
lines around mock declarations and the areas flagged at 46-56 and 81-92) to
remove any/unknown and preserve type safety while keeping only the properties
used by the tests.
In `@src/utils/litegraphUtil.ts`:
- Around line 39-64: The createNode helper can hang because it awaits a Promise
that only resolves via onNodeCreated; if LiteGraph.createNode returns null or
doesn't invoke onNodeCreated the promise never resolves—update createNode to
synchronously check the return of LiteGraph.createNode (the newNode variable)
and immediately resolve/short-circuit when it is null (or alternatively
implement a short timeout fallback) instead of only relying on the onNodeCreated
callback; ensure you still set pos, add to graph via graph.add(newNode) and call
graph.change() only when a valid LGraphNode was actually created.
Comment on lines
+107
to
121
| it('should create new LoadImage node when no image node provided', async () => { | ||
| const mockNode = createMockNode() | ||
| vi.mocked(LiteGraph.createNode).mockReturnValue( | ||
| mockNode as unknown as LGraphNode | ||
| ) | ||
| vi.mocked(createNode).mockResolvedValue(mockNode as unknown as LGraphNode) | ||
|
|
||
| const file = createImageFile() | ||
| const dataTransfer = createDataTransfer([file]) | ||
|
|
||
| pasteImageNode(mockCanvas as unknown as LGraphCanvas, dataTransfer.items) | ||
| await pasteImageNode( | ||
| mockCanvas as unknown as LGraphCanvas, | ||
| dataTransfer.items | ||
| ) | ||
|
|
||
| expect(LiteGraph.createNode).toHaveBeenCalledWith('LoadImage') | ||
| expect(mockNode.pos).toEqual([100, 200]) | ||
| expect(mockCanvas.graph!.add).toHaveBeenCalledWith(mockNode) | ||
| expect(mockCanvas.graph!.change).toHaveBeenCalled() | ||
| expect(createNode).toHaveBeenCalledWith(mockCanvas, 'LoadImage') | ||
| expect(mockNode.pasteFile).toHaveBeenCalledWith(file) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer Partial<LGraphNode> casts for mock nodes.
Several tests cast mock objects via as unknown as LGraphNode. Use Partial<LGraphNode> as LGraphNode to make incomplete mocks explicit and safer. Based on learnings, prefer Partial<Interface> as Interface for partial mocks in tests.
♻️ Proposed refactor
- vi.mocked(createNode).mockResolvedValue(mockNode as unknown as LGraphNode)
+ vi.mocked(createNode).mockResolvedValue(
+ mockNode as Partial<LGraphNode> as LGraphNode
+ )
@@
- const mockNode = {
- is_selected: true,
- pasteFile: vi.fn(),
- pasteFiles: vi.fn()
- } as unknown as Partial<LGraphNode> as LGraphNode
+ const mockNode = {
+ is_selected: true,
+ pasteFile: vi.fn(),
+ pasteFiles: vi.fn()
+ } as Partial<LGraphNode> as LGraphNodeAlso applies to: 190-212, 237-250, 302-309
🤖 Prompt for AI Agents
In `@src/composables/usePaste.test.ts` around lines 107 - 121, Replace unsafe
casts like "as unknown as LGraphNode" used for test mocks with explicit partial
casts "Partial<LGraphNode> as LGraphNode"; update occurrences where
createMockNode returns mockNode and anywhere tests call createNode and assert
mockNode.pasteFile (including the other test blocks referenced) so the mocks are
typed as Partial<LGraphNode> as LGraphNode (e.g., change the mockNode
declaration/return and any similar casts around createMockNode, mockNode,
createNode, and pasteImageNode uses).
| for (const item of items) { | ||
| if (item.type.startsWith('image/')) { | ||
| pasteImageNode(canvas as LGraphCanvas, items, imageNode) | ||
| await pasteImageNode(canvas as LGraphCanvas, items, imageNode) |
Contributor
There was a problem hiding this comment.
Handle errors from async pasteImageNode to avoid unhandled rejections.
Line 127 now awaits an async path that can throw (e.g., node creation failure). Wrap the await in a try/catch and route errors through your existing UI error reporting mechanism; ensure any user-visible message uses i18n.
🤖 Prompt for AI Agents
In `@src/composables/usePaste.ts` at line 127, Awaiting pasteImageNode can throw
and must be wrapped in try/catch; modify the call in the function that currently
does "await pasteImageNode(canvas as LGraphCanvas, items, imageNode)" to catch
errors, send them to the existing UI error reporting function (use the project's
error reporting helper) and present a user-visible message via i18n (use the
current i18n instance/keys). Ensure the catch logs/debug-reports the original
error and shows a localized message to the user.
Comment on lines
+1531
to
+1552
| async handleFileList(fileList: FileList) { | ||
| if (fileList[0].type.startsWith('image')) { | ||
| const imageNodes = await pasteImageNodes(this.canvas, fileList) | ||
| const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode') | ||
| if (!batchImagesNode) return | ||
|
|
||
| this.positionNodes(imageNodes, batchImagesNode) | ||
|
|
||
| Array.from(imageNodes).forEach((imageNode, index) => { | ||
| imageNode.connect(0, batchImagesNode, index) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Positions batched nodes in drag and drop | ||
| * @param nodes | ||
| * @param batchNode | ||
| */ | ||
| positionNodes(nodes: LGraphNode[], batchNode: LGraphNode) { | ||
| const [x, y, width, height] = nodes[0].getBounding() | ||
| batchNode.pos = [ x + width + 100, y + 30 ] |
Contributor
There was a problem hiding this comment.
Guard against empty FileList/imageNodes before indexing.
positionNodes reads nodes[0] unconditionally. If fileList is empty or pasteImageNodes returns an empty array (e.g., node creation failure), this will throw and abort the drop flow. Add early returns before accessing nodes[0].
🐛 Proposed fix
async handleFileList(fileList: FileList) {
- if (fileList[0].type.startsWith('image')) {
+ if (!fileList.length) return
+ if (fileList[0].type.startsWith('image')) {
const imageNodes = await pasteImageNodes(this.canvas, fileList)
+ if (!imageNodes.length) return
const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode')
if (!batchImagesNode) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async handleFileList(fileList: FileList) { | |
| if (fileList[0].type.startsWith('image')) { | |
| const imageNodes = await pasteImageNodes(this.canvas, fileList) | |
| const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode') | |
| if (!batchImagesNode) return | |
| this.positionNodes(imageNodes, batchImagesNode) | |
| Array.from(imageNodes).forEach((imageNode, index) => { | |
| imageNode.connect(0, batchImagesNode, index) | |
| }) | |
| } | |
| } | |
| /** | |
| * Positions batched nodes in drag and drop | |
| * @param nodes | |
| * @param batchNode | |
| */ | |
| positionNodes(nodes: LGraphNode[], batchNode: LGraphNode) { | |
| const [x, y, width, height] = nodes[0].getBounding() | |
| batchNode.pos = [ x + width + 100, y + 30 ] | |
| async handleFileList(fileList: FileList) { | |
| if (!fileList.length) return | |
| if (fileList[0].type.startsWith('image')) { | |
| const imageNodes = await pasteImageNodes(this.canvas, fileList) | |
| if (!imageNodes.length) return | |
| const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode') | |
| if (!batchImagesNode) return | |
| this.positionNodes(imageNodes, batchImagesNode) | |
| Array.from(imageNodes).forEach((imageNode, index) => { | |
| imageNode.connect(0, batchImagesNode, index) | |
| }) | |
| } | |
| } | |
| /** | |
| * Positions batched nodes in drag and drop | |
| * `@param` nodes | |
| * `@param` batchNode | |
| */ | |
| positionNodes(nodes: LGraphNode[], batchNode: LGraphNode) { | |
| const [x, y, width, height] = nodes[0].getBounding() | |
| batchNode.pos = [ x + width + 100, y + 30 ] | |
| } |
🤖 Prompt for AI Agents
In `@src/scripts/app.ts` around lines 1531 - 1552, The code accesses nodes[0] in
positionNodes without guarding for empty arrays which can throw when
pasteImageNodes returns no nodes or fileList is empty; update handleFileList to
check fileList.length and that imageNodes is non-empty (and batchImagesNode
exists) before calling positionNodes, and add an early-return at the start of
positionNodes to return if nodes is falsy or nodes.length === 0; reference
functions/variables: handleFileList, pasteImageNodes, createNode, imageNodes,
batchImagesNode, and positionNodes.
Contributor
Author
There was a problem hiding this comment.
I do that in the method that calls this.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the drop handler that calls handleFileList and look for guards
rg -nP -A15 -B5 'handleFileList' src/scripts/app.ts
# Also check if there are any length checks before calling positionNodes
rg -nP -A3 -B3 'positionNodes' src/scripts/app.ts
# Check the implementation of pasteImageNodes to see if it can return empty array
rg -nP -A10 'export.*pasteImageNodes' src/composables/usePaste.ts
Length of output: 2381
🏁 Script executed:
#!/bin/bash
# Check extractFileFromDragEvent to see if it can return empty FileList
rg -nP -A30 'export.*extractFileFromDragEvent' src/utils/eventUtils.ts
Length of output: 1028
@JemiloII, I've reviewed the code flow and I don't see guards for empty arrays in the calling method. Here's what I found:
Drop handler (line 550-551):
if (fileMaybe instanceof FileList) {
await this.handleFileList(fileMaybe)
}This only checks the type, not the length.
The actual issue is in handleFileList itself:
- Line 1534:
fileList[0].type.startsWith('image')- would throw iffileListis empty - Line 1535:
pasteImageNodescan return an empty array if all node creations fail (e.g., ifcreateNodereturns null for all files) - Line 1539:
positionNodes(imageNodes, ...)is called without checking ifimageNodesis empty - Line 1553 in
positionNodes:nodes[0].getBounding()throws ifnodesis empty
While extractFileFromDragEvent currently won't return an empty FileList (it requires files.length > 1), the more critical issue is that pasteImageNodes can legitimately return an empty array if node creation fails.
The suggested guards are needed to prevent runtime errors when imageNodes ends up empty.
🧬 Code Graph Analysis Results
File: src/utils/graphTraversalUtil.ts (lines 285-307)
export function getNodeByExecutionId(
rootGraph: LGraph,
executionId: string
): LGraphNode | null {
if (!rootGraph) return null
const localNodeId = getLocalNodeIdFromExecutionId(executionId)
if (!localNodeId) return null
const subgraphPath = getSubgraphPathFromExecutionId(executionId)
// If no subgraph path, it's in the root graph
if (subgraphPath.length === 0) {
return rootGraph.getNodeById(localNodeId) || null
}
// Traverse to the target subgraph
const targetGraph = traverseSubgraphPath(rootGraph, subgraphPath)
if (!targetGraph) return null
// Get the node from the target graph
return targetGraph.getNodeById(localNodeId) || null
}
File: src/workbench/utils/modelMetadataUtil.ts (lines 22-51)
export function getSelectedModelsMetadata(node: {
type: string
widgets_values?: unknown[] | Record<string, unknown>
properties?: { models?: ModelFile[] }
}): ModelFile[] | undefined {
try {
if (!node.properties?.models?.length) return
if (!node.widgets_values) return
const widgetValues = Array.isArray(node.widgets_values)
? node.widgets_values
: Object.values(node.widgets_values)
if (!widgetValues.length) return
const stringWidgetValues = new Set<string>()
for (const widgetValue of widgetValues) {
if (typeof widgetValue === 'string' && widgetValue.trim()) {
stringWidgetValues.add(widgetValue)
}
}
// Return the node's models that are present in the widget values
return node.properties.models.filter((model) =>
stringWidgetValues.has(model.name)
)
} catch (error) {
console.error('Error filtering models by current selection:', error)
}
}
File: src/composables/usePaste.ts (lines 84-96)
export async function pasteImageNode(
canvas: LGraphCanvas,
items: DataTransferItemList,
imageNode: LGraphNode | null = null
): Promise<LGraphNode | null> {
if (!imageNode) {
// No image node selected: add a new one
imageNode = await createNode(canvas, 'LoadImage')
}
pasteItemsOnNode(items, imageNode, 'image')
return imageNode
}
File: src/composables/usePaste.ts (lines 98-115)
export async function pasteImageNodes(
canvas: LGraphCanvas,
fileList: FileList
): Promise<LGraphNode[]> {
const nodes: LGraphNode[] = [];
for (const file of fileList) {
const transfer = new DataTransfer()
transfer.items.add(file)
const imageNode = await pasteImageNode(canvas, transfer.items)
if (imageNode) {
nodes.push(imageNode)
}
}
return nodes
}
File: src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts (lines 15-166)
export function ensureCorrectLayoutScale(
renderer: RendererType = 'LG',
targetGraph?: LGraph
) {
const autoScaleLayoutSetting = useSettingStore().get(
'Comfy.VueNodes.AutoScaleLayout'
)
if (!autoScaleLayoutSetting) return
const canvas = comfyApp.canvas
const graph = targetGraph ?? canvas?.graph
if (!graph?.nodes) return
const { shouldRenderVueNodes } = useVueFeatureFlags()
const needsUpscale = renderer === 'LG' && shouldRenderVueNodes.value
const needsDownscale = renderer === 'Vue' && !shouldRenderVueNodes.value
if (!needsUpscale && !needsDownscale) {
// Don't scale, but ensure workflowRendererVersion is set for future checks
graph.extra.workflowRendererVersion ??= renderer
return
}
const lgBounds = createBounds(graph.nodes)
if (!lgBounds) return
const [originX, originY] = lgBounds
const lgNodesById = new Map(graph.nodes.map((node) => [node.id, node]))
const yjsMoveNodeUpdates: NodeBoundsUpdate[] = []
const scaleFactor = needsUpscale ? SCALE_FACTOR : 1 / SCALE_FACTOR
const onActiveGraph = !targetGraph || targetGraph === canvas?.graph
//TODO: once we remove the need for LiteGraph.NODE_TITLE_HEIGHT in vue nodes we nned to remove everything here.
for (const node of graph.nodes) {
const lgNode = lgNodesById.get(node.id)
if (!lgNode) continue
const [oldX, oldY] = lgNode.pos
const relativeX = oldX - originX
const relativeY = oldY - originY
const scaledX = originX + relativeX * scaleFactor
const scaledY = originY + relativeY * scaleFactor
const scaledWidth = lgNode.width * scaleFactor
const scaledHeight = needsUpscale
? lgNode.size[1] * scaleFactor + LiteGraph.NODE_TITLE_HEIGHT
: (lgNode.size[1] - LiteGraph.NODE_TITLE_HEIGHT) * scaleFactor
// Directly update LiteGraph node to ensure immediate consistency
// Dont need to reference vue directly because the pos and dims are already in yjs
lgNode.pos[0] = scaledX
lgNode.pos[1] = scaledY
lgNode.size[0] = scaledWidth
lgNode.size[1] = scaledHeight
// Track updates for layout store (only if this is the active graph)
if (onActiveGraph) {
yjsMoveNodeUpdates.push({
nodeId: String(lgNode.id),
bounds: {
x: scaledX,
y: scaledY,
width: scaledWidth,
height: scaledHeight
}
})
}
}
if (onActiveGraph && yjsMoveNodeUpdates.length > 0) {
layoutStore.batchUpdateNodeBounds(yjsMoveNodeUpdates)
}
for (const reroute of graph.reroutes.values()) {
const [oldX, oldY] = reroute.pos
const relativeX = oldX - originX
const relativeY = oldY - originY
const scaledX = originX + relativeX * scaleFactor
const scaledY = originY + relativeY * scaleFactor
reroute.pos = [scaledX, scaledY]
if (onActiveGraph && shouldRenderVueNodes.value) {
const layoutMutations = useLayoutMutations()
layoutMutations.moveReroute(
reroute.id,
{ x: scaledX, y: scaledY },
{ x: oldX, y: oldY }
)
}
}
if ('inputNode' in graph && 'outputNode' in graph) {
const ioNodes = [
graph.inputNode as SubgraphInputNode,
graph.outputNode as SubgraphOutputNode
]
for (const ioNode of ioNodes) {
const [oldX, oldY] = ioNode.pos
const [oldWidth, oldHeight] = ioNode.size
const relativeX = oldX - originX
const relativeY = oldY - originY
const scaledX = originX + relativeX * scaleFactor
const scaledY = originY + relativeY * scaleFactor
const scaledWidth = oldWidth * scaleFactor
const scaledHeight = oldHeight * scaleFactor
ioNode.pos = [scaledX, scaledY]
ioNode.size = [scaledWidth, scaledHeight]
}
}
graph.groups.forEach((group) => {
const [oldX, oldY] = group.pos
const [oldWidth, oldHeight] = group.size
const relativeX = oldX - originX
const relativeY = oldY - originY
const scaledX = originX + relativeX * scaleFactor
const scaledY = originY + relativeY * scaleFactor
const scaledWidth = oldWidth * scaleFactor
const scaledHeight = oldHeight * scaleFactor
group.pos = [scaledX, scaledY]
group.size = [scaledWidth, scaledHeight]
})
if (onActiveGraph && canvas) {
const originScreen = canvas.ds.convertOffsetToCanvas([originX, originY])
canvas.ds.changeScale(canvas.ds.scale / scaleFactor, originScreen)
}
graph.extra.workflowRendererVersion = needsUpscale ? 'Vue' : 'LG'
}
Comment on lines
+6
to
+11
| const { files } = event.dataTransfer | ||
| // Dragging from Chrome->Firefox there is a file, but it's a bmp, so ignore it | ||
| if (files.length === 1 && files[0].type !== 'image/bmp') { | ||
| return files[0] | ||
| } else if (files.length > 1 && Array.from(files).every(hasImageType)) { | ||
| return files |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a hoisted function declaration for hasImageType and return boolean.
const hasImageType is defined after use; in circular-import scenarios this can hit TDZ, and the Boolean wrapper type is unnecessary. Convert to a function declaration returning boolean. As per coding guidelines, prefer function declarations for pure helpers.
♻️ Proposed refactor
-const hasImageType = ({ type }: File): Boolean => type.startsWith('image');
+function hasImageType({ type }: File): boolean {
+ return type.startsWith('image')
+}Also applies to: 29-29
🤖 Prompt for AI Agents
In `@src/utils/eventUtils.ts` around lines 6 - 11, The helper `hasImageType` is
currently declared as a const after its use which risks TDZ in circular-import
scenarios and uses the unnecessary Boolean wrapper; change `const hasImageType =
...` into a hoisted function declaration `function hasImageType(...) : boolean`
that returns a boolean directly, update any other occurrence (the second use
noted later in the file) to rely on that hoisted function, and remove any
`Boolean(...)` wrapping so callers like the Array.from(...).every(hasImageType)
invoke a plain boolean-returning function.
Comment on lines
+31
to
+33
| let mockCanvas: Partial<LGraphCanvas> | ||
| let mockGraph: any | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "litegraphUtil.test.ts"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 101
🏁 Script executed:
wc -l src/utils/litegraphUtil.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 105
🏁 Script executed:
cat -n src/utils/litegraphUtil.test.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3607
🏁 Script executed:
cat -n src/utils/litegraphUtil.test.ts | tail -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3414
🏁 Script executed:
cat -n src/utils/litegraphUtil.test.ts | sed -n '80,105p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1142
🏁 Script executed:
rg "export.*type.*LGraph[^C]" src/lib/litegraph/src/litegraph.ts | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 188
🏁 Script executed:
rg "export.*LGraph\b" src/lib/litegraph/src/litegraph.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "export.*class LGraph|export.*type LGraph\b" src/lib/litegraph/src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 707
🏁 Script executed:
grep -n "LGraph\b" src/lib/litegraph/src/litegraph.ts | grep -E "(export|import)" | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
cat src/lib/litegraph/src/litegraph.ts | grep -A 5 -B 5 "LGraph"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2316
🏁 Script executed:
cat src/utils/litegraphUtil.ts | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1362
Replace any and unknown casts with properly typed mocks to maintain type safety.
The test file uses mockGraph: any, as unknown as LGraphNode, options: any, null as any, and { addAlert: mockAddAlert } as any. Replace these with typed mocks and Partial<...> casts for incomplete objects, following the coding guidelines and test best practices.
♻️ Proposed refactor
-import type { LGraphCanvas, LGraphNode } from '@/lib/litegraph/src/litegraph'
+import type { LGraph, LGraphCanvas, LGraphNode } from '@/lib/litegraph/src/litegraph'
@@
- let mockGraph: any
+ let mockGraph: Partial<LGraph>
@@
- const mockNode = {
- pos: [0, 0]
- } as unknown as LGraphNode
+ const mockNode = {
+ pos: [0, 0]
+ } as Partial<LGraphNode> as LGraphNode
@@
- vi.mocked(LiteGraph.createNode).mockImplementation(
- (_name, _title, options: any) => {
+ vi.mocked(LiteGraph.createNode).mockImplementation(
+ (_name, _title, options?: { onNodeCreated?: () => void }) => {
setTimeout(() => options?.onNodeCreated?.(), 0)
return mockNode
}
)
@@
- const result = await createNode(mockCanvas as LGraphCanvas, null as any)
+ const result = await createNode(
+ mockCanvas as LGraphCanvas,
+ null as unknown as string
+ )
@@
- vi.mocked(useToastStore).mockReturnValue({ addAlert: mockAddAlert } as any)
+ type ToastStore = { addAlert: (message: string) => void }
+ vi.mocked(useToastStore).mockReturnValue(
+ { addAlert: mockAddAlert } as ToastStore
+ )Also applies to: 46-56, 81-92
🤖 Prompt for AI Agents
In `@src/utils/litegraphUtil.test.ts` around lines 31 - 33, The tests use untyped
casts (mockGraph: any, as unknown as LGraphNode, options: any, null as any, and
{ addAlert: mockAddAlert } as any); replace these with proper typed partial
mocks using TypeScript generics (e.g., mockCanvas: Partial<LGraphCanvas>,
mockGraph: Partial<LGraphNode> or a concrete interface for the mocked shape,
options: Partial<YourOptionsType>, null values typed as null as unknown as
ExpectedType when necessary, and { addAlert: mockAddAlert } as
Partial<AlertService>), updating all occurrences (including lines around mock
declarations and the areas flagged at 46-56 and 81-92) to remove any/unknown and
preserve type safety while keeping only the properties used by the tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/scripts/app.ts`:
- Around line 1447-1449: Remove the debug console.log call that prints
transfer.items before creating the image node; specifically delete the line
console.log('transfer items', transfer.items) so the flow proceeds directly to
calling createNode(this.canvas, 'LoadImage') and pasteImageNode(this.canvas,
transfer.items, imageNode) without logging.
In `@src/utils/litegraphUtil.ts`:
- Around line 53-57: The condition uses an unnecessary type assertion "if
(newNode as LGraphNode | null)" and then uses non-null assertions (newNode!)
inside; change the guard to a normal truthiness check "if (newNode)" so
TypeScript narrows newNode to LGraphNode, remove the "!" usages (use
newNode.pos, graph?.add(newNode)), keep the existing graph?.change() call and
return value (addedNode) unchanged; adjust only in the function containing
newNode and graph references in litegraphUtil.ts.
♻️ Duplicate comments (2)
src/scripts/app.ts (2)
1533-1544: Guard against emptyFileListandimageNodesbefore indexing.Accessing
fileList[0]without checking length will throw ifFileListis empty. Similarly, ifpasteImageNodesreturns an empty array (e.g., all node creations failed),positionNodeswill fail when accessingnodes[0].🐛 Proposed fix
async handleFileList(fileList: FileList) { + if (!fileList.length) return if (fileList[0].type.startsWith('image')) { const imageNodes = await pasteImageNodes(this.canvas, fileList) + if (!imageNodes.length) return const batchImagesNode = await createNode(this.canvas, 'BatchImagesNode') if (!batchImagesNode) return
1552-1561: Guard against emptynodesarray and batchgraph.change()call.
positionNodesaccessesnodes[0]unconditionally, which will throw if the array is empty. Additionally,graph.change()is called inside the loop on every iteration—this should be called once after all positions are set.🐛 Proposed fix
positionNodes(nodes: LGraphNode[], batchNode: LGraphNode) { + if (!nodes.length) return const [x, y, width, height] = nodes[0].getBounding() batchNode.pos = [ x + width + 100, y + 30 ] nodes.forEach((node, index) => { if (index > 0) { - node.pos = [ x, y + (height * index) + (25 * (index + 1))]; + node.pos = [ x, y + (height * index) + (25 * (index + 1)) ] } - this.canvas.graph?.change() - }); + }) + this.canvas.graph?.change() }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/composables/usePaste.ts`:
- Around line 23-36: The code currently adds files twice by iterating
original.files and then iterating original.items for kind==='file'; remove the
first loop that iterates original.files (the for (const file of original.files)
{ persistent.items.add(file); } block) and rely on the existing original.items
loop that checks item.kind === 'file' and uses item.getAsFile() before calling
persistent.items.add(file), so each file is only added once.
♻️ Duplicate comments (1)
src/composables/usePaste.ts (1)
159-162: Missing error handling for asyncpasteImageNodecall.The
await pasteImageNode(...)call at line 161 can throw if node creation fails. Wrap in try/catch and report errors via the existing UI mechanism with i18n.
Comment on lines
+23
to
+36
| // Copy files | ||
| for (const file of original.files) { | ||
| persistent.items.add(file); | ||
| } | ||
|
|
||
| // Also handle any file-kind items that might not be in .files | ||
| for (const item of original.items) { | ||
| if (item.kind === 'file') { | ||
| const file = item.getAsFile(); | ||
| if (file) { | ||
| persistent.items.add(file); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate files added when cloning DataTransfer.
The function copies files from original.files (lines 24-26), then again from original.items where kind === 'file' (lines 29-35). Since .files already contains all file-kind items, this results in each file being added twice to the cloned DataTransfer.
🐛 Proposed fix: Remove the redundant files loop
- // Copy files
- for (const file of original.files) {
- persistent.items.add(file);
- }
-
- // Also handle any file-kind items that might not be in .files
+ // Copy file-kind items
for (const item of original.items) {
if (item.kind === 'file') {
const file = item.getAsFile();
if (file) {
persistent.items.add(file);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Copy files | |
| for (const file of original.files) { | |
| persistent.items.add(file); | |
| } | |
| // Also handle any file-kind items that might not be in .files | |
| for (const item of original.items) { | |
| if (item.kind === 'file') { | |
| const file = item.getAsFile(); | |
| if (file) { | |
| persistent.items.add(file); | |
| } | |
| } | |
| } | |
| // Copy file-kind items | |
| for (const item of original.items) { | |
| if (item.kind === 'file') { | |
| const file = item.getAsFile(); | |
| if (file) { | |
| persistent.items.add(file); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/composables/usePaste.ts` around lines 23 - 36, The code currently adds
files twice by iterating original.files and then iterating original.items for
kind==='file'; remove the first loop that iterates original.files (the for
(const file of original.files) { persistent.items.add(file); } block) and rely
on the existing original.items loop that checks item.kind === 'file' and uses
item.getAsFile() before calling persistent.items.add(file), so each file is only
added once.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/litegraphUtil.ts`:
- Line 47: The current destructuring const { graph, graph_mouse: [ posX, posY ]
} = canvas can throw if canvas.graph_mouse is undefined or too short; update the
logic around canvas and graph_mouse (where this destructuring occurs) to
defensively check canvas.graph_mouse and provide a fallback tuple (e.g., [0,0])
before assigning posX/posY, or use a safe default when graph_mouse.length < 2,
while preserving extraction of graph; ensure you reference the same symbols
(canvas, graph_mouse, posX, posY, graph) so callers of the surrounding function
keep expected behavior.
♻️ Duplicate comments (1)
src/utils/litegraphUtil.ts (1)
53-57: Remove unnecessary non-null assertions.After
if (newNode), TypeScript narrowsnewNodetoLGraphNode, making the!assertions redundant. This was partially addressed in a prior review round, but the assertions remain.♻️ Proposed fix
if (newNode) { - newNode!.pos = [ posX, posY ] - const addedNode = graph?.add(newNode!) ?? null + newNode.pos = [ posX, posY ] + const addedNode = graph?.add(newNode) ?? null graph?.change() return addedNode } else {
| return null | ||
| } | ||
|
|
||
| const { graph, graph_mouse: [ posX, posY ] } = canvas |
Contributor
There was a problem hiding this comment.
Add defensive check for graph_mouse before destructuring.
If canvas.graph_mouse is undefined or has fewer than 2 elements, destructuring [ posX, posY ] will throw at runtime. Consider providing a fallback default position.
🐛 Proposed fix
- const { graph, graph_mouse: [ posX, posY ] } = canvas
+ const { graph, graph_mouse } = canvas
+ const [ posX, posY ] = graph_mouse ?? [ 0, 0 ]🤖 Prompt for AI Agents
In `@src/utils/litegraphUtil.ts` at line 47, The current destructuring const {
graph, graph_mouse: [ posX, posY ] } = canvas can throw if canvas.graph_mouse is
undefined or too short; update the logic around canvas and graph_mouse (where
this destructuring occurs) to defensively check canvas.graph_mouse and provide a
fallback tuple (e.g., [0,0]) before assigning posX/posY, or use a safe default
when graph_mouse.length < 2, while preserving extraction of graph; ensure you
reference the same symbols (canvas, graph_mouse, posX, posY, graph) so callers
of the surrounding function keep expected behavior.
Contributor
Author
|
did some talking with austin about litegraph, going to close and reopen the PR so we don't have a flood of commits. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added feature to drag and drop multiple images into the UI and connect them with a Batch Images node with tests to add convenience for users. Only works with a group of images, mixing files not supported.
Review Focus
I've updated our usage of Litegraph.createNode, honestly, that method is pretty bad, onNodeCreated option method doesn't even return the node created. I think I will probably go check out their repo to do a PR over there. Anyways, I made a createNode method to avoid race conditions when creating nodes for the paste actions. Will allow us to better programmatically create nodes that do not have workflows that also need to be connected to other nodes.
https://www.notion.so/comfy-org/Implement-Multi-image-drag-and-drop-to-canvas-2eb6d73d36508195ad8addfc4367db10
Screenshots (if applicable)
2026-01-22.04-04-39.mp4
┆Issue is synchronized with this Notion page by Unito