Infinite scroll: implementation clean up (#81725)

* Infinite scroll: clean up for clarity

* Infinite scroll: clean up test

* Formatting

* Infinite scroll: disable by feature flag

* Infinite scroll: improve visibility of the lower loader

public/app/features/logs/components/InfiniteScroll.test.tsx

@@ -3,6 +3,7 @@ import React, { useEffect, useRef, useState } from 'react';
 
 import { LogRowModel, dateTimeForTimeZone } from '@grafana/data';
 import { convertRawToRange } from '@grafana/data/src/datetime/rangeutil';
+import { config } from '@grafana/runtime';
 import { LogsSortOrder } from '@grafana/schema';
 
 import { InfiniteScroll, Props, SCROLLING_THRESHOLD } from './InfiniteScroll';
@@ -50,6 +51,45 @@ function ScrollWithWrapper({ children, ...props }: Props) {
   );
 }
 
+function setup(loadMoreMock: () => void, startPosition: number, rows: LogRowModel[], order: LogsSortOrder) {
+  const { element, events } = getMockElement(startPosition);
+
+  function scrollTo(position: number) {
+    element.scrollTop = position;
+
+    act(() => {
+      events['scroll'](new Event('scroll'));
+    });
+  }
+  function wheel(deltaY: number) {
+    act(() => {
+      const event = new WheelEvent('wheel', { deltaY });
+      events['wheel'](event);
+    });
+  }
+
+  render(
+    <InfiniteScroll
+      {...defaultProps}
+      sortOrder={order}
+      rows={rows}
+      scrollElement={element as unknown as HTMLDivElement}
+      loadMoreLogs={loadMoreMock}
+    >
+      <div data-testid="contents" style={{ height: 100 }} />
+    </InfiniteScroll>
+  );
+
+  return { element, events, scrollTo, wheel };
+}
+
+beforeAll(() => {
+  config.featureToggles.logsInfiniteScrolling = true;
+});
+afterAll(() => {
+  config.featureToggles.logsInfiniteScrolling = false;
+});
+
 describe('InfiniteScroll', () => {
   test('Wraps components without adding DOM elements', async () => {
     const { container } = render(
@@ -81,39 +121,29 @@ describe('InfiniteScroll', () => {
         rows = createLogRows(absoluteRange.from + 2 * SCROLLING_THRESHOLD, absoluteRange.to - 2 * SCROLLING_THRESHOLD);
       });
 
-      function setup(loadMoreMock: () => void, startPosition: number) {
-        const { element, events } = getMockElement(startPosition);
-        render(
-          <InfiniteScroll
-            {...defaultProps}
-            sortOrder={order}
-            rows={rows}
-            scrollElement={element as unknown as HTMLDivElement}
-            loadMoreLogs={loadMoreMock}
-          >
-            <div data-testid="contents" style={{ height: 100 }} />
-          </InfiniteScroll>
-        );
-        return { element, events };
-      }
-
       test.each([
         ['top', 10, 0],
-        ['bottom', 90, 100],
+        ['bottom', 50, 60],
-      ])('Requests more logs when scrolling %s', async (_: string, startPosition: number, endPosition: number) => {
+      ])(
-        const loadMoreMock = jest.fn();
+        'Requests more logs when scrolling %s',
-        const { element, events } = setup(loadMoreMock, startPosition);
+        async (direction: string, startPosition: number, endPosition: number) => {
+          const loadMoreMock = jest.fn();
+          const { scrollTo, element } = setup(loadMoreMock, startPosition, rows, order);
 
-        expect(await screen.findByTestId('contents')).toBeInTheDocument();
+          expect(await screen.findByTestId('contents')).toBeInTheDocument();
-        element.scrollTop = endPosition;
 
-        act(() => {
+          scrollTo(endPosition);
-          events['scroll'](new Event('scroll'));
-        });
 
-        expect(loadMoreMock).toHaveBeenCalled();
+          expect(loadMoreMock).toHaveBeenCalled();
-        expect(await screen.findByTestId('Spinner')).toBeInTheDocument();
+          expect(await screen.findByTestId('Spinner')).toBeInTheDocument();
-      });
+          if (direction === 'bottom') {
+            // Bottom loader visibility trick
+            expect(element.scrollTo).toHaveBeenCalled();
+          } else {
+            expect(element.scrollTo).not.toHaveBeenCalled();
+          }
+        }
+      );
 
       test.each([
         ['up', -5, 0],
@@ -122,14 +152,11 @@ describe('InfiniteScroll', () => {
         'Requests more logs when moving the mousewheel %s',
         async (_: string, deltaY: number, startPosition: number) => {
           const loadMoreMock = jest.fn();
-          const { events } = setup(loadMoreMock, startPosition);
+          const { wheel } = setup(loadMoreMock, startPosition, rows, order);
 
           expect(await screen.findByTestId('contents')).toBeInTheDocument();
 
-          act(() => {
+          wheel(deltaY);
-            const event = new WheelEvent('wheel', { deltaY });
-            events['wheel'](event);
-          });
 
           expect(loadMoreMock).toHaveBeenCalled();
           expect(await screen.findByTestId('Spinner')).toBeInTheDocument();
@@ -138,33 +165,28 @@ describe('InfiniteScroll', () => {
 
       test('Does not request more logs when there is no scroll', async () => {
         const loadMoreMock = jest.fn();
-        const { element, events } = setup(loadMoreMock, 0);
+        const { scrollTo, element } = setup(loadMoreMock, 0, rows, order);
 
         expect(await screen.findByTestId('contents')).toBeInTheDocument();
         element.clientHeight = 40;
         element.scrollHeight = element.clientHeight;
 
-        act(() => {
+        scrollTo(40);
-          events['scroll'](new Event('scroll'));
-        });
 
         expect(loadMoreMock).not.toHaveBeenCalled();
         expect(screen.queryByTestId('Spinner')).not.toBeInTheDocument();
       });
 
       test('Requests newer logs from the most recent timestamp', async () => {
-        const startPosition = order === LogsSortOrder.Descending ? 10 : 90; // Scroll top
+        const startPosition = order === LogsSortOrder.Descending ? 10 : 50; // Scroll top
-        const endPosition = order === LogsSortOrder.Descending ? 0 : 100; // Scroll bottom
+        const endPosition = order === LogsSortOrder.Descending ? 0 : 60; // Scroll bottom
 
         const loadMoreMock = jest.fn();
-        const { element, events } = setup(loadMoreMock, startPosition);
+        const { scrollTo } = setup(loadMoreMock, startPosition, rows, order);
 
         expect(await screen.findByTestId('contents')).toBeInTheDocument();
-        element.scrollTop = endPosition;
 
-        act(() => {
+        scrollTo(endPosition);
-          events['scroll'](new Event('scroll'));
-        });
 
         expect(loadMoreMock).toHaveBeenCalledWith({
           from: rows[rows.length - 1].timeEpochMs,
@@ -173,18 +195,15 @@ describe('InfiniteScroll', () => {
       });
 
       test('Requests older logs from the oldest timestamp', async () => {
-        const startPosition = order === LogsSortOrder.Ascending ? 10 : 90; // Scroll top
+        const startPosition = order === LogsSortOrder.Ascending ? 10 : 50; // Scroll top
-        const endPosition = order === LogsSortOrder.Ascending ? 0 : 100; // Scroll bottom
+        const endPosition = order === LogsSortOrder.Ascending ? 0 : 60; // Scroll bottom
 
         const loadMoreMock = jest.fn();
-        const { element, events } = setup(loadMoreMock, startPosition);
+        const { scrollTo } = setup(loadMoreMock, startPosition, rows, order);
 
         expect(await screen.findByTestId('contents')).toBeInTheDocument();
-        element.scrollTop = endPosition;
 
-        act(() => {
+        scrollTo(endPosition);
-          events['scroll'](new Event('scroll'));
-        });
 
         expect(loadMoreMock).toHaveBeenCalledWith({
           from: absoluteRange.from,
@@ -193,39 +212,20 @@ describe('InfiniteScroll', () => {
       });
 
       describe('With absolute range matching visible range', () => {
-        function setup(loadMoreMock: () => void, startPosition: number, rows: LogRowModel[]) {
-          const { element, events } = getMockElement(startPosition);
-          render(
-            <InfiniteScroll
-              {...defaultProps}
-              sortOrder={order}
-              rows={rows}
-              scrollElement={element as unknown as HTMLDivElement}
-              loadMoreLogs={loadMoreMock}
-            >
-              <div data-testid="contents" style={{ height: 100 }} />
-            </InfiniteScroll>
-          );
-          return { element, events };
-        }
-
         test.each([
           ['top', 10, 0],
-          ['bottom', 90, 100],
+          ['bottom', 50, 60],
         ])(
           'It does not request more when scrolling %s',
           async (_: string, startPosition: number, endPosition: number) => {
             // Visible range matches the current range
             const rows = createLogRows(absoluteRange.from, absoluteRange.to);
             const loadMoreMock = jest.fn();
-            const { element, events } = setup(loadMoreMock, startPosition, rows);
+            const { scrollTo } = setup(loadMoreMock, startPosition, rows, order);
 
             expect(await screen.findByTestId('contents')).toBeInTheDocument();
-            element.scrollTop = endPosition;
 
-            act(() => {
+            scrollTo(endPosition);
-              events['scroll'](new Event('scroll'));
-            });
 
             expect(loadMoreMock).not.toHaveBeenCalled();
             expect(screen.queryByTestId('Spinner')).not.toBeInTheDocument();
@@ -235,39 +235,20 @@ describe('InfiniteScroll', () => {
       });
 
       describe('With relative range matching visible range', () => {
-        function setup(loadMoreMock: () => void, startPosition: number, rows: LogRowModel[]) {
-          const { element, events } = getMockElement(startPosition);
-          render(
-            <InfiniteScroll
-              {...defaultProps}
-              sortOrder={order}
-              rows={rows}
-              scrollElement={element as unknown as HTMLDivElement}
-              loadMoreLogs={loadMoreMock}
-            >
-              <div data-testid="contents" style={{ height: 100 }} />
-            </InfiniteScroll>
-          );
-          return { element, events };
-        }
-
         test.each([
           ['top', 10, 0],
-          ['bottom', 90, 100],
+          ['bottom', 50, 60],
         ])(
           'It does not request more when scrolling %s',
           async (_: string, startPosition: number, endPosition: number) => {
             // Visible range matches the current range
             const rows = createLogRows(absoluteRange.from, absoluteRange.to);
             const loadMoreMock = jest.fn();
-            const { element, events } = setup(loadMoreMock, startPosition, rows);
+            const { scrollTo } = setup(loadMoreMock, startPosition, rows, order);
 
             expect(await screen.findByTestId('contents')).toBeInTheDocument();
-            element.scrollTop = endPosition;
 
-            act(() => {
+            scrollTo(endPosition);
-              events['scroll'](new Event('scroll'));
-            });
 
             expect(loadMoreMock).not.toHaveBeenCalled();
             expect(screen.queryByTestId('Spinner')).not.toBeInTheDocument();
@@ -301,6 +282,7 @@ function getMockElement(scrollTop: number) {
     scrollHeight: 100,
     clientHeight: 40,
     scrollTop,
+    scrollTo: jest.fn(),
   };
 
   return { element, events };

public/app/features/logs/components/InfiniteScroll.tsx

@@ -3,7 +3,7 @@ import React, { ReactNode, useEffect, useRef, useState } from 'react';
 
 import { AbsoluteTimeRange, LogRowModel, TimeRange } from '@grafana/data';
 import { convertRawToRange, isRelativeTime, isRelativeTimeRange } from '@grafana/data/src/datetime/rangeutil';
-import { reportInteraction } from '@grafana/runtime';
+import { config, reportInteraction } from '@grafana/runtime';
 import { LogsSortOrder, TimeZone } from '@grafana/schema';
 
 import { LoadingIndicator } from './LoadingIndicator';
@@ -33,13 +33,16 @@ export const InfiniteScroll = ({
   const [lowerOutOfRange, setLowerOutOfRange] = useState(false);
   const [upperLoading, setUpperLoading] = useState(false);
   const [lowerLoading, setLowerLoading] = useState(false);
+  const rowsRef = useRef<LogRowModel[]>(rows);
   const lastScroll = useRef<number>(scrollElement?.scrollTop || 0);
 
+  // Reset messages when range/order/rows change
   useEffect(() => {
     setUpperOutOfRange(false);
     setLowerOutOfRange(false);
   }, [range, rows, sortOrder]);
 
+  // Reset loading messages when loading stops
   useEffect(() => {
     if (!loading) {
       setUpperLoading(false);
@@ -47,13 +50,32 @@ export const InfiniteScroll = ({
     }
   }, [loading]);
 
+  // Ensure bottom loader visibility
+  useEffect(() => {
+    if (lowerLoading && scrollElement) {
+      scrollElement.scrollTo(0, scrollElement.scrollHeight - scrollElement.clientHeight);
+    }
+  }, [lowerLoading, scrollElement]);
+
+  // Request came back with no new past rows
+  useEffect(() => {
+    if (rows !== rowsRef.current && rows.length === rowsRef.current.length && (upperLoading || lowerLoading)) {
+      if (sortOrder === LogsSortOrder.Descending && lowerLoading) {
+        setLowerOutOfRange(true);
+      } else if (sortOrder === LogsSortOrder.Ascending && upperLoading) {
+        setUpperOutOfRange(true);
+      }
+    }
+    rowsRef.current = rows;
+  }, [lowerLoading, rows, sortOrder, upperLoading]);
+
   useEffect(() => {
     if (!scrollElement || !loadMoreLogs) {
       return;
     }
 
     function handleScroll(event: Event | WheelEvent) {
-      if (!scrollElement || !loadMoreLogs || !rows.length || loading) {
+      if (!scrollElement || !loadMoreLogs || !rows.length || loading || !config.featureToggles.logsInfiniteScrolling) {
         return;
       }
       event.stopImmediatePropagation();
@@ -69,15 +91,12 @@ export const InfiniteScroll = ({
     }
 
     function scrollTop() {
-      if (!canScrollTop(getVisibleRange(rows), range, timeZone, sortOrder)) {
+      const newRange = canScrollTop(getVisibleRange(rows), range, timeZone, sortOrder);
+      if (!newRange) {
         setUpperOutOfRange(true);
         return;
       }
       setUpperOutOfRange(false);
-      const newRange =
-        sortOrder === LogsSortOrder.Descending
-          ? getNextRange(getVisibleRange(rows), range, timeZone)
-          : getPrevRange(getVisibleRange(rows), range);
       loadMoreLogs?.(newRange);
       setUpperLoading(true);
       reportInteraction('grafana_logs_infinite_scrolling', {
@@ -87,15 +106,12 @@ export const InfiniteScroll = ({
     }
 
     function scrollBottom() {
-      if (!canScrollBottom(getVisibleRange(rows), range, timeZone, sortOrder)) {
+      const newRange = canScrollBottom(getVisibleRange(rows), range, timeZone, sortOrder);
+      if (!newRange) {
         setLowerOutOfRange(true);
         return;
       }
       setLowerOutOfRange(false);
-      const newRange =
-        sortOrder === LogsSortOrder.Descending
-          ? getPrevRange(getVisibleRange(rows), range)
-          : getNextRange(getVisibleRange(rows), range, timeZone);
       loadMoreLogs?.(newRange);
       setLowerLoading(true);
       reportInteraction('grafana_logs_infinite_scrolling', {
@@ -160,9 +176,8 @@ function shouldLoadMore(event: Event | WheelEvent, element: HTMLDivElement, last
     scrollDirection === ScrollDirection.Top
       ? element.scrollTop
       : element.scrollHeight - element.scrollTop - element.clientHeight;
-  const coef = 1;
 
-  return diff <= coef ? scrollDirection : ScrollDirection.NoScroll;
+  return diff <= 1 ? scrollDirection : ScrollDirection.NoScroll;
 }
 
 function getVisibleRange(rows: LogRowModel[]) {
@@ -195,13 +210,16 @@ function canScrollTop(
   currentRange: TimeRange,
   timeZone: TimeZone,
   sortOrder: LogsSortOrder
-) {
+): AbsoluteTimeRange | undefined {
   if (sortOrder === LogsSortOrder.Descending) {
     // When requesting new logs, update the current range if using relative time ranges.
     currentRange = updateCurrentRange(currentRange, timeZone);
-    return currentRange.to.valueOf() - visibleRange.to > SCROLLING_THRESHOLD;
+    const canScroll = currentRange.to.valueOf() - visibleRange.to > SCROLLING_THRESHOLD;
+    return canScroll ? getNextRange(visibleRange, currentRange, timeZone) : undefined;
   }
-  return Math.abs(currentRange.from.valueOf() - visibleRange.from) > SCROLLING_THRESHOLD;
+
+  const canScroll = Math.abs(currentRange.from.valueOf() - visibleRange.from) > SCROLLING_THRESHOLD;
+  return canScroll ? getPrevRange(visibleRange, currentRange) : undefined;
 }
 
 function canScrollBottom(
@@ -209,13 +227,15 @@ function canScrollBottom(
   currentRange: TimeRange,
   timeZone: TimeZone,
   sortOrder: LogsSortOrder
-) {
+): AbsoluteTimeRange | undefined {
   if (sortOrder === LogsSortOrder.Descending) {
-    return Math.abs(currentRange.from.valueOf() - visibleRange.from) > SCROLLING_THRESHOLD;
+    const canScroll = Math.abs(currentRange.from.valueOf() - visibleRange.from) > SCROLLING_THRESHOLD;
+    return canScroll ? getPrevRange(visibleRange, currentRange) : undefined;
   }
   // When requesting new logs, update the current range if using relative time ranges.
   currentRange = updateCurrentRange(currentRange, timeZone);
-  return currentRange.to.valueOf() - visibleRange.to > SCROLLING_THRESHOLD;
+  const canScroll = currentRange.to.valueOf() - visibleRange.to > SCROLLING_THRESHOLD;
+  return canScroll ? getNextRange(visibleRange, currentRange, timeZone) : undefined;
 }
 
 // Given a TimeRange, returns a new instance if using relative time, or else the same.