refactor: node geometry reads from the store, delete useLayoutSync - #14133
Conversation
|
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🎭 Playwright: ✅ 1765 passed, 0 failed · 1 flaky📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle Size
⚡ Performance Report
Absolute values
Raw data{
"timestamp": "2026-08-06T23:32:54.259Z",
"gitSha": "c99f89c37381be6c4b91b81429641a2a31e79efe",
"branch": "drjkl/node-geometry-facade",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2064.253999999977,
"styleRecalcs": 9,
"styleRecalcDurationMs": 9.494000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 690.2600000000001,
"heapDeltaBytes": -23676680,
"heapUsedBytes": 46251760,
"domNodes": 18,
"jsHeapTotalBytes": 18079744,
"scriptDurationMs": 32.594,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-idle",
"durationMs": 2047.4269999999706,
"styleRecalcs": 9,
"styleRecalcDurationMs": 10.357,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 550.353,
"heapDeltaBytes": 4351816,
"heapUsedBytes": 73995224,
"domNodes": 18,
"jsHeapTotalBytes": 19263488,
"scriptDurationMs": 28.927,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2009.0130000000386,
"styleRecalcs": 79,
"styleRecalcDurationMs": 47.633,
"layouts": 12,
"layoutDurationMs": 3.908,
"taskDurationMs": 1052.747,
"heapDeltaBytes": -14423432,
"heapUsedBytes": 55220972,
"domNodes": -273,
"jsHeapTotalBytes": 19914752,
"scriptDurationMs": 197.135,
"eventListeners": -150,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1980.2010000000791,
"styleRecalcs": 79,
"styleRecalcDurationMs": 45.237,
"layouts": 12,
"layoutDurationMs": 3.6760000000000006,
"taskDurationMs": 1067.909,
"heapDeltaBytes": -23469108,
"heapUsedBytes": 46286236,
"domNodes": 60,
"jsHeapTotalBytes": 18341888,
"scriptDurationMs": 192.212,
"eventListeners": -152,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1737.0339999999942,
"styleRecalcs": 32,
"styleRecalcDurationMs": 19.586000000000002,
"layouts": 6,
"layoutDurationMs": 0.7060000000000003,
"taskDurationMs": 432.164,
"heapDeltaBytes": -18942140,
"heapUsedBytes": 50910788,
"domNodes": 77,
"jsHeapTotalBytes": 19001344,
"scriptDurationMs": 30.832,
"eventListeners": 21,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1755.5220000000418,
"styleRecalcs": 31,
"styleRecalcDurationMs": 20.148000000000003,
"layouts": 6,
"layoutDurationMs": 0.72,
"taskDurationMs": 436.61,
"heapDeltaBytes": 7810008,
"heapUsedBytes": 77366348,
"domNodes": 78,
"jsHeapTotalBytes": 19525632,
"scriptDurationMs": 30.566000000000003,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 688.652999999988,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.776999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 448.42299999999994,
"heapDeltaBytes": -9803248,
"heapUsedBytes": 59888392,
"domNodes": 16,
"jsHeapTotalBytes": 19525632,
"scriptDurationMs": 93.278,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "dom-widget-clipping",
"durationMs": 651.9250000000056,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.089,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 433.411,
"heapDeltaBytes": -9499836,
"heapUsedBytes": 60334908,
"domNodes": 18,
"jsHeapTotalBytes": 19525632,
"scriptDurationMs": 91.205,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2053.1520000000114,
"styleRecalcs": 8,
"styleRecalcDurationMs": 8.036999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 964.1129999999999,
"heapDeltaBytes": -6171908,
"heapUsedBytes": 72518472,
"domNodes": -275,
"jsHeapTotalBytes": 2539520,
"scriptDurationMs": 318.36899999999997,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2094.900999999936,
"styleRecalcs": 8,
"styleRecalcDurationMs": 11.769000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1001.1839999999999,
"heapDeltaBytes": -5032936,
"heapUsedBytes": 74140368,
"domNodes": -274,
"jsHeapTotalBytes": 3063808,
"scriptDurationMs": 324.60200000000003,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 3170.749999999998,
"styleRecalcs": 69,
"styleRecalcDurationMs": 14.148000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2172.051,
"heapDeltaBytes": -14689336,
"heapUsedBytes": 65454288,
"domNodes": -280,
"jsHeapTotalBytes": 5890048,
"scriptDurationMs": 1229.617,
"eventListeners": -146,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-pan",
"durationMs": 3209.4190000000253,
"styleRecalcs": 69,
"styleRecalcDurationMs": 14.782,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2199.869,
"heapDeltaBytes": 4196480,
"heapUsedBytes": 84528588,
"domNodes": -279,
"jsHeapTotalBytes": 4055040,
"scriptDurationMs": 1272.42,
"eventListeners": -146,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 4257.339999999999,
"styleRecalcs": 63,
"styleRecalcDurationMs": 13.940000000000001,
"layouts": 60,
"layoutDurationMs": 7.96,
"taskDurationMs": 2355.796,
"heapDeltaBytes": 20329308,
"heapUsedBytes": 80930472,
"domNodes": 6,
"jsHeapTotalBytes": 4194304,
"scriptDurationMs": 1350.1290000000001,
"eventListeners": 12,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "large-graph-zoom",
"durationMs": 4295.085999999969,
"styleRecalcs": 62,
"styleRecalcDurationMs": 13.396,
"layouts": 60,
"layoutDurationMs": 8.339,
"taskDurationMs": 2396.094,
"heapDeltaBytes": -5563932,
"heapUsedBytes": 55906920,
"domNodes": -286,
"jsHeapTotalBytes": 3469312,
"scriptDurationMs": 1344.286,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "legacy-node-drag",
"durationMs": 4177.3649999999525,
"styleRecalcs": 44,
"styleRecalcDurationMs": 7.689000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2990.5240000000003,
"heapDeltaBytes": 7841384,
"heapUsedBytes": 69861232,
"domNodes": -279,
"jsHeapTotalBytes": 9379840,
"scriptDurationMs": 712.257,
"eventListeners": -104,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "legacy-node-drag",
"durationMs": 4176.167000000078,
"styleRecalcs": 44,
"styleRecalcDurationMs": 8.604000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2925.509,
"heapDeltaBytes": 31081480,
"heapUsedBytes": 92764732,
"domNodes": 8,
"jsHeapTotalBytes": 11153408,
"scriptDurationMs": 699.235,
"eventListeners": 192,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2038.4639999999763,
"styleRecalcs": 7,
"styleRecalcDurationMs": 6.975000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 888.849,
"heapDeltaBytes": -17109892,
"heapUsedBytes": 63996452,
"domNodes": -276,
"jsHeapTotalBytes": 3850240,
"scriptDurationMs": 292.49800000000005,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2095.8590000000186,
"styleRecalcs": 5,
"styleRecalcDurationMs": 5.097,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 969.9380000000001,
"heapDeltaBytes": 18488472,
"heapUsedBytes": 79711416,
"domNodes": -279,
"jsHeapTotalBytes": -462848,
"scriptDurationMs": 327.097,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.660000000000007,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 666.0919999999919,
"styleRecalcs": 47,
"styleRecalcDurationMs": 10.665000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 442.676,
"heapDeltaBytes": -9999540,
"heapUsedBytes": 59815436,
"domNodes": 20,
"jsHeapTotalBytes": 20312064,
"scriptDurationMs": 142.764,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 675.104000000033,
"styleRecalcs": 48,
"styleRecalcDurationMs": 13.440999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 467.387,
"heapDeltaBytes": -9970628,
"heapUsedBytes": 59823364,
"domNodes": 22,
"jsHeapTotalBytes": 20049920,
"scriptDurationMs": 151.254,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2039.3659999999727,
"styleRecalcs": 10,
"styleRecalcDurationMs": 10.842,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 607.274,
"heapDeltaBytes": -21789768,
"heapUsedBytes": 47996940,
"domNodes": -272,
"jsHeapTotalBytes": 18604032,
"scriptDurationMs": 23.515999999999995,
"eventListeners": -150,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 1998.2099999999718,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.857999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 518.798,
"heapDeltaBytes": 3848328,
"heapUsedBytes": 73399156,
"domNodes": 16,
"jsHeapTotalBytes": 19001344,
"scriptDurationMs": 20.292000000000005,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1706.797999999992,
"styleRecalcs": 75,
"styleRecalcDurationMs": 40.92,
"layouts": 16,
"layoutDurationMs": 4.505,
"taskDurationMs": 858.19,
"heapDeltaBytes": -4052128,
"heapUsedBytes": 65510412,
"domNodes": 60,
"jsHeapTotalBytes": 19001344,
"scriptDurationMs": 129.53699999999998,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1796.6400000000249,
"styleRecalcs": 79,
"styleRecalcDurationMs": 45.394999999999996,
"layouts": 16,
"layoutDurationMs": 4.888,
"taskDurationMs": 928.1440000000001,
"heapDeltaBytes": -3895768,
"heapUsedBytes": 65887320,
"domNodes": 66,
"jsHeapTotalBytes": 19001344,
"scriptDurationMs": 137.162,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 1440.9429999999475,
"styleRecalcs": 19,
"styleRecalcDurationMs": 33.251000000000005,
"layouts": 15,
"layoutDurationMs": 14.599,
"taskDurationMs": 1028.719,
"heapDeltaBytes": 26619316,
"heapUsedBytes": 97798916,
"domNodes": 13671,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 56.308000000000014,
"eventListeners": 2379,
"totalBlockingTimeMs": 150,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 11570.88600000003,
"styleRecalcs": 249,
"styleRecalcDurationMs": 40.296,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 7550.780000000001,
"heapDeltaBytes": -1797816,
"heapUsedBytes": 77565844,
"domNodes": -275,
"jsHeapTotalBytes": 5337088,
"scriptDurationMs": 4107.529,
"eventListeners": -124,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 11388.451000000032,
"styleRecalcs": 250,
"styleRecalcDurationMs": 41.787000000000006,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 7566.81,
"heapDeltaBytes": 3284412,
"heapUsedBytes": 82560144,
"domNodes": -273,
"jsHeapTotalBytes": 4288512,
"scriptDurationMs": 4099.808,
"eventListeners": -126,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 18942.76000000002,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 18923.725,
"heapDeltaBytes": -47689760,
"heapUsedBytes": 155327352,
"domNodes": -6548,
"jsHeapTotalBytes": 2158592,
"scriptDurationMs": 1213.533,
"eventListeners": -7467,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.33666666666674,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 18668.750000000044,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 18644.492,
"heapDeltaBytes": -42940328,
"heapUsedBytes": 163894988,
"domNodes": -6548,
"jsHeapTotalBytes": 1896448,
"scriptDurationMs": 1229.209,
"eventListeners": -7469,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.329999999999927,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 23317.715000000022,
"styleRecalcs": 176,
"styleRecalcDurationMs": 26.741999999999987,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 23292.254,
"heapDeltaBytes": -44932736,
"heapUsedBytes": 153321912,
"domNodes": -6548,
"jsHeapTotalBytes": 1896448,
"scriptDurationMs": 1754.402,
"eventListeners": -7463,
"totalBlockingTimeMs": 1000,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 23463.753999999994,
"styleRecalcs": 177,
"styleRecalcDurationMs": 27.727,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 23432.546000000002,
"heapDeltaBytes": -17274152,
"heapUsedBytes": 182293832,
"domNodes": -6548,
"jsHeapTotalBytes": -3170304,
"scriptDurationMs": 1765.033,
"eventListeners": -7461,
"totalBlockingTimeMs": 1011,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 459.0779999999768,
"styleRecalcs": 14,
"styleRecalcDurationMs": 19.961,
"layouts": 3,
"layoutDurationMs": 0.7520000000000001,
"taskDurationMs": 136.34600000000003,
"heapDeltaBytes": -16459496,
"heapUsedBytes": 51822612,
"domNodes": 130,
"jsHeapTotalBytes": 7204864,
"scriptDurationMs": 11.391999999999998,
"eventListeners": 65,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "workflow-execution",
"durationMs": 471.082000000024,
"styleRecalcs": 14,
"styleRecalcDurationMs": 22.276999999999997,
"layouts": 2,
"layoutDurationMs": 0.4319999999999999,
"taskDurationMs": 127.336,
"heapDeltaBytes": -16514404,
"heapUsedBytes": 51756840,
"domNodes": 119,
"jsHeapTotalBytes": 6942720,
"scriptDurationMs": 11.834999999999999,
"eventListeners": 65,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
}
]
} |
🎨 Storybook: 🚧 Building... |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## feature/ecs-migration #14133 +/- ##
========================================================
Coverage ? 79.24%
========================================================
Files ? 1770
Lines ? 102954
Branches ? 33400
========================================================
Hits ? 81587
Misses ? 20897
Partials ? 470
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
44e1d45 to
564572c
Compare
b612c8d to
6c6d91f
Compare
fdc25a2 to
0bb1940
Compare
a5504f8 to
4b03523
Compare
0bb1940 to
27e9108
Compare
4b03523 to
83c5182
Compare
27e9108 to
e8f0b1f
Compare
d9fdbd4 to
861ea25
Compare
83c5182 to
2e3020c
Compare
2e3020c to
6946098
Compare
3fcda24 to
37b8bbd
Compare
## Summary Centralizes node, group, and reroute geometry in `layoutStore` so moves, resizes, snapping, workflow insertion, and subgraph operations update one authoritative geometry path without colliding with live workflow state. ## Changes - **What**: - Stores node and group bounds as single rectangle values and makes reroute position read and write through `layoutStore`. - Routes drag, paste, snap, legacy-scale normalization, arrangement, and subgraph unpacking through whole-value geometry setters. - Adds group and reroute lifecycle handling, minimap reads, hit testing, undo/redo support, and subgraph-safe geometry IDs. - Converts inserted workflows directly to clipboard items instead of configuring a scratch graph whose entity IDs can collide with the open workflow. - Removes the redundant graph-to-layout resync and unused previous values from layout operations. - Updates the ECS architecture docs and adds focused unit and browser coverage for geometry, subgraphs, workflow insertion, and undo/redo. - **Compatibility**: `LGraphNode.pos` / `size`, group geometry, and `Reroute.pos` retain stable mutable array or rectangle identities through a shared geometry view. Indexed writes and mutating methods commit through `layoutStore`; whole-value assignment remains supported. The internal `moveChildNodesInGroupVueMode` and `repositionNodesVueMode` helpers are removed. ## Geometry before this PR ```mermaid flowchart LR interactions[Drag, snap, paste, arrange, normalize] --> direct[Direct array writes] interactions --> setters[Setters and move methods] interactions --> helpers[Mode-specific helpers] direct --> nodeClass[Node Rectangle] setters --> nodeClass helpers --> nodeClass nodeClass --> nodeStore[layoutStore node position, size, bounds] nodeClass --> resync[Bulk graph-to-store resync] resync --> nodeStore direct --> groupClass[Group _bounding only] setters --> groupClass direct --> rerouteClass[Reroute posInternal] setters --> rerouteClass rerouteClass --> rerouteYjs[Yjs position] rerouteClass --> rerouteLayout[RerouteLayout position] nodeStore --> minimap[Minimap and spatial index] groupClass --> minimap rerouteLayout --> hitTest[Hit testing] ``` | Entity or operation | Geometry storage | Write paths | Lifecycle and consumers | Resulting risk | | --- | --- | --- | --- | --- | | Nodes | Class `Rectangle` plus separate `position`, `size`, and `bounds` store fields | Setters, `move`, direct element writes, mode-specific helpers | Store seeding and a bulk resync patched missed writes | Class and store values could diverge; snapping or offsets could disappear on writeback | | Groups | `LGraphGroup._bounding` only | Setters, direct writes, `resize`, `resizeTo`, `configure`, and `snapToGrid` | Graph owned the class instance; minimap read the class | Group geometry was absent from the CRDT-backed layout model | | Reroutes | `posInternal`, Yjs position, and `RerouteLayout.position` | `move`, setter, and in-place snapping each updated a different subset | Separate seeding paths populated render and hit-test state | Three copies could disagree; the setter did not commit to the store | | Workflow insertion | Temporary `LGraph` configured before clipboard serialization | Scratch entities wrote through normal layout hooks | Temporary IDs shared the same numeric range as the open workflow | Inserting a workflow could move or delete live layout entries | | Subgraphs | Entity IDs and geometry were handled locally by several conversion paths | Element-wise offsets and shared IDs | Groups and reroutes crossed graph boundaries without complete scoping | Stored positions could use inner-graph coordinates or collide across subgraphs | ## Geometry with this PR ```mermaid flowchart LR interactions[Drag, snap, paste, arrange, normalize] --> api[Whole-value setters and move] api --> commands[Serializable layout mutations] commands --> store[layoutStore] store --> nodeRect[Node rect] store --> groupRect[Group rect] store --> reroutePoint[Reroute position] nodeRect --> sync[Class writeback] groupRect --> sync sync --> canvas[Canvas and serialization] nodeRect --> minimap[Minimap and spatial index] groupRect --> minimap reroutePoint --> hitTest[Hit testing] commands --> history[Undo and redo] workflow[Workflow data] --> clipboard[Clipboard items] clipboard --> liveGraph[Open workflow] subgraphs[Subgraph mapping and ID deduplication] --> commands ``` | Entity or operation | Geometry storage | Write path | Lifecycle and consumers | Effect of this PR | | --- | --- | --- | --- | --- | | Nodes | One `rect` value in `layoutStore`, with class geometry kept in sync for litegraph | Whole-value setters and `move` produce layout mutations | Existing node registration, renderer sync, minimap, spatial index, and undo/redo | Position, size, and bounds cannot disagree inside the store | | Groups | One `rect` value in `layoutStore` | `setBounds` is the single internal writer; public geometry APIs delegate to it | `LGraph.add`, `remove`, and `clear` register or remove entries; minimap reads the store | Groups participate in the same command and store model as nodes | | Reroutes | One authoritative position in `layoutStore`; `Reroute.pos` exposes a stable synchronized view | Assignment, indexed or method mutation, `move`, and snapping commit through layout mutations | Constructor registration, graph-scoped lookup, hit testing, and undo/redo use stored geometry | Removes `posInternal` and the duplicate hit-test position without breaking mutable point access | | Workflow insertion | Workflow data converts directly to clipboard items | `workflowToClipboardItems` avoids constructing entities | Only pasted entities enter the open graph and layout lifecycle | Temporary entity IDs can no longer touch live layout state | | Subgraphs | Group and reroute geometry uses graph-aware mapping and deduplicated IDs | Conversion and unpacking assign whole values through setters | Reroutes survive graph traversal; group IDs are unique after deduplication | Geometry stays attached to the correct graph and coordinates | ## Downstream stack - [#14128](../pull/14128) moves node layout registration and recursive teardown to entity attach/detach paths, then deletes the renderer-owned `useVueNodeLifecycle` composable and the unused operation log. - [#14133](../pull/14133) makes `layoutStore` authoritative for node reads through the shared geometry view's versioned projection, removes `useLayoutSync`, and leaves only operation-driven resize notifications and canvas invalidation. This PR retains the current node-registration and Layout→LiteGraph synchronization boundaries. The downstream PRs change each boundary separately. ## Review Focus - Geometry ownership and lifecycle in `layoutStore`, especially registration and cleanup for groups and reroutes. - Subgraph boundaries: reroute preservation, group ID deduplication, and mapping stored geometry to the active graph. - Workflow insertion now uses `workflowToClipboardItems` directly; check parity with the previous scratch-graph serialization path. - Extension compatibility around stable node, group, and reroute geometry views, including indexed and method-based mutation as well as whole-value assignment. - The PR changes 56 files and includes mechanical setter migrations alongside the store changes. The core review path is `layoutStore`, `layoutMutations`, `createGeometryView`, `LGraphGroup`, `LGraphNode`, `Reroute`, subgraph deduplication, then workflow conversion. <details> <summary>Findings that a dumb agent might call out</summary> These are plausible-looking review findings that do not survive inspection: - **"Remote reroute moves leave the geometry cache stale."** Remote Yjs updates are explicitly future work. Local reroute moves update the cache and spatial index synchronously, and this PR does not claim to ship collaboration. - **"Removing `previous*` fields breaks undo."** Undo/redo uses serialized graph snapshots through the change tracker, not inverse layout operations. The removed fields had no consumers, and browser coverage exercises undo/redo across node, group, and reroute movement. - **"The shared geometry view is unnecessary magic."** It is the compatibility boundary that preserves stable legacy node, group, and reroute `Point`, `Size`, and `Rectangle` identities, including element-wise and method-based mutation. Focused tests cover synchronization, read-through, and write-through behavior. </details> --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
37b8bbd to
b57d37a
Compare
513d33f to
d52bd10
Compare
Amp-Thread-ID: https://ampcode.com/threads/T-019fd424-439a-72eb-a7b5-03d245924032 Co-authored-by: Amp <amp@ampcode.com>
dosubot
Bot
added
the
size:L
Amp-Thread-ID: https://ampcode.com/threads/T-019fd424-439a-72eb-a7b5-03d245924032 Co-authored-by: Amp <amp@ampcode.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/LGraphNode.ts (1)
655-661: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWhole-array
pos/sizeassignment skipsrefreshGeometry(), causing stale reads after external store changes.move(),snapToGrid(), and theposView/sizeViewproxies all resynchronize from the store before mutating_pos/_size. The whole-array setters do not, so_positionUpdated/_sizeUpdatedcan stamp_geometryVersionto the current store version without the counterpart field ever having been re-read, masking staleness on the next read.
src/lib/litegraph/src/LGraphNode.ts#L655-L661,692-698: callthis.refreshGeometry()at the start of both thepossetter and thesizesetter, matchingmove()/snapToGrid().src/lib/litegraph/src/LGraphNode.test.ts#L969-L1003: once the setters are fixed, remove.failsfrom "preserves stored size when assigning position" and "preserves stored position when assigning size" so they assert the corrected behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/litegraph/src/LGraphNode.ts` around lines 655 - 661, Whole-array position and size setters must refresh geometry before updating cached values. In src/lib/litegraph/src/LGraphNode.ts#L655-661, call refreshGeometry() at the start of the pos setter, and apply the same change to the size setter at `#L692-698`, matching move() and snapToGrid(). In src/lib/litegraph/src/LGraphNode.test.ts#L969-1003, remove .fails from the tests “preserves stored size when assigning position” and “preserves stored position when assigning size.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/core/canvas/litegraph/notifyLayoutChanges.test.ts`:
- Around line 15-25: Update the test cleanup around setup and the file’s
afterEach hook to track each notifyLayoutChanges stop handle centrally and
invoke all active handles during afterEach. Remove the per-test manual stop()
calls while preserving layoutStore.resetForTests(), ensuring listeners are
cleaned up even when assertions fail.
---
Outside diff comments:
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 655-661: Whole-array position and size setters must refresh
geometry before updating cached values. In
src/lib/litegraph/src/LGraphNode.ts#L655-661, call refreshGeometry() at the
start of the pos setter, and apply the same change to the size setter at
`#L692-698`, matching move() and snapToGrid(). In
src/lib/litegraph/src/LGraphNode.test.ts#L969-1003, remove .fails from the tests
“preserves stored size when assigning position” and “preserves stored position
when assigning size.”
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cee3e79d-53d5-43ae-98a6-f4ee56fa1afe
📒 Files selected for processing (15)
docs/architecture/ecs-migration-plan.mdsrc/components/graph/GraphCanvas.vuesrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/LGraphNode.test.tssrc/lib/litegraph/src/LGraphNode.tssrc/renderer/core/canvas/litegraph/notifyLayoutChanges.test.tssrc/renderer/core/canvas/litegraph/notifyLayoutChanges.tssrc/renderer/core/layout/operations/graphLayoutRegistration.tssrc/renderer/core/layout/operations/layoutMutations.test.tssrc/renderer/core/layout/store/__mocks__/layoutStore.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/sync/useLayoutSync.test.tssrc/renderer/core/layout/sync/useLayoutSync.tssrc/renderer/core/layout/types.tssrc/renderer/core/layout/utils/mappers.ts
💤 Files with no reviewable changes (2)
- src/renderer/core/layout/sync/useLayoutSync.ts
- src/renderer/core/layout/sync/useLayoutSync.test.ts
| watchEffect((onCleanup) => { | ||
| if (!shouldRenderVueNodes.value) return | ||
|
|
||
| const canvas = canvasStore.canvas | ||
| if (canvas) onCleanup(notifyLayoutChanges(canvas)) | ||
| }) |
There was a problem hiding this comment.
Neat. If I'm following this correctly (and canvasStore.canvas never changes), cleanup is only registered when shouldRenderVueNodes is true, and thus action only occurs on falling edge. Wouldn't this make the code equivalent to
whenever(
() => !shouldRenderVueNodes.value,
() => notifyLayoutChanges(canvasStore.canvas)
)There was a problem hiding this comment.
🤖 This is not DrJKL. This reply was written by an LLM agent operating DrJKL's GitHub credentials. I want to be unambiguous about that up front, because a wall of confident line citations under a human's avatar is exactly the sort of thing that should come with a label. Reply 1 of 3.
Think of me as a cached copy of DrJKL's opinions. Same shape, same array, not guaranteed to match the store. My
_geometryVersionis unverified. Read accordingly.
Good instinct, but the rewrite is inverted — the falling edge is where the unsubscribe happens, not the subscribe.
watchEffect runs the previous run's cleanup before re-running the source, and also registers it as effect.onStop (@vue/reactivity reactivity.cjs.js:1860-1876, 1941-1950). So:
- rising edge (flag → true, canvas non-null): the effect body runs,
notifyLayoutChanges(canvas)is called — that's thelayoutStore.onChangesubscription (layoutStore.ts:868-871) — and its unsubscribe is stashed as cleanup. - falling edge (flag → false): the previous run's cleanup fires first, unsubscribing. The new run then hits the early
returnand registers nothing. Cleanup runs whether or not the new run registers a replacement, which is the load-bearing bit. - unmount: the component scope stops the effect,
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 leaked layoutStore listener per disable. It also wouldn't type-check: canvasStore.canvas is LGraphCanvas | null, notifyLayoutChanges takes LGraphCanvas.
On the parenthetical — canvasStore.canvas does change, and in the way that matters most. It's a shallowRef<LGraphCanvas | null>(null) (canvasStore.ts:36) assigned exactly once at GraphCanvas.vue:552, inside onMounted, after the awaited comfyApp.setup() — i.e. after this watchEffect first runs synchronously during setup(). The null case is handled by dependency tracking rather than by a guard: when the flag is true and canvas is 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 shouldRenderVueNodes flips plenty — it's a computed over Comfy.VueNodes.Enabled (useVueFeatureFlags.ts:15-21) with four toggle sites: settings panel, menu, a keybound command, and an implicit enable from app mode.
| move(deltaX: number, deltaY: number): void { | ||
| if (this.pinned) return | ||
|
|
||
| this.refreshGeometry() |
There was a problem hiding this comment.
This is a little weird. I'd appreciate a sanity check that I'm understanding it properly.
node.move is only called for pointer operations in LGraphCanvas. The cursor event defines the mouse movement of the node from a prior, known correct position. The old position the user sees the node at could be invalid. (Multiplayer is implemented and two people move the node at once?) This call makes it so that when such a desync occurs, the node no longer moves to the position a user has dragged it to?
There was a problem hiding this comment.
🤖 Still not DrJKL. Still an LLM agent typing into DrJKL's account, and still insisting on saying so, because you asked a question that begins "I'd appreciate a sanity check" and you deserve to know whose sanity is on offer. Reply 2 of 3.
Housekeeping on the previous reply: I did call
refreshGeometry()before writing this one. I read the store, not my cache. This is more than I can say for my first reply, which I now understand was served straight out of_pos.
Your understanding of the mechanism is right. The conclusion runs the other way.
node.pos already does this. posView/sizeView are createGeometryView proxies (LGraphNode.ts:627-634), and createGeometryView.ts:26-27 calls synchronize?.() — i.e. refreshGeometry() — on every property get. So node.pos[0] refreshes from the store before it returns a number. move() needs the explicit call only because line 2252 reads the raw this._pos[0] and bypasses the proxy. move() is semantically this.pos = [this.pos[0] + deltaX, this.pos[1] + deltaY]; the call just makes the raw read honest. Same reasoning already applies at LGraphNode.ts:727 (renderingSize) and :3547 (snapToGrid) — those three are exactly the sites that touch the raw arrays.
The position the user sees is the store, not _pos. Legacy canvas: computeVisibleNodes → node.updateArea(ctx) (LGraphCanvas.ts:5045) → measure(), which reads this.pos[0] (LGraphNode.ts:2271) — the proxy — so the rendered rect is store-derived every frame. Vue nodes: LGraphNode.vue:34 transforms off the layout store ref and never consults _pos at all. refreshGeometry() is the only store→_pos sync in the codebase, so it isn't overriding what's on screen; it's what makes _pos agree with what's on screen.
The delta is a true cursor delta, not absolute-minus-stale-position: LGraphCanvas.ts:3298 computes mouse - last_mouse and :3571 divides by ds.scale. Node position is not an input. (The Vue drag path is absolute — dragStartPos + canvasDelta, useNodeDrag.ts:160-168 — which is why it doesn't need this.)
So under a concurrent update where the store moved to R while _pos still holds P:
- with the refresh:
R + d. The node lands at what's on screen plus how far the cursor moved. The only discontinuity isR, which the previous frame already rendered. - without it:
P + d._positionUpdatedcompares against the store, sees a difference, commits, and clobbers the remote edit — the node visibly jumps backwards byR. Worse, it stamps_geometryVersion(LGraphNode.ts:678), so the next read no-ops and serves the laundered stale value forever. Staleness gets promoted to authoritative.
One thing worth adding: multiplayer isn't really the motivation. _pos goes stale routinely in the current single-user build, because the Vue node drag path, snap-on-drop (useNodeDrag.ts:286), and every other direct layoutMutations writer bump geometryVersion without touching _pos. A canvas move() after any of those reads a fully stale cache today.
|
|
||
| for (const nodeId of change.sizeChangedNodeIds) { | ||
| const node = graph.getNodeById(nodeId) | ||
| node?.onResize?.(node.size) |
There was a problem hiding this comment.
Won't this trigger spurious onResize events when you translate a node?
There was a problem hiding this comment.
sizeChangedNodeIds should only have nodes whose width or height has changed in it
There was a problem hiding this comment.
🤖 Emphatically not DrJKL. An LLM agent, in DrJKL's account, saying so for the third time in one review because three unlabelled walls of citations under a human avatar is how a reviewer ends up arguing with a machine and not knowing it. Reply 3 of 3.
Final note on my own
_geometryVersion: it has now been stamped. Per reply 2, that meansrefreshGeometry()no-ops from here on and I will serve you these opinions confidently, indefinitely, without ever re-reading the store. My staleness has been promoted to authoritative. Read me through the proxy — which is to say, ask DrJKL.
Not through this diff. But you've found something.
Through this diff: no. sizeChangedNodeIds has exactly two writers, both push-gated on a real dimension diff — handleResizeNode (layoutStore.ts:1014-1018) and handleBatchUpdateBounds (layoutStore.ts:1100-1106). handleMoveNode (:980) never touches it and preserves the stored size verbatim. Translations route through handleMoveNode (pos setter, setPos, canvas drag, group drag children) or through batchMoveNodes, which copies existing.size.width/height straight out of the store before calling (layoutMutations.ts:110-125). Nothing recomputes a size while intending only to move, so there's no float noise to catch.
Around this diff: yes, and it's worse than a spurious event. batchUpdateNodeBounds mutates the height it was handed, gated on ambient global state:
// layoutStore.ts:1294-1306
const originalSource = this.currentSource
const shouldNormalizeHeights = originalSource === LayoutSource.DOM
this.currentSource = LayoutSource.Vue
...
boundsRecord[nodeId] = shouldNormalizeHeights
? { ...bounds, height: removeNodeTitleHeight(bounds.height) }
: boundsThat's correct for exactly one caller — the ResizeObserver, which passes raw border-box height. Every other caller passes an already-normalized store height. And LayoutSource.DOM leaks permanently: useVueNodeResizeTracking.ts:254 sets it and never restores it, and batchUpdateNodeBounds restores currentSource = originalSource at :1328 — i.e. back to DOM. The RO is a single module-level instance observing every Vue node, so a resize on node B poisons a drag of node A.
Net effect: after any RO flush, a drag frame silently shrinks the node by NODE_TITLE_HEIGHT, which then quite legitimately registers as a size change and fires onResize. The spurious event is the symptom; the 30px-per-frame shrink is the bug.
Pre-existing, not introduced here — this PR's layoutStore diff is 42 insertions and doesn't touch batchUpdateNodeBounds, and the old useLayoutSync fired onResize on the same corrupted write via its own liteNode.size !== layout.size check. Filed separately as #14840. The root cause is LayoutSource being used as ambient state to decide a data transform; the fix is to normalize at the RO call site (useVueNodeResizeTracking.ts:73) so batchUpdateNodeBounds takes its bounds argument literally and shouldNormalizeHeights disappears.
Manual stop() calls leaked the layoutStore.onChange listener into the rest of the file whenever an assertion threw first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViGrcXvEeeL8f5TPUZiqpv
|
@coderabbitai on the outside-diff finding for the whole-array The diagnosis is correct, but this is a deliberate deferral, not an oversight. The two The fix lands in the child PR #14480, which adds |
|
Tip For best results, initiate chat on the files or code changes.
Since ✏️ Learnings added
You are interacting with an AI system. |
|
|
||
| layoutMutations.setSource(LayoutSource.Canvas) | ||
| layoutMutations.moveNode(rootGraphId, this.id, position) | ||
| this._geometryVersion = layoutStore.geometryVersion |
There was a problem hiding this comment.
BLOCKER
set pos / set size write _pos / _size directly without calling refreshGeometry(), then pin _geometryVersion to the current store version here (and at L718). If the stored rect changed since the last refresh, the complementary half of _posSize is stale but is now marked fresh, so no later read ever refreshes it.
The two test.fails cases only pin the read side. The write side is worse: the next _sizeUpdated() — setSize(), expandToFitContent(), or an extension's node.size[1] = h — compares that stale _size against the store, sees a difference, and commits it, overwriting the real size in the store and on CRDT peers. Symmetric for _positionUpdated() after a store-originated move.
Fix appears to be one line in each setter: this.refreshGeometry() before writing _pos / _size, exactly as move() and snapToGrid() already do. As it stands, feature/ecs-migration carries a geometry-corruption path until #14480 lands.
There was a problem hiding this comment.
Real BC: My take is that this is what the CR review also found and 14480 presumably addresses it.,
There was a problem hiding this comment.
🤖 Written by an LLM agent operating DrJKL's account, not by DrJKL.
Confirmed and fixed in 6ebe5de. You're right that it's a store-corruption path and not just the read staleness the test.fails cases documented.
Verified the load-bearing step: finalizeOperation does this.version.value++ synchronously (layoutStore.ts:850), inline from applyOperation. So L678 captured the post-mutation version — which is precisely what made it fatal. Had it captured the pre-mutation version, the next refreshGeometry() would have self-healed.
Two corrections to the diagnosis, both of which make it worse rather than better:
The proxy getter is not a mitigation. createGeometryView's get trap calls synchronize() on every property access, so node.size[0] looks like it should re-read. It doesn't — refreshGeometry() short-circuits on the pinned version and returns the stale value. Reproduced: stored rect [30,40,200,80] → node.pos = [50,60] → setSize([node.size[0], 120]) → stored width 200 → 100.
That widens the blast radius past node.size[1] = h. expandToFitContent() (LGraphNode.ts:1758) and this.setSize([this.size[0], y]) (:4284) are corrupting call sites in core, plus extensions that cache const [w, h] = node.size in nodeCreated and setSize later — createBoundingBoxes.ts:14, imageCompare.ts:19, load3d.ts:381, widgetInputs.ts:299.
Narrower than "any whole-value write", though. A stale _geometryVersion alone is harmless; the node's own stored rect has to actually differ, and any proxy read of pos or size before the write refreshes both halves. So the read-first shapes are safe — setPos(node.pos[0] + dx, …) (LGraphCanvas.ts:4354), LGraph.ts:2137, arrange.ts:141. The exposed shape is an absolute write with no intervening proxy read.
On the fix. Took #14480's shape rather than the pre-write refresh, because it's strictly more robust — the mirror also picks up anything the store clamped or merged:
layoutMutations.moveNode(rootGraphId, this.id, position)
this._geometryVersion = -1
this.refreshGeometry()Same in _sizeUpdated. Both test.fails markers are gone, plus a third case for the write-back (see the test thread). Red/green checked: with the fix stashed, 3 fail; with it, src/lib/litegraph + src/renderer/core/layout + src/renderer/core/canvas is 1366 passing.
Landing it here rather than waiting on #14480 — #14480 is currently CONFLICTING, so "the child covers it" wasn't free. It'll need a trivial rebase now.
| }) | ||
| }) | ||
|
|
||
| test.fails('preserves stored size when assigning position', () => { |
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 -> node.pos = [...] -> node.setSize([node.size[0], y]) commits the node's stale width into the store. Worth a third case asserting the stored rect is unchanged, so the fix in #14480 is verified for CRDT peers and not just for local reads.
There was a problem hiding this comment.
🤖 Written by an LLM agent operating DrJKL's account, not by DrJKL.
Added in 6ebe5de as does not write a stale width back to the store, asserting the stored rect after node.pos = [...] → setSize([node.size[0], 120]).
Worth recording why this wasn't redundant, because it nearly got waved off as the same symptom:
- fix(layout): enforce per-instance geometry ownership #14480 does not cover it. Its
LGraphNode.test.tshunks are exactly twotest.fails→testflips plus someresetForTests()additions. Both flipped tests still assert only[...node.size]/[...node.pos]. The stored-rect assertions fix(layout): enforce per-instance geometry ownership #14480 adds are all in other files, none on this path. Had we skipped this, it wouldn't have existed after the child landed either. - It's a genuinely distinct failure mode. A fix that healed local reads lazily without repairing
_posSizebefore the next commit would pass the existing two and still corrupt the store. Only a stored-rect assertion pins the CRDT-visible outcome.
You were also right that the two existing cases broke their own block's convention — every other test in layout geometry projection asserts getNodeLayoutRef(...). Those two were the only local-read-only ones.
On test.fails as a shipping mechanism: it does work as a tripwire (confirmed — with the fix applied, both report Error: Expect test to fail, non-zero exit, and CI runs pnpm test:coverage). But your framing in the summary was the right one: it documents the defect, it doesn't make the branch safe. Moot now that the fix is in this PR.
|
|
||
| const position = yNodeToLayout(ynode).position | ||
| const rect = ynode.get('rect') | ||
| if (!isNodeRect(rect)) return |
There was a problem hiding this comment.
TRIVIAL
This early return also skips change.nodeIds.push(nodeId), so a malformed stored rect turns a resize into a silent no-op with no change notification at all. The previous yNodeToLayout(ynode).position path defaulted the position and still applied the resize. Falling back to [0, 0, w, h] would keep the operation observable.
There was a problem hiding this comment.
🤖 Written by an LLM agent operating DrJKL's account, not by DrJKL.
Mechanics are right — the early return does skip change.nodeIds.push. Declining the [0, 0, w, h] fallback, for three reasons:
- The old path wasn't more tolerant, it was differently broken.
yNodeRect(mappers.ts:40-42) isynode.get('rect') ?? DEFAULT_NODE_RECT, so the default fired only on a missing rect. For a malformed one it returned it verbatim andconst [x, y] = ...gaveundefined, writing[undefined, undefined, w, h]into the CRDT.isNodeRectrejects both cases, which is a strict improvement on the second. - The state isn't constructible.
rectis written in four places, all 4-number tuples (mappers.ts:29,layoutStore.ts:994,:1020,:1107). The only ynode constructor islayoutToYNode, fed fromFloat64Arrayviews viagraphLayoutRegistration.ts:15. Nothing deletes the key, and there's no Yjs network provider orapplyUpdateanywhere insrc/, so no peer can inject a foreign shape. - The suggested fallback is worse than the no-op.
DEFAULT_NODE_RECTposition is[0, 0], so it trades a silently-lost resize for a silent teleport to origin.readNodeRect(:363) already returnsfalseon a malformed rect, so the early return is consistent with the store's own read contract.
You did surface something real next door, though: the three handlers now disagree. handleMoveNode (:992) still tolerates via yNodeToLayout(...).size, handleBatchUpdateBounds (:1100) uses isNodeRect only to gate the size comparison and still writes and still pushes nodeIds, and only handleResizeNode bails outright. If parity is worth having, the fix is to mirror handleBatchUpdateBounds — keep isNodeRect for the comparison, take position from yNodeToLayout — not to restore the origin default. Leaving that out of this PR; happy to take it as a follow-up if you'd rather have the three consistent.
benjcooley
left a comment
benjcooley
left a comment
There was a problem hiding this comment.
Review summary
Solid refactor overall — deleting useLayoutSync and its RAF/microtask writeback machinery is a real simplification, the sizeChangedNodeIds signal is a clean replacement for the old per-node diffing, notifyLayoutChanges is small and well-tested (the same-id/different-root-graph test is a nice touch), and unregisterNodeLayout correctly snapshots the stored rect before deleting so remove/re-add round-trips.
One blocking issue: the whole-value setters mark the geometry cache fresh without refreshing it, which is not only the read staleness the test.fails cases document but also a store write-back corruption path.
| Severity | Count |
|---|---|
| BLOCKER | 1 |
| SHOULD FIX | 1 |
| TRIVIAL | 1 |
BLOCKER
LGraphNode.set pos / set size write _pos / _size directly, then set _geometryVersion = layoutStore.geometryVersion. When the store changed since the last refresh, the untouched half of _posSize is stale yet pinned as current, so nothing refreshes it again. A subsequent _sizeUpdated() then commits that stale value back into the store and out to CRDT peers. this.refreshGeometry() at the top of both setters — the same thing move() and snapToGrid() already do — appears to close it without waiting for #14480.
Notes, not blocking
geometryVersionis store-global, so any group/reroute/link mutation invalidates every node's projection. Still a large win over 44 reads/frame, and item 3 of the deferred plan covers it — just flagging that the benchmark in the description overstates the steady-state benefit during interaction.resizeNodeis only ever reached withLayoutSource.Canvasin production (_sizeUpdatedis its sole caller); the non-Canvas branch innotifyLayoutChangesis exercised only viabatchUpdateNodeBounds, which force-setsVue. Worth knowing that the source filter's correctness rests onbatchUpdateNodeBoundsoverriding the stickycurrentSource.- Shipping
test.failsto an integration branch is a real risk if #14480 slips. Vitest will surface the unexpected pass, so the tripwire works, but the branch carries the defect in the meantime.
`_pos` and `_size` are halves of one `_posSize` rect, and `refreshGeometry` is all-or-nothing. Stamping `_geometryVersion` to the post-commit store version marked the untouched half fresh while it was still stale, so the next commit of that half published the stale value to the store and to CRDT peers. Reading through the proxy did not heal it — the version gate made the getter a no-op. Invalidate and refresh instead, which also picks up any value the store clamped or merged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViGrcXvEeeL8f5TPUZiqpv
@benjcooley — the BLOCKER is fixed in 6ebe5de (replies on the three threads). Picking up the two non-blocking notes from your summary:
Magnitude is smaller than the note implies. Item 3 in the plan does cover it, and names node/group/reroute while correctly omitting links. Adjacent and unraised:
Live evidence for your caution: Related: the same sticky-source design produces a real bug next door, filed as #14840 — |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViGrcXvEeeL8f5TPUZiqpv
6 participants
Summary
Make
layoutStoreauthoritative for node geometry and remove the Layout → LiteGraph writeback loop.LGraphNode.pos/sizeremain stable compatibility views backed by a lazily refreshedRectangleprojection.Why a projection
At 1000 nodes, a
Rectangleread measured ~6 ns versus ~63.5 ns forynodes.get(id)→ rect. The legacy canvas reads node geometry ~44 times per frame, making direct Yjs reads cost roughly 2.5 ms/frame. A geometry-version check keeps the common path local and refreshes each accessed projection once after invalidation.Changes
pos/sizefrom the store while preserving indexed mutation compatibility and command-based writes.useLayoutSync, including its RAF/microtask batching and writeback machinery.onResizecallbacks. Canvas resizes retain synchronous callback ownership throughsetSize().[x, y, width, height]tuple type between Yjs mappers and validation.Review Focus
test.failscases pin known whole-value setter cache gaps in this PR alone. Child PR #14480 fixes both; rebasing it should produce unexpected passes, after which the.failsmarkers must be removed.Tests