Deprecate old Wizard (HMS-10754)#4488
Open
regexowl wants to merge 5 commits into
Open
Conversation
Collaborator
Author
|
/jira-epic RHIN-2262 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #4488 +/- ##
==========================================
+ Coverage 75.34% 76.02% +0.68%
==========================================
Files 229 224 -5
Lines 7446 7188 -258
Branches 2768 2657 -111
==========================================
- Hits 5610 5465 -145
+ Misses 1578 1482 -96
+ Partials 258 241 -17
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
3661d27 to
cfd24cd
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The query parameter handling for
release,target, andarch(initializing state on open and cleaning them up on close) is now duplicated across multipleuseEffect/helper blocks; consider extracting this into a small utility or hook so the behavior is defined in one place and easier to maintain. - The large JSX blocks that build the Base settings and Advanced settings steps repeat the same pattern of conditionally rendering multiple substeps with dividers; extracting a small helper/component to generate these sections would improve readability and make future adjustments to step ordering or layout less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The query parameter handling for `release`, `target`, and `arch` (initializing state on open and cleaning them up on close) is now duplicated across multiple `useEffect`/helper blocks; consider extracting this into a small utility or hook so the behavior is defined in one place and easier to maintain.
- The large JSX blocks that build the Base settings and Advanced settings steps repeat the same pattern of conditionally rendering multiple substeps with dividers; extracting a small helper/component to generate these sections would improve readability and make future adjustments to step ordering or layout less error-prone.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/CreateImageWizard.tsx" line_range="359-360" />
<code_context>
+ const handleClose = () => {
+ dispatch(closeWizardModal());
+ dispatch(initializeWizard());
+ hasTrackedInitialStepRef.current = false;
+ hasTrackedWizardOpenedRef.current = false;
+
+ if (
</code_context>
<issue_to_address>
**issue (bug_risk):** Reset `hasInitialized` when closing the wizard so create-mode setup runs correctly on subsequent opens.
Because the create-mode setup is guarded by `hasInitialized.current`, closing the wizard without resetting this ref prevents the initialization (including registration defaults and URL param handling) from running on the next open. Add `hasInitialized.current = false;` in `handleClose` so reopening the create wizard triggers a full re-init.
</issue_to_address>
### Comment 2
<location path="src/Components/CreateImageWizard/CreateImageWizard.tsx" line_range="463" />
<code_context>
- id='step-review'
- footer={<ReviewWizardFooter />}
- >
+ <Form>
+ <Content>
+ <Title headingLevel='h1' size='xl'>
</code_context>
<issue_to_address>
**issue (bug_risk):** Prevent default form submission on the base settings step to avoid accidental page reloads when pressing Enter.
On the other steps, the `<Form>` uses `onSubmit={handleFormSubmit}` (which calls `event.preventDefault()`), but the base settings `<Form>` has no `onSubmit` handler. This makes Enter behave differently here than on other steps and can trigger a native submit. To align behavior, wire this `<Form>` through `onSubmit={handleFormSubmit}` as well.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
Can take a look at this in a separate PR, it's out of scope here. |
Collaborator
Author
|
/retest |
This removes old Wizard from the code base and updates all references in the code base.
This moves the new wizard files to the old wizard location to clean up the structure.
When navigating to `/imagewizard/invalid-id` the wizard opened in a permanent loading state. This updates the behaviour to: 1. the wizard is closed 2. the invalid id gets cleared from the path 3. a notification informing the user about an invalid id gets dispatched
This ensures the behaviour is consistent with the other steps.
This resets the reference for Wizard initialization, so when the Wizard gets closed and re-opened, it will be initialized with fresh values.
1629af1 to
12c12ff
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
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.
This removes old Wizard from the code base and updates all references in the code base.
JIRA: HMS-10754