Skip to content

Commit

Permalink
[8.x] [Controls] Fix load more request size (#207901) (#208657)
Browse files Browse the repository at this point in the history
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Controls] Fix load more request size
(#207901)](#207901)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Hannah
Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-01-29T00:27:23Z","message":"[Controls]
Fix load more request size (#207901)\n\nCloses
https://github.com/elastic/kibana/issues/207884\r\n\r\n##
Summary\r\n\r\nThis PR ensures that the options list control only
fetches a maximum of\r\n**1,000** terms on scroll rather than the whole
cardinality of the given\r\nfield. This prevents the user from hitting
an error when the cardinality\r\nof their field is greater than their
`search.max_buckets` setting\r\n(which, by default, is set to
`65,536`)\r\n\r\n**Before**\r\n\r\n\r\nhttps://github.com/user-attachments/assets/0b60818a-497e-4f1f-b2ed-788c7e7371e3\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/user-attachments/assets/2e52b45b-9e35-4e7e-920c-c0b4fe3aa728\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] The PR
description includes the appropriate Release Notes section,\r\nand the
correct `release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"1f3c9cb9bb39699ca8dc0a83d6da59914a654977","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:Presentation","loe:small","impact:medium","v9.0.0","backport:prev-major"],"title":"[Controls]
Fix load more request
size","number":207901,"url":"https://github.com/elastic/kibana/pull/207901","mergeCommit":{"message":"[Controls]
Fix load more request size (#207901)\n\nCloses
https://github.com/elastic/kibana/issues/207884\r\n\r\n##
Summary\r\n\r\nThis PR ensures that the options list control only
fetches a maximum of\r\n**1,000** terms on scroll rather than the whole
cardinality of the given\r\nfield. This prevents the user from hitting
an error when the cardinality\r\nof their field is greater than their
`search.max_buckets` setting\r\n(which, by default, is set to
`65,536`)\r\n\r\n**Before**\r\n\r\n\r\nhttps://github.com/user-attachments/assets/0b60818a-497e-4f1f-b2ed-788c7e7371e3\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/user-attachments/assets/2e52b45b-9e35-4e7e-920c-c0b4fe3aa728\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] The PR
description includes the appropriate Release Notes section,\r\nand the
correct `release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"1f3c9cb9bb39699ca8dc0a83d6da59914a654977"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/207901","number":207901,"mergeCommit":{"message":"[Controls]
Fix load more request size (#207901)\n\nCloses
https://github.com/elastic/kibana/issues/207884\r\n\r\n##
Summary\r\n\r\nThis PR ensures that the options list control only
fetches a maximum of\r\n**1,000** terms on scroll rather than the whole
cardinality of the given\r\nfield. This prevents the user from hitting
an error when the cardinality\r\nof their field is greater than their
`search.max_buckets` setting\r\n(which, by default, is set to
`65,536`)\r\n\r\n**Before**\r\n\r\n\r\nhttps://github.com/user-attachments/assets/0b60818a-497e-4f1f-b2ed-788c7e7371e3\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/user-attachments/assets/2e52b45b-9e35-4e7e-920c-c0b4fe3aa728\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] The PR
description includes the appropriate Release Notes section,\r\nand the
correct `release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"1f3c9cb9bb39699ca8dc0a83d6da59914a654977"}}]}]
BACKPORT-->

Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
  • Loading branch information
kibanamachine and Heenawter authored Jan 29, 2025
1 parent b65acc7 commit ce405bc
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { BehaviorSubject } from 'rxjs';
import { BehaviorSubject, Subject } from 'rxjs';

import { DataViewField } from '@kbn/data-views-plugin/common';

Expand All @@ -19,6 +19,7 @@ import type {
OptionsListSortingType,
OptionsListSuggestions,
} from '../../../../common/options_list';
import { MIN_OPTIONS_LIST_REQUEST_SIZE } from '../options_list_control/constants';

export const getOptionsListMocks = () => {
const selectedOptions$ = new BehaviorSubject<OptionsListSelection[] | undefined>(undefined);
Expand All @@ -30,14 +31,15 @@ export const getOptionsListMocks = () => {
field$: new BehaviorSubject<DataViewField | undefined>({ type: 'string' } as DataViewField),
availableOptions$: new BehaviorSubject<OptionsListSuggestions | undefined>(undefined),
invalidSelections$: new BehaviorSubject<Set<OptionsListSelection>>(new Set([])),
totalCardinality$: new BehaviorSubject<number | undefined>(undefined),
totalCardinality$: new BehaviorSubject<number>(0),
dataLoading$: new BehaviorSubject<boolean>(false),
parentApi: {
allowExpensiveQueries$: new BehaviorSubject<boolean>(true),
},
fieldFormatter: new BehaviorSubject((value: string | number) => String(value)),
makeSelection: jest.fn(),
setExclude: (next: boolean | undefined) => exclude$.next(next),
loadMoreSubject: new Subject<void>(),
},
stateManager: {
searchString: new BehaviorSubject<string>(''),
Expand All @@ -48,6 +50,7 @@ export const getOptionsListMocks = () => {
sort: new BehaviorSubject<OptionsListSortingType | undefined>(undefined),
selectedOptions: selectedOptions$ as PublishingSubject<OptionsListSelection[] | undefined>,
searchTechnique: new BehaviorSubject<OptionsListSearchTechnique | undefined>(undefined),
requestSize: new BehaviorSubject<number>(MIN_OPTIONS_LIST_REQUEST_SIZE),
},
displaySettings: {} as OptionsListDisplaySettings,
// setSelectedOptions and setExistsSelected are not exposed via API because
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import React from 'react';

import { fireEvent, render, waitFor } from '@testing-library/react';

import { take } from 'lodash';
import { getOptionsListMocks } from '../../mocks/api_mocks';
import { MAX_OPTIONS_LIST_REQUEST_SIZE, MIN_OPTIONS_LIST_REQUEST_SIZE } from '../constants';
import { ContextStateManager, OptionsListControlContext } from '../options_list_context_provider';
import type { OptionsListComponentApi } from '../types';
import { OptionsListPopoverSuggestions } from './options_list_popover_suggestions';

describe('Options list popover', () => {
const allOptions = [
{ value: 'moo', docCount: 1 },
{ value: 'tweet', docCount: 2 },
{ value: 'oink', docCount: 3 },
{ value: 'bark', docCount: 4 },
{ value: 'meow', docCount: 5 },
{ value: 'woof', docCount: 6 },
{ value: 'roar', docCount: 7 },
{ value: 'honk', docCount: 8 },
];

const mountComponent = ({
api,
displaySettings,
stateManager,
showOnlySelected,
}: {
api: any;
displaySettings: any;
stateManager: any;
showOnlySelected?: boolean;
}) => {
return render(
<OptionsListControlContext.Provider
value={{
api: api as unknown as OptionsListComponentApi,
displaySettings,
stateManager: stateManager as unknown as ContextStateManager,
}}
>
<OptionsListPopoverSuggestions showOnlySelected={showOnlySelected ?? false} />
</OptionsListControlContext.Provider>
);
};

test('displays "load more" text when possible', async () => {
const mocks = getOptionsListMocks();
mocks.api.totalCardinality$.next(allOptions.length);
mocks.api.availableOptions$.next(take(allOptions, 5));
const suggestionsComponent = mountComponent(mocks);

// the cardinality is larger than the available options, so display text
let optionComponents = await suggestionsComponent.findAllByRole('option');
expect(optionComponents.length).toBe(6);
expect(
suggestionsComponent.queryByTestId('optionsList-control-selection-honk')
).not.toBeInTheDocument();
expect(suggestionsComponent.queryByTestId('optionslist--canLoadMore')).toBeInTheDocument();

// we are displaying all the available options - so don't display "load more" text
mocks.api.availableOptions$.next(allOptions);
await waitFor(async () => {
optionComponents = await suggestionsComponent.findAllByRole('option');
expect(optionComponents.length).toBe(9);
});
expect(
suggestionsComponent.queryByTestId('optionsList-control-selection-honk')
).toBeInTheDocument();
expect(suggestionsComponent.queryByTestId('optionslist--canLoadMore')).not.toBeInTheDocument();
});

test('only fetch up to maximum request size on scroll', async () => {
const mocks = getOptionsListMocks();
mocks.api.totalCardinality$.next(100);
mocks.api.availableOptions$.next(take(allOptions, 5));
const suggestionsComponent = mountComponent(mocks);

// ensure we fetch the cardinality on scroll
expect(mocks.stateManager.requestSize.getValue()).toBe(MIN_OPTIONS_LIST_REQUEST_SIZE);
fireEvent.scroll(suggestionsComponent.getByTestId('optionsList--scrollListener'));
expect(mocks.stateManager.requestSize.getValue()).toBe(100);

// reset request size + update cardinality
mocks.stateManager.requestSize.next(MIN_OPTIONS_LIST_REQUEST_SIZE);
mocks.api.totalCardinality$.next(MAX_OPTIONS_LIST_REQUEST_SIZE + 100);
await waitFor(async () => {
// wait for request size to be reset in UI
const optionComponents = await suggestionsComponent.findAllByRole('option');
expect(optionComponents.length).toBe(6);
});

// ensure we don't fetch more than MAX_OPTIONS_LIST_REQUEST_SIZE
fireEvent.scroll(suggestionsComponent.getByTestId('optionsList--scrollListener'));
expect(mocks.stateManager.requestSize.getValue()).toBe(MAX_OPTIONS_LIST_REQUEST_SIZE);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ export const OptionsListPopoverSuggestions = ({
if (canLoadMoreSuggestions) {
options.push({
key: 'loading-option',
'data-test-subj': 'optionslist--canLoadMore',
className: 'optionslist--loadingMoreGroupLabel',
label: OptionsListStrings.popover.getLoadingMoreMessage(),
isGroupLabel: true,
Expand Down Expand Up @@ -150,8 +151,8 @@ export const OptionsListPopoverSuggestions = ({
if (scrollTop + clientHeight >= scrollHeight - parseInt(euiThemeVars.euiSizeXXL, 10)) {
// reached the "bottom" of the list, where euiSizeXXL acts as a "margin of error" so that the user doesn't
// have to scroll **all the way** to the bottom in order to load more options
stateManager.requestSize.next(totalCardinality ?? MAX_OPTIONS_LIST_REQUEST_SIZE);
api.loadMoreSubject.next(null); // trigger refetch with loadMoreSubject
stateManager.requestSize.next(Math.min(totalCardinality, MAX_OPTIONS_LIST_REQUEST_SIZE));
api.loadMoreSubject.next(); // trigger refetch with loadMoreSubject
}
}, [api.loadMoreSubject, stateManager.requestSize, totalCardinality]);

Expand Down Expand Up @@ -186,7 +187,7 @@ export const OptionsListPopoverSuggestions = ({

return (
<>
<div ref={listRef}>
<div data-test-subj="optionsList--scrollListener" ref={listRef}>
<EuiSelectable
options={selectableOptions}
renderOption={(option) => renderOption(option, searchString)}
Expand All @@ -196,7 +197,7 @@ export const OptionsListPopoverSuggestions = ({
selectableOptions.length
)}
emptyMessage={<OptionsListPopoverEmptyMessage showOnlySelected={showOnlySelected} />}
onChange={(newSuggestions, _, changedOption) => {
onChange={(newSuggestions, event, changedOption) => {
api.makeSelection(changedOption.key, showOnlySelected);
}}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ export function fetchAndValidate$({
stateManager.sort,
stateManager.searchTechnique,
// cannot use requestSize directly, because we need to be able to reset the size to the default without refetching
api.loadMoreSubject.pipe(debounceTime(100)), // debounce load more so "loading" state briefly shows
api.loadMoreSubject.pipe(
startWith(null), // start with null so that `combineLatest` subscription fires
debounceTime(100) // debounce load more so "loading" state briefly shows
),
apiPublishesReload(api.parentApi)
? api.parentApi.reload$.pipe(
tap(() => requestCache.clearCache()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
filter,
map,
skip,
Subject,
} from 'rxjs';

import { buildExistsFilter, buildPhraseFilter, buildPhrasesFilter, Filter } from '@kbn/es-query';
Expand Down Expand Up @@ -172,7 +173,7 @@ export const getOptionsListControlFactory = (): DataControlFactory<
});

/** Fetch the suggestions and perform validation */
const loadMoreSubject = new BehaviorSubject<null>(null);
const loadMoreSubject = new Subject<void>();
const fetchSubscription = fetchAndValidate$({
api: {
...dataControl.api,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { BehaviorSubject } from 'rxjs';
import { Subject } from 'rxjs';

import type { PublishingSubject } from '@kbn/presentation-publishing';
import type {
Expand Down Expand Up @@ -37,6 +37,6 @@ export type OptionsListComponentApi = OptionsListControlApi &
PublishesOptions & {
deselectOption: (key: string | undefined) => void;
makeSelection: (key: string | undefined, showOnlySelected: boolean) => void;
loadMoreSubject: BehaviorSubject<null>;
loadMoreSubject: Subject<void>;
setExclude: (next: boolean | undefined) => void;
};

0 comments on commit ce405bc

Please sign in to comment.