Skip to content
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

fix: Make onboarding a bit more robust when coordinator is down #2603

Closed
wants to merge 1 commit into from

Conversation

Restioson
Copy link
Contributor

This PR adds two related commits which do the following:

  1. Show an error to the user if the coordinator is down (or unavailable) during onboarding instead of just hanging
  2. If the coordinator is down during onboarding, try to register them for the beta program the next time they launch the app instead of just leaving them unregistered.

This might address #2430 somewhat, but I'm currently unclear exactly what the issue with #2430 is.

@Restioson Restioson self-assigned this Jun 4, 2024
@holzeis
Copy link
Contributor

holzeis commented Jun 4, 2024

I think we can't delay the user registration to the next start as the user might trade once the coordinator is back online. This will result in errors if the user is not registered.

@Restioson
Copy link
Contributor Author

I think we can't delay the user registration to the next start as the user might trade once the coordinator is back online. This will result in errors if the user is not registered.

Is the user able to create a position whilst the coordinator is offline? Or is this in the case of the coordinator coming back whilst the user is still online?

@Restioson
Copy link
Contributor Author

Perhaps this could be addressed by checking if the user is registered every time before they open a position?

@holzeis
Copy link
Contributor

holzeis commented Jun 6, 2024

Is the user able to create a position whilst the coordinator is offline? Or is this in the case of the coordinator coming back whilst the user is still online?

If the coordinator is coming back, while the user is online.

@holzeis
Copy link
Contributor

holzeis commented Jun 6, 2024

Perhaps this could be addressed by checking if the user is registered every time before they open a position?

I wouldn't make it too complicated.. I think your solution of simply retrying to register, when the app starts the next time is fine.

@holzeis
Copy link
Contributor

holzeis commented Jun 6, 2024

@Restioson This feels a bit weird. I would go for the warning we already show when you open the app after you have registered. "Coordinator is not available" but show the app.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-06-06.at.13.58.40.mp4

@Restioson
Copy link
Contributor Author

@holzeis: I've gone for showing both warnings. Now, it will still show that you have failed to register for beta but also continue to wallet and then show that the coordinator is offline. I did not want to simply silence the failure to register for beta as I think the user should still know about it.

@holzeis
Copy link
Contributor

holzeis commented Jun 12, 2024

@holzeis: I've gone for showing both warnings. Now, it will still show that you have failed to register for beta but also continue to wallet and then show that the coordinator is offline. I did not want to simply silence the failure to register for beta as I think the user should still know about it.

Can you share a video.. I would like to see how this looks like. There was also the issue of a hanging loading screen.

@Restioson
Copy link
Contributor Author

For future reference: @holzeis and I discussed that the additional warning about beta signup should be removed, since there is no observable effect of this failure (it's only internal). If this changes, like if we add an email notification or the like, we could consider reintroducing it as an extra line on the main 'coordinator down' warning.

Previously we would just crash and never retry. Now, we don't crash, and do retry next boot of the app.
@Restioson Restioson force-pushed the fix/show-coord-error-onboarding branch from b7e8788 to bb6b936 Compare June 18, 2024 15:26
@Restioson
Copy link
Contributor Author

Restioson commented Jun 18, 2024

I changed it to a snackbar just to make it consistent with the import seed which does the same thing when it's LoadingScreenTask fails. I did it in such a way that I can remove it very easily though if you think it should go though @holzeis, on purpose 😄

@holzeis
Copy link
Contributor

holzeis commented Jun 18, 2024

I changed it to a snackbar just to make it consistent with the import seed which does the same thing when it's LoadingScreenTask fails. I did it in such a way that I can remove it very easily though if you think it should go though @holzeis, on purpose 😄

Sounds reasonable to me, can you share a video?

@Restioson
Copy link
Contributor Author

Kazam_screencast_00002.webm

@github-actions github-actions bot added the Stale label Jul 20, 2024
@github-actions github-actions bot closed this Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants