From a3261946e17efaa08ead368118c8307a6d9a1f62 Mon Sep 17 00:00:00 2001 From: Radoslaw Szwajkowski Date: Mon, 5 Aug 2024 20:34:54 +0200 Subject: [PATCH 1/4] Use InfiniteScroller pattern for the Task Drawer Implement InfiniteScroller based on oVirt VmPortal implementation and use it to fetch additional tasks in the Task Drawer. Key features: 1. fetch data using useInfiniteQuery from React Query 2. track sentinel visibility using IntersectionObserver API 3. monitor item count to prevent extra fetchMore calls Reference-Url: https://github.com/oVirt/ovirt-web-ui/blob/dfe0c4b8c92638f6e41b9fe0b09e0d07509618ae/src/components/VmsList/index.js#L50 Signed-off-by: Radoslaw Szwajkowski --- client/public/locales/en/translation.json | 1 + .../InfiniteScroller/InfiniteScroller.css | 6 + .../InfiniteScroller/InfiniteScroller.tsx | 70 ++++++++ .../app/components/InfiniteScroller/index.ts | 1 + .../InfiniteScroller/useVisibilityTracker.tsx | 43 +++++ .../task-manager/TaskManagerDrawer.tsx | 158 ++++++++++-------- client/src/app/queries/tasks.ts | 48 +++++- 7 files changed, 259 insertions(+), 68 deletions(-) create mode 100644 client/src/app/components/InfiniteScroller/InfiniteScroller.css create mode 100644 client/src/app/components/InfiniteScroller/InfiniteScroller.tsx create mode 100644 client/src/app/components/InfiniteScroller/index.ts create mode 100644 client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx diff --git a/client/public/locales/en/translation.json b/client/public/locales/en/translation.json index c9d16c6362..6942e06e7d 100644 --- a/client/public/locales/en/translation.json +++ b/client/public/locales/en/translation.json @@ -200,6 +200,7 @@ "issuesEffortTooltip": "This column shows the effort weight for a single issue incident.", "dependentQuestionTooltip": "This question is conditionally included or excluded based on tags:", "jiraInstanceNotConnected": "Jira instance {{name}} is not connected.", + "loadingTripleDot": "Loading...", "manageDependenciesInstructions": "Add northbound and southbound dependencies for the selected application here. Note that any selections made will be saved automatically. To undo any changes, you must manually delete the applications from the dropdowns.", "noDataAvailableBody": "No data available to be shown here.", "noDataAvailableTitle": "No data available", diff --git a/client/src/app/components/InfiniteScroller/InfiniteScroller.css b/client/src/app/components/InfiniteScroller/InfiniteScroller.css new file mode 100644 index 0000000000..6d8316199a --- /dev/null +++ b/client/src/app/components/InfiniteScroller/InfiniteScroller.css @@ -0,0 +1,6 @@ +.infinite-scroll-sentinel { + width: 100%; + text-align: center; + font-style: italic; + font-weight: bold; +} diff --git a/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx b/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx new file mode 100644 index 0000000000..9578b536bb --- /dev/null +++ b/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx @@ -0,0 +1,70 @@ +import React, { ReactNode, useEffect, useRef } from "react"; +import { useTranslation } from "react-i18next"; +import { useVisibilityTracker } from "./useVisibilityTracker"; +import "./InfiniteScroller.css"; + +export interface InfiniteScrollerProps { + // content to display + children: ReactNode; + // function triggered if sentinel node is visible to load more items + // returns false if call was rejected by the scheduler + fetchMore: () => boolean; + hasMore: boolean; + // number of items currently displayed/known + itemCount: number; +} + +let fetchRefHolder: any = null; + +export const InfiniteScroller = ({ + children, + fetchMore, + hasMore, + itemCount, +}: 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 { visible: isSentinelVisible, nodeRef: sentinelRef } = + useVisibilityTracker({ + enable: hasMore, + }); + + console.log("infinite props ", hasMore, itemCount, itemCountRef.current); + useEffect( + () => { + console.log( + `infinite [visible= >${isSentinelVisible}<] `, + itemCount, + itemCountRef.current, + fetchRefHolder === fetchMore + ); + fetchRefHolder = fetchMore; + 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; + } + }, + // reference to fetchMore() changes based on query state and ensures that the effect is triggered + [isSentinelVisible, fetchMore, itemCount] + ); + + return ( +
+ {children} + {hasMore && ( +
+ {t("message.loadingTripleDot")} +
+ )} +
+ ); +}; diff --git a/client/src/app/components/InfiniteScroller/index.ts b/client/src/app/components/InfiniteScroller/index.ts new file mode 100644 index 0000000000..762e8fb641 --- /dev/null +++ b/client/src/app/components/InfiniteScroller/index.ts @@ -0,0 +1 @@ +export * from "./InfiniteScroller"; diff --git a/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx b/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx new file mode 100644 index 0000000000..7e5b2bde6f --- /dev/null +++ b/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx @@ -0,0 +1,43 @@ +import { useEffect, useRef, useState } from "react"; + +export function useVisibilityTracker({ enable }: { enable: boolean }) { + const nodeRef = useRef(null); + const [visible, setVisible] = useState(false); + const node = nodeRef.current; + + useEffect(() => { + if (!enable || !node) { + console.log("useVisibilityTracker - disabled"); + 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 withing 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[]) => + entries.forEach(({ isIntersecting, ...rest }) => { + if (isIntersecting) { + setVisible(true); + console.log("useVisibilityTracker - intersection", rest); + } else { + setVisible(false); + console.log("useVisibilityTracker - out-of-box", rest); + } + }) + ); + observer.observe(node); + + console.log("useVisibilityTracker - observe"); + + return () => { + observer.disconnect(); + setVisible(false); + console.log("useVisibilityTracker - disconnect"); + }; + }, [enable, node]); + + return { visible, nodeRef }; +} diff --git a/client/src/app/components/task-manager/TaskManagerDrawer.tsx b/client/src/app/components/task-manager/TaskManagerDrawer.tsx index 56867b8038..f4489fe283 100644 --- a/client/src/app/components/task-manager/TaskManagerDrawer.tsx +++ b/client/src/app/components/task-manager/TaskManagerDrawer.tsx @@ -1,17 +1,10 @@ -import React, { forwardRef, useMemo, useState } from "react"; +import React, { forwardRef, useCallback, useMemo, useState } from "react"; import { Link } from "react-router-dom"; import dayjs from "dayjs"; import { Dropdown, DropdownItem, DropdownList, - EmptyState, - EmptyStateActions, - EmptyStateBody, - EmptyStateFooter, - EmptyStateHeader, - EmptyStateIcon, - EmptyStateVariant, MenuToggle, MenuToggleElement, NotificationDrawer, @@ -22,17 +15,25 @@ import { NotificationDrawerListItemBody, NotificationDrawerListItemHeader, Tooltip, + EmptyState, + EmptyStateHeader, + EmptyStateIcon, + EmptyStateBody, + EmptyStateVariant, + EmptyStateFooter, + EmptyStateActions, } from "@patternfly/react-core"; -import { CubesIcon, EllipsisVIcon } from "@patternfly/react-icons"; +import { EllipsisVIcon, CubesIcon } from "@patternfly/react-icons"; import { css } from "@patternfly/react-styles"; import { Task, TaskState } from "@app/api/models"; import { useTaskManagerContext } from "./TaskManagerContext"; -import { useServerTasks } from "@app/queries/tasks"; +import { useInfiniteServerTasks } from "@app/queries/tasks"; import "./TaskManagerDrawer.css"; import { TaskStateIcon } from "../Icons"; import { useTaskActions } from "@app/pages/tasks/useTaskActions"; +import { InfiniteScroller } from "../InfiniteScroller"; /** A version of `Task` specific for the task manager drawer components */ interface TaskManagerTask { @@ -58,7 +59,7 @@ interface TaskManagerTask { _: Task; } -const PAGE_SIZE = 20; +const PAGE_SIZE = 2; interface TaskManagerDrawerProps { ref?: React.ForwardedRef; @@ -67,7 +68,8 @@ interface TaskManagerDrawerProps { export const TaskManagerDrawer: React.FC = forwardRef( (_props, ref) => { const { isExpanded, setIsExpanded, queuedCount } = useTaskManagerContext(); - const { tasks } = useTaskManagerData(); + const { tasks, hasNextPage, fetchNextPage, isReadyToFetch } = + useTaskManagerData(); const [expandedItems, setExpandedItems] = useState([]); const [taskWithExpandedActions, setTaskWithExpandedAction] = useState< @@ -79,6 +81,7 @@ export const TaskManagerDrawer: React.FC = forwardRef( setExpandedItems([]); }; + console.log("tasks", tasks?.length); return ( = forwardRef( ) : ( - - {tasks.map((task) => ( - { - setExpandedItems( - expand - ? [...expandedItems, task.id] - : expandedItems.filter((i) => i !== task.id) - ); - }} - actionsExpanded={task.id === taskWithExpandedActions} - onActionsExpandToggle={(flag: boolean) => - setTaskWithExpandedAction(flag && task.id) - } - /> - ))} - + + + {tasks.map((task) => ( + { + setExpandedItems( + expand + ? [...expandedItems, task.id] + : expandedItems.filter((i) => i !== task.id) + ); + }} + actionsExpanded={task.id === taskWithExpandedActions} + onActionsExpandToggle={(flag: boolean) => + setTaskWithExpandedAction(flag && task.id) + } + /> + ))} + + )} @@ -228,12 +237,13 @@ const TaskItem: React.FC<{ }; const useTaskManagerData = () => { - const [pageSize, setPageSize] = useState(PAGE_SIZE); - const increasePageSize = () => { - setPageSize(pageSize + PAGE_SIZE); - }; - - const { result, isFetching } = useServerTasks( + const { + data, + fetchNextPage, + hasNextPage = false, + isFetching, + isFetchingNextPage, + } = useInfiniteServerTasks( { filters: [{ field: "state", operator: "=", value: "queued" }], sort: { @@ -242,7 +252,7 @@ const useTaskManagerData = () => { }, page: { pageNumber: 1, - itemsPerPage: pageSize, + itemsPerPage: PAGE_SIZE, }, }, 5000 @@ -250,40 +260,54 @@ const useTaskManagerData = () => { const tasks: TaskManagerTask[] = useMemo( () => - !result.data - ? [] - : result.data.map( - (task) => - ({ - id: task.id ?? -1, - createUser: task.createUser ?? "", - updateUser: task.updateUser ?? "", - createTime: task.createTime ?? "", - started: task.started ?? "", - terminated: task.terminated ?? "", - name: task.name, - kind: task.kind, - addon: task.addon, - extensions: task.extensions, - state: task.state ?? "", - priority: task.priority ?? 0, - applicationId: task.application.id, - applicationName: task.application.name, - preemptEnabled: task?.policy?.preemptEnabled ?? false, + data?.pages + ?.flatMap((data) => data?.data ?? []) + ?.map( + (task) => + ({ + id: task.id ?? -1, + createUser: task.createUser ?? "", + updateUser: task.updateUser ?? "", + createTime: task.createTime ?? "", + started: task.started ?? "", + terminated: task.terminated ?? "", + name: task.name, + kind: task.kind, + addon: task.addon, + extensions: task.extensions, + state: task.state ?? "", + priority: task.priority ?? 0, + applicationId: task.application.id, + applicationName: task.application.name, + preemptEnabled: task?.policy?.preemptEnabled ?? false, - _: task, + _: task, - // TODO: Add any checks that could be needed later... - // - isCancelable (does the current user own the task? other things to check?) - // - isPreemptionToggleAllowed - }) as TaskManagerTask - ), - [result.data] + // TODO: Add any checks that could be needed later... + // - isCancelable (does the current user own the task? other things to check?) + // - isPreemptionToggleAllowed + }) as TaskManagerTask + ) ?? [], + [data] ); + const fetchMore = useCallback(() => { + // forced fetch is not allowed when background fetch or other forced fetch is in progress + if (!isFetching && !isFetchingNextPage) { + fetchNextPage(); + console.log("fetchMore - started"); + return true; + } else { + console.log("fetchMore - blocked"); + return false; + } + }, [isFetching, isFetchingNextPage, fetchNextPage]); + return { tasks, - increasePageSize, isFetching, + hasNextPage, + fetchNextPage: fetchMore, + isReadyToFetch: !isFetching && !isFetchingNextPage, }; }; diff --git a/client/src/app/queries/tasks.ts b/client/src/app/queries/tasks.ts index 6c9818dbf3..81b70ef4d1 100644 --- a/client/src/app/queries/tasks.ts +++ b/client/src/app/queries/tasks.ts @@ -1,4 +1,9 @@ -import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { + useInfiniteQuery, + useMutation, + useQuery, + useQueryClient, +} from "@tanstack/react-query"; import { cancelTask, @@ -84,6 +89,47 @@ export const useFetchTaskDashboard = (refetchDisabled: boolean = false) => { }; }; +export const useInfiniteServerTasks = ( + initialParams: HubRequestParams, + refetchInterval?: number +) => { + return useInfiniteQuery({ + // usually the params are part of the key + // infinite query tracks the actual params for all pages under one key + // eslint-disable-next-line @tanstack/query/exhaustive-deps + queryKey: [TasksQueryKey], + queryFn: async ({ pageParam = initialParams }) => + await getServerTasks(pageParam), + getNextPageParam: (lastPage) => { + const pageNumber = lastPage?.params.page?.pageNumber ?? 0; + const itemsPerPage = lastPage?.params.page?.itemsPerPage ?? 20; + const total = lastPage?.total ?? 0; + if (total <= pageNumber * itemsPerPage) { + return undefined; + } + + return { + ...lastPage.params, + page: { + pageNumber: pageNumber + 1, + itemsPerPage, + }, + }; + }, + select: (infiniteData) => { + infiniteData?.pages?.forEach((page) => { + if (page.data?.length > 0) { + page.data = page.data.map(calculateSyntheticTaskState); + } + }); + return infiniteData; + }, + onError: (error: Error) => console.log("error, ", error), + keepPreviousData: true, + refetchInterval: refetchInterval ?? false, + }); +}; + export const useServerTasks = ( params: HubRequestParams, refetchInterval?: number From 681394d31ed55d093630e0d67019aa04f14ff2cf Mon Sep 17 00:00:00 2001 From: Radoslaw Szwajkowski Date: Thu, 5 Sep 2024 15:14:55 +0200 Subject: [PATCH 2/4] Update client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx Co-authored-by: Scott Dickerson Signed-off-by: Radoslaw Szwajkowski --- .../InfiniteScroller/useVisibilityTracker.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx b/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx index 7e5b2bde6f..9535b929e6 100644 --- a/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx +++ b/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx @@ -39,5 +39,15 @@ export function useVisibilityTracker({ enable }: { enable: boolean }) { }; }, [enable, node]); - return { visible, nodeRef }; + return { + /** + * Is the node referenced via `nodeRef` currently visible on the page? + */ + visible, + /** + * A ref to a node whose visibility will be tracked. This should be set as a ref to a + * relevant dom element by the component using this hook. + */ + nodeRef, + }; } From a7af8229aed6c7aaf3c44f51ff74e8697a8af4ba Mon Sep 17 00:00:00 2001 From: Radoslaw Szwajkowski Date: Thu, 5 Sep 2024 18:24:42 +0200 Subject: [PATCH 3/4] Remove extra logs, add setVisibleSafe wrapper for extra safety Signed-off-by: Radoslaw Szwajkowski --- .../InfiniteScroller/InfiniteScroller.tsx | 13 ++----- .../InfiniteScroller/useVisibilityTracker.tsx | 36 ++++++++++++------- .../task-manager/TaskManagerDrawer.tsx | 6 +--- 3 files changed, 26 insertions(+), 29 deletions(-) diff --git a/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx b/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx index 9578b536bb..9e4ddfbc69 100644 --- a/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx +++ b/client/src/app/components/InfiniteScroller/InfiniteScroller.tsx @@ -14,8 +14,6 @@ export interface InfiniteScrollerProps { itemCount: number; } -let fetchRefHolder: any = null; - export const InfiniteScroller = ({ children, fetchMore, @@ -34,16 +32,8 @@ export const InfiniteScroller = ({ enable: hasMore, }); - console.log("infinite props ", hasMore, itemCount, itemCountRef.current); useEffect( () => { - console.log( - `infinite [visible= >${isSentinelVisible}<] `, - itemCount, - itemCountRef.current, - fetchRefHolder === fetchMore - ); - fetchRefHolder = fetchMore; if ( isSentinelVisible && itemCountRef.current !== itemCount && @@ -53,7 +43,8 @@ export const InfiniteScroller = ({ itemCountRef.current = itemCount; } }, - // reference to fetchMore() changes based on query state and ensures that the effect is triggered + // 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] ); diff --git a/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx b/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx index 9535b929e6..c6f8135b24 100644 --- a/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx +++ b/client/src/app/components/InfiniteScroller/useVisibilityTracker.tsx @@ -1,43 +1,53 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState, useCallback } from "react"; export function useVisibilityTracker({ enable }: { enable: boolean }) { const nodeRef = useRef(null); const [visible, setVisible] = useState(false); const node = nodeRef.current; + // state is set from IntersectionObserver callbacks which may not align with React lifecycle + // we can add extra safety by using the same approach as Console's useSafetyFirst() hook + // https://github.com/openshift/console/blob/9d4a9b0a01b2de64b308f8423a325f1fae5f8726/frontend/packages/console-dynamic-plugin-sdk/src/app/components/safety-first.tsx#L10 + const mounted = useRef(true); + useEffect( + () => () => { + mounted.current = false; + }, + [] + ); + const setVisibleSafe = useCallback((newValue) => { + if (mounted.current) { + setVisible(newValue); + } + }, []); + useEffect(() => { if (!enable || !node) { - console.log("useVisibilityTracker - disabled"); 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 withing the (scrollable!)parent + // 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[]) => - entries.forEach(({ isIntersecting, ...rest }) => { + entries.forEach(({ isIntersecting }) => { if (isIntersecting) { - setVisible(true); - console.log("useVisibilityTracker - intersection", rest); + setVisibleSafe(true); } else { - setVisible(false); - console.log("useVisibilityTracker - out-of-box", rest); + setVisibleSafe(false); } }) ); observer.observe(node); - console.log("useVisibilityTracker - observe"); - return () => { observer.disconnect(); - setVisible(false); - console.log("useVisibilityTracker - disconnect"); + setVisibleSafe(false); }; - }, [enable, node]); + }, [enable, node, setVisibleSafe]); return { /** diff --git a/client/src/app/components/task-manager/TaskManagerDrawer.tsx b/client/src/app/components/task-manager/TaskManagerDrawer.tsx index 1a5fe37249..ccbf6bdfad 100644 --- a/client/src/app/components/task-manager/TaskManagerDrawer.tsx +++ b/client/src/app/components/task-manager/TaskManagerDrawer.tsx @@ -66,8 +66,7 @@ interface TaskManagerDrawerProps { export const TaskManagerDrawer: React.FC = forwardRef( (_props, ref) => { const { isExpanded, setIsExpanded, queuedCount } = useTaskManagerContext(); - const { tasks, hasNextPage, fetchNextPage, isReadyToFetch } = - useTaskManagerData(); + const { tasks, hasNextPage, fetchNextPage } = useTaskManagerData(); const [expandedItems, setExpandedItems] = useState([]); const [taskWithExpandedActions, setTaskWithExpandedAction] = useState< @@ -79,7 +78,6 @@ export const TaskManagerDrawer: React.FC = forwardRef( setExpandedItems([]); }; - console.log("tasks", tasks?.length); return ( { // forced fetch is not allowed when background fetch or other forced fetch is in progress if (!isFetching && !isFetchingNextPage) { fetchNextPage(); - console.log("fetchMore - started"); return true; } else { - console.log("fetchMore - blocked"); return false; } }, [isFetching, isFetchingNextPage, fetchNextPage]); From d54892f3e33e84c45a11643750c5e8625c69430a Mon Sep 17 00:00:00 2001 From: Radoslaw Szwajkowski Date: Thu, 5 Sep 2024 18:47:15 +0200 Subject: [PATCH 4/4] Change page saize to 20 Signed-off-by: Radoslaw Szwajkowski --- client/src/app/components/task-manager/TaskManagerDrawer.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/app/components/task-manager/TaskManagerDrawer.tsx b/client/src/app/components/task-manager/TaskManagerDrawer.tsx index ccbf6bdfad..4a1faf69fd 100644 --- a/client/src/app/components/task-manager/TaskManagerDrawer.tsx +++ b/client/src/app/components/task-manager/TaskManagerDrawer.tsx @@ -57,7 +57,7 @@ interface TaskManagerTask { _: Task; } -const PAGE_SIZE = 2; +const PAGE_SIZE = 20; interface TaskManagerDrawerProps { ref?: React.ForwardedRef;