fix: resolve react-hooks/exhaustive-deps warnings (#4789) #4804
Conversation
sammy200-ui
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR addresses React Hook dependency warnings across 7 components by updating dependency arrays with missing dependencies, adding ESLint suppression comments where mount-only behavior is intentional, and improving null-handling safety with optional chaining. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4804 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 798 798
Branches 146 146
=========================================
Hits 798 798 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/AlgoliaSearch.tsxcomponents/campaigns/AnnouncementHero.tsxcomponents/dashboard/table/Filters.tsxcomponents/navigation/DocsNav.tsxcomponents/navigation/Filter.tsxcomponents/tools/ToolsCard.tsxcomponents/tools/ToolsDashboard.tsxtsconfig.tsbuildinfo
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/AlgoliaSearch.tsxcomponents/navigation/Filter.tsxcomponents/tools/ToolsCard.tsxcomponents/dashboard/table/Filters.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Agent
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: cypress-run
- GitHub Check: Lighthouse CI
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (7)
components/AlgoliaSearch.tsx (1)
318-319: Acceptable mount-only pattern, but consider a minor edge case.The lint suppression is justified here. The
actionKeyis stable (derived fromnavigator.userAgent), and thechildrenprop is typically a static render function. Based on retrieved learnings, this pattern is acceptable for this component.However, if
childrenwere to change dynamically after mount, the UI wouldn't reflect the update. If this is intentional, the current implementation is fine.components/navigation/DocsNav.tsx (1)
77-79: LGTM!Correctly adds
item.item.slugto the dependency array since it's used in the effect body. The effect will now properly re-run when eitheractiveor the slug changes, ensuringopenSubCategorystays in sync.components/dashboard/table/Filters.tsx (1)
22-42: LGTM!Adding
setOpento the dependency array is correct. WhilesetOpenfromuseStateis referentially stable and won't cause re-runs, including it satisfies the exhaustive-deps rule and makes the code more explicit about its dependencies.components/campaigns/AnnouncementHero.tsx (1)
24-24: Acceptable, but consider keeping[banners]for clarity.Since
bannersis a module-level import (constant), changing from[banners]to[]has no runtime impact. However, keeping[banners]would be more self-documenting about what the memo depends on and safer ifbannersever becomes dynamic in the future.If the goal is purely to silence the lint warning, this works. The behavior is unchanged.
components/tools/ToolsDashboard.tsx (2)
158-160: Good improvement: safer optional chaining.Replacing the non-null assertion with optional chaining (
?.) is a safer approach that prevents potential runtime errors iftoolsList[elementID]is undefined or ifelementRefis not set.
166-167: Mount-only scroll is appropriate here.The effect intentionally runs once on mount to scroll to the URL hash. Since
toolsListis synchronously computed viauseMemobefore this effect runs, the refs are available on first render. The lint suppression is justified for this one-time scroll-into-view behavior.components/tools/ToolsCard.tsx (1)
31-36: LGTM - Correct fix for ref dependency anti-pattern.The original
[descriptionRef.current]was incorrect because:
- Ref
.currentmutations don't trigger re-renders- React doesn't track ref changes as dependencies
Using
[]for mount-only truncation detection is appropriate since the ref is attached after the first render. If dynamic resize handling is needed in the future, consider aResizeObserver.
| useEffect(() => { | ||
| setQuery(route.query); | ||
| applyFilterList(checks, data, setFilters); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only on route changes | ||
| }, [route]); | ||
|
|
||
| useEffect(() => { | ||
| onFilterApply(data, onFilter, routeQuery); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes | ||
| }, [routeQuery]); |
There was a problem hiding this comment.
Potential stale closure: data, checks, and onFilter are omitted from dependencies.
The lint suppression may mask actual bugs:
- First effect (lines 32-36): If
checksordataprops change without a route change,applyFilterListwill use stale values. - Second effect (lines 38-41): If
dataoronFilterprops change,onFilterApplywill use stale closures.
If these props are truly stable (never change after mount), consider documenting that assumption. Otherwise, include them in the dependency arrays:
🔎 Suggested fix if props can change
useEffect(() => {
setQuery(route.query);
applyFilterList(checks, data, setFilters);
- // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only on route changes
- }, [route]);
+ }, [route, checks, data]);
useEffect(() => {
onFilterApply(data, onFilter, routeQuery);
- // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes
- }, [routeQuery]);
+ }, [routeQuery, data, onFilter]);📝 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.
| useEffect(() => { | |
| setQuery(route.query); | |
| applyFilterList(checks, data, setFilters); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only on route changes | |
| }, [route]); | |
| useEffect(() => { | |
| onFilterApply(data, onFilter, routeQuery); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes | |
| }, [routeQuery]); | |
| useEffect(() => { | |
| setQuery(route.query); | |
| applyFilterList(checks, data, setFilters); | |
| }, [route, checks, data]); | |
| useEffect(() => { | |
| onFilterApply(data, onFilter, routeQuery); | |
| }, [routeQuery, data, onFilter]); |
🤖 Prompt for AI Agents
In components/navigation/Filter.tsx around lines 32 to 41, the two useEffect
hooks omit props (data, checks, onFilter) from their dependency arrays which can
lead to stale closures; either add the missing dependencies to the effect
dependency arrays (include checks and data in the first effect, include data and
onFilter in the second) or, if those props are guaranteed stable, explicitly
memoize them (useMemo/useCallback) or document the stability with a comment and
assert it via types, then remove the eslint-disable comments accordingly so the
effects always run with current values.
There was a problem hiding this comment.
This is intentional - the Filter component is designed to react only to route/URL changes, not prop changes.
The maintainer guidance for this issue was to use eslint-disable comments for cases where dependencies are intentionally omitted. The comments document this is by design, not an oversight.
There was a problem hiding this comment.
@sammy200-ui, thank you for the clarification! That makes sense—if the Filter component is designed to respond only to route/URL changes and not to prop updates, then the current dependency arrays are correct by design. The eslint-disable comments clearly document this intent, and following maintainer guidance to use them for intentionally omitted dependencies is the right approach here.
I appreciate you explaining the design decision. I'll note this for future reference.
✏️ Learnings added
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the Filter component (components/navigation/Filter.tsx), the useEffect hooks are intentionally designed to react only to route/URL changes, not to prop changes (data, checks, onFilter). The omitted dependencies are by design.
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: For the asyncapi/website repository, maintainer guidance is to use eslint-disable comments with explanatory text for cases where useEffect/useMemo dependencies are intentionally omitted by design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4804--asyncapi-website.netlify.app/ |
There was a problem hiding this comment.
Pull request overview
This PR addresses React's exhaustive-deps ESLint warnings across 7 component files by either adding missing dependencies to hook dependency arrays or adding eslint-disable comments with justifications for intentionally omitted dependencies.
- Updates
useEffectanduseMemodependency arrays to include all referenced values - Adds eslint-disable comments for cases where dependencies are intentionally omitted
- Refactors null assertion operators to use optional chaining for safer null handling
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| components/AlgoliaSearch.tsx | Adds eslint-disable comment for mount-only effect that sets children state |
| components/campaigns/AnnouncementHero.tsx | Removes banners from useMemo dependency array |
| components/dashboard/table/Filters.tsx | Adds setOpen to useEffect dependencies for click-outside handler |
| components/navigation/DocsNav.tsx | Adds item.item.slug to useEffect dependencies |
| components/navigation/Filter.tsx | Adds eslint-disable comments for two effects with intentionally limited dependencies |
| components/tools/ToolsCard.tsx | Changes dependency array from [descriptionRef.current] to empty array with disable comment |
| components/tools/ToolsDashboard.tsx | Refactors to use optional chaining and adds eslint-disable comment for mount-only scroll effect |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [activeIndex, setActiveIndex] = useState(0); | ||
|
|
||
| const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), [banners]); | ||
| const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), []); |
There was a problem hiding this comment.
The dependency array for this useMemo is incorrect. The banners variable is imported from './banners' and is a constant that doesn't change between renders. However, removing it from the dependency array when it's being used in the computation is not the correct fix. The correct approach would be to keep the dependency array as-is since banners is a stable import, or if React is warning about it, the warning itself may be incorrect. An empty dependency array means this will only compute once on mount, but if banners were to change (e.g., in a hot reload scenario), the memo wouldn't recompute.
| const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), []); | |
| const visibleBanners = useMemo( | |
| () => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), | |
| [banners], | |
| ); |
There was a problem hiding this comment.
The original ESLint warning specifically stated that banners is an "outer scope value" that shouldn't be included in dependencies because it's an imported constant, not a prop or state. The warning was to remove it, which we did. The [] dependency array is correct for this case.
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only on route changes | ||
| }, [route]); | ||
|
|
||
| useEffect(() => { | ||
| onFilterApply(data, onFilter, routeQuery); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes | ||
| }, [routeQuery]); |
There was a problem hiding this comment.
While adding data and onFilter to the dependencies would satisfy the exhaustive-deps rule, these appear to be props that could change on every render, which would cause unnecessary re-filtering. The comment states "Intentionally run only on route changes", but the dependencies are incomplete. Consider whether checks should also be included, or if this hook should use useCallback/useMemo for the functions and data to stabilize their references. The current approach may miss updates when these props change.
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only on route changes | |
| }, [route]); | |
| useEffect(() => { | |
| onFilterApply(data, onFilter, routeQuery); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes | |
| }, [routeQuery]); | |
| }, [route, data, checks]); | |
| useEffect(() => { | |
| onFilterApply(data, onFilter, routeQuery); | |
| }, [routeQuery, data, onFilter]); |
There was a problem hiding this comment.
As explained for the coderabbit concern
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes | ||
| }, [routeQuery]); | ||
|
|
There was a problem hiding this comment.
The eslint-disable comment states "Intentionally run only when routeQuery changes", but the effect also depends on data and onFilter props which are not included in the dependency array. If these props change while routeQuery remains the same, the filter won't be reapplied with the new data or callback. This could lead to stale closures or incorrect filtering behavior.
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run only when routeQuery changes | |
| }, [routeQuery]); | |
| }, [routeQuery, data, onFilter]); |
There was a problem hiding this comment.
Same as above - the component is designed to react only to routeQuery changes.
components/AlgoliaSearch.tsx
Outdated
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run on mount only | ||
| }, []); |
There was a problem hiding this comment.
The effect depends on children and actionKey which are not included in the dependency array. If children is a function and changes between renders, or if actionKey changes, the component won't update to reflect the new value. Consider including these dependencies or documenting why they should be excluded (e.g., if actionKey is stable and children is not expected to change).
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run on mount only | |
| }, []); | |
| }, [children, actionKey]); |
There was a problem hiding this comment.
Adding these dependencies could cause issues if children is a render function that creates new references. The eslint-disable is intentional per maintainer guidance.
|
@anshgoyalevil pr is open for review!! , coderabbit and copilot left some suggestion that should not to taken into consideration or if you want any changes happy to do so!! |
|
Hi @princerajpoot20 , Should I accept the incoming changes with full dependencies, or keep my eslint-disable approach per the guidance? 
|
2 participants
Fixes #4789
This PR resolves all 8
react-hooks/exhaustive-depswarnings across 7 files.components/AlgoliaSearch.tsxcomponents/campaigns/AnnouncementHero.tsxcomponents/dashboard/table/Filters.tsxcomponents/navigation/DocsNav.tsxcomponents/navigation/Filter.tsxcomponents/tools/ToolsCard.tsxcomponents/tools/ToolsDashboard.tsxSummary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.