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. 📝 WalkthroughWalkthroughBuild and runtime now detect if the bundled ComfyUI includes manager_requirements.txt. The build skips cloning the legacy ComfyUI-Manager when present; runtime resolves primary vs legacy manager requirements paths. CLI and virtual-environment code add fallbacks to support pip-installed manager and legacy custom-node layouts; tests and README updated. Changes
Possibly related PRs
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
⏰ 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)
🔇 Additional comments (1)
Comment |
dosubot
bot
added
size:L
![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
🧹 Nitpick comments (3)
scripts/makeComfy.js (1)
26-30: Consider cross-platform compatibility for thecdcommand chain.The shell command uses
cd ... && git checkout ... && cd ...which works on Unix-like systems but may behave differently on Windows cmd vs PowerShell. SinceexecSyncuses the default shell, this could be problematic on Windows.Consider using separate commands or setting
cwdinexecSyncoptions:- execAndLog(`git clone ${managerRepo} assets/ComfyUI/custom_nodes/ComfyUI-Manager`); - execAndLog( - `cd assets/ComfyUI/custom_nodes/ComfyUI-Manager && git ${noWarning} checkout ${pkg.config.managerCommit} && cd ../../..` - ); + execAndLog(`git clone ${managerRepo} assets/ComfyUI/custom_nodes/ComfyUI-Manager`); + execAndLog(`git ${noWarning} checkout ${pkg.config.managerCommit}`, { + cwd: 'assets/ComfyUI/custom_nodes/ComfyUI-Manager' + });This would require updating
execAndLogto accept options.scripts/verifyBuild.js (2)
35-35: Unnecessary type conversion.The
config.requiredis already typed asstring[]per the typedef. The.map(String)conversion is redundant.- const required = config.required.map(String); + const required = [...config.required];
1-2: Remove extraneous blank lines at file start.The file begins with two empty lines which appear unintentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(4 hunks)scripts/makeComfy.js(2 hunks)scripts/todesktop/afterPack.cjs(0 hunks)scripts/verifyBuild.js(1 hunks)src/constants.ts(1 hunks)src/services/cmCli.ts(2 hunks)src/virtualEnvironment.ts(7 hunks)tests/unit/install/installationManager.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- scripts/todesktop/afterPack.cjs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript as the primary language and maintain exceptionally high type standards - theanytype must not be used.unknowncan be used when the type is unknown.
Use JSDoc format to write documentation for methods, including common tags such as@paramand@return(do not use@returnforvoidreturn types), and use{@link }to reference symbols.
Follow existing TypeScript patterns in the codebase and maintain clean separation between main process, renderer, and preload scripts.
Follow Electron security best practices in TypeScript code.
Runyarn typescriptto perform TypeScript type checking after making changes.
Files:
src/constants.tstests/unit/install/installationManager.test.tssrc/services/cmCli.tssrc/virtualEnvironment.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint and Prettier for code formatting and linting - run
yarn lintandyarn formatto ensure consistent code style.
Files:
src/constants.tstests/unit/install/installationManager.test.tssrc/services/cmCli.tsscripts/verifyBuild.jsscripts/makeComfy.jssrc/virtualEnvironment.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the proper project structure with main process entry at
main.ts, core application logic indesktopApp.ts, preload script atpreload.ts, and organize modules in appropriate subdirectories (main-process/,install/,handlers/,services/,config/,store/,utils.ts).
Files:
src/constants.tssrc/services/cmCli.tssrc/virtualEnvironment.ts
**/*.{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/constants.tstests/unit/install/installationManager.test.tssrc/services/cmCli.tssrc/virtualEnvironment.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/constants.tssrc/services/cmCli.tssrc/virtualEnvironment.ts
tests/unit/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Test changes with unit tests (Vitest) - use Vitest to create any tests needed and do not attempt to custom code testing infrastructure.
Files:
tests/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
🧠 Learnings (9)
📚 Learning: 2025-11-25T20:49:40.905Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.905Z
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:49:28.063Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.063Z
Learning: Applies to tests/unit/**/*.{ts,tsx} : Test changes with unit tests (Vitest) - use Vitest to create any tests needed and do not attempt to custom code testing infrastructure.
Applied to files:
tests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:49:40.905Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.905Z
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.905Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.905Z
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
📚 Learning: 2025-11-25T20:49:40.905Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.905Z
Learning: Integration tests require COMFYUI_ENABLE_VOLATILE_TESTS=1 environment variable or CI environment to run
Applied to files:
tests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:49:28.063Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.063Z
Learning: Applies to tests/integration/**/*.{ts,tsx} : Test changes with E2E tests (Playwright) for UI changes.
Applied to files:
tests/unit/install/installationManager.test.ts
📚 Learning: 2025-11-25T20:50:25.360Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.360Z
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:
scripts/verifyBuild.js
📚 Learning: 2025-11-25T20:49:50.631Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.631Z
Learning: Applies to tests/integration/**/*.{ts,js} : Use `path.join()` and `path.sep` to ensure file path handling works consistently across Windows, macOS, and Linux platforms
Applied to files:
scripts/verifyBuild.js
📚 Learning: 2025-11-25T20:49:28.063Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.063Z
Learning: Before committing, run `yarn format` to ensure consistent formatting, `yarn lint` and `yarn typescript` to check code quality, and `yarn test:unit` to ensure unit tests pass.
Applied to files:
README.mdscripts/makeComfy.js
🧬 Code graph analysis (3)
src/services/cmCli.ts (2)
src/install/resourcePaths.ts (1)
getAppResourcesPath(4-6)src/utils.ts (1)
pathAccessible(13-20)
scripts/verifyBuild.js (2)
scripts/makeComfy.js (1)
managerRequirementsPath(21-21)scripts/todesktop/afterPack.cjs (2)
path(3-3)fs(2-2)
src/virtualEnvironment.ts (3)
scripts/makeComfy.js (1)
managerRequirementsPath(21-21)scripts/verifyBuild.js (1)
managerRequirementsPath(36-36)src/utils.ts (1)
pathAccessible(13-20)
🪛 GitHub Actions: Unit Test, Lint, and Format
src/virtualEnvironment.ts
[error] 765-765: Manager requirements file was not found. Path: \test\resources\ComfyUI\manager_requirements.txt. If you are using a legacy build, ensure the ComfyUI-Manager custom node is present at \test\resources\ComfyUI\custom_nodes\ComfyUI-Manager\requirements.txt.
🪛 LanguageTool
README.md
[grammar] ~25-~25: Use a hyphen to join words.
Context: ...he uv executable as well as some desktop specific features. Developers, read on....
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (10)
src/constants.ts (1)
154-158: LGTM!The addition of
'enable-manager': ''as a default server argument aligns with the PR objective to enable the manager by default. The empty string value is appropriate for boolean-style CLI flags.tests/unit/install/installationManager.test.ts (1)
80-94: LGTM!The mock correctly adds the new
legacyComfyUIManagerRequirementsPathproperty, consistent with theVirtualEnvironmentclass changes and following the established mock pattern.scripts/makeComfy.js (1)
20-25: LGTM on the conditional manager detection logic.The path construction using
path.joinensures cross-platform compatibility, and the conditional check correctly determines whether to use pip-based or legacy git-clone installation.src/services/cmCli.ts (2)
14-27: LGTM!The
buildCommandArgsmethod elegantly handles both pip-installed and legacy ComfyUI-Manager installations by checking for the CLI script's accessibility and falling back to module invocation (-m comfyui_manager.cm_cli). This maintains backward compatibility while supporting the new pip-based installation.
42-44: Good async integration.The await on
buildCommandArgsis correctly placed before passing the result torunPythonCommandAsync.README.md (1)
22-23: Documentation updates are clear and accurate.The README properly documents the transition to pip-based ComfyUI-Manager installation with appropriate guidance for legacy bundles.
Also applies to: 36-36, 173-173, 218-219
scripts/verifyBuild.js (1)
38-44: Good fallback logic for manager path verification.The three-way conditional correctly handles:
- Pip-based installation (manager_requirements.txt exists)
- Legacy installation (ComfyUI-Manager directory exists)
- Fallback to manager_requirements.txt to ensure verification fails with a clear error message if neither exists
This aligns with the detection logic in
makeComfy.js.src/virtualEnvironment.ts (3)
241-245: LGTM on resolver logic.The
resolveManagerRequirementsPathhelper correctly implements the priority: prefer primary path if exists, fall back to legacy path if exists, otherwise default to primary (allowing downstream validation to report the missing file).
393-396: Good addition to ensure manager requirements are installed.After installing compiled requirements, calling
installComfyUIManagerRequirementsensures the manager dependencies are present even if the compiled file didn't include them. This provides robustness for varying build configurations.
5-5: Consistent use of synchronousexistsSyncin constructor.Using
existsSyncin the constructor is appropriate since constructors cannot be async. The resolver logic and property initialization are clean.Also applies to: 191-202, 120-120
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR transitions ComfyUI-Manager from being bundled as a custom node to being installed as a pip package, with backward compatibility for legacy builds. The changes enable --enable-manager by default and allow the manager's CLI tool to run from either the legacy file-based location or the installed Python module.
- Implement fallback logic to support both pip-distributed manager_requirements.txt and legacy custom node requirements
- Enable manager by default via server args and update cm-cli to support module-based execution
- Update build scripts and documentation to reflect the new pip-based manager distribution
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/virtualEnvironment.ts |
Adds legacyComfyUIManagerRequirementsPath property and resolveManagerRequirementsPath method to support both new pip-based and legacy file-based manager requirements, with validation checks in install methods |
src/services/cmCli.ts |
Updates command building to check for legacy cm-cli.py file first, falling back to running as Python module (-m comfyui_manager.cm_cli) |
src/constants.ts |
Enables manager by default by adding 'enable-manager': '' to DEFAULT_SERVER_ARGS |
scripts/verifyBuild.js |
Updates build verification to check for either manager_requirements.txt or legacy ComfyUI-Manager custom node, with fallback logic |
scripts/todesktop/afterPack.cjs |
Removes git cleanup for ComfyUI-Manager custom node since it may not be present in pip-based builds |
scripts/makeComfy.js |
Conditionally clones ComfyUI-Manager custom node only when manager_requirements.txt is not present |
README.md |
Updates documentation to explain pip-based manager installation with legacy fallback, including updated compiled requirements commands |
tests/unit/install/installationManager.test.ts |
Adds legacyComfyUIManagerRequirementsPath to the VirtualEnvironment mock |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!(await pathAccessible(this.comfyUIManagerRequirementsPath))) { | ||
| throw new Error( | ||
| `Manager requirements file was not found at ${this.comfyUIManagerRequirementsPath}. ` + | ||
| `If you are using a legacy build, ensure the ComfyUI-Manager custom node is present at ${this.legacyComfyUIManagerRequirementsPath}.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
This error message is duplicated on lines 764-769. Consider extracting this to a private method or constant to maintain consistency and make future updates easier. For example:
private getManagerRequirementsNotFoundError(): Error {
return new Error(
`Manager requirements file was not found at ${this.comfyUIManagerRequirementsPath}. ` +
`If you are using a legacy build, ensure the ComfyUI-Manager custom node is present at ${this.legacyComfyUIManagerRequirementsPath}.`
);
}Then use throw this.getManagerRequirementsNotFoundError(); in both locations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/verifyBuild.js (1)
33-33: Optional: Simplify array copy with spread operator.Since
config.requiredis already typed asstring[], the.map(String)is unnecessary. Using spread is more idiomatic:- const required = config.required.map(String); + const required = [...config.required];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.md(4 hunks)scripts/verifyBuild.js(1 hunks)src/main-process/comfyDesktopApp.ts(1 hunks)tests/resources/ComfyUI/manager_requirements.txt(1 hunks)tests/unit/main-process/comfyDesktopApp.test.ts(3 hunks)tests/unit/virtualEnvironment.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript as the primary language and maintain exceptionally high type standards - theanytype must not be used.unknowncan be used when the type is unknown.
Use JSDoc format to write documentation for methods, including common tags such as@paramand@return(do not use@returnforvoidreturn types), and use{@link }to reference symbols.
Follow existing TypeScript patterns in the codebase and maintain clean separation between main process, renderer, and preload scripts.
Follow Electron security best practices in TypeScript code.
Runyarn typescriptto perform TypeScript type checking after making changes.
Files:
src/main-process/comfyDesktopApp.tstests/unit/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint and Prettier for code formatting and linting - run
yarn lintandyarn formatto ensure consistent code style.
Files:
src/main-process/comfyDesktopApp.tstests/unit/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.test.tsscripts/verifyBuild.js
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the proper project structure with main process entry at
main.ts, core application logic indesktopApp.ts, preload script atpreload.ts, and organize modules in appropriate subdirectories (main-process/,install/,handlers/,services/,config/,store/,utils.ts).
Files:
src/main-process/comfyDesktopApp.ts
**/*.{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/comfyDesktopApp.tstests/unit/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.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/comfyDesktopApp.ts
tests/unit/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Test changes with unit tests (Vitest) - use Vitest to create any tests needed and do not attempt to custom code testing infrastructure.
Files:
tests/unit/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.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/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.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/main-process/comfyDesktopApp.test.ts
🧠 Learnings (18)
📚 Learning: 2025-11-25T20:49:28.076Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.076Z
Learning: Applies to src/**/*.ts : Maintain the proper project structure with main process entry at `main.ts`, core application logic in `desktopApp.ts`, preload script at `preload.ts`, and organize modules in appropriate subdirectories (`main-process/`, `install/`, `handlers/`, `services/`, `config/`, `store/`, `utils.ts`).
Applied to files:
src/main-process/comfyDesktopApp.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/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.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/**/*.{ts,js} : Use `path.join()` and `path.sep` to ensure file path handling works consistently across Windows, macOS, and Linux platforms
Applied to files:
tests/unit/virtualEnvironment.test.tsscripts/verifyBuild.js
📚 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/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} : 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.tsscripts/verifyBuild.js
📚 Learning: 2025-11-25T20:49:28.076Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.076Z
Learning: Applies to tests/unit/**/*.{ts,tsx} : Test changes with unit tests (Vitest) - use Vitest to create any tests needed and do not attempt to custom code testing infrastructure.
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.tstests/unit/main-process/comfyDesktopApp.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/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} : Do not build custom testing infrastructure; use Vitest and existing helpers
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} : Prefer the use of `test.extend` over loose variables in Vitest 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: 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 : Use Playwright + TypeScript for Electron testing in integration tests
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 : Mock native dialogs when needed using app.app.evaluate() with electron.dialog overrides
Applied to files:
tests/unit/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.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/virtualEnvironment.test.tstests/unit/main-process/comfyDesktopApp.test.ts
📚 Learning: 2025-11-25T20:49:28.076Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.076Z
Learning: Before committing, run `yarn format` to ensure consistent formatting, `yarn lint` and `yarn typescript` to check code quality, and `yarn test:unit` to ensure unit tests pass.
Applied to files:
README.md
📚 Learning: 2025-11-25T20:49:28.076Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T20:49:28.076Z
Learning: Applies to tests/integration/**/*.{ts,tsx} : Test changes with E2E tests (Playwright) for UI changes.
Applied to files:
tests/unit/main-process/comfyDesktopApp.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/main-process/comfyDesktopApp.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} : Prefer `T[]` over `Array<T>` for readability
Applied to files:
scripts/verifyBuild.js
🧬 Code graph analysis (2)
src/main-process/comfyDesktopApp.ts (1)
src/constants.ts (1)
DEFAULT_SERVER_ARGS(154-158)
scripts/verifyBuild.js (1)
scripts/makeComfy.js (1)
managerRequirementsPath(21-21)
⏰ 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-and-test-e2e-windows / integration-windows-test
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: lint-and-format (macos-latest)
- GitHub Check: lint-and-format (windows-latest)
🔇 Additional comments (9)
README.md (4)
22-22: LGTM! Clear documentation of pip-based manager installation.The description accurately reflects the new installation flow with legacy fallback for older ComfyUI versions.
25-25: Grammar fix correctly applied.The hyphenation of "desktop-specific" addresses the previous review feedback appropriately.
173-173: LGTM! Consistent documentation of manager requirements paths.The updated paths and commands correctly reference the new
manager_requirements.txtlocation and align with the pip-based manager packaging approach.Also applies to: 201-201, 206-206, 214-214
218-219: Excellent addition for backwards compatibility.This note provides clear guidance for developers working with legacy ComfyUI bundles, ensuring the documentation covers both new and old installation paths.
tests/unit/virtualEnvironment.test.ts (1)
3-3: LGTM! Improved test maintainability with dynamic path resolution.The dynamic computation of
resourcesPathusingpath.join(__dirname, '../resources')makes the tests more maintainable and cross-platform compatible, aligning with coding guidelines for proper path handling.Also applies to: 31-31, 36-36
tests/resources/ComfyUI/manager_requirements.txt (1)
1-1: LGTM! Appropriate test resource.The minimal dummy file is appropriate for test purposes, providing a realistic test resource for verifying manager requirements file detection.
tests/unit/main-process/comfyDesktopApp.test.ts (1)
128-128: LGTM! Tests correctly updated for new default server argument.The test expectations properly include the
enable-managerfield added toDEFAULT_SERVER_ARGS, validating thatbuildServerArgscorrectly spreads all default arguments across different scenarios (defaults, overrides, and external server).Also applies to: 142-142, 157-157
scripts/verifyBuild.js (2)
8-12: LGTM! Improved type documentation.The
VerifyConfigtypedef and explicit type annotation forPATHSimprove code documentation and provide better editor support.
34-42: LGTM! Manager requirements detection logic is correct.The detection logic properly implements the fallback chain (new pip-based → legacy custom node → default to pip-based), aligning with the runtime behavior. Iterating over the augmented
requiredlist ensures the build verification correctly validates the presence of appropriate manager files.Also applies to: 46-46
|
the one test failure is expected from a past change that I will need to tweak and tune, will manually test this one before the next major ComfyUI patch |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/verifyBuild.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint and Prettier for code formatting and linting - run
yarn lintandyarn formatto ensure consistent code style.
Files:
scripts/verifyBuild.js
🧠 Learnings (3)
📚 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:
scripts/verifyBuild.js
📚 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/**/*.{ts,js} : Use `path.join()` and `path.sep` to ensure file path handling works consistently across Windows, macOS, and Linux platforms
Applied to files:
scripts/verifyBuild.js
📚 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 `T[]` over `Array<T>` for readability
Applied to files:
scripts/verifyBuild.js
🧬 Code graph analysis (1)
scripts/verifyBuild.js (2)
scripts/makeComfy.js (1)
managerRequirementsPath(21-21)scripts/todesktop/afterPack.cjs (2)
path(3-3)fs(2-2)
⏰ 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-and-test-e2e-windows / integration-windows-test
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: lint-and-format (windows-latest)
- GitHub Check: lint-and-format (macos-latest)
🔇 Additional comments (1)
scripts/verifyBuild.js (1)
33-33: LGTM!The spread operator correctly creates a copy of the required array, addressing the previous review feedback.
| const managerRequirementsPath = path.join(config.base, 'ComfyUI', 'manager_requirements.txt'); | ||
| const legacyManagerPath = path.join(config.base, 'ComfyUI', 'custom_nodes', 'ComfyUI-Manager'); | ||
| if (fs.existsSync(managerRequirementsPath)) { | ||
| required.push('ComfyUI/manager_requirements.txt'); | ||
| } else if (fs.existsSync(legacyManagerPath)) { | ||
| required.push('ComfyUI/custom_nodes/ComfyUI-Manager'); | ||
| } else { | ||
| required.push('ComfyUI/manager_requirements.txt'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use path.join() for cross-platform path construction.
Lines 37, 39, and 41 push strings with hardcoded forward slashes. While path.join() normalizes these later, explicitly constructing these paths ensures consistency across platforms.
Based on learnings, prefer path.join() for all path construction to ensure cross-platform compatibility.
Apply this diff:
const managerRequirementsPath = path.join(config.base, 'ComfyUI', 'manager_requirements.txt');
const legacyManagerPath = path.join(config.base, 'ComfyUI', 'custom_nodes', 'ComfyUI-Manager');
if (fs.existsSync(managerRequirementsPath)) {
- required.push('ComfyUI/manager_requirements.txt');
+ required.push(path.join('ComfyUI', 'manager_requirements.txt'));
} else if (fs.existsSync(legacyManagerPath)) {
- required.push('ComfyUI/custom_nodes/ComfyUI-Manager');
+ required.push(path.join('ComfyUI', 'custom_nodes', 'ComfyUI-Manager'));
} else {
- required.push('ComfyUI/manager_requirements.txt');
+ required.push(path.join('ComfyUI', 'manager_requirements.txt'));
}🤖 Prompt for AI Agents
In scripts/verifyBuild.js around lines 34 to 42, the code pushes hardcoded path
strings with forward slashes into the required array; replace those string
literals with path.join calls (e.g.,
path.join('ComfyUI','manager_requirements.txt') and
path.join('ComfyUI','custom_nodes','ComfyUI-Manager')) so all pushed paths are
constructed via path.join for consistent cross-platform path separators and
normalization.
| } else { | ||
| required.push('ComfyUI/manager_requirements.txt'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment to clarify fallback intent.
The else block adds manager_requirements.txt to required even if it doesn't exist, which will cause verification to fail. This enforces that at least one manager configuration must be present. A brief comment would make this intent explicit.
} else {
+ // Fallback: require new manager format, which will fail verification if neither config exists
required.push(path.join('ComfyUI', 'manager_requirements.txt'));
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/verifyBuild.js around lines 40-42, the else branch unconditionally
pushes 'ComfyUI/manager_requirements.txt' into the required array as an
intentional fallback to enforce at least one manager configuration; add a brief
inline comment explaining this fallback intent (e.g., that the script requires
at least one manager config and will fail verification if none of the recognized
files are present) so future readers understand why a non-existent file is being
added and that the behavior is deliberate.
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
This reverts commit 2e1a5b5.
|
Ready to go after stable is bumped |
|
Stable is set to be bumped today |
|
This is the greenest desktop CI I've seen in months |
|
Test failure is expected and will be fixed soon |
1 participant
Summary
┆Issue is synchronized with this Notion page by Unito