From e897daafd8e86b2023bb54d6d16f6da3aa8a9ed5 Mon Sep 17 00:00:00 2001 From: James Gowdy Date: Mon, 18 Jan 2021 11:49:31 +0000 Subject: [PATCH 1/3] [ML] Improving model snapshot revert UI experience --- .../model_snapshots/model_snapshots_table.tsx | 27 +++++++---- .../revert_model_snapshot_flyout.tsx | 48 +++++++++++-------- 2 files changed, 47 insertions(+), 28 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..c57c8bd719d99 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 @@ -50,15 +50,24 @@ export const ModelSnapshotTable: FC = ({ job, refreshJobList }) => { const [closeJobModalVisible, setCloseJobModalVisible] = useState(null); const [combinedJobState, setCombinedJobState] = useState(null); + let unmounted = false; useEffect(() => { loadModelSnapshots(); + return () => { + unmounted = true; + }; }, []); - const loadModelSnapshots = useCallback(async () => { + async function loadModelSnapshots() { + if (unmounted === true) { + // 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 = ({ 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> = [ @@ -231,6 +241,7 @@ export const ModelSnapshotTable: FC = ({ 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..d999d18c73d7b 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 = ({ snapshot, snapshots, job, closeFlyout }) => { +export const RevertModelSnapshotFlyout: FC = ({ + snapshot, + snapshots, + job, + closeFlyout, + refresh, +}) => { const { toasts } = useNotifications(); const { loadAnomalyDataForJob, loadEventRateForJob } = useMemo( () => chartLoaderProvider(mlResultsService), @@ -110,13 +117,6 @@ export const RevertModelSnapshotFlyout: FC = ({ snapshot, snapshots, job, setChartReady(true); }, [job]); - function closeWithReload() { - closeFlyout(true); - } - function closeWithoutReload() { - closeFlyout(false); - } - function showRevertModal() { setRevertModalVisible(true); } @@ -138,15 +138,18 @@ export const RevertModelSnapshotFlyout: FC = ({ 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), { @@ -166,7 +169,7 @@ export const RevertModelSnapshotFlyout: FC = ({ snapshot, snapshots, job, return ( <> - +
@@ -347,7 +350,7 @@ export const RevertModelSnapshotFlyout: FC = ({ snapshot, snapshots, job, - + = ({ snapshot, snapshots, job, confirmButtonDisabled={applying} buttonColor="danger" defaultFocusedButton="confirm" - /> + > + + )} From bd0ae2d1eab29cbdadda6be430f6d5193d97de2e Mon Sep 17 00:00:00 2001 From: James Gowdy Date: Mon, 18 Jan 2021 12:00:37 +0000 Subject: [PATCH 2/3] removing button disabling --- .../revert_model_snapshot_flyout.tsx | 4 ---- 1 file changed, 4 deletions(-) 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 d999d18c73d7b..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 @@ -80,7 +80,6 @@ export const RevertModelSnapshotFlyout: FC = ({ const [eventRateData, setEventRateData] = useState([]); const [anomalies, setAnomalies] = useState([]); const [chartReady, setChartReady] = useState(false); - const [applying, setApplying] = useState(false); useEffect(() => { createChartData(); @@ -125,7 +124,6 @@ export const RevertModelSnapshotFlyout: FC = ({ } async function applyRevert() { - setApplying(true); const end = replay && runInRealTime === false ? job.data_counts.latest_record_timestamp : undefined; try { @@ -151,7 +149,6 @@ export const RevertModelSnapshotFlyout: FC = ({ hideRevertModal(); closeFlyout(); } catch (error) { - setApplying(false); toasts.addError(new Error(error.body.message), { title: i18n.translate('xpack.ml.revertModelSnapshotFlyout.revertErrorTitle', { defaultMessage: 'Model snapshot revert failed', @@ -395,7 +392,6 @@ export const RevertModelSnapshotFlyout: FC = ({ defaultMessage: 'Apply', } )} - confirmButtonDisabled={applying} buttonColor="danger" defaultFocusedButton="confirm" > From 6f1867da0d5ece31d12035d186f9eeda3e18797f Mon Sep 17 00:00:00 2001 From: James Gowdy Date: Tue, 19 Jan 2021 17:01:31 +0000 Subject: [PATCH 3/3] updating component is mounted check --- .../components/model_snapshots/model_snapshots_table.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 c57c8bd719d99..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,16 +50,16 @@ export const ModelSnapshotTable: FC = ({ job, refreshJobList }) => { const [closeJobModalVisible, setCloseJobModalVisible] = useState(null); const [combinedJobState, setCombinedJobState] = useState(null); - let unmounted = false; + const isMounted = useRef(true); useEffect(() => { loadModelSnapshots(); return () => { - unmounted = true; + isMounted.current = false; }; }, []); async function loadModelSnapshots() { - if (unmounted === true) { + 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;