ECOPROJECT-3706 | feat: Introduce Cluster Sizer feature for OpenShift cluster recommendations #308
Conversation
📝 WalkthroughWalkthroughAdds a two-step Cluster Sizer wizard to Report that collects migration preferences, calls AssessmentApi.calculateAssessmentClusterRequirements via DI, displays recommendations, introduces UI types/constants, and keeps mocks for tests while removing runtime mock usage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Report as Report.tsx
participant Wizard as ClusterSizingWizard
participant API as AssessmentApi
participant Result as SizingResult
User->>Report: Click "View target cluster recommendations"
Report->>Wizard: open(assessmentId, clusterId, clusterName)
Wizard->>Wizard: render Step 1 (SizingInputForm)
User->>Wizard: submit preferences
Wizard->>API: calculateAssessmentClusterRequirements(assessmentId, request)
API-->>Wizard: ClusterRequirementsResponse
Wizard->>Result: render Step 2 with sizerOutput
User->>Result: review / copy / close
User->>Wizard: close
Wizard-->>Report: close modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-05T07:49:36.795ZApplied to files:
🧬 Code graph analysis (4)src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
src/pages/report/cluster-sizer/SizingInputForm.tsx (3)
src/pages/report/cluster-sizer/SizingResult.tsx (4)
src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts (1)
🪛 markdownlint-cli2 (0.18.1)specs/cluster-sizer.md37-37: Fenced code blocks should have a language specified (MD040, fenced-code-language) 77-77: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🔇 Additional comments (14)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jkilzi
force-pushed
the
ECOPROJECT-3706
branch
6 times, most recently
from
d331ab5 to
47f2f0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 5-6: Replace the current ignore/negation pair so Git still
traverses .cursor to allow negation: stop ignoring the directory itself by
changing ".cursor/" to ".cursor/*" and add explicit negations for the plans
directory and its contents (e.g. add "!.cursor/plans/" and "!.cursor/plans/**")
so that .cursor/* is ignored but .cursor/plans/ and all files under it are
tracked.
In `@src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts`:
- Around line 7-10: Import fails because OvercommitRatioString is not exported
from types.ts; add a string-format type export to types.ts so the mock can
import it. In src/pages/report/cluster-sizer/types.ts export a new type named
OvercommitRatioString with the API string literals '1:1' | '1:2' | '1:4' |
'1:6'; keep the existing numeric OvercommitRatio type and, if necessary, add a
small comment or mapping helper (e.g., between OvercommitRatio and
OvercommitRatioString) so consumers know how to convert, then re-run the build
to ensure the mock import of OvercommitRatioString resolves.
In `@src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts`:
- Around line 7-12: The import list incorrectly references ResourceConsumption;
replace it with SizingResourceConsumption in the import statement and update any
type annotations/usages in createMockResourceConsumption to use
SizingResourceConsumption (e.g., change function return type and any
variable/type references inside createMockResourceConsumption and any related
mocks that expect ResourceConsumption) so they match the exported type name
SizingResourceConsumption.
In `@src/pages/report/cluster-sizer/SizingResult.tsx`:
- Around line 72-73: The two bullet lines in SizingResult.tsx are using
inconsistent bullet characters ("~" for CPU and "-" for Memory) causing
mismatched UI and text; update the template strings and any rendered JSX that
produce these lines (references: the template lines containing "CPU Over-Commit
Ratio: ${formatRatio(cpuOverCommitRatio)}" and "Memory Over-Commit Ratio:
${formatRatio(memoryOverCommitRatio)}" and the rendered UI block around lines
219-224) to use the same bullet character (pick either "~" or "-") for both
entries so the plain text recommendation and rendered UI match.
🧹 Nitpick comments (6)
src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
9-17: Consider broadeningIconComponenttype to accept functional components.
React.ComponentClass<SVGIconProps>excludes functional components. PatternFly icons can be either class or function components depending on version. Consider usingReact.ComponentType<SVGIconProps>instead.Suggested fix
type PopoverIconProps = PopoverProps & { variant?: ButtonProps['variant']; component?: ButtonProps['component']; - IconComponent?: React.ComponentClass<SVGIconProps>; + IconComponent?: React.ComponentType<SVGIconProps>; noVerticalAlign?: boolean; buttonClassName?: string; buttonOuiaId?: string; buttonStyle?: React.CSSProperties; };specs/cluster-sizer.md (2)
36-39: Add language specifier to fenced code block.Per markdown linting, fenced code blocks should have a language specified. For endpoint paths, use
textorhttp.Suggested fix
### Endpoint -``` +```text POST /api/v1/assessments/{id}/cluster-requirements ```
76-86: Add language specifier and verify file structure accuracy.Two issues:
- The fenced code block should have a language specified (e.g.,
text).- Line 85 references
mockData.ts, but the PR introduces amocks/folder withindex.tsand separate mock files. Consider updating to reflect the actual structure.Suggested fix
## File Structure -``` +```text src/pages/report/cluster-sizer/ ├── index.ts # Re-exports ├── types.ts # TypeScript interfaces and type definitions ├── constants.ts # Form options (CPU, memory, over-commit dropdowns) ├── ClusterSizingWizard.tsx # Main wizard modal component ├── SizingInputForm.tsx # Step 1: Migration preferences form ├── SizingResult.tsx # Step 2: Results display -└── mockData.ts # Mock API responses for development +├── PopoverIcon.tsx # Popover helper component +└── mocks/ # Mock API responses for development + ├── index.ts + ├── clusterRequirementsRequest.mock.ts + └── clusterRequirementsResponse.mock.ts ```src/pages/report/Report.tsx (1)
381-387: Consider handling the undefinedidcase more explicitly.The
id || ''fallback silently passes an empty string to the API ifidis undefined. While the early returns (lines 80-120) should prevent this scenario, an emptyassessmentIdwould cause the API call to fail.Consider asserting that
idexists or adding a guard:{id && ( <ClusterSizingWizard isOpen={isSizingWizardOpen} onClose={() => setIsSizingWizardOpen(false)} clusterName={clusterView.selectionLabel} clusterId={selectedClusterId} assessmentId={id} /> )}src/pages/report/cluster-sizer/SizingResult.tsx (2)
97-103: Consider adding user feedback for copy action.The copy handler silently succeeds or fails. Users have no indication whether the copy worked. A common pattern is to temporarily change the button text to "Copied!" or show a toast notification.
Example enhancement
const [copied, setCopied] = useState(false); const handleCopyRecommendations = useCallback(async () => { try { await navigator.clipboard.writeText(plainTextRecommendation); setCopied(true); setTimeout(() => setCopied(false), 2000); } catch (err) { console.error('Failed to copy recommendations:', err); } }, [plainTextRecommendation]); // In the button: <Button ...> {copied ? 'Copied!' : 'Copy recommendations'} </Button>
123-135: Consider using PatternFly Alert for error display.The error state uses plain text. Using a PatternFly
Alertcomponent withvariant="danger"would provide better visual consistency and accessibility.Optional enhancement
import { Alert } from '@patternfly/react-core'; if (error) { return ( <Stack hasGutter> <StackItem> <Alert variant="danger" isInline title="Calculation failed"> {error.message} </Alert> </StackItem> </Stack> ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.cursor/plans/cluster_sizer_api_integration_4747313e.plan.md.gitignorepackage.jsonspecs/cluster-sizer.mdspecs/cluster-switcher.mdsrc/pages/report/Report.tsxsrc/pages/report/cluster-sizer/ClusterSizingWizard.tsxsrc/pages/report/cluster-sizer/PopoverIcon.tsxsrc/pages/report/cluster-sizer/SizingInputForm.tsxsrc/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsxsrc/pages/report/cluster-sizer/SizingResult.tsxsrc/pages/report/cluster-sizer/SizingResultWizardStepFooter.tsxsrc/pages/report/cluster-sizer/constants.tssrc/pages/report/cluster-sizer/index.tssrc/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.tssrc/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.tssrc/pages/report/cluster-sizer/mocks/data.mock.tssrc/pages/report/cluster-sizer/mocks/index.tssrc/pages/report/cluster-sizer/types.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-05T07:49:36.795Z
Learnt from: ammont82
Repo: kubev2v/migration-planner-ui-app PR: 248
File: src/pages/report/assessment-report/ClustersOverview.tsx:0-0
Timestamp: 2025-12-05T07:49:36.795Z
Learning: In src/pages/report/assessment-report/ClustersOverview.tsx, the CPU overcommitment view uses TOP_N=5 to show the top 5 clusters individually without aggregating remaining values, while vmByCluster and dataCenterDistribution views use TOP_N=4 and then add a "Rest of..." aggregated category, resulting in 5 total displayed items across all views but with different aggregation strategies.
Applied to files:
src/pages/report/cluster-sizer/SizingResult.tsxsrc/pages/report/cluster-sizer/constants.tsspecs/cluster-sizer.mdsrc/pages/report/cluster-sizer/types.tssrc/pages/report/cluster-sizer/SizingInputForm.tsxsrc/pages/report/cluster-sizer/ClusterSizingWizard.tsx.cursor/plans/cluster_sizer_api_integration_4747313e.plan.mdsrc/pages/report/Report.tsx
🧬 Code graph analysis (5)
src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
src/components/SVGIcon.tsx (1)
SVGIconProps(3-8)
src/pages/report/cluster-sizer/constants.ts (1)
src/pages/report/cluster-sizer/types.ts (4)
WorkerNodePreset(29-29)OvercommitRatio(34-34)HAReplicaCount(39-39)SizingFormValues(44-57)
src/pages/report/cluster-sizer/mocks/data.mock.ts (2)
src/pages/report/cluster-sizer/types.ts (2)
SizingFormValues(44-57)ClusterRequirementsResponse(18-18)src/pages/report/cluster-sizer/constants.ts (1)
WORKER_NODE_PRESETS(11-33)
src/pages/report/cluster-sizer/SizingInputForm.tsx (3)
src/pages/report/cluster-sizer/types.ts (2)
SizingFormValues(44-57)OvercommitRatio(34-34)src/pages/report/cluster-sizer/index.ts (1)
SizingInputForm(2-2)src/pages/report/cluster-sizer/constants.ts (3)
CPU_OPTIONS(38-44)MEMORY_OPTIONS(49-55)OVERCOMMIT_OPTIONS(60-91)
src/pages/report/Report.tsx (2)
src/pages/report/cluster-sizer/ClusterSizingWizard.tsx (1)
ClusterSizingWizard(31-148)src/pages/report/cluster-sizer/index.ts (1)
ClusterSizingWizard(1-1)
🪛 GitHub Check: type-check
src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts
[failure] 11-11:
'"../types"' has no exported member named 'ResourceConsumption'. Did you mean 'SizingResourceConsumption'?
src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts
[failure] 9-9:
'"../types"' has no exported member named 'OvercommitRatioString'. Did you mean 'OvercommitRatio'?
🪛 markdownlint-cli2 (0.18.1)
specs/cluster-sizer.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Red Hat Konflux / migration-planner-ui-on-pull-request
🔇 Additional comments (25)
src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
19-48: LGTM!The component implementation is clean:
- Proper use of
classNamesfor CSS compositiononClick={(e) => e.preventDefault()}correctly prevents form submission when used within forms- Props are well-destructured with sensible defaults
src/pages/report/cluster-sizer/mocks/index.ts (1)
1-8: LGTM!Clean barrel file for mock exports with helpful JSDoc referencing the related PR.
src/pages/report/cluster-sizer/index.ts (1)
1-5: LGTM!Well-structured barrel file establishing a clean public API surface for the cluster-sizer feature. Named exports for components and wildcard re-exports for types/constants is an appropriate pattern.
specs/cluster-sizer.md (1)
1-99: Good documentation coverage.The spec comprehensively documents the feature including wizard flow, API contracts, UX behaviors, and integration points. This will serve as a valuable reference for the team.
package.json (1)
35-35: LGTM!The dependency bump to
^0.0.37aligns with the cluster sizer feature requirements for the new API types.src/pages/report/Report.tsx (2)
43-43: LGTM: Clean integration of the ClusterSizingWizard.The new imports and state management for the sizing wizard are well-structured and follow existing patterns in this file.
Also applies to: 59-59
344-354: LGTM: Good conditional visibility for the sizing wizard button.The button correctly appears only when a specific cluster is selected (not "all"), and the placement alongside the download button provides logical grouping of cluster-related actions.
src/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsx (2)
23-26: Verify: Navigation proceeds even when calculation fails.The
handleNextfunction always callsgoToNextStep()afteronCalculate()completes, including when the API call fails (since the parent catches the error and doesn't re-throw). This means users navigate to the results step where the error is displayed.If this is intentional UX (show error on results page), this is fine. Otherwise, consider conditionally navigating:
const handleNext = async (): Promise<void> => { - await onCalculate(); - goToNextStep(); + try { + await onCalculate(); + goToNextStep(); + } catch { + // Stay on current step if calculation fails + } };This would require the parent to re-throw the error after catching it.
28-52: LGTM: Clean wizard footer implementation.The footer correctly disables both buttons during loading, preventing users from triggering multiple API calls or closing the wizard mid-request. The PatternFly component usage follows established patterns.
src/pages/report/cluster-sizer/SizingInputForm.tsx (2)
36-66: LGTM: Handler implementations are consistent and type-safe.The handlers correctly use
parseIntwith radix 10, and the type assertion forOvercommitRatiois safe since the options are controlled byOVERCOMMIT_OPTIONSconstants. SettingworkerNodePresetto'custom'on CPU/memory changes aligns with the form's intended behavior.
68-193: LGTM: Well-structured form with good accessibility.The form follows PatternFly best practices with proper
fieldIdmatching,aria-labelattributes, and helpful tooltips viaPopoverIcon. The dropdown-based inputs naturally prevent invalid input.src/pages/report/cluster-sizer/SizingResultWizardStepFooter.tsx (1)
16-43: LGTM: Clean footer for the results step.The footer correctly positions "Close" as the primary action for the final wizard step, with "Back" as a secondary option. Simple and effective implementation.
src/pages/report/cluster-sizer/SizingResult.tsx (2)
149-156: LGTM: Safe extraction of optional fields.The nullish coalescing (
??) correctly handles potentially undefined nested fields from the API response, providing sensible defaults of0.
157-247: LGTM: Well-structured results panel.The Panel component with sticky header and scrollable content provides good UX. The content is logically organized into cluster info, additional info, and resource breakdown sections with consistent formatting.
src/pages/report/cluster-sizer/ClusterSizingWizard.tsx (2)
31-95: LGTM! Clean DI-based API integration.The component correctly uses dependency injection to obtain the
AssessmentApi, properly manages loading/error states, and handles cleanup on close. TheuseCallbackhooks have correct dependency arrays.
101-147: Well-structured wizard modal.Good use of PatternFly's
WizardHeaderwith close handler and custom step footers for controlling navigation flow. Thearia-labelon the Modal provides appropriate accessibility.src/pages/report/cluster-sizer/mocks/data.mock.ts (1)
1-109: LGTM! Well-documented mock module.The mock calculations align with the expected API behavior, providing reasonable test data. The TODO comment at the top clearly marks this as temporary code for development/testing.
src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts (1)
12-109: Good mock data coverage once type issue is resolved.The factory function and predefined variants (small, large, min, max values) provide comprehensive test coverage. The structure is well-organized for unit testing scenarios.
.cursor/plans/cluster_sizer_api_integration_4747313e.plan.md (1)
1-110: Useful integration plan document.The plan effectively documents the DI-based API integration approach, with clear implementation steps and code examples. Good reference for understanding the architectural decisions.
src/pages/report/cluster-sizer/constants.ts (1)
1-190: LGTM! Comprehensive and well-organized constants module.The constants provide a clean, centralized source for form defaults, validation constraints, and UI labels. The type-safe definitions using
OvercommitRatio,HAReplicaCount, andWorkerNodePresetensure consistency with the type system.src/pages/report/cluster-sizer/types.ts (3)
10-24: LGTM!Clean import and re-export pattern that creates a single source of truth for API types within the cluster-sizer module.
26-62: LGTM!Well-documented type definitions with clear JSDoc comments. The type constraints (literal unions for presets, ratios, and replica counts) provide good type safety.
86-100: No action required; haReplicas omission is intentional.The
haReplicasfield is a frontend-only form value used for client-side calculations (determining the number of control plane nodes). The API payload does not require or accept this field—cluster control plane configuration is conveyed via thecontrolPlaneSchedulableboolean instead. All mockClusterRequirementsRequestobjects consistently excludehaReplicas, confirming this is by design.Likely an incorrect or invalid review comment.
src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts (2)
17-42: LGTM!Well-structured default mock with realistic values representing a typical medium-sized cluster.
195-247: Good factory pattern for flexible mock creation.The factory functions with
Partial<T>overrides provide a clean API for creating custom mocks in tests. The nullish coalescing defaults ensure complete objects are always returned.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts
Outdated
Show resolved
Hide resolved
src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts
Show resolved
Hide resolved
jkilzi
force-pushed
the
ECOPROJECT-3706
branch
from
47f2f0f to
adb138a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/report/cluster-sizer/PopoverIcon.tsx`:
- Line 12: IconComponent is currently typed as
React.ComponentClass<SVGIconProps>, which excludes function components like
PatternFly icons; change the prop type to React.ComponentType<SVGIconProps> (or
React.FC<SVGIconProps>) so both class and function components are accepted,
update the IconComponent prop declaration in PopoverIcon.tsx accordingly, and
ensure any default assignment (e.g., OutlinedQuestionCircleIcon) and usages
still type-check after the change.
♻️ Duplicate comments (1)
src/pages/report/cluster-sizer/SizingResult.tsx (1)
72-73: Inconsistent bullet characters in plain text output.Line 72 uses
~while line 73 uses-for bullet points. This inconsistency also appears in the rendered UI (lines 220 and 223).Proposed fix
-~ CPU Over-Commit Ratio: ${formatRatio(cpuOverCommitRatio)} +- CPU Over-Commit Ratio: ${formatRatio(cpuOverCommitRatio)}
🧹 Nitpick comments (5)
src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
9-17: Consider exportingPopoverIconPropsfor consumer type access.If this component will be reused across the codebase, exporting the type definition would allow consumers to properly type their props when extending or wrapping this component.
Proposed change
-type PopoverIconProps = PopoverProps & { +export type PopoverIconProps = PopoverProps & {Also applies to: 50-50
specs/cluster-sizer.md (2)
37-39: Add language specifier to fenced code block.The API endpoint code block is missing a language specifier. Per markdownlint, fenced code blocks should have a language specified.
Proposed fix
-``` +```text POST /api/v1/assessments/{id}/cluster-requirements</details> --- `77-86`: **Add language specifier to file structure code block.** The file structure code block is missing a language specifier. <details> <summary>Proposed fix</summary> ```diff -``` +```text src/pages/report/cluster-sizer/ ├── index.ts # Re-exportssrc/pages/report/cluster-sizer/SizingResult.tsx (1)
97-103: Consider providing user feedback on copy success/failure.The copy handler silently logs errors to console. Consider adding user feedback (e.g., a toast notification or button state change) to indicate successful copy or failure.
src/pages/report/cluster-sizer/mocks/data.mock.ts (1)
53-56: Worker node calculation ignores memory requirements.The node count is derived solely from CPU. With the mock inventory (281 CPU / 1117 GB), choosing a small preset (16 CPU / 64 GB) yields ~6 workers (with overcommit 1.0), providing 384 GB memory for 1117 GB needed—memory would be significantly oversubscribed.
This is acceptable for mock data, but ensure the real API considers both CPU and memory when determining minimum nodes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.cursor/plans/cluster_sizer_api_integration_4747313e.plan.md.gitignorepackage.jsonspecs/cluster-sizer.mdspecs/cluster-switcher.mdsrc/pages/report/Report.tsxsrc/pages/report/cluster-sizer/ClusterSizingWizard.tsxsrc/pages/report/cluster-sizer/PopoverIcon.tsxsrc/pages/report/cluster-sizer/SizingInputForm.tsxsrc/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsxsrc/pages/report/cluster-sizer/SizingResult.tsxsrc/pages/report/cluster-sizer/SizingResultWizardStepFooter.tsxsrc/pages/report/cluster-sizer/constants.tssrc/pages/report/cluster-sizer/index.tssrc/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.tssrc/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.tssrc/pages/report/cluster-sizer/mocks/data.mock.tssrc/pages/report/cluster-sizer/mocks/index.tssrc/pages/report/cluster-sizer/types.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- .cursor/plans/cluster_sizer_api_integration_4747313e.plan.md
- src/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsx
- src/pages/report/cluster-sizer/SizingResultWizardStepFooter.tsx
- src/pages/report/cluster-sizer/SizingInputForm.tsx
- src/pages/report/cluster-sizer/constants.ts
- package.json
- src/pages/report/cluster-sizer/ClusterSizingWizard.tsx
- src/pages/report/cluster-sizer/mocks/index.ts
- .gitignore
- src/pages/report/Report.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-05T07:49:36.795Z
Learnt from: ammont82
Repo: kubev2v/migration-planner-ui-app PR: 248
File: src/pages/report/assessment-report/ClustersOverview.tsx:0-0
Timestamp: 2025-12-05T07:49:36.795Z
Learning: In src/pages/report/assessment-report/ClustersOverview.tsx, the CPU overcommitment view uses TOP_N=5 to show the top 5 clusters individually without aggregating remaining values, while vmByCluster and dataCenterDistribution views use TOP_N=4 and then add a "Rest of..." aggregated category, resulting in 5 total displayed items across all views but with different aggregation strategies.
Applied to files:
specs/cluster-sizer.mdsrc/pages/report/cluster-sizer/types.tssrc/pages/report/cluster-sizer/SizingResult.tsx
🧬 Code graph analysis (4)
src/pages/report/cluster-sizer/mocks/data.mock.ts (2)
src/pages/report/cluster-sizer/types.ts (2)
SizingFormValues(44-57)ClusterRequirementsResponse(18-18)src/pages/report/cluster-sizer/constants.ts (1)
WORKER_NODE_PRESETS(11-33)
src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts (1)
src/pages/report/cluster-sizer/types.ts (1)
ClusterRequirementsRequest(17-17)
src/pages/report/cluster-sizer/SizingResult.tsx (4)
src/pages/report/cluster-sizer/types.ts (1)
SizingFormValues(44-57)src/utils/formatters.ts (1)
formatNumber(8-16)src/pages/report/cluster-sizer/constants.ts (1)
OVERCOMMIT_OPTIONS(60-91)src/pages/report/cluster-sizer/index.ts (1)
SizingResult(3-3)
src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
src/components/SVGIcon.tsx (1)
SVGIconProps(3-8)
🪛 markdownlint-cli2 (0.18.1)
specs/cluster-sizer.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (17)
src/pages/report/cluster-sizer/PopoverIcon.tsx (1)
19-48: LGTM!The component implementation is clean and well-structured. The default values are sensible, the
onClickhandler correctly prevents default behavior (useful when inside forms), and the prop spreading to Popover is appropriate.src/pages/report/cluster-sizer/types.ts (3)
1-24: LGTM! Clean type organization and re-exports.The type definitions are well-documented, and the re-export pattern from the API client keeps the import surface clean for consumers.
67-84: LGTM! Exhaustive enum mapping.The
OVERCOMMIT_RATIO_MAPcorrectly maps allOvercommitRatiovalues to their API enum counterparts, ensuring type safety at compile time.
89-100:haReplicasfield is not included in the API request payload.The
SizingFormValuesinterface includeshaReplicas(line 52), butformValuesToRequestdoes not map it to the API payload. If this field is intended for future use, consider adding a TODO comment. Otherwise, verify if it should be included inClusterRequirementsRequest.src/pages/report/cluster-sizer/SizingResult.tsx (3)
30-38: LGTM! Local formatting functions are appropriate.The local
formatNumberusingtoLocaleString()is intentionally different from the shared utility insrc/utils/formatters.ts(which uses K/M abbreviations). Locale-specific formatting is more suitable for the detailed recommendation display.
105-147: LGTM! Proper handling of loading, error, and empty states.The component correctly handles all three edge cases with appropriate UI feedback before rendering the main content.
149-156: LGTM! Safe optional field extraction with sensible defaults.The nullish coalescing pattern for extracting nested optional fields (
overCommitRatio,limits) ensures the component won't crash if the API response has missing fields.src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts (3)
7-18: LGTM! Well-structured mock data with proper type import.The import from
'../types'correctly uses the re-exportedClusterRequirementsRequesttype, and the mock provides realistic default values.
79-93: LGTM! Factory function provides good flexibility for testing.The
createMockClusterRequirementsRequestfactory with partial overrides enables easy creation of custom test scenarios while maintaining type safety.
98-106: LGTM! Convenient lookup map for over-commit ratio variants.The
mockRequestsByOvercommitRatiorecord provides type-safe access to pre-built request mocks for each over-commit ratio, useful for parameterized tests.src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts (3)
7-12: LGTM! Import issue resolved.The import now correctly uses
SizingResourceConsumptionas re-exported from'../types'.
17-42: LGTM! Comprehensive default mock response.The
mockClusterRequirementsResponseprovides a realistic medium-sized cluster scenario with all nested fields populated, serving as a good baseline for tests.
198-247: LGTM! Composable factory functions.The factory functions for nested types (
createMockClusterSizing,createMockInventoryTotals,createMockResourceConsumption) enable flexible test data composition whilecreateMockClusterRequirementsResponseties them together with sensible defaults.src/pages/report/cluster-sizer/mocks/data.mock.ts (3)
1-23: LGTM!Clear documentation with ticket reference for future cleanup. The
as constassertion onMOCK_INVENTORYensures type safety and immutability.
84-91: Verify the semantics oflimitsandoverCommitRatiofields.The
limitsvalues are computed asinventory * overcommitRatio, which represents the inventory's resource needs adjusted by overcommit. Typically, "limits" in cluster sizing refers to maximum schedulable resources (clusterCapacity * overcommitRatio).Similarly,
overCommitRatiohere computes actual utilization ratios (inventory / totalCluster), not the user-configured overcommit ratio.Please verify these calculations align with the API specification in ECOPROJECT-3631.
96-109: LGTM!Clean async wrapper with configurable delay for realistic API simulation during development.
src/pages/report/cluster-sizer/index.ts (1)
1-5: LGTM!Clean barrel file providing a single entry point for the cluster-sizer module. Good practice to exclude mocks from the public exports—they remain accessible via direct import when needed for testing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jkilzi
force-pushed
the
ECOPROJECT-3706
branch
from
adb138a to
a160afc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/pages/report/cluster-sizer/mocks/index.ts`:
- Around line 1-8: The file header comment incorrectly references PR `#819` and
ECOPROJECT-3719; update the top comment block above the exports in
src/pages/report/cluster-sizer/mocks/index.ts to reference the correct PR number
and ticket (PR `#308` / ECOPROJECT-3706) or explicitly state that PR `#819` is a
related backend change — edit the JSDoc comment string at the top of this module
to reflect the accurate PR/ticket or clarify the relationship.
In `@src/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsx`:
- Around line 23-26: The handleNext function currently awaits onCalculate()
without error handling, causing unhandled promise rejections and no user
feedback; wrap the await in a try-catch inside handleNext, only call
goToNextStep() on success, and in the catch either call a provided error
callback (e.g., an onError prop) or set a local error state so the component can
display the error and avoid unhandled rejections while preserving correct
non-advancement on failure.
♻️ Duplicate comments (1)
src/pages/report/cluster-sizer/SizingResult.tsx (1)
72-73: Inconsistent bullet characters.Lines 72 and 73 use different bullet characters (
~vs-). This inconsistency also appears in the UI render (lines 220, 223).
🧹 Nitpick comments (6)
src/pages/report/cluster-sizer/constants.ts (1)
35-55: Minor discrepancy with spec documentation.The spec (
specs/cluster-sizer.mdline 20) lists CPU options as(8, 16, 32, 64, 96, 128), butCPU_OPTIONShere defines[16, 32, 64, 128, 200]. Similarly, memory options differ. Please ensure the spec is updated to match the implementation, or clarify if this is an intentional deviation from the Figma design.specs/cluster-sizer.md (2)
37-39: Add language specifier to fenced code block.Per linting rules (MD040), fenced code blocks should have a language specified for syntax highlighting and accessibility.
📝 Suggested fix
-``` +```text POST /api/v1/assessments/{id}/cluster-requirements</details> --- `76-86`: **Update file structure to match implementation.** The documented structure shows `mockData.ts`, but the actual implementation uses a `mocks/` directory with multiple files. Also, add a language specifier to the code block. <details> <summary>📝 Suggested fix</summary> ```diff -``` +```text src/pages/report/cluster-sizer/ ├── index.ts # Re-exports ├── types.ts # TypeScript interfaces and type definitions ├── constants.ts # Form options (CPU, memory, over-commit dropdowns) ├── ClusterSizingWizard.tsx # Main wizard modal component ├── SizingInputForm.tsx # Step 1: Migration preferences form ├── SizingResult.tsx # Step 2: Results display -└── mockData.ts # Mock API responses for development +└── mocks/ # Mock API responses for development + ├── index.ts + ├── clusterRequirementsRequest.mock.ts + ├── clusterRequirementsResponse.mock.ts + └── data.mock.tssrc/pages/report/cluster-sizer/SizingInputForm.tsx (1)
36-66: Consider adding Number() instead of parseInt for consistency.Since the option values are already numbers converted to strings,
Number(cpu)would be cleaner thanparseInt(cpu, 10). However, this is a minor stylistic preference and the current implementation is correct.♻️ Optional refactor
const handleCpuChange = ( _event: React.FormEvent<HTMLSelectElement>, cpu: string, ): void => { onChange({ ...values, workerNodePreset: 'custom', - customCpu: parseInt(cpu, 10), + customCpu: Number(cpu), }); };src/pages/report/cluster-sizer/SizingResult.tsx (2)
30-38: Consider clarifying the local formatNumber purpose.There's a
formatNumberutility insrc/utils/formatters.tsthat produces compact output (1.2M), while this local version produces locale-formatted output (1,234,567). Both are valid but the naming overlap could cause confusion. Consider either:
- Renaming to
formatNumberLocalefor clarity, or- Adding a brief comment distinguishing from the utility version
97-103: Consider adding user feedback for copy action.The clipboard copy silently succeeds or fails with only a console log on error. Users would benefit from visual feedback (e.g., a toast notification or temporary button text change like "Copied!") to confirm the action succeeded.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.cursor/plans/cluster_sizer_api_integration_4747313e.plan.md.gitignorepackage.jsonspecs/cluster-sizer.mdspecs/cluster-switcher.mdsrc/pages/report/Report.tsxsrc/pages/report/cluster-sizer/ClusterSizingWizard.tsxsrc/pages/report/cluster-sizer/PopoverIcon.tsxsrc/pages/report/cluster-sizer/SizingInputForm.tsxsrc/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsxsrc/pages/report/cluster-sizer/SizingResult.tsxsrc/pages/report/cluster-sizer/SizingResultWizardStepFooter.tsxsrc/pages/report/cluster-sizer/constants.tssrc/pages/report/cluster-sizer/index.tssrc/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.tssrc/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.tssrc/pages/report/cluster-sizer/mocks/data.mock.tssrc/pages/report/cluster-sizer/mocks/index.tssrc/pages/report/cluster-sizer/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- package.json
- src/pages/report/cluster-sizer/index.ts
- src/pages/report/cluster-sizer/SizingResultWizardStepFooter.tsx
- src/pages/report/cluster-sizer/mocks/clusterRequirementsRequest.mock.ts
- src/pages/report/cluster-sizer/PopoverIcon.tsx
- src/pages/report/Report.tsx
- src/pages/report/cluster-sizer/ClusterSizingWizard.tsx
- src/pages/report/cluster-sizer/mocks/data.mock.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ammont82
Repo: kubev2v/migration-planner-ui-app PR: 248
File: src/pages/report/assessment-report/ClustersOverview.tsx:0-0
Timestamp: 2025-12-05T07:49:36.795Z
Learning: In src/pages/report/assessment-report/ClustersOverview.tsx, the CPU overcommitment view uses TOP_N=5 to show the top 5 clusters individually without aggregating remaining values, while vmByCluster and dataCenterDistribution views use TOP_N=4 and then add a "Rest of..." aggregated category, resulting in 5 total displayed items across all views but with different aggregation strategies.
📚 Learning: 2025-12-05T07:49:36.795Z
Learnt from: ammont82
Repo: kubev2v/migration-planner-ui-app PR: 248
File: src/pages/report/assessment-report/ClustersOverview.tsx:0-0
Timestamp: 2025-12-05T07:49:36.795Z
Learning: In src/pages/report/assessment-report/ClustersOverview.tsx, the CPU overcommitment view uses TOP_N=5 to show the top 5 clusters individually without aggregating remaining values, while vmByCluster and dataCenterDistribution views use TOP_N=4 and then add a "Rest of..." aggregated category, resulting in 5 total displayed items across all views but with different aggregation strategies.
Applied to files:
src/pages/report/cluster-sizer/SizingInputForm.tsx.cursor/plans/cluster_sizer_api_integration_4747313e.plan.mdsrc/pages/report/cluster-sizer/constants.tssrc/pages/report/cluster-sizer/SizingResult.tsxsrc/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.tsspecs/cluster-sizer.md
🧬 Code graph analysis (4)
src/pages/report/cluster-sizer/SizingInputForm.tsx (3)
src/pages/report/cluster-sizer/types.ts (2)
SizingFormValues(44-57)OvercommitRatio(34-34)src/pages/report/cluster-sizer/index.ts (1)
SizingInputForm(2-2)src/pages/report/cluster-sizer/constants.ts (3)
CPU_OPTIONS(38-44)MEMORY_OPTIONS(49-55)OVERCOMMIT_OPTIONS(60-91)
src/pages/report/cluster-sizer/constants.ts (1)
src/pages/report/cluster-sizer/types.ts (4)
WorkerNodePreset(29-29)OvercommitRatio(34-34)HAReplicaCount(39-39)SizingFormValues(44-57)
src/pages/report/cluster-sizer/SizingResult.tsx (4)
src/pages/report/cluster-sizer/types.ts (2)
SizingFormValues(44-57)ClusterRequirementsResponse(18-18)src/utils/formatters.ts (1)
formatNumber(8-16)src/pages/report/cluster-sizer/constants.ts (1)
OVERCOMMIT_OPTIONS(60-91)src/pages/report/cluster-sizer/index.ts (1)
SizingResult(3-3)
src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts (1)
src/pages/report/cluster-sizer/types.ts (4)
ClusterRequirementsResponse(18-18)ClusterSizing(19-19)InventoryTotals(20-20)SizingResourceConsumption(22-22)
🪛 markdownlint-cli2 (0.18.1)
specs/cluster-sizer.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (18)
.gitignore (1)
5-7: LGTM!The
.gitignorepattern is now correct. Using.cursor/*allows Git to traverse into the directory, and the negation patterns properly include.cursor/plans/and its contents.src/pages/report/cluster-sizer/types.ts (3)
1-24: LGTM!The module organization is clean with clear separation between API type re-exports and UI-specific types. JSDoc comments provide helpful context linking to the Jira ticket.
26-62: LGTM!The UI types are well-designed with appropriate use of literal types for constrained values and comprehensive JSDoc documentation for each field.
86-100: Verify:haReplicasis not included in the API request.The
formValuesToRequestfunction excludeshaReplicasfrom theClusterRequirementsRequestpayload. Based on the spec, this appears intentional since the API schema doesn't include it. Please confirm that HA configuration is handled separately or is purely UI state for display purposes.src/pages/report/cluster-sizer/mocks/clusterRequirementsResponse.mock.ts (3)
1-42: LGTM!The mock module is well-organized with clear documentation. The default mock response provides representative values for a medium-sized cluster scenario.
44-193: LGTM!Good coverage of different cluster scenarios with realistic data. The mocks appropriately vary resource consumption, overcommit ratios, and cluster sizes to support comprehensive testing.
195-247: LGTM!The factory functions are well-designed with sensible defaults and proper composition. Using nullish coalescing (
??) correctly preserves explicit0orfalseoverrides.src/pages/report/cluster-sizer/constants.ts (4)
1-33: LGTM!The worker node presets are well-defined with clear labels and descriptions. The type exclusion for 'custom' is appropriate since custom doesn't have predefined CPU/memory values.
57-120: LGTM!The overcommit and HA options are comprehensive with clear user-facing labels, descriptions, and help text. The values correctly match their respective type constraints.
138-148: Verify: Default overcommit ratio differs from spec.The spec (
specs/cluster-sizer.mdline 90) states the default over-commit is1:6, butDEFAULT_FORM_VALUES.overcommitRatiois set to4(1:4). Please confirm which is correct -1:4is more conservative and may be a safer default, but the spec should be aligned with the implementation.
150-190: LGTM!Validation constraints are reasonable, and form labels provide helpful context for users making sizing decisions.
specs/cluster-sizer.md (1)
88-96: Default over-commit value is inconsistent with implementation.Line 90 states the default over-commit is
1:6, butconstants.tssetsovercommitRatio: 4(which maps to1:4). Please align the spec with the actual implementation.src/pages/report/cluster-sizer/SizingInputForm.tsx (2)
1-28: LGTM! Clean controlled form component.The component follows good patterns with immutable state updates and clean separation of concerns. The props interface with
valuesandonChangecreates a proper controlled component pattern.
68-198: Well-structured form with good accessibility.The form properly uses PatternFly components, includes aria-labels for accessibility, and provides helpful popovers for user guidance. The Grid/Stack layout is appropriate for the form structure.
src/pages/report/cluster-sizer/SizingInputFormWizardStepFooter.tsx (1)
28-53: LGTM! Proper wizard footer implementation.The footer correctly uses PatternFly's wizard patterns with appropriate loading states and button disabling during async operations.
src/pages/report/cluster-sizer/SizingResult.tsx (2)
149-155: Good defensive handling of optional API fields.The null coalescing for
overCommitRatioandlimitsaligns with the API integration plan noting these fields are optional in the api-client types.
81-147: Well-structured state handling.The component properly handles loading, error, and empty data states with appropriate UI feedback for each case. The memoization of
plainTextRecommendationis a good optimization..cursor/plans/cluster_sizer_api_integration_4747313e.plan.md (1)
1-110: No action needed. This plan file is intentionally committed to the repository.The
.gitignoreexplicitly whitelists.cursor/plans/with!.cursor/plans/and!.cursor/plans/**, indicating the team deliberately includes these plan files in version control for shared documentation purposes. This is not an oversight.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
… cluster recommendations - Added a new `ClusterSizingWizard` component to guide users through selecting migration preferences and reviewing cluster sizing recommendations. - Implemented a two-step wizard that collects user inputs for worker node configurations and displays calculated requirements based on VMware inventory data. - Created supporting components including `SizingInputForm`, `SizingResult`, and their respective footers for navigation. - Integrated mock data for development and testing purposes, with a plan for future API integration. This feature enhances the user experience by providing tailored cluster sizing recommendations, facilitating smoother migrations to OpenShift. Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com> ECOPROJECT-3706 | feat: Update Cluster Sizer: Plan, API integration and dependencies - Updated the `.gitignore` to include specific cursor plans while excluding others. - Bumped the version of `@migration-planner-ui/api-client` from `0.0.36` to `0.0.37` in both `package.json` and `package-lock.json`. - Introduced a new plan document for the Cluster Sizer API integration detailing implementation steps and key files. - Refactored `ClusterSizingWizard` to utilize the new API client for fetching cluster requirements, removing mock data dependencies. - Enhanced `SizingResult` and `SizingInputForm` components to handle optional API response fields and improve user feedback. - Cleaned up code formatting and comments for better readability. This update improves the integration of the Cluster Sizer feature with the backend API, enhancing the overall user experience in the migration planning process. Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com>
jkilzi
force-pushed
the
ECOPROJECT-3706
branch
from
a160afc to
a45156c
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 participants
ClusterSizingWizardcomponent to guide users through selecting migration preferences and reviewing cluster sizing recommendations.SizingInputForm,SizingResult, and their respective footers for navigation.This feature enhances the user experience by providing tailored cluster sizing recommendations, facilitating smoother migrations to OpenShift.
Signed-off-by: Jonathan Kilzi jkilzi@redhat.com
Summary by CodeRabbit
New Features
Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.