Moved owners list calculations into the store and added tests

This is being done to fix a drill-through bug, although the initial fix is perhaps not the most performant one. At least we have test coverage now and a temporary fix.

src/__tests__/__snapshots__/storeOwners-test.js.snap

@@ -0,0 +1,20 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Store owners list should drill through intermediate components: 1: mount 1`] = `
+[root]
+  ▾ <Root>
+    ▾ <Intermediate>
+      ▾ <Wrapper>
+          <Leaf key="children">
+`;
+
+exports[`Store owners list should drill through intermediate components: 2: components owned by <Root> 1`] = `
+"  ▾ <Root>
+    ▾ <Intermediate>
+        <Leaf key=\\"children\\">"
+`;
+
+exports[`Store owners list should drill through intermediate components: 3: components owned by <Intermediate> 1`] = `
+"  ▾ <Intermediate>
+    ▾ <Wrapper>"
+`;

src/__tests__/storeOwners-test.js

@@ -0,0 +1,50 @@
+// @flow
+
+const { printOwnersList } = require('./storeSerializer');
+
+describe('Store owners list', () => {
+  let React;
+  let ReactDOM;
+  let TestUtils;
+  let store;
+
+  const act = (callback: Function) => {
+    TestUtils.act(() => {
+      callback();
+    });
+    jest.runAllTimers(); // Flush Bridge operations
+  };
+
+  beforeEach(() => {
+    store = global.store;
+    store.collapseNodesByDefault = false;
+
+    React = require('react');
+    ReactDOM = require('react-dom');
+    TestUtils = require('react-dom/test-utils');
+  });
+
+  it('should drill through intermediate components', () => {
+    const Root = () => (
+      <Intermediate>
+        <Leaf key="children" />
+      </Intermediate>
+    );
+    const Wrapper = ({ children }) => children;
+    const Leaf = () => <div>Leaf</div>;
+    const Intermediate = ({ children }) => <Wrapper>{children}</Wrapper>;
+
+    act(() => ReactDOM.render(<Root />, document.createElement('div')));
+    expect(store).toMatchSnapshot('1: mount');
+
+    const rootID = store.getElementIDAtIndex(0);
+    expect(
+      printOwnersList(store.getOwnersListForElement(rootID))
+    ).toMatchSnapshot('2: components owned by <Root>');
+
+    const intermediateID = store.getElementIDAtIndex(1);
+    expect(
+      printOwnersList(store.getOwnersListForElement(intermediateID))
+    ).toMatchSnapshot('3: components owned by <Intermediate>');
+  });
+});

src/__tests__/storeSerializer.js

@@ -1,9 +1,74 @@
 import Store from 'src/devtools/store';
 
-export function test(value) {
-  return value instanceof Store;
+// test() is part of Jest's serializer API
+export function test(maybeStore) {
+  return maybeStore instanceof Store;
 }
 
-export function print(value, serialize, indent) {
-  return value.__toSnapshot();
+// print() is part of Jest's serializer API
+export function print(store, serialize, indent) {
+  return printStore(store);
+}
+
+export function printElement(element, includeWeight = false) {
+  let prefix = ' ';
+  if (element.children.length > 0) {
+    prefix = element.isCollapsed ? '▸' : '▾';
+  }
+
+  let key = '';
+  if (element.key !== null) {
+    key = ` key="${element.key}"`;
+  }
+
+  let suffix = '';
+  if (includeWeight) {
+    suffix = ` (${element.isCollapsed ? 1 : element.weight})`;
+  }
+
+  return `${'  '.repeat(element.depth + 1)}${prefix} <${element.displayName ||
+    'null'}${key}>${suffix}`;
+}
+
+export function printOwnersList(elements, includeWeight = false) {
+  return elements
+    .map(element => printElement(element, includeWeight))
+    .join('\n');
+}
+
+// Used for Jest snapshot testing.
+// May also be useful for visually debugging the tree, so it lives on the Store.
+export function printStore(store, includeWeight = false) {
+  const snapshotLines = [];
+
+  let rootWeight = 0;
+
+  store.roots.forEach(rootID => {
+    const { weight } = store.getElementByID(rootID);
+
+    snapshotLines.push('[root]' + (includeWeight ? ` (${weight})` : ''));
+
+    for (let i = rootWeight; i < rootWeight + weight; i++) {
+      const element = store.getElementAtIndex(i);
+
+      if (element == null) {
+        throw Error(`Could not find element at index ${i}`);
+      }
+
+      snapshotLines.push(printElement(element, includeWeight));
+    }
+
+    rootWeight += weight;
+  });
+
+  // Make sure the pretty-printed test align with the Store's reported number of total rows.
+  if (rootWeight !== store.numElements) {
+    throw Error(
+      `Inconsistent Store state. Individual root weights (${rootWeight}) do not match total weight (${
+        store.numElements
+      })`
+    );
+  }
+
+  return snapshotLines.join('\n');
 }

src/devtools/store.js

@@ -13,6 +13,7 @@ import { ElementTypeRoot } from './types';
 import { utfDecodeString } from '../utils';
 import { __DEBUG__ } from '../constants';
 import ProfilingCache from './ProfilingCache';
+import { printStore } from 'src/__tests__/storeSerializer';
 
 import type { ElementType } from './types';
 import type { Element } from './views/Components/types';
@@ -405,6 +406,14 @@ export default class Store extends EventEmitter {
     return index;
   }
 
+  getOwnersListForElement(id: number): Array<Element> {
+    const list = [];
+
+    this._populateOwnersList(id, id, 0, list);
+
+    return list;
+  }
+
   getRendererIDForElement(id: number): number | null {
     let current = this._idToElement.get(id);
     while (current != null) {
@@ -552,6 +561,32 @@ export default class Store extends EventEmitter {
     THROTTLE_CAPTURE_SCREENSHOT_DURATION
   );
 
+  _populateOwnersList(
+    id: number,
+    ownerID: number,
+    depth: number,
+    list: Array<Element>
+  ) {
+    const element = this._idToElement.get(id);
+    if (element != null) {
+      const isInList = id === ownerID || element.ownerID === ownerID;
+      if (isInList) {
+        list.push({
+          ...element,
+          depth: depth,
+        });
+      }
+      element.children.forEach(childID =>
+        this._populateOwnersList(
+          childID,
+          ownerID,
+          isInList ? depth + 1 : depth,
+          list
+        )
+      );
+    }
+  }
+
   _takeProfilingSnapshotRecursive = (id: number) => {
     const element = this.getElementByID(id);
     if (element !== null) {
@@ -870,7 +905,7 @@ export default class Store extends EventEmitter {
     }
 
     if (__DEBUG__) {
-      console.log(this.__toSnapshot(true));
+      console.log(printStore(this, true));
       console.groupEnd();
     }
 
@@ -917,59 +952,4 @@ export default class Store extends EventEmitter {
     this._bridge.removeListener('profilingStatus', this.onProfilingStatus);
     this._bridge.removeListener('shutdown', this.onBridgeShutdown);
   };
-
-  // Used for Jest snapshot testing.
-  // May also be useful for visually debugging the tree, so it lives on the Store.
-  __toSnapshot = (includeWeight: boolean = false) => {
-    const snapshotLines = [];
-
-    let rootWeight = 0;
-
-    this._roots.forEach(rootID => {
-      const { weight } = ((this.getElementByID(rootID): any): Element);
-
-      snapshotLines.push('[root]' + (includeWeight ? ` (${weight})` : ''));
-
-      for (let i = rootWeight; i < rootWeight + weight; i++) {
-        const element = ((this.getElementAtIndex(i): any): Element);
-
-        if (element == null) {
-          throw Error(`Could not find element at index ${i}`);
-        }
-
-        let prefix = ' ';
-        if (element.children.length > 0) {
-          prefix = element.isCollapsed ? '▸' : '▾';
-        }
-
-        let key = '';
-        if (element.key !== null) {
-          key = ` key="${element.key}"`;
-        }
-
-        let suffix = '';
-        if (includeWeight) {
-          suffix = ` (${element.isCollapsed ? 1 : element.weight})`;
-        }
-
-        snapshotLines.push(
-          `${'  '.repeat(element.depth + 1)}${prefix} <${element.displayName ||
-            'null'}${key}>${suffix}`
-        );
-      }
-
-      rootWeight += weight;
-    });
-
-    // Make sure the pretty-printed test align with the Store's reported number of total rows.
-    if (rootWeight !== this._weightAcrossRoots) {
-      throw Error(
-        `Inconsistent Store state. Individual root weights (${rootWeight}) do not match total weight (${
-          this._weightAcrossRoots
-        })`
-      );
-    }
-
-    return snapshotLines.join('\n');
-  };
 }

src/devtools/views/Components/Element.js

@@ -29,17 +29,14 @@ type Props = {
 
 export default function ElementView({ data, index, style }: Props) {
   const store = useContext(StoreContext);
-  const {
-    baseDepth,
-    ownerFlatTree,
-    ownerStack,
-    selectedElementID,
-  } = useContext(TreeStateContext);
+  const { ownerFlatTree, ownerStack, selectedElementID } = useContext(
+    TreeStateContext
+  );
   const dispatch = useContext(TreeDispatcherContext);
 
   const element =
     ownerFlatTree !== null
-      ? store.getElementByID(ownerFlatTree[index])
+      ? ownerFlatTree[index]
       : store.getElementAtIndex(index);
 
   const [isHovered, setIsHovered] = useState(false);
@@ -158,7 +155,7 @@ export default function ElementView({ data, index, style }: Props) {
         ...style, // "style" comes from react-window
 
         // Left padding presents the appearance of a nested tree structure.
-        paddingLeft: `${(depth - baseDepth) * 0.75 + 0.25}rem`,
+        paddingLeft: `${depth * 0.75 + 0.25}rem`,
 
         // These style overrides enable the background color to fill the full visible width,
         // when combined with the CSS tweaks in Tree.

src/devtools/views/Components/Tree.js

@@ -22,7 +22,6 @@ import SearchInput from './SearchInput';
 import styles from './Tree.css';
 
 export type ItemData = {|
-  baseDepth: number,
   numElements: number,
   isNavigatingWithKeyboard: boolean,
   lastScrolledIDRef: { current: number | null },
@@ -35,7 +34,6 @@ type Props = {||};
 export default function Tree(props: Props) {
   const dispatch = useContext(TreeDispatcherContext);
   const {
-    baseDepth,
     numElements,
     ownerStack,
     searchIndex,
@@ -255,7 +253,6 @@ export default function Tree(props: Props) {
   // This includes the owner context, since it controls a filtered view of the tree.
   const itemData = useMemo<ItemData>(
     () => ({
-      baseDepth,
       numElements,
       isNavigatingWithKeyboard,
       onElementMouseEnter: handleElementMouseEnter,
@@ -263,7 +260,6 @@ export default function Tree(props: Props) {
       treeFocused,
     }),
     [
-      baseDepth,
       numElements,
       isNavigatingWithKeyboard,
       handleElementMouseEnter,

src/devtools/views/Components/TreeContext.js

@@ -40,7 +40,6 @@ import type { Element } from './types';
 
 type StateContext = {|
   // Tree
-  baseDepth: number,
   numElements: number,
   selectedElementID: number | null,
   selectedElementIndex: number | null,
@@ -51,7 +50,7 @@ type StateContext = {|
   searchText: string,
 
   // Owners
-  ownerFlatTree: Array<number> | null,
+  ownerFlatTree: Array<Element> | null,
   ownerStack: Array<number>,
   ownerStackIndex: number | null,
 
@@ -133,7 +132,6 @@ TreeDispatcherContext.displayName = 'TreeDispatcherContext';
 
 type State = {|
   // Tree
-  baseDepth: number,
   numElements: number,
   selectedElementID: number | null,
   selectedElementIndex: number | null,
@@ -146,7 +144,7 @@ type State = {|
   // Owners
   ownerStack: Array<number>,
   ownerStackIndex: number | null,
-  ownerFlatTree: Array<number> | null,
+  ownerFlatTree: Array<Element> | null,
 
   // Inspection element panel
   inspectedElementID: number | null,
@@ -441,7 +439,6 @@ function reduceSearchState(store: Store, state: State, action: Action): State {
 
 function reduceOwnersState(store: Store, state: State, action: Action): State {
   let {
-    baseDepth,
     numElements,
     selectedElementID,
     selectedElementIndex,
@@ -477,7 +474,9 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
         }
         if (selectedElementID !== null && ownerFlatTree !== null) {
           // Mutation might have caused the index of this ID to shift.
-          selectedElementIndex = ownerFlatTree.indexOf(selectedElementID);
+          selectedElementIndex = ownerFlatTree.findIndex(
+            element => element.id === selectedElementID
+          );
         }
       } else {
         if (selectedElementID !== null) {
@@ -509,7 +508,9 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
       if (ownerFlatTree !== null) {
         const payload = (action: ACTION_SELECT_ELEMENT_BY_ID).payload;
         selectedElementIndex =
-          payload === null ? null : ownerFlatTree.indexOf(payload);
+          payload === null
+            ? null
+            : ownerFlatTree.findIndex(element => element.id === payload);
       }
       break;
     case 'SELECT_NEXT_ELEMENT_IN_TREE':
@@ -574,18 +575,11 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
   ) {
     if (ownerStackIndex === null) {
       ownerFlatTree = null;
-      baseDepth = 0;
       numElements = store.numElements;
     } else {
-      ownerFlatTree = calculateCurrentOwnerList(
-        store,
-        ownerStack[ownerStackIndex],
-        ownerStack[ownerStackIndex],
-        []
+      ownerFlatTree = store.getOwnersListForElement(
+        ownerStack[ownerStackIndex]
       );
-
-      baseDepth = ((store.getElementByID(ownerFlatTree[0]): any): Element)
-        .depth;
       numElements = ownerFlatTree.length;
     }
   }
@@ -595,14 +589,14 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
     if (selectedElementIndex === null) {
       selectedElementID = null;
     } else if (ownerFlatTree !== null) {
-      selectedElementID = ownerFlatTree[((selectedElementIndex: any): number)];
+      selectedElementID =
+        ownerFlatTree[((selectedElementIndex: any): number)].id;
     }
   }
 
   return {
     ...state,
 
-    baseDepth,
     numElements,
     selectedElementID,
     selectedElementIndex,
@@ -692,7 +686,6 @@ function TreeContextController({ children }: Props) {
 
   const [state, dispatch] = useReducer(reducer, {
     // Tree
-    baseDepth: 0,
     numElements: store.numElements,
     selectedElementIndex: null,
     selectedElementID: null,
@@ -783,33 +776,6 @@ function TreeContextController({ children }: Props) {
   );
 }
 
-function calculateCurrentOwnerList(
-  store: Store,
-  rootOwnerID: number,
-  elementID: number,
-  ownerList: Array<number>
-): Array<number> {
-  if (elementID === rootOwnerID) {
-    ownerList.push(elementID);
-    const { children } = ((store.getElementByID(elementID): any): Element);
-    children.forEach(childID =>
-      calculateCurrentOwnerList(store, rootOwnerID, childID, ownerList)
-    );
-  } else {
-    const { children, ownerID } = ((store.getElementByID(
-      elementID
-    ): any): Element);
-    if (ownerID === rootOwnerID) {
-      ownerList.push(elementID);
-      children.forEach(childID =>
-        calculateCurrentOwnerList(store, rootOwnerID, childID, ownerList)
-      );
-    }
-  }
-
-  return ownerList;
-}
-
 function recursivelySearchTree(
   store: Store,
   elementID: number,