mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-06-28 02:27:21 +00:00
## Summary Muted and bypassed nodes are excluded from execution but were still triggering missing model/media/node warnings. This PR makes the error system mode-aware: muted/bypassed nodes no longer produce missing asset errors, and all error lifecycle events (mode toggle, deletion, paste, undo, tab switch) are handled consistently. - Fixes Comfy-Org/ComfyUI#13256 ## Behavioral notes - **Tab switch overlay suppression (intentional)**: Switching back to a workflow with missing assets no longer re-shows the error overlay. This reverses the behavior introduced in #10190. The error state is still restored silently in the errors tab — users can access it via the properties panel without being interrupted by the overlay on every tab switch. ## Changes ### 1. Scan filtering - `scanAllModelCandidates`, `scanAllMediaCandidates`, `scanMissingNodes`: skip nodes with `mode === NEVER || BYPASS` - `collectMissingNodes` (serialized data): skip error reporting for muted/bypassed nodes while still calling `sanitizeNodeName` for safe `configure()` - `collectEmbeddedModelsWithSource`: skip muted/bypassed nodes; workflow-level `graphData.models` only create candidates when active nodes exist - `enrichWithEmbeddedMetadata`: filter unmatched workflow-level models when all referencing nodes are inactive ### 2. Realtime mode change handling - `useErrorClearingHooks.ts` chains `graph.onTrigger` to detect `node:property:changed` (mode) - Deactivation (active → muted/bypassed): remove missing model/media/node errors for the node - Activation (muted/bypassed → active): scan the node and add confirmed errors, show overlay - Subgraph container deactivation: remove all interior node errors (execution ID prefix match) - Subgraph container activation: scan all active interior nodes recursively - Subgraph interior mode change: resolve node via `localGraph.getNodeById()` then compute execution ID from root graph ### 3. Node deletion - `graph.onNodeRemoved`: remove missing model/media/node errors for the deleted node - Handle `node.graph === null` at callback time by using `String(node.id)` for root-level nodes ### 4. Node paste/duplicate - `graph.onNodeAdded`: scan via `queueMicrotask` (deferred until after `node.configure()` restores widget values) - Guard: skip during `ChangeTracker.isLoadingGraph` (undo/redo/tab switch handled by pipeline) - Guard: skip muted/bypassed nodes ### 5. Workflow tab switch optimization - `skipAssetScans` option in `loadGraphData`: skip full pipeline on tab switch - Cache missing model/media/node state per workflow via `PendingWarnings` - `beforeLoadNewGraph`: save current store state to outgoing workflow's `pendingWarnings` - `showPendingWarnings`: restore cached errors silently (no overlay), always sync missing nodes store (even when null) - Preserve UI state (`fileSizes`, `urlInputs`) on tab switch by using `setMissingModels([])` instead of `clearMissingModels()` - `MissingModelRow.vue`: fetch file size on mount via `fetchModelMetadata` memory cache ### 6. Undo/redo overlay suppression - `silentAssetErrors` option propagated through pipeline → `surfaceMissingModels`/`surfaceMissingMedia` `{ silent }` option - `showPendingWarnings` `{ silent }` option for missing nodes overlay - `changeTracker.ts`: pass `silentAssetErrors: true` on undo/redo ### 7. Error tab node filtering - Selected node filters missing model/media card contents (not just group visibility) - `isAssetErrorInSelection`: resolve execution ID → graph node for selection matching - Missing nodes intentionally unfiltered (pack-level scope) - `hasMissingMediaSelected` added to `RightSidePanel.vue` error tab visibility - Download All button: show only when 2+ downloadable models exist ### 8. New store functions - `missingModelStore`: `addMissingModels`, `removeMissingModelsByNodeId` - `missingMediaStore`: `addMissingMedia`, `removeMissingMediaByNodeId` - `missingNodesErrorStore`: `removeMissingNodesByNodeId` - `missingModelScan`: `scanNodeModelCandidates` (extracted single-node scan) - `missingMediaScan`: `scanNodeMediaCandidates` (extracted single-node scan) ### 9. Test infrastructure improvements - `data-testid` on `RightSidePanel.vue` tabs (`panel-tab-{value}`) - Error-related TestIds moved from `dialogs` to `errorsTab` namespace in `selectors.ts` - Removed unused `TestIdValue` type - Extracted `cleanupFakeModel` to shared `ErrorsTabHelper.ts` - Renamed `openErrorsTabViaSeeErrors` → `loadWorkflowAndOpenErrorsTab` - Added `aria-label` to pencil edit button and subgraph toggle button ## Test plan ### Unit tests (41 new) - Store functions: `addMissing*`, `removeMissing*ByNodeId` - `executionErrorStore`: `surfaceMissing*` silent option - Scan functions: muted/bypassed filtering, `scanNodeModelCandidates`, `scanNodeMediaCandidates` - `workflowService`: `showPendingWarnings` silent, `beforeLoadNewGraph` caching ### E2E tests (17 new in `errorsTabModeAware.spec.ts`) **Missing nodes** - [x] Deleting a missing node removes its error from the errors tab - [x] Undo after bypass restores error without showing overlay **Missing models** - [x] Loading a workflow with all nodes bypassed shows no errors - [x] Bypassing a node hides its error, un-bypassing restores it - [x] Deleting a node with missing model removes its error - [x] Undo after bypass restores error without showing overlay - [x] Pasting a node with missing model increases referencing node count - [x] Pasting a bypassed node does not add a new error - [x] Selecting a node filters errors tab to only that node **Missing media** - [x] Loading a workflow with all nodes bypassed shows no errors - [x] Bypassing a node hides its error, un-bypassing restores it - [x] Pasting a bypassed node does not add a new error - [x] Selecting a node filters errors tab to only that node **Subgraph** - [x] Bypassing a subgraph hides interior errors, un-bypassing restores them - [x] Bypassing a node inside a subgraph hides its error, un-bypassing restores it **Workflow switching** - [x] Does not resurface error overlay when switching back to workflow with missing nodes - [x] Restores missing nodes in errors tab when switching back to workflow # Screenshots https://github.com/user-attachments/assets/e0a5bcb8-69ba-4120-ab7f-5c83e4cfc3c5 ## Follow-up work - Extract error-detection computed properties from `RightSidePanel.vue` into a composable (e.g. `useErrorsTabVisibility`) --------- Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: GitHub Action <action@github.com>
275 lines
8.5 KiB
TypeScript
275 lines
8.5 KiB
TypeScript
import { fromAny, fromPartial } from '@total-typescript/shoehorn'
|
|
import { createPinia, setActivePinia } from 'pinia'
|
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
|
|
import type { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph'
|
|
import { LiteGraph } from '@/lib/litegraph/src/litegraph'
|
|
|
|
vi.mock('@/lib/litegraph/src/litegraph', async (importOriginal) => {
|
|
const actual = await importOriginal<Record<string, unknown>>()
|
|
return {
|
|
...actual,
|
|
LiteGraph: {
|
|
...(actual.LiteGraph as Record<string, unknown>),
|
|
registered_node_types: {} as Record<string, unknown>
|
|
}
|
|
}
|
|
})
|
|
|
|
vi.mock('@/utils/graphTraversalUtil', () => ({
|
|
collectAllNodes: vi.fn(),
|
|
getExecutionIdByNode: vi.fn()
|
|
}))
|
|
|
|
vi.mock('@/platform/nodeReplacement/cnrIdUtil', () => ({
|
|
getCnrIdFromNode: vi.fn(() => undefined)
|
|
}))
|
|
|
|
vi.mock('@/i18n', () => ({
|
|
st: vi.fn((_key: string, fallback: string) => fallback)
|
|
}))
|
|
|
|
vi.mock('@/platform/distribution/types', () => ({
|
|
isCloud: false
|
|
}))
|
|
|
|
vi.mock('@/stores/settingStore', () => ({
|
|
useSettingStore: () => ({
|
|
get: vi.fn(() => true)
|
|
})
|
|
}))
|
|
|
|
vi.mock('@/platform/settings/settingStore', () => ({
|
|
useSettingStore: () => ({
|
|
get: vi.fn(() => true)
|
|
})
|
|
}))
|
|
|
|
import {
|
|
collectAllNodes,
|
|
getExecutionIdByNode
|
|
} from '@/utils/graphTraversalUtil'
|
|
import { getCnrIdFromNode } from '@/platform/nodeReplacement/cnrIdUtil'
|
|
import { useNodeReplacementStore } from '@/platform/nodeReplacement/nodeReplacementStore'
|
|
import { rescanAndSurfaceMissingNodes } from './missingNodeScan'
|
|
import { useMissingNodesErrorStore } from '@/platform/nodeReplacement/missingNodesErrorStore'
|
|
|
|
function mockNode(
|
|
id: number,
|
|
type: string,
|
|
overrides: Record<string, unknown> = {}
|
|
): LGraphNode {
|
|
return fromAny<LGraphNode, unknown>({
|
|
id,
|
|
type,
|
|
last_serialization: { type },
|
|
...overrides
|
|
})
|
|
}
|
|
|
|
function mockGraph(): LGraph {
|
|
return fromAny<LGraph, unknown>({})
|
|
}
|
|
|
|
function getMissingNodesError(
|
|
store: ReturnType<typeof useMissingNodesErrorStore>
|
|
) {
|
|
const error = store.missingNodesError
|
|
if (!error) throw new Error('Expected missingNodesError to be defined')
|
|
return error
|
|
}
|
|
|
|
describe('scanMissingNodes (via rescanAndSurfaceMissingNodes)', () => {
|
|
beforeEach(() => {
|
|
setActivePinia(createPinia())
|
|
vi.clearAllMocks()
|
|
// Reset registered_node_types
|
|
const reg = LiteGraph.registered_node_types as Record<string, unknown>
|
|
for (const key of Object.keys(reg)) {
|
|
delete reg[key]
|
|
}
|
|
})
|
|
|
|
it('returns empty when all nodes are registered', () => {
|
|
const reg = LiteGraph.registered_node_types as Record<string, unknown>
|
|
reg['KSampler'] = {}
|
|
|
|
vi.mocked(collectAllNodes).mockReturnValue([mockNode(1, 'KSampler')])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
expect(store.missingNodesError).toBeNull()
|
|
})
|
|
|
|
it('detects unregistered nodes as missing', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([
|
|
mockNode(1, 'OldNode'),
|
|
mockNode(2, 'AnotherOldNode')
|
|
])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
expect(error.nodeTypes).toHaveLength(2)
|
|
})
|
|
|
|
it('skips registered nodes and lists only unregistered', () => {
|
|
const reg = LiteGraph.registered_node_types as Record<string, unknown>
|
|
reg['RegisteredNode'] = {}
|
|
|
|
vi.mocked(collectAllNodes).mockReturnValue([
|
|
mockNode(1, 'RegisteredNode'),
|
|
mockNode(2, 'UnregisteredNode')
|
|
])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
expect(error.nodeTypes).toHaveLength(1)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.type).toBe('UnregisteredNode')
|
|
})
|
|
|
|
it('uses executionId when available for nodeId', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([mockNode(1, 'Missing')])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue('exec-42')
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.nodeId).toBe('exec-42')
|
|
})
|
|
|
|
it('falls back to node.id when executionId is null', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([mockNode(99, 'Missing')])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.nodeId).toBe('99')
|
|
})
|
|
|
|
it('populates cnrId from getCnrIdFromNode', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([mockNode(1, 'Missing')])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
vi.mocked(getCnrIdFromNode).mockReturnValue('comfy-nodes/my-pack')
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.cnrId).toBe(
|
|
'comfy-nodes/my-pack'
|
|
)
|
|
})
|
|
|
|
it('marks node as replaceable when replacement exists', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([mockNode(1, 'OldNode')])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
const replacementStore = useNodeReplacementStore()
|
|
replacementStore.replacements = {
|
|
OldNode: [
|
|
{
|
|
old_node_id: 'OldNode',
|
|
new_node_id: 'NewNode',
|
|
old_widget_ids: null,
|
|
input_mapping: null,
|
|
output_mapping: null
|
|
}
|
|
]
|
|
}
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.isReplaceable).toBe(true)
|
|
expect(
|
|
typeof missing !== 'string' && missing.replacement?.new_node_id
|
|
).toBe('NewNode')
|
|
})
|
|
|
|
it('marks node as not replaceable when no replacement', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([mockNode(1, 'OldNode')])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.isReplaceable).toBe(false)
|
|
})
|
|
|
|
it('skips muted nodes (mode NEVER = 2)', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([
|
|
mockNode(1, 'MutedNode', { mode: 2 })
|
|
])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
expect(store.missingNodesError).toBeNull()
|
|
})
|
|
|
|
it('skips bypassed nodes (mode BYPASS = 4)', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([
|
|
mockNode(1, 'BypassedNode', { mode: 4 })
|
|
])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
expect(store.missingNodesError).toBeNull()
|
|
})
|
|
|
|
it('detects active nodes (mode ALWAYS = 0) as missing', () => {
|
|
vi.mocked(collectAllNodes).mockReturnValue([
|
|
mockNode(1, 'ActiveMissingNode', { mode: 0 })
|
|
])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
expect(error.nodeTypes).toHaveLength(1)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.type).toBe(
|
|
'ActiveMissingNode'
|
|
)
|
|
})
|
|
|
|
it('uses last_serialization.type over node.type', () => {
|
|
const node = mockNode(1, 'LiveType')
|
|
node.last_serialization = fromPartial<LGraphNode['last_serialization']>({
|
|
type: 'OriginalType'
|
|
})
|
|
vi.mocked(collectAllNodes).mockReturnValue([node])
|
|
vi.mocked(getExecutionIdByNode).mockReturnValue(null)
|
|
|
|
rescanAndSurfaceMissingNodes(mockGraph())
|
|
|
|
const store = useMissingNodesErrorStore()
|
|
const error = getMissingNodesError(store)
|
|
const missing = error.nodeTypes[0]
|
|
expect(typeof missing !== 'string' && missing.type).toBe('OriginalType')
|
|
})
|
|
})
|