Prevent partial copy of custom widgets when performing linked promotion on subgraphs #6079
Conversation
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/15/2025, 09:06:02 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/15/2025, 09:21:28 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
dosubot
bot
added
the
size:XS
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| this | ||
| ) | ||
| const promotedWidget = | ||
| widget instanceof BaseWidget |
There was a problem hiding this comment.
[architecture] high Priority
Issue: Type safety concern - no runtime type checking for BaseWidget instanceof check
Context: The instanceof check relies on runtime type information which could fail if widgets are created through different contexts or loaded modules
Suggestion: Add explicit type checking or use duck typing to verify the widget has createCopyForNode method before calling it
| const promotedWidget = | ||
| widget instanceof BaseWidget | ||
| ? widget.createCopyForNode(this) | ||
| : { ...widget, node: this } |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Inconsistent widget copying strategies may lead to different behavior
Context: BaseWidget instances use createCopyForNode() which may have different copying semantics than object spread with node assignment
Suggestion: Document the differences between these approaches or consider unifying the widget copying strategy to ensure consistent behavior
| const promotedWidget = | ||
| widget instanceof BaseWidget | ||
| ? widget.createCopyForNode(this) | ||
| : { ...widget, node: this } |
There was a problem hiding this comment.
[security] low Priority
Issue: Object spread may inadvertently copy unsafe properties from untrusted widgets
Context: The spread operator {...widget} copies all enumerable properties, potentially including functions or references that weren't intended to be cloned
Suggestion: Use a more explicit copying approach that only copies known safe properties, similar to how BaseWidget.constructor already filters out unsafe properties
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: Prevent partial copy of custom widgets when performing linked promotion on subgraphs (#6079)
Impact: 5 additions, 4 deletions across 1 file
Issue Distribution
- Critical: 0
- High: 1
- Medium: 2
- Low: 2
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 0 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
The PR introduces a conditional approach to widget copying that addresses the original issue of restricted widget views being propagated through createCopyForNode. However, it introduces type safety concerns around instanceof checks and potential inconsistencies between the two copying strategies.
The change moves away from the centralized toConcreteWidget function which provided consistent widget transformation logic, potentially fragmenting widget handling across the codebase.
Security Considerations
The object spread operation ({...widget, node: this}) for non-BaseWidget instances could inadvertently copy unsafe properties. The original BaseWidget constructor already has logic to filter out potentially dangerous properties (displayValue, outline_color, etc.), but the spread approach bypasses this safety mechanism.
Performance Impact
No significant performance concerns identified. The instanceof check is a lightweight operation and the elimination of the toConcreteWidget call may actually provide minor performance benefits.
Integration Points
This change affects the subgraph widget promotion system, which is a core part of the nested workflow functionality. The dual copying strategies could lead to subtle differences in behavior between legacy and BaseWidget-derived widgets.
Positive Observations
- The change addresses a real issue with toConcrete creating restricted widget views
- Import cleanup is correctly done (toConcreteWidget is not used elsewhere)
- The solution is minimal and targeted to the specific problem
- Preserves backward compatibility with existing widget types
References
Next Steps
- Address the type safety concerns around instanceof checks
- Consider using duck typing or explicit method checking for better reliability
- Document the differences between the two widget copying strategies
- Consider adding unit tests to verify both code paths work correctly
- Review if similar pattern needs to be applied elsewhere in the codebase
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
|
This PR is only 9 LOC, but there's a lot of history, depth, and complexity to it, haha. |
|
Yeah. Wound up merging the proposed workaround in VHS so there's no rush here. Probably best I do some more digging and add test cases for each of the edge cases that can cause things to break here. |
|
Some more note taking.
|
2 participants
The
toConcretecall creates a restricted view of a widget that extends fromBaseWidget. A copy of the widget created bycreateCopyForNodewill also inherit this restricted view. This creates two problemsdisplayValue) have been judged unsafe and are explicitly blacklisted from being copiedBaseWidget. This results in the widget being processed differently in some logic, such as#processWidgetClickLegacyWidgetprovides an implementation foronClick, the presence of click handlers can not be used to determine which should be used.As a proposed, minimal workaround. Widgets which do not already extend from BaseWidget are no longer cloned through
createCopyForNode.Because this PR involves side-stepping properties which have been explicitly blacklisted. I'd recommend waiting to merge/backport until after the release of 1.28.7
Resolves Kosinkadink/ComfyUI-VideoHelperSuite#569
┆Issue is synchronized with this Notion page by Unito