Skip to content

Commit

Permalink
[Unified field list] debounce search (elastic#187143)
Browse files Browse the repository at this point in the history
## Summary

Updates to unified field list on typing are debounced - this way we
don't get so many updates when typing in the search input.

Flaky test runner:
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6424

## Performance comparison
Test: typing the string: activem for metricbeat data (~6000 fields)

before (costly update on every keystroke):
<img width="669" alt="Screenshot 2024-06-28 at 17 28 38"
src="https://github.com/elastic/kibana/assets/4283304/7075f7bc-2d90-4177-acac-69ac101b2ef1">

after (only one costly update when user stops typing):
<img width="269" alt="Screenshot 2024-06-28 at 17 24 43"
src="https://github.com/elastic/kibana/assets/4283304/8c0ce4a3-7c1a-428b-a482-f6b4d87911e0">
  • Loading branch information
mbondyra authored Jul 6, 2024
1 parent 4504088 commit e6f17e7
Show file tree
Hide file tree
Showing 38 changed files with 163 additions and 79 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
* Side Public License, v 1.
*/

import React from 'react';
import { mountWithIntl } from '@kbn/test-jest-helpers';
import { act } from 'react-dom/test-utils';
import React, { useState } from 'react';
import userEvent from '@testing-library/user-event';
import { FieldNameSearch, type FieldNameSearchProps } from './field_name_search';
import { render, screen, waitFor } from '@testing-library/react';

describe('UnifiedFieldList <FieldNameSearch />', () => {
it('should render correctly', async () => {
Expand All @@ -19,35 +19,34 @@ describe('UnifiedFieldList <FieldNameSearch />', () => {
screenReaderDescriptionId: 'htmlId',
'data-test-subj': 'searchInput',
};
const wrapper = mountWithIntl(<FieldNameSearch {...props} />);
expect(wrapper.find('input').prop('aria-describedby')).toBe('htmlId');

act(() => {
wrapper.find('input').simulate('change', {
target: { value: 'hi' },
});
});

expect(props.onChange).toBeCalledWith('hi');
render(<FieldNameSearch {...props} />);
const input = screen.getByRole('searchbox', { name: 'Search field names' });
expect(input).toHaveAttribute('aria-describedby', 'htmlId');
userEvent.type(input, 'hey');
await waitFor(() => expect(props.onChange).toHaveBeenCalledWith('hey'), { timeout: 256 });
expect(props.onChange).toBeCalledTimes(1);
});

it('should update correctly', async () => {
const props: FieldNameSearchProps = {
nameFilter: 'this',
onChange: jest.fn(),
screenReaderDescriptionId: 'htmlId',
'data-test-subj': 'searchInput',
it('should accept the updates from the top', async () => {
const FieldNameSearchWithWrapper = ({ defaultNameFilter = '' }) => {
const [nameFilter, setNameFilter] = useState(defaultNameFilter);
const props: FieldNameSearchProps = {
nameFilter,
onChange: jest.fn(),
screenReaderDescriptionId: 'htmlId',
'data-test-subj': 'searchInput',
};
return (
<div>
<button onClick={() => setNameFilter('that')}>update nameFilter</button>
<FieldNameSearch {...props} />
</div>
);
};
const wrapper = mountWithIntl(<FieldNameSearch {...props} />);

expect(wrapper.find('input').prop('value')).toBe('this');

wrapper.setProps({
nameFilter: 'that',
});

expect(wrapper.find('input').prop('value')).toBe('that');

expect(props.onChange).not.toBeCalled();
render(<FieldNameSearchWithWrapper defaultNameFilter="this" />);
expect(screen.getByRole('searchbox')).toHaveValue('this');
const button = screen.getByRole('button', { name: 'update nameFilter' });
userEvent.click(button);
expect(screen.getByRole('searchbox')).toHaveValue('that');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import React from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFieldSearch, type EuiFieldSearchProps } from '@elastic/eui';
import { useDebouncedValue } from '@kbn/visualization-utils';

/**
* Props for FieldNameSearch component
Expand Down Expand Up @@ -45,15 +46,22 @@ export const FieldNameSearch: React.FC<FieldNameSearchProps> = ({
description: 'Search the list of fields in the data view for the provided text',
});

const { inputValue, handleInputChange } = useDebouncedValue({
onChange,
value: nameFilter,
});

return (
<EuiFieldSearch
aria-describedby={screenReaderDescriptionId}
aria-label={searchPlaceholder}
data-test-subj={`${dataTestSubject}FieldSearch`}
fullWidth
onChange={(event) => onChange(event.target.value)}
onChange={(e) => {
handleInputChange(e.target.value);
}}
placeholder={searchPlaceholder}
value={nameFilter}
value={inputValue}
append={append}
compressed={compressed}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ import { FieldsAccordion } from './fields_accordion';
import { NoFieldsCallout } from './no_fields_callout';
import { useGroupedFields, type GroupedFieldsParams } from '../../hooks/use_grouped_fields';

jest.mock('lodash', () => {
const original = jest.requireActual('lodash');

return {
...original,
debounce: (fn: unknown) => fn,
};
});

describe('UnifiedFieldList FieldListGrouped + useGroupedFields()', () => {
let defaultProps: FieldListGroupedProps<DataViewField>;
let mockedServices: GroupedFieldsParams<DataViewField>['services'];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import React from 'react';
import { EuiFieldText, EuiFieldTextProps } from '@elastic/eui';
import { useDebouncedValue } from './debounced_value';
import { useDebouncedValue } from '@kbn/visualization-utils';

type Props = {
value: string;
Expand Down
2 changes: 0 additions & 2 deletions packages/kbn-visualization-ui-components/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ export * from './name_input';

export * from './debounced_input';

export * from './debounced_value';

export * from './color_picker';

export * from './icon_select';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type { DataPublicPluginStart } from '@kbn/data-plugin/public';
import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser';
import type { NotificationsStart } from '@kbn/core-notifications-browser';
import type { DocLinksStart } from '@kbn/core-doc-links-browser';
import { useDebouncedValue } from '../debounced_value';
import { useDebouncedValue } from '@kbn/visualization-utils';

export interface QueryInputServices {
http: HttpStart;
Expand Down
1 change: 0 additions & 1 deletion packages/kbn-visualization-ui-components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export {
FieldPicker,
NameInput,
DebouncedInput,
useDebouncedValue,
ColorPicker,
IconSelect,
IconSelectSetting,
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-visualization-utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@
export { getTimeZone } from './src/get_timezone';
export { getLensAttributesFromSuggestion } from './src/get_lens_attributes';
export { TooltipWrapper } from './src/tooltip_wrapper';
export { useDebouncedValue } from './src/debounced_value';
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,24 @@
import { act, renderHook } from '@testing-library/react-hooks';
import { useDebouncedValue } from './debounced_value';

jest.mock('lodash', () => {
const original = jest.requireActual('lodash');
describe('useDebouncedValue', () => {
beforeAll(() => {
jest.useFakeTimers();
});

return {
...original,
debounce: (fn: unknown) => fn,
};
});
afterAll(() => {
jest.useRealTimers();
});

describe('useDebouncedValue', () => {
it('should update upstream value changes', () => {
const onChangeMock = jest.fn();
const { result } = renderHook(() => useDebouncedValue({ value: 'a', onChange: onChangeMock }));

act(() => {
result.current.handleInputChange('b');
});
expect(onChangeMock).not.toHaveBeenCalled();
jest.advanceTimersByTime(256);

expect(onChangeMock).toHaveBeenCalledWith('b');
});
Expand All @@ -37,7 +38,8 @@ describe('useDebouncedValue', () => {
act(() => {
result.current.handleInputChange('');
});

expect(onChangeMock).not.toHaveBeenCalled();
jest.advanceTimersByTime(256);
expect(onChangeMock).toHaveBeenCalledWith('a');
});

Expand All @@ -50,7 +52,23 @@ describe('useDebouncedValue', () => {
act(() => {
result.current.handleInputChange('');
});

expect(onChangeMock).not.toHaveBeenCalled();
jest.advanceTimersByTime(256);
expect(onChangeMock).toHaveBeenCalledWith('');
});
it('custom wait time is respected', () => {
const onChangeMock = jest.fn();
const { result } = renderHook(() =>
useDebouncedValue({ value: 'a', onChange: onChangeMock }, { wait: 500 })
);

act(() => {
result.current.handleInputChange('b');
});
expect(onChangeMock).not.toHaveBeenCalled();
jest.advanceTimersByTime(256);
expect(onChangeMock).not.toHaveBeenCalled();
jest.advanceTimersByTime(244); // sums to 500
expect(onChangeMock).toHaveBeenCalledWith('b');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@
import { useState, useMemo, useEffect, useRef } from 'react';
import { debounce } from 'lodash';

const DEFAULT_TIMEOUT = 256;
/**
* Debounces value changes and updates inputValue on root state changes if no debounced changes
* are in flight because the user is currently modifying the value.
*
* * allowFalsyValue: update upstream with all falsy values but null or undefined
*
* When testing this function mock the "debounce" function in lodash (see this module test for an example)
* * wait: debounce timeout
*/

export const useDebouncedValue = <T>(
Expand All @@ -28,7 +28,9 @@ export const useDebouncedValue = <T>(
value: T;
defaultValue?: T;
},
{ allowFalsyValue }: { allowFalsyValue?: boolean } = {}
{ allowFalsyValue, wait = DEFAULT_TIMEOUT }: { allowFalsyValue?: boolean; wait?: number } = {
wait: DEFAULT_TIMEOUT,
}
) => {
const [inputValue, setInputValue] = useState(value);
const unflushedChanges = useRef(false);
Expand All @@ -45,16 +47,16 @@ export const useDebouncedValue = <T>(
// do not reset unflushed flag right away, wait a bit for upstream to pick it up
flushChangesTimeout.current = setTimeout(() => {
unflushedChanges.current = false;
}, 256);
}, 256);
}, wait);
}, wait);
return (val: T) => {
if (flushChangesTimeout.current) {
clearTimeout(flushChangesTimeout.current);
}
unflushedChanges.current = true;
callback(val);
};
}, [onChange]);
}, [onChange, wait]);

useEffect(() => {
if (!unflushedChanges.current && value !== inputValue) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,15 @@ jest.mock('../../../../customizations', () => ({
}),
}));

jest.mock('lodash', () => {
const original = jest.requireActual('lodash');

return {
...original,
debounce: (fn: unknown) => fn,
};
});

jest.mock('@kbn/unified-field-list/src/services/field_stats', () => ({
loadFieldStats: jest.fn().mockResolvedValue({
totalDocuments: 1624,
Expand Down
14 changes: 10 additions & 4 deletions test/functional/apps/discover/classic/_doc_table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

describe('add and remove columns', async function () {
const extraColumns = ['phpmemory', 'ip'];

const expectedFieldLength: Record<string, number> = {
phpmemory: 1,
ip: 4,
};
afterEach(async function () {
for (const column of extraColumns) {
await PageObjects.unifiedFieldList.clickFieldListItemRemove(column);
Expand All @@ -242,6 +245,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
for (const column of extraColumns) {
await PageObjects.unifiedFieldList.clearFieldSearchInput();
await PageObjects.unifiedFieldList.findFieldByName(column);
await PageObjects.unifiedFieldList.waitUntilFieldlistHasCountOfFields(
expectedFieldLength[column]
);
await retry.waitFor('field to appear', async function () {
return await testSubjects.exists(`field-${column}`);
});
Expand All @@ -258,9 +264,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
for (const column of extraColumns) {
await PageObjects.unifiedFieldList.clearFieldSearchInput();
await PageObjects.unifiedFieldList.findFieldByName(column);
await retry.waitFor('field to appear', async function () {
return await testSubjects.exists(`field-${column}`);
});
await PageObjects.unifiedFieldList.waitUntilFieldlistHasCountOfFields(
expectedFieldLength[column]
);
await PageObjects.unifiedFieldList.clickFieldListItemAdd(column);
await PageObjects.header.waitUntilLoadingHasFinished();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

describe('add and remove columns', function () {
const extraColumns = ['phpmemory', 'ip'];
const expectedFieldLength: Record<string, number> = {
phpmemory: 1,
ip: 4,
};

afterEach(async function () {
for (const column of extraColumns) {
Expand All @@ -232,6 +236,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
for (const column of extraColumns) {
await PageObjects.unifiedFieldList.clearFieldSearchInput();
await PageObjects.unifiedFieldList.findFieldByName(column);
await PageObjects.unifiedFieldList.waitUntilFieldlistHasCountOfFields(
expectedFieldLength[column]
);
await PageObjects.unifiedFieldList.clickFieldListItemAdd(column);
await PageObjects.header.waitUntilLoadingHasFinished();
// test the header now
Expand All @@ -244,6 +251,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
for (const column of extraColumns) {
await PageObjects.unifiedFieldList.clearFieldSearchInput();
await PageObjects.unifiedFieldList.findFieldByName(column);
await PageObjects.unifiedFieldList.waitUntilFieldlistHasCountOfFields(
expectedFieldLength[column]
);
await PageObjects.unifiedFieldList.clickFieldListItemAdd(column);
await PageObjects.header.waitUntilLoadingHasFinished();
}
Expand Down
9 changes: 9 additions & 0 deletions test/functional/page_objects/unified_field_list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ export class UnifiedFieldListPageObject extends FtrService {
});
}

public async waitUntilFieldlistHasCountOfFields(count: number) {
await this.retry.waitFor('wait until fieldlist has updated number of fields', async () => {
return (
(await this.find.allByCssSelector('#fieldListGroupedAvailableFields .kbnFieldButton'))
.length === count
);
});
}

public async doesSidebarShowFields() {
return await this.testSubjects.exists('fieldListGroupedFieldGroups');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
EuiLink,
useEuiTheme,
} from '@elastic/eui';
import { useDebouncedValue } from '@kbn/visualization-ui-components';
import { useDebouncedValue } from '@kbn/visualization-utils';
import { useKibana } from '@kbn/kibana-react-plugin/public';
import {
DEFAULT_DURATION_INPUT_FORMAT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
ExpressionAstExpressionBuilder,
ExpressionAstFunctionBuilder,
} from '@kbn/expressions-plugin/public';
import { useDebouncedValue } from '@kbn/visualization-ui-components';
import { useDebouncedValue } from '@kbn/visualization-utils';
import { PERCENTILE_ID, PERCENTILE_NAME } from '@kbn/lens-formula-docs';
import { OperationDefinition } from '.';
import {
Expand Down
Loading

0 comments on commit e6f17e7

Please sign in to comment.