Make sure plugin name selections are displayed consistently with sidebar, [Flipper] Make sure plugins have the same name everywhere

Summary:
Not all plugin names are created equal in flipper. For example, plugins would bear different names in the sidebar and in the plugin selection when making a support request / flipper trace. Fixed this and also introduced a `getPluginTitle` utility that produces this name consistently.

Plugin listview now also sort their items consitently with the sidebar.

Probably also fixed an error in the flipper export screen, where a correct TS error was supressed.

Reviewed By: jknoxville

Differential Revision: D19499404

fbshipit-source-id: c5b23a170d41d96799eb7899e249f70778717d45

src/NotificationsHub.tsx

@@ -34,6 +34,7 @@ import {selectPlugin} from './reducers/connections';
 import {State as StoreState} from './reducers/index';
 import textContent from './utils/textContent';
 import createPaste from './fb-stubs/createPaste';
+import {getPluginTitle} from './utils/pluginUtils';
 
 type OwnProps = {
   onClear: () => void;
@@ -429,7 +430,7 @@ class NotificationItem extends Component<
     const items = [];
     if (props.onHidePlugin && props.plugin) {
       items.push({
-        label: `Hide ${props.plugin.title || props.plugin.id} plugin`,
+        label: `Hide ${getPluginTitle(props.plugin)} plugin`,
         click: this.props.onHidePlugin,
       });
     }
@@ -534,7 +535,7 @@ class NotificationItem extends Component<
                 <FlexRow>
                   {action && (
                     <Button onClick={this.openDeeplink}>
-                      Open in {plugin.title}
+                      Open in {getPluginTitle(plugin)}
                     </Button>
                   )}
                   <ButtonGroup>
@@ -543,7 +544,7 @@ class NotificationItem extends Component<
                     )}
                     {onHidePlugin && (
                       <Button onClick={onHidePlugin}>
-                        Hide {plugin.title}
+                        Hide {getPluginTitle(plugin)}
                       </Button>
                     )}
                   </ButtonGroup>

src/chrome/ExportDataPluginSheet.tsx

@@ -58,12 +58,12 @@ const Container = styled(FlexColumn)({
 });
 
 type State = {
-  availablePluginsToExport: Array<string>;
+  availablePluginsToExport: Array<{id: string; label: string}>;
 };
 
 class ExportDataPluginSheet extends Component<Props, State> {
   state: State = {availablePluginsToExport: []};
-  static getDerivedStateFromProps(props: Props, _state: State) {
+  static getDerivedStateFromProps(props: Props, _state: State): State {
     const {plugins, pluginStates, pluginMessageQueue, selectedClient} = props;
     const availablePluginsToExport = getActivePersistentPlugins(
       pluginStates,

src/chrome/ListView.tsx

@@ -37,7 +37,7 @@ type Props = {
   onSubmit?: () => void;
   onChange: (elements: Array<string>) => void;
   onHide: () => any;
-  elements: Array<string>;
+  elements: Array<{label: string; id: string}>;
   title?: string;
 } & SubType;
 
@@ -80,14 +80,15 @@ const Padder = styled.div<{
 }));
 
 type RowComponentProps = {
-  name: string;
+  id: string;
+  label: string;
   selected: boolean;
   onChange: (name: string, selected: boolean) => void;
 };
 
 class RowComponent extends Component<RowComponentProps> {
   render() {
-    const {name, selected, onChange} = this.props;
+    const {id, label, selected, onChange} = this.props;
     return (
       <FlexColumn>
         <Padder
@@ -96,12 +97,12 @@ class RowComponent extends Component<RowComponentProps> {
           paddingBottom={8}
           paddingLeft={8}>
           <FlexRow>
-            <Text> {name} </Text>
+            <Text> {label} </Text>
             <Spacer />
             <Checkbox
               checked={selected}
               onChange={selected => {
-                onChange(name, selected);
+                onChange(id, selected);
               }}
             />
           </FlexRow>
@@ -151,10 +152,11 @@ export default class ListView extends Component<Props, State> {
         <FlexColumn>
           {this.props.title && <Title>{this.props.title}</Title>}
           <RowComponentContainer>
-            {this.props.elements.map(id => {
+            {this.props.elements.map(({id, label}) => {
               return (
                 <RowComponent
-                  name={id}
+                  id={id}
+                  label={label}
                   key={id}
                   selected={this.state.selectedElements.has(id)}
                   onChange={this.handleChange}

src/chrome/mainsidebar/MainSidebar.tsx

@@ -10,7 +10,7 @@
 import BaseDevice from '../../devices/BaseDevice';
 import Client from '../../Client';
 import {UninitializedClient} from '../../UninitializedClient';
-import {sortPluginsByName} from '../../plugin';
+import {sortPluginsByName} from '../../utils/pluginUtils';
 import {PluginNotification} from '../../reducers/notifications';
 import {ActiveSheet} from '../../reducers/application';
 import {State as Store} from '../../reducers';

src/chrome/mainsidebar/MainSidebar2.tsx

@@ -10,7 +10,7 @@
 import BaseDevice from '../../devices/BaseDevice';
 import Client from '../../Client';
 import {UninitializedClient} from '../../UninitializedClient';
-import {sortPluginsByName} from '../../plugin';
+import {sortPluginsByName} from '../../utils/pluginUtils';
 import {PluginNotification} from '../../reducers/notifications';
 import {ActiveSheet} from '../../reducers/application';
 import {State as Store} from '../../reducers';

src/chrome/mainsidebar/sidebarUtils.tsx

@@ -27,6 +27,7 @@ import {
 } from 'flipper';
 import {StaticView} from '../../reducers/connections';
 import {BackgroundColorProperty} from 'csstype';
+import {getPluginTitle} from '../../utils/pluginUtils';
 
 export type FlipperPlugins = typeof FlipperPlugin[];
 export type PluginsByCategory = [string, FlipperPlugins][];
@@ -167,7 +168,7 @@ export const PluginSidebarListItem: React.FC<{
         backgroundColor={starred === false ? colors.light20 : iconColor}
         color={colors.white}
       />
-      <PluginName>{plugin.title || plugin.id}</PluginName>
+      <PluginName>{getPluginTitle(plugin)}</PluginName>
       {starred !== undefined && (!starred || isActive) && (
         <ToggleButton
           onClick={onFavorite}

src/devices/BaseDevice.tsx

@@ -9,7 +9,7 @@
 
 import stream from 'stream';
 import {FlipperDevicePlugin} from 'flipper';
-import {sortPluginsByName} from '../plugin';
+import {sortPluginsByName} from '../utils/pluginUtils';
 
 export type LogLevel =
   | 'unknown'

src/dispatcher/notifications.tsx

@@ -23,6 +23,7 @@ import {textContent} from '../utils/index';
 import GK from '../fb-stubs/GK';
 import {deconstructPluginKey} from '../utils/clientUtils';
 import NotificationScreen from '../chrome/NotificationScreen';
+import {getPluginTitle} from '../utils/pluginUtils';
 
 type NotificationEvents = 'show' | 'click' | 'close' | 'reply' | 'action';
 const NOTIFICATION_THROTTLE = 5 * 1000; // in milliseconds
@@ -173,7 +174,9 @@ export default (store: Store, logger: Logger) => {
               },
               {
                 type: 'button',
-                text: `Hide all ${plugin != null ? plugin.title : ''}`,
+                text: `Hide all ${
+                  plugin != null ? getPluginTitle(plugin) : ''
+                }`,
               },
             ],
             closeButtonText: 'Hide',

src/plugin.tsx

@@ -19,7 +19,6 @@ import {serialize, deserialize} from './utils/serialization';
 import {Idler} from './utils/Idler';
 import {StaticView} from './reducers/connections';
 import {State as ReduxState} from './reducers';
-import {PersistedState} from './plugins/layout';
 import {DEFAULT_MAX_QUEUE_SIZE} from './reducers/pluginMessageQueue';
 type Parameters = any;
 
@@ -275,10 +274,3 @@ export class FlipperPlugin<
     this.init();
   }
 }
-
-export function sortPluginsByName(
-  a: typeof FlipperBasePlugin,
-  b: typeof FlipperBasePlugin,
-): number {
-  return (a.title || a.id) > (b.title || b.id) ? 1 : -1;
-}

src/utils/__tests__/pluginUtils.node.js

@@ -18,33 +18,45 @@ import type {State as PluginMessageQueueState} from '../../reducers/pluginStates
 import {FlipperBasePlugin} from 'flipper';
 import type {ReduxState} from '../../reducers/index.tsx';
 
-class MockFlipperPluginWithDefaultPersistedState extends FlipperBasePlugin<
+function createMockFlipperPluginWithDefaultPersistedState(id: string) {
+  return class MockFlipperPluginWithDefaultPersistedState extends FlipperBasePlugin<
     *,
     *,
     {msg: string},
   > {
+    static id = id;
     static defaultPersistedState = {msg: 'MockFlipperPluginWithPersistedState'};
+  };
 }
 
-class MockFlipperPluginWithExportPersistedState extends FlipperBasePlugin<
+function createMockFlipperPluginWithExportPersistedState(id: string) {
+  return class MockFlipperPluginWithExportPersistedState extends FlipperBasePlugin<
     *,
     *,
     {msg: string},
   > {
+    static id = id;
     static exportPersistedState = (
       callClient: (string, ?Object) => Promise<Object>,
       persistedState: ?{msg: string},
       store: ?ReduxState,
     ): Promise<?{msg: string}> => {
-    return Promise.resolve({msg: 'MockFlipperPluginWithExportPersistedState'});
+      return Promise.resolve({
+        msg: 'MockFlipperPluginWithExportPersistedState',
+      });
+    };
   };
 }
 
-class MockFlipperPluginWithNoPersistedState extends FlipperBasePlugin<
+function createMockFlipperPluginWithNoPersistedState(id: string) {
+  return class MockFlipperPluginWithNoPersistedState extends FlipperBasePlugin<
     *,
     *,
     *,
-> {}
+  > {
+    static id = id;
+  };
+}
 
 function mockPluginState(
   gatekeepedPlugins: Array<PluginDefinition>,
@@ -53,12 +65,24 @@ function mockPluginState(
 ): PluginsState {
   return {
     devicePlugins: new Map([
-      ['DevicePlugin1', MockFlipperPluginWithDefaultPersistedState],
+      [
-      ['DevicePlugin2', MockFlipperPluginWithDefaultPersistedState],
+        'DevicePlugin1',
+        createMockFlipperPluginWithDefaultPersistedState('DevicePlugin1'),
+      ],
+      [
+        'DevicePlugin2',
+        createMockFlipperPluginWithDefaultPersistedState('DevicePlugin2'),
+      ],
     ]),
     clientPlugins: new Map([
-      ['ClientPlugin1', MockFlipperPluginWithDefaultPersistedState],
+      [
-      ['ClientPlugin2', MockFlipperPluginWithDefaultPersistedState],
+        'ClientPlugin1',
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin1'),
+      ],
+      [
+        'ClientPlugin2',
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin2'),
+      ],
     ]),
     gatekeepedPlugins,
     disabledPlugins,
@@ -98,12 +122,24 @@ test('getPersistentPlugins with no plugins getting excluded', () => {
 test('getPersistentPlugins, where the plugins with exportPersistedState not getting excluded', () => {
   const state: PluginsState = {
     devicePlugins: new Map([
-      ['DevicePlugin1', MockFlipperPluginWithExportPersistedState],
+      [
-      ['DevicePlugin2', MockFlipperPluginWithExportPersistedState],
+        'DevicePlugin1',
+        createMockFlipperPluginWithExportPersistedState('DevicePlugin1'),
+      ],
+      [
+        'DevicePlugin2',
+        createMockFlipperPluginWithExportPersistedState('DevicePlugin2'),
+      ],
     ]),
     clientPlugins: new Map([
-      ['ClientPlugin1', MockFlipperPluginWithExportPersistedState],
+      [
-      ['ClientPlugin2', MockFlipperPluginWithExportPersistedState],
+        'ClientPlugin1',
+        createMockFlipperPluginWithExportPersistedState('ClientPlugin1'),
+      ],
+      [
+        'ClientPlugin2',
+        createMockFlipperPluginWithExportPersistedState('ClientPlugin2'),
+      ],
     ]),
     gatekeepedPlugins: [],
     disabledPlugins: [],
@@ -122,12 +158,24 @@ test('getPersistentPlugins, where the plugins with exportPersistedState not gett
 test('getPersistentPlugins, where the non persistent plugins getting excluded', () => {
   const state: PluginsState = {
     devicePlugins: new Map([
-      ['DevicePlugin1', MockFlipperPluginWithNoPersistedState],
+      [
-      ['DevicePlugin2', MockFlipperPluginWithDefaultPersistedState],
+        'DevicePlugin1',
+        createMockFlipperPluginWithNoPersistedState('DevicePlugin1'),
+      ],
+      [
+        'DevicePlugin2',
+        createMockFlipperPluginWithDefaultPersistedState('DevicePlugin2'),
+      ],
     ]),
     clientPlugins: new Map([
-      ['ClientPlugin1', MockFlipperPluginWithDefaultPersistedState],
+      [
-      ['ClientPlugin2', MockFlipperPluginWithNoPersistedState],
+        'ClientPlugin1',
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin1'),
+      ],
+      [
+        'ClientPlugin2',
+        createMockFlipperPluginWithNoPersistedState('ClientPlugin2'),
+      ],
     ]),
     gatekeepedPlugins: [],
     disabledPlugins: [],
@@ -141,12 +189,24 @@ test('getPersistentPlugins, where the non persistent plugins getting excluded',
 test('getActivePersistentPlugins, where the non persistent plugins getting excluded', () => {
   const state: PluginsState = {
     devicePlugins: new Map([
-      ['DevicePlugin1', MockFlipperPluginWithNoPersistedState],
+      [
-      ['DevicePlugin2', MockFlipperPluginWithDefaultPersistedState],
+        'DevicePlugin1',
+        createMockFlipperPluginWithNoPersistedState('DevicePlugin1'),
+      ],
+      [
+        'DevicePlugin2',
+        createMockFlipperPluginWithDefaultPersistedState('DevicePlugin2'),
+      ],
     ]),
     clientPlugins: new Map([
-      ['ClientPlugin1', MockFlipperPluginWithDefaultPersistedState],
+      [
-      ['ClientPlugin2', MockFlipperPluginWithNoPersistedState],
+        'ClientPlugin1',
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin1'),
+      ],
+      [
+        'ClientPlugin2',
+        createMockFlipperPluginWithNoPersistedState('ClientPlugin2'),
+      ],
     ]),
     gatekeepedPlugins: [],
     disabledPlugins: [],
@@ -161,19 +221,43 @@ test('getActivePersistentPlugins, where the non persistent plugins getting exclu
   };
   const queues: PluginMessageQueueState = {};
   const list = getActivePersistentPlugins(plugins, queues, state);
-  expect(list).toEqual(['ClientPlugin1', 'DevicePlugin2']);
+  expect(list).toEqual([
+    {
+      id: 'ClientPlugin1',
+      label: 'ClientPlugin1',
+    },
+    {
+      id: 'DevicePlugin2',
+      label: 'DevicePlugin2',
+    },
+  ]);
 });
 
 test('getActivePersistentPlugins, where the plugins not in pluginState or queue gets excluded', () => {
   const state: PluginsState = {
     devicePlugins: new Map([
-      ['DevicePlugin1', MockFlipperPluginWithDefaultPersistedState],
+      [
-      ['DevicePlugin2', MockFlipperPluginWithDefaultPersistedState],
+        'DevicePlugin1',
+        createMockFlipperPluginWithDefaultPersistedState('DevicePlugin1'),
+      ],
+      [
+        'DevicePlugin2',
+        createMockFlipperPluginWithDefaultPersistedState('DevicePlugin2'),
+      ],
     ]),
     clientPlugins: new Map([
-      ['ClientPlugin1', MockFlipperPluginWithDefaultPersistedState],
+      [
-      ['ClientPlugin2', MockFlipperPluginWithDefaultPersistedState],
+        'ClientPlugin1',
-      ['ClientPlugin3', MockFlipperPluginWithDefaultPersistedState],
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin1'),
+      ],
+      [
+        'ClientPlugin2',
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin2'),
+      ],
+      [
+        'ClientPlugin3',
+        createMockFlipperPluginWithDefaultPersistedState('ClientPlugin3'),
+      ],
     ]),
     gatekeepedPlugins: [],
     disabledPlugins: [],
@@ -190,5 +274,18 @@ test('getActivePersistentPlugins, where the plugins not in pluginState or queue
     ],
   };
   const list = getActivePersistentPlugins(plugins, queues, state);
-  expect(list).toEqual(['ClientPlugin2', 'ClientPlugin3', 'DevicePlugin1']);
+  expect(list).toEqual([
+    {
+      id: 'ClientPlugin2',
+      label: 'ClientPlugin2',
+    },
+    {
+      id: 'ClientPlugin3',
+      label: 'ClientPlugin3',
+    },
+    {
+      id: 'DevicePlugin1',
+      label: 'DevicePlugin1',
+    },
+  ]);
 });

src/utils/exportData.tsx

@@ -42,6 +42,7 @@ import {setSelectPluginsToExportActiveSheet} from '../reducers/application';
 import {deconstructClientId, deconstructPluginKey} from '../utils/clientUtils';
 import {performance} from 'perf_hooks';
 import {processMessageQueue} from './messageQueue';
+import {getPluginTitle} from './pluginUtils';
 
 export const IMPORT_FLIPPER_TRACE_EVENT = 'import-flipper-trace';
 export const EXPORT_FLIPPER_TRACE_EVENT = 'export-flipper-trace';
@@ -85,7 +86,8 @@ type SerializePluginStatesOptions = {
 
 type PluginsToProcess = {
   pluginKey: string;
-  plugin: string;
+  pluginId: string;
+  pluginName: string;
   pluginClass: typeof FlipperPlugin | typeof FlipperDevicePlugin;
   client: Client;
 }[];
@@ -398,33 +400,39 @@ export async function fetchMetadata(
   const newPluginState = {...pluginStates};
   const errorArray: Array<Error> = [];
 
-  for (const {plugin, pluginClass, client, pluginKey} of pluginsToProcess) {
+  for (const {
+    pluginName,
+    pluginId,
+    pluginClass,
+    client,
+    pluginKey,
+  } of pluginsToProcess) {
     const exportState = pluginClass ? pluginClass.exportPersistedState : null;
     if (exportState) {
       const fetchMetaDataMarker = `${EXPORT_FLIPPER_TRACE_EVENT}:fetch-meta-data-per-plugin`;
       performance.mark(fetchMetaDataMarker);
       try {
         statusUpdate &&
-          statusUpdate(`Fetching metadata for plugin ${plugin}...`);
+          statusUpdate(`Fetching metadata for plugin ${pluginName}...`);
         const data = await promiseTimeout(
           240000, // Fetching MobileConfig data takes ~ 3 mins, thus keeping timeout at 4 mins.
           exportState(
-            callClient(client, plugin),
+            callClient(client, pluginId),
             newPluginState[pluginKey],
             state,
             idler,
             statusUpdate,
           ),
-          `Timed out while collecting data for ${plugin}`,
+          `Timed out while collecting data for ${pluginName}`,
         );
         getLogger().trackTimeSince(fetchMetaDataMarker, fetchMetaDataMarker, {
-          plugin,
+          pluginId,
         });
         newPluginState[pluginKey] = data;
       } catch (e) {
         errorArray.push(e);
         getLogger().trackTimeSince(fetchMetaDataMarker, fetchMetaDataMarker, {
-          plugin,
+          pluginId,
           error: e,
         });
         continue;
@@ -441,7 +449,12 @@ async function processQueues(
   statusUpdate?: (msg: string) => void,
   idler?: Idler,
 ) {
-  for (const {plugin, pluginKey, pluginClass} of pluginsToProcess) {
+  for (const {
+    pluginName,
+    pluginId,
+    pluginKey,
+    pluginClass,
+  } of pluginsToProcess) {
     if (pluginClass.persistedStateReducer) {
       const processQueueMarker = `${EXPORT_FLIPPER_TRACE_EVENT}:process-queue-per-plugin`;
       performance.mark(processQueueMarker);
@@ -454,14 +467,14 @@ async function processQueues(
           statusUpdate?.(
             `Processing event ${current} / ${total} (${Math.round(
               (current / total) * 100,
-            )}%) for plugin ${plugin}`,
+            )}%) for plugin ${pluginName}`,
           );
         },
         idler,
       );
 
       getLogger().trackTimeSince(processQueueMarker, processQueueMarker, {
-        plugin,
+        pluginId,
       });
     }
   }
@@ -506,7 +519,8 @@ export function determinePluginsToProcess(
         pluginsToProcess.push({
           pluginKey: key,
           client,
-          plugin,
+          pluginId: plugin,
+          pluginName: getPluginTitle(pluginClass),
           pluginClass,
         });
       }

src/utils/pluginUtils.tsx

@@ -77,13 +77,15 @@ export function getActivePersistentPlugins(
   pluginsMessageQueue: PluginMessageQueueState,
   plugins: PluginsState,
   selectedClient?: Client,
-): Array<string> {
+): {id: string; label: string}[] {
   const pluginsMap: Map<
     string,
     typeof FlipperDevicePlugin | typeof FlipperPlugin
   > = pluginsClassMap(plugins);
-  return getPersistentPlugins(plugins).filter(plugin => {
+  return getPersistentPlugins(plugins)
-    const pluginClass = pluginsMap.get(plugin);
+    .map(pluginName => pluginsMap.get(pluginName)!)
+    .sort(sortPluginsByName)
+    .map(plugin => {
       const keys = [
         ...new Set([
           ...Object.keys(pluginsState),
@@ -92,23 +94,29 @@ export function getActivePersistentPlugins(
       ]
         .filter(k => !selectedClient || k.includes(selectedClient.id))
         .map(key => deconstructPluginKey(key).pluginName);
-    let result = plugin == 'DeviceLogs';
+      let result = plugin.id == 'DeviceLogs';
       const pluginsWithExportPersistedState =
-      pluginClass && pluginClass.exportPersistedState != undefined;
+        plugin && plugin.exportPersistedState != undefined;
-    const pluginsWithReduxData = keys.includes(plugin);
+      const pluginsWithReduxData = keys.includes(plugin.id);
       if (!result && selectedClient) {
         // If there is a selected client, active persistent plugin is the plugin which is active for selectedClient and also persistent.
         result =
-        selectedClient.plugins.includes(plugin) &&
+          selectedClient.plugins.includes(plugin.id) &&
           (pluginsWithExportPersistedState || pluginsWithReduxData);
       } else if (!result && !selectedClient) {
         // If there is no selected client, active persistent plugin is the plugin which is just persistent.
         result =
-        (pluginClass && pluginClass.exportPersistedState != undefined) ||
+          (plugin && plugin.exportPersistedState != undefined) ||
-        keys.includes(plugin);
+          keys.includes(plugin.id);
       }
-    return result;
+      return (result
-  });
+        ? {
+            id: plugin.id,
+            label: getPluginTitle(plugin),
+          }
+        : undefined)!;
+    })
+    .filter(Boolean);
 }
 
 export function getPersistentPlugins(plugins: PluginsState): Array<string> {
@@ -144,3 +152,27 @@ export function getPersistentPlugins(plugins: PluginsState): Array<string> {
     );
   });
 }
+
+export function getPluginTitle(pluginClass: typeof FlipperBasePlugin) {
+  return pluginClass.title || pluginClass.id;
+}
+
+export function sortPluginsByName(
+  a: typeof FlipperBasePlugin,
+  b: typeof FlipperBasePlugin,
+): number {
+  // make sure Device plugins are sorted before normal plugins
+  if (
+    a.prototype instanceof FlipperDevicePlugin &&
+    !(b.prototype instanceof FlipperDevicePlugin)
+  ) {
+    return -1;
+  }
+  if (
+    b.prototype instanceof FlipperDevicePlugin &&
+    !(a.prototype instanceof FlipperDevicePlugin)
+  ) {
+    return 1;
+  }
+  return getPluginTitle(a) > getPluginTitle(b) ? 1 : -1;
+}