Address PR feedback: refactor browser tests for better type safety

browser_tests/fixtures/utils/litegraphUtils.ts

@@ -159,19 +159,6 @@ class NodeSlotReference {
         const rawPos = node.getConnectionPos(type === 'input', index)
         const convertedPos = app.canvas.ds.convertOffsetToCanvas(rawPos)
 
-        // Debug logging - convert Float64Arrays to regular arrays for visibility
-
-        console.log(
-          `NodeSlotReference debug for ${type} slot ${index} on node ${id}:`,
-          {
-            nodePos: [node.pos[0], node.pos[1]],
-            nodeSize: [node.size[0], node.size[1]],
-            rawConnectionPos: [rawPos[0], rawPos[1]],
-            convertedPos: [convertedPos[0], convertedPos[1]],
-            currentGraphType: graph.constructor.name
-          }
-        )
-
         return convertedPos
       },
       [this.type, this.node.id, this.index] as const
@@ -256,8 +243,8 @@ class NodeWidgetReference {
     const pos: [number, number] = await this.node.comfyPage.page.evaluate(
       ([id, index]) => {
         const app = window.app
-        if (!app?.graph) throw new Error('App not initialized')
-        const node = app.graph.getNodeById(id)
+        if (!app?.canvas?.graph) throw new Error('App not initialized')
+        const node = app.canvas.graph.getNodeById(id)
         if (!node) throw new Error(`Node ${id} not found.`)
         if (!node.widgets) throw new Error(`Node ${id} has no widgets.`)
         const widget = node.widgets[index]
@@ -310,8 +297,8 @@ class NodeWidgetReference {
     return await this.node.comfyPage.page.evaluate(
       ([id, index]) => {
         const app = window.app
-        if (!app?.graph) throw new Error('App not initialized')
-        const node = app.graph.getNodeById(id)
+        if (!app?.canvas?.graph) throw new Error('App not initialized')
+        const node = app.canvas.graph.getNodeById(id)
         if (!node) throw new Error(`Node ${id} not found.`)
         if (!node.widgets) throw new Error(`Node ${id} has no widgets.`)
         const widget = node.widgets[index]

browser_tests/fixtures/utils/subgraphUtils.ts

@@ -10,6 +10,15 @@ export interface SubgraphSlot {
   label?: string
   name?: string
   displayName?: string
+  labelPos?: [number, number]
+}
+
+export interface SubgraphInputNode {
+  onPointerDown?: (e: unknown, pointer: unknown, linkConnector: unknown) => void
+}
+
+export interface SubgraphGraphWithNodes extends SubgraphGraph {
+  inputNode?: SubgraphInputNode
 }
 
 /**

browser_tests/fixtures/utils/taskHistory.ts

@@ -120,23 +120,15 @@ export default class TaskHistory {
     filenames: string[],
     filetype: OutputFileType
   ): TaskOutput {
-    return filenames.reduce(
-      (outputs, filename, i) => {
-        const nodeId = `${i + 1}`
-        outputs[nodeId] = {
-          [filetype]: [{ filename, subfolder: '', type: 'output' }]
-        }
-        const contentType = getContentType(filename, filetype)
-        this.outputContentTypes.set(filename, contentType)
-        return outputs
-      },
-      {} as Record<
-        string,
-        {
-          [key: string]: { filename: string; subfolder: string; type: string }[]
-        }
-      >
-    )
+    return filenames.reduce<TaskOutput>((outputs, filename, i) => {
+      const nodeId = `${i + 1}`
+      outputs[nodeId] = {
+        [filetype]: [{ filename, subfolder: '', type: 'output' }]
+      }
+      const contentType = getContentType(filename, filetype)
+      this.outputContentTypes.set(filename, contentType)
+      return outputs
+    }, {})
   }
 
   private addTask(task: HistoryTaskItem) {

browser_tests/fixtures/utils/workflowUtils.ts

@@ -0,0 +1,22 @@
+interface ExtensionManagerWorkflow {
+  workflow?: {
+    activeWorkflow?: {
+      filename?: string
+      delete?: () => Promise<void>
+    }
+  }
+}
+
+interface AppWithExtensionManager {
+  extensionManager: ExtensionManagerWorkflow
+}
+
+export function getActiveWorkflowFilename(app: unknown): string | undefined {
+  const extMgr = (app as AppWithExtensionManager).extensionManager
+  return extMgr.workflow?.activeWorkflow?.filename
+}
+
+export async function deleteActiveWorkflow(app: unknown): Promise<void> {
+  const extMgr = (app as AppWithExtensionManager).extensionManager
+  await extMgr.workflow?.activeWorkflow?.delete?.()
+}

browser_tests/helpers/actionbar.ts

@@ -47,9 +47,10 @@ class ComfyQueueButtonOptions {
       const extMgr = app.extensionManager as {
         queueSettings?: { mode: string }
       }
-      if (extMgr.queueSettings) {
-        extMgr.queueSettings.mode = mode
+      if (!extMgr.queueSettings) {
+        throw new Error('queueSettings not initialized')
       }
+      extMgr.queueSettings.mode = mode
     }, mode)
   }
 

browser_tests/tests/browserTabTitle.spec.ts

@@ -1,6 +1,10 @@
 import { expect } from '@playwright/test'
 
 import { comfyPageFixture as test } from '../fixtures/ComfyPage'
+import {
+  deleteActiveWorkflow,
+  getActiveWorkflowFilename
+} from '../fixtures/utils/workflowUtils'
 
 test.describe('Browser tab title', () => {
   test.describe('Beta Menu', () => {
@@ -9,14 +13,11 @@ test.describe('Browser tab title', () => {
     })
 
     test('Can display workflow name', async ({ comfyPage }) => {
-      const workflowName = await comfyPage.page.evaluate(async () => {
+      const workflowName = await comfyPage.page.evaluate(() => {
         const app = window['app']
         if (!app) throw new Error('App not initialized')
-        const extMgr = app.extensionManager as {
-          workflow?: { activeWorkflow?: { filename?: string } }
-        }
-        return extMgr.workflow?.activeWorkflow?.filename
-      })
+        return getActiveWorkflowFilename(app)
+      }, getActiveWorkflowFilename)
       expect(await comfyPage.page.title()).toBe(`*${workflowName} - ComfyUI`)
     })
 
@@ -25,14 +26,11 @@ test.describe('Browser tab title', () => {
     test.skip('Can display workflow name with unsaved changes', async ({
       comfyPage
     }) => {
-      const workflowName = await comfyPage.page.evaluate(async () => {
+      const workflowName = await comfyPage.page.evaluate(() => {
         const app = window['app']
         if (!app) throw new Error('App not initialized')
-        const extMgr = app.extensionManager as {
-          workflow?: { activeWorkflow?: { filename?: string } }
-        }
-        return extMgr.workflow?.activeWorkflow?.filename
-      })
+        return getActiveWorkflowFilename(app)
+      }, getActiveWorkflowFilename)
       expect(await comfyPage.page.title()).toBe(`${workflowName} - ComfyUI`)
 
       await comfyPage.menu.topbar.saveWorkflow('test')
@@ -47,11 +45,8 @@ test.describe('Browser tab title', () => {
       await comfyPage.page.evaluate(async () => {
         const app = window['app']
         if (!app) throw new Error('App not initialized')
-        const extMgr = app.extensionManager as {
-          workflow?: { activeWorkflow?: { delete?: () => Promise<void> } }
-        }
-        return extMgr.workflow?.activeWorkflow?.delete?.()
-      })
+        await deleteActiveWorkflow(app)
+      }, deleteActiveWorkflow)
     })
   })
 

browser_tests/tests/colorPalette.spec.ts

@@ -189,7 +189,15 @@ test.describe('Color Palette', () => {
       const extMgr = app.extensionManager as {
         colorPalette?: { addCustomColorPalette?: (p: unknown) => void }
       }
-      extMgr.colorPalette?.addCustomColorPalette?.(p)
+      if (!extMgr.colorPalette) {
+        throw new Error('colorPalette extension not found on extensionManager')
+      }
+      if (!extMgr.colorPalette.addCustomColorPalette) {
+        throw new Error(
+          'addCustomColorPalette method not found on colorPalette extension'
+        )
+      }
+      extMgr.colorPalette.addCustomColorPalette(p)
     }, customColorPalettes.obsidian_dark)
     expect(await comfyPage.getToastErrorCount()).toBe(0)
 

browser_tests/tests/groupNode.spec.ts

@@ -23,9 +23,7 @@ test.describe('Group Node', () => {
       await libraryTab.open()
     })
 
-    test('Is added to node library sidebar', async ({
-      comfyPage: _comfyPage
-    }) => {
+    test('Is added to node library sidebar', async () => {
       expect(await libraryTab.getFolder('group nodes').count()).toBe(1)
     })
 

browser_tests/tests/sidebar/nodeLibrary.spec.ts

@@ -268,10 +268,11 @@ test.describe('Node library sidebar', () => {
     const setting = (await comfyPage.getSetting(
       'Comfy.NodeLibrary.BookmarksCustomization'
     )) as Record<string, { icon?: string; color?: string }> | undefined
-    await expect(setting).toHaveProperty(['foo/', 'color'])
-    await expect(setting?.['foo/'].color).not.toBeNull()
-    await expect(setting?.['foo/'].color).not.toBeUndefined()
-    await expect(setting?.['foo/'].color).not.toBe('')
+    expect(setting).toHaveProperty(['foo/', 'color'])
+    expect(setting?.['foo/']).toBeDefined()
+    expect(setting?.['foo/']?.color).not.toBeNull()
+    expect(setting?.['foo/']?.color).not.toBeUndefined()
+    expect(setting?.['foo/']?.color).not.toBe('')
   })
 
   test('Can rename customized bookmark folder', async ({ comfyPage }) => {

browser_tests/tests/sidebar/workflows.spec.ts

@@ -182,7 +182,9 @@ test.describe('Workflows sidebar', () => {
     // Compare the exported workflow with the original
     expect(downloadedContent).toBeDefined()
     expect(downloadedContentZh).toBeDefined()
-    if (!downloadedContent || !downloadedContentZh) return
+    if (!downloadedContent || !downloadedContentZh) {
+      throw new Error('Downloaded workflow content is undefined')
+    }
     delete downloadedContent.id
     delete downloadedContentZh.id
     expect(downloadedContent).toEqual(downloadedContentZh)

browser_tests/tests/subgraph-rename-dialog.spec.ts

@@ -11,6 +11,39 @@ const SELECTORS = {
   promptDialog: '.graphdialog input'
 } as const
 
+interface SubgraphSlot {
+  label?: string
+  name?: string
+  displayName?: string
+}
+
+interface SubgraphGraph {
+  inputs: SubgraphSlot[]
+  outputs: SubgraphSlot[]
+}
+
+function getSubgraph(): SubgraphGraph {
+  const assertSubgraph = (graph: unknown): asserts graph is SubgraphGraph => {
+    if (
+      graph === null ||
+      typeof graph !== 'object' ||
+      !('inputs' in graph) ||
+      !('outputs' in graph) ||
+      !Array.isArray((graph as { inputs: unknown }).inputs) ||
+      !Array.isArray((graph as { outputs: unknown }).outputs)
+    ) {
+      throw new Error('Not in subgraph')
+    }
+  }
+  const app = window['app']
+  if (!app) throw new Error('App not available')
+  const canvas = app.canvas
+  if (!canvas) throw new Error('Canvas not available')
+  const graph = canvas.graph
+  assertSubgraph(graph)
+  return graph
+}
+
 test.describe('Subgraph Slot Rename Dialog', () => {
   test.beforeEach(async ({ comfyPage }) => {
     await comfyPage.setSetting('Comfy.UseNewMenu', 'Disabled')
@@ -25,32 +58,10 @@ test.describe('Subgraph Slot Rename Dialog', () => {
     await subgraphNode.navigateIntoSubgraph()
 
     // Get initial slot label
-    const initialInputLabel = await comfyPage.page.evaluate(() => {
-      const assertSubgraph = (
-        graph: unknown
-      ): asserts graph is {
-        inputs: { label?: string; name?: string }[]
-        outputs: unknown[]
-      } => {
-        if (
-          graph === null ||
-          typeof graph !== 'object' ||
-          !('inputs' in graph) ||
-          !('outputs' in graph) ||
-          !Array.isArray((graph as { inputs: unknown }).inputs) ||
-          !Array.isArray((graph as { outputs: unknown }).outputs)
-        ) {
-          throw new Error('Not in subgraph')
-        }
-      }
-      const app = window['app']
-      if (!app) throw new Error('App not available')
-      const canvas = app.canvas
-      if (!canvas) throw new Error('Canvas not available')
-      const graph = canvas.graph
-      assertSubgraph(graph)
+    const initialInputLabel = await comfyPage.page.evaluate((getSubgraph) => {
+      const graph = getSubgraph()
       return graph.inputs[0]?.label || graph.inputs[0]?.name || null
-    })
+    }, getSubgraph)
 
     // First rename
     await comfyPage.rightClickSubgraphInputSlot(initialInputLabel ?? undefined)
@@ -75,37 +86,15 @@ test.describe('Subgraph Slot Rename Dialog', () => {
     await comfyPage.nextFrame()
 
     // Verify the rename worked
-    const afterFirstRename = await comfyPage.page.evaluate(() => {
-      const assertSubgraph = (
-        graph: unknown
-      ): asserts graph is {
-        inputs: { label?: string; name?: string; displayName?: string }[]
-        outputs: unknown[]
-      } => {
-        if (
-          graph === null ||
-          typeof graph !== 'object' ||
-          !('inputs' in graph) ||
-          !('outputs' in graph) ||
-          !Array.isArray((graph as { inputs: unknown }).inputs) ||
-          !Array.isArray((graph as { outputs: unknown }).outputs)
-        ) {
-          throw new Error('Not in subgraph')
-        }
-      }
-      const app = window['app']
-      if (!app) throw new Error('App not available')
-      const canvas = app.canvas
-      if (!canvas) throw new Error('Canvas not available')
-      const graph = canvas.graph
-      assertSubgraph(graph)
+    const afterFirstRename = await comfyPage.page.evaluate((getSubgraph) => {
+      const graph = getSubgraph()
       const slot = graph.inputs[0]
       return {
         label: slot?.label || null,
         name: slot?.name || null,
         displayName: slot?.displayName || slot?.label || slot?.name || null
       }
-    })
+    }, getSubgraph)
     expect(afterFirstRename.label).toBe(RENAMED_NAME)
 
     // Now rename again - this is where the bug would show
@@ -139,32 +128,10 @@ test.describe('Subgraph Slot Rename Dialog', () => {
     await comfyPage.nextFrame()
 
     // Verify the second rename worked
-    const afterSecondRename = await comfyPage.page.evaluate(() => {
-      const assertSubgraph = (
-        graph: unknown
-      ): asserts graph is {
-        inputs: { label?: string }[]
-        outputs: unknown[]
-      } => {
-        if (
-          graph === null ||
-          typeof graph !== 'object' ||
-          !('inputs' in graph) ||
-          !('outputs' in graph) ||
-          !Array.isArray((graph as { inputs: unknown }).inputs) ||
-          !Array.isArray((graph as { outputs: unknown }).outputs)
-        ) {
-          throw new Error('Not in subgraph')
-        }
-      }
-      const app = window['app']
-      if (!app) throw new Error('App not available')
-      const canvas = app.canvas
-      if (!canvas) throw new Error('Canvas not available')
-      const graph = canvas.graph
-      assertSubgraph(graph)
+    const afterSecondRename = await comfyPage.page.evaluate((getSubgraph) => {
+      const graph = getSubgraph()
       return graph.inputs[0]?.label || null
-    })
+    }, getSubgraph)
     expect(afterSecondRename).toBe(SECOND_RENAMED_NAME)
   })
 
@@ -177,32 +144,10 @@ test.describe('Subgraph Slot Rename Dialog', () => {
     await subgraphNode.navigateIntoSubgraph()
 
     // Get initial output slot label
-    const initialOutputLabel = await comfyPage.page.evaluate(() => {
-      const assertSubgraph = (
-        graph: unknown
-      ): asserts graph is {
-        inputs: unknown[]
-        outputs: { label?: string; name?: string }[]
-      } => {
-        if (
-          graph === null ||
-          typeof graph !== 'object' ||
-          !('inputs' in graph) ||
-          !('outputs' in graph) ||
-          !Array.isArray((graph as { inputs: unknown }).inputs) ||
-          !Array.isArray((graph as { outputs: unknown }).outputs)
-        ) {
-          throw new Error('Not in subgraph')
-        }
-      }
-      const app = window['app']
-      if (!app) throw new Error('App not available')
-      const canvas = app.canvas
-      if (!canvas) throw new Error('Canvas not available')
-      const graph = canvas.graph
-      assertSubgraph(graph)
+    const initialOutputLabel = await comfyPage.page.evaluate((getSubgraph) => {
+      const graph = getSubgraph()
       return graph.outputs[0]?.label || graph.outputs[0]?.name || null
-    })
+    }, getSubgraph)
 
     // First rename
     await comfyPage.rightClickSubgraphOutputSlot(

browser_tests/tests/subgraph.spec.ts

@@ -1,6 +1,7 @@
 import { expect } from '@playwright/test'
 
 import { comfyPageFixture as test } from '../fixtures/ComfyPage'
+import type { SubgraphGraphWithNodes } from '../fixtures/utils/subgraphUtils'
 
 // Constants
 const RENAMED_INPUT_NAME = 'renamed_input'
@@ -322,16 +323,7 @@ test.describe('Subgraph Operations', () => {
       await comfyPage.page.evaluate(() => {
         const app = window['app']
         if (!app) throw new Error('App not initialized')
-        const graph = app.canvas.graph as {
-          inputs?: { label?: string; labelPos?: [number, number] }[]
-          inputNode?: {
-            onPointerDown?: (
-              e: unknown,
-              pointer: unknown,
-              linkConnector: unknown
-            ) => void
-          }
-        } | null
+        const graph = app.canvas.graph as SubgraphGraphWithNodes | null
         if (!graph || !('inputs' in graph)) {
           throw new Error('Not in a subgraph')
         }

browser_tests/tests/widget.spec.ts

@@ -109,7 +109,9 @@ test.describe('Slider widget', () => {
 
     await comfyPage.page.evaluate(() => {
       const app = window['app']
-      if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return
+      if (!app?.graph?.nodes?.[0]?.widgets?.[0]) {
+        throw new Error('widget not found')
+      }
       const widget = app.graph.nodes[0].widgets[0]
       widget.callback = (value: number) => {
         window.widgetValue = value

docs/typescript/type-safety.md

@@ -203,9 +203,9 @@ const config: Config = {
 
 ## Prefer Interfaces Over Type Aliases for Objects
 
-Use interfaces for object types - they have better error messages and performance:
+Use interfaces for object types. While modern TypeScript has narrowed performance differences between interfaces and type aliases, interfaces still offer clearer error messages and support declaration merging:
 
-❌ **Wrong:**
+❌ **Avoid for object types:**
 
 ```typescript
 type NodeConfig = {
@@ -214,7 +214,7 @@ type NodeConfig = {
 }
 ```
 
-✅ **Correct:**
+✅ **Preferred:**
 
 ```typescript
 interface NodeConfig {
@@ -385,7 +385,7 @@ declare function fn<T>(arg: T): T
 
 ## Summary
 
-1. **Never use `as`** outside of custom type guards
+1. **Never use `as`** outside of custom type guards (exception: test files may use `as Partial<T> as T` for partial mocks)
 2. **Use `instanceof`** for class/DOM element checks
 3. **Use `'prop' in obj`** for property existence checks
 4. **Use `typeof`** for primitive type checks