Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[BUG] Transition issue #227

Closed
2 tasks done
ScreamZ opened this issue Aug 7, 2024 · 6 comments
Closed
2 tasks done

[BUG] Transition issue #227

ScreamZ opened this issue Aug 7, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@ScreamZ
Copy link

ScreamZ commented Aug 7, 2024

Are you using the latest version of this library?

  • I verified that the issue exists in the latest next-safe-action release

Is there an existing issue for this?

  • I have searched the existing issues and found nothing that matches

Describe the bug

Hello @TheEdoRan

I've already detailled everything here, just added a repro. For me this is a bug.

#218

Reproduction steps

Try both buttons, in the actual pattern because isExecuting is back to false, before the redirect occurs you get small glitches on redirects.

Click the one that uses useAction after the 3 seconds delay, you quickly get isExecuting to false.
This means the button gets its "clickable" state again for milliseconds, before getting redirected.

Adding the state from the transition (named isPendingTransition) to isExecuting as a second requirement seems a quick fix, currently, you're just discarding it.

✅ Expected behavior

Click the button that uses useTransition, and when the 3-second delay finishes, you get redirected without a UI glitch. (because isExecuting is back to false, before the redirect occurs)

Maybe isExecuting could be: isExecuting && isPendingTransition instead of isExecuting?

isExecuting (internal) && isPendingTransition (from useTransition)

Minimal reproduction example

https://codesandbox.io/p/devbox/repro-bug-qs8svh

Operating System

MacOS

Library version

7.4.3

Next.js version

14.2.1

Additional context

No response

@ScreamZ ScreamZ added the bug Something isn't working label Aug 7, 2024
@TheEdoRan
Copy link
Owner

Will look into it soon, thank you for providing a reproduction example.

@TheEdoRan
Copy link
Owner

Update: this problem is quite complex. If I use just isPending from the useTransition hook, then callbacks get called twice and onSuccess/onSettled never get called when doing a redirect, so it's not a viable option. If instead, like until now, I use isExecuting from the state, callbacks get invoked correctly (which is why it's implemented this way), but then, as you found out, the state value goes back to false before concluding the redirect, creating weird glitches.

The thing is: if I return the value of isExecuting || isPending to the client, then the hasSucceeded status is never set after a redirect, because it works by throwing an error caught by Next.js, and I assume that the component gets unmounted after completion.
I could also return both isExecuting and isPending separately, but hasSucceeded status is also never reached in this case.

If you have any ideas please let me know, I'm still trying to figure out what's the best way to fix this issue while keeping the correct callbacks behavior.

@TheEdoRan
Copy link
Owner

#230 adds isTransitioning and isPending shorthand statuses, as described in the PR body. Not entirely happy about this solution, but at least this way you'll be able to access the under the hood transition state.

@ScreamZ
Copy link
Author

ScreamZ commented Aug 14, 2024

I concede this is a complex one… Maybe we could use some memoization with request ID to avoid callback getting triggered twice (if it has already been triggered, a flag is turned on), something similar we do with one-time useEffect.

My first question is: are you really sure that onSuccess/onSettled callbacks are triggered all the time, or it's just race-condition that those callbacks are executed while the redirection occurs? What happens if I do some long-running job (like 5 sec) in onSuccess?

I think we have to make some tradeoffs. But returning multiple fields as you did in the #230, could be a nice first workaround. Something similar is done by react-query with the pending state. So at first I'm ok with it. We just need to document it well.

I'll think about it further. But currently, I'm forced to manage useTransition outside.

Maybe we could fake the isExecuting to be updated when useTransition ends, with some useEffect? This starts getting hacky but…

@TheEdoRan
Copy link
Owner

My first question is: are you really sure that onSuccess/onSettled callbacks are triggered all the time, or it's just race-condition that those callbacks are executed while the redirection occurs? What happens if I do some long-running job (like 5 sec) in onSuccess?

That was my question as well when I first saw the redirect behavior, but onSuccess/onSettled callbacks are indeed fired every time a redirect occurs, even if you await something inside of them, so at least that works as expected.

I think we have to make some tradeoffs. But returning multiple fields as you did in the #230, could be a nice first workaround.

Nice, I agree. It will improve the UX for the time being.

Maybe we could fake the isExecuting to be updated when useTransition ends, with some useEffect? This starts getting hacky but…

Yeah, very hacky, I'll see if I can come up with a nice working solution. If you think you've found one, feel free to submit a PR with a proper fix. I'll keep this issue open for now.

TheEdoRan added a commit that referenced this issue Aug 14, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…to return object (#230)

This PR introduces a workaround for the issues described in #227, by adding `isTransitioning` and `isPending` shorthand statuses to the hooks return object. `isTransitioning` is the value returned from the `useTransition` hook that is used under the hood, while `isPending` has the value of `isExecuting || isTransitioning`, for convenience.

re #227
@TheEdoRan
Copy link
Owner

TheEdoRan commented Aug 14, 2024

Added documentation for checking action status on the docs website: here.

TheEdoRan added a commit that referenced this issue Aug 14, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…to return object (#231)

This PR introduces a workaround for the issues described in #227, by adding `isTransitioning` and `isPending` shorthand statuses to the hooks return object. `isTransitioning` is the value returned from the `useTransition` hook that is used under the hood, while `isPending` has the value of `isExecuting || isTransitioning`, for convenience.
Repository owner locked and limited conversation to collaborators Aug 14, 2024
@TheEdoRan TheEdoRan converted this issue into discussion #232 Aug 14, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants