Wizard: let the target radios drive the official image (HMS-11166) - #4737
Wizard: let the target radios drive the official image (HMS-11166)#4737lucasgarfield wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/ImageOutput/components/ImageSourceSelect/OnPrem/ContainerSection.tsx" line_range="89-91" />
<code_context>
+
+ // Local images can be removed outside the wizard (e.g. podman rmi),
+ // so bypass the cache and re-check whenever this section mounts.
+ const { data: imageExists } = useGetImageExistsQuery(
+ { reference: reference! },
+ { skip: !reference, refetchOnMountOrArgChange: true },
+ );
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid using a non-null assertion for reference when the hook is skipped
`reference!` is unsafe here: `reference` is allowed to be undefined and the query is already guarded via `skip: !reference`. The non-null assertion hides that possibility and makes future refactors more error-prone. Prefer either:
- Passing a safe fallback (e.g. `reference ?? ''`), or
- Only creating the args when `reference` is truthy (e.g. `reference ? { reference } : skipToken`).
This keeps the types accurate and avoids relying on `!` in the hook args.
Suggested implementation:
```typescript
// Local images can be removed outside the wizard (e.g. podman rmi),
// so bypass the cache and re-check whenever this section mounts.
+ const { data: imageExists } = useGetImageExistsQuery(
+ reference ? { reference } : skipToken,
+ { refetchOnMountOrArgChange: true },
+ );
+
```
You also need to ensure `skipToken` is imported in this file. If not already present, add:
```ts
import { skipToken } from '@reduxjs/toolkit/query';
```
near the other imports (or from the specific RTK Query entrypoint your project uses, such as `@reduxjs/toolkit/query/react` if that's the convention in this codebase).
</issue_to_address>
### Comment 2
<location path="src/store/slices/wizard/listeners.ts" line_range="121-125" />
<code_context>
+ return;
+ }
+
+ const imageTypes = selectImageTypes(state);
+ if (imageTypes.length === 0) {
+ return;
+ }
+ const targetType = imageTypes[0];
+
+ const currentRef = selectImageSource(state);
</code_context>
<issue_to_address>
**suggestion:** Clarify or guard the assumption that only a single image type is relevant
`resolveOfficialImage` assumes `selectImageTypes` returns exactly one type (`const targetType = imageTypes[0];`). This matches the current single-select UI, but if multi-select is ever introduced, we’ll silently ignore additional types and always use the first. Please either enforce the single-select assumption (e.g., bail out or handle explicitly when `imageTypes.length > 1`) or add a clear guard/assertion so future changes don’t accidentally rely on the first element arbitrarily.
```suggestion
const imageTypes = selectImageTypes(state);
// This logic assumes that exactly one image type is selected.
// If the UI is ever extended to support multi-select, this guard
// should be revisited so we don't arbitrarily pick the first type.
if (imageTypes.length !== 1) {
return;
}
const [targetType] = imageTypes;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const { data: imageExists } = useGetImageExistsQuery( | ||
| { reference: reference! }, | ||
| { skip: !reference, refetchOnMountOrArgChange: true }, |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid using a non-null assertion for reference when the hook is skipped
reference! is unsafe here: reference is allowed to be undefined and the query is already guarded via skip: !reference. The non-null assertion hides that possibility and makes future refactors more error-prone. Prefer either:
- Passing a safe fallback (e.g.
reference ?? ''), or - Only creating the args when
referenceis truthy (e.g.reference ? { reference } : skipToken).
This keeps the types accurate and avoids relying on ! in the hook args.
Suggested implementation:
// Local images can be removed outside the wizard (e.g. podman rmi),
// so bypass the cache and re-check whenever this section mounts.
+ const { data: imageExists } = useGetImageExistsQuery(
+ reference ? { reference } : skipToken,
+ { refetchOnMountOrArgChange: true },
+ );
+You also need to ensure skipToken is imported in this file. If not already present, add:
import { skipToken } from '@reduxjs/toolkit/query';near the other imports (or from the specific RTK Query entrypoint your project uses, such as @reduxjs/toolkit/query/react if that's the convention in this codebase).
| const imageTypes = selectImageTypes(state); | ||
| if (imageTypes.length === 0) { | ||
| return; | ||
| } | ||
| const targetType = imageTypes[0]; |
There was a problem hiding this comment.
suggestion: Clarify or guard the assumption that only a single image type is relevant
resolveOfficialImage assumes selectImageTypes returns exactly one type (const targetType = imageTypes[0];). This matches the current single-select UI, but if multi-select is ever introduced, we’ll silently ignore additional types and always use the first. Please either enforce the single-select assumption (e.g., bail out or handle explicitly when imageTypes.length > 1) or add a clear guard/assertion so future changes don’t accidentally rely on the first element arbitrarily.
| const imageTypes = selectImageTypes(state); | |
| if (imageTypes.length === 0) { | |
| return; | |
| } | |
| const targetType = imageTypes[0]; | |
| const imageTypes = selectImageTypes(state); | |
| // This logic assumes that exactly one image type is selected. | |
| // If the UI is ever extended to support multi-select, this guard | |
| // should be revisited so we don't arbitrarily pick the first type. | |
| if (imageTypes.length !== 1) { | |
| return; | |
| } | |
| const [targetType] = imageTypes; |
cc9dcdf to
f804dd5
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## lucas/cockpit/3-official-images #4737 +/- ##
===================================================================
+ Coverage 78.98% 79.22% +0.24%
===================================================================
Files 264 264
Lines 7033 7004 -29
Branches 2548 2576 +28
===================================================================
- Hits 5555 5549 -6
+ Misses 1381 1358 -23
Partials 97 97
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
f804dd5 to
9ba64b5
Compare
9ba64b5 to
bc382f9
Compare
585f043 to
f804dd5
Compare
f804dd5 to
ee2294d
Compare
ee2294d to
f83134b
Compare
The official on-prem images now include an AWS (ec2) variant next to the guest image, published under a shared name. On-prem image mode always offers the official images' output types as target environment radios — the same list whether or not an image has been selected yet — and since each type maps to exactly one official image, selecting a type now fully determines the image: a resolveOfficialImage listener on changeImageTypes resolves the image source and distribution, following the same pattern as the other cross-slice listeners. The resolver is guarded by IS_ON_PREMISE, a build-time constant, so the hosted bundle dead-code-eliminates it, and it only acts in image mode with the official source type — package mode dispatches changeImageTypes too (the architecture filter), and unknown references and the local source type are left untouched. Since the environments no longer derive from the selected image or the podman image list, drop the imageSource narrowing from the derived distributions endpoint and the selectImageSourceFilter selector. Resolves #4715 Resolves #4721
With the target radios fully determining the official image, the image dropdown had nothing left to choose - it duplicated the radios with a second control. Replace it with PatternFly's read-only boxed form control: the container reference in a copyable field (useful when pulling manually, which the logged-out flow suggests), with the human-readable image name beneath it. Before a target environment is picked the section shows a short hint instead. The field, its pull button, the existence check, and the pull validation collapse into one ContainerSection component; ImageSelect and the selection error are gone, and OfficialImageSource no longer dispatches anything. When local images become selectable the same form-group slot swaps to a real select, so the layout deliberately reads as a form field.
f83134b to
1b3b16f
Compare
|
Replaced by #4756 |
Target environment radios now fully determine the selected official image; the dropdown is replaced by a read-only, copyable container reference.
Stack created with GitHub Stacks CLI • Give Feedback 💬