Skip to content

feat(bootstrap): Cache projects in IndexedDB #89139

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

Merged
merged 7 commits into from
Apr 15, 2025
Merged

Conversation

scttcper
Copy link
Member

@scttcper scttcper commented Apr 9, 2025

Implements the caching part of https://www.notion.so/sentry/Tech-Spec-Frontend-Bootstrap-Cache-Project-Loading-Performance-1748b10e4b5d80759127c15d3772e063

Went with IndexedDB because some organizations have multiple megabytes of projects to cache.

This helps bandaid the entire app waiting for projects to load by storing them in the cache and revalidating.

Implements the caching part of https://www.notion.so/sentry/Tech-Spec-Frontend-Bootstrap-Cache-Project-Loading-Performance-1748b10e4b5d80759127c15d3772e063

Went with IndexedDB because some organizations have multiple megabytes of projects to cache.

This helps bandaid the entire app waiting for projects to load by storing them in the cache and revalidating.
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 9, 2025
Copy link

codecov bot commented Apr 9, 2025

❌ 919 Tests Failed:

Tests completed Failed Passed Skipped
7810 919 6891 5
View the top 3 failed test(s) by shortest run time
useVirtualizedTree shows 5-15 items
Stack Traces | 0.005s run time
ReferenceError: indexedDB is not defined
    at createStore (.../idb-keyval/dist/index.cjs:14:21)
    at defaultGetStore (.../idb-keyval/dist/index.cjs:22:31)
    at Object.del [as removeItem] (.../idb-keyval/dist/index.cjs:100:33)
    at Object.removeClient (.../sentry/sentry/node_modules/@.../query-async-storage-persister/src/index.ts:93:35)
    at persistQueryClientRestore (.../sentry/sentry/node_modules/@.../query-persist-client-core/src/persist.ts:102:15)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
PageFilters ActionCreators initializeUrlState does not update local storage (persist) when `shouldPersist` is false
Stack Traces | 0.006s run time
ReferenceError: indexedDB is not defined
    at createStore (.../idb-keyval/dist/index.cjs:14:21)
    at defaultGetStore (.../idb-keyval/dist/index.cjs:22:31)
    at Object.del [as removeItem] (.../idb-keyval/dist/index.cjs:100:33)
    at Object.removeClient (.../sentry/sentry/node_modules/@.../query-async-storage-persister/src/index.ts:93:35)
    at persistQueryClientRestore (.../sentry/sentry/node_modules/@.../query-persist-client-core/src/persist.ts:102:15)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
TeamStore updating teams removes teams
Stack Traces | 0.006s run time
ReferenceError: indexedDB is not defined
    at createStore (.../idb-keyval/dist/index.cjs:14:21)
    at defaultGetStore (.../idb-keyval/dist/index.cjs:22:31)
    at del (.../idb-keyval/dist/index.cjs:100:33)
    at clearQueryCache (.../static/app/appQueryClient.tsx:74:30)
    at Store.onDeleteTeam (.../app/stores/projectsStore.tsx:128:41)
    at Store.onRemoveSuccess (.../app/stores/teamStore.tsx:100:28)
    at Object.<anonymous> (.../app/stores/teamStore.spec.tsx:82:26)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

}

export function restoreQueryCache() {
if (!isProjectsCacheEnabled) {
Copy link
Member Author

Choose a reason for hiding this comment

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

revert before merging

@scttcper scttcper marked this pull request as ready for review April 14, 2025 20:10
@scttcper scttcper requested a review from a team as a code owner April 14, 2025 20:10
import {DEFAULT_QUERY_CLIENT_CONFIG, QueryClient} from 'sentry/utils/queryClient';

/**
* Named it appQueryClient because we already have a queryClient in sentry/utils/queryClient
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this comment explain the difference to try and help avoid people using this one if they don't need it?

return (
// Query is not pending or failed
query.state.status === 'success' &&
// Query is not stale
Copy link
Member

Choose a reason for hiding this comment

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

can drop this comment I think, since it literally reads not query is stale

@scttcper scttcper merged commit 368eedf into master Apr 15, 2025
41 checks passed
@scttcper scttcper deleted the scttcper/cache-projects-2 branch April 15, 2025 17:29
// Query is not pending or failed
query.state.status === 'success' &&
!query.isStale() &&
Array.isArray(query.queryKey) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

this check is unnecessary - the queryKey is always an Array.

Comment on lines +72 to +73
// Mark queries as stale so they won't be recached
appQueryClient.invalidateQueries({queryKey: ['bootstrap-projects']});
Copy link
Contributor

Choose a reason for hiding this comment

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

this not only marks them as stale, but also refetches them if they are still active!

to avoid this, we need:

appQueryClient.invalidateQueries({
  queryKey: ['bootstrap-projects'],
  refetchType: 'none',
});

@@ -30,7 +25,7 @@ function Main() {
const [router] = useState(buildRouter);

return (
<QueryClientProvider client={queryClient}>
<QueryClientProvider client={appQueryClient}>
Copy link
Contributor

Choose a reason for hiding this comment

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

we really want to use the PersistQueryClientProvider here if we use persistence. It will handle restoring and subscriptions to the storage, and avoids race conditions between queries that mount while we are still restoring from indexDB.

scttcper added a commit that referenced this pull request Apr 16, 2025
In #89139 @TkDodo recommended we use the PersistQueryClientProvider instead of the isRestoring states it provides. Adds a function that checks if the feature is enabled.
Copy link

sentry-io bot commented Apr 16, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: The data does not contain any plottable values. /insights/backend/http/domains/ View Issue
  • ‼️ Error: The data does not contain any plottable values. /insights/backend/http/domains/ View Issue
  • ‼️ Error: useOrganization called but organization is not set. /issues/:groupId/ View Issue
  • ‼️ Error: useOrganization called but organization is not set. /issues/:groupId/ View Issue

Did you find this useful? React with a 👍 or 👎

andrewshie-sentry pushed a commit that referenced this pull request Apr 22, 2025
Implements the caching part of
https://www.notion.so/sentry/Tech-Spec-Frontend-Bootstrap-Cache-Project-Loading-Performance-1748b10e4b5d80759127c15d3772e063

Went with IndexedDB because some organizations have multiple megabytes
of projects to cache.

This helps bandaid the entire app waiting for projects to load by
storing them in the cache and revalidating.
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants