Skip to content

Commit de95683

Browse files
rowansmithaucode-asher
rowansmithau
and
code-asher
authored
fix: do not clobber dynamic parameters (backport #24645 to 2.29) (#25828)
Backport of #24645 to `release/2.29`. 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: d958d89 Closes #23418 <details> <summary>Cherry-pick conflict resolution</summary> Two conflicts resolved: 1. **`site/src/testHelpers/websockets.ts`**: Added new `mockDynamicParameterWebSocket` helper. Converted `vi.*` to `jest.*` and `#/` to bare import paths to match release branch conventions. 2. **`site/src/pages/CreateWorkspacePage/CreateWorkspacePage.jest.tsx`**: File is `.jest.tsx` on the release branch (`.test.tsx` on main). Applied the cherry-pick's structural changes (refactored inline websocket mocks, added two new test cases) while converting `vi.*` to `jest.*` and `#/` to bare import paths. </details> > [!NOTE] > Generated with [Coder Agents](https://coder.com) by @rowansmithau Co-authored-by: Asher <ash@coder.com>
1 parent 9292a3a commit de95683

6 files changed

Lines changed: 305 additions & 135 deletions

File tree

‎site/src/modules/hooks/useSyncFormParameters.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { useEffect, useRef } from "react";
55
type UseSyncFormParametersProps = {
66
parameters: readonly PreviewParameter[];
77
formValues: readonly TypesGen.WorkspaceBuildParameter[];
8+
protectedParameters?: ReadonlySet<string>;
89
setFieldValue: (
910
field: string,
1011
value: TypesGen.WorkspaceBuildParameter[],
@@ -14,6 +15,7 @@ type UseSyncFormParametersProps = {
1415
export function useSyncFormParameters({
1516
parameters,
1617
formValues,
18+
protectedParameters,
1719
setFieldValue,
1820
}: UseSyncFormParametersProps) {
1921
// Form values only needs to be updated when parameters change
@@ -32,13 +34,12 @@ export function useSyncFormParameters({
3234
);
3335

3436
const newParameterValues = parameters.map((param) => {
35-
// When the server value is not valid (e.g., the initial
36-
// WebSocket response before any user input is sent),
37-
// preserve the current form value. This prevents the sync
38-
// hook from overwriting autofilled values (from the
39-
// previous build) with empty strings before the server
40-
// has had a chance to process them.
41-
if (!param.value.valid) {
37+
// Do not mess with values the user has changed (or were auto-filled).
38+
// Otherwise based on timing web socket responses can undo changes, and it
39+
// seems bad to change a user's inputs from under them anyway.
40+
const isProtected =
41+
!param.value.valid || protectedParameters?.has(param.name);
42+
if (isProtected) {
4243
const existingValue = currentFormValuesMap.get(param.name);
4344
if (existingValue !== undefined) {
4445
return { name: param.name, value: existingValue };
@@ -62,5 +63,5 @@ export function useSyncFormParameters({
6263
if (isChanged) {
6364
setFieldValue("rich_parameter_values", newParameterValues);
6465
}
65-
}, [parameters, setFieldValue]);
66+
}, [parameters, protectedParameters, setFieldValue]);
6667
}

‎site/src/pages/CreateWorkspacePage/CreateWorkspacePage.jest.tsx‎

Lines changed: 100 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
MockDynamicParametersResponse,
44
MockDynamicParametersResponseWithError,
55
MockPermissions,
6+
MockPreviewParameter,
67
MockSliderParameter,
78
MockTemplate,
89
MockTemplateVersionExternalAuthGithub,
@@ -15,8 +16,11 @@ import {
1516
renderWithAuth,
1617
waitForLoaderToBeRemoved,
1718
} from "testHelpers/renderHelpers";
18-
import { createMockWebSocket } from "testHelpers/websockets";
19-
import { screen, waitFor } from "@testing-library/react";
19+
import {
20+
createMockWebSocket,
21+
mockDynamicParameterWebSocket,
22+
} from "testHelpers/websockets";
23+
import { screen, waitFor, within } from "@testing-library/react";
2024
import userEvent from "@testing-library/user-event";
2125
import { API } from "api/api";
2226
import type { DynamicParametersResponse } from "api/typesGenerated";
@@ -47,33 +51,7 @@ describe("CreateWorkspacePage", () => {
4751
jest.spyOn(API, "getTemplateVersionPresets").mockResolvedValue([]);
4852
jest.spyOn(API, "createWorkspace").mockResolvedValue(MockWorkspace);
4953
jest.spyOn(API, "checkAuthorization").mockResolvedValue(MockPermissions);
50-
51-
jest
52-
.spyOn(API, "templateVersionDynamicParameters")
53-
.mockImplementation((_versionId, _ownerId, callbacks) => {
54-
const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
55-
56-
mockWebSocket.addEventListener("message", (event) => {
57-
callbacks.onMessage(JSON.parse(event.data));
58-
});
59-
mockWebSocket.addEventListener("error", () => {
60-
callbacks.onError(
61-
new Error("Connection for dynamic parameters failed."),
62-
);
63-
});
64-
mockWebSocket.addEventListener("close", () => {
65-
callbacks.onClose();
66-
});
67-
68-
publisher.publishOpen(new Event("open"));
69-
publisher.publishMessage(
70-
new MessageEvent("message", {
71-
data: JSON.stringify(MockDynamicParametersResponse),
72-
}),
73-
);
74-
75-
return mockWebSocket;
76-
});
54+
mockDynamicParameterWebSocket(MockDynamicParametersResponse);
7755
});
7856

7957
afterEach(() => {
@@ -105,32 +83,9 @@ describe("CreateWorkspacePage", () => {
10583
});
10684

10785
it("sends parameter updates via WebSocket when form values change", async () => {
108-
const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
109-
110-
jest
111-
.spyOn(API, "templateVersionDynamicParameters")
112-
.mockImplementation((_versionId, _ownerId, callbacks) => {
113-
mockWebSocket.addEventListener("message", (event) => {
114-
callbacks.onMessage(JSON.parse(event.data));
115-
});
116-
mockWebSocket.addEventListener("error", () => {
117-
callbacks.onError(
118-
new Error("Connection for dynamic parameters failed."),
119-
);
120-
});
121-
mockWebSocket.addEventListener("close", () => {
122-
callbacks.onClose();
123-
});
124-
125-
publisher.publishOpen(new Event("open"));
126-
publisher.publishMessage(
127-
new MessageEvent("message", {
128-
data: JSON.stringify(MockDynamicParametersResponse),
129-
}),
130-
);
131-
132-
return mockWebSocket;
133-
});
86+
const [mockWebSocket] = mockDynamicParameterWebSocket(
87+
MockDynamicParametersResponse,
88+
);
13489

13590
renderCreateWorkspacePage();
13691
await waitForLoaderToBeRemoved();
@@ -216,27 +171,9 @@ describe("CreateWorkspacePage", () => {
216171
});
217172

218173
it("only parameters from latest response are displayed", async () => {
219-
const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test");
220-
jest
221-
.spyOn(API, "templateVersionDynamicParameters")
222-
.mockImplementation((_versionId, _ownerId, callbacks) => {
223-
mockWebSocket.addEventListener("message", (event) => {
224-
callbacks.onMessage(JSON.parse(event.data));
225-
});
226-
227-
mockPublisher.publishOpen(new Event("open"));
228-
mockPublisher.publishMessage(
229-
new MessageEvent("message", {
230-
data: JSON.stringify({
231-
id: 0,
232-
parameters: [MockDropdownParameter],
233-
diagnostics: [],
234-
}),
235-
}),
236-
);
237-
238-
return mockWebSocket;
239-
});
174+
const [, mockPublisher] = mockDynamicParameterWebSocket([
175+
MockDropdownParameter,
176+
]);
240177

241178
renderCreateWorkspacePage();
242179
await waitForLoaderToBeRemoved();
@@ -265,27 +202,89 @@ describe("CreateWorkspacePage", () => {
265202
expect(screen.queryByText("CPU Count")).toBeInTheDocument();
266203
expect(screen.queryByText("Instance Type")).not.toBeInTheDocument();
267204
});
268-
});
269205

270-
describe("Dynamic Parameter Types", () => {
271-
it("displays parameter validation errors", async () => {
272-
jest
273-
.spyOn(API, "templateVersionDynamicParameters")
274-
.mockImplementation((_versionId, _ownerId, callbacks) => {
275-
const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
206+
it("does not clobber user values", async () => {
207+
const [, mockPublisher] = mockDynamicParameterWebSocket([
208+
MockPreviewParameter,
209+
]);
276210

277-
mockWebSocket.addEventListener("message", (event) => {
278-
callbacks.onMessage(JSON.parse(event.data));
279-
});
211+
renderCreateWorkspacePage();
212+
await waitForLoaderToBeRemoved();
280213

281-
publisher.publishMessage(
282-
new MessageEvent("message", {
283-
data: JSON.stringify(MockDynamicParametersResponseWithError),
214+
const form = screen.getByTestId("form");
215+
const input = await within(form).findByRole("textbox", {
216+
name: /parameter 1/i,
217+
});
218+
await userEvent.clear(input);
219+
await userEvent.type(input, "hi there hello");
220+
221+
await waitFor(() => {
222+
expect(
223+
within(form).getByDisplayValue("hi there hello"),
224+
).toBeInTheDocument();
225+
});
226+
227+
// Simulate a stale response.
228+
await act(async () => {
229+
mockPublisher.publishMessage(
230+
new MessageEvent("message", {
231+
data: JSON.stringify({
232+
id: 1,
233+
parameters: [MockPreviewParameter, MockValidationParameter],
284234
}),
285-
);
235+
}),
236+
);
237+
});
286238

287-
return mockWebSocket;
288-
});
239+
// Should have the new field, but keep the existing user-filled values.
240+
await waitFor(() => {
241+
expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
242+
expect(
243+
within(form).getByDisplayValue("hi there hello"),
244+
).toBeInTheDocument();
245+
});
246+
});
247+
248+
it("does not clobber auto-filled values", async () => {
249+
const [, mockPublisher] = mockDynamicParameterWebSocket([
250+
MockPreviewParameter,
251+
MockSliderParameter,
252+
]);
253+
254+
renderCreateWorkspacePage(
255+
`/templates/${MockTemplate.name}/workspace?param.cpu_count=44&param.parameter1=auto`,
256+
);
257+
await waitForLoaderToBeRemoved();
258+
259+
// Simulate a stale response.
260+
await act(async () => {
261+
mockPublisher.publishMessage(
262+
new MessageEvent("message", {
263+
data: JSON.stringify({
264+
id: 2,
265+
parameters: [
266+
MockPreviewParameter,
267+
MockSliderParameter,
268+
MockValidationParameter,
269+
],
270+
}),
271+
}),
272+
);
273+
});
274+
275+
// Should have the new field, but keep the existing auto-filled values.
276+
const form = screen.getByTestId("form");
277+
await waitFor(() => {
278+
expect(within(form).getByDisplayValue("50")).toBeInTheDocument();
279+
expect(within(form).getByDisplayValue("44")).toBeInTheDocument();
280+
expect(within(form).getByDisplayValue("auto")).toBeInTheDocument();
281+
});
282+
});
283+
});
284+
285+
describe("Dynamic Parameter Types", () => {
286+
it("displays parameter validation errors", async () => {
287+
mockDynamicParameterWebSocket(MockDynamicParametersResponseWithError);
289288

290289
renderCreateWorkspacePage();
291290
await waitForLoaderToBeRemoved();
@@ -329,38 +328,20 @@ describe("CreateWorkspacePage", () => {
329328
diagnostics: [],
330329
};
331330

332-
jest
333-
.spyOn(API, "templateVersionDynamicParameters")
334-
.mockImplementation((_versionId, _ownerId, callbacks) => {
335-
const [mockWebSocket, publisher] = createMockWebSocket("ws://test");
336-
337-
mockWebSocket.addEventListener("message", (event) => {
338-
callbacks.onMessage(JSON.parse(event.data));
339-
});
340-
341-
publisher.publishOpen(new Event("open"));
331+
const [mockWebSocket, publisher] =
332+
mockDynamicParameterWebSocket(mockResponseInitial);
333+
const originalSend = mockWebSocket.send;
334+
mockWebSocket.send = jest.fn((data) => {
335+
originalSend.call(mockWebSocket, data);
342336

337+
if (typeof data === "string" && data.includes('"200"')) {
343338
publisher.publishMessage(
344339
new MessageEvent("message", {
345-
data: JSON.stringify(mockResponseInitial),
340+
data: JSON.stringify(mockResponseWithError),
346341
}),
347342
);
348-
349-
const originalSend = mockWebSocket.send;
350-
mockWebSocket.send = jest.fn((data) => {
351-
originalSend.call(mockWebSocket, data);
352-
353-
if (typeof data === "string" && data.includes('"200"')) {
354-
publisher.publishMessage(
355-
new MessageEvent("message", {
356-
data: JSON.stringify(mockResponseWithError),
357-
}),
358-
);
359-
}
360-
});
361-
362-
return mockWebSocket;
363-
});
343+
}
344+
});
364345

365346
renderCreateWorkspacePage();
366347
await waitForLoaderToBeRemoved();

0 commit comments

Comments
 (0)