CodeRabbit Generated Unit Tests: Add tests for ensureCorrectLayoutScale and LGraph types

Consistent names and order of operations

src/lib/litegraph/src/LGraph.ts

@@ -79,7 +79,7 @@ export type {
   LGraphTriggerParam
 } from './types/graphTriggers'
 
-export type rendererType = 'LG' | 'Vue'
+export type RendererType = 'LG' | 'Vue'
 
 export interface LGraphState {
   lastGroupId: number
@@ -106,7 +106,7 @@ export interface LGraphExtra extends Dictionary<unknown> {
   reroutes?: SerialisableReroute[]
   linkExtensions?: { id: number; parentId: number | undefined }[]
   ds?: DragAndScaleState
-  workflowRendererVersion?: rendererType
+  workflowRendererVersion?: RendererType
 }
 
 export interface BaseLGraph {

src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts

@@ -1,5 +1,5 @@
 import { useVueFeatureFlags } from '@/composables/useVueFeatureFlags'
-import type { LGraph, rendererType } from '@/lib/litegraph/src/LGraph'
+import type { LGraph, RendererType } from '@/lib/litegraph/src/LGraph'
 import { LiteGraph } from '@/lib/litegraph/src/litegraph'
 import { createBounds } from '@/lib/litegraph/src/measure'
 import { useSettingStore } from '@/platform/settings/settingStore'
@@ -13,135 +13,108 @@ import type { SubgraphOutputNode } from '@/lib/litegraph/src/subgraph/SubgraphOu
 const SCALE_FACTOR = 1.2
 
 export function ensureCorrectLayoutScale(
-  renderer?: rendererType,
+  renderer: RendererType = 'LG',
   targetGraph?: LGraph
 ) {
-  const settingStore = useSettingStore()
-
-  const autoScaleLayoutSetting = settingStore.get(
+  const autoScaleLayoutSetting = useSettingStore().get(
     'Comfy.VueNodes.AutoScaleLayout'
   )
 
-  if (autoScaleLayoutSetting === false) {
-    return
-  }
-
-  const { shouldRenderVueNodes } = useVueFeatureFlags()
+  if (!autoScaleLayoutSetting) return
 
   const canvas = comfyApp.canvas
   const graph = targetGraph ?? canvas?.graph
 
-  if (!graph || !graph.nodes) return
+  if (!graph?.nodes) return
 
-  // Use renderer from graph, default to 'LG' for the check (but don't modify graph yet)
-  if (!renderer) {
-    // Always assume legacy LG format when unknown (pre-dates this feature)
-    renderer = 'LG'
-  }
+  const { shouldRenderVueNodes } = useVueFeatureFlags()
 
-  const doesntNeedScale =
-    (renderer === 'LG' && shouldRenderVueNodes.value === false) ||
-    (renderer === 'Vue' && shouldRenderVueNodes.value === true)
+  const needsUpscale = renderer === 'LG' && shouldRenderVueNodes.value
+  const needsDownscale = renderer === 'Vue' && !shouldRenderVueNodes.value
 
-  if (doesntNeedScale) {
+  if (!needsUpscale && !needsDownscale) {
     // Don't scale, but ensure workflowRendererVersion is set for future checks
-    if (!graph.extra.workflowRendererVersion) {
-      graph.extra.workflowRendererVersion = renderer
-    }
+    graph.extra.workflowRendererVersion ??= renderer
     return
   }
 
-  const needsUpscale = renderer === 'LG' && shouldRenderVueNodes.value === true
-  const needsDownscale =
-    renderer === 'Vue' && shouldRenderVueNodes.value === false
-
   const lgBounds = createBounds(graph.nodes)
 
   if (!lgBounds) return
 
-  const originX = lgBounds[0]
-  const originY = lgBounds[1]
+  const [originX, originY] = lgBounds
 
   const lgNodesById = new Map(graph.nodes.map((node) => [node.id, node]))
 
   const yjsMoveNodeUpdates: NodeBoundsUpdate[] = []
 
-  const scaleFactor = needsUpscale
-    ? SCALE_FACTOR
-    : needsDownscale
-      ? 1 / SCALE_FACTOR
-      : 1
+  const scaleFactor = needsUpscale ? SCALE_FACTOR : 1 / SCALE_FACTOR
+
+  const onActiveGraph = !targetGraph || targetGraph === canvas?.graph
 
   //TODO: once we remove the need for LiteGraph.NODE_TITLE_HEIGHT in vue nodes we nned to remove everything here.
   for (const node of graph.nodes) {
     const lgNode = lgNodesById.get(node.id)
     if (!lgNode) continue
 
-    const lgBodyY = lgNode.pos[1]
+    const [oldX, oldY] = lgNode.pos
 
-    const adjustedY = needsDownscale
-      ? lgBodyY - LiteGraph.NODE_TITLE_HEIGHT / 2
-      : lgBodyY
+    const adjustedY = oldY - (needsUpscale ? LiteGraph.NODE_TITLE_HEIGHT : 0)
 
-    const relativeX = lgNode.pos[0] - originX
+    const relativeX = oldX - originX
     const relativeY = adjustedY - originY
-    const newX = originX + relativeX * scaleFactor
-    const scaledY = originY + relativeY * scaleFactor
-    const newWidth = lgNode.width * scaleFactor
-    const newHeight = lgNode.height * scaleFactor
 
-    const finalY = needsUpscale
-      ? scaledY + LiteGraph.NODE_TITLE_HEIGHT / 2
-      : scaledY
+    const scaledX = originX + relativeX * scaleFactor
+    const scaledY = originY + relativeY * scaleFactor
+
+    const scaledWidth = lgNode.width * scaleFactor
+    const scaledHeight =
+      lgNode.height * scaleFactor -
+      (needsUpscale ? 0 : LiteGraph.NODE_TITLE_HEIGHT)
+
+    const finalY = scaledY + (needsUpscale ? 0 : LiteGraph.NODE_TITLE_HEIGHT) // Litegraph Position further down
 
     // Directly update LiteGraph node to ensure immediate consistency
     // Dont need to reference vue directly because the pos and dims are already in yjs
-    lgNode.pos[0] = newX
+    lgNode.pos[0] = scaledX
     lgNode.pos[1] = finalY
-    lgNode.size[0] = newWidth
-    lgNode.size[1] =
-      newHeight - (needsDownscale ? LiteGraph.NODE_TITLE_HEIGHT : 0)
+    lgNode.size[0] = scaledWidth
+    lgNode.size[1] = scaledHeight
 
     // Track updates for layout store (only if this is the active graph)
-    if (!targetGraph || targetGraph === canvas?.graph) {
+    if (onActiveGraph) {
       yjsMoveNodeUpdates.push({
         nodeId: String(lgNode.id),
         bounds: {
-          x: newX,
+          x: scaledX,
           y: finalY,
-          width: newWidth,
-          height: newHeight - (needsDownscale ? LiteGraph.NODE_TITLE_HEIGHT : 0)
+          width: scaledWidth,
+          height: scaledHeight
         }
       })
     }
   }
 
-  if (
-    (!targetGraph || targetGraph === canvas?.graph) &&
-    yjsMoveNodeUpdates.length > 0
-  ) {
+  if (onActiveGraph && yjsMoveNodeUpdates.length > 0) {
     layoutStore.batchUpdateNodeBounds(yjsMoveNodeUpdates)
   }
 
   for (const reroute of graph.reroutes.values()) {
-    const oldX = reroute.pos[0]
-    const oldY = reroute.pos[1]
+    const [oldX, oldY] = reroute.pos
 
     const relativeX = oldX - originX
     const relativeY = oldY - originY
-    const newX = originX + relativeX * scaleFactor
-    const newY = originY + relativeY * scaleFactor
 
-    reroute.pos = [newX, newY]
+    const scaledX = (originX + relativeX) * scaleFactor
+    const scaledY = (originY + relativeY) * scaleFactor
 
-    if (
-      (!targetGraph || targetGraph === canvas?.graph) &&
-      shouldRenderVueNodes.value
-    ) {
+    reroute.pos = [scaledX, scaledY]
+
+    if (onActiveGraph && shouldRenderVueNodes.value) {
       const layoutMutations = useLayoutMutations()
       layoutMutations.moveReroute(
         reroute.id,
-        { x: newX, y: newY },
+        { x: scaledX, y: scaledY },
         { x: oldX, y: oldY }
       )
     }
@@ -153,60 +126,48 @@ export function ensureCorrectLayoutScale(
       graph.outputNode as SubgraphOutputNode
     ]
     for (const ioNode of ioNodes) {
-      const oldX = ioNode.pos[0]
-      const oldY = ioNode.pos[1]
-      const oldWidth = ioNode.size[0]
-      const oldHeight = ioNode.size[1]
+      const [oldX, oldY] = ioNode.pos
+      const [oldWidth, oldHeight] = ioNode.size
 
       const relativeX = oldX - originX
       const relativeY = oldY - originY
-      const newX = originX + relativeX * scaleFactor
-      const newY = originY + relativeY * scaleFactor
-      const newWidth = oldWidth * scaleFactor
-      const newHeight = oldHeight * scaleFactor
 
-      ioNode.pos = [newX, newY]
-      ioNode.size = [newWidth, newHeight]
+      const scaledX = originX + relativeX * scaleFactor
+      const scaledY = originY + relativeY * scaleFactor
+
+      const scaledWidth = oldWidth * scaleFactor
+      const scaledHeight = oldHeight * scaleFactor
+
+      ioNode.pos = [scaledX, scaledY]
+      ioNode.size = [scaledWidth, scaledHeight]
     }
   }
 
   graph.groups.forEach((group) => {
-    const originalPosX = group.pos[0]
-    const originalPosY = group.pos[1]
-    const originalWidth = group.size[0]
-    const originalHeight = group.size[1]
+    const [oldX, oldY] = group.pos
+    const [oldWidth, oldHeight] = group.size
 
-    const adjustedY = needsDownscale
-      ? originalPosY - LiteGraph.NODE_TITLE_HEIGHT
-      : originalPosY
+    const adjustedY = oldY - (needsUpscale ? LiteGraph.NODE_TITLE_HEIGHT : 0)
 
-    const relativeX = originalPosX - originX
+    const relativeX = oldX - originX
     const relativeY = adjustedY - originY
 
-    const newWidth = originalWidth * scaleFactor
-    const newHeight = originalHeight * scaleFactor
-
     const scaledX = originX + relativeX * scaleFactor
     const scaledY = originY + relativeY * scaleFactor
 
-    const finalY = needsUpscale
-      ? scaledY + LiteGraph.NODE_TITLE_HEIGHT
-      : scaledY
+    const scaledWidth = oldWidth * scaleFactor
+    const scaledHeight = oldHeight * scaleFactor
+
+    const finalY = scaledY + (needsUpscale ? 0 : LiteGraph.NODE_TITLE_HEIGHT)
 
     group.pos = [scaledX, finalY]
-    group.size = [newWidth, newHeight]
+    group.size = [scaledWidth, scaledHeight]
   })
 
-  if ((!targetGraph || targetGraph === canvas?.graph) && canvas) {
+  if (onActiveGraph && canvas) {
     const originScreen = canvas.ds.convertOffsetToCanvas([originX, originY])
     canvas.ds.changeScale(canvas.ds.scale / scaleFactor, originScreen)
   }
 
-  if (needsUpscale) {
-    graph.extra.workflowRendererVersion = 'Vue'
-  }
-
-  if (needsDownscale) {
-    graph.extra.workflowRendererVersion = 'LG'
-  }
+  graph.extra.workflowRendererVersion = needsUpscale ? 'Vue' : 'LG'
 }

tests-ui/LGraph.type.test.ts

@@ -0,0 +1,161 @@
+import { describe, it, expect } from 'vitest'
+import type { RendererType, LGraphExtra } from '../src/lib/litegraph/src/LGraph'
+
+/**
+ * Tests for the RendererType (formerly rendererType) type definition
+ * 
+ * This tests the type rename from 'rendererType' to 'RendererType' to follow
+ * TypeScript naming conventions where types should use PascalCase.
+ */
+describe('RendererType', () => {
+  describe('type definition', () => {
+    it('should accept "LG" as a valid value', () => {
+      const renderer: RendererType = 'LG'
+      expect(renderer).toBe('LG')
+    })
+
+    it('should accept "Vue" as a valid value', () => {
+      const renderer: RendererType = 'Vue'
+      expect(renderer).toBe('Vue')
+    })
+
+    it('should be assignable to LGraphExtra workflowRendererVersion property', () => {
+      const extra: LGraphExtra = {
+        workflowRendererVersion: 'LG'
+      }
+      expect(extra.workflowRendererVersion).toBe('LG')
+
+      extra.workflowRendererVersion = 'Vue'
+      expect(extra.workflowRendererVersion).toBe('Vue')
+    })
+
+    it('should allow undefined for workflowRendererVersion', () => {
+      const extra: LGraphExtra = {
+        workflowRendererVersion: undefined
+      }
+      expect(extra.workflowRendererVersion).toBeUndefined()
+    })
+
+    it('should work with optional chaining', () => {
+      const extra: LGraphExtra = {}
+      const version = extra.workflowRendererVersion
+      expect(version).toBeUndefined()
+    })
+  })
+
+  describe('type safety', () => {
+    it('should prevent assignment of invalid string values at compile time', () => {
+      // This test validates TypeScript compiler behavior
+      // The following would cause a compile error:
+      // const invalid: RendererType = 'Invalid'
+      
+      // We can test runtime validation if needed
+      const validValues: RendererType[] = ['LG', 'Vue']
+      validValues.forEach((value) => {
+        expect(['LG', 'Vue']).toContain(value)
+      })
+    })
+
+    it('should work with type guards', () => {
+      const isValidRendererType = (value: string): value is RendererType => {
+        return value === 'LG' || value === 'Vue'
+      }
+
+      expect(isValidRendererType('LG')).toBe(true)
+      expect(isValidRendererType('Vue')).toBe(true)
+      expect(isValidRendererType('Invalid')).toBe(false)
+    })
+
+    it('should work in switch statements', () => {
+      const testSwitch = (renderer: RendererType): string => {
+        switch (renderer) {
+          case 'LG':
+            return 'Legacy LiteGraph'
+          case 'Vue':
+            return 'Vue Renderer'
+          default:
+            // TypeScript should ensure this is unreachable
+            const _exhaustive: never = renderer
+            return _exhaustive
+        }
+      }
+
+      expect(testSwitch('LG')).toBe('Legacy LiteGraph')
+      expect(testSwitch('Vue')).toBe('Vue Renderer')
+    })
+  })
+
+  describe('usage in graph serialization', () => {
+    it('should serialize and deserialize RendererType correctly', () => {
+      const extra: LGraphExtra = {
+        workflowRendererVersion: 'Vue'
+      }
+
+      const serialized = JSON.stringify(extra)
+      const deserialized: LGraphExtra = JSON.parse(serialized)
+
+      expect(deserialized.workflowRendererVersion).toBe('Vue')
+    })
+
+    it('should handle missing workflowRendererVersion in deserialization', () => {
+      const json = '{}'
+      const extra: LGraphExtra = JSON.parse(json)
+
+      expect(extra.workflowRendererVersion).toBeUndefined()
+    })
+
+    it('should handle null workflowRendererVersion', () => {
+      const extra: LGraphExtra = {
+        workflowRendererVersion: undefined
+      }
+
+      expect(extra.workflowRendererVersion).toBeUndefined()
+    })
+  })
+
+  describe('backward compatibility considerations', () => {
+    it('should work with both old and new renderer formats', () => {
+      // Even though the type is renamed, the values remain the same
+      const lgRenderer: RendererType = 'LG'
+      const vueRenderer: RendererType = 'Vue'
+
+      expect(lgRenderer).toBe('LG')
+      expect(vueRenderer).toBe('Vue')
+    })
+
+    it('should support migration scenarios', () => {
+      // Simulating reading from an old format
+      const oldFormat = { workflowRendererVersion: 'LG' as RendererType }
+      
+      // Converting to new format (type is already compatible)
+      const newFormat: LGraphExtra = {
+        workflowRendererVersion: oldFormat.workflowRendererVersion
+      }
+
+      expect(newFormat.workflowRendererVersion).toBe('LG')
+    })
+  })
+
+  describe('default values and initialization', () => {
+    it('should handle default initialization', () => {
+      const extra: LGraphExtra = {}
+      
+      // workflowRendererVersion should be undefined by default
+      expect(extra.workflowRendererVersion).toBeUndefined()
+    })
+
+    it('should support nullish coalescing', () => {
+      const extra: LGraphExtra = {}
+      const renderer = extra.workflowRendererVersion ?? 'LG'
+      
+      expect(renderer).toBe('LG')
+    })
+
+    it('should support optional chaining with default', () => {
+      let extra: LGraphExtra | undefined = undefined
+      const renderer = extra?.workflowRendererVersion ?? 'LG'
+      
+      expect(renderer).toBe('LG')
+    })
+  })
+})