From 193c7ce91bd49c935676c8a0934a3b43bae4080f Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 14 Aug 2025 00:06:15 -0400 Subject: [PATCH 1/5] fix(site): tighten interface design for various frontend utility functions (#18894) Precursor to https://github.com/coder/coder/pull/18895 Splitting this off so that the changes are easier to review. ## Changes made - Improve type-safety for the `withQuery` Storybook decorator - Centralized almost all queries that deal with template versions to use a shared, exported query key prefix - Updated `useFilter` and `usePagination` to have much more strict return types, and decoupled them from React Router at the interface level - Also added some extra input validation and performance optimizations to `useFilter` and `usePagination` - Removed a stale data when working with checked workspaces for the `/workspaces` page - Removed hacky `useEffect` call for syncing URL state to checked workspaces, in favor of explicit state syncs - Did some extra cleanup and removed unused values ## Notes - Many of the changes here were in service of the main PR. I'll try to highlight notable areas, but if there's anything that's not clear, feel free to post a comment in this PR. Ideally you shouldn't really have to look at the other PR to understand this one, so if something's confusing, that's a sign that something's wrong ## Summary by CodeRabbit * **Refactor** * Improved handling of URL search parameters and state synchronization in filter and pagination features across multiple pages. * Centralized and clarified state management for workspace selection and batch actions on the Workspaces page. * Enhanced type safety and naming consistency in batch actions and filter components. * Updated filter and pagination hooks to accept explicit parameters and callbacks for better maintainability. * Streamlined prop naming and menu handling in workspace filter components for clarity. * **Bug Fixes** * Prevented unnecessary state updates when filter values remain unchanged. * **Tests** * Updated tests for improved type safety and more precise assertions. --- site/.storybook/preview.jsx | 11 +- site/src/api/queries/templates.ts | 35 ++-- site/src/components/Filter/Filter.tsx | 47 ++++-- site/src/hooks/usePagination.ts | 52 ++++-- site/src/pages/AuditPage/AuditPage.test.tsx | 3 +- site/src/pages/AuditPage/AuditPage.tsx | 3 +- .../ConnectionLogPage/ConnectionLogPage.tsx | 3 +- .../CreateWorkspacePageViewExperimental.tsx | 14 +- .../src/pages/TemplatesPage/TemplatesPage.tsx | 5 +- site/src/pages/UsersPage/UsersPage.tsx | 8 +- .../WorkspacesPage/WorkspacesPage.test.tsx | 13 +- .../pages/WorkspacesPage/WorkspacesPage.tsx | 157 ++++++++++++------ .../WorkspacesPageView.stories.tsx | 10 +- .../WorkspacesPage/WorkspacesPageView.tsx | 51 +++--- .../{batchActions.tsx => batchActions.ts} | 52 ++++-- .../filter/WorkspacesFilter.tsx | 43 +++-- 16 files changed, 309 insertions(+), 198 deletions(-) rename site/src/pages/WorkspacesPage/{batchActions.tsx => batchActions.ts} (61%) diff --git a/site/.storybook/preview.jsx b/site/.storybook/preview.jsx index f7a5407c457c1..c491428178f37 100644 --- a/site/.storybook/preview.jsx +++ b/site/.storybook/preview.jsx @@ -101,6 +101,13 @@ function withHelmet(Story) { ); } +/** + * This JSX file isn't part of the main project, so it doesn't get to use the + * ambient types defined in `storybook.d.ts` to provide extra type-safety. + * Extracting main key to avoid typos. + */ +const queryParametersKey = "queries"; + /** @type {Decorator} */ function withQuery(Story, { parameters }) { const queryClient = new QueryClient({ @@ -112,8 +119,8 @@ function withQuery(Story, { parameters }) { }, }); - if (parameters.queries) { - for (const query of parameters.queries) { + if (parameters[queryParametersKey]) { + for (const query of parameters[queryParametersKey]) { queryClient.setQueryData(query.key, query.data); } } diff --git a/site/src/api/queries/templates.ts b/site/src/api/queries/templates.ts index 5135f2304426e..9057179a8740c 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], @@ -121,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), }; }; @@ -134,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), }; @@ -153,7 +140,7 @@ export const templateVersions = (templateId: string) => { }; export const templateVersionVariablesKey = (versionId: string) => [ - "templateVersion", + templateVersionRoot, versionId, "variables", ]; @@ -216,7 +203,7 @@ export const templaceACLAvailable = ( }; const templateVersionExternalAuthKey = (versionId: string) => [ - "templateVersion", + templateVersionRoot, versionId, "externalAuth", ]; @@ -257,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), }; }; @@ -293,7 +280,7 @@ export const previousTemplateVersion = ( ) => { return { queryKey: [ - "templateVersion", + templateVersionRoot, organizationId, templateName, versionName, @@ -313,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/components/Filter/Filter.tsx b/site/src/components/Filter/Filter.tsx index b253c947ca486..ea5e60ff0a7f9 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 type { useSearchParams } from "react-router"; type PresetFilter = { name: string; @@ -27,35 +26,55 @@ 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); + const noUpdateNeeded = query === serialized; + if (noUpdateNeeded) { + return; + } + /** + * @todo 2025-07-15 - We have a slightly nasty bug here, where trying to + * update state via immutable state updates causes our code to break. + * + * In theory, it would be better to make a copy of the search params. We + * can then mutate and dispatch the copy instead of the original. Doing + * that causes other parts of our existing logic to break, though. + * That's a sign that our other code is slightly broken, and only just + * happens to work by chance right now. + */ searchParams.set(useFilterParamsKey, serialized); - setSearchParams(searchParams); - - if (onUpdate !== undefined) { - onUpdate(serialized); - } + onSearchParamsChange(searchParams); + onUpdate?.(serialized); }; const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction( @@ -73,8 +92,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 146ad9707da25..235a1505033e1 100644 --- a/site/src/hooks/usePagination.ts +++ b/site/src/hooks/usePagination.ts @@ -1,25 +1,43 @@ import { DEFAULT_RECORDS_PER_PAGE } from "components/PaginationWidget/utils"; -import type { useSearchParams } from "react-router"; -export const usePagination = ({ - searchParamsResult, -}: { - searchParamsResult: ReturnType; -}) => { - const [searchParams, setSearchParams] = searchParamsResult; - 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 paginationKey = "page"; - const goToPage = (page: number) => { - searchParams.set("page", page.toString()); - setSearchParams(searchParams); - }; +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 limit = DEFAULT_RECORDS_PER_PAGE; + const rawPage = Number.parseInt(searchParams.get(paginationKey) || "1", 10); + const page = Number.isNaN(rawPage) || rawPage <= 0 ? 1 : rawPage; return { page, limit, - goToPage, - offset, + offset: Math.max(0, (page - 1) * limit), + goToPage: (newPage) => { + const abortNavigation = + page === newPage || + !Number.isFinite(newPage) || + !Number.isInteger(newPage); + if (abortNavigation) { + return; + } + + const copy = new URLSearchParams(searchParams); + copy.set("page", newPage.toString()); + onSearchParamsChange(copy); + }, }; -}; +} diff --git a/site/src/pages/AuditPage/AuditPage.test.tsx b/site/src/pages/AuditPage/AuditPage.test.tsx index bcbc40da8af5c..80f6e74ae1a26 100644 --- a/site/src/pages/AuditPage/AuditPage.test.tsx +++ b/site/src/pages/AuditPage/AuditPage.test.tsx @@ -1,6 +1,7 @@ import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { API } from "api/api"; +import type { AuditLogsRequest } from "api/typesGenerated"; import { DEFAULT_RECORDS_PER_PAGE } from "components/PaginationWidget/utils"; import { http, HttpResponse } from "msw"; import { @@ -106,7 +107,7 @@ describe("AuditPage", () => { await userEvent.type(filterField, query); await waitFor(() => - expect(getAuditLogsSpy).toBeCalledWith({ + expect(getAuditLogsSpy).toHaveBeenCalledWith<[AuditLogsRequest]>({ limit: DEFAULT_RECORDS_PER_PAGE, offset: 0, q: query, diff --git a/site/src/pages/AuditPage/AuditPage.tsx b/site/src/pages/AuditPage/AuditPage.tsx index 02adf36186999..6c8c52a679ada 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/ConnectionLogPage/ConnectionLogPage.tsx b/site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx index aeb34d7bebd40..fd7fc12e38901 100644 --- a/site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx +++ b/site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx @@ -29,7 +29,8 @@ const ConnectionLogPage: FC = () => { paginatedConnectionLogs(searchParams), ); const filter = useFilter({ - searchParamsResult: [searchParams, setSearchParams], + searchParams, + onSearchParamsChange: setSearchParams, onUpdate: connectionlogsQuery.goToFirstPage, }); diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx index 9b4bb9608e90b..99876bfdb534d 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx @@ -104,9 +104,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); @@ -120,14 +118,8 @@ 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( - (touched, parameter) => { - if (autofillByName[parameter.name] !== undefined) { - touched[parameter.name] = true; - } - return touched; - }, - {} as Record, + const initialTouched = Object.fromEntries( + parameters.filter((p) => autofillByName[p.name]).map((p) => [p, true]), ); // The form parameters values hold the working state of the parameters that will be submitted when creating a workspace diff --git a/site/src/pages/TemplatesPage/TemplatesPage.tsx b/site/src/pages/TemplatesPage/TemplatesPage.tsx index fb4d2a8454dc6..d03d29716b4c9 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 354e77aceac7e..aaa67e060d50b 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.test.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx index 56f8fb34a32e8..678f0331331a6 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx @@ -1,7 +1,7 @@ import { screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { API } from "api/api"; -import type { Workspace } from "api/typesGenerated"; +import type { Workspace, WorkspacesResponse } from "api/typesGenerated"; import { http, HttpResponse } from "msw"; import { MockDormantOutdatedWorkspace, @@ -28,18 +28,17 @@ describe("WorkspacesPage", () => { }); it("renders an empty workspaces page", async () => { - // Given server.use( http.get("/api/v2/workspaces", async () => { - return HttpResponse.json({ workspaces: [], count: 0 }); + return HttpResponse.json({ + workspaces: [], + count: 0, + }); }), ); - // When renderWithAuth(); - - // Then - await screen.findByText("Create a workspace"); + await screen.findByRole("heading", { name: /Create a workspace/ }); }); it("renders a filled workspaces page", async () => { diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 22739d21dce89..2fec0c3975403 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -1,8 +1,8 @@ import { getErrorDetail, getErrorMessage } from "api/errors"; import { workspacePermissionsByOrganization } from "api/queries/organizations"; -import { templates } from "api/queries/templates"; +import { templateVersionRoot, templates } from "api/queries/templates"; import { workspaces } from "api/queries/workspaces"; -import type { Workspace, WorkspaceStatus } from "api/typesGenerated"; +import type { WorkspaceStatus } from "api/typesGenerated"; import { useFilter } from "components/Filter/Filter"; import { useUserFilterMenu } from "components/Filter/UserFilter"; import { displayError } from "components/GlobalSnackbar/utils"; @@ -11,7 +11,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"; @@ -22,15 +22,21 @@ import { WorkspacesPageView } from "./WorkspacesPageView"; import { useBatchActions } from "./batchActions"; import { useStatusFilterMenu, useTemplateFilterMenu } from "./filter/menus"; -// To reduce the number of fetches, we reduce the fetch interval if there are no -// active workspace builds. -const ACTIVE_BUILD_STATUSES: WorkspaceStatus[] = [ +/** + * The set of all workspace statuses that indicate that the state for a + * workspace is in the middle of a transition and will eventually reach a more + * stable state/status. + */ +const ACTIVE_BUILD_STATUSES: readonly WorkspaceStatus[] = [ "canceling", "deleting", "pending", "starting", "stopping", ]; + +// To reduce the number of fetches, we reduce the fetch interval if there are no +// active workspace builds. const ACTIVE_BUILDS_REFRESH_INTERVAL = 5_000; const NO_ACTIVE_BUILDS_REFRESH_INTERVAL = 30_000; @@ -48,13 +54,35 @@ function useSafeSearchParams() { >; } +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. - const searchParamsResult = useSafeSearchParams(); - const pagination = usePagination({ searchParamsResult }); + // We have to be careful with how we use useSearchParams or any other + // 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 + // which workspaces are shown on screen and which can be interacted with + const [checkedWorkspaceIds, setCheckedWorkspaceIds] = useState( + new Set(), + ); + const resetChecked = () => { + if (checkedWorkspaceIds.size !== 0) { + setCheckedWorkspaceIds(new Set()); + } + }; + + const pagination = usePagination({ + searchParams, + onSearchParamsChange: (newParams) => { + setSearchParams(newParams); + resetChecked(); + }, + }); const { permissions, user: me } = useAuthenticated(); const { entitlements } = useDashboard(); const templatesQuery = useQuery(templates()); @@ -78,14 +106,18 @@ const WorkspacesPage: FC = () => { }); }, [templatesQuery.data, workspacePermissionsQuery.data]); - const filterProps = useWorkspacesFilter({ - searchParamsResult, - onFilterChange: () => pagination.goToPage(1), + const filterState = useWorkspacesFilter({ + searchParams, + onSearchParamsChange: setSearchParams, + onFilterChange: () => { + pagination.goToPage(1); + resetChecked(); + }, }); const workspacesQueryOptions = workspaces({ ...pagination, - q: filterProps.filter.query, + q: filterState.filter.query, }); const { data, error, refetch } = useQuery({ ...workspacesQueryOptions, @@ -109,28 +141,18 @@ const WorkspacesPage: FC = () => { refetchOnWindowFocus: "always", }); - const [checkedWorkspaces, setCheckedWorkspaces] = useState< - readonly Workspace[] - >([]); - const [confirmingBatchAction, setConfirmingBatchAction] = useState< - "delete" | "update" | null - >(null); - const [urlSearchParams] = searchParamsResult; + const [activeBatchAction, setActiveBatchAction] = useState(); const canCheckWorkspaces = entitlements.features.workspace_batch_actions.enabled; const batchActions = useBatchActions({ onSuccess: async () => { await refetch(); - setCheckedWorkspaces([]); + resetChecked(); }, }); - // 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([]); - }, [urlSearchParams]); + const checkedWorkspaces = + data?.workspaces.filter((w) => checkedWorkspaceIds.has(w.id)) ?? []; return ( <> @@ -142,7 +164,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} @@ -152,12 +185,31 @@ const WorkspacesPage: FC = () => { page={pagination.page} limit={pagination.limit} onPageChange={pagination.goToPage} - filterProps={filterProps} - isRunningBatchAction={batchActions.isLoading} - onDeleteAll={() => setConfirmingBatchAction("delete")} - onUpdateAll={() => setConfirmingBatchAction("update")} - onStartAll={() => batchActions.startAll(checkedWorkspaces)} - onStopAll={() => batchActions.stopAll(checkedWorkspaces)} + filterState={filterState} + isRunningBatchAction={batchActions.isProcessing} + 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 + // 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], + exact: true, + type: "all", + }); + } + setActiveBatchAction("update"); + }} onActionSuccess={async () => { await queryClient.invalidateQueries({ queryKey: workspacesQueryOptions.queryKey, @@ -172,31 +224,27 @@ const WorkspacesPage: FC = () => { /> setActiveBatchAction(undefined)} onConfirm={async () => { - await batchActions.deleteAll(checkedWorkspaces); - setConfirmingBatchAction(null); - }} - onClose={() => { - setConfirmingBatchAction(null); + await batchActions.delete(checkedWorkspaces); + setActiveBatchAction(undefined); }} /> setActiveBatchAction(undefined)} onConfirm={async () => { - await batchActions.updateAll({ + await batchActions.updateTemplateVersions({ workspaces: checkedWorkspaces, isDynamicParametersEnabled: false, }); - setConfirmingBatchAction(null); - }} - onClose={() => { - setConfirmingBatchAction(null); + setActiveBatchAction(undefined); }} /> @@ -206,17 +254,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, }); diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx index e0fa40027cfc7..5696721a089d6 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx @@ -11,7 +11,6 @@ import { import { DEFAULT_RECORDS_PER_PAGE } from "components/PaginationWidget/utils"; import dayjs from "dayjs"; import uniqueId from "lodash/uniqueId"; -import type { ComponentProps } from "react"; import { expect, within } from "storybook/test"; import { MockBuildInfo, @@ -31,6 +30,7 @@ import { withProxyProvider, } from "testHelpers/storybook"; import { WorkspacesPageView } from "./WorkspacesPageView"; +import type { WorkspaceFilterState } from "./filter/WorkspacesFilter"; const createWorkspace = ( name: string, @@ -134,9 +134,7 @@ const allWorkspaces = [ ...Object.values(additionalWorkspaces), ]; -type FilterProps = ComponentProps["filterProps"]; - -const defaultFilterProps = getDefaultFilterProps({ +const defaultFilterProps = getDefaultFilterProps({ query: "owner:me", menus: { user: MockMenu, @@ -169,7 +167,7 @@ const meta: Meta = { component: WorkspacesPageView, args: { limit: DEFAULT_RECORDS_PER_PAGE, - filterProps: defaultFilterProps, + filterState: defaultFilterProps, checkedWorkspaces: [], canCheckWorkspaces: true, templates: mockTemplates, @@ -266,7 +264,7 @@ export const UserHasNoWorkspacesAndNoTemplates: Story = { export const NoSearchResults: Story = { args: { workspaces: [], - filterProps: { + filterState: { ...defaultFilterProps, filter: { ...defaultFilterProps.filter, diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx index 6563533bc43da..4476211387871 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx @@ -26,7 +26,7 @@ import { mustUpdateWorkspace } from "utils/workspace"; import { WorkspaceHelpTooltip } from "./WorkspaceHelpTooltip"; import { WorkspacesButton } from "./WorkspacesButton"; import { - type WorkspaceFilterProps, + type WorkspaceFilterState, WorkspacesFilter, } from "./filter/WorkspacesFilter"; @@ -45,16 +45,16 @@ interface WorkspacesPageViewProps { workspaces?: readonly Workspace[]; checkedWorkspaces: readonly Workspace[]; count?: number; - filterProps: WorkspaceFilterProps; + filterState: WorkspaceFilterState; page: number; limit: number; onPageChange: (page: number) => void; onCheckChange: (checkedWorkspaces: readonly Workspace[]) => void; isRunningBatchAction: boolean; - onDeleteAll: () => void; - onUpdateAll: () => void; - onStartAll: () => void; - onStopAll: () => void; + onBatchDeleteTransition: () => void; + onBatchUpdateTransition: () => void; + onBatchStartTransition: () => void; + onBatchStopTransition: () => void; canCheckWorkspaces: boolean; templatesFetchStatus: TemplateQuery["status"]; templates: TemplateQuery["data"]; @@ -69,15 +69,15 @@ export const WorkspacesPageView: FC = ({ error, limit, count, - filterProps, + filterState, onPageChange, page, checkedWorkspaces, onCheckChange, - onDeleteAll, - onUpdateAll, - onStopAll, - onStartAll, + onBatchDeleteTransition, + onBatchUpdateTransition, + onBatchStopTransition, + onBatchStartTransition, isRunningBatchAction, canCheckWorkspaces, templates, @@ -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 ( @@ -117,9 +117,12 @@ export const WorkspacesPageView: FC = ({ )} @@ -155,7 +158,7 @@ export const WorkspacesPageView: FC = ({ !mustUpdateWorkspace(w, canChangeVersions), ) } - onClick={onStartAll} + onClick={onBatchStartTransition} > Start @@ -165,12 +168,12 @@ export const WorkspacesPageView: FC = ({ (w) => w.latest_build.status === "running", ) } - onClick={onStopAll} + onClick={onBatchStopTransition} > Stop - + = ({ Delete… @@ -187,7 +190,7 @@ export const WorkspacesPageView: FC = ({ ) : ( - !invalidPageNumber && ( + !pageNumberIsInvalid && ( = ({ )} - {invalidPageNumber ? ( + {pageNumberIsInvalid ? ( ({ border: `1px solid ${theme.palette.divider}`, @@ -221,7 +224,7 @@ export const WorkspacesPageView: FC = ({ 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 @@ -63,8 +79,8 @@ export function useBatchActions(options: UseBatchActionsProps) { }); const favoriteAllMutation = useMutation({ - mutationFn: (workspaces: readonly Workspace[]) => { - return Promise.all( + mutationFn: async (workspaces: readonly Workspace[]): Promise => { + await Promise.all( workspaces .filter((w) => !w.favorite) .map((w) => API.putFavoriteWorkspace(w.id)), @@ -77,8 +93,8 @@ export function useBatchActions(options: UseBatchActionsProps) { }); const unfavoriteAllMutation = useMutation({ - mutationFn: (workspaces: readonly Workspace[]) => { - return Promise.all( + mutationFn: async (workspaces: readonly Workspace[]): Promise => { + await Promise.all( workspaces .filter((w) => w.favorite) .map((w) => API.deleteFavoriteWorkspace(w.id)), @@ -91,13 +107,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 || diff --git a/site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx b/site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx index ea3081d84c7f3..caebfd04526d4 100644 --- a/site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx +++ b/site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx @@ -1,4 +1,8 @@ -import { Filter, MenuSkeleton, type useFilter } from "components/Filter/Filter"; +import { + Filter, + MenuSkeleton, + type UseFilterResult, +} from "components/Filter/Filter"; import { type UserFilterMenu, UserMenu } from "components/Filter/UserFilter"; import { useDashboard } from "modules/dashboard/useDashboard"; import { @@ -62,8 +66,8 @@ const PRESETS_WITH_DORMANT: FilterPreset[] = [ }, ]; -export type WorkspaceFilterProps = { - filter: ReturnType; +export type WorkspaceFilterState = { + filter: UseFilterResult; error?: unknown; menus: { user?: UserFilterMenu; @@ -73,21 +77,36 @@ export type WorkspaceFilterProps = { }; }; +type WorkspaceFilterProps = Readonly<{ + filter: UseFilterResult; + error: unknown; + templateMenu: TemplateFilterMenu; + statusMenu: StatusFilterMenu; + + userMenu?: UserFilterMenu; + organizationsMenu?: OrganizationsFilterMenu; +}>; + export const WorkspacesFilter: FC = ({ filter, error, - menus, + templateMenu, + statusMenu, + userMenu, + organizationsMenu, }) => { const { entitlements, showOrganizations } = useDashboard(); const width = showOrganizations ? 175 : undefined; const presets = entitlements.features.advanced_template_scheduling.enabled ? PRESETS_WITH_DORMANT : PRESET_FILTERS; + const organizationsActive = + showOrganizations && organizationsMenu !== undefined; return ( = ({ )} options={ <> - {menus.user && } - - - {showOrganizations && menus.organizations !== undefined && ( - + {userMenu && } + + + {organizationsActive && ( + )} } optionsSkeleton={ <> - {menus.user && } + {userMenu && } - {showOrganizations && } + {organizationsActive && } } /> From 1ffc5a0e97b6c20c96643b16ab7f5cb962bb9b80 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 14 Aug 2025 14:17:25 +1000 Subject: [PATCH 2/5] docs: update status of coder desktop + corporate vpn issue (#19350) A customer read this on the docs and thought the issue was still unresolved. --- docs/user-guides/desktop/index.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/user-guides/desktop/index.md b/docs/user-guides/desktop/index.md index 116f7d4d6de69..7ba19666ca17b 100644 --- a/docs/user-guides/desktop/index.md +++ b/docs/user-guides/desktop/index.md @@ -144,9 +144,7 @@ To avoid system VPN configuration conflicts, only one copy of `Coder Desktop.app If the logged in Coder deployment requires a corporate VPN to connect, Coder Connect can't establish communication through the VPN, and will time out. -This is due to known issues with [macOS](https://github.com/coder/coder-desktop-macos/issues/201) and -[Windows](https://github.com/coder/coder-desktop-windows/issues/147) networking. -A resolution is in progress. +This issue has been fixed in Coder v2.24.3 and later. For macOS clients, Coder Desktop v0.8.0 or later is also required. ## Next Steps From 0c203b0cf87f22a9fa7f768828142e4dbecfddde Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 14 Aug 2025 00:20:00 -0400 Subject: [PATCH 3/5] fix: correct markup for Abbr component (#19317) Fixes some accidental styling issues introduced in #19242 ## Changes made - Updated styles - Added support for `className` prop so that we can override the styles as needed - Removed the aria-label in favor of injecting the main text directly ## Notes - This feels like a case where the changes in the previous PR were actually *correct overall*, but something with our MUI+Tailwind setup created conflicting styles, and we accidentally introduced an underline style that shouldn't be there - Removed the Aria label because I've realized in the past year that Aria is really easy to misuse, and it's best just to do things with the base HTML features as much as possible. There's a risk that the old code had compliance issues with certain types of screen readers (even though it worked fine when I did manual testing back in 2023). These changes hopefully remove those risks completely --- site/src/components/Abbr/Abbr.stories.tsx | 8 +-- site/src/components/Abbr/Abbr.test.tsx | 61 +++++++++++++---------- site/src/components/Abbr/Abbr.tsx | 20 +++++--- 3 files changed, 50 insertions(+), 39 deletions(-) diff --git a/site/src/components/Abbr/Abbr.stories.tsx b/site/src/components/Abbr/Abbr.stories.tsx index b32138ad54fd5..7d079e4ac7416 100644 --- a/site/src/components/Abbr/Abbr.stories.tsx +++ b/site/src/components/Abbr/Abbr.stories.tsx @@ -6,10 +6,10 @@ const meta: Meta = { component: Abbr, decorators: [ (Story) => ( - <> +

Try the following text out in a screen reader!

- +
), ], }; @@ -25,9 +25,9 @@ export const InlinedShorthand: Story = { }, decorators: [ (Story) => ( -

+

The physical pain of getting bonked on the head with a cartoon mallet - lasts precisely 593{" "} + lasts precisely 593 diff --git a/site/src/components/Abbr/Abbr.test.tsx b/site/src/components/Abbr/Abbr.test.tsx index 3ae76f071bdfb..b67406299685a 100644 --- a/site/src/components/Abbr/Abbr.test.tsx +++ b/site/src/components/Abbr/Abbr.test.tsx @@ -1,5 +1,5 @@ import { render, screen } from "@testing-library/react"; -import { Abbr, type Pronunciation } from "./Abbr"; +import { Abbr } from "./Abbr"; type AbbreviationData = { abbreviation: string; @@ -7,28 +7,8 @@ type AbbreviationData = { expectedLabel: string; }; -type AssertionInput = AbbreviationData & { - pronunciation: Pronunciation; -}; - -function assertAccessibleLabel({ - abbreviation, - title, - expectedLabel, - pronunciation, -}: AssertionInput) { - const { unmount } = render( - - {abbreviation} - , - ); - - screen.getByLabelText(expectedLabel, { selector: "abbr" }); - unmount(); -} - describe(Abbr.name, () => { - it("Has an aria-label that equals the title if the abbreviation is shorthand", () => { + it("Omits abbreviation from screen-reader output if it is shorthand", () => { const sampleShorthands: AbbreviationData[] = [ { abbreviation: "ms", @@ -43,11 +23,22 @@ describe(Abbr.name, () => { ]; for (const shorthand of sampleShorthands) { - assertAccessibleLabel({ ...shorthand, pronunciation: "shorthand" }); + const { unmount } = render( + + {shorthand.abbreviation} + , + ); + + // The element doesn't have any ARIA role semantics baked in, + // so we have to get a little bit more creative with making sure the + // expected content is on screen in an accessible way + const element = screen.getByTitle(shorthand.title); + expect(element).toHaveTextContent(shorthand.expectedLabel); + unmount(); } }); - it("Has an aria label with title and 'flattened' pronunciation if abbreviation is acronym", () => { + it("Adds title and 'flattened' pronunciation if abbreviation is acronym", () => { const sampleAcronyms: AbbreviationData[] = [ { abbreviation: "NASA", @@ -67,11 +58,19 @@ describe(Abbr.name, () => { ]; for (const acronym of sampleAcronyms) { - assertAccessibleLabel({ ...acronym, pronunciation: "acronym" }); + const { unmount } = render( + + {acronym.abbreviation} + , + ); + + const element = screen.getByTitle(acronym.title); + expect(element).toHaveTextContent(acronym.expectedLabel); + unmount(); } }); - it("Has an aria label with title and initialized pronunciation if abbreviation is initialism", () => { + it("Adds title and initialized pronunciation if abbreviation is initialism", () => { const sampleInitialisms: AbbreviationData[] = [ { abbreviation: "FBI", @@ -91,7 +90,15 @@ describe(Abbr.name, () => { ]; for (const initialism of sampleInitialisms) { - assertAccessibleLabel({ ...initialism, pronunciation: "initialism" }); + const { unmount } = render( + + {initialism.abbreviation} + , + ); + + const element = screen.getByTitle(initialism.title); + expect(element).toHaveTextContent(initialism.expectedLabel); + unmount(); } }); }); diff --git a/site/src/components/Abbr/Abbr.tsx b/site/src/components/Abbr/Abbr.tsx index b27141818efb3..0c08c33e111ce 100644 --- a/site/src/components/Abbr/Abbr.tsx +++ b/site/src/components/Abbr/Abbr.tsx @@ -1,12 +1,13 @@ import type { FC, HTMLAttributes } from "react"; import { cn } from "utils/cn"; -export type Pronunciation = "shorthand" | "acronym" | "initialism"; +type Pronunciation = "shorthand" | "acronym" | "initialism"; type AbbrProps = HTMLAttributes & { children: string; title: string; pronunciation?: Pronunciation; + className?: string; }; /** @@ -22,23 +23,26 @@ export const Abbr: FC = ({ children, title, pronunciation = "shorthand", + className, ...delegatedProps }) => { return ( {children} + + {getAccessibleLabel(children, title, pronunciation)} + ); }; From c6d62e2de1b37a2b0542a17337d9983b9179f77d Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 14 Aug 2025 15:06:41 +1000 Subject: [PATCH 4/5] docs: update coder desktop + corporate vpn issue (#19353) I missed this in my first PR :face_exhaling:. We already include the version requirements for the VPN fix in the `Known Issues` section. --- docs/user-guides/desktop/index.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/docs/user-guides/desktop/index.md b/docs/user-guides/desktop/index.md index 7ba19666ca17b..d5f5e5aabb3c2 100644 --- a/docs/user-guides/desktop/index.md +++ b/docs/user-guides/desktop/index.md @@ -8,12 +8,6 @@ Coder Desktop requires a Coder deployment running [v2.20.0](https://github.com/c ## Install Coder Desktop -> [!IMPORTANT] -> Coder Desktop can't connect through a corporate VPN. -> -> Due to a [known issue](#coder-desktop-cant-connect-through-another-vpn), -> if your Coder deployment requires that you connect through a corporate VPN, Desktop will timeout when it tries to connect. -

You can install Coder Desktop on macOS or Windows. From b2b3edf0f1b0a7971666b92dc2c8ed960c74fbc3 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 14 Aug 2025 15:49:48 +1000 Subject: [PATCH 5/5] test(codersdk/toolsdk): skip `coder_workspace_bash` tool test on windows (#19351) Fixes flakes on the nightly-gauntlet like: https://github.com/coder/coder/actions/runs/16955654896 since there's no `bash` on windows... ``` === Failed === FAIL: codersdk/toolsdk (0.00s) PASS The following tools were not tested: - coder_workspace_bash Please ensure that all tools are tested using testTool(). If you just added a new tool, please add a test for it. NOTE: if you just ran an individual test, this is expected. FAIL github.com/coder/coder/v2/codersdk/toolsdk 4.185s ``` --- codersdk/toolsdk/toolsdk_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/codersdk/toolsdk/toolsdk_test.go b/codersdk/toolsdk/toolsdk_test.go index 0754514693a0e..fb321e90e7dee 100644 --- a/codersdk/toolsdk/toolsdk_test.go +++ b/codersdk/toolsdk/toolsdk_test.go @@ -665,6 +665,10 @@ func TestMain(m *testing.M) { var untested []string for _, tool := range toolsdk.All { if tested, ok := testedTools.Load(tool.Name); !ok || !tested.(bool) { + // Test is skipped on Windows + if runtime.GOOS == "windows" && tool.Name == "coder_workspace_bash" { + continue + } untested = append(untested, tool.Name) } }