[7.x] [Dashboard] Fix cloning panels reactive issue (#74253) (#82812)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

src/plugins/dashboard/public/application/actions/add_to_library_action.test.tsx

@@ -134,19 +134,15 @@ test('Add to library is not compatible when embeddable is not in a dashboard con
   expect(await action.isCompatible({ embeddable: orphanContactCard })).toBe(false);
 });
 
-test('Add to library replaces embeddableId but retains panel count', async () => {
+test('Add to library replaces embeddableId and retains panel count', async () => {
   const dashboard = embeddable.getRoot() as IContainer;
   const originalPanelCount = Object.keys(dashboard.getInput().panels).length;
-  const originalPanelKeySet = new Set(Object.keys(dashboard.getInput().panels));
+
   const action = new AddToLibraryAction({ toasts: coreStart.notifications.toasts });
   await action.execute({ embeddable });
   expect(Object.keys(container.getInput().panels).length).toEqual(originalPanelCount);
-
-  const newPanelId = Object.keys(container.getInput().panels).find(
-    (key) => !originalPanelKeySet.has(key)
-  );
-  expect(newPanelId).toBeDefined();
-  const newPanel = container.getInput().panels[newPanelId!];
+  expect(Object.keys(container.getInput().panels)).toContain(embeddable.id);
+  const newPanel = container.getInput().panels[embeddable.id!];
   expect(newPanel.type).toEqual(embeddable.type);
 });
 
@@ -162,15 +158,10 @@ test('Add to library returns reference type input', async () => {
     mockedByReferenceInput: { savedObjectId: 'testSavedObjectId', id: embeddable.id },
     mockedByValueInput: { attributes: complicatedAttributes, id: embeddable.id } as EmbeddableInput,
   });
-  const dashboard = embeddable.getRoot() as IContainer;
-  const originalPanelKeySet = new Set(Object.keys(dashboard.getInput().panels));
   const action = new AddToLibraryAction({ toasts: coreStart.notifications.toasts });
   await action.execute({ embeddable });
-  const newPanelId = Object.keys(container.getInput().panels).find(
-    (key) => !originalPanelKeySet.has(key)
-  );
-  expect(newPanelId).toBeDefined();
-  const newPanel = container.getInput().panels[newPanelId!];
+  expect(Object.keys(container.getInput().panels)).toContain(embeddable.id);
+  const newPanel = container.getInput().panels[embeddable.id!];
   expect(newPanel.type).toEqual(embeddable.type);
   expect(newPanel.explicitInput.attributes).toBeUndefined();
   expect(newPanel.explicitInput.savedObjectId).toBe('testSavedObjectId');

src/plugins/dashboard/public/application/actions/clone_panel_action.test.tsx

@@ -108,7 +108,12 @@ test('Clone adds a new embeddable', async () => {
   );
   expect(newPanelId).toBeDefined();
   const newPanel = container.getInput().panels[newPanelId!];
-  expect(newPanel.type).toEqual(embeddable.type);
+  expect(newPanel.type).toEqual('placeholder');
+  // let the placeholder load
+  await dashboard.untilEmbeddableLoaded(newPanelId!);
+  // now wait for the full embeddable to replace it
+  const loadedPanel = await dashboard.untilEmbeddableLoaded(newPanelId!);
+  expect(loadedPanel.type).toEqual(embeddable.type);
 });
 
 test('Clones an embeddable without a saved object ID', async () => {

src/plugins/dashboard/public/application/actions/unlink_from_library_action.test.tsx

@@ -132,19 +132,14 @@ test('Unlink is not compatible when embeddable is not in a dashboard container',
   expect(await action.isCompatible({ embeddable: orphanContactCard })).toBe(false);
 });
 
-test('Unlink replaces embeddableId but retains panel count', async () => {
+test('Unlink replaces embeddableId and retains panel count', async () => {
   const dashboard = embeddable.getRoot() as IContainer;
   const originalPanelCount = Object.keys(dashboard.getInput().panels).length;
-  const originalPanelKeySet = new Set(Object.keys(dashboard.getInput().panels));
   const action = new UnlinkFromLibraryAction({ toasts: coreStart.notifications.toasts });
   await action.execute({ embeddable });
   expect(Object.keys(container.getInput().panels).length).toEqual(originalPanelCount);
-
-  const newPanelId = Object.keys(container.getInput().panels).find(
-    (key) => !originalPanelKeySet.has(key)
-  );
-  expect(newPanelId).toBeDefined();
-  const newPanel = container.getInput().panels[newPanelId!];
+  expect(Object.keys(container.getInput().panels)).toContain(embeddable.id);
+  const newPanel = container.getInput().panels[embeddable.id!];
   expect(newPanel.type).toEqual(embeddable.type);
 });
 
@@ -164,15 +159,10 @@ test('Unlink unwraps all attributes from savedObject', async () => {
     mockedByReferenceInput: { savedObjectId: 'testSavedObjectId', id: embeddable.id },
     mockedByValueInput: { attributes: complicatedAttributes, id: embeddable.id },
   });
-  const dashboard = embeddable.getRoot() as IContainer;
-  const originalPanelKeySet = new Set(Object.keys(dashboard.getInput().panels));
   const action = new UnlinkFromLibraryAction({ toasts: coreStart.notifications.toasts });
   await action.execute({ embeddable });
-  const newPanelId = Object.keys(container.getInput().panels).find(
-    (key) => !originalPanelKeySet.has(key)
-  );
-  expect(newPanelId).toBeDefined();
-  const newPanel = container.getInput().panels[newPanelId!];
+  expect(Object.keys(container.getInput().panels)).toContain(embeddable.id);
+  const newPanel = container.getInput().panels[embeddable.id!];
   expect(newPanel.type).toEqual(embeddable.type);
   expect(newPanel.explicitInput.attributes).toEqual(complicatedAttributes);
 });

src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx

@@ -27,6 +27,7 @@ import {
   ContactCardEmbeddableInput,
   ContactCardEmbeddable,
   ContactCardEmbeddableOutput,
+  EMPTY_EMBEDDABLE,
 } from '../../embeddable_plugin_test_samples';
 import { embeddablePluginMock } from 'src/plugins/embeddable/public/mocks';
 
@@ -100,6 +101,48 @@ test('DashboardContainer.addNewEmbeddable', async () => {
   expect(embeddableInContainer.id).toBe(embeddable.id);
 });
 
+test('DashboardContainer.replacePanel', async (done) => {
+  const ID = '123';
+  const initialInput = getSampleDashboardInput({
+    panels: {
+      [ID]: getSampleDashboardPanel<ContactCardEmbeddableInput>({
+        explicitInput: { firstName: 'Sam', id: ID },
+        type: CONTACT_CARD_EMBEDDABLE,
+      }),
+    },
+  });
+
+  const container = new DashboardContainer(initialInput, options);
+  let counter = 0;
+
+  const subscriptionHandler = jest.fn(({ panels }) => {
+    counter++;
+    expect(panels[ID]).toBeDefined();
+    // It should be called exactly 2 times and exit the second time
+    switch (counter) {
+      case 1:
+        return expect(panels[ID].type).toBe(CONTACT_CARD_EMBEDDABLE);
+
+      case 2: {
+        expect(panels[ID].type).toBe(EMPTY_EMBEDDABLE);
+        subscription.unsubscribe();
+        done();
+      }
+
+      default:
+        throw Error('Called too many times!');
+    }
+  });
+
+  const subscription = container.getInput$().subscribe(subscriptionHandler);
+
+  // replace the panel now
+  container.replacePanel(container.getInput().panels[ID], {
+    type: EMPTY_EMBEDDABLE,
+    explicitInput: { id: ID },
+  });
+});
+
 test('Container view mode change propagates to existing children', async () => {
   const initialInput = getSampleDashboardInput({
     panels: {

src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx

@@ -154,42 +154,43 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
       placementMethod,
       placementArgs
     );
+
     this.updateInput({
       panels: {
         ...this.input.panels,
         [placeholderPanelState.explicitInput.id]: placeholderPanelState,
       },
     });
-    newStateComplete.then((newPanelState: Partial<PanelState>) =>
-      this.replacePanel(placeholderPanelState, newPanelState)
-    );
+
+    // wait until the placeholder is ready, then replace it with new panel
+    // this is useful as sometimes panels can load faster than the placeholder one (i.e. by value embeddables)
+    this.untilEmbeddableLoaded(originalPanelState.explicitInput.id)
+      .then(() => newStateComplete)
+      .then((newPanelState: Partial<PanelState>) =>
+        this.replacePanel(placeholderPanelState, newPanelState)
+      );
   }
 
   public replacePanel(
     previousPanelState: DashboardPanelState<EmbeddableInput>,
     newPanelState: Partial<PanelState>
   ) {
-    // TODO: In the current infrastructure, embeddables in a container do not react properly to
-    // changes. Removing the existing embeddable, and adding a new one is a temporary workaround
-    // until the container logic is fixed.
-
-    const finalPanels = { ...this.input.panels };
-    delete finalPanels[previousPanelState.explicitInput.id];
-    const newPanelId = newPanelState.explicitInput?.id ? newPanelState.explicitInput.id : uuid.v4();
-    finalPanels[newPanelId] = {
-      ...previousPanelState,
-      ...newPanelState,
-      gridData: {
-        ...previousPanelState.gridData,
-        i: newPanelId,
-      },
-      explicitInput: {
-        ...newPanelState.explicitInput,
-        id: newPanelId,
+    // Because the embeddable type can change, we have to operate at the container level here
+    return this.updateInput({
+      panels: {
+        ...this.input.panels,
+        [previousPanelState.explicitInput.id]: {
+          ...previousPanelState,
+          ...newPanelState,
+          gridData: {
+            ...previousPanelState.gridData,
+          },
+          explicitInput: {
+            ...newPanelState.explicitInput,
+            id: previousPanelState.explicitInput.id,
+          },
+        },
       },
-    };
-    this.updateInput({
-      panels: finalPanels,
       lastReloadRequestTime: new Date().getTime(),
     });
   }
@@ -201,16 +202,15 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
   >(type: string, explicitInput: Partial<EEI>, embeddableId?: string) {
     const idToReplace = embeddableId || explicitInput.id;
     if (idToReplace && this.input.panels[idToReplace]) {
-      this.replacePanel(this.input.panels[idToReplace], {
+      return this.replacePanel(this.input.panels[idToReplace], {
         type,
         explicitInput: {
           ...explicitInput,
-          id: uuid.v4(),
+          id: idToReplace,
         },
       });
-    } else {
-      this.addNewEmbeddable<EEI, EEO, E>(type, explicitInput);
     }
+    return this.addNewEmbeddable<EEI, EEO, E>(type, explicitInput);
   }
 
   public render(dom: HTMLElement) {

src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx

@@ -265,13 +265,16 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
         <div
           style={{ zIndex: focusedPanelIndex === panel.explicitInput.id ? 2 : 'auto' }}
           className={classes}
+          // This key is required for the ReactGridLayout to work properly
           key={panel.explicitInput.id}
           data-test-subj="dashboardPanel"
           ref={(reactGridItem) => {
             this.gridItems[panel.explicitInput.id] = reactGridItem;
           }}
         >
           <EmbeddableChildPanel
+            // This key is used to force rerendering on embeddable type change while the id remains the same
+            key={panel.type}
             embeddableId={panel.explicitInput.id}
             container={this.props.container}
             PanelComponent={this.props.PanelComponent}

src/plugins/embeddable/public/lib/containers/container.ts

@@ -19,6 +19,7 @@
 
 import uuid from 'uuid';
 import { merge, Subscription } from 'rxjs';
+import { startWith, pairwise } from 'rxjs/operators';
 import {
   Embeddable,
   EmbeddableInput,
@@ -55,7 +56,12 @@ export abstract class Container<
     parent?: Container
   ) {
     super(input, output, parent);
-    this.subscription = this.getInput$().subscribe(() => this.maybeUpdateChildren());
+    this.subscription = this.getInput$()
+      // At each update event, get both the previous and current state
+      .pipe(startWith(input), pairwise())
+      .subscribe(([{ panels: prevPanels }, { panels: currentPanels }]) => {
+        this.maybeUpdateChildren(currentPanels, prevPanels);
+      });
   }
 
   public updateInputForChild<EEI extends EmbeddableInput = EmbeddableInput>(
@@ -329,16 +335,30 @@ export abstract class Container<
     return embeddable;
   }
 
-  private maybeUpdateChildren() {
-    const allIds = Object.keys({ ...this.input.panels, ...this.output.embeddableLoaded });
+  private panelHasChanged(currentPanel: PanelState, prevPanel: PanelState) {
+    if (currentPanel.type !== prevPanel.type) {
+      return true;
+    }
+  }
+
+  private maybeUpdateChildren(
+    currentPanels: TContainerInput['panels'],
+    prevPanels: TContainerInput['panels']
+  ) {
+    const allIds = Object.keys({ ...currentPanels, ...this.output.embeddableLoaded });
     allIds.forEach((id) => {
-      if (this.input.panels[id] !== undefined && this.output.embeddableLoaded[id] === undefined) {
-        this.onPanelAdded(this.input.panels[id]);
-      } else if (
-        this.input.panels[id] === undefined &&
-        this.output.embeddableLoaded[id] !== undefined
-      ) {
-        this.onPanelRemoved(id);
+      if (currentPanels[id] !== undefined && this.output.embeddableLoaded[id] === undefined) {
+        return this.onPanelAdded(currentPanels[id]);
+      }
+      if (currentPanels[id] === undefined && this.output.embeddableLoaded[id] !== undefined) {
+        return this.onPanelRemoved(id);
+      }
+      // In case of type change, remove and add a panel with the same id
+      if (currentPanels[id] && prevPanels[id]) {
+        if (this.panelHasChanged(currentPanels[id], prevPanels[id])) {
+          this.onPanelRemoved(id);
+          this.onPanelAdded(currentPanels[id]);
+        }
       }
     });
   }