pass nodeId to widget component #5853
Conversation
🎭 Playwright Test Results⏰ Completed at: 10/01/2025, 12:19:30 AM 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
|
I was actually hoping we could move in the other direction. Right now we're passing large objects around through props instead of just the pieces needed by each component. It makes it tricky to draw clear lines of responsibility and might be part of the rendering performance issues. |
|
@jtydhr88 Could you give a general explanation of why 3D nodes need the nodedata in order to function properly? Once we all fully understand, we could discuss a good solution. From my recollection, 3D nodes are storing state on What other details or requirements are there? |
|
@christian-byrne you are right, the main reason I need this is to store properties settings on Node level, previously I could get node directly but now I need do in the way https://github.com/Comfy-Org/ComfyUI_frontend/pull/5765/files#diff-f421a89c7c66125361b3049ff1cbe1815b3a6a8c24f5ed4e54795c5148d20f9bL218 |
|
@DrJKL for my case, in fact I only need nodeId |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/01/2025, 12:10:14 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
jtydhr88
changed the title
| :widget="widget.simplified" | ||
| :model-value="widget.value" | ||
| :readonly="readonly" | ||
| :node-id="nodeData?.id != null ? String(nodeData.id) : ''" |
There was a problem hiding this comment.
This pattern is everywhere and doesn't make sense - nodeId is defined as optional (string | undefined), yet we're forcing it to empty string for some reason? Is that actually valid?
There was a problem hiding this comment.
yes, I just copy this line from above code in the same file, and follow the same pattern what I see...
There was a problem hiding this comment.
I agree with @webfiltered that we should remove the check and conversion later.
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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: pass nodeId to widget component (#5853)
Impact: 1 additions, 0 deletions across 1 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 0
- Low: 0
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 0 issues
- Code Quality: 0 issues
Key Findings
Architecture & Design
✅ Excellent architectural consistency: The change follows the exact same pattern already established on line 35 for the InputSlot component. The prop passing logic is consistent and predictable.
✅ Proper Vue 3 Composition API usage: The implementation correctly uses optional chaining and appropriate type conversion, following Vue best practices.
✅ Clean separation of concerns: This preparatory change cleanly isolates the node ID prop passing without affecting existing functionality.
Security Considerations
✅ No security concerns identified: The change simply passes a node ID string with proper null checking. No injection vulnerabilities or data exposure risks.
✅ Safe defaults: Uses empty string as fallback when node ID is unavailable, preventing undefined prop issues.
Performance Impact
✅ Minimal performance impact: Adding one computed prop has negligible overhead. The String() conversion and null check are lightweight operations.
✅ No render cycle issues: The change doesn't introduce any reactive dependencies that could cause unnecessary re-renders.
Integration Points
✅ Backward compatibility maintained: Since widget components don't currently expect the node-id prop, this change is fully backward compatible.
✅ Prerequisite architecture: As documented, this change properly sets up the foundation for PR #5765 without introducing breaking changes.
Positive Observations
🎯 Pattern consistency: The implementation is identical to the existing pattern on line 35, showing attention to code consistency.
🔒 Defensive programming: Proper null checking prevents runtime errors in edge cases.
📐 Minimal scope: The change is appropriately scoped - adding exactly what's needed without over-engineering.
🏗️ Future-ready: Positions the codebase well for the upcoming feature in PR #5765.
References
- Repository Architecture Guide
- Vue 3 Composition API best practices
- ComfyUI Frontend coding standards
Next Steps
✅ Ready for merge: No blocking issues identified. The change is clean and follows established patterns.
- This change can be merged as-is - it's a clean preparatory step
- Ensure PR #5765 properly utilizes this new node-id prop in widget components
- Consider documenting the node-id prop in widget component interfaces when implementing #5765
This is a comprehensive automated review. The change is well-structured and follows established patterns with no issues found.
## Summary pass nodeId to widget component, which is a prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5853-pass-nodeData-to-widget-component-27e6d73d3650811e9e48ceb7c3a00545) by [Unito](https://www.unito.io)
## Summary pass nodeId to widget component, which is a prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5853-pass-nodeData-to-widget-component-27e6d73d3650811e9e48ceb7c3a00545) by [Unito](https://www.unito.io)
4 participants
Summary
pass nodeId to widget component, which is a prerequirist for #5765
┆Issue is synchronized with this Notion page by Unito