[Controls] Move diffing from the dashboard to the control group #175146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Heenawter
added
Feature:Dashboard
|
/ci |
Heenawter
force-pushed
the
control-group-diffing_2024-01-11
branch
2 times, most recently
from
ead27d0 to
206bc0e
Compare
|
/ci |
Heenawter
force-pushed
the
control-group-diffing_2024-01-11
branch
from
9680720 to
bd39063
Compare
|
/ci |
Heenawter
force-pushed
the
control-group-diffing_2024-01-11
branch
from
a37f9a7 to
2986be6
Compare
|
/ci |
2 similar comments
|
/ci |
|
/ci |
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
…er/kibana into control-group-diffing_2024-01-11
nickpeihl
approved these changes
Jan 30, 2024
src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts
Show resolved
Hide resolved
ThomThomson
reviewed
Jan 30, 2024
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @Heenawter |
kibanamachine
added
v8.13.0
backport:skip
CoenWarmer
pushed a commit
to CoenWarmer/kibana
that referenced
this pull request
Feb 15, 2024
…tic#175146) Closes elastic#174703 ## Summary Currently, the burden of determining unsaved changes is handled entirely by the Dashboard - this means that we need special logic for comparing the state of every piece of a dashboard, including the controls and the panels. Once the [embeddable refactor](elastic#167429) work is complete, this will no longer be the case; instead, each component will be responsible for handling its **own** unsaved changes, and the Dashboard will simply listen for updates. As an intermediate step, this PR separates out the control group logic from the Dashboard plugin so that, when it comes time, the transition to the new embeddable framework will be much smoother. This PR also unblocks elastic#170396 since, now that the control group is responsible for handling its own unsaved changes, I should be able to determine when to enable/disable the "reset changes" button. ### 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] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
fkanout
pushed a commit
to fkanout/kibana
that referenced
this pull request
Mar 4, 2024
…tic#175146) Closes elastic#174703 ## Summary Currently, the burden of determining unsaved changes is handled entirely by the Dashboard - this means that we need special logic for comparing the state of every piece of a dashboard, including the controls and the panels. Once the [embeddable refactor](elastic#167429) work is complete, this will no longer be the case; instead, each component will be responsible for handling its **own** unsaved changes, and the Dashboard will simply listen for updates. As an intermediate step, this PR separates out the control group logic from the Dashboard plugin so that, when it comes time, the transition to the new embeddable framework will be much smoother. This PR also unblocks elastic#170396 since, now that the control group is responsible for handling its own unsaved changes, I should be able to determine when to enable/disable the "reset changes" button. ### 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] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
1 task
Heenawter
added a commit
that referenced
this pull request
Jul 5, 2024
…#187509) ## Summary > [!WARNING] > Beware - the longest description ever for a one line change is incoming. > >  > > **TLDR:** We were previously `await`ing the initialization of the control group before navigating to the destination dashboard, which caused a race condition where, if the control group had time to report unsaved changes before the initialization promise was resolved, the control group's input would get backed up under the wrong ID. If we no longer `await` control group initialization, we remove this race condition. Previously, on dashboard navigation, we were `await`ing the initialization of the control group before navigating to the destination dashboard - this was because, before #174201, the control group could change its selections and the dashboard needed to know the most up-to-date control group output before it could start loading its panels. However, once #175146 was merged and the control group started reporting its own unsaved changes, this caused a race condition on navigation depending on whether or not the dashboard had time to backup its unsaved changes to the session storage before the control group was initialized. ### Description of the race condition Consider the following repro steps: 1. You start at your source dashboard (which has no controls), clear your cache, and slow down your network speed. 2. You click on a markdown link to navigate to your destination dashboard (which has controls). 3. You think everything worked as expected - hoorah! 4. You click on a markdown link to navigate back to your source dashboard.... but your source dashboard now has the controls of your destination dashboard! What just happened? > [!NOTE] > If the initialization of the control group happens **before the dashboard has a chance to backup the control group input to session storage under the wrong ID**, then this bug does not happen - that is why it is important to slow down the network speed when trying to reproduce this, and it is also why this bug was more prevalent on Cloud than local instances of Kibana. https://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9 On step 2 when the markdown link is clicked, this is what happens in the code: 1. The `navigateToDashboard` method is called. 2. The control group is told to update its input and reinitialize via the call to `controlGroup.updateInputAndReinitialize` in the `initializeDashboard` method. 3. The dashboard is `await`ing the initialization of the control group before proceeding with navigation. 4. The control group is updated, which triggers its `unsavedChanges` subscription - this is comparing its own state to that of the **source** dashboard, which is the **wrong** input to be comparing against. 5. The control group reports to the dashboard that it **has** unsaved changes. 6. The dashboard backs up its unsaved changes to session storage under the wrong ID since navigation hasn't happened yet - i.e. the **destination dashboard's** control group input gets backed up under the **source dashboard's ID** 7. Finally, the control group reports that it is initialized and the dashboard can proceed with navigation - so, the dashboard ID changes and its input gets updated. 8. This triggers the control group to **once again** trigger the `unsavedChanges` subscription - this time, the comparison occurs with the **proper** dashboard input (i.e. the input from the **destination** dashboard). Assuming no previous unsaved changes, this would return **false** (i.e. the control group reports to the dashboard that it has **no** unsaved changes). On step 3, that is why the destination dashboard appears as expected - it has the correct controls, and no unsaved changes. But then, on step 4, this is what happens: 1. The `navigateToDashboard` method is called. 2. We fetch the session storage so that the "unsaved changes" can be applied to the dashboard saved object 3. Uh oh! As described in step 6 above, the session storage for the source dashboard includes the control group input from the **destination** dashboard! 4. So, when you go back to the source, the destination dashboard's controls come with you 🔥 🔥 🔥 ### Description of the fix Now, let's instead consider what happens when we **don't** `await` the control group initialization - if we go back to step 2 of the repro steps, then this is what happens in the code: 1. The `navigateToDashboard` method is called. 2. The control group is told to update its input and reinitialize via the call to `controlGroup.updateInputAndReinitialize` in the `initializeDashboard` method. 3. The dashboard is **not** waiting for initialization, so it goes ahead with navigation **before** the control group has time to report its unsaved changes (the control group's unsaved changes subscription is debounced). 4. Navigation occurs and **nothing** gets backed up to session storage! That is why, by no longer waiting for the control group to be initialized on navigation, we are no longer seeing the bug where controls were getting "replaced" on navigation: https://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jul 5, 2024
…elastic#187509) ## Summary > [!WARNING] > Beware - the longest description ever for a one line change is incoming. > >  > > **TLDR:** We were previously `await`ing the initialization of the control group before navigating to the destination dashboard, which caused a race condition where, if the control group had time to report unsaved changes before the initialization promise was resolved, the control group's input would get backed up under the wrong ID. If we no longer `await` control group initialization, we remove this race condition. Previously, on dashboard navigation, we were `await`ing the initialization of the control group before navigating to the destination dashboard - this was because, before elastic#174201, the control group could change its selections and the dashboard needed to know the most up-to-date control group output before it could start loading its panels. However, once elastic#175146 was merged and the control group started reporting its own unsaved changes, this caused a race condition on navigation depending on whether or not the dashboard had time to backup its unsaved changes to the session storage before the control group was initialized. ### Description of the race condition Consider the following repro steps: 1. You start at your source dashboard (which has no controls), clear your cache, and slow down your network speed. 2. You click on a markdown link to navigate to your destination dashboard (which has controls). 3. You think everything worked as expected - hoorah! 4. You click on a markdown link to navigate back to your source dashboard.... but your source dashboard now has the controls of your destination dashboard! What just happened? > [!NOTE] > If the initialization of the control group happens **before the dashboard has a chance to backup the control group input to session storage under the wrong ID**, then this bug does not happen - that is why it is important to slow down the network speed when trying to reproduce this, and it is also why this bug was more prevalent on Cloud than local instances of Kibana. https://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9 On step 2 when the markdown link is clicked, this is what happens in the code: 1. The `navigateToDashboard` method is called. 2. The control group is told to update its input and reinitialize via the call to `controlGroup.updateInputAndReinitialize` in the `initializeDashboard` method. 3. The dashboard is `await`ing the initialization of the control group before proceeding with navigation. 4. The control group is updated, which triggers its `unsavedChanges` subscription - this is comparing its own state to that of the **source** dashboard, which is the **wrong** input to be comparing against. 5. The control group reports to the dashboard that it **has** unsaved changes. 6. The dashboard backs up its unsaved changes to session storage under the wrong ID since navigation hasn't happened yet - i.e. the **destination dashboard's** control group input gets backed up under the **source dashboard's ID** 7. Finally, the control group reports that it is initialized and the dashboard can proceed with navigation - so, the dashboard ID changes and its input gets updated. 8. This triggers the control group to **once again** trigger the `unsavedChanges` subscription - this time, the comparison occurs with the **proper** dashboard input (i.e. the input from the **destination** dashboard). Assuming no previous unsaved changes, this would return **false** (i.e. the control group reports to the dashboard that it has **no** unsaved changes). On step 3, that is why the destination dashboard appears as expected - it has the correct controls, and no unsaved changes. But then, on step 4, this is what happens: 1. The `navigateToDashboard` method is called. 2. We fetch the session storage so that the "unsaved changes" can be applied to the dashboard saved object 3. Uh oh! As described in step 6 above, the session storage for the source dashboard includes the control group input from the **destination** dashboard! 4. So, when you go back to the source, the destination dashboard's controls come with you 🔥 🔥 🔥 ### Description of the fix Now, let's instead consider what happens when we **don't** `await` the control group initialization - if we go back to step 2 of the repro steps, then this is what happens in the code: 1. The `navigateToDashboard` method is called. 2. The control group is told to update its input and reinitialize via the call to `controlGroup.updateInputAndReinitialize` in the `initializeDashboard` method. 3. The dashboard is **not** waiting for initialization, so it goes ahead with navigation **before** the control group has time to report its unsaved changes (the control group's unsaved changes subscription is debounced). 4. Navigation occurs and **nothing** gets backed up to session storage! That is why, by no longer waiting for the control group to be initialized on navigation, we are no longer seeing the bug where controls were getting "replaced" on navigation: https://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit e5cc4d5)
Heenawter
added a commit
to Heenawter/kibana
that referenced
this pull request
Jul 5, 2024
…elastic#187509) ## Summary > [!WARNING] > Beware - the longest description ever for a one line change is incoming. > >  > > **TLDR:** We were previously `await`ing the initialization of the control group before navigating to the destination dashboard, which caused a race condition where, if the control group had time to report unsaved changes before the initialization promise was resolved, the control group's input would get backed up under the wrong ID. If we no longer `await` control group initialization, we remove this race condition. Previously, on dashboard navigation, we were `await`ing the initialization of the control group before navigating to the destination dashboard - this was because, before elastic#174201, the control group could change its selections and the dashboard needed to know the most up-to-date control group output before it could start loading its panels. However, once elastic#175146 was merged and the control group started reporting its own unsaved changes, this caused a race condition on navigation depending on whether or not the dashboard had time to backup its unsaved changes to the session storage before the control group was initialized. ### Description of the race condition Consider the following repro steps: 1. You start at your source dashboard (which has no controls), clear your cache, and slow down your network speed. 2. You click on a markdown link to navigate to your destination dashboard (which has controls). 3. You think everything worked as expected - hoorah! 4. You click on a markdown link to navigate back to your source dashboard.... but your source dashboard now has the controls of your destination dashboard! What just happened? > [!NOTE] > If the initialization of the control group happens **before the dashboard has a chance to backup the control group input to session storage under the wrong ID**, then this bug does not happen - that is why it is important to slow down the network speed when trying to reproduce this, and it is also why this bug was more prevalent on Cloud than local instances of Kibana. https://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9 On step 2 when the markdown link is clicked, this is what happens in the code: 1. The `navigateToDashboard` method is called. 2. The control group is told to update its input and reinitialize via the call to `controlGroup.updateInputAndReinitialize` in the `initializeDashboard` method. 3. The dashboard is `await`ing the initialization of the control group before proceeding with navigation. 4. The control group is updated, which triggers its `unsavedChanges` subscription - this is comparing its own state to that of the **source** dashboard, which is the **wrong** input to be comparing against. 5. The control group reports to the dashboard that it **has** unsaved changes. 6. The dashboard backs up its unsaved changes to session storage under the wrong ID since navigation hasn't happened yet - i.e. the **destination dashboard's** control group input gets backed up under the **source dashboard's ID** 7. Finally, the control group reports that it is initialized and the dashboard can proceed with navigation - so, the dashboard ID changes and its input gets updated. 8. This triggers the control group to **once again** trigger the `unsavedChanges` subscription - this time, the comparison occurs with the **proper** dashboard input (i.e. the input from the **destination** dashboard). Assuming no previous unsaved changes, this would return **false** (i.e. the control group reports to the dashboard that it has **no** unsaved changes). On step 3, that is why the destination dashboard appears as expected - it has the correct controls, and no unsaved changes. But then, on step 4, this is what happens: 1. The `navigateToDashboard` method is called. 2. We fetch the session storage so that the "unsaved changes" can be applied to the dashboard saved object 3. Uh oh! As described in step 6 above, the session storage for the source dashboard includes the control group input from the **destination** dashboard! 4. So, when you go back to the source, the destination dashboard's controls come with you 🔥 🔥 🔥 ### Description of the fix Now, let's instead consider what happens when we **don't** `await` the control group initialization - if we go back to step 2 of the repro steps, then this is what happens in the code: 1. The `navigateToDashboard` method is called. 2. The control group is told to update its input and reinitialize via the call to `controlGroup.updateInputAndReinitialize` in the `initializeDashboard` method. 3. The dashboard is **not** waiting for initialization, so it goes ahead with navigation **before** the control group has time to report its unsaved changes (the control group's unsaved changes subscription is debounced). 4. Navigation occurs and **nothing** gets backed up to session storage! That is why, by no longer waiting for the control group to be initialized on navigation, we are no longer seeing the bug where controls were getting "replaced" on navigation: https://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit e5cc4d5)
Heenawter
added a commit
that referenced
this pull request
Jul 5, 2024
…igation (#187509) (#187694) # Backport This will backport the following commits from `main` to `8.14`: - [[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)](#187509) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-07-05T15:35:24Z","message":"[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)\n\n## Summary\r\n\r\n\r\n> [!WARNING]\r\n> Beware - the longest description ever for a one line change is\r\nincoming.\r\n>\r\n>\r\n\r\n>\r\n> **TLDR:** We were previously `await`ing the initialization of the\r\ncontrol group before navigating to the destination dashboard, which\r\ncaused a race condition where, if the control group had time to report\r\nunsaved changes before the initialization promise was resolved, the\r\ncontrol group's input would get backed up under the wrong ID. If we no\r\nlonger `await` control group initialization, we remove this race\r\ncondition.\r\n\r\nPreviously, on dashboard navigation, we were `await`ing the\r\ninitialization of the control group before navigating to the destination\r\ndashboard - this was because, before\r\nhttps://github.com//pull/174201, the control group could\r\nchange its selections and the dashboard needed to know the most\r\nup-to-date control group output before it could start loading its\r\npanels. However, once #175146 was\r\nmerged and the control group started reporting its own unsaved changes,\r\nthis caused a race condition on navigation depending on whether or not\r\nthe dashboard had time to backup its unsaved changes to the session\r\nstorage before the control group was initialized.\r\n\r\n\r\n### Description of the race condition\r\n\r\nConsider the following repro steps:\r\n\r\n1. You start at your source dashboard (which has no controls), clear\r\nyour cache, and slow down your network speed.\r\n2. You click on a markdown link to navigate to your destination\r\ndashboard (which has controls).\r\n3. You think everything worked as expected - hoorah!\r\n4. You click on a markdown link to navigate back to your source\r\ndashboard.... but your source dashboard now has the controls of your\r\ndestination dashboard! What just happened?\r\n\r\n> [!NOTE]\r\n> If the initialization of the control group happens **before the\r\ndashboard has a chance to backup the control group input to session\r\nstorage under the wrong ID**, then this bug does not happen - that is\r\nwhy it is important to slow down the network speed when trying to\r\nreproduce this, and it is also why this bug was more prevalent on Cloud\r\nthan local instances of Kibana.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9\r\n\r\n\r\nOn step 2 when the markdown link is clicked, this is what happens in the\r\ncode:\r\n \r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is `await`ing the initialization of the control group\r\nbefore proceeding with navigation.\r\n4. The control group is updated, which triggers its `unsavedChanges`\r\nsubscription - this is comparing its own state to that of the **source**\r\ndashboard, which is the **wrong** input to be comparing against.\r\n5. The control group reports to the dashboard that it **has** unsaved\r\nchanges.\r\n6. The dashboard backs up its unsaved changes to session storage under\r\nthe wrong ID since navigation hasn't happened yet - i.e. the\r\n**destination dashboard's** control group input gets backed up under the\r\n**source dashboard's ID**\r\n7. Finally, the control group reports that it is initialized and the\r\ndashboard can proceed with navigation - so, the dashboard ID changes and\r\nits input gets updated.\r\n8. This triggers the control group to **once again** trigger the\r\n`unsavedChanges` subscription - this time, the comparison occurs with\r\nthe **proper** dashboard input (i.e. the input from the **destination**\r\ndashboard). Assuming no previous unsaved changes, this would return\r\n**false** (i.e. the control group reports to the dashboard that it has\r\n**no** unsaved changes).\r\n\r\nOn step 3, that is why the destination dashboard appears as expected -\r\nit has the correct controls, and no unsaved changes. But then, on step\r\n4, this is what happens:\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. We fetch the session storage so that the \"unsaved changes\" can be\r\napplied to the dashboard saved object\r\n3. Uh oh! As described in step 6 above, the session storage for the\r\nsource dashboard includes the control group input from the\r\n**destination** dashboard!\r\n4. So, when you go back to the source, the destination dashboard's\r\ncontrols come with you 🔥 🔥 🔥\r\n\r\n\r\n### Description of the fix\r\n\r\n\r\nNow, let's instead consider what happens when we **don't** `await` the\r\ncontrol group initialization - if we go back to step 2 of the repro\r\nsteps, then this is what happens in the code:\r\n\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is **not** waiting for initialization, so it goes ahead\r\nwith navigation **before** the control group has time to report its\r\nunsaved changes (the control group's unsaved changes subscription is\r\ndebounced).\r\n4. Navigation occurs and **nothing** gets backed up to session storage! \r\n\r\nThat is why, by no longer waiting for the control group to be\r\ninitialized on navigation, we are no longer seeing the bug where\r\ncontrols were getting \"replaced\" on navigation:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"e5cc4d58fb7e0b12ef99ec69ca35fda97925de5f","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","release_note:fix","Team:Presentation","loe:small","impact:high","Project:Controls","backport:prev-minor","v8.16.0"],"number":187509,"url":"https://github.com/elastic/kibana/pull/187509","mergeCommit":{"message":"[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)\n\n## Summary\r\n\r\n\r\n> [!WARNING]\r\n> Beware - the longest description ever for a one line change is\r\nincoming.\r\n>\r\n>\r\n\r\n>\r\n> **TLDR:** We were previously `await`ing the initialization of the\r\ncontrol group before navigating to the destination dashboard, which\r\ncaused a race condition where, if the control group had time to report\r\nunsaved changes before the initialization promise was resolved, the\r\ncontrol group's input would get backed up under the wrong ID. If we no\r\nlonger `await` control group initialization, we remove this race\r\ncondition.\r\n\r\nPreviously, on dashboard navigation, we were `await`ing the\r\ninitialization of the control group before navigating to the destination\r\ndashboard - this was because, before\r\nhttps://github.com//pull/174201, the control group could\r\nchange its selections and the dashboard needed to know the most\r\nup-to-date control group output before it could start loading its\r\npanels. However, once #175146 was\r\nmerged and the control group started reporting its own unsaved changes,\r\nthis caused a race condition on navigation depending on whether or not\r\nthe dashboard had time to backup its unsaved changes to the session\r\nstorage before the control group was initialized.\r\n\r\n\r\n### Description of the race condition\r\n\r\nConsider the following repro steps:\r\n\r\n1. You start at your source dashboard (which has no controls), clear\r\nyour cache, and slow down your network speed.\r\n2. You click on a markdown link to navigate to your destination\r\ndashboard (which has controls).\r\n3. You think everything worked as expected - hoorah!\r\n4. You click on a markdown link to navigate back to your source\r\ndashboard.... but your source dashboard now has the controls of your\r\ndestination dashboard! What just happened?\r\n\r\n> [!NOTE]\r\n> If the initialization of the control group happens **before the\r\ndashboard has a chance to backup the control group input to session\r\nstorage under the wrong ID**, then this bug does not happen - that is\r\nwhy it is important to slow down the network speed when trying to\r\nreproduce this, and it is also why this bug was more prevalent on Cloud\r\nthan local instances of Kibana.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9\r\n\r\n\r\nOn step 2 when the markdown link is clicked, this is what happens in the\r\ncode:\r\n \r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is `await`ing the initialization of the control group\r\nbefore proceeding with navigation.\r\n4. The control group is updated, which triggers its `unsavedChanges`\r\nsubscription - this is comparing its own state to that of the **source**\r\ndashboard, which is the **wrong** input to be comparing against.\r\n5. The control group reports to the dashboard that it **has** unsaved\r\nchanges.\r\n6. The dashboard backs up its unsaved changes to session storage under\r\nthe wrong ID since navigation hasn't happened yet - i.e. the\r\n**destination dashboard's** control group input gets backed up under the\r\n**source dashboard's ID**\r\n7. Finally, the control group reports that it is initialized and the\r\ndashboard can proceed with navigation - so, the dashboard ID changes and\r\nits input gets updated.\r\n8. This triggers the control group to **once again** trigger the\r\n`unsavedChanges` subscription - this time, the comparison occurs with\r\nthe **proper** dashboard input (i.e. the input from the **destination**\r\ndashboard). Assuming no previous unsaved changes, this would return\r\n**false** (i.e. the control group reports to the dashboard that it has\r\n**no** unsaved changes).\r\n\r\nOn step 3, that is why the destination dashboard appears as expected -\r\nit has the correct controls, and no unsaved changes. But then, on step\r\n4, this is what happens:\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. We fetch the session storage so that the \"unsaved changes\" can be\r\napplied to the dashboard saved object\r\n3. Uh oh! As described in step 6 above, the session storage for the\r\nsource dashboard includes the control group input from the\r\n**destination** dashboard!\r\n4. So, when you go back to the source, the destination dashboard's\r\ncontrols come with you 🔥 🔥 🔥\r\n\r\n\r\n### Description of the fix\r\n\r\n\r\nNow, let's instead consider what happens when we **don't** `await` the\r\ncontrol group initialization - if we go back to step 2 of the repro\r\nsteps, then this is what happens in the code:\r\n\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is **not** waiting for initialization, so it goes ahead\r\nwith navigation **before** the control group has time to report its\r\nunsaved changes (the control group's unsaved changes subscription is\r\ndebounced).\r\n4. Navigation occurs and **nothing** gets backed up to session storage! \r\n\r\nThat is why, by no longer waiting for the control group to be\r\ninitialized on navigation, we are no longer seeing the bug where\r\ncontrols were getting \"replaced\" on navigation:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"e5cc4d58fb7e0b12ef99ec69ca35fda97925de5f"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187509","number":187509,"mergeCommit":{"message":"[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)\n\n## Summary\r\n\r\n\r\n> [!WARNING]\r\n> Beware - the longest description ever for a one line change is\r\nincoming.\r\n>\r\n>\r\n\r\n>\r\n> **TLDR:** We were previously `await`ing the initialization of the\r\ncontrol group before navigating to the destination dashboard, which\r\ncaused a race condition where, if the control group had time to report\r\nunsaved changes before the initialization promise was resolved, the\r\ncontrol group's input would get backed up under the wrong ID. If we no\r\nlonger `await` control group initialization, we remove this race\r\ncondition.\r\n\r\nPreviously, on dashboard navigation, we were `await`ing the\r\ninitialization of the control group before navigating to the destination\r\ndashboard - this was because, before\r\nhttps://github.com//pull/174201, the control group could\r\nchange its selections and the dashboard needed to know the most\r\nup-to-date control group output before it could start loading its\r\npanels. However, once #175146 was\r\nmerged and the control group started reporting its own unsaved changes,\r\nthis caused a race condition on navigation depending on whether or not\r\nthe dashboard had time to backup its unsaved changes to the session\r\nstorage before the control group was initialized.\r\n\r\n\r\n### Description of the race condition\r\n\r\nConsider the following repro steps:\r\n\r\n1. You start at your source dashboard (which has no controls), clear\r\nyour cache, and slow down your network speed.\r\n2. You click on a markdown link to navigate to your destination\r\ndashboard (which has controls).\r\n3. You think everything worked as expected - hoorah!\r\n4. You click on a markdown link to navigate back to your source\r\ndashboard.... but your source dashboard now has the controls of your\r\ndestination dashboard! What just happened?\r\n\r\n> [!NOTE]\r\n> If the initialization of the control group happens **before the\r\ndashboard has a chance to backup the control group input to session\r\nstorage under the wrong ID**, then this bug does not happen - that is\r\nwhy it is important to slow down the network speed when trying to\r\nreproduce this, and it is also why this bug was more prevalent on Cloud\r\nthan local instances of Kibana.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9\r\n\r\n\r\nOn step 2 when the markdown link is clicked, this is what happens in the\r\ncode:\r\n \r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is `await`ing the initialization of the control group\r\nbefore proceeding with navigation.\r\n4. The control group is updated, which triggers its `unsavedChanges`\r\nsubscription - this is comparing its own state to that of the **source**\r\ndashboard, which is the **wrong** input to be comparing against.\r\n5. The control group reports to the dashboard that it **has** unsaved\r\nchanges.\r\n6. The dashboard backs up its unsaved changes to session storage under\r\nthe wrong ID since navigation hasn't happened yet - i.e. the\r\n**destination dashboard's** control group input gets backed up under the\r\n**source dashboard's ID**\r\n7. Finally, the control group reports that it is initialized and the\r\ndashboard can proceed with navigation - so, the dashboard ID changes and\r\nits input gets updated.\r\n8. This triggers the control group to **once again** trigger the\r\n`unsavedChanges` subscription - this time, the comparison occurs with\r\nthe **proper** dashboard input (i.e. the input from the **destination**\r\ndashboard). Assuming no previous unsaved changes, this would return\r\n**false** (i.e. the control group reports to the dashboard that it has\r\n**no** unsaved changes).\r\n\r\nOn step 3, that is why the destination dashboard appears as expected -\r\nit has the correct controls, and no unsaved changes. But then, on step\r\n4, this is what happens:\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. We fetch the session storage so that the \"unsaved changes\" can be\r\napplied to the dashboard saved object\r\n3. Uh oh! As described in step 6 above, the session storage for the\r\nsource dashboard includes the control group input from the\r\n**destination** dashboard!\r\n4. So, when you go back to the source, the destination dashboard's\r\ncontrols come with you 🔥 🔥 🔥\r\n\r\n\r\n### Description of the fix\r\n\r\n\r\nNow, let's instead consider what happens when we **don't** `await` the\r\ncontrol group initialization - if we go back to step 2 of the repro\r\nsteps, then this is what happens in the code:\r\n\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is **not** waiting for initialization, so it goes ahead\r\nwith navigation **before** the control group has time to report its\r\nunsaved changes (the control group's unsaved changes subscription is\r\ndebounced).\r\n4. Navigation occurs and **nothing** gets backed up to session storage! \r\n\r\nThat is why, by no longer waiting for the control group to be\r\ninitialized on navigation, we are no longer seeing the bug where\r\ncontrols were getting \"replaced\" on navigation:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"e5cc4d58fb7e0b12ef99ec69ca35fda97925de5f"}}]}] BACKPORT--> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Heenawter
added a commit
that referenced
this pull request
Jul 5, 2024
…igation (#187509) (#187693) # Backport This will backport the following commits from `main` to `8.15`: - [[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)](#187509) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-07-05T15:35:24Z","message":"[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)\n\n## Summary\r\n\r\n\r\n> [!WARNING]\r\n> Beware - the longest description ever for a one line change is\r\nincoming.\r\n>\r\n>\r\n\r\n>\r\n> **TLDR:** We were previously `await`ing the initialization of the\r\ncontrol group before navigating to the destination dashboard, which\r\ncaused a race condition where, if the control group had time to report\r\nunsaved changes before the initialization promise was resolved, the\r\ncontrol group's input would get backed up under the wrong ID. If we no\r\nlonger `await` control group initialization, we remove this race\r\ncondition.\r\n\r\nPreviously, on dashboard navigation, we were `await`ing the\r\ninitialization of the control group before navigating to the destination\r\ndashboard - this was because, before\r\nhttps://github.com//pull/174201, the control group could\r\nchange its selections and the dashboard needed to know the most\r\nup-to-date control group output before it could start loading its\r\npanels. However, once #175146 was\r\nmerged and the control group started reporting its own unsaved changes,\r\nthis caused a race condition on navigation depending on whether or not\r\nthe dashboard had time to backup its unsaved changes to the session\r\nstorage before the control group was initialized.\r\n\r\n\r\n### Description of the race condition\r\n\r\nConsider the following repro steps:\r\n\r\n1. You start at your source dashboard (which has no controls), clear\r\nyour cache, and slow down your network speed.\r\n2. You click on a markdown link to navigate to your destination\r\ndashboard (which has controls).\r\n3. You think everything worked as expected - hoorah!\r\n4. You click on a markdown link to navigate back to your source\r\ndashboard.... but your source dashboard now has the controls of your\r\ndestination dashboard! What just happened?\r\n\r\n> [!NOTE]\r\n> If the initialization of the control group happens **before the\r\ndashboard has a chance to backup the control group input to session\r\nstorage under the wrong ID**, then this bug does not happen - that is\r\nwhy it is important to slow down the network speed when trying to\r\nreproduce this, and it is also why this bug was more prevalent on Cloud\r\nthan local instances of Kibana.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9\r\n\r\n\r\nOn step 2 when the markdown link is clicked, this is what happens in the\r\ncode:\r\n \r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is `await`ing the initialization of the control group\r\nbefore proceeding with navigation.\r\n4. The control group is updated, which triggers its `unsavedChanges`\r\nsubscription - this is comparing its own state to that of the **source**\r\ndashboard, which is the **wrong** input to be comparing against.\r\n5. The control group reports to the dashboard that it **has** unsaved\r\nchanges.\r\n6. The dashboard backs up its unsaved changes to session storage under\r\nthe wrong ID since navigation hasn't happened yet - i.e. the\r\n**destination dashboard's** control group input gets backed up under the\r\n**source dashboard's ID**\r\n7. Finally, the control group reports that it is initialized and the\r\ndashboard can proceed with navigation - so, the dashboard ID changes and\r\nits input gets updated.\r\n8. This triggers the control group to **once again** trigger the\r\n`unsavedChanges` subscription - this time, the comparison occurs with\r\nthe **proper** dashboard input (i.e. the input from the **destination**\r\ndashboard). Assuming no previous unsaved changes, this would return\r\n**false** (i.e. the control group reports to the dashboard that it has\r\n**no** unsaved changes).\r\n\r\nOn step 3, that is why the destination dashboard appears as expected -\r\nit has the correct controls, and no unsaved changes. But then, on step\r\n4, this is what happens:\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. We fetch the session storage so that the \"unsaved changes\" can be\r\napplied to the dashboard saved object\r\n3. Uh oh! As described in step 6 above, the session storage for the\r\nsource dashboard includes the control group input from the\r\n**destination** dashboard!\r\n4. So, when you go back to the source, the destination dashboard's\r\ncontrols come with you 🔥 🔥 🔥\r\n\r\n\r\n### Description of the fix\r\n\r\n\r\nNow, let's instead consider what happens when we **don't** `await` the\r\ncontrol group initialization - if we go back to step 2 of the repro\r\nsteps, then this is what happens in the code:\r\n\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is **not** waiting for initialization, so it goes ahead\r\nwith navigation **before** the control group has time to report its\r\nunsaved changes (the control group's unsaved changes subscription is\r\ndebounced).\r\n4. Navigation occurs and **nothing** gets backed up to session storage! \r\n\r\nThat is why, by no longer waiting for the control group to be\r\ninitialized on navigation, we are no longer seeing the bug where\r\ncontrols were getting \"replaced\" on navigation:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"e5cc4d58fb7e0b12ef99ec69ca35fda97925de5f","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","release_note:fix","Team:Presentation","loe:small","impact:high","Project:Controls","backport:prev-minor","v8.16.0"],"title":"[Dashboard] [Controls] Fix controls getting overwritten on navigation","number":187509,"url":"https://github.com/elastic/kibana/pull/187509","mergeCommit":{"message":"[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)\n\n## Summary\r\n\r\n\r\n> [!WARNING]\r\n> Beware - the longest description ever for a one line change is\r\nincoming.\r\n>\r\n>\r\n\r\n>\r\n> **TLDR:** We were previously `await`ing the initialization of the\r\ncontrol group before navigating to the destination dashboard, which\r\ncaused a race condition where, if the control group had time to report\r\nunsaved changes before the initialization promise was resolved, the\r\ncontrol group's input would get backed up under the wrong ID. If we no\r\nlonger `await` control group initialization, we remove this race\r\ncondition.\r\n\r\nPreviously, on dashboard navigation, we were `await`ing the\r\ninitialization of the control group before navigating to the destination\r\ndashboard - this was because, before\r\nhttps://github.com//pull/174201, the control group could\r\nchange its selections and the dashboard needed to know the most\r\nup-to-date control group output before it could start loading its\r\npanels. However, once #175146 was\r\nmerged and the control group started reporting its own unsaved changes,\r\nthis caused a race condition on navigation depending on whether or not\r\nthe dashboard had time to backup its unsaved changes to the session\r\nstorage before the control group was initialized.\r\n\r\n\r\n### Description of the race condition\r\n\r\nConsider the following repro steps:\r\n\r\n1. You start at your source dashboard (which has no controls), clear\r\nyour cache, and slow down your network speed.\r\n2. You click on a markdown link to navigate to your destination\r\ndashboard (which has controls).\r\n3. You think everything worked as expected - hoorah!\r\n4. You click on a markdown link to navigate back to your source\r\ndashboard.... but your source dashboard now has the controls of your\r\ndestination dashboard! What just happened?\r\n\r\n> [!NOTE]\r\n> If the initialization of the control group happens **before the\r\ndashboard has a chance to backup the control group input to session\r\nstorage under the wrong ID**, then this bug does not happen - that is\r\nwhy it is important to slow down the network speed when trying to\r\nreproduce this, and it is also why this bug was more prevalent on Cloud\r\nthan local instances of Kibana.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9\r\n\r\n\r\nOn step 2 when the markdown link is clicked, this is what happens in the\r\ncode:\r\n \r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is `await`ing the initialization of the control group\r\nbefore proceeding with navigation.\r\n4. The control group is updated, which triggers its `unsavedChanges`\r\nsubscription - this is comparing its own state to that of the **source**\r\ndashboard, which is the **wrong** input to be comparing against.\r\n5. The control group reports to the dashboard that it **has** unsaved\r\nchanges.\r\n6. The dashboard backs up its unsaved changes to session storage under\r\nthe wrong ID since navigation hasn't happened yet - i.e. the\r\n**destination dashboard's** control group input gets backed up under the\r\n**source dashboard's ID**\r\n7. Finally, the control group reports that it is initialized and the\r\ndashboard can proceed with navigation - so, the dashboard ID changes and\r\nits input gets updated.\r\n8. This triggers the control group to **once again** trigger the\r\n`unsavedChanges` subscription - this time, the comparison occurs with\r\nthe **proper** dashboard input (i.e. the input from the **destination**\r\ndashboard). Assuming no previous unsaved changes, this would return\r\n**false** (i.e. the control group reports to the dashboard that it has\r\n**no** unsaved changes).\r\n\r\nOn step 3, that is why the destination dashboard appears as expected -\r\nit has the correct controls, and no unsaved changes. But then, on step\r\n4, this is what happens:\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. We fetch the session storage so that the \"unsaved changes\" can be\r\napplied to the dashboard saved object\r\n3. Uh oh! As described in step 6 above, the session storage for the\r\nsource dashboard includes the control group input from the\r\n**destination** dashboard!\r\n4. So, when you go back to the source, the destination dashboard's\r\ncontrols come with you 🔥 🔥 🔥\r\n\r\n\r\n### Description of the fix\r\n\r\n\r\nNow, let's instead consider what happens when we **don't** `await` the\r\ncontrol group initialization - if we go back to step 2 of the repro\r\nsteps, then this is what happens in the code:\r\n\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is **not** waiting for initialization, so it goes ahead\r\nwith navigation **before** the control group has time to report its\r\nunsaved changes (the control group's unsaved changes subscription is\r\ndebounced).\r\n4. Navigation occurs and **nothing** gets backed up to session storage! \r\n\r\nThat is why, by no longer waiting for the control group to be\r\ninitialized on navigation, we are no longer seeing the bug where\r\ncontrols were getting \"replaced\" on navigation:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"e5cc4d58fb7e0b12ef99ec69ca35fda97925de5f"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187509","number":187509,"mergeCommit":{"message":"[Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)\n\n## Summary\r\n\r\n\r\n> [!WARNING]\r\n> Beware - the longest description ever for a one line change is\r\nincoming.\r\n>\r\n>\r\n\r\n>\r\n> **TLDR:** We were previously `await`ing the initialization of the\r\ncontrol group before navigating to the destination dashboard, which\r\ncaused a race condition where, if the control group had time to report\r\nunsaved changes before the initialization promise was resolved, the\r\ncontrol group's input would get backed up under the wrong ID. If we no\r\nlonger `await` control group initialization, we remove this race\r\ncondition.\r\n\r\nPreviously, on dashboard navigation, we were `await`ing the\r\ninitialization of the control group before navigating to the destination\r\ndashboard - this was because, before\r\nhttps://github.com//pull/174201, the control group could\r\nchange its selections and the dashboard needed to know the most\r\nup-to-date control group output before it could start loading its\r\npanels. However, once #175146 was\r\nmerged and the control group started reporting its own unsaved changes,\r\nthis caused a race condition on navigation depending on whether or not\r\nthe dashboard had time to backup its unsaved changes to the session\r\nstorage before the control group was initialized.\r\n\r\n\r\n### Description of the race condition\r\n\r\nConsider the following repro steps:\r\n\r\n1. You start at your source dashboard (which has no controls), clear\r\nyour cache, and slow down your network speed.\r\n2. You click on a markdown link to navigate to your destination\r\ndashboard (which has controls).\r\n3. You think everything worked as expected - hoorah!\r\n4. You click on a markdown link to navigate back to your source\r\ndashboard.... but your source dashboard now has the controls of your\r\ndestination dashboard! What just happened?\r\n\r\n> [!NOTE]\r\n> If the initialization of the control group happens **before the\r\ndashboard has a chance to backup the control group input to session\r\nstorage under the wrong ID**, then this bug does not happen - that is\r\nwhy it is important to slow down the network speed when trying to\r\nreproduce this, and it is also why this bug was more prevalent on Cloud\r\nthan local instances of Kibana.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/91f9b9e1-87f0-44aa-b596-577dd4a541f9\r\n\r\n\r\nOn step 2 when the markdown link is clicked, this is what happens in the\r\ncode:\r\n \r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is `await`ing the initialization of the control group\r\nbefore proceeding with navigation.\r\n4. The control group is updated, which triggers its `unsavedChanges`\r\nsubscription - this is comparing its own state to that of the **source**\r\ndashboard, which is the **wrong** input to be comparing against.\r\n5. The control group reports to the dashboard that it **has** unsaved\r\nchanges.\r\n6. The dashboard backs up its unsaved changes to session storage under\r\nthe wrong ID since navigation hasn't happened yet - i.e. the\r\n**destination dashboard's** control group input gets backed up under the\r\n**source dashboard's ID**\r\n7. Finally, the control group reports that it is initialized and the\r\ndashboard can proceed with navigation - so, the dashboard ID changes and\r\nits input gets updated.\r\n8. This triggers the control group to **once again** trigger the\r\n`unsavedChanges` subscription - this time, the comparison occurs with\r\nthe **proper** dashboard input (i.e. the input from the **destination**\r\ndashboard). Assuming no previous unsaved changes, this would return\r\n**false** (i.e. the control group reports to the dashboard that it has\r\n**no** unsaved changes).\r\n\r\nOn step 3, that is why the destination dashboard appears as expected -\r\nit has the correct controls, and no unsaved changes. But then, on step\r\n4, this is what happens:\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. We fetch the session storage so that the \"unsaved changes\" can be\r\napplied to the dashboard saved object\r\n3. Uh oh! As described in step 6 above, the session storage for the\r\nsource dashboard includes the control group input from the\r\n**destination** dashboard!\r\n4. So, when you go back to the source, the destination dashboard's\r\ncontrols come with you 🔥 🔥 🔥\r\n\r\n\r\n### Description of the fix\r\n\r\n\r\nNow, let's instead consider what happens when we **don't** `await` the\r\ncontrol group initialization - if we go back to step 2 of the repro\r\nsteps, then this is what happens in the code:\r\n\r\n\r\n1. The `navigateToDashboard` method is called.\r\n2. The control group is told to update its input and reinitialize via\r\nthe call to `controlGroup.updateInputAndReinitialize` in the\r\n`initializeDashboard` method.\r\n3. The dashboard is **not** waiting for initialization, so it goes ahead\r\nwith navigation **before** the control group has time to report its\r\nunsaved changes (the control group's unsaved changes subscription is\r\ndebounced).\r\n4. Navigation occurs and **nothing** gets backed up to session storage! \r\n\r\nThat is why, by no longer waiting for the control group to be\r\ninitialized on navigation, we are no longer seeing the bug where\r\ncontrols were getting \"replaced\" on navigation:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/0e45a207-ff2a-46a6-9609-11a8dc5bcf67\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"e5cc4d58fb7e0b12ef99ec69ca35fda97925de5f"}}]}] BACKPORT--> Co-authored-by: Hannah Mudge <Heenawter@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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #174703
Summary
Currently, the burden of determining unsaved changes is handled entirely by the Dashboard - this means that we need special logic for comparing the state of every piece of a dashboard, including the controls and the panels. Once the embeddable refactor work is complete, this will no longer be the case; instead, each component will be responsible for handling its own unsaved changes, and the Dashboard will simply listen for updates.
As an intermediate step, this PR separates out the control group logic from the Dashboard plugin so that, when it comes time, the transition to the new embeddable framework will be much smoother. This PR also unblocks #170396 since, now that the control group is responsible for handling its own unsaved changes, I should be able to determine when to enable/disable the "reset changes" button.
Checklist
For maintainers