Skip to content

Connect: Show resource search errors in the search bar#24520

Merged
ravicious merged 20 commits intomasterfrom
ravicious/error-handling
Apr 17, 2023
Merged

Connect: Show resource search errors in the search bar#24520
ravicious merged 20 commits intomasterfrom
ravicious/error-handling

Conversation

@ravicious
Copy link
Copy Markdown
Member

This PR adds the foundation for error handling in the search bar. At the moment, it adds an extra item at the top of the search bar if a request for any resource fails.

This is suboptimal because it doesn't account for a bunch of different edge cases, for example requests to clusters with expired certs. This will be done in another PR since this one is quite large already.

Only 4 of the last 5 commits actually implement this. The rest are cleanup/refactoring commits which either make this feature possible or just clean up the code.

Screenshots

search bar

modal with errors

The most important pieces

At the heart of this PR is the change to ResourcesService.prototype.searchResources in the commit "Refactor resource search to use Promise.allSettled". We used to simply return an array with all search result promises wrapped in Promise.all. This wasn't that great because if any of the requests failed, you wouldn't see the results at all.

To work around this, we use Promise.allSettled and then return a flattened array of results across different clusters. Another important piece is the custom ResourceSearchError. If a resource search request fails, this is the error that is returned. It contains extra fields, one which points at the relevant cluster and one which describes what kind of resource we were trying to fetch.

Thanks to this, later in the UI we can extract those errors and tell the user what exactly went wrong.

Again, this is a very basic implementation which doesn't account for a plethora of edge cases, we'll deal with that in a separate PR.

Keeping the search bar open when inspecting errors

One issue I ran into is that we want the user to be able to inspect the details behind the errors in a modal. However, the search bar was written in a way where any click outside of the search bar is going to close it.

To fix this, I added a lockOpen function to the search context which takes a promise and prevents the modal from being closed for the duration of the promise.

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.
* useSearchAttempts has been renamed to useActionAttempts
* useActionAttempts returns resourceSearchAttempt in order to supply errors
  from ResourcesService.searchResources to ActionPicker.
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.
@github-actions github-actions Bot requested review from avatus and ryanclark April 13, 2023 11:03
@ravicious ravicious requested review from gzdunek and removed request for ryanclark April 13, 2023 11:03
Comment thread web/packages/teleterm/src/ui/services/resources/resourcesService.ts
Comment thread web/packages/teleterm/src/ui/Search/SearchContext.test.tsx
Comment thread web/packages/teleterm/src/ui/services/resources/resourcesService.ts
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx Outdated
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.

The code looks really good, I will test it tomorrow.

Comment thread web/packages/teleterm/src/ui/services/resources/resourcesService.ts
Comment thread web/packages/teleterm/src/ui/Search/SearchContext.tsx
Comment thread web/packages/teleterm/src/ui/Search/SearchContext.test.tsx
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx Outdated
Comment thread web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx Outdated
@ravicious ravicious requested a review from gzdunek April 14, 2023 09:25
Comment thread web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx
@ravicious ravicious requested a review from gzdunek April 14, 2023 13:20
@ravicious ravicious enabled auto-merge April 14, 2023 14:03
Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

this is really cool. its so concise, but also so informative. I REALLY like this, incredible job.

To fix this, I added a lockOpen function to the search context which takes a promise and prevents the modal from being closed for the duration of the promise.

Interesting solution, i dig it

@ravicious
Copy link
Copy Markdown
Member Author

Thanks Michael!

To fix this, I added a lockOpen function to the search context which takes a promise and prevents the modal from being closed for the duration of the promise.

Interesting solution, i dig it

The inspiration for this comes from Ruby's File.open which takes an anonymous function and automatically closes the file descriptor after the function returns. There's a lot of methods like that in Ruby and Rails. Another one is ActiveRecord::Base.transaction which wraps any SQL calls within the function in a transaction. Though Ruby is much more OOP oriented than say JS, they had support for anonymous functions since forever but they call them "blocks".

@ravicious ravicious added this pull request to the merge queue Apr 17, 2023
Merged via the queue into master with commit dd9c042 Apr 17, 2023
@ravicious ravicious deleted the ravicious/error-handling branch April 17, 2023 13:01
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