Skip to content

feat: setup url autofill for dynamic parameters #17739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 16, 2025
Merged

feat: setup url autofill for dynamic parameters #17739

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4209d7d
feat: setup autofill for dynamic parameters
jaaydenh May 7, 2025
1d798ea
chore: cleanup
jaaydenh May 8, 2025
9d10195
feat: set initial param values from autofill
jaaydenh May 9, 2025
ec35a7f
fix: setup ParameterLabel to use htmlfor
jaaydenh May 12, 2025
04c7f83
chore: change autofill to boolean
jaaydenh May 12, 2025
03dde8a
chore: updates for PR review comments
jaaydenh May 12, 2025
afdab85
chore: move logic for sending initial parameters to websocket
jaaydenh May 12, 2025
a76159f
fix: improvements to dynamic parameter handling
jaaydenh May 13, 2025
6711e28
fix: updates for PR review
jaaydenh May 15, 2025
7dc96bb
fix: update logic for onMessage
jaaydenh May 15, 2025
eb413fb
fix: format
jaaydenh May 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: move logic for sending initial parameters to websocket
  • Loading branch information
@jaaydenh
jaaydenh committed May 15, 2025
commit afdab8527b7ad64f3b45e2311f5ae5f96711a27c
94 changes: 69 additions & 25 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { API } from "api/api";
import { type ApiErrorResponse, DetailedError } from "api/errors";
import { checkAuthorization } from "api/queries/authCheck";
import {
Expand All @@ -9,11 +10,13 @@ import { autoCreateWorkspace, createWorkspace } from "api/queries/workspaces";
import type {
DynamicParametersRequest,
DynamicParametersResponse,
PreviewParameter,
Workspace,
} from "api/typesGenerated";
import { Loader } from "components/Loader/Loader";
import { useAuthenticated } from "hooks";
import { useEffectEvent } from "hooks/hookPolyfills";
import { getInitialParameterValues } from "modules/workspaces/DynamicParameter/DynamicParameter";
import { generateWorkspaceName } from "modules/workspaces/generateWorkspaceName";
import {
type FC,
Expand All @@ -29,13 +32,13 @@ import { useNavigate, useParams, useSearchParams } from "react-router-dom";
import { pageTitle } from "utils/page";
import type { AutofillBuildParameter } from "utils/richParameters";
import { CreateWorkspacePageViewExperimental } from "./CreateWorkspacePageViewExperimental";
const createWorkspaceModes = ["form", "auto", "duplicate"] as const;
export type CreateWorkspaceMode = (typeof createWorkspaceModes)[number];
import { API } from "api/api";
import {
type CreateWorkspacePermissions,
createWorkspaceChecks,
} from "./permissions";

const createWorkspaceModes = ["form", "auto", "duplicate"] as const;
export type CreateWorkspaceMode = (typeof createWorkspaceModes)[number];
export type ExternalAuthPollingState = "idle" | "polling" | "abandoned";

const CreateWorkspacePageExperimental: FC = () => {
Expand All @@ -50,6 +53,7 @@ const CreateWorkspacePageExperimental: FC = () => {
const [wsResponseId, setWSResponseId] = useState<number>(-1);
const ws = useRef<WebSocket | null>(null);
const [wsError, setWsError] = useState<Error | null>(null);
const initialParamsSentRef = useRef(false);

const customVersionId = searchParams.get("version") ?? undefined;
const defaultName = searchParams.get("name");
Expand Down Expand Up @@ -84,15 +88,72 @@ const CreateWorkspacePageExperimental: FC = () => {
const realizedVersionId =
customVersionId ?? templateQuery.data?.active_version_id;

const onMessage = useCallback((response: DynamicParametersResponse) => {
setCurrentResponse((prev) => {
if (prev?.id === response.id) {
return prev;
const autofillParameters = useMemo(
() => getAutofillParameters(searchParams),
[searchParams],
);

const sendMessage = useCallback((formValues: Record<string, string>) => {
setWSResponseId((prevId) => {
const request: DynamicParametersRequest = {
id: prevId + 1,
inputs: formValues,
};
if (ws.current && ws.current.readyState === WebSocket.OPEN) {
ws.current.send(JSON.stringify(request));
return prevId + 1;
}
return response;
return prevId;
Copy link
Member

@Parkreiner Parkreiner May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

State setter callbacks have to be 100% pure. I want to say React Strict Mode will also double-call a state dispatch update to make sure of that. As in, it does the first dispatch, throws away the result, and then only flushes the second one to state

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also, I'm seeing that we're never using the ID in the render logic, so assuming we don't need it for any future work, we can safely replace it with a useRef call

});
}, []);

// On 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) {
initialParamsToSend[param.name] = param.value;
}
}

if (Object.keys(initialParamsToSend).length === 0) return;

sendMessage(initialParamsToSend);
initialParamsSentRef.current = true;
},
);

const onMessage = useCallback(
(response: DynamicParametersResponse) => {
setCurrentResponse((prev) => {
if (prev?.id === response.id) {
return prev;
}

if (!initialParamsSentRef.current && response.parameters.length > 0) {
sendInitialParameters([...response.parameters]);
}

return response;
});
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another case where we want to remove the side effect from the state update. I'm actually much more concerned about this actually turning into a bug

I want to say useEffectEvent should help here. As in, since the function isn't ever being passed down as props, it should be safe to turn this into:

const onMessage = useEffectEvent((response: DynamicParametersResponse) => {
  if (currentResponse?.id === response.id) {
    return;
  }

  if (!initialParamsSentRef.current && response.parameters.length > 0) {
    sendInitialParameters([...response.parameters]);
  }
  
  setCurrentReswponse(response);
});

[sendInitialParameters],
);

// Initialize the WebSocket connection when there is a valid template version ID
useEffect(() => {
if (!realizedVersionId) return;
Expand Down Expand Up @@ -127,20 +188,6 @@ const CreateWorkspacePageExperimental: FC = () => {
};
}, [owner.id, realizedVersionId, onMessage]);

const sendMessage = useCallback((formValues: Record<string, string>) => {
setWSResponseId((prevId) => {
const request: DynamicParametersRequest = {
id: prevId + 1,
inputs: formValues,
};
if (ws.current && ws.current.readyState === WebSocket.OPEN) {
ws.current.send(JSON.stringify(request));
return prevId + 1;
}
return prevId;
});
}, []);

const organizationId = templateQuery.data?.organization_id;

const {
Expand All @@ -167,9 +214,6 @@ const CreateWorkspacePageExperimental: FC = () => {
[navigate],
);

// Auto fill parameters
const autofillParameters = getAutofillParameters(searchParams);

const autoCreationStartedRef = useRef(false);
const automateWorkspaceCreation = useEffectEvent(async () => {
if (autoCreationStartedRef.current || !organizationId) {
Expand Down
24 changes: 0 additions & 24 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,30 +141,6 @@ export const CreateWorkspacePageViewExperimental: FC<
},
});

// On component mount, 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 hasInitializedWebsocket = useRef(false);
useEffect(() => {
if (hasInitializedWebsocket.current) return;

const formValues = form.values.rich_parameter_values;
if (parameters.length > 0 && formValues && formValues.length > 0) {
const initialParams: Record<string, string> = {};
for (const param of formValues) {
if (param.name && param.value) {
initialParams[param.name] = param.value;
}
}
if (Object.keys(initialParams).length > 0) {
sendMessage(initialParams);
hasInitializedWebsocket.current = true;
}
}
}, [parameters, form.values.rich_parameter_values, sendMessage]);

const autofillByName = Object.fromEntries(
autofillParameters.map((param) => [param.name, param]),
);
Expand Down