[data view editor] Form state refactor - move all state logic into service and out of react state management by mattkime · Pull Request #143604 · elastic/kibana

mattkime · 2022-10-18T22:55:49Z

Summary

This PR completes the data view editor state service. Of note:

All form business logic is now in service although validation is still handled by the form lib
Service is initialized with dependencies, form config, and initial state
Service state is updated via simple method calls
Service state updates are provided to React code via observables using useObservable
Service is provided via parent component which uses useRef so the service is created once per flyout creation

IMO the diff isn't very helpful. It might be easier to review the changed files in completion.

Follow up to #142421

src/plugins/data_view_editor/public/components/form_fields/title_field.tsx

…-fix'

src/plugins/data_view_editor/public/components/data_view_editor_flyout_content.tsx

…kime/kibana into reduce_react_state_data_view_create

src/plugins/data_view_editor/public/components/form_fields/title_field.tsx

…eanup

mattkime · 2022-11-01T17:39:59Z

src/plugins/data_view_editor/public/components/data_view_editor_flyout_content.tsx

-          showAllIndices: allowHidden,
-        });
+    dataViewEditorService.setIndexPattern(title);
+  }, [dataViewEditorService, title]);


setting state

mattkime · 2022-11-01T17:40:55Z

src/plugins/data_view_editor/public/components/data_view_editor_flyout_content.tsx

-  const isLoadingDataViewNames$ = useRef(new BehaviorSubject<boolean>(true));
-  const existingDataViewNames$ = useRef(new BehaviorSubject<string[]>([]));
-  const isLoadingDataViewNames = useObservable(isLoadingDataViewNames$.current, true);
+  const existingDataViewNames = useObservable(dataViewEditorService.dataViewNames$);


getting state

mattkime · 2022-11-01T17:48:58Z

src/plugins/data_view_editor/public/data_view_editor_service.ts

+  // alernates between value and undefined so validation can treat new value as thought its a promise
+  rollupIndexForProvider$ = new Subject<string | undefined | null>();
+
+  matchedIndices$ = new BehaviorSubject<MatchedIndicesSet>(matchedIndiciesDefault);


public observables for react code to subscribe to

mattkime · 2022-11-01T17:49:20Z

src/plugins/data_view_editor/public/data_view_editor_service.ts

+
+  setIndexPattern = (indexPattern: string) => {
+    this.indexPattern = indexPattern;
+    this.loadIndices();


simple state update that kicks off observable updates

mattkime · 2022-11-02T05:03:50Z

src/plugins/data_view_editor/public/components/data_view_flyout_content_container.tsx

    services: { dataViews, notifications, http },
  } = useKibana<DataViewEditorContext>();

+  const dataViewEditorService = useRef(


The intention is to have a stable reference to a single service for the life of the component.

…kime/kibana into reduce_react_state_data_view_create

Dosant

Recent changes to the service initialization lgtm 👍

I started testing,
Noticed that you edit a data view, press save, then it goes into an indefinite "loading" state. In main it simply saves and closes the flyout. I assume that the culprit might cause other similar bugs

Also seem to be the same problem for editing and ad-hoc view, even after doing some changes,

mattkime · 2022-11-05T04:06:09Z

@Dosant I resolved that issue - the index pattern field wasn't being updated and therefore an observable wasn't getting a new value which was necessary for validation. I added a line of code which generates the new value.

I think there's a bit of a mismatch between form lib and using observables for state. In the long run I'd like to rectify this but I see a number of conversations around state management before that could happen.

Dosant

Retested, seem to be working well

…kime/kibana into reduce_react_state_data_view_create

jughosta

LGTM 👍
Thanks for the refactoring! Left some suggestions which can be addressed later if relevant.

jughosta · 2022-11-28T09:25:21Z

src/plugins/data_view_editor/public/data_view_editor_service.ts

+
+  // state
+  private indexPattern = '';
+  private allowHidden = false;


Would be great if allowHidden becomes enabled when editing a saved data view for hidden indices.

Steps:

Create a data view for hidden indices

Open it in Edit mode and see that it can't find matching indices unless the switch is toggled manually again.

Also it does not restore a saved time field value:

I'll address these in a follow up PR.

jughosta · 2022-11-28T09:28:23Z

src/plugins/data_view_editor/public/data_view_editor_service.ts

+      name: initialName = '',
+    },
+    requireTimestampField = false,
+  }: DataViewEditorServiceConstructorArgs) {


A redundant error message is being briefly shown when submitting a valid form.

For example after editing a time field:

…kime/kibana into reduce_react_state_data_view_create

kibana-ci · 2022-11-29T01:16:18Z

💚 Build Succeeded

Buildkite Build
Commit: 659dcd3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dataViewEditor`	63	62	-1

Async chunks

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

id	before	after	diff
`dataViewEditor`	34.8KB	35.5KB	+769.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	109	115	+6
`securitySolution`	442	448	+6
total			+20

References to deprecated APIs

id	before	after	diff
`dataViewEditor`	5	0	-5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	68	74	+6
`osquery`	110	117	+7
`securitySolution`	519	525	+6
total			+21

History

💚 Build #90824 succeeded 3f0e539
💚 Build #90815 succeeded b597595
💔 Build #90790 failed 054c1e6
💔 Build #88679 failed 5fb1dde
💔 Build #88284 failed fbd0594
💔 Build #88277 failed 0299122

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

kibanamachine · 2022-11-29T01:20:33Z

💔 All backports failed

Status	Branch	Result
❌	8.6	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 143604

Questions ?

Please refer to the Backport tool documentation

partial progress

93f99cf

mattkime commented Oct 18, 2022

View reviewed changes

src/plugins/data_view_editor/public/components/form_fields/title_field.tsx Outdated Show resolved Hide resolved

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

009ca50

…-fix'

sebelga reviewed Oct 19, 2022

View reviewed changes

src/plugins/data_view_editor/public/components/data_view_editor_flyout_content.tsx Outdated Show resolved Hide resolved

mattkime added 2 commits October 20, 2022 01:11

fix title field validation, work in progress

9ff8b24

Merge branch 'reduce_react_state_data_view_create' of github.com:matt…

0f8884f

…kime/kibana into reduce_react_state_data_view_create

sebelga reviewed Oct 20, 2022

View reviewed changes

src/plugins/data_view_editor/public/components/form_fields/title_field.tsx Outdated Show resolved Hide resolved

mattkime added 8 commits October 26, 2022 15:36

Merge branch 'main' into reduce_react_state_data_view_create

0723097

remove debounce, partial progress

de0361c

lots of refactoring, most of the way there, still needs a bit more cl…

32b9a2c

…eanup

add update method and make most things private

2c1a7f1

Merge branch 'main' into reduce_react_state_data_view_create

19ce8d5

partial progress, lots of cleanup, still some todos

a0e4fde

Merge branch 'main' into reduce_react_state_data_view_create

d8ba032

remove a few comments

e5b93ca

mattkime commented Nov 1, 2022

View reviewed changes

mattkime added 3 commits November 1, 2022 23:08

export observables rather than subjects

c2b2a15

remove service context

27c1fb4

Merge branch 'main' into reduce_react_state_data_view_create

798edb6

mattkime commented Nov 2, 2022

View reviewed changes

display errors on submission of form

535a5ab

mattkime changed the title ~~partial progress~~ [data view editor] Form state refactor - move all state logic into service and out of react state management Nov 3, 2022

final cleanup and test fix, all working

4f8524d

mattkime added 3 commits November 3, 2022 11:28

add destroy method to service

1b9ac2f

Merge branch 'reduce_react_state_data_view_create' of github.com:matt…

b0626f4

…kime/kibana into reduce_react_state_data_view_create

useState to store service reference instead of useRef

3f4bc34

Dosant reviewed Nov 4, 2022

View reviewed changes

fix case where index pattern wasn't changed

33f252b

Dosant approved these changes Nov 7, 2022

View reviewed changes

mattkime added 5 commits November 15, 2022 19:49

Merge branch 'main' into reduce_react_state_data_view_create

0299122

Merge branch 'main' into reduce_react_state_data_view_create

fbd0594

Merge branch 'main' into reduce_react_state_data_view_create

428a0ad

Merge branch 'reduce_react_state_data_view_create' of github.com:matt…

5fb1dde

…kime/kibana into reduce_react_state_data_view_create

Merge branch 'main' into reduce_react_state_data_view_create

054c1e6

mattkime requested a review from a team as a code owner November 25, 2022 06:28

mattkime added 3 commits November 25, 2022 22:43

Merge branch 'main' into reduce_react_state_data_view_create

2f4e0fd

add small delay to functional test

b597595

Merge branch 'main' into reduce_react_state_data_view_create

3f0e539

This was referenced Nov 27, 2022

[DataViews] Multiple fast clicking to create new data view cause multiple data view save #146125

Closed

[DataViews] Create data view flyout duplicate name validation fails for auto-generated names #146338

Closed

jughosta approved these changes Nov 28, 2022

View reviewed changes

mattkime added 3 commits November 28, 2022 15:55

Merge branch 'main' into reduce_react_state_data_view_create

c790676

remove error flash on successful submit

5b6bb93

Merge branch 'reduce_react_state_data_view_create' of github.com:matt…

659dcd3

…kime/kibana into reduce_react_state_data_view_create

mattkime enabled auto-merge (squash) November 28, 2022 23:47

mattkime merged commit 9e0da98 into elastic:main Nov 29, 2022

kibanamachine added the v8.7.0 label Nov 29, 2022

mattkime removed the v8.6.0 label Nov 29, 2022

kibanamachine added the backport:skip This PR does not require backporting label Nov 29, 2022

mattkime mentioned this pull request Dec 18, 2022

Data views: index pattern error when editing existing data view #140679

Closed

Conversation

mattkime commented Oct 18, 2022 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Nov 5, 2022

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

jughosta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Nov 29, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled in files

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

Uh oh!

kibanamachine commented Nov 29, 2022

💔 All backports failed

Manual backport

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattkime commented Oct 18, 2022 •

edited by kibanamachine

Loading

Dosant left a comment •

edited

Loading