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

TypeError: cannot read properties of undefined on the main Agents view #4158

Closed
AlexRuiz7 opened this issue May 18, 2022 · 6 comments · Fixed by #4233, #4250 or #4251
Closed

TypeError: cannot read properties of undefined on the main Agents view #4158

AlexRuiz7 opened this issue May 18, 2022 · 6 comments · Fixed by #4233, #4250 or #4251
Assignees
Labels
type/bug Bug issue

Comments

@AlexRuiz7
Copy link
Member

Wazuh Elastic Rev Security
4.x 7.x 4xxx Basic, ODFE, Xpack
Browser
Chrome, Firefox, Safari, etc

Description
On the Agent view, the navigation to the Inventory Data throws an error only when the Agent view has been previously resized.

Steps to reproduce

  1. Navigate to an Agent's view.
  2. Resize your browser's window.
  3. Open the browser's console.
  4. Navigate to the Inventory Data section.
  5. See the error.

Screenshots

typeerror

@AlexRuiz7 AlexRuiz7 added the type/bug Bug issue label May 18, 2022
@AlexRuiz7
Copy link
Member Author

AlexRuiz7 commented May 18, 2022

Also, on the Inventory Data view, the Agent's name (Ubuntu in this case) becomes a link to the previous view, but it is not colored as a link (should be on a light blue color instead of black).

On hover, the text is shown as a link, but it's hard to guess at first that it's a link. In order to improve the UI/UX, this text should be colored properly, as the rest of the links on the App, and/or add a back arrow ◀️ <.

Link's appearance on its common state and on hover:
image

image

@AlexRuiz7 AlexRuiz7 moved this to Todo in Release 4.3.4 May 18, 2022
@chantal-kelm chantal-kelm self-assigned this May 23, 2022
@AlexRuiz7 AlexRuiz7 moved this from Todo to In Progress in Release 4.3.4 May 25, 2022
@chantal-kelm
Copy link
Member

Debugging I saw that the error occurs inside a function called myRender. I got to see that myRender is called only from the updateVis function and that function is called from:

  • The component Will unmount which is when the component is unmounted

  • And the componentdidupdate which is when the component is updated.

I saw that updateVis executes 2 functions renderComplete and myRender, and what is happening is that when it is executed from the WillUnmount, the updateVis function executes myRender and myRender since it is asynchronous, it passes to the callStack and what is synchronous continues to be executed.
The WillUnmount component then executes the destroyAll function, what destroyAll does is set visHandler to null.
When it finishes doing the synchronous thing, it will do the asynchronous function, this asynchronous function searches inside visHandler but since destroyAll set visHandler to null, it shows the error in the console.
Commenting out the updateVis function inside WillUnmount fixes the problem.
I do not see the need for myRender to be executed when the component is unmounted, but it is necessary for the renderComplete function to be executed.
So what I propose as a solution that does not interfere with the current logic is to add the renderComplete function inside the WillUnmount component.

@AlexRuiz7
Copy link
Member Author

Thanks for the report.

We could schedule a meeting to discuss your proposal.

@Desvelao
Copy link
Member

Desvelao commented May 27, 2022

Research

The error appears when the visHandler.render method is called with a not found DOM node, because the view changed to the Inventory data.

The visHandler.render method is called in the myRender component method and this by updateVis component method.

this.updateVis > ? this.myRender > ? visHandler.render

The updateVis component method is currently called when the component was updated or unmounted and in my opinion, this doesn't have sense because the visualization should be only built when the component is mounted. The current logic causes indirectly the error when it complies with some conditions. In the myRender component method, there is a condition that controls when the visualization should be created and use the visHandler.render method to render it, but this condition is checked when the component is updated or will unmount. The error is caused when the KibanaVis component is unmounted and the visualization was not created, it calls to updateVis > myRender, surpasses the condition, and tries to build the visualization because it wasn't built previously. The method visHandler.render fails because the selected DOM node is not present in the current view for that moment, that is Inventory data section. Moreover, this case does that the visualization is not visible in the panel.

Image

On the other hand, as @chantal-kelm said, it seems that it doesn't have sense to update the visualization when the component is going to be unmounted. We should review if there is something that is required to do before unmounting the component.

@gdiazlo gdiazlo removed this from Release 4.3.4 Jun 1, 2022
@gdiazlo gdiazlo moved this to Todo in Release 4.3.5 Jun 1, 2022
@gdiazlo gdiazlo moved this from Todo to Triage in Release 4.3.5 Jun 1, 2022
@chantal-kelm
Copy link
Member

As mentioned above the component's updateVis() method is called in the componentWillUnmount and executes two methods. One of the methods it executes is myRender which has been determined to not need to be called when the component is unmounted. The other method it executes is renderComplete which, together with my colleague @Desvelao , we investigated if it was necessary to execute or not when the component is unmounted, and in the case of confirming that it was not necessary, we could already eliminate the updateVis() of the componentWillUnmount.
As we saw that this investigation was going to take a lot of time, which we don't have now, we decided to implement the solution proposed in the first comment, which is to remove the updateVis() method that executes myRender and renderComplete from within the componentWillUnmount and execute the renderComplete method within the componentWillUnmount.
It would be good to continue investigating if it is necessary for the renderComplete() to be in the componentWillUnmount. In the renderComplete() the value of the $rootScope is changed, which affects the entire application.

Video where you see the error solved:
https://user-images.githubusercontent.com/99441266/172658878-3eb21f0c-61d8-410a-a6f3-862d96fdf923.mp4

@snaow snaow removed this from Release 4.3.5 Jun 9, 2022
@snaow snaow moved this to Todo in Release 4.4.0 Jun 9, 2022
@Desvelao Desvelao linked a pull request Jun 13, 2022 that will close this issue
@AlexRuiz7
Copy link
Member Author

Related to this issue, React clearly doesn't like our code.

image

Repository owner moved this from Todo to Done in Release 4.4.0 Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment