Extract output subslice from wizard store#4566
Open
kingsleyzissou wants to merge 9 commits into
Open
Conversation
Collaborator
Author
|
These are all pretty mechanical changes. The only super new thing is adding listeners for some of the changes and moving where those live |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The test helpers for listener effects (e.g. the
createListenerApiimplementations infilterImageTypes.test.tsandregisterLater.test.ts) are nearly identical; consider extracting a shared factory into a common test utility to reduce duplication and keep listener tests consistent. - In
listeners.ts,filterImageTypescurrently casts the RootState toanywhen callingbackendApi.endpoints.getArchitectures.select; if possible, tightening this typing (e.g. via a discriminated union or a typed wrapper aroundbackendApi) would avoid the escape hatch and make the middleware safer to refactor.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test helpers for listener effects (e.g. the `createListenerApi` implementations in `filterImageTypes.test.ts` and `registerLater.test.ts`) are nearly identical; consider extracting a shared factory into a common test utility to reduce duplication and keep listener tests consistent.
- In `listeners.ts`, `filterImageTypes` currently casts the RootState to `any` when calling `backendApi.endpoints.getArchitectures.select`; if possible, tightening this typing (e.g. via a discriminated union or a typed wrapper around `backendApi`) would avoid the escape hatch and make the middleware safer to refactor.
## Individual Comments
### Comment 1
<location path="src/store/slices/wizard/listeners.ts" line_range="46-55" />
<code_context>
+ );
+};
+
+// This was previously a mutation inside the changeDistribution reducer.
+// As a listener it fires *after* the reducer commits, so any other listener
+// reading the `registration.type` in the same tick will observe the old value
+// until this dispatch is processed.
+export const registerLater: WizardListenerEffect = (_action, listenerApi) => {
+ const state = listenerApi.getState();
+ const distribution = selectDistribution(state);
+
+ if (process.env.IS_ON_PREMISE && !isRhel(distribution)) {
+ listenerApi.dispatch(changeRegistrationType('register-later'));
+ }
+};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Moving registration-type changes into a listener may introduce ordering subtleties
Previously, `changeDistribution` updated `output.distribution` and `registration.type` atomically in one reducer. With `registerLater` as a listener, `registration.type` now changes in a separate dispatch after the reducer completes. This can temporarily desynchronize `distribution` and `registration.type` if other listeners or UI code read both in the same tick. If that ordering matters, either move this back into the reducer or ensure downstream consumers only react to `registration.type` changes and don’t assume atomic updates with `distribution`.
Suggested implementation:
```typescript
```
1. In the `changeDistribution` reducer (likely in `src/store/slices/wizard/slice.ts` or a similar file), reintroduce the registration-type update that was previously moved into the `registerLater` listener, e.g.:
- When `process.env.IS_ON_PREMISE` is true and the new distribution is not RHEL, set `registration.type = 'register-later'` in the same reducer branch that updates `output.distribution`.
2. Ensure that any removed imports used only by `registerLater` (such as `selectDistribution`, `isRhel`, `WizardListenerEffect`, or `changeRegistrationType` if it was only used here) are cleaned up from `listeners.ts` to avoid unused-import warnings.
3. Verify that no other listeners or UI components now assume that `registration.type` is driven by a separate `changeRegistrationType('register-later')` dispatch; they should instead rely on the atomic update inside `changeDistribution`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4566 +/- ##
==========================================
- Coverage 76.70% 76.65% -0.06%
==========================================
Files 248 247 -1
Lines 7131 7114 -17
Branches 2650 2641 -9
==========================================
- Hits 5470 5453 -17
Misses 1436 1436
Partials 225 225
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
e8c0bc5 to
05099df
Compare
Follow the pattern established by compliance, details, and filesystem subslices so output state has its own module boundary.
Move isImageType out of the top-level typeGuards module into the output subslice where the ImageTypes it narrows are used. Replace the manually maintained type unions and runtime Sets in useTargetEnvironmentCategories with as-const arrays in the output subslice. Derive types with indexed access and typeguards with array inclusion checks, giving a single source of truth. Remove the standalone isRhel utility in favour of a co-located typeguard over RHEL_DISTRIBUTIONS.
The output slice's initial state was inlined in the main wizard slice file. Moving it to its own state.ts follows the pattern used by other sub-slices and keeps the wizard slice focused on composition.
The output selectors were defined in the monolithic wizard slice file alongside unrelated selectors. Co-locating them with the rest of the output module keeps things consistent and easier to find.
The image-type filtering listeners were defined inline in the store entry point alongside unrelated store setup. Moving the middleware infrastructure into store/middleware/ and the effect logic into slices/wizard/listeners.ts keeps the store root focused on reducer composition and middleware wiring.
The on-premise register-later logic was a synchronous mutation inside the changeDistribution reducer, coupling output state to registration state. Moving it to a listener effect keeps the reducer pure and avoids cross-slice writes.
The output reducers were the last remaining inline reducers in the parent wizard slice. Moving them to output/slice.ts follows the same pattern used by compliance, details, and filesystem subslices. Tests are updated to import combinedInitialState (which includes the output key) instead of the core initialState which no longer carries it.
05099df to
41b7fd1
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.
Extract output subslice from wizard store
Summary
Extracts the
outputsubdomain (image types, architecture, distribution) from the monolithic wizard slice into a dedicated subslice, following the same pattern established bycompliance,details, andfilesystem. This reduces the size of the wizard slice and co-locates output-related types, selectors, typeguards, constants, and tests.Changes
store/slices/wizard/output/with a barrel re-exportfilterImageTypes,registerLater) fromstore/index.tsintostore/middleware/andslices/wizard/listeners.ts, deduplicating the previously copy-pastedfilterImageTypeslogic forchangeArchitectureandchangeDistributionisRhelfrom a plain boolean function to a type predicate (distro is RhelDistribution) and addisPublicCloud/isPrivateCloudpredicates, replacing inlineSet.has()checks inuseTargetEnvironmentCategoriesloadWizardStatefallback for blueprints serialised before the subslice existedno-restricted-importsrule foroutput/*internals to enforce barrel discipline