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

[TableListView] Tries to update React state on unmonted component #159704

Closed
mbondyra opened this issue Jun 14, 2023 · 7 comments
Closed

[TableListView] Tries to update React state on unmonted component #159704

mbondyra opened this issue Jun 14, 2023 · 7 comments
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Component:TableListView Feature:Content Management User generated content (saved objects) management impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@mbondyra
Copy link
Contributor

mbondyra commented Jun 14, 2023

Kibana version:
main

Currently there is only one listing page with 2 tabs, the visualize library.

  1. Open visualize listing page and a dev console.
  2. Switch from Visualization tab to annotation tab - everything works fine
  3. Switch back. You'll get a react warning:
Screenshot 2023-06-14 at 16 24 33
@mbondyra mbondyra added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Jun 14, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

@mbondyra mbondyra changed the title [Listing page] When switching between annotation group tab and visualization tab there's a memory leak [Visualize library listing page] When switching between annotation group tab and visualization tab there's a memory leak Jun 14, 2023
@stratoula
Copy link
Contributor

The problem is here https://github.com/elastic/kibana/blob/main/packages/content-management/table_list_view_table/src/table_list_view_table.tsx#L880

Moving this to the shared-ux team as they own the content management component.

@stratoula stratoula added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) and removed Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jun 19, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@stratoula stratoula changed the title [Visualize library listing page] When switching between annotation group tab and visualization tab there's a memory leak [Content management] When switching between two tabs there's a memory leak Jun 19, 2023
@stratoula stratoula added the Feature:Content Management User generated content (saved objects) management label Jun 19, 2023
@sebelga
Copy link
Contributor

sebelga commented Jun 20, 2023

I will look into it but I am pretty sure we are in this scenario:
facebook/react#22114 (thanks @Dosant for sharing) which would be a "false positive".

There isn't really any memory leak, it's just a state that gets updated whenever the query params changes. Which comes from useQuery (https://github.com/elastic/kibana/blob/main/packages/content-management/table_list_view_table/src/use_url_state.ts#L37) from react-router-dom. I am pretty sure that the listener is removed whenever the component unmounts.

[EDIT]: Looks like the warning has been removed in react 18.

@vadimkibana vadimkibana changed the title [Content management] When switching between two tabs there's a memory leak [TableListView] Tries to update React state on unmonted component Aug 11, 2023
@vadimkibana
Copy link
Contributor

Just a guess, while looking through code on GitHub: it might be solved by checking if component is still mounted before executing this state update line:

@sebelga
Copy link
Contributor

sebelga commented Aug 16, 2023

Just a guess, while looking through code on GitHub: it might be solved by checking if component is still mounted before executing this state update line

Yes the fix is easy. From the React issue I link it just seems that those "check if component mounted before updating state" dance might just be noise (that will be removed the day we upgrade to v18)

@sebelga
Copy link
Contributor

sebelga commented May 3, 2024

Closing this issue as it will be fixed when upgrading to React (#181936)

@sebelga sebelga closed this as completed May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Component:TableListView Feature:Content Management User generated content (saved objects) management impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

No branches or pull requests

5 participants