Resolve infinite scroll issue in Resource page #9267
Conversation
WalkthroughThis pull request introduces a comprehensive update to the pagination and infinite scrolling mechanism across multiple components. The changes focus on implementing a new Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Kanban/Board.tsx (3)
97-100: Add error handling for refresh operationWhile the reset logic is correct, consider adding error handling to manage potential failures during refresh operations.
if (refresh) { + try { setPages([]); setOffset(0); + } catch (error) { + console.error('Failed to refresh kanban board:', error); + // Consider adding error state handling + } }
125-138: Consider making scroll threshold configurableThe scroll threshold of 100 pixels should be made configurable to accommodate different use cases and screen sizes.
+interface KanbanSectionProps<T> extends Omit<KanbanBoardProps<T>, "sections" | "onDragEnd"> { + section: KanbanBoardProps<T>["sections"][number]; + boardRef: RefObject<HTMLDivElement>; + scrollThreshold?: number; // New prop +} const onBoardReachEnd = () => { 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) { + const threshold = props.scrollThreshold ?? 100; + if (scrollTop + visibleHeight >= sectionHeight - threshold) { fetchNextPage(); } };
177-190: Consider adding item-level loading statesWhile there's a loading state for the entire section, consider adding loading states for individual items to improve user experience during drag operations.
{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" + className={`mx-2 mt-2 w-[284px] rounded-lg border border-secondary-300 bg-white ${ + item.isLoading ? 'opacity-50' : '' + }`} > {props.itemRender(item)} </div> )} </Draggable> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Kanban/Board.tsx(6 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (3)
tsconfig.json (1)
18-18: LGTM: Node types addition supports debounce implementation
The addition of Node.js types is necessary to support the NodeJS.Timeout type used in the debounce implementation.
src/components/Kanban/Board.tsx (2)
60-60: LGTM: Improved scrollbar behavior
The change from overflow-scroll to overflow-auto is an improvement as it shows scrollbars only when necessary, providing a better user experience.
143-147: LGTM: Effective scroll performance optimization
The implementation of debounced scroll handling effectively prevents excessive API calls during rapid scrolling, addressing the infinite scroll issue mentioned in the PR objectives.
|
Hey @NikhilA8606 I'm working on building |
|
Also useDebounce hook is being implemented by @SwanandBhuskute which can be reused here instead of implementing it. |
rithviknishad
added
waiting for related PR
tsconfig.json
Outdated
| @@ -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"], | |||
There was a problem hiding this comment.
Could you elaborate more? state the reason and what it solves? Just curious on what this does.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
care_fe is strictly client-side based and doesn't / shouldn't be using any nodejs specific globals or modules directly.
cc: @sainak
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I tried resolving the issue without including Node.js, but I encountered related errors
There was a problem hiding this comment.
Thats why i asked help for chatgpt
There was a problem hiding this comment.
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!
Hey, i walked through the pull requests of @SwanandBhuskute but i can't see this hook in there. |
|
The link attached takes right to the useDebounce hook implementation |
Yeah i got it |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/hooks/useDebounce.ts (1)
3-6: Consider strengthening type safetyWhile the generic type parameter works, it could be more restrictive to prevent potential misuse.
-export default function useDebounce<T extends any[]>( +export default function useDebounce<T extends unknown[]>( callback: (...args: T) => void, delay: number, )src/components/Resource/ResourceBoard.tsx (1)
28-33: Consider moving helper function to utilsThe
lazyWithPropshelper is a generic utility function that could be useful across the application.Consider moving it to a shared utilities file, e.g.,
src/Utils/lazy.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(2 hunks)src/components/Kanban/Board.tsx(8 hunks)src/components/Resource/ResourceBoard.tsx(7 hunks)src/components/Shifting/ShiftingBoard.tsx(6 hunks)src/hooks/useDebounce.ts(1 hunks)
🔇 Additional comments (9)
src/hooks/useDebounce.ts (1)
20-27: 🛠️ Refactor suggestion
Potential race condition with multiple rapid invocations
The current implementation might miss the latest arguments if the function is called multiple times rapidly, as it uses closure over the args parameter.
Consider testing the behavior with this script:
Consider storing the latest arguments in a ref to ensure the most recent call is processed:
const debouncedCallback = (...args: T) => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}
+ const argsRef = args; // Capture latest arguments
timeoutRef.current = setTimeout(() => {
- callbackRef.current(...args);
+ callbackRef.current(...argsRef);
}, delay);
};src/components/Resource/ResourceBoard.tsx (1)
Line range hint 113-155: Verify infinite scroll fix implementation
The PR's objective is to fix the infinite scroll issue, but the changes in this component don't directly address the scroll behavior. Ensure that the infinite scroll fix is properly implemented in the KanbanBoard component.
Let's verify the KanbanBoard implementation:
✅ Verification successful
Let me gather more information about the KanbanBoard component specifically.
Let me try to locate the KanbanBoard component with a different approach.
Let me check the Kanban Board implementation and its scroll-related functionality.
Infinite scroll is properly implemented in the KanbanBoard component
The KanbanBoard component has a robust infinite scroll implementation:
- Uses scroll event listener with debouncing to detect when user reaches near the bottom
- Implements pagination with offset and limit
- Handles loading states and displays loading placeholders
- Maintains separate page states for each section
- Properly cleans up event listeners
The implementation in src/components/Kanban/Board.tsx shows that the infinite scroll functionality is working as expected, with proper handling of:
- Scroll detection (
debouncedScrollfunction) - Page fetching (
fetchNextPagefunction) - Loading states (
fetchingNextPage) - Empty states
- Scroll cleanup
The review comment's concern about infinite scroll issues has been addressed in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check KanbanBoard component for scroll-related changes
ast-grep --pattern 'useInfiniteQuery($_)'
rg -A 10 "onScroll|InfiniteScroll|useInfinite"
Length of output: 4444
Script:
#!/bin/bash
# Check KanbanBoard component implementation
fd KanbanBoard.tsx --exec cat {}
rg -A 10 "fetchMore|useInfinite|onScroll" src/components/Common/KanbanBoard.tsx
Length of output: 196
Script:
#!/bin/bash
# Find KanbanBoard component location and its content
fd -e tsx -e ts -e js Kanban
rg -l "KanbanBoard"
Length of output: 166
Script:
#!/bin/bash
# Check the Kanban board implementation
cat src/components/Kanban/Board.tsx
# Look for infinite scroll or query-related implementations in the component
rg -A 10 "useQuery|useInfinite|onScroll|fetchMore" src/components/Kanban/Board.tsx
Length of output: 6901
src/components/Kanban/Board.tsx (4)
Line range hint 18-35: LGTM! Well-structured type definitions.
The exported interface and type alias improve reusability and type safety across the codebase.
64-64: Good choice using overflow-auto.
Using overflow-auto instead of overflow-scroll is more user-friendly as it only shows scrollbars when needed.
101-115: Improved pagination reliability and error handling.
Good improvements:
- Proper reset of pagination state on refresh
- Added error handling for API response
- Consistent query parameter naming
128-150: 🛠️ Refactor suggestion
Consider cleaning up the debounced function.
While the scroll handling implementation is good, consider cleaning up the debounced function to prevent potential memory leaks.
This was previously suggested in an earlier review. The recommendation to enhance the debounce utility and its cleanup remains valid.
src/components/Shifting/ShiftingBoard.tsx (3)
34-42: Well-implemented lazy loading with proper type safety.
The lazyWithProps utility function is a good abstraction that ensures type safety for lazy-loaded components.
177-186: Good defensive programming with proper type safety.
The null check for result.destination prevents potential runtime errors, and the type annotation improves code clarity.
Line range hint 167-192: LGTM! Improved type safety with explicit annotations.
The explicit type annotations in fetchOptions and itemRender props improve code clarity and help catch potential type-related errors early.
There was a problem hiding this comment.
Let's switch to using useInfiniteQuery hook as implemented in #9190 similar to how useDebounce was copied over. Do make sure changes its copied exactly as is to avoid merge conflicts.
src/components/Kanban/Board.tsx
Outdated
| export type KanbanBoardType<T extends { id: string }> = KanbanBoardProps<T>; | ||
|
|
There was a problem hiding this comment.
if it's going to be exactly same as KanbanBoardProps, why not just re-use it?
| export type KanbanBoardType<T extends { id: string }> = KanbanBoardProps<T>; |
There was a problem hiding this comment.
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.
rithviknishad
added
changes required
and removed
waiting for related PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Kanban/Board.tsx (2)
126-142: Consider parameterizing scroll thresholdThe scroll threshold of 100 pixels should be a named constant or configurable parameter for better maintainability.
Consider this change:
+ const SCROLL_THRESHOLD = 100; // pixels before bottom to trigger next page 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) { + if (scrollTop + visibleHeight >= sectionHeight - SCROLL_THRESHOLD) { fetchNextPage(); } }, 200);
144-148: Prevent potential memory leaksThe cleanup could be enhanced to handle in-flight requests when the component unmounts.
Consider adding an abort controller:
+ const abortController = useRef<AbortController>(); + useEffect(() => { + abortController.current = new AbortController(); props.boardRef.current?.addEventListener("scroll", debouncedScroll); - return () => + return () => { props.boardRef.current?.removeEventListener("scroll", debouncedScroll); + abortController.current?.abort(); + } }, [fetchingNextPage, hasMore, props.boardRef, debouncedScroll]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Kanban/Board.tsx(7 hunks)
🔇 Additional comments (3)
src/components/Kanban/Board.tsx (3)
12-18: LGTM: Clean interface export and hook import
The export of KanbanBoardProps interface and import of useDebounce hook are well-structured changes that support the infinite scroll fix.
177-190: LGTM: Clean rendering implementation
The rendering logic for draggable items is well-implemented with proper props handling and clean markup.
12-12: Verify useDebounce hook integration
Let's verify that the useDebounce hook is properly implemented and available.
✅ Verification successful
useDebounce hook is properly implemented and ready to use
The hook implementation in src/hooks/useDebounce.ts is correct and includes:
- Proper TypeScript generics for type safety
- Correct usage of useRef for callback and timeout references
- Proper cleanup in useEffect
- Memory leak prevention by clearing timeouts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify useDebounce hook implementation
# Check if the hook exists and is properly exported
rg -l "export.*function useDebounce|export.*const useDebounce" "src/hooks/"
# Check for any other usages that might need to be updated
rg "useDebounce" --type ts --type tsx
Length of output: 166
Script:
#!/bin/bash
# Let's verify with more precise commands
# Check the implementation of useDebounce
cat src/hooks/useDebounce.ts
# Search for usages with proper file type options
rg "useDebounce" -t ts -t typescript
Length of output: 817
src/components/Kanban/Board.tsx
Outdated
| 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; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety
While the refresh logic is good, there are two areas for improvement:
- Error case should reset fetchingNextPage state
- '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.
| 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); |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/components/Kanban/Board.tsx (1)
86-92: Optimize by CachingfetchOptionsResult
section.fetchOptions(section.id)is called twice, which can lead to unnecessary computations iffetchOptionsis an expensive operation.Store the result of
fetchOptionsin a variable to avoid redundant calls:+ const fetchOptions = section.fetchOptions(section.id); const { items, loading, fetchNextPage, hasMore } = useInfiniteQuery<T>( - section.fetchOptions(section.id).route, + fetchOptions.route, { - ...section.fetchOptions(section.id).options, + ...fetchOptions.options, deduplicateBy: (item) => item.id, }, );This improves performance by eliminating duplicate function calls.
| const { refetch, loading, ...queryResponse } = useQuery(route, { | ||
| ...options, | ||
| query: { | ||
| ...(options?.query ?? {}), | ||
| offset, | ||
| }, | ||
| onResponse: ({ data }) => { |
There was a problem hiding this comment.
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.
src/components/Kanban/Board.tsx
Outdated
| 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); |
There was a problem hiding this comment.
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.
src/components/Kanban/Board.tsx
Outdated
| 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); | ||
|
|
||
| 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 |
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Utils/request/useInfiniteQuery.ts (2)
8-11: Consider makingdeduplicateByoptional.The
deduplicateByfunction is currently mandatory inInfiniteQueryOptions. However, not all infinite queries require deduplication. Making it optional would provide more flexibility while maintaining type safety.export interface InfiniteQueryOptions<TItem> extends QueryOptions<PaginatedResponse<TItem>> { - deduplicateBy: (item: TItem) => string | number; + deduplicateBy?: (item: TItem) => string | number; }
31-40: Consider extracting the deduplication logic.The deduplication logic could be useful in other contexts. Consider extracting it into a reusable utility function.
// src/Utils/array.ts export function deduplicate<T>( items: T[], keyFn: (item: T) => string | number ): T[] { return Array.from( items .reduce((map, item) => map.set(keyFn(item), item), new Map<string | number, T>()) .values() ); }Then simplify the code to:
- 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; + const deduplicatedItems = options?.deduplicateBy + ? deduplicate(allItems, options.deduplicateBy) + : allItems;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Utils/request/useInfiniteQuery.ts(1 hunks)
🔇 Additional comments (3)
src/Utils/request/useInfiniteQuery.ts (3)
17-19: Well-implemented state management!
Good choice initializing totalCount with Infinity. This ensures proper handling of the hasMore condition when the total count is not yet known.
27-45:
Chain the onResponse callback with options.
The current implementation overwrites any onResponse callback provided in options. Consider chaining the callbacks to preserve caller-provided functionality.
- onResponse: ({ data }) => {
+ onResponse: (response) => {
+ options?.onResponse?.(response);
+ const { data } = response;
if (!data) return;
const allItems = items.concat(data.results);13-16: Verify integration with useDebounce hook.
Based on the PR comments, this hook should be used with useDebounce to prevent excessive API calls during scrolling.
✅ Verification successful
Integration with useDebounce is properly implemented
The verification shows that useDebounce is correctly integrated with useInfiniteQuery in the Kanban Board component:
useDebounceis imported and used insrc/components/Kanban/Board.tsx- The scroll handler is debounced with a 200ms delay
- The implementation includes proper loading checks and pagination handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if useDebounce is properly integrated with useInfiniteQuery in the Resource page components
# Look for useInfiniteQuery usage with useDebounce
rg -A 10 "useInfiniteQuery.*useDebounce"
# Check for scroll event handlers using these hooks
ast-grep --pattern 'const $_ = useCallback(($_, $_) => {
$$$
fetchNextPage();
$$$
}, $_)'
Length of output: 140
Script:
#!/bin/bash
# Let's try a broader search to find any usage of useInfiniteQuery
rg -A 5 "useInfiniteQuery"
# Also search for scroll handlers separately
rg -A 5 "onScroll|handleScroll"
# And search for useDebounce usage
rg "useDebounce"
# Look for any fetchNextPage calls
rg -A 3 "fetchNextPage"
Length of output: 4086
|
@NikhilA8606 any update on this issue |
|
useInfiniteQuery implemented |
|
Any other changes required? |
|
let's not implement the hook. use the one provided by tanstack query instead. |
Okay |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Resource/ResourceBoard.tsx(7 hunks)src/components/Shifting/ShiftingBoard.tsx(6 hunks)
🔇 Additional comments (6)
src/components/Resource/ResourceBoard.tsx (3)
36-39: LGTM: Type-safe lazy loading implementation.
The implementation correctly uses the generic type parameter to ensure type safety for the lazy-loaded component.
154-156: LGTM: Type-safe item rendering.
The itemRender prop correctly specifies the ResourceModel type for the resource parameter.
Line range hint 138-145: Add error handling and loading state management for infinite scroll.
To address the infinite scroll issue mentioned in the PR objectives, we should add proper error handling and loading state management.
Add error and loading state handling:
fetchOptions: (id: string) => ({
route: routes.listResourceRequests,
options: {
query: formatFilter({
...qParams,
status: id,
}),
+ onError: (error) => {
+ console.error('Failed to fetch resources:', error);
+ // Add error notification here
+ },
},
}),src/components/Shifting/ShiftingBoard.tsx (3)
179-188: LGTM: Improved drag-and-drop handling.
The changes add proper null checking for the destination and improve type safety with the DropResult type.
Line range hint 169-177: Add error handling for infinite scroll in ShiftingBoard.
Similar to ResourceBoard, we need proper error handling for the infinite scroll functionality.
Add error handling:
fetchOptions: (id: string) => ({
route: routes.listShiftRequests,
options: {
query: formatFilter({
...qParams,
status: id,
}),
+ onError: (error) => {
+ console.error('Failed to fetch shifts:', error);
+ // Add error notification here
+ },
},
}),41-43: Verify the infinite scroll implementation in KanbanBoard.
The PR objectives mention infinite scroll issues, but the implementation details are in the KanbanBoard component.
✅ Verification successful
Based on the Board.tsx implementation found, I can now provide a conclusive response about the infinite scroll implementation:
The infinite scroll implementation in KanbanBoard is properly handled
The implementation includes:
- Proper infinite scroll using
useInfiniteQueryhook with debounced scroll handling - Clear loading states and stopping conditions (
loading,hasMoreflags) - Scroll threshold detection for fetching next page
- Proper cleanup of scroll event listeners
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the infinite scroll implementation in KanbanBoard
ast-grep --pattern 'useInfiniteQuery($$$)'
rg -A 10 "useInfiniteQuery" --type typescript
Length of output: 125
Script:
#!/bin/bash
# Search for infinite scroll related patterns in the codebase
rg -l "useInfiniteQuery|onScroll|InfiniteScroll|fetchNextPage" --type ts --type tsx
# Check the Board component implementation
fd "Board.tsx" --type f --exec cat {}
# Look for pagination or infinite scroll related props
rg "interface.*Props" -A 10 --type ts --type tsx
Length of output: 19893
| function lazyWithProps<T>( | ||
| factory: () => Promise<{ default: React.ComponentType<T> }>, | ||
| ) { | ||
| return lazy(factory) as React.LazyExoticComponent<React.ComponentType<T>>; | ||
| } |
There was a problem hiding this comment.
🛠️ 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>>;
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/Kanban/Board.tsx (5)
28-29: Consider removing type assertion for query paramsThe type assertion in the usage of these types (line 96) could be avoided by properly typing the options parameter here.
- options?: { query?: QueryParams }; + options?: { query: QueryParams } | { query?: never };
92-97: Remove type assertion in query optionsThe type assertion on line 96 could be avoided by using proper typing.
- query: section.fetchOptions(section.id).options?.query as QueryParams, + query: section.fetchOptions(section.id).options?.query,
123-131: Optimize IntersectionObserver optionsThe current threshold of 0.5 might trigger too early or late depending on the item size.
const observer = new IntersectionObserver( (entries) => { const lastEntry = entries[0]; if (lastEntry.isIntersecting) { fetchNextPage(); } }, - { root: currentRef, threshold: 0.5 }, + { + root: currentRef, + threshold: 0.1, + rootMargin: '100px' + }, );
177-181: Enhance loading state feedbackThe current loading state could be more informative to users.
- {loading || isFetching ? ( - <div className="mt-2 h-[50px] w-[284px] animate-pulse rounded-lg bg-secondary-300"> - Loading... - </div> - ) : null} + {loading || isFetching ? ( + <div className="mt-2 h-[50px] w-[284px] animate-pulse rounded-lg bg-secondary-300 p-4"> + <div className="flex items-center justify-center gap-2"> + <div className="h-4 w-4 animate-spin rounded-full border-2 border-primary border-t-transparent" /> + <span className="text-sm text-gray-600">Loading more items...</span> + </div> + </div> + ) : null}
141-141: Remove console.log statementRemove the commented-out console.log statement as it's not needed in production code.
- // console.log("Items:", items);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Kanban/Board.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Kanban/Board.tsx (2)
11-16: LGTM! Clean import organization for pagination types
The new imports are well-organized and properly typed for the pagination functionality.
Line range hint 62-74: LGTM! Clean implementation of horizontal scrolling container
The scroll container implementation with proper overflow handling and ref usage is well done.
| if (data?.results) { | ||
| setItems((prevItems) => { | ||
| const newItems = data.results.filter( | ||
| (newItem) => !prevItems.some((item) => item.id === newItem.id), | ||
| ); | ||
| return [...prevItems, ...newItems]; | ||
| }); | ||
| setNextPage(data.next || null); | ||
| setTotalCount(data.count || 0); | ||
| } | ||
| }, [data]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for data synchronization
The effect handles successful data updates but lacks error handling.
useEffect(() => {
+ if (data?.error) {
+ console.error('Failed to fetch items:', data.error);
+ return;
+ }
if (data?.results) {
setItems((prevItems) => {
const newItems = data.results.filter(
(newItem) => !prevItems.some((item) => item.id === newItem.id),
);
return [...prevItems, ...newItems];
});
setNextPage(data.next || null);
setTotalCount(data.count || 0);
}
}, [data]);📝 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.
| if (data?.results) { | |
| setItems((prevItems) => { | |
| const newItems = data.results.filter( | |
| (newItem) => !prevItems.some((item) => item.id === newItem.id), | |
| ); | |
| return [...prevItems, ...newItems]; | |
| }); | |
| setNextPage(data.next || null); | |
| setTotalCount(data.count || 0); | |
| } | |
| }, [data]); | |
| useEffect(() => { | |
| if (data?.error) { | |
| console.error('Failed to fetch items:', data.error); | |
| return; | |
| } | |
| if (data?.results) { | |
| setItems((prevItems) => { | |
| const newItems = data.results.filter( | |
| (newItem) => !prevItems.some((item) => item.id === newItem.id), | |
| ); | |
| return [...prevItems, ...newItems]; | |
| }); | |
| setNextPage(data.next || null); | |
| setTotalCount(data.count || 0); | |
| } | |
| }, [data]); |
Describe the bug
The resource page has an infinite scroll issue where content keeps loading repeatedly, causing poor performance and a bad user experience. The stopping condition for the scroll is not working correctly, leading to continuous or unnecessary data fetching.
Screen recording of the error is given below
Screen.Recording.2024-12-02.094908.mp4
Screen recording of the page after resolving the error
Screen.Recording.2024-12-02.115614.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Documentation