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"