fix: layer editor UX and review follow-ups - #14941
Open
jtydhr88 wants to merge 4 commits into
Open
Conversation
dosubot
Bot
added
the
size:L
🎭 Playwright: ✅ 1840 passed, 0 failed📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 9.12 MB gzip 🔴 +1.28 kBDetailsSummary
Category Glance App Entry Points — 3.71 kB (baseline 3.71 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.37 MB (baseline 1.37 MB) • 🔴 +1 BGraph editor runtime, canvas, workflow orchestration
Status: 2 added / 2 removed / 1 unchanged Views & Navigation — 124 kB (baseline 124 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 13 added / 13 removed / 4 unchanged Panels & Settings — 566 kB (baseline 566 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 16 unchanged User & Accounts — 27.5 kB (baseline 27.5 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed / 5 unchanged Editors & Dialogs — 126 kB (baseline 125 kB) • 🔴 +172 BModals, dialogs, drawers, and in-app editors
Status: 7 added / 7 removed / 1 unchanged UI Components — 67.1 kB (baseline 67.1 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed / 8 unchanged Data & Services — 3.53 MB (baseline 3.53 MB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 550 kB (baseline 549 kB) • 🔴 +1.23 kBHelpers, composables, and utility bundles
Status: 21 added / 21 removed / 16 unchanged Vendor & Third-Party — 18.1 MB (baseline 18.1 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 18 unchanged Other — 14.2 MB (baseline 14.2 MB) • 🔴 +2.96 kBBundles that do not match a named category
Status: 76 added / 76 removed / 211 unchanged ⚡ Performance Report
Show regressions
All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-08-20T03:38:05.059Z",
"gitSha": "aa2cfa97e35941bf3bdc38f41e54b95db57833a7",
"branch": "fix/layer-editor-review-nits",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2047.2110000000043,
"styleRecalcs": 7,
"styleRecalcDurationMs": 5.9430000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 511.48199999999997,
"heapDeltaBytes": 15288132,
"heapUsedBytes": 76564116,
"domNodes": -285,
"jsHeapTotalBytes": 4448256,
"scriptDurationMs": 7.285,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2054.4869999999946,
"styleRecalcs": 10,
"styleRecalcDurationMs": 9.358,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 539.871,
"heapDeltaBytes": 15157984,
"heapUsedBytes": 76499812,
"domNodes": -280,
"jsHeapTotalBytes": 3923968,
"scriptDurationMs": 9.427999999999999,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2021.2339999999926,
"styleRecalcs": 77,
"styleRecalcDurationMs": 41.344,
"layouts": 12,
"layoutDurationMs": 3.6299999999999994,
"taskDurationMs": 990.0659999999999,
"heapDeltaBytes": -4597616,
"heapUsedBytes": 56813736,
"domNodes": -285,
"jsHeapTotalBytes": 4186112,
"scriptDurationMs": 109.23,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1878.1270000000632,
"styleRecalcs": 74,
"styleRecalcDurationMs": 40.324,
"layouts": 12,
"layoutDurationMs": 3.9579999999999997,
"taskDurationMs": 901.694,
"heapDeltaBytes": -5393448,
"heapUsedBytes": 55740452,
"domNodes": -283,
"jsHeapTotalBytes": 4710400,
"scriptDurationMs": 108.392,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1744.466999999986,
"styleRecalcs": 31,
"styleRecalcDurationMs": 19.264999999999997,
"layouts": 6,
"layoutDurationMs": 0.821,
"taskDurationMs": 439.094,
"heapDeltaBytes": 2760196,
"heapUsedBytes": 64179348,
"domNodes": 76,
"jsHeapTotalBytes": 4718592,
"scriptDurationMs": 14.278999999999998,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1725.8350000000746,
"styleRecalcs": 31,
"styleRecalcDurationMs": 16.231999999999996,
"layouts": 6,
"layoutDurationMs": 0.586,
"taskDurationMs": 364.01300000000003,
"heapDeltaBytes": 2474232,
"heapUsedBytes": 63821804,
"domNodes": 75,
"jsHeapTotalBytes": 4456448,
"scriptDurationMs": 10.030000000000001,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 560.7750000000351,
"styleRecalcs": 11,
"styleRecalcDurationMs": 7.297999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 350.5160000000001,
"heapDeltaBytes": 10526288,
"heapUsedBytes": 73014700,
"domNodes": 18,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 52.36999999999999,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 570.4640000000154,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.159000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 364.30300000000005,
"heapDeltaBytes": 10739984,
"heapUsedBytes": 71833100,
"domNodes": 16,
"jsHeapTotalBytes": 4718592,
"scriptDurationMs": 55.486999999999995,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-idle",
"durationMs": 2023.46,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.173000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 630.4019999999999,
"heapDeltaBytes": 13060820,
"heapUsedBytes": 87693968,
"domNodes": -282,
"jsHeapTotalBytes": 2879488,
"scriptDurationMs": 14.962999999999997,
"eventListeners": -179,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-idle",
"durationMs": 2037.093999999911,
"styleRecalcs": 9,
"styleRecalcDurationMs": 10.505,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 633.088,
"heapDeltaBytes": -4532540,
"heapUsedBytes": 71319620,
"domNodes": -277,
"jsHeapTotalBytes": -528384,
"scriptDurationMs": 17.558,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-pan",
"durationMs": 2159.631999999988,
"styleRecalcs": 69,
"styleRecalcDurationMs": 15.036000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1180.1209999999999,
"heapDeltaBytes": -4698832,
"heapUsedBytes": 71824112,
"domNodes": -270,
"jsHeapTotalBytes": -565248,
"scriptDurationMs": 316.913,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2160.87600000003,
"styleRecalcs": 69,
"styleRecalcDurationMs": 15.982000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1198.453,
"heapDeltaBytes": -13293380,
"heapUsedBytes": 63481136,
"domNodes": -277,
"jsHeapTotalBytes": 1568768,
"scriptDurationMs": 319.587,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3214.683999999977,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.675999999999995,
"layouts": 60,
"layoutDurationMs": 8.129,
"taskDurationMs": 1403.7469999999998,
"heapDeltaBytes": -10064552,
"heapUsedBytes": 67803196,
"domNodes": -287,
"jsHeapTotalBytes": 4452352,
"scriptDurationMs": 393.904,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666696,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3136.3979999999856,
"styleRecalcs": 64,
"styleRecalcDurationMs": 14.810999999999998,
"layouts": 60,
"layoutDurationMs": 7.963999999999999,
"taskDurationMs": 1382.9669999999999,
"heapDeltaBytes": -1113532,
"heapUsedBytes": 76952300,
"domNodes": -275,
"jsHeapTotalBytes": -2101248,
"scriptDurationMs": 384.324,
"eventListeners": -145,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "legacy-node-drag",
"durationMs": 2221.6399999999794,
"styleRecalcs": 45,
"styleRecalcDurationMs": 9.294,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1394.882,
"heapDeltaBytes": 11943572,
"heapUsedBytes": 95606600,
"domNodes": 10,
"jsHeapTotalBytes": 8581120,
"scriptDurationMs": 456.173,
"eventListeners": 186,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "legacy-node-drag",
"durationMs": 2259.531000000038,
"styleRecalcs": 46,
"styleRecalcDurationMs": 10.883000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1421.4640000000002,
"heapDeltaBytes": 10779140,
"heapUsedBytes": 89555788,
"domNodes": 12,
"jsHeapTotalBytes": 7532544,
"scriptDurationMs": 466.252,
"eventListeners": 186,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2039.9429999999938,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.959999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 770.4899999999999,
"heapDeltaBytes": -12950488,
"heapUsedBytes": 69168628,
"domNodes": -282,
"jsHeapTotalBytes": 3928064,
"scriptDurationMs": 20.610000000000003,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2045.6349999999475,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.541999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 624.136,
"heapDeltaBytes": -12801920,
"heapUsedBytes": 69387672,
"domNodes": -282,
"jsHeapTotalBytes": 2355200,
"scriptDurationMs": 16.705000000000005,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 635.0050000000351,
"styleRecalcs": 46,
"styleRecalcDurationMs": 10.959999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 418.693,
"heapDeltaBytes": 11444260,
"heapUsedBytes": 72803124,
"domNodes": 18,
"jsHeapTotalBytes": 5505024,
"scriptDurationMs": 123.854,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 583.0609999999297,
"styleRecalcs": 48,
"styleRecalcDurationMs": 11.174,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 382.53000000000003,
"heapDeltaBytes": 11915696,
"heapUsedBytes": 73376628,
"domNodes": 22,
"jsHeapTotalBytes": 5505024,
"scriptDurationMs": 119.061,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2009.3590000000177,
"styleRecalcs": 10,
"styleRecalcDurationMs": 9.383000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 498.878,
"heapDeltaBytes": -5810816,
"heapUsedBytes": 55548512,
"domNodes": -280,
"jsHeapTotalBytes": 4710400,
"scriptDurationMs": 7.1819999999999995,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2042.748999999958,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 532.0709999999999,
"heapDeltaBytes": 3924324,
"heapUsedBytes": 65439500,
"domNodes": -281,
"jsHeapTotalBytes": 4448256,
"scriptDurationMs": 8.607999999999997,
"eventListeners": -181,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1762.61199999999,
"styleRecalcs": 76,
"styleRecalcDurationMs": 36.504000000000005,
"layouts": 16,
"layoutDurationMs": 4.609999999999999,
"taskDurationMs": 791.763,
"heapDeltaBytes": 16527548,
"heapUsedBytes": 78042844,
"domNodes": -280,
"jsHeapTotalBytes": 4448256,
"scriptDurationMs": 79.597,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1709.2840000000251,
"styleRecalcs": 75,
"styleRecalcDurationMs": 38.958,
"layouts": 16,
"layoutDurationMs": 4.809,
"taskDurationMs": 829.4090000000001,
"heapDeltaBytes": 15595568,
"heapUsedBytes": 76704236,
"domNodes": -279,
"jsHeapTotalBytes": 5234688,
"scriptDurationMs": 89.88600000000001,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 1384.7640000000183,
"styleRecalcs": 19,
"styleRecalcDurationMs": 32.002,
"layouts": 14,
"layoutDurationMs": 13.889999999999999,
"taskDurationMs": 951.7549999999999,
"heapDeltaBytes": -7852608,
"heapUsedBytes": 86823724,
"domNodes": 13673,
"jsHeapTotalBytes": 12320768,
"scriptDurationMs": 17.668999999999997,
"eventListeners": 2375,
"totalBlockingTimeMs": 131,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 8532.553000000007,
"styleRecalcs": 250,
"styleRecalcDurationMs": 44.98700000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4556.698,
"heapDeltaBytes": -2582808,
"heapUsedBytes": 73279296,
"domNodes": -277,
"jsHeapTotalBytes": 745472,
"scriptDurationMs": 1040.39,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "viewport-pan-sweep",
"durationMs": 8278.654999999959,
"styleRecalcs": 249,
"styleRecalcDurationMs": 42.24400000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4182.593,
"heapDeltaBytes": -10156044,
"heapUsedBytes": 64706236,
"domNodes": -282,
"jsHeapTotalBytes": 4452352,
"scriptDurationMs": 963.1030000000001,
"eventListeners": -163,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 18387.598000000027,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17625.29,
"heapDeltaBytes": -22169984,
"heapUsedBytes": 184732028,
"domNodes": -8312,
"jsHeapTotalBytes": -9900032,
"scriptDurationMs": 120.402,
"eventListeners": -16387,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.779999999999927,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 18750.69600000006,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17823.449999999997,
"heapDeltaBytes": -27644448,
"heapUsedBytes": 179592020,
"domNodes": -8312,
"jsHeapTotalBytes": -13570048,
"scriptDurationMs": 135.44899999999998,
"eventListeners": -16389,
"totalBlockingTimeMs": 3,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 23511.266999999974,
"styleRecalcs": 175,
"styleRecalcDurationMs": 27.907000000000014,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 23144.033999999996,
"heapDeltaBytes": -14140720,
"heapUsedBytes": 194092368,
"domNodes": -8312,
"jsHeapTotalBytes": -12820480,
"scriptDurationMs": 469.13899999999995,
"eventListeners": -16381,
"totalBlockingTimeMs": 147,
"frameDurationMs": 18.33666666666662,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 23022.667999999954,
"styleRecalcs": 177,
"styleRecalcDurationMs": 23.089000000000027,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 22331.613999999998,
"heapDeltaBytes": -26558444,
"heapUsedBytes": 194170180,
"domNodes": -8312,
"jsHeapTotalBytes": -10756096,
"scriptDurationMs": 452.26199999999994,
"eventListeners": -16379,
"totalBlockingTimeMs": 227,
"frameDurationMs": 17.780000000000047,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "workflow-execution",
"durationMs": 138.77999999999702,
"styleRecalcs": 13,
"styleRecalcDurationMs": 21.875,
"layouts": 10,
"layoutDurationMs": 2.7920000000000003,
"taskDurationMs": 98.76299999999999,
"heapDeltaBytes": 3198456,
"heapUsedBytes": 64511852,
"domNodes": 139,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 8.843,
"eventListeners": 25,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 110.80600000002505,
"styleRecalcs": 6,
"styleRecalcDurationMs": 14.528,
"layouts": 3,
"layoutDurationMs": 1.068,
"taskDurationMs": 71.42300000000002,
"heapDeltaBytes": 2949564,
"heapUsedBytes": 64267920,
"domNodes": 126,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 4.619,
"eventListeners": 27,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
}
]
} |
🎨 Storybook: 🚧 Building... |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds layer-panel drag-and-drop reordering, alpha-aware layer picking, localized validation feedback, transient compositor-state persistence, safer numeric input handling, and fixes for history merging and render-target cleanup. ChangesLayer editor interaction and reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LayerPanel
participant layerPanelDnd
participant useLayerEditorSession
participant History
LayerPanel->>layerPanelDnd: calculate drop position
layerPanelDnd-->>LayerPanel: return destination index
LayerPanel->>useLayerEditorSession: call moveLayerTo
useLayerEditorSession->>History: record layer movement
History-->>useLayerEditorSession: update undo state
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/extensions/compositor/components/WidgetCompositor.test.ts`:
- Around line 43-45: Remove the vue-i18n mock around WidgetCompositor and update
renderWidget to install the project’s real i18n test plugin/configuration.
Ensure WidgetCompositor’s useI18n call resolves through the installed plugin
while preserving the existing test rendering behavior.
In `@src/renderer/extensions/layerEditor/components/PropertyNumberField.test.ts`:
- Around line 28-33: Update the emptied-field test around commitValue and the
restored-value assertion to await Vue’s nextTick after tabbing before reading
input.value. Keep the existing commit assertion and verify the field restores to
“40” only after the scheduled draft reset has rendered.
In `@src/renderer/extensions/layerEditor/composables/layerPanelDnd.ts`:
- Around line 7-16: The drop-index calculation must account for removal of the
dragged layer. In
src/renderer/extensions/layerEditor/composables/layerPanelDnd.ts lines 7-16,
update reorderDropIndex to accept draggedId, remove it before finding targetId,
and calculate the position from the filtered list. In
src/renderer/extensions/layerEditor/components/LayerPanel.vue lines 244-250,
pass dragged to reorderDropIndex. In
src/renderer/extensions/layerEditor/composables/layerPanelDnd.test.ts lines
13-32, add coverage for dragged layers both before and after the target.
In `@src/renderer/extensions/layerEditor/engine/history.ts`:
- Around line 100-105: The commit path around merge handling must invalidate the
saved-state reachability when branching from an undone state: before clearing
redoStack, check whether it contains mergeBarrier and set cleanReachable to
false when present. Preserve existing dirtyCount behavior and add a regression
test covering push A, markSaved, undo A, then push B.
In `@src/renderer/extensions/layerEditor/engine/render/renderStack.ts`:
- Around line 286-294: Make the nested group allocation path in buildInputs
exception-safe: when the group composite using handle throws, free handle and
invoke sub.cleanup() before propagating the error. Preserve normal cleanup
ownership for successful composites, and extend the regression test to cover a
throwing composite with a non-null target.
🪄 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: 4feb2437-f0a6-415f-be59-b83f3d1d2b20
📒 Files selected for processing (19)
src/locales/en/main.jsonsrc/renderer/extensions/compositor/components/WidgetCompositor.test.tssrc/renderer/extensions/compositor/components/WidgetCompositor.vuesrc/renderer/extensions/layerEditor/components/LayerEditorCanvas.vuesrc/renderer/extensions/layerEditor/components/LayerPanel.vuesrc/renderer/extensions/layerEditor/components/LayerPropertiesPanel.vuesrc/renderer/extensions/layerEditor/components/PropertyNumberField.test.tssrc/renderer/extensions/layerEditor/components/PropertyNumberField.vuesrc/renderer/extensions/layerEditor/composables/layerPanelDnd.test.tssrc/renderer/extensions/layerEditor/composables/layerPanelDnd.tssrc/renderer/extensions/layerEditor/composables/useLayerEditor.test.tssrc/renderer/extensions/layerEditor/composables/useLayerEditor.tssrc/renderer/extensions/layerEditor/composables/useLayerEditorSession.test.tssrc/renderer/extensions/layerEditor/composables/useLayerEditorSession.tssrc/renderer/extensions/layerEditor/engine/compositor/webglCompositor.tssrc/renderer/extensions/layerEditor/engine/history.test.tssrc/renderer/extensions/layerEditor/engine/history.tssrc/renderer/extensions/layerEditor/engine/render/renderStack.test.tssrc/renderer/extensions/layerEditor/engine/render/renderStack.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/compositor/components/WidgetCompositor.test.ts (1)
92-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the node-present, no-layers branch.
The current tests cover a missing node and a node with cached layers. They do not cover an existing node when
hasCompositorLayers(node)returnsfalse.WidgetCompositor.vuerequires both conditions before enabling the button. Add this case and assert that the open button remains disabled. Otherwise, a regression to node-only availability would pass.As per path instructions, colocated Vitest tests must cover the changed widget-state behavior with real behavioral assertions.
🤖 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/renderer/extensions/compositor/components/WidgetCompositor.test.ts` around lines 92 - 103, Extend the colocated WidgetCompositor tests with an existing graph node that has no compositor layers, ensuring hasCompositorLayers(graphNode) remains false; render via renderWidget and assert the compositor-open-button is disabled. Preserve the existing missing-node and cached-layer cases.Source: Path instructions
🤖 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/extensions/compositor/components/WidgetCompositor.test.ts`:
- Around line 84-88: Update the rendered-state assertions in the
WidgetCompositor test to query the empty message with getByText and the
compositor control with getByRole('button', { name: 'Open Compositor' }); remove
the data-testid-based lookups and assert the button through its accessible name.
In
`@src/renderer/extensions/layerEditor/composables/useLayerEditorSession.test.ts`:
- Around line 391-392: Update the test helper idOf and the related
reorderDropIndex usage to explicitly check nullable results before accessing or
passing them to moveLayerTo. Fail with clear messages when a layer name is
missing or the drop index is unavailable, and remove the non-null assertions
while preserving the existing test flow.
---
Outside diff comments:
In `@src/renderer/extensions/compositor/components/WidgetCompositor.test.ts`:
- Around line 92-103: Extend the colocated WidgetCompositor tests with an
existing graph node that has no compositor layers, ensuring
hasCompositorLayers(graphNode) remains false; render via renderWidget and assert
the compositor-open-button is disabled. Preserve the existing missing-node and
cached-layer cases.
🪄 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: bad2ba8c-83ca-48f5-aed5-89ef1272abd9
📒 Files selected for processing (7)
src/renderer/extensions/compositor/components/WidgetCompositor.test.tssrc/renderer/extensions/layerEditor/components/PropertyNumberField.test.tssrc/renderer/extensions/layerEditor/composables/useLayerEditorSession.test.tssrc/renderer/extensions/layerEditor/engine/history.test.tssrc/renderer/extensions/layerEditor/engine/history.tssrc/renderer/extensions/layerEditor/engine/render/renderStack.test.tssrc/renderer/extensions/layerEditor/engine/render/renderStack.ts
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #14941 +/- ##
==========================================
+ Coverage 79.22% 81.76% +2.53%
==========================================
Files 2213 1887 -326
Lines 113476 107256 -6220
Branches 34896 33477 -1419
==========================================
- Hits 89898 87693 -2205
+ Misses 23106 19208 -3898
+ Partials 472 355 -117
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 362 files with indirect coverage changes 🚀 New features to boost your workflow:
|
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
Review — layer editor UX follow-ups
15 parallel agents (12 domain + 3 adversarial/skeptic) reviewed this PR. One blind-spot candidate (missing crossOrigin) was a false positive — already set at useLayerEditorSession.ts:82. All findings below are post-skeptic confirmed.
issue: layerOpacityAt ignores group-level opacity — wrong-layer picks on semi-transparent groups
src/renderer/extensions/layerEditor/engine/editor/pickOps.ts line 81
The group case returns best (max child alpha) without multiplying by node.opacity. A group set to e.g. 20% opacity with fully-opaque raster children returns 1.0, exceeding PICK_OPACITY_THRESHOLD — the click lands on the nearly-invisible group instead of passing through. This contradicts the pixel-accurate picking intent of this PR.
// current
return best
// fix
return best * node.opacityThe existing if (node.opacity <= 0) return 0 early-out already handles the fully-transparent edge case, so this one-line change is safe.
issue: onRowDrop calls e.preventDefault() unconditionally — swallows external file drops
src/renderer/extensions/layerEditor/components/LayerPanel.vue, onRowDrop
onRowDragOver correctly guards with if (!dragId.value || dragId.value === id) return (without calling preventDefault), showing the no-drop cursor for external drags. But onRowDrop calls e.preventDefault() before checking dragId.value, silently swallowing any external file drop and preventing an outer upload zone from handling it.
function onRowDrop(id: string, e: DragEvent): void {
const dragged = dragId.value
const hint = dropHint.value
if (!dragged || hint?.id !== id) {
endDrag()
return // let external drops propagate
}
e.preventDefault()
// ... rest
}suggestion: (non-blocking) No @dragleave — dropHint indicator persists when cursor exits panel
src/renderer/extensions/layerEditor/components/LayerPanel.vue (three agents flagged independently)
When the cursor moves from a row to the scrollbar or a gap between rows, dragover stops firing but dragend has not yet. The drop-indicator line stays on the last-hovered row. A container-level @dragleave checking e.relatedTarget would clear dropHint cleanly:
<div class="..." @dragleave="onContainerDragLeave">function onContainerDragLeave(e: DragEvent): void {
const container = e.currentTarget as HTMLElement
if (!container.contains(e.relatedTarget as Node | null)) endDrag()
}question: Does clicking a selected layer on a transparent interior with nothing below intentionally deselect it?
src/renderer/extensions/layerEditor/composables/useLayerEditorSession.ts:871
With insideBox removed: user has raster layer A selected → clicks inside A's bounding box on a transparent pixel where no other layer is underneath → pickLayerAt returns null → setSelectedNodes([]) fires → A is deselected → drag does not start.
In Figma/PS, clicking inside a selected layer's bounding box always initiates drag regardless of pixel transparency. The PR description covers "transparent pixels fall through" for unselected layers but does not address this edge case for already-selected layers. Is this intentional? If so a brief note would help future reviewers.
suggestion: (non-blocking) buildInputs — sub resources may leak if allocTarget throws before cleanup registration
src/renderer/extensions/layerEditor/engine/render/renderStack.ts
The exception-safety refactor handles composite(sub.inputs, handle) throwing correctly. However, between buildInputs(g, ...) returning sub and the allocTarget call entering the inner try block, if allocTarget throws, the outer catch runs accumulated cleanups — but sub.cleanup is not yet registered there, so sub's allocated GPU targets are orphaned.
In practice this is GPU-loss territory where the render loop halts anyway, so practical impact is low. But structurally the exception-safety story has this gap. Fix: register sub.cleanup in cleanups immediately after buildInputs returns, or wrap the post-buildInputs block (including allocTarget) in its own try/catch that calls sub.cleanup() on throw.
suggestion: (non-blocking) mergeBarrier becomes stale after undo() and evict()
src/renderer/extensions/layerEditor/engine/history.ts
After markSaved() sets mergeBarrier to the undo-stack top, a subsequent undo() moves that command to redoStack. The barrier ref is not cleared. On the next push() (which clears redoStack), mergeBarrier points to a command in neither stack — merge-barrier protection for that save point is silently lost.
Additionally, evict() can remove the barrier command from the undo stack while mergeBarrier still holds a strong reference, preventing GC of the command's undo-state snapshot until the next clear() or markSaved().
// in undo():
if (cmd === this.mergeBarrier) this.mergeBarrier = null
// in evict():
if (dropped === this.mergeBarrier) this.mergeBarrier = nullnitpick: (non-blocking) dataTransfer.setData is dead code with a subtle side effect
src/renderer/extensions/layerEditor/components/LayerPanel.vue, onRowDragStart
dragId.value is the source of truth in onRowDrop — the getData counterpart is never called. Side effect: browsers treat this as a text drag, so dropping a layer row onto any text input or external drop zone delivers the layer UUID as typed text.
if (e.dataTransfer) {
e.dataTransfer.effectAllowed = 'move'
// remove: e.dataTransfer.setData('text/plain', id)
}nitpick: (non-blocking) reorderDropIndex — document the bottom-up z-order convention
src/renderer/extensions/layerEditor/composables/layerPanelDnd.ts
The visual panel rows are rendered top-down ([...imageLayers].reverse()) but bottomUpIds is passed bottom-up (z=0 at index 0). The formula pos === 'above' ? index + 1 : index depends on this inversion — "above" in visual space = higher z-index = higher array index. A doc comment prevents future readers from second-guessing the math:
/**
* Returns the target index in root.children for a drag-drop reorder.
* @param bottomUpIds IDs ordered z=0 (bottom) to z=n (top) — inverse of panel display order.
* @param pos Visual drop position. 'above' = higher z-order (index + 1).
* @param offset Reserved bottom slots (1 if a pinned background fill exists, else 0).
*/
export function reorderDropIndex(nitpick: (non-blocking) History merge path intentionally omits bumpDirty() — add a comment
src/renderer/extensions/layerEditor/engine/history.ts (merge path)
The missing bumpDirty() in the merge path is the core of the dirty-tracking fix (merged edits share one dirty step). Worth a one-liner so a future reader does not add it back thinking it was accidentally dropped:
// Merged edits share the first commit's dirty step — intentionally no bumpDirty() here.
this.emit(cmd.dirtyMask)nitpick: (non-blocking) dropHintClass returns 'relative' for every row — add it to rowClass once
src/renderer/extensions/layerEditor/components/LayerPanel.vue:211
Since pseudo-elements need position: relative on the parent, dropHintClass returns 'relative' for all rows (hinted and non-hinted). Cleaner to add relative to rowClass once and have dropHintClass return '' for non-hinted rows.
suggestion: (non-blocking) history.test.ts — add redo-after-undo-of-merged-edit coverage
src/renderer/extensions/layerEditor/engine/history.test.ts
The new tests verify the undo direction. The redo round-trip is unverified:
it('redo after undoing a merge makes dirty again', () => {
const h = new History()
h.push(new MergingCommand('opacity'))
h.push(new MergingCommand('opacity'))
h.markSaved()
h.push(new MergingCommand('opacity'))
h.undo()
expect(h.dirty()).toBe(false)
h.redo()
expect(h.dirty()).toBe(true)
})nitpick: (non-blocking) sampleCache WeakMap may be unnecessary
src/renderer/extensions/layerEditor/engine/editor/pickOps.ts:13
canvas.getContext('2d') is idempotent per the HTML spec — browsers return the same context object on every call. The WeakMap adds a three-way cache-state check (undefined vs null vs context) and a subtle risk: if a canvas later acquires a WebGL context (getContext('2d') would return null), the cached non-null 2D context produces stale pixel reads. Consider removing it and calling getContext('2d') directly, or at minimum document that content canvases are never repurposed for WebGL.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/extensions/layerEditor/engine/editor/pickOps.test.ts`:
- Around line 85-96: Replace the `group` fixture’s `unknown`-based cast with a
complete object validated using `satisfies GroupData`, including the correct
typed value for `mode` and all required group fields. If an existing typed group
factory is available, reuse it instead, while preserving the fixture’s current
test values.
🪄 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: e6a6b63b-4519-485a-8d44-5a02f2b58930
📒 Files selected for processing (8)
src/renderer/extensions/layerEditor/components/LayerPanel.vuesrc/renderer/extensions/layerEditor/composables/layerPanelDnd.tssrc/renderer/extensions/layerEditor/engine/editor/pickOps.test.tssrc/renderer/extensions/layerEditor/engine/editor/pickOps.tssrc/renderer/extensions/layerEditor/engine/history.test.tssrc/renderer/extensions/layerEditor/engine/history.tssrc/renderer/extensions/layerEditor/engine/render/renderStack.test.tssrc/renderer/extensions/layerEditor/engine/render/renderStack.ts
jtydhr88
force-pushed
the
fix/layer-editor-review-nits
branch
from
August 10, 2026 00:44
9634002 to
c80c4fc
Compare
Contributor
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/scripts/changeTracker.ts (1)
313-359: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd ChangeTracker integration coverage.
Add a test that stores transient state for one workflow, switches state, and restores the original workflow. Assert that compositor layers are restored. Also assert that a workflow without transient state clears the compositor cache.
The provided unit tests do not exercise the
ChangeTracker.store()toChangeTracker.restore()contract.As per coding guidelines, “Write tests for code changes.”
🤖 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/scripts/changeTracker.ts` around lines 313 - 359, Add integration coverage for the ChangeTracker.store() and ChangeTracker.restore() contract: save transient state for one workflow, switch workflow state, restore the original workflow, and assert its compositor layers are restored. Also cover restoring a workflow with no transient state and verify that the compositor cache is cleared, using the existing ChangeTracker and compositor-state test utilities.Source: Coding guidelines
🤖 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/extensions/core/imageCompositor.ts`:
- Around line 23-27: Update the restore callback for
registerWorkflowTransientState('Comfy.ImageCompositor.layers') to validate the
unknown state as a valid ReadonlyMap<NodeLocatorId, CompositorNodeCache> before
calling restoreCompositorLayers. For invalid or unsupported values, clear the
compositor state instead, preventing restoreCompositorLayers from throwing or
creating invalid cache entries.
---
Outside diff comments:
In `@src/scripts/changeTracker.ts`:
- Around line 313-359: Add integration coverage for the ChangeTracker.store()
and ChangeTracker.restore() contract: save transient state for one workflow,
switch workflow state, restore the original workflow, and assert its compositor
layers are restored. Also cover restoring a workflow with no transient state and
verify that the compositor cache is cleared, using the existing ChangeTracker
and compositor-state test utilities.
🪄 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: a7647414-f250-475f-baac-fd4c047d0f83
📒 Files selected for processing (7)
src/extensions/core/imageCompositor.tssrc/locales/en/main.jsonsrc/platform/workflow/management/workflowTransientState.test.tssrc/platform/workflow/management/workflowTransientState.tssrc/renderer/extensions/compositor/composables/useCompositorLayers.test.tssrc/renderer/extensions/compositor/composables/useCompositorLayers.tssrc/scripts/changeTracker.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/changeTracker.ts (1)
313-313: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDeep-copy compositor snapshots before storing them.
snapshotCompositorLayers()copies only theMap. It shares each cache object and its mutable arrays with the live state. A later mutation through the compositor API can change the snapshot used bystore(). Copy the cache values and arrays, then add a round-trip test that mutates compositor state afterstore()and checks thatrestore()returns the saved state.🤖 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/scripts/changeTracker.ts` at line 313, Update the snapshot workflow around snapshotWorkflowTransientState and snapshotCompositorLayers so compositor cache values and their mutable arrays are deep-copied before being stored in transientState. Preserve the saved snapshot when compositor state is mutated after store(), and add a round-trip test verifying restore() returns the original state.
🤖 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.
Outside diff comments:
In `@src/scripts/changeTracker.ts`:
- Line 313: Update the snapshot workflow around snapshotWorkflowTransientState
and snapshotCompositorLayers so compositor cache values and their mutable arrays
are deep-copied before being stored in transientState. Preserve the saved
snapshot when compositor state is mutated after store(), and add a round-trip
test verifying restore() returns the original state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9caafd94-ffaf-4779-b874-212d7c614896
📒 Files selected for processing (2)
src/locales/en/main.jsonsrc/scripts/changeTracker.ts
christian-byrne
approved these changes
Aug 16, 2026
christian-byrne
requested changes
Aug 16, 2026
Contributor
There was a problem hiding this comment.
Can you provide some reasoning for this? The problem being solved, why this approach was chosen, and what alternatives exist? Is there some limitation in the current systems that require this new concept?
Collaborator
Author
There was a problem hiding this comment.
The compositor's layer cache (temp refs + fingerprints from ui.* on execute) is the same lifecycle as nodeOutputs: execution-scoped, not serializable (backend-session temp files, putting them in the workflow JSON means dead refs cross-session).
Tab switches run configure() -> LGraph.clear(), which fires onRemoved on every node and wipes the cache; nodeOutputs survives because ChangeTracker snapshots/restores it, ours had no seat on that mechanism.
My first version wired it directly into ChangeTracker next to nodeOutputs, but that makes the tracker import a feature module, and every future node with editor state tied to execution needs another hand-written pair there.
The registry is just that same snapshot/restore lifecycle turned into an extension point, the tracker calls it at the two spots it already handles nodeOutputs, features register themselves.
No new lifecycle, no store; a missing key restores as undefined so workflows stay isolated.
Contributor
There was a problem hiding this comment.
Thanks for the explanation. After reviewing the broader context, I do not think we should add this generic snapshot/restore registry here.
The per-workflow state TDD and proposed frontend document model identify the missing primitives as document identity and lifecycle events. This registry provides only snapshot/restore semantics, without document identity, close/disposal, or explicit ownership.
Suggested next steps:
- Remove the
workflowTransientStatecommit from this PR so the layer-editor UX fixes can land independently. - Model compositor state by document uid and manage it through document lifecycle events such as
Deactivate,Activate,Close, andPostClose. - Do not wire those events directly to
beforeLoadNewGraph/afterLoadNewGraph, since those hooks also run for undo and same-document reloads. Lifecycle events must be gated on document identity transitions.
If a temporary registry is still required, it should be handled separately, marked @internal, and include an explicit removal condition tied to the document lifecycle implementation.
christian-byrne
added a commit
that referenced
this pull request
Aug 19, 2026
## Summary Node preview images are lost permanently on a workflow tab switch, because `app.clean()` revokes their object URLs and nothing restores them. This scopes preview state per workflow instead. Fixes FE-1645 ## Changes - **What**: `workflowService.beforeLoadNewGraph()` hands the live previews to `nodeOutputStore` keyed by the outgoing workflow path, so `app.clean()` has nothing left to revoke. `afterLoadNewGraph()` puts back the previews belonging to the workflow that just became active. ### Before 1. Open workflow A with a KSampler, preview method anything but `none`. 2. Queue it and let it finish — the KSampler shows its final latent preview. 3. Switch to tab B, switch back to tab A. 4. The preview is gone and never comes back. Output images on other nodes survive the same switch. `app.clean()` (`src/scripts/app.ts:2434`) runs on every workflow load and calls `resetAllOutputsAndPreviews()` → `revokeAllPreviews()` (`src/stores/nodeOutputStore.ts:301`), which releases every preview object URL and empties the maps. `nodeOutputs` survives only because `ChangeTracker` snapshots and restores it (`src/scripts/changeTracker.ts:306`/`350`); previews have no equivalent. Nothing revokes previews at execution end, so a finished run's last frame is real state a user is looking at — and a `b_preview` websocket frame cannot be re-fetched, so revoking makes the loss permanent. ### After The preview is still there when you come back to the tab. ## Review Focus - Keying by workflow path is load-bearing, not incidental. Node locator ids for root-graph nodes are bare node ids, so simply not clearing on load would show workflow A's node 5 preview on workflow B's node 5. - Ownership of the object URL retain moves from the live map to the stash and back, so nothing is released on a tab switch. Released instead when: the workflow is cleared in place (`Clear Workflow` still calls `app.clean()` with no load following it), the workflow has been closed, or a preview arriving for the incoming graph supersedes the stashed one. - Undo/redo goes through `loadGraphData(..., clean = false)` for the same workflow — it stashes and restores under the same path, so the net effect is unchanged. - Deliberately not done: no `ChangeTracker` snapshot/restore field for previews, and no use of the `workflowTransientState` mechanism proposed in #14941, so this does not prejudge that review. ## Tests `src/stores/nodeOutputStore.workflowSwitch.test.ts` covers the round trip, the same-node-id cross-workflow case, release on close, and clear-in-place. `workflowService.test.ts` covers the two lifecycle hooks.
comfydesigner
pushed a commit
that referenced
this pull request
Aug 20, 2026
## Summary Node preview images are lost permanently on a workflow tab switch, because `app.clean()` revokes their object URLs and nothing restores them. This scopes preview state per workflow instead. Fixes FE-1645 ## Changes - **What**: `workflowService.beforeLoadNewGraph()` hands the live previews to `nodeOutputStore` keyed by the outgoing workflow path, so `app.clean()` has nothing left to revoke. `afterLoadNewGraph()` puts back the previews belonging to the workflow that just became active. ### Before 1. Open workflow A with a KSampler, preview method anything but `none`. 2. Queue it and let it finish — the KSampler shows its final latent preview. 3. Switch to tab B, switch back to tab A. 4. The preview is gone and never comes back. Output images on other nodes survive the same switch. `app.clean()` (`src/scripts/app.ts:2434`) runs on every workflow load and calls `resetAllOutputsAndPreviews()` → `revokeAllPreviews()` (`src/stores/nodeOutputStore.ts:301`), which releases every preview object URL and empties the maps. `nodeOutputs` survives only because `ChangeTracker` snapshots and restores it (`src/scripts/changeTracker.ts:306`/`350`); previews have no equivalent. Nothing revokes previews at execution end, so a finished run's last frame is real state a user is looking at — and a `b_preview` websocket frame cannot be re-fetched, so revoking makes the loss permanent. ### After The preview is still there when you come back to the tab. ## Review Focus - Keying by workflow path is load-bearing, not incidental. Node locator ids for root-graph nodes are bare node ids, so simply not clearing on load would show workflow A's node 5 preview on workflow B's node 5. - Ownership of the object URL retain moves from the live map to the stash and back, so nothing is released on a tab switch. Released instead when: the workflow is cleared in place (`Clear Workflow` still calls `app.clean()` with no load following it), the workflow has been closed, or a preview arriving for the incoming graph supersedes the stashed one. - Undo/redo goes through `loadGraphData(..., clean = false)` for the same workflow — it stashes and restores under the same path, so the net effect is unchanged. - Deliberately not done: no `ChangeTracker` snapshot/restore field for previews, and no use of the `workflowTransientState` mechanism proposed in #14941, so this does not prejudge that review. ## Tests `src/stores/nodeOutputStore.workflowSwitch.test.ts` covers the round trip, the same-node-id cross-workflow case, release on close, and clear-in-place. `workflowService.test.ts` covers the two lifecycle hooks.
jtydhr88
force-pushed
the
fix/layer-editor-review-nits
branch
from
August 20, 2026 01:22
ce68a9f to
b1fb868
Compare
github-actions
Bot
added
the
risk:R2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary