- Notifications
You must be signed in to change notification settings - Fork1k
fix(site): revamp UI for batch-updating workspaces#18895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| isLoading={batchActions.isProcessing} | ||
| onClose={()=>setActiveBatchAction(undefined)} | ||
| onConfirm={async()=>{ | ||
| workspacesToUpdate={checkedWorkspaces} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Renamed prop because I don't think that a component should need to be aware of how its parent chooses to represent workspace selections
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if we should even bother showing the "dormant" workspaces section. you can filter fordormant:true on the workspaces page to easily find offenders. and maybe the "already up to date" section could be collapsed by default?
Uh oh!
There was an error while loading.Please reload this page.
| import{BatchUpdateModalForm}from"./BatchUpdateModalForm"; | ||
| import{ACTIVE_BUILD_STATUSES}from"./WorkspacesPage"; | ||
| typeWriteable<T>={-readonly[KeyinkeyofT]:T[Key]}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was so prepared to tell you to use a built-in TypeScript util but apparently somehow we don't have a standard inverse ofReadonly 🤦♀️ is this a useful enough thing to justify having in a different file for reuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 likenullable.ts, where it's just a single type
| }; | ||
| exportconstOnlyReadyToUpdate:Story={ | ||
| beforeEach:(ctx)=>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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)
ParkreinerAug 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I actually disagree – the arg type isStoryContext<TRenderer, TArgs>. The type doesn't even give you access to the main story that the context was generated for
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| {/* | ||
| * 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. | ||
| */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you don't actually explain what it is about theDialog 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| onSubmit:()=>void; | ||
| }>; | ||
| exportconstBatchUpdateModalForm:FC<BatchUpdateModalFormProps>=({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel weird about the titular component being the last thing in the file
| <li | ||
| key={ws.id} | ||
| className="[&:not(:last-child)]:border-b-border [&:not(:last-child)]:border-b [&:not(:last-child)]:border-solid border-0" | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ThisReviewPanel component is always rendered inside of this exactli, longclassName and all. could this be refactored?
ParkreinerAug 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I extracted it out into a separate component. I really don't like havingli 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)
| id={failedValidationId} | ||
| className="m-0 text-highlight-red text-right text-sm pt-2" | ||
| > | ||
| Please acknowledge consequences to continue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
tbh this feels a little overkill. restarting a workspacecan be destructive, but in practice it shouldn't be.
| workspaces:readonlyWorkspace[]; | ||
| queries:QueryEntry; | ||
| }>; | ||
| functioncreatePatchedDependencies(size:number):PatchedDependencies{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 easierand clearer to write out theworkspaces and query results like we usually do.
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#18895Splitting 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 usea shared, exported query key prefix- Updated `useFilter` and `usePagination` to have much more strictreturn types, and decoupled them from React Router at the interfacelevel- Also added some extra input validation and performance optimizationsto `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 checkedworkspaces, 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 tohighlight notable areas, but if there's anything that's not clear, feelfree to post a comment in this PR. Ideally you shouldn't really have tolook at the other PR to understand this one, so if something'sconfusing, 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 synchronizationin filter and pagination features across multiple pages.* Centralized and clarified state management for workspace selection andbatch actions on the Workspaces page.* Enhanced type safety and naming consistency in batch actions andfilter components.* Updated filter and pagination hooks to accept explicit parameters andcallbacks for better maintainability.* Streamlined prop naming and menu handling in workspace filtercomponents for clarity.* **Bug Fixes*** Prevented unnecessary state updates when filter values remainunchanged.* **Tests** * Updated tests for improved type safety and more precise assertions.<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@aslilac Do you have time to pair with me next week to try simplifying the stories (and probably the component, too)? I would love to remove a bunch of the |
absolutely! feel free to schedule something |
| onAcceptedConsequencesChange:(newValue:boolean)=>void; | ||
| checkboxRef:ForwardedRef<HTMLButtonElement>; | ||
| containerRef:ForwardedRef<HTMLDivElement>; | ||
| }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| }>; | |
| }>; | |
| checked={acceptedConsequences} | ||
| onCheckedChange={onAcceptedConsequencesChange} | ||
| /> | ||
| I acknowledge these consequences. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel like consequences is just such a serious word here. maybe effects?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
8a6852f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#18879
Builds on#18895
Changes made
BatchUpdateConfirmationcomponent, replacing it withBatchUpdateModalFormScreenshots
Notes