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)); });