Fix venv troubleshooting package check #1536
Conversation
dosubot
bot
added
the
size:S
📝 WalkthroughWalkthroughReplaced string-based hasRequirements returns with a structured Changes
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (7)**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
tests/**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
tests/unit/**/*.test.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/unit/**/*.{test,spec}.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
🧠 Learnings (22)📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-11-25T20:49:50.649ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
📚 Learning: 2025-12-18T19:46:11.878ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-12-18T19:46:11.878ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
📚 Learning: 2025-11-25T20:50:17.635ZApplied to files:
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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 PR fixes the virtual environment troubleshooting logic to properly treat unknown or missing packages as errors rather than package upgrades, ensuring the maintenance UI correctly shows the install-packages task when a broken venv is detected.
Changes:
- Modified
hasRequirements()to return'error'instead of'package-upgrade'for unknown/missing packages - Updated log message to be more descriptive: "Requirements missing beyond known upgrade cases"
- Updated JSDoc comment for better clarity on return value meanings
- Updated unit tests to reflect the new behavior and expectations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/virtualEnvironment.ts | Changed return value from 'package-upgrade' to 'error' for unknown packages, updated log message and documentation |
| tests/unit/virtualEnvironment.test.ts | Updated test expectations and descriptions to match new behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
first green ci of 2026 |
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
treat unknown/missing packages as error in hasRequirements so maintenance UI shows install‑packages task after a broken venv
Returning 'error' instead of 'package-upgrade' is what does this? Seems unintuitive.
dosubot
bot
added
size:M
it's internal naming for hasRequirements. I do agree it is unintuitive. I'll change it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/virtualEnvironment.ts`:
- Around line 61-66: The public type RequirementsCheckResult currently has an
unconstrained reason?: string; tighten this by introducing a literal union type
(e.g., export type RequirementsCheckReason = 'missing' | 'upgrade-required' |
'incompatible' | 'error' | 'unknown';) and replace reason?: string with reason?:
RequirementsCheckReason in the RequirementsCheckResult declaration; update any
code that sets or checks result.reason to use those exact literals (or map other
messages into them) so the shape is discoverable and type-safe, and update any
tests or switch/case branches that inspect reason to the new literals.
In `@tests/unit/virtualEnvironment.test.ts`:
- Around line 215-220: The test title "returns upgrade for manager upgrade case"
is duplicated; rename this test to a unique descriptive name (e.g. include
"manager" vs "pip" or add "(manager upgrade)" suffix) to make failures
distinguishable—update the test declaration string in
tests/unit/virtualEnvironment.test.ts (the test that calls mockSpawnOutputOnce,
invokes virtualEnv.hasRequirements(), and asserts on log.info) so the new title
clearly differentiates it from the other test with the same name.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/main-process/comfyInstallation.tssrc/virtualEnvironment.tstests/unit/virtualEnvironment.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,mts,cts}: Use features available in TypeScript 5.x
Use ESM only (type: module). Avoid CommonJS (require,module.exports)
Target ESNext runtime APIs and syntax. Prefer top-levelawaitonly when it improves clarity
Code must compile withstrictmode andnoImplicitAnyenabled, with zero implicitany
Useunknowninstead ofany. Narrow with type guards.anyis allowed only when interfacing with untyped code and must be localized and justified
Useinterfacefor public object shapes intended for extension/implementation
Usetypefor unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g.,kind: 'X' | 'Y'). Use exhaustiveswitchstatements
Preferas constobjects and string/number literal unions overenum
Preferreadonlyproperties andReadonlyArray<T>where appropriate. Do not mutate function parameters
PreferT[]overArray<T>for readability
PreferRecord<Key, T>for simple maps; useMap/Setwhen iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Usesatisfiesto enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Useimport type { X } from '…'for type-only imports. Keep value and type imports separated when helpful for clarity and bundling
Exported functions, classes, and public APIs must have explicit parameter and return types. Internal locals can rely on inference
Prefer arrow functions for local functions and callbacks. Use function declarations when hoisting orthisbinding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over rawthen/catchchains. Keep linear flow and usetry/catchfor failures
Eitherawaitpromises or deliberately mark them...
Files:
src/main-process/comfyInstallation.tssrc/virtualEnvironment.tstests/unit/virtualEnvironment.test.ts
src/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Centralize reusable domain types in
src/**where discoverable. Avoid ad-hoc inline types for shared structures
Files:
src/main-process/comfyInstallation.tssrc/virtualEnvironment.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript: Do not use theanytype; useunknownwhen the type is genuinely unknown
Use JSDoc format for method documentation with common tags@paramand@return(omit@returnforvoidreturn type); use{@link}to reference symbols
Files:
src/main-process/comfyInstallation.tssrc/virtualEnvironment.tstests/unit/virtualEnvironment.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
src/main-process/comfyInstallation.tssrc/virtualEnvironment.tstests/unit/virtualEnvironment.test.ts
{src/main.ts,src/preload.ts,src/main-process/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Main process code must maintain clean separation between main process, renderer, and preload scripts; follow Electron security best practices
Files:
src/main-process/comfyInstallation.ts
tests/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Files:
tests/unit/virtualEnvironment.test.ts
tests/unit/**/*.test.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)
tests/unit/**/*.test.{js,ts}: Usevitestfor unit testing
Do not build custom testing infrastructure; use Vitest and existing helpers
Store tests in thetests/unit/directory with directory structure matching that of the tested file insrc/
Tests should be cross-platform compatible; ensure proper use ofpath.normalize,path.join, andpath.sepfor Windows, macOS, and Linux compatibility
Mocks should be cleanly written, easy to understand, and reusable where possible
Prefer the use oftest.extendover loose variables in Vitest tests
Usetestinstead ofitfor defining tests
Files:
tests/unit/virtualEnvironment.test.ts
tests/unit/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Files:
tests/unit/virtualEnvironment.test.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Leverage TestEnvironment for state manipulation and error simulation instead of manual setup
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Check CI behavior with process.env.CI in integration tests
Applied to files:
tests/unit/virtualEnvironment.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
🔇 Additional comments (15)
src/main-process/comfyInstallation.ts (1)
186-195: Status handling aligns with the new requirements result.This matches the new structured status semantics and correctly maps upgrade/missing paths to UI flags.
tests/unit/virtualEnvironment.test.ts (8)
149-150: Parametrized expectations updated correctly.The status-object assertion matches the new return shape.
183-188: Updated OK-path assertion looks good.Matches the new
{ status: 'ok' }contract.
191-199: Missing-package assertion and log expectation are consistent.Covers the unknown-package path with the new status shape.
202-207: Manager-upgrade case updated correctly.Status assertion aligns with the new result contract.
228-233: Core+manager upgrade assertion is aligned.Status and log expectations reflect the new behavior.
236-241: Core-upgrade case updated correctly.The new
{ status: 'upgrade' }expectation is consistent.
255-260: stderr-handling assertion matches the new result type.Good coverage for the stderr-only output case.
262-267: Unknown-removal case mapped tomissingcorrectly.Matches the new missing-status behavior.
src/virtualEnvironment.ts (6)
335-339: Status check update is correct.Using
requirementsStatus.statuspreserves behavior with the new type.
895-901: Signature/docs now match the new result shape.The updated return documentation is consistent.
972-978: Known-upgrade return path is consistent.The new structured return matches the logged branch.
980-988: Missing-package branch maps correctly tomissing.The structured return and log details are consistent.
989-993: NVIDIA-torch upgrade status is consistent with new type.Return value aligns with the updated contract.
995-996: OK-path return updated correctly.No behavior change beyond the new result shape.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/virtualEnvironment.test.ts (1)
202-226: Test names still need differentiation.Line 202 tests "(uv/toml)" case and Line 215 tests the same scenario with slightly different packages but shares an almost identical name "returns upgrade for manager upgrade case". While one was renamed to include "(uv/toml)", the other at line 215 should also be differentiated for clarity when interpreting test failures.
♻️ Suggested rename for line 215
- test('returns upgrade for manager upgrade case', async ({ virtualEnv }) => { + test('returns upgrade for manager upgrade case (uv + toml legacy)', async ({ virtualEnv }) => {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/virtualEnvironment.tstests/unit/install/installationManager.test.tstests/unit/virtualEnvironment.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,mts,cts}: Use features available in TypeScript 5.x
Use ESM only (type: module). Avoid CommonJS (require,module.exports)
Target ESNext runtime APIs and syntax. Prefer top-levelawaitonly when it improves clarity
Code must compile withstrictmode andnoImplicitAnyenabled, with zero implicitany
Useunknowninstead ofany. Narrow with type guards.anyis allowed only when interfacing with untyped code and must be localized and justified
Useinterfacefor public object shapes intended for extension/implementation
Usetypefor unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g.,kind: 'X' | 'Y'). Use exhaustiveswitchstatements
Preferas constobjects and string/number literal unions overenum
Preferreadonlyproperties andReadonlyArray<T>where appropriate. Do not mutate function parameters
PreferT[]overArray<T>for readability
PreferRecord<Key, T>for simple maps; useMap/Setwhen iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Usesatisfiesto enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Useimport type { X } from '…'for type-only imports. Keep value and type imports separated when helpful for clarity and bundling
Exported functions, classes, and public APIs must have explicit parameter and return types. Internal locals can rely on inference
Prefer arrow functions for local functions and callbacks. Use function declarations when hoisting orthisbinding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over rawthen/catchchains. Keep linear flow and usetry/catchfor failures
Eitherawaitpromises or deliberately mark them...
Files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.tssrc/virtualEnvironment.ts
tests/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
tests/unit/**/*.test.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)
tests/unit/**/*.test.{js,ts}: Usevitestfor unit testing
Do not build custom testing infrastructure; use Vitest and existing helpers
Store tests in thetests/unit/directory with directory structure matching that of the tested file insrc/
Tests should be cross-platform compatible; ensure proper use ofpath.normalize,path.join, andpath.sepfor Windows, macOS, and Linux compatibility
Mocks should be cleanly written, easy to understand, and reusable where possible
Prefer the use oftest.extendover loose variables in Vitest tests
Usetestinstead ofitfor defining tests
Files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript: Do not use theanytype; useunknownwhen the type is genuinely unknown
Use JSDoc format for method documentation with common tags@paramand@return(omit@returnforvoidreturn type); use{@link}to reference symbols
Files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.tssrc/virtualEnvironment.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.tssrc/virtualEnvironment.ts
tests/unit/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
src/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Centralize reusable domain types in
src/**where discoverable. Avoid ad-hoc inline types for shared structures
Files:
src/virtualEnvironment.ts
🧠 Learnings (22)
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Leverage TestEnvironment for state manipulation and error simulation instead of manual setup
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to tests/**/*.{ts,mts,cts} : Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Applied to files:
tests/unit/virtualEnvironment.test.tssrc/virtualEnvironment.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Check CI behavior with process.env.CI in integration tests
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Tests should be cross-platform compatible; ensure proper use of `path.normalize`, `path.join`, and `path.sep` for Windows, macOS, and Linux compatibility
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Prefer the use of `test.extend` over loose variables in Vitest tests
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Applies to tests/integration/test*.ts : Test helper classes should contain clean methods and properties for use in spec files to improve code readability
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Use `vitest` for unit testing
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Add screenshots for visual regression testing in integration tests
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Do not use non-null assertions or `ts-ignore`. Use `ts-expect-error` with an explanation when unavoidable
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-12-18T19:46:11.878Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T19:46:11.878Z
Learning: Applies to tests/unit/**/*.{test,spec}.ts : Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Use `test` instead of `it` for defining tests
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Run `yarn test:unit` before committing changes
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-12-18T19:46:11.878Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T19:46:11.878Z
Learning: Applies to tests/integration/**/*.{test,spec}.ts : Use Playwright for E2E testing; configure tests in `tests/integration/` with subdirectories for `install/`, `post-install/`, and `shared/`
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Use Playwright + TypeScript for Electron testing in integration tests
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Mock native dialogs when needed using app.app.evaluate() with electron.dialog overrides
Applied to files:
tests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Import fixtures from testExtensions.ts, not raw Playwright
Applied to files:
tests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Use fixture classes (TestApp, TestInstallWizard, TestInstalledApp, TestServerStart, TestTroubleshooting, TestGraphCanvas) instead of raw locators
Applied to files:
tests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Use exhaustive `switch` statements over unions and assert on the `never` case (e.g., via an `assertUnreachable` helper)
Applied to files:
src/virtualEnvironment.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Prefer `as const` objects and string/number literal unions over `enum`
Applied to files:
src/virtualEnvironment.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Use `satisfies` to enforce object literal conformance without widening values
Applied to files:
src/virtualEnvironment.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Use `unknown` instead of `any`. Narrow with type guards. `any` is allowed only when interfacing with untyped code and must be localized and justified
Applied to files:
src/virtualEnvironment.ts
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Use `type` for unions, mapped/conditional types, function types, and when composition is needed
Applied to files:
src/virtualEnvironment.ts
🔇 Additional comments (10)
src/virtualEnvironment.ts (3)
61-68: Well-structured type definitions for the requirements check result.The literal union types for
RequirementsCheckStatusandRequirementsCheckReasonare correctly defined, following the coding guidelines to prefer literal unions over enums. The optionalreasonfield appropriately provides additional context for non-OK statuses.
337-344: Call site correctly updated to use structured result.The code now properly accesses
requirementsStatus.statusto determine behavior. The conditional logic is sound—only proceeding to skip installation when status is'ok'.
977-999: Return statements properly structured with status and reason.The return paths are clear and well-documented:
{ status: 'upgrade', reason: 'known-packages' }for recognized package upgrades{ status: 'missing', reason: 'unknown-packages' }for unrecognized missing packages{ status: 'upgrade', reason: 'nvidia-torch' }for NVIDIA torch upgrades{ status: 'ok' }when all requirements are satisfiedThe logging statements provide good observability for each case.
tests/unit/virtualEnvironment.test.ts (6)
145-152: Parameterized test correctly updated for new result shape.The test properly computes the expected status based on package counts and uses
toMatchObjectto verify the structured result. This covers the combinatorial cases efficiently.
183-189: OK status test correctly verifies structured result.The assertion
toMatchObject({ status: 'ok' })properly validates the new return shape while allowing for optional fields.
191-200: Test name and assertions properly reflect 'missing' status semantics.The test name "returns missing when packages are missing and not a known upgrade case" accurately describes the scenario, and the log expectation correctly matches the updated log message.
228-241: Combined core + manager upgrade test is clear and correct.The test properly verifies the upgrade status when both core and manager packages need updates.
255-260: Stderr handling test properly updated.The test verifies that stderr output is correctly processed and the status is determined from combined stdout/stderr.
262-267: Rejection test correctly expects 'missing' status.When unrecognized packages are being removed, the test correctly expects
{ status: 'missing' }rather than an upgrade path.tests/unit/install/installationManager.test.ts (1)
85-98: Mock correctly updated to return structured RequirementsCheckResult.The mock now returns
{ status: 'ok' }instead of a boolean, properly aligning with the updatedhasRequirementsreturn type. This ensures the installation manager tests work correctly with the new API contract.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
This PR has evolved, I now want to fully refactor this python package upgrading system. Taking draft until ready. |
Summary
hasRequirementsok/upgrade/missingstatuses to install validation states without changing UI behaviorStory
A few months back when packages were missing from the hardcoded allowlist we would prompt users to update instead of doing it automatically. I changed it to do it automatically, and forgot to update the test. This PR updates the test and internal naming, making it a bit more clear to developers what is going on.
The next PR should probably get rid of the hardcoded allowlist to just let the app autoupdate python dependencies, reviewers LMK WYT. I think we could change it back to manual as a beneficial change if the allowlist was derived dynamically from requirements.txt of core and manager.