refactor(plexus): migrate MeasurableNodes to functional component#3533
Conversation
|
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: 10/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
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@hharshhsaini please address copilot comments |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3533 +/- ##
==========================================
+ Coverage 88.68% 88.98% +0.30%
==========================================
Files 300 300
Lines 9560 9559 -1
Branches 2445 2536 +91
==========================================
+ Hits 8478 8506 +28
+ Misses 1078 1050 -28
+ Partials 4 3 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0017638 to
521fd22
Compare
|
@jkowall @yurishkuro The Copilot feedback on the default export double type assertion has been resolved. The signature has been casted directly to The PR is fully ready for final review and merge. |
|
Thanks for the contribution @hharshhsaini! The refactoring logic looks correct — the
|
521fd22 to
e0743ba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Convert class component to functional component using React hooks - Replace React.Component with React.memo() - Convert shouldComponentUpdate to custom areEqual comparison function - Destructure props directly in function signature - Preserve all functionality including custom update logic - Maintain generic type support with proper type casting VALIDATION RESULTS: ✅ All plexus package tests passing (18 tests) ✅ Build successful ✅ Code formatted with Prettier ✅ No visual regression ✅ Performance maintained with React.memo MIGRATION DETAILS: - State: None (stateless component) - Lifecycle methods: shouldComponentUpdate → areEqual comparator - Memoization: Custom comparison using isSamePropSetter - Generic types: Preserved with type assertion - Lines changed: 36 insertions, 41 deletions (minimal, focused) Part of jaegertracing#2610 (Jaeger UI modernization - refactor class components) Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
e0743ba to
51f4020
Compare
|
@yurishkuro @jkowall The The misleading comment about |
Which problem is this PR solving?
Description of the changes
MeasurableNodescomponent from a class-based component to a functional component withReact.memo()MeasurableNodesclass component extendingReact.Componentto a functional componentshouldComponentUpdateto customareEqualcomparison function forReact.memo()render()method and converted to direct return statementas unknown as <T = {}>(props: TProps<T>) => React.ReactElement[]isSamePropSetterHow was this change tested?
Test proof -
Before
After
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.