Comfy-Org / Comfy-Org/ComfyUI_frontend

Failed node-template load returns an empty list that is then written over the user's saved templates

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

Description

## Summary

`ManageTemplates.load()` in `src/extensions/core/nodeTemplates.ts` cannot tell "you have no saved templates" from "I could not read your saved templates", and the empty list it returns on failure is then written back over the user's template file. A single unreadable response destroys the user's entire saved node-template collection.

## Mechanism

`load()` swallows a `res.json()` failure and returns `[]`:

```ts
const res = await api.getUserData(file)
if (res.status === 200) {
try {
templates = await res.json()
} catch (error) {
reportError(error, { /* ... */ }) // reports, but still falls through
}
}
return templates ?? []
```

The constructor assigns that straight to instance state:

```ts
this.load().then((v) => {
this.templates = v
})
```

and every mutation path calls `store()`, which serializes `this.templates` wholesale:

```ts
async store() {
const templates = JSON.stringify(this.templates, undefined, 4)
await api.storeUserData(file, templates, { stringify: false })
}
```

So after a failed load, the next `store()` replaces the user's file with the empty list plus whatever was just added. Every entry point does this: saving a selection as a template (`manage.templates.push(...); manage.store()`), `importAll()`, rename, delete, and drag-reorder.

## Impact

Silent, total loss of saved node templates. The user sees an empty template list — which reads as "I have no templates yet", not as an error — and the very next save makes the loss permanent.

The load failure does not require the stored file to be corrupt. A truncated response body is enough to fail `res.json()` while the file on the server is still intact, at which point we overwrite a good file with an empty one.

## Repro

1. Save two or three node templates.
2. Make `getUserData` for the templates file return a 200 with a body that is not valid JSON (truncate it, or stub the response).
3. Open Manage Node Templates — the list is empty.
4. Save any new template.
5. Reload with the response restored to normal. The original templates are gone; only the one saved in step 4 remains.

## Suggested fix

Make the failure explicit rather than an empty result, so a read failure cannot be laundered into a write. Return a discriminated result from `load()` (loaded vs unreadable), keep the dialog in a distinct "could not load templates" state on failure, and have `store()` refuse to write while the last load is known to have failed. Offer the user an explicit recovery action rather than silently continuing.

Whatever the shape, the invariant worth testing is: a failed load must never lead to a `storeUserData` call that shrinks the persisted set.

## Notes

Found while reviewing #16797, which added the `reportError()` call in the `catch` above. That PR makes the failure visible in telemetry but deliberately does not change the recovery behavior — this issue is the recovery behavior.

Contributor guide

Open the contributing guide

Research direction

Start in src/extensions/core/nodeTemplates.ts and trace the constructor's load assignment, each mutation path, and store(); inspect the getUserData and storeUserData entry points plus the dialog state described in the issue. Done means a failed JSON read leaves an explicit recovery state, store() does not write, and regression coverage verifies that persisted templates are not silently replaced.

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
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.