[Discover] A bunch of navigation fixes (opensearch-project#5168)

* Discover: Fixes state persistence after nav * Fixed breadcrumbs and navigation * fixes mobile view --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Signed-off-by: Willie Hung <willie880201044@gmail.com>
willie-hung · Oct 5, 2023 · 0f5357b · 0f5357b
1 parent fb17b80
commit 0f5357b
Show file tree

Hide file tree

Showing 16 changed files with 206 additions and 175 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ### 🛡 Security
 
+- [CVE-2022-25869] Remove AngularJS 1.8 ([#5086](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5086))
 - [CVE-2022-37599] Bump loader-utils from `2.0.3` to `2.0.4` ([#3031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3031)). Backwards-compatible fixes included in v2.6.0 and v1.3.7 releases.
 - [CVE-2022-37603] Bump loader-utils from `2.0.3` to `2.0.4` ([#3031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3031)). Backwards-compatible fixes included in v2.6.0 and v1.3.7 releases.
 - [WS-2021-0638] Bump mocha from `7.2.0` to `10.1.0` ([#2711](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2711))
@@ -36,8 +37,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Decouple] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster([#4612](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4612))
 - [Advanced Settings] Consolidate settings into new "Appearance" category and add category IDs ([#4845](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4845))
 - Adds Data explorer framework and implements Discover using it ([#4806](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4806))
-- [Theme] Use themes' definitions to render the initial view ([#4936](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4936/))
-- [Theme] Make `next` theme the default ([#4854](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4854/))
+- [Theme] Use themes' definitions to render the initial view ([#4936](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4936))
+- [Theme] Make `next` theme the default ([#4854](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4854))
+- [Discover] Update embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081))
 
 ### 🐛 Bug Fixes
 
@@ -55,6 +57,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [BUG] Fix buildPointSeriesData unit test fails due to local timezone ([#4992](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4992))
 - [BUG][Data Explorer][Discover] Fix total hits issue for no time based data ([#5087](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5087))
 - [BUG][Data Explorer][Discover] Add onQuerySubmit to top nav and allow force update to embeddable ([#5160](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5160))
+- [BUG][Discover] Fix misc navigation issues ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))
+- [BUG][Discover] Fix mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))
 
 ### 🚞 Infrastructure
 

diff --git a/src/plugins/data_explorer/public/utils/state_management/store.ts b/src/plugins/data_explorer/public/utils/state_management/store.ts
@@ -9,6 +9,13 @@ import { reducer as metadataReducer } from './metadata_slice';
 import { loadReduxState, persistReduxState } from './redux_persistence';
 import { DataExplorerServices } from '../../types';
 
+const HYDRATE = 'HYDRATE';
+
+export const hydrate = (newState: RootState) => ({
+  type: HYDRATE,
+  payload: newState,
+});
+
 const commonReducers = {
   metadata: metadataReducer,
 };
@@ -22,9 +29,20 @@ let dynamicReducers: {
 
 const rootReducer = combineReducers(dynamicReducers);
 
+const createRootReducer = (): Reducer<RootState> => {
+  const combinedReducer = combineReducers(dynamicReducers);
+
+  return (state: RootState | undefined, action: any): RootState => {
+    if (action.type === HYDRATE) {
+      return action.payload;
+    }
+    return combinedReducer(state, action);
+  };
+};
+
 export const configurePreloadedStore = (preloadedState: PreloadedState<RootState>) => {
   // After registering the slices the root reducer needs to be updated
-  const updatedRootReducer = combineReducers(dynamicReducers);
+  const updatedRootReducer = createRootReducer();
 
   return configureStore({
     reducer: updatedRootReducer,
@@ -62,6 +80,18 @@ export const getPreloadedStore = async (services: DataExplorerServices) => {
   // the store subscriber will automatically detect changes and call handleChange function
   const unsubscribe = store.subscribe(handleChange);
 
+  // This is necessary because browser navigation updates URL state that isnt reflected in the redux state
+  services.scopedHistory.listen(async (location, action) => {
+    const urlState = await loadReduxState(services);
+    const currentState = store.getState();
+
+    // If the url state is different from the current state, then we need to update the store
+    // the state should have a view property if it was loaded from the url
+    if (action === 'POP' && urlState.metadata?.view && !isEqual(urlState, currentState)) {
+      store.dispatch(hydrate(urlState as RootState));
+    }
+  });
+
   const onUnsubscribe = () => {
     dynamicReducers = {
       ...commonReducers,

diff --git a/src/plugins/discover/public/application/components/doc_views/surrounding_docs_app.tsx b/src/plugins/discover/public/application/components/doc_views/surrounding_docs_app.tsx
@@ -47,7 +47,7 @@ export function SurroundingDocsApp() {
 
   useEffect(() => {
     chrome.setBreadcrumbs([
-      ...getRootBreadcrumbs(baseUrl),
+      ...getRootBreadcrumbs(),
       {
         text: i18n.translate('discover.context.breadcrumb', {
           defaultMessage: `Context of #{docId}`,

diff --git a/src/plugins/discover/public/application/components/doc_views/surrounding_docs_view.tsx b/src/plugins/discover/public/application/components/doc_views/surrounding_docs_view.tsx
@@ -93,7 +93,7 @@ export const SurroundingDocsView = ({ id, indexPattern }: SurroundingDocsViewPar
         field,
         values,
         operation,
-        indexPattern.id
+        indexPattern.id || ''
       );
       return filterManager.addFilters(newFilters);
     },
@@ -115,23 +115,25 @@ export const SurroundingDocsView = ({ id, indexPattern }: SurroundingDocsViewPar
     [onAddFilter, rows, indexPattern, setContextAppState, contextQueryState, contextAppState]
   );
 
+  if (isLoading) {
+    return null;
+  }
+
   return (
-    !isLoading && (
-      <Fragment>
-        <TopNavMenu
-          appName={'discover.context.surroundingDocs.topNavMenu'}
-          showSearchBar={true}
-          showQueryBar={false}
-          showDatePicker={false}
-          indexPatterns={[indexPattern]}
-          useDefaultBehaviors={true}
-        />
-        <EuiPage className="discover.context.appPage">
-          <EuiPageContent paddingSize="s" className="dscDocsContent">
-            {contextAppMemoized}
-          </EuiPageContent>
-        </EuiPage>
-      </Fragment>
-    )
+    <Fragment>
+      <TopNavMenu
+        appName={'discover.context.surroundingDocs.topNavMenu'}
+        showSearchBar={true}
+        showQueryBar={false}
+        showDatePicker={false}
+        indexPatterns={[indexPattern]}
+        useDefaultBehaviors={true}
+      />
+      <EuiPage className="discover.context.appPage">
+        <EuiPageContent paddingSize="s" className="dscDocsContent">
+          {contextAppMemoized}
+        </EuiPageContent>
+      </EuiPage>
+    </Fragment>
   );
 };
diff --git a/src/plugins/discover/public/application/components/sidebar/discover_field.scss b/src/plugins/discover/public/application/components/sidebar/discover_field.scss
@@ -11,4 +11,8 @@
   &:focus {
     opacity: 1;
   }
+
+  @include ouiBreakpoint("xs", "s", "m") {
+    opacity: 1;
+  }
 }
diff --git a/src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx b/src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx
@@ -19,6 +19,7 @@ import {
 import { DiscoverState, setSavedSearchId } from '../../utils/state_management';
 import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../../../common';
 import { getSortForSearchSource } from '../../view_components/utils/get_sort_for_search_source';
+import { getRootBreadcrumbs } from '../../helpers/breadcrumbs';
 
 export const getTopNavLinks = (
   services: DiscoverViewServices,
@@ -45,11 +46,9 @@ export const getTopNavLinks = (
       defaultMessage: 'New Search',
     }),
     run() {
-      setTimeout(() => {
-        history().push('/');
-        // TODO: figure out why a history push doesn't update the app state. The page reload is a hack around it
-        window.location.reload();
-      }, 0);
+      core.application.navigateToApp('discover', {
+        path: '#/',
+      });
     },
     testId: 'discoverNewButton',
   };
@@ -103,15 +102,7 @@ export const getTopNavLinks = (
               history().push(`/view/${encodeURIComponent(id)}`);
             } else {
               chrome.docTitle.change(savedSearch.lastSavedTitle);
-              chrome.setBreadcrumbs([
-                {
-                  text: i18n.translate('discover.discoverBreadcrumbTitle', {
-                    defaultMessage: 'Discover',
-                  }),
-                  href: '#/',
-                },
-                { text: savedSearch.title },
-              ]);
+              chrome.setBreadcrumbs([...getRootBreadcrumbs(), { text: savedSearch.title }]);
             }
 
             // set App state to clean

diff --git a/src/plugins/discover/public/application/components/top_nav/open_search_panel.tsx b/src/plugins/discover/public/application/components/top_nav/open_search_panel.tsx
@@ -55,7 +55,7 @@ interface Props {
 export function OpenSearchPanel({ onClose, makeUrl }: Props) {
   const {
     services: {
-      core: { uiSettings, savedObjects },
+      core: { uiSettings, savedObjects, application },
       addBasePath,
     },
   } = useOpenSearchDashboards<DiscoverViewServices>();
@@ -90,12 +90,8 @@ export function OpenSearchPanel({ onClose, makeUrl }: Props) {
             },
           ]}
           onChoose={(id) => {
-            setTimeout(() => {
-              window.location.assign(makeUrl(id));
-              // TODO: figure out why a history push doesn't update the app state. The page reload is a hack around it
-              window.location.reload();
-              onClose();
-            }, 0);
+            application.navigateToApp('discover', { path: `#/view/${id}` });
+            onClose();
           }}
           uiSettings={uiSettings}
           savedObjects={savedObjects}

diff --git a/src/plugins/discover/public/application/helpers/breadcrumbs.ts b/src/plugins/discover/public/application/helpers/breadcrumbs.ts
@@ -29,14 +29,17 @@
  */
 
 import { i18n } from '@osd/i18n';
+import { EuiBreadcrumb } from '@elastic/eui';
+import { getServices } from '../../opensearch_dashboards_services';
 
-export function getRootBreadcrumbs(baseUrl: string) {
+export function getRootBreadcrumbs(): EuiBreadcrumb[] {
+  const { core } = getServices();
   return [
     {
       text: i18n.translate('discover.rootBreadcrumb', {
         defaultMessage: 'Discover',
       }),
-      href: baseUrl,
+      onClick: () => core.application.navigateToApp('discover'),
     },
   ];
 }
diff --git a/src/plugins/discover/public/application/utils/state_management/discover_slice.tsx b/src/plugins/discover/public/application/utils/state_management/discover_slice.tsx
@@ -11,6 +11,7 @@ import { RootState, DefaultViewState } from '../../../../../data_explorer/public
 import { buildColumns } from '../columns';
 import * as utils from './common';
 import { SortOrder } from '../../../saved_searches/types';
+import { PLUGIN_ID } from '../../../../common';
 
 export interface DiscoverState {
   /**
@@ -79,6 +80,7 @@ export const getPreloadedState = async ({
       preloadedState.root = {
         metadata: {
           indexPattern: indexPatternId,
+          view: PLUGIN_ID,
         },
       };
 

diff --git a/src/plugins/discover/public/application/view_components/canvas/discover_chart_container.tsx b/src/plugins/discover/public/application/view_components/canvas/discover_chart_container.tsx
@@ -13,7 +13,7 @@ import { DiscoverChart } from '../../components/chart/chart';
 
 export const DiscoverChartContainer = ({ hits, bucketInterval, chartData }: SearchData) => {
   const { services } = useOpenSearchDashboards<DiscoverViewServices>();
-  const { uiSettings, data } = services;
+  const { uiSettings, data, core } = services;
   const { indexPattern, savedSearch } = useDiscoverContext();
 
   const isTimeBased = useMemo(() => (indexPattern ? indexPattern.isTimeBased() : false), [
@@ -30,8 +30,7 @@ export const DiscoverChartContainer = ({ hits, bucketInterval, chartData }: Sear
       data={data}
       hits={hits}
       resetQuery={() => {
-        window.location.href = `#/view/${savedSearch?.id}`;
-        window.location.reload();
+        core.application.navigateToApp('discover', { path: `#/view/${savedSearch?.id}` });
       }}
       services={services}
       showResetButton={!!savedSearch && !!savedSearch.id}

diff --git a/src/plugins/discover/public/application/view_components/canvas/index.tsx b/src/plugins/discover/public/application/view_components/canvas/index.tsx
@@ -4,7 +4,7 @@
  */
 
 import React, { useEffect, useState, useRef, useCallback } from 'react';
-import { EuiFlexGroup, EuiFlexItem, EuiPanel } from '@elastic/eui';
+import { EuiPanel } from '@elastic/eui';
 import { TopNav } from './top_nav';
 import { ViewProps } from '../../../../../data_explorer/public';
 import { DiscoverTable } from './discover_table';
@@ -20,6 +20,7 @@ import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_re
 import { filterColumns } from '../utils/filter_columns';
 import { DEFAULT_COLUMNS_SETTING } from '../../../../common';
 import './discover_canvas.scss';
+
 // eslint-disable-next-line import/no-default-export
 export default function DiscoverCanvas({ setHeaderActionMenu, history }: ViewProps) {
   const { data$, refetch$, indexPattern } = useDiscoverContext();
@@ -77,38 +78,36 @@ export default function DiscoverCanvas({ setHeaderActionMenu, history }: ViewPro
   const timeField = indexPattern?.timeFieldName ? indexPattern.timeFieldName : undefined;
 
   return (
-    <EuiFlexGroup direction="column" gutterSize="none" className="dscCanvas">
-      <EuiFlexItem grow={false}>
-        <TopNav
-          opts={{
-            setHeaderActionMenu,
-            onQuerySubmit,
-          }}
-        />
-      </EuiFlexItem>
+    <EuiPanel
+      hasBorder={false}
+      hasShadow={false}
+      color="transparent"
+      paddingSize="none"
+      className="dscCanvas"
+    >
+      <TopNav
+        opts={{
+          setHeaderActionMenu,
+          onQuerySubmit,
+        }}
+      />
       {status === ResultStatus.NO_RESULTS && (
-        <EuiFlexItem>
-          <DiscoverNoResults timeFieldName={timeField} queryLanguage={''} />
-        </EuiFlexItem>
+        <DiscoverNoResults timeFieldName={timeField} queryLanguage={''} />
       )}
       {status === ResultStatus.UNINITIALIZED && (
         <DiscoverUninitialized onRefresh={() => refetch$.next()} />
       )}
       {status === ResultStatus.LOADING && <LoadingSpinner />}
       {status === ResultStatus.READY && (
         <>
-          <EuiFlexItem grow={false}>
-            <EuiPanel hasBorder={false} hasShadow={false} color="transparent" paddingSize="s">
-              <EuiPanel>
-                <DiscoverChartContainer {...fetchState} />
-              </EuiPanel>
+          <EuiPanel hasBorder={false} hasShadow={false} color="transparent" paddingSize="s">
+            <EuiPanel>
+              <DiscoverChartContainer {...fetchState} />
             </EuiPanel>
-          </EuiFlexItem>
-          <EuiFlexItem>
-            <DiscoverTable history={history} />
-          </EuiFlexItem>
+          </EuiPanel>
+          <DiscoverTable history={history} />
         </>
       )}
-    </EuiFlexGroup>
+    </EuiPanel>
   );
 }
diff --git a/src/plugins/discover/public/application/view_components/canvas/top_nav.tsx b/src/plugins/discover/public/application/view_components/canvas/top_nav.tsx
@@ -62,12 +62,9 @@ export const TopNav = ({ opts }: TopNavProps) => {
     chrome.docTitle.change(`Discover${pageTitleSuffix}`);
 
     if (savedSearch?.id) {
-      chrome.setBreadcrumbs([
-        ...getRootBreadcrumbs(getUrlForApp(PLUGIN_ID)),
-        { text: savedSearch.title },
-      ]);
+      chrome.setBreadcrumbs([...getRootBreadcrumbs(), { text: savedSearch.title }]);
     } else {
-      chrome.setBreadcrumbs([...getRootBreadcrumbs(getUrlForApp(PLUGIN_ID))]);
+      chrome.setBreadcrumbs([...getRootBreadcrumbs()]);
     }
   }, [chrome, getUrlForApp, savedSearch?.id, savedSearch?.title]);
 

diff --git a/src/plugins/discover/public/application/view_components/context/index.tsx b/src/plugins/discover/public/application/view_components/context/index.tsx
@@ -17,12 +17,14 @@ const SearchContext = React.createContext<SearchContextValue>({} as SearchContex
 
 // eslint-disable-next-line import/no-default-export
 export default function DiscoverContext({ children }: React.PropsWithChildren<ViewProps>) {
+  const { services: deServices } = useOpenSearchDashboards<DataExplorerServices>();
   const services = getServices();
-  const searchParams = useSearch(services);
+  const searchParams = useSearch({
+    ...deServices,
+    ...services,
+  });
 
-  const {
-    services: { osdUrlStateStorage },
-  } = useOpenSearchDashboards<DataExplorerServices>();
+  const { osdUrlStateStorage } = deServices;
 
   // Connect the query service to the url state
   useEffect(() => {