Skip to content

refactor: bring in new onboarding mechanics #2406

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

Merged
merged 27 commits into from
Mar 26, 2025
Merged

Conversation

jleach
Copy link
Member

@jleach jleach commented Mar 13, 2025

This PR brings in the changes from openwallet-foundation/bifold-wallet#1464 as well as:

  • Renames "AppState" in App.tsx to be more descriptive to what it actually does;
  • Integrates the new on boarding mechanics from PR 1464
  • Integrates the new update check mechanics from PR 1466
  • Excludes a working directory from git.
  • Moves the app help URL to constants with some other URLs.

@jleach jleach changed the title Refactor/update onboarding refactor: bring in new onboarding mechanics Mar 13, 2025
@jleach jleach marked this pull request as draft March 13, 2025 21:23
@jleach jleach added the do not merge This PR is on hold. label Mar 13, 2025
jleach added 11 commits March 25, 2025 12:20
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
@jleach jleach force-pushed the refactor/update-onboarding branch from 1bd72d1 to 591ff72 Compare March 25, 2025 21:48
jleach added 13 commits March 25, 2025 14:50
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
Signed-off-by: Jason C. Leach <[email protected]>
@jleach jleach marked this pull request as ready for review March 26, 2025 00:19
@jleach jleach requested a review from bryce-mcmath March 26, 2025 00:19
@jleach jleach requested a review from al-rosenthal March 26, 2025 00:19
if (
!mounted ||
initializing.current ||
!store.authentication.didAuthenticate ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still need just the !store.authentication.didAuthenticate check here as per this PR:
openwallet-foundation/bifold-wallet#1477

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is there a reason you moved the early return after the checkForUpdates() call? Might cause it to be called many times

Copy link
Member Author

@jleach jleach Mar 26, 2025

Choose a reason for hiding this comment

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

I'm on the fence with this one. You can't get to the Splash screen unless you complete either the PIN Create or PIN Enter which both complete authentication. So if you're seeing Splash you must have completed authentication. So, on 2nd though - this change may not have been needed.

EDIT: It was needed in the previous incarnation because Splash would have routed to the PIN mechanics. This is no longer the case.

Thoughts?

I moved it after because it should be cached at this point and didAuthenticate is only required for the agent. I think it would be better to move the initialized check up if we don't want to repeat this fn call. Thoughts?

if (!agent) {
initializing.current = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still need these two, no? Or else if it fails initialization the first, the retry button won't work

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

navigation.dispatch(
CommonActions.reset({
index: 0,
routes: [{ name: Stacks.TabStack }],
})
)
} catch (e: unknown) {
initializing.current = false
Copy link
Contributor

Choose a reason for hiding this comment

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

"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

One small nit about placement of checkForUpdates() call

@@ -24,77 +22,11 @@ import TipCarousel from '../components/TipCarousel'
import useInitializeBCAgent from '../hooks/initialize-agent'
import { BCState } from '../store'

import { TermsVersion } from './Terms'

const OnboardingVersion = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

So OnboardingVersion just isn't needed anymore? That's awesome

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I can't think of any reason to keep it. We do use TermsVersion.

if (
!mounted ||
initializing.current ||
!store.authentication.didAuthenticate ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is there a reason you moved the early return after the checkForUpdates() call? Might cause it to be called many times

@jleach jleach requested a review from bryce-mcmath March 26, 2025 17:48
Copy link
Contributor

Choose a reason for hiding this comment

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

accidentally committed?

Signed-off-by: Jason C. Leach <[email protected]>
@jleach jleach force-pushed the refactor/update-onboarding branch from ddf41df to 84aa285 Compare March 26, 2025 18:59
@jleach jleach removed the do not merge This PR is on hold. label Mar 26, 2025
@jleach jleach enabled auto-merge (squash) March 26, 2025 19:08
@jleach jleach requested a review from bryce-mcmath March 26, 2025 20:36
Copy link

@jleach jleach merged commit 664eb75 into main Mar 26, 2025
26 checks passed
@jleach jleach deleted the refactor/update-onboarding branch March 26, 2025 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants