Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 14 commits into from Oct 18, 2023
Merged

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6ba1228
Run flaky test runner with session storage logs
Heenawter Sep 29, 2023
004f1f2
Try catching error
Heenawter Oct 4, 2023
f1d07e7
Fix infinite loop on error thrown
Heenawter Oct 5, 2023
8dccdd8
Add some more debug statements
Heenawter Oct 16, 2023
294c3ee
Try to determine when request is cancelled
Heenawter Oct 17, 2023
d101aa1
Try only clearing state on not found error
Heenawter Oct 17, 2023
b90df40
Try FTR again on full test suite
Heenawter Oct 17, 2023
4d4547d
Clean up component + remove `.only`
Heenawter Oct 17, 2023
19cdc32
Remove cancelled test + change `statusCode`
Heenawter Oct 17, 2023
a8a8866
Fix linting
Heenawter Oct 17, 2023
1f3f4d3
Merge branch 'main' into fix-flaky-167661_2023-09-29
Heenawter Oct 17, 2023
9c5c15a
Merge branch 'main' into fix-flaky-167661_2023-09-29
Heenawter Oct 18, 2023
cad9af0
Clean up diffing of unsaved ids
Heenawter Oct 18, 2023
709a3e1
Remove unused dependency
Heenawter Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/plugins/dashboard/public/dashboard_listing/dashboard_listing_empty_prompt.tsx
View file
Open in desktop
Expand Up @@ -78,7 +78,9 @@ export const DashboardListingEmptyPrompt = ({
onClick={() =>
confirmDiscardUnsavedChanges(() => {
dashboardBackup.clearState(DASHBOARD_PANELS_UNSAVED_ID);
setUnsavedDashboardIds(dashboardBackup.getDashboardIdsWithUnsavedChanges());
setUnsavedDashboardIds(
dashboardBackup.getDashboardIdsWithUnsavedChanges(unsavedDashboardIds)
);
})
}
data-test-subj="discardDashboardPromptButton"
Expand Down Expand Up @@ -106,8 +108,9 @@ export const DashboardListingEmptyPrompt = ({
createItem,
disableCreateDashboardButton,
dashboardBackup,
setUnsavedDashboardIds,
goToDashboard,
setUnsavedDashboardIds,
unsavedDashboardIds,
]);

if (!showWriteControls) {
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/dashboard/public/dashboard_listing/dashboard_unsaved_listing.test.tsx
View file
Open in desktop
Expand Up @@ -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 },
},
]);

Expand Down
7 changes: 6 additions & 1 deletion src/plugins/dashboard/public/dashboard_listing/dashboard_unsaved_listing.tsx
View file
Open in desktop
Expand Up @@ -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);
Comment on lines +159 to +161
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return map;
}
return {
Expand All @@ -170,6 +173,7 @@ export const DashboardUnsavedListing = ({
}
setItems(newItems);
});

return () => {
canceled = true;
};
Expand All @@ -179,6 +183,7 @@ export const DashboardUnsavedListing = ({
<>
<EuiCallOut
heading="h3"
data-test-subj="unsavedDashboardsCallout"
title={dashboardUnsavedListingStrings.getUnsavedChangesTitle(
unsavedDashboardIds.length > 1
)}
Expand Down
19 changes: 13 additions & 6 deletions src/plugins/dashboard/public/dashboard_listing/hooks/use_dashboard_listing_table.tsx
View file
Open in desktop
Expand Up @@ -127,9 +127,11 @@ export const useDashboardListingTable = ({
async (props: Pick<DashboardContainerInput, 'id' | 'title' | 'description' | 'tags'>) => {
await updateDashboardMeta(props);

setUnsavedDashboardIds(dashboardBackup.getDashboardIdsWithUnsavedChanges());
setUnsavedDashboardIds(
dashboardBackup.getDashboardIdsWithUnsavedChanges(unsavedDashboardIds)
);
},
[dashboardBackup, updateDashboardMeta]
[dashboardBackup, updateDashboardMeta, unsavedDashboardIds]
);

const contentEditorValidators: OpenContentEditorParams['customValidators'] = useMemo(
Expand Down Expand Up @@ -252,9 +254,11 @@ export const useDashboardListingTable = ({
});
}

setUnsavedDashboardIds(dashboardBackup.getDashboardIdsWithUnsavedChanges());
setUnsavedDashboardIds(
dashboardBackup.getDashboardIdsWithUnsavedChanges(unsavedDashboardIds)
);
},
[dashboardBackup, deleteDashboards, toasts]
[dashboardBackup, deleteDashboards, toasts, unsavedDashboardIds]
);

const editItem = useCallback(
Expand Down Expand Up @@ -324,8 +328,11 @@ export const useDashboardListingTable = ({
);

const refreshUnsavedDashboards = useCallback(
() => setUnsavedDashboardIds(dashboardBackup.getDashboardIdsWithUnsavedChanges()),
[dashboardBackup]
() =>
setUnsavedDashboardIds(
dashboardBackup.getDashboardIdsWithUnsavedChanges(unsavedDashboardIds)
),
[dashboardBackup, unsavedDashboardIds]
);

return {
Expand Down
13 changes: 11 additions & 2 deletions src/plugins/dashboard/public/services/dashboard_backup/dashboard_backup_service.ts
View file
Open in desktop
Expand Up @@ -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';
Expand Down Expand Up @@ -110,7 +111,12 @@ class DashboardBackupService implements DashboardBackupServiceType {
}
}

public getDashboardIdsWithUnsavedChanges() {
/**
* 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; therefore, in order to maintain this reference, this
* method needs to take the old array in as an argument.
*/
public getDashboardIdsWithUnsavedChanges(oldDashboardsWithUnsavedChanges: string[] = []) {
try {
const dashboardStatesInSpace =
this.sessionStorage.get(DASHBOARD_PANELS_SESSION_KEY)?.[this.activeSpaceId] || {};
Expand All @@ -125,7 +131,10 @@ class DashboardBackupService implements DashboardBackupServiceType {
)
dashboardsWithUnsavedChanges.push(dashboardId);
});
return dashboardsWithUnsavedChanges;

return isEqual(oldDashboardsWithUnsavedChanges, dashboardsWithUnsavedChanges)
? oldDashboardsWithUnsavedChanges
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
: dashboardsWithUnsavedChanges;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (e) {
this.notifications.toasts.addDanger({
title: backupServiceStrings.getPanelsGetError(e.message),
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/dashboard/public/services/dashboard_backup/types.ts
View file
Open in desktop
Expand Up @@ -15,6 +15,6 @@ export interface DashboardBackupServiceType {
setState: (id: string | undefined, newState: Partial<DashboardContainerInput>) => void;
getViewMode: () => ViewMode;
storeViewMode: (viewMode: ViewMode) => void;
getDashboardIdsWithUnsavedChanges: () => string[];
getDashboardIdsWithUnsavedChanges: (oldDashboardIdsWithUnsavedChanges?: string[]) => string[];
dashboardHasUnsavedEdits: (id?: string) => boolean;
}
41 changes: 28 additions & 13 deletions src/plugins/dashboard/public/services/dashboard_content_management/lib/find_dashboards.ts
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
};
}
Comment on lines +88 to +113
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🫠

}

export async function findDashboardsByIds(
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/dashboard/public/services/dashboard_content_management/types.ts
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@ export interface DashboardContentManagementRequiredServices {

export interface DashboardContentManagementService {
findDashboards: FindDashboardsService;
deleteDashboards: (ids: string[]) => void;
deleteDashboards: (ids: string[]) => Promise<void>;
Copy link
Contributor Author

@Heenawter Heenawter Oct 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

loadDashboardState: (props: { id?: string }) => Promise<LoadDashboardReturn>;
saveDashboardState: (props: SaveDashboardProps) => Promise<SaveDashboardReturn>;
checkForDuplicateDashboardTitle: (meta: DashboardDuplicateTitleCheckProps) => Promise<boolean>;
Expand Down
6 changes: 4 additions & 2 deletions test/functional/apps/dashboard/group1/dashboard_unsaved_state.ts
View file
Open in desktop
Expand Up @@ -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(
Expand Down Expand Up @@ -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');
}
Comment on lines +142 to +144
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await PageObjects.dashboard.loadSavedDashboard('few panels');
const currentPanelCount = await PageObjects.dashboard.getPanelCount();
expect(currentPanelCount).to.eql(unsavedPanelCount);
Expand Down