fix(desktop): preserve collapsed-provider set across profile switches

The collapsed-providers atom (`hermes.desktop.collapsed-providers`) is a
global presentation-layer preference, but the catalog the picker renders is
profile-scoped (`getGlobalModelOptions` routes through `profileScoped()`,
model-menu-panel.tsx:87-93). The previous code called `pruneStaleCollapsed`
on every render against `pickerProviders`, which silently deleted any
slug not present in the active catalog.

Bug class (review from @teknium1):
- Profile switch to a catalog that lacks a previously-collapsed provider
  → that collapse preference is permanently lost.
- Refresh Models that drops a provider (revoked key, plugin disabled,
  backend policy change) → same loss.
- Any transient empty catalog (loading → []) → guarded by `length > 0`,
  but still loses state once the new (smaller) catalog resolves.

Fix:
- Remove the prune `useEffect` from model-menu-panel.tsx.
- Delete `pruneStaleCollapsed` (no other caller; the AGENTS.md "no
  speculative infrastructure" rule applies — keeping it exported with a
  plausible-sounding docstring is a foot-gun for future contributors who
  would re-call it against a single active catalog and reintroduce the bug).
- Document on the atom why we deliberately don't prune: provider slugs
  come from a bounded configured set (not user input); the render loop
  only visits providers in the active `groups`; dead slugs have no
  observable effect (`collapsedProviders.includes(slug)` against an
  absent slug is a no-op).

Tests:
- `preserves the collapsed set across a profile switch whose catalog
  lacks the slug` — regression pin for the profile-switch case.
- `preserves the collapsed set when Refresh Models drops a provider` —
  regression pin for the refresh-models case.

Verified: `tsc -p . --noEmit` is clean on the changed files. CI is the
source of truth for the vitest run (the local React-detection env
failure pre-dates this PR; CI passes).

Closes the review thread on #64690.
This commit is contained in:
David Metcalfe 2026-07-17 22:50:46 -07:00 committed by Teknium
parent f52b6530ff
commit 0831e5e326
3 changed files with 69 additions and 24 deletions

View file

@ -4,7 +4,7 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vite
import { DropdownMenu, DropdownMenuContent } from '@/components/ui/dropdown-menu'
import { $activeSessionId, $currentModel, $currentProvider } from '@/store/session'
import { $collapsedProviders } from '@/store/provider-collapse'
import { $collapsedProviders, toggleCollapsedProvider } from '@/store/provider-collapse'
import { ModelMenuPanel } from './model-menu-panel'
@ -197,4 +197,55 @@ describe('ModelMenuPanel provider collapse', () => {
expect(content.queryByText('Deepseek V4 Pro')).toBeNull()
})
// The collapsed-providers set is a global presentation preference
// (`hermes.desktop.collapsed-providers`), but the catalog the picker renders
// is profile-scoped (`getGlobalModelOptions` routes through
// `profileScoped()`). Pruning the global set against only the active catalog
// would silently delete a user's collapse preference on every profile switch
// whose configured providers don't include the slug — the bug the maintainer
// flagged. The set must survive catalog changes; if the same provider shows
// up again later, the previous collapse is preserved.
it('preserves the collapsed set across a profile switch whose catalog lacks the slug', async () => {
toggleCollapsedProvider('deepseek')
toggleCollapsedProvider('google')
expect($collapsedProviders.get()).toEqual(['deepseek', 'google'])
// Profile A: both providers present, render + unmount.
getGlobalModelOptions.mockResolvedValueOnce({ providers: MOCK_PROVIDERS })
const a = renderPanel()
await a.content.findByText('DeepSeek')
a.content.unmount()
// Profile B: google is not in the catalog (simulates a profile whose
// configured providers differ). The previously-collapsed 'google' slug
// must survive — pruning it would lose state across a profile switch.
getGlobalModelOptions.mockResolvedValueOnce({ providers: [DEEPSEEK_PROVIDER, MOA_PROVIDER] })
const b = renderPanel()
await b.content.findByText('DeepSeek')
expect($collapsedProviders.get()).toEqual(['deepseek', 'google'])
})
it('preserves the collapsed set when Refresh Models drops a provider', async () => {
toggleCollapsedProvider('deepseek')
toggleCollapsedProvider('google')
// First load: both providers present.
getGlobalModelOptions.mockResolvedValueOnce({ providers: MOCK_PROVIDERS })
const a = renderPanel()
await a.content.findByText('DeepSeek')
a.content.unmount()
// Refresh Models returns a catalog that drops google (revoked key,
// plugin disabled, backend policy change). 'google' must survive — the
// user explicitly collapsed it, and the global set is not tied to any
// single refresh.
getGlobalModelOptions.mockResolvedValueOnce({ providers: [DEEPSEEK_PROVIDER, MOA_PROVIDER] })
const b = renderPanel()
await b.content.findByText('DeepSeek')
expect($collapsedProviders.get()).toContain('google')
expect($collapsedProviders.get()).toContain('deepseek')
})
})

View file

@ -1,6 +1,6 @@
import { useStore } from '@nanostores/react'
import { useQuery, useQueryClient } from '@tanstack/react-query'
import { createContext, useContext, useEffect, useMemo, useState } from 'react'
import { createContext, useContext, useMemo, useState } from 'react'
import { useSessionView } from '@/app/chat/session-view'
import { Codicon } from '@/components/ui/codicon'
@ -38,7 +38,7 @@ import {
modelVisibilityKey,
setModelVisibilityOpen
} from '@/store/model-visibility'
import { $collapsedProviders, pruneStaleCollapsed, toggleCollapsedProvider } from '@/store/provider-collapse'
import { $collapsedProviders, toggleCollapsedProvider } from '@/store/provider-collapse'
import type { ModelOptionProvider, ModelOptionsResponse } from '@/types/hermes'
import { ModelEditSubmenu, resolveFastControl } from './model-edit-submenu'
@ -123,14 +123,6 @@ export function ModelMenuPanel({ gateway, onSelectModel, requestGateway }: Model
[providers]
)
// Drop stale collapsed provider slugs when the provider list changes
// (e.g. after Refresh Models, plugin install/uninstall).
useEffect(() => {
if (pickerProviders.length > 0) {
pruneStaleCollapsed(pickerProviders.map(p => p.slug))
}
}, [pickerProviders])
const effectiveVisibleModels = useMemo(
() => effectiveVisibleKeys(visibleModels, pickerProviders),
[visibleModels, pickerProviders]

View file

@ -4,7 +4,21 @@ const STORAGE_KEY = 'hermes.desktop.collapsed-providers'
/** Set of provider slugs whose model groups are currently collapsed in the
* model picker dropdown. Persisted to localStorage globally this is a
* presentation-layer preference, not a per-profile setting. */
* presentation-layer preference, not a per-profile setting.
*
* We deliberately do NOT prune this set when the active provider catalog
* changes (e.g. profile switch, Refresh Models, API key revoked). The catalog
* the picker renders is profile-scoped (`getGlobalModelOptions` routes through
* `profileScoped()`), so pruning against only the active catalog would delete
* a user's collapse preference every time they switch to a profile whose
* configured providers don't include it silently losing state across what
* is otherwise a pure presentational toggle.
*
* Provider slugs come from a small bounded configured set (not user input),
* so dead entries in the array cost a few bytes and have no observable effect:
* the render loop only visits providers present in the active `groups`, and
* `collapsedProviders.includes(slug)` against an absent slug is a no-op.
*/
export const $collapsedProviders = persistentAtom<string[]>(
STORAGE_KEY,
[],
@ -20,15 +34,3 @@ export function toggleCollapsedProvider(slug: string): void {
: [...current, slug]
)
}
/** Remove slugs from the collapsed set that no longer match any current
* provider. Call this after provider list changes (e.g. Refresh Models,
* plugin install/uninstall) to prevent unbounded growth. */
export function pruneStaleCollapsed(slugs: string[]): void {
const valid = new Set(slugs)
const next = $collapsedProviders.get().filter(s => valid.has(s))
if (next.length !== $collapsedProviders.get().length) {
$collapsedProviders.set(next)
}
}