Add dismissible NVIDIA driver warning with versioned suppression #1535
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughExports Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as InstallationManager
participant Config as DesktopConfig
participant Window as AppWindow
participant System as SystemInspector
rect rgba(200,200,255,0.5)
Manager->>Config: get(suppressNvidiaDriverWarningFor)
alt value == NVIDIA_DRIVER_MIN_VERSION
Config-->>Manager: suppressed
Manager-->>System: skip driver warning
else not suppressed
Manager->>System: query GPU vendor & driver version
System-->>Manager: {vendor, deviceType, driverVersion}
Manager->>Manager: shouldWarnAboutNvidiaDriver(context)
alt should warn
Manager->>Window: show warning dialog (with "Don't show again" checkbox)
Window-->>Manager: {userChoice, checkboxChecked}
alt checkboxChecked
Manager->>Config: set(suppressNvidiaDriverWarningFor = NVIDIA_DRIVER_MIN_VERSION)
end
end
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
dosubot
bot
added
size:M
There was a problem hiding this comment.
Pull request overview
This PR adds a dismissible "Don't show again" checkbox to the NVIDIA driver warning dialog, with version-keyed suppression that ensures warnings reappear when the minimum driver version requirement is bumped.
Changes:
- Added a
suppressNvidiaDriverWarningForsetting to track which driver version warning was dismissed - Modified the driver warning dialog to include a dismissible checkbox that persists the suppression
- Added unit tests for the suppression mechanism
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/store/desktopSettings.ts | Added new optional suppressNvidiaDriverWarningFor field to track dismissed warning version |
| src/install/installationManager.ts | Updated warning logic to check suppression status before showing dialog and persist user's dismissal choice |
| tests/unit/install/installationManager.test.ts | Added test coverage for suppression behavior and updated mocks to support new config field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('warnIfNvidiaDriverTooOld', () => { | ||
| const createInstallation = (device: string) => | ||
| ({ virtualEnvironment: { selectedDevice: device } }) as ComfyInstallation; | ||
|
|
||
| const getManagerMethods = () => | ||
| manager as unknown as { | ||
| warnIfNvidiaDriverTooOld: (installation: ComfyInstallation) => Promise<void>; | ||
| getNvidiaDriverVersionFromSmi: () => Promise<string | undefined>; | ||
| getNvidiaDriverVersionFromSmiFallback: () => Promise<string | undefined>; | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| vi.mocked(config.get).mockImplementation((key: string) => { | ||
| if (key === 'installState') return 'installed'; | ||
| if (key === 'basePath') return 'valid/base'; | ||
| return undefined; | ||
| }); | ||
| vi.mocked(config.set).mockClear(); | ||
| vi.mocked(mockAppWindow.showMessageBox).mockClear(); | ||
| }); | ||
|
|
||
| it('skips dialog when warning is suppressed for the current minimum version', async () => { | ||
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | ||
| const managerMethods = getManagerMethods(); | ||
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | ||
| const smiFallbackSpy = vi | ||
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | ||
| .mockResolvedValue(undefined); | ||
|
|
||
| try { | ||
| vi.mocked(config.get).mockImplementation((key: string) => { | ||
| if (key === 'installState') return 'installed'; | ||
| if (key === 'basePath') return 'valid/base'; | ||
| if (key === 'suppressNvidiaDriverWarningFor') return '580'; | ||
| return undefined; | ||
| }); | ||
|
|
||
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | ||
|
|
||
| expect(smiSpy).not.toHaveBeenCalled(); | ||
| expect(smiFallbackSpy).not.toHaveBeenCalled(); | ||
| expect(mockAppWindow.showMessageBox).not.toHaveBeenCalled(); | ||
| } finally { | ||
| smiSpy.mockRestore(); | ||
| smiFallbackSpy.mockRestore(); | ||
| platformSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it('stores the current minimum version when dismissed', async () => { | ||
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | ||
| const managerMethods = getManagerMethods(); | ||
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | ||
| const smiFallbackSpy = vi | ||
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | ||
| .mockResolvedValue(undefined); | ||
|
|
||
| try { | ||
| vi.mocked(mockAppWindow.showMessageBox).mockResolvedValue({ response: 0, checkboxChecked: true }); | ||
|
|
||
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | ||
|
|
||
| expect(config.set).toHaveBeenCalledWith('suppressNvidiaDriverWarningFor', '580'); | ||
| } finally { | ||
| smiSpy.mockRestore(); | ||
| smiFallbackSpy.mockRestore(); | ||
| platformSpy.mockRestore(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test coverage is missing a test case for when the user dismisses the dialog WITHOUT checking the "Don't show again" checkbox. This would verify that the config is not updated when checkboxChecked is false, ensuring the warning appears again on the next trigger. Consider adding a test case that mocks showMessageBox to return checkboxChecked: false and verifies that config.set is not called with 'suppressNvidiaDriverWarningFor'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/install/installationManager.test.ts`:
- Around line 279-326: Replace the two test declarations that use "it(" with
"test(" to follow the repository guideline; specifically change the blocks that
call warnIfNvidiaDriverTooOld (the test titled "skips dialog when warning is
suppressed for the current minimum version" and the one titled "stores the
current minimum version when dismissed") so they use test(...) instead of
it(...), leaving all mocks and assertions involving
getNvidiaDriverVersionFromSmi, getNvidiaDriverVersionFromSmiFallback,
mockAppWindow.showMessageBox, and config.set unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/install/installationManager.tssrc/store/desktopSettings.tstests/unit/install/installationManager.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/store/desktopSettings.tstests/unit/install/installationManager.test.tssrc/install/installationManager.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/store/desktopSettings.tssrc/install/installationManager.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/store/desktopSettings.tstests/unit/install/installationManager.test.tssrc/install/installationManager.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
src/store/desktopSettings.tstests/unit/install/installationManager.test.tssrc/install/installationManager.ts
{src/config/**/*.ts,src/store/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use the existing store patterns for configuration management in
src/store/andsrc/config/
Files:
src/store/desktopSettings.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/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/install/installationManager.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/install/installationManager.test.ts
🧠 Learnings (10)
📚 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-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/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 : Leverage TestEnvironment for state manipulation and error simulation instead of manual setup
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 tests/**/*.{ts,mts,cts} : Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Applied to files:
tests/unit/install/installationManager.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/**/*.spec.ts : Prefer imports from `testExtensions.ts` over Playwright defaults in spec files
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 : Add screenshots for visual regression testing in integration tests
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 Playwright + TypeScript for Electron testing in integration tests
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: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 : Check CI behavior with process.env.CI in integration tests
Applied to files:
tests/unit/install/installationManager.test.ts
🧬 Code graph analysis (1)
src/install/installationManager.ts (1)
src/store/desktopConfig.ts (1)
useDesktopConfig(13-16)
🔇 Additional comments (6)
src/store/desktopSettings.ts (1)
38-39: LGTM!The new optional field is well-documented and follows the existing patterns in the interface. Using a versioned key ensures the warning reappears when the minimum driver version is bumped.
src/install/installationManager.ts (2)
401-402: LGTM - Early return when suppressed for current minimum version.The strict equality check ensures the warning reappears whenever
NVIDIA_DRIVER_MIN_VERSIONis bumped, which aligns with the versioned suppression requirement.
410-422: LGTM - Checkbox handling and persistence.The implementation correctly:
- Adds the "Don't show again" checkbox to the dialog
- Only persists the suppression when the user explicitly checks the box
- Stores the minimum version (not a boolean), enabling automatic re-prompting when the version requirement changes
tests/unit/install/installationManager.test.ts (3)
36-45: LGTM - Config mock properly extended.The allowlist validation in the
setmock is a good defensive pattern that catches unintended config key usage during testing.
115-115: LGTM - Mock updated with checkbox support.The default
checkboxChecked: falseensures existing tests behave correctly without modification.
258-267: LGTM - Test helpers are appropriately minimal.The type casts are localized and acceptable per testing guidelines. The helpers provide clean access to private methods for testing the suppression logic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| it('skips dialog when warning is suppressed for the current minimum version', async () => { | ||
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | ||
| const managerMethods = getManagerMethods(); | ||
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | ||
| const smiFallbackSpy = vi | ||
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | ||
| .mockResolvedValue(undefined); | ||
|
|
||
| try { | ||
| vi.mocked(config.get).mockImplementation((key: string) => { | ||
| if (key === 'installState') return 'installed'; | ||
| if (key === 'basePath') return 'valid/base'; | ||
| if (key === 'suppressNvidiaDriverWarningFor') return '580'; | ||
| return undefined; | ||
| }); | ||
|
|
||
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | ||
|
|
||
| expect(smiSpy).not.toHaveBeenCalled(); | ||
| expect(smiFallbackSpy).not.toHaveBeenCalled(); | ||
| expect(mockAppWindow.showMessageBox).not.toHaveBeenCalled(); | ||
| } finally { | ||
| smiSpy.mockRestore(); | ||
| smiFallbackSpy.mockRestore(); | ||
| platformSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it('stores the current minimum version when dismissed', async () => { | ||
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | ||
| const managerMethods = getManagerMethods(); | ||
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | ||
| const smiFallbackSpy = vi | ||
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | ||
| .mockResolvedValue(undefined); | ||
|
|
||
| try { | ||
| vi.mocked(mockAppWindow.showMessageBox).mockResolvedValue({ response: 0, checkboxChecked: true }); | ||
|
|
||
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | ||
|
|
||
| expect(config.set).toHaveBeenCalledWith('suppressNvidiaDriverWarningFor', '580'); | ||
| } finally { | ||
| smiSpy.mockRestore(); | ||
| smiFallbackSpy.mockRestore(); | ||
| platformSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use test instead of it per coding guidelines.
The tests are well-structured with proper mock setup and cleanup. However, per coding guidelines for tests/unit/**/*.test.{js,ts}: "Use test instead of it for defining tests."
Suggested change
- it('skips dialog when warning is suppressed for the current minimum version', async () => {
+ test('skips dialog when warning is suppressed for the current minimum version', async () => {- it('stores the current minimum version when dismissed', async () => {
+ test('stores the current minimum version when dismissed', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('skips dialog when warning is suppressed for the current minimum version', async () => { | |
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | |
| const managerMethods = getManagerMethods(); | |
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | |
| const smiFallbackSpy = vi | |
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | |
| .mockResolvedValue(undefined); | |
| try { | |
| vi.mocked(config.get).mockImplementation((key: string) => { | |
| if (key === 'installState') return 'installed'; | |
| if (key === 'basePath') return 'valid/base'; | |
| if (key === 'suppressNvidiaDriverWarningFor') return '580'; | |
| return undefined; | |
| }); | |
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | |
| expect(smiSpy).not.toHaveBeenCalled(); | |
| expect(smiFallbackSpy).not.toHaveBeenCalled(); | |
| expect(mockAppWindow.showMessageBox).not.toHaveBeenCalled(); | |
| } finally { | |
| smiSpy.mockRestore(); | |
| smiFallbackSpy.mockRestore(); | |
| platformSpy.mockRestore(); | |
| } | |
| }); | |
| it('stores the current minimum version when dismissed', async () => { | |
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | |
| const managerMethods = getManagerMethods(); | |
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | |
| const smiFallbackSpy = vi | |
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | |
| .mockResolvedValue(undefined); | |
| try { | |
| vi.mocked(mockAppWindow.showMessageBox).mockResolvedValue({ response: 0, checkboxChecked: true }); | |
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | |
| expect(config.set).toHaveBeenCalledWith('suppressNvidiaDriverWarningFor', '580'); | |
| } finally { | |
| smiSpy.mockRestore(); | |
| smiFallbackSpy.mockRestore(); | |
| platformSpy.mockRestore(); | |
| } | |
| }); | |
| test('skips dialog when warning is suppressed for the current minimum version', async () => { | |
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | |
| const managerMethods = getManagerMethods(); | |
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | |
| const smiFallbackSpy = vi | |
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | |
| .mockResolvedValue(undefined); | |
| try { | |
| vi.mocked(config.get).mockImplementation((key: string) => { | |
| if (key === 'installState') return 'installed'; | |
| if (key === 'basePath') return 'valid/base'; | |
| if (key === 'suppressNvidiaDriverWarningFor') return '580'; | |
| return undefined; | |
| }); | |
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | |
| expect(smiSpy).not.toHaveBeenCalled(); | |
| expect(smiFallbackSpy).not.toHaveBeenCalled(); | |
| expect(mockAppWindow.showMessageBox).not.toHaveBeenCalled(); | |
| } finally { | |
| smiSpy.mockRestore(); | |
| smiFallbackSpy.mockRestore(); | |
| platformSpy.mockRestore(); | |
| } | |
| }); | |
| test('stores the current minimum version when dismissed', async () => { | |
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | |
| const managerMethods = getManagerMethods(); | |
| const smiSpy = vi.spyOn(managerMethods, 'getNvidiaDriverVersionFromSmi').mockResolvedValue('570.0'); | |
| const smiFallbackSpy = vi | |
| .spyOn(managerMethods, 'getNvidiaDriverVersionFromSmiFallback') | |
| .mockResolvedValue(undefined); | |
| try { | |
| vi.mocked(mockAppWindow.showMessageBox).mockResolvedValue({ response: 0, checkboxChecked: true }); | |
| await managerMethods.warnIfNvidiaDriverTooOld(createInstallation('nvidia')); | |
| expect(config.set).toHaveBeenCalledWith('suppressNvidiaDriverWarningFor', '580'); | |
| } finally { | |
| smiSpy.mockRestore(); | |
| smiFallbackSpy.mockRestore(); | |
| platformSpy.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@tests/unit/install/installationManager.test.ts` around lines 279 - 326,
Replace the two test declarations that use "it(" with "test(" to follow the
repository guideline; specifically change the blocks that call
warnIfNvidiaDriverTooOld (the test titled "skips dialog when warning is
suppressed for the current minimum version" and the one titled "stores the
current minimum version when dismissed") so they use test(...) instead of
it(...), leaving all mocks and assertions involving
getNvidiaDriverVersionFromSmi, getNvidiaDriverVersionFromSmiFallback,
mockAppWindow.showMessageBox, and config.set unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/install/installationManager.ts`:
- Around line 50-56: The exported shape NvidiaDriverWarningContext (used by
shouldWarnAboutNvidiaDriver) is currently declared as a local type; change it to
an exported interface so the API is explicit and extensible: replace the type
alias NvidiaDriverWarningContext with an exported interface
NvidiaDriverWarningContext including the same fields (platform, selectedDevice?,
suppressWarningFor?, driverVersion?, minimumVersion?) and ensure any references
(e.g., the shouldWarnAboutNvidiaDriver function signature) import/accept the
interface; export the interface from the module so consumers can
extend/implement it.
♻️ Duplicate comments (1)
tests/unit/install/installationManager.test.ts (1)
260-333: Usetest.eachinstead ofit.eachfor new tests.♻️ Suggested update
-import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, test, vi } from 'vitest'; ... - it.each(scenarios)('$scenario', ({ input, expected }) => { + test.each(scenarios)('$scenario', ({ input, expected }) => { expect(shouldWarnAboutNvidiaDriver(input)).toBe(expected); });As per coding guidelines, prefer
testoveritin unit tests.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/install/installationManager.tstests/unit/install/installationManager.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:
src/install/installationManager.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/install/installationManager.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/install/installationManager.tstests/unit/install/installationManager.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/install/installationManager.tstests/unit/install/installationManager.test.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/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/install/installationManager.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/install/installationManager.test.ts
🧠 Learnings (14)
📚 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 src/main.ts : For main process entry point at `src/main.ts`, implement Electron application lifecycle management and window initialization
Applied to files:
src/install/installationManager.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 src/preload.ts : For preload script at `src/preload.ts`, implement secure IPC bridge between main and renderer processes without exposing sensitive APIs
Applied to files:
src/install/installationManager.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 {src/main.ts,src/preload.ts,src/main-process/**/*.ts} : Main process code must maintain clean separation between main process, renderer, and preload scripts; follow Electron security best practices
Applied to files:
src/install/installationManager.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/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} : Use `test` instead of `it` for defining tests
Applied to files:
tests/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/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} : Mocks should be cleanly written, easy to understand, and reusable where possible
Applied to files:
tests/unit/install/installationManager.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/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/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 : 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: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/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/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: Run `yarn test:unit` before committing changes
Applied to files:
tests/unit/install/installationManager.test.ts
🧬 Code graph analysis (2)
src/install/installationManager.ts (2)
src/preload.ts (1)
TorchDeviceType(22-22)src/store/desktopConfig.ts (1)
useDesktopConfig(13-16)
tests/unit/install/installationManager.test.ts (1)
src/install/installationManager.ts (2)
shouldWarnAboutNvidiaDriver(63-78)NVIDIA_DRIVER_MIN_VERSION(26-26)
⏰ 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). (4)
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: lint-and-format (macos-latest)
- GitHub Check: lint-and-format (windows-latest)
🔇 Additional comments (7)
src/install/installationManager.ts (4)
14-14: No concerns with the added type import.
26-26: Centralizing the minimum driver version is good.
63-78: Helper predicate looks good and keeps the warning decision testable.
431-460: Suppression flow and checkbox persistence look correct.tests/unit/install/installationManager.test.ts (3)
10-13: No concerns with the import additions.
37-47: Mock updates look fine.
117-117: No concerns with the checkbox default mock.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| type NvidiaDriverWarningContext = { | ||
| platform: NodeJS.Platform; | ||
| selectedDevice?: TorchDeviceType; | ||
| suppressWarningFor?: string; | ||
| driverVersion?: string; | ||
| minimumVersion?: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer an exported interface for the warning context.
This shape is part of the exported API via shouldWarnAboutNvidiaDriver, so make it explicit and extensible.
♻️ Suggested refactor
- type NvidiaDriverWarningContext = {
+ /** Context for {`@link` shouldWarnAboutNvidiaDriver}. */
+ export interface NvidiaDriverWarningContext {
platform: NodeJS.Platform;
selectedDevice?: TorchDeviceType;
suppressWarningFor?: string;
driverVersion?: string;
minimumVersion?: string;
- };
+ }As per coding guidelines, prefer interfaces for public object shapes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type NvidiaDriverWarningContext = { | |
| platform: NodeJS.Platform; | |
| selectedDevice?: TorchDeviceType; | |
| suppressWarningFor?: string; | |
| driverVersion?: string; | |
| minimumVersion?: string; | |
| }; | |
| /** Context for {`@link` shouldWarnAboutNvidiaDriver}. */ | |
| export interface NvidiaDriverWarningContext { | |
| platform: NodeJS.Platform; | |
| selectedDevice?: TorchDeviceType; | |
| suppressWarningFor?: string; | |
| driverVersion?: string; | |
| minimumVersion?: string; | |
| } |
🤖 Prompt for AI Agents
In `@src/install/installationManager.ts` around lines 50 - 56, The exported shape
NvidiaDriverWarningContext (used by shouldWarnAboutNvidiaDriver) is currently
declared as a local type; change it to an exported interface so the API is
explicit and extensible: replace the type alias NvidiaDriverWarningContext with
an exported interface NvidiaDriverWarningContext including the same fields
(platform, selectedDevice?, suppressWarningFor?, driverVersion?,
minimumVersion?) and ensure any references (e.g., the
shouldWarnAboutNvidiaDriver function signature) import/accept the interface;
export the interface from the module so consumers can extend/implement it.
Test Evidence CheckIf 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:
|
Summary
Resolves #1534
┆Issue is synchronized with this Notion page by Unito