Revert "refactor(plexus): migrate Digraph to functional component"

[yurishkuro]

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)

// 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

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

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

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

// 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

// 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

[codecov]

Codecov Report

❌ Patch coverage is 0% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.13%. Comparing base (f0a387a) to head (63d2ed4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/plexus/src/Digraph/index.tsx	0.00%	53 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3600      +/-   ##
==========================================
- Coverage   90.78%   89.13%   -1.65%     
==========================================
  Files         304      304              
  Lines        9731     9712      -19     
  Branches     2564     2505      -59     
==========================================
- Hits         8834     8657     -177     
- Misses        894     1052     +158     
  Partials        3        3              

☔ 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.

[Copilot]

Pull request overview

This PR reverts the previously-merged refactor that migrated plexus’s Digraph from a class component to a functional component, returning the implementation to a React.PureComponent-based class and removing the associated zoom cleanup API and tests.

Changes:

• Reintroduces class-based Digraph implementation (including memoize-one className factory).
• Removes ZoomManager.dispose() and the Digraph test file added with the functional refactor.
• Adds memoize-one as a direct dependency of @jaegertracing/plexus.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
packages/plexus/src/zoom/ZoomManager.ts	Removes the dispose() API that detached d3-zoom listeners.
packages/plexus/src/Digraph/index.tsx	Reverts Digraph back to a React.PureComponent class implementation.
packages/plexus/src/Digraph/index.test.tsx	Deletes the Digraph test suite introduced in the reverted refactor.
packages/plexus/package.json	Adds memoize-one dependency needed by the restored class implementation.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[graphite-app]

Typo: 'recieved' should be 'received'

The error message contains a spelling error. The functional component version correctly had 'received' (line 168-169 of deleted code).

Fix:

const values = `expected ${JSON.stringify(expectedKey)}, received ${JSON.stringify(senderKey)}`;

Suggested change

	const values = `expected ${JSON.stringify(expectedKey)}, recieved ${JSON.stringify(senderKey)}`;
	const values = `expected ${JSON.stringify(expectedKey)}, received ${JSON.stringify(senderKey)}`;

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.