From b5b80dbe8e683300a2f05b7ca922e27c5e098edc Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 15 Jun 2026 16:38:54 -0800 Subject: [PATCH] fix: do not send or use stale init dynamic parameter state (#26357) Previously, there could be a gap where the web socket has been connected and gets the initial message but the build parameters request was still in flight. This caused two issues: 1. Because we only send initial parameters as a response to a message, when the message comes first and the build parameters have not resolved yet, we end up not sending the initial parameters, meaning the form could be stale until the next edit the user makes. 2. And if the user does make an edit, once we get that response back we would then send the initial parameters, essentially reverting back to the initial state since the initial params do not include the user's edits. So the user would need a second edit to finally sync up. To resolve both issues, we ignore the web socket's initial message until we get the build parameters, at which point we decide whether we can use that initial message (when there are no build params) or if we need to continue ignoring it and send the initial parameters to get the correct state then finally render the form. --- site/src/@types/storybook.d.ts | 2 +- site/src/api/api.ts | 6 + ...paceParametersPageExperimental.stories.tsx | 9 + ...rkspaceParametersPageExperimental.test.tsx | 215 +++++++++++++++--- .../WorkspaceParametersPageExperimental.tsx | 101 ++++---- site/src/testHelpers/entities.ts | 40 ++++ site/src/testHelpers/parameters.ts | 28 +++ site/src/testHelpers/storybook.tsx | 1 + site/src/testHelpers/websockets.ts | 3 + 9 files changed, 326 insertions(+), 79 deletions(-) create mode 100644 site/src/testHelpers/parameters.ts diff --git a/site/src/@types/storybook.d.ts b/site/src/@types/storybook.d.ts index ba17103c42..76166ba53c 100644 --- a/site/src/@types/storybook.d.ts +++ b/site/src/@types/storybook.d.ts @@ -13,7 +13,7 @@ import type { ReactRouterAddonStoryParameters } from "storybook-addon-remix-reac declare module "@storybook/react-vite" { type WebSocketEvent = | { event: "message"; data: string } - | { event: "error" | "close" }; + | { event: "open" | "error" | "close" }; interface Parameters { features?: FeatureName[]; experiments?: Experiments; diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 3580a85992..6de3dde1bc 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1165,10 +1165,12 @@ class ApiMethods { versionId: string, userId: string, { + onOpen, onMessage, onError, onClose, }: { + onOpen?: () => void; onMessage: (response: TypesGen.DynamicParametersResponse) => void; onError: (error: Error) => void; onClose: () => void; @@ -1179,6 +1181,10 @@ class ApiMethods { new URLSearchParams({ user_id: userId }), ); + socket.addEventListener("open", () => { + onOpen?.(); + }); + socket.addEventListener("message", (event) => onMessage(JSON.parse(event.data) as TypesGen.DynamicParametersResponse), ); diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.stories.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.stories.tsx index d1268fea2d..9ff0a6b39c 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.stories.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.stories.tsx @@ -48,6 +48,9 @@ const meta = { reactRouter: workspaceRouterParameters(MockWorkspace), queries: workspaceQueries(MockWorkspace), webSocket: [ + { + event: "open", + }, { event: "message", data: JSON.stringify({ @@ -66,6 +69,9 @@ type Story = StoryObj; export const NoParameters: Story = { parameters: { webSocket: [ + { + event: "open", + }, { event: "message", data: JSON.stringify({ @@ -200,6 +206,9 @@ function workspaceQueries(workspace: Workspace) { function filledWebSocketParams(): WebSocketEvent[] { return [ + { + event: "open", + }, { event: "message", data: JSON.stringify({ diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx index 2332d8cabe..85d5fd3251 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.test.tsx @@ -1,17 +1,19 @@ import { screen, waitFor, within } from "@testing-library/react"; import { act } from "react"; import { API } from "#/api/api"; +import type * as TypesGen from "#/api/typesGenerated"; +import { createDeferred } from "#/testHelpers/deferred"; import { - MockPreviewParameter, + MockPreviewParameter1, + MockPreviewParameter2, + MockPreviewParameter4, MockTemplateVersionParameter1, - MockTemplateVersionParameter2, MockTemplateVersionParameter4, - MockValidationParameter, MockWorkspace, MockWorkspaceBuildParameter1, - MockWorkspaceBuildParameter2, MockWorkspaceBuildParameter4, } from "#/testHelpers/entities"; +import { checkParameters } from "#/testHelpers/parameters"; import { renderWithWorkspaceSettingsLayout, waitForLoaderToBeRemoved, @@ -45,14 +47,8 @@ describe("WorkspaceParametersPageExperimental", () => { MockWorkspace, ); vi.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValueOnce([ - MockTemplateVersionParameter1, - MockTemplateVersionParameter2, - MockTemplateVersionParameter4, - ]); - vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([ - MockWorkspaceBuildParameter1, - MockWorkspaceBuildParameter2, - MockWorkspaceBuildParameter4, + MockTemplateVersionParameter1, // a mutable string + MockTemplateVersionParameter4, // an immutable string ]); }); @@ -61,19 +57,19 @@ describe("WorkspaceParametersPageExperimental", () => { vi.restoreAllMocks(); }); - it("does not clobber touched parameters", async () => { - const [, mockPublisher] = mockDynamicParameterWebSocket((publisher) => { + it("waits for and sends initial build parameters", async () => { + const { promise, resolve } = + createDeferred(); + vi.spyOn(API, "getWorkspaceBuildParameters").mockReturnValueOnce(promise); + + 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, - name: MockWorkspaceBuildParameter1.name, - }, - ], + parameters: [MockPreviewParameter1, MockPreviewParameter4], diagnostics: [], }), }), @@ -81,34 +77,179 @@ describe("WorkspaceParametersPageExperimental", () => { }); renderWorkspaceParametersPageExperimental(); - await waitForLoaderToBeRemoved(); - // Simulate a stale response. + // Wait for both requests to have been made. Client should not have sent + // any message yet since build parameters have not resolved. + await waitFor(() => { + expect(API.getWorkspaceBuildParameters).toHaveBeenCalled(); + expect(API.templateVersionDynamicParameters).toHaveBeenCalled(); + expect(mockPublisher.clientSentData).toHaveLength(0); + }); + + // Build parameters now resolve. + const buildParameters = [ + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter4, + ]; await act(async () => { - mockPublisher.publishMessage( + resolve(buildParameters); + }); + + // The client's init message should include the build 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]), + ), + }), + ); + }); + + // Should still be waiting for the response. + expect(screen.queryByTestId("loader")).toBeInTheDocument(); + + // Respond to the init message with up-to-date values. + mockPublisher.publishMessage( + new MessageEvent("message", { + data: JSON.stringify({ + id: 0, + parameters: [ + { + ...MockPreviewParameter1, + value: { valid: true, value: MockWorkspaceBuildParameter1.value }, + }, + { + ...MockPreviewParameter4, + value: { valid: true, value: MockWorkspaceBuildParameter4.value }, + }, + ], + diagnostics: [], + }), + }), + ); + + // Finally the page is rendered with the build values. + await waitForLoaderToBeRemoved(); + await checkParameters( + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter4, + ); + + // The submit button should be enabled. + const form = screen.getByTestId("form"); + const submitButton = within(form).getByRole("button", { + name: /update and restart/i, + }); + await waitFor(() => expect(submitButton).toBeEnabled()); + }); + + it("skips zero-length initial parameters", async () => { + const { promise, resolve } = + createDeferred(); + vi.spyOn(API, "getWorkspaceBuildParameters").mockReturnValueOnce(promise); + + 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: 2, - parameters: [ - { - ...MockPreviewParameter, - name: MockWorkspaceBuildParameter1.name, - }, - MockValidationParameter, - ], + id: -1, + parameters: [MockPreviewParameter1, MockPreviewParameter4], diagnostics: [], }), }), ); }); - // Should have the new field, but keep the existing auto-filled values. - const form = screen.getByTestId("form"); + renderWorkspaceParametersPageExperimental(); + + // Wait for both requests to have been made. Client should not have sent + // any message yet since build parameters have not resolved. await waitFor(() => { - expect(within(form).getByDisplayValue("50")).toBeInTheDocument(); - expect( - within(form).getByDisplayValue(MockWorkspaceBuildParameter1.value), - ).toBeInTheDocument(); + expect(API.getWorkspaceBuildParameters).toHaveBeenCalled(); + expect(API.templateVersionDynamicParameters).toHaveBeenCalled(); + expect(mockPublisher.clientSentData).toHaveLength(0); }); + + // Build parameters now resolve. + await act(async () => { + resolve([]); + }); + + // 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); + expect(mockPublisher.clientSentData).toHaveLength(0); + + // The submit button should be enabled. + const form = screen.getByTestId("form"); + const submitButton = within(form).getByRole("button", { + name: /update and restart/i, + }); + await waitFor(() => expect(submitButton).toBeEnabled()); + }); + + it("does not clobber touched parameters", async () => { + vi.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValueOnce([ + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter4, + ]); + + 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], + diagnostics: [], + }), + }), + ); + }); + + renderWorkspaceParametersPageExperimental(); + + // Wait for the client's init message then respond with different values. + await waitFor(() => { + expect(mockPublisher.clientSentData).toHaveLength(1); + }); + + mockPublisher.publishMessage( + new MessageEvent("message", { + data: JSON.stringify({ + id: 0, + parameters: [ + MockPreviewParameter1, + MockPreviewParameter2, + MockPreviewParameter4, + ], + diagnostics: [], + }), + }), + ); + + // Page should render with the build values, but the new field that was not + // part of the previous build should also show up. + await waitForLoaderToBeRemoved(); + await checkParameters( + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter4, + 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()); }); }); diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx index 35e44d9958..7dd35166f6 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx @@ -49,66 +49,73 @@ const WorkspaceParametersPageExperimental: FC = () => { const [latestResponse, setLatestResponse] = useState(null); + // The current expected response ID. Starts at -1 because the backend sends + // an initial message when the web socket is connected with -1. const wsResponseId = useRef(-1); const ws = useRef(null); const [wsError, setWsError] = useState(null); - const initialParamsSentRef = useRef(false); + // The expected ID of the init message, so we can wait until the initial + // parameters have gone through before rendering the form. + const [initId, setInitId] = useState(Number.NaN); + // Parameters from the latest build, formatted as auto-fill parameters. const autofillParameters: AutofillBuildParameter[] = latestBuildParameters?.map((p) => ({ ...p, source: "active_build", })) ?? []; - const sendMessage = (formValues: Record) => { + // sendMessage increments the ID and sends the form values on the web socket + // and return true. If the socket is not open, it does not increment the ID + // and returns false. + const sendMessage = (formValues: Record): boolean => { const request: DynamicParametersRequest = { id: wsResponseId.current + 1, owner_id: workspace.owner_id, inputs: formValues, }; if (ws.current && ws.current.readyState === WebSocket.OPEN) { - ws.current.send(JSON.stringify(request)); wsResponseId.current = wsResponseId.current + 1; + ws.current.send(JSON.stringify(request)); + return true; } + if (ws.current) { + console.error( + "Tried to send message but the web socket state is %s", + ws.current.readyState, + request, + ); + } + return false; }; - // On page load, sends initial workspace build parameters to the websocket. - // This ensures the backend has the form's complete initial state, - // vital for rendering dynamic UI elements dependent on initial parameter values. + // Send the initial parameters if necessary and mark the ID of the response we + // need to wait for until we can finally render the form with the right state. const sendInitialParameters = useEffectEvent(() => { - if (initialParamsSentRef.current) return; - if (autofillParameters.length === 0) return; - - const initialParamsToSend: Record = {}; - for (const param of autofillParameters) { - if (param.name && param.value) { - initialParamsToSend[param.name] = param.value; + if (latestBuildParametersLoading || !Number.isNaN(initId)) { + return; + } + if (autofillParameters.length > 0) { + const values = Object.fromEntries( + autofillParameters.map((afp) => [afp.name, afp.value]), + ); + if (!sendMessage(values)) { + return; } } - if (Object.keys(initialParamsToSend).length === 0) return; - - sendMessage(initialParamsToSend); - initialParamsSentRef.current = true; + // If there were no parameters to send, this will end up just using the + // response we already have. Otherwise it will wait for the next response. + setInitId(wsResponseId.current); }); - const onMessage = useEffectEvent((response: DynamicParametersResponse) => { - if (latestResponse && latestResponse?.id >= response.id) { - return; - } - - // Skip stale responses. If we've already sent a newer request, - // this response contains outdated parameter values that would - // overwrite the user's more recent input. - if (response.id < wsResponseId.current) { - return; - } - - setLatestResponse(response); - - if (!initialParamsSentRef.current && response.parameters?.length > 0) { + // Send the build parameters once we get them. + useEffect(() => { + // sendInitialParameters already makes this check but the linter complains + // if the dependency is not used. + if (!latestBuildParametersLoading) { sendInitialParameters(); } - }); + }, [latestBuildParametersLoading]); useEffect(() => { if (!templateVersionId && !workspace.latest_build.template_version_id) @@ -118,7 +125,17 @@ const WorkspaceParametersPageExperimental: FC = () => { templateVersionId ?? workspace.latest_build.template_version_id, workspace.owner_id, { - onMessage, + onOpen: () => { + // If we already have the build parameters, send them now. + sendInitialParameters(); + }, + // Record the latest message every time we get one from the web + // socket. Stale responses are discarded. + onMessage: (response: DynamicParametersResponse) => { + if (response.id >= wsResponseId.current) { + setLatestResponse(response); + } + }, onError: (error) => { if (ws.current === socket) { setWsError(error); @@ -226,13 +243,13 @@ const WorkspaceParametersPageExperimental: FC = () => { const error = wsError || startWithParameters.error || restartWithParameters.error; - if ( + // Some of these checks conceptually overlap, but opting to be explicit. + const isLoading = latestBuildParametersLoading || - (!latestResponse && !wsError) || - (ws.current && ws.current.readyState === WebSocket.CONNECTING) - ) { - return ; - } + !latestResponse || + Number.isNaN(initId) || + latestResponse.id < initId || + (ws.current && ws.current.readyState === WebSocket.CONNECTING); let submitLabel = "Update and start"; if (restartWithParameters.isPending) { @@ -277,7 +294,9 @@ const WorkspaceParametersPageExperimental: FC = () => { {Boolean(error) && } - {sortedParams.length > 0 ? ( + {isLoading ? ( + + ) : sortedParams.length > 0 ? ( { + for (const parameter of parameters) { + const field = within(form).getByTestId( + `parameter-field-${parameter.name}`, + ); + const value = isBuildParameter(parameter) + ? parameter.value + : parameter.value.value; + expect(within(field).getByDisplayValue(value)).toBeInTheDocument(); + } + }); +} diff --git a/site/src/testHelpers/storybook.tsx b/site/src/testHelpers/storybook.tsx index bea5436738..6a885776d1 100644 --- a/site/src/testHelpers/storybook.tsx +++ b/site/src/testHelpers/storybook.tsx @@ -100,6 +100,7 @@ export const withWebSocket = (Story: FC, { parameters }: StoryContext) => { window.WebSocket = class WebSocket { public readyState = 1; public binaryType = "blob"; + static OPEN = 1; #listeners = new Map(); #callEventsDelay: number | undefined; diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts index 5c2318d798..137fafeeea 100644 --- a/site/src/testHelpers/websockets.ts +++ b/site/src/testHelpers/websockets.ts @@ -169,6 +169,9 @@ export function mockDynamicParameterWebSocket( const [mockWebSocket, mockPublisher] = createMockWebSocket("ws://test"); vi.spyOn(API, "templateVersionDynamicParameters").mockImplementation( (_versionId, _ownerId, callbacks) => { + mockWebSocket.addEventListener("open", () => { + callbacks.onOpen?.(); + }); mockWebSocket.addEventListener("message", (event) => { callbacks.onMessage(JSON.parse(event.data)); });