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

Closed
wants to merge 9 commits into from
Closed

Resolve infinite scroll issue in Resource page #9267

Show file tree
Hide file tree
Changes from 2 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
112 changes: 109 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
"qrcode.react": "^4.1.0",
"raviger": "^4.1.2",
"react": "18.3.1",
"react-beautiful-dnd": "^13.1.1",
rithviknishad marked this conversation as resolved.
Show resolved Hide resolved
"react-copy-to-clipboard": "^5.1.0",
"react-dom": "18.3.1",
"react-google-recaptcha": "^3.1.0",
Expand All @@ -111,6 +112,7 @@
"@types/lodash-es": "^4.17.12",
"@types/node": "^22.9.0",
"@types/react": "^18.3.12",
"@types/react-beautiful-dnd": "^13.1.8",
rithviknishad marked this conversation as resolved.
Show resolved Hide resolved
"@types/react-copy-to-clipboard": "^5.0.7",
"@types/react-csv": "^1.1.10",
"@types/react-dom": "^18.3.1",
Expand Down
99 changes: 53 additions & 46 deletions src/components/Kanban/Board.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Requested twice:

  1. Resolve infinite scroll issue in Resource page #9267 (review)
  2. Resolve infinite scroll issue in Resource page #9267 (comment)

Still I don't see useInfiniteQuery from tanstack-query being used in this file.

Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import { useTranslation } from "react-i18next";

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

import useDebounce from "@/hooks/useDebounce";

import request from "@/Utils/request/request";
import { QueryRoute } from "@/Utils/request/types";
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 @@ -30,6 +32,8 @@ interface KanbanBoardProps<T extends { id: string }> {
itemRender: (item: T) => ReactNode;
}

export type KanbanBoardType<T extends { id: string }> = KanbanBoardProps<T>;

Copy link
Member

Choose a reason for hiding this comment

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

if it's going to be exactly same as KanbanBoardProps, why not just re-use it?

Suggested change
export type KanbanBoardType<T extends { id: string }> = KanbanBoardProps<T>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it's going to be exactly same as KanbanBoardProps, why not just re-use it?

Thank you for the suggestion. You're absolutely right that KanbanBoardType is redundant since it's exactly the same as KanbanBoardProps. I'll remove KanbanBoardType and directly reuse KanbanBoardProps to keep the code clean and consistent.

export default function KanbanBoard<T extends { id: string }>(
props: KanbanBoardProps<T>,
) {
Expand Down Expand Up @@ -57,7 +61,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,7 +78,7 @@ 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>;
Expand All @@ -92,20 +96,25 @@ export function KanbanSection<T extends { id: string }>(
const defaultLimit = 14;
const { t } = useTranslation();

// should be replaced with useInfiniteQuery when we move over to react query

const fetchNextPage = async (refresh: boolean = false) => {
if (!refresh && (fetchingNextPage || !hasMore)) return;
if (refresh) setPages([]);
if (refresh) {
setPages([]);
setOffset(0);
}
const offsetToUse = refresh ? 0 : offset;
setFetchingNextPage(true);
const res = await request(options.route, {
...options.options,
query: { ...options.options?.query, offsetToUse, limit: defaultLimit },
query: {
...options.options?.query,
offset: offsetToUse,
limit: defaultLimit,
},
});
if (res.error) return;
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

Enhance error handling and type safety

While the refresh logic is good, there are two areas for improvement:

  1. Error case should reset fetchingNextPage state
  2. 'as any' type assertions could be replaced with proper types

Consider applying these changes:

-    if (res.error) return;
+    if (res.error) {
+      setFetchingNextPage(false);
+      return;
+    }
+    
+    interface PaginatedResponse {
+      results: T[];
+      next: string | null;
+      count: number;
+    }
+    
     const newPages = refresh ? [] : [...pages];
     const page = Math.floor(offsetToUse / defaultLimit);
-    newPages[page] = (res.data as any).results;
+    const data = res.data as PaginatedResponse;
+    newPages[page] = data.results;
-    setHasMore(!!(res.data as any)?.next);
-    setTotalCount((res.data as any)?.count);
+    setHasMore(!!data.next);
+    setTotalCount(data.count);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (refresh) {
setPages([]);
setOffset(0);
}
const offsetToUse = refresh ? 0 : offset;
setFetchingNextPage(true);
const res = await request(options.route, {
...options.options,
query: { ...options.options?.query, offsetToUse, limit: defaultLimit },
query: {
...options.options?.query,
offset: offsetToUse,
limit: defaultLimit,
},
});
if (res.error) return;
if (refresh) {
setPages([]);
setOffset(0);
}
const offsetToUse = refresh ? 0 : offset;
setFetchingNextPage(true);
const res = await request(options.route, {
...options.options,
query: {
...options.options?.query,
offset: offsetToUse,
limit: defaultLimit,
},
});
if (res.error) {
setFetchingNextPage(false);
return;
}
interface PaginatedResponse {
results: T[];
next: string | null;
count: number;
}
const newPages = refresh ? [] : [...pages];
const page = Math.floor(offsetToUse / defaultLimit);
const data = res.data as PaginatedResponse;
newPages[page] = data.results;
setHasMore(!!data.next);
setTotalCount(data.count);

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);
Expand All @@ -116,25 +125,29 @@ export function KanbanSection<T extends { id: string }>(

const items = pages.flat();

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();
}
};
const debouncedScroll = useDebounce(() => {
if (
!sectionRef.current ||
!props.boardRef.current ||
fetchingNextPage ||
!hasMore
)
return;

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

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

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

useEffect(() => {
fetchNextPage(true);
Expand All @@ -145,9 +158,7 @@ export function KanbanSection<T extends { id: string }>(
{(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">
Expand All @@ -165,22 +176,20 @@ export function KanbanSection<T extends { id: string }>(
{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>
))}
{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>
))}
{fetchingNextPage && (
<div className="mt-2 h-[300px] w-[284px] animate-pulse rounded-lg bg-secondary-300" />
)}
Expand All @@ -190,5 +199,3 @@ export function KanbanSection<T extends { id: string }>(
</Droppable>
);
}

export type KanbanBoardType = typeof KanbanBoard;
Loading
Loading