EDM-3529: Properly set default for new device img. Do not require con…#574
EDM-3529: Properly set default for new device img. Do not require con…#574rawagner merged 1 commit intoflightctl:mainfrom
Conversation
…firmation when clicking on close
WalkthroughThese changes modify the InstallWizard component flow by conditionally hiding a leave confirmation dialog when targeting a new device, and refactoring artifact filtering in the SelectTargetStep to use only artifacts present in the current version's references. 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)
📝 Coding Plan
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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx`:
- Around line 227-230: The current effect only sets a default when
values.deploymentTarget is falsy, leaving a stale deploymentTarget if the
selected version's artifacts no longer include that type; update the effect
watching artifacts and values.deploymentTarget to check whether
values.deploymentTarget exists in artifacts.map(a => a.type) and if not call
setFieldValue('deploymentTarget', artifacts.length ? artifacts[0].type :
undefined) (or null) so the field is reset to a valid option or cleared;
reference values.deploymentTarget, artifacts, and setFieldValue in the
SelectTargetStep effect when implementing this change.
- Around line 220-223: The current render builds the artifact list by calling
.sort() directly on catalogItem.spec.artifacts (used in SelectTargetStep), which
mutates the source; change the pipeline that produces the sorted list to operate
on an immutable copy (e.g., clone via slice() or spread) before calling sort,
keep the existing filter that uses versionRefs (from const versionRefs =
catalogItem.spec.versions.find(...)?), and ensure you still call
getArtifactLabel(t, a.type, a.name) for comparison—this prevents side effects
across consumers of catalogItem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50a0a671-d05f-4374-bdcc-f60052f9ccb9
📒 Files selected for processing (2)
libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
| const versionRefs = catalogItem.spec.versions.find((v) => v.version === values.version)?.references || {}; | ||
| return catalogItem.spec.artifacts | ||
| ?.sort((a, b) => getArtifactLabel(t, a.type, a.name).localeCompare(getArtifactLabel(t, b.type, b.name))) | ||
| .filter((a) => Object.keys(versionRefs).includes(a.type)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this in-place sort usage in the changed file
rg -nP --type=tsx 'catalogItem\.spec\.artifacts\s*\??\.sort\('Repository: flightctl/flightctl-ui
Length of output: 93
🏁 Script executed:
fd "SelectTargetStep.tsx" --type fRepository: flightctl/flightctl-ui
Length of output: 148
🏁 Script executed:
wc -l libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsxRepository: flightctl/flightctl-ui
Length of output: 152
🏁 Script executed:
sed -n '210,260p' libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsxRepository: flightctl/flightctl-ui
Length of output: 2083
🏁 Script executed:
sed -n '1,220p' libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx | tail -30Repository: flightctl/flightctl-ui
Length of output: 1049
🏁 Script executed:
sed -n '200,225p' libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsxRepository: flightctl/flightctl-ui
Length of output: 917
🏁 Script executed:
# Check if catalogItem type definition confirms spec.artifacts is array from API
rg "spec\s*:\s*{" libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx -A 10Repository: flightctl/flightctl-ui
Length of output: 153
🏁 Script executed:
# Search for CatalogItem type definition
rg "type CatalogItem|interface CatalogItem" -A 5Repository: flightctl/flightctl-ui
Length of output: 6812
🏁 Script executed:
cat libs/types/alpha/models/CatalogItemSpec.tsRepository: flightctl/flightctl-ui
Length of output: 1888
🏁 Script executed:
# Check if catalogItem comes from API/parent by looking at how NewDeviceTarget is used
rg "NewDeviceTarget" -B 5 -A 2 | head -40Repository: flightctl/flightctl-ui
Length of output: 2178
Avoid mutating catalogItem.spec.artifacts in render-time memo.
Line 222 calls .sort() directly on catalogItem.spec.artifacts, which mutates the source array and can cause side effects across consumers of the same object.
Suggested fix
- return catalogItem.spec.artifacts
- ?.sort((a, b) => getArtifactLabel(t, a.type, a.name).localeCompare(getArtifactLabel(t, b.type, b.name)))
- .filter((a) => Object.keys(versionRefs).includes(a.type));
+ const refTypes = new Set(Object.keys(versionRefs));
+ return [...catalogItem.spec.artifacts]
+ .filter((a) => refTypes.has(a.type))
+ .sort((a, b) => getArtifactLabel(t, a.type, a.name).localeCompare(getArtifactLabel(t, b.type, b.name)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const versionRefs = catalogItem.spec.versions.find((v) => v.version === values.version)?.references || {}; | |
| return catalogItem.spec.artifacts | |
| ?.sort((a, b) => getArtifactLabel(t, a.type, a.name).localeCompare(getArtifactLabel(t, b.type, b.name))) | |
| .filter((a) => Object.keys(versionRefs).includes(a.type)); | |
| const versionRefs = catalogItem.spec.versions.find((v) => v.version === values.version)?.references || {}; | |
| const refTypes = new Set(Object.keys(versionRefs)); | |
| return (catalogItem.spec.artifacts || []) | |
| .filter((a) => refTypes.has(a.type)) | |
| .sort((a, b) => getArtifactLabel(t, a.type, a.name).localeCompare(getArtifactLabel(t, b.type, b.name))); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx`
around lines 220 - 223, The current render builds the artifact list by calling
.sort() directly on catalogItem.spec.artifacts (used in SelectTargetStep), which
mutates the source; change the pipeline that produces the sorted list to operate
on an immutable copy (e.g., clone via slice() or spread) before calling sort,
keep the existing filter that uses versionRefs (from const versionRefs =
catalogItem.spec.versions.find(...)?), and ensure you still call
getArtifactLabel(t, a.type, a.name) for comparison—this prevents side effects
across consumers of catalogItem.
| if (!values.deploymentTarget && artifacts.length) { | ||
| setFieldValue('deploymentTarget', artifacts[0].type); | ||
| } | ||
| }, [values, artifacts, setFieldValue]); | ||
| }, [artifacts, values.deploymentTarget, setFieldValue]); |
There was a problem hiding this comment.
Reset deploymentTarget when it becomes invalid for the selected version.
Line 227 only sets a default when deploymentTarget is empty. If version changes and the currently selected type is no longer present in artifacts, stale state remains.
Suggested fix
React.useEffect(() => {
- if (!values.deploymentTarget && artifacts.length) {
- setFieldValue('deploymentTarget', artifacts[0].type);
- }
+ const isCurrentSelectionValid = artifacts.some((a) => a.type === values.deploymentTarget);
+ if (!isCurrentSelectionValid) {
+ setFieldValue('deploymentTarget', artifacts[0]?.type);
+ }
}, [artifacts, values.deploymentTarget, setFieldValue]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx`
around lines 227 - 230, The current effect only sets a default when
values.deploymentTarget is falsy, leaving a stale deploymentTarget if the
selected version's artifacts no longer include that type; update the effect
watching artifacts and values.deploymentTarget to check whether
values.deploymentTarget exists in artifacts.map(a => a.type) and if not call
setFieldValue('deploymentTarget', artifacts.length ? artifacts[0].type :
undefined) (or null) so the field is reset to a valid option or cleared;
reference values.deploymentTarget, artifacts, and setFieldValue in the
SelectTargetStep effect when implementing this change.
2 participants
…firmation when clicking on close
Summary by CodeRabbit
Release Notes