Error and retry support for infinite scroll (#30964) (#32130)

* Add some unit tests to the existing useInfiniteScroll hook * Change the interface of the infinite scroll hook Now it only exposes one fetch function that can be used to trigger fetches from infinite scroll, regardless of whether it's the initial or a subsequent fetch. Restarting the data stream is driven by changing filtering parameters. * Harden useKeyBasedPagination * Install jsdom-testing-mocks * Error and retry support for infinite scroll * Fix type-check * Fix a state management bug * Make useInfiniteScroll a wrapper over useKeyBasedPagination * Get rid of unnecessary useEffect * Address review comments * Layout fixes * Review * Lint fix * Add the license * One more review round * Tweak the comment in useInfiniteScroll --------- Co-authored-by: Isaiah Becker-Mayer <[email protected]>
gravitational · Sep 19, 2023 · 4723c82 · 4723c82
1 parent cc80adb
commit 4723c82
Show file tree

Hide file tree

Showing 14 changed files with 1,003 additions and 214 deletions.
diff --git a/web/packages/build/package.json b/web/packages/build/package.json
@@ -80,6 +80,7 @@
     "jest": "^27.3.1",
     "jest-styled-components": "^7.0.8",
     "jsdom": "^21.1.0",
+    "jsdom-testing-mocks": "^1.9.0",
     "msw": "^0.47.4",
     "optimist": "^0.6.1",
     "react": "^16.8.4",

diff --git a/web/packages/teleport/src/UnifiedResources/ResourceCard.tsx b/web/packages/teleport/src/UnifiedResources/ResourceCard.tsx
@@ -323,7 +323,7 @@ function CopyButton({ name }: { name: string }) {
   );
 }
 
-function resourceName(resource: UnifiedResource) {
+export function resourceName(resource: UnifiedResource) {
   if (resource.kind === 'app' && resource.friendlyName) {
     return resource.friendlyName;
   }

diff --git a/web/packages/teleport/src/UnifiedResources/Resources.tsx b/web/packages/teleport/src/UnifiedResources/Resources.tsx
@@ -14,31 +14,41 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { useEffect, useRef, useState } from 'react';
+import React, { useEffect, useState } from 'react';
+
 import styled from 'styled-components';
-import { Box, Indicator, Flex, Text } from 'design';
+import {
+  Box,
+  Indicator,
+  Flex,
+  ButtonLink,
+  ButtonSecondary,
+  Text,
+} from 'design';
 import { Magnifier } from 'design/Icon';
 
+import { Danger } from 'design/Alert';
+
 import { TextIcon } from 'teleport/Discover/Shared';
 
 import {
   FeatureBox,
   FeatureHeader,
   FeatureHeaderTitle,
 } from 'teleport/components/Layout';
-import ErrorMessage from 'teleport/components/AgentErrorMessage';
 import Empty, { EmptyStateInfo } from 'teleport/components/Empty';
 import useTeleport from 'teleport/useTeleport';
 import cfg from 'teleport/config';
 import history from 'teleport/services/history/history';
 import localStorage from 'teleport/services/localStorage';
 import useStickyClusterId from 'teleport/useStickyClusterId';
 import AgentButtonAdd from 'teleport/components/AgentButtonAdd';
-import { useInfiniteScroll } from 'teleport/components/hooks/useInfiniteScroll';
 import { SearchResource } from 'teleport/Discover/SelectResource';
-import { useUrlFiltering } from 'teleport/components/hooks';
+import { useUrlFiltering, useInfiniteScroll } from 'teleport/components/hooks';
 
-import { ResourceCard, LoadingCard } from './ResourceCard';
+import { UnifiedResource } from 'teleport/services/agents';
+
+import { ResourceCard, LoadingCard, resourceName } from './ResourceCard';
 import SearchPanel from './SearchPanel';
 import { FilterPanel } from './FilterPanel';
 import './unifiedStyles.css';
@@ -58,34 +68,26 @@ export function Resources() {
   const canCreate = teleCtx.storeUser.getTokenAccess().create;
   const { clusterId } = useStickyClusterId();
 
-  const filtering = useUrlFiltering({
-    fieldName: 'name',
-    dir: 'ASC',
-  });
+  const { params, setParams, replaceHistory, pathname, setSort, onLabelClick } =
+    useUrlFiltering({
+      fieldName: 'name',
+      dir: 'ASC',
+    });
+
   const {
-    params,
-    search,
-    setParams,
-    replaceHistory,
-    pathname,
-    setSort,
-    onLabelClick,
-  } = filtering;
-
-  const { fetchInitial, fetchedData, attempt, fetchMore } = useInfiniteScroll({
+    setTrigger: setScrollDetector,
+    forceFetch,
+    resources,
+    attempt,
+  } = useInfiniteScroll({
     fetchFunc: teleCtx.resourceService.fetchUnifiedResources,
     clusterId,
+    filter: params,
     initialFetchSize: INITIAL_FETCH_SIZE,
     fetchMoreSize: FETCH_MORE_SIZE,
-    params,
   });
 
-  useEffect(() => {
-    fetchInitial();
-  }, [clusterId, search]);
-
-  const noResults =
-    attempt.status === 'success' && fetchedData.agents.length === 0;
+  const noResults = attempt.status === 'success' && resources.length === 0;
 
   const [isSearchEmpty, setIsSearchEmpty] = useState(true);
 
@@ -95,34 +97,14 @@ export function Resources() {
     setIsSearchEmpty(!params?.query && !params?.search);
   }, [params.query, params.search]);
 
-  const infiniteScrollDetector = useRef(null);
-
-  // Install the infinite scroll intersection observer.
-  //
-  // TODO(bl-nero): There's a known issue here. We need to have `fetchMore` in
-  // the list of hook dependencies, because using a stale `fetchMore` closure
-  // means we will fetch the same data over and over. However, as it's
-  // implemented now, every time `fetchMore` changes, we reinstall the observer.
-  // This is mitigated by `fetchMore` implementation, which doesn't spawn
-  // another request before the first one finishes, but it's still a potential
-  // for trouble in future. We need to decouple updating the `fetchMore` closure
-  // and installing the observer.
-  useEffect(() => {
-    if (infiniteScrollDetector.current) {
-      const observer = new IntersectionObserver(entries => {
-        if (entries[0]?.isIntersecting) {
-          fetchMore();
-        }
-      });
-      observer.observe(infiniteScrollDetector.current);
-      return () => observer.disconnect();
-    }
-  });
-
   if (!enabled) {
     history.replace(cfg.getNodesRoute(clusterId));
   }
 
+  const onRetryClicked = () => {
+    forceFetch();
+  };
+
   return (
     <FeatureBox
       className="ContainerContext"
@@ -132,6 +114,18 @@ export function Resources() {
         margin: auto;
       `}
     >
+      {attempt.status === 'failed' && (
+        <ErrorBox>
+          <ErrorBoxInternal>
+            <Danger>
+              {attempt.statusText}
+              <Box flex="0 0 auto" ml={2}>
+                <ButtonLink onClick={onRetryClicked}>Retry</ButtonLink>
+              </Box>
+            </Danger>
+          </ErrorBoxInternal>
+        </ErrorBox>
+      )}
       <FeatureHeader
         css={`
           border-bottom: none;
@@ -163,46 +157,45 @@ export function Resources() {
         pathname={pathname}
         replaceHistory={replaceHistory}
       />
-      {attempt.status === 'failed' && (
-        <ErrorMessage message={attempt.statusText} />
-      )}
       <ResourcesContainer className="ResourcesContainer" gap={2}>
-        {fetchedData.agents.map((agent, i) => (
-          <ResourceCard key={i} onLabelClick={onLabelClick} resource={agent} />
+        {resources.map(res => (
+          <ResourceCard
+            key={resourceKey(res)}
+            resource={res}
+            onLabelClick={onLabelClick}
+          />
         ))}
         {/* Using index as key here is ok because these elements never change order */}
         {attempt.status === 'processing' &&
           loadingCardArray.map((_, i) => <LoadingCard key={i} />)}
       </ResourcesContainer>
-      <div
-        ref={infiniteScrollDetector}
-        style={{
-          visibility: attempt.status === 'processing' ? 'visible' : 'hidden',
-        }}
-      >
-        {(attempt.status === 'processing' || fetchedData.startKey) && (
-          <Box
-            textAlign="center"
-            style={{ visible: attempt.status === 'processing' }}
-          >
-            <Indicator />
-          </Box>
+      <div ref={setScrollDetector} />
+      <ListFooter>
+        <IndicatorContainer status={attempt.status}>
+          <Indicator size={INDICATOR_SIZE} />
+        </IndicatorContainer>
+        {attempt.status === 'failed' && resources.length > 0 && (
+          <ButtonSecondary onClick={onRetryClicked}>Load more</ButtonSecondary>
         )}
-      </div>
-      {noResults && isSearchEmpty && (
-        <Empty
-          clusterId={clusterId}
-          canCreate={canCreate && !isLeafCluster}
-          emptyStateInfo={emptyStateInfo}
-        />
-      )}
-      {noResults && !isSearchEmpty && (
-        <NoResults query={params?.query || params?.search} />
-      )}
+        {noResults && isSearchEmpty && (
+          <Empty
+            clusterId={clusterId}
+            canCreate={canCreate && !isLeafCluster}
+            emptyStateInfo={emptyStateInfo}
+          />
+        )}
+        {noResults && !isSearchEmpty && (
+          <NoResults query={params?.query || params?.search} />
+        )}
+      </ListFooter>
     </FeatureBox>
   );
 }
 
+function resourceKey(resource: UnifiedResource) {
+  return `${resource.kind}/${resourceName(resource)}`;
+}
+
 function NoResults({ query }: { query: string }) {
   // Prevent `No resources were found for ""` flicker.
   if (query) {
@@ -234,6 +227,39 @@ const ResourcesContainer = styled(Flex)`
   grid-template-columns: repeat(auto-fill, minmax(400px, 1fr));
 `;
 
+const ErrorBox = styled(Box)`
+  position: sticky;
+  top: 0;
+  z-index: 1;
+`;
+
+const ErrorBoxInternal = styled(Box)`
+  position: absolute;
+  left: 0;
+  right: 0;
+  margin: ${props => props.theme.space[1]}px 10% 0 10%;
+`;
+
+const INDICATOR_SIZE = '48px';
+
+// It's important to make the footer at least as big as the loading indicator,
+// since in the typical case, we want to avoid UI "jumping" when loading the
+// final fragment finishes, and the final fragment is just one element in the
+// final row (i.e. the number of rows doesn't change). It's then important to
+// keep the same amount of whitespace below the resource list.
+const ListFooter = styled.div`
+  margin-top: ${props => props.theme.space[2]}px;
+  min-height: ${INDICATOR_SIZE};
+  text-align: center;
+`;
+
+// Line height is set to 0 to prevent the layout engine from adding extra pixels
+// to the element's height.
+const IndicatorContainer = styled(Box)`
+  display: ${props => (props.status === 'processing' ? 'block' : 'none')};
+  line-height: 0;
+`;
+
 const emptyStateInfo: EmptyStateInfo = {
   title: 'Add your first resource to Teleport',
   byline:

diff --git a/web/packages/teleport/src/components/hooks/index.ts b/web/packages/teleport/src/components/hooks/index.ts
@@ -16,3 +16,4 @@
 
 export { useUrlFiltering } from './useUrlFiltering';
 export { useServerSidePagination } from './useServersidePagination';
+export { useInfiniteScroll } from './useInfiniteScroll';
Original file line number	Diff line number	Diff line change
Expand Up		@@ -16,3 +16,4 @@

		export { useUrlFiltering } from './useUrlFiltering';
		export { useServerSidePagination } from './useServersidePagination';
		export { useInfiniteScroll } from './useInfiniteScroll';