Rework desktop install / startup UX #5292
Conversation
🎭 Playwright Test Results⏰ Completed at: 09/24/2025, 07:16:37 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
webfiltered
force-pushed
the
app-startup-stage-tracking
branch
from
3bb3c1a to
a97a439
Compare
webfiltered
force-pushed
the
app-startup-stage-tracking
branch
5 times, most recently
from
6810750 to
941c774
Compare
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
webfiltered
force-pushed
the
app-startup-stage-tracking
branch
from
8850e38 to
19d3c4b
Compare
webfiltered
requested review from
Yorha4D,
KarryCharon,
shinshin86 and
a team
as code owners
dosubot
bot
added
the
size:XXL
webfiltered
force-pushed
the
app-startup-stage-tracking
branch
from
46f0373 to
ec04f84
Compare
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.
Review Summary
PR: Rework desktop install / startup UX (#5292)
Impact: 1633 additions, 630 deletions across 23 files
Issue Distribution
- Critical: 1
- High: 3
- Medium: 5
- Low: 2
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 1 issue
- Code Quality: 6 issues
Key Findings
Critical Issues
- TypeScript Error Suppression (src/views/InstallView.vue:156): Using @ts-expect-error to suppress TypeScript error instead of fixing the underlying type issue. This violates project standards - fix the type assertion properly.
High Priority Issues
-
Missing Input Validation (src/components/install/InstallLocationPicker.vue): Path validation relies only on client-side checks without proper sanitization before sending to Electron backend.
-
Missing Type Definitions (src/components/install/HardwareOption.vue): Component uses TorchDeviceType but interface definition could be more explicit for extension compatibility.
-
Hardcoded Asset Paths (src/components/install/GpuPicker.vue:24): Asset paths like /assets/images/nvidia-logo-square.jpg are hardcoded without proper asset management or fallback handling.
Medium Priority Issues
-
Large Component Size: InstallLocationPicker.vue is quite large (226+ lines) and handles multiple concerns - consider breaking into smaller, focused components.
-
Inline Styles in Template (src/components/install/HardwareOption.vue:22): Using style="object-position: 57% center" inline instead of CSS classes violates the Tailwind-first approach.
-
Missing i18n Coverage: Some text strings may not be properly internationalized, particularly error messages and dynamic content.
-
Console Event Tracking: Multiple electronAPI().Events.trackEvent calls without error handling or privacy considerations.
-
Regex in Computed Property (src/views/InstallView.vue:110): Hard-coded regex for disk space validation should be configurable or use constants.
Low Priority Issues
-
CSS Deep Selectors: Multiple :deep() selectors suggest component encapsulation could be improved.
-
Magic Numbers: Various hardcoded dimensions and spacing values that should use design system tokens.
Architecture & Design
Positive Observations
- Good use of Vue 3 Composition API patterns
- Proper component separation with clear responsibilities
- Effective use of PrimeVue components
- Good TypeScript adoption overall
- Proper use of Tailwind utility classes in most places
- Well-structured stepper flow with proper state management
Areas for Improvement
- Some components are quite large and handle multiple responsibilities
- Asset management could be more robust
- Error handling could be more comprehensive
Security Considerations
- Path validation needs server-side validation in addition to client-side checks
- Asset loading should have proper error handling and fallbacks
- Event tracking should consider privacy implications
Performance Impact
- New images added to bundle (nvidia-logo-square.jpg, comfy-brand-mark.svg) - ensure they are optimized
- Component lazy loading could be considered for the install flow
- Bundle size impact appears minimal but should be monitored
References
- Repository Architecture Guide: https://github.com/Comfy-Org/comfy-claude-prompt-library/blob/master/project-summaries-for-agents/ComfyUI_frontend/REPOSITORY_GUIDE.md
- ComfyUI Frontend Guidelines: CLAUDE.md
Next Steps
- Critical: Fix TypeScript error suppression in InstallView.vue
- High: Add proper input validation and error handling
- Medium: Consider component refactoring for better maintainability
- Low: Clean up inline styles and magic numbers
This is a comprehensive automated review. The PR shows good architectural patterns overall with room for improvement in error handling and type safety.
christian-byrne
added
needs-backport
Ugh what is it with Claude and this line. This makes 3 Claude reviews telling me to fix something that wasn't even touched by this PR.
Should probably add a note on this. I'm fine with double-handling validation in env. with real frontend/backend separation, but this is not that.
Considered it. A lot. WWYD? Next...
This falls under the new (stretch) goal: It needs to be changed for that anyway. No point doing it twice. |
🧐 Sounds like a plan. Will ping tomorrow as the split also came up in planning. To reduce review fatigue / ensure changes are reviewed in isolation, I'm merging this and will add some small fixes in a follow-up PR. |
|
SGTM |
|
@webfiltered Backport to Please manually cherry-pick commit Conflicting files
|
Complete redesign of Desktop app installer, implementing the new app startup progress system and error reporting.
## Summary Backporting #5292 to core/1.27 branch to fix desktop installation and startup UX issues. ## What's Changed This is a manual backport of commit b0f81b2 which reworks the desktop install and startup user experience. ### Merge Conflicts Resolved The automatic backport failed with conflicts in: - `src/components/install/GpuPicker.vue` - Resolved by keeping the changes made in the PR - `src/components/install/MirrorsConfiguration.vue` - Resolved by keeping the file deletion from the PR - `src/components/install/mirror/MirrorItem.vue` - Resolved by combination merge (all changes) - `src/views/ServerStartView.vue` - Resolved by combination merge (all changes) ## Original PR - PR: #5292 - Commit: b0f81b2 - Title: Rework desktop install / startup UX ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5756-Backport-1-27-Rework-desktop-install-startup-UX-5292-2786d73d365081668f4dc16694c51185) by [Unito](https://www.unito.io)
## What's Changed ### 🚀 Features - add pricing for new api nodes (#5725) - Rework desktop install / startup UX (#5292) ### 🐛 Bug Fixes - Make experiment asset api setting hidden (#5861) - Only add the listeners for DomWidget components once (#5849) - fix invalid JSON (contains merge conflict markers) in 1.27 RC branch's locale json (#5839) - fix: Status indicator and close button appearing together (#5741) - Fix cyclic prototype errors with subgraphNodes (#5650) - fix: don't immediately close missing nodes dialog if manager is disabled (#5649) - Fix SaveAs (#5644) ### 🔧 Maintenance - Add JSON validation CI workflow (#5838) - Cherry-pick manager flag refactor to core/1.27 (#5646) - Cherry-pick desktop dialogs framework to core/1.27 (#5634) **Full Changelog**: v1.27.5...v1.27.7 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5862-1-27-7-27e6d73d3650819aa712e682f6b88077) by [Unito](https://www.unito.io)
Summary
Complete redesign of Desktop app installer, implementing the new app startup progress system and error reporting.
Review Focus
I would like to put some limitations on the review scope, if that is acceptable. This PR explicitly does not attempt to refactor / fix existing bad logic that is simply being moved or added to. The project identifies it as explicitly out of scope.
Some code has been moved between files (e.g. mirror config), looking like it is new, but really is just refactored enough to continue functioning exactly as it did before, within the new design.
Adding nits to every apparent change would take an excessive amount of time, and produce a lot of noise that could be addressed outside of this PR.
Screenshots
Video
0: Welcome
1: GPU Selection
2: Install Location
3: Online Behaviour
┆Issue is synchronized with this Notion page by Unito