feat: add extra workspace actions in the workspaces table #17775
Next
Next commit
Refactor base menu component
- Loading branch information
commit 496473f425fc5a44c7781c0031150c2d80e555ce
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,19 @@ | ||
| import { API } from "api/api"; | ||
| import type { | ||
| AuthorizationRequest, | ||
| AuthorizationResponse, | ||
| } from "api/typesGenerated"; | ||
|
|
||
| const AUTHORIZATION_KEY = "authorization"; | ||
|
|
||
| export const getAuthorizationKey = (req: AuthorizationRequest) => | ||
| [AUTHORIZATION_KEY, req] as const; | ||
|
|
||
| export const checkAuthorization = <TResponse extends AuthorizationResponse>( | ||
| req: AuthorizationRequest, | ||
| ) => { | ||
| return { | ||
| queryKey: getAuthorizationKey(req), | ||
| queryFn: () => API.checkAuthorization<TResponse>(req), | ||
| }; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import { Button } from "components/Button/Button"; | ||
| import { | ||
| DropdownMenu, | ||
| DropdownMenuContent, | ||
| DropdownMenuItem, | ||
| DropdownMenuSeparator, | ||
| DropdownMenuTrigger, | ||
| } from "components/DropdownMenu/DropdownMenu"; | ||
| import { | ||
| EllipsisVertical, | ||
| SettingsIcon, | ||
| HistoryIcon, | ||
| TrashIcon, | ||
| CopyIcon, | ||
| DownloadIcon, | ||
| } from "lucide-react"; | ||
| import { useState, type FC } from "react"; | ||
| import { Link as RouterLink } from "react-router-dom"; | ||
|
|
||
| type WorkspaceMoreActionsProps = { | ||
| isDuplicationReady: boolean; | ||
| disabled?: boolean; | ||
|
There was a problem hiding this comment. Two points of feedback on this prop:
The dropdown is able to be open while the button is disabled. Steps to reproduce:
You have more context than I do, but I was thinking:
There was a problem hiding this comment. Though I guess if we want to make sure the dots always appear for aesthetic consistency, no matter what transitions we have, another option is to make it so that the dropdowns have conditional rendering tied to the There was a problem hiding this comment. I think this should be solved not in the component level, but in the data level. What I want to say is, when stopping the workspace, or triggering a build, we should revalidate the workspaces data to represent the correct state of each workspace. I'm going to take a look into that 👍 There was a problem hiding this comment. lol, update the data is more complex than I thought because of pagination so I will just close the menu when the button becomes disabled. |
||
| onDuplicate: () => void; | ||
| onDelete: () => void; | ||
| onChangeVersion?: () => void; | ||
| permissions?: { | ||
| changeWorkspaceVersion?: boolean; | ||
| }; | ||
| }; | ||
|
|
||
| export const WorkspaceMoreActions: FC<WorkspaceMoreActionsProps> = ({ | ||
| disabled, | ||
| isDuplicationReady, | ||
| onDuplicate, | ||
| onDelete, | ||
| onChangeVersion, | ||
| permissions, | ||
| }) => { | ||
| // Download logs | ||
| const [isDownloadDialogOpen, setIsDownloadDialogOpen] = useState(false); | ||
| const canChangeVersion = permissions?.changeWorkspaceVersion !== false; | ||
|
|
||
| return ( | ||
| <> | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| size="icon-lg" | ||
| variant="subtle" | ||
| data-testid="workspace-options-button" | ||
| aria-controls="workspace-options" | ||
| disabled={disabled} | ||
| > | ||
| <EllipsisVertical aria-hidden="true" /> | ||
| <span className="sr-only">Workspace actions</span> | ||
|
There was a problem hiding this comment. Appreciate the screenreader support text! |
||
| </Button> | ||
| </DropdownMenuTrigger> | ||
|
|
||
| <DropdownMenuContent id="workspace-options" align="end"> | ||
| <DropdownMenuItem asChild> | ||
| <RouterLink to="settings"> | ||
| <SettingsIcon /> | ||
| Settings | ||
| </RouterLink> | ||
| </DropdownMenuItem> | ||
|
|
||
| {onChangeVersion && canChangeVersion && ( | ||
| <DropdownMenuItem onClick={onChangeVersion}> | ||
| <HistoryIcon /> | ||
| Change version… | ||
| </DropdownMenuItem> | ||
| )} | ||
|
|
||
| <DropdownMenuItem | ||
| onClick={onDuplicate} | ||
| disabled={!isDuplicationReady} | ||
| > | ||
| <CopyIcon /> | ||
| Duplicate… | ||
| </DropdownMenuItem> | ||
|
|
||
| <DropdownMenuItem onClick={() => setIsDownloadDialogOpen(true)}> | ||
| <DownloadIcon /> | ||
| Download logs… | ||
| </DropdownMenuItem> | ||
|
|
||
| <DropdownMenuSeparator /> | ||
|
|
||
| <DropdownMenuItem | ||
| className="text-content-destructive focus:text-content-destructive" | ||
| onClick={onDelete} | ||
| data-testid="delete-button" | ||
| > | ||
| <TrashIcon /> | ||
| Delete… | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| </> | ||
| ); | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import type { AuthorizationCheck, Workspace } from "api/typesGenerated"; | ||
|
|
||
| export const workspaceChecks = (workspace: Workspace) => | ||
| ({ | ||
| readWorkspace: { | ||
| object: { | ||
| resource_type: "workspace", | ||
| resource_id: workspace.id, | ||
| owner_id: workspace.owner_id, | ||
| }, | ||
| action: "read", | ||
| }, | ||
| updateWorkspace: { | ||
| object: { | ||
| resource_type: "workspace", | ||
| resource_id: workspace.id, | ||
| owner_id: workspace.owner_id, | ||
| }, | ||
| action: "update", | ||
| }, | ||
| updateWorkspaceVersion: { | ||
| object: { | ||
| resource_type: "template", | ||
| resource_id: workspace.template_id, | ||
| }, | ||
| action: "update", | ||
| }, | ||
| // We only want to allow template admins to delete failed workspaces since | ||
| // they can leave orphaned resources. | ||
| deleteFailedWorkspace: { | ||
| object: { | ||
| resource_type: "template", | ||
| resource_id: workspace.template_id, | ||
| }, | ||
| action: "update", | ||
| }, | ||
| // To run a build in debug mode we need to be able to read the deployment | ||
| // config (enable_terraform_debug_mode). | ||
| deploymentConfig: { | ||
| object: { | ||
| resource_type: "deployment_config", | ||
| }, | ||
| action: "read", | ||
| }, | ||
| }) satisfies Record<string, AuthorizationCheck>; | ||
|
There was a problem hiding this comment. I was skeptical of whether we needed this (as opposed to explicitly returning |
||
|
|
||
| export type WorkspacePermissions = Record< | ||
| keyof ReturnType<typeof workspaceChecks>, | ||
| boolean | ||
| >; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 concerned about why we have a generic for a function that checks anything auth-based. Does the server really give back multiple return types based on what parameters we give it?
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, I'm a little concerned that with how the function is set up right now, we force anyone calling this function to pass in the correct type parameter. To some degree that's okay, since we have such a specific type constraint on here, I have to wonder if this is asking for trouble, not just as far as complexity, but also in terms of some other employee passing the wrong type in a few months from now
Two things come to mind:
paramsso that we can infer the return type automatically. With less need to pass in types manually, there's less room for things to go wrongTResponse extends TypesGen.AuthorizationResponse = TypesGen.AuthorizationResponse. That way, most users get back the baseAuthorizationResponsetype by default (which I'd imagine would be okay 99% of the time, since we haven't needed this constraint until now), and then if someone needs something more specific, they have the ability to pass in the type, and override the default return valueThere 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 not possible right now since the response is dynamic and depends on the request parameters. However, I do agree it would be better to have something more predictable to avoid these kinda of errors.