Skip to content

Commit

Permalink
Refactor discover loading state
Browse files Browse the repository at this point in the history
  • Loading branch information
@kertal
kertal committed Nov 13, 2024
1 parent fc4ddd2 commit 244583e
Show file tree
Hide file tree
Showing 9 changed files with 72 additions and 72 deletions.
13 changes: 1 addition & 12 deletions src/plugins/discover/public/application/main/components/layout/discover_documents.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -404,7 +393,7 @@ function DiscoverDocumentsComponent({
[viewModeToggle, callouts, loadingIndicator]
);

if (isDataViewLoading || (isEmptyDataResult && isDataLoading)) {
if (isEmptyDataResult && isDataLoading) {
return (
<div className="dscDocuments__loading">
<EuiText size="xs" color="subdued">
Expand Down
19 changes: 3 additions & 16 deletions src/plugins/discover/public/application/main/components/layout/discover_layout.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ import {
EuiPage,
EuiPageBody,
EuiPanel,
EuiProgress,
useEuiBackgroundColor,
EuiDelayRender,
} from '@elastic/eui';
import { css } from '@emotion/react';
import { i18n } from '@kbn/i18n';
Expand Down Expand Up @@ -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$);
Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -423,11 +415,6 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) {
height: 100%;
`}
>
{dataViewLoading && (
<EuiDelayRender delay={300}>
<EuiProgress size="xs" color="accent" position="absolute" />
</EuiDelayRender>
)}
<SavedSearchURLConflictCallout
savedSearch={savedSearch}
spaces={spaces}
Expand Down
6 changes: 1 addition & 5 deletions src/plugins/discover/public/application/main/components/top_nav/on_save_search.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ async function saveDataSource({
}

try {
const response = await state.savedSearchState.persist(savedSearch, saveOptions);
const response = await state.actions.persistSavedSearch(savedSearch, saveOptions);
if (response?.id) {
onSuccess(response.id!);
}
Expand Down Expand Up @@ -156,10 +156,6 @@ export async function onSaveSearch({
isTitleDuplicateConfirmed,
};

if (newCopyOnSave) {
await state.actions.updateAdHocDataViewId();
}

const navigateOrReloadSavedSearch = !Boolean(onSaveCb);
const response = await saveDataSource({
saveOptions,
Expand Down
7 changes: 5 additions & 2 deletions src/plugins/discover/public/application/main/discover_main_route.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,7 @@ export function DiscoverMainRoute({
mainContent={mainContent}
showNoDataPage={noDataState.showNoDataPage}
stateContainer={stateContainer}
loadingIndicator={loadingIndicator}
/>
</rootProfileState.AppWrapper>
</DiscoverMainProvider>
Expand All @@ -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);

Expand All @@ -379,7 +382,7 @@ export function DiscoverMainLoading({
stateContainer={stateContainer}
hideNavMenuItems={showNoDataPage || loading}
/>
{loading && !showNoDataPage ? <LoadingIndicator /> : mainContent}
{loading && !showNoDataPage ? loadingIndicator : mainContent}
</>
);
}
Expand Down
7 changes: 0 additions & 7 deletions ...ns/discover/public/application/main/state_management/discover_internal_state_container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand All @@ -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;
Expand Down Expand Up @@ -73,7 +71,6 @@ export function getInternalStateContainer() {
return createStateContainer<InternalState, InternalStateTransitions, {}>(
{
dataView: undefined,
isDataViewLoading: false,
isLoading: true,
adHocDataViews: [],
savedDataViews: [],
Expand All @@ -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,
Expand Down
7 changes: 3 additions & 4 deletions src/plugins/discover/public/application/main/state_management/discover_state.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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();
});

Expand Down
61 changes: 45 additions & 16 deletions src/plugins/discover/public/application/main/state_management/discover_state.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -177,6 +177,8 @@ export interface DiscoverStateContainer {
* @param dataView
*/
onDataViewEdited: (dataView: DataView) => Promise<void>;

onDataViewFieldEdited: () => Promise<void>;
/**
* Triggered when transitioning from ESQL to Dataview
* Clean ups the ES|QL query and moves to the dataview mode
Expand Down Expand Up @@ -221,10 +223,14 @@ export interface DiscoverStateContainer {
*/
undoSavedSearchChanges: () => Promise<SavedSearch>;
/**
* 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<DataView | undefined>;
persistSavedSearch: (
savedSearch: SavedSearch,
saveOptions?: SaveSavedSearchOptions
) => Promise<{ id: string | undefined } | undefined>;
/**
* Updates the ES|QL query string
*/
Expand Down Expand Up @@ -338,8 +344,6 @@ export function getDiscoverStateContainer({
id: uuidv4(),
});

services.dataViews.clearInstanceCache(prevDataView.id);

updateFiltersReferences({
prevDataView,
nextDataView,
Expand All @@ -358,7 +362,6 @@ export function getDiscoverStateContainer({

const trackingEnabled = Boolean(nextDataView.isPersisted() || savedSearchContainer.getId());
services.urlTracker.setTrackingEnabled(trackingEnabled);

return nextDataView;
};

Expand Down Expand Up @@ -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<SavedSearch> => {
Expand Down Expand Up @@ -552,6 +554,7 @@ export function getDiscoverStateContainer({
services,
internalState: internalStateContainer,
appState: appStateContainer,
savedSearchState: savedSearchContainer,
});
};

Expand Down Expand Up @@ -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,
Expand All @@ -623,14 +651,15 @@ export function getDiscoverStateContainer({
createAndAppendAdHocDataView,
onDataViewCreated,
onDataViewEdited,
onDataViewFieldEdited,
onOpenSavedSearch,
setIsLoading,
transitionFromESQLToDataView,
transitionFromDataViewToESQL,
onUpdateQuery,
persistSavedSearch,
setDataView,
undoSavedSearchChanges,
updateAdHocDataViewId,
updateESQLQuery,
},
};
Expand Down
14 changes: 7 additions & 7 deletions src/plugins/discover/public/application/main/state_management/utils/change_data_view.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<PureTransitionsToTransitions<InternalStateTransitions>>;
return {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand Down
Loading

0 comments on commit 244583e

Please sign in to comment.