fix(site): tighten interface design for various frontend utility functions #18894

Parkreiner · 2025-07-16T01:40:49Z

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

Parkreiner · 2025-07-16T01:42:25Z

site/src/api/queries/templates.ts

-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),
-	};
-};


Weren't used at all

Parkreiner · 2025-07-16T01:44:16Z

site/src/api/queries/templates.ts

@@ -121,9 +106,11 @@ export const templateExamples = () => {
 	};
 };

+export const templateVersionRoot: string = "templateVersion";


This ends up being more important for the upcoming PR, but I really, really think that we need to find the time to formalize our entire data layer, and be way more deliberate with our React Query caching

I fully expect our caching to create a super, super hard-to-recreate bug sometime in the future, because it's a bunch of semi-structured global mutable state right now

Also added an explicit string type annotation so that TypeScript doesn't over-infer the value and treat it as a literal. We shouldn't know what exact combination of characters it is – just what purpose the string serves in the greater codebase as an opaque value

Parkreiner · 2025-07-16T01:45:17Z

site/src/components/Filter/Filter.tsx

+	searchParams: URLSearchParams;
+	onSearchParamsChange: (newParams: URLSearchParams) => void;


Decoupled type signature from React Router. Splitting this up gives us a lot more freedom, which comes up later in this PR

Parkreiner · 2025-07-16T01:46:58Z

site/src/components/Filter/Filter.tsx

+export type UseFilterResult = Readonly<{
+	query: string;
+	values: FilterValues;
+	used: boolean;
+	update: (newValues: string | FilterValues) => void;
+	debounceUpdate: (newValues: string | FilterValues) => void;
+	cancelDebounce: () => void;
+}>;


I feel like the previous type was super, super risky, because it was derived from a function that didn't have much type-safety

With this, the contract is way more explicit and simple, and there's basically no chance of the TypeScript LSP getting confused and dropping properties for tooltips

Parkreiner · 2025-07-16T01:47:44Z

site/src/hooks/usePagination.ts

+type UsePaginationResult = Readonly<{
+	page: number;
+	limit: number;
+	offset: number;
+	goToPage: (page: number) => void;
+}>;


We were missing a return type altogether before

Parkreiner · 2025-07-16T01:48:10Z

site/src/hooks/usePagination.ts

+			const abortNavigation =
+				page === newPage || !Number.isFinite(page) || !Number.isInteger(page);
+			if (abortNavigation) {
+				return;
+			}


Another render optimization

What is this render optimization? Not re-rendering the page if it's the same page?

So there's two parts:

The basic input validation from the Number static methods

The equality check

I could maybe add a comment, but React only has Object.is for change detection, so in general, a lot of render optimizations boil down to "only dispatch a new value (especially a non-primitive value) when you know the contents have changed". Most of the time, you don't have to worry about over-dispatching primitive values (as long as they're done outside a render, React will detect that nothing changed and do a no-op for the state update). But because JS is wacky, there are edge cases with Object.is and numbers, which is why there's the extra === check

Parkreiner · 2025-07-16T01:48:27Z

site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx

-	const [suggestedName, setSuggestedName] = useState(() =>
-		generateWorkspaceName(),
-	);
+	const [suggestedName, setSuggestedName] = useState(generateWorkspaceName);


The extra wrapper function was redundant

Parkreiner · 2025-07-16T01:48:58Z

site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx

 		(touched, parameter) => {
 			if (autofillByName[parameter.name] !== undefined) {
 				touched[parameter.name] = true;
 			}
 			return touched;
 		},
-		{} as Record<string, boolean>,
+		{},


Removed type assertion to remove risk of accidentally silencing the compiler

Parkreiner · 2025-07-16T01:50:43Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

+	const [checkedWorkspaceIds, setCheckedWorkspaceIds] = useState(
+		new Set<string>(),
+	);


This is part of a fix for a super nasty bug that I ran into when updating the batch-update UI

Because we were storing an entire copy of the original workspaces each time we checked off something, that also meant that as the query cache updated its data and triggered re-renders, anything that used the checked workspaces would receive the stale versions of the workspaces

Parkreiner · 2025-07-16T01:51:48Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

-		"delete" | "update" | null
-	>(null);
-	const [urlSearchParams] = searchParamsResult;
+	const [activeBatchAction, setActiveBatchAction] = useState<BatchAction>();


Slightly re-jiggered confirmingBatchAction

Parkreiner · 2025-07-16T01:52:19Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

@@ -142,7 +164,18 @@ const WorkspacesPage: FC = () => {
 				canCreateTemplate={permissions.createTemplates}
 				canChangeVersions={permissions.updateTemplates}
 				checkedWorkspaces={checkedWorkspaces}
-				onCheckChange={setCheckedWorkspaces}
+				onCheckChange={(newWorkspaces) => {
+					setCheckedWorkspaceIds((current) => {


Another render optimization

Parkreiner · 2025-07-16T02:00:27Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

+				onBatchDeleteTransition={() => setActiveBatchAction("delete")}
+				onBatchStartTransition={() => batchActions.start(checkedWorkspaces)}
+				onBatchStopTransition={() => batchActions.stop(checkedWorkspaces)}
+				onBatchUpdateTransition={() => {


This change becomes a little more relevant for the other PR, but even if this were the only PR, this would still help the update UI avoid stale state

Parkreiner · 2025-07-16T02:01:19Z

site/src/pages/WorkspacesPage/batchActions.ts

+type UseBatchActionsResult = Readonly<{
+	isProcessing: boolean;
+	start: (workspaces: readonly Workspace[]) => Promise<WorkspaceBuild[]>;
+	stop: (workspaces: readonly Workspace[]) => Promise<WorkspaceBuild[]>;
+	delete: (workspaces: readonly Workspace[]) => Promise<WorkspaceBuild[]>;
+	updateTemplateVersions: (
+		payload: UpdateAllPayload,
+	) => Promise<WorkspaceBuild[]>;
+	favorite: (payload: readonly Workspace[]) => Promise<void>;
+	unfavorite: (payload: readonly Workspace[]) => Promise<void>;
+}>;


Added explicit return type, which was super important here, because the function's return type was getting inferred as something super complicated and nasty from React Query

Parkreiner · 2025-07-16T02:31:09Z

site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx

+type WorkspaceFilterProps = Readonly<{
+	filter: UseFilterResult;
+	error: unknown;
+	templateMenu: TemplateFilterMenu;
+	statusMenu: StatusFilterMenu;
+
+	userMenu?: UserFilterMenu;
+	organizationsMenu?: OrganizationsFilterMenu;
+}>;


Decouples the main state from the actual component props

The way I think about it is that props are a hack to get JS function to support named arguments and make it mirror HTML more, so you really shouldn't ever have any direct dependencies on that

brettkolodny

Overall lgtm, I think these changes are a lot cleaner but would definitely wait for Kayla's review

brettkolodny · 2025-07-16T14:18:45Z

site/src/hooks/usePagination.ts

+			const abortNavigation =
+				page === newPage || !Number.isFinite(page) || !Number.isInteger(page);
+			if (abortNavigation) {
+				return;
+			}


What is this render optimization? Not re-rendering the page if it's the same page?

brettkolodny · 2025-07-16T14:28:02Z

site/src/api/queries/templates.ts

@@ -121,9 +106,11 @@ export const templateExamples = () => {
 	};
 };

+export const templateVersionRoot: string = "templateVersion";


nit: there seems to be inconsistencies within the code base between using camelCase or UPPER_SNAKE_CASE for file-wide constants so while out of scope for this PR I think deciding on one and sticking to it would be good

Yeah, that's fair. The JS/TS way seems to be SCREAMING_CASE for constants, while the Go way seems to be to treat them as any other package value

I'm leaning towards switching this to SCREAMING_CASE, but I'll wait for Kayla to chime in

Parkreiner · 2025-07-16T15:09:42Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

+ * workspace is in the middle of a transition and will eventually reach a more
+ * stable state/status.
+ */
+const ACTIVE_BUILD_STATUSES: readonly WorkspaceStatus[] = [


This becomes exported in the second PR, which is why I put more of a public-facing comment on it

Parkreiner · 2025-07-16T15:15:16Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

-	// 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]);


Forgot to comment on this, but the resetChecked logic above is meant to make sure that if we add more URL logic in the future, it doesn't break our filtering and pagination

We were coarsely syncing based on the entire URL params object. That worked fine for now, but if we were to add any logic on the same page that also dealt with URLs and had nothing to do with the filtering/pagination, it would still cause all the checkboxes to get unchecked on every change

Moving the state syncing to the event handlers for the filter/pagination hooks seemed like the better choice. That way, we're also not coupled to the hooks formatting the URLs a specific way either

Parkreiner added 5 commits July 16, 2025 00:48

refactor: centralize storybook queries key

bb05088

refactor: centralize template version key prefix

a119d70

refactor: remove JSX extension from batch actions

5dc67cf

fix: update API design of various utility hooks

4c77bff

fix: remove promise race conditions for batch utilities

4833ae1

Parkreiner self-assigned this Jul 16, 2025

Parkreiner requested a review from aslilac as a code owner July 16, 2025 01:40

Parkreiner changed the title ~~fix(site): tighten up interface design for various frontend utility functions~~ fix(site): tighten interface design for various frontend utility functions Jul 16, 2025

Parkreiner commented Jul 16, 2025

View reviewed changes

fix: decouple component props from one another

b8d92fa

Parkreiner commented Jul 16, 2025

View reviewed changes

Parkreiner added 2 commits July 16, 2025 02:06

fix: apply biome fixes

1662a55

refactor: remove more bad coupling

e326414

Parkreiner requested a review from brettkolodny July 16, 2025 02:09

Parkreiner added 3 commits July 16, 2025 02:12

fix: format

cc00106

fix: update call site mismatch

7304644

fix: update import

b512e18

Parkreiner commented Jul 16, 2025

View reviewed changes

fix: update Filter logic to account for fallback filter

cb92b79

Parkreiner added 3 commits July 16, 2025 03:32

docs: add comment about wonky code

86d19b8

fix: get workspace tests passing

330d91d

fix: update biome again

fb9d8f2

brettkolodny approved these changes Jul 16, 2025

View reviewed changes

Parkreiner commented Jul 16, 2025

View reviewed changes

		searchParams: URLSearchParams;
		onSearchParamsChange: (newParams: URLSearchParams) => void;

fix(site): tighten interface design for various frontend utility functions #18894

Are you sure you want to change the base?

fix(site): tighten interface design for various frontend utility functions #18894

Uh oh!

Conversation

Parkreiner commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettkolodny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Parkreiner commented Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading

Parkreiner Jul 16, 2025 •

edited

Loading