Add internalState loading function

elastic · Nov 8, 2024 · 0cb4cf1 · 0cb4cf1
2 parents 1ac22dc + 4cfc4f3
commit 0cb4cf1
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 52 deletions.
diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts
@@ -42,7 +42,7 @@ import type { DiscoverStateContainer } from '../../state_management/discover_sta
 import { addLog } from '../../../../utils/add_log';
 import { useInternalStateSelector } from '../../state_management/discover_internal_state_container';
 import type { DiscoverAppState } from '../../state_management/discover_app_state_container';
-import { DataDocumentsMsg } from '../../state_management/discover_data_state_container';
+import type { DataDocumentsMsg } from '../../state_management/discover_data_state_container';
 import { useSavedSearch } from '../../state_management/discover_state_provider';
 import { useIsEsqlMode } from '../../hooks/use_is_esql_mode';
 
@@ -177,7 +177,7 @@ export const useDiscoverHistogram = ({
           totalHitsResult &&
           typeof result !== 'number'
         ) {
-          // ignore the histogram initial loading state if discover state already has a total hits value
+          addLog(`[UnifiedHistogram] skip ${status}: total hits > 0 in Discover`, result);
           return;
         }
 
@@ -192,6 +192,10 @@ export const useDiscoverHistogram = ({
         }
 
         if (status !== UnifiedHistogramFetchStatus.complete || typeof result !== 'number') {
+          addLog(
+            '[UnifiedHistogram] ignore the histogram complete/partial state if discover state already has a total hits value',
+            { status, result }
+          );
           return;
         }
 

diff --git a/src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx b/src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx
@@ -8,7 +8,7 @@
  */
 
 import React, { useCallback, useEffect, useMemo, useRef } from 'react';
-import { type DataView, DataViewType } from '@kbn/data-views-plugin/public';
+import { DataViewType } from '@kbn/data-views-plugin/public';
 import { DataViewPickerProps } from '@kbn/unified-search-plugin/public';
 import { ENABLE_ESQL } from '@kbn/esql-utils';
 import { TextBasedLanguages } from '@kbn/esql-utils';
@@ -20,7 +20,6 @@ import { useDiscoverServices } from '../../../../hooks/use_discover_services';
 import type { DiscoverStateContainer } from '../../state_management/discover_state';
 import { onSaveSearch } from './on_save_search';
 import { useDiscoverCustomization } from '../../../../customizations';
-import { addLog } from '../../../../utils/add_log';
 import { useAppStateSelector } from '../../state_management/discover_app_state_container';
 import { useDiscoverTopNav } from './use_discover_topnav';
 import { useIsEsqlMode } from '../../hooks/use_is_esql_mode';
@@ -47,15 +46,8 @@ export const DiscoverTopNav = ({
   onCancelClick,
 }: DiscoverTopNavProps) => {
   const services = useDiscoverServices();
-  const {
-    dataViewEditor,
-    navigation,
-    dataViewFieldEditor,
-    data,
-    uiSettings,
-    dataViews,
-    setHeaderActionMenu,
-  } = services;
+  const { dataViewEditor, navigation, dataViewFieldEditor, data, uiSettings, setHeaderActionMenu } =
+    services;
   const query = useAppStateSelector((state) => state.query);
   const adHocDataViews = useInternalStateSelector((state) => state.adHocDataViews);
   const dataView = useInternalStateSelector((state) => state.dataView!);
@@ -123,23 +115,6 @@ export const DiscoverTopNav = ({
     });
   }, [dataViewEditor, stateContainer]);
 
-  const onEditDataView = useCallback(
-    async (editedDataView: DataView) => {
-      if (editedDataView.isPersisted()) {
-        // Clear the current data view from the cache and create a new instance
-        // of it, ensuring we have a new object reference to trigger a re-render
-        dataViews.clearInstanceCache(editedDataView.id);
-        stateContainer.actions.setDataView(await dataViews.create(editedDataView.toSpec(), true));
-      } else {
-        await stateContainer.actions.updateAdHocDataViewId();
-      }
-      stateContainer.actions.loadDataViewList();
-      addLog('[DiscoverTopNav] onEditDataView triggers data fetching');
-      stateContainer.dataState.fetch();
-    },
-    [dataViews, stateContainer.actions, stateContainer.dataState]
-  );
-
   const updateSavedQueryId = (newSavedQueryId: string | undefined) => {
     const { appState } = stateContainer;
     if (newSavedQueryId) {
@@ -223,14 +198,13 @@ export const DiscoverTopNav = ({
       textBasedLanguages: supportedTextBasedLanguages,
       adHocDataViews,
       savedDataViews,
-      onEditDataView,
+      onEditDataView: stateContainer.actions.onDataViewEdited,
     };
   }, [
     adHocDataViews,
     addField,
     createNewDataView,
     dataView,
-    onEditDataView,
     savedDataViews,
     stateContainer,
     uiSettings,

diff --git a/src/plugins/discover/public/application/main/data_fetching/fetch_all.test.ts b/src/plugins/discover/public/application/main/data_fetching/fetch_all.test.ts
@@ -69,6 +69,7 @@ describe('test fetchAll', () => {
       getAppState: () => ({}),
       getInternalState: () => ({
         dataView: undefined,
+        isLoading: false,
         isDataViewLoading: false,
         savedDataViews: [],
         adHocDataViews: [],
@@ -262,6 +263,7 @@ describe('test fetchAll', () => {
       getInternalState: () => ({
         dataView: undefined,
         isDataViewLoading: false,
+        isLoading: false,
         savedDataViews: [],
         adHocDataViews: [],
         expandedDoc: undefined,
@@ -384,6 +386,7 @@ describe('test fetchAll', () => {
         getInternalState: () => ({
           dataView: undefined,
           isDataViewLoading: false,
+          isLoading: false,
           savedDataViews: [],
           adHocDataViews: [],
           expandedDoc: undefined,

diff --git a/src/plugins/discover/public/application/main/discover_main_route.tsx b/src/plugins/discover/public/application/main/discover_main_route.tsx
@@ -22,6 +22,7 @@ import { reportPerformanceMetricEvent } from '@kbn/ebt-tools';
 import { withSuspense } from '@kbn/shared-ux-utility';
 import { getInitialESQLQuery } from '@kbn/esql-utils';
 import { ESQL_TYPE } from '@kbn/data-view-utils';
+import { useInternalStateSelector } from './state_management/discover_internal_state_container';
 import { useUrl } from './hooks/use_url';
 import { useDiscoverStateContainer } from './hooks/use_discover_state_container';
 import { MainHistoryLocationState } from '../../../common';
@@ -85,7 +86,6 @@ export function DiscoverMainRoute({
       stateContainer,
     });
   const [error, setError] = useState<Error>();
-  const [loading, setLoading] = useState(true);
   const [noDataState, setNoDataState] = useState({
     hasESData: false,
     hasUserDataView: false,
@@ -157,10 +157,10 @@ export function DiscoverMainRoute({
       initialAppState,
     }: { nextDataView?: DataView; initialAppState?: LoadParams['initialAppState'] } = {}) => {
       const loadSavedSearchStartTime = window.performance.now();
-      setLoading(true);
+      stateContainer.actions.setIsLoading(true);
       const skipNoData = await skipNoDataPage(nextDataView);
       if (!skipNoData) {
-        setLoading(false);
+        stateContainer.actions.setIsLoading(false);
         return;
       }
       try {
@@ -181,7 +181,7 @@ export function DiscoverMainRoute({
 
           setBreadcrumbs({ services, titleBreadcrumbText: currentSavedSearch?.title ?? undefined });
         }
-        setLoading(false);
+        stateContainer.actions.setIsLoading(false);
         if (services.analytics) {
           const loadSavedSearchDuration = window.performance.now() - loadSavedSearchStartTime;
           reportPerformanceMetricEvent(services.analytics, {
@@ -231,7 +231,7 @@ export function DiscoverMainRoute({
 
   useEffect(() => {
     if (!isCustomizationServiceInitialized) return;
-    setLoading(true);
+    stateContainer.actions.setIsLoading(true);
     setNoDataState({
       hasESData: false,
       hasUserDataView: false,
@@ -259,13 +259,13 @@ export function DiscoverMainRoute({
   const onDataViewCreated = useCallback(
     async (nextDataView: unknown) => {
       if (nextDataView) {
-        setLoading(true);
+        stateContainer.actions.setIsLoading(true);
         setNoDataState((state) => ({ ...state, showNoDataPage: false }));
         setError(undefined);
         await loadSavedSearch({ nextDataView: nextDataView as DataView });
       }
     },
-    [loadSavedSearch]
+    [loadSavedSearch, stateContainer]
   );
 
   const onESQLNavigationComplete = useCallback(async () => {
@@ -326,14 +326,8 @@ export function DiscoverMainRoute({
       );
     }
 
-    if (loading) {
-      return loadingIndicator;
-    }
-
     return <DiscoverMainAppMemoized stateContainer={stateContainer} />;
   }, [
-    loading,
-    loadingIndicator,
     noDataDependencies,
     onDataViewCreated,
     onESQLNavigationComplete,
@@ -355,11 +349,11 @@ export function DiscoverMainRoute({
     <DiscoverCustomizationProvider value={customizationService}>
       <DiscoverMainProvider value={stateContainer}>
         <rootProfileState.AppWrapper>
-          <DiscoverTopNavInline
+          <DiscoverMainLoading
+            mainContent={mainContent}
+            showNoDataPage={noDataState.showNoDataPage}
             stateContainer={stateContainer}
-            hideNavMenuItems={loading || noDataState.showNoDataPage}
           />
-          {mainContent}
         </rootProfileState.AppWrapper>
       </DiscoverMainProvider>
     </DiscoverCustomizationProvider>
@@ -368,6 +362,28 @@ export function DiscoverMainRoute({
 // eslint-disable-next-line import/no-default-export
 export default DiscoverMainRoute;
 
+export function DiscoverMainLoading({
+  stateContainer,
+  showNoDataPage,
+  mainContent,
+}: {
+  stateContainer: DiscoverStateContainer;
+  showNoDataPage: boolean;
+  mainContent: JSX.Element;
+}) {
+  const loading = useInternalStateSelector((state) => state.isLoading);
+
+  return (
+    <>
+      <DiscoverTopNavInline
+        stateContainer={stateContainer}
+        hideNavMenuItems={showNoDataPage || loading}
+      />
+      {loading && !showNoDataPage ? <LoadingIndicator /> : mainContent}
+    </>
+  );
+}
+
 function getLoadParamsForNewSearch(stateContainer: DiscoverStateContainer): {
   nextDataView: LoadParams['dataView'];
   initialAppState: LoadParams['initialAppState'];

diff --git a/...ns/discover/public/application/main/state_management/discover_internal_state_container.ts b/...ns/discover/public/application/main/state_management/discover_internal_state_container.ts
@@ -20,6 +20,7 @@ import type { UnifiedHistogramVisContext } from '@kbn/unified-histogram-plugin/p
 export interface InternalState {
   dataView: DataView | undefined;
   isDataViewLoading: boolean;
+  isLoading: boolean;
   savedDataViews: DataViewListItem[];
   adHocDataViews: DataView[];
   expandedDoc: DataTableRecord | undefined;
@@ -32,6 +33,7 @@ export interface InternalState {
 export interface InternalStateTransitions {
   setDataView: (state: InternalState) => (dataView: DataView) => InternalState;
   setIsDataViewLoading: (state: InternalState) => (isLoading: boolean) => InternalState;
+  setIsLoading: (state: InternalState) => (isLoading: boolean) => InternalState;
   setSavedDataViews: (state: InternalState) => (dataView: DataViewListItem[]) => InternalState;
   setAdHocDataViews: (state: InternalState) => (dataViews: DataView[]) => InternalState;
   appendAdHocDataViews: (
@@ -72,6 +74,7 @@ export function getInternalStateContainer() {
     {
       dataView: undefined,
       isDataViewLoading: false,
+      isLoading: true,
       adHocDataViews: [],
       savedDataViews: [],
       expandedDoc: undefined,
@@ -90,6 +93,10 @@ export function getInternalStateContainer() {
         ...prevState,
         isDataViewLoading: loading,
       }),
+      setIsLoading: (prevState: InternalState) => (isLoading: boolean) => ({
+        ...prevState,
+        isLoading,
+      }),
       setIsESQLToDataViewTransitionModalVisible:
         (prevState: InternalState) => (isVisible: boolean) => ({
           ...prevState,

diff --git a/src/plugins/discover/public/application/main/state_management/discover_state.test.ts b/src/plugins/discover/public/application/main/state_management/discover_state.test.ts
@@ -956,6 +956,15 @@ describe('Test discover state actions', () => {
     expect(setTime).toHaveBeenCalledWith({ from: 'now-15d', to: 'now-10d' });
     expect(setRefreshInterval).toHaveBeenCalledWith({ pause: false, value: 1000 });
   });
+
+  test('setIsLoading', async () => {
+    const { state } = await getState('/');
+    expect(state.internalState.getState().isLoading).toBe(true);
+    await state.actions.setIsLoading(false);
+    expect(state.internalState.getState().isLoading).toBe(false);
+    await state.actions.setIsLoading(true);
+    expect(state.internalState.getState().isLoading).toBe(true);
+  });
 });
 
 describe('Test discover state with embedded mode', () => {

diff --git a/src/plugins/discover/public/application/main/state_management/discover_state.ts b/src/plugins/discover/public/application/main/state_management/discover_state.ts
@@ -211,6 +211,11 @@ export interface DiscoverStateContainer {
      * @param dataView
      */
     setDataView: (dataView: DataView) => void;
+    /**
+     * Set Discover to loading state on the highest level, this cleans up all internal UI state
+     * @param value
+     */
+    setIsLoading: (value: boolean) => void;
     /**
      * Undo changes made to the saved search, e.g. when the user triggers the "Reset search" button
      */
@@ -412,17 +417,24 @@ export function getDiscoverStateContainer({
     }
   };
 
+  const setIsLoading = (value: boolean) => {
+    internalStateContainer.transitions.setIsLoading(value);
+  };
+
   const onDataViewEdited = async (editedDataView: DataView) => {
+    setIsLoading(true);
+    const edited = editedDataView.id;
     if (editedDataView.isPersisted()) {
       // Clear the current data view from the cache and create a new instance
       // of it, ensuring we have a new object reference to trigger a re-render
-      services.dataViews.clearInstanceCache(editedDataView.id);
+      services.dataViews.clearInstanceCache(edited);
       setDataView(await services.dataViews.create(editedDataView.toSpec(), true));
     } else {
       await updateAdHocDataViewId();
     }
-    loadDataViewList();
+    await loadDataViewList();
     addLog('[getDiscoverStateContainer] onDataViewEdited triggers data fetching');
+    internalStateContainer.transitions.setIsLoading(false);
     fetchData();
   };
 
@@ -612,6 +624,7 @@ export function getDiscoverStateContainer({
       onDataViewCreated,
       onDataViewEdited,
       onOpenSavedSearch,
+      setIsLoading,
       transitionFromESQLToDataView,
       transitionFromDataViewToESQL,
       onUpdateQuery,

diff --git a/test/functional/apps/discover/group3/_lens_vis.ts b/test/functional/apps/discover/group3/_lens_vis.ts
@@ -110,8 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
     return seriesType;
   }
 
-  // FLAKY: https://github.com/elastic/kibana/issues/184600
-  describe.skip('discover lens vis', function () {
+  describe('discover lens vis', function () {
     before(async () => {
       await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']);
       await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional');