Comfy-Org / Comfy-Org/ComfyUI_frontend

chore: address missing-asset correctness edge cases and test gaps

Open
#11,216 1 comment 0 reactions 1 assignee Claimed by @jaeone94 View on GitHub
area:subgraph area:testing area:undo-redo
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

Follow-up from #10856.

## Background

Several correctness and coverage concerns surfaced during #10856 review that are independent of the architecture refactor tracked in #11215. Each item is self-contained and can be addressed sequentially.

## Items

### 1. Shared subgraph definition: execId ambiguity

`findPartialExecutionPathToGraph` (`src/utils/graphTraversalUtil.ts`) does a DFS and returns the first `node.subgraph === target` match. When the same subgraph definition is referenced by multiple container nodes (confirmed possible per `LGraph.ts:1090-1098`), interior-node execIds computed via this helper map to an arbitrary container.

This predates #10856 (`getExecutionIdByNode` has the same limitation), and the newly added `getExecutionIdForNodeInGraph` inherits it. Missing-asset errors keyed under one container's path may not be removed when the user edits via another container.

- Investigate real-world frequency of shared subgraph definitions
- If non-trivial: either disambiguate via a path hint (caller provides the container chain) or store per-instance candidates keyed by full container identity

### 2. Undo/redo: full pipeline cost

`changeTracker.updateState` runs `loadGraphData` with `silentAssetErrors: true`, which re-runs the entire `runMissingModelPipeline` + `runMissingMediaPipeline` on every undo/redo. For large workflows, this iterates all nodes/widgets and triggers async verification.

The per-workflow `PendingWarnings` cache is insufficient here because each undo snapshot represents a potentially different missing-asset set. Options:

- Side-cache per-snapshot scan results on the undo/redo queue entry (adds 'restorable-vs-currently-removed' tracking complexity)
- Debounce consecutive undo/redo operations and only scan on the final state

### 3. Missing-media in-subgraph E2E

E2E coverage for subgraph interior deletion exists for missing-model and missing-node-type (`errorsTabModeAware.spec.ts`), but not missing-media. Requires a new fixture (`missing_media_in_subgraph.json`) similar to `missing_models_in_subgraph.json`.

Unit tests cover the prefix removal path, so this is a coverage-completion task rather than a correctness fix.

### 4. Cloud-mode realtime verification E2E

The realtime pending-candidate verification added to `useErrorClearingHooks.ts` (`verifyAndAddPendingModels/Media`) has unit coverage only. The OSS E2E suite can't exercise the cloud-specific `isMissing: undefined` path.

Once cloud E2E harness is available (or a mocked cloud-mode E2E variant), add:
- Bypass → un-bypass cycle surfaces missing model in errors tab
- Bypass → un-bypass → bypass race: staleness guard drops the late verification result

### 5. Consolidate near-duplicate prefix tests across stores

Per test-quality review: `missingModelStore`, `missingMediaStore`, `missingNodesErrorStore` each have 6–7 near-identical tests for the new `removeMissing*ByPrefix` methods (basic, deep nesting, sibling colon guard, null transition, no-op). Only the interaction-state cleanup and string-entry preservation cases are genuinely unique per store.

Candidate trim: one full set on `missingModelStore`, minimal coverage on the others. Saves ~10 tests with no coverage loss.

### 6. Document subgraph interior node ID reassignment

E2E fixtures author interior nodes with specific IDs, but subgraph configure reassigns them on ID collisions with root-graph IDs — so `getNodeRefById(interiorId)` fails silently when run inside a subgraph. The workaround (`selectAll` + keyboard action) is noted as an inline comment in `errorsTabModeAware.spec.ts` but not documented anywhere else.

Promote to a formal rule in `browser_tests/AGENTS.md` or `docs/guidance/playwright.md` so future test authors don't rediscover the trap.

## Dependencies

Item 1 (shared subgraph execId) is technically orthogonal but benefits from the composable split in #11215 — easier to evolve the execId-producing helper once callers are localized.

## Acceptance criteria

Each item is independently landable. Close the issue once all six are resolved or explicitly deferred with a linked ticket.

┆Issue is synchronized with this [Notion page](https://www.notion.so/Issue-11216-chore-address-missing-asset-correctness-edge-cases-and-test-gaps-3426d73d3650816cb57cd4a913b676d8) by [Unito](https://www.unito.io)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.