EDM-2925: Adding helm application type#457
Conversation
WalkthroughAdds Helm application support across types, UI, validation, serialization, and translations: new AppTypeHelm, Helm form component, Helm fields in specs, YAML parsing/validation, UploadField accessibility changes, and label/utility helpers. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In `@libs/i18n/locales/en/translation.json`:
- Line 391: Fix the typo in the translation entry whose key/value is "Order of
precedence: Files are applied in the ordered listed. If the same parameter is
defined in multiple files, the value in the last file takes precedence and
overrides previous values." — change "ordered listed" to "listed order" (or
"order listed") consistently in both the JSON key and its value so the
user-facing string reads correctly (e.g., "Order of precedence: Files are
applied in the listed order. ...").
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx`:
- Around line 60-66: The reset logic misses the case where appType is Helm but
specType isn't OCI_IMAGE, so switching from INLINE to Helm won't mark the app as
incomplete; update the Helm detection and incomplete check: modify isHelm (from
isHelmImageAppForm) to also return true when app.appType === AppType.HELM and
specType !== AppSpecType.OCI_IMAGE, and keep isHelmIncomplete = isHelm &&
!('valuesFiles' in app) (or add an explicit check for appType === AppType.HELM
in isHelmIncomplete) so shouldResetApp becomes true in that transition
(referencing isHelmImageAppForm, isHelmIncomplete, shouldResetApp, specType,
AppSpecType, and app.appType).
In `@libs/ui-components/src/components/form/UploadField.tsx`:
- Line 92: The aria-label is currently applied to FormGroup in UploadField; move
it to the actual control by removing aria-label from the FormGroup (keep
fieldId/isRequired) and pass aria-label into the FileUpload component instead
(ensure prop name matches the local variable, e.g., ariaLabel). Update the JSX
in UploadField so FileUpload receives ariaLabel and the FormGroup only handles
fieldId/isRequired/label concerns to follow PatternFly accessibility best
practices.
🧹 Nitpick comments (3)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
485-488: Consider using empty object fallback instead of whitespace string for YAML parsing.The
' '(whitespace) fallback foryaml.loadparses tonull, which then gets stringified as"null"byJSON.stringify. This could cause false positives when comparing against an empty{}fromimageApp.values || {}.♻️ Suggested improvement
- const updatedValues = yaml.load(updatedApp.valuesYaml || ' ') as Record<string, unknown>; - if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) { + const updatedValues = updatedApp.valuesYaml + ? (yaml.load(updatedApp.valuesYaml) as Record<string, unknown>) + : {}; + if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues || {})) {libs/ui-components/src/types/deviceSpec.ts (1)
125-144: Type guards correctly updated forAppFormunion.The signature change from
AppBasetoAppFormis correct since these guards discriminate members of theAppFormunion type.However,
getAppIdentifier(lines 155-164 in this file) doesn't have an explicit branch forHelmImageAppForm. Since Helm apps have optionalnameand mandatoryimage, they should follow the same pattern as other image apps.♻️ Suggested fix for getAppIdentifier
export const getAppIdentifier = (app: AppForm) => { if (isSingleContainerAppForm(app)) { return app.name || app.image; } - if (isQuadletImageAppForm(app) || isComposeImageAppForm(app)) { + if (isQuadletImageAppForm(app) || isComposeImageAppForm(app) || isHelmImageAppForm(app)) { return app.name || app.image; } // Name is mandatory for inline applications return app.name; };libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx (1)
144-151: Consider addingmaxFileByteslimit for consistency.The
UploadFieldhere doesn't specifymaxFileBytes, whileApplicationInlineFormusesMAX_INLINE_FILE_SIZE_BYTES(1MB). Large YAML values could cause issues.♻️ Optional: Add file size limit
- <UploadField ariaLabel={t('Inline values')} name={`${appFieldName}.valuesYaml`} isDisabled={isReadOnly} /> + <UploadField + ariaLabel={t('Inline values')} + name={`${appFieldName}.valuesYaml`} + isDisabled={isReadOnly} + maxFileBytes={1024 * 1024} // 1MB limit + />
| "Delete values file": "Delete values file", | ||
| "Fill in the existing values files before you can add more values files.": "Fill in the existing values files before you can add more values files.", | ||
| "Add values file": "Add values file", | ||
| "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", |
There was a problem hiding this comment.
Fix typo in user-facing string.
"ordered listed" should be "order listed" or "listed order".
✏️ Proposed fix
-"Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.",
+"Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.",📝 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.
| "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", | |
| "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", |
🤖 Prompt for AI Agents
In `@libs/i18n/locales/en/translation.json` at line 391, Fix the typo in the
translation entry whose key/value is "Order of precedence: Files are applied in
the ordered listed. If the same parameter is defined in multiple files, the
value in the last file takes precedence and overrides previous values." — change
"ordered listed" to "listed order" (or "order listed") consistently in both the
JSON key and its value so the user-facing string reads correctly (e.g., "Order
of precedence: Files are applied in the listed order. ...").
There was a problem hiding this comment.
Will fix in a separate PR
There was a problem hiding this comment.
@celdrake, sounds good! Thanks for catching that—I'll look for the fix in the follow-up PR.
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 207
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewDeviceStep.tsx:55-55
Timestamp: 2025-02-04T09:04:36.106Z
Learning: The codebase has automated CI checks ("lint" step) that verify translation coverage for all text strings, ensuring that new or modified strings have corresponding entries in translation files.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR `#371` (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
celdrake
force-pushed
the
EDM-2925-helm-application-type
branch
2 times, most recently
from
5151f6b to
95640a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx`:
- Around line 29-31: The FormGroup for the application name is marked isRequired
in EditDeviceWizard (FormGroup with label t('Application name') containing
TextField name `${appFieldName}.name`) but the Helm validation schema
(validApplicationAndVolumeName(t) used in validations.ts around the Helm app
schema) does not call .required(), causing a mismatch; fix by either removing
the isRequired prop from that FormGroup/TextField if the name is optional, or
update the Helm validation to require the field by chaining .required() onto
validApplicationAndVolumeName(t) in the Helm schema so the UI and validation
agree.
♻️ Duplicate comments (2)
libs/ui-components/src/components/form/UploadField.tsx (1)
92-128: Move the accessible label to the control, not the FormGroup.
aria-labelonFormGroupdoesn’t label the actual input;FileUploadshould receive it directly, andfieldIdshould match the controlid. This matches prior guidance.🔧 Suggested fix
- <FormGroup fieldId={fieldId} aria-label={ariaLabel} isRequired={isRequired}> + <FormGroup fieldId={fieldId} isRequired={isRequired}> <FileUpload type="text" - id={field.name} + id={fieldId} + aria-label={ariaLabel} isRequired={isRequired}libs/i18n/locales/en/translation.json (1)
391-391: Fix typo in user-facing string."ordered listed" should be "listed order".
✏️ Proposed fix
- "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", + "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.",
🧹 Nitpick comments (4)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx (1)
36-43: Avoid nestedFormGroupwrappers.
UploadFieldalready renders aFormGroup, so wrapping it creates nested groups and can disconnect the visible label from the control. Consider adding a prop to letUploadFieldrender without its internalFormGroup, or move the visible label intoUploadFielditself.libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (2)
229-237: Consider graceful degradation instead of throwing.The YAML validation in
validations.tsalready ensures valid YAML before submission. However, iftoAPIApplicationis called with invalid YAML (e.g., during testing or from external code), throwing an error here could be unexpected. Consider whether logging a warning and returning an emptyvaluesobject might be more resilient, or document that this function expects pre-validated input.
485-488: Object comparison is order-sensitive and whitespace fallback is unusual.Two concerns:
yaml.load(updatedApp.valuesYaml || ' ')- the fallback to' '(two spaces) is unconventional. Both empty string and whitespace-only YAML parse toundefined. Consider using|| ''for clarity.
JSON.stringifycomparison is property-order sensitive. If the same values are serialized in different orders, this will incorrectly detect a change.♻️ Suggested improvement
- const updatedValues = yaml.load(updatedApp.valuesYaml || ' ') as Record<string, unknown>; - if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) { + const updatedValues = yaml.load(updatedApp.valuesYaml || '') as Record<string, unknown> | undefined; + const currentValues = imageApp.values || {}; + const newValues = updatedValues || {}; + // Note: JSON.stringify comparison is order-sensitive; consider deep-equal for robustness + if (JSON.stringify(currentValues) !== JSON.stringify(newValues)) {libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx (1)
108-128: Tooltip shows misleading message when button is enabled.The tooltip with "Fill in the existing values files before you can add more values files" displays even when the button is enabled and clickable. This could confuse users.
Consider conditionally showing the tooltip only when the button is disabled, or changing the tooltip content to something always-relevant like guidance on values file usage.
♻️ Suggested approach
- <Tooltip - className="fctl-application-helm-form__tooltip" - content={ - <Content>{t('Fill in the existing values files before you can add more values files.')}</Content> - } - > + <Tooltip + className="fctl-application-helm-form__tooltip" + content={ + <Content>{t('Fill in the existing values files before you can add more values files.')}</Content> + } + trigger={canAddValuesFile ? 'manual' : 'mouseenter focus'} + >Alternatively, wrap the Tooltip conditionally:
{!canAddValuesFile ? ( <Tooltip content={...}> <Button .../> </Tooltip> ) : ( <Button .../> )}
celdrake
force-pushed
the
EDM-2925-helm-application-type
branch
from
95640a2 to
21820ba
Compare
|
@coderabitai could you do the review? |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/components/form/UploadField.tsx (1)
58-131: Move the accessible label to the FileUpload control and align the id with FormGroup's fieldId.The FileUpload control lacks its own accessible name and has a mismatched id. FormGroup's
fieldId={fieldId}expects the child control to haveid={fieldId}, but FileUpload currently usesid={field.name}(the unqualified Formik field name). The aria-label should also be on the FileUpload itself rather than FormGroup for proper screen reader association.Proposed fix
- return ( - <FormGroup fieldId={fieldId} aria-label={ariaLabel} isRequired={isRequired}> - <FileUpload - type="text" - id={field.name} + return ( + <FormGroup fieldId={fieldId} isRequired={isRequired}> + <FileUpload + type="text" + id={fieldId} + aria-label={ariaLabel} isRequired={isRequired}
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts`:
- Around line 469-494: The Helm-change detection incorrectly treats empty
valuesYaml as a change and inconsistently handles whitespace in valuesFiles; in
isHelmImageAppForm branch, ensure valuesFiles are trimmed before filtering by
replacing (imageApp.valuesFiles || []) and updatedApp.valuesFiles with trimmed
versions (e.g., map(file => file.trim())), then compare lengths and entries; for
valuesYaml use yaml.load(updatedApp.valuesYaml || '{}') (or coerce undefined to
an empty object) and guard the parsed result so that
JSON.stringify(imageApp.values || {}) is compared against
JSON.stringify(parsedValues || {}); keep the rest of the early returns, and
leave areEnvVariablesEqual usage unchanged for non-Helm apps.
- Around line 220-244: The YAML parsing for Helm app values in the
isHelmImageAppForm branch currently accepts scalars/arrays because it only
checks Object.keys(values). Ensure that after calling yaml.load(app.valuesYaml)
you validate the result is a plain mapping/object (e.g., typeof values ===
'object' && values !== null && !Array.isArray(values')) before assigning to
data.values; if the parsed value is not an object, throw the existing YAML error
(or a new Error('Values content must be a YAML mapping/object.')) so only
object/mapping types are sent to the API and non-mapping YAML is rejected.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
87-101: Avoid persisting emptyvaluesFilesentries by default.Initializing
valuesFilesas['']can leak an empty path into the payload if the user leaves it untouched. Consider keeping the array empty and letting the UI render a blank row without persisting it, or filter empty strings on submit.♻️ Suggested tweak
- valuesFiles: [''], // We want to show a "values files" field by default + valuesFiles: [],
| if (isHelmImageAppForm(app)) { | ||
| const data: ImageApplicationProviderSpec & ApplicationProviderSpec = { | ||
| name: app.name, | ||
| image: app.image, | ||
| appType: app.appType, | ||
| }; | ||
| if (app.namespace) { | ||
| data.namespace = app.namespace; | ||
| } | ||
| if (app.valuesYaml) { | ||
| try { | ||
| const values = yaml.load(app.valuesYaml) as Record<string, unknown>; | ||
| if (values && Object.keys(values).length > 0) { | ||
| data.values = values; | ||
| } | ||
| } catch (error) { | ||
| throw new Error('Values content is not valid YAML.'); | ||
| } | ||
| } | ||
| const fileNames = app.valuesFiles.filter((file) => file && file.trim() !== ''); | ||
| if (fileNames.length > 0) { | ||
| data.valuesFiles = fileNames; | ||
| } | ||
| return data; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "deviceSpecUtils.ts" -type f | head -20Repository: flightctl/flightctl-ui
Length of output: 144
🏁 Script executed:
find . -name "EditDeviceWizard*" -type f | head -20Repository: flightctl/flightctl-ui
Length of output: 383
🏁 Script executed:
rg -l "valuesYaml" --type ts --type tsx | head -20Repository: flightctl/flightctl-ui
Length of output: 93
🏁 Script executed:
head -n 250 ./libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | tail -n 40Repository: flightctl/flightctl-ui
Length of output: 1094
🏁 Script executed:
rg "app\.valuesYaml" --type ts | head -20Repository: flightctl/flightctl-ui
Length of output: 322
🏁 Script executed:
rg "valuesYaml" -A 3 -B 3 ./libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx | head -60Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
rg "type AppForm\|interface AppForm" --type ts -A 10 | head -50Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
rg "YamlEditor" --type ts -B 3 -A 3 | head -80Repository: flightctl/flightctl-ui
Length of output: 8291
🏁 Script executed:
grep -r "valuesYaml" ./libs/ui-components/src/components/Device/EditDeviceWizard/ | head -20Repository: flightctl/flightctl-ui
Length of output: 1007
🏁 Script executed:
cat ./libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsxRepository: flightctl/flightctl-ui
Length of output: 6260
🏁 Script executed:
rg "UploadField" -A 5 -B 5 ./libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsxRepository: flightctl/flightctl-ui
Length of output: 1025
🏁 Script executed:
rg "type AppForm|interface AppForm" -r . --type ts --type tsx | head -20Repository: flightctl/flightctl-ui
Length of output: 93
🏁 Script executed:
find . -name "deviceSpec.ts" -o -name "deviceSpec.tsx" | head -10Repository: flightctl/flightctl-ui
Length of output: 110
🏁 Script executed:
rg "export.*HelmImageAppForm|interface HelmImageAppForm|type HelmImageAppForm" --type ts -A 8Repository: flightctl/flightctl-ui
Length of output: 1375
🏁 Script executed:
find . -name "UploadField.tsx" -o -name "UploadField.ts" | head -5Repository: flightctl/flightctl-ui
Length of output: 122
🏁 Script executed:
cat ./libs/ui-components/src/components/form/UploadField.tsxRepository: flightctl/flightctl-ui
Length of output: 4511
🏁 Script executed:
rg "toAPIApplication" -B 5 -A 2 ./libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsxRepository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
rg "toAPIApplication" --type ts | head -20Repository: flightctl/flightctl-ui
Length of output: 802
🏁 Script executed:
rg "toAPIApplication\(" -B 10 -A 2 | head -60Repository: flightctl/flightctl-ui
Length of output: 1548
🏁 Script executed:
cat -n ./libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | sed -n '210,250p'Repository: flightctl/flightctl-ui
Length of output: 1404
🏁 Script executed:
rg "yaml\.load" --type ts -B 2 -A 2 | head -40Repository: flightctl/flightctl-ui
Length of output: 1668
🏁 Script executed:
rg "valuesYaml" ./libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts -B 2 -A 10 | head -80Repository: flightctl/flightctl-ui
Length of output: 1349
🏁 Script executed:
rg "valuesYaml" ./libs/ui-components/src/components/form/validations.ts -B 10 -A 10Repository: flightctl/flightctl-ui
Length of output: 1047
🏁 Script executed:
rg "yaml.load.*Record" --type ts -B 3 -A 3Repository: flightctl/flightctl-ui
Length of output: 1683
🏁 Script executed:
cat ./libs/ui-components/src/components/form/validations.ts | head -100Repository: flightctl/flightctl-ui
Length of output: 3770
🏁 Script executed:
rg "yaml\.load.*as Record" --type ts -B 5 -A 5Repository: flightctl/flightctl-ui
Length of output: 2495
🏁 Script executed:
cat ./libs/ui-components/src/components/form/validations.ts | sed -n '60,85p'Repository: flightctl/flightctl-ui
Length of output: 1201
🏁 Script executed:
cat ./libs/ui-components/src/components/form/validations.ts | sed -n '160,190p'Repository: flightctl/flightctl-ui
Length of output: 1050
🏁 Script executed:
rg "valuesYaml.*Yup" --type ts -A 12Repository: flightctl/flightctl-ui
Length of output: 1306
🏁 Script executed:
cat ./libs/ui-components/src/components/form/validations.ts | sed -n '370,405p'Repository: flightctl/flightctl-ui
Length of output: 1100
🏁 Script executed:
cat -n ./libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | sed -n '250,270p'Repository: flightctl/flightctl-ui
Length of output: 711
🏁 Script executed:
python3 << 'EOF'
import json
# Simulate yaml.load behavior with different inputs
test_cases = [
("hello", "scalar string"),
("123", "scalar number"),
("[1, 2, 3]", "array"),
("key: value", "object/mapping"),
("null", "null value"),
]
for yaml_str, description in test_cases:
print(f"\n{description}: {yaml_str}")
print(f" typeof check: typeof would be 'object' for arrays/objects/null, 'string'/'number' for scalars")
if yaml_str in ["123", "hello"]:
print(f" Object.keys() would fail or return empty")
elif yaml_str == "null":
print(f" Object.keys(null) would throw TypeError")
elif yaml_str.startswith("["):
print(f" Object.keys() would return numeric indices: ['0', '1', '2']")
else:
print(f" Object.keys() would work correctly for object")
EOFRepository: flightctl/flightctl-ui
Length of output: 152
🏁 Script executed:
cat ./libs/ui-components/src/components/form/validations.ts | sed -n '395,425p'Repository: flightctl/flightctl-ui
Length of output: 1037
🏁 Script executed:
cat ./libs/ui-components/src/components/form/validations.ts | sed -n '327,345p'Repository: flightctl/flightctl-ui
Length of output: 874
🏁 Script executed:
cat -n ./libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | sed -n '255,275p'Repository: flightctl/flightctl-ui
Length of output: 710
Validate that valuesYaml parses to a mapping (not scalar/array).
yaml.load() can return scalars or arrays; form validation only checks YAML syntax, not structure. Strings and arrays would pass the Object.keys(values).length > 0 check and be sent to the API as values. Add explicit type validation to ensure only object/mapping types are accepted.
🐛 Proposed fix
if (app.valuesYaml) {
try {
const parsed = yaml.load(app.valuesYaml);
const isObject = !!parsed && typeof parsed === 'object' && !Array.isArray(parsed);
if (!isObject) {
throw new Error('Values content must be a YAML mapping.');
}
if (Object.keys(parsed as Record<string, unknown>).length > 0) {
data.values = parsed as Record<string, unknown>;
}
} catch (error) {
throw new Error('Values content is not valid YAML.');
}
}📝 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.
| if (isHelmImageAppForm(app)) { | |
| const data: ImageApplicationProviderSpec & ApplicationProviderSpec = { | |
| name: app.name, | |
| image: app.image, | |
| appType: app.appType, | |
| }; | |
| if (app.namespace) { | |
| data.namespace = app.namespace; | |
| } | |
| if (app.valuesYaml) { | |
| try { | |
| const values = yaml.load(app.valuesYaml) as Record<string, unknown>; | |
| if (values && Object.keys(values).length > 0) { | |
| data.values = values; | |
| } | |
| } catch (error) { | |
| throw new Error('Values content is not valid YAML.'); | |
| } | |
| } | |
| const fileNames = app.valuesFiles.filter((file) => file && file.trim() !== ''); | |
| if (fileNames.length > 0) { | |
| data.valuesFiles = fileNames; | |
| } | |
| return data; | |
| } | |
| if (isHelmImageAppForm(app)) { | |
| const data: ImageApplicationProviderSpec & ApplicationProviderSpec = { | |
| name: app.name, | |
| image: app.image, | |
| appType: app.appType, | |
| }; | |
| if (app.namespace) { | |
| data.namespace = app.namespace; | |
| } | |
| if (app.valuesYaml) { | |
| try { | |
| const parsed = yaml.load(app.valuesYaml); | |
| const isObject = !!parsed && typeof parsed === 'object' && !Array.isArray(parsed); | |
| if (!isObject) { | |
| throw new Error('Values content must be a YAML mapping.'); | |
| } | |
| if (Object.keys(parsed as Record<string, unknown>).length > 0) { | |
| data.values = parsed as Record<string, unknown>; | |
| } | |
| } catch (error) { | |
| throw new Error('Values content is not valid YAML.'); | |
| } | |
| } | |
| const fileNames = app.valuesFiles.filter((file) => file && file.trim() !== ''); | |
| if (fileNames.length > 0) { | |
| data.valuesFiles = fileNames; | |
| } | |
| return data; | |
| } |
🤖 Prompt for AI Agents
In `@libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts`
around lines 220 - 244, The YAML parsing for Helm app values in the
isHelmImageAppForm branch currently accepts scalars/arrays because it only
checks Object.keys(values). Ensure that after calling yaml.load(app.valuesYaml)
you validate the result is a plain mapping/object (e.g., typeof values ===
'object' && values !== null && !Array.isArray(values')) before assigning to
data.values; if the parsed value is not an object, throw the existing YAML error
(or a new Error('Values content must be a YAML mapping/object.')) so only
object/mapping types are sent to the API and non-mapping YAML is rejected.
| // The app is a Helm application | ||
| if (isHelmImageAppForm(updatedApp)) { | ||
| const imageApp = currentApp as ImageApplicationProviderSpec; | ||
| if (imageApp.image !== updatedApp.image || imageApp.namespace !== updatedApp.namespace) { | ||
| return true; | ||
| } | ||
|
|
||
| // Compare valuesFiles arrays | ||
| const currentValuesFiles = (imageApp.valuesFiles || []).filter((file) => file !== ''); | ||
| const updatedValuesFiles = updatedApp.valuesFiles.filter((file) => file !== ''); | ||
| if (currentValuesFiles.length !== updatedValuesFiles.length) { | ||
| return true; | ||
| } | ||
| if (!currentValuesFiles.every((file, index) => file === updatedValuesFiles[index])) { | ||
| return true; | ||
| } | ||
| const updatedValues = yaml.load(updatedApp.valuesYaml || ' ') as Record<string, unknown>; | ||
| if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| if (!areEnvVariablesEqual(currentApp.envVars, updatedApp.variables)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the specific file and check the code at lines 469-494
find . -type f -name "deviceSpecUtils.ts" | head -5Repository: flightctl/flightctl-ui
Length of output: 144
🏁 Script executed:
# Once we locate the file, examine the specific lines
if [ -f "libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts" ]; then
wc -l "libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts"
fiRepository: flightctl/flightctl-ui
Length of output: 146
🏁 Script executed:
# Check the yaml library being used and its behavior
grep -r "import.*yaml" libs/ui-components/src/components/Device/EditDeviceWizard/ | head -5Repository: flightctl/flightctl-ui
Length of output: 700
🏁 Script executed:
# Test yaml.load behavior with empty strings
node <<'EOF'
const yaml = require('js-yaml');
console.log("Testing yaml.load behavior:");
console.log("yaml.load(' '):", yaml.load(' '));
console.log("yaml.load(''):", yaml.load(''));
console.log("yaml.load(null):", yaml.load(null));
console.log("yaml.load(undefined):", yaml.load(undefined));
console.log("typeof yaml.load(' '):", typeof yaml.load(' '));
console.log("yaml.load(' ') === undefined:", yaml.load(' ') === undefined);
console.log("yaml.load('') === undefined:", yaml.load('') === undefined);
EOFRepository: flightctl/flightctl-ui
Length of output: 903
🏁 Script executed:
sed -n '469,494p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 1062
🏁 Script executed:
# Let's also see context around these lines to understand the function
sed -n '450,510p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 2365
🏁 Script executed:
# Check js-yaml documentation and behavior - search for how it's used elsewhere
grep -A 5 "yaml.load" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -20Repository: flightctl/flightctl-ui
Length of output: 554
🏁 Script executed:
# Let's check the package.json to confirm js-yaml version
grep -A 2 "js-yaml" package.jsonRepository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Check how imageApp.values is typically set/populated
grep -B 5 -A 5 "imageApp.values" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -30Repository: flightctl/flightctl-ui
Length of output: 828
🏁 Script executed:
# Look for where updatedApp.valuesYaml comes from and whether it can be empty
grep -B 3 -A 3 "valuesYaml" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -40Repository: flightctl/flightctl-ui
Length of output: 928
🏁 Script executed:
# Check the structure of ImageApplicationProviderSpec and AppForm to understand values field
rg "interface.*ImageApplicationProviderSpec|type.*ImageApplicationProviderSpec" -A 10Repository: flightctl/flightctl-ui
Length of output: 3106
🏁 Script executed:
# Look for where hasApplicationChanged is called to understand the data flow
rg "hasApplicationChanged" -B 2 -A 2 libs/ui-components/src/components/Device/EditDeviceWizard/Repository: flightctl/flightctl-ui
Length of output: 1327
🏁 Script executed:
# Check AppForm interface to see valuesYaml type
rg "interface.*AppForm|type.*AppForm" -A 15 | head -50Repository: flightctl/flightctl-ui
Length of output: 3593
🏁 Script executed:
# See how js-yaml is referenced in package.json
cat package.json | grep -A 5 -B 5 "js-yaml"Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Find package.json and check js-yaml version
find . -name "package.json" -type f | head -3Repository: flightctl/flightctl-ui
Length of output: 155
🏁 Script executed:
# Look at the root package.json
head -50 package.json | grep -A 5 -B 5 "js-yaml" || echo "Not in first 50 lines, checking further"Repository: flightctl/flightctl-ui
Length of output: 105
🏁 Script executed:
# Search more broadly in package.json
cat package.json | grep "js-yaml"Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's understand the logic better - look at what happens with imageApp.values
sed -n '1,100p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -50Repository: flightctl/flightctl-ui
Length of output: 1200
🏁 Script executed:
# Check root directory package.json more carefully
cat package.json | grep -C 10 "dependencies\|devDependencies" | grep -A 50 "dependencies" | grep "js-yaml"Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's look at the actual storage/conversion of valuesYaml to understand the flow better
rg "valuesYaml" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts -B 5 -A 5Repository: flightctl/flightctl-ui
Length of output: 1220
🏁 Script executed:
# Check the entire function to see how values comparison works
sed -n '440,520p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 2820
🏁 Script executed:
# Let's search for other uses of yaml.load in the codebase to understand behavior patterns
rg "yaml\.load" -B 2 -A 2Repository: flightctl/flightctl-ui
Length of output: 1668
🏁 Script executed:
# Check how valuesFiles are initialized and used in the YAML editor
rg "valuesFiles" libs/ui-components/src/components/Device/EditDeviceWizard/ -B 3 -A 3Repository: flightctl/flightctl-ui
Length of output: 7933
🏁 Script executed:
# Let's look at how the form initializes and what happens with empty valuesYaml
sed -n '1,50p' libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsxRepository: flightctl/flightctl-ui
Length of output: 2040
🏁 Script executed:
# Check the YamlEditorBase mentioned in the learning to understand empty YAML handling
find . -name "YamlEditorBase.tsx" -type fRepository: flightctl/flightctl-ui
Length of output: 138
🏁 Script executed:
# Let's trace through - when convertDeviceSpecToForm is called, what's the initial state?
rg "convertDeviceSpecToForm" -A 20 libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -40Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's check the YamlEditorBase to understand the context from the learning
sed -n '59,64p' libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsxRepository: flightctl/flightctl-ui
Length of output: 215
🏁 Script executed:
# Look at what yaml.load actually returns - let's check if there are any tests or documentation
rg "yaml\.load.*undefined\|undefined.*yaml\.load" -iRepository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's verify the issue by looking at a broader context - how values are actually initialized
sed -n '360,395p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 1259
🏁 Script executed:
# Let's look at the convertDeviceSpecToForm function more carefully
sed -n '250,320p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 1897
🏁 Script executed:
# Let's look at how imageApp.values is initialized/handled in the incoming data
rg "\.values\s*=" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -15Repository: flightctl/flightctl-ui
Length of output: 97
🏁 Script executed:
# Check what happens when an app is serialized - does it ever have undefined values?
sed -n '200,250p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 1274
🏁 Script executed:
# Let's trace convertDeviceSpecToForm to see how valuesYaml is converted back
rg "convertDeviceSpecToForm" -B 5 -A 30 | grep -A 30 "HelmImageAppForm\|isHelmImageAppForm" | head -50Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's find where convertDeviceSpecToForm is defined
grep -n "const convertDeviceSpecToForm" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Look at that function
sed -n '120,180p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 2072
🏁 Script executed:
# Search for the function that converts DeviceSpec to Form
rg "convertDeviceSpecToForm|convertSpecToForm" -B 2 -A 30 libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts | head -80Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's search for where the form is initialized from the device spec
rg "HelmImageAppForm|valuesYaml.*=" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts -B 3 -A 3 | head -80Repository: flightctl/flightctl-ui
Length of output: 723
🏁 Script executed:
# Let's look for where the form is converted back from the device spec
rg "fromAppProvider|toAppForm" -B 2 -A 30 libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's search more broadly for form initialization from app spec
rg "imageApp\.values|app\.values" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts -B 5 -A 5Repository: flightctl/flightctl-ui
Length of output: 1846
🏁 Script executed:
# Let's understand the full flow - let's see where the ApplicationProviderSpec comes from
rg "imageApp\.values" libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts -B 10 -A 5Repository: flightctl/flightctl-ui
Length of output: 973
🏁 Script executed:
# Let's verify yaml.load behavior by checking js-yaml documentation or similar code patterns
rg "yaml\." libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 343
🏁 Script executed:
# Let's check if there's a test file for deviceSpecUtils
find . -name "*deviceSpecUtils*.test.ts" -o -name "*deviceSpecUtils*.spec.ts"Repository: flightctl/flightctl-ui
Length of output: 48
🏁 Script executed:
# Let's check what the actual ImageApplicationProviderSpec structure looks like and how values field is defined
cat libs/types/models/ImageApplicationProviderSpec.tsRepository: flightctl/flightctl-ui
Length of output: 1258
🏁 Script executed:
# Let's verify the exact logic by reviewing how the comparison works step by step
sed -n '480,490p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 430
🏁 Script executed:
# Let's verify the exact type signature for imageApp to understand what imageApp.values could be
rg "type ImageApplicationProviderSpec|interface ImageApplicationProviderSpec" -A 20 libs/types/models/ImageApplicationProviderSpec.tsRepository: flightctl/flightctl-ui
Length of output: 873
🏁 Script executed:
# Let's check how valuesFiles is handled in the conversion - specifically in both directions
sed -n '115,155p' libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 1588
🏁 Script executed:
# Most importantly, let's see the complete flow - where does the form come from initially?
rg "convertAppProviderSpecToForm|appProviderSpec.*HelmImageAppForm" -B 5 -A 20 libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tsRepository: flightctl/flightctl-ui
Length of output: 48
Fix Helm change detection for empty values and trim valuesFiles consistently.
When updatedApp.valuesYaml is empty or undefined, yaml.load(' ') may return undefined, causing the JSON comparison to incorrectly report changes. Additionally, valuesFiles serialization trims whitespace before filtering, but the comparison does not, creating an inconsistency.
🐛 Proposed fix
- const currentValuesFiles = (imageApp.valuesFiles || []).filter((file) => file !== '');
- const updatedValuesFiles = updatedApp.valuesFiles.filter((file) => file !== '');
+ const normalizeFiles = (files: string[]) => files.map((f) => f.trim()).filter(Boolean);
+ const currentValuesFiles = normalizeFiles(imageApp.valuesFiles || []);
+ const updatedValuesFiles = normalizeFiles(updatedApp.valuesFiles);
@@
- const updatedValues = yaml.load(updatedApp.valuesYaml || ' ') as Record<string, unknown>;
- if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) {
+ const updatedValuesRaw = yaml.load(updatedApp.valuesYaml || '') as unknown;
+ const updatedValues =
+ updatedValuesRaw && typeof updatedValuesRaw === 'object' && !Array.isArray(updatedValuesRaw)
+ ? (updatedValuesRaw as Record<string, unknown>)
+ : {};
+ if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) {
return true;
}🤖 Prompt for AI Agents
In `@libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts`
around lines 469 - 494, The Helm-change detection incorrectly treats empty
valuesYaml as a change and inconsistently handles whitespace in valuesFiles; in
isHelmImageAppForm branch, ensure valuesFiles are trimmed before filtering by
replacing (imageApp.valuesFiles || []) and updatedApp.valuesFiles with trimmed
versions (e.g., map(file => file.trim())), then compare lengths and entries; for
valuesYaml use yaml.load(updatedApp.valuesYaml || '{}') (or coerce undefined to
an empty object) and guard the parsed result so that
JSON.stringify(imageApp.values || {}) is compared against
JSON.stringify(parsedValues || {}); keep the rest of the early returns, and
leave areEnvVariablesEqual usage unchanged for non-Helm apps.
2 participants
New application type: Helm charts.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.