Comfy-Org / Comfy-Org/ComfyUI_frontend

Migrate hydration transaction pattern to ECS SerializationSystem

Open
#10,529 0 comments 0 reactions 1 assignee Claimed by @kaili-yang View on GitHub
area:widgets developer experience enhancement
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Context

PR #10201 adds a hydration transaction system to `widgetValueStore` (`beginHydration`/`commitHydration`/`isHydrating`/`onHydrationComplete`) to fix widget restoration ordering during paste. The fix works and the **transaction primitives are ECS-aligned**, but the integration seams are not.

This issue tracks migrating those seams when the ECS systems layer from ADR #10420 lands.

## What's ECS-Aligned (Keep)

- **Hydration transaction primitives** in `widgetValueStore.ts` — `beginHydration()`, `commitHydration()`, `isHydrating()` are structurally identical to what a `HydrationSystem` would do: batch component writes, defer derived-state reactions until coherent
- **`onHydrationComplete()` callback deferral** — correct concept (suppress `updateCombo` until all widget values restored), will become a system-driven reconciliation pass
- **Node-scoped, idempotent transactions** — the `try/finally` pattern with `Set` is the right granularity

## What Needs Migration (Known Debt)

### 1. `getActivePinia()` guard in `LGraphNode.configure()`
**File:** `src/lib/litegraph/src/LGraphNode.ts` ~line 901

```ts
const store = getActivePinia() ? useWidgetValueStore() : null
store?.beginHydration(this.id)
```

**Problem:** Domain object (`LGraphNode`) reaches into Vue/Pinia runtime. The ECS ADR explicitly identifies "module-scope store access from domain objects" as an anti-pattern that makes domain objects untestable without a Vue app context.

**Migration:** When `SerializationSystem` exists, it owns deserialization and wraps `configure()` externally:
```ts
// SerializationSystem.deserializeNode()
store.beginHydration(nodeId)
try { node.configure(info) }
finally { store.commitHydration(nodeId) }
```
`LGraphNode` becomes Pinia-unaware.

### 2. `useChainCallback(this.onConfigure, ...)` in `customWidgets.ts`
**File:** `src/extensions/core/customWidgets.ts` ~line 132

```ts
this.onConfigure = useChainCallback(
this.onConfigure,
function (this: LGraphNode) {
useWidgetValueStore().onHydrationComplete(this.id, updateCombo)
}
)
```

**Problem:** Adds new callback chaining where the ADR says behavior should move to systems, not deepen "Change Notification Sprawl." Each widget type that needs post-hydration reconciliation adds another chained callback — this does not scale.

**Migration:** Post-hydration reconciliation becomes a system responsibility:
- `SerializationSystem` or `HydrationSystem` runs reconcilers after `commitHydration()`
- Widget types register reconciliation needs declaratively (e.g., `CustomCombo` needs `updateCombo` after hydration)
- No `onConfigure` mutation needed

### 3. `onHydrationComplete` callback registration pattern
**File:** `src/stores/widgetValueStore.ts` ~line 122

**Problem:** `widgetValueStore` is becoming store + event bus + orchestration layer. The callback queue (`hydrationCallbacks`) is system-level orchestration living inside a component store.

**Migration:** The callback queue moves to `HydrationSystem`. `widgetValueStore` retains only the `isHydrating` flag (or that also moves to a `HydrationState` component).

## When to Do This

- **Blocked on:** ADR #10420 being accepted and the systems layer being scaffolded
- **Trigger:** When `SerializationSystem` or equivalent exists and owns `node.configure()` call sites
- **Scope:** ~2-3 hours — delete 2 integration seams, move wrapping to system layer
- **Risk:** Low — the transaction primitives migrate directly; only attachment points change

## References

- PR #10201 — hydration transaction fix
- PR #10420 — ECS ADR (the architectural target)
- `proto-ecs-stores.md` in ADR #10420 — identifies `WidgetValueStore` as "closest to true ECS component store"
- `entity-problems.md` in ADR #10420 — documents "Module-scope store access" and "Change Notification Sprawl" anti-patterns

┆Issue is synchronized with this [Notion page](https://www.notion.so/Issue-10529-Migrate-hydration-transaction-pattern-to-ECS-SerializationSystem-32e6d73d365081e3aa4ffc27beb5fdff) by [Unito](https://www.unito.io)

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.