[refactor] Move pure functions from layout store to separate modules so they can be tested (and add tests) (#5462)

* refactor layout store utils * [refactor] use nullish coalescing in getOr helper - addresses @DrJKL's suggestion Replaces manual undefined/null checks with more concise ?? operator for cleaner code that achieves the same functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [refactor] improve Y.Map typing for better type safety - addresses @DrJKL's typing suggestions - Use Y.Map<NodeLayout[keyof NodeLayout]> instead of Y.Map<unknown> - Provides compile-time type safety for stored values - Improves IntelliSense and prevents type mismatches - Updates mappers, store, tests, and helper functions consistently - No runtime changes, pure TypeScript improvement 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [refactor] address @arjansingh code quality feedback - Remove AI-generated refactoring comment that adds no value - Reorganize tests with nested describe blocks for better readability - Group related test cases by function for easier scanning 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [refactor] move makeLinkSegmentKey to layoutUtils - addresses @arjansingh's file organization feedback - Move string concatenation function from layoutMath.ts to new layoutUtils.ts - Keep layoutMath.ts focused on pure geometric calculations - Create dedicated layoutUtils.ts for general layout utilities - Update imports in store and create separate test file - Improves module cohesion and clarity of purpose 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [cleanup] remove leftover AI refactoring comments - Remove "Constants moved to utils" and "Node layout mapping moved to utils" - Clean up extra blank lines from previous refactoring - Keep meaningful organizational comments like "Helper methods" 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [cleanup] remove unnecessary import aliases Remove pointInBoundsUtil/boundsIntersectUtil aliases as there are no naming conflicts. Use direct function names for cleaner code. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [refactor] improve Y.Map typing with named NodeLayoutMap type - addresses @DrJKL's performance and type safety suggestions - Create named NodeLayoutMap type for TypeScript performance optimization - Improve getOr function with proper key constraints and type safety - Update all Y.Map<NodeLayout[keyof NodeLayout]> usages to use NodeLayoutMap - Remove manual type assertions in favor of generic key constraints - Clean up unused imports and fix formatting issues * [cleanup] remove explanatory comment per @DrJKL's preference * don't wait for dialog close button to be stable --------- Co-authored-by: Claude <noreply@anthropic.com>
2026-02-04 15:10:06 +00:00 · 2025-09-13 21:17:09 -07:00
parent 6f878abea4
commit 90bf8dc74a
8 changed files with 222 additions and 101 deletions
--- a/browser_tests/fixtures/ComfyPage.ts
+++ b/browser_tests/fixtures/ComfyPage.ts
@@ -1424,7 +1424,7 @@ export class ComfyPage {
  }

  async closeDialog() {
-    await this.page.locator('.p-dialog-close-button').click()
+    await this.page.locator('.p-dialog-close-button').click({ force: true })
    await expect(this.page.locator('.p-dialog')).toBeHidden()
  }

--- a/src/renderer/core/layout/store/layoutStore.ts
+++ b/src/renderer/core/layout/store/layoutStore.ts
@@ -36,9 +36,19 @@ import {
  type Point,
  type RerouteId,
  type RerouteLayout,
-  type Size,
  type SlotLayout
 } from '@/renderer/core/layout/types'
+import {
+  REROUTE_RADIUS,
+  boundsIntersect,
+  pointInBounds
+} from '@/renderer/core/layout/utils/layoutMath'
+import { makeLinkSegmentKey } from '@/renderer/core/layout/utils/layoutUtils'
+import {
+  type NodeLayoutMap,
+  layoutToYNode,
+  yNodeToLayout
+} from '@/renderer/core/layout/utils/mappers'
 import { SpatialIndexManager } from '@/renderer/core/spatial/SpatialIndex'

 type YEventChange = {
@@ -48,9 +58,6 @@ type YEventChange = {

 const logger = log.getLogger('LayoutStore')

-// Constants
-const REROUTE_RADIUS = 8
-
 // Utility functions
 function asRerouteId(id: string | number): RerouteId {
  return Number(id)
@@ -60,15 +67,6 @@ function asLinkId(id: string | number): LinkId {
  return Number(id)
 }

-interface NodeLayoutData {
-  id: NodeId
-  position: Point
-  size: Size
-  zIndex: number
-  visible: boolean
-  bounds: Bounds
-}
-
 interface LinkData {
  id: LinkId
  sourceNodeId: NodeId
@@ -91,15 +89,6 @@ interface TypedYMap<T> {
 }

 class LayoutStoreImpl implements LayoutStore {
-  private static readonly NODE_DEFAULTS: NodeLayoutData = {
-    id: 'unknown-node',
-    position: { x: 0, y: 0 },
-    size: { width: 100, height: 50 },
-    zIndex: 0,
-    visible: true,
-    bounds: { x: 0, y: 0, width: 100, height: 50 }
-  }
-
  private static readonly REROUTE_DEFAULTS: RerouteData = {
    id: 0,
    position: { x: 0, y: 0 },
@@ -109,7 +98,7 @@ class LayoutStoreImpl implements LayoutStore {

  // Yjs document and shared data structures
  private ydoc = new Y.Doc()
-  private ynodes: Y.Map<Y.Map<unknown>> // Maps nodeId -> Y.Map containing NodeLayout data
+  private ynodes: Y.Map<NodeLayoutMap> // Maps nodeId -> NodeLayoutMap containing NodeLayout data
  private ylinks: Y.Map<Y.Map<unknown>> // Maps linkId -> Y.Map containing link data
  private yreroutes: Y.Map<Y.Map<unknown>> // Maps rerouteId -> Y.Map containing reroute data
  private yoperations: Y.Array<LayoutOperation> // Operation log
@@ -155,7 +144,7 @@ class LayoutStoreImpl implements LayoutStore {
    this.rerouteSpatialIndex = new SpatialIndexManager()

    // Listen for Yjs changes and trigger Vue reactivity
-    this.ynodes.observe((event: Y.YMapEvent<Y.Map<unknown>>) => {
+    this.ynodes.observe((event: Y.YMapEvent<NodeLayoutMap>) => {
      this.version++

      // Trigger all affected node refs
@@ -184,16 +173,6 @@ class LayoutStoreImpl implements LayoutStore {
    })
  }

-  private getNodeField<K extends keyof NodeLayoutData>(
-    ynode: Y.Map<unknown>,
-    field: K,
-    defaultValue: NodeLayoutData[K] = LayoutStoreImpl.NODE_DEFAULTS[field]
-  ): NodeLayoutData[K] {
-    const typedNode = ynode as TypedYMap<NodeLayoutData>
-    const value = typedNode.get(field)
-    return value ?? defaultValue
-  }
-
  private getLinkField<K extends keyof LinkData>(
    ylink: Y.Map<unknown>,
    field: K
@@ -227,7 +206,7 @@ class LayoutStoreImpl implements LayoutStore {
          get: () => {
            track()
            const ynode = this.ynodes.get(nodeId)
-            const layout = ynode ? this.yNodeToLayout(ynode) : null
+            const layout = ynode ? yNodeToLayout(ynode) : null
            return layout
          },
          set: (newLayout: NodeLayout | null) => {
@@ -242,7 +221,7 @@ class LayoutStoreImpl implements LayoutStore {
                  timestamp: Date.now(),
                  source: this.currentSource,
                  actor: this.currentActor,
-                  previousLayout: this.yNodeToLayout(existing)
+                  previousLayout: yNodeToLayout(existing)
                })
              }
            } else {
@@ -260,7 +239,7 @@ class LayoutStoreImpl implements LayoutStore {
                  actor: this.currentActor
                })
              } else {
-                const existingLayout = this.yNodeToLayout(existing)
+                const existingLayout = yNodeToLayout(existing)

                // Check what properties changed
                if (
@@ -330,8 +309,8 @@ class LayoutStoreImpl implements LayoutStore {
      for (const [nodeId] of this.ynodes) {
        const ynode = this.ynodes.get(nodeId)
        if (ynode) {
-          const layout = this.yNodeToLayout(ynode)
-          if (layout && this.boundsIntersect(layout.bounds, bounds)) {
+          const layout = yNodeToLayout(ynode)
+          if (layout && boundsIntersect(layout.bounds, bounds)) {
            result.push(nodeId)
          }
        }
@@ -352,7 +331,7 @@ class LayoutStoreImpl implements LayoutStore {
      for (const [nodeId] of this.ynodes) {
        const ynode = this.ynodes.get(nodeId)
        if (ynode) {
-          const layout = this.yNodeToLayout(ynode)
+          const layout = yNodeToLayout(ynode)
          if (layout) {
            result.set(nodeId, layout)
          }
@@ -378,7 +357,7 @@ class LayoutStoreImpl implements LayoutStore {
    for (const [nodeId] of this.ynodes) {
      const ynode = this.ynodes.get(nodeId)
      if (ynode) {
-        const layout = this.yNodeToLayout(ynode)
+        const layout = yNodeToLayout(ynode)
        if (layout) {
          nodes.push([nodeId, layout])
        }
@@ -389,7 +368,7 @@ class LayoutStoreImpl implements LayoutStore {
    nodes.sort(([, a], [, b]) => b.zIndex - a.zIndex)

    for (const [nodeId, layout] of nodes) {
-      if (this.pointInBounds(point, layout.bounds)) {
+      if (pointInBounds(point, layout.bounds)) {
        return nodeId
      }
    }
@@ -561,16 +540,6 @@ class LayoutStoreImpl implements LayoutStore {
    return this.rerouteLayouts.get(rerouteId) || null
  }

-  /**
-   * Helper to create internal key for link segment
-   */
-  private makeLinkSegmentKey(
-    linkId: LinkId,
-    rerouteId: RerouteId | null
-  ): string {
-    return `${linkId}:${rerouteId ?? 'final'}`
-  }
-
  /**
   * Update link segment layout data
   */
@@ -579,7 +548,7 @@ class LayoutStoreImpl implements LayoutStore {
    rerouteId: RerouteId | null,
    layout: Omit<LinkSegmentLayout, 'linkId' | 'rerouteId'>
  ): void {
-    const key = this.makeLinkSegmentKey(linkId, rerouteId)
+    const key = makeLinkSegmentKey(linkId, rerouteId)
    const existing = this.linkSegmentLayouts.get(key)

    // Short-circuit if bounds and centerPos unchanged (prevents spatial index churn)
@@ -629,7 +598,7 @@ class LayoutStoreImpl implements LayoutStore {
   * Delete link segment layout data
   */
  deleteLinkSegmentLayout(linkId: LinkId, rerouteId: RerouteId | null): void {
-    const key = this.makeLinkSegmentKey(linkId, rerouteId)
+    const key = makeLinkSegmentKey(linkId, rerouteId)
    const deleted = this.linkSegmentLayouts.delete(key)
    if (deleted) {
      // Remove from spatial index
@@ -693,7 +662,7 @@ class LayoutStoreImpl implements LayoutStore {
            rerouteId: segmentLayout.rerouteId
          }
        }
-      } else if (this.pointInBounds(point, segmentLayout.bounds)) {
+      } else if (pointInBounds(point, segmentLayout.bounds)) {
        // Fallback to bounding box test
        return {
          linkId: segmentLayout.linkId,
@@ -733,7 +702,7 @@ class LayoutStoreImpl implements LayoutStore {
    // Check precise bounds for candidates
    for (const key of candidateSlotKeys) {
      const slotLayout = this.slotLayouts.get(key)
-      if (slotLayout && this.pointInBounds(point, slotLayout.bounds)) {
+      if (slotLayout && pointInBounds(point, slotLayout.bounds)) {
        return slotLayout
      }
    }
@@ -969,7 +938,7 @@ class LayoutStoreImpl implements LayoutStore {
          }
        }

-        this.ynodes.set(layout.id, this.layoutToYNode(layout))
+        this.ynodes.set(layout.id, layoutToYNode(layout))

        // Add to spatial index
        this.spatialIndex.insert(layout.id, layout.bounds)
@@ -987,7 +956,7 @@ class LayoutStoreImpl implements LayoutStore {
      return
    }

-    const size = this.getNodeField(ynode, 'size')
+    const size = yNodeToLayout(ynode).size
    const newBounds = {
      x: operation.position.x,
      y: operation.position.y,
@@ -1016,7 +985,7 @@ class LayoutStoreImpl implements LayoutStore {
    const ynode = this.ynodes.get(operation.nodeId)
    if (!ynode) return

-    const position = this.getNodeField(ynode, 'position')
+    const position = yNodeToLayout(ynode).position
    const newBounds = {
      x: position.x,
      y: position.y,
@@ -1053,7 +1022,7 @@ class LayoutStoreImpl implements LayoutStore {
    operation: CreateNodeOperation,
    change: LayoutChange
  ): void {
-    const ynode = this.layoutToYNode(operation.layout)
+    const ynode = layoutToYNode(operation.layout)
    this.ynodes.set(operation.nodeId, ynode)

    // Add to spatial index
@@ -1187,7 +1156,7 @@ class LayoutStoreImpl implements LayoutStore {
   * Update node bounds helper
   */
  private updateNodeBounds(
-    ynode: Y.Map<unknown>,
+    ynode: NodeLayoutMap,
    position: Point,
    size: { width: number; height: number }
  ): void {
@@ -1335,27 +1304,6 @@ class LayoutStoreImpl implements LayoutStore {
  }

  // Helper methods
-  private layoutToYNode(layout: NodeLayout): Y.Map<unknown> {
-    const ynode = new Y.Map<unknown>()
-    ynode.set('id', layout.id)
-    ynode.set('position', layout.position)
-    ynode.set('size', layout.size)
-    ynode.set('zIndex', layout.zIndex)
-    ynode.set('visible', layout.visible)
-    ynode.set('bounds', layout.bounds)
-    return ynode
-  }
-
-  private yNodeToLayout(ynode: Y.Map<unknown>): NodeLayout {
-    return {
-      id: this.getNodeField(ynode, 'id'),
-      position: this.getNodeField(ynode, 'position'),
-      size: this.getNodeField(ynode, 'size'),
-      zIndex: this.getNodeField(ynode, 'zIndex'),
-      visible: this.getNodeField(ynode, 'visible'),
-      bounds: this.getNodeField(ynode, 'bounds')
-    }
-  }

  private notifyChange(change: LayoutChange): void {
    this.changeListeners.forEach((listener) => {
@@ -1367,24 +1315,6 @@ class LayoutStoreImpl implements LayoutStore {
    })
  }

-  private pointInBounds(point: Point, bounds: Bounds): boolean {
-    return (
-      point.x >= bounds.x &&
-      point.x <= bounds.x + bounds.width &&
-      point.y >= bounds.y &&
-      point.y <= bounds.y + bounds.height
-    )
-  }
-
-  private boundsIntersect(a: Bounds, b: Bounds): boolean {
-    return !(
-      a.x + a.width < b.x ||
-      b.x + b.width < a.x ||
-      a.y + a.height < b.y ||
-      b.y + b.height < a.y
-    )
-  }
-
  // CRDT-specific methods
  getOperationsSince(timestamp: number): LayoutOperation[] {
    const operations: LayoutOperation[] = []
--- a/src/renderer/core/layout/utils/layoutMath.ts
+++ b/src/renderer/core/layout/utils/layoutMath.ts
@@ -0,0 +1,21 @@
+import type { Bounds, Point } from '@/renderer/core/layout/types'
+
+export const REROUTE_RADIUS = 8
+
+export function pointInBounds(point: Point, bounds: Bounds): boolean {
+  return (
+    point.x >= bounds.x &&
+    point.x <= bounds.x + bounds.width &&
+    point.y >= bounds.y &&
+    point.y <= bounds.y + bounds.height
+  )
+}
+
+export function boundsIntersect(a: Bounds, b: Bounds): boolean {
+  return !(
+    a.x + a.width < b.x ||
+    b.x + b.width < a.x ||
+    a.y + a.height < b.y ||
+    b.y + b.height < a.y
+  )
+}
--- a/src/renderer/core/layout/utils/layoutUtils.ts
+++ b/src/renderer/core/layout/utils/layoutUtils.ts
@@ -0,0 +1,11 @@
+import type { LinkId, RerouteId } from '@/renderer/core/layout/types'
+
+/**
+ * Creates a unique key for identifying link segments in spatial indexes
+ */
+export function makeLinkSegmentKey(
+  linkId: LinkId,
+  rerouteId: RerouteId | null
+): string {
+  return `${linkId}:${rerouteId ?? 'final'}`
+}
--- a/src/renderer/core/layout/utils/mappers.ts
+++ b/src/renderer/core/layout/utils/mappers.ts
@@ -0,0 +1,45 @@
+import * as Y from 'yjs'
+
+import type { NodeLayout } from '@/renderer/core/layout/types'
+
+export type NodeLayoutMap = Y.Map<NodeLayout[keyof NodeLayout]>
+
+export const NODE_LAYOUT_DEFAULTS: NodeLayout = {
+  id: 'unknown-node',
+  position: { x: 0, y: 0 },
+  size: { width: 100, height: 50 },
+  zIndex: 0,
+  visible: true,
+  bounds: { x: 0, y: 0, width: 100, height: 50 }
+}
+
+export function layoutToYNode(layout: NodeLayout): NodeLayoutMap {
+  const ynode = new Y.Map<NodeLayout[keyof NodeLayout]>() as NodeLayoutMap
+  ynode.set('id', layout.id)
+  ynode.set('position', layout.position)
+  ynode.set('size', layout.size)
+  ynode.set('zIndex', layout.zIndex)
+  ynode.set('visible', layout.visible)
+  ynode.set('bounds', layout.bounds)
+  return ynode
+}
+
+function getOr<K extends keyof NodeLayout>(
+  map: NodeLayoutMap,
+  key: K,
+  fallback: NodeLayout[K]
+): NodeLayout[K] {
+  const v = map.get(key)
+  return (v ?? fallback) as NodeLayout[K]
+}
+
+export function yNodeToLayout(ynode: NodeLayoutMap): NodeLayout {
+  return {
+    id: getOr(ynode, 'id', NODE_LAYOUT_DEFAULTS.id),
+    position: getOr(ynode, 'position', NODE_LAYOUT_DEFAULTS.position),
+    size: getOr(ynode, 'size', NODE_LAYOUT_DEFAULTS.size),
+    zIndex: getOr(ynode, 'zIndex', NODE_LAYOUT_DEFAULTS.zIndex),
+    visible: getOr(ynode, 'visible', NODE_LAYOUT_DEFAULTS.visible),
+    bounds: getOr(ynode, 'bounds', NODE_LAYOUT_DEFAULTS.bounds)
+  }
+}
--- a/tests-ui/tests/renderer/core/layout/utils/layoutMath.test.ts
+++ b/tests-ui/tests/renderer/core/layout/utils/layoutMath.test.ts
@@ -0,0 +1,49 @@
+import { describe, expect, it } from 'vitest'
+
+import {
+  REROUTE_RADIUS,
+  boundsIntersect,
+  pointInBounds
+} from '@/renderer/core/layout/utils/layoutMath'
+
+describe('layoutMath utils', () => {
+  describe('pointInBounds', () => {
+    it('detects inclusion correctly', () => {
+      const bounds = { x: 10, y: 10, width: 100, height: 50 }
+      expect(pointInBounds({ x: 10, y: 10 }, bounds)).toBe(true)
+      expect(pointInBounds({ x: 110, y: 60 }, bounds)).toBe(true)
+      expect(pointInBounds({ x: 9, y: 10 }, bounds)).toBe(false)
+      expect(pointInBounds({ x: 111, y: 10 }, bounds)).toBe(false)
+      expect(pointInBounds({ x: 10, y: 61 }, bounds)).toBe(false)
+    })
+
+    it('works with zero-size bounds', () => {
+      const zero = { x: 10, y: 20, width: 0, height: 0 }
+      expect(pointInBounds({ x: 10, y: 20 }, zero)).toBe(true)
+      expect(pointInBounds({ x: 10, y: 21 }, zero)).toBe(false)
+      expect(pointInBounds({ x: 9, y: 20 }, zero)).toBe(false)
+    })
+  })
+
+  describe('boundsIntersect', () => {
+    it('detects intersection correctly', () => {
+      const a = { x: 0, y: 0, width: 10, height: 10 }
+      const b = { x: 5, y: 5, width: 10, height: 10 }
+      const c = { x: 11, y: 0, width: 5, height: 5 }
+      expect(boundsIntersect(a, b)).toBe(true)
+      expect(boundsIntersect(a, c)).toBe(false)
+    })
+
+    it('treats touching edges as intersecting', () => {
+      const a = { x: 0, y: 0, width: 10, height: 10 }
+      const d = { x: 10, y: 0, width: 5, height: 5 } // touches at right edge
+      expect(boundsIntersect(a, d)).toBe(true)
+    })
+  })
+
+  describe('REROUTE_RADIUS', () => {
+    it('exports a sensible reroute radius', () => {
+      expect(REROUTE_RADIUS).toBeGreaterThan(0)
+    })
+  })
+})
--- a/tests-ui/tests/renderer/core/layout/utils/layoutUtils.test.ts
+++ b/tests-ui/tests/renderer/core/layout/utils/layoutUtils.test.ts
@@ -0,0 +1,18 @@
+import { describe, expect, it } from 'vitest'
+
+import { makeLinkSegmentKey } from '@/renderer/core/layout/utils/layoutUtils'
+
+describe('layoutUtils', () => {
+  describe('makeLinkSegmentKey', () => {
+    it('creates stable keys for null reroute', () => {
+      expect(makeLinkSegmentKey(10, null)).toBe('10:final')
+      expect(makeLinkSegmentKey(42, null)).toBe('42:final')
+    })
+
+    it('creates stable keys for numeric reroute ids', () => {
+      expect(makeLinkSegmentKey(10, 3)).toBe('10:3')
+      expect(makeLinkSegmentKey(42, 0)).toBe('42:0')
+      expect(makeLinkSegmentKey(42, 7)).toBe('42:7')
+    })
+  })
+})
--- a/tests-ui/tests/renderer/core/layout/utils/mappers.test.ts
+++ b/tests-ui/tests/renderer/core/layout/utils/mappers.test.ts
@@ -0,0 +1,47 @@
+import { describe, expect, it } from 'vitest'
+import * as Y from 'yjs'
+
+import {
+  NODE_LAYOUT_DEFAULTS,
+  type NodeLayoutMap,
+  yNodeToLayout
+} from '@/renderer/core/layout/utils/mappers'
+
+describe('mappers', () => {
+  it('yNodeToLayout reads from Yjs-attached map', () => {
+    const layout = {
+      id: 'node-1',
+      position: { x: 12, y: 34 },
+      size: { width: 111, height: 222 },
+      zIndex: 5,
+      visible: true,
+      bounds: { x: 12, y: 34, width: 111, height: 222 }
+    }
+
+    const doc = new Y.Doc()
+    const ynode = doc.getMap('node') as NodeLayoutMap
+    ynode.set('id', layout.id)
+    ynode.set('position', layout.position)
+    ynode.set('size', layout.size)
+    ynode.set('zIndex', layout.zIndex)
+    ynode.set('visible', layout.visible)
+    ynode.set('bounds', layout.bounds)
+
+    const back = yNodeToLayout(ynode)
+    expect(back).toEqual(layout)
+  })
+
+  it('yNodeToLayout applies defaults for missing fields', () => {
+    const doc = new Y.Doc()
+    const ynode = doc.getMap('node') as NodeLayoutMap
+    // Don't set any fields - they should all use defaults
+
+    const back = yNodeToLayout(ynode)
+    expect(back.id).toBe(NODE_LAYOUT_DEFAULTS.id)
+    expect(back.position).toEqual(NODE_LAYOUT_DEFAULTS.position)
+    expect(back.size).toEqual(NODE_LAYOUT_DEFAULTS.size)
+    expect(back.zIndex).toEqual(NODE_LAYOUT_DEFAULTS.zIndex)
+    expect(back.visible).toEqual(NODE_LAYOUT_DEFAULTS.visible)
+    expect(back.bounds).toEqual(NODE_LAYOUT_DEFAULTS.bounds)
+  })
+})