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 use url state hook #112675

Merged
merged 6 commits into from
Sep 28, 2021
Merged

Conversation

machadoum
Copy link
Member

@machadoum machadoum commented Sep 21, 2021

Summary

This PR aims to simplify useUrlState and improve readability by implementing quick and easy changes (low hanging fruits).

  • There are no big refactor or any visual changes.
  • It only contains one feature change:
    • Delete feature that tried to set the document.title with the page detail name. I deleted this feature because it didn't work correctly.

The most relevant changes are:

  • Refactor useUrlStateHooks by batching updates to location
  • Delete URL_STATE_KEYS constants. Since inside security threat hitting all pages use the same query string.
  • Simplify x-pack/plugins/security_solution/public/common/components/url_state/index.tsx
  • Transform dispatchSetInitialStateFromUrl into a hook named useSetInitialStateFromUrl
  • Refactor use_url_state.tsx
    • Add useCallback to handleInitialize
    • Simplify logic in order to improve readability

What is NOT included:

  • Deleting updateTimeRangelogic
  • Any change to <Spyroute>
  • Use router state instead of routeSpy (I don't think that it is possible)
  • Use the same router and route spy logic for admin and threat hunting pages
  • Move dispatch(timelineActions.showTimeline away from route_capture.tsx
  • Remove history.replace from "replace state to location" and use navigate to app instead
  • Refactor makeMapStateToProps to use hooks as in this commit 7a4029c

How to test it?

All possible query strings/filters:
query, filters, savedQuery, sourcerer, timerange, timeline

When we change one of the previously listed filters we rewrite the URL to contain add them. So if we refresh the page the application should persist the data.
Ex: Open alerts and search for test. The URL will update and it should contain query=(language:kuery,query:test)

It can be tested in many different ways. And it should be tested for every query string. I am listing here what this code is responsible for:

  • URL updates when changing filter/query/sourcerer/timerange/timeline/savedQuery.
  • When navigating from a page to another the state should be persisted and the new page query string should contain the same query string as the previous page.
  • Remove all query string keys from the URL when navigating to admin. they should be restored when navigating back to threat hunting pages.
  • When initializing the application it should add a query string to the URL with the default filters.
  • When loading a page with the query string in the URL it initializes the APP with preselected values corresponding to the query string.

Checklist

Delete any items that are not applicable to this PR.

@machadoum machadoum force-pushed the refactor-useUrlStateHook branch from 84d2671 to 241b6d6 Compare September 21, 2021 15:28
@machadoum machadoum self-assigned this Sep 22, 2021
@machadoum machadoum added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.16.0 v8.0.0 labels Sep 22, 2021
@machadoum machadoum marked this pull request as ready for review September 22, 2021 12:28
@machadoum machadoum requested review from a team as code owners September 22, 2021 12:28
@elasticmachine
Copy link
Contributor

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

@machadoum machadoum force-pushed the refactor-useUrlStateHook branch from c6454df to f88842c Compare September 28, 2021 15:20
@machadoum machadoum removed the request for review from a team September 28, 2021 15:20
Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

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

ran it locally, parts of the app that use url state heavily all seemed to work properly, lgtm 🚀

@machadoum machadoum enabled auto-merge (squash) September 28, 2021 15:52
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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 4.3MB 4.3MB -1.9KB
Unknown metric groups

References to deprecated APIs

id before after diff
securitySolution 877 874 -3

History

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

cc @machadoum

@machadoum machadoum merged commit 2663181 into elastic:master Sep 28, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 28, 2021
* Refactor useUrlStateHooks by batching updates to location

* Delete unused detail name

* Delete URL_STATE_KEYS constants

* Refactor useUrlStateHook

* Fix cypress tests by removing empty AppQuery from URL query string

Fix empty query validation by considering 'query=()' as empty

* Small code readability improvements

It mostly rename variables
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 28, 2021
* Refactor useUrlStateHooks by batching updates to location

* Delete unused detail name

* Delete URL_STATE_KEYS constants

* Refactor useUrlStateHook

* Fix cypress tests by removing empty AppQuery from URL query string

Fix empty query validation by considering 'query=()' as empty

* Small code readability improvements

It mostly rename variables

Co-authored-by: Pablo Machado <pablo.nevesmachado@elastic.co>
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 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants