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] 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) }