sync devtools nodes with test ComfyUI dir in test global setup and add dev nodes for all new widget types #6623
Conversation
- Add proper type guards for all widget input specs (Color, TreeSelect, MultiSelect, FileUpload, Galleria) - Enhance schemas with missing properties (format, placeholder, accept, extensions, tooltip) - Fix widgets to honor runtime props like disabled while accessing spec metadata - Eliminate all 'as any' usage in widget components with proper TypeScript types - Clean separation: widget.spec.options for metadata, widget.options for runtime state - Refactor devtools into modular structure with vue_widgets showcase nodes
christian-byrne
changed the title
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/06/2025, 10:25:57 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/06/2025, 10:57:58 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.25 MB (baseline 3.25 MB) • 🔴 +2.28 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 792 kB (baseline 792 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.18 kB (baseline 8.18 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 293 kB (baseline 293 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.6 kB (baseline 12.6 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 10.4 kB (baseline 10.4 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| inputSpec: InputSpec | ||
| ): inputSpec is FileUploadInputSpec => { | ||
| return inputSpec.type === 'FILEUPLOAD' | ||
| } |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Missing type guard for ImageInputSpec - no matching isImageInputSpec function exported
Context: All other input specs have corresponding type guard functions, but ImageInputSpec is missing its type guard
Suggestion: Add export const isImageInputSpec = (inputSpec: InputSpec): inputSpec is ImageInputSpec => inputSpec.type === 'IMAGE'
| @@ -0,0 +1,477 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| SAMPLE_IMAGE_DATA_URI = ( | |||
There was a problem hiding this comment.
[security] medium Priority
Issue: Hard-coded data URI images could pose security risks if served over network
Context: Base64 encoded images are embedded directly in code and could be executed if improperly handled
Suggestion: Consider loading these from secure static assets or validating data URIs before use
| // PrimeVue TreeSelect expects 'options' to be an array of tree nodes | ||
| options: (specOptions.values as TreeNode[]) || [], | ||
| // Convert 'multiple' to PrimeVue's 'selectionMode' | ||
| selectionMode: (specOptions.multiple ? 'multiple' : 'single') as |
There was a problem hiding this comment.
[architecture] high Priority
Issue: Type safety concern with casting selectionMode from boolean to union type
Context: Line 73 uses type assertion to cast boolean to union type which can break at runtime
Suggestion: Use proper conditional logic: selectionMode: specOptions.multiple ? 'multiple' : 'single' (remove the union type assertion)
| // Include runtime props like disabled | ||
| ...props.widget.options, | ||
| // PrimeVue TreeSelect expects 'options' to be an array of tree nodes | ||
| options: (specOptions.values as TreeNode[]) || [], |
There was a problem hiding this comment.
[performance] medium Priority
Issue: Missing memoization for expensive tree node casting
Context: Line 71 performs array casting on every computed update which could be expensive with large tree data
Suggestion: Consider memoizing the tree node transformation or using a more efficient type check
| return | ||
| } | ||
|
|
||
| const devtoolsDest = path.resolve( |
There was a problem hiding this comment.
[security] high Priority
Issue: Directory traversal vulnerability - fs.copySync without path validation
Context: The devtoolsDest path is constructed from user input without validation, potentially allowing directory traversal attacks
Suggestion: Validate and sanitize the targetComfyDir path before using it in file operations. Use path.resolve() and check that the resolved path stays within expected boundaries
| fs.copySync(devtoolsSrc, devtoolsDest, { overwrite: true }) | ||
| console.warn('syncDevtools: copy complete') | ||
| } catch (error) { | ||
| console.error(`Failed to sync DevTools to ${devtoolsDest}:`, error) |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Error logging uses console.error but function continues execution after failure
Context: The function catches errors during file copying but doesn't throw or return error status to caller
Suggestion: Either throw the error to propagate failure or return a boolean indicating success/failure for better error handling
| if (!spec || !isSelectButtonInputSpec(spec)) { | ||
| return [] | ||
| } | ||
| return spec.options?.values || [] |
There was a problem hiding this comment.
[quality] low Priority
Issue: Potential null reference in options extraction
Context: Line 44 accesses spec.options?.values but could return undefined values, not empty array
Suggestion: Use more explicit fallback: return spec.options?.values ?? [] to ensure type safety
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: sync devtools nodes with test ComfyUI dir in test global setup and add dev nodes for all new widget types (#6623)
Impact: 705 additions, 44 deletions across 13 files
Issue Distribution
- Critical: 0
- High: 2
- Medium: 4
- Low: 1
Category Breakdown
- Architecture: 1 issue
- Security: 2 issues
- Performance: 1 issue
- Code Quality: 3 issues
Key Findings
Architecture & Design
The PR introduces comprehensive widget type support with proper schema definitions and type guards. However, there's an inconsistency in the TreeSelect component where type assertions are used instead of proper conditional logic, which could lead to runtime type errors. The overall architectural approach of extending the schema with new widget types is sound and follows established patterns.
Security Considerations
Two security concerns were identified:
- High Priority: Directory traversal vulnerability in devtoolsSync.ts where user input is used to construct file paths without proper validation
- Medium Priority: Hard-coded data URI images in Python files that could pose risks if improperly handled over networks
Performance Impact
The TreeSelect component performs array casting on every computed update without memoization, which could be expensive with large tree datasets. Consider implementing proper memoization for better performance with complex tree structures.
Integration Points
The PR properly extends the widget system architecture and maintains backward compatibility. The devtools synchronization feature integrates well with the test infrastructure. Type safety improvements ensure better developer experience and runtime reliability.
Positive Observations
- Comprehensive schema definitions for all new widget types
- Proper implementation of type guards for most widget specs
- Good separation of concerns between schema validation and component implementation
- Consistent use of Vue 3 Composition API patterns
- Appropriate use of PrimeVue components with proper customization
References
Next Steps
- Address critical issues before merge
- Consider architectural feedback for long-term maintainability
- Add tests for uncovered scenarios
- Update documentation if needed
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
Current issues:
┆Issue is synchronized with this Notion page by Unito