Comfy-Org / Comfy-Org/ComfyUI_frontend
A throwing extension handler during delete permanently disables undo and workflow persistence for the session
- 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
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