Browse Source

ref(insights): Improve async dropdown load behaviour (#74297)

We have a few UI dropdowns that fetch more data on search. The Queries
page "Table" selector and the "Assets" page "Page" selector are two
prominent examples. When a user types into that box, we make a server
request to fetch possible matches, since there are more items than we
can load on page load.

The two dropdowns have different behaviour, which is yucky. This PR
consolidates them.

1. Moves `TransactionSelector` into the `common` directory, out of its
parent, so it can be re-used
2. Extracts the caching logic for dropdown results

`TransactionSelector` and `DomainSelector` code looks very similar now,
which is good news! The _main_ benefit though, is they both have the
same behaviour:

- the dropdown contents are _always available_ even if we're currently
loading more possible matches
- a loading spinner shows when data is loading, instead of loading text
- we don't make extra requests if there are < 100 possible total matches
and we've loaded them all
- the current selection from URL is always shown, even if it wasn't in
the autocomplete fetch results
George Gritsouk 7 months ago
parent
commit
88de3ed39b

+ 2 - 79
static/app/views/insights/browser/resources/components/resourceView.tsx

@@ -1,6 +1,5 @@
-import {Fragment, useCallback, useEffect, useState} from 'react';
+import {Fragment} from 'react';
 import styled from '@emotion/styled';
-import debounce from 'lodash/debounce';
 
 import {t} from 'sentry/locale';
 import {space} from 'sentry/styles/space';
@@ -16,7 +15,6 @@ import {
   FONT_FILE_EXTENSIONS,
   IMAGE_FILE_EXTENSIONS,
 } from 'sentry/views/insights/browser/resources/constants';
-import {useResourcePagesQuery} from 'sentry/views/insights/browser/resources/queries/useResourcePagesQuery';
 import {
   DEFAULT_RESOURCE_TYPES,
   RESOURCE_THROUGHPUT_UNIT,
@@ -29,6 +27,7 @@ import {
 import {useResourceSort} from 'sentry/views/insights/browser/resources/utils/useResourceSort';
 import {useHasDataTrackAnalytics} from 'sentry/views/insights/common/utils/useHasDataTrackAnalytics';
 import {QueryParameterNames} from 'sentry/views/insights/common/views/queryParameters';
+import {TransactionSelector} from 'sentry/views/insights/common/views/spans/selectors/transactionSelector';
 import {SpanTimeCharts} from 'sentry/views/insights/common/views/spans/spanTimeCharts';
 import type {ModuleFilters} from 'sentry/views/insights/common/views/spans/useModuleFilters';
 import {ModuleName} from 'sentry/views/insights/types';
@@ -128,82 +127,6 @@ function ResourceTypeSelector({value}: {value?: string}) {
   );
 }
 
-export function TransactionSelector({
-  value,
-  defaultResourceTypes,
-}: {
-  defaultResourceTypes?: string[];
-  value?: string;
-}) {
-  const [state, setState] = useState({
-    search: '',
-    inputChanged: false,
-    shouldRequeryOnInputChange: false,
-  });
-  const location = useLocation();
-  const organization = useOrganization();
-
-  const {data: pages, isLoading} = useResourcePagesQuery(
-    defaultResourceTypes,
-    state.search
-  );
-
-  // If the maximum number of pages is returned, we need to requery on input change to get full results
-  if (!state.shouldRequeryOnInputChange && pages && pages.length >= 100) {
-    setState({...state, shouldRequeryOnInputChange: true});
-  }
-
-  // Everytime loading is complete, reset the inputChanged state
-  useEffect(() => {
-    if (!isLoading && state.inputChanged) {
-      setState({...state, inputChanged: false});
-    }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [isLoading]);
-
-  const optionsReady = !isLoading && !state.inputChanged;
-
-  const options: Option[] = optionsReady
-    ? [{value: '', label: 'All'}, ...pages.map(page => ({value: page, label: page}))]
-    : [];
-
-  // eslint-disable-next-line react-hooks/exhaustive-deps
-  const debounceUpdateSearch = useCallback(
-    debounce((search, currentState) => {
-      setState({...currentState, search});
-    }, 500),
-    []
-  );
-
-  return (
-    <SelectControlWithProps
-      inFieldLabel={`${t('Page')}:`}
-      options={options}
-      value={value}
-      onInputChange={input => {
-        if (state.shouldRequeryOnInputChange) {
-          setState({...state, inputChanged: true});
-          debounceUpdateSearch(input, state);
-        }
-      }}
-      noOptionsMessage={() => (optionsReady ? undefined : t('Loading...'))}
-      onChange={newValue => {
-        trackAnalytics('insight.asset.filter_by_page', {
-          organization,
-        });
-        browserHistory.push({
-          ...location,
-          query: {
-            ...location.query,
-            [TRANSACTION]: newValue?.value,
-            [QueryParameterNames.SPANS_CURSOR]: undefined,
-          },
-        });
-      }}
-    />
-  );
-}
-
 export const SpanTimeChartsContainer = styled('div')`
   margin-bottom: ${space(2)};
 `;

+ 97 - 0
static/app/views/insights/common/utils/useCompactSelectOptionsCache.spec.tsx

@@ -0,0 +1,97 @@
+import {renderHook} from 'sentry-test/reactTestingLibrary';
+
+import {useCompactSelectOptionsCache} from 'sentry/views/insights/common/utils/useCompactSelectOptionsCache';
+
+describe('useCompactSelectOptionsCache', function () {
+  it('keeps a cache', function () {
+    const {result, rerender} = renderHook(
+      (args: Parameters<typeof useCompactSelectOptionsCache>) => {
+        return useCompactSelectOptionsCache(...args);
+      },
+      {
+        initialProps: [
+          [
+            {value: '', label: 'All'},
+            {value: '2', label: '2XX'},
+            {value: '3', label: '3XX'},
+          ],
+        ],
+      }
+    );
+
+    expect(result.current.options).toEqual([
+      {value: '', label: 'All'},
+      {value: '2', label: '2XX'},
+      {value: '3', label: '3XX'},
+    ]);
+
+    rerender([[{value: '4', label: '4XX'}]]);
+    rerender([[{value: '5', label: '5XX'}]]);
+    expect(result.current.options).toEqual([
+      {value: '', label: 'All'},
+      {value: '2', label: '2XX'},
+      {value: '3', label: '3XX'},
+      {value: '4', label: '4XX'},
+      {value: '5', label: '5XX'},
+    ]);
+  });
+
+  it('sorts the output', function () {
+    const {result} = renderHook(
+      (args: Parameters<typeof useCompactSelectOptionsCache>) => {
+        return useCompactSelectOptionsCache(...args);
+      },
+      {
+        initialProps: [
+          [
+            {value: '3', label: '3XX'},
+            {value: '5', label: '5XX'},
+            {value: '', label: 'All'},
+            {value: '2', label: '2XX'},
+            {value: '4', label: '4XX'},
+          ],
+        ],
+      }
+    );
+
+    expect(result.current.options).toEqual([
+      {value: '', label: 'All'},
+      {value: '2', label: '2XX'},
+      {value: '3', label: '3XX'},
+      {value: '4', label: '4XX'},
+      {value: '5', label: '5XX'},
+    ]);
+  });
+
+  it('clears the cache', function () {
+    const {result, rerender} = renderHook(
+      (args: Parameters<typeof useCompactSelectOptionsCache>) => {
+        return useCompactSelectOptionsCache(...args);
+      },
+      {
+        initialProps: [
+          [
+            {value: '3', label: '3XX'},
+            {value: '5', label: '5XX'},
+            {value: '', label: 'All'},
+            {value: '2', label: '2XX'},
+            {value: '4', label: '4XX'},
+          ],
+        ],
+      }
+    );
+
+    expect(result.current.options).toEqual([
+      {value: '', label: 'All'},
+      {value: '2', label: '2XX'},
+      {value: '3', label: '3XX'},
+      {value: '4', label: '4XX'},
+      {value: '5', label: '5XX'},
+    ]);
+
+    result.current.clear();
+    rerender([[]]);
+
+    expect(result.current.options).toEqual([]);
+  });
+});

+ 48 - 0
static/app/views/insights/common/utils/useCompactSelectOptionsCache.tsx

@@ -0,0 +1,48 @@
+import {useCallback, useMemo, useRef} from 'react';
+
+import type {SelectKey, SelectOption} from 'sentry/components/compactSelect';
+
+type Option = SelectOption<SelectKey>;
+
+type OptionCache = Map<SelectKey, Option>;
+
+/**
+ * Cache designed for the `option` prop of a `CompactSelect`. Accepts
+ * an array of `Option` objects. Returns a list of all `Option` objects
+ * it has been passed since instantiation.
+ *
+ * Useful when passing a `CompactSelect` `options` that come from a server
+ *  response. With a cache, `CompactSelect` can:
+ *
+ * 1. Display an options dropdown even when new results are loading
+ * 2. Shows options from 3 searches ago if the user changes their mind
+ *
+ * NOTE: The `clear` callback does not trigger a re-render. The cleared
+ * cache is returned on next call of the hook.
+ */
+export function useCompactSelectOptionsCache(options: Option[]): {
+  clear: () => void;
+  options: readonly Option[];
+} {
+  const cache = useRef<OptionCache>(new Map());
+
+  const clearCache = useCallback(() => {
+    cache.current.clear();
+  }, []);
+
+  const outgoingOptions = useMemo(() => {
+    options.forEach(option => {
+      cache.current.set(option.value, option);
+    });
+
+    return Array.from(cache.current.values()).sort(alphabeticalCompare);
+  }, [options]);
+
+  return {options: outgoingOptions, clear: clearCache};
+}
+
+type OptionComparator = (a: Option, b: Option) => number;
+
+const alphabeticalCompare: OptionComparator = (a, b) => {
+  return a.value.toString().localeCompare(b.value.toString());
+};

+ 32 - 0
static/app/views/insights/common/utils/useWasSearchSpaceExhausted.tsx

@@ -0,0 +1,32 @@
+import {useEffect, useState} from 'react';
+
+import parseLinkHeader from 'sentry/utils/parseLinkHeader';
+
+interface Props {
+  isLoading: boolean;
+  query: string;
+  pageLinks?: string;
+}
+
+/**
+ * Keeps track of the passed responses. Remembers if at any point a response
+ *  had no query, and didn't have any subsequent data. This means that at
+ * some point, a _complete_ response was served. Useful for caches and re-fetch
+ * behavior where we want to _avoid fetches_ if we know we've loaded the
+ * entire data set at some point and a cache is full.
+ */
+export function useWasSearchSpaceExhausted({query, isLoading, pageLinks}: Props) {
+  const [wasSearchSpaceExhausted, setWasSearchSpaceExhausted] = useState<boolean>(false);
+
+  useEffect(() => {
+    if (query === '' && !isLoading) {
+      const {next} = parseLinkHeader(pageLinks ?? '');
+
+      if (!next) {
+        setWasSearchSpaceExhausted(true);
+      }
+    }
+  }, [query, isLoading, pageLinks]);
+
+  return wasSearchSpaceExhausted;
+}

+ 30 - 43
static/app/views/insights/common/views/spans/selectors/domainSelector.tsx

@@ -1,5 +1,5 @@
 import type {ReactNode} from 'react';
-import {useCallback, useEffect, useRef, useState} from 'react';
+import {useCallback, useEffect, useState} from 'react';
 import type {Location} from 'history';
 import debounce from 'lodash/debounce';
 import omit from 'lodash/omit';
@@ -11,12 +11,14 @@ import {uniq} from 'sentry/utils/array/uniq';
 import {browserHistory} from 'sentry/utils/browserHistory';
 import EventView from 'sentry/utils/discover/eventView';
 import {DiscoverDatasets} from 'sentry/utils/discover/types';
-import parseLinkHeader from 'sentry/utils/parseLinkHeader';
 import {EMPTY_OPTION_VALUE} from 'sentry/utils/tokenizeSearch';
 import {useLocation} from 'sentry/utils/useLocation';
 import useOrganization from 'sentry/utils/useOrganization';
+import usePageFilters from 'sentry/utils/usePageFilters';
 import {useSpansQuery} from 'sentry/views/insights/common/queries/useSpansQuery';
 import {buildEventViewQuery} from 'sentry/views/insights/common/utils/buildEventViewQuery';
+import {useCompactSelectOptionsCache} from 'sentry/views/insights/common/utils/useCompactSelectOptionsCache';
+import {useWasSearchSpaceExhausted} from 'sentry/views/insights/common/utils/useWasSearchSpaceExhausted';
 import {QueryParameterNames} from 'sentry/views/insights/common/views/queryParameters';
 import {EmptyContainer} from 'sentry/views/insights/common/views/spans/selectors/emptyOption';
 import {ModuleName, SpanMetricsField} from 'sentry/views/insights/types';
@@ -33,11 +35,6 @@ interface DomainData {
   'span.domain': string[];
 }
 
-interface DomainCacheValue {
-  domains: Set<string>;
-  initialLoadHadMoreData: boolean;
-}
-
 export function DomainSelector({
   value = '',
   moduleName = ModuleName.ALL,
@@ -47,14 +44,15 @@ export function DomainSelector({
 }: Props) {
   const location = useLocation();
   const organization = useOrganization();
+  const pageFilters = usePageFilters();
 
   const [searchInputValue, setSearchInputValue] = useState<string>(''); // Realtime domain search value in UI
-  const [domainQuery, setDomainQuery] = useState<string>(''); // Debounced copy of `searchInputValue` used for the Discover query
+  const [searchQuery, setSearchQuery] = useState<string>(''); // Debounced copy of `searchInputValue` used for the Discover query
 
   // eslint-disable-next-line react-hooks/exhaustive-deps
   const debouncedSetSearch = useCallback(
     debounce(newSearch => {
-      setDomainQuery(newSearch);
+      setSearchQuery(newSearch);
     }, 500),
     []
   );
@@ -63,7 +61,7 @@ export function DomainSelector({
     location,
     moduleName,
     spanCategory,
-    domainQuery,
+    searchQuery,
     additionalQuery
   );
 
@@ -78,36 +76,33 @@ export function DomainSelector({
     referrer: 'api.starfish.get-span-domains',
   });
 
-  const incomingDomains = uniq(
-    domainData?.flatMap(row => row[SpanMetricsField.SPAN_DOMAIN])
-  );
-
-  // Cache for all previously seen domains
-  const domainCache = useRef<DomainCacheValue>({
-    domains: new Set(),
-    initialLoadHadMoreData: true,
+  const wasSearchSpaceExhausted = useWasSearchSpaceExhausted({
+    query: searchQuery,
+    isLoading,
+    pageLinks,
   });
 
-  // The current selected table might not be in the cached set. Ensure it's always there
+  const incomingDomains = [
+    ...uniq(domainData?.flatMap(row => row[SpanMetricsField.SPAN_DOMAIN])),
+  ];
+
   if (value) {
-    domainCache.current.domains.add(value);
+    incomingDomains.push(value);
   }
 
-  // When caching the unfiltered domain data result, check if it had more data. If not, there's no point making any more requests when users update the search filter that narrows the search
-  useEffect(() => {
-    if (domainQuery === '' && !isLoading) {
-      const {next} = parseLinkHeader(pageLinks ?? '');
-
-      domainCache.current.initialLoadHadMoreData = next?.results ?? false;
-    }
-  }, [domainQuery, pageLinks, isLoading]);
+  const {options: domainOptions, clear: clearDomainOptionsCache} =
+    useCompactSelectOptionsCache(
+      incomingDomains.filter(Boolean).map(datum => {
+        return {
+          value: datum,
+          label: datum,
+        };
+      })
+    );
 
-  // Cache all known domains from previous requests
   useEffect(() => {
-    incomingDomains?.forEach(domain => {
-      domainCache.current.domains.add(domain);
-    });
-  }, [incomingDomains]);
+    clearDomainOptionsCache();
+  }, [pageFilters.selection.projects, clearDomainOptionsCache]);
 
   const emptyOption = {
     value: EMPTY_OPTION_VALUE,
@@ -119,14 +114,7 @@ export function DomainSelector({
   const options = [
     {value: '', label: 'All'},
     ...(emptyOptionLocation === 'top' ? [emptyOption] : []),
-    ...Array.from(domainCache.current.domains)
-      .map(datum => {
-        return {
-          value: datum,
-          label: datum,
-        };
-      })
-      .sort((a, b) => a.value.localeCompare(b.value)),
+    ...domainOptions,
     ...(emptyOptionLocation === 'bottom' ? [emptyOption] : []),
   ];
 
@@ -140,8 +128,7 @@ export function DomainSelector({
       onInputChange={input => {
         setSearchInputValue(input);
 
-        // If the initial query didn't fetch all the domains, update the search query and fire off a new query with the given search
-        if (domainCache.current.initialLoadHadMoreData) {
+        if (!wasSearchSpaceExhausted) {
           debouncedSetSearch(input);
         }
       }}

+ 96 - 0
static/app/views/insights/common/views/spans/selectors/transactionSelector.tsx

@@ -0,0 +1,96 @@
+import {useCallback, useEffect, useState} from 'react';
+import debounce from 'lodash/debounce';
+
+import SelectControl from 'sentry/components/forms/controls/selectControl';
+import {t} from 'sentry/locale';
+import {trackAnalytics} from 'sentry/utils/analytics';
+import {browserHistory} from 'sentry/utils/browserHistory';
+import {useLocation} from 'sentry/utils/useLocation';
+import useOrganization from 'sentry/utils/useOrganization';
+import usePageFilters from 'sentry/utils/usePageFilters';
+import {useResourcePagesQuery} from 'sentry/views/insights/browser/resources/queries/useResourcePagesQuery';
+import {BrowserStarfishFields} from 'sentry/views/insights/browser/resources/utils/useResourceFilters';
+import {useCompactSelectOptionsCache} from 'sentry/views/insights/common/utils/useCompactSelectOptionsCache';
+import {useWasSearchSpaceExhausted} from 'sentry/views/insights/common/utils/useWasSearchSpaceExhausted';
+import {QueryParameterNames} from 'sentry/views/insights/common/views/queryParameters';
+
+export function TransactionSelector({
+  value,
+  defaultResourceTypes,
+}: {
+  defaultResourceTypes?: string[];
+  value?: string;
+}) {
+  const location = useLocation();
+  const organization = useOrganization();
+  const pageFilters = usePageFilters();
+
+  const [searchInputValue, setSearchInputValue] = useState<string>(''); // Realtime domain search value in UI
+  const [searchQuery, setSearchQuery] = useState<string>(''); // Debounced copy of `searchInputValue` used for the Discover query
+
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  const debouncedSetSearch = useCallback(
+    debounce(newSearch => {
+      setSearchQuery(newSearch);
+    }, 500),
+    []
+  );
+
+  const {
+    data: incomingPages,
+    isLoading,
+    pageLinks,
+  } = useResourcePagesQuery(defaultResourceTypes, searchQuery);
+
+  if (value) {
+    incomingPages.push(value);
+  }
+
+  const wasSearchSpaceExhausted = useWasSearchSpaceExhausted({
+    query: searchQuery,
+    isLoading,
+    pageLinks,
+  });
+
+  const {options: transactionOptions, clear: clearTransactionOptionsCache} =
+    useCompactSelectOptionsCache(
+      incomingPages.filter(Boolean).map(page => ({value: page, label: page}))
+    );
+
+  useEffect(() => {
+    clearTransactionOptionsCache();
+  }, [pageFilters.selection.projects, clearTransactionOptionsCache]);
+
+  const options = [{value: '', label: 'All'}, ...transactionOptions];
+
+  return (
+    <SelectControl
+      inFieldLabel={`${t('Page')}:`}
+      inputValue={searchInputValue}
+      value={value}
+      options={options}
+      isLoading={isLoading}
+      onInputChange={input => {
+        setSearchInputValue(input);
+
+        if (!wasSearchSpaceExhausted) {
+          debouncedSetSearch(input);
+        }
+      }}
+      onChange={newValue => {
+        trackAnalytics('insight.asset.filter_by_page', {
+          organization,
+        });
+        browserHistory.push({
+          ...location,
+          query: {
+            ...location.query,
+            [BrowserStarfishFields.TRANSACTION]: newValue?.value,
+            [QueryParameterNames.SPANS_CURSOR]: undefined,
+          },
+        });
+      }}
+      noOptionsMessage={() => t('No results')}
+    />
+  );
+}