refactor: core extension on-demand import #6675
Conversation
LittleSound
requested review from
brucew4yn3rp,
jtydhr88 and
trsommer
as code owners
dosubot
bot
added
the
size:M
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 11/14/2025, 01:47:36 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: 11/14/2025, 01:37:18 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.01 MB (baseline 2.94 MB) • 🔴 +70.9 kBMain entry bundles and manifests
Status: 32 added / 4 removed Graph Workspace — 799 kB (baseline 799 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8 kB (baseline 8 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 306 kB (baseline 306 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 266 kB (baseline 266 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 12.6 kB (baseline 12.6 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 3 added / 3 removed Utilities & Hooks — 5.87 kB (baseline 5.87 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 3 added / 3 removed Other — 3.93 MB (baseline 3.92 MB) • 🔴 +7.65 kBBundles that do not match a named category
Status: 16 added / 15 removed |
jtydhr88
left a comment
jtydhr88
left a comment
There was a problem hiding this comment.
I made a quick test on my local, the model list dropdown on vueNodes for load3d doesn't work after this refactor:
but it is working well before this refactor
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,73 @@ | |||
| import type { ComfyExtension } from '@/types' | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-function-type | |||
There was a problem hiding this comment.
[quality] medium Priority
Issue: Using eslint-disable comment to suppress TypeScript function type rule rather than providing proper type annotation
Context: The line disables the no-unsafe-function-type rule which is designed to catch potentially unsafe function type usage
Suggestion: Replace with explicit Function type annotation or use more specific function signature type
| extension.config | ||
| ) | ||
| activationEvents.forEach((event) => | ||
| onceExtImportEvent(event, async () => void (await extension.entry())) |
There was a problem hiding this comment.
[architecture] high Priority
Issue: No error handling for extension entry point execution in dispatchComfyExtensions
Context: Line 30 uses void operator to suppress Promise rejection warnings, but doesn't handle actual errors that could crash the extension loading system
Suggestion: Add proper error handling with try-catch and logging to prevent individual extension failures from affecting the entire system
| export async function importExtensionsByEvent(event: string) { | ||
| const callbacks = eventMap.get(event) | ||
| if (!callbacks) return | ||
| eventMap.delete(event) |
There was a problem hiding this comment.
[architecture] high Priority
Issue: Race condition in importExtensionsByEvent - event map is deleted before callbacks complete execution
Context: Line 42 deletes the event from the map immediately, but callbacks are executed asynchronously with Promise.all which could cause issues if the same event is triggered again before completion
Suggestion: Move eventMap.delete(event) to after the Promise.all completes to prevent race conditions
| await Promise.all([...callbacks].map((cb) => cb({ event }))) | ||
| } | ||
|
|
||
| export function extentionsImportEventHas(event: string) { |
There was a problem hiding this comment.
[quality] low Priority
Issue: Typo in function name extentionsImportEventHas - missing 's' in 'extensions'
Context: Function name has spelling error which affects code readability and consistency
Suggestion: Rename to extensionsImportEventHas to fix the typo
| const pkgs: ComfyExtensionPackages = {} | ||
| for (const [entryPath, entry] of Object.entries(entrance)) { | ||
| const pathArr = entryPath.split('/') | ||
| const name = pathArr.at(-2)! |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Using non-null assertion with .at(-2) without proper null check
Context: Line 22 uses ! to assert the name exists but .at(-2) can return undefined if array has less than 2 elements
Suggestion: Add proper null check and error handling before using non-null assertion to prevent runtime errors
| import './webcamCapture' | ||
| import './widgetInputs' | ||
| export async function registerExtensions() { | ||
| console.log('importExtensions running...') |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Debug console.log left in production code
Context: Line 5 contains debugging statement that should be removed from production code
Suggestion: Remove the console.log or replace with proper logging system that can be disabled in production
| } | ||
| } | ||
|
|
||
| ;(async () => { |
There was a problem hiding this comment.
[performance] high Priority
Issue: Async IIFE with Promise.all could block widget creation flow and create race conditions
Context: Lines 88-92 create widgets asynchronously while sync widgets are created immediately, potentially causing UI inconsistencies and timing issues
Suggestion: Consider using a more structured approach like a callback system or ensuring proper ordering between sync and async widget creation
|
|
||
| import { useExtensionService } from './extensionService' | ||
| import { | ||
| extentionsImportEventHas, |
There was a problem hiding this comment.
[quality] low Priority
Issue: Same typo in import - extentionsImportEventHas should be extensionsImportEventHas
Context: The typo from dispatch.ts is imported here, propagating the naming inconsistency
Suggestion: Fix the typo when the original function is renamed in dispatch.ts
| import { defineComfyExtConfig } from '@/extensions/utils' | ||
|
|
||
| export default defineComfyExtConfig({ | ||
| name: 'Comfy.SaveGLB', |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Inconsistent naming pattern between config name and contributes name
Context: Config has name 'Comfy.SaveGLB' but contributes section has same name, while other configs like load3d use different names for different contributions
Suggestion: Consider using a more descriptive pattern like 'Comfy.SaveMesh' to match the actual functionality and be consistent with the load3d pattern
| return isCloud | ||
| }, | ||
| get subscriptionRequired() { | ||
| return !!window.__CONFIG__?.subscription_required |
There was a problem hiding this comment.
[security] medium Priority
Issue: Direct access to global window.CONFIG object for subscription validation
Context: Line 14 accesses window.CONFIG?.subscription_required which could be manipulated by client-side code to bypass subscription checks
Suggestion: Ensure subscription validation is also performed server-side and this client-side check is only used for UI behavior, not security enforcement
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: refactor: core extension on-demand import (#6675)
Impact: 540 additions, 125 deletions across 95 files
Issue Distribution
- Critical: 0
- High: 2
- Medium: 6
- Low: 2
Category Breakdown
- Architecture: 4 issues
- Security: 1 issues
- Performance: 1 issues
- Code Quality: 4 issues
Key Findings
Architecture & Design
This PR introduces a significant architectural change implementing on-demand extension loading with configuration-based activation events. The new system replaces synchronous imports with event-driven lazy loading, which should improve initial bundle size and startup time. However, several issues were identified:
- Error Handling: The extension loading system lacks proper error handling, which could cause cascading failures
- Race Conditions: The event dispatch system has a potential race condition where events are deleted before callbacks complete
- Widget Loading Flow: The async widget loading implementation could create UI inconsistencies
Security Considerations
The security review identified one medium-priority concern regarding client-side subscription validation. The system relies on window.CONFIG.subscription_required which could be manipulated by client-side code. This should only be used for UI behavior, not actual security enforcement.
Performance Impact
The performance analysis found one high-priority issue with the widget loading system using async IIFEs that could block the UI creation flow. The mixing of synchronous and asynchronous widget creation could lead to timing issues and inconsistent user experience.
Integration Points
The new system integrates well with the existing Vite build system using import.meta.glob for dynamic extension discovery. The configuration-based approach provides good separation of concerns between extension metadata and implementation.
Positive Observations
- Clean Architecture: The new config-based approach provides clear separation between extension metadata and implementation
- Build Optimization: Using eager imports for configs and lazy imports for extensions should improve bundle splitting
- Type Safety: Strong TypeScript typing throughout the new extension system
- Cloud Integration: Thoughtful handling of cloud-only extensions with proper conditional loading
References
- Repository Architecture Guide: https://github.com/Comfy-Org/comfy-claude-prompt-library/blob/master/project-summaries-for-agents/ComfyUI_frontend/REPOSITORY_GUIDE.md
- Vue 3 Composition API Best Practices: https://vuejs.org/guide/extras/composition-api-faq.html
- Vite Dynamic Imports: https://vitejs.dev/guide/features.html#dynamic-import
Next Steps
- Address high-priority race condition and performance issues before merge
- Add comprehensive error handling to extension loading system
- Fix naming typos throughout the codebase
- Consider adding integration tests for the new extension loading system
- Update documentation to reflect the new extension architecture
This is a comprehensive automated review focusing on architecture, security, performance, and code quality. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
DrJKL
force-pushed
the
rizumu/refactor/on-demand-loading-extension
branch
from
ee4ed1b to
40dd82c
Compare
|
Hi @DrJKL . what changes did you make? |
Just rebasing onto main |
Why. Did something happen to the main branch? |
A healthy number of merges today, including some fixes for some of the tests, yeah |
|
Understood. Thank you. |
This pull request implements an on-demand import system for core extensions, with initial adaptations for Three.js-related extensions.
Overview
The new system introduces a configuration-based approach to manage extension loading, replacing the previous synchronous import-all pattern with an event-driven lazy loading mechanism.
Key Features
1. Configuration-Based Extension Loading
Each extension now defines its loading behavior through a
comfy.ext.config.tsfile:2. Event-Driven Loading System
Extensions are loaded on-demand based on activation events:
*- Load immediately on startuponWidgets:contributes- Load when widget contributions are neededonCommands:contributes- Load when command contributions are neededonSettings:contributes- Load when settings contributions are needed3. Automatic Extension Discovery
Using Vite's
import.meta.glob, the system automatically discovers extensions without maintaining manual import lists:Benefits
Adapted Extensions
The following extensions have been migrated to the new system:
load3d- 3D model loading and preview (Three.js-dependent)saveMesh- 3D mesh export functionalitycloudBadges- Cloud subscription badgescloudRemoteConfig- Remote configuration pollingcloudSessionCookie- Cloud session managementcloudSubscription- Subscription featurescloudFeedbackTopbarButton- Feedback buttonFuture Work
Technical Details
import: 'default'for optimal tree-shakingimport()┆Issue is synchronized with this Notion page by Unito