Scaling: Consolidate ScalingTab and clean up components#330
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request consolidates the ScalingTab component into the Scaling module and extracts reusable hooks and components to improve code organization and maintainability. The refactoring separates presentation logic from business logic, making the codebase more testable and reducing duplication.
Changes:
- Moved
ScalingTabfromScalingTab/directory intoScaling/alongsideScalingCard - Extracted
useEditDialoghook to encapsulate dialog state management and HPA/manual scaling save logic - Extracted
ScalingEditDialogandMetricTilecomponents to reduce JSX complexity - Made time range and step parameters explicit and required in
useChartData, allowing different components to use different resolutions (24h/1h for ScalingTab, 2h/15min for ScalingCard) - Added comprehensive test coverage for new hooks with 10 new tests for
useEditDialogand expanded tests foruseDeploymentsanduseChartData
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/aks-desktop/src/index.tsx | Updated import path for ScalingTab from ScalingTab/ to Scaling/ |
| plugins/aks-desktop/src/components/ScalingTab/ScalingTab.tsx | Removed old ScalingTab implementation (721 lines deleted) |
| plugins/aks-desktop/src/components/Scaling/ScalingTab.tsx | New consolidated ScalingTab using extracted hooks and components (184 lines) |
| plugins/aks-desktop/src/components/Scaling/hooks/useEditDialog.ts | New hook encapsulating edit dialog state, form pre-population, and save logic |
| plugins/aks-desktop/src/components/Scaling/hooks/useEditDialog.test.ts | Comprehensive test suite for useEditDialog (10 tests covering all paths) |
| plugins/aks-desktop/src/components/Scaling/hooks/useDeployments.test.ts | Expanded tests for setSelectedDeployment and data reload stability |
| plugins/aks-desktop/src/components/Scaling/hooks/useChartData.ts | Made timeRangeSecs and step required parameters; changed cache from single object to Map |
| plugins/aks-desktop/src/components/Scaling/hooks/useChartData.test.ts | Updated all test calls to include required timeRangeSecs/step parameters; added cache collision tests |
| plugins/aks-desktop/src/components/Scaling/components/ScalingMetrics.tsx | Added TSDoc, extracted MetricTile helper, lifted CPU/bounds logic out of JSX |
| plugins/aks-desktop/src/components/Scaling/components/ScalingEditDialog.tsx | New extracted dialog component for editing scaling configuration |
| plugins/aks-desktop/src/components/Scaling/components/ScalingChart.tsx | Simplified Alert usage by removing redundant Typography wrapper |
| plugins/aks-desktop/src/components/Scaling/ScalingCard.tsx | Updated to pass explicit timeRangeSecs/step to useChartData; simplified Alert usage |
Comments suppressed due to low confidence (1)
plugins/aks-desktop/src/components/Scaling/hooks/useChartData.ts:126
- When cached data is returned (lines 121-126), loading state is not reset to false. The loading state was set to true on line 101, but the early return on line 126 bypasses the finally block on line 190 that sets loading to false. This would leave the loading indicator visible even though cached data is available. Add setLoading(false) in the cache hit block.
if (cachedChartData) {
applyIfLatest(() => {
setChartData(cachedChartData);
setError(null);
});
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Looks good at a glance. Marked it as draft because some convos are still open. |
52f794e to
9221789
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- plugins/aks-desktop/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
There was a problem hiding this comment.
lgtm
Please remove the package-lock.json change?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- plugins/aks-desktop/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@skoeva the package-lock file is still in the PR |
Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
These changes consolidate
ScalingTabinto theScalingmodule and extract reusable hooks and components to reduce duplication and improve maintainability.Summary
ScalingTabfromScalingTab/intoScaling/alongsideScalingCard, removing the separate directoryuseEditDialoghook from inlineScalingTablogic, encapsulating dialog state, form pre-population, and HPA/manual PATCH save logic viaclusterRequestScalingEditDialogcomponent from inlineScalingTabJSXMetricTilelocal helper inScalingMetricsto reduce repeatedTypographypairs, and lift CPU/bounds ternaries out of JSX into variablesScalingTab, 2h/15min-step forScalingCard; maketimeRangeSecsand step required params inuseChartDataAlertusage across components by removing redundantBox/TypographywrappersScalingMetricsPropsuseEditDialogtest suite (10 tests) covering dialog state, form pre-population, HPA and manual save paths,onSavedcallback, and error handlinguseDeploymentstests withsetSelectedDeploymentcoverage and data-reload stabilityuseChartDatatests for required params and additional cache/time-range edge casesboundsValueleak inScalingMetrics, || 0 coercion in HPA inputs, missing guard inhandleSave, and grid layout wrapping with 5 tilesFixes: #100
Testing
cd plugins/aks-desktop && npm testand ensure the tests passScreenshot