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

[Search Sessions] Improve session restoration back button #87635

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
e3aa170
wip: search session restoration and back button
Dosant Jan 7, 2021
41cbf7d
clean up
Dosant Jan 7, 2021
1603eaa
unit tests
Dosant Jan 7, 2021
1dbb919
fix
Dosant Jan 7, 2021
235451a
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 7, 2021
ec36856
Merge branch 'master' into dev/search/restore-search-session-back-button
kibanamachine Jan 11, 2021
a41ee33
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 12, 2021
44998d5
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 12, 2021
16cadae
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 12, 2021
9a638da
Merge branch 'master' into dev/search/restore-search-session-back-button
kibanamachine Jan 13, 2021
c1f3d79
Merge branch 'dev/search/restore-search-session-back-button' of githu…
Dosant Jan 15, 2021
573640c
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 15, 2021
01b2f0f
improve
Dosant Jan 15, 2021
c4ff5a1
add tests
Dosant Jan 15, 2021
e75f381
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 18, 2021
3967bdc
fix session id and back button in discover
Dosant Jan 18, 2021
586ed47
remove usused functions
Dosant Jan 18, 2021
419f443
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 18, 2021
931615e
improve
Dosant Jan 18, 2021
db9119d
remove index pattern workaround from session test
Dosant Jan 18, 2021
822a3cc
fix loadOnInitialLoad prop
Dosant Jan 18, 2021
9ff8dab
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 18, 2021
0d2425e
fix initial query param emit
Dosant Jan 19, 2021
d2b0664
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 19, 2021
1f55a8e
fix typo in test names
Dosant Jan 19, 2021
978eb3b
fix edge case with initial emit
Dosant Jan 19, 2021
8076a9b
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 20, 2021
9f35e91
license header
Dosant Jan 20, 2021
15fa884
shouldSearchOnPageLoad -> true in case there is a searchSessionid in …
Dosant Jan 20, 2021
0ce788c
better comments
Dosant Jan 20, 2021
8c6d8e7
Merge branch 'master' of github.com:elastic/kibana into dev/search/re…
Dosant Jan 21, 2021
c726a86
fix bad tests merge
Dosant Jan 21, 2021
35a6b8e
Merge branch 'master' into dev/search/restore-search-session-back-button
kibanamachine Jan 25, 2021
6944ac1
Merge branch 'master' into dev/search/restore-search-session-back-button
kibanamachine Jan 26, 2021
2e4854a
Merge branch 'master' into dev/search/restore-search-session-back-button
kibanamachine Jan 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-plugins-kibana\_utils-public-state\_sync](./kibana-plugin-plugins-kibana_utils-public-state_sync.md) &gt; [IKbnUrlStateStorage](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.md) &gt; [kbnUrlControls](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.kbnurlcontrols.md)

## IKbnUrlStateStorage.kbnUrlControls property

Lower level wrapper around history library that handles batching multiple URL updates into one history change

<b>Signature:</b>

```typescript
kbnUrlControls: IKbnUrlControls;
```
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ export interface IKbnUrlStateStorage extends IStateStorage

| Property | Type | Description |
| --- | --- | --- |
| [cancel](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.cancel.md) | <code>() =&gt; void</code> | cancels any pending url updates |
| [change$](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.change_.md) | <code>&lt;State = unknown&gt;(key: string) =&gt; Observable&lt;State &#124; null&gt;</code> | |
| [flush](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.flush.md) | <code>(opts?: {</code><br/><code> replace?: boolean;</code><br/><code> }) =&gt; boolean</code> | Synchronously runs any pending url updates, returned boolean indicates if change occurred. |
| [get](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.get.md) | <code>&lt;State = unknown&gt;(key: string) =&gt; State &#124; null</code> | |
| [kbnUrlControls](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.kbnurlcontrols.md) | <code>IKbnUrlControls</code> | Lower level wrapper around history library that handles batching multiple URL updates into one history change |
| [set](./kibana-plugin-plugins-kibana_utils-public-state_sync.ikbnurlstatestorage.set.md) | <code>&lt;State&gt;(key: string, state: State, opts?: {</code><br/><code> replace: boolean;</code><br/><code> }) =&gt; Promise&lt;string &#124; undefined&gt;</code> | |

38 changes: 31 additions & 7 deletions src/plugins/dashboard/public/application/dashboard_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@ import {
useSavedDashboard,
} from './hooks';

import { removeQueryParam } from '../services/kibana_utils';
import { IndexPattern } from '../services/data';
import { EmbeddableRenderer } from '../services/embeddable';
import { DashboardContainerInput } from '.';
import { leaveConfirmStrings } from '../dashboard_strings';
import { replaceUrlHashQuery } from '../../../kibana_utils/public';

export interface DashboardAppProps {
history: History;
Expand Down Expand Up @@ -111,19 +111,43 @@ export function DashboardApp({
const shouldRefetch = Object.keys(changes).some(
(changeKey) => !noRefetchKeys.includes(changeKey as keyof DashboardContainerInput)
);
if (getSearchSessionIdFromURL(history)) {
// going away from a background search results
removeQueryParam(history, DashboardConstants.SEARCH_SESSION_ID, true);
}

const newSearchSessionId: string | undefined = (() => {
// do not update session id if this is irrelevant state change to prevent excessive searches
if (!shouldRefetch) return;

let searchSessionIdFromURL = getSearchSessionIdFromURL(history);
if (searchSessionIdFromURL) {
if (
data.search.session.isRestore() &&
data.search.session.getSessionId() === searchSessionIdFromURL
) {
// navigating away from a restored session
dashboardStateManager.kbnUrlStateStorage.kbnUrlControls.updateAsync((nextUrl) => {
if (nextUrl.includes(DashboardConstants.SEARCH_SESSION_ID)) {
return replaceUrlHashQuery(nextUrl, (query) => {
delete query[DashboardConstants.SEARCH_SESSION_ID];
return query;
});
}
return nextUrl;
});
searchSessionIdFromURL = undefined;
} else {
data.search.session.restore(searchSessionIdFromURL);
}
}

return searchSessionIdFromURL ?? data.search.session.start();
})();

if (changes.viewMode) {
setViewMode(changes.viewMode);
}

dashboardContainer.updateInput({
...changes,
// do not start a new session if this is irrelevant state change to prevent excessive searches
...(shouldRefetch && { searchSessionId: data.search.session.start() }),
...(newSearchSessionId && { searchSessionId: newSearchSessionId }),
});
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class DashboardStateManager {
>;
private readonly stateContainerChangeSub: Subscription;
private readonly STATE_STORAGE_KEY = '_a';
private readonly kbnUrlStateStorage: IKbnUrlStateStorage;
public readonly kbnUrlStateStorage: IKbnUrlStateStorage;
private readonly stateSyncRef: ISyncStateRef;
private readonly history: History;
private readonly usageCollection: UsageCollectionSetup | undefined;
Expand Down Expand Up @@ -599,7 +599,7 @@ export class DashboardStateManager {
this.toUrlState(this.stateContainer.get())
);
// immediately forces scheduled updates and changes location
return this.kbnUrlStateStorage.flush({ replace });
return !!this.kbnUrlStateStorage.kbnUrlControls.flush(replace);
}

// TODO: find nicer solution for this
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ describe('sync_query_state_with_url', () => {
test('url is actually changed when data in services changes', () => {
const { stop } = syncQueryStateWithUrl(queryServiceStart, kbnUrlStateStorage);
filterManager.setFilters([gF, aF]);
kbnUrlStateStorage.flush(); // sync force location change
kbnUrlStateStorage.kbnUrlControls.flush(); // sync force location change
expect(history.location.hash).toMatchInlineSnapshot(
`"#?_g=(filters:!(('$state':(store:globalState),meta:(alias:!n,disabled:!t,index:'logstash-*',key:query,negate:!t,type:custom,value:'%7B%22match%22:%7B%22key1%22:%22value1%22%7D%7D'),query:(match:(key1:value1)))),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))"`
);
Expand Down Expand Up @@ -135,7 +135,7 @@ describe('sync_query_state_with_url', () => {

test('when url is changed, filters synced back to filterManager', () => {
const { stop } = syncQueryStateWithUrl(queryServiceStart, kbnUrlStateStorage);
kbnUrlStateStorage.cancel(); // stop initial syncing pending update
kbnUrlStateStorage.kbnUrlControls.cancel(); // stop initial syncing pending update
history.push(pathWithFilter);
expect(filterManager.getGlobalFilters()).toHaveLength(1);
stop();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ export function getState({
}
},
// helper function just needed for testing
flushToUrl: (replace?: boolean) => stateStorage.flush({ replace }),
flushToUrl: (replace?: boolean) => stateStorage.kbnUrlControls.flush(replace),
};
}

Expand Down
21 changes: 11 additions & 10 deletions src/plugins/discover/public/application/angular/discover.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,6 @@ function discoverController($element, $route, $scope, $timeout, Promise, uiCapab
};

const history = getHistory();
// used for restoring a search session
let isInitialSearch = true;

// search session requested a data refresh
subscriptions.add(
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Dosant Dosant Jan 12, 2021

Choose a reason for hiding this comment

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

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

if (searchSessionIdFromURL) {
if (isInitialSearch) {
data.search.session.restore(searchSessionIdFromURL);
isInitialSearch = false;
return searchSessionIdFromURL;
} else {
// navigating away from background search
if (
data.search.session.isRestore() &&
data.search.session.getSessionId() === searchSessionIdFromURL
) {
// navigating away from a restored session
removeQueryParam(history, SEARCH_SESSION_ID_QUERY_PARAM);
searchSessionIdFromURL = undefined;
} else {
data.search.session.restore(searchSessionIdFromURL);
}
}
return data.search.session.start();

return searchSessionIdFromURL ?? data.search.session.start();
})();

$scope
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ export function getState({
setState(appStateContainerModified, defaultState);
},
getPreviousAppState: () => previousAppState,
flushToUrl: () => stateStorage.flush(),
flushToUrl: () => stateStorage.kbnUrlControls.flush(),
isAppStateDirty: () => !isEqualState(initialAppState, appStateContainer.getState()),
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ setTimeout(() => {
}, 0);
```

For cases, where granular control over URL updates is needed, `kbnUrlStateStorage` provides these advanced apis:
For cases, where granular control over URL updates is needed, `kbnUrlStateStorage` exposes `kbnUrlStateStorage.kbnUrlControls` that exposes these advanced apis:

- `kbnUrlStateStorage.flush({replace: boolean})` - allows to synchronously apply any pending updates.
`replace` option allows to use `history.replace()` instead of `history.push()`. Returned boolean indicates if any update happened
- `kbnUrlStateStorage.cancel()` - cancels any pending updates
- `kbnUrlStateStorage.kbnUrlControls.flush({replace: boolean})` - allows to synchronously apply any pending updates.
`replace` option allows using `history.replace()` instead of `history.push()`.
- `kbnUrlStateStorage.kbnUrlControls.cancel()` - cancels any pending updates.

### Sharing one `kbnUrlStateStorage` instance

Expand Down
2 changes: 2 additions & 0 deletions src/plugins/kibana_utils/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ export {
getStatesFromKbnUrl,
setStateToKbnUrl,
withNotifyOnErrors,
replaceUrlHashQuery,
replaceUrlQuery,
} from './state_management/url';
export {
syncState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@ export {
export { createKbnUrlTracker } from './kbn_url_tracker';
export { createUrlTracker } from './url_tracker';
export { withNotifyOnErrors, saveStateInUrlErrorTitle, restoreUrlErrorTitle } from './errors';
export { replaceUrlHashQuery, replaceUrlQuery } from './format';
6 changes: 2 additions & 4 deletions src/plugins/kibana_utils/public/state_sync/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,12 @@ export const createSessionStorageStateStorage: (storage?: Storage) => ISessionSt

// @public
export interface IKbnUrlStateStorage extends IStateStorage {
cancel: () => void;
// (undocumented)
change$: <State = unknown>(key: string) => Observable<State | null>;
flush: (opts?: {
replace?: boolean;
}) => boolean;
// (undocumented)
get: <State = unknown>(key: string) => State | null;
// Warning: (ae-forgotten-export) The symbol "IKbnUrlControls" needs to be exported by the entry point index.d.ts
kbnUrlControls: IKbnUrlControls;
// (undocumented)
set: <State>(key: string, state: State, opts?: {
replace: boolean;
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/kibana_utils/public/state_sync/state_sync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ describe('state_sync', () => {
expect(history.length).toBe(startHistoryLength);
expect(getCurrentUrl()).toMatchInlineSnapshot(`"/"`);

urlSyncStrategy.flush();
urlSyncStrategy.kbnUrlControls.flush();

expect(history.length).toBe(startHistoryLength + 1);
expect(getCurrentUrl()).toMatchInlineSnapshot(
Expand Down Expand Up @@ -301,7 +301,7 @@ describe('state_sync', () => {
expect(history.length).toBe(startHistoryLength);
expect(getCurrentUrl()).toMatchInlineSnapshot(`"/"`);

urlSyncStrategy.cancel();
urlSyncStrategy.kbnUrlControls.cancel();

expect(history.length).toBe(startHistoryLength);
expect(getCurrentUrl()).toMatchInlineSnapshot(`"/"`);
Expand Down
Loading