[Dashboard Navigation] Simplify state management (elastic#171581)

Closes elastic#167577 ## Summary Previously, the Link embeddable used the whole redux embeddable package - however, the overall state that needs to be managed for this panel is very simple, so this ended up being overkill. This PR fixes that by adding a `useLinksAttributes` hook to replace the redux package that subscribes to changes made to the attributes instead. I also made two smaller changes in this PR: 1. Called the "Organize imports" command from VSCode on all of the touched files - this explains all of the seemingly unrelated import changes. 2. I fixed the React warning that was being thrown due to calling `setIsSaving` after the component was unmounted. ### How to Test To test number 2 above, create a by-reference Links panel and refresh the dashboard. Then, 1. Make some sort of change to the Links panel, such as re-arranging the links 2. Save the changes - note that, without the mount check, the following React error will be thrown: ![image](https://github.com/elastic/kibana/assets/8698078/88573c7b-8469-490d-83dd-5e335573aa75) 3. Now, with the mount check, this no longer happens 🎉 ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### 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: Kibana Machine <[email protected]>
tsullivan · Nov 21, 2023 · be46cce · be46cce
1 parent a5d1810
commit be46cce
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 144 deletions.
diff --git a/src/plugins/links/public/components/dashboard_link/dashboard_link_component.tsx b/src/plugins/links/public/components/dashboard_link/dashboard_link_component.tsx
@@ -7,24 +7,24 @@
  */
 
 import classNames from 'classnames';
-import useAsync from 'react-use/lib/useAsync';
 import React, { useEffect, useMemo, useState } from 'react';
+import useAsync from 'react-use/lib/useAsync';
 import useObservable from 'react-use/lib/useObservable';
 
-import {
-  DashboardDrilldownOptions,
-  DEFAULT_DASHBOARD_DRILLDOWN_OPTIONS,
-} from '@kbn/presentation-util-plugin/public';
 import { EuiButtonEmpty, EuiListGroupItem } from '@elastic/eui';
-import { DashboardContainer } from '@kbn/dashboard-plugin/public/dashboard_container';
 import {
   DashboardLocatorParams,
   getDashboardLocatorParamsFromEmbeddable,
 } from '@kbn/dashboard-plugin/public';
+import { DashboardContainer } from '@kbn/dashboard-plugin/public/dashboard_container';
+import {
+  DashboardDrilldownOptions,
+  DEFAULT_DASHBOARD_DRILLDOWN_OPTIONS,
+} from '@kbn/presentation-util-plugin/public';
 
-import { LINKS_VERTICAL_LAYOUT, LinksLayoutType, Link } from '../../../common/content_management';
+import { Link, LinksLayoutType, LINKS_VERTICAL_LAYOUT } from '../../../common/content_management';
+import { useLinks } from '../links_hooks';
 import { DashboardLinkStrings } from './dashboard_link_strings';
-import { useLinks } from '../../embeddable/links_embeddable';
 import { fetchDashboard } from './dashboard_link_tools';
 
 export const DashboardLinkComponent = ({

diff --git a/src/plugins/links/public/components/editor/links_editor.tsx b/src/plugins/links/public/components/editor/links_editor.tsx
@@ -7,41 +7,42 @@
  */
 
 import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react';
+import useMountedState from 'react-use/lib/useMountedState';
 
 import {
-  EuiForm,
   EuiBadge,
-  EuiTitle,
   EuiButton,
-  EuiSwitch,
-  EuiFormRow,
-  EuiToolTip,
-  EuiFlexItem,
-  EuiFlexGroup,
-  EuiDroppable,
-  EuiDraggable,
-  EuiFlyoutBody,
   EuiButtonEmpty,
   EuiButtonGroup,
-  EuiFlyoutFooter,
-  EuiFlyoutHeader,
+  EuiButtonGroupOptionProps,
   EuiDragDropContext,
   euiDragDropReorder,
-  EuiButtonGroupOptionProps,
+  EuiDraggable,
+  EuiDroppable,
+  EuiFlexGroup,
+  EuiFlexItem,
+  EuiFlyoutBody,
+  EuiFlyoutFooter,
+  EuiFlyoutHeader,
+  EuiForm,
+  EuiFormRow,
+  EuiSwitch,
+  EuiTitle,
+  EuiToolTip,
 } from '@elastic/eui';
 import { DashboardContainer } from '@kbn/dashboard-plugin/public/dashboard_container';
 
-import { LinksLayoutInfo } from '../../embeddable/types';
 import {
   Link,
   LinksLayoutType,
   LINKS_HORIZONTAL_LAYOUT,
   LINKS_VERTICAL_LAYOUT,
 } from '../../../common/content_management';
+import { memoizedGetOrderedLinkList } from '../../editor/links_editor_tools';
+import { openLinkEditorFlyout } from '../../editor/open_link_editor_flyout';
+import { LinksLayoutInfo } from '../../embeddable/types';
 import { coreServices } from '../../services/kibana_services';
 import { LinksStrings } from '../links_strings';
-import { openLinkEditorFlyout } from '../../editor/open_link_editor_flyout';
-import { memoizedGetOrderedLinkList } from '../../editor/links_editor_tools';
 import { LinksEditorEmptyPrompt } from './links_editor_empty_prompt';
 import { LinksEditorSingleLink } from './links_editor_single_link';
 
@@ -80,6 +81,7 @@ const LinksEditor = ({
   isByReference: boolean;
 }) => {
   const toasts = coreServices.notifications.toasts;
+  const isMounted = useMountedState();
   const editLinkFlyoutRef = useRef<HTMLDivElement>(null);
 
   const [currentLayout, setCurrentLayout] = useState<LinksLayoutType>(
@@ -294,7 +296,9 @@ const LinksEditor = ({
                             });
                           })
                           .finally(() => {
-                            setIsSaving(false);
+                            if (isMounted()) {
+                              setIsSaving(false);
+                            }
                           });
                       } else {
                         onAddToDashboard(orderedLinks, currentLayout);

diff --git a/src/plugins/links/public/components/links_component.tsx b/src/plugins/links/public/components/links_component.tsx
@@ -6,29 +6,28 @@
  * Side Public License, v 1.
  */
 
+import { EuiListGroup, EuiPanel } from '@elastic/eui';
 import React, { useEffect, useMemo } from 'react';
 import useMap from 'react-use/lib/useMap';
-import { EuiListGroup, EuiPanel } from '@elastic/eui';
-import { useLinks } from '../embeddable/links_embeddable';
-import { ExternalLinkComponent } from './external_link/external_link_component';
-import { DashboardLinkComponent } from './dashboard_link/dashboard_link_component';
-import { memoizedGetOrderedLinkList } from '../editor/links_editor_tools';
 import {
   DASHBOARD_LINK_TYPE,
   LINKS_HORIZONTAL_LAYOUT,
   LINKS_VERTICAL_LAYOUT,
 } from '../../common/content_management';
+import { memoizedGetOrderedLinkList } from '../editor/links_editor_tools';
+import { DashboardLinkComponent } from './dashboard_link/dashboard_link_component';
+import { ExternalLinkComponent } from './external_link/external_link_component';
 
 import './links_component.scss';
+import { useLinks, useLinksAttributes } from './links_hooks';
 
 export const LinksComponent = () => {
   const linksEmbeddable = useLinks();
-  const links = linksEmbeddable.select((state) => state.componentState.links);
-  const layout = linksEmbeddable.select((state) => state.componentState.layout);
+  const linksAttributes = useLinksAttributes();
 
   const [linksLoading, { set: setLinkIsLoading }] = useMap(
     Object.fromEntries(
-      (links ?? []).map((link) => {
+      (linksAttributes?.links ?? []).map((link) => {
         return [link.id, true];
       })
     )
@@ -43,12 +42,12 @@ export const LinksComponent = () => {
   }, [linksLoading, linksEmbeddable]);
 
   const orderedLinks = useMemo(() => {
-    if (!links) return [];
-    return memoizedGetOrderedLinkList(links);
-  }, [links]);
+    if (!linksAttributes?.links) return [];
+    return memoizedGetOrderedLinkList(linksAttributes?.links);
+  }, [linksAttributes]);
 
   const linkItems: { [id: string]: { id: string; content: JSX.Element } } = useMemo(() => {
-    return (links ?? []).reduce((prev, currentLink) => {
+    return (linksAttributes?.links ?? []).reduce((prev, currentLink) => {
       return {
         ...prev,
         [currentLink.id]: {
@@ -58,34 +57,34 @@ export const LinksComponent = () => {
               <DashboardLinkComponent
                 key={currentLink.id}
                 link={currentLink}
-                layout={layout ?? LINKS_VERTICAL_LAYOUT}
+                layout={linksAttributes?.layout ?? LINKS_VERTICAL_LAYOUT}
                 onLoading={() => setLinkIsLoading(currentLink.id, true)}
                 onRender={() => setLinkIsLoading(currentLink.id, false)}
               />
             ) : (
               <ExternalLinkComponent
                 key={currentLink.id}
                 link={currentLink}
-                layout={layout ?? LINKS_VERTICAL_LAYOUT}
+                layout={linksAttributes?.layout ?? LINKS_VERTICAL_LAYOUT}
                 onRender={() => setLinkIsLoading(currentLink.id, false)}
               />
             ),
         },
       };
     }, {});
-  }, [links, layout, setLinkIsLoading]);
+  }, [linksAttributes?.links, linksAttributes?.layout, setLinkIsLoading]);
 
   return (
     <EuiPanel
       className={`linksComponent ${
-        layout === LINKS_HORIZONTAL_LAYOUT ? 'eui-xScroll' : 'eui-yScroll'
+        linksAttributes?.layout === LINKS_HORIZONTAL_LAYOUT ? 'eui-xScroll' : 'eui-yScroll'
       }`}
       paddingSize="xs"
       data-test-subj="links--component"
     >
       <EuiListGroup
         maxWidth={false}
-        className={`${layout}LayoutWrapper`}
+        className={`${linksAttributes?.layout ?? LINKS_VERTICAL_LAYOUT}LayoutWrapper`}
         data-test-subj="links--component--listGroup"
       >
         {orderedLinks.map((link) => linkItems[link.id].content)}

diff --git a/src/plugins/links/public/components/links_hooks.tsx b/src/plugins/links/public/components/links_hooks.tsx
@@ -0,0 +1,38 @@
+/*
+ * 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 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 or the Server
+ * Side Public License, v 1.
+ */
+
+import { useContext, useEffect, useState } from 'react';
+
+import { LinksAttributes } from '../../common/content_management';
+import { LinksContext, LinksEmbeddable } from '../embeddable/links_embeddable';
+
+export const useLinks = (): LinksEmbeddable => {
+  const linksEmbeddable = useContext<LinksEmbeddable | null>(LinksContext);
+  if (linksEmbeddable == null) {
+    throw new Error('useLinks must be used inside LinksContext.');
+  }
+  return linksEmbeddable!;
+};
+
+export const useLinksAttributes = (): LinksAttributes | undefined => {
+  const linksEmbeddable = useLinks();
+  const [attributes, setAttributes] = useState<LinksAttributes | undefined>(
+    linksEmbeddable.attributes
+  );
+
+  useEffect(() => {
+    const attributesSubscription = linksEmbeddable.attributes$.subscribe((newAttributes) => {
+      setAttributes(newAttributes);
+    });
+    return () => {
+      attributesSubscription.unsubscribe();
+    };
+  }, [linksEmbeddable.attributes$]);
+
+  return attributes;
+};
diff --git a/src/plugins/links/public/embeddable/links_embeddable.tsx b/src/plugins/links/public/embeddable/links_embeddable.tsx
@@ -6,37 +6,25 @@
  * Side Public License, v 1.
  */
 
-import React, { createContext, useContext } from 'react';
-import { unmountComponentAtNode } from 'react-dom';
-import { Subscription, distinctUntilChanged, skip, switchMap } from 'rxjs';
 import deepEqual from 'fast-deep-equal';
+import React, { createContext } from 'react';
+import { unmountComponentAtNode } from 'react-dom';
+import { distinctUntilChanged, skip, Subject, Subscription, switchMap } from 'rxjs';
 
+import { DashboardContainer } from '@kbn/dashboard-plugin/public/dashboard_container';
 import {
   AttributeService,
   Embeddable,
   ReferenceOrValueEmbeddable,
   SavedObjectEmbeddableInput,
 } from '@kbn/embeddable-plugin/public';
-import { DashboardContainer } from '@kbn/dashboard-plugin/public/dashboard_container';
-import { ReduxEmbeddableTools, ReduxToolsPackage } from '@kbn/presentation-util-plugin/public';
 
-import { linksReducers } from './links_reducers';
-import { LinksByReferenceInput, LinksByValueInput, LinksReduxState } from './types';
-import { LinksComponent } from '../components/links_component';
-import { LinksInput, LinksOutput } from './types';
-import { LinksAttributes } from '../../common/content_management';
 import { CONTENT_ID } from '../../common';
+import { LinksAttributes } from '../../common/content_management';
+import { LinksComponent } from '../components/links_component';
+import { LinksByReferenceInput, LinksByValueInput, LinksInput, LinksOutput } from './types';
 
 export const LinksContext = createContext<LinksEmbeddable | null>(null);
-export const useLinks = (): LinksEmbeddable => {
-  const linksEmbeddable = useContext<LinksEmbeddable | null>(LinksContext);
-  if (linksEmbeddable == null) {
-    throw new Error('useLinks must be used inside LinksContext.');
-  }
-  return linksEmbeddable!;
-};
-
-type LinksReduxEmbeddableTools = ReduxEmbeddableTools<LinksReduxState, typeof linksReducers>;
 
 export interface LinksConfig {
   editable: boolean;
@@ -53,20 +41,10 @@ export class LinksEmbeddable
   private isDestroyed?: boolean;
   private subscriptions: Subscription = new Subscription();
 
-  // state management
-  /**
-   * TODO: Keep track of the necessary state without the redux embeddable tools; it's kind of overkill here.
-   *       Related issue: https://github.com/elastic/kibana/issues/167577
-   */
-  public select: LinksReduxEmbeddableTools['select'];
-  public getState: LinksReduxEmbeddableTools['getState'];
-  public dispatch: LinksReduxEmbeddableTools['dispatch'];
-  public onStateChange: LinksReduxEmbeddableTools['onStateChange'];
-
-  private cleanupStateTools: () => void;
+  public attributes?: LinksAttributes;
+  public attributes$ = new Subject<LinksAttributes>();
 
   constructor(
-    reduxToolsPackage: ReduxToolsPackage,
     config: LinksConfig,
     initialInput: LinksInput,
     private attributeService: AttributeService<LinksAttributes>,
@@ -81,29 +59,11 @@ export class LinksEmbeddable
       parent
     );
 
-    /** Build redux embeddable tools */
-    const reduxEmbeddableTools = reduxToolsPackage.createReduxEmbeddableTools<
-      LinksReduxState,
-      typeof linksReducers
-    >({
-      embeddable: this,
-      reducers: linksReducers,
-      initialComponentState: {
-        title: '',
-      },
-    });
-
-    this.select = reduxEmbeddableTools.select;
-    this.getState = reduxEmbeddableTools.getState;
-    this.dispatch = reduxEmbeddableTools.dispatch;
-    this.cleanupStateTools = reduxEmbeddableTools.cleanup;
-    this.onStateChange = reduxEmbeddableTools.onStateChange;
-
     this.initializeSavedLinks()
       .then(() => this.setInitializationFinished())
       .catch((e: Error) => this.onFatalError(e));
 
-    // By-value panels should update the componentState when input changes
+    // By-value panels should update the links attributes when input changes
     this.subscriptions.add(
       this.getInput$()
         .pipe(
@@ -113,25 +73,28 @@ export class LinksEmbeddable
         )
         .subscribe()
     );
+
+    // Keep attributes in sync with subject value so it can be used in output
+    this.subscriptions.add(
+      this.attributes$.pipe(distinctUntilChanged(deepEqual)).subscribe((attributes) => {
+        this.attributes = attributes;
+      })
+    );
   }
 
   private async initializeSavedLinks() {
     const { attributes } = await this.attributeService.unwrapAttributes(this.getInput());
-    if (this.isDestroyed) return;
-
-    this.dispatch.setAttributes(attributes);
-
+    this.attributes$.next(attributes);
     await this.initializeOutput();
   }
 
   private async initializeOutput() {
-    const attributes = this.getState().componentState;
     const { title, description } = this.getInput();
     this.updateOutput({
-      defaultTitle: attributes.title,
-      defaultDescription: attributes.description,
-      title: title ?? attributes.title,
-      description: description ?? attributes.description,
+      defaultTitle: this.attributes?.title,
+      defaultDescription: this.attributes?.description,
+      title: title ?? this.attributes?.title,
+      description: description ?? this.attributes?.description,
     });
   }
 
@@ -162,7 +125,7 @@ export class LinksEmbeddable
 
   public async reload() {
     if (this.isDestroyed) return;
-    // By-reference embeddable panels are reloaded when changed, so update the componentState
+    // By-reference embeddable panels are reloaded when changed, so update the attributes
     this.initializeSavedLinks();
     if (this.domNode) {
       this.render(this.domNode);
@@ -173,7 +136,6 @@ export class LinksEmbeddable
     this.isDestroyed = true;
     super.destroy();
     this.subscriptions.unsubscribe();
-    this.cleanupStateTools();
     if (this.domNode) {
       unmountComponentAtNode(this.domNode);
     }