From 0a6d637619fe357ee44f43827487d2dbe6051486 Mon Sep 17 00:00:00 2001
From: Dan <dan.abramov@gmail.com>
Date: Sun, 7 Apr 2019 21:09:51 +0100
Subject: [PATCH] Fix spurious autoscroll

---
 src/devtools/views/Components/Element.js | 26 +++++++++++++++++++-----
 src/devtools/views/Components/Tree.js    | 14 +++++++++++--
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/devtools/views/Components/Element.js b/src/devtools/views/Components/Element.js
index eef967d931..14acf8ae2f 100644
--- a/src/devtools/views/Components/Element.js
+++ b/src/devtools/views/Components/Element.js
@@ -4,7 +4,7 @@ import React, {
   Fragment,
   useCallback,
   useContext,
-  useEffect,
+  useLayoutEffect,
   useMemo,
   useRef,
 } from 'react';
@@ -19,9 +19,11 @@ import styles from './Element.css';
 type Props = {
   index: number,
   style: Object,
+  // TODO: I can't get the correct type to work here:
+  data: Object,
 };
 
-export default function ElementView({ index, style }: Props) {
+export default function ElementView({ index, style, data }: Props) {
   const {
     baseDepth,
     getElementAtIndex,
@@ -29,11 +31,11 @@ export default function ElementView({ index, style }: Props) {
     selectedElementID,
     selectElementByID,
   } = useContext(TreeContext);
-
   const element = getElementAtIndex(index);
 
   const id = element === null ? null : element.id;
   const isSelected = selectedElementID === id;
+  const lastScrolledIDRef = data.lastScrolledIDRef;
 
   const handleDoubleClick = useCallback(() => {
     if (id !== null) {
@@ -43,8 +45,22 @@ export default function ElementView({ index, style }: Props) {
 
   const ref = useRef<HTMLSpanElement | null>(null);
 
-  useEffect(() => {
+  // The tree above has its own autoscrolling, but it only works for rows.
+  // However, even when the row gets into the viewport, the component name
+  // might be too far left or right on the screen. Adjust it in this case.
+  useLayoutEffect(() => {
     if (isSelected) {
+      // Don't select the same item twice.
+      // A row may appear and disappear just by scrolling:
+      // https://github.com/bvaughn/react-devtools-experimental/issues/67
+      // It doesn't necessarily indicate a user action.
+      // TODO: we might want to revamp the autoscroll logic
+      // to only happen explicitly for user-initiated events.
+      if (lastScrolledIDRef.current === id) {
+        return;
+      }
+      lastScrolledIDRef.current = id;
+
       if (ref.current !== null) {
         ref.current.scrollIntoView({
           behavior: 'auto',
@@ -53,7 +69,7 @@ export default function ElementView({ index, style }: Props) {
         });
       }
     }
-  }, [isSelected]);
+  }, [id, isSelected, lastScrolledIDRef]);
 
   // TODO Add click and key handlers for toggling element open/close state.
 
diff --git a/src/devtools/views/Components/Tree.js b/src/devtools/views/Components/Tree.js
index 8019a61768..4b87eb77d1 100644
--- a/src/devtools/views/Components/Tree.js
+++ b/src/devtools/views/Components/Tree.js
@@ -38,13 +38,22 @@ export default function Tree(props: Props) {
   const { lineHeight } = useContext(SettingsContext);
 
   // Make sure a newly selected element is visible in the list.
-  // This is helpful for things like the owners list.
+  // This is helpful for things like the owners list and search.
   useLayoutEffect(() => {
     if (selectedElementIndex !== null && listRef.current != null) {
       listRef.current.scrollToItem(selectedElementIndex);
+      // Note this autoscroll only works for rows.
+      // There's another autoscroll inside the elements
+      // that ensures the component name is visible horizontally.
+      // It's too early to do it now because the row might not exist yet.
     }
   }, [listRef, selectedElementIndex]);
 
+  // This ref is passed down the context to elements.
+  // It lets them avoid autoscrolling to the same item many times
+  // when a selected virtual row goes in and out of the viewport.
+  const lastScrolledIDRef = useRef(null);
+
   // Navigate the tree with up/down arrow keys.
   useEffect(() => {
     if (treeRef.current === null) {
@@ -98,8 +107,9 @@ export default function Tree(props: Props) {
       baseDepth,
       numElements,
       getElementAtIndex,
+      lastScrolledIDRef,
     }),
-    [baseDepth, numElements, getElementAtIndex]
+    [baseDepth, numElements, getElementAtIndex, lastScrolledIDRef]
   );
 
   return (