[Controls Anywhere] Fix panels being backed up when controls have unsaved changes#248029
Conversation
b04c2b8 to
51c4dc2
Compare
51c4dc2 to
a87d9f5
Compare
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
src/platform/plugins/shared/dashboard/public/dashboard_api/layout_manager/are_layouts_equal.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Chatted offline about moving panels and pinnedPanels unsaved changes logic into layout manager. So instead of unsaved changes manager having to understand 2 different types of children, layoutManager should expose 2 observables hasPanelUnsavedChanges$ and hasPinnedPanelUnsavedChanges$. Then unsaved changes manager can just listen to these and populate panels when hasPanelUnsavedChanges$ is true and populate controlGroupInput when hasPinnedPanelUnsavedChanges$ is true. This will avoid leaking a bunch of layout internal details into unsaved changes manager.
…ed-changes-subset_2026-01-06
|
@nreese I have implemented your suggestion - it is much cleaner now IMO. Let me know what you think!! 🙇 |
src/platform/plugins/shared/dashboard/public/dashboard_api/layout_manager/layout_manager.ts
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/dashboard/public/dashboard_api/layout_manager/layout_manager.ts
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/dashboard/public/dashboard_api/unsaved_changes_manager.test.ts
Outdated
Show resolved
Hide resolved
|
I was thinking something more like this Heenawter#15 |
Layout unsaved changes
nreese
left a comment
There was a problem hiding this comment.
kibana-presentation changes LGTM
code review and tested in chrome
src/platform/plugins/shared/dashboard/public/dashboard_api/unsaved_changes_manager.test.ts
Outdated
Show resolved
Hide resolved
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
cc @Heenawter |
…aved changes (elastic#248029) ## Summary This PR ensures that **only** the necessary pieces of layout state are backed up when a dashboard has unsaved changes. Specifically, it fixes two separate bugs with backup state: 1. Consider the following code: https://github.com/elastic/kibana/blob/18f8010cd26a8fa6abe3dc844d199ca52dcd122d/src/platform/plugins/shared/dashboard/public/dashboard_api/unsaved_changes_manager.ts#L125-L128 Because elastic#245588 made pinned controls **normal children** of the Dashboard rather than having separate unsaved changes management through the control group, the above code resulted in **panels** being backed up as unsaved changes **even if** only controls had changes. For example, making a single selection in a control resulted in both controls **and** panels being backed up to session storage - which caused extremely long URLs when `Short URL` permissions were turned off. 2. Now consider the following: https://github.com/elastic/kibana/blob/18f8010cd26a8fa6abe3dc844d199ca52dcd122d/src/platform/plugins/shared/dashboard/public/dashboard_api/unsaved_changes_manager.ts#L83-L88 https://github.com/elastic/kibana/blob/18f8010cd26a8fa6abe3dc844d199ca52dcd122d/src/platform/plugins/shared/dashboard/public/dashboard_api/layout_manager/layout_manager.ts#L492-L517 Because we were using `isLayoutEqual`, which is checking for layout changes to **both** controls and panel layouts, we were reporting layout changes for panels even if only controls changed. For example, reordering a single control resulted in both controls **and** panels being backed up to session storage - which caused extremely long URLs when `Short URL` permissions were turned off. This PR fixes both of these bugs by... 1. We now compare **which children actually changed** and determine which ones are controls and which are normal panels. We only backup panels if at least one panel child changed, and same with controls. 2. We now separate out our comparison into `arePanelLayoutsEqual` and `areControlsLayoutsEqual` and we conditionally return each piece of state as unsaved depending on the results. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Nathan Reese <reese.nathan@gmail.com>
Summary
This PR ensures that only the necessary pieces of layout state are backed up when a dashboard has unsaved changes. Specifically, it fixes two separate bugs with backup state:
Consider the following code:
kibana/src/platform/plugins/shared/dashboard/public/dashboard_api/unsaved_changes_manager.ts
Lines 125 to 128 in 18f8010
Because [Controls Anywhere] Feature Branch #245588 made pinned controls normal children of the Dashboard rather than having separate unsaved changes management through the control group, the above code resulted in panels being backed up as unsaved changes even if only controls had changes. For example, making a single selection in a control resulted in both controls and panels being backed up to session storage - which caused extremely long URLs when
Short URLpermissions were turned off.Now consider the following:
kibana/src/platform/plugins/shared/dashboard/public/dashboard_api/unsaved_changes_manager.ts
Lines 83 to 88 in 18f8010
kibana/src/platform/plugins/shared/dashboard/public/dashboard_api/layout_manager/layout_manager.ts
Lines 492 to 517 in 18f8010
Because we were using
isLayoutEqual, which is checking for layout changes to both controls and panel layouts, we were reporting layout changes for panels even if only controls changed. For example, reordering a single control resulted in both controls and panels being backed up to session storage - which caused extremely long URLs whenShort URLpermissions were turned off.This PR fixes both of these bugs by...
We now compare which children actually changed and determine which ones are controls and which are normal panels. We only backup panels if at least one panel child changed, and same with controls.
We now separate out our comparison into
arePanelLayoutsEqualandareControlsLayoutsEqualand we conditionally return each piece of state as unsaved depending on the results.Checklist
release_note:*label is applied per the guidelinesbackport:*labels.