mirror of
https://github.com/elastic/kibana.git
synced 2025-04-24 17:59:23 -04:00
# Backport This will backport the following commits from `main` to `8.8`: - [[Dashboard] [Controls] Fix race condition on reset (#156209)](https://github.com/elastic/kibana/pull/156209) <!--- Backport version: 8.9.7 --> ### 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":"2023-05-04T15:29:07Z","message":"[Dashboard] [Controls] Fix race condition on reset (#156209)\n\n## Summary\r\n\r\nThis PR fixes a race condition where new selections made in an options\r\nlist control were not always reset as expected due to multiple input\r\nchanges firing at the same time - because we were using separate\r\nsubscriptions for these two input changes, their timing was not\r\npredictable, which meant that you could sometimes get unexpected\r\nresults.\r\n\r\nThis PR does two things to make this race condition less likely: \r\n1) it simplifies the logic for how the control group input is reset,\r\nwhich makes the timing of the input changes more predictable\r\n2) it only resets the `timeRange` when necessary, which prevents the\r\ndashboard's `dashboardRefetchDiff` input subscription from firing on\r\nreset when it shouldn't (i.e. when `timeRestore` is `false`)\r\n\r\nRefer to\r\nhttps://github.com/elastic/kibana/pull/156209#discussion_r1181730783 for\r\na more verbose description of these changes + why they help fix this\r\nrace condition.\r\n\r\n### Before\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/235259648-0f0923ff-9ad8-47b6-b814-5b036f20c80e.mov\r\n\r\n\r\nSteps to recreate the bug:\r\n1. Create an empty dashboard with at least one options list control\r\n(note that the dashboard doesn't necessarily have to be empty, but it is\r\neasier to consistently reproduce this bug with a fast loading\r\ndashboard).\r\n2. Make some selections in the control and save the dashboard.\r\n<p align=\"center\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235253087-e4a49695-e547-4f7a-b9aa-d28c211cfefb.png\"/></p>\r\n3. Refresh the page (just to make sure any options list caching is\r\ncleared)\r\n4. Make some new selections in the control, leaving the dashboard in an\r\nunsaved state.\r\n<p align=\"center\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235253396-b5cfc321-272f-4635-868d-a832ca97f337.png\"/></p>\r\n5. Reset the changes via the new \"Reset\" button.\r\n6. You should see the control button text briefly be reset to the old\r\n\"saved\" selections from step 2, before it switches back to the \"new\"\r\nselections from step 3 - the `unsaved changes` badge will be gone, and\r\nthe selections shown in the popover will be inconsistent with those\r\nshown in the button text:\r\n<p align=\"center\"><img width=\"500px\"\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235252671-b8f78aae-6268-467b-9dd0-336eb55b29c2.png\"/></p>\r\n\r\n### After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/235259659-33712061-9d48-400c-939e-fcf2f2a1bfb7.mov\r\n\r\nSteps to test:\r\n1. Follow steps 1 through 5 in the above \"Steps to recreate the bug\"\r\nsection\r\n2. When the dashboard is reset, you should actually see the selections\r\nreset as expected - try this out with a few different dashboards to\r\nensure that you don't see the weird \"flicker\" anymore\r\n\r\n### Checklist\r\n\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\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":"bb1dbceaa5afbcaffdb3fda3ef4f72a2a7cc7317","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:Presentation","loe:days","impact:medium","v8.8.0","v8.9.0"],"number":156209,"url":"https://github.com/elastic/kibana/pull/156209","mergeCommit":{"message":"[Dashboard] [Controls] Fix race condition on reset (#156209)\n\n## Summary\r\n\r\nThis PR fixes a race condition where new selections made in an options\r\nlist control were not always reset as expected due to multiple input\r\nchanges firing at the same time - because we were using separate\r\nsubscriptions for these two input changes, their timing was not\r\npredictable, which meant that you could sometimes get unexpected\r\nresults.\r\n\r\nThis PR does two things to make this race condition less likely: \r\n1) it simplifies the logic for how the control group input is reset,\r\nwhich makes the timing of the input changes more predictable\r\n2) it only resets the `timeRange` when necessary, which prevents the\r\ndashboard's `dashboardRefetchDiff` input subscription from firing on\r\nreset when it shouldn't (i.e. when `timeRestore` is `false`)\r\n\r\nRefer to\r\nhttps://github.com/elastic/kibana/pull/156209#discussion_r1181730783 for\r\na more verbose description of these changes + why they help fix this\r\nrace condition.\r\n\r\n### Before\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/235259648-0f0923ff-9ad8-47b6-b814-5b036f20c80e.mov\r\n\r\n\r\nSteps to recreate the bug:\r\n1. Create an empty dashboard with at least one options list control\r\n(note that the dashboard doesn't necessarily have to be empty, but it is\r\neasier to consistently reproduce this bug with a fast loading\r\ndashboard).\r\n2. Make some selections in the control and save the dashboard.\r\n<p align=\"center\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235253087-e4a49695-e547-4f7a-b9aa-d28c211cfefb.png\"/></p>\r\n3. Refresh the page (just to make sure any options list caching is\r\ncleared)\r\n4. Make some new selections in the control, leaving the dashboard in an\r\nunsaved state.\r\n<p align=\"center\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235253396-b5cfc321-272f-4635-868d-a832ca97f337.png\"/></p>\r\n5. Reset the changes via the new \"Reset\" button.\r\n6. You should see the control button text briefly be reset to the old\r\n\"saved\" selections from step 2, before it switches back to the \"new\"\r\nselections from step 3 - the `unsaved changes` badge will be gone, and\r\nthe selections shown in the popover will be inconsistent with those\r\nshown in the button text:\r\n<p align=\"center\"><img width=\"500px\"\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235252671-b8f78aae-6268-467b-9dd0-336eb55b29c2.png\"/></p>\r\n\r\n### After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/235259659-33712061-9d48-400c-939e-fcf2f2a1bfb7.mov\r\n\r\nSteps to test:\r\n1. Follow steps 1 through 5 in the above \"Steps to recreate the bug\"\r\nsection\r\n2. When the dashboard is reset, you should actually see the selections\r\nreset as expected - try this out with a few different dashboards to\r\nensure that you don't see the weird \"flicker\" anymore\r\n\r\n### Checklist\r\n\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\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":"bb1dbceaa5afbcaffdb3fda3ef4f72a2a7cc7317"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156209","number":156209,"mergeCommit":{"message":"[Dashboard] [Controls] Fix race condition on reset (#156209)\n\n## Summary\r\n\r\nThis PR fixes a race condition where new selections made in an options\r\nlist control were not always reset as expected due to multiple input\r\nchanges firing at the same time - because we were using separate\r\nsubscriptions for these two input changes, their timing was not\r\npredictable, which meant that you could sometimes get unexpected\r\nresults.\r\n\r\nThis PR does two things to make this race condition less likely: \r\n1) it simplifies the logic for how the control group input is reset,\r\nwhich makes the timing of the input changes more predictable\r\n2) it only resets the `timeRange` when necessary, which prevents the\r\ndashboard's `dashboardRefetchDiff` input subscription from firing on\r\nreset when it shouldn't (i.e. when `timeRestore` is `false`)\r\n\r\nRefer to\r\nhttps://github.com/elastic/kibana/pull/156209#discussion_r1181730783 for\r\na more verbose description of these changes + why they help fix this\r\nrace condition.\r\n\r\n### Before\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/235259648-0f0923ff-9ad8-47b6-b814-5b036f20c80e.mov\r\n\r\n\r\nSteps to recreate the bug:\r\n1. Create an empty dashboard with at least one options list control\r\n(note that the dashboard doesn't necessarily have to be empty, but it is\r\neasier to consistently reproduce this bug with a fast loading\r\ndashboard).\r\n2. Make some selections in the control and save the dashboard.\r\n<p align=\"center\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235253087-e4a49695-e547-4f7a-b9aa-d28c211cfefb.png\"/></p>\r\n3. Refresh the page (just to make sure any options list caching is\r\ncleared)\r\n4. Make some new selections in the control, leaving the dashboard in an\r\nunsaved state.\r\n<p align=\"center\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235253396-b5cfc321-272f-4635-868d-a832ca97f337.png\"/></p>\r\n5. Reset the changes via the new \"Reset\" button.\r\n6. You should see the control button text briefly be reset to the old\r\n\"saved\" selections from step 2, before it switches back to the \"new\"\r\nselections from step 3 - the `unsaved changes` badge will be gone, and\r\nthe selections shown in the popover will be inconsistent with those\r\nshown in the button text:\r\n<p align=\"center\"><img width=\"500px\"\r\nsrc=\"https://user-images.githubusercontent.com/8698078/235252671-b8f78aae-6268-467b-9dd0-336eb55b29c2.png\"/></p>\r\n\r\n### After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/235259659-33712061-9d48-400c-939e-fcf2f2a1bfb7.mov\r\n\r\nSteps to test:\r\n1. Follow steps 1 through 5 in the above \"Steps to recreate the bug\"\r\nsection\r\n2. When the dashboard is reset, you should actually see the selections\r\nreset as expected - try this out with a few different dashboards to\r\nensure that you don't see the weird \"flicker\" anymore\r\n\r\n### Checklist\r\n\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\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":"bb1dbceaa5afbcaffdb3fda3ef4f72a2a7cc7317"}}]}] BACKPORT--> Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
This commit is contained in:
parent
de9c32a7f1
commit
09d3dc42a4
3 changed files with 23 additions and 21 deletions
|
@ -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
|
||||
|
|
|
@ -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 {
|
||||
|
|
|
@ -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,
|
||||
};
|
||||
},
|
||||
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue