ECOPROJECT-4354 | feat: redesign Migration Time Estimation tab#568
Merged
openshift-merge-bot[bot] merged 3 commits intokubev2v:masterfrom Apr 15, 2026
Merged
ECOPROJECT-4354 | feat: redesign Migration Time Estimation tab#568openshift-merge-bot[bot] merged 3 commits intokubev2v:masterfrom
openshift-merge-bot[bot] merged 3 commits intokubev2v:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/report/view-models/useClusterSizingWizardViewModel.ts (1)
166-197:⚠️ Potential issue | 🟠 MajorProtect this request from out-of-order completions.
Unlike the complexity flow below, this path has no request-id guard. If
calculateEstimation()is triggered again before a previous request finishes, an older response can overwrite the newer estimate becausesetMigrationEstimation()andsetManualEstimationError()still run for the stale call.Possible fix
const [estimationState, doCalculateEstimation] = useAsyncFn(async () => { setManualEstimationError(undefined); + const requestId = `${assessmentId}-${clusterId}-${Date.now()}`; + latestEstimationRequestIdRef.current = requestId; try { const result = await assessmentsStore.calculateMigrationEstimation({ id: assessmentId, migrationEstimationRequest: { clusterId, estimationSchema: ["network-based", "storage-offload"], params: estimationFormToParams(estimationFormValues), }, }); - const hasSchemas = result && Object.keys(result).length > 0; - setMigrationEstimation(hasSchemas ? result : null); + if (latestEstimationRequestIdRef.current === requestId) { + const hasSchemas = result && Object.keys(result).length > 0; + setMigrationEstimation(hasSchemas ? result : null); + } } catch (err) { + if (latestEstimationRequestIdRef.current !== requestId) { + return; + } if (err instanceof ResponseError) {Add a sibling ref next to
latestComplexityRequestIdRef:const latestEstimationRequestIdRef = useRef<string>("");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/report/view-models/useClusterSizingWizardViewModel.ts` around lines 166 - 197, This handler in useAsyncFn (the async function assigned to estimationState/doCalculateEstimation) lacks a request-id guard and can let stale responses overwrite state; add a sibling ref (e.g., latestEstimationRequestIdRef, placed near latestComplexityRequestIdRef) and generate a unique id at the start of the async function, assign it to latestEstimationRequestIdRef.current, then before calling setMigrationEstimation(...) or setManualEstimationError(...) verify the id still matches the ref (if not, bail out and do not update state); ensure the check is applied in both the success path after receiving result and in the catch path so only the latest request updates state.
🧹 Nitpick comments (1)
src/ui/report/views/cluster-sizer/__tests__/ClusterSizingWizard.test.tsx (1)
23-46: Please cover the new estimation-form wiring in this suite.The mock now exposes
estimationFormValuesandsetEstimationFormValues, but none of the tests actually drive the time-estimation inputs or assert that the setter is used. One interaction test around the new sliders would protect the main behavior added in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/report/views/cluster-sizer/__tests__/ClusterSizingWizard.test.tsx` around lines 23 - 46, Tests currently don't exercise the new estimation-form wiring: update ClusterSizingWizard.test.tsx to simulate user interactions against the estimation inputs and assert setEstimationFormValues is called with updated values. Locate mockViewModel (with estimationFormValues and setEstimationFormValues) and the rendered ClusterSizingWizard instance, find the UI controls bound to transferRateMbps, workHoursPerDay and postMigrationEngineers (the sliders/inputs rendered by the estimation form), fire events to change their values, and assert mockSetEstimationFormValues (setEstimationFormValues) was invoked with the expected partial/complete form object reflecting the new values. Ensure you restore any mocks between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/report/views/cluster-sizer/constants.ts`:
- Around line 237-240: Update ESTIMATION_SLIDER_LIMITS so the slider minimums
are positive (do not allow 0): set transferRateMbps.min, workHoursPerDay.min,
and postMigrationEngineers.min to appropriate >0 values (e.g., 1 or another
domain-appropriate lower bound) and ensure any slider components that read
ESTIMATION_SLIDER_LIMITS use those updated mins; additionally add frontend
validation in the form submit path to reject or clamp zero values before
submission so the UI cannot send impossible scenarios to the backend.
---
Outside diff comments:
In `@src/ui/report/view-models/useClusterSizingWizardViewModel.ts`:
- Around line 166-197: This handler in useAsyncFn (the async function assigned
to estimationState/doCalculateEstimation) lacks a request-id guard and can let
stale responses overwrite state; add a sibling ref (e.g.,
latestEstimationRequestIdRef, placed near latestComplexityRequestIdRef) and
generate a unique id at the start of the async function, assign it to
latestEstimationRequestIdRef.current, then before calling
setMigrationEstimation(...) or setManualEstimationError(...) verify the id still
matches the ref (if not, bail out and do not update state); ensure the check is
applied in both the success path after receiving result and in the catch path so
only the latest request updates state.
---
Nitpick comments:
In `@src/ui/report/views/cluster-sizer/__tests__/ClusterSizingWizard.test.tsx`:
- Around line 23-46: Tests currently don't exercise the new estimation-form
wiring: update ClusterSizingWizard.test.tsx to simulate user interactions
against the estimation inputs and assert setEstimationFormValues is called with
updated values. Locate mockViewModel (with estimationFormValues and
setEstimationFormValues) and the rendered ClusterSizingWizard instance, find the
UI controls bound to transferRateMbps, workHoursPerDay and
postMigrationEngineers (the sliders/inputs rendered by the estimation form),
fire events to change their values, and assert mockSetEstimationFormValues
(setEstimationFormValues) was invoked with the expected partial/complete form
object reflecting the new values. Ensure you restore any mocks between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c843393-5970-4bd4-a400-1e205e479cef
📒 Files selected for processing (7)
src/ui/report/view-models/useClusterSizingWizardViewModel.tssrc/ui/report/views/cluster-sizer/ClusterSizingWizard.tsxsrc/ui/report/views/cluster-sizer/TimeEstimationForm.tsxsrc/ui/report/views/cluster-sizer/TimeEstimationResult.tsxsrc/ui/report/views/cluster-sizer/__tests__/ClusterSizingWizard.test.tsxsrc/ui/report/views/cluster-sizer/constants.tssrc/ui/report/views/cluster-sizer/types.ts
ammont82
force-pushed
the
ECOPROJECT-4354
branch
2 times, most recently
from
850b776 to
2979be9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/report/views/cluster-sizer/TimeEstimationForm.tsx`:
- Around line 79-86: handleInputChange currently only clamps parsed integers to
min/max, allowing typed values that the slider cannot produce; change it to snap
the parsed value to the configured step before clamping and calling onChange.
Specifically, in handleInputChange (and using the existing min, max, step,
onChange symbols) compute a stepped value like min + Math.round((parsed - min) /
step) * step (or Math.floor/ceil if you prefer a different tie behavior), then
clamp that stepped value between min and max and pass it to onChange; keep the
NaN guard so invalid input is ignored.
In `@src/ui/report/views/cluster-sizer/TimeEstimationResult.tsx`:
- Around line 244-253: The copy handler currently ignores clipboard availability
and write failures; add a canCopy boolean (derived from estimationOutput,
navigator.clipboard?.writeText existence, and secure context check) and use it
to disable the "Copy as plain text" button, then update handleCopy to return
early if !canCopy and to await
navigator.clipboard.writeText(generatePlainText(estimationOutput)) inside a
try/catch so write failures are caught and surfaced via an appropriate UI
callback/logger (e.g., showToast or processLogger) instead of being discarded;
reference handleCopy and generatePlainText to locate where to implement the
guard and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 957ab0de-973b-4fb9-9a0c-2154424fd7b5
📒 Files selected for processing (7)
src/ui/report/view-models/useClusterSizingWizardViewModel.tssrc/ui/report/views/cluster-sizer/ClusterSizingWizard.tsxsrc/ui/report/views/cluster-sizer/TimeEstimationForm.tsxsrc/ui/report/views/cluster-sizer/TimeEstimationResult.tsxsrc/ui/report/views/cluster-sizer/__tests__/ClusterSizingWizard.test.tsxsrc/ui/report/views/cluster-sizer/constants.tssrc/ui/report/views/cluster-sizer/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/ui/report/views/cluster-sizer/tests/ClusterSizingWizard.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/report/views/cluster-sizer/ClusterSizingWizard.tsx
- src/ui/report/view-models/useClusterSizingWizardViewModel.ts
- src/ui/report/views/cluster-sizer/constants.ts
rh-gvincent
reviewed
Apr 14, 2026
- Send user-configurable params (transfer_rate_mbps, work_hours_per_day, post_migration_engineers) to the migration-estimation endpoint. - Replace read-only form with interactive sliders, show network-based and storage-offload results in bordered cards with popover assumptions and copy-as-plain-text support. Signed-off-by: Montse Ortega <mortegag@redhat.com>
…tion in the same place with some basic tests Signed-off-by: Montse Ortega <mortegag@redhat.com>
ammont82
force-pushed
the
ECOPROJECT-4354
branch
from
2979be9 to
a40743a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/report/view-models/useClusterSizingWizardViewModel.ts (1)
166-198:⚠️ Potential issue | 🟠 MajorIgnore stale migration-estimation responses.
This path can now be triggered repeatedly from interactive parameter changes, but unlike
calculateComplexityit has no request-order guard. A slower response for older slider values can overwrite the latest estimate or error state.♻️ Suggested fix
const [resetCounter, setResetCounter] = useState<number>(0); const latestComplexityRequestIdRef = useRef<string>(""); + const latestEstimationRequestIdRef = useRef<string>(""); const [estimationState, doCalculateEstimation] = useAsyncFn(async () => { setManualEstimationError(undefined); + const requestId = `${assessmentId}-${clusterId}-${Date.now()}`; + latestEstimationRequestIdRef.current = requestId; try { const result = await assessmentsStore.calculateMigrationEstimation({ id: assessmentId, migrationEstimationRequest: { clusterId, estimationSchema: ["network-based", "storage-offload"], params: estimationFormToParams(estimationFormValues), }, }); + if (latestEstimationRequestIdRef.current !== requestId) { + return; + } const hasSchemas = result?.estimation && Object.keys(result.estimation).length > 0; setMigrationEstimation(hasSchemas ? result : null); } catch (err) { + if (latestEstimationRequestIdRef.current !== requestId) { + return; + } if (err instanceof ResponseError) { const message = await err.response.text();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/report/view-models/useClusterSizingWizardViewModel.ts` around lines 166 - 198, The calculateMigrationEstimation async flow (inside useAsyncFn that returns estimationState and doCalculateEstimation) can produce stale responses that overwrite newer results; add a request-order guard by stamping each invocation with an incrementing requestId (or using an AbortController) inside doCalculateEstimation, pass that token into the async call, and when the response or error arrives only call setMigrationEstimation or setManualEstimationError if the token matches the latest request; ensure this guard wraps both the success path (after assessmentsStore.calculateMigrationEstimation resolves) and the error handling branch (before throwing or setting errors) so older/resolved promises cannot clobber current state.src/ui/report/views/cluster-sizer/TimeEstimationResult.tsx (1)
103-218: 🛠️ Refactor suggestion | 🟠 MajorMove the new formatting/serialization logic out of this view.
formatTotalDisplay,formatHoursSubtitle, assumption extraction, popover-body construction, andgeneratePlainTextare all business logic. Keeping them in the view makes this component harder to test and reuse, and it breaks the render-only rule for files undersrc/ui/**/views/.As per coding guidelines,
src/ui/**/views/**/*.{ts,tsx}: Views in src/ui/*/views/ must render only with no business logic, calling vm hook at top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/report/views/cluster-sizer/TimeEstimationResult.tsx` around lines 103 - 218, Extract all non-render business logic out of the view into a separate module (e.g., a presenter/util): move formatTotalDisplay, formatHoursSubtitle, formatDetailDuration, extractDetailText, getAssumptions, renderAssumptionItems, buildPopoverBody, and generatePlainText into that new file, export them, and import only the functions you need in TimeEstimationResult.tsx so the view file contains only top-level vm hook(s) and JSX rendering; update any references to the moved functions accordingly and add unit tests for the new module to cover formatting, assumption parsing, and plaintext generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/report/views/cluster-sizer/TimeEstimationResult.tsx`:
- Around line 110-127: Update pluralization in formatHoursSubtitle and
formatDetailDuration so they choose "hour" when the numeric hour value equals 1
and "hours" otherwise: for formatHoursSubtitle check if minH === maxH and then
use singular if minH === 1, otherwise plural; for ranges keep the en-dash format
but use singular only when both min and max equal 1; for formatDetailDuration
apply the same singular/plural logic for the single duration branch
(detail.duration -> h) and the min/max branch (minH/maxH), ensuring you use the
same toLocaleString formatting and only swap the trailing word between "hour"
and "hours" based on the numeric values.
In `@src/ui/report/views/cluster-sizer/timeUtils.ts`:
- Around line 78-89: formatHumanDuration currently renders singular units as "1
hours"/"1 days"/"1 months"; update the function to pluralize correctly by
computing the numeric value for each branch (months = hours / 730, days =
Math.ceil(hours / 24), hours as-is), then choose the unit label based on whether
that numeric value equals 1 (use "month"/"months", "day"/"days",
"hour"/"hours"); preserve the existing formatting for fractional months
(toFixed(1)) and integer months/days/hours but adjust the returned string to use
the singular label when the numeric value === 1.0 and the plural label
otherwise, referring to the formatHumanDuration function and its
months/days/hours variables.
---
Outside diff comments:
In `@src/ui/report/view-models/useClusterSizingWizardViewModel.ts`:
- Around line 166-198: The calculateMigrationEstimation async flow (inside
useAsyncFn that returns estimationState and doCalculateEstimation) can produce
stale responses that overwrite newer results; add a request-order guard by
stamping each invocation with an incrementing requestId (or using an
AbortController) inside doCalculateEstimation, pass that token into the async
call, and when the response or error arrives only call setMigrationEstimation or
setManualEstimationError if the token matches the latest request; ensure this
guard wraps both the success path (after
assessmentsStore.calculateMigrationEstimation resolves) and the error handling
branch (before throwing or setting errors) so older/resolved promises cannot
clobber current state.
In `@src/ui/report/views/cluster-sizer/TimeEstimationResult.tsx`:
- Around line 103-218: Extract all non-render business logic out of the view
into a separate module (e.g., a presenter/util): move formatTotalDisplay,
formatHoursSubtitle, formatDetailDuration, extractDetailText, getAssumptions,
renderAssumptionItems, buildPopoverBody, and generatePlainText into that new
file, export them, and import only the functions you need in
TimeEstimationResult.tsx so the view file contains only top-level vm hook(s) and
JSX rendering; update any references to the moved functions accordingly and add
unit tests for the new module to cover formatting, assumption parsing, and
plaintext generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8129a4d-207a-4df6-81fd-e246248fa305
📒 Files selected for processing (10)
src/ui/report/view-models/useClusterSizingWizardViewModel.tssrc/ui/report/views/cluster-sizer/ClusterSizingWizard.tsxsrc/ui/report/views/cluster-sizer/ComplexityResult.tsxsrc/ui/report/views/cluster-sizer/TimeEstimationForm.tsxsrc/ui/report/views/cluster-sizer/TimeEstimationResult.tsxsrc/ui/report/views/cluster-sizer/__tests__/ClusterSizingWizard.test.tsxsrc/ui/report/views/cluster-sizer/__tests__/timeUtils.test.tssrc/ui/report/views/cluster-sizer/constants.tssrc/ui/report/views/cluster-sizer/timeUtils.tssrc/ui/report/views/cluster-sizer/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ui/report/views/cluster-sizer/types.ts
- src/ui/report/views/cluster-sizer/constants.ts
- src/ui/report/views/cluster-sizer/ClusterSizingWizard.tsx
- src/ui/report/views/cluster-sizer/TimeEstimationForm.tsx
Collaborator
|
Thank you for addressing my comments |
…raction Signed-off-by: Montse Ortega <mortegag@redhat.com>
Collaborator
Author
|
/approve |
|
[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 |
Collaborator
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
flex_parameters_2.mp4
Summary by CodeRabbit
New Features
Improvements
Tests