Convert SvgEdges to functional component by thc1006 · Pull Request #3402 · jaegertracing/jaeger-ui

thc1006 · 2026-01-14T19:13:14Z

Which problem is this PR solving?

Resolves #3397

Description of the changes

Converted the SvgEdges class component to a functional component using React.memo with custom comparator.

How was this change tested?

Added unit tests for SvgEdges component
Run npm run test in packages/plexus directory

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger-ui: npm run lint and npm run test

Add test coverage for SvgEdges component before refactoring to functional component. Tests cover: - Basic rendering with multiple edges - Empty layoutEdges array handling - Props propagation to child SvgEdge components - Optional marker props handling Part of jaegertracing#2610 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Copilot

Pull request overview

This PR converts the SvgEdges component from a class-based to a functional component using React.memo with a custom comparison function to maintain the original shouldComponentUpdate optimization behavior.

Changes:

Converted SvgEdges from class component to functional component with React.memo
Implemented arePropsEqual comparator function that mirrors the original shouldComponentUpdate logic
Added comprehensive unit tests achieving 100% test coverage

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
packages/plexus/src/Digraph/SvgEdges.tsx	Refactored component from class to functional with React.memo, preserving optimization logic
packages/plexus/src/Digraph/SvgEdges.test.js	Added new test file with comprehensive test cases for the component

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Convert SvgEdges from class-based to functional component using React.memo with custom comparator to preserve the original shouldComponentUpdate behavior. Resolves jaegertracing#3397 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

yurishkuro · 2026-01-19T22:53:58Z

unit tests are failing

Move mockSvgEdgeProps array clearing to beforeEach hook to ensure consistent test isolation regardless of test execution order. This prevents potential flaky test behavior in CI environments. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-20T05:33:41Z

packages/plexus/src/Digraph/SvgEdges.test.js

+describe('SvgEdges', () => {
+  beforeEach(() => {
+    mockSvgEdgeProps.length = 0;
+  });
+
+  const mockGetClassName = name => `plexus--${name}`;
+
+  const mockRenderUtils = {
+    getGlobalId: id => `global-${id}`,
+    getZoomTransform: () => ({ k: 1, x: 0, y: 0 }),
+  };
+
+  // Create mock edges for testing
+  const createMockEdge = (from, to) => ({
+    edge: { from, to },
+    pathPoints: [
+      [0, 0],
+      [100, 100],
+    ],
+  });
+
+  const mockLayoutEdges = [createMockEdge('node-a', 'node-b'), createMockEdge('node-b', 'node-c')];
+
+  // Wrap in svg container since SvgEdge renders SVG elements
+  const renderInSvg = edges => {
+    return render(
+      <svg>
+        <SvgEdges getClassName={mockGetClassName} layoutEdges={edges} renderUtils={mockRenderUtils} />
+      </svg>
+    );
+  };
+
+  it('renders SvgEdge for each layout edge', () => {
+    const { getAllByTestId } = renderInSvg(mockLayoutEdges);
+    const edges = getAllByTestId('svg-edge');
+    expect(edges).toHaveLength(2);
+  });
+
+  it('renders nothing when layoutEdges is empty', () => {
+    const { queryAllByTestId } = renderInSvg([]);
+    const edges = queryAllByTestId('svg-edge');
+    expect(edges).toHaveLength(0);
+  });
+
+  it('passes from and to correctly to SvgEdge', () => {
+    const { getAllByTestId } = renderInSvg(mockLayoutEdges);
+    const edges = getAllByTestId('svg-edge');
+
+    // Check first edge
+    expect(edges[0]).toHaveAttribute('data-from', 'node-a');
+    expect(edges[0]).toHaveAttribute('data-to', 'node-b');
+
+    // Check second edge
+    expect(edges[1]).toHaveAttribute('data-from', 'node-b');
+    expect(edges[1]).toHaveAttribute('data-to', 'node-c');
+  });
+
+  it('handles optional marker props', () => {
+    const { getAllByTestId } = render(
+      <svg>
+        <SvgEdges
+          getClassName={mockGetClassName}
+          layoutEdges={mockLayoutEdges}
+          renderUtils={mockRenderUtils}
+          markerEndId="arrow-end"
+          markerStartId="arrow-start"
+        />
+      </svg>
+    );
+    expect(getAllByTestId('svg-edge')).toHaveLength(2);
+  });
+
+  it('passes setOnEdge to child components', () => {
+    const mockSetOnEdge = jest.fn();
+    const { getAllByTestId } = render(
+      <svg>
+        <SvgEdges
+          getClassName={mockGetClassName}
+          layoutEdges={mockLayoutEdges}
+          renderUtils={mockRenderUtils}
+          setOnEdge={mockSetOnEdge}
+        />
+      </svg>
+    );
+    expect(getAllByTestId('svg-edge')).toHaveLength(2);
+
+    // Verify setOnEdge is passed to each child component
+    mockSvgEdgeProps.forEach(props => {
+      expect(props.setOnEdge).toBe(mockSetOnEdge);
+    });
+  });
+});


The tests do not verify the memoization behavior of the React.memo wrapper. Since the conversion from a class component with shouldComponentUpdate to a functional component with React.memo is a key aspect of this PR, consider adding tests that verify the component does not re-render when props haven't changed according to the arePropsEqual logic. This could involve using the rerender function from testing-library and tracking render counts.

Add tests to verify React.memo memoization works correctly: - Component does not re-render when same props are passed - Component re-renders when layoutEdges changes - Component re-renders when markerEndId changes This addresses AI review feedback to ensure the class-to-functional component conversion preserves the original shouldComponentUpdate optimization behavior. Also improves SvgEdges.tsx coverage to 100%. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

thc1006 · 2026-01-20T13:05:25Z

unit tests are failing

Done!

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/plexus/src/Digraph/SvgEdges.test.js

Add test cases for all props compared in arePropsEqual: - markerStartId changes - getClassName changes - renderUtils changes - setOnEdge changes This addresses Copilot feedback requesting complete coverage of the memoization logic for all props, not just layoutEdges and markerEndId. Signed-off-by: thc1006 <109750520+thc1006@users.noreply.github.com> Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Restore the original Uber Technologies copyright in SvgEdges.tsx and use consistent Uber copyright in test file to match other plexus package files. Per maintainer guidance: "never remove copyright, but OK to add Jaeger copyright on top" (PR jaegertracing#3407) Signed-off-by: thc1006 <109750520+thc1006@users.noreply.github.com> Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/plexus/src/Digraph/SvgEdges.test.js

thc1006 · 2026-01-21T10:03:04Z

Dear @yurishkuro,

Following up on the previous fixes, I have addressed the additional feedback:

Added complete memoization test coverage for all props in arePropsEqual (getClassName, renderUtils, markerStartId, setOnEdge)
Restored the original Uber copyright header in SvgEdges.tsx to maintain consistency with other plexus files
Updated test file copyright to match the plexus package convention

All 21 tests pass with 100% coverage for SvgEdges.tsx.

Regarding the latest Copilot suggestion about testing setOnEdge as an array: the current tests verify memoization behavior for function references, which covers the primary use case. The array comparison logic is an implementation detail of isSamePropSetter utility. I can add those tests if you think they are necessary for this PR.

Ready for review when you have a chance. Thank you.

github-actions · 2026-03-23T02:17:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

yurishkuro · 2026-03-23T17:44:54Z

packages/plexus/src/Digraph/SvgEdges.tsx

-      />
-    ));
-  }
+// Comparison function that mirrors the original shouldComponentUpdate logic


Why can't we simply rely on React doing the comparison? That is literally the reason why React was created.

Sorry for the delayed reply, I was at KubeCon EU these past two weeks.

Thank you for the review. You're right, the custom comparator shouldn't be here. I raised the same point on #3403 and offered to simplify, but should have just gone ahead and done it. The isSamePropSetter call on setOnEdge is handling something the caller should stabilize with useMemo, not something the child should work around.

I'll switch to plain React.memo() here and on #3403. Both need a rebase against the toolchain changes anyway, so I'll handle them together.

thc1006 · 2026-04-02T06:09:24Z

Apologies for the back and forth here. After taking a closer look at main, I realized SvgEdges.tsx was already converted to a functional component in #3518 by @hharshhsaini. I should have checked before replying to the arePropsEqual comment earlier.

Thank you for the feedback regardless, it clarified the right approach for Nodes (#3403) as well.

One thing that might still have value: this branch includes a 326-line test file for SvgEdges covering rendering, memoization behavior, and edge cases. There don't appear to be tests for this component on main yet.

Would a standalone test PR (migrated to Vitest) be welcome? Happy to put one together if so.

thc1006 requested a review from a team as a code owner January 14, 2026 19:13

thc1006 requested review from Copilot and joe-elliott and removed request for a team January 14, 2026 19:13

Copilot started reviewing on behalf of thc1006 January 14, 2026 19:13 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

thc1006 force-pushed the feat/svg-edges-functional branch from 96c0e7c to 076e27d Compare January 14, 2026 19:29

thc1006 force-pushed the feat/svg-edges-functional branch from 076e27d to 49b9cdf Compare January 14, 2026 19:39

jkowall added the changelog:refactoring Internal, non-functional code improvements label Jan 14, 2026

github-actions bot added the waiting-for-author PR is waiting for author to respond to maintainer's comments label Jan 19, 2026

Copilot AI review requested due to automatic review settings January 20, 2026 05:31

github-actions bot removed the waiting-for-author PR is waiting for author to respond to maintainer's comments label Jan 20, 2026

Copilot started reviewing on behalf of thc1006 January 20, 2026 05:32 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

Merge branch 'main' into feat/svg-edges-functional

16917ef

Copilot AI review requested due to automatic review settings January 20, 2026 13:05

Copilot started reviewing on behalf of thc1006 January 20, 2026 13:05 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

packages/plexus/src/Digraph/SvgEdges.test.js Show resolved Hide resolved

thc1006 added 2 commits January 21, 2026 17:18

Copilot AI review requested due to automatic review settings January 21, 2026 09:55

Copilot started reviewing on behalf of thc1006 January 21, 2026 09:56 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

packages/plexus/src/Digraph/SvgEdges.test.js Show resolved Hide resolved

thc1006 mentioned this pull request Feb 13, 2026

refactor(plexus): convert SvgEdges from class to functional component #3422

Closed

github-actions bot added the stale label Mar 23, 2026

yurishkuro removed the stale label Mar 23, 2026

yurishkuro reviewed Mar 23, 2026

View reviewed changes

thc1006 closed this Apr 2, 2026

thc1006 mentioned this pull request Apr 2, 2026

refactor(plexus): convert SvgDefEntry from class to functional component #3420

Closed

thc1006 deleted the feat/svg-edges-functional branch April 2, 2026 14:27

Conversation

thc1006 commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

yurishkuro commented Jan 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

thc1006 commented Jan 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

thc1006 commented Jan 21, 2026

Uh oh!

github-actions bot commented Mar 23, 2026

Uh oh!

yurishkuro Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

thc1006 Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

thc1006 commented Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thc1006 commented Jan 14, 2026 •

edited

Loading