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

Throttle queries when viewing many accounts for selection #4926

Closed
markmhendrickson opened this issue Feb 7, 2024 · 30 comments · Fixed by #5062
Closed

Throttle queries when viewing many accounts for selection #4926

markmhendrickson opened this issue Feb 7, 2024 · 30 comments · Fixed by #5062
Assignees
Labels
area:networks enhancement enhancement-p2 Critical functionality needed by few users, with no clear alternatives

Comments

@markmhendrickson
Copy link
Collaborator

To avoid going over 100 RPMs for any given user with many (e.g. over 50) accounts to view and scroll through here:

image
@markmhendrickson markmhendrickson added enhancement-p2 Critical functionality needed by few users, with no clear alternatives area:networks enhancement labels Feb 7, 2024
@markmhendrickson
Copy link
Collaborator Author

@mica000 Could you indicate a simple solution for how to display placeholders for balances and account label values (i.e. BNS names) in the scenario in which we can't load them for over 1+ more minutes given the user has been rate limited by the API?

I imagine for the account label, we'd simply show "Account 1, 2, etc." and for the balances something like "–" instead of the value?

We can improve our loading state UX in general for this component as part of our accounts UX redesign project, so this particular issue is meant to be a short-term solution as we roll out throttling to prevent errors related to lower rate limits from Hiro.

@mica000
Copy link

mica000 commented Feb 8, 2024

@markmhendrickson my initial thought would be to have a ghost state added the to the InteractiveItem. Wdyt?
cc @fbwoolf

If so, I can add the state to the designs and we just need to add this state to the same component on Storybook.

Bonus point to being able to use the same behavior in all other places were we use the component

@kyranjamie
Copy link
Collaborator

kyranjamie commented Feb 8, 2024

@mica000 in this case, the row is still interactive, but only the money values are unknown.

I don't think we should change InteractiveItem, because then we are extending the scope of the base component, for one specific instance. Asked chatgpt for help explaining difference between a component, and an instance of a component.

A component is like the blueprint for a house—it outlines the structure, design, and functionalities the house will have. An instance of a component is like a house built from that blueprint. There can be many houses built from the same blueprint (component), but each house (instance) is a separate entity, can be customized with different colors or interior designs, and is located in a different place."

Using same analogy, we shouldn't change the blueprint for all houses, just to solve a problem specific to one house.

@markmhendrickson
Copy link
Collaborator Author

Would the ghost state apply to just the balances part of the item rather than the item in its entirety? Because we'd want the user to be able to view and select each account item even if the remaining label and balance data hasn't loaded yet.

@kyranjamie
Copy link
Collaborator

kyranjamie commented Feb 8, 2024

Can we just remove the balances entirely? They consume a huge number of requests and cause 429 issues. What other information could we display to help users identify accounts?

@fbwoolf
Copy link
Contributor

fbwoolf commented Feb 8, 2024

I was thinking here @mica000 was meaning to provide a 'loading' state for the data passed into the InteractiveItem which would render in the ItemLayout, not changing the base component itself ...I think I misunderstood her comment.

@mica000
Copy link

mica000 commented Feb 8, 2024

@mica000 in this case, the row is still interactive, but only the money values are unknown.

I don't think we should change InteractiveItem, because then we are extending the scope of the base component, for one specific instance. Asked chatgpt for help explaining difference between a component, and an instance of a component.

A component is like the blueprint for a house—it outlines the structure, design, and functionalities the house will have. An instance of a component is like a house built from that blueprint. There can be many houses built from the same blueprint (component), but each house (instance) is a separate entity, can be customized with different colors or interior designs, and is located in a different place."

Using same analogy, we shouldn't change the blueprint for all houses, just to solve a problem specific to one house.

@kyranjamie What I meant was the opposite. The ghost state would be a state in any InteractiveItem before data is fetched. In doing so we would be able to use in all the other places, like Approval, Activity, Home etc.. Maybe a better term is LAZY LOADING?

image Screenshot 2024-02-08 at 20 56 46

@mica000
Copy link

mica000 commented Feb 8, 2024

Can we just remove the balances entirely? They consume a huge number of requests and cause 429 issues. What other information could we display to help users identify accounts?

cc @314159265359879

@mica000
Copy link

mica000 commented Feb 8, 2024

I was thinking here @mica000 was meaning to provide a 'loading' state for the data passed into the InteractiveItem which would render in the ItemLayout, not changing the base component itself ...I think I misunderstood her comment.

I wasn't going to this extend in how we would build in the FE. The proposal is mostly on how the component would work visually but that could be a good direction!

@fbwoolf
Copy link
Contributor

fbwoolf commented Feb 8, 2024

@mica000 I think the confusion might be when you ref the state of InteractiveItem as being something specific to the data laid out as the content in it? The InteractiveItem (the blueprint) is just a rectangular button that has the click behavior and handles these states, that is it. It wouldn't know if there is data or not, or care.

Screenshot 2024-02-08 at 3 43 55 PM

You can see here in the AccountListItem (the instance) where it is used, that we currently use an isLoading conditional to pass a loading spinner in for the balance as the content to the titleRight of the ItemLayout component, which lays out the content inside of ItemInteractive.

Screenshot 2024-02-08 at 3 53 17 PM

This is ItemLayout:
Screenshot 2024-02-08 at 3 45 26 PM

I think if we want to share a Skeleton component which uses ItemLayout, that would be it's own component not a new state of ItemInteractive, but imo we are just passing in a loading state to the ItemLayout to render when data isn't available yet.

@markmhendrickson
Copy link
Collaborator Author

Can we just remove the balances entirely?

I'd prefer to avoid removing entirely unless we can't come up with a better solution here since the information is quite important for distinguishing and identifying accounts.

@mica000
Copy link

mica000 commented Feb 9, 2024

I'd prefer to avoid removing entirely unless we can't come up with a better solution here since the information is quite important for distinguishing and identifying accounts.

Agree. Im not aware of any other element we could use to identify accounts besides the amount, specially since users can't edit the name of their accounts.

@kyranjamie
Copy link
Collaborator

kyranjamie commented Feb 9, 2024

Very important comment from Fara. InteractiveItem doesn't ever know what's in it. This is a crucial concept of how components work. The source of most confusion between developers and designers stems from this mixed understanding. Let us know we if can help explain better what we mean by this, happy to demo examples.

Understood on balance. I know it's important just very resource intensive. Another options is to show the most recent balance? Or just load successfully but very slowly.

@pete-watters
Copy link
Contributor

@markmhendrickson just to chime in on this bug. If you test Leather with the login for Leather Development Wallet and simply login, you will start hitting 429 errors when viewing the landing page for the first time.

That wallet only has 3 accounts so I think we have some work to do in general

  • account 1 - 17 inscriptions - 3 assets - 1 BNS
  • account 2 - 2 assets - 1 BNS
  • account 3 - 2 assets

That's hardly a lot of data but it will still hit the 100 RPM limit on initial page load only

@markmhendrickson
Copy link
Collaborator Author

you will start hitting 429 errors when viewing the landing page for the first time.

How many of these queries are generated by the landing page itself for each of those assets to display in particular?

@314159265359879
Copy link
Contributor

Can we just remove the balances entirely? They consume a huge number of requests and cause 429 issues. What other information could we display to help users identify accounts?

@mica000 I think users use four factors that help users find the right address when they switch account.

  1. BNS name, Balance, Address and Account number (= orientation/order compared to other accounts).
  2. The more we show the easier it is for users to choose the address they intended, especially when they have many.

I see the benefit of making this page less heavy on API calls. If we remove fetching balance for every address automatically can we add a button to force it for a specific address instead? Something like a "load balance" button in case a user needs more info to find their address they can use it selectively? but it would have the benefit of limiting the number of calls greatly.

I don't think we should stop showing BNS names, they are the most useful (most used) to identify accounts. They also change less often (compared to balance) so perhaps they can be shown from memory/persisted and only be reloaded when the user forces a "reload" button?

@pete-watters
Copy link
Contributor

you will start hitting 429 errors when viewing the landing page for the first time.

How many of these queries are generated by the landing page itself for each of those assets to display in particular?

I did a quick investigation of this but it's hard to tell without diving in. For the leather dev wallet we seem to make a lot more queries than my standard wallet. I can share a video of it internally in case it helps. We seem to call balances quite often and for more accounts than we have.

I can help investigate this once I clear my plate as it's important to do what we can client side if we can't avoid the api limits.

An idea I have is for Leather to use a graphQl proxy as an interface to the hiro API but that would require a lot of work and setup of our own graphQl server etc. and even then I'm not sure its a good idea.

We can first assess the requests we are making and how often we do it and try and be more efficient

@mica000
Copy link

mica000 commented Feb 13, 2024

Proposed designs
image

https://www.figma.com/file/2MLHeIeL6XPVi3Tc2DfFCr/%E2%9D%96-Leather-%E2%80%93-Design-System?type=design&node-id=8912-67205&mode=dev

⚠️ To keep in mind that:
We store "loading" and "refreshing" inside the states of ItemIteractive ONLY in figma. On the Front End this state is a independent component which property is applied to the Label

In relation to the gradient animation:
Name of the component: Refreshing data (?);
Where it takes place: Any existing piece of data that;

Question:

  • Does anyone know a library we could use that animates the gradient over the text?
  • Will it get stored on Storybook?
  • Dark/lightmode: for the color to work on both themes, do we need to create a gradient use case?

@fbwoolf
Copy link
Contributor

fbwoolf commented Feb 13, 2024

I think we should only keep the 'interactive' states of the item in this component in Figma. We prob don't want to leave data states in the Figma component.

@mica000
Copy link

mica000 commented Feb 13, 2024

I think we should only keep the 'interactive' states of the item in this component in Figma. We prob don't want to leave data states in the Figma component.

If we were to remove the data, it would break ~80% of ApprovalUx so I Would rather not because this is super important for us when designing. This is because we cant rely on FlagLayout or ItemLayout in Figma hence we will need to have a shared team understanding that this component doesn't map 1:1 to Storybook.

Maybe it's smt we can fix with a different naming, by giving a different name in Figma that is different than IntemInteractive? How do we call components that are built using the FlagLayout?

@fbwoolf
Copy link
Contributor

fbwoolf commented Feb 13, 2024

We can have a call to talk thru the difference in how you are using them in Figma, but we should have design system primitive components that we build both designs and the UI from, like @kyranjamie pointed out, blueprints vs instances. Like here, as a ref, the Pressable component doesn't care how it is used: https://docs.nativebase.io/pressable ...there only exists an example of how it COULD be used.

You could create instances of the ItemInteractive to use specifically in the Figma designs and call them what is best for that instance, like AccountListItem which would use the primitive component ItemInteractive. You could use ItemLayout like we do in code to layout the content in that instance. In doing so, you would have to handle the data states of the content for that specific use case ...which then might be able to be reused itself in other places.

So, to answer your question, when we use the Flag layout component it is named specific to the use case, like AccountListItem. If you need a reusable component in the designs that is built from our design system components, maybe you could create something like ItemInteractiveWithFlagContent which is reused separately in your designs?

@mica000
Copy link

mica000 commented Feb 13, 2024

We can have a call to talk thru the difference in how you are using them in Figma, but we should have design system primitive components that we build both designs and the UI from, like @kyranjamie pointed out, blueprints vs instances. Like here, as a ref, the Pressable component doesn't care how it is used: https://docs.nativebase.io/pressable ...there only exists an example of how it COULD be used.

You could create instances of the ItemInteractive to use specifically in the Figma designs and call them what is best for that instance, like AccountListItem which would use the primitive component ItemInteractive. You could use ItemLayout like we do in code to layout the content in that instance. In doing so, you would have to handle the data states of the content for that specific use case ...which then might be able to be reused itself in other places.

So, to answer your question, when we use the Flag layout component it is named specific to the use case, like AccountListItem. If you need a reusable component in the designs that is built from our design system components, maybe you could create something like ItemInteractiveWithFlagContent which is reused separately in your designs?

Yep, let's schedule a call tomorrow so I can show how we are handling in Figma.

@kyranjamie
Copy link
Collaborator

We keep going back and forward on this, would love to hash it out on a call today 🙏🏼. Writing these notes as a reference for when we speak.

let's schedule a call tomorrow so I can show how we are handling in Figma

Our design system should be tool agnostic. As developers, we don't mind whether you use Figma, Photoshop, Sketch, MS paint. For us, it shouldn't be important how the tool works, so I don't think it's helpful to go into much detail on it (plus I think developers understand how it works already).

What's important is that the design system doesn't expose implementation details of the tool in which it was built. That's where a lot of the confusion stems from. E.g. originally ItemInteractive was presented as one component with content and styles mixed together because ....that's how it's built in Figma.

Figma components can be created in whatever way works for the designers. How the components are built in Figma, and how they're displayed in Figma are two different things.

...we're getting there. @mica000 it's a super helpful change you've made in ItemInteractive, making it absolutely clear the content doesn't belong inside the component. Breaking it down to just the core styles that can be reused anywhere.

image

Content is just the example of a component being used, an instance of a component. Presenting these concepts separately would improve our design system I think:

  1. Component blueprint with no content
  2. Examples of a component instance with content

@mica000
Copy link

mica000 commented Feb 14, 2024

InteractiveItem

Cool! Updated the naming to reflect the suggestions above. This is how it looks in Figma now:
image

https://www.figma.com/file/2MLHeIeL6XPVi3Tc2DfFCr/%E2%9D%96-Leather-%E2%80%93-Design-System?type=design&node-id=8912-67205&mode=dev


"Refreshing data" and (maybe) "Ghost" states - How would we like to proceed?

Here is the proposal:

State 1 - Initial data:
We could show a ghost skeleton with a pulsating animation until the initial data loads;
(This might not be necessary but curious to understand wether is needed or not);

State 2 - Refreshing data:
The idea would be to have a gradient animation pulsating over the existing balance;

image

Few questions:

  • How do we call them?
  • Will both "states" exist for any piece of data?
  • Will it get added in Storybook?
  • Are there existing libraries recommended to use that animates the gradient over the text?
  • Dark/lightmode: for the color to work on both themes, do we need to create a gradient use case or this can be solved using the animation?

@kyranjamie
Copy link
Collaborator

This organisation will help developers a lot Michelly, thanks.

  • RefetchingTextAnimation might work as a name?
  • Not exactly sure how we'll build the loading animation. Something along the lines of https://codepen.io/propagandjw/pen/WNVPRK
  • Demos and examples of how this is used in a generic way would be a great addition to storybook if we reuse this pattern elsewhere
  • Imagine we'll have to use a different underlaying background animation for the text mask in lightmode vs dark mode

@mica000
Copy link

mica000 commented Feb 15, 2024

I'd say in order of

  1. Making the generic loading components separately
  2. Applying these to the account selection list

Coolio. Can we can move to dev backlog then?

@kyranjamie
Copy link
Collaborator

Also to look into query cancellation https://tanstack.com/query/v3/docs/framework/react/guides/query-cancellation

@alter-eggo
Copy link
Contributor

kyranjamie pushed a commit that referenced this issue Mar 26, 2024
## [6.32.0](v6.31.0...v6.32.0) (2024-03-26)

### Features

* add blockstream and hiro api rate limiters, closes [#4926](#4926) ([b1b2ec5](b1b2ec5))

### Bug Fixes

* add new test for sign psbt ([d5d7cb7](d5d7cb7))
* add test for psbt wrong index failure ([7ec9744](7ec9744))
* client side nested button error ([c800721](c800721))
* increase swap hardcoded fee, closes [#4984](#4984) ([1eb268e](1eb268e))
* index bug and modify tests ([f9efae6](f9efae6))
* input payment type, closes [#5076](#5076) ([5f558da](5f558da))
* missing regtest prefix in address check ([7e8549f](7e8549f))
* modify input payment type index for ledger also ([17f57f3](17f57f3))
* swap arrow icon ([9dcd980](9dcd980))
* swap duplicate toast, closes [#5068](#5068) ([5f08a9c](5f08a9c))
* swapping welsh with alex sdk, closes [#5022](#5022) ([1f1216c](1f1216c))
* validation regtest addresses ([462ab8c](462ab8c))

### Internal

* add chromatic visual tests ([3ed207f](3ed207f))
* fix audit vulnerability ([f577570](f577570))
* post-release merge back ([7ad58b1](7ad58b1))
* update link to fee info ([0d0dc34](0d0dc34))
* update link to nonce info ([c79ddda](c79ddda))
* upgrade signer packages ([9ddea5f](9ddea5f))
* upgrade storybook ([fb0ed2a](fb0ed2a))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:networks enhancement enhancement-p2 Critical functionality needed by few users, with no clear alternatives
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants