From 59fd0ec13a1410cd9dba50da8eef29c42cdea68a Mon Sep 17 00:00:00 2001 From: Adam Higgins Date: Tue, 3 Nov 2026 09:16:58 +0000 Subject: [PATCH] fix(web): a deleted default provider leaves the provider list Deleting Codex or Claude saved a disabled entry marked with config.enabled: false, but settings normalization folds that flag into the envelope and drops it, so the provider stayed listed as Disabled. Mark the entry with its own config key instead, and clear it when the provider is edited or re-enabled. Co-Authored-By: Claude Opus 5.5 --- apps/server/src/serverSettings.test.ts | 27 ++++++++++++++ ...SettingsPanel.environment.browser.test.tsx | 7 ++-- .../settings/ProviderSettingsPanel.tsx | 36 +++++-------------- packages/contracts/src/providerInstance.ts | 34 ++++++++++++++++++ 4 files changed, 73 insertions(+), 31 deletions(-) diff --git a/apps/server/src/serverSettings.test.ts b/apps/server/src/serverSettings.test.ts index 3e809ef6cd4cc..fbea8cfa8b605 100644 --- a/apps/server/src/serverSettings.test.ts +++ b/apps/server/src/serverSettings.test.ts @@ -1,14 +1,17 @@ import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; import * as NodeServices from "@effect/platform-node/NodeServices"; import { + clearRemovedFromProviderList, DEFAULT_SERVER_SETTINGS, globalIdleCompaction, + isRemovedDefaultProviderInstance, type ManagedPolicyDocument, ModelSelection, ProjectId, ProjectScript, ProviderDriverKind, ProviderInstanceId, + removedDefaultProviderInstance, resolveProviderInstanceEnabled, SecretProviderId, ServerSettings, @@ -1178,6 +1181,30 @@ it.layer(Layer.mergeAll(NodeServices.layer, NodeCrypto.layer))("server settings" }).pipe(Effect.provide(layerServerSettings())), ); + it.effect("keeps a removed default provider marked until it is edited", () => + Effect.gen(function* () { + const serverSettings = yield* ServerSettingsModule.ServerSettingsService; + const codexId = ProviderInstanceId.make("codex"); + const removed = removedDefaultProviderInstance(ProviderDriverKind.make("codex")); + + yield* serverSettings.updateProviderInstance({ + operation: "upsert", + instanceId: codexId, + instance: removed, + }); + const afterRemove = (yield* serverSettings.getSettings).providerInstances[codexId]!; + assert.isTrue(isRemovedDefaultProviderInstance(afterRemove)); + + yield* serverSettings.updateProviderInstance({ + operation: "upsert", + instanceId: codexId, + instance: clearRemovedFromProviderList({ ...afterRemove, enabled: true }), + }); + const reenabled = (yield* serverSettings.getSettings).providerInstances[codexId]!; + assert.isFalse(isRemovedDefaultProviderInstance({ ...reenabled, enabled: false })); + }).pipe(Effect.provide(layerServerSettings())), + ); + it.effect("reads settings secrets once per settings change", () => { let reads = 0; const layerSecret = Layer.effect( diff --git a/apps/web/src/components/settings/ProviderSettingsPanel.environment.browser.test.tsx b/apps/web/src/components/settings/ProviderSettingsPanel.environment.browser.test.tsx index ce0383e2105d5..abdb23d23c77e 100644 --- a/apps/web/src/components/settings/ProviderSettingsPanel.environment.browser.test.tsx +++ b/apps/web/src/components/settings/ProviderSettingsPanel.environment.browser.test.tsx @@ -4,6 +4,7 @@ import { EnvironmentId, ProviderDriverKind, ProviderInstanceId, + removedDefaultProviderInstance, type ProviderAuthState, type ServerProvider, type UnifiedSettings, @@ -360,7 +361,7 @@ describe("EnvironmentProviderSettings routing", () => { expect(resetMutation).toEqual({ operation: "upsert", instanceId: codexId, - instance: { driver: "codex", enabled: false, config: { enabled: false } }, + instance: removedDefaultProviderInstance(ProviderDriverKind.make("codex")), }); // Deleting touches only the instance; shared preferences stay untouched. expect(resetPatch ?? {}).toEqual({}); @@ -379,7 +380,7 @@ describe("EnvironmentProviderSettings routing", () => { expect(mutation).toEqual({ operation: "upsert", instanceId: codexId, - instance: { driver: "codex", enabled: false, config: { enabled: false } }, + instance: removedDefaultProviderInstance(ProviderDriverKind.make("codex")), }); state.settings = { @@ -547,7 +548,7 @@ describe("EnvironmentProviderSettings instance removal", () => { expect(state.mutateProviderInstance).toHaveBeenCalledWith({ operation: "upsert", instanceId: codexId, - instance: { driver: "codex", enabled: false, config: { enabled: false } }, + instance: removedDefaultProviderInstance(ProviderDriverKind.make("codex")), }), ); }); diff --git a/apps/web/src/components/settings/ProviderSettingsPanel.tsx b/apps/web/src/components/settings/ProviderSettingsPanel.tsx index c1723f8327696..129791395afb4 100644 --- a/apps/web/src/components/settings/ProviderSettingsPanel.tsx +++ b/apps/web/src/components/settings/ProviderSettingsPanel.tsx @@ -15,8 +15,10 @@ import { squashAtomCommandFailure, } from "@t3tools/client-runtime/state/runtime"; import { + clearRemovedFromProviderList, defaultInstanceIdForDriver, type EnvironmentId, + isRemovedDefaultProviderInstance, type AcpRegistryUrlAuthAction, PROVIDER_DISPLAY_NAMES, ProviderDriverKind, @@ -26,6 +28,7 @@ import { type ServerProvider, resolveEnvironmentMachineKind, resolveProjectProviderInstanceEnabled, + removedDefaultProviderInstance, resolveProviderInstanceEnabled, } from "@t3tools/contracts"; import { DEFAULT_UNIFIED_SETTINGS, type UnifiedSettings } from "@t3tools/contracts/settings"; @@ -168,25 +171,6 @@ const PROVIDER_SETTINGS = providerClients.definitions.map((definition) => ({ hasDefaultInstance: definition.hasDefaultInstance !== false, })); -/** - * The entry `deleteDefaultInstance` leaves for a built-in that runs unconfigured - * (Codex, Claude): disabled on the envelope and in its config, with nothing else - * set. Turning a provider off only sets the envelope flag, so it stays listed. - */ -const DELETED_DEFAULT_INSTANCE_CONFIG = { enabled: false } as const; - -function isDeletedDefaultInstance(instance: ProviderInstanceConfig): boolean { - const { driver: _driver, enabled, config, ...rest } = instance; - return ( - enabled === false && - Object.keys(rest).length === 0 && - typeof config === "object" && - config !== null && - Object.keys(config).length === 1 && - (config as { enabled?: unknown }).enabled === false - ); -} - function configuredBinaryPath(config: unknown): string { if (config === null || typeof config !== "object" || !("binaryPath" in config)) return ""; return typeof config.binaryPath === "string" ? config.binaryPath.trim() : ""; @@ -773,10 +757,10 @@ export function EnvironmentProviderSettings({ const hasDefaultSlot = providerSettings.hasDefaultInstance || explicitInstance !== undefined; // A slot is listed when the user configured it or it is live. An untouched, // disabled slot is a provider type the user never set up, and a deleted - // built-in (see `deleteDefaultInstance`) is one they removed; both stay out + // built-in (see `resetDefaultInstance`) is one they removed; both stay out // of the list until the + dialog adds them back. A deep-linked slot always shows. const isConfigured = - explicitInstance !== undefined && !isDeletedDefaultInstance(explicitInstance); + explicitInstance !== undefined && !isRemovedDefaultProviderInstance(explicitInstance); if ( hasDefaultSlot && (isConfigured || @@ -834,7 +818,7 @@ export function EnvironmentProviderSettings({ textGenerationModelSelection, }); const result = await persistProviderInstance( - { operation: "upsert", instanceId, instance: next }, + { operation: "upsert", instanceId, instance: clearRemovedFromProviderList(next) }, patch, ); if (result._tag === "Failure" && !isAtomCommandInterrupted(result)) { @@ -915,7 +899,7 @@ export function EnvironmentProviderSettings({ // Deleting a default slot drops it out of the provider list; it can be re-added // from the + dialog like any provider. A built-in that runs unconfigured (Codex, // Claude) would come straight back as enabled once its entry is gone, so it keeps - // the disabled marker entry `isDeletedDefaultInstance` recognizes instead. + // the disabled marker entry `removedDefaultProviderInstance` writes instead. const resetDefaultInstance = useStableCallback(async (driverKind: ProviderDriverKind) => { const instanceId = defaultInstanceIdForDriver(driverKind); const instance = settings.providerInstances?.[instanceId] ?? { driver: driverKind }; @@ -929,11 +913,7 @@ export function EnvironmentProviderSettings({ ? { operation: "upsert", instanceId, - instance: { - driver: driverKind, - enabled: false, - config: DELETED_DEFAULT_INSTANCE_CONFIG, - }, + instance: removedDefaultProviderInstance(driverKind), } : { operation: "remove", instanceId }, ); diff --git a/packages/contracts/src/providerInstance.ts b/packages/contracts/src/providerInstance.ts index 61fee930c3193..8271de9122346 100644 --- a/packages/contracts/src/providerInstance.ts +++ b/packages/contracts/src/providerInstance.ts @@ -82,6 +82,40 @@ export const ProviderInstanceConfig = Schema.Struct({ }); export type ProviderInstanceConfig = typeof ProviderInstanceConfig.Type; +// Not `config.enabled`: settings normalization folds that into the envelope and drops it. +const REMOVED_FROM_PROVIDER_LIST_KEY = "removedFromProviderList"; + +/** A removed built-in that runs unconfigured (Codex, Claude): kept as a disabled entry clients leave out of the provider list. */ +export const removedDefaultProviderInstance = ( + driver: ProviderDriverKind, +): ProviderInstanceConfig => ({ + driver, + enabled: false, + config: { [REMOVED_FROM_PROVIDER_LIST_KEY]: true }, +}); + +const removedMarkerConfig = (config: unknown): Record | undefined => + config !== null && + typeof config === "object" && + !Array.isArray(config) && + (config as Record)[REMOVED_FROM_PROVIDER_LIST_KEY] === true + ? (config as Record) + : undefined; + +export const isRemovedDefaultProviderInstance = (instance: ProviderInstanceConfig): boolean => + instance.enabled === false && removedMarkerConfig(instance.config) !== undefined; + +/** Any edit to a removed entry, such as turning it back on, makes it an ordinary listed instance. */ +export const clearRemovedFromProviderList = ( + instance: ProviderInstanceConfig, +): ProviderInstanceConfig => { + const config = removedMarkerConfig(instance.config); + if (config === undefined) return instance; + const { [REMOVED_FROM_PROVIDER_LIST_KEY]: _removed, ...rest } = config; + const { config: _config, ...envelope } = instance; + return Object.keys(rest).length > 0 ? { ...envelope, config: rest } : envelope; +}; + export const ProviderInstanceMutation = Schema.Union([ Schema.Struct({ operation: Schema.Literal("create"),