Only store messages for plugins that are starred

Summary:
From several reviews / early feedbacks it was suggested several times to use the star mechanism to distinguish which plugins are allowed to send messages. This diff implements that:

- If a plugin is not selected, and not starred, it will drop the messages it received in the background
- This logic is still behind the same GK
- I think this change warrants upping the message queue limits significantly
- A future optimization would be to disable sending messages from the device side of things to reduce bridge usage, but that change is probably a lot more complicated with less impact
- In the next diff I'll make clear from UI perspective that unstarred plugins don't queue messages

In the attach video one can see how graphQL plugin keeps storing messages if it is starred, but if it isn't starred and not selected either, it will skip messages

{F225692819}

Reviewed By: jknoxville

Differential Revision: D19250928

fbshipit-source-id: 7e6af0eb6830dc79951cfd206e05b44061f1b14a

src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap

@@ -21,7 +21,7 @@ Object {
       "logs": Array [],
       "os": "Android",
       "serial": "serial",
-      "title": "title",
+      "title": "MockAndroidDevice",
     },
   ],
   "errors": Array [],
@@ -31,15 +31,19 @@ Object {
     "logs": Array [],
     "os": "Android",
     "serial": "serial",
-    "title": "title",
+    "title": "MockAndroidDevice",
   },
   "selectedPlugin": "TestPlugin",
   "staticView": null,
   "uninitializedClients": Array [],
-  "userPreferredApp": null,
-  "userPreferredDevice": null,
+  "userPreferredApp": "TestApp#Android#unit_test#serial",
+  "userPreferredDevice": "MockAndroidDevice",
   "userPreferredPlugin": "TestPlugin",
-  "userStarredPlugins": Object {},
+  "userStarredPlugins": Object {
+    "TestApp": Array [
+      "TestPlugin",
+    ],
+  },
 }
 `;
 

src/test-utils/createMockFlipperWithPlugin.tsx

@@ -9,7 +9,12 @@
 
 import {createStore} from 'redux';
 
-import {selectPlugin} from '../reducers/connections';
+import {
+  selectPlugin,
+  starPlugin,
+  selectDevice,
+  selectClient,
+} from '../reducers/connections';
 import BaseDevice from '../devices/BaseDevice';
 
 import reducers, {Store} from '../reducers/index';
@@ -38,7 +43,12 @@ export async function createMockFlipperWithPlugin(
   }) => Promise<void>,
 ) {
   const store = createStore(reducers);
-  const device = new BaseDevice('serial', 'physical', 'title', 'Android');
+  const device = new BaseDevice(
+    'serial',
+    'physical',
+    'MockAndroidDevice',
+    'Android',
+  );
   const logger = createStubLogger();
 
   store.dispatch(registerPlugins([pluginClazz]));
@@ -67,7 +77,7 @@ export async function createMockFlipperWithPlugin(
     null, // create a stub connection to avoid this plugin to be archived?
     logger,
     store,
-    [pluginClazz.name],
+    [pluginClazz.id],
     device,
   );
 
@@ -79,14 +89,26 @@ export async function createMockFlipperWithPlugin(
     },
   } as any;
 
+  // As convenience, by default we select the new client, star the plugin, and select it
   store.dispatch({
     type: 'NEW_CLIENT',
     payload: client,
   });
 
+  store.dispatch(selectDevice(device));
+
+  store.dispatch(selectClient(client.id));
+
+  store.dispatch(
+    starPlugin({
+      selectedPlugin: pluginClazz.id,
+      selectedApp: client.query.app,
+    }),
+  );
+
   store.dispatch(
     selectPlugin({
-      selectedPlugin: pluginClazz.name,
+      selectedPlugin: pluginClazz.id,
       selectedApp: client.query.app,
       deepLinkPayload: null,
     }),

src/utils/__tests__/messageQueue.node.tsx

@@ -9,8 +9,8 @@
 
 import {FlipperPlugin} from '../../plugin';
 import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin';
-import {GK} from 'flipper';
-import {selectPlugin} from '../../reducers/connections';
+import {GK, Store, Client} from 'flipper';
+import {selectPlugin, starPlugin} from '../../reducers/connections';
 import {processMessageQueue} from '../messageQueue';
 import {getPluginKey} from '../pluginUtils';
 import {TestIdler} from '../Idler';
@@ -48,6 +48,35 @@ class TestPlugin extends FlipperPlugin<any, any, any> {
   }
 }
 
+function starTestPlugin(store: Store, client: Client) {
+  store.dispatch(
+    starPlugin({
+      selectedPlugin: TestPlugin.id,
+      selectedApp: client.query.app,
+    }),
+  );
+}
+
+function selectDeviceLogs(store: Store) {
+  store.dispatch(
+    selectPlugin({
+      selectedPlugin: 'DeviceLogs',
+      selectedApp: null,
+      deepLinkPayload: null,
+    }),
+  );
+}
+
+function selectTestPlugin(store: Store, client: Client) {
+  store.dispatch(
+    selectPlugin({
+      selectedPlugin: TestPlugin.id,
+      selectedApp: client.query.app,
+      deepLinkPayload: null,
+    }),
+  );
+}
+
 test('will process event with GK disabled', async () => {
   await createMockFlipperWithPlugin(
     TestPlugin,
@@ -87,18 +116,12 @@ test('queue - events are processed immediately if plugin is selected', async ()
   );
 });
 
-test('queue - events are NOT processed immediately if plugin is NOT selected', async () => {
+test('queue - events are NOT processed immediately if plugin is NOT selected (but starred)', async () => {
   await createMockFlipperWithPlugin(
     TestPlugin,
     async ({client, device, store, sendMessage}) => {
       await GK.withWhitelistedGK('flipper_event_queue', async () => {
-        store.dispatch(
-          selectPlugin({
-            selectedPlugin: 'DeviceLogs',
-            selectedApp: null,
-            deepLinkPayload: null,
-          }),
-        );
+        selectDeviceLogs(store);
         expect(store.getState().connections.selectedPlugin).not.toBe(
           'TestPlugin',
         );
@@ -137,6 +160,32 @@ test('queue - events are NOT processed immediately if plugin is NOT selected', a
         expect(store.getState().pluginMessageQueue).toEqual({
           [pluginKey]: [],
         });
+
+        // unstar, but, messages still arrives because selected
+        starTestPlugin(store, client);
+        selectTestPlugin(store, client);
+        sendMessage('inc', {delta: 3});
+        // active, immediately processed
+        expect(store.getState().pluginStates).toEqual({
+          [pluginKey]: {
+            count: 6,
+          },
+        });
+
+        // different plugin, and not starred, message will never arrive
+        selectDeviceLogs(store);
+        sendMessage('inc', {delta: 4});
+        expect(store.getState().pluginMessageQueue).toEqual({
+          [pluginKey]: [],
+        });
+
+        // star again, plugin still not selected, message is queued
+        starTestPlugin(store, client);
+        sendMessage('inc', {delta: 5});
+
+        expect(store.getState().pluginMessageQueue).toEqual({
+          [pluginKey]: [{method: 'inc', params: {delta: 5}}],
+        });
       });
     },
   );
@@ -147,14 +196,7 @@ test('queue - events processing will be paused', async () => {
     TestPlugin,
     async ({client, device, store, sendMessage}) => {
       await GK.withWhitelistedGK('flipper_event_queue', async () => {
-        // select a different plugin
-        store.dispatch(
-          selectPlugin({
-            selectedPlugin: 'DeviceLogs',
-            selectedApp: null,
-            deepLinkPayload: null,
-          }),
-        );
+        selectDeviceLogs(store);
 
         sendMessage('inc', {});
         sendMessage('inc', {delta: 3});
@@ -208,14 +250,7 @@ test('queue - messages that arrive during processing will be queued', async () =
     TestPlugin,
     async ({client, device, store, sendMessage}) => {
       await GK.withWhitelistedGK('flipper_event_queue', async () => {
-        // select a different plugin
-        store.dispatch(
-          selectPlugin({
-            selectedPlugin: 'DeviceLogs',
-            selectedApp: null,
-            deepLinkPayload: null,
-          }),
-        );
+        selectDeviceLogs(store);
 
         sendMessage('inc', {});
         sendMessage('inc', {delta: 2});
@@ -269,14 +304,7 @@ test('queue - processing can be cancelled', async () => {
     TestPlugin,
     async ({client, device, store, sendMessage}) => {
       await GK.withWhitelistedGK('flipper_event_queue', async () => {
-        // select a different plugin
-        store.dispatch(
-          selectPlugin({
-            selectedPlugin: 'DeviceLogs',
-            selectedApp: null,
-            deepLinkPayload: null,
-          }),
-        );
+        selectDeviceLogs(store);
 
         sendMessage('inc', {});
         sendMessage('inc', {delta: 2});
@@ -317,14 +345,7 @@ test('queue - make sure resetting plugin state clears the message queue', async
     TestPlugin,
     async ({client, device, store, sendMessage}) => {
       await GK.withWhitelistedGK('flipper_event_queue', async () => {
-        // select a different plugin
-        store.dispatch(
-          selectPlugin({
-            selectedPlugin: 'DeviceLogs',
-            selectedApp: null,
-            deepLinkPayload: null,
-          }),
-        );
+        selectDeviceLogs(store);
 
         sendMessage('inc', {});
         sendMessage('inc', {delta: 2});

src/utils/messageQueue.tsx

@@ -7,7 +7,7 @@
  * @format
  */
 
-import {PersistedStateReducer} from '../plugin';
+import {PersistedStateReducer, FlipperDevicePlugin} from '../plugin';
 import {State, MiddlewareAPI} from '../reducers/index';
 import {setPluginState} from '../reducers/pluginStates';
 import {flipperRecorderAddEvent} from './pluginStateRecorder';
@@ -18,6 +18,7 @@ import {
 } from '../reducers/pluginMessageQueue';
 import {Idler, BaseIdler} from './Idler';
 import {getPluginKey} from './pluginUtils';
+import {deconstructClientId} from './clientUtils';
 
 const MAX_BACKGROUND_TASK_TIME = 25;
 
@@ -137,35 +138,46 @@ export function processMessageLater(
   },
   message: {method: string; params?: any},
 ) {
-  // TODO: can we make this better?
-  const selection = store.getState().connections;
-  const selectedPlugin =
-    selection.selectedPlugin &&
-    getPluginKey(
-      selection.selectedApp,
-      selection.selectedDevice,
-      selection.selectedPlugin,
-    );
-  // if the plugin is active, and has no queued messaged, process the message immediately
-  if (
-    selectedPlugin === pluginKey &&
-    getPendingMessages(store, pluginKey).length === 0
-  ) {
-    processMessageImmediately(store, pluginKey, plugin, message);
-  } else {
-    // TODO: possible optimization: drop all messages for non-favorited plugins
-    // TODO: possible optimization: drop messages if queue is too large
-    store.dispatch(
-      queueMessage(
-        pluginKey,
-        message.method,
-        message.params,
-        plugin.maxQueueSize,
-      ),
-    );
+  const isSelected = pluginKey === getSelectedPluginKey(store.getState());
+  switch (true) {
+    case isSelected && getPendingMessages(store, pluginKey).length === 0:
+      processMessageImmediately(store, pluginKey, plugin, message);
+      break;
+    case isSelected:
+    case plugin instanceof FlipperDevicePlugin:
+    case pluginIsStarred(store.getState(), plugin.id):
+      store.dispatch(
+        queueMessage(
+          pluginKey,
+          message.method,
+          message.params,
+          plugin.maxQueueSize,
+        ),
+      );
+      break;
   }
 }
 
+function getSelectedPluginKey(state: State): string | undefined {
+  return state.connections.selectedPlugin
+    ? getPluginKey(
+        state.connections.selectedApp,
+        state.connections.selectedDevice,
+        state.connections.selectedPlugin,
+      )
+    : undefined;
+}
+
+function pluginIsStarred(state: State, pluginId: string): boolean {
+  const {selectedApp} = state.connections;
+  if (!selectedApp) {
+    return false;
+  }
+  const appInfo = deconstructClientId(selectedApp);
+  const starred = state.connections.userStarredPlugins[appInfo.app];
+  return starred && starred.indexOf(pluginId) > -1;
+}
+
 export async function processMessageQueue(
   plugin: {
     defaultPersistedState: any;

src/utils/pluginUtils.tsx

@@ -34,12 +34,12 @@ export function pluginsClassMap(
 }
 
 export function getPluginKey(
-  selectedApp: string | null,
+  selectedAppId: string | null,
   baseDevice: BaseDevice | null,
   pluginID: string,
 ): string {
-  if (selectedApp) {
-    return `${selectedApp}#${pluginID}`;
+  if (selectedAppId) {
+    return `${selectedAppId}#${pluginID}`;
   }
   if (baseDevice) {
     // If selected App is not defined, then the plugin is a device plugin