fix: node library drag drop to add node appears in wrong place in firefox#12419
Draft
pythongosssss wants to merge 2 commits into
Draft
fix: node library drag drop to add node appears in wrong place in firefox#12419pythongosssss wants to merge 2 commits into
pythongosssss wants to merge 2 commits into
Conversation
… position (https://bugzilla.mozilla.org/show_bug.cgi?id=1773886) - change to track during drag events
Contributor
📝 WalkthroughWalkthroughuseNodeDragToCanvas records native dragover client coordinates (ignoring (0,0)), registers a document dragover listener, clears tracking on cancel, and prefers tracked coordinates when placing nodes on native drop; tests validate tracked-coordinate preference, fallbacks, ordering, and clearing. ChangesNative drag position tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: ✅ Built — View Storybook |
🎭 Playwright: ✅ 1636 passed, 0 failed · 2 flaky📊 Browser Reports
|
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/composables/node/useNodeDragToCanvas.test.ts`:
- Around line 460-544: Add a new test in the "native drag position tracking"
suite that verifies dragover events fired before a native drag is started do not
affect later handleNativeDrop coordinates: call setupGlobalListeners(),
fireDrag(...) to simulate a pre-start dragover, then startDrag(mockNodeDef,
'native'), call handleNativeDrop(...) with known coords, and assert
mockConvertEventToCanvasOffset was called with the handleNativeDrop
clientX/clientY (e.g., {clientX: 300, clientY: 300}); reference
helpers/useNodeDragToCanvas functions startDrag, setupGlobalListeners,
handleNativeDrop and the local fireDrag helper when locating where to add the
test.
In `@src/composables/node/useNodeDragToCanvas.ts`:
- Around line 23-26: trackNativeDragPosition currently always sets
lastNativeDragPosition on every dragover causing stale coordinates to be used by
handleNativeDrop; modify trackNativeDragPosition to only set
lastNativeDragPosition when a native drag is actually in progress (e.g., when
e.dataTransfer exists and its types indicate a dragged payload) and otherwise
clear/reset lastNativeDragPosition (e.g., null/undefined) so stale positions are
not used; apply the same guarding/reset logic to the other native-drag handlers
referenced in this file (the other dragover/drop helpers around the
handleNativeDrop flow) so handleNativeDrop reads a valid current position or
falls back safely.
🪄 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: 9f79b7d6-9078-4d2f-bb52-fa4eebdec1fd
📒 Files selected for processing (2)
src/composables/node/useNodeDragToCanvas.test.tssrc/composables/node/useNodeDragToCanvas.ts
Comment on lines
+23
to
+26
| function trackNativeDragPosition(e: DragEvent) { | ||
| if (e.clientX === 0 && e.clientY === 0) return | ||
| lastNativeDragPosition.value = { x: e.clientX, y: e.clientY } | ||
| } |
Contributor
There was a problem hiding this comment.
Guard/reset tracked native coordinates to prevent stale drop positions.
lastNativeDragPosition is currently updated for any document dragover, even when no native node drag is active. That stale value is then preferred in handleNativeDrop, which can place a node at an unrelated prior drag location.
💡 Suggested fix
function trackNativeDragPosition(e: DragEvent) {
+ if (!isDragging.value || dragMode.value !== 'native') return
if (e.clientX === 0 && e.clientY === 0) return
lastNativeDragPosition.value = { x: e.clientX, y: e.clientY }
}
...
export function useNodeDragToCanvas() {
function startDrag(nodeDef: ComfyNodeDefImpl, mode: DragMode = 'click') {
isDragging.value = true
draggedNode.value = nodeDef
dragMode.value = mode
+ lastNativeDragPosition.value = null
}Also applies to: 113-117, 119-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/composables/node/useNodeDragToCanvas.ts` around lines 23 - 26,
trackNativeDragPosition currently always sets lastNativeDragPosition on every
dragover causing stale coordinates to be used by handleNativeDrop; modify
trackNativeDragPosition to only set lastNativeDragPosition when a native drag is
actually in progress (e.g., when e.dataTransfer exists and its types indicate a
dragged payload) and otherwise clear/reset lastNativeDragPosition (e.g.,
null/undefined) so stale positions are not used; apply the same guarding/reset
logic to the other native-drag handlers referenced in this file (the other
dragover/drop helpers around the handleNativeDrop flow) so handleNativeDrop
reads a valid current position or falls back safely.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #12419 +/- ##
===========================================
- Coverage 74.58% 60.03% -14.55%
===========================================
Files 1529 1417 -112
Lines 92743 72540 -20203
Branches 25373 20154 -5219
===========================================
- Hits 69169 43551 -25618
- Misses 22725 28516 +5791
+ Partials 849 473 -376
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1027 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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
Firefox can give invalid drag coordinates causing incorrect drop position (https://bugzilla.mozilla.org/show_bug.cgi?id=1773886)
I was unable to consistently recreate this issue and it only happened in Firefox, so not a candidate for e2e tests.
Changes
dragoverwhich looks to reliably report the correct position and use that value on dropScreenshots (if applicable)
dragandbad.mp4