upcoming: [DI-29393] : Utils and Hooks set up for supporting zoom in inside the charts in CloudPulse graphs#13308
Conversation
|
Team, Around 355 lines are UT's |
… the charts in `CloudPulse metrics graphs`
|
@pmakode-akamai , can we have a look at this? |
There was a problem hiding this comment.
Pull request overview
This PR introduces infrastructure for zoom-in functionality in CloudPulse metrics graphs through new utilities and a custom React hook. The changes enable users to click and drag on chart timestamps to zoom into specific time ranges, with support for dynamic data filtering and legend recalculation.
Changes:
- Added
useZoomControllerhook to manage drag-to-zoom state and mouse event handlers - Created
CloudPulseZoomInUtilswith functions for filtering data, recalculating legend metrics, and computing aggregated values based on zoom state - Comprehensive unit tests for both the hook and utility functions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/manager/src/features/CloudPulse/Widget/components/useZoomController.ts | Implements zoom state management and mouse event handlers for drag-to-zoom |
| packages/manager/src/features/CloudPulse/Widget/components/useZoomController.test.ts | Unit tests covering zoom controller behavior including drag handling and state reset |
| packages/manager/src/features/CloudPulse/Utils/CloudPulseZoomInUtils.ts | Utility functions for filtering data by zoom range and recalculating metrics |
| packages/manager/src/features/CloudPulse/Utils/CloudPulseZoomInUtils.test.ts | Unit tests for zoom utilities covering edge cases and metric calculations |
| packages/manager/.changeset/pr-13308-upcoming-features-1769081320065.md | Changeset documenting the new zoom functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const data = [10, 20, NaN]; | ||
| const result = getMetricsFromDimensionData(data); | ||
|
|
||
| expect(result.last).toBe(0); |
There was a problem hiding this comment.
The test expects last to be 0 when the last value is NaN, but according to the implementation in getMetricsFromDimensionData (line 138 of CloudPulseZoomInUtils.ts), last is assigned data[length - 1] || 0. Since NaN || 0 evaluates to 0, the test is correct. However, this behavior is inconsistent with how NaN values are handled in sum and max calculations (where they are skipped). Consider updating getMetricsFromDimensionData to filter out NaN values when determining the last value, or document this edge case behavior explicitly.
| it('should ignore NaN values', () => { | ||
| const data = [10, NaN, 30, NaN, 50]; | ||
| const result = getMetricsFromDimensionData(data); | ||
| expect(result.total).toBe(90); |
There was a problem hiding this comment.
The test checks total and max but does not verify average and length when NaN values are present. The current implementation includes NaN values in the length calculation (line 133), which would make the average calculation include them in the denominator despite excluding them from the sum. Add assertions for average and length to ensure the behavior matches expectations.
pmakode-akamai
left a comment
There was a problem hiding this comment.
I can see the tests are passing but I noticed some edge cases and potential issues, and left comments for them that you might want to take a look at. We could consider adding additional test coverage for those scenarios if possible
| return legendRows; | ||
| } | ||
|
|
||
| const minZoom = zoom.left === 'dataMin' ? data[0].timestamp : zoom.left; // left zoom boundary |
There was a problem hiding this comment.
This could throw error if data is undefined or [] array. We may want to add a similar early return as in the utility above (returning whatever is appropriate in this context)
if (!data || data.length === 0) {
return legendRows;
}
|
|
||
| const onMouseDown = React.useCallback((e: CategoricalChartState) => { | ||
| const payload = e?.activePayload?.[0]?.payload; | ||
| if (!payload?.timestamp) return; |
There was a problem hiding this comment.
| if (!payload?.timestamp) return; | |
| if (payload?.timestamp === undefined) return; |
If timestamp can be 0, this check would incorrectly exit early. The same applies to other similar checks in this hook
There was a problem hiding this comment.
@pmakode-akamai , timestamps can never be 0, as there will no time with 0 value, but it is better to have a defensive check as well, so addressed these too.
| if (dragStart === null) return; | ||
|
|
||
| const payload = e?.activePayload?.[0]?.payload; | ||
| if (!payload?.timestamp) return; |
There was a problem hiding this comment.
same potential issue
| !prev.refAreaLeft || | ||
| !prev.refAreaRight || |
There was a problem hiding this comment.
| !prev.refAreaLeft || | |
| !prev.refAreaRight || | |
| prev.refAreaLeft === undefined || | |
| prev.refAreaRight === undefined || |
Same potential issue: 0 can be a valid value, so this check could fail. Could you double check this?
|
@pmakode-akamai , addressed the comments, lets see if it is good for approval |
Cloud Manager UI test results🔺 1 failing test on test run #5 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts" |
|||||||||||||||||
|
Merging it since it has enough approvals, cypress test failures are unrelated, jobs passed |
…inside the charts in CloudPulse graphs (linode#13308) * [DI-29167] - Initial changes for widget zoom in feature, Utility setup * [DI-29167] - Add understanding comments * Added changeset: Utils and Hooks set up for supporting zoom in inside the charts in `CloudPulse metrics graphs` * [DI-29167] - Address PR comments
…inside the charts in CloudPulse graphs (linode#13308) * [DI-29167] - Initial changes for widget zoom in feature, Utility setup * [DI-29167] - Add understanding comments * Added changeset: Utils and Hooks set up for supporting zoom in inside the charts in `CloudPulse metrics graphs` * [DI-29167] - Address PR comments
Description 📝
As a ask from customer, we are working towards adding the support for zooming in inside the CloudPulse graphs using click and drag on the timestamps inside the graphs, this PR has the initial changes for the zoom in such as the utilities required to support the click and drag and update data based on it,
Changes 🔄
computeZoomedInData - Using the original data, and left and right timestamp of the click and drag event, returns the updated / truncated data
computeLegendRowsBasedOnData - Using the truncated data and left and right timestamp, re-calculates the avergage, max and last displayed in the legend rows
getMetricsFromDimensionData - Calculated the Average, Max and Last for the given data
- Returns the zoom state, the min timestamp and the max timestamp
- Returns the OnMouseDown, OnMouseUp and OnMouseMove callbacks to listen to click and drag event
- Returns whether the data is actually zoomed in or not
- Retuns zoomOut callback, to reset the zoom state
Scope 🚢
Upon production release, changes in this PR will be visible to:
No customers are impacted, since this is just utilities
Target release date 🗓️ Next Release
Preview 📷
No visible UI changes as this is just adding of utilities / helpers needed for zoom in
How to test 🧪
No visible UI changes as this is just adding of utilities / helpers needed for zoom in
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅