[Data Explorer] State management + enhancements #4580

ashwin-pc · 2023-07-17T08:19:23Z

Description

This PR is mostly the introduction of state management for Data Explorer. It also additionally does the following:

Adds a button to the nav bar that allows for easily toggling between the legacy and new discover app's
rewrites the view service logic for how to add new views to the app. The new way uses portals and react lazy to achieve the same result but keeps the react tree intact allowing for state and other information in the react tree to pass smoothly.
Fixes the canvas style for the expanded header.

Note: This is built on top of #4563, if this is merged before that, #4563 can be closed.

Issues Resolved

closes [Data Explorer] Design state management & implement it #4225

Screenshot

Screen.Recording.2023-07-17.at.2.17.00.AM.mov

Testing the changes

Testing the toggle:

Clicking between the "Legacy Discover" and "New Discover" apps should allow you to toggle between the two apps easily.

Testing the state management:

Ive added a input in the canvas and show its value in the panel. Changing the value int he input should change the value in the panel
For the global state, changing the index pattern in the side panel drop down should work.

Check List

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

codecov · 2023-07-17T10:31:19Z

Codecov Report

Merging #4580 (031fc07) into feature/data-explorer (146cc36) will decrease coverage by 0.17%.
The diff coverage is 36.42%.

❗ Current head 031fc07 differs from pull request most recent head c568126. Consider uploading reports for the commit c568126 to get more accurate results

@@                    Coverage Diff                    @@
##           feature/data-explorer    #4580      +/-   ##
=========================================================
- Coverage                  66.60%   66.43%   -0.17%     
=========================================================
  Files                       3282     3287       +5     
  Lines                      62732    62789      +57     
  Branches                    9780     9774       -6     
=========================================================
- Hits                       41780    41715      -65     
- Misses                     18575    18702     +127     
+ Partials                    2377     2372       -5

Flag	Coverage Δ
Linux	`?`
Windows	`66.43% <36.42%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...plugins/data_explorer/public/utils/use/use_view.ts	`7.69% <0.00%> (-4.81%)`	⬇️
src/plugins/discover/public/build_services.ts	`0.00% <ø> (ø)`
src/plugins/discover/public/index.ts	`0.00% <ø> (ø)`
src/plugins/discover/public/plugin.ts	`1.33% <0.00%> (+1.33%)`	⬆️
.../data_explorer/public/components/sidebar/index.tsx	`3.44% <3.44%> (ø)`
...ta_explorer/public/utils/state_management/store.ts	`20.00% <20.00%> (ø)`
..._explorer/public/utils/state_management/preload.ts	`33.33% <33.33%> (ø)`
.../data_explorer/public/components/app_container.tsx	`40.00% <50.00%> (+6.66%)`	⬆️
...lication/utils/state_management/discover_slice.tsx	`60.00% <60.00%> (ø)`
...er/public/utils/state_management/metadata_slice.ts	`69.23% <69.23%> (ø)`
... and 6 more

... and 22 files with indirect coverage changes

kavilla · 2023-07-17T22:39:55Z

src/plugins/discover_legacy/public/plugin.ts

@@ -318,6 +320,25 @@ export class DiscoverPlugin
        if (!this.initializeInnerAngular) {


Are we in a state to remove this or is the legacy still using Angular?

The legacy still uses angular

joshuarrrr

mostly minor questions and comments

joshuarrrr · 2023-07-18T00:32:21Z

src/plugins/discover_legacy/opensearch_dashboards.json

+    "opensearchDashboardsUtils",
+    "savedObjects",
+    "opensearchDashboardsReact",
+    "discover"


Is there any risk of having legacy discover depend on the new discover plugin? I suppose it means that if the new discover plugin can't start up, neither can the legacy one.

Not really. the legacy discover here only depends on the discover plugin bundles. So its safe to assume that that will not be the case. I also dont think that such a scenario is honestly possible

joshuarrrr · 2023-07-18T00:33:19Z

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

+      id: 'discover-new',
+      label: i18n.translate('discover.localMenu.newDiscoverTitle', {
+        defaultMessage: 'New Discover',


I know this is a feature branch, but you probably want a TODO here if this isn't finalized text (which I'm assuming it's not)

The issue for this change is still unresolved. This will most likely become a toggle. I added this for now simply to make development easier to validate changes between the two discovers.

joshuarrrr · 2023-07-18T00:34:51Z

src/plugins/discover_legacy/public/plugin.ts

+        const v2Enabled = core.uiSettings.get<boolean>(NEW_DISCOVER_APP);
+        if (v2Enabled) {
+          navigateToApp('data-explorer', {
+            replace: true,
+            path: `/discover`,
+            state: {
+              path,
+            } as ViewRedirectParams,
+          });
+        }


So this redirection will happen in discover once we remove this plugin, right?

This redirection already occurs in discover today. the change once we remove the legacy app is that the redirect will always happen there instead of sometimes.

joshuarrrr · 2023-07-18T19:06:32Z

src/plugins/discover/public/plugin.ts

@@ -1,31 +1,6 @@
 /*
+ * Copyright OpenSearch Contributors


seems like keeping the old license makes sense here.

I modified it since this file will change significantly by the end of the project.

joshuarrrr · 2023-07-18T19:06:39Z

src/plugins/discover/public/plugin.ts

-        defaults: {},
-        reducer: () => ({}),
+        defaults: async () => {
+          this.initializeServices?.();


why do we want/need the nullish coalescing here?

This is undefined in the plugin class but defined in the plugin start function. This should in theory never be null since it will always run after the start method, but ive added this to avoid typescript complaining, and i also did not want to mark it as non null.

joshuarrrr · 2023-07-18T19:07:31Z

src/plugins/discover/public/application/view_components/canvas/canvas.tsx

+  const dispatch = useDispatch();
+
+  return (
+    <div>


nit - should be just <> or else an OUI component - but is this just placeholder demo content?

Yes this is placeholder content for now. Anan is adding the actual content inhere soon

joshuarrrr · 2023-07-18T19:07:50Z

src/plugins/discover/public/application/utils/state_management/discover_slice.tsx

+  /**
+   * Array of applied filters
+   */
+  filters?: Filter[];
+  /**
+   * Used interval of the histogram
+   */
+  interval?: string;


Can't these just be retrieved from the global state, because they're universal and not specific to discover?

I copied this over form the legacy discover state. When Anan adds the doc table, she will update this to be a more accurate representation of the discover state.

joshuarrrr · 2023-07-18T19:07:54Z

src/plugins/discover/public/application/utils/state_management/discover_slice.tsx

+export const getPreloadedState = async ({ data }: DiscoverServices): Promise<DiscoverState> => {
+  return {
+    ...initialState,
+    interval: data.query.timefilter.timefilter.getRefreshInterval().value.toString(),


Why is the interval the only part retrieved from the data plugin?

This is just to validate that the state management is working as expected across views. Its placeholder content.

joshuarrrr · 2023-07-18T19:12:38Z

src/plugins/discover/public/application/view_components/panel/index.tsx

+// eslint-disable-next-line import/no-default-export
+export default function PanelApp(props: ViewProps) {


This may be temp - otherwise should include an explanation of why it's necessary.

I will add this in a followup, but the reason is that to lazy load components, the syntax requires default components to be exported.

joshuarrrr · 2023-07-18T19:19:05Z

src/plugins/data_explorer/public/application.tsx


 export const renderApp = (
-  { notifications, http }: CoreStart,
+  core: CoreStart,


core and params appear unused?

params is passed on to the views since they need it to render the top nav menu items correctly. core is not used right now but will be needed later on.

abbyhu2000 · 2023-07-19T18:31:23Z

src/plugins/data_explorer/public/components/app_container.tsx

@@ -76,8 +36,9 @@ export const AppContainer = ({ view }: { view?: View }) => {
      paddingSize="none"
    >
      {/* TODO: improve loading state */}


is this TODO here still needed? do we plan to improve the loading state?

Yeah, this will be improved before merge to main. Its currently not that good.

abbyhu2000 · 2023-07-19T18:42:49Z

src/plugins/data_explorer/public/utils/state_management/store.ts

So the app state persistence for discover is changed to using redux store like vis builder. The global persistence is still using state container and osd url state storage right?

yes thats going to come soon :)

abbyhu2000 · 2023-07-19T18:46:44Z

The state management part LGTM. But could you please remind me if u mentioned it before, what are the relationships among discover, discover_legacy, and data_explorer right now?

ananzh · 2023-07-27T16:11:07Z

The state management part LGTM. But could you please remind me if u mentioned it before, what are the relationships among discover, discover_legacy, and data_explorer right now?

data_explorer is like a view manager which controls all the states and routing logic. discover is a view and needs to be registered to use state management from data_explorer. discover is purely react. discover_legacy currently is still an angular plugin. discover page has a legacy toggle to switch to discover_legacy. data_explorer handles the routing. discover_legacy will be removed eventually.

…ate-management Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc added 5 commits July 13, 2023 06:52

Adds toggle between legacy and new discover

d18c5a4

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Fixes header offset

a1d1270

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

adds basic state management

4a72f86

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

attempt 1 at dynamic state management

32a1299

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Working multi view state management

5981002

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc added discover for discover reinvent de-angular de-angularize work data explorer Issues related to the Data Explorer project labels Jul 17, 2023

ashwin-pc requested review from ananzh, kavilla, seanneumann, AMoo-Miki, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX and Flyingliuhub as code owners July 17, 2023 08:19

github-actions bot added the distinguished-contributor label Jul 17, 2023

Adds global state persistence to data explorer

031fc07

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

kavilla reviewed Jul 17, 2023

View reviewed changes

joshuarrrr reviewed Jul 18, 2023

View reviewed changes

abbyhu2000 reviewed Jul 19, 2023

View reviewed changes

ananzh approved these changes Jul 27, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/feature/data-explorer' into st…

c568126

…ate-management Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc merged commit 521f306 into opensearch-project:feature/data-explorer Jul 27, 2023
16 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data Explorer] State management + enhancements #4580

[Data Explorer] State management + enhancements #4580

ashwin-pc commented Jul 17, 2023 •

edited

Loading

codecov bot commented Jul 17, 2023 •

edited

Loading

kavilla Jul 17, 2023

ashwin-pc Jul 19, 2023

joshuarrrr left a comment

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

joshuarrrr Jul 18, 2023

ashwin-pc Jul 19, 2023

abbyhu2000 Jul 19, 2023

ashwin-pc Jul 27, 2023

abbyhu2000 Jul 19, 2023

ashwin-pc Jul 27, 2023

abbyhu2000 commented Jul 19, 2023

ananzh commented Jul 27, 2023

		@@ -318,6 +320,25 @@ export class DiscoverPlugin
		if (!this.initializeInnerAngular) {

		// eslint-disable-next-line import/no-default-export
		export default function PanelApp(props: ViewProps) {

[Data Explorer] State management + enhancements #4580

[Data Explorer] State management + enhancements #4580

Conversation

ashwin-pc commented Jul 17, 2023 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Jul 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarrrr left a 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

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

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

abbyhu2000 commented Jul 19, 2023

ananzh commented Jul 27, 2023

ashwin-pc commented Jul 17, 2023 •

edited

Loading

codecov bot commented Jul 17, 2023 •

edited

Loading