From 6f7ed005aee37ef22bbf2ff4bd7825328731fa33 Mon Sep 17 00:00:00 2001
From: Liam Fitzgerald <liam.fitzgerald@tlon.io>
Date: Thu, 22 Apr 2021 15:04:38 +1000
Subject: [PATCH] VirtualScroller: fix race condition in ref deletion

A callback ref is called after the component is mounted, but before the
component is unmounted. However, we might still be adjusting scroll
position based on a component that is going to be remounted. Previously,
we delayed the deletion until the next tick with setTimeout. With the
faster ordered map implementation, the component may be remounted
before the next tick, leading to the deletion of a ref that is still
mounted. To work around this, we store a set of 'orphans' and clear the
map of orphans on an interval, and only clear the map if we are not
currently adjusting our scroll position. Also includes fixes for jumpy
scroll behaviour on initial mount.
---
 .../src/views/components/VirtualScroller.tsx  | 60 ++++++++++++++++---
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/pkg/interface/src/views/components/VirtualScroller.tsx b/pkg/interface/src/views/components/VirtualScroller.tsx
index 70f6dd8897..c7a3361ee0 100644
--- a/pkg/interface/src/views/components/VirtualScroller.tsx
+++ b/pkg/interface/src/views/components/VirtualScroller.tsx
@@ -115,6 +115,10 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
    * A map of child refs, used to calculate scroll position
    */
   private childRefs = new BigIntOrderedMap<HTMLElement>();
+  /**
+   * A set of child refs which have been unmounted
+   */
+  private orphans = new Set<string>();
   /**
    *  If saving, the bottommost visible element that we pin our scroll to
    */
@@ -140,6 +144,10 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
 
   private scrollRef: HTMLElement | null = null;
 
+  private cleanupRefInterval: NodeJS.Timeout | null = null;
+
+  private initScroll: NodeJS.Timeout | null = null;
+
   constructor(props: VirtualScrollerProps<T>) {
     super(props);
     this.state = {
@@ -157,6 +165,7 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
     this.onScroll = IS_IOS ? _.debounce(this.onScroll.bind(this), 400) : this.onScroll.bind(this);
     this.scrollKeyMap = this.scrollKeyMap.bind(this);
     this.setWindow = this.setWindow.bind(this);
+    this.restore = this.restore.bind(this);
   }
 
   componentDidMount() {
@@ -164,8 +173,27 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
     this.resetScroll();
     this.loadTop();
     this.loadBottom();
+    this.cleanupRefInterval = setInterval(this.cleanupRefs, 5000);
+    this.initScroll = setTimeout(() => {
+      log('scroll', 'initialised scroll');
+      this.restore();
+      this.initScroll = null;
+    }, 100);
   }
 
+
+
+  cleanupRefs = () => {
+    if(this.saveDepth > 0) {
+      return;
+    }
+    [...this.orphans].forEach(o => {
+      const index = bigInt(o);
+      this.childRefs.delete(index);
+    });
+    this.orphans.clear();
+  };
+  
   // manipulate scrollbar manually, to dodge change detection
   updateScroll = IS_IOS ? () => {} : _.throttle(() => {
     if(!this.window || !this.scrollRef) {
@@ -199,6 +227,12 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
 
   componentWillUnmount() {
     window.removeEventListener('keydown', this.invertedKeyHandler);
+    if(this.cleanupRefInterval) {
+      clearInterval(this.cleanupRefInterval);
+    }
+    if(this.initScroll) {
+      clearTimeout(this.initScroll);
+    }
   }
 
   startOffset() {
@@ -237,9 +271,6 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
     }, () => {
       requestAnimationFrame(() => {
         this.restore();
-        requestAnimationFrame(() => {
-
-        });
       });
     });
   }
@@ -339,6 +370,10 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
       // bail if we're going to adjust scroll anyway
       return;
     }
+    if(this.initScroll) {
+      clearTimeout(this.initScroll);
+      this.initScroll = null;
+    }
     if(this.saveDepth > 0) {
       log('bail', 'deep scroll queue');
       return;
@@ -394,8 +429,15 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
       log('bail', 'Deep restore');
       return;
     }
+    if(this.initScroll) {
+      log('bail', 'still initialising scroll');
+      return;
+    }
 
-    const ref = this.childRefs.get(this.savedIndex)!;
+    let ref = this.childRefs.get(this.savedIndex) 
+    if(!ref) {
+      return;
+    }
     const newScrollTop = this.window.scrollHeight - ref.offsetTop - this.savedDistance;
 
     this.window.scrollTo(0, newScrollTop);
@@ -435,11 +477,12 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
     if(!this.window || this.savedIndex) {
       return;
     }
-    this.saveDepth++;
-    if(this.saveDepth !== 1) {
+    if(this.saveDepth !== 0) {
       console.log('bail', 'deep save');
       return;
     }
+    
+    this.saveDepth++;
 
     let bottomIndex: BigInteger | null = null;
     const { scrollTop, scrollHeight } = this.window;
@@ -472,10 +515,9 @@ export default class VirtualScroller<T> extends Component<VirtualScrollerProps<T
   setRef = (element: HTMLElement | null, index: BigInteger) => {
     if(element) {
       this.childRefs.set(index, element);
+      this.orphans.delete(index.toString());
     } else {
-      setTimeout(() => {
-        this.childRefs.delete(index);
-      });
+      this.orphans.add(index.toString());
     }
   }