refactor: centralize layout ownership and command submission - #15017
Conversation
dosubot
Bot
added
the
size:XL
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1805 passed, 0 failed · 6 flaky📊 Browser Reports
📦 Bundle Size
⚡ Performance Report
Absolute values
Raw data{
"timestamp": "2026-08-14T00:59:18.026Z",
"gitSha": "a45606c2bb568d05fbc178bdc4c5e16a58801bcd",
"branch": "drjkl/split-layout-attachment",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2038.2289999999443,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.192999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 508.8499999999999,
"heapDeltaBytes": 8292252,
"heapUsedBytes": 76860536,
"domNodes": 16,
"jsHeapTotalBytes": 21233664,
"scriptDurationMs": 29.334,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2038.0390000000261,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.658999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 469.7779999999999,
"heapDeltaBytes": 8200892,
"heapUsedBytes": 77002608,
"domNodes": 18,
"jsHeapTotalBytes": 20971520,
"scriptDurationMs": 27.093000000000004,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2224.9510000000328,
"styleRecalcs": 80,
"styleRecalcDurationMs": 40.369,
"layouts": 12,
"layoutDurationMs": 3.222,
"taskDurationMs": 1238.524,
"heapDeltaBytes": -12022444,
"heapUsedBytes": 56476248,
"domNodes": -280,
"jsHeapTotalBytes": 21860352,
"scriptDurationMs": 171.967,
"eventListeners": -185,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2223.731999999927,
"styleRecalcs": 78,
"styleRecalcDurationMs": 39.616,
"layouts": 12,
"layoutDurationMs": 3.436,
"taskDurationMs": 1232.329,
"heapDeltaBytes": -1982140,
"heapUsedBytes": 66740308,
"domNodes": -279,
"jsHeapTotalBytes": 21336064,
"scriptDurationMs": 171.491,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1730.0690000000714,
"styleRecalcs": 32,
"styleRecalcDurationMs": 14.185,
"layouts": 6,
"layoutDurationMs": 0.562,
"taskDurationMs": 380.431,
"heapDeltaBytes": -16154892,
"heapUsedBytes": 52634448,
"domNodes": 75,
"jsHeapTotalBytes": 21233664,
"scriptDurationMs": 27.037,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1744.9600000001055,
"styleRecalcs": 31,
"styleRecalcDurationMs": 19.086,
"layouts": 6,
"layoutDurationMs": 0.732,
"taskDurationMs": 433.03000000000003,
"heapDeltaBytes": -16071828,
"heapUsedBytes": 52523244,
"domNodes": 76,
"jsHeapTotalBytes": 20971520,
"scriptDurationMs": 34.803,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 803.5069999999678,
"styleRecalcs": 9,
"styleRecalcDurationMs": 6.660000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 485.74800000000005,
"heapDeltaBytes": -2283172,
"heapUsedBytes": 66555240,
"domNodes": 14,
"jsHeapTotalBytes": 21233664,
"scriptDurationMs": 42.667,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 718.0959999999459,
"styleRecalcs": 8,
"styleRecalcDurationMs": 5.143,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 422.137,
"heapDeltaBytes": -2258504,
"heapUsedBytes": 66570184,
"domNodes": 12,
"jsHeapTotalBytes": 21495808,
"scriptDurationMs": 36.441,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2062.8829999999994,
"styleRecalcs": 6,
"styleRecalcDurationMs": 5.274000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 833.49,
"heapDeltaBytes": -346880,
"heapUsedBytes": 62676584,
"domNodes": -279,
"jsHeapTotalBytes": 1933312,
"scriptDurationMs": 310.607,
"eventListeners": -183,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2046.1540000000014,
"styleRecalcs": 7,
"styleRecalcDurationMs": 6.366999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 833.7669999999999,
"heapDeltaBytes": -202152,
"heapUsedBytes": 62708904,
"domNodes": -280,
"jsHeapTotalBytes": 2195456,
"scriptDurationMs": 315.083,
"eventListeners": -183,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 3764.9749999999926,
"styleRecalcs": 66,
"styleRecalcDurationMs": 10.376999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2925.1809999999996,
"heapDeltaBytes": 12839736,
"heapUsedBytes": 79315696,
"domNodes": -284,
"jsHeapTotalBytes": 4485120,
"scriptDurationMs": 1380.378,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 3756.962000000044,
"styleRecalcs": 66,
"styleRecalcDurationMs": 10.78,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2923.4900000000002,
"heapDeltaBytes": 22447040,
"heapUsedBytes": 87729528,
"domNodes": -285,
"jsHeapTotalBytes": 4485120,
"scriptDurationMs": 1364.851,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-zoom",
"durationMs": 5068.934000000013,
"styleRecalcs": 69,
"styleRecalcDurationMs": 13.301,
"layouts": 60,
"layoutDurationMs": 7.026999999999999,
"taskDurationMs": 3022.907,
"heapDeltaBytes": -13501052,
"heapUsedBytes": 58194200,
"domNodes": -265,
"jsHeapTotalBytes": 3244032,
"scriptDurationMs": 1457.774,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-zoom",
"durationMs": 5015.381999999931,
"styleRecalcs": 67,
"styleRecalcDurationMs": 11.604,
"layouts": 60,
"layoutDurationMs": 7.165,
"taskDurationMs": 3041.7300000000005,
"heapDeltaBytes": -15541900,
"heapUsedBytes": 57169248,
"domNodes": -266,
"jsHeapTotalBytes": 4030464,
"scriptDurationMs": 1454.66,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "legacy-node-drag",
"durationMs": 4118.222000000003,
"styleRecalcs": 41,
"styleRecalcDurationMs": 4.166999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2771.282,
"heapDeltaBytes": -15354408,
"heapUsedBytes": 57580564,
"domNodes": -291,
"jsHeapTotalBytes": 9011200,
"scriptDurationMs": 719.416,
"eventListeners": -141,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "legacy-node-drag",
"durationMs": 4184.530999999993,
"styleRecalcs": 42,
"styleRecalcDurationMs": 4.863000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2860.0789999999997,
"heapDeltaBytes": -11961700,
"heapUsedBytes": 59796876,
"domNodes": -286,
"jsHeapTotalBytes": 10846208,
"scriptDurationMs": 764.1360000000001,
"eventListeners": -108,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2030.8320000000322,
"styleRecalcs": 5,
"styleRecalcDurationMs": 4.0310000000000015,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 799.065,
"heapDeltaBytes": 5208676,
"heapUsedBytes": 71176740,
"domNodes": -285,
"jsHeapTotalBytes": 2981888,
"scriptDurationMs": 304.161,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "minimap-idle",
"durationMs": 2044.793000000027,
"styleRecalcs": 4,
"styleRecalcDurationMs": 3.6130000000000018,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 940.9830000000001,
"heapDeltaBytes": 4525092,
"heapUsedBytes": 70123252,
"domNodes": -284,
"jsHeapTotalBytes": 2981888,
"scriptDurationMs": 340.32199999999995,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 745.9809999999152,
"styleRecalcs": 46,
"styleRecalcDurationMs": 9.439,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 530.386,
"heapDeltaBytes": -5152808,
"heapUsedBytes": 64043828,
"domNodes": 18,
"jsHeapTotalBytes": 22020096,
"scriptDurationMs": 136.967,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 721.178000000009,
"styleRecalcs": 45,
"styleRecalcDurationMs": 8.993999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 513.298,
"heapDeltaBytes": -4979232,
"heapUsedBytes": 64455920,
"domNodes": 16,
"jsHeapTotalBytes": 22544384,
"scriptDurationMs": 133.943,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2016.5660000000116,
"styleRecalcs": 7,
"styleRecalcDurationMs": 7.146,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 553.2669999999999,
"heapDeltaBytes": -19226672,
"heapUsedBytes": 49974392,
"domNodes": -299,
"jsHeapTotalBytes": 18583552,
"scriptDurationMs": 19.533,
"eventListeners": -203,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2024.8370000000477,
"styleRecalcs": 7,
"styleRecalcDurationMs": 6.2989999999999995,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 502.946,
"heapDeltaBytes": -20290852,
"heapUsedBytes": 49271880,
"domNodes": -300,
"jsHeapTotalBytes": 19107840,
"scriptDurationMs": 18.497,
"eventListeners": -203,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1864.4429999999943,
"styleRecalcs": 77,
"styleRecalcDurationMs": 36.05500000000001,
"layouts": 16,
"layoutDurationMs": 4.0969999999999995,
"taskDurationMs": 1109.494,
"heapDeltaBytes": -16313400,
"heapUsedBytes": 52935668,
"domNodes": -298,
"jsHeapTotalBytes": 18583552,
"scriptDurationMs": 117.201,
"eventListeners": -203,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1869.690999999989,
"styleRecalcs": 76,
"styleRecalcDurationMs": 37.41,
"layouts": 16,
"layoutDurationMs": 4.005,
"taskDurationMs": 1144.622,
"heapDeltaBytes": -11726768,
"heapUsedBytes": 57531352,
"domNodes": -298,
"jsHeapTotalBytes": 19894272,
"scriptDurationMs": 124.274,
"eventListeners": -203,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-transition-enter",
"durationMs": 1405.0340000000006,
"styleRecalcs": 19,
"styleRecalcDurationMs": 29.505999999999997,
"layouts": 15,
"layoutDurationMs": 12.011999999999999,
"taskDurationMs": 924.8389999999999,
"heapDeltaBytes": 28444620,
"heapUsedBytes": 94324708,
"domNodes": 13671,
"jsHeapTotalBytes": 13369344,
"scriptDurationMs": 45.19300000000001,
"eventListeners": 2381,
"totalBlockingTimeMs": 125,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 13486.255999999968,
"styleRecalcs": 247,
"styleRecalcDurationMs": 33.342000000000006,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 10166.205000000002,
"heapDeltaBytes": 19807664,
"heapUsedBytes": 82995868,
"domNodes": -280,
"jsHeapTotalBytes": 4485120,
"scriptDurationMs": 4722.9349999999995,
"eventListeners": -125,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333338,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 13851.076000000034,
"styleRecalcs": 248,
"styleRecalcDurationMs": 34.33899999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 10160.584,
"heapDeltaBytes": 10879356,
"heapUsedBytes": 74093080,
"domNodes": -276,
"jsHeapTotalBytes": 4747264,
"scriptDurationMs": 4760.816,
"eventListeners": -123,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.669999999999952,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 17171.208000000093,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17152.658000000003,
"heapDeltaBytes": -36576580,
"heapUsedBytes": 153748812,
"domNodes": -6548,
"jsHeapTotalBytes": 4251648,
"scriptDurationMs": 1157.511,
"eventListeners": -7471,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-idle",
"durationMs": 18013.738999999987,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17992.87,
"heapDeltaBytes": -40955260,
"heapUsedBytes": 164919296,
"domNodes": -6548,
"jsHeapTotalBytes": 3465216,
"scriptDurationMs": 1233.578,
"eventListeners": -7471,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.333333333333332,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 21130.663000000026,
"styleRecalcs": 144,
"styleRecalcDurationMs": 16.590999999999966,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 21114.583,
"heapDeltaBytes": -37717240,
"heapUsedBytes": 167830400,
"domNodes": -6552,
"jsHeapTotalBytes": 5070848,
"scriptDurationMs": 1760.699,
"eventListeners": -7467,
"totalBlockingTimeMs": 306,
"frameDurationMs": 17.776666666666763,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 21878.010000000017,
"styleRecalcs": 160,
"styleRecalcDurationMs": 20.576999999999956,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 21859.918,
"heapDeltaBytes": -22511756,
"heapUsedBytes": 167439792,
"domNodes": -6548,
"jsHeapTotalBytes": 3203072,
"scriptDurationMs": 1791.7069999999999,
"eventListeners": -7463,
"totalBlockingTimeMs": 549,
"frameDurationMs": 18.886666666666617,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "workflow-execution",
"durationMs": 468.2319999999436,
"styleRecalcs": 18,
"styleRecalcDurationMs": 26.543000000000003,
"layouts": 4,
"layoutDurationMs": 1.3670000000000002,
"taskDurationMs": 149.05400000000003,
"heapDeltaBytes": -14056064,
"heapUsedBytes": 52202440,
"domNodes": 126,
"jsHeapTotalBytes": 9699328,
"scriptDurationMs": 10.986,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "workflow-execution",
"durationMs": 466.8500000000222,
"styleRecalcs": 14,
"styleRecalcDurationMs": 21.677000000000003,
"layouts": 3,
"layoutDurationMs": 0.769,
"taskDurationMs": 132.31199999999998,
"heapDeltaBytes": -14150332,
"heapUsedBytes": 52153216,
"domNodes": 121,
"jsHeapTotalBytes": 9961472,
"scriptDurationMs": 11.124000000000002,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
}
]
} |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## feature/ecs-migration #15017 +/- ##
========================================================
Coverage ? 81.08%
========================================================
Files ? 1898
Lines ? 109789
Branches ? 34638
========================================================
Hits ? 89021
Misses ? 20377
Partials ? 391
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
084a8fc to
b3850f1
Compare
dosubot
Bot
added
size:XXL
DrJKL
changed the title
dosubot
Bot
added
size:XL
f5da9ff to
588402b
Compare
827386f to
66f9af2
Compare
dosubot
Bot
added
size:L
66f9af2 to
a43634e
Compare
dosubot
Bot
added
size:XL
Retargeted squash of the layout-source stack onto the pruned attachment rework: useLayoutMutations(source) bakes the source when instantiated, operations carry their source explicitly, the store stamps the session actor on operations that do not carry one, and the ambient setSource/setActor/getCurrentSource/getCurrentActor knobs are removed. DOM height convention is passed explicitly through batchUpdateNodeBounds options instead of sniffed from an ambient DOM source.
DrJKL
changed the title
benjcooley
left a comment
benjcooley
left a comment
There was a problem hiding this comment.
Review summary
Read the whole layout/attachment path against LGraph, LGraphGroup, LGraphNode, Reroute, layoutStore, and the Vue-node consumers. The core design holds up: instance-bound attachment descriptors are a genuine improvement over id-derived registration, the topology-before-layout ordering in _addReroute is correct, and routing root teardown through #15009's teardownOwnedGraphs + resetAfterClear (rather than a second clear) is the right call. Deep Yjs observation plus a single afterTransaction geometry emit is also a real fix — group- and reroute-only edits now invalidate rendering, which the old nodeIds.length === 0 guard in notifyLayoutChanges swallowed.
No blocking defects found. Findings below are correctness-adjacent hardening, not crashes.
| Severity | Count |
|---|---|
| BLOCKER | 0 |
| SHOULD FIX | 3 |
| TRIVIAL | 2 |
SHOULD FIX
adoptNodeAttachmentbails without recording the attachment whenreadNodeRectfails, after thecreateNodeop has already been committed — orphan store entry plus a node whose geometry writes silently no-op. This branch is also load-bearing for the inert test mock.syncReroutePositionFromLayout/syncGroupBoundsFromLayoutarevoid x.pos[0]— the "canonical geometry survives removal" guarantee rests entirely on a Proxygettrap side effect. Both classes already have a real sync method.LGraph.addgroup branch: the re-mint retry loop was replaced with a single mint, while the collision check moved tolayoutStore. Check and mint now consult different sources, and the failure mode is silent geometry adoption rather than a duplicate id.
TRIVIAL
__mocks__/layoutStore.tsdroppedclearGraph, still called unconditionally byLGraph.resetAfterClear().- Orphaned JSDoc left above
storedRectScratchinLGraphNode.ts. - Not inline (line outside the diff):
Reroute.posgetter doc atReroute.ts:119-120still claims "the reroute registers itself on construction, so there is always an entry to read." Registration now happens at_addReroute, andstoredPositionhas a local fallback. Please update.
Verified, no action
LayoutSource.DOM→Vue+ explicitboundsIncludeTitleHeightis behaviour-preserving:batchUpdateNodeBoundsalready forcedVueon the emitted change, and title-height normalization is applied at exactly the one former DOM call site (useVueNodeResizeTracking).Externalhad no remaining producer.getNodeLayoutRefreturning aComputedRefis safe — every caller is read-only, and on Vue 3.5 computeds are not effect-scope-stopped, so the new caching cannot go stale on unmount.- Node-scoped listeners moving from synchronous to microtask-deferred affects only
LGraphNode.vue's CSS-var sync; the newhasNodeListenergate strictly widens delivery versus the oldchangeListeners.size === 0check. detachGraphLayoutssubgraph recursion is equivalent to the oldisRootGraphbranch, and dropping theif (!graph.rootGraph) returnguard is fine now thatrootGraphis no longer read during collection.
| } | ||
|
|
||
| function adoptNodeAttachment(graphId: UUID, node: LGraphNode): void { | ||
| if (!layoutStore.readNodeRect(graphId, node.id, node._posSize)) return |
There was a problem hiding this comment.
SHOULD FIX — silent orphan when readNodeRect fails.
attachNodeLayout has already submitted the createNode operation by the time it calls adoptNodeAttachment. If readNodeRect returns false here, the function returns without recording the attachment, so:
- the store keeps a
createNodeentry that nothing owns —detachNodeLayout/detachGraphLayoutsboth key off the WeakMap, so it is never deleted; - the node's
pos/sizewrites go throughmoveNodeLayout/resizeNodeLayout, which no-op without an attachment, so the node is silently immovable.
Note this branch is also what makes the inert __mocks__/layoutStore work (readNodeRect: vi.fn(() => false)), so production control flow is currently shaped by a test double. Record the attachment unconditionally (the rect read is only a geometry sync), or make the failure loud.
| * the store position into the reroute's local buffer before detach. | ||
| */ | ||
| function syncReroutePositionFromLayout(reroute: Reroute): void { | ||
| void reroute.pos[0] |
There was a problem hiding this comment.
SHOULD FIX — geometry preservation on detach depends on a side-effecting property read.
void reroute.pos[0] (and void group.pos[0] below) works only because pos returns a createGeometryView Proxy whose get trap calls synchronize(). Nothing at this call site says that, and the whole "canonical geometry survives removal" guarantee rests on it. Anything that makes pos return a plain array, memoizes the getter, or moves the sync onto a different accessor breaks detach silently.
Both classes already have the real method — Reroute.syncPosition() and LGraphGroup.syncBoundsFromStore(). Expose one internal sync entry point per class and call it directly instead of reading an index for its side effect.
| ) { | ||
| do node.id = mintGroupId(state) | ||
| while (rootGroups.some((group) => group.id === node.id)) | ||
| node.id = mintGroupId(state) |
There was a problem hiding this comment.
SHOULD FIX — the collision retry loop was dropped, and losing now means silent geometry adoption rather than a bad id.
Old code re-minted in a do/while until the id was free. The single mint is safe today only because observeGroupId keeps state.lastGroupId above every live group id — but the check on the line above is against layoutStore, not state. Those two can disagree (any group layout entry that outlives its group), and when they do attachGroupLayout takes the adopt branch: the new group silently inherits the other group's bounds and both instances share one store entry.
Restoring the loop keeps the check and the mint consistent:
| node.id = mintGroupId(state) | |
| do node.id = mintGroupId(state) | |
| while (layoutStore.getGroupLayout(this.rootGraph.id, node.id)) |
There was a problem hiding this comment.
We don't want to have to keep doing scans for things like the next valid ID, that's why we're centralizing them. If there are any places that mint without updating the lastX value we should eliminate them.
| allocateZIndex: vi.fn(() => 1), | ||
| applyOperation: vi.fn(), | ||
| applyOperation: vi.fn(() => 'applied'), | ||
| applyOperations: vi.fn(() => 'applied'), |
There was a problem hiding this comment.
TRIVIAL — clearGraph was removed from the mock, but LGraph.resetAfterClear() still calls layoutStore.clearGraph(graphId) unconditionally for any root graph with a real id. No current test using this mock clears or reconfigures a root graph, so CI is green, but the first one that does gets clearGraph is not a function rather than an inert no-op. Keep the stub.
| @@ -132,7 +134,6 @@ import type { WidgetTypeMap } from './widgets/widgetMap' | |||
| export type NodeProperty = string | number | boolean | object | null | |||
|
|
|||
| /** Captures only the {@link layoutStore} singleton, so shared across nodes. */ | |||
There was a problem hiding this comment.
TRIVIAL — orphaned doc comment. const layoutMutations = useLayoutMutations() is gone; this JSDoc now floats above storedRectScratch, which it does not describe. Delete it.
benjcooley
left a comment
benjcooley
left a comment
There was a problem hiding this comment.
Have agents check the should fixes. Looks like they're comments on some of the test code.
Split 5/6 of #14480. Stacked on #15017. Node replacement bypasses `graph.add()`. Copying `pos` and `size` preserved legacy canvas geometry only; the replacement did not own the existing layout attachment and could lose its position when Vue Nodes read from the layout store. This change centralizes replacement ownership transfer in the existing node-replacement subsystem: - Preflights graph indices, node-state store ownership, same-ID identity, and layout attachment before mutation. - Runs the existing `onRemoved` callback while ownership is unchanged, then revalidates before committing. - Moves the existing registered node-state proxy and layout attachment to the replacement without delete/recreate windows. - Reads canonical geometry before stamping the replacement’s geometry version. - Detaches the old instance from graph, state, and layout ownership so retained stale references cannot mutate the replacement. - Keeps orchestration private to `useNodeReplacement`; no new public `LGraph` mutation API. The browser regression performs replacement, drags the node on the legacy canvas, enables Vue Nodes, and verifies the rendered position remains within 2px. Unit coverage verifies exact state transfer, graph identity preservation, stale-instance isolation, mismatched ownership rejection, and latest-geometry adoption. Test plan: - 178 focused node replacement, node store, LGraph, and LGraphNode tests - application and browser typechecks - changed-file lint/format and diff checks --------- Co-authored-by: Amp <amp@ampcode.com>
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
5-agent parallel review (Bug Hunter, Architecture, Regression Risk, Error Handling, Test Quality). 35 raw findings → 19 after dedup.
Two issues that together undermine the PR's stated goals:
applyOperationdocumented contract doesn't match implementation — no reentrancy guard, no return value, callers can't detect silent no-ops- Module-level
layoutStoresingleton coupling not eliminated — same testability problem one indirection away
Critical
[C1] applyOperation returns void — PR description claims 'applied' | 'no-op' | 'rejected'
src/renderer/core/layout/store/layoutStore.ts — applyOperation signature
todo: PR description and context state applyOperation returns 'applied' | 'no-op' | 'rejected' with a console.warn on reentrant writes. The actual implementation returns void; the internal applied boolean is only used to gate finalizeOperation. No reentrancy guard exists, no console.warn fires on rejection, and no caller can detect a bounced operation. The __mocks__/layoutStore.ts returns 'applied' (string) — a type mismatch that misleads future callers. Every applyOperation call in graphLayoutAttachment.ts is fire-and-forget with no ability to detect silent no-ops (e.g. duplicate createNode).
Fix: either (a) implement the documented return type — return 'applied' | 'no-op' from the public method, surfacing the existing applied boolean; or (b) remove the claim from the PR description and update the mock to match void. If a reentrancy guard is desired, add it now or file a follow-up; the description currently implies it exists.
[C2] Module-level layoutStore singleton coupling not eliminated
src/lib/litegraph/src/LGraphNode.ts, LGraphGroup.ts, Reroute.ts, graphLayoutAttachment.ts
todo: The PR removes const layoutMutations = useLayoutMutations() at module scope (good), but layoutStore is still imported and accessed at module scope inside these files (e.g. LGraphGroup.ts calls layoutStore.getGroupLayout inside setBounds). Any test importing these classes still triggers layoutStore singleton construction and requires a Pinia context before module load — the same class of problem the PR aimed to fix, one level of indirection away.
Fix: document explicitly that the layoutStore singleton import is intentional and permanent (not a future injection target), or add it to the exceptions log if injection is planned. If it is a migration constraint, state the sunset condition.
Major
[M1] adoptExisting path silently discards canonical store geometry
src/renderer/core/layout/operations/graphLayoutAttachment.ts:367–373
issue: attachNodeLayout calls detachNodeLayout(node) when a prior attachment exists, issuing a deleteNode to the store. In the adoptExisting scenario the preceding detachNodeLayout already wiped the entry. The subsequent layoutStore.getNodeLayout(...) check is now false, so adoptNodeAttachment is skipped and a fresh createNode is issued from node._pos/_size — silently discarding whatever canonical geometry was stored.
Fix: read and cache any pre-existing layout entry BEFORE calling detachNodeLayout. After detach, either restore via the cache or bypass the delete when re-attaching to the same graph. Minimum viable: if nodeAttachments.get(node)?.graphId === graphId, call adoptNodeAttachment directly without delete-then-create.
[M2] Reroute pos setter silently drops writes before materializeRerouteLayout
src/lib/litegraph/src/Reroute.ts:960–965, LGraph.ts:1407
issue: The pos setter calls moveRerouteLayout(this, ...), which looks up rerouteAttachments.get(reroute). If pos is set on a Reroute constructed but not yet added to a graph (e.g. during deserialization), the attachment is absent and the write is silently dropped. The old code called createReroute in the constructor so the store always had an entry. Additionally, materializeRerouteLayout has no pre-check: if a store entry already exists, the supplied initial position is silently ignored and the reroute's in-memory array is out of sync until syncPosition() is called.
Fix: either buffer pos writes until materializeRerouteLayout runs, or document that pre-attachment writes are intentionally local-only and audit all deserialization call sites. In materializeRerouteLayout, when a store entry already exists, call reroute.syncPosition() to reconcile (mirror the adoptNodeAttachment pattern).
[M3] notifyNodeChange throw silences notifyChange for the same change
src/renderer/core/layout/store/layoutStore.ts — microtask flush in queueChange
issue: The microtask flush loop calls this.notifyNodeChange(queuedChange) then this.notifyChange(queuedChange) without a try/catch around the first call. If any onNodeChange listener throws, notifyChange for that entry is never called — global onChange listeners (including notifyLayoutChanges → canvas dirty) miss the change entirely. The queueGeometryChange equivalent wraps each listener in try/catch; this loop does not.
Fix:
queuedChanges.forEach((queuedChange) => {
try { this.notifyNodeChange(queuedChange) } catch (e) { console.error(e) }
this.notifyChange(queuedChange)
})[M4] detachGraphLayouts leaves partially-detached state on exception mid-walk
src/renderer/core/layout/operations/graphLayoutAttachment.ts:1553–1611
issue: collect() calls group.syncBoundsFromStore() and reroute.syncPosition() before clearing WeakMap entries. If either throws, the WeakMap entry is NOT cleared (leaked attachment) and the delete operation is never queued. On the next detachGraphLayouts call the attachment re-appears and a redundant delete is attempted, but the WeakMap leak is permanent until GC.
Fix: clear the WeakMap entry and set _layoutRegistered = false unconditionally first; push delete operations unconditionally; wrap sync calls in try/catch separately.
[M5] Deeply nested subgraph trees may not be fully detached
src/renderer/core/layout/operations/graphLayoutAttachment.ts:1607
issue: collect(owner) recurses via owner._subgraphs.values(). If _subgraphs on a non-root subgraph returns only its direct children (not the root's flat map), sub-sub-graphs are never detached. The visited-set prevents infinite loops but does not guarantee exhaustiveness. No test exercises a 2+-level-deep subgraph chain.
Fix: confirm whether _subgraphs on a non-root subgraph returns the root's flat map or the subgraph's own children. Add a test: root → subgraph A → subgraph B, nodes in all three, trigger teardown, assert all layout entries gone. Add a cycle test for the visited-set.
[M6] clear() graph-id remint: pending geometry notifications may misidentify graph
src/lib/litegraph/src/LGraph.ts:578–583
issue: layoutStore.clearGraph(oldId) runs, then this.id = newUuid. Geometry changes queued before clear() may fire as microtasks after the id changes: notifyLayoutChanges checks graphIds.has(graph.rootGraph.id), which now returns the new UUID. Pending pendingGeometryChanges sets containing the old UUID fail the check → canvas dirty mark missed. Additionally, external code caching rootGraphId across clear() silently reads/writes the wrong bucket.
Fix: flush or drop pendingGeometryChanges before the id changes (call or mirror clearViewGeometry from within clear()). Document the id-remint in ADR-0003 or a comment at the remint site. Consider a graphIdChanged event for consumers that must re-subscribe.
[M7] attachGroupLayout does not detach prior attachment — old store entry orphaned
src/renderer/core/layout/operations/graphLayoutAttachment.ts (~line 423)
issue: Unlike attachNodeLayout (which calls detachNodeLayout first if a prior attachment exists), attachGroupLayout does not check groupAttachments.has(group) before writing the new entry. Re-adding a group to a second graph silently overwrites the old groupAttachments entry without emitting a deleteGroup for the old graphId. The old store entry persists as an orphan.
Fix: mirror the attachNodeLayout pattern — check groupAttachments.has(group) and call detachGroupLayout(group) before setting the new attachment.
Minor
[m1] layoutStore.ts:3258–3263 — observeDeep on yreroutes may project reroute at (0,0) if the creation event fires before the position field is written. Add a guard in projectReroute to skip projection when the position field is absent, or add a test confirming event order.
[m2] layoutStore.ts (cleanupNodeRef) — nodeChangeListeners not cleared on component unmount; only cleared by clearViewGeometry. With observeDeep amplifying nested field events, stale listeners now receive more calls than before. Fix: unregister the corresponding nodeChangeListeners entry in cleanupNodeRef.
[m3] graphLayoutAttachment.ts:358–363 — teardown operations hardcode LayoutSource.Canvas. In non-canvas contexts (server-side hydration, ECS systems), the source is wrong; listeners that key on source (e.g. notifyLayoutChanges skips Canvas-sourced resizes) will behave incorrectly. Fix: accept an optional source parameter defaulting to Canvas, or introduce LayoutSource.Lifecycle.
[m4] graphLayoutAttachment.ts:1495–1546 — moveNodeLayout, resizeNodeLayout, setGroupBoundsLayout, moveRerouteLayout all silently return if the entity has no WeakMap entry. Pre-attach writes are dropped with no indication — harder to debug than the old singleton behavior. Fix: add a console.warn when an entity is not attached and the operation is not during teardown.
[m5] layoutStore.ts:queueGeometryChange — when multiple Yjs transactions fire before the microtask drains, each listener is called once per pending set rather than once with the merged union. Side-effectful listeners are called unnecessarily multiple times per drain. Fix: merge all pending sets into a single Set<UUID> before dispatching, or document the multiple-call contract.
[m6] LGraphGroup.ts:165–174 — setBounds calls setGroupBoundsLayout (silent no-op if unattached) then syncBoundsFromStore() which may read null and reset local values to zero. Pre-attach setBounds calls during deserialization silently zero-out bounds. Fix: document or enforce that setBounds must not be called before attachGroupLayout; audit deserialization paths.
[m7] layoutStore.ts:61–73 (collectEventKeys) — no guard for YArrayEvent/YTextEvent nested inside maps (path[0] would be a number, coerced to string, never matching any ScopedLayoutKey). Currently can't occur but no safeguard against schema drift. Fix: add an else debug-log branch for unrecognised event shapes.
Nitpicks
- [N1]
LGraph.ts(clear()) —nodeRefs/nodeTriggerskeyed by old scoped keys accumulate acrossclear()cycles;clearGraphdeletes Yjs entries but not the Maps. Over many workflow reloads, stale entries grow unbounded. Fix: add aclearScopedRefs(oldGraphId)pass called fromclearGraph. - [N2]
layoutStore.ts:collectEventKeys—event.path[0]relies on Yjs 13.x path encoding being the parent-map string key. Correct but fragile with no documentation. Fix: add a comment referencing the Yjs path encoding contract, or derive the key fromevent.currentTarget. - [N3]
graphLayoutAttachment.ts(~line 366) — comment "If already attached, detach first" omits the cross-graph re-attach motivation, risking future removal as redundant. Fix: "Node may be moved between graphs; detach from the previous graph before re-attaching."
4 participants
Centralizes entity layout ownership and command submission on top of #15009's graph identity, collision, and teardown boundaries.
Nodes, groups, and reroutes now hold instance-bound
{ graphId, id }attachments. Only the attached instance can mutate canonical geometry; stale instances become inert without delete/recreate compensation.Changes:
graphLayoutAttachment.ts, covering attach, adopt, detach, geometry writes, and batched teardown.LayoutSource; the store stamps its session actor at submission. DOM measurement conventions are explicit rather than ambient process state.useLayoutMutationsto its four production behaviors and removes redundant commandentity, operation ID, and apply-result fields.Review focus:
graphLayoutAttachment.ts._addReroute.layoutStore.ts.Validation:
feature/ecs-migration.