Skip to content

Conversation

Myestery
Copy link
Collaborator

@Myestery Myestery commented Oct 9, 2025

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 (getCanvasMenuItems and getNodeMenuItems) 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

  • Introduced a new API for extensions to contribute context menu items via getCanvasMenuItems and getNodeMenuItems methods, replacing legacy monkey-patching of LGraphCanvas.prototype.getCanvasMenuOptions. Major extension files (groupNode.ts, groupOptions.ts, nodeTemplates.ts) now use this new API. [1] [2] [3]
  • Added a compatibility layer (legacyMenuCompat in contextMenuCompat.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

  • Refactored core extensions (groupNode, groupOptions, and nodeTemplates) 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

  • Updated imports for context menu types (IContextMenuValue) across affected files for consistency with the new API. [1] [2] [3] [4]

Backward Compatibility and Migration Guidance

  • The compatibility layer logs a deprecation warning to the console when legacy monkey-patching is detected, helping developers migrate to the new API.

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

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.
Copy link

github-actions bot commented Oct 9, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 10/20/2025, 06:29:18 PM UTC

🔗 Links


🎉 Your Storybook is ready for review!

Copy link

github-actions bot commented Oct 9, 2025

🎭 Playwright Test Results

Some tests failed

⏰ Completed at: 10/20/2025, 06:47:44 PM UTC

📈 Summary

  • Total Tests: 484
  • Passed: 436 ✅
  • Failed: 14 ❌
  • Flaky: 3 ⚠️
  • Skipped: 31 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 427 / ❌ 14 / ⚠️ 3 / ⏭️ 31
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

christian-byrne pushed a commit that referenced this pull request Oct 10, 2025
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
arjansingh pushed a commit that referenced this pull request Oct 12, 2025
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
arjansingh pushed a commit that referenced this pull request Oct 13, 2025
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
@Myestery Myestery marked this pull request as ready for review October 14, 2025 02:08
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Oct 14, 2025
@christian-byrne
Copy link
Contributor

Is this ready for review or still in progress?

@Myestery
Copy link
Collaborator Author

Is this ready for review or still in progress?

its ready

@christian-byrne
Copy link
Contributor

I'm not able to make the code work. Right-clicking the canvas seems to throw an error.

@Myestery
Copy link
Collaborator Author

I'm not able to make the code work. Right-clicking the canvas seems to throw an error.

Okay let me check that

Copy link

Bundle Size Report

Summary

  • Raw size: 12.5 MB baseline 12.5 MB — 🔴 +3.14 kB
  • Gzip: 2.54 MB baseline 2.54 MB — 🔴 +1.15 kB
  • Brotli: 2 MB baseline 2 MB — 🔴 +1.05 kB
  • Bundles: 56 current • 56 baseline • 12 added / 12 removed

Category Glance
App Entry Points 🔴 +2.62 kB (3.31 MB) · Graph Workspace 🔴 +514 B (708 kB) · Vendor & Third-Party ⚪ 0 B (5.36 MB) · Other ⚪ 0 B (2.79 MB) · Panels & Settings ⚪ 0 B (294 kB) · UI Components ⚪ 0 B (12.3 kB) · + 3 more

Per-category breakdown
App Entry Points — 3.31 MB (baseline 3.31 MB) • 🔴 +2.62 kB _Main entry bundles and manifests_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------- | ------- | ------ | ----------------------- | ---------------------- | ----------------------- | | **assets/index-p1rAzl9C.js** _(new)_ | — | 2.7 MB | 🔴 +2.7 MB | 🔴 +562 kB | 🔴 +425 kB | | ~~assets/index-CaV8pXdw.js~~ _(removed)_ | 2.69 MB | — | 🟢 -2.69 MB | 🟢 -561 kB | 🟢 -424 kB | | ~~assets/index-Ct219RwX.js~~ _(removed)_ | 616 kB | — | 🟢 -616 kB | 🟢 -114 kB | 🟢 -90.3 kB | | **assets/index-CVNqoNaW.js** _(new)_ | — | 614 kB | 🔴 +614 kB | 🔴 +114 kB | 🔴 +90.2 kB |

Status: 2 added / 2 removed

Graph Workspace — 708 kB (baseline 708 kB) • 🔴 +514 B _Graph editor runtime, canvas, workflow orchestration_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | -------------------------------------------- | ------ | ------ | ---------------------- | ---------------------- | ---------------------- | | **assets/GraphView-Jkh4Fa7r.js** _(new)_ | — | 708 kB | 🔴 +708 kB | 🔴 +139 kB | 🔴 +107 kB | | ~~assets/GraphView-D2a0FYNg.js~~ _(removed)_ | 708 kB | — | 🟢 -708 kB | 🟢 -139 kB | 🟢 -107 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-Cgp7opb2.js** _(new)_ | — | 8.15 kB | 🔴 +8.15 kB | 🔴 +2.46 kB | 🔴 +2.16 kB | | ~~assets/UserSelectView-CON6J0Dz.js~~ _(removed)_ | 8.15 kB | — | 🟢 -8.15 kB | 🟢 -2.46 kB | 🟢 -2.16 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-BiIXIaCO.js~~ _(removed)_ | 22.1 kB | — | 🟢 -22.1 kB | 🟢 -5.28 kB | 🟢 -4.62 kB | | **assets/CreditsPanel-YosUV0RJ.js** _(new)_ | — | 22.1 kB | 🔴 +22.1 kB | 🔴 +5.28 kB | 🔴 +4.61 kB | | **assets/KeybindingPanel-BJTe3S29.js** _(new)_ | — | 15.2 kB | 🔴 +15.2 kB | 🔴 +3.75 kB | 🔴 +3.31 kB | | ~~assets/KeybindingPanel-DxWtfA9o.js~~ _(removed)_ | 15.2 kB | — | 🟢 -15.2 kB | 🟢 -3.75 kB | 🟢 -3.31 kB | | **assets/ExtensionPanel-05AuQkZW.js** _(new)_ | — | 12.1 kB | 🔴 +12.1 kB | 🔴 +2.83 kB | 🔴 +2.48 kB | | ~~assets/ExtensionPanel-D6OwTUa-.js~~ _(removed)_ | 12.1 kB | — | 🟢 -12.1 kB | 🟢 -2.82 kB | 🟢 -2.47 kB | | ~~assets/AboutPanel-DcPdiTGc.js~~ _(removed)_ | 10.3 kB | — | 🟢 -10.3 kB | 🟢 -2.65 kB | 🟢 -2.33 kB | | **assets/AboutPanel-DzMdIC3_.js** _(new)_ | — | 10.3 kB | 🔴 +10.3 kB | 🔴 +2.65 kB | 🔴 +2.33 kB | | **assets/ServerConfigPanel-D8I2-DC4.js** _(new)_ | — | 8.2 kB | 🔴 +8.2 kB | 🔴 +2.16 kB | 🔴 +1.9 kB | | ~~assets/ServerConfigPanel-Y8ifmC5j.js~~ _(removed)_ | 8.2 kB | — | 🟢 -8.2 kB | 🟢 -2.16 kB | 🟢 -1.9 kB | | ~~assets/UserPanel-BEL1Cna3.js~~ _(removed)_ | 7.91 kB | — | 🟢 -7.91 kB | 🟢 -2.06 kB | 🟢 -1.8 kB | | **assets/UserPanel-lOjAGdGn.js** _(new)_ | — | 7.91 kB | 🔴 +7.91 kB | 🔴 +2.05 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-8KODZbqh.js~~ _(removed)_ | 11.1 kB | — | 🟢 -11.1 kB | 🟢 -2.76 kB | 🟢 -2.44 kB | | **assets/ComfyQueueButton-iJSFq2y-.js** _(new)_ | — | 11.1 kB | 🔴 +11.1 kB | 🔴 +2.76 kB | 🔴 +2.43 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-CBKQpWli.js** _(new)_ | — | 7.21 kB | 🔴 +7.21 kB | 🔴 +1.75 kB | 🔴 +1.5 kB | | ~~assets/keybindingService-D5APnKo7.js~~ _(removed)_ | 7.21 kB | — | 🟢 -7.21 kB | 🟢 -1.75 kB | 🟢 -1.5 kB | | assets/serverConfigStore-B1bw4Pe0.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-kaNE-JGc.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-DKA7Hxfn.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-QImF2beP.js | 92.4 kB | 92.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |
Other — 2.79 MB (baseline 2.79 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-6UgCUkrV.js | 108 kB | 108 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-BfHN1fzx.js | 125 kB | 125 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-BhulUfFD.js | 77.5 kB | 77.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-BPHe683n.js | 92.4 kB | 92.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-C75C4LWt.js | 90.9 kB | 90.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CEhf19j2.js | 99.4 kB | 99.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CipazGd8.js | 79.3 kB | 79.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CKz_lTAz.js | 94.3 kB | 94.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-Dzm38Va4.js | 90.3 kB | 90.3 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 |

Copy link
Contributor

@DrJKL DrJKL left a 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?

...args
)
for (const item of legacyItems) {
// @ts-expect-error - Generic types differ but runtime compatibility is ensured
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you enforce this in the return type of extractLegacyItems so that you can avoid the compiler check suppression?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth doing defense in depth when it comes to extensions. A lot of the runtime errors come from missing nullish checks when things leak through.

// Add items from new extension API
const newApiItems = app.collectCanvasMenuItems(this)
for (const item of newApiItems) {
// @ts-expect-error - Generic types differ but runtime compatibility is ensured
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as the one below.

setup() {
addConvertToGroupOptions()

getCanvasMenuItems(_canvas): IContextMenuValue[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the unused parameter?

!!selected.find((n) => GroupNodeHandler.isGroupNode(n))

items.push({
content: `Convert to Group Node`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but this seems like it should go through i18n, right?

Comment on lines +1732 to +1734
const convertDisabled =
selected.length < 2 ||
!!selected.find((n) => GroupNodeHandler.isGroupNode(n))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: This logic is in two places, but with different names.

Comment on lines +23 to +26
setCurrentExtension(extensionName: string | null) {
this.currentExtension = extensionName
}

Copy link
Contributor

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?

if (!ENABLE_LEGACY_SUPPORT) return []

// Prevent infinite recursion - if we're already extracting, return empty
if (this.isExtracting) return []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would lead to this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this lead to missing menus from some extensions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically when monkeypatching, the function itself is called in the definition of the patch

private originalMethods = new Map<string, AnyFunction>()
private hasWarned = new Set<string>()
private currentExtension: string | null = null
private isExtracting = false
Copy link
Contributor

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.

}
currentImpl = newImpl
},
configurable: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking: Does this need to be configurable?

Comment on lines +147 to +149
const items1 = extensionService
.invokeExtensions('getCanvasMenuItems', mockCanvas)
.flat() as IContextMenuValue[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work?

Suggested change
const items1 = extensionService
.invokeExtensions('getCanvasMenuItems', mockCanvas)
.flat() as IContextMenuValue[]
const items1: IContextMenuValue[] = extensionService
.invokeExtensions('getCanvasMenuItems', mockCanvas)
.flat()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants