From 8666fd7635bab1f66d82d180e5afffa89a57e8ba Mon Sep 17 00:00:00 2001 From: David Doan Date: Tue, 16 Jun 2026 21:08:54 +0000 Subject: [PATCH 1/2] fix(desktop): preserve other providers' hide-all in model visibility dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #43496 added a per-provider hide-all sentinel ('provider::') so emptying a provider in the Edit Models dialog stopped re-expanding its defaults. That fixed the single-provider case, but the dialog's toggle handler seeds its working set from effectiveVisibleKeys(), which strips ALL sentinels before returning. So persisting after any toggle silently dropped every OTHER provider's hide-all sentinel; those providers then looked 'never customized' and re-enabled all their models on the next render. Split resolution into two functions: - resolveVisibleKeys(): stored keys + curated default expansion, with hide-all sentinels PRESERVED — the canonical working set the toggle handler mutates and persists. - effectiveVisibleKeys(): resolveVisibleKeys() then strips sentinels, for display only (unchanged contract). Move the toggle set-computation into a pure, unit-tested toggleModelVisibility() that seeds from resolveVisibleKeys(), so sibling sentinels survive the persist. Add regression tests that drive the real toggle handler across multiple providers. Follow-up to #43496; completes the fix for #43485 (cross-provider case). --- .../components/model-visibility-dialog.tsx | 25 +------ .../src/store/model-visibility.test.ts | 65 ++++++++++++++++++- apps/desktop/src/store/model-visibility.ts | 56 +++++++++++++++- 3 files changed, 120 insertions(+), 26 deletions(-) diff --git a/apps/desktop/src/components/model-visibility-dialog.tsx b/apps/desktop/src/components/model-visibility-dialog.tsx index 0b92dba36fb..05a5e92cb3a 100644 --- a/apps/desktop/src/components/model-visibility-dialog.tsx +++ b/apps/desktop/src/components/model-visibility-dialog.tsx @@ -14,10 +14,9 @@ import { $visibleModels, collapseModelFamilies, effectiveVisibleKeys, - emptyProviderSentinelKey, - isProviderSentinel, modelVisibilityKey, - setVisibleModels + setVisibleModels, + toggleModelVisibility } from '@/store/model-visibility' import type { ModelOptionProvider, ModelOptionsResponse } from '@/types/hermes' @@ -61,25 +60,7 @@ export function ModelVisibilityDialog({ const visible = effectiveVisibleKeys(stored, providers) const toggle = (provider: ModelOptionProvider, model: string) => { - const next = new Set(effectiveVisibleKeys($visibleModels.get(), providers)) - const key = modelVisibilityKey(provider.slug, model) - const sentinel = emptyProviderSentinelKey(provider.slug) - - if (next.has(key)) { - next.delete(key) - - // Check if this was the last real model for this provider. - const remainingForProvider = [...next].some(k => k.startsWith(`${provider.slug}::`) && !isProviderSentinel(k)) - - if (!remainingForProvider) { - next.add(sentinel) - } - } else { - next.delete(sentinel) - next.add(key) - } - - setVisibleModels(next) + setVisibleModels(toggleModelVisibility($visibleModels.get(), providers, provider.slug, model)) } const q = search.trim().toLowerCase() diff --git a/apps/desktop/src/store/model-visibility.test.ts b/apps/desktop/src/store/model-visibility.test.ts index 90eccdf457e..446a61f874e 100644 --- a/apps/desktop/src/store/model-visibility.test.ts +++ b/apps/desktop/src/store/model-visibility.test.ts @@ -7,7 +7,9 @@ import { effectiveVisibleKeys, emptyProviderSentinelKey, isProviderSentinel, - modelVisibilityKey + modelVisibilityKey, + resolveVisibleKeys, + toggleModelVisibility } from './model-visibility' const provider = (slug: string, models: string[]): ModelOptionProvider => ({ @@ -96,4 +98,65 @@ describe('model visibility', () => { expect(isProviderSentinel('openai::')).toBe(true) expect(isProviderSentinel('openai::gpt-4o')).toBe(false) }) + + it('resolveVisibleKeys preserves sentinels that effectiveVisibleKeys strips', () => { + const stored = new Set([emptyProviderSentinelKey('nous')]) + const providers = [provider('nous', ['hermes-x', 'hermes-y']), provider('ollama', ['qwen3:latest'])] + + const resolved = resolveVisibleKeys(stored, providers) + expect(resolved.has(emptyProviderSentinelKey('nous'))).toBe(true) + expect(resolved.has(modelVisibilityKey('nous', 'hermes-x'))).toBe(false) + // Un-customized providers still expand to their defaults. + expect(resolved.has(modelVisibilityKey('ollama', 'qwen3:latest'))).toBe(true) + + // Display variant drops the sentinel. + expect(effectiveVisibleKeys(stored, providers).has(emptyProviderSentinelKey('nous'))).toBe(false) + }) +}) + +describe('toggleModelVisibility', () => { + const providers = [provider('openai', ['gpt-a', 'gpt-b']), provider('nous', ['hermes-x', 'hermes-y'])] + + // Drive the handler the way the dialog does: feed each result back in as the + // next `stored`, so the persisted set is what the next toggle starts from. + const apply = (stored: Set | null, slug: string, model: string) => + toggleModelVisibility(stored, providers, slug, model) + + it('records a hide-all sentinel when the last model of a provider is toggled off', () => { + let stored: Set | null = null + stored = apply(stored, 'openai', 'gpt-a') + stored = apply(stored, 'openai', 'gpt-b') + + expect(stored.has(emptyProviderSentinelKey('openai'))).toBe(true) + expect(effectiveVisibleKeys(stored, providers).has(modelVisibilityKey('openai', 'gpt-a'))).toBe(false) + expect(effectiveVisibleKeys(stored, providers).has(modelVisibilityKey('openai', 'gpt-b'))).toBe(false) + }) + + it('keeps a hidden provider hidden when a different provider is toggled (regression for #43485)', () => { + // Hide ALL of nous — its sentinel is now stored. + let stored: Set | null = null + stored = apply(stored, 'nous', 'hermes-x') + stored = apply(stored, 'nous', 'hermes-y') + expect(stored.has(emptyProviderSentinelKey('nous'))).toBe(true) + + // Toggle a model in another provider. nous must NOT snap back on. + stored = apply(stored, 'openai', 'gpt-a') + + expect(stored.has(emptyProviderSentinelKey('nous'))).toBe(true) + const visible = effectiveVisibleKeys(stored, providers) + expect(visible.has(modelVisibilityKey('nous', 'hermes-x'))).toBe(false) + expect(visible.has(modelVisibilityKey('nous', 'hermes-y'))).toBe(false) + }) + + it('clears only the toggled provider sentinel when a model is re-enabled', () => { + let stored: Set | null = new Set([emptyProviderSentinelKey('openai'), emptyProviderSentinelKey('nous')]) + + stored = apply(stored, 'openai', 'gpt-a') + + expect(stored.has(emptyProviderSentinelKey('openai'))).toBe(false) + expect(stored.has(emptyProviderSentinelKey('nous'))).toBe(true) + const visible = effectiveVisibleKeys(stored, providers) + expect(visible.has(modelVisibilityKey('openai', 'gpt-a'))).toBe(true) + expect(visible.has(modelVisibilityKey('nous', 'hermes-x'))).toBe(false) + }) }) diff --git a/apps/desktop/src/store/model-visibility.ts b/apps/desktop/src/store/model-visibility.ts index 5c2b568c596..c5611dc274f 100644 --- a/apps/desktop/src/store/model-visibility.ts +++ b/apps/desktop/src/store/model-visibility.ts @@ -116,9 +116,12 @@ export function defaultVisibleKeys(providers: readonly ModelOptionProvider[]): S return keys } -/** Resolve which keys are currently visible: the user's explicit set when - * configured, otherwise the curated default for the given providers. */ -export function effectiveVisibleKeys( +/** Resolve the canonical working set: the user's stored keys plus the curated + * default expansion for any provider they haven't customized. Hide-all + * sentinels are PRESERVED here — this is the set the toggle handler mutates and + * persists, so dropping a sentinel would silently re-enable a provider the user + * emptied. Use `effectiveVisibleKeys` for display (sentinels stripped). */ +export function resolveVisibleKeys( stored: Set | null, providers: readonly ModelOptionProvider[] ): Set { @@ -134,9 +137,11 @@ export function effectiveVisibleKeys( for (const provider of providers) { const providerPrefix = `${provider.slug}::` + const hasStoredProvider = [...stored].some( key => key.startsWith(providerPrefix) && !isProviderSentinel(key) ) + const hasSentinel = stored.has(emptyProviderSentinelKey(provider.slug)) if (hasStoredProvider || hasSentinel) { @@ -150,6 +155,17 @@ export function effectiveVisibleKeys( } } + return next +} + +/** Resolve which keys are currently visible for DISPLAY: the resolved working + * set with bookkeeping sentinels stripped (they are not real models). */ +export function effectiveVisibleKeys( + stored: Set | null, + providers: readonly ModelOptionProvider[] +): Set { + const next = resolveVisibleKeys(stored, providers) + // Strip sentinel keys — they are bookkeeping, not real visibility entries. for (const key of [...next]) { if (isProviderSentinel(key)) { @@ -159,3 +175,37 @@ export function effectiveVisibleKeys( return next } + +/** Compute the next persisted visibility set when one model row is toggled. + * Seeds from `resolveVisibleKeys` (NOT `effectiveVisibleKeys`) so other + * providers' hide-all sentinels survive the persist. When the last visible + * model of a provider is toggled off, a sentinel records the explicit + * hide-all; re-enabling any model clears that provider's sentinel. */ +export function toggleModelVisibility( + stored: Set | null, + providers: readonly ModelOptionProvider[], + providerSlug: string, + model: string +): Set { + const next = new Set(resolveVisibleKeys(stored, providers)) + const key = modelVisibilityKey(providerSlug, model) + const sentinel = emptyProviderSentinelKey(providerSlug) + + if (next.has(key)) { + next.delete(key) + + // Check if this was the last real model for this provider. + const remainingForProvider = [...next].some( + k => k.startsWith(`${providerSlug}::`) && !isProviderSentinel(k) + ) + + if (!remainingForProvider) { + next.add(sentinel) + } + } else { + next.delete(sentinel) + next.add(key) + } + + return next +} From 461fcc096479f548a1990fe26f329649fe40c371 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 21 Jun 2026 15:46:58 +0530 Subject: [PATCH 2/2] test(desktop): harden model-visibility toggle + dedupe default expansion Follow-up to the salvaged #47450 fix: - Extract expandProviderDefaults() so the curated-default expansion rule lives in one place (was duplicated between defaultVisibleKeys and resolveVisibleKeys). - Drop the redundant new Set() wrap in toggleModelVisibility (resolveVisibleKeys already returns a fresh Set; effectiveVisibleKeys already relied on this). - Document the intentional re-enable behavior (re-enabling one model of a hidden-all provider restores only that model, not the curated defaults) and tighten the toggleModelVisibility JSDoc. - Add 7 hardening tests: re-enable-restores-only-that-model, full hide/re-enable round-trip, empty-non-null stored, single toggle-off from null defaults, zero-model provider, and direct resolveVisibleKeys null/empty assertions. --- .../src/store/model-visibility.test.ts | 69 +++++++++++++++++++ apps/desktop/src/store/model-visibility.ts | 32 +++++---- 2 files changed, 89 insertions(+), 12 deletions(-) diff --git a/apps/desktop/src/store/model-visibility.test.ts b/apps/desktop/src/store/model-visibility.test.ts index 446a61f874e..805493cd5bc 100644 --- a/apps/desktop/src/store/model-visibility.test.ts +++ b/apps/desktop/src/store/model-visibility.test.ts @@ -4,6 +4,7 @@ import type { ModelOptionProvider } from '@/types/hermes' import { collapseModelFamilies, + defaultVisibleKeys, effectiveVisibleKeys, emptyProviderSentinelKey, isProviderSentinel, @@ -159,4 +160,72 @@ describe('toggleModelVisibility', () => { expect(visible.has(modelVisibilityKey('openai', 'gpt-a'))).toBe(true) expect(visible.has(modelVisibilityKey('nous', 'hermes-x'))).toBe(false) }) + + it('re-enabling one model of a hidden-all provider restores ONLY that model, not the curated defaults', () => { + // openai hidden-all, nous untouched. + let stored: Set | null = new Set([emptyProviderSentinelKey('openai')]) + + stored = apply(stored, 'openai', 'gpt-a') + + const visible = effectiveVisibleKeys(stored, providers) + expect(visible.has(modelVisibilityKey('openai', 'gpt-a'))).toBe(true) + // gpt-b is NOT restored — "you hid everything, you get back only what you re-enable". + expect(visible.has(modelVisibilityKey('openai', 'gpt-b'))).toBe(false) + }) + + it('re-hiding the last re-enabled model re-adds the sentinel (full round-trip)', () => { + let stored: Set | null = new Set([emptyProviderSentinelKey('openai')]) + + // Re-enable gpt-a (clears sentinel, set = {gpt-a}), then toggle it back off. + stored = apply(stored, 'openai', 'gpt-a') + expect(stored.has(emptyProviderSentinelKey('openai'))).toBe(false) + stored = apply(stored, 'openai', 'gpt-a') + + expect(stored.has(emptyProviderSentinelKey('openai'))).toBe(true) + expect(effectiveVisibleKeys(stored, providers).has(modelVisibilityKey('openai', 'gpt-a'))).toBe(false) + }) + + it('toggling from an empty (non-null) stored set adds the model without expanding defaults', () => { + // Empty-but-not-null = "everything hidden". resolveVisibleKeys short-circuits to {}. + const stored = new Set() + + const next = apply(stored, 'openai', 'gpt-a') + + expect(next.has(modelVisibilityKey('openai', 'gpt-a'))).toBe(true) + // No curated defaults were expanded for any provider. + expect(next.has(modelVisibilityKey('openai', 'gpt-b'))).toBe(false) + expect(next.has(modelVisibilityKey('nous', 'hermes-x'))).toBe(false) + }) + + it('toggling off one default model from null stored keeps the rest of the curated defaults', () => { + // null = "never customized": resolveVisibleKeys expands all defaults first. + const next = apply(null, 'openai', 'gpt-a') + + expect(next.has(modelVisibilityKey('openai', 'gpt-a'))).toBe(false) + expect(next.has(modelVisibilityKey('openai', 'gpt-b'))).toBe(true) + expect(next.has(modelVisibilityKey('nous', 'hermes-x'))).toBe(true) + // Other models remain, so no sentinel. + expect(next.has(emptyProviderSentinelKey('openai'))).toBe(false) + }) + + it('tolerates a provider with zero models (defensive — dialog filters these out)', () => { + const ps = [provider('empty', []), provider('openai', ['gpt-a'])] + const next = toggleModelVisibility(new Set([modelVisibilityKey('openai', 'gpt-a')]), ps, 'empty', 'ghost') + + // No crash; the phantom key is recorded but no defaults are invented. + expect([...next].some(k => k.startsWith('empty::') && !isProviderSentinel(k))).toBe(true) + expect(next.has(modelVisibilityKey('openai', 'gpt-a'))).toBe(true) + }) +}) + +describe('resolveVisibleKeys', () => { + const providers = [provider('openai', ['gpt-a', 'gpt-b']), provider('nous', ['hermes-x', 'hermes-y'])] + + it('returns the curated defaults verbatim for null stored', () => { + expect(resolveVisibleKeys(null, providers)).toEqual(defaultVisibleKeys(providers)) + }) + + it('returns an empty set for an empty (non-null) stored set', () => { + expect([...resolveVisibleKeys(new Set(), providers)]).toEqual([]) + }) }) diff --git a/apps/desktop/src/store/model-visibility.ts b/apps/desktop/src/store/model-visibility.ts index c5611dc274f..44f15b4c32a 100644 --- a/apps/desktop/src/store/model-visibility.ts +++ b/apps/desktop/src/store/model-visibility.ts @@ -106,16 +106,23 @@ export function defaultVisibleKeys(providers: readonly ModelOptionProvider[]): S const keys = new Set() for (const provider of providers) { - const families = collapseModelFamilies(provider.models ?? []) - - for (const family of families.slice(0, DEFAULT_VISIBLE_PER_PROVIDER)) { - keys.add(modelVisibilityKey(provider.slug, family.id)) - } + expandProviderDefaults(provider, keys) } return keys } +/** Add a provider's curated default model keys (top-N collapsed families) to + * `target`. Shared by `defaultVisibleKeys` and `resolveVisibleKeys` so the + * expansion rule lives in exactly one place. */ +function expandProviderDefaults(provider: ModelOptionProvider, target: Set): void { + const families = collapseModelFamilies(provider.models ?? []) + + for (const family of families.slice(0, DEFAULT_VISIBLE_PER_PROVIDER)) { + target.add(modelVisibilityKey(provider.slug, family.id)) + } +} + /** Resolve the canonical working set: the user's stored keys plus the curated * default expansion for any provider they haven't customized. Hide-all * sentinels are PRESERVED here — this is the set the toggle handler mutates and @@ -148,11 +155,7 @@ export function resolveVisibleKeys( continue } - const families = collapseModelFamilies(provider.models ?? []) - - for (const family of families.slice(0, DEFAULT_VISIBLE_PER_PROVIDER)) { - next.add(modelVisibilityKey(provider.slug, family.id)) - } + expandProviderDefaults(provider, next) } return next @@ -180,14 +183,15 @@ export function effectiveVisibleKeys( * Seeds from `resolveVisibleKeys` (NOT `effectiveVisibleKeys`) so other * providers' hide-all sentinels survive the persist. When the last visible * model of a provider is toggled off, a sentinel records the explicit - * hide-all; re-enabling any model clears that provider's sentinel. */ + * hide-all; re-enabling a model clears THAT provider's sentinel (only). */ export function toggleModelVisibility( stored: Set | null, providers: readonly ModelOptionProvider[], providerSlug: string, model: string ): Set { - const next = new Set(resolveVisibleKeys(stored, providers)) + // `resolveVisibleKeys` always returns a fresh Set, so we can mutate it directly. + const next = resolveVisibleKeys(stored, providers) const key = modelVisibilityKey(providerSlug, model) const sentinel = emptyProviderSentinelKey(providerSlug) @@ -203,6 +207,10 @@ export function toggleModelVisibility( next.add(sentinel) } } else { + // Re-enabling promotes a previously hidden-all provider to an explicit + // set of exactly the one re-enabled model — the curated defaults are NOT + // restored. Intentional: "you hid everything, you get back only what you + // re-enable." (Locked in by the sentinel-clear-on-re-enable test.) next.delete(sentinel) next.add(key) }