[Lens] fix annotation controls bug (#174951)

## Summary Fix #174653 The bug that was causing the failures https://github.com/elastic/kibana/assets/315764/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 elastic/eui#6147 (comment) for more discussion). Passing flaky test runner https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4895 --------- Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Stratoula Kalafateli <[email protected]>
elastic · Jan 26, 2024 · 2331a4c · 2331a4c
1 parent 23135f5
commit 2331a4c
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 89 deletions.
diff --git a/...nnotation-components/components/annotation_editor_controls/annotation_editor_controls.tsx b/...nnotation-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]
   );
 

diff --git a/...-annotation-components/components/annotation_editor_controls/tooltip_annotation_panel.tsx b/...-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 {
-  id: string;
-  value: string | undefined;
-  isNew?: boolean;
+function removeNewEmptyField(v: string) {
+  return v !== '';
 }
 
-type SafeWrappedValue = Omit<WrappedValue, 'value'> & { value: string };
+const generateId = htmlIdGenerator();
 
-function removeNewEmptyField(v: WrappedValue): v is SafeWrappedValue {
-  return v.value != null;
+interface LocalFieldEntry {
+  name: string;
+  id: string;
 }
 
-export function TooltipSection({
-  currentConfig,
-  setConfig,
-  dataView,
-  invalidFields,
-}: FieldInputsProps) {
+export function TooltipSection({ currentConfig, setConfig, dataView }: FieldInputsProps) {
   const { hasFieldData } = useExistingFieldsReader();
-  const onChangeWrapped = useCallback(
-    (values: WrappedValue[]) => {
-      setConfig({
-        ...currentConfig,
-        extraFields: values.filter(removeNewEmptyField).map(({ value }) => value),
-      });
-    },
-    [setConfig, currentConfig]
+
+  // 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 { 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);
+
+  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: newExtraFields,
+        });
       }
     },
-    [localValues, dataView, handleInputChange]
+    [setConfig, currentConfig]
   );
 
-  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}
     </>
   );
 }
diff --git a/packages/kbn-visualization-ui-components/components/query_input/filter_query_input.tsx b/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,18 +55,14 @@ 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);
   }, []);
 
   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}

diff --git a/...s/public/visualizations/xy/xy_config_panel/annotations_config_panel/annotations_panel.tsx b/...s/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>();

diff --git a/x-pack/test/functional/apps/lens/group6/annotations.ts b/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 });
     });