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

)

Closes #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)

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

@@ -106,8 +106,8 @@ export const DashboardListingEmptyPrompt = ({
     createItem,
     disableCreateDashboardButton,
     dashboardBackup,
-    setUnsavedDashboardIds,
     goToDashboard,
+    setUnsavedDashboardIds,
   ]);
 
   if (!showWriteControls) {

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

@@ -104,12 +104,12 @@ describe('Unsaved listing', () => {
       {
         id: 'failCase1',
         status: 'error',
-        error: { error: 'oh no', message: 'bwah', statusCode: 100 },
+        error: { error: 'oh no', message: 'bwah', statusCode: 404 },
       },
       {
         id: 'failCase2',
         status: 'error',
-        error: { error: 'oh no', message: 'bwah', statusCode: 100 },
+        error: { error: 'oh no', message: 'bwah', statusCode: 404 },
       },
     ]);
 

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

@@ -156,7 +156,10 @@ export const DashboardUnsavedListing = ({
       const newItems = results.reduce((map, result) => {
         if (result.status === 'error') {
           hasError = true;
-          dashboardBackup.clearState(result.id);
+          if (result.error.statusCode === 404) {
+            // Save object not found error
+            dashboardBackup.clearState(result.id);
+          }
           return map;
         }
         return {
@@ -170,6 +173,7 @@ export const DashboardUnsavedListing = ({
       }
       setItems(newItems);
     });
+
     return () => {
       canceled = true;
     };
@@ -179,6 +183,7 @@ export const DashboardUnsavedListing = ({
     <>
       <EuiCallOut
         heading="h3"
+        data-test-subj="unsavedDashboardsCallout"
         title={dashboardUnsavedListingStrings.getUnsavedChangesTitle(
           unsavedDashboardIds.length > 1
         )}

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

@@ -7,6 +7,7 @@
  */
 
 import { firstValueFrom } from 'rxjs';
+import { isEqual } from 'lodash';
 
 import { set } from '@kbn/safer-lodash-set';
 import { ViewMode } from '@kbn/embeddable-plugin/public';
@@ -42,6 +43,8 @@ class DashboardBackupService implements DashboardBackupServiceType {
   private notifications: DashboardNotificationsService;
   private spaces: DashboardSpacesService;
 
+  private oldDashboardsWithUnsavedChanges: string[] = [];
+
   constructor(requiredServices: DashboardBackupRequiredServices) {
     ({ notifications: this.notifications, spaces: this.spaces } = requiredServices);
     this.sessionStorage = new Storage(sessionStorage);
@@ -125,7 +128,16 @@ class DashboardBackupService implements DashboardBackupServiceType {
         )
           dashboardsWithUnsavedChanges.push(dashboardId);
       });
-      return dashboardsWithUnsavedChanges;
+
+      /**
+       * Because we are storing these unsaved dashboard IDs in React component state, we only want things to be re-rendered
+       * if the **contents** change, not if the array reference changes
+       */
+      if (!isEqual(this.oldDashboardsWithUnsavedChanges, dashboardsWithUnsavedChanges)) {
+        this.oldDashboardsWithUnsavedChanges = dashboardsWithUnsavedChanges;
+      }
+
+      return this.oldDashboardsWithUnsavedChanges;
     } catch (e) {
       this.notifications.toasts.addDanger({
         title: backupServiceStrings.getPanelsGetError(e.message),

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

@@ -6,16 +6,16 @@
  * Side Public License, v 1.
  */
 
+import { Reference } from '@kbn/content-management-utils';
 import { SavedObjectError, SavedObjectsFindOptionsReference } from '@kbn/core/public';
 
-import { Reference } from '@kbn/content-management-utils';
 import {
-  DashboardItem,
-  DashboardCrudTypes,
   DashboardAttributes,
+  DashboardCrudTypes,
+  DashboardItem,
 } from '../../../../common/content_management';
-import { DashboardStartDependencies } from '../../../plugin';
 import { DASHBOARD_CONTENT_ID } from '../../../dashboard_constants';
+import { DashboardStartDependencies } from '../../../plugin';
 import { dashboardContentManagementCache } from '../dashboard_content_management_service';
 
 export interface SearchDashboardsArgs {
@@ -83,19 +83,34 @@ export async function findDashboardById(
       references: cachedDashboard.item.references,
     };
   }
+
   /** Otherwise, fetch the dashboard from the content management client, add it to the cache, and return the result */
-  const response = await contentManagement.client
-    .get<DashboardCrudTypes['GetIn'], DashboardCrudTypes['GetOut']>({
+  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,
+    };
+  }
 }
 
 export async function findDashboardsByIds(

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>;
   loadDashboardState: (props: { id?: string }) => Promise<LoadDashboardReturn>;
   saveDashboardState: (props: SaveDashboardProps) => Promise<SaveDashboardReturn>;
   checkForDuplicateDashboardTitle: (meta: DashboardDuplicateTitleCheckProps) => Promise<boolean>;

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

@@ -23,8 +23,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
   let unsavedPanelCount = 0;
   const testQuery = 'Test Query';
 
-  // Failing: See https://github.com/elastic/kibana/issues/167661
-  describe.skip('dashboard unsaved state', () => {
+  describe('dashboard unsaved state', () => {
     before(async () => {
       await kibanaServer.savedObjects.cleanStandardList();
       await kibanaServer.importExport.load(
@@ -140,6 +139,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         await PageObjects.visualize.gotoVisualizationLandingPage();
         await PageObjects.header.waitUntilLoadingHasFinished();
         await PageObjects.dashboard.navigateToApp();
+        if (await PageObjects.dashboard.onDashboardLandingPage()) {
+          await testSubjects.existOrFail('unsavedDashboardsCallout');
+        }
         await PageObjects.dashboard.loadSavedDashboard('few panels');
         const currentPanelCount = await PageObjects.dashboard.getPanelCount();
         expect(currentPanelCount).to.eql(unsavedPanelCount);