Fix unable to navigate away from 'Timeline' when additional filter is applied #111369

machadoum · 2021-09-07T12:12:06Z

Summary

Fix the redirect bug by using useLocation
Revert "Fix Kibana page crash on redirect navigation when timeline modal is open
Revert "Fix breadcrumbs path reopens timeline when timeline modal is open
update setBreadcrumbs to dispatch an "close timeline" action when clicked.

Quick explanation

After fixing the redirect loop properly, I could revert the previous commits that tried to fix it. I also reverted a fix for closing the modal when breadcrumbs links are clicked because now we can easily dispatch close timeline action without causing a redirect loop.

Long explanation

We have recurrent issues related to closing the timeline and navigating to other pages. The current issue was introduced on the following fix attempt:
#104288

Original issue:
Whenever the redux store changes, useUrlStateHooks updates the URL to sync with the store content. But when we navigate to a different page, it is also called to build the new page query string. If we update the store and navigate to a URL in a small time window, it could create an endless loop of useUrlStateHooks calls because the current URL doesn't get passed down to the router props. So when calling history.replace to update the query string, it would redirect to the previous page.

I tried to fix it before by moving timeline.isOpen dispatch to inside useUrlStateHooks , so the loop wouldn't happen. But that also creates more bugs. Like the ones described below:

Bug one

Bug two

To fix this problem once and for all, I have stepped back. I reverted the two previous fixes, and I am tackling the core issue. When calling history.replace to update a query string in the URL, it will use window.location instead of the path inside router props. This way, the redirect loop won't happen anymore.

How to test it

Open the timeline and try to click on every link to any possible page. It should close the timeline and never crash 🙏

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-09-08T10:51:49Z

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

elasticmachine · 2021-09-08T10:51:50Z

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

machadoum · 2021-09-08T15:12:44Z

@elasticmachine merge upstream

machadoum · 2021-09-15T09:28:25Z

x-pack/plugins/security_solution/public/common/components/endpoint/route_capture.tsx

@@ -18,6 +20,10 @@ export const RouteCapture = memo(({ children }) => {
  const location: AppLocation = useLocation();
  const dispatch = useDispatch();

+  useEffect(() => {


This change is a revert

this dispatch looks like it should be somewhere inside a middleware and not on this component. This component can easily become cluttered with unrelated domain-specific logic if we were to follow this pattern.

is there a timeline id middleware where you can instead listen to the userChangedUrl and dispatch this action there?

I agree with you. This changed was reintroduced because I reverted a fix that had moved it to somewhere else. Today most routing logic of security threat hunting is inside x-pack/plugins/security_solution/public/common/components/url_state/use_url_state.tsx. I am working on refactoring this file, so I will consider moving this change back there.

machadoum · 2021-09-15T09:28:34Z

x-pack/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.test.ts

@@ -438,35 +440,13 @@ describe('Navigation Breadcrumbs', () => {
        },
      ]);
    });
-


This change is a revert

machadoum · 2021-09-15T09:29:21Z

x-pack/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.ts

-      }))
-    );
-  }
+export const useSetBreadcrumbs = () => {


Now that the redirect loop doesn't happen we can easily dispatch(timelineActions.showTimeline to close the timeline

machadoum · 2021-09-15T09:29:49Z

x-pack/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.ts

  getUrlForApp: GetUrlForApp
 ): ChromeBreadcrumb[] | null => {
-  const spyState: RouteSpyState = omit('navTabs', objectParam);


This change is a revert

machadoum · 2021-09-15T09:34:05Z

x-pack/plugins/security_solution/public/common/components/url_state/use_url_state.tsx

  setInitialStateFromUrl,
  updateTimeline,
  updateTimelineIsLoading,
  urlState,
 }: UrlStateContainerPropTypes) => {
  const [isInitializing, setIsInitializing] = useState(true);
  const { filterManager, savedQueries } = useKibana().services.data.query;
+  const { pathname: pathName, search } = useLocation();


Here is the fix! 🎉🍾 🥳
pathName and search were passed as props from the redux store, which gets out of sync with the real pathname in the URL. So later on in the code when we used to call histoty.replace(outdatedPathname, ...). It was redirecting the user to the previous page creating the redirect loop.

machadoum · 2021-09-15T09:34:22Z

x-pack/plugins/security_solution/public/common/components/url_state/use_url_state.tsx

@@ -226,10 +221,9 @@ export const useUrlStateHooks = ({
      });
    } else if (pathName !== prevProps.pathName) {
      handleInitialize(type, isDetectionsPages(pageName));
-      dispatch(timelineActions.showTimeline({ id: TimelineId.active, show: false }));


This line is from a revert

kqualters-elastic · 2021-09-16T14:54:56Z

x-pack/plugins/security_solution/public/common/components/url_state/use_url_state.tsx

+    //  * Redirects as "security/hosts" -> "security/hosts/allHosts"
+    //  * It also happens once on every location change because browserPathName gets updated before pathName
+    // *Warning*: Removing this return would cause redirect loops that crashes the APP.
+    if (browserPathName !== pathName) return;


Whenever the redux store changes, useUrlStateHooks updates the URL to sync with the store content. But when we navigate to a different page, it is also called to build the new page query string. If we update the store and navigate to a URL in a small time window, it could create an endless loop of useUrlStateHooks calls because the current URL doesn't get passed down to the router props. So when calling history.replace to update the query string, it would redirect to the previous page.

… open (elastic#104288)" This reverts commit 1ae7afd.

…open (elastic#101568)" This reverts commit 5b0d325.

…s open" Since the redirect loop is fixed we can easily dispatch an action to close the timeline.

kibanamachine · 2021-09-20T10:34:52Z

💚 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.2MB	4.2MB	+179.0B

History

💔 Build #154156 failed f1aec7e
💚 Build #153833 succeeded 96d06af
💔 Build #153806 failed 8ef0235
💚 Build #153791 succeeded 05d9f99
💔 Build #153498 failed 7eff83c

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

cc @machadoum

paul-tavares

LGTM 🔥 🚢

(ps. thanks for the comment in useUrlState() 👏

paul-tavares · 2021-09-20T12:07:08Z

x-pack/plugins/security_solution/public/common/components/url_state/use_url_state.tsx

@@ -175,6 +172,14 @@ export const useUrlStateHooks = ({
  };

  useEffect(() => {
+    // When browser location and store location are out of sync, skip the execution.


Thanks for the comment - supper helpful for the "future" us 😄

… applied (elastic#111369) * Fix useUrlStateHooks endless redirect loop Whenever the redux store changes, useUrlStateHooks updates the URL to sync with the store content. But when we navigate to a different page, it is also called to build the new page query string. If we update the store and navigate to a URL in a small time window, it could create an endless loop of useUrlStateHooks calls because the current URL doesn't get passed down to the router props. So when calling history.replace to update the query string, it would redirect to the previous page. * Revert "Fix Kibana page crash on redirect navigation when timeline is open (elastic#104288)" This reverts commit 1ae7afd. * Revert "Fix breadcrumbs path reopens timeline when timeline modal is open (elastic#101568)" This reverts commit 5b0d325. * Fix properly "breadcrumbs path reopens timeline when timeline modal is open" Since the redirect loop is fixed we can easily dispatch an action to close the timeline. * Fix eslint issue

kibanamachine · 2021-09-20T12:21:45Z

💔 Backport failed

Status	Branch	Result
✅	7.x
❌	7.15	Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 111369

… applied (#111369) (#112546) * Fix useUrlStateHooks endless redirect loop Whenever the redux store changes, useUrlStateHooks updates the URL to sync with the store content. But when we navigate to a different page, it is also called to build the new page query string. If we update the store and navigate to a URL in a small time window, it could create an endless loop of useUrlStateHooks calls because the current URL doesn't get passed down to the router props. So when calling history.replace to update the query string, it would redirect to the previous page. * Revert "Fix Kibana page crash on redirect navigation when timeline is open (#104288)" This reverts commit 1ae7afd. * Revert "Fix breadcrumbs path reopens timeline when timeline modal is open (#101568)" This reverts commit 5b0d325. * Fix properly "breadcrumbs path reopens timeline when timeline modal is open" Since the redirect loop is fixed we can easily dispatch an action to close the timeline. * Fix eslint issue Co-authored-by: Pablo Machado <pablo.nevesmachado@elastic.co>

… applied (elastic#111369) * Fix useUrlStateHooks endless redirect loop Whenever the redux store changes, useUrlStateHooks updates the URL to sync with the store content. But when we navigate to a different page, it is also called to build the new page query string. If we update the store and navigate to a URL in a small time window, it could create an endless loop of useUrlStateHooks calls because the current URL doesn't get passed down to the router props. So when calling history.replace to update the query string, it would redirect to the previous page. * Revert "Fix Kibana page crash on redirect navigation when timeline is open (elastic#104288)" This reverts commit 1ae7afd. * Revert "Fix breadcrumbs path reopens timeline when timeline modal is open (elastic#101568)" This reverts commit 5b0d325. * Fix properly "breadcrumbs path reopens timeline when timeline modal is open" Since the redirect loop is fixed we can easily dispatch an action to close the timeline. * Fix eslint issue # Conflicts: # x-pack/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.test.ts

… applied (#111369) (#112637) * Fix useUrlStateHooks endless redirect loop Whenever the redux store changes, useUrlStateHooks updates the URL to sync with the store content. But when we navigate to a different page, it is also called to build the new page query string. If we update the store and navigate to a URL in a small time window, it could create an endless loop of useUrlStateHooks calls because the current URL doesn't get passed down to the router props. So when calling history.replace to update the query string, it would redirect to the previous page. * Revert "Fix Kibana page crash on redirect navigation when timeline is open (#104288)" This reverts commit 1ae7afd. * Revert "Fix breadcrumbs path reopens timeline when timeline modal is open (#101568)" This reverts commit 5b0d325. * Fix properly "breadcrumbs path reopens timeline when timeline modal is open" Since the redirect loop is fixed we can easily dispatch an action to close the timeline. * Fix eslint issue # Conflicts: # x-pack/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.test.ts

machadoum self-assigned this Sep 7, 2021

machadoum force-pushed the siem-timeline-issue-109802 branch from cbdad15 to 0f8401d Compare September 8, 2021 10:04

machadoum marked this pull request as ready for review September 8, 2021 10:51

machadoum requested a review from a team as a code owner September 8, 2021 10:51

machadoum marked this pull request as draft September 10, 2021 13:50

machadoum force-pushed the siem-timeline-issue-109802 branch from f3c8b65 to ef5124e Compare September 13, 2021 13:47

machadoum marked this pull request as ready for review September 13, 2021 14:11

machadoum requested a review from a team as a code owner September 13, 2021 14:11

machadoum requested review from pzl and parkiino September 13, 2021 14:11

pzl requested review from paul-tavares and removed request for pzl September 13, 2021 14:20

machadoum force-pushed the siem-timeline-issue-109802 branch 3 times, most recently from 63a9918 to 9330818 Compare September 15, 2021 09:00

machadoum requested review from kqualters-elastic and XavierM September 15, 2021 09:25

machadoum commented Sep 15, 2021

View reviewed changes

machadoum force-pushed the siem-timeline-issue-109802 branch 2 times, most recently from 962333c to 7eff83c Compare September 15, 2021 11:33

machadoum marked this pull request as draft September 15, 2021 12:11

machadoum force-pushed the siem-timeline-issue-109802 branch 2 times, most recently from 05d9f99 to 8ef0235 Compare September 16, 2021 11:44

machadoum marked this pull request as ready for review September 16, 2021 11:44

machadoum force-pushed the siem-timeline-issue-109802 branch 2 times, most recently from 34b259f to 96d06af Compare September 16, 2021 13:24

kqualters-elastic reviewed Sep 16, 2021

View reviewed changes

kqualters-elastic approved these changes Sep 16, 2021

View reviewed changes

machadoum added 4 commits September 20, 2021 09:35

Revert "Fix Kibana page crash on redirect navigation when timeline is…

9240b68

… open (elastic#104288)" This reverts commit 1ae7afd.

Revert "Fix breadcrumbs path reopens timeline when timeline modal is …

38b9a5b

…open (elastic#101568)" This reverts commit 5b0d325.

Fix properly "breadcrumbs path reopens timeline when timeline modal i…

f1aec7e

…s open" Since the redirect loop is fixed we can easily dispatch an action to close the timeline.

machadoum force-pushed the siem-timeline-issue-109802 branch from 96d06af to f1aec7e Compare September 20, 2021 07:37

Fix eslint issue

4a7e485

paul-tavares approved these changes Sep 20, 2021

View reviewed changes

machadoum merged commit 8942c8b into elastic:master Sep 20, 2021

kibanamachine mentioned this pull request Sep 20, 2021

[7.x] Fix unable to navigate away from 'Timeline' when additional filter is applied (#111369) #112546

Merged

machadoum mentioned this pull request Sep 21, 2021

[7.15] Fix unable to navigate away from 'Timeline' when additional filter is applied (#111369) #112637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unable to navigate away from 'Timeline' when additional filter is applied #111369

Fix unable to navigate away from 'Timeline' when additional filter is applied #111369

machadoum commented Sep 7, 2021 •

edited

Loading

elasticmachine commented Sep 8, 2021

elasticmachine commented Sep 8, 2021

machadoum commented Sep 8, 2021

machadoum Sep 15, 2021

academo Sep 20, 2021

machadoum Sep 20, 2021

machadoum Sep 15, 2021

machadoum Sep 15, 2021

machadoum Sep 15, 2021

machadoum Sep 15, 2021

machadoum Sep 15, 2021

kqualters-elastic Sep 16, 2021

kibanamachine commented Sep 20, 2021

paul-tavares left a comment

paul-tavares Sep 20, 2021

kibanamachine commented Sep 20, 2021

@@ @@ -438,35 +440,13 @@ describe('Navigation Breadcrumbs', () => { @@
                       },
                     ]);
                   });

Fix unable to navigate away from 'Timeline' when additional filter is applied #111369

Fix unable to navigate away from 'Timeline' when additional filter is applied #111369

Conversation

machadoum commented Sep 7, 2021 • edited Loading

Summary

Quick explanation

Long explanation

Bug one

Bug two

How to test it

Checklist

elasticmachine commented Sep 8, 2021

elasticmachine commented Sep 8, 2021

machadoum commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Sep 20, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Sep 20, 2021

💔 Backport failed

machadoum commented Sep 7, 2021 •

edited

Loading