fix(models): duplicate dropdown entries, stale default model, lowercase injected label (#907 #908 #909) (#918)
Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
This commit is contained in:
nesquena-hermes
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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();
|
||||
|
||||
13
static/ui.js
13
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;
|
||||
|
||||
153
tests/test_issues_907_908_909_model_dropdown.py
Normal file
153
tests/test_issues_907_908_909_model_dropdown.py
Normal file
@@ -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)"
|
||||
)
|
||||
Reference in New Issue
Block a user