Browse Source

ref(replay): Leverage useApiQuery to fetch the replay list (#68849)

This replaces the replay-list-fetching code in `/replays` with a call to
`useApiQuery` instead of the direct call to `api.requestPromise`. This
has the advantage of returning cached responses when possible, and being
a more forward looking api. Also, I took the chance to stop depending on
EventView, because it's a big class and can be replaced with a few
memoized calls like `useLocationQuery`.

There are still some places calling the old `useReplayList`, namely the
Issue>Replay page and Txn>Replay page. These pages can be updated in a
followup, and would depend on the new version `useFetchReplayList` and
build ontop of it

Related to https://github.com/getsentry/sentry/issues/68846
Ryan Albrecht 11 months ago
parent
commit
b570036d8a

+ 67 - 0
static/app/utils/replays/hooks/useFetchReplayList.ts

@@ -0,0 +1,67 @@
+import {useMemo} from 'react';
+
+import {ALL_ACCESS_PROJECTS} from 'sentry/constants/pageFilters';
+import type {Organization} from 'sentry/types';
+import {uniq} from 'sentry/utils/array/uniq';
+import {type ApiQueryKey, useApiQuery} from 'sentry/utils/queryClient';
+import {mapResponseToReplayRecord} from 'sentry/utils/replays/replayDataUtils';
+import {
+  REPLAY_LIST_FIELDS,
+  type ReplayListQueryReferrer,
+  type ReplayListRecord,
+} from 'sentry/views/replays/types';
+
+type Options = {
+  options: ApiQueryKey[1];
+  organization: Organization;
+  queryReferrer: ReplayListQueryReferrer;
+};
+
+export default function useFetchReplayList({
+  options,
+  organization,
+  queryReferrer,
+}: Options) {
+  const fixedQueryKey = useMemo<ApiQueryKey>(() => {
+    const url = `/organizations/${organization.slug}/replays/`;
+    if (!options || !options.query) {
+      return [url];
+    }
+
+    // HACK!!! Because the sort field needs to be in the eventView, but I cannot
+    // ask the server for compound fields like `os.name`.
+    const splitFields = REPLAY_LIST_FIELDS.map(field => field.split('.')[0]);
+    const fields = uniq(splitFields);
+
+    // when queryReferrer === 'issueReplays' we override the global view check on the backend
+    // we also require a project param otherwise we won't yield results
+    const {project: originalProject} = options.query;
+    const project =
+      queryReferrer === 'issueReplays' || queryReferrer === 'transactionReplays'
+        ? ALL_ACCESS_PROJECTS
+        : originalProject;
+    return [
+      url,
+      {
+        ...options,
+        query: {
+          per_page: 50,
+          ...options.query,
+          fields,
+          project,
+          queryReferrer,
+        },
+      },
+    ];
+  }, [options, organization.slug, queryReferrer]);
+
+  const {data, ...result} = useApiQuery<{data: any[]}>(fixedQueryKey, {
+    staleTime: Infinity,
+    enabled: true,
+  });
+
+  return {
+    data: data?.data?.map<ReplayListRecord>(mapResponseToReplayRecord),
+    ...result,
+  };
+}

+ 6 - 2
static/app/utils/url/useLocationQuery.tsx

@@ -1,11 +1,15 @@
 import {useMemo} from 'react';
 
-import type {decodeList} from 'sentry/utils/queryString';
+import type {decodeList, decodeSorts} from 'sentry/utils/queryString';
 import {decodeInteger, decodeScalar} from 'sentry/utils/queryString';
 import {useLocation} from 'sentry/utils/useLocation';
 
 type Scalar = string | boolean | number | undefined;
-type Decoder = typeof decodeList | typeof decodeScalar | typeof decodeInteger;
+type Decoder =
+  | typeof decodeInteger
+  | typeof decodeList
+  | typeof decodeScalar
+  | typeof decodeSorts;
 
 /**
  * Select and memoize query params from location.

+ 11 - 34
static/app/views/replays/list/listContent.spec.tsx

@@ -5,7 +5,6 @@ import {render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
 
 import type {Organization as TOrganization} from 'sentry/types';
 import useDeadRageSelectors from 'sentry/utils/replays/hooks/useDeadRageSelectors';
-import useReplayList from 'sentry/utils/replays/hooks/useReplayList';
 import {
   useHaveSelectedProjectsSentAnyReplayEvents,
   useReplayOnboardingSidebarPanel,
@@ -14,26 +13,12 @@ import useOrganization from 'sentry/utils/useOrganization';
 import useProjectSdkNeedsUpdate from 'sentry/utils/useProjectSdkNeedsUpdate';
 import ListPage from 'sentry/views/replays/list/listContent';
 
-jest.mock('sentry/utils/replays/hooks/useReplayOnboarding');
 jest.mock('sentry/utils/replays/hooks/useDeadRageSelectors');
+jest.mock('sentry/utils/replays/hooks/useReplayOnboarding');
 jest.mock('sentry/utils/replays/hooks/useReplayPageview');
 jest.mock('sentry/utils/useOrganization');
 jest.mock('sentry/utils/useProjectSdkNeedsUpdate');
-jest.mock('sentry/utils/replays/hooks/useReplayList', () => {
-  return {
-    __esModule: true,
-    default: jest.fn(() => {
-      return {
-        fetchError: undefined,
-        isFetching: false,
-        pageLinks: null,
-        replays: [],
-      };
-    }),
-  };
-});
 
-const mockUseReplayList = jest.mocked(useReplayList);
 const mockUseDeadRageSelectors = jest.mocked(useDeadRageSelectors);
 
 const mockUseHaveSelectedProjectsSentAnyReplayEvents = jest.mocked(
@@ -63,8 +48,8 @@ function getMockContext(mockOrg: TOrganization) {
 }
 
 describe('ReplayList', () => {
+  let mockFetchReplayListRequest;
   beforeEach(() => {
-    mockUseReplayList.mockClear();
     mockUseHaveSelectedProjectsSentAnyReplayEvents.mockClear();
     mockUseProjectSdkNeedsUpdate.mockClear();
     mockUseDeadRageSelectors.mockClear();
@@ -73,6 +58,10 @@ describe('ReplayList', () => {
       url: '/organizations/org-slug/tags/',
       body: [],
     });
+    mockFetchReplayListRequest = MockApiClient.addMockResponse({
+      url: `/organizations/org-slug/replays/`,
+      body: {},
+    });
   });
 
   it('should render the onboarding panel when the org is on AM1', async () => {
@@ -94,7 +83,7 @@ describe('ReplayList', () => {
     await waitFor(() =>
       expect(screen.getByText('Get to the root cause faster')).toBeInTheDocument()
     );
-    expect(mockUseReplayList).not.toHaveBeenCalled();
+    expect(mockFetchReplayListRequest).not.toHaveBeenCalled();
   });
 
   it('should render the onboarding panel when the org is on AM1 and has sent some replays', async () => {
@@ -116,7 +105,7 @@ describe('ReplayList', () => {
     await waitFor(() =>
       expect(screen.getByText('Get to the root cause faster')).toBeInTheDocument()
     );
-    expect(mockUseReplayList).not.toHaveBeenCalled();
+    expect(mockFetchReplayListRequest).not.toHaveBeenCalled();
   });
 
   it('should render the onboarding panel when the org is on AM2 and has never sent a replay', async () => {
@@ -138,7 +127,7 @@ describe('ReplayList', () => {
     await waitFor(() =>
       expect(screen.getByText('Get to the root cause faster')).toBeInTheDocument()
     );
-    expect(mockUseReplayList).not.toHaveBeenCalled();
+    expect(mockFetchReplayListRequest).not.toHaveBeenCalled();
   });
 
   it('should render the rage-click sdk update banner when the org is AM2, has sent replays, but the sdk version is low', async () => {
@@ -152,12 +141,6 @@ describe('ReplayList', () => {
       isFetching: false,
       needsUpdate: true,
     });
-    mockUseReplayList.mockReturnValue({
-      replays: [],
-      isFetching: false,
-      fetchError: undefined,
-      pageLinks: null,
-    });
 
     render(<ListPage />, {
       context: getMockContext(mockOrg),
@@ -167,7 +150,7 @@ describe('ReplayList', () => {
       expect(screen.queryByText('Introducing Rage and Dead Clicks')).toBeInTheDocument();
       expect(screen.queryByTestId('replay-table')).toBeInTheDocument();
     });
-    expect(mockUseReplayList).toHaveBeenCalled();
+    expect(mockFetchReplayListRequest).toHaveBeenCalled();
   });
 
   it('should fetch the replay table and show selector tables when the org is on AM2, has sent some replays, and has a newer SDK version', async () => {
@@ -181,12 +164,6 @@ describe('ReplayList', () => {
       isFetching: false,
       needsUpdate: false,
     });
-    mockUseReplayList.mockReturnValue({
-      replays: [],
-      isFetching: false,
-      fetchError: undefined,
-      pageLinks: null,
-    });
     mockUseDeadRageSelectors.mockReturnValue({
       isLoading: false,
       isError: false,
@@ -202,6 +179,6 @@ describe('ReplayList', () => {
     await waitFor(() =>
       expect(screen.queryAllByTestId('selector-widget')).toHaveLength(2)
     );
-    expect(mockUseReplayList).toHaveBeenCalled();
+    expect(mockFetchReplayListRequest).toHaveBeenCalled();
   });
 });

+ 27 - 57
static/app/views/replays/list/replaysList.tsx

@@ -1,74 +1,47 @@
 import {Fragment, useMemo} from 'react';
 import {browserHistory} from 'react-router';
 import styled from '@emotion/styled';
-import type {Location} from 'history';
 
 import Pagination from 'sentry/components/pagination';
 import {t, tct} from 'sentry/locale';
-import type {Organization} from 'sentry/types';
 import {trackAnalytics} from 'sentry/utils/analytics';
-import EventView from 'sentry/utils/discover/eventView';
-import {decodeScalar} from 'sentry/utils/queryString';
-import {DEFAULT_SORT} from 'sentry/utils/replays/fetchReplayList';
-import useReplayList from 'sentry/utils/replays/hooks/useReplayList';
+import {decodeList, decodeScalar, decodeSorts} from 'sentry/utils/queryString';
+import useFetchReplayList from 'sentry/utils/replays/hooks/useFetchReplayList';
 import {MutableSearch} from 'sentry/utils/tokenizeSearch';
-import {useLocation} from 'sentry/utils/useLocation';
+import useLocationQuery from 'sentry/utils/url/useLocationQuery';
 import useOrganization from 'sentry/utils/useOrganization';
 import usePageFilters from 'sentry/utils/usePageFilters';
 import useProjectSdkNeedsUpdate from 'sentry/utils/useProjectSdkNeedsUpdate';
 import useAllMobileProj from 'sentry/views/replays/detail/useAllMobileProj';
 import ReplayTable from 'sentry/views/replays/replayTable';
 import {ReplayColumn} from 'sentry/views/replays/replayTable/types';
-import type {ReplayListLocationQuery} from 'sentry/views/replays/types';
-import {REPLAY_LIST_FIELDS} from 'sentry/views/replays/types';
+
+const MIN_REPLAY_CLICK_SDK = '7.44.0';
 
 function ReplaysList() {
-  const location = useLocation<ReplayListLocationQuery>();
   const organization = useOrganization();
 
-  const eventView = useMemo(() => {
-    const query = decodeScalar(location.query.query, '');
-    const conditions = new MutableSearch(query);
-
-    return EventView.fromNewQueryWithLocation(
-      {
-        id: '',
-        name: '',
-        version: 2,
-        fields: REPLAY_LIST_FIELDS,
-        projects: [],
-        query: conditions.formatString(),
-        orderby: decodeScalar(location.query.sort, DEFAULT_SORT),
-      },
-      location
-    );
-  }, [location]);
-
-  return (
-    <ReplaysListTable
-      eventView={eventView}
-      location={location}
-      organization={organization}
-    />
-  );
-}
-
-const MIN_REPLAY_CLICK_SDK = '7.44.0';
+  const query = useLocationQuery({
+    fields: {
+      cursor: decodeScalar,
+      project: decodeList,
+      query: decodeScalar,
+      sort: value => decodeScalar(value, '-started_at'),
+      statsPeriod: decodeScalar,
+    },
+  });
 
-function ReplaysListTable({
-  eventView,
-  location,
-  organization,
-}: {
-  eventView: EventView;
-  location: Location;
-  organization: Organization;
-}) {
-  const {replays, pageLinks, isFetching, fetchError} = useReplayList({
-    eventView,
-    location,
+  const {
+    data: replays,
+    getResponseHeader,
+    isLoading,
+    error,
+  } = useFetchReplayList({
+    options: {query},
     organization,
+    queryReferrer: 'replayList',
   });
+  const pageLinks = getResponseHeader?.('Link') ?? null;
 
   const {
     selection: {projects},
@@ -82,10 +55,7 @@ function ReplaysListTable({
     projectId: projects.map(String),
   });
 
-  const conditions = useMemo(() => {
-    return new MutableSearch(decodeScalar(location.query.query, ''));
-  }, [location.query.query]);
-
+  const conditions = useMemo(() => new MutableSearch(query.query), [query.query]);
   const hasReplayClick = conditions.getFilterKeys().some(k => k.startsWith('click.'));
 
   // browser isn't applicable for mobile projects
@@ -113,10 +83,10 @@ function ReplaysListTable({
     <Fragment>
       <ReplayTable
         referrerLocation={'replay'}
-        fetchError={fetchError}
-        isFetching={isFetching}
+        fetchError={error}
+        isFetching={isLoading}
         replays={replays}
-        sort={eventView.sorts[0]}
+        sort={decodeSorts(query.sort).at(0)}
         visibleColumns={visibleCols}
         showDropdownFilters
         emptyMessage={

+ 1 - 1
static/app/views/replays/replayTable/index.tsx

@@ -31,7 +31,7 @@ import {ReplayColumn} from 'sentry/views/replays/replayTable/types';
 import type {ReplayListRecord} from 'sentry/views/replays/types';
 
 type Props = {
-  fetchError: undefined | Error;
+  fetchError: null | undefined | Error;
   isFetching: boolean;
   replays: undefined | ReplayListRecord[] | ReplayListRecordWithTx[];
   sort: Sort | undefined;

+ 4 - 1
static/app/views/replays/types.tsx

@@ -112,7 +112,10 @@ export type ReplayListLocationQuery = {
   utc?: 'true' | 'false';
 };
 
-export type ReplayListQueryReferrer = 'issueReplays' | 'transactionReplays';
+export type ReplayListQueryReferrer =
+  | 'replayList'
+  | 'issueReplays'
+  | 'transactionReplays';
 
 // Sync with ReplayListRecord below
 export const REPLAY_LIST_FIELDS = [