Skip to content

Show resource search errors in search bar when fetching a preview#25791

Merged
ravicious merged 4 commits intomasterfrom
ravicious/preview-errors
May 11, 2023
Merged

Show resource search errors in search bar when fetching a preview#25791
ravicious merged 4 commits intomasterfrom
ravicious/preview-errors

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented May 8, 2023

This does not need to be merged before the v13 release.


#25314 made it so that we fetch a preview of matching resources as soon as the user selects one of the filters. However, we don't display errors if fetching that preview fails. This PR fixes that.

The easiest way to review this PR is to go bottom-up, from the changes to the data structures to the actual UI changes. Take a look at how type ActionPickerStatus has changed in ActionPicker.tsx and it should be easier to follow the changes that I had to make in getActionPickerStatus and ExtraTopComponents.

The previous definition of ActionPickerStatus assumed that we make a search request only when the user types in something in the search bar. The new definition recognizes that we fetch a preview when the input is empty and at least one filter is selected.

Comment on lines +318 to +320
* 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.
Copy link
Copy Markdown
Member Author

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 ExtraTopComponents only 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 hasNoResults is false because we didn't finish fetching yet.

If the request is done, hasNoResults can 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 getActionPickerStatus knows what the actual status of the request is, so ActionPickerStatus can act as a proxy when answering the questions, without the components in ExtraTopComponents having to worry whether the request is in progress or not. This is not the equivalent of forgetting to check the isLoading flag.

searchMode:
| {
// no-search: The search bar is pristine, that is the input and the filters are empty.
kind: 'no-search';
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel kinda uneasy about having two sources of truth when it comes to searchMode. It is defined in useSearch.tsx as well:

function getResourceSearchMode(
search: string,
filters: SearchFilter[]
): ResourceSearchMode {
// Trim the search to avoid sending requests with limit set to 100 if the user just pressed some
// spaces.
const trimmedSearch = search.trim();
if (!trimmedSearch) {
// The preview should be fetched only when at least one filter is selected. Otherwise we'd send
// three requests for each connected cluster when the search bar gets open.
if (filters.length >= 1) {
return 'preview';
}
return 'no-search';
}
return 'full-search';
}

Perhaps it'd make more sense for getActionPickerStatus to accept searchMode coming from useSearch as an argument instead of accepting both inputValue and filters and repeating the logic.

searchMode would need to be extracted from useResourceSearch though so that it can be known without waiting for the search to resolve. This could be done by exporting getResourceSearchMode and then passing searchMode to both the function returned from useResourceSearch and to getActionPickerStatus.

On top of that, those search modes are not 100% the same, useResourceSearch trims the input to avoid making unnecessary requests, so that logic would need to be made consistent between these two places.

Alas, this would require more time then I have at the moment. Plus ActionPickerStatus is still subject to change, for example when taking filters into account when displaying offline clusters (#24190 (comment)).

@ravicious ravicious marked this pull request as ready for review May 8, 2023 11:42
@github-actions github-actions Bot requested review from rudream and ryanclark May 8, 2023 11:42
@ravicious ravicious requested review from avatus and gzdunek and removed request for rudream and ryanclark May 8, 2023 11:42
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx Outdated
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx Outdated
@ravicious ravicious requested a review from ryanclark May 11, 2023 12:40
@ravicious ravicious enabled auto-merge May 11, 2023 12:41
@ravicious ravicious added this pull request to the merge queue May 11, 2023
Merged via the queue into master with commit 16eee15 May 11, 2023
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants