fix: prevent escape from exiting subgraph while a context menu is open#12308
Draft
christian-byrne wants to merge 3 commits into
Draft
fix: prevent escape from exiting subgraph while a context menu is open#12308christian-byrne wants to merge 3 commits into
christian-byrne wants to merge 3 commits into
Conversation
Pressing Escape while a right-click context menu is open inside a subgraph used to fire the global Comfy.Graph.ExitSubgraph keybinding, exiting the subgraph while leaving the menu open. The keybinding service only suppressed Escape when a Pinia dialog was open, so other raised surfaces leaked the event to window-level handlers. Introduce a raisedSurfaceStore that tracks open popovers, context menus, and top-level modals as a single source of truth, and consult it from the keybinding service alongside the existing dialog check. The legacy LiteGraph ContextMenu now closes itself on Escape (mirroring its existing outside-pointerdown handler) and reports open/close lifecycle via document events so the store stays in sync without monkey patches. NodeContextMenu registers itself via the new useRaisedSurface composable. Fixes the bug demonstrated in the attached screencast.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a raised-surface Pinia store and composable, ContextMenu ownerDocument-aware lifecycle and Escape handling with open/close events, a LiteGraph bridge forwarding those events to the store, component wiring to register context menus, and keybinding checks to suppress Escape when any raised surface is open. ChangesRaised Surface Keybinding Suppression
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ❌ 1604 passed, 1 failed · 1 flaky❌ Failed Tests📊 Browser Reports
|
📦 Bundle: 5.36 MB gzip 🔴 +1.04 kBDetailsSummary
Category Glance App Entry Points — 26.1 kB (baseline 26.1 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.24 MB (baseline 1.24 MB) • 🔴 +872 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 82.9 kB (baseline 82.9 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 2 unchanged Panels & Settings — 527 kB (baseline 527 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 14 unchanged User & Accounts — 17.8 kB (baseline 17.8 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed / 2 unchanged Editors & Dialogs — 112 kB (baseline 112 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 4 added / 4 removed UI Components — 58 kB (baseline 58 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 8 unchanged Data & Services — 3.16 MB (baseline 3.16 MB) • 🔴 +1.85 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed / 4 unchanged Utilities & Hooks — 366 kB (baseline 366 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 13 added / 13 removed / 18 unchanged Vendor & Third-Party — 9.94 MB (baseline 9.94 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 9.16 MB (baseline 9.16 MB) • ⚪ 0 BBundles that do not match a named category
Status: 57 added / 57 removed / 86 unchanged ⚡ Performance Report
No regressions detected. All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-05-16T03:34:10.684Z",
"gitSha": "6c9ae27462916f2ee9d92a4a5f955277ef0f9724",
"branch": "glary/escape-key-raised-surfaces",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2067.286999999993,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.568999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 395.12500000000006,
"heapDeltaBytes": 19538312,
"heapUsedBytes": 69389000,
"domNodes": -264,
"jsHeapTotalBytes": 17522688,
"scriptDurationMs": 14.752,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2025.621000000001,
"styleRecalcs": 11,
"styleRecalcDurationMs": 7.482000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 361.04900000000004,
"heapDeltaBytes": -10791868,
"heapUsedBytes": 57580132,
"domNodes": -257,
"jsHeapTotalBytes": 21979136,
"scriptDurationMs": 18.223,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1673.7939999999867,
"styleRecalcs": 71,
"styleRecalcDurationMs": 29.955,
"layouts": 12,
"layoutDurationMs": 3.25,
"taskDurationMs": 586.3699999999999,
"heapDeltaBytes": 20123920,
"heapUsedBytes": 69189720,
"domNodes": 53,
"jsHeapTotalBytes": 17301504,
"scriptDurationMs": 98.182,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1691.3079999999923,
"styleRecalcs": 72,
"styleRecalcDurationMs": 26.241999999999997,
"layouts": 12,
"layoutDurationMs": 2.7459999999999996,
"taskDurationMs": 603.0419999999999,
"heapDeltaBytes": 1797700,
"heapUsedBytes": 50447072,
"domNodes": -259,
"jsHeapTotalBytes": 15855616,
"scriptDurationMs": 90.667,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999727
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1705.383999999981,
"styleRecalcs": 31,
"styleRecalcDurationMs": 13.224,
"layouts": 6,
"layoutDurationMs": 0.509,
"taskDurationMs": 293.398,
"heapDeltaBytes": -3989360,
"heapUsedBytes": 44886712,
"domNodes": -207,
"jsHeapTotalBytes": 19787776,
"scriptDurationMs": 16.212999999999997,
"eventListeners": -118,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1718.6039999999707,
"styleRecalcs": 33,
"styleRecalcDurationMs": 13.607,
"layouts": 6,
"layoutDurationMs": 0.528,
"taskDurationMs": 237.82499999999996,
"heapDeltaBytes": 883088,
"heapUsedBytes": 48924500,
"domNodes": 79,
"jsHeapTotalBytes": 14942208,
"scriptDurationMs": 20.704000000000004,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 493.9150000000154,
"styleRecalcs": 13,
"styleRecalcDurationMs": 8.282000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 313.036,
"heapDeltaBytes": -4354168,
"heapUsedBytes": 61420540,
"domNodes": 21,
"jsHeapTotalBytes": 24735744,
"scriptDurationMs": 52.02899999999999,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "dom-widget-clipping",
"durationMs": 438.0450000000451,
"styleRecalcs": 13,
"styleRecalcDurationMs": 10.292000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 271.575,
"heapDeltaBytes": 11073736,
"heapUsedBytes": 60155624,
"domNodes": 20,
"jsHeapTotalBytes": 16515072,
"scriptDurationMs": 46.913,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2012.9660000000058,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.055,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 510.788,
"heapDeltaBytes": 42614124,
"heapUsedBytes": 100195064,
"domNodes": -258,
"jsHeapTotalBytes": 32534528,
"scriptDurationMs": 79.004,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2018.4879999999907,
"styleRecalcs": 12,
"styleRecalcDurationMs": 7.321999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 436.31199999999995,
"heapDeltaBytes": 10596872,
"heapUsedBytes": 67392088,
"domNodes": -256,
"jsHeapTotalBytes": 290816,
"scriptDurationMs": 67.40899999999999,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-pan",
"durationMs": 2071.96799999997,
"styleRecalcs": 68,
"styleRecalcDurationMs": 16.823999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 887.2850000000001,
"heapDeltaBytes": 60415608,
"heapUsedBytes": 120247704,
"domNodes": -259,
"jsHeapTotalBytes": 36409344,
"scriptDurationMs": 290.79,
"eventListeners": -125,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2115.4019999999605,
"styleRecalcs": 70,
"styleRecalcDurationMs": 17.563999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 926.695,
"heapDeltaBytes": 48390992,
"heapUsedBytes": 108069780,
"domNodes": -254,
"jsHeapTotalBytes": 40341504,
"scriptDurationMs": 306.751,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3088.895999999977,
"styleRecalcs": 66,
"styleRecalcDurationMs": 16.917,
"layouts": 60,
"layoutDurationMs": 6.834,
"taskDurationMs": 1029.268,
"heapDeltaBytes": 2611352,
"heapUsedBytes": 64683972,
"domNodes": -262,
"jsHeapTotalBytes": 4542464,
"scriptDurationMs": 376.238,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3080.767000000037,
"styleRecalcs": 67,
"styleRecalcDurationMs": 17.59,
"layouts": 60,
"layoutDurationMs": 6.784999999999999,
"taskDurationMs": 1039.971,
"heapDeltaBytes": 8734472,
"heapUsedBytes": 69563452,
"domNodes": -262,
"jsHeapTotalBytes": 4747264,
"scriptDurationMs": 381.70799999999997,
"eventListeners": -125,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "minimap-idle",
"durationMs": 2045.5499999999915,
"styleRecalcs": 10,
"styleRecalcDurationMs": 9.898,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 559.0789999999998,
"heapDeltaBytes": 44902992,
"heapUsedBytes": 104839376,
"domNodes": -253,
"jsHeapTotalBytes": 31223808,
"scriptDurationMs": 88.762,
"eventListeners": -125,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 1991.3050000000112,
"styleRecalcs": 10,
"styleRecalcDurationMs": 6.6469999999999985,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 417.01899999999995,
"heapDeltaBytes": 4003304,
"heapUsedBytes": 62369100,
"domNodes": -260,
"jsHeapTotalBytes": 4747264,
"scriptDurationMs": 65.424,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 441.86100000001716,
"styleRecalcs": 49,
"styleRecalcDurationMs": 10.461,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 285.088,
"heapDeltaBytes": 9912000,
"heapUsedBytes": 58821652,
"domNodes": 23,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 100.101,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 440.28199999996787,
"styleRecalcs": 49,
"styleRecalcDurationMs": 10.273,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 278.479,
"heapDeltaBytes": 9136688,
"heapUsedBytes": 58032768,
"domNodes": 22,
"jsHeapTotalBytes": 15990784,
"scriptDurationMs": 96.32800000000002,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2007.7159999999594,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.863000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 394.106,
"heapDeltaBytes": 17955848,
"heapUsedBytes": 83958132,
"domNodes": -244,
"jsHeapTotalBytes": 40853504,
"scriptDurationMs": 16.697,
"eventListeners": -155,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2020.8210000000122,
"styleRecalcs": 11,
"styleRecalcDurationMs": 6.813,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 282.875,
"heapDeltaBytes": 3738284,
"heapUsedBytes": 72537200,
"domNodes": 20,
"jsHeapTotalBytes": 18874368,
"scriptDurationMs": 12.831,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1702.9420000000073,
"styleRecalcs": 76,
"styleRecalcDurationMs": 31.04,
"layouts": 16,
"layoutDurationMs": 3.8739999999999997,
"taskDurationMs": 611.865,
"heapDeltaBytes": 1360380,
"heapUsedBytes": 51472012,
"domNodes": -255,
"jsHeapTotalBytes": 21454848,
"scriptDurationMs": 81.993,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666682,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1703.1200000000126,
"styleRecalcs": 77,
"styleRecalcDurationMs": 29.121999999999996,
"layouts": 16,
"layoutDurationMs": 3.6820000000000004,
"taskDurationMs": 576.8030000000001,
"heapDeltaBytes": 15591192,
"heapUsedBytes": 64247984,
"domNodes": -255,
"jsHeapTotalBytes": 19525632,
"scriptDurationMs": 76.738,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-transition-enter",
"durationMs": 832.8950000000077,
"styleRecalcs": 16,
"styleRecalcDurationMs": 21.373000000000005,
"layouts": 7,
"layoutDurationMs": 9.127999999999998,
"taskDurationMs": 577.5799999999999,
"heapDeltaBytes": 4259504,
"heapUsedBytes": 94697704,
"domNodes": 13513,
"jsHeapTotalBytes": 16515072,
"scriptDurationMs": 20.79700000000001,
"eventListeners": 2527,
"totalBlockingTimeMs": 89,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "viewport-pan-sweep",
"durationMs": 8095.304999999997,
"styleRecalcs": 249,
"styleRecalcDurationMs": 50.129,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 2871.9199999999996,
"heapDeltaBytes": 10838816,
"heapUsedBytes": 68131752,
"domNodes": -257,
"jsHeapTotalBytes": 5738496,
"scriptDurationMs": 951.5519999999999,
"eventListeners": -143,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333338,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 8097.150999999996,
"styleRecalcs": 250,
"styleRecalcDurationMs": 50.452,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3015.171,
"heapDeltaBytes": 19537060,
"heapUsedBytes": 78184556,
"domNodes": -257,
"jsHeapTotalBytes": 6787072,
"scriptDurationMs": 1073.7100000000003,
"eventListeners": -111,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 13469.859000000042,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 13453.619000000002,
"heapDeltaBytes": -39268096,
"heapUsedBytes": 214345832,
"domNodes": -8331,
"jsHeapTotalBytes": -1249280,
"scriptDurationMs": 580.2689999999999,
"eventListeners": -16488,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 13794.851999999992,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 13755.398000000001,
"heapDeltaBytes": -30882956,
"heapUsedBytes": 213256476,
"domNodes": -8331,
"jsHeapTotalBytes": -11472896,
"scriptDurationMs": 582.9159999999999,
"eventListeners": -16488,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 15060.966000000008,
"styleRecalcs": 74,
"styleRecalcDurationMs": 17.799999999999983,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 15045.756,
"heapDeltaBytes": -17331628,
"heapUsedBytes": 252612384,
"domNodes": -8331,
"jsHeapTotalBytes": 12382208,
"scriptDurationMs": 920.9099999999999,
"eventListeners": -16488,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 15236.779999999953,
"styleRecalcs": 77,
"styleRecalcDurationMs": 17.75399999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 15214.451999999997,
"heapDeltaBytes": -9946532,
"heapUsedBytes": 258195964,
"domNodes": -8333,
"jsHeapTotalBytes": 13955072,
"scriptDurationMs": 923.974,
"eventListeners": -16486,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.77333333333336,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 437.7939999999967,
"styleRecalcs": 19,
"styleRecalcDurationMs": 23.214,
"layouts": 4,
"layoutDurationMs": 1.092,
"taskDurationMs": 105.651,
"heapDeltaBytes": 5406768,
"heapUsedBytes": 55336244,
"domNodes": 170,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 22.036,
"eventListeners": 71,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "workflow-execution",
"durationMs": 439.8439999999937,
"styleRecalcs": 19,
"styleRecalcDurationMs": 20.044000000000004,
"layouts": 5,
"layoutDurationMs": 1.1680000000000001,
"taskDurationMs": 101.76600000000002,
"heapDeltaBytes": 5179808,
"heapUsedBytes": 54994064,
"domNodes": 160,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 24.783,
"eventListeners": 71,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999727
}
]
} |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/litegraph/src/ContextMenu.test.ts`:
- Around line 20-47: Tests create ContextMenu instances but don’t always call
close(), leaking document-level listeners; update each test that constructs a
top-level ContextMenu (the first "dispatches an 'open' event..." test and the
third "does not dispatch lifecycle events for submenus" test) to keep a
reference to the created menu (e.g., const menu = new ContextMenu(...)) and call
menu.close() (or parent.close() for the parent menu) after the assertions (or in
a finally/afterEach), ensuring you still reset calls.length where needed;
reference the ContextMenu constructor, close() method, and the existing calls
array when making the changes.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: f2e04346-6974-44bb-97bd-b01d0cbab579
📒 Files selected for processing (9)
src/components/graph/GraphCanvas.vuesrc/components/graph/NodeContextMenu.vuesrc/lib/litegraph/src/ContextMenu.test.tssrc/lib/litegraph/src/ContextMenu.tssrc/platform/keybindings/keybindingService.escape.test.tssrc/platform/keybindings/keybindingService.tssrc/platform/keybindings/raisedSurfaceLiteGraphBridge.tssrc/platform/keybindings/raisedSurfaceStore.test.tssrc/platform/keybindings/raisedSurfaceStore.ts
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #12308 +/- ##
===========================================
- Coverage 74.67% 59.56% -15.12%
===========================================
Files 1526 1414 -112
Lines 95359 71925 -23434
Branches 27134 19033 -8101
===========================================
- Hits 71212 42840 -28372
- Misses 23285 28612 +5327
+ Partials 862 473 -389
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1020 files with indirect coverage changes 🚀 New features to boost your workflow:
|
- ContextMenu: use the menu's owner document (event target ownerDocument or fallback) for listeners, DOM insertion, and lifecycle events, so menus opened in fullscreen or alternate documents stay consistent. - raisedSurfaceLiteGraphBridge: track active context-menu ids and release them on scope dispose, so a menu open during unmount doesn't leak a stale store entry. - ContextMenu tests: close all top-level menus in afterEach to prevent document-level listeners leaking between tests.
Addressed review feedback (1e381cc)Oracle review:
CodeRabbit review:
All 113 keybinding + LiteGraph context-menu tests and all 935 litegraph tests still pass. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/ContextMenu.ts (1)
199-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
ownerDocumentfor bounds calculation to avoid wrong positioning in non-global documents.Line 216 still reads from
document.body, which can clamp menu coordinates against the wrong viewport when the menu belongs to another document (iframe/fullscreen/alternate document). Please compute bounds fromownerDocumentconsistently.Suggested fix
- const body_rect = document.body.getBoundingClientRect() + const body_rect = ownerDocument.body.getBoundingClientRect()🤖 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/lib/litegraph/src/ContextMenu.ts` around lines 199 - 217, The positioning code in ContextMenu.ts computes clipping bounds using the global document instead of the menu's ownerDocument; update the bounds calculations in the block that computes left/top (the code referencing ownerDocument, parent, root and options) to use ownerDocument consistently—replace any use of document.body.getBoundingClientRect() (and any other document.* references in that positioning block) with ownerDocument.body.getBoundingClientRect() so the menu is clamped against the correct document/viewport (e.g., when in an iframe or fullscreen document).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/litegraph/src/ContextMenu.ts`:
- Around line 199-217: The positioning code in ContextMenu.ts computes clipping
bounds using the global document instead of the menu's ownerDocument; update the
bounds calculations in the block that computes left/top (the code referencing
ownerDocument, parent, root and options) to use ownerDocument
consistently—replace any use of document.body.getBoundingClientRect() (and any
other document.* references in that positioning block) with
ownerDocument.body.getBoundingClientRect() so the menu is clamped against the
correct document/viewport (e.g., when in an iframe or fullscreen document).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cb18984-b961-4434-a91e-f8b4072106c2
📒 Files selected for processing (3)
src/lib/litegraph/src/ContextMenu.test.tssrc/lib/litegraph/src/ContextMenu.tssrc/platform/keybindings/raisedSurfaceLiteGraphBridge.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts
Last remaining global `document.body` reference in the positioning block: switch to `ownerDocument.body` so menus opened inside iframes, fullscreen elements, or alternate documents are clamped against the correct viewport.
|
Fixed in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Created by the Glary-Bot Agent
Summary
Pressing Escape on a right-click context menu while inside a subgraph used to fire the global
Comfy.Graph.ExitSubgraphkeybinding, exiting the subgraph while leaving the menu open. The keybinding service only suppressed Escape when a Pinia dialog was on the stack, so other raised surfaces (LiteGraph context menus, PrimeVue popovers) leaked the event to the window-level handler.This PR encodes the UX axiom — standard keybindings are suppressed whenever any raised surface (context menu, popover, or top-level modal) is open — as a centralized reactive registry that the keybinding service consults as its single source of truth.
Changes
useRaisedSurfaceStore(src/platform/keybindings/raisedSurfaceStore.ts) — a Pinia store that tracks open raised surfaces by id. Sibling to the existingdialogStore.useRaisedSurface(kind, isOpen)composable — one-line opt-in for any surface. Releases the entry both whenisOpenflips to false and when the owning Vue scope is disposed.keybindingService.tsconsultsraisedSurfaceStore.isAnyOpenalongside the existingdialogStore.dialogStackcheck.ContextMenu.ts(LiteGraph) now closes itself on Escape (mirroring its existing outside-pointerdown handler) and dispatcheslitegraph:contextmenudocument events on open/close. No monkey patching — the registration lives directly in litegraph.raisedSurfaceLiteGraphBridge.tssubscribes to those events and updates the store. Installed once fromGraphCanvas.vuenext to the existinguseContextMenuTranslation()install.NodeContextMenu.vueregisters viauseRaisedSurface('context-menu', isOpen)— one line.The legacy LiteGraph menu's Escape handler also calls
e.stopPropagation(), consistent with the existingstopEscapeToDocumentpattern used bySingleSelectandMultiSelect. This keeps the bubble-phasekeybindingServiceconsultation race-free without preempting other surfaces' Escape handlers via capture-phase listening.Verification
raisedSurfaceStore.test.ts(7),ContextMenu.test.ts(3), plus 2 new regression cases inkeybindingService.escape.test.ts.pnpm typecheck,pnpm lint,pnpm format:check,pnpm knipclean (3 pre-existing lint warnings, 1 pre-existing knip tag, all unrelated).event.defaultPreventedis true.Follow-ups (out of scope for this PR)
useDialogStore.dialogStackto register itself inraisedSurfaceStoreaskind: 'modal'so there's a single source of truth.useRaisedSurface('popover', isOpen)inSingleSelect/MultiSelect(and other Reka popovers) so their existingstopEscapeToDocumentcalls can eventually be removed.e.stopPropagation()atLGraphCanvas.ts:3727.Screenshots
┆Issue is synchronized with this Notion page by Unito