From 6fe0543d2e401d08cd3fa8bc52cab1f675944b6c Mon Sep 17 00:00:00 2001 From: Will Miao Date: Sun, 13 Sep 2026 11:30:57 +0800 Subject: [PATCH] fix(other-models): default downloads to a flat path, not {base_model}/{first_tag} get_download_path_template() fell back to "{base_model}/{first_tag}" for any unconfigured model type, so other-model downloads were silently nested under an arbitrary CivitAI tag even though the settings UI exposes no template row for "other" and priority_tags has no "other" entry (making {first_tag} resolve to tags[0]). Add DEFAULT_DOWNLOAD_PATH_TEMPLATES with other -> "" so unconfigured and unknown types resolve to a flat layout under the already sub_type-scoped default_other_roots; explicit settings.json values still win. Mirror the flat default in the frontend DEFAULT_PATH_TEMPLATES and stop the download/move default-path previews from rendering "/undefined" or a dangling slash. --- docs/plans/other-models-page.md | 2 +- py/services/settings_manager.py | 27 ++- py/utils/constants.py | 13 ++ static/js/managers/DownloadManager.js | 11 +- static/js/managers/MoveManager.js | 9 +- static/js/utils/constants.js | 6 +- .../downloadManager.targetPath.test.js | 183 ++++++++++++++++++ tests/services/test_download_manager_other.py | 23 +++ tests/services/test_settings_manager.py | 20 ++ 9 files changed, 269 insertions(+), 25 deletions(-) create mode 100644 tests/frontend/managers/downloadManager.targetPath.test.js diff --git a/docs/plans/other-models-page.md b/docs/plans/other-models-page.md index 3f270193..37c1d488 100644 --- a/docs/plans/other-models-page.md +++ b/docs/plans/other-models-page.md @@ -170,7 +170,7 @@ Flow: `POST /api/lm/download-model` (`py/routes/model_route_registrar.py:104`; G Hooks: `_record_downloaded_version_history` (model_type is free text — zero change); `_sync_downloaded_version` (`:1984` → scanner dispatch `:2130-2135`) add `other`; `py/utils/example_images_download_manager.py` scanner dispatch at `:411-421`, `:591-601`, `:1089+` — add `other` at all three (silent no-scanner otherwise). -Path templates: `get_download_path_template("other")` already falls back to `"{base_model}/{first_tag}"` — works with zero change; optional settings-UI row (§9.4). +Path templates: `get_download_path_template("other")` is unset, so `other` resolves to a **flat** layout (empty template) — downloads land directly under the resolved sub_type root. This is deliberate: other-model roots are already split per sub_type (`default_other_roots`), and `priority_tags` has no `other` entry, so `{first_tag}` would fall back to an arbitrary CivitAI tag and scatter files into unstable folders. Users who want nesting can still set `download_path_templates["other"]` in `settings.json`. See `DEFAULT_DOWNLOAD_PATH_TEMPLATES` (`py/utils/constants.py`) and `DEFAULT_PATH_TEMPLATES` (`static/js/utils/constants.js`). ### 9.2 File-level routing (model.type / file.type → sub_type) **[locked]** diff --git a/py/services/settings_manager.py b/py/services/settings_manager.py index 0d03c2d1..4217f92c 100644 --- a/py/services/settings_manager.py +++ b/py/services/settings_manager.py @@ -25,6 +25,7 @@ from typing import ( from platformdirs import user_config_dir from ..utils.constants import ( + DEFAULT_DOWNLOAD_PATH_TEMPLATES, DEFAULT_ENABLED_OTHER_SUB_TYPES, DEFAULT_HASH_CHUNK_SIZE_MB, DEFAULT_PRIORITY_TAG_CONFIG, @@ -2335,10 +2336,14 @@ class SettingsManager: """Get download path template for specific model type Args: - model_type: The type of model ('lora', 'checkpoint', 'embedding') + model_type: The type of model ('lora', 'checkpoint', 'embedding', + 'other') Returns: - Template string for the model type, defaults to '{base_model}/{first_tag}' + Template string for the model type. Falls back to the per-type + default in ``DEFAULT_DOWNLOAD_PATH_TEMPLATES``; unknown model types + resolve to an empty string (flat layout) rather than silently + nesting downloads under an unconfigured subfolder. """ templates = self.settings.get("download_path_templates", {}) @@ -2362,27 +2367,19 @@ class SettingsManager: logger.warning( f"Failed to parse download_path_templates JSON string: {e}. Setting default values." ) - default_template = "{base_model}/{first_tag}" - templates = { - "lora": default_template, - "checkpoint": default_template, - "embedding": default_template, - } + templates = dict(DEFAULT_DOWNLOAD_PATH_TEMPLATES) self.settings["download_path_templates"] = templates self._save_settings() # Ensure templates is a dictionary if not isinstance(templates, dict): - default_template = "{base_model}/{first_tag}" - templates = { - "lora": default_template, - "checkpoint": default_template, - "embedding": default_template, - } + templates = dict(DEFAULT_DOWNLOAD_PATH_TEMPLATES) self.settings["download_path_templates"] = templates self._save_settings() - return templates.get(model_type, "{base_model}/{first_tag}") + return templates.get( + model_type, DEFAULT_DOWNLOAD_PATH_TEMPLATES.get(model_type, "") + ) _SETTINGS_MANAGER: Optional["SettingsManager"] = None diff --git a/py/utils/constants.py b/py/utils/constants.py index 4f770dd0..c1539973 100644 --- a/py/utils/constants.py +++ b/py/utils/constants.py @@ -251,6 +251,19 @@ DEFAULT_PRIORITY_TAG_CONFIG = { "embedding": ", ".join(CIVITAI_MODEL_TAGS), } +# Default download path template for each model type. "other" defaults to a +# flat layout (empty template) on purpose: other-model downloads are already +# separated by sub_type roots (default_other_roots), and priority_tags has no +# "other" entry, so {first_tag} would resolve to an arbitrary CivitAI tag and +# scatter files into unstable folders. Users can still opt in to a template by +# writing "other" into download_path_templates in settings.json. +DEFAULT_DOWNLOAD_PATH_TEMPLATES: Dict[str, str] = { + "lora": "{base_model}/{first_tag}", + "checkpoint": "{base_model}/{first_tag}", + "embedding": "{base_model}/{first_tag}", + "other": "", +} + # baseModel values from CivitAI that should be treated as diffusion models (unet) # These model types are incorrectly labeled as "checkpoint" by CivitAI but are actually diffusion models DIFFUSION_MODEL_BASE_MODELS = frozenset( diff --git a/static/js/managers/DownloadManager.js b/static/js/managers/DownloadManager.js index a4acc43a..4150d9c4 100644 --- a/static/js/managers/DownloadManager.js +++ b/static/js/managers/DownloadManager.js @@ -2520,9 +2520,14 @@ export class DownloadManager { const singularType = this._isDiffusionModel ? 'unet' : this.apiClient.modelType.replace(/s$/, ''); - const templates = state.global.settings.download_path_templates; - const template = templates[singularType]; - fullPath += `/${template}`; + const templates = state.global?.settings?.download_path_templates; + const template = templates?.[singularType]; + // An empty or absent template means a flat layout: keep the + // root as-is instead of appending "/undefined" or a + // dangling slash. + if (template) { + fullPath += `/${template}`; + } } catch (error) { console.error('Failed to fetch template:', error); fullPath += '/' + translate('modals.download.autoOrganizedPath'); diff --git a/static/js/managers/MoveManager.js b/static/js/managers/MoveManager.js index d5042415..088fffa4 100644 --- a/static/js/managers/MoveManager.js +++ b/static/js/managers/MoveManager.js @@ -226,14 +226,13 @@ class MoveManager { if (modelRoot) { if (this.useDefaultPath) { - // Show actual template path + // Show actual template path; an empty/absent template means a + // flat layout, so keep the root as-is. const singularType = config.singularName || apiClient.modelType.replace(/s$/, ''); - const templates = state.global.settings.download_path_templates; - const template = templates[singularType]; + const templates = state.global?.settings?.download_path_templates; + const template = templates?.[singularType]; if (template) { fullPath += `/${template}`; - } else { - fullPath += '/' + translate('modals.download.autoOrganizedPath'); } } else { // Show manual path selection diff --git a/static/js/utils/constants.js b/static/js/utils/constants.js index 412ac647..755d60f3 100644 --- a/static/js/utils/constants.js +++ b/static/js/utils/constants.js @@ -353,7 +353,11 @@ export const DEFAULT_PATH_TEMPLATES = { lora: '{base_model}/{first_tag}', checkpoint: '{base_model}', unet: '{base_model}', - embedding: '{first_tag}' + embedding: '{first_tag}', + // Other models (VAE/upscaler/...) default to a flat layout: their root is + // already split per sub_type, and priority_tags has no "other" entry, so + // {first_tag} would resolve to an arbitrary CivitAI tag. + other: '' }; // Model type labels for UI diff --git a/tests/frontend/managers/downloadManager.targetPath.test.js b/tests/frontend/managers/downloadManager.targetPath.test.js new file mode 100644 index 00000000..1caecae5 --- /dev/null +++ b/tests/frontend/managers/downloadManager.targetPath.test.js @@ -0,0 +1,183 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { + DOWNLOAD_MANAGER_MODULE, + MODAL_MANAGER_MODULE, + UI_HELPERS_MODULE, + STATE_MODULE, + LOADING_MANAGER_MODULE, + API_FACTORY_MODULE, + STORAGE_HELPERS_MODULE, + FOLDER_TREE_MANAGER_MODULE, + I18N_HELPERS_MODULE, + SUMMARY_MODULE, + mockApiClient, + mockLoadingManager, + mockFolderTreeManager, + mockState, +} = vi.hoisted(() => { + const mockApiClient = { + modelType: 'loras', + apiConfig: { + config: { + displayName: 'LoRA', + singularName: 'lora', + }, + }, + }; + + const mockLoadingManager = { + showSimpleLoading: vi.fn(), + setStatus: vi.fn(), + hide: vi.fn(), + }; + + const mockFolderTreeManager = { + getSelectedPath: vi.fn(() => ''), + }; + + const mockState = { + global: { + settings: { + download_path_templates: {}, + }, + }, + loadingManager: mockLoadingManager, + }; + + return { + DOWNLOAD_MANAGER_MODULE: new URL('../../../static/js/managers/DownloadManager.js', import.meta.url).pathname, + MODAL_MANAGER_MODULE: new URL('../../../static/js/managers/ModalManager.js', import.meta.url).pathname, + UI_HELPERS_MODULE: new URL('../../../static/js/utils/uiHelpers.js', import.meta.url).pathname, + STATE_MODULE: new URL('../../../static/js/state/index.js', import.meta.url).pathname, + LOADING_MANAGER_MODULE: new URL('../../../static/js/managers/LoadingManager.js', import.meta.url).pathname, + API_FACTORY_MODULE: new URL('../../../static/js/api/modelApiFactory.js', import.meta.url).pathname, + STORAGE_HELPERS_MODULE: new URL('../../../static/js/utils/storageHelpers.js', import.meta.url).pathname, + FOLDER_TREE_MANAGER_MODULE: new URL('../../../static/js/components/FolderTreeManager.js', import.meta.url).pathname, + I18N_HELPERS_MODULE: new URL('../../../static/js/utils/i18nHelpers.js', import.meta.url).pathname, + SUMMARY_MODULE: new URL('../../../static/js/components/DownloadBatchSummaryModal.js', import.meta.url).pathname, + mockApiClient, + mockLoadingManager, + mockFolderTreeManager, + mockState, + }; +}); + +vi.mock(MODAL_MANAGER_MODULE, () => ({ + modalManager: { + showModal: vi.fn(), + closeModal: vi.fn(), + }, +})); + +vi.mock(UI_HELPERS_MODULE, () => ({ + showToast: vi.fn(), + showActionToast: vi.fn(), + setupAutoNewlineOnPaste: vi.fn(), +})); + +vi.mock(STATE_MODULE, () => ({ + state: mockState, +})); + +vi.mock(LOADING_MANAGER_MODULE, () => ({ + LoadingManager: vi.fn(() => mockLoadingManager), +})); + +vi.mock(API_FACTORY_MODULE, () => ({ + getModelApiClient: vi.fn(() => mockApiClient), + resetAndReload: vi.fn(), +})); + +vi.mock(STORAGE_HELPERS_MODULE, () => ({ + getStorageItem: vi.fn((_key, defaultValue) => defaultValue), + setStorageItem: vi.fn(), +})); + +vi.mock(FOLDER_TREE_MANAGER_MODULE, () => ({ + FolderTreeManager: vi.fn(() => mockFolderTreeManager), +})); + +vi.mock(I18N_HELPERS_MODULE, () => ({ + translate: vi.fn((_, __, fallback) => fallback ?? ''), +})); + +vi.mock(SUMMARY_MODULE, () => ({ + showDownloadBatchSummary: vi.fn(), +})); + +describe('DownloadManager default-path preview', () => { + let DownloadManager; + + beforeEach(async () => { + vi.resetModules(); + vi.clearAllMocks(); + ({ DownloadManager } = await import(DOWNLOAD_MANAGER_MODULE)); + + document.body.innerHTML = ` + +
+ `; + document.getElementById('modelRoot').value = '/models/vae'; + + mockState.global.settings.download_path_templates = {}; + }); + + afterEach(() => { + document.body.innerHTML = ''; + }); + + function useOtherClient(manager) { + mockApiClient.modelType = 'other'; + mockApiClient.apiConfig.config = { displayName: 'Other Model', singularName: 'other' }; + manager.apiClient = mockApiClient; + } + + it('renders the bare root for a flat (empty) other template', () => { + mockState.global.settings.download_path_templates = { other: '' }; + const manager = new DownloadManager(); + useOtherClient(manager); + manager.useDefaultPath = true; + + manager.updateTargetPath(); + + const text = document.getElementById('targetPathDisplay').textContent; + expect(text).toBe('/models/vae'); + expect(text).not.toContain('undefined'); + }); + + it('renders the bare root when the other template key is absent', () => { + mockState.global.settings.download_path_templates = {}; + const manager = new DownloadManager(); + useOtherClient(manager); + manager.useDefaultPath = true; + + manager.updateTargetPath(); + + const text = document.getElementById('targetPathDisplay').textContent; + expect(text).toBe('/models/vae'); + expect(text).not.toContain('undefined'); + }); + + it('appends a configured template to the root', () => { + mockState.global.settings.download_path_templates = { other: '{base_model}' }; + const manager = new DownloadManager(); + useOtherClient(manager); + manager.useDefaultPath = true; + + manager.updateTargetPath(); + + expect(document.getElementById('targetPathDisplay').textContent).toBe('/models/vae/{base_model}'); + }); + + it('renders the manual selection when default paths are off', () => { + mockFolderTreeManager.getSelectedPath.mockReturnValue('nested/folder'); + const manager = new DownloadManager(); + useOtherClient(manager); + manager.useDefaultPath = false; + + manager.updateTargetPath(); + + expect(document.getElementById('targetPathDisplay').textContent).toBe('/models/vae/nested/folder'); + }); +}); diff --git a/tests/services/test_download_manager_other.py b/tests/services/test_download_manager_other.py index 6ddc3b5e..0054fdf8 100644 --- a/tests/services/test_download_manager_other.py +++ b/tests/services/test_download_manager_other.py @@ -428,6 +428,29 @@ async def test_default_paths_use_per_sub_type_root( assert result["success"] is True assert str(tmp_path / "vae") in str(captured["save_dir"]) + assert captured["relative_path"] == "" + + +@pytest.mark.asyncio +async def test_default_paths_other_is_flat_without_configured_template( + monkeypatch, scanners, metadata_provider, tmp_path +): + """Regression: an unconfigured 'other' template must resolve to a flat + layout at the sub_type root instead of the {base_model}/{first_tag} + fallback (which scattered files into arbitrary CivitAI-tag folders).""" + get_settings_manager().settings["download_path_templates"].pop("other", None) + + captured = {} + _capture_execute(monkeypatch, captured) + + manager = DownloadManager() + result = await manager.download_from_civitai( + model_version_id=99, use_default_paths=True + ) + + assert result["success"] is True + assert captured["relative_path"] == "" + assert str(tmp_path / "vae") in str(captured["save_dir"]) @pytest.mark.asyncio diff --git a/tests/services/test_settings_manager.py b/tests/services/test_settings_manager.py index 0cbb2166..342e0a41 100644 --- a/tests/services/test_settings_manager.py +++ b/tests/services/test_settings_manager.py @@ -345,6 +345,26 @@ def test_download_path_template_invalid_json(manager): ) +def test_download_path_template_defaults_flat_for_other(manager): + """'other' has no settings-UI row and no priority_tags entry, so an + unconfigured template must resolve to a flat layout rather than the + {base_model}/{first_tag} fallback (which scatters files into arbitrary + CivitAI-tag folders).""" + manager.settings["download_path_templates"] = {} + + assert manager.get_download_path_template("other") == "" + + # An explicit user configuration still wins. + manager.settings["download_path_templates"] = {"other": "{base_model}"} + assert manager.get_download_path_template("other") == "{base_model}" + + +def test_download_path_template_unknown_type_is_flat(manager): + manager.settings["download_path_templates"] = {} + + assert manager.get_download_path_template("not-a-model-type") == "" + + def test_auto_set_default_roots(manager): # Clear any previously auto-set values to test fresh behavior manager.settings["default_lora_root"] = ""