Skip to content

[BUGFIX] Dashboard: remove unmountOnExit flag of the row collapse#80

Open
shahrokni wants to merge 1 commit intomainfrom
fix/row_performance_issue
Open

[BUGFIX] Dashboard: remove unmountOnExit flag of the row collapse#80
shahrokni wants to merge 1 commit intomainfrom
fix/row_performance_issue

Conversation

@shahrokni
Copy link
Contributor

@shahrokni shahrokni commented Mar 3, 2026

relates to perses/perses#3448

⚠️ I would like to have the approval from both @jgbernalp and @AntoineThebaud

Description

This small change makes the heavy dashboards open/close collapses 10X faster!

I drilled down and found out that when row -> collapse -> unmountOnExit is set to true and the user closes and opens the collapse again, (VERY LIKELY) two things happen. (Based on the browser profiling metrics)

  • During open the children hierarchy of the collapsed row are rendered from scratch due to unmountOnExit
  • Tanstack tries to find the results of this query from the cache which is a very heavy process. You can check the network and see that there is no network traffic. All data is picked from the cache.

Given the volume of data downloaded for a huge dashboard like nodexporter, retrieving the data from the cache itself is a huge time consuming task! This takes more than 2000 ms

Confusion 💀 Open Scary Question

Why have we set unmountOnExit to true?! Here, the only benefit of collapse is to hide unnecessary info. Why should it destroy the the only and immediate child?! What is the advantage?

Tested corner cases

  • refresh when the group is open
  • refresh when the group is closed and then open (should have been updated)
  • Suggested by @Gladorme It would be nice to test on a dashboard with 100+ panels, and see what happens

Test instruction 🧪

You can find the details of the test instruction here
perses/perses#3448

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
    following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
  • All commits have DCO signoffs.

UI Changes

  • Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
  • Code follows the UI guidelines.
  • E2E tests are stable and unlikely to be flaky.
    See e2e docs for more details. Common issues include:
    • Is the data inconsistent? You need to mock API requests.
    • Does the time change? You need to use consistent time values or mock time utilities.
    • Does it have loading states? You need to wait for loading to complete.

Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com>
@Gladorme
Copy link
Member

Gladorme commented Mar 4, 2026

Yeah, as told in private. The unMount has been probably added for perf reasons (https://mui.com/material-ui/react-accordion/#performance). To remove some nodes from DOM, if panel group has a lot of panels in it . That need to be tested too.

@shahrokni
Copy link
Contributor Author

Yeah, as told in private. The unMount has been probably added for perf reasons (https://mui.com/material-ui/react-accordion/#performance). To remove some nodes from DOM, if panel group has a lot of panels in it . That need to be tested too.

Yes it makes sense and I will provide a test. Thank you for this great suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants