-
Notifications
You must be signed in to change notification settings - Fork 661
Convert TraceStatisticsHeader to a functional component #3479
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?
Convert TraceStatisticsHeader to a functional component #3479
Conversation
Signed-off-by: ConradKash <[email protected]>
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 converts the TraceStatisticsHeader component from a React class component to a functional component using hooks, as part of the larger UI modernization effort (#2610).
Changes:
- Converted class component to functional component with hooks (useState, useEffect, useCallback, useMemo)
- Replaced lifecycle method (constructor) with useEffect hook for initialization
- Added comprehensive test suite using React Testing Library
- Refactored all class methods to useCallback hooks
- Fixed typo in variable name (newWohleTable → newWholeTable)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| TraceStatisticsHeader.tsx | Converted from class to functional component with hooks; replaced state with useState, constructor initialization with useEffect, and methods with useCallback |
| TraceStatisticsHeader.test.js | New test file with comprehensive test coverage using React Testing Library instead of Enzyme |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.test.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.test.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
|
@ConradKash please process the feedback accordingly, and sign the DCO. Read more in CONTRIBUTING.md. |
Signed-off-by: ConradKash <[email protected]>
a8ab3fb to
bf2b7e9
Compare
…alculations Signed-off-by: ConradKash <[email protected]>
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: ConradKash <[email protected]>
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.test.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: ConradKash <[email protected]>
…k and update handler to use initialServiceName Signed-off-by: ConradKash <[email protected]>
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.clearValue = this.clearValue.bind(this); | ||
| } | ||
| // This ensures that the service name is only computed once on initial render | ||
| const initialServiceName = getServiceName(); |
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.
Performance Issue: Unnecessary computation on every render
getServiceName() is called on every render of the component, but the value is only used in the useEffect which runs once due to the empty dependency array []. After the initial render, this computation is wasted.
The comment claims "This ensures that the service name is only computed once on initial render" but this is incorrect - it's computed on every render.
Fix:
const initialServiceName = useMemo(() => getServiceName(), []);Or better yet, compute it inside the useEffect where it's actually used, eliminating the need for this variable entirely.
| const initialServiceName = getServiceName(); | |
| const initialServiceName = useMemo(() => getServiceName(), []); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
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.
why is this resolved? The bot is right, and the comment is lying.
…aceStatisticsHeader.tsx Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Signed-off-by: Kakuru Conrad Akankwasa <[email protected]>
…aceStatisticsHeader.test.js Co-authored-by: Copilot <[email protected]> Signed-off-by: Kakuru Conrad Akankwasa <[email protected]>
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/TracePage/TraceStatistics/TraceStatisticsHeader.tsx
Outdated
Show resolved
Hide resolved
…ServiceName computation with useMemo Signed-off-by: ConradKash <[email protected]>
Hey @jkowall, I have finished setting PR to the standard kindly review. |
yurishkuro
left a comment
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.
please address comments. Minimize the diff by avoiding unnecessary code moves.
|
|
||
| }, []); | ||
|
|
||
| const setValueNameSelector1 = useCallback( |
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.
setValueNameSelector1 was defined earlier in the code, why is it moved down? This increases the size of the diff, making it harder to review
| * @param {string} valueNameSelector1 - The primary value selector name. | ||
| * @param {string | null} valueNameSelector2 - The optional secondary value selector name. | ||
| * @property {boolean} useOtelTerms - Flag to determine whether to use OpenTelemetry terminology in the UI. | ||
| */ |
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.
documentation is useful but need to be inline, not as a separate block that will get out of sync with the actual definition
Which problem is this PR solving?
Fixes #3370
Description of the changes
Converts the TraceStatisticsHeader class component to a functional component using React hooks:
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test