Open
Conversation
dosubot
bot
added
the
size:XL
Contributor
📝 WalkthroughWalkthroughAdds async node creation ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EventUtils as EventUtils\n(extractFileFromDragEvent)
participant ComfyApp as ComfyApp
participant PasteUtils as usePaste\n(cloneDataTransfer, pasteImageNodes, pasteImageNode)
participant CreateNode as litegraphUtil\n(createNode)
participant LGraph as LGraph
participant UI as UI
User->>EventUtils: Drop images
EventUtils-->>ComfyApp: FileList
ComfyApp->>PasteUtils: pasteImageNodes(canvas, FileList)
loop for each file
PasteUtils->>CreateNode: createNode(canvas, "LoadImage")
CreateNode->>LGraph: LiteGraph.createNode(...)
CreateNode-->>PasteUtils: positioned LGraphNode
PasteUtils->>PasteUtils: await paste image into node
PasteUtils-->>ComfyApp: image LGraphNode
end
ComfyApp->>CreateNode: createNode(canvas, "BatchImages")
CreateNode-->>ComfyApp: batch LGraphNode
ComfyApp->>ComfyApp: positionBatchNodes(imageNodes, batchNode)
ComfyApp->>LGraph: connect image nodes to batch slots
ComfyApp->>UI: select nodes / trigger graph.changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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/28/2026, 11:24:15 PM UTC 🚀 Building Storybook
⏱️ Please wait while the Storybook build is in progress... |
🎭 Playwright Tests: ⏳ Running...Tests started at 01/28/2026, 11:24:13 PM UTC 📊 Browser Tests
|
coderabbitai
bot
requested review from
DrJKL,
Myestery,
benceruleanlu and
jtydhr88
![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: 12
🤖 Fix all issues with AI agents
In `@src/composables/usePaste.test.ts`:
- Around line 112-115: Replace the opaque "as unknown as LGraphNode" casts in
tests by returning a typed partial mock and casting that to LGraphNode; update
the helper createMockNode to build and return Partial<LGraphNode> and then use
vi.mocked(createNode).mockResolvedValue(createMockNode() as Partial<LGraphNode>
as LGraphNode) (or simply cast the helper's Partial to LGraphNode where used).
Locate uses around createMockNode and createNode in the test (including the
other occurrences at lines ~195-201) and switch them to the Partial<LGraphNode>
pattern so mocks are explicit and centralized.
- Around line 372-383: The cloneDataTransfer helper is adding files from both
.files and .items causing duplicate File entries; change cloneDataTransfer to
deduplicate by identity (e.g., track added files in a Set using a stable key
like file.name+file.size+file.type or track File objects with a WeakSet) before
appending to the cloned DataTransfer, and update the test in usePaste.test.ts to
assert exact counts (expect(cloned.files.length).toBe(2) and still assert the
two specific files are present).
In `@src/composables/usePaste.ts`:
- Around line 12-43: cloneDataTransfer currently adds files from both
original.files and original.items which can produce duplicates; deduplicate
file-kind entries before calling persistent.items.add. In cloneDataTransfer
collect files from original.files and those from original.items (via
item.getAsFile()), use a Set key (e.g., name + size + lastModified) or compare
File references to filter out duplicates, then add only unique files via
persistent.items.add; leave string data and properties
(dropEffect/effectAllowed) as-is.
In `@src/scripts/app.test.ts`:
- Around line 34-44: The helper createMockNode currently uses `any` and an
`unknown` cast which hides missing fields; change the function signature to
accept options: Partial<LGraphNode> (not any), merge defaults with the provided
options, and do a single explicit cast to LGraphNode on the returned object
(remove the intermediate unknown cast) so tests remain fully typed and adhere to
the repo's typing rules.
- Around line 46-52: The mock uses an unsafe "as any" for the graph; change
createMockCanvas to type the graph explicitly (e.g., use graph: Partial<LGraph>
or graph: Partial<NonNullable<LGraphCanvas['graph']>> ) so the mock implements
the expected shape rather than using any, and type graph.change as a vi.fn()
with the correct signature; update the return signature of createMockCanvas
accordingly so TypeScript infers the mock types for LGraphCanvas and its graph
instead of relying on any.
- Around line 119-124: The test fails because ComfyApp.handleFileList
dereferences fileList[0] without guarding empty lists; modify
ComfyApp.handleFileList to immediately return (no-op) when the incoming fileList
is empty (e.g., check fileList.length === 0 or !fileList || fileList.length ===
0) before any dereference, and ensure any downstream logic handles the
early-return case. Then update the test (calling app.handleFileList) to assert
that the promise resolves (e.g., not rejected or returns undefined) instead of
expecting a throw so the test verifies the new safe no-op behavior.
In `@src/scripts/app.ts`:
- Around line 1552-1568: In positionBatchNodes, avoid calling
this.canvas.graph?.change() inside the nodes.forEach loop; instead, compute and
set all node.pos values (including batchNode.pos already set) within the loop
and then invoke this.canvas.graph?.change() a single time after the loop
completes to improve performance for large drops.
- Around line 1527-1545: handleFileList currently assumes fileList[0] exists and
that all files are images; update it to first validate the FileList is non-empty
and contains only image/* entries, and if not show a localized toast (add the
new i18n key in src/locales/en/main.json) and return early. After calling
pasteImageNodes(this.canvas, fileList) check that imageNodes is non-empty before
creating/connecting the BatchImagesNode (createNode(..., 'BatchImagesNode')) and
before calling positionBatchNodes(imageNodes, batchImagesNode); if imageNodes is
empty, bail without creating the batch node. Ensure you reference the existing
functions handleFileList, pasteImageNodes, positionBatchNodes and createNode
when making changes.
In `@src/utils/__tests__/eventUtils.test.ts`:
- Around line 36-55: The test repeats type assertions for the result of
extractFileFromDragEvent; to simplify, assign the awaited result to a typed
variable (e.g., const files = await extractFileFromDragEvent(event) as FileList)
and use files.length and files[0]/files[1] in the assertions; update references
in this spec to use the new variable and keep FakeDragEvent and
extractFileFromDragEvent as the source identifiers to locate the code to change.
In `@src/utils/eventUtils.ts`:
- Line 29: Replace the arrow constant using the boxed Boolean with a plain
function declaration that returns the primitive boolean; specifically change the
const hasImageType = ({ type }: File): Boolean => type.startsWith('image'); to a
function declaration like function hasImageType(file: File): boolean { return
file.type.startsWith('image'); } (use the File parameter name `file`, reference
the hasImageType function/method name, and return primitive `boolean`).
- Around line 1-12: The helper hasImageType is currently declared as an arrow
returning a Boolean object and should be a function declaration returning a
primitive boolean; replace the existing arrow-expression version with a function
declaration named hasImageType that takes a File ({ type }: File) and returns
type.startsWith('image') as a boolean, so callers (including
extractFileFromDragEvent) follow coding guidelines and get the correct primitive
return type.
In `@src/utils/litegraphUtil.test.ts`:
- Around line 31-88: Tests use unsafe any/opaque casts (mockGraph: any, null as
any, as unknown as LGraphNode); update them to use Partial types and proper
typings: declare mockGraph as Partial<LGraph> and mockNode as
Partial<LGraphNode> (typed as LGraphNode when needed), remove or adjust the
`null as any` test (either drop it since empty string covers falsy guard or
update createNode signature to accept string | null | undefined), and ensure
vi.mocked(LiteGraph.createNode) returns the Partial mock correctly; also type
mockCanvas as Partial<LGraphCanvas> to avoid any casts and keep
useToastStore/mockAddAlert typed consistently.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/composables/usePaste.ts (1)
167-177: Consider aligning audio node creation with the new async pattern.The audio node creation still uses
LiteGraph.createNode()directly, while image nodes now use the new asynccreateNode()utility. For consistency and to avoid race conditions mentioned in the PR objectives, consider updating this path in a follow-up.
🤖 Fix all issues with AI agents
In `@src/composables/usePaste.ts`:
- Around line 23-31: The comment above the loop incorrectly states items.add()
is idempotent; update it to accurately explain why duplicates are avoided: note
that we only iterate original.items (and only add file-kind entries) rather than
iterating both .files and .items, so duplicates are prevented by the iteration
strategy, not by items.add() itself; change the comment near the loop that
references items.add(), original.items, and persistent.items to reflect this
accurate behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/composables/usePaste.ts (1)
165-176: Inconsistent node creation pattern for audio nodes.Audio node creation still uses the synchronous
LiteGraph.createNodepattern, while image handling now uses the asynccreateNodeutility. Per PR objectives,createNodewas introduced to avoid race conditions withonNodeCreatedcallbacks. This inconsistency could lead to the same race conditions for audio paste operations.♻️ Suggested refactor
} else if (item.type.startsWith('audio/')) { if (!audioNode) { // No audio node selected: add a new one - const newNode = LiteGraph.createNode('LoadAudio') - if (newNode) { - newNode.pos = [canvas.graph_mouse[0], canvas.graph_mouse[1]] - audioNode = graph?.add(newNode) ?? null - } - graph?.change() + audioNode = await createNode(canvas as LGraphCanvas, 'LoadAudio') } pasteItemsOnNode(items, audioNode, 'audio') return }
🤖 Fix all issues with AI agents
In `@src/composables/usePaste.ts`:
- Around line 92-109: The pasteImageNodes function iterates all files but
doesn't validate they're images; update pasteImageNodes to skip non-image files
by checking each File's MIME (e.g., file.type.startsWith("image/")) before
creating a DataTransfer and calling pasteImageNode, and optionally fall back to
checking file.name extensions if file.type is empty; only call pasteImageNode
and push into nodes for files that pass this image check (reference
pasteImageNodes and pasteImageNode and the LoadImage node behavior).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/scripts/app.test.ts`:
- Around line 38-48: The createMockNode helper currently types its options
parameter as Record<string, unknown>; change its signature to use
Partial<LGraphNode> for better type safety (function createMockNode(options:
Partial<LGraphNode> = {})). Update any related casts if needed (the final as
unknown as LGraphNode can remain) and ensure the spread of ...options onto the
returned object still compiles against LGraphNode members; this makes TypeScript
validate that provided mock fields match LGraphNode properties.
- Around line 258-275: Replace the unsafe "as unknown as ReturnType<typeof
useToastStore>" cast with a Partial-based stub for the toast store: create a
const toastStub: Partial<ReturnType<typeof useToastStore>> = { addAlert:
mockAddAlert } and pass that to vi.mocked(useToastStore).mockReturnValue (or
cast only the Partial to the exact return type in a single safe cast if the mock
API requires it); update the test around useToastStore and mockAddAlert to use
toastStub instead of the as unknown as pattern so the test uses a typed Partial
of useToastStore rather than an unsafe double-cast.
♻️ Duplicate comments (2)
src/scripts/app.ts (1)
1527-1545: Guard empty/mixed FileList drops before batching.The existing concern from the previous review still applies: Line 1533 assumes
fileList[0]exists and will throw on empty drops. Additionally, the MIME type check should use'image/'(with trailing slash) for correctness, andimageNodesshould be validated as non-empty before proceeding.src/scripts/app.test.ts (1)
125-130: Test documents a bug rather than desired behavior.This test expects a throw for empty file lists, which confirms the implementation bug flagged in
handleFileList. Once the implementation is fixed to guard against empty lists, update this test to assert a successful no-op.♻️ Suggested update once implementation is fixed
it('should handle empty file list', async () => { const dataTransfer = new DataTransfer() - // The implementation doesn't check for empty list and will throw - await expect(app.handleFileList(dataTransfer.files)).rejects.toThrow() + // Empty file list should be a no-op + await expect(app.handleFileList(dataTransfer.files)).resolves.toBeUndefined() })
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.3 kB (baseline 22.3 kB) • 🟢 -34 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 949 kB (baseline 953 kB) • 🟢 -4.13 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 440 kB (baseline 440 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 23 added / 23 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.83 kB (baseline 2.83 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 7 added / 7 removed Data & Services — 3.17 MB (baseline 3.17 MB) • 🔴 +1.98 kBStores, services, APIs, and repositories
Status: 9 added / 9 removed Utilities & Hooks — 24 kB (baseline 24 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 6.42 MB (baseline 6.42 MB) • 🟢 -1.38 kBBundles that do not match a named category
Status: 79 added / 80 removed |
DrJKL
reviewed
Jan 24, 2026
| 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.
Nit: I know it's not introduced here, but can we clean up the type assertion?
Contributor
Author
There was a problem hiding this comment.
I was planning on cleaning up all the type assertion when I got to the audio/video story as it's the last drag and drop / copy & paste task. Unless there is some other type beyond that I should add to that task. I already have another task waiting with the text drag/drop/copy/paste
DrJKL
reviewed
Jan 24, 2026
DrJKL
reviewed
Jan 24, 2026
DrJKL
reviewed
Jan 24, 2026
AustinMroz
reviewed
Jan 24, 2026
src/scripts/app.ts
Outdated
| batchNode.pos = [ x + width + 100, y + 30 ] | ||
|
|
||
| // Retrieving Node Height is inconsistent | ||
| let height = nodeHeight; |
Collaborator
There was a problem hiding this comment.
I see where the difficulty comes from...
Node image previews is some super mucky code. useLitegraphService().updatePreview has an optional callback modifier that's triggered after the preview image finishes loading, but it's mostly intended for use inside subgraphs and wouldn't be reliable enough to use here.
For litegraph, image previews have a fixed height and you can calc the positions in advance
My personal recommendation would be to always use 344 for height and skip the measurement entirely. This would also allow you to set the node position prior to node creation.
Nodes 2.0 makes things a good bit more difficult.
Contributor
Author
There was a problem hiding this comment.
@AustinMroz Updated to use just the fixed height. Sorry about the delay. I saw an approval and assumed I was done.
DrJKL
previously approved these changes
Jan 24, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/litegraphUtil.ts`:
- Around line 51-55: The code calls graph.change() even when graph.add(newNode)
may return undefined; modify the logic in the block handling newNode to capture
the result of graph.add via the variable addedNode, check if addedNode is falsy,
and if so show an alert (e.g., alert("Failed to add node")) and return null
without calling graph.change(); only call graph.change() and return addedNode
when addedNode is truthy. Ensure you reference newNode, graph.add, addedNode and
graph.change in the updated flow so the add-failure path is handled cleanly.
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 1463-1464: createNode returns Promise<LGraphNode | null> but
imageNode is used unguarded; add a null-check after awaiting createNode in the
block that calls pasteImageNode to bail out or handle the error when imageNode
is null. Specifically, after const imageNode = await createNode(this.canvas,
'LoadImage') verify imageNode !== null (or throw/log and return) before calling
pasteImageNode(this.canvas, transfer.items, imageNode) so pasteImageNode never
receives a null node.
- Around line 1568-1576: positionBatchNodes currently assumes nodes is non-empty
and uses a hardcoded height for LoadImage; update it to first guard against an
empty nodes array (return early or no-op if nodes.length === 0) and then compute
node height dynamically instead of the magic 344: for example read
nodes[0].size[1] or call nodes[0].computeSize()[1] (fallback to a sensible
default if those are undefined) and use that value when positioning batchNode;
references: positionBatchNodes, nodes, batchNode, and the 'LoadImage' node type.
AustinMroz
approved these changes
Feb 6, 2026
Collaborator
There was a problem hiding this comment.
I must thoroughly apologize. I want to give feedback that is both directed and helpful. That I struggle with doing so has placed unfair burden on you.
The quality of the code looks sufficient to me, and I will give approval as such.
However there's a lot litegraph specific quirks at play here that I could not expect others to either be aware of or to work on fixes for.
- Positioning of nodes in vue mode after initialization is not currently functional. #7591 was recently reverted in #8619
- This means that the current arrangement process causes nodes to stack up on the origin point.
- I've been working on a way to clean this up for a while now, but in the meantime, the safe way to position a newly created node is to pass the desired position for the node as an option for node creation
- The newly introduced
createNodedoes not include a way to pass such additional options.
- The newly introduced
- Vue nodes currently scale height proportionally to the aspect ratio of the contained image preview. This makes programatically determining the height prior to adding nodes to the graph impossible.
- This has caused frustration in other places as well. I've opened a separate PR to fix this, so no action is required here: #8702
- I am conflicted about the addition of a new node creation function. I should have directed you towards
litegraphService:addNodeOnGraph, but became a little too caught up in the addressing the pertinent technical complications. app.tsis an old part of the code base. While the code changes to it are fine, introducing anapp.test.tsfeels a little misleading when its only being used to test drag and drop functionality. Perhaps a more targetedapp.dropHandler.test.tswould be more appropriate.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Summary by CodeRabbit
New Features
Bug Fixes
Tests