-
Notifications
You must be signed in to change notification settings - Fork 390
[fix] Enable AUDIO_RECORD widget in both LiteGraph and Vue nodes modes #6094
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
Fixed the AUDIO_RECORD widget to display correctly in both rendering modes: - Removed conflicting AUDIO_RECORD registration from ComfyWidgets that was blocking the custom widget implementation in uploadAudio.ts extension - Changed canvasOnly flags from true to false on both audioUIWidget and recordWidget to enable Vue nodes rendering - Added type override (recordWidget.type = 'audiorecord') after widget creation to enable Vue component lookup while preserving LiteGraph button rendering - Removed unused IAudioRecordWidget type definition The widget now works correctly: - LiteGraph mode: Displays as a functional button - Vue nodes mode: Displays full recording UI with waveform visualization
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/18/2025, 09:26:32 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 10/18/2025, 09:41:04 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
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.
How about a test that just loads a workflow with a single record audio node and take a screenshot?
Good idea |
|
|
||
| recordWidget.label = t('g.startRecording') | ||
| // Override the type for Vue rendering while keeping 'button' for LiteGraph | ||
| recordWidget.type = 'audiorecord' |
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.
[architecture] medium Priority
Issue: Type override hack using recordWidget.type = 'audiorecord' after widget creation
Context: While this solves the immediate problem, it creates a maintenance concern by bypassing the formal widget type system
Suggestion: Consider registering 'audiorecord' as a proper button widget variant or creating a formal audio record widget type in the widget registry
| SELECTBUTTON: transformWidgetConstructorV2ToV1(useSelectButtonWidget()), | ||
| TEXTAREA: transformWidgetConstructorV2ToV1(useTextareaWidget()), | ||
| AUDIO_RECORD: transformWidgetConstructorV2ToV1(useAudioRecordWidget()) | ||
| TEXTAREA: transformWidgetConstructorV2ToV1(useTextareaWidget()) |
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.
[architecture] high Priority
Issue: Removing formal AUDIO_RECORD widget registration entirely from ComfyWidgets
Context: This creates an inconsistency where AUDIO_RECORD widgets exist but aren't formally registered in the widget system, potentially causing issues with widget discovery and type checking
Suggestion: Either register AUDIO_RECORD as a proper widget type or document why this specific widget type needs to bypass the formal registration system
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.
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: [fix] Enable AUDIO_RECORD widget in both LiteGraph and Vue nodes modes (#6094)
Impact: 5 additions, 44 deletions across 5 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 2
- Low: 2
Category Breakdown
- Architecture: 3 issues
- Security: 0 issues
- Performance: 0 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
The PR takes a pragmatic but architecturally concerning approach to fixing the AUDIO_RECORD widget by:
- Type Override Pattern: Uses recordWidget.type = 'audiorecord' after widget creation to enable Vue rendering while preserving LiteGraph functionality
- Widget Registration Removal: Completely removes formal AUDIO_RECORD widget registration from the ComfyWidgets system
- Schema Cleanup: Removes AUDIORECORD input specifications and related TypeScript types
While this approach solves the immediate rendering issue, it creates technical debt by bypassing the formal widget type system. The solution works but doesn't follow established patterns used by other widget types.
Security Considerations
No security issues were identified in this PR. The changes are limited to widget rendering behavior and don't introduce new attack vectors.
Performance Impact
The changes should have minimal performance impact:
- Enabling Vue rendering for audioUIWidget and recordWidget may have slight overhead
- Removing unused schema types and composables reduces bundle size marginally
- Overall performance impact is negligible
Integration Points
Breaking Changes Potential: Removing AudioRecordInputSpec types could break external code that references these types.
Extension Compatibility: The type override approach means AUDIO_RECORD widgets exist in the system but aren't discoverable through standard widget registration mechanisms.
Positive Observations
- Targeted Fix: The PR focuses specifically on the reported issue without over-engineering
- Backward Compatibility: LiteGraph button rendering is preserved while enabling Vue nodes
- Clean Removals: Unused code and schema definitions are properly removed
- Functional Solution: The approach successfully enables the widget in both rendering modes
References
Next Steps
- Address the high-priority widget registration consistency issue
- Consider formalizing the 'audiorecord' widget type or documenting the exception
- Add documentation for the canvasOnly changes and type override pattern
- Consider providing migration path for removed AudioRecordInputSpec types
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
|
Updating Playwright Expectations |
af17a5d to
dc0c8b7
Compare
Bundle Size ReportApp Entry PointsMain application bundles
Category Total: 11.7 MB Core ViewsMajor application views and screens
Category Total: 721 kB UI PanelsSettings and configuration panels
Category Total: 74.8 kB ServicesBusiness logic and services
Category Total: 10 kB UtilitiesHelper functions and utilities
Category Total: 1.07 kB OtherUncategorized bundles
Category Total: 1.12 kB Overall Total Size: 12.5 MB |
Fixed the AUDIO_RECORD widget to display correctly in both rendering modes:
The widget now works correctly:
Summary
Changes
Review Focus
Screenshots (if applicable)
Screen.Recording.2025-10-16.at.22.29.59.mov
┆Issue is synchronized with this Notion page by Unito