Skip to content

Conversation

@ryu-man
Copy link
Contributor

@ryu-man ryu-man commented Oct 9, 2025

Addresses #4160

  • This PR helps reduce the number of times to call the API by caching data locally for 5 mn.
  • Currently it is used to cache users & groups

@ryu-man ryu-man changed the title feat: intilize a KV store solution built ont IndexedDB to cache frequent used data feat: intilize a KV store solution built on IndexedDB to cache frequent used data Oct 9, 2025
@ryu-man ryu-man changed the title feat: intilize a KV store solution built on IndexedDB to cache frequent used data feat: intialize a KV store solution built on IndexedDB to cache frequent used data Oct 9, 2025
@ryu-man ryu-man marked this pull request as ready for review October 9, 2025 18:57
@ryu-man ryu-man requested a review from ivyjeong13 October 9, 2025 18:57
* Production-ready IndexedDB Key-Value Store with expiration support
* Features: TTL, batch operations, automatic cleanup, listeners, and error handling
*/
export class KV {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but this seems like overkill for a simple cache to fetch users and groups every 5 minutes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think I can make it more simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove batch operation and listeners logic. kept only the need methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you meant going with IndexedDB was an overkill, then the reason I chose it is because it does handle large data better than localstorage, and because users and groups will grow in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess just feels like a svelte store to hold/pass around the user/groups and do a refetch every 5 mins would be enough currently? I'd assume we'd switch over to backend pagination as we scale.

I'm actually thinking wouldn't the audit logs instead be a better use case for indexedDB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess just feels like a svelte store to hold/pass around the user/groups and do a refetch every 5 mins would be enough currently?

The downside of a store is that it is not persistent. If the user reloads the page before timeout, then it will fetch data again.

I'm actually thinking wouldn't the audit logs instead be a better use case for indexedDB?

Yes, I do agree. But the case of audit logs is that data is dynamic due to filtering. So it looks to me that using the store would not be efficient; the only good reason to use it is to load data ASAP before fetching from the API, which is possibly outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds okay to me, make one call on page load then cache for 5mins?

IndexedDB still feels overkill to me over a store but not a blocker as it not's negatively impacting!

Since we are caching the fetching of user/groups, we should go ahead and utilize it everywhere else and not just the access-control page. Loom for what I mean:
https://www.loom.com/share/125858277f78402691e6178ed32e1cfe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do totally agree on using caching with other entities across the application. The current implementation addresses the issues on the access control. I will go ahead and implement an expanded solution to be used in the whole app

@cjellick
Copy link
Contributor

I'm not comfortable with this change. Let's not pursue it for now. Right now my priority will just be to make that groups call faster.

@cjellick
Copy link
Contributor

dont think we're going to do this

@cjellick cjellick closed this Nov 11, 2025
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