Retry search in Connect if current workspace cert has expired#24880
Merged
Retry search in Connect if current workspace cert has expired#24880
Conversation
The state setter coming from useState is always stable. https://legacy.reactjs.org/docs/hooks-reference.html#usestate
lockOpen worked great when we were concerned only about user interaction with a modal closing the search bar as well. However, in the next commit I'm going to add a login modal that's shown if the search fails with a retryable error. In that scenario, pressing Enter in the modal wouldn't work, as it would be captured by the window listener that ResultList adds. To work around this problem, I refactored lockOpen into pauseUserInteraction. It still works pretty much the same way. But then instead of having checking isLockedOpen in the close function, we have a new addWindowEventListener function. addWindowEventListener automatically removes the listener after pauseUserInteraction is called. This solves both the problem of closing the modal and the problem of using the enter key in the modal.
gzdunek
approved these changes
Apr 20, 2023
Contributor
gzdunek
left a comment
gzdunek
left a comment
There was a problem hiding this comment.
Love the solution with addWindowListener, looks so clean and solid!
ravicious
commented
Apr 21, 2023
| } = props; | ||
| const activeItemRef = useRef<HTMLDivElement>(); | ||
| const [activeItemIndex, setActiveItemIndex] = useState(0); | ||
| const { addWindowEventListener } = useSearchContext(); |
Member
Author
There was a problem hiding this comment.
Adding useSearchContext to ResultList broke the ActionPicker story because the story was not wrapping that component in the context. I refactored ResultList to take addWindowEventListener as a prop instead. It is a pretty much a self-contained component with only presentational logic so in the end I figured it might not make sense to contaminate it with a context.
gzdunek
approved these changes
Apr 21, 2023
avatus
approved these changes
Apr 25, 2023
ryanclark
approved these changes
Apr 26, 2023
|
@ravicious See the table below for backport results.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The main goal of this PR is to detect if any search request made to clusters from the current workspace has failed, if so offer the user a way to relogin and then retry the search.
We do this only for current workspace requests because otherwise we'd have to get the user through a modal for every cluster they're logged in to.
As usual, the first couple of commits are small refactors of related functions. The actual feature is implemented in the last two commits.
The best way to test it is to copy the tsh folder with the expired certs:
Then start Connect, log in to the cluster and then replace the fresh certs with expired ones:
The next request to the cluster should make Connect think that the certs have expired.
lockOpen->pauseUserInteractionThe big thing is how
lockOpenhas been refactored intopauseUserInteraction.lockOpenwas added in #24520. #24520 makes it so that when any of the requests fails, we offer the ability to open a modal which shows error details. We don't want the search bar to get closed when the user interacts with the modal, so we introducedlockOpen.This PR shows another modal, the login modal, while the search bar is supposed to stay open. However, the login modal is much more complex than the one with error details. It not only contains some buttons, but also some form fields.
Because
ResultListadds a global window listener which captures some keyboard presses, it was impossible to press Enter in the login modal while the search bar was open.So I completely changed how
lockOpenworks. Instead of merely causing theclosefunction to be a noop, I added a new function toSearchContextcalledaddWindowEventListener. Any window listener related to the search bar should be registered through this function.addWindowEventListeneris supposed to be used inuseEffectand it automatically removes the listeners for the duration ofpauseUserInteraction, which is the new name forlockOpen.pauseUserInteractionis intended for those cases where we want to keep the search bar displayed while the user interacts with some other element of the UI.Why do we need global window listeners in the first place?
One is used to detect outside clicks while the search bar is open to close the search bar. I'm not sure if there's a way to get rid of this one. The other listener is to provide keyboard navigation while the search bar is open. Grzegorz had some ideas on how to get rid of this one but the current situation is a consequence of how the components and the underlying DOM tree are structured. The input tag is rendered outside of
ResultListbutResultListwould need to add event listeners on the input.