-
Notifications
You must be signed in to change notification settings - Fork 390
Contextmenu extension migration #5993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Introduces a new extension API that allows extensions to provide context menu items directly, without monkey-patching. This provides a clean, type-safe way for extensions to add menu items. **New API methods:** - `getCanvasMenuItems(canvas)`: Add items to canvas right-click menus - `getNodeMenuItems(node)`: Add items to node right-click menus **Implementation:** - Added TypeScript interfaces in `src/types/comfy.ts` - Added collection methods in `ComfyApp` class - Comprehensive test coverage for the new API
…extensions Adds a compatibility layer that detects and supports legacy extensions using the monkey-patching pattern, while warning developers about the deprecated approach. **Features:** - Automatic detection of monkey-patched context menu methods - Console warnings with extension name for deprecated patterns - Extraction and integration of legacy menu items - Extension tracking during setup for accurate warnings **Files:** - `src/lib/litegraph/src/contextMenuCompat.ts`: Core compatibility logic - `src/services/extensionService.ts`: Extension name tracking - `src/composables/useContextMenuTranslation.ts`: Integration layer - Comprehensive test coverage Depends on PR #5977 (context menu extension API)
Migrates core extensions from monkey-patching to the new context menu extension API, demonstrating the migration path for extension developers. **Migrated extensions:** - `groupOptions.ts`: Group management context menu items - `nodeTemplates.ts`: Template save/load context menu items **Legacy compatibility test:** - `groupNode.ts`: Kept with monkey-patching as compatibility test case - Includes documentation explaining the old vs new approach **Benefits:** - Type-safe menu item registration - No prototype pollution - Easier to test and maintain - Clear separation of concerns Depends on PR #5977 (context menu extension API)
Completes the migration by converting the groupNode extension from monkey-patching to the new context menu API. **Changes:** - Removed `addConvertToGroupOptions()` function and monkey-patching - Implemented `getCanvasMenuItems()` method - Implemented `getNodeMenuItems()` method - Removed `setup()` hook (no longer needed) All core extensions now use the standardized API with no monkey-patching.
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/24/2025, 07:10:49 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 10/24/2025, 07:27:12 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
This pull request introduces a new extension API for context menu customization, allowing extensions to contribute items to both canvas and node right-click menus. It adds two collection methods to the `ComfyApp` class to aggregate these menu items from all registered extensions, and updates the extension interface accordingly. Comprehensive unit tests are included to verify the correct aggregation behavior and error handling. **Extension API for Context Menus:** * Added optional `getCanvasMenuItems` and `getNodeMenuItems` methods to the `ComfyExtension` interface, enabling extensions to provide context menu items for canvas and node right-click menus (`src/types/comfy.ts`). * Updated type imports to support the new API, including `IContextMenuValue`, `LGraphCanvas`, and `LGraphNode` (`src/types/comfy.ts`, `src/scripts/app.ts`). [[1]](diffhunk://#diff-c29886a1b0c982c6fff3545af0ca8ec269876c2cf3948f867d08c14032c04d66L1-R5) [[2]](diffhunk://#diff-bde0dce9fe2403685d27b0e94a938c3d72824d02d01d1fd6167a0dddc6e585ddR10) **Core Implementation:** * Implemented `collectCanvasMenuItems` and `collectNodeMenuItems` methods in the `ComfyApp` class to gather menu items from all extensions, with robust error handling and logging for extension failures (`src/scripts/app.ts`). **Testing:** * Added a comprehensive test suite for the new context menu extension API, covering aggregation logic, error handling, and integration scenarios (`tests-ui/tests/extensions/contextMenuExtension.test.ts`). This is PR 1 of the 3 PRs in the Contextmenu standardizations. -#5992 -#5993
This pull request introduces a new extension API for context menu customization, allowing extensions to contribute items to both canvas and node right-click menus. It adds two collection methods to the `ComfyApp` class to aggregate these menu items from all registered extensions, and updates the extension interface accordingly. Comprehensive unit tests are included to verify the correct aggregation behavior and error handling. **Extension API for Context Menus:** * Added optional `getCanvasMenuItems` and `getNodeMenuItems` methods to the `ComfyExtension` interface, enabling extensions to provide context menu items for canvas and node right-click menus (`src/types/comfy.ts`). * Updated type imports to support the new API, including `IContextMenuValue`, `LGraphCanvas`, and `LGraphNode` (`src/types/comfy.ts`, `src/scripts/app.ts`). [[1]](diffhunk://#diff-c29886a1b0c982c6fff3545af0ca8ec269876c2cf3948f867d08c14032c04d66L1-R5) [[2]](diffhunk://#diff-bde0dce9fe2403685d27b0e94a938c3d72824d02d01d1fd6167a0dddc6e585ddR10) **Core Implementation:** * Implemented `collectCanvasMenuItems` and `collectNodeMenuItems` methods in the `ComfyApp` class to gather menu items from all extensions, with robust error handling and logging for extension failures (`src/scripts/app.ts`). **Testing:** * Added a comprehensive test suite for the new context menu extension API, covering aggregation logic, error handling, and integration scenarios (`tests-ui/tests/extensions/contextMenuExtension.test.ts`). This is PR 1 of the 3 PRs in the Contextmenu standardizations. -#5992 -#5993
This pull request introduces a new extension API for context menu customization, allowing extensions to contribute items to both canvas and node right-click menus. It adds two collection methods to the `ComfyApp` class to aggregate these menu items from all registered extensions, and updates the extension interface accordingly. Comprehensive unit tests are included to verify the correct aggregation behavior and error handling. **Extension API for Context Menus:** * Added optional `getCanvasMenuItems` and `getNodeMenuItems` methods to the `ComfyExtension` interface, enabling extensions to provide context menu items for canvas and node right-click menus (`src/types/comfy.ts`). * Updated type imports to support the new API, including `IContextMenuValue`, `LGraphCanvas`, and `LGraphNode` (`src/types/comfy.ts`, `src/scripts/app.ts`). [[1]](diffhunk://#diff-c29886a1b0c982c6fff3545af0ca8ec269876c2cf3948f867d08c14032c04d66L1-R5) [[2]](diffhunk://#diff-bde0dce9fe2403685d27b0e94a938c3d72824d02d01d1fd6167a0dddc6e585ddR10) **Core Implementation:** * Implemented `collectCanvasMenuItems` and `collectNodeMenuItems` methods in the `ComfyApp` class to gather menu items from all extensions, with robust error handling and logging for extension failures (`src/scripts/app.ts`). **Testing:** * Added a comprehensive test suite for the new context menu extension API, covering aggregation logic, error handling, and integration scenarios (`tests-ui/tests/extensions/contextMenuExtension.test.ts`). This is PR 1 of the 3 PRs in the Contextmenu standardizations. -#5992 -#5993
|
Is this ready for review or still in progress? |
its ready |
|
I'm not able to make the code work. Right-clicking the canvas seems to throw an error. |
Okay let me check that |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.29 MB (baseline 3.29 MB) • 🔴 +3.17 kB_Main entry bundles and manifests_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------- | ------- | ------- | ----------------------- | ---------------------- | ----------------------- | | **assets/index-DePU7lXW.js** _(new)_ | — | 2.68 MB | 🔴 +2.68 MB | 🔴 +557 kB | 🔴 +422 kB | | ~~assets/index-Bmbgidu2.js~~ _(removed)_ | 2.68 MB | — | 🟢 -2.68 MB | 🟢 -556 kB | 🟢 -421 kB | | ~~assets/index-B-aay2HB.js~~ _(removed)_ | 614 kB | — | 🟢 -614 kB | 🟢 -114 kB | 🟢 -90.1 kB | | **assets/index-BavvOtwb.js** _(new)_ | — | 613 kB | 🔴 +613 kB | 🔴 +114 kB | 🔴 +90 kB |Status: 2 added / 2 removed Graph Workspace — 714 kB (baseline 713 kB) • 🔴 +1.07 kB_Graph editor runtime, canvas, workflow orchestration_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | -------------------------------------------- | ------ | ------ | ---------------------- | ---------------------- | ---------------------- | | **assets/GraphView-DHr0F4TK.js** _(new)_ | — | 714 kB | 🔴 +714 kB | 🔴 +140 kB | 🔴 +108 kB | | ~~assets/GraphView-BQu7TvL_.js~~ _(removed)_ | 713 kB | — | 🟢 -713 kB | 🟢 -140 kB | 🟢 -108 kB |Status: 1 added / 1 removed Views & Navigation — 8.15 kB (baseline 8.15 kB) • ⚪ 0 B_Top-level views, pages, and routed surfaces_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/UserSelectView-D-DTHlC0.js** _(new)_ | — | 8.15 kB | 🔴 +8.15 kB | 🔴 +2.47 kB | 🔴 +2.16 kB | | ~~assets/UserSelectView-DawphGZS.js~~ _(removed)_ | 8.15 kB | — | 🟢 -8.15 kB | 🟢 -2.47 kB | 🟢 -2.15 kB |Status: 1 added / 1 removed Panels & Settings — 294 kB (baseline 294 kB) • ⚪ 0 B_Configuration panels, inspectors, and settings screens_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/CreditsPanel-BjVb5pKF.js** _(new)_ | — | 22.1 kB | 🔴 +22.1 kB | 🔴 +5.28 kB | 🔴 +4.61 kB | | ~~assets/CreditsPanel-DzRoZr5x.js~~ _(removed)_ | 22.1 kB | — | 🟢 -22.1 kB | 🟢 -5.28 kB | 🟢 -4.6 kB | | **assets/KeybindingPanel-jAne56fF.js** _(new)_ | — | 15.2 kB | 🔴 +15.2 kB | 🔴 +3.77 kB | 🔴 +3.31 kB | | ~~assets/KeybindingPanel-Otkwl45J.js~~ _(removed)_ | 15.2 kB | — | 🟢 -15.2 kB | 🟢 -3.76 kB | 🟢 -3.31 kB | | ~~assets/ExtensionPanel-BT2N9Yco.js~~ _(removed)_ | 12.1 kB | — | 🟢 -12.1 kB | 🟢 -2.83 kB | 🟢 -2.48 kB | | **assets/ExtensionPanel-CQE-DFl_.js** _(new)_ | — | 12.1 kB | 🔴 +12.1 kB | 🔴 +2.83 kB | 🔴 +2.48 kB | | ~~assets/AboutPanel-BbtR4v-_.js~~ _(removed)_ | 10.3 kB | — | 🟢 -10.3 kB | 🟢 -2.66 kB | 🟢 -2.32 kB | | **assets/AboutPanel-DvClxgar.js** _(new)_ | — | 10.3 kB | 🔴 +10.3 kB | 🔴 +2.66 kB | 🔴 +2.32 kB | | ~~assets/ServerConfigPanel-DC5b9OPz.js~~ _(removed)_ | 8.2 kB | — | 🟢 -8.2 kB | 🟢 -2.17 kB | 🟢 -1.89 kB | | **assets/ServerConfigPanel-nfgozpCg.js** _(new)_ | — | 8.2 kB | 🔴 +8.2 kB | 🔴 +2.17 kB | 🔴 +1.9 kB | | ~~assets/UserPanel-CJxDkvso.js~~ _(removed)_ | 7.91 kB | — | 🟢 -7.91 kB | 🟢 -2.06 kB | 🟢 -1.79 kB | | **assets/UserPanel-t6c-T5MY.js** _(new)_ | — | 7.91 kB | 🔴 +7.91 kB | 🔴 +2.06 kB | 🔴 +1.79 kB | | assets/settings-B-df0dZe.js | 20.7 kB | 20.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-CI6OKvJn.js | 22.9 kB | 22.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-CXGVj_nD.js | 24.5 kB | 24.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DfQ6dSJj.js | 31.6 kB | 31.6 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DJ2QgDzm.js | 25.2 kB | 25.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DRNLPMG6.js | 23.7 kB | 23.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DVVycxDc.js | 19.9 kB | 19.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-G6Dybj1b.js | 24.1 kB | 24.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-M6_GZccG.js | 26 kB | 26 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 6 added / 6 removed UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 B_Reusable component library chunks_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ----------------------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | ~~assets/ComfyQueueButton-1e-mP3q_.js~~ _(removed)_ | 11.1 kB | — | 🟢 -11.1 kB | 🟢 -2.75 kB | 🟢 -2.44 kB | | **assets/ComfyQueueButton-qbwbkAiz.js** _(new)_ | — | 11.1 kB | 🔴 +11.1 kB | 🔴 +2.76 kB | 🔴 +2.44 kB | | assets/UserAvatar.vue_vue_type_script_setup_true_lang-C9bSkTC5.js | 1.12 kB | 1.12 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 1 added / 1 removed Data & Services — 10 kB (baseline 10 kB) • ⚪ 0 B_Stores, services, APIs, and repositories_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/keybindingService-fuXA4kuO.js** _(new)_ | — | 7.21 kB | 🔴 +7.21 kB | 🔴 +1.75 kB | 🔴 +1.51 kB | | ~~assets/keybindingService-IUN9y5Z1.js~~ _(removed)_ | 7.21 kB | — | 🟢 -7.21 kB | 🟢 -1.75 kB | 🟢 -1.51 kB | | assets/serverConfigStore-CvX_HMTJ.js | 2.79 kB | 2.79 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 B_Helpers, composables, and utility bundles_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/mathUtil-CTARWQ-l.js | 1.07 kB | 1.07 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Vendor & Third-Party — 5.36 MB (baseline 5.36 MB) • ⚪ 0 B_External libraries and shared vendor chunks_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/vendor-other-EuL9bHKm.js | 3.22 MB | 3.22 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-primevue-PESgPnbc.js | 517 B | 517 B | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-tiptap-DY0_3CMM.js | 232 kB | 232 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-visualization-BEfdbjRw.js | 1.82 MB | 1.82 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-vue-Di9L6lvm.js | 92.4 kB | 92.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Other — 2.58 MB (baseline 2.58 MB) • ⚪ 0 B_Bundles that do not match a named category_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/commands-B2KZRBmX.js | 15.1 kB | 15.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-Bw-ckyga.js | 13.9 kB | 13.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-C_NmM85I.js | 13.8 kB | 13.8 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-CuozCW4W.js | 14 kB | 14 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-DGfVUJCR.js | 16.2 kB | 16.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-dOJNDogK.js | 14.5 kB | 14.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-DwiE551e.js | 14.7 kB | 14.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-Fw7mvqSy.js | 13.1 kB | 13.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-FXnO1W4Q.js | 13.2 kB | 13.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-Bgu6_Hvd.js | 59.5 kB | 59.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-Bv0L0qvp.js | 93 kB | 93 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-C3Doz3n_.js | 67.6 kB | 67.6 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-C7eBl607.js | 70.7 kB | 70.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CHiV9ds2.js | 76.4 kB | 76.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CIc79Nts.js | 68.5 kB | 68.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-DK5LmuBm.js | 58.8 kB | 58.8 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-J1nit7cj.js | 66.3 kB | 66.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-W97XgvAQ.js | 80.4 kB | 80.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BePSqkA4.js | 195 kB | 195 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BfT7dJcF.js | 204 kB | 204 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BiAtoiXc.js | 194 kB | 194 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CDfbduPY.js | 219 kB | 219 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CDurg_KW.js | 197 kB | 197 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CE-vG3RG.js | 182 kB | 182 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-DAwVV156.js | 200 kB | 200 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-DexhCMEi.js | 233 kB | 233 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-kTrYLFPK.js | 184 kB | 184 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which custom nodes did you test this with, so I can try it myself?
| setCurrentExtension(extensionName: string | null) { | ||
| this.currentExtension = extensionName | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please find a way to not have to track state mutably like this. Maybe a stack of operations where the mutation is atomic and the extension's identity is an immutable property?
| private originalMethods = new Map<string, AnyFunction>() | ||
| private hasWarned = new Set<string>() | ||
| private currentExtension: string | null = null | ||
| private isExtracting = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that isExtracting is clear, descriptive, and unambiguous in context. But also, I think there's a way to do this without needing to have this semaphore.
…ontext menu item state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the PR to add https://docs.comfy.org/custom-nodes/js/context-menu-migration (the link used in the console.warn)? We can also just make a new page in https://github.com/Comfy-Org/docs/tree/main/custom-nodes/js (following the patterns/style of the existing pages with a brief explanation of how to use the API then some snippets we have verified work -- I usually verify just in devtools console by pasting my snippet)?
This pull request refactors how context menu items are contributed by extensions in the LiteGraph-based canvas. The legacy monkey-patching approach for adding context menu options is replaced by a new, explicit API (
getCanvasMenuItemsandgetNodeMenuItems) for extensions. A compatibility layer is added to support legacy extensions and warn developers about deprecated usage. The changes improve maintainability, extension interoperability, and migration to the new context menu system.Context Menu System Refactor
getCanvasMenuItemsandgetNodeMenuItemsmethods, replacing legacy monkey-patching ofLGraphCanvas.prototype.getCanvasMenuOptions. Major extension files (groupNode.ts,groupOptions.ts,nodeTemplates.ts) now use this new API. [1] [2] [3]legacyMenuCompatincontextMenuCompat.ts) to detect and warn when legacy monkey-patching is used, and to extract legacy-added menu items for backward compatibility. [1] [2]Extension Migration
groupNode,groupOptions, andnodeTemplates) to implement the new context menu API, moving menu item logic out of monkey-patched methods and into explicit extension methods. [1] [2] [3]Type and Import Cleanup
IContextMenuValue) across affected files for consistency with the new API. [1] [2] [3] [4]Backward Compatibility and Migration Guidance
These changes collectively modernize the context menu extension mechanism, improve code clarity, and provide a migration path for legacy extensions.
┆Issue is synchronized with this Notion page by Unito