Merge pull request #4806 from urbit/lf/fix-virt-scroller

VirtualScroller: fix race conditions and excessive scroll adjusting

pkg/interface/src/logic/lib/virtualContext.tsx

@@ -7,6 +7,7 @@ import React, {
   useEffect,
 } from "react";
 import usePreviousValue from "./usePreviousValue";
+import {Primitive} from "~/types";
 
 export interface VirtualContextProps {
   save: () => void;
@@ -49,7 +50,7 @@ export function useVirtualResizeState(s: boolean) {
   return [state, setState] as const;
 }
 
-export function useVirtualResizeProp<T>(prop: T) {
+export function useVirtualResizeProp(prop: Primitive) {
   const { save, restore } = useVirtual();
   const oldProp = usePreviousValue(prop)
 
@@ -58,7 +59,7 @@ export function useVirtualResizeProp<T>(prop: T) {
   }
 
   useLayoutEffect(() => {
-    restore();
+    requestAnimationFrame(restore);
   }, [prop]);
 
 

pkg/interface/src/views/apps/permalinks/embed.tsx

@@ -18,7 +18,7 @@ import { GroupLink } from "~/views/components/GroupLink";
 import GlobalApi from "~/logic/api/global";
 import { getModuleIcon } from "~/logic/lib/util";
 import useMetadataState from "~/logic/state/metadata";
-import { Association, resourceFromPath } from "@urbit/api";
+import { Association, resourceFromPath, GraphNode } from "@urbit/api";
 import { Link } from "react-router-dom";
 import useGraphState from "~/logic/state/graph";
 import { GraphNodeContent } from "../notifications/graph";
@@ -51,7 +51,7 @@ function GraphPermalink(
   const { full = false, showOurContact, pending, link, graph, group, index, api, transcluded } = props;
   const { ship, name } = resourceFromPath(graph);
   const node = useGraphState(
-    useCallback((s) => s.looseNodes?.[`${ship.slice(1)}/${name}`]?.[index], [
+    useCallback((s) => s.looseNodes?.[`${ship.slice(1)}/${name}`]?.[index] as GraphNode, [
       graph,
       index,
     ])
@@ -63,7 +63,7 @@ function GraphPermalink(
     ])
   );
 
-  useVirtualResizeProp(node)
+  useVirtualResizeProp(!!node)
   useEffect(() => {
     (async () => {
       if (pending || !index) {

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,12 +477,13 @@ 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;
     const topSpacing = scrollHeight - scrollTop;
@@ -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());
     }
   }