Merged
Conversation
When the variant analysis view was being rerendered, we were also reregistering the message listeners, while not deregistering the old ones. This resulted in a loop of message listeners being registered, and the variant analysis being rerendered every time a message was received by one of the listeners. This will ensure that the listener is only registered once to prevent this from happening.
This cleanup function would never be called in normal operation, but if we do decide to add a dependency to this `useEffect`, this will ensure that only one listener is registered at a time.
charisk
approved these changes
Oct 11, 2022
Contributor
charisk
left a comment
There was a problem hiding this comment.
Good catch! I guess this is technically a problem in other views as well?
Member
Author
|
Yes, this would also be a problem in other views. However, those other views only receive 1 or 2 messages, so those don't run into the same problem. I'll open another PR to fix it for the compare and remote queries views (the results view does handle it correctly). |
Contributor
|
Thanks for catching this. We'll have to do the same for our other views. Tracking this in #1582. |
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
3 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.
It's probably easiest to review this PR commit-by-commit; these also contain some more detailed information about why certain things are necessary. In summary, on every rerender we were registering a new message listener. This would cause every message sent by the extension to trigger N+1 rerenders for the Nth render. The renders would look as follows:
This would continue forever and would sometimes result in 10's of thousands of renders happening. This caused the live results view to be unresponsive until no more messages were being sent.
Checklist
ready-for-doc-reviewlabel there.