Skip to content

Cleanup app.graph usage #7399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
christian-byrne merged 4 commits into from
Dec 12, 2025
Merged

Cleanup app.graph usage #7399

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74b7cf3
Remove incorrect rootGraph usage
AustinMroz Dec 9, 2025
b87e660
WIP app.graph removal
AustinMroz Dec 9, 2025
10c9098
Fix tests
AustinMroz Dec 12, 2025
eae2cfb
nits
AustinMroz Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
nits
  • Loading branch information
@AustinMroz
AustinMroz committed Dec 12, 2025
commit eae2cfbdfe729bccfa25708dd5d0e7bce5764704
39 changes: 13 additions & 26 deletions tests-ui/tests/composables/useMissingNodes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { nextTick, ref } from 'vue'

import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
import { app } from '@/scripts/app'
import type { LGraphNode, LGraph } from '@/lib/litegraph/src/litegraph'
import { useNodeDefStore } from '@/stores/nodeDefStore'
import { collectAllNodes } from '@/utils/graphTraversalUtil'
import { useMissingNodes } from '@/workbench/extensions/manager/composables/nodePack/useMissingNodes'
Expand Down Expand Up @@ -40,12 +39,10 @@ vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({
}))
}))

const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({}))

vi.mock('@/scripts/app', () => ({
app: {
graph: {
nodes: []
}
}
app: mockApp
}))
Comment on lines +42 to 46
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Make mockApp.rootGraph non-optional to avoid undefined runtime traps
rootGraph?: ... means useMissingNodes() can still blow up if anything touches app.rootGraph before beforeEach runs (or if a future test forgets to set it). Initialize it in the hoisted factory and drop the optional.

-const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({}))
+const mockApp: { rootGraph: Partial<LGraph> } = vi.hoisted(() => ({
+  rootGraph: { nodes: [], subgraphs: new Map() }
+}))

As per coding guidelines (type-safety / avoid brittle setup), this makes the mock more robust.
[scratchpad_end] -->

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({}))
vi.mock('@/scripts/app', () => ({
app: {
graph: {
nodes: []
}
}
app: mockApp
}))
const mockApp: { rootGraph: Partial<LGraph> } = vi.hoisted(() => ({
rootGraph: { nodes: [], subgraphs: new Map() }
}))
vi.mock('@/scripts/app', () => ({
app: mockApp
}))
🤖 Prompt for AI Agents 
In tests-ui/tests/composables/useMissingNodes.test.ts around lines 42 to 46, the
mockApp declaration makes rootGraph optional which can cause runtime errors if
accessed before beforeEach sets it; change the hoisted factory to initialize
rootGraph (e.g. {} as Partial<LGraph>) and update the mockApp type so rootGraph
is non-optional (remove the ?), ensuring the mocked app always has a defined
rootGraph before tests run.


vi.mock('@/utils/graphTraversalUtil', () => ({
Expand Down Expand Up @@ -107,9 +104,8 @@ describe('useMissingNodes', () => {
nodeDefsByName: {}
})

// Reset app.graph.nodes
// @ts-expect-error - app.graph.nodes is readonly, but we need to modify it for testing.
app.graph.nodes = []
// Reset app.rootGraph.nodes
mockApp.rootGraph = { nodes: [] }

Comment on lines +107 to 109
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Reset should clear subgraphs too (not just nodes)
Some tests build subgraph scenarios; leaving subgraphs undefined (or stale if later added) makes cross-test coupling easier to introduce.

-mockApp.rootGraph = { nodes: [] }
+mockApp.rootGraph = { nodes: [], subgraphs: new Map() }
🤖 Prompt for AI Agents 
In tests-ui/tests/composables/useMissingNodes.test.ts around lines 107 to 109,
the test reset only clears mockApp.rootGraph.nodes which leaves
mockApp.rootGraph.subgraphs undefined or stale; update the reset to explicitly
clear subgraphs as well (e.g., set mockApp.rootGraph = { nodes: [], subgraphs:
[] } or ensure mockApp.rootGraph.subgraphs = [] after reset) so each test starts
with a clean rootGraph state.

// Default mock for collectAllNodes - returns empty array
mockCollectAllNodes.mockReturnValue([])
Expand Down Expand Up @@ -306,13 +302,7 @@ describe('useMissingNodes', () => {
})

describe('missing core nodes detection', () => {
const createMockNode = (
type: string,
packId?: string,
version?: string
): LGraphNode =>
// @ts-expect-error - Creating a partial mock of LGraphNode for testing.
// We only need specific properties for our tests, not the full LGraphNode interface.
const createMockNode = (type: string, packId?: string, version?: string) =>
({
type,
properties: { cnr_id: packId, ver: version },
Expand All @@ -325,7 +315,7 @@ describe('useMissingNodes', () => {
mode: 0,
inputs: [],
outputs: []
})
}) as unknown as LGraphNode

it('identifies missing core nodes not in nodeDefStore', () => {
const coreNode1 = createMockNode('CoreNode1', 'comfy-core', '1.2.0')
Expand Down Expand Up @@ -467,8 +457,7 @@ describe('useMissingNodes', () => {

it('calls collectAllNodes with the app graph and filter function', () => {
const mockGraph = { nodes: [], subgraphs: new Map() }
// @ts-expect-error - Mocking app.graph for testing
app.rootGraph = mockGraph
mockApp.rootGraph = mockGraph

const { missingCoreNodes } = useMissingNodes()
// Access the computed to trigger the function
Expand All @@ -490,8 +479,7 @@ describe('useMissingNodes', () => {

it('filter function correctly identifies missing core nodes', () => {
const mockGraph = { nodes: [], subgraphs: new Map() }
// @ts-expect-error - Mocking app.graph for testing
app.graph = mockGraph
mockApp.rootGraph = mockGraph

mockUseNodeDefStore.mockReturnValue({
nodeDefsByName: {
Expand Down Expand Up @@ -579,14 +567,13 @@ describe('useMissingNodes', () => {
subgraph: mockSubgraph,
type: 'SubgraphContainer',
properties: { cnr_id: 'custom-pack' }
}
} as unknown as LGraphNode

const mockMainGraph = {
nodes: [mainMissingNode, mockSubgraphNode]
}
} as Partial<LGraph> as LGraph

// @ts-expect-error - Mocking app.graph for testing
app.rootGraph = mockMainGraph
mockApp.rootGraph = mockMainGraph

mockUseNodeDefStore.mockReturnValue({
nodeDefsByName: {
Expand Down
23 changes: 11 additions & 12 deletions tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@ import { createTestingPinia } from '@pinia/testing'
import { mount } from '@vue/test-utils'
import { beforeEach, describe, expect, it, vi } from 'vitest'

import type {
LGraph,
LGraphNode,
SubgraphNode
} from '@/lib/litegraph/src/litegraph'
import type { VueNodeData } from '@/composables/graph/useGraphNodeManager'
import NodeHeader from '@/renderer/extensions/vueNodes/components/NodeHeader.vue'
import { getNodeByLocatorId } from '@/utils/graphTraversalUtil'

const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({}))
// Mock dependencies
vi.mock('@/scripts/app', () => ({
app: {
rootGraph: null as any
}
app: mockApp
}))

vi.mock('@/utils/graphTraversalUtil', () => ({
Expand Down Expand Up @@ -55,17 +59,12 @@ vi.mock('@/i18n', () => ({
describe('NodeHeader - Subgraph Functionality', () => {
// Helper to setup common mocks
const setupMocks = async (isSubgraph = true, hasGraph = true) => {
const { app } = await import('@/scripts/app')

if (hasGraph) {
;(app as any).rootGraph = { rootGraph: {} }
} else {
;(app as any).rootGraph = null
}
if (hasGraph) mockApp.rootGraph = {}
else mockApp.rootGraph = undefined

vi.mocked(getNodeByLocatorId).mockReturnValue({
isSubgraphNode: () => isSubgraph
} as any)
isSubgraphNode: (): this is SubgraphNode => isSubgraph
} as LGraphNode)
Comment on lines 65 to +67
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider avoiding the type assertion in the mock.

The mock return value is cast as LGraphNode but only provides the isSubgraphNode method. While this works if no other LGraphNode properties are accessed, a more robust approach would be to either:

  • Create a fuller mock object implementing the minimal LGraphNode interface
  • Return a properly typed object without the cast

This is acceptable for test code but could be cleaner.

vi.mocked(getNodeByLocatorId).mockReturnValue({
  isSubgraphNode: (): this is SubgraphNode => isSubgraph,
  // Add other minimal properties if accessed by the component
} as unknown as LGraphNode)
🤖 Prompt for AI Agents 
In
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts
around lines 65-67, the mock return value uses a narrow object cast directly to
LGraphNode which hides missing properties; replace the unsafe cast by returning
a fuller mock that implements the minimal LGraphNode surface used by the
component (add any accessed properties/methods alongside isSubgraphNode) or, if
you prefer keeping the small object, cast via unknown as LGraphNode to be
explicit; ensure the mock includes any other properties accessed in the test to
avoid runtime/typing gaps.

}

beforeEach(() => {
Expand Down