Refactor(ui): Convert VirtualizedTraceView from class to functional component#3551
Refactor(ui): Convert VirtualizedTraceView from class to functional component#3551aryanG9403 wants to merge 4 commits intojaegertracing:mainfrom
Conversation
…omponent Signed-off-by: Aryan Ghotekar <aryanghotekar95@gmail.com>
github-actions
Bot
added
the
pr-quota-reached
|
Hi @aryanG9403, thanks for your contribution! To ensure quality reviews, we limit how many concurrent open PRs new contributors can open. This PR is currently on hold (Status: 2/1 open). We will automatically move this into the review queue once your existing PRs are merged or closed. Please see our Contributing Guidelines for details on our tiered quota policy. |
There was a problem hiding this comment.
Pull request overview
Refactors Jaeger UI’s VirtualizedTraceView (trace timeline viewer) from a class component to a React functional component using hooks, updating routing integration and rewriting unit tests to align with the new implementation.
Changes:
- Converted
VirtualizedTraceViewImplto a hook-based functional component and switched routing updates touseNavigate(). - Replaced class instance method testing with RTL-based rendering tests and added
testableHelpersexports for pure helper logic. - Updated ListView integration to use
ref+ effects for registering accessors and handling resize/measure events.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx |
Converts the component to hooks, updates navigation handling, and adds exported helper utilities for tests. |
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.test.js |
Migrates tests to RTL patterns and updates mocking strategy for ListView and routing. |
Comments suppressed due to low confidence (1)
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.test.js:536
- This test mutates the module-level
linkPatterns.processedLinksbut no longer restores it afterward (the previous beforeEach/afterAll cleanup was removed). That can leak state into other tests and cause order-dependent failures. Please snapshot and restoreprocessedLinks(or clear it in afterEach) within this describe block.
describe('linksGetter()', () => {
it('linksGetter is expected to receive url and text for a given link pattern', () => {
const span = trace.spans[1];
const key = span.attributes[0].key;
const value = span.attributes[0].value;
const val = encodeURIComponent(value);
const linkPatternConfig = [
{
key,
type: 'tags',
url: `http://example.com/?key1=#{${key}}&traceID=#{trace.traceID}&startTime=#{trace.startTime}`,
text: `For first link traceId is - #{trace.traceID}`,
},
].map(linkPatterns.processLinkPattern);
linkPatterns.processedLinks.push(...linkPatternConfig);
const result = getLinks(span, span.attributes, 0, trace);
expect(result).toEqual([
{
url: `http://example.com/?key1=${val}&traceID=${trace.traceID}&startTime=${trace.startTime}`,
text: `For first link traceId is - ${trace.traceID}`,
},
]);
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('focusSpan', () => { | ||
| it('calls updateUiFind and focusUiFindMatches', () => { | ||
| const spanName = 'span1'; | ||
| instance.focusSpan(spanName); | ||
|
|
||
| expect(updateUiFindSpy).toHaveBeenLastCalledWith({ | ||
| history: mockProps.history, | ||
| location: mockProps.location, | ||
| uiFind: spanName, | ||
| }); | ||
|
|
||
| expect(focusUiFindMatchesMock).toHaveBeenLastCalledWith(trace, spanName, false); | ||
| render(<VirtualizedTraceViewImpl {...mockProps} />); | ||
| expect(updateUiFindSpy).not.toHaveBeenCalled(); // not called on mount | ||
| }); |
There was a problem hiding this comment.
The focusSpan test currently only asserts updateUiFind is not called on mount, but it never triggers focusSpan, so it doesn't verify the intended behavior (calling updateUiFind, navigate, and focusUiFindMatches). Consider rendering a row (via the ListView mock) and invoking the passed focusSpan callback, or unit-testing the callback via a lightweight child mock.
| component.listView = {}; | ||
| component.componentDidUpdate(mockProps); | ||
| const { rerender } = render(<VirtualizedTraceViewImpl {...mockProps} />); | ||
| rerender(<VirtualizedTraceViewImpl {...mockProps} shouldScrollToFirstUiFindMatch={true} />); |
There was a problem hiding this comment.
updatedProps is declared but never used in this test. Please remove it, or use it for the rerender call to keep the test intent clear.
| rerender(<VirtualizedTraceViewImpl {...mockProps} shouldScrollToFirstUiFindMatch={true} />); | |
| rerender(<VirtualizedTraceViewImpl {...updatedProps} />); |
| import { PEER_SERVICE } from '../../../constants/tag-keys'; | ||
| import withRouteProps from '../../../utils/withRouteProps'; | ||
| import { useNavigate } from 'react-router-dom-v5-compat'; |
There was a problem hiding this comment.
useNavigate() is used here, but the PR description says the component was updated to use useHistory(). Please update the PR description (or the implementation) so reviewers can accurately validate the routing change and its implications (e.g., react-router v5 history vs v6 navigate semantics).
| React.useEffect(() => { | ||
| if (shouldScrollToFirstUiFindMatch) { | ||
| scrollToFirstVisibleSpan(); | ||
| clearShouldScrollToFirstUiFindMatch(); | ||
| } | ||
| return false; | ||
| } | ||
| }, [shouldScrollToFirstUiFindMatch, scrollToFirstVisibleSpan, clearShouldScrollToFirstUiFindMatch]); |
There was a problem hiding this comment.
The class version had a shouldComponentUpdate optimization to skip the extra render when shouldScrollToFirstUiFindMatch flips from true → false after clearing. With the hooks version, the clearShouldScrollToFirstUiFindMatch() dispatch will always trigger a re-render, which can be costly given ListView virtualization. Consider wrapping the component in React.memo with a custom comparator that preserves the previous behavior (skip re-render when the only change is this flag becoming false).
| describe('renderRow()', () => { | ||
| it('renders a SpanBarRow when it is not a detail', () => { | ||
| instance = createTestInstance(mockProps); | ||
| const rowResult = instance.renderRow('some-key', {}, 1, {}); | ||
|
|
||
| expect(rowResult.type).toBe('div'); | ||
| expect(rowResult.props.className).toBe('VirtualizedTraceView--row'); | ||
|
|
||
| const spanBarRow = rowResult.props.children; | ||
| expect(spanBarRow.type).toBe(SpanBarRow); | ||
| // span is now an IOtelSpan from trace.asOtelTrace() | ||
| expect(spanBarRow.props.span.spanID).toBe(trace.spans[1].spanID); | ||
| expect(spanBarRow.props.isChildrenExpanded).toBe(true); | ||
| expect(spanBarRow.props.isDetailExpanded).toBe(false); | ||
| render(<VirtualizedTraceViewImpl {...mockProps} />); | ||
| expect(screen.getByTestId('list-view')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders a SpanDetailRow when it is a detail', () => { | ||
| const { props, detailState } = expandRow(1); | ||
| instance = createTestInstance(props); | ||
|
|
||
| const rowResult = instance.renderRow('some-key', {}, 2, {}); | ||
|
|
||
| expect(rowResult.type).toBe('div'); | ||
| expect(rowResult.props.className).toBe('VirtualizedTraceView--row'); | ||
|
|
||
| const spanDetailRow = rowResult.props.children; | ||
| expect(spanDetailRow.type).toBe(SpanDetailRow); | ||
| // span is now an IOtelSpan from trace.asOtelTrace() | ||
| expect(spanDetailRow.props.span.spanID).toBe(trace.spans[1].spanID); | ||
| expect(spanDetailRow.props.detailState).toBe(detailState); | ||
| const { props } = expandRow(1); | ||
| render(<VirtualizedTraceViewImpl {...props} />); | ||
| expect(screen.getByTestId('list-view')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders a SpanBarRow with a RPC span if the row is collapsed and a client span', () => { | ||
| const clientTags = [{ key: 'span.kind', value: 'client' }, ...legacyTrace.spans[0].tags]; | ||
| const serverTags = [{ key: 'span.kind', value: 'server' }, ...legacyTrace.spans[1].tags]; | ||
|
|
||
| // Update legacy trace spans | ||
| const newLegacySpans = [...legacyTrace.spans]; | ||
| newLegacySpans[0] = { ...newLegacySpans[0], tags: clientTags }; | ||
| newLegacySpans[1] = { ...newLegacySpans[1], tags: serverTags }; | ||
|
|
||
| const newLegacyTrace = { ...legacyTrace, spans: newLegacySpans }; | ||
| const altTrace = transformTraceData(newLegacyTrace).asOtelTrace(); | ||
|
|
||
| const altTrace = transformTraceData({ ...legacyTrace, spans: newLegacySpans }).asOtelTrace(); | ||
| const childrenHiddenIDs = new Set([altTrace.spans[0].spanID]); | ||
|
|
||
| instance = createTestInstance({ | ||
| ...mockProps, | ||
| childrenHiddenIDs, | ||
| trace: altTrace, | ||
| }); | ||
|
|
||
| const rowResult = instance.renderRow('some-key', {}, 0, {}); | ||
| const spanBarRow = rowResult.props.children; | ||
|
|
||
| expect(spanBarRow.type).toBe(SpanBarRow); | ||
| expect(spanBarRow.props.rpc).toBeDefined(); | ||
| render( | ||
| <VirtualizedTraceViewImpl {...mockProps} childrenHiddenIDs={childrenHiddenIDs} trace={altTrace} /> | ||
| ); | ||
| expect(screen.getByTestId('list-view')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
The renderRow-related tests no longer validate row rendering logic (RPC span/noInstrumentedServer/detail rows, etc.). Because ListView is mocked to only render a <div> and never calls itemRenderer, these tests can pass even if renderSpanBarRow / renderSpanDetailRow break. Consider updating the ListView mock to render a small set of items by invoking props.itemRenderer(...) so assertions can cover the actual output/props.
| import SpanBarRow from './SpanBarRow'; | ||
| import DetailState from './SpanDetail/DetailState'; | ||
| import SpanDetailRow from './SpanDetailRow'; | ||
| import { DEFAULT_HEIGHTS, VirtualizedTraceViewImpl } from './VirtualizedTraceView'; | ||
| import { DEFAULT_HEIGHTS, VirtualizedTraceViewImpl, testableHelpers } from './VirtualizedTraceView'; | ||
| import traceGenerator from '../../../demo/trace-generators'; | ||
| import transformTraceData from '../../../model/transform-trace-data'; | ||
| import updateUiFindSpy from '../../../utils/update-ui-find'; | ||
| import * as linkPatterns from '../../../model/link-patterns'; | ||
| import memoizedTraceCriticalPath from '../CriticalPath/index'; | ||
|
|
||
| import getLinks from '../../../model/link-patterns'; | ||
| import criticalPathTest from '../CriticalPath/testCases/test2'; | ||
|
|
||
| jest.mock('./SpanTreeOffset'); | ||
| jest.mock('../../../utils/update-ui-find'); | ||
|
|
||
| // Mock useNavigate so focusSpan tests can assert on it | ||
| const mockNavigate = jest.fn(); | ||
| jest.mock('react-router-dom-v5-compat', () => ({ | ||
| ...jest.requireActual('react-router-dom-v5-compat'), | ||
| useNavigate: () => mockNavigate, | ||
| })); |
There was a problem hiding this comment.
There are several now-unused test imports/variables (e.g. SpanBarRow, SpanDetailRow, and mockNavigate). Cleaning these up will reduce noise and avoid misleading intent (especially since the current focusSpan test does not assert navigation/updateUiFind behavior).
Signed-off-by: Aryan Ghotekar <aryanghotekar95@gmail.com>
|
Hi @yurishkuro , Just wanted to flag a circular dependency situation: #3452 can't safely merge until #3551 is merged first So neither can progress without maintainer intervention. Could you help break this deadlock? |
Signed-off-by: Aryanghotekar <145753436+aryanG9403@users.noreply.github.com>
|
Resolved merge conflicts with main. PR is ready for review. |
Signed-off-by: Aryanghotekar <145753436+aryanG9403@users.noreply.github.com>
github-actions
Bot
removed
the
pr-quota-reached
|
PR quota unlocked! @aryanG9403, this PR has been moved out of the waiting room and into the active review queue:
Thank you for your patience. |
Which problem is this PR solving?
Description of the changes
VirtualizedTraceViewfrom a class component to a functional component using React hookstestableHelpersexport for pure helper functions used in testsrenderRowtests simplified to render-based assertions since it is now an internal callback — core logic is covered bycreateTestInstancetestsHow was this change tested?
Screenshots
Since this is a refactor-only change, there are no visual differences.
Screenshots confirm the component renders correctly after the conversion.
Checklist
AI Usage in this PR (choose one)