Skip to content

Account for request errors when displaying "no results" message in the search bar#25061

Merged
ravicious merged 5 commits intomasterfrom
ravicious/action-picker-status
Apr 26, 2023
Merged

Account for request errors when displaying "no results" message in the search bar#25061
ravicious merged 5 commits intomasterfrom
ravicious/action-picker-status

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Apr 24, 2023

What needs to be fixed

When the user types in a query for which there's no search results, we look at ClustersService to see if there are any clusters that are not "connected", that is clusters with expired certs. We then tell the user that those clusters were not included in the search because the user is not logged in to them.

no results found message

However, the connected property is updated only in a couple of scenarios, the most relevant being the start of the app. After that we pretty much don't touch it at all, so at the moment it only tells us whether the user had valid certs when the app was started.

Naturally, this approach doesn't handle a scenario in which the user was logged in to multiple clusters, left the app running, then came back to it, ran a search and then refreshed the cert for the active workspace (#24880). If they type a query with no search results now, those other clusters with expired certs will not be listed in that "no results" message. Instead, those failed requests will be treated like any other failed requests, even though we know that they failed due to expired certs.

How this PR fixes that

This PR makes it so that the list of clusters with expired certs is based not only on the state of ClustersService, but also the results of the resource search requests.

To do this, I refactored how any extra top messages above the results are rendered. The business and presentational logic used to be scattered around ActionPicker and auxiliary components that were passed to ResultList as ExtraTopComponent. Instead, I made the distinction more clear cut:

  • getActionPickerStatus is a function which takes necessary arguments and returns ActionPickerStatus, a discriminated union describing possible states for the action picker.
  • ExtraTopComponents takes ActionPickerStatus and renders relevant top messages through auxiliary components.
  • The auxiliary components receive relevant pieces of action picker status.

The idea here being that all business logic is performed in getActionPickerStatus so that after ActionPickerStatus is returned, the other components can just display what's necessary.

Other business logic changes

  • Leaf clusters are included in the "excluded clusters" message too.
    • It doesn't hurt us to include them there too. This way we also make less assumptions about how much the user knows about the relation of any given leaf cluster to the root cluster.
  • When there are multiple excluded clusters, they're included at the end of the message.
    • This makes reading the message easier because the first line doesn't change and what follows are just cluster names.
  • Retryable errors are excluded from the error item.

Comment on lines +287 to +294
type ActionPickerStatus =
| { status: 'no-input'; hasSelectedAllFilters: boolean }
| { status: 'processing' }
| {
status: 'finished';
hasNoResults: boolean;
nonRetryableResourceSearchErrors: ResourceSearchError[];
clustersWithExpiredCerts: Set<uri.ClusterUri>;
};
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.

When making ActionPickerStatus, I tried to follow the principle of making sure that impossible states are impossible to express with data structures. Initially some properties were included directly in the ActionPickerState while others were gated behind a discriminated union.

For example, I knew that hasNoResults would make no sense if status was different than finished, because if the search hasn't finished yet how we could be sure that there's no results.

But there are fields which technically aren't tied to the status. hasSelectedAllFilters can be true or false no matter whether the search is in progress or not. clustersWithExpiredCerts could technically include only clusters from ClustersService when the search is still in progress.

Initially I included those directly on the object. However, soon I refactored the code to include them only where necessary and tied all of them to particular statuses. When reading the type alone, it's more clear in what situation the given property is used. If there was a field used by 2/3 of the statuses, then I'd probably repeat it for those two statuses.

@ravicious ravicious marked this pull request as ready for review April 24, 2023 11:48
@github-actions github-actions Bot requested review from rudream and ryanclark April 24, 2023 11:48
@ravicious ravicious requested review from avatus and gzdunek and removed request for rudream and ryanclark April 24, 2023 11:48
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Found one bug, but except that it looks good.

Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx Outdated
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.test.ts Outdated
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx Outdated
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from avatus April 24, 2023 13:32
Base automatically changed from ravicious/search-bar-retry to master April 26, 2023 08:47
@ravicious ravicious force-pushed the ravicious/action-picker-status branch from 1e5b01a to 0661ee5 Compare April 26, 2023 09:46
@ravicious ravicious enabled auto-merge April 26, 2023 09:46
@ravicious ravicious added this pull request to the merge queue Apr 26, 2023
Merged via the queue into master with commit 839afa8 Apr 26, 2023
@ravicious ravicious deleted the ravicious/action-picker-status branch April 26, 2023 10:06
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v13 Failed

@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.

3 participants