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 1 commit
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
89 changes: 52 additions & 37 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 @@ -57,7 +57,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 Down Expand Up @@ -92,20 +92,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 @@ -117,24 +122,30 @@ 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
const onBoardReachEnd = () => {
if (
scrolled &&
sectionElementHeight &&
scrolled > sectionElementHeight * (3 / 4)
) {
!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();
}
};

props.boardRef.current?.addEventListener("scroll", onBoardReachEnd);
const debouncedScroll = debounce(onBoardReachEnd, 200);

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]);

useEffect(() => {
fetchNextPage(true);
Expand All @@ -145,9 +156,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 +174,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 @@ -192,3 +199,11 @@ export function KanbanSection<T extends { id: string }>(
}

export type KanbanBoardType = typeof KanbanBoard;

function debounce(fn: () => void, delay: number) {
let timeout: NodeJS.Timeout | null = null;
return () => {
if (timeout) clearTimeout(timeout);
timeout = setTimeout(fn, delay);
};
}
NikhilA8606 marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"isolatedModules": true,
"noEmit": true,
"jsx": "react-jsx",
"types": ["vite/client", "vite-plugin-pwa/client"],
"types": ["vite/client", "vite-plugin-pwa/client","node"],
Copy link
Member

Choose a reason for hiding this comment

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

was this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate more? state the reason and what it solves? Just curious on what this does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to ensure TypeScript recognizes Node.js globals and core modules, preventing related errors.And it solves issues related to unrecognized Node.js globals (like process), core modules (like fs and path), and environment variables in TypeScript.
Example of the error which i saw are " Cannot find name 'process', __dirname, or require",
Cannot find module 'fs' or its corresponding type declarations etc

Copy link
Member

Choose a reason for hiding this comment

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

care_fe is strictly client-side based and doesn't / shouldn't be using any nodejs specific globals or modules directly.

cc: @sainak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out! I did use ChatGPT to explore a solution for the issue, which led to including "node" in the types array

Copy link
Contributor Author

@NikhilA8606 NikhilA8606 Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tried resolving the issue without including Node.js, but I encountered related errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats why i asked help for chatgpt

Copy link
Member

Choose a reason for hiding this comment

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

what error exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error occurred before I implemented the useDebounce.ts solution, which prompted me to include "node" in the types array as a temporary fix. After implementing the useDebounce logic, I overlooked removing "node" from the configuration. I’ve rechecked the issue now, and it’s working perfectly without needing Node.js. Apologies for the oversight, and thank you for pointing it out!

"baseUrl": ".",
"paths": {
"@/*": ["./src/*"],
Expand Down