Skip to content

Commit

Permalink
🐛 Fix fetching in InfiniteScroller (konveyor#2085)
Browse files Browse the repository at this point in the history
Fixes:
1. initial delay in first fetch - root cause was delayed initialization
of sentinel reference

2. extra page fetch request - algorithm based on items count was not
sufficient
 
Reference-Url:  konveyor#2049

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>
  • Loading branch information
rszwajko and sjd78 authored Sep 14, 2024
1 parent 7768bf6 commit 0d87a57
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 26 deletions.
41 changes: 20 additions & 21 deletions client/src/app/components/InfiniteScroller/InfiniteScroller.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { ReactNode, useEffect, useRef } from "react";
import React, { ReactNode, useEffect, useState } from "react";
import { useTranslation } from "react-i18next";
import { useVisibilityTracker } from "./useVisibilityTracker";
import "./InfiniteScroller.css";
Expand All @@ -12,47 +12,46 @@ export interface InfiniteScrollerProps {
hasMore: boolean;
// number of items currently displayed/known
itemCount: number;
pageSize: number;
}

export const InfiniteScroller = ({
children,
fetchMore,
hasMore,
itemCount,
pageSize,
}: InfiniteScrollerProps) => {
const { t } = useTranslation();
// Track how many items were known at time of triggering the fetch.
// This allows to detect edge case when second(or more) fetchMore() is triggered before
// IntersectionObserver is able to detect out-of-view event.
// Initializing with zero ensures that the effect will be triggered immediately
// (parent is expected to display empty state until some items are available).
const itemCountRef = useRef(0);
const [readyForFetch, setReadyForFetch] = useState(false);
const { visible: isSentinelVisible, nodeRef: sentinelRef } =
useVisibilityTracker({
enable: hasMore,
});
useEffect(() => {
// enable or clear the flag depending on the sentinel visibility
setReadyForFetch(!!isSentinelVisible);
}, [isSentinelVisible]);

useEffect(
() => {
if (
isSentinelVisible &&
itemCountRef.current !== itemCount &&
fetchMore() // fetch may be blocked if background refresh is in progress (or other manual fetch)
) {
// fetchMore call was triggered (it may fail but will be subject to React Query retry policy)
itemCountRef.current = itemCount;
}
},
useEffect(() => {
if (readyForFetch) {
// clear the flag if fetch request is accepted
setReadyForFetch(!fetchMore());
}
// reference to fetchMore() changes based on query state and ensures that the effect is triggered in the right moment
// i.e. after fetch triggered by the previous fetchMore() call finished
[isSentinelVisible, fetchMore, itemCount]
);
}, [fetchMore, readyForFetch]);

return (
<div>
{children}
{hasMore && (
<div ref={sentinelRef} className="infinite-scroll-sentinel">
<div
ref={sentinelRef}
// re-create the node with every page change to force new Intersection observer
key={Math.ceil(itemCount / pageSize)}
className="infinite-scroll-sentinel"
>
{t("message.loadingTripleDot")}
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useEffect, useRef, useState, useCallback } from "react";

export function useVisibilityTracker({ enable }: { enable: boolean }) {
const nodeRef = useRef<HTMLDivElement>(null);
const [visible, setVisible] = useState(false);
const [visible, setVisible] = useState<boolean | undefined>(false);
const node = nodeRef.current;

// state is set from IntersectionObserver callbacks which may not align with React lifecycle
Expand All @@ -22,14 +22,19 @@ export function useVisibilityTracker({ enable }: { enable: boolean }) {
}, []);

useEffect(() => {
if (enable && !node) {
// use falsy value different than initial value - state change will trigger render()
// otherwise we need to wait for the next render() to read node ref
setVisibleSafe(undefined);
return undefined;
}

if (!enable || !node) {
return undefined;
}

// Observer with default options - the whole view port used.
// Note that if root element is used then it needs to be the ancestor of the target.
// In case of infinite scroller the target is always within the (scrollable!)parent
// even if the node is technically hidden from the user.
// https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API#root
const observer = new IntersectionObserver(
(entries: IntersectionObserverEntry[]) =>
Expand Down
7 changes: 5 additions & 2 deletions client/src/app/components/task-manager/TaskManagerDrawer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ interface TaskManagerDrawerProps {
export const TaskManagerDrawer: React.FC<TaskManagerDrawerProps> = forwardRef(
(_props, ref) => {
const { isExpanded, setIsExpanded, queuedCount } = useTaskManagerContext();
const { tasks, hasNextPage, fetchNextPage } = useTaskManagerData();
const { tasks, hasNextPage, fetchNextPage, pageSize } =
useTaskManagerData();

const [expandedItems, setExpandedItems] = useState<number[]>([]);
const [taskWithExpandedActions, setTaskWithExpandedAction] = useState<
Expand Down Expand Up @@ -106,6 +107,7 @@ export const TaskManagerDrawer: React.FC<TaskManagerDrawerProps> = forwardRef(
fetchMore={fetchNextPage}
hasMore={hasNextPage}
itemCount={tasks?.length ?? 0}
pageSize={pageSize}
>
<NotificationDrawerList>
{tasks.map((task) => (
Expand Down Expand Up @@ -282,6 +284,7 @@ const useTaskManagerData = () => {
[data]
);

// note that the callback will change when query fetching state changes
const fetchMore = useCallback(() => {
// forced fetch is not allowed when background fetch or other forced fetch is in progress
if (!isFetching && !isFetchingNextPage) {
Expand All @@ -297,6 +300,6 @@ const useTaskManagerData = () => {
isFetching,
hasNextPage,
fetchNextPage: fetchMore,
isReadyToFetch: !isFetching && !isFetchingNextPage,
pageSize: PAGE_SIZE,
};
};

0 comments on commit 0d87a57

Please sign in to comment.