Restore Vue upload-node file drops and harden media drag/paste handling after #9463#10841
Open
xmarre wants to merge 16 commits intoComfy-Org:mainfrom
Open
Restore Vue upload-node file drops and harden media drag/paste handling after #9463#10841xmarre wants to merge 16 commits intoComfy-Org:mainfrom
xmarre wants to merge 16 commits intoComfy-Org:mainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
dosubot
bot
added
the
size:M
Contributor
📝 WalkthroughWalkthroughEnhanced media type detection for files with empty MIME types by introducing filename-based classification helpers. Updated paste, drag-drop, and widget file-handling to use these utilities instead of simple MIME prefix checks. Refined drop event handling in components with improved state cleanup and validation. Added comprehensive test coverage for empty-MIME-type scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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:
|
🎭 Playwright: ⏳ Running... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/components/LGraphNode.test.ts (1)
346-369: Cover the “promise resolvesfalsestill claims event” contract explicitly.Great regression test. One small gap: it only checks the
Promise<true>path. Since this PR intentionally claims the event for async handlers regardless of final resolution, add amockResolvedValue(false)variant so that behavior is locked in.Suggested test addition
+ it('should still stop propagation when onDragDrop promise resolves false', async () => { + const onDragDrop = vi.fn().mockResolvedValue(false) + mockData.mockLgraphNode = { + onDragDrop, + onDragOver: vi.fn(), + isSubgraphNode: () => false + } + + const wrapper = mountLGraphNode({ nodeData: mockNodeData }) + const parentListener = vi.fn() + const parent = wrapper.element.parentElement + expect(parent).not.toBeNull() + parent!.addEventListener('drop', parentListener) + + wrapper.element.dispatchEvent( + new Event('drop', { bubbles: true, cancelable: true }) + ) + + await Promise.resolve() + + expect(onDragDrop).toHaveBeenCalled() + expect(parentListener).not.toHaveBeenCalled() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/LGraphNode.test.ts` around lines 346 - 369, Add a second test case mirroring the existing "should stop propagation when onDragDrop returns a promise" scenario but set onDragDrop = vi.fn().mockResolvedValue(false); mount the node the same way (using mountLGraphNode with mockNodeData), attach a parent 'drop' listener (parentListener), dispatch a cancelable bubbling 'drop' event on wrapper.element, await the promise resolution (e.g., await Promise.resolve()), and assert that onDragDrop was called and parentListener was not called; reference the mockData.mockLgraphNode object and the onDragDrop mock to locate where to inject this new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/extensions/vueNodes/components/LGraphNode.test.ts`:
- Around line 346-369: Add a second test case mirroring the existing "should
stop propagation when onDragDrop returns a promise" scenario but set onDragDrop
= vi.fn().mockResolvedValue(false); mount the node the same way (using
mountLGraphNode with mockNodeData), attach a parent 'drop' listener
(parentListener), dispatch a cancelable bubbling 'drop' event on
wrapper.element, await the promise resolution (e.g., await Promise.resolve()),
and assert that onDragDrop was called and parentListener was not called;
reference the mockData.mockLgraphNode object and the onDragDrop mock to locate
where to inject this new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d532a8e-8dc2-481a-8248-e5d5ebbdd6fb
📒 Files selected for processing (2)
src/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vue
dosubot
bot
added
size:L
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/usePaste.ts`:
- Around line 72-83: The initial paste dispatch in usePaste() currently checks
item.type.startsWith(...) and therefore ignores clipboard File items with empty
MIME; update that dispatch loop to detect files by item.kind === 'file', call
item.getAsFile(), and use
hasImageType(file)/hasAudioType(file)/hasVideoType(file) to decide whether to
call pasteImageNode(), pasteAudioNode(), or pasteVideoNode(); ensure this
mirrors pasteItemsOnNode() behavior and add a regression test for usePaste()
that pastes a File with type === '' and verifies it routes to the appropriate
paste*Node handler.
In `@src/utils/eventUtils.ts`:
- Around line 36-46: The function getMediaTypeFromFile currently falls back to
getMediaTypeFromFilename even when File.type is non-empty; change
getMediaTypeFromFile to only call getMediaTypeFromFilename when type is empty
(e.g., type === ''), and if type is non-empty and doesn't start with
image|audio|video return null immediately; update the function's control flow
around the existing type.startsWith checks and the filename fallback, and add a
regression test that asserts a mismatched non-empty MIME (e.g., name
"report.jpg" with type "application/pdf") returns null to ensure precedence is
enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e865afd-ab9e-460d-9288-9c1d49c68649
📒 Files selected for processing (7)
src/composables/usePaste.test.tssrc/composables/usePaste.tssrc/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.tssrc/scripts/app.test.tssrc/scripts/app.tssrc/utils/__tests__/eventUtils.test.tssrc/utils/eventUtils.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/components/LGraphNode.test.ts (1)
347-353: Add a concreteidto the asyncmockLgraphNode.At Line 349, the drop cleanup path is keyed by
node.id; this mock currently omits it, so the guard is exercised viaundefinedrather than realistic IDs.Suggested test tweak
const onDragDrop = vi.fn().mockResolvedValue(true) mockData.mockLgraphNode = { + id: 123, onDragDrop, onDragOver: vi.fn(() => true), isSubgraphNode: () => false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/LGraphNode.test.ts` around lines 347 - 353, The test's mockLgraphNode lacks a concrete id which causes the drop-cleanup guard (keyed by node.id) to run with undefined; update the mockLgraphNode in the "should stop propagation when onDragDrop returns a promise" test to include a realistic id (e.g., id: 'node-1') so the cleanup path is exercised with a proper identifier; modify the mock object that contains onDragDrop, onDragOver and isSubgraphNode to also include this id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/extensions/vueNodes/components/LGraphNode.test.ts`:
- Around line 347-353: The test's mockLgraphNode lacks a concrete id which
causes the drop-cleanup guard (keyed by node.id) to run with undefined; update
the mockLgraphNode in the "should stop propagation when onDragDrop returns a
promise" test to include a realistic id (e.g., id: 'node-1') so the cleanup path
is exercised with a proper identifier; modify the mock object that contains
onDragDrop, onDragOver and isSubgraphNode to also include this id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2586fa1-3836-4d05-b2c0-cd8bb9a55823
📒 Files selected for processing (2)
src/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vue
added 7 commits
April 4, 2026 02:16
dosubot
bot
added
size:XL
dosubot
bot
added
size:L
dosubot
bot
added
size:XL
xmarre
changed the title
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
xmarre
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/components/LGraphNode.test.ts (1)
325-445:⚠️ Potential issue | 🟠 MajorThis bug fix still needs a browser-level regression or an explicit exemption.
These component tests cover the Vue boundary, but the reported failure only reproduced when the browser delivered the final drop to
canvas#graph-canvas. The PR metadata still shows nobrowser_tests/change and no explanation for skipping one, so the fix does not satisfy the repo’s bug-fix regression requirement yet.Based on learnings, "End-to-end regression coverage for fixes: For PRs that include bug-fix language in the title or commit subjects (fix, fixed, fixes, fixing, bugfix, hotfix), ensure at least one of the following: (1) files are changed under browser_tests/, or (2) the PR description includes a concrete, non-placeholder explanation of why an end-to-end regression test was not added."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/LGraphNode.test.ts` around lines 325 - 445, The PR adds unit tests around Vue handling in LGraphNode.test.ts but lacks the required browser-level regression for the bug that reproduced only when the browser delivered the final drop to canvas#graph-canvas; either add an end-to-end browser test under browser_tests/ that reproduces the failing drop behavior (exercise the real DOM drop to canvas#graph-canvas and assert propagation/handling matches the fix), or update the PR description to include a concrete explanation why an E2E test is not applicable; reference the related test/spec file LGraphNode.test.ts and the DOM target canvas#graph-canvas when adding the browser test or documenting the exemption.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/node/useNodeDragAndDrop.ts`:
- Around line 33-37: The Drag-and-Drop flow incorrectly treats Chrome→Firefox
BMP placeholders as real files; update the file-filtering so that any File with
type 'image/bmp' is removed before branching on "file-backed" length checks.
Concretely, change filterFiles and filterItemFiles (and the other spots
referenced near the other ranges) to filter out files with file.type ===
'image/bmp' (in addition to the existing fileFilter) and ensure any logic that
decides "is file-backed" uses the filtered array returned by getFilesFromItems /
filterFiles rather than the raw dataTransfer.files or raw File[]; keep using
getFilesFromItems for items but apply the BMP exclusion first so
extractFilesFromDragEvent receives a true URI-only path when appropriate.
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 827-845: hasFileTransfer currently treats any file item or
FileList entry as a file drop, which misclassifies Chrome→Firefox URI drags that
include a synthetic "image/bmp" placeholder; update hasFileTransfer to ignore
placeholder BMP entries (or otherwise mirror extractFilesFromDragEvent's filter)
by only returning true for file items/entries whose MIME type is not 'image/bmp'
(or not in the list extractFilesFromDragEvent ignores), so isUriOnlyDrop
correctly recognizes URI-only drops; apply the same logic where file-list checks
occur so both hasFileTransfer and isUriOnlyDrop behave consistently with
extractFilesFromDragEvent.
In `@src/scripts/app.ts`:
- Around line 610-627: Remove the drop tracing console.log added in the
app.documentDrop handler in src/scripts/app.ts: delete the entire
console.log('app.documentDrop', {...}) block (and any similar drag/upload
tracing added in this PR) so filenames, item file names, and the text/uri-list
payload are not logged; if lightweight telemetry is required instead, replace it
with a non-sensitive log that only records safe booleans or counts (e.g.,
whether a drop occurred and number of items) without including file names,
types, or URI contents.
---
Outside diff comments:
In `@src/renderer/extensions/vueNodes/components/LGraphNode.test.ts`:
- Around line 325-445: The PR adds unit tests around Vue handling in
LGraphNode.test.ts but lacks the required browser-level regression for the bug
that reproduced only when the browser delivered the final drop to
canvas#graph-canvas; either add an end-to-end browser test under browser_tests/
that reproduces the failing drop behavior (exercise the real DOM drop to
canvas#graph-canvas and assert propagation/handling matches the fix), or update
the PR description to include a concrete explanation why an E2E test is not
applicable; reference the related test/spec file LGraphNode.test.ts and the DOM
target canvas#graph-canvas when adding the browser test or documenting the
exemption.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48b9157e-c849-42e1-b597-5bfd50f33319
📒 Files selected for processing (8)
src/composables/node/useNodeDragAndDrop.test.tssrc/composables/node/useNodeDragAndDrop.tssrc/composables/node/useNodeImageUpload.tssrc/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/scripts/app.tssrc/utils/__tests__/eventUtils.test.tssrc/utils/eventUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/tests/eventUtils.test.ts
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
873-880: Minor: Consider consistent cleanup pattern for early return.This early return path clears
app.dragOverNodeunconditionally if it exists, unlikeclearDragOverState(node.id)which checks the id. This is functionally fine since a node withoutonDragDropshouldn't have been set asdragOverNode, but for consistency you could use:if (!node?.onDragDrop) { - isDraggingOver.value = false - if (app.dragOverNode) { - app.dragOverNode = null - app.canvas.setDirty(false, true) - } + clearDragOverState(node?.id) return }With
node?.idbeingundefined,clearDragOverStatewould only clearisDraggingOver.value(as intended). However, the current code explicitly clearsapp.dragOverNodewhich may be intentional for edge cases—no functional bug here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue` around lines 873 - 880, The early-return block that handles nodes without onDragDrop currently sets isDraggingOver.value = false and unconditionally clears app.dragOverNode and calls app.canvas.setDirty, which is inconsistent with the elsewhere-used clearDragOverState behavior; replace the manual cleanup with a call to clearDragOverState(node?.id) so the same id-check and cleanup logic is reused (this will clear isDraggingOver.value and only clear app.dragOverNode when the ids match) while preserving the canvas dirty handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 873-880: The early-return block that handles nodes without
onDragDrop currently sets isDraggingOver.value = false and unconditionally
clears app.dragOverNode and calls app.canvas.setDirty, which is inconsistent
with the elsewhere-used clearDragOverState behavior; replace the manual cleanup
with a call to clearDragOverState(node?.id) so the same id-check and cleanup
logic is reused (this will clear isDraggingOver.value and only clear
app.dragOverNode when the ids match) while preserving the canvas dirty handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f435411b-482a-45c4-8987-270dbcfa600f
📒 Files selected for processing (5)
src/composables/node/useNodeDragAndDrop.test.tssrc/composables/node/useNodeDragAndDrop.tssrc/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/scripts/app.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/composables/node/useNodeDragAndDrop.test.ts
- src/renderer/extensions/vueNodes/components/LGraphNode.test.ts
- src/composables/node/useNodeDragAndDrop.ts
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
1 participant
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
This fixes the regression introduced in the
v1.43.4 -> v1.43.5window by #9463, where local file drops on Vue-rendered upload nodes (for exampleLoad Image-style nodes) could highlight correctly on dragover but then escape the node path on drop.The load-bearing failure was the graph/document handoff after Vue-node drop bubbling changed. Once some Vue-node drops were allowed to bubble, the document-level drop handler in
app.tscould discard graph-canvas drops too early because they were alreadydefaultPrevented, which prevented the graph/node recovery path from running.In addition to the handoff fix, this PR also hardens the surrounding media drag/paste paths so they agree on file extraction and media-type detection when browsers expose dragged content through
DataTransferItemListor empty MIME types.Root cause
The broken invariant after #9463 was:
What happened in practice was:
dropfrequently landed oncanvas#graph-canvas.drophandler inapp.tstreatedevent.defaultPrevented === trueas "already fully handled" and returned too early.dropNode?.onDragDrop?.(event)So the primary regression was not image decoding or MIME handling by itself. The root cause was the post-#9463 routing contract between Vue-node bubbling and the document/canvas drop handler.
What changed
1) Restore the graph/document handoff in
app.tsThe document-level drop handler no longer blindly discards graph-canvas drops just because they are already
defaultPrevented.Instead, it preserves the old fast-exit only for drops outside the graph canvas/container, while still processing bubbled graph-canvas drops through the node-resolution path.
This is the actual fix for the regression introduced by #9463.
2) Keep node-level file extraction robust
useNodeDragAndDrop.tsnow falls back fromdataTransfer.filestodataTransfer.itemswhen needed.This matters because some browser/drag-source combinations expose usable file payloads through
DataTransferItemListeven whenFileListis empty or incomplete on the relevant path.3) Keep document-level file extraction consistent
eventUtils.tsnow also supports extracting files fromdataTransfer.itemswhendataTransfer.filesis empty, so the document fallback path agrees with the node-local path.4) Harden media-type detection
Media classification now:
This keeps empty-MIME file drags/pastes working without misclassifying unrelated files.
5) Keep clipboard/media paste behavior aligned
usePaste.tsnow uses the same media classification logic as the drag/drop paths, including empty-MIME cases, so the paste path no longer lags behind drag/drop handling.6) Narrow Vue-node drop ownership to the intended cases
LGraphNode.vuekeeps the URI-only carveout while ensuring file-backed drops continue throughawait node.onDragDrop(event)instead of escaping the node path.Why no
browser_tests/change was addedI did not add a Playwright/browser test for this regression because the failing behavior depends on a native OS-to-browser external file drag landing on the graph canvas, and the current
browser_tests/harness does not reproduce the browser-providedDataTransfersemantics that caused this bug.Specifically, the regression depended on details of a real external drag payload such as:
droptargetingcanvas#graph-canvasdefaultPreventedDataTransfer.items/DataTransfer.filesbehavior for external explorer dragsA synthetic DOM
DragEventin Playwright would not faithfully reproduce that browser/runtime behavior and would not have been a trustworthy regression test for the original failure mode.Instead, this PR adds boundary-level regression coverage at the actual code paths that were involved:
src/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/composables/node/useNodeDragAndDrop.test.tssrc/utils/__tests__/eventUtils.test.tssrc/composables/usePaste.test.tssrc/scripts/app.test.tsThose tests cover the routing and extraction invariants that regressed, while manual verification covered the real native external-drag path.
Test coverage added
src/renderer/extensions/vueNodes/components/LGraphNode.test.tsCovers the Vue boundary cases:
src/composables/node/useNodeDragAndDrop.test.tsCovers
DataTransfer.itemsfallback whenDataTransfer.filesis empty.src/utils/__tests__/eventUtils.test.tsCovers:
DataTransfer.itemssrc/composables/usePaste.test.tsCovers empty-MIME media paste routing by extension.
src/scripts/app.test.tsCovers app-level empty-MIME media routing behavior.
Manual verification
Verified with the real external file-drag repro that originally failed:
v1.43.4: goodv1.43.5: brokenv1.43.5frontend: fixedObserved broken behavior before the fix:
Load Image-style nodes highlighted on dragoverdroplanded oncanvas#graph-canvasObserved behavior after the fix:
Scope note
Although the regression was introduced by #9463 and manifested as a Vue-node drop-routing bug, the full fix necessarily touches the surrounding media drag/paste helpers as well, because the broken runtime path exposed inconsistencies between:
That is why this PR spans the graph/document handoff plus the related media handling helpers and regression tests.
Notes
The docstring coverage warning is non-blocking for this PR.
I intentionally did not add broad JSDoc/docstring churn across small internal frontend helpers as part of a bug-fix changeset, because that would increase patch size and regression surface without changing runtime behavior. If maintainers want that warning silenced in this PR rather than in a follow-up cleanup, I can add targeted docstrings to the new helper functions.