shishantbiswas/coder
1
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-06-02 20:48:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(site): address provider delete review feedback

Browse Source
This commit is contained in:
Tracy Johnson
2026-05-29 17:21:16 +00:00
parent 742aef0354
commit d3c679253c
10 changed files with 313 additions and 158 deletions
Download Patch File Download Diff File Expand all files Collapse all files
site/src/pages/AISettingsPage/ProvidersPage/UpdateProviderPage/UpdateProviderPageView.stories.tsx
+12 -6
View File
@@ -152,6 +152,9 @@ export const DeleteDialogWithAssociatedModels: Story = {
await screen.findByText(/Deleting this provider will also disable/i),
).toBeInTheDocument();
await expect(screen.getByText("2 models")).toBeInTheDocument();
await expect(
screen.getByText("No other model exists to become the default."),
).toBeInTheDocument();
},
};
@@ -189,19 +192,19 @@ export const DeleteDialogCascadeConfirmed: Story = {
});
expect(API.experimental.updateChatModelConfig).toHaveBeenNthCalledWith(
1,
"model-anthropic-fallback",
{ is_default: true },
);
expect(API.experimental.updateChatModelConfig).toHaveBeenNthCalledWith(
2,
"model-openai-default",
{ enabled: false },
);
expect(API.experimental.updateChatModelConfig).toHaveBeenNthCalledWith(
2,
3,
"model-openai-secondary",
{ enabled: false },
);
expect(API.experimental.updateChatModelConfig).toHaveBeenNthCalledWith(
3,
"model-anthropic-fallback",
{ is_default: true },
);
await waitFor(() => {
expect(API.deleteAIProvider).toHaveBeenCalledWith(
MockAIProviderOpenAI.name,
@@ -245,5 +248,8 @@ export const DeleteDialogCascadeFailure: Story = {
expect(API.deleteAIProvider).not.toHaveBeenCalled();
await expect(await screen.findByRole("dialog")).toBeInTheDocument();
await expect(screen.getByRole("button", { name: "Cancel" })).toBeEnabled();
await expect(
await screen.findByText("Failed to disable model."),
).toBeInTheDocument();
},
};
site/src/pages/AISettingsPage/ProvidersPage/UpdateProviderPage/UpdateProviderPageView.tsx
+98 -114
View File
@@ -18,19 +18,11 @@ import {
import { Avatar } from "#/components/Avatar/Avatar";
import { Badge } from "#/components/Badge/Badge";
import { Button } from "#/components/Button/Button";
import {
Dialog,
DialogContent,
DialogDescription,
DialogFooter,
DialogHeader,
DialogTitle,
} from "#/components/Dialog/Dialog";
import { Input } from "#/components/Input/Input";
import { Loader } from "#/components/Loader/Loader";
import { SettingsHeaderTitle } from "#/components/SettingsHeader/SettingsHeader";
import { Spinner } from "#/components/Spinner/Spinner";
import { Switch } from "#/components/Switch/Switch";
import { ConfirmDeleteDialog } from "#/pages/AgentsPage/components/ConfirmDeleteDialog";
import { cascadeDisableProviderModels } from "#/pages/AISettingsPage/utils/providerDelete";
import { pageTitle } from "#/utils/page";
import { ProviderForm } from "../components/ProviderForm";
@@ -80,6 +72,17 @@ const UpdateProviderPageView: React.FC = () => {
(mc) => mc.ai_provider_id === provider?.id,
);
const associatedModelCount = associatedModels.length;
const associatedModelIds = new Set(associatedModels.map((model) => model.id));
const willReassignDefault = associatedModels.some(
(model) => model.is_default,
);
const hasDefaultReplacement = willReassignDefault
? (modelConfigsQuery.data ?? []).some(
(model) => model.enabled && !associatedModelIds.has(model.id),
)
: false;
const modelConfigsUnavailable =
modelConfigsQuery.isLoading || modelConfigsQuery.isError;
// Rendered into every non-redirect return so the document title reflects
// the provider as soon as we know it; falls back to a placeholder while
@@ -237,23 +240,11 @@ const UpdateProviderPageView: React.FC = () => {
}}
/>
</div>
<Dialog
open={deleteDialogOpen}
onOpenChange={(open) => {
if (!open && !isCascadeDeleting && !deleteMutation.isPending) {
setDeleteDialogOpen(false);
setDeleteConfirmText("");
}
}}
>
<DialogContent variant="destructive">
<DialogHeader>
<DialogTitle>Delete provider</DialogTitle>
<DialogDescription>
Deleting this provider is irreversible!
</DialogDescription>
</DialogHeader>
<div className="flex flex-col gap-3 text-sm text-content-secondary">
<ConfirmDeleteDialog
entity="provider"
description={
<div className="flex flex-col gap-3">
<p className="m-0">Deleting this provider is irreversible!</p>
{associatedModelCount > 0 && (
<ul className="m-0 pl-5">
<li>
@@ -264,97 +255,90 @@ const UpdateProviderPageView: React.FC = () => {
</strong>{" "}
from your settings.
</li>
{willReassignDefault && (
<li>
{hasDefaultReplacement
? "The default model will be reassigned."
: "No other model exists to become the default."}
</li>
)}
</ul>
)}
<p className="m-0">
Type{" "}
<strong className="text-content-primary">
{provider.name}
</strong>{" "}
to confirm.
</p>
<Input
id="delete-confirm"
aria-label={`Type ${provider.name} to confirm`}
autoFocus
autoComplete="off"
placeholder={provider.name}
value={deleteConfirmText}
onChange={(e) => setDeleteConfirmText(e.target.value)}
/>
</div>
<DialogFooter>
<Button
variant="outline"
onClick={() => {
setDeleteDialogOpen(false);
setDeleteConfirmText("");
}}
disabled={isCascadeDeleting || deleteMutation.isPending}
>
Cancel
</Button>
<Button
variant="destructive"
className="disabled:border-border"
disabled={
deleteConfirmText !== provider.name ||
isCascadeDeleting ||
deleteMutation.isPending ||
modelConfigsQuery.isLoading ||
modelConfigsQuery.isError
}
onClick={() => {
const deleteAll = async () => {
setIsCascadeDeleting(true);
try {
await cascadeDisableProviderModels({
associatedModels,
allModels: modelConfigsQuery.data ?? [],
updateModelConfig:
API.experimental.updateChatModelConfig,
});
await invalidateChatConfigurationQueries(queryClient);
deleteMutation.mutate(undefined, {
onSuccess: () => {
toast.success(
`Provider "${provider.display_name || provider.name}" deleted.`,
);
setDeleteDialogOpen(false);
setDeleteConfirmText("");
void navigate(BACK_HREF, { replace: true });
},
onError: (error) => {
toast.error(
getErrorMessage(
error,
`Failed to delete provider "${provider.display_name || provider.name}".`,
),
);
},
onSettled: () => {
setIsCascadeDeleting(false);
},
});
} catch (error) {
toast.error(
getErrorMessage(error, "Failed to delete provider."),
);
setIsCascadeDeleting(false);
await invalidateChatConfigurationQueries(queryClient);
}
};
void deleteAll();
}}
>
{(isCascadeDeleting || deleteMutation.isPending) && (
<Spinner className="h-4 w-4" loading />
)}
Delete provider
</Button>
</DialogFooter>
</DialogContent>
</Dialog>
}
confirmDisabled={
deleteConfirmText !== provider.name || modelConfigsUnavailable
}
isPending={isCascadeDeleting || deleteMutation.isPending}
open={deleteDialogOpen}
onOpenChange={(open) => {
if (!open && !isCascadeDeleting && !deleteMutation.isPending) {
setDeleteDialogOpen(false);
setDeleteConfirmText("");
}
}}
onConfirm={() => {
const deleteProvider = async () => {
setIsCascadeDeleting(true);
try {
await cascadeDisableProviderModels({
associatedModels,
allModels: modelConfigsQuery.data ?? [],
updateModelConfig: API.experimental.updateChatModelConfig,
});
await invalidateChatConfigurationQueries(queryClient);
deleteMutation.mutate(undefined, {
onSuccess: () => {
toast.success(
`Provider "${provider.display_name || provider.name}" deleted.`,
);
setDeleteDialogOpen(false);
setDeleteConfirmText("");
void navigate(BACK_HREF, { replace: true });
},
onError: (error) => {
toast.error(
getErrorMessage(
error,
`Failed to delete provider "${provider.display_name || provider.name}".`,
),
);
},
onSettled: () => {
setIsCascadeDeleting(false);
},
});
} catch (error) {
toast.error(
getErrorMessage(
error,
"Failed to disable models for provider deletion.",
),
);
setIsCascadeDeleting(false);
await invalidateChatConfigurationQueries(queryClient);
}
};
void deleteProvider();
}}
>
<div className="flex flex-col gap-3 text-sm text-content-secondary">
<p className="m-0">
Type{" "}
<strong className="text-content-primary">{provider.name}</strong>{" "}
to confirm.
</p>
<Input
id="delete-confirm"
aria-label={`Type ${provider.name} to confirm`}
autoFocus
autoComplete="off"
placeholder={provider.name}
value={deleteConfirmText}
onChange={(e) => setDeleteConfirmText(e.target.value)}
/>
</div>
</ConfirmDeleteDialog>
</div>
</>
);
site/src/pages/AISettingsPage/utils/providerDelete.test.ts
+49 -7
View File
@@ -21,7 +21,7 @@ const model = (
});
describe("cascadeDisableProviderModels", () => {
it("disables associated models before reassigning the default", async () => {
it("reassigns the default before disabling associated models", async () => {
const associatedModels = [
model({
id: "model-associated-default",
@@ -51,19 +51,19 @@ describe("cascadeDisableProviderModels", () => {
updateModelConfig,
});
expect(updateModelConfig).toHaveBeenNthCalledWith(1, "model-next-default", {
is_default: true,
});
expect(updateModelConfig).toHaveBeenNthCalledWith(
1,
2,
"model-associated-default",
{ enabled: false },
);
expect(updateModelConfig).toHaveBeenNthCalledWith(
2,
3,
"model-associated-secondary",
{ enabled: false },
);
expect(updateModelConfig).toHaveBeenNthCalledWith(3, "model-next-default", {
is_default: true,
});
});
it("does not reassign the default when the deleted provider had no default", async () => {
@@ -99,11 +99,12 @@ describe("cascadeDisableProviderModels", () => {
});
});
it("stops before reassigning the default when a model disable fails", async () => {
it("rejects when a model disable fails after reassigning the default", async () => {
const error = new Error("failed to disable model");
const updateModelConfig = vi
.fn()
.mockResolvedValueOnce(undefined)
.mockResolvedValueOnce(undefined)
.mockRejectedValueOnce(error);
await expect(
@@ -132,6 +133,47 @@ describe("cascadeDisableProviderModels", () => {
}),
).rejects.toThrow(error);
expect(updateModelConfig).toHaveBeenCalledTimes(3);
expect(updateModelConfig).toHaveBeenNthCalledWith(1, "model-next-default", {
is_default: true,
});
});
it("does not reassign the default when no eligible fallback exists", async () => {
const associatedModels = [
model({
id: "model-associated-default",
provider: "openai",
model: "gpt-4o",
is_default: true,
}),
model({
id: "model-associated-secondary",
provider: "openai",
model: "gpt-4o-mini",
}),
];
const updateModelConfig = vi.fn(async () => undefined);
await cascadeDisableProviderModels({
associatedModels,
allModels: associatedModels,
updateModelConfig,
});
expect(updateModelConfig).toHaveBeenCalledTimes(2);
expect(updateModelConfig).toHaveBeenNthCalledWith(
1,
"model-associated-default",
{ enabled: false },
);
expect(updateModelConfig).toHaveBeenNthCalledWith(
2,
"model-associated-secondary",
{ enabled: false },
);
expect(updateModelConfig).not.toHaveBeenCalledWith(expect.any(String), {
is_default: true,
});
});
});
site/src/pages/AISettingsPage/utils/providerDelete.ts
+9 -11
View File
@@ -17,18 +17,16 @@ export const cascadeDisableProviderModels = async ({
const disabledIds = new Set(associatedModels.map((model) => model.id));
const hadDefault = associatedModels.some((model) => model.is_default);
if (hadDefault) {
const newDefault = allModels.find(
(model) => model.enabled && !disabledIds.has(model.id),
);
if (newDefault) {
await updateModelConfig(newDefault.id, { is_default: true });
}
}
for (const model of associatedModels) {
await updateModelConfig(model.id, { enabled: false });
}
if (!hadDefault) {
return;
}
const newDefault = allModels.find(
(model) => model.enabled && !disabledIds.has(model.id),
);
if (newDefault) {
await updateModelConfig(newDefault.id, { is_default: true });
}
};
site/src/pages/AgentsPage/components/ChatModelAdminPanel/ChatModelAdminPanel.stories.tsx
+78 -8
View File
@@ -2252,6 +2252,46 @@ export const ModelDeleteConfirmed: Story = {
},
};
export const DeletedProviderModelWarning: Story = {
args: {
section: "models" as ChatModelAdminSection,
providerConfigsData: [
createProviderConfig({
id: "provider-anthropic",
provider: "anthropic",
display_name: "Anthropic",
source: "database",
has_api_key: true,
allow_user_api_key: true,
}),
],
modelConfigsData: [
createModelConfig({
id: "model-orphaned",
provider: "openai",
ai_provider_id: "provider-deleted-openai",
model: "gpt-4o",
display_name: "Orphaned GPT-4o",
enabled: true,
}),
],
},
play: async ({ canvasElement, args }) => {
const body = within(canvasElement.ownerDocument.body);
await userEvent.click(await body.findByText("Orphaned GPT-4o"));
await expect(
await body.findByText(/This model's provider was deleted/i),
).toBeInTheDocument();
await expect(
body.getByText(/Create a replacement model with an active provider/i),
).toBeInTheDocument();
await expect(body.getByRole("switch", { name: "Enabled" })).toBeDisabled();
await expect(body.getByRole("button", { name: "Save" })).toBeDisabled();
expect(args.onUpdateModel).not.toHaveBeenCalled();
},
};
export const ProviderDeleteConfirmation: Story = {
args: {
section: "providers" as ChatModelAdminSection,
@@ -2298,6 +2338,9 @@ export const ProviderDeleteConfirmation: Story = {
await body.findByText(/Deleting this provider will also disable/i),
).toBeInTheDocument();
expect(body.getByText(/2 models/i)).toBeInTheDocument();
expect(
body.getByText("No other model exists to become the default."),
).toBeInTheDocument();
await expect(body.getByRole("dialog")).toBeInTheDocument();
await expect(
body.getByRole("button", { name: "Delete provider" }),
@@ -2308,6 +2351,35 @@ export const ProviderDeleteConfirmation: Story = {
},
};
export const ProviderDeleteWaitingForModels: Story = {
args: {
section: "providers" as ChatModelAdminSection,
providerConfigsData: [
createProviderConfig({
id: "provider-openai",
provider: "openai",
display_name: "OpenAI",
source: "database",
has_api_key: true,
}),
],
modelConfigsData: undefined,
isLoading: true,
},
play: async ({ canvasElement, args }) => {
const body = within(canvasElement.ownerDocument.body);
await userEvent.click(await body.findByRole("button", { name: /OpenAI/i }));
await userEvent.click(await body.findByRole("button", { name: "Delete" }));
await expect(
await body.findByRole("button", { name: "Delete provider" }),
).toBeDisabled();
expect(args.onDeleteProvider).not.toHaveBeenCalled();
expect(args.onUpdateModel).not.toHaveBeenCalled();
},
};
export const ProviderDeleteCancelled: Story = {
args: {
section: "providers" as ChatModelAdminSection,
@@ -2327,7 +2399,7 @@ export const ProviderDeleteCancelled: Story = {
// Navigate to provider detail, open delete dialog, then cancel.
await userEvent.click(await body.findByRole("button", { name: /OpenAI/i }));
await userEvent.click(await body.findByRole("button", { name: "Delete" }));
await body.findByText(/Are you sure/i);
await body.findByText(/Deleting this provider is irreversible/i);
await userEvent.click(body.getByRole("button", { name: "Cancel" }));
// The dialog should be closed and the form footer restored.
@@ -2402,20 +2474,18 @@ export const ProviderDeleteConfirmed: Story = {
});
expect(args.onUpdateModel).toHaveBeenNthCalledWith(
1,
"model-openai-default",
{
enabled: false,
},
"model-anthropic-fallback",
{ is_default: true },
);
expect(args.onUpdateModel).toHaveBeenNthCalledWith(
2,
"model-openai-secondary",
"model-openai-default",
{ enabled: false },
);
expect(args.onUpdateModel).toHaveBeenNthCalledWith(
3,
"model-anthropic-fallback",
{ is_default: true },
"model-openai-secondary",
{ enabled: false },
);
await waitFor(() => {
expect(args.onDeleteProvider).toHaveBeenCalledTimes(1);
site/src/pages/AgentsPage/components/ChatModelAdminPanel/ChatModelAdminPanel.tsx
+3
View File
@@ -303,6 +303,8 @@ export const ChatModelAdminPanel: FC<ChatModelAdminPanelProps> = ({
const providerConfigsUnavailable = providerConfigsData === null;
const modelConfigsUnavailable = modelConfigsData === null;
const modelConfigsReady =
modelConfigsData !== undefined && modelConfigsData !== null;
return (
<div className={cn("flex min-h-full flex-col", className)}>
@@ -320,6 +322,7 @@ export const ChatModelAdminPanel: FC<ChatModelAdminPanelProps> = ({
sectionDescription={sectionDescription}
providerStates={providerStates}
providerConfigsUnavailable={providerConfigsUnavailable}
modelConfigsReady={modelConfigsReady}
isProviderMutationPending={isProviderMutationPending}
onCreateProvider={onCreateProvider}
onUpdateProvider={onUpdateProvider}
site/src/pages/AgentsPage/components/ChatModelAdminPanel/ModelForm.tsx
+6 -7
View File
@@ -278,6 +278,7 @@ export const ModelForm: FC<ModelFormProps> = ({
>
<SelectTrigger
id="providerSelect"
aria-label="Provider"
className="h-10 max-w-[240px] text-[13px]"
>
<SelectValue placeholder="Select provider" />
@@ -354,8 +355,8 @@ export const ModelForm: FC<ModelFormProps> = ({
{isProviderDeleted && (
<Alert severity="warning" className="mb-4">
<AlertDescription>
This model's provider was deleted. Remove and re-add this model to
reconfigure it.
This model's provider was deleted. Create a replacement model with
an active provider to continue using it.
</AlertDescription>
</Alert>
)}
@@ -397,9 +398,7 @@ export const ModelForm: FC<ModelFormProps> = ({
}}
aria-label="Enabled"
disabled={
isSaving ||
defaultModelDisableGuard ||
(isProviderDeleted && !form.values.enabled)
isSaving || defaultModelDisableGuard || isProviderDeleted
}
/>
</span>
@@ -407,8 +406,8 @@ export const ModelForm: FC<ModelFormProps> = ({
<TooltipContent side="bottom">
{defaultModelDisableGuard
? "Default model cannot be disabled. Remove default status first."
: isProviderDeleted && !form.values.enabled
? "This model's provider was deleted."
: isProviderDeleted
? "This model's provider was deleted. Create a replacement model first."
: form.values.enabled
? "Disable this model. It will be hidden from users."
: "Enable this model. It will be visible to users."}
site/src/pages/AgentsPage/components/ChatModelAdminPanel/ProviderForm.tsx
+48 -4
View File
@@ -41,6 +41,7 @@ const API_KEY_PLACEHOLDER = "••••••••••••••••";
interface ProviderFormProps {
providerState: ProviderState;
providerConfigsUnavailable: boolean;
modelConfigsReady: boolean;
isProviderMutationPending: boolean;
onCreateProvider: (
req: TypesGen.CreateChatProviderConfigRequest,
@@ -61,6 +62,7 @@ interface ProviderFormProps {
export const ProviderForm: FC<ProviderFormProps> = ({
providerState,
providerConfigsUnavailable,
modelConfigsReady,
isProviderMutationPending,
onCreateProvider,
onUpdateProvider,
@@ -121,10 +123,38 @@ export const ProviderForm: FC<ProviderFormProps> = ({
: "Endpoint used to call this provider.";
const apiKeyPlaceholder = isBedrockProvider ? "Enter bearer token" : "sk-...";
const associatedModelCount = providerState.modelConfigs.length;
const deleteProviderDescription =
associatedModelCount > 0
? `Deleting this provider will also disable ${associatedModelCount} ${associatedModelCount === 1 ? "model" : "models"} from your settings.`
: "Are you sure you want to delete this provider? This action is irreversible.";
const associatedModelIds = new Set(
providerState.modelConfigs.map((model) => model.id),
);
const willReassignDefault = providerState.modelConfigs.some(
(model) => model.is_default,
);
const hasDefaultReplacement = willReassignDefault
? allModelConfigs.some(
(model) => model.enabled && !associatedModelIds.has(model.id),
)
: false;
const deleteProviderDescription = (
<div className="flex flex-col gap-3">
<p className="m-0">Deleting this provider is irreversible!</p>
{associatedModelCount > 0 && (
<ul className="m-0 pl-5">
<li>
Deleting this provider will also disable {associatedModelCount}{" "}
{associatedModelCount === 1 ? "model" : "models"} from your
settings.
</li>
{willReassignDefault && (
<li>
{hasDefaultReplacement
? "The default model will be reassigned."
: "No other model exists to become the default."}
</li>
)}
</ul>
)}
</div>
);
const hasNewProviderConfiguration = !providerConfig;
const isDirty =
@@ -395,6 +425,19 @@ export const ProviderForm: FC<ProviderFormProps> = ({
allModels: allModelConfigs,
updateModelConfig: onUpdateModel,
});
} catch (error) {
toast.error(
getErrorMessage(
error,
"Failed to disable models for provider deletion.",
),
);
setIsCascadeDeleting(false);
await invalidateChatConfigurationQueries(queryClient);
return;
}
try {
await onDeleteProvider(providerConfig.id);
setIsCascadeDeleting(false);
} catch (error) {
@@ -407,6 +450,7 @@ export const ProviderForm: FC<ProviderFormProps> = ({
};
void deleteProvider();
}}
confirmDisabled={!modelConfigsReady}
isPending={isCascadeDeleting || isProviderMutationPending}
open={confirmingDelete}
onOpenChange={(open) => {
site/src/pages/AgentsPage/components/ChatModelAdminPanel/ProvidersSection.tsx
+3
View File
@@ -72,6 +72,7 @@ interface ProvidersSectionProps {
sectionDescription?: string;
providerStates: readonly ProviderState[];
providerConfigsUnavailable: boolean;
modelConfigsReady: boolean;
isProviderMutationPending: boolean;
onCreateProvider: (
req: CreateChatProviderConfigRequest,
@@ -93,6 +94,7 @@ export const ProvidersSection: FC<ProvidersSectionProps> = ({
sectionDescription,
providerStates,
providerConfigsUnavailable,
modelConfigsReady,
isProviderMutationPending,
onCreateProvider,
onUpdateProvider,
@@ -173,6 +175,7 @@ export const ProvidersSection: FC<ProvidersSectionProps> = ({
key={providerFormKey}
providerState={detailProvider}
providerConfigsUnavailable={providerConfigsUnavailable}
modelConfigsReady={modelConfigsReady}
isProviderMutationPending={isProviderMutationPending}
onCreateProvider={async (req) => {
const createdProvider = await onCreateProvider(req);
site/src/pages/AgentsPage/components/ConfirmDeleteDialog.tsx
+7 -1
View File
@@ -22,6 +22,7 @@ interface ConfirmDeleteDialogProps {
description?: ReactNode;
children?: ReactNode;
onConfirm: () => void;
confirmDisabled?: boolean;
isPending?: boolean;
}
@@ -32,6 +33,7 @@ export const ConfirmDeleteDialog: FC<ConfirmDeleteDialogProps> = ({
description,
children,
onConfirm,
confirmDisabled = false,
isPending = false,
}) => (
<Dialog open={open} onOpenChange={onOpenChange}>
@@ -52,7 +54,11 @@ export const ConfirmDeleteDialog: FC<ConfirmDeleteDialogProps> = ({
>
Cancel
</Button>
<Button variant="destructive" onClick={onConfirm} disabled={isPending}>
<Button
variant="destructive"
onClick={onConfirm}
disabled={isPending || confirmDisabled}
>
{isPending && <Spinner className="h-4 w-4" loading />}
Delete {entity}
</Button>