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

fix(entities-*): persist fetcher cache key within session #1820

Merged
merged 4 commits into from
Dec 4, 2024

Conversation

Justineo
Copy link
Contributor

@Justineo Justineo commented Nov 27, 2024

Summary

KM-747

Before:

cache-key.mp4

After:

persistent-cache-key.mp4

Issue:

This PR addresses an issue where a flash of outdated list data appears when navigating away from and back to a list page after changes are made to the entity list. This happens because the fetcher-cache-key is reset every time the list page reloads.

To fix this, the PR introduces a global map to track fetcher cache keys by cacheIdentifier within the same session. This ensures the fetcher cache remains consistent, reducing flickers of stale data while still allowing revalidation to update the list seamlessly.

I also added a default row-key to <EntityBaseTable> to minimize subtle flickering during revalidation.

Before:

no-key.mp4

After:

key-id.mp4

@Justineo Justineo requested review from a team as code owners November 27, 2024 09:09
Comment on lines 275 to 276
const preferencesStorageKey = 'kong-ui-entities-ca-certificates-list'
const cacheId = computed(() => props.cacheIdentifier || preferencesStorageKey)
Copy link
Member

@adamdehaven adamdehaven Nov 29, 2024

Choose a reason for hiding this comment

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

issue: I'm not sure the fallback here makes sense. I understand it will work; however, there's no documentation or expectation that the table's cache identifier will fallback to the table preferences storage key.

If anyone upstream was utilizing one or the other to save data in localStorage or the session, etc., this would be unexpected and could cause conflicting values to override each other in the upstream implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the cacheId isn't used for local storage, it's used for as SWRV cache key so there shouldn't be any clashes unless the preferences storage key was used as a swrv cache key somewhere.

Either way, we can mitigate this risk by adding some static string like:

const cacheId = computed(() => `${props.cacheIdentifier || preferencesStorageKey}-cache-identifier`)

Copy link
Member

Choose a reason for hiding this comment

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

I believe the cacheId isn't used for local storage

It's not currently that we know of, but it could be

Copy link
Contributor Author

@Justineo Justineo Dec 2, 2024

Choose a reason for hiding this comment

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

I added this because in <EntityBaseTable>, we have the following logic:

const cacheId = computed((): string => {
// Utilize the cacheIdentifier if provided; otherwise, fallback to the preferencesStorageKey that should always be defined
return props.cacheIdentifier || props.preferencesStorageKey
})

I thought we should apply similar logic to ensure a consistent fetcher cache key. However, if this isn't necessary, I believe we can simply remove the fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To add further context: In <EntityBaseTable>, the props.cacheIdentifier || props.preferencesStorageKey is already used as both the cacheIdentifier for <KTableData> and the key for table preferences in localStorage. Even if users do not provide a cacheIdentifier for the entity lists, the fetcher results are stored using fallback keys derived from preferencesStorageKey. Therefore, the intention here is to generate keys for persistent fetcherCacheKey containers here using the same logic.

Before this change: Even when cacheIdentifier is not provided, caching still occurs, and its behavior is essentially consistent with when it is provided.
After: If cacheIdentifier is not passed, the fetcherCacheKey cannot be persisted, leading to inconsistent behavior compared to when it is provided. That said, this inconsistency only reflects the behavior prior to optimization, so the impact may not be significant. And indeed, I also think the current approach reveals a bit of abstraction leakage for <EntityBaseTable>. So I think I'd just remove the fallback.

@adamdehaven @kaiarrowood

@Justineo Justineo enabled auto-merge (squash) December 3, 2024 01:58
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

I didn't functionally test, but other than the comments here, the rest lgtm.

It would be nice if we can get someone more familiar with the area to test

Copy link
Member

@Leopoldthecoder Leopoldthecoder left a comment

Choose a reason for hiding this comment

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

@Justineo Justineo merged commit d2dff5c into main Dec 4, 2024
23 checks passed
@Justineo Justineo deleted the fix/entity-list-cache-key branch December 4, 2024 06:16
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.

4 participants