From 5411b8abd65a3f5cd637258bd51c2dccf9096d81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 6 Apr 2023 12:58:13 +0200 Subject: [PATCH 01/19] Move tshd test helpers to a better location --- .../tshd/testHelpers.ts} | 16 +--------- .../ui/Search/pickers/ActionPicker.story.tsx | 8 ++--- .../teleterm/src/ui/Search/testHelpers.ts | 29 +++++++++++++++++++ .../teleterm/src/ui/Search/useSearch.test.tsx | 11 ++++--- 4 files changed, 39 insertions(+), 25 deletions(-) rename web/packages/teleterm/src/{ui/Search/searchResultTestHelpers.ts => services/tshd/testHelpers.ts} (79%) create mode 100644 web/packages/teleterm/src/ui/Search/testHelpers.ts diff --git a/web/packages/teleterm/src/ui/Search/searchResultTestHelpers.ts b/web/packages/teleterm/src/services/tshd/testHelpers.ts similarity index 79% rename from web/packages/teleterm/src/ui/Search/searchResultTestHelpers.ts rename to web/packages/teleterm/src/services/tshd/testHelpers.ts index af48315d466d1..2221fa7fb74ab 100644 --- a/web/packages/teleterm/src/ui/Search/searchResultTestHelpers.ts +++ b/web/packages/teleterm/src/services/tshd/testHelpers.ts @@ -14,9 +14,7 @@ * limitations under the License. */ -import { ResourceSearchResult } from './searchResult'; - -import type * as tsh from 'teleterm/services/tshd/types'; +import type * as tsh from './types'; export const makeServer = (props: Partial): tsh.Server => ({ uri: '/clusters/teleport-local/servers/178ef081-259b-4aa5-a018-449b5ea7e694', @@ -49,15 +47,3 @@ export const makeKube = (props: Partial): tsh.Kube => ({ export const makeLabelsList = (labels: Record): tsh.Label[] => Object.entries(labels).map(([name, value]) => ({ name, value })); - -export const makeResourceResult = ( - props: Partial & { - kind: ResourceSearchResult['kind']; - resource: ResourceSearchResult['resource']; - } -): ResourceSearchResult => ({ - score: 0, - labelMatches: [], - resourceMatches: [], - ...props, -}); diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx index 0f7f451dd0bff..561bd12ae49b0 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx @@ -18,15 +18,15 @@ import React from 'react'; import { makeSuccessAttempt } from 'shared/hooks/useAsync'; import { routing } from 'teleterm/ui/uri'; - -import { SearchResult } from '../searchResult'; import { makeDatabase, makeKube, - makeResourceResult, makeServer, makeLabelsList, -} from '../searchResultTestHelpers'; +} from 'teleterm/services/tshd/testHelpers'; + +import { SearchResult } from '../searchResult'; +import { makeResourceResult } from '../testHelpers'; import { ComponentMap, NoResultsItem, TypeToSearchItem } from './ActionPicker'; import { ResultList } from './ResultList'; diff --git a/web/packages/teleterm/src/ui/Search/testHelpers.ts b/web/packages/teleterm/src/ui/Search/testHelpers.ts new file mode 100644 index 0000000000000..3499869e95d98 --- /dev/null +++ b/web/packages/teleterm/src/ui/Search/testHelpers.ts @@ -0,0 +1,29 @@ +/** + * 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 { ResourceSearchResult } from './searchResult'; + +export const makeResourceResult = ( + props: Partial & { + kind: ResourceSearchResult['kind']; + resource: ResourceSearchResult['resource']; + } +): ResourceSearchResult => ({ + score: 0, + labelMatches: [], + resourceMatches: [], + ...props, +}); diff --git a/web/packages/teleterm/src/ui/Search/useSearch.test.tsx b/web/packages/teleterm/src/ui/Search/useSearch.test.tsx index 58740912e6a22..9ee2e3f8f868d 100644 --- a/web/packages/teleterm/src/ui/Search/useSearch.test.tsx +++ b/web/packages/teleterm/src/ui/Search/useSearch.test.tsx @@ -15,21 +15,20 @@ */ import React from 'react'; - import { renderHook } from '@testing-library/react-hooks'; import { ServerUri } from 'teleterm/ui/uri'; import { MockAppContext } from 'teleterm/ui/fixtures/mocks'; import { SearchResult } from 'teleterm/ui/services/resources'; - -import { MockAppContextProvider } from '../fixtures/MockAppContextProvider'; - import { - makeResourceResult, makeServer, makeKube, makeLabelsList, -} from './searchResultTestHelpers'; +} from 'teleterm/services/tshd/testHelpers'; + +import { MockAppContextProvider } from '../fixtures/MockAppContextProvider'; + +import { makeResourceResult } from './testHelpers'; import { rankResults, useResourceSearch } from './useSearch'; describe('rankResults', () => { From 4b72f63bffcbc7cd442c19382987d38694950a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 6 Apr 2023 13:02:14 +0200 Subject: [PATCH 02/19] Support passing no props to tshd test helpers --- web/packages/teleterm/src/services/tshd/testHelpers.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/web/packages/teleterm/src/services/tshd/testHelpers.ts b/web/packages/teleterm/src/services/tshd/testHelpers.ts index 2221fa7fb74ab..051d6a8f38fe6 100644 --- a/web/packages/teleterm/src/services/tshd/testHelpers.ts +++ b/web/packages/teleterm/src/services/tshd/testHelpers.ts @@ -16,7 +16,7 @@ import type * as tsh from './types'; -export const makeServer = (props: Partial): tsh.Server => ({ +export const makeServer = (props: Partial = {}): tsh.Server => ({ uri: '/clusters/teleport-local/servers/178ef081-259b-4aa5-a018-449b5ea7e694', tunnel: false, name: '178ef081-259b-4aa5-a018-449b5ea7e694', @@ -26,7 +26,9 @@ export const makeServer = (props: Partial): tsh.Server => ({ ...props, }); -export const makeDatabase = (props: Partial): tsh.Database => ({ +export const makeDatabase = ( + props: Partial = {} +): tsh.Database => ({ uri: '/clusters/teleport-local/dbs/foo', name: 'foo', protocol: 'postgres', @@ -38,7 +40,7 @@ export const makeDatabase = (props: Partial): tsh.Database => ({ ...props, }); -export const makeKube = (props: Partial): tsh.Kube => ({ +export const makeKube = (props: Partial = {}): tsh.Kube => ({ name: 'foo', labelsList: [], uri: '/clusters/bar/kubes/foo', From 1ddfeda10b7d85e827c05d737be3fb7b30698917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 6 Apr 2023 13:02:40 +0200 Subject: [PATCH 03/19] Refactor ResourcesService getServerByHostname tests --- .../resources/resourcesService.test.ts | 128 +++++++++--------- 1 file changed, 62 insertions(+), 66 deletions(-) diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts index 64bda031c21d4..f66e56cc9f259 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts @@ -14,79 +14,75 @@ * limitations under the License. */ +import { makeServer } from 'teleterm/services/tshd/testHelpers'; + import { AmbiguousHostnameError, ResourcesService } from './resourcesService'; import type * as tsh from 'teleterm/services/tshd/types'; -const server: tsh.Server = { - uri: '/clusters/bar/servers/foo', - tunnel: false, - name: 'foo', - hostname: 'foo', - addr: 'localhost', - labelsList: [], -}; - -const getServerByHostnameTests: Array< - { - name: string; - getServersMockedValue: Awaited>; - } & ( - | { expectedServer: tsh.Server; expectedErr?: never } - | { expectedErr: any; expectedServer?: never } - ) -> = [ - { - name: 'returns a server when the hostname matches a single server', - getServersMockedValue: { - agentsList: [server], - totalCount: 1, - startKey: 'foo', +describe('getServerByHostname', () => { + const server: tsh.Server = makeServer(); + const getServerByHostnameTests: Array< + { + name: string; + getServersMockedValue: Awaited>; + } & ( + | { expectedServer: tsh.Server; expectedErr?: never } + | { expectedErr: any; expectedServer?: never } + ) + > = [ + { + name: 'returns a server when the hostname matches a single server', + getServersMockedValue: { + agentsList: [server], + totalCount: 1, + startKey: 'foo', + }, + expectedServer: server, }, - expectedServer: server, - }, - { - name: 'throws an error when the hostname matches multiple servers', - getServersMockedValue: { - agentsList: [server, server], - totalCount: 2, - startKey: 'foo', + { + name: 'throws an error when the hostname matches multiple servers', + getServersMockedValue: { + agentsList: [server, server], + totalCount: 2, + startKey: 'foo', + }, + expectedErr: AmbiguousHostnameError, }, - expectedErr: AmbiguousHostnameError, - }, - { - name: 'returns nothing if the hostname does not match any servers', - getServersMockedValue: { - agentsList: [], - totalCount: 0, - startKey: 'foo', + { + name: 'returns nothing if the hostname does not match any servers', + getServersMockedValue: { + agentsList: [], + totalCount: 0, + startKey: 'foo', + }, + expectedServer: undefined, }, - expectedServer: undefined, - }, -]; -test.each(getServerByHostnameTests)( - 'getServerByHostname $name', - async ({ getServersMockedValue, expectedServer, expectedErr }) => { - const tshClient: Partial = { - getServers: jest.fn().mockResolvedValueOnce(getServersMockedValue), - }; - const service = new ResourcesService(tshClient as tsh.TshClient); + ]; + test.each(getServerByHostnameTests)( + '$name', + async ({ getServersMockedValue, expectedServer, expectedErr }) => { + const tshClient: Partial = { + getServers: jest.fn().mockResolvedValueOnce(getServersMockedValue), + }; + const service = new ResourcesService(tshClient as tsh.TshClient); - const promise = service.getServerByHostname('/clusters/bar', 'foo'); + const promise = service.getServerByHostname('/clusters/bar', 'foo'); - if (expectedErr) { - // eslint-disable-next-line jest/no-conditional-expect - await expect(promise).rejects.toThrow(expectedErr); - } else { - // eslint-disable-next-line jest/no-conditional-expect - await expect(promise).resolves.toStrictEqual(expectedServer); - } + if (expectedErr) { + // eslint-disable-next-line jest/no-conditional-expect + await expect(promise).rejects.toThrow(expectedErr); + } else { + // eslint-disable-next-line jest/no-conditional-expect + await expect(promise).resolves.toStrictEqual(expectedServer); + } - expect(tshClient.getServers).toHaveBeenCalledWith({ - clusterUri: '/clusters/bar', - query: 'name == "foo"', - limit: 2, - sort: null, - }); - } -); + expect(tshClient.getServers).toHaveBeenCalledWith({ + clusterUri: '/clusters/bar', + query: 'name == "foo"', + limit: 2, + sort: null, + }); + } + ); +}); From bc8fa80b3287d6c89c98e2211ea9d5d4e8d095e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 12 Apr 2023 13:22:54 +0200 Subject: [PATCH 04/19] Move pluralize to shared package --- web/packages/shared/utils/getDurationText.ts | 2 +- web/packages/shared/utils/text.ts | 30 ++++++++++++++++++++ web/packages/teleport/src/lib/util.ts | 17 +++++------ 3 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 web/packages/shared/utils/text.ts diff --git a/web/packages/shared/utils/getDurationText.ts b/web/packages/shared/utils/getDurationText.ts index 14c2b23222d58..41a552a2054dc 100644 --- a/web/packages/shared/utils/getDurationText.ts +++ b/web/packages/shared/utils/getDurationText.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { pluralize } from 'teleport/lib/util'; +import { pluralize } from './text'; export function getDurationText(hrs: number, mins: number, secs: number) { if (!hrs && !mins) { diff --git a/web/packages/shared/utils/text.ts b/web/packages/shared/utils/text.ts new file mode 100644 index 0000000000000..e4772876df209 --- /dev/null +++ b/web/packages/shared/utils/text.ts @@ -0,0 +1,30 @@ +/** + * 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. + */ + +// If you ever need to pluralize a word which cannot be pluralized by appending 's', just add a +// third optional argument which is the pluralized noun. +// https://api.rubyonrails.org/v7.0.4.2/classes/ActionView/Helpers/TextHelper.html#method-i-pluralize + +/** + * pluralize adds an 's' to the given word if num is bigger than 1. + */ +export function pluralize(num: number, word: string) { + if (num > 1) { + return `${word}s`; + } + + return word; +} diff --git a/web/packages/teleport/src/lib/util.ts b/web/packages/teleport/src/lib/util.ts index 83d70301101de..e9409ea11ae45 100644 --- a/web/packages/teleport/src/lib/util.ts +++ b/web/packages/teleport/src/lib/util.ts @@ -16,6 +16,15 @@ limitations under the License. import { AuthType } from 'teleport/services/user'; +// TODO(ravicious): Refactor teleport.e and teleterm.e to import pluralize from shared/utils/text +// and remove this temporary reexport. +export { + /** + * @deprecated Import pluralize from `shared/utils/text` instead. + */ + pluralize, +} from 'shared/utils/text'; + export const openNewTab = (url: string) => { const element = document.createElement('a'); element.setAttribute('href', `${url}`); @@ -27,14 +36,6 @@ export const openNewTab = (url: string) => { document.body.removeChild(element); }; -export function pluralize(num: number, word: string) { - if (num > 1) { - return `${word}s`; - } - - return word; -} - // Adapted from https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest#converting_a_digest_to_a_hex_string export async function Sha256Digest( message: string, From 173624928d678f976d6eb203766f3fcc44cb127e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 7 Apr 2023 15:20:49 +0200 Subject: [PATCH 05/19] SearchContext: Rename `opened` to `isOpen` --- .../teleterm/src/ui/Search/SearchBar.test.tsx | 4 ++-- .../teleterm/src/ui/Search/SearchBar.tsx | 10 +++++----- .../teleterm/src/ui/Search/SearchContext.tsx | 18 +++++++++--------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx index 80d1f7e17ee30..a6847e837f18d 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx @@ -48,7 +48,7 @@ it('does not display empty results copy after selecting two filters', () => { inputValue: '', setFilter: () => {}, removeFilter: () => {}, - opened: true, + isOpen: true, open: () => {}, close: () => {}, closeAndResetInput: () => {}, @@ -143,7 +143,7 @@ const mockedSearchContext = { filters: [], setFilter: () => {}, removeFilter: () => {}, - opened: true, + isOpen: true, open: () => {}, close: () => {}, closeAndResetInput: () => {}, diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.tsx index 6fb2d8b88fce8..6c1ae5a51df65 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.tsx @@ -55,7 +55,7 @@ function SearchBar() { inputValue, onInputValueChange, inputRef, - opened, + isOpen, open, close, } = useSearchContext(); @@ -74,11 +74,11 @@ function SearchBar() { close(); } }; - if (opened) { + if (isOpen) { window.addEventListener('click', onClickOutside); return () => window.removeEventListener('click', onClickOutside); } - }, [close, opened]); + }, [close, isOpen]); function handleOnFocus(e: React.FocusEvent) { open(e.relatedTarget); @@ -117,13 +117,13 @@ function SearchBar() { ref={containerRef} onFocus={handleOnFocus} > - {!opened && ( + {!isOpen && ( <> {getAccelerator(OPEN_SEARCH_BAR_SHORTCUT_ACTION)} )} - {opened && ( + {isOpen && ( } diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.tsx index 8885cbc49b3f7..da6ed930f8d3e 100644 --- a/web/packages/teleterm/src/ui/Search/SearchContext.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchContext.tsx @@ -31,14 +31,14 @@ import { actionPicker, SearchPicker } from './pickers/pickers'; export interface SearchContext { inputRef: MutableRefObject; inputValue: string; - opened: boolean; filters: SearchFilter[]; activePicker: SearchPicker; onInputValueChange(value: string): void; changeActivePicker(picker: SearchPicker): void; + isOpen: boolean; + open(fromElement?: Element): void; close(): void; closeAndResetInput(): void; - open(fromElement?: Element): void; resetInput(): void; setFilter(filter: SearchFilter): void; removeFilter(filter: SearchFilter): void; @@ -49,7 +49,7 @@ const SearchContext = createContext(null); export const SearchContextProvider: FC = props => { const previouslyActive = useRef(); const inputRef = useRef(); - const [opened, setOpened] = useState(false); + const [isOpen, setIsOpen] = useState(false); const [inputValue, setInputValue] = useState(''); const [activePicker, setActivePicker] = useState(actionPicker); // TODO(ravicious): Consider using another data structure for search filters as we know that we @@ -67,7 +67,7 @@ export const SearchContextProvider: FC = props => { } const close = useCallback(() => { - setOpened(false); + setIsOpen(false); setActivePicker(actionPicker); if (previouslyActive.current instanceof HTMLElement) { previouslyActive.current.focus(); @@ -84,11 +84,11 @@ export const SearchContextProvider: FC = props => { }, []); function open(fromElement?: Element): void { - if (opened) { + if (isOpen) { return; } previouslyActive.current = fromElement || document.activeElement; - setOpened(true); + setIsOpen(true); } function setFilter(filter: SearchFilter) { @@ -121,11 +121,11 @@ export const SearchContextProvider: FC = props => { filters, setFilter, removeFilter, - close, - closeAndResetInput, resetInput, - opened, + isOpen, open, + close, + closeAndResetInput, }} children={props.children} /> From 5b66b8bfc073903e7bbac359ef44e36e3c926b8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 13 Apr 2023 11:06:38 +0200 Subject: [PATCH 06/19] ActionPicker story: Show auxiliary items in a separate column --- .../ui/Search/pickers/ActionPicker.story.tsx | 155 ++++++++++-------- 1 file changed, 87 insertions(+), 68 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx index 561bd12ae49b0..5f67df1edb0aa 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx @@ -17,6 +17,8 @@ import React from 'react'; import { makeSuccessAttempt } from 'shared/hooks/useAsync'; +import { Flex } from 'design'; + import { routing } from 'teleterm/ui/uri'; import { makeDatabase, @@ -41,46 +43,52 @@ const clusterUri: uri.ClusterUri = '/clusters/teleport-local'; const longClusterUri: uri.ClusterUri = '/clusters/teleport-very-long-cluster-name-with-uuid-2f96e498-88ec-442f-a25b-569fa915041c'; -export const Items = () => { +export const Items = (props: { maxWidth: string }) => { + const { maxWidth = '600px' } = props; + return ( -
+
* { + max-height: unset; + } + `} + > + +
+
* { - max-height: unset; - } - `} - > - -
+ > * { + max-height: unset; + } + `} + > + +
+ ); }; -export const ItemsNarrow = () => { - return ( -
* { - max-height: unset; - } - `} - > - -
- ); +export const ItemsNarrow = () => { + return ; }; -const List = () => { +const SearchResultItems = () => { const searchResults: SearchResult[] = [ makeResourceResult({ kind: 'server', @@ -306,41 +314,52 @@ const List = () => { const attempt = makeSuccessAttempt(searchResults); return ( - <> - - attempts={[attempt]} - onPick={() => {}} - onBack={() => {}} - render={searchResult => { - const Component = ComponentMap[searchResult.kind]; + + attempts={[attempt]} + onPick={() => {}} + onBack={() => {}} + render={searchResult => { + const Component = ComponentMap[searchResult.kind]; - return { - key: - searchResult.kind !== 'resource-type-filter' - ? searchResult.resource.uri - : searchResult.resource, - Component: ( - - ), - }; - }} - /> - - - + return { + key: + searchResult.kind !== 'resource-type-filter' + ? searchResult.resource.uri + : searchResult.resource, + Component: ( + + ), + }; + }} + /> ); }; + +const AuxiliaryItems = () => ( + + onPick={() => {}} + onBack={() => {}} + render={() => null} + attempts={[]} + ExtraComponent={ + <> + + + + } + /> +); From ab486b2c26af299127129482d4c8be494bc03d72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 13 Apr 2023 12:04:00 +0200 Subject: [PATCH 07/19] ActionPicker: Split getClusterName into two functions getClusterName used to not return the name of the cluster if there's only a single cluster present. Some places needed to get the cluster name no matter what, such as the modal with resource errors that will be added to ActionPicker. --- .../ui/Search/pickers/ActionPicker.story.tsx | 2 +- .../src/ui/Search/pickers/ActionPicker.tsx | 22 ++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx index 5f67df1edb0aa..25fb19be3bee0 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx @@ -329,7 +329,7 @@ const SearchResultItems = () => { Component: ( ), }; diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx index 6f1b73c6731a1..a2a40d481ac25 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx @@ -62,16 +62,18 @@ export function ActionPicker(props: { input: ReactElement }) { const getClusterName = useCallback( (resourceUri: uri.ClusterOrResourceUri) => { - if (totalCountOfClusters === 1) { - return; - } - const clusterUri = uri.routing.ensureClusterUri(resourceUri); const cluster = clustersService.findCluster(clusterUri); return cluster ? cluster.name : uri.routing.parseClusterName(resourceUri); }, - [clustersService, totalCountOfClusters] + [clustersService] + ); + + const getOptionalClusterName = useCallback( + (resourceUri: uri.ClusterOrResourceUri) => + totalCountOfClusters === 1 ? undefined : getClusterName(resourceUri), + [getClusterName, totalCountOfClusters] ); const onPick = useCallback( @@ -166,7 +168,7 @@ export function ActionPicker(props: { input: ReactElement }) { Component: ( ), }; @@ -207,7 +209,7 @@ export const ComponentMap: Record< type SearchResultItem = { searchResult: T; - getClusterName: (uri: uri.ResourceUri) => string; + getOptionalClusterName: (uri: uri.ResourceUri) => string; }; function Item( @@ -288,7 +290,7 @@ export function ServerItem(props: SearchResultItem) { - {props.getClusterName(server.uri)} + {props.getOptionalClusterName(server.uri)} @@ -362,7 +364,7 @@ export function DatabaseItem(props: SearchResultItem) { - {props.getClusterName(db.uri)} + {props.getOptionalClusterName(db.uri)} @@ -401,7 +403,7 @@ export function KubeItem(props: SearchResultItem) { - {props.getClusterName(searchResult.resource.uri)} + {props.getOptionalClusterName(searchResult.resource.uri)} From 3c335448be479824fe37f6d24cefe074e927dd87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 6 Apr 2023 15:09:39 +0200 Subject: [PATCH 08/19] Refactor resource search to use Promise.allSettled * useSearchAttempts has been renamed to useActionAttempts * useActionAttempts returns resourceSearchAttempt in order to supply errors from ResourcesService.searchResources to ActionPicker. --- .../teleterm/src/ui/Search/SearchBar.test.tsx | 35 +++-- .../src/ui/Search/pickers/ActionPicker.tsx | 10 +- ...SearchAttempts.ts => useActionAttempts.ts} | 11 +- .../teleterm/src/ui/Search/useSearch.test.tsx | 2 +- .../teleterm/src/ui/Search/useSearch.ts | 48 +++++-- .../resources/resourcesService.test.ts | 123 +++++++++++++++++- .../ui/services/resources/resourcesService.ts | 61 ++++++--- 7 files changed, 243 insertions(+), 47 deletions(-) rename web/packages/teleterm/src/ui/Search/pickers/{useSearchAttempts.ts => useActionAttempts.ts} (90%) diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx index a6847e837f18d..c5547a7aeeab4 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx @@ -22,7 +22,7 @@ import { MockAppContext } from 'teleterm/ui/fixtures/mocks'; import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvider'; import * as pickers from './pickers/pickers'; -import * as useSearchAttempts from './pickers/useSearchAttempts'; +import * as useActionAttempts from './pickers/useActionAttempts'; import * as SearchContext from './SearchContext'; import { SearchBarConnected } from './SearchBar'; @@ -33,13 +33,18 @@ it('does not display empty results copy after selecting two filters', () => { draft.rootClusterUri = '/clusters/foo'; }); - const mockAttempts = { + const mockActionAttempts = { filterActionsAttempt: makeSuccessAttempt([]), resourceActionsAttempt: makeSuccessAttempt([]), + resourceSearchAttempt: makeSuccessAttempt({ + results: [], + errors: [], + search: '', + }), }; jest - .spyOn(useSearchAttempts, 'useSearchAttempts') - .mockImplementation(() => mockAttempts); + .spyOn(useActionAttempts, 'useActionAttempts') + .mockImplementation(() => mockActionAttempts); jest.spyOn(SearchContext, 'useSearchContext').mockImplementation(() => ({ filters: [ { filter: 'cluster', clusterUri: '/clusters/foo' }, @@ -75,13 +80,18 @@ it('does display empty results copy after providing search query for which there draft.rootClusterUri = '/clusters/foo'; }); - const mockAttempts = { + const mockActionAttempts = { filterActionsAttempt: makeSuccessAttempt([]), resourceActionsAttempt: makeSuccessAttempt([]), + resourceSearchAttempt: makeSuccessAttempt({ + results: [], + errors: [], + search: '', + }), }; jest - .spyOn(useSearchAttempts, 'useSearchAttempts') - .mockImplementation(() => mockAttempts); + .spyOn(useActionAttempts, 'useActionAttempts') + .mockImplementation(() => mockActionAttempts); jest .spyOn(SearchContext, 'useSearchContext') .mockImplementation(() => mockedSearchContext); @@ -114,13 +124,18 @@ it('does display empty results copy and excluded clusters after providing search draft.rootClusterUri = '/clusters/foo'; }); - const mockAttempts = { + const mockActionAttempts = { filterActionsAttempt: makeSuccessAttempt([]), resourceActionsAttempt: makeSuccessAttempt([]), + resourceSearchAttempt: makeSuccessAttempt({ + results: [], + errors: [], + search: '', + }), }; jest - .spyOn(useSearchAttempts, 'useSearchAttempts') - .mockImplementation(() => mockAttempts); + .spyOn(useActionAttempts, 'useActionAttempts') + .mockImplementation(() => mockActionAttempts); jest .spyOn(SearchContext, 'useSearchContext') .mockImplementation(() => mockedSearchContext); diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx index a2a40d481ac25..083a6d85c4363 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx @@ -38,7 +38,7 @@ import * as uri from 'teleterm/ui/uri'; import { SearchAction } from '../actions'; import { useSearchContext } from '../SearchContext'; -import { useSearchAttempts } from './useSearchAttempts'; +import { useActionAttempts } from './useActionAttempts'; import { getParameterPicker } from './pickers'; import { ResultList, NonInteractiveItem } from './ResultList'; import { PickerContainer } from './PickerContainer'; @@ -57,7 +57,7 @@ export function ActionPicker(props: { input: ReactElement }) { filters, removeFilter, } = useSearchContext(); - const { filterActionsAttempt, resourceActionsAttempt } = useSearchAttempts(); + const { filterActionsAttempt, resourceActionsAttempt } = useActionAttempts(); const totalCountOfClusters = clustersService.getClusters().length; const getClusterName = useCallback( @@ -130,8 +130,8 @@ export function ActionPicker(props: { input: ReactElement }) { let ExtraComponent = null; // The order of attempts is important. Filter actions should be displayed before resource actions. - const attempts = [filterActionsAttempt, resourceActionsAttempt]; - const attemptsHaveFinishedWithoutActions = attempts.every( + const actionAttempts = [filterActionsAttempt, resourceActionsAttempt]; + const attemptsHaveFinishedWithoutActions = actionAttempts.every( a => hasFinished(a) && a.data.length === 0 ); const noRemainingFilters = @@ -155,7 +155,7 @@ export function ActionPicker(props: { input: ReactElement }) { {props.input} - attempts={attempts} + attempts={actionAttempts} onPick={onPick} onBack={close} render={item => { diff --git a/web/packages/teleterm/src/ui/Search/pickers/useSearchAttempts.ts b/web/packages/teleterm/src/ui/Search/pickers/useActionAttempts.ts similarity index 90% rename from web/packages/teleterm/src/ui/Search/pickers/useSearchAttempts.ts rename to web/packages/teleterm/src/ui/Search/pickers/useActionAttempts.ts index bd8a366652968..590e5cc15c6d0 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/useSearchAttempts.ts +++ b/web/packages/teleterm/src/ui/Search/pickers/useActionAttempts.ts @@ -33,7 +33,7 @@ import Logger from 'teleterm/logger'; import { useAppContext } from 'teleterm/ui/appContextProvider'; import { useSearchContext } from 'teleterm/ui/Search/SearchContext'; -export function useSearchAttempts() { +export function useActionAttempts() { const searchLogger = useRef(new Logger('search')); const ctx = useAppContext(); // Both states are used by mapToActions. @@ -83,7 +83,14 @@ export function useSearchAttempts() { runResourceSearchDebounced, ]); - return { filterActionsAttempt, resourceActionsAttempt }; + return { + filterActionsAttempt, + resourceActionsAttempt, + // resourceSearchAttempt is the raw version of useResourceSearch attempt that has not been + // mapped to actions. Returning this will allow ActionPicker to inspect errors returned from the + // resource search. + resourceSearchAttempt, + }; } function useDebounce( diff --git a/web/packages/teleterm/src/ui/Search/useSearch.test.tsx b/web/packages/teleterm/src/ui/Search/useSearch.test.tsx index 9ee2e3f8f868d..ddfdb9361ee1b 100644 --- a/web/packages/teleterm/src/ui/Search/useSearch.test.tsx +++ b/web/packages/teleterm/src/ui/Search/useSearch.test.tsx @@ -142,7 +142,7 @@ describe('useResourceSearch', () => { }); jest .spyOn(appContext.resourcesService, 'searchResources') - .mockResolvedValue(servers); + .mockResolvedValue([{ status: 'fulfilled', value: servers }]); const { result } = renderHook(() => useResourceSearch(), { wrapper: ({ children }) => ( diff --git a/web/packages/teleterm/src/ui/Search/useSearch.ts b/web/packages/teleterm/src/ui/Search/useSearch.ts index 42bc538a816cd..499fea7462ceb 100644 --- a/web/packages/teleterm/src/ui/Search/useSearch.ts +++ b/web/packages/teleterm/src/ui/Search/useSearch.ts @@ -46,7 +46,14 @@ export function useResourceSearch() { clustersService.useState(); return useCallback( - async (search: string, restrictions: SearchFilter[]) => { + async ( + search: string, + restrictions: SearchFilter[] + ): Promise<{ + results: resourcesServiceTypes.SearchResult[]; + errors: resourcesServiceTypes.ResourceSearchError[]; + search: string; + }> => { // useResourceSearch has to return _something_ when the input is empty. Imagine this scenario: // // 1. The user types in 'data' into the search bar. @@ -57,7 +64,7 @@ export function useResourceSearch() { // issuing another one for empty input and making `useResourceSearch` return an empty array // in that scenario. if (!search) { - return { results: [], search }; + return { results: [], errors: [], search }; } const clusterSearchFilter = restrictions.find( @@ -76,16 +83,37 @@ export function useResourceSearch() { ) : connectedClusters; - const searchPromises = clustersToSearch.map(cluster => - resourcesService.searchResources( - cluster.uri, - search, - resourceTypeSearchFilter + // ResourcesService.searchResources uses Promise.allSettled so the returned promise will never + // get rejected. + const promiseResults = ( + await Promise.all( + clustersToSearch.map(cluster => + resourcesService.searchResources( + cluster.uri, + search, + resourceTypeSearchFilter + ) + ) ) - ); - const results = (await Promise.all(searchPromises)).flat(); + ).flat(); + + const results: resourcesServiceTypes.SearchResult[] = []; + const errors: resourcesServiceTypes.ResourceSearchError[] = []; + + for (const promiseResult of promiseResults) { + switch (promiseResult.status) { + case 'fulfilled': { + results.push(...promiseResult.value); + break; + } + case 'rejected': { + errors.push(promiseResult.reason); + break; + } + } + } - return { results, search }; + return { results, errors, search }; }, [clustersService, resourcesService] ); diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts index f66e56cc9f259..89b464da2d9d6 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts @@ -14,9 +14,17 @@ * limitations under the License. */ -import { makeServer } from 'teleterm/services/tshd/testHelpers'; +import { + makeDatabase, + makeKube, + makeServer, +} from 'teleterm/services/tshd/testHelpers'; -import { AmbiguousHostnameError, ResourcesService } from './resourcesService'; +import { + AmbiguousHostnameError, + ResourceSearchError, + ResourcesService, +} from './resourcesService'; import type * as tsh from 'teleterm/services/tshd/types'; @@ -86,3 +94,114 @@ describe('getServerByHostname', () => { } ); }); + +describe('searchResources', () => { + it('returns settled promises for each resource type', async () => { + const server = makeServer(); + const db = makeDatabase(); + const kube = makeKube(); + + const tshClient: Partial = { + getServers: jest.fn().mockResolvedValueOnce({ + agentsList: [server], + totalCount: 1, + startKey: '', + }), + getDatabases: jest.fn().mockResolvedValueOnce({ + agentsList: [db], + totalCount: 1, + startKey: '', + }), + getKubes: jest.fn().mockResolvedValueOnce({ + agentsList: [kube], + totalCount: 1, + startKey: '', + }), + }; + const service = new ResourcesService(tshClient as tsh.TshClient); + + const searchResults = await service.searchResources( + '/clusters/foo', + '', + undefined + ); + expect(searchResults).toHaveLength(3); + + const [actualServers, actualDatabases, actualKubes] = searchResults; + expect(actualServers).toEqual({ + status: 'fulfilled', + value: [{ kind: 'server', resource: server }], + }); + expect(actualDatabases).toEqual({ + status: 'fulfilled', + value: [{ kind: 'database', resource: db }], + }); + expect(actualKubes).toEqual({ + status: 'fulfilled', + value: [{ kind: 'kube', resource: kube }], + }); + }); + + it('returns a single item if a filter is supplied', async () => { + const server = makeServer(); + const tshClient: Partial = { + getServers: jest.fn().mockResolvedValueOnce({ + agentsList: [server], + totalCount: 1, + startKey: '', + }), + }; + const service = new ResourcesService(tshClient as tsh.TshClient); + + const searchResults = await service.searchResources('/clusters/foo', '', { + filter: 'resource-type', + resourceType: 'servers', + }); + expect(searchResults).toHaveLength(1); + + const [actualServers] = searchResults; + expect(actualServers).toEqual({ + status: 'fulfilled', + value: [{ kind: 'server', resource: server }], + }); + }); + + it('returns a custom error pointing at resource kind and cluster when an underlying promise gets rejected', async () => { + const expectedCause = new Error('oops'); + const tshClient: Partial = { + getServers: jest.fn().mockRejectedValueOnce(expectedCause), + getDatabases: jest.fn().mockRejectedValueOnce(expectedCause), + getKubes: jest.fn().mockRejectedValueOnce(expectedCause), + }; + const service = new ResourcesService(tshClient as tsh.TshClient); + + const searchResults = await service.searchResources( + '/clusters/foo', + '', + undefined + ); + expect(searchResults).toHaveLength(3); + + const [actualServers, actualDatabases, actualKubes] = searchResults; + expect(actualServers).toEqual({ + status: 'rejected', + reason: new ResourceSearchError('/clusters/foo', 'server', expectedCause), + }); + expect(actualDatabases).toEqual({ + status: 'rejected', + reason: new ResourceSearchError( + '/clusters/foo', + 'database', + expectedCause + ), + }); + expect(actualKubes).toEqual({ + status: 'rejected', + reason: new ResourceSearchError('/clusters/foo', 'kube', expectedCause), + }); + + expect((actualServers as PromiseRejectedResult).reason.cause).toEqual( + expectedCause + ); + }); +}); diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts index 2e8a62ffecdeb..239f3f828041d 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts @@ -73,29 +73,37 @@ export class ResourcesService { clusterUri: uri.ClusterUri, search: string, searchFilter: ResourceTypeSearchFilter | undefined - ): Promise { + ): Promise[]> { const params = { search, clusterUri, sort: null, limit: 100 }; const getServers = () => - this.fetchServers(params).then(res => - res.agentsList.map(resource => ({ - kind: 'server' as const, - resource, - })) + this.fetchServers(params).then( + res => + res.agentsList.map(resource => ({ + kind: 'server' as const, + resource, + })), + err => + Promise.reject(new ResourceSearchError(clusterUri, 'server', err)) ); const getDatabases = () => - this.fetchDatabases(params).then(res => - res.agentsList.map(resource => ({ - kind: 'database' as const, - resource, - })) + this.fetchDatabases(params).then( + res => + res.agentsList.map(resource => ({ + kind: 'database' as const, + resource, + })), + err => + Promise.reject(new ResourceSearchError(clusterUri, 'database', err)) ); const getKubes = () => - this.fetchKubes(params).then(res => - res.agentsList.map(resource => ({ - kind: 'kube' as const, - resource, - })) + this.fetchKubes(params).then( + res => + res.agentsList.map(resource => ({ + kind: 'kube' as const, + resource, + })), + err => Promise.reject(new ResourceSearchError(clusterUri, 'kube', err)) ); const promises = searchFilter @@ -106,7 +114,7 @@ export class ResourcesService { ].filter(Boolean) : [getServers(), getDatabases(), getKubes()]; - return (await Promise.all(promises)).flat(); + return Promise.allSettled(promises); } } @@ -117,6 +125,25 @@ export class AmbiguousHostnameError extends Error { } } +export class ResourceSearchError extends Error { + clusterUri: uri.ClusterUri; + resourceKind: SearchResult['kind']; + + constructor( + clusterUri: uri.ClusterUri, + resourceKind: SearchResult['kind'], + cause: Error + ) { + super( + `Error while fetching resources of type ${resourceKind} from a cluster`, + { cause } + ); + this.name = 'ResourceSearchError'; + this.clusterUri = clusterUri; + this.resourceKind = resourceKind; + } +} + export type SearchResultServer = { kind: 'server'; resource: types.Server }; export type SearchResultDatabase = { kind: 'database'; From f160c98271a16b87d25ca76dab2a1a3997e0f2c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 7 Apr 2023 15:52:12 +0200 Subject: [PATCH 09/19] SearchContext: Implement lockOpen We'll want to display error details in a modal. While the user interacts with the modal, we don't want to close the search bar and reset the results. So instead, we are going to force the search bar to stay open until the user closes the modal. This will use the lockOpen function from this commit. --- .../teleterm/src/ui/Search/SearchBar.test.tsx | 2 + .../src/ui/Search/SearchContext.test.tsx | 102 ++++++++++++++++++ .../teleterm/src/ui/Search/SearchContext.tsx | 30 +++++- 3 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 web/packages/teleterm/src/ui/Search/SearchContext.test.tsx diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx index c5547a7aeeab4..0bf920fa84636 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx @@ -55,6 +55,7 @@ it('does not display empty results copy after selecting two filters', () => { removeFilter: () => {}, isOpen: true, open: () => {}, + lockOpen: async () => {}, close: () => {}, closeAndResetInput: () => {}, resetInput: () => {}, @@ -160,6 +161,7 @@ const mockedSearchContext = { removeFilter: () => {}, isOpen: true, open: () => {}, + lockOpen: async () => {}, close: () => {}, closeAndResetInput: () => {}, resetInput: () => {}, diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx new file mode 100644 index 0000000000000..0a55beafc1a44 --- /dev/null +++ b/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx @@ -0,0 +1,102 @@ +/** + * 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 { SearchContextProvider, useSearchContext } from './SearchContext'; + +describe('lockOpen', () => { + it('prevents the search bar from being closed for the duration of the action', async () => { + const { result } = renderHook(() => useSearchContext(), { + wrapper: ({ children }) => ( + {children} + ), + }); + act(() => { + result.current.open(); + }); + + let resolveAction: (value?: unknown) => void; + const action = new Promise(resolve => { + resolveAction = resolve; + }); + + let lockOpenPromise: Promise; + act(() => { + lockOpenPromise = result.current.lockOpen(action); + }); + + // Closing while the search bar is locked open should be a noop. + act(() => { + result.current.close(); + }); + expect(result.current.isOpen).toBe(true); + + await act(async () => { + resolveAction(); + await lockOpenPromise; + }); + + // The search bar should be no longer locked open, so close should behave as expected. + act(() => { + result.current.close(); + }); + expect(result.current.isOpen).toBe(false); + }); + + it('prevents the search bar from being closed even when the action rejects', async () => { + const { result } = renderHook(() => useSearchContext(), { + wrapper: ({ children }) => ( + {children} + ), + }); + act(() => { + result.current.open(); + }); + + let rejectAction: () => void; + const action = new Promise((resolve, reject) => { + rejectAction = reject; + }); + + let lockOpenPromise: Promise; + act(() => { + lockOpenPromise = result.current.lockOpen(action); + }); + + // Closing while the search bar is locked open should be a noop. + act(() => { + result.current.close(); + }); + expect(result.current.isOpen).toBe(true); + + await act(async () => { + rejectAction(); + try { + await lockOpenPromise; + } catch { + // Ignore the error. + } + }); + + // The search bar should be no longer locked open, so close should behave as expected. + act(() => { + result.current.close(); + }); + expect(result.current.isOpen).toBe(false); + }); +}); diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.tsx index da6ed930f8d3e..2b8127fc035e2 100644 --- a/web/packages/teleterm/src/ui/Search/SearchContext.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchContext.tsx @@ -37,6 +37,7 @@ export interface SearchContext { changeActivePicker(picker: SearchPicker): void; isOpen: boolean; open(fromElement?: Element): void; + lockOpen(action: Promise): Promise; close(): void; closeAndResetInput(): void; resetInput(): void; @@ -50,6 +51,7 @@ export const SearchContextProvider: FC = props => { const previouslyActive = useRef(); const inputRef = useRef(); const [isOpen, setIsOpen] = useState(false); + const [isLockedOpen, setIsLockedOpen] = useState(false); const [inputValue, setInputValue] = useState(''); const [activePicker, setActivePicker] = useState(actionPicker); // TODO(ravicious): Consider using another data structure for search filters as we know that we @@ -67,17 +69,25 @@ export const SearchContextProvider: FC = props => { } const close = useCallback(() => { + if (isLockedOpen) { + return; + } + setIsOpen(false); setActivePicker(actionPicker); if (previouslyActive.current instanceof HTMLElement) { previouslyActive.current.focus(); } - }, []); + }, [isLockedOpen]); const closeAndResetInput = useCallback(() => { + if (isLockedOpen) { + return; + } + close(); setInputValue(''); - }, [close]); + }, [isLockedOpen, close]); const resetInput = useCallback(() => { setInputValue(''); @@ -91,6 +101,21 @@ export const SearchContextProvider: FC = props => { setIsOpen(true); } + /** + * lockOpen forces the search bar to stay open for the duration of the action. + * This is useful in situations where want the search bar to not close when the user interacts + * with modals shown from the search bar. + */ + async function lockOpen(action: Promise): Promise { + setIsLockedOpen(true); + + try { + await action; + } finally { + setIsLockedOpen(false); + } + } + function setFilter(filter: SearchFilter) { // UI prevents adding more than one filter of the same type setFilters(prevState => [...prevState, filter]); @@ -124,6 +149,7 @@ export const SearchContextProvider: FC = props => { resetInput, isOpen, open, + lockOpen, close, closeAndResetInput, }} From 9da3c19e14331818bf1f559ffe3a9e9a5d1f4bf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 13 Apr 2023 12:00:59 +0200 Subject: [PATCH 10/19] Add modal for showing resource search errors --- .../teleterm/src/ui/ModalsHost/ModalsHost.tsx | 21 ++++- .../ui/Search/ResourceSearchErrors.story.tsx | 77 +++++++++++++++++ .../src/ui/Search/ResourceSearchErrors.tsx | 84 +++++++++++++++++++ .../src/ui/services/modals/modalsService.ts | 11 +++ .../ui/services/resources/resourcesService.ts | 22 ++++- 5 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 web/packages/teleterm/src/ui/Search/ResourceSearchErrors.story.tsx create mode 100644 web/packages/teleterm/src/ui/Search/ResourceSearchErrors.tsx diff --git a/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx b/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx index dd420c49cf784..1cebd130e3c50 100644 --- a/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx +++ b/web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx @@ -23,6 +23,8 @@ import { DocumentsReopen } from 'teleterm/ui/DocumentsReopen'; import { Dialog } from 'teleterm/ui/services/modals'; import ClusterLogout from '../ClusterLogout/ClusterLogout'; +import { ResourceSearchErrors } from '../Search/ResourceSearchErrors'; +import { assertUnreachable } from '../utils'; import { UsageData } from './modals/UsageData'; import { UserJobRole } from './modals/UserJobRole'; @@ -117,8 +119,25 @@ function renderDialog(dialog: Dialog, handleClose: () => void) { ); } - default: { + case 'resource-search-errors': { + return ( + { + handleClose(); + dialog.onCancel(); + }} + /> + ); + } + + case 'none': { return null; } + + default: { + return assertUnreachable(dialog); + } } } diff --git a/web/packages/teleterm/src/ui/Search/ResourceSearchErrors.story.tsx b/web/packages/teleterm/src/ui/Search/ResourceSearchErrors.story.tsx new file mode 100644 index 0000000000000..8b20cc3857901 --- /dev/null +++ b/web/packages/teleterm/src/ui/Search/ResourceSearchErrors.story.tsx @@ -0,0 +1,77 @@ +/** + * 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 { routing } from 'teleterm/ui/uri'; +import { ResourceSearchError } from 'teleterm/ui/services/resources'; + +import { ResourceSearchErrors } from './ResourceSearchErrors'; + +export default { + title: 'Teleterm/ModalsHost/ResourceSearchErrors', +}; + +export const Story = () => ( + {}} + errors={[ + new ResourceSearchError( + '/clusters/foo', + 'server', + new Error( + '14 UNAVAILABLE: connection error: desc = "transport: authentication handshake failed: EOF"' + ) + ), + new ResourceSearchError( + '/clusters/bar', + 'database', + new Error( + '2 UNKNOWN: Unable to connect to ssh proxy at teleport.local:443. Confirm connectivity and availability.\n dial tcp: lookup teleport.local: no such host' + ) + ), + new ResourceSearchError( + '/clusters/baz', + 'kube', + new Error( + '14 UNAVAILABLE: connection error: desc = "transport: authentication handshake failed: EOF"' + ) + ), + new ResourceSearchError( + '/clusters/foo', + 'server', + new Error( + '2 UNKNOWN: Unable to connect to ssh proxy at teleport.local:443. Confirm connectivity and availability.\n dial tcp: lookup teleport.local: no such host' + ) + ), + new ResourceSearchError( + '/clusters/baz', + 'kube', + new Error( + '14 UNAVAILABLE: connection error: desc = "transport: authentication handshake failed: EOF"' + ) + ), + new ResourceSearchError( + '/clusters/foo', + 'server', + new Error( + '2 UNKNOWN: Unable to connect to ssh proxy at teleport.local:443. Confirm connectivity and availability.\n dial tcp: lookup teleport.local: no such host' + ) + ), + ]} + /> +); diff --git a/web/packages/teleterm/src/ui/Search/ResourceSearchErrors.tsx b/web/packages/teleterm/src/ui/Search/ResourceSearchErrors.tsx new file mode 100644 index 0000000000000..1d59fa62d9291 --- /dev/null +++ b/web/packages/teleterm/src/ui/Search/ResourceSearchErrors.tsx @@ -0,0 +1,84 @@ +/** + * 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 DialogConfirmation, { + DialogContent, + DialogFooter, + DialogHeader, +} from 'design/DialogConfirmation'; +import { ButtonIcon, ButtonSecondary, Text } from 'design'; +import { Close } from 'design/Icon'; + +import { ResourceSearchError } from 'teleterm/ui/services/resources'; + +import type * as uri from 'teleterm/ui/uri'; + +export function ResourceSearchErrors(props: { + errors: ResourceSearchError[]; + getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string; + onCancel: () => void; +}) { + const formattedErrorText = props.errors + .map(error => error.messageAndCauseWithClusterName(props.getClusterName)) + .join('\n\n'); + + return ( + ({ + maxWidth: '800px', + width: '100%', + })} + > + + + Resource search errors + + + + + + + + 
 props.theme.space[2]}px;
+              background-color: ${props => props.theme.colors.levels.sunken};
+              color: ${props => props.theme.colors.text.primary};
+
+              white-space: pre-wrap;
+              max-height: calc(${props => props.theme.space[6]}px * 10);
+              overflow-y: auto;
+            `}
+          >
+            {formattedErrorText}
+
+ + + + + Close + + + + ); +} diff --git a/web/packages/teleterm/src/ui/services/modals/modalsService.ts b/web/packages/teleterm/src/ui/services/modals/modalsService.ts index b916ec391b289..46c1b6fad7830 100644 --- a/web/packages/teleterm/src/ui/services/modals/modalsService.ts +++ b/web/packages/teleterm/src/ui/services/modals/modalsService.ts @@ -18,9 +18,12 @@ import { useStore } from 'shared/libs/stores'; import * as types from 'teleterm/services/tshd/types'; import { RootClusterUri } from 'teleterm/ui/uri'; +import { ResourceSearchError } from 'teleterm/ui/services/resources'; import { ImmutableStore } from '../immutableStore'; +import type * as uri from 'teleterm/ui/uri'; + type State = { // At most two modals can be displayed at the same time. // The important dialog is displayed above the regular one. This is to avoid losing the state of @@ -183,10 +186,18 @@ export interface DialogUserJobRole { onCancel(): void; } +export interface DialogResourceSearchErrors { + kind: 'resource-search-errors'; + errors: ResourceSearchError[]; + getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string; + onCancel: () => void; +} + export type Dialog = | DialogClusterConnect | DialogClusterLogout | DialogDocumentsReopen | DialogUsageData | DialogUserJobRole + | DialogResourceSearchErrors | DialogNone; diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts index 239f3f828041d..57b4d797dfce4 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts @@ -14,8 +14,9 @@ * limitations under the License. */ -import type { ResourceTypeSearchFilter } from 'teleterm/ui/Search/searchResult'; +import { pluralize } from 'shared/utils/text'; +import type { ResourceTypeSearchFilter } from 'teleterm/ui/Search/searchResult'; import type * as types from 'teleterm/services/tshd/types'; import type * as uri from 'teleterm/ui/uri'; @@ -72,6 +73,8 @@ export class ResourcesService { async searchResources( clusterUri: uri.ClusterUri, search: string, + // TODO(ravicious): Accept just `server | database | kube` as searchFilter here, wrap it in a + // variant of a discriminated union in searchResult.ts. searchFilter: ResourceTypeSearchFilter | undefined ): Promise[]> { const params = { search, clusterUri, sort: null, limit: 100 }; @@ -142,6 +145,23 @@ export class ResourceSearchError extends Error { this.clusterUri = clusterUri; this.resourceKind = resourceKind; } + + messageWithClusterName( + getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string + ) { + const resource = pluralize(2, this.resourceKind); + const cluster = getClusterName(this.clusterUri); + + return `Could not fetch ${resource} from ${cluster}`; + } + + messageAndCauseWithClusterName( + getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string + ) { + return `${this.messageWithClusterName(getClusterName)}:\n${ + this.cause['message'] + }`; + } } export type SearchResultServer = { kind: 'server'; resource: types.Server }; From d0412c5b561c2ba2eeccb69d4a6871b2ebf91cac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 13 Apr 2023 12:22:45 +0200 Subject: [PATCH 11/19] Refactor mockedSearchContext to not be a top-level mutable var --- .../teleterm/src/ui/Search/SearchBar.test.tsx | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx index 0bf920fa84636..cc7e82145cfae 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx @@ -46,23 +46,12 @@ it('does not display empty results copy after selecting two filters', () => { .spyOn(useActionAttempts, 'useActionAttempts') .mockImplementation(() => mockActionAttempts); jest.spyOn(SearchContext, 'useSearchContext').mockImplementation(() => ({ + ...getMockedSearchContext(), filters: [ { filter: 'cluster', clusterUri: '/clusters/foo' }, { filter: 'resource-type', resourceType: 'servers' }, ], inputValue: '', - setFilter: () => {}, - removeFilter: () => {}, - isOpen: true, - open: () => {}, - lockOpen: async () => {}, - close: () => {}, - closeAndResetInput: () => {}, - resetInput: () => {}, - changeActivePicker: () => {}, - onInputValueChange: () => {}, - activePicker: pickers.actionPicker, - inputRef: undefined, })); render( @@ -95,7 +84,7 @@ it('does display empty results copy after providing search query for which there .mockImplementation(() => mockActionAttempts); jest .spyOn(SearchContext, 'useSearchContext') - .mockImplementation(() => mockedSearchContext); + .mockImplementation(getMockedSearchContext); render( @@ -139,7 +128,7 @@ it('does display empty results copy and excluded clusters after providing search .mockImplementation(() => mockActionAttempts); jest .spyOn(SearchContext, 'useSearchContext') - .mockImplementation(() => mockedSearchContext); + .mockImplementation(getMockedSearchContext); render( @@ -154,7 +143,7 @@ it('does display empty results copy and excluded clusters after providing search ); }); -const mockedSearchContext = { +const getMockedSearchContext = () => ({ inputValue: 'foo', filters: [], setFilter: () => {}, @@ -169,4 +158,4 @@ const mockedSearchContext = { onInputValueChange: () => {}, activePicker: pickers.actionPicker, inputRef: undefined, -}; +}); From 0fd39b73b7f10db8db4663c1d530a03c186c69a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 13 Apr 2023 12:06:44 +0200 Subject: [PATCH 12/19] Show an item in search bar with resource search errors --- web/packages/shared/utils/text.ts | 4 + .../teleterm/src/ui/Search/SearchBar.test.tsx | 59 ++++++++++ .../ui/Search/pickers/ActionPicker.story.tsx | 41 ++++++- .../src/ui/Search/pickers/ActionPicker.tsx | 107 +++++++++++++++++- .../src/ui/Search/pickers/ResultList.tsx | 10 +- 5 files changed, 210 insertions(+), 11 deletions(-) diff --git a/web/packages/shared/utils/text.ts b/web/packages/shared/utils/text.ts index e4772876df209..5abf045c2cb96 100644 --- a/web/packages/shared/utils/text.ts +++ b/web/packages/shared/utils/text.ts @@ -28,3 +28,7 @@ export function pluralize(num: number, word: string) { return word; } + +export function firstLowerCase(string: string) { + return string.charAt(0).toLowerCase() + string.slice(1); +} diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx index cc7e82145cfae..ca504d0b26282 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx @@ -20,6 +20,7 @@ import { makeSuccessAttempt } from 'shared/hooks/useAsync'; import { MockAppContext } from 'teleterm/ui/fixtures/mocks'; import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvider'; +import { ResourceSearchError } from 'teleterm/ui/services/resources'; import * as pickers from './pickers/pickers'; import * as useActionAttempts from './pickers/useActionAttempts'; @@ -143,6 +144,64 @@ it('does display empty results copy and excluded clusters after providing search ); }); +it('notifies about resource search errors and allows to display details', () => { + const appContext = new MockAppContext(); + appContext.workspacesService.setState(draft => { + draft.rootClusterUri = '/clusters/foo'; + }); + + const resourceSearchError = new ResourceSearchError( + '/clusters/foo', + 'server', + new Error('whoops') + ); + + const mockActionAttempts = { + filterActionsAttempt: makeSuccessAttempt([]), + resourceActionsAttempt: makeSuccessAttempt([]), + resourceSearchAttempt: makeSuccessAttempt({ + results: [], + errors: [resourceSearchError], + search: '', + }), + }; + jest + .spyOn(useActionAttempts, 'useActionAttempts') + .mockImplementation(() => mockActionAttempts); + const mockedSearchContext = { + ...getMockedSearchContext(), + inputValue: 'foo', + }; + jest + .spyOn(SearchContext, 'useSearchContext') + .mockImplementation(() => mockedSearchContext); + jest.spyOn(appContext.modalsService, 'openRegularDialog'); + jest.spyOn(mockedSearchContext, 'lockOpen'); + + render( + + + + ); + + const results = screen.getByRole('menu'); + expect(results).toHaveTextContent( + 'Some of the search results are incomplete.' + ); + expect(results).toHaveTextContent('Could not fetch servers from foo'); + expect(results).not.toHaveTextContent(resourceSearchError.cause['message']); + + screen.getByText('Show details').click(); + + expect(appContext.modalsService.openRegularDialog).toHaveBeenCalledWith( + expect.objectContaining({ + kind: 'resource-search-errors', + errors: [resourceSearchError], + }) + ); + expect(mockedSearchContext.lockOpen).toHaveBeenCalled(); +}); + const getMockedSearchContext = () => ({ inputValue: 'foo', filters: [], diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx index 25fb19be3bee0..b030c5782bbc3 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.story.tsx @@ -26,11 +26,17 @@ import { makeServer, makeLabelsList, } from 'teleterm/services/tshd/testHelpers'; +import { ResourceSearchError } from 'teleterm/ui/services/resources'; import { SearchResult } from '../searchResult'; import { makeResourceResult } from '../testHelpers'; -import { ComponentMap, NoResultsItem, TypeToSearchItem } from './ActionPicker'; +import { + ComponentMap, + NoResultsItem, + ResourceSearchErrorsItem, + TypeToSearchItem, +} from './ActionPicker'; import { ResultList } from './ResultList'; import type * as uri from 'teleterm/ui/uri'; @@ -358,6 +364,39 @@ const AuxiliaryItems = () => ( }, ]} /> + window.alert('Error details')} + errors={[ + new ResourceSearchError( + '/clusters/foo', + 'server', + new Error( + '14 UNAVAILABLE: connection error: desc = "transport: authentication handshake failed: EOF"' + ) + ), + ]} + /> + window.alert('Error details')} + errors={[ + new ResourceSearchError( + '/clusters/bar', + 'database', + new Error( + '2 UNKNOWN: Unable to connect to ssh proxy at teleport.local:443. Confirm connectivity and availability.\n dial tcp: lookup teleport.local: no such host' + ) + ), + new ResourceSearchError( + '/clusters/foo', + 'server', + new Error( + '14 UNAVAILABLE: connection error: desc = "transport: authentication handshake failed: EOF"' + ) + ), + ]} + /> } diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx index 083a6d85c4363..4d4da8de62867 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx @@ -16,10 +16,18 @@ import React, { ReactElement, useCallback } from 'react'; import styled from 'styled-components'; -import { Box, ButtonPrimary, Flex, Label as DesignLabel, Text } from 'design'; +import { + Box, + ButtonBorder, + ButtonPrimary, + Flex, + Label as DesignLabel, + Text, +} from 'design'; import * as icons from 'design/Icon'; import { Highlight } from 'shared/components/Highlight'; import { hasFinished } from 'shared/hooks/useAsync'; +import { firstLowerCase } from 'shared/utils/text'; import { useAppContext } from 'teleterm/ui/appContextProvider'; import { @@ -34,6 +42,7 @@ import { } from 'teleterm/ui/Search/searchResult'; import * as tsh from 'teleterm/services/tshd/types'; import * as uri from 'teleterm/ui/uri'; +import { ResourceSearchError } from 'teleterm/ui/services/resources'; import { SearchAction } from '../actions'; import { useSearchContext } from '../SearchContext'; @@ -45,11 +54,12 @@ import { PickerContainer } from './PickerContainer'; export function ActionPicker(props: { input: ReactElement }) { const ctx = useAppContext(); - const { clustersService } = ctx; + const { clustersService, modalsService } = ctx; ctx.clustersService.useState(); const { changeActivePicker, + lockOpen, close, inputValue, resetInput, @@ -57,7 +67,11 @@ export function ActionPicker(props: { input: ReactElement }) { filters, removeFilter, } = useSearchContext(); - const { filterActionsAttempt, resourceActionsAttempt } = useActionAttempts(); + const { + filterActionsAttempt, + resourceActionsAttempt, + resourceSearchAttempt, + } = useActionAttempts(); const totalCountOfClusters = clustersService.getClusters().length; const getClusterName = useCallback( @@ -148,6 +162,35 @@ export function ActionPicker(props: { input: ReactElement }) { ExtraComponent = ; } + if ( + resourceSearchAttempt.status === 'success' && + resourceSearchAttempt.data.errors.length > 0 + ) { + const showErrorsInModal = () => { + lockOpen( + new Promise(resolve => { + modalsService.openRegularDialog({ + kind: 'resource-search-errors', + errors: resourceSearchAttempt.data.errors, + getClusterName, + onCancel: () => resolve(undefined), + }); + }) + ); + }; + + ExtraComponent = ( + <> + + {ExtraComponent} + + ); + } + return ( @@ -420,9 +463,7 @@ export function NoResultsItem(props: { clusters: tsh.Cluster[] }) { No matching results found. {excludedClustersCopy && ( - - {excludedClustersCopy} - + {excludedClustersCopy} )} @@ -439,7 +480,61 @@ export function TypeToSearchItem() { ); } +export function ResourceSearchErrorsItem(props: { + errors: ResourceSearchError[]; + getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string; + onShowDetails: () => void; +}) { + const { errors, getClusterName } = props; + + let shortDescription: string; + + if (errors.length === 1) { + const firstErrorMessage = errors[0].messageWithClusterName(getClusterName); + shortDescription = `${firstErrorMessage}.`; + } else { + const allErrorMessages = errors + .map(err => firstLowerCase(err.messageWithClusterName(getClusterName))) + .join(', '); + shortDescription = `Ran into ${errors.length} errors: ${allErrorMessages}.`; + } + + return ( + + + + Some of the search results are incomplete. + + + + + {shortDescription} + + + + Show details + + + + + ); +} + function getExcludedClustersCopy(allClusters: tsh.Cluster[]): string { + // TODO(ravicious): Include leaf clusters. const excludedClusters = allClusters.filter(c => !c.connected); const excludedClustersString = excludedClusters.map(c => c.name).join(', '); if (excludedClusters.length === 0) { diff --git a/web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx b/web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx index 190db995fe7a3..56e1d915a70e4 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx @@ -145,10 +145,7 @@ export const NonInteractiveItem = styled.div` padding: ${props => props.theme.space[2]}px; color: ${props => props.theme.colors.text.contrast}; - background: ${props => - props.$active - ? props.theme.colors.levels.elevated - : props.theme.colors.levels.surface}; + background: ${props => props.theme.colors.levels.surface}; `; const InteractiveItem = styled(NonInteractiveItem)` @@ -157,6 +154,11 @@ const InteractiveItem = styled(NonInteractiveItem)` cursor: pointer; background: ${props => props.theme.colors.levels.elevated}; } + + background: ${props => + props.$active + ? props.theme.colors.levels.elevated + : props.theme.colors.levels.surface}; `; function getNext(selectedIndex = 0, max = 0) { From f3fd1041d5f5e3287fc74cbd95208785a7c8756f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 14 Apr 2023 10:57:16 +0200 Subject: [PATCH 13/19] ResourceSearchError: Add instanceof check to tests, include clusterUri in message --- .../src/ui/services/resources/resourcesService.test.ts | 3 +++ .../teleterm/src/ui/services/resources/resourcesService.ts | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts index 89b464da2d9d6..42b76b9041747 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.test.ts @@ -200,6 +200,9 @@ describe('searchResources', () => { reason: new ResourceSearchError('/clusters/foo', 'kube', expectedCause), }); + expect((actualServers as PromiseRejectedResult).reason).toBeInstanceOf( + ResourceSearchError + ); expect((actualServers as PromiseRejectedResult).reason.cause).toEqual( expectedCause ); diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts index 57b4d797dfce4..40263ca33d044 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts @@ -138,7 +138,7 @@ export class ResourceSearchError extends Error { cause: Error ) { super( - `Error while fetching resources of type ${resourceKind} from a cluster`, + `Error while fetching resources of type ${resourceKind} from cluster ${clusterUri}`, { cause } ); this.name = 'ResourceSearchError'; From 07f4206cc251299b43b81c502fc929990b207939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 14 Apr 2023 11:11:26 +0200 Subject: [PATCH 14/19] Make isLockedOpen into a ref --- .../teleterm/src/ui/Search/SearchContext.tsx | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.tsx index 2b8127fc035e2..8acd81e6b4cef 100644 --- a/web/packages/teleterm/src/ui/Search/SearchContext.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchContext.tsx @@ -51,7 +51,7 @@ export const SearchContextProvider: FC = props => { const previouslyActive = useRef(); const inputRef = useRef(); const [isOpen, setIsOpen] = useState(false); - const [isLockedOpen, setIsLockedOpen] = useState(false); + const isLockedOpenRef = useRef(false); const [inputValue, setInputValue] = useState(''); const [activePicker, setActivePicker] = useState(actionPicker); // TODO(ravicious): Consider using another data structure for search filters as we know that we @@ -69,7 +69,7 @@ export const SearchContextProvider: FC = props => { } const close = useCallback(() => { - if (isLockedOpen) { + if (isLockedOpenRef.current) { return; } @@ -78,16 +78,16 @@ export const SearchContextProvider: FC = props => { if (previouslyActive.current instanceof HTMLElement) { previouslyActive.current.focus(); } - }, [isLockedOpen]); + }, []); const closeAndResetInput = useCallback(() => { - if (isLockedOpen) { + if (isLockedOpenRef.current) { return; } close(); setInputValue(''); - }, [isLockedOpen, close]); + }, [close]); const resetInput = useCallback(() => { setInputValue(''); @@ -105,14 +105,18 @@ export const SearchContextProvider: FC = props => { * lockOpen forces the search bar to stay open for the duration of the action. * This is useful in situations where want the search bar to not close when the user interacts * with modals shown from the search bar. + * + * Calling lockOpen concurrently is not safe, so make sure to structure the UI in such a way + * that it doesn't happen. lockOpen was made for cases like opening a modal where after a modal is + * open it guarantees that the user won't be able to interact with the search bar. */ async function lockOpen(action: Promise): Promise { - setIsLockedOpen(true); + isLockedOpenRef.current = true; try { await action; } finally { - setIsLockedOpen(false); + isLockedOpenRef.current = false; } } From 6a171a4ebb52f06565f5fcd7d01b96f96e26eccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 14 Apr 2023 11:23:22 +0200 Subject: [PATCH 15/19] Use table tests for lockOpen tests --- .../src/ui/Search/SearchContext.test.tsx | 64 ++++++------------- 1 file changed, 20 insertions(+), 44 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx index 0a55beafc1a44..23208851c3a0c 100644 --- a/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx @@ -20,45 +20,26 @@ import { renderHook, act } from '@testing-library/react-hooks'; import { SearchContextProvider, useSearchContext } from './SearchContext'; describe('lockOpen', () => { - it('prevents the search bar from being closed for the duration of the action', async () => { - const { result } = renderHook(() => useSearchContext(), { - wrapper: ({ children }) => ( - {children} - ), - }); - act(() => { - result.current.open(); - }); - - let resolveAction: (value?: unknown) => void; - const action = new Promise(resolve => { - resolveAction = resolve; - }); - - let lockOpenPromise: Promise; - act(() => { - lockOpenPromise = result.current.lockOpen(action); - }); - - // Closing while the search bar is locked open should be a noop. - act(() => { - result.current.close(); - }); - expect(result.current.isOpen).toBe(true); - - await act(async () => { - resolveAction(); - await lockOpenPromise; - }); - - // The search bar should be no longer locked open, so close should behave as expected. - act(() => { - result.current.close(); - }); - expect(result.current.isOpen).toBe(false); + let resolveSuccessAction, rejectFailureAction; + const successAction = new Promise(resolve => { + resolveSuccessAction = resolve; + }); + const failureAction = new Promise((resolve, reject) => { + rejectFailureAction = reject; }); - it('prevents the search bar from being closed even when the action rejects', async () => { + test.each([ + { + name: 'prevents the search bar from being closed for the duration of the action', + action: successAction, + finishAction: resolveSuccessAction, + }, + { + name: 'properly cleans up the ref even after the action fails', + action: failureAction, + finishAction: rejectFailureAction, + }, + ])('$name', async ({ action, finishAction }) => { const { result } = renderHook(() => useSearchContext(), { wrapper: ({ children }) => ( {children} @@ -68,11 +49,6 @@ describe('lockOpen', () => { result.current.open(); }); - let rejectAction: () => void; - const action = new Promise((resolve, reject) => { - rejectAction = reject; - }); - let lockOpenPromise: Promise; act(() => { lockOpenPromise = result.current.lockOpen(action); @@ -85,11 +61,11 @@ describe('lockOpen', () => { expect(result.current.isOpen).toBe(true); await act(async () => { - rejectAction(); + finishAction(); try { await lockOpenPromise; } catch { - // Ignore the error. + // Ignore the error happening when `finishAction` rejects `action`. } }); From 4163cb8a7db04dd02287bfc18859a43d1796cb64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 14 Apr 2023 15:01:24 +0200 Subject: [PATCH 16/19] Revert "Make isLockedOpen into a ref" This reverts commit 07f4206cc251299b43b81c502fc929990b207939. --- .../teleterm/src/ui/Search/SearchContext.tsx | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.tsx index 8acd81e6b4cef..2b8127fc035e2 100644 --- a/web/packages/teleterm/src/ui/Search/SearchContext.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchContext.tsx @@ -51,7 +51,7 @@ export const SearchContextProvider: FC = props => { const previouslyActive = useRef(); const inputRef = useRef(); const [isOpen, setIsOpen] = useState(false); - const isLockedOpenRef = useRef(false); + const [isLockedOpen, setIsLockedOpen] = useState(false); const [inputValue, setInputValue] = useState(''); const [activePicker, setActivePicker] = useState(actionPicker); // TODO(ravicious): Consider using another data structure for search filters as we know that we @@ -69,7 +69,7 @@ export const SearchContextProvider: FC = props => { } const close = useCallback(() => { - if (isLockedOpenRef.current) { + if (isLockedOpen) { return; } @@ -78,16 +78,16 @@ export const SearchContextProvider: FC = props => { if (previouslyActive.current instanceof HTMLElement) { previouslyActive.current.focus(); } - }, []); + }, [isLockedOpen]); const closeAndResetInput = useCallback(() => { - if (isLockedOpenRef.current) { + if (isLockedOpen) { return; } close(); setInputValue(''); - }, [close]); + }, [isLockedOpen, close]); const resetInput = useCallback(() => { setInputValue(''); @@ -105,18 +105,14 @@ export const SearchContextProvider: FC = props => { * lockOpen forces the search bar to stay open for the duration of the action. * This is useful in situations where want the search bar to not close when the user interacts * with modals shown from the search bar. - * - * Calling lockOpen concurrently is not safe, so make sure to structure the UI in such a way - * that it doesn't happen. lockOpen was made for cases like opening a modal where after a modal is - * open it guarantees that the user won't be able to interact with the search bar. */ async function lockOpen(action: Promise): Promise { - isLockedOpenRef.current = true; + setIsLockedOpen(true); try { await action; } finally { - isLockedOpenRef.current = false; + setIsLockedOpen(false); } } From 245d80f1a77c79075941d3ecec310eb0c1fb6532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 14 Apr 2023 15:10:24 +0200 Subject: [PATCH 17/19] Move capitalization to ResourceSearchError.messageWithClusterName --- web/packages/shared/utils/text.ts | 4 ---- .../teleterm/src/ui/Search/pickers/ActionPicker.tsx | 5 +++-- .../teleterm/src/ui/services/resources/resourcesService.ts | 7 +++++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/web/packages/shared/utils/text.ts b/web/packages/shared/utils/text.ts index 5abf045c2cb96..e4772876df209 100644 --- a/web/packages/shared/utils/text.ts +++ b/web/packages/shared/utils/text.ts @@ -28,7 +28,3 @@ export function pluralize(num: number, word: string) { return word; } - -export function firstLowerCase(string: string) { - return string.charAt(0).toLowerCase() + string.slice(1); -} diff --git a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx index 4d4da8de62867..fe6b1b7191dae 100644 --- a/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx +++ b/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx @@ -27,7 +27,6 @@ import { import * as icons from 'design/Icon'; import { Highlight } from 'shared/components/Highlight'; import { hasFinished } from 'shared/hooks/useAsync'; -import { firstLowerCase } from 'shared/utils/text'; import { useAppContext } from 'teleterm/ui/appContextProvider'; import { @@ -494,7 +493,9 @@ export function ResourceSearchErrorsItem(props: { shortDescription = `${firstErrorMessage}.`; } else { const allErrorMessages = errors - .map(err => firstLowerCase(err.messageWithClusterName(getClusterName))) + .map(err => + err.messageWithClusterName(getClusterName, { capitalize: false }) + ) .join(', '); shortDescription = `Ran into ${errors.length} errors: ${allErrorMessages}.`; } diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts index 40263ca33d044..f84cabb4fd6d2 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts @@ -147,12 +147,15 @@ export class ResourceSearchError extends Error { } messageWithClusterName( - getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string + getClusterName: (resourceUri: uri.ClusterOrResourceUri) => string, + opts = { capitalize: true } ) { const resource = pluralize(2, this.resourceKind); const cluster = getClusterName(this.clusterUri); - return `Could not fetch ${resource} from ${cluster}`; + return `${ + opts.capitalize ? 'Could' : 'could' + } not fetch ${resource} from ${cluster}`; } messageAndCauseWithClusterName( From b204716431c80a781c7ea94691ac45b8d10214fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 14 Apr 2023 15:12:45 +0200 Subject: [PATCH 18/19] ResourceSearchError: Use `public` in constructor --- .../teleterm/src/ui/services/resources/resourcesService.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts index f84cabb4fd6d2..2108490911995 100644 --- a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts +++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts @@ -129,12 +129,9 @@ export class AmbiguousHostnameError extends Error { } export class ResourceSearchError extends Error { - clusterUri: uri.ClusterUri; - resourceKind: SearchResult['kind']; - constructor( - clusterUri: uri.ClusterUri, - resourceKind: SearchResult['kind'], + public clusterUri: uri.ClusterUri, + public resourceKind: SearchResult['kind'], cause: Error ) { super( From fae3091f9da3103b9e3a771e36246a74f8ceccd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Mon, 17 Apr 2023 14:39:53 +0200 Subject: [PATCH 19/19] Connect: Improve focus management in search bar (#24665) --- .../teleterm/src/ui/Search/SearchBar.test.tsx | 64 ++++++++++++++++++- .../src/ui/Search/SearchContext.test.tsx | 46 +++++++++++++ .../teleterm/src/ui/Search/SearchContext.tsx | 18 +++++- .../teleterm/src/ui/Search/useSearch.test.tsx | 4 ++ 4 files changed, 130 insertions(+), 2 deletions(-) diff --git a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx index ca504d0b26282..b71170771c982 100644 --- a/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchBar.test.tsx @@ -15,12 +15,13 @@ */ import React from 'react'; -import { render, screen } from 'design/utils/testing'; +import { render, screen, waitFor } from 'design/utils/testing'; import { makeSuccessAttempt } from 'shared/hooks/useAsync'; import { MockAppContext } from 'teleterm/ui/fixtures/mocks'; import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvider'; import { ResourceSearchError } from 'teleterm/ui/services/resources'; +import ModalsHost from 'teleterm/ui/ModalsHost'; import * as pickers from './pickers/pickers'; import * as useActionAttempts from './pickers/useActionAttempts'; @@ -28,6 +29,10 @@ import * as SearchContext from './SearchContext'; import { SearchBarConnected } from './SearchBar'; +beforeEach(() => { + jest.restoreAllMocks(); +}); + it('does not display empty results copy after selecting two filters', () => { const appContext = new MockAppContext(); appContext.workspacesService.setState(draft => { @@ -202,6 +207,63 @@ it('notifies about resource search errors and allows to display details', () => expect(mockedSearchContext.lockOpen).toHaveBeenCalled(); }); +it('maintains focus on the search input after closing a resource search error modal', async () => { + const appContext = new MockAppContext(); + appContext.workspacesService.setState(draft => { + draft.rootClusterUri = '/clusters/foo'; + }); + + const resourceSearchError = new ResourceSearchError( + '/clusters/foo', + 'server', + new Error('whoops') + ); + + const mockActionAttempts = { + filterActionsAttempt: makeSuccessAttempt([]), + resourceActionsAttempt: makeSuccessAttempt([]), + resourceSearchAttempt: makeSuccessAttempt({ + results: [], + errors: [resourceSearchError], + search: '', + }), + }; + jest + .spyOn(useActionAttempts, 'useActionAttempts') + .mockImplementation(() => mockActionAttempts); + + render( + + + + + ); + + screen.getByRole('searchbox').focus(); + expect(screen.getByRole('menu')).toHaveTextContent( + 'Some of the search results are incomplete.' + ); + screen.getByText('Show details').click(); + + const modal = screen.getByTestId('Modal'); + expect(modal).toHaveTextContent('Resource search errors'); + expect(modal).toHaveTextContent('whoops'); + + // Lose focus on the search input. + screen.getByText('Close').focus(); + screen.getByText('Close').click(); + + // Need to await this since some state updates in SearchContext are done after the modal closes. + // Otherwise we'd get a warning about missing `act`. + await waitFor(() => { + expect(modal).not.toBeInTheDocument(); + }); + + expect(screen.getByRole('searchbox')).toHaveFocus(); + // Verify that the search bar wasn't closed. + expect(screen.getByRole('menu')).toBeInTheDocument(); +}); + const getMockedSearchContext = () => ({ inputValue: 'foo', filters: [], diff --git a/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx b/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx index 23208851c3a0c..1d610ec2062a4 100644 --- a/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx +++ b/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx @@ -15,6 +15,8 @@ */ import React from 'react'; +import '@testing-library/jest-dom'; +import { render, screen } from '@testing-library/react'; import { renderHook, act } from '@testing-library/react-hooks'; import { SearchContextProvider, useSearchContext } from './SearchContext'; @@ -40,11 +42,16 @@ describe('lockOpen', () => { finishAction: rejectFailureAction, }, ])('$name', async ({ action, finishAction }) => { + const inputFocus = jest.fn(); const { result } = renderHook(() => useSearchContext(), { wrapper: ({ children }) => ( {children} ), }); + result.current.inputRef.current = { + focus: inputFocus, + } as unknown as HTMLInputElement; + act(() => { result.current.open(); }); @@ -74,5 +81,44 @@ describe('lockOpen', () => { result.current.close(); }); expect(result.current.isOpen).toBe(false); + + // Called the first time during `open`, then again after `lockOpen` finishes. + expect(inputFocus).toHaveBeenCalledTimes(2); + }); +}); + +describe('open', () => { + it('manages the focus properly when called with no arguments', () => { + const SearchInput = () => { + const { inputRef, open, close } = useSearchContext(); + + return ( + <> + +