- Notifications
You must be signed in to change notification settings - Fork906
feat: add inline actions into workspaces table#17636
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
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.
Pull Request Overview
This PR introduces inline actions to the workspaces table while refactoring how workspace data is queried and updated. Key changes include:
- Refactoring the useWorkspacesData hook to accept a WorkspacesRequest object and updating query keys.
- Implementing a new WorkspaceActionsCell with inline action buttons and associated success/error callbacks.
- Removing redundant isOwner props by computing ownership via the authenticated user, and updating related update/cancellation hooks.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| site/src/pages/WorkspacesPage/data.ts | Refactors workspace data fetching to use WorkspacesRequest and updates the queryKey handling. |
| site/src/pages/WorkspacesPage/WorkspacesTable.tsx | Introduces the WorkspaceActionsCell component and updates table cell layout. |
| site/src/pages/WorkspacesPage/WorkspacesPageView.tsx | Passes inline action callbacks to the table component. |
| site/src/pages/WorkspacesPage/WorkspacesPage.tsx | Updates query parameter naming and adds action success/error callbacks. |
| site/src/pages/WorkspacePage/WorkspaceTopbar.tsx | Removes the isOwner prop; ownership is now derived via authentication. |
| site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx | Switches to the new workspace update hook and renders WorkspaceUpdateDialogs. |
| site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx | Refactors cancel button logic by computing isOwner locally and replacing the showCancel variable. |
| site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx | Adds an auth provider for story consistency. |
| site/src/modules/workspaces/useWorkspaceUpdate.tsx | Implements a new hook for workspace update flows with confirmation/missing parameter dialogs. |
| site/src/modules/workspaces/actions.ts | Adjusts abilities logic to take a permissions object instead of a boolean for canDebug. |
| site/src/hooks/usePagination.ts | Replaces manual offset calculation with the calcOffset helper function. |
| site/src/components/Dialogs/Dialog.tsx | Adds stopPropagation logic to cancel button clicks to avoid undesired events. |
| site/src/api/queries/workspaces.ts | Updates queryFn to pass full request config to the API. |
Comments suppressed due to low confidence (2)
site/src/pages/WorkspacePage/WorkspaceTopbar.tsx:55
- [nitpick] Since the isOwner prop is removed and ownership is now derived from the authenticated user, ensure that all child components relying on 'isOwner' are updated accordingly to prevent any unexpected UI behavior.
// Removed: isOwner propsite/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx:174
- [nitpick] Replacing the 'showCancel' condition with 'canCancel' changes the criteria for displaying the CancelButton; please verify that this aligns with the intended cancellation permissions and workspace state logic.
{canCancel && <CancelButton handleAction={handleCancel} />}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.
Looks good to me overall! Just had a few questions/comments on some small details
Uh oh!
There was an error while loading.Please reload this page.
| @@ -23,3 +23,7 @@ export const usePagination = ({ | |||
| offset, | |||
| }; | |||
| }; | |||
| export const calcOffset = (page: number, limit: number) => { | |||
| return page <= 0 ? 0 : (page - 1) * limit; | |||
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.
This can be turned intoMath.max(0, limit * (page - 1))
ParkreinerMay 2, 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.
Also, do we want to put this function in this file? It's being imported by a lot of files that don't care about the hook
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.
That is true. I just didn't want to create a moduleutils/pagination.ts just for one function, but you have a good point. What would you suggest?
| @@ -58,7 +66,7 @@ export const abilitiesByWorkspaceStatus = ( | |||
| case "starting": { | |||
| return { | |||
| actions: ["starting"], | |||
| canCancel: true, | |||
| canCancel: true && hasPermissionToCancel, | |||
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.
Thetrue literals can be removed here and forstopping. If you AND a boolean with true, you always get back the original value
| missingBuildParameters: MissingBuildParametersDialogProps; | ||
| }; | ||
| export const WorkspaceUpdateDialogs: FC<WorkspaceUpdateDialogsProps> = ({ |
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.
Because this file is exporting components that aren't simple pass-through components like context providers, I feel like it's probably better to rename the file to put more emphasis on the components over the hook (could probably name it WorkspaceUpdateDialogs?). The hook just feels like an implementation detail
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.
Especially since this isn't the firstuseWorkspaceUpdate in the codebase
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.
Makes total sense 👍
| }; | ||
| return { | ||
| update, |
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'm not opposed to having a general function that handles both the confirmed case and the non-confirmed case, but I'd prefer for that to be kept as an implementation detail. I feel like there's a risk of accidentally callingupdate the wrong way if we return it out directly
I think it'd be better if we did something like this:
update:()=>update()
This also makes sure that the types can't lie to us
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 see... Maybe in this case, we could have two functionsaskForConfirmation andupdate. What do you think?
| return ( | ||
| <> | ||
| <UpdateConfirmationDialog {...updateConfirmation} /> | ||
| <MissingBuildParametersDialog {...missingBuildParameters} /> | ||
| </> |
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'm also wondering: do we want to make sure that both dialogs are mounted together? I feel like passing two separate, custom objects as props is a bit much, but I don't know if we get away with splitting them up as separate exports
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 wondering about that too 🤔. Since they are suppose to always working together I thought that would be better to enforce this by exporting only one component with the two dialogs, but I'm open for suggestions.
| @@ -264,7 +264,7 @@ describe("WorkspacesPage", () => { | |||
| await user.click(getWorkspaceCheckbox(workspaces[0])); | |||
| await user.click(getWorkspaceCheckbox(workspaces[1])); | |||
| await user.click(screen.getByRole("button", { name: /actions/i })); | |||
| const stopButton = await screen.findByText(/stop/i); | |||
| const stopButton = await screen.findByRole("menuitem", { name:/stop/i }); | |||
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.
Appreciate the more accessible selectors!
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.
Me too! It makes tests easier to write.
| return ( | ||
| <TableCell> | ||
| <div className="flex gap-1 justify-end"> | ||
| {abilities.actions.includes("start") && ( |
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.
Wondering if it might be worthwhile to throw all of these into a.map callback. We're iterating overabilities.actions three times, which shouldn't be a huge deal, but a.map could let us iterate over everything exactly once
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.
Good callout! I will check the actions, but I don't think we want to have all of them in the table. With that in mind, we would have to filter the actions before mapping them, which would not provide too much improvements IMO. For now, I would leave it as it is, but I'm wondering what you would do in this case.
| }; | ||
| }; | ||
| export const useWorkspaceUpdate = ({ |
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.
Actually, now that I think about it, do we need to export the custom hook? We could keep it as an implementation detail, but couldn't we updateWorkspaceUpdateDialogs to accept the hook inputs directly, and then have it hook everything up?
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.
Could you show me an example? Or a draft the interface/usage of it?
d9b00e4 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Related to#17311
This PR adds inline actions in the workspaces page. It is a bit different of theoriginal design because I'm splitting the work into three phases that I will explain in more details in the demo.
Screen.Recording.2025-05-01.at.09.50.50.mp4