Skip to content

Commit

Permalink
[Dashboard] [Controls] Fix controls getting overwritten on navigation (
Browse files Browse the repository at this point in the history
…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)
  • Loading branch information
Heenawter authored Jul 5, 2024
1 parent 24d5083 commit e5cc4d5
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -497,12 +497,11 @@ test('creates new embeddable with specified size if size is provided', async ()
expect(dashboard!.getState().explicitInput.panels.new_panel.gridData.h).toBe(1);
});

test('creates a control group from the control group factory and waits for it to be initialized', async () => {
test('creates a control group from the control group factory', async () => {
const mockControlGroupContainer = {
destroy: jest.fn(),
render: jest.fn(),
updateInput: jest.fn(),
untilInitialized: jest.fn(),
getInput: jest.fn().mockReturnValue({}),
getInput$: jest.fn().mockReturnValue(new Observable()),
getOutput: jest.fn().mockReturnValue({}),
Expand Down Expand Up @@ -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();
});

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,6 @@ export const initializeDashboard = async ({
dashboardContainer.controlGroup = controlGroup;
startSyncingDashboardControlGroup.bind(dashboardContainer)();
});
await controlGroup.untilInitialized();
}

// --------------------------------------------------------------------------------------
Expand Down

0 comments on commit e5cc4d5

Please sign in to comment.