Skip to content

[Controls Anywhere] Fix panels being backed up when controls have unsaved changes#248029

Merged
Heenawter merged 28 commits intoelastic:mainfrom
Heenawter:dashboard-unsaved-changes-subset_2026-01-06
Jan 13, 2026
Merged

[Controls Anywhere] Fix panels being backed up when controls have unsaved changes#248029
Heenawter merged 28 commits intoelastic:mainfrom
Heenawter:dashboard-unsaved-changes-subset_2026-01-06

Conversation

@Heenawter
Copy link
Copy Markdown
Contributor

@Heenawter Heenawter commented Jan 6, 2026

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:

    if (hasChildrenUnsavedChanges) {
    dashboardBackupState.panels = panels;
    dashboardBackupState.controlGroupInput = controlGroupInput;
    }

    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 URL permissions were turned off.

  2. Now consider the following:

    const dashboardStateChanges$ = combineLatest([
    settingsManager.internalApi.startComparing$(lastSavedState$),
    unifiedSearchManager.internalApi.startComparing$(lastSavedState$),
    layoutManager.internalApi.startComparing$(lastSavedState$),
    projectRoutingManager?.internalApi.startComparing$(lastSavedState$) ?? of({}),
    ]).pipe(

    startComparing$: (
    lastSavedState$: BehaviorSubject<DashboardState>
    ): Observable<{ panels?: DashboardState['panels'] }> => {
    return layout$.pipe(
    debounceTime(100),
    combineLatestWith(
    lastSavedState$.pipe(
    map((lastSaved) =>
    deserializeLayout(lastSaved.panels, lastSaved.controlGroupInput, getReferences)
    ),
    tap(({ layout, childState }) => {
    lastSavedChildState = childState;
    lastSavedLayout = layout;
    })
    )
    ),
    map(([currentLayout]) => {
    if (!areLayoutsEqual(lastSavedLayout, currentLayout)) {
    logStateDiff('dashboard layout', lastSavedLayout, currentLayout);
    return serializeLayout(currentLayout, currentChildState);
    }
    return {};
    })
    );
    },

    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

  • Unit or functional tests were updated or added to match the most common scenarios
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
  • Review the backport guidelines and apply applicable backport:* labels.

@Heenawter Heenawter added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Jan 6, 2026
@Heenawter Heenawter self-assigned this Jan 7, 2026
@Heenawter Heenawter force-pushed the dashboard-unsaved-changes-subset_2026-01-06 branch 2 times, most recently from b04c2b8 to 51c4dc2 Compare January 7, 2026 22:21
@Heenawter Heenawter force-pushed the dashboard-unsaved-changes-subset_2026-01-06 branch from 51c4dc2 to a87d9f5 Compare January 7, 2026 22:22
@Heenawter Heenawter changed the title [Dashboards] Store unsaved changes as a subset [Controls Anywhere] Fix panels being backed up when controls have unsaved changes Jan 7, 2026
@Heenawter Heenawter added the backport:skip This PR does not require backporting label Jan 8, 2026
@Heenawter Heenawter marked this pull request as ready for review January 8, 2026 18:00
@Heenawter Heenawter requested a review from a team as a code owner January 8, 2026 18:00
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@Heenawter Heenawter requested a review from nreese January 8, 2026 21:26
@Heenawter Heenawter added loe:medium Medium Level of Effort and removed loe:small Small Level of Effort labels Jan 9, 2026
Copy link
Copy Markdown
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.

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.

@Heenawter
Copy link
Copy Markdown
Contributor Author

@nreese I have implemented your suggestion - it is much cleaner now IMO. Let me know what you think!! 🙇

@Heenawter Heenawter requested a review from nreese January 13, 2026 00:18
@Heenawter Heenawter requested a review from nreese January 13, 2026 17:01
@nreese
Copy link
Copy Markdown
Contributor

nreese commented Jan 13, 2026

I was thinking something more like this Heenawter#15

Copy link
Copy Markdown
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.

kibana-presentation changes LGTM
code review and tested in chrome

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jan 13, 2026

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #36 / EPM Endpoints Knowledge Base returns knowledge base content for an installed package
  • [job] [logs] FTR Configs #130 / Search solution tests Search index details page Solution Nav - Search search index details page has index actions enabled add field button is enabled
  • [job] [logs] Jest Tests #19 / unsavedChangesManager onUnsavedChanges onSettingsChanges should have unsaved changes when tags change
  • [job] [logs] Jest Tests #19 / unsavedChangesManager onUnsavedChanges onSettingsChanges should have unsaved changes when tags change
  • [job] [logs] Jest Tests #19 / unsavedChangesManager onUnsavedChanges session state should backup unsaved panel changes and references when only layout changes
  • [job] [logs] Jest Tests #19 / unsavedChangesManager onUnsavedChanges session state should backup unsaved panel changes and references when only layout changes
  • [job] [logs] Jest Tests #19 / unsavedChangesManager projectRouting changes should detect projectRouting changes as unsaved changes
  • [job] [logs] Jest Tests #19 / unsavedChangesManager projectRouting changes should detect projectRouting changes as unsaved changes
  • [job] [logs] Jest Tests #19 / unsavedChangesManager projectRouting changes should have unsaved changes when projectRouting is different from saved value
  • [job] [logs] Jest Tests #19 / unsavedChangesManager projectRouting changes should have unsaved changes when projectRouting is different from saved value

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 786.2KB 786.3KB +179.0B

History

cc @Heenawter

@Heenawter Heenawter merged commit f81087a into elastic:main Jan 13, 2026
13 checks passed
@Heenawter Heenawter deleted the dashboard-unsaved-changes-subset_2026-01-06 branch January 13, 2026 20:42
smith pushed a commit to smith/kibana that referenced this pull request Jan 16, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium 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// v9.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants