[Dashboard] [Controls] Fix controls getting overwritten on navigation #187509
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
Heenawter
added
the
backport:prev-minor
|
/ci |
Heenawter
force-pushed
the
fix-controls-unsaved-changes-race-condition_2024-07-03
branch
from
a4302b8 to
34c061f
Compare
|
/ci |
|
/ci |
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
Heenawter
commented
Jul 3, 2024
| @@ -532,7 +531,6 @@ test('creates a control group from the control group factory and waits for it to | |||
| undefined, | |||
| { lastSavedInput: expect.objectContaining({ controlStyle: 'oneLine' }) } | |||
| ); | |||
| expect(mockControlGroupContainer.untilInitialized).toHaveBeenCalled(); | |||
There was a problem hiding this comment.
I wanted to remove the untilInitialized function all-together since Dashboard is the only one that uses it, but some tests rely on it and I wanted to get this fix in ASAP 🙈 This will be cleaned up in the embeddable refactor, though.
Heenawter
added
the
bug
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
cc @Heenawter |
nickpeihl
approved these changes
Jul 5, 2024
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)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
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)
nickpeihl
requested review from
nickpeihl and
a team
and removed request for
nickpeihl
Heenawter
deleted the
fix-controls-unsaved-changes-race-condition_2024-07-03
branch
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>
|
With a little delay, but confirming here that this PR did indeed improve the initial rendering performance 
|
nreese
added a commit
that referenced
this pull request
Sep 10, 2024
…shboard container - reloaded (#192221) PR replaces legacy embeddable control group implementation with react control group implementation in DashboardContainer. #### background Work originally done in #190273. #190273 was reverted by #191993 because of dashboard performance degradation. It was determined that degradation was because new react embeddable controls fixed a regression where dashboard panels are loading before control filters are created. This regression was introduced by #187509. The work around is that this PR keeps the currently broken behavior in main and loads panels before control filters are ready. The thinking is that the migration would replace like for like and not introduce any performance changes. Then, at a later time, the regression could be resolved. #### reviewing These are the same changes from #190273 minus some work to introduce a current regression in main. A full re-review is not needed. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Hannah Mudge <hannah.wright@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@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.
Summary
Warning
Beware - the longest description ever for a one line change is incoming.
TLDR: We were previously
awaiting 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 longerawaitcontrol group initialization, we remove this race condition.Previously, on dashboard navigation, we were
awaiting 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:
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.
Screen.Recording.2024-07-03.at.1.01.21.PM.mov
On step 2 when the markdown link is clicked, this is what happens in the code:
navigateToDashboardmethod is called.controlGroup.updateInputAndReinitializein theinitializeDashboardmethod.awaiting the initialization of the control group before proceeding with navigation.unsavedChangessubscription - this is comparing its own state to that of the source dashboard, which is the wrong input to be comparing against.unsavedChangessubscription - 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:
navigateToDashboardmethod is called.Description of the fix
Now, let's instead consider what happens when we don't
awaitthe control group initialization - if we go back to step 2 of the repro steps, then this is what happens in the code:navigateToDashboardmethod is called.controlGroup.updateInputAndReinitializein theinitializeDashboardmethod.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:
Screen.Recording.2024-07-03.at.1.00.12.PM.mov
For maintainers