EDM-3687: Allow user to go back to step to fix problems#625
EDM-3687: Allow user to go back to step to fix problems#625celdrake wants to merge 1 commit intoflightctl:mainfrom
Conversation
WalkthroughA new Cypress test validates the Create fleet wizard's back navigation and validation error handling. Supporting changes include a page object accessor for the Back button and updated wizard step-disable logic to allow the current step to remain enabled despite validation errors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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.
🧹 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
| const stepIdx = orderedIds.findIndex((orderedStepId) => orderedStepId === stepId); | ||
| return orderedIds.some((orderedId, orderedStepIdx) => { | ||
| return orderedStepIdx < stepIdx && !validStepIds.includes(orderedId); | ||
| }); |
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 ?
| 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.
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.
2 participants
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.
Summary by CodeRabbit
Tests
Bug Fixes