fix: do not clobber dynamic parameters (#24645)

Once a user has touched a field, it is better to leave it alone and display explicit validation errors over silently overwriting their inputs.  Same for auto-filled values (whether from query parameters or a previous build).

site/src/modules/hooks/useSyncFormParameters.ts

@@ -1,3 +1,4 @@
+import type { FormikTouched } from "formik";
 import { useEffect, useRef } from "react";
 import type * as TypesGen from "#/api/typesGenerated";
 import type { PreviewParameter } from "#/api/typesGenerated";
@@ -5,6 +6,9 @@ import type { PreviewParameter } from "#/api/typesGenerated";
 type UseSyncFormParametersProps = {
 	parameters: readonly PreviewParameter[];
 	formValues: readonly TypesGen.WorkspaceBuildParameter[];
+	touched: FormikTouched<{
+		rich_parameter_values?: readonly TypesGen.WorkspaceBuildParameter[];
+	}>;
 	setFieldValue: (
 		field: string,
 		value: TypesGen.WorkspaceBuildParameter[],
@@ -14,6 +18,7 @@ type UseSyncFormParametersProps = {
 export function useSyncFormParameters({
 	parameters,
 	formValues,
+	touched,
 	setFieldValue,
 }: UseSyncFormParametersProps) {
 	// Form values only needs to be updated when parameters change
@@ -30,13 +35,16 @@ export function useSyncFormParameters({
 		);
 
 		const newParameterValues = parameters.map((param) => {
-			// When the server value is not valid (e.g., the initial
-			// WebSocket response before any user input is sent),
-			// preserve the current form value. This prevents the sync
-			// hook from overwriting autofilled values (from the
-			// previous build) with empty strings before the server
-			// has had a chance to process them.
-			if (!param.value.valid) {
+			// Do not mess with values the user has changed (or were auto-filled).
+			// Otherwise based on timing web socket responses can undo changes, and it
+			// seems bad to change a user's inputs from under them anyway.
+			if (
+				touched[
+					param.name as keyof {
+						rich_parameter_values?: readonly TypesGen.WorkspaceBuildParameter[];
+					}
+				]
+			) {
 				const existingValue = currentFormValuesMap.get(param.name);
 				if (existingValue !== undefined) {
 					return { name: param.name, value: existingValue };
@@ -60,5 +68,5 @@ export function useSyncFormParameters({
 		if (isChanged) {
 			setFieldValue("rich_parameter_values", newParameterValues);
 		}
-	}, [parameters, setFieldValue]);
+	}, [parameters, touched, setFieldValue]);
 }

site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx

@@ -8,6 +8,7 @@ import {
 	MockDynamicParametersResponse,
 	MockDynamicParametersResponseWithError,
 	MockPermissions,
+	MockPreviewParameter,
 	MockSliderParameter,
 	MockTemplate,
 	MockTemplateVersionExternalAuthGithub,
@@ -20,7 +21,7 @@ import {
 	renderWithAuth,
 	waitForLoaderToBeRemoved,
 } from "#/testHelpers/renderHelpers";
-import { createMockWebSocket } from "#/testHelpers/websockets";
+import { mockDynamicParameterWebSocket } from "#/testHelpers/websockets";
 import CreateWorkspacePage from "./CreateWorkspacePage";
 
 describe("CreateWorkspacePage", () => {
@@ -47,33 +48,7 @@ describe("CreateWorkspacePage", () => {
 		vi.spyOn(API, "getTemplateVersionPresets").mockResolvedValue([]);
 		vi.spyOn(API, "createWorkspace").mockResolvedValue(MockWorkspace);
 		vi.spyOn(API, "checkAuthorization").mockResolvedValue(MockPermissions);
-
-		vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-			(_versionId, _ownerId, callbacks) => {
-				const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
-
-				mockWebSocket.addEventListener("message", (event) => {
-					callbacks.onMessage(JSON.parse(event.data));
-				});
-				mockWebSocket.addEventListener("error", () => {
-					callbacks.onError(
-						new Error("Connection for dynamic parameters failed."),
-					);
-				});
-				mockWebSocket.addEventListener("close", () => {
-					callbacks.onClose();
-				});
-
-				publisher.publishOpen(new Event("open"));
-				publisher.publishMessage(
-					new MessageEvent("message", {
-						data: JSON.stringify(MockDynamicParametersResponse),
-					}),
-				);
-
-				return mockWebSocket;
-			},
-		);
+		mockDynamicParameterWebSocket(MockDynamicParametersResponse);
 	});
 
 	afterEach(() => {
@@ -106,31 +81,8 @@ describe("CreateWorkspacePage", () => {
 		});
 
 		it("sends parameter updates via WebSocket when form values change", async () => {
-			const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
-
-			vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-				(_versionId, _ownerId, callbacks) => {
-					mockWebSocket.addEventListener("message", (event) => {
-						callbacks.onMessage(JSON.parse(event.data));
-					});
-					mockWebSocket.addEventListener("error", () => {
-						callbacks.onError(
-							new Error("Connection for dynamic parameters failed."),
-						);
-					});
-					mockWebSocket.addEventListener("close", () => {
-						callbacks.onClose();
-					});
-
-					publisher.publishOpen(new Event("open"));
-					publisher.publishMessage(
-						new MessageEvent("message", {
-							data: JSON.stringify(MockDynamicParametersResponse),
-						}),
-					);
-
-					return mockWebSocket;
-				},
+			const [mockWebSocket] = mockDynamicParameterWebSocket(
+				MockDynamicParametersResponse,
 			);
 
 			renderCreateWorkspacePage();
@@ -167,17 +119,7 @@ describe("CreateWorkspacePage", () => {
 		});
 
 		it("handles WebSocket error gracefully", async () => {
-			const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
-
-			vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-				(_versionId, _ownerId, callbacks) => {
-					mockWebSocket.addEventListener("error", () => {
-						callbacks.onError(new Error("Connection failed"));
-					});
-
-					return mockWebSocket;
-				},
-			);
+			const [, mockPublisher] = mockDynamicParameterWebSocket([]);
 
 			renderCreateWorkspacePage();
 
@@ -192,23 +134,15 @@ describe("CreateWorkspacePage", () => {
 			await waitFor(() => {
 				const alert = screen.getByRole("alert");
 				expect(
-					within(alert).getByRole("heading", { name: /connection failed/i }),
+					within(alert).getByRole("heading", {
+						name: /connection for dynamic parameters failed/i,
+					}),
 				).toBeInTheDocument();
 			});
 		});
 
 		it("handles WebSocket close event", async () => {
-			const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
-
-			vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-				(_versionId, _ownerId, callbacks) => {
-					mockWebSocket.addEventListener("close", () => {
-						callbacks.onClose();
-					});
-
-					return mockWebSocket;
-				},
-			);
+			const [, mockPublisher] = mockDynamicParameterWebSocket([]);
 
 			renderCreateWorkspacePage();
 
@@ -231,27 +165,9 @@ describe("CreateWorkspacePage", () => {
 		});
 
 		it("only parameters from latest response are displayed", async () => {
-			const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
-			vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-				(_versionId, _ownerId, callbacks) => {
-					mockWebSocket.addEventListener("message", (event) => {
-						callbacks.onMessage(JSON.parse(event.data));
-					});
-
-					mockPublisher.publishOpen(new Event("open"));
-					mockPublisher.publishMessage(
-						new MessageEvent("message", {
-							data: JSON.stringify({
-								id: 0,
-								parameters: [MockDropdownParameter],
-								diagnostics: [],
-							}),
-						}),
-					);
-
-					return mockWebSocket;
-				},
-			);
+			const [, mockPublisher] = mockDynamicParameterWebSocket([
+				MockDropdownParameter,
+			]);
 
 			renderCreateWorkspacePage();
 			await waitForLoaderToBeRemoved();
@@ -282,27 +198,89 @@ describe("CreateWorkspacePage", () => {
 				expect(screen.queryByText("Instance Type")).not.toBeInTheDocument();
 			});
 		});
+
+		it("does not clobber user values", async () => {
+			const [, mockPublisher] = mockDynamicParameterWebSocket([
+				MockPreviewParameter,
+			]);
+
+			renderCreateWorkspacePage();
+			await waitForLoaderToBeRemoved();
+
+			const form = screen.getByTestId("form");
+			const input = await within(form).findByRole("textbox", {
+				name: /parameter 1/i,
+			});
+			await userEvent.clear(input);
+			await userEvent.type(input, "hi there hello");
+
+			await waitFor(() => {
+				expect(
+					within(form).getByDisplayValue("hi there hello"),
+				).toBeInTheDocument();
+			});
+
+			// Simulate a stale response.
+			await act(async () => {
+				mockPublisher.publishMessage(
+					new MessageEvent("message", {
+						data: JSON.stringify({
+							id: 1,
+							parameters: [MockPreviewParameter, MockValidationParameter],
+						}),
+					}),
+				);
+			});
+
+			// Should have the new field, but keep the existing user-filled values.
+			await waitFor(() => {
+				expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
+				expect(
+					within(form).getByDisplayValue("hi there hello"),
+				).toBeInTheDocument();
+			});
+		});
+
+		it("does not clobber auto-filled values", async () => {
+			const [, mockPublisher] = mockDynamicParameterWebSocket([
+				MockPreviewParameter,
+				MockSliderParameter,
+			]);
+
+			renderCreateWorkspacePage(
+				`/templates/${MockTemplate.name}/workspace?param.cpu_count=44&param.parameter1=auto`,
+			);
+			await waitForLoaderToBeRemoved();
+
+			// Simulate a stale response.
+			await act(async () => {
+				mockPublisher.publishMessage(
+					new MessageEvent("message", {
+						data: JSON.stringify({
+							id: 2,
+							parameters: [
+								MockPreviewParameter,
+								MockSliderParameter,
+								MockValidationParameter,
+							],
+						}),
+					}),
+				);
+			});
+
+			// Should have the new field, but keep the existing auto-filled values.
+			const form = screen.getByTestId("form");
+			await waitFor(() => {
+				expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
+				expect(within(form).getByDisplayValue("44")).toBeInTheDocument();
+				expect(within(form).getByDisplayValue("auto")).toBeInTheDocument();
+			});
+		});
 	});
 
 	describe("Dynamic Parameter Types", () => {
 		it("displays parameter validation errors", async () => {
-			vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-				(_versionId, _ownerId, callbacks) => {
-					const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
-
-					mockWebSocket.addEventListener("message", (event) => {
-						callbacks.onMessage(JSON.parse(event.data));
-					});
-
-					publisher.publishMessage(
-						new MessageEvent("message", {
-							data: JSON.stringify(MockDynamicParametersResponseWithError),
-						}),
-					);
-
-					return mockWebSocket;
-				},
-			);
+			mockDynamicParameterWebSocket(MockDynamicParametersResponseWithError);
 
 			renderCreateWorkspacePage();
 			await waitForLoaderToBeRemoved();
@@ -346,38 +324,20 @@ describe("CreateWorkspacePage", () => {
 				diagnostics: [],
 			};
 
-			vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
-				(_versionId, _ownerId, callbacks) => {
-					const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
-
-					mockWebSocket.addEventListener("message", (event) => {
-						callbacks.onMessage(JSON.parse(event.data));
-					});
-
-					publisher.publishOpen(new Event("open"));
+			const [mockWebSocket, publisher] =
+				mockDynamicParameterWebSocket(mockResponseInitial);
+			const originalSend = mockWebSocket.send;
+			mockWebSocket.send = vi.fn((data) => {
+				originalSend.call(mockWebSocket, data);
 
+				if (typeof data === "string" && data.includes('"200"')) {
 					publisher.publishMessage(
 						new MessageEvent("message", {
-							data: JSON.stringify(mockResponseInitial),
+							data: JSON.stringify(mockResponseWithError),
 						}),
 					);
-
-					const originalSend = mockWebSocket.send;
-					mockWebSocket.send = vi.fn((data) => {
-						originalSend.call(mockWebSocket, data);
-
-						if (typeof data === "string" && data.includes('"200"')) {
-							publisher.publishMessage(
-								new MessageEvent("message", {
-									data: JSON.stringify(mockResponseWithError),
-								}),
-							);
-						}
-					});
-
-					return mockWebSocket;
-				},
-			);
+				}
+			});
 
 			renderCreateWorkspacePage();
 			await waitForLoaderToBeRemoved();

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx

@@ -138,7 +138,9 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
 	// 1. The form parameter values are initialized from the websocket response when the form is mounted
 	// 2. Only touched form fields are sent to the websocket, a field is touched if edited by the user or set by autofill
 	// 3. The websocket response may add or remove parameters, these are added or removed from the form values in the useSyncFormParameters hook
-	// 4. All existing form parameters are updated to match the websocket response in the useSyncFormParameters hook
+	// 4. All existing form parameters are updated to match the websocket response
+	//    in the useSyncFormParameters hook, unless they have been touched by the
+	//    user or auto-filled.
 	const form: FormikContextType<TypesGen.CreateWorkspaceRequest> =
 		useFormik<TypesGen.CreateWorkspaceRequest>({
 			initialValues: {
@@ -360,6 +362,7 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
 	useSyncFormParameters({
 		parameters,
 		formValues: form.values.rich_parameter_values ?? [],
+		touched: form.touched,
 		setFieldValue: form.setFieldValue,
 	});
 
@@ -443,6 +446,7 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
 					onSubmit={form.handleSubmit}
 					aria-label="Create workspace form"
 					className="flex flex-col gap-10 w-full border border-border-default border-solid rounded-lg p-6"
+					data-testid="form"
 				>
 					{Boolean(error) && <ErrorAlert error={error} />}
 

site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx

@@ -0,0 +1,102 @@
+import { screen, waitFor, within } from "@testing-library/react";
+import { act } from "react";
+import { API } from "#/api/api";
+import {
+	MockPreviewParameter,
+	MockTemplateVersionParameter1,
+	MockTemplateVersionParameter2,
+	MockTemplateVersionParameter4,
+	MockValidationParameter,
+	MockWorkspace,
+	MockWorkspaceBuildParameter1,
+	MockWorkspaceBuildParameter2,
+	MockWorkspaceBuildParameter4,
+} from "#/testHelpers/entities";
+import {
+	renderWithWorkspaceSettingsLayout,
+	waitForLoaderToBeRemoved,
+} from "#/testHelpers/renderHelpers";
+import { mockDynamicParameterWebSocket } from "#/testHelpers/websockets";
+import WorkspaceParametersPageExperimental from "./WorkspaceParametersPageExperimental";
+
+describe("WorkspaceParametersPageExperimental", () => {
+	const renderWorkspaceParametersPageExperimental = (
+		route = `/@${MockWorkspace.owner_name}/${MockWorkspace.name}/settings`,
+	) => {
+		return renderWithWorkspaceSettingsLayout(
+			<WorkspaceParametersPageExperimental />,
+			{
+				route,
+				path: "/:username/:workspace/settings",
+				extraRoutes: [
+					{
+						// Need this because after submit the user is redirected.
+						path: "/:username/:workspace",
+						element: <div>Workspace Page</div>,
+					},
+				],
+			},
+		);
+	};
+
+	beforeEach(() => {
+		vi.clearAllMocks();
+		vi.spyOn(API, "getWorkspaceByOwnerAndName").mockResolvedValueOnce(
+			MockWorkspace,
+		);
+		vi.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValueOnce([
+			MockTemplateVersionParameter1,
+			MockTemplateVersionParameter2,
+			MockTemplateVersionParameter4,
+		]);
+		vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([
+			MockWorkspaceBuildParameter1,
+			MockWorkspaceBuildParameter2,
+			MockWorkspaceBuildParameter4,
+		]);
+	});
+
+	afterEach(() => {
+		vi.useRealTimers();
+		vi.restoreAllMocks();
+	});
+
+	it("does not clobber touched parameters", async () => {
+		const [, mockPublisher] = mockDynamicParameterWebSocket([
+			{
+				...MockPreviewParameter,
+				name: MockWorkspaceBuildParameter1.name,
+			},
+		]);
+
+		renderWorkspaceParametersPageExperimental();
+		await waitForLoaderToBeRemoved();
+
+		// Simulate a stale response.
+		await act(async () => {
+			mockPublisher.publishMessage(
+				new MessageEvent("message", {
+					data: JSON.stringify({
+						id: 2,
+						parameters: [
+							{
+								...MockPreviewParameter,
+								name: MockWorkspaceBuildParameter1.name,
+							},
+							MockValidationParameter,
+						],
+					}),
+				}),
+			);
+		});
+
+		// Should have the new field, but keep the existing auto-filled values.
+		const form = screen.getByTestId("form");
+		await waitFor(() => {
+			expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
+			expect(
+				within(form).getByDisplayValue(MockWorkspaceBuildParameter1.value),
+			).toBeInTheDocument();
+		});
+	});
+});

site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageViewExperimental.tsx

@@ -60,6 +60,9 @@ export const WorkspaceParametersPageViewExperimental: FC<
 				autofillParameters,
 			),
 		},
+		initialTouched: Object.fromEntries(
+			autofillParameters.map((p) => [p.name, true]),
+		),
 		validationSchema: useValidationSchemaForDynamicParameters(parameters),
 		enableReinitialize: false,
 		validateOnChange: true,
@@ -97,12 +100,14 @@ export const WorkspaceParametersPageViewExperimental: FC<
 			name: parameter.name,
 			value,
 		});
+		form.setFieldTouched(parameter.name, true);
 		sendDynamicParamsRequest(parameter, value);
 	};
 
 	useSyncFormParameters({
 		parameters,
 		formValues: form.values.rich_parameter_values ?? [],
+		touched: form.touched,
 		setFieldValue: form.setFieldValue,
 	});
 
@@ -201,7 +206,11 @@ export const WorkspaceParametersPageViewExperimental: FC<
 				</div>
 			)}
 
-			<form onSubmit={form.handleSubmit} className="flex flex-col gap-8">
+			<form
+				onSubmit={form.handleSubmit}
+				className="flex flex-col gap-8"
+				data-testid="form"
+			>
 				{parameters.length > 0 && (
 					<section className="flex flex-col gap-9">
 						<hgroup>

site/src/testHelpers/websockets.ts

@@ -1,4 +1,9 @@
 import type { Mock } from "vitest";
+import { API } from "#/api/api";
+import type {
+	DynamicParametersResponse,
+	PreviewParameter,
+} from "#/api/typesGenerated";
 import type { WebSocketEventType } from "#/utils/OneWayWebSocket";
 
 type SocketSendData = Parameters<WebSocket["send"]>[0];
@@ -161,3 +166,41 @@ export function createMockWebSocket(
 
 	return [mockSocket, publisher] as const;
 }
+
+export function mockDynamicParameterWebSocket(
+	response: DynamicParametersResponse | readonly PreviewParameter[],
+): readonly [MockWebSocket, MockWebSocketServer] {
+	let message: DynamicParametersResponse;
+	if (Array.isArray(response)) {
+		message = {
+			id: 0,
+			parameters: response,
+			diagnostics: [],
+		};
+	} else {
+		message = response as DynamicParametersResponse;
+	}
+	const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
+	vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation(
+		(_versionId, _ownerId, callbacks) => {
+			mockWebSocket.addEventListener("message", (event) => {
+				callbacks.onMessage(JSON.parse(event.data));
+			});
+			mockWebSocket.addEventListener("error", () => {
+				callbacks.onError(
+					new Error("Connection for dynamic parameters failed."),
+				);
+			});
+			mockWebSocket.addEventListener("close", () => {
+				callbacks.onClose();
+			});
+			mockPublisher.publishOpen(new Event("open"));
+			mockPublisher.publishMessage(
+				new MessageEvent("message", { data: JSON.stringify(message) }),
+			);
+
+			return mockWebSocket;
+		},
+	);
+	return [mockWebSocket, mockPublisher];
+}