refactor(plexus): migrate Digraph to functional component#3534
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 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@hharshhsaini please address copilot comments. |
1fa6d95 to
7329a91
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3534 +/- ##
==========================================
+ Coverage 88.68% 90.78% +2.10%
==========================================
Files 300 304 +4
Lines 9560 9731 +171
Branches 2445 2595 +150
==========================================
+ Hits 8478 8834 +356
+ Misses 1078 894 -184
+ Partials 4 3 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
711fdc3 to
c8d25fc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c8d25fc to
cd464ee
Compare
cd464ee to
9c03659
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Convert class component to functional component with hooks - Replace this.state with useState for state management - Convert componentDidMount to useEffect hook - Replace memoizeOne with useMemo for className factory - Convert instance variables to useRef (baseId, rootRef, zoomManager) - Maintain PureComponent behavior with React.memo - Preserve all static properties (propsFactories, scaleProperty) - All existing tests pass without modification Resolves jaegertracing#3386 Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
|
@jkowall Fixed, Added a public |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…anager - Use never-resolving promise in mockLayoutManager to avoid React 'not wrapped in act()' warnings from async state updates in tests - Assert .plexus-MiniMap element instead of generic svg selector to make MiniMap test specific and avoid false positives - Reset contentSize and currentTransform in ZoomManager.dispose() so instance is fully returned to initial state after teardown - Fix misleading renderLayers comment: renderUtils is stable (memoized); the real reason no useCallback is needed is that state changes on every layout phase update and zoom event Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
|
@jkowall addressed and resolved all the github sugestions , with coments and fixes. |
|
@hharshhsaini why are you changing the lockfile? |
Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
@jkowall Sorry about that, it was an unintentional artifact from running npm install locally. Restored package-lock.json to match upstream main. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 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.
Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
|
@hharshhsaini been there before, will approve. |
jkowall
left a comment
There was a problem hiding this comment.
Looks good, thanks for the contribution.
…egertracing#3534)" This reverts commit 5114ee3. Signed-off-by: Yuri Shkuro <github@ysh.us>
|
reverting #3600 |
…3600) This reverts commit 5114ee3. Code Review: PR #3534 — `refactor(plexus): migrate Digraph to functional component` **Status: MERGED** | 1,053 additions, 531 deletions across 5 files --- ## Overview This PR migrates `packages/plexus/src/Digraph/index.tsx` from a class component to a functional component, as part of the broader React modernization effort (#2610). It also adds a `dispose()` method to `ZoomManager` and introduces a new test file. The general direction is correct and the migration is largely sound. --- ## Positive Aspects - **`dispose()` on `ZoomManager`** is a valuable addition — it properly removes d3-zoom event listeners to prevent leaks when zoom is toggled or the component unmounts. - **`builtinStyle` based on `zoomEnabled` prop** instead of checking `zoomManagerRef.current` eliminates the initial paint/layout shift bug present in the original design. - **Single atomic `setState`** in `setSizeVertices` is an improvement over the original class component which had two `setState` calls (first setting `sizeVertices` alone, then redundantly re-setting it alongside `layoutPhase`). - **Lazy ref initialization** for `baseId` correctly gates the `idCounter++` side-effect behind an `if (!baseIdRef.current)` guard. - **`zoomTransformRef` pattern** for `getZoomTransform` is the right approach — stable callback via `[]` deps that always returns the latest transform. --- ## Issues ### 1. `setSizeVertices` stability regression (potential performance issue) ```tsx // packages/plexus/src/Digraph/index.tsx const setSizeVertices = React.useCallback( (senderKey: string, sizeVertices: TSizeVertex<T>[]) => { ... }, [edges, layoutManager, measurableNodesKey, onLayoutDone] // recreated on new edges prop ); ``` In the original class component, `setSizeVertices` was a stable method reference — it never changed identity. Now it is recreated whenever `edges` or `layoutManager` changes. This means `MeasurableNodesLayer` (which receives `setSizeVertices` as a prop) will re-render on every parent re-render that produces a new `edges` array reference, even if the edge data hasn't changed. Consider using a ref-based approach like `onLayoutDone` does — capture `edges` and `layoutManager` in a ref updated each render, and give `setSizeVertices` `[]` deps. ### 2. `onLayoutDone` empty dep array is correct but undocumented ```tsx const onLayoutDone = React.useCallback((result: ...) => { ... if (zoomManagerRef.current) { zoomManagerRef.current.setContentSize(layoutGraph); } }, []); // empty deps ``` This is correct because `setState` is stable and `zoomManagerRef` is a ref. However, there is no comment explaining why empty deps are appropriate here, which can look like a mistake to future readers. ### 3. Generic type erasure with `React.memo` ```tsx const DigraphMemo = React.memo(Digraph) as unknown as TDigraphComponent; ``` The `as unknown as TDigraphComponent` cast drops the generic `<T, U>` parameters. Downstream callers using `<Digraph<MyNode, MyEdge> .../>` lose type safety. The comment in the PR acknowledges this is a known React limitation — that is fine for now, but it should be tracked as a follow-up since it is a type regression from the original. ### 4. Redundant `zoomManagerRef` ```tsx const zoomManager = React.useMemo(() => { ... }, [zoomEnabled]); const zoomManagerRef = React.useRef<ZoomManager | null>(zoomManager); zoomManagerRef.current = zoomManager; ``` `zoomManagerRef` is only used in `onLayoutDone` (to avoid stale closure with `[]` deps). The pattern works, but mutating a ref during render (`zoomManagerRef.current = zoomManager`) is technically a side effect in the render phase. Since `zoomManager` comes from `useMemo`, this is benign in practice, but a comment would help clarify the intent. ### 5. Unrelated dependency updates in `package-lock.json` The PR mixes the functional component refactor with `package-lock.json` updates (`@typescript-eslint` 8.50→8.56, various `@rc-component/*` bumps, rollup 4.53→4.59, etc.). These lockfile-only changes should be in a separate PR for easier review and bisectability. ### 6. New test file: copyright header inconsistency ```tsx // Copyright (c) 2026 Uber Technologies, Inc. ``` Other plexus source files use `Uber Technologies, Inc.` (consistent with the original 2019 header), but `CLAUDE.md` specifies `The Jaeger Authors.` as the expected copyright holder for new files. This should be reconciled. ### 7. Test: `getLayout` assertion has edge-case fragility ```tsx // index.test.tsx expect(getLayout).toHaveBeenCalledWith(edges, expect.any(Array)); ``` `getLayout` is called inside `setSizeVertices`, which is triggered by `MeasurableNodesLayer` measuring nodes. The test relies on `measureNode: () => ({ height: 10, width: 10 })` being called synchronously during render. This works today but is implementation-dependent. A comment explaining why this fires synchronously (RTL flushes sync measurement in the DOM render) would help future maintainers. --- ## Summary The migration is functionally correct and the tests pass. The main concerns are: | Priority | Issue | | --------- | ------------------------------------------------------------------------ | | Medium | `setSizeVertices` recreated on `edges` change — potential render churn | | Low | Generic type erasure via `React.memo` cast (needs follow-up tracking) | | Low | `package-lock.json` changes mixed into refactor PR | | Cosmetic | Copyright header in new test file | | Cosmetic | Missing comments explaining empty `[]` dep arrays | Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro sir , may I know the reasons for reverting the PRs , while completing all the sugestions from both the maintainers @jkowall @yurishkuro . Its been confusing for performing the different different changes again and again. |
|
There is a summary in the reverting PR. This should not have been merged with existing issues. |
…ing#3534) ## Which problem is this PR solving? - Resolves jaegertracing#3386 - Part of jaegertracing#2610 - Jaeger UI modernization (refactor class components to functional components) ## Description of the changes - Migrated Digraph component from class-based to functional component - Converted this.state to useState for state management - Replaced componentDidMount lifecycle method with useEffect hook - Converted memoizeOne to useMemo for className factory memoization - Converted instance variables (baseId, rootRef, zoomManager) to useRef - Maintained PureComponent behavior with React.memo wrapper - Preserved all static properties (propsFactories, scaleProperty) - All existing tests pass without modification ## Component Migration Details - [x] Converted class component to functional component - [x] Replaced this.state with useState (8 state variables) - [x] Converted componentDidMount to useEffect - [x] Replaced memoizeOne with useMemo - [x] Converted instance variables to useRef - [x] Wrapped component with React.memo for performance - [x] Preserved static properties on memoized component ## State Management - [x] edges - array of graph edges - [x] layoutEdges - processed layout edges - [x] layoutGraph - graph layout data - [x] layoutPhase - current layout phase - [x] layoutVertices - processed layout vertices - [x] sizeVertices - vertex size data - [x] vertices - array of graph vertices - [x] zoomTransform - zoom transformation state ## Hooks Migration - [x] useState for component state - [x] useEffect for lifecycle (componentDidMount) - [x] useRef for instance variables (baseId, rootRef, zoomManager) - [x] useMemo for memoized values (className factory, renderUtils) - [x] useCallback for callbacks (getGlobalId, getZoomTransform, onLayoutDone, setSizeVertices, renderLayers) ## How was this change tested? - [x] Ran npm run prettier - formatting correct - [x] Ran npm run tsc-lint - TypeScript type checking passing (no new errors) - [x] Ran npm run lint - passing (249 pre-existing warnings, 0 errors) - [x] Ran npm test -- packages/plexus - all 18 tests passing - [x] Ran npm run build --workspace=@jaegertracing/plexus - build successful ## Test proof - <img width="428" height="147" alt="3386 proof" src="https://github.com/user-attachments/assets/d55cd1e2-5177-40da-89f2-da3a3303be0a" /> ## Code Quality - [x] Minimal changes - only 4 files modified - [x] 348 insertions, 195 deletions (net +153 lines) - [x] All hooks have correct dependency arrays - [x] No stale closures or infinite loops - [x] Performance maintained with React.memo ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality (N/A - migration maintains existing functionality) - [x] I have run lint and test steps successfully: - npm run prettier - npm run lint - npm run tsc-lint - npm test - npm run build ## AI Usage in this PR See AI Usage Policy. - [ ] None: No AI tools were used in creating this PR - [x] Light: AI provided minor assistance (formatting, simple suggestions) - [ ] Moderate: AI helped with code generation or debugging specific parts - [ ] Heavy: AI generated most or all of the code changes --------- Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
…aegertracing#3600) This reverts commit 5114ee3. Code Review: PR jaegertracing#3534 — `refactor(plexus): migrate Digraph to functional component` **Status: MERGED** | 1,053 additions, 531 deletions across 5 files --- ## Overview This PR migrates `packages/plexus/src/Digraph/index.tsx` from a class component to a functional component, as part of the broader React modernization effort (jaegertracing#2610). It also adds a `dispose()` method to `ZoomManager` and introduces a new test file. The general direction is correct and the migration is largely sound. --- ## Positive Aspects - **`dispose()` on `ZoomManager`** is a valuable addition — it properly removes d3-zoom event listeners to prevent leaks when zoom is toggled or the component unmounts. - **`builtinStyle` based on `zoomEnabled` prop** instead of checking `zoomManagerRef.current` eliminates the initial paint/layout shift bug present in the original design. - **Single atomic `setState`** in `setSizeVertices` is an improvement over the original class component which had two `setState` calls (first setting `sizeVertices` alone, then redundantly re-setting it alongside `layoutPhase`). - **Lazy ref initialization** for `baseId` correctly gates the `idCounter++` side-effect behind an `if (!baseIdRef.current)` guard. - **`zoomTransformRef` pattern** for `getZoomTransform` is the right approach — stable callback via `[]` deps that always returns the latest transform. --- ## Issues ### 1. `setSizeVertices` stability regression (potential performance issue) ```tsx // packages/plexus/src/Digraph/index.tsx const setSizeVertices = React.useCallback( (senderKey: string, sizeVertices: TSizeVertex<T>[]) => { ... }, [edges, layoutManager, measurableNodesKey, onLayoutDone] // recreated on new edges prop ); ``` In the original class component, `setSizeVertices` was a stable method reference — it never changed identity. Now it is recreated whenever `edges` or `layoutManager` changes. This means `MeasurableNodesLayer` (which receives `setSizeVertices` as a prop) will re-render on every parent re-render that produces a new `edges` array reference, even if the edge data hasn't changed. Consider using a ref-based approach like `onLayoutDone` does — capture `edges` and `layoutManager` in a ref updated each render, and give `setSizeVertices` `[]` deps. ### 2. `onLayoutDone` empty dep array is correct but undocumented ```tsx const onLayoutDone = React.useCallback((result: ...) => { ... if (zoomManagerRef.current) { zoomManagerRef.current.setContentSize(layoutGraph); } }, []); // empty deps ``` This is correct because `setState` is stable and `zoomManagerRef` is a ref. However, there is no comment explaining why empty deps are appropriate here, which can look like a mistake to future readers. ### 3. Generic type erasure with `React.memo` ```tsx const DigraphMemo = React.memo(Digraph) as unknown as TDigraphComponent; ``` The `as unknown as TDigraphComponent` cast drops the generic `<T, U>` parameters. Downstream callers using `<Digraph<MyNode, MyEdge> .../>` lose type safety. The comment in the PR acknowledges this is a known React limitation — that is fine for now, but it should be tracked as a follow-up since it is a type regression from the original. ### 4. Redundant `zoomManagerRef` ```tsx const zoomManager = React.useMemo(() => { ... }, [zoomEnabled]); const zoomManagerRef = React.useRef<ZoomManager | null>(zoomManager); zoomManagerRef.current = zoomManager; ``` `zoomManagerRef` is only used in `onLayoutDone` (to avoid stale closure with `[]` deps). The pattern works, but mutating a ref during render (`zoomManagerRef.current = zoomManager`) is technically a side effect in the render phase. Since `zoomManager` comes from `useMemo`, this is benign in practice, but a comment would help clarify the intent. ### 5. Unrelated dependency updates in `package-lock.json` The PR mixes the functional component refactor with `package-lock.json` updates (`@typescript-eslint` 8.50→8.56, various `@rc-component/*` bumps, rollup 4.53→4.59, etc.). These lockfile-only changes should be in a separate PR for easier review and bisectability. ### 6. New test file: copyright header inconsistency ```tsx // Copyright (c) 2026 Uber Technologies, Inc. ``` Other plexus source files use `Uber Technologies, Inc.` (consistent with the original 2019 header), but `CLAUDE.md` specifies `The Jaeger Authors.` as the expected copyright holder for new files. This should be reconciled. ### 7. Test: `getLayout` assertion has edge-case fragility ```tsx // index.test.tsx expect(getLayout).toHaveBeenCalledWith(edges, expect.any(Array)); ``` `getLayout` is called inside `setSizeVertices`, which is triggered by `MeasurableNodesLayer` measuring nodes. The test relies on `measureNode: () => ({ height: 10, width: 10 })` being called synchronously during render. This works today but is implementation-dependent. A comment explaining why this fires synchronously (RTL flushes sync measurement in the DOM render) would help future maintainers. --- ## Summary The migration is functionally correct and the tests pass. The main concerns are: | Priority | Issue | | --------- | ------------------------------------------------------------------------ | | Medium | `setSizeVertices` recreated on `edges` change — potential render churn | | Low | Generic type erasure via `React.memo` cast (needs follow-up tracking) | | Low | `package-lock.json` changes mixed into refactor PR | | Cosmetic | Copyright header in new test file | | Cosmetic | Missing comments explaining empty `[]` dep arrays | Signed-off-by: Yuri Shkuro <github@ysh.us>
Which problem is this PR solving?
Description of the changes
Component Migration Details
State Management
Hooks Migration
How was this change tested?
Test proof -
Code Quality
Checklist
AI Usage in this PR
See AI Usage Policy.