diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index 2bc6218c..f90c6220 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -10,7 +10,12 @@ import AcpConfigPickerSection from './AcpConfigPickerSection.svelte'; import AcpConfigPickerShell from './AcpConfigPickerShell.svelte'; import { agentState, REMOTE_AGENTS } from './agent.svelte'; - import { setAiAgent, getPreferredAgent } from '../settings/preferences.svelte'; + import { + setAiAgent, + getPreferredAgent, + setAcpConfigPref, + getAcpConfigPref, + } from '../settings/preferences.svelte'; import { discoverAcpConfig, type AcpConfigDiscovery, @@ -18,7 +23,13 @@ } from '../../api/commands'; import Spinner from '../../shared/Spinner.svelte'; import * as DropdownMenu from '$lib/components/ui/dropdown-menu'; - import { buildAcpConfigSelection, type AcpConfigPickerSelection } from './acpConfigSelection'; + import { + buildAcpConfigSelection, + defaultSelectorValue, + reconcileSelectorValue, + selectorHasValue, + type AcpConfigPickerSelection, + } from './acpConfigSelection'; interface AgentOption { id: string; @@ -141,10 +152,12 @@ if (cancelled || run !== discoveryRun) return; config = data; configLoading = false; + restorePersistedModel(data); revalidating ?.then((fresh) => { if (!cancelled && run === discoveryRun) { config = fresh; + restorePersistedModel(fresh); } }) .catch((error) => { @@ -177,11 +190,23 @@ } }); + // Mirror of the model effect above, with two twists: while a model-specific + // fetch is in flight the selector is null, and the current selection is + // retained as the desired value for the incoming options; and when there is + // no valid explicit selection, the persisted preference is tried before the + // selector default. $effect(() => { const nextKey = selectorKey(effortSelector); if (nextKey !== effortSelectorKey) { - selectedEffortValue = defaultSelectorValue(effortSelector); - effortSelectionExplicit = false; + if ( + effortSelector && + !(effortSelectionExplicit && selectorHasValue(effortSelector, selectedEffortValue)) + ) { + const persisted = selectedProviderId ? getAcpConfigPref(selectedProviderId) : null; + const reconciled = reconcileSelectorValue(effortSelector, persisted?.effort ?? null); + selectedEffortValue = reconciled.valueId; + effortSelectionExplicit = reconciled.explicit; + } effortSelectorKey = nextKey; } }); @@ -207,19 +232,63 @@ function handleModelChange(value: string) { selectedModelValue = value; modelSelectionExplicit = true; - selectedEffortValue = null; - effortSelectionExplicit = false; - config = config ? { ...config, effort: null } : config; + if (!remote && selectedProviderId) { + setAcpConfigPref(selectedProviderId, { model: value }); + } + // The current effort selection is kept as the desired value; the effort + // effect reconciles it once the model-specific options arrive. + discoverEffortOptionsForModel(value); + } + function handleEffortChange(value: string) { + selectedEffortValue = value; + effortSelectionExplicit = true; + if (!remote && selectedProviderId) { + setAcpConfigPref(selectedProviderId, { effort: value }); + } + } + + /** + * Re-apply the last explicitly chosen model for this provider once + * discovery lands. Restored values are marked explicit so they are sent at + * launch — non-explicit selections are display-only (see + * buildAcpConfigSelection) — which is safe because the driver skips config + * values the agent no longer offers. Persisted values missing from the + * options fall back silently; options can vary by working directory and + * agent version, so the pref is not deleted. + */ + function restorePersistedModel(discovered: AcpConfigDiscovery | null) { + if (remote || modelSelectionExplicit || !selectedProviderId) return; + const persistedModel = getAcpConfigPref(selectedProviderId)?.model ?? null; + const selector = discovered?.model ?? null; + if (!persistedModel || !selectorHasValue(selector, persistedModel)) return; + + selectedModelValue = persistedModel; + modelSelectionExplicit = true; + // The effort options in the initial discovery belong to the provider's + // default model; fetch the ones for the restored model instead. + if (persistedModel !== defaultSelectorValue(selector)) { + discoverEffortOptionsForModel(persistedModel); + } + } + + /** + * Fetch the effort options specific to a model. The effort selector is + * cleared so the column shows its loading state; the current effort + * selection is retained and reconciled when the new options arrive. + */ + function discoverEffortOptionsForModel(modelValue: string) { const providerId = selectedProviderId; const discoveryWorkingDir = workingDir ?? null; if (remote || !providerId) return; + config = config ? { ...config, effort: null } : config; + const run = ++discoveryRun; configLoading = true; configError = null; - discoverAcpConfig(providerId, discoveryWorkingDir, { selectedModelValue: value }) + discoverAcpConfig(providerId, discoveryWorkingDir, { selectedModelValue: modelValue }) .then(({ data, revalidating }) => { if (run !== discoveryRun) return; config = data; @@ -243,38 +312,21 @@ configError = error instanceof Error ? error.message : String(error); // The effort column was cleared for the incoming model-specific // options; restore the previous model's selector so the column stays - // usable after a transient failure. If a choice from it turns out to - // be stale for the new model, the driver skips it and the launch - // falls back to provider-default effort. + // usable — with its selection intact — after a transient failure. If + // the choice turns out to be stale for the new model, the driver + // skips it and the launch falls back to provider-default effort. if (config && !config.effort && retainedEffortSelector) { config = { ...config, effort: retainedEffortSelector }; } }); } - function handleEffortChange(value: string) { - selectedEffortValue = value; - effortSelectionExplicit = true; - } - function selectorKey(selector: AcpConfigSelector | null): string | null { if (!selector) return null; const optionIds = selector.options.map((option) => option.valueId).join(','); return `${selector.configId}:${selector.currentValueId}:${optionIds}`; } - function defaultSelectorValue(selector: AcpConfigSelector | null): string | null { - if (!selector || selector.options.length === 0) return null; - if (selector.options.some((option) => option.valueId === selector.currentValueId)) { - return selector.currentValueId; - } - return selector.options[0]?.valueId ?? null; - } - - function selectorHasValue(selector: AcpConfigSelector | null, valueId: string | null): boolean { - return !!selector && !!valueId && selector.options.some((option) => option.valueId === valueId); - } - function selectorValueLabel( selector: AcpConfigSelector | null, valueId: string | null diff --git a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts index c8dacb0f..df54ce03 100644 --- a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts +++ b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; import type { AcpConfigSelector } from '../../api/commands'; -import { buildAcpConfigSelection } from './acpConfigSelection'; +import { buildAcpConfigSelection, reconcileSelectorValue } from './acpConfigSelection'; function selector(overrides: Partial = {}): AcpConfigSelector { return { @@ -97,3 +97,41 @@ describe('buildAcpConfigSelection', () => { ).toBeNull(); }); }); + +describe('reconcileSelectorValue', () => { + it('keeps a desired value the selector still offers, marked explicit', () => { + expect(reconcileSelectorValue(selector(), 'opus')).toEqual({ + valueId: 'opus', + explicit: true, + }); + }); + + it('falls back to the selector default when the desired value is gone', () => { + expect(reconcileSelectorValue(selector(), 'removed-model')).toEqual({ + valueId: 'sonnet', + explicit: false, + }); + }); + + it('falls back to the selector default when there is no desired value', () => { + expect(reconcileSelectorValue(selector(), null)).toEqual({ + valueId: 'sonnet', + explicit: false, + }); + }); + + it('falls back to the first option when the selector default is not listed', () => { + expect(reconcileSelectorValue(selector({ currentValueId: 'missing' }), null)).toEqual({ + valueId: 'sonnet', + explicit: false, + }); + }); + + it('returns no value for a missing or empty selector', () => { + expect(reconcileSelectorValue(null, 'opus')).toEqual({ valueId: null, explicit: false }); + expect(reconcileSelectorValue(selector({ options: [] }), 'opus')).toEqual({ + valueId: null, + explicit: false, + }); + }); +}); diff --git a/apps/staged/src/lib/features/agents/acpConfigSelection.ts b/apps/staged/src/lib/features/agents/acpConfigSelection.ts index cafbc155..4891ddf2 100644 --- a/apps/staged/src/lib/features/agents/acpConfigSelection.ts +++ b/apps/staged/src/lib/features/agents/acpConfigSelection.ts @@ -17,6 +17,46 @@ interface AcpSelectorSelections { effort?: SelectorSelection; } +export interface ReconciledSelectorValue { + valueId: string | null; + explicit: boolean; +} + +/** + * The value a selector shows when nothing has been chosen: its reported + * current value when listed, otherwise the first option. + */ +export function defaultSelectorValue(selector: AcpConfigSelector | null): string | null { + if (!selector || selector.options.length === 0) return null; + if (selector.options.some((option) => option.valueId === selector.currentValueId)) { + return selector.currentValueId; + } + return selector.options[0]?.valueId ?? null; +} + +export function selectorHasValue( + selector: AcpConfigSelector | null, + valueId: string | null +): boolean { + return !!selector && !!valueId && selector.options.some((option) => option.valueId === valueId); +} + +/** + * Reconcile a desired value (a prior explicit choice or persisted preference) + * against a selector's live options. Keeps the desired value — marked + * explicit so it is sent at launch — when the selector still offers it; + * otherwise falls back to the selector default as a display-only value. + */ +export function reconcileSelectorValue( + selector: AcpConfigSelector | null, + desiredValueId: string | null +): ReconciledSelectorValue { + if (selectorHasValue(selector, desiredValueId)) { + return { valueId: desiredValueId, explicit: true }; + } + return { valueId: defaultSelectorValue(selector), explicit: false }; +} + function selectedValueId(selector: AcpConfigSelector, valueId: string | null): string | null { if (valueId && selector.options.some((option) => option.valueId === valueId)) { return valueId; diff --git a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte index 94f27cb8..8765d4f4 100644 --- a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte +++ b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte @@ -80,6 +80,7 @@ import AcpFixedConfigPicker from '../agents/AcpFixedConfigPicker.svelte'; import { agentState } from '../agents/agent.svelte'; import { buildAcpConfigSelection } from '../agents/acpConfigSelection'; + import { setAcpConfigPref } from '../settings/preferences.svelte'; import ChatComposer, { composerControlClass } from './ChatComposer.svelte'; import { buildBranchHashtagItems, @@ -1312,6 +1313,10 @@ } }); + // Mirror of the model effect above: a touched selection survives selector + // changes while it stays valid. While model-specific options load the + // selector is null; the selection is retained as the desired value and + // reconciled against the options once they arrive. $effect(() => { const key = selectorStateKey( session?.id, @@ -1319,11 +1324,19 @@ session?.acpConfigSelection?.effort ?? null ); if (key !== followupEffortStateKey) { - selectedFollowupEffortValue = initialSelectorValue( - followupEffortSelector, - session?.acpConfigSelection?.effort ?? null - ); - followupEffortTouched = false; + if ( + followupEffortSelector && + !( + followupEffortTouched && + selectorHasValue(followupEffortSelector, selectedFollowupEffortValue) + ) + ) { + selectedFollowupEffortValue = initialSelectorValue( + followupEffortSelector, + session?.acpConfigSelection?.effort ?? null + ); + followupEffortTouched = false; + } followupEffortStateKey = key; } }); @@ -1392,13 +1405,16 @@ function handleFollowupModelChange(value: string) { selectedFollowupModelValue = value; followupModelTouched = true; - selectedFollowupEffortValue = null; - followupEffortTouched = false; + // The effort selection is kept as the desired value; the effort effect + // reconciles it once the model-specific options arrive. modelSpecificFollowupConfig = null; modelSpecificFollowupModelValue = value; modelSpecificFollowupSessionId = session?.id ?? null; const providerId = session?.provider ?? null; + if (providerId) { + setAcpConfigPref(providerId, { model: value }); + } const workingDir = session?.workingDir ?? null; if (!active || !providerId) return; @@ -1439,6 +1455,9 @@ function handleFollowupEffortChange(value: string) { selectedFollowupEffortValue = value; followupEffortTouched = true; + if (session?.provider) { + setAcpConfigPref(session.provider, { effort: value }); + } } let grouped = $derived.by(() => diff --git a/apps/staged/src/lib/features/settings/acpConfigPrefs.test.ts b/apps/staged/src/lib/features/settings/acpConfigPrefs.test.ts new file mode 100644 index 00000000..d38d8180 --- /dev/null +++ b/apps/staged/src/lib/features/settings/acpConfigPrefs.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest'; +import { mergeAcpConfigPref } from './acpConfigPrefs'; + +describe('mergeAcpConfigPref', () => { + it('creates a pref from scratch', () => { + expect(mergeAcpConfigPref(undefined, { model: 'opus' })).toEqual({ model: 'opus' }); + }); + + it('updates one field without touching the other', () => { + expect(mergeAcpConfigPref({ model: 'opus', effort: 'high' }, { effort: 'medium' })).toEqual({ + model: 'opus', + effort: 'medium', + }); + }); + + it('leaves absent fields untouched', () => { + expect(mergeAcpConfigPref({ effort: 'high' }, { model: 'opus' })).toEqual({ + model: 'opus', + effort: 'high', + }); + }); + + it('clears a field with null', () => { + expect(mergeAcpConfigPref({ model: 'opus', effort: 'high' }, { effort: null })).toEqual({ + model: 'opus', + }); + }); + + it('does not mutate the current pref', () => { + const current = { model: 'opus' }; + mergeAcpConfigPref(current, { model: 'sonnet' }); + expect(current).toEqual({ model: 'opus' }); + }); +}); diff --git a/apps/staged/src/lib/features/settings/acpConfigPrefs.ts b/apps/staged/src/lib/features/settings/acpConfigPrefs.ts new file mode 100644 index 00000000..51eec1eb --- /dev/null +++ b/apps/staged/src/lib/features/settings/acpConfigPrefs.ts @@ -0,0 +1,37 @@ +/** + * Merge logic for the per-provider ACP config picker preferences persisted + * by the preferences store. Kept separate from preferences.svelte.ts so it + * can be unit-tested without the Svelte runtime. + */ + +/** + * The last explicitly chosen picker values for one provider, stored as + * selector valueIds. Labels come from live discovery options, so persisting + * ids alone avoids staleness. + */ +export interface AcpConfigPref { + model?: string; + effort?: string; +} + +/** Patch for one provider's pref: absent fields are untouched, `null` clears. */ +export interface AcpConfigPrefPatch { + model?: string | null; + effort?: string | null; +} + +export function mergeAcpConfigPref( + current: AcpConfigPref | undefined, + patch: AcpConfigPrefPatch +): AcpConfigPref { + const next: AcpConfigPref = { ...current }; + if (patch.model !== undefined) { + if (patch.model === null) delete next.model; + else next.model = patch.model; + } + if (patch.effort !== undefined) { + if (patch.effort === null) delete next.effort; + else next.effort = patch.effort; + } + return next; +} diff --git a/apps/staged/src/lib/features/settings/preferences.svelte.ts b/apps/staged/src/lib/features/settings/preferences.svelte.ts index 853a3c03..6ab60b13 100644 --- a/apps/staged/src/lib/features/settings/preferences.svelte.ts +++ b/apps/staged/src/lib/features/settings/preferences.svelte.ts @@ -20,6 +20,10 @@ import { } from '../diff/highlighter'; import { initPersistentStore, getStoreValue, setStoreValue } from '../../shared/persistentStore'; import { createAdaptiveTheme, themeToVarMap, type ThemeGitColors } from '../../theme'; +import { mergeAcpConfigPref, type AcpConfigPref, type AcpConfigPrefPatch } from './acpConfigPrefs'; + +// Re-export so picker code can import the type alongside the actions. +export type { AcpConfigPref, AcpConfigPrefPatch }; // Re-export for convenience export { isLightTheme, loadAllThemePreviewColors, type ThemePreviewColors }; @@ -41,6 +45,8 @@ const DIFF_THEME_STORE_KEY = 'diff-theme'; /** The app chrome appearance: light / dark / system. */ const APP_MODE_STORE_KEY = 'app-mode'; const RECENT_AGENTS_STORE_KEY = 'recent-agents'; +/** Per-provider last explicitly chosen model/effort selector values. */ +const ACP_CONFIG_PREFS_STORE_KEY = 'acp-config-prefs'; const AUTO_REVIEW_STORE_KEY = 'auto-start-code-reviews'; /** Maximum number of recent agents to remember. */ const RECENT_AGENTS_MAX = 10; @@ -125,6 +131,12 @@ export const preferences = $state({ * Used to pick the best available agent for a given context (local vs remote). */ recentAgents: [] as string[], + /** + * Per-provider last explicitly chosen model/effort picker values, keyed by + * provider id. Values are selector valueIds validated against live + * discovery options on restore, so stale ids fall back silently. + */ + acpConfigPrefs: {} as Record, /** Whether auto code reviews are triggered after commits */ autoReviewMode: 'after-changes' as AutoReviewMode, /** Whether all preferences have been loaded from storage */ @@ -265,6 +277,18 @@ export async function initPreferences(): Promise { } } + // Load per-provider model/effort picker preferences + const savedConfigPrefs = await getStoreValue>( + ACP_CONFIG_PREFS_STORE_KEY + ); + if ( + savedConfigPrefs && + typeof savedConfigPrefs === 'object' && + !Array.isArray(savedConfigPrefs) + ) { + preferences.acpConfigPrefs = savedConfigPrefs; + } + // Load auto-review mode const savedAutoReview = await getStoreValue(AUTO_REVIEW_STORE_KEY); if (savedAutoReview === 'never' || savedAutoReview === 'after-changes') { @@ -370,6 +394,26 @@ export function setAiAgent(agentId: string): void { setStoreValue(RECENT_AGENTS_STORE_KEY, preferences.recentAgents); } +/** + * Persist the last explicitly chosen model/effort for a provider. + * + * Only fields present in `patch` change; `null` clears a field. Untouched + * picker values are never persisted, so pickers keep tracking provider + * defaults until the user makes an explicit choice. + */ +export function setAcpConfigPref(providerId: string, patch: AcpConfigPrefPatch): void { + preferences.acpConfigPrefs = { + ...preferences.acpConfigPrefs, + [providerId]: mergeAcpConfigPref(preferences.acpConfigPrefs[providerId], patch), + }; + setStoreValue(ACP_CONFIG_PREFS_STORE_KEY, preferences.acpConfigPrefs); +} + +/** The last explicitly chosen model/effort for a provider, if any. */ +export function getAcpConfigPref(providerId: string): AcpConfigPref | null { + return preferences.acpConfigPrefs[providerId] ?? null; +} + /** * Return the most recently used agent that is present in `available`. *