opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-09-24 15:04:27 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: do not send or use stale init dynamic parameter state (#27283)

Browse Source 
In summary, we use the init message if we have no autofill params. If we
do, then we ignore the init message, send another message with the
autofilled values, and then when we get *that* response, finally we
render the form since we know we have good state. This eliminates the
possibility of temporarily rendering with stale state.

This is the same fix that was applied to the edit page, but on the
create page this time. The only difference is that the create page does
not need to wait on a build parameters query, instead it has to wait on
the first message from the socket (to get defaults).

This also makes one change where we would send the defaults in the init
message. The server already knows the defaults so there is no need to
send them. The advantage here is that we no longer need to wait for the
first message, and it also fixes an issue where fields with blank values
were getting validation errors because they were not filled out, before
the user had a chance to actually fill them out.
This commit is contained in:
Asher
2026-07-16 12:55:43 -08:00
committed by GitHub
parent 1453b4b1fc
commit 2ac2295b1e
7 changed files with 539 additions and 420 deletions
Download Patch File Download Diff File Expand all files Collapse all files
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx
+1 -1
View File
@@ -677,7 +677,7 @@ const ParameterDiagnostics: FC<ParameterDiagnosticsProps> = ({
};
export const getInitialParameterValues = (
params: PreviewParameter[],
params: readonly PreviewParameter[],
autofillParams?: AutofillBuildParameter[],
): WorkspaceBuildParameter[] => {
return params.map((parameter) => {
site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx
+431 -348
View File
File diff suppressed because it is too large Load Diff
site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx
+51 -43
View File
@@ -22,13 +22,11 @@ import type {
DynamicParametersRequest,
DynamicParametersResponse,
MinimalUser,
PreviewParameter,
Workspace,
} from "#/api/typesGenerated";
import { Loader } from "#/components/Loader/Loader";
import { useAuthenticated } from "#/hooks/useAuthenticated";
import { useExternalAuth } from "#/hooks/useExternalAuth";
import { getInitialParameterValues } from "#/modules/workspaces/DynamicParameter/DynamicParameter";
import { generateWorkspaceName } from "#/modules/workspaces/generateWorkspaceName";
import { pageTitle } from "#/utils/page";
import type { AutofillBuildParameter } from "#/utils/richParameters";
@@ -51,10 +49,14 @@ const CreateWorkspacePage: FC = () => {
const [latestResponse, setLatestResponse] =
useState<DynamicParametersResponse | null>(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<number>(-1);
const ws = useRef<WebSocket | null>(null);
const [wsError, setWsError] = useState<Error | null>(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);
const customVersionId = searchParams.get("version") ?? undefined;
const defaultName = searchParams.get("name");
@@ -155,61 +157,50 @@ const CreateWorkspacePage: FC = () => {
const hasIgnoredUrlParams =
urlAutofillParameters.length > 0 && urlPresetResult.preset !== undefined;
// sendMessage increments the ID and sends the form values on the web socket
// and returns true. If the socket is not open, it does not increment the ID
// and returns false.
const sendMessage = (
formValues: Record<string, string>,
ownerId?: string,
) => {
): boolean => {
const request: DynamicParametersRequest = {
id: wsResponseId.current + 1,
owner_id: ownerId ?? 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 all initial parameter values to the websocket
// (including defaults and autofilled from the url)
// This ensures the backend has the complete initial state of the form,
// which is vital for correctly rendering dynamic UI elements where parameter visibility
// or options might depend on the initial values of other parameters.
const sendInitialParameters = useEffectEvent(
(parameters: PreviewParameter[]) => {
if (initialParamsSentRef.current) return;
if (parameters.length === 0) return;
const initialFormValues = getInitialParameterValues(
parameters,
autofillParameters,
);
if (initialFormValues.length === 0) return;
const initialParamsToSend: Record<string, string> = {};
for (const param of initialFormValues) {
if (param.name && param.value !== undefined) {
initialParamsToSend[param.name] = param.value;
}
}
if (Object.keys(initialParamsToSend).length === 0) return;
sendMessage(initialParamsToSend);
initialParamsSentRef.current = true;
},
);
const onMessage = useEffectEvent((response: DynamicParametersResponse) => {
if (latestResponse && latestResponse?.id >= response.id) {
// 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 (!Number.isNaN(initId)) {
return;
}
if (!initialParamsSentRef.current && response.parameters?.length > 0) {
sendInitialParameters([...response.parameters]);
if (autofillParameters.length > 0) {
const values = Object.fromEntries(
autofillParameters.map((afp) => [afp.name, afp.value]),
);
if (!sendMessage(values)) {
return;
}
}
setLatestResponse(response);
// 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);
});
// Initialize the WebSocket connection when there is a valid template version ID
@@ -220,7 +211,17 @@ const CreateWorkspacePage: FC = () => {
realizedVersionId,
defaultOwner.id,
{
onMessage,
// Send initial parameters once the web socket is open.
onOpen: () => {
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);
@@ -373,12 +374,19 @@ const CreateWorkspacePage: FC = () => {
return [...latestResponse.parameters].sort((a, b) => a.order - b.order);
}, [latestResponse?.parameters]);
const isInitializing =
!latestResponse ||
Number.isNaN(initId) ||
latestResponse.id < initId ||
(ws.current && ws.current.readyState === WebSocket.CONNECTING);
const shouldShowLoader =
!templateQuery.data ||
isLoadingFormData ||
isLoadingExternalAuth ||
autoCreateReady ||
(!latestResponse && !wsError) ||
(isInitializing && !wsError) ||
(effectivePresetName &&
!templateVersionPresetsQuery.isSuccess &&
!templateVersionPresetsQuery.isError);
site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.tsx
+1 -1
View File
@@ -67,7 +67,7 @@ const WorkspaceParametersPage: FC = () => {
})) ?? [];
// 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 true. If the socket is not open, it does not increment the ID
// and returns false.
const sendMessage = (formValues: Record<string, string>): boolean => {
const request: DynamicParametersRequest = {
site/src/testHelpers/entities.ts
+3 -16
View File
@@ -3560,7 +3560,7 @@ export const MockDropdownParameter: TypesGen.PreviewParameter = {
order: 1,
};
const MockTagSelectParameter: TypesGen.PreviewParameter = {
export const MockTagSelectParameter: TypesGen.PreviewParameter = {
...MockPreviewParameter,
name: "tags",
display_name: "Tags",
@@ -3578,7 +3578,7 @@ const MockTagSelectParameter: TypesGen.PreviewParameter = {
order: 4,
};
const MockSwitchParameter: TypesGen.PreviewParameter = {
export const MockSwitchParameter: TypesGen.PreviewParameter = {
...MockPreviewParameter,
name: "enable_monitoring",
display_name: "Enable Monitoring",
@@ -3613,7 +3613,7 @@ export const MockSliderParameter: TypesGen.PreviewParameter = {
order: 2,
};
const MockMultiSelectParameter: TypesGen.PreviewParameter = {
export const MockMultiSelectParameter: TypesGen.PreviewParameter = {
...MockPreviewParameter,
name: "ides",
display_name: "IDEs",
@@ -3673,19 +3673,6 @@ export const MockValidationParameter: TypesGen.PreviewParameter = {
order: 1,
};
export const MockDynamicParametersResponse: TypesGen.DynamicParametersResponse =
{
id: 1,
parameters: [
MockDropdownParameter,
MockSliderParameter,
MockSwitchParameter,
MockTagSelectParameter,
MockMultiSelectParameter,
],
diagnostics: [],
};
export const MockDynamicParametersResponseWithError: TypesGen.DynamicParametersResponse =
{
id: 2,
site/src/testHelpers/parameters.ts
+51 -10
View File
@@ -2,7 +2,12 @@ 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;
type BuildParameter = TypesGen.WorkspaceBuildParameter & {
display_name?: string;
form_type?: TypesGen.ParameterFormType;
};
type Parameter = BuildParameter | TypesGen.PreviewParameter;
export function isBuildParameter(
parameter: Parameter,
@@ -11,8 +16,9 @@ export function isBuildParameter(
}
// checkParameters waits until all the provided parameters have the expected
// display value within the parameters form. Requires that the form and
// parameters all have test IDs (`form` and `parameter-field-$name`).
// display value within the parameters form and that there are no additional
// parameters. Requires that the form and parameters all have test IDs (`form`
// and `parameter-field-$name`).
export async function checkParameters(...parameters: Parameter[]) {
const form = screen.getByTestId("form");
await waitFor(() => {
@@ -23,8 +29,34 @@ export async function checkParameters(...parameters: Parameter[]) {
const value = isBuildParameter(parameter)
? parameter.value
: parameter.value.value;
expect(within(field).getByDisplayValue(value)).toBeInTheDocument();
const type = parameter.form_type || "input";
switch (type) {
case "switch":
if (value === "true") {
expect(within(field).getByRole("switch")).toBeChecked();
} else {
expect(within(field).getByRole("switch")).not.toBeChecked();
}
break;
case "dropdown":
expect(within(field).getByRole("combobox")).toHaveTextContent(
value || "Select option",
);
break;
case "multi-select":
case "tag-select":
// TODO: Validate these values as well, not just that they exist.
break;
case "input":
case "slider":
expect(within(field).getByDisplayValue(value)).toBeInTheDocument();
break;
default:
throw new Error(`checking ${type} fields is not implemented`);
}
}
const fields = within(form).queryAllByTestId(/^parameter-field-/);
expect(fields).toHaveLength(parameters.length);
});
}
@@ -35,15 +67,24 @@ 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);
const type = parameter.form_type || "input";
const label = parameter.display_name || parameter.name;
switch (type) {
case "input": {
const input = await within(field).findByLabelText(
new RegExp(label, "i"),
);
await userEvent.clear(input);
if (value !== "") {
await userEvent.type(input, value);
}
break;
}
default:
throw new Error(`editing ${type} fields is not implemented`);
}
}
}
site/src/testHelpers/websockets.ts
+1 -1
View File
@@ -18,7 +18,7 @@ type CallbackStore = {
[K in keyof WebSocketEventMap]: Set<(event: WebSocketEventMap[K]) => void>;
};
type MockWebSocket = Omit<WebSocket, "send"> & {
export type MockWebSocket = Omit<WebSocket, "send"> & {
/**
* A version of the WebSocket `send` method that has been pre-wrapped inside
* a vitest mock.