fix: address viewport virtualization review feedback

Author: r-vage

browser_tests/tests/vueNodes/viewportVirtualization.spec.ts

@@ -109,8 +109,6 @@ test.describe(
       if (!nodeId) throw new Error('Expected a node in the group workflow')
       await expect.poll(() => comfyPage.vueNodes.getNodeIds()).toEqual([nodeId])
 
-      await comfyPage.page.clock.install()
-
       await comfyPage.page.evaluate(() => {
         const canvas = window.app!.canvas
         const node = window.app!.graph.nodes[0]
@@ -120,9 +118,13 @@ test.describe(
         node.updateArea()
         canvas.setDirty(true, true)
       })
-      await comfyPage.page.clock.runFor(200)
-      expect(await comfyPage.vueNodes.getNodeIds()).toEqual([nodeId])
-      await comfyPage.page.clock.resume()
+      const settleDeadline = Date.now() + 200
+      await expect
+        .poll(async () => {
+          if (Date.now() < settleDeadline) return []
+          return await comfyPage.vueNodes.getNodeIds()
+        })
+        .toEqual([nodeId])
 
       await comfyPage.page.evaluate(() => {
         window.app!.canvas.isDragging = false

src/locales/en/settings.json

@@ -430,6 +430,18 @@
     "name": "Modern Node Design (Nodes 2.0)",
     "tooltip": "Modern: DOM-based rendering with enhanced interactivity, native browser features, and updated visual design. Classic: Traditional canvas rendering."
   },
+  "Comfy_VueNodes_FullDetailZoom": {
+    "name": "Full-detail zoom",
+    "tooltip": "Nodes use full detail at and above this zoom percentage. Low detail is used only below it."
+  },
+  "Comfy_VueNodes_LowZoomLOD": {
+    "name": "Low-zoom level of detail",
+    "tooltip": "Hide expensive node details below the full-detail zoom threshold while preserving node shells, titles, sockets, links, and layout."
+  },
+  "Comfy_VueNodes_ViewportVirtualization": {
+    "name": "Viewport virtualization",
+    "tooltip": "Mount all nodes once to establish layout, then render only nodes in the settled viewport. Workflow execution and serialization are unaffected."
+  },
   "Comfy_WidgetControlMode": {
     "name": "Widget control mode",
     "tooltip": "Controls when widget values are updated (randomize/increment/decrement), either before the prompt is queued or after.",

src/platform/settings/constants/coreSettings.ts

@@ -1221,7 +1221,7 @@ export const CORE_SETTINGS: SettingParams[] = [
     tooltip:
       'Hide expensive node details below the full-detail zoom threshold while preserving node shells, titles, sockets, links, and layout.',
     defaultValue: true,
-    sortOrder: 80,
+    sortOrder: 75,
     experimental: true,
     versionAdded: '1.49.0'
   },

src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts

@@ -1,7 +1,7 @@
 import { createTestingPinia } from '@pinia/testing'
 import type * as VueUse from '@vueuse/core'
 import { setActivePinia } from 'pinia'
-import { effectScope, nextTick, ref } from 'vue'
+import { computed, effectScope, nextTick, ref } from 'vue'
 import type { ShallowRef } from 'vue'
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
@@ -277,6 +277,84 @@ describe('viewport virtualization behavior', () => {
     scope.stop()
   })
 
+  it('retries a settled refresh when the node manager becomes available', () => {
+    vi.useFakeTimers()
+    setActivePinia(createTestingPinia({ stubActions: false }))
+    const { runAnimationFrame } = stubAnimationFrames()
+    const nodeData = createNodeData(1)
+    const node = createNode(1, 0, 0)
+    const canvas = {
+      ds: {
+        scale: 1,
+        offset: [0, 0],
+        visible_area: [0, 0, 200, 200],
+        computeVisibleArea: vi.fn()
+      },
+      graph: {},
+      linkConnector: { renderLinks: [] },
+      viewport: [0, 0, 200, 200]
+    } as unknown as LGraphCanvas
+    const nodeManager = ref<GraphNodeManager | null>(null)
+    const scope = effectScope()
+    const virtualization = scope.run(() =>
+      useViewportVirtualization({
+        allNodes: [nodeData],
+        canvas,
+        enabled: true,
+        nodeManager
+      })
+    )
+    if (!virtualization)
+      throw new Error('Failed to create virtualization scope')
+
+    virtualization.onNodeMounted(nodeData.id)
+    runAnimationFrame()
+    runAnimationFrame()
+    expect(virtualization.renderedNodes.value).toEqual([])
+
+    nodeManager.value = {
+      getNode: vi.fn(() => node)
+    } as unknown as GraphNodeManager
+    vi.advanceTimersByTime(150)
+
+    expect(virtualization.renderedNodes.value).toEqual([nodeData])
+    scope.stop()
+  })
+
+  it('does not reconcile node ids when only node data changes', async () => {
+    setActivePinia(createTestingPinia({ stubActions: false }))
+    const { animationFrames, runAnimationFrame } = stubAnimationFrames()
+    const title = ref('Initial')
+    const allNodes = computed(() => [
+      {
+        ...createNodeData(1),
+        title: title.value
+      }
+    ])
+    const scope = effectScope()
+    const virtualization = scope.run(() =>
+      useViewportVirtualization({
+        allNodes,
+        canvas: null,
+        enabled: true,
+        nodeManager: null
+      })
+    )
+    if (!virtualization)
+      throw new Error('Failed to create virtualization scope')
+
+    virtualization.onNodeMounted(toNodeId(1))
+    runAnimationFrame()
+    runAnimationFrame()
+    expect(animationFrames.size).toBe(0)
+
+    title.value = 'Updated'
+    await nextTick()
+
+    expect(animationFrames.size).toBe(0)
+    scope.stop()
+  })
+
   it('recomputes the viewport after a canvas transform settles', () => {
     vi.useFakeTimers()
     setActivePinia(createTestingPinia({ stubActions: false }))

src/renderer/extensions/vueNodes/composables/useViewportVirtualization.ts

@@ -167,7 +167,10 @@ export function useViewportVirtualization({
     }
     const activeCanvas = toValue(canvas)
     const manager = toValue(nodeManager)
-    if (!activeCanvas || !manager) return
+    if (!activeCanvas || !manager) {
+      scheduleSettledRefresh()
+      return
+    }
     const mountSetFrozen =
       layoutStore.isDraggingVueNodes.value ||
       layoutStore.isResizingVueNodes.value ||
@@ -281,8 +284,9 @@ export function useViewportVirtualization({
   )
 
   watch(
-    () => toValue(allNodes).map((node) => node.id),
-    (nodeIds) => {
+    () => JSON.stringify(toValue(allNodes).map((node) => node.id)),
+    () => {
+      const nodeIds = toValue(allNodes).map((node) => node.id)
       const currentNodeIds = new Set(nodeIds)
       const nextHydratedNodeIds = new Set(
         Array.from(hydratedNodeIds.value).filter((nodeId) =>
@@ -295,9 +299,12 @@ export function useViewportVirtualization({
       for (const nodeId of pendingHydrationNodeIds) {
         if (!currentNodeIds.has(nodeId)) pendingHydrationNodeIds.delete(nodeId)
       }
-      viewportNodeIds.value = new Set(
+      const nextViewportNodeIds = new Set(
         Array.from(viewportNodeIds.value).filter((id) => currentNodeIds.has(id))
       )
+      if (!areNodeIdSetsEqual(viewportNodeIds.value, nextViewportNodeIds)) {
+        viewportNodeIds.value = nextViewportNodeIds
+      }
       if (
         nodeIds.every(
           (nodeId) =>

src/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.ts

@@ -1,6 +1,29 @@
-import { describe, expect, it } from 'vitest'
+import type * as VueUse from '@vueuse/core'
+import { effectScope } from 'vue'
+import { describe, expect, it, vi } from 'vitest'
 
-import { shouldUseVueNodeLowDetail } from './useVueNodeLOD'
+import type { LGraphCanvas } from '@/lib/litegraph/src/litegraph'
+
+import { shouldUseVueNodeLowDetail, useVueNodeLOD } from './useVueNodeLOD'
+
+const rafWatcher = vi.hoisted(() => ({
+  callback: undefined as Parameters<typeof VueUse.useRafFn>[0] | undefined
+}))
+
+vi.mock('@vueuse/core', async (importOriginal) => {
+  const vueUse = await importOriginal<typeof VueUse>()
+  return {
+    ...vueUse,
+    useRafFn: vi.fn((callback: Parameters<typeof VueUse.useRafFn>[0]) => {
+      rafWatcher.callback = callback
+      return {
+        isActive: true,
+        pause: vi.fn(),
+        resume: vi.fn()
+      }
+    })
+  }
+})
 
 describe('shouldUseVueNodeLowDetail', () => {
   it('uses low detail strictly below the percentage threshold', () => {
@@ -25,4 +48,29 @@ describe('shouldUseVueNodeLowDetail', () => {
     expect(shouldUseVueNodeLowDetail(0.99, true, 200, true)).toBe(true)
     expect(shouldUseVueNodeLowDetail(1, true, 200, true)).toBe(false)
   })
+
+  it('does not reevaluate an unchanged NaN scale on every frame', () => {
+    const canvas = {
+      ds: { scale: Number.NaN }
+    } as unknown as LGraphCanvas
+    const fullDetailZoom = vi.fn(() => 95)
+    const scope = effectScope()
+    scope.run(() =>
+      useVueNodeLOD({
+        canvas,
+        enabled: true,
+        fullDetailZoom,
+        vueNodesEnabled: true
+      })
+    )
+    const callback = rafWatcher.callback
+    if (!callback) throw new Error('RAF watcher callback was not captured')
+
+    callback({ delta: 0, timestamp: 0 })
+    const evaluationCount = fullDetailZoom.mock.calls.length
+    callback({ delta: 0, timestamp: 16 })
+
+    expect(fullDetailZoom).toHaveBeenCalledTimes(evaluationCount)
+    scope.stop()
+  })
 })

src/renderer/extensions/vueNodes/composables/useVueNodeLOD.ts

@@ -65,7 +65,7 @@ export function useVueNodeLOD({
 
   useRafFn(() => {
     const scale = toValue(canvas)?.ds.scale
-    if (scale == null || scale === lastScale) return
+    if (scale == null || Object.is(scale, lastScale)) return
     lastScale = scale
     update(scale)
   })