From 7db8ea1a6b7848f2e99b434b99b5026059366280 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 1 May 2026 18:28:40 -0700 Subject: [PATCH 1/6] Refactor provider settings into declarative form - Centralize provider field metadata and rendering - Reuse the same form in add-instance and instance cards - Add coverage for schema-driven field derivation and config merging --- .../settings/AddProviderInstanceDialog.tsx | 89 +++---- .../settings/ProviderInstanceCard.tsx | 81 ++---- .../settings/ProviderSettingsForm.test.ts | 44 ++++ .../settings/ProviderSettingsForm.tsx | 220 +++++++++++++++++ .../components/settings/SettingsPanels.tsx | 57 +---- .../components/settings/providerDriverMeta.ts | 231 +++++++++++------- 6 files changed, 460 insertions(+), 262 deletions(-) create mode 100644 apps/web/src/components/settings/ProviderSettingsForm.test.ts create mode 100644 apps/web/src/components/settings/ProviderSettingsForm.tsx diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx index b8bce8f0cd9..b86065dd5ff 100644 --- a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx @@ -13,7 +13,7 @@ import { useSettings, useUpdateSettings } from "../../hooks/useSettings"; import { cn } from "../../lib/utils"; import { normalizeProviderAccentColor } from "../../providerInstances"; import { Button } from "../ui/button"; -import { ACPRegistryIcon, Gemini, GithubCopilotIcon, PiAgentIcon } from "../Icons"; +import { ACPRegistryIcon, Gemini, GithubCopilotIcon, PiAgentIcon, type Icon } from "../Icons"; import { Dialog, DialogDescription, @@ -26,7 +26,8 @@ import { Badge } from "../ui/badge"; import { Input } from "../ui/input"; import { RadioGroup } from "../ui/radio-group"; import { toastManager } from "../ui/toast"; -import { DRIVER_OPTION_BY_VALUE, DRIVER_OPTIONS, type DriverOption } from "./providerDriverMeta"; +import { DRIVER_OPTION_BY_VALUE, DRIVER_OPTIONS } from "./providerDriverMeta"; +import { ProviderSettingsForm, deriveProviderSettingsFields } from "./ProviderSettingsForm"; const PROVIDER_ACCENT_SWATCHES = [ "#2563eb", @@ -61,30 +62,32 @@ function deriveInstanceId(driver: ProviderDriverKind, label: string): string { const INSTANCE_ID_PATTERN = /^[a-zA-Z][a-zA-Z0-9_-]*$/; const DEFAULT_DRIVER_KIND = ProviderDriverKind.make("codex"); const DEFAULT_DRIVER_OPTION = DRIVER_OPTIONS[0]!; -const COMING_SOON_DRIVER_OPTIONS: readonly DriverOption[] = [ +interface ComingSoonDriverOption { + readonly value: ProviderDriverKind; + readonly label: string; + readonly icon: Icon; +} + +const COMING_SOON_DRIVER_OPTIONS: readonly ComingSoonDriverOption[] = [ { value: ProviderDriverKind.make("githubCopilot"), label: "Github Copilot", icon: GithubCopilotIcon, - fields: [], }, { value: ProviderDriverKind.make("gemini"), label: "Gemini", icon: Gemini, - fields: [], }, { value: ProviderDriverKind.make("acpRegistry"), label: "ACP Registry", icon: ACPRegistryIcon, - fields: [], }, { value: ProviderDriverKind.make("piAgent"), label: "Pi Agent", icon: PiAgentIcon, - fields: [], }, ]; @@ -118,10 +121,9 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns const [accentColor, setAccentColor] = useState(""); const [instanceId, setInstanceId] = useState(""); const [instanceIdDirty, setInstanceIdDirty] = useState(false); - // Driver-specific field values keyed by `${driver}:${fieldKey}` so toggling - // between drivers during the same dialog session doesn't lose in-progress - // input. Only the active driver's values are persisted on save. - const [fieldValues, setFieldValues] = useState>({}); + // Driver-specific config drafts keyed by driver so toggling between drivers + // during the same dialog session does not lose in-progress input. + const [configByDriver, setConfigByDriver] = useState>>({}); // Errors are suppressed until the user has tried to submit once. After that // they update live so fixing the problem clears the message in place. const [hasAttemptedSubmit, setHasAttemptedSubmit] = useState(false); @@ -141,7 +143,7 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns setInstanceId(""); setWizardStep(0); setInstanceIdDirty(false); - setFieldValues({}); + setConfigByDriver({}); setHasAttemptedSubmit(false); }, [open]); @@ -153,23 +155,28 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns }, [driver, label, instanceIdDirty]); const driverOption = DRIVER_OPTION_BY_VALUE[driver] ?? DEFAULT_DRIVER_OPTION; + const driverSettingsFields = useMemo( + () => deriveProviderSettingsFields(driverOption), + [driverOption], + ); const instanceIdError = validateInstanceId(instanceId, existingIds); const showInstanceIdError = hasAttemptedSubmit && instanceIdError !== null; const previewLabel = label.trim() || `${driverOption.label} Workspace`; const wizardSteps = ["Driver", "Identity", "Config"] as const; const wizardStepSummaries = [driverOption.label, previewLabel, null] as const; - const getFieldValue = useCallback( - (fieldKey: string) => fieldValues[`${driver}:${fieldKey}`] ?? "", - [driver, fieldValues], - ); - - const setFieldValue = useCallback( - (fieldKey: string, value: string) => { - setFieldValues((existing) => ({ - ...existing, - [`${driver}:${fieldKey}`]: value, - })); + const configDraft = configByDriver[driver] ?? {}; + const setConfigDraft = useCallback( + (config: Record | undefined) => { + setConfigByDriver((existing) => { + const next = { ...existing }; + if (config === undefined || Object.keys(config).length === 0) { + delete next[driver]; + } else { + next[driver] = config; + } + return next; + }); }, [driver], ); @@ -178,13 +185,7 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns setHasAttemptedSubmit(true); if (instanceIdError !== null) return; - // Build the config blob from non-empty driver-specific field values. - // Empty strings are dropped so defaults remain in effect on the server. - const config: Record = {}; - for (const field of driverOption.fields) { - const value = (fieldValues[`${driver}:${field.key}`] ?? "").trim(); - if (value.length > 0) config[field.key] = value; - } + const config = configByDriver[driver] ?? {}; const hasConfig = Object.keys(config).length > 0; const normalizedAccentColor = normalizeProviderAccentColor(accentColor); @@ -222,7 +223,7 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns }, [ driver, driverOption, - fieldValues, + configByDriver, instanceId, instanceIdError, label, @@ -433,25 +434,15 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns - {driverOption.fields.length > 0 ? ( + {driverSettingsFields.length > 0 ? (
- {driverOption.fields.map((field) => ( - - ))} +
) : wizardStep === 2 ? (
diff --git a/apps/web/src/components/settings/ProviderInstanceCard.tsx b/apps/web/src/components/settings/ProviderInstanceCard.tsx index 21889412c30..6d492618f5b 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.tsx +++ b/apps/web/src/components/settings/ProviderInstanceCard.tsx @@ -1,7 +1,7 @@ "use client"; import { ChevronDownIcon, PlusIcon, Trash2Icon, XIcon } from "lucide-react"; -import { useEffect, useMemo, useState } from "react"; +import { useEffect, useMemo, useState, type ReactNode } from "react"; import { isProviderDriverKind, type ProviderInstanceConfig, @@ -21,6 +21,7 @@ import { DraftInput } from "../ui/draft-input"; import { Switch } from "../ui/switch"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import type { DriverOption } from "./providerDriverMeta"; +import { ProviderSettingsForm } from "./ProviderSettingsForm"; import { ProviderModelsSection } from "./ProviderModelsSection"; import { ProviderInstanceIcon } from "../chat/ProviderInstanceIcon"; import { @@ -86,20 +87,6 @@ function redactedEmailPlaceholder(email: string): string { }).join(""); } -/** - * Read a string value at `key` from the opaque per-driver config blob. - * Returns an empty string when the key is missing or the stored value is - * not a string. The permissive shape reflects that `config` is - * `Schema.Unknown` at the contract boundary — forks may populate it with - * non-string values that the built-in UI should round-trip without - * throwing. - */ -function readConfigString(config: unknown, key: string): string { - if (config === null || typeof config !== "object") return ""; - const value = (config as Record)[key]; - return typeof value === "string" ? value : ""; -} - /** * Read a string[] at `key` from the opaque config blob, filtering out * non-string entries. Used for `customModels`, which is always typed as @@ -113,35 +100,9 @@ function readConfigStringArray(config: unknown, key: string): ReadonlyArray typeof entry === "string"); } -/** - * Produce the next config blob after setting `key` to `value`. Empty - * strings drop the key so server defaults stay in effect, mirroring the - * save-time normalization in `AddProviderInstanceDialog`. Returns - * `undefined` when the resulting blob has no keys, which matches - * `ProviderInstanceConfig.config` being optional. - * - * Non-string values already stored in the blob are carried through - * verbatim so fork-owned fields survive edits made through this UI. - */ -function nextConfigBlobWithString( - config: unknown, - key: string, - value: string, -): Record | undefined { - const base: Record = - config !== null && typeof config === "object" ? { ...(config as Record) } : {}; - const trimmed = value.trim(); - if (trimmed.length > 0) { - base[key] = value; - } else { - delete base[key]; - } - return Object.keys(base).length > 0 ? base : undefined; -} - /** * Set `key` to an arbitrary value on the opaque config blob. Unlike - * `nextConfigBlobWithString`, does not drop empty-looking values — the + * provider settings field updates, does not drop empty-looking values — the * caller is responsible for deciding whether an empty array / empty * object should be stored explicitly (e.g. `customModels: []` is a * meaningful "user cleared their custom list" state distinct from @@ -473,7 +434,7 @@ interface ProviderInstanceCardProps { * default slots supply a reset-to-factory control here; custom instances * omit it. */ - readonly headerAction?: React.ReactNode | undefined; + readonly headerAction?: ReactNode | undefined; readonly hiddenModels: ReadonlyArray; readonly favoriteModels: ReadonlyArray; readonly modelOrder: ReadonlyArray; @@ -585,8 +546,7 @@ export function ProviderInstanceCard({ ); }; - const updateConfigField = (key: string, value: string) => { - const nextConfig = nextConfigBlobWithString(instance.config, key, value); + const updateConfig = (nextConfig: Record | undefined) => { const { config: _omit, ...rest } = instance; onUpdate( nextConfig !== undefined @@ -759,28 +719,15 @@ export function ProviderInstanceCard({ />
- {driverOption?.fields.map((field) => ( -
- -
- ))} + {driverOption ? ( + + ) : null} {driverOption !== undefined ? ( { + it("derives visible provider config fields from the client definition schema", () => { + const codex = DRIVER_OPTION_BY_VALUE[ProviderDriverKind.make("codex")]; + + expect(codex).toBeDefined(); + expect(deriveProviderSettingsFields(codex!).map((field) => field.key)).toEqual([ + "binaryPath", + "homePath", + "shadowHomePath", + ]); + }); + + it("preserves unknown config keys while omitting empty configurable fields", () => { + const opencode = DRIVER_OPTION_BY_VALUE[ProviderDriverKind.make("opencode")]; + expect(opencode).toBeDefined(); + + const serverUrl = deriveProviderSettingsFields(opencode!).find( + (field) => field.key === "serverUrl", + ); + expect(serverUrl).toBeDefined(); + + const next = nextProviderConfigWithFieldValue( + { forkOwned: 1, serverUrl: "http://127.0.0.1:4096" }, + serverUrl!, + "", + ); + + expect(next).toEqual({ forkOwned: 1 }); + }); + + it("reads non-string config values as blank strings", () => { + expect(readProviderConfigString({ binaryPath: 123 }, "binaryPath")).toBe(""); + }); +}); diff --git a/apps/web/src/components/settings/ProviderSettingsForm.tsx b/apps/web/src/components/settings/ProviderSettingsForm.tsx new file mode 100644 index 00000000000..46555f63aa3 --- /dev/null +++ b/apps/web/src/components/settings/ProviderSettingsForm.tsx @@ -0,0 +1,220 @@ +"use client"; + +import { useMemo, type ReactNode } from "react"; + +import { cn } from "../../lib/utils"; +import { DraftInput } from "../ui/draft-input"; +import { Input } from "../ui/input"; +import { Switch } from "../ui/switch"; +import { Textarea } from "../ui/textarea"; +import type { + ProviderClientDefinition, + ProviderSettingsControl, + ProviderSettingsFieldUi, +} from "./providerDriverMeta"; + +export interface ProviderSettingsFieldModel { + readonly key: string; + readonly control: ProviderSettingsControl; + readonly label: string; + readonly description?: string | undefined; + readonly placeholder?: string | undefined; + readonly clearWhenEmpty: "omit" | "persist"; +} + +function titleizeFieldKey(key: string): string { + return key + .replace(/([a-z0-9])([A-Z])/g, "$1 $2") + .replace(/[-_]+/g, " ") + .replace(/^./, (char) => char.toUpperCase()); +} + +function fieldControlFromUi(ui: ProviderSettingsFieldUi | undefined): ProviderSettingsControl { + return ui?.control ?? "text"; +} + +export function deriveProviderSettingsFields( + definition: ProviderClientDefinition, +): ReadonlyArray { + const schemaKeys = Object.keys(definition.settingsSchema.fields); + const schemaKeySet = new Set(schemaKeys); + const orderedKeys = [ + ...(definition.settingsUi.order ?? []).filter((key) => schemaKeySet.has(key)), + ...schemaKeys.filter((key) => !(definition.settingsUi.order ?? []).includes(key)), + ]; + + return orderedKeys.flatMap((key) => { + const ui = definition.settingsUi.fields?.[key]; + if (ui?.hidden) return []; + return [ + { + key, + control: fieldControlFromUi(ui), + label: ui?.label ?? titleizeFieldKey(key), + ...(ui?.description !== undefined ? { description: ui.description } : {}), + ...(ui?.placeholder !== undefined ? { placeholder: ui.placeholder } : {}), + clearWhenEmpty: ui?.clearWhenEmpty ?? "omit", + } satisfies ProviderSettingsFieldModel, + ]; + }); +} + +export function readProviderConfigString(config: unknown, key: string): string { + if (config === null || typeof config !== "object") return ""; + const value = (config as Record)[key]; + return typeof value === "string" ? value : ""; +} + +export function readProviderConfigBoolean(config: unknown, key: string): boolean { + if (config === null || typeof config !== "object") return false; + const value = (config as Record)[key]; + return typeof value === "boolean" ? value : false; +} + +export function nextProviderConfigWithFieldValue( + config: unknown, + field: ProviderSettingsFieldModel, + value: string | boolean, +): Record | undefined { + const base: Record = + config !== null && typeof config === "object" ? { ...(config as Record) } : {}; + + if (typeof value === "boolean") { + base[field.key] = value; + return Object.keys(base).length > 0 ? base : undefined; + } + + const trimmed = value.trim(); + if (field.clearWhenEmpty === "omit" && trimmed.length === 0) { + delete base[field.key]; + } else { + base[field.key] = value; + } + return Object.keys(base).length > 0 ? base : undefined; +} + +interface ProviderSettingsFormProps { + readonly definition: ProviderClientDefinition; + readonly value: unknown; + readonly idPrefix: string; + readonly variant: "card" | "dialog"; + readonly onChange: (nextConfig: Record | undefined) => void; +} + +function FieldFrame(props: { + readonly variant: ProviderSettingsFormProps["variant"]; + readonly children: ReactNode; +}) { + if (props.variant === "card") { + return
{props.children}
; + } + return
{props.children}
; +} + +export function ProviderSettingsForm({ + definition, + value, + idPrefix, + variant, + onChange, +}: ProviderSettingsFormProps) { + const fields = useMemo(() => deriveProviderSettingsFields(definition), [definition]); + + if (fields.length === 0) { + return null; + } + + return ( + <> + {fields.map((field) => { + const inputId = `${idPrefix}-${field.key}`; + const descriptionClassName = + variant === "card" + ? "mt-1 block text-xs text-muted-foreground" + : "text-[11px] text-muted-foreground"; + const label = {field.label}; + const description = field.description ? ( + {field.description} + ) : null; + + if (field.control === "switch") { + return ( + +
+
+ {label} + {description} +
+ + onChange(nextProviderConfigWithFieldValue(value, field, Boolean(checked))) + } + aria-label={field.label} + /> +
+ + ); + } + + if (field.control === "textarea") { + return ( + +