Skip to content

Conversation

@XavierM
Copy link
Contributor

@XavierM XavierM commented Feb 10, 2021

Summary

We found a problem around our paradigm in search strategy, the problem was that useCallback does not know when a component is going unmounted, however useEffect does know all the secrets/ life cycle of a component therefore we are taking advantage of it. Also, in the mean time we did simplify our logic to use unsubscribe method instead of using didCancel. We also had a bug that we did not setLoading to false when we get an error from our request.

@XavierM XavierM added bug Fixes for quality problems that affect the customer experience v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Team:Threat Hunting Security Solution Threat Hunting Team v7.11.1 labels Feb 10, 2021
@XavierM XavierM self-assigned this Feb 10, 2021
@XavierM XavierM requested a review from a team as a code owner February 10, 2021 17:29
@elasticmachine
Copy link
Contributor

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

@XavierM XavierM force-pushed the bug-cancel-request-search-strategy branch from c97f0d1 to fd53046 Compare February 10, 2021 21:19
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

LGTM! Some nit comments:

  1. For readability reasons I think didCancel.current = false; is better to be along with
abortCtrl.current.abort();
asyncSearch();
refetch.current = asyncSearch;

after the declaration of the asyncSearch function. I think it makes more clear the order of the actions.

Example:

didCancel.current = false;
abortCtrl.current.abort();
asyncSearch();
refetch.current = asyncSearch;
  1. Why we setLoading(false); only if !(msg instanceof AbortError)? Shouldn't we do it on any error? If the user aborts the component will stuck on a loading state, right?

  2. What do you think about putting searchSubscription$.unsubscribe(); inside the clean-up function of useEffect?

Example:

return () => {
	searchSubscription$.unsubscribe();
	didCancel.current = true;
	abortCtrl.current.abort();
};

@cnasikas

This comment has been minimized.

@cnasikas

This comment has been minimized.

@XavierM XavierM removed the v7.11.1 label Feb 22, 2021
@elastic elastic deleted a comment from kibanamachine Feb 22, 2021
@XavierM XavierM enabled auto-merge (squash) February 22, 2021 19:09
@XavierM XavierM disabled auto-merge February 22, 2021 19:09
@XavierM XavierM added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 22, 2021
@kibanamachine
Copy link
Contributor

kibanamachine commented Feb 22, 2021

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / general / can be marked as favorite.Timelines Creates a timeline by clicking untitled timeline from bottom bar can be marked as favorite

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: Expected to find element: `[data-test-subj="timeline-favorite-empty-star"]`, but never found it.
    markAsFavorite@http://localhost:6121/__cypress/tests?p=cypress/integration/timelines/creation.spec.ts:15930:8
    ./cypress/integration/timelines/creation.spec.ts/</</<@http://localhost:6121/__cypress/tests?p=cypress/integration/timelines/creation.spec.ts:15072:24
    setRunnable/runnable.fn@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:170493:45
    callFn@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:104252:21
    onRunnableRun/<@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:176051:28
    finallyHandler@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:7163:23
    tryCatcher@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:10609:23
    ../../node_modules/bluebird/js/release/promise.js/</module.exports/Promise.prototype._settlePromiseFromHandler@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:8544:31
    ../../node_modules/bluebird/js/release/promise.js/</module.exports/Promise.prototype._settlePromise@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:8601:18
    ../../node_modules/bluebird/js/release/promise.js/</module.exports/Promise.prototype._settlePromise0@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:8646:10
    ../../node_modules/bluebird/js/release/promise.js/</module.exports/Promise.prototype._settlePromises@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:8726:18
    _drainQueueStep@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:5316:12
    _drainQueue@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:5309:24
    ../../node_modules/bluebird/js/release/async.js/</Async.prototype._drainQueues@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:5325:16
    Async/this.drainQueues@http://elastic:changeme@localhost:6121/__cypress/runner/cypress_runner.js:5195:14


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 7.7MB 7.7MB +4.4KB

History

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

cc @XavierM

@XavierM XavierM merged commit eb21f57 into elastic:master Feb 23, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 23, 2021
…tegy query (elastic#90982)

* fix paradigm around our serach strategy query

* simplify logic to act with search strategy

* miss to delete a declaration
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.12 / #92332

Successful backport PRs will be merged automatically after passing CI.

XavierM added a commit to XavierM/kibana that referenced this pull request Feb 23, 2021
…tegy query (elastic#90982)

* fix paradigm around our serach strategy query

* simplify logic to act with search strategy

* miss to delete a declaration
XavierM added a commit to XavierM/kibana that referenced this pull request Feb 23, 2021
…tegy query (elastic#90982)

* fix paradigm around our serach strategy query

* simplify logic to act with search strategy

* miss to delete a declaration
kibanamachine added a commit that referenced this pull request Feb 23, 2021
…tegy query (#90982) (#92332)

* fix paradigm around our serach strategy query

* simplify logic to act with search strategy

* miss to delete a declaration

Co-authored-by: Xavier Mouligneau <[email protected]>
XavierM added a commit that referenced this pull request Feb 23, 2021
…tegy query (#90982) (#92335)

* fix paradigm around our serach strategy query

* simplify logic to act with search strategy

* miss to delete a declaration
XavierM added a commit that referenced this pull request Feb 23, 2021
…tegy query (#90982) (#92336)

* fix paradigm around our serach strategy query

* simplify logic to act with search strategy

* miss to delete a declaration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.12.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants