Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[discover-next][bug] add max height to dataset navigator and use memoization #7540

Merged
merged 7 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"start": "scripts/use_node scripts/opensearch_dashboards --dev",
"start:docker": "scripts/use_node scripts/opensearch_dashboards --dev --opensearch.hosts=$OPENSEARCH_HOSTS --opensearch.ignoreVersionMismatch=true --server.host=$SERVER_HOST",
"start:security": "scripts/use_node scripts/opensearch_dashboards --dev --security",
"start:enhancements": "scripts/use_node scripts/opensearch_dashboards --dev --uiSettings.overrides['query:enhancements:enabled']=true --uiSettings.overrides['home:useNewHomePage']=true",
"start:enhancements": "scripts/use_node scripts/opensearch_dashboards --dev --uiSettings.overrides['query:enhancements:enabled']=true --uiSettings.overrides['home:useNewHomePage']=true --uiSettings.overrides['state:storeInSessionStorage']=true",
"debug": "scripts/use_node --nolazy --inspect scripts/opensearch_dashboards --dev",
"debug-break": "scripts/use_node --nolazy --inspect-brk scripts/opensearch_dashboards --dev",
"lint": "yarn run lint:es && yarn run lint:style",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
services,
}: QuerySuggestionGetFnArgs): Promise<QuerySuggestion[]> => {
const { api } = services.uiSettings;
const dataSetManager = services.data.query.dataSet;
const dataSetManager = services.data.query.dataSetManager;

Check warning on line 49 in src/plugins/data/public/antlr/opensearch_sql/code_completion.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/antlr/opensearch_sql/code_completion.ts#L49

Added line #L49 was not covered by tests
const suggestions = getOpenSearchSqlAutoCompleteSuggestions(query, {
line: position?.lineNumber || selectionStart,
column: position?.column || selectionEnd,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { CoreStart } from 'opensearch-dashboards/public';
import { skip } from 'rxjs/operators';
import {
IndexPattern,
SIMPLE_DATA_SET_TYPES,
SimpleDataSet,
SimpleDataSource,
Expand All @@ -24,9 +25,33 @@
}

public init = async (indexPatterns: IndexPatternsContract) => {
if (!this.uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED)) return;
this.indexPatterns = indexPatterns;
this.defaultDataSet = await this.fetchDefaultDataSet();
return this.defaultDataSet;
};

public initWithIndexPattern = (indexPattern: IndexPattern | null) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be setDataset

if (!this.uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED)) return;
if (!indexPattern || !indexPattern.id) {
return undefined;

Check warning on line 36 in src/plugins/data/public/query/dataset_manager/dataset_manager.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/dataset_manager/dataset_manager.ts#L36

Added line #L36 was not covered by tests
}

this.defaultDataSet = {

Check warning on line 39 in src/plugins/data/public/query/dataset_manager/dataset_manager.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/dataset_manager/dataset_manager.ts#L39

Added line #L39 was not covered by tests
id: indexPattern.id,
title: indexPattern.title,
type: SIMPLE_DATA_SET_TYPES.INDEX_PATTERN,
timeFieldName: indexPattern.timeFieldName,
fields: indexPattern.fields,
...(indexPattern.dataSourceRef
? {
dataSourceRef: {
id: indexPattern.dataSourceRef?.id,
name: indexPattern.dataSourceRef?.name,
type: indexPattern.dataSourceRef?.type,
} as SimpleDataSource,
}
: {}),
};
};

public getUpdates$ = () => {
Expand All @@ -43,6 +68,13 @@
*/
public setDataSet = (dataSet: SimpleDataSet | undefined) => {
if (!this.uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED)) return;

// if (dataSet) {
// const { fields, ...dataSetWithoutFields } = dataSet;
// this.dataSet$.next(dataSetWithoutFields);
// } else {
// this.dataSet$.next(undefined);
// }
this.dataSet$.next(dataSet);
};

Expand All @@ -57,11 +89,7 @@
}

const indexPattern = await this.indexPatterns?.get(defaultIndexPatternId);
if (!indexPattern) {
return undefined;
}

if (!indexPattern.id) {
if (!indexPattern || !indexPattern.id) {
return undefined;
}

Expand Down
10 changes: 5 additions & 5 deletions src/plugins/data/public/query/query_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { TimefilterService, TimefilterSetup } from './timefilter';
import { createSavedQueryService } from './saved_query/saved_query_service';
import { createQueryStateObservable } from './state_sync/create_global_query_observable';
import { QueryStringManager, QueryStringContract } from './query_string';
import { DataSetManager } from './dataset_manager';
import { DataSetContract, DataSetManager } from './dataset_manager';
import { buildOpenSearchQuery, getOpenSearchQueryConfig, IndexPatternsService } from '../../common';
import { getUiSettings } from '../services';
import { IndexPattern } from '..';
Expand All @@ -63,7 +63,7 @@ export class QueryService {
filterManager!: FilterManager;
timefilter!: TimefilterSetup;
queryStringManager!: QueryStringContract;
dataSetManager!: DataSetManager;
dataSetManager!: DataSetContract;

state$!: ReturnType<typeof createQueryStateObservable>;

Expand All @@ -83,14 +83,14 @@ export class QueryService {
filterManager: this.filterManager,
timefilter: this.timefilter,
queryString: this.queryStringManager,
dataSet: this.dataSetManager,
dataSetManager: this.dataSetManager,
}).pipe(share());

return {
filterManager: this.filterManager,
timefilter: this.timefilter,
queryString: this.queryStringManager,
dataSet: this.dataSetManager,
dataSetManager: this.dataSetManager,
state$: this.state$,
};
}
Expand All @@ -109,7 +109,7 @@ export class QueryService {
}),
filterManager: this.filterManager,
queryString: this.queryStringManager,
dataSet: this.dataSetManager,
dataSetManager: this.dataSetManager,
savedQueries: createSavedQueryService(savedObjectsClient),
state$: this.state$,
timefilter: this.timefilter,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ setupMock.uiSettings.get.mockImplementation((key: string) => {
return { from: 'now-15m', to: 'now' };
case UI_SETTINGS.TIMEPICKER_REFRESH_INTERVAL_DEFAULTS:
return { pause: false, value: 0 };
case UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED:
return false;
default:
throw new Error(`sync_query test: not mocked uiSetting: ${key}`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@
*/
export const connectStorageToQueryState = async (
{
dataSet,
dataSetManager,
filterManager,
queryString,
state$,
}: Pick<
QueryStart | QuerySetup,
'timefilter' | 'filterManager' | 'queryString' | 'dataSet' | 'state$'
'timefilter' | 'filterManager' | 'queryString' | 'dataSetManager' | 'state$'
>,
OsdUrlStateStorage: IOsdUrlStateStorage,
syncConfig: {
Expand All @@ -89,7 +89,7 @@
filters: filterManager.getAppFilters(),
...(uiSettings &&
uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) && {
dataSet: dataSet.getDataSet(),
dataSet: dataSetManager.getDataSet(),
}),
};

Expand All @@ -106,13 +106,16 @@
}
}

if (syncConfig.dataSet && !_.isEqual(initialStateFromURL.dataSet, dataSet.getDataSet())) {
if (
syncConfig.dataSet &&
!_.isEqual(initialStateFromURL.dataSet, dataSetManager.getDataSet())
) {
if (initialStateFromURL.dataSet) {
dataSet.setDataSet(_.cloneDeep(initialStateFromURL.dataSet));
dataSetManager.setDataSet(_.cloneDeep(initialStateFromURL.dataSet));

Check warning on line 114 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L114

Added line #L114 was not covered by tests
} else {
const defaultDataSet = await dataSet.getDefaultDataSet();
const defaultDataSet = await dataSetManager.getDefaultDataSet();

Check warning on line 116 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L116

Added line #L116 was not covered by tests
if (defaultDataSet) {
dataSet.setDataSet(defaultDataSet);
dataSetManager.setDataSet(defaultDataSet);

Check warning on line 118 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L118

Added line #L118 was not covered by tests
}
}
}
Expand Down Expand Up @@ -150,7 +153,7 @@
}

if (syncConfig.dataSet && changes.dataSet) {
newState.dataSet = dataSet.getDataSet();
newState.dataSet = dataSetManager.getDataSet();

Check warning on line 156 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L156

Added line #L156 was not covered by tests
}

return newState;
Expand Down Expand Up @@ -182,11 +185,11 @@
timefilter: { timefilter },
filterManager,
queryString,
dataSet,
dataSetManager,
state$,
}: Pick<
QueryStart | QuerySetup,
'timefilter' | 'filterManager' | 'dataSet' | 'queryString' | 'state$'
'timefilter' | 'filterManager' | 'dataSetManager' | 'queryString' | 'state$'
>,
stateContainer: BaseStateContainer<S>,
syncConfig: {
Expand Down Expand Up @@ -278,7 +281,7 @@
}

if (syncConfig.dataSet && !initialState.dataSet) {
initialState.dataSet = dataSet.getDefaultDataSet();
initialState.dataSet = dataSetManager.getDefaultDataSet();
initialDirty = true;
}

Expand Down Expand Up @@ -320,7 +323,7 @@
}
}
if (syncConfig.dataSet && changes.dataSet) {
newState.dataSet = dataSet.getDataSet();
newState.dataSet = dataSetManager.getDataSet();

Check warning on line 326 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L326

Added line #L326 was not covered by tests
}
return newState;
})
Expand Down Expand Up @@ -382,14 +385,14 @@
}

if (syncConfig.dataSet) {
const currentDataSet = dataSet.getDataSet();
const currentDataSet = dataSetManager.getDataSet();
if (!_.isEqual(state.dataSet, currentDataSet)) {
if (state.dataSet) {
dataSet.setDataSet(state.dataSet);
dataSetManager.setDataSet(state.dataSet);

Check warning on line 391 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L391

Added line #L391 was not covered by tests
} else {
const defaultDataSet = await dataSet.getDefaultDataSet();
const defaultDataSet = dataSetManager.getDefaultDataSet();

Check warning on line 393 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L393

Added line #L393 was not covered by tests
if (defaultDataSet) {
dataSet.setDataSet(defaultDataSet);
dataSetManager.setDataSet(defaultDataSet);

Check warning on line 395 in src/plugins/data/public/query/state_sync/connect_to_query_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/connect_to_query_state.ts#L395

Added line #L395 was not covered by tests
stateContainer.set({ ...stateContainer.get(), dataSet: defaultDataSet });
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,20 @@
timefilter: { timefilter },
filterManager,
queryString,
dataSet,
dataSetManager,
}: {
timefilter: TimefilterSetup;
filterManager: FilterManager;
queryString: QueryStringContract;
dataSet: DataSetContract;
dataSetManager: DataSetContract;
}): Observable<{ changes: QueryStateChange; state: QueryState }> {
return new Observable((subscriber) => {
const state = createStateContainer<QueryState>({
time: timefilter.getTime(),
refreshInterval: timefilter.getRefreshInterval(),
filters: filterManager.getFilters(),
query: queryString.getQuery(),
dataSet: dataSet.getDataSet(),
dataSet: dataSetManager.getDataSet(),
});

let currentChange: QueryStateChange = {};
Expand All @@ -64,9 +64,9 @@
currentChange.query = true;
state.set({ ...state.get(), query: queryString.getQuery() });
}),
dataSet.getUpdates$().subscribe(() => {
dataSetManager.getUpdates$().subscribe(() => {
currentChange.dataSet = true;
state.set({ ...state.get(), dataSet: dataSet.getDataSet() });
state.set({ ...state.get(), dataSet: dataSetManager.getDataSet() });

Check warning on line 69 in src/plugins/data/public/query/state_sync/create_global_query_observable.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/state_sync/create_global_query_observable.ts#L69

Added line #L69 was not covered by tests
}),
timefilter.getTimeUpdate$().subscribe(() => {
currentChange.time = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ setupMock.uiSettings.get.mockImplementation((key: string) => {
return 'kuery';
case UI_SETTINGS.TIMEPICKER_REFRESH_INTERVAL_DEFAULTS:
return { pause: false, value: 0 };
case UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED:
return false;
default:
throw new Error(`sync_query test: not mocked uiSetting: ${key}`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,23 @@ const GLOBAL_STATE_STORAGE_KEY = '_g';
export const syncQueryStateWithUrl = (
query: Pick<
QueryStart | QuerySetup,
'filterManager' | 'timefilter' | 'queryString' | 'dataSet' | 'state$'
'filterManager' | 'timefilter' | 'queryString' | 'dataSetManager' | 'state$'
>,
osdUrlStateStorage: IOsdUrlStateStorage,
uiSettings?: CoreStart['uiSettings']
) => {
const {
timefilter: { timefilter },
filterManager,
dataSet,
dataSetManager,
} = query;
const defaultState: QueryState = {
time: timefilter.getTime(),
refreshInterval: timefilter.getRefreshInterval(),
filters: filterManager.getGlobalFilters(),
...(uiSettings &&
uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) && {
dataSet: dataSet.getDataSet(),
dataSet: dataSetManager.getDataSet(),
}),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,42 @@
.dataSetNavigator {
padding: $euiSizeXS;
color: $euiColorPrimaryText;
max-height: 60vh;
overflow-y: auto;
scrollbar-width: thin;
scrollbar-color: $euiColorMediumShade $euiColorLightestShade;

&__icon {
margin-right: 4px;
}
}

.dataSetNavigatorFormWrapper {
padding: $euiSizeS;
&__loading {
padding: $euiSizeS;
}

&:not(:hover)::-webkit-scrollbar {
width: 0;
background: transparent;
}

&::-webkit-scrollbar {
width: 4px;
}

&::-webkit-scrollbar-track {
background: $euiColorLightestShade;
}

&::-webkit-scrollbar-thumb {
background-color: $euiColorMediumShade;
border-radius: 4px;
}
Comment on lines +22 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just use the EUI Scrollbar mixin here


&__menu {
width: 365px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a fixed width?

}
}

.dataSetNavigator__loading {
.dataSetNavigatorFormWrapper {
padding: $euiSizeS;
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ import { DataSetContract } from '../../query';
export function createDataSetNavigator(
savedObjectsClient: SavedObjectsClientContract,
http: HttpStart,
dataSet: DataSetContract
dataSetManager: DataSetContract
) {
// Return a function that takes props, omitting the dependencies from the props type
return (props: Omit<DataSetNavigatorProps, 'savedObjectsClient' | 'http' | 'dataSet'>) => (
<DataSetNavigator
{...props}
savedObjectsClient={savedObjectsClient}
http={http}
dataSet={dataSet}
dataSetManager={dataSetManager}
/>
);
}
Loading
Loading