mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-22 05:19:03 +00:00
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.
This commit is contained in:
@@ -120,4 +120,31 @@ describe('_startDraggingItems defers onSelectionChange', () => {
|
||||
|
||||
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('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
|
||||
})
|
||||
})
|
||||
|
||||
@@ -3615,11 +3615,20 @@ export class LGraphCanvas implements CustomEventDispatcher<LGraphCanvasEventMap>
|
||||
// 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
|
||||
this.onSelectionChange = undefined
|
||||
this.processSelect(item, pointer.eDown, sticky)
|
||||
this.onSelectionChange = onSelectionChange
|
||||
if (onSelectionChange) {
|
||||
let selectionNotified = false
|
||||
this.onSelectionChange = () => {
|
||||
selectionNotified = true
|
||||
}
|
||||
try {
|
||||
this.processSelect(item, pointer.eDown, sticky)
|
||||
} finally {
|
||||
this.onSelectionChange = onSelectionChange
|
||||
}
|
||||
if (onSelectionChange && selectionNotified) {
|
||||
requestAnimationFrame(() => onSelectionChange(this.selected_nodes))
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user