diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx b/apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx index 3c502c624ddd1..d1e4066daabc0 100644 --- a/apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.environment.test.tsx @@ -1,11 +1,19 @@ -import { EnvironmentId } from "@t3tools/contracts"; +import { EnvironmentId, ProviderDriverKind, type ProviderInstanceConfig } from "@t3tools/contracts"; +import { Cause } from "effect"; +import { AsyncResult } from "effect/unstable/reactivity"; import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; import { reactHookHarness as hooks } from "../../test/reactHookHarness"; +import { visitElements } from "../../test/reactElementTree"; -const settingsHooks = vi.hoisted(() => ({ +const state = vi.hoisted(() => ({ read: vi.fn(() => ({ providerInstances: {} })), - update: vi.fn(() => vi.fn()), + latest: { providerInstances: {} } as { + providerInstances: Record; + } | null, + update: vi.fn(), + toast: vi.fn(), + effect: null as (() => void | (() => void)) | null, })); vi.mock("react", async (importOriginal) => { @@ -15,40 +23,255 @@ vi.mock("react", async (importOriginal) => { ...actual, useMemo: reactHookHarness.useMemo, useState: reactHookHarness.useState, + useRef: reactHookHarness.useRef, + useEffect: (effect: () => void | (() => void)) => { + state.effect = effect; + }, }; }); - vi.mock("react/compiler-runtime", async () => { const { reactHookHarness } = await import("../../test/reactHookHarness"); return { c: reactHookHarness.useMemoCache }; }); - -vi.mock("../../hooks/useSettings", () => ({ - useEnvironmentSettings: settingsHooks.read, - useUpdateEnvironmentSettings: settingsHooks.update, +vi.mock("../../hooks/useSettings", () => ({ useEnvironmentSettings: state.read })); +vi.mock("../../state/server", () => ({ + serverEnvironment: { + updateSettings: {}, + settingsValueAtom: (environmentId: string) => environmentId, + }, })); +vi.mock("../../state/use-atom-command", () => ({ + useAtomCommand: () => state.update, + useAtomReader: () => () => state.latest, +})); +vi.mock("../ui/toast", () => ({ toastManager: { add: state.toast } })); import { AddProviderInstanceDialog } from "./AddProviderInstanceDialog"; const remoteEnvironmentId = EnvironmentId.make("remote-device"); +const close = vi.fn(); +let cleanup: (() => void) | void; +function render(environmentId = remoteEnvironmentId) { + hooks.beginRender(); + return AddProviderInstanceDialog({ + open: true, + environmentId, + environmentLabel: "Remote device", + onOpenChange: close, + }); +} +function control(tree: ReturnType, text: string) { + const element = visitElements(tree, (item) => item.props.children === text); + if (!element) throw new Error(`Missing control: ${text}`); + return element.props as { onClick: () => void; disabled?: boolean }; +} +function prepare() { + let tree = render(); + cleanup = state.effect?.(); + control(tree, "Configure manually").onClick(); + tree = render(); + const input = visitElements(tree, (item) => item.props.placeholder === "e.g. Work"); + (input!.props.onChange as (event: { target: { value: string } }) => void)({ + target: { value: "Work" }, + }); + tree = render(); + control(tree, "Next").onClick(); + tree = render(); + const config = visitElements(tree, (item) => item.props.idPrefix === "add-provider-codex"); + (config!.props.onChange as (value: Record) => void)({ + binaryPath: "/synthetic/codex", + }); + return render(); +} +function deferred() { + let resolve!: (value: A) => void; + let reject!: (reason: unknown) => void; + const promise = new Promise((yes, no) => { + resolve = yes; + reject = no; + }); + return { promise, resolve, reject }; +} +async function flush() { + await Promise.resolve(); + await Promise.resolve(); +} -describe("AddProviderInstanceDialog environment routing", () => { +describe("AddProviderInstanceDialog acknowledged save", () => { beforeEach(() => { + cleanup?.(); + cleanup = undefined; hooks.reset(); - settingsHooks.read.mockClear(); - settingsHooks.update.mockClear(); + vi.clearAllMocks(); + state.latest = { providerInstances: {} }; }); - it("reads and writes settings through the supplied environment", () => { - hooks.beginRender(); - AddProviderInstanceDialog({ - open: true, + it("waits for persistence on the selected environment and ignores duplicate clicks", async () => { + const receipt = deferred>(); + state.update.mockReturnValue(receipt.promise); + const tree = prepare(); + const add = control(tree, "Add instance"); + add.onClick(); + add.onClick(); + expect(state.update).toHaveBeenCalledTimes(1); + expect(state.update).toHaveBeenCalledWith({ environmentId: remoteEnvironmentId, - environmentLabel: "Remote device", - onOpenChange: vi.fn(), + input: { + patch: { + providerInstances: { + codex_work: { + driver: "codex", + enabled: true, + displayName: "Work", + config: { binaryPath: "/synthetic/codex", setupMode: "existing" }, + }, + }, + }, + }, }); + expect(state.read).toHaveBeenCalledWith(remoteEnvironmentId); + expect(control(render(), "Adding…").disabled).toBe(true); + expect(close).not.toHaveBeenCalled(); + expect(state.toast).not.toHaveBeenCalled(); + receipt.resolve(AsyncResult.success(null)); + await flush(); + expect(close).toHaveBeenCalledWith(false); + expect(state.toast).toHaveBeenCalledWith(expect.objectContaining({ type: "success" })); + }); - expect(settingsHooks.read).toHaveBeenCalledWith(remoteEnvironmentId); - expect(settingsHooks.update).toHaveBeenCalledWith(remoteEnvironmentId); + it("preserves providers published after the form rendered", async () => { + const tree = prepare(); + const other = { + driver: ProviderDriverKind.make("codex"), + displayName: "Other", + } satisfies ProviderInstanceConfig; + state.latest = { providerInstances: { codex_other: other } }; + state.update.mockResolvedValue(AsyncResult.success(null)); + control(tree, "Add instance").onClick(); + await flush(); + expect(state.update.mock.lastCall?.[0].input.patch.providerInstances).toMatchObject({ + codex_other: other, + codex_work: { displayName: "Work" }, + }); + expect(close).toHaveBeenCalledExactlyOnceWith(false); }); + + it.each(["unavailable", "duplicate"])( + "keeps the draft without writing when current settings are %s", + async (kind) => { + const tree = prepare(); + state.latest = + kind === "unavailable" + ? null + : { + providerInstances: { + codex_work: { driver: ProviderDriverKind.make("codex"), displayName: "Existing" }, + }, + }; + control(tree, "Add instance").onClick(); + await flush(); + expect(state.update).not.toHaveBeenCalled(); + expect(close).not.toHaveBeenCalled(); + expect(state.toast).not.toHaveBeenCalled(); + expect(visitElements(render(), (item) => item.props.value === "Work")).not.toBeNull(); + expect(visitElements(render(), (item) => item.props.role === "alert")).not.toBeNull(); + state.latest = { providerInstances: {} }; + state.update.mockResolvedValue(AsyncResult.success(null)); + control(render(), "Add instance").onClick(); + await flush(); + expect(state.update).toHaveBeenCalledTimes(1); + expect(close).toHaveBeenCalledExactlyOnceWith(false); + }, + ); + + it.each(["failure", "interruption", "rejection"])( + "retains input after %s and allows retry", + async (kind) => { + const receipt = deferred(); + state.update + .mockReturnValueOnce(receipt.promise) + .mockResolvedValue(AsyncResult.success(null)); + control(prepare(), "Add instance").onClick(); + if (kind === "rejection") receipt.reject(new Error("synthetic failure")); + else + receipt.resolve( + AsyncResult.failure( + kind === "interruption" ? Cause.interrupt(1) : Cause.fail("synthetic failure"), + ), + ); + await flush(); + const tree = render(); + expect(visitElements(tree, (item) => item.props.role === "alert")?.props.children).toContain( + "try again", + ); + expect( + visitElements(tree, (item) => item.props.placeholder === "e.g. Work")?.props.value, + ).toBe("Work"); + expect( + visitElements(tree, (item) => item.props.idPrefix === "add-provider-codex")?.props.value, + ).toEqual({ binaryPath: "/synthetic/codex" }); + expect(close).not.toHaveBeenCalled(); + expect(state.toast).not.toHaveBeenCalled(); + control(tree, "Add instance").onClick(); + await flush(); + expect(close).toHaveBeenCalledWith(false); + expect(state.update).toHaveBeenCalledTimes(2); + }, + ); + + it.each(["success", "failure", "rejection"])( + "does not release a newer environment save after old %s", + async (outcome) => { + const oldReceipt = deferred(); + const newReceipt = deferred(); + state.update.mockReturnValueOnce(oldReceipt.promise).mockReturnValueOnce(newReceipt.promise); + control(prepare(), "Add instance").onClick(); + cleanup?.(); + const newEnvironmentId = EnvironmentId.make("another-device"); + render(newEnvironmentId); + cleanup = state.effect?.(); + control(render(newEnvironmentId), "Add instance").onClick(); + if (outcome === "rejection") oldReceipt.reject(new Error("old save")); + else + oldReceipt.resolve( + outcome === "success" + ? AsyncResult.success(null) + : AsyncResult.failure(Cause.interrupt(1)), + ); + await flush(); + const tree = render(newEnvironmentId); + expect(control(tree, "Adding…").disabled).toBe(true); + expect(visitElements(tree, (item) => item.props.role === "alert")).toBeNull(); + expect(close).not.toHaveBeenCalled(); + expect(state.toast).not.toHaveBeenCalled(); + control(tree, "Adding…").onClick(); + expect(state.update).toHaveBeenCalledTimes(2); + expect(state.update.mock.lastCall?.[0].environmentId).toBe(newEnvironmentId); + newReceipt.resolve(AsyncResult.success(null)); + await flush(); + expect(close).toHaveBeenCalledExactlyOnceWith(false); + expect(state.toast).toHaveBeenCalledTimes(1); + }, + ); + + it.each(["reopen", "environment change"])( + "ignores an old completion after %s", + async (transition) => { + const receipt = deferred>(); + state.update.mockReturnValue(receipt.promise); + control(prepare(), "Add instance").onClick(); + cleanup?.(); + if (transition === "reopen") hooks.reset(); + render( + transition === "environment change" + ? EnvironmentId.make("another-device") + : remoteEnvironmentId, + ); + cleanup = state.effect?.(); + receipt.resolve(AsyncResult.success(null)); + await flush(); + expect(close).not.toHaveBeenCalled(); + expect(state.toast).not.toHaveBeenCalled(); + }, + ); }); diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx index b1cb8bf3555f4..c4403817f6870 100644 --- a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx @@ -2,7 +2,7 @@ import { Radio as RadioPrimitive } from "@base-ui/react/radio"; import { CheckIcon } from "lucide-react"; -import { useMemo, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { ProviderInstanceId, ProviderDriverKind, @@ -10,7 +10,9 @@ import { type ProviderInstanceConfig, } from "@t3tools/contracts"; -import { useEnvironmentSettings, useUpdateEnvironmentSettings } from "../../hooks/useSettings"; +import { useEnvironmentSettings } from "../../hooks/useSettings"; +import { serverEnvironment } from "../../state/server"; +import { useAtomCommand, useAtomReader } from "../../state/use-atom-command"; import { cn } from "../../lib/utils"; import { normalizeProviderAccentColor } from "../../providerInstances"; import { Button } from "../ui/button"; @@ -125,7 +127,25 @@ export function AddProviderInstanceDialog({ onOpenChange, }: AddProviderInstanceDialogProps) { const settings = useEnvironmentSettings(environmentId); - const updateSettings = useUpdateEnvironmentSettings(environmentId); + const updateSettings = useAtomCommand(serverEnvironment.updateSettings, { reportFailure: false }); + const readAtom = useAtomReader(); + const pendingSave = useRef(null); + const [isSaving, setIsSaving] = useState(false); + const [saveError, setSaveError] = useState(null); + + useEffect(() => { + setIsSaving(false); + setSaveError(null); + return () => { + // The RPC may finish after this form closes or changes environments. + pendingSave.current = null; + }; + }, [open, environmentId]); + + const changeOpen = (nextOpen: boolean) => { + if (!nextOpen) pendingSave.current = null; + onOpenChange(nextOpen); + }; const [wizardStep, setWizardStep] = useState(0); const [addingChatGptAccount, setAddingChatGptAccount] = useState(false); @@ -158,6 +178,7 @@ export function AddProviderInstanceDialog({ const configDraft = configByDriver[driver] ?? EMPTY_CONFIG_DRAFT; const setConfigDraft = (config: Record | undefined) => { + if (pendingSave.current !== null) return; setConfigByDriver((existing) => { const next = { ...existing }; if (config === undefined || Object.keys(config).length === 0) { @@ -170,6 +191,7 @@ export function AddProviderInstanceDialog({ }; const applyWizardNavigation = (navigation: WizardNavigation) => { + if (isSaving) return; if (navigation.kind === "blocked") { setHasAttemptedSubmit(true); } @@ -184,7 +206,8 @@ export function AddProviderInstanceDialog({ ); }; - const handleSave = () => { + const handleSave = async () => { + if (!open || pendingSave.current !== null) return; setHasAttemptedSubmit(true); if (instanceIdError !== null) return; @@ -207,24 +230,52 @@ export function AddProviderInstanceDialog({ // keeps the type boundary honest and guards against any future drift in // the slug rules. const brandedId = ProviderInstanceId.make(instanceId); + const latestSettings = readAtom(serverEnvironment.settingsValueAtom(environmentId)); + if (latestSettings === null) { + setSaveError("Could not confirm the save. Check the environment connection and try again."); + return; + } + const latestIdError = validateInstanceId( + instanceId, + new Set(Object.keys(latestSettings.providerInstances ?? {})), + ); + if (latestIdError !== null) { + setSaveError(latestIdError); + return; + } const nextMap = { - ...settings.providerInstances, + ...latestSettings.providerInstances, [brandedId]: nextInstance, }; + const submission = {}; + pendingSave.current = submission; + setIsSaving(true); + setSaveError(null); try { - updateSettings({ providerInstances: nextMap }); + const result = await updateSettings({ + environmentId, + input: { patch: { providerInstances: nextMap } }, + }); + if (pendingSave.current !== submission) return; + // Failure includes interruption: only an explicit success confirms the save. + if (result._tag !== "Success") { + setSaveError("Could not confirm the save. Check the environment connection and try again."); + return; + } toastManager.add({ type: "success", title: "Provider instance added", description: `${driverOption.label} instance '${instanceId}' was added.`, }); - onOpenChange(false); - } catch (error) { - toastManager.add({ - type: "error", - title: "Could not add provider instance", - description: error instanceof Error ? error.message : "Update failed.", - }); + changeOpen(false); + } catch { + if (pendingSave.current !== submission) return; + setSaveError("Could not confirm the save. Check the environment connection and try again."); + } finally { + if (pendingSave.current === submission) { + pendingSave.current = null; + setIsSaving(false); + } } }; @@ -238,7 +289,7 @@ export function AddProviderInstanceDialog({ } return ( - + -
-
- Driver -
- setDriver(ProviderDriverKind.make(value))} - aria-labelledby="add-instance-driver-label" - className="grid grid-cols-1 sm:grid-cols-2" - > - {DRIVER_OPTIONS.map((option) => { - const IconComponent = option.icon; - return ( - - - - {option.label} - - +
+
+ Driver +
+ { + if (pendingSave.current === null) setDriver(ProviderDriverKind.make(value)); + }} + aria-labelledby="add-instance-driver-label" + className="grid grid-cols-1 sm:grid-cols-2" + > + {DRIVER_OPTIONS.map((option) => { + const IconComponent = option.icon; + return ( + + + + {option.label} + + + + + {option.badgeLabel ? ( + + {option.badgeLabel} + + ) : null} + + ); + })} + {COMING_SOON_DRIVER_OPTIONS.map((option) => { + const IconComponent = option.icon; + return ( + - - - {option.badgeLabel ? ( + + + {option.label} + - {option.badgeLabel} + Coming Soon - ) : null} - - ); - })} - {COMING_SOON_DRIVER_OPTIONS.map((option) => { - const IconComponent = option.icon; - return ( - - - - {option.label} - - - Coming Soon - - - ); - })} - -
- - + + ); + })} + +
- -
- Accent color -
- + Instance ID + { + if (pendingSave.current === null) setInstanceIdOverride(event.target.value); + }} + aria-invalid={showInstanceIdError} /> -
- {PROVIDER_ACCENT_SWATCHES.map((swatch) => { - const selected = accentColor.toLowerCase() === swatch; - return ( -
+ {accentColor ? ( + + ) : null}
- {accentColor ? ( - - ) : null} + + Optional marker shown in the picker. +
- - Optional marker shown in the picker. - - - {driverSettingsFields.length > 0 ? ( -
- -
- ) : wizardStep === 2 ? ( -
-

- This driver has no required configuration. You can add the instance now. -

-
+ {driverSettingsFields.length > 0 ? ( +
+ +
+ ) : wizardStep === 2 ? ( +
+

+ This driver has no required configuration. You can add the instance now. +

+
+ ) : null} + + {saveError ? ( +

+ {saveError} +

) : null} ) : ( - + )} diff --git a/apps/web/src/components/settings/CustomModelEditor.tsx b/apps/web/src/components/settings/CustomModelEditor.tsx index 48b91748d7809..d317be3d60403 100644 --- a/apps/web/src/components/settings/CustomModelEditor.tsx +++ b/apps/web/src/components/settings/CustomModelEditor.tsx @@ -34,6 +34,7 @@ interface CustomModelEditorProps { readonly entry: CustomModelDefinition; /** Built-in models whose descriptors can be copied as a starting point. */ readonly builtInModels: ReadonlyArray; + readonly disabled?: boolean; readonly onSave: (next: CustomModelDefinition) => void; readonly onCancel: () => void; } @@ -48,10 +49,14 @@ export function CustomModelEditor({ driverKind, entry, builtInModels, + disabled = false, onSave, onCancel, }: CustomModelEditorProps) { - const [draft, setDraft] = useState(() => draftFromDefinition(entry)); + const [draft, setDraftState] = useState(() => draftFromDefinition(entry)); + const setDraft: typeof setDraftState = (next) => { + if (!disabled) setDraftState(next); + }; const [error, setError] = useState(null); const presets = useMemo( () => (driverKind ? (DESCRIPTOR_PRESETS_BY_KIND[driverKind] ?? []) : []), @@ -136,6 +141,7 @@ export function CustomModelEditor({ }; const handleSave = () => { + if (disabled) return; const problem = validateDraft(draft); if (problem) { setError(problem); @@ -150,6 +156,7 @@ export function CustomModelEditor({ const renderChoice = (descriptor: EditorDescriptor, choice: EditorChoice) => (
updateChoice(descriptor.key, choice.key, { id: event.target.value })} @@ -160,6 +167,7 @@ export function CustomModelEditor({ aria-label="Choice value" /> @@ -171,6 +179,7 @@ export function CustomModelEditor({ /> -
diff --git a/apps/web/src/components/settings/ProviderInstanceCard.test.ts b/apps/web/src/components/settings/ProviderInstanceCard.test.ts index 6bcc7fe5d8ce9..1ae0f7ecd594d 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.test.ts +++ b/apps/web/src/components/settings/ProviderInstanceCard.test.ts @@ -107,6 +107,7 @@ describe("deriveProviderModelsForDisplay", () => { liveProvider, mode: "editor", onUpdate: () => undefined, + onSaveCustomModels: async () => true, hiddenModels: [], favoriteModels: [], modelOrder: [], @@ -146,6 +147,7 @@ describe("deriveProviderModelsForDisplay", () => { driverOption: undefined, liveProvider, onUpdate: () => undefined, + onSaveCustomModels: async () => true, hiddenModels: [], favoriteModels: [], modelOrder: [], diff --git a/apps/web/src/components/settings/ProviderInstanceCard.tsx b/apps/web/src/components/settings/ProviderInstanceCard.tsx index fea12160a3cfb..d49fccb08b9a0 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.tsx +++ b/apps/web/src/components/settings/ProviderInstanceCard.tsx @@ -373,6 +373,8 @@ interface ProviderInstanceCardProps { readonly onSelect?: (() => void) | undefined; readonly readOnly?: boolean | undefined; readonly onUpdate: (nextInstance: ProviderInstanceConfig) => void; + /** Custom-model editors close only after this write is acknowledged. */ + readonly onSaveCustomModels: (nextInstance: ProviderInstanceConfig) => Promise; /** * Pass `undefined` to hide the delete footer entirely. Built-in default * instance slots use `undefined` — they can't be deleted without losing @@ -430,6 +432,7 @@ export function ProviderInstanceCard({ onSelect, readOnly = false, onUpdate, + onSaveCustomModels, onDelete, headerAction, setup, @@ -553,7 +556,7 @@ export function ProviderInstanceCard({ next.map(toCustomModelSetting), ); const { config: _omit, ...rest } = instance; - onUpdate({ ...rest, config: nextConfig } as ProviderInstanceConfig); + return onSaveCustomModels({ ...rest, config: nextConfig } as ProviderInstanceConfig); }; const updateEnvironment = (environment: ReadonlyArray) => { diff --git a/apps/web/src/components/settings/ProviderModelsSection.completion.test.tsx b/apps/web/src/components/settings/ProviderModelsSection.completion.test.tsx new file mode 100644 index 0000000000000..ce5ae3d3167ac --- /dev/null +++ b/apps/web/src/components/settings/ProviderModelsSection.completion.test.tsx @@ -0,0 +1,214 @@ +import { ProviderInstanceId, ProviderDriverKind } from "@t3tools/contracts"; +import type { CustomModelDefinition } from "@t3tools/shared/model"; +import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; +import { reactHookHarness as hooks } from "../../test/reactHookHarness"; +import { visitElements } from "../../test/reactElementTree"; + +const effects = vi.hoisted(() => ({ cleanups: [] as Array<() => void> })); +vi.mock("react", async (importOriginal) => { + const actual = await importOriginal(); + const { reactHookHarness } = await import("../../test/reactHookHarness"); + return { + ...actual, + useMemo: reactHookHarness.useMemo, + useState: reactHookHarness.useState, + useRef: reactHookHarness.useRef, + useEffect: (effect: () => void | (() => void)) => { + const cleanup = effect(); + if (cleanup) effects.cleanups.push(cleanup); + }, + }; +}); +vi.mock("react/compiler-runtime", async () => { + const { reactHookHarness } = await import("../../test/reactHookHarness"); + return { c: reactHookHarness.useMemoCache }; +}); + +import { ProviderModelsSection } from "./ProviderModelsSection"; +const change = vi.fn<(next: ReadonlyArray) => Promise>(); +const favorite = vi.fn(); +const order = vi.fn(); +const instanceId = ProviderInstanceId.make("codex_work"); +const custom = { slug: "synthetic", name: "Synthetic", capabilities: null }; +function render(existing = false) { + hooks.beginRender(); + return ProviderModelsSection({ + instanceId, + driverKind: ProviderDriverKind.make("codex"), + models: existing ? [{ ...custom, isCustom: true }] : [], + customModels: existing ? [custom] : [], + hiddenModels: [], + favoriteModels: ["synthetic", "other"], + modelOrder: ["other", "synthetic"], + onChange: change, + onHiddenModelsChange: vi.fn(), + onFavoriteModelsChange: favorite, + onModelOrderChange: order, + }); +} +function button(tree: ReturnType, text: string) { + const element = visitElements( + tree, + (item) => + item.props["aria-label"] === text || + item.props.children === text || + (Array.isArray(item.props.children) && item.props.children.includes(text)), + ); + if (!element) throw new Error(`Missing ${text}`); + return element.props.onClick as () => void; +} +function addForm() { + button(render(), "Add custom model")(); + const tree = render(); + const input = visitElements( + tree, + (item) => item.props.id === `provider-instance-${instanceId}-custom-model`, + )!; + (input.props.onChange as (event: { target: { value: string } }) => void)({ + target: { value: "synthetic" }, + }); + return render(); +} +function deferred() { + let resolve!: (value: boolean) => void; + let reject!: (reason: unknown) => void; + const promise = new Promise((yes, no) => { + resolve = yes; + reject = no; + }); + return { promise, resolve, reject }; +} +async function flush() { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); +} + +describe("custom model persistence completion", () => { + beforeEach(() => { + effects.cleanups.splice(0).forEach((cleanup) => cleanup()); + hooks.reset(); + vi.clearAllMocks(); + }); + it("retains the add draft while pending and closes only after success", async () => { + const receipt = deferred(); + change.mockReturnValue(receipt.promise); + const tree = addForm(); + const add = button(tree, "Add"); + add(); + add(); + expect(change).toHaveBeenCalledTimes(1); + expect(render().props.disabled).toBe(true); + expect(visitElements(render(), (item) => item.props.role === "status")).not.toBeNull(); + expect(visitElements(render(), (item) => item.props.value === "synthetic")).not.toBeNull(); + receipt.resolve(true); + await flush(); + expect( + visitElements( + render(), + (item) => item.props.id === `provider-instance-${instanceId}-custom-model`, + ), + ).toBeNull(); + }); + it.each(["failure", "rejection"])("keeps the add draft on %s and permits retry", async (kind) => { + const receipt = deferred(); + change.mockReturnValueOnce(receipt.promise).mockResolvedValue(true); + button(addForm(), "Add")(); + if (kind === "failure") receipt.resolve(false); + else receipt.reject(new Error("synthetic")); + await flush(); + const tree = render(); + expect(visitElements(tree, (item) => item.props.value === "synthetic")).not.toBeNull(); + expect(visitElements(tree, (item) => item.props.role === "alert")).not.toBeNull(); + button(tree, "Add")(); + await flush(); + expect(change).toHaveBeenCalledTimes(2); + expect( + visitElements( + render(), + (item) => item.props.id === `provider-instance-${instanceId}-custom-model`, + ), + ).toBeNull(); + }); + it("keeps an edit open until persistence succeeds", async () => { + const receipt = deferred(); + change.mockReturnValueOnce(receipt.promise).mockResolvedValue(true); + button(render(true), "Edit synthetic")(); + const editor = () => + visitElements(render(true), (item) => typeof item.props.onSave === "function"); + const save = editor()!.props.onSave as (next: CustomModelDefinition) => Promise; + const pending = save({ ...custom, name: "Renamed" }); + expect(editor()).not.toBeNull(); + receipt.resolve(false); + await pending; + expect(editor()).not.toBeNull(); + await (editor()!.props.onSave as typeof save)({ ...custom, name: "Renamed" }); + expect(editor()).toBeNull(); + }); + it("does not remove model preferences or close its editor after failed removal", async () => { + change.mockResolvedValue(false); + button(render(true), "Edit synthetic")(); + button(render(true), "Remove synthetic")(); + await flush(); + expect(favorite).not.toHaveBeenCalled(); + expect(order).not.toHaveBeenCalled(); + expect( + visitElements(render(true), (item) => typeof item.props.onSave === "function"), + ).not.toBeNull(); + }); + it("cleans up only the removed model after acknowledged removal", async () => { + const receipt = deferred(); + change.mockReturnValue(receipt.promise); + button(render(true), "Edit synthetic")(); + const remove = button(render(true), "Remove synthetic"); + remove(); + remove(); + expect(change).toHaveBeenCalledExactlyOnceWith([]); + expect(favorite).not.toHaveBeenCalled(); + expect(order).not.toHaveBeenCalled(); + receipt.resolve(true); + await flush(); + expect(favorite).toHaveBeenCalledExactlyOnceWith(["other"]); + expect(order).toHaveBeenCalledExactlyOnceWith(["other"]); + expect( + visitElements(render(true), (item) => typeof item.props.onSave === "function"), + ).toBeNull(); + }); + it.each(["success", "failure", "rejection"])( + "ignores old removal %s while a new card is saving", + async (outcome) => { + const oldReceipt = deferred(); + const newReceipt = deferred(); + change.mockReturnValueOnce(oldReceipt.promise).mockReturnValueOnce(newReceipt.promise); + button(render(true), "Remove synthetic")(); + effects.cleanups.splice(0).forEach((cleanup) => cleanup()); + hooks.reset(); + button(addForm(), "Add")(); + if (outcome === "rejection") oldReceipt.reject(new Error("old save")); + else oldReceipt.resolve(outcome === "success"); + await flush(); + const tree = render(); + expect(tree.props.disabled).toBe(true); + expect(visitElements(tree, (item) => item.props.value === "synthetic")).not.toBeNull(); + expect(visitElements(tree, (item) => item.props.role === "alert")).toBeNull(); + expect(favorite).not.toHaveBeenCalled(); + expect(order).not.toHaveBeenCalled(); + button(tree, "Add")(); + expect(change).toHaveBeenCalledTimes(2); + newReceipt.resolve(true); + await flush(); + expect(visitElements(render(), (item) => item.props.value === "synthetic")).toBeNull(); + }, + ); + it("ignores a completion after the provider card unmounts", async () => { + const receipt = deferred(); + change.mockReturnValue(receipt.promise); + button(addForm(), "Add")(); + effects.cleanups.splice(0).forEach((cleanup) => cleanup()); + hooks.reset(); + addForm(); + receipt.resolve(true); + await flush(); + expect(visitElements(render(), (item) => item.props.value === "synthetic")).not.toBeNull(); + }); +}); diff --git a/apps/web/src/components/settings/ProviderModelsSection.tsx b/apps/web/src/components/settings/ProviderModelsSection.tsx index f4b1bfd7439b9..26e72647c4076 100644 --- a/apps/web/src/components/settings/ProviderModelsSection.tsx +++ b/apps/web/src/components/settings/ProviderModelsSection.tsx @@ -141,7 +141,7 @@ interface ProviderModelsSectionProps { * write to the correct storage (legacy `settings.providers[kind]` vs. * `providerInstances[id].config`). */ - readonly onChange: (next: ReadonlyArray) => void; + readonly onChange: (next: ReadonlyArray) => Promise; readonly onHiddenModelsChange: (next: ReadonlyArray) => void; readonly onFavoriteModelsChange: (next: ReadonlyArray) => void; readonly onModelOrderChange: (next: ReadonlyArray) => void; @@ -171,6 +171,14 @@ export function ProviderModelsSection({ onFavoriteModelsChange, onModelOrderChange, }: ProviderModelsSectionProps) { + const pendingSave = useRef(null); + const [isSaving, setIsSaving] = useState(false); + useEffect( + () => () => { + pendingSave.current = null; + }, + [], + ); const [input, setInput] = useState(""); const [isAdding, setIsAdding] = useState(false); const [filter, setFilter] = useState(""); @@ -220,10 +228,35 @@ export function ProviderModelsSection({ if (!row) return; scrollToSlugRef.current = null; row.scrollIntoView({ block: "nearest" }); - }, [displayModels]); + }, [displayModels, isAdding]); + + const save = async (next: ReadonlyArray) => { + if (pendingSave.current !== null) return false; + const submission = {}; + pendingSave.current = submission; + setIsSaving(true); + setError(null); + try { + const saved = await onChange(next); + if (pendingSave.current !== submission) return false; + if (!saved) + setError("Could not save custom models. Check the environment connection and try again."); + return saved; + } catch { + if (pendingSave.current === submission) { + setError("Could not save custom models. Check the environment connection and try again."); + } + return false; + } finally { + if (pendingSave.current === submission) { + pendingSave.current = null; + setIsSaving(false); + } + } + }; - const handleAdd = () => { - if (driverKind === "antigravity") return; + const handleAdd = async () => { + if (driverKind === "antigravity" || pendingSave.current !== null) return; const normalized = normalizeCustomModelSlug(input); if (!normalized) { setError("Enter a model slug."); @@ -242,32 +275,37 @@ export function ProviderModelsSection({ return; } + if ( + !(await save([...customModels, { slug: normalized, name: normalized, capabilities: null }])) + ) + return; // Clear the filter so the new row renders even when it does not match, // which is also what lets the pending scroll target resolve and clear. scrollToSlugRef.current = normalized; setFilter(""); - onChange([...customModels, { slug: normalized, name: normalized, capabilities: null }]); setInput(""); setError(null); setIsAdding(false); }; const cancelAdd = () => { + if (pendingSave.current !== null) return; setInput(""); setError(null); setIsAdding(false); }; - const handleRemove = (slug: string) => { + const handleRemove = async (slug: string) => { + if (!(await save(customModels.filter((entry) => entry.slug !== slug)))) return; if (editingSlug === slug) setEditingSlug(null); - onChange(customModels.filter((entry) => entry.slug !== slug)); onModelOrderChange(modelOrder.filter((model) => model !== slug)); onFavoriteModelsChange(favoriteModels.filter((model) => model !== slug)); setError(null); }; - const handleSaveEdit = (next: CustomModelDefinition) => { - onChange(customModels.map((entry) => (entry.slug === next.slug ? next : entry))); + const handleSaveEdit = async (next: CustomModelDefinition) => { + if (!(await save(customModels.map((entry) => (entry.slug === next.slug ? next : entry))))) + return; setEditingSlug(null); }; @@ -500,7 +538,10 @@ export function ProviderModelsSection({ ); return ( -
+
{showFilter ? ( setEditingSlug(null)} + onCancel={() => { + if (pendingSave.current === null) setEditingSlug(null); + }} /> ) : null}
@@ -600,6 +644,7 @@ export function ProviderModelsSection({ autoFocus value={input} onChange={(event) => { + if (pendingSave.current !== null) return; setInput(event.target.value); if (error) setError(null); }} @@ -627,9 +672,16 @@ export function ProviderModelsSection({
) : null} + {isSaving ? ( +

+ Saving… +

+ ) : null} {driverKind !== "antigravity" && error ? ( -

{error}

+

+ {error} +

) : null} - + ); } diff --git a/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx b/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx index 2d5d5d7517190..1f25bf60f8d9d 100644 --- a/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx +++ b/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx @@ -1,3 +1,5 @@ +import { AsyncResult } from "effect/unstable/reactivity"; +import { Cause } from "effect"; import type { ReactElement } from "react"; import { DEFAULT_UNIFIED_SETTINGS, @@ -15,13 +17,16 @@ import { reactHookHarness as hooks } from "../../test/reactHookHarness"; const atoms = vi.hoisted(() => ({ providers: null as ReadonlyArray | null, providersAtom: Symbol("providers"), + settingsAtom: Symbol("settings"), refreshProviders: Symbol("refreshProviders"), updateProvider: Symbol("updateProvider"), + updateSettings: Symbol("updateSettings"), })); const commands = vi.hoisted(() => ({ refresh: vi.fn(), updateProvider: vi.fn(), + saveSettings: vi.fn(), })); const settingsState = vi.hoisted(() => ({ @@ -72,14 +77,21 @@ vi.mock("../../state/server", () => ({ EMPTY_SERVER_PROVIDERS: [], serverEnvironment: { providersValueAtom: () => atoms.providersAtom, + settingsValueAtom: () => atoms.settingsAtom, refreshProviders: atoms.refreshProviders, updateProvider: atoms.updateProvider, + updateSettings: atoms.updateSettings, }, })); vi.mock("../../state/use-atom-command", () => ({ useAtomCommand: (atom: symbol) => - atom === atoms.refreshProviders ? commands.refresh : commands.updateProvider, + atom === atoms.refreshProviders + ? commands.refresh + : atom === atoms.updateSettings + ? commands.saveSettings + : commands.updateProvider, + useAtomReader: () => () => settingsState.value, })); vi.mock("../../hooks/useSettings", () => ({ @@ -180,6 +192,7 @@ describe("EnvironmentProviderSettings routing", () => { settingsState.updateClientSettings.mockReset(); settingsSearchState.targetId = null; settingsSearchState.effects = []; + commands.saveSettings.mockReset(); commands.refresh.mockReset().mockResolvedValue({ _tag: "Success" }); commands.updateProvider.mockReset().mockResolvedValue({ _tag: "Success" }); }); @@ -389,4 +402,43 @@ describe("EnvironmentProviderSettings routing", () => { expect(resetPatch).not.toHaveProperty("favorites"); expect(resetPatch).not.toHaveProperty("providerModelPreferences"); }); + it("acknowledges custom model writes only on the selected environment's success", async () => { + settingsState.value = { + ...DEFAULT_UNIFIED_SETTINGS, + providerInstances: { [codexId]: { driver: ProviderDriverKind.make("codex"), enabled: true } }, + }; + const panel = renderPanel(); + const card = visitElements( + panel, + (element) => element.props.instanceId === codexId && element.props.mode === "editor", + ); + const save = card!.props.onSaveCustomModels as ( + next: Record, + ) => Promise; + const next = { + driver: ProviderDriverKind.make("codex"), + config: { customModels: ["synthetic-model"] }, + }; + commands.saveSettings.mockResolvedValueOnce(AsyncResult.failure(Cause.interrupt(1))); + expect(await save(next)).toBe(false); + commands.saveSettings.mockResolvedValueOnce(AsyncResult.success(null)); + expect(await save(next)).toBe(true); + expect(commands.saveSettings).toHaveBeenLastCalledWith( + expect.objectContaining({ + environmentId, + input: { + patch: expect.objectContaining({ + providerInstances: expect.objectContaining({ [codexId]: next }), + }), + }, + }), + ); + + // Without loaded settings the replacement map would be built from defaults and erase + // the environment's other instances, so the save must fail without writing. + settingsState.value = null; + commands.saveSettings.mockClear(); + expect(await save(next)).toBe(false); + expect(commands.saveSettings).not.toHaveBeenCalled(); + }); }); diff --git a/apps/web/src/components/settings/ProviderSettingsPanel.tsx b/apps/web/src/components/settings/ProviderSettingsPanel.tsx index aa947d5d1de72..37e65d6a17705 100644 --- a/apps/web/src/components/settings/ProviderSettingsPanel.tsx +++ b/apps/web/src/components/settings/ProviderSettingsPanel.tsx @@ -47,7 +47,7 @@ import { } from "../../state/environments"; import { EMPTY_SERVER_PROVIDERS, serverEnvironment } from "../../state/server"; import { useEnvironmentSessionState } from "../../state/session"; -import { useAtomCommand } from "../../state/use-atom-command"; +import { useAtomCommand, useAtomReader } from "../../state/use-atom-command"; import { getRelativeTimeState } from "../../timestampFormat"; import { ConnectionStatusDot, @@ -596,6 +596,10 @@ export function EnvironmentProviderSettings({ // Provider instances hold per-machine credentials and binaries, so this // page always edits exactly the environment it displays. const updateSettings = useUpdateEnvironmentSettings(environmentId); + const saveProviderSettings = useAtomCommand(serverEnvironment.updateSettings, { + reportFailure: false, + }); + const readAtom = useAtomReader(); const updateClientSettings = useUpdateClientSettings(); const serverProviders = useAtomValue(serverEnvironment.providersValueAtom(environmentId)) ?? EMPTY_SERVER_PROVIDERS; @@ -930,7 +934,7 @@ export function EnvironmentProviderSettings({ return ( ) : null } + onSaveCustomModels={async (next) => { + // Read settings at dispatch time so the replacement map includes every provider + // write the environment has already confirmed. Without loaded settings, a map built + // from defaults would erase this environment's other instances, so fail the save. + const latestSettings = readAtom(serverEnvironment.settingsValueAtom(environmentId)); + if (latestSettings === null) return false; + const result = await saveProviderSettings({ + environmentId, + input: { + patch: buildProviderInstanceUpdatePatch({ + settings: latestSettings, + instanceId: row.instanceId, + instance: next, + driver: row.driver, + isDefault: row.isDefault, + }), + }, + }); + return result._tag === "Success"; + }} onUpdate={(next) => { const wasEnabled = resolveProviderInstanceEnabled(row.instance); const isDisabling = next.enabled === false && wasEnabled; diff --git a/apps/web/src/state/use-atom-command.ts b/apps/web/src/state/use-atom-command.ts index 37ce280e9f41b..433be72cd4a76 100644 --- a/apps/web/src/state/use-atom-command.ts +++ b/apps/web/src/state/use-atom-command.ts @@ -5,8 +5,18 @@ import { type AtomCommandResult, runAtomCommand, } from "@t3tools/client-runtime/state/runtime"; +import type { Atom } from "effect/unstable/reactivity"; import { useCallback, useContext } from "react"; +/** + * Read the latest published atom value at call time rather than at render. + * This does not include writes that are still queued or awaiting publication. + */ +export function useAtomReader(): (atom: Atom.Atom) => A { + const registry = useContext(RegistryContext); + return useCallback((atom) => registry.get(atom), [registry]); +} + export function useAtomCommand( command: AtomCommand, options?: string | AtomCommandOptions, diff --git a/docs/internals/consistency-settings-completion.md b/docs/internals/consistency-settings-completion.md new file mode 100644 index 0000000000000..42da9b8bfb3cd --- /dev/null +++ b/docs/internals/consistency-settings-completion.md @@ -0,0 +1,38 @@ +# Settings completion follows persistence + +Proposed contract, not a ratified maintainer decision: a settings form that says +“Added” or “Saved”, or closes to signal a successful save, must first receive +confirmation from the store that owns the setting. Accepting input or scheduling +an update is not that confirmation. + +[Apple's Feedback guidance](https://developer.apple.com/design/human-interface-guidelines/feedback) +and [Loading guidance](https://developer.apple.com/design/human-interface-guidelines/loading) +support keeping people informed while work proceeds. Here, that means a pending +save prevents duplicate submission; a failed or interrupted save keeps the draft +and provides a retry path. Only confirmed success may clear the form or announce +completion. + +This boundary spans the form, client command, and authoritative persistence. +A promise, including `Promise`, confirms persistence only if it resolves +after the write succeeds and rejects on failure. Scheduling a write or swallowing +its error does not provide confirmation. Server-owned +settings require the command's successful receipt for the environment selected at +submission. Closing, reopening, or changing environments must detach the old +completion from the new form; it does not cancel or undo a server write. + +The rule applies to explicit settings completion claims in web, Electron, and +React Native where those forms exist. The additional-provider form and custom-model editors are shared by +web and Electron. Custom-model add and edit retain their drafts until the +environment confirms persistence; removal cleans up local preferences only +after that confirmation. Quiet preference controls may update optimistically only +if their callers observe write failures, show an error and reconcile the displayed +value with persisted state. Otherwise they must await persistence. A helper that +logs a failed write and resolves does not qualify for this exception. This contract does +not require modal saves for toggles, change shared-setting synchronization, or +extend to clipboard acknowledgements. + +Observable acceptance cases are delayed success with pending feedback and one +submission; rejection or interruption with retained input and an actionable +error; successful retry; and an old receipt arriving after a form or environment +change without dismissing the current form or announcing its success. A server +receipt confirms persistence, not provider authentication or readiness.