refactor: extract node shell-state lifecycle into src/core/graph/nodeShell - #14257
refactor: extract node shell-state lifecycle into src/core/graph/nodeShell#14257mattmillerai wants to merge 3 commits into
Conversation
|
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 |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1987 passed, 0 failed · 2 flaky📊 Browser Reports
📦 Bundle Size
⚡ Performance Report
Absolute values
Raw data{
"timestamp": "2026-08-22T08:11:56.661Z",
"gitSha": "0c49f4f1551d9353d588237cfc3b4cd9677dfc17",
"branch": "matt/be-5050-node-shell-state",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2017.7740000000028,
"styleRecalcs": 7,
"styleRecalcDurationMs": 6.029,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 479.19300000000004,
"heapDeltaBytes": 11456020,
"heapUsedBytes": 80378844,
"domNodes": -283,
"jsHeapTotalBytes": 4186112,
"scriptDurationMs": 8.1,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2022.8839999999764,
"styleRecalcs": 9,
"styleRecalcDurationMs": 6.858999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 479.7130000000001,
"heapDeltaBytes": 6274204,
"heapUsedBytes": 75579396,
"domNodes": -280,
"jsHeapTotalBytes": 5234688,
"scriptDurationMs": 10.459,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2372.126000000037,
"styleRecalcs": 84,
"styleRecalcDurationMs": 48.209,
"layouts": 12,
"layoutDurationMs": 4.673,
"taskDurationMs": 1425.796,
"heapDeltaBytes": 5339504,
"heapUsedBytes": 74624608,
"domNodes": -279,
"jsHeapTotalBytes": 5496832,
"scriptDurationMs": 177.195,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2207.2070000000394,
"styleRecalcs": 79,
"styleRecalcDurationMs": 42.169,
"layouts": 12,
"layoutDurationMs": 3.703,
"taskDurationMs": 1226.747,
"heapDeltaBytes": 4653636,
"heapUsedBytes": 73680812,
"domNodes": -279,
"jsHeapTotalBytes": 4710400,
"scriptDurationMs": 149.565,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1708.3099999999831,
"styleRecalcs": 30,
"styleRecalcDurationMs": 15.267000000000001,
"layouts": 6,
"layoutDurationMs": 0.6970000000000002,
"taskDurationMs": 339.877,
"heapDeltaBytes": 4150424,
"heapUsedBytes": 73684624,
"domNodes": 74,
"jsHeapTotalBytes": 5505024,
"scriptDurationMs": 10.488,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1707.5179999999364,
"styleRecalcs": 31,
"styleRecalcDurationMs": 14.781,
"layouts": 6,
"layoutDurationMs": 0.5770000000000001,
"taskDurationMs": 343.011,
"heapDeltaBytes": 3713284,
"heapUsedBytes": 72983184,
"domNodes": 75,
"jsHeapTotalBytes": 5505024,
"scriptDurationMs": 10.624999999999998,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "dom-widget-clipping",
"durationMs": 746.1989999999901,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.362,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 432.40700000000004,
"heapDeltaBytes": -6895468,
"heapUsedBytes": 62113732,
"domNodes": 16,
"jsHeapTotalBytes": 6291456,
"scriptDurationMs": 35.672000000000004,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 705.1470000000108,
"styleRecalcs": 11,
"styleRecalcDurationMs": 6.7860000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 408.36400000000003,
"heapDeltaBytes": -7048412,
"heapUsedBytes": 61997460,
"domNodes": 18,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 30.967000000000002,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2025.8739999999875,
"styleRecalcs": 2,
"styleRecalcDurationMs": 1.6509999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 586.493,
"heapDeltaBytes": 8426104,
"heapUsedBytes": 72684500,
"domNodes": -274,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 41.853,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2031.1590000000024,
"styleRecalcs": 4,
"styleRecalcDurationMs": 3.231000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 566.989,
"heapDeltaBytes": -17904120,
"heapUsedBytes": 72705472,
"domNodes": -273,
"jsHeapTotalBytes": -2625536,
"scriptDurationMs": 38.595000000000006,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 3403.3489999999915,
"styleRecalcs": 66,
"styleRecalcDurationMs": 12.16,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2750.587,
"heapDeltaBytes": -22496324,
"heapUsedBytes": 69368420,
"domNodes": -285,
"jsHeapTotalBytes": 4976640,
"scriptDurationMs": 999.077,
"eventListeners": -181,
"totalBlockingTimeMs": 16,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 3272.472999999991,
"styleRecalcs": 66,
"styleRecalcDurationMs": 11.850999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2547.232,
"heapDeltaBytes": -13331440,
"heapUsedBytes": 78092188,
"domNodes": -281,
"jsHeapTotalBytes": 2056192,
"scriptDurationMs": 907.314,
"eventListeners": -151,
"totalBlockingTimeMs": 24,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-zoom",
"durationMs": 4367.262000000039,
"styleRecalcs": 66,
"styleRecalcDurationMs": 12.193000000000003,
"layouts": 60,
"layoutDurationMs": 7.997000000000001,
"taskDurationMs": 2507.905,
"heapDeltaBytes": 7958992,
"heapUsedBytes": 73806892,
"domNodes": 24,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 868.2639999999999,
"eventListeners": 12,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 4353.097000000048,
"styleRecalcs": 66,
"styleRecalcDurationMs": 13.097999999999999,
"layouts": 60,
"layoutDurationMs": 8.726,
"taskDurationMs": 2573.2219999999998,
"heapDeltaBytes": 8084976,
"heapUsedBytes": 74196300,
"domNodes": 24,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 877.59,
"eventListeners": 10,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.669999999999952,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "legacy-node-drag",
"durationMs": 3268.470999999977,
"styleRecalcs": 41,
"styleRecalcDurationMs": 5.836999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2103.8920000000003,
"heapDeltaBytes": -1280064,
"heapUsedBytes": 70707692,
"domNodes": -287,
"jsHeapTotalBytes": -565248,
"scriptDurationMs": 219.62500000000003,
"eventListeners": 29,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "legacy-node-drag",
"durationMs": 3277.6719999999386,
"styleRecalcs": 41,
"styleRecalcDurationMs": 5.710999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2097.4339999999997,
"heapDeltaBytes": 13520804,
"heapUsedBytes": 80580704,
"domNodes": 0,
"jsHeapTotalBytes": 1990656,
"scriptDurationMs": 215.12200000000004,
"eventListeners": 188,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666696,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2014.269000000013,
"styleRecalcs": 2,
"styleRecalcDurationMs": 1.417999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 494.8459999999999,
"heapDeltaBytes": 8744872,
"heapUsedBytes": 73340512,
"domNodes": -274,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 34.156000000000006,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2033.9549999999917,
"styleRecalcs": 4,
"styleRecalcDurationMs": 3.355999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 526.1360000000002,
"heapDeltaBytes": -19024404,
"heapUsedBytes": 73421704,
"domNodes": -271,
"jsHeapTotalBytes": -1576960,
"scriptDurationMs": 37.767,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 711.656000000005,
"styleRecalcs": 45,
"styleRecalcDurationMs": 8.757,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 507.04499999999996,
"heapDeltaBytes": 17355188,
"heapUsedBytes": 87592544,
"domNodes": 16,
"jsHeapTotalBytes": 6291456,
"scriptDurationMs": 135.53699999999998,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 713.5050000000547,
"styleRecalcs": 46,
"styleRecalcDurationMs": 10.068999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 512.9889999999999,
"heapDeltaBytes": 17927932,
"heapUsedBytes": 87796024,
"domNodes": 18,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 136.024,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2046.0140000000138,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.81,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 505.7750000000001,
"heapDeltaBytes": 4447620,
"heapUsedBytes": 74245216,
"domNodes": -294,
"jsHeapTotalBytes": 5758976,
"scriptDurationMs": 8.200000000000001,
"eventListeners": -231,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2007.7969999999823,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.055999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 468.045,
"heapDeltaBytes": 12362472,
"heapUsedBytes": 82031932,
"domNodes": -297,
"jsHeapTotalBytes": 4972544,
"scriptDurationMs": 7.1419999999999995,
"eventListeners": -201,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1892.723999999987,
"styleRecalcs": 78,
"styleRecalcDurationMs": 40.864000000000004,
"layouts": 16,
"layoutDurationMs": 4.598,
"taskDurationMs": 1137.802,
"heapDeltaBytes": -9408348,
"heapUsedBytes": 60354616,
"domNodes": -296,
"jsHeapTotalBytes": 6807552,
"scriptDurationMs": 113.322,
"eventListeners": -201,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1824.9989999999343,
"styleRecalcs": 79,
"styleRecalcDurationMs": 37.871,
"layouts": 16,
"layoutDurationMs": 4.447,
"taskDurationMs": 1088.924,
"heapDeltaBytes": -2116956,
"heapUsedBytes": 67757100,
"domNodes": -295,
"jsHeapTotalBytes": 5758976,
"scriptDurationMs": 107.482,
"eventListeners": -201,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-transition-enter",
"durationMs": 1384.7509999999374,
"styleRecalcs": 19,
"styleRecalcDurationMs": 29.272,
"layouts": 14,
"layoutDurationMs": 13.137,
"taskDurationMs": 890.6630000000002,
"heapDeltaBytes": 18029648,
"heapUsedBytes": 100431696,
"domNodes": 13671,
"jsHeapTotalBytes": 10485760,
"scriptDurationMs": 27.907,
"eventListeners": 2383,
"totalBlockingTimeMs": 120,
"frameDurationMs": 16.666666666666636,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 12507.689000000028,
"styleRecalcs": 245,
"styleRecalcDurationMs": 38.272,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 9409.396,
"heapDeltaBytes": 224304,
"heapUsedBytes": 91140484,
"domNodes": -271,
"jsHeapTotalBytes": 5431296,
"scriptDurationMs": 3303.468,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000073,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 12313.655000000039,
"styleRecalcs": 245,
"styleRecalcDurationMs": 38.307,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 8948.024,
"heapDeltaBytes": -13579108,
"heapUsedBytes": 77311528,
"domNodes": -278,
"jsHeapTotalBytes": 2023424,
"scriptDurationMs": 3066.749,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 18332.873000000007,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17724.681999999997,
"heapDeltaBytes": -27253768,
"heapUsedBytes": 179230068,
"domNodes": -6548,
"jsHeapTotalBytes": 5521408,
"scriptDurationMs": 358.82900000000006,
"eventListeners": -7473,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 18325.372000000018,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17626.747000000003,
"heapDeltaBytes": -33372824,
"heapUsedBytes": 177103912,
"domNodes": -6548,
"jsHeapTotalBytes": -2502656,
"scriptDurationMs": 363.093,
"eventListeners": -7473,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 22278.778000000046,
"styleRecalcs": 176,
"styleRecalcDurationMs": 21.120000000000026,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 21972.671,
"heapDeltaBytes": -28372704,
"heapUsedBytes": 164964864,
"domNodes": -6548,
"jsHeapTotalBytes": -704512,
"scriptDurationMs": 938.943,
"eventListeners": -7469,
"totalBlockingTimeMs": 51,
"frameDurationMs": 18.329999999999927,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 22419.179999999982,
"styleRecalcs": 177,
"styleRecalcDurationMs": 20.650000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 22013.689,
"heapDeltaBytes": -18923876,
"heapUsedBytes": 187802888,
"domNodes": -6548,
"jsHeapTotalBytes": -1101824,
"scriptDurationMs": 997.075,
"eventListeners": -7469,
"totalBlockingTimeMs": 73,
"frameDurationMs": 18.329999999999927,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "workflow-execution",
"durationMs": 496.2279999999737,
"styleRecalcs": 20,
"styleRecalcDurationMs": 23.173,
"layouts": 4,
"layoutDurationMs": 1.7280000000000002,
"taskDurationMs": 151.83,
"heapDeltaBytes": 6000572,
"heapUsedBytes": 72590432,
"domNodes": 150,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 9.841999999999999,
"eventListeners": 99,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "workflow-execution",
"durationMs": 458.0549999999448,
"styleRecalcs": 14,
"styleRecalcDurationMs": 22.576,
"layouts": 3,
"layoutDurationMs": 0.891,
"taskDurationMs": 123.628,
"heapDeltaBytes": 5431696,
"heapUsedBytes": 72319948,
"domNodes": 121,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 8.389,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
}
]
} |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## feature/ecs-migration #14257 +/- ##
======================================================
Coverage 82.30% 82.31%
======================================================
Files 1900 1902 +2
Lines 107652 107688 +36
Branches 34116 34068 -48
======================================================
+ Hits 88605 88641 +36
- Misses 18711 18718 +7
+ Partials 336 329 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
|
STACKED — merging lands on Flagging because this PR is now approved with all review threads resolved, so it reads as mergeable at a glance. Merging it does not ship to Resolution summary: the cursor-review panel's 3 findings all landed on code this PR only relocated.
Also verified the two tests dropped from |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
STACKED — merging lands on |
|
STACKED — merging lands on |
0098081 to
8a0f977
Compare
950e26e to
7ca4449
Compare
ef435ab to
20cd48c
Compare
7ca4449 to
af6567c
Compare
|
STACKED — merging lands on Rebased onto the current Already approved with all threads resolved — flagging the stack again since this push changed HEAD. |
1f09c53 to
fb4f0eb
Compare
fb4f0eb to
b00f2ff
Compare
**STACKED — merging lands on `matt/be-5050-node-shell-state` (#14257, which itself targets `feature/ecs-migration`), NOT `main`.** ## ELI-5 When a node joins or leaves a graph, the app files its "shell state" into a store so the renderer can draw it. Two ways that filing can go wrong today fail silently: filing a node that is already filed under a *different* workflow strands the old entry, and un-filing a state the store never held leaves a ghost the renderer keeps drawing. Both now trip an assertion (throws in DEV, reports via the assert reporter in prod) instead of drifting quietly. No behavior changes when the invariants hold. ## What changed `src/core/graph/nodeShell/nodeShellState.ts` - `registerNodeState` asserts `node._graphId` is unset or already equals `graph.rootGraph.id` before overwriting it. Membership is by state identity, so a node re-registered under a second root graph leaves its first bucket entry behind with nothing to remove it. - `unregisterNodeState` captures `deleteNode`'s boolean instead of discarding it and asserts it. `false` means the bucket did not contain `node._state` — identity drift, e.g. `_state` reassigned after registration. Both messages name the node id. `_graphId` is cleared before the assertion so a failing teardown still leaves the node detached rather than half-registered. ## Deliberately not shipped: the duplicate-`NodeState` assertion The third invariant asked for — asserting the store never holds two distinct `NodeState` objects for one `(graphId, id)` — was implemented (an auxiliary forward `Map<string, NodeState>` plus a `Map<NodeState, string>` reverse map per bucket, so a renumbered node still de-indexes by identity) and then **reverted**, because it is not a regression net on this base: it fires on legitimate existing behavior. Evidence, all from the suite on this branch: - `LGraph.serialise.test.ts > can (de)serialise node / group titles` — `new LGraph(data)` (`LGraph.ts:476` → `configure:2636` → `add:1092`) keeps the *persisted* graph id, so a round-tripped copy shares a bucket with the graph it was serialised from and collides on every node id. - `LGraph.test.ts > Link serialization goldens` (4 tests) and `LGraph.test.ts > deduplicateSubgraphNodeIds (via configure) > warns when configuring a host with legacy proxyWidgets…` fail the same way. That is 6 tests, green on this base and green again with the assertion removed — so this is the assertion's premise being wrong, not a latent bug those tests were hiding. `subgraphDeduplication.ts` guarantees id-uniqueness *within* one root graph; it says nothing about two live `LGraph` instances that carry the same id. Making duplicate ids assertable means giving each `LGraph` instance its own bucket identity rather than keying buckets by graph id — a separate change with real blast radius (`getGraphNodesFor` callers, `clearGraph`, the `canvasStore.rootGraphId` readers), not a rider on this one. `docs/architecture/node-data-store.md` records both the two shipped invariants and this gap. ## Tests `nodeShellState.test.ts` gains the two cases; both are red without the production change (verified by stashing it). The cross-root case needs an explicit `second.id = createUuidv4()` — two fresh `LGraph`s both sit at `zeroUuid` until `configure`, so they share a bucket and the invariant genuinely holds. ## Verification Targeted: `nodeShellState.test.ts`, `nodeDataStore.test.ts`, `useNodeReplacement.test.ts`, `LGraph.test.ts`, `LGraph.serialise.test.ts` — 82 tests pass, 0 fail. `oxfmt --check` clean. Full `vitest run` was executed and is the reason the third assertion was reverted (it caught the 6 failures above). That run is **not** a clean-suite claim: this worktree borrows the parent clone's `node_modules`, which predates the `minisearch` dependency this branch adds, so 222 suites failed to resolve imports for reasons unrelated to the diff. Of the tests that did run, the only failures attributable to the diff were the 6 named above; the rest (`assetService`, `useAssetGridSelection`, `onboardingCloudRoutes`, …) are the same load-dependent set #14257 reported. A reviewer should treat repo-wide CI as the authority here, not this local run. Repo-wide `pnpm typecheck` was likewise not run locally — the diff is 11 lines of source plus tests, and both changed files typecheck under vitest's transform.
71b9b9f to
13a302e
Compare
|
STACKED — merging lands on Restacked to clear the merge conflict. Details: The parent commit this branch carried ( Resolution: rebased the three One real conflict needed a decision: Net effect on the PR diff: 377 files → 13 files (+592/−213). That is the actual scope of this change; the rest was the duplicated parent. Verification on the rebased tree: Also squashed a small lint fix into |
**STACKED — merging lands on `matt/be-5050-node-shell-state` (#14257, which itself targets `feature/ecs-migration`), NOT `main`.** ## ELI-5 When a node joins or leaves a graph, the app files its "shell state" into a store so the renderer can draw it. Two ways that filing can go wrong today fail silently: filing a node that is already filed under a *different* workflow strands the old entry, and un-filing a state the store never held leaves a ghost the renderer keeps drawing. Both now trip an assertion (throws in DEV, reports via the assert reporter in prod) instead of drifting quietly. No behavior changes when the invariants hold. ## What changed `src/core/graph/nodeShell/nodeShellState.ts` - `registerNodeState` asserts `node._graphId` is unset or already equals `graph.rootGraph.id` before overwriting it. Membership is by state identity, so a node re-registered under a second root graph leaves its first bucket entry behind with nothing to remove it. - `unregisterNodeState` captures `deleteNode`'s boolean instead of discarding it and asserts it. `false` means the bucket did not contain `node._state` — identity drift, e.g. `_state` reassigned after registration. Both messages name the node id. `_graphId` is cleared before the assertion so a failing teardown still leaves the node detached rather than half-registered. ## Deliberately not shipped: the duplicate-`NodeState` assertion The third invariant asked for — asserting the store never holds two distinct `NodeState` objects for one `(graphId, id)` — was implemented (an auxiliary forward `Map<string, NodeState>` plus a `Map<NodeState, string>` reverse map per bucket, so a renumbered node still de-indexes by identity) and then **reverted**, because it is not a regression net on this base: it fires on legitimate existing behavior. Evidence, all from the suite on this branch: - `LGraph.serialise.test.ts > can (de)serialise node / group titles` — `new LGraph(data)` (`LGraph.ts:476` → `configure:2636` → `add:1092`) keeps the *persisted* graph id, so a round-tripped copy shares a bucket with the graph it was serialised from and collides on every node id. - `LGraph.test.ts > Link serialization goldens` (4 tests) and `LGraph.test.ts > deduplicateSubgraphNodeIds (via configure) > warns when configuring a host with legacy proxyWidgets…` fail the same way. That is 6 tests, green on this base and green again with the assertion removed — so this is the assertion's premise being wrong, not a latent bug those tests were hiding. `subgraphDeduplication.ts` guarantees id-uniqueness *within* one root graph; it says nothing about two live `LGraph` instances that carry the same id. Making duplicate ids assertable means giving each `LGraph` instance its own bucket identity rather than keying buckets by graph id — a separate change with real blast radius (`getGraphNodesFor` callers, `clearGraph`, the `canvasStore.rootGraphId` readers), not a rider on this one. `docs/architecture/node-data-store.md` records both the two shipped invariants and this gap. ## Tests `nodeShellState.test.ts` gains the two cases; both are red without the production change (verified by stashing it). The cross-root case needs an explicit `second.id = createUuidv4()` — two fresh `LGraph`s both sit at `zeroUuid` until `configure`, so they share a bucket and the invariant genuinely holds. ## Verification Targeted: `nodeShellState.test.ts`, `nodeDataStore.test.ts`, `useNodeReplacement.test.ts`, `LGraph.test.ts`, `LGraph.serialise.test.ts` — 82 tests pass, 0 fail. `oxfmt --check` clean. Full `vitest run` was executed and is the reason the third assertion was reverted (it caught the 6 failures above). That run is **not** a clean-suite claim: this worktree borrows the parent clone's `node_modules`, which predates the `minisearch` dependency this branch adds, so 222 suites failed to resolve imports for reasons unrelated to the diff. Of the tests that did run, the only failures attributable to the diff were the 6 named above; the rest (`assetService`, `useAssetGridSelection`, `onboardingCloudRoutes`, …) are the same load-dependent set #14257 reported. A reviewer should treat repo-wide CI as the authority here, not this local run. Repo-wide `pnpm typecheck` was likewise not run locally — the diff is 11 lines of source plus tests, and both changed files typecheck under vitest's transform.
6ae10d9 to
811a2a5
Compare
|
STACKED — merging lands on Already approved with all threads resolved, so it reads as mergeable at a glance — flagging the stack again because this push changes HEAD. Merged the current
Post-merge verification: Diff vs. base is unchanged in scope: still the same 13 files. |
**STACKED — merging lands on `matt/be-5050-node-shell-state` (#14257, which itself targets `feature/ecs-migration`), NOT `main`.** ## ELI-5 When a node joins or leaves a graph, the app files its "shell state" into a store so the renderer can draw it. Two ways that filing can go wrong today fail silently: filing a node that is already filed under a *different* workflow strands the old entry, and un-filing a state the store never held leaves a ghost the renderer keeps drawing. Both now trip an assertion (throws in DEV, reports via the assert reporter in prod) instead of drifting quietly. No behavior changes when the invariants hold. ## What changed `src/core/graph/nodeShell/nodeShellState.ts` - `registerNodeState` asserts `node._graphId` is unset or already equals `graph.rootGraph.id` before overwriting it. Membership is by state identity, so a node re-registered under a second root graph leaves its first bucket entry behind with nothing to remove it. - `unregisterNodeState` captures `deleteNode`'s boolean instead of discarding it and asserts it. `false` means the bucket did not contain `node._state` — identity drift, e.g. `_state` reassigned after registration. Both messages name the node id. `_graphId` is cleared before the assertion so a failing teardown still leaves the node detached rather than half-registered. ## Deliberately not shipped: the duplicate-`NodeState` assertion The third invariant asked for — asserting the store never holds two distinct `NodeState` objects for one `(graphId, id)` — was implemented (an auxiliary forward `Map<string, NodeState>` plus a `Map<NodeState, string>` reverse map per bucket, so a renumbered node still de-indexes by identity) and then **reverted**, because it is not a regression net on this base: it fires on legitimate existing behavior. Evidence, all from the suite on this branch: - `LGraph.serialise.test.ts > can (de)serialise node / group titles` — `new LGraph(data)` (`LGraph.ts:476` → `configure:2636` → `add:1092`) keeps the *persisted* graph id, so a round-tripped copy shares a bucket with the graph it was serialised from and collides on every node id. - `LGraph.test.ts > Link serialization goldens` (4 tests) and `LGraph.test.ts > deduplicateSubgraphNodeIds (via configure) > warns when configuring a host with legacy proxyWidgets…` fail the same way. That is 6 tests, green on this base and green again with the assertion removed — so this is the assertion's premise being wrong, not a latent bug those tests were hiding. `subgraphDeduplication.ts` guarantees id-uniqueness *within* one root graph; it says nothing about two live `LGraph` instances that carry the same id. Making duplicate ids assertable means giving each `LGraph` instance its own bucket identity rather than keying buckets by graph id — a separate change with real blast radius (`getGraphNodesFor` callers, `clearGraph`, the `canvasStore.rootGraphId` readers), not a rider on this one. `docs/architecture/node-data-store.md` records both the two shipped invariants and this gap. ## Tests `nodeShellState.test.ts` gains the two cases; both are red without the production change (verified by stashing it). The cross-root case needs an explicit `second.id = createUuidv4()` — two fresh `LGraph`s both sit at `zeroUuid` until `configure`, so they share a bucket and the invariant genuinely holds. ## Verification Targeted: `nodeShellState.test.ts`, `nodeDataStore.test.ts`, `useNodeReplacement.test.ts`, `LGraph.test.ts`, `LGraph.serialise.test.ts` — 82 tests pass, 0 fail. `oxfmt --check` clean. Full `vitest run` was executed and is the reason the third assertion was reverted (it caught the 6 failures above). That run is **not** a clean-suite claim: this worktree borrows the parent clone's `node_modules`, which predates the `minisearch` dependency this branch adds, so 222 suites failed to resolve imports for reasons unrelated to the diff. Of the tests that did run, the only failures attributable to the diff were the 6 named above; the rest (`assetService`, `useAssetGridSelection`, `onboardingCloudRoutes`, …) are the same load-dependent set #14257 reported. A reviewer should treat repo-wide CI as the authority here, not this local run. Repo-wide `pnpm typecheck` was likewise not run locally — the diff is 11 lines of source plus tests, and both changed files typecheck under vitest's transform.
eef58cb to
90d9ee2
Compare
…Shell Move the app-owned node shell-state lifecycle coordination out of the litegraph folder into an app-layer module, leaving thin call sites in litegraph. No behaviour change. - createNodeShellState / setTrackedNodeState / registerNodeState / unregisterNodeState / unregisterAllNodeStates now live in src/core/graph/nodeShell/nodeShellState.ts - attachNodeToStores / releaseGraphStores in nodeShellLifecycle.ts are the single calls LGraph.add and LGraph.clear make into the app stores - LGraphNode no longer imports useNodeDataStore - the litegraph barrel no longer re-exports the registration functions Amp-Thread-ID: https://ampcode.com/threads/T-01a027af-1c32-7532-84f9-b94aa90620fa
**STACKED — merging lands on `matt/be-5050-node-shell-state` (#14257, which itself targets `feature/ecs-migration`), NOT `main`.** ## ELI-5 When a node joins or leaves a graph, the app files its "shell state" into a store so the renderer can draw it. Two ways that filing can go wrong today fail silently: filing a node that is already filed under a *different* workflow strands the old entry, and un-filing a state the store never held leaves a ghost the renderer keeps drawing. Both now trip an assertion (throws in DEV, reports via the assert reporter in prod) instead of drifting quietly. No behavior changes when the invariants hold. ## What changed `src/core/graph/nodeShell/nodeShellState.ts` - `registerNodeState` asserts `node._graphId` is unset or already equals `graph.rootGraph.id` before overwriting it. Membership is by state identity, so a node re-registered under a second root graph leaves its first bucket entry behind with nothing to remove it. - `unregisterNodeState` captures `deleteNode`'s boolean instead of discarding it and asserts it. `false` means the bucket did not contain `node._state` — identity drift, e.g. `_state` reassigned after registration. Both messages name the node id. `_graphId` is cleared before the assertion so a failing teardown still leaves the node detached rather than half-registered. ## Deliberately not shipped: the duplicate-`NodeState` assertion The third invariant asked for — asserting the store never holds two distinct `NodeState` objects for one `(graphId, id)` — was implemented (an auxiliary forward `Map<string, NodeState>` plus a `Map<NodeState, string>` reverse map per bucket, so a renumbered node still de-indexes by identity) and then **reverted**, because it is not a regression net on this base: it fires on legitimate existing behavior. Evidence, all from the suite on this branch: - `LGraph.serialise.test.ts > can (de)serialise node / group titles` — `new LGraph(data)` (`LGraph.ts:476` → `configure:2636` → `add:1092`) keeps the *persisted* graph id, so a round-tripped copy shares a bucket with the graph it was serialised from and collides on every node id. - `LGraph.test.ts > Link serialization goldens` (4 tests) and `LGraph.test.ts > deduplicateSubgraphNodeIds (via configure) > warns when configuring a host with legacy proxyWidgets…` fail the same way. That is 6 tests, green on this base and green again with the assertion removed — so this is the assertion's premise being wrong, not a latent bug those tests were hiding. `subgraphDeduplication.ts` guarantees id-uniqueness *within* one root graph; it says nothing about two live `LGraph` instances that carry the same id. Making duplicate ids assertable means giving each `LGraph` instance its own bucket identity rather than keying buckets by graph id — a separate change with real blast radius (`getGraphNodesFor` callers, `clearGraph`, the `canvasStore.rootGraphId` readers), not a rider on this one. `docs/architecture/node-data-store.md` records both the two shipped invariants and this gap. ## Tests `nodeShellState.test.ts` gains the two cases; both are red without the production change (verified by stashing it). The cross-root case needs an explicit `second.id = createUuidv4()` — two fresh `LGraph`s both sit at `zeroUuid` until `configure`, so they share a bucket and the invariant genuinely holds. ## Verification Targeted: `nodeShellState.test.ts`, `nodeDataStore.test.ts`, `useNodeReplacement.test.ts`, `LGraph.test.ts`, `LGraph.serialise.test.ts` — 82 tests pass, 0 fail. `oxfmt --check` clean. Full `vitest run` was executed and is the reason the third assertion was reverted (it caught the 6 failures above). That run is **not** a clean-suite claim: this worktree borrows the parent clone's `node_modules`, which predates the `minisearch` dependency this branch adds, so 222 suites failed to resolve imports for reasons unrelated to the diff. Of the tests that did run, the only failures attributable to the diff were the 6 named above; the rest (`assetService`, `useAssetGridSelection`, `onboardingCloudRoutes`, …) are the same load-dependent set #14257 reported. A reviewer should treat repo-wide CI as the authority here, not this local run. Repo-wide `pnpm typecheck` was likewise not run locally — the diff is 11 lines of source plus tests, and both changed files typecheck under vitest's transform.
…#14276) **STACKED — merging lands on `matt/be-5050-node-shell-state` (owned by @mattmillerai), NOT `main`.** That branch is itself based on `feature/ecs-migration`, where `src/core/graph/nodeShell/` lives. Do not merge before its parent lands; GitHub retargets this PR as the stack unwinds. ## ELI-5 When a node joins a graph we write it into three places: its shell state, its widget bindings, and its widget ordering (plus preview exposures accumulate against the same bucket). When a node left, we only erased the first one. The leftovers piled up in a bucket shared by the whole workflow and stayed there until the entire graph was thrown away. This adds the missing "undo" for the other two and points every teardown path at it. ## Summary `attachNodeToStores` registers three things; every teardown path only undid one, so widget-order and preview-exposure entries orphaned in the shared root-graph bucket. This adds `detachNodeFromStores` as its exact inverse and routes the teardown paths through it. ## Changes - **What**: New `detachNodeFromStores(graph, node, mode)` in `nodeShellLifecycle.ts` — unregisters the shell state, removes each `getWidgetIds(node.widgets)` entry from the widget order, and clears the node's preview exposures (`String(node.id)` host locator, the same key `promotionUtils`/`SubgraphNode` write under). `unregisterAllNodeStates` moves out of `nodeShellState.ts` and becomes `detachAllNodesFromStores` in the same module — it needs the root graph to reach those buckets, so it now takes the graph (which carries `rootGraph`) rather than just `_nodes`/`_subgraphs`. Call sites: `LGraph.remove` (both the per-node teardown and the released-subgraph-definition walk) and `releaseGraphStores`'s non-root branch. - **Breaking**: none — `unregisterAllNodeStates` had exactly one external caller (`LGraph.ts`), and no behavior the extension surface can observe changes. ## Reachability the leak had (all three are load-bearing, not theoretical) - Subgraphs always take the non-root `else` branch of `releaseGraphStores`, and `LGraph.configure` calls `clear()` first — so re-configuring a subgraph orphaned its nodes' entries. - `clear()` sets `id = zeroUuid` on exit, so a root graph whose serialized data carries no id keeps taking the `else` branch for the rest of the session. - Plain `LGraph.remove(node)` leaked per-node on every branch, since it never removed the widget order it added. ## Review Focus **The judgment call the ticket left open — `deleteWidget` vs `removeNodeWidgetOrder` — is decided per call site, and both are covered by tests.** `LGraph.remove(node)` drops only the ordering and **keeps** the stored widget values: a single node removal feeds undo/redo and a node moved between graphs (`remove` then `add`) must not lose its values. Graph and subgraph-definition teardown (`detachAllNodesFromStores`) **discards** them, which is what the root branch already does one line up — `clearGraph` on `widgetValueStore` wipes that graph's widget states wholesale, so the non-root path was the inconsistent one. **Riskiest line: `deleteWidget` in the discard path**, because it is the one edit that removes data rather than an index. It is safe because `deleteWidget` only drops the store's map entry — the widget instance's `_state` object is untouched, so a widget that survives the clear still reads its own value, and re-registration (`setNodeId` → `registerWidget`) writes it back from the widget/serialized data rather than from store leftovers. Without this, a re-configure would silently reuse the *stale* store value: `registerWidget` returns the existing state when the type matches and ignores the incoming `init.value`. **Deliberately not routed through the new helper:** `useNodeReplacement.replaceWithMapping`. It calls `unregisterNodeState`/`registerNodeState` for an in-place swap where the new node inherits the same id and re-registers the same widgets — clearing exposures there would drop a replaced host node's promoted previews. It is a replacement, not a teardown. **Deviation from the ticket's sketch:** preview exposures are cleared with `setExposures(rootGraphId, locator, [])` (guarded by a `getExposures(...).length` read) rather than a `removeExposure` per name. Same result, one write instead of N, and the guard keeps the reactive `exposures` ref from being touched on every ordinary node deletion. ## Verification New `src/core/graph/nodeShell/nodeShellLifecycle.test.ts` covers all three: widget order dropped and values kept on `subgraph.remove(node)`; exposures dropped when a host `SubgraphNode` is removed; order, values and exposures all released when a zero-uuid root graph is cleared. **All three fail on the base branch** (verified by stashing the source change and re-running — `expected [ Array(1) ] to deeply equal []`). `vitest run src/lib/litegraph src/core/graph src/platform/nodeReplacement src/stores/widgetValueStore.test.ts src/composables/node` — 1591 passed. `pnpm typecheck`, `pnpm knip`, eslint and oxfmt on the changed files: clean. Amp-Thread-ID: https://ampcode.com/threads/T-01a027af-1c32-7532-84f9-b94aa90620fa
90d9ee2 to
b1468ac
Compare
STACKED — merging lands on
feature/ecs-migration(owned by @DrJKL, PR #13963's base), NOTmain.ELI-5
The code that wires a node's "shell state" (title, mode, colours, shape…) into the app's Pinia stores was living inside the vendored
src/lib/litegraphfolder, even though it is entirely app-owned. This moves those definitions intosrc/core/graph/nodeShell/and leaves one-line call sites behind in litegraph. Nothing changes at runtime — same functions, same order, same events.Fulfils the TODO committed to in #13963 (comment). cc @DrJKL
What moved
src/core/graph/nodeShell/nodeShellState.tscreateNodeShellState(title, type, titleMode)— the object literal that was inline in theLGraphNodeconstructor.setTrackedNodeState(node, property, value)— the body of the former privateLGraphNode.setTrackedState; the six accessors (title,mode,color,bgcolor,showAdvanced,shape) now call it.registerNodeState/unregisterNodeState/unregisterAllNodeStates— moved verbatim.src/core/graph/nodeShell/nodeShellLifecycle.tsattachNodeToStores(graph, node)—registerNodeStateplus the widgetsetNodeId/setNodeWidgetOrderblock that was inline inLGraph.add.LGraph.addis now a single call, in exactly the positionregisterNodeStateoccupied (afternode.graph = this, before_nodes.push/onAdded/node:added).releaseGraphStores(graph)— the store-teardown block fromLGraph.clear.LGraph.clearis now a single call.Also:
LGraph.removeimportsunregisterNodeState/unregisterAllNodeStatesfrom the new module,LGraphNode.tsno longer importsuseNodeDataStore, and the litegraph barrel no longer re-exportsregisterNodeState/unregisterNodeState(new in #13963, nothing external can depend on them yet — keeping the surface private per review feedback).useNodeReplacementimports them from the new module instead.fireNodeRemovalLifecyclestays inLGraph.ts(litegraph events only, no store access), and the link/reroute helpers stay inLLink.ts/Reroute.ts—releaseGraphStoresimports them from their current homes.Judgment calls
nodeShellState.tsholds the node-level definitions and imports onlynodeDataStore;nodeShellLifecycle.tsholds the graph-level attach/release coordination and is the only one that reaches forLLink/Reroute/ the other stores. That keepsLGraphNode's new runtime import off theReroute → litegraph barrel → LGraphNodecycle. A productionvite buildwas run to confirm the module graph still resolves.graphTriggers.tsmove. DrJKL's stack (refactor(layout): centralize node, group, and reroute geometry #14110 → refactor: register node geometry at attach, delete useVueNodeLifecycle #14128 → refactor: node geometry reads from the store, delete useLayoutSync #14133) is still open against this base, so the diff is kept to a strict definitions-move to keep those rebases cheap. MovinggraphTriggers.tswould touch the barrel's export path andLGraph.ts's import block for no functional gain in this pass.LGraphNode.nodeState.test.tsmoved tosrc/core/graph/nodeShell/nodeShellState.test.tswith assertions unchanged; the accessor andconfigurecases stay where they are. Two new cases covercreateNodeShellState's defaults. No test was deleted or weakened.useNodeReplacement.test.ts's barrel mock split into a secondvi.mockfor the new module.Verification
pnpm typecheck,pnpm lint(0 errors),pnpm knip, andvite buildare clean. Targeted suites (litegraph, nodeShell, nodeDataStore, nodeReplacement, minimap) — 91 files / 1341 tests pass.Full
vitest run: 14260 passed, 1 failed —src/platform/cloud/onboarding/onboardingCloudRoutes.test.ts > lazily resolves the /oauth layout and consent view components, a 5s timeout on a dynamic route import. It passes in isolation, is untouched by this diff, and an earlier full run under load failed a different pair of tests, so it reads as a load-dependent flake rather than a regression. Flagging rather than hiding it.