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

ABW-1839 App spills some runtime warnings after the creation of an account on simulator #596

Merged
merged 18 commits into from
Jun 30, 2023

Conversation

maciek-rdx
Copy link
Contributor

@maciek-rdx maciek-rdx commented Jun 28, 2023

Jira ticket: paste link here

Description

This PR solves the problem with warnings. In the scope I cleaned up the unnecessary @MainActor attributes, as well as eliminated the noop CreationOfAccount.ViewAction.onFirstTask.

Notes

The journey has been discussed here in Slack. I think it would be good to ultimately solve it in a more generic way, but the current implementation should be fine for now.

How to test

I'd suggest trying on at least two devices, just to make sure, as this is kind of a critical path for first time users.

Happy path (or test variation 1)

  1. Install fresh app
  2. Go through the Onboarding and create an account

Unhappy path (or test variation 2)

  1. In Home add new account

I'd also suggest smoke-testing for other onboarding paths (restore, ledger).

Video

As requested by @CyonAlexRDX:

RPReplay_Final1688030995.MP4

PR submission checklist

  • I have tested on simulator and device

@@ -98,6 +98,8 @@ public struct DerivePublicKeys: Sendable, FeatureReducer {
case .device:
return .task {
do {
try? await Task.sleep(for: .seconds(1))
Copy link
Contributor Author

@maciek-rdx maciek-rdx Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, right?! Better than what we have now.

The runtime issues only went away when using 1000 ms..? What happens if you use 250ms?

@maciek-rdx
Copy link
Contributor Author

maciek-rdx commented Jun 29, 2023

Looks good, right?! Better than what we have now.

The runtime issues only went away when using 1000 ms..? What happens if you use 250ms?

It's better for sure, as it eliminates warnings and overengineering (the "noop" ViewAction in CreationOfAccount and @MainActors).

We're good with 500 + something... 700 is a safe bet I think. Will clean it up and make a proper PR tomorrow 👍

JFYI @CyonAlexRDX - we may also consider future-proofing NavigationStackStore (or I can propose it independently to TCA guys).

Base automatically changed from maciek/temp/preview to main June 30, 2023 11:35
@maciek-rdx maciek-rdx changed the title DRAFT: Abw 1839 runtime warnings ABW-1839 App spills some runtime warnings after the creation of an account on simulator Jun 30, 2023
@maciek-rdx maciek-rdx marked this pull request as ready for review June 30, 2023 11:49
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.