From ed0d8a0115b6c5eb575fb9039ee03182a3801026 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Mon, 18 Sep 2023 11:20:12 +0200 Subject: [PATCH] Error and retry support for infinite scroll (#30964) * 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 --------- Co-authored-by: Isaiah Becker-Mayer --- web/packages/build/package.json | 1 + .../src/UnifiedResources/ResourceCard.tsx | 2 +- .../src/UnifiedResources/Resources.tsx | 186 +++++---- .../teleport/src/components/hooks/index.ts | 1 + .../src/components/hooks/useInfiniteScroll.ts | 128 ------- .../hooks/useInfiniteScroll/index.ts | 17 + .../hooks/useInfiniteScroll/testUtils.ts | 104 ++++++ .../useInfiniteScroll.test.tsx | 90 +++++ .../useInfiniteScroll/useInfiniteScroll.ts | 98 +++++ .../useKeyBasedPagination.test.ts | 352 ++++++++++++++++++ .../useKeyBasedPagination.ts | 204 ++++++++++ web/packages/teleport/src/services/api/api.js | 12 +- .../teleport/src/services/api/parseError.ts | 4 +- yarn.lock | 18 + 14 files changed, 1003 insertions(+), 214 deletions(-) delete mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll.ts create mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll/index.ts create mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll/testUtils.ts create mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.test.tsx create mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.ts create mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.test.ts create mode 100644 web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.ts diff --git a/web/packages/build/package.json b/web/packages/build/package.json index 2c8a4b56b602b..62afd0dfca945 100644 --- 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 index 373c109eaef0d..8e854874b0f0a 100644 --- 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 index a9369c79cc5b1..9befd75985680 100644 --- a/web/packages/teleport/src/UnifiedResources/Resources.tsx +++ b/web/packages/teleport/src/UnifiedResources/Resources.tsx @@ -14,11 +14,21 @@ 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 { @@ -26,7 +36,6 @@ import { 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'; @@ -34,11 +43,12 @@ 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 ( + {attempt.status === 'failed' && ( + + + + {attempt.statusText} + + Retry + + + + + )} - {attempt.status === 'failed' && ( - - )} - {fetchedData.agents.map((agent, i) => ( - + {resources.map(res => ( + ))} {/* Using index as key here is ok because these elements never change order */} {attempt.status === 'processing' && loadingCardArray.map((_, i) => )} -
- {(attempt.status === 'processing' || fetchedData.startKey) && ( - - - +
+ + + + + {attempt.status === 'failed' && resources.length > 0 && ( + Load more )} -
- {noResults && isSearchEmpty && ( - - )} - {noResults && !isSearchEmpty && ( - - )} + {noResults && isSearchEmpty && ( + + )} + {noResults && !isSearchEmpty && ( + + )} + ); } +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 index 266c8a30d8811..99553b486cd43 100644 --- 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'; diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll.ts b/web/packages/teleport/src/components/hooks/useInfiniteScroll.ts deleted file mode 100644 index c9070db0dc6f7..0000000000000 --- a/web/packages/teleport/src/components/hooks/useInfiniteScroll.ts +++ /dev/null @@ -1,128 +0,0 @@ -/** - * Copyright 2023 Gravitational, Inc - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { useState } from 'react'; -import useAttempt, { Attempt } from 'shared/hooks/useAttemptNext'; - -import { - ResourcesResponse, - UnifiedResource, - ResourceFilter, -} from 'teleport/services/agents'; -import { UrlResourcesParams } from 'teleport/config'; - -/** - * Supports fetching more data from the server when more data is available. Pass - * a `fetchFunc` that retrieves a single batch of data. After the initial - * request, the server is expected to return a `startKey` field that denotes the - * next `startKey` to use for the next request. - */ -export function useInfiniteScroll({ - fetchFunc, - clusterId, - params, - initialFetchSize = 30, - fetchMoreSize = 20, -}: Props): State { - const { attempt, setAttempt } = useAttempt('processing'); - - const [fetchedData, setFetchedData] = useState>({ - agents: [], - startKey: '', - totalCount: 0, - }); - - const fetchInitial = async () => { - setAttempt({ status: 'processing' }); - try { - const res = await fetchFunc(clusterId, { - ...params, - limit: initialFetchSize, - startKey: '', - }); - - setFetchedData({ - ...fetchedData, - agents: res.agents, - startKey: res.startKey, - totalCount: res.totalCount, - }); - setAttempt({ status: 'success' }); - } catch (err) { - setAttempt({ status: 'failed', statusText: err.message }); - setFetchedData({ - agents: [], - startKey: '', - totalCount: 0, - }); - } - }; - - const fetchMore = async () => { - // TODO(bl-nero): Disallowing further requests on failed status is a - // temporary fix to prevent multiple requests from being sent. Currently, - // they wouldn't go through anyway, but at least we don't thrash the UI - // constantly. - if ( - attempt.status === 'processing' || - attempt.status === 'failed' || - !fetchedData.startKey - ) { - return; - } - try { - setAttempt({ status: 'processing' }); - const res = await fetchFunc(clusterId, { - ...params, - limit: fetchMoreSize, - startKey: fetchedData.startKey, - }); - setFetchedData({ - ...fetchedData, - agents: [...fetchedData.agents, ...res.agents], - startKey: res.startKey, - }); - setAttempt({ status: 'success' }); - } catch (err) { - setAttempt({ status: 'failed', statusText: err.message }); - } - }; - - return { - fetchInitial, - fetchMore, - attempt, - fetchedData, - }; -} - -type Props = { - fetchFunc: ( - clusterId: string, - params: UrlResourcesParams - ) => Promise>; - clusterId: string; - params: ResourceFilter; - initialFetchSize?: number; - fetchMoreSize?: number; -}; - -type State = { - fetchInitial: (() => void) | null; - fetchMore: (() => void) | null; - attempt: Attempt; - fetchedData: ResourcesResponse; -}; diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll/index.ts b/web/packages/teleport/src/components/hooks/useInfiniteScroll/index.ts new file mode 100644 index 0000000000000..eaab69e8c522e --- /dev/null +++ b/web/packages/teleport/src/components/hooks/useInfiniteScroll/index.ts @@ -0,0 +1,17 @@ +/** + * Copyright 2023 Gravitational, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { useInfiniteScroll } from './useInfiniteScroll'; diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll/testUtils.ts b/web/packages/teleport/src/components/hooks/useInfiniteScroll/testUtils.ts new file mode 100644 index 0000000000000..0d6f32257a15f --- /dev/null +++ b/web/packages/teleport/src/components/hooks/useInfiniteScroll/testUtils.ts @@ -0,0 +1,104 @@ +/** + * Copyright 2023 Gravitational, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import 'whatwg-fetch'; +import { RenderResult } from '@testing-library/react-hooks'; + +import { UrlResourcesParams } from 'teleport/config'; +import { ApiError } from 'teleport/services/api/parseError'; + +import { Node } from 'teleport/services/nodes'; + +/** + * Creates `n` nodes. We use the `Node` type for testing, because it's slim and + * it has a `clusterId` field. + */ +function makeTestResources( + clusterId: string, + namePrefix: string, + n: number +): Node[] { + return Array(n) + .fill(0) + .map((_, i) => ({ + kind: 'node', + id: i.toString(), + clusterId: clusterId, + hostname: `${namePrefix}${i}`, + labels: [], + addr: '', + tunnel: false, + sshLogins: [], + })); +} + +export function newDOMAbortError() { + return new DOMException('Aborted', 'AbortError'); +} + +export function newApiAbortError() { + return new ApiError('The user aborted a request', new Response(), { + cause: newDOMAbortError(), + }); +} + +/** + * Creates a mock fetch function that pretends to query a pool of given number + * of resources. To simulate a search, `params.search` is used as a resource + * name prefix. + */ +export function newFetchFunc( + numResources: number, + newAbortError: () => Error = newDOMAbortError +) { + return async ( + clusterId: string, + params: UrlResourcesParams, + signal?: AbortSignal + ) => { + const { startKey, limit } = params; + const startIndex = parseInt(startKey || '0'); + const namePrefix = params.search ?? 'r'; + const endIndex = startIndex + limit; + const nextStartKey = + endIndex < numResources ? endIndex.toString() : undefined; + if (signal) { + // Give the caller a chance to abort the request. + await Promise.resolve(); + if (signal.aborted) { + const err = newAbortError(); + if (err) throw err; + } + } + return { + agents: makeTestResources(clusterId, namePrefix, numResources).slice( + startIndex, + startIndex + limit + ), + startKey: nextStartKey, + }; + }; +} + +export function resourceNames(result: RenderResult<{ resources: Node[] }>) { + return result.current.resources.map(r => r.hostname); +} + +export function resourceClusterIds( + result: RenderResult<{ resources: Node[] }> +) { + return result.current.resources.map(r => r.clusterId); +} diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.test.tsx b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.test.tsx new file mode 100644 index 0000000000000..e75d63d5c2315 --- /dev/null +++ b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.test.tsx @@ -0,0 +1,90 @@ +/** + * Copyright 2023 Gravitational, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; + +import { renderHook, act } from '@testing-library/react-hooks'; +import { render, screen } from 'design/utils/testing'; +import { mockIntersectionObserver } from 'jsdom-testing-mocks'; + +import { useInfiniteScroll } from './useInfiniteScroll'; +import { newFetchFunc, resourceNames } from './testUtils'; + +const mio = mockIntersectionObserver(); + +function hookProps() { + return { + fetchFunc: newFetchFunc(7), + trigger: null, + clusterId: 'test-cluster', + filter: {}, + initialFetchSize: 2, + fetchMoreSize: 3, + }; +} + +test('fetches data whenever an element is in view', async () => { + const { result, waitForNextUpdate } = renderHook(useInfiniteScroll, { + initialProps: hookProps(), + }); + render(
); + const trigger = screen.getByTestId('trigger'); + expect(resourceNames(result)).toEqual([]); + + act(() => mio.enterNode(trigger)); + await waitForNextUpdate(); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + + act(() => mio.leaveNode(trigger)); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + + act(() => mio.enterNode(trigger)); + await waitForNextUpdate(); + expect(resourceNames(result)).toEqual(['r0', 'r1', 'r2', 'r3', 'r4']); +}); + +test('supports changing nodes', async () => { + render( + <> +
+
+ + ); + const trigger1 = screen.getByTestId('trigger1'); + const trigger2 = screen.getByTestId('trigger2'); + let props = hookProps(); + const { result, rerender, waitForNextUpdate } = renderHook( + useInfiniteScroll, + { + initialProps: props, + } + ); + result.current.setTrigger(trigger1); + + act(() => mio.enterNode(trigger1)); + await waitForNextUpdate(); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + + rerender(props); + result.current.setTrigger(trigger2); + + // Should only register entering trigger2, reading resources r2 through r4. + act(() => mio.leaveNode(trigger1)); + act(() => mio.enterNode(trigger1)); + act(() => mio.enterNode(trigger2)); + await waitForNextUpdate(); + expect(resourceNames(result)).toEqual(['r0', 'r1', 'r2', 'r3', 'r4']); +}); diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.ts b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.ts new file mode 100644 index 0000000000000..c75e6a99276dd --- /dev/null +++ b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useInfiniteScroll.ts @@ -0,0 +1,98 @@ +/** + * Copyright 2023 Gravitational, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { useLayoutEffect, useRef } from 'react'; + +import { Attempt } from 'shared/hooks/useAttemptNext'; + +import { UnifiedResource } from 'teleport/services/agents'; + +import { + useKeyBasedPagination, + Props as PaginationProps, +} from './useKeyBasedPagination'; + +export type Props = PaginationProps; + +/** + * Fetches a part of resource list whenever the `trigger` element intersects the + * viewport until the list is exhausted or an error happens. + * + * Callers must set the `trigger` element by passing the [`State.setTrigger`] function + * as the `ref` prop of the element they want to use as the trigger. + * + * Use the [`State.forceFetch`] to continue after an error. + */ +export function useInfiniteScroll( + props: Props +): State { + const observer = useRef(null); + const trigger = useRef(null); + + const { fetch, forceFetch, attempt, resources } = + useKeyBasedPagination(props); + + const recreateObserver = () => { + observer.current?.disconnect(); + if (trigger.current) { + observer.current = new IntersectionObserver(entries => { + if (entries[0]?.isIntersecting) { + fetch(); + } + }); + observer.current.observe(trigger.current); + } + }; + + const setTrigger = (el: Element | null) => { + trigger.current = el; + recreateObserver(); + }; + + // Using layout effect instead of a regular one helps prevent sneaky race + // conditions. If we used a regular effect, the observer may be recreated + // after the current one (which, by now, may be tied to a stale state) + // triggers a fetch. Thus, the fetch would use stale state and may ultimately + // cause us to display incorrect data. (This issue can be reproduced by + // switching this to `useEffect` and rapidly changing filtering data on the + // resources list page). + useLayoutEffect(() => { + recreateObserver(); + return () => { + observer.current?.disconnect(); + }; + }, [fetch]); + + return { setTrigger, forceFetch, attempt, resources }; +} + +export type State = { + /** + * Fetches a new batch of data. Cancels a pending request, if there is one. + * Disregards whether error has previously occurred. + */ + forceFetch: () => Promise; + + /** + * Sets an element that will be observed and will trigger a fetch once it + * becomes visible. The element doesn't need to become fully visible; a single + * pixel will be enough to trigger. + */ + setTrigger: (el: Element | null) => void; + + attempt: Attempt; + resources: T[]; +}; diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.test.ts b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.test.ts new file mode 100644 index 0000000000000..a504ca1e2f0e1 --- /dev/null +++ b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.test.ts @@ -0,0 +1,352 @@ +/** + * Copyright 2023 Gravitational, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { renderHook, act } from '@testing-library/react-hooks'; + +import { ApiError } from 'teleport/services/api/parseError'; + +import { Node } from 'teleport/services/nodes'; + +import { useKeyBasedPagination, Props } from './useKeyBasedPagination'; +import { + newApiAbortError, + newDOMAbortError, + newFetchFunc, + resourceClusterIds, + resourceNames, +} from './testUtils'; + +function hookProps(overrides: Partial> = {}) { + return { + fetchFunc: newFetchFunc(7), + clusterId: 'test-cluster', + filter: {}, + initialFetchSize: 2, + fetchMoreSize: 3, + ...overrides, + }; +} + +test.each` + n | names + ${3} | ${['r0', 'r1', 'r2']} + ${4} | ${['r0', 'r1', 'r2', 'r3']} + ${10} | ${['r0', 'r1', 'r2', 'r3']} +`('fetches one data batch, n=$n', async ({ n, names }) => { + const { result } = renderHook(useKeyBasedPagination, { + initialProps: hookProps({ + fetchFunc: newFetchFunc(4), + initialFetchSize: n, + }), + }); + + expect(result.current.resources).toEqual([]); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(names); +}); + +test('fetches multiple data batches', async () => { + const { result } = renderHook(useKeyBasedPagination, { + initialProps: hookProps(), + }); + expect(result.current.finished).toBe(false); + + await act(result.current.fetch); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(['r0', 'r1', 'r2', 'r3', 'r4']); + expect(result.current.finished).toBe(false); + await act(result.current.fetch); + + const allResources = ['r0', 'r1', 'r2', 'r3', 'r4', 'r5', 'r6']; + expect(resourceNames(result)).toEqual(allResources); + expect(result.current.finished).toBe(true); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(allResources); + expect(result.current.finished).toBe(true); +}); + +test('maintains attempt state', async () => { + const { result } = renderHook(useKeyBasedPagination, { + initialProps: hookProps(), + }); + + expect(result.current.attempt.status).toBe(''); + let fetchPromise; + act(() => { + fetchPromise = result.current.fetch(); + }); + expect(result.current.attempt.status).toBe('processing'); + await act(async () => fetchPromise); + expect(result.current.attempt.status).toBe('success'); + + act(() => { + fetchPromise = result.current.fetch(); + }); + expect(result.current.attempt.status).toBe('processing'); + await act(async () => fetchPromise); + expect(result.current.attempt.status).toBe('success'); +}); + +test('restarts after query params change', async () => { + let props = hookProps({ + fetchFunc: newFetchFunc(4), + clusterId: 'cluster1', + filter: { search: 'foo' }, + }); + const { result, rerender } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster1', 'cluster1']); + expect(resourceNames(result)).toEqual(['foo0', 'foo1']); + + props = { ...props, clusterId: 'cluster2' }; + rerender(props); + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster2', 'cluster2']); + + props = { ...props, filter: { search: 'bar' } }; + rerender(props); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(['bar0', 'bar1']); + + // Make sure we reached the end of the data set. + await act(result.current.fetch); + expect(result.current.finished).toBe(true); + props = { ...props, clusterId: 'cluster3' }; + rerender(props); + expect(result.current.finished).toBe(false); + + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster3', 'cluster3']); +}); + +test("doesn't restart if params didn't change on rerender", async () => { + const props = hookProps(); + const { result, rerender } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + rerender(props); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(['r0', 'r1', 'r2', 'r3', 'r4']); +}); + +describe("doesn't react to fetch() calls before the previous one finishes", () => { + let props: Props, fetchSpy; + + beforeEach(() => { + props = hookProps(); + fetchSpy = jest.spyOn(props, 'fetchFunc'); + }); + + test('when called once per state reconciliation cycle', async () => { + const { result } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + let f1, f2; + act(() => { + f1 = result.current.fetch(); + }); + act(() => { + f2 = result.current.fetch(); + }); + + await act(async () => f1); + await act(async () => f2); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + expect(props.fetchFunc).toHaveBeenCalledTimes(1); + }); + + test('when called multiple times per state reconciliation cycle', async () => { + const { result } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + let f1, f2; + act(() => { + f1 = result.current.fetch(); + f2 = result.current.fetch(); + }); + await act(async () => f1); + await act(async () => f2); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); +}); + +test.each([ + ['DOMException', newDOMAbortError], + ['ApiError', newApiAbortError], +])('aborts pending request if params change (%s)', async (_, newError) => { + let props = hookProps({ + clusterId: 'cluster1', + fetchFunc: newFetchFunc(7, newError), + }); + const { result, rerender } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + let fetchPromise; + act(() => { + fetchPromise = result.current.fetch(); + }); + props = { ...props, clusterId: 'cluster2' }; + rerender(props); + await act(async () => fetchPromise); + expect(resourceClusterIds(result)).toEqual([]); + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster2', 'cluster2']); +}); + +describe.each` + name | ErrorType + ${'Error'} | ${Error} + ${'ApiError'} | ${ApiError} +`('for error type $name', ({ ErrorType }) => { + it('stops fetching more pages once error is encountered', async () => { + const props = hookProps(); + const { result } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + const fetchSpy = jest.spyOn(props, 'fetchFunc'); + + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + + fetchSpy.mockImplementationOnce(async () => { + throw new ErrorType('OMGOMG'); + }); + await act(result.current.fetch); + expect(result.current.attempt.status).toBe('failed'); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + + await act(result.current.fetch); + expect(result.current.attempt.status).toBe('failed'); + expect(resourceNames(result)).toEqual(['r0', 'r1']); + }); + + it('restarts fetching after error if params change', async () => { + let props = hookProps({ clusterId: 'cluster1' }); + const fetchSpy = jest.spyOn(props, 'fetchFunc'); + + const { result, rerender } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster1', 'cluster1']); + + fetchSpy.mockImplementationOnce(async () => { + throw new ErrorType('OMGOMG'); + }); + + // Rerender with the same options: still no action expected. + rerender(props); + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster1', 'cluster1']); + + // Rerender with different props: expect new data to be fetched. + props = { ...props, clusterId: 'cluster2' }; + rerender(props); + await act(result.current.fetch); + expect(resourceClusterIds(result)).toEqual(['cluster2', 'cluster2']); + }); + + it('resumes fetching once forceFetch is called after an error', async () => { + const props = hookProps(); + const { result } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + const fetchSpy = jest.spyOn(props, 'fetchFunc'); + + await act(result.current.fetch); + fetchSpy.mockImplementationOnce(async () => { + throw new ErrorType('OMGOMG'); + }); + await act(result.current.fetch); + await act(result.current.forceFetch); + + expect(result.current.attempt.status).toBe('success'); + expect(resourceNames(result)).toEqual(['r0', 'r1', 'r2', 'r3', 'r4']); + }); +}); + +test('forceFetch spawns another request, even if there is one pending', async () => { + const props = hookProps(); + const fetchSpy = jest.spyOn(props, 'fetchFunc'); + const { result } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + fetchSpy.mockImplementationOnce(async () => { + return { + agents: [ + { + kind: 'node', + id: 'sus', + clusterId: 'test-cluster', + hostname: `impostor`, + labels: [], + addr: '', + tunnel: false, + sshLogins: [], + }, + ], + }; + }); + let f1, f2; + act(() => { + f1 = result.current.fetch(); + }); + act(() => { + f2 = result.current.forceFetch(); + }); + await act(async () => Promise.all([f1, f2])); + expect(resourceNames(result)).toEqual(['r0', 'r1']); +}); + +test("doesn't get confused if aborting a request still results in a successful promise", async () => { + // This one is tricky. It turns out that somewhere in our API layer, we + // perform some asynchronous operation that disregards the abort signal. + // Whether it's because some platform implementation doesn't adhere to the + // spec, or whether we miss some detail - all in all, in the principle, looks + // like this hook can't really trust the abort signal to be 100% effective. + let props = hookProps({ + // Create a function that will never throw an abort error. + fetchFunc: newFetchFunc(1, () => null), + filter: { search: 'rabbit' }, + }); + const { result, rerender } = renderHook(useKeyBasedPagination, { + initialProps: props, + }); + await act(result.current.fetch); + expect(resourceNames(result)).toEqual(['rabbit0']); + + let f1, f2; + props = { ...props, filter: { search: 'duck' } }; + rerender(props); + act(() => { + f1 = result.current.fetch(); + }); + + props = { ...props, filter: { search: 'rabbit' } }; + rerender(props); + act(() => { + f2 = result.current.fetch(); + }); + + await act(async () => Promise.all([f1, f2])); + expect(resourceNames(result)).toEqual(['rabbit0']); +}); diff --git a/web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.ts b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.ts new file mode 100644 index 0000000000000..683111cd43b46 --- /dev/null +++ b/web/packages/teleport/src/components/hooks/useInfiniteScroll/useKeyBasedPagination.ts @@ -0,0 +1,204 @@ +/** + * Copyright 2023 Gravitational, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { useState, useRef, useCallback } from 'react'; +import useAttempt, { Attempt } from 'shared/hooks/useAttemptNext'; + +import { + ResourcesResponse, + ResourceFilter, + UnifiedResource, +} from 'teleport/services/agents'; +import { UrlResourcesParams } from 'teleport/config'; + +/** + * Supports fetching more data from the server when more data is available. Pass + * a `fetchFunc` that retrieves a single batch of data. After the initial + * request, the server is expected to return a `startKey` field that denotes the + * next `startKey` to use for the next request. + * + * The hook maintains an invariant that there's only up to one valid + * pending request at all times. Any out-of-order responses are discarded. + * + * This hook is an implementation detail of the `useInfiniteScroll` hook and + * should not be used directly. + */ +export function useKeyBasedPagination({ + fetchFunc, + clusterId, + filter, + initialFetchSize = 30, + fetchMoreSize = 20, +}: Props): State { + const { attempt, setAttempt } = useAttempt(); + const [finished, setFinished] = useState(false); + const [resources, setResources] = useState([]); + const [startKey, setStartKey] = useState(null); + + // Ephemeral state used solely to coordinate fetch calls, doesn't need to + // cause rerenders. + const abortController = useRef(null); + const pendingPromise = useRef> | null>(null); + + // This state is used to recognize when the `clusterId` or `filter` props + // have changed, and reset the overall state of this hook. It's tempting to use a + // `useEffect` here, but doing so can cause unwanted behavior where the previous, + // now stale `fetch` is executed once more before the new one (with the new + // `clusterId` or `filter`) is executed. This is because the `useEffect` is + // executed after the render, and `fetch` is called by an IntersectionObserver + // in `useInfiniteScroll`. If the render includes `useInfiniteScroll`'s `trigger` + // element, the old, stale `fetch` will be called before `useEffect` has a chance + // to run and update the state, and thereby the `fetch` function. + // + // By using the pattern described in this article: + // https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes, + // we can ensure that the state is reset before anything renders, and thereby + // ensure that the new `fetch` function is used. + const [prevClusterId, setPrevClusterId] = useState(clusterId); + const [prevFilter, setPrevFilter] = useState(filter); + + if (prevClusterId !== clusterId || prevFilter !== filter) { + setPrevClusterId(clusterId); + setPrevFilter(filter); + + abortController.current?.abort(); + abortController.current = null; + pendingPromise.current = null; + + setAttempt({ status: '', statusText: '' }); + setFinished(false); + setResources([]); + setStartKey(null); + } + + const fetchInternal = async (force: boolean) => { + if ( + finished || + (!force && + (pendingPromise.current || + attempt.status === 'processing' || + attempt.status === 'failed')) + ) { + return; + } + + try { + setAttempt({ status: 'processing' }); + abortController.current?.abort(); + abortController.current = new AbortController(); + const limit = resources.length > 0 ? fetchMoreSize : initialFetchSize; + const newPromise = fetchFunc( + clusterId, + { + ...filter, + limit, + startKey, + }, + abortController.current.signal + ); + pendingPromise.current = newPromise; + + const res = await newPromise; + + if (pendingPromise.current !== newPromise) { + return; + } + + pendingPromise.current = null; + abortController.current = null; + // Note: even though the old resources appear in this call, this _is_ more + // correct than a standard practice of using a callback form of + // `setState`. This is because, contrary to an "increasing a counter" + // analogy, adding given set of resources to the current set of resources + // strictly depends on the exact set of resources that were there when + // `fetch` was called. This shouldn't make a difference in practice (we + // have other ways to mitigate discrepancies here), but better safe than + // sorry. + setResources([...resources, ...res.agents]); + setStartKey(res.startKey); + if (!res.startKey) { + setFinished(true); + } + setAttempt({ status: 'success' }); + } catch (err) { + // Aborting is not really an error here. + if (isAbortError(err)) { + setAttempt({ status: '', statusText: '' }); + return; + } + setAttempt({ status: 'failed', statusText: err.message }); + } + }; + + const callbackDeps = [ + clusterId, + filter, + startKey, + resources, + finished, + attempt, + ]; + + const fetch = useCallback(() => fetchInternal(false), callbackDeps); + const forceFetch = useCallback(() => fetchInternal(true), callbackDeps); + + return { + fetch, + forceFetch, + attempt, + resources, + finished, + }; +} + +const isAbortError = (err: any): boolean => + (err instanceof DOMException && err.name === 'AbortError') || + (err.cause && isAbortError(err.cause)); + +export type Props = { + fetchFunc: ( + clusterId: string, + params: UrlResourcesParams, + signal?: AbortSignal + ) => Promise>; + clusterId: string; + filter: ResourceFilter; + initialFetchSize?: number; + fetchMoreSize?: number; +}; + +export type State = { + /** + * Attempts to fetch a new batch of data, unless one is already being fetched, + * or the previous fetch resulted with an error. It is intended to be called + * as a mere suggestion to fetch more data and can be called multiple times, + * for example when the user scrolls to the bottom of the page. This is the + * function that you should pass to `useInfiniteScroll` hook. + */ + fetch: () => Promise; + + /** + * Fetches a new batch of data. Cancels a pending request, if there is one. + * Disregards whether error has previously occurred. Intended for using as an + * explicit user's action. Don't call it from `useInfiniteScroll`, or you'll + * risk flooding the server with requests! + */ + forceFetch: () => Promise; + + attempt: Attempt; + resources: T[]; + finished: boolean; +}; diff --git a/web/packages/teleport/src/services/api/api.js b/web/packages/teleport/src/services/api/api.js index 59eff4da7a600..d36fb19d396a1 100644 --- a/web/packages/teleport/src/services/api/api.js +++ b/web/packages/teleport/src/services/api/api.js @@ -71,14 +71,20 @@ const api = { return response .json() .then(json => resolve(json)) - .catch(err => reject(new ApiError(err.message, response))); + .catch(err => + reject(new ApiError(err.message, response, { cause: err })) + ); } else { return response .json() .then(json => reject(new ApiError(parseError(json), response))) - .catch(() => { + .catch(err => { reject( - new ApiError(`${response.status} - ${response.url}`, response) + new ApiError( + `${response.status} - ${response.url}`, + response, + { cause: err } + ) ); }); } diff --git a/web/packages/teleport/src/services/api/parseError.ts b/web/packages/teleport/src/services/api/parseError.ts index fd922ef3b156e..9824830de6e6b 100644 --- a/web/packages/teleport/src/services/api/parseError.ts +++ b/web/packages/teleport/src/services/api/parseError.ts @@ -30,9 +30,9 @@ export default function parseError(json) { export class ApiError extends Error { response: Response; - constructor(message, response: Response) { + constructor(message: string, response: Response, opts?: ErrorOptions) { message = message || 'Unknown error'; - super(message); + super(message, opts); this.response = response; this.name = 'ApiError'; } diff --git a/yarn.lock b/yarn.lock index faafb6fd7bbc3..03304f37e1054 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5304,6 +5304,11 @@ better-opn@^2.1.1: dependencies: open "^7.0.3" +bezier-easing@^2.1.0: + version "2.1.0" + resolved "https://registry.yarnpkg.com/bezier-easing/-/bezier-easing-2.1.0.tgz#c04dfe8b926d6ecaca1813d69ff179b7c2025d86" + integrity sha512-gbIqZ/eslnUFC1tjEvtz0sgx+xTK20wDnYMIA27VA04R7w6xxXQPZDbibjA9DTWZRA2CXtwHykkVzlCaAJAZig== + big-integer@^1.6.7: version "1.6.51" resolved "https://registry.yarnpkg.com/big-integer/-/big-integer-1.6.51.tgz#0df92a5d9880560d3ff2d5fd20245c889d130686" @@ -6379,6 +6384,11 @@ css-loader@^5.0.1: schema-utils "^3.0.0" semver "^7.3.5" +css-mediaquery@^0.1.2: + version "0.1.2" + resolved "https://registry.yarnpkg.com/css-mediaquery/-/css-mediaquery-0.1.2.tgz#6a2c37344928618631c54bd33cedd301da18bea0" + integrity sha512-COtn4EROW5dBGlE/4PiKnh6rZpAPxDeFLaEEwt4i10jpDMFt2EhQGS79QmmrO+iKCHv0PU/HrOWEhijFd1x99Q== + css-select@^4.1.3: version "4.1.3" resolved "https://registry.yarnpkg.com/css-select/-/css-select-4.1.3.tgz#a70440f70317f2669118ad74ff105e65849c7067" @@ -10267,6 +10277,14 @@ js-yaml@^4.1.0: dependencies: argparse "^2.0.1" +jsdom-testing-mocks@^1.9.0: + version "1.9.0" + resolved "https://registry.yarnpkg.com/jsdom-testing-mocks/-/jsdom-testing-mocks-1.9.0.tgz#81ca7f5630cafe5d2084a02afaad8013f0df72db" + integrity sha512-NsuAqHFi0j4FcxIzSp8NOBNB+ln2T5PvJ0ShFgEXwMnTP2K68dRv5mJE/yJadbMAhzYqUL85Aj+cjhG9lHGapQ== + dependencies: + bezier-easing "^2.1.0" + css-mediaquery "^0.1.2" + jsdom@^16.6.0: version "16.7.0" resolved "https://registry.yarnpkg.com/jsdom/-/jsdom-16.7.0.tgz#918ae71965424b197c819f8183a754e18977b710"