From 5ea7a56b64c56b1f060174573f7625e2dbce54d3 Mon Sep 17 00:00:00 2001 From: Austin Mroz Date: Tue, 23 Sep 2025 16:45:46 -0500 Subject: [PATCH] Stricter typing for nodeids --- browser_tests/fixtures/VueNodeHelpers.ts | 8 ++++--- browser_tests/tests/chatHistory.spec.ts | 4 +++- .../management/stores/workflowStore.ts | 22 +++++++++---------- src/services/litegraphService.ts | 14 +++++++----- src/stores/executionStore.ts | 8 ++++--- src/types/nodeIdentification.ts | 2 +- tests-ui/tests/store/executionStore.test.ts | 3 +-- .../tests/utils/graphTraversalUtil.test.ts | 3 ++- 8 files changed, 35 insertions(+), 29 deletions(-) diff --git a/browser_tests/fixtures/VueNodeHelpers.ts b/browser_tests/fixtures/VueNodeHelpers.ts index e3b3de542..e2f210a29 100644 --- a/browser_tests/fixtures/VueNodeHelpers.ts +++ b/browser_tests/fixtures/VueNodeHelpers.ts @@ -3,6 +3,8 @@ */ import type { Locator, Page } from '@playwright/test' +import type { NodeId } from '@/lib/litegraph/src/LGraphNode' + export class VueNodeHelpers { constructor(private page: Page) {} @@ -39,7 +41,7 @@ export class VueNodeHelpers { /** * Get all Vue node IDs currently in the DOM */ - async getNodeIds(): Promise { + async getNodeIds(): Promise { return await this.nodes.evaluateAll((nodes) => nodes .map((n) => n.getAttribute('data-node-id')) @@ -50,14 +52,14 @@ export class VueNodeHelpers { /** * Select a specific Vue node by ID */ - async selectNode(nodeId: string): Promise { + async selectNode(nodeId: nodeId): Promise { await this.page.locator(`[data-node-id="${nodeId}"]`).click() } /** * Select multiple Vue nodes by IDs using Ctrl+click */ - async selectNodes(nodeIds: string[]): Promise { + async selectNodes(nodeIds: NodeId[]): Promise { if (nodeIds.length === 0) return // Select first node normally diff --git a/browser_tests/tests/chatHistory.spec.ts b/browser_tests/tests/chatHistory.spec.ts index 7d1bf6c10..8f08055d0 100644 --- a/browser_tests/tests/chatHistory.spec.ts +++ b/browser_tests/tests/chatHistory.spec.ts @@ -1,6 +1,8 @@ import type { Page } from '@playwright/test' import { expect } from '@playwright/test' +import type { NodeId } from '@/lib/litegraph/src/LGraphNode' + import { comfyPageFixture as test } from '../fixtures/ComfyPage' interface ChatHistoryEntry { @@ -33,7 +35,7 @@ async function renderChatHistory(page: Page, history: ChatHistoryEntry[]) { } test.describe('Chat History Widget', () => { - let nodeId: string + let nodeId: NodeId test.beforeEach(async ({ comfyPage }) => { nodeId = await renderChatHistory(comfyPage.page, [ diff --git a/src/platform/workflow/management/stores/workflowStore.ts b/src/platform/workflow/management/stores/workflowStore.ts index 8ed6d90a5..16c05df6c 100644 --- a/src/platform/workflow/management/stores/workflowStore.ts +++ b/src/platform/workflow/management/stores/workflowStore.ts @@ -182,14 +182,14 @@ interface WorkflowStore { activeSubgraph: Subgraph | undefined /** Updates the {@link subgraphNamePath} and {@link isSubgraphActive} values. */ updateActiveGraph: () => void - executionIdToCurrentId: (id: string) => any + executionIdToCurrentId: (id: NodeExecutionId) => any nodeIdToNodeLocatorId: (nodeId: NodeId, subgraph?: Subgraph) => NodeLocatorId nodeExecutionIdToNodeLocatorId: ( - nodeExecutionId: NodeExecutionId | string + nodeExecutionId: NodeExecutionId ) => NodeLocatorId | null - nodeLocatorIdToNodeId: (locatorId: NodeLocatorId | string) => NodeId | null + nodeLocatorIdToNodeId: (locatorId: NodeLocatorId) => NodeId | null nodeLocatorIdToNodeExecutionId: ( - locatorId: NodeLocatorId | string, + locatorId: NodeLocatorId, targetSubgraph?: Subgraph ) => NodeExecutionId | null } @@ -518,14 +518,14 @@ export const useWorkflowStore = defineStore('workflow', () => { isSubgraphActive.value = isSubgraph(subgraph) } - const subgraphNodeIdToSubgraph = (id: string, graph: LGraph | Subgraph) => { + const subgraphNodeIdToSubgraph = (id: NodeId, graph: LGraph | Subgraph) => { const node = graph.getNodeById(id) if (node?.isSubgraphNode()) return node.subgraph } const getSubgraphsFromInstanceIds = ( currentGraph: LGraph | Subgraph, - subgraphNodeIds: string[], + subgraphNodeIds: NodeId[], subgraphs: Subgraph[] = [] ): Subgraph[] => { const currentPart = subgraphNodeIds.shift() @@ -538,7 +538,7 @@ export const useWorkflowStore = defineStore('workflow', () => { return getSubgraphsFromInstanceIds(subgraph, subgraphNodeIds, subgraphs) } - const executionIdToCurrentId = (id: string) => { + const executionIdToCurrentId = (id: NodeExecutionId) => { const subgraph = activeSubgraph.value // Short-circuit: ID belongs to the parent workflow / no active subgraph @@ -588,7 +588,7 @@ export const useWorkflowStore = defineStore('workflow', () => { * @returns The NodeLocatorId or null if conversion fails */ const nodeExecutionIdToNodeLocatorId = ( - nodeExecutionId: NodeExecutionId | string + nodeExecutionId: NodeExecutionId ): NodeLocatorId | null => { // Handle simple node IDs (root graph - no colons) if (!nodeExecutionId.includes(':')) { @@ -623,9 +623,7 @@ export const useWorkflowStore = defineStore('workflow', () => { * @param locatorId The NodeLocatorId * @returns The local node ID or null if invalid */ - const nodeLocatorIdToNodeId = ( - locatorId: NodeLocatorId | string - ): NodeId | null => { + const nodeLocatorIdToNodeId = (locatorId: NodeLocatorId): NodeId | null => { const parsed = parseNodeLocatorId(locatorId) return parsed?.localNodeId ?? null } @@ -637,7 +635,7 @@ export const useWorkflowStore = defineStore('workflow', () => { * @returns The execution ID or null if the node is not accessible from the target context */ const nodeLocatorIdToNodeExecutionId = ( - locatorId: NodeLocatorId | string, + locatorId: NodeLocatorId, targetSubgraph?: Subgraph ): NodeExecutionId | null => { const parsed = parseNodeLocatorId(locatorId) diff --git a/src/services/litegraphService.ts b/src/services/litegraphService.ts index 2149bb378..d4d50653e 100644 --- a/src/services/litegraphService.ts +++ b/src/services/litegraphService.ts @@ -151,8 +151,9 @@ export const useLitegraphService = () => { */ #setupStrokeStyles() { this.strokeStyles['running'] = function (this: LGraphNode) { - const nodeId = String(this.id) - const nodeLocatorId = useWorkflowStore().nodeIdToNodeLocatorId(nodeId) + const nodeLocatorId = useWorkflowStore().nodeIdToNodeLocatorId( + this.id + ) const state = useExecutionStore().nodeLocationProgressStates[nodeLocatorId]?.state if (state === 'running') { @@ -376,7 +377,7 @@ export const useLitegraphService = () => { node.title = nodeDef.display_name || nodeDef.name } - async function registerNodeDef(nodeId: string, nodeDefV1: ComfyNodeDefV1) { + async function registerNodeDef(type: string, nodeDefV1: ComfyNodeDefV1) { const node = class ComfyNode extends LGraphNode { static comfyClass: string static override title: string @@ -416,8 +417,9 @@ export const useLitegraphService = () => { */ #setupStrokeStyles() { this.strokeStyles['running'] = function (this: LGraphNode) { - const nodeId = String(this.id) - const nodeLocatorId = useWorkflowStore().nodeIdToNodeLocatorId(nodeId) + const nodeLocatorId = useWorkflowStore().nodeIdToNodeLocatorId( + this.id + ) const state = useExecutionStore().nodeLocationProgressStates[nodeLocatorId]?.state if (state === 'running') { @@ -649,7 +651,7 @@ export const useLitegraphService = () => { const nodeDef = new ComfyNodeDefImpl(nodeDefV1) node.nodeData = nodeDef - LiteGraph.registerNodeType(nodeId, node) + LiteGraph.registerNodeType(type, node) // Note: Do not following assignments before `LiteGraph.registerNodeType` // because `registerNodeType` will overwrite the assignments. node.category = nodeDef.category diff --git a/src/stores/executionStore.ts b/src/stores/executionStore.ts index 8791ab4e1..d858f0cd5 100644 --- a/src/stores/executionStore.ts +++ b/src/stores/executionStore.ts @@ -28,7 +28,7 @@ import type { import { api } from '@/scripts/api' import { app } from '@/scripts/app' import { useNodeOutputStore } from '@/stores/imagePreviewStore' -import type { NodeLocatorId } from '@/types/nodeIdentification' +import type { NodeExecutionId, NodeLocatorId } from '@/types/nodeIdentification' import { createNodeLocatorId } from '@/types/nodeIdentification' interface QueuedPrompt { @@ -78,8 +78,10 @@ function getSubgraphsFromInstanceIds( * @param nodeId The node ID from execution context (could be execution ID) * @returns The NodeLocatorId */ -function executionIdToNodeLocatorId(nodeId: string | number): NodeLocatorId { - const nodeIdStr = String(nodeId) +function executionIdToNodeLocatorId( + executionId: NodeExecutionId +): NodeLocatorId { + const nodeIdStr = String(executionId) if (!nodeIdStr.includes(':')) { // It's a top-level node ID diff --git a/src/types/nodeIdentification.ts b/src/types/nodeIdentification.ts index 90871bfd1..c4fc7b9de 100644 --- a/src/types/nodeIdentification.ts +++ b/src/types/nodeIdentification.ts @@ -105,7 +105,7 @@ export function createNodeLocatorId( * @param id The NodeExecutionId to parse * @returns Array of node IDs from root to target, or null if not an execution ID */ -export function parseNodeExecutionId(id: string): NodeId[] | null { +export function parseNodeExecutionId(id: NodeExecutionId): NodeId[] | null { if (!isNodeExecutionId(id)) return null return id diff --git a/tests-ui/tests/store/executionStore.test.ts b/tests-ui/tests/store/executionStore.test.ts index 498148e57..077bdada5 100644 --- a/tests-ui/tests/store/executionStore.test.ts +++ b/tests-ui/tests/store/executionStore.test.ts @@ -16,7 +16,6 @@ vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({ // Remove any previous global types declare global { - // eslint-disable-next-line @typescript-eslint/no-empty-object-type interface Window {} } @@ -148,7 +147,7 @@ describe('useExecutionStore - NodeLocatorId conversions', () => { }) it('should handle numeric node IDs', () => { - const result = store.executionIdToNodeLocatorId(123) + const result = store.executionIdToNodeLocatorId('123') // For numeric IDs, it should convert to string and return as-is expect(result).toBe('123') diff --git a/tests-ui/tests/utils/graphTraversalUtil.test.ts b/tests-ui/tests/utils/graphTraversalUtil.test.ts index f6cb74804..a434429e9 100644 --- a/tests-ui/tests/utils/graphTraversalUtil.test.ts +++ b/tests-ui/tests/utils/graphTraversalUtil.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest' +import type { NodeId } from '@/lib/litegraph/src/LGraphNode' import type { LGraph, LGraphNode, @@ -61,7 +62,7 @@ function createMockSubgraph(id: string, nodes: LGraphNode[]): Subgraph { id, _nodes: nodes, nodes: nodes, - getNodeById: (nodeId: string | number) => + getNodeById: (nodeId: NodeId) => nodes.find((n) => String(n.id) === String(nodeId)) || null } as unknown as Subgraph }