diff --git a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx index 6092a59333290..dffbb5fcf1f33 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx @@ -156,17 +156,6 @@ function DiscoverDocumentsComponent({ documentState.fetchStatus === FetchStatus.LOADING || documentState.fetchStatus === FetchStatus.PARTIAL; - // This is needed to prevent EuiDataGrid pushing onSort because the data view has been switched. - // It's just necessary for non ES|QL requests since they don't have a partial result state, that's - // considered as loading state in the Component. - // 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 - // 5. this is propagated to Discover's URL and causes an unwanted change of state to an unsorted state - // This solution switches to the loading state in this component when the URL index doesn't match the dataView.id - const isDataViewLoading = - useInternalStateSelector((state) => state.isDataViewLoading) && !isEsqlMode; const isEmptyDataResult = isEsqlMode || !documentState.result || documentState.result.length === 0; const rows = useMemo(() => documentState.result || [], [documentState.result]); @@ -404,7 +393,7 @@ function DiscoverDocumentsComponent({ [viewModeToggle, callouts, loadingIndicator] ); - if (isDataViewLoading || (isEmptyDataResult && isDataLoading)) { + if (isEmptyDataResult && isDataLoading) { return (
diff --git a/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx b/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx index 7784b8308053e..c9daa360aa6c5 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx @@ -9,14 +9,7 @@ import './discover_layout.scss'; import React, { ReactElement, useCallback, useEffect, useMemo, useRef, useState } from 'react'; -import { - EuiPage, - EuiPageBody, - EuiPanel, - EuiProgress, - useEuiBackgroundColor, - EuiDelayRender, -} from '@elastic/eui'; +import { EuiPage, EuiPageBody, EuiPanel, useEuiBackgroundColor } from '@elastic/eui'; import { css } from '@emotion/react'; import { i18n } from '@kbn/i18n'; import { isOfAggregateQueryType } from '@kbn/es-query'; @@ -101,10 +94,7 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) { } return state.viewMode ?? VIEW_MODE.DOCUMENT_LEVEL; }); - const [dataView, dataViewLoading] = useInternalStateSelector((state) => [ - state.dataView!, - state.isDataViewLoading, - ]); + const [dataView] = useInternalStateSelector((state) => [state.dataView!]); const customFilters = useInternalStateSelector((state) => state.customFilters); const dataState: DataMainMsg = useDataState(main$); @@ -276,12 +266,9 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) { if (removedFieldName && currentColumns.includes(removedFieldName)) { onRemoveColumn(removedFieldName); } - if (!dataView.isPersisted()) { - await stateContainer.actions.updateAdHocDataViewId(); - } - stateContainer.dataState.refetch$.next('reset'); + await stateContainer.actions.onDataViewFieldEdited(); }, - [dataView, stateContainer, currentColumns, onRemoveColumn] + [stateContainer, currentColumns, onRemoveColumn] ); const onDisableFilters = useCallback(() => { @@ -423,11 +410,6 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) { height: 100%; `} > - {dataViewLoading && ( - - - - )} { 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 index 205926cf4943b..c3d92ce3a78c4 100644 --- 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(); - 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 ; }, [ - loading, - loadingIndicator, noDataDependencies, onDataViewCreated, onESQLNavigationComplete, @@ -355,11 +349,12 @@ export function DiscoverMainRoute({ - - {mainContent} @@ -368,6 +363,30 @@ export function DiscoverMainRoute({ // eslint-disable-next-line import/no-default-export export default DiscoverMainRoute; +export function DiscoverMainLoading({ + stateContainer, + showNoDataPage, + mainContent, + loadingIndicator, +}: { + stateContainer: DiscoverStateContainer; + showNoDataPage: boolean; + mainContent: React.ReactNode; + loadingIndicator: React.ReactNode; +}) { + const loading = useInternalStateSelector((state) => state.isLoading); + + return ( + <> + + {loading && !showNoDataPage ? loadingIndicator : mainContent} + + ); +} + function getLoadParamsForNewSearch(stateContainer: DiscoverStateContainer): { nextDataView: LoadParams['dataView']; initialAppState: LoadParams['initialAppState']; diff --git a/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts b/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts index d5f7deecf980a..b52c7a0d0973b 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts +++ b/src/plugins/discover/public/application/main/state_management/discover_internal_state_container.ts @@ -19,7 +19,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; @@ -31,7 +31,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: ( @@ -71,7 +71,7 @@ export function getInternalStateContainer() { return createStateContainer( { dataView: undefined, - isDataViewLoading: false, + isLoading: true, adHocDataViews: [], savedDataViews: [], expandedDoc: undefined, @@ -86,9 +86,9 @@ export function getInternalStateContainer() { expandedDoc: nextDataView?.id !== prevState.dataView?.id ? undefined : prevState.expandedDoc, }), - setIsDataViewLoading: (prevState: InternalState) => (loading: boolean) => ({ + setIsLoading: (prevState: InternalState) => (isLoading: boolean) => ({ ...prevState, - isDataViewLoading: loading, + isLoading, }), setIsESQLToDataViewTransitionModalVisible: (prevState: InternalState) => (isVisible: boolean) => ({ 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 index a64e36bc39097..09cdd389b0ba7 100644 --- 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 @@ -851,10 +851,8 @@ describe('Test discover state actions', () => { }); const unsubscribe = state.actions.initializeAndSync(); await state.actions.onDataViewEdited(dataViewMock); - - await waitFor(() => { - expect(state.internalState.getState().dataView).not.toBe(selectedDataView); - }); + expect(state.internalState.getState().dataView?.id).toBe(dataViewMock.id); + expect(state.dataState.fetch).toHaveBeenCalledTimes(1); unsubscribe(); }); test('onDataViewEdited - ad-hoc data view', async () => { @@ -866,6 +864,7 @@ describe('Test discover state actions', () => { await waitFor(() => { expect(state.internalState.getState().dataView?.id).not.toBe(previousId); }); + expect(state.dataState.fetch).toHaveBeenCalledTimes(1); unsubscribe(); }); @@ -956,6 +955,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 index 56d1ca44c3853..9766ae773d787 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_state.ts +++ b/src/plugins/discover/public/application/main/state_management/discover_state.ts @@ -21,7 +21,7 @@ import { SearchSessionInfoProvider, } from '@kbn/data-plugin/public'; import { DataView, DataViewSpec, DataViewType } from '@kbn/data-views-plugin/public'; -import type { SavedSearch } from '@kbn/saved-search-plugin/public'; +import { SavedSearch, SaveSavedSearchOptions } from '@kbn/saved-search-plugin/public'; import { v4 as uuidv4 } from 'uuid'; import { merge } from 'rxjs'; import { getInitialESQLQuery } from '@kbn/esql-utils'; @@ -177,6 +177,8 @@ export interface DiscoverStateContainer { * @param dataView */ onDataViewEdited: (dataView: DataView) => Promise; + + onDataViewFieldEdited: () => Promise; /** * Triggered when transitioning from ESQL to Dataview * Clean ups the ES|QL query and moves to the dataview mode @@ -211,15 +213,24 @@ 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 */ undoSavedSearchChanges: () => Promise; /** - * When saving a saved search with an ad hoc data view, a new id needs to be generated for the data view - * This is to prevent duplicate ids messing with our system + * Persist the given saved search + * @param savedSearch + * @param saveOptions */ - updateAdHocDataViewId: () => Promise; + persistSavedSearch: ( + savedSearch: SavedSearch, + saveOptions?: SaveSavedSearchOptions + ) => Promise<{ id: string | undefined } | undefined>; /** * Updates the ES|QL query string */ @@ -333,8 +344,6 @@ export function getDiscoverStateContainer({ id: uuidv4(), }); - services.dataViews.clearInstanceCache(prevDataView.id); - updateFiltersReferences({ prevDataView, nextDataView, @@ -353,7 +362,6 @@ export function getDiscoverStateContainer({ const trackingEnabled = Boolean(nextDataView.isPersisted() || savedSearchContainer.getId()); services.urlTracker.setTrackingEnabled(trackingEnabled); - return nextDataView; }; @@ -412,18 +420,24 @@ export function getDiscoverStateContainer({ } }; + const setIsLoading = (value: boolean) => { + internalStateContainer.transitions.setIsLoading(value); + }; + const onDataViewEdited = 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 - services.dataViews.clearInstanceCache(editedDataView.id); - setDataView(await services.dataViews.create(editedDataView.toSpec(), true)); - } else { + setIsLoading(true); + const edited = editedDataView.id; + let clearCache = false; + if (!editedDataView.isPersisted()) { await updateAdHocDataViewId(); + clearCache = true; } - loadDataViewList(); addLog('[getDiscoverStateContainer] onDataViewEdited triggers data fetching'); fetchData(); + if (clearCache) { + services.dataViews.clearInstanceCache(edited); + } + setIsLoading(false); }; const loadSavedSearch = async (params?: LoadParams): Promise => { @@ -540,6 +554,7 @@ export function getDiscoverStateContainer({ services, internalState: internalStateContainer, appState: appStateContainer, + savedSearchState: savedSearchContainer, }); }; @@ -593,6 +608,31 @@ export function getDiscoverStateContainer({ appStateContainer.update({ query }); }; + const persistSavedSearch = async ( + savedSearch: SavedSearch, + saveOptions?: SaveSavedSearchOptions + ) => { + const prevDataView = internalStateContainer.getState().dataView; + if (prevDataView && !prevDataView?.isPersisted() && saveOptions?.copyOnSave) { + await updateAdHocDataViewId(); + services.dataViews.clearInstanceCache(prevDataView.id!); + } + return savedSearchContainer.persist(savedSearch, saveOptions); + }; + + const onDataViewFieldEdited = async () => { + setIsLoading(true); + const dataView = internalStateContainer.getState().dataView; + if (!dataView?.isPersisted()) { + await updateAdHocDataViewId(); + } + dataStateContainer.refetch$.next('reset'); + setIsLoading(false); + if (dataView && !dataView?.isPersisted()) { + services.dataViews.clearInstanceCache(dataView.id!); + } + }; + return { globalState: globalStateContainer, appState: appStateContainer, @@ -611,13 +651,15 @@ export function getDiscoverStateContainer({ createAndAppendAdHocDataView, onDataViewCreated, onDataViewEdited, + onDataViewFieldEdited, onOpenSavedSearch, + setIsLoading, transitionFromESQLToDataView, transitionFromDataViewToESQL, onUpdateQuery, + persistSavedSearch, setDataView, undoSavedSearchChanges, - updateAdHocDataViewId, updateESQLQuery, }, }; diff --git a/src/plugins/discover/public/application/main/state_management/utils/change_data_view.test.ts b/src/plugins/discover/public/application/main/state_management/utils/change_data_view.test.ts index ff25a77930ebb..cc52a19fc2f24 100644 --- a/src/plugins/discover/public/application/main/state_management/utils/change_data_view.test.ts +++ b/src/plugins/discover/public/application/main/state_management/utils/change_data_view.test.ts @@ -31,13 +31,14 @@ const setupTestParams = (dataView: DataView | undefined) => { services.dataViews.get = jest.fn(() => Promise.resolve(dataView as DataView)); discoverState.appState.update = jest.fn(); discoverState.internalState.transitions = { - setIsDataViewLoading: jest.fn(), + setIsLoading: jest.fn(), setResetDefaultProfileState: jest.fn(), } as unknown as Readonly>; return { services, appState: discoverState.appState, internalState: discoverState.internalState, + savedSearchState: discoverState.savedSearchState, }; }; @@ -50,8 +51,8 @@ describe('changeDataView', () => { dataSource: createDataViewDataSource({ dataViewId: 'data-view-with-user-default-column-id' }), sort: [['@timestamp', 'desc']], }); - expect(params.internalState.transitions.setIsDataViewLoading).toHaveBeenNthCalledWith(1, true); - expect(params.internalState.transitions.setIsDataViewLoading).toHaveBeenNthCalledWith(2, false); + expect(params.internalState.transitions.setIsLoading).toHaveBeenNthCalledWith(1, true); + expect(params.internalState.transitions.setIsLoading).toHaveBeenNthCalledWith(2, false); }); it('should set the right app state when a valid data view to switch to is given', async () => { @@ -62,16 +63,16 @@ describe('changeDataView', () => { dataSource: createDataViewDataSource({ dataViewId: 'data-view-with-various-field-types-id' }), sort: [['data', 'desc']], }); - expect(params.internalState.transitions.setIsDataViewLoading).toHaveBeenNthCalledWith(1, true); - expect(params.internalState.transitions.setIsDataViewLoading).toHaveBeenNthCalledWith(2, false); + expect(params.internalState.transitions.setIsLoading).toHaveBeenNthCalledWith(1, true); + expect(params.internalState.transitions.setIsLoading).toHaveBeenNthCalledWith(2, false); }); it('should not set the app state when an invalid data view to switch to is given', async () => { const params = setupTestParams(undefined); await changeDataView('data-view-with-various-field-types', params); expect(params.appState.update).not.toHaveBeenCalled(); - expect(params.internalState.transitions.setIsDataViewLoading).toHaveBeenNthCalledWith(1, true); - expect(params.internalState.transitions.setIsDataViewLoading).toHaveBeenNthCalledWith(2, false); + expect(params.internalState.transitions.setIsLoading).toHaveBeenNthCalledWith(1, true); + expect(params.internalState.transitions.setIsLoading).toHaveBeenNthCalledWith(2, false); }); it('should call setResetDefaultProfileState correctly when switching data view', async () => { diff --git a/src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts b/src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts index 54885c5de9bfd..45b3920e8c377 100644 --- a/src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts +++ b/src/plugins/discover/public/application/main/state_management/utils/change_data_view.ts @@ -14,6 +14,7 @@ import { SORT_DEFAULT_ORDER_SETTING, DEFAULT_COLUMNS_SETTING, } from '@kbn/discover-utils'; +import { DiscoverSavedSearchContainer } from '../discover_saved_search_container'; import { DiscoverInternalStateContainer } from '../discover_internal_state_container'; import { DiscoverAppStateContainer } from '../discover_app_state_container'; import { addLog } from '../../../../utils/add_log'; @@ -29,10 +30,12 @@ export async function changeDataView( services, internalState, appState, + savedSearchState, }: { services: DiscoverServices; internalState: DiscoverInternalStateContainer; appState: DiscoverAppStateContainer; + savedSearchState: DiscoverSavedSearchContainer; } ) { addLog('[ui] changeDataView', { id }); @@ -42,7 +45,7 @@ export async function changeDataView( const state = appState.getState(); let nextDataView: DataView | null = null; - internalState.transitions.setIsDataViewLoading(true); + internalState.transitions.setIsLoading(true); internalState.transitions.setResetDefaultProfileState({ columns: true, rowHeight: true }); try { @@ -62,7 +65,8 @@ export async function changeDataView( uiSettings.get(SORT_DEFAULT_ORDER_SETTING), state.query ); - + savedSearchState.update({ nextDataView, nextState: nextAppState }); + internalState.transitions.setDataView(nextDataView); appState.update(nextAppState); if (internalState.getState().expandedDoc) { @@ -70,5 +74,5 @@ export async function changeDataView( } } - internalState.transitions.setIsDataViewLoading(false); + internalState.transitions.setIsLoading(false); } diff --git a/test/functional/apps/discover/group3/_lens_vis.ts b/test/functional/apps/discover/group3/_lens_vis.ts index 03641ee5bcb41..0864382cad7a8 100644 --- 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');