Preserve device state after disconnect

Summary:
This diff stack introduces support for keeping devices and clients around after they have disconnected. This is a pretty important debugging improvement, that will allow inspecting a device / app after it crashed for example.

This feature existed partially before, but only supported Android, and only support plugins with persisted state; as it replace the current device with an archived version of the same device. In practice this didn't work really well, as most plugins would not be available, and all non-persisted state would be lost.

This diff makes sure we can keep devices around after disconnecting, the next one will keep the clients around as well. And explain some code choices in more detail.

Note that `Device.isArchived` was an overloaded term before, and even more now (both representing imported and disconnected devices), will address that in a later diff.

https://github.com/facebook/flipper/issues/1460
https://github.com/facebook/flipper/issues/812
https://github.com/facebook/flipper/issues/1487

Changelog: iOS and Android devices will preserve their state after being disconnected

Reviewed By: nikoant

Differential Revision: D26224310

fbshipit-source-id: 7dfc93c2a109a51c2880ec212a00463bc8d32041

desktop/app/src/__tests__/disconnect.node.tsx

@@ -0,0 +1,108 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @format
+ */
+
+import {default as BaseDevice} from '../devices/BaseDevice';
+import {createMockFlipperWithPlugin} from '../test-utils/createMockFlipperWithPlugin';
+import {
+  TestUtils,
+  _SandyPluginDefinition,
+  createState,
+  DevicePluginClient,
+} from 'flipper-plugin';
+
+test('Devices can disconnect', async () => {
+  const deviceplugin = new _SandyPluginDefinition(
+    TestUtils.createMockPluginDetails(),
+    {
+      devicePlugin(client: DevicePluginClient) {
+        const destroy = jest.fn();
+        client.onDestroy(destroy);
+        const counter = createState(0);
+        return {
+          counter,
+          destroy,
+        };
+      },
+      supportsDevice() {
+        return true;
+      },
+      Component() {
+        return null;
+      },
+    },
+  );
+  const {device} = await createMockFlipperWithPlugin(deviceplugin);
+
+  device.sandyPluginStates.get(deviceplugin.id)!.instanceApi.counter.set(1);
+
+  expect(device.isArchived).toBe(false);
+
+  device.markDisconnected();
+
+  expect(device.isArchived).toBe(true);
+  const instance = device.sandyPluginStates.get(deviceplugin.id)!;
+  expect(instance).toBeTruthy();
+  expect(instance.instanceApi.counter.get(1)).toBe(1); // state preserved
+  expect(instance.instanceApi.destroy).toBeCalledTimes(0);
+
+  device.destroy();
+  expect(device.isArchived).toBe(true);
+  expect(instance.instanceApi.destroy).toBeCalledTimes(1);
+
+  expect(device.sandyPluginStates.get(deviceplugin.id)).toBeUndefined();
+});
+
+test('New device with same serial removes & cleans the old one', async () => {
+  const deviceplugin = new _SandyPluginDefinition(
+    TestUtils.createMockPluginDetails(),
+    {
+      devicePlugin(client: DevicePluginClient) {
+        const destroy = jest.fn();
+        client.onDestroy(destroy);
+        return {
+          destroy,
+        };
+      },
+      supportsDevice() {
+        return true;
+      },
+      Component() {
+        return null;
+      },
+    },
+  );
+  const {device, store} = await createMockFlipperWithPlugin(deviceplugin);
+
+  const instance = device.sandyPluginStates.get(deviceplugin.id)!;
+
+  expect(device.isArchived).toBe(false);
+  expect(instance.instanceApi.destroy).toBeCalledTimes(0);
+  expect(store.getState().connections.devices).toEqual([device]);
+
+  // submit a new device with same serial
+  const device2 = new BaseDevice(
+    device.serial,
+    'physical',
+    'MockAndroidDevice',
+    'Android',
+  );
+  store.dispatch({
+    type: 'REGISTER_DEVICE',
+    payload: device2,
+  });
+  device2.loadDevicePlugins(store.getState().plugins.devicePlugins);
+
+  expect(device.isArchived).toBe(true);
+  expect(instance.instanceApi.destroy).toBeCalledTimes(1);
+  expect(
+    device2.sandyPluginStates.get(deviceplugin.id)!.instanceApi.destroy,
+  ).toBeCalledTimes(0);
+  expect(store.getState().connections.devices.length).toBe(1);
+  expect(store.getState().connections.devices[0]).toBe(device2);
+});

desktop/app/src/devices/AndroidDevice.tsx

@@ -89,16 +89,6 @@ export default class AndroidDevice extends BaseDevice {
     return this.executeShell(['logcat', '-c']);
   }
 
-  archive(): ArchivedDevice {
-    return new ArchivedDevice({
-      serial: this.serial,
-      deviceType: this.deviceType,
-      title: this.title,
-      os: this.os,
-      screenshotHandle: null,
-    });
-  }
-
   navigateToLocation(location: string) {
     const shellCommand = `am start ${encodeURI(location)}`;
     this.adb.shell(this.serial, shellCommand);

desktop/app/src/devices/ArchivedDevice.tsx

@@ -23,13 +23,13 @@ export default class ArchivedDevice extends BaseDevice {
     supportRequestDetails?: SupportFormRequestDetailsState;
   }) {
     super(options.serial, options.deviceType, options.title, options.os);
+    this.archivedState.set(true);
     this.source = options.source || '';
     this.supportRequestDetails = options.supportRequestDetails;
     this.archivedScreenshotHandle = options.screenshotHandle;
   }
 
   archivedScreenshotHandle: string | null;
-  isArchived = true;
 
   displayTitle(): string {
     return `${this.title} ${this.source ? '(Imported)' : '(Offline)'}`;

desktop/app/src/devices/BaseDevice.tsx

@@ -15,6 +15,7 @@ import {
   DeviceType,
   DeviceLogListener,
   Idler,
+  createState,
 } from 'flipper-plugin';
 import type {DevicePluginDefinition, DevicePluginMap} from '../plugin';
 import {getFlipperLibImplementation} from '../utils/flipperLibImplementation';
@@ -70,7 +71,12 @@ export default class BaseDevice {
   icon: string | null | undefined;
 
   logListeners: Map<Symbol, DeviceLogListener> = new Map();
-  isArchived: boolean = false;
+
+  archivedState = createState(false);
+  get isArchived() {
+    return this.archivedState.get();
+  }
+
   // if imported, stores the original source location
   source = '';
 
@@ -87,7 +93,7 @@ export default class BaseDevice {
   }
 
   displayTitle(): string {
-    return this.title;
+    return this.isArchived ? `${this.title} (Offline)` : this.title;
   }
 
   async exportState(
@@ -154,10 +160,6 @@ export default class BaseDevice {
     throw new Error('unimplemented');
   }
 
-  archive(): any | null | undefined {
-    return null;
-  }
-
   screenshot(): Promise<Buffer> {
     return Promise.reject(
       new Error('No screenshot support for current device'),
@@ -218,4 +220,16 @@ export default class BaseDevice {
     }
     this.devicePlugins.splice(this.devicePlugins.indexOf(pluginId), 1);
   }
+
+  markDisconnected() {
+    this.archivedState.set(true);
+  }
+
+  destroy() {
+    this.markDisconnected();
+    this.sandyPluginStates.forEach((instance) => {
+      instance.destroy();
+    });
+    this.sandyPluginStates.clear();
+  }
 }

desktop/app/src/dispatcher/androidDevice.tsx

@@ -262,28 +262,11 @@ export default (store: Store, logger: Logger) => {
       }),
     );
 
-    const archivedDevices = deviceIds
-      .map((id) => {
-        const device = store
-          .getState()
-          .connections.devices.find((device) => device.serial === id);
-        if (device && !device.isArchived) {
-          return device.archive();
-        }
-      })
-      .filter(Boolean);
-
-    store.dispatch({
-      type: 'UNREGISTER_DEVICES',
-      payload: new Set(deviceIds),
-    });
-
-    archivedDevices.forEach((device: BaseDevice) => {
-      device.loadDevicePlugins(store.getState().plugins.devicePlugins);
-      store.dispatch({
-        type: 'REGISTER_DEVICE',
-        payload: device,
-      });
+    deviceIds.forEach((id) => {
+      const device = store
+        .getState()
+        .connections.devices.find((device) => device.serial === id);
+      device?.markDisconnected();
     });
   }
 

desktop/app/src/dispatcher/iOSDevice.tsx

@@ -148,15 +148,12 @@ function processDevices(
     }
   }
 
-  if (currentDeviceIDs.size > 0) {
-    currentDeviceIDs.forEach((id) =>
-      logger.track('usage', 'unregister-device', {os: 'iOS', serial: id}),
-    );
-    store.dispatch({
-      type: 'UNREGISTER_DEVICES',
-      payload: currentDeviceIDs,
-    });
-  }
+  currentDeviceIDs.forEach((id) => {
+    const device = store
+      .getState()
+      .connections.devices.find((device) => device.serial === id);
+    device?.markDisconnected();
+  });
 }
 
 function getDeviceSetPath() {

desktop/app/src/dispatcher/metroDevice.tsx

@@ -79,26 +79,10 @@ async function unregisterDevices(store: Store, logger: Logger) {
     serial: METRO_URL,
   });
 
-  let archivedDevice: ArchivedDevice | undefined = undefined;
-  const device = store
-    .getState()
-    .connections.devices.find((device) => device.serial === METRO_URL);
-  if (device && !device.isArchived) {
-    archivedDevice = device.archive();
-  }
-
   store.dispatch({
     type: 'UNREGISTER_DEVICES',
     payload: new Set([METRO_URL]),
   });
-
-  if (archivedDevice) {
-    archivedDevice.loadDevicePlugins(store.getState().plugins.devicePlugins);
-    store.dispatch({
-      type: 'REGISTER_DEVICE',
-      payload: archivedDevice,
-    });
-  }
 }
 
 export default (store: Store, logger: Logger) => {

desktop/app/src/reducers/connections.tsx

@@ -197,6 +197,7 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
         console.debug(
           `Got a new device instance for already existing serial ${payload.serial}`,
         );
+        state.devices[existing].destroy();
         newDevices[existing] = payload;
       } else {
         newDevices.push(payload);

desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx

@@ -11,7 +11,7 @@ import React from 'react';
 import {Alert} from 'antd';
 import {LeftSidebar, SidebarTitle, InfoIcon} from '../LeftSidebar';
 import {Layout, Link, styled} from '../../ui';
-import {theme} from 'flipper-plugin';
+import {theme, useValue} from 'flipper-plugin';
 import {AppSelector} from './AppSelector';
 import {useStore} from '../../utils/useStore';
 import {PluginList} from './PluginList';
@@ -52,7 +52,7 @@ export function AppInspect() {
     metroDevice,
     connections.userPreferredDevice,
   ]);
-  const isArchived = !!activeDevice?.isArchived;
+  const isArchived = useValue(activeDevice?.archivedState, false);
 
   return (
     <LeftSidebar>

desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx

@@ -17,7 +17,7 @@ import {
   RocketOutlined,
 } from '@ant-design/icons';
 import {Glyph, Layout, styled} from '../../ui';
-import {theme, useTrackedCallback} from 'flipper-plugin';
+import {theme, useTrackedCallback, useValue} from 'flipper-plugin';
 import {batch} from 'react-redux';
 import {useDispatch, useStore} from '../../utils/useStore';
 import {
@@ -56,6 +56,7 @@ export function AppSelector() {
     uninitializedClients,
     selectedApp,
   } = useStore((state) => state.connections);
+  useValue(selectedDevice?.archivedState, false); // subscribe to future archived state changes
 
   const onSelectDevice = useTrackedCallback(
     'select-device',

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

@@ -21,7 +21,7 @@ import {FlipperPlugin, FlipperDevicePlugin} from '../../plugin';
 import {Notification} from '../../plugin';
 import {default as Client, ClientExport} from '../../Client';
 import {State as PluginsState} from '../../reducers/plugins';
-import {renderMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin';
+import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin';
 import {
   TestUtils,
   _SandyPluginDefinition,
@@ -1116,7 +1116,7 @@ const sandyTestPlugin = new _SandyPluginDefinition(
 );
 
 test('Sandy plugins are exported properly', async () => {
-  const {client, sendMessage, store} = await renderMockFlipperWithPlugin(
+  const {client, sendMessage, store} = await createMockFlipperWithPlugin(
     sandyTestPlugin,
   );
 
@@ -1145,7 +1145,7 @@ test('Sandy plugins are exported properly', async () => {
 });
 
 test('Sandy plugins with custom export are exported properly', async () => {
-  const {client, sendMessage, store} = await renderMockFlipperWithPlugin(
+  const {client, sendMessage, store} = await createMockFlipperWithPlugin(
     sandyTestPlugin,
   );
 
@@ -1218,7 +1218,7 @@ test('Sandy plugins are imported properly', async () => {
     },
   };
 
-  const {client, store} = await renderMockFlipperWithPlugin(sandyTestPlugin);
+  const {client, store} = await createMockFlipperWithPlugin(sandyTestPlugin);
 
   await importDataToStore('unittest.json', JSON.stringify(data), store);
 
@@ -1312,7 +1312,7 @@ test('Sandy device plugins are exported / imported properly', async () => {
     },
   };
 
-  const {device, store} = await renderMockFlipperWithPlugin(
+  const {device, store} = await createMockFlipperWithPlugin(
     sandyDeviceTestPlugin,
   );
 
@@ -1350,7 +1350,7 @@ test('Sandy device plugins are exported / imported properly', async () => {
 });
 
 test('Sandy device plugins with custom export are export properly', async () => {
-  const {device, store} = await renderMockFlipperWithPlugin(
+  const {device, store} = await createMockFlipperWithPlugin(
     sandyDeviceTestPlugin,
   );
 
@@ -1384,7 +1384,7 @@ test('Sandy plugin with custom import', async () => {
     },
   );
 
-  const {store} = await renderMockFlipperWithPlugin(plugin);
+  const {store} = await createMockFlipperWithPlugin(plugin);
 
   const data = {
     clients: [
@@ -1484,7 +1484,7 @@ test('Sandy device plugin with custom import', async () => {
     },
   };
 
-  const {store} = await renderMockFlipperWithPlugin(plugin);
+  const {store} = await createMockFlipperWithPlugin(plugin);
 
   await importDataToStore('unittest.json', JSON.stringify(data), store);
 
@@ -1503,7 +1503,7 @@ test('Sandy device plugin with custom import', async () => {
 });
 
 test('Sandy plugins with complex data are imported  / exported correctly', async () => {
-  const deviceplugin = new _SandyPluginDefinition(
+  const plugin = new _SandyPluginDefinition(
     TestUtils.createMockPluginDetails(),
     {
       plugin() {
@@ -1522,7 +1522,7 @@ test('Sandy plugins with complex data are imported  / exported correctly', async
     },
   );
 
-  const {store} = await renderMockFlipperWithPlugin(deviceplugin);
+  const {store} = await createMockFlipperWithPlugin(plugin);
 
   const data = await exportStore(store);
   expect(Object.values(data.exportStoreData.pluginStates2)).toMatchObject([
@@ -1551,7 +1551,7 @@ test('Sandy plugins with complex data are imported  / exported correctly', async
   await importDataToStore('unittest.json', data.serializedString, store);
   const api = store
     .getState()
-    .connections.clients[1].sandyPluginStates.get(deviceplugin.id)?.instanceApi;
+    .connections.clients[1].sandyPluginStates.get(plugin.id)?.instanceApi;
   expect(api.m.get()).toMatchInlineSnapshot(`
     Map {
       "a" => 1,
@@ -1590,7 +1590,7 @@ test('Sandy device plugins with complex data are imported  / exported correctly'
     },
   );
 
-  const {store} = await renderMockFlipperWithPlugin(deviceplugin);
+  const {store} = await createMockFlipperWithPlugin(deviceplugin);
 
   const data = await exportStore(store);
   expect(data.exportStoreData.device?.pluginStates).toMatchObject({

desktop/flipper-plugin/src/plugin/PluginBase.tsx

@@ -132,7 +132,9 @@ export abstract class BasePluginInstance {
       realDevice, // TODO: temporarily, clean up T70688226
       // N.B. we model OS as string, not as enum, to make custom device types possible in the future
       os: realDevice.os,
-      isArchived: realDevice.isArchived,
+      get isArchived() {
+        return realDevice.isArchived;
+      },
       deviceType: realDevice.deviceType,
 
       onLogEntry(cb) {

desktop/flipper-plugin/src/state/atom.tsx

@@ -84,9 +84,16 @@ export function createState<T>(
   return atom;
 }
 
-export function useValue<T>(atom: Atom<T>): T {
-  const [localValue, setLocalValue] = useState<T>(atom.get());
+export function useValue<T>(atom: Atom<T>): T;
+export function useValue<T>(atom: Atom<T> | undefined, defaultValue: T): T;
+export function useValue<T>(atom: Atom<T> | undefined, defaultValue?: T): T {
+  const [localValue, setLocalValue] = useState<T>(
+    atom ? atom.get() : defaultValue!,
+  );
   useEffect(() => {
+    if (!atom) {
+      return;
+    }
     // atom might have changed between mounting and effect setup
     // in that case, this will cause a re-render, otherwise not
     setLocalValue(atom.get());