Show resource search errors in search bar when fetching a preview #25791
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ import { | |
| SearchResultServer, | ||
| SearchResultCluster, | ||
| SearchResultResourceType, | ||
| SearchFilter, | ||
| } from 'teleterm/ui/Search/searchResult'; | ||
| import * as tsh from 'teleterm/services/tshd/types'; | ||
| import * as uri from 'teleterm/ui/uri'; | ||
|
|
@@ -152,13 +153,15 @@ export function ActionPicker(props: { input: ReactElement }) { | |
| () => | ||
| getActionPickerStatus({ | ||
| inputValue, | ||
| filters, | ||
| filterActionsAttempt, | ||
| actionAttempts, | ||
| resourceSearchAttempt, | ||
| allClusters: clustersService.getClusters(), | ||
| }), | ||
| [ | ||
| inputValue, | ||
| filters, | ||
| filterActionsAttempt, | ||
| actionAttempts, | ||
| resourceSearchAttempt, | ||
|
|
@@ -243,18 +246,41 @@ const ExtraTopComponents = (props: { | |
| }) => { | ||
| const { status, getClusterName, showErrorsInModal } = props; | ||
|
|
||
| switch (status.inputState) { | ||
| case 'no-input': { | ||
| switch (status.searchMode.kind) { | ||
| case 'no-search': { | ||
| return <TypeToSearchItem hasNoRemainingFilterActions={false} />; | ||
| } | ||
| case 'preview': { | ||
| const { | ||
| nonRetryableResourceSearchErrors, | ||
| hasNoRemainingFilterActions, | ||
| } = status.searchMode; | ||
|
|
||
| return ( | ||
| <> | ||
| <TypeToSearchItem | ||
| hasNoRemainingFilterActions={hasNoRemainingFilterActions} | ||
| /> | ||
| {nonRetryableResourceSearchErrors.length > 0 && ( | ||
| <ResourceSearchErrorsItem | ||
| errors={nonRetryableResourceSearchErrors} | ||
| getClusterName={getClusterName} | ||
| showErrorsInModal={() => { | ||
| showErrorsInModal(nonRetryableResourceSearchErrors); | ||
| }} | ||
| /> | ||
| )} | ||
| </> | ||
| ); | ||
| } | ||
| default: { | ||
| return assertUnreachable(status.searchMode); | ||
| } | ||
| } | ||
| } | ||
| case 'some-input': { | ||
| return ( | ||
| <> | ||
| {status.nonRetryableResourceSearchErrors.length > 0 && ( | ||
|
|
@@ -281,30 +307,71 @@ const ExtraTopComponents = (props: { | |
| } | ||
| }; | ||
|
|
||
| /** | ||
| * ActionPickerStatus helps with displaying ExtraTopComponents. It has two goals: | ||
| * | ||
| * * Encapsulate business logic so that anything that ExtraTopComponents renders can just read | ||
| * ActionPickerStatus fields. | ||
| * * Represent only valid UI states. For example, inputState 'no-input' doesn't have hasNoResults | ||
| * field as this field would make no sense in a situation where no search requests were made. | ||
| * | ||
| * As you may notice, ActionPickerStatus doesn't say whether the search request is in progress or | ||
| * not, simply because displaying the progress bar is handled by another component. The questions | ||
| * answered by ActionPickerStatus are valid to ask no matter what the state of the request is. | ||
| */ | ||
| type ActionPickerStatus = | ||
| | { | ||
| // no-input: The input is empty. | ||
| inputState: 'no-input'; | ||
| searchMode: | ||
| | { | ||
| // no-search: The search bar is pristine, that is the input and the filters are empty. | ||
| kind: 'no-search'; | ||
|
Member
Author
There was a problem hiding this comment. I feel kinda uneasy about having two sources of truth when it comes to teleport/web/packages/teleterm/src/ui/Search/useSearch.ts Lines 360 to 377 in ef0cb91 Perhaps it'd make more sense for
On top of that, those search modes are not 100% the same, Alas, this would require more time then I have at the moment. Plus |
||
| } | ||
| | { | ||
| // preview: At least one filter is selected. The search bar is fetching or shows | ||
| // a preview of results matching the filters. | ||
| kind: 'preview'; | ||
| hasNoRemainingFilterActions: boolean; | ||
| nonRetryableResourceSearchErrors: ResourceSearchError[]; | ||
| }; | ||
| } | ||
| | { | ||
| // some-input: The input is not empty. The search bar is fetching or shows results matching | ||
| // the query and filters. | ||
| inputState: 'some-input'; | ||
| hasNoResults: boolean; | ||
| nonRetryableResourceSearchErrors: ResourceSearchError[]; | ||
| clustersWithExpiredCerts: Set<uri.ClusterUri>; | ||
| }; | ||
|
|
||
| export function getActionPickerStatus({ | ||
| inputValue, | ||
| filters, | ||
| filterActionsAttempt, | ||
| allClusters, | ||
| actionAttempts, | ||
| resourceSearchAttempt, | ||
| }: { | ||
| inputValue: string; | ||
| filters: SearchFilter[]; | ||
| filterActionsAttempt: Attempt<SearchAction[]>; | ||
| allClusters: tsh.Cluster[]; | ||
| actionAttempts: Attempt<SearchAction[]>[]; | ||
| resourceSearchAttempt: Attempt<CrossClusterResourceSearchResult>; | ||
| }): ActionPickerStatus { | ||
| if (!inputValue) { | ||
| const didNotSelectAnyFilters = filters.length === 0; | ||
|
|
||
| // If the input is empty, we fetch the preview only after the user selected some filters. | ||
| // So at this point we know that no search request was sent. | ||
| if (didNotSelectAnyFilters) { | ||
| return { | ||
| inputState: 'no-input', | ||
| searchMode: { kind: 'no-search' }, | ||
| }; | ||
| } | ||
|
|
||
| // The number of available filters the user can select changes dynamically based on how many | ||
| // clusters are in the state. That's why instead of inspecting the filters array from | ||
| // SearchContext, we inspect the actual filter actions attempt to see if any further filter | ||
|
|
@@ -316,30 +383,46 @@ export function getActionPickerStatus({ | |
| filterActionsAttempt.status === 'success' && | ||
| filterActionsAttempt.data.length === 0; | ||
|
|
||
| const nonRetryableResourceSearchErrors = | ||
| resourceSearchAttempt.status === 'success' | ||
| ? resourceSearchAttempt.data.errors.filter( | ||
| err => !isRetryable(err.cause) | ||
| ) | ||
| : []; | ||
|
|
||
| return { | ||
| inputState: 'no-input', | ||
| searchMode: { | ||
| kind: 'preview', | ||
| hasNoRemainingFilterActions, | ||
| nonRetryableResourceSearchErrors, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const nonRetryableResourceSearchErrors = []; | ||
| const clustersWithExpiredCerts = new Set( | ||
| allClusters.filter(c => !c.connected).map(c => c.uri) | ||
| ); | ||
| const haveActionAttemptsFinished = actionAttempts.every(attempt => | ||
| hasFinished(attempt) | ||
| ); | ||
|
|
||
| if (!haveActionAttemptsFinished) { | ||
| return { | ||
| inputState: 'some-input', | ||
| hasNoResults: false, | ||
| nonRetryableResourceSearchErrors, | ||
| clustersWithExpiredCerts, | ||
| }; | ||
| } | ||
|
|
||
| const hasNoResults = actionAttempts.every( | ||
| attempt => attempt.data.length === 0 | ||
| ); | ||
|
|
||
| // We could assume that resourceSearchAttempt has finished since action attempts depend on it and | ||
| // we know that they all finished at this point. But we check status explicitly anyway. | ||
| if (resourceSearchAttempt.status === 'success') { | ||
| resourceSearchAttempt.data.errors.forEach(err => { | ||
| if (isRetryable(err.cause)) { | ||
|
|
@@ -351,7 +434,7 @@ export function getActionPickerStatus({ | |
| } | ||
|
|
||
| return { | ||
| inputState: 'some-input', | ||
| hasNoResults, | ||
| clustersWithExpiredCerts, | ||
| nonRetryableResourceSearchErrors, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(philosophical tangent ahead)
To elaborate on that, for example
ExtraTopComponentsonly needs to know if there are no search results, no matter what the status of the request is.If the request is in progress, the
hasNoResultsis false because we didn't finish fetching yet.If the request is done,
hasNoResultscan be true/false depending on what the response is.Had we returned request status next to
hasNoResults, it'd make it possible to introduce nonsensical states, e.g.hasNoResults: true, requestState: 'processing'. But since we don't tell what the status of the request is, every combination of fields that are currently present in this type makes sense.This is all possible only because
getActionPickerStatusknows what the actual status of the request is, soActionPickerStatuscan act as a proxy when answering the questions, without the components inExtraTopComponentshaving to worry whether the request is in progress or not. This is not the equivalent of forgetting to check theisLoadingflag.