[Search Sessions] Improve session restoration back button #87635

Dosant · 2021-01-07T14:18:47Z

Summary

Improves back button behavior for search session restoration state
Discover and dashboard because this is only apps we have session restoration flow for now

How to test

Full flow will be testable after #81707 is merged, but for now:

Add a fake searchSessionId to discover or dashboard URL. Load it. The session should fail to restore in this case. (you can also use a real seaerchSessionId using inspector)
Change the state that would trigger a new search. e.g. change time
Press back -> should return you in the previous state with searchSessionId in the URL

demo-back-button.mov

Both for Discover and Dashboard there are different main cases with this navigation that are different from one another and expected to work:

In case navigating away from a restored session together with another state change, like changing time
In case navigating away from a restored session without another state change, like when hitting "refresh"
another case that should work: directly changing the searchSessionId in the browser address bar

Discover has a very specific code path when changing index patterns. This is another code path that is worth testing and it should work.

Implemintation

I fully exposed underlying kbnUrlControls on kbnUrlStorage. Dashboard needs them to remove searchSessionId during pending state sync update.

~~There is a known edge case that if you open a Discover or Dashboard URL and then change only searchSessionId then nothing would happen.~~
~~That is because there is no listener that listens specifically to that query param. Such change is happening in a real-world scenario in case the "refresh" button was hit.~~

~~@Dosant: Todo: try fixing this, see how difficult this is...~~ This if fixed but made implementation more complicated :(

In Dashboard I had to add a denounce on updating dashboard container input to cover the following edge case:

Restoring a session -> navigating away with other state change, for example, with time range change
time is changed in the URL. searchSessionId is removed from the URL.
Hit back
both connectToGlobalQueryState (time) and searchSessionIdQuery$ will ask for dashboard container update. to avoid double fetch in this cases I added debounce.

Notes

Please note when testing in Discover: [Search Sessions] Improve session restoration back button #87635 (comment)

Checklist

Delete any items that are not applicable to this PR.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

…store-search-session-back-button # Conflicts: # src/plugins/discover/public/application/angular/discover.js

Dosant · 2021-01-11T09:25:17Z

@elasticmachine merge upstream

elasticmachine · 2021-01-11T09:27:33Z

Pinging @elastic/app-search-frontend (Team:AppSearch)

elasticmachine · 2021-01-11T09:27:53Z

Pinging @elastic/kibana-app-services (Team:AppServices)

Dosant · 2021-01-11T09:28:43Z

Pinging elastic/app-search-frontend (Team:AppSearch)

Sorry for the ping 😢 confused Team:AppSearch and Team:AppServices

lizozom · 2021-01-12T11:48:59Z

src/plugins/discover/public/application/angular/discover.js

@@ -577,18 +575,21 @@ function discoverController($element, $route, $scope, $timeout, Promise, uiCapab
    abortController = new AbortController();

    const searchSessionId = (() => {
-      const searchSessionIdFromURL = getSearchSessionIdFromURL(history);
+      let searchSessionIdFromURL = getSearchSessionIdFromURL(history);


I'm not a huge fan of this entire block being duplicated.

What do you say we add a function called restoreFromUrl(urlHistory) to the session service?
The function will extract the searchSessionId from the URL and then apply the logic below.

What do you say we add a function called restoreFromUrl(urlHistory) to the session service?

This part is app-specific. It just happens that Discover and Dashboard do this similar:

It is up to the apps how they restore search sessions. Discover and Dashboard URLs structure are similar (both use searchSessionId query param inside hash part of the URL.)

But not every app has a hash in the URL, so they might store searchSessionId as a query param in a pathname (before the hash).

They also might have a param named differently or be serialized in rison/json or other structures.

We will support restoring session without the URL all together: [Search Session] Use new URL Service for session restoration #85126

I'd extract to common place if we'd have a shared place for Discover/Dashboard/Visualize, but as we don't, I think it is fine to leave it as is

lizozom · 2021-01-12T12:38:04Z

I tested the PR
It works as expected in dashboard, but not in discover.
See video

Discover.-.Elastic.3.mp4

…store-search-session-back-button

Dosant · 2021-01-12T16:44:06Z

@lizozom: It works as expected in the dashboard, but not in discover.

Discussed with @lizozom that this is not actually a problem:

The back button triggers the previous search which is expected, but Discover doesn't remove old results from the screen in this case, but the Dashboard does. This is not what we want to address in this pr, this is just how Discover / Dashboard error handling works in case of failed requests.

Instead, we should focus on is that back button triggers the previous search and on a real-world scenario where the session restoration is successful and we see the new results

…store-search-session-back-button

kertal · 2021-01-19T13:43:10Z

ACK, aye, aye, will review

…store-search-session-back-button

…the URL

ThomThomson

Presentation team changes LGTM. Was able to test this locally and confirm that the back button restored the invalid search session.

ThomThomson · 2021-01-20T17:31:45Z

src/plugins/dashboard/public/application/dashboard_app.tsx

@@ -59,7 +59,7 @@ export function DashboardApp({
    indexPatterns: indexPatternService,
  } = useKibana<DashboardAppServices>().services;

-  const [lastReloadTime, setLastReloadTime] = useState(0);
+  const triggerRefresh$ = useMemo(() => new Subject<{ force?: boolean }>(), []);


This is a nice improvement. Much better than using a lastReloadTime to trigger forced resets.

kertal

Kibana App owned code LGTM, tested locally in Chrome, worked correctly. Every time I look at the change index pattern behavior, it gives me headache. Aspirine will be further deangularization.

…store-search-session-back-button # Conflicts: # x-pack/test/send_search_to_background_integration/tests/apps/dashboard/async_search/send_to_background.ts

Dosant · 2021-01-25T08:27:51Z

@elasticmachine merge upstream

Dosant · 2021-01-26T10:16:46Z

@elasticmachine merge upstream

lizozom

LGTM

Tested OSS back behavior
Tested searchSessionId change in URL is picked up without having to refresh the whole page
Navigating back to a restored session works
Navigating back and forth between two restored sessions works
Refresh button properly cleans the searchSessionId
Same tests work on Discover and Dashboard 👍

lizozom · 2021-01-27T12:23:16Z

src/plugins/kibana_utils/public/history/history_observable.ts

@@ -0,0 +1,60 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one


Maybe add unit tests for this?

It is there 😅
https://github.com/elastic/kibana/pull/87635/files/6944ac10f785e16a3c11b3e4d0b41ae9135714da#diff-e92c62c77f010c77c6a8dcd294ca0e506d0f586a695bb08848a1e719e9d6e1ec

Dosant · 2021-01-28T06:11:04Z

@elasticmachine merge upstream

kibanamachine · 2021-01-28T07:49:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2e4854a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	336	337	+1
`kibanaUtils`	191	192	+1
total			+2

Async chunks

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

id	before	after	diff
`dashboard`	158.6KB	159.9KB	+1.3KB
`discover`	411.3KB	412.8KB	+1.5KB
`maps`	2.6MB	2.6MB	+5.0B
total			+2.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dashboard`	295.5KB	295.3KB	-138.0B
`kibanaUtils`	140.8KB	142.1KB	+1.3KB
total			+1.2KB

History

💚 Build #101035 succeeded 6944ac1
💚 Build #100594 succeeded 35a6b8e
💛 Build #100292 was flaky c726a86
💔 Build #100283 failed 8c6d8e7
💚 Build #99948 succeeded 0ce788c

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

)

…y-tests * 'master' of github.com:elastic/kibana: (31 commits) [Discover] Add grid flyout jest test (elastic#89088) [Search Sessions] Improve session restoration back button (elastic#87635) [TSVB] Remove vis_type_timeseries_enhanced plugin (elastic#89274) [Security Solution] Init Osquery plugin (elastic#87109) [Fleet] Do not defined aliases inside datastream template (elastic#89512) skip flaky suite (elastic#86950) chore(NA): bazel machinery installation on kbn bootstrap (elastic#89469) [build/docker] Add support for centos ARM builds (elastic#84831) Convert default_watch.json to a JS object in order to avoid TS complaints (elastic#89488) [CI] Decrease number of Jest workers (elastic#89504) [Maps] remove maps_oss TS project (elastic#89502) Adds migration settings to Docker (elastic#89501) [Lens] Fix crash in transition from unique count to last value (elastic#88916) [kbn-es] Always use bundled JDK when starting Elasticsearch (elastic#89437) unskip getting_started/shakespeare test elasticsearch 64016 (elastic#89346) [Maps] migrate maps, maps_file_upload, and maps_legacy_licensing to TS projects (elastic#89439) skip flaky suite (elastic#89478) skip flaky suite (elastic#89476) skip flaky suite (elastic#89477) skip flaky suite (elastic#89475) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx

…updates-and-timeline-cleanup * 'master' of github.com:elastic/kibana: (44 commits) [Discover] Add grid flyout jest test (elastic#89088) [Search Sessions] Improve session restoration back button (elastic#87635) [TSVB] Remove vis_type_timeseries_enhanced plugin (elastic#89274) [Security Solution] Init Osquery plugin (elastic#87109) [Fleet] Do not defined aliases inside datastream template (elastic#89512) skip flaky suite (elastic#86950) chore(NA): bazel machinery installation on kbn bootstrap (elastic#89469) [build/docker] Add support for centos ARM builds (elastic#84831) Convert default_watch.json to a JS object in order to avoid TS complaints (elastic#89488) [CI] Decrease number of Jest workers (elastic#89504) [Maps] remove maps_oss TS project (elastic#89502) Adds migration settings to Docker (elastic#89501) [Lens] Fix crash in transition from unique count to last value (elastic#88916) [kbn-es] Always use bundled JDK when starting Elasticsearch (elastic#89437) unskip getting_started/shakespeare test elasticsearch 64016 (elastic#89346) [Maps] migrate maps, maps_file_upload, and maps_legacy_licensing to TS projects (elastic#89439) skip flaky suite (elastic#89478) skip flaky suite (elastic#89476) skip flaky suite (elastic#89477) skip flaky suite (elastic#89475) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/lib/absolute_timing_to_relative_timing.ts

…89532)

Dosant added 5 commits January 7, 2021 15:14

wip: search session restoration and back button

e3aa170

clean up

41cbf7d

unit tests

1603eaa

fix

1dbb919

Merge branch 'master' of github.com:elastic/kibana into dev/search/re…

235451a

…store-search-session-back-button # Conflicts: # src/plugins/discover/public/application/angular/discover.js

Merge branch 'master' into dev/search/restore-search-session-back-button

ec36856

Dosant added Feature:Dashboard Dashboard related features Feature:Discover Discover Application Feature:Search Querying infrastructure in Kibana Feature:StateManagement Team:AppSearch v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 11, 2021

Dosant marked this pull request as ready for review January 11, 2021 09:27

Dosant requested a review from a team January 11, 2021 09:27

Dosant requested review from a team as code owners January 11, 2021 09:27

Dosant added Team:AppServices and removed Team:AppSearch labels Jan 11, 2021

lizozom reviewed Jan 12, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into dev/search/re…

a41ee33

…store-search-session-back-button

Dosant requested a review from lizozom January 12, 2021 16:44

Merge branch 'master' of github.com:elastic/kibana into dev/search/re…

44998d5

…store-search-session-back-button

Dosant added 2 commits January 19, 2021 10:47

fix typo in test names

1f55a8e

fix edge case with initial emit

978eb3b

Dosant marked this pull request as ready for review January 19, 2021 12:13

Dosant requested review from ThomThomson and kertal January 19, 2021 13:27

Dosant added 4 commits January 20, 2021 12:21

Merge branch 'master' of github.com:elastic/kibana into dev/search/re…

8076a9b

…store-search-session-back-button

license header

9f35e91

shouldSearchOnPageLoad -> true in case there is a searchSessionid in …

15fa884

…the URL

better comments

0ce788c

ThomThomson approved these changes Jan 20, 2021

View reviewed changes

kertal approved these changes Jan 20, 2021

View reviewed changes

Dosant added 2 commits January 21, 2021 11:47

Merge branch 'master' of github.com:elastic/kibana into dev/search/re…

8c6d8e7

…store-search-session-back-button # Conflicts: # x-pack/test/send_search_to_background_integration/tests/apps/dashboard/async_search/send_to_background.ts

fix bad tests merge

c726a86

Merge branch 'master' into dev/search/restore-search-session-back-button

35a6b8e

Merge branch 'master' into dev/search/restore-search-session-back-button

6944ac1

lizozom approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into dev/search/restore-search-session-back-button

2e4854a

Dosant merged commit 07b210e into elastic:master Jan 28, 2021

Dosant added a commit to Dosant/kibana that referenced this pull request Jan 28, 2021

[Search Sessions] Improve session restoration back button (elastic#87635

1c7213e

)

Dosant mentioned this pull request Jan 28, 2021

[7.x] [Search Sessions] Improve session restoration back button (#87635) #89532

Merged

Dosant added a commit that referenced this pull request Jan 28, 2021

[Search Sessions] Improve session restoration back button (#87635) (#…

bba95e8

…89532)

poffdeluxe mentioned this pull request Feb 1, 2021

Adding Visualize visualization to dashboard from modal directs user to dashboard listing page #89917

Closed

Dosant mentioned this pull request Feb 2, 2021

fix adding Visualize visualization to dashboard from modal directs user to dashboard listing page #90090

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search Sessions] Improve session restoration back button #87635

[Search Sessions] Improve session restoration back button #87635

Dosant commented Jan 7, 2021 •

edited

Loading

Dosant commented Jan 11, 2021

elasticmachine commented Jan 11, 2021

elasticmachine commented Jan 11, 2021

Dosant commented Jan 11, 2021

lizozom Jan 12, 2021

Dosant Jan 12, 2021 •

edited

Loading

lizozom commented Jan 12, 2021

Dosant commented Jan 12, 2021 •

edited

Loading

kertal commented Jan 19, 2021

ThomThomson left a comment

ThomThomson Jan 20, 2021

kertal left a comment

Dosant commented Jan 25, 2021

Dosant commented Jan 26, 2021

lizozom left a comment

lizozom Jan 27, 2021

Dosant Jan 28, 2021

Dosant commented Jan 28, 2021

kibanamachine commented Jan 28, 2021

		@@ -0,0 +1,60 @@
		/*
		* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one

[Search Sessions] Improve session restoration back button #87635

[Search Sessions] Improve session restoration back button #87635

Conversation

Dosant commented Jan 7, 2021 • edited Loading

Summary

How to test

Implemintation

Notes

Checklist

For maintainers

Dosant commented Jan 11, 2021

elasticmachine commented Jan 11, 2021

elasticmachine commented Jan 11, 2021

Dosant commented Jan 11, 2021

lizozom Jan 12, 2021

Choose a reason for hiding this comment

Dosant Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

lizozom commented Jan 12, 2021

Dosant commented Jan 12, 2021 • edited Loading

kertal commented Jan 19, 2021

ThomThomson left a comment

Choose a reason for hiding this comment

ThomThomson Jan 20, 2021

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

Dosant commented Jan 25, 2021

Dosant commented Jan 26, 2021

lizozom left a comment

Choose a reason for hiding this comment

lizozom Jan 27, 2021

Choose a reason for hiding this comment

Dosant Jan 28, 2021

Choose a reason for hiding this comment

Dosant commented Jan 28, 2021

kibanamachine commented Jan 28, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Dosant commented Jan 7, 2021 •

edited

Loading

Dosant Jan 12, 2021 •

edited

Loading

Dosant commented Jan 12, 2021 •

edited

Loading