Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug - Page - resize observer causes many unecessary re-renders #7810

Closed
Hyperkid123 opened this issue Aug 10, 2022 · 2 comments · Fixed by #7940
Closed

Bug - Page - resize observer causes many unecessary re-renders #7810

Hyperkid123 opened this issue Aug 10, 2022 · 2 comments · Fixed by #7940
Assignees
Milestone

Comments

@Hyperkid123
Copy link
Contributor

Hyperkid123 commented Aug 10, 2022

Describe the problem
The Page component resizeObserver triggers on rach rezise event in 250ms intervals, causing the Page React.Context to be continuously updated which results in a very large number of re-renders in its subtree.

How do you reproduce the problem?
Login into console.stage.redhat.com and resize the window. The left nav will flicker.

This draft can be used for testing in local environment: RedHatInsights/insights-chrome#2002

Expected behavior
The context should be updated only once.

Is this issue blocking you?
No workaround, service developers are affected and it is slowing feature development and blocking further release.

Screenshots

re-render-bug.webm

If applicable, add screenshots to help explain the issue.

What is your environment?

  • OS: Linux
  • Browser Chrome
  • Version Version 104.0.5112.79 (Official Build) (64-bit)

What is your product and what release date are you targeting?

Hybrid cloud console. Issues is currently in the development version of the site. Blocking additional production releases.

Any other information?

Chrome profiling report used in recording

https://gist.github.com/Hyperkid123/0c82f1041437ec3ba6334a580b72ae6d#file-pf-page-profile

Possible quick fix

After quickly reviewing the Page component there is a one quick fox we can apply before coming up with a proper long term solution:

The resize observer initialization is hidden behind this condition:

    const { isManagedSidebar, onPageResize } = this.props;
    if (isManagedSidebar || onPageResize) {
      this.observer = getResizeObserver(this.pageRef.current, this.handleResize);

We do not use isManagedSidebar nor the onPageResize, yet the observer is created. That is because the component has a default prop setup:

  static defaultProps: PageProps = {
    isManagedSidebar: false,
    isBreadcrumbWidthLimited: false,
    defaultManagedSidebarIsOpen: true,
    onPageResize: (): void => null, // this default prop makes the condition always true
    mainTabIndex: -1,
    isNotificationDrawerExpanded: false,
    onNotificationDrawerExpand: () => null,
    getBreakpoint,
    getVerticalBreakpoint
  };

After removing the default prop, the observer is not created and we no longer see the bug in our application. This can serve as a hotfix, that we desperately need to be released ASAP. The default can be safely removed because all the function calls in the component itself are again checked with a condition.

For the proper long-term fix, I don't have a suggestion ATM. I can say tat the issue is somewhere within the debouce function or how/when it is initialized. You can see from the profiling that the re-renders and context updates happen in exact 250ms intervals, which is the same interval the debounce function is configured with.

@Hyperkid123 Hyperkid123 changed the title Bug - [Page] - [resize observer causes many unecessary re-renders] Bug - Page - resize observer causes many unecessary re-renders Aug 10, 2022
@Andrewgdewar
Copy link

Andrewgdewar commented Aug 10, 2022

This may be related to this other bug I was seeing. Basically seeing a heartbeat every 250ms that caused a rerender of all patternfly components using an internal incremental ID: #7761

181628044-bd070b51-0b36-43fc-b065-6a721a3c48ba

A note: the reason my app may be seeing this more than others was due to having a top level context provider (react-query), as well as a component with it's own context internal state.

This is also happening in all document.body attached select menu's :

Aug-10-2022 11-59-57

@jschuler
Copy link
Collaborator

Looks like there are state updates even if the width has not changed, could be potential reason behind the issue

this.setState({ width: this.pageRef.current.clientWidth, height: this.pageRef.current.clientHeight });

Perhaps wrapping it in a conditional can help alleviate it

if (this.pageRef.current) {
  const currentWidth = this.pageRef.current.clientWidth;
  const currentHeight = this.pageRef.current.clientHeight;
  if (this.state.width !== currentWidth) {
    this.setState({ width: currentWidth });
  }
  if (this.state.height !== currentHeight) {
    this.setState({ height: currentHeight });
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants