-
Notifications
You must be signed in to change notification settings - Fork 378
Contextmenu Monkeypatch Standardization #5977
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
Conversation
🎭 Playwright Test Results⏰ Completed at: 10/10/2025, 01:15:48 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/10/2025, 12:59:22 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
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
4d23f7b
to
3bc9bcc
Compare
…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)
src/scripts/app.ts
Outdated
collectNodeMenuItems(node: LGraphNode): IContextMenuValue[] { | ||
const items: IContextMenuValue[] = [] | ||
|
||
for (const ext of this.extensions) { |
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.
Same here.
let mockCanvas: LGraphCanvas | ||
let mockNode: LGraphNode |
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.
nit: Could you use a factory to make these per-testcase instead of keeping them in the describe scope? That could also add some type safety via Partial
Also, I don't love things being called mock that aren't using the mocking system, but that's a lesser issue.
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.
What do you think about using the same pattern as the selecton toolbox API
ComfyUI_frontend/src/components/graph/SelectionToolbox.vue
Lines 87 to 101 in f78cf05
const extensionToolboxCommands = computed<ComfyCommandImpl[]>(() => { | |
const commandIds = new Set<string>( | |
canvasStore.selectedItems | |
.map( | |
(item) => | |
extensionService | |
.invokeExtensions('getSelectionToolboxCommands', item) | |
.flat() as string[] | |
) | |
.flat() | |
) | |
return Array.from(commandIds) | |
.map((commandId) => commandStore.getCommand(commandId)) | |
.filter((command): command is ComfyCommandImpl => command !== undefined) | |
}) |
comfyApp.extensions
is deprecated and invokeExtensions
is the newer API I would like to be using if possible.
Refactored collectCanvasMenuItems and collectNodeMenuItems to use the standardized invokeExtensions pattern from extensionService, matching the approach used in SelectionToolbox. This simplifies the code and provides consistent error handling across all extension invocations.
- Test getCanvasMenuItems and getNodeMenuItems extension methods - Verify results are collected into flat array from multiple extensions - Add submenu tests with proper submenu.options structure - Support menu separators (null items) - Test extensions without menu methods are skipped - Use extensionService.invokeExtensions directly - Configure Pinia with stubActions: false
## What's Changed ### 🚀 Features - Implement DOMWidget for vue (#6006) - Implement drop-on-canvas + linkconnectoradapter consolidation (#5898) - Vuenodes/audio widgets (#5627) - Allow reordering of linked subgraph widgets (#5981) - Contextmenu Monkeypatch Standardization (#5977) - Fix/vue nodes snap to grid (#5973) - Select Vue Nodes After Drag (#5863) - fix Vue node widgets should be in disabled state if their slots are connected with a link (#5834) ### 🐛 Bug Fixes - [bugfix] Fix update-playwright-expectations workflow missing frontend build (#6005) - Fix: Reset size when collapsing (#6004) - fix: misc LOD polish (#6001) - Fix: Allow uncoloring Vue Nodes (#5991) - [ci] Fix detached HEAD state in Playwright update workflow (#5985) - Close zoom menu when toggling minimap visibility (#5974) ### 🔧 Maintenance - Devex: Improve dev server (#6002) - CI: Add concurrency checks to PR workflows (#6000) - [feat] Auto-remove New Browser Test Expectations label after workflow completes (#5998) - CI: Simplify update playwright expectations (maybe) (#5994) - Lint: Add tailwind linter (#5984) - [feat] Auto-remove claude-review label after CI review completes (#5983) - Fix CI: Remove explicit repository parameter causing non-reproducible test results (#5950) - refactor: Reorganize GitHub Actions for better reusability (#5949) - devex: Update CODEOWNERS (#5999) - Docs: Update agent instructions about style classes (#5990) - Style: Fix move cursors that should be grabs (#5989) - Workflow templates review (#5975) **Full Changelog**: v1.29.0...v1.29.1 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6012-1-29-1-2896d73d365081b08418f46934651c41) by [Unito](https://www.unito.io) Co-authored-by: arjansingh <[email protected]>
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
andgetNodeMenuItems
methods to theComfyExtension
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
, andLGraphNode
(src/types/comfy.ts
,src/scripts/app.ts
). [1] [2]Core Implementation:
collectCanvasMenuItems
andcollectNodeMenuItems
methods in theComfyApp
class to gather menu items from all extensions, with robust error handling and logging for extension failures (src/scripts/app.ts
).Testing:
tests-ui/tests/extensions/contextMenuExtension.test.ts
).This is PR 1 of the 3 PRs in the Contextmenu standardizations.
-#5992
-#5993