Rename upgradePackages to needsPackageUpdate #1546
Conversation
Change from ValidationIssueState to boolean for clearer intent: - upgradePackages?: ValidationIssueState -> needsPackageUpdate?: boolean - The field is a flag for auto-update, not a validation issue state
dosubot
bot
added
the
size:XS
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the validation interface to improve code clarity by changing the upgradePackages field to needsPackageUpdate and converting it from a ValidationIssueState enum to a simple boolean type, better reflecting its purpose as a flag for auto-update rather than a validation issue state.
Changes:
- Renamed
upgradePackagestoneedsPackageUpdatein theInstallValidationinterface - Changed the field type from
ValidationIssueStatetoboolean - Updated all references throughout the codebase
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/preload.ts | Updated InstallValidation interface to replace upgradePackages?: ValidationIssueState with needsPackageUpdate?: boolean and added documentation comment |
| src/main-process/comfyInstallation.ts | Updated field references in the getter method and validation logic to use the new boolean field name and compare against true |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** `true` if Manager needs toml and uv to be installed, otherwise `false`. */ | ||
| get needsRequirementsUpdate() { | ||
| return this.validation.upgradePackages === 'warning'; | ||
| return this.validation.needsPackageUpdate === true; |
There was a problem hiding this comment.
The explicit comparison to true is redundant for a boolean field. Since needsPackageUpdate is now typed as boolean, you can simplify this to return this.validation.needsPackageUpdate ?? false; or return !!this.validation.needsPackageUpdate; for better idiomatic TypeScript.
| return this.validation.needsPackageUpdate === true; | |
| return this.validation.needsPackageUpdate ?? false; |
Test Evidence CheckIf this PR modifies behavior that requires testing, a test explanation is required. PRs lacking applicable test explanations may not be reviewed until added. Please add test explanations to ensure code quality and prevent regressions. If this PR changes user-facing behavior, visual proof (screen recording or screenshot) is required. PRs without applicable visual documentation may not be reviewed until provided. You can add it by:
|
benceruleanlu
force-pushed
the
fix/venv-troubleshooting-upgrade
branch
from
c26acd8 to
663d77a
Compare
benceruleanlu
force-pushed
the
fix/venv-troubleshooting-upgrade
branch
from
6122436 to
73d552c
Compare
Change from
ValidationIssueStatetobooleanfor clearer intent:upgradePackages?: ValidationIssueState→needsPackageUpdate?: booleanThis makes the code more readable -
'warning'was a confusing value for what is essentially a boolean trigger.┆Issue is synchronized with this Notion page by Unito