fix: do not clobber dynamic parameters (backport #24645 to 2.32) (#25827)

Backport of https://github.com/coder/coder/pull/24645 to `release/2.32`.

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).

Original PR: #24645 — fix: do not clobber dynamic parameters
Merge commit: d958d89b6f

Closes #23418

<details>
<summary>Cherry-pick conflict resolution</summary>

Two conflicts resolved:

1. **`site/src/testHelpers/websockets.ts`**: File was empty on
release/2.32. Took the incoming version with the new
`mockDynamicParameterWebSocket` helper.

2.
**`site/src/pages/CreateWorkspacePage/CreateWorkspacePage.jest.tsx`**:
File is `.jest.tsx` on the release branch (`.test.tsx` on main). Applied
the incoming content (refactored websocket mocking, modernized test
calls) to the existing `.jest.tsx` filename.

</details>

> [!NOTE]
> Generated with [Coder Agents](https://coder.com) by @rowansmithau

---------

Co-authored-by: Asher <ash@coder.com>
Co-authored-by: Garrett Delfosse <delfossegarrett@gmail.com>
This commit is contained in:
Rowan Smith
2026-05-30 09:44:44 +10:00
committed by GitHub
parent e4defea43c
commit 9614d55400
6 changed files with 381 additions and 183 deletions
@@ -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]);
}
@@ -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,36 +48,11 @@ describe("CreateWorkspacePage", () => {
jest.spyOn(API, "getTemplateVersionPresets").mockResolvedValue([]);
jest.spyOn(API, "createWorkspace").mockResolvedValue(MockWorkspace);
jest.spyOn(API, "checkAuthorization").mockResolvedValue(MockPermissions);
jest
.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(() => {
jest.useRealTimers();
jest.restoreAllMocks();
});
@@ -105,32 +81,9 @@ describe("CreateWorkspacePage", () => {
});
it("sends parameter updates via WebSocket when form values change", async () => {
const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
jest
.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();
await waitForLoaderToBeRemoved();
@@ -150,18 +103,16 @@ describe("CreateWorkspacePage", () => {
await userEvent.click(instanceTypeSelect);
});
let mediumOption: Element | null = null;
await waitFor(() => {
mediumOption = screen.queryByRole("option", { name: /t3\.medium/i });
expect(mediumOption).toBeTruthy();
const mediumOption = await screen.findByRole("option", {
name: /t3\.medium/i,
});
await waitFor(async () => {
await userEvent.click(mediumOption!);
await userEvent.click(mediumOption);
});
act(() => {
jest.runAllTimers();
await act(async () => {
await jest.runAllTimersAsync();
});
expect(mockWebSocket.send).toHaveBeenCalledWith(
@@ -172,48 +123,42 @@ describe("CreateWorkspacePage", () => {
});
it("handles WebSocket error gracefully", async () => {
const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
jest
.spyOn(API, "templateVersionDynamicParameters")
.mockImplementation((_versionId, _ownerId, callbacks) => {
mockWebSocket.addEventListener("error", () => {
callbacks.onError(new Error("Connection failed"));
});
return mockWebSocket;
});
const [, mockPublisher] = mockDynamicParameterWebSocket([]);
renderCreateWorkspacePage();
await waitFor(() => {
expect(mockPublisher).toBeDefined();
expect(API.templateVersionDynamicParameters).toHaveBeenCalled();
});
await act(async () => {
mockPublisher.publishError(new Event("Connection failed"));
});
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");
jest
.spyOn(API, "templateVersionDynamicParameters")
.mockImplementation((_versionId, _ownerId, callbacks) => {
mockWebSocket.addEventListener("close", () => {
callbacks.onClose();
});
return mockWebSocket;
});
const [, mockPublisher] = mockDynamicParameterWebSocket([]);
renderCreateWorkspacePage();
await waitFor(() => {
expect(mockPublisher).toBeDefined();
expect(API.templateVersionDynamicParameters).toHaveBeenCalled();
});
await act(async () => {
mockPublisher.publishClose(new Event("close") as CloseEvent);
});
await waitFor(() => {
const alert = screen.getByRole("alert");
expect(
within(alert).getByRole("heading", {
@@ -224,27 +169,9 @@ describe("CreateWorkspacePage", () => {
});
it("only parameters from latest response are displayed", async () => {
const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
jest
.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();
@@ -260,7 +187,7 @@ describe("CreateWorkspacePage", () => {
diagnostics: [],
};
await waitFor(() => {
await act(async () => {
mockPublisher.publishMessage(
new MessageEvent("message", { data: JSON.stringify(response1) }),
);
@@ -270,30 +197,94 @@ describe("CreateWorkspacePage", () => {
);
});
expect(screen.queryByText("CPU Count")).toBeInTheDocument();
expect(screen.queryByText("Instance Type")).not.toBeInTheDocument();
await waitFor(() => {
expect(screen.queryByText("CPU Count")).toBeInTheDocument();
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 () => {
jest
.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();
@@ -337,38 +328,20 @@ describe("CreateWorkspacePage", () => {
diagnostics: [],
};
jest
.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 = jest.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 = jest.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();
@@ -380,10 +353,8 @@ describe("CreateWorkspacePage", () => {
const numberInput = screen.getByDisplayValue("50");
expect(numberInput).toBeInTheDocument();
await waitFor(async () => {
await userEvent.clear(numberInput);
await userEvent.type(numberInput, "200");
});
await userEvent.clear(numberInput);
await userEvent.type(numberInput, "200");
await waitFor(() => {
expect(screen.getByDisplayValue("200")).toBeInTheDocument();
@@ -512,17 +483,13 @@ describe("CreateWorkspacePage", () => {
const nameInput = screen.getByRole("textbox", {
name: /workspace name/i,
});
await waitFor(async () => {
await userEvent.clear(nameInput);
await userEvent.type(nameInput, "my-test-workspace");
});
await userEvent.clear(nameInput);
await userEvent.type(nameInput, "my-test-workspace");
const createButton = screen.getByRole("button", {
name: /create workspace/i,
});
await waitFor(async () => {
await userEvent.click(createButton);
});
await userEvent.click(createButton);
await waitFor(() => {
expect(API.createWorkspace).toHaveBeenCalledWith(
@@ -600,18 +567,13 @@ describe("CreateWorkspacePage", () => {
name: /workspace name/i,
});
await waitFor(async () => {
await userEvent.clear(nameInput);
await userEvent.type(nameInput, "my-test-workspace");
});
await userEvent.clear(nameInput);
await userEvent.type(nameInput, "my-test-workspace");
// Submit form
const createButton = screen.getByRole("button", {
name: /create workspace/i,
});
await waitFor(async () => {
await userEvent.click(createButton);
});
await userEvent.click(createButton);
await waitFor(() => {
expect(router.state.location.pathname).toBe(
@@ -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,
});
@@ -445,6 +448,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} />}
@@ -0,0 +1,172 @@
import { screen, waitFor, within } from "@testing-library/react";
import { act } from "react";
import { API } from "#/api/api";
import type { DynamicParametersResponse } from "#/api/typesGenerated";
import {
MockPreviewParameter,
MockTemplateVersionParameter1,
MockTemplateVersionParameter2,
MockTemplateVersionParameter4,
MockValidationParameter,
MockWorkspace,
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter2,
MockWorkspaceBuildParameter4,
} from "#/testHelpers/entities";
import {
renderWithWorkspaceSettingsLayout,
waitForLoaderToBeRemoved,
} from "#/testHelpers/renderHelpers";
import WorkspaceParametersPageExperimental from "./WorkspaceParametersPageExperimental";
// Minimal inline publisher for vitest. The shared websockets.ts helper
// uses jest globals and cannot be imported from vitest on this branch.
type Publisher = {
publishMessage: (event: MessageEvent<string>) => void;
publishOpen: (event: Event) => void;
};
function setupDynamicParameterWebSocket(
response: DynamicParametersResponse,
): Publisher {
const subs: Record<string, Set<(e: unknown) => void>> = {
message: new Set(),
error: new Set(),
close: new Set(),
open: new Set(),
};
const fakeSocket = {
addEventListener: (type: string, cb: (e: unknown) => void) => {
subs[type]?.add(cb);
},
removeEventListener: (type: string, cb: (e: unknown) => void) => {
subs[type]?.delete(cb);
},
close: jest.fn(),
};
const publisher: Publisher = {
publishOpen: (event) => {
for (const sub of subs.open) sub(event);
},
publishMessage: (event) => {
for (const sub of subs.message) sub(event);
},
};
jest
.spyOn(API, "templateVersionDynamicParameters")
.mockImplementation((_versionId, _ownerId, callbacks) => {
fakeSocket.addEventListener("message", (event) => {
callbacks.onMessage(JSON.parse((event as MessageEvent<string>).data));
});
fakeSocket.addEventListener("error", () => {
callbacks.onError(
new Error("Connection for dynamic parameters failed."),
);
});
fakeSocket.addEventListener("close", () => {
callbacks.onClose();
});
publisher.publishOpen(new Event("open"));
publisher.publishMessage(
new MessageEvent("message", {
data: JSON.stringify(response),
}),
);
return fakeSocket as unknown as WebSocket;
});
return publisher;
}
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(() => {
jest.clearAllMocks();
jest
.spyOn(API, "getWorkspaceByOwnerAndName")
.mockResolvedValueOnce(MockWorkspace);
jest
.spyOn(API, "getTemplateVersionRichParameters")
.mockResolvedValueOnce([
MockTemplateVersionParameter1,
MockTemplateVersionParameter2,
MockTemplateVersionParameter4,
]);
jest
.spyOn(API, "getWorkspaceBuildParameters")
.mockResolvedValueOnce([
MockWorkspaceBuildParameter1,
MockWorkspaceBuildParameter2,
MockWorkspaceBuildParameter4,
]);
});
afterEach(() => {
jest.useRealTimers();
jest.restoreAllMocks();
});
it("does not clobber touched parameters", async () => {
const publisher = setupDynamicParameterWebSocket({
id: 0,
parameters: [
{
...MockPreviewParameter,
name: MockWorkspaceBuildParameter1.name,
},
],
diagnostics: [],
});
renderWorkspaceParametersPageExperimental();
await waitForLoaderToBeRemoved();
// Simulate a stale response.
await act(async () => {
publisher.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();
});
});
});
@@ -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>
+43
View File
@@ -1,3 +1,8 @@
import { API } from "#/api/api";
import type {
DynamicParametersResponse,
PreviewParameter,
} from "#/api/typesGenerated";
import type { WebSocketEventType } from "#/utils/OneWayWebSocket";
type SocketSendData = Parameters<WebSocket["send"]>[0];
@@ -160,3 +165,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");
jest
.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];
}