Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Embeddable] Reset dashboard breaks panel state when ran after editor edits #201627

Open
dej611 opened this issue Nov 25, 2024 · 4 comments · Fixed by #201687
Open

[Embeddable] Reset dashboard breaks panel state when ran after editor edits #201627

dej611 opened this issue Nov 25, 2024 · 4 comments · Fixed by #201687
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Embeddables Relating to the Embeddable system regression Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@dej611
Copy link
Contributor

dej611 commented Nov 25, 2024

Describe the bug:

The bug seems to affect any embeddable who not rely on inline editing state.
The main problem is relying on the fact that the initial state necessarily starts from a deserializedState in a dashboard, which is not true, as a dashboard can be loaded AFTER passing thru an editor edit session.
Saving this broken state wipes out entirely the panel state and breaks the dashboard.

Steps to reproduce:

  1. Create a Visualize panel (or Maps)
  2. Once done with the edit hit Save and return
  3. Now save the dashboard
  4. Edit the panel again and add something to it
  5. Hit Save and return again
  6. In the dashboard click Reset now
  7. Look in the dev console to see some error about initialization of the panel (specific per viz)

Any additional context:

The problem seems related to the fact that navigating back from an editor doesn't pass thru the deserializeState function, but assign an empty object:

const lastSavedRuntimeState = serializedState
? await factory.deserializeState(serializedState)
: ({} as RuntimeState);

This lastSavedRuntimeState is then passed to the initializeUnsavedChanges function who is in charge to enrich the specific embeddable API with additional methods to manage the dashboard Reset feature:

const unsavedChanges = initializeUnsavedChanges<RuntimeState>(
lastSavedRuntimeState,
parentApi,
comparators
);

Given the empty object passed when coming from a full editor any state within a panel will get wiped out.
Also it is not possible to workaround it as specific embeddable api will be overriden by the initializeUnsavedChanges anyway.

@dej611 dej611 added bug Fixes for quality problems that affect the customer experience Feature:Embeddables Relating to the Embeddable system impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Nov 25, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese nreese added regression and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Nov 25, 2024
@nreese
Copy link
Contributor

nreese commented Nov 25, 2024

regression caused by #188039. Explicit input is getting overridden.

      if (embeddableService.reactEmbeddableRegistryHasKey(incomingEmbeddable.type)) {
        panelToUpdate.explicitInput = { id: panelToUpdate.explicitInput.id };
        runtimePanelsToRestore[incomingEmbeddable.embeddableId] = nextRuntimeState;
      }

Instead, only runtimePanelsToRestore should be set for the panel and panelToUpdate.explicitInput should remain the same.

@nreese nreese self-assigned this Nov 25, 2024
@nreese nreese closed this as completed in 289bb16 Nov 27, 2024
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Nov 27, 2024
…tor edits (elastic#201687)

Closes elastic#201627

### The problem
`ReactEmbeddableRender` uses `lastSavedRuntimeState` as last saved state
baseline. Resetting reverts to this state.
```
// Code sample from src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.tsx
const serializedState = parentApi.getSerializedStateForChild(uuid);
const lastSavedRuntimeState = serializedState
  ? await factory.deserializeState(serializedState)
  : ({} as RuntimeState);

...

const unsavedChanges = initializeUnsavedChanges<RuntimeState>(
  lastSavedRuntimeState,
  parentApi,
  comparators
);
```

`DashboardContainer` getSerializedStateForChild implemenation
```
// Code sample from src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx
public getSerializedStateForChild = (childId: string) => {
    const rawState = this.getInput().panels[childId].explicitInput;
    const { id, ...serializedState } = rawState;
    if (!rawState || Object.keys(serializedState).length === 0) return;
    const references = getReferencesForPanelId(childId, this.savedObjectReferences);
    return {
      rawState,
      // references from old installations may not be prefixed with panel id
      // fall back to passing all references in these cases to preserve backwards compatability
      references: references.length > 0 ? references : this.savedObjectReferences,
    };
  };
```

The problem is that `create_dashboard` clears
`this.getInput().panels[childId].explicitInput` for a panel with
embeddable transfer state. This causes `lastSavedRuntimeState` to be an
empty object, which causes reset to use `undefined` for the previous
value when reseting an embeddables keys.
```
// Code sample from src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts
const incomingEmbeddable = creationOptions?.getIncomingEmbeddable?.();
if (
  incomingEmbeddable.embeddableId &&
  Boolean(initialDashboardInput.panels[incomingEmbeddable.embeddableId])
) {
  // this embeddable already exists, we will update the explicit input.
  const panelToUpdate = initialDashboardInput.panels[incomingEmbeddable.embeddableId];
  const sameType = panelToUpdate.type === incomingEmbeddable.type;

  panelToUpdate.type = incomingEmbeddable.type;
  const nextRuntimeState = {
    // if the incoming panel is the same type as what was there before we can safely spread the old panel's explicit input
    ...(sameType ? panelToUpdate.explicitInput : {}),

    ...incomingEmbeddable.input,
    id: incomingEmbeddable.embeddableId,

    // maintain hide panel titles setting.
    hidePanelTitles: panelToUpdate.explicitInput.hidePanelTitles,
  };
  if (embeddableService.reactEmbeddableRegistryHasKey(incomingEmbeddable.type)) {
    panelToUpdate.explicitInput = { id: panelToUpdate.explicitInput.id };
    runtimePanelsToRestore[incomingEmbeddable.embeddableId] = nextRuntimeState;
  } else {
    panelToUpdate.explicitInput = nextRuntimeState;
  }
}
```

### The fix
The solution is to retain `panelToUpdate.explicitInput` original value
so that `lastSavedRuntimeState` is set to the original runtime state and
reset will revert to this value instead of `{}`.

### test instructions
1) install web logs sample data
2) create new dashboard
3) click "Add panel" and select "Legacy => Agg based"
4) create pie chart with terms split on "machine.os", click "Save and
return"
5) save dashboard
6) edit pie chart, Under "options", unselect "donut". Click "Save and
return"
7) click reset in dashboard. Verify that visualization returns to
original state. Note, the same will not happen for map embeddables.
There is a bug in map embeddables where resetting "attributes" state
does not update the current map.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit 289bb16)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Nov 27, 2024
…tor edits (elastic#201687)

Closes elastic#201627

### The problem
`ReactEmbeddableRender` uses `lastSavedRuntimeState` as last saved state
baseline. Resetting reverts to this state.
```
// Code sample from src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.tsx
const serializedState = parentApi.getSerializedStateForChild(uuid);
const lastSavedRuntimeState = serializedState
  ? await factory.deserializeState(serializedState)
  : ({} as RuntimeState);

...

const unsavedChanges = initializeUnsavedChanges<RuntimeState>(
  lastSavedRuntimeState,
  parentApi,
  comparators
);
```

`DashboardContainer` getSerializedStateForChild implemenation
```
// Code sample from src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx
public getSerializedStateForChild = (childId: string) => {
    const rawState = this.getInput().panels[childId].explicitInput;
    const { id, ...serializedState } = rawState;
    if (!rawState || Object.keys(serializedState).length === 0) return;
    const references = getReferencesForPanelId(childId, this.savedObjectReferences);
    return {
      rawState,
      // references from old installations may not be prefixed with panel id
      // fall back to passing all references in these cases to preserve backwards compatability
      references: references.length > 0 ? references : this.savedObjectReferences,
    };
  };
```

The problem is that `create_dashboard` clears
`this.getInput().panels[childId].explicitInput` for a panel with
embeddable transfer state. This causes `lastSavedRuntimeState` to be an
empty object, which causes reset to use `undefined` for the previous
value when reseting an embeddables keys.
```
// Code sample from src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts
const incomingEmbeddable = creationOptions?.getIncomingEmbeddable?.();
if (
  incomingEmbeddable.embeddableId &&
  Boolean(initialDashboardInput.panels[incomingEmbeddable.embeddableId])
) {
  // this embeddable already exists, we will update the explicit input.
  const panelToUpdate = initialDashboardInput.panels[incomingEmbeddable.embeddableId];
  const sameType = panelToUpdate.type === incomingEmbeddable.type;

  panelToUpdate.type = incomingEmbeddable.type;
  const nextRuntimeState = {
    // if the incoming panel is the same type as what was there before we can safely spread the old panel's explicit input
    ...(sameType ? panelToUpdate.explicitInput : {}),

    ...incomingEmbeddable.input,
    id: incomingEmbeddable.embeddableId,

    // maintain hide panel titles setting.
    hidePanelTitles: panelToUpdate.explicitInput.hidePanelTitles,
  };
  if (embeddableService.reactEmbeddableRegistryHasKey(incomingEmbeddable.type)) {
    panelToUpdate.explicitInput = { id: panelToUpdate.explicitInput.id };
    runtimePanelsToRestore[incomingEmbeddable.embeddableId] = nextRuntimeState;
  } else {
    panelToUpdate.explicitInput = nextRuntimeState;
  }
}
```

### The fix
The solution is to retain `panelToUpdate.explicitInput` original value
so that `lastSavedRuntimeState` is set to the original runtime state and
reset will revert to this value instead of `{}`.

### test instructions
1) install web logs sample data
2) create new dashboard
3) click "Add panel" and select "Legacy => Agg based"
4) create pie chart with terms split on "machine.os", click "Save and
return"
5) save dashboard
6) edit pie chart, Under "options", unselect "donut". Click "Save and
return"
7) click reset in dashboard. Verify that visualization returns to
original state. Note, the same will not happen for map embeddables.
There is a bug in map embeddables where resetting "attributes" state
does not update the current map.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit 289bb16)
@nreese nreese reopened this Nov 27, 2024
@nreese
Copy link
Contributor

nreese commented Nov 27, 2024

#201902 reverted so issue is still open

@nreese
Copy link
Contributor

nreese commented Nov 27, 2024

Why was revert required?

  1. Fix branch opened, CI returned green
  2. Lens embeddable refactor merged, converting lens embeddable from legacy system to react system
  3. Fix PR merged - without rebase to capture lens embeddable refactor.
  4. In main tests started to fail around converting by-value TSVB panel to lens embeddable. Test failures caused by Fix PR changes not getting run on react lens embeddable.

Why did tests start failing

Without the fix, dashboard panel state is getting cleared. This is the source of the original issue; that resetting causes the embeddable to reset itself against empty state. Lens embeddable is only getting state from embeddable transfer service that looks like

{
  attributes: {
    title: 'My TSVB to Lens viz 2 (converted)'
  }
}

With this state, the panel title gets set to 'My TSVB to Lens viz 2 (converted)' from the by-ref lens saved object.

With the fix, dashboard panel state is not getting cleared. This means that lens embeddable is getting original dashboard panel state plus state from embeddable transfer and state looks like

{
  attributes: {
    title: 'My TSVB to Lens viz 2 (converted)'
  },
  title: 'My TSVB to Lens viz 2'
}

With this state, the panel title gets set to 'My TSVB to Lens viz 2' from the previous dashboard panel state.

What is the problem

The root of the problem is that there are 2 versions of serialize state

  1. the last saved serialized state
  2. the current serialized state

In most cases these are the same values, but for panels with incoming state from embeddable service, these are separate values. The current logic of ReactEmbeddableRender only supports a single value when 2 values are needed for this use case.

nreese added a commit that referenced this issue Dec 10, 2024
…#203158)

Part of #201627

This is a short term fix for serverless and 8.x branches (long term fix
is an architectural change that will only be merged into 9.0 and 8.18
branches).

Fix prevents users from reseting a panel edited via embeddable transfer
service. This prevents panel from getting into an invalid state.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Dec 10, 2024
…elastic#203158)

Part of elastic#201627

This is a short term fix for serverless and 8.x branches (long term fix
is an architectural change that will only be merged into 9.0 and 8.18
branches).

Fix prevents users from reseting a panel edited via embeddable transfer
service. This prevents panel from getting into an invalid state.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit e103a25)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Dec 10, 2024
…elastic#203158)

Part of elastic#201627

This is a short term fix for serverless and 8.x branches (long term fix
is an architectural change that will only be merged into 9.0 and 8.18
branches).

Fix prevents users from reseting a panel edited via embeddable transfer
service. This prevents panel from getting into an invalid state.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit e103a25)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Dec 10, 2024
…elastic#203158)

Part of elastic#201627

This is a short term fix for serverless and 8.x branches (long term fix
is an architectural change that will only be merged into 9.0 and 8.18
branches).

Fix prevents users from reseting a panel edited via embeddable transfer
service. This prevents panel from getting into an invalid state.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit e103a25)
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this issue Dec 12, 2024
…tor edits (elastic#201687)

Closes elastic#201627

### The problem
`ReactEmbeddableRender` uses `lastSavedRuntimeState` as last saved state
baseline. Resetting reverts to this state.
```
// Code sample from src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.tsx
const serializedState = parentApi.getSerializedStateForChild(uuid);
const lastSavedRuntimeState = serializedState
  ? await factory.deserializeState(serializedState)
  : ({} as RuntimeState);

...

const unsavedChanges = initializeUnsavedChanges<RuntimeState>(
  lastSavedRuntimeState,
  parentApi,
  comparators
);
```

`DashboardContainer` getSerializedStateForChild implemenation
```
// Code sample from src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx
public getSerializedStateForChild = (childId: string) => {
    const rawState = this.getInput().panels[childId].explicitInput;
    const { id, ...serializedState } = rawState;
    if (!rawState || Object.keys(serializedState).length === 0) return;
    const references = getReferencesForPanelId(childId, this.savedObjectReferences);
    return {
      rawState,
      // references from old installations may not be prefixed with panel id
      // fall back to passing all references in these cases to preserve backwards compatability
      references: references.length > 0 ? references : this.savedObjectReferences,
    };
  };
```

The problem is that `create_dashboard` clears
`this.getInput().panels[childId].explicitInput` for a panel with
embeddable transfer state. This causes `lastSavedRuntimeState` to be an
empty object, which causes reset to use `undefined` for the previous
value when reseting an embeddables keys.
```
// Code sample from src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts
const incomingEmbeddable = creationOptions?.getIncomingEmbeddable?.();
if (
  incomingEmbeddable.embeddableId &&
  Boolean(initialDashboardInput.panels[incomingEmbeddable.embeddableId])
) {
  // this embeddable already exists, we will update the explicit input.
  const panelToUpdate = initialDashboardInput.panels[incomingEmbeddable.embeddableId];
  const sameType = panelToUpdate.type === incomingEmbeddable.type;

  panelToUpdate.type = incomingEmbeddable.type;
  const nextRuntimeState = {
    // if the incoming panel is the same type as what was there before we can safely spread the old panel's explicit input
    ...(sameType ? panelToUpdate.explicitInput : {}),

    ...incomingEmbeddable.input,
    id: incomingEmbeddable.embeddableId,

    // maintain hide panel titles setting.
    hidePanelTitles: panelToUpdate.explicitInput.hidePanelTitles,
  };
  if (embeddableService.reactEmbeddableRegistryHasKey(incomingEmbeddable.type)) {
    panelToUpdate.explicitInput = { id: panelToUpdate.explicitInput.id };
    runtimePanelsToRestore[incomingEmbeddable.embeddableId] = nextRuntimeState;
  } else {
    panelToUpdate.explicitInput = nextRuntimeState;
  }
}
```

### The fix
The solution is to retain `panelToUpdate.explicitInput` original value
so that `lastSavedRuntimeState` is set to the original runtime state and
reset will revert to this value instead of `{}`.

### test instructions
1) install web logs sample data
2) create new dashboard
3) click "Add panel" and select "Legacy => Agg based"
4) create pie chart with terms split on "machine.os", click "Save and
return"
5) save dashboard
6) edit pie chart, Under "options", unselect "donut". Click "Save and
return"
7) click reset in dashboard. Verify that visualization returns to
original state. Note, the same will not happen for map embeddables.
There is a bug in map embeddables where resetting "attributes" state
does not update the current map.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this issue Dec 12, 2024
…elastic#203158)

Part of elastic#201627

This is a short term fix for serverless and 8.x branches (long term fix
is an architectural change that will only be merged into 9.0 and 8.18
branches).

Fix prevents users from reseting a panel edited via embeddable transfer
service. This prevents panel from getting into an invalid state.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Embeddables Relating to the Embeddable system regression Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants