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..0b3b16ad6a1e1 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 @@ -13,9 +13,7 @@ import { EuiPage, EuiPageBody, EuiPanel, - EuiProgress, useEuiBackgroundColor, - EuiDelayRender, } from '@elastic/eui'; import { css } from '@emotion/react'; import { i18n } from '@kbn/i18n'; @@ -101,10 +99,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 +271,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 +415,6 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) { height: 100%; `} > - {dataViewLoading && ( - - - - )} @@ -366,10 +367,12 @@ export function DiscoverMainLoading({ stateContainer, showNoDataPage, mainContent, + loadingIndicator, }: { stateContainer: DiscoverStateContainer; showNoDataPage: boolean; - mainContent: JSX.Element; + mainContent: React.ReactNode; + loadingIndicator: React.ReactNode; }) { const loading = useInternalStateSelector((state) => state.isLoading); @@ -379,7 +382,7 @@ export function DiscoverMainLoading({ stateContainer={stateContainer} hideNavMenuItems={showNoDataPage || loading} /> - {loading && !showNoDataPage ? : mainContent} + {loading && !showNoDataPage ? loadingIndicator : mainContent} ); } 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 75cdcecd336c8..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,6 @@ import type { UnifiedHistogramVisContext } from '@kbn/unified-histogram-plugin/p export interface InternalState { dataView: DataView | undefined; - isDataViewLoading: boolean; isLoading: boolean; savedDataViews: DataViewListItem[]; adHocDataViews: DataView[]; @@ -32,7 +31,6 @@ 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; @@ -73,7 +71,6 @@ export function getInternalStateContainer() { return createStateContainer( { dataView: undefined, - isDataViewLoading: false, isLoading: true, adHocDataViews: [], savedDataViews: [], @@ -89,10 +86,6 @@ export function getInternalStateContainer() { expandedDoc: nextDataView?.id !== prevState.dataView?.id ? undefined : prevState.expandedDoc, }), - setIsDataViewLoading: (prevState: InternalState) => (loading: boolean) => ({ - ...prevState, - isDataViewLoading: loading, - }), setIsLoading: (prevState: InternalState) => (isLoading: boolean) => ({ ...prevState, isLoading, 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 cb5ba0dcdbced..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(); }); 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 3dce218b5fce8..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 @@ -221,10 +223,14 @@ export interface DiscoverStateContainer { */ 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 */ @@ -338,8 +344,6 @@ export function getDiscoverStateContainer({ id: uuidv4(), }); - services.dataViews.clearInstanceCache(prevDataView.id); - updateFiltersReferences({ prevDataView, nextDataView, @@ -358,7 +362,6 @@ export function getDiscoverStateContainer({ const trackingEnabled = Boolean(nextDataView.isPersisted() || savedSearchContainer.getId()); services.urlTracker.setTrackingEnabled(trackingEnabled); - return nextDataView; }; @@ -424,18 +427,17 @@ export function getDiscoverStateContainer({ 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(edited); - setDataView(await services.dataViews.create(editedDataView.toSpec(), true)); - } else { + let clearCache = false; + if (!editedDataView.isPersisted()) { await updateAdHocDataViewId(); + clearCache = true; } - await loadDataViewList(); addLog('[getDiscoverStateContainer] onDataViewEdited triggers data fetching'); - setIsLoading(false); fetchData(); + if (clearCache) { + services.dataViews.clearInstanceCache(edited); + } + setIsLoading(false); }; const loadSavedSearch = async (params?: LoadParams): Promise => { @@ -552,6 +554,7 @@ export function getDiscoverStateContainer({ services, internalState: internalStateContainer, appState: appStateContainer, + savedSearchState: savedSearchContainer, }); }; @@ -605,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, @@ -623,14 +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..7711d99edf4f5 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,7 +31,7 @@ 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 { @@ -50,8 +50,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 +62,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); }