From 055c69de609ec14c4953c583931f3d45050878e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E4=BA=8C=E8=B4=A7=E6=9C=BA=E5=99=A8=E4=BA=BA?=
 <smith3816@gmail.com>
Date: Tue, 19 Sep 2023 20:48:03 +0800
Subject: [PATCH 1/6] refactor: use layout effect for scroll

---
 src/hooks/useScrollTo.tsx | 166 ++++++++++++++++++++++----------------
 1 file changed, 97 insertions(+), 69 deletions(-)

diff --git a/src/hooks/useScrollTo.tsx b/src/hooks/useScrollTo.tsx
index 4bc038f7..a41aee5d 100644
--- a/src/hooks/useScrollTo.tsx
+++ b/src/hooks/useScrollTo.tsx
@@ -3,6 +3,9 @@ import * as React from 'react';
 import raf from 'rc-util/lib/raf';
 import type { GetKey } from '../interface';
 import type CacheMap from '../utils/CacheMap';
+import useLayoutEffect from 'rc-util/lib/hooks/useLayoutEffect';
+
+const MAX_TIMES = 3;
 
 export type ScrollAlign = 'top' | 'bottom' | 'auto';
 
@@ -35,6 +38,94 @@ export default function useScrollTo<T>(
 ): (arg: number | ScrollTarget) => void {
   const scrollRef = React.useRef<number>();
 
+  const [syncState, setSyncState] = React.useState<{
+    times: number;
+    index: number;
+    offset: number;
+    originAlign: ScrollAlign;
+    targetAlign?: 'top' | 'bottom';
+  }>(null);
+
+  // ========================== Sync Scroll ==========================
+  useLayoutEffect(() => {
+    if (syncState && syncState.times < MAX_TIMES) {
+      // Never reach
+      if (!containerRef.current) {
+        setSyncState((ori) => ({ ...ori }));
+        return;
+      }
+
+      const { targetAlign, originAlign, index, offset } = syncState;
+
+      const height = containerRef.current.clientHeight;
+      let needCollectHeight = false;
+      let newTargetAlign: 'top' | 'bottom' | null = targetAlign;
+
+      // Go to next frame if height not exist
+      if (height) {
+        const mergedAlign = targetAlign || originAlign;
+
+        // Get top & bottom
+        let stackTop = 0;
+        let itemTop = 0;
+        let itemBottom = 0;
+
+        const maxLen = Math.min(data.length, index);
+
+        for (let i = 0; i <= maxLen; i += 1) {
+          const key = getKey(data[i]);
+          itemTop = stackTop;
+          const cacheHeight = heights.get(key);
+          itemBottom = itemTop + (cacheHeight === undefined ? itemHeight : cacheHeight);
+
+          stackTop = itemBottom;
+
+          if (i === index && cacheHeight === undefined) {
+            needCollectHeight = true;
+          }
+        }
+
+        // Scroll to
+        let targetTop: number | null = null;
+
+        switch (mergedAlign) {
+          case 'top':
+            targetTop = itemTop - offset;
+            break;
+          case 'bottom':
+            targetTop = itemBottom - height + offset;
+            break;
+
+          default: {
+            const { scrollTop } = containerRef.current;
+            const scrollBottom = scrollTop + height;
+            if (itemTop < scrollTop) {
+              newTargetAlign = 'top';
+            } else if (itemBottom > scrollBottom) {
+              newTargetAlign = 'bottom';
+            }
+          }
+        }
+
+        if (targetTop !== null && targetTop !== containerRef.current.scrollTop) {
+          syncScrollTop(targetTop);
+        }
+      }
+
+      // Trigger next effect
+      if (needCollectHeight) {
+        collectHeight();
+      }
+
+      setSyncState((ori) => ({
+        ...ori,
+        times: ori.times + 1,
+        targetAlign: newTargetAlign,
+      }));
+    }
+  }, [syncState, containerRef.current]);
+
+  // =========================== Scroll To ===========================
   return (arg) => {
     // When not argument provided, we think dev may want to show the scrollbar
     if (arg === null || arg === undefined) {
@@ -59,75 +150,12 @@ export default function useScrollTo<T>(
 
       const { offset = 0 } = arg;
 
-      // We will retry 3 times in case dynamic height shaking
-      const syncScroll = (times: number, targetAlign?: 'top' | 'bottom') => {
-        if (times < 0 || !containerRef.current) return;
-
-        const height = containerRef.current.clientHeight;
-        let needCollectHeight = false;
-        let newTargetAlign: 'top' | 'bottom' | null = targetAlign;
-
-        // Go to next frame if height not exist
-        if (height) {
-          const mergedAlign = targetAlign || align;
-
-          // Get top & bottom
-          let stackTop = 0;
-          let itemTop = 0;
-          let itemBottom = 0;
-
-          const maxLen = Math.min(data.length, index);
-
-          for (let i = 0; i <= maxLen; i += 1) {
-            const key = getKey(data[i]);
-            itemTop = stackTop;
-            const cacheHeight = heights.get(key);
-            itemBottom = itemTop + (cacheHeight === undefined ? itemHeight : cacheHeight);
-
-            stackTop = itemBottom;
-
-            if (i === index && cacheHeight === undefined) {
-              needCollectHeight = true;
-            }
-          }
-
-          // Scroll to
-          let targetTop: number | null = null;
-
-          switch (mergedAlign) {
-            case 'top':
-              targetTop = itemTop - offset;
-              break;
-            case 'bottom':
-              targetTop = itemBottom - height + offset;
-              break;
-
-            default: {
-              const { scrollTop } = containerRef.current;
-              const scrollBottom = scrollTop + height;
-              if (itemTop < scrollTop) {
-                newTargetAlign = 'top';
-              } else if (itemBottom > scrollBottom) {
-                newTargetAlign = 'bottom';
-              }
-            }
-          }
-
-          if (targetTop !== null && targetTop !== containerRef.current.scrollTop) {
-            syncScrollTop(targetTop);
-          }
-        }
-
-        // We will retry since element may not sync height as it described
-        scrollRef.current = raf(() => {
-          if (needCollectHeight) {
-            collectHeight();
-          }
-          syncScroll(times - 1, newTargetAlign);
-        }, 2); // Delay 2 to wait for List collect heights
-      };
-
-      syncScroll(3);
+      setSyncState({
+        times: 0,
+        index,
+        offset,
+        originAlign: align,
+      });
     }
   };
 }

From 8e717de8b55f56039f2f1512a57cd2ded3a8afe1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E4=BA=8C=E8=B4=A7=E6=9C=BA=E5=99=A8=E4=BA=BA?=
 <smith3816@gmail.com>
Date: Tue, 19 Sep 2023 21:52:38 +0800
Subject: [PATCH 2/6] refactor: record heights

---
 examples/basic.tsx        | 19 ++++++++--------
 src/List.tsx              |  2 +-
 src/hooks/useHeights.tsx  | 19 ++++++++++++----
 src/hooks/useScrollTo.tsx | 46 ++++++++++++++++++++++++++++++---------
 4 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/examples/basic.tsx b/examples/basic.tsx
index 5d50c92a..2f14a202 100644
--- a/examples/basic.tsx
+++ b/examples/basic.tsx
@@ -1,18 +1,17 @@
-/* eslint-disable jsx-a11y/label-has-associated-control, jsx-a11y/label-has-for */
 import * as React from 'react';
-import List, { ListRef } from '../src/List';
+import List, { type ListRef } from '../src/List';
 import './basic.less';
 
 interface Item {
-  id: string;
+  id: number;
 }
 
 const MyItem: React.ForwardRefRenderFunction<any, Item> = ({ id }, ref) => (
   <span
     ref={ref}
-    // style={{
-    //   // height: 30 + (id % 2 ? 0 : 10),
-    // }}
+    style={{
+      height: 30 + (id % 2 ? 0 : 10),
+    }}
     className="fixed-item"
     onClick={() => {
       console.log('Click:', id);
@@ -35,7 +34,7 @@ class TestItem extends React.Component<Item, {}> {
 const data: Item[] = [];
 for (let i = 0; i < 1000; i += 1) {
   data.push({
-    id: String(i),
+    id: i,
   });
 }
 
@@ -44,7 +43,7 @@ const TYPES = [
   { name: 'ref react node', type: 'react', component: TestItem },
 ];
 
-const onScroll: React.UIEventHandler<HTMLElement> = e => {
+const onScroll: React.UIEventHandler<HTMLElement> = (e) => {
   console.log('scroll:', e.currentTarget.scrollTop);
 };
 
@@ -160,7 +159,7 @@ const Demo = () => {
           type="button"
           onClick={() => {
             listRef.current.scrollTo({
-              key: '50',
+              key: 50,
               align: 'auto',
             });
           }}
@@ -171,7 +170,7 @@ const Demo = () => {
         <button
           type="button"
           onClick={() => {
-            setVisible(v => !v);
+            setVisible((v) => !v);
           }}
         >
           visible
diff --git a/src/List.tsx b/src/List.tsx
index 9b6d892b..2a3fd3b6 100644
--- a/src/List.tsx
+++ b/src/List.tsx
@@ -431,7 +431,7 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) {
     heights,
     itemHeight,
     getKey,
-    collectHeight,
+    () => collectHeight(true),
     syncScrollTop,
     delayHideScrollBar,
   );
diff --git a/src/hooks/useHeights.tsx b/src/hooks/useHeights.tsx
index 9a6bb832..ad45d4a2 100644
--- a/src/hooks/useHeights.tsx
+++ b/src/hooks/useHeights.tsx
@@ -9,7 +9,12 @@ export default function useHeights<T>(
   getKey: GetKey<T>,
   onItemAdd?: (item: T) => void,
   onItemRemove?: (item: T) => void,
-): [(item: T, instance: HTMLElement) => void, () => void, CacheMap, number] {
+): [
+  setInstanceRef: (item: T, instance: HTMLElement) => void,
+  collectHeight: (sync?: boolean) => void,
+  cacheMap: CacheMap,
+  updatedMark: number,
+] {
   const [updatedMark, setUpdatedMark] = React.useState(0);
   const instanceRef = useRef(new Map<React.Key, HTMLElement>());
   const heightsRef = useRef(new CacheMap());
@@ -19,10 +24,10 @@ export default function useHeights<T>(
     raf.cancel(collectRafRef.current);
   }
 
-  function collectHeight() {
+  function collectHeight(sync = false) {
     cancelRaf();
 
-    collectRafRef.current = raf(() => {
+    const doCollect = () => {
       instanceRef.current.forEach((element, key) => {
         if (element && element.offsetParent) {
           const htmlElement = findDOMNode<HTMLElement>(element);
@@ -35,7 +40,13 @@ export default function useHeights<T>(
 
       // Always trigger update mark to tell parent that should re-calculate heights when resized
       setUpdatedMark((c) => c + 1);
-    });
+    };
+
+    if (sync) {
+      doCollect();
+    } else {
+      collectRafRef.current = raf(doCollect);
+    }
   }
 
   function setInstanceRef(item: T, instance: HTMLElement) {
diff --git a/src/hooks/useScrollTo.tsx b/src/hooks/useScrollTo.tsx
index a41aee5d..19a33b6e 100644
--- a/src/hooks/useScrollTo.tsx
+++ b/src/hooks/useScrollTo.tsx
@@ -4,8 +4,9 @@ import raf from 'rc-util/lib/raf';
 import type { GetKey } from '../interface';
 import type CacheMap from '../utils/CacheMap';
 import useLayoutEffect from 'rc-util/lib/hooks/useLayoutEffect';
+import { warning } from 'rc-util';
 
-const MAX_TIMES = 3;
+const MAX_TIMES = 10;
 
 export type ScrollAlign = 'top' | 'bottom' | 'auto';
 
@@ -48,6 +49,7 @@ export default function useScrollTo<T>(
 
   // ========================== Sync Scroll ==========================
   useLayoutEffect(() => {
+    console.log('🚨 Effect!', syncState?.times, '~~~~~~~~~~~~~~~~~');
     if (syncState && syncState.times < MAX_TIMES) {
       // Never reach
       if (!containerRef.current) {
@@ -55,6 +57,8 @@ export default function useScrollTo<T>(
         return;
       }
 
+      collectHeight();
+
       const { targetAlign, originAlign, index, offset } = syncState;
 
       const height = containerRef.current.clientHeight;
@@ -79,9 +83,22 @@ export default function useScrollTo<T>(
           itemBottom = itemTop + (cacheHeight === undefined ? itemHeight : cacheHeight);
 
           stackTop = itemBottom;
+        }
+
+        // Check if need sync height (visible range has item not record height)
+        let leftHeight = height;
+        for (let i = maxLen; i >= 0; i -= 1) {
+          const key = getKey(data[i]);
+          const cacheHeight = heights.get(key);
 
-          if (i === index && cacheHeight === undefined) {
+          if (cacheHeight === undefined) {
             needCollectHeight = true;
+            break;
+          }
+
+          leftHeight -= cacheHeight;
+          if (leftHeight <= 0) {
+            break;
           }
         }
 
@@ -107,21 +124,30 @@ export default function useScrollTo<T>(
           }
         }
 
-        if (targetTop !== null && targetTop !== containerRef.current.scrollTop) {
+        console.log('sync top:', targetTop, containerRef.current.scrollTop);
+
+        if (targetTop !== null) {
+          console.log('sss!');
           syncScrollTop(targetTop);
+        } else {
+          needCollectHeight = true;
         }
       }
 
+      console.log('Need?', needCollectHeight);
       // Trigger next effect
       if (needCollectHeight) {
-        collectHeight();
+        setSyncState((ori) => ({
+          ...ori,
+          times: ori.times + 1,
+          targetAlign: newTargetAlign,
+        }));
       }
-
-      setSyncState((ori) => ({
-        ...ori,
-        times: ori.times + 1,
-        targetAlign: newTargetAlign,
-      }));
+    } else if (process.env.NODE_ENV !== 'production' && syncState?.times === MAX_TIMES) {
+      warning(
+        false,
+        'Seems `scrollTo` with `rc-virtual-list` reach toe max limitation. Please fire issue for us. Thanks.',
+      );
     }
   }, [syncState, containerRef.current]);
 

From 5cd806baa1dd1072c89a2942dd0d90e1afa291b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E4=BA=8C=E8=B4=A7=E6=9C=BA=E5=99=A8=E4=BA=BA?=
 <smith3816@gmail.com>
Date: Tue, 19 Sep 2023 22:01:21 +0800
Subject: [PATCH 3/6] chore: auto roll

---
 src/hooks/useScrollTo.tsx | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/hooks/useScrollTo.tsx b/src/hooks/useScrollTo.tsx
index 19a33b6e..b1cabc1f 100644
--- a/src/hooks/useScrollTo.tsx
+++ b/src/hooks/useScrollTo.tsx
@@ -86,7 +86,7 @@ export default function useScrollTo<T>(
         }
 
         // Check if need sync height (visible range has item not record height)
-        let leftHeight = height;
+        let leftHeight = mergedAlign === 'top' ? offset : height - offset;
         for (let i = maxLen; i >= 0; i -= 1) {
           const key = getKey(data[i]);
           const cacheHeight = heights.get(key);
@@ -104,6 +104,7 @@ export default function useScrollTo<T>(
 
         // Scroll to
         let targetTop: number | null = null;
+        let inView = false;
 
         switch (mergedAlign) {
           case 'top':
@@ -120,16 +121,25 @@ export default function useScrollTo<T>(
               newTargetAlign = 'top';
             } else if (itemBottom > scrollBottom) {
               newTargetAlign = 'bottom';
+            } else {
+              // No need to collect since already in view
+              inView = true;
             }
           }
         }
 
-        console.log('sync top:', targetTop, containerRef.current.scrollTop);
+        console.log(
+          'sync top:',
+          targetTop,
+          containerRef.current.scrollTop,
+          mergedAlign,
+          newTargetAlign,
+        );
 
         if (targetTop !== null) {
           console.log('sss!');
           syncScrollTop(targetTop);
-        } else {
+        } else if (!inView) {
           needCollectHeight = true;
         }
       }

From acfc059b8f94a6b8cedfbfdb33777a510bb4a86e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E4=BA=8C=E8=B4=A7=E6=9C=BA=E5=99=A8=E4=BA=BA?=
 <smith3816@gmail.com>
Date: Tue, 19 Sep 2023 22:03:44 +0800
Subject: [PATCH 4/6] chore: clean up

---
 src/hooks/useScrollTo.tsx | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/src/hooks/useScrollTo.tsx b/src/hooks/useScrollTo.tsx
index b1cabc1f..04030b73 100644
--- a/src/hooks/useScrollTo.tsx
+++ b/src/hooks/useScrollTo.tsx
@@ -49,7 +49,6 @@ export default function useScrollTo<T>(
 
   // ========================== Sync Scroll ==========================
   useLayoutEffect(() => {
-    console.log('🚨 Effect!', syncState?.times, '~~~~~~~~~~~~~~~~~');
     if (syncState && syncState.times < MAX_TIMES) {
       // Never reach
       if (!containerRef.current) {
@@ -128,23 +127,13 @@ export default function useScrollTo<T>(
           }
         }
 
-        console.log(
-          'sync top:',
-          targetTop,
-          containerRef.current.scrollTop,
-          mergedAlign,
-          newTargetAlign,
-        );
-
         if (targetTop !== null) {
-          console.log('sss!');
           syncScrollTop(targetTop);
         } else if (!inView) {
           needCollectHeight = true;
         }
       }
 
-      console.log('Need?', needCollectHeight);
       // Trigger next effect
       if (needCollectHeight) {
         setSyncState((ori) => ({

From f7bb858e8704b3f02e1b4bcb8cf17fc79944dfeb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E4=BA=8C=E8=B4=A7=E6=9C=BA=E5=99=A8=E4=BA=BA?=
 <smith3816@gmail.com>
Date: Tue, 19 Sep 2023 22:50:17 +0800
Subject: [PATCH 5/6] test: fix test case

---
 tests/scroll.test.js | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/tests/scroll.test.js b/tests/scroll.test.js
index f1173920..b0562c0b 100644
--- a/tests/scroll.test.js
+++ b/tests/scroll.test.js
@@ -82,17 +82,38 @@ describe('List.Scroll', () => {
     const listRef = React.createRef();
     const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100), ref: listRef });
 
+    function presetList() {
+      const ref = React.createRef();
+
+      const result = genList({ itemHeight: 20, height: 100, data: genData(100), ref }, render);
+
+      return {
+        ...result,
+        ref,
+      };
+    }
+
     describe('index scroll', () => {
-      it('work', () => {
-        listRef.current.scrollTo({ index: 30, align: 'top' });
-        jest.runAllTimers();
-        expect(wrapper.find('ul').instance().scrollTop).toEqual(600);
+      it('work in range', () => {
+        const { ref, container } = presetList();
+
+        ref.current.scrollTo({ index: 30, align: 'top' });
+
+        act(() => {
+          jest.runAllTimers();
+        });
+
+        expect(container.querySelector('ul').scrollTop).toEqual(600);
       });
 
       it('out of range should not crash', () => {
         expect(() => {
-          listRef.current.scrollTo({ index: 99999999999, align: 'top' });
-          jest.runAllTimers();
+          const { ref } = presetList();
+          ref.current.scrollTo({ index: 99999999999, align: 'top' });
+
+          act(() => {
+            jest.runAllTimers();
+          });
         }).not.toThrow();
       });
     });

From 8ad4ae4c921f37f6f60c8993e0b010f88e8420df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E4=BA=8C=E8=B4=A7=E6=9C=BA=E5=99=A8=E4=BA=BA?=
 <smith3816@gmail.com>
Date: Tue, 19 Sep 2023 22:59:14 +0800
Subject: [PATCH 6/6] test: update test

---
 tests/scroll.test.js | 54 ++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/tests/scroll.test.js b/tests/scroll.test.js
index b0562c0b..dd61bf96 100644
--- a/tests/scroll.test.js
+++ b/tests/scroll.test.js
@@ -79,9 +79,6 @@ describe('List.Scroll', () => {
   });
 
   describe('scroll to object', () => {
-    const listRef = React.createRef();
-    const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100), ref: listRef });
-
     function presetList() {
       const ref = React.createRef();
 
@@ -90,56 +87,55 @@ describe('List.Scroll', () => {
       return {
         ...result,
         ref,
+        scrollTo: (...args) => {
+          ref.current.scrollTo(...args);
+
+          act(() => {
+            jest.runAllTimers();
+          });
+        },
       };
     }
 
     describe('index scroll', () => {
       it('work in range', () => {
-        const { ref, container } = presetList();
-
-        ref.current.scrollTo({ index: 30, align: 'top' });
+        const { scrollTo, container } = presetList();
 
-        act(() => {
-          jest.runAllTimers();
-        });
+        scrollTo({ index: 30, align: 'top' });
 
         expect(container.querySelector('ul').scrollTop).toEqual(600);
       });
 
       it('out of range should not crash', () => {
         expect(() => {
-          const { ref } = presetList();
-          ref.current.scrollTo({ index: 99999999999, align: 'top' });
-
-          act(() => {
-            jest.runAllTimers();
-          });
+          const { scrollTo } = presetList();
+          scrollTo({ index: 99999999999, align: 'top' });
         }).not.toThrow();
       });
     });
 
     it('scroll top should not out of range', () => {
-      listRef.current.scrollTo({ index: 0, align: 'bottom' });
+      const { scrollTo, container } = presetList();
+      scrollTo({ index: 0, align: 'bottom' });
       jest.runAllTimers();
-      expect(wrapper.find('ul').instance().scrollTop).toEqual(0);
+      expect(container.querySelector('ul').scrollTop).toEqual(0);
     });
 
     it('key scroll', () => {
-      listRef.current.scrollTo({ key: '30', align: 'bottom' });
-      jest.runAllTimers();
-      expect(wrapper.find('ul').instance().scrollTop).toEqual(520);
+      const { scrollTo, container } = presetList();
+      scrollTo({ key: '30', align: 'bottom' });
+      expect(container.querySelector('ul').scrollTop).toEqual(520);
     });
 
     it('smart', () => {
-      listRef.current.scrollTo(0);
-      listRef.current.scrollTo({ index: 30 });
-      jest.runAllTimers();
-      expect(wrapper.find('ul').instance().scrollTop).toEqual(520);
-
-      listRef.current.scrollTo(800);
-      listRef.current.scrollTo({ index: 30 });
-      jest.runAllTimers();
-      expect(wrapper.find('ul').instance().scrollTop).toEqual(600);
+      const { scrollTo, container } = presetList();
+      scrollTo(0);
+      scrollTo({ index: 30 });
+      expect(container.querySelector('ul').scrollTop).toEqual(520);
+
+      scrollTo(800);
+      scrollTo({ index: 30 });
+      expect(container.querySelector('ul').scrollTop).toEqual(600);
     });
   });