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

Clear out loading indicator when asset discovery is not being run #30921

Closed
HEagle18 opened this issue Jun 8, 2023 · 4 comments · Fixed by brave/brave-core#20738
Closed

Clear out loading indicator when asset discovery is not being run #30921

HEagle18 opened this issue Jun 8, 2023 · 4 comments · Fixed by brave/brave-core#20738
Assignees
Labels
bug feature/web3/wallet Integrating Ethereum+ wallet support front-end-change This task is a front end task and doesn't need any C++ changes OS/Android Fixes related to Android browser functionality OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass - Android ARM QA Pass-Win64 QA/Yes release-notes/include

Comments

@HEagle18
Copy link

HEagle18 commented Jun 8, 2023

Description

Issue - loading indicator keeps going at the bottom of the wallet portfolio

Please see conversation here --> https://bravesoftware.slack.com/archives/C023VS4HJ6Q/p1686164364305479

Nick's comments - ´I think the loading indicator is not being cleared in cases when asset discovery is not being run on portfolio refresh due to rate limiting. I think the frontend is waiting for an onDiscoverAssetsCompleted event that will never actually come.
Screenshot 2023-06-07 at 20 13 03

The frontend might be assuming asset discovery is run on every single portfolio refresh, when it should only make that assumption when it receives a onDiscoverAssetsStarted event´

Steps to Reproduce

Actual result:

Expected result:

Reproduces how often:

Desktop Brave version:

Android Device details:

  • Install type (ARM, x86):
  • Device type (Phone, Tablet, Phablet):
  • Android version:

Version/Channel Information:

  • Can you reproduce this issue with the current release?
  • Can you reproduce this issue with the beta channel?
  • Can you reproduce this issue with the nightly channel?

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields?
  • Does the issue resolve itself when disabling Brave Rewards?
  • Is the issue reproducible on the latest version of Chrome?

Miscellaneous Information:

@HEagle18 HEagle18 added OS/Android Fixes related to Android browser functionality OS/Desktop front-end-change This task is a front end task and doesn't need any C++ changes labels Jun 8, 2023
@HEagle18
Copy link
Author

@Douglashdaniel @nvonpentz to discuss exactly what's needed here.

@HEagle18 HEagle18 added the priority/P3 The next thing for us to work on. It'll ride the trains. label Jun 23, 2023
@nvonpentz
Copy link
Member

nvonpentz commented Jun 27, 2023

I think this is what's happening @Douglashdaniel

  1. Every time the portfolio page is refreshed, the frontend displays the loading spinner until it receives an onDiscoverAssetsCompleted event.
  2. When the portfolio page is refreshed multiple times in under a minute, the backend applies rate limiting, and does not fulfill the subsequent requests, and thus no onDiscoverAssetsCompleted event is emitted.
  3. In these cases the loading spinner is not cleared until the user waits a minute and refreshes again

If that's right, then I think this can be fixed by the frontend using the onDiscoverAssetsStarted event to display the loading spinner rather than just portfolio refresh. There should be a guarantee that an onDiscoverAssetsCompleted event will follow every time. Also, there should not be any overlap, e.g. two onDiscoverAssetsStarted events in a row before an onDiscoverAssetsCompleted event.

Ref #27776

@brave-builds brave-builds added the feature/web3/wallet Integrating Ethereum+ wallet support label Oct 30, 2023
@brave-builds brave-builds added this to the 1.62.x - Nightly milestone Oct 30, 2023
@rebron rebron changed the title clear out loading indicator when asset discovery is not being run. Clear out loading indicator when asset discovery is not being run Nov 9, 2023
@srirambv
Copy link
Contributor

srirambv commented Jan 2, 2024

Verification passed on Google Pixel 8 with Android 14 running 1.62.123 x64 Beta build

  • Verified steps from brave/brave-core#20738
  • Verified loading state indicator is cleared when asset discovery is complete
30921.mp4

@srirambv
Copy link
Contributor

Verification passed on

Brave 1.62.130 Chromium: 120.0.6099.199 (Official Build) beta (64-bit)
Revision 6d589c6dc0f3489830d40332c82cdd5597e41c88
OS Windows 11 Version 23H2 (Build 22631.2861)
  • Verified steps from brave/brave-core#20738
  • Verified loading state indicator is cleared when asset discovery is complete
30921.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/web3/wallet Integrating Ethereum+ wallet support front-end-change This task is a front end task and doesn't need any C++ changes OS/Android Fixes related to Android browser functionality OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass - Android ARM QA Pass-Win64 QA/Yes release-notes/include
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants