Fix disabling a plugin nog clearing the message queue

Summary:
While writing unit tests discovered a bug that disabling a plugin doesn't guarantee cleaning the messagequeues (both the buffer in client and the messagequeue reducer).

Fixed that. That was thanks to @#%@#$@#%@ Redux a lot harder than it should be; as 'STAR_PLUGIN' reasons about a plugin + app name, while the message queue reducer would need to deduct the plugin keys from that, but it can't because that mapping is stored in the connections reducers. So I moved the `STAR_PLUGIN` action handling to the root reducer, sot that it can reason about the state of multiple reducers, which looked like the least of all evils. For more ranting about that and possible alternative solutions: https://twitter.com/mweststrate/status/1277556309706117122

Reviewed By: nikoant

Differential Revision: D22284043

fbshipit-source-id: 35d0a8ba3a21a5959d2bb6ef17da3ff5077f48fd

desktop/app/src/Client.tsx

@@ -336,6 +336,12 @@ export default class Client extends EventEmitter {
   }
 
   stopPluginIfNeeded(pluginId: string) {
+    const pluginKey = getPluginKey(
+      this.id,
+      {serial: this.query.device_id},
+      pluginId,
+    );
+    delete this.messageBuffer[pluginKey];
     const instance = this.sandyPluginStates.get(pluginId);
     if (instance) {
       instance.destroy();

desktop/app/src/reducers/connections.tsx

@@ -135,6 +135,7 @@ export type Action =
       payload: number;
     }
   | {
+      // Implemented by rootReducer in `store.tsx`
       type: 'STAR_PLUGIN';
       payload: {
         selectedApp: string;
@@ -277,51 +278,6 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
       });
     }
 
-    case 'STAR_PLUGIN': {
-      const {plugin, selectedApp} = action.payload;
-      const selectedPlugin = plugin.id;
-      const clients = state.clients.filter(
-        (client) => client.query.app === selectedApp,
-      );
-      return produce(state, (draft) => {
-        if (!draft.userStarredPlugins[selectedApp]) {
-          draft.userStarredPlugins[selectedApp] = [];
-        }
-        const plugins = draft.userStarredPlugins[selectedApp];
-        const idx = plugins.indexOf(selectedPlugin);
-        if (idx === -1) {
-          plugins.push(selectedPlugin);
-          // enabling a plugin on one device enables it on all...
-          clients.forEach((client) => {
-            // sandy plugin? initialize it
-            client.startPluginIfNeeded(plugin, true);
-            // background plugin? connect it needed
-            if (
-              !defaultEnabledBackgroundPlugins.includes(selectedPlugin) &&
-              client?.isBackgroundPlugin(selectedPlugin)
-            ) {
-              client.initPlugin(selectedPlugin);
-            }
-          });
-        } else {
-          plugins.splice(idx, 1);
-          // enabling a plugin on one device disables it on all...
-          clients.forEach((client) => {
-            // disconnect background plugins
-            if (
-              !defaultEnabledBackgroundPlugins.includes(selectedPlugin) &&
-              client?.isBackgroundPlugin(selectedPlugin)
-            ) {
-              client.deinitPlugin(selectedPlugin);
-            }
-            // stop sandy plugins
-            // TODO: forget any persisted state as well T68683449
-            client.stopPluginIfNeeded(plugin.id);
-          });
-        }
-      });
-    }
-
     case 'SELECT_USER_PREFERRED_PLUGIN': {
       const {payload} = action;
       return {...state, userPreferredPlugin: payload};

desktop/app/src/store.tsx

@@ -11,9 +11,14 @@ import {createStore} from 'redux';
 import reducers, {Actions, State as StoreState} from './reducers/index';
 import {stateSanitizer} from './utils/reduxDevToolsConfig';
 import isProduction from './utils/isProduction';
+import produce from 'immer';
+import {
+  defaultEnabledBackgroundPlugins,
+  getPluginKey,
+} from './utils/pluginUtils';
 
 export const store = createStore<StoreState, Actions, any, any>(
-  reducers,
+  rootReducer,
   // @ts-ignore Type definition mismatch
   window.__REDUX_DEVTOOLS_EXTENSION__
     ? window.__REDUX_DEVTOOLS_EXTENSION__({
@@ -23,6 +28,62 @@ export const store = createStore<StoreState, Actions, any, any>(
     : undefined,
 );
 
+export function rootReducer(
+  state: StoreState | undefined,
+  action: Actions,
+): StoreState {
+  if (action.type === 'STAR_PLUGIN' && state) {
+    const {plugin, selectedApp} = action.payload;
+    const selectedPlugin = plugin.id;
+    const clients = state.connections.clients.filter(
+      (client) => client.query.app === selectedApp,
+    );
+    return produce(state, (draft) => {
+      if (!draft.connections.userStarredPlugins[selectedApp]) {
+        draft.connections.userStarredPlugins[selectedApp] = [];
+      }
+      const plugins = draft.connections.userStarredPlugins[selectedApp];
+      const idx = plugins.indexOf(selectedPlugin);
+      if (idx === -1) {
+        plugins.push(selectedPlugin);
+        // enabling a plugin on one device enables it on all...
+        clients.forEach((client) => {
+          // sandy plugin? initialize it
+          client.startPluginIfNeeded(plugin, true);
+          // background plugin? connect it needed
+          if (
+            !defaultEnabledBackgroundPlugins.includes(selectedPlugin) &&
+            client?.isBackgroundPlugin(selectedPlugin)
+          ) {
+            client.initPlugin(selectedPlugin);
+          }
+        });
+      } else {
+        plugins.splice(idx, 1);
+        // enabling a plugin on one device disables it on all...
+        clients.forEach((client) => {
+          // disconnect background plugins
+          if (
+            !defaultEnabledBackgroundPlugins.includes(selectedPlugin) &&
+            client?.isBackgroundPlugin(selectedPlugin)
+          ) {
+            client.deinitPlugin(selectedPlugin);
+          }
+          // stop sandy plugins
+          // TODO: forget any persisted state as well T68683449
+          client.stopPluginIfNeeded(plugin.id);
+          delete draft.pluginMessageQueue[
+            getPluginKey(client.id, {serial: client.query.device_id}, plugin.id)
+          ];
+        });
+      }
+    });
+  }
+
+  // otherwise
+  return reducers(state, action);
+}
+
 if (!isProduction()) {
   // For debugging purposes only
   // @ts-ignore

desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx

@@ -24,7 +24,8 @@ import {
 } from '../reducers/connections';
 import BaseDevice from '../devices/BaseDevice';
 
-import reducers, {Store} from '../reducers/index';
+import {rootReducer} from '../store';
+import {Store} from '../reducers/index';
 import Client, {ClientQuery} from '../Client';
 
 import {buildClientId} from '../utils/clientUtils';
@@ -58,7 +59,7 @@ export async function createMockFlipperWithPlugin(
   pluginClazz: PluginDefinition,
   options?: MockOptions,
 ): Promise<MockFlipperResult> {
-  const store = createStore(reducers);
+  const store = createStore(rootReducer);
   const logger = getInstance();
   store.dispatch(registerPlugins([pluginClazz]));
 

desktop/app/src/utils/__tests__/messageQueue.node.tsx

@@ -190,9 +190,7 @@ test('queue - events are NOT processed immediately if plugin is NOT selected (bu
   selectDeviceLogs(store);
   sendMessage('inc', {delta: 4});
   client.flushMessageBuffer();
-  expect(store.getState().pluginMessageQueue).toEqual({
+  expect(store.getState().pluginMessageQueue).toEqual({});
-    [pluginKey]: [],
-  });
 
   // star again, plugin still not selected, message is queued
   starTestPlugin(store, client);
@@ -646,3 +644,57 @@ test('client - incoming messages are buffered and flushed together', async () =>
         }
       `);
 });
+
+test('queue - messages that have not yet flushed be lost when disabling the plugin', async () => {
+  const {
+    client,
+    store,
+    sendMessage,
+    pluginKey,
+  } = await createMockFlipperWithPlugin(TestPlugin);
+  selectDeviceLogs(store);
+
+  sendMessage('inc', {});
+  sendMessage('inc', {delta: 2});
+
+  expect(client.messageBuffer).toMatchInlineSnapshot(`
+    Object {
+      "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Object {
+        "messages": Array [
+          Object {
+            "api": "TestPlugin",
+            "method": "inc",
+            "params": Object {
+              "delta": 2,
+            },
+          },
+        ],
+        "plugin": [Function],
+      },
+    }
+  `);
+  expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(`
+    Object {
+      "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [
+        Object {
+          "api": "TestPlugin",
+          "method": "inc",
+          "params": Object {},
+        },
+      ],
+    }
+  `);
+
+  // disable
+  starTestPlugin(store, client);
+  expect(client.messageBuffer).toMatchInlineSnapshot(`Object {}`);
+  expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(
+    `Object {}`,
+  );
+
+  // re-enable, no messages arrive
+  starTestPlugin(store, client);
+  client.flushMessageBuffer();
+  processMessageQueue(TestPlugin, pluginKey, store);
+  expect(store.getState().pluginStates).toMatchInlineSnapshot(`Object {}`);
+});

desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx

@@ -197,9 +197,8 @@ test('queue - events are NOT processed immediately if plugin is NOT selected (bu
   selectDeviceLogs(store);
   sendMessage('inc', {delta: 4});
   client.flushMessageBuffer();
-  expect(store.getState().pluginMessageQueue).toEqual({
+  expect(client.messageBuffer).toMatchInlineSnapshot(`Object {}`);
-    [pluginKey]: [],
+  expect(store.getState().pluginMessageQueue).toEqual({});
-  });
 
   // star again, plugin still not selected, message is queued
   starTestPlugin(store, client);
@@ -637,3 +636,61 @@ test('client - incoming messages are buffered and flushed together', async () =>
         }
       `);
 });
+
+test('queue - messages that have not yet flushed be lost when disabling the plugin', async () => {
+  const {
+    client,
+    store,
+    sendMessage,
+    pluginKey,
+  } = await createMockFlipperWithPlugin(TestPlugin);
+  selectDeviceLogs(store);
+
+  sendMessage('inc', {});
+  sendMessage('inc', {delta: 2});
+
+  expect(client.messageBuffer).toMatchInlineSnapshot(`
+    Object {
+      "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Object {
+        "messages": Array [
+          Object {
+            "api": "TestPlugin",
+            "method": "inc",
+            "params": Object {
+              "delta": 2,
+            },
+          },
+        ],
+        "plugin": undefined,
+      },
+    }
+  `);
+  expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(`
+    Object {
+      "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [
+        Object {
+          "api": "TestPlugin",
+          "method": "inc",
+          "params": Object {},
+        },
+      ],
+    }
+  `);
+
+  // disable
+  starTestPlugin(store, client);
+  expect(client.messageBuffer).toMatchInlineSnapshot(`Object {}`);
+  expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(
+    `Object {}`,
+  );
+
+  // re-enable, no messages arrive
+  starTestPlugin(store, client);
+  client.flushMessageBuffer();
+  processMessageQueue(
+    client.sandyPluginStates.get(TestPlugin.id)!,
+    pluginKey,
+    store,
+  );
+  expect(getTestPluginState(client)).toEqual({count: 0});
+});