Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve infinite scroll issue in Resource page #9267

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
+184 −109
Open

Resolve infinite scroll issue in Resource page #9267

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
83e9154
Resolve infinite scroll issue in Resource page
NikhilA8606 Dec 2, 2024
97119a0
useDebounce hook implemented
NikhilA8606 Dec 5, 2024
4b3a557
removed react-beautiful-dnd
NikhilA8606 Dec 5, 2024
c8452e5
removed node
NikhilA8606 Dec 9, 2024
669bdc9
Removed KanbanBoardType<T>
NikhilA8606 Dec 10, 2024
d3b8102
useInfiniteQuery hook implemented
NikhilA8606 Dec 11, 2024
f9249dc
Initialize totalCount with Infinity
NikhilA8606 Dec 11, 2024
a780d43
Merge pull request #1 from ohcnetwork/develop
NikhilA8606 Dec 16, 2024
b9c0bc3
useDebounce removed
NikhilA8606 Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/Utils/request/useInfiniteQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { useCallback, useState } from "react";

import { RESULTS_PER_PAGE_LIMIT } from "@/common/constants";

import { PaginatedResponse, QueryRoute } from "@/Utils/request/types";
import useQuery, { QueryOptions } from "@/Utils/request/useQuery";

export interface InfiniteQueryOptions<TItem>
extends QueryOptions<PaginatedResponse<TItem>> {
deduplicateBy: (item: TItem) => string | number;
}

export function useInfiniteQuery<TItem>(
route: QueryRoute<PaginatedResponse<TItem>>,
options?: InfiniteQueryOptions<TItem>,
) {
const [items, setItems] = useState<TItem[]>([]);
const [totalCount, setTotalCount] = useState<number>(Infinity);
const [offset, setOffset] = useState(0);

const { refetch, loading, ...queryResponse } = useQuery(route, {
...options,
query: {
...(options?.query ?? {}),
offset,
},
onResponse: ({ data }) => {
Comment on lines +21 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential Overwrite of onResponse and query Options

By spreading ...options and then overriding query and onResponse, any query or onResponse properties provided in the options object will be overwritten. This could lead to unintended behavior if the caller supplies their own onResponse or query parameters.

Consider merging the query parameters and chaining the onResponse callbacks to preserve the functionality provided by the caller:

const { refetch, loading, ...queryResponse } = useQuery(route, {
  ...options,
  query: {
    ...(options?.query ?? {}),
    offset,
  },
- onResponse: ({ data }) => {
+ onResponse: (response) => {
+   options?.onResponse?.(response);
    if (!response.data) return;
-   if (!data) return;
-   const allItems = items.concat(data.results);
+   const allItems = items.concat(response.data.results);

    const deduplicatedItems = options?.deduplicateBy
      ? Array.from(
          allItems
            .reduce((map, item) => {
              const key = options.deduplicateBy(item);
              return map.set(key, item);
            }, new Map<string | number, TItem>())
            .values(),
        )
      : allItems;

    setItems(deduplicatedItems);
    setTotalCount(response.data.count);
  },
});

This ensures that any onResponse logic provided in options is still executed.

Committable suggestion skipped: line range outside the PR's diff.

if (!data) return;
const allItems = items.concat(data.results);

const deduplicatedItems = options?.deduplicateBy
? Array.from(
allItems
.reduce((map, item) => {
const key = options.deduplicateBy(item);
return map.set(key, item);
}, new Map<string | number, TItem>())
.values(),
)
: allItems;

setItems(deduplicatedItems);
setTotalCount(data.count);
},
});

const fetchNextPage = useCallback(() => {
if (loading) return;
setOffset((prevOffset) => prevOffset + RESULTS_PER_PAGE_LIMIT);
}, [loading]);
NikhilA8606 marked this conversation as resolved.
Show resolved Hide resolved

return {
items,
loading,
fetchNextPage,
refetch,
totalCount,
hasMore: totalCount ? items.length < totalCount : true,
...queryResponse,
};
}
142 changes: 45 additions & 97 deletions src/components/Kanban/Board.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@ import {
Droppable,
OnDragEndResponder,
} from "@hello-pangea/dnd";
import { ReactNode, RefObject, useEffect, useRef, useState } from "react";
import { useTranslation } from "react-i18next";
import { ReactNode, RefObject, useEffect, useRef } from "react";

import CareIcon from "@/CAREUI/icons/CareIcon";

import request from "@/Utils/request/request";
import { QueryRoute } from "@/Utils/request/types";
import useDebounce from "@/hooks/useDebounce";

import { PaginatedResponse, QueryRoute } from "@/Utils/request/types";
import { useInfiniteQuery } from "@/Utils/request/useInfiniteQuery";
import { QueryOptions } from "@/Utils/request/useQuery";

interface KanbanBoardProps<T extends { id: string }> {
export interface KanbanBoardProps<T extends { id: string }> {
title?: ReactNode;
onDragEnd: OnDragEndResponder<string>;
sections: {
Expand All @@ -23,7 +24,7 @@ interface KanbanBoardProps<T extends { id: string }> {
id: string,
...args: unknown[]
) => {
route: QueryRoute<unknown>;
route: QueryRoute<PaginatedResponse<T>>;
options?: QueryOptions<unknown>;
};
}[];
Expand Down Expand Up @@ -57,7 +58,7 @@ export default function KanbanBoard<T extends { id: string }>(
</div>
</div>
<DragDropContext onDragEnd={props.onDragEnd}>
<div className="h-full overflow-scroll" ref={board}>
<div className="h-full overflow-auto" ref={board}>
<div className="flex items-stretch px-0 pb-2">
{props.sections.map((section, i) => (
<KanbanSection<T>
Expand All @@ -74,114 +75,63 @@ export default function KanbanBoard<T extends { id: string }>(
);
}

export function KanbanSection<T extends { id: string }>(
function KanbanSection<T extends { id: string }>(
props: Omit<KanbanBoardProps<T>, "sections" | "onDragEnd"> & {
section: KanbanBoardProps<T>["sections"][number];
boardRef: RefObject<HTMLDivElement>;
},
) {
const { section } = props;
const [offset, setOffset] = useState(0);
const [pages, setPages] = useState<T[][]>([]);
const [fetchingNextPage, setFetchingNextPage] = useState(false);
const [hasMore, setHasMore] = useState(true);
const [totalCount, setTotalCount] = useState<number>();

const options = section.fetchOptions(section.id);
const sectionRef = useRef<HTMLDivElement>(null);
const defaultLimit = 14;
const { t } = useTranslation();
const { items, loading, fetchNextPage, hasMore } = useInfiniteQuery<T>(
section.fetchOptions(section.id).route,
{
...section.fetchOptions(section.id).options,
deduplicateBy: (item) => item.id,
},
);

// should be replaced with useInfiniteQuery when we move over to react query
const debouncedScroll = useDebounce(() => {
if (!props.boardRef.current || loading || !hasMore) return;

const fetchNextPage = async (refresh: boolean = false) => {
if (!refresh && (fetchingNextPage || !hasMore)) return;
if (refresh) setPages([]);
const offsetToUse = refresh ? 0 : offset;
setFetchingNextPage(true);
const res = await request(options.route, {
...options.options,
query: { ...options.options?.query, offsetToUse, limit: defaultLimit },
});
const newPages = refresh ? [] : [...pages];
const page = Math.floor(offsetToUse / defaultLimit);
if (res.error) return;
newPages[page] = (res.data as any).results;
setPages(newPages);
setHasMore(!!(res.data as any)?.next);
setTotalCount((res.data as any)?.count);
setOffset(offsetToUse + defaultLimit);
setFetchingNextPage(false);
};
const scrollTop = props.boardRef.current.scrollTop;
const visibleHeight = props.boardRef.current.offsetHeight;
const sectionHeight = props.boardRef.current.scrollHeight;

const items = pages.flat();
if (scrollTop + visibleHeight >= sectionHeight - 100) {
fetchNextPage();
}
}, 200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure Stability of debouncedScroll to Prevent Excessive Event Listeners

The debouncedScroll function may change on every render because useDebounce might return a new function reference each time. This causes the useEffect hook to remove and add the event listener on every render, which can degrade performance.

Modify the useDebounce hook to return a stable function reference across renders. You can achieve this by utilizing useRef internally within useDebounce:

// Inside useDebounce.ts
import { useCallback, useRef } from "react";

export default function useDebounce<T extends (...args: any[]) => void>(
  fn: T,
  delay: number,
  deps: any[] = []
): T {
  const timeoutRef = useRef<ReturnType<typeof setTimeout>>();
  const memoizedFn = useCallback(fn, deps);

  return useCallback(
    (...args: Parameters<T>) => {
      if (timeoutRef.current) clearTimeout(timeoutRef.current);
      timeoutRef.current = setTimeout(() => {
        memoizedFn(...args);
      }, delay);
    },
    [delay, memoizedFn]
  ) as T;
}

Then, update the usage in your component:

const debouncedScroll = useDebounce(
  () => {
    if (!props.boardRef.current || loading || !hasMore) return;

    const scrollTop = props.boardRef.current.scrollTop;
    const visibleHeight = props.boardRef.current.offsetHeight;
    const sectionHeight = props.boardRef.current.scrollHeight;

    if (scrollTop + visibleHeight >= sectionHeight - 100) {
      fetchNextPage();
    }
  },
  200,
+ [loading, hasMore]
);

This ensures that debouncedScroll remains stable unless loading or hasMore changes.


useEffect(() => {
const onBoardReachEnd = async () => {
const sectionElementHeight =
sectionRef.current?.getBoundingClientRect().height;
const scrolled = props.boardRef.current?.scrollTop;
// if user has scrolled 3/4th of the current items
if (
scrolled &&
sectionElementHeight &&
scrolled > sectionElementHeight * (3 / 4)
) {
fetchNextPage();
}
};

props.boardRef.current?.addEventListener("scroll", onBoardReachEnd);
props.boardRef.current?.addEventListener("scroll", debouncedScroll);
return () =>
props.boardRef.current?.removeEventListener("scroll", onBoardReachEnd);
}, [props.boardRef, fetchingNextPage, hasMore]);

useEffect(() => {
fetchNextPage(true);
}, [props.section]);
props.boardRef.current?.removeEventListener("scroll", debouncedScroll);
}, [loading, hasMore, debouncedScroll, props.boardRef]); // Add props.boardRef here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid Adding Multiple Event Listeners to the Same Element

Each KanbanSection adds an event listener to the same boardRef element. This results in multiple identical listeners, which is inefficient and can cause unexpected behavior.

Move the scroll event listener to the parent KanbanBoard component to ensure only one listener is attached:

  • In KanbanBoard.tsx, add the scroll listener:
+ const debouncedScroll = useDebounce(
+   () => {
+     // Logic to fetch next pages for all sections or manage state centrally
+   },
+   200,
+   [loading, hasMore]
+ );

useEffect(() => {
  board.current?.addEventListener("scroll", debouncedScroll);
  return () =>
    board.current?.removeEventListener("scroll", debouncedScroll);
}, [debouncedScroll]);
  • Remove the scroll logic from KanbanSection:
function KanbanSection<T extends { id: string }>(
  props: Omit<KanbanBoardProps<T>, "sections" | "onDragEnd"> & {
    section: KanbanBoardProps<T>["sections"][number];
-   boardRef: RefObject<HTMLDivElement>;
  },
) {
  // Remove debouncedScroll and related useEffect
}

This centralizes the scroll handling and prevents multiple listeners on the same element.

Committable suggestion skipped: line range outside the PR's diff.


return (
<Droppable droppableId={section.id}>
{(provided) => (
<div
ref={provided.innerRef}
className={
"relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200"
}
className="relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200"
>
<div className="sticky top-0 rounded-xl bg-secondary-200 pt-2">
<div className="mx-2 flex items-center justify-between rounded-lg border border-secondary-300 bg-white p-4">
<div>{section.title}</div>
<div>
<span className="ml-2 rounded-lg bg-secondary-300 px-2">
{typeof totalCount === "undefined" ? "..." : totalCount}
</span>
</div>
</div>
</div>
<div ref={sectionRef}>
{!fetchingNextPage && totalCount === 0 && (
<div className="flex items-center justify-center py-10 text-secondary-500">
{t("no_results_found")}
</div>
)}
{items
.filter((item) => item)
.map((item, i) => (
<Draggable draggableId={item.id} key={i} index={i}>
{(provided) => (
<div
ref={provided.innerRef}
{...provided.draggableProps}
{...provided.dragHandleProps}
className="mx-2 mt-2 w-[284px] rounded-lg border border-secondary-300 bg-white"
>
{props.itemRender(item)}
</div>
)}
</Draggable>
))}
{fetchingNextPage && (
<div ref={props.boardRef}>
{items.map((item, i) => (
<Draggable draggableId={item.id} key={i} index={i}>
{(provided) => (
<div
ref={provided.innerRef}
{...provided.draggableProps}
{...provided.dragHandleProps}
className="mx-2 mt-2 w-[284px] rounded-lg border border-secondary-300 bg-white"
>
{props.itemRender(item)}
</div>
)}
</Draggable>
))}
{loading && (
<div className="mt-2 h-[300px] w-[284px] animate-pulse rounded-lg bg-secondary-300" />
)}
</div>
Expand All @@ -190,5 +140,3 @@ export function KanbanSection<T extends { id: string }>(
</Droppable>
);
}

export type KanbanBoardType = typeof KanbanBoard;
27 changes: 18 additions & 9 deletions src/components/Resource/ResourceBoard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { navigate } from "raviger";
import { Suspense, lazy, useState } from "react";
import React, { Suspense, lazy, useState } from "react";
import { useTranslation } from "react-i18next";

import CareIcon from "@/CAREUI/icons/CareIcon";
Expand All @@ -13,7 +13,7 @@ import PageTitle from "@/components/Common/PageTitle";
import Tabs from "@/components/Common/Tabs";
import { ResourceModel } from "@/components/Facility/models";
import SearchInput from "@/components/Form/SearchInput";
import type { KanbanBoardType } from "@/components/Kanban/Board";
import type { KanbanBoardProps } from "@/components/Kanban/Board";
import BadgesList from "@/components/Resource/ResourceBadges";
import ResourceBlock from "@/components/Resource/ResourceBlock";
import { formatFilter } from "@/components/Resource/ResourceCommons";
Expand All @@ -26,9 +26,17 @@ import { RESOURCE_CHOICES } from "@/common/constants";
import routes from "@/Utils/request/api";
import request from "@/Utils/request/request";

const KanbanBoard = lazy(
// Helper function to type lazy-loaded components
function lazyWithProps<T>(
factory: () => Promise<{ default: React.ComponentType<T> }>,
) {
return lazy(factory) as React.LazyExoticComponent<React.ComponentType<T>>;
}
Comment on lines +30 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider moving the lazyWithProps helper to a shared utilities file.

The lazyWithProps helper function is duplicated in both board components. Since it's a generic utility function, it should be moved to a shared location.

Create a new file src/Utils/lazyLoad.ts:

import { lazy } from 'react';

export function lazyWithProps<T>(
  factory: () => Promise<{ default: React.ComponentType<T> }>,
) {
  return lazy(factory) as React.LazyExoticComponent<React.ComponentType<T>>;
}


// Correctly lazy-load KanbanBoard with its props
const KanbanBoard = lazyWithProps<KanbanBoardProps<ResourceModel>>(
() => import("@/components/Kanban/Board"),
) as KanbanBoardType;
);

const resourceStatusOptions = RESOURCE_CHOICES.map((obj) => obj.text);

Expand All @@ -40,8 +48,7 @@ export default function BoardView() {
limit: -1,
cacheBlacklist: ["title"],
});
const [boardFilter, setBoardFilter] = useState(ACTIVE);
// eslint-disable-next-line
const [boardFilter, setBoardFilter] = useState(resourceStatusOptions);
const appliedFilters = formatFilter(qParams);
const { t } = useTranslation();

Expand Down Expand Up @@ -104,7 +111,7 @@ export default function BoardView() {
</div>
</div>
<Suspense fallback={<Loading />}>
<KanbanBoard<ResourceModel>
<KanbanBoard
title={<BadgesList {...{ appliedFilters, FilterBadges }} />}
sections={boardFilter.map((board) => ({
id: board,
Expand All @@ -128,7 +135,7 @@ export default function BoardView() {
/>
</h3>
),
fetchOptions: (id) => ({
fetchOptions: (id: string) => ({
NikhilA8606 marked this conversation as resolved.
Show resolved Hide resolved
route: routes.listResourceRequests,
options: {
query: formatFilter({
Expand All @@ -144,7 +151,9 @@ export default function BoardView() {
`/resource/${result.draggableId}/update?status=${result.destination?.droppableId}`,
);
}}
itemRender={(resource) => <ResourceBlock resource={resource} />}
itemRender={(resource: ResourceModel) => (
<ResourceBlock resource={resource} />
)}
/>
</Suspense>

Expand Down
Loading
Loading