fix(providers): keep removal live after config updates

This commit is contained in:
Mohamed Boudra
2026-07-16 15:10:53 +02:00
parent 1d20ba5250
commit f3abaf850e
8 changed files with 288 additions and 25 deletions
@@ -1,4 +1,4 @@
import { useCallback, useMemo, useState } from "react";
import { useCallback, useMemo, useRef, useState } from "react";
import type { TFunction } from "i18next";
import { useTranslation } from "react-i18next";
import {
@@ -95,6 +95,10 @@ interface ProviderActionsMenuProps {
providerId: string;
providerLabel: string;
isRemoving: boolean;
iconSize: number;
foregroundColor: string;
foregroundMutedColor: string;
dangerColor: string;
onRemove: (providerId: string, providerLabel: string) => void;
}
@@ -102,10 +106,13 @@ function ProviderActionsMenu({
providerId,
providerLabel,
isRemoving,
iconSize,
foregroundColor,
foregroundMutedColor,
dangerColor,
onRemove,
}: ProviderActionsMenuProps) {
const { t } = useTranslation();
const { theme } = useUnistyles();
const handleRemove = useCallback(() => {
onRemove(providerId, providerLabel);
}, [onRemove, providerId, providerLabel]);
@@ -121,14 +128,12 @@ function ProviderActionsMenu({
],
[],
);
const trashLeading = useMemo(
() => <Trash2 size={16} color={theme.colors.statusDanger} />,
[theme.colors.statusDanger],
);
const trashLeading = useMemo(() => <Trash2 size={16} color={dangerColor} />, [dangerColor]);
return (
<DropdownMenu>
<DropdownMenuTrigger
disabled={isRemoving}
hitSlop={8}
onPressIn={stopPressInPropagation}
style={triggerStyle}
@@ -138,8 +143,8 @@ function ProviderActionsMenu({
>
{({ hovered, open }) => (
<MoreHorizontal
size={theme.iconSize.sm}
color={hovered || open ? theme.colors.foreground : theme.colors.foregroundMuted}
size={iconSize}
color={hovered || open ? foregroundColor : foregroundMutedColor}
/>
)}
</DropdownMenuTrigger>
@@ -246,6 +251,10 @@ function ProviderRow({
providerId={def.id}
providerLabel={def.label}
isRemoving={isRemoving}
iconSize={theme.iconSize.sm}
foregroundColor={theme.colors.foreground}
foregroundMutedColor={theme.colors.foregroundMuted}
dangerColor={theme.colors.statusDanger}
onRemove={onRemove}
/>
) : null}
@@ -312,6 +321,7 @@ export function ProvidersSection({ serverId }: ProvidersSectionProps) {
const openProviderSettings = useProviderSettingsStore((state) => state.open);
const [pendingProviderId, setPendingProviderId] = useState<string | null>(null);
const [removingProviderId, setRemovingProviderId] = useState<string | null>(null);
const removingProviderIdRef = useRef<string | null>(null);
const [installingProviderId, setInstallingProviderId] = useState<string | null>(null);
const providerDefinitions = useMemo(() => buildProviderDefinitions(entries), [entries]);
@@ -343,18 +353,20 @@ export function ProvidersSection({ serverId }: ProvidersSectionProps) {
const handleRemoveProvider = useCallback(
async (providerId: string, providerLabel: string) => {
const confirmed = await confirmDialog({
title: t("settings.providers.remove.confirmTitle", { name: providerLabel }),
message: t("settings.providers.remove.confirmMessage"),
confirmLabel: t("settings.providers.remove.confirm"),
destructive: true,
});
if (!confirmed) {
return;
}
if (removingProviderIdRef.current) return;
removingProviderIdRef.current = providerId;
setRemovingProviderId(providerId);
try {
const confirmed = await confirmDialog({
title: t("settings.providers.remove.confirmTitle", { name: providerLabel }),
message: t("settings.providers.remove.confirmMessage"),
confirmLabel: t("settings.providers.remove.confirm"),
destructive: true,
});
if (!confirmed) {
return;
}
await patchConfig({ removeProviders: [providerId] });
} catch (error) {
Alert.alert(
@@ -362,6 +374,9 @@ export function ProvidersSection({ serverId }: ProvidersSectionProps) {
error instanceof Error ? error.message : String(error),
);
} finally {
if (removingProviderIdRef.current === providerId) {
removingProviderIdRef.current = null;
}
setRemovingProviderId((current) => (current === providerId ? null : current));
}
},
@@ -2272,6 +2272,47 @@ test("updateProviderRegistry registers a previously unknown provider", async ()
expect(snapshot.config.provider).toBe("codex");
});
test("updateProviderRegistry removes providers omitted from the next registry", async () => {
const workdir = mkdtempSync(join(tmpdir(), "agent-manager-test-"));
const storagePath = join(workdir, "agents");
const storage = new AgentStorage(storagePath, logger);
const removedProvider = "zai-claude" as AgentProvider;
class RemovedProviderClient extends TestAgentClient {
createSessionCalls = 0;
override async createSession(config: AgentSessionConfig): Promise<AgentSession> {
this.createSessionCalls += 1;
return await super.createSession(config);
}
}
const removedClient = new RemovedProviderClient();
const manager = new AgentManager({
clients: { codex: new TestAgentClient(), [removedProvider]: removedClient },
providerDefinitions: {
codex: { enabled: true },
[removedProvider]: { enabled: true },
},
registry: storage,
logger,
});
expect(manager.getRegisteredProviderIds()).toContain(removedProvider);
manager.updateProviderRegistry({
providerDefinitions: { codex: { enabled: true } },
clients: { codex: new TestAgentClient() },
});
expect(manager.getRegisteredProviderIds()).not.toContain(removedProvider);
await expect(
manager.createAgent({ provider: removedProvider, cwd: workdir }, undefined, {
workspaceId: undefined,
}),
).rejects.toThrow("Unknown provider 'zai-claude'");
expect(removedClient.createSessionCalls).toBe(0);
});
test("createAgent passes explicit model strings through to the provider", async () => {
const workdir = mkdtempSync(join(tmpdir(), "agent-manager-test-"));
const storagePath = join(workdir, "agents");
@@ -622,11 +622,14 @@ export class AgentManager {
providerDefinitions: ProviderEnabledMap;
clients: ProviderClientMap;
}): void {
this.providerEnabled.clear();
for (const [provider, definition] of Object.entries(input.providerDefinitions)) {
if (definition) {
this.providerEnabled.set(provider, definition.enabled);
}
}
this.clients.clear();
for (const [provider, client] of Object.entries(input.clients)) {
if (client) {
this.clients.set(provider, client);
@@ -960,6 +960,29 @@ describe("ProviderSnapshotManager applyMutableProviderConfig", () => {
}
});
test("removes startup provider overrides from the live registry", () => {
const manager = new ProviderSnapshotManager({
logger: createTestLogger(),
providerOverrides: {
"zai-claude": { extends: "claude", label: "ZAI", enabled: true },
},
});
try {
expect(manager.hasProvider("zai-claude")).toBe(true);
const state = manager.applyMutableProviderConfig({}, { removeProviders: ["zai-claude"] });
expect(manager.hasProvider("zai-claude")).toBe(false);
expect(state.providerDefinitions["zai-claude"]).toBeUndefined();
expect(manager.getSnapshot().some((entry) => entry.provider === "zai-claude")).toBe(false);
manager.applyMutableProviderConfig({ codex: { enabled: false } });
expect(manager.hasProvider("zai-claude")).toBe(false);
} finally {
manager.destroy();
}
});
test("drops disabled built-in providers from clients while preserving providerDefinitions", () => {
const manager = new ProviderSnapshotManager({
logger: createTestLogger(),
@@ -65,6 +65,22 @@ function resolveDiagnosticTimeoutMs(option: number | undefined, refreshTimeoutMs
return Math.max(refreshTimeoutMs, DEFAULT_DIAGNOSTIC_TIMEOUT_MS);
}
function omitProviderOverrides(
overrides: Record<string, ProviderOverride> | undefined,
providers: readonly string[],
): Record<string, ProviderOverride> | undefined {
if (!overrides || providers.length === 0) {
return overrides;
}
const nextOverrides = { ...overrides };
for (const provider of providers) {
delete nextOverrides[provider];
}
return Object.keys(nextOverrides).length > 0 ? nextOverrides : undefined;
}
type ProviderSnapshotChangeListener = (entries: ProviderSnapshotEntry[], cwd: string) => void;
export interface ProviderSnapshotManagerOptions {
@@ -95,6 +111,10 @@ interface ProviderSnapshotReadOptions {
wait?: boolean;
}
interface ApplyMutableProviderConfigOptions {
removeProviders?: readonly string[];
}
interface ProviderSnapshotProviderOptions {
cwd?: string | null;
provider: AgentProvider;
@@ -164,7 +184,7 @@ export class ProviderSnapshotManager {
private readonly extraClients: Partial<Record<AgentProvider, AgentClient>>;
private runtimeSettings: AgentProviderRuntimeSettingsMap | undefined;
private providerOverrides: Record<string, ProviderOverride> | undefined;
private readonly baseProviderOverrides: Record<string, ProviderOverride> | undefined;
private baseProviderOverrides: Record<string, ProviderOverride> | undefined;
private providerRegistry: Record<AgentProvider, ProviderDefinition>;
private providerClients: Record<AgentProvider, AgentClient>;
@@ -370,7 +390,12 @@ export class ProviderSnapshotManager {
applyMutableProviderConfig(
mutableProviders: MutableDaemonConfig["providers"] | undefined,
options: ApplyMutableProviderConfigOptions = {},
): AgentManagerProviderState {
this.baseProviderOverrides = omitProviderOverrides(
this.baseProviderOverrides,
options.removeProviders ?? [],
);
this.providerOverrides = applyMutableProviderConfigToOverrides(
this.baseProviderOverrides,
mutableProviders,
@@ -218,6 +218,121 @@ describe("DaemonConfigStore", () => {
expect(persisted.agents?.providers).toBeUndefined();
});
test("patch removes deleted providers from metadata generation", () => {
const paseoHome = mkdtempSync(path.join(tmpdir(), "paseo-daemon-config-store-"));
tempDirs.push(paseoHome);
const configPath = path.join(paseoHome, "config.json");
writeFileSync(
configPath,
`${JSON.stringify(
{
version: 1,
agents: {
providers: {
gemini: {
extends: "acp",
label: "Gemini",
command: ["gemini", "--acp"],
},
claude: {
enabled: false,
},
},
metadataGeneration: {
providers: [
{ provider: "gemini", model: "flash" },
{ provider: "claude", model: "haiku" },
],
},
},
},
null,
2,
)}\n`,
);
const store = new DaemonConfigStore(
paseoHome,
{
mcp: { injectIntoAgents: false },
browserTools: { enabled: false },
providers: {
gemini: {},
claude: { enabled: false },
},
metadataGeneration: {
providers: [
{ provider: "gemini", model: "flash" },
{ provider: "claude", model: "haiku" },
],
},
autoArchiveAfterMerge: false,
enableTerminalAgentHooks: false,
appendSystemPrompt: "",
},
undefined,
);
const next = store.patch({ removeProviders: ["gemini"] });
expect(next.metadataGeneration.providers).toEqual([{ provider: "claude", model: "haiku" }]);
const persisted = loadPersistedConfig(paseoHome);
expect(persisted.agents?.metadataGeneration).toEqual({
providers: [{ provider: "claude", model: "haiku" }],
});
});
test("patch persists provider removal when in-memory config is already clean", () => {
const paseoHome = mkdtempSync(path.join(tmpdir(), "paseo-daemon-config-store-"));
tempDirs.push(paseoHome);
const configPath = path.join(paseoHome, "config.json");
writeFileSync(
configPath,
`${JSON.stringify(
{
version: 1,
agents: {
providers: {
gemini: {
extends: "acp",
label: "Gemini",
command: ["gemini", "--acp"],
},
},
metadataGeneration: {
providers: [{ provider: "gemini", model: "flash" }],
},
},
},
null,
2,
)}\n`,
);
const store = new DaemonConfigStore(
paseoHome,
{
mcp: { injectIntoAgents: false },
browserTools: { enabled: false },
providers: {},
metadataGeneration: { providers: [] },
autoArchiveAfterMerge: false,
enableTerminalAgentHooks: false,
appendSystemPrompt: "",
},
undefined,
);
const next = store.patch({ removeProviders: ["gemini"] });
expect(next.providers.gemini).toBeUndefined();
const persisted = loadPersistedConfig(paseoHome);
expect(persisted.agents?.providers).toBeUndefined();
expect(persisted.agents?.metadataGeneration).toEqual({ providers: [] });
});
test("patch persists append system prompt into config.json", () => {
const paseoHome = mkdtempSync(path.join(tmpdir(), "paseo-daemon-config-store-"));
tempDirs.push(paseoHome);
@@ -20,7 +20,11 @@ interface LoggerLike {
info(...args: unknown[]): void;
}
type ConfigListener = (config: MutableDaemonConfig) => void;
export interface DaemonConfigChangeDetails {
removedProviders: readonly string[];
}
type ConfigListener = (config: MutableDaemonConfig, details: DaemonConfigChangeDetails) => void;
type FieldChangeHandler = (value: unknown) => void;
function getLogger(logger: LoggerLike | undefined): LoggerLike | undefined {
@@ -72,6 +76,30 @@ function omitProvidersFromConfig<T extends { providers?: Record<string, unknown>
return changed ? ({ ...config, providers: nextProviders } as T) : config;
}
function omitMetadataGenerationProvidersFromConfig<
T extends { metadataGeneration?: { providers?: Array<{ provider?: unknown }> } },
>(config: T, providers: readonly string[]): T {
if (providers.length === 0 || !config.metadataGeneration?.providers) {
return config;
}
const removedProviderIds = new Set(providers);
const nextProviders = config.metadataGeneration.providers.filter((entry) => {
return typeof entry.provider !== "string" || !removedProviderIds.has(entry.provider);
});
if (nextProviders.length === config.metadataGeneration.providers.length) {
return config;
}
return {
...config,
metadataGeneration: {
...config.metadataGeneration,
providers: nextProviders,
},
} as T;
}
function omitProvidersFromOverrides(
overrides: Record<string, ProviderOverride> | undefined,
providers: readonly string[],
@@ -148,20 +176,31 @@ export class DaemonConfigStore {
public patch(partial: MutableDaemonConfigPatch): MutableDaemonConfig {
const parsedPatch = MutableDaemonConfigPatchSchema.parse(partial);
const { removeProviders = [], ...configPatch } = parsedPatch;
const removedProviders = Array.from(new Set(removeProviders));
const merged = deepMerge(this.current, configPatch);
const next = MutableDaemonConfigSchema.parse(omitProvidersFromConfig(merged, removeProviders));
const next = MutableDaemonConfigSchema.parse(
omitMetadataGenerationProvidersFromConfig(
omitProvidersFromConfig(merged, removedProviders),
removedProviders,
),
);
const changedFieldPaths = Array.from(this.fieldChangeHandlers.keys()).filter((path) => {
return !isEqualValue(getValueAtPath(this.current, path), getValueAtPath(next, path));
});
const configChanged = !isEqualValue(this.current, next);
if (changedFieldPaths.length === 0 && isEqualValue(this.current, next)) {
if (!configChanged && removedProviders.length === 0) {
return this.current;
}
// Persist before updating in-memory state so that if persistence fails,
// runtime and disk stay consistent.
this.persistConfig(next, removeProviders);
this.persistConfig(next, removedProviders);
if (!configChanged) {
return this.current;
}
this.current = next;
for (const path of changedFieldPaths) {
@@ -175,8 +214,9 @@ export class DaemonConfigStore {
}
}
const changeDetails: DaemonConfigChangeDetails = { removedProviders };
for (const listener of this.changeListeners) {
listener(next);
listener(next, changeDetails);
}
return next;
@@ -554,9 +554,10 @@ export class VoiceAssistantWebSocketServer {
this.speech?.onReadinessChange((snapshot) => {
this.publishSpeechReadiness(snapshot);
}) ?? null;
this.unsubscribeDaemonConfigChange = this.daemonConfigStore.onChange((config) => {
this.unsubscribeDaemonConfigChange = this.daemonConfigStore.onChange((config, details) => {
const nextAgentManagerState = this.providerSnapshotManager.applyMutableProviderConfig(
config.providers,
{ removeProviders: details.removedProviders },
);
this.agentManager.updateProviderRegistry(nextAgentManagerState);
this.broadcastDaemonConfigChanged(config);