Skip to content

Get rid of unmounted component update in ResultList#26143

Merged
ravicious merged 1 commit intomasterfrom
ravicious/action-picker-leak
May 12, 2023
Merged

Get rid of unmounted component update in ResultList#26143
ravicious merged 1 commit intomasterfrom
ravicious/action-picker-leak

Conversation

@ravicious
Copy link
Copy Markdown
Member

Reproduction steps:

  1. Start dev version of Connect, open the console.
  2. Open the search bar, select a filter.
  3. Select an SSH server or a database item.
  4. You should see now the leak warning in the console.

Strangely enough, this happen only the first time you do this, not on subsequent calls.

ResultList is rendered by ActionPicker and ParameterPicker. onPick on an SSH server or a database calls changeActivePicker from SearchContext which unmounts ActionPicker and mounts ParameterPicker.

Still, this doesn't explain how the leak happens. I was under the impression that React first processes all state updates and only then re-renders the components.

Well, that's true, but only for React 18. React 17 does not batch the state updates in certain cases, for example when executing native event listeners.

It just so happens that pickAndResetActiveItem is called from an event listener that is added directly on window with addEventListener.

By calling setActiveItemIndex first we make sure that this state update is processed before onPick potentially causes ResultList to unmount.

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.

❤️

@ravicious ravicious added this pull request to the merge queue May 12, 2023
Merged via the queue into master with commit 67181c8 May 12, 2023
@ravicious ravicious deleted the ravicious/action-picker-leak branch May 12, 2023 16:06
@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