From 04a71b13a3ae9d454c67a5031e79cbdfebd761ca Mon Sep 17 00:00:00 2001 From: James Gowdy <jgowdy@elastic.co> Date: Thu, 21 Jan 2021 10:54:28 +0000 Subject: [PATCH] [ML] Improving model snapshot revert UI experience (#88588) * [ML] Improving model snapshot revert UI experience * removing button disabling * updating component is mounted check Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../model_snapshots/model_snapshots_table.tsx | 29 +++++++---- .../revert_model_snapshot_flyout.tsx | 52 ++++++++++--------- 2 files changed, 48 insertions(+), 33 deletions(-) diff --git a/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx b/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx index 5b175eb06a4a3..edf038160e0a8 100644 --- a/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx +++ b/x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { FC, useEffect, useCallback, useState } from 'react'; +import React, { FC, useEffect, useCallback, useState, useRef } from 'react'; import { i18n } from '@kbn/i18n'; import { @@ -50,15 +50,24 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => { const [closeJobModalVisible, setCloseJobModalVisible] = useState<ModelSnapshot | null>(null); const [combinedJobState, setCombinedJobState] = useState<COMBINED_JOB_STATE | null>(null); + const isMounted = useRef(true); useEffect(() => { loadModelSnapshots(); + return () => { + isMounted.current = false; + }; }, []); - const loadModelSnapshots = useCallback(async () => { + async function loadModelSnapshots() { + if (isMounted.current === false) { + // table refresh can be triggered a while after a snapshot revert has been triggered. + // ensure the table is still visible before attempted to refresh it. + return; + } const { model_snapshots: ms } = await ml.getModelSnapshots(job.job_id); setSnapshots(ms); setSnapshotsLoaded(true); - }, [job]); + } const checkJobIsClosed = useCallback( async (snapshot: ModelSnapshot) => { @@ -107,13 +116,14 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => { } }, []); - const closeRevertFlyout = useCallback((reload: boolean) => { + const closeRevertFlyout = useCallback(() => { setRevertSnapshot(null); - if (reload) { - loadModelSnapshots(); - // wait half a second before refreshing the jobs list - setTimeout(refreshJobList, 500); - } + }, []); + + const refresh = useCallback(() => { + loadModelSnapshots(); + // wait half a second before refreshing the jobs list + setTimeout(refreshJobList, 500); }, []); const columns: Array<EuiBasicTableColumn<any>> = [ @@ -231,6 +241,7 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => { snapshots={snapshots} job={job} closeFlyout={closeRevertFlyout} + refresh={refresh} /> )} diff --git a/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx b/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx index 62f5623f67964..05e624c194e6e 100644 --- a/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx +++ b/x-pack/plugins/ml/public/application/components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx @@ -53,10 +53,17 @@ interface Props { snapshot: ModelSnapshot; snapshots: ModelSnapshot[]; job: CombinedJobWithStats; - closeFlyout(reload: boolean): void; + closeFlyout(): void; + refresh(): void; } -export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, closeFlyout }) => { +export const RevertModelSnapshotFlyout: FC<Props> = ({ + snapshot, + snapshots, + job, + closeFlyout, + refresh, +}) => { const { toasts } = useNotifications(); const { loadAnomalyDataForJob, loadEventRateForJob } = useMemo( () => chartLoaderProvider(mlResultsService), @@ -73,7 +80,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, const [eventRateData, setEventRateData] = useState<LineChartPoint[]>([]); const [anomalies, setAnomalies] = useState<Anomaly[]>([]); const [chartReady, setChartReady] = useState(false); - const [applying, setApplying] = useState(false); useEffect(() => { createChartData(); @@ -110,13 +116,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, setChartReady(true); }, [job]); - function closeWithReload() { - closeFlyout(true); - } - function closeWithoutReload() { - closeFlyout(false); - } - function showRevertModal() { setRevertModalVisible(true); } @@ -125,7 +124,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, } async function applyRevert() { - setApplying(true); const end = replay && runInRealTime === false ? job.data_counts.latest_record_timestamp : undefined; try { @@ -138,17 +136,19 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, })) : undefined; - await ml.jobs.revertModelSnapshot( - job.job_id, - currentSnapshot.snapshot_id, - replay, - end, - events - ); + ml.jobs + .revertModelSnapshot(job.job_id, currentSnapshot.snapshot_id, replay, end, events) + .then(() => { + toasts.addSuccess( + i18n.translate('xpack.ml.revertModelSnapshotFlyout.revertSuccessTitle', { + defaultMessage: 'Model snapshot revert successful', + }) + ); + refresh(); + }); hideRevertModal(); - closeWithReload(); + closeFlyout(); } catch (error) { - setApplying(false); toasts.addError(new Error(error.body.message), { title: i18n.translate('xpack.ml.revertModelSnapshotFlyout.revertErrorTitle', { defaultMessage: 'Model snapshot revert failed', @@ -166,7 +166,7 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, return ( <> - <EuiFlyout onClose={closeWithoutReload} hideCloseButton size="m"> + <EuiFlyout onClose={closeFlyout} hideCloseButton size="m"> <EuiFlyoutHeader hasBorder> <EuiTitle size="s"> <h5> @@ -347,7 +347,7 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, <EuiFlyoutFooter> <EuiFlexGroup justifyContent="spaceBetween"> <EuiFlexItem grow={false}> - <EuiButtonEmpty iconType="cross" onClick={closeWithoutReload} flush="left"> + <EuiButtonEmpty iconType="cross" onClick={closeFlyout} flush="left"> <FormattedMessage id="xpack.ml.newJob.wizard.revertModelSnapshotFlyout.closeButton" defaultMessage="Close" @@ -392,10 +392,14 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, defaultMessage: 'Apply', } )} - confirmButtonDisabled={applying} buttonColor="danger" defaultFocusedButton="confirm" - /> + > + <FormattedMessage + id="xpack.ml.newJob.wizard.revertModelSnapshotFlyout.modalBody" + defaultMessage="The snapshot revert will be carried out in the background and may take some time." + /> + </EuiConfirmModal> </EuiOverlayMask> )} </>