Skip to content

Commit b1468ac

Browse files
mattmilleraiampagent
authored andcommitted
fix: clear widget-order and preview-exposure entries on node teardown (#14276)
**STACKED — merging lands on `matt/be-5050-node-shell-state` (owned by @mattmillerai), NOT `main`.** That branch is itself based on `feature/ecs-migration`, where `src/core/graph/nodeShell/` lives. Do not merge before its parent lands; GitHub retargets this PR as the stack unwinds. ## ELI-5 When a node joins a graph we write it into three places: its shell state, its widget bindings, and its widget ordering (plus preview exposures accumulate against the same bucket). When a node left, we only erased the first one. The leftovers piled up in a bucket shared by the whole workflow and stayed there until the entire graph was thrown away. This adds the missing "undo" for the other two and points every teardown path at it. ## Summary `attachNodeToStores` registers three things; every teardown path only undid one, so widget-order and preview-exposure entries orphaned in the shared root-graph bucket. This adds `detachNodeFromStores` as its exact inverse and routes the teardown paths through it. ## Changes - **What**: New `detachNodeFromStores(graph, node, mode)` in `nodeShellLifecycle.ts` — unregisters the shell state, removes each `getWidgetIds(node.widgets)` entry from the widget order, and clears the node's preview exposures (`String(node.id)` host locator, the same key `promotionUtils`/`SubgraphNode` write under). `unregisterAllNodeStates` moves out of `nodeShellState.ts` and becomes `detachAllNodesFromStores` in the same module — it needs the root graph to reach those buckets, so it now takes the graph (which carries `rootGraph`) rather than just `_nodes`/`_subgraphs`. Call sites: `LGraph.remove` (both the per-node teardown and the released-subgraph-definition walk) and `releaseGraphStores`'s non-root branch. - **Breaking**: none — `unregisterAllNodeStates` had exactly one external caller (`LGraph.ts`), and no behavior the extension surface can observe changes. ## Reachability the leak had (all three are load-bearing, not theoretical) - Subgraphs always take the non-root `else` branch of `releaseGraphStores`, and `LGraph.configure` calls `clear()` first — so re-configuring a subgraph orphaned its nodes' entries. - `clear()` sets `id = zeroUuid` on exit, so a root graph whose serialized data carries no id keeps taking the `else` branch for the rest of the session. - Plain `LGraph.remove(node)` leaked per-node on every branch, since it never removed the widget order it added. ## Review Focus **The judgment call the ticket left open — `deleteWidget` vs `removeNodeWidgetOrder` — is decided per call site, and both are covered by tests.** `LGraph.remove(node)` drops only the ordering and **keeps** the stored widget values: a single node removal feeds undo/redo and a node moved between graphs (`remove` then `add`) must not lose its values. Graph and subgraph-definition teardown (`detachAllNodesFromStores`) **discards** them, which is what the root branch already does one line up — `clearGraph` on `widgetValueStore` wipes that graph's widget states wholesale, so the non-root path was the inconsistent one. **Riskiest line: `deleteWidget` in the discard path**, because it is the one edit that removes data rather than an index. It is safe because `deleteWidget` only drops the store's map entry — the widget instance's `_state` object is untouched, so a widget that survives the clear still reads its own value, and re-registration (`setNodeId` → `registerWidget`) writes it back from the widget/serialized data rather than from store leftovers. Without this, a re-configure would silently reuse the *stale* store value: `registerWidget` returns the existing state when the type matches and ignores the incoming `init.value`. **Deliberately not routed through the new helper:** `useNodeReplacement.replaceWithMapping`. It calls `unregisterNodeState`/`registerNodeState` for an in-place swap where the new node inherits the same id and re-registers the same widgets — clearing exposures there would drop a replaced host node's promoted previews. It is a replacement, not a teardown. **Deviation from the ticket's sketch:** preview exposures are cleared with `setExposures(rootGraphId, locator, [])` (guarded by a `getExposures(...).length` read) rather than a `removeExposure` per name. Same result, one write instead of N, and the guard keeps the reactive `exposures` ref from being touched on every ordinary node deletion. ## Verification New `src/core/graph/nodeShell/nodeShellLifecycle.test.ts` covers all three: widget order dropped and values kept on `subgraph.remove(node)`; exposures dropped when a host `SubgraphNode` is removed; order, values and exposures all released when a zero-uuid root graph is cleared. **All three fail on the base branch** (verified by stashing the source change and re-running — `expected [ Array(1) ] to deeply equal []`). `vitest run src/lib/litegraph src/core/graph src/platform/nodeReplacement src/stores/widgetValueStore.test.ts src/composables/node` — 1591 passed. `pnpm typecheck`, `pnpm knip`, eslint and oxfmt on the changed files: clean. Amp-Thread-ID: https://ampcode.com/threads/T-01a027af-1c32-7532-84f9-b94aa90620fa
1 parent 50e6c4c commit b1468ac

5 files changed

Lines changed: 195 additions & 26 deletions

File tree

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
import { createTestingPinia } from '@pinia/testing'
2+
import { setActivePinia } from 'pinia'
3+
import { beforeEach, describe, expect, it } from 'vitest'
4+
5+
import { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph'
6+
import type { Subgraph } from '@/lib/litegraph/src/litegraph'
7+
import {
8+
createTestRootGraph,
9+
createTestSubgraph,
10+
createTestSubgraphNode
11+
} from '@/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers'
12+
import { getWidgetIds } from '@/lib/litegraph/src/utils/widget'
13+
import { usePreviewExposureStore } from '@/stores/previewExposureStore'
14+
import { useWidgetValueStore } from '@/stores/widgetValueStore'
15+
16+
describe('node shell teardown', () => {
17+
beforeEach(() => {
18+
setActivePinia(createTestingPinia({ stubActions: false }))
19+
})
20+
21+
function addWidgetedNode(graph: LGraph | Subgraph): LGraphNode {
22+
const node = new LGraphNode('Node')
23+
node.addWidget('text', 'prompt', 'a value', () => {})
24+
graph.add(node)
25+
return node
26+
}
27+
28+
function widgetIdsOf(node: LGraphNode) {
29+
return getWidgetIds(node.widgets ?? [])
30+
}
31+
32+
it('drops the widget order a removed node registered, keeping its values', () => {
33+
const subgraph = createTestSubgraph()
34+
const rootGraphId = subgraph.rootGraph.id
35+
const node = addWidgetedNode(subgraph)
36+
const [widgetId] = widgetIdsOf(node)
37+
const widgetValueStore = useWidgetValueStore()
38+
39+
expect(widgetValueStore.getNodeWidgetIds(rootGraphId, node.id)).toEqual([
40+
widgetId
41+
])
42+
43+
subgraph.remove(node)
44+
45+
expect(widgetValueStore.getNodeWidgetIds(rootGraphId, node.id)).toEqual([])
46+
expect(widgetValueStore.getWidget(widgetId)?.value).toBe('a value')
47+
})
48+
49+
it('drops the preview exposures a removed host node owned', () => {
50+
const rootGraph = createTestRootGraph()
51+
const subgraph = createTestSubgraph({ rootGraph })
52+
const hostNode = createTestSubgraphNode(subgraph)
53+
rootGraph.add(hostNode)
54+
const hostLocator = String(hostNode.id)
55+
const previewExposureStore = usePreviewExposureStore()
56+
previewExposureStore.addExposure(rootGraph.id, hostLocator, {
57+
sourceNodeId: 1,
58+
sourcePreviewName: 'images'
59+
})
60+
61+
rootGraph.remove(hostNode)
62+
63+
expect(
64+
previewExposureStore.getExposures(rootGraph.id, hostLocator)
65+
).toEqual([])
66+
})
67+
68+
it('releases widget and exposure entries when a root graph is cleared', () => {
69+
const graph = new LGraph()
70+
const graphId = graph.id
71+
72+
const node = addWidgetedNode(graph)
73+
const [widgetId] = widgetIdsOf(node)
74+
const hostLocator = String(node.id)
75+
const previewExposureStore = usePreviewExposureStore()
76+
previewExposureStore.addExposure(graphId, hostLocator, {
77+
sourceNodeId: 1,
78+
sourcePreviewName: 'images'
79+
})
80+
const widgetValueStore = useWidgetValueStore()
81+
82+
graph.clear()
83+
84+
expect(widgetValueStore.getNodeWidgetIds(graphId, node.id)).toEqual([])
85+
expect(widgetValueStore.getWidget(widgetId)).toBeUndefined()
86+
expect(previewExposureStore.getExposures(graphId, hostLocator)).toEqual([])
87+
})
88+
89+
it('releases entries of a widget the node dropped without unregistering', () => {
90+
const graph = new LGraph()
91+
const graphId = graph.id
92+
const node = addWidgetedNode(graph)
93+
const [widgetId] = widgetIdsOf(node)
94+
const widgetValueStore = useWidgetValueStore()
95+
node.widgets = []
96+
97+
graph.clear()
98+
99+
expect(widgetValueStore.getNodeWidgetIds(graphId, node.id)).toEqual([])
100+
expect(widgetValueStore.getWidget(widgetId)).toBeUndefined()
101+
})
102+
})

src/core/graph/nodeShell/nodeShellLifecycle.ts

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
import { isNodeBindable } from '@/lib/litegraph/src/utils/type'
22
import { getWidgetIds } from '@/lib/litegraph/src/utils/widget'
3+
import { usePreviewExposureStore } from '@/stores/previewExposureStore'
34
import { useWidgetValueStore } from '@/stores/widgetValueStore'
45

5-
import { registerNodeState } from './nodeShellState'
6+
import { registerNodeState, unregisterNodeState } from './nodeShellState'
67

78
import type { LGraph } from '@/lib/litegraph/src/LGraph'
89
import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
910
import type { NodeId } from '@/types/nodeId'
1011
import type { Subgraph } from '@/lib/litegraph/src/subgraph/Subgraph'
12+
import type { UUID } from '@/utils/uuid'
1113

1214
/**
1315
* Registers a node's shell state and its widget bindings with the app
@@ -31,3 +33,57 @@ export function attachNodeToStores(
3133
getWidgetIds(node.widgets)
3234
)
3335
}
36+
37+
/**
38+
* Whether a detached node's widget values leave the store with it. A node that
39+
* may come back — undo of a deletion — keeps its values and drops only its
40+
* ordering; a node whose whole graph is going away takes its values along.
41+
*/
42+
type WidgetDetachMode = 'keep-values' | 'discard-values'
43+
44+
function releaseNodePreviewExposures(
45+
rootGraphId: UUID,
46+
node: LGraphNode
47+
): void {
48+
const previewExposureStore = usePreviewExposureStore()
49+
const hostNodeLocator = String(node.id)
50+
if (!previewExposureStore.getExposures(rootGraphId, hostNodeLocator).length) {
51+
return
52+
}
53+
previewExposureStore.setExposures(rootGraphId, hostNodeLocator, [])
54+
}
55+
56+
/**
57+
* The inverse of {@link attachNodeToStores}: drops the node's shell state, the
58+
* widget order it registered, and the preview exposures it hosts.
59+
*/
60+
export function detachNodeFromStores(
61+
graph: Pick<LGraph, 'rootGraph'>,
62+
node: LGraphNode,
63+
mode: WidgetDetachMode = 'keep-values'
64+
): void {
65+
const rootGraphId = graph.rootGraph.id
66+
unregisterNodeState(node)
67+
useWidgetValueStore().releaseNodeWidgets(rootGraphId, node.id, {
68+
discardValues: mode === 'discard-values'
69+
})
70+
releaseNodePreviewExposures(rootGraphId, node)
71+
}
72+
73+
/**
74+
* Detaches every node a graph owns, including those inside the subgraph
75+
* definitions it holds. Used when a graph's nodes leave the stores without a
76+
* whole-bucket wipe: subgraph-definition removal, and clearing a graph that
77+
* shares its bucket with other graphs. The graph is going away, so its nodes'
78+
* widget values go with it — the same reach as the wipe a root graph performs.
79+
*/
80+
export function detachAllNodesFromStores(
81+
graph: Pick<LGraph, '_nodes' | '_subgraphs' | 'rootGraph'>
82+
): void {
83+
for (const node of graph._nodes) {
84+
detachNodeFromStores(graph, node, 'discard-values')
85+
}
86+
for (const subgraph of graph._subgraphs.values()) {
87+
detachAllNodesFromStores(subgraph)
88+
}
89+
}

src/core/graph/nodeShell/nodeShellState.ts

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -110,22 +110,6 @@ export function unregisterNodeState(node: LGraphNode): void {
110110
)
111111
}
112112

113-
/**
114-
* Unregisters every node a graph owns, including those inside the subgraph
115-
* definitions it holds. Used when a graph's nodes leave the store without a
116-
* whole-bucket wipe: subgraph-definition removal, and clearing a graph that
117-
* shares its bucket with other graphs.
118-
* @param graph The graph whose nodes should be unregistered
119-
*/
120-
export function unregisterAllNodeStates(
121-
graph: Pick<LGraph, '_nodes' | '_subgraphs'>
122-
): void {
123-
for (const node of graph._nodes) unregisterNodeState(node)
124-
for (const subgraph of graph._subgraphs.values()) {
125-
unregisterAllNodeStates(subgraph)
126-
}
127-
}
128-
129113
function canTransferNodeState(
130114
node: LGraphNode,
131115
replacement: LGraphNode

src/lib/litegraph/src/LGraph.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@ import {
55
SUBGRAPH_INPUT_ID,
66
SUBGRAPH_OUTPUT_ID
77
} from '@/lib/litegraph/src/constants'
8-
import { attachNodeToStores } from '@/core/graph/nodeShell/nodeShellLifecycle'
98
import {
10-
unregisterAllNodeStates,
11-
unregisterNodeState
12-
} from '@/core/graph/nodeShell/nodeShellState'
9+
attachNodeToStores,
10+
detachAllNodesFromStores,
11+
detachNodeFromStores
12+
} from '@/core/graph/nodeShell/nodeShellLifecycle'
1313
import type { UUID } from '@/utils/uuid'
1414
import { createUuidv4, zeroUuid } from '@/utils/uuid'
1515
import {
@@ -24,7 +24,9 @@ import {
2424
import { layoutStore } from '@/renderer/core/layout/store/layoutStore'
2525
import { useLinkStore } from '@/stores/linkStore'
2626
import { useNodeDataStore } from '@/stores/nodeDataStore'
27+
import { usePreviewExposureStore } from '@/stores/previewExposureStore'
2728
import { useRerouteStore } from '@/stores/rerouteStore'
29+
import { useWidgetValueStore } from '@/stores/widgetValueStore'
2830
import { toLinkId } from '@/types/linkId'
2931
import { isFloatingTopology } from '@/types/linkTopology'
3032
import { toRerouteId } from '@/types/rerouteId'
@@ -48,8 +50,6 @@ import {
4850
outputLinks
4951
} from './node/slotLinks'
5052
import { normalizeWidgetsView } from './node/widgetsView'
51-
import { usePreviewExposureStore } from '@/stores/previewExposureStore'
52-
import { useWidgetValueStore } from '@/stores/widgetValueStore'
5353
import { UNASSIGNED_NODE_ID, parseNodeId, toNodeId } from '@/types/nodeId'
5454
import type { NodeId, SerializedNodeId } from '@/types/nodeId'
5555
import { forEachNode, visitGraphNodes } from '@/utils/graphTraversalUtil'
@@ -251,7 +251,7 @@ function teardownOwnedGraphs(owner: LGraph): void {
251251
for (const node of graph._nodes) nodes.add(node)
252252
}
253253
for (const node of nodes) {
254-
unregisterNodeState(node)
254+
detachNodeFromStores(owner, node, 'discard-values')
255255
node.graph = null
256256
}
257257
detachGraphLayouts([owner], { removeLayouts: !owner.isRootGraph })
@@ -1284,7 +1284,7 @@ export class LGraph
12841284
for (const subgraph of releasedSubgraphs) {
12851285
unregisterAllLinkTopologies(subgraph)
12861286
unregisterAllRerouteChains(subgraph)
1287-
unregisterAllNodeStates(subgraph)
1287+
detachAllNodesFromStores(subgraph)
12881288
this.rootGraph.subgraphs.delete(subgraph.id)
12891289
}
12901290
detachGraphLayouts(releasedSubgraphs)
@@ -1293,7 +1293,7 @@ export class LGraph
12931293
// callback
12941294
node.onRemoved?.()
12951295

1296-
unregisterNodeState(node)
1296+
detachNodeFromStores(this, node)
12971297
detachNodeLayout(node)
12981298

12991299
node.graph = null

src/stores/widgetValueStore.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,32 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
240240
}
241241
}
242242

243+
/**
244+
* Releases the widget ids tracked for a node, from the store's own record
245+
* rather than the node's live widget list — the two diverge once a node drops
246+
* widgets without unregistering them. `discardValues` also drops the widget
247+
* states; retaining them lets a node that comes back keep what the user set.
248+
*/
249+
function releaseNodeWidgets(
250+
graphId: UUID,
251+
localNodeId: NodeId,
252+
{ discardValues }: { discardValues: boolean }
253+
): void {
254+
const graphOrders = graphNodeWidgetOrders.value.get(graphId)
255+
if (!graphOrders) return
256+
257+
const order = graphOrders.get(localNodeId)
258+
if (!order) return
259+
260+
if (discardValues) {
261+
for (const widgetId of order) {
262+
graphWidgetStates.value.get(graphId)?.delete(widgetId)
263+
graphWidgetRenderStates.value.get(graphId)?.delete(widgetId)
264+
}
265+
}
266+
graphOrders.delete(localNodeId)
267+
}
268+
243269
function clearGraph(graphId: UUID): void {
244270
graphWidgetStates.value.delete(graphId)
245271
graphWidgetRenderStates.value.delete(graphId)
@@ -258,6 +284,7 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
258284
setNodeWidgetOrder,
259285
replaceNodeWidgetOrder,
260286
removeNodeWidgetOrder,
287+
releaseNodeWidgets,
261288
clearGraph
262289
}
263290
})

0 commit comments

Comments
 (0)