- Notifications
You must be signed in to change notification settings - Fork914
feat: add stop workspace button with confirmation dialog#18372
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
base:main
Are you sure you want to change the base?
Conversation
- Add stop workspace button with square icon that replaces start button for running workspaces- Implement confirmation dialog for stop operation to prevent accidental stops- Button shows only when workspace status allows stop action- Uses existing stopWorkspace API mutation- Follows existing UI patterns for workspace actionsFixes#18298Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>
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
Adds a stop workspace button with a confirmation dialog to complement the existing start button and prevent accidental stops.
- Introduces a stop action button (square icon) shown for running workspaces.
- Implements a confirmation dialog that prompts before calling the
stopWorkspacemutation. - Leverages existing API mutation and workspace status logic for visibility and loading indicators.
Comments suppressed due to low confidence (1)
site/src/pages/WorkspacesPage/WorkspacesTable.tsx:594
- Add tests to cover the stop workspace confirmation dialog: verify it opens on button click, displays the correct workspace name, and invokes the
stopWorkspacemutation on confirm.
<ConfirmDialogUh oh!
There was an error while loading.Please reload this page.
- Add disabled prop to PrimaryActionProps interface- Implement disabled state in PrimaryAction component- Apply disabled prop to stop button when stopWorkspaceMutation is pending- Prevents accidental duplicate stop requests during operationCo-authored-by: matifali <10648092+matifali@users.noreply.github.com>
- ConfirmDialog component doesn't accept loading prop- Fixes TypeScript compilation error in CI- Button disabled state still prevents duplicate clicksCo-authored-by: matifali <10648092+matifali@users.noreply.github.com>
@matifali I'm in a rush with the website right now, so I will not have time this weeek to QA and review this PR. Maybe someone in the FE guild, can help with that in the meantime. |
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.
Works, but we should remove thedisabled prop and just useisLoading for disablement.
Technically it might not even be necessary as a state change removes the button, but there could be some latency where the workspace is stoping and the state has not updated on the frontend yet.
Uh oh!
There was an error while loading.Please reload this page.
Use isLoading for button disabling instead of separate disabled prop.This simplifies the component interface while maintaining the samefunctionality - the button is disabled when loading.Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>
code-asher commentedJun 16, 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.
Also@matifali just to check, we have no confirmation dialog for stopping on the main workspace page (or any other actions on this table), just confirming that it was intended we add one here. |
Good point about consistency! You're right that other actions on this page don't have confirmation dialogs. Looking at the original feature request in#18298, it specifically mentioned wanting a confirmation dialog to prevent accidental stops. However, for consistency with the rest of the workspaces table actions, we could remove the confirmation dialog. Would you prefer I remove the confirmation dialog to match the existing pattern, or keep it since stopping a workspace is a more disruptive action than starting one? |
Remove empty line left after removing disabled prop to satisfybiome formatter requirements.Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>
This is a more accessible button, so IMO, there is a greater chance of hitting stop by accident, which is why I added the confirmation dialogue. |
Uh oh!
There was an error while loading.Please reload this page.
Remove the disabled prop from PrimaryActionProps interface andcomponent implementation since we now only use isLoading forbutton disabling.Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>
code-asher left a comment• 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.
Makes sense, also I was wrong about there being no dialogs on this table, the update action does have a confirmation prompt.
Looks good!
Summary
Adds a stop workspace button with confirmation dialog to complement the existing start button, addressing the feature request in#18298.
Changes
stopWorkspaceAPI mutationBehavior
Testing
Fixes#18298