refactor: redesign compaction settings to table layout with batch save (#23844)

site/src/pages/AgentsPage/components/UserCompactionThresholdSettings.stories.tsx

@@ -68,7 +68,7 @@ export default meta;
 type Story = StoryObj<typeof meta>;
 
 export const Default: Story = {
-	play: async ({ canvasElement, args }) => {
+	play: async ({ canvasElement }) => {
 		const canvas = within(canvasElement);
 		const gpt4oInput = await canvas.findByRole("spinbutton", {
 			name: /GPT-4o compaction threshold/i,
@@ -78,23 +78,44 @@ export const Default: Story = {
 		expect(canvas.getByText("Claude Sonnet")).toBeInTheDocument();
 		expect(canvas.queryByText("GPT-3.5 (Disabled)")).not.toBeInTheDocument();
 
-		await userEvent.type(gpt4oInput, "100");
+		// No footer visible when nothing is dirty
 		expect(
-			canvas.getByText(
-				"⚠ Setting 100% will disable auto-compaction for this model.",
-			),
-		).toBeInTheDocument();
-		await userEvent.clear(gpt4oInput);
-		await userEvent.type(gpt4oInput, "95");
+			canvas.queryByRole("button", { name: /Save/i }),
+		).not.toBeInTheDocument();
 
-		const saveButtons = canvas.getAllByRole("button", { name: "Save" });
+		// Type a value to make the footer appear
+		await userEvent.type(gpt4oInput, "95");
 		await waitFor(() => {
-			expect(saveButtons[0]).toBeEnabled();
+			expect(
+				canvas.getByRole("button", { name: /Save 1 change/i }),
+			).toBeInTheDocument();
+		});
+	},
+};
+
+export const SaveAll: Story = {
+	play: async ({ canvasElement, args }) => {
+		const canvas = within(canvasElement);
+		const gpt4oInput = await canvas.findByRole("spinbutton", {
+			name: /GPT-4o compaction threshold/i,
+		});
+		const claudeInput = await canvas.findByRole("spinbutton", {
+			name: /Claude Sonnet compaction threshold/i,
 		});
 
-		await userEvent.click(saveButtons[0]);
+		// Edit both models
+		await userEvent.type(gpt4oInput, "95");
+		await userEvent.type(claudeInput, "50");
+
+		// Footer should show "Save 2 changes"
+		const saveButton = await canvas.findByRole("button", {
+			name: /Save 2 changes/i,
+		});
+		await userEvent.click(saveButton);
+
 		await waitFor(() => {
 			expect(args.onSaveThreshold).toHaveBeenCalledWith("model-1", 95);
+			expect(args.onSaveThreshold).toHaveBeenCalledWith("model-2", 50);
 		});
 	},
 };
@@ -118,7 +139,12 @@ export const WithOverrides: Story = {
 		expect(gpt4oInput).toHaveValue(90);
 		expect(claudeInput).toHaveValue(50);
 
-		const resetButtons = canvas.getAllByRole("button", { name: "Reset" });
+		// Reset buttons should be visible for both overridden models
+		const resetButtons = canvas.getAllByRole("button", {
+			name: /Reset .+ to default/i,
+		});
+		expect(resetButtons).toHaveLength(2);
+
 		await userEvent.click(resetButtons[0]);
 		await waitFor(() => {
 			expect(args.onResetThreshold).toHaveBeenCalledWith("model-1");
@@ -126,12 +152,124 @@ export const WithOverrides: Story = {
 	},
 };
 
+export const CancelChanges: Story = {
+	play: async ({ canvasElement }) => {
+		const canvas = within(canvasElement);
+		const gpt4oInput = await canvas.findByRole("spinbutton", {
+			name: /GPT-4o compaction threshold/i,
+		});
+
+		await userEvent.type(gpt4oInput, "42");
+		const cancelButton = await canvas.findByRole("button", { name: /Cancel/i });
+		await userEvent.click(cancelButton);
+
+		// Footer should disappear after cancel
+		await waitFor(() => {
+			expect(
+				canvas.queryByRole("button", { name: /Save/i }),
+			).not.toBeInTheDocument();
+		});
+
+		// Input should be cleared back to empty (no override)
+		expect(gpt4oInput).toHaveValue(null);
+	},
+};
+
+export const InvalidDraftShowsFooter: Story = {
+	name: "Invalid Draft Shows Footer",
+	play: async ({ canvasElement }) => {
+		const canvas = within(canvasElement);
+		const gpt4oInput = await canvas.findByRole("spinbutton", {
+			name: /GPT-4o compaction threshold/i,
+		});
+
+		// Type an out-of-range value (number inputs reject non-numeric chars)
+		await userEvent.type(gpt4oInput, "150");
+
+		// Input should be marked invalid
+		await waitFor(() => {
+			expect(gpt4oInput).toHaveAttribute("aria-invalid", "true");
+		});
+
+		// Cancel button should be visible so user can discard the edit
+		expect(canvas.getByRole("button", { name: /Cancel/i })).toBeInTheDocument();
+
+		// Save button should NOT be visible (nothing valid to save)
+		expect(
+			canvas.queryByRole("button", { name: /Save/i }),
+		).not.toBeInTheDocument();
+	},
+};
+
+export const DisableCompactionWarning: Story = {
+	name: "100% Disable Compaction Warning",
+	play: async ({ canvasElement }) => {
+		const canvas = within(canvasElement);
+		const gpt4oInput = await canvas.findByRole("spinbutton", {
+			name: /GPT-4o compaction threshold/i,
+		});
+
+		await userEvent.type(gpt4oInput, "100");
+
+		// sr-only warning should be in the DOM for screen readers
+		await waitFor(() => {
+			expect(
+				canvas.getByText(
+					"Setting 100% will disable auto-compaction for this model.",
+				),
+			).toBeInTheDocument();
+		});
+	},
+};
+
 export const Loading: Story = {
 	args: {
 		isThresholdsLoading: true,
 	},
 };
 
+export const PartialSaveFailure: Story = {
+	name: "Partial Save Failure",
+	args: {
+		onSaveThreshold: fn(async (modelConfigId: string) => {
+			if (modelConfigId === "model-2") {
+				throw new globalThis.Error("Network error");
+			}
+		}),
+	},
+	play: async ({ canvasElement, args }) => {
+		const canvas = within(canvasElement);
+		const gpt4oInput = await canvas.findByRole("spinbutton", {
+			name: /GPT-4o compaction threshold/i,
+		});
+		const claudeInput = await canvas.findByRole("spinbutton", {
+			name: /Claude Sonnet compaction threshold/i,
+		});
+
+		await userEvent.type(gpt4oInput, "90");
+		await userEvent.type(claudeInput, "55");
+
+		const saveButton = await canvas.findByRole("button", {
+			name: /Save 2 changes/i,
+		});
+		await userEvent.click(saveButton);
+
+		await waitFor(() => {
+			expect(args.onSaveThreshold).toHaveBeenCalledWith("model-1", 90);
+			expect(args.onSaveThreshold).toHaveBeenCalledWith("model-2", 55);
+		});
+
+		// model-2 should show an error, footer should still be visible
+		// with Save showing "Save 1 change" for the failed row
+		await waitFor(() => {
+			expect(canvas.getByText("Network error")).toBeInTheDocument();
+			expect(
+				canvas.getByRole("button", { name: /Save 1 change/i }),
+			).toBeInTheDocument();
+		});
+	},
+};
+
 export const ErrorState: Story = {
 	name: "Error",
 	args: {

site/src/pages/AgentsPage/components/UserCompactionThresholdSettings.tsx

@@ -1,9 +1,25 @@
+import { RotateCcwIcon } from "lucide-react";
 import { type FC, useState } from "react";
 import { getErrorMessage } from "#/api/errors";
 import type * as TypesGen from "#/api/typesGenerated";
 import { Button } from "#/components/Button/Button";
 import { Input } from "#/components/Input/Input";
 import { Spinner } from "#/components/Spinner/Spinner";
+import {
+	Table,
+	TableBody,
+	TableCell,
+	TableFooter,
+	TableHead,
+	TableHeader,
+	TableRow,
+} from "#/components/Table/Table";
+import {
+	Tooltip,
+	TooltipContent,
+	TooltipTrigger,
+} from "#/components/Tooltip/Tooltip";
+import { cn } from "#/utils/cn";
 
 interface UserCompactionThresholdSettingsProps {
 	modelConfigs: readonly TypesGen.ChatModelConfig[];
@@ -49,6 +65,16 @@ export const UserCompactionThresholdSettings: FC<
 	const [rowErrors, setRowErrors] = useState<Record<string, string>>({});
 	const [pendingModels, setPendingModels] = useState<Set<string>>(new Set());
 
+	const enabledModelConfigs = modelConfigs.filter((config) => config.enabled);
+	const overridesByModelID = new Map(
+		(thresholds ?? []).map(
+			(threshold: TypesGen.UserChatCompactionThreshold) => [
+				threshold.model_config_id,
+				threshold.threshold_percent,
+			],
+		),
+	);
+
 	const clearDraft = (modelConfigID: string) => {
 		setDrafts((currentDrafts) => {
 			const nextDrafts = { ...currentDrafts };
@@ -68,39 +94,21 @@ export const UserCompactionThresholdSettings: FC<
 		});
 	};
 
-	const handleSave = (modelConfigId: string, thresholdPercent: number) => {
-		clearRowError(modelConfigId);
-		setPendingModels((currentPendingModels) =>
-			new Set(currentPendingModels).add(modelConfigId),
-		);
-		onSaveThreshold(modelConfigId, thresholdPercent)
-			.then(() => {
-				clearDraft(modelConfigId);
-				clearRowError(modelConfigId);
-			})
-			.catch((error: unknown) => {
-				setRowErrors((currentErrors) => ({
-					...currentErrors,
-					[modelConfigId]: getErrorMessage(
-						error,
-						"Failed to save compaction threshold.",
-					),
-				}));
-			})
-			.finally(() => {
-				setPendingModels((currentPendingModels) => {
-					const nextPendingModels = new Set(currentPendingModels);
-					nextPendingModels.delete(modelConfigId);
-					return nextPendingModels;
-				});
-			});
+	const addPending = (id: string) => {
+		setPendingModels((pending) => new Set(pending).add(id));
+	};
+
+	const removePending = (id: string) => {
+		setPendingModels((pending) => {
+			const next = new Set(pending);
+			next.delete(id);
+			return next;
+		});
 	};
 
 	const handleReset = (modelConfigId: string) => {
 		clearRowError(modelConfigId);
-		setPendingModels((currentPendingModels) =>
-			new Set(currentPendingModels).add(modelConfigId),
-		);
+		addPending(modelConfigId);
 		onResetThreshold(modelConfigId)
 			.then(() => {
 				clearDraft(modelConfigId);
@@ -116,23 +124,57 @@ export const UserCompactionThresholdSettings: FC<
 				}));
 			})
 			.finally(() => {
-				setPendingModels((currentPendingModels) => {
-					const nextPendingModels = new Set(currentPendingModels);
-					nextPendingModels.delete(modelConfigId);
-					return nextPendingModels;
-				});
+				removePending(modelConfigId);
 			});
 	};
 
-	const enabledModelConfigs = modelConfigs.filter((config) => config.enabled);
-	const overridesByModelID = new Map(
-		(thresholds ?? []).map(
-			(threshold: TypesGen.UserChatCompactionThreshold) => [
-				threshold.model_config_id,
-				threshold.threshold_percent,
-			],
-		),
-	);
+	// Compute dirty rows: rows where the user has typed a valid value
+	// that differs from the current server-side override.
+	const dirtyRows: Array<{ modelConfigId: string; value: number }> = [];
+	for (const modelConfig of enabledModelConfigs) {
+		const draft = drafts[modelConfig.id];
+		if (draft === undefined) continue;
+		const parsed = parseThresholdDraft(draft);
+		if (parsed === null) continue;
+		const existingOverride = overridesByModelID.get(modelConfig.id);
+		if (parsed === existingOverride) continue;
+		dirtyRows.push({ modelConfigId: modelConfig.id, value: parsed });
+	}
+
+	const handleSaveAll = () => {
+		const saves = dirtyRows.map(({ modelConfigId, value }) => {
+			clearRowError(modelConfigId);
+			addPending(modelConfigId);
+			return onSaveThreshold(modelConfigId, value)
+				.then(() => {
+					clearDraft(modelConfigId);
+					clearRowError(modelConfigId);
+				})
+				.catch((error: unknown) => {
+					setRowErrors((currentErrors) => ({
+						...currentErrors,
+						[modelConfigId]: getErrorMessage(
+							error,
+							"Failed to save compaction threshold.",
+						),
+					}));
+				})
+				.finally(() => {
+					removePending(modelConfigId);
+				});
+		});
+		void Promise.allSettled(saves);
+	};
+
+	const handleCancelAll = () => {
+		setDrafts({});
+		setRowErrors({});
+	};
+
+	const hasAnyPending = pendingModels.size > 0;
+	const hasAnyErrors = Object.keys(rowErrors).length > 0;
+	const hasAnyDrafts = Object.keys(drafts).length > 0;
+
 	if (isThresholdsLoading) {
 		return (
 			<div className="space-y-2">
@@ -198,107 +240,165 @@ export const UserCompactionThresholdSettings: FC<
 					models before compaction thresholds can be set.
 				</p>
 			) : (
-				<div className="space-y-2">
-					{enabledModelConfigs.map((modelConfig) => {
-						const existingOverride = overridesByModelID.get(modelConfig.id);
-						const hasOverride = overridesByModelID.has(modelConfig.id);
-						const draftValue =
-							drafts[modelConfig.id] ??
-							(existingOverride !== undefined ? String(existingOverride) : "");
-						const parsedDraftValue = parseThresholdDraft(draftValue);
-						const isThisModelMutating = pendingModels.has(modelConfig.id);
-						const isSaveDisabled =
-							draftValue.length === 0 ||
-							parsedDraftValue === null ||
-							parsedDraftValue === existingOverride ||
-							isThisModelMutating;
+				<Table>
+					<TableHeader>
+						<TableRow>
+							<TableHead>Model</TableHead>
+							<TableHead className="w-0 whitespace-nowrap">Default</TableHead>
+							<TableHead className="w-0 whitespace-nowrap">Threshold</TableHead>
+						</TableRow>
+					</TableHeader>
+					<TableBody>
+						{enabledModelConfigs.map((modelConfig) => {
+							const existingOverride = overridesByModelID.get(modelConfig.id);
+							const hasOverride = overridesByModelID.has(modelConfig.id);
+							const draftValue =
+								drafts[modelConfig.id] ??
+								(existingOverride !== undefined
+									? String(existingOverride)
+									: "");
+							const parsedDraftValue = parseThresholdDraft(draftValue);
+							const isThisModelMutating = pendingModels.has(modelConfig.id);
+							const isInvalid =
+								draftValue.length > 0 && parsedDraftValue === null;
+							// Only warn when user-typed, not when loaded from
+							// the server.
+							const isDraftDisablingCompaction =
+								draftValue === "100" && drafts[modelConfig.id] !== undefined;
+							const rowError = rowErrors[modelConfig.id];
+							const modelName = modelConfig.display_name || modelConfig.model;
 
-						return (
-							<div key={modelConfig.id} className="space-y-1">
-								<div className="flex items-center justify-between gap-3">
-									<div className="flex min-w-0 flex-1 items-baseline gap-2">
-										<span className="text-[13px] font-medium text-content-primary">
-											{modelConfig.display_name || modelConfig.model}
-										</span>
-										<span className="ml-auto text-xs text-content-secondary">
-											System default:{" "}
-											<span className="inline-block w-[4ch] text-right tabular-nums">
-												{modelConfig.compression_threshold}%
+							return (
+								<TableRow key={modelConfig.id}>
+									<TableCell className="text-[13px] font-medium text-content-primary">
+										{modelName}
+										{rowError && (
+											<p
+												aria-live="polite"
+												className="m-0 mt-0.5 text-2xs font-normal text-content-destructive"
+											>
+												{rowError}
+											</p>
+										)}
+									</TableCell>
+									<TableCell className="w-0 whitespace-nowrap tabular-nums">
+										{modelConfig.compression_threshold}%
+									</TableCell>
+									<TableCell className="w-0 whitespace-nowrap">
+										<div className="flex items-center gap-1.5">
+											<Tooltip>
+												<TooltipTrigger asChild>
+													<Input
+														aria-label={`${modelName} compaction threshold`}
+														aria-invalid={isInvalid || undefined}
+														type="number"
+														min={0}
+														max={100}
+														inputMode="numeric"
+														className={cn(
+															"h-7 w-16 px-2 text-xs tabular-nums",
+															isInvalid &&
+																"border-content-destructive focus:ring-content-destructive/30",
+														)}
+														value={draftValue}
+														placeholder={String(
+															modelConfig.compression_threshold,
+														)}
+														onChange={(event) => {
+															setDrafts((currentDrafts) => ({
+																...currentDrafts,
+																[modelConfig.id]: event.target.value,
+															}));
+															clearRowError(modelConfig.id);
+														}}
+														disabled={isThisModelMutating}
+													/>
+												</TooltipTrigger>
+												{(isInvalid || isDraftDisablingCompaction) && (
+													<TooltipContent>
+														{isInvalid
+															? "Enter a whole number between 0 and 100."
+															: "Setting 100% will disable auto-compaction for this model."}
+													</TooltipContent>
+												)}
+											</Tooltip>
+											<span className="text-xs text-content-secondary">%</span>
+											<Tooltip>
+												<TooltipTrigger asChild>
+													<Button
+														size="icon"
+														variant="subtle"
+														className={cn(
+															"size-7",
+															hasOverride
+																? "opacity-100"
+																: "pointer-events-none opacity-0",
+														)}
+														aria-label={`Reset ${modelName} to default`}
+														aria-hidden={!hasOverride}
+														tabIndex={hasOverride ? 0 : -1}
+														disabled={isThisModelMutating || !hasOverride}
+														onClick={() => handleReset(modelConfig.id)}
+													>
+														<RotateCcwIcon className="size-3.5" />
+													</Button>
+												</TooltipTrigger>
+												{hasOverride && (
+													<TooltipContent>
+														Reset to default (
+														{modelConfig.compression_threshold}%)
+													</TooltipContent>
+												)}
+											</Tooltip>
+										</div>
+										{isInvalid && (
+											<span className="sr-only" aria-live="polite">
+												Enter a whole number between 0 and 100.
 											</span>
-										</span>
-									</div>
-									<div className="flex items-center gap-1.5">
-										<Input
-											aria-label={`${modelConfig.display_name || modelConfig.model} compaction threshold`}
-											type="number"
-											min={0}
-											max={100}
-											inputMode="numeric"
-											className="h-7 w-16 px-2 text-xs"
-											value={draftValue}
-											placeholder={String(modelConfig.compression_threshold)}
-											onChange={(event) => {
-												setDrafts((currentDrafts) => ({
-													...currentDrafts,
-													[modelConfig.id]: event.target.value,
-												}));
-												clearRowError(modelConfig.id);
-											}}
-											disabled={isThisModelMutating}
-										/>
-										<span className="text-xs text-content-secondary">%</span>
+										)}
+										{isDraftDisablingCompaction && (
+											<span className="sr-only" aria-live="polite">
+												Setting 100% will disable auto-compaction for this
+												model.
+											</span>
+										)}
+									</TableCell>
+								</TableRow>
+							);
+						})}
+					</TableBody>
+					{(dirtyRows.length > 0 || hasAnyErrors || hasAnyDrafts) && (
+						<TableFooter className="bg-transparent">
+							<TableRow className="border-0">
+								<TableCell colSpan={3} className="border-0 p-0">
+									<div className="flex items-center justify-end gap-2 px-3 py-1.5">
 										<Button
 											size="sm"
-											className="h-7"
+											variant="outline"
 											type="button"
-											disabled={isSaveDisabled}
-											onClick={() => {
-												if (parsedDraftValue === null) {
-													return;
-												}
-												handleSave(modelConfig.id, parsedDraftValue);
-											}}
+											onClick={handleCancelAll}
+											disabled={hasAnyPending}
 										>
-											Save
+											Cancel
 										</Button>
-										{hasOverride && (
+										{dirtyRows.length > 0 && (
 											<Button
 												size="sm"
-												className="h-7"
-												variant="outline"
 												type="button"
-												disabled={isThisModelMutating}
-												onClick={() => {
-													handleReset(modelConfig.id);
-												}}
+												disabled={hasAnyPending}
+												onClick={handleSaveAll}
 											>
-												Reset
+												{hasAnyPending
+													? "Saving..."
+													: `Save ${dirtyRows.length} ${dirtyRows.length === 1 ? "change" : "changes"}`}
 											</Button>
 										)}
 									</div>
-								</div>
-								{draftValue.length > 0 && parsedDraftValue === null && (
-									<p className="m-0 text-xs text-content-destructive">
-										Enter a whole number between 0 and 100.
-									</p>
-								)}
-								{rowErrors[modelConfig.id] && (
-									<p
-										aria-live="polite"
-										className="m-0 text-xs text-content-destructive"
-									>
-										{rowErrors[modelConfig.id]}
-									</p>
-								)}
-								{draftValue === "100" && (
-									<p className="m-0 text-xs text-content-secondary">
-										⚠ Setting 100% will disable auto-compaction for this model.
-									</p>
-								)}
-							</div>
-						);
-					})}
-				</div>
+								</TableCell>
+							</TableRow>
+						</TableFooter>
+					)}
+				</Table>
 			)}
 		</div>
 	);