refactor: error system cleanup — store separation, DDD fix, test improvements#10302
Draft
refactor: error system cleanup — store separation, DDD fix, test improvements#10302
Conversation
🎨 Storybook: ✅ Built — View Storybook |
Contributor
📝 WalkthroughWalkthroughExtracted missing-node error state into a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User / UI
participant App as App (rescan / workflow)
participant MissingStore as useMissingNodesErrorStore
participant ExecStore as useExecutionErrorStore
participant RightPanel as RightSidePanel / Error UI
participant GraphSync as useNodeErrorFlagSync
participant ErrorActions as useErrorActions
UI->>App: trigger rescan / detect missing node types
App->>MissingStore: surfaceMissingNodes(types, ExecStore.showErrorOverlay)
MissingStore-->>RightPanel: missingNodesError (reactive)
RightPanel->>GraphSync: watch changes -> reconcile node flags
UI->>RightPanel: click "Find on GitHub" / "Contact Support"
RightPanel->>ErrorActions: findOnGitHub / contactSupport
ErrorActions->>ExecStore: track telemetry
ErrorActions->>External: open URL / execute support command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
🎭 Playwright: ✅ 663 passed, 0 failed · 4 flaky📊 Browser Reports
|
📦 Bundle: 5.02 MB gzip 🔴 +617 BDetailsSummary
Category Glance App Entry Points — 22.7 kB (baseline 22.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.11 MB (baseline 1.11 MB) • 🔴 +2.43 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 75.9 kB (baseline 75.9 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 2 unchanged Panels & Settings — 478 kB (baseline 478 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 12 unchanged User & Accounts — 16.9 kB (baseline 16.9 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed / 2 unchanged Editors & Dialogs — 82 kB (baseline 82 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 59.4 kB (baseline 59.4 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 8 unchanged Data & Services — 2.94 MB (baseline 2.94 MB) • 🔴 +576 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 322 kB (baseline 322 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 14 added / 14 removed / 8 unchanged Vendor & Third-Party — 9.79 MB (baseline 9.79 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 8.24 MB (baseline 8.24 MB) • ⚪ 0 BBundles that do not match a named category
Status: 53 added / 53 removed / 79 unchanged ⚡ Performance Report
All metrics
Historical variance (last 10 runs)
Trend (last 10 commits on main)
Raw data{
"timestamp": "2026-03-20T12:16:24.436Z",
"gitSha": "17f6a60751f7b528e42699e8830d1701d790a7fe",
"branch": "refactor/error-system-cleanup",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2074.5660000000044,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.368,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 410.165,
"heapDeltaBytes": 20306572,
"domNodes": 22,
"jsHeapTotalBytes": 22806528,
"scriptDurationMs": 21.074999999999996,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999947
},
{
"name": "canvas-idle",
"durationMs": 2019.4930000000113,
"styleRecalcs": 11,
"styleRecalcDurationMs": 9.838999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 356.559,
"heapDeltaBytes": 20051376,
"domNodes": 22,
"jsHeapTotalBytes": 23068672,
"scriptDurationMs": 22.537999999999997,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "canvas-idle",
"durationMs": 2004.7230000000127,
"styleRecalcs": 12,
"styleRecalcDurationMs": 10.828999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 374.576,
"heapDeltaBytes": 20051704,
"domNodes": 23,
"jsHeapTotalBytes": 22544384,
"scriptDurationMs": 23.705,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999947
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2024.2220000000088,
"styleRecalcs": 86,
"styleRecalcDurationMs": 46.86800000000001,
"layouts": 12,
"layoutDurationMs": 3.89,
"taskDurationMs": 983.5770000000001,
"heapDeltaBytes": 16152384,
"domNodes": 69,
"jsHeapTotalBytes": 22544384,
"scriptDurationMs": 138.64499999999998,
"eventListeners": 30,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1982.7809999999886,
"styleRecalcs": 81,
"styleRecalcDurationMs": 51.06099999999999,
"layouts": 12,
"layoutDurationMs": 3.3289999999999993,
"taskDurationMs": 954.799,
"heapDeltaBytes": 15936228,
"domNodes": 66,
"jsHeapTotalBytes": 23068672,
"scriptDurationMs": 133.108,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999947
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2034.317000000101,
"styleRecalcs": 84,
"styleRecalcDurationMs": 44.324000000000005,
"layouts": 12,
"layoutDurationMs": 3.334,
"taskDurationMs": 968.8890000000001,
"heapDeltaBytes": 15944312,
"domNodes": 66,
"jsHeapTotalBytes": 23855104,
"scriptDurationMs": 133.126,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1758.403000000044,
"styleRecalcs": 30,
"styleRecalcDurationMs": 18.189,
"layouts": 6,
"layoutDurationMs": 0.7329999999999999,
"taskDurationMs": 312.191,
"heapDeltaBytes": 24505488,
"domNodes": 78,
"jsHeapTotalBytes": 19922944,
"scriptDurationMs": 24.46,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000073
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1721.0269999999923,
"styleRecalcs": 32,
"styleRecalcDurationMs": 17.624000000000006,
"layouts": 6,
"layoutDurationMs": 0.49799999999999994,
"taskDurationMs": 309.079,
"heapDeltaBytes": 24560040,
"domNodes": 79,
"jsHeapTotalBytes": 20709376,
"scriptDurationMs": 25.397999999999996,
"eventListeners": 21,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1713.653000000022,
"styleRecalcs": 32,
"styleRecalcDurationMs": 17.748,
"layouts": 6,
"layoutDurationMs": 0.5999999999999999,
"taskDurationMs": 297.816,
"heapDeltaBytes": 24411792,
"domNodes": 80,
"jsHeapTotalBytes": 20971520,
"scriptDurationMs": 21.213,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000073
},
{
"name": "dom-widget-clipping",
"durationMs": 536.9890000000055,
"styleRecalcs": 11,
"styleRecalcDurationMs": 7.615999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 321.935,
"heapDeltaBytes": 7056484,
"domNodes": 18,
"jsHeapTotalBytes": 12320768,
"scriptDurationMs": 58.449999999999996,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.65999999999999
},
{
"name": "dom-widget-clipping",
"durationMs": 592.0810000000074,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.041000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 353.72999999999996,
"heapDeltaBytes": 7480032,
"domNodes": 19,
"jsHeapTotalBytes": 12058624,
"scriptDurationMs": 67.67300000000002,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000027
},
{
"name": "dom-widget-clipping",
"durationMs": 579.9729999999954,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.095,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 348.781,
"heapDeltaBytes": 7448280,
"domNodes": 19,
"jsHeapTotalBytes": 12582912,
"scriptDurationMs": 64.894,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.65999999999999
},
{
"name": "large-graph-idle",
"durationMs": 2053.9070000000092,
"styleRecalcs": 12,
"styleRecalcDurationMs": 12.048000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 551.2760000000001,
"heapDeltaBytes": -20369164,
"domNodes": -260,
"jsHeapTotalBytes": 18022400,
"scriptDurationMs": 99.228,
"eventListeners": -135,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "large-graph-idle",
"durationMs": 2037.6739999999813,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.885,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 533.9929999999999,
"heapDeltaBytes": 4050996,
"domNodes": -265,
"jsHeapTotalBytes": 16199680,
"scriptDurationMs": 96.78299999999999,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "large-graph-idle",
"durationMs": 2019.5699999999306,
"styleRecalcs": 10,
"styleRecalcDurationMs": 10.278000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 519.2710000000001,
"heapDeltaBytes": -6158728,
"domNodes": -265,
"jsHeapTotalBytes": 15675392,
"scriptDurationMs": 89.27600000000001,
"eventListeners": -157,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "large-graph-pan",
"durationMs": 2130.970999999988,
"styleRecalcs": 69,
"styleRecalcDurationMs": 16.607,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1074.8629999999998,
"heapDeltaBytes": 16832256,
"domNodes": -267,
"jsHeapTotalBytes": 18268160,
"scriptDurationMs": 396.429,
"eventListeners": -159,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "large-graph-pan",
"durationMs": 2115.2079999999955,
"styleRecalcs": 68,
"styleRecalcDurationMs": 15.153,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1063.0230000000001,
"heapDeltaBytes": 13266424,
"domNodes": -270,
"jsHeapTotalBytes": 17219584,
"scriptDurationMs": 395.10499999999996,
"eventListeners": -161,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "large-graph-pan",
"durationMs": 2135.0499999999784,
"styleRecalcs": 68,
"styleRecalcDurationMs": 15.549,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1124.3529999999998,
"heapDeltaBytes": 1525316,
"domNodes": -268,
"jsHeapTotalBytes": 17743872,
"scriptDurationMs": 439.233,
"eventListeners": -161,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999947
},
{
"name": "minimap-idle",
"durationMs": 2029.440999999963,
"styleRecalcs": 10,
"styleRecalcDurationMs": 10.482999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 525.474,
"heapDeltaBytes": 2854436,
"domNodes": -267,
"jsHeapTotalBytes": 15937536,
"scriptDurationMs": 90.293,
"eventListeners": -159,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000073
},
{
"name": "minimap-idle",
"durationMs": 2017.2339999999735,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.793000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 525.898,
"heapDeltaBytes": 1727212,
"domNodes": -271,
"jsHeapTotalBytes": 16199680,
"scriptDurationMs": 92.19,
"eventListeners": -161,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.660000000000036
},
{
"name": "minimap-idle",
"durationMs": 2010.9579999999596,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.856999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 528.079,
"heapDeltaBytes": 1709044,
"domNodes": -269,
"jsHeapTotalBytes": 16461824,
"scriptDurationMs": 92.563,
"eventListeners": -159,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999947
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 544.2840000000047,
"styleRecalcs": 50,
"styleRecalcDurationMs": 14.316999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 359.061,
"heapDeltaBytes": 6059352,
"domNodes": 23,
"jsHeapTotalBytes": 14155776,
"scriptDurationMs": 124.514,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000027
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 553.9380000000165,
"styleRecalcs": 48,
"styleRecalcDurationMs": 11.34,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 364.08099999999996,
"heapDeltaBytes": 6367828,
"domNodes": 22,
"jsHeapTotalBytes": 14417920,
"scriptDurationMs": 123.58100000000002,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.65
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 591.6159999999309,
"styleRecalcs": 49,
"styleRecalcDurationMs": 13.834000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 366.725,
"heapDeltaBytes": 6651884,
"domNodes": 25,
"jsHeapTotalBytes": 13631488,
"scriptDurationMs": 128.65699999999998,
"eventListeners": 32,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "subgraph-idle",
"durationMs": 2000.9079999999813,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.091000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 341.3,
"heapDeltaBytes": 19236928,
"domNodes": 22,
"jsHeapTotalBytes": 23068672,
"scriptDurationMs": 19.211,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "subgraph-idle",
"durationMs": 2011.825999999985,
"styleRecalcs": 12,
"styleRecalcDurationMs": 10.338,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 363.84399999999994,
"heapDeltaBytes": 10672364,
"domNodes": 23,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 19.383999999999997,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.660000000000036
},
{
"name": "subgraph-idle",
"durationMs": 2020.3310000000556,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.007,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 355.19900000000007,
"heapDeltaBytes": 19362880,
"domNodes": 22,
"jsHeapTotalBytes": 23330816,
"scriptDurationMs": 20.195,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999947
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 2006.7319999999995,
"styleRecalcs": 84,
"styleRecalcDurationMs": 46.682,
"layouts": 16,
"layoutDurationMs": 4.17,
"taskDurationMs": 935.928,
"heapDeltaBytes": 2252268,
"domNodes": -191,
"jsHeapTotalBytes": 23470080,
"scriptDurationMs": 97.696,
"eventListeners": -157,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1975.3899999999476,
"styleRecalcs": 84,
"styleRecalcDurationMs": 46.048,
"layouts": 16,
"layoutDurationMs": 4.590000000000001,
"taskDurationMs": 904.8290000000001,
"heapDeltaBytes": 11276220,
"domNodes": 73,
"jsHeapTotalBytes": 23592960,
"scriptDurationMs": 100.86700000000002,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1990.6550000000607,
"styleRecalcs": 85,
"styleRecalcDurationMs": 53.026,
"layouts": 16,
"layoutDurationMs": 4.473,
"taskDurationMs": 923.55,
"heapDeltaBytes": 11948672,
"domNodes": 75,
"jsHeapTotalBytes": 23330816,
"scriptDurationMs": 103.49899999999998,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.660000000000036
},
{
"name": "vue-large-graph-idle",
"durationMs": 12000.580000000013,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 11987.934999999998,
"heapDeltaBytes": -36290764,
"domNodes": -8342,
"jsHeapTotalBytes": 17915904,
"scriptDurationMs": 611.3299999999999,
"eventListeners": -16474,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000073
},
{
"name": "vue-large-graph-idle",
"durationMs": 12082.936999999958,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 12073.86,
"heapDeltaBytes": -29190304,
"domNodes": -8342,
"jsHeapTotalBytes": 26566656,
"scriptDurationMs": 608.1070000000001,
"eventListeners": -16478,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.339999999999783
},
{
"name": "vue-large-graph-idle",
"durationMs": 12129.71099999993,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 12119.546000000002,
"heapDeltaBytes": -29724172,
"domNodes": -8342,
"jsHeapTotalBytes": 25780224,
"scriptDurationMs": 610.962,
"eventListeners": -16472,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.659999999999854
},
{
"name": "vue-large-graph-pan",
"durationMs": 14509.979999999985,
"styleRecalcs": 65,
"styleRecalcDurationMs": 13.298999999999978,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14486.965999999999,
"heapDeltaBytes": -21048940,
"domNodes": -8342,
"jsHeapTotalBytes": -19656704,
"scriptDurationMs": 887.418,
"eventListeners": -16468,
"totalBlockingTimeMs": 0,
"frameDurationMs": 20
},
{
"name": "vue-large-graph-pan",
"durationMs": 14014.067000000012,
"styleRecalcs": 65,
"styleRecalcDurationMs": 13.644000000000018,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 13995.694,
"heapDeltaBytes": -21070976,
"domNodes": -8342,
"jsHeapTotalBytes": -17559552,
"scriptDurationMs": 879.3740000000001,
"eventListeners": -16468,
"totalBlockingTimeMs": 0,
"frameDurationMs": 20
},
{
"name": "vue-large-graph-pan",
"durationMs": 14209.664000000088,
"styleRecalcs": 67,
"styleRecalcDurationMs": 14.913999999999984,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14188.384,
"heapDeltaBytes": -33944504,
"domNodes": -8344,
"jsHeapTotalBytes": -2093056,
"scriptDurationMs": 861.6240000000001,
"eventListeners": -16472,
"totalBlockingTimeMs": 38,
"frameDurationMs": 20
},
{
"name": "workflow-execution",
"durationMs": 460.6950000000438,
"styleRecalcs": 16,
"styleRecalcDurationMs": 25.243000000000002,
"layouts": 5,
"layoutDurationMs": 1.4580000000000002,
"taskDurationMs": 126.36699999999998,
"heapDeltaBytes": 4718464,
"domNodes": 166,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 30.474999999999994,
"eventListeners": 71,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.660000000000036
},
{
"name": "workflow-execution",
"durationMs": 451.652000000081,
"styleRecalcs": 19,
"styleRecalcDurationMs": 26.953999999999997,
"layouts": 5,
"layoutDurationMs": 1.3980000000000001,
"taskDurationMs": 126.35799999999998,
"heapDeltaBytes": 4492252,
"domNodes": 157,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 30.016000000000002,
"eventListeners": 71,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.65999999999999
},
{
"name": "workflow-execution",
"durationMs": 439.3489999999929,
"styleRecalcs": 17,
"styleRecalcDurationMs": 23.236,
"layouts": 4,
"layoutDurationMs": 1.0610000000000002,
"taskDurationMs": 112.44499999999996,
"heapDeltaBytes": 4379236,
"domNodes": 155,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 23.313,
"eventListeners": 71,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000027
}
]
} |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/rightSidePanel/errors/TabErrors.vue (1)
370-385:⚠️ Potential issue | 🟠 MajorThis focus-collapse loop still closes non-execution groups.
setSectionCollapsed()is still called for every group, somissing_node,swap_nodes, andmissing_modelend up collapsed wheneverfocusedErrorNodeIdis set. That keeps#10085unresolved; only execution groups should participate in this behavior.Suggested change
const prefix = `${graphNodeId}:` for (const group of allErrorGroups.value) { - const hasMatch = - group.type === 'execution' && - group.cards.some( - (card) => - card.graphNodeId === graphNodeId || - (card.nodeId?.startsWith(prefix) ?? false) - ) - setSectionCollapsed(group.title, !hasMatch) + if (group.type !== 'execution') continue + + const hasMatch = group.cards.some( + (card) => + card.graphNodeId === graphNodeId || + (card.nodeId?.startsWith(prefix) ?? false) + ) + setSectionCollapsed(group.title, !hasMatch) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/TabErrors.vue` around lines 370 - 385, The watcher on rightSidePanelStore.focusedErrorNodeId is collapsing every group because setSectionCollapsed(group.title, !) is invoked unconditionally; change the loop to only call setSectionCollapsed for groups where group.type === 'execution' (i.e., check group.type before evaluating hasMatch and invoking setSectionCollapsed), keeping the existing matching logic (card.graphNodeId / card.nodeId?.startsWith) and still clearing rightSidePanelStore.focusedErrorNodeId at the end.
🧹 Nitpick comments (6)
src/composables/graph/useErrorClearingHooks.test.ts (1)
319-337: Consider verifyingonWidgetChangedrestoration as well.This test verifies that
onConnectionsChangeis restored, but the implementation also wraps and restoresonWidgetChanged. For complete behavioral coverage, consider asserting on both callbacks:💡 Suggested addition
it('restores original node callbacks when a node is removed', () => { const graph = new LGraph() const node = new LGraphNode('test') node.addInput('clip', 'CLIP') + node.addWidget('number', 'steps', 20, () => undefined, {}) const originalOnConnectionsChange = vi.fn() + const originalOnWidgetChanged = vi.fn() node.onConnectionsChange = originalOnConnectionsChange + node.onWidgetChanged = originalOnWidgetChanged graph.add(node) installErrorClearingHooks(graph) // Callbacks should be chained (not the originals) expect(node.onConnectionsChange).not.toBe(originalOnConnectionsChange) + expect(node.onWidgetChanged).not.toBe(originalOnWidgetChanged) // Simulate node removal via the graph hook graph.onNodeRemoved!(node) // Original callbacks should be restored expect(node.onConnectionsChange).toBe(originalOnConnectionsChange) + expect(node.onWidgetChanged).toBe(originalOnWidgetChanged) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/graph/useErrorClearingHooks.test.ts` around lines 319 - 337, The test only asserts restoration of onConnectionsChange but installErrorClearingHooks also wraps onWidgetChanged; update the spec in the test case (the one creating LGraph, LGraphNode 'test', assigning originalOnConnectionsChange) to also set an originalOnWidgetChanged mock on node.onWidgetChanged, assert node.onWidgetChanged is replaced after installErrorClearingHooks, then after invoking graph.onNodeRemoved!(node) assert node.onWidgetChanged has been restored to the original mock; reference installErrorClearingHooks, node.onConnectionsChange, node.onWidgetChanged, and graph.onNodeRemoved in your changes.src/platform/nodeReplacement/useNodeReplacement.ts (1)
331-333: Update stale doc comments that still mention the execution error store.Implementation now removes types from
useMissingNodesErrorStore, so the block comments should match to avoid confusion.Suggested doc-only patch
- * replaced types from the execution error store. + * replaced types from the missing nodes error store. ... - * the succeeded types from the execution error store. + * the succeeded types from the missing nodes error store.Also applies to: 342-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/useNodeReplacement.ts` around lines 331 - 333, Update the stale doc comments in useNodeReplacement.ts that refer to "execution error store" to instead reference the current store name and behavior (useMissingNodesErrorStore); specifically update the comment block above the function that "Replaces all nodes in a single swap group and removes successfully replaced types from the execution error store" to mention removing types from useMissingNodesErrorStore (also fix the similar text at the later doc block around the other comment at lines ~342-344) so the prose matches the implementation in functions like useNodeReplacement and any helpers that call useMissingNodesErrorStore.src/components/rightSidePanel/errors/TabErrors.test.ts (1)
219-245: Prove the runtime error is rendered only once.This currently proves the dedicated panel exists, but it will still pass if the same runtime error also renders inside the accordion. Add a uniqueness or explicit negative assertion so the test actually covers the regression.
As per coding guidelines, "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly."💡 One simple way to tighten it
const runtimePanel = wrapper.find('[data-testid="runtime-error-panel"]') expect(runtimePanel.exists()).toBe(true) + expect( + wrapper.text().match(/RuntimeError: Out of memory/g) ?? [] + ).toHaveLength(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/TabErrors.test.ts` around lines 219 - 245, The test currently only asserts the dedicated runtime panel exists but not that the same error isn't also rendered inside the accordion; update the test in TabErrors.test.ts to assert uniqueness by checking that the runtime error text (e.g., "RuntimeError: Out of memory" or the node title "KSampler") appears exactly once or that the accordion container does not contain that text: after mountComponent (which supplies executionError.lastExecutionError and mocks getNodeByExecutionId) add an explicit negative assertion that the accordion element (find by its test id or selector) does not contain the runtime error content, or assert runtimePanel.length === 1 and no matching elements exist inside the accordion to ensure the runtime error is not duplicated.src/platform/nodeReplacement/missingNodeScan.test.ts (1)
23-25: Useundefinedin thegetCnrIdFromNodemock.The real helper returns
string | undefined, so returningnullhere introduces a state the production path never emits and can mask null-handling regressions.💡 Mock the real contract
vi.mock('@/platform/nodeReplacement/cnrIdUtil', () => ({ - getCnrIdFromNode: vi.fn(() => null) + getCnrIdFromNode: vi.fn(() => undefined) }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/missingNodeScan.test.ts` around lines 23 - 25, The mock for getCnrIdFromNode currently returns null which doesn't match the real contract (string | undefined); update the vi.mock for '@/platform/nodeReplacement/cnrIdUtil' so getCnrIdFromNode returns undefined instead (e.g., vi.fn(() => undefined) or vi.fn().mockReturnValue(undefined)) to mirror production behavior and avoid introducing a null-only state.src/components/rightSidePanel/errors/useErrorGroups.test.ts (1)
546-555: Assert the missing-node title, not just non-empty output.
length > 0only proves that some message was emitted. This will still pass ifgroupedErrorMessagesstops reflecting missing-node data and contains an unrelated entry instead.As per coding guidelines, "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly."💡 Stronger assertion
missingNodesStore.setMissingNodeTypes([ makeMissingNodeType('NodeA', { cnrId: 'pack-1' }) ]) await nextTick() - expect(groups.groupedErrorMessages.value.length).toBeGreaterThan(0) + const missingGroup = groups.allErrorGroups.value.find( + (g) => g.type === 'missing_node' + ) + if (!missingGroup) throw new Error('missing_node group was not created') + expect(groups.groupedErrorMessages.value).toContain(missingGroup.title)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/useErrorGroups.test.ts` around lines 546 - 555, Replace the weak length-based assertion with a specific check that the missing-node title emitted by createErrorGroups()/makeMissingNodeType appears in the grouped messages: after setting useMissingNodesErrorStore().setMissingNodeTypes([makeMissingNodeType('NodeA', { cnrId: 'pack-1' })]) and awaiting nextTick(), assert that groups.groupedErrorMessages.value contains the expected title string produced for the missing node (e.g., "NodeA" or the exact title format returned by makeMissingNodeType) rather than only asserting length > 0.src/components/rightSidePanel/errors/ErrorNodeCard.test.ts (1)
232-240: Assert the fallbackserverLogspayload here.
mockGenerateErrorReportalways returns a report, so this still passes even if the fallback log string is never forwarded. Please assert the call payload as well; otherwise this recovery path is not really covered.As per coding guidelines: "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly."🧪 Suggested assertion
expect(mockGenerateErrorReport).toHaveBeenCalledOnce() + expect(mockGenerateErrorReport).toHaveBeenCalledWith( + expect.objectContaining({ + serverLogs: 'Failed to retrieve server logs' + }) + ) expect(wrapper.text()).toContain('ComfyUI Error Report')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/ErrorNodeCard.test.ts` around lines 232 - 240, The test 'generates report with fallback logs when getLogs fails' currently only checks that mockGenerateErrorReport was called and the UI text; update it to assert the actual payload passed to mockGenerateErrorReport contains the fallback serverLogs string. After calling mountCard(makeRuntimeErrorCard()) and await flushPromises(), inspect mockGenerateErrorReport.mock.calls[0] and assert that the first argument's serverLogs (or payload.serverLogs) equals or contains the expected fallback message (the string your code sets when mockGetLogs rejects); keep using mockGetLogs.mockRejectedValue(...) and ensure this assertion fails if the fallback is not forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@browser_tests/tests/dialog.spec.ts`:
- Around line 31-32: Replace fragile getByText(/Missing Node Packs/) and similar
text-based assertions with the centralized test IDs from fixtures/selectors.ts:
use the overlay group test IDs (e.g. TestIds.MISSING_NODE_PACKS_GROUP and
TestIds.MISSING_MODELS_GROUP) with errorOverlay.getByTestId(...) and
expect(...).toBeVisible() so the tests no longer depend on English copy or
locale; update the occurrences referenced (around the blocks currently using
missingNodesTitle and the analogous missing models checks at the other ranges)
to use these TestIds instead.
- Around line 147-180: The test currently uses page-global English button labels
which break under non-English locales; change it to scope to the runtime error
panel and use centralized TestIds for locale-safe selectors: after opening the
overlay, anchor to TestIds.dialogs.runtimeErrorPanel (use
comfyPage.page.getByTestId(TestIds.dialogs.runtimeErrorPanel) or similar) and
replace getByRole(name: 'See Errors' / 'Find on GitHub' / 'Copy') with
getByTestId calls that reference the centralized IDs from fixtures/selectors.ts
(e.g., TestIds.dialogs.seeErrorsButton, TestIds.dialogs.findOnGithubButton,
TestIds.dialogs.copyButton), and use the runtime panel locator to query those
buttons so the assertions only target the intended runtime-action UI.
In `@src/components/rightSidePanel/errors/useErrorReport.ts`:
- Around line 60-73: The call to app.rootGraph.serialize() is outside the
try/catch and can throw, preventing fallback error reporting; move or wrap
serialization inside the same try/catch used for generateErrorReport (or add a
small try/catch just around app.rootGraph.serialize()) so that if serialize()
fails you catch the error and still call generateErrorReport with a safe
fallback (e.g., null or an indicative string) for the workflow field; update the
block that iterates runtimeErrors (and the generateErrorReport invocation) to
use the guarded workflow value so serialization failures don't reject the
mounting hook.
- Around line 42-50: The code is triggering refetchSystemStats while the store's
initial fetch may already be in flight; update the logic in useErrorReport to
avoid calling systemStatsStore.refetchSystemStats when the store is currently
loading. Specifically, check the store's loading flag (e.g.,
systemStatsStore.isLoading or systemStatsStore.loading) before calling
refetchSystemStats: only call refetchSystemStats when systemStats is null AND
the store is not loading, otherwise wait for the in-flight load to resolve (or
rely on the store's populated systemStats) to prevent duplicate requests and
race conditions.
In `@src/composables/graph/useNodeErrorFlagSync.ts`:
- Around line 83-103: The watcher for reconciling node error flags misses the
Errors Tab setting as a reactive source, so toggling
Comfy.RightSidePanel.ShowErrorsTab won't retrigger the watcher; update the watch
sources to include the setting (e.g., add a computed or arrow function that
returns useSettingStore().get('Comfy.RightSidePanel.ShowErrorsTab')) alongside
lastNodeErrors and missingModelStore.missingModelNodeIds so that
reconcileNodeErrorFlags(app.rootGraph, lastNodeErrors.value, ...) runs when the
setting changes; keep the rest of the logic (app.isGraphReady guard,
missingModelAncestorExecutionIds branch) unchanged.
---
Outside diff comments:
In `@src/components/rightSidePanel/errors/TabErrors.vue`:
- Around line 370-385: The watcher on rightSidePanelStore.focusedErrorNodeId is
collapsing every group because setSectionCollapsed(group.title, !) is invoked
unconditionally; change the loop to only call setSectionCollapsed for groups
where group.type === 'execution' (i.e., check group.type before evaluating
hasMatch and invoking setSectionCollapsed), keeping the existing matching logic
(card.graphNodeId / card.nodeId?.startsWith) and still clearing
rightSidePanelStore.focusedErrorNodeId at the end.
---
Nitpick comments:
In `@src/components/rightSidePanel/errors/ErrorNodeCard.test.ts`:
- Around line 232-240: The test 'generates report with fallback logs when
getLogs fails' currently only checks that mockGenerateErrorReport was called and
the UI text; update it to assert the actual payload passed to
mockGenerateErrorReport contains the fallback serverLogs string. After calling
mountCard(makeRuntimeErrorCard()) and await flushPromises(), inspect
mockGenerateErrorReport.mock.calls[0] and assert that the first argument's
serverLogs (or payload.serverLogs) equals or contains the expected fallback
message (the string your code sets when mockGetLogs rejects); keep using
mockGetLogs.mockRejectedValue(...) and ensure this assertion fails if the
fallback is not forwarded.
In `@src/components/rightSidePanel/errors/TabErrors.test.ts`:
- Around line 219-245: The test currently only asserts the dedicated runtime
panel exists but not that the same error isn't also rendered inside the
accordion; update the test in TabErrors.test.ts to assert uniqueness by checking
that the runtime error text (e.g., "RuntimeError: Out of memory" or the node
title "KSampler") appears exactly once or that the accordion container does not
contain that text: after mountComponent (which supplies
executionError.lastExecutionError and mocks getNodeByExecutionId) add an
explicit negative assertion that the accordion element (find by its test id or
selector) does not contain the runtime error content, or assert
runtimePanel.length === 1 and no matching elements exist inside the accordion to
ensure the runtime error is not duplicated.
In `@src/components/rightSidePanel/errors/useErrorGroups.test.ts`:
- Around line 546-555: Replace the weak length-based assertion with a specific
check that the missing-node title emitted by
createErrorGroups()/makeMissingNodeType appears in the grouped messages: after
setting
useMissingNodesErrorStore().setMissingNodeTypes([makeMissingNodeType('NodeA', {
cnrId: 'pack-1' })]) and awaiting nextTick(), assert that
groups.groupedErrorMessages.value contains the expected title string produced
for the missing node (e.g., "NodeA" or the exact title format returned by
makeMissingNodeType) rather than only asserting length > 0.
In `@src/composables/graph/useErrorClearingHooks.test.ts`:
- Around line 319-337: The test only asserts restoration of onConnectionsChange
but installErrorClearingHooks also wraps onWidgetChanged; update the spec in the
test case (the one creating LGraph, LGraphNode 'test', assigning
originalOnConnectionsChange) to also set an originalOnWidgetChanged mock on
node.onWidgetChanged, assert node.onWidgetChanged is replaced after
installErrorClearingHooks, then after invoking graph.onNodeRemoved!(node) assert
node.onWidgetChanged has been restored to the original mock; reference
installErrorClearingHooks, node.onConnectionsChange, node.onWidgetChanged, and
graph.onNodeRemoved in your changes.
In `@src/platform/nodeReplacement/missingNodeScan.test.ts`:
- Around line 23-25: The mock for getCnrIdFromNode currently returns null which
doesn't match the real contract (string | undefined); update the vi.mock for
'@/platform/nodeReplacement/cnrIdUtil' so getCnrIdFromNode returns undefined
instead (e.g., vi.fn(() => undefined) or vi.fn().mockReturnValue(undefined)) to
mirror production behavior and avoid introducing a null-only state.
In `@src/platform/nodeReplacement/useNodeReplacement.ts`:
- Around line 331-333: Update the stale doc comments in useNodeReplacement.ts
that refer to "execution error store" to instead reference the current store
name and behavior (useMissingNodesErrorStore); specifically update the comment
block above the function that "Replaces all nodes in a single swap group and
removes successfully replaced types from the execution error store" to mention
removing types from useMissingNodesErrorStore (also fix the similar text at the
later doc block around the other comment at lines ~342-344) so the prose matches
the implementation in functions like useNodeReplacement and any helpers that
call useMissingNodesErrorStore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d992ca3b-d72f-440f-835c-aac5ca5f32b2
📒 Files selected for processing (37)
browser_tests/fixtures/selectors.tsbrowser_tests/tests/dialog.spec.tsbrowser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.tssrc/components/dialog/content/error/FindIssueButton.vuesrc/components/rightSidePanel/RightSidePanel.vuesrc/components/rightSidePanel/errors/ErrorNodeCard.test.tssrc/components/rightSidePanel/errors/ErrorNodeCard.vuesrc/components/rightSidePanel/errors/MissingNodeCard.test.tssrc/components/rightSidePanel/errors/MissingPackGroupRow.vuesrc/components/rightSidePanel/errors/TabErrors.test.tssrc/components/rightSidePanel/errors/TabErrors.vuesrc/components/rightSidePanel/errors/swapNodeGroups.test.tssrc/components/rightSidePanel/errors/types.tssrc/components/rightSidePanel/errors/useErrorActions.tssrc/components/rightSidePanel/errors/useErrorGroups.test.tssrc/components/rightSidePanel/errors/useErrorGroups.tssrc/components/rightSidePanel/errors/useErrorReport.tssrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/composables/graph/useNodeErrorFlagSync.tssrc/locales/en/main.jsonsrc/platform/nodeReplacement/cnrIdUtil.test.tssrc/platform/nodeReplacement/cnrIdUtil.tssrc/platform/nodeReplacement/missingNodeScan.test.tssrc/platform/nodeReplacement/missingNodeScan.tssrc/platform/nodeReplacement/missingNodesErrorStore.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/platform/nodeReplacement/useNodeReplacement.tssrc/platform/workflow/core/services/workflowService.test.tssrc/platform/workflow/core/services/workflowService.tssrc/renderer/extensions/vueNodes/VideoPreview.vuesrc/renderer/extensions/vueNodes/components/ImagePreview.vuesrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/scripts/app.tssrc/stores/executionErrorStore.test.tssrc/stores/executionErrorStore.tssrc/stores/executionStore.test.ts
Comment on lines
+31
to
32
| const missingNodesTitle = errorOverlay.getByText(/Missing Node Packs/) | ||
| await expect(missingNodesTitle).toBeVisible() |
Contributor
There was a problem hiding this comment.
Use the new group test IDs here instead of English overlay copy.
These assertions still depend on the literal “Missing Node Packs / Missing Models” text, which leaves #10033 unresolved and will break on locale or copy changes. This PR already adds dedicated selectors for both groups, so the overlay checks can be made stable now.
Suggested change
- const missingNodesTitle = errorOverlay.getByText(/Missing Node Packs/)
+ const missingNodesTitle = errorOverlay.getByTestId(
+ TestIds.dialogs.missingNodePacksGroup
+ )
await expect(missingNodesTitle).toBeVisible()- const missingModelsTitle = errorOverlay.getByText(/Missing Models/)
+ const missingModelsTitle = errorOverlay.getByTestId(
+ TestIds.dialogs.missingModelsGroup
+ )
await expect(missingModelsTitle).toBeVisible()As per coding guidelines, use centralized TestIds from fixtures/selectors.ts for DOM element selection in E2E tests.
Also applies to: 45-46, 207-208, 223-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@browser_tests/tests/dialog.spec.ts` around lines 31 - 32, Replace fragile
getByText(/Missing Node Packs/) and similar text-based assertions with the
centralized test IDs from fixtures/selectors.ts: use the overlay group test IDs
(e.g. TestIds.MISSING_NODE_PACKS_GROUP and TestIds.MISSING_MODELS_GROUP) with
errorOverlay.getByTestId(...) and expect(...).toBeVisible() so the tests no
longer depend on English copy or locale; update the occurrences referenced
(around the blocks currently using missingNodesTitle and the analogous missing
models checks at the other ranges) to use these TestIds instead.
Comment on lines
+147
to
+180
| test.describe('Error actions in Errors Tab', { tag: '@ui' }, () => { | ||
| test.beforeEach(async ({ comfyPage }) => { | ||
| await comfyPage.settings.setSetting('Comfy.UseNewMenu', 'Top') | ||
| await comfyPage.settings.setSetting( | ||
| 'Comfy.RightSidePanel.ShowErrorsTab', | ||
| true | ||
| ) | ||
| }) | ||
|
|
||
| test('Should show Find on GitHub and Copy buttons in error card after execution error', async ({ | ||
| comfyPage | ||
| }) => { | ||
| await comfyPage.workflow.loadWorkflow('nodes/execution_error') | ||
| await comfyPage.command.executeCommand('Comfy.QueuePrompt') | ||
| await comfyPage.nextFrame() | ||
|
|
||
| // Wait for error overlay and click "See Errors" | ||
| const errorOverlay = comfyPage.page.getByTestId( | ||
| TestIds.dialogs.errorOverlay | ||
| ) | ||
| await expect(errorOverlay).toBeVisible() | ||
| await errorOverlay.getByRole('button', { name: 'See Errors' }).click() | ||
| await expect(errorOverlay).not.toBeVisible() | ||
|
|
||
| // Verify Find on GitHub button is present in the error card | ||
| const findOnGithubButton = comfyPage.page.getByRole('button', { | ||
| name: 'Find on GitHub' | ||
| }) | ||
| await expect(findOnGithubButton).toBeVisible() | ||
|
|
||
| // Verify Copy button is present in the error card | ||
| const copyButton = comfyPage.page.getByRole('button', { name: 'Copy' }) | ||
| await expect(copyButton).toBeVisible() | ||
| }) |
Contributor
There was a problem hiding this comment.
Scope the new runtime-action test and make the button selectors locale-safe.
This new spec still uses page-global English button names (See Errors, Find on GitHub, Copy), so it can match unrelated controls and still fails under non-English locales. Please anchor the assertions to TestIds.dialogs.runtimeErrorPanel, then resolve the buttons via a centralized test id or a locale-aware selector instead of raw English text.
As per coding guidelines, use centralized TestIds from fixtures/selectors.ts for DOM element selection in E2E tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@browser_tests/tests/dialog.spec.ts` around lines 147 - 180, The test
currently uses page-global English button labels which break under non-English
locales; change it to scope to the runtime error panel and use centralized
TestIds for locale-safe selectors: after opening the overlay, anchor to
TestIds.dialogs.runtimeErrorPanel (use
comfyPage.page.getByTestId(TestIds.dialogs.runtimeErrorPanel) or similar) and
replace getByRole(name: 'See Errors' / 'Find on GitHub' / 'Copy') with
getByTestId calls that reference the centralized IDs from fixtures/selectors.ts
(e.g., TestIds.dialogs.seeErrorsButton, TestIds.dialogs.findOnGithubButton,
TestIds.dialogs.copyButton), and use the runtime panel locator to query those
buttons so the assertions only target the intended runtime-action UI.
…ovements - Extract missingNodesErrorStore from executionErrorStore, remove delegation pattern - Extract useNodeErrorFlagSync composable for node error flag reconciliation - Move getCnrIdFromNode/getCnrIdFromProperties to platform layer (DDD fix) - Add explicit callback cleanup on node removal in useErrorClearingHooks - Fix mock hoisting inconsistency in MissingNodeCard.test.ts - Add data-testid to error groups, image/video error spans - Update E2E tests to use scoped locators and testids Fixes #9875, Fixes #10027, Fixes #10033, Fixes #10085
…uality - Fix per-node callback restoration on cleanup in useErrorClearingHooks - Add unmount cancellation guard to useErrorReport async onMounted - Move missingNodesErrorStore to platform/nodeReplacement (DDD alignment) - Extract activeMissingNodeGraphIds to component-level computed - Extract useErrorActions composable (deduplicate telemetry+command pattern) - Add data-testid to copy button, use in test selector - Add tests for onNodeRemoved restoration and double-install guard - Return watch stop handle from useNodeErrorFlagSync - Add asyncResolvedIds eviction on missingNodesError reset - Add console.warn to silent catch blocks and empty array guard - Hoist useCommandStore to setup scope, fix floating promises - Replace replace() with replaceAll() in testid expression - Convert function expression to declaration in FindIssueButton
jaeone94
force-pushed
the
refactor/error-system-cleanup
branch
from
6ed156a to
9f3cacf
Compare
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/platform/nodeReplacement/useNodeReplacement.ts (1)
334-350:⚠️ Potential issue | 🟠 MajorDon’t clear missing-node entries by type after only a partial replacement.
replaceNodesInPlace()records a type as soon as the first node of that type succeeds, but these callers then prune the error store by type. If another instance of the same type fails later or the loop aborts, the remaining unresolved placeholders disappear from the Errors tab even though they are still missing. Remove by successfully replaced node ids/execution ids, or only prune a type after all of its instances finish successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/nodeReplacement/useNodeReplacement.ts` around lines 334 - 350, The current logic in replaceGroup and replaceAllGroups incorrectly calls useMissingNodesErrorStore().removeMissingNodesByType(replaced) because replaceNodesInPlace() marks a type as "replaced" on the first success, which can prematurely clear remaining missing placeholders; change the callers to remove by the specific successfully replaced node identifiers (or execution ids) instead of by type, or only call removeMissingNodesByType for a given type after all instances of that type completed successfully. Concretely: update replaceNodesInPlace to return the list of concrete replaced node ids/execution ids (not just types) or return a map of type→successfulIds, then in replaceGroup and replaceAllGroups call the error store removal API that accepts ids (e.g., removeMissingNodesById or a new removeMissingNodesSuccesses) or check counts so you only invoke removeMissingNodesByType when every instance of a type is confirmed replaced; keep references to replaceNodesInPlace, replaceGroup, replaceAllGroups, and useMissingNodesErrorStore().removeMissingNodesByType when making the change.
♻️ Duplicate comments (1)
src/components/rightSidePanel/errors/useErrorReport.ts (1)
42-50:⚠️ Potential issue | 🟡 MinorAvoid triggering a second stats fetch while initial load may still be in flight.
!systemStatsStore.systemStatscan be true during the store’s first async load, so this branch may still callrefetchSystemStats()unnecessarily.Please verify store-level dedupe/loading semantics first:
#!/bin/bash set -euo pipefail STORE_FILE="$(fd 'systemStatsStore.ts$' src | head -n 1)" echo "Inspecting: ${STORE_FILE}" rg -n -C3 'defineStore|systemStats|refetchSystemStats|isLoading|loading|fetch' "${STORE_FILE}"Expected: either
refetchSystemStatsis explicitly no-op/deduped while loading, or this caller should guard on a loading flag before refetching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/useErrorReport.ts` around lines 42 - 50, The guard currently calls systemStatsStore.refetchSystemStats() whenever !systemStatsStore.systemStats, which can trigger a duplicate fetch during the store's initial async load; update the logic to first check the store's loading/dedupe flag (e.g., systemStatsStore.isLoading or systemStatsStore.loading) and only call refetchSystemStats() if not already loading, or alternatively make refetchSystemStats itself no-op when a fetch is in-flight; locate the usage around systemStatsStore.systemStats and refetchSystemStats and add the loading check (or make refetchSystemStats dedupe internally) so we never start a second fetch while the initial load is in flight and keep the existing cancelled and null checks intact.
🧹 Nitpick comments (1)
src/components/rightSidePanel/errors/useErrorReport.ts (1)
58-60: Remove the duplicated cancellation guard.Line 58 and Line 60 perform the same immediate
if (cancelled) returncheck; the second one is redundant.Suggested cleanup
- if (cancelled) return - if (cancelled) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/useErrorReport.ts` around lines 58 - 60, The code contains a duplicated cancellation guard in the async flow inside useErrorReport (two consecutive `if (cancelled) return` checks); remove the redundant second guard so only a single `if (cancelled) return` remains (locate the duplicate inside the useErrorReport function and delete the extra occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/nodeReplacement/missingNodeScan.test.ts`:
- Around line 23-25: The mock for getCnrIdFromNode in missingNodeScan.test.ts
currently returns null which widens the helper's contract; update the vi.mock
setup so getCnrIdFromNode returns undefined (or no value) instead of null to
match the real helper's string | undefined return type; locate the mock
definition for getCnrIdFromNode and change its implementation to return
undefined so tests exercise the same "missing CNR id" branch as production.
In `@src/platform/nodeReplacement/missingNodesErrorStore.ts`:
- Around line 83-85: The computed missingNodeCount currently collapses any
non-empty missingNodesError to 1; update missingNodeCount to return the actual
number of missing nodes by using the real collection size (e.g.,
missingNodesError.value.size for a Set or missingNodesError.value.length for an
Array) instead of (missingNodesError.value ? 1 : 0); keep hasMissingNodes as is
and ensure missingNodeCount handles null/undefined safely (return 0 when
missingNodesError.value is falsy).
---
Outside diff comments:
In `@src/platform/nodeReplacement/useNodeReplacement.ts`:
- Around line 334-350: The current logic in replaceGroup and replaceAllGroups
incorrectly calls useMissingNodesErrorStore().removeMissingNodesByType(replaced)
because replaceNodesInPlace() marks a type as "replaced" on the first success,
which can prematurely clear remaining missing placeholders; change the callers
to remove by the specific successfully replaced node identifiers (or execution
ids) instead of by type, or only call removeMissingNodesByType for a given type
after all instances of that type completed successfully. Concretely: update
replaceNodesInPlace to return the list of concrete replaced node ids/execution
ids (not just types) or return a map of type→successfulIds, then in replaceGroup
and replaceAllGroups call the error store removal API that accepts ids (e.g.,
removeMissingNodesById or a new removeMissingNodesSuccesses) or check counts so
you only invoke removeMissingNodesByType when every instance of a type is
confirmed replaced; keep references to replaceNodesInPlace, replaceGroup,
replaceAllGroups, and useMissingNodesErrorStore().removeMissingNodesByType when
making the change.
---
Duplicate comments:
In `@src/components/rightSidePanel/errors/useErrorReport.ts`:
- Around line 42-50: The guard currently calls
systemStatsStore.refetchSystemStats() whenever !systemStatsStore.systemStats,
which can trigger a duplicate fetch during the store's initial async load;
update the logic to first check the store's loading/dedupe flag (e.g.,
systemStatsStore.isLoading or systemStatsStore.loading) and only call
refetchSystemStats() if not already loading, or alternatively make
refetchSystemStats itself no-op when a fetch is in-flight; locate the usage
around systemStatsStore.systemStats and refetchSystemStats and add the loading
check (or make refetchSystemStats dedupe internally) so we never start a second
fetch while the initial load is in flight and keep the existing cancelled and
null checks intact.
---
Nitpick comments:
In `@src/components/rightSidePanel/errors/useErrorReport.ts`:
- Around line 58-60: The code contains a duplicated cancellation guard in the
async flow inside useErrorReport (two consecutive `if (cancelled) return`
checks); remove the redundant second guard so only a single `if (cancelled)
return` remains (locate the duplicate inside the useErrorReport function and
delete the extra occurrence).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb6dcaaa-0c5c-4313-b995-cd6847739771
📒 Files selected for processing (33)
browser_tests/fixtures/selectors.tsbrowser_tests/tests/dialog.spec.tsbrowser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.tssrc/components/dialog/content/error/FindIssueButton.vuesrc/components/rightSidePanel/RightSidePanel.vuesrc/components/rightSidePanel/errors/ErrorNodeCard.vuesrc/components/rightSidePanel/errors/MissingNodeCard.test.tssrc/components/rightSidePanel/errors/TabErrors.test.tssrc/components/rightSidePanel/errors/TabErrors.vuesrc/components/rightSidePanel/errors/swapNodeGroups.test.tssrc/components/rightSidePanel/errors/useErrorActions.tssrc/components/rightSidePanel/errors/useErrorGroups.test.tssrc/components/rightSidePanel/errors/useErrorGroups.tssrc/components/rightSidePanel/errors/useErrorReport.tssrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/composables/graph/useNodeErrorFlagSync.tssrc/platform/nodeReplacement/cnrIdUtil.test.tssrc/platform/nodeReplacement/cnrIdUtil.tssrc/platform/nodeReplacement/missingNodeScan.test.tssrc/platform/nodeReplacement/missingNodeScan.tssrc/platform/nodeReplacement/missingNodesErrorStore.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/platform/nodeReplacement/useNodeReplacement.tssrc/platform/workflow/core/services/workflowService.test.tssrc/platform/workflow/core/services/workflowService.tssrc/renderer/extensions/vueNodes/VideoPreview.vuesrc/renderer/extensions/vueNodes/components/ImagePreview.vuesrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/scripts/app.tssrc/stores/executionErrorStore.test.tssrc/stores/executionErrorStore.tssrc/stores/executionStore.test.ts
✅ Files skipped from review due to trivial changes (6)
- src/platform/nodeReplacement/cnrIdUtil.test.ts
- src/renderer/extensions/vueNodes/components/ImagePreview.vue
- src/renderer/extensions/vueNodes/VideoPreview.vue
- src/composables/graph/useErrorClearingHooks.test.ts
- src/composables/graph/useNodeErrorFlagSync.ts
- browser_tests/fixtures/selectors.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/components/dialog/content/error/FindIssueButton.vue
- src/components/rightSidePanel/errors/swapNodeGroups.test.ts
- browser_tests/tests/dialog.spec.ts
- src/components/rightSidePanel/errors/TabErrors.test.ts
- src/platform/nodeReplacement/useNodeReplacement.test.ts
- src/components/rightSidePanel/errors/MissingNodeCard.test.ts
- src/renderer/extensions/vueNodes/components/LGraphNode.vue
- src/composables/graph/useErrorClearingHooks.ts
- src/platform/workflow/core/services/workflowService.test.ts
- src/components/rightSidePanel/errors/useErrorGroups.ts
- browser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.ts
- src/stores/executionErrorStore.test.ts
- src/platform/nodeReplacement/missingNodeScan.ts
- src/components/rightSidePanel/errors/TabErrors.vue
- src/components/rightSidePanel/RightSidePanel.vue
- src/components/rightSidePanel/errors/ErrorNodeCard.vue
- src/platform/workflow/core/services/workflowService.ts
- src/components/rightSidePanel/errors/useErrorGroups.test.ts
- Fix focusedErrorNodeId watch collapsing non-execution groups (#10085) - Fix missingNodeCount always returning 0/1 instead of actual count - Add ShowErrorsTab setting to useNodeErrorFlagSync watch sources - Guard rootGraph.serialize() with try/catch, use const declarations - Deduplicate cancelled guard in useErrorReport - Await initial systemStats loading before refetching - Fix mock return type null → undefined in missingNodeScan test - Update stale doc comments referencing executionErrorStore - Strengthen test assertions: onWidgetChanged restoration, runtime error uniqueness, missing-node title verification, fallback logs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/rightSidePanel/errors/useErrorReport.ts (1)
58-60: Log-fetch failures are still swallowed without diagnostics.This catch returns a fallback string but drops the underlying error context, which makes field debugging harder when
/logsintermittently fails.♻️ Suggested tweak
- const logs = await api - .getLogs() - .catch(() => 'Failed to retrieve server logs') + const logs = await api.getLogs().catch((e) => { + console.warn('Failed to retrieve server logs for error report:', e) + return 'Failed to retrieve server logs' + })As per coding guidelines: "Implement proper error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/useErrorReport.ts` around lines 58 - 60, The current useErrorReport code swallows the original error by using api.getLogs().catch(() => 'Failed to retrieve server logs'), losing diagnostics; update the logs retrieval around api.getLogs() (in useErrorReport) to catch the error, log or attach the original error details (e.g., include the caught error in processLogger/error reporting or return an object like { message: 'Failed to retrieve server logs', error }) and rethrow or propagate an enriched error so callers and telemetry can see the underlying exception from api.getLogs().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/rightSidePanel/errors/useErrorReport.ts`:
- Around line 58-60: The current useErrorReport code swallows the original error
by using api.getLogs().catch(() => 'Failed to retrieve server logs'), losing
diagnostics; update the logs retrieval around api.getLogs() (in useErrorReport)
to catch the error, log or attach the original error details (e.g., include the
caught error in processLogger/error reporting or return an object like {
message: 'Failed to retrieve server logs', error }) and rethrow or propagate an
enriched error so callers and telemetry can see the underlying exception from
api.getLogs().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 989940a0-a53b-4b53-80e6-0c9ccca3e428
📒 Files selected for processing (10)
src/components/rightSidePanel/errors/ErrorNodeCard.test.tssrc/components/rightSidePanel/errors/TabErrors.test.tssrc/components/rightSidePanel/errors/TabErrors.vuesrc/components/rightSidePanel/errors/useErrorGroups.test.tssrc/components/rightSidePanel/errors/useErrorReport.tssrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useNodeErrorFlagSync.tssrc/platform/nodeReplacement/missingNodeScan.test.tssrc/platform/nodeReplacement/missingNodesErrorStore.tssrc/platform/nodeReplacement/useNodeReplacement.ts
✅ Files skipped from review due to trivial changes (2)
- src/composables/graph/useErrorClearingHooks.test.ts
- src/platform/nodeReplacement/missingNodeScan.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/rightSidePanel/errors/TabErrors.test.ts
- src/platform/nodeReplacement/useNodeReplacement.ts
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
1 participant
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
Refactors the error system to improve separation of concerns, fix DDD layer violations, and address code quality issues.
missingNodesErrorStorefromexecutionErrorStore, removing the delegation pattern that coupled missing-node logic into the execution error storeuseNodeErrorFlagSynccomposable for node error flag reconciliation (previously inlined)useErrorClearingHookscomposable with explicit callback cleanup on node removaluseErrorActionscomposable to deduplicate telemetry+command patterns across error card componentsgetCnrIdFromNode/getCnrIdFromPropertiestoplatform/nodeReplacementlayer (DDD fix)missingNodesErrorStoretoplatform/nodeReplacement(DDD alignment)useErrorReportasynconMounteduseNodeErrorFlagSyncasyncResolvedIdseviction onmissingNodesErrorresetconsole.warnto silent catch blocks and empty array guarduseCommandStoreto setup scope, fix floating promisesdata-testidto error groups, image/video error spans, copy buttononNodeRemovedrestoration and double-install guardFixes #9875, Fixes #10027, Fixes #10033, Fixes #10085
Test plan
useErrorClearingHooks(callback restoration, double-install guard)data-testid┆Issue is synchronized with this Notion page by Unito