Fix actions fetching logic and loading state, prevent duplicate toasts

[silverwind]

1. Fix the bug where action steps would not expand sometimes. This was because when a fetch was running and the user clicked "expand" it would not propagate the expansion request to the backend, leaving the UI state invalid. Fixed this by adding a force flag.
2. When a fetch failed, it would just throw the error and the browser would dump it into the console, invisible to the user. Show a toast in such a case.
3. Add a dedicated loading state to steps to ensure they can not get stuck in that state when cursor remains at null, for example when a fetch error happened.

Also, because in the actions view, fetches can fail repeatedly because of the refreshing, I saw the need to introduce a deduplication for toasts, so if an attempt is made to raise a toast with the same message and level, it will suppress opening them.

[lunny]

Could we split these as two PRs for easier review?

[silverwind]

Can't easily split this because if I remove only the toast changes, there would be the problem of repeated error toasts every second because of that refresh mechanism. The only thing that could be done is to have a reduced backport that only fixes issue 1 with the force flag.

[silverwind]

BTW one reason we need the toast deduplication: Go to admin dashboard and disconnect your network:

[wxiaoguang]

Missed this review request.

No block from my side (I don't use Actions and I haven't spent time on reading code), just share some opinions:

1. the loadData state management becomes more and more complex and fragile, at first glance I can't tell whether it is complete but I guess there could be some edge cases, for example: force=true will reset other this.loading=true
2. maybe there should be some tests to cover some functions (even without e2e test, we could mock and test some code blocks)
3. the change of showToast is quite hacky
   • preventDuplicates should not default to true, it is a breaking behavior.
   • it should hide the existing toast, but not skip the new toast.
4. it shouldn't be backported since the change is not covered by tests, and it doesn't fix blocker problems for end users.

[silverwind]

maybe there should be some tests to cover some functions (even without e2e test, we could mock and test some code blocks)

Quite hard I assume unless Vue works in happy-dom, but I would doubt that. Maybe with some Vue-specific testing framework, but I have no experience with those.

preventDuplicates should not default to true, it is a breaking behavior.

It helps with other cases like systems stats flooding the page with error toasts when going offline.

it should hide the existing toast, but not skip the new toast.

This is actually a nice idea, I will try to implement.

it shouldn't be backported since the change is not covered by tests, and it doesn't fix blocker problems for end users.

It's quite annoying when you click an action step and nothing happens though. Happens to me like a third of the time.

[silverwind]

Will also fix the boolean trap with loadData :)

[wxiaoguang]

maybe there should be some tests to cover some functions (even without e2e test, we could mock and test some code blocks)

Quite hard I assume unless Vue works in happy-dom, but I would doubt that. Maybe with some Vue-specific testing framework, but I have no experience with those.

Maybe something like "Improve <SvgIcon> to make it output svg node and optimize performance (#23570)"

[silverwind]

Right, this createApp + mount pattern I had a helper function in another PR, so likely tests could use it in place of this h function.

BTW, I think I will extract the toast changes to another PR. I have a few improvements in mind.

[silverwind]

preventDuplicates should not default to true, it is a breaking behavior.

Ended up following your suggestion and defaulting to false, so it requires opt-in at the caller and I did so on all known callers that do fetch in a loop. Maybe later we could make it default for all error level toasts.

[wxiaoguang]

Thought again about this and I think it's not ideal to have a toast constantly re-show. It's bad UX as user can not easily select text in the toast.

I will try to propose a solution to see whether it is better. There are some details in my mind.

[wxiaoguang]

-> Make toast support preventDuplicates #31501

[silverwind]

I'll merge this now for the bugfix part. We can iterate on the toast in #31501.

[wxiaoguang]

I have objection for backporting, unless TOC agrees to backport.

[wxiaoguang]

And one more thing, could you revert the toast related code?

[silverwind]

I can remove the toast code in favor of #31501 if you port all my changes from here (htmx and actions view preventDuplicates) into it.

I don't really care about the backport so much as I'm not using Actions, but other users might and I think it's a important bugfix for them.

[wxiaoguang]

I don't really care about the backport so much as I'm not using Actions, but other users might and I think it's a important bugfix for them.

The problem is that the code really changed a lot and I am not sure there would be regressions, for example:

the loadData state management becomes more and more complex and fragile, at first glance I can't tell whether it is complete but I guess there could be some edge cases, for example: force=true will reset other this.loading=true

I think as a bug fix it is really good to end users and the work is awesome, while I also think we should make 1.22 branch as stable as possible to avoid introducing new regressions (especially for a large PR like this)

So, maybe if this change is stable enough in 1.23, then we could backport it to the next 1.22 release (eg 1.22.2 or 1.22.3)

[wxiaoguang]

Here and above: why it needs new two if here? They are from [job, artifacts] = await Promise.all, and the response shouldn't be null.

[wxiaoguang]

If the await Promise.all fails, none of these 2 variables could have value.

So it should return early in the catch block above, but not use extra if blocks here.

[wxiaoguang]

I think the root problem is that the state management in this loadData/loadJob is quite messy (because after the initial version, then people only add code to "patch" it).

As a bug fix, I think this PR could work. While if we'd like to make the code right, I guess we need to rewrite the code and re-design the state management logic. For example, maybe this.loading doesn't make sense now, we need to track every request's state separately.

(Just some new thoughts)

[silverwind]

The ifs are meant to perform a partial state update if one of the two requests fails (which would be caught and show a toast).

[silverwind]

But I see you are right, Promise.all can not deliver partial results, it either resolves both or rejects with the first encountered error, so my interpretion of the return value was wrong and the ifs are not needed.

[wxiaoguang]

I can remove the toast code in favor of #31501 if you port all my changes from here (htmx and actions view preventDuplicates) into it.

I think #31501 could satisfy your requirement, it uses preventDuplicates=true and provides necessary visual feedback, so the htmx and actions view should work well without any change.

[silverwind]

Ah, I see you went with the "default enabled" variant, so yes in this case we don't need to special-case these callers. I will remove all toast-related code from this commit.

[wxiaoguang]

OK, I am sure the state management is not right now. See the duplicate lines:

That's my first worry in #31124 (review)

[wxiaoguang]

@silverwind you edited my comment ...... (reverted)

To answer:

Got some steps how to reproduce that?

I think the reason is still related to the "force" behavior: to produce, make the backend sleep 500-1500ms to respond to make the edge cases could easily be triggered, then toggle the steps multiple times, then since you use force to reset the loading, then there are multiple requests in flight, then they get duplicate results.

[silverwind]

Sorry, meant to reply. I will rework this later.