Adopt: keep legacy extra_model_paths.yaml install-scoped#1105
Adopt: keep legacy extra_model_paths.yaml install-scoped#1105Kosinkadink wants to merge 1 commit into
Conversation
Desktop adoption now treats the legacy `extra_model_paths.yaml` (the file ComfyUI auto-loads from the source dir and the Storage tab shows read-only) as install-local, never merging its paths into the global shared model dirs. This avoids one migrated install's arbitrary/external paths leaking into every other install. - `computeModelsDirsToCarry` / `carryLegacySettings` again feed only `extra_models_config.yaml` into the global `modelsDirs` (the user's main models stay shared, as before). - New `preserveInstallExtraModelPaths` ensures the legacy `extra_model_paths.yaml` lands in the adopted install's source dir so ComfyUI loads it for that install and the Storage tab reports it: left as-is when the pre-swap copy already carried it, recovered from the reused basePath in the git-clone fallback, no-op otherwise. - Surfaced as `has_install_extra_model_paths_yaml` in adopt telemetry. The Storage tab already renders the install's extra_model_paths.yaml (buildExtraModelPathsView), so no UI change is needed. Amp-Thread-ID: https://ampcode.com/threads/T-019ebfcb-e545-73d8-a33d-52580e41588a Co-authored-by: Amp <amp@ampcode.com>
|
|
📝 WalkthroughWalkthroughAdds ChangesInstall-local extra_model_paths.yaml preservation
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/desktopAdopt.test.ts (1)
950-956: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the new telemetry flag in the success payload contract.
Please extend this
comfy.desktop.adopt.succeededassertion to includehas_install_extra_model_paths_yaml, or telemetry drift may slip through quietly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/desktopAdopt.test.ts` around lines 950 - 956, The telemetry assertion for the 'comfy.desktop.adopt.succeeded' event is missing a check for the new telemetry flag has_install_extra_model_paths_yaml. Extend the expect.objectContaining call in the assertion to include has_install_extra_model_paths_yaml alongside the existing carried_keys check to ensure this new flag is part of the telemetry payload contract and prevent telemetry payload drift from going undetected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/lib/desktopAdopt.ts`:
- Around line 57-61: The multi-line comment preceding the EXTRA_MODEL_PATHS_YAML
constant declaration is too verbose and misleadingly suggests the file carries
over globally across model directories, when the actual implementation keeps it
install-scoped. Shorten this comment to a single concise sentence that
accurately describes what the constant is and its install-local scope, removing
narrative details about history and implementation specifics.
---
Outside diff comments:
In `@src/main/lib/desktopAdopt.test.ts`:
- Around line 950-956: The telemetry assertion for the
'comfy.desktop.adopt.succeeded' event is missing a check for the new telemetry
flag has_install_extra_model_paths_yaml. Extend the expect.objectContaining call
in the assertion to include has_install_extra_model_paths_yaml alongside the
existing carried_keys check to ensure this new flag is part of the telemetry
payload contract and prevent telemetry payload drift from going undetected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e59487f6-9c5e-4c93-b769-f11eb099fa8a
📒 Files selected for processing (2)
src/main/lib/desktopAdopt.test.tssrc/main/lib/desktopAdopt.ts
| // The other model-paths file: ComfyUI auto-loads it from the source dir and | ||
| // the launcher displays it read-only. Carried alongside EXTRA_MODELS_YAML so | ||
| // its dirs survive into v2's modelsDirs (not just the adopted ComfyUI's own | ||
| // search), including in the git-clone source fallback where it isn't copied. | ||
| const EXTRA_MODEL_PATHS_YAML = 'extra_model_paths.yaml' |
There was a problem hiding this comment.
Correct the EXTRA_MODEL_PATHS_YAML comment to match install-local behavior.
Line 57-61 currently implies global modelsDirs carry-over, but the implementation intentionally keeps extra_model_paths.yaml install-scoped, so this comment should be shortened and aligned to avoid future drift.
As per coding guidelines, “Write concise comments — one sentence is better than five paragraphs to justify a small change” and “Don't narrate history in comments (e.g., 'This used to do X, now it does Y'); rely on git log for that information.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/lib/desktopAdopt.ts` around lines 57 - 61, The multi-line comment
preceding the EXTRA_MODEL_PATHS_YAML constant declaration is too verbose and
misleadingly suggests the file carries over globally across model directories,
when the actual implementation keeps it install-scoped. Shorten this comment to
a single concise sentence that accurately describes what the constant is and its
install-local scope, removing narrative details about history and implementation
specifics.
Source: Coding guidelines
2 participants
Summary
During Legacy Desktop → Desktop 2.0 adoption, the legacy
extra_model_paths.yaml(the file ComfyUI auto-loads from the source dir, and which the Storage tab shows read-only) is now treated as install-local rather than being merged into the global shared model dirs.Previously the desktop-managed
extra_models_config.yamlwas the only model-paths file carried intosettings.modelsDirs. An interim approach also dumpedextra_model_paths.yamlpaths into that global list — but those are often arbitrary/external paths for one specific legacy setup, so making them global would make them apply to every install (including future fresh ones), which is confusing.What changed
computeModelsDirsToCarry/carryLegacySettingsagain feed onlyextra_models_config.yamlinto the globalmodelsDirs(the user's main<basePath>/modelsstays shared, as before).preserveInstallExtraModelPathsensures the legacyextra_model_paths.yamlends up in the adopted install's source dir, so ComfyUI loads it for that install only and the Storage tab reports it:basePathin the git-clone fallback (a fresh clone ships onlyextra_model_paths.yaml.example),has_install_extra_model_paths_yamlin the adopt success telemetry.The Storage tab already renders the install's
extra_model_paths.yamlviabuildExtraModelPathsView, so no UI change is needed — preserving the file is what makes that read-only row appear. Users who want those paths elsewhere can open the file and copy them out manually.Testing
pnpm run typecheck(node/web/e2e/integration) — passpnpm run lint— passpnpm run build— passpnpm run test— 2193/2193 pass, including new unit tests forpreserveInstallExtraModelPathsand the install-scopedcomputeModelsDirsToCarrybehavior.