Skip to content
9 changes: 4 additions & 5 deletions src/extensions/core/customWidgets.clone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'
import { LGraph, LGraphNode, LiteGraph } from '@/lib/litegraph/src/litegraph'
import type { ComfyNodeDef } from '@/schemas/nodeDefSchema'
import { app } from '@/scripts/app'
import { getRootGraph, setRootGraph } from '@/scripts/__tests__/appTestUtils'
import { useExtensionStore } from '@/stores/extensionStore'
import type { ComfyExtension } from '@/types/comfy'

Expand Down Expand Up @@ -65,10 +66,8 @@ describe('CustomCombo copy/paste', () => {

it('preserves combo options and selected value through clone and paste', () => {
const graph = new LGraph()
type AppWithRootGraph = { rootGraphInternal?: LGraph }
const appWithRootGraph = app as unknown as AppWithRootGraph
const previousRootGraph = appWithRootGraph.rootGraphInternal
appWithRootGraph.rootGraphInternal = graph
const previousRootGraph = getRootGraph(app)
setRootGraph(app, graph)

try {
const original = LiteGraph.createNode(TEST_CUSTOM_COMBO_TYPE)!
Expand Down Expand Up @@ -97,7 +96,7 @@ describe('CustomCombo copy/paste', () => {
'gamma'
])
} finally {
appWithRootGraph.rootGraphInternal = previousRootGraph
setRootGraph(app, previousRootGraph)
}
})
})
17 changes: 7 additions & 10 deletions src/extensions/core/customWidgets.subgraphPromotion.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '@/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers'
import type { ComfyNodeDef } from '@/schemas/nodeDefSchema'
import { app } from '@/scripts/app'
import { getRootGraph, setRootGraph } from '@/scripts/__tests__/appTestUtils'
import { useExtensionStore } from '@/stores/extensionStore'
import { useWidgetValueStore } from '@/stores/widgetValueStore'
import type { ComfyExtension } from '@/types/comfy'
Expand Down Expand Up @@ -122,10 +123,8 @@ describe('CustomCombo index widget after subgraph promotion', () => {

it('resolves INDEX from the promoted host choice, not the frozen interior value', async () => {
const rootGraph = new LGraph()
type AppWithRootGraph = { rootGraphInternal?: LGraph }
const appWithRootGraph = app as unknown as AppWithRootGraph
const previousRootGraph = appWithRootGraph.rootGraphInternal
appWithRootGraph.rootGraphInternal = rootGraph
const previousRootGraph = getRootGraph(app)
setRootGraph(app, rootGraph)

try {
const subgraph = createTestSubgraph({ rootGraph })
Expand Down Expand Up @@ -167,16 +166,14 @@ describe('CustomCombo index widget after subgraph promotion', () => {
// "four" is index 3 of ["one", "two", "three", "four"].
expect(promptInputs.index).toBe(3)
} finally {
appWithRootGraph.rootGraphInternal = previousRootGraph
setRootGraph(app, previousRootGraph)
}
})

it('resolves INDEX from the interior widget when choice was never promoted', async () => {
const rootGraph = new LGraph()
type AppWithRootGraph = { rootGraphInternal?: LGraph }
const appWithRootGraph = app as unknown as AppWithRootGraph
const previousRootGraph = appWithRootGraph.rootGraphInternal
appWithRootGraph.rootGraphInternal = rootGraph
const previousRootGraph = getRootGraph(app)
setRootGraph(app, rootGraph)

try {
const comboNode = LiteGraph.createNode(
Expand All @@ -195,7 +192,7 @@ describe('CustomCombo index widget after subgraph promotion', () => {
// "two" is index 1 of ["one", "two", "three"].
expect(promptInputs.index).toBe(1)
} finally {
appWithRootGraph.rootGraphInternal = previousRootGraph
setRootGraph(app, previousRootGraph)
}
})
})
22 changes: 22 additions & 0 deletions src/scripts/__tests__/appTestUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import type { ShallowRef } from 'vue'

import type { LGraph } from '@/lib/litegraph/src/litegraph'
import type { ComfyApp } from '@/scripts/app'

/**
* `ComfyApp.setup` is the only production writer of the root graph, and it needs
* a real canvas. Tests that just need a graph in place reach the same storage
* through this seam instead.
*/
type AppWithRootGraphRef = { rootGraphRef: ShallowRef<LGraph | undefined> }

const rootGraphRefOf = (app: ComfyApp) =>
(app as unknown as AppWithRootGraphRef).rootGraphRef

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this without the type assertions by chance?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — dropped both assertions in 909788e0.

TypeScript sanctions element access as the escape hatch for a private member, so app['rootGraphRef'] reaches the same storage with no cast and no hand-written shadow interface. The seam is now two lines:

export function setRootGraph(app: ComfyApp, graph: LGraph | undefined) {
  app['rootGraphRef'].value = graph
}

The old as unknown as AppWithRootGraphRef was the worse form of the two problems: it did not just bypass privacy, it re-declared the field's type by hand, so a change to rootGraphRef in app.ts would have gone unnoticed here. Element access keeps the real ShallowRef<LGraph | undefined>.

Verified the type actually survives rather than degrading to any — assigned a number to .value and pnpm typecheck rejected it (Type 'number' is not assignable to type 'LGraph'), then reverted. pnpm typecheck, pnpm lint, pnpm knip, pnpm format all clean; rootGraphReadiness and both customWidgets suites green.


export function setRootGraph(app: ComfyApp, graph: LGraph | undefined) {
rootGraphRefOf(app).value = graph
}

export function getRootGraph(app: ComfyApp) {
return rootGraphRefOf(app).value
}
50 changes: 26 additions & 24 deletions src/scripts/app.test.ts
Comment thread
mattmillerai marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createTestingPinia } from '@pinia/testing'
import { fromPartial } from '@total-typescript/shoehorn'
import { setActivePinia } from 'pinia'
import { beforeEach, describe, expect, it, vi } from 'vitest'

Expand All @@ -18,6 +19,7 @@ import { createMockChangeTracker } from '@/utils/__tests__/litegraphTestUtils'
import { useNodeReplacementStore } from '@/platform/nodeReplacement/nodeReplacementStore'
import type { NodeReplacement } from '@/platform/nodeReplacement/types'
import { ComfyApp, app as singletonApp } from './app'
import { setRootGraph } from './__tests__/appTestUtils'
import { createNode } from '@/utils/litegraphUtil'
import {
pasteAudioNode,
Expand Down Expand Up @@ -302,8 +304,8 @@ describe('ComfyApp', () => {
size: 0
})
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
mockWorkspaceWorkflow.activeWorkflow = workflow
vi.spyOn(app, 'graphToPrompt').mockResolvedValue({
output: {},
Expand Down Expand Up @@ -340,8 +342,8 @@ describe('ComfyApp', () => {
]
}
}
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
mockWorkspaceWorkflow.activeWorkflow = workflow
vi.spyOn(app, 'graphToPrompt').mockResolvedValue({
output: promptOutput,
Expand Down Expand Up @@ -792,8 +794,8 @@ describe('ComfyApp', () => {
it('clears missing node packs before loading API JSON without missing nodes', async () => {
const graph = new LGraph()
const activeSubgraph = createTestSubgraph({ rootGraph: graph })
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
Reflect.set(mockCanvas, 'graph', activeSubgraph)
Reflect.set(mockCanvas, 'subgraph', activeSubgraph)
vi.mocked(mockCanvas.setGraph).mockImplementation((nextGraph) => {
Expand Down Expand Up @@ -829,8 +831,8 @@ describe('ComfyApp', () => {

it('creates a removable placeholder for an API JSON missing node', async () => {
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
const cleanupErrorHooks = installErrorClearingHooks(graph)
const missingNodesStore = useMissingNodesErrorStore()
const missingNodeType = 'Uninstalled<&Node>'
Expand Down Expand Up @@ -895,8 +897,8 @@ describe('ComfyApp', () => {

it('preserves API JSON inputs on a missing node across reload', async () => {
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
const sourceNodeType = 'test/ApiJsonSourceNode'
const missingNodeType = 'UninstalledInputNode'
class ApiJsonSourceNode extends LGraphNode {
Expand Down Expand Up @@ -1000,8 +1002,8 @@ describe('ComfyApp', () => {

it('defers API JSON missing node warnings until they are flushed', async () => {
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
const nodeReplacementStore = useNodeReplacementStore()
vi.spyOn(nodeReplacementStore, 'load').mockResolvedValue()
vi.spyOn(nodeReplacementStore, 'getReplacementFor').mockReturnValue(null)
Expand Down Expand Up @@ -1129,7 +1131,7 @@ describe('ComfyApp', () => {
experimental: false
}
}
Reflect.set(app, 'rootGraphInternal', rootGraph)
setRootGraph(app, rootGraph)
vi.spyOn(app, 'getNodeDefs').mockResolvedValue(defs)
vi.spyOn(app, 'registerNodeDef').mockResolvedValue(undefined)

Expand All @@ -1148,15 +1150,15 @@ describe('ComfyApp', () => {

describe('refreshMissingModels', () => {
it('delegates to the app-independent missing model refresh pipeline', async () => {
const graph = {
const graph = fromPartial<LGraph>({
nodes: [],
serialize: vi.fn(() => createWorkflowGraphData())
}
})
const result = {
missingModels: [],
confirmedCandidates: []
}
Reflect.set(app, 'rootGraphInternal', graph)
setRootGraph(app, graph)
vi.spyOn(app, 'reloadNodeDefs').mockResolvedValue()
mockRefreshMissingModelPipeline.mockResolvedValue(result)

Expand All @@ -1176,11 +1178,11 @@ describe('ComfyApp', () => {
})

it('omits the node definition reload when reloadDefs is false', async () => {
const graph = {
const graph = fromPartial<LGraph>({
nodes: [],
serialize: vi.fn(() => createWorkflowGraphData())
}
Reflect.set(app, 'rootGraphInternal', graph)
})
setRootGraph(app, graph)
vi.spyOn(app, 'reloadNodeDefs').mockResolvedValue()
mockRefreshMissingModelPipeline.mockResolvedValue({
missingModels: [],
Expand Down Expand Up @@ -1565,7 +1567,7 @@ describe('ComfyApp', () => {
it('preserves the current graph when A1111 core nodes are unavailable', async () => {
const graph = new LGraph()
const parameters = 'positive\nNegative prompt: negative\nSteps: 20'
Reflect.set(app, 'rootGraphInternal', graph)
setRootGraph(app, graph)
vi.mocked(getWorkflowDataFromFile).mockResolvedValue({ parameters })
mockImportA1111.mockResolvedValue('core-nodes-unavailable')

Expand All @@ -1588,7 +1590,7 @@ describe('ComfyApp', () => {
it('shows one file-load error when parameters are not A1111-shaped', async () => {
const graph = new LGraph()
const parameters = 'positive\nSteps: 20'
Reflect.set(app, 'rootGraphInternal', graph)
setRootGraph(app, graph)
vi.mocked(getWorkflowDataFromFile).mockResolvedValue({ parameters })
mockImportA1111.mockResolvedValue('not-a1111')

Expand All @@ -1605,7 +1607,7 @@ describe('ComfyApp', () => {
it('awaits persistence and orders its clear callback before setGraph', async () => {
const graph = new LGraph()
const parameters = 'positive\nNegative prompt: negative\nSteps: 20'
Reflect.set(app, 'rootGraphInternal', graph)
setRootGraph(app, graph)
vi.mocked(getWorkflowDataFromFile).mockResolvedValue({ parameters })
mockImportA1111.mockImplementation(
async (_graph, _parameters, beforeGraphClear) => {
Expand Down Expand Up @@ -1656,8 +1658,8 @@ describe('ComfyApp', () => {
} as unknown as LGraphCanvas

const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
Reflect.set(singletonApp, 'rootGraphInternal', graph)
setRootGraph(app, graph)
setRootGraph(singletonApp, graph)
const outgoingWorkflow = new ComfyWorkflow({
path: 'workflows/outgoing.json',
modified: 0,
Expand Down
13 changes: 7 additions & 6 deletions src/scripts/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,24 +281,25 @@ export class ComfyApp {
private _nodeOutputs!: Record<string, NodeExecutionOutput>
nodePreviewImages: Record<string, string[]>

private rootGraphInternal: LGraph | undefined
/** Shallow: the graph is observed for readiness, never deep-proxied. */
private readonly rootGraphRef = shallowRef<LGraph | undefined>(undefined)

// TODO: Migrate internal usage to the
/** @deprecated Use {@link rootGraph} instead */
get graph() {
return this.rootGraphInternal!
Comment thread
mattmillerai marked this conversation as resolved.
return this.rootGraphRef.value!
}

get rootGraph(): LGraph {
if (!this.rootGraphInternal) {
if (!this.rootGraphRef.value) {
console.error('ComfyApp graph accessed before initialization')
}
return this.rootGraphInternal!
return this.rootGraphRef.value!
}

/** Whether the root graph has been initialized. Safe to check without triggering error logs. */
get isGraphReady(): boolean {
return !!this.rootGraphInternal
return !!this.rootGraphRef.value
}

canvas!: LGraphCanvas
Expand Down Expand Up @@ -957,7 +958,7 @@ export class ComfyApp {

this.addAfterConfigureHandler(graph)

this.rootGraphInternal = graph
this.rootGraphRef.value = graph
installNodeAddedTelemetry(graph)
this.canvas = new LGraphCanvas(canvasEl, graph)
// Make canvas states reactive so we can observe changes on them.
Expand Down
55 changes: 55 additions & 0 deletions src/scripts/rootGraphReadiness.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { useEventListener } from '@vueuse/core'
import { createPinia, setActivePinia } from 'pinia'
import { effectScope, nextTick, watchEffect } from 'vue'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'

import { LGraph } from '@/lib/litegraph/src/litegraph'
import { getRootGraph, setRootGraph } from '@/scripts/__tests__/appTestUtils'
import { app } from '@/scripts/app'

describe('ComfyApp root graph readiness', () => {
let scope: ReturnType<typeof effectScope>
let previousRootGraph: LGraph | undefined

beforeEach(() => {
setActivePinia(createPinia())
vi.spyOn(console, 'error').mockImplementation(() => {})
previousRootGraph = getRootGraph(app)
setRootGraph(app, undefined)
scope = effectScope()
})

afterEach(() => {
scope.stop()
setRootGraph(app, previousRootGraph)
vi.restoreAllMocks()
})

it('re-runs an effect reading isGraphReady when the graph is assigned', async () => {
const readiness: boolean[] = []
scope.run(() => {
watchEffect(() => readiness.push(app.isGraphReady))
})

expect(readiness).toEqual([false])

setRootGraph(app, new LGraph())
await nextTick()

expect(readiness).toEqual([false, true])
})

it('binds a rootGraph.events listener registered before the graph exists', async () => {
const onConfigured = vi.fn()
scope.run(() => {
useEventListener(() => app.rootGraph?.events, 'configured', onConfigured)
})

const graph = new LGraph()
setRootGraph(app, graph)
await nextTick()
graph.events.dispatch('configured')

expect(onConfigured).toHaveBeenCalledOnce()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
})
Loading