Copy of Batch Drag & Drop Images #8812
Closed
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
# Conflicts: # src/lib/litegraph/src/LGraph.ts
|
Playwright: ✅ 523 passed, 0 failed · 2 flaky 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/11/2026, 11:45:52 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Contributor
📝 WalkthroughWalkthroughThis PR introduces batch image file handling with async node creation, enabling drag-and-drop of multiple images to create connected LoadImage nodes and a BatchImagesNode. It exports clipboard cloning utilities, makes image pasting asynchronous, and updates graph null-handling to support the new workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas
participant EventUtils
participant App
participant NodeCreation
participant Graph
User->>Canvas: Drop multiple image files
Canvas->>EventUtils: extractFileFromDragEvent
EventUtils-->>Canvas: FileList (multiple images)
Canvas->>App: handleFileList(fileList)
App->>NodeCreation: createNode for each file
NodeCreation->>Graph: add LoadImage node
Graph-->>NodeCreation: LGraphNode
NodeCreation-->>App: LGraphNode[]
App->>NodeCreation: pasteImageNodes(fileList)
NodeCreation-->>App: Promise<LGraphNode[]>
App->>NodeCreation: createNode(BatchImagesNode)
NodeCreation->>Graph: add BatchImagesNode
Graph-->>NodeCreation: BatchNode
NodeCreation-->>App: BatchNode
App->>App: positionBatchNodes(imageNodes, batchNode)
App->>Graph: Connect image outputs to batch
Graph-->>App: Connections established
App-->>User: Batch + image nodes created and positioned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22 kB (baseline 22 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 872 kB (baseline 872 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 454 kB (baseline 454 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 751 B (baseline 751 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 36.5 kB (baseline 36.5 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.11 MB (baseline 2.1 MB) • 🔴 +2.29 kBStores, services, APIs, and repositories
Status: 12 added / 12 removed Utilities & Hooks — 237 kB (baseline 237 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 8.67 MB (baseline 8.67 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.29 MB (baseline 7.29 MB) • ⚪ 0 BBundles that do not match a named category
Status: 54 added / 54 removed |
![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: 8
🤖 Fix all issues with AI agents
In `@src/scripts/app.test.ts`:
- Around line 39-49: The mapped type on createMockNode uses `any`, which
violates the no-explicit-any rule; change the signature to use `unknown` instead
(e.g., options: { [K in keyof LGraphNode]?: unknown } = {}) so the helper
remains type-safe while still allowing arbitrary overrides for the created
LGraphNode; update the createMockNode function declaration accordingly
(references: createMockNode, LGraphNode).
In `@src/scripts/app.ts`:
- Around line 1583-1596: The handleFileList function currently assumes
fileList[0] exists and is an image; add a guard at the top of handleFileList to
return early if fileList is empty, then build an array of image files by
filtering FileList items where file.type.startsWith('image'); if that filtered
array is empty return early; call pasteImageNodes(this.canvas, imageFiles)
(instead of passing the original FileList), only create the BatchImagesNode and
call positionBatchNodes/connect on the resulting imageNodes when imageNodes
length > 0 to avoid creating an empty batch node.
- Around line 1603-1620: positionBatchNodes currently assumes nodes is non-empty
and uses a hardcoded LoadImage height; first add an early guard in
positionBatchNodes to return immediately if nodes.length === 0 (so
nodes[0].getBounding() is never called), and ensure callers like
handleFileList/pasteImageNodes handle and propagate empty arrays from
createNode. Replace the hardcoded height logic by computing each node's height
from its bounding box (use node.getBounding() or node.size[1] as a fallback) and
use that per-node height when setting node.pos; keep setting batchNode.pos from
the first node's bounding result and call this.canvas.graph?.change() after
positioning.
- Around line 1592-1594: The loop attempts to connect multiple imageNodes to
batchImagesNode using increasing target indices, but BatchImagesNode only
declares a single input slot named "images", so connect(...) silently fails for
indices >0; update the connection logic to either add input slots dynamically on
the BatchImagesNode (use batchImagesNode.onBeforeConnectInput to append a new
input slot before each connect call so the target index exists) or change the
wiring to connect all imageNodes into the single "images" input (e.g., combine
into one array/source if the backend supports it); reference imageNodes,
batchImagesNode, connect(), and onBeforeConnectInput when making the change.
In `@src/utils/eventUtils.ts`:
- Around line 6-12: The single-file path rejects image/bmp but the multi-file
path uses hasImageType which allows bmp, causing inconsistent behavior; modify
the multi-file branch that uses Array.from(files).every(hasImageType) to also
exclude files with type === 'image/bmp' (e.g., change the predicate to check
hasImageType(file) && file.type !== 'image/bmp' or pre-filter the FileList), so
both the single-file return (files[0]) and the multi-file return (files)
consistently omit BMPs when inspecting event.dataTransfer.files.
In `@src/utils/litegraphUtil.test.ts`:
- Around line 31-89: Tests currently use explicit any for mockCanvas, mockGraph
and mock nodes which triggers typescript-eslint/no-explicit-any; replace those
anys with concrete mock types or unknown-to-specific casts. Define mock types
(e.g., type MockGraph = { add: (n: LGraphNode) => LGraphNode; change: () => void
} and type MockCanvas = { graph: MockGraph | null; graph_mouse: [number, number]
}) and type the mockNode as Partial<LGraphNode> or cast via unknown to
LGraphNode (e.g., mockNode as unknown as LGraphNode) when calling
LiteGraph.createNode; update references to mockCanvas, mockGraph, mockNode,
LiteGraph.createNode and createNode in the tests accordingly so no explicit any
or as any remains.
In `@src/utils/litegraphUtil.ts`:
- Around line 40-62: The createNode function issues two problems: remove the
redundant graph.change() call after graph.add(newNode) since graph.add already
invokes this.change() (refer to createNode, graph.add and graph.change), or if a
second notification is truly required document the reason above the call; also
remove the unnecessary await new Promise(r => setTimeout(r, 0)) between
LiteGraph.createNode and graph.add unless there's a documented asynchronous
side-effect—if an async yield is required, replace it with a clear comment
explaining why (e.g., waiting for node registration) so the intent is explicit.
- Around line 3-8: There are duplicate imports from the same module: the
type-only import of ColorOption, LGraph, LGraphCanvas and the value import of
LiteGraph; remove the duplicate by merging them into a single import statement
from '@/lib/litegraph/src/litegraph' that includes both the value symbol
LiteGraph and the type symbols ColorOption, LGraph, LGraphCanvas (e.g., a single
import that lists LiteGraph and the types), so only one import from that module
remains.
🧹 Nitpick comments (5)
src/lib/litegraph/src/LGraph.ts (1)
888-906: Overload asymmetry: only the deprecated overload acceptsnull.The primary overload (line 889) still requires a non-null node, while only the deprecated overload (line 899) now accepts
null. Callers using the modernoptionsoverload cannot pass a nullable node. If the intent is to allownullbroadly (e.g., fromcreateNodewhich may returnnull), consider also updating the primary overload, or at minimum the implementation signature on line 903, so the contract is consistent.That said, the runtime guard on line 906 (
if (!node) return) already handles falsy values safely, so this is purely a type-level concern.src/utils/eventUtils.ts (1)
29-31: Prefer'image/'prefix for stricter MIME type matching.
type.startsWith('image')works in practice since all image MIME types useimage/, but using'image/'would be more precise and guard against hypothetical non-standard types.Suggested fix
function hasImageType({ type }: File): boolean { - return type.startsWith('image') + return type.startsWith('image/') }src/scripts/app.test.ts (1)
139-179: Position tests rely on exact pixel values derived from mock bounding boxes.The assertions on lines 149, 165–166 use specific pixel values (500, 230, 594, 963) computed from the mock bounding data. This is fine for pinning behavior, but if the positioning algorithm changes (padding, margins), all these values need updating simultaneously. Consider adding a brief comment in the test explaining the calculation (e.g.,
// x: 100 + 300 (width) + 100 (gap) = 500).src/composables/usePaste.test.ts (1)
259-276: Audio paste path still usesLiteGraph.createNodedirectly.The image paste tests now mock
createNodefrom litegraphUtil (line 254), but the audio paste test (line 261) still mocksLiteGraph.createNode. This is consistent with the source code where only the image path was migrated, but worth noting as a follow-up to migrate the audio path similarly for consistency.src/scripts/app.ts (1)
557-563: Useelse ifinstead of two separateifblocks.
FileandFileListare mutually exclusive results fromextractFileFromDragEvent. Usingelse ifmakes the exclusivity explicit and avoids evaluating the second condition unnecessarily.Proposed fix
- if (fileMaybe instanceof File) { - await this.handleFile(fileMaybe, 'file_drop') - } - - if (fileMaybe instanceof FileList) { + if (fileMaybe instanceof File) { + await this.handleFile(fileMaybe, 'file_drop') + } else if (fileMaybe instanceof FileList) { await this.handleFileList(fileMaybe) }
Comment on lines
+39
to
+49
| function createMockNode(options: { [K in keyof LGraphNode]?: any } = {}) { | ||
| return { | ||
| id: 1, | ||
| pos: [0, 0], | ||
| size: [200, 100], | ||
| type: 'LoadImage', | ||
| connect: vi.fn(), | ||
| getBounding: vi.fn(() => new Float64Array([0, 0, 200, 100])), | ||
| ...options | ||
| } as LGraphNode | ||
| } |
Contributor
There was a problem hiding this comment.
Replace any with unknown in mock helper type.
Line 39 uses any in the mapped type, which will fail the no-explicit-any lint rule:
Suggested fix
-function createMockNode(options: { [K in keyof LGraphNode]?: any } = {}) {
+function createMockNode(options: Partial<Record<keyof LGraphNode, unknown>> = {}) {
return {
id: 1,
pos: [0, 0],
size: [200, 100],
type: 'LoadImage',
connect: vi.fn(),
getBounding: vi.fn(() => new Float64Array([0, 0, 200, 100])),
...options
} as LGraphNode
}As per coding guidelines: "Never use any type; use proper TypeScript types."
🤖 Prompt for AI Agents
In `@src/scripts/app.test.ts` around lines 39 - 49, The mapped type on
createMockNode uses `any`, which violates the no-explicit-any rule; change the
signature to use `unknown` instead (e.g., options: { [K in keyof LGraphNode]?:
unknown } = {}) so the helper remains type-safe while still allowing arbitrary
overrides for the created LGraphNode; update the createMockNode function
declaration accordingly (references: createMockNode, LGraphNode).
Comment on lines
+1583
to
+1596
| 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.positionBatchNodes(imageNodes, batchImagesNode) | ||
| this.canvas.selectItems([...imageNodes, batchImagesNode]) | ||
|
|
||
| Array.from(imageNodes).forEach((imageNode, index) => { | ||
| imageNode.connect(0, batchImagesNode, index) | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing guard for empty fileList and non-image file types.
fileList[0].type will throw if fileList is empty. Additionally, only the first file's MIME type is checked—if a user drops a mix of images and non-images, all files are treated as images.
Proposed fix
async handleFileList(fileList: FileList) {
+ if (fileList.length === 0) return
+
if (fileList[0].type.startsWith('image')) {🤖 Prompt for AI Agents
In `@src/scripts/app.ts` around lines 1583 - 1596, The handleFileList function
currently assumes fileList[0] exists and is an image; add a guard at the top of
handleFileList to return early if fileList is empty, then build an array of
image files by filtering FileList items where file.type.startsWith('image'); if
that filtered array is empty return early; call pasteImageNodes(this.canvas,
imageFiles) (instead of passing the original FileList), only create the
BatchImagesNode and call positionBatchNodes/connect on the resulting imageNodes
when imageNodes length > 0 to avoid creating an empty batch node.
Comment on lines
+1592
to
+1594
| Array.from(imageNodes).forEach((imageNode, index) => { | ||
| imageNode.connect(0, batchImagesNode, index) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg "BatchImagesNode" --type=ts -C 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3417
🏁 Script executed:
rg "class.*BatchImagesNode|BatchImagesNode.*=" --type=ts -B 3 -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "BatchImagesNode" --type=json -C 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 6179
🏁 Script executed:
fd -t f -e ts -e js | xargs rg "inputs|slots" | grep -i batch || echo "No batch-related slot definitions found"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 109
🏁 Script executed:
cat -n src/locales/en/nodeDefs.json | grep -A 30 '"BatchImagesNode"'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 816
🏁 Script executed:
rg "inputs.*images" src/locales/en/nodeDefs.json -A 20 | grep -A 20 "BatchImagesNode"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "\.connect\(" src/scripts/app.ts -B 2 -A 2 | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 327
🏁 Script executed:
rg "connect.*function|function.*connect" --type=ts | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 916
🏁 Script executed:
rg "class.*Node|LNode" --type=ts -A 15 | grep -A 15 "connect" | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1052
🏁 Script executed:
rg "class LGraphNode" --type=ts -A 30 | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4113
🏁 Script executed:
rg "connect\s*\(" --type=ts -B 3 -A 8 | grep -A 8 "connect\s*\(.*?" | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 93
🏁 Script executed:
fd -type f -name "*.ts" -path "*/lib/*" -o -name "litegraph*" 2>/dev/null | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "LGraphNode|litegraph" --type=ts | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2218
🏁 Script executed:
rg "connect\s*\(\s*[^)]*\)\s*\{" src/lib/litegraph/src/LGraphNode.ts -A 20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
grep -n "connect(" src/lib/litegraph/src/LGraphNode.ts | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 477
🏁 Script executed:
sed -n '2726,2800p' src/lib/litegraph/src/LGraphNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2488
🏁 Script executed:
rg "onBeforeConnectInput|addInput|dynamic.*slot" src/ --type=ts | grep -i batchRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "inputs\s*\[\s*\]|addInput|findInputSlot" src/ --type=ts | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3078
🏁 Script executed:
rg "BatchImagesNode" src/ --type=ts -B 5 -A 15 | grep -v "test\|locale\|mock"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1137
🏁 Script executed:
rg "node\s*\.\s*addInput|dynamicly.*add|createInputSlot" src/ --type=ts | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2094
🏁 Script executed:
rg "onBeforeConnectInput" src/ --type=ts -B 3 -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2202
🏁 Script executed:
rg "BatchImagesNode.*onBeforeConnectInput|class.*BatchImages" src/ --type=ts -B 5 -A 30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "onBeforeConnectInput" src/ --type=ts --type=js | grep -v test | grep -v "\.ts-"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 309
🏁 Script executed:
rg "inputs\s*=" src/ --type=ts | grep -i batchRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "\.inputs\s*\[" src/ --type=ts | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1906
🏁 Script executed:
python3 << 'EOF'
# Check if there's any code that validates or logs connection failures
import subprocess
result = subprocess.run(['rg', 'inputs.*length|targetIndex.*>=', 'src/', '--type=ts', '-A', '3'],
capture_output=True, text=True)
print(result.stdout[:2000])
EOFRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2070
🏁 Script executed:
sed -n '2770,2810p' src/lib/litegraph/src/LGraphNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1364
🏁 Script executed:
rg "targetIndex >= target_node.inputs" src/lib/litegraph/src/LGraphNode.ts -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 238
Fix silent connection failures when connecting multiple image nodes to BatchImagesNode.
The code assumes BatchImagesNode has sequential input slots (0, 1, 2...) indexed by image count, but the node definition declares only a single input slot named "images". The connect() call will silently fail (returning null without error unless debug mode is enabled) for all connections beyond the first, as the target slot indices exceed target_node.inputs.length.
Consider using onBeforeConnectInput to dynamically add input slots for each connection, or restructure to connect multiple images to a single slot if the backend supports it.
🤖 Prompt for AI Agents
In `@src/scripts/app.ts` around lines 1592 - 1594, The loop attempts to connect
multiple imageNodes to batchImagesNode using increasing target indices, but
BatchImagesNode only declares a single input slot named "images", so
connect(...) silently fails for indices >0; update the connection logic to
either add input slots dynamically on the BatchImagesNode (use
batchImagesNode.onBeforeConnectInput to append a new input slot before each
connect call so the target index exists) or change the wiring to connect all
imageNodes into the single "images" input (e.g., combine into one array/source
if the backend supports it); reference imageNodes, batchImagesNode, connect(),
and onBeforeConnectInput when making the change.
Comment on lines
+1603
to
+1620
| positionBatchNodes(nodes: LGraphNode[], batchNode: LGraphNode): void { | ||
| const [x, y, width] = nodes[0].getBounding() | ||
| batchNode.pos = [ x + width + 100, y + 30 ] | ||
|
|
||
| // Retrieving Node Height is inconsistent | ||
| let height = 0; | ||
| if (nodes[0].type === 'LoadImage') { | ||
| height = 344 | ||
| } | ||
|
|
||
| nodes.forEach((node, index) => { | ||
| if (index > 0) { | ||
| node.pos = [ x, y + (height * index) + (25 * (index + 1)) ] | ||
| } | ||
| }); | ||
|
|
||
| this.canvas.graph?.change() | ||
| } |
Contributor
There was a problem hiding this comment.
positionBatchNodes will crash on an empty nodes array and uses a fragile hardcoded height.
nodes[0].getBounding()throws ifnodesis empty.handleFileListdoesn't guard againstpasteImageNodesreturning[](e.g., if allcreateNodecalls fail).- Hardcoding
height = 344forLoadImageis brittle—it will silently break if the node's rendered size changes. Consider computing the actual height from the node's bounding box ornode.size[1].
Proposed fix
positionBatchNodes(nodes: LGraphNode[], batchNode: LGraphNode): void {
+ if (nodes.length === 0) return
+
const [x, y, width] = nodes[0].getBounding()
batchNode.pos = [ x + width + 100, y + 30 ]
- // Retrieving Node Height is inconsistent
- let height = 0;
- if (nodes[0].type === 'LoadImage') {
- height = 344
- }
+ const height = nodes[0].size[1]
nodes.forEach((node, index) => {
if (index > 0) {
node.pos = [ x, y + (height * index) + (25 * (index + 1)) ]
}
});
this.canvas.graph?.change()
}🤖 Prompt for AI Agents
In `@src/scripts/app.ts` around lines 1603 - 1620, positionBatchNodes currently
assumes nodes is non-empty and uses a hardcoded LoadImage height; first add an
early guard in positionBatchNodes to return immediately if nodes.length === 0
(so nodes[0].getBounding() is never called), and ensure callers like
handleFileList/pasteImageNodes handle and propagate empty arrays from
createNode. Replace the hardcoded height logic by computing each node's height
from its bounding box (use node.getBounding() or node.size[1] as a fallback) and
use that per-node height when setting node.pos; keep setting batchNode.pos from
the first node's bounding result and call this.canvas.graph?.change() after
positioning.
Comment on lines
+6
to
12
| 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.
BMP handling is inconsistent between single-file and multi-file paths.
A single image/bmp file is explicitly rejected (line 8), but when multiple files are dragged, hasImageType (line 10) accepts image/bmp because it only checks type.startsWith('image'). This means dragging one bmp from Chrome→Firefox is filtered, but dragging two bmps passes through.
If the bmp exclusion is intentional for cross-browser drag artifacts, consider filtering it in both paths:
Suggested fix
- if (files.length === 1 && files[0].type !== 'image/bmp') {
- return files[0]
- } else if (files.length > 1 && Array.from(files).every(hasImageType)) {
+ const imageFiles = Array.from(files).filter(
+ (f) => hasImageType(f) && f.type !== 'image/bmp'
+ )
+ if (imageFiles.length === 1) {
+ return imageFiles[0]
+ } else if (imageFiles.length > 1) {
return files
}🤖 Prompt for AI Agents
In `@src/utils/eventUtils.ts` around lines 6 - 12, The single-file path rejects
image/bmp but the multi-file path uses hasImageType which allows bmp, causing
inconsistent behavior; modify the multi-file branch that uses
Array.from(files).every(hasImageType) to also exclude files with type ===
'image/bmp' (e.g., change the predicate to check hasImageType(file) && file.type
!== 'image/bmp' or pre-filter the FileList), so both the single-file return
(files[0]) and the multi-file return (files) consistently omit BMPs when
inspecting event.dataTransfer.files.
Comment on lines
+31
to
+89
| describe('createNode', () => { | ||
| let mockCanvas: any | ||
| let mockGraph: any | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
|
|
||
| mockGraph = { | ||
| add: vi.fn((node) => node), | ||
| change: vi.fn() | ||
| } | ||
|
|
||
| mockCanvas = { | ||
| graph: mockGraph, | ||
| graph_mouse: [100, 200] | ||
| } | ||
| }) | ||
|
|
||
| it('should create a node successfully', async () => { | ||
| const mockNode = { pos: [0, 0] } | ||
| vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode as any) | ||
|
|
||
| const result = await createNode(mockCanvas, 'LoadImage') | ||
|
|
||
| expect(LiteGraph.createNode).toHaveBeenCalledWith('LoadImage') | ||
| expect(mockNode.pos).toEqual([100, 200]) | ||
| expect(mockGraph.add).toHaveBeenCalledWith(mockNode) | ||
| expect(mockGraph.change).toHaveBeenCalled() | ||
| expect(result).toBe(mockNode) | ||
| }) | ||
|
|
||
| it('should return null when name is empty', async () => { | ||
| const result = await createNode(mockCanvas, '') | ||
|
|
||
| expect(LiteGraph.createNode).not.toHaveBeenCalled() | ||
| expect(result).toBeNull() | ||
| }) | ||
|
|
||
| it('should handle graph being null', async () => { | ||
| const mockNode = { pos: [0, 0] } | ||
| mockCanvas.graph = null | ||
| vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode as any) | ||
|
|
||
| const result = await createNode(mockCanvas, 'LoadImage') | ||
|
|
||
| expect(mockNode.pos).toEqual([0, 0]) | ||
| expect(result).toBeNull() | ||
| }) | ||
|
|
||
| it('should set position based on canvas graph_mouse', async () => { | ||
| mockCanvas.graph_mouse = [250, 350] | ||
| const mockNode = { pos: [0, 0] } | ||
| vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode as any) | ||
|
|
||
| await createNode(mockCanvas, 'LoadAudio') | ||
|
|
||
| expect(mockNode.pos).toEqual([250, 350]) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Fix any type usage — CI lint failures on lines 32, 33, 51, 72, 83.
The pipeline reports typescript-eslint/no-explicit-any errors. Replace any with proper types or unknown:
Suggested approach
describe('createNode', () => {
- let mockCanvas: any
- let mockGraph: any
+ let mockCanvas: { graph: unknown; graph_mouse: number[] }
+ let mockGraph: { add: ReturnType<typeof vi.fn>; change: ReturnType<typeof vi.fn> }
beforeEach(() => {
vi.clearAllMocks()
// ...
})
it('should create a node successfully', async () => {
const mockNode = { pos: [0, 0] }
- vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode as any)
+ vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode as unknown as LGraphNode)
// ...
})Apply similar as unknown as LGraphNode casts (or narrower mock types) for lines 72 and 83.
As per coding guidelines: "Never use any type; use proper TypeScript types" and "Never use as any type assertions; fix the underlying type issue instead."
🧰 Tools
🪛 GitHub Actions: CI: Lint Format
[error] 32-32: typescript-eslint/no-explicit-any: Unexpected any. Specify a different type. Use unknown instead, this will force you to explicitly, and safely, assert the type is correct.
[error] 33-33: typescript-eslint/no-explicit-any: Unexpected any. Specify a different type. Use unknown instead, this will force you to explicitly, and safely, assert the type is correct.
[error] 51-51: typescript-eslint/no-explicit-any: Unexpected any. Specify a different type. Use unknown instead, this will force you to explicitly, and safely, assert the type is correct.
[error] 72-72: typescript-eslint/no-explicit-any: Unexpected any. Specify a different type. Use unknown instead, this will force you to explicitly, and safely, assert the type is correct.
[error] 83-83: typescript-eslint/no-explicit-any: Unexpected any. Specify a different type. Use unknown instead, this will force you to explicitly, and safely, assert the type is correct.
🤖 Prompt for AI Agents
In `@src/utils/litegraphUtil.test.ts` around lines 31 - 89, Tests currently use
explicit any for mockCanvas, mockGraph and mock nodes which triggers
typescript-eslint/no-explicit-any; replace those anys with concrete mock types
or unknown-to-specific casts. Define mock types (e.g., type MockGraph = { add:
(n: LGraphNode) => LGraphNode; change: () => void } and type MockCanvas = {
graph: MockGraph | null; graph_mouse: [number, number] }) and type the mockNode
as Partial<LGraphNode> or cast via unknown to LGraphNode (e.g., mockNode as
unknown as LGraphNode) when calling LiteGraph.createNode; update references to
mockCanvas, mockGraph, mockNode, LiteGraph.createNode and createNode in the
tests accordingly so no explicit any or as any remains.
Comment on lines
+3
to
+8
| import type { | ||
| ColorOption, | ||
| LGraph, | ||
| LGraphCanvas, | ||
| } from '@/lib/litegraph/src/litegraph' | ||
| import { LiteGraph } from '@/lib/litegraph/src/litegraph' |
Contributor
There was a problem hiding this comment.
Fix duplicate import — CI lint failure.
The pipeline reports eslint-plugin-import(no-duplicates) on line 8. Lines 3–7 and line 8 both import from @/lib/litegraph/src/litegraph. Merge the value import into the existing value-import block on lines 9–14.
Suggested fix
import type {
ColorOption,
LGraph,
LGraphCanvas,
} from '@/lib/litegraph/src/litegraph'
-import { LiteGraph } from '@/lib/litegraph/src/litegraph'
import {
LGraphGroup,
LGraphNode,
+ LiteGraph,
Reroute,
isColorable
} from '@/lib/litegraph/src/litegraph'🧰 Tools
🪛 GitHub Actions: CI: Lint Format
[error] 8-8: eslint-plugin-import(no-duplicates): Modules should not be imported multiple times in the same file. Merge these imports into a single import statement.
🤖 Prompt for AI Agents
In `@src/utils/litegraphUtil.ts` around lines 3 - 8, There are duplicate imports
from the same module: the type-only import of ColorOption, LGraph, LGraphCanvas
and the value import of LiteGraph; remove the duplicate by merging them into a
single import statement from '@/lib/litegraph/src/litegraph' that includes both
the value symbol LiteGraph and the type symbols ColorOption, LGraph,
LGraphCanvas (e.g., a single import that lists LiteGraph and the types), so only
one import from that module remains.
Comment on lines
+40
to
+62
| export async function createNode( | ||
| canvas: LGraphCanvas, | ||
| name: string | ||
| ): Promise<LGraphNode | null> { | ||
| if (!name) { | ||
| return null | ||
| } | ||
|
|
||
| const { graph, graph_mouse: [ posX, posY ] } = canvas | ||
| const newNode = LiteGraph.createNode(name) | ||
| await new Promise(r => setTimeout(r, 0)) | ||
|
|
||
| if (newNode && graph) { | ||
| newNode.pos = [ posX, posY ] | ||
| const addedNode = graph.add(newNode) ?? null | ||
|
|
||
| if (addedNode) graph.change() | ||
| return addedNode | ||
| } else { | ||
| useToastStore().addAlert(t('assetBrowser.failedToCreateNode')) | ||
| return null | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Double graph.change() call and unexplained setTimeout(0) yield.
Two concerns with createNode:
-
Double
graph.change():graph.add(newNode)(LGraph.ts line 989) already callsthis.change()internally. Line 56 callsgraph.change()again, resulting in two change notifications per node creation. Remove the redundant call or document why a second notification is needed. -
await new Promise(r => setTimeout(r, 0))(line 50): This yields to the event loop betweenLiteGraph.createNodeandgraph.add. The intent isn't documented. If it's needed for async node registration side effects, add a comment explaining why. Otherwise, this adds unnecessary latency to every node creation.
Suggested fix (remove redundant change call)
if (newNode && graph) {
newNode.pos = [ posX, posY ]
const addedNode = graph.add(newNode) ?? null
-
- if (addedNode) graph.change()
return addedNode
} else {📝 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
| export async function createNode( | |
| canvas: LGraphCanvas, | |
| name: string | |
| ): Promise<LGraphNode | null> { | |
| if (!name) { | |
| return null | |
| } | |
| const { graph, graph_mouse: [ posX, posY ] } = canvas | |
| const newNode = LiteGraph.createNode(name) | |
| await new Promise(r => setTimeout(r, 0)) | |
| if (newNode && graph) { | |
| newNode.pos = [ posX, posY ] | |
| const addedNode = graph.add(newNode) ?? null | |
| if (addedNode) graph.change() | |
| return addedNode | |
| } else { | |
| useToastStore().addAlert(t('assetBrowser.failedToCreateNode')) | |
| return null | |
| } | |
| } | |
| export async function createNode( | |
| canvas: LGraphCanvas, | |
| name: string | |
| ): Promise<LGraphNode | null> { | |
| if (!name) { | |
| return null | |
| } | |
| const { graph, graph_mouse: [ posX, posY ] } = canvas | |
| const newNode = LiteGraph.createNode(name) | |
| await new Promise(r => setTimeout(r, 0)) | |
| if (newNode && graph) { | |
| newNode.pos = [ posX, posY ] | |
| const addedNode = graph.add(newNode) ?? null | |
| return addedNode | |
| } else { | |
| useToastStore().addAlert(t('assetBrowser.failedToCreateNode')) | |
| return null | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/litegraphUtil.ts` around lines 40 - 62, The createNode function
issues two problems: remove the redundant graph.change() call after
graph.add(newNode) since graph.add already invokes this.change() (refer to
createNode, graph.add and graph.change), or if a second notification is truly
required document the reason above the call; also remove the unnecessary await
new Promise(r => setTimeout(r, 0)) between LiteGraph.createNode and graph.add
unless there's a documented asynchronous side-effect—if an async yield is
required, replace it with a clear comment explaining why (e.g., waiting for node
registration) so the intent is explicit.
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.
Tests are failing on #8282. Cursory investigation suggests that this is an issue of token management and not the branch in question, so I'm opening a copy of the branch that's in-repo
┆Issue is synchronized with this Notion page by Unito