[Dashboard] Prevent unnecessary loss of dashboard unsaved state #167707

Heenawter · 2023-09-29T21:53:25Z

Summary

After a whole bunch of investigation, I ultimately realized that the attached test was flaky because the dashboard session storage was being cleared in the DashboardUnsavedListing component. When loading the unsaved dashboards, we used to remove the unsaved state for dashboards that returned any error from the CM service - this was designed so that, if a dashboard was deleted, we would remove it from the unsaved dashboard listing callout. However, as an unintended consequence, other errors, which should not cause the unsaved state to be lost, also caused it to be cleared.

Since I could only replicate some of the possible CM errors locally, it was impossible to narrow down exactly what error was being thrown in the attached flaky test since the FTR does not provide console logs. Therefore, rather than preventing that specific error from clearing the session storage, I instead made it so that only 404 errors (i.e. "Saved object not found" errors) cause the session storage to be cleared - this will guarantee that we only remove the unsaved state from the session storage if we know for sure that the dashboard has been deleted. Any other errors that are thrown by the CM will not cause the unsaved state to be unnecessarily lost.

Also, in my attempt to solve the above flaky test, I discovered and fixed the following:

Previously, when an error was thrown and caught in the DashboardUnsavedListing component, the refreshUnsavedDashboards would cause a useEffect infinite loop because the reference for the unsavedDashboardIds array would always be different even if the contents of the array were identical. This PR fixes that by ensuring the array reference only changes if the contents change.
Our previous way of catching errors in the findDashboardById method was not reliable, and did not catch errors that were thrown in, for example, the CM client get method. I refactored this so that all errors should now be caught.

Flaky Test Runner

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2023-10-17T22:15:12Z

src/plugins/dashboard/public/dashboard_listing/dashboard_unsaved_listing.tsx

+          if (result.error.statusCode === 404) {
+            // Save object not found error
+            dashboardBackup.clearState(result.id);


This is ultimately what fixed this flakiness 🎉 We should only clear the state specifically if the CM service returned an "Object not found" error.

Looks good that you used the status code directly rather than testing against the error message!

Heenawter · 2023-10-17T22:15:52Z

src/plugins/dashboard/public/services/dashboard_backup/dashboard_backup_service.ts

+      return isEqual(oldDashboardsWithUnsavedChanges, dashboardsWithUnsavedChanges)
+        ? oldDashboardsWithUnsavedChanges
+        : dashboardsWithUnsavedChanges;


Only change the array reference if the contents of the array have changed - this prevents an infinite React loop that would happen when an error was caught in the DashboardUnsavedListing component.

Heenawter · 2023-10-17T22:16:30Z

src/plugins/dashboard/public/services/dashboard_content_management/lib/find_dashboards.ts

+  try {
+    const response = await contentManagement.client.get<
+      DashboardCrudTypes['GetIn'],
+      DashboardCrudTypes['GetOut']
+    >({
      contentTypeId: DASHBOARD_CONTENT_ID,
      id,
-    })
-    .then((result) => {
-      dashboardContentManagementCache.addDashboard(result);
-      return { id, status: 'success', attributes: result.item.attributes };
-    })
-    .catch((e) => ({ status: 'error', error: e.body, id }));
+    });
+    if (response.item.error) {
+      throw response.item.error;
+    }

-  return response as FindDashboardsByIdResponse;
+    dashboardContentManagementCache.addDashboard(response);
+    return {
+      id,
+      status: 'success',
+      attributes: response.item.attributes,
+      references: response.item.references,
+    };
+  } catch (e) {
+    return {
+      status: 'error',
+      error: e.body || e.message,
+      id,
+    };
+  }


Had to refactor this - for some reason, the old code wasn't catching errors being thrown in contentManagement.client.get. Now, no matter where the error is thrown, it should (hopefully) be caught 🫠

Heenawter · 2023-10-17T22:16:42Z

src/plugins/dashboard/public/services/dashboard_content_management/types.ts

@@ -39,7 +39,7 @@ export interface DashboardContentManagementRequiredServices {

 export interface DashboardContentManagementService {
  findDashboards: FindDashboardsService;
-  deleteDashboards: (ids: string[]) => void;
+  deleteDashboards: (ids: string[]) => Promise<void>;


Unrelated, but noticed during my investigation: this method was async but, since the return value type didn't match, you would get a TS warning when awaiting it.

Heenawter · 2023-10-17T22:16:52Z

test/functional/apps/dashboard/group1/dashboard_unsaved_state.ts

+        if (await PageObjects.dashboard.onDashboardLandingPage()) {
+          await testSubjects.existOrFail('unsavedDashboardsCallout');
+        }


Adding this so that, if this failure happens again, it will be clearer why the failure is happening; because the failure (and the corresponding failure screenshot) happened on the dashboard itself, it took me awhile to confirm that the failure was happening due to the session storage being cleared. Failing on the listing page makes this clearer.

elasticmachine · 2023-10-18T16:17:11Z

Pinging @elastic/kibana-presentation (Team:Presentation)

ThomThomson

Glad to see this opened, it was quite a journey to understand this flaky test. Changes LGTM! Left one nit.

src/plugins/dashboard/public/services/dashboard_backup/dashboard_backup_service.ts

kibana-ci · 2023-10-18T22:09:20Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #53 / discover value suggestions non time based shows all autosuggest options for a filter in discover context app
[job] [logs] FTR Configs #40 / Spaces app Spaces Login Space Selector "after all" hook for "allows user to select initial space"
[job] [logs] FTR Configs #40 / Spaces app Spaces Login Space Selector allows user to select initial space

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`	375.4KB	375.8KB	+345.0B

History

💔 Build #169231 failed cad9af0
💛 Build #169052 was flaky 9c5c15a
💔 Build #168831 failed 1f3f4d3
💔 Build #168822 failed a8a8866

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…tic#167707) Closes elastic#167661 ## Summary After a whole bunch of investigation, I ultimately realized that the attached test was flaky because the dashboard session storage was being cleared in the `DashboardUnsavedListing` component. When loading the unsaved dashboards, we used to remove the unsaved state for dashboards that returned **any** error from the CM service - this was designed so that, if a dashboard was deleted, we would remove it from the unsaved dashboard listing callout. However, as an unintended consequence, **other** errors, which should **not** cause the unsaved state to be lost, also caused it to be cleared. Since I could only replicate **some** of the possible CM errors locally, it was impossible to narrow down exactly what error was being thrown in the attached flaky test since the FTR does not provide console logs. Therefore, rather than **preventing** that specific error from clearing the session storage, I instead made it so that **only** `404` errors (i.e. `"Saved object not found"` errors) cause the session storage to be cleared - this will guarantee that we only remove the unsaved state from the session storage if we know **for sure** that the dashboard has been deleted. Any other errors that are thrown by the CM will **not** cause the unsaved state to be unnecessarily lost. Also, in my attempt to solve the above flaky test, I discovered and fixed the following: 1. Previously, when an error was thrown and caught in the `DashboardUnsavedListing` component, the `refreshUnsavedDashboards` would cause a `useEffect` infinite loop because the reference for the `unsavedDashboardIds` array would always be different even if the contents of the array were identical. This PR fixes that by ensuring the array reference **only** changes if the contents change. 2. Our previous way of catching errors in the `findDashboardById` method was not reliable, and did not catch errors that were thrown in, for example, the CM client `get` method. I refactored this so that all errors should now be caught. ### [Flaky Test Runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3554) ![image](https://github.com/elastic/kibana/assets/8698078/1bcd9d6a-0c37-43ee-b5d6-f418cf878b41) ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### 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)

Heenawter changed the title ~~[Dashboard] Fix flaky test~~ [Dashboard] Fix unsaved changes flaky test Oct 4, 2023

Heenawter changed the title ~~[Dashboard] Fix unsaved changes flaky test~~ [Dashboard] Fix unsaved state flaky test Oct 4, 2023

Heenawter changed the title ~~[Dashboard] Fix unsaved state flaky test~~ [Dashboard] Fix flaky unsaved state test Oct 4, 2023

Heenawter force-pushed the fix-flaky-167661_2023-09-29 branch 2 times, most recently from 25b1bca to 192923a Compare October 6, 2023 19:11

Heenawter force-pushed the fix-flaky-167661_2023-09-29 branch 4 times, most recently from 107f3e8 to c8daaf3 Compare October 17, 2023 21:20

Heenawter added 4 commits October 17, 2023 15:23

Run flaky test runner with session storage logs

6ba1228

Try catching error

004f1f2

Fix infinite loop on error thrown

f1d07e7

Add some more debug statements

8dccdd8

Heenawter force-pushed the fix-flaky-167661_2023-09-29 branch from c8daaf3 to 484495e Compare October 17, 2023 21:24

Heenawter added 4 commits October 17, 2023 15:25

Try to determine when request is cancelled

294c3ee

Try only clearing state on not found error

d101aa1

Try FTR again on full test suite

b90df40

Clean up component + remove .only

4d4547d

Heenawter force-pushed the fix-flaky-167661_2023-09-29 branch 2 times, most recently from 92c1e6a to c23d21c Compare October 17, 2023 21:27

Remove cancelled test + change statusCode

19cdc32

Heenawter force-pushed the fix-flaky-167661_2023-09-29 branch from c23d21c to 19cdc32 Compare October 17, 2023 21:27

Heenawter added release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Oct 17, 2023

Heenawter changed the title ~~[Dashboard] Fix flaky unsaved state test~~ [Dashboard] Prevent unnecessary loss of dashboard unsaved state Oct 17, 2023

Heenawter added Feature:Dashboard Dashboard related features loe:medium Medium Level of Effort and removed loe:large Large Level of Effort labels Oct 17, 2023

Heenawter added impact:critical This issue should be addressed immediately due to a critical level of impact on the product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Oct 17, 2023

Fix linting

a8a8866

Heenawter commented Oct 17, 2023

View reviewed changes

Heenawter added 2 commits October 17, 2023 16:59

Merge branch 'main' into fix-flaky-167661_2023-09-29

1f3f4d3

Merge branch 'main' into fix-flaky-167661_2023-09-29

9c5c15a

Heenawter marked this pull request as ready for review October 18, 2023 16:17

Heenawter requested a review from a team as a code owner October 18, 2023 16:17

ThomThomson approved these changes Oct 18, 2023

View reviewed changes

src/plugins/dashboard/public/services/dashboard_backup/dashboard_backup_service.ts Outdated Show resolved Hide resolved

Clean up diffing of unsaved ids

cad9af0

Heenawter enabled auto-merge (squash) October 18, 2023 20:48

Remove unused dependency

709a3e1

Heenawter merged commit d6f7384 into elastic:main Oct 18, 2023
29 of 33 checks passed

kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Oct 18, 2023

Heenawter deleted the fix-flaky-167661_2023-09-29 branch October 18, 2023 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Prevent unnecessary loss of dashboard unsaved state #167707

[Dashboard] Prevent unnecessary loss of dashboard unsaved state #167707

Heenawter commented Sep 29, 2023 •

edited

Heenawter Oct 17, 2023

ThomThomson Oct 18, 2023

Heenawter Oct 17, 2023

Heenawter Oct 17, 2023

Heenawter Oct 17, 2023 •

edited

Heenawter Oct 17, 2023

elasticmachine commented Oct 18, 2023

ThomThomson left a comment

kibana-ci commented Oct 18, 2023 •

edited

[Dashboard] Prevent unnecessary loss of dashboard unsaved state #167707

[Dashboard] Prevent unnecessary loss of dashboard unsaved state #167707

Conversation

Heenawter commented Sep 29, 2023 • edited

Summary

Flaky Test Runner

Checklist

For maintainers

Heenawter Oct 17, 2023

Choose a reason for hiding this comment

ThomThomson Oct 18, 2023

Choose a reason for hiding this comment

Heenawter Oct 17, 2023

Choose a reason for hiding this comment

Heenawter Oct 17, 2023

Choose a reason for hiding this comment

Heenawter Oct 17, 2023 • edited

Choose a reason for hiding this comment

Heenawter Oct 17, 2023

Choose a reason for hiding this comment

elasticmachine commented Oct 18, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

kibana-ci commented Oct 18, 2023 • edited

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

Heenawter commented Sep 29, 2023 •

edited

Heenawter Oct 17, 2023 •

edited

kibana-ci commented Oct 18, 2023 •

edited