fix(desktop): address Copilot review on model picker

- selectModel reports success; edits bail (and roll back) instead of landing
  on the previously active model when a switch fails
- Fast toggle stays available to turn off a carried-over speed param even when
  the new model has no native fast mechanism
- active row's "Fast" label derives from the same fastControl as the submenu
  toggle, so it's consistent and handles standalone `-fast` model ids
This commit is contained in:
Brooklyn Nicholson
2026-06-02 19:28:11 -05:00
parent ea4fe15631
commit 115671ae6b
3 changed files with 52 additions and 24 deletions

View File

@ -48,11 +48,11 @@ export function useModelControls({ activeSessionId, queryClient, requestGateway
}
}, [])
// Returns a promise so callers can await the switch before applying
// follow-up changes (e.g. editing a model's reasoning/fast must land on the
// right active model). Resolves even on failure (error is surfaced inline).
// Returns whether the switch succeeded so callers can await it before
// applying follow-up changes (e.g. editing a model's reasoning/fast must land
// on the right active model — bail rather than write to the previous one).
const selectModel = useCallback(
async (selection: ModelSelection): Promise<void> => {
async (selection: ModelSelection): Promise<boolean> => {
setCurrentModel(selection.model)
setCurrentProvider(selection.provider)
updateModelOptionsCache(selection.provider, selection.model, selection.persistGlobal || !activeSessionId)
@ -72,14 +72,18 @@ export function useModelControls({ activeSessionId, queryClient, requestGateway
queryKey: selection.persistGlobal ? ['model-options'] : ['model-options', activeSessionId]
})
return
return true
}
await setGlobalModel(selection.provider, selection.model)
void refreshCurrentModel()
void queryClient.invalidateQueries({ queryKey: ['model-options'] })
return true
} catch (err) {
notifyError(err, 'Model switch failed')
return false
}
},
[activeSessionId, queryClient, refreshCurrentModel, requestGateway, updateModelOptionsCache]

View File

@ -67,6 +67,13 @@ export function resolveFastControl(
return { kind: 'variant', baseId: model, fastId, on: false }
}
// Fast isn't natively offered here, but if the session still has the speed
// param on (carried over from a previous model), expose the toggle so it can
// be turned off rather than stranded.
if (currentFastMode) {
return { kind: 'param', on: true }
}
return { kind: 'none' }
}
@ -75,10 +82,11 @@ interface ModelEditSubmenuProps {
fastControl: FastControl
/** Whether this row's model is the active one. */
isActive: boolean
/** Switch to this model. Awaited before applying edits when not active. */
onActivate: () => Promise<void> | void
/** Switch to this model (resolves false on failure). Awaited before applying
* edits when not active so a failed switch doesn't write to the old model. */
onActivate: () => Promise<boolean> | void
/** Switch to a specific model id (used to swap base ⇄ -fast variant). */
onSelectModel: (model: string) => Promise<void> | void
onSelectModel: (model: string) => Promise<boolean> | void
/** Whether this model supports reasoning effort. */
reasoning: boolean
requestGateway: <T>(method: string, params?: Record<string, unknown>) => Promise<T>
@ -101,19 +109,26 @@ export function ModelEditSubmenu({
const thinkingOn = isThinkingEnabled(currentReasoningEffort)
// Reasoning/fast are session-scoped (they apply to the active model), so
// editing a non-active model first switches to it — otherwise the change
// would land on (and be capability-checked against) the wrong model.
const ensureActive = async () => {
if (!isActive) {
await onActivate()
// editing a non-active model first switches to it. Returns false if the
// switch failed, so callers skip applying to the wrong (previous) model.
const ensureActive = async (): Promise<boolean> => {
if (isActive) {
return true
}
return (await onActivate()) !== false
}
const patchReasoning = async (next: string, rollback: string) => {
setCurrentReasoningEffort(next)
try {
await ensureActive()
if (!(await ensureActive())) {
setCurrentReasoningEffort(rollback)
return
}
await requestGateway('config.set', {
key: 'reasoning',
session_id: activeSessionId ?? '',
@ -138,7 +153,12 @@ export function ModelEditSubmenu({
void (async () => {
try {
await ensureActive()
if (!(await ensureActive())) {
setCurrentFastMode(!enabled)
return
}
await requestGateway('config.set', {
key: 'fast',
session_id: activeSessionId ?? '',

View File

@ -40,7 +40,7 @@ import { ModelEditSubmenu, resolveFastControl } from './model-edit-submenu'
interface ModelMenuPanelProps {
gateway?: HermesGateway
onSelectModel: (selection: { model: string; persistGlobal: boolean; provider: string }) => Promise<void> | void
onSelectModel: (selection: { model: string; persistGlobal: boolean; provider: string }) => Promise<boolean> | void
requestGateway: <T>(method: string, params?: Record<string, unknown>) => Promise<T>
}
@ -143,12 +143,21 @@ export function ModelMenuPanel({ gateway, onSelectModel, requestGateway }: Model
// Capabilities are looked up against the active/base id; the
// -fast variant carries the same param support as its base.
const caps = group.provider.capabilities?.[family.id]
const fastActive = optionsModel === family.fastId || (isCurrent && currentFastMode)
// Single source of truth for the active row's fast state — keeps
// the row label in lock-step with the submenu's Fast toggle and
// handles the standalone `-fast` id case.
const fastControl = resolveFastControl(
activeId ?? family.id,
group.provider.models ?? [],
caps?.fast ?? false,
currentFastMode
)
// Grayed text: active row shows live state (Fast + effort);
// others show a fast-capability hint.
const meta = isCurrent
? [fastActive ? 'Fast' : null, reasoningEffortLabel(currentReasoningEffort) || 'Med']
? [fastControl.kind !== 'none' && fastControl.on ? 'Fast' : null, reasoningEffortLabel(currentReasoningEffort) || 'Med']
.filter(Boolean)
.join(' ')
: caps?.fast || family.fastId
@ -176,12 +185,7 @@ export function ModelMenuPanel({ gateway, onSelectModel, requestGateway }: Model
{isCurrent ? <Codicon className="ml-auto text-foreground" name="check" size="0.75rem" /> : null}
</DropdownMenuSubTrigger>
<ModelEditSubmenu
fastControl={resolveFastControl(
activeId ?? family.id,
group.provider.models ?? [],
caps?.fast ?? false,
currentFastMode
)}
fastControl={fastControl}
isActive={isCurrent}
onActivate={() => switchTo(family.id, group.provider.slug)}
onSelectModel={nextModel => switchTo(nextModel, group.provider.slug)}