fix: onNodeRemoved not called when loading new graph (and tearing down previous) (#5407)

* standardize graph cleanup

* test: fix useCoreCommands tests and add regression test

- Fix mocking to properly simulate app.clean() calling graph.clear()
- Add intelligent subgraph detection in mock to match real implementation
- Add regression test for Vue node cleanup bug to prevent future regressions
- Ensures app.clean() properly triggers onNodeRemoved events through graph.clear()

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix unit tests

* move beforeLoadNewGraph to before graph is cleaned

---------

Co-authored-by: Claude <noreply@anthropic.com>

src/composables/useCoreCommands.ts

@@ -180,8 +180,6 @@ export function useCoreCommands(): ComfyCommand[] {
             const subgraph = app.canvas.subgraph
             const nonIoNodes = getAllNonIoNodesInSubgraph(subgraph)
             nonIoNodes.forEach((node) => subgraph.remove(node))
-          } else {
-            app.graph.clear()
           }
           api.dispatchCustomEvent('graphCleared')
         }

src/lib/litegraph/src/LGraph.ts

@@ -314,6 +314,7 @@ export class LGraph
     if (this._nodes) {
       for (const _node of this._nodes) {
         _node.onRemoved?.()
+        this.onNodeRemoved?.(_node)
       }
     }
 

src/scripts/app.ts

@@ -1058,6 +1058,8 @@ export class ComfyApp {
       checkForRerouteMigration = false
     } = {}
   ) {
+    useWorkflowService().beforeLoadNewGraph()
+
     if (clean !== false) {
       this.clean()
     }
@@ -1093,7 +1095,6 @@ export class ComfyApp {
         severity: 'warn'
       })
     }
-    useWorkflowService().beforeLoadNewGraph()
     useSubgraphService().loadSubgraphs(graphData)
 
     const missingNodeTypes: MissingNodeType[] = []
@@ -1765,6 +1766,12 @@ export class ComfyApp {
     executionStore.lastExecutionError = null
 
     useDomWidgetStore().clear()
+
+    // Subgraph does not properly implement `clear` and the parent class's
+    // (`LGraph`) `clear` breaks the subgraph structure.
+    if (this.graph && !this.canvas.subgraph) {
+      this.graph.clear()
+    }
   }
 
   clientPosToCanvasPos(pos: Vector2): Vector2 {

src/scripts/ui.ts

@@ -634,7 +634,6 @@ export class ComfyUI {
               confirm('Clear workflow?')
             ) {
               app.clean()
-              app.graph.clear()
               useLitegraphService().resetView()
               api.dispatchCustomEvent('graphCleared')
             }

tests-ui/tests/composables/useCoreCommands.test.ts

@@ -6,17 +6,25 @@ import { api } from '@/scripts/api'
 import { app } from '@/scripts/app'
 import { useSettingStore } from '@/stores/settingStore'
 
-vi.mock('@/scripts/app', () => ({
-  app: {
-    clean: vi.fn(),
-    canvas: {
-      subgraph: null
-    },
-    graph: {
-      clear: vi.fn()
+vi.mock('@/scripts/app', () => {
+  const mockGraphClear = vi.fn()
+  const mockCanvas = { subgraph: undefined }
+
+  return {
+    app: {
+      clean: vi.fn(() => {
+        // Simulate app.clean() calling graph.clear() only when not in subgraph
+        if (!mockCanvas.subgraph) {
+          mockGraphClear()
+        }
+      }),
+      canvas: mockCanvas,
+      graph: {
+        clear: mockGraphClear
+      }
     }
   }
-}))
+})
 
 vi.mock('@/scripts/api', () => ({
   api: {

tests-ui/tests/litegraph/core/LGraph.test.ts

@@ -1,6 +1,6 @@
 import { describe } from 'vitest'
 
-import { LGraph, LiteGraph } from '@/lib/litegraph/src/litegraph'
+import { LGraph, LGraphNode, LiteGraph } from '@/lib/litegraph/src/litegraph'
 
 import { test } from './fixtures/testExtensions'
 
@@ -128,6 +128,54 @@ describe('Floating Links / Reroutes', () => {
   })
 })
 
+describe('Graph Clearing and Callbacks', () => {
+  test('clear() calls both node.onRemoved() and graph.onNodeRemoved()', ({
+    expect
+  }) => {
+    const graph = new LGraph()
+
+    // Create test nodes with onRemoved callbacks
+    const node1 = new LGraphNode('TestNode1')
+    const node2 = new LGraphNode('TestNode2')
+
+    // Add nodes to graph
+    graph.add(node1)
+    graph.add(node2)
+
+    // Track callback invocations
+    const nodeRemovedCallbacks = new Set<string>()
+    const graphRemovedCallbacks = new Set<string>()
+
+    // Set up node.onRemoved() callbacks
+    node1.onRemoved = () => {
+      nodeRemovedCallbacks.add(String(node1.id))
+    }
+    node2.onRemoved = () => {
+      nodeRemovedCallbacks.add(String(node2.id))
+    }
+
+    // Set up graph.onNodeRemoved() callback
+    graph.onNodeRemoved = (node) => {
+      graphRemovedCallbacks.add(String(node.id))
+    }
+
+    // Verify nodes are in graph before clearing
+    expect(graph.nodes.length).toBe(2)
+
+    // Clear the graph
+    graph.clear()
+
+    // Verify both types of callbacks were called
+    expect(nodeRemovedCallbacks).toContain(String(node1.id))
+    expect(nodeRemovedCallbacks).toContain(String(node2.id))
+    expect(graphRemovedCallbacks).toContain(String(node1.id))
+    expect(graphRemovedCallbacks).toContain(String(node2.id))
+
+    // Verify nodes were actually removed
+    expect(graph.nodes.length).toBe(0)
+  })
+})
+
 describe('Legacy LGraph Compatibility Layer', () => {
   test('can be extended via prototype', ({ expect, minimalGraph }) => {
     // @ts-expect-error Should always be an error.