test(plexus): add zoom utils coverage#3716
Conversation
Add unit tests for the plexus zoom helpers and fold in the follow-up fixes from automated review feedback. The updated tests cover the exported scale bounds, avoid brittle referential assertions, and explicitly verify null handling for the zoom formatting helpers. Signed-off-by: Jonah Kowall <jkowall@kowall.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3716 +/- ##
==========================================
+ Coverage 89.22% 89.90% +0.67%
==========================================
Files 331 327 -4
Lines 9970 9845 -125
Branches 2591 2569 -22
==========================================
- Hits 8896 8851 -45
+ Misses 931 861 -70
+ Partials 143 133 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkowall
added
the
changelog:test
There was a problem hiding this comment.
Pull request overview
Adds Jest unit coverage for the Plexus zoom helper utilities and aligns TypeScript signatures with existing null-safe runtime behavior.
Changes:
- Add
packages/plexus/src/zoom/utils.test.tsto cover scale extent bounds, fit/centering, constrain clamping, and formatting helpers. - Widen
getZoomStyle()/getZoomAttr()(andZoomManagerwrappers) to acceptnullin addition toundefined.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/plexus/src/zoom/ZoomManager.ts | Widens wrapper method parameter types to accept null transforms. |
| packages/plexus/src/zoom/utils.ts | Widens helper function parameter types to accept null transforms; keeps behavior unchanged. |
| packages/plexus/src/zoom/utils.test.ts | New Jest test coverage for zoom scale/fit/constrain and formatting helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove nullable parameters from getZoomAttr and getZoomStyle since all callers always pass a ZoomTransform. Update test mocks to provide zoomTransform: zoomIdentity and restrict vitest to src/ directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getZoomStyle(transform: ZoomTransform): React.CSSProperties { | ||
| const { x, y, k } = transform; | ||
| const rv: React.CSSProperties = { | ||
| return { | ||
| transform: `translate(${x.toFixed()}px, ${y.toFixed()}px) scale(${k})`, | ||
| transformOrigin: '0 0', | ||
| }; |
There was a problem hiding this comment.
The previous implementation of getZoomStyle() handled null/undefined transforms by returning a default (identity) style. This change removes that guard and will now throw at runtime if any caller passes a missing transform (and it also contradicts the PR description about explicitly accepting null). Consider restoring the null-safe behavior (and typing the param as ZoomTransform | null | undefined) or ensure all call sites always pass zoomIdentity and update the PR description/API docs accordingly.
| if (!transform) { | ||
| return undefined; | ||
| } | ||
| export function getZoomAttr(transform: ZoomTransform): string { |
There was a problem hiding this comment.
getZoomAttr() used to return undefined when transform was falsy, which is safe to pass through to the SVG transform attribute. It now unconditionally formats x/y/k and will throw if transform is null/undefined, so this is a runtime behavior change and potential breaking change for any deep-import consumers. Consider reintroducing the null/undefined guard (and reflect it in the return type) or explicitly guarantee a non-null transform everywhere this is called.
| export function getZoomAttr(transform: ZoomTransform): string { | |
| export function getZoomAttr(transform: ZoomTransform | null | undefined): string | undefined { | |
| if (!transform) { | |
| return undefined; | |
| } |
| static getZoomAttr(zoomTransform: ZoomTransform): string { | ||
| return getZoomAttr(zoomTransform); | ||
| } | ||
|
|
||
| static getZoomStyle(zoomTransform: ZoomTransform | void): React.CSSProperties { | ||
| static getZoomStyle(zoomTransform: ZoomTransform): React.CSSProperties { | ||
| return getZoomStyle(zoomTransform); |
There was a problem hiding this comment.
These wrappers were changed from accepting an optional transform (ZoomTransform | void) to requiring a ZoomTransform. Since ZoomManager is a public-ish API (deep import) and getZoomAttr/getZoomStyle previously tolerated missing transforms, this is a potentially breaking change and also conflicts with the PR description about explicitly accepting null. If the intent is to stay backward compatible, keep the parameter optional/nullable and preserve the null-safe fallback behavior in utils.
| describe('getZoomStyle', () => { | ||
| it('returns CSS properties for identity transform', () => { | ||
| expect(getZoomStyle(zoomIdentity)).toEqual({ | ||
| transform: 'translate(0px, 0px) scale(1)', | ||
| transformOrigin: '0 0', | ||
| }); |
There was a problem hiding this comment.
The new tests cover only non-null transforms for getZoomStyle() / getZoomAttr(). Given the prior behavior (and PR description) around explicitly handling null/undefined, add explicit test cases for missing transforms (or, if missing transforms are no longer supported, update the PR description and consider adding a negative test/assertion so the behavior change is intentional and documented).
Summary
packages/plexus/src/zoom/utils.ts, including scale bounds, centering, constraint clamping, and zoom formatting helpersDEFAULT_SCALE_EXTENTin the bound checksgetZoomStyle()/getZoomAttr()andZoomManagerstrictly require typed argument and fix tests to pass oneWhy
The plexus zoom helpers did not have direct unit coverage, which left the zoom math and formatting behavior unguarded. While addressing that gap, automated review feedback surfaced a few follow-up fixes in the new tests: one brittle identity assertion, missing explicit
nullcases, and a type mismatch where the implementation already handlednullbut the TypeScript signatures did not.Impact
This change increases confidence in the zoom utility behavior without changing the underlying zoom math. The only source change outside the tests is the type widening for the formatting helpers so the types match the existing null-safe implementation.
Validation
npm run fmtnpm test -- src/zoom/utils.test.tsnpm run buildnpm run lintcurrently fails on unrelated existing 2026 copyright-header issues inpackages/jaeger-ui/src/components/TraceDiff/duck.test.js,packages/jaeger-ui/src/components/TraceDiff/duck.ts,packages/jaeger-ui/src/components/TracePage/ArchiveNotifier/duck.test.js, andpackages/jaeger-ui/src/components/TracePage/ArchiveNotifier/duck.tsnpm testcurrently fails on an unrelated existing missing-module issue inpackages/jaeger-ui/src/utils/zustand-class-bridge.test.tsx(Cannot find module 'zustand')