[Lens] fix annotation controls bug (#174951)

## Summary

Fix https://github.com/elastic/kibana/issues/174653

The bug that was causing the failures


058a99cd-b7a6-4e3b-9f99-208b2712136a



I fixed it by divorcing the state of the annotation controls from the
redux store. Redux is still notified with changes to the annotation
group, but the UI no longer waits on the global state. That makes things
faster and resolved this state management weirdness.

I feel that we should consider a similar approach with the rest of Lens
(see https://github.com/elastic/eui/issues/6147#issuecomment-1531870102
for more discussion).

Passing flaky test runner
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4895

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

packages/kbn-event-annotation-components/components/annotation_editor_controls/annotation_editor_controls.tsx

@@ -79,9 +79,11 @@ const AnnotationEditorControls = ({
   }, [isQueryBased]);
 
   const update = useCallback(
-    <T extends EventAnnotationConfig>(newAnnotation: Partial<T> | undefined) =>
-      newAnnotation &&
-      onAnnotationChange(sanitizeProperties({ ...currentAnnotation, ...newAnnotation })),
+    <T extends EventAnnotationConfig>(newAnnotation: Partial<T> | undefined) => {
+      if (!newAnnotation) return;
+
+      onAnnotationChange(sanitizeProperties({ ...currentAnnotation, ...newAnnotation }));
+    },
     [currentAnnotation, onAnnotationChange]
   );
 

packages/kbn-event-annotation-components/components/annotation_editor_controls/tooltip_annotation_panel.tsx

@@ -6,16 +6,16 @@
  * Side Public License, v 1.
  */
 
-import { htmlIdGenerator, EuiFlexItem, EuiPanel, EuiText } from '@elastic/eui';
+import { EuiFlexItem, EuiPanel, EuiText, htmlIdGenerator } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
-import React, { useCallback, useMemo } from 'react';
+import React, { useCallback, useState } from 'react';
+import fastIsEqual from 'fast-deep-equal';
 import { getFieldIconType } from '@kbn/field-utils';
 import { useExistingFieldsReader } from '@kbn/unified-field-list';
 import {
   FieldOption,
   FieldOptionValue,
   FieldPicker,
-  useDebouncedValue,
   NewBucketButton,
   DragDropBuckets,
   DraggableBucketContainer,
@@ -25,9 +25,8 @@ import {
 import { DataView } from '@kbn/data-views-plugin/common';
 import type { QueryPointEventAnnotationConfig } from '@kbn/event-annotation-common';
 
-export const MAX_TOOLTIP_FIELDS_SIZE = 2;
+export const MAX_TOOLTIP_FIELDS_SIZE = 3;
 
-const generateId = htmlIdGenerator();
 const supportedTypes = new Set(['string', 'boolean', 'number', 'ip', 'date']);
 
 export interface FieldInputsProps {
@@ -37,76 +36,60 @@ export interface FieldInputsProps {
   invalidFields?: string[];
 }
 
-interface WrappedValue {
+function removeNewEmptyField(v: string) {
+  return v !== '';
+}
+
+const generateId = htmlIdGenerator();
+
+interface LocalFieldEntry {
+  name: string;
   id: string;
-  value: string | undefined;
-  isNew?: boolean;
 }
 
-type SafeWrappedValue = Omit<WrappedValue, 'value'> & { value: string };
-
-function removeNewEmptyField(v: WrappedValue): v is SafeWrappedValue {
-  return v.value != null;
-}
-
-export function TooltipSection({
-  currentConfig,
-  setConfig,
-  dataView,
-  invalidFields,
-}: FieldInputsProps) {
+export function TooltipSection({ currentConfig, setConfig, dataView }: FieldInputsProps) {
   const { hasFieldData } = useExistingFieldsReader();
-  const onChangeWrapped = useCallback(
-    (values: WrappedValue[]) => {
+
+  // This is a local list that governs the state of UI, including empty fields which
+  // are never reported to the parent component.
+  const [currentFields, _setFields] = useState<LocalFieldEntry[]>(
+    (currentConfig.extraFields ?? []).map((field) => ({ name: field, id: generateId() }))
+  );
+
+  const setFields = useCallback(
+    (fields: LocalFieldEntry[]) => {
+      _setFields(fields);
+
+      let newExtraFields: QueryPointEventAnnotationConfig['extraFields'] = fields
+        .map(({ name }) => name)
+        .filter(removeNewEmptyField);
+      newExtraFields = newExtraFields.length ? newExtraFields : undefined;
+
+      if (!fastIsEqual(newExtraFields, currentConfig.extraFields)) {
         setConfig({
           ...currentConfig,
-        extraFields: values.filter(removeNewEmptyField).map(({ value }) => value),
+          extraFields: newExtraFields,
         });
+      }
     },
     [setConfig, currentConfig]
   );
-  const { wrappedValues, rawValuesLookup } = useMemo(() => {
-    const rawValues = currentConfig.extraFields ?? [];
-    return {
-      wrappedValues: rawValues.map((value) => ({ id: generateId(), value })),
-      rawValuesLookup: new Set(rawValues),
-    };
-  }, [currentConfig]);
 
-  const { inputValue: localValues, handleInputChange } = useDebouncedValue<WrappedValue[]>({
-    onChange: onChangeWrapped,
-    value: wrappedValues,
-  });
-
-  const onFieldSelectChange = useCallback(
-    (choice, index = 0) => {
-      const fields = [...localValues];
-
-      if (dataView.getFieldByName(choice.field)) {
-        fields[index] = { id: generateId(), value: choice.field };
-
-        // update the layer state
-        handleInputChange(fields);
-      }
-    },
-    [localValues, dataView, handleInputChange]
-  );
-
-  const newBucketButton = (
+  const addFieldButton = (
     <NewBucketButton
       className="lnsConfigPanelAnnotations__addButton"
       data-test-subj={`lnsXY-annotation-tooltip-add_field`}
       onClick={() => {
-        handleInputChange([...localValues, { id: generateId(), value: undefined, isNew: true }]);
+        setFields([...currentFields, { name: '', id: generateId() }]);
       }}
       label={i18n.translate('eventAnnotationComponents.xyChart.annotation.tooltip.addField', {
         defaultMessage: 'Add field',
       })}
-      isDisabled={localValues.length > MAX_TOOLTIP_FIELDS_SIZE}
+      isDisabled={currentFields.length >= MAX_TOOLTIP_FIELDS_SIZE}
     />
   );
 
-  if (localValues.length === 0) {
+  if (currentFields.length === 0) {
     return (
       <>
         <EuiFlexItem grow={true}>
@@ -122,7 +105,7 @@ export function TooltipSection({
             </EuiText>
           </EuiPanel>
         </EuiFlexItem>
-        {newBucketButton}
+        {addFieldButton}
       </>
     );
   }
@@ -131,7 +114,7 @@ export function TooltipSection({
     .filter(isFieldLensCompatible)
     .filter(
       ({ displayName, type }) =>
-        displayName && !rawValuesLookup.has(displayName) && supportedTypes.has(type)
+        displayName && !currentConfig.extraFields?.includes(displayName) && supportedTypes.has(type)
     )
     .map(
       (field) =>
@@ -152,23 +135,24 @@ export function TooltipSection({
   return (
     <>
       <DragDropBuckets
-        onDragEnd={(updatedValues: WrappedValue[]) => {
-          handleInputChange(updatedValues);
+        onDragEnd={(updatedFields: LocalFieldEntry[]) => {
+          setFields(updatedFields);
         }}
         droppableId="ANNOTATION_TOOLTIP_DROPPABLE_AREA"
-        items={localValues}
+        items={currentFields}
         bgColor="subdued"
       >
-        {localValues.map(({ id, value, isNew }, index, arrayRef) => {
-          const fieldIsValid = value ? Boolean(dataView.getFieldByName(value)) : true;
+        {currentFields.map((field, index, arrayRef) => {
+          const fieldIsValid =
+            field.name === '' ? true : Boolean(dataView.getFieldByName(field.name));
 
           return (
             <DraggableBucketContainer
-              id={(value ?? 'newField') + id}
-              key={(value ?? 'newField') + id}
+              id={field.id}
+              key={field.id}
               idx={index}
               onRemoveClick={() => {
-                handleInputChange(arrayRef.filter((_, i) => i !== index));
+                setFields(arrayRef.filter((_, i) => i !== index));
               }}
               removeTitle={i18n.translate(
                 'eventAnnotationComponents.xyChart.annotation.tooltip.deleteButtonLabel',
@@ -184,29 +168,37 @@ export function TooltipSection({
               <FieldPicker
                 compressed
                 selectedOptions={
-                  value
+                  field.name
                     ? [
                         {
-                          label: value,
-                          value: { type: 'field', field: value },
+                          label: field.name,
+                          value: { type: 'field', field: field.name },
                         },
                       ]
                     : []
                 }
                 options={options}
                 onChoose={(choice) => {
-                  onFieldSelectChange(choice, index);
+                  if (!choice) {
+                    return;
+                  }
+
+                  if (dataView.getFieldByName(choice.field)) {
+                    const newFields = [...currentFields];
+                    newFields[index] = { name: choice.field, id: generateId() };
+                    setFields(newFields);
+                  }
                 }}
                 fieldIsInvalid={!fieldIsValid}
                 className="lnsConfigPanelAnnotations__fieldPicker"
                 data-test-subj={`lnsXY-annotation-tooltip-field-picker--${index}`}
-                autoFocus={isNew && value == null}
+                autoFocus={field.name === ''}
               />
             </DraggableBucketContainer>
           );
         })}
       </DragDropBuckets>
-      {newBucketButton}
+      {addFieldButton}
     </>
   );
 }

packages/kbn-visualization-ui-components/components/query_input/filter_query_input.tsx

@@ -19,7 +19,6 @@ import {
   EuiPopoverProps,
 } from '@elastic/eui';
 import type { DataViewBase, Query } from '@kbn/es-query';
-import { useDebouncedValue } from '../debounced_value';
 import { QueryInput, validateQuery } from '.';
 import type { QueryInputServices } from '.';
 import './filter_query_input.scss';
@@ -56,10 +55,6 @@ export function FilterQueryInput({
   appName: string;
 }) {
   const [filterPopoverOpen, setFilterPopoverOpen] = useState(Boolean(initiallyOpen));
-  const { inputValue: queryInput, handleInputChange: setQueryInput } = useDebouncedValue<Query>({
-    value: inputFilter ?? defaultFilter,
-    onChange,
-  });
 
   const onClosePopup: EuiPopoverProps['closePopover'] = useCallback(() => {
     setFilterPopoverOpen(false);
@@ -67,7 +62,7 @@ export function FilterQueryInput({
 
   const { isValid: isInputFilterValid } = validateQuery(inputFilter, dataView);
   const { isValid: isQueryInputValid, error: queryInputError } = validateQuery(
-    queryInput,
+    inputFilter,
     dataView
   );
 
@@ -150,8 +145,8 @@ export function FilterQueryInput({
                     : { type: 'title', value: dataView.title }
                 }
                 disableAutoFocus={true}
-                value={queryInput}
-                onChange={setQueryInput}
+                value={inputFilter ?? defaultFilter}
+                onChange={onChange}
                 isInvalid={!isQueryInputValid}
                 onSubmit={() => {}}
                 data-test-subj={dataTestSubj}

x-pack/plugins/lens/public/visualizations/xy/xy_config_panel/annotations_config_panel/annotations_panel.tsx

@@ -10,7 +10,6 @@ import type { DatatableUtilitiesService } from '@kbn/data-plugin/common';
 import { AnnotationEditorControls } from '@kbn/event-annotation-components';
 import type { EventAnnotationConfig } from '@kbn/event-annotation-common';
 import { useKibana } from '@kbn/kibana-react-plugin/public';
-import { useDebouncedValue } from '@kbn/visualization-ui-components';
 import { DataViewsPublicPluginStart, DataView } from '@kbn/data-views-plugin/public';
 import moment from 'moment';
 import { search } from '@kbn/data-plugin/public';
@@ -30,10 +29,9 @@ export const AnnotationsPanel = (
 ) => {
   const { state, setState, layerId, accessor, frame } = props;
 
-  const { inputValue: localState, handleInputChange: setLocalState } = useDebouncedValue<XYState>({
-    value: state,
-    onChange: setState,
-  });
+  // we don't listen to the state prop after the initial render, because we don't want to
+  // slow the annotation settings UI updates down on a full Redux state update
+  const [localState, setLocalState] = useState<XYState>(state);
 
   const index = localState.layers.findIndex((l) => l.layerId === layerId);
   const localLayer = localState.layers.find(
@@ -56,9 +54,13 @@ export const AnnotationsPanel = (
           'should never happen because annotation is created before config panel is opened'
         );
       }
-      setLocalState(updateLayer(localState, { ...localLayer, annotations: newConfigs }, index));
+
+      const newState = updateLayer(localState, { ...localLayer, annotations: newConfigs }, index);
+
+      setLocalState(newState); // keep track up updates for controls state
+      setState(newState); // notify the rest of the world, but don't wait
     },
-    [accessor, index, localState, localLayer, setLocalState]
+    [localLayer, localState, index, setState, accessor]
   );
 
   const [currentDataView, setCurrentDataView] = useState<DataView>();

x-pack/test/functional/apps/lens/group6/annotations.ts

@@ -26,8 +26,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
   const from = 'Sep 19, 2015 @ 06:31:44.000';
   const to = 'Sep 23, 2015 @ 18:31:44.000';
 
-  // FLAKY: https://github.com/elastic/kibana/issues/174653
-  describe.skip('lens annotations tests', () => {
+  describe('lens annotations tests', () => {
     before(async () => {
       await PageObjects.common.setTime({ from, to });
     });