Apply PR reviews

coder · BrunoQuaresma · May 13, 2025 · May 12, 2025 · May 12, 2025 · May 12, 2025
commit 84b65a83d9f47dc30366fe2c36a8891d45056971
@@ -16,7 +16,7 @@ import { Pill } from "components/Pill/Pill";
 import { Stack } from "components/Stack/Stack";
 import { InfoIcon } from "lucide-react";
 import { TemplateUpdateMessage } from "modules/templates/TemplateUpdateMessage";
-import { type FC, useEffect, useState } from "react";
+import { type FC, useState } from "react";
 import { useQuery } from "react-query";
 import { createDayString } from "utils/createDayString";
 
@@ -31,28 +31,23 @@ export const ChangeWorkspaceVersionDialog: FC<
 > = ({ workspace, onClose, onConfirm, ...dialogProps }) => {
 	const { data: versions } = useQuery({
 		...templateVersions(workspace.template_id),
-		select: (data) => data.reverse(),
+		select: (data) => [...data].reverse(),
 	});
 	const [isAutocompleteOpen, setIsAutocompleteOpen] = useState(false);
 	const activeVersion = versions?.find(
 		(v) => workspace.template_active_version_id === v.id,
 	);
-	const [selectedVersion, setSelectedVersion] = useState<TemplateVersion>();
+	const [newVersion, setNewVersion] = useState<TemplateVersion>();
 	const validVersions = versions?.filter((v) => v.job.status === "succeeded");
-
-	useEffect(() => {
-		if (activeVersion) {
-			setSelectedVersion(activeVersion);
-		}
-	}, [activeVersion]);
+	const selectedVersion = newVersion || activeVersion;
 
 	return (
 		<ConfirmDialog
 			{...dialogProps}
 			onClose={onClose}
 			onConfirm={() => {
-				if (selectedVersion) {
-					onConfirm(selectedVersion);
+				if (newVersion) {
+					onConfirm(newVersion);
 				}
 			}}
 			hideCancel={false}
@@ -73,7 +68,7 @@ export const ChangeWorkspaceVersionDialog: FC<
 									id="template-version-autocomplete"
 									open={isAutocompleteOpen}
 									onChange={(_, newTemplateVersion) => {
-										setSelectedVersion(newTemplateVersion);
+										setNewVersion(newTemplateVersion);
 									}}
 									onOpen={() => {
 										setIsAutocompleteOpen(true);

@@ -21,7 +21,7 @@ import {
 	SettingsIcon,
 	TrashIcon,
 } from "lucide-react";
-import { type FC, useState } from "react";
+import { type FC, useEffect, useState } from "react";
 import { useMutation, useQuery, useQueryClient } from "react-query";
 import { Link as RouterLink } from "react-router-dom";
 import { ChangeWorkspaceVersionDialog } from "./ChangeWorkspaceVersionDialog";
@@ -32,7 +32,7 @@ import { useWorkspaceDuplication } from "./useWorkspaceDuplication";
 
 type WorkspaceMoreActionsProps = {
 	workspace: Workspace;
-	disabled?: boolean;
+	disabled: boolean;
 };
 
 export const WorkspaceMoreActions: FC<WorkspaceMoreActionsProps> = ({
@@ -63,9 +63,17 @@ export const WorkspaceMoreActions: FC<WorkspaceMoreActionsProps> = ({
 	const { duplicateWorkspace, isDuplicationReady } =
 		useWorkspaceDuplication(workspace);
 
+	// Since the workspace state is not updated immediately after the mutation, we
+	// need to be sure the menu is closed when the action gets disabled.
+	// Reference: https://github.com/coder/coder/pull/17775#discussion_r2087273706
+	const [open, setOpen] = useState(false);
+	useEffect(() => {
+		setOpen((open) => (disabled ? false : open));
+	});
+
 	return (
 		<>
-			<DropdownMenu>
+			<DropdownMenu open={open} onOpenChange={setOpen}>
 				<DropdownMenuTrigger asChild>
 					<Button
 						size="icon-lg"
@@ -136,16 +144,16 @@ export const WorkspaceMoreActions: FC<WorkspaceMoreActionsProps> = ({
 
 			<UpdateBuildParametersDialog
 				missedParameters={
-					isMissingBuildParameters(changeVersionMutation.error)
+					changeVersionMutation.error instanceof MissingBuildParameters
 						? changeVersionMutation.error.parameters
 						: []
 				}
-				open={isMissingBuildParameters(changeVersionMutation.error)}
+				open={changeVersionMutation.error instanceof MissingBuildParameters}
 				onClose={() => {
 					changeVersionMutation.reset();
 				}}
 				onUpdate={(buildParameters) => {
-					if (isMissingBuildParameters(changeVersionMutation.error)) {
+					if (changeVersionMutation.error instanceof MissingBuildParameters) {
 						changeVersionMutation.mutate({
 							versionId: changeVersionMutation.error.versionId,
 							buildParameters,
@@ -181,7 +189,3 @@ export const WorkspaceMoreActions: FC<WorkspaceMoreActionsProps> = ({
 		</>
 	);
 };
-
-const isMissingBuildParameters = (e: unknown): e is MissingBuildParameters => {
-	return Boolean(e && e instanceof MissingBuildParameters);
-};
@@ -1,5 +1,6 @@
 import type { Meta, StoryObj } from "@storybook/react";
 import { expect, userEvent, within } from "@storybook/test";
+import { deploymentConfigQueryKey } from "api/queries/deployment";
 import { agentLogsKey, buildLogsKey } from "api/queries/workspaces";
 import * as Mocks from "testHelpers/entities";
 import {
@@ -8,7 +9,6 @@ import {
 	withDesktopViewport,
 } from "testHelpers/storybook";
 import { WorkspaceActions } from "./WorkspaceActions";
-import { deploymentConfigQueryKey } from "api/queries/deployment";
 
 const meta: Meta<typeof WorkspaceActions> = {
 	title: "pages/WorkspacePage/WorkspaceActions",

@@ -65,7 +65,7 @@ export const WorkspaceActions: FC<WorkspaceActionsProps> = ({
 		workspace,
 		{
 			canDebug: !!deployment?.config.enable_terraform_debug_mode,
-			isOwner: !!user.roles.find((role) => role.name === "owner"),
+			isOwner: user.roles.some((role) => role.name === "owner"),
 		},
 	);
 

@@ -3,7 +3,6 @@ import Button from "@mui/material/Button";
 import { API } from "api/api";
 import { isApiValidationError } from "api/errors";
 import { checkAuthorization } from "api/queries/authCheck";
-import { templateByName } from "api/queries/templates";
 import type { Workspace, WorkspaceBuildParameter } from "api/typesGenerated";
 import { ErrorAlert } from "components/Alert/ErrorAlert";
 import { EmptyState } from "components/EmptyState/EmptyState";
@@ -43,17 +42,11 @@ const WorkspaceParametersPage: FC = () => {
 		},
 	});
 
-	const templateQuery = useQuery({
-		...templateByName(workspace.organization_id, workspace.template_name ?? ""),
-		enabled: workspace !== undefined,
-	});
-	const template = templateQuery.data;
-
 	// Permissions
-	const checks = workspace && template ? workspaceChecks(workspace) : {};
+	const checks = workspace ? workspaceChecks(workspace) : {};
 	const permissionsQuery = useQuery({
 		...checkAuthorization({ checks }),
-		enabled: workspace !== undefined && template !== undefined,
+		enabled: workspace !== undefined,
 	});
 	const permissions = permissionsQuery.data as WorkspacePermissions | undefined;
 	const canChangeVersions = Boolean(permissions?.updateWorkspaceVersion);
@@ -71,14 +64,23 @@ const WorkspaceParametersPage: FC = () => {
 				submitError={updateParameters.error}
 				isSubmitting={updateParameters.isLoading}
 				onSubmit={(values) => {
+					if (!parameters.data) {
+						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) =>
-								values.rich_parameter_values.find((v) => v.name === p.name)!,
-						);
+					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);
 				}}
 				onCancel={() => {