Keep composer mode preferences stable (#1658)
* fix(app): preserve composer mode preferences Treat undefined provider preference updates as no-ops, and keep saved mode selections instead of silently defaulting them when provider metadata is incomplete or does not list the saved mode. * fix(app): document mode preference preservation
This commit is contained in:
@@ -81,6 +81,39 @@ describe("create agent preferences", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not erase a saved mode when a later partial update has no mode", () => {
|
||||||
|
expect(
|
||||||
|
mergeProviderPreferences({
|
||||||
|
preferences: {
|
||||||
|
provider: "codex",
|
||||||
|
providerPreferences: {
|
||||||
|
codex: {
|
||||||
|
model: "gpt-5.5",
|
||||||
|
mode: "full-access",
|
||||||
|
thinkingByModel: { "gpt-5.5": "high" },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
provider: "codex",
|
||||||
|
updates: {
|
||||||
|
model: "gpt-5.6",
|
||||||
|
mode: undefined,
|
||||||
|
thinkingByModel: undefined,
|
||||||
|
featureValues: undefined,
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
).toEqual({
|
||||||
|
provider: "codex",
|
||||||
|
providerPreferences: {
|
||||||
|
codex: {
|
||||||
|
model: "gpt-5.6",
|
||||||
|
mode: "full-access",
|
||||||
|
thinkingByModel: { "gpt-5.5": "high" },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("loads invalid stored preferences as empty preferences", () => {
|
it("loads invalid stored preferences as empty preferences", () => {
|
||||||
expect(parseFormPreferences({ providerPreferences: { codex: { mode: 42 } } })).toEqual({});
|
expect(parseFormPreferences({ providerPreferences: { codex: { mode: 42 } } })).toEqual({});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -46,6 +46,43 @@ export function parseFormPreferences(value: unknown): FormPreferences {
|
|||||||
return result.success ? result.data : DEFAULT_FORM_PREFERENCES;
|
return result.success ? result.data : DEFAULT_FORM_PREFERENCES;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function mergeDefinedRecord<T>(
|
||||||
|
existing: Record<string, T> | undefined,
|
||||||
|
updates: Record<string, T> | undefined,
|
||||||
|
): Record<string, T> | undefined {
|
||||||
|
if (updates === undefined) {
|
||||||
|
return existing;
|
||||||
|
}
|
||||||
|
return {
|
||||||
|
...existing,
|
||||||
|
...updates,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
function applyProviderPreferenceUpdates(
|
||||||
|
existing: ProviderPreferences,
|
||||||
|
updates: Partial<ProviderPreferences>,
|
||||||
|
): ProviderPreferences {
|
||||||
|
const next: ProviderPreferences = { ...existing };
|
||||||
|
const nextThinkingByModel = mergeDefinedRecord(existing.thinkingByModel, updates.thinkingByModel);
|
||||||
|
const nextFeatureValues = mergeDefinedRecord(existing.featureValues, updates.featureValues);
|
||||||
|
|
||||||
|
if (updates.model !== undefined) {
|
||||||
|
next.model = updates.model;
|
||||||
|
}
|
||||||
|
if (updates.mode !== undefined) {
|
||||||
|
next.mode = updates.mode;
|
||||||
|
}
|
||||||
|
if (nextThinkingByModel !== undefined) {
|
||||||
|
next.thinkingByModel = nextThinkingByModel;
|
||||||
|
}
|
||||||
|
if (nextFeatureValues !== undefined) {
|
||||||
|
next.featureValues = nextFeatureValues;
|
||||||
|
}
|
||||||
|
|
||||||
|
return next;
|
||||||
|
}
|
||||||
|
|
||||||
export function mergeProviderPreferences(args: {
|
export function mergeProviderPreferences(args: {
|
||||||
preferences: FormPreferences;
|
preferences: FormPreferences;
|
||||||
provider: AgentProvider;
|
provider: AgentProvider;
|
||||||
@@ -54,32 +91,13 @@ export function mergeProviderPreferences(args: {
|
|||||||
const { preferences, provider, updates } = args;
|
const { preferences, provider, updates } = args;
|
||||||
const existingProviderPreferences = preferences.providerPreferences ?? {};
|
const existingProviderPreferences = preferences.providerPreferences ?? {};
|
||||||
const existing = existingProviderPreferences[provider] ?? {};
|
const existing = existingProviderPreferences[provider] ?? {};
|
||||||
const nextThinkingByModel =
|
|
||||||
updates.thinkingByModel === undefined
|
|
||||||
? existing.thinkingByModel
|
|
||||||
: {
|
|
||||||
...existing.thinkingByModel,
|
|
||||||
...updates.thinkingByModel,
|
|
||||||
};
|
|
||||||
const nextFeatureValues =
|
|
||||||
updates.featureValues === undefined
|
|
||||||
? existing.featureValues
|
|
||||||
: {
|
|
||||||
...existing.featureValues,
|
|
||||||
...updates.featureValues,
|
|
||||||
};
|
|
||||||
|
|
||||||
return {
|
return {
|
||||||
...preferences,
|
...preferences,
|
||||||
provider,
|
provider,
|
||||||
providerPreferences: {
|
providerPreferences: {
|
||||||
...existingProviderPreferences,
|
...existingProviderPreferences,
|
||||||
[provider]: {
|
[provider]: applyProviderPreferenceUpdates(existing, updates),
|
||||||
...existing,
|
|
||||||
...updates,
|
|
||||||
...(nextThinkingByModel ? { thinkingByModel: nextThinkingByModel } : {}),
|
|
||||||
...(nextFeatureValues ? { featureValues: nextFeatureValues } : {}),
|
|
||||||
},
|
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -511,6 +511,59 @@ describe("resolveFormState", () => {
|
|||||||
expect(resolved.thinkingOptionId).toBe("xhigh");
|
expect(resolved.thinkingOptionId).toBe("xhigh");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("preserves the saved mode while provider modes are absent from a loading snapshot", () => {
|
||||||
|
const loadingEntries: ProviderSnapshotEntry[] = [
|
||||||
|
{
|
||||||
|
provider: "codex",
|
||||||
|
status: "loading",
|
||||||
|
enabled: true,
|
||||||
|
label: TEST_CODEX_DEFINITION.label,
|
||||||
|
description: TEST_CODEX_DEFINITION.description,
|
||||||
|
defaultModeId: TEST_CODEX_DEFINITION.defaultModeId,
|
||||||
|
},
|
||||||
|
];
|
||||||
|
const providerDefinitions = buildProviderDefinitions(loadingEntries);
|
||||||
|
const resolvableProviderMap = buildProviderDefinitionMapForStatuses({
|
||||||
|
snapshotEntries: loadingEntries,
|
||||||
|
providerDefinitions,
|
||||||
|
statuses: new Set<ProviderSnapshotEntry["status"]>(["ready", "loading"]),
|
||||||
|
});
|
||||||
|
|
||||||
|
const resolved = resolveFormState(
|
||||||
|
undefined,
|
||||||
|
{
|
||||||
|
provider: "codex",
|
||||||
|
providerPreferences: { codex: { mode: "full-access", model: "gpt-5.3-codex" } },
|
||||||
|
},
|
||||||
|
null,
|
||||||
|
INITIAL_USER_MODIFIED,
|
||||||
|
makeState({ provider: "codex", modeId: "full-access", model: "gpt-5.3-codex" }).form,
|
||||||
|
|
||||||
|
resolvableProviderMap,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(resolved.provider).toBe("codex");
|
||||||
|
expect(resolved.modeId).toBe("full-access");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("preserves a saved mode that is not in the current mode list", () => {
|
||||||
|
const resolved = resolveFormState(
|
||||||
|
undefined,
|
||||||
|
{
|
||||||
|
provider: "codex",
|
||||||
|
providerPreferences: { codex: { mode: "workspace-write", model: "gpt-5.3-codex" } },
|
||||||
|
},
|
||||||
|
CODEX_MODELS,
|
||||||
|
INITIAL_USER_MODIFIED,
|
||||||
|
makeState({ provider: "codex" }).form,
|
||||||
|
|
||||||
|
codexProviderMap,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(resolved.provider).toBe("codex");
|
||||||
|
expect(resolved.modeId).toBe("workspace-write");
|
||||||
|
});
|
||||||
|
|
||||||
it("ignores disabled ready providers when resolving selectable defaults", () => {
|
it("ignores disabled ready providers when resolving selectable defaults", () => {
|
||||||
const entries: ProviderSnapshotEntry[] = [
|
const entries: ProviderSnapshotEntry[] = [
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -144,6 +144,24 @@ export function resolveThinkingOptionId(args: {
|
|||||||
return effectiveModel?.defaultThinkingOptionId ?? thinkingOptions[0]?.id ?? "";
|
return effectiveModel?.defaultThinkingOptionId ?? thinkingOptions[0]?.id ?? "";
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const normalizeSelectedModeId = normalizeSelectedModelId;
|
||||||
|
|
||||||
|
function resolvePreferredModeId(input: {
|
||||||
|
initialModeId?: string | null;
|
||||||
|
preferredModeId?: string | null;
|
||||||
|
providerDef: AgentProviderDefinition | undefined;
|
||||||
|
}): string {
|
||||||
|
// Saved modes are user intent. Provider create config validates unknown modes
|
||||||
|
// at submission time, so background form resolution should not erase them.
|
||||||
|
const initialModeId = normalizeSelectedModeId(input.initialModeId);
|
||||||
|
if (initialModeId) return initialModeId;
|
||||||
|
|
||||||
|
const preferredModeId = normalizeSelectedModeId(input.preferredModeId);
|
||||||
|
if (preferredModeId) return preferredModeId;
|
||||||
|
|
||||||
|
return input.providerDef?.defaultModeId ?? input.providerDef?.modes[0]?.id ?? "";
|
||||||
|
}
|
||||||
|
|
||||||
export function mergeSelectedComposerPreferences(args: {
|
export function mergeSelectedComposerPreferences(args: {
|
||||||
preferences: FormPreferences;
|
preferences: FormPreferences;
|
||||||
provider: AgentProvider;
|
provider: AgentProvider;
|
||||||
@@ -259,18 +277,11 @@ function resolveModeId(input: {
|
|||||||
input;
|
input;
|
||||||
if (userModified) return currentModeId;
|
if (userModified) return currentModeId;
|
||||||
if (!provider) return "";
|
if (!provider) return "";
|
||||||
const validModeIds = providerDef?.modes.map((m) => m.id) ?? [];
|
return resolvePreferredModeId({
|
||||||
if (
|
initialModeId: initialValues?.modeId,
|
||||||
typeof initialValues?.modeId === "string" &&
|
preferredModeId: providerPrefs?.mode,
|
||||||
initialValues.modeId.length > 0 &&
|
providerDef,
|
||||||
validModeIds.includes(initialValues.modeId)
|
});
|
||||||
) {
|
|
||||||
return initialValues.modeId;
|
|
||||||
}
|
|
||||||
if (providerPrefs?.mode && validModeIds.includes(providerPrefs.mode)) {
|
|
||||||
return providerPrefs.mode;
|
|
||||||
}
|
|
||||||
return providerDef?.defaultModeId ?? validModeIds[0] ?? "";
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function resolveModelField(input: {
|
function resolveModelField(input: {
|
||||||
@@ -411,11 +422,10 @@ function pickNextModeForProvider(input: {
|
|||||||
providerPrefs: ProviderPrefs | undefined;
|
providerPrefs: ProviderPrefs | undefined;
|
||||||
}): string {
|
}): string {
|
||||||
const { providerDef, providerPrefs } = input;
|
const { providerDef, providerPrefs } = input;
|
||||||
const validModeIds = providerDef?.modes.map((m) => m.id) ?? [];
|
return resolvePreferredModeId({
|
||||||
if (providerPrefs?.mode && validModeIds.includes(providerPrefs.mode)) {
|
preferredModeId: providerPrefs?.mode,
|
||||||
return providerPrefs.mode;
|
providerDef,
|
||||||
}
|
});
|
||||||
return providerDef?.defaultModeId ?? "";
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function pickNextModeForProviderAndModel(input: {
|
function pickNextModeForProviderAndModel(input: {
|
||||||
@@ -425,14 +435,8 @@ function pickNextModeForProviderAndModel(input: {
|
|||||||
providerDef: AgentProviderDefinition | undefined;
|
providerDef: AgentProviderDefinition | undefined;
|
||||||
providerPrefs: ProviderPrefs | undefined;
|
providerPrefs: ProviderPrefs | undefined;
|
||||||
}): string {
|
}): string {
|
||||||
const validModeIds = input.providerDef?.modes.map((m) => m.id) ?? [];
|
const currentModeId = normalizeSelectedModeId(input.currentModeId);
|
||||||
if (
|
if (input.currentProvider === input.provider && currentModeId) return currentModeId;
|
||||||
input.currentProvider === input.provider &&
|
|
||||||
input.currentModeId &&
|
|
||||||
validModeIds.includes(input.currentModeId)
|
|
||||||
) {
|
|
||||||
return input.currentModeId;
|
|
||||||
}
|
|
||||||
return pickNextModeForProvider({
|
return pickNextModeForProvider({
|
||||||
providerDef: input.providerDef,
|
providerDef: input.providerDef,
|
||||||
providerPrefs: input.providerPrefs,
|
providerPrefs: input.providerPrefs,
|
||||||
|
|||||||
Reference in New Issue
Block a user