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

[dashboard] Do not reset panel to undefined or empty last saved state #203158

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Dec 5, 2024

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.

@nreese
Copy link
Contributor Author

nreese commented Dec 5, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 5, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 5, 2024

/ci

@nreese nreese marked this pull request as ready for review December 6, 2024 00:32
@nreese nreese requested a review from a team as a code owner December 6, 2024 00:32
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v9.0.0 backport:version Backport to applied version labels v8.18.0 v8.16.2 v8.15.6 v8.17.1 labels Dec 6, 2024
@elasticmachine
Copy link
Contributor

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

@nreese
Copy link
Contributor Author

nreese commented Dec 9, 2024

@elasticmachine merge upstream

@ThomThomson ThomThomson self-requested a review December 9, 2024 17:37
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Changes LGTM as a temporary fix. It's great that the user cannot get into a broken state anymore, but we do still make it very difficult to get the last saved state back unfortunately. Left a few questions but nothing blocking. Tested locally in chrome.

@@ -11,7 +11,7 @@ import { PublishingSubject } from '../publishing_subject';

export interface PublishesUnsavedChanges<Runtime extends object = object> {
unsavedChanges: PublishingSubject<Partial<Runtime> | undefined>;
resetUnsavedChanges: () => void;
resetUnsavedChanges: () => boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should remove this typing after the architectural fix is in? I think this should be temporary because in theory, the reset function shouldn't be capable of failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. This typing fix is only temporary.

if (apiPublishesUnsavedChanges(child)) {
const success = child.resetUnsavedChanges();
if (!success) {
coreServices.notifications.toasts.addWarning(
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to have a warning in this case. I wonder if we should clarify that the user needs to start a new session (open a new tab etc) to get their last saved state back?

Copy link
Contributor Author

@nreese nreese Dec 9, 2024

Choose a reason for hiding this comment

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

I did not want to complicate the error message. Its complex because if its a by-ref panel then a new session will not revert the changes because the by-ref saved object has already been saved

Copy link
Contributor

Choose a reason for hiding this comment

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

because the by-ref saved object has already been saved

Yet another example of why we need to fix this architecturally, as save and return on a by reference panel shouldn't even show unsaved changes on the Dashboard.

++ to not complicating the error message. Ideally this isn't seen by users at all.

@nreese
Copy link
Contributor Author

nreese commented Dec 9, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Dec 9, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Dec 9, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

There are no new commits on the base branch.

@nreese
Copy link
Contributor Author

nreese commented Dec 9, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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
aiops 614.8KB 615.0KB +189.0B
controls 494.0KB 494.3KB +252.0B
dashboard 684.4KB 684.5KB +136.0B
imageEmbeddable 69.9KB 70.0KB +63.0B
ml 4.7MB 4.7MB +252.0B
total +892.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
embeddable 73.2KB 73.2KB +63.0B

History

@nreese nreese merged commit e103a25 into elastic:main Dec 10, 2024
8 checks passed
@kibanamachine
Copy link
Contributor

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

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request 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 <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit e103a25)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request 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 <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit e103a25)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request 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 <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit e103a25)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

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

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 203158

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 10, 2024
…d state (#203158) (#203511)

# Backport

This will backport the following commits from `main` to `8.17`:
- [[dashboard] Do not reset panel to undefined or empty last saved state
(#203158)](#203158)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-10T03:04:15Z","message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","v9.0.0","project:embeddableRebuild","backport:version","v8.18.0","v8.16.2","v8.15.6","v8.17.1"],"title":"[dashboard]
Do not reset panel to undefined or empty last saved
state","number":203158,"url":"https://github.com/elastic/kibana/pull/203158","mergeCommit":{"message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16","8.15","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203158","number":203158,"mergeCommit":{"message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.2","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.15","label":"v8.15.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <[email protected]>
kibanamachine added a commit that referenced this pull request Dec 10, 2024
… state (#203158) (#203512)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[dashboard] Do not reset panel to undefined or empty last saved state
(#203158)](#203158)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-10T03:04:15Z","message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","v9.0.0","project:embeddableRebuild","backport:version","v8.18.0","v8.16.2","v8.15.6","v8.17.1"],"title":"[dashboard]
Do not reset panel to undefined or empty last saved
state","number":203158,"url":"https://github.com/elastic/kibana/pull/203158","mergeCommit":{"message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16","8.15","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203158","number":203158,"mergeCommit":{"message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.2","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.15","label":"v8.15.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <[email protected]>
kibanamachine added a commit that referenced this pull request Dec 10, 2024
…d state (#203158) (#203510)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[dashboard] Do not reset panel to undefined or empty last saved state
(#203158)](#203158)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-10T03:04:15Z","message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","v9.0.0","project:embeddableRebuild","backport:version","v8.18.0","v8.16.2","v8.15.6","v8.17.1"],"title":"[dashboard]
Do not reset panel to undefined or empty last saved
state","number":203158,"url":"https://github.com/elastic/kibana/pull/203158","mergeCommit":{"message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16","8.15","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203158","number":203158,"mergeCommit":{"message":"[dashboard]
Do not reset panel to undefined or empty last saved state
(#203158)\n\nPart of
https://github.com/elastic/kibana/issues/201627\r\n\r\nThis is a short
term fix for serverless and 8.x branches (long term fix\r\nis an
architectural change that will only be merged into 9.0 and
8.18\r\nbranches).\r\n\r\nFix prevents users from reseting a panel
edited via embeddable transfer\r\nservice. This prevents panel from
getting into an invalid state.\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e103a253d9b756605dbeb92955ff517597055970"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.2","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.15","label":"v8.15.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <[email protected]>
@nreese nreese removed the v8.15.6 label Dec 11, 2024
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request 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 <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
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 project:embeddableRebuild release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.2 v8.17.0 v8.17.1 v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants