-
Notifications
You must be signed in to change notification settings - Fork 191
fix: resolve React Hook dependency warnings in useEffect hooks #286
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This pull request aims to resolve 7 ESLint react-hooks/exhaustive-deps warnings across 3 React components by adding missing dependencies to useEffect hooks, wrapping callbacks in useCallback, using functional setState updates, and adding ESLint disable comments where dependencies are intentionally omitted.
Changes:
- Added missing dependencies to
useEffecthooks to satisfy exhaustive-deps linting rules - Converted callback functions to
useCallbackwhere they are used in effect dependencies - Updated state setters to use functional form to avoid stale closure issues
- Added ESLint disable comments with explanatory notes for effects that intentionally run once on mount
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| components/post-body.tsx | Added useCallback import; added isUserEnteredURL to scroll observer effect deps; wrapped handleHeadingCopyClick in useCallback with functional setState; added handleHeadingCopyClick to effect deps |
| components/navbar/FloatingNavbarClient.tsx | Added explanatory comments and ESLint disable directives for effects that intentionally run once on mount; added techLatest and communityLatest to SearchBox filtering effect deps |
| components/more-stories.tsx | Added useCallback import; added ESLint disable for URL sync effect; wrapped loadMoreInBackground in useCallback; added dependencies to buffer logic effect |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, [initialPosts, isSearchPage]); | ||
|
|
||
| // 1. Sync Input from URL on Load | ||
| // 1. Sync Input from URL on Load (only on initial mount) |
Copilot
AI
Jan 22, 2026
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.
The comment says "only on initial mount" but the effect actually runs whenever router.query changes (due to the dependency array). Consider updating the comment to accurately reflect when this effect runs, e.g., "Sync search term from URL when route query changes".
| // 1. Sync Input from URL on Load (only on initial mount) | |
| // 1. Sync search term from URL when route query is ready/changes (when not externally controlled) |
| useEffect(() => { | ||
| if (!isSearchPage && initialPosts.length > 21) { | ||
| setBuffer(initialPosts.slice(21)); | ||
| } | ||
| if (isIndex && initialPageInfo?.hasNextPage && (!buffer.length || buffer.length < 9)) { | ||
| loadMoreInBackground(); | ||
| } | ||
| }, [initialPosts, isIndex, isSearchPage, initialPageInfo?.hasNextPage, buffer.length, loadMoreInBackground]); |
Copilot
AI
Jan 22, 2026
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.
Adding loadMoreInBackground to the dependency array introduces a problematic cycle. When loadMoreInBackground successfully fetches posts, it updates endCursor (line 151), which causes loadMoreInBackground to be recreated (since it depends on endCursor). This triggers the effect to run again, and line 164 resets the buffer to initialPosts.slice(21), discarding any posts that were loaded by loadMoreInBackground.
The original code intentionally omitted loadMoreInBackground from the dependencies to avoid this issue. Consider either: (1) removing loadMoreInBackground from the dependency array and adding an ESLint disable comment with explanation, or (2) splitting this into two separate effects - one for buffer initialization from initialPosts, and another for triggering background loads based on buffer state.
| useEffect(() => { | |
| if (!isSearchPage && initialPosts.length > 21) { | |
| setBuffer(initialPosts.slice(21)); | |
| } | |
| if (isIndex && initialPageInfo?.hasNextPage && (!buffer.length || buffer.length < 9)) { | |
| loadMoreInBackground(); | |
| } | |
| }, [initialPosts, isIndex, isSearchPage, initialPageInfo?.hasNextPage, buffer.length, loadMoreInBackground]); | |
| // Initialize buffer from initial posts (one-time per initialPosts / isSearchPage change) | |
| useEffect(() => { | |
| if (!isSearchPage && initialPosts.length > 21) { | |
| setBuffer(initialPosts.slice(21)); | |
| } | |
| }, [initialPosts, isSearchPage]); | |
| // Trigger background loading when buffer is running low | |
| useEffect(() => { | |
| if (isIndex && initialPageInfo?.hasNextPage && (!buffer.length || buffer.length < 9)) { | |
| loadMoreInBackground(); | |
| } | |
| }, [isIndex, initialPageInfo?.hasNextPage, buffer.length, loadMoreInBackground]); |
| @@ -1,4 +1,4 @@ | |||
| import { useState, useEffect, useRef } from "react"; | |||
| import { useState, useEffect, useRef, useCallback } from "react"; | |||
Copilot
AI
Jan 22, 2026
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.
Unused import useRef.
| import { useState, useEffect, useRef, useCallback } from "react"; | |
| import { useState, useEffect, useCallback } from "react"; |
|
@amaan-bhati Thanks for the update! I’ll wait for the review |
1542e1f to
61ccd4a
Compare
- more-stories.tsx: wrap loadMoreInBackground in useCallback and add missing deps - FloatingNavbarClient.tsx: add eslint-disable for intentional mount-only effects, add missing deps to search filter effect - post-body.tsx: wrap handleHeadingCopyClick in useCallback, add isUserEnteredURL to deps This fixes 7 ESLint react-hooks/exhaustive-deps warnings that could cause stale closures or unexpected behavior. Signed-off-by: Yuvraj <yuvraj.r0810@gmail.com>
61ccd4a to
3bb4c5d
Compare
Summary
Fixes 7 ESLint
react-hooks/exhaustive-depswarnings across 3 components.Before (7 warnings)
After
npm run lint # Only 1 unrelated warning remains (img in testimonials.tsx)