Skip to content

Commit 193c7ce

Browse files
ParkreinerParkreiner
authored
fix(site): tighten interface design for various frontend utility functions (#18894)
Precursor to #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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 2180d17 commit 193c7ce

File tree

16 files changed

+309
-198
lines changed

16 files changed

+309
-198
lines changed

‎site/.storybook/preview.jsx

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ function withHelmet(Story) {
101101
);
102102
}
103103

104+
/**
105+
* This JSX file isn't part of the main project, so it doesn't get to use the
106+
* ambient types defined in `storybook.d.ts` to provide extra type-safety.
107+
* Extracting main key to avoid typos.
108+
*/
109+
const queryParametersKey = "queries";
110+
104111
/** @type {Decorator} */
105112
function withQuery(Story, { parameters }) {
106113
const queryClient = new QueryClient({
@@ -112,8 +119,8 @@ function withQuery(Story, { parameters }) {
112119
},
113120
});
114121

115-
if (parameters.queries) {
116-
for (const query of parameters.queries) {
122+
if (parameters[queryParametersKey]) {
123+
for (const query of parameters[queryParametersKey]) {
117124
queryClient.setQueryData(query.key, query.data);
118125
}
119126
}

‎site/src/api/queries/templates.ts

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -48,21 +48,6 @@ export const templates = (
4848
};
4949
};
5050

51-
const getTemplatesByOrganizationQueryKey = (
52-
organization: string,
53-
options?: GetTemplatesOptions,
54-
) => [organization, "templates", options?.deprecated];
55-
56-
const templatesByOrganization = (
57-
organization: string,
58-
options: GetTemplatesOptions = {},
59-
) => {
60-
return {
61-
queryKey: getTemplatesByOrganizationQueryKey(organization, options),
62-
queryFn: () => API.getTemplatesByOrganization(organization, options),
63-
};
64-
};
65-
6651
export const templateACL = (templateId: string) => {
6752
return {
6853
queryKey: ["templateAcl", templateId],
@@ -121,9 +106,11 @@ export const templateExamples = () => {
121106
};
122107
};
123108

109+
export const templateVersionRoot: string = "templateVersion";
110+
124111
export const templateVersion = (versionId: string) => {
125112
return {
126-
queryKey: ["templateVersion", versionId],
113+
queryKey: [templateVersionRoot, versionId],
127114
queryFn: () => API.getTemplateVersion(versionId),
128115
};
129116
};
@@ -134,7 +121,7 @@ export const templateVersionByName = (
134121
versionName: string,
135122
) => {
136123
return {
137-
queryKey: ["templateVersion", organizationId, templateName, versionName],
124+
queryKey: [templateVersionRoot, organizationId, templateName, versionName],
138125
queryFn: () =>
139126
API.getTemplateVersionByName(organizationId, templateName, versionName),
140127
};
@@ -153,7 +140,7 @@ export const templateVersions = (templateId: string) => {
153140
};
154141

155142
export const templateVersionVariablesKey = (versionId: string) => [
156-
"templateVersion",
143+
templateVersionRoot,
157144
versionId,
158145
"variables",
159146
];
@@ -216,7 +203,7 @@ export const templaceACLAvailable = (
216203
};
217204

218205
const templateVersionExternalAuthKey = (versionId: string) => [
219-
"templateVersion",
206+
templateVersionRoot,
220207
versionId,
221208
"externalAuth",
222209
];
@@ -257,21 +244,21 @@ const createTemplateFn = async (options: CreateTemplateOptions) => {
257244

258245
export const templateVersionLogs = (versionId: string) => {
259246
return {
260-
queryKey: ["templateVersion", versionId, "logs"],
247+
queryKey: [templateVersionRoot, versionId, "logs"],
261248
queryFn: () => API.getTemplateVersionLogs(versionId),
262249
};
263250
};
264251

265252
export const richParameters = (versionId: string) => {
266253
return {
267-
queryKey: ["templateVersion", versionId, "richParameters"],
254+
queryKey: [templateVersionRoot, versionId, "richParameters"],
268255
queryFn: () => API.getTemplateVersionRichParameters(versionId),
269256
};
270257
};
271258

272259
export const resources = (versionId: string) => {
273260
return {
274-
queryKey: ["templateVersion", versionId, "resources"],
261+
queryKey: [templateVersionRoot, versionId, "resources"],
275262
queryFn: () => API.getTemplateVersionResources(versionId),
276263
};
277264
};
@@ -293,7 +280,7 @@ export const previousTemplateVersion = (
293280
) => {
294281
return {
295282
queryKey: [
296-
"templateVersion",
283+
templateVersionRoot,
297284
organizationId,
298285
templateName,
299286
versionName,
@@ -313,7 +300,7 @@ export const previousTemplateVersion = (
313300

314301
export const templateVersionPresets = (versionId: string) => {
315302
return {
316-
queryKey: ["templateVersion", versionId, "presets"],
303+
queryKey: [templateVersionRoot, versionId, "presets"],
317304
queryFn: () => API.getTemplateVersionPresets(versionId),
318305
};
319306
};

‎site/src/components/Filter/Filter.tsx

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import { useDebouncedFunction } from "hooks/debounce";
1616
import { ExternalLinkIcon } from "lucide-react";
1717
import { ChevronDownIcon } from "lucide-react";
1818
import { type FC, type ReactNode, useEffect, useRef, useState } from "react";
19-
import type { useSearchParams } from "react-router";
2019

2120
type PresetFilter = {
2221
name: string;
@@ -27,35 +26,55 @@ type FilterValues = Record<string, string | undefined>;
2726

2827
type UseFilterConfig = {
2928
/**
30-
* The fallback value to use in the event that no filter params can be parsed
31-
* from the search params object. This value is allowed to change on
32-
* re-renders.
29+
* The fallback value to use in the event that no filter params can be
30+
* parsed from the search params object.
3331
*/
3432
fallbackFilter?: string;
35-
searchParamsResult: ReturnType<typeof useSearchParams>;
33+
searchParams: URLSearchParams;
34+
onSearchParamsChange: (newParams: URLSearchParams) => void;
3635
onUpdate?: (newValue: string) => void;
3736
};
3837

38+
export type UseFilterResult = Readonly<{
39+
query: string;
40+
values: FilterValues;
41+
used: boolean;
42+
update: (newValues: string | FilterValues) => void;
43+
debounceUpdate: (newValues: string | FilterValues) => void;
44+
cancelDebounce: () => void;
45+
}>;
46+
3947
export const useFilterParamsKey = "filter";
4048

4149
export const useFilter = ({
4250
fallbackFilter = "",
43-
searchParamsResult,
51+
searchParams,
52+
onSearchParamsChange,
4453
onUpdate,
45-
}: UseFilterConfig) => {
46-
const [searchParams, setSearchParams] = searchParamsResult;
54+
}: UseFilterConfig): UseFilterResult => {
4755
const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter;
4856

4957
const update = (newValues: string | FilterValues) => {
5058
const serialized =
5159
typeof newValues === "string" ? newValues : stringifyFilter(newValues);
60+
const noUpdateNeeded = query === serialized;
61+
if (noUpdateNeeded) {
62+
return;
63+
}
5264

65+
/**
66+
* @todo 2025-07-15 - We have a slightly nasty bug here, where trying to
67+
* update state via immutable state updates causes our code to break.
68+
*
69+
* In theory, it would be better to make a copy of the search params. We
70+
* can then mutate and dispatch the copy instead of the original. Doing
71+
* that causes other parts of our existing logic to break, though.
72+
* That's a sign that our other code is slightly broken, and only just
73+
* happens to work by chance right now.
74+
*/
5375
searchParams.set(useFilterParamsKey, serialized);
54-
setSearchParams(searchParams);
55-
56-
if (onUpdate !== undefined) {
57-
onUpdate(serialized);
58-
}
76+
onSearchParamsChange(searchParams);
77+
onUpdate?.(serialized);
5978
};
6079

6180
const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction(
@@ -73,8 +92,6 @@ export const useFilter = ({
7392
};
7493
};
7594

76-
export type UseFilterResult = ReturnType<typeof useFilter>;
77-
7895
const parseFilterQuery = (filterQuery: string): FilterValues => {
7996
if (filterQuery === "") {
8097
return {};

‎site/src/hooks/usePagination.ts

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,43 @@
11
import { DEFAULT_RECORDS_PER_PAGE } from "components/PaginationWidget/utils";
2-
import type { useSearchParams } from "react-router";
32

4-
export const usePagination = ({
5-
searchParamsResult,
6-
}: {
7-
searchParamsResult: ReturnType<typeof useSearchParams>;
8-
}) => {
9-
const [searchParams, setSearchParams] = searchParamsResult;
10-
const page = searchParams.get("page") ? Number(searchParams.get("page")) : 1;
11-
const limit = DEFAULT_RECORDS_PER_PAGE;
12-
const offset = page <= 0 ? 0 : (page - 1) * limit;
3+
const paginationKey = "page";
134

14-
const goToPage = (page: number) => {
15-
searchParams.set("page", page.toString());
16-
setSearchParams(searchParams);
17-
};
5+
type UsePaginationOptions = Readonly<{
6+
searchParams: URLSearchParams;
7+
onSearchParamsChange: (newParams: URLSearchParams) => void;
8+
}>;
9+
10+
type UsePaginationResult = Readonly<{
11+
page: number;
12+
limit: number;
13+
offset: number;
14+
goToPage: (page: number) => void;
15+
}>;
16+
17+
export function usePagination(
18+
options: UsePaginationOptions,
19+
): UsePaginationResult {
20+
const { searchParams, onSearchParamsChange } = options;
21+
const limit = DEFAULT_RECORDS_PER_PAGE;
22+
const rawPage = Number.parseInt(searchParams.get(paginationKey) || "1", 10);
23+
const page = Number.isNaN(rawPage) || rawPage <= 0 ? 1 : rawPage;
1824

1925
return {
2026
page,
2127
limit,
22-
goToPage,
23-
offset,
28+
offset: Math.max(0, (page - 1) * limit),
29+
goToPage: (newPage) => {
30+
const abortNavigation =
31+
page === newPage ||
32+
!Number.isFinite(newPage) ||
33+
!Number.isInteger(newPage);
34+
if (abortNavigation) {
35+
return;
36+
}
37+
38+
const copy = new URLSearchParams(searchParams);
39+
copy.set("page", newPage.toString());
40+
onSearchParamsChange(copy);
41+
},
2442
};
25-
};
43+
}

‎site/src/pages/AuditPage/AuditPage.test.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { screen, waitFor } from "@testing-library/react";
22
import userEvent from "@testing-library/user-event";
33
import { API } from "api/api";
4+
import type { AuditLogsRequest } from "api/typesGenerated";
45
import { DEFAULT_RECORDS_PER_PAGE } from "components/PaginationWidget/utils";
56
import { http, HttpResponse } from "msw";
67
import {
@@ -106,7 +107,7 @@ describe("AuditPage", () => {
106107
await userEvent.type(filterField, query);
107108

108109
await waitFor(() =>
109-
expect(getAuditLogsSpy).toBeCalledWith({
110+
expect(getAuditLogsSpy).toHaveBeenCalledWith<[AuditLogsRequest]>({
110111
limit: DEFAULT_RECORDS_PER_PAGE,
111112
offset: 0,
112113
q: query,

‎site/src/pages/AuditPage/AuditPage.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ const AuditPage: FC = () => {
3333
const [searchParams, setSearchParams] = useSearchParams();
3434
const auditsQuery = usePaginatedQuery(paginatedAudits(searchParams));
3535
const filter = useFilter({
36-
searchParamsResult: [searchParams, setSearchParams],
36+
searchParams,
37+
onSearchParamsChange: setSearchParams,
3738
onUpdate: auditsQuery.goToFirstPage,
3839
});
3940

‎site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ const ConnectionLogPage: FC = () => {
2929
paginatedConnectionLogs(searchParams),
3030
);
3131
const filter = useFilter({
32-
searchParamsResult: [searchParams, setSearchParams],
32+
searchParams,
33+
onSearchParamsChange: setSearchParams,
3334
onUpdate: connectionlogsQuery.goToFirstPage,
3435
});
3536

‎site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,7 @@ export const CreateWorkspacePageViewExperimental: FC<
104104
owner,
105105
setOwner,
106106
}) => {
107-
const [suggestedName, setSuggestedName] = useState(() =>
108-
generateWorkspaceName(),
109-
);
107+
const [suggestedName, setSuggestedName] = useState(generateWorkspaceName);
110108
const [showPresetParameters, setShowPresetParameters] = useState(false);
111109
const id = useId();
112110
const workspaceNameInputRef = useRef<HTMLInputElement>(null);
@@ -120,14 +118,8 @@ export const CreateWorkspacePageViewExperimental: FC<
120118

121119
// Only touched fields are sent to the websocket
122120
// Autofilled parameters are marked as touched since they have been modified
123-
const initialTouched = parameters.reduce(
124-
(touched, parameter) => {
125-
if (autofillByName[parameter.name] !== undefined) {
126-
touched[parameter.name] = true;
127-
}
128-
return touched;
129-
},
130-
{} as Record<string, boolean>,
121+
const initialTouched = Object.fromEntries(
122+
parameters.filter((p) => autofillByName[p.name]).map((p) => [p, true]),
131123
);
132124

133125
// The form parameters values hold the working state of the parameters that will be submitted when creating a workspace

‎site/src/pages/TemplatesPage/TemplatesPage.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,11 @@ const TemplatesPage: FC = () => {
1414
const { permissions, user: me } = useAuthenticated();
1515
const { showOrganizations } = useDashboard();
1616

17-
const searchParamsResult = useSearchParams();
17+
const [searchParams, setSearchParams] = useSearchParams();
1818
const filter = useFilter({
1919
fallbackFilter: "deprecated:false",
20-
searchParamsResult,
20+
searchParams,
21+
onSearchParamsChange: setSearchParams,
2122
onUpdate: () => {}, // reset pagination
2223
});
2324

‎site/src/pages/UsersPage/UsersPage.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,8 @@ type UserPageProps = {
3939
const UsersPage: FC<UserPageProps> = ({ defaultNewPassword }) => {
4040
const queryClient = useQueryClient();
4141
const navigate = useNavigate();
42-
const searchParamsResult = useSearchParams();
42+
const [searchParams, setSearchParams] = useSearchParams();
4343
const { entitlements } = useDashboard();
44-
const [searchParams] = searchParamsResult;
4544

4645
const groupsByUserIdQuery = useQuery(groupsByUserId());
4746
const authMethodsQuery = useQuery(authMethods());
@@ -58,9 +57,10 @@ const UsersPage: FC<UserPageProps> = ({ defaultNewPassword }) => {
5857
enabled: viewDeploymentConfig,
5958
});
6059

61-
const usersQuery = usePaginatedQuery(paginatedUsers(searchParamsResult[0]));
60+
const usersQuery = usePaginatedQuery(paginatedUsers(searchParams));
6261
const useFilterResult = useFilter({
63-
searchParamsResult,
62+
searchParams,
63+
onSearchParamsChange: setSearchParams,
6464
onUpdate: usersQuery.goToFirstPage,
6565
});
6666

0 commit comments

Comments
 (0)