From 0bf02a3bdd62b37ece43cc40a3aaa7bd6b5f5388 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 16 Jul 2020 11:32:00 +0200 Subject: [PATCH 01/12] :poop: Draft code to debug the issue --- .../embeddable/dashboard_container.tsx | 56 +++++++++++++------ .../public/lib/containers/container.ts | 3 + .../public/lib/panel/embeddable_panel.tsx | 12 +++- .../actions/panel_notifications_action.ts | 2 + 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index ff74580ba256be..867588098c7622 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -171,24 +171,44 @@ export class DashboardContainer extends Container { + // console.log('Pre', embeddable, { + // ...previousPanelState.explicitInput, + // ...newPanelState, + // }); + const changes = { + panels: { + [previousPanelState.explicitInput.id]: { + type: newPanelState.type, + explicitInput: { ...previousPanelState.explicitInput, ...newPanelState.explicitInput }, + }, + }, + }; + // console.log({ changes }); + embeddable.updateInput(changes); + // console.log('Post', embeddable, { + // ...previousPanelState.explicitInput, + // ...newPanelState, + // }); }); } diff --git a/src/plugins/embeddable/public/lib/containers/container.ts b/src/plugins/embeddable/public/lib/containers/container.ts index 8cf3d1a15883b5..4fa934c6f9087f 100644 --- a/src/plugins/embeddable/public/lib/containers/container.ts +++ b/src/plugins/embeddable/public/lib/containers/container.ts @@ -211,6 +211,9 @@ export abstract class Container< throw new PanelNotFoundError(); } const panelState: PanelState = this.input.panels[embeddableId]; + if (!panelState.explicitInput) { + // console.log('No data in here!', embeddableId, panelState); + } return panelState as PanelState; } diff --git a/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx b/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx index cb02ffc470e95c..7a937935ee1406 100644 --- a/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx +++ b/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx @@ -125,9 +125,15 @@ export class EmbeddablePanel extends React.Component { } private async refreshNotifications() { - let notifications = await this.props.getActions(PANEL_NOTIFICATION_TRIGGER, { - embeddable: this.props.embeddable, - }); + let notifications; + // TODO: remove this try/catch + try { + notifications = await this.props.getActions(PANEL_NOTIFICATION_TRIGGER, { + embeddable: this.props.embeddable, + }); + } catch (e) { + return; + } if (!this.mounted) return; const { disabledActions } = this.props.embeddable.getInput(); diff --git a/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts b/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts index 165ce24c13ea3d..69658c99da545f 100644 --- a/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts +++ b/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts @@ -48,6 +48,8 @@ export class PanelNotificationsAction implements ActionDefinition { if (embeddable.getInput().viewMode !== ViewMode.EDIT) return false; + // TODO: remove this later on + if (!embeddable.enhancements) return false; return this.getEventCount(embeddable) > 0; }; From 66a1973868ca3fbd2bc170262f5cab28f32c1f76 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 3 Aug 2020 12:11:57 +0200 Subject: [PATCH 02/12] :recycle: Revert changes shape --- .../embeddable/dashboard_container.tsx | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index 867588098c7622..e3eb1086552de4 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -195,14 +195,16 @@ export class DashboardContainer extends Container Date: Tue, 4 Aug 2020 10:50:13 +0200 Subject: [PATCH 03/12] :bug: Fix the cloning issue for the placeholder and final embeddable state --- .../embeddable/dashboard_container.tsx | 61 ++++++------------- .../embeddable/grid/dashboard_grid.tsx | 2 + .../public/lib/containers/container.ts | 34 ++++++++--- 3 files changed, 44 insertions(+), 53 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index e3eb1086552de4..9f572745340576 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -168,49 +168,24 @@ export class DashboardContainer extends Container, newPanelState: Partial ) { - // 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 || previousPanelState.explicitInput.id; - // this.input.panels[newPanelId] = { - // ...previousPanelState, - // ...newPanelState, - // gridData: { - // ...previousPanelState.gridData, - // i: newPanelId, - // }, - // explicitInput: { - // ...newPanelState.explicitInput, - // id: newPanelId, - // }, - // }; - // this.updateInput({ - // panels: this.input.panels, - // lastReloadRequestTime: Date.now(), - // }); - this.untilEmbeddableLoaded(previousPanelState.explicitInput.id).then((embeddable) => { - // console.log('Pre', embeddable, { - // ...previousPanelState.explicitInput, - // ...newPanelState, - // }); - // const changes = { - // panels: { - // [previousPanelState.explicitInput.id]: { - // type: newPanelState.type, - // explicitInput: { ...previousPanelState.explicitInput, ...newPanelState.explicitInput }, - // }, - // }, - // }; - // explicitInput content here: - const changes = { ...previousPanelState.explicitInput, ...newPanelState.explicitInput }; - // console.log({ changes }); - embeddable.updateInput(changes); - // console.log('Post', embeddable, { - // ...previousPanelState.explicitInput, - // ...newPanelState, - // }); + this.untilEmbeddableLoaded(previousPanelState.explicitInput.id).then(() => { + // 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, + }, + }, + }, + }); }); } diff --git a/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx b/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx index dcd07fe394c7d1..4bc5260c8af722 100644 --- a/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx +++ b/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx @@ -270,6 +270,8 @@ class DashboardGridUi extends React.Component { }} > 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( @@ -331,16 +337,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]); + } } }); } From c01d56b82174d3ec5706a029bb3d90fb404fb522 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 4 Aug 2020 11:14:04 +0200 Subject: [PATCH 04/12] :fire: Remove double loading state handler --- .../embeddable/dashboard_container.tsx | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index 9f572745340576..6955e76206932a 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -168,24 +168,22 @@ export class DashboardContainer extends Container, newPanelState: Partial ) { - this.untilEmbeddableLoaded(previousPanelState.explicitInput.id).then(() => { - // 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, - }, + // 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, }, }, - }); + }, }); } From d5025d49af04096db295915f189f8c029aecacfd Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 4 Aug 2020 11:28:52 +0200 Subject: [PATCH 05/12] :recycle: Limit the key to the type --- .../public/application/embeddable/grid/dashboard_grid.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx b/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx index 4bc5260c8af722..2b3f8cebbc80db 100644 --- a/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx +++ b/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx @@ -263,6 +263,7 @@ class DashboardGridUi extends React.Component {
{ @@ -270,8 +271,8 @@ class DashboardGridUi extends React.Component { }} > Date: Tue, 4 Aug 2020 11:33:01 +0200 Subject: [PATCH 06/12] :mute: Remove debugging logs --- .../embeddable/public/lib/containers/container.ts | 3 --- .../embeddable/public/lib/panel/embeddable_panel.tsx | 12 +++--------- .../public/actions/panel_notifications_action.ts | 2 -- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/src/plugins/embeddable/public/lib/containers/container.ts b/src/plugins/embeddable/public/lib/containers/container.ts index b921ae88b3a78a..dfd9ea1b0adb98 100644 --- a/src/plugins/embeddable/public/lib/containers/container.ts +++ b/src/plugins/embeddable/public/lib/containers/container.ts @@ -217,9 +217,6 @@ export abstract class Container< throw new PanelNotFoundError(); } const panelState: PanelState = this.input.panels[embeddableId]; - if (!panelState.explicitInput) { - // console.log('No data in here!', embeddableId, panelState); - } return panelState as PanelState; } diff --git a/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx b/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx index 7a937935ee1406..cb02ffc470e95c 100644 --- a/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx +++ b/src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx @@ -125,15 +125,9 @@ export class EmbeddablePanel extends React.Component { } private async refreshNotifications() { - let notifications; - // TODO: remove this try/catch - try { - notifications = await this.props.getActions(PANEL_NOTIFICATION_TRIGGER, { - embeddable: this.props.embeddable, - }); - } catch (e) { - return; - } + let notifications = await this.props.getActions(PANEL_NOTIFICATION_TRIGGER, { + embeddable: this.props.embeddable, + }); if (!this.mounted) return; const { disabledActions } = this.props.embeddable.getInput(); diff --git a/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts b/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts index 69658c99da545f..165ce24c13ea3d 100644 --- a/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts +++ b/x-pack/plugins/embeddable_enhanced/public/actions/panel_notifications_action.ts @@ -48,8 +48,6 @@ export class PanelNotificationsAction implements ActionDefinition { if (embeddable.getInput().viewMode !== ViewMode.EDIT) return false; - // TODO: remove this later on - if (!embeddable.enhancements) return false; return this.getEventCount(embeddable) > 0; }; From 02265a42cb97061a7088db9befb10cf27285e01d Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 5 Aug 2020 12:24:24 +0200 Subject: [PATCH 07/12] :recycle: Refactor addOrUpdateEmbeddable method --- .../public/application/embeddable/dashboard_container.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index 6955e76206932a..eec82431f9e156 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -193,16 +193,16 @@ export class DashboardContainer extends Container = IEmbeddable >(type: string, explicitInput: Partial) { if (explicitInput.id && this.input.panels[explicitInput.id]) { - this.replacePanel(this.input.panels[explicitInput.id], { + return this.replacePanel(this.input.panels[explicitInput.id], { type, explicitInput: { ...explicitInput, - id: uuid.v4(), + // TS does not catch up with the typeguard above, so it needs to be explicit + id: explicitInput.id, }, }); - } else { - this.addNewEmbeddable(type, explicitInput); } + return this.addNewEmbeddable(type, explicitInput); } public render(dom: HTMLElement) { From 38aa24bd87e64b32bf4286c1ac8fcf3c91cc1d25 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 5 Aug 2020 12:25:05 +0200 Subject: [PATCH 08/12] :white_check_mark: Add replacePanel test --- .../embeddable/dashboard_container.test.tsx | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx index 3cebe2b8471550..5c3a5361028be8 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx @@ -29,9 +29,11 @@ import { ContactCardEmbeddableInput, ContactCardEmbeddable, ContactCardEmbeddableOutput, + EMPTY_EMBEDDABLE, } from '../../embeddable_plugin_test_samples'; // eslint-disable-next-line import { embeddablePluginMock } from 'src/plugins/embeddable/public/mocks'; +import { ErrorEmbeddable } from 'src/plugins/embeddable/public'; const options: DashboardContainerOptions = { application: {} as any, @@ -103,6 +105,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({ + 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: { From 46d7b634283056a469899906ba21a1cdf109a7d9 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 5 Aug 2020 12:31:40 +0200 Subject: [PATCH 09/12] :rotating_light: Fix unused import --- .../public/application/embeddable/dashboard_container.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx index 5c3a5361028be8..1b4947d26ab8cb 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.test.tsx @@ -33,7 +33,6 @@ import { } from '../../embeddable_plugin_test_samples'; // eslint-disable-next-line import { embeddablePluginMock } from 'src/plugins/embeddable/public/mocks'; -import { ErrorEmbeddable } from 'src/plugins/embeddable/public'; const options: DashboardContainerOptions = { application: {} as any, From 01ac18716de868568d8e99a096a92228451ad8b7 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 3 Nov 2020 14:07:24 +0100 Subject: [PATCH 10/12] :bug: Reuse same id when cloning --- .../embeddable/dashboard_container.tsx | 59 +++++++++++-------- .../embeddable/grid/dashboard_grid.tsx | 1 + .../public/lib/containers/container.ts | 32 +++++++--- 3 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index 757488185fe8e5..b6a571601e575c 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -169,27 +169,40 @@ export class DashboardContainer extends Container, newPanelState: Partial ) { - // 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, - }, - }; + // // 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, + // }, + // }; this.updateInput({ - panels: finalPanels, + panels: { + ...this.input.panels, + [previousPanelState.explicitInput.id]: { + ...previousPanelState, + ...newPanelState, + gridData: { + ...previousPanelState.gridData, + }, + explicitInput: { + ...newPanelState.explicitInput, + id: previousPanelState.explicitInput.id, + }, + }, + }, lastReloadRequestTime: new Date().getTime(), }); } @@ -201,15 +214,15 @@ export class DashboardContainer extends Container(type: string, explicitInput: Partial, 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(type, explicitInput); + return this.addNewEmbeddable(type, explicitInput); } } diff --git a/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx b/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx index d4d8fd0a4374b9..3d4ee8ded690c3 100644 --- a/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx +++ b/src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx @@ -272,6 +272,7 @@ class DashboardGridUi extends React.Component { }} > 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( @@ -329,17 +335,25 @@ 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 - ) { + if (currentPanels[id] !== undefined && this.output.embeddableLoaded[id] === undefined) { + return this.onPanelAdded(this.input.panels[id]); + } + if (currentPanels[id] === undefined && this.output.embeddableLoaded[id] !== undefined) { 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]); + } + } }); } } From 8fd7316a3de6be9f1390265aca120e7b67a16433 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Nov 2020 12:44:21 +0100 Subject: [PATCH 11/12] :bug: Revisit placeholder loading logic to handle different types of embeddables --- .../application/actions/clone_panel_action.test.tsx | 7 ++++++- .../application/embeddable/dashboard_container.tsx | 12 +++++++++--- .../embeddable/public/lib/containers/container.ts | 8 +++++++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/plugins/dashboard/public/application/actions/clone_panel_action.test.tsx b/src/plugins/dashboard/public/application/actions/clone_panel_action.test.tsx index b6bee5c3360b4c..e61cbd09d81d3d 100644 --- a/src/plugins/dashboard/public/application/actions/clone_panel_action.test.tsx +++ b/src/plugins/dashboard/public/application/actions/clone_panel_action.test.tsx @@ -98,7 +98,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 () => { diff --git a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx index c950e07655b994..051a7ef8bfb929 100644 --- a/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx @@ -154,15 +154,21 @@ export class DashboardContainer extends Container) => - 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) => + this.replacePanel(placeholderPanelState, newPanelState) + ); } public replacePanel( diff --git a/src/plugins/embeddable/public/lib/containers/container.ts b/src/plugins/embeddable/public/lib/containers/container.ts index e07e8693364c5a..4dede8bf5d752f 100644 --- a/src/plugins/embeddable/public/lib/containers/container.ts +++ b/src/plugins/embeddable/public/lib/containers/container.ts @@ -335,6 +335,12 @@ export abstract class Container< return embeddable; } + private panelHasChanged(currentPanel: PanelState, prevPanel: PanelState) { + if (currentPanel.type !== prevPanel.type) { + return true; + } + } + private maybeUpdateChildren( currentPanels: TContainerInput['panels'], prevPanels: TContainerInput['panels'] @@ -349,7 +355,7 @@ export abstract class Container< } // 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) { + if (this.panelHasChanged(currentPanels[id], prevPanels[id])) { this.onPanelRemoved(id); this.onPanelAdded(currentPanels[id]); } From 20808a73ecbc0688d7a5b3ece0ff63a06fdbfe19 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Nov 2020 12:46:30 +0100 Subject: [PATCH 12/12] :bug: Refactor test code to work with new replace logic --- .../actions/add_to_library_action.test.tsx | 21 ++++++------------- .../unlink_from_library_action.test.tsx | 20 +++++------------- 2 files changed, 11 insertions(+), 30 deletions(-) diff --git a/src/plugins/dashboard/public/application/actions/add_to_library_action.test.tsx b/src/plugins/dashboard/public/application/actions/add_to_library_action.test.tsx index 3f7d05e8692c2a..99e8e8d7db362a 100644 --- a/src/plugins/dashboard/public/application/actions/add_to_library_action.test.tsx +++ b/src/plugins/dashboard/public/application/actions/add_to_library_action.test.tsx @@ -124,19 +124,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); }); @@ -152,15 +148,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'); diff --git a/src/plugins/dashboard/public/application/actions/unlink_from_library_action.test.tsx b/src/plugins/dashboard/public/application/actions/unlink_from_library_action.test.tsx index 0f61a74cd7036d..844f412b224afb 100644 --- a/src/plugins/dashboard/public/application/actions/unlink_from_library_action.test.tsx +++ b/src/plugins/dashboard/public/application/actions/unlink_from_library_action.test.tsx @@ -118,19 +118,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); }); @@ -150,15 +145,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); });