Scaling: Consolidate ScalingTab and clean up components

[skoeva]

These changes consolidate ScalingTab into the Scaling module and extract reusable hooks and components to reduce duplication and improve maintainability.

Summary

• Move ScalingTab from ScalingTab/ into Scaling/ alongside ScalingCard, removing the separate directory
• Extract useEditDialog hook from inline ScalingTab logic, encapsulating dialog state, form pre-population, and HPA/manual PATCH save logic via clusterRequest
• Extract ScalingEditDialog component from inline ScalingTab JSX
• Extract MetricTile local helper in ScalingMetrics to reduce repeated Typography pairs, and lift CPU/bounds ternaries out of JSX into variables
• Set explicit time ranges per component: 24h/1h-step for ScalingTab, 2h/15min-step for ScalingCard; make timeRangeSecs and step required params in useChartData
• Simplify Alert usage across components by removing redundant Box/Typography wrappers
• Add TSDoc field docs to ScalingMetricsProps
• Add useEditDialog test suite (10 tests) covering dialog state, form pre-population, HPA and manual save paths, onSaved callback, and error handling
• Expand useDeployments tests with setSelectedDeployment coverage and data-reload stability
• Update useChartData tests for required params and additional cache/time-range edge cases

Fixes: #100

Testing

• Run cd plugins/aks-desktop && npm test and ensure the tests pass

Screenshot

[Copilot]

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 ScalingTab from ScalingTab/ directory into Scaling/ alongside ScalingCard
• Extracted useEditDialog hook to encapsulate dialog state management and HPA/manual scaling save logic
• Extracted ScalingEditDialog and MetricTile components 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 useEditDialog and expanded tests for useDeployments and useChartData

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.

[Copilot]

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.

[Copilot]

boundsValue falls back to currentDeployment.availableReplicas even when hpaInfo is present but minReplicas/maxReplicas are undefined. In that case the label is still “Replica Bounds”, so the value can become misleading. Consider returning N/A when hpaInfo is present but bounds are missing, and only using availableReplicas when in manual mode.

Suggested change

	const boundsValue =
	hpaInfo?.minReplicas !== undefined && hpaInfo?.maxReplicas !== undefined
	? `${hpaInfo.minReplicas}-${hpaInfo.maxReplicas}`
	: currentDeployment?.availableReplicas ?? 'N/A';
	const boundsValue = hpaInfo
	? hpaInfo.minReplicas !== undefined && hpaInfo.maxReplicas !== undefined
	? `${hpaInfo.minReplicas}-${hpaInfo.maxReplicas}`
	: 'N/A'
	: currentDeployment?.availableReplicas ?? 'N/A';

[Copilot]

The HPA fields coerce invalid/empty input to 0 (parseInt(...) || 0), but these inputs have min: 1. This can let users submit invalid values (e.g., minReplicas=0) and then PATCH an invalid HPA spec. Consider preserving the previous value when the input is empty/NaN, or clamping to the allowed minimum before saving.

[Copilot]

expect.toSatisfy(...) is used as an asymmetric matcher inside toHaveBeenCalledWith. Vitest/Jest typically don’t expose toSatisfy in that asymmetric form, so this can fail at runtime or typecheck. Prefer asserting expect.any(Number) here and then validating the actual start argument via mockQueryPrometheus.mock.calls[...] (or using a supported asymmetric matcher) to keep the test stable.

Suggested change

	expect.toSatisfy((start: number) => now - start <= 7200 + 5), // within 5s of expected
	expect.any(Number),
	900,
	'my-sub'
	);
	expect.any(Number),
	expect.any(Number),
	900,
	'my-sub'
	);
	// Validate that the start time is within 5s of the expected 2-hour window
	const firstCallArgs = mockQueryPrometheus.mock.calls[0];
	const start = firstCallArgs[2];
	expect(typeof start).toBe('number');
	expect(now - start).toBeLessThanOrEqual(7200 + 5);

[Copilot]

handleSave can issue PATCH requests with an undefined namespace/cluster (and in manual mode even when selectedDeployment is empty). That would generate URLs like /namespaces/undefined/... and can produce confusing failures. Add an early guard that validates namespace, cluster, and selectedDeployment (and ideally that the selected deployment exists in deployments) and sets saveError instead of calling clusterRequest.

[Copilot]

ScalingMetrics now renders 5 tiles (mode + 4 MetricTiles), but each Grid item uses xs={3} (4-per-row). This will force wrapping and can leave a single tile on its own row. Consider adjusting grid sizing (e.g., columns={15} with xs={3}, or responsive breakpoints like xs={12} sm={6} md={3} / switching to flex) so the layout remains balanced.

[Copilot]

This alert surfaces error/saveError strings directly, but at least saveError is currently produced as hard-coded English in useEditDialog. To keep UI text consistently localized, consider ensuring the hook returns a localized message (or an error key) before rendering it here.

Suggested change

	{error ?? saveError}
	{error ? t(error) : saveError ? t(saveError) : null}

[Copilot]

UseEditDialogResult references React.Dispatch / React.SetStateAction, but this module doesn’t import React (and the project uses classic JSX runtime per tsconfig). This will fail typechecking unless React is in scope. Import the needed types from react (e.g., Dispatch/SetStateAction or import type React from 'react').

[Copilot]

The save failure message is hard-coded English (and bypasses the plugin’s useTranslation flow used elsewhere in the Scaling UI). Consider having this hook return an error code/structure, or accept a formatter/translator callback, so the surfaced UI error can be localized consistently.

Suggested change

	setSaveError(
	`Failed to save scaling configuration: ${
	error instanceof Error ? error.message : 'Unknown error'
	}`
	);
	setSaveError(error instanceof Error ? error.message : 'Unknown error');

[Copilot]

chartDataCache was changed to a Map, but entries are never evicted—once a key expires (TTL), getCachedChartData returns null but the entry remains, so the map can grow unbounded across deployments/time ranges. Consider deleting expired entries on read, and/or pruning/capping the cache size when inserting new entries.

Suggested change

	return Date.now() - entry.timestamp < CACHE_TTL_MS ? entry.data : null;
	if (Date.now() - entry.timestamp < CACHE_TTL_MS) {
	return entry.data;
	}
	// Evict expired entries to prevent unbounded growth of the cache.
	chartDataCache.delete(cacheKey);
	return null;