From 1175ee363f64045e84fb3d0601a0710c1c2b99d5 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 23 Apr 2026 14:41:06 -0700 Subject: [PATCH] fix(models): duplicate dropdown entries, stale default model, lowercase injected label (#907 #908 #909) (#918) Co-authored-by: nesquena-hermes --- CHANGELOG.md | 7 + api/config.py | 41 ++++- static/panels.js | 2 + static/ui.js | 13 ++ .../test_issues_907_908_909_model_dropdown.py | 153 ++++++++++++++++++ 5 files changed, 211 insertions(+), 5 deletions(-) create mode 100644 tests/test_issues_907_908_909_model_dropdown.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 860cafa..9d44b6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,13 @@ workspace subtree) and never enumerate blocked system roots. (`api/routes.py`, `api/workspace.py`, `static/panels.js`, `static/style.css`) (partial for #616) +## [v0.50.176] — 2026-04-23 + +### Fixed +- **Duplicate model dropdown entries when CLI default matches live-fetched model** — `_addLiveModelsToSelect()` now normalises IDs before the dedup check (strips `@provider:` prefix using `indexOf`+`substring` to preserve multi-colon Ollama tag suffixes like `qwen3-vl:235b-instruct`, strips namespace prefix, unifies separators). (`static/ui.js`) Closes #907. +- **New Chat uses stale default model after saving Preferences without reload** — `window._defaultModel` is now updated in `_applySavedSettingsUi()` so `newSession()` picks up the newly saved default immediately. (`static/panels.js`) Closes #908. +- **Injected CLI default model shows raw lowercase label** — new `_get_label_for_model()` helper looks up the model's formatted label from existing catalog groups before falling back to title-casing the bare ID. (`api/config.py`) Closes #909. + ## [v0.50.175] — 2026-04-23 ### Fixed diff --git a/api/config.py b/api/config.py index 9bb5cc7..1fef4b3 100644 --- a/api/config.py +++ b/api/config.py @@ -1071,6 +1071,39 @@ def invalidate_models_cache(): _available_models_cache_ts = 0.0 +def _get_label_for_model(model_id: str, existing_groups: list) -> str: + """Return a human-friendly label for *model_id*. + + Resolution order: + 1. If the model already appears in *existing_groups* with a label, use it. + 2. Strip @provider: prefix and namespace prefix, then title-case. + + This ensures the injected default model entry in the dropdown always shows + the same label as the live-fetched or static-catalog version, rather than + the raw lowercase ID string (#909). + """ + # Strip @provider: prefix for lookup + lookup_id = model_id + if lookup_id.startswith("@") and ":" in lookup_id: + lookup_id = lookup_id.split(":", 1)[1] + + # Check existing groups for a matching label + _norm = lambda s: (s.split("/", 1)[-1] if "/" in s else s).replace("-", ".").lower() + norm_lookup = _norm(lookup_id) + for g in existing_groups: + for m in g.get("models", []): + if m.get("label") and _norm(str(m.get("id", ""))) == norm_lookup: + return m["label"] + + # Fall back: capitalize each hyphen-separated word, preserve dots in version numbers. + # The catalog lookup above handles well-known models; this only fires for unlisted IDs. + bare = lookup_id.split("/")[-1] if "/" in lookup_id else lookup_id + return " ".join( + w.upper() if (len(w) <= 3 and w.replace(".", "").isalnum() and not w.isdigit()) else w.capitalize() + for w in bare.replace("_", "-").split("-") + ) + + def get_available_models() -> dict: """ Return available models grouped by provider. @@ -1450,7 +1483,7 @@ def get_available_models() -> dict: _cp_model = _cp.get("model", "") _cp_name = (_cp.get("name") or "").strip() if _cp_model and _cp_model not in _seen_custom_ids: - _cp_label = _cp_model.split("/")[-1] if "/" in _cp_model else _cp_model + _cp_label = _get_label_for_model(_cp_model, []) _seen_custom_ids.add(_cp_model) if _cp_name: # Named custom provider — own group keyed by slug @@ -1584,7 +1617,7 @@ def get_available_models() -> dict: # can at least send messages with their current setting. Avoid showing a # generic multi-provider list — those models wouldn't be routable anyway. if default_model: - label = default_model.split("/")[-1] if "/" in default_model else default_model + label = _get_label_for_model(default_model, groups) groups.append( {"provider": "Default", "provider_id": "default", "models": [{"id": default_model, "label": label}]} ) @@ -1606,9 +1639,7 @@ def get_available_models() -> dict: # vs display name 'OpenAI Codex' (hyphen vs. space), which # silently falls through to groups[0] and lands the model in # the wrong group. - label = ( - default_model.split("/")[-1] if "/" in default_model else default_model - ) + label = _get_label_for_model(default_model, groups) target_display = ( _PROVIDER_DISPLAY.get(active_provider, active_provider or "").lower() if active_provider diff --git a/static/panels.js b/static/panels.js index 6d2669a..bfd3fde 100644 --- a/static/panels.js +++ b/static/panels.js @@ -1697,6 +1697,8 @@ function _applySavedSettingsUi(saved, body, opts){ const bar=$('settingsUnsavedBar'); if(bar) bar.style.display='none'; _settingsHermesDefaultModelOnOpen=body.default_model||_settingsHermesDefaultModelOnOpen||''; + // Sync window._defaultModel so newSession() uses the just-saved default without a reload (#908). + if(body.default_model) window._defaultModel=body.default_model; renderMessages(); if(typeof syncTopbar==='function') syncTopbar(); if(typeof renderSessionList==='function') renderSessionList(); diff --git a/static/ui.js b/static/ui.js index 191f553..b630e90 100644 --- a/static/ui.js +++ b/static/ui.js @@ -139,6 +139,18 @@ function _addLiveModelsToSelect(provider, models, sel){ sel.appendChild(providerGroup); } const existingIds=new Set([...sel.options].map(o=>o.value)); + // Normalized dedup: strip @provider: prefix and unify separators so + // 'minimax/minimax-m2.7' matches '@nous:minimax/minimax-m2.7' (#907). + // Strip ONLY the first colon — Ollama tag IDs are multi-colon + // (e.g. '@ollama-cloud:qwen3-vl:235b-instruct') and split(':',2) would + // truncate the tag suffix in JS (the limit arg discards extras, unlike Python). + const _normId=id=>{ + let s=String(id||''); + if(s.startsWith('@')&&s.includes(':')) s=s.substring(s.indexOf(':')+1); // strip only @provider: + s=s.split('/').pop(); // strip namespace prefix + return s.replace(/-/g,'.').toLowerCase(); + }; + const existingNorm=new Set([...sel.options].map(o=>_normId(o.value))); let added=0; const _ap=(window._activeProvider||'').toLowerCase(); const _isPortalFetch=_ap && _ap!=='openrouter' && _ap!=='custom' && provider===_ap; @@ -148,6 +160,7 @@ function _addLiveModelsToSelect(provider, models, sel){ mid=`@${provider}:${mid}`; } if(existingIds.has(mid)) continue; + if(existingNorm.has(_normId(mid))) continue; // dedup cross-prefix duplicates (#907) const opt=document.createElement('option'); opt.value=mid; opt.textContent=m.label||m.id; diff --git a/tests/test_issues_907_908_909_model_dropdown.py b/tests/test_issues_907_908_909_model_dropdown.py new file mode 100644 index 0000000..b10c0fa --- /dev/null +++ b/tests/test_issues_907_908_909_model_dropdown.py @@ -0,0 +1,153 @@ +"""Regression tests for issues #907, #908, #909 — model dropdown fixes.""" +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +UI_JS = (REPO_ROOT / "static" / "ui.js").read_text(encoding="utf-8") +PANELS_JS = (REPO_ROOT / "static" / "panels.js").read_text(encoding="utf-8") + + +# ── #907: Normalized dedup in _addLiveModelsToSelect ───────────────────────── + +class TestIssue907LiveModelDedup: + """Live-fetched models with @provider: prefix must not duplicate server-injected bare entries.""" + + def test_addLiveModelsToSelect_has_norm_dedup(self): + # _normId helper and existingNorm set must be present in _addLiveModelsToSelect + fn_idx = UI_JS.find('function _addLiveModelsToSelect(') + assert fn_idx != -1, "_addLiveModelsToSelect not found" + # Find the closing brace of the function (~800 chars is enough) + fn_body = UI_JS[fn_idx:fn_idx + 2000] + assert '_normId' in fn_body or 'existingNorm' in fn_body, ( + "_addLiveModelsToSelect must normalise IDs before dedup check (#907)" + ) + + def test_normId_strips_at_prefix(self): + # The _normId lambda/function must strip @provider: prefix + fn_idx = UI_JS.find('function _addLiveModelsToSelect(') + fn_body = UI_JS[fn_idx:fn_idx + 2000] + has_at_strip = ("startsWith('@')" in fn_body or "split(':'" in fn_body) + assert has_at_strip, ( + "_normId in _addLiveModelsToSelect must strip @provider: prefix for dedup (#907)" + ) + + def test_existingNorm_used_as_guard(self): + fn_idx = UI_JS.find('function _addLiveModelsToSelect(') + fn_body = UI_JS[fn_idx:fn_idx + 2000] + assert 'existingNorm.has(' in fn_body, ( + "_addLiveModelsToSelect must check existingNorm before appending (#907)" + ) + + def test_normId_handles_multi_colon_ollama_ids(self): + """_normId must strip ONLY the first colon so multi-colon Ollama tag IDs + (e.g. '@ollama-cloud:qwen3-vl:235b-instruct' vs bare 'qwen3-vl:235b-instruct') + still dedup correctly. JS `split(':',2)[1]` with limit=2 TRUNCATES in JS + (unlike Python's split), so the naive variant would lose the tag suffix + and mis-dedup. + """ + fn_idx = UI_JS.find('function _addLiveModelsToSelect(') + assert fn_idx != -1 + fn_body = UI_JS[fn_idx:fn_idx + 2000] + # The implementation must use indexOf/substring or split().slice(1).join(), + # not split(':', 2)[1] which truncates the tail. + good = 'indexOf' in fn_body or "slice(1).join(':')" in fn_body + assert good, ( + "_normId must strip only the first colon to preserve Ollama multi-colon " + "tag IDs (e.g. @ollama-cloud:qwen3-vl:235b-instruct). Use " + "substring(indexOf(':')+1) or split(':').slice(1).join(':') — NOT " + "split(':', 2)[1] which silently truncates in JS." + ) + assert "split(':',2)[1]" not in fn_body and "split(':', 2)[1]" not in fn_body, ( + "_normId still uses split(':', 2)[1] which truncates multi-colon IDs in JS; " + "use indexOf/substring instead." + ) + + +# ── #908: window._defaultModel updated on settings save ───────────────────── + +class TestIssue908DefaultModelSync: + """window._defaultModel must be updated when the user saves a new default in Preferences.""" + + def test_applySavedSettingsUi_updates_window_defaultModel(self): + fn_idx = PANELS_JS.find('function _applySavedSettingsUi(') + assert fn_idx != -1, "_applySavedSettingsUi not found" + # Find the end of the function (next function definition) + fn_end = PANELS_JS.find('\nasync function saveSettings(', fn_idx) + fn_body = PANELS_JS[fn_idx:fn_end] + assert 'window._defaultModel' in fn_body, ( + "_applySavedSettingsUi must update window._defaultModel so newSession() " + "uses the newly saved default without a page reload (#908)" + ) + + def test_defaultModel_update_conditioned_on_body_default_model(self): + fn_idx = PANELS_JS.find('function _applySavedSettingsUi(') + fn_end = PANELS_JS.find('\nasync function saveSettings(', fn_idx) + fn_body = PANELS_JS[fn_idx:fn_end] + # Must be guarded so we don't clear _defaultModel when body.default_model is absent + assert "if(body.default_model)" in fn_body or "body.default_model &&" in fn_body, ( + "window._defaultModel assignment must be conditional on body.default_model being set" + ) + + +# ── #909: Injected default model label quality ─────────────────────────────── + +class TestIssue909InjectedModelLabel: + """The server must use a proper label for the injected default model (not raw lowercase ID).""" + + def test_get_label_for_model_helper_exists(self): + import api.config as config + assert hasattr(config, '_get_label_for_model'), ( + "api/config.py must define _get_label_for_model() for the injected default label (#909)" + ) + + def test_label_helper_capitalizes_bare_id(self): + from api.config import _get_label_for_model + label = _get_label_for_model('minimax/minimax-m2.7', []) + assert label != 'minimax-m2.7', ( + "_get_label_for_model should not return the raw lowercase ID (#909)" + ) + # Should capitalize: "Minimax M2.7" or similar + assert label[0].isupper(), "Label should start with an uppercase letter" + + def test_label_helper_uses_catalog_when_available(self): + from api.config import _get_label_for_model + existing_groups = [ + {"provider": "Nous", "models": [ + {"id": "minimax/minimax-m2.7", "label": "Minimax M2.7 (Nous)"} + ]} + ] + label = _get_label_for_model('minimax/minimax-m2.7', existing_groups) + assert label == "Minimax M2.7 (Nous)", ( + "_get_label_for_model should prefer catalog label over generated one" + ) + + def test_label_helper_strips_at_prefix_for_lookup(self): + from api.config import _get_label_for_model + existing_groups = [ + {"provider": "Nous", "models": [ + {"id": "minimax/minimax-m2.7", "label": "Minimax M2.7"} + ]} + ] + # @nous:minimax/minimax-m2.7 should match minimax/minimax-m2.7 in catalog + label = _get_label_for_model('@nous:minimax/minimax-m2.7', existing_groups) + assert label == "Minimax M2.7", ( + "_get_label_for_model must strip @provider: prefix before catalog lookup" + ) + + def test_config_uses_label_helper_not_raw_split(self): + from pathlib import Path + config_src = (Path(__file__).resolve().parent.parent / "api" / "config.py").read_text() + # The raw label-building pattern should be replaced by the helper + assert "_get_label_for_model" in config_src, ( + "api/config.py must call _get_label_for_model() for injected default model labels (#909)" + ) + # The old raw pattern should NOT be present in the injection block + old_pattern = 'default_model.split("/")[-1] if "/" in default_model else default_model' + label_sections = [ + config_src[i:i+200] + for i in [m.start() for m in re.finditer(r'label\s*=\s*', config_src)] + ] + for sec in label_sections: + assert old_pattern not in sec, ( + "api/config.py still uses raw split-based label for injected default model (#909)" + )