[Discover] Add container for internal state #144149

kertal · 2022-10-27T23:20:18Z

Summary

This PR introduces a new state container for the internal state, that's the state used in several components but not synced with the URL (While appState and globalState are synced with the URL). For a start, it contains the currently selected dataView, savedDataViews, and the list of currently available ad-hoc data views adHocDataViews.

export interface InternalState {
  dataView: DataView | undefined;
  savedDataViews: DataViewListItem[];
  adHocDataViews: DataView[];
}

This new state container is part of DiscoverStateContainer (formerly known as GetStateReturn), preparing for the consolidation of various state-related code in a central container drafted in #140765.

Additionally this PR adds more usage of selector hooks, allowing a more granular way to update parts of the UI when there are changes of state, and cleans up ad-hoc data related code.

This PR allows us to move forward to enable editing of data views directly in Discover like Lens #142723

Resolves #145074

Testing

This is a refactoring, everything should work like before

Checklist

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

…-ref HEAD~1..HEAD --fix'

…to discover-internal-state

…tate

[Discover] Make adhoc data view logic more explicit

kertal · 2022-11-04T08:16:01Z

src/plugins/discover/public/application/main/services/discover_internal_state_container.ts

+export function getInternalStateContainer() {
+  return createStateContainer<InternalState, InternalStateTransitions, {}>(
+    {
+      dataView: undefined,
+      dataViewAdHocList: [],
+    },
+    {
+      setDataView: (prevState: InternalState) => (nextDataView: DataView) => ({
+        ...prevState,
+        dataView: nextDataView,
+      }),
+      appendAdHocDataView: (prevState: InternalState) => (dataViewAdHoc: DataView) => ({
+        ...prevState,
+        dataViewAdHocList: prevState.dataViewAdHocList.concat(dataViewAdHoc),
+      }),
+      removeAdHocDataViewById: (prevState: InternalState) => (id: string) => ({
+        ...prevState,
+        dataViewAdHocList: prevState.dataViewAdHocList.filter((dataView) => dataView.id !== id),
+      }),
+      replaceAdHocDataViewWithId:
+        (prevState: InternalState) => (prevId: string, newDataView: DataView) => ({
+          ...prevState,
+          dataViewAdHocList: prevState.dataViewAdHocList.map((dataView) =>
+            dataView.id === prevId ? newDataView : dataView
+          ),
+        }),
+    },
+    {},
+    { freeze: (state) => state }
+  );
+}


Here is where the internal state container is created

kertal · 2022-11-04T08:17:03Z

src/plugins/discover/public/application/main/services/discover_internal_state_container.ts

+  InternalStateTransitions
+>;
+
+export const { Provider: InternalStateProvider, useSelector: useInternalStateSelector } =


This is where Provider / useSelector hook are initialized

kertal · 2022-11-04T08:19:11Z

src/plugins/discover/public/application/main/discover_main_app.tsx

@@ -101,10 +98,8 @@ export function DiscoverMainApp(props: DiscoverMainProps) {
  useSavedSearchAliasMatchRedirect({ savedSearch, spaces, history });

  return (
-    <DiscoverAppStateProvider value={stateContainer.appStateContainer}>
+    <DiscoverMainProvider value={stateContainer}>


Here's where AppState & InternalState providers get the stateContainer assigned

kertal · 2022-11-04T08:21:22Z

src/plugins/discover/public/application/main/services/discover_state_provider.tsx

+    <DiscoverStateProvider value={value}>
+      <DiscoverAppStateProvider value={value.appState}>
+        <InternalStateProvider value={value.internalState}>{children}</InternalStateProvider>
+      </DiscoverAppStateProvider>
+    </DiscoverStateProvider>


Combining multiple providers to a single one, more convenient when writing tests

elasticmachine · 2022-11-04T09:59:30Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

kertal · 2022-11-07T10:34:59Z

@elasticmachine merge upstream

kertal · 2022-12-20T19:53:53Z

dear @mattkime if you have the resources, could you review this PR? But be careful! Everbody had to go on well deserved vacation during the reviews 😄 .... this is a so called vacationizer PR 🌴

kertal · 2022-12-21T08:39:31Z

@elasticmachine merge upstream

kertal · 2022-12-22T07:20:57Z

src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.test.tsx

    expect(updatedDataView!.id).toEqual('updated-mock-id');
  });
-
-  it('should update the adHocList correctly for text based mode', async () => {


this is list is no longer stored in this hook, moved to internal state, that's why I removed this test

stratoula

Unified search changes LGTM! I did some tests on text based mode and works as expected 👏

dimaanj

Tested locally adhoc data views related functionality one more time and it works as expected!

mattkime · 2022-12-28T18:37:49Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

-    () => getRawRecordType(state.query) === RecordRawType.PLAIN,
-    [state.query]
-  );
+  // this is needed to prevent data view pushing onSort because there has been a state change with a new data view


I'd like to walk through the code so you can show me this. It sounds like a good test of whether I understand this code. Also, it sounds like a flaw in the state model.

I've improved documentation why this is written that way .. generally it's fixing a situation when switching between data views with different timestamps, by showing the loading state for this cases. Not saying this is ideal. I've improved the documentation for this, and happy to have a chat about it for sure 👍

src/plugins/discover/public/application/main/discover_main_route.tsx

mattkime

I left a couple of comments. While this PR is very big, it seems like a large number of small changes. Lets just touch base before I give the official 👍 since I'm still new to this code.

kertal · 2022-12-29T10:18:19Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

+  // 1. When switching the data view, the sorting in the URL is reset to the default sorting of the selected data view.
+  // 2. The new sort param is already available in this component and propagated to the EuiDataGrid.
+  // 3. currentColumns are still referring to the old state
+  // 4. since the new sort by field isn't available in currentColumns EuiDataGrid is emitting a 'onSort', which is unsorting the grid


Here's where EuiDataGrid calls the onSort event:

https://github.com/elastic/eui/blob/dff7a774b7065119838a3791ae77c9536f2ccf4c/src/components/datagrid/controls/column_sorting.tsx#L41-L59

kertal · 2022-12-29T10:21:54Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

-  );
+  const isDataLoading = documentState.fetchStatus === FetchStatus.LOADING;
+  // This is needed to prevent EuiDataGrid pushing onSort because the data view has been switched.
+  // 1. When switching the data view, the sorting in the URL is reset to the default sorting of the selected data view.


Here's the logic what's happening with the state when a data view is switched:

kibana/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.ts

Lines 38 to 58 in 1a213bd

// when switching from an data view with timeField to an data view without timeField

// filter out sorting by timeField in case it is set. data views without timeField don't

// prepend this field in the table, so in legacy grid you would need to add this column to

// remove sorting

let nextSort = getSortArray(currentSort, nextDataView).filter((value) => {

return nextDataView.timeFieldName || value[0] !== currentDataView.timeFieldName;

});

if (nextDataView.isTimeBased() && !nextSort.length) {

// set default sorting if it was not set

nextSort = [[nextDataView.timeFieldName, sortDirection]];

} else if (

nextDataView.isTimeBased() &&

currentDataView.isTimeBased() &&

nextDataView.timeFieldName !== currentDataView.timeFieldName

) {

// switch time fields

nextSort = nextSort.map((cur) =>

cur[0] === currentDataView.timeFieldName ? [nextDataView.timeFieldName, cur[1]] : cur

);

}

mattkime

changes look good to me and work well, nice work!

…tate

kibana-ci · 2022-12-30T08:04:49Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	442	448	+6

Async chunks

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

id	before	after	diff
`discover`	414.2KB	407.3KB	-7.0KB
`unifiedSearch`	273.3KB	273.3KB	+1.0B
total			-6.9KB

Page load bundle

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

id	before	after	diff
`discover`	28.0KB	28.0KB	+29.0B

Unknown metric groups

async chunk count

id	before	after	diff
`discover`	9	10	+1

References to deprecated APIs

id	before	after	diff
`discover`	35	30	-5

History

💔 Build #97480 failed d0bc3d4
💔 Build #97465 failed 1ab6046
💚 Build #97422 succeeded fb71953
💚 Build #97350 succeeded ce9cc2a
💚 Build #97337 succeeded 177e1b3

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

cc @kertal

kertal and others added 4 commits October 28, 2022 01:19

Initial commit

5ab45e2

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

2557d20

…-ref HEAD~1..HEAD --fix'

Remove dataViews from internal state since it is not used

67c1d23

Merge branch 'discover-internal-state' of github.com:kertal/kibana in…

838b95f

…to discover-internal-state

kertal self-assigned this Oct 28, 2022

kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Oct 28, 2022

kertal changed the title ~~[Discover] Add internal state~~ [Discover] Add container for internal state Oct 28, 2022

kertal mentioned this pull request Oct 28, 2022

[Discover] Refactor and simplify state container #142131

Closed

5 tasks

kertal and others added 8 commits October 29, 2022 09:36

Fix tests

d74ab22

Fix more tests

43c06f9

Merge remote-tracking branch 'upstream/main' into discover-internal-s…

c2fe3f5

…tate

Refactor AppState type location

9404801

[Discover] make adhoc data view logic more explicit

5e784c3

[Discover] remove log

badaf79

Merge pull request #14 from dimaanj/make-adhoc-logic-more-explicit

0fd1b90

[Discover] Make adhoc data view logic more explicit

Rename dataViewsAdHoc to dataViewAdHocList

a01df77

kertal commented Nov 4, 2022

View reviewed changes

fix conflicts

2f482d7

kertal marked this pull request as ready for review November 4, 2022 09:59

kertal requested a review from a team as a code owner November 4, 2022 09:59

Fix context_app.tsx

ad88bff

Merge branch 'main' into discover-internal-state

95483dd

kertal added 4 commits December 21, 2022 11:09

Merge branch 'main' into discover-internal-state

8b75e85

fix conflicts

6fe5cd2

Fix type errors

06c6bb6

Fix jest test

475033b

kertal commented Dec 22, 2022

View reviewed changes

stratoula removed the ci:cloud-deploy Create or update a Cloud deployment label Dec 22, 2022

stratoula approved these changes Dec 22, 2022

View reviewed changes

kertal added the chore label Dec 22, 2022

dimaanj approved these changes Dec 23, 2022

View reviewed changes

mattkime reviewed Dec 28, 2022

View reviewed changes

src/plugins/discover/public/application/main/discover_main_route.tsx Outdated Show resolved Hide resolved

mattkime requested changes Dec 28, 2022

View reviewed changes

kertal added 3 commits December 29, 2022 08:31

fix conflicts

8c8e92e

apply review suggestion

177e1b3

Improve documentation

ce9cc2a

kertal commented Dec 29, 2022

View reviewed changes

kertal requested a review from mattkime December 29, 2022 13:44

Merge main, fix conflicts

fb71953

mattkime approved these changes Dec 29, 2022

View reviewed changes

kertal added 4 commits December 29, 2022 21:41

Add functional test

1ab6046

Add missing await to functional test

d0bc3d4

Fix functional test

ca6926a

Merge remote-tracking branch 'upstream/main' into discover-internal-s…

1c73ea9

…tate

kertal merged commit f87ee1d into elastic:main Dec 30, 2022

kibanamachine added the backport:skip This commit does not require backporting label Dec 30, 2022

kertal mentioned this pull request Dec 30, 2022

[Discover] Enable Data View edit from flyout #147772

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Add container for internal state #144149

[Discover] Add container for internal state #144149

kertal commented Oct 27, 2022 •

edited

Loading

kertal Nov 4, 2022

kertal Nov 4, 2022

kertal Nov 4, 2022 •

edited

Loading

kertal Nov 4, 2022 •

edited

Loading

elasticmachine commented Nov 4, 2022

kertal commented Nov 7, 2022

kertal commented Dec 20, 2022

kertal commented Dec 21, 2022

kertal Dec 22, 2022

stratoula left a comment

dimaanj left a comment

mattkime Dec 28, 2022

kertal Dec 29, 2022

mattkime left a comment

kertal Dec 29, 2022

kertal Dec 29, 2022

mattkime left a comment

kibana-ci commented Dec 30, 2022

async chunk count

References to deprecated APIs

	// when switching from an data view with timeField to an data view without timeField
	// filter out sorting by timeField in case it is set. data views without timeField don't
	// prepend this field in the table, so in legacy grid you would need to add this column to
	// remove sorting
	let nextSort = getSortArray(currentSort, nextDataView).filter((value) => {
	return nextDataView.timeFieldName \|\| value[0] !== currentDataView.timeFieldName;
	});

	if (nextDataView.isTimeBased() && !nextSort.length) {
	// set default sorting if it was not set
	nextSort = [[nextDataView.timeFieldName, sortDirection]];
	} else if (
	nextDataView.isTimeBased() &&
	currentDataView.isTimeBased() &&
	nextDataView.timeFieldName !== currentDataView.timeFieldName
	) {
	// switch time fields
	nextSort = nextSort.map((cur) =>
	cur[0] === currentDataView.timeFieldName ? [nextDataView.timeFieldName, cur[1]] : cur
	);
	}

[Discover] Add container for internal state #144149

[Discover] Add container for internal state #144149

Conversation

kertal commented Oct 27, 2022 • edited Loading

Summary

Testing

Checklist

kertal Nov 4, 2022

Choose a reason for hiding this comment

kertal Nov 4, 2022

Choose a reason for hiding this comment

kertal Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

kertal Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 4, 2022

kertal commented Nov 7, 2022

kertal commented Dec 20, 2022

kertal commented Dec 21, 2022

kertal Dec 22, 2022

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

dimaanj left a comment

Choose a reason for hiding this comment

mattkime Dec 28, 2022

Choose a reason for hiding this comment

kertal Dec 29, 2022

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

kertal Dec 29, 2022

Choose a reason for hiding this comment

kertal Dec 29, 2022

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

kibana-ci commented Dec 30, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

References to deprecated APIs

History

kertal commented Oct 27, 2022 •

edited

Loading

kertal Nov 4, 2022 •

edited

Loading

kertal Nov 4, 2022 •

edited

Loading