[ML] Improving model snapshot revert UI experience

[jgowdyelastic]

When applying a snapshot revert, the apply modal no longer stays open and disabled while the revert is happening. Instead the modal closes an a toast appears once the revert has been applied.

@lcawl is this text ok?

Fixes #74484
cc @pheyos

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• This was checked for cross-browser compatibility

[elasticmachine]

Pinging @elastic/ml-ui (:ml)

[peteharverson]

Tested and LGTM

[walterra]

To avoid the let and resetting the variable on every render we use this alternative pattern in some places: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ml/public/application/data_frame_analytics/pages/analytics_exploration/components/outlier_exploration/use_outlier_data.ts#L81

We discussed making a reusable custom hook in the past but didn't get around to do it. Here's the PR that originally introduced it: #81123

[jgowdyelastic]

I had a look at other similar examples in our code, but could not find one where the guard variable is used in a separate function and so needed to in the outer scope.

I might missing something, but using a const wrapper around the variable won't make any difference? Both will be redefined on render.

let unmounted = false;

vs

const options = { unmounted: false };

[walterra]

You're right this is different because of the additional outside callback. In the other examples the difference was that everything could be kept within the useEffect.

As an alternative you could use a refresh value to trigger the useEffect instead of directly calling loadModelSnapshots().

Something like:

const [isRefreshed, setIsRefreshed] = useState(true);

useEffect(() => {
    const options = { didCancel: false };
    loadModelSnapshots(options);
    return () => {
      options.didCancel = true;
    };
  }, [isRefreshed]);

async function loadModelSnapshots(options) {
    if (options.didCancel === 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);
}

const refresh = useCallback(
    () => {
        setIsRefreshed(state => !state);
        // wait half a second before refreshing the jobs list
        setTimeout(refreshJobList, 500);
    }
    [setIsRefreshed],
);

[jgowdyelastic]

I've updated this check so it now uses useRef to keep track of the flag for whether the component is mounted.
in 6f1867d

[walterra]

Latest changes LGTM 🥳

[jgowdyelastic]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 2c1e579

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
ml	7.0MB	7.0MB	+348.0B

History

• 💚 Build #99658 succeeded 6f1867d
• 💚 Build #99391 succeeded e897daa

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