fix: stop event propagation on keybinding match to prevent dropdown expansion#11886
Open
kaili-yang wants to merge 3 commits intomainfrom
Open
fix: stop event propagation on keybinding match to prevent dropdown expansion#11886kaili-yang wants to merge 3 commits intomainfrom
kaili-yang wants to merge 3 commits intomainfrom
Conversation
dosubot
Bot
added
the
size:XS
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements keyboard event propagation control for keybindings. The keybinding service now calls ChangesKeyboard Event Propagation Control
Sequence DiagramsequenceDiagram
participant User
participant Window
participant GraphView
participant KeybindingService
participant CommandStore
participant CanvasState
rect rgba(200, 150, 100, 0.5)
Note over User,CanvasState: Before (default bubbling phase)
User->>Window: Press Ctrl+Enter
Window->>GraphView: keydown event (bubbling)
GraphView->>KeybindingService: keybindHandler called
KeybindingService->>KeybindingService: Match keybinding
KeybindingService->>CommandStore: execute(QueuePrompt)
CommandStore->>CanvasState: Check ghostNodeId
Note over Window,CanvasState: Event continues bubbling
end
rect rgba(100, 150, 200, 0.5)
Note over User,CanvasState: After (capture phase + stopPropagation)
User->>Window: Press Ctrl+Enter
Window->>GraphView: keydown event (capture phase)
GraphView->>KeybindingService: keybindHandler called
KeybindingService->>KeybindingService: Match keybinding
KeybindingService->>Window: stopPropagation()
KeybindingService->>CommandStore: execute(QueuePrompt)
CommandStore->>CanvasState: Check ghostNodeId
CanvasState-->>CommandStore: If ghostNodeId set, finalize & abort
Note over Window,CanvasState: Event propagation halted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 1483 passed, 0 failed · 1 flaky📊 Browser Reports
|
📦 Bundle: 5.26 MB gzip 🔴 +174 BDetailsSummary
Category Glance App Entry Points — 22.6 kB (baseline 22.6 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.24 MB (baseline 1.24 MB) • 🔴 +237 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 82.3 kB (baseline 82.3 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 2 unchanged Panels & Settings — 489 kB (baseline 489 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 11 unchanged User & Accounts — 17.6 kB (baseline 17.6 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 — 62.9 kB (baseline 62.9 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 9 unchanged Data & Services — 3.05 MB (baseline 3.05 MB) • 🔴 +112 BStores, services, APIs, and repositories
Status: 13 added / 13 removed / 4 unchanged Utilities & Hooks — 365 kB (baseline 365 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 — 8.82 MB (baseline 8.82 MB) • ⚪ 0 BBundles that do not match a named category
Status: 57 added / 57 removed / 79 unchanged ⚡ Performance Report
No regressions detected. All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-05-04T23:07:35.051Z",
"gitSha": "3acdd9f187e84ba6a05d7598e28325dd66d1b393",
"branch": "fix/dropdown-expands-on-run-keybind",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2017.0739999999796,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.9319999999999995,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 369.468,
"heapDeltaBytes": 22717124,
"heapUsedBytes": 72220300,
"domNodes": 16,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 13.824999999999998,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2045.0250000000096,
"styleRecalcs": 11,
"styleRecalcDurationMs": 9.764,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 380.247,
"heapDeltaBytes": 23885056,
"heapUsedBytes": 72803848,
"domNodes": 22,
"jsHeapTotalBytes": 15466496,
"scriptDurationMs": 23.556,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-idle",
"durationMs": 2001.0090000000673,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 347.433,
"heapDeltaBytes": 22923152,
"heapUsedBytes": 71633260,
"domNodes": 18,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 14.927000000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1927.3529999999823,
"styleRecalcs": 75,
"styleRecalcDurationMs": 43.142,
"layouts": 12,
"layoutDurationMs": 3.7409999999999997,
"taskDurationMs": 821.033,
"heapDeltaBytes": -3061064,
"heapUsedBytes": 45782484,
"domNodes": -263,
"jsHeapTotalBytes": 15855616,
"scriptDurationMs": 133.594,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1885.9939999999824,
"styleRecalcs": 73,
"styleRecalcDurationMs": 39.072,
"layouts": 12,
"layoutDurationMs": 4.08,
"taskDurationMs": 793.667,
"heapDeltaBytes": -694328,
"heapUsedBytes": 47287564,
"domNodes": -265,
"jsHeapTotalBytes": 16117760,
"scriptDurationMs": 124.299,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1790.5379999999695,
"styleRecalcs": 74,
"styleRecalcDurationMs": 36.418,
"layouts": 12,
"layoutDurationMs": 3.42,
"taskDurationMs": 742.61,
"heapDeltaBytes": -1200832,
"heapUsedBytes": 66674896,
"domNodes": 58,
"jsHeapTotalBytes": 19660800,
"scriptDurationMs": 126.25799999999998,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1711.7080000000442,
"styleRecalcs": 30,
"styleRecalcDurationMs": 16.994000000000003,
"layouts": 6,
"layoutDurationMs": 0.72,
"taskDurationMs": 290.219,
"heapDeltaBytes": 219556,
"heapUsedBytes": 48482616,
"domNodes": 74,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 17.804,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1720.6429999999955,
"styleRecalcs": 31,
"styleRecalcDurationMs": 17.927000000000003,
"layouts": 6,
"layoutDurationMs": 0.6410000000000001,
"taskDurationMs": 290.873,
"heapDeltaBytes": 218416,
"heapUsedBytes": 48751984,
"domNodes": 79,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 17.941999999999993,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1717.4989999999752,
"styleRecalcs": 31,
"styleRecalcDurationMs": 16.029999999999998,
"layouts": 6,
"layoutDurationMs": 0.518,
"taskDurationMs": 286.39599999999996,
"heapDeltaBytes": 170168,
"heapUsedBytes": 48524424,
"domNodes": 76,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 17.735999999999994,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 534.7939999999767,
"styleRecalcs": 10,
"styleRecalcDurationMs": 6.959999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 327.429,
"heapDeltaBytes": 9141588,
"heapUsedBytes": 57463032,
"domNodes": 16,
"jsHeapTotalBytes": 15728640,
"scriptDurationMs": 59.81800000000001,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 539.7720000000845,
"styleRecalcs": 13,
"styleRecalcDurationMs": 9.628999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 332.61,
"heapDeltaBytes": 9169824,
"heapUsedBytes": 56929384,
"domNodes": 21,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 63.237,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "dom-widget-clipping",
"durationMs": 530.9919999999693,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.126999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 320.689,
"heapDeltaBytes": 9135924,
"heapUsedBytes": 57426908,
"domNodes": 19,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 58.455000000000005,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "large-graph-idle",
"durationMs": 2013.2060000000251,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.870000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 527.4989999999999,
"heapDeltaBytes": 8260064,
"heapUsedBytes": 67729324,
"domNodes": -262,
"jsHeapTotalBytes": 3756032,
"scriptDurationMs": 90.06900000000002,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2012.5860000000557,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.328999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 545.6650000000001,
"heapDeltaBytes": 16027696,
"heapUsedBytes": 75502152,
"domNodes": -264,
"jsHeapTotalBytes": 3756032,
"scriptDurationMs": 91.93099999999998,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2054.2800000000625,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.183,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 544.366,
"heapDeltaBytes": 9320348,
"heapUsedBytes": 67517068,
"domNodes": -262,
"jsHeapTotalBytes": 1077248,
"scriptDurationMs": 94.992,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2119.121000000007,
"styleRecalcs": 68,
"styleRecalcDurationMs": 17.407000000000004,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1066.9430000000002,
"heapDeltaBytes": 9719396,
"heapUsedBytes": 69170744,
"domNodes": -260,
"jsHeapTotalBytes": -815104,
"scriptDurationMs": 401.448,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2129.6350000000075,
"styleRecalcs": 69,
"styleRecalcDurationMs": 16.868000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1055.9650000000001,
"heapDeltaBytes": 2344568,
"heapUsedBytes": 61473296,
"domNodes": -264,
"jsHeapTotalBytes": 1282048,
"scriptDurationMs": 403.374,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-pan",
"durationMs": 2112.7219999999625,
"styleRecalcs": 68,
"styleRecalcDurationMs": 17.041,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1056.3890000000001,
"heapDeltaBytes": 4409096,
"heapUsedBytes": 63707112,
"domNodes": -264,
"jsHeapTotalBytes": 757760,
"scriptDurationMs": 399.578,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3149.10100000003,
"styleRecalcs": 64,
"styleRecalcDurationMs": 16.199,
"layouts": 60,
"layoutDurationMs": 6.979000000000001,
"taskDurationMs": 1310.2720000000002,
"heapDeltaBytes": 8615640,
"heapUsedBytes": 69689804,
"domNodes": -272,
"jsHeapTotalBytes": 4485120,
"scriptDurationMs": 506.071,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-zoom",
"durationMs": 3131.603000000041,
"styleRecalcs": 66,
"styleRecalcDurationMs": 18.784,
"layouts": 60,
"layoutDurationMs": 7.2620000000000005,
"taskDurationMs": 1300.694,
"heapDeltaBytes": -7973072,
"heapUsedBytes": 60333296,
"domNodes": 16,
"jsHeapTotalBytes": 14299136,
"scriptDurationMs": 527.1129999999999,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3181.6690000000563,
"styleRecalcs": 65,
"styleRecalcDurationMs": 17.139,
"layouts": 60,
"layoutDurationMs": 6.960000000000001,
"taskDurationMs": 1306.076,
"heapDeltaBytes": 8925052,
"heapUsedBytes": 69786680,
"domNodes": -268,
"jsHeapTotalBytes": 815104,
"scriptDurationMs": 497.58399999999995,
"eventListeners": -125,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "minimap-idle",
"durationMs": 2051.1960000000045,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.572000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 540.022,
"heapDeltaBytes": 14050888,
"heapUsedBytes": 73887840,
"domNodes": -263,
"jsHeapTotalBytes": 290816,
"scriptDurationMs": 96.15499999999999,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2019.8030000000244,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.421000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 526.3739999999999,
"heapDeltaBytes": 4295408,
"heapUsedBytes": 64193588,
"domNodes": -265,
"jsHeapTotalBytes": 552960,
"scriptDurationMs": 91.364,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2019.8560000000043,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.160000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 531.0989999999999,
"heapDeltaBytes": 8285828,
"heapUsedBytes": 68608004,
"domNodes": -267,
"jsHeapTotalBytes": -233472,
"scriptDurationMs": 92.82400000000001,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 525.0169999999912,
"styleRecalcs": 46,
"styleRecalcDurationMs": 10.403,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 335.691,
"heapDeltaBytes": 9124868,
"heapUsedBytes": 57752784,
"domNodes": 18,
"jsHeapTotalBytes": 15728640,
"scriptDurationMs": 113.68299999999998,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 554.4290000000274,
"styleRecalcs": 47,
"styleRecalcDurationMs": 11.669999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 372.07,
"heapDeltaBytes": -11666880,
"heapUsedBytes": 49763568,
"domNodes": 20,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 128.59,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 539.3309999999474,
"styleRecalcs": 46,
"styleRecalcDurationMs": 9.671000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 346.723,
"heapDeltaBytes": 9438624,
"heapUsedBytes": 57761212,
"domNodes": 18,
"jsHeapTotalBytes": 15990784,
"scriptDurationMs": 119.60900000000001,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "subgraph-idle",
"durationMs": 1997.829999999965,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.236999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 350.626,
"heapDeltaBytes": 22436956,
"heapUsedBytes": 70391972,
"domNodes": 22,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 16.656000000000006,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2003.9229999999861,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.209,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 384.994,
"heapDeltaBytes": -3460040,
"heapUsedBytes": 45188064,
"domNodes": -262,
"jsHeapTotalBytes": 15069184,
"scriptDurationMs": 13.305000000000005,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2003.1149999999798,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.930000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 333.843,
"heapDeltaBytes": 22495992,
"heapUsedBytes": 71241844,
"domNodes": 16,
"jsHeapTotalBytes": 14417920,
"scriptDurationMs": 12.313,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1706.165999999996,
"styleRecalcs": 75,
"styleRecalcDurationMs": 34.325,
"layouts": 16,
"layoutDurationMs": 4.412,
"taskDurationMs": 684.745,
"heapDeltaBytes": 645484,
"heapUsedBytes": 49242024,
"domNodes": -261,
"jsHeapTotalBytes": 15593472,
"scriptDurationMs": 90.72300000000001,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1705.885999999964,
"styleRecalcs": 75,
"styleRecalcDurationMs": 35.224,
"layouts": 16,
"layoutDurationMs": 4.1499999999999995,
"taskDurationMs": 653.882,
"heapDeltaBytes": 14034236,
"heapUsedBytes": 62622132,
"domNodes": 60,
"jsHeapTotalBytes": 15466496,
"scriptDurationMs": 89.84899999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1701.4820000000555,
"styleRecalcs": 75,
"styleRecalcDurationMs": 35.943,
"layouts": 16,
"layoutDurationMs": 4.192,
"taskDurationMs": 651.982,
"heapDeltaBytes": 14272212,
"heapUsedBytes": 62985084,
"domNodes": 60,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 93.174,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "viewport-pan-sweep",
"durationMs": 8148.286999999982,
"styleRecalcs": 249,
"styleRecalcDurationMs": 50.50099999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3585.348,
"heapDeltaBytes": 17221624,
"heapUsedBytes": 76037724,
"domNodes": -260,
"jsHeapTotalBytes": 5505024,
"scriptDurationMs": 1271.5159999999998,
"eventListeners": -113,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8109.933999999953,
"styleRecalcs": 250,
"styleRecalcDurationMs": 51.50000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3611.8929999999996,
"heapDeltaBytes": 20950524,
"heapUsedBytes": 79255148,
"domNodes": -255,
"jsHeapTotalBytes": 3117056,
"scriptDurationMs": 1275.607,
"eventListeners": -113,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 8172.759999999926,
"styleRecalcs": 250,
"styleRecalcDurationMs": 50.675,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3645.806,
"heapDeltaBytes": 19480900,
"heapUsedBytes": 78956592,
"domNodes": -260,
"jsHeapTotalBytes": 6029312,
"scriptDurationMs": 1287.7669999999998,
"eventListeners": -113,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 11382.724999999993,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 11371.48,
"heapDeltaBytes": -36778608,
"heapUsedBytes": 164343856,
"domNodes": -8331,
"jsHeapTotalBytes": 15527936,
"scriptDurationMs": 632.83,
"eventListeners": -16468,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333237,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 11106.992999999988,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 11085.293,
"heapDeltaBytes": -25100032,
"heapUsedBytes": 172477568,
"domNodes": -8331,
"jsHeapTotalBytes": 24965120,
"scriptDurationMs": 593.2249999999999,
"eventListeners": -16465,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333237,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 11334.98099999997,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 11322.097,
"heapDeltaBytes": -25848508,
"heapUsedBytes": 173021344,
"domNodes": -8331,
"jsHeapTotalBytes": 24702976,
"scriptDurationMs": 623.779,
"eventListeners": -16468,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.220000000000073,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 14169.07699999996,
"styleRecalcs": 66,
"styleRecalcDurationMs": 15.307000000000015,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14145.868,
"heapDeltaBytes": -42258024,
"heapUsedBytes": 153200256,
"domNodes": -8331,
"jsHeapTotalBytes": -2297856,
"scriptDurationMs": 907.2280000000001,
"eventListeners": -16488,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 14255.45999999997,
"styleRecalcs": 68,
"styleRecalcDurationMs": 15.52199999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14234.836,
"heapDeltaBytes": -23851660,
"heapUsedBytes": 174602228,
"domNodes": -8329,
"jsHeapTotalBytes": -2383872,
"scriptDurationMs": 889.3100000000001,
"eventListeners": -16490,
"totalBlockingTimeMs": 28,
"frameDurationMs": 17.220000000000073,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 14238.238000000023,
"styleRecalcs": 67,
"styleRecalcDurationMs": 16.30799999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14218.717,
"heapDeltaBytes": -33377272,
"heapUsedBytes": 171207916,
"domNodes": -8331,
"jsHeapTotalBytes": -4743168,
"scriptDurationMs": 961.2160000000001,
"eventListeners": -16488,
"totalBlockingTimeMs": 1,
"frameDurationMs": 16.66333333333338,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 464.76400000005924,
"styleRecalcs": 20,
"styleRecalcDurationMs": 22.397999999999996,
"layouts": 5,
"layoutDurationMs": 1.6660000000000001,
"taskDurationMs": 122.422,
"heapDeltaBytes": 5385300,
"heapUsedBytes": 55531260,
"domNodes": 188,
"jsHeapTotalBytes": 524288,
"scriptDurationMs": 22.510999999999996,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "workflow-execution",
"durationMs": 460.1639999999634,
"styleRecalcs": 15,
"styleRecalcDurationMs": 20.519999999999996,
"layouts": 5,
"layoutDurationMs": 1.2899999999999998,
"taskDurationMs": 113.33100000000002,
"heapDeltaBytes": 5028960,
"heapUsedBytes": 56509796,
"domNodes": 155,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 23.445,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "workflow-execution",
"durationMs": 461.5329999999176,
"styleRecalcs": 16,
"styleRecalcDurationMs": 23.032000000000004,
"layouts": 5,
"layoutDurationMs": 1.5610000000000002,
"taskDurationMs": 116.36899999999997,
"heapDeltaBytes": 5034464,
"heapUsedBytes": 56346004,
"domNodes": 155,
"jsHeapTotalBytes": 262144,
"scriptDurationMs": 23.03,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"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.
🧹 Nitpick comments (2)
src/platform/keybindings/keybindingService.propagation.test.ts (2)
45-63: 💤 Low value
beforeEachre-mock ofuseDialogStoreis redundant with the module-level mock.
vi.clearAllMocks()only resets call history — not mock return values — so thevi.mock('@/stores/dialogStore', ...)factory's return value already persists into each test. Thevi.mocked(useDialogStore).mockReturnValue(...)block (lines 55–59) is therefore a no-op in the current setup.🧹 Suggested simplification
beforeEach(() => { vi.clearAllMocks() setActivePinia(createTestingPinia({ stubActions: false })) const commandStore = useCommandStore() commandStore.execute = vi.fn() - vi.mocked(useDialogStore).mockReturnValue({ - dialogStack: [] - } as Partial<ReturnType<typeof useDialogStore>> as ReturnType< - typeof useDialogStore - >) - keybindingService = useKeybindingService() keybindingService.registerCoreKeybindings() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/keybindings/keybindingService.propagation.test.ts` around lines 45 - 63, The test's beforeEach re-mocks useDialogStore unnecessarily: remove the vi.mocked(useDialogStore).mockReturnValue(...) block inside beforeEach (the lines setting dialogStack) because the module-level vi.mock('@/stores/dialogStore', ...) already provides the mock return value and persists across tests; keep vi.clearAllMocks(), setActivePinia/createTestingPinia, commandStore.execute mock, and the keybindingService initialization and registerCoreKeybindings calls, but delete the redundant re-mock of useDialogStore.
65-83: Consider adding explicit documentation of keybinding assumptions in the test.The test's correctness depends on three assumptions: (1)
Ctrl+Enteris the core keybinding forComfy.QueuePrompt, (2) the command has notargetElementId, and (3)F13has no binding. These assumptions are valid (confirmed bysrc/platform/keybindings/defaults.ts), but documenting them explicitly within the test file—either as comments or assertion helpers—would make the test's dependencies clearer for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/keybindings/keybindingService.propagation.test.ts` around lines 65 - 83, Add explicit assertions/comments in the test to document the three implicit assumptions: that the command 'Comfy.QueuePrompt' is bound to Ctrl+Enter, that the command returned by useCommandStore().execute (or the command registry entry) has no targetElementId, and that 'F13' has no keybinding; update the test around keybindingService.keybindHandler to either (a) add short inline comments referencing these assumptions and file defaults.ts, or (b) add quick assertions that look up the keybinding for 'Comfy.QueuePrompt' and assert it equals the expected Ctrl+Enter, assert the command's targetElementId is undefined, and assert no binding exists for 'F13', so future maintainers can see the prerequisites explicitly.
🤖 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/platform/keybindings/keybindingService.propagation.test.ts`:
- Around line 45-63: The test's beforeEach re-mocks useDialogStore
unnecessarily: remove the vi.mocked(useDialogStore).mockReturnValue(...) block
inside beforeEach (the lines setting dialogStack) because the module-level
vi.mock('@/stores/dialogStore', ...) already provides the mock return value and
persists across tests; keep vi.clearAllMocks(),
setActivePinia/createTestingPinia, commandStore.execute mock, and the
keybindingService initialization and registerCoreKeybindings calls, but delete
the redundant re-mock of useDialogStore.
- Around line 65-83: Add explicit assertions/comments in the test to document
the three implicit assumptions: that the command 'Comfy.QueuePrompt' is bound to
Ctrl+Enter, that the command returned by useCommandStore().execute (or the
command registry entry) has no targetElementId, and that 'F13' has no
keybinding; update the test around keybindingService.keybindHandler to either
(a) add short inline comments referencing these assumptions and file
defaults.ts, or (b) add quick assertions that look up the keybinding for
'Comfy.QueuePrompt' and assert it equals the expected Ctrl+Enter, assert the
command's targetElementId is undefined, and assert no binding exists for 'F13',
so future maintainers can see the prerequisites explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 702b1872-30f7-4e60-abbb-242f4b618ff1
📒 Files selected for processing (3)
src/platform/keybindings/keybindingService.propagation.test.tssrc/platform/keybindings/keybindingService.tssrc/views/GraphView.vue
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #11886 +/- ##
===========================================
- Coverage 71.98% 56.32% -15.67%
===========================================
Files 1493 1385 -108
Lines 89839 70906 -18933
Branches 25661 18861 -6800
===========================================
- Hits 64672 39935 -24737
- Misses 24255 30441 +6186
+ Partials 912 530 -382
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1001 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/platform/keybindings/keybindingService.escape.test.ts (2)
103-120: ⚡ Quick winNote: the two Escape guard paths are tested independently but there's no test that exercises them together via target mismatch.
The dialog-open guard (lines 110–120) blocks
Comfy.Graph.ExitSubgrapheven when the event originates fromcanvasContainer. That's one protection path. The other path — thetargetElementIdcheck blocking execution when the event originates from outsidegraph-canvas-container(e.g., a focused dialog element) — has no dedicated test case here. Consider adding a test wheretargetis overridden to a non-canvas element and no dialog is open, asserting the command is not called. This would confirm thetargetElementIdmechanism on its own, independent of the dialog guard.✅ Suggested additional test
+ it('should NOT execute Escape keybinding when event originates outside graph-canvas-container', async () => { + const outsideTarget = document.createElement('div') + outsideTarget.id = 'some-dialog-content' + document.body.appendChild(outsideTarget) + + try { + const event = createKeyboardEvent('Escape', { target: outsideTarget }) + await keybindingService.keybindHandler(event) + + expect(mockCommandExecute).not.toHaveBeenCalled() + } finally { + outsideTarget.remove() + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/keybindings/keybindingService.escape.test.ts` around lines 103 - 120, Add a new test that verifies the targetElementId guard independently: create no dialog (do not push into useDialogStore().dialogStack), obtain keybindingService via useKeybindingService(), create a keyboard event with createKeyboardEvent('Escape') but override its target to an element whose id is not 'graph-canvas-container' (simulate a focused dialog/control), call keybindingService.keybindHandler(event) and assert mockCommandExecute was not called for 'Comfy.Graph.ExitSubgraph'; this isolates the target-based blocking path from the dialog-stack guard.
168-191: ⚡ Quick winImplicit "no keybindings" assumption via fresh Pinia is fragile.
The test achieves "no keybinding matched" by calling
setActivePinia(createPinia())and thenuseKeybindingService()withoutregisterCoreKeybindings(). IfuseKeybindingService()ever auto-registers core bindings internally, the Escape combo will now matchcanvasContainer(sincetargetElementIdaligns), the command will fire, and the legacy modal code may not run — silently changing what the test proves. A more explicit approach would be to keep the existing pinia but not register any keybinding that matches a bare Escape key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/keybindings/keybindingService.escape.test.ts` around lines 168 - 191, The test currently relies on setActivePinia(createPinia()) to implicitly avoid any Escape binding, which is fragile if useKeybindingService() later auto-registers core bindings; instead, ensure no keybinding matches a bare Escape explicitly: remove the createPinia call (keep the existing Pinia), call useKeybindingService() to get keybindingService, and then either do not call registerCoreKeybindings() or explicitly clear/unregister any Escape/canvasContainer binding on keybindingService before invoking keybindHandler(event) so the legacy modal-close branch runs deterministically.
🤖 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/platform/keybindings/keybindingService.escape.test.ts`:
- Around line 103-120: Add a new test that verifies the targetElementId guard
independently: create no dialog (do not push into useDialogStore().dialogStack),
obtain keybindingService via useKeybindingService(), create a keyboard event
with createKeyboardEvent('Escape') but override its target to an element whose
id is not 'graph-canvas-container' (simulate a focused dialog/control), call
keybindingService.keybindHandler(event) and assert mockCommandExecute was not
called for 'Comfy.Graph.ExitSubgraph'; this isolates the target-based blocking
path from the dialog-stack guard.
- Around line 168-191: The test currently relies on
setActivePinia(createPinia()) to implicitly avoid any Escape binding, which is
fragile if useKeybindingService() later auto-registers core bindings; instead,
ensure no keybinding matches a bare Escape explicitly: remove the createPinia
call (keep the existing Pinia), call useKeybindingService() to get
keybindingService, and then either do not call registerCoreKeybindings() or
explicitly clear/unregister any Escape/canvasContainer binding on
keybindingService before invoking keybindHandler(event) so the legacy
modal-close branch runs deterministically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bc119f9-932f-4ce2-a3f6-43d8f3b6f330
📒 Files selected for processing (2)
src/platform/keybindings/defaults.tssrc/platform/keybindings/keybindingService.escape.test.ts
dosubot
Bot
added
size:S
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/views/GraphView.test.ts (1)
124-127: ⚡ Quick winShare a single Pinia instance across
setActivePiniaand the render plugin.
createTestingPiniais called twice, producing two unrelated instances. The component's dependency injection resolves stores against the plugin-injected instance (line 141), while any direct store access in test code uses thesetActivePiniainstance (line 126). For the current test this is harmless, but any future test that pre-populates store state in thebeforeEachand expects the component to observe it will fail silently.♻️ Proposed refactor
+import type { TestingPinia } from '@pinia/testing' +let pinia: TestingPinia beforeEach(() => { vi.clearAllMocks() - setActivePinia(createTestingPinia({ stubActions: true })) + pinia = createTestingPinia({ stubActions: true }) + setActivePinia(pinia) })render(GraphView, { global: { - plugins: [createTestingPinia({ stubActions: true })], + plugins: [pinia],Also applies to: 139-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/GraphView.test.ts` around lines 124 - 127, Tests create two Pinia instances causing stores to be different between setActivePinia and the render plugin; instead call createTestingPinia once, assign it to a variable (e.g., const pinia = createTestingPinia({ stubActions: true })), pass that same pinia to setActivePinia(pinia) in beforeEach and also pass it into the render call's plugins option so the component and test code share the same Pinia instance (update usages of setActivePinia, createTestingPinia, and the render invocation accordingly).
🤖 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/views/GraphView.test.ts`:
- Around line 124-127: Tests create two Pinia instances causing stores to be
different between setActivePinia and the render plugin; instead call
createTestingPinia once, assign it to a variable (e.g., const pinia =
createTestingPinia({ stubActions: true })), pass that same pinia to
setActivePinia(pinia) in beforeEach and also pass it into the render call's
plugins option so the component and test code share the same Pinia instance
(update usages of setActivePinia, createTestingPinia, and the render invocation
accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca4b5cd7-1a52-43cd-80fc-9f189decd8bd
📒 Files selected for processing (4)
src/composables/useCoreCommands.tssrc/platform/keybindings/keybindingService.propagation.test.tssrc/platform/keybindings/keybindingService.tssrc/views/GraphView.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/keybindings/keybindingService.ts
kaili-yang
force-pushed
the
fix/dropdown-expands-on-run-keybind
branch
from
ca3eade to
35d171c
Compare
dosubot
Bot
added
size:XS
Move keybindHandler to window capture phase and call stopPropagation() on keybinding match so the event never reaches Reka UI dropdown triggers. Add ghost-node guards in ExitSubgraph and DeleteSelectedItems so Escape and Delete cancel ghost placement instead of bypassing the _ghostKeyHandler that can no longer intercept in document capture.
dosubot
Bot
added
size:S
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/composables/useCoreCommands.ts (1)
1134-1137: 💤 Low valueGuard is correct; consider extracting the repeated pattern.
The guard mirrors the one in
Comfy.Canvas.DeleteSelectedItemsfor the same reason—Escape is now intercepted at the window-capture phase before_ghostKeyHandlercan fire. The logic is sound.That said, the same 3-line guard (check
ghostNodeId, callfinalizeGhostPlacement(true), early-return) now exists verbatim in two commands (differing only in whether the canvas is accessed viaapp.canvasorcanvas). If this pattern spreads to additional commands, extracting it reduces drift risk.♻️ Optional: extract a guard helper
+function cancelGhostPlacementIfActive( + canvas: ReturnType<typeof useCanvasStore>['getCanvas'] extends () => infer R ? R : never +): boolean { + if (canvas.state.ghostNodeId == null) return false + canvas.finalizeGhostPlacement(true) + return true +}Then in each command:
- if (canvas.state.ghostNodeId != null) { - canvas.finalizeGhostPlacement(true) - return - } + if (cancelGhostPlacementIfActive(canvas)) return🤖 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/useCoreCommands.ts` around lines 1134 - 1137, The repeated 3-line guard (checking canvas.state.ghostNodeId, calling canvas.finalizeGhostPlacement(true), and early-return) appears in both the current command and Comfy.Canvas.DeleteSelectedItems; extract this into a small helper (e.g., a function like handleGhostEscape(canvas) or finalizeGhostIfPresent(canvas)) and call it from each command instead of duplicating the three lines; ensure the helper returns a boolean indicating that it handled the ghost so callers can early-return, and update uses in the current file (useCoreCommands.ts) and the Comfy.Canvas.DeleteSelectedItems command to call the new helper.
🤖 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.
Nitpick comments:
In `@src/composables/useCoreCommands.ts`:
- Around line 1134-1137: The repeated 3-line guard (checking
canvas.state.ghostNodeId, calling canvas.finalizeGhostPlacement(true), and
early-return) appears in both the current command and
Comfy.Canvas.DeleteSelectedItems; extract this into a small helper (e.g., a
function like handleGhostEscape(canvas) or finalizeGhostIfPresent(canvas)) and
call it from each command instead of duplicating the three lines; ensure the
helper returns a boolean indicating that it handled the ghost so callers can
early-return, and update uses in the current file (useCoreCommands.ts) and the
Comfy.Canvas.DeleteSelectedItems command to call the new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fa8848f-0112-48c7-a7ea-78dfbc58cf14
📒 Files selected for processing (3)
src/composables/useCoreCommands.tssrc/platform/keybindings/keybindingService.tssrc/views/GraphView.vue
✅ Files skipped from review due to trivial changes (1)
- src/views/GraphView.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/keybindings/keybindingService.ts
stopPropagation in capture phase for Escape was blocking BuilderFooterToolbar's window-bubble listener and Reka UI / PrimeVue element-level handlers, causing E2E tests to fail when pressing Escape to close menus, popovers, and dialogs. Only suppress propagation for non-Escape keybindings; bare Escape must reach all downstream handlers. Adds tests for the new propagation behaviour and for the ghost-placement guards in DeleteSelectedItems and ExitSubgraph.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2e6ec6c. Configure here.
| event.key === 'Escape' && | ||
| !event.ctrlKey && | ||
| !event.altKey && | ||
| !event.metaKey |
There was a problem hiding this comment.
isBareEscape missing shiftKey check allows unintended propagation
Low Severity
The isBareEscape condition checks !event.ctrlKey, !event.altKey, and !event.metaKey but omits !event.shiftKey. If a user registers a custom Shift+Escape keybinding, the check would still evaluate to true, skipping stopPropagation(). This is the exact same class of bug this PR fixes for Ctrl+Enter — a matched keybinding failing to prevent element-level handlers from firing — just for a hypothetical Shift+Escape binding instead.
Reviewed by Cursor Bugbot for commit 2e6ec6c. Configure here.
dante01yoon
requested changes
May 5, 2026
Collaborator
dante01yoon
left a comment
dante01yoon
left a comment
There was a problem hiding this comment.
Requesting changes for one Escape-priority regression I think is still present on the current head. Focused validation passed locally in an isolated PR worktree: pnpm exec vitest run src/platform/keybindings/keybindingService.propagation.test.ts src/platform/keybindings/keybindingService.escape.test.ts src/composables/useCoreCommands.test.ts (36 tests). GitHub checks are green.
| !event.ctrlKey && | ||
| !event.altKey && | ||
| !event.metaKey | ||
| if (!isBareEscape) { |
Collaborator
There was a problem hiding this comment.
issue: Bare Escape still executes the global keybinding before element-level handlers get a chance to consume it. Because GraphView now registers keybindHandler on window capture, this branch skips stopPropagation() but still falls through to commandStore.execute("Comfy.Graph.ExitSubgraph") before the event reaches controls such as SingleSelect/MultiSelect, whose Escape handlers call stopEscapeToDocument() at the target. In a subgraph with an open select, Escape now exits the subgraph and then closes the select; before this PR the target handler could stop the bubble-phase keybinding and only close the select. Could we preserve the old priority for bare Escape, for example by not handling it in the capture listener and letting a bubble-phase path run after target handlers, or otherwise skipping the command when an Escape-consuming overlay is the target?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When pressing Ctrl+Enter to run a job while a dropdown widget was focused, the dropdown would expand. This happened because:
windowin bubble phase — firing after child element handlersonKeyDownfires first and opens the dropdown on anyEnterkeypress regardless of modifier keyspreventDefault(), which doesn't stop other listeners from runningFix:
{ capture: true }) so it fires top-down, before any element-level handlersevent.stopPropagation()when a keybinding is matched, preventing the event from reaching the dropdownRed-Green Verification
test: add failing test for keybinding stopPropagation when dropdown is focusedfix: stop event propagation on keybinding match to prevent dropdown expansionTest Plan
focus a dropdown widget (e.g. KSampler
sampler_name), press Ctrl+Enter → dropdown stays closed, job runsbefore
before11.mov
after
after111.mov
Note
Medium Risk
Changes global
keydownhandling and propagation behavior, which can subtly affect other keyboard interactions across the app. Scope is small and covered by new targeted tests, reducing regression risk.Overview
Prevents UI controls (e.g. dropdowns) from also handling keys when a global keybinding fires by stopping event propagation on matched shortcuts in
keybindingService.keybindHandler.Registers the window
keydownlistener in capture phase inGraphView.vueso keybindings run before component-level handlers. Adds a new Vitest suite verifying propagation is stopped forCtrl+Enterwhen a non-input element is focused, and not stopped when no binding matches or when typing in text inputs.Reviewed by Cursor Bugbot for commit 426d8a6. Bugbot is set up for automated code reviews on this repo. Configure here.
┆Issue is synchronized with this Notion page by Unito