Comfy-Org / Comfy-Org/ComfyUI_frontend

A configure() that throws still dispatches 'configured', so listeners cannot detect failed loads

Open
#15,755 0 comments 0 reactions 0 assignees View on GitHub
Potential Bug Public API
Dominant language
TypeScript
Stars
2k
Forks
704
Avg merge
1d 8h
Merged PRs (30d)
512

Description

`LGraph.configure()` dispatches `configured` in a `finally` block, so a configure that throws still fires the event. No listener (canvas, Vue layer, workflow store, extensions) can distinguish a failed load from a successful one, and at that point the graph is half-built.

```ts
} finally {
endNamedValuesShadowDiffLoad()
this.events.dispatch('configured')
}
```

Pre-existing, not a branch regression: the block is byte-identical on `main` `296fc5cd07` (`src/lib/litegraph/src/LGraph.ts:2734-2736`) and on `feature/ecs-migration` `f1bfb313d6` (`LGraph.ts:2836-2839`).

Why it matters more on the ECS branch: `configure` now registers entity state into external Pinia stores as it builds (`nodeDataStore` via `registerNodeState`, `linkStore`, `widgetValueStore`, `rerouteStore`, `previewExposureStore`, plus `layoutStore` via `attachNodeLayout`). The half-built state a `configured` listener observes after a throw is materially larger than on main, and `configure` is not transactional (best-effort rollback for the additive path only; recovery via the next good load is covered by `LGraph.interruptedConfigure.test.ts` on #15729, which pins this dispatch-on-throw behavior as characterisation rather than fixing it).

Decision needed before changing anything: `configuring`/`configured` are extension-facing (ADR 0008 lists entity callback changes as affecting 40+ custom node repos). Options:

1. Dispatch a distinct event on failure (e.g. `configure-failed`) and keep `configured` success-only. Breaking for any extension that relies on `configured` always firing.
2. Add a success/error field to the `configured` payload. Additive, non-breaking, but every listener must opt in to checking it.
3. Keep the behavior and document that `configured` means "configure finished", not "configure succeeded".

Routing to @DrJKL for the call since it constrains the extension-facing event contract.

Contributor guide

Open the contributing guide

Research direction

Start in src/lib/litegraph/src/LGraph.ts at LGraph.configure() and run the LGraph.interruptedConfigure.test.ts coverage referenced in #15729. Read ADR 0008 and inspect the extension-facing configuring/configured listeners before proposing the event contract. Done means the failure semantics are decided, documented, and covered by tests without leaving listeners unable to distinguish success from failure.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.