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

[Dashboard] improve performance for initial rendering #208050

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Jan 23, 2025

Summary

While investigating why renderPanelContents runs three times on page load (I remove one of 3 in my refactoring PR, but want to understand the second re-render too), I found that the issue is caused by the useCallback dependency on dashboardContainer. dashboardContainer is the HTML element we pass to the grid items. To prevent unnecessary re-renders, I’ve updated it to pass the entire ref instead of the element itself.

I’m unsure if this change might introduce other issues, as I don't know if I caught all the usecases for using it. What I tested (and didn't break) is this flow (and it still works correctly):

  • open a dashboard
  • for a Lens visualization that's not on top of the screen, click on 'edit' button
  • expected behavior: the visualization scrolls to the top of the dashboard container screen, but is not hidden below the header content.
right.mp4

If I replace the line const dashboardContainerTopOffset = dashboardContainerRef?.current?.offsetTop || 0; to const dashboardContainerTopOffset = 0;, it breaks so I guess we're set :)

wrong.mp4

@mbondyra mbondyra added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas backport:skip This commit does not require backporting backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Jan 23, 2025
@mbondyra mbondyra requested review from nreese and Heenawter January 23, 2025 13:12
@mbondyra mbondyra requested a review from a team as a code owner January 23, 2025 13:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@mbondyra mbondyra added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting labels Jan 23, 2025
@mbondyra mbondyra force-pushed the dashboard/performance_fix branch from df54707 to 8bee593 Compare January 23, 2025 13:30
@mbondyra mbondyra force-pushed the dashboard/performance_fix branch from 8bee593 to b833a89 Compare January 23, 2025 13:31
@mbondyra mbondyra changed the title [Dashboard] fix performance for initial rendering [Dashboard] improve performance for initial rendering Jan 23, 2025
Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

This LGTM! Thanks so much for tackling this 🙇 Fix makes sense to me + tested the Dashboard in fullscreen mode (&_a=(fullScreenMode:true)) + with a banner, and the offset continues to work as expected. Also ensured that renderPanelContents is only called once on load now, which is amazing!!

Just to be sure, I am running a performance test here to see if we can call this a fix for #207011 🎉 The results will take around 3-4 hours, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) performance release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants