Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/design-system/src/css/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -1328,6 +1328,15 @@ audio.comfy-audio.empty-audio-widget {
font-size 0.1s ease;
}

/* Performance optimization during canvas interaction */
.transform-pane--interacting .lg-node * {
transition: none !important;
}

.transform-pane--interacting .lg-node {
will-change: transform;
}
Comment on lines +1331 to +1338

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Over-broad * { transition: none !important; } risks unintended UI regressions; prefer targeting known transitioning elements.

Right now this disables all descendant transitions under nodes during zoom, not just the LiteGraph node transition surfaces. Given you already define transitions on .lg-node .lg-slot / .lg-node .lg-widget (Line 1324-1329), consider scoping the override to those instead of *.

 /* Performance optimization during canvas interaction */
-.transform-pane--interacting .lg-node * {
-  transition: none !important;
-}
+.transform-pane--interacting .lg-node .lg-slot,
+.transform-pane--interacting .lg-node .lg-widget {
+  transition: none !important;
+}
 
 .transform-pane--interacting .lg-node {
   will-change: transform;
 }
🤖 Prompt for AI Agents
In packages/design-system/src/css/style.css around lines 1331 to 1338, the rule
using the universal selector to disable transitions is too broad and can cause
regressions; narrow the selector to only the known transition targets (e.g.,
.lg-node .lg-slot and .lg-node .lg-widget or any other specific transitioning
descendants) so that during .transform-pane--interacting only those elements
receive transition: none !important, leaving unrelated descendants unaffected.


/* ===================== Mask Editor Styles ===================== */
/* To be migrated to Tailwind later */
#maskEditor_brush {
Expand Down
71 changes: 70 additions & 1 deletion src/renderer/core/layout/__tests__/TransformPane.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,73 @@ describe('TransformPane', () => {
})
})

describe('canvas event listeners', () => {
it('should add event listeners to canvas on mount', async () => {
const mockCanvas = createMockCanvas()
mount(TransformPane, {
props: {
canvas: mockCanvas
}
})

await nextTick()

expect(mockCanvas.canvas.addEventListener).toHaveBeenCalledWith(
'wheel',
expect.any(Function),
expect.any(Object)
)
expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
'pointerdown',
expect.any(Function),
expect.any(Object)
)
expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
'pointerup',
expect.any(Function),
expect.any(Object)
)
expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
'pointercancel',
expect.any(Function),
expect.any(Object)
)
})

it('should remove event listeners on unmount', async () => {
const mockCanvas = createMockCanvas()
const wrapper = mount(TransformPane, {
props: {
canvas: mockCanvas
}
})

await nextTick()
wrapper.unmount()

expect(mockCanvas.canvas.removeEventListener).toHaveBeenCalledWith(
'wheel',
expect.any(Function),
expect.any(Object)
)
expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
'pointerdown',
expect.any(Function),
expect.any(Object)
)
expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
'pointerup',
expect.any(Function),
expect.any(Object)
)
expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
'pointercancel',
expect.any(Function),
expect.any(Object)
)
})
})
Comment on lines +120 to +185

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Event-listener tests should assert handler identity (and avoid brittle “no pointer listeners” assertions).

removeEventListener requires the same function reference and compatible options; expect.any(Function) can pass even if the component leaks the real handler. Also, “not called with pointer*” will break if pointer listeners are legitimately added later.

Consider tightening to “wheel is registered with capture/passive” and “wheel is removed with the same handler”:

 it('should add event listeners to canvas on mount', async () => {
   const mockCanvas = createMockCanvas()
   mount(TransformPane, {
     props: {
       canvas: mockCanvas
     }
   })
 
   await nextTick()
 
-  expect(mockCanvas.canvas.addEventListener).toHaveBeenCalledWith(
-    'wheel',
-    expect.any(Function),
-    expect.any(Object)
-  )
-  expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
-    'pointerdown',
-    expect.any(Function),
-    expect.any(Object)
-  )
-  expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
-    'pointerup',
-    expect.any(Function),
-    expect.any(Object)
-  )
-  expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
-    'pointercancel',
-    expect.any(Function),
-    expect.any(Object)
-  )
+  const [type, handler, options] =
+    (mockCanvas.canvas.addEventListener as unknown as { mock: { calls: unknown[][] } })
+      .mock.calls[0]
+
+  expect(type).toBe('wheel')
+  expect(handler).toEqual(expect.any(Function))
+  expect(options).toEqual(expect.objectContaining({ capture: true }))
 })
 
 it('should remove event listeners on unmount', async () => {
   const mockCanvas = createMockCanvas()
   const wrapper = mount(TransformPane, {
     props: {
       canvas: mockCanvas
     }
   })
 
   await nextTick()
+  const [, handler, options] =
+    (mockCanvas.canvas.addEventListener as unknown as { mock: { calls: unknown[][] } })
+      .mock.calls[0]
   wrapper.unmount()
 
-  expect(mockCanvas.canvas.removeEventListener).toHaveBeenCalledWith(
-    'wheel',
-    expect.any(Function),
-    expect.any(Object)
-  )
-  expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
-    'pointerdown',
-    expect.any(Function),
-    expect.any(Object)
-  )
-  expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
-    'pointerup',
-    expect.any(Function),
-    expect.any(Object)
-  )
-  expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
-    'pointercancel',
-    expect.any(Function),
-    expect.any(Object)
-  )
+  expect(mockCanvas.canvas.removeEventListener).toHaveBeenCalledWith(
+    'wheel',
+    handler as EventListener,
+    options as AddEventListenerOptions
+  )
 })

(If you’d rather avoid mocking intricacies altogether, consider using a real HTMLCanvasElement and vi.spyOn(canvasEl, 'addEventListener').)
Based on learnings, prefer assertions that reflect behavior over internal wiring where possible.

🤖 Prompt for AI Agents
In src/renderer/core/layout/__tests__/TransformPane.test.ts around lines 120 to
185, the tests currently assert add/remove calls using expect.any(Function) and
negative assertions for pointer listeners which is brittle; update the tests to
(1) capture the actual handler reference returned/attached when addEventListener
is spied or when using a real HTMLCanvasElement so you can assert the same
function is used in removeEventListener, (2) assert the wheel listener was
registered with the expected options (e.g. passive/capture) rather than broadly
checking any function, and (3) remove the
“not.toHaveBeenCalledWith('pointer...')” negative assertions or replace them
with behavior-based assertions; alternatively switch to a real canvas element
and use vi.spyOn(canvasEl, 'addEventListener') so you can compare handler
identity and options precisely and then assert removeEventListener was called
with that exact handler and matching options.


describe('interaction state management', () => {
it('should apply interacting class during interactions', async () => {
const mockCanvas = createMockCanvas()
Expand All @@ -131,7 +198,9 @@ describe('TransformPane', () => {
const transformPane = wrapper.find('[data-testid="transform-pane"]')

// Initially should not have interacting class
expect(transformPane.classes()).not.toContain('will-change-transform')
expect(transformPane.classes()).not.toContain(
'transform-pane--interacting'
)
Comment on lines +201 to +203

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Interaction class test only asserts the initial state; consider validating the “interacting” state is reachable.

Right now it only checks the class is absent initially. If feasible, dispatch a wheel event on a real canvas element (and advance fake timers) to assert transform-pane--interacting toggles on/off.

🤖 Prompt for AI Agents
In src/renderer/core/layout/__tests__/TransformPane.test.ts around lines 201 to
203, the test only asserts the initial absence of the
'transform-pane--interacting' class; extend it to actually trigger and verify
the interacting state by dispatching a real 'wheel' event on the canvas element
and advancing fake timers to observe the class toggle. Specifically, get the
actual canvas DOM element from the mounted wrapper, dispatch a WheelEvent with a
non-zero delta (e.g., deltaY), assert the wrapper now contains
'transform-pane--interacting', then advance timers by the interaction timeout
duration and assert the class is removed; ensure
jest.useFakeTimers/jest.advanceTimersByTime (or equivalent) are used and timers
are restored/cleared after the test.

})

it('should handle pointer events for node delegation', async () => {
Expand Down
21 changes: 20 additions & 1 deletion src/renderer/core/layout/transform/TransformPane.vue
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
<template>
<div
data-testid="transform-pane"
class="absolute inset-0 w-full h-full pointer-events-none"
:class="
cn(
'absolute inset-0 w-full h-full pointer-events-none',
isInteracting ? 'transform-pane--interacting' : 'will-change-auto'
)
"
:style="transformStyle"
>
<!-- Vue nodes will be rendered here -->
Expand All @@ -11,9 +16,12 @@

<script setup lang="ts">
import { useRafFn } from '@vueuse/core'
import { computed } from 'vue'

import type { LGraphCanvas } from '@/lib/litegraph/src/litegraph'
import { useTransformSettling } from '@/renderer/core/layout/transform/useTransformSettling'
import { useTransformState } from '@/renderer/core/layout/transform/useTransformState'
import { cn } from '@/utils/tailwindUtil'

interface TransformPaneProps {
canvas?: LGraphCanvas
Expand All @@ -23,6 +31,11 @@ const props = defineProps<TransformPaneProps>()

const { transformStyle, syncWithCanvas } = useTransformState()

const canvasElement = computed(() => props.canvas?.canvas)
const { isTransforming: isInteracting } = useTransformSettling(canvasElement, {
settleDelay: 16
})

useRafFn(
() => {
if (!props.canvas) {
Expand All @@ -33,3 +46,9 @@ useRafFn(
{ immediate: true }
)
</script>

<style scoped>
.transform-pane--interacting {
will-change: transform;
}
</style>
84 changes: 84 additions & 0 deletions src/renderer/core/layout/transform/useTransformSettling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { useDebounceFn, useEventListener } from '@vueuse/core'
import { ref } from 'vue'
import type { MaybeRefOrGetter } from 'vue'

interface TransformSettlingOptions {
/**
* Delay in ms before transform is considered "settled" after last interaction
* @default 200
*/
settleDelay?: number
/**
* Whether to use passive event listeners (better performance but can't preventDefault)
* @default true
*/
passive?: boolean
}
Comment on lines +5 to +16

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/renderer/core/layout/transform/useTransformSettling.ts

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 2905


🌐 Web query:

VueUse useEventListener return type documentation

💡 Result:

The VueUse docs show the TypeScript declarations for useEventListener (including its return type). In short: useEventListener returns a cleanup function you can call to unregister the listener, and the library also exports a type alias UseEventListenerReturn = ReturnType. See the Type Declarations in the docs for details and overloads. [1][2]

Sources:
[1] VueUse useEventListener (Type declarations). https://vueuse.org/core/useeventlistener/
[2] VueUse docs (versioned/type alias showing UseEventListenerReturn = ReturnType). https://v7-7-1.vueuse.org/core/useeventlistener/


🏁 Script executed:

rg "useTransformSettling" --type ts --type vue -A 3 -B 1

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 97


🏁 Script executed:

rg "useTransformSettling" -A 3 -B 1

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 5818


🏁 Script executed:

cat -n tests-ui/tests/composables/graph/useTransformSettling.test.ts

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 6338


Fix documentation mismatch for default settleDelay value.

The JSDoc @default 200 at line 8 disagrees with the actual implementation at line 49 which uses settleDelay = 256. Please align the documentation to match the actual default value of 256.

Additionally, consider whether returning the stop() function from useEventListener would improve API flexibility. While the composable currently works correctly via automatic cleanup in component setup contexts (as verified by the test suite), exposing the stop function would allow explicit cleanup if the composable is ever used outside a Vue effect scope.

🤖 Prompt for AI Agents
In src/renderer/core/layout/transform/useTransformSettling.ts around lines 5 to
16, update the JSDoc for TransformSettlingOptions to state the correct default
settleDelay of 256 (replace @default 200 with @default 256) so docs match the
implementation, and modify the composable to expose the stop() cleanup function
returned by useEventListener (either return stop from useTransformSettling or
include it in the composable's returned object) so callers can explicitly
teardown listeners when used outside a Vue effect scope.


/**
* Tracks when canvas zoom transforms are actively changing vs settled.
*
* This composable helps optimize rendering quality during zoom transformations.
* When the user is actively zooming, we can reduce rendering quality
* for better performance. Once the transform "settles" (stops changing), we can
* trigger high-quality re-rasterization.
*
* The settling concept prevents constant quality switching during interactions
* by waiting for a period of inactivity before considering the transform complete.
*
* Uses VueUse's useEventListener for automatic cleanup and useDebounceFn for
* efficient settle detection.
*
* @example
* ```ts
* const { isTransforming } = useTransformSettling(canvasRef, {
* settleDelay: 200
* })
*
* // Use in CSS classes or rendering logic
* const cssClass = computed(() => ({
* 'low-quality': isTransforming.value,
* 'high-quality': !isTransforming.value
* }))
* ```
*/
export function useTransformSettling(
target: MaybeRefOrGetter<HTMLElement | null | undefined>,
options: TransformSettlingOptions = {}
) {
const { settleDelay = 256, passive = true } = options

const isTransforming = ref(false)

/**
* Mark transform as active
*/
const markTransformActive = () => {
isTransforming.value = true
}

/**
* Mark transform as settled (debounced)
*/
const markTransformSettled = useDebounceFn(() => {
isTransforming.value = false
}, settleDelay)

/**
* Handle zoom transform event - mark active then queue settle
*/
const handleWheel = () => {
markTransformActive()
void markTransformSettled()
}

// Register wheel event listener with auto-cleanup
useEventListener(target, 'wheel', handleWheel, {
capture: true,
passive
})

return {
isTransforming
}
}
Loading