[Lens] fix dimension panel datasource updates (#161881)

## Summary Fixes #161759 Fixes #161854 Fixes #161988 Fixes #161666 This bug surfaced by my change with NativeRenderer removal PR. It happens because sometimes there are two updates happening at the same time, in this case *`setState` coming from optimizing middleware to update the relative timerange in a call to es * updating datasource. Two updates is not a problem and works correctly for the same case for visualizations etc, but for the `updateDatasourceState` we break one of the fundamental rules of redux by passing non-serializable prop to action (an `updater` function that is then called internally with the previous state). The problem is that in the meantime the state gets updated from the previous `setState` call. To avoid this problem I rewrote the `updateDatasourceState` action to not accept updater function. To avoid these problems in the future I've also included a `serializableCheck` that ignores dataViews and some other properties (they contain some non-serializable data, I hope we'll take care of them in the future) ``` serializableCheck: { ignoredActionPaths: ['lens.dataViews.indexPatterns'], ignoredPaths: ['lens.dataViews.indexPatterns'], }, ``` I also fixed a few small issues, will leave comments in the code. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Stratoula Kalafateli <[email protected]>
elastic · Jul 17, 2023 · 9f67549 · 9f67549
1 parent 55bb58b
commit 9f67549
Show file tree

Hide file tree

Showing 21 changed files with 105 additions and 199 deletions.
diff --git a/...ins/chart_expressions/expression_heatmap/public/expression_renderers/heatmap_renderer.tsx b/...ins/chart_expressions/expression_heatmap/public/expression_renderers/heatmap_renderer.tsx
@@ -14,6 +14,7 @@ import { ExpressionRenderDefinition } from '@kbn/expressions-plugin/common/expre
 import { StartServicesGetter } from '@kbn/kibana-utils-plugin/public';
 import { METRIC_TYPE } from '@kbn/analytics';
 import { extractContainerType, extractVisualizationType } from '@kbn/chart-expressions-common';
+import { I18nProvider } from '@kbn/i18n-react';
 import { ExpressionHeatmapPluginStart } from '../plugin';
 import {
   EXPRESSION_HEATMAP_NAME,
@@ -79,24 +80,26 @@ export const heatmapRenderer: (
 
     render(
       <KibanaThemeProvider theme$={core.theme.theme$}>
-        <div className="heatmap-container" data-test-subj="heatmapChart">
-          <HeatmapComponent
-            {...config}
-            onClickValue={onClickValue}
-            onSelectRange={onSelectRange}
-            timeZone={timeZone}
-            datatableUtilities={getDatatableUtilities()}
-            formatFactory={getFormatService().deserialize}
-            chartsThemeService={plugins.charts.theme}
-            paletteService={getPaletteService()}
-            renderComplete={renderComplete}
-            uiState={handlers.uiState as PersistedState}
-            interactive={isInteractive()}
-            chartsActiveCursorService={plugins.charts.activeCursor}
-            syncTooltips={config.syncTooltips}
-            syncCursor={config.syncCursor}
-          />
-        </div>
+        <I18nProvider>
+          <div className="heatmap-container" data-test-subj="heatmapChart">
+            <HeatmapComponent
+              {...config}
+              onClickValue={onClickValue}
+              onSelectRange={onSelectRange}
+              timeZone={timeZone}
+              datatableUtilities={getDatatableUtilities()}
+              formatFactory={getFormatService().deserialize}
+              chartsThemeService={plugins.charts.theme}
+              paletteService={getPaletteService()}
+              renderComplete={renderComplete}
+              uiState={handlers.uiState as PersistedState}
+              interactive={isInteractive()}
+              chartsActiveCursorService={plugins.charts.activeCursor}
+              syncTooltips={config.syncTooltips}
+              syncCursor={config.syncCursor}
+            />
+          </div>
+        </I18nProvider>
       </KibanaThemeProvider>,
       domNode
     );

diff --git a/src/plugins/chart_expressions/expression_heatmap/tsconfig.json b/src/plugins/chart_expressions/expression_heatmap/tsconfig.json
@@ -27,6 +27,7 @@
     "@kbn/kibana-react-plugin",
     "@kbn/analytics",
     "@kbn/chart-expressions-common",
+    "@kbn/i18n-react",
   ],
   "exclude": [
     "target/**/*",

diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx
@@ -511,13 +511,17 @@ export function App({
             datasourceStates[activeDatasourceId].state,
             {
               frame: frameDatasourceAPI,
-              setState: (newStateOrUpdater) =>
+              setState: (newStateOrUpdater) => {
                 dispatch(
                   updateDatasourceState({
-                    updater: newStateOrUpdater,
+                    newDatasourceState:
+                      typeof newStateOrUpdater === 'function'
+                        ? newStateOrUpdater(datasourceStates[activeDatasourceId].state)
+                        : newStateOrUpdater,
                     datasourceId: activeDatasourceId,
                   })
-                ),
+                );
+              },
             }
           )
         : []),

diff --git a/x-pack/plugins/lens/public/datasources/form_based/datapanel.tsx b/x-pack/plugins/lens/public/datasources/form_based/datapanel.tsx
@@ -40,7 +40,7 @@ import { FieldItem } from '../common/field_item';
 
 export type Props = Omit<
   DatasourceDataPanelProps<FormBasedPrivateState>,
-  'core' | 'onChangeIndexPattern' | 'dragDropContext'
+  'core' | 'onChangeIndexPattern'
 > & {
   data: DataPublicPluginStart;
   dataViews: DataViewsPublicPluginStart;
@@ -183,7 +183,7 @@ export const InnerFormBasedDataPanel = function InnerFormBasedDataPanel({
   activeIndexPatterns,
 }: Omit<
   DatasourceDataPanelProps,
-  'state' | 'setState' | 'core' | 'onChangeIndexPattern' | 'usedIndexPatterns' | 'dragDropContext'
+  'state' | 'setState' | 'core' | 'onChangeIndexPattern' | 'usedIndexPatterns'
 > & {
   data: DataPublicPluginStart;
   dataViews: DataViewsPublicPluginStart;

diff --git a/x-pack/plugins/lens/public/datasources/form_based/dimension_panel/dimension_editor.tsx b/x-pack/plugins/lens/public/datasources/form_based/dimension_panel/dimension_editor.tsx
@@ -220,10 +220,6 @@ export function DimensionEditor(props: DimensionEditorProps) {
     [columnId, fireOrResetRandomSamplingToast, layerId, setState, state.layers]
   );
 
-  const setIsCloseable = (isCloseable: boolean) => {
-    setState((prevState) => ({ ...prevState, isDimensionClosePrevented: !isCloseable }));
-  };
-
   const incompleteInfo = (state.layers[layerId].incompleteColumns ?? {})[columnId];
   const {
     operationType: incompleteOperation,
@@ -276,7 +272,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
   // changes from the static value operation (which has to be a function)
   // Note: it forced a rerender at this point to avoid UI glitches in async updates (another hack upstream)
   // TODO: revisit this once we get rid of updateDatasourceAsync upstream
-  const moveDefinetelyToStaticValueAndUpdate = (
+  const moveDefinitelyToStaticValueAndUpdate = (
     setter:
       | FormBasedLayer
       | ((prevLayer: FormBasedLayer) => FormBasedLayer)
@@ -664,7 +660,6 @@ export function DimensionEditor(props: DimensionEditorProps) {
     operationDefinitionMap,
     toggleFullscreen,
     isFullscreen,
-    setIsCloseable,
     paramEditorCustomProps,
     ReferenceEditor,
     dataSectionExtra: props.dataSectionExtra,
@@ -865,7 +860,6 @@ export function DimensionEditor(props: DimensionEditorProps) {
                 })}
                 isFullscreen={isFullscreen}
                 toggleFullscreen={toggleFullscreen}
-                setIsCloseable={setIsCloseable}
                 paramEditorCustomProps={paramEditorCustomProps}
                 {...services}
               />
@@ -927,7 +921,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
         layer={state.layers[layerId]}
         activeData={props.activeData}
         paramEditorUpdater={
-          temporaryStaticValue ? moveDefinetelyToStaticValueAndUpdate : setStateWrapper
+          temporaryStaticValue ? moveDefinitelyToStaticValueAndUpdate : setStateWrapper
         }
         columnId={columnId}
         currentColumn={state.layers[layerId].columns[columnId]}
@@ -938,7 +932,6 @@ export function DimensionEditor(props: DimensionEditorProps) {
         isFullscreen={isFullscreen}
         indexPattern={currentIndexPattern}
         toggleFullscreen={toggleFullscreen}
-        setIsCloseable={setIsCloseable}
         ReferenceEditor={ReferenceEditor}
         {...services}
       />

diff --git a/x-pack/plugins/lens/public/datasources/form_based/dimension_panel/reference_editor.tsx b/x-pack/plugins/lens/public/datasources/form_based/dimension_panel/reference_editor.tsx
@@ -82,7 +82,6 @@ export interface ReferenceEditorProps {
   labelAppend?: EuiFormRowProps['labelAppend'];
   isFullscreen: boolean;
   toggleFullscreen: () => void;
-  setIsCloseable: (isCloseable: boolean) => void;
   paramEditorCustomProps?: ParamEditorCustomProps;
   paramEditorUpdater: (
     setter:

diff --git a/x-pack/plugins/lens/public/datasources/form_based/form_based.tsx b/x-pack/plugins/lens/public/datasources/form_based/form_based.tsx
@@ -541,10 +541,6 @@ export function getFormBasedDatasource({
       );
     },
 
-    canCloseDimensionEditor: (state) => {
-      return !state.isDimensionClosePrevented;
-    },
-
     getDropProps,
     onDrop,
 

diff --git a/...ns/public/datasources/form_based/operations/definitions/formula/editor/formula_editor.tsx b/...ns/public/datasources/form_based/operations/definitions/formula/editor/formula_editor.tsx
@@ -106,7 +106,6 @@ export function FormulaEditor({
   dataViews,
   toggleFullscreen,
   isFullscreen,
-  setIsCloseable,
   dateHistogramInterval,
   hasData,
   dateRange,
@@ -175,7 +174,6 @@ export function FormulaEditor({
   }, []);
 
   useUnmount(() => {
-    setIsCloseable(true);
     // If the text is not synced, update the column.
     if (text !== currentColumn.params.formula) {
       paramEditorUpdater(
@@ -792,20 +790,6 @@ export function FormulaEditor({
                   if (model) {
                     editorModel.current = model;
                   }
-                  disposables.current.push(
-                    editor.onDidFocusEditorWidget(() => {
-                      setTimeout(() => {
-                        setIsCloseable(false);
-                      });
-                    })
-                  );
-                  disposables.current.push(
-                    editor.onDidBlurEditorWidget(() => {
-                      setTimeout(() => {
-                        setIsCloseable(true);
-                      });
-                    })
-                  );
                   // If we ever introduce a second Monaco editor, we need to toggle
                   // the typing handler to the active editor to maintain the cursor
                   disposables.current.push(

diff --git a/x-pack/plugins/lens/public/datasources/form_based/operations/definitions/index.ts b/x-pack/plugins/lens/public/datasources/form_based/operations/definitions/index.ts
@@ -192,7 +192,6 @@ export interface ParamEditorProps<
   paramEditorUpdater: (setter: U) => void;
   ReferenceEditor?: (props: ReferenceEditorProps) => JSX.Element | null;
   toggleFullscreen: () => void;
-  setIsCloseable: (isCloseable: boolean) => void;
   isFullscreen: boolean;
   columnId: string;
   layerId: string;

diff --git a/x-pack/plugins/lens/public/datasources/form_based/types.ts b/x-pack/plugins/lens/public/datasources/form_based/types.ts
@@ -67,7 +67,6 @@ export type PersistedIndexPatternLayer = Omit<FormBasedLayer, 'indexPatternId'>;
 export interface FormBasedPrivateState {
   currentIndexPatternId: string;
   layers: Record<string, FormBasedLayer>;
-  isDimensionClosePrevented?: boolean;
 }
 
 export interface DataViewDragDropOperation extends DragDropOperation {

diff --git a/.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx b/.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx
@@ -168,7 +168,7 @@ describe('ConfigPanel', () => {
     expect(instance.find(LayerPanel).exists()).toBe(false);
   });
 
-  it('allow datasources and visualizations to use setters', async () => {
+  it('updates datasources and visualizations', async () => {
     const props = getDefaultProps();
     const onUpdateCbSpy = jest.fn();
     const newProps = {
@@ -178,27 +178,23 @@ describe('ConfigPanel', () => {
     const { instance, lensStore } = await prepareAndMountComponent(newProps);
     const { updateDatasource, updateAll } = instance.find(LayerPanel).props();
 
-    const updater = () => 'updated';
-    updateDatasource('testDatasource', updater);
+    const newDatasourceState = 'updated';
+    updateDatasource('testDatasource', newDatasourceState);
     await waitMs(0);
     expect(lensStore.dispatch).toHaveBeenCalledTimes(1);
     expect(onUpdateCbSpy).toHaveBeenCalled();
-    expect(
-      (lensStore.dispatch as jest.Mock).mock.calls[0][0].payload.updater(
-        props.datasourceStates.testDatasource.state
-      )
-    ).toEqual('updated');
+    expect((lensStore.dispatch as jest.Mock).mock.calls[0][0].payload.newDatasourceState).toEqual(
+      'updated'
+    );
 
-    updateAll('testDatasource', updater, props.visualizationState);
+    updateAll('testDatasource', newDatasourceState, props.visualizationState);
     expect(onUpdateCbSpy).toHaveBeenCalled();
     // wait for one tick so async updater has a chance to trigger
     await waitMs(0);
-    expect(lensStore.dispatch).toHaveBeenCalledTimes(2);
-    expect(
-      (lensStore.dispatch as jest.Mock).mock.calls[0][0].payload.updater(
-        props.datasourceStates.testDatasource.state
-      )
-    ).toEqual('updated');
+    expect(lensStore.dispatch).toHaveBeenCalledTimes(3);
+    expect((lensStore.dispatch as jest.Mock).mock.calls[0][0].payload.newDatasourceState).toEqual(
+      'updated'
+    );
   });
 
   describe('focus behavior when adding or removing layers', () => {

diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.tsx
@@ -14,6 +14,7 @@ import {
   UPDATE_FILTER_REFERENCES_ACTION,
   UPDATE_FILTER_REFERENCES_TRIGGER,
 } from '@kbn/unified-search-plugin/public';
+import { isEqual } from 'lodash';
 import { changeIndexPattern, removeDimension } from '../../../state_management/lens_slice';
 import { AddLayerFunction, Visualization } from '../../../types';
 import { LayerPanel } from './layer_panel';
@@ -26,7 +27,6 @@ import {
   removeOrClearLayer,
   cloneLayer,
   addLayer as addLayerAction,
-  updateState,
   updateDatasourceState,
   updateVisualizationState,
   setToggleFullscreen,
@@ -87,31 +87,36 @@ export function LayerPanels(
     () =>
       (datasourceId: string | undefined, newState: unknown, dontSyncLinkedDimensions?: boolean) => {
         if (datasourceId) {
+          const newDatasourceState =
+            typeof newState === 'function'
+              ? newState(datasourceStates[datasourceId].state)
+              : newState;
+
+          if (isEqual(newDatasourceState, datasourceStates[datasourceId].state)) {
+            return;
+          }
+
+          onUpdateStateCb?.(newDatasourceState, visualization.state);
+
           dispatchLens(
             updateDatasourceState({
-              updater: (prevState: unknown) => {
-                onUpdateStateCb?.(
-                  typeof newState === 'function' ? newState(prevState) : newState,
-                  visualization.state
-                );
-                return typeof newState === 'function' ? newState(prevState) : newState;
-              },
+              newDatasourceState,
               datasourceId,
               clearStagedPreview: false,
               dontSyncLinkedDimensions,
             })
           );
         }
       },
-    [dispatchLens, onUpdateStateCb, visualization.state]
+    [dispatchLens, onUpdateStateCb, visualization.state, datasourceStates]
   );
   const updateDatasourceAsync = useMemo(
     () => (datasourceId: string | undefined, newState: unknown) => {
       // React will synchronously update if this is triggered from a third party component,
       // which we don't want. The timeout lets user interaction have priority, then React updates.
       setTimeout(() => {
         updateDatasource(datasourceId, newState);
-      }, 0);
+      });
     },
     [updateDatasource]
   );
@@ -128,40 +133,34 @@ export function LayerPanels(
         // which we don't want. The timeout lets user interaction have priority, then React updates.
 
         setTimeout(() => {
+          const newDsState =
+            typeof newDatasourceState === 'function'
+              ? newDatasourceState(datasourceStates[datasourceId].state)
+              : newDatasourceState;
+
+          const newVisState =
+            typeof newVisualizationState === 'function'
+              ? newVisualizationState(visualization.state)
+              : newVisualizationState;
+
+          onUpdateStateCb?.(newDsState, newVisState);
+
+          dispatchLens(
+            updateVisualizationState({
+              visualizationId: activeVisualization.id,
+              newState: newVisState,
+            })
+          );
           dispatchLens(
-            updateState({
-              updater: (prevState) => {
-                const updatedDatasourceState =
-                  typeof newDatasourceState === 'function'
-                    ? newDatasourceState(prevState.datasourceStates[datasourceId].state)
-                    : newDatasourceState;
-
-                const updatedVisualizationState =
-                  typeof newVisualizationState === 'function'
-                    ? newVisualizationState(prevState.visualization.state)
-                    : newVisualizationState;
-                onUpdateStateCb?.(updatedDatasourceState, updatedVisualizationState);
-
-                return {
-                  ...prevState,
-                  datasourceStates: {
-                    ...prevState.datasourceStates,
-                    [datasourceId]: {
-                      state: updatedDatasourceState,
-                      isLoading: false,
-                    },
-                  },
-                  visualization: {
-                    ...prevState.visualization,
-                    state: updatedVisualizationState,
-                  },
-                };
-              },
+            updateDatasourceState({
+              newDatasourceState: newDsState,
+              datasourceId,
+              clearStagedPreview: false,
             })
           );
         }, 0);
       },
-    [dispatchLens, onUpdateStateCb]
+    [dispatchLens, onUpdateStateCb, visualization.state, datasourceStates, activeVisualization.id]
   );
 
   const toggleFullscreen = useMemo(
-Original file line number
+Diff line change
@@ Expand Up / @@ -541,10 +541,6 @@ export function getFormBasedDatasource({ @@
           );
         },
-        canCloseDimensionEditor: (state) => {
-          return !state.isDimensionClosePrevented;
-        },
         getDropProps,
         onDrop,
@@ Expand Down @@