Comfy-Org / Comfy-Org/ComfyUI_frontend

refactor: split useErrorClearingHooks and consolidate missing-asset APIs

Open
#11,215 1 comment 0 reactions 1 assignee Claimed by @jaeone94 View on GitHub
developer experience
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

Follow-up from #10856.

## Background

Work on #10856 added substantial functionality to the missing-asset system. Reviewers and internal analysis flagged several architecture/API concerns worth addressing in a focused cleanup PR rather than piling onto the original fix. Tracking them together since they touch the same area and benefit from a single coherent refactor.

> **Status update (2026-05-04):** Item 3 has been partially addressed (the `activeUnmatched` filter is in place); see the note inline. All other items remain actionable on `main`.

## Items

### 1. Split `useErrorClearingHooks.ts` by responsibility

`src/composables/graph/useErrorClearingHooks.ts` is 416 lines and coordinates clearing, surfacing, async verification, and lifecycle — well beyond what the file name suggests. Architecture review during #10856 flagged it as a 'god composable in the making'.

Proposed split:
- `useErrorClearingHooks.ts` — keep the original scope: `onConnectionsChange` / `onWidgetChanged` wrapping only
- `useMissingAssetSurfacing.ts` — `scanSingleNodeErrors`, `verifyAndAddPending*` helpers, pending/confirmed candidate routing
- `useNodeLifecycleErrors.ts` — `onNodeAdded` / `onNodeRemoved` hooks and `handleNodeModeChange`

`GraphCanvas.vue` composes the three. Each composable becomes independently testable without the full graph-callback scaffolding.

### 2. Consolidate missing-asset store removal APIs

Each of `missingModelStore`, `missingMediaStore`, `missingNodesErrorStore` now exposes 3–5 overlapping removal methods (`removeMissing*ByName` / `ByWidget` / `ByNodeId` / `ByPrefix`). They share identical plumbing: filter → recompute `removedNames` → `clearInteractionStateForName` → null-if-empty.

Proposal: collapse to a single `removeMissing*Where(predicate)` (or shared internal helper with thin named wrappers preserving readable call sites). Eliminates the risk that future `ByX` methods forget the interaction-state cleanup invariant.

### 3. Revisit the workflow-level `graphData.models` → new-candidate path

`scanAllModelCandidates` is now always called in `runMissingModelPipeline` (both OSS and cloud). In that world, the "unmatched workflow-level entry creates a new candidate" path in `enrichWithEmbeddedMetadata` mostly surfaces stale metadata rather than legitimate missing-model notifications.

> **Partial progress:** `enrichWithEmbeddedMetadata` already filters via `activeUnmatched` (`src/platform/missingModel/missingModelScan.ts:284-293`), keeping workflow-level entries (`sourceNodeType === ''`) only when `isModelReferencedByActiveNode` confirms an active reference. The investigation/removal of the new-candidate path itself is still open.

Remaining work:
- Investigate whether the 5 `missingModelScan.test.ts` 'OSS dialog path' tests still map to a real production flow
- If not, remove the new-candidate path entirely; keep the enrichment path (url/hash/directory)

### 4. Enforce the trailing-colon convention on prefix removal APIs

`removeMissing*ByPrefix(prefix)` relies on callers passing `${execId}:`. Current protection is JSDoc + a single call site. Options:

- Rename to `removeMissing*BelowContainer(containerExecId)` and prepend `:` internally; callers cannot get it wrong
- Or add a runtime assertion that prefix ends with `:`

The first option is cleaner but renames the public shape.

### 5. Extract `useErrorsTabVisibility` from `RightSidePanel.vue`

Already mentioned as follow-up in #10856 body. Error-detection computed properties are scattered across `RightSidePanel.vue` — extract to a composable for testability and reuse.

## Acceptance criteria

- Public behavior unchanged; all existing unit + E2E tests pass
- Each new composable / refactored store has focused unit tests
- Call-site diff is mechanical (composable imports rearranged, store method names updated) — no logic drift

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.