[bugfix] Fix Vue nodes combo widgets not displaying deserialized values #6545
Conversation
Fixes an issue where combo widgets in Vue nodes would not display values from deserialized workflows if those values were not in the current options list (e.g., deleted model files, removed checkpoints). This brings Vue nodes behavior in line with legacy canvas rendering, which always displays the current value regardless of whether it exists in the options.
dosubot
bot
added
the
size:L
🎨 Storybook Build Status❌ Build failed! ⏰ Completed at: 11/03/2025, 02:33:29 AM UTC 🔗 Links
|
🎭 Playwright Test Results🕵🏻 No test results found ⏰ Completed at: 11/03/2025, 02:35:54 AM UTC 📊 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.27 MB (baseline 3.27 MB) • 🔴 +502 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 725 kB (baseline 725 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 — 295 kB (baseline 295 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 11.4 kB (baseline 11.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
|
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ): T[] { | ||
| // Early return for empty/null values | ||
| if (currentValue == null || currentValue === '') { | ||
| return [...options] |
There was a problem hiding this comment.
[performance] medium Priority
Issue: The ensureValueInOptions function creates new arrays on every computed execution, even when no changes occur
Context: In both WidgetSelectDefault.vue and WidgetSelectDropdown.vue, the computed selectOptions/inputItems will recreate arrays on every re-render, causing unnecessary memory allocations
Suggestion: Consider memoizing the result or adding a check to avoid array recreation when values haven't changed
| currentValue: T | undefined | null | ||
| ): T[] { | ||
| // Early return for empty/null values | ||
| if (currentValue == null || currentValue === '') { |
There was a problem hiding this comment.
[quality] low Priority
Issue: The empty string check may not be appropriate for numeric types
Context: The function handles both string and number types but checks for empty string ('') even for numeric values, which could cause unexpected behavior for 0 values
Suggestion: Consider separating the empty checks by type: '(currentValue == null || (typeof currentValue === "string" && currentValue === ""))'
| const result = ensureValueInOptions(options, 99) | ||
|
|
||
| expect(result).toEqual([99, 1, 2, 3]) | ||
| }) |
There was a problem hiding this comment.
[quality] low Priority
Issue: Test coverage missing for numeric 0 edge case
Context: The tests verify numeric values but don't test the edge case where currentValue is 0, which might be treated as falsy in the empty check condition
Suggestion: Add test case: 'it("handles numeric zero value", () => { const result = ensureValueInOptions([1,2,3], 0); expect(result).toEqual([0,1,2,3]); })'
| // Should not duplicate the value | ||
| expect(selectOptions).toEqual(options) | ||
| expect( | ||
| selectOptions.filter((opt: string) => opt === 'option2') |
There was a problem hiding this comment.
[quality] low Priority
Issue: Type annotation added for TS7006 fix but inconsistent application across codebase
Context: The recent fix added explicit type annotation 'as string[]' but this pattern should be consistent with other similar lines in the file
Suggestion: Consider applying the same type annotation pattern to line 211 'selectOptions.filter((opt: string) => opt === "option2")' for consistency
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: [bugfix] Fix Vue nodes combo widgets not displaying deserialized values (#6545)
Impact: 194 additions, 2 deletions across 5 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 1
- Low: 3
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 1 issues
- Code Quality: 3 issues
Key Findings
Architecture & Design
The PR follows Vue 3 Composition API patterns correctly and maintains consistency with existing widget architecture. The new ensureValueInOptions utility is appropriately placed and follows the project's separation of concerns. The solution elegantly addresses the deserialized workflow value display issue by preserving legacy behavior while maintaining Vue component reactivity.
Security Considerations
No security vulnerabilities identified. The function properly handles input validation and type checking without introducing XSS or injection risks.
Performance Impact
The main performance concern is the repeated array creation in computed properties. While functional and correct, the ensureValueInOptions function creates new arrays on every computed execution, which could cause unnecessary memory allocations during frequent re-renders.
Integration Points
The changes maintain backward compatibility and correctly integrate with existing Vue widget infrastructure. The solution preserves the behavior users expect when loading workflows with missing assets (deleted models, removed files) by displaying the deserialized values even when not in current options lists.
Positive Observations
- Comprehensive test coverage with excellent edge case handling
- Clean, readable implementation with clear documentation
- Proper TypeScript typing with generic constraints
- Follows established patterns for Vue widget components
- Maintains immutability by returning new arrays instead of mutating inputs
- Good separation of concerns with utility function extraction
- Excellent commit message explaining the problem and solution
References
- Repository Architecture Guide
- Frontend Standards
- Security Guidelines
Next Steps
- Address the performance optimization for array recreation if needed
- Consider the type safety improvement for numeric zero handling
- Add the suggested test case for numeric zero edge case
- Verify consistent TypeScript annotation patterns
This is a comprehensive automated review. The PR successfully addresses the stated issue with high code quality. The implementation is solid and the minor suggestions are optimizations rather than blocking issues.
github-actions
bot
removed
the
claude-review
|
All 7 Playwright test failures are due to the widget now showing the value from the de-serialized data (how litegraph works). |
…odifying options Simplified the approach to show deserialized workflow values that are not in the current options list. Instead of adding missing values to the options array, we now display them as the placeholder text. This is cleaner, avoids array manipulation, and addresses performance concerns from code review.
dosubot
bot
added
size:M
Refactored ApproachI've simplified the implementation based on the suggestion. Instead of modifying the options array to include deserialized values, we now display them as the placeholder text when they're not in the current options list. Changes:
This maintains the legacy behavior while being cleaner and more performant. |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
|
Replaced by #6555 |
Fixes an issue where combo widgets in Vue nodes would not display values from deserialized workflows if those values were not in the current options list (e.g., deleted model files, removed checkpoints). This brings Vue nodes behavior in line with legacy canvas rendering, which always displays the current value regardless of whether it exists in the options list.
┆Issue is synchronized with this Notion page by Unito