Testing Reactive Issue With Vis By Value

src/plugins/dashboard/public/application/dashboard_app_controller.tsx

@@ -468,9 +468,14 @@ export class DashboardAppController {
                 const explicitInput = {
                   savedVis: input,
                 };
+                const embeddableId =
+                  'embeddableId' in incomingEmbeddable
+                    ? incomingEmbeddable.embeddableId
+                    : undefined;
                 container.addOrUpdateEmbeddable<EmbeddableInput>(
                   incomingEmbeddable.type,
-                  explicitInput
+                  explicitInput,
+                  embeddableId
                 );
               }
             }

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

@@ -168,45 +168,42 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
     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(),
     });
   }
 
   public async addOrUpdateEmbeddable<
     EEI extends EmbeddableInput = EmbeddableInput,
     EEO extends EmbeddableOutput = EmbeddableOutput,
     E extends IEmbeddable<EEI, EEO> = IEmbeddable<EEI, EEO>
-  >(type: string, explicitInput: Partial<EEI>) {
-    if (explicitInput.id && this.input.panels[explicitInput.id]) {
-      this.replacePanel(this.input.panels[explicitInput.id], {
+  >(type: string, explicitInput: Partial<EEI>, embeddableId?: string) {
+    const idToReplace = embeddableId || explicitInput.id;
+    if (idToReplace && this.input.panels[idToReplace]) {
+      this.replacePanel(this.input.panels[idToReplace], {
         type,
         explicitInput: {
           ...explicitInput,
-          id: uuid.v4(),
+          // TS does not catch up with the typeguard above, so it needs to be explicit
+          id: idToReplace,
         },
       });
     } else {
-      this.addNewEmbeddable<EEI, EEO, E>(type, explicitInput);
+      return this.addNewEmbeddable<EEI, EEO, E>(type, explicitInput);
     }
   }
 

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/actions/edit_panel_action.test.tsx

@@ -56,12 +56,18 @@ test('is compatible when edit url is available, in edit mode and editable', asyn
 test('redirects to app using state transfer', async () => {
   applicationMock.currentAppId$ = of('superCoolCurrentApp');
   const action = new EditPanelAction(getFactory, applicationMock, stateTransferMock);
-  const embeddable = new EditableEmbeddable({ id: '123', viewMode: ViewMode.EDIT }, true);
+  const input = { id: '123', viewMode: ViewMode.EDIT };
+  const embeddable = new EditableEmbeddable(input, true);
   embeddable.getOutput = jest.fn(() => ({ editApp: 'ultraVisualize', editPath: '/123' }));
   await action.execute({ embeddable });
   expect(stateTransferMock.navigateToEditor).toHaveBeenCalledWith('ultraVisualize', {
     path: '/123',
-    state: { originatingApp: 'superCoolCurrentApp' },
+    state: {
+      originatingApp: 'superCoolCurrentApp',
+      byValueMode: true,
+      embeddableId: '123',
+      valueInput: input,
+    },
   });
 });
 

src/plugins/embeddable/public/lib/actions/edit_panel_action.ts

@@ -24,7 +24,12 @@ import { take } from 'rxjs/operators';
 import { ViewMode } from '../types';
 import { EmbeddableFactoryNotFoundError } from '../errors';
 import { EmbeddableStart } from '../../plugin';
-import { IEmbeddable, EmbeddableEditorState, EmbeddableStateTransfer } from '../..';
+import {
+  IEmbeddable,
+  EmbeddableEditorState,
+  EmbeddableStateTransfer,
+  SavedObjectEmbeddableInput,
+} from '../..';
 
 export const ACTION_EDIT_PANEL = 'editPanel';
 
@@ -109,8 +114,17 @@ export class EditPanelAction implements Action<ActionContext> {
     const app = embeddable ? embeddable.getOutput().editApp : undefined;
     const path = embeddable ? embeddable.getOutput().editPath : undefined;
     if (app && path) {
-      const state = this.currentAppId ? { originatingApp: this.currentAppId } : undefined;
-      return { app, path, state };
+      if (this.currentAppId) {
+        const byValueMode = !(embeddable.getInput() as SavedObjectEmbeddableInput).savedObjectId;
+        const state: EmbeddableEditorState = {
+          originatingApp: this.currentAppId,
+          byValueMode,
+          valueInput: byValueMode ? embeddable.getInput() : undefined,
+          embeddableId: embeddable.id,
+        };
+        return { app, path, state };
+      }
+      return { app, path };
     }
   }
 

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,
@@ -54,7 +55,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>(
@@ -328,16 +334,24 @@ export abstract class Container<
     return embeddable;
   }
 
-  private maybeUpdateChildren() {
-    const allIds = Object.keys({ ...this.input.panels, ...this.output.embeddableLoaded });
+  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 (currentPanels[id].type !== prevPanels[id].type) {
+          this.onPanelRemoved(id);
+          this.onPanelAdded(currentPanels[id]);
+        }
       }
     });
   }

src/plugins/embeddable/public/lib/state_transfer/types.ts

@@ -27,6 +27,7 @@ export interface EmbeddableEditorState {
   originatingApp: string;
   byValueMode?: boolean;
   valueInput?: EmbeddableInput;
+  embeddableId?: string;
 }
 
 export function isEmbeddableEditorState(state: unknown): state is EmbeddableEditorState {
@@ -49,6 +50,7 @@ export interface EmbeddablePackageByReferenceState {
 export interface EmbeddablePackageByValueState {
   type: string;
   input: EmbeddableInput;
+  embeddableId?: string;
 }
 
 export type EmbeddablePackageState =

src/plugins/navigation/public/top_nav_menu/top_nav_menu_item.tsx

@@ -46,6 +46,7 @@ export function TopNavMenuItem(props: TopNavMenuData) {
     iconType: props.iconType,
     iconSide: props.iconSide,
     'data-test-subj': props.testId,
+    className: props.className,
   };
 
   const btn = props.emphasize ? (

src/plugins/vis_default_editor/public/default_editor_controller.tsx

@@ -69,7 +69,6 @@ class DefaultEditorController {
           ]
         : visType.editorConfig.optionTabs),
     ];
-
     this.state = {
       vis,
       optionTabs,

src/plugins/visualizations/public/embeddable/create_vis_embeddable_from_object.ts

@@ -41,7 +41,8 @@ export const createVisEmbeddableFromObject = (deps: VisualizeEmbeddableFactoryDe
   try {
     const visId = vis.id as string;
 
-    const editPath = visId ? savedVisualizations.urlFor(visId) : '';
+    const editPath = visId ? savedVisualizations.urlFor(visId) : '#/edit_by_value';
+
     const editUrl = visId
       ? getHttp().basePath.prepend(`/app/visualize${savedVisualizations.urlFor(visId)}`)
       : '';

src/plugins/visualize/config.ts

@@ -20,7 +20,7 @@
 import { schema, TypeOf } from '@kbn/config-schema';
 
 export const configSchema = schema.object({
-  showNewVisualizeFlow: schema.boolean({ defaultValue: false }),
+  showNewVisualizeFlow: schema.boolean({ defaultValue: true }),
 });
 
 export type ConfigSchema = TypeOf<typeof configSchema>;

src/plugins/visualize/public/application/app.tsx

@@ -24,7 +24,12 @@ import { Route, Switch, useLocation } from 'react-router-dom';
 import { syncQueryStateWithUrl } from '../../../data/public';
 import { useKibana } from '../../../kibana_react/public';
 import { VisualizeServices } from './types';
-import { VisualizeEditor, VisualizeListing, VisualizeNoMatch } from './components';
+import {
+  VisualizeEditor,
+  VisualizeListing,
+  VisualizeNoMatch,
+  VisualizeByValueEditor,
+} from './components';
 import { VisualizeConstants } from './visualize_constants';
 
 export const VisualizeApp = () => {
@@ -48,6 +53,9 @@ export const VisualizeApp = () => {
 
   return (
     <Switch>
+      <Route exact path={`${VisualizeConstants.EDIT_BY_VALUE_PATH}`}>
+        <VisualizeByValueEditor />
+      </Route>
       <Route path={[VisualizeConstants.CREATE_PATH, `${VisualizeConstants.EDIT_PATH}/:id`]}>
         <VisualizeEditor />
       </Route>

src/plugins/visualize/public/application/components/index.ts

@@ -20,3 +20,4 @@
 export { VisualizeListing } from './visualize_listing';
 export { VisualizeEditor } from './visualize_editor';
 export { VisualizeNoMatch } from './visualize_no_match';
+export { VisualizeByValueEditor } from './visualize_byvalue_editor';