refactor: rendering workflow list after perfoming action button in WorkflowToolBar

[juijeong8324]

Follow up to #11891

Motivation

While refactoring WorkflowsToolBar to fucntional components, I found that Workflow List wasn't rendering after performing selected workflows by clicking the action button in WorkflowsToolBar, It was redering by clicking workflows page button.

Example, below
click the delete button in WorkflowsToolBar, you can see workflow list is not rendering while notification shows.

Modifications

• Add to performed state as boolean type
• Add performed as dependency array in useEffect that rendering workflowList
• convert loadWorkflows function values clearSelectedWorkflows -> loadPerformedWorkflows. It update performed state.

Verification

• workflows list in the initial state

• workflow list after DELETE button

• workflow list after RESUBMIT button

[agilgur5]

Partial fix for #9810

Rather a follow-up to #11891

click the delete button in WorkflowsToolBar, you can see workflow list is not rendering while notification shows

So it should reload once the Workflows are deleted, does it not? I'm not sure if I tested delete specifically, but I definitely tested resubmit and that worked as expected.

The ListWatch should eventually automatically update the WorkflowsList when the k8s objects are deleted. Does it not do that if you wait a bit?

The previous behavior, before #11891, did something similar to the logic you have here, but that meant it did a secondary network request while cancelling the existing stream -- i.e. unnecessary networking. This change will result in the same behavior

So I don't think this is a bug, but we could add an optimistic update on the UI for the delete scenario

[juijeong8324]

click the delete button in WorkflowsToolBar, you can see workflow list is not rendering while notification shows

So it should reload once the Workflows are deleted, does it not? I'm not sure if I tested delete specifically, but I definitely tested resubmit and that worked as expected.

The ListWatch should eventually automatically update the WorkflowsList when the k8s objects are deleted. Does it not do that if you wait a bit?

The previous behavior, before #11891, did something similar to the logic you have here, but that meant it did a secondary network request while cancelling the existing stream -- i.e. unnecessary networking. This change will result in the same behavior

So I don't think this is a bug, but we could add an optimistic update on the UI for the delete scenario

Ah!!! I got it!! Thanks for the clarification!

So, It is not bug since result is same behavior!

But I have some quesitons!

1. fix pr convetntion means to fix a bug, What can I say in this PR? (refactor? or feat?)
2. I tested serveral cases for this. Sometimes the workflow list loads automatically as expected, but sometimes it needs to be updated to reload! Why does this happen?!

Above Gif is what I tested.
The update in workflows list was not automatically. Also, It doesn't do even I wait a bit.

[juijeong8324]

So I don't think this is a bug, but we could add an optimistic update on the UI for the delete scenario

Yeah! great!

[agilgur5]

1. fix pr convetntion means to fix a bug, What can I say in this PR? (refactor? or feat?)

Good question. The PR as it currently is I suppose would be more of a "refactor". It's a bit of a gray area as it does improve usability, so it could be considered a "fix" as well.

2. I tested serveral cases for this. Sometimes the workflow list loads automatically as expected, but sometimes it needs to be updated to reload! Why does this happen?!

Above Gif is what I tested.
The update in workflows list was not automatically. Also, It doesn't do even I wait a bit.

The GIF does look like everything gets updated eventually, just not instantly. Unless I'm missing something there?

Deletions are perhaps slower in the event stream as they require a cache eviction on the server-side Informer as well, vs. additions, like through resubmits, are captured faster. Not sure to be honest.

So I don't think this is a bug, but we could add an optimistic update on the UI for the delete scenario

Yeah! great!

Do you want to rework this PR or file a new PR for that? I think the WorkflowsToolbar may need some refactoring to allow for that, as the current prop, loadWorkflows, does not specify the action being taken. That would probably need to be re-written and re-named.

[GeunSam2]

I think UI improvement could be another solution.
something like, showing progress icon to target items until items disapear from list.

[juijeong8324]

The previous behavior, before #11891, did something similar to the logic you have here, but that meant it did a secondary network request while cancelling the existing stream -- i.e. unnecessary networking. This change will result in the same behavior

Ah!! You means that.. It doesn't need to care that workflow was performed but not rendered automatically in the workflow list UI?? (Did I understand corretly?)

Actually I thought it should have improvement because the UI is updated by Triggers(click another button for rendering this workflow list), not automatically, even though workflows was already perfomed.

Do you want to rework this PR or file a new PR for that? I think the WorkflowsToolbar may need some refactoring to allow for that, as the current prop, loadWorkflows, does not specify the action being taken. That would probably need to be re-written and re-named.

Umm I think it is better to work refactoring WorkflowsToolbar first.
I will reopen new PR for this after refactoring WorkflowsToolbar!

[agilgur5]

Ah!! You means that.. It doesn't need to care that workflow was performed but not rendered automatically in the workflow list UI?? (Did I understand corretly?)

The ListWatch will auto-update to match the server whenever there is a server-side change. So it should eventually render the correct state.
We shouldn't need to re-run the ListWatch, that is unnecessary networking. The ListWatch is always subscribed to the event stream already.

But it seems like the ListWatch may be slow to update in some cases (like delete), so an optimistic update may help with that.

Actually I thought it should have improvement because the UI is updated by Triggers(click another button for rendering this workflow list), not automatically, even though workflows was already perfomed.

Since we already have the ListWatch subscription, it will eventually get an update from the event stream.

Clicking the delete button (an "action" in the code) creates a delete request, which will eventually result in a delete event in the event stream as well.
But if it is slow, we can additionally delete the Workflows on the client-side as an optimistic update.

[juijeong8324]

Clicking the delete button (an "action" in the code) creates a delete request, which will eventually result in a delete event in the event stream as well. But if it is slow, we can additionally delete the Workflows on the client-side as an optimistic update.

Wow...thank you so much for the detailed explanation!! 😎😎
I completely understand the logic!
I will reconsider the issue of slow UI updates! Thank you🫡

[agilgur5]

Following up here that the ListWatch did indeed have an update issue, but not due to networking actually. It was due to a stale internal ref; fixed it in #12562, see root cause analysis in #12327 (comment)