Plugin download progress and error handling

Summary: Show progress reporting in sidebar during plugin download. Also handle plugin download errors and show them to user as notifications. Reviewed By: mweststrate Differential Revision: D25530385 fbshipit-source-id: 42bf0e65b4434d00c1465705ce9ec5c723df6841
2020-12-15 09:28:58 -08:00
parent c3d61cc32d
commit efb82e80b5
5 changed files with 100 additions and 87 deletions
--- a/desktop/app/src/dispatcher/pluginDownloads.tsx
+++ b/desktop/app/src/dispatcher/pluginDownloads.tsx
@@ -15,8 +15,7 @@ import {Store} from '../reducers/index';
 import {
  PluginDownloadStatus,
  pluginDownloadStarted,
-  pluginDownloadFailed,
-  pluginDownloadSucceeded,
+  pluginDownloadFinished,
 } from '../reducers/pluginDownloads';
 import {sideEffect} from '../utils/sideEffect';
 import {default as axios} from 'axios';
@@ -25,7 +24,12 @@ import path from 'path';
 import tmp from 'tmp';
 import {promisify} from 'util';
 import {requirePlugin} from './plugins';
-import {registerPluginUpdate} from '../reducers/connections';
+import {registerPluginUpdate, setStaticView} from '../reducers/connections';
+import {notification, Typography} from 'antd';
+import React from 'react';
+import {ConsoleLogs} from '../chrome/ConsoleLogs';
+
+const {Text, Link} = Typography;

 // Adapter which forces node.js implementation for axios instead of browser implementation
 // used by default in Electron. Node.js implementation is better, because it
@@ -44,11 +48,7 @@ export default (store: Store) => {
    (state, store) => {
      for (const download of Object.values(state)) {
        if (download.status === PluginDownloadStatus.QUEUED) {
-          handlePluginDownload(
-            download.plugin,
-            download.enableDownloadedPlugin,
-            store,
-          );
+          handlePluginDownload(download.plugin, download.startedByUser, store);
        }
      }
    },
@@ -58,7 +58,7 @@ export default (store: Store) => {

 async function handlePluginDownload(
  plugin: DownloadablePluginDetails,
-  enableDownloadedPlugin: boolean,
+  startedByUser: boolean,
  store: Store,
 ) {
  const dispatch = store.dispatch;
@@ -115,21 +115,35 @@ async function handlePluginDownload(
      dispatch(
        registerPluginUpdate({
          plugin: pluginDefinition,
-          enablePlugin: enableDownloadedPlugin,
+          enablePlugin: startedByUser,
        }),
      );
    }
    console.log(
      `Successfully downloaded and installed plugin "${title}" v${version} from "${downloadUrl}" to "${dir}".`,
    );
-    dispatch(pluginDownloadSucceeded({plugin}));
  } catch (error) {
    console.error(
      `Failed to download plugin "${title}" v${version} from "${downloadUrl}" to "${dir}".`,
      error,
    );
-    dispatch(pluginDownloadFailed({plugin, error}));
+    if (startedByUser) {
+      notification.error({
+        message: `Failed to install plugin "${title}".`,
+        description: (
+          <Text>
+            See{' '}
+            <Link onClick={() => dispatch(setStaticView(ConsoleLogs))}>
+              logs
+            </Link>{' '}
+            for details.
+          </Text>
+        ),
+        placement: 'bottomLeft',
+      });
+    }
  } finally {
+    dispatch(pluginDownloadFinished({plugin}));
    await fs.remove(targetDir);
  }
 }
--- a/desktop/app/src/reducers/pluginDownloads.tsx
+++ b/desktop/app/src/reducers/pluginDownloads.tsx
@@ -20,11 +20,10 @@ export enum PluginDownloadStatus {

 export type DownloadablePluginState = {
  plugin: DownloadablePluginDetails;
-  enableDownloadedPlugin: boolean;
+  startedByUser: boolean;
 } & (
  | {status: PluginDownloadStatus.QUEUED}
  | {status: PluginDownloadStatus.STARTED; cancel: Canceler}
-  | {status: PluginDownloadStatus.FAILED; error: Error}
 );

 // We use plugin installation path as key as it is unique for each plugin version.
@@ -34,7 +33,7 @@ export type PluginDownloadStart = {
  type: 'PLUGIN_DOWNLOAD_START';
  payload: {
    plugin: DownloadablePluginDetails;
-    enableDownloadedPlugin: boolean;
+    startedByUser: boolean;
  };
 };

@@ -46,26 +45,17 @@ export type PluginDownloadStarted = {
  };
 };

-export type PluginDownloadSucceeded = {
-  type: 'PLUGIN_DOWNLOAD_SUCCEEDED';
+export type PluginDownloadFinished = {
+  type: 'PLUGIN_DOWNLOAD_FINISHED';
  payload: {
    plugin: DownloadablePluginDetails;
  };
 };

-export type PluginDownloadFailed = {
-  type: 'PLUGIN_DOWNLOAD_FAILED';
-  payload: {
-    plugin: DownloadablePluginDetails;
-    error: Error;
-  };
-};
-
 export type Action =
  | PluginDownloadStart
  | PluginDownloadStarted
-  | PluginDownloadSucceeded
-  | PluginDownloadFailed;
+  | PluginDownloadFinished;

 const INITIAL_STATE: State = {};

@@ -75,24 +65,21 @@ export default function reducer(
 ): State {
  switch (action.type) {
    case 'PLUGIN_DOWNLOAD_START': {
-      const {plugin, enableDownloadedPlugin} = action.payload;
+      const {plugin, startedByUser} = action.payload;
      const downloadState = state[plugin.dir];
-      if (
-        downloadState && // If download is already in progress - re-use the existing state.
-        downloadState.status !== PluginDownloadStatus.FAILED // Note that for failed downloads we want to retry downloads.
-      ) {
+      if (downloadState) {
+        // If download is already in progress - re-use the existing state.
        return produce(state, (draft) => {
          draft[plugin.dir] = {
            ...downloadState,
-            enableDownloadedPlugin:
-              enableDownloadedPlugin || downloadState.enableDownloadedPlugin,
+            startedByUser: startedByUser || downloadState.startedByUser,
          };
        });
      }
      return produce(state, (draft) => {
        draft[plugin.dir] = {
          plugin,
-          enableDownloadedPlugin: enableDownloadedPlugin,
+          startedByUser: startedByUser,
          status: PluginDownloadStatus.QUEUED,
        };
      });
@@ -110,29 +97,12 @@ export default function reducer(
        draft[plugin.dir] = {
          status: PluginDownloadStatus.STARTED,
          plugin,
-          enableDownloadedPlugin: downloadState.enableDownloadedPlugin,
+          startedByUser: downloadState.startedByUser,
          cancel,
        };
      });
    }
-    case 'PLUGIN_DOWNLOAD_FAILED': {
-      const {plugin, error} = action.payload;
-      const downloadState = state[plugin.dir];
-      if (!downloadState) {
-        console.warn(
-          `Invalid state transition PLUGIN_DOWNLOAD_FAILED when there is no download in progress to directory ${plugin.dir}`,
-        );
-      }
-      return produce(state, (draft) => {
-        draft[plugin.dir] = {
-          status: PluginDownloadStatus.FAILED,
-          plugin: downloadState.plugin,
-          enableDownloadedPlugin: downloadState.enableDownloadedPlugin,
-          error,
-        };
-      });
-    }
-    case 'PLUGIN_DOWNLOAD_SUCCEEDED': {
+    case 'PLUGIN_DOWNLOAD_FINISHED': {
      const {plugin} = action.payload;
      return produce(state, (draft) => {
        delete draft[plugin.dir];
@@ -145,7 +115,7 @@ export default function reducer(

 export const startPluginDownload = (payload: {
  plugin: DownloadablePluginDetails;
-  enableDownloadedPlugin: boolean;
+  startedByUser: boolean;
 }): Action => ({
  type: 'PLUGIN_DOWNLOAD_START',
  payload,
@@ -156,11 +126,6 @@ export const pluginDownloadStarted = (payload: {
  cancel: Canceler;
 }): Action => ({type: 'PLUGIN_DOWNLOAD_STARTED', payload});

-export const pluginDownloadSucceeded = (payload: {
+export const pluginDownloadFinished = (payload: {
  plugin: DownloadablePluginDetails;
-}): Action => ({type: 'PLUGIN_DOWNLOAD_SUCCEEDED', payload});
-
-export const pluginDownloadFailed = (payload: {
-  plugin: DownloadablePluginDetails;
-  error: Error;
-}): Action => ({type: 'PLUGIN_DOWNLOAD_FAILED', payload});
+}): Action => ({type: 'PLUGIN_DOWNLOAD_FINISHED', payload});
--- a/desktop/app/src/sandy-chrome/appinspect/PluginList.tsx
+++ b/desktop/app/src/sandy-chrome/appinspect/PluginList.tsx
@@ -10,7 +10,13 @@
 import React, {memo, useCallback, useEffect, useRef, useState} from 'react';
 import {Badge, Button, Menu, Tooltip, Typography} from 'antd';
 import {InfoIcon, SidebarTitle} from '../LeftSidebar';
-import {PlusOutlined, MinusOutlined, DeleteOutlined} from '@ant-design/icons';
+import {
+  PlusOutlined,
+  MinusOutlined,
+  DeleteOutlined,
+  LoadingOutlined,
+  DownloadOutlined,
+} from '@ant-design/icons';
 import {Glyph, Layout, styled} from '../../ui';
 import {theme, NUX, Tracked} from 'flipper-plugin';
 import {useDispatch, useStore} from '../../utils/useStore';
@@ -21,11 +27,14 @@ import Client from '../../Client';
 import {State} from '../../reducers';
 import BaseDevice from '../../devices/BaseDevice';
 import {getFavoritePlugins} from '../../chrome/mainsidebar/sidebarUtils';
-import {PluginDetails} from 'flipper-plugin-lib';
+import {PluginDetails, DownloadablePluginDetails} from 'flipper-plugin-lib';
 import {useMemoize} from '../../utils/useMemoize';
 import MetroDevice from '../../devices/MetroDevice';
-import {DownloadablePluginDetails} from 'plugin-lib/lib';
-import {startPluginDownload} from '../../reducers/pluginDownloads';
+import {
+  DownloadablePluginState,
+  PluginDownloadStatus,
+  startPluginDownload,
+} from '../../reducers/pluginDownloads';
 import {uninstallPlugin} from '../../reducers/pluginManager';

 const {SubMenu} = Menu;
@@ -43,6 +52,7 @@ export const PluginList = memo(function PluginList({
  const dispatch = useDispatch();
  const connections = useStore((state) => state.connections);
  const plugins = useStore((state) => state.plugins);
+  const downloads = useStore((state) => state.pluginDownloads);

  const {
    devicePlugins,
@@ -50,7 +60,7 @@ export const PluginList = memo(function PluginList({
    enabledPlugins,
    disabledPlugins,
    unavailablePlugins,
-    uninstalledPlugins,
+    downloadablePlugins,
  } = useMemoize(computePluginLists, [
    activeDevice,
    metroDevice,
@@ -60,6 +70,22 @@ export const PluginList = memo(function PluginList({
  ]);
  const isArchived = !!activeDevice?.isArchived;

+  const annotatedDownloadablePlugins = useMemoize<
+    [Record<string, DownloadablePluginState>, DownloadablePluginDetails[]],
+    [plugin: DownloadablePluginDetails, downloadStatus?: PluginDownloadStatus][]
+  >(
+    (downloads, downloadablePlugins) => {
+      const downloadMap = new Map(
+        Object.values(downloads).map((x) => [x.plugin.id, x]),
+      );
+      return downloadablePlugins.map((plugin) => [
+        plugin,
+        downloadMap.get(plugin.id)?.status,
+      ]);
+    },
+    [downloads, downloadablePlugins],
+  );
+
  const handleAppPluginClick = useCallback(
    (pluginId) => {
      dispatch(
@@ -99,15 +125,15 @@ export const PluginList = memo(function PluginList({
  );
  const handleInstallPlugin = useCallback(
    (id: string) => {
-      const plugin = uninstalledPlugins.find((p) => p.id === id)!;
+      const plugin = downloadablePlugins.find((p) => p.id === id)!;
      dispatch(
        startPluginDownload({
          plugin,
-          enableDownloadedPlugin: true,
+          startedByUser: true,
        }),
      );
    },
-    [uninstalledPlugins, dispatch],
+    [downloadablePlugins, dispatch],
  );
  const handleUninstallPlugin = useCallback(
    (id: string) => {
@@ -234,8 +260,9 @@ export const PluginList = memo(function PluginList({
          <PluginGroup
            key="uninstalled"
            title="Detected in App"
-            hint="The plugins below are supported by the selected device / application, but not installed in Flipper.">
-            {uninstalledPlugins.map((plugin) => (
+            hint="The plugins below are supported by the selected device / application, but not installed in Flipper.
+            To install plugin, hover it and click to the 'Download' icon.">
+            {annotatedDownloadablePlugins.map(([plugin, downloadStatus]) => (
              <PluginEntry
                key={plugin.id}
                plugin={plugin}
@@ -244,9 +271,15 @@ export const PluginList = memo(function PluginList({
                actions={
                  <ActionButton
                    id={plugin.id}
-                    title="Install and enable plugin"
+                    title="Download and install plugin"
                    onClick={handleInstallPlugin}
-                    icon={<PlusOutlined size={16} style={{marginRight: 0}} />}
+                    icon={
+                      downloadStatus ? (
+                        <LoadingOutlined size={16} style={{marginRight: 0}} />
+                      ) : (
+                        <DownloadOutlined size={16} style={{marginRight: 0}} />
+                      )
+                    }
                  />
                }
                disabled
@@ -432,7 +465,7 @@ export function computePluginLists(
  const enabledPlugins: ClientPluginDefinition[] = [];
  const disabledPlugins: ClientPluginDefinition[] = [];
  const unavailablePlugins: [plugin: PluginDetails, reason: string][] = [];
-  const uninstalledPlugins: DownloadablePluginDetails[] = [];
+  const downloadablePlugins: DownloadablePluginDetails[] = [];

  if (device) {
    // find all device plugins that aren't part of the current device / metro
@@ -504,7 +537,7 @@ export function computePluginLists(
    );
    uninstalledMarketplacePlugins.forEach((plugin) => {
      if (client.supportsPlugin(plugin.id)) {
-        uninstalledPlugins.push(plugin);
+        downloadablePlugins.push(plugin);
      } else {
        unavailablePlugins.push([
          plugin,
@@ -521,7 +554,7 @@ export function computePluginLists(
  unavailablePlugins.sort(([a], [b]) => {
    return getPluginTitle(a) > getPluginTitle(b) ? 1 : -1;
  });
-  uninstalledPlugins.sort((a, b) => {
+  downloadablePlugins.sort((a, b) => {
    return getPluginTitle(a) > getPluginTitle(b) ? 1 : -1;
  });

@@ -531,7 +564,7 @@ export function computePluginLists(
    enabledPlugins,
    disabledPlugins,
    unavailablePlugins,
-    uninstalledPlugins,
+    downloadablePlugins,
  };
 }

--- a/desktop/app/src/sandy-chrome/appinspect/tests/PluginList.spec.tsx
+++ b/desktop/app/src/sandy-chrome/appinspect/tests/PluginList.spec.tsx
@@ -168,7 +168,7 @@ describe('basic findBestDevice with metro present', () => {
        state.connections.userStarredPlugins,
      ),
    ).toEqual({
-      uninstalledPlugins: [],
+      downloadablePlugins: [],
      devicePlugins: [logsPlugin],
      metroPlugins: [logsPlugin],
      enabledPlugins: [],
@@ -237,12 +237,12 @@ describe('basic findBestDevice with metro present', () => {
      noopPlugin,
    );

-    const supportedUninstalledPlugin = createMockDownloadablePluginDetails({
+    const supportedDownloadablePlugin = createMockDownloadablePluginDetails({
      id: 'supportedUninstalledPlugin',
      title: 'Supported Uninstalled Plugin',
    });

-    const unsupportedUninstalledPlugin = createMockDownloadablePluginDetails({
+    const unsupportedDownloadablePlugin = createMockDownloadablePluginDetails({
      id: 'unsupportedUninstalledPlugin',
      title: 'Unsupported Uninstalled Plugin',
    });
@@ -258,8 +258,8 @@ describe('basic findBestDevice with metro present', () => {
    flipper.store.dispatch(addGatekeepedPlugins([gateKeepedPlugin]));
    flipper.store.dispatch(
      registerMarketplacePlugins([
-        supportedUninstalledPlugin,
-        unsupportedUninstalledPlugin,
+        supportedDownloadablePlugin,
+        unsupportedDownloadablePlugin,
      ]),
    );

@@ -297,11 +297,11 @@ describe('basic findBestDevice with metro present', () => {
          "Plugin 'Unsupported Plugin' is installed in Flipper, but not supported by the client application",
        ],
        [
-          unsupportedUninstalledPlugin,
+          unsupportedDownloadablePlugin,
          "Plugin 'Unsupported Uninstalled Plugin' is not installed in Flipper and not supported by the client application",
        ],
      ],
-      uninstalledPlugins: [supportedUninstalledPlugin],
+      downloadablePlugins: [supportedDownloadablePlugin],
    });

    flipper.store.dispatch(
--- a/desktop/app/src/ui/components/Link.tsx
+++ b/desktop/app/src/ui/components/Link.tsx
@@ -30,6 +30,7 @@ export default function Link(props: {
  href: string;
  children?: React.ReactNode;
  style?: React.CSSProperties;
+  onClick?: ((event: React.MouseEvent<any>) => void) | undefined;
 }) {
  const isSandy = useIsSandy();
  const onClick = useCallback(
@@ -42,9 +43,9 @@ export default function Link(props: {
  );

  return isSandy ? (
-    <AntOriginalLink {...props} onClick={onClick} />
+    <AntOriginalLink {...props} onClick={props.onClick ?? onClick} />
  ) : (
-    <StyledLink onClick={onClick} style={props.style}>
+    <StyledLink onClick={props.onClick ?? onClick} style={props.style}>
      {props.children || props.href}
    </StyledLink>
  );