[Dashboard] [Controls] Fix race condition on reset (#156209)

## Summary

This PR fixes a race condition where new selections made in an options
list control were not always reset as expected due to multiple input
changes firing at the same time - because we were using separate
subscriptions for these two input changes, their timing was not
predictable, which meant that you could sometimes get unexpected
results.

This PR does two things to make this race condition less likely: 
1) it simplifies the logic for how the control group input is reset,
which makes the timing of the input changes more predictable
2) it only resets the `timeRange` when necessary, which prevents the
dashboard's `dashboardRefetchDiff` input subscription from firing on
reset when it shouldn't (i.e. when `timeRestore` is `false`)

Refer to
https://github.com/elastic/kibana/pull/156209#discussion_r1181730783 for
a more verbose description of these changes + why they help fix this
race condition.

### Before



https://user-images.githubusercontent.com/8698078/235259648-0f0923ff-9ad8-47b6-b814-5b036f20c80e.mov


Steps to recreate the bug:
1. Create an empty dashboard with at least one options list control
(note that the dashboard doesn't necessarily have to be empty, but it is
easier to consistently reproduce this bug with a fast loading
dashboard).
2. Make some selections in the control and save the dashboard.
<p align="center"><img
src="https://user-images.githubusercontent.com/8698078/235253087-e4a49695-e547-4f7a-b9aa-d28c211cfefb.png"/></p>
3. Refresh the page (just to make sure any options list caching is
cleared)
4. Make some new selections in the control, leaving the dashboard in an
unsaved state.
<p align="center"><img
src="https://user-images.githubusercontent.com/8698078/235253396-b5cfc321-272f-4635-868d-a832ca97f337.png"/></p>
5. Reset the changes via the new "Reset" button.
6. You should see the control button text briefly be reset to the old
"saved" selections from step 2, before it switches back to the "new"
selections from step 3 - the `unsaved changes` badge will be gone, and
the selections shown in the popover will be inconsistent with those
shown in the button text:
<p align="center"><img width="500px"
src="https://user-images.githubusercontent.com/8698078/235252671-b8f78aae-6268-467b-9dd0-336eb55b29c2.png"/></p>

### After


https://user-images.githubusercontent.com/8698078/235259659-33712061-9d48-400c-939e-fcf2f2a1bfb7.mov

Steps to test:
1. Follow steps 1 through 5 in the above "Steps to recreate the bug"
section
2. When the dashboard is reset, you should actually see the selections
reset as expected - try this out with a few different dashboards to
ensure that you don't see the weird "flicker" anymore

### Checklist

- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### 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)
This commit is contained in:
Hannah Mudge 2023-05-04 09:29:07 -06:00 committed by GitHub
parent bb08db94e0
commit bb1dbceaa5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 23 additions and 21 deletions

View file

@ -10,11 +10,10 @@ import { isEqual } from 'lodash';
import { Observable } from 'rxjs';
import deepEqual from 'fast-deep-equal';
import { compareFilters, COMPARE_ALL_OPTIONS, type Filter } from '@kbn/es-query';
import { debounceTime, distinctUntilChanged, distinctUntilKeyChanged, skip } from 'rxjs/operators';
import { distinctUntilChanged, skip } from 'rxjs/operators';
import {
ControlGroupInput,
getDefaultControlGroupInput,
persistableControlGroupInputIsEqual,
} from '@kbn/controls-plugin/common';
import { ControlGroupContainer } from '@kbn/controls-plugin/public';
@ -106,23 +105,6 @@ export function startSyncingDashboardControlGroup(this: DashboardContainer) {
})
);
// dashboard may reset the control group input when discarding changes. Subscribe to these changes and update accordingly
this.subscriptions.add(
(this.getInput$() as Readonly<Observable<DashboardContainerInput>>)
.pipe(debounceTime(10), distinctUntilKeyChanged('controlGroupInput'))
.subscribe(() => {
if (!isControlGroupInputEqual()) {
if (!this.getInput().controlGroupInput) {
this.controlGroup!.updateInput(getDefaultControlGroupInput());
return;
}
this.controlGroup!.updateInput({
...this.getInput().controlGroupInput,
});
}
})
);
// when control group outputs filters, force a refresh!
this.subscriptions.add(
this.controlGroup

View file

@ -27,6 +27,7 @@ import type { RefreshInterval } from '@kbn/data-plugin/public';
import { KibanaThemeProvider } from '@kbn/kibana-react-plugin/public';
import type { ControlGroupContainer } from '@kbn/controls-plugin/public';
import type { KibanaExecutionContext, OverlayRef } from '@kbn/core/public';
import { persistableControlGroupInputIsEqual } from '@kbn/controls-plugin/common';
import { ExitFullScreenButtonKibanaProvider } from '@kbn/shared-ux-button-exit-full-screen';
import {
@ -331,10 +332,21 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
const {
explicitInput: { timeRange, refreshInterval },
componentState: {
lastSavedInput: { timeRestore: lastSavedTimeRestore },
lastSavedInput: {
controlGroupInput: lastSavedControlGroupInput,
timeRestore: lastSavedTimeRestore,
},
},
} = this.getState();
if (
this.controlGroup &&
lastSavedControlGroupInput &&
!persistableControlGroupInputIsEqual(this.controlGroup.getInput(), lastSavedControlGroupInput)
) {
this.controlGroup.updateInput(lastSavedControlGroupInput);
}
// if we are using the unified search integration, we need to force reset the time picker.
if (this.creationOptions?.useUnifiedSearchIntegration && lastSavedTimeRestore) {
const {

View file

@ -116,10 +116,18 @@ export const dashboardContainerReducers = {
state.componentState.lastSavedInput = action.payload;
},
/**
* Resets the dashboard to the last saved input, excluding:
* 1) The time range, unless `timeRestore` is `true` - if we include the time range on reset even when
* `timeRestore` is `false`, this causes unecessary data fetches for the control group.
* 2) The view mode, since resetting should never impact this - sometimes the Dashboard saved objects
* have this saved in and we don't want resetting to cause unexpected view mode changes.
*/
resetToLastSavedInput: (state: DashboardReduxState) => {
state.explicitInput = {
...state.componentState.lastSavedInput,
viewMode: state.explicitInput.viewMode, // keep current view mode when resetting
...(!state.explicitInput.timeRestore && { timeRange: state.explicitInput.timeRange }),
viewMode: state.explicitInput.viewMode,
};
},