From 014b602825e3f5bcb7fc40f829ea5dd778387f69 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 30 Jun 2025 16:12:05 +0000 Subject: [PATCH 01/37] refactor: rename variable for clarity --- site/src/pages/WorkspacesPage/WorkspacesPageView.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx index 6563533bc43da..d2f440a1e1fcc 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx @@ -87,10 +87,10 @@ export const WorkspacesPageView: FC = ({ onActionSuccess, onActionError, }) => { - // Let's say the user has 5 workspaces, but tried to hit page 100, which does - // not exist. In this case, the page is not valid and we want to show a better - // error message. - const invalidPageNumber = page !== 1 && workspaces?.length === 0; + // Let's say the user has 5 workspaces, but tried to hit page 100, which + // does not exist. In this case, the page is not valid and we want to show a + // better error message. + const pageNumberIsInvalid = page !== 1 && workspaces?.length === 0; return ( @@ -187,7 +187,7 @@ export const WorkspacesPageView: FC = ({ ) : ( - !invalidPageNumber && ( + !pageNumberIsInvalid && ( = ({ )} - {invalidPageNumber ? ( + {pageNumberIsInvalid ? ( ({ border: `1px solid ${theme.palette.divider}`, From 667361cb268529c18c67af98515d020e9c3b7c73 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 30 Jun 2025 16:56:49 +0000 Subject: [PATCH 02/37] refactor: reduce coupling with React Router for utilities --- site/src/components/Filter/Filter.tsx | 41 ++++++++++------- site/src/hooks/usePagination.ts | 45 ++++++++++++------- site/src/pages/AuditPage/AuditPage.tsx | 3 +- .../src/pages/TemplatesPage/TemplatesPage.tsx | 5 ++- site/src/pages/UsersPage/UsersPage.tsx | 8 ++-- .../pages/WorkspacesPage/WorkspacesPage.tsx | 24 ++++++---- 6 files changed, 78 insertions(+), 48 deletions(-) diff --git a/site/src/components/Filter/Filter.tsx b/site/src/components/Filter/Filter.tsx index 736592116730d..2d5094fce7ca7 100644 --- a/site/src/components/Filter/Filter.tsx +++ b/site/src/components/Filter/Filter.tsx @@ -16,7 +16,7 @@ import { useDebouncedFunction } from "hooks/debounce"; import { ExternalLinkIcon } from "lucide-react"; import { ChevronDownIcon } from "lucide-react"; import { type FC, type ReactNode, useEffect, useRef, useState } from "react"; -import type { useSearchParams } from "react-router-dom"; +import { useSearchParams } from "react-router-dom"; type PresetFilter = { name: string; @@ -27,35 +27,46 @@ type FilterValues = Record; type UseFilterConfig = { /** - * The fallback value to use in the event that no filter params can be parsed - * from the search params object. This value is allowed to change on - * re-renders. + * The fallback value to use in the event that no filter params can be + * parsed from the search params object. */ fallbackFilter?: string; - searchParamsResult: ReturnType; + searchParams: URLSearchParams; + onSearchParamsChange: (newParams: URLSearchParams) => void; onUpdate?: (newValue: string) => void; }; +export type UseFilterResult = Readonly<{ + query: string; + values: FilterValues; + used: boolean; + update: (newValues: string | FilterValues) => void; + debounceUpdate: (newValues: string | FilterValues) => void; + cancelDebounce: () => void; +}>; + export const useFilterParamsKey = "filter"; export const useFilter = ({ fallbackFilter = "", - searchParamsResult, + searchParams, + onSearchParamsChange, onUpdate, -}: UseFilterConfig) => { - const [searchParams, setSearchParams] = searchParamsResult; +}: UseFilterConfig): UseFilterResult => { const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter; const update = (newValues: string | FilterValues) => { const serialized = typeof newValues === "string" ? newValues : stringifyFilter(newValues); - - searchParams.set(useFilterParamsKey, serialized); - setSearchParams(searchParams); - - if (onUpdate !== undefined) { - onUpdate(serialized); + const noUpdateNeeded = searchParams.get(useFilterParamsKey) === serialized; + if (noUpdateNeeded) { + return; } + + const copy = new URLSearchParams(searchParams); + copy.set(useFilterParamsKey, serialized); + onSearchParamsChange(copy); + onUpdate?.(serialized); }; const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction( @@ -73,8 +84,6 @@ export const useFilter = ({ }; }; -export type UseFilterResult = ReturnType; - const parseFilterQuery = (filterQuery: string): FilterValues => { if (filterQuery === "") { return {}; diff --git a/site/src/hooks/usePagination.ts b/site/src/hooks/usePagination.ts index 72ea70868fb30..a9c4703b88ae5 100644 --- a/site/src/hooks/usePagination.ts +++ b/site/src/hooks/usePagination.ts @@ -1,25 +1,38 @@ import { DEFAULT_RECORDS_PER_PAGE } from "components/PaginationWidget/utils"; -import type { useSearchParams } from "react-router-dom"; -export const usePagination = ({ - searchParamsResult, -}: { - searchParamsResult: ReturnType; -}) => { - const [searchParams, setSearchParams] = searchParamsResult; +type UsePaginationOptions = Readonly<{ + searchParams: URLSearchParams; + onSearchParamsChange: (newParams: URLSearchParams) => void; +}>; + +type UsePaginationResult = Readonly<{ + page: number; + limit: number; + offset: number; + goToPage: (page: number) => void; +}>; + +export function usePagination( + options: UsePaginationOptions, +): UsePaginationResult { + const { searchParams, onSearchParamsChange } = options; const page = searchParams.get("page") ? Number(searchParams.get("page")) : 1; const limit = DEFAULT_RECORDS_PER_PAGE; - const offset = page <= 0 ? 0 : (page - 1) * limit; - - const goToPage = (page: number) => { - searchParams.set("page", page.toString()); - setSearchParams(searchParams); - }; return { page, limit, - goToPage, - offset, + offset: page <= 0 ? 0 : (page - 1) * limit, + goToPage: (newPage) => { + const abortNavigation = + page === newPage || !Number.isFinite(page) || !Number.isInteger(page); + if (abortNavigation) { + return; + } + + const copy = new URLSearchParams(searchParams); + copy.set("page", page.toString()); + onSearchParamsChange(copy); + }, }; -}; +} diff --git a/site/src/pages/AuditPage/AuditPage.tsx b/site/src/pages/AuditPage/AuditPage.tsx index f63adbcd4136b..669fbbdb14a27 100644 --- a/site/src/pages/AuditPage/AuditPage.tsx +++ b/site/src/pages/AuditPage/AuditPage.tsx @@ -33,7 +33,8 @@ const AuditPage: FC = () => { const [searchParams, setSearchParams] = useSearchParams(); const auditsQuery = usePaginatedQuery(paginatedAudits(searchParams)); const filter = useFilter({ - searchParamsResult: [searchParams, setSearchParams], + searchParams, + onSearchParamsChange: setSearchParams, onUpdate: auditsQuery.goToFirstPage, }); diff --git a/site/src/pages/TemplatesPage/TemplatesPage.tsx b/site/src/pages/TemplatesPage/TemplatesPage.tsx index bef25e17bb755..832c5b43c1c9c 100644 --- a/site/src/pages/TemplatesPage/TemplatesPage.tsx +++ b/site/src/pages/TemplatesPage/TemplatesPage.tsx @@ -14,10 +14,11 @@ const TemplatesPage: FC = () => { const { permissions, user: me } = useAuthenticated(); const { showOrganizations } = useDashboard(); - const searchParamsResult = useSearchParams(); + const [searchParams, setSearchParams] = useSearchParams(); const filter = useFilter({ fallbackFilter: "deprecated:false", - searchParamsResult, + searchParams, + onSearchParamsChange: setSearchParams, onUpdate: () => {}, // reset pagination }); diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index 581a9166bce3d..bb176e9150c4b 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -39,9 +39,8 @@ type UserPageProps = { const UsersPage: FC = ({ defaultNewPassword }) => { const queryClient = useQueryClient(); const navigate = useNavigate(); - const searchParamsResult = useSearchParams(); + const [searchParams, setSearchParams] = useSearchParams(); const { entitlements } = useDashboard(); - const [searchParams] = searchParamsResult; const groupsByUserIdQuery = useQuery(groupsByUserId()); const authMethodsQuery = useQuery(authMethods()); @@ -58,9 +57,10 @@ const UsersPage: FC = ({ defaultNewPassword }) => { enabled: viewDeploymentConfig, }); - const usersQuery = usePaginatedQuery(paginatedUsers(searchParamsResult[0])); + const usersQuery = usePaginatedQuery(paginatedUsers(searchParams)); const useFilterResult = useFilter({ - searchParamsResult, + searchParams, + onSearchParamsChange: setSearchParams, onUpdate: usersQuery.goToFirstPage, }); diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 22ba0d15f1f9a..5b6b5760e6fb5 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -22,7 +22,7 @@ import { WorkspacesPageView } from "./WorkspacesPageView"; import { useBatchActions } from "./batchActions"; import { useStatusFilterMenu, useTemplateFilterMenu } from "./filter/menus"; -function useSafeSearchParams() { +function useSafeSearchParams(): ReturnType { // Have to wrap setSearchParams because React Router doesn't make sure that // the function's memory reference stays stable on each render, even though // its logic never changes, and even though it has function update support @@ -41,8 +41,11 @@ const WorkspacesPage: FC = () => { // If we use a useSearchParams for each hook, the values will not be in sync. // So we have to use a single one, centralizing the values, and pass it to // each hook. - const searchParamsResult = useSafeSearchParams(); - const pagination = usePagination({ searchParamsResult }); + const [searchParams, setSearchParams] = useSafeSearchParams(); + const pagination = usePagination({ + searchParams, + onSearchParamsChange: setSearchParams, + }); const { permissions, user: me } = useAuthenticated(); const { entitlements } = useDashboard(); const templatesQuery = useQuery(templates()); @@ -67,7 +70,8 @@ const WorkspacesPage: FC = () => { }, [templatesQuery.data, workspacePermissionsQuery.data]); const filterProps = useWorkspacesFilter({ - searchParamsResult, + searchParams, + onSearchParamsChange: setSearchParams, onFilterChange: () => pagination.goToPage(1), }); @@ -88,7 +92,6 @@ const WorkspacesPage: FC = () => { const [confirmingBatchAction, setConfirmingBatchAction] = useState< "delete" | "update" | null >(null); - const [urlSearchParams] = searchParamsResult; const canCheckWorkspaces = entitlements.features.workspace_batch_actions.enabled; const batchActions = useBatchActions({ @@ -103,7 +106,7 @@ const WorkspacesPage: FC = () => { // biome-ignore lint/correctness/useExhaustiveDependencies: consider refactoring useEffect(() => { setCheckedWorkspaces([]); - }, [urlSearchParams]); + }, [searchParams]); return ( <> @@ -179,17 +182,20 @@ const WorkspacesPage: FC = () => { export default WorkspacesPage; type UseWorkspacesFilterOptions = { - searchParamsResult: ReturnType; + searchParams: URLSearchParams; + onSearchParamsChange: (newParams: URLSearchParams) => void; onFilterChange: () => void; }; const useWorkspacesFilter = ({ - searchParamsResult, + searchParams, + onSearchParamsChange, onFilterChange, }: UseWorkspacesFilterOptions) => { const filter = useFilter({ fallbackFilter: "owner:me", - searchParamsResult, + searchParams, + onSearchParamsChange, onUpdate: onFilterChange, }); From d07ba1730f122b347bd67ecd92467b97ee97023b Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 30 Jun 2025 22:09:44 +0000 Subject: [PATCH 03/37] refactor: clean up page view --- .../CreateWorkspacePageViewExperimental.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx index 8cb6c4acb6e49..5b4a966ae6965 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx @@ -108,9 +108,7 @@ export const CreateWorkspacePageViewExperimental: FC< owner, setOwner, }) => { - const [suggestedName, setSuggestedName] = useState(() => - generateWorkspaceName(), - ); + const [suggestedName, setSuggestedName] = useState(generateWorkspaceName); const [showPresetParameters, setShowPresetParameters] = useState(false); const id = useId(); const workspaceNameInputRef = useRef(null); @@ -124,14 +122,14 @@ export const CreateWorkspacePageViewExperimental: FC< // Only touched fields are sent to the websocket // Autofilled parameters are marked as touched since they have been modified - const initialTouched = parameters.reduce( + const initialTouched = parameters.reduce>( (touched, parameter) => { if (autofillByName[parameter.name] !== undefined) { touched[parameter.name] = true; } return touched; }, - {} as Record, + {}, ); // The form parameters values hold the working state of the parameters that will be submitted when creating a workspace From e82c63aca3abb6d9888359a247a36c2885a195c4 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 1 Jul 2025 15:22:04 +0000 Subject: [PATCH 04/37] wip: commit current progress --- .vscode/settings.json | 3 + .../WorkspacesPage/BatchUpdateModalForm.tsx | 137 ++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx diff --git a/.vscode/settings.json b/.vscode/settings.json index f2cf72b7d8ae0..3667afcdc18ed 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -60,5 +60,8 @@ "typos.config": ".github/workflows/typos.toml", "[markdown]": { "editor.defaultFormatter": "DavidAnson.vscode-markdownlint" + }, + "[typescriptreact]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" } } diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx new file mode 100644 index 0000000000000..23e7a39a73762 --- /dev/null +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -0,0 +1,137 @@ +import { TemplateVersion, type Workspace } from "api/typesGenerated"; +import { type FC, useMemo, useState } from "react"; +import { Dialog, DialogContent } from "components/Dialog/Dialog"; +import { Button } from "components/Button/Button"; +import { useQueries } from "react-query"; +import { templateVersion } from "api/queries/templates"; + +/** + * @todo Need to decide if we should include the template display name here, or + * if we'll be able to get that data as part of other fetches. It's also + * possible that the new UX might not require it at all? + */ +type TemplateVersionGroup = Readonly<{ + templateVersionId: string; + affectedWorkspaces: readonly Workspace[]; +}>; + +function groupWorkspacesByTemplateVersionId( + workspaces: readonly Workspace[], +): readonly TemplateVersionGroup[] { + const grouped = new Map(); + + for (const ws of workspaces) { + const templateVersionId = ws.latest_build.template_version_id; + const value = grouped.get(templateVersionId); + if (value !== undefined) { + // Need to do type assertion to make value mutable as an + // implementation detail. Doing things the "proper" way adds a bunch + // of needless boilerplate for a single-line computation + const target = value.affectedWorkspaces as Workspace[]; + target.push(ws); + continue; + } + + grouped.set(templateVersionId, { + templateVersionId, + affectedWorkspaces: [ws], + }); + } + + return [...grouped.values()]; +} + +type WorkspaceDeltaEntry = Readonly<{}>; +type WorkspaceDeltas = Map; + +function separateWorkspacesByDormancy( + workspaces: readonly Workspace[], +): readonly [dormant: readonly Workspace[], active: readonly Workspace[]] { + const dormant: Workspace[] = []; + const active: Workspace[] = []; + + for (const ws of workspaces) { + // If a workspace doesn't have any pending updates whatsoever, we can + // safely skip processing it + if (!ws.outdated) { + continue; + } + if (ws.dormant_at) { + dormant.push(ws); + } else { + active.push(ws); + } + } + + return [dormant, active]; +} + +type BatchUpdateModalFormProps = Readonly<{ + workspacesToUpdate: readonly Workspace[]; + onClose: () => void; + onSubmit: () => void; +}>; + +export const BatchUpdateModalForm: FC = ({ + workspacesToUpdate, + onClose, + onSubmit, +}) => { + // We need to take a local snapshot of the workspaces that existed on mount + // because workspaces are such a mutable resource, and there's a chance that + // they can be changed by another user + be subject to a query invalidation + // while the form is open. We need to cross-reference these with the latest + // workspaces from props so that we can display any changes in the UI + const [cachedWorkspaces, setCachedWorkspaces] = useState(workspacesToUpdate); + // Dormant workspaces can't be activated without activating them first. For + // now, we'll only show the user that some workspaces can't be updated, and + // then skip over them for all other update logic + const [dormant, active] = separateWorkspacesByDormancy(cachedWorkspaces); + + // The workspaces don't have all necessary data by themselves, so we need to + // fetch the unique template versions, and massage the results + const groups = groupWorkspacesByTemplateVersionId(active); + const templateVersionQueries = useQueries({ + queries: groups.map((g) => templateVersion(g.templateVersionId)), + }); + // React Query persists previous errors even if a query is no longer in the + // error state, so we need to explicitly check the isError property + const error = templateVersionQueries.find((q) => q.isError)?.error; + const merged = templateVersionQueries.every((q) => q.isSuccess) + ? templateVersionQueries.map((q) => q.data) + : undefined; + + // Also need to tease apart workspaces that are actively running, because + // there's a whole set of warnings we need to issue about them + const running = active.filter((a) => a.latest_build.status === "running"); + const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; + + const deltas = useMemo(() => new Map(), []); + + return ( + + +
{ + e.preventDefault(); + console.log("Blah"); + onSubmit(); + }} + > +
+

+ Review updates +

+ +
+
+ + + ); +}; From 696ec335911e595d8115a6adf35579e24721facd Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 1 Jul 2025 18:08:51 +0000 Subject: [PATCH 05/37] wip: commit more progress --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 217 ++++++++++++++---- .../pages/WorkspacesPage/WorkspacesPage.tsx | 49 ++-- 2 files changed, 198 insertions(+), 68 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 23e7a39a73762..0bc2a827a95b6 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,9 +1,12 @@ -import { TemplateVersion, type Workspace } from "api/typesGenerated"; -import { type FC, useMemo, useState } from "react"; +import type { Workspace } from "api/typesGenerated"; +import { type FC, ReactElement, ReactNode, useState } from "react"; import { Dialog, DialogContent } from "components/Dialog/Dialog"; import { Button } from "components/Button/Button"; import { useQueries } from "react-query"; import { templateVersion } from "api/queries/templates"; +import { Loader } from "components/Loader/Loader"; +import { ErrorAlert } from "components/Alert/ErrorAlert"; +import { Avatar } from "components/Avatar/Avatar"; /** * @todo Need to decide if we should include the template display name here, or @@ -41,61 +44,85 @@ function groupWorkspacesByTemplateVersionId( return [...grouped.values()]; } -type WorkspaceDeltaEntry = Readonly<{}>; -type WorkspaceDeltas = Map; +type Separation = Readonly<{ + dormant: readonly Workspace[]; + noUpdateNeeded: readonly Workspace[]; + readyToUpdate: readonly Workspace[]; +}>; -function separateWorkspacesByDormancy( - workspaces: readonly Workspace[], -): readonly [dormant: readonly Workspace[], active: readonly Workspace[]] { +function separateWorkspaces(workspaces: readonly Workspace[]): Separation { + const noUpdateNeeded: Workspace[] = []; const dormant: Workspace[] = []; - const active: Workspace[] = []; + const readyToUpdate: Workspace[] = []; for (const ws of workspaces) { - // If a workspace doesn't have any pending updates whatsoever, we can - // safely skip processing it if (!ws.outdated) { + noUpdateNeeded.push(ws); continue; } - if (ws.dormant_at) { + if (ws.dormant_at !== null) { dormant.push(ws); - } else { - active.push(ws); + continue; } + readyToUpdate.push(ws); } - return [dormant, active]; + return { dormant, noUpdateNeeded, readyToUpdate }; } -type BatchUpdateModalFormProps = Readonly<{ +type WorkspacePanelProps = Readonly<{ + workspaceName: string; + workspaceIconUrl: string; + label?: ReactNode; + adornment?: ReactNode; +}>; + +const ReviewPanel: FC = ({ + workspaceName, + label, + workspaceIconUrl, +}) => { + return ( +
+
+ + {workspaceName} +
+
+ ); +}; + +type ReviewFormProps = Readonly<{ workspacesToUpdate: readonly Workspace[]; - onClose: () => void; + onCancel: () => void; onSubmit: () => void; }>; -export const BatchUpdateModalForm: FC = ({ +const ReviewForm: FC = ({ workspacesToUpdate, - onClose, + onCancel, onSubmit, }) => { // We need to take a local snapshot of the workspaces that existed on mount // because workspaces are such a mutable resource, and there's a chance that // they can be changed by another user + be subject to a query invalidation - // while the form is open. We need to cross-reference these with the latest - // workspaces from props so that we can display any changes in the UI + // while the form is open const [cachedWorkspaces, setCachedWorkspaces] = useState(workspacesToUpdate); // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and // then skip over them for all other update logic - const [dormant, active] = separateWorkspacesByDormancy(cachedWorkspaces); + const { dormant, noUpdateNeeded, readyToUpdate } = + separateWorkspaces(cachedWorkspaces); // The workspaces don't have all necessary data by themselves, so we need to // fetch the unique template versions, and massage the results - const groups = groupWorkspacesByTemplateVersionId(active); + const groups = groupWorkspacesByTemplateVersionId(readyToUpdate); const templateVersionQueries = useQueries({ queries: groups.map((g) => templateVersion(g.templateVersionId)), }); // React Query persists previous errors even if a query is no longer in the - // error state, so we need to explicitly check the isError property + // error state, so we need to explicitly check the isError property to see + // if any of the queries actively have an error const error = templateVersionQueries.find((q) => q.isError)?.error; const merged = templateVersionQueries.every((q) => q.isSuccess) ? templateVersionQueries.map((q) => q.data) @@ -103,34 +130,132 @@ export const BatchUpdateModalForm: FC = ({ // Also need to tease apart workspaces that are actively running, because // there's a whole set of warnings we need to issue about them - const running = active.filter((a) => a.latest_build.status === "running"); - const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; + const running = readyToUpdate.filter( + (ws) => ws.latest_build.status === "running", + ); - const deltas = useMemo(() => new Map(), []); + const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; + const updateIsReady = error !== undefined && readyToUpdate.length > 0; return ( - - -
{ - e.preventDefault(); - console.log("Blah"); - onSubmit(); - }} + { + e.preventDefault(); + onSubmit(); + }} + > +
+

+ Review update +

+ + + Refresh list + +
+ + {error !== undefined && } + + {noUpdateNeeded.length > 0 && ( +
+
+

Updated workspaces

+

+ These workspaces are fully up to date and will be skipped during + the update. +

- + +
    + {noUpdateNeeded.map((ws) => ( +
  • + +
    • + ))} +
+
+ )} + + {dormant.length > 0 && ( +
+
+

Dormant workspaces

+

+ Dormant workspaces cannot be updated without first activating the + workspace. They will be skipped during the batch update. +

+
+ +
    + {dormant.map((ws) => ( +
  • + +
    • + ))} +
+
+ )} + +
+ + +
+ + ); +}; + +type BatchUpdateModalFormProps = Readonly<{ + workspacesToUpdate: readonly Workspace[]; + open: boolean; + loading: boolean; + onClose: () => void; + onSubmit: () => void; +}>; + +export const BatchUpdateModalForm: FC = ({ + open, + loading, + workspacesToUpdate, + onClose, + onSubmit, +}) => { + return ( + { + if (open) { + onClose(); + } + }} + > + + {loading ? ( + + ) : ( + { + onSubmit(); + onClose(); + }} + /> + )} ); diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 5b6b5760e6fb5..8d9f7a76c90e8 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -17,10 +17,10 @@ import { useQuery, useQueryClient } from "react-query"; import { useSearchParams } from "react-router-dom"; import { pageTitle } from "utils/page"; import { BatchDeleteConfirmation } from "./BatchDeleteConfirmation"; -import { BatchUpdateConfirmation } from "./BatchUpdateConfirmation"; import { WorkspacesPageView } from "./WorkspacesPageView"; import { useBatchActions } from "./batchActions"; import { useStatusFilterMenu, useTemplateFilterMenu } from "./filter/menus"; +import { BatchUpdateModalForm } from "./BatchUpdateModalForm"; function useSafeSearchParams(): ReturnType { // Have to wrap setSearchParams because React Router doesn't make sure that @@ -36,6 +36,8 @@ function useSafeSearchParams(): ReturnType { >; } +type BatchAction = "delete" | "update"; + const WorkspacesPage: FC = () => { const queryClient = useQueryClient(); // If we use a useSearchParams for each hook, the values will not be in sync. @@ -89,9 +91,7 @@ const WorkspacesPage: FC = () => { const [checkedWorkspaces, setCheckedWorkspaces] = useState< readonly Workspace[] >([]); - const [confirmingBatchAction, setConfirmingBatchAction] = useState< - "delete" | "update" | null - >(null); + const [activeBatchAction, setActiveBatchAction] = useState(); const canCheckWorkspaces = entitlements.features.workspace_batch_actions.enabled; const batchActions = useBatchActions({ @@ -130,8 +130,8 @@ const WorkspacesPage: FC = () => { onPageChange={pagination.goToPage} filterProps={filterProps} isRunningBatchAction={batchActions.isLoading} - onDeleteAll={() => setConfirmingBatchAction("delete")} - onUpdateAll={() => setConfirmingBatchAction("update")} + onDeleteAll={() => setActiveBatchAction("delete")} + onUpdateAll={() => setActiveBatchAction("update")} onStartAll={() => batchActions.startAll(checkedWorkspaces)} onStopAll={() => batchActions.stopAll(checkedWorkspaces)} onActionSuccess={async () => { @@ -150,29 +150,34 @@ const WorkspacesPage: FC = () => { { await batchActions.deleteAll(checkedWorkspaces); - setConfirmingBatchAction(null); + setActiveBatchAction(undefined); }} onClose={() => { - setConfirmingBatchAction(null); + setActiveBatchAction(undefined); }} /> - { - await batchActions.updateAll({ - workspaces: checkedWorkspaces, - isDynamicParametersEnabled: false, - }); - setConfirmingBatchAction(null); - }} - onClose={() => { - setConfirmingBatchAction(null); + setActiveBatchAction(undefined)} + onSubmit={async () => { + console.log("Hooray!"); + /** + * @todo Make sure this gets added back in once more of the + * component has been fleshed out + */ + if (false) { + await batchActions.updateAll({ + workspaces: checkedWorkspaces, + isDynamicParametersEnabled: false, + }); + } + setActiveBatchAction(undefined); }} /> From 1461efc29d129dbc3f3bf43f535e50ec0cd8420f Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 1 Jul 2025 19:22:22 +0000 Subject: [PATCH 06/37] wip: commit more progress --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 222 ++++++++++++------ 1 file changed, 150 insertions(+), 72 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 0bc2a827a95b6..9cc0039740c58 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,6 +1,6 @@ import type { Workspace } from "api/typesGenerated"; -import { type FC, ReactElement, ReactNode, useState } from "react"; -import { Dialog, DialogContent } from "components/Dialog/Dialog"; +import { type FC, type ReactNode, useEffect, useState } from "react"; +import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; import { Button } from "components/Button/Button"; import { useQueries } from "react-query"; import { templateVersion } from "api/queries/templates"; @@ -24,7 +24,7 @@ function groupWorkspacesByTemplateVersionId( const grouped = new Map(); for (const ws of workspaces) { - const templateVersionId = ws.latest_build.template_version_id; + const templateVersionId = ws.template_active_version_id; const value = grouped.get(templateVersionId); if (value !== undefined) { // Need to do type assertion to make value mutable as an @@ -83,15 +83,36 @@ const ReviewPanel: FC = ({ workspaceIconUrl, }) => { return ( -
-
+
+
- {workspaceName} +
+ {workspaceName} + + {label} + +
); }; +type TemplateNameChangeProps = Readonly<{ + oldTemplateName: string; + newTemplateName: string; +}>; + +const TemplateNameChange: FC = ({ + oldTemplateName, + newTemplateName, +}) => { + return ( + + {oldTemplateName} → {newTemplateName} + + ); +}; + type ReviewFormProps = Readonly<{ workspacesToUpdate: readonly Workspace[]; onCancel: () => void; @@ -108,6 +129,11 @@ const ReviewForm: FC = ({ // they can be changed by another user + be subject to a query invalidation // while the form is open const [cachedWorkspaces, setCachedWorkspaces] = useState(workspacesToUpdate); + + useEffect(() => { + console.log(cachedWorkspaces); + }, [cachedWorkspaces]); + // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and // then skip over them for all other update logic @@ -120,10 +146,12 @@ const ReviewForm: FC = ({ const templateVersionQueries = useQueries({ queries: groups.map((g) => templateVersion(g.templateVersionId)), }); + // React Query persists previous errors even if a query is no longer in the // error state, so we need to explicitly check the isError property to see // if any of the queries actively have an error const error = templateVersionQueries.find((q) => q.isError)?.error; + const merged = templateVersionQueries.every((q) => q.isSuccess) ? templateVersionQueries.map((q) => q.data) : undefined; @@ -135,86 +163,133 @@ const ReviewForm: FC = ({ ); const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; - const updateIsReady = error !== undefined && readyToUpdate.length > 0; + const updateIsReady = error === undefined && readyToUpdate.length > 0; return (
{ e.preventDefault(); onSubmit(); }} > -
-

- Review update -

- - -
+ {error !== undefined ? ( + + ) : ( + <> +
+
+ +

+ Review update +

+ - {error !== undefined && } + +
- {noUpdateNeeded.length > 0 && ( -
-
-

Updated workspaces

-

- These workspaces are fully up to date and will be skipped during - the update. -

-
+ {readyToUpdate.length > 0 && ( +
+
+

Ready to update

+

+ These workspaces have available updates. +

+
-
    - {noUpdateNeeded.map((ws) => ( -
  • - -
    • - ))} -
-
- )} +
    + {readyToUpdate.map((ws) => { + const matchedQuery = templateVersionQueries.find( + (q) => q.data?.id === ws.template_active_version_id, + ); + const newTemplateName = matchedQuery?.data?.name; + + return ( +
  • + + ) + } + /> +
    • + ); + })} +
+
+ )} + + {noUpdateNeeded.length > 0 && ( +
+
+

Updated workspaces

+

+ These workspaces are fully updated and will be skipped. +

+
- {dormant.length > 0 && ( -
-
-

Dormant workspaces

-

- Dormant workspaces cannot be updated without first activating the - workspace. They will be skipped during the batch update. -

+
    + {noUpdateNeeded.map((ws) => ( +
  • + +
    • + ))} +
+
+ )} + + {dormant.length > 0 && ( +
+
+

Dormant workspaces

+

+ Dormant workspaces cannot be updated without first + activating the workspace. They will be skipped during the + batch update. +

+
+ +
    + {dormant.map((ws) => ( +
  • + +
    • + ))} +
+
+ )}
-
    - {dormant.map((ws) => ( -
  • - -
    • - ))} -
- +
+ + +
+ )} - -
- - -
); }; @@ -245,7 +320,10 @@ export const BatchUpdateModalForm: FC = ({ > {loading ? ( - + <> + Loading… + + ) : ( Date: Tue, 1 Jul 2025 20:07:56 +0000 Subject: [PATCH 07/37] chore: add basic border grouping --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 67 +++++++++++++------ 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 9cc0039740c58..5ad0f20b1c421 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,5 +1,5 @@ import type { Workspace } from "api/typesGenerated"; -import { type FC, type ReactNode, useEffect, useState } from "react"; +import { type FC, type ReactNode, useState } from "react"; import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; import { Button } from "components/Button/Button"; import { useQueries } from "react-query"; @@ -7,6 +7,7 @@ import { templateVersion } from "api/queries/templates"; import { Loader } from "components/Loader/Loader"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Avatar } from "components/Avatar/Avatar"; +import { cn } from "utils/cn"; /** * @todo Need to decide if we should include the template display name here, or @@ -75,15 +76,24 @@ type WorkspacePanelProps = Readonly<{ workspaceIconUrl: string; label?: ReactNode; adornment?: ReactNode; + className?: string; }>; const ReviewPanel: FC = ({ workspaceName, label, workspaceIconUrl, + className, }) => { + // Preemptively adding border to this component so that it still looks + // decent when used outside of a list return ( -
+
@@ -107,9 +117,14 @@ const TemplateNameChange: FC = ({ newTemplateName, }) => { return ( - - {oldTemplateName} → {newTemplateName} - + <> + + {oldTemplateName} → {newTemplateName} + + + {oldTemplateName} will be updated to {newTemplateName} + + ); }; @@ -130,10 +145,6 @@ const ReviewForm: FC = ({ // while the form is open const [cachedWorkspaces, setCachedWorkspaces] = useState(workspacesToUpdate); - useEffect(() => { - console.log(cachedWorkspaces); - }, [cachedWorkspaces]); - // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and // then skip over them for all other update logic @@ -177,16 +188,17 @@ const ReviewForm: FC = ({ ) : ( <> -
-
+
+

- Review update + Review updates

-
    +
      {dormant.map((ws) => ( -
    • +
    • From adaa585b1b9c94adca01acca485630c073932f9c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 1 Jul 2025 20:43:12 +0000 Subject: [PATCH 08/37] wip: commit more progress --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 73 +++++++++++++++---- 1 file changed, 59 insertions(+), 14 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 5ad0f20b1c421..6c8e479ad3193 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -8,6 +8,7 @@ import { Loader } from "components/Loader/Loader"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Avatar } from "components/Avatar/Avatar"; import { cn } from "utils/cn"; +import _ from "lodash"; /** * @todo Need to decide if we should include the template display name here, or @@ -45,13 +46,15 @@ function groupWorkspacesByTemplateVersionId( return [...grouped.values()]; } -type Separation = Readonly<{ +type UpdateTypePartition = Readonly<{ dormant: readonly Workspace[]; noUpdateNeeded: readonly Workspace[]; readyToUpdate: readonly Workspace[]; }>; -function separateWorkspaces(workspaces: readonly Workspace[]): Separation { +function separateWorkspacesByUpdateType( + workspaces: readonly Workspace[], +): UpdateTypePartition { const noUpdateNeeded: Workspace[] = []; const dormant: Workspace[] = []; const readyToUpdate: Workspace[] = []; @@ -74,6 +77,7 @@ function separateWorkspaces(workspaces: readonly Workspace[]): Separation { type WorkspacePanelProps = Readonly<{ workspaceName: string; workspaceIconUrl: string; + running?: boolean; label?: ReactNode; adornment?: ReactNode; className?: string; @@ -144,12 +148,17 @@ const ReviewForm: FC = ({ // they can be changed by another user + be subject to a query invalidation // while the form is open const [cachedWorkspaces, setCachedWorkspaces] = useState(workspacesToUpdate); + // Used to force the user to acknowledge that batch updating has risks in + // certain situations and could destroy their data. Initial value + // deliberately *not* based on any derived values to avoid state sync issues + // as cachedWorkspaces gets refreshed + const [acceptedConsequences, setAcceptedConsequences] = useState(false); // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and // then skip over them for all other update logic const { dormant, noUpdateNeeded, readyToUpdate } = - separateWorkspaces(cachedWorkspaces); + separateWorkspacesByUpdateType(cachedWorkspaces); // The workspaces don't have all necessary data by themselves, so we need to // fetch the unique template versions, and massage the results @@ -167,18 +176,21 @@ const ReviewForm: FC = ({ ? templateVersionQueries.map((q) => q.data) : undefined; - // Also need to tease apart workspaces that are actively running, because - // there's a whole set of warnings we need to issue about them - const running = readyToUpdate.filter( + const [running, notRunning] = _.partition( + readyToUpdate, (ws) => ws.latest_build.status === "running", ); const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; - const updateIsReady = error === undefined && readyToUpdate.length > 0; + const consequencesResolved = running.length === 0 || acceptedConsequences; + const canSubmit = + consequencesResolved && + error === undefined && + (running.length > 0 || notRunning.length > 0); return (
      { e.preventDefault(); onSubmit(); @@ -200,7 +212,10 @@ const ReviewForm: FC = ({ variant="outline" size="sm" disabled={!workspacesChangedWhileOpen} - onClick={() => setCachedWorkspaces(workspacesToUpdate)} + onClick={() => { + setCachedWorkspaces(workspacesToUpdate); + setAcceptedConsequences(false); + }} > Refresh list @@ -211,13 +226,43 @@ const ReviewForm: FC = ({

      Ready to update

      - These workspaces have available updates and require no - additional action before updating. + These workspaces require no additional action before + updating.

        - {readyToUpdate.map((ws) => { + {running.map((ws) => { + const matchedQuery = templateVersionQueries.find( + (q) => q.data?.id === ws.template_active_version_id, + ); + const newTemplateName = matchedQuery?.data?.name; + + return ( +
      • + + ) + } + /> +
        • + ); + })} + {notRunning.map((ws) => { const matchedQuery = templateVersionQueries.find( (q) => q.data?.id === ws.template_active_version_id, ); @@ -305,11 +350,11 @@ const ReviewForm: FC = ({ )}
-
+
-
From 8f9ce72c947c3673e5e433d833eb2a25bd2024b6 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 2 Jul 2025 14:43:36 +0000 Subject: [PATCH 09/37] fix: update setup for batchActions --- .../pages/WorkspacesPage/WorkspacesPage.tsx | 14 ++--- .../src/pages/WorkspacesPage/batchActions.tsx | 56 +++++++++++++------ 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 8d9f7a76c90e8..83e4b117c84a1 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -129,11 +129,11 @@ const WorkspacesPage: FC = () => { limit={pagination.limit} onPageChange={pagination.goToPage} filterProps={filterProps} - isRunningBatchAction={batchActions.isLoading} + isRunningBatchAction={batchActions.isProcessing} onDeleteAll={() => setActiveBatchAction("delete")} onUpdateAll={() => setActiveBatchAction("update")} - onStartAll={() => batchActions.startAll(checkedWorkspaces)} - onStopAll={() => batchActions.stopAll(checkedWorkspaces)} + onStartAll={() => batchActions.start(checkedWorkspaces)} + onStopAll={() => batchActions.stop(checkedWorkspaces)} onActionSuccess={async () => { await queryClient.invalidateQueries({ queryKey: workspacesQueryOptions.queryKey, @@ -148,11 +148,11 @@ const WorkspacesPage: FC = () => { /> { - await batchActions.deleteAll(checkedWorkspaces); + await batchActions.delete(checkedWorkspaces); setActiveBatchAction(undefined); }} onClose={() => { @@ -162,7 +162,7 @@ const WorkspacesPage: FC = () => { setActiveBatchAction(undefined)} onSubmit={async () => { @@ -172,7 +172,7 @@ const WorkspacesPage: FC = () => { * component has been fleshed out */ if (false) { - await batchActions.updateAll({ + await batchActions.updateTemplateVersions({ workspaces: checkedWorkspaces, isDynamicParametersEnabled: false, }); diff --git a/site/src/pages/WorkspacesPage/batchActions.tsx b/site/src/pages/WorkspacesPage/batchActions.tsx index 806c7a03afddb..3e342b38ee132 100644 --- a/site/src/pages/WorkspacesPage/batchActions.tsx +++ b/site/src/pages/WorkspacesPage/batchActions.tsx @@ -1,13 +1,32 @@ import { API } from "api/api"; -import type { Workspace } from "api/typesGenerated"; +import type { Workspace, WorkspaceBuild } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; import { useMutation } from "react-query"; -interface UseBatchActionsProps { +interface UseBatchActionsOptions { onSuccess: () => Promise; } -export function useBatchActions(options: UseBatchActionsProps) { +type UpdateAllPayload = Readonly<{ + workspaces: readonly Workspace[]; + isDynamicParametersEnabled: boolean; +}>; + +type UseBatchActionsResult = Readonly<{ + isProcessing: boolean; + start: (workspaces: readonly Workspace[]) => Promise; + stop: (workspaces: readonly Workspace[]) => Promise; + delete: (workspaces: readonly Workspace[]) => Promise; + updateTemplateVersions: ( + payload: UpdateAllPayload, + ) => Promise; + favorite: (payload: readonly Workspace[]) => Promise; + unfavorite: (payload: readonly Workspace[]) => Promise; +}>; + +export function useBatchActions( + options: UseBatchActionsOptions, +): UseBatchActionsResult { const { onSuccess } = options; const startAllMutation = useMutation({ @@ -45,10 +64,7 @@ export function useBatchActions(options: UseBatchActionsProps) { }); const updateAllMutation = useMutation({ - mutationFn: (payload: { - workspaces: readonly Workspace[]; - isDynamicParametersEnabled: boolean; - }) => { + mutationFn: (payload: UpdateAllPayload) => { const { workspaces, isDynamicParametersEnabled } = payload; return Promise.all( workspaces @@ -62,9 +78,13 @@ export function useBatchActions(options: UseBatchActionsProps) { }, }); + // We have to explicitly make the mutation functions for the + // favorite/unfavorite functionality be async and then void out the + // Promise.all result because otherwise the return type becomes a void + // array, which doesn't ever make sense const favoriteAllMutation = useMutation({ - mutationFn: (workspaces: readonly Workspace[]) => { - return Promise.all( + mutationFn: async (workspaces: readonly Workspace[]) => { + void Promise.all( workspaces .filter((w) => !w.favorite) .map((w) => API.putFavoriteWorkspace(w.id)), @@ -77,8 +97,8 @@ export function useBatchActions(options: UseBatchActionsProps) { }); const unfavoriteAllMutation = useMutation({ - mutationFn: (workspaces: readonly Workspace[]) => { - return Promise.all( + mutationFn: async (workspaces: readonly Workspace[]) => { + void Promise.all( workspaces .filter((w) => w.favorite) .map((w) => API.deleteFavoriteWorkspace(w.id)), @@ -91,13 +111,13 @@ export function useBatchActions(options: UseBatchActionsProps) { }); return { - favoriteAll: favoriteAllMutation.mutateAsync, - unfavoriteAll: unfavoriteAllMutation.mutateAsync, - startAll: startAllMutation.mutateAsync, - stopAll: stopAllMutation.mutateAsync, - deleteAll: deleteAllMutation.mutateAsync, - updateAll: updateAllMutation.mutateAsync, - isLoading: + favorite: favoriteAllMutation.mutateAsync, + unfavorite: unfavoriteAllMutation.mutateAsync, + start: startAllMutation.mutateAsync, + stop: stopAllMutation.mutateAsync, + delete: deleteAllMutation.mutateAsync, + updateTemplateVersions: updateAllMutation.mutateAsync, + isProcessing: favoriteAllMutation.isPending || unfavoriteAllMutation.isPending || startAllMutation.isPending || From dbca22a5d5ff7b7bac883f12ccac2a2050b07b64 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 2 Jul 2025 14:45:46 +0000 Subject: [PATCH 10/37] fix: rename misleading prop --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 52 ++++--------------- .../pages/WorkspacesPage/WorkspacesPage.tsx | 4 +- 2 files changed, 13 insertions(+), 43 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 6c8e479ad3193..a7d000ee39e94 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -176,17 +176,16 @@ const ReviewForm: FC = ({ ? templateVersionQueries.map((q) => q.data) : undefined; - const [running, notRunning] = _.partition( - readyToUpdate, - (ws) => ws.latest_build.status === "running", + const runningIds = new Set( + readyToUpdate + .filter((ws) => ws.latest_build.status === "running") + .map((ws) => ws.id), ); const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; - const consequencesResolved = running.length === 0 || acceptedConsequences; + const consequencesResolved = runningIds.size === 0 || acceptedConsequences; const canSubmit = - consequencesResolved && - error === undefined && - (running.length > 0 || notRunning.length > 0); + consequencesResolved && error === undefined && readyToUpdate.length > 0; return ( = ({
    - {running.map((ws) => { - const matchedQuery = templateVersionQueries.find( - (q) => q.data?.id === ws.template_active_version_id, - ); - const newTemplateName = matchedQuery?.data?.name; - - return ( -
  • - - ) - } - /> -
    • - ); - })} - {notRunning.map((ws) => { + {readyToUpdate.map((ws) => { const matchedQuery = templateVersionQueries.find( (q) => q.data?.id === ws.template_active_version_id, ); @@ -275,6 +244,7 @@ const ReviewForm: FC = ({ > = ({ type BatchUpdateModalFormProps = Readonly<{ workspacesToUpdate: readonly Workspace[]; open: boolean; - loading: boolean; + isProcessing: boolean; onClose: () => void; onSubmit: () => void; }>; export const BatchUpdateModalForm: FC = ({ open, - loading, + isProcessing, workspacesToUpdate, onClose, onSubmit, @@ -389,7 +359,7 @@ export const BatchUpdateModalForm: FC = ({ }} > - {loading ? ( + {isProcessing ? ( <> Loading… diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 83e4b117c84a1..d902d0c70391e 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -162,11 +162,11 @@ const WorkspacesPage: FC = () => { setActiveBatchAction(undefined)} onSubmit={async () => { - console.log("Hooray!"); + window.alert("Hooray!"); /** * @todo Make sure this gets added back in once more of the * component has been fleshed out From 44ad230e5be55a02dc34a450b12e26adfc640a48 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 2 Jul 2025 14:54:11 +0000 Subject: [PATCH 11/37] fix: remove unused import --- site/src/components/Filter/Filter.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/site/src/components/Filter/Filter.tsx b/site/src/components/Filter/Filter.tsx index 2d5094fce7ca7..1cdff25547c30 100644 --- a/site/src/components/Filter/Filter.tsx +++ b/site/src/components/Filter/Filter.tsx @@ -16,7 +16,6 @@ import { useDebouncedFunction } from "hooks/debounce"; import { ExternalLinkIcon } from "lucide-react"; import { ChevronDownIcon } from "lucide-react"; import { type FC, type ReactNode, useEffect, useRef, useState } from "react"; -import { useSearchParams } from "react-router-dom"; type PresetFilter = { name: string; From 3f57b67e05cef329617f3d3c61b387a4613f2a8c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 2 Jul 2025 17:28:26 +0000 Subject: [PATCH 12/37] wip: commit progress --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 99 +++++++++++++++---- 1 file changed, 82 insertions(+), 17 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index a7d000ee39e94..71304915ff239 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,14 +1,16 @@ import type { Workspace } from "api/typesGenerated"; -import { type FC, type ReactNode, useState } from "react"; +import { type FC, type ReactNode, Suspense, useState } from "react"; import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; import { Button } from "components/Button/Button"; -import { useQueries } from "react-query"; +import { useSuspenseQueries } from "react-query"; import { templateVersion } from "api/queries/templates"; import { Loader } from "components/Loader/Loader"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Avatar } from "components/Avatar/Avatar"; import { cn } from "utils/cn"; -import _ from "lodash"; +import { Checkbox } from "components/Checkbox/Checkbox"; +import { Badge } from "components/Badge/Badge"; +import { Label } from "@radix-ui/react-label"; /** * @todo Need to decide if we should include the template display name here, or @@ -86,6 +88,7 @@ type WorkspacePanelProps = Readonly<{ const ReviewPanel: FC = ({ workspaceName, label, + running, workspaceIconUrl, className, }) => { @@ -101,7 +104,14 @@ const ReviewPanel: FC = ({
    - {workspaceName} + + {workspaceName} + {running && ( + + Running + + )} + {label} @@ -123,23 +133,60 @@ const TemplateNameChange: FC = ({ return ( <> - {oldTemplateName} → {newTemplateName} + Template: {oldTemplateName} → {newTemplateName} - {oldTemplateName} will be updated to {newTemplateName} + Template {oldTemplateName} will be updated to template {newTemplateName} ); }; +type RunningWorkspacesWarningProps = Readonly<{ + acceptedConsequences: boolean; + onAcceptedConsequencesChange: (newValue: boolean) => void; +}>; + +const RunningWorkspacesWarning: FC = ({ + acceptedConsequences, + onAcceptedConsequencesChange, +}) => { + return ( +
    +

    Running workspaces detected

    +
      +
    • + Updating a workspace will start it on its latest template version. + This can delete non-persistent data. +
      • +
    • + Anyone connected to a running workspace will be disconnected until the + update is complete. +
      • +
    • Any unsaved data will be lost.
      • +
    + +
    + ); +}; + type ReviewFormProps = Readonly<{ workspacesToUpdate: readonly Workspace[]; + isProcessing: boolean; onCancel: () => void; onSubmit: () => void; }>; const ReviewForm: FC = ({ workspacesToUpdate, + isProcessing, onCancel, onSubmit, }) => { @@ -163,7 +210,7 @@ const ReviewForm: FC = ({ // The workspaces don't have all necessary data by themselves, so we need to // fetch the unique template versions, and massage the results const groups = groupWorkspacesByTemplateVersionId(readyToUpdate); - const templateVersionQueries = useQueries({ + const templateVersionQueries = useSuspenseQueries({ queries: groups.map((g) => templateVersion(g.templateVersionId)), }); @@ -182,8 +229,9 @@ const ReviewForm: FC = ({ .map((ws) => ws.id), ); + const hasRunningWorkspaces = runningIds.size > 0; + const consequencesResolved = !hasRunningWorkspaces || acceptedConsequences; const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; - const consequencesResolved = runningIds.size === 0 || acceptedConsequences; const canSubmit = consequencesResolved && error === undefined && readyToUpdate.length > 0; @@ -220,13 +268,24 @@ const ReviewForm: FC = ({
    + {hasRunningWorkspaces && ( +
    + { + setAcceptedConsequences(newValue); + }} + /> +
    + )} + {readyToUpdate.length > 0 && (

    Ready to update

    - These workspaces require no additional action before - updating. + These workspaces require no additional build parameters to + update.

    @@ -349,6 +408,9 @@ export const BatchUpdateModalForm: FC = ({ onClose, onSubmit, }) => { + // Splitting up the component so that we (1) we only mount the state when + // we know a user actually opened the dialog content, and (2) so it's easy + // to add a Suspense boundary for the data fetching return ( = ({ }} > - {isProcessing ? ( - <> - Loading… - - - ) : ( + + Loading… + + + } + > { onSubmit(); onClose(); }} /> - )} + ); From 9b8f43759e05e09dcc47bb8b5d39a45adec946a1 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 2 Jul 2025 22:30:30 +0000 Subject: [PATCH 13/37] fix: remove stale data issues in check logic --- .../pages/WorkspacesPage/WorkspacesPage.tsx | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index d902d0c70391e..0aecb70afeb85 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -2,7 +2,6 @@ import { getErrorDetail, getErrorMessage } from "api/errors"; import { workspacePermissionsByOrganization } from "api/queries/organizations"; import { templates } from "api/queries/templates"; import { workspaces } from "api/queries/workspaces"; -import type { Workspace } from "api/typesGenerated"; import { useFilter } from "components/Filter/Filter"; import { useUserFilterMenu } from "components/Filter/UserFilter"; import { displayError } from "components/GlobalSnackbar/utils"; @@ -11,7 +10,7 @@ import { useEffectEvent } from "hooks/hookPolyfills"; import { usePagination } from "hooks/usePagination"; import { useDashboard } from "modules/dashboard/useDashboard"; import { useOrganizationsFilterMenu } from "modules/tableFiltering/options"; -import { type FC, useEffect, useMemo, useState } from "react"; +import { type FC, useMemo, useState } from "react"; import { Helmet } from "react-helmet-async"; import { useQuery, useQueryClient } from "react-query"; import { useSearchParams } from "react-router-dom"; @@ -88,25 +87,32 @@ const WorkspacesPage: FC = () => { }, }); - const [checkedWorkspaces, setCheckedWorkspaces] = useState< - readonly Workspace[] - >([]); + const [checkedWorkspaceIds, setCheckedWorkspaceIds] = useState( + new Set(), + ); + const checkedWorkspaces = + data?.workspaces.filter((w) => checkedWorkspaceIds.has(w.id)) ?? []; const [activeBatchAction, setActiveBatchAction] = useState(); const canCheckWorkspaces = entitlements.features.workspace_batch_actions.enabled; const batchActions = useBatchActions({ onSuccess: async () => { await refetch(); - setCheckedWorkspaces([]); + setCheckedWorkspaceIds(new Set()); }, }); - // We want to uncheck the selected workspaces always when the url changes - // because of filtering or pagination - // biome-ignore lint/correctness/useExhaustiveDependencies: consider refactoring - useEffect(() => { - setCheckedWorkspaces([]); - }, [searchParams]); + // We always want to uncheck the selected workspaces when the url changes, + // because it has filtering and pagination that can change which workspaces + // the user can interact with. Can probably make this logic more fine- + // grained, but we don't want to swap this to a useEffect, because that + // will introduce extra page-wide renders in one of the heaviest pages in + // the entire site + const [cachedParams, setCachedParams] = useState(searchParams); + if (cachedParams !== searchParams && checkedWorkspaceIds.size > 0) { + setCachedParams(searchParams); + setCheckedWorkspaceIds(new Set()); + } return ( <> @@ -118,7 +124,18 @@ const WorkspacesPage: FC = () => { canCreateTemplate={permissions.createTemplates} canChangeVersions={permissions.updateTemplates} checkedWorkspaces={checkedWorkspaces} - onCheckChange={setCheckedWorkspaces} + onCheckChange={(newWorkspaces) => { + setCheckedWorkspaceIds((current) => { + const newIds = newWorkspaces.map((ws) => ws.id); + const sameContent = + current.size === newIds.length && + newIds.every((id) => current.has(id)); + if (sameContent) { + return current; + } + return new Set(newIds); + }); + }} canCheckWorkspaces={canCheckWorkspaces} templates={filteredTemplates} templatesFetchStatus={templatesQuery.status} @@ -164,7 +181,7 @@ const WorkspacesPage: FC = () => { open={activeBatchAction === "update"} isProcessing={batchActions.isProcessing} workspacesToUpdate={checkedWorkspaces} - onClose={() => setActiveBatchAction(undefined)} + onCancel={() => setActiveBatchAction(undefined)} onSubmit={async () => { window.alert("Hooray!"); /** From 543c012b2480a5ba78800abda240116ff7387fee Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 2 Jul 2025 23:47:28 +0000 Subject: [PATCH 14/37] fix: move all checked reset logic to event handlers to avoid state sync issues/extra re-renders --- .../pages/WorkspacesPage/WorkspacesPage.tsx | 49 ++++++++++--------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 0aecb70afeb85..856c79652a1aa 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -39,13 +39,29 @@ type BatchAction = "delete" | "update"; const WorkspacesPage: FC = () => { const queryClient = useQueryClient(); - // If we use a useSearchParams for each hook, the values will not be in sync. - // So we have to use a single one, centralizing the values, and pass it to - // each hook. + // We have to be careful with how we use useSearchParams or any other + // derived hooks. The URL is global state, but useSearchParams gives back a + // different state value based on where the hook was mounted. If we call the + // hook in the other hooks, we'll have multiple conflicting sources of truth const [searchParams, setSearchParams] = useSafeSearchParams(); + // Always need to make sure that we reset the checked workspaces each time + // the filtering or pagination changes, as that will almost always change + // which workspaces are shown on screen and which can be interacted with + const [checkedWorkspaceIds, setCheckedWorkspaceIds] = useState( + new Set(), + ); + const resetChecked = () => { + setCheckedWorkspaceIds((current) => { + return current.size === 0 ? current : new Set(); + }); + }; + const pagination = usePagination({ searchParams, - onSearchParamsChange: setSearchParams, + onSearchParamsChange: (newParams) => { + setSearchParams(newParams); + resetChecked(); + }, }); const { permissions, user: me } = useAuthenticated(); const { entitlements } = useDashboard(); @@ -73,7 +89,10 @@ const WorkspacesPage: FC = () => { const filterProps = useWorkspacesFilter({ searchParams, onSearchParamsChange: setSearchParams, - onFilterChange: () => pagination.goToPage(1), + onFilterChange: () => { + pagination.goToPage(1); + resetChecked(); + }, }); const workspacesQueryOptions = workspaces({ @@ -87,32 +106,18 @@ const WorkspacesPage: FC = () => { }, }); - const [checkedWorkspaceIds, setCheckedWorkspaceIds] = useState( - new Set(), - ); - const checkedWorkspaces = - data?.workspaces.filter((w) => checkedWorkspaceIds.has(w.id)) ?? []; const [activeBatchAction, setActiveBatchAction] = useState(); const canCheckWorkspaces = entitlements.features.workspace_batch_actions.enabled; const batchActions = useBatchActions({ onSuccess: async () => { await refetch(); - setCheckedWorkspaceIds(new Set()); + resetChecked(); }, }); - // We always want to uncheck the selected workspaces when the url changes, - // because it has filtering and pagination that can change which workspaces - // the user can interact with. Can probably make this logic more fine- - // grained, but we don't want to swap this to a useEffect, because that - // will introduce extra page-wide renders in one of the heaviest pages in - // the entire site - const [cachedParams, setCachedParams] = useState(searchParams); - if (cachedParams !== searchParams && checkedWorkspaceIds.size > 0) { - setCachedParams(searchParams); - setCheckedWorkspaceIds(new Set()); - } + const checkedWorkspaces = + data?.workspaces.filter((w) => checkedWorkspaceIds.has(w.id)) ?? []; return ( <> From 1319ee776fe5808b236e6c0dcd9fd34dc75bfc2c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 00:20:45 +0000 Subject: [PATCH 15/37] wip: more progress --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 179 ++++++++++-------- 1 file changed, 104 insertions(+), 75 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 71304915ff239..912c598cae246 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,8 +1,16 @@ -import type { Workspace } from "api/typesGenerated"; -import { type FC, type ReactNode, Suspense, useState } from "react"; +import type { Workspace, WorkspaceStatus } from "api/typesGenerated"; +import { + type FC, + forwardRef, + type ReactNode, + Suspense, + useId, + useRef, + useState, +} from "react"; import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; import { Button } from "components/Button/Button"; -import { useSuspenseQueries } from "react-query"; +import { useQueries, useSuspenseQueries } from "react-query"; import { templateVersion } from "api/queries/templates"; import { Loader } from "components/Loader/Loader"; import { ErrorAlert } from "components/Alert/ErrorAlert"; @@ -11,6 +19,7 @@ import { cn } from "utils/cn"; import { Checkbox } from "components/Checkbox/Checkbox"; import { Badge } from "components/Badge/Badge"; import { Label } from "@radix-ui/react-label"; +import { Spinner } from "components/Spinner/Spinner"; /** * @todo Need to decide if we should include the template display name here, or @@ -133,7 +142,7 @@ const TemplateNameChange: FC = ({ return ( <> - Template: {oldTemplateName} → {newTemplateName} + {oldTemplateName} → {newTemplateName} Template {oldTemplateName} will be updated to template {newTemplateName} @@ -147,10 +156,10 @@ type RunningWorkspacesWarningProps = Readonly<{ onAcceptedConsequencesChange: (newValue: boolean) => void; }>; -const RunningWorkspacesWarning: FC = ({ - acceptedConsequences, - onAcceptedConsequencesChange, -}) => { +const RunningWorkspacesWarning = forwardRef< + HTMLButtonElement, + RunningWorkspacesWarningProps +>(({ acceptedConsequences, onAcceptedConsequencesChange }, ref) => { return (

    Running workspaces detected

    @@ -167,6 +176,7 @@ const RunningWorkspacesWarning: FC = ({
); -}; +}); + +// Used to force the user to acknowledge that batch updating has risks in +// certain situations and could destroy their data +type ConsequencesStage = "notAccepted" | "accepted" | "failedValidation"; + +const transitioningStatuses: readonly WorkspaceStatus[] = [ + "canceling", + "deleting", + "pending", + "starting", + "stopping", +]; type ReviewFormProps = Readonly<{ workspacesToUpdate: readonly Workspace[]; @@ -190,27 +212,31 @@ const ReviewForm: FC = ({ onCancel, onSubmit, }) => { - // We need to take a local snapshot of the workspaces that existed on mount - // because workspaces are such a mutable resource, and there's a chance that - // they can be changed by another user + be subject to a query invalidation - // while the form is open - const [cachedWorkspaces, setCachedWorkspaces] = useState(workspacesToUpdate); - // Used to force the user to acknowledge that batch updating has risks in - // certain situations and could destroy their data. Initial value - // deliberately *not* based on any derived values to avoid state sync issues - // as cachedWorkspaces gets refreshed - const [acceptedConsequences, setAcceptedConsequences] = useState(false); + const hookId = useId(); + const [stage, setStage] = useState("notAccepted"); + const checkboxRef = useRef(null); + + // We have to make sure that we don't let the user submit anything while + // workspaces are transitioning, or else we'll run into a race condition. + // If a user starts a workspace, and then immediately batch-updates it, the + // workspace won't be in the running state yet. We need to issue warnings + // about how updating running workspaces is a destructive action, but if + // the user goes through the form quickly enough, they'll be able to update + // without seeing the warning + const transitioning = workspacesToUpdate.filter((ws) => + transitioningStatuses.includes(ws.latest_build.status), + ); // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and // then skip over them for all other update logic const { dormant, noUpdateNeeded, readyToUpdate } = - separateWorkspacesByUpdateType(cachedWorkspaces); + separateWorkspacesByUpdateType(workspacesToUpdate); // The workspaces don't have all necessary data by themselves, so we need to // fetch the unique template versions, and massage the results const groups = groupWorkspacesByTemplateVersionId(readyToUpdate); - const templateVersionQueries = useSuspenseQueries({ + const templateVersionQueries = useQueries({ queries: groups.map((g) => templateVersion(g.templateVersionId)), }); @@ -219,19 +245,15 @@ const ReviewForm: FC = ({ // if any of the queries actively have an error const error = templateVersionQueries.find((q) => q.isError)?.error; - const merged = templateVersionQueries.every((q) => q.isSuccess) - ? templateVersionQueries.map((q) => q.data) - : undefined; - const runningIds = new Set( readyToUpdate .filter((ws) => ws.latest_build.status === "running") .map((ws) => ws.id), ); + const failedValidationId = `${hookId}-failed-validation`; const hasRunningWorkspaces = runningIds.size > 0; - const consequencesResolved = !hasRunningWorkspaces || acceptedConsequences; - const workspacesChangedWhileOpen = workspacesToUpdate !== cachedWorkspaces; + const consequencesResolved = !hasRunningWorkspaces || stage === "accepted"; const canSubmit = consequencesResolved && error === undefined && readyToUpdate.length > 0; @@ -240,7 +262,14 @@ const ReviewForm: FC = ({ className="max-h-[80vh]" onSubmit={(e) => { e.preventDefault(); - onSubmit(); + if (canSubmit) { + onSubmit(); + return; + } + if (stage === "notAccepted") { + setStage("failedValidation"); + checkboxRef.current?.scrollIntoView(); + } }} > {error !== undefined ? ( @@ -254,26 +283,15 @@ const ReviewForm: FC = ({ Review updates - -
{hasRunningWorkspaces && (
{ - setAcceptedConsequences(newValue); + acceptedConsequences={stage === "accepted"} + onAcceptedConsequencesChange={(newChecked) => { + const newStage = newChecked ? "accepted" : "notAccepted"; + setStage(newStage); }} />
@@ -379,13 +397,39 @@ const ReviewForm: FC = ({ )}
-
- - +
+
+ + +
+ + {stage === "failedValidation" && ( +

+ Please check the checkbox to continue. +

+ )}
)} @@ -394,10 +438,10 @@ const ReviewForm: FC = ({ }; type BatchUpdateModalFormProps = Readonly<{ - workspacesToUpdate: readonly Workspace[]; open: boolean; isProcessing: boolean; - onClose: () => void; + workspacesToUpdate: readonly Workspace[]; + onCancel: () => void; onSubmit: () => void; }>; @@ -405,40 +449,25 @@ export const BatchUpdateModalForm: FC = ({ open, isProcessing, workspacesToUpdate, - onClose, + onCancel, onSubmit, }) => { - // Splitting up the component so that we (1) we only mount the state when - // we know a user actually opened the dialog content, and (2) so it's easy - // to add a Suspense boundary for the data fetching return ( { if (open) { - onClose(); + onCancel(); } }} > - - Loading… - - - } - > - { - onSubmit(); - onClose(); - }} - /> - + ); From 035e8023716b5c406bd92e8ae5620b7809c28ce8 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 00:24:00 +0000 Subject: [PATCH 16/37] docs: rewrite comment for clarity --- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 856c79652a1aa..5129e7e08b475 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -40,9 +40,10 @@ type BatchAction = "delete" | "update"; const WorkspacesPage: FC = () => { const queryClient = useQueryClient(); // We have to be careful with how we use useSearchParams or any other - // derived hooks. The URL is global state, but useSearchParams gives back a - // different state value based on where the hook was mounted. If we call the - // hook in the other hooks, we'll have multiple conflicting sources of truth + // derived hooks. The URL is global state, but each call to useSearchParams + // creates a different, contradictory source of truth for what the URL + // should look like. We need to make sure that we only mount the hook once + // per page const [searchParams, setSearchParams] = useSafeSearchParams(); // Always need to make sure that we reset the checked workspaces each time // the filtering or pagination changes, as that will almost always change From 702b9b00944af387239b9b51e0fc0008094005ae Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 00:25:10 +0000 Subject: [PATCH 17/37] refactor: rename variable for accuracy --- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 5129e7e08b475..aaf411e194be7 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -87,7 +87,7 @@ const WorkspacesPage: FC = () => { }); }, [templatesQuery.data, workspacePermissionsQuery.data]); - const filterProps = useWorkspacesFilter({ + const filterState = useWorkspacesFilter({ searchParams, onSearchParamsChange: setSearchParams, onFilterChange: () => { @@ -98,7 +98,7 @@ const WorkspacesPage: FC = () => { const workspacesQueryOptions = workspaces({ ...pagination, - q: filterProps.filter.query, + q: filterState.filter.query, }); const { data, error, refetch } = useQuery({ ...workspacesQueryOptions, @@ -151,7 +151,7 @@ const WorkspacesPage: FC = () => { page={pagination.page} limit={pagination.limit} onPageChange={pagination.goToPage} - filterProps={filterProps} + filterProps={filterState} isRunningBatchAction={batchActions.isProcessing} onDeleteAll={() => setActiveBatchAction("delete")} onUpdateAll={() => setActiveBatchAction("update")} From 0e61af8f1409b7c28f7aaf932459eae18910ece3 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 01:36:42 +0000 Subject: [PATCH 18/37] fix: remove unused values --- site/src/api/queries/templates.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index 5135f2304426e..a09c3c9d6dc52 100644 --- a/site/src/api/queries/templates.ts +++ b/site/src/api/queries/templates.ts @@ -48,21 +48,6 @@ export const templates = ( }; }; -const getTemplatesByOrganizationQueryKey = ( - organization: string, - options?: GetTemplatesOptions, -) => [organization, "templates", options?.deprecated]; - -const templatesByOrganization = ( - organization: string, - options: GetTemplatesOptions = {}, -) => { - return { - queryKey: getTemplatesByOrganizationQueryKey(organization, options), - queryFn: () => API.getTemplatesByOrganization(organization, options), - }; -}; - export const templateACL = (templateId: string) => { return { queryKey: ["templateAcl", templateId], From 1d72f3679f42c09c901ea05b5acb3b67600a14cc Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 02:06:56 +0000 Subject: [PATCH 19/37] chore: add basic prefetch logic --- site/src/api/queries/templates.ts | 20 ++++++------ .../pages/WorkspacesPage/WorkspacesPage.tsx | 32 ++++++++++++++++--- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index a09c3c9d6dc52..fe42d4aa6854b 100644 --- a/site/src/api/queries/templates.ts +++ b/site/src/api/queries/templates.ts @@ -106,9 +106,11 @@ export const templateExamples = () => { }; }; +export const templateVersionRoot: string = "templateVersion" + export const templateVersion = (versionId: string) => { return { - queryKey: ["templateVersion", versionId], + queryKey: [templateVersionRoot, versionId], queryFn: () => API.getTemplateVersion(versionId), }; }; @@ -119,7 +121,7 @@ export const templateVersionByName = ( versionName: string, ) => { return { - queryKey: ["templateVersion", organizationId, templateName, versionName], + queryKey: [templateVersionRoot, organizationId, templateName, versionName], queryFn: () => API.getTemplateVersionByName(organizationId, templateName, versionName), }; @@ -138,7 +140,7 @@ export const templateVersions = (templateId: string) => { }; export const templateVersionVariablesKey = (versionId: string) => [ - "templateVersion", +templateVersionRoot, versionId, "variables", ]; @@ -201,7 +203,7 @@ export const templaceACLAvailable = ( }; const templateVersionExternalAuthKey = (versionId: string) => [ - "templateVersion", +templateVersionRoot, versionId, "externalAuth", ]; @@ -242,21 +244,21 @@ const createTemplateFn = async (options: CreateTemplateOptions) => { export const templateVersionLogs = (versionId: string) => { return { - queryKey: ["templateVersion", versionId, "logs"], + queryKey: [templateVersionRoot, versionId, "logs"], queryFn: () => API.getTemplateVersionLogs(versionId), }; }; export const richParameters = (versionId: string) => { return { - queryKey: ["templateVersion", versionId, "richParameters"], + queryKey: [templateVersionRoot, versionId, "richParameters"], queryFn: () => API.getTemplateVersionRichParameters(versionId), }; }; export const resources = (versionId: string) => { return { - queryKey: ["templateVersion", versionId, "resources"], + queryKey: [templateVersionRoot, versionId, "resources"], queryFn: () => API.getTemplateVersionResources(versionId), }; }; @@ -278,7 +280,7 @@ export const previousTemplateVersion = ( ) => { return { queryKey: [ - "templateVersion", +templateVersionRoot, organizationId, templateName, versionName, @@ -298,7 +300,7 @@ export const previousTemplateVersion = ( export const templateVersionPresets = (versionId: string) => { return { - queryKey: ["templateVersion", versionId, "presets"], + queryKey: [templateVersionRoot, versionId, "presets"], queryFn: () => API.getTemplateVersionPresets(versionId), }; }; diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index aaf411e194be7..8d3c841565939 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -1,6 +1,10 @@ import { getErrorDetail, getErrorMessage } from "api/errors"; import { workspacePermissionsByOrganization } from "api/queries/organizations"; -import { templates } from "api/queries/templates"; +import { + templates, + templateVersion, + templateVersionRoot, +} from "api/queries/templates"; import { workspaces } from "api/queries/workspaces"; import { useFilter } from "components/Filter/Filter"; import { useUserFilterMenu } from "components/Filter/UserFilter"; @@ -154,9 +158,29 @@ const WorkspacesPage: FC = () => { filterProps={filterState} isRunningBatchAction={batchActions.isProcessing} onDeleteAll={() => setActiveBatchAction("delete")} - onUpdateAll={() => setActiveBatchAction("update")} onStartAll={() => batchActions.start(checkedWorkspaces)} onStopAll={() => batchActions.stop(checkedWorkspaces)} + onUpdateAll={() => { + // Just because batch-updating can be really dangerous + // action for running workspaces, we're going to invalidate + // all relevant queries as a prefetch strategy before the + // modal content is even allowed to mount. + for (const ws of checkedWorkspaces) { + // Our data layer is a little messy right now, so + // there's no great way to invalidate a bunch of + // template version queries with a single function call, + // while also avoiding all other tangentially connected + // resources that use the same key pattern. Have to be + // super granular and make one call per workspace. + queryClient.invalidateQueries({ + queryKey: [templateVersionRoot, ws.template_active_version_id], + type: "all", + exact: true, + stale: true, + }); + } + setActiveBatchAction("update"); + }} onActionSuccess={async () => { await queryClient.invalidateQueries({ queryKey: workspacesQueryOptions.queryKey, @@ -174,13 +198,11 @@ const WorkspacesPage: FC = () => { isLoading={batchActions.isProcessing} checkedWorkspaces={checkedWorkspaces} open={activeBatchAction === "delete"} + onClose={() => setActiveBatchAction(undefined)} onConfirm={async () => { await batchActions.delete(checkedWorkspaces); setActiveBatchAction(undefined); }} - onClose={() => { - setActiveBatchAction(undefined); - }} /> Date: Thu, 3 Jul 2025 02:09:00 +0000 Subject: [PATCH 20/37] wip: commit more progress --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 172 +++++++++--------- 1 file changed, 88 insertions(+), 84 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 912c598cae246..d573a87918752 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,61 +1,27 @@ -import type { Workspace, WorkspaceStatus } from "api/typesGenerated"; +import { Label } from "@radix-ui/react-label"; +import { templateVersion } from "api/queries/templates"; +import type { + TemplateVersion, + Workspace, + WorkspaceStatus, +} from "api/typesGenerated"; +import { ErrorAlert } from "components/Alert/ErrorAlert"; +import { Avatar } from "components/Avatar/Avatar"; +import { Badge } from "components/Badge/Badge"; +import { Button } from "components/Button/Button"; +import { Checkbox } from "components/Checkbox/Checkbox"; +import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; +import { Spinner } from "components/Spinner/Spinner"; import { type FC, - forwardRef, type ReactNode, - Suspense, + forwardRef, useId, useRef, useState, } from "react"; -import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; -import { Button } from "components/Button/Button"; -import { useQueries, useSuspenseQueries } from "react-query"; -import { templateVersion } from "api/queries/templates"; -import { Loader } from "components/Loader/Loader"; -import { ErrorAlert } from "components/Alert/ErrorAlert"; -import { Avatar } from "components/Avatar/Avatar"; +import { useQueries, type UseQueryOptions } from "react-query"; import { cn } from "utils/cn"; -import { Checkbox } from "components/Checkbox/Checkbox"; -import { Badge } from "components/Badge/Badge"; -import { Label } from "@radix-ui/react-label"; -import { Spinner } from "components/Spinner/Spinner"; - -/** - * @todo Need to decide if we should include the template display name here, or - * if we'll be able to get that data as part of other fetches. It's also - * possible that the new UX might not require it at all? - */ -type TemplateVersionGroup = Readonly<{ - templateVersionId: string; - affectedWorkspaces: readonly Workspace[]; -}>; - -function groupWorkspacesByTemplateVersionId( - workspaces: readonly Workspace[], -): readonly TemplateVersionGroup[] { - const grouped = new Map(); - - for (const ws of workspaces) { - const templateVersionId = ws.template_active_version_id; - const value = grouped.get(templateVersionId); - if (value !== undefined) { - // Need to do type assertion to make value mutable as an - // implementation detail. Doing things the "proper" way adds a bunch - // of needless boilerplate for a single-line computation - const target = value.affectedWorkspaces as Workspace[]; - target.push(ws); - continue; - } - - grouped.set(templateVersionId, { - templateVersionId, - affectedWorkspaces: [ws], - }); - } - - return [...grouped.values()]; -} type UpdateTypePartition = Readonly<{ dormant: readonly Workspace[]; @@ -88,7 +54,8 @@ function separateWorkspacesByUpdateType( type WorkspacePanelProps = Readonly<{ workspaceName: string; workspaceIconUrl: string; - running?: boolean; + running: boolean; + transitioning: boolean; label?: ReactNode; adornment?: ReactNode; className?: string; @@ -98,11 +65,13 @@ const ReviewPanel: FC = ({ workspaceName, label, running, + transitioning, workspaceIconUrl, className, }) => { - // Preemptively adding border to this component so that it still looks - // decent when used outside of a list + // Preemptively adding border to this component to help decouple the styling + // from the rest of the components in this file, and make the core parts of + // this component easier to reason about return (
= ({ Running )} + {transitioning && ( + + Getting latest status + + )} {label} @@ -131,21 +105,22 @@ const ReviewPanel: FC = ({ }; type TemplateNameChangeProps = Readonly<{ - oldTemplateName: string; - newTemplateName: string; + oldTemplateVersionName: string; + newTemplateVersionName: string; }>; const TemplateNameChange: FC = ({ - oldTemplateName, - newTemplateName, + oldTemplateVersionName: oldTemplateName, + newTemplateVersionName: newTemplateName, }) => { return ( <> - + {oldTemplateName} → {newTemplateName} - Template {oldTemplateName} will be updated to template {newTemplateName} + Workspace will go from version {oldTemplateName} to version{" "} + {newTemplateName} ); @@ -191,6 +166,13 @@ const RunningWorkspacesWarning = forwardRef< // certain situations and could destroy their data type ConsequencesStage = "notAccepted" | "accepted" | "failedValidation"; +// We have to make sure that we don't let the user submit anything while +// workspaces are transitioning, or else we'll run into a race condition. If a +// user starts a workspace, and then immediately batch-updates it, the workspace +// won't be in the running state yet. We need to issue warnings about how +// updating running workspaces is a destructive action, but if the user goes +// through the form quickly enough, they'll be able to update without seeing the +// warning. const transitioningStatuses: readonly WorkspaceStatus[] = [ "canceling", "deleting", @@ -216,17 +198,6 @@ const ReviewForm: FC = ({ const [stage, setStage] = useState("notAccepted"); const checkboxRef = useRef(null); - // We have to make sure that we don't let the user submit anything while - // workspaces are transitioning, or else we'll run into a race condition. - // If a user starts a workspace, and then immediately batch-updates it, the - // workspace won't be in the running state yet. We need to issue warnings - // about how updating running workspaces is a destructive action, but if - // the user goes through the form quickly enough, they'll be able to update - // without seeing the warning - const transitioning = workspacesToUpdate.filter((ws) => - transitioningStatuses.includes(ws.latest_build.status), - ); - // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and // then skip over them for all other update logic @@ -235,9 +206,11 @@ const ReviewForm: FC = ({ // The workspaces don't have all necessary data by themselves, so we need to // fetch the unique template versions, and massage the results - const groups = groupWorkspacesByTemplateVersionId(readyToUpdate); + const uniqueTemplateVersionIds = new Set( + readyToUpdate.map((ws) => ws.template_active_version_id), + ); const templateVersionQueries = useQueries({ - queries: groups.map((g) => templateVersion(g.templateVersionId)), + queries: [...uniqueTemplateVersionIds].map((id) => templateVersion(id)), }); // React Query persists previous errors even if a query is no longer in the @@ -251,10 +224,20 @@ const ReviewForm: FC = ({ .map((ws) => ws.id), ); + // Just to be on the safe side, we need to derive the IDs from all checked + // workspaces, because the separation result could theoretically change + // after the transitions end + const transitioningIds = new Set( + workspacesToUpdate + .filter((ws) => transitioningStatuses.includes(ws.latest_build.status)) + .map((ws) => ws.id), + ); + const failedValidationId = `${hookId}-failed-validation`; const hasRunningWorkspaces = runningIds.size > 0; const consequencesResolved = !hasRunningWorkspaces || stage === "accepted"; - const canSubmit = + const submitButtonDisabled = isProcessing || transitioningIds.size > 0; + const submitIsPossible = consequencesResolved && error === undefined && readyToUpdate.length > 0; return ( @@ -262,13 +245,16 @@ const ReviewForm: FC = ({ className="max-h-[80vh]" onSubmit={(e) => { e.preventDefault(); - if (canSubmit) { + if (submitIsPossible) { onSubmit(); return; } if (stage === "notAccepted") { setStage("failedValidation"); - checkboxRef.current?.scrollIntoView(); + // Makes sure that if the modal is long enough to scroll + // that the checkbox isn't on screen anymore, it goes back + // to being on screen + checkboxRef.current?.scrollIntoView({ behavior: "smooth" }); } }} > @@ -290,8 +276,11 @@ const ReviewForm: FC = ({ { - const newStage = newChecked ? "accepted" : "notAccepted"; - setStage(newStage); + if (newChecked) { + setStage("accepted"); + } else { + setStage("notAccepted"); + } }} />
@@ -302,8 +291,8 @@ const ReviewForm: FC = ({

Ready to update

- These workspaces require no additional build parameters to - update. + These workspaces will have their templates be updated to the + latest version.

@@ -322,13 +311,14 @@ const ReviewForm: FC = ({ @@ -359,6 +349,8 @@ const ReviewForm: FC = ({ > @@ -387,6 +379,8 @@ const ReviewForm: FC = ({ > @@ -405,12 +399,12 @@ const ReviewForm: FC = ({
@@ -462,6 +456,16 @@ export const BatchUpdateModalForm: FC = ({ }} > + {/* + * Because of how the Dialog component works, we need to make + * sure that at least the parent stays mounted at all times. But + * if we move all the state into ReviewForm, that means that its + * state only mounts when the user actually opens up the batch + * update form. That saves us from mounting a bunch of extra + * state and firing extra queries, when realistically, the form + * will stay closed 99% of the time the user is on the + * workspaces page. + */} Date: Thu, 3 Jul 2025 02:14:59 +0000 Subject: [PATCH 21/37] fix: add missing a11y description --- site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index d573a87918752..6151c2568e54b 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -10,7 +10,12 @@ import { Avatar } from "components/Avatar/Avatar"; import { Badge } from "components/Badge/Badge"; import { Button } from "components/Button/Button"; import { Checkbox } from "components/Checkbox/Checkbox"; -import { Dialog, DialogContent, DialogTitle } from "components/Dialog/Dialog"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogTitle, +} from "components/Dialog/Dialog"; import { Spinner } from "components/Spinner/Spinner"; import { type FC, @@ -269,6 +274,9 @@ const ReviewForm: FC = ({ Review updates + + The following workspaces will be updated: +
{hasRunningWorkspaces && ( From 08e4faf87a016018dc8e93736aac9491d6646dc8 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 02:16:01 +0000 Subject: [PATCH 22/37] fix: remove unused imports --- site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 6151c2568e54b..e4d40f52e5230 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,10 +1,6 @@ import { Label } from "@radix-ui/react-label"; import { templateVersion } from "api/queries/templates"; -import type { - TemplateVersion, - Workspace, - WorkspaceStatus, -} from "api/typesGenerated"; +import type { Workspace, WorkspaceStatus } from "api/typesGenerated"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Avatar } from "components/Avatar/Avatar"; import { Badge } from "components/Badge/Badge"; @@ -25,7 +21,7 @@ import { useRef, useState, } from "react"; -import { useQueries, type UseQueryOptions } from "react-query"; +import { useQueries } from "react-query"; import { cn } from "utils/cn"; type UpdateTypePartition = Readonly<{ From e34426fa91fa2036714c7a78559e28707626c553 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 02:20:48 +0000 Subject: [PATCH 23/37] fix: make sure container overflows properly --- site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index e4d40f52e5230..3c392a63f6165 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -243,7 +243,7 @@ const ReviewForm: FC = ({ return ( { e.preventDefault(); if (submitIsPossible) { From bf0b18a964fc1222640d6b725439ffa0367588fa Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 02:27:54 +0000 Subject: [PATCH 24/37] fix: update scroll behavior --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 44 +++++++++++++------ 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 3c392a63f6165..ce0c554dde26f 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -15,6 +15,7 @@ import { import { Spinner } from "components/Spinner/Spinner"; import { type FC, + ForwardedRef, type ReactNode, forwardRef, useId, @@ -130,14 +131,21 @@ const TemplateNameChange: FC = ({ type RunningWorkspacesWarningProps = Readonly<{ acceptedConsequences: boolean; onAcceptedConsequencesChange: (newValue: boolean) => void; + checkboxRef: ForwardedRef; + containerRef: ForwardedRef; }>; -const RunningWorkspacesWarning = forwardRef< - HTMLButtonElement, - RunningWorkspacesWarningProps ->(({ acceptedConsequences, onAcceptedConsequencesChange }, ref) => { +const RunningWorkspacesWarning: FC = ({ + acceptedConsequences, + onAcceptedConsequencesChange, + checkboxRef, + containerRef, +}) => { return ( -
+

Running workspaces detected

  • @@ -152,7 +160,7 @@ const RunningWorkspacesWarning = forwardRef<
); -}); +}; // Used to force the user to acknowledge that batch updating has risks in // certain situations and could destroy their data @@ -197,6 +205,7 @@ const ReviewForm: FC = ({ }) => { const hookId = useId(); const [stage, setStage] = useState("notAccepted"); + const consequencesContainerRef = useRef(null); const checkboxRef = useRef(null); // Dormant workspaces can't be activated without activating them first. For @@ -250,13 +259,18 @@ const ReviewForm: FC = ({ onSubmit(); return; } - if (stage === "notAccepted") { - setStage("failedValidation"); - // Makes sure that if the modal is long enough to scroll - // that the checkbox isn't on screen anymore, it goes back - // to being on screen - checkboxRef.current?.scrollIntoView({ behavior: "smooth" }); + if (stage === "accepted") { + return; } + + setStage("failedValidation"); + // Makes sure that if the modal is long enough to scroll + // that the checkbox isn't on screen anymore, it goes back + // to being on screen + consequencesContainerRef.current?.scrollIntoView({ + behavior: "smooth", + }); + checkboxRef.current?.focus(); }} > {error !== undefined ? ( @@ -278,6 +292,8 @@ const ReviewForm: FC = ({ {hasRunningWorkspaces && (
{ if (newChecked) { @@ -301,7 +317,7 @@ const ReviewForm: FC = ({
    - {readyToUpdate.map((ws) => { + {[...readyToUpdate, ...readyToUpdate].map((ws) => { const matchedQuery = templateVersionQueries.find( (q) => q.data?.id === ws.template_active_version_id, ); From 62ccd6b8b1d8839ae2d0beaa4b87dc60c367e4ed Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 02:31:46 +0000 Subject: [PATCH 25/37] fix: remove code that was for debug --- site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index ce0c554dde26f..8eb568bbe5c50 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -317,7 +317,7 @@ const ReviewForm: FC = ({
    - {[...readyToUpdate, ...readyToUpdate].map((ws) => { + {readyToUpdate.map((ws) => { const matchedQuery = templateVersionQueries.find( (q) => q.data?.id === ws.template_active_version_id, ); From 52b07d046a96678d5a7568242754067f260cebbd Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 14:52:35 +0000 Subject: [PATCH 26/37] refactor: rename props for clarity --- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 4 ++-- site/src/pages/WorkspacesPage/WorkspacesPageView.tsx | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 8d3c841565939..3312918ecdcd6 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -157,10 +157,10 @@ const WorkspacesPage: FC = () => { onPageChange={pagination.goToPage} filterProps={filterState} isRunningBatchAction={batchActions.isProcessing} - onDeleteAll={() => setActiveBatchAction("delete")} + onBatchDeleteStart={() => setActiveBatchAction("delete")} onStartAll={() => batchActions.start(checkedWorkspaces)} onStopAll={() => batchActions.stop(checkedWorkspaces)} - onUpdateAll={() => { + onBatchUpdateStart={() => { // Just because batch-updating can be really dangerous // action for running workspaces, we're going to invalidate // all relevant queries as a prefetch strategy before the diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx index d2f440a1e1fcc..f795d9a97fa5f 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx @@ -51,8 +51,8 @@ interface WorkspacesPageViewProps { onPageChange: (page: number) => void; onCheckChange: (checkedWorkspaces: readonly Workspace[]) => void; isRunningBatchAction: boolean; - onDeleteAll: () => void; - onUpdateAll: () => void; + onBatchDeleteStart: () => void; + onBatchUpdateStart: () => void; onStartAll: () => void; onStopAll: () => void; canCheckWorkspaces: boolean; @@ -74,8 +74,8 @@ export const WorkspacesPageView: FC = ({ page, checkedWorkspaces, onCheckChange, - onDeleteAll, - onUpdateAll, + onBatchDeleteStart, + onBatchUpdateStart, onStopAll, onStartAll, isRunningBatchAction, @@ -170,7 +170,7 @@ export const WorkspacesPageView: FC = ({ Stop - + = ({ Delete… From d47eba3e5f77f0ca8c26a877bc82a7096ab8dba4 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 14:56:30 +0000 Subject: [PATCH 27/37] refactor: choose better names --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 8eb568bbe5c50..2a36eeae5c989 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -15,9 +15,8 @@ import { import { Spinner } from "components/Spinner/Spinner"; import { type FC, - ForwardedRef, + type ForwardedRef, type ReactNode, - forwardRef, useId, useRef, useState, @@ -206,7 +205,7 @@ const ReviewForm: FC = ({ const hookId = useId(); const [stage, setStage] = useState("notAccepted"); const consequencesContainerRef = useRef(null); - const checkboxRef = useRef(null); + const consequencesCheckboxRef = useRef(null); // Dormant workspaces can't be activated without activating them first. For // now, we'll only show the user that some workspaces can't be updated, and @@ -246,6 +245,12 @@ const ReviewForm: FC = ({ const failedValidationId = `${hookId}-failed-validation`; const hasRunningWorkspaces = runningIds.size > 0; const consequencesResolved = !hasRunningWorkspaces || stage === "accepted"; + + // For UX/accessibility reasons, we're splitting hairs between whether a + // form submission seems possible, versus whether clicking the button will + // give the user useful results/feedback. If we do a blanket disable for the + // button, there's many cases where there's no way to give them feedback + // on how to get themselves unstuck. const submitButtonDisabled = isProcessing || transitioningIds.size > 0; const submitIsPossible = consequencesResolved && error === undefined && readyToUpdate.length > 0; @@ -264,13 +269,13 @@ const ReviewForm: FC = ({ } setStage("failedValidation"); - // Makes sure that if the modal is long enough to scroll - // that the checkbox isn't on screen anymore, it goes back - // to being on screen + // Makes sure that if the modal is long enough to scroll and if + // the warning section checkbox isn't on screen anymore, the + // warning section goes back to being on screen consequencesContainerRef.current?.scrollIntoView({ behavior: "smooth", }); - checkboxRef.current?.focus(); + consequencesCheckboxRef.current?.focus(); }} > {error !== undefined ? ( @@ -292,7 +297,7 @@ const ReviewForm: FC = ({ {hasRunningWorkspaces && (
    { @@ -483,7 +488,7 @@ export const BatchUpdateModalForm: FC = ({ * state only mounts when the user actually opens up the batch * update form. That saves us from mounting a bunch of extra * state and firing extra queries, when realistically, the form - * will stay closed 99% of the time the user is on the + * will stay closed 99% of the time while the user is on the * workspaces page. */} Date: Thu, 3 Jul 2025 15:15:50 +0000 Subject: [PATCH 28/37] refactor: rename props again --- .../pages/WorkspacesPage/WorkspacesPage.tsx | 11 ++++----- .../WorkspacesPage/WorkspacesPageView.tsx | 24 +++++++++---------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 3312918ecdcd6..bb1d9a38b91d9 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -157,10 +157,10 @@ const WorkspacesPage: FC = () => { onPageChange={pagination.goToPage} filterProps={filterState} isRunningBatchAction={batchActions.isProcessing} - onBatchDeleteStart={() => setActiveBatchAction("delete")} - onStartAll={() => batchActions.start(checkedWorkspaces)} - onStopAll={() => batchActions.stop(checkedWorkspaces)} - onBatchUpdateStart={() => { + onBatchDeleteTransition={() => setActiveBatchAction("delete")} + onBatchStartTransition={() => batchActions.start(checkedWorkspaces)} + onBatchStopTransition={() => batchActions.stop(checkedWorkspaces)} + onBatchUpdateTransition={() => { // Just because batch-updating can be really dangerous // action for running workspaces, we're going to invalidate // all relevant queries as a prefetch strategy before the @@ -174,9 +174,8 @@ const WorkspacesPage: FC = () => { // super granular and make one call per workspace. queryClient.invalidateQueries({ queryKey: [templateVersionRoot, ws.template_active_version_id], - type: "all", exact: true, - stale: true, + type: "all", }); } setActiveBatchAction("update"); diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx index f795d9a97fa5f..1921b8f501a2c 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx @@ -51,10 +51,10 @@ interface WorkspacesPageViewProps { onPageChange: (page: number) => void; onCheckChange: (checkedWorkspaces: readonly Workspace[]) => void; isRunningBatchAction: boolean; - onBatchDeleteStart: () => void; - onBatchUpdateStart: () => void; - onStartAll: () => void; - onStopAll: () => void; + onBatchDeleteTransition: () => void; + onBatchUpdateTransition: () => void; + onBatchStartTransition: () => void; + onBatchStopTransition: () => void; canCheckWorkspaces: boolean; templatesFetchStatus: TemplateQuery["status"]; templates: TemplateQuery["data"]; @@ -74,10 +74,10 @@ export const WorkspacesPageView: FC = ({ page, checkedWorkspaces, onCheckChange, - onBatchDeleteStart, - onBatchUpdateStart, - onStopAll, - onStartAll, + onBatchDeleteTransition, + onBatchUpdateTransition, + onBatchStopTransition, + onBatchStartTransition, isRunningBatchAction, canCheckWorkspaces, templates, @@ -155,7 +155,7 @@ export const WorkspacesPageView: FC = ({ !mustUpdateWorkspace(w, canChangeVersions), ) } - onClick={onStartAll} + onClick={onBatchStartTransition} > Start @@ -165,12 +165,12 @@ export const WorkspacesPageView: FC = ({ (w) => w.latest_build.status === "running", ) } - onClick={onStopAll} + onClick={onBatchStopTransition} > Stop - + = ({ Delete… From 48d42b796dfe505e55307b5cb9c45fcf273f0246 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 17:16:43 +0000 Subject: [PATCH 29/37] refactor: start teasing apart components --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 307 +++++++++--------- 1 file changed, 154 insertions(+), 153 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 2a36eeae5c989..dbca930974bc6 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -245,6 +245,7 @@ const ReviewForm: FC = ({ const failedValidationId = `${hookId}-failed-validation`; const hasRunningWorkspaces = runningIds.size > 0; const consequencesResolved = !hasRunningWorkspaces || stage === "accepted"; + const showForm = readyToUpdate.length > 0 && error === undefined; // For UX/accessibility reasons, we're splitting hairs between whether a // form submission seems possible, versus whether clicking the button will @@ -281,177 +282,177 @@ const ReviewForm: FC = ({ {error !== undefined ? ( ) : ( - <> -
    -
    - -

    - Review updates -

    -     - - The following workspaces will be updated: - -
    +
    +
    + +

    + Review updates +

    +     + + The following workspaces will be updated: + +
    - {hasRunningWorkspaces && ( -
    - { - if (newChecked) { - setStage("accepted"); - } else { - setStage("notAccepted"); - } - }} - /> + {hasRunningWorkspaces && ( +
    + { + if (newChecked) { + setStage("accepted"); + } else { + setStage("notAccepted"); + } + }} + /> +
    + )} + + {readyToUpdate.length > 0 && ( +
    +
    +

    Ready to update

    +

    + These workspaces will have their templates be updated to the + latest version. +

    - )} - {readyToUpdate.length > 0 && ( -
    -
    -

    Ready to update

    -

    - These workspaces will have their templates be updated to the - latest version. -

    -
    - -
      - {readyToUpdate.map((ws) => { - const matchedQuery = templateVersionQueries.find( - (q) => q.data?.id === ws.template_active_version_id, - ); - const newTemplateName = matchedQuery?.data?.name; - - return ( -
    • - - ) - } - /> -
      • - ); - })} -
    -
    - )} - - {noUpdateNeeded.length > 0 && ( -
    -
    -

    Already updated

    -

    - These workspaces are already updated and will be skipped. -

    -
    - -
      - {noUpdateNeeded.map((ws) => ( -
    • - -
      • - ))} -
    -
    - )} +
      + {readyToUpdate.map((ws) => { + const matchedQuery = templateVersionQueries.find( + (q) => q.data?.id === ws.template_active_version_id, + ); + const newTemplateName = matchedQuery?.data?.name; - {dormant.length > 0 && ( -
      -
      -

      Dormant workspaces

      -

      - Dormant workspaces cannot be updated without first - activating the workspace. They will be skipped during the - batch update. -

      -
      - -
        - {dormant.map((ws) => ( + return (
      • + ) + } />
        • - ))} -
      -
      - )} -
    + ); + })} +
+ + )} + + {noUpdateNeeded.length > 0 && ( +
+
+

Already updated

+

+ These workspaces are already updated and will be skipped. +

+
-
-
- - -
+
    + {noUpdateNeeded.map((ws) => ( +
  • + +
    • + ))} +
+
+ )} + + {dormant.length > 0 && ( +
+
+

Dormant workspaces

+

+ Dormant workspaces cannot be updated without first activating + the workspace. They will be skipped during the batch update. +

+
- {stage === "failedValidation" && ( -

- Please check the checkbox to continue. -

- )} -
- +
    + {dormant.map((ws) => ( +
  • + +
    • + ))} +
+ + )} +
)} + +
+
+ + + {showForm && ( + + )} +
+ + {stage === "failedValidation" && ( +

+ Please acknowledge consequence to continue. +

+ )} +
); }; From f7d2e1f5c92453bb8ae2eaf6ab043a54f7f2d2be Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 17:59:59 +0000 Subject: [PATCH 30/37] fix: resolve markup concerns for forms vs non-forms --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 372 +++++++++++------- 1 file changed, 221 insertions(+), 151 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index dbca930974bc6..b8e0e7d13b04d 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -16,6 +16,7 @@ import { Spinner } from "components/Spinner/Spinner"; import { type FC, type ForwardedRef, + PropsWithChildren, type ReactNode, useId, useRef, @@ -170,6 +171,58 @@ const RunningWorkspacesWarning: FC = ({ ); }; +type MainContainerProps = Readonly< + PropsWithChildren<{ + headerText: ReactNode; + description: ReactNode; + showDescription?: boolean; + }> +>; +const MainContainer: FC = ({ + children, + headerText, + description, + showDescription = false, +}) => { + return ( +
+
+ +

+ {headerText} +

+ + + + {description} + +
+ + {children} +
+ ); +}; + +type ContainerFooterProps = Readonly< + PropsWithChildren<{ + className?: string; + }> +>; +const ContainerFooter: FC = ({ children, className }) => { + return ( +
+ {children} +
+ ); +}; + // Used to force the user to acknowledge that batch updating has risks in // certain situations and could destroy their data type ConsequencesStage = "notAccepted" | "accepted" | "failedValidation"; @@ -227,6 +280,32 @@ const ReviewForm: FC = ({ // if any of the queries actively have an error const error = templateVersionQueries.find((q) => q.isError)?.error; + // The setup here is a little wonky because of accessibility. We need to + // make sure that we never mount a form if there's nothing for the user to + // ever meaningfully submit, but conditionally sharing styling/elements + // between form content and non-form content is awkward + const showForm = false && (readyToUpdate.length > 0 || dormant.length > 0); + if (!showForm) { + return ( + // Top-level styles should stay in sync with
below +
+ + {error !== undefined && } + + + + + +
+ ); + } + const runningIds = new Set( readyToUpdate .filter((ws) => ws.latest_build.status === "running") @@ -235,7 +314,7 @@ const ReviewForm: FC = ({ // Just to be on the safe side, we need to derive the IDs from all checked // workspaces, because the separation result could theoretically change - // after the transitions end + // on re-render after any workspace state transitions end const transitioningIds = new Set( workspacesToUpdate .filter((ws) => transitioningStatuses.includes(ws.latest_build.status)) @@ -245,15 +324,14 @@ const ReviewForm: FC = ({ const failedValidationId = `${hookId}-failed-validation`; const hasRunningWorkspaces = runningIds.size > 0; const consequencesResolved = !hasRunningWorkspaces || stage === "accepted"; - const showForm = readyToUpdate.length > 0 && error === undefined; // For UX/accessibility reasons, we're splitting hairs between whether a - // form submission seems possible, versus whether clicking the button will - // give the user useful results/feedback. If we do a blanket disable for the + // form submission seems valid, versus whether clicking the button will give + // the user useful results/feedback. If we do a blanket disable for the // button, there's many cases where there's no way to give them feedback // on how to get themselves unstuck. const submitButtonDisabled = isProcessing || transitioningIds.size > 0; - const submitIsPossible = + const submitIsValid = consequencesResolved && error === undefined && readyToUpdate.length > 0; return ( @@ -261,7 +339,7 @@ const ReviewForm: FC = ({ className="max-h-[80vh] flex flex-col flex-nowrap" onSubmit={(e) => { e.preventDefault(); - if (submitIsPossible) { + if (submitIsValid) { onSubmit(); return; } @@ -279,169 +357,161 @@ const ReviewForm: FC = ({ consequencesCheckboxRef.current?.focus(); }} > - {error !== undefined ? ( - - ) : ( -
-
- -

- Review updates -

- - - The following workspaces will be updated: - -
- - {hasRunningWorkspaces && ( -
- { - if (newChecked) { - setStage("accepted"); - } else { - setStage("notAccepted"); - } - }} - /> -
- )} - - {readyToUpdate.length > 0 && ( -
-
-

Ready to update

-

- These workspaces will have their templates be updated to the - latest version. -

+ + {error !== undefined ? ( + + ) : ( + <> + {hasRunningWorkspaces && ( +
+ { + if (newChecked) { + setStage("accepted"); + } else { + setStage("notAccepted"); + } + }} + />
+ )} -
    - {readyToUpdate.map((ws) => { - const matchedQuery = templateVersionQueries.find( - (q) => q.data?.id === ws.template_active_version_id, - ); - const newTemplateName = matchedQuery?.data?.name; + {readyToUpdate.length > 0 && ( +
    +
    +

    Ready to update

    +

    + These workspaces will have their templates be updated to the + latest version. +

    +
    + +
      + {readyToUpdate.map((ws) => { + const matchedQuery = templateVersionQueries.find( + (q) => q.data?.id === ws.template_active_version_id, + ); + const newTemplateName = matchedQuery?.data?.name; + + return ( +
    • + + ) + } + /> +
      • + ); + })} +
    +
    + )} - return ( + {noUpdateNeeded.length > 0 && ( +
    +
    +

    Already updated

    +

    + These workspaces are already updated and will be skipped. +

    +
    + +
      + {noUpdateNeeded.map((ws) => (
    • - ) - } />
      • - ); - })} -
    -
    - )} - - {noUpdateNeeded.length > 0 && ( -
    -
    -

    Already updated

    -

    - These workspaces are already updated and will be skipped. -

    -
    - -
      - {noUpdateNeeded.map((ws) => ( -
    • - -
      • - ))} -
    -
    - )} - - {dormant.length > 0 && ( -
    -
    -

    Dormant workspaces

    -

    - Dormant workspaces cannot be updated without first activating - the workspace. They will be skipped during the batch update. -

    -
    + ))} +
+
+ )} -
    - {dormant.map((ws) => ( -
  • - -
    • - ))} -
- - )} -
- )} + {dormant.length > 0 && ( +
+
+

Dormant workspaces

+

+ Dormant workspaces cannot be updated without first + activating the workspace. They will be skipped during the + batch update. +

+
+ +
    + {dormant.map((ws) => ( +
  • + +
    • + ))} +
+
+ )} + + )} + -
+
+ - - {showForm && ( - - )}
{stage === "failedValidation" && ( @@ -452,7 +522,7 @@ const ReviewForm: FC = ({ Please acknowledge consequence to continue.

)} -
+ ); }; From 5a637cb17e14992c1072a603319da98c9a06bf2d Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 18:07:24 +0000 Subject: [PATCH 31/37] fix: adjust layout for clarity --- .../pages/WorkspacesPage/BatchUpdateModalForm.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index b8e0e7d13b04d..f812c22b6f783 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -185,7 +185,10 @@ const MainContainer: FC = ({ showDescription = false, }) => { return ( -
+ // Have to subtract padding via margin values and then add it back so + // that there's no risk of the scrollbar covering up content when the + // container gets tall enough to overflow +

@@ -214,7 +217,10 @@ const ContainerFooter: FC = ({ children, className }) => { return (
@@ -284,8 +290,8 @@ const ReviewForm: FC = ({ // make sure that we never mount a form if there's nothing for the user to // ever meaningfully submit, but conditionally sharing styling/elements // between form content and non-form content is awkward - const showForm = false && (readyToUpdate.length > 0 || dormant.length > 0); - if (!showForm) { + const formIsNeeded = readyToUpdate.length > 0 || dormant.length > 0; + if (!formIsNeeded) { return ( // Top-level styles should stay in sync with
below
From 12a0ff62dce282d9a003d557f3cb3933bf12d769 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 18:36:13 +0000 Subject: [PATCH 32/37] refactor: centralize list styling --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 224 +++++++++--------- 1 file changed, 117 insertions(+), 107 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index f812c22b6f783..30643f00b14ac 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -25,7 +25,7 @@ import { import { useQueries } from "react-query"; import { cn } from "utils/cn"; -type UpdateTypePartition = Readonly<{ +type WorkspacePartitionByUpdateType = Readonly<{ dormant: readonly Workspace[]; noUpdateNeeded: readonly Workspace[]; readyToUpdate: readonly Workspace[]; @@ -33,7 +33,7 @@ type UpdateTypePartition = Readonly<{ function separateWorkspacesByUpdateType( workspaces: readonly Workspace[], -): UpdateTypePartition { +): WorkspacePartitionByUpdateType { const noUpdateNeeded: Workspace[] = []; const dormant: Workspace[] = []; const readyToUpdate: Workspace[] = []; @@ -53,7 +53,7 @@ function separateWorkspacesByUpdateType( return { dormant, noUpdateNeeded, readyToUpdate }; } -type WorkspacePanelProps = Readonly<{ +type ReviewPanelProps = Readonly<{ workspaceName: string; workspaceIconUrl: string; running: boolean; @@ -63,7 +63,7 @@ type WorkspacePanelProps = Readonly<{ className?: string; }>; -const ReviewPanel: FC = ({ +const ReviewPanel: FC = ({ workspaceName, label, running, @@ -185,11 +185,11 @@ const MainContainer: FC = ({ showDescription = false, }) => { return ( - // Have to subtract padding via margin values and then add it back so - // that there's no risk of the scrollbar covering up content when the - // container gets tall enough to overflow -
-
+ // Have to subtract parent padding via margin values and then add it + // back as child padding so that there's no risk of the scrollbar + // covering up content when the container gets tall enough to overflow +
+

{headerText} @@ -229,6 +229,33 @@ const ContainerFooter: FC = ({ children, className }) => { ); }; +type WorkspacesListSectionProps = Readonly< + PropsWithChildren<{ + headerText: ReactNode; + description: ReactNode; + }> +>; +const WorkspacesListSection: FC = ({ + children, + headerText, + description, +}) => { + return ( +
+
+

{headerText}

+

+ {description} +

+
+ +
    + {children} +
+
+ ); +}; + // Used to force the user to acknowledge that batch updating has risks in // certain situations and could destroy their data type ConsequencesStage = "notAccepted" | "accepted" | "failedValidation"; @@ -372,124 +399,107 @@ const ReviewForm: FC = ({ ) : ( <> {hasRunningWorkspaces && ( -
- { - if (newChecked) { - setStage("accepted"); - } else { - setStage("notAccepted"); - } - }} - /> -
+ { + if (newChecked) { + setStage("accepted"); + } else { + setStage("notAccepted"); + } + }} + /> )} {readyToUpdate.length > 0 && ( -
-
-

Ready to update

-

- These workspaces will have their templates be updated to the - latest version. -

-
- -
    - {readyToUpdate.map((ws) => { - const matchedQuery = templateVersionQueries.find( - (q) => q.data?.id === ws.template_active_version_id, - ); - const newTemplateName = matchedQuery?.data?.name; - - return ( -
  • - - ) - } - /> -
    • - ); - })} -
-
- )} - - {noUpdateNeeded.length > 0 && ( -
-
-

Already updated

-

- These workspaces are already updated and will be skipped. -

-
- -
    - {noUpdateNeeded.map((ws) => ( + + {readyToUpdate.map((ws) => { + const matchedQuery = templateVersionQueries.find( + (q) => q.data?.id === ws.template_active_version_id, + ); + const newTemplateName = matchedQuery?.data?.name; + + return (
  • + ) + } />
    • - ))} -
-
+ ); + })} + + )} + + {noUpdateNeeded.length > 0 && ( + + {noUpdateNeeded.map((ws) => ( +
  • + +
    • + ))} +     )} {dormant.length > 0 && ( -
    -
    -

    Dormant workspaces

    -

    + Dormant workspaces cannot be updated without first activating the workspace. They will be skipped during the batch update. -

    -
    - -
      - {dormant.map((ws) => ( -
    • - -
      • - ))} -
    -
    + + } + > + {dormant.map((ws) => ( +
  • + +
    • + ))} + )} )} From 148f0c892dcc2a05ef0a36021e6be3c475df2330 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 18:44:43 +0000 Subject: [PATCH 33/37] fix: adjust spacing again --- site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 30643f00b14ac..96b327c2edc66 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -241,7 +241,7 @@ const WorkspacesListSection: FC = ({ description, }) => { return ( -
    +

    {headerText}

    From 24dc0f7e6ddc02e587a34dcb8b17692b10dda8b0 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 18:54:41 +0000 Subject: [PATCH 34/37] refactor: centralize container styling --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 347 +++++++++--------- 1 file changed, 181 insertions(+), 166 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 96b327c2edc66..5f753b057cb81 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -1,4 +1,5 @@ import { Label } from "@radix-ui/react-label"; +import { Slot } from "@radix-ui/react-slot"; import { templateVersion } from "api/queries/templates"; import type { Workspace, WorkspaceStatus } from "api/typesGenerated"; import { ErrorAlert } from "components/Alert/ErrorAlert"; @@ -171,14 +172,28 @@ const RunningWorkspacesWarning: FC = ({ ); }; -type MainContainerProps = Readonly< +type ContainerProps = Readonly< + PropsWithChildren<{ + asChild?: boolean; + }> +>; +const Container: FC = ({ children, asChild = false }) => { + const Wrapper = asChild ? Slot : "div"; + return ( + + {children} + + ); +}; + +type ContainerBodyProps = Readonly< PropsWithChildren<{ headerText: ReactNode; description: ReactNode; showDescription?: boolean; }> >; -const MainContainer: FC = ({ +const ContainerBody: FC = ({ children, headerText, description, @@ -320,22 +335,21 @@ const ReviewForm: FC = ({ const formIsNeeded = readyToUpdate.length > 0 || dormant.length > 0; if (!formIsNeeded) { return ( - // Top-level styles should stay in sync with below -

    - + {error !== undefined && } - + -
    + ); } @@ -368,178 +382,179 @@ const ReviewForm: FC = ({ consequencesResolved && error === undefined && readyToUpdate.length > 0; return ( - { - e.preventDefault(); - if (submitIsValid) { - onSubmit(); - return; - } - if (stage === "accepted") { - return; - } - - setStage("failedValidation"); - // Makes sure that if the modal is long enough to scroll and if - // the warning section checkbox isn't on screen anymore, the - // warning section goes back to being on screen - consequencesContainerRef.current?.scrollIntoView({ - behavior: "smooth", - }); - consequencesCheckboxRef.current?.focus(); - }} - > - + { + e.preventDefault(); + if (submitIsValid) { + onSubmit(); + return; + } + if (stage === "accepted") { + return; + } + + setStage("failedValidation"); + // Makes sure that if the modal is long enough to scroll and + // if the warning section checkbox isn't on screen anymore, + // the warning section goes back to being on screen + consequencesContainerRef.current?.scrollIntoView({ + behavior: "smooth", + }); + consequencesCheckboxRef.current?.focus(); + }} > - {error !== undefined ? ( - - ) : ( - <> - {hasRunningWorkspaces && ( - { - if (newChecked) { - setStage("accepted"); - } else { - setStage("notAccepted"); + + {error !== undefined ? ( + + ) : ( + <> + {hasRunningWorkspaces && ( + { + if (newChecked) { + setStage("accepted"); + } else { + setStage("notAccepted"); + } + }} + /> + )} + + {readyToUpdate.length > 0 && ( + + {readyToUpdate.map((ws) => { + const matchedQuery = templateVersionQueries.find( + (q) => q.data?.id === ws.template_active_version_id, + ); + const newTemplateName = matchedQuery?.data?.name; + + return ( +
  • + + ) + } + /> +
    • + ); + })} +     + )} + + {noUpdateNeeded.length > 0 && ( + + {noUpdateNeeded.map((ws) => ( +
  • + +
    • + ))} +     + )} + + {dormant.length > 0 && ( + + Dormant workspaces cannot be updated without first + activating the workspace. They will be skipped during the + batch update. + } - }} - /> - )} - - {readyToUpdate.length > 0 && ( - - {readyToUpdate.map((ws) => { - const matchedQuery = templateVersionQueries.find( - (q) => q.data?.id === ws.template_active_version_id, - ); - const newTemplateName = matchedQuery?.data?.name; - - return ( + > + {dormant.map((ws) => (
  • - ) - } />
    • - ); - })} -     - )} - - {noUpdateNeeded.length > 0 && ( - - {noUpdateNeeded.map((ws) => ( -
  • - -
    • - ))} -     - )} - - {dormant.length > 0 && ( - - Dormant workspaces cannot be updated without first - activating the workspace. They will be skipped during the - batch update. - - } - > - {dormant.map((ws) => ( -
  • - -
    • - ))} -     - )} - - )} -     - - -
    - - -
    - - {stage === "failedValidation" && ( -

    - Please acknowledge consequence to continue. -

    - )} -     - + ))} + + )} + + )} + + + +
    + + +
    + + {stage === "failedValidation" && ( +

    + Please acknowledge consequence to continue. +

    + )} +     + + ); }; From 534fab345c8dfc644c4b59b5023c123fe53e067a Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Jul 2025 20:41:24 +0000 Subject: [PATCH 35/37] chore: finish initial impementation --- .../WorkspacesPage/BatchUpdateModalForm.tsx | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx index 5f753b057cb81..355c1742605f3 100644 --- a/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx +++ b/site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx @@ -14,6 +14,7 @@ import { DialogTitle, } from "components/Dialog/Dialog"; import { Spinner } from "components/Spinner/Spinner"; +import { TriangleAlert } from "lucide-react"; import { type FC, type ForwardedRef, @@ -135,7 +136,6 @@ type RunningWorkspacesWarningProps = Readonly<{ checkboxRef: ForwardedRef; containerRef: ForwardedRef; }>; - const RunningWorkspacesWarning: FC = ({ acceptedConsequences, onAcceptedConsequencesChange, @@ -147,8 +147,12 @@ const RunningWorkspacesWarning: FC = ({ ref={containerRef} className="rounded-md border-border-warning border border-solid p-4" > -

    Running workspaces detected

    -
      +

      + + Running workspaces detected +

      + +
      • Updating a workspace will start it on its latest template version. This can delete non-persistent data. @@ -159,10 +163,10 @@ const RunningWorkspacesWarning: FC = ({
      • Any unsaved data will be lost.
      -