Enable react-hooks/exhaustive-deps linter rule#3471
Merged
yurishkuro merged 8 commits intoFeb 3, 2026
Merged
Conversation
Closes jaegertracing#3445 Signed-off-by: Taanvi Khevaria <[email protected]>
taanvi2205
force-pushed
the
enable-react-hooks-exhaustive-deps
branch
from
1e752f4 to
7f247d8
Compare
The component was incorrectly trying to use a navigate prop that does not exist. The withRouteProps HOC provides a history object with a push method Signed-off-by: Taanvi Khevaria <[email protected]>
taanvi2205
force-pushed
the
enable-react-hooks-exhaustive-deps
branch
from
57547e6 to
01cab1b
Compare
yurishkuro
added
the
changelog:ci
yurishkuro
reviewed
Jan 30, 2026
| viewModifiers: new Map(), | ||
| }, | ||
| navigate: jest.fn(), | ||
| history: { push: jest.fn() }, |
Member
There was a problem hiding this comment.
how is this change related to the linter?
yurishkuro
requested changes
Jan 30, 2026
Member
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
there seem to be a lot of unrelated changes in this PR.
Contributor
Author
|
@yurishkuro The ~10k lines are from package-lock.json (adding the eslint-plugin-react-hooks dependency). I've removed the unrelated DeepDependencies changes. |
yurishkuro
reviewed
Feb 2, 2026
Member
|
all commits must be signed |
github-actions
Bot
added
the
waiting-for-author
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request enables the react-hooks/exhaustive-deps ESLint rule to catch missing dependencies in React hooks, improving code correctness and preventing potential bugs from stale closures.
Changes:
- Added
eslint-plugin-react-hooksdependency and configured bothrules-of-hooksandexhaustive-depsrules - Fixed 12 hook dependency violations across 6 files by adding missing dependencies to useEffect and useCallback hooks
- Added an eslint-disable comment for an intentional mount-only useEffect in SearchTracePage
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added eslint-plugin-react-hooks ^7.0.1 dependency; alphabetized ajv entry |
| package-lock.json | Installed eslint-plugin-react-hooks and its dependencies (hermes-parser, zod, etc.) |
| eslint.config.js | Configured react-hooks plugin and enabled rules-of-hooks and exhaustive-deps rules |
| packages/jaeger-ui/src/utils/documentTitle.ts | Fixed cleanup function to capture ref value in local variable before closure |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/index.tsx | Destructured props and updated useCallback dependencies to include action functions and trace.spans |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordionEvents.tsx | Added spanID to useEffect dependency array |
| packages/jaeger-ui/src/components/TracePage/ArchiveNotifier/index.tsx | Destructured props and added acknowledge to useEffect dependencies |
| packages/jaeger-ui/src/components/SearchTracePage/index.tsx | Added eslint-disable comment for intentional mount-only useEffect |
| packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.tsx | Added calculateContainerWidth to useEffect dependency array |
| packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx | Refactored to destructure props and updated useCallback/useEffect dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
removed
the
waiting-for-author
Signed-off-by: Taanvi Khevaria <[email protected]>
Signed-off-by: Taanvi Khevaria <[email protected]>
Signed-off-by: Taanvi Khevaria <[email protected]>
taanvi2205
force-pushed
the
enable-react-hooks-exhaustive-deps
branch
from
d468f72 to
5d39a76
Compare
Contributor
Author
|
@yurishkuro please review it now |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3471 +/- ##
==========================================
+ Coverage 88.26% 88.31% +0.05%
==========================================
Files 299 299
Lines 9485 9486 +1
Branches 2419 2509 +90
==========================================
+ Hits 8372 8378 +6
+ Misses 1109 1104 -5
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Taanvi Khevaria <[email protected]>
yurishkuro
reviewed
Feb 2, 2026
yurishkuro
reviewed
Feb 2, 2026
| // This may require "eslint-disable-next-line react-hooks/exhaustive-deps" | ||
| // in the future if we enable this linter. | ||
| // https://github.com/jaegertracing/jaeger-ui/issues/3445 | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
Member
There was a problem hiding this comment.
disabling linter must be done with a comment explaining why
yurishkuro
reviewed
Feb 2, 2026
Member
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
mostly LGTM, just two questions
yurishkuro
removed
the
changelog:ci
yurishkuro
added
the
changelog:bugfix-or-minor-feature
Signed-off-by: Taanvi Khevaria <[email protected]>
yurishkuro
approved these changes
Feb 3, 2026
samar-703
pushed a commit
to samar-703/jaeger-ui
that referenced
this pull request
Feb 3, 2026
Closes jaegertracing#3445 ## Which problem is this PR solving? - closes jaegertracing#3445 ## Description of the changes - Add eslint-plugin-react-hooks dependency - Fix 12 violations across 6 files by adding missing hook dependencies - Add eslint-disable comment for intentional mount-only useEffect in SearchTracePage ## How was this change tested? - npm run eslint passes with 0 errors - npm run test passes ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Taanvi Khevaria <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3445
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test