Fix actions fetching logic and loading state, prevent duplicate toasts

[silverwind]

trafficstars

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:

[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]

force=true will reset other this.loading=true

A ideal implementation would detect a request with same args and any subsequent calls with those args while a request is already running would yield a promise that resolves with the response of the running invocation, but I feel that should be done in the request function, not here and it's likely not an issue for this case anyways.

[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.

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

Meaningfully unit-testing components like RepoActionView requires a fetch mock like https://github.com/mswjs/msw which is a interesting topic but honestly too much work for this. I tested the functionality again locally and it works as expected. I'm willing to take this "risk" with this PR and backport it as well.

[silverwind]

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

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.

We should implement a counter like we have on the global error handler (data-global-error-msg-count) to indicate a toast was triggered more than once. Also, if the user hides such a toast, it should stay hidden and not trigger any more same toasts, at least for the error case. I will take a look at this for 1.23 in another PR.

[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 https://github.com/go-gitea/gitea/pull/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 https://github.com/go-gitea/gitea/pull/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]

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 https://github.com/go-gitea/gitea/pull/31124#pullrequestreview-2134345703

[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.

[silverwind]

FYI https://codeberg.org/forgejo/forgejo/pulls/6122 is related and deals with the same bug of the job step sometimes not expanding.

[wxiaoguang]

FYI https://codeberg.org/forgejo/forgejo/pulls/6122 is related and deals with the same bug of the job step sometimes not expanding.

Just take a quick look, I do not think it is right, it seems to be a hacky patch.

[wxiaoguang]

I will try to work on it in the following few days, to see whether we could properly fix it in 1.23

[silverwind]

I will try to work on it in the following few days, to see whether we could properly fix it in 1.23

Thanks, I haven't gotten around to this. I hope you can reproduce the issue. It happens when a job is expanded while the fetch is still running.

[wxiaoguang]

Done, see this: Refactor RepoActionView.vue #32713

[lunny]

Replaced by #32713 ?

[silverwind]

Yes, that includes the most important fix. As for the other 2 topics, I think we can do them another time.