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.

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
Open
Changes from 6 commits
Commits
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
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>();
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: items.length < (totalCount ?? 0),
NikhilA8606 marked this conversation as resolved.
Show resolved Hide resolved
...queryResponse,
};
}
142 changes: 45 additions & 97 deletions src/components/Kanban/Board.tsx
Original file line number Diff line number Diff line change
@@ -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: {
@@ -23,7 +24,7 @@ interface KanbanBoardProps<T extends { id: string }> {
id: string,
...args: unknown[]
) => {
route: QueryRoute<unknown>;
route: QueryRoute<PaginatedResponse<T>>;
options?: QueryOptions<unknown>;
};
}[];
@@ -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>
@@ -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>
@@ -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
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";
@@ -12,7 +12,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";
@@ -25,9 +25,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);

@@ -39,8 +47,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();

@@ -103,7 +110,7 @@ export default function BoardView() {
</div>
</div>
<Suspense fallback={<Loading />}>
<KanbanBoard<ResourceModel>
<KanbanBoard
title={<BadgesList {...{ appliedFilters, FilterBadges }} />}
sections={boardFilter.map((board) => ({
id: board,
@@ -127,7 +134,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({
@@ -143,7 +150,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>

Loading