Fixed some issues with too many devices showing up in the sidebar

Summary:
Device management was inconsistent so far, this diff addresses the following issues

* pending a subtle timing issue, a physical android device might also show up as emulator, so effectively the device would be shown twice, but with the same content
* Metro devices now behave more like the android devices: offline devices are replaced if it comes online again
* Generalized this logic; the reducer now forces serials to be unique
* Fixed issue where a Metro device that disconnected due to a connection failure would be archived twice
* Use the metro connection url as serial, to have a slightly more future proof serial

Reviewed By: jknoxville

Differential Revision: D19996385

fbshipit-source-id: 0f6e3ddc6444542553d25cc3b592591652d688f2

src/devices/MetroDevice.tsx

@@ -201,7 +201,7 @@ export default class MetroDevice extends BaseDevice {
 
   archive() {
     return new ArchivedDevice(
-      this.serial + v4(),
+      this.serial,
       this.deviceType,
       this.title,
       this.os,

src/dispatcher/metroDevice.tsx

@@ -20,7 +20,6 @@ const METRO_URL = `http://${METRO_HOST}:${METRO_PORT}`;
 const METRO_LOGS_ENDPOINT = `ws://${METRO_HOST}:${METRO_PORT}/events`;
 const METRO_MESSAGE = ['React Native packager is running', 'Metro is running'];
 const QUERY_INTERVAL = 5000;
-const METRO_DEVICE_ID = 'metro'; // there is always only one activve
 
 async function isMetroRunning(): Promise<boolean> {
   return new Promise(resolve => {
@@ -50,7 +49,7 @@ async function registerDevice(
   store: Store,
   logger: Logger,
 ) {
-  const metroDevice = new MetroDevice(METRO_DEVICE_ID, ws);
+  const metroDevice = new MetroDevice(METRO_URL, ws);
   logger.track('usage', 'register-device', {
     os: 'Metro',
     name: metroDevice.title,
@@ -60,7 +59,7 @@ async function registerDevice(
   store.dispatch({
     type: 'REGISTER_DEVICE',
     payload: metroDevice,
-    serial: METRO_DEVICE_ID,
+    serial: METRO_URL,
   });
 
   registerDeviceCallbackOnPlugins(
@@ -74,20 +73,20 @@ async function registerDevice(
 async function unregisterDevices(store: Store, logger: Logger) {
   logger.track('usage', 'unregister-device', {
     os: 'Metro',
-    serial: METRO_DEVICE_ID,
+    serial: METRO_URL,
   });
 
   let archivedDevice: ArchivedDevice | undefined = undefined;
   const device = store
     .getState()
-    .connections.devices.find(device => device.serial === METRO_DEVICE_ID);
+    .connections.devices.find(device => device.serial === METRO_URL);
   if (device && !device.isArchived) {
     archivedDevice = device.archive();
   }
 
   store.dispatch({
     type: 'UNREGISTER_DEVICES',
-    payload: new Set([METRO_DEVICE_ID]),
+    payload: new Set([METRO_URL]),
   });
 
   if (archivedDevice) {
@@ -102,6 +101,7 @@ async function unregisterDevices(store: Store, logger: Logger) {
 export default (store: Store, logger: Logger) => {
   let timeoutHandle: NodeJS.Timeout;
   let ws: WebSocket | undefined;
+  let unregistered = false;
 
   async function tryConnectToMetro() {
     if (ws) {
@@ -118,10 +118,13 @@ export default (store: Store, logger: Logger) => {
       };
 
       _ws.onclose = _ws.onerror = () => {
-        clearTimeout(guard);
+        if (!unregistered) {
-        ws = undefined;
+          unregistered = true;
-        unregisterDevices(store, logger);
+          clearTimeout(guard);
-        scheduleNext();
+          ws = undefined;
+          unregisterDevices(store, logger);
+          scheduleNext();
+        }
       };
 
       const guard = setTimeout(() => {

src/reducers/__tests__/connections.node.tsx

@@ -12,6 +12,7 @@ import {State, selectPlugin} from '../connections';
 import BaseDevice from '../../devices/BaseDevice';
 import MacDevice from '../../devices/MacDevice';
 import {FlipperDevicePlugin} from '../../plugin';
+import MetroDevice from '../../devices/MetroDevice';
 
 test('REGISTER_DEVICE doesnt remove error', () => {
   const initialState: State = reducer(undefined, {
@@ -34,6 +35,55 @@ test('REGISTER_DEVICE doesnt remove error', () => {
   ]);
 });
 
+test('doing a double REGISTER_DEVICE keeps the last', () => {
+  const device1 = new BaseDevice('serial', 'physical', 'title', 'Android');
+  const device2 = new BaseDevice('serial', 'physical', 'title2', 'Android');
+  const initialState: State = reducer(undefined, {
+    type: 'REGISTER_DEVICE',
+    payload: device1,
+  });
+  expect(initialState.devices.length).toBe(1);
+  expect(initialState.devices[0]).toBe(device1);
+
+  const endState = reducer(initialState, {
+    type: 'REGISTER_DEVICE',
+    payload: device2,
+  });
+  expect(endState.devices.length).toBe(1);
+  expect(endState.devices[0]).toBe(device2);
+});
+
+test('register, remove, re-register a metro device works correctly', () => {
+  const device1 = new MetroDevice('http://localhost:8081', undefined);
+  let state: State = reducer(undefined, {
+    type: 'REGISTER_DEVICE',
+    payload: device1,
+  });
+  expect(state.devices.length).toBe(1);
+  expect(state.devices[0].displayTitle()).toBe('React Native');
+
+  const archived = device1.archive();
+  state = reducer(state, {
+    type: 'UNREGISTER_DEVICES',
+    payload: new Set([device1.serial]),
+  });
+  expect(state.devices.length).toBe(0);
+
+  state = reducer(state, {
+    type: 'REGISTER_DEVICE',
+    payload: archived,
+  });
+  expect(state.devices.length).toBe(1);
+  expect(state.devices[0].displayTitle()).toBe('React Native (Offline)');
+
+  state = reducer(state, {
+    type: 'REGISTER_DEVICE',
+    payload: new MetroDevice('http://localhost:8081', undefined),
+  });
+  expect(state.devices.length).toBe(1);
+  expect(state.devices[0].displayTitle()).toBe('React Native');
+});
+
 test('triggering REGISTER_DEVICE before REGISTER_PLUGINS still registers device plugins', () => {
   class TestDevicePlugin extends FlipperDevicePlugin<any, any, any> {
     static id = 'test';

src/reducers/connections.tsx

@@ -204,9 +204,22 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => {
     case 'REGISTER_DEVICE': {
       const {payload} = action;
 
+      const newDevices = state.devices.slice();
+      const existing = state.devices.findIndex(
+        device => device.serial === payload.serial,
+      );
+      if (existing !== -1) {
+        console.debug(
+          `Got a new device instance for already existing serial ${payload.serial}`,
+        );
+        newDevices[existing] = payload;
+      } else {
+        newDevices.push(payload);
+      }
+
       return updateSelection({
         ...state,
-        devices: state.devices.concat(payload),
+        devices: newDevices,
       });
     }