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

Refactor(CVEs): Remove Unnecessary Re-render and Code Duplication #2083

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

LiorKGOW
Copy link
Member

@LiorKGOW LiorKGOW commented Mar 19, 2024

This PR refactors some minor things inside the CVEs component

What does this refactor PR include?

  • Remove unnecessary function definition in onSelect
  • Reduce unnecessary re-renders
  • Remove code duplication by creating a helper function: updateRefFuncBuilder

These changes are optional, please let me know what you guys think

@LiorKGOW LiorKGOW self-assigned this Mar 19, 2024
@LiorKGOW LiorKGOW requested a review from a team as a code owner March 19, 2024 12:27
Copy link

jira-linking bot commented Mar 19, 2024

Commits missing Jira IDs:
976712d
ed43705
ca97158
eda8a99

@LiorKGOW LiorKGOW force-pushed the refactor-cves branch 2 times, most recently from b4896ea to 891a7ad Compare March 19, 2024 12:46
@bastilian
Copy link
Member

I think the issue here could be more that the Modals are in a state.

Refactoring this to pull them out of the state and only make them appear via a ...isOpen state would probably be better.

@LiorKGOW
Copy link
Member Author

@bastilian I have looked into the option you suggested, but I am afraid it would require a lot of effort for this kind of refactor then what I thought initially...

I encountered an issue in the last commit in this PR, where I try to send the handleCloseModal down the component tree, but when trying to save the changes in the modal it doesn't do anything

I think what is going on is that it tries to change the businessRisk for some selected CVEs, but when the cvesList changes (to an empty Array) at the CVEs component, it rerenders all the components down the tree which causes the function to stop in the middle and it does not change anything

There is an error message saying that it did not contain any selected CVEs, that is why I think this is the reason it fails...

On master, we created a NEW component every time we open one of those Modals, and what I think happens is that we set the state to be an empty function (empty component) which sort of unmounts it from the component tree, but the component is kept alive until the necessary actions are dispatched and then cleared (Or there is a memory leak where the component keeps on living in the background, which is totally not good)

Do you think this part of the refactor is necessary? Should we invest more time investigating it?

@LiorKGOW LiorKGOW marked this pull request as draft March 22, 2024 09:29
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