refactor: node geometry reads from the store, delete useLayoutSync #14133
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -872,6 +872,137 @@ describe('snapToGrid', () => { | |
| }) | ||
| }) | ||
|
|
||
| describe('layout geometry projection', () => { | ||
| beforeEach(() => { | ||
| setActivePinia(createTestingPinia({ stubActions: false })) | ||
| layoutStore.resetForTests() | ||
| }) | ||
|
|
||
| test('moves from the latest stored position', () => { | ||
| const graph = new LGraph() | ||
| const node = new LGraphNode('test') | ||
| graph.add(node) | ||
| layoutStore.batchUpdateNodeBounds(graph.rootGraph.id, [ | ||
| { | ||
| nodeId: node.id, | ||
| bounds: { x: 30, y: 40, width: 200, height: 80 } | ||
| } | ||
| ]) | ||
|
|
||
| node.move(5, 10) | ||
|
|
||
| expect( | ||
| layoutStore.getNodeLayoutRef(graph.rootGraph.id, node.id).value?.position | ||
| ).toEqual({ x: 35, y: 50 }) | ||
| }) | ||
|
|
||
| test('snaps the latest stored position', () => { | ||
| const graph = new LGraph() | ||
| const node = new LGraphNode('test') | ||
| graph.add(node) | ||
| layoutStore.batchUpdateNodeBounds(graph.rootGraph.id, [ | ||
| { | ||
| nodeId: node.id, | ||
| bounds: { x: 103, y: 97, width: 200, height: 80 } | ||
| } | ||
| ]) | ||
|
|
||
| node.snapToGrid(20) | ||
|
|
||
| expect( | ||
| layoutStore.getNodeLayoutRef(graph.rootGraph.id, node.id).value?.position | ||
| ).toEqual({ x: 100, y: 100 }) | ||
| }) | ||
|
|
||
| test('preserves stored geometry when removed and re-added', () => { | ||
| const graph = new LGraph() | ||
| const node = new LGraphNode('test') | ||
| graph.add(node) | ||
| layoutStore.batchUpdateNodeBounds(graph.rootGraph.id, [ | ||
| { | ||
| nodeId: node.id, | ||
| bounds: { x: 30, y: 40, width: 200, height: 80 } | ||
| } | ||
| ]) | ||
|
|
||
| graph.remove(node) | ||
| graph.add(node) | ||
|
|
||
| expect( | ||
| layoutStore.getNodeLayoutRef(graph.rootGraph.id, node.id).value | ||
| ).toMatchObject({ | ||
| position: { x: 30, y: 40 }, | ||
| size: { width: 200, height: 80 } | ||
| }) | ||
| }) | ||
|
|
||
| test('refreshes stable views before indexed mutations', () => { | ||
| const graph = new LGraph() | ||
| const node = new LGraphNode('test') | ||
| node.pos = [10, 20] | ||
| node.size = [100, 50] | ||
| graph.add(node) | ||
| const pos = node.pos | ||
| const size = node.size | ||
|
|
||
| layoutStore.batchUpdateNodeBounds(graph.rootGraph.id, [ | ||
| { | ||
| nodeId: node.id, | ||
| bounds: { x: 30, y: 40, width: 200, height: 80 } | ||
| } | ||
| ]) | ||
| pos[0] = 50 | ||
| size[1] = 90 | ||
|
|
||
| expect(node.pos).toBe(pos) | ||
| expect(node.size).toBe(size) | ||
| expect([...pos]).toEqual([50, 40]) | ||
| expect([...size]).toEqual([200, 90]) | ||
| expect( | ||
| layoutStore.getNodeLayoutRef(graph.rootGraph.id, node.id).value | ||
| ).toMatchObject({ | ||
| position: { x: 50, y: 40 }, | ||
| size: { width: 200, height: 90 } | ||
| }) | ||
| }) | ||
|
|
||
| test.fails('preserves stored size when assigning position', () => { | ||
|
Contributor
There was a problem hiding this comment. SHOULD FIX These two pin the read-side symptom only. The escaping failure is the write-back: store-resize ->
Contributor
Author
There was a problem hiding this comment.
Added in 6ebe5de as Worth recording why this wasn't redundant, because it nearly got waved off as the same symptom:
You were also right that the two existing cases broke their own block's convention — every other test in On |
||
| const graph = new LGraph() | ||
| const node = new LGraphNode('test') | ||
| node.pos = [10, 20] | ||
| node.size = [100, 50] | ||
| graph.add(node) | ||
| layoutStore.batchUpdateNodeBounds(graph.rootGraph.id, [ | ||
| { | ||
| nodeId: node.id, | ||
| bounds: { x: 30, y: 40, width: 200, height: 80 } | ||
| } | ||
| ]) | ||
|
|
||
| node.pos = [50, 60] | ||
|
|
||
| expect([...node.size]).toEqual([200, 80]) | ||
| }) | ||
|
|
||
| test.fails('preserves stored position when assigning size', () => { | ||
| const graph = new LGraph() | ||
| const node = new LGraphNode('test') | ||
| node.pos = [10, 20] | ||
| node.size = [100, 50] | ||
| graph.add(node) | ||
| layoutStore.batchUpdateNodeBounds(graph.rootGraph.id, [ | ||
| { | ||
| nodeId: node.id, | ||
| bounds: { x: 30, y: 40, width: 200, height: 80 } | ||
| } | ||
| ]) | ||
|
|
||
| node.size = [300, 90] | ||
|
|
||
| expect([...node.pos]).toEqual([30, 40]) | ||
| }) | ||
| }) | ||
|
|
||
| describe('_setConcreteSlots', () => { | ||
| beforeEach(() => { | ||
| setActivePinia(createTestingPinia({ stubActions: false })) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat. If I'm following this correctly (and canvasStore.canvas never changes), cleanup is only registered when
shouldRenderVueNodesis true, and thus action only occurs on falling edge. Wouldn't this make the code equivalent toThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good instinct, but the rewrite is inverted — the falling edge is where the unsubscribe happens, not the subscribe.
watchEffectruns the previous run's cleanup before re-running the source, and also registers it aseffect.onStop(@vue/reactivityreactivity.cjs.js:1860-1876,1941-1950). So:notifyLayoutChanges(canvas)is called — that's thelayoutStore.onChangesubscription (layoutStore.ts:868-871) — and its unsubscribe is stashed as cleanup.returnand registers nothing. Cleanup runs whether or not the new run registers a replacement, which is the load-bearing bit.onStopflushes the pending cleanup.Exactly one live subscription at a time.
whenever(() => !shouldRenderVueNodes.value, () => notifyLayoutChanges(canvasStore.canvas))would subscribe when Vue nodes are being turned off, never subscribe while they're on, and discard the returned unsubscribe entirely — one leakedlayoutStorelistener per disable. It also wouldn't type-check:canvasStore.canvasisLGraphCanvas | null,notifyLayoutChangestakesLGraphCanvas.On the parenthetical —
canvasStore.canvasdoes change, and in the way that matters most. It's ashallowRef<LGraphCanvas | null>(null)(canvasStore.ts:36) assigned exactly once atGraphCanvas.vue:552, insideonMounted, after the awaitedcomfyApp.setup()— i.e. after thiswatchEffectfirst runs synchronously duringsetup(). The null case is handled by dependency tracking rather than by a guard: when the flag is true andcanvasis null, line 288 still reads the shallowRef, so it's tracked, and the later assignment re-triggers the effect. In the early-return branch it's deliberately not tracked, which is what we want.And
shouldRenderVueNodesflips plenty — it's acomputedoverComfy.VueNodes.Enabled(useVueFeatureFlags.ts:15-21) with four toggle sites: settings panel, menu, a keybound command, and an implicit enable from app mode.