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

Local Caching #267

Merged
merged 23 commits into from
Feb 5, 2025
Merged

Local Caching #267

merged 23 commits into from
Feb 5, 2025

Conversation

nofurtherinformation
Copy link
Collaborator

@nofurtherinformation nofurtherinformation commented Jan 29, 2025

This PR adds clientside caching for gerrydb views and zone assignments for better user experience and lighter server loads.

Description

  • Adds a timestamp updated_at sent with update assignment patches to track versioning. This handles asynchronous changes but will not currently alert the FE to changes happening while they are active
  • Uses indexeddb via (idb package) to cache gerrydb views, map state, and map state updated at timestamps
  • The document table already has an updated_at property, so this does not change db schema
  • The FE already gets the full document row, so it can check if its latest cached timestamp matches the database without pulling the full assignments
  • The idb implementation to cache the state will store the assignments, shattered IDs and shatter mappings; the store state is very close to the in-memory representation to streamline reads and writes
  • LocalStorage would work and may be easier, but with this approach would be limited to ~66 cached plans, which for power users could be hit

Reviewers

Checklist

  • Update timestamps
  • Cache map state in idb
  • Cache map state updated timestamps in idb
  • Cache gerrydb views in idb
  • Reload map state from idb conditionally
  • Reload gerrydb views from idb
  • Test if better to fully stringify or just convert to plain JS objects
  • Test perf implications
  • User test / bug squash

Screenshots (if applicable):

@nofurtherinformation
Copy link
Collaborator Author

After some testing, storing vanilla js objects and converting to map/set structures is faster than fully stringifying

Copy link
Collaborator

@raphaellaude raphaellaude left a comment

Choose a reason for hiding this comment

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

This works so so well and feels way better for the user. One small change requested on the backend.

Other than that, I noticed that when the user reloads we load from the local cache OR the remote store. I'm wondering if there's a case for the AND approach: first load from the local store, then verify with the remote store. Since we're in the single user per map paradigm I figure this approach is fine, but wanted to discuss trade-offs. Thoughts?

Comment on lines 79 to 80
execute: bool = True,
commit: bool = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: Why these two params? Since you're passing the session from the caller, if the caller errors later all operations in the session's transaction will be rolled back. So you can safely execute this w/o worry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that's a good call!

@nofurtherinformation
Copy link
Collaborator Author

This works so so well and feels way better for the user. One small change requested on the backend.

Thank you!

Other than that, I noticed that when the user reloads we load from the local cache OR the remote store. I'm wondering if there's a case for the AND approach: first load from the local store, then verify with the remote store. Since we're in the single user per map paradigm I figure this approach is fine, but wanted to discuss trade-offs. Thoughts?

That's a good question -- we take this approach for the map views caching (local but verify). I think my two questions here are:

  • Is it worth saving the extra load / capacity on the backend?
  • If the user paints before the backend assignments respond, how do we reconcile the state?

Copy link
Collaborator

@raphaellaude raphaellaude left a comment

Choose a reason for hiding this comment

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

If the user paints before the backend assignments respond, how do we reconcile the state?

Hmm yeah true that would be difficult. You've convinced me that we shouldn't pull.

Another approach might be to do a full document push to the server using the local cache then?

Co-authored-by: Raphael Paul Laude <[email protected]>
Co-authored-by: Raphael Paul Laude <[email protected]>
@nofurtherinformation
Copy link
Collaborator Author

If the user paints before the backend assignments respond, how do we reconcile the state?

Hmm yeah true that would be difficult. You've convinced me that we shouldn't pull.

Another approach might be to do a full document push to the server using the local cache then?

I believe this is the case currently but will double check tomorrow!

raphaellaude
raphaellaude previously approved these changes Feb 5, 2025
Copy link
Collaborator

@raphaellaude raphaellaude left a comment

Choose a reason for hiding this comment

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

lgtm – we can leave the push pull discussion for another time / an issue rather than PR

@raphaellaude raphaellaude dismissed their stale review February 5, 2025 00:51

Saw a couple of sentry errors come through. Let's handle those before we merge.

Copy link
Collaborator

@raphaellaude raphaellaude left a comment

Choose a reason for hiding this comment

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

apparently the above errors were from a local build so re-approving!

@nofurtherinformation nofurtherinformation merged commit 95536ff into main Feb 5, 2025
2 checks passed
@nofurtherinformation nofurtherinformation deleted the caching branch February 5, 2025 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants