fix: re-check tab fallback after deferred error scans - #15012
Merged
Conversation
Paste registers widgets in reactive stores before node:added fires, so the Vue flush that recomputes the panel tabs can run before the deferred missing-model scan registers the pasted node's error. The fallback watchEffect then saw no relevant errors for the selection and flipped the panel off the Errors tab permanently. Re-check one microtask later so in-flight scans can keep the active tab.
dosubot
Bot
added
the
size:S
🎭 Playwright: ✅ 1798 passed, 0 failed · 5 flaky📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 8.72 MB gzip 🔴 +934 BDetailsSummary
Category Glance App Entry Points — 3.67 kB (baseline 3.67 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.37 MB (baseline 1.36 MB) • 🔴 +4 kBGraph 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 — 565 kB (baseline 565 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 16 unchanged User & Accounts — 27 kB (baseline 27 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed / 4 unchanged Editors & Dialogs — 125 kB (baseline 125 kB) • ⚪ 0 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.51 MB (baseline 3.51 MB) • 🔴 +979 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 550 kB (baseline 550 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 18 added / 18 removed / 20 unchanged Vendor & Third-Party — 16.3 MB (baseline 16.3 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 17 unchanged Other — 14.2 MB (baseline 14.2 MB) • ⚪ 0 BBundles that do not match a named category
Status: 68 added / 68 removed / 217 unchanged ⚡ Performance Report
Show regressions
All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-08-13T00:49:21.968Z",
"gitSha": "e4ae50204d11717162a4c3b58ed1af5bab97be81",
"branch": "drjkl/tab-fallback-deferred-scans",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2050.0380000000005,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.591000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 565.1890000000001,
"heapDeltaBytes": 5962584,
"heapUsedBytes": 70656148,
"domNodes": 16,
"jsHeapTotalBytes": 24117248,
"scriptDurationMs": 21.833,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2020.483000000013,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.2959999999999985,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 516.7570000000001,
"heapDeltaBytes": 5826096,
"heapUsedBytes": 70191228,
"domNodes": 16,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 19.123,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1864.0579999999716,
"styleRecalcs": 76,
"styleRecalcDurationMs": 46.187,
"layouts": 12,
"layoutDurationMs": 3.6270000000000002,
"taskDurationMs": 950.379,
"heapDeltaBytes": -13037868,
"heapUsedBytes": 51330048,
"domNodes": -281,
"jsHeapTotalBytes": 24088576,
"scriptDurationMs": 122.911,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1881.4709999999195,
"styleRecalcs": 77,
"styleRecalcDurationMs": 40.413,
"layouts": 12,
"layoutDurationMs": 3.597,
"taskDurationMs": 919.265,
"heapDeltaBytes": -18579312,
"heapUsedBytes": 46138144,
"domNodes": 5,
"jsHeapTotalBytes": 23302144,
"scriptDurationMs": 116.235,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1729.379999999992,
"styleRecalcs": 31,
"styleRecalcDurationMs": 17.395,
"layouts": 6,
"layoutDurationMs": 0.5870000000000001,
"taskDurationMs": 417.133,
"heapDeltaBytes": 8765540,
"heapUsedBytes": 73363396,
"domNodes": 77,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 24.573000000000004,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1755.359999999996,
"styleRecalcs": 32,
"styleRecalcDurationMs": 19.577000000000005,
"layouts": 6,
"layoutDurationMs": 0.6869999999999999,
"taskDurationMs": 447.399,
"heapDeltaBytes": 8820260,
"heapUsedBytes": 73239240,
"domNodes": 78,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 25.921,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 657.9360000000065,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.987000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 424.962,
"heapDeltaBytes": -11367752,
"heapUsedBytes": 53193548,
"domNodes": 18,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 61.959999999999994,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 624.3009999999458,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.777,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 407.49099999999993,
"heapDeltaBytes": -11150680,
"heapUsedBytes": 53238360,
"domNodes": 18,
"jsHeapTotalBytes": 25427968,
"scriptDurationMs": 64.813,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-idle",
"durationMs": 2052.558000000033,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.608,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 763.6959999999999,
"heapDeltaBytes": 7316644,
"heapUsedBytes": 67343184,
"domNodes": -281,
"jsHeapTotalBytes": 2981888,
"scriptDurationMs": 112.626,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2065.7209999999395,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.435000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 797.6020000000001,
"heapDeltaBytes": 7633932,
"heapUsedBytes": 67300396,
"domNodes": -281,
"jsHeapTotalBytes": 2195456,
"scriptDurationMs": 124.03999999999998,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2187.7700000000004,
"styleRecalcs": 69,
"styleRecalcDurationMs": 15.651999999999996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1324.7050000000002,
"heapDeltaBytes": 4825788,
"heapUsedBytes": 65766652,
"domNodes": -283,
"jsHeapTotalBytes": 3436544,
"scriptDurationMs": 412.136,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2151.0079999999334,
"styleRecalcs": 68,
"styleRecalcDurationMs": 15.313999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1308.702,
"heapDeltaBytes": 1508464,
"heapUsedBytes": 62651508,
"domNodes": -283,
"jsHeapTotalBytes": 4485120,
"scriptDurationMs": 424.60900000000004,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3247.544000000005,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.468,
"layouts": 60,
"layoutDurationMs": 8.283999999999999,
"taskDurationMs": 1609.543,
"heapDeltaBytes": 795368,
"heapUsedBytes": 62994728,
"domNodes": -288,
"jsHeapTotalBytes": 6127616,
"scriptDurationMs": 536.356,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3298.2170000000224,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.861,
"layouts": 60,
"layoutDurationMs": 8.424999999999999,
"taskDurationMs": 1621.0640000000003,
"heapDeltaBytes": 1394768,
"heapUsedBytes": 64498496,
"domNodes": -290,
"jsHeapTotalBytes": 6127616,
"scriptDurationMs": 556.9960000000001,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "minimap-idle",
"durationMs": 2031.6170000000398,
"styleRecalcs": 6,
"styleRecalcDurationMs": 5.516,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 747.7570000000001,
"heapDeltaBytes": 5283108,
"heapUsedBytes": 66631448,
"domNodes": -283,
"jsHeapTotalBytes": 3768320,
"scriptDurationMs": 114.199,
"eventListeners": -181,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "minimap-idle",
"durationMs": 2034.3930000000228,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.422999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 703.1930000000001,
"heapDeltaBytes": 7072412,
"heapUsedBytes": 67575580,
"domNodes": -281,
"jsHeapTotalBytes": 3506176,
"scriptDurationMs": 104.552,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 620.2299999999923,
"styleRecalcs": 47,
"styleRecalcDurationMs": 10.774000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 415.845,
"heapDeltaBytes": -10682824,
"heapUsedBytes": 53836184,
"domNodes": 20,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 125.79499999999999,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 577.6779999999917,
"styleRecalcs": 47,
"styleRecalcDurationMs": 10.318,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 409.2,
"heapDeltaBytes": -10711668,
"heapUsedBytes": 53826100,
"domNodes": 20,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 120.6,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2010.183999999981,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.156,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 528.9849999999999,
"heapDeltaBytes": 5851416,
"heapUsedBytes": 70568204,
"domNodes": 18,
"jsHeapTotalBytes": 24903680,
"scriptDurationMs": 20.741,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2009.27200000001,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.627,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 501.239,
"heapDeltaBytes": 5938272,
"heapUsedBytes": 70546756,
"domNodes": 20,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 17.619,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1827.1819999999934,
"styleRecalcs": 79,
"styleRecalcDurationMs": 38.637,
"layouts": 16,
"layoutDurationMs": 4.656000000000001,
"taskDurationMs": 911.8779999999999,
"heapDeltaBytes": -18236140,
"heapUsedBytes": 46549524,
"domNodes": -280,
"jsHeapTotalBytes": 23040000,
"scriptDurationMs": 103.80799999999999,
"eventListeners": -151,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1705.2479999999832,
"styleRecalcs": 78,
"styleRecalcDurationMs": 38.812,
"layouts": 16,
"layoutDurationMs": 4.513999999999999,
"taskDurationMs": 777.138,
"heapDeltaBytes": -3796372,
"heapUsedBytes": 60769764,
"domNodes": 67,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 94.32099999999998,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 1384.016000000031,
"styleRecalcs": 19,
"styleRecalcDurationMs": 28.817999999999998,
"layouts": 15,
"layoutDurationMs": 11.336000000000002,
"taskDurationMs": 967.8690000000001,
"heapDeltaBytes": 3757508,
"heapUsedBytes": 74667352,
"domNodes": 13673,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 31.67299999999999,
"eventListeners": 2375,
"totalBlockingTimeMs": 128,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8356.683000000032,
"styleRecalcs": 251,
"styleRecalcDurationMs": 43.446,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4689.469,
"heapDeltaBytes": 10897400,
"heapUsedBytes": 70431088,
"domNodes": -281,
"jsHeapTotalBytes": 5795840,
"scriptDurationMs": 1386.832,
"eventListeners": -163,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 8319.928000000004,
"styleRecalcs": 251,
"styleRecalcDurationMs": 43.037,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4488.556,
"heapDeltaBytes": 8847864,
"heapUsedBytes": 67799208,
"domNodes": -279,
"jsHeapTotalBytes": 4747264,
"scriptDurationMs": 1331.479,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 18054.281000000003,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 18011.501,
"heapDeltaBytes": -26823000,
"heapUsedBytes": 183799680,
"domNodes": -8312,
"jsHeapTotalBytes": -9904128,
"scriptDurationMs": 579.909,
"eventListeners": -16385,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.333333333333332,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 18043.137000000002,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 18005.619,
"heapDeltaBytes": -61968124,
"heapUsedBytes": 167012320,
"domNodes": -8316,
"jsHeapTotalBytes": -7282688,
"scriptDurationMs": 576.138,
"eventListeners": -16387,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.77333333333336,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 22204.08500000002,
"styleRecalcs": 154,
"styleRecalcDurationMs": 22.48500000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 22165.706,
"heapDeltaBytes": -31383940,
"heapUsedBytes": 166523336,
"domNodes": -8312,
"jsHeapTotalBytes": -9117696,
"scriptDurationMs": 922.5669999999999,
"eventListeners": -16379,
"totalBlockingTimeMs": 471,
"frameDurationMs": 18.333333333333332,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 23399.963999999956,
"styleRecalcs": 172,
"styleRecalcDurationMs": 26.156000000000013,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 23343.738,
"heapDeltaBytes": -59949548,
"heapUsedBytes": 166597676,
"domNodes": -8312,
"jsHeapTotalBytes": -10166272,
"scriptDurationMs": 925.475,
"eventListeners": -16379,
"totalBlockingTimeMs": 638,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "workflow-execution",
"durationMs": 456.07100000000855,
"styleRecalcs": 16,
"styleRecalcDurationMs": 23.791,
"layouts": 4,
"layoutDurationMs": 1.449,
"taskDurationMs": 124.14200000000001,
"heapDeltaBytes": 5190236,
"heapUsedBytes": 68690984,
"domNodes": 128,
"jsHeapTotalBytes": 5242880,
"scriptDurationMs": 11.060999999999998,
"eventListeners": 99,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "workflow-execution",
"durationMs": 467.79400000002624,
"styleRecalcs": 11,
"styleRecalcDurationMs": 18.449,
"layouts": 3,
"layoutDurationMs": 0.6509999999999999,
"taskDurationMs": 105.73,
"heapDeltaBytes": 5047756,
"heapUsedBytes": 68589464,
"domNodes": 119,
"jsHeapTotalBytes": 5242880,
"scriptDurationMs": 9.052999999999999,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
}
]
} |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded-node error scans now track pending asynchronous verification per graph and execution ID. Scan cancellation suppresses stale results after node removal, graph changes, or hook disposal. RightSidePanel uses pending scan state when validating the Errors tab. ChangesExecution-error scan lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR improves retention and fallback of the Errors tab during deferred node checks, but two regression tests may not reliably exercise the abort and pending-scan states they are intended to protect. The change is mergeable with explicit owner awareness and follow-up to strengthen those assertions. Sequence Diagram(s)sequenceDiagram
participant Graph
participant useErrorClearingHooks
participant executionErrorStore
participant RightSidePanel
Graph->>useErrorClearingHooks: add node
useErrorClearingHooks->>executionErrorStore: begin pending scan
useErrorClearingHooks-->>executionErrorStore: finish scan after verification
RightSidePanel->>executionErrorStore: query selected-node scan state
executionErrorStore-->>RightSidePanel: return pending status
RightSidePanel-->>Graph: retain or fallback active tab
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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/components/rightSidePanel/RightSidePanel.vue`:
- Around line 235-241: Add focused regression tests for the RightSidePanel
watchEffect behavior: verify an active errors tab restored before the queued
microtask remains selected, an unavailable active tab selects tabs.value[0], and
unmounting before the microtask prevents rightSidePanelStore.openPanel from
being called.
- Around line 235-241: Update the watchEffect around isActiveTabAvailable to
accept onCleanup and invalidate the queued microtask during cleanup, preventing
rightSidePanelStore.openPanel from running after unmount or a watcher rerun. Add
focused tests covering both cleanup paths: component unmount and watcher rerun
before the microtask executes.
🪄 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: 635bc502-d036-40a2-ab60-0839abd458dd
📒 Files selected for processing (1)
src/components/rightSidePanel/RightSidePanel.vue
Comment on lines
+235
to
+241
| watchEffect(() => { | ||
| if ( | ||
| !tabs.value.some((tab) => tab.value === activeTab.value) && | ||
| !(activeTab.value === 'subgraph' && isSingleSubgraphNode.value) | ||
| ) { | ||
| rightSidePanelStore.openPanel(tabs.value[0].value) | ||
| } | ||
| if (isActiveTabAvailable()) return | ||
| queueMicrotask(() => { | ||
| if (!isActiveTabAvailable()) { | ||
| rightSidePanelStore.openPanel(tabs.value[0].value) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add focused component regression tests.
Test both timing outcomes:
- Restore the active
'errors'tab before the microtask and verify that it remains active. - Keep the active tab unavailable and verify that the first available tab is selected.
- Verify that unmounting before the microtask does not call
openPanel.
As per coding guidelines, tests are required for Vue code changes, especially bug fixes.
🤖 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/components/rightSidePanel/RightSidePanel.vue` around lines 235 - 241, Add
focused regression tests for the RightSidePanel watchEffect behavior: verify an
active errors tab restored before the queued microtask remains selected, an
unavailable active tab selects tabs.value[0], and unmounting before the
microtask prevents rightSidePanelStore.openPanel from being called.
Source: Coding guidelines
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 --glob '*.{ts,tsx,vue}' \
'queueMicrotask|watchEffect|onCleanup|onScopeDispose|onBeforeUnmount|closePanel|unmount' \
srcRepository: Comfy-Org/ComfyUI_frontend
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RightSidePanel.vue ---'
sed -n '1,280p' src/components/rightSidePanel/RightSidePanel.vue
printf '%s\n' '--- Related tests and references ---'
rg -n --glob '*.{ts,tsx,vue}' \
'RightSidePanel|isActiveTabAvailable|openPanel\(tabs|activeTab|rightSidePanelStore' \
src tests 2>/dev/null | head -n 300
printf '%s\n' '--- Guidance ---'
sed -n '1,220p' docs/guidance/vue-components.md
sed -n '1,160p' docs/guidance/typescript.mdRepository: Comfy-Org/ComfyUI_frontend
Length of output: 34334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Store implementation ---'
cat -n src/stores/workspace/rightSidePanelStore.ts
printf '%s\n' '--- Component lifecycle and template ---'
sed -n '280,430p' src/components/rightSidePanel/RightSidePanel.vue
printf '%s\n' '--- Right-side-panel files and tests ---'
git ls-files '*RightSidePanel*' '*rightSidePanel*' | sort
printf '%s\n' '--- Exact cleanup/deferred-work patterns ---'
rg -n -C 6 --glob '*.{ts,tsx,vue}' \
'watchEffect\s*\(\s*\(onCleanup|queueMicrotask\s*\(' \
src/components src/composables src/stores | head -n 240
printf '%s\n' '--- Repository status and relevant diff summary ---'
git status --short
git diff --stat -- src/components/rightSidePanel/RightSidePanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 21380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RightSidePanel mount condition ---'
rg -n -C 12 'NodePropertiesPanel|RightSidePanel' src/components/graph/GraphCanvas.vue src --glob '*.vue' | head -n 220
printf '%s\n' '--- Existing browser coverage ---'
sed -n '1,260p' browser_tests/tests/rightSidePanelTabs.spec.ts
printf '%s\n' '--- All active-tab writes ---'
rg -n -C 5 --glob '*.{ts,tsx,vue}' \
'activeTab(?:\.value)?\s*=|openPanel\(' \
src/stores/workspace/rightSidePanelStore.ts \
src/components/rightSidePanel \
src/composables/graph \
src/services/litegraphService.ts \
src/composables/useCoreCommands.ts | head -n 320Repository: Comfy-Org/ComfyUI_frontend
Length of output: 36596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const events = []
function scheduleFallback(withCleanup, cleanupBeforeRun) {
let cancelled = false
const cleanup = () => {
cancelled = true
}
queueMicrotask(() => {
if (!cancelled) events.push(withCleanup ? 'open-with-cleanup' : 'open')
})
if (cleanupBeforeRun) cleanup()
}
scheduleFallback(false, true)
scheduleFallback(true, true)
scheduleFallback(true, false)
queueMicrotask(() => {
console.log(JSON.stringify(events))
})
JSRepository: Comfy-Org/ComfyUI_frontend
Length of output: 186
Cancel the deferred fallback during watcher cleanup.
If the component unmounts or the watcher reruns before the microtask runs, the callback can still call rightSidePanelStore.openPanel(...). Pass onCleanup to watchEffect, invalidate the callback during cleanup, and add focused tests for both paths.
🤖 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/components/rightSidePanel/RightSidePanel.vue` around lines 235 - 241,
Update the watchEffect around isActiveTabAvailable to accept onCleanup and
invalidate the queued microtask during cleanup, preventing
rightSidePanelStore.openPanel from running after unmount or a watcher rerun. Add
focused tests covering both cleanup paths: component unmount and watcher rerun
before the microtask executes.
Source: Path instructions
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #15012 +/- ##
==========================================
+ Coverage 81.11% 81.15% +0.04%
==========================================
Files 1882 1882
Lines 108707 107519 -1188
Branches 34153 33205 -948
==========================================
- Hits 88180 87262 -918
+ Misses 20174 19873 -301
- Partials 353 384 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 69 files with indirect coverage changes 🚀 New features to boost your workflow:
|
4 tasks
christian-byrne
previously approved these changes
Aug 11, 2026
Contributor
Author
|
One microtask does not cover all deferred error scans. The fallback in Consequently, pasting a node with missing media—or a cloud-verified missing model—can still switch away from Errors before the error is registered, reproducing the behavior this PR aims to fix. The fallback should coordinate with scan completion rather than assume one microtask is sufficient. A timing-focused regression test should cover this. |
DrJKL
commented
Aug 11, 2026
dosubot
Bot
added
size:L
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/rightSidePanel/RightSidePanel.test.ts`:
- Around line 118-170: Add a Playwright regression test under browser_tests/
using the documented workflow that performs node addition, selection changes,
deferred model and media verification, and tab fallback in one real browser
flow. Assert that the Errors tab remains active while verification is pending
and remains active when verification surfaces an error, then validate the
expected fallback behavior after the scan settles without errors.
- Around line 125-129: Update the RightSidePanel test to use Vue’s nextTick()
instead of Promise.resolve() when waiting for reactivity at
src/components/rightSidePanel/RightSidePanel.test.ts:125-129. In the
pending-scan unmount case at
src/components/rightSidePanel/RightSidePanel.test.ts:161-169, call unmount()
before finishScan() so completion occurs after unmount.
🪄 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: a1a04273-338a-45d2-98cd-8dc46e1f0132
📒 Files selected for processing (6)
src/components/rightSidePanel/RightSidePanel.test.tssrc/components/rightSidePanel/RightSidePanel.vuesrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/stores/executionErrorStore.test.tssrc/stores/executionErrorStore.ts
jaeone94
reviewed
Aug 11, 2026
jaeone94
left a comment
jaeone94
left a comment
Collaborator
There was a problem hiding this comment.
The core mechanism looks solid — I traced the begin/finish lifecycle across settle, node-removal, and dispose paths and found no counter leak, and the synchronous beginAddedNodeErrorScan inside onNodeAdded genuinely closes the flush-vs-scan race described in the PR body. Store placement and the minimal begin/hasPending surface read well, and the component tests are mutation-sensitive in both directions (reverting the tabs condition fails tests 1–2; over-broadening it fails test 3).
A few things before merge, one of which I think is a real regression:
- Issue: dispose-time
cancel()aborts in-flight verifications on every subgraph enter/exit, which can permanently drop missing-model errors within the same workflow (inline comment with repro). - Issue:
runAddedNodeScan(...).finally(finish)has no.catch, so a sync throw in a scan stage becomes an unhandled rejection and releases the pending flag while stage-1 verifications are still in flight. - Issue: the new
onNodeRemovedcancellation line has no test — deleting it fails nothing, and its failure mode is exactly this PR's bug class. - Issue: the unmount test can't fail as ordered.
- Smaller suggestions inline (comment relocation, reactivity guard hardening, i18n messages, coverage gaps).
Also — the PR body still describes the abandoned "re-check one microtask later" approach. What shipped is a ref-counted pending-scan registry that holds the Errors tab for the whole scan lifetime including network verification, which is the better design, but worth updating the description (and noting the visible UX change: an empty Errors tab during verification instead of the tab vanishing). Known-gap on e2e is acknowledged in the body; a tracked follow-up issue link would be good.
| if (disposed) return | ||
| disposed = true | ||
| for (const cancellations of pendingScanCancellations.values()) { | ||
| for (const cancel of cancellations) cancel() |
Collaborator
There was a problem hiding this comment.
Issue: This runs on every canvasStore.currentGraph change — GraphCanvas.vue:266-273 disposes and reinstalls the hooks on subgraph enter/exit, not just on workflow teardown. Since cancel() aborts the verification (not just releases the counter), entering a subgraph while a pasted node's cloud verification is in flight now drops the result at the signal?.aborted check in verifyAndAddPendingModels, and the missing-model error never surfaces. Pre-PR these verifications were only guarded by app.rootGraph !== rootGraphAtScan, which subgraph navigation doesn't change, so the error still appeared (legacy canvas mode has no replay to recover it; Vue-nodes mode happens to recover via useGraphNodeManager).
Would it make sense for dispose to release the pending flags only (so the tab hold ends) and keep the abort for onNodeRemoved, where the result is genuinely irrelevant? Note "does not surface async verification after hooks are disposed" currently pins the abort-on-dispose behavior, so it would need to flip. WDYT?
Contributor
Author
There was a problem hiding this comment.
Thank you enormously for tracing this through GraphCanvas and catching the subgraph-navigation regression. This was an exceptionally valuable review point. 4873e0d482 now separates releasing the pending flag from aborting verification: hook disposal releases only, while actual node removal aborts. It also guards workflow/node replacement so late results cannot leak across workflows.
| } | ||
|
|
||
| cancellationsForNode.add(cancel) | ||
| void runAddedNodeScan(node, abortController.signal).finally(finish) |
Collaborator
There was a problem hiding this comment.
Issue: A sync throw inside a scan stage (e.g. scanNodeModelCandidates on a malformed node) passes through .finally and the void leaves it unhandled — and finish() then releases the pending flag while stage-1 verifications are still outstanding.
Suggested change
| void runAddedNodeScan(node, abortController.signal).finally(finish) | |
| void runAddedNodeScan(node, abortController.signal) | |
| .catch((error: unknown) => { | |
| console.warn('[useErrorClearingHooks] added-node scan failed:', error) | |
| }) | |
| .finally(finish) |
Contributor
Author
There was a problem hiding this comment.
Deeply grateful for this catch—the distinction between handling the rejection and preserving the lifetime of already-started verification was crucial. 4873e0d482 catches scan-stage failures and uses finally inside the scan to await all started verifications before releasing the pending flag.
| originalOnNodeRemoved?.call(this, node) | ||
| return | ||
| } | ||
| for (const cancel of pendingScanCancellations.get(node) ?? []) cancel() |
Collaborator
There was a problem hiding this comment.
Issue: This line has no coverage — deleting it fails no test (the dispose tests cancel via cleanup(), and the existing removal-mid-verification test covers the mode-change path). Its failure mode is this PR's own bug class: a deleted node's in-flight verification holds the counter and keeps the Errors tab pinned. Suggest a test: add a node, wait for verifyAssetSupportedCandidates to be called, graph.remove(node), assert pending releases immediately and nothing surfaces after resolving the verification.
Contributor
Author
There was a problem hiding this comment.
Thank you very much for identifying the mutation hole so precisely. 4873e0d482 adds direct node-removal tests for both model and media verification, asserting immediate pending-flag release, signal abortion, and suppression of late results.
| scanSingleNodeModelsAndTypes(target, pendingVerifications, signal) | ||
| ) | ||
|
|
||
| await Promise.resolve() |
Collaborator
There was a problem hiding this comment.
Suggestion: The rewrite dropped the comment explaining why media waits a second microtask (paste/drop upload handlers set node.isUploading synchronously before their first await). Two bare await Promise.resolve() lines are exactly what a future simplification collapses into one, silently resurrecting the upload race. Suggest moving the old rationale here, and trimming the now-stale caller comment at 515-521 that still narrates the deferral logic that no longer lives there.
Contributor
Author
There was a problem hiding this comment.
Thank you for protecting this subtle timing invariant. The rationale now sits directly beside the second microtask, and the stale caller-side narration was removed in 4873e0d482.
| // transient upload state before media detection reads the widget value. | ||
| if (!ChangeTracker.isLoadingGraph) { | ||
| scheduleAddedNodeScan(node) | ||
| scheduleAddedNodeScan(node, pendingScanCancellations) |
Collaborator
There was a problem hiding this comment.
Nit: !ChangeTracker.isLoadingGraph is re-checked synchronously first thing inside scheduleAddedNodeScan (line 367), so one of the two is dead. I'd drop this caller-side guard and let scheduleAddedNodeScan own its preconditions.
Contributor
Author
There was a problem hiding this comment.
Thank you for the careful cleanup note. The duplicate caller guard is gone; scheduleAddedNodeScan now owns that precondition.
| expect(openPanel).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not update the panel after unmount', async () => { |
Collaborator
There was a problem hiding this comment.
Issue: This test can't fail — finishScan() fires and flushes before unmount(), and after mockClear() nothing in the system can still call openPanel, for any implementation. Reordering (unmount first, then finishScan()) makes it meaningful, though at that point it mostly asserts Vue's own effect-scope teardown — deleting it may be the honest fix.
Contributor
Author
There was a problem hiding this comment.
Thank you—this was exactly right, and it prevented a false-confidence test from surviving. The test now unmounts first, finishes the scan afterward, and waits for Vue reactivity before asserting.
| ) | ||
| const openPanel = vi.spyOn(rightSidePanelStore, 'openPanel') | ||
|
|
||
| const i18n = createI18n({ |
Collaborator
There was a problem hiding this comment.
Suggestion: Other component tests import the real messages (import enMessages from '@/locales/en/main.json' with { type: 'json' }) rather than a hand-picked subset — the subset rots silently into vue-i18n fallback warnings when a key is renamed.
Contributor
Author
There was a problem hiding this comment.
Thank you for calling this out. The test now imports the real English messages rather than maintaining a brittle subset.
| expect(openPanel).not.toHaveBeenCalled() | ||
|
|
||
| finishScan() | ||
| await Promise.resolve() |
Collaborator
There was a problem hiding this comment.
Nit: Tests 1 and 4 use await Promise.resolve() while test 2 uses await nextTick(). The Promise.resolve() variant only works because Vue's scheduler queued its flush first; docs/guidance/vitest.md says to wait for reactivity with nextTick() — and for the not.toHaveBeenCalled() assertion an under-flushed await would pass vacuously.
Contributor
Author
There was a problem hiding this comment.
Thank you for the precise scheduler analysis. The component tests now use nextTick(). The scan-settlement test additionally blocks queueMicrotask deliberately so it remains mutation-sensitive and fails against the old deferred fallback implementation.
| expect(mediaScan).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it('keeps an added-node scan pending until async verification settles', async () => { |
Collaborator
There was a problem hiding this comment.
Suggestion (coverage): The new abort tests only exercise the model path — scanNodeMediaCandidates is mocked to [], so removing signal?.aborted || from verifyAndAddPendingMedia fails nothing. The existing deferMediaVerification fixture makes this cheap to add. Also worth one subgraph-interior-node case: both sides of the pending-scan seam compute the ID via getExecutionIdByNode, but tests only ever check root-level IDs where the two trivially coincide.
Nit: these two async tests duplicate ~25 lines of candidate/spy/deferred-resolve scaffolding — the file already has the deferMediaVerification fixture pattern to mirror.
Contributor
Author
There was a problem hiding this comment.
Extremely grateful for this coverage review. 4873e0d482 adds direct media cancellation/late-result coverage, the analogous model case, and a subgraph-interior execution-ID component case. The existing media deferral fixture is reused.
| rootGraph: LGraph, | ||
| executionId: NodeExecutionId | ||
| ): boolean { | ||
| void pendingAddedNodeScanRevision.value |
Collaborator
There was a problem hiding this comment.
Nit: void pendingAddedNodeScanRevision.value is a correct dependency-touch over the non-reactive WeakMap, but it reads as dead code to a future editor (or lint autofix). A one-line comment stating it's the reactivity subscription point would protect it.
Contributor
Author
There was a problem hiding this comment.
Thank you for anticipating the maintenance hazard. A one-line comment now identifies the revision read as the reactivity subscription point.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@browser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.ts`:
- Around line 198-253: Add a sibling test next to “keeps Errors active through
pasted-node verification and falls back when resolved” that uses a pasted node
with a missing media reference, gates the media verification request, and
verifies the Errors tab remains selected while verification is pending and after
it settles. Then resolve the media reference, trigger the appropriate refresh,
and assert the Errors tab hides with Parameters selected.
In `@src/components/rightSidePanel/RightSidePanel.test.ts`:
- Around line 130-132: Remove the unnecessary globalThis.queueMicrotask spy and
mock from the scan-completion test around finishScan and nextTick, leaving the
existing nextTick watcher flush and beforeEach mock cleanup unchanged.
🪄 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: 93156c02-9335-479d-9159-afb4e85e82f8
📒 Files selected for processing (6)
browser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.tssrc/components/rightSidePanel/RightSidePanel.test.tssrc/components/rightSidePanel/RightSidePanel.vuesrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/stores/executionErrorStore.ts
Contributor
Author
|
@jaeone94 Thank you immensely for the exceptionally thorough and thoughtful review. The lifecycle tracing, concrete repros, mutation checks, and attention to the smaller maintainability details were extraordinarily helpful—especially the subgraph-disposal regression, which would have been easy to miss and consequential in legacy canvas mode. Every inline point was addressed in |
This was referenced Aug 12, 2026
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@browser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.ts`:
- Around line 230-232: Update the test around verificationStarted so it first
waits for the pasted node to exist, using the pasted-node locator or assertion
already defined in the test, and only then asserts errorsTab has
aria-selected="true". Keep the existing verificationStarted synchronization, but
ensure the pending-state assertion is tied to the added node’s verification
rather than any unrelated asset request.
In `@src/composables/graph/useErrorClearingHooks.test.ts`:
- Around line 533-553: Extract the repeated model verification setup into a
shared model-side deferral fixture, parallel to deferMediaVerification in
promotedMedia.ts. Update the affected tests to use this fixture for the
MissingModelCandidate, scanNodeModelCandidates, scanNodeMediaCandidates, and
deferred verifyAssetSupportedCandidates behavior, centralizing the verification
signature and preserving each test’s ability to resolve verification and mark
the candidate missing.
- Around line 869-871: Update the verification test around resolveVerification
to await the observable settle marker used by the corresponding media test
before asserting missingModelCandidates is null; remove the single
Promise.resolve flush and preserve the assertion that no late result is stored.
🪄 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: 19f6d6d2-9bb0-469a-bbbf-d5cb68921b1e
📒 Files selected for processing (7)
browser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.tssrc/components/rightSidePanel/RightSidePanel.test.tssrc/components/rightSidePanel/RightSidePanel.vuesrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/stores/executionErrorStore.test.tssrc/stores/executionErrorStore.ts
Comment on lines
+230
to
+232
| await verificationStarted | ||
|
|
||
| await expect(panel.errorsTab).toHaveAttribute('aria-selected', 'true') |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tie the pending-state assertion to the pasted node.
verificationStarted resolves on the first /api/assets request that the route intercepts. Any asset request can resolve it, not only the added-node verification. If an unrelated request arrives first, Line 232 can assert aria-selected before the pasted-node scan starts, so the test passes without exercising the pending-scan hold.
Wait for the pasted node to exist before asserting the tab state.
💚 Proposed fix
await comfyPage.clipboard.paste()
+ await expect.poll(() => comfyPage.nodeOps.getNodeCount()).toBe(2)
await verificationStarted
await expect(panel.errorsTab).toHaveAttribute('aria-selected', 'true')
releaseVerification()
- await expect.poll(() => comfyPage.nodeOps.getNodeCount()).toBe(2)As per path instructions, browser_tests/README.md is the canonical guide for browser tests, including flake prevention.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await verificationStarted | |
| await expect(panel.errorsTab).toHaveAttribute('aria-selected', 'true') | |
| await comfyPage.clipboard.paste() | |
| await expect.poll(() => comfyPage.nodeOps.getNodeCount()).toBe(2) | |
| await verificationStarted | |
| await expect(panel.errorsTab).toHaveAttribute('aria-selected', 'true') | |
| releaseVerification() |
🤖 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 `@browser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.ts`
around lines 230 - 232, Update the test around verificationStarted so it first
waits for the pasted node to exist, using the pasted-node locator or assertion
already defined in the test, and only then asserts errorsTab has
aria-selected="true". Keep the existing verificationStarted synchronization, but
ensure the pending-state assertion is tied to the added node’s verification
rather than any unrelated asset request.
Source: Path instructions
Comment on lines
+533
to
+553
| const candidate = fromAny<MissingModelCandidate, unknown>({ | ||
| nodeId: '1', | ||
| nodeType: 'CheckpointLoaderSimple', | ||
| widgetName: 'ckpt_name', | ||
| name: 'pending.safetensors', | ||
| isMissing: undefined | ||
| }) | ||
| vi.spyOn(missingModelScan, 'scanNodeModelCandidates').mockReturnValue([ | ||
| candidate | ||
| ]) | ||
| vi.spyOn(missingMediaScan, 'scanNodeMediaCandidates').mockReturnValue([]) | ||
| let resolveVerification: () => void = () => undefined | ||
| vi.spyOn( | ||
| missingModelScan, | ||
| 'verifyAssetSupportedCandidates' | ||
| ).mockImplementation(async () => { | ||
| await new Promise<void>((resolve) => { | ||
| resolveVerification = resolve | ||
| }) | ||
| candidate.isMissing = true | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract a model-verification deferral fixture.
Five new tests repeat the same candidate literal, scanNodeModelCandidates mock, scanNodeMediaCandidates mock, and deferred verifyAssetSupportedCandidates implementation. The repository already provides deferMediaVerification in src/platform/missingMedia/__fixtures__/promotedMedia.ts. Add the model-side equivalent and reuse it, so future changes to the verification signature touch one place.
🤖 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/composables/graph/useErrorClearingHooks.test.ts` around lines 533 - 553,
Extract the repeated model verification setup into a shared model-side deferral
fixture, parallel to deferMediaVerification in promotedMedia.ts. Update the
affected tests to use this fixture for the MissingModelCandidate,
scanNodeModelCandidates, scanNodeMediaCandidates, and deferred
verifyAssetSupportedCandidates behavior, centralizing the verification signature
and preserving each test’s ability to resolve verification and mark the
candidate missing.
Contributor
Author
There was a problem hiding this comment.
Open a tracking issue for this.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="Comfy-Org/ComfyUI_frontend"
body=$(cat <<'EOF'
## Summary
Extract a shared deferred model-verification fixture for
`useErrorClearingHooks` tests.
## Required changes
Add a model-side fixture that is parallel to
`deferMediaVerification` in
`src/platform/missingMedia/__fixtures__/promotedMedia.ts`.
Update the affected tests in
`src/composables/graph/useErrorClearingHooks.test.ts` to use the
fixture for:
- `MissingModelCandidate` setup
- `scanNodeModelCandidates` mocking
- `scanNodeMediaCandidates` mocking
- deferred `verifyAssetSupportedCandidates` behavior
The fixture must preserve each test's ability to resolve verification
and set the candidate `isMissing` state when required.
## Rationale
Several tests duplicate the same deferred model-verification setup.
A shared fixture will centralize the verification signature and reduce
maintenance work when that signature changes.
## Affected areas
- `src/composables/graph/useErrorClearingHooks.test.ts`
- Model verification test fixtures near missing-model test support
## Acceptance criteria
- A reusable deferred model-verification fixture exists.
- The repeated model-verification test setup uses the fixture.
- The updated tests retain their current behavioral coverage.
- Changes are limited to test support and affected tests.
## Backlinks
- Pull request: https://github.com/Comfy-Org/ComfyUI_frontend/pull/15012
- Review comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/15012#discussion_r3771038268
- Requested by: `@DrJKL`
EOF
)
gh issue create \
--repo "$repo" \
--title "Extract a deferred model-verification fixture for error-clearing tests" \
--body "$body" \
--assignee "DrJKL"Length of output: 4305
@DrJKL, I created and assigned tracking issue #15203:
#15203
🧬 Code Graph Analysis Results
src/composables/graph/useErrorClearingHooks.ts:550-636
installErrorClearingHooks(graph) installs node-added, node-removed, and trigger callbacks; tracks and cancels pending scans, derives removed-node execution IDs, removes related errors, restores callbacks during cleanup, and prevents disposed hooks from scheduling scans.
src/lib/litegraph/src/LGraph.ts:1051-1157
LGraph.remove(node) invokes node.onRemoved, sets node.graph = null, then invokes graph.onNodeRemoved(node). This ordering is relevant to removal-hook execution-ID derivation.
src/platform/missingModel/missingModelStore.ts:21-180
Defines missing-model candidate state and methods for setting candidates, removing candidates by node ID or execution-ID prefix, and clearing associated interaction state.
src/platform/missingMedia/missingMediaStore.ts:18-175
Defines missing-media candidate state and methods for setting, adding, and removing candidates by widget, node ID, or execution-ID prefix; verification cancellation is handled by clearMissingMedia().
src/platform/nodeReplacement/missingNodesErrorStore.ts:19-154
Defines missing-node error state and methods for surfacing and removing missing-node entries by node ID or execution-ID prefix; string-based group entries are preserved during prefix removal.
src/types/nodeIdentification.ts:139-148
createNodeExecutionId(nodeIds) validates and joins serialized node-ID segments with :; returns null for an empty or invalid path.
src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.ts:268-288
createTestSubgraphNode(subgraph, options) creates a subgraph container with an optional parent graph and explicit node ID, used by tests involving nested execution-ID paths.
src/platform/missingMedia/__fixtures__/promotedMedia.ts:78-92
deferMediaVerification() returns a verification spy and resolver. Verification remains pending until the resolver is called, then marks all supplied media candidates as missing.
src/platform/missingMedia/__fixtures__/promotedMedia.ts:153-245
createPromotedMediaRuntime(options) builds root, nested subgraph, promoted host, and source-node relationships with configurable depth, IDs, fanout, widget values, and options. It returns graphs, hosts, source nodes, and intermediate hosts for promotion lifecycle tests.
You are interacting with an AI system.
Comment on lines
+869
to
+871
| resolveVerification() | ||
| await Promise.resolve() | ||
| expect(useMissingModelStore().missingModelCandidates).toBeNull() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wait for the verification to settle before asserting no late result.
await Promise.resolve() flushes one microtask. The mocked verifyAssetSupportedCandidates continuation and the post-verification filter need more than one microtask, so expect(...).toBeNull() can pass before any late result could be stored. The assertion then passes vacuously, including against an implementation that ignores the abort signal.
Mirror the media test at Lines 900-902, which awaits an observable settle marker first.
💚 Proposed fix
resolveVerification()
- await Promise.resolve()
+ await vi.waitFor(() => expect(verifySpy).toHaveResolved())
expect(useMissingModelStore().missingModelCandidates).toBeNull()As per path instructions, required review context states: "Avoid excessive mocks, fragile timing assertions, snapshots, and shared mutable state."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resolveVerification() | |
| await Promise.resolve() | |
| expect(useMissingModelStore().missingModelCandidates).toBeNull() | |
| resolveVerification() | |
| await vi.waitFor(() => expect(verifySpy).toHaveResolved()) | |
| expect(useMissingModelStore().missingModelCandidates).toBeNull() |
🤖 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/composables/graph/useErrorClearingHooks.test.ts` around lines 869 - 871,
Update the verification test around resolveVerification to await the observable
settle marker used by the corresponding media test before asserting
missingModelCandidates is null; remove the single Promise.resolve flush and
preserve the assertion that no late result is stored.
Source: Path instructions
github-actions
Bot
added
the
risk:R2
jaeone94
approved these changes
Aug 13, 2026
jaeone94
left a comment
jaeone94
left a comment
Collaborator
There was a problem hiding this comment.
All findings resolved; nothing further blocking from my side. LGTM.
pull Bot
pushed a commit
to Penguinjanator/ComfyUI_frontend
that referenced
this pull request
Aug 17, 2026
…14668) ## Summary Follow-up to [Comfy-Org#14578](Comfy-Org#14578): the comment/naming cleanup requested during its review, plus what remains of DrJKL's post-merge findings after [Comfy-Org#14948](Comfy-Org#14948) landed the stale-value guard. Rebased onto current `main`. Contained to files Comfy-Org#14578 already touched — no new modules, exports, or store surfaces. ## Changes **Reconciliation during multi-node deletion (`useErrorClearingHooks.ts`)** — deleting N nodes ran the global candidate walk N times. Targeted removal for the deleted node stays synchronous; only the global pass is coalesced into one microtask per burst, and it drops if the hooks were torn down meanwhile. The mode-change path keeps its synchronous semantics. The scheduler lives in the `installErrorClearingHooks` closure rather than at module scope, so it follows the per-installation `disposed` lifecycle Comfy-Org#15012 introduced. Two promotion lifecycle tests seeded their stale candidate after the removal and relied on the synchronous pass having already gone by; they now let the removal's own reconcile land first. **Un-bypass transition test** — hand-rolled the mode event while leaving the node in its default mode, so it never established the BYPASS state its transition claimed. Driven through `setNodeMode`, which already sits in that file. **Workflow-load coverage for the stale-value guard** — Comfy-Org#14948 pinned the guard through the realtime path and through the predicate itself. The pipeline also writes confirmed candidates into the workflow's `pendingWarnings`, which `workflowService` restores without re-validating, so a stale candidate surviving there outlives the session. Asserting the cache needs a workflow stub, which is why the `workspaceStore` mock stops returning a null `activeWorkflow`. **Comment and naming cleanup** — comments say what a thing *is* rather than narrating the situation around it: the reconciler's module doc keeps its first line and drops the essay; the notes above the `dropOutOfScope` guard, the scan's ordering, `isEditableValueOwner`, and `rootGraph` are gone because the code and the names already carry them. The unparseable `missingMediaStore.test.ts` comment is deleted. `findNodesReferencingValues` no longer explains a caller's behaviour. Settles on `drop` for the operation spelled both `prune` and `drop`. Aliases `comfyPageFixture` to `test` in `errorsTabMissingMediaRuntime.spec.ts`, matching the 213 other specs that do. **Breaking**: none. ## Dropped on rebase The stale-value hunk and its realtime regression test are identical to what Comfy-Org#14948 shipped, so `main` already has them. The promoted-media harness also encoded an impossible fixture state — `createPromotedMissingMediaCandidate` derives `name` from `hostWidget.value`, but the harness overrode `name` while leaving the widget elsewhere. Comfy-Org#14948 fixed that by seeding through `hostValue`, which keeps the two in sync rather than dropping the distinct name; that resolution is kept. ## Review focus Both behavioural changes carry a mutation-proved regression test on this base: reverting the coalescing makes the burst test fail (2 calls instead of 0 before the microtask), and reverting the value guard makes the workflow-load test fail. Commits are split so each is independently reviewable and independently green. Deliberately **not** included: - Moving `isEditableValueOwner` into `src/core/graph/subgraph/`, raised on Comfy-Org#14578. The point stands — nothing there combines link state, promotion and liveness into "is this widget the editable value owner", and the predicate isn't missing-media-specific. But the move creates a new shared surface in the area `feature/ecs-migration` is actively reworking, which is not worth it for a single-caller helper. Left where it is. - `WidgetSelectDropdown`'s `root`/`trigger` semantics, and the reconciler's dependency on node add/remove — both scoped out by the reviewer, the latter to be handled in his refactor. - The E2E-structure items — the conditional repair path in `enterSubgraphWithFallback`, driving promoted-widget setup through user behaviour instead of `page.evaluate`, the options argument, and the `_testNodes` mock branches. Those change how the suite is built, so they're tracked in [FE-1493](https://linear.app/comfyorg/issue/FE-1493/no-e2e-for-promotedemote-error-reconciliation-and-promoted-media). ## Test plan Static gates clean on the rebased tree (`typecheck`, `lint`, `format:check`, `knip`). Unit run scoped to the affected areas — `composables/graph`, `platform/missingMedia` — 188 tests green, and each of the four commits verified green on its own. Full suite left to CI.
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
5 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
Pasted and duplicated nodes register widgets before their deferred missing-model and missing-media scans finish. The right-side panel could therefore recalculate its tabs, see no selected-node error yet, and switch away from Errors permanently.
This change tracks graph-scoped added-node scans from node addition through asynchronous verification. An already-active Errors tab remains available while its selected node is being scanned, then either stays active when an error surfaces or falls back when the scan settles cleanly.
Scan lifecycle handling now:
Testing
pnpm test:unit src/components/rightSidePanel/RightSidePanel.test.ts src/composables/graph/useErrorClearingHooks.test.ts src/stores/executionErrorStore.test.tspnpm typecheck