Skip to content

Commit 9f5c195

Browse files
NanmiCoderNanmiCoder
committed
fix(desktop): reuse model picker for agent settings
1 parent 49cfd17 commit 9f5c195

11 files changed

Lines changed: 265 additions & 142 deletions

File tree

‎desktop/src/components/controls/ModelSelector.test.tsx‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,38 @@ describe('ModelSelector', () => {
363363
expect(onChange).toHaveBeenCalledWith('beta')
364364
})
365365

366+
it('uses caller-supplied models in the reusable field appearance', async () => {
367+
const onChange = vi.fn()
368+
useSettingsStore.setState({
369+
locale: 'en',
370+
availableModels: MODELS,
371+
currentModel: MODELS[0],
372+
})
373+
const agentModels: ModelInfo[] = [
374+
{ id: 'inherit', name: 'Inherit from parent', description: 'Use the parent model', context: '' },
375+
{ id: 'provider-model', name: 'Provider Model', description: 'Configured model', context: '200k' },
376+
]
377+
378+
render(
379+
<ModelSelector
380+
value="inherit"
381+
onChange={onChange}
382+
models={agentModels}
383+
ariaLabel="Model"
384+
appearance="field"
385+
/>,
386+
)
387+
388+
const trigger = screen.getByRole('button', { name: 'Model' })
389+
expect(trigger.parentElement).toHaveClass('h-10', 'border-[var(--color-border)]')
390+
fireEvent.click(trigger)
391+
const dropdown = screen.getByTestId('model-selector-dropdown')
392+
expect(within(dropdown).queryByRole('button', { name: /Alpha/ })).not.toBeInTheDocument()
393+
fireEvent.click(within(dropdown).getByRole('button', { name: /Provider Model/ }))
394+
395+
expect(onChange).toHaveBeenCalledWith('provider-model')
396+
})
397+
366398
it('routes uncontrolled model changes through settings actions', async () => {
367399
const setModel = vi.fn(async () => {})
368400
useSettingsStore.setState({

‎desktop/src/components/controls/ModelSelector.tsx‎

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ type ProviderChoice = {
5050
type Props = {
5151
value?: string
5252
onChange?: (modelId: string) => void
53+
/**
54+
* Overrides the settings-store catalog for controlled pickers. This keeps
55+
* the shared search, portal, and viewport positioning while allowing a
56+
* feature to prepend semantic choices such as an inherited model.
57+
*/
58+
models?: ModelInfo[]
59+
ariaLabel?: string
60+
appearance?: 'toolbar' | 'field'
5361
runtimeSelection?: RuntimeSelection
5462
onRuntimeSelectionChange?: (selection: RuntimeSelection) => void
5563
runtimeKey?: string
@@ -223,6 +231,9 @@ function modelMatchesSearch(model: ModelInfo, query: string): boolean {
223231
export const ModelSelector = forwardRef<ModelSelectorHandle, Props>(function ModelSelector({
224232
value,
225233
onChange,
234+
models,
235+
ariaLabel,
236+
appearance = 'toolbar',
226237
runtimeSelection: controlledRuntimeSelection,
227238
onRuntimeSelectionChange,
228239
runtimeKey,
@@ -395,6 +406,7 @@ export const ModelSelector = forwardRef<ModelSelectorHandle, Props>(function Mod
395406
[activeId, availableModels, providers, roleLabels, t, claudeOAuthStatus, grokOAuthStatus, openAIOAuthStatus],
396407
)
397408
const normalizedSearchQuery = searchQuery.trim().toLocaleLowerCase()
409+
const selectableModels = isControlled && models ? models : availableModels
398410
const filteredProviderChoices = useMemo(() => {
399411
if (!normalizedSearchQuery) return providerChoices
400412

@@ -408,13 +420,13 @@ export const ModelSelector = forwardRef<ModelSelectorHandle, Props>(function Mod
408420
}, [normalizedSearchQuery, providerChoices])
409421
const filteredAvailableModels = useMemo(
410422
() => normalizedSearchQuery
411-
? availableModels.filter(model => modelMatchesSearch(model, normalizedSearchQuery))
412-
: availableModels,
413-
[availableModels, normalizedSearchQuery],
423+
? selectableModels.filter(model => modelMatchesSearch(model, normalizedSearchQuery))
424+
: selectableModels,
425+
[normalizedSearchQuery, selectableModels],
414426
)
415427

416428
const selectedModel = isControlled
417-
? availableModels.find((model) => model.id === value) || null
429+
? selectableModels.find((model) => model.id === value) || null
418430
: storeModel
419431

420432
const requestedRuntimeSelection = isRuntimeScoped
@@ -781,7 +793,7 @@ export const ModelSelector = forwardRef<ModelSelectorHandle, Props>(function Mod
781793
return (
782794
<div
783795
data-testid="model-selector-shell"
784-
className={`relative min-w-0 ${fluid ? 'flex-1' : 'shrink-0'}`}
796+
className={`relative min-w-0 ${fluid || appearance === 'field' ? 'flex-1' : 'shrink-0'}`}
785797
>
786798
{/* No fill at rest: on the composer row the model name is type, not a
787799
control chip — the handoff reserves filled pills for the permission
@@ -790,8 +802,16 @@ export const ModelSelector = forwardRef<ModelSelectorHandle, Props>(function Mod
790802
bottom sheet, so both halves stretch to the same 44px touch target
791803
`PermissionModeSelector` uses; `compact` alone would also shrink the
792804
desktop composer, which narrows for the right panel, not for touch. */}
793-
<div ref={ref} className={`flex min-w-0 items-stretch rounded-[var(--radius-md)] transition-colors hover:bg-[var(--color-surface-hover)] ${isMobileBrowser ? 'min-h-11' : ''} ${fluid ? 'w-full' : ''} ${disabled ? 'opacity-50' : ''}`}>
805+
<div
806+
ref={ref}
807+
className={`flex min-w-0 items-stretch rounded-[var(--radius-md)] transition-colors ${
808+
appearance === 'field'
809+
? 'h-10 w-full border border-[var(--color-border)] bg-[var(--color-surface)] hover:border-[var(--color-border-focus)] hover:bg-[var(--color-surface-container-low)]'
810+
: 'hover:bg-[var(--color-surface-hover)]'
811+
} ${isMobileBrowser ? 'min-h-11' : ''} ${fluid ? 'w-full' : ''} ${disabled ? 'opacity-50' : ''}`}
812+
>
794813
<button
814+
type="button"
795815
onClick={() => {
796816
if (disabled) return
797817
if (open) {
@@ -801,25 +821,27 @@ export const ModelSelector = forwardRef<ModelSelectorHandle, Props>(function Mod
801821
openSelector()
802822
}}
803823
disabled={disabled}
804-
aria-label={buttonProviderLabel ? `${buttonModelLabel}, ${buttonProviderLabel}` : buttonModelLabel}
824+
aria-label={ariaLabel ?? (buttonProviderLabel ? `${buttonModelLabel}, ${buttonProviderLabel}` : buttonModelLabel)}
805825
title={buttonProviderLabel ? `${buttonProviderLabel} · ${buttonModelLabel}` : buttonModelLabel}
806826
// `focus-visible:rounded-*` restores the other pair of corners while
807827
// focused. The ring traces `border-radius`, so on the half-rounded
808828
// halves of this segmented control it otherwise drew a box that was
809829
// rounded down one side and square down the other.
810-
className={`flex min-w-0 items-center gap-2 rounded-l-[var(--radius-md)] text-xs font-medium text-[var(--color-text-secondary)] outline-none transition-colors focus-visible:rounded-[var(--radius-md)] focus-visible:ring-2 focus-visible:ring-[var(--color-border-focus)] disabled:cursor-not-allowed ${
811-
compact ? `${fluid ? 'flex-1' : ''} max-w-[112px] py-1.5 pl-2.5 pr-1` : 'max-w-[220px] py-2 pl-2.5 pr-1'
830+
className={`flex min-w-0 items-center gap-2 text-xs font-medium text-[var(--color-text-secondary)] outline-none transition-colors focus-visible:ring-2 focus-visible:ring-[var(--color-border-focus)] disabled:cursor-not-allowed ${
831+
appearance === 'field'
832+
? 'h-full w-full rounded-[var(--radius-md)] px-3 text-left'
833+
: `rounded-l-[var(--radius-md)] focus-visible:rounded-[var(--radius-md)] ${compact ? `${fluid ? 'flex-1' : ''} max-w-[112px] py-1.5 pl-2.5 pr-1` : 'max-w-[220px] py-2 pl-2.5 pr-1'}`
812834
}`}
813835
>
814-
<span className={`${compact ? 'text-xs' : 'text-[15px]'} min-w-0 flex-1 truncate font-semibold text-[var(--color-text-primary)]`}>
836+
<span className={`${appearance === 'field' ? 'text-sm font-normal' : compact ? 'text-xs font-semibold' : 'text-[15px] font-semibold'} min-w-0 flex-1 truncate text-[var(--color-text-primary)]`}>
815837
{buttonModelLabel}
816838
</span>
817839
{!canEditRuntimeEffort && !compact && buttonProviderLabel && (
818840
<span className="max-w-[108px] flex-shrink-0 truncate text-[11px] text-[var(--color-text-tertiary)]">
819841
{buttonProviderLabel}
820842
</span>
821843
)}
822-
<span className="material-symbols-outlined flex-shrink-0 text-[12px] text-[var(--color-text-tertiary)]">
844+
<span className={`material-symbols-outlined flex-shrink-0 text-[var(--color-text-tertiary)] ${appearance === 'field' ? 'text-[16px]' : 'text-[12px]'}`}>
823845
{needsProviderConfiguration ? 'arrow_forward' : 'expand_more'}
824846
</span>
825847
</button>

‎desktop/src/components/settings/AgentManager.test.tsx‎

Lines changed: 67 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'
1+
import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react'
22
import '@testing-library/jest-dom'
33
import { beforeEach, describe, expect, it, vi } from 'vitest'
44

@@ -109,11 +109,15 @@ async function renderManager(response: AgentListResponse = EMPTY_RESPONSE) {
109109
}
110110

111111
function chooseAgentSelect(label: string, option: string) {
112-
// The trigger is a button; the entries inside the panel are listbox options.
113-
// They used to be buttons, which is invalid inside a `role="listbox"` and
114-
// left the dropdown without arrow-key navigation.
115-
fireEvent.click(screen.getByRole('button', { name: label }))
116-
fireEvent.click(screen.getByRole('option', { name: option }))
112+
const select = screen.getByRole('combobox', { name: label })
113+
const selectedOption = within(select).getByRole('option', { name: option }) as HTMLOptionElement
114+
fireEvent.change(select, { target: { value: selectedOption.value } })
115+
}
116+
117+
function chooseAgentModel(option: string | RegExp) {
118+
fireEvent.click(screen.getByRole('button', { name: 'Model' }))
119+
const picker = screen.getByTestId('model-selector-dropdown')
120+
fireEvent.click(within(picker).getByRole('button', { name: option }))
117121
}
118122

119123
describe('AgentManager', () => {
@@ -131,7 +135,23 @@ describe('AgentManager', () => {
131135
},
132136
})
133137
recentProjectsMock.mockResolvedValue({ projects: [] })
134-
useSettingsStore.setState({ locale: 'en' })
138+
useSettingsStore.setState({
139+
locale: 'en',
140+
availableModels: [
141+
{
142+
id: 'provider/custom-model',
143+
name: 'Provider Custom',
144+
description: 'Current provider model',
145+
context: '200k',
146+
},
147+
{
148+
id: 'deepseek-v4-pro',
149+
name: 'DeepSeek V4 Pro',
150+
description: 'Current provider model',
151+
context: '200k',
152+
},
153+
],
154+
})
135155
setProjectSession('/workspace/project')
136156
useAgentStore.setState({
137157
activeAgents: [],
@@ -165,17 +185,19 @@ describe('AgentManager', () => {
165185
fireEvent.click(screen.getByRole('button', { name: 'Create Agent' }))
166186
fireEvent.click(screen.getByRole('button', { name: 'Project' }))
167187
expect(screen.getByRole('button', { name: 'Select a project...' })).toBeInTheDocument()
168-
expect(document.querySelector('select')).not.toBeInTheDocument()
188+
expect(screen.getByRole('button', { name: 'Select a project...' }).tagName).toBe('BUTTON')
169189
fireEvent.click(screen.getByRole('button', { name: 'Select a project...' }))
170190
fireEvent.click(await screen.findByRole('button', { name: /Selected Project/ }))
171191
expect(screen.getByText('Target project: /workspace/selected')).toBeInTheDocument()
172192
fireEvent.click(screen.getByRole('button', { name: 'Model' }))
173-
const modelMenuOption = screen.getByRole('option', { name: 'fable' })
193+
const modelPicker = screen.getByTestId('model-selector-dropdown')
194+
const modelMenuOption = within(modelPicker).getByRole('button', { name: /fable/i })
174195
expect(modelMenuOption).toBeInTheDocument()
175-
expect(modelMenuOption.parentElement).toHaveClass('bottom-full')
196+
expect(screen.getByRole('dialog', { name: 'Create Agent' })).not.toContainElement(modelPicker)
197+
expect(modelPicker).toHaveClass('fixed', 'z-[var(--z-dropdown)]')
176198
fireEvent.keyDown(document, { key: 'Escape' })
177199
expect(screen.getByRole('heading', { name: 'Create Agent' })).toBeInTheDocument()
178-
expect(screen.queryByRole('button', { name: 'fable' })).not.toBeInTheDocument()
200+
expect(screen.queryByTestId('model-selector-dropdown')).not.toBeInTheDocument()
179201
expect(screen.getByLabelText('System prompt').parentElement).toHaveTextContent('System prompt*')
180202
})
181203

@@ -253,7 +275,7 @@ describe('AgentManager', () => {
253275
expect(apiListMock).toHaveBeenNthCalledWith(3, '/workspace/b')
254276
})
255277

256-
it('creates an underscore slug with custom model and effort, then selects the refreshed agent', async () => {
278+
it('creates an underscore slug with a configured provider model and effort, then selects the refreshed agent', async () => {
257279
const created = makeAgent({
258280
source: 'projectSettings',
259281
model: 'provider/custom-model',
@@ -278,8 +300,7 @@ describe('AgentManager', () => {
278300
fireEvent.change(screen.getByLabelText(/^Name/), { target: { value: 'code_reviewer' } })
279301
fireEvent.change(screen.getByLabelText(/^Description/), { target: { value: 'Review code' } })
280302
fireEvent.change(screen.getByLabelText('System prompt'), { target: { value: 'Review carefully.' } })
281-
chooseAgentSelect('Model', 'Custom model ID')
282-
fireEvent.change(screen.getByLabelText(/^Custom model ID/), { target: { value: 'provider/custom-model' } })
303+
chooseAgentModel(/Provider Custom/)
283304
chooseAgentSelect('Reasoning effort', 'xhigh')
284305
chooseAgentSelect('Tools', 'Custom list')
285306
fireEvent.click(screen.getByRole('checkbox', { name: /Read/ }))
@@ -445,7 +466,7 @@ describe('AgentManager', () => {
445466
render(<AgentManager />)
446467
await waitFor(() => expect(apiListMock).toHaveBeenCalledTimes(1))
447468
fireEvent.click(screen.getByRole('button', { name: 'Edit' }))
448-
chooseAgentSelect('Model', 'Inherit from parent')
469+
chooseAgentModel(/Inherit from parent/)
449470
chooseAgentSelect('Reasoning effort', 'Inherit from parent')
450471
chooseAgentSelect('Tools', 'All tools')
451472
chooseAgentSelect('Color', 'Default')
@@ -466,6 +487,27 @@ describe('AgentManager', () => {
466487
expect(screen.getAllByText('Inherit').length).toBeGreaterThanOrEqual(2)
467488
})
468489

490+
it('preserves a saved model ID that the current provider no longer lists', async () => {
491+
const agent = makeAgent({ model: 'legacy/provider-model' })
492+
apiListMock.mockResolvedValue({ activeAgents: [agent], allAgents: [agent] })
493+
apiUpdateMock.mockResolvedValue({ agent })
494+
useAgentStore.setState({ selectedAgent: agent, activeAgents: [agent], allAgents: [agent] })
495+
496+
render(<AgentManager />)
497+
await waitFor(() => expect(apiListMock).toHaveBeenCalledTimes(1))
498+
fireEvent.click(screen.getByRole('button', { name: 'Edit' }))
499+
fireEvent.click(screen.getByRole('button', { name: 'Model' }))
500+
const picker = screen.getByTestId('model-selector-dropdown')
501+
expect(within(picker).getByRole('button', { name: /legacy\/provider-model.*not listed by the current provider/i })).toBeInTheDocument()
502+
fireEvent.click(within(picker).getByRole('button', { name: /legacy\/provider-model/i }))
503+
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
504+
505+
await waitFor(() => expect(apiUpdateMock).toHaveBeenCalledWith(
506+
'code_reviewer',
507+
expect.objectContaining({ model: 'legacy/provider-model' }),
508+
))
509+
})
510+
469511
it('preserves an explicit empty tools list when editing only the description', async () => {
470512
const agent = makeAgent({ tools: [] })
471513
const updated = makeAgent({ tools: [], description: 'Updated review' })
@@ -480,7 +522,7 @@ describe('AgentManager', () => {
480522
render(<AgentManager />)
481523
await waitFor(() => expect(useAgentStore.getState().availableTools).toContain('Read'))
482524
fireEvent.click(screen.getByRole('button', { name: 'Edit' }))
483-
expect(screen.getByRole('button', { name: 'Tools' })).toHaveTextContent('No tools')
525+
expect(screen.getByRole('combobox', { name: 'Tools' })).toHaveValue('none')
484526
fireEvent.change(screen.getByLabelText(/^Description/), { target: { value: 'Updated review' } })
485527
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
486528

@@ -529,7 +571,7 @@ describe('AgentManager', () => {
529571
render(<AgentManager />)
530572
await waitFor(() => expect(apiListMock).toHaveBeenCalledTimes(1))
531573
fireEvent.click(screen.getByRole('button', { name: 'Edit' }))
532-
expect(screen.getByRole('button', { name: 'Reasoning effort' })).toHaveTextContent('7')
574+
expect(screen.getByRole('combobox', { name: 'Reasoning effort' })).toHaveValue('7')
533575
fireEvent.change(screen.getByLabelText(/^Description/), { target: { value: 'Updated review' } })
534576
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
535577

@@ -776,10 +818,7 @@ describe('AgentManager', () => {
776818
fireEvent.click(
777819
screen.getByRole('button', { name: 'Adjust the model and effort for Explore' }),
778820
)
779-
chooseAgentSelect('Model', 'Custom model ID')
780-
fireEvent.change(screen.getByLabelText(/^Custom model ID/), {
781-
target: { value: 'deepseek-v4-pro' },
782-
})
821+
chooseAgentModel(/DeepSeek V4 Pro/)
783822
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
784823

785824
await waitFor(() => expect(apiReloadMock).toHaveBeenCalledWith('running-session'))
@@ -819,9 +858,10 @@ describe('AgentManager', () => {
819858
// inherit means "follow the main session" — collapsing them would make
820859
// inherit unreachable, and the default label is read from the server.
821860
fireEvent.click(screen.getByRole('button', { name: 'Model' }))
822-
expect(screen.getByRole('option', { name: 'Built-in default (haiku)' })).toBeInTheDocument()
823-
expect(screen.getByRole('option', { name: 'Inherit from parent' })).toBeInTheDocument()
824-
fireEvent.click(screen.getByRole('option', { name: 'Built-in default (haiku)' }))
861+
const modelPicker = screen.getByTestId('model-selector-dropdown')
862+
expect(within(modelPicker).getByRole('button', { name: /Built-in default \(haiku\)/ })).toBeInTheDocument()
863+
expect(within(modelPicker).getByRole('button', { name: /Inherit from parent/ })).toBeInTheDocument()
864+
fireEvent.click(within(modelPicker).getByRole('button', { name: /Built-in default \(haiku\)/ }))
825865

826866
chooseAgentSelect('Reasoning effort', 'high')
827867
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
@@ -844,7 +884,7 @@ describe('AgentManager', () => {
844884
fireEvent.click(
845885
screen.getByRole('button', { name: 'Adjust the model and effort for Explore' }),
846886
)
847-
chooseAgentSelect('Model', 'Inherit from parent')
887+
chooseAgentModel(/Inherit from parent/)
848888
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
849889

850890
await waitFor(() => expect(apiSetOverrideMock).toHaveBeenCalledWith('Explore', {
@@ -893,7 +933,7 @@ describe('AgentManager', () => {
893933
)
894934

895935
expect(screen.getByRole('button', { name: 'Model' })).toBeDisabled()
896-
expect(screen.getByRole('button', { name: 'Reasoning effort' })).toBeDisabled()
936+
expect(screen.getByRole('combobox', { name: 'Reasoning effort' })).toBeDisabled()
897937
expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled()
898938
// Resetting would write to the user file, which cannot win over a policy.
899939
expect(screen.queryByRole('button', { name: 'Reset to built-in default' })).toBeNull()
@@ -925,7 +965,7 @@ describe('AgentManager', () => {
925965
fireEvent.click(
926966
screen.getByRole('button', { name: 'Adjust the model and effort for Explore' }),
927967
)
928-
chooseAgentSelect('Model', 'sonnet')
968+
chooseAgentModel(/^sonnet/i)
929969
fireEvent.click(screen.getByRole('button', { name: 'Save' }))
930970

931971
expect(await screen.findByRole('alert')).toHaveTextContent('Failed to save the override')

0 commit comments

Comments
 (0)