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

feat: migrate all entry screen, ctas to new add account v2 #8670

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

themooneer
Copy link
Contributor

@themooneer themooneer commented Dec 11, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by integration test of AssetSelection in this scope.
  • Impact of the changes:
    • ...

πŸ“ Description

When llmNetworkBasedAddAccount feature flag is enabled, all the ctas that open an add account process will be redirected to the new flow and support currency and related route params.
Entry point list:

  • Swap
  • Buy
  • Earn/Stake
  • Asset
  • Wallet
  • Deeplinks
  • Receive Flow

The new add account flow start either by a new Crypto selection or via Network provider (AssetSelection feature). If those informations are provided in the route params then the next step is to Connect the device and Scan & Select found account (will be adjusted in the next ticket).

Important

  • The scope of the ticket is the entry point, SelectCrypto or SelectNetwork when currency is provided in the route params.

Changes in ledger-live-common

Here is my analysis on the subject!

It is the SelectCrypto component, during its mounting, that intercepts the currency from the params and uses this info in a useEffect to simulate an explicit redirection:

  • To SelectNetwork: if the currency has at least one network provider (I will come back to this)
    Otherwise,
  • To SelectAccount: if a scan has already been done previously on this currency
    Otherwise,
  • To the SelectDevice process and scan & co

What happens during the mounting is that it intercepts ethereum as currency, calls a custom hook useGroupedCurrenciesByProvider, and triggers a useEffect to redirect if needed.

The problem is that the custom hook is slow since it depends on an asynchronous operation to return a 'result' containing 2 arrays:

  • sortedCryptoCurrencies
  • currenciesByProvider
    The first execution returns 2 empty arrays.

From the second execution, the arrays are filled.

Hence, the useEffect is triggered twice on the SelectCrypto side,

The first without providers redirects to SelectAccount,
Then the second arrives a few seconds later, following the new return of the custom hook which changes the ref of the onPress function that triggers the useEffect a second time
Consequently, a redirection to SelectNetwork occurs. This results in this strange and ambiguous behavior from a UX point of view.
The root cause of all this is that the useEffect managing potential navigation depends on an onPress callback, which in turn depends on the result of the custom hook useGroupedCurrenciesByProvider, while we do not have the information that it is loading the data. It executes with the result of its initial state and then with the filled arrays.

On the add-account, I made sure to better control this custom hook with a loading system with 4 states: idle, pending, success, and error, so that we extract this information at the instantiation of the hook.

Consequently, the useEffect only triggers if the asynchronous operation is finished (success OR error state).

  • Example of issue
addaccprovier-before.mov
  • When fixed
after-loading-provider.mov

Test case revision is done


Full Video preview

one.navigator.entry.point.mov
Post onboarding workflow test
post.onboarding.test.OK.mov

❓ Context

  • JIRA or GitHub link:

https://ledgerhq.atlassian.net/browse/LIVE-15374

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@themooneer themooneer requested a review from a team as a code owner December 11, 2024 17:16
@themooneer themooneer marked this pull request as draft December 11, 2024 17:16
Copy link

vercel bot commented Dec 11, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
web-tools βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Jan 2, 2025 8:20am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jan 2, 2025 8:20am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 2, 2025 8:20am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 2, 2025 8:20am

@live-github-bot live-github-bot bot added the common Has changes in live-common label Dec 11, 2024
@live-github-bot live-github-bot bot added the mobile Has changes in LLM label Dec 11, 2024
@live-github-bot live-github-bot bot added the translations Translation files have been touched label Dec 12, 2024
@themooneer themooneer force-pushed the feat/LIVE-15374 branch 2 times, most recently from d582aaf to e06d1a9 Compare December 13, 2024 12:19
@themooneer themooneer changed the title feat: migrate all entry screen, ctas to new add account (uncompleted … feat: migrate all entry screen, ctas to new add account v2 Dec 13, 2024
@themooneer
Copy link
Contributor Author

image
Unit test for LLC are working fine even after purging turbo cache, probably it's a flaky test on ubuntu

@themooneer themooneer merged commit a070f4e into develop Jan 3, 2025
55 checks passed
@themooneer themooneer deleted the feat/LIVE-15374 branch January 3, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants