From 90b64c5e040d7db8c78b9ce49079f51aeccc7d77 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Mon, 27 Oct 2025 12:47:44 -0300 Subject: [PATCH] chore: remove unecessary API.getWorkspaceParameters (#20462) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I initially created `API.getWorkspaceParameters` to group two related requests, but after revisiting the implementation, I realized this abstraction doesn’t add much value. It also prevents us from taking full advantage of React Query’s built-in caching and invalidation. So instead of grouping them, I removed the helper and replaced it with separate queries — this simplifies the flow and lets React Query handle caching more efficiently. Related to https://github.com/coder/coder/pull/20431#discussion_r2457010137 --- site/src/api/api.ts | 13 --- site/src/api/queries/templates.ts | 8 +- site/src/api/queries/workspaceBuilds.ts | 2 +- site/src/pages/TaskPage/TaskPage.tsx | 10 +- .../BuildParametersPopover.tsx | 20 ++-- .../WorkspaceActions/DebugButton.stories.tsx | 23 ++++- .../WorkspaceActions/RetryButton.stories.tsx | 45 +++++---- .../WorkspacePage/WorkspacePage.test.tsx | 25 +++-- .../WorkspaceParametersPage.stories.tsx | 91 +++++++++---------- .../WorkspaceParametersPage.tsx | 63 +++++++------ 10 files changed, 160 insertions(+), 140 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 04bf78538a..4c02a96fe2 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2385,19 +2385,6 @@ class ApiMethods { return response.data; }; - getWorkspaceParameters = async (workspace: TypesGen.Workspace) => { - const latestBuild = workspace.latest_build; - const [templateVersionRichParameters, buildParameters] = await Promise.all([ - this.getTemplateVersionRichParameters(latestBuild.template_version_id), - this.getWorkspaceBuildParameters(latestBuild.id), - ]); - - return { - templateVersionRichParameters, - buildParameters, - }; - }; - getInsightsUserLatency = async ( filters: InsightsParams, ): Promise => { diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index 8c3b294f7f..686611cb6c 100644 --- a/site/src/api/queries/templates.ts +++ b/site/src/api/queries/templates.ts @@ -249,9 +249,15 @@ export const templateVersionLogs = (versionId: string) => { }; }; +export const richParametersKey = (versionId: string) => [ + templateVersionRoot, + versionId, + "richParameters", +]; + export const richParameters = (versionId: string) => { return { - queryKey: [templateVersionRoot, versionId, "richParameters"], + queryKey: richParametersKey(versionId), queryFn: () => API.getTemplateVersionRichParameters(versionId), }; }; diff --git a/site/src/api/queries/workspaceBuilds.ts b/site/src/api/queries/workspaceBuilds.ts index 8f5e088b3a..4617d988e3 100644 --- a/site/src/api/queries/workspaceBuilds.ts +++ b/site/src/api/queries/workspaceBuilds.ts @@ -6,7 +6,7 @@ import type { } from "api/typesGenerated"; import type { QueryOptions, UseInfiniteQueryOptions } from "react-query"; -function workspaceBuildParametersKey(workspaceBuildId: string) { +export function workspaceBuildParametersKey(workspaceBuildId: string) { return ["workspaceBuilds", workspaceBuildId, "parameters"] as const; } diff --git a/site/src/pages/TaskPage/TaskPage.tsx b/site/src/pages/TaskPage/TaskPage.tsx index 9724c09f5d..17cdc8b686 100644 --- a/site/src/pages/TaskPage/TaskPage.tsx +++ b/site/src/pages/TaskPage/TaskPage.tsx @@ -1,6 +1,7 @@ import { API } from "api/api"; import { getErrorDetail, getErrorMessage, isApiError } from "api/errors"; import { template as templateQueryOptions } from "api/queries/templates"; +import { workspaceBuildParameters } from "api/queries/workspaceBuilds"; import { startWorkspace, workspaceByOwnerAndName, @@ -198,10 +199,9 @@ type WorkspaceNotRunningProps = { const WorkspaceNotRunning: FC = ({ workspace }) => { const queryClient = useQueryClient(); - const { data: parameters } = useQuery({ - queryKey: ["workspace", workspace.id, "parameters"], - queryFn: () => API.getWorkspaceParameters(workspace), - }); + const { data: buildParameters } = useQuery( + workspaceBuildParameters(workspace.latest_build.id), + ); const mutateStartWorkspace = useMutation({ ...startWorkspace(workspace, queryClient), @@ -237,7 +237,7 @@ const WorkspaceNotRunning: FC = ({ workspace }) => { disabled={isWaitingForStart} onClick={() => { mutateStartWorkspace.mutate({ - buildParameters: parameters?.buildParameters, + buildParameters, }); }} > diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx index 557ea2d9be..a39dcd949c 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/BuildParametersPopover.tsx @@ -1,6 +1,7 @@ import { useTheme } from "@emotion/react"; import visuallyHidden from "@mui/utils/visuallyHidden"; -import { API } from "api/api"; +import { richParameters } from "api/queries/templates"; +import { workspaceBuildParameters } from "api/queries/workspaceBuilds"; import type { TemplateVersionParameter, Workspace, @@ -48,12 +49,15 @@ export const BuildParametersPopover: FC = ({ onSubmit, }) => { const [isOpen, setIsOpen] = useState(false); - const { data: parameters } = useQuery({ - queryKey: ["workspace", workspace.id, "parameters"], - queryFn: () => API.getWorkspaceParameters(workspace), - }); - const ephemeralParameters = parameters - ? parameters.templateVersionRichParameters.filter((p) => p.ephemeral) + const build = workspace.latest_build; + const { data: templateVersionParameters } = useQuery( + richParameters(build.template_version_id), + ); + const { data: buildParameters } = useQuery( + workspaceBuildParameters(build.id), + ); + const ephemeralParameters = templateVersionParameters + ? templateVersionParameters.filter((p) => p.ephemeral) : undefined; return ( @@ -75,7 +79,7 @@ export const BuildParametersPopover: FC = ({ diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/DebugButton.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/DebugButton.stories.tsx index c611031cbe..e7d3bc04c4 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/DebugButton.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/DebugButton.stories.tsx @@ -1,5 +1,10 @@ -import { MockWorkspace } from "testHelpers/entities"; +import { + MockTemplateVersionParameter1, + MockWorkspace, +} from "testHelpers/entities"; import type { Meta, StoryObj } from "@storybook/react-vite"; +import { richParametersKey } from "api/queries/templates"; +import { workspaceBuildParametersKey } from "api/queries/workspaceBuilds"; import { expect, screen, userEvent, waitFor } from "storybook/test"; import { DebugButton } from "./DebugButton"; @@ -21,8 +26,12 @@ export const WithBuildParameters: Story = { parameters: { queries: [ { - key: ["workspace", MockWorkspace.id, "parameters"], - data: { templateVersionRichParameters: [], buildParameters: [] }, + key: richParametersKey(MockWorkspace.latest_build.template_version_id), + data: [], + }, + { + key: workspaceBuildParametersKey(MockWorkspace.latest_build.id), + data: [], }, ], }, @@ -36,8 +45,12 @@ export const WithOpenBuildParameters: Story = { parameters: { queries: [ { - key: ["workspace", MockWorkspace.id, "parameters"], - data: { templateVersionRichParameters: [], buildParameters: [] }, + key: richParametersKey(MockWorkspace.latest_build.template_version_id), + data: [MockTemplateVersionParameter1], + }, + { + key: workspaceBuildParametersKey(MockWorkspace.latest_build.id), + data: [], }, ], }, diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/RetryButton.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/RetryButton.stories.tsx index b369603880..2b85ae75b1 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/RetryButton.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/RetryButton.stories.tsx @@ -1,9 +1,12 @@ import { MockNonClassicParameterFlowWorkspace, + MockTemplateVersionParameter1, MockTemplateVersionParameter6, MockWorkspace, } from "testHelpers/entities"; import type { Meta, StoryObj } from "@storybook/react-vite"; +import { richParametersKey } from "api/queries/templates"; +import { workspaceBuildParametersKey } from "api/queries/workspaceBuilds"; import { expect, screen, userEvent, waitFor } from "storybook/test"; import { RetryButton } from "./RetryButton"; @@ -25,8 +28,12 @@ export const WithBuildParameters: Story = { parameters: { queries: [ { - key: ["workspace", MockWorkspace.id, "parameters"], - data: { templateVersionRichParameters: [], buildParameters: [] }, + key: richParametersKey(MockWorkspace.latest_build.template_version_id), + data: [], + }, + { + key: workspaceBuildParametersKey(MockWorkspace.latest_build.id), + data: [], }, ], }, @@ -40,8 +47,12 @@ export const WithOpenBuildParameters: Story = { parameters: { queries: [ { - key: ["workspace", MockWorkspace.id, "parameters"], - data: { templateVersionRichParameters: [], buildParameters: [] }, + key: richParametersKey(MockWorkspace.latest_build.template_version_id), + data: [MockTemplateVersionParameter1], + }, + { + key: workspaceBuildParametersKey(MockWorkspace.latest_build.id), + data: [], }, ], }, @@ -63,11 +74,12 @@ export const WithOpenEphemeralBuildParameters: Story = { parameters: { queries: [ { - key: ["workspace", MockWorkspace.id, "parameters"], - data: { - templateVersionRichParameters: [MockTemplateVersionParameter6], - buildParameters: [], - }, + key: richParametersKey(MockWorkspace.latest_build.template_version_id), + data: [MockTemplateVersionParameter6], + }, + { + key: workspaceBuildParametersKey(MockWorkspace.latest_build.id), + data: [], }, ], }, @@ -91,15 +103,12 @@ export const WithOpenEphemeralBuildParametersNotClassic: Story = { parameters: { queries: [ { - key: [ - "workspace", - MockNonClassicParameterFlowWorkspace.id, - "parameters", - ], - data: { - templateVersionRichParameters: [MockTemplateVersionParameter6], - buildParameters: [], - }, + key: richParametersKey(MockWorkspace.latest_build.template_version_id), + data: [MockTemplateVersionParameter6], + }, + { + key: workspaceBuildParametersKey(MockWorkspace.latest_build.id), + data: [], }, ], }, diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index c204a3bb9d..8551de709f 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -47,7 +47,6 @@ const renderWorkspacePage = async ( ) => { jest.spyOn(API, "getWorkspaceByOwnerAndName").mockResolvedValue(workspace); jest.spyOn(API, "getTemplate").mockResolvedValueOnce(MockTemplate); - jest.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValueOnce([]); jest .spyOn(API, "getDeploymentConfig") .mockResolvedValueOnce(MockDeploymentConfig); @@ -380,18 +379,18 @@ describe("WorkspacePage", () => { it("restart the workspace with one time parameters when having the confirmation dialog", async () => { localStorage.removeItem(`${MockUserOwner.id}_ignoredWarnings`); - jest.spyOn(API, "getWorkspaceParameters").mockResolvedValue({ - templateVersionRichParameters: [ - { - ...MockTemplateVersionParameter1, - ephemeral: true, - name: "rebuild", - description: "Rebuild", - required: false, - }, - ], - buildParameters: [{ name: "rebuild", value: "false" }], - }); + jest.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValue([ + { + ...MockTemplateVersionParameter1, + ephemeral: true, + name: "rebuild", + description: "Rebuild", + required: false, + }, + ]); + jest + .spyOn(API, "getWorkspaceBuildParameters") + .mockResolvedValue([{ name: "rebuild", value: "false" }]); const restartWorkspaceSpy = jest.spyOn(API, "restartWorkspace"); const user = userEvent.setup(); await renderWorkspacePage(MockWorkspace); diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.stories.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.stories.tsx index 16fa481976..cbeee6be23 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.stories.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.stories.tsx @@ -21,22 +21,19 @@ const meta: Meta = { workspace: MockWorkspace, canChangeVersions: true, onCancel: action("onCancel"), - - data: { - buildParameters: [ - MockWorkspaceBuildParameter1, - MockWorkspaceBuildParameter2, - MockWorkspaceBuildParameter3, - ], - templateVersionRichParameters: [ - MockTemplateVersionParameter1, - MockTemplateVersionParameter2, - { - ...MockTemplateVersionParameter3, - mutable: false, - }, - ], - }, + buildParameters: [ + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter2, + MockWorkspaceBuildParameter3, + ], + templateVersionParameters: [ + MockTemplateVersionParameter1, + MockTemplateVersionParameter2, + { + ...MockTemplateVersionParameter3, + mutable: false, + }, + ], }, }; @@ -47,10 +44,8 @@ const Example: Story = {}; export const Empty: Story = { args: { - data: { - buildParameters: [], - templateVersionRichParameters: [], - }, + buildParameters: [], + templateVersionParameters: [], }, }; @@ -58,21 +53,19 @@ export const RequireActiveVersionNoChangeVersion: Story = { args: { workspace: MockOutdatedStoppedWorkspaceRequireActiveVersion, canChangeVersions: false, - data: { - buildParameters: [ - MockWorkspaceBuildParameter1, - MockWorkspaceBuildParameter2, - MockWorkspaceBuildParameter3, - ], - templateVersionRichParameters: [ - MockTemplateVersionParameter1, - MockTemplateVersionParameter2, - { - ...MockTemplateVersionParameter3, - mutable: false, - }, - ], - }, + buildParameters: [ + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter2, + MockWorkspaceBuildParameter3, + ], + templateVersionParameters: [ + MockTemplateVersionParameter1, + MockTemplateVersionParameter2, + { + ...MockTemplateVersionParameter3, + mutable: false, + }, + ], }, }; @@ -80,21 +73,19 @@ export const RequireActiveVersionCanChangeVersion: Story = { args: { workspace: MockOutdatedStoppedWorkspaceRequireActiveVersion, canChangeVersions: true, - data: { - buildParameters: [ - MockWorkspaceBuildParameter1, - MockWorkspaceBuildParameter2, - MockWorkspaceBuildParameter3, - ], - templateVersionRichParameters: [ - MockTemplateVersionParameter1, - MockTemplateVersionParameter2, - { - ...MockTemplateVersionParameter3, - mutable: false, - }, - ], - }, + buildParameters: [ + MockWorkspaceBuildParameter1, + MockWorkspaceBuildParameter2, + MockWorkspaceBuildParameter3, + ], + templateVersionParameters: [ + MockTemplateVersionParameter1, + MockTemplateVersionParameter2, + { + ...MockTemplateVersionParameter3, + mutable: false, + }, + ], }, }; diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.tsx index cdf1412daf..db67b52429 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPage.tsx @@ -1,7 +1,13 @@ import { API } from "api/api"; import { isApiValidationError } from "api/errors"; import { checkAuthorization } from "api/queries/authCheck"; -import type { Workspace, WorkspaceBuildParameter } from "api/typesGenerated"; +import { richParameters } from "api/queries/templates"; +import { workspaceBuildParameters } from "api/queries/workspaceBuilds"; +import type { + TemplateVersionParameter, + Workspace, + WorkspaceBuildParameter, +} from "api/typesGenerated"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Button } from "components/Button/Button"; import { EmptyState } from "components/EmptyState/EmptyState"; @@ -24,10 +30,13 @@ import { const WorkspaceParametersPage: FC = () => { const workspace = useWorkspaceSettings(); - const parameters = useQuery({ - queryKey: ["workspace", workspace.id, "parameters"], - queryFn: () => API.getWorkspaceParameters(workspace), - }); + const build = workspace.latest_build; + const { data: templateVersionParameters } = useQuery( + richParameters(build.template_version_id), + ); + const { data: buildParameters } = useQuery( + workspaceBuildParameters(build.id), + ); const navigate = useNavigate(); const updateParameters = useMutation({ mutationFn: (buildParameters: WorkspaceBuildParameter[]) => @@ -75,30 +84,30 @@ const WorkspaceParametersPage: FC = () => { { - if (!parameters.data) { + if (!templateVersionParameters) { return; } // When updating the parameters, the API does not accept immutable // values so we need to filter them - const onlyMultableValues = - parameters.data.templateVersionRichParameters - .filter((p) => p.mutable) - .map((p) => { - const value = values.rich_parameter_values.find( - (v) => v.name === p.name, - ); - if (!value) { - throw new Error(`Missing value for parameter ${p.name}`); - } - return value; - }); - updateParameters.mutate(onlyMultableValues); + const onlyMutableValues = templateVersionParameters + .filter((p) => p.mutable) + .map((p) => { + const value = values.rich_parameter_values.find( + (v) => v.name === p.name, + ); + if (!value) { + throw new Error(`Missing value for parameter ${p.name}`); + } + return value; + }); + updateParameters.mutate(onlyMutableValues); }} onCancel={() => { navigate("../.."); @@ -112,7 +121,8 @@ type WorkspaceParametersPageViewProps = { workspace: Workspace; canChangeVersions: boolean; templatePermissions: { canUpdateTemplate: boolean } | undefined; - data: Awaited> | undefined; + templateVersionParameters?: TemplateVersionParameter[]; + buildParameters?: WorkspaceBuildParameter[]; submitError: unknown; isSubmitting: boolean; onSubmit: (formValues: WorkspaceParametersFormValues) => void; @@ -125,7 +135,8 @@ export const WorkspaceParametersPageView: FC< workspace, canChangeVersions, templatePermissions, - data, + templateVersionParameters, + buildParameters, submitError, onSubmit, isSubmitting, @@ -143,17 +154,17 @@ export const WorkspaceParametersPageView: FC< ) : null} - {data ? ( - data.templateVersionRichParameters.length > 0 ? ( + {templateVersionParameters && buildParameters ? ( + templateVersionParameters.length > 0 ? ( ({ + autofillParams={buildParameters.map((p) => ({ ...p, source: "active_build", }))} - templateVersionRichParameters={data.templateVersionRichParameters} + templateVersionRichParameters={templateVersionParameters} error={submitError} isSubmitting={isSubmitting} onSubmit={onSubmit}