Skip to content

[8.14] [Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)#187694

Merged
Heenawter merged 3 commits intoelastic:8.14from
Heenawter:backport/8.14/pr-187509
Jul 5, 2024
Merged

[8.14] [Dashboard] [Controls] Fix controls getting overwritten on navigation (#187509)#187694
Heenawter merged 3 commits intoelastic:8.14from
Heenawter:backport/8.14/pr-187509

Conversation

@Heenawter
Copy link
Copy Markdown
Contributor

Backport

This will backport the following commits from main to 8.14:

Questions ?

Please refer to the Backport tool documentation

…elastic#187509)

## Summary

> [!WARNING]
> Beware - the longest description ever for a one line change is
incoming.
>
>
![The-Hangover-Math-GIF-source](https://github.com/elastic/kibana/assets/8698078/3ab94f20-0401-4c42-9bb2-c35b6025301b)
>
> **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 Heenawter added the backport This PR is a backport of another PR label Jul 5, 2024
@Heenawter Heenawter enabled auto-merge (squash) July 5, 2024 15:41
@elasticmachine
Copy link
Copy Markdown
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 #kibana-operations if you believe this is an error.

The following branches are currently considered to be open:

  • main
  • 8.15
  • 7.17

@jbudz
Copy link
Copy Markdown
Contributor

jbudz commented Jul 5, 2024

buildkite test this

@jbudz
Copy link
Copy Markdown
Contributor

jbudz commented Jul 5, 2024

@elasticmachine merge upstream

@Heenawter
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #30 / management advanced settings feature controls settings can be saved with the advancedSettings: ["all"] feature privilege

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 395.7KB 395.7KB -27.0B

History

Copy link
Copy Markdown
Contributor

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR is a backport of another PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants