fix(ui): Only redirect/reload to wf list page when wf deletion succeeds #11676
Conversation
This file contains 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
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
evgeny-goldin
approved these changes
Aug 25, 2023
3 tasks
agilgur5
reviewed
Aug 25, 2023
| @@ -197,10 +197,12 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< | |||
| if (isArchivedWorkflow(workflow) && (globalDeleteArchived || !isWorkflowInCluster(workflow))) { | |||
| services.workflows.deleteArchived(workflow.metadata.uid, workflow.metadata.namespace).catch(setError); | |||
There was a problem hiding this comment.
I'm pretty sure that this line and the equivalent above are asynchronous promises. If they're not waited on to finish, the redirect and reload may happen before the request completes.
It is possible the requests get cancelled as a result of that, would need to see some traces to confirm that though (as it depends).
That could be the root cause here, but not really sure without looking in more detail
Comment on lines
+202
to
+204
| // TODO: This is a temporary workaround so that the list of workflows | ||
| // is correctly displayed. Workflow list page needs to be more responsive. | ||
| window.location.reload(); |
There was a problem hiding this comment.
a better workaround would be to reload only the list of workflows, i.e. have it refetch instead of reloading the entire window
Comment on lines
+200
to
+205
| if (error === null) { | ||
| navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); | ||
| // TODO: This is a temporary workaround so that the list of workflows | ||
| // is correctly displayed. Workflow list page needs to be more responsive. | ||
| window.location.reload(); | ||
| } |
There was a problem hiding this comment.
this if statement could be inverted to remove nesting. same with the if (yes) one above too
Suggested change
| if (error === null) { | |
| navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); | |
| // TODO: This is a temporary workaround so that the list of workflows | |
| // is correctly displayed. Workflow list page needs to be more responsive. | |
| window.location.reload(); | |
| } | |
| if (error !== null) { | |
| return | |
| } | |
| navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); | |
| // TODO: This is a temporary workaround so that the list of workflows | |
| // is correctly displayed. Workflow list page needs to be more responsive. | |
| window.location.reload(); |
rbreeze
approved these changes
Aug 25, 2023
5 tasks
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Potentially fix and surface the underlying issue in #11661.