-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(table): resolve double fetch issue in useInfiniteScroll hook #3332
base: canary
Are you sure you want to change the base?
fix(table): resolve double fetch issue in useInfiniteScroll hook #3332
Conversation
🦋 Changeset detectedLatest commit: 35f54ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@abhisektomar1 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Hook
participant Observer
participant API
User ->> Hook: Initialize useInfiniteScroll
Hook ->> Observer: Create IntersectionObserver
Observer ->> Hook: Observe scroll position
Hook ->> Hook: Check condition and debounce
Hook ->> API: Fetch data
API -->> Hook: Return data
Hook -->> User: Update data state
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/hooks/use-infinite-scroll/src/index.ts (3 hunks)
Additional context used
Biome
packages/hooks/use-infinite-scroll/src/index.ts
[error] 85-100: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (6)
packages/hooks/use-infinite-scroll/src/index.ts (6)
2-2
: Imports look good.The addition of
useCallback
supports the new debouncedloadMore
function, aligning with the PR objectives.
43-51
: Effective use ofuseCallback
and concurrency control inloadMore
.This implementation effectively prevents double fetching and ensures that
loadMore
is not recreated unnecessarily, which aligns with the PR objectives to optimize infinite scroll behavior.
66-66
: OptimizedIntersectionObserver
setup.The new
IntersectionObserver
setup uses improved options, including a root margin based on thedistance
prop and a lower threshold, which better handles the trigger for loading more content.Also applies to: 69-75, 78-78
80-84
: Proper cleanup forIntersectionObserver
.The cleanup logic properly disconnects the observer when the component unmounts or is no longer needed, preventing potential memory leaks and ensuring resource efficiency.
86-98
: Optimized scroll-based loading logic with debounce.The debounced event listener for scroll events is a good optimization, preventing excessive loading triggers and enhancing performance during rapid scrolling.
103-103
: Appropriate return values for external interaction.Returning
loaderRef
andscrollContainerRef
allows for external manipulation and interaction, which is essential for the hook's functionality in various scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you've modified code inside packages, please add a changeset file. The content should be like this.
---
"@nextui-org/use-infinite-scroll": patch
---
fix(table): resolve double fetch issue in useInfiniteScroll hook (#3251)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/hooks/use-infinite-scroll/src/index.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/hooks/use-infinite-scroll/src/index.ts
I noticed that there's a test failure related to the NextUI.Input component inside an Autocomplete component. However, my changes were only to the useInfiniteScroll hook and didn't touch these components. This seems to be a pre-existing issue in the test suite. Could someone from the maintenance team please advise on how to proceed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- .changeset/fix-infinite-scroll.md (1 hunks)
- packages/hooks/use-infinite-scroll/src/index.ts (3 hunks)
Additional comments not posted (5)
.changeset/fix-infinite-scroll.md (1)
1-5
: Changelog entry looks good.The changeset file correctly documents the fix for the double fetch issue in the
useInfiniteScroll
hook.packages/hooks/use-infinite-scroll/src/index.ts (4)
2-2
: Import statement looks good.The import of
useCallback
is necessary for the newloadMore
function.
40-41
: State management elements look good.The additions of
observerRef
andisLoadingRef
are necessary for managing theIntersectionObserver
and preventing concurrent load calls.
Line range hint
54-105
:
useLayoutEffect
logic looks good.The updates to simplify the
IntersectionObserver
logic and improve cleanup enhance the robustness of theuseInfiniteScroll
hook.
107-107
: Return statement looks good.The return statement is consistent with the expected return type for the
useInfiniteScroll
hook.
Summary
Resolved the issue where the useInfiniteScroll hook was fetching data twice on initial render. This fix ensures that data is fetched only once during initial render and additional data is fetched upon scrolling.
Issue
Changes
Closes #
📝 Description
I use isLoadingRef to prevent concurrent load calls, implements a debounced loadMore function with useCallback, and simplifies the IntersectionObserver logic.
The hook now handles both loader-based and scroll-based detection more efficiently, using debounce for scroll events. I also improved cleanup for the IntersectionObserver and better TypeScript typing. These changes collectively make the hook more robust, reducing issues like multiple simultaneous loads and excessive re-renders, while providing a smoother infinite scrolling experience
⛳️ Current behavior (updates)
Table useInfiniteScroll hook fetches twice
🚀 New behavior
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
loadMore
callback for improved data loading.