Skip to content

[Dashboard] Fix explicitInput removal#215580

Merged
ThomThomson merged 6 commits intoelastic:mainfrom
ThomThomson:dashboard/fixExplicitInputRemoval
Mar 27, 2025
Merged

[Dashboard] Fix explicitInput removal#215580
ThomThomson merged 6 commits intoelastic:mainfrom
ThomThomson:dashboard/fixExplicitInputRemoval

Conversation

@ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Mar 21, 2025

Summary

Temporary fix for #215429 to unblock #197716 before #215551 can be completed.

Warning

This fix works, but it introduces a duplication of information that should be immediately removed as soon as #215551 is solved.

Explanation
This PR fixes #215429 by making the Dashboard's serialize function fall back to whatever state was last saved to the Dashboard for that panel when a child API cannot be found.

Unfortunately, when attempting to serialize a newly added panel it's possible that the child API cannot be found and the panel hasn't been saved before. In this edge case the explicitInput would still be missing. Because of this, this PR adds another state backup which stores the result of serializing every panel with unsaved changes. This way in cases where the panel is new and the API is missing we're still able to serialize it.

@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// loe:small Small Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:skip Skip the PR/issue when compiling release notes labels Mar 24, 2025
@ThomThomson ThomThomson marked this pull request as ready for review March 25, 2025 15:30
@ThomThomson ThomThomson requested a review from a team as a code owner March 25, 2025 15:30
@elasticmachine
Copy link
Contributor

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

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

const getApiMissingFallback = (id: string): SerializedPanelState<object> => {
// If the Dashboard has been saved with this panel before, we should be able to grab it from the current value of panels$
if (Object.keys(panels$.value[id].explicitInput ?? {}).length > 0) {
return {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a panel is not new so explicitInput is populated but has unsaved changes - Shouldn't unsaved changes version of explicitInput be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think you're right. We could confirm this by making a change to a panel, minimizing it with a collapsible section and then saving the Dashboard.

That change would be overwritten. It seems to me that we should only use the serializedPanelBackup here. I'll make that change.

const serializeResult = apiHasSerializableState(childApi)
? childApi.serializeState()
: { rawState: {} };
: getDashboardBackupService().getSerializedPanelBackup(id, dashboardId) ?? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we need to fallback to panels$.value[id].explicitInput but check for getSerializedPanelBackup first.

const serializeResult = apiHasSerializableState(childApi)
  ? childApi.serializeState()
  : getDashboardBackupService().getSerializedPanelBackup(id, dashboardId) ?? {
      rawState: panels$.value[id].explicitInput ?? {},
      references: getReferencesForPanelId(id)
    };

Here is a use case. A dashboard is opened with a collapsable panel section that is closed on load and never opened. The dashboard title is changed and the dashboard is saved.

For embeddables in unopened collapsable section:

  • childApi will not be provided because the embeddable was never rendered
  • getSerializedPanelBackup will not be provided because the embeddable was never rendered and no unsaved changes where recorded. We can not count on the session state from when the panel was added because this dashboard may be opened on a different session.
  • fallback to panels$.value[id].explicitInput

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. You're right again. Thanks Nathan. It's obvious I'm not using my whole brain power on this PR - the situation you describe was the one I had in mind when I made the first function.

This is another reason why this code needs to be removed ASAP. Implemented your suggestion in
d18d50e, thanks again!

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This is a good work around until the larger problem can be addressed.

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #19 / useBulkFetchFleetIntegrationPolicies() should call the correct fleet api with the query data provided

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 542.9KB 544.0KB +1.1KB

History

@ThomThomson ThomThomson merged commit 62f2daf into elastic:main Mar 27, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.17, 8.18, 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/14115648062

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 27, 2025
Temporarily adds a serializedStateBackup to Dashboard

(cherry picked from commit 62f2daf)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.16 Backport failed because of merge conflicts
8.17 Backport failed because of merge conflicts
8.18 Backport failed because of merge conflicts
8.x
9.0 Backport failed because of merge conflicts

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 215580

Questions ?

Please refer to the Backport tool documentation

@ThomThomson ThomThomson added backport:version Backport to applied version labels and removed backport:prev-major labels Mar 27, 2025
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/14115827016

@kibanamachine
Copy link
Contributor

💔 Backport failed

The pull request could not be backported due to the following error:
Git clone failed with exit code: 128

Manual backport

To create the backport manually run:

node scripts/backport --pr 215580

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 28, 2025
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Mar 31, 2025
Temporarily adds a serializedStateBackup to Dashboard
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Apr 3, 2025
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Dashboard] Fix explicitInput removal
(#215580)](#215580)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Devon
Thomson","email":"devon.thomson@elastic.co"},"sourceCommit":{"committedDate":"2025-03-27T20:02:22Z","message":"[Dashboard]
Fix explicitInput removal (#215580)\n\nTemporarily adds a
serializedStateBackup to
Dashboard","sha":"62f2daf32e07a8aace4648578641f9965436c424","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","loe:small","release_note:skip","impact:high","backport:prev-minor","backport:prev-major","v9.1.0"],"title":"[Dashboard]
Fix explicitInput
removal","number":215580,"url":"https://github.com/elastic/kibana/pull/215580","mergeCommit":{"message":"[Dashboard]
Fix explicitInput removal (#215580)\n\nTemporarily adds a
serializedStateBackup to
Dashboard","sha":"62f2daf32e07a8aace4648578641f9965436c424"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/215580","number":215580,"mergeCommit":{"message":"[Dashboard]
Fix explicitInput removal (#215580)\n\nTemporarily adds a
serializedStateBackup to
Dashboard","sha":"62f2daf32e07a8aace4648578641f9965436c424"}}]}]
BACKPORT-->

Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Dashboard] Missing child APIs cause explicitInput removal

4 participants