Compare commits

...

6 Commits

Author SHA1 Message Date
Glary-Bot
c6a4fa1ca2 test(e2e): drag-from-unselected node lands at target and matches selected path
Two browser tests exercising the FE-558 repro end-to-end:

1. Click-drag on an unselected node moves the node and leaves it
   selected. Asserts the node was *not* selected before the gesture
   and translates by >50px in both axes (well above the 3px Vue
   drag-guard and 6px CanvasPointer ClickDrift thresholds).

2. Unselected and already-selected drags produce equivalent translation
   deltas (within 4px). Guards against the deferred-onSelectionChange
   path regressing already-selected drags, which were always instant
   via the sticky=true early-return in processSelect.
2026-05-13 19:56:49 +00:00
Glary-Bot
0db984b433 test: pin captured-callback identity and canvas-teardown safety
Two non-blocking review questions about edge-case behavior. Both
guarantees were already implicit in the implementation; these tests
pin them so future refactors can't silently regress.

1. The RAF closure captures the onSelectionChange reference snapshotted
   at drag-start, not the live this.onSelectionChange. Reassigning the
   property between drag-start and the next frame must not redirect the
   pending notification.

2. Tearing down the canvas DOM between drag-start and the RAF firing
   must not throw. The deferred callback is a closure over plain
   references, so it runs to completion regardless of element lifecycle.
2026-05-11 19:26:27 +00:00
Glary-Bot
187c9e12b2 fix: schedule deferred onSelectionChange on the canvas-owning window
LGraphCanvas already supports popup-hosted canvases via getCanvasWindow().
The deferred RAF was using the global requestAnimationFrame, which would
defer against the wrong frame clock for detached windows and could lag
or stall selection sync only outside the main window.

Switch to this.getCanvasWindow().requestAnimationFrame to match every
other RAF call in this class (e.g. line 2140). getCanvasWindow() always
returns a Window — falls back to the global window when canvas is null —
so no defensive fallback is needed.

Adds a test that mocks getCanvasWindow to return a popup window and
asserts the deferred RAF is dispatched against the popup's
requestAnimationFrame, not the global.
2026-05-06 20:19:29 +00:00
Glary-Bot
8a3f06e2b7 fix: preserve canvas receiver when invoking deferred onSelectionChange
Synchronous callsites use this.onSelectionChange?.(...) which binds the
LGraphCanvas instance as the callback's receiver. The deferred RAF
invocation called the saved reference as a bare function, so this would
be undefined inside the callback under strict mode — silently changing
the contract for an extension-facing callback only on the drag-start
path.

Use Function.prototype.call so the deferred invocation matches every
other onSelectionChange call in the file.

Adds a test that pins the receiver contract.
2026-05-06 20:10:43 +00:00
Glary-Bot
d3dad95ce7 fix: address review — try/finally restoration and skip RAF on no-op processSelect
CodeRabbit raised two valid concerns:

1. If processSelect throws (e.g. through a node/extension callback), the
   plain assign-and-restore pattern leaves onSelectionChange unset and
   silently drops every later selection change on this canvas.

2. The unconditional RAF schedules a pointless store sync on the
   already-selected sticky-resselect path where processSelect early-returns
   without notifying.

Both fixes are the same shape: replace the temporary undefined with a
sentinel that records whether processSelect actually attempted a
notification, wrap processSelect in try/finally so the original listener
is always restored, then schedule the RAF only if the sentinel fired.

Adds two tests covering each guarantee.
2026-05-06 20:02:22 +00:00
Glary-Bot
1f599e014d perf: defer onSelectionChange notification in _startDraggingItems
When click-dragging an unselected node, processSelect() runs synchronously
at drag start to update selection state, then fires onSelectionChange,
which downstream triggers Vue store reactivity (canvasStore.updateSelectedItems).
That work blocks the first paint of the dragged node, producing visible lag
before the node starts following the pointer (FE-558).

Selection state must be updated synchronously (used by drag math), but the
notification can wait one frame: the user sees the node move immediately,
and Vue store updates settle by the next frame — far less perceptible than
a dragged node lagging the cursor.

Suppress onSelectionChange across the processSelect call (avoiding the
nested notification fired by deselectAll), then schedule a single RAF
notification with the post-processSelect selection set. Already-selected
node drags are unaffected (sticky=true short-circuits processSelect).
2026-05-06 19:41:20 +00:00
3 changed files with 314 additions and 1 deletions

View File

@@ -0,0 +1,87 @@
import {
comfyExpect as expect,
comfyPageFixture as test
} from '@e2e/fixtures/ComfyPage'
import type { ComfyPage } from '@e2e/fixtures/ComfyPage'
import type { Position } from '@e2e/fixtures/types'
test.describe(
'Vue Node drag-from-unselected (FE-558)',
{ tag: '@vue-nodes' },
() => {
const getHeaderPos = async (comfyPage: ComfyPage, title: string) => {
const box = await comfyPage.vueNodes
.getNodeByTitle(title)
.getByTestId('node-title')
.first()
.boundingBox()
if (!box) throw new Error(`${title} header not found`)
return box
}
const deltaBetween = (before: Position, after: Position) => ({
x: after.x - before.x,
y: after.y - before.y
})
test('drags an unselected node in a single gesture without first selecting it', async ({
comfyPage
}) => {
await comfyPage.canvasOps.moveMouseToEmptyArea()
await expect(comfyPage.vueNodes.selectedNodes).toHaveCount(0)
const before = await getHeaderPos(comfyPage, 'Load Checkpoint')
await comfyPage.canvasOps.dragAndDrop(before, {
x: before.x + 180,
y: before.y + 120
})
const after = await getHeaderPos(comfyPage, 'Load Checkpoint')
const delta = deltaBetween(before, after)
expect(Math.abs(delta.x)).toBeGreaterThan(50)
expect(Math.abs(delta.y)).toBeGreaterThan(50)
await expect(comfyPage.vueNodes.selectedNodes).toHaveCount(1)
})
test('unselected and already-selected drags produce the same translation', async ({
comfyPage
}) => {
const node = comfyPage.vueNodes.getNodeByTitle('Load Checkpoint')
// Unselected drag from current position.
await comfyPage.canvasOps.moveMouseToEmptyArea()
await expect(comfyPage.vueNodes.selectedNodes).toHaveCount(0)
const before1 = await getHeaderPos(comfyPage, 'Load Checkpoint')
await comfyPage.canvasOps.dragAndDrop(before1, {
x: before1.x + 120,
y: before1.y + 80
})
const after1 = await getHeaderPos(comfyPage, 'Load Checkpoint')
const unselectedDelta = deltaBetween(before1, after1)
// Now the node is selected from the previous drag — drag it again
// and compare deltas. The fix must not regress already-selected drags.
await expect(node).toBeVisible()
const before2 = await getHeaderPos(comfyPage, 'Load Checkpoint')
await comfyPage.canvasOps.dragAndDrop(before2, {
x: before2.x + 120,
y: before2.y + 80
})
const after2 = await getHeaderPos(comfyPage, 'Load Checkpoint')
const selectedDelta = deltaBetween(before2, after2)
// Both deltas should be ≈ (120, 80). Tolerance covers integer rounding
// and any minor canvas easing.
expect(Math.abs(unselectedDelta.x - selectedDelta.x)).toBeLessThanOrEqual(
4
)
expect(Math.abs(unselectedDelta.y - selectedDelta.y)).toBeLessThanOrEqual(
4
)
})
}
)

View File

@@ -0,0 +1,205 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { LGraph, LGraphCanvas, LGraphNode } from '@/lib/litegraph/src/litegraph'
import { createMockCanvasRenderingContext2D } from '@/utils/__tests__/litegraphTestUtils'
vi.mock('@/renderer/core/layout/store/layoutStore', () => ({
layoutStore: {
querySlotAtPoint: vi.fn(),
queryRerouteAtPoint: vi.fn(),
getNodeLayoutRef: vi.fn(() => ({ value: null })),
getSlotLayout: vi.fn(),
setSource: vi.fn(),
setActor: vi.fn()
}
}))
function createDragHarness() {
const canvasElement = document.createElement('canvas')
canvasElement.width = 800
canvasElement.height = 600
canvasElement.getContext = vi
.fn()
.mockReturnValue(createMockCanvasRenderingContext2D())
document.body.appendChild(canvasElement)
canvasElement.getBoundingClientRect = vi.fn().mockReturnValue({
left: 0,
top: 0,
right: 800,
bottom: 600,
width: 800,
height: 600,
x: 0,
y: 0,
toJSON: () => {}
})
const graph = new LGraph()
const canvas = new LGraphCanvas(canvasElement, graph, {
skip_render: true,
skip_events: true
})
const node = new LGraphNode('test')
node.size = [200, 100]
graph.add(node)
const pointer = canvas.pointer
const downEvent = new PointerEvent('pointerdown', {
pointerId: 1,
button: 0,
buttons: 1
})
Object.assign(downEvent, {
canvasX: 0,
canvasY: 0,
deltaX: 0,
deltaY: 0,
safeOffsetX: 0,
safeOffsetY: 0
})
pointer.eDown = downEvent as typeof pointer.eDown
return { canvas, canvasElement, graph, node, pointer }
}
describe('_startDraggingItems defers onSelectionChange', () => {
let canvas: LGraphCanvas
let canvasElement: HTMLCanvasElement
let node: LGraphNode
let pointer: ReturnType<typeof createDragHarness>['pointer']
beforeEach(() => {
vi.useFakeTimers()
;({ canvas, canvasElement, node, pointer } = createDragHarness())
})
afterEach(() => {
canvasElement.remove()
vi.useRealTimers()
})
it('does not call onSelectionChange synchronously when an unselected node starts dragging', () => {
const onSelectionChange = vi.fn()
canvas.onSelectionChange = onSelectionChange
canvas['_startDraggingItems'](node, pointer, true)
expect(onSelectionChange).not.toHaveBeenCalled()
})
it('calls onSelectionChange exactly once on the next animation frame', () => {
const onSelectionChange = vi.fn()
canvas.onSelectionChange = onSelectionChange
canvas['_startDraggingItems'](node, pointer, true)
expect(onSelectionChange).not.toHaveBeenCalled()
vi.advanceTimersByTime(16)
expect(onSelectionChange).toHaveBeenCalledTimes(1)
expect(onSelectionChange).toHaveBeenCalledWith(canvas.selected_nodes)
})
it('updates selection state synchronously even though the listener fires later', () => {
canvas.onSelectionChange = vi.fn()
expect(node.selected).toBeFalsy()
canvas['_startDraggingItems'](node, pointer, true)
expect(node.selected).toBe(true)
expect(canvas.selected_nodes[node.id]).toBe(node)
expect(canvas.isDragging).toBe(true)
})
it('restores onSelectionChange after processSelect so subsequent selection changes notify normally', () => {
const onSelectionChange = vi.fn()
canvas.onSelectionChange = onSelectionChange
canvas['_startDraggingItems'](node, pointer, true)
expect(canvas.onSelectionChange).toBe(onSelectionChange)
})
it('does not schedule a deferred notification when starting a drag on an already-selected sticky item', () => {
canvas.select(node)
const onSelectionChange = vi.fn()
canvas.onSelectionChange = onSelectionChange
canvas['_startDraggingItems'](node, pointer, true)
vi.advanceTimersByTime(16)
expect(onSelectionChange).not.toHaveBeenCalled()
})
it('invokes the deferred onSelectionChange with the canvas as receiver', () => {
const receivedThis: unknown[] = []
canvas.onSelectionChange = function (this: unknown) {
receivedThis.push(this)
}
canvas['_startDraggingItems'](node, pointer, true)
vi.advanceTimersByTime(16)
expect(receivedThis).toEqual([canvas])
})
it('schedules the deferred callback on the canvas-owning window (popup-host safe)', () => {
const onSelectionChange = vi.fn()
canvas.onSelectionChange = onSelectionChange
const popupRaf = vi.fn(
(_cb: FrameRequestCallback) => 1
) as unknown as Window['requestAnimationFrame']
const popupWindow = { requestAnimationFrame: popupRaf } as unknown as Window
const getCanvasWindowSpy = vi
.spyOn(canvas, 'getCanvasWindow')
.mockReturnValue(popupWindow)
canvas['_startDraggingItems'](node, pointer, true)
expect(getCanvasWindowSpy).toHaveBeenCalled()
expect(popupRaf).toHaveBeenCalledTimes(1)
getCanvasWindowSpy.mockRestore()
})
it('restores onSelectionChange even when processSelect throws', () => {
const onSelectionChange = vi.fn()
canvas.onSelectionChange = onSelectionChange
const original = canvas.processSelect
canvas.processSelect = () => {
throw new Error('boom')
}
expect(() => canvas['_startDraggingItems'](node, pointer, true)).toThrow(
'boom'
)
expect(canvas.onSelectionChange).toBe(onSelectionChange)
canvas.processSelect = original
})
it('invokes the callback captured at drag-start, not a later replacement', () => {
const captured = vi.fn()
const replacement = vi.fn()
canvas.onSelectionChange = captured
canvas['_startDraggingItems'](node, pointer, true)
canvas.onSelectionChange = replacement
vi.advanceTimersByTime(16)
expect(captured).toHaveBeenCalledTimes(1)
expect(replacement).not.toHaveBeenCalled()
})
it('does not throw if the canvas is torn down before the RAF fires', () => {
canvas.onSelectionChange = vi.fn()
canvas['_startDraggingItems'](node, pointer, true)
canvasElement.remove()
expect(() => vi.advanceTimersByTime(16)).not.toThrow()
})
})

View File

@@ -3612,7 +3612,28 @@ export class LGraphCanvas implements CustomEventDispatcher<LGraphCanvasEventMap>
this.emitAfterChange()
}
this.processSelect(item, pointer.eDown, sticky)
// Selection-update side effects (onSelectionChange callback) are deferred
// to the next frame so the node visibly starts following the pointer
// before downstream reactivity (e.g. Vue store updates) runs.
// The sentinel records whether processSelect actually notified, so we
// skip the RAF on the no-op sticky-resselect path and avoid swallowing
// the listener if processSelect throws.
const onSelectionChange = this.onSelectionChange
let selectionNotified = false
this.onSelectionChange = () => {
selectionNotified = true
}
try {
this.processSelect(item, pointer.eDown, sticky)
} finally {
this.onSelectionChange = onSelectionChange
}
if (onSelectionChange && selectionNotified) {
this.getCanvasWindow().requestAnimationFrame(() => {
onSelectionChange.call(this, this.selected_nodes)
})
}
this.isDragging = true
this._startNodeAutoPan()