decentdao / decent-interface

Govern at startup speed
https://app.decentdao.org
MIT License
14 stars 7 forks source link

`[Issue #2529]` Organization Hierarchy Cache #2566

Open Da-Colon opened 3 days ago

Da-Colon commented 3 days ago

Closes #2529

GIF

screencast 2024-11-22 09-20-44

netlify[bot] commented 3 days ago

Deploy Preview for decent-interface-dev ready!

Name Link
Latest commit 55a4cd4976e87554d885dad37cb141351e84df3c
Latest deploy log https://app.netlify.com/sites/decent-interface-dev/deploys/6741f34aedef1300086a0584
Deploy Preview https://deploy-preview-2566.app.dev.decentdao.org
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Da-Colon commented 3 days ago

This implementation uses a cache expiry of 1 week. So at least every 7 days this cache will expire and be re-fetched fresh again.

I'm not sure if that experience is really what we're after here.

But the problem with making the cache expiration longer, is that DAO names can change! So, if a DAO name changes, we'd be displaying the old name until the cache expires.

I dealt with this recently in my "cache favorites" PR. My solution was to:

  • set the cache to never expire
  • load the data from the cache immediately
  • in the background do a fetch and if there have been changes, update the cache

Would something like that be appropriate for this PR?

As an aside, we are storing a lot of data for each "Node"; the whole DaoInfo object. All we really need are the address and name, right? Wonder if this PR is the place to refactor our Organization page's data requirements.

We can definitely discuss, but this is that change. The view layer component has been updated with only what it needs.

About DAO Info that is the bare min. it does two calls per safe right now

SafeAPI -> We will need to modules, I could take this out but we will need it back in later anyway Subgraph -> All the other stuff, DAOName and Heirarchy.

And thats currently it that is what makes up DAOInfo

Da-Colon commented 3 days ago

I could break out the Safe Type from DAOInfo, right now I only need address and the in the future we will be using the modules to determine type (so still don't really need to save the all whole safe thing right now)

Da-Colon commented 3 days ago

I don't know why I argue you are right, second I took at look at the type I mean duh. one second

Da-Colon commented 3 days ago

@adamgall Still need to make the same calls. But created its own type

Da-Colon commented 3 days ago

Before this is merged. I am pausing with the thought. So we don't have to migrate. might need to go ahead and add in the work for the type of governance? so that can get cached as well?

adamgall commented 3 days ago

Can bump the CACHE_VERSION for this new local storage store if we ever need to invalidate its cache.

edit: speaking of which, should probably set a CACHE_VERSION for this so we can invalidate it in the future if/when we need to add more stuff to it, like you're hypothesizing in your previous comment @Da-Colon !

edit2: just tested, if we don't have a CACHE_VERSION for this new CacheKey, it's all good it just starts at version undefined. In the future we can add a CACHE_VERSION and set it to 1 and that will cause old local storage keys to be invalidated.

edit3: but i mean, it might be good just to explicitly set the version to 1 now in this PR, just for clarity.

Da-Colon commented 2 days ago

Can bump the CACHE_VERSION for this new local storage store if we ever need to invalidate its cache.

edit: speaking of which, should probably set a CACHE_VERSION for this so we can invalidate it in the future if/when we need to add more stuff to it, like you're hypothesizing in your previous comment @Da-Colon !

edit2: just tested, if we don't have a CACHE_VERSION for this new CacheKey, it's all good it just starts at version undefined. In the future we can add a CACHE_VERSION and set it to 1 and that will cause old local storage keys to be invalidated.

edit3: but i mean, it might be good just to explicitly set the version to 1 now in this PR, just for clarity.

100% agreed to set at 1

Da-Colon commented 2 days ago

So as you said the expiry is defaults to ONE_WEEK. I'm not really sure what to set it.

This got me thinking. There are places were caching got us in trouble but saves alot. We could think about a sync IconButton that will allow a user to manually refresh cached data? (not for this PR lol). For when things look a little off. got refresh a section of data or something.

In this case. we are caching the childDAOs. if that changes then this quickly will no longer be displaying correctly..