Comfy-Org / Comfy-Org/ComfyUI_frontend

A throwing extension handler during delete permanently disables undo and workflow persistence for the session

Open
#17,438 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
2k
Forks
702
Avg merge
1d 8h
Merged PRs (30d)
512

Description

### What happened?

`LGraphCanvas.deleteSelected()` emits its `before-change` / `after-change` transaction pair without a `try`/`finally`. If anything throws between them, the closing event never fires and `ChangeTracker.changeCount` stays above zero permanently.

`ChangeTracker.captureCanvasState()` returns early whenever `changeCount > 0`, silently. Because every persistence path is downstream of that one function, a single exception during a delete disables all of the following for the rest of the browser session:

- undo/redo recording
- the `graphChanged` event, and therefore localStorage draft persistence
- autosave, and any manual save, which write `activeState` rather than the live graph
- `deactivate()` flushing canvas state before a workflow tab switch

The user-visible result is silent data loss. Undo appears to do nothing, or jumps back to an old state. Switching workflow tabs and returning restores a stale graph. A later save then writes that stale graph over the file on disk. The only signal is the original error toast, which looks unrelated and is easy to dismiss.

I lost a substantial amount of work twice before finding this.

### Steps to Reproduce

1. Arm a counter mirroring the internal one:

```js
window.__cc = 0; document.addEventListener('litegraph:canvas', e => { const s = e.detail?.subType; if (s === 'before-change') window.__cc++; else if (s === 'after-change') window.__cc--; }); 'armed'
```

2. Trigger any extension handler that throws during node removal. A readily available one: connect a node to `input1` of a wildcard-typed `Switch (Any)` from ComfyUI-Impact-Pack, then delete the connected node.
3. Observe the `TypeError` in the console.
4. Check `__cc`. It rests above zero and never returns.
5. From this point, undo records nothing and the tab-switch round trip reverts work.

### Observed stack

```
TypeError: target_node.findInputSlot is not a function
at ComfyNode.connect
at nodeType.onConnectionsChange (impact-pack.js:639)
at ComfyNode.onConnectionsChange
at ComfyNode.disconnectInput
at LGraph.remove
at LGraphCanvas.deleteSelected
```

### Secondary issue in the same trace

The specific throw above is also worth a look, and may deserve splitting out.

`LGraphNode.connect()` supports a legacy form where the target is a node id, guarded by `typeof target_node === 'number'`. `NodeId` is now `string & { __brand }` and `LGraphNode.id` is always a string, so that branch is unreachable for `node.id`. A string id falls through unresolved and is then used as a node, producing the confusing `findInputSlot` error rather than resolving or failing clearly.

This silently breaks every extension still using the previously valid `node.connect(slot, otherNode.id, name)` pattern. Either accept strings in that branch or throw a clear error. I have submitted the corresponding caller fix as ltdrdata/ComfyUI-Impact-Pack#1233, but the API-level gap will keep surfacing elsewhere.

### Suggested fix

Wrap the emit pair in `deleteSelected()` in `try`/`finally`. The same unprotected shape exists in `_pasteFromClipboard()` and in the ghost placement flow, where `finalizeGhostPlacement()` has two early returns before its matching `after-change`.

A defensive measure at the `ChangeTracker` level would also help, since the counter design assumes balanced pairs across all extension code. Never trusting a raised counter indefinitely, or resetting it on graph load, would stop any future instance of this from being silent and unrecoverable.

### Workflow

Not attached. The graph contents are not relevant and any graph reproduces it. The minimal case is two nodes: any node feeding `input1` of a wildcard `Switch (Any)`, then deleting the upstream node.

### How is this affecting you?

Silent loss of unsaved work, with the saved file subsequently overwritten by stale state.

### ComfyUI Frontend Version

1.49.6, with ComfyUI 0.34.0.

### Browser

Chrome (Chromium-based), Windows.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Contributor guide

Open the contributing guide

Research direction

Start in LGraphCanvas.deleteSelected() and trace the before-change/after-change transaction handling through ChangeTracker.captureCanvasState(). Then inspect _pasteFromClipboard() and finalizeGhostPlacement() for the same pattern. Done means an exception or early return cannot leave the change counter elevated, so undo and workflow persistence resume afterward; the separate string NodeId issue may need its own scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.