[Controls] Do not ignore invalid selections #174201
Merged
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
nickpeihl
commented
Jan 5, 2024
…s-highlight-invalid
nickpeihl
added
release_note:feature
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
nickpeihl
commented
Jan 31, 2024
src/plugins/controls/public/options_list/components/options_list_popover_invalid_selections.tsx
Outdated
Show resolved
Hide resolved
nickpeihl
commented
Jan 31, 2024
test/functional/apps/dashboard_elements/controls/common/control_group_chaining.ts
Outdated
Show resolved
Hide resolved
pgayvallet
approved these changes
Feb 1, 2024
|
@andreadelrio I've updated the design of the options list invalid selections label + switched back to the Screen.Recording.2024-02-20.at.4.00.21.PM.mov@amyjtechwriter Would appreciate your help for the tour text!
If we choose to keep the word "invalid" here, this will also resolve #174201 (comment). |
|
Options list - Some selections are returning no results. For full results, change the selections. Range slider - The selected range is returning no results. For full results, change the selected rage. I think if using 'invalid' solves two problems we should keep it. |
|
@amyjtechwriter Thanks so much 🎉 Changes made in b5bf9da
|
|
@amyjtechwriter So sorry - @andreadelrio brought up a good point offline that requires more of your advice. Basically, the
Do you think the range slider needs a custom title in this case? I feel like "Invalid selections are no longer ignored" works as a title for both types - but we would keep the custom body text to clarify range selection vs options list selection. What do you think? I also had a question WRT the warning icon tooltips. Currently, we have the call to action
|
|
Hey hey, I think the title 'Invalid selections are no longer ignored' makes more sense than 'Invalid selections are now recognized', which my brain has been stuck on. For the body text on the range slider we could change it to 'The selected range is returning no results. Change the range.' or 'The selected range is returning no results. Try changing the range.'. <--- I think this second option has a bit of a friendlier tone, but that's just a personal opinion. WRT the warning icon tooltips, I see what you mean and I think your suggestion is the way to go @Heenawter. Let's take out the unnecessary words! |
|
@amyjtechwriter Thank you so much! Made the changes in 6fd9848 🙇
Do we want to change the options list body text to match the range slider - i.e. by removing "For full results"? So the text would become Update:
|
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
andreadelrio
approved these changes
Feb 23, 2024
semd
pushed a commit
to semd/kibana
that referenced
this pull request
Mar 4, 2024
Fixes elastic#164633 ## Summary Improves dashboard performance by not ignoring invalid Controls selections. This improves Dashboard performance as panels do not wait until Controls selections are validated. Prior to this, the Controls would run validations to find selections that would result in No Data. These "invalid selections" would be ignored and not applied to the filters. With this PR, all selections whether valid or invalid are applied to the filters. This means all controls and panels can be loaded immediately which improves the performance of the Dashboard. Since this can be considered a breaking UI change, I've added a suppressible toast warning users about the change. The screenshots below show the same dashboard with invalid selections. In the "Before" screenshot, the "Agent version" control selection is validated before the rest of the panels are loaded. Once validated, the invalid selection is ignored and the panels load based only on valid selections. In the "After" screenshot the "Agent version" control selection is immediately applied to the filters and the rest of the dashboard is loaded. The control selections are checked for validity only after the filters are applied and highlighted. The warning popover notifies the user that invalid selections are no longer ignored. Clicking the "Do not show again" button updates the browser's localStorage so that future warnings are suppressed. Before:  After:  @amyjtechwriter Can you please review the text in the warnings? We want to make users aware of the change as it could abruptly change the data in their dashboards. ### 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: Hannah Mudge <[email protected]> Co-authored-by: Hannah Mudge <[email protected]>
fkanout
pushed a commit
to fkanout/kibana
that referenced
this pull request
Mar 4, 2024
Fixes elastic#164633 ## Summary Improves dashboard performance by not ignoring invalid Controls selections. This improves Dashboard performance as panels do not wait until Controls selections are validated. Prior to this, the Controls would run validations to find selections that would result in No Data. These "invalid selections" would be ignored and not applied to the filters. With this PR, all selections whether valid or invalid are applied to the filters. This means all controls and panels can be loaded immediately which improves the performance of the Dashboard. Since this can be considered a breaking UI change, I've added a suppressible toast warning users about the change. The screenshots below show the same dashboard with invalid selections. In the "Before" screenshot, the "Agent version" control selection is validated before the rest of the panels are loaded. Once validated, the invalid selection is ignored and the panels load based only on valid selections. In the "After" screenshot the "Agent version" control selection is immediately applied to the filters and the rest of the dashboard is loaded. The control selections are checked for validity only after the filters are applied and highlighted. The warning popover notifies the user that invalid selections are no longer ignored. Clicking the "Do not show again" button updates the browser's localStorage so that future warnings are suppressed. Before:  After:  @amyjtechwriter Can you please review the text in the warnings? We want to make users aware of the change as it could abruptly change the data in their dashboards. ### 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: Hannah Mudge <[email protected]> Co-authored-by: Hannah Mudge <[email protected]>
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":"[email protected]"},"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 <[email protected]>
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":"[email protected]"},"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 <[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
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.
Fixes #164633
Summary
Improves dashboard performance by not ignoring invalid Controls selections.
This improves Dashboard performance as panels do not wait until Controls selections are validated. Prior to this, the Controls would run validations to find selections that would result in No Data. These "invalid selections" would be ignored and not applied to the filters.
With this PR, all selections whether valid or invalid are applied to the filters. This means all controls and panels can be loaded immediately which improves the performance of the Dashboard.
Since this can be considered a breaking UI change, I've added a suppressible toast warning users about the change.
The screenshots below show the same dashboard with invalid selections. In the "Before" screenshot, the "Agent version" control selection is validated before the rest of the panels are loaded. Once validated, the invalid selection is ignored and the panels load based only on valid selections.
In the "After" screenshot the "Agent version" control selection is immediately applied to the filters and the rest of the dashboard is loaded. The control selections are checked for validity only after the filters are applied and highlighted. The warning popover notifies the user that invalid selections are no longer ignored. Clicking the "Do not show again" button updates the browser's localStorage so that future warnings are suppressed.
Before:
After:
@amyjtechwriter Can you please review the text in the warnings? We want to make users aware of the change as it could abruptly change the data in their dashboards.
For maintainers