Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NickAkhmetov/HMP-503-HOTFIX Fix workspace launch form lock #3341

Merged
merged 8 commits into from
Nov 21, 2023

Conversation

NickAkhmetov
Copy link
Collaborator

@NickAkhmetov NickAkhmetov commented Nov 21, 2023

My previous PR (#3336) introduced a regression where submitting the workspace launch dialog and having a validation failure occur locked the user out of continuing the workspace creation process until they reopened the dialog to reset form state.

This PR fixes this regression by using isSubmitSuccessful instead of isSubmitted to prevent queueing multiple workspace launches. The former is only toggled to true after validation succeeds, while the latter is set to true as soon as the user submits - even if the submit fails runtime validation.

The original issue of duplicate workspace creation requests being created is still handled.

This PR also fixes an error finding the appropriate file path to provide when launching a workspace.

Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting /workspaces/start/439?notebook_path= when checking this out locally.

@john-conroy
Copy link
Collaborator

The workspace passed to startAndOpenWorkspace only has files in request_workspace_details not current_workspace_details.

Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files in request_workspace_details do not have a leading / where the ones in current_workspace_details do. Could that affect this?

@NickAkhmetov
Copy link
Collaborator Author

I did notice the leading slash is missing, but it didn't seem to affect anything functionality-wise; for the sake of consistency, I will add logic to prepend a / if one is not already present.

Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works! Thanks!

@NickAkhmetov NickAkhmetov merged commit 0e33502 into main Nov 21, 2023
8 checks passed
@NickAkhmetov NickAkhmetov deleted the nickakhmetov/hotfix-workspace-launch branch November 21, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants