From 115671ae6b2c838cde7dce73a945a0af05a95a7e Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 2 Jun 2026 19:28:11 -0500 Subject: [PATCH] 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 --- .../app/session/hooks/use-model-controls.ts | 14 ++++--- .../src/app/shell/model-edit-submenu.tsx | 40 ++++++++++++++----- .../src/app/shell/model-menu-panel.tsx | 22 +++++----- 3 files changed, 52 insertions(+), 24 deletions(-) diff --git a/apps/desktop/src/app/session/hooks/use-model-controls.ts b/apps/desktop/src/app/session/hooks/use-model-controls.ts index 6d30297db..6cefb7059 100644 --- a/apps/desktop/src/app/session/hooks/use-model-controls.ts +++ b/apps/desktop/src/app/session/hooks/use-model-controls.ts @@ -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 => { + async (selection: ModelSelection): Promise => { 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] diff --git a/apps/desktop/src/app/shell/model-edit-submenu.tsx b/apps/desktop/src/app/shell/model-edit-submenu.tsx index df3d3ffa3..8328f7ddf 100644 --- a/apps/desktop/src/app/shell/model-edit-submenu.tsx +++ b/apps/desktop/src/app/shell/model-edit-submenu.tsx @@ -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 + /** 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 | void /** Switch to a specific model id (used to swap base ⇄ -fast variant). */ - onSelectModel: (model: string) => Promise | void + onSelectModel: (model: string) => Promise | void /** Whether this model supports reasoning effort. */ reasoning: boolean requestGateway: (method: string, params?: Record) => Promise @@ -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 => { + 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 ?? '', diff --git a/apps/desktop/src/app/shell/model-menu-panel.tsx b/apps/desktop/src/app/shell/model-menu-panel.tsx index 7ee757ba7..8a8374674 100644 --- a/apps/desktop/src/app/shell/model-menu-panel.tsx +++ b/apps/desktop/src/app/shell/model-menu-panel.tsx @@ -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 + onSelectModel: (selection: { model: string; persistGlobal: boolean; provider: string }) => Promise | void requestGateway: (method: string, params?: Record) => Promise } @@ -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 ? : null} switchTo(family.id, group.provider.slug)} onSelectModel={nextModel => switchTo(nextModel, group.provider.slug)}