docs: detail Phase 2 download design for Other Models page

docs/plans/other-models-page.md

@@ -1,7 +1,7 @@
 # Plan: "Other Models" Page — Unified Management for VAE / Upscaler / Text Encoder / etc.
 
-**Status:** v1 — decisions locked with feature owner (2026-09-12); ready for implementation.
-**Scope (Phase 1):** scan + manage (list, search, filter, tags, folders, preview, rename, move, delete/exclude, CivitAI metadata fetch) for a new model type `other`, exposed as a new web page. **Phase 2 (out of scope here, outlined in §9):** one-click download from CivitAI for these types.
+**Status:** v2 — **Phase 1 implemented** (2026-09-12, commits `27da7b3c` backend + `fa7ce725` frontend; verified live against a running ComfyUI instance: scan/hash/sub_type-derivation/fetch/previews all green). **Phase 2 designed** (§9, decisions pending owner sign-off).
+**Scope (Phase 1):** scan + manage (list, search, filter, tags, folders, preview, rename, move, delete/exclude, CivitAI metadata fetch) for a new model type `other`, exposed as a new web page. **Phase 2 (§9):** one-click download from CivitAI for these types.
 
 ## 1. Goal
 
@@ -152,14 +152,87 @@ Follow existing conventions (`pytest.ini`, `tests/frontend/` vitest):
 6. i18n keys + sync script
 7. `pytest` + `npm test` full runs; hand to user for manual UI check
 
-## 9. Phase 2 Outline (NOT in this implementation)
+## 9. Phase 2 Detailed Design — CivitAI Downloads for `other`
 
-CivitAI downloads for `other` types:
+Designed 2026-09-12 against the Phase-1 code on this branch; decisions marked **[locked]** follow the same recommendations the feature owner approved for Phase 1.
 
-- `py/services/download_manager.py`: extend the type map (`:1496-1507` — currently rejects unknown types), `_get_scanner_for_model_type` (`:230-236`), `_build_metadata_for_resume` (`:977-981`).
-- **Default-root selection** (`:1699-1727`): needs per-sub_type settings keys (`default_vae_root`, `default_upscaler_root`, …) — this is what pulls in `settings_manager.py` (library default keys `:82-85`, `_check_and_auto_set` ~`:890`, `set()` dispatch `:1621-1628`, `upsert_library` params, library-switch scanner notifications `:1795-1799`/`:2150-2155`).
-- File-type routing: consult CivitAI's `primaryFileTypesByModelType` (e.g. CLIP-type models ship `Text Encoder`/`Vision Encoder` primary files, not `Model`); a `file.type → target folder` map analogous to `download_routing.py:is_diffusion_model_download`.
-- Settings UI for the new default roots.
+### 9.1 Download pipeline touch points
+
+Flow: `POST /api/lm/download-model` (`py/routes/model_route_registrar.py:104`; GET variant `:105` for the browser extension) → `ModelDownloadHandler.download_model` (`model_handlers.py:1740`) → `DownloadModelUseCase.execute` → `DownloadCoordinator.schedule_download` → `DownloadManager.download_from_civitai` (`download_manager.py:386`) → `_execute_original_download` (`:1415`). Inside, seven scatter points need an `other` branch:
+
+1. **Type map** (`:1496-1507`): accept `model.type.lower() in VALID_OTHER_CIVITAI_TYPES` → `model_type = "other"` (reuses the Phase-1 set, incl. `"other"` itself).
+2. **Early version-exists gate** (`:1436-1463`): add `other_scanner.check_model_version_exists`.
+3. **File-level exists gate** (`:1640-1655` → `_find_local_file_entry` `:320-346` → `_get_scanner_for_model_type` `:230-236`): add explicit `other` branch. **Trap**: the function currently falls through to the lora scanner for unknown types — `"other"` would silently dedupe against loras. Also narrow the fall-through to `"lora"` only / raise on unknown.
+4. **Version-level fallback gate** (`:1656-1688`): add `elif model_type == "other"`.
+5. **Default-root selection** (`:1690-1727`): for `other`, first resolve sub_type (§9.2), then read `default_other_roots[sub_type]` (§9.3); if sub_type is undecidable or no default root configured → error guiding the user to pick a folder explicitly.
+6. **Metadata class selection** (`:1909-1928`) + `_build_metadata_for_resume` (`:969-981`): add `OtherModelMetadata.from_civitai_info` branches.
+7. **Post-download cache write** (`_execute_download_pipeline` `:2622-2679`): add `other` scanner branch; `adjust_metadata` re-derives sub_type from the on-disk root automatically. `_get_supported_extensions_for_type` (`:2720-2744`): `other` reuses the checkpoint extension set.
+
+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).
+
+### 9.2 File-level routing (model.type / file.type → sub_type) **[locked]**
+
+Table-driven, mirroring Phase 1. New in `py/utils/constants.py`:
+
+```python
+CIVITAI_FILE_TYPE_TO_OTHER_SUB_TYPE = {
+    "VAE": "vae", "Upscaler": "upscaler", "Text Encoder": "text_encoder",
+    "Vision Encoder": "clip_vision", "CLIPVision": "clip_vision",
+    "ControlNet": "controlnet",
+}
+```
+
+`download_routing.py` gains `resolve_other_download_sub_type(civitai_model_type, file_types, selected_file_type=None)` with fixed priority:
+
+1. **Explicit user file pick** (`file_params` from #1058's `_resolve_target_file`) — if the picked file's type maps, it wins even when model.type is `Checkpoint`.
+2. **model.type** via the existing `CIVITAI_TYPE_TO_OTHER_SUB_TYPE` (`constants.py:120-127`).
+3. **file.type fallback** — only when model.type maps to nothing (e.g. model.type `Other` or retired `CLIP`). MUST NOT override a mapped model.type: checkpoint models routinely bundle VAE/Text Encoder component files, and unconditional file-type routing would misroute them.
+4. Still undecidable → `None`; `use_default_paths` errors and the UI offers all other roots for manual selection.
+
+HTTP: extend `DownloadRoutingHandler.get_download_routing` (`download_routing_handlers.py:23`) with an `other` branch returning `{root_kind: "other", sub_type: ...}`; add `GET /api/lm/other/roots_by_subtype` in `OtherRoutes.setup_specific_routes` (data from `config._prepare_other_paths`'s per-key roots, aggregating `text_encoders` + legacy `clip` under `text_encoder`).
+
+### 9.3 Settings: single dict key `default_other_roots` **[locked]**
+
+Rejected: four flat keys (`default_vae_root`…) — each flat key costs ~13 touch points in `settings_manager.py` (defaults `:82-85`, `_check_and_auto_set` `:890-895`, `set()` `:1621-1628`, `_update_active_library_entry` `:738-805`, upsert/create signatures `:1953-2132`, `_build_library_payload` `:552-612`, `_sync_active_library_to_root` `:519-547`, three library constructors, frontend `DEFAULT_SETTINGS_BASE`), repeated per future sub_type.
+
+Chosen: one mapping key `default_other_roots: {sub_type: path}`, copying the `extra_folder_paths` precedent (generic Mapping handling at `:533-535`, `:573-578`, `:763-767`). `_check_and_auto_set` generalizes to per-sub_type candidates (union over that sub_type's folder keys — `text_encoder` → `text_encoders` + `clip`). `set()` validates keys against `VALID_OTHER_SUB_TYPES`.
+
+Also fix the Phase-1 omission: add `"other_scanner"` to `_notify_library_change` (`:2150-2156`) and `_notify_model_name_display_change` (`:1795-1800`) — otherwise switching libraries leaves the other page stale.
+
+### 9.4 Settings UI
+
+- `templates/components/modals/settings/library.html:34-40`: sub_type selectors after the existing four `setting_select`s (Jinja loop; controlnet selector only when `enabled_other_folders` includes it). Dict-subkey save helper `saveOtherRootSetting(subType, value)` alongside the flat `saveSelectSetting`.
+- `static/js/managers/SettingsManager.js:1547-1697`: `loadOtherRoots()` mirroring `loadUnetRoots()`, fed by `/api/lm/other/roots_by_subtype`; current values from `state.global.settings.default_other_roots`. `state/index.js:24` `DEFAULT_SETTINGS_BASE` += `default_other_roots: {}`.
+- Optional: one `other` row in the download-path-template block (`library.html:153-211`).
+- i18n: `settings.folderSettings.*` keys into `locales/en.json` + sync script; other locales keep `[TODO: Translate]`.
+- Settings GET (`misc_handlers.py:1528-1536`) already returns all non-sensitive keys — new key reaches the frontend for free.
+
+### 9.5 Frontend download entry
+
+- `templates/components/controls.html:83`: drop the `page_id != 'other'` exclusion on the download button (keyboard shortcut D self-enables via `PageControls.js:196-198`).
+- `OtherControls.js:22-55`: add `showDownloadModal: () => downloadManager.showDownloadModal()` (mirror `EmbeddingsControls.js:43-45`).
+- `DownloadManager.js` `proceedToLocationContent` (`:955-1017`): add `_resolveOtherSubType()` (mirror `_resolveIsDiffusionModel` `:1026`): selected file type → `/api/lm/download/routing` → `otherApiClient.fetchModelRoots(subType)` (new); default-root preselect reads `default_other_roots[subType]` instead of `` `default_${singularType}_root` `` (`:974`). Undecidable → list all other roots (`/api/lm/other/roots`) for manual pick; an explicit save_dir skips backend default-root logic, so the two paths cannot disagree.
+- `ModelVersionsTab` download buttons are modelType-generic and already work via `getModelApiClient('other')`; context menu has no CivitAI download entry — no change.
+- Version-list type validation (`get_civitai_versions` → `_validate_civitai_model_type`) already accepts `VALID_OTHER_CIVITAI_TYPES` from Phase 1.
+
+### 9.6 CivitAI type mapping decisions **[locked]**
+
+- Download accepts exactly `VALID_OTHER_CIVITAI_TYPES` (`VAE, Upscaler, TextEncoder, CLIP, CLIPVision, Controlnet, Other`) — reuse the Phase-1 tables; do NOT create new ones.
+- Extend `CIVITAI_USER_MODEL_TYPES` (`constants.py:133-137`) with the 7 aliases, and point them at the other scanner / `"other"` history bucket in `misc_handlers.py` (`type_scanner_map` `:2793-2797`, `downloaded_version_map` `:2821-2827`) — otherwise creator pages silently filter these models while downloads claim support.
+- Fix (small Phase-1 bug): `OtherModelMetadata.from_civitai_info` (`py/utils/models.py:343`) reads `version_info.get("type")`, but the type lives at `version["model"]["type"]` — the mapping never fires and always degrades to the placeholder. Read `version_info.get("model", {}).get("type")` instead. (`CheckpointMetadata:290` has the same shape; leave it alone here.)
+
+### 9.7 Tests
+
+Existing base: `tests/services/test_download_manager_basic.py` (incl. `test_download_rejects_unsupported_model_type` `:1336`), `test_download_manager_error.py`, `test_download_manager_concurrent.py`, `tests/integration/test_download_flow.py`, `tests/services/test_settings_manager.py`; frontend `tests/frontend/managers/downloadManager.routing.test.js`, `settingsManager.library.test.js`.
+
+Add: (1) `resolve_other_download_sub_type` unit tests — every priority tier, bundled-component anti-misrouting, undecidable → None, civarchive-shaped payload; (2) download_manager — six model.types accepted → other scanner (mock), unknown still rejected, no lora-scanner fall-through, per-sub_type default roots + unconfigured error, resume metadata, extension set; (3) settings_manager — `default_other_roots` defaults/auto-set (incl. text_encoder dual-key union)/library sync/upsert passthrough/illegal sub_type rejection; (4) routes — `/api/lm/download/routing` other branch, `roots_by_subtype` shape; (5) example-images dispatch accepts `other` (3 sites); (6) vitest — `_resolveOtherSubType` + root select + default preselect, `loadOtherRoots`; (7) user-models existsLocally for VAE.
+
+### 9.8 Phase 2 file list
+
+Backend: `py/utils/constants.py`, `py/services/download_routing.py`, `py/routes/handlers/download_routing_handlers.py`, `py/services/download_manager.py`, `py/utils/example_images_download_manager.py`, `py/services/settings_manager.py`, `py/utils/models.py`, `py/routes/other_routes.py`, `py/routes/handlers/misc_handlers.py`, `settings.json.example`.
+Frontend/templates: `templates/components/controls.html`, `static/js/components/controls/OtherControls.js`, `static/js/managers/DownloadManager.js`, `static/js/api/otherApi.js`, `templates/components/modals/settings/library.html`, `static/js/managers/SettingsManager.js`, `static/js/state/index.js`, `locales/en.json` + sync.
 
 ## 10. Risks / Open Questions
 
@@ -168,3 +241,12 @@ CivitAI downloads for `other` types:
 - **Retired CivitAI types**: `CLIP`/`CLIPVision` are retired upstream (grandfathered for existing models); metadata fetch must tolerate both retired and current types — `VALID_OTHER_CIVITAI_TYPES` includes them deliberately.
 - **Standalone users** must add the new `folder_paths` keys to `settings.json` themselves; document in `settings.json.example` and the feature doc.
 - **Page display name** is i18n-only; if "Other Models" tests poorly, rename `other.title` without code changes.
+
+### Phase 2 risks
+
+- **Bundled component files**: checkpoint models routinely ship VAE/Text Encoder component files — file.type routing must stay a fallback (or explicit user pick), never an override (§9.2 priority is load-bearing; test it).
+- **`_get_scanner_for_model_type` lora fall-through** (`download_manager.py:236`): without an explicit `other` branch, dedupe checks run against the lora scanner — the most insidious trap in Phase 2.
+- **text_encoder dual folder keys** (`text_encoders` + legacy `clip`): default-root candidates, `roots_by_subtype`, and auto-set must all merge both keys; miss one and the default-root dropdown comes up empty.
+- **Undecidable sub_type** (model.type `Other` + unknown file types): must error and ask, never silently default to the vae folder.
+- **Lazy hash after download**: downloads carry CivitAI SHA256 (no recompute needed) — ensure the post-download cache write doesn't leave `hash_status="pending"`, or the next metadata fetch re-hashes a 10 GB file.
+- **CivArchive source**: same `_execute_original_download` path, same payload shape — cover it once in tests.