EDM-3687: Allow user to go back to step to fix problems#625
Open
celdrake wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
Summary by CodeRabbit
WalkthroughAdds a shared wizard-step gating utility used across many wizards, refactors wizard components to use it (computing ordered/valid step IDs from Formik where needed), and adds a Cypress E2E test plus a page-object getter for the wizard Back button. ChangesWizard Step Logic Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/cypress/e2e/fleets/createFleet.cy.ts (1)
71-73: Assert the validation state is still active after returning.The test confirms the invalid value is preserved, but it should also confirm the invalid step still disables Next after re-entry.
Suggested test assertion
createFleetWizardPage.backFleetWizardButton.click(); createFleetWizardPage.nextFleetWizardButton.should('be.enabled').click(); createFleetWizardPage.newFleetSystemImageField.should('be.visible').should('have.value', 'invalid!oci'); + createFleetWizardPage.nextFleetWizardButton.should('be.disabled');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cypress/e2e/fleets/createFleet.cy.ts` around lines 71 - 73, The test currently checks that createFleetWizardPage.newFleetSystemImageField retains the invalid value but doesn't assert the wizard step remains invalid; after navigating back (where createFleetWizardPage.nextFleetWizardButton.should('be.enabled').click() is used to advance), add an assertion that createFleetWizardPage.nextFleetWizardButton is disabled when re-entering the step with the invalid value (e.g., assert .should('be.disabled') before attempting to proceed) so the invalid validation state is enforced; reference createFleetWizardPage.nextFleetWizardButton and createFleetWizardPage.newFleetSystemImageField to locate where to add the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/cypress/e2e/fleets/createFleet.cy.ts`:
- Around line 71-73: The test currently checks that
createFleetWizardPage.newFleetSystemImageField retains the invalid value but
doesn't assert the wizard step remains invalid; after navigating back (where
createFleetWizardPage.nextFleetWizardButton.should('be.enabled').click() is used
to advance), add an assertion that createFleetWizardPage.nextFleetWizardButton
is disabled when re-entering the step with the invalid value (e.g., assert
.should('be.disabled') before attempting to proceed) so the invalid validation
state is enforced; reference createFleetWizardPage.nextFleetWizardButton and
createFleetWizardPage.newFleetSystemImageField to locate where to add the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2baa480f-44ef-41a2-9275-cf855b783eb0
📒 Files selected for processing (3)
libs/cypress/e2e/fleets/createFleet.cy.tslibs/cypress/pages/CreateFleetWizardPage.tslibs/ui-components/src/components/Fleet/CreateFleet/CreateFleetWizard.tsx
rawagner
reviewed
Apr 28, 2026
Comment on lines
+62
to
+65
| const stepIdx = orderedIds.findIndex((orderedStepId) => orderedStepId === stepId); | ||
| return orderedIds.some((orderedId, orderedStepIdx) => { | ||
| return orderedStepIdx < stepIdx && !validStepIds.includes(orderedId); | ||
| }); |
Collaborator
There was a problem hiding this comment.
the condition is quite confusing TBH.
Can't we just pass a current step ID to this function and return early ?
Suggested change
| const stepIdx = orderedIds.findIndex((orderedStepId) => orderedStepId === stepId); | |
| return orderedIds.some((orderedId, orderedStepIdx) => { | |
| return orderedStepIdx < stepIdx && !validStepIds.includes(orderedId); | |
| }); | |
| const isDisabledStep = (currentStepId: string, stepId: string, validStepIds: string[]) => { | |
| if (currentStepId === stepId) { | |
| return false; //current step is never disabled | |
| } | |
| const validIndex = validStepIds.indexOf(stepId); | |
| return validIndex === -1 || validIndex !== orderedIds.indexOf(stepId); | |
| } |
also we use this condition in multiple wizards - maybe its worth making it a common func.
Collaborator
Author
There was a problem hiding this comment.
With this change, the initial bug would not be fixed: When on step 2 with some invalid input, users can go back to step 1. And now clicking on "Next" does nothing since the step with the error is a "future step" from the POV of the new current step.
I agree that it would be better to unify the check for the disabled steps in the different Wizards.
I can see two options:
- Use in the CreateFleetWizard the pattern we have in the other wizards, that for step N+2, they check the steps N, N+1.
- Make this function reusable and use it in all the wizards.
Personally I find the second one better in the long run, even though it's harder to understand. We could properly document it and it would not require new custom logic for any new Wizards.
Collaborator
There was a problem hiding this comment.
Ok, sounds good. Lets go with the second one then.
Collaborator
Author
There was a problem hiding this comment.
Thanks.
I think it should be clearer now.
celdrake
force-pushed
the
EDM-3687-enable-create-fleet-wizard-step
branch
from
9f2b94a to
dc63d36
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/ui-components/src/utils/wizards.ts (1)
1-7: ⚡ Quick winAdd focused tests for the shared step-gating helper.
This function now controls navigation gating across multiple wizards, so a small table-driven test suite here would materially reduce regression risk (especially for “current step stays enabled when invalid” and “future step blocked when any prior step is invalid”).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-components/src/utils/wizards.ts` around lines 1 - 7, Add a focused, table-driven test suite for isWizardStepDisabled that exercises gating behavior across orderedStepIds and validStepIds: include cases where (1) the current step is invalid but should remain enabled (stepId present and invalid but no prior invalid steps => expect false), (2) a future step is blocked when any prior step is invalid (expect true when any orderedId with index < stepId is missing from validStepIds), (3) all steps valid (expect false), and (4) the first step behavior (expect false). Implement tests that call isWizardStepDisabled with named scenarios and assert the Boolean result for each to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/ui-components/src/utils/wizards.ts`:
- Around line 1-7: Add a focused, table-driven test suite for
isWizardStepDisabled that exercises gating behavior across orderedStepIds and
validStepIds: include cases where (1) the current step is invalid but should
remain enabled (stepId present and invalid but no prior invalid steps => expect
false), (2) a future step is blocked when any prior step is invalid (expect true
when any orderedId with index < stepId is missing from validStepIds), (3) all
steps valid (expect false), and (4) the first step behavior (expect false).
Implement tests that call isWizardStepDisabled with named scenarios and assert
the Boolean result for each to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 11cf7821-f4d6-4209-a467-8fc185eb103d
📒 Files selected for processing (12)
libs/cypress/e2e/fleets/createFleet.cy.tslibs/cypress/pages/CreateFleetWizardPage.tslibs/ui-components/src/components/Catalog/AddCatalogItemWizard/AddCatalogItemWizard.tsxlibs/ui-components/src/components/Catalog/EditWizard/EditAppWizard.tsxlibs/ui-components/src/components/Catalog/EditWizard/EditOsWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsxlibs/ui-components/src/components/Fleet/CreateFleet/CreateFleetWizard.tsxlibs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/CreateImageBuildWizard.tsxlibs/ui-components/src/components/ImportResourceWizard/ImportResourceWizard.tsxlibs/ui-components/src/utils/wizards.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/cypress/e2e/fleets/createFleet.cy.ts
- libs/cypress/pages/CreateFleetWizardPage.ts
celdrake
force-pushed
the
EDM-3687-enable-create-fleet-wizard-step
branch
from
dc63d36 to
11ba06b
Compare
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.
Fixes the issue in the CreateFleetWizard whereby when a validation error was present in step N, and user moved to N-1, then they couldn't move back to step N.