[8.14] [Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)#187694
Merged
Heenawter merged 3 commits intoelastic:8.14from Jul 5, 2024
Merged
Conversation
…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)
1 task
Contributor
|
CI was triggered for this PR, but this PR targets 8.14 which should not receive a future release. CI is not supported for these branches. Please consult the release schedule, or contact The following branches are currently considered to be open:
|
Contributor
|
buildkite test this |
Contributor
|
@elasticmachine merge upstream |
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
|
This file contains hidden or 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
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.
Backport
This will backport the following commits from
mainto8.14:Questions ?
Please refer to the Backport tool documentation