fix(site): revamp UI for batch-updating workspaces #18895

Parkreiner · 2025-07-16T02:34:48Z

Closes #18879
Builds on #18895

Changes made

Deleted BatchUpdateConfirmation component, replacing it with BatchUpdateModalForm
Added stories for the new component, trying to capture every variant I could think of

Screenshots

Notes

There's too many problems to list, but look at the issue to see all the problems we had with the old implementation
It's definitely helpful to look at the stories to see all the things the component is meant to cover

Parkreiner · 2025-07-16T02:35:56Z

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

-				isLoading={batchActions.isProcessing}
-				onClose={() => setActiveBatchAction(undefined)}
-				onConfirm={async () => {
+				workspacesToUpdate={checkedWorkspaces}


Renamed prop because I don't think that a component should need to be aware of how its parent chooses to represent workspace selections

Parkreiner · 2025-07-16T04:19:18Z

@aslilac I don't think there's a good way to ping you specifically now that you're a code owner, but I was hoping I could have you review this PR

aslilac

I wonder if we should even bother showing the "dormant" workspaces section. you can filter for dormant:true on the workspaces page to easily find offenders. and maybe the "already up to date" section could be collapsed by default?

site/src/pages/WorkspacesPage/BatchUpdateModalForm.stories.tsx

aslilac · 2025-07-21T22:14:46Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.stories.tsx

+import { BatchUpdateModalForm } from "./BatchUpdateModalForm";
+import { ACTIVE_BUILD_STATUSES } from "./WorkspacesPage";
+
+type Writeable<T> = { -readonly [Key in keyof T]: T[Key] };


I was so prepared to tell you to use a built-in TypeScript util but apparently somehow we don't have a standard inverse of Readonly 🤦‍♀️ is this a useful enough thing to justify having in a different file for reuse?

I do think it'll be useful sometime in the future. Any preferences as for where to put it? I feel like we definitely don't want to make another file like nullable.ts, where it's just a single type

aslilac · 2025-07-21T22:17:43Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.stories.tsx

+};
+
+export const OnlyReadyToUpdate: Story = {
+	beforeEach: (ctx) => {


Suggested change

beforeEach: (ctx) => {

beforeEach: (story) => {

I think this makes it a bit clearer what object we're dealing with (it appears to be the fully normalized story)

I actually disagree – the arg type is StoryContext<TRenderer, TArgs>. The type doesn't even give you access to the main story that the context was generated for

site/src/pages/WorkspacesPage/BatchUpdateModalForm.stories.tsx

aslilac · 2025-07-21T22:56:10Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx

+				{/*
+				 * 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 while the user is on the
+				 * workspaces page.
+				 */}


you don't actually explain what it is about the Dialog component that requires this tho. I think the benefits of not mounting a component when you don't need to should be relatively obvious, so this very wordy comment really just raises more questions than answer for me.

Yeah, I think I have a bad habit of trying to explain why old code was bad. And I guess with you becoming a code owner now, there's a lot less risk of some of those sub-optimal UI patterns creeping back in

aslilac · 2025-07-21T22:57:28Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx

+	onSubmit: () => void;
+}>;
+
+export const BatchUpdateModalForm: FC<BatchUpdateModalFormProps> = ({


I feel weird about the titular component being the last thing in the file

aslilac · 2025-07-21T23:00:11Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx

+										<li
+											key={ws.id}
+											className="[&:not(:last-child)]:border-b-border [&:not(:last-child)]:border-b [&:not(:last-child)]:border-solid border-0"
+										>


This ReviewPanel component is always rendered inside of this exact li, long className and all. could this be refactored?

I extracted it out into a separate component. I really don't like having li elements be coupled to the top level of a component definition, because it's really easy to wire them up wrong by accident and create invalid HTML (that React won't warn you about either)

aslilac · 2025-07-21T23:01:59Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.tsx

+							id={failedValidationId}
+							className="m-0 text-highlight-red text-right text-sm pt-2"
+						>
+							Please acknowledge consequences to continue.


tbh this feels a little overkill. restarting a workspace can be destructive, but in practice it shouldn't be.

aslilac · 2025-07-21T23:07:09Z

site/src/pages/WorkspacesPage/BatchUpdateModalForm.stories.tsx

+	workspaces: readonly Workspace[];
+	queries: QueryEntry;
+}>;
+function createPatchedDependencies(size: number): PatchedDependencies {


the more I look at these stories the more I'm worried that this function is the wrong abstraction. we're doing all kinds of hoop jumping to make it work rn and I wonder if it'd just be easier and clearer to write out the workspaces and query results like we usually do.

aslilac · 2025-07-21T23:08:47Z

also sorry I didn't review this sooner. I have no idea how this slid under my radar, I did a ton of reviews on thursday and just absolutely missed this one.

…tions (#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  ## 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.

Parkreiner added 12 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

fix: decouple component props from one another

b8d92fa

fix: apply biome fixes

1662a55

refactor: remove more bad coupling

e326414

fix: format

cc00106

fix: update call site mismatch

7304644

fix: update import

b512e18

fix: revamp UI for batch-updating workspaces

18ad064

Parkreiner self-assigned this Jul 16, 2025

Parkreiner requested a review from aslilac as a code owner July 16, 2025 02:34

Parkreiner commented Jul 16, 2025

View reviewed changes

fix: update Filter logic to account for fallback filter

cb92b79

Parkreiner mentioned this pull request Jul 16, 2025

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

Merged

Parkreiner added 4 commits July 16, 2025 03:32

docs: add comment about wonky code

86d19b8

fix: get workspace tests passing

330d91d

fix: update biome again

fb9d8f2

Merge branch 'mes/batch-update-01' into mes/batch-update-02

338cf4d

Parkreiner added 5 commits July 16, 2025 04:26

fix: remove outdated tests

5504b5b

wip: add tests back

5ec7dac

chore: get first integration test working

527104b

fix: update all integration tests

1555627

fix: move test to shrink diff

007efc1

aslilac reviewed Jul 21, 2025

View reviewed changes

github-actions bot added the stale This issue is like stale bread. label Jul 29, 2025

This comment was marked as spam.

Sign in to view

github-actions bot removed the stale This issue is like stale bread. label Jul 30, 2025

github-actions bot added the stale This issue is like stale bread. label Aug 7, 2025

github-actions bot closed this Aug 10, 2025

Parkreiner reopened this Aug 14, 2025

Base automatically changed from mes/batch-update-01 to main August 14, 2025 04:06

Parkreiner removed the stale This issue is like stale bread. label Aug 14, 2025

Parkreiner added 2 commits August 14, 2025 04:30

Merge branch 'main' into mes/batch-update-02

087fe94

chore: swap in action callbacks

6a9a37b

fix(site): revamp UI for batch-updating workspaces #18895

Are you sure you want to change the base?

fix(site): revamp UI for batch-updating workspaces #18895

Conversation

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

Uh oh!

Changes made

Screenshots

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner commented Jul 16, 2025

Uh oh!

aslilac left a comment

Choose a reason for hiding this comment

Uh oh!

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 Aug 14, 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!

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 Aug 14, 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!

aslilac commented Jul 21, 2025

Uh oh!

This comment was marked as spam.

This comment was marked as spam.

Uh oh!

Parkreiner commented Jul 16, 2025 •

edited

Loading

Parkreiner Aug 14, 2025 •

edited

Loading

Parkreiner Aug 14, 2025 •

edited

Loading