Comfy-Org / Comfy-Org/ComfyUI_frontend

Save/load round-trip tests are vacuous by default: reloaded nodes re-adopt live widgetValueStore state instead of reading the JSON

Open
#15,604 1 comment 0 reactions 2 assignees Claimed by @DrJKL View on GitHub
area:testing Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Summary

Save/load tests written as `configure(...serialize())` are **vacuous by default** in this repo, and nothing warns you. The reloaded node re-adopts still-live store state instead of reading the JSON it just produced, so the assertion passes and the round trip is never exercised.

Measured at `9f6ab9adda` on `feature/ecs-migration`.

## Mechanism

- `widgetValueStore` is keyed `graphId:nodeId:name` (`src/stores/widgetValueStore.ts:110-117`).
- `LGraph.clear()` drops the store **for the graph's current id** — `resetAfterClear()` calls `useWidgetValueStore().clearGraph(graphId)` at `src/lib/litegraph/src/LGraph.ts:578`, using `this.id` *before* configure runs.
- `LGraph._configureBase()` then **re-adopts the payload's graph id** (`src/lib/litegraph/src/LGraph.ts:2566-2568`).

So `new LGraph().configure(sourceGraph.serialize())` ends up with **the same graph id as the source graph**, whose widget entries were never cleared. `registerWidget` finds the existing state and returns it, and the reloaded widget silently inherits the source's value/label. The serialized payload is never consulted.

Confirmed directly: in `LGraphNode.widgetLabelRename.regression.test.ts`, both graphs report the same id (`8016438e-…`) and the same `widgetId` (`8016438e-…:1:text`).

## Impact: the #13861 regression test does not guard #13861

`src/lib/litegraph/src/LGraphNode.widgetLabelRename.regression.test.ts` exists for regression #13861 ("a renamed widget label reverted to its default on save/reload, delete/undo, and copy/paste"). Its docblock says the tests "assert the label survives a real serialize -> configure round-trip".

**Reintroducing #13861's exact root cause** — deleting `if (input) input.label = label` from `renameWidget` (`src/utils/widgetUtil.ts:65`) — leaves **3 of its 5 tests green**:

| test | root-cause mutation |
|---|---|
| `renameWidget writes the label onto the normal-node backing input` | **killed** |
| `label survives a full graph serialize -> configure round-trip` | survives |
| `label survives delete -> undo` | survives |
| `clearing a rename reverts the label to its default after round-trip` | survives |
| `label survives copy -> paste` | **killed** (paste mints new node ids, so the store key differs and the JSON must be read) |

Adding a store-amnesia probe on top (clear `widgetValueStore` for the payload's graph id inside `_configureBase`) flips two of the three to failing, which pins the store as the mechanism rather than some other fallback.

## Impact: label persistence has almost no coverage repo-wide

Mutation: make `LGraphNode.serialize()` silently drop `label` and `localized_name` from every input.

- Across the **29 test files** in `src/` matching a round-trip shape: **1 of 922 tests killed** (`label survives copy -> paste`).
- Poisoning every node/group `title` in the payload: **1 of 922 killed** — and not `LGraph Serialisation > can (de)serialise node / group titles`, which asserts counts only and never asserts a title.

## Impact: `SubgraphNode` "round-trip" test reads no JSON at all

`src/lib/litegraph/src/subgraph/SubgraphNode.test.ts` → `should preserve renamed label through serialize/configure round-trip` survives **total payload blinding** (replacing the argument to `SubgraphNode.configure` with `{ id, type }`), and survives `serialize()` dropping every input label. The payload provably carries the label — dumped: `"inputs":[{"label":"My Seed",…}]` — but `SubgraphNode.configure` rebuilds `this.inputs` from the live `subgraph.inputNode.slots` and the assertions read live/store state. Killed only by a mutation that makes `configure` append instead of resetting inputs, i.e. it is an input-list-reset test wearing a round-trip name.

## What is *not* affected

The defect does **not** generalise to link topology, floating links, or reroutes. Dropping `links` / `floatingLinks` / `reroutes` from the payload inside `LGraph.configure` kills **21 of 922** tests across `LGraph.test.ts`, `LGraph.serialise.test.ts`, `SubgraphSerialization.test.ts`, `SubgraphWidgetPromotion.test.ts`. Those stores are rebuilt from per-graph instance collections and the tests use fresh scopes, so they read the JSON. Subgraph IO round trips are also real (2 kills under an inputs/outputs blinding mutation), as are `widgets_values` round trips (14 kills under a `widgets_values` poison).

**The vulnerable shape is specifically: a field whose only home is a store keyed by the graph id, asserted after a reload that re-adopts that graph id.** Widget `value` mostly escapes because most tests either clear the store explicitly or mint a new node id. Widget `label` does not escape.

## Proposed fix

Test-side, in https://github.com/Comfy-Org/ComfyUI_frontend/pull/15603:

1. A `reloadSerializedGraph()` helper in `src/utils/__tests__/litegraphTestUtils.ts` that forces the payload through `JSON.parse(JSON.stringify(...))` and drops the widget/preview stores for the **payload's** graph id before `configure()`.
2. The three reload tests in the #13861 regression file switched to it; the negative-assertion test given a live-rename control arm.
3. `SubgraphNode`'s test renamed to what it asserts, with a real definition-reload test added beside it.

After: the #13861 root-cause mutation kills **5 of 5** (was 2 of 5); the input-label serialization mutation kills **3 of 5** (was 1 of 5).

## Open question, not fixed here

Should `LGraph.configure` clear the widget store for the **incoming** graph id as well as the outgoing one? A store-amnesia probe applied at `_configureBase` across all 29 files killed **0 of 922** tests, so production rehydration is correct today and the change would be behaviour-preserving for these paths — but it would also make the whole class of test un-writable-wrong. Worth a decision rather than leaving it implicit.

## Reproduction

```
git checkout 9f6ab9adda
# root-cause mutation
sed -i 's/ if (input) input.label = label/ if (false \&\& input) input.label = label/' src/utils/widgetUtil.ts
npx vitest run src/lib/litegraph/src/LGraphNode.widgetLabelRename.regression.test.ts
# 2 failed | 3 passed
```

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.