Skip to content

[Security solution] Fix side effects in useRefetchByRestartingSession#148591

Closed
stephmilovic wants to merge 10 commits intoelastic:mainfrom
stephmilovic:fix_async_side_effects
Closed

[Security solution] Fix side effects in useRefetchByRestartingSession#148591
stephmilovic wants to merge 10 commits intoelastic:mainfrom
stephmilovic:fix_async_side_effects

Conversation

@stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Jan 9, 2023

Summary

I noticed the below console error on the network page and tracked it down to the searchSessionId variable in useRefetchByRestartingSession which was previously stored in a useMemo. The hook calls data.search.session.start() which returns the id. However, this call was running before the component was mounted and react did not like it, resulting in the error. Fixed by storing the id in useState and updating the state with the start() call once the component is mounted

Warning: Cannot update a component (`Unknown`) while rendering a different component (`NetworkKpiUniquePrivateIpsComponent`). To locate the bad setState() call inside `NetworkKpiUniquePrivateIpsComponent`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
    at NetworkKpiUniquePrivateIpsComponent (http://localhost:5601/kbn/9007199254740991/bundles/plugin/securitySolution/8.0.0/securitySolution.chunk.lazy_sub_plugins.js:185537:3)
    at div
    ... very long log

I also moved this hook to be in public/explore in accordance with #147298

@stephmilovic stephmilovic added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore v8.7.0 labels Jan 9, 2023
setSearchSessionId(session.current.start());
const currentSession = session.current;
return () => {
data.search.session.clear();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to use the session.current ref since its the only place we're not using the ref. I think it was done like this in the first place because of the following error:

ESLint: The ref value 'session.current' will likely have changed by the time this effect cleanup function runs. If this ref points to a node rendered by React, copy 'session.current' to a variable inside the effect, and use that variable in the cleanup function.(react-hooks/exhaustive-deps)

Thus the variable on L62

currentSession.clear();
};
});
}, []);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react wants empty dependency argument now that we're using a setState call in the effect

@stephmilovic stephmilovic marked this pull request as ready for review January 9, 2023 19:37
@stephmilovic stephmilovic requested review from a team as code owners January 9, 2023 19:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

indexNames,
startDate: from,
skip: querySkip,
skip: querySkip || isChartEmbeddablesEnabled,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I happened to notice this was the only chart embeddables query without this condition, I think this is correct??

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the check!

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link

kibana-ci commented Jan 9, 2023

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #4 / Add exception using data views from rule details Creates an exception item from alert actions overflow menu
  • [job] [logs] FTR Configs #21 / alerting api integration security and spaces enabled - Group 2 Alerts alerts alerts space_1_all_with_restricted_fixture at space1 should schedule task, run alert and schedule actions when appropriate
  • [job] [logs] Security Solution Tests #1 / Detections : Page Filters Alert list is updated when the alerts are updated
  • [job] [logs] Security Solution Tests #1 / Detections : Page Filters URL is updated when filters are updated
  • [job] [logs] Security Solution Tests #3 / Timelines Creates a timeline by clicking untitled timeline from bottom bar can be added notes
  • [job] [logs] Security Solution Tests #3 / Timelines Creates a timeline by clicking untitled timeline from bottom bar can be added notes

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 12.7MB 12.7MB +22.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@jamster10 jamster10 left a comment

Choose a reason for hiding this comment

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

Good solution, thank you!

@stephmilovic
Copy link
Contributor Author

closing in favor of #148552

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

Labels

release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants