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
Show file tree
Hide file tree
Changes from 4 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
99 changes: 53 additions & 46 deletions src/components/Kanban/Board.tsx
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
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;
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";
Expand All @@ -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";
Expand All @@ -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);

Expand All @@ -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();

Expand Down Expand Up @@ -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,
Expand All @@ -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({
Expand All @@ -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>

Expand Down
30 changes: 21 additions & 9 deletions src/components/Shifting/ShiftingBoard.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import careConfig from "@careConfig";
import { DropResult } from "@hello-pangea/dnd";
import { navigate } from "raviger";
import { Suspense, lazy, useState } from "react";
import { useTranslation } from "react-i18next";
Expand All @@ -14,7 +15,7 @@ import PageTitle from "@/components/Common/PageTitle";
import Tabs from "@/components/Common/Tabs";
import { ShiftingModel } 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/Shifting/ShiftingBadges";
import ShiftingBlock from "@/components/Shifting/ShiftingBlock";
import { formatFilter } from "@/components/Shifting/ShiftingCommons";
Expand All @@ -30,9 +31,15 @@ import {
import routes from "@/Utils/request/api";
import request from "@/Utils/request/request";

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

const KanbanBoard = lazyWithProps<KanbanBoardProps<ShiftingModel>>(
() => import("@/components/Kanban/Board"),
) as KanbanBoardType;
);

export default function BoardView() {
const { qParams, updateQuery, FilterBadges, advancedFilter } = useFilters({
Expand Down Expand Up @@ -133,7 +140,7 @@ export default function BoardView() {
</div>
</div>
<Suspense fallback={<Loading />}>
<KanbanBoard<ShiftingModel>
<KanbanBoard
title={<BadgesList {...{ qParams, FilterBadges }} />}
sections={boardFilter.map((board) => ({
id: board.text,
Expand All @@ -157,7 +164,7 @@ export default function BoardView() {
/>
</h3>
),
fetchOptions: (id) => ({
fetchOptions: (id: string) => ({
route: routes.listShiftRequests,
options: {
query: formatFilter({
Expand All @@ -167,13 +174,18 @@ export default function BoardView() {
},
}),
}))}
onDragEnd={(result) => {
if (result.source.droppableId !== result.destination?.droppableId)
onDragEnd={(result: DropResult) => {
// Ensure destination is not null
if (
result.destination &&
result.source.droppableId !== result.destination.droppableId
) {
navigate(
`/shifting/${result.draggableId}/update?status=${result.destination?.droppableId}`,
`/shifting/${result.draggableId}/update?status=${result.destination.droppableId}`,
);
}
}}
itemRender={(shift) => (
itemRender={(shift: ShiftingModel) => (
<ShiftingBlock
onTransfer={() => setModalFor(shift)}
shift={shift}
Expand Down
29 changes: 29 additions & 0 deletions src/hooks/useDebounce.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { useEffect, useRef } from "react";

export default function useDebounce<T extends any[]>(
callback: (...args: T) => void,
delay: number,
) {
const callbackRef = useRef(callback);
const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

useEffect(() => {
callbackRef.current = callback;
}, [callback]);

useEffect(() => {
return () => {
if (timeoutRef.current) clearTimeout(timeoutRef.current);
};
}, []);

const debouncedCallback = (...args: T) => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}
timeoutRef.current = setTimeout(() => {
callbackRef.current(...args);
}, delay);
};
return debouncedCallback;
}
Loading