From 744c00f45ce781949695c56e408aa9ba31fb2f70 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 16 Jun 2026 15:05:27 -0800 Subject: [PATCH] fix: handle blank dynamic parameter values consistently (#26122) There was inconsistency with what the form showed and what actually was sent to the backend. I opted to make it so that explicitly blank values are always sent rather than have blank values silently changing to the default value. --- .../DynamicParameter/DynamicParameter.tsx | 3 +- .../CreateWorkspacePage.test.tsx | 83 +++++---- .../CreateWorkspacePage.tsx | 2 +- ...rkspaceParametersPageExperimental.test.tsx | 162 ++++++++++++++++-- ...orkspaceParametersPageViewExperimental.tsx | 2 +- site/src/testHelpers/entities.ts | 41 +++++ site/src/testHelpers/parameters.ts | 23 ++- 7 files changed, 268 insertions(+), 48 deletions(-) diff --git a/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx b/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx index 9254cb29bc..c0335d4cb8 100644 --- a/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx +++ b/site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx @@ -698,7 +698,8 @@ export const getInitialParameterValues = ( ); const useAutofill = - autofillParam?.value && isValidParameterOption(parameter, autofillParam); + autofillParam?.value !== undefined && + isValidParameterOption(parameter, autofillParam); return { name: parameter.name, diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx index 633e5ab208..0b1195cb2c 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx @@ -8,7 +8,9 @@ import { MockDynamicParametersResponse, MockDynamicParametersResponseWithError, MockPermissions, - MockPreviewParameter, + MockPreviewParameter1, + MockPreviewParameter2, + MockPreviewParameter7, MockSliderParameter, MockTemplate, MockTemplateVersion, @@ -18,6 +20,7 @@ import { MockValidationParameter, MockWorkspace, } from "#/testHelpers/entities"; +import { checkParameters, editParameters } from "#/testHelpers/parameters"; import { renderWithAuth, waitForLoaderToBeRemoved, @@ -253,11 +256,12 @@ describe("CreateWorkspacePage", () => { it("does not clobber user values", async () => { const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => { publisher.publishOpen(new Event("open")); + // The initial message always has the default values. publisher.publishMessage( new MessageEvent("message", { data: JSON.stringify({ id: -1, - parameters: [MockPreviewParameter], + parameters: [MockPreviewParameter1, MockPreviewParameter7], diagnostics: [], }), }), @@ -267,26 +271,32 @@ describe("CreateWorkspacePage", () => { 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"); + // Blank out one field and fill out another. + const editedParameters = [ + // Put the blank one first to ensure we are preserving blank values and + // not just including it the first time due to the change handler. + { + name: MockPreviewParameter1.name, + value: "", + }, + { + name: MockPreviewParameter7.name, + value: "not-blank", + }, + ]; + editParameters(...editedParameters); - await waitFor(() => { - expect( - within(form).getByDisplayValue("hi there hello"), - ).toBeInTheDocument(); - }); - - // Simulate a stale response. + // Respond with different values. await act(async () => { mockPublisher.publishMessage( new MessageEvent("message", { data: JSON.stringify({ id: 2, - parameters: [MockPreviewParameter, MockValidationParameter], + parameters: [ + MockPreviewParameter1, + MockPreviewParameter2, // new field + MockPreviewParameter7, + ], diagnostics: [], }), }), @@ -294,43 +304,53 @@ describe("CreateWorkspacePage", () => { }); // 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(); - }); + await checkParameters(...editedParameters, MockPreviewParameter2); }); it("does not clobber auto-filled values", async () => { const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => { publisher.publishOpen(new Event("open")); + // The initial message always has the default values. publisher.publishMessage( new MessageEvent("message", { data: JSON.stringify({ id: -1, - parameters: [MockPreviewParameter, MockSliderParameter], + parameters: [MockPreviewParameter1, MockPreviewParameter7], diagnostics: [], }), }), ); }); + // Blank out one field and fill out another. + const editedParameters = [ + { + name: MockPreviewParameter1.name, + value: "", + }, + { + name: MockPreviewParameter7.name, + value: "not-blank", + }, + ]; + const query = editedParameters + .map((param) => `param.${param.name}=${param.value}`) + .join("&"); renderCreateWorkspacePage( - `/templates/${MockTemplate.name}/workspace?param.cpu_count=44¶m.parameter1=auto`, + `/templates/${MockTemplate.name}/workspace?${query}`, ); await waitForLoaderToBeRemoved(); - // Simulate a stale response. + // Respond with different values. await act(async () => { mockPublisher.publishMessage( new MessageEvent("message", { data: JSON.stringify({ id: 2, parameters: [ - MockPreviewParameter, - MockSliderParameter, - MockValidationParameter, + MockPreviewParameter1, + MockPreviewParameter2, // new field + MockPreviewParameter7, ], diagnostics: [], }), @@ -339,12 +359,7 @@ describe("CreateWorkspacePage", () => { }); // 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(); - }); + await checkParameters(...editedParameters, MockPreviewParameter2); }); }); diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 74a8356bcd..a6b34cb9bf 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -189,7 +189,7 @@ const CreateWorkspacePage: FC = () => { const initialParamsToSend: Record = {}; for (const param of initialFormValues) { - if (param.name && param.value) { + if (param.name && param.value !== undefined) { initialParamsToSend[param.name] = param.value; } } diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx index 85d5fd3251..9dc8315f61 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx @@ -7,13 +7,20 @@ import { MockPreviewParameter1, MockPreviewParameter2, MockPreviewParameter4, + MockPreviewParameter7, MockTemplateVersionParameter1, MockTemplateVersionParameter4, + MockTemplateVersionParameter7, MockWorkspace, MockWorkspaceBuildParameter1, MockWorkspaceBuildParameter4, + MockWorkspaceBuildParameter7, } from "#/testHelpers/entities"; -import { checkParameters } from "#/testHelpers/parameters"; +import { + checkParameters, + editParameters, + isBuildParameter, +} from "#/testHelpers/parameters"; import { renderWithWorkspaceSettingsLayout, waitForLoaderToBeRemoved, @@ -49,7 +56,11 @@ describe("WorkspaceParametersPageExperimental", () => { vi.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValueOnce([ MockTemplateVersionParameter1, // a mutable string MockTemplateVersionParameter4, // an immutable string + MockTemplateVersionParameter7, // optional string ]); + vi.spyOn(API, "postWorkspaceBuild").mockRejectedValueOnce( + new Error("not implemented"), + ); }); afterEach(() => { @@ -69,7 +80,11 @@ describe("WorkspaceParametersPageExperimental", () => { new MessageEvent("message", { data: JSON.stringify({ id: -1, - parameters: [MockPreviewParameter1, MockPreviewParameter4], + parameters: [ + MockPreviewParameter1, + MockPreviewParameter4, + MockPreviewParameter7, + ], diagnostics: [], }), }), @@ -90,12 +105,13 @@ describe("WorkspaceParametersPageExperimental", () => { const buildParameters = [ MockWorkspaceBuildParameter1, MockWorkspaceBuildParameter4, + MockWorkspaceBuildParameter7, ]; await act(async () => { resolve(buildParameters); }); - // The client's init message should include the build values. + // The client's init message should include all the build values. await waitFor(() => { expect(mockPublisher.clientSentData).toHaveLength(1); expect(JSON.parse(mockPublisher.clientSentData[0] as string)).toEqual( @@ -125,6 +141,10 @@ describe("WorkspaceParametersPageExperimental", () => { ...MockPreviewParameter4, value: { valid: true, value: MockWorkspaceBuildParameter4.value }, }, + { + ...MockPreviewParameter7, + value: { valid: true, value: MockWorkspaceBuildParameter7.value }, + }, ], diagnostics: [], }), @@ -136,6 +156,7 @@ describe("WorkspaceParametersPageExperimental", () => { await checkParameters( MockWorkspaceBuildParameter1, MockWorkspaceBuildParameter4, + MockWorkspaceBuildParameter7, ); // The submit button should be enabled. @@ -158,7 +179,11 @@ describe("WorkspaceParametersPageExperimental", () => { new MessageEvent("message", { data: JSON.stringify({ id: -1, - parameters: [MockPreviewParameter1, MockPreviewParameter4], + parameters: [ + MockPreviewParameter1, + MockPreviewParameter4, + MockPreviewParameter7, + ], diagnostics: [], }), }), @@ -183,7 +208,11 @@ describe("WorkspaceParametersPageExperimental", () => { // Since there are no build values, the page is rendered with defaults and // the client does not need to send anything. await waitForLoaderToBeRemoved(); - await checkParameters(MockPreviewParameter1, MockPreviewParameter4); + await checkParameters( + MockPreviewParameter1, + MockPreviewParameter4, + MockPreviewParameter7, + ); expect(mockPublisher.clientSentData).toHaveLength(0); // The submit button should be enabled. @@ -194,11 +223,16 @@ describe("WorkspaceParametersPageExperimental", () => { await waitFor(() => expect(submitButton).toBeEnabled()); }); - it("does not clobber touched parameters", async () => { - vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([ + it("does not clobber build parameters", async () => { + const buildParameters = [ MockWorkspaceBuildParameter1, MockWorkspaceBuildParameter4, - ]); + MockWorkspaceBuildParameter7, + ]; + + vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce( + buildParameters, + ); const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => { publisher.publishOpen(new Event("open")); @@ -207,7 +241,11 @@ describe("WorkspaceParametersPageExperimental", () => { new MessageEvent("message", { data: JSON.stringify({ id: -1, - parameters: [MockPreviewParameter1, MockPreviewParameter4], + parameters: [ + MockPreviewParameter1, + MockPreviewParameter4, + MockPreviewParameter7, + ], diagnostics: [], }), }), @@ -219,6 +257,14 @@ describe("WorkspaceParametersPageExperimental", () => { // Wait for the client's init message then respond with different values. await waitFor(() => { expect(mockPublisher.clientSentData).toHaveLength(1); + expect(JSON.parse(mockPublisher.clientSentData[0] as string)).toEqual( + expect.objectContaining({ + id: 0, + inputs: Object.fromEntries( + buildParameters.map((p) => [p.name, p.value]), + ), + }), + ); }); mockPublisher.publishMessage( @@ -227,8 +273,9 @@ describe("WorkspaceParametersPageExperimental", () => { id: 0, parameters: [ MockPreviewParameter1, - MockPreviewParameter2, + MockPreviewParameter2, // new field MockPreviewParameter4, + MockPreviewParameter7, ], diagnostics: [], }), @@ -241,6 +288,101 @@ describe("WorkspaceParametersPageExperimental", () => { await checkParameters( MockWorkspaceBuildParameter1, MockWorkspaceBuildParameter4, + MockWorkspaceBuildParameter7, + MockPreviewParameter2, + ); + + // However the submit button should be disabled because the state + // mismatches. + const form = screen.getByTestId("form"); + const submitButton = within(form).getByRole("button", { + name: /update and restart/i, + }); + await waitFor(() => expect(submitButton).toBeDisabled()); + }); + + it("does not clobber edited parameters", async () => { + vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([]); + + const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => { + publisher.publishOpen(new Event("open")); + // The initial message always has the default values. + publisher.publishMessage( + new MessageEvent("message", { + data: JSON.stringify({ + id: -1, + parameters: [ + MockPreviewParameter1, + MockPreviewParameter4, + MockPreviewParameter7, + ], + diagnostics: [], + }), + }), + ); + }); + + renderWorkspaceParametersPageExperimental(); + + // Page should render with the default values. + await waitForLoaderToBeRemoved(); + await checkParameters( + MockPreviewParameter1, + MockPreviewParameter4, + MockPreviewParameter7, + ); + + // Blank out one field and fill out another. + const editedParameters = [ + // Put the blank one first to ensure we are preserving blank values and + // not just including it the first time due to the change handler. + { + name: MockPreviewParameter1.name, + value: "", + }, + { + name: MockPreviewParameter7.name, + value: "not-blank", + }, + ]; + editParameters(...editedParameters); + + // The client should now send all parameters. + await waitFor(() => { + expect(mockPublisher.clientSentData).toHaveLength(1); + expect(JSON.parse(mockPublisher.clientSentData[0] as string)).toEqual( + expect.objectContaining({ + id: 0, + inputs: Object.fromEntries( + [...editedParameters, MockPreviewParameter4].map((p) => [ + p.name, + isBuildParameter(p) ? p.value : p.value.value, + ]), + ), + }), + ); + }); + + // Respond with different values. + mockPublisher.publishMessage( + new MessageEvent("message", { + data: JSON.stringify({ + id: 0, + parameters: [ + MockPreviewParameter1, + MockPreviewParameter2, // new field + MockPreviewParameter4, + MockPreviewParameter7, + ], + diagnostics: [], + }), + }), + ); + + // The form should keep the user's values but include the new field. + await checkParameters( + ...editedParameters, + MockPreviewParameter4, MockPreviewParameter2, ); diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageViewExperimental.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageViewExperimental.tsx index b35c4218cb..86234c2e70 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageViewExperimental.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageViewExperimental.tsx @@ -81,7 +81,7 @@ export const WorkspaceParametersPageViewExperimental: FC< const formInputs: Record = {}; const formParameters = form.values.rich_parameter_values ?? []; for (const param of formParameters) { - if (param?.name && param?.value) { + if (param?.name && param?.value !== undefined) { formInputs[param.name] = param.value; } } diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 15c20b683d..310cc1d1ef 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1947,6 +1947,25 @@ export const MockTemplateVersionParameter6: TypesGen.TemplateVersionParameter = ephemeral: true, }; +// Not required and the default is a blank string. +export const MockTemplateVersionParameter7: TypesGen.TemplateVersionParameter = + { + name: "seventh_parameter", + type: "string", + form_type: "input", + description: "This is seventh parameter", + description_plaintext: "Markdown: This is seventh parameter", + default_value: "", + mutable: true, + icon: "/icon/folder.svg", + options: [], + validation_min: 1, + validation_max: 10, + validation_monotonic: "decreasing", + required: false, + ephemeral: false, + }; + export const MockTemplateVersionVariable1: TypesGen.TemplateVersionVariable = { name: "first_variable", description: "This is first variable.", @@ -3424,6 +3443,12 @@ export const MockWorkspaceBuildParameter5: TypesGen.WorkspaceBuildParameter = { value: "5", }; +// Has a blank value. +export const MockWorkspaceBuildParameter7: TypesGen.WorkspaceBuildParameter = { + name: MockTemplateVersionParameter7.name, + value: "", +}; + export const MockPreviewParameter: TypesGen.PreviewParameter = { name: "parameter1", display_name: "Parameter 1", @@ -3483,6 +3508,22 @@ export const MockPreviewParameter4: TypesGen.PreviewParameter = { mutable: false, }; +// A text parameter that is mutable, not required, and has a blank value. Maps +// to MockTemplateVersionParameter7. +export const MockPreviewParameter7: TypesGen.PreviewParameter = { + ...MockPreviewParameter, + name: MockTemplateVersionParameter7.name, + display_name: MockTemplateVersionParameter7.name, + default_value: { + valid: true, + value: MockTemplateVersionParameter7.default_value, + }, + value: { valid: true, value: MockTemplateVersionParameter7.default_value }, + required: MockTemplateVersionParameter7.required, + mutable: MockTemplateVersionParameter7.mutable, + ephemeral: MockTemplateVersionParameter7.ephemeral, +}; + export const MockDropdownParameter: TypesGen.PreviewParameter = { ...MockPreviewParameter, name: "instance_type", diff --git a/site/src/testHelpers/parameters.ts b/site/src/testHelpers/parameters.ts index 274ff20995..f3699e2dec 100644 --- a/site/src/testHelpers/parameters.ts +++ b/site/src/testHelpers/parameters.ts @@ -1,9 +1,10 @@ import { screen, waitFor, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; import type * as TypesGen from "#/api/typesGenerated"; type Parameter = TypesGen.WorkspaceBuildParameter | TypesGen.PreviewParameter; -function isBuildParameter( +export function isBuildParameter( parameter: Parameter, ): parameter is TypesGen.WorkspaceBuildParameter { return typeof parameter.value === "string"; @@ -26,3 +27,23 @@ export async function checkParameters(...parameters: Parameter[]) { } }); } + +// editParameters edits each parameter so it has the provided value. Requires +// that the form and parameters all have test IDs (`form` and +// `parameter-field-$name`). +export async function editParameters(...parameters: Parameter[]) { + const form = screen.getByTestId("form"); + for (const parameter of parameters) { + const field = within(form).getByTestId(`parameter-field-${parameter.name}`); + const input = await within(field).findByRole("textbox", { + name: new RegExp(parameter.name, "i"), + }); + await userEvent.clear(input); + const value = isBuildParameter(parameter) + ? parameter.value + : parameter.value.value; + if (value !== "") { + await userEvent.type(input, value); + } + } +}