mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-04 21:22:07 +00:00
Follow-up to #10856. Four correctness issues and their regression tests. ## Bugs fixed ### 1. ErrorOverlay model count reflected node selection `useErrorGroups` exposed `filteredMissingModelGroups` under the public name `missingModelGroups`. `ErrorOverlay.vue` read that alias to compute its model count label, so selecting a node shrank the overlay total. The overlay must always show the whole workflow's errors. Exposed both shapes explicitly: `missingModelGroups` / `missingMediaGroups` (unfiltered totals) and `filteredMissingModelGroups` / `filteredMissingMediaGroups` (selection-scoped). `TabErrors.vue` destructures the filtered variant with an alias. Before https://github.com/user-attachments/assets/eb848c5f-d092-4a4f-b86f-d22bb4408003 After https://github.com/user-attachments/assets/75e67819-c9f2-45ec-9241-74023eca6120 ### 2. Bypass → un-bypass dropped url/hash metadata Realtime `scanNodeModelCandidates` only reads widget values, so un-bypass produced a fresh candidate without the url that `enrichWithEmbeddedMetadata` had previously attached from `graphData.models`. `MissingModelRow`'s download/copy-url buttons disappeared after a bypass/un-bypass cycle. Added `enrichCandidateFromNodeProperties` that copies `url`/`hash`/`directory` from the node's own `properties.models` — which persists across mode toggles — into each scanned candidate. Applied to every call site of the per-node scan. A later fix in the same branch also enforces directory agreement to prevent a same-name / different-directory collision from stamping the wrong metadata. Before https://github.com/user-attachments/assets/39039d83-4d55-41a9-9d01-dec40843741b After https://github.com/user-attachments/assets/047a603b-fb52-4320-886d-dfeed457d833 ### 3. Initial full scan surfaced interior errors of a muted/bypassed subgraph container `scanAllModelCandidates`, `scanAllMediaCandidates`, and the JSON-based missing-node scan only check each node's own mode. Interior nodes whose parent container was bypassed passed the filter. Added `isAncestorPathActive(rootGraph, executionId)` to `graphTraversalUtil` and post-filter the three pipelines in `app.ts` after the live rootGraph is configured. The filter uses the execution-ID path (`"65:63"` → check node 65's mode) so it handles both live-scan-produced and JSON-enrichment-produced candidates. Before https://github.com/user-attachments/assets/3032d46b-81cd-420e-ab8e-f58392267602 After https://github.com/user-attachments/assets/02a01931-951d-4a48-986c-06424044fbf8 ### 4. Bypassed subgraph entry re-surfaced interior errors `useGraphNodeManager` replays `graph.onNodeAdded` for each existing interior node when the Vue node manager initializes on subgraph entry. That chain reached `scanSingleNodeErrors` via `installErrorClearingHooks`' `onNodeAdded` override. Each interior node's own mode was active, so the caller guards passed and the scan re-introduced the error that the initial pipeline had correctly suppressed. Added an ancestor-activity gate at the top of `scanSingleNodeErrors`, the single entry point shared by paste, un-bypass, subgraph entry, and subgraph container activation. A later commit also hardens this guard against detached nodes (null execution ID → skip) and applies the same ancestor check to `isCandidateStillActive` in the realtime verification callback. Before https://github.com/user-attachments/assets/fe44862d-f1d6-41ed-982d-614a7e83d441 After https://github.com/user-attachments/assets/497a76ce-3caa-479f-9024-4cd0f7bd20a4 ## Tests - 6 unit tests for `isAncestorPathActive` (root, active, immediate-bypass, deep-nested mute, unresolvable ancestor, null rootGraph) - 4 unit tests for `enrichCandidateFromNodeProperties` (enrichment, no-overwrite, name mismatch, directory mismatch) - 1 unit test for `scanSingleNodeErrors` ancestor guard (subgraph entry replaying onNodeAdded) - 2 unit tests for `useErrorGroups` dual export + ErrorOverlay contract - 4 E2E tests: - ErrorOverlay model count stays constant when a node is selected (new fixture `missing_models_distinct.json`) - Bypass/un-bypass cycle preserves Copy URL button (uses `missing_models_from_node_properties`) - Loading a workflow with bypassed subgraph suppresses interior missing model error (new fixture `missing_models_in_bypassed_subgraph.json`) - Entering a bypassed subgraph does not resurface interior missing model error (shares the above fixture) `pnpm typecheck`, `pnpm lint`, 206 related unit tests passing. ## Follow-up Several items raised by code review are deferred as pre-existing tech debt or scope-avoided refactors. Tracked via comments on #11215 and #11216. --- Follows up on #10856.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import type {
|
||||
ComfyWorkflowJSON,
|
||||
ModelFile,
|
||||
NodeId
|
||||
} from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
import { flattenWorkflowNodes } from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
@@ -19,6 +20,7 @@ import type {
|
||||
IBaseWidget,
|
||||
IComboWidget
|
||||
} from '@/lib/litegraph/src/types/widgets'
|
||||
import { getParentExecutionIds } from '@/types/nodeIdentification'
|
||||
import {
|
||||
collectAllNodes,
|
||||
getExecutionIdByNode
|
||||
@@ -30,6 +32,39 @@ function isComboWidget(widget: IBaseWidget): widget is IComboWidget {
|
||||
return widget.type === 'combo'
|
||||
}
|
||||
|
||||
/**
|
||||
* Fills url/hash/directory onto a candidate from the node's embedded
|
||||
* `properties.models` metadata when the names match. The full pipeline
|
||||
* does this via enrichWithEmbeddedMetadata + graphData.models, but the
|
||||
* realtime single-node scan (paste, un-bypass) otherwise loses these
|
||||
* fields — making the Missing Model row's download/copy-url buttons
|
||||
* disappear after a bypass/un-bypass cycle.
|
||||
*/
|
||||
function enrichCandidateFromNodeProperties(
|
||||
candidate: MissingModelCandidate,
|
||||
embeddedModels: readonly ModelFile[] | undefined
|
||||
): MissingModelCandidate {
|
||||
if (!embeddedModels?.length) return candidate
|
||||
// Require directory agreement when the candidate already has one —
|
||||
// a single node can reference two models with the same name under
|
||||
// different directories (e.g. a LoRA present in multiple folders);
|
||||
// name-only matching would stamp the wrong url/hash onto the
|
||||
// candidate. Mirrors the directory check in enrichWithEmbeddedMetadata.
|
||||
const match = embeddedModels.find(
|
||||
(m) =>
|
||||
m.name === candidate.name &&
|
||||
(!candidate.directory || candidate.directory === m.directory)
|
||||
)
|
||||
if (!match) return candidate
|
||||
return {
|
||||
...candidate,
|
||||
directory: candidate.directory ?? match.directory,
|
||||
url: candidate.url ?? match.url,
|
||||
hash: candidate.hash ?? match.hash,
|
||||
hashType: candidate.hashType ?? match.hash_type
|
||||
}
|
||||
}
|
||||
|
||||
function isAssetWidget(widget: IBaseWidget): widget is IAssetWidget {
|
||||
return widget.type === 'asset'
|
||||
}
|
||||
@@ -107,6 +142,8 @@ export function scanNodeModelCandidates(
|
||||
if (!executionId) return []
|
||||
|
||||
const candidates: MissingModelCandidate[] = []
|
||||
const embeddedModels = (node as { properties?: { models?: ModelFile[] } })
|
||||
.properties?.models
|
||||
for (const widget of node.widgets) {
|
||||
let candidate: MissingModelCandidate | null = null
|
||||
|
||||
@@ -122,7 +159,11 @@ export function scanNodeModelCandidates(
|
||||
)
|
||||
}
|
||||
|
||||
if (candidate) candidates.push(candidate)
|
||||
if (candidate) {
|
||||
candidates.push(
|
||||
enrichCandidateFromNodeProperties(candidate, embeddedModels)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
return candidates
|
||||
@@ -231,9 +272,18 @@ export async function enrichWithEmbeddedMetadata(
|
||||
// model — not merely because any unrelated active node exists. A
|
||||
// reference is any widget value (or node.properties.models entry)
|
||||
// that matches the model name on an active node.
|
||||
// Hoist the id→node map once; isModelReferencedByActiveNode would
|
||||
// otherwise rebuild it on every unmatched entry.
|
||||
const flattenedNodeById = new Map(allNodes.map((n) => [String(n.id), n]))
|
||||
const activeUnmatched = unmatched.filter(
|
||||
(m) =>
|
||||
m.sourceNodeType !== '' || isModelReferencedByActiveNode(m.name, allNodes)
|
||||
m.sourceNodeType !== '' ||
|
||||
isModelReferencedByActiveNode(
|
||||
m.name,
|
||||
m.directory,
|
||||
allNodes,
|
||||
flattenedNodeById
|
||||
)
|
||||
)
|
||||
|
||||
const settled = await Promise.allSettled(
|
||||
@@ -276,7 +326,9 @@ export async function enrichWithEmbeddedMetadata(
|
||||
|
||||
function isModelReferencedByActiveNode(
|
||||
modelName: string,
|
||||
allNodes: ReturnType<typeof flattenWorkflowNodes>
|
||||
modelDirectory: string | undefined,
|
||||
allNodes: ReturnType<typeof flattenWorkflowNodes>,
|
||||
nodeById: Map<string, ReturnType<typeof flattenWorkflowNodes>[number]>
|
||||
): boolean {
|
||||
for (const node of allNodes) {
|
||||
if (
|
||||
@@ -284,12 +336,30 @@ function isModelReferencedByActiveNode(
|
||||
node.mode === LGraphEventMode.BYPASS
|
||||
)
|
||||
continue
|
||||
if (!isAncestorPathActiveInFlattened(String(node.id), nodeById)) continue
|
||||
|
||||
// Require directory agreement when both sides specify one, so a
|
||||
// same-name entry under a different folder does not keep an
|
||||
// unrelated workflow-level model alive as missing.
|
||||
const embeddedModels = (
|
||||
node.properties as { models?: Array<{ name: string }> } | undefined
|
||||
node.properties as
|
||||
| { models?: Array<{ name: string; directory?: string }> }
|
||||
| undefined
|
||||
)?.models
|
||||
if (embeddedModels?.some((m) => m.name === modelName)) return true
|
||||
if (
|
||||
embeddedModels?.some(
|
||||
(m) =>
|
||||
m.name === modelName &&
|
||||
(modelDirectory === undefined ||
|
||||
m.directory === undefined ||
|
||||
m.directory === modelDirectory)
|
||||
)
|
||||
) {
|
||||
return true
|
||||
}
|
||||
|
||||
// widgets_values carries only the name, so directory cannot be
|
||||
// checked here — fall back to filename matching.
|
||||
const values = node.widgets_values
|
||||
if (!values) continue
|
||||
const valueArray = Array.isArray(values) ? values : Object.values(values)
|
||||
@@ -300,6 +370,22 @@ function isModelReferencedByActiveNode(
|
||||
return false
|
||||
}
|
||||
|
||||
function isAncestorPathActiveInFlattened(
|
||||
executionId: string,
|
||||
nodeById: Map<string, ReturnType<typeof flattenWorkflowNodes>[number]>
|
||||
): boolean {
|
||||
for (const ancestorId of getParentExecutionIds(executionId)) {
|
||||
const ancestor = nodeById.get(ancestorId)
|
||||
if (!ancestor) continue
|
||||
if (
|
||||
ancestor.mode === LGraphEventMode.NEVER ||
|
||||
ancestor.mode === LGraphEventMode.BYPASS
|
||||
)
|
||||
return false
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
function collectEmbeddedModelsWithSource(
|
||||
allNodes: ReturnType<typeof flattenWorkflowNodes>,
|
||||
graphData: ComfyWorkflowJSON
|
||||
|
||||
Reference in New Issue
Block a user