Repository: getsentry/sentry
Pull request: perf(issue-details): cache group details response
this pr adds in caching for the group details response on the issue details page. there are a couple changes being made to make this happen. first, the response will be cached (similar to how we are caching event details). to make this work, right now we are syncing the useApiQuery cache and GroupStore so they are the same. we also do not want to call fetchData each time we load a new event so that functionality has been moved to other parts of the file. 

Closes https://github.com/getsentry/sentry/issues/48462 and https://github.com/getsentry/sentry/issues/47519

Problem statement:
Cache group release endpoint
The way we are calling the `/issues/<group_id>/first-last-release` endpoint can be improved in a few ways:

1. Only fetch once when loading the issue. Right now the value is being refetched when scrolling through events, which is not necessary.
2. Cache the response
3. Fetch in parallel with the group and event details endpoints. Right now it waits for those to complete which is not necessary.

Hints from issue discussion:

Commit message: fix issue with project query

Patch to review:
diff --git a/static/app/views/issueDetails/groupDetails.tsx b/static/app/views/issueDetails/groupDetails.tsx
index 8b65f67eb5ae06..4a4ebba1ad748e 100644
--- a/static/app/views/issueDetails/groupDetails.tsx
+++ b/static/app/views/issueDetails/groupDetails.tsx
@@ -9,6 +9,7 @@ import {
 import {browserHistory, RouteComponentProps} from 'react-router';
 import styled from '@emotion/styled';
 import * as Sentry from '@sentry/react';
+import * as qs from 'query-string';
 
 import {fetchOrganizationEnvironments} from 'sentry/actionCreators/environments';
 import LoadingError from 'sentry/components/loadingError';
@@ -21,7 +22,7 @@ import {t} from 'sentry/locale';
 import GroupStore from 'sentry/stores/groupStore';
 import {useLegacyStore} from 'sentry/stores/useLegacyStore';
 import {space} from 'sentry/styles/space';
-import {Group, IssueCategory, Organization, Project} from 'sentry/types';
+import {Group, GroupRelease, IssueCategory, Organization, Project} from 'sentry/types';
 import {Event} from 'sentry/types/event';
 import {defined} from 'sentry/utils';
 import {trackAnalytics} from 'sentry/utils/analytics';
@@ -33,7 +34,12 @@ import {
   getTitle,
 } from 'sentry/utils/events';
 import {getAnalyicsDataForProject} from 'sentry/utils/projects';
-import {useApiQuery} from 'sentry/utils/queryClient';
+import {
+  ApiQueryKey,
+  setApiQueryData,
+  useApiQuery,
+  useQueryClient,
+} from 'sentry/utils/queryClient';
 import recreateRoute from 'sentry/utils/recreateRoute';
 import RequestError from 'sentry/utils/requestError/requestError';
 import useRouteAnalyticsEventNames from 'sentry/utils/routeAnalytics/useRouteAnalyticsEventNames';
@@ -41,7 +47,6 @@ import useRouteAnalyticsParams from 'sentry/utils/routeAnalytics/useRouteAnalyti
 import useApi from 'sentry/utils/useApi';
 import {useLocation} from 'sentry/utils/useLocation';
 import useOrganization from 'sentry/utils/useOrganization';
-import usePrevious from 'sentry/utils/usePrevious';
 import useProjects from 'sentry/utils/useProjects';
 import useRouter from 'sentry/utils/useRouter';
 import {normalizeUrl} from 'sentry/utils/withDomainRequired';
@@ -75,7 +80,6 @@ type FetchGroupDetailsState = {
   errorType: Error;
   event: Event | null;
   eventError: boolean;
-  fetchData: () => Promise<void>;
   group: Group | null;
   loadingEvent: boolean;
   loadingGroup: boolean;
@@ -240,13 +244,11 @@ function useRefetchGroupForReprocessing({
   }, [hasReprocessingV2Feature, refetchGroup]);
 }
 
-function useFetchOnMount({fetchData}: Pick<FetchGroupDetailsState, 'fetchData'>) {
+function useFetchOnMount() {
   const api = useApi();
   const organization = useOrganization();
 
   useEffect(() => {
-    fetchData();
-
     // Fetch environments early - used in GroupEventDetailsContainer
     fetchOrganizationEnvironments(api, organization.slug);
     // eslint-disable-next-line react-hooks/exhaustive-deps
@@ -273,6 +275,37 @@ function useEventApiQuery(
   return isLatest ? latestEventQuery : otherEventQuery;
 }
 
+type FetchGroupQueryParameters = {
+  environments: string[];
+  groupId: string;
+};
+
+function makeFetchGroupQueryKey({
+  groupId,
+  environments,
+}: FetchGroupQueryParameters): ApiQueryKey {
+  return [`/issues/${groupId}/`, {query: getGroupQuery({environments})}];
+}
+
+function useSyncGroupStore(environments: string[]) {
+  const queryClient = useQueryClient();
+  const router = useRouter();
+  const allGroups = useLegacyStore(GroupStore);
+
+  const params = router.params;
+  const group = allGroups.find(({id}) => id === params.groupId) as Group;
+
+  useEffect(() => {
+    if (defined(group)) {
+      setApiQueryData(
+        queryClient,
+        makeFetchGroupQueryKey({groupId: group.id, environments}),
+        group
+      );
+    }
+  }, [group, queryClient, environments]);
+}
+
 function useFetchGroupDetails({
   isGlobalSelectionReady,
   environments,
@@ -284,13 +317,9 @@ function useFetchGroupDetails({
   const organization = useOrganization();
   const router = useRouter();
   const params = router.params;
-  const location = useLocation();
   const {projects} = useProjects();
-  const allGroups = useLegacyStore(GroupStore);
-  const group = allGroups.find(({id}) => id === params.groupId) as Group;
 
   const [project, setProject] = useState<Project | null>(null);
-  const [loadingGroup, setLoadingGroup] = useState<boolean>(false);
   const [error, setError] = useState<boolean>(false);
   const [errorType, setErrorType] = useState<Error | null>(null);
   const [event, setEvent] = useState<Event | null>(null);
@@ -312,41 +341,51 @@ function useFetchGroupDetails({
     refetch: refetchEvent,
   } = useEventApiQuery(eventId, [eventUrl, {query: eventQuery}]);
 
+  const {
+    data: groupData,
+    isLoading: loadingGroup,
+    isError: isGroupError,
+    error: groupError,
+    refetch: refetchGroupCall,
+  } = useApiQuery<Group>(makeFetchGroupQueryKey({groupId, environments}), {
+    staleTime: 30000,
+    cacheTime: 30000,
+    enabled: isGlobalSelectionReady,
+  });
+
+  const {data: groupReleaseData, isLoading: groupReleaseLoading} =
+    useApiQuery<GroupRelease>([`/issues/${groupId}/first-last-release/`], {
+      staleTime: 60000,
+      cacheTime: 60000,
+      enabled: !!groupData,
+    });
+
   useEffect(() => {
-    if (eventData) {
-      setEvent(eventData);
+    if (!groupReleaseLoading && groupReleaseData) {
+      GroupStore.onPopulateReleases(groupId, groupReleaseData);
     }
-  }, [eventData]);
+  }, [groupReleaseData, groupReleaseLoading, groupId]);
 
-  const fetchGroupReleases = useCallback(async () => {
-    const releases = await api.requestPromise(
-      `/issues/${params.groupId}/first-last-release/`
-    );
-    GroupStore.onPopulateReleases(params.groupId, releases);
-  }, [api, params.groupId]);
+  const group = groupData ?? null;
 
-  const handleError = useCallback((e: RequestError) => {
-    Sentry.captureException(e);
-
-    setLoadingGroup(false);
-    setErrorType(getFetchDataRequestErrorType(e?.status));
-    setError(true);
-  }, []);
+  useSyncGroupStore(environments);
 
-  const fetchData = useCallback(async () => {
-    // Need to wait for global selection store to be ready before making request
-    if (!isGlobalSelectionReady) {
-      return;
+  useEffect(() => {
+    if (eventData) {
+      setEvent(eventData);
     }
+  }, [eventData]);
 
-    try {
-      const groupResponse = await api.requestPromise(`/issues/${params.groupId}/`, {
-        query: getGroupQuery({environments}),
-      });
-      fetchGroupReleases();
+  useEffect(() => {
+    if (!loadingGroup && group) {
+      GroupStore.loadInitialData([group]);
+    }
+  }, [group, loadingGroup]);
 
+  useEffect(() => {
+    if (group && event) {
       const reprocessingNewRoute = getReprocessingNewRoute({
-        group: groupResponse,
+        group,
         event,
         router,
         organization,
@@ -356,21 +395,26 @@ function useFetchGroupDetails({
         browserHistory.push(reprocessingNewRoute);
         return;
       }
+    }
+  }, [group, event, router, organization]);
 
-      const matchingProject = projects?.find(p => p.id === groupResponse.project.id);
+  useEffect(() => {
+    if (group?.project) {
+      const matchingProject = projects?.find(p => p.id === group.project.id);
 
       if (!matchingProject) {
         Sentry.withScope(scope => {
           const projectIds = projects.map(item => item.id);
           scope.setContext('missingProject', {
-            projectId: groupResponse.project.id,
+            projectId: group.project.id,
             availableProjects: projectIds,
           });
           Sentry.captureException(new Error('Project not found'));
         });
       } else {
         markEventSeen(api, organization.slug, matchingProject.slug, params.groupId);
-        const locationQuery = {...location.query};
+        const locationQuery = qs.parse(window.location.search) || {};
+
         if (locationQuery.project === undefined && locationQuery._allp === undefined) {
           // We use _allp as a temporary measure to know they came from the
           // issue list page with no project selected (all projects included in
@@ -392,54 +436,26 @@ function useFetchGroupDetails({
         delete locationQuery._allp;
         browserHistory.replace({...window.location, query: locationQuery});
       }
+      setProject(matchingProject || group.project);
+    }
+  }, [group?.project, loadingGroup, projects, api, organization.slug, params.groupId]);
 
-      setProject(matchingProject || groupResponse.project);
-      setLoadingGroup(false);
+  const handleError = useCallback((e: RequestError) => {
+    Sentry.captureException(e);
 
-      GroupStore.loadInitialData([groupResponse]);
-    } catch (e) {
-      handleError(e);
-    }
-  }, [
-    api,
-    environments,
-    fetchGroupReleases,
-    handleError,
-    isGlobalSelectionReady,
-    location,
-    organization,
-    params,
-    projects,
-    event,
-    router,
-  ]);
+    setErrorType(getFetchDataRequestErrorType(e?.status));
+    setError(true);
+  }, []);
 
-  // Refetch when group is stale
   useEffect(() => {
-    if (group) {
-      // TODO(ts) This needs a better approach. issueActions is splicing attributes onto
-      // group objects to cheat here.
-      if ((group as Group & {stale?: boolean}).stale) {
-        fetchData();
-        return;
-      }
+    if (isGroupError) {
+      handleError(groupError);
     }
-  }, [fetchData, group]);
+  }, [isGroupError, groupError, handleError]);
 
   useTrackView({group, event, project});
 
-  const refetchData = useCallback(() => {
-    // Set initial state
-    setLoadingGroup(true);
-    setError(false);
-    setErrorType(null);
-
-    // refetchEvent comes from useApiQuery since event and group data are separately fetched
-    refetchEvent();
-    fetchData();
-  }, [fetchData, refetchEvent]);
-
-  const refetchGroup = useCallback(async () => {
+  const refetchGroup = useCallback(() => {
     if (
       group?.status !== ReprocessingStatus.REPROCESSING ||
       loadingGroup ||
@@ -448,44 +464,30 @@ function useFetchGroupDetails({
       return;
     }
 
-    setLoadingGroup(true);
+    refetchGroupCall();
+  }, [group, loadingGroup, loadingEvent, refetchGroupCall]);
 
-    try {
-      const updatedGroup = await api.requestPromise(`/issues/${params.groupId}/`, {
-        query: getGroupQuery({environments}),
-      });
+  const refetchData = useCallback(() => {
+    // Set initial state
+    setError(false);
+    setErrorType(null);
 
-      const reprocessingNewRoute = getReprocessingNewRoute({
-        group: updatedGroup,
-        event,
-        organization,
-        router,
-      });
+    // refetchEvent comes from useApiQuery since event and group data are separately fetched
+    refetchEvent();
+    refetchGroup();
+  }, [refetchGroup, refetchEvent]);
 
-      if (reprocessingNewRoute) {
-        browserHistory.push(reprocessingNewRoute);
+  // Refetch when group is stale
+  useEffect(() => {
+    if (group) {
+      if ((group as Group & {stale?: boolean}).stale) {
+        refetchGroup();
         return;
       }
-
-      setLoadingGroup(false);
-      GroupStore.loadInitialData([updatedGroup]);
-    } catch (e) {
-      handleError(e);
     }
-  }, [
-    api,
-    environments,
-    group?.status,
-    handleError,
-    loadingEvent,
-    loadingGroup,
-    params.groupId,
-    event,
-    organization,
-    router,
-  ]);
+  }, [refetchGroup, group]);
 
-  useFetchOnMount({fetchData});
+  useFetchOnMount();
   useRefetchGroupForReprocessing({refetchGroup});
 
   useEffect(() => {
@@ -498,7 +500,6 @@ function useFetchGroupDetails({
     project,
     loadingGroup,
     loadingEvent,
-    fetchData,
     group,
     event,
     errorType,
@@ -714,15 +715,9 @@ function GroupDetailsPageContent(props: GroupDetailsProps & FetchGroupDetailsSta
 
 function GroupDetails(props: GroupDetailsProps) {
   const organization = useOrganization();
-  const location = useLocation();
   const router = useRouter();
 
-  const {fetchData, project, group, ...fetchGroupDetailsProps} =
-    useFetchGroupDetails(props);
-
-  const previousPathname = usePrevious(location.pathname);
-  const previousEventId = usePrevious(router.params.eventId);
-  const previousIsGlobalSelectionReady = usePrevious(props.isGlobalSelectionReady);
+  const {project, group, ...fetchGroupDetailsProps} = useFetchGroupDetails(props);
 
   const {data} = useFetchIssueTagsForDetailsPage(
     {
@@ -734,24 +729,6 @@ function GroupDetails(props: GroupDetailsProps) {
   );
   const isSampleError = data?.some(tag => tag.key === 'sample_event') ?? false;
 
-  useEffect(() => {
-    const globalSelectionReadyChanged =
-      previousIsGlobalSelectionReady !== props.isGlobalSelectionReady;
-
-    if (globalSelectionReadyChanged || location.pathname !== previousPathname) {
-      fetchData();
-    }
-  }, [
-    fetchData,
-    group,
-    location.pathname,
-    previousEventId,
-    previousIsGlobalSelectionReady,
-    previousPathname,
-    props.isGlobalSelectionReady,
-    router.params.eventId,
-  ]);
-
   const getGroupDetailsTitle = () => {
     const defaultTitle = 'Sentry';
 
@@ -783,7 +760,6 @@ function GroupDetails(props: GroupDetailsProps) {
             {...{
               group,
               project,
-              fetchData,
               ...fetchGroupDetailsProps,
             }}
           />


Code review comments:
diff_hunk: @@ -284,13 +317,9 @@ function useFetchGroupDetails({
   const organization = useOrganization();
   const router = useRouter();
   const params = router.params;
-  const location = useLocation();
   const {projects} = useProjects();
-  const allGroups = useLegacyStore(GroupStore);
-  const group = allGroups.find(({id}) => id === params.groupId) as Group;
 
   const [project, setProject] = useState<Project | null>(null);
path: static/app/views/issueDetails/groupDetails.tsx
text: @user1:
Something that you can do now (here or in a followup PR) is remove this state variable and replace it with something like this:

```
const project =
    projects?.find(({id}) => id === groupData?.project?.id) ?? groupData?.project ?? null;
```

Less state is always good and this means the project will be filled in without an extra render cycle.

diff_hunk: @@ -312,41 +341,51 @@ function useFetchGroupDetails({
     refetch: refetchEvent,
   } = useEventApiQuery(eventId, [eventUrl, {query: eventQuery}]);
 
+  const {
+    data: groupData,
+    isLoading: loadingGroup,
+    isError: isGroupError,
+    error: groupError,
+    refetch: refetchGroupCall,
+  } = useApiQuery<Group>(makeFetchGroupQueryKey({groupId, environments}), {
+    staleTime: 30000,
+    cacheTime: 30000,
+    enabled: isGlobalSelectionReady,
+  });
+
+  const {data: groupReleaseData, isLoading: groupReleaseLoading} =
+    useApiQuery<GroupRelease>([`/issues/${groupId}/first-last-release/`], {
+      staleTime: 60000,
+      cacheTime: 60000,
+      enabled: !!groupData,
path: static/app/views/issueDetails/groupDetails.tsx
text: @user1:
nit: prefer using `defined` to `!!` - not a big deal, it just makes readability a bit better and avoids some gotchas with this syntax.

diff_hunk: @@ -312,41 +341,51 @@ function useFetchGroupDetails({
     refetch: refetchEvent,
   } = useEventApiQuery(eventId, [eventUrl, {query: eventQuery}]);
 
+  const {
+    data: groupData,
+    isLoading: loadingGroup,
+    isError: isGroupError,
+    error: groupError,
+    refetch: refetchGroupCall,
+  } = useApiQuery<Group>(makeFetchGroupQueryKey({groupId, environments}), {
+    staleTime: 30000,
+    cacheTime: 30000,
+    enabled: isGlobalSelectionReady,
+  });
+
+  const {data: groupReleaseData, isLoading: groupReleaseLoading} =
+    useApiQuery<GroupRelease>([`/issues/${groupId}/first-last-release/`], {
+      staleTime: 60000,
+      cacheTime: 60000,
+      enabled: !!groupData,
+    });
+
   useEffect(() => {
-    if (eventData) {
-      setEvent(eventData);
+    if (!groupReleaseLoading && groupReleaseData) {
+      GroupStore.onPopulateReleases(groupId, groupReleaseData);
     }
-  }, [eventData]);
+  }, [groupReleaseData, groupReleaseLoading, groupId]);
 
-  const fetchGroupReleases = useCallback(async () => {
-    const releases = await api.requestPromise(
-      `/issues/${params.groupId}/first-last-release/`
-    );
-    GroupStore.onPopulateReleases(params.groupId, releases);
-  }, [api, params.groupId]);
+  const group = groupData ?? null;
 
-  const handleError = useCallback((e: RequestError) => {
-    Sentry.captureException(e);
-
-    setLoadingGroup(false);
-    setErrorType(getFetchDataRequestErrorType(e?.status));
-    setError(true);
-  }, []);
+  useSyncGroupStore(environments);
 
-  const fetchData = useCallback(async () => {
-    // Need to wait for global selection store to be ready before making request
-    if (!isGlobalSelectionReady) {
-      return;
+  useEffect(() => {
+    if (eventData) {
+      setEvent(eventData);
     }
+  }, [eventData]);
 
-    try {
-      const groupResponse = await api.requestPromise(`/issues/${params.groupId}/`, {
-        query: getGroupQuery({environments}),
-      });
-      fetchGroupReleases();
+  useEffect(() => {
+    if (!loadingGroup && group) {
+      GroupStore.loadInitialData([group]);
path: static/app/views/issueDetails/groupDetails.tsx
text: @user1:
I found a bug where sometimes this line would fire twice, once before onPopulateReleases and once after. I _believe_ this occurs when the group releases response is still cached, but the group details needs to be refetched.

To ensure that `loadInitialData` never gets called after `onPopulateReleases`, I'd try putting both of them in the same useEffect.

diff_hunk: @@ -312,41 +341,51 @@ function useFetchGroupDetails({
     refetch: refetchEvent,
   } = useEventApiQuery(eventId, [eventUrl, {query: eventQuery}]);
 
+  const {
+    data: groupData,
+    isLoading: loadingGroup,
+    isError: isGroupError,
+    error: groupError,
+    refetch: refetchGroupCall,
+  } = useApiQuery<Group>(makeFetchGroupQueryKey({groupId, environments}), {
+    staleTime: 30000,
+    cacheTime: 30000,
+    enabled: isGlobalSelectionReady,
+  });
+
+  const {data: groupReleaseData, isLoading: groupReleaseLoading} =
+    useApiQuery<GroupRelease>([`/issues/${groupId}/first-last-release/`], {
+      staleTime: 60000,
path: static/app/views/issueDetails/groupDetails.tsx
text: @user1:
I'd make this the same value as the group query (30 seconds) for consistency