Fix: add null safety for optional app catalog fields#518
Fix: add null safety for optional app catalog fields#518Darshika11b wants to merge 2 commits intoheadlamp-k8s:mainfrom
Conversation
There was a problem hiding this comment.
Thanks for those changes.
Can you please check the git commit messages match the git commit guidelines in the contributing guide?
Please see open review comments?
There's an issue with the DCO GitHub check. Please have a look. git commit --amend -s
See the GitHub check failure with lint.
npm run lint
There was a problem hiding this comment.
Pull request overview
This PR updates the App Catalog release editor dialog to be more defensive when optional Helm release/chart fields are missing, aiming to prevent runtime crashes when clusters return partial data.
Changes:
- Adds null-safe fallbacks for optional
release.chart.values,release.config, andrelease.chart.metadata.name. - Refactors chart-version fetching to handle missing metadata and missing
response.charts. - Introduces YAML parse error tracking (
yamlError) and disables upgrade when YAML is invalid.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| checkUpgradeStatus(); | ||
| }) | ||
| .catch(() => { | ||
| enqueueSnackbar('Upgrade failed', { variant: 'error' }); |
There was a problem hiding this comment.
upgradeLoading is set to true when submitting, but it’s never reset on success (the promise then closes the dialog without clearing it). If the component stays mounted and the dialog is reopened, the Upgrade button can remain disabled. Ensure setUpgradeLoading(false) runs on both success and failure (or when closing).
| enqueueSnackbar('Upgrade failed', { variant: 'error' }); | |
| enqueueSnackbar('Upgrade failed', { variant: 'error' }); | |
| }) | |
| .finally(() => { |
| const { | ||
| openEditor, | ||
| handleEditor, | ||
| releaseName, | ||
| releaseNamespace, | ||
| release, | ||
| isUpdateRelease, | ||
| handleUpdate, | ||
| } = props; |
There was a problem hiding this comment.
handleUpdate is destructured from props but is no longer used after removing the previous status polling/callback. This changes behavior (the parent won’t refresh after an upgrade) and will likely trigger unused-variable lint/TS warnings. Either wire handleUpdate() back into the successful upgrade path (or restore the previous status-check logic) or remove the prop/state that’s no longer needed.
| @@ -125,230 +112,112 @@ | |||
| return () => { | |||
| isMounted = false; | |||
| }; | |||
| }, [isUpdateRelease]); | |||
| }, [isUpdateRelease, release, enqueueSnackbar]); | |||
|
|
|||
| function handleValueChange(event: any) { | |||
| if (event.target.checked) { | |||
| setValuesToShow(userValues); | |||
| } else { | |||
| setValuesToShow(values); | |||
| } | |||
| setIsUserValues(event.target.checked); | |||
| const checked = event.target.checked; | |||
| setIsUserValues(checked); | |||
| setValuesToShow(checked ? userValues : values); | |||
| } | |||
|
|
|||
| function checkUpgradeStatus() { | |||
| setTimeout(() => { | |||
| getActionStatus(releaseName, 'upgrade') | |||
| .then((response: any) => { | |||
| if (response.status === 'processing') { | |||
| checkUpgradeStatus(); | |||
| } else if (response.status && response.status === 'failed') { | |||
| enqueueSnackbar(`Error upgrading release ${releaseName} ${response.message}`, { | |||
| variant: 'error', | |||
| autoHideDuration: 5000, | |||
| }); | |||
| handleEditor(false); | |||
| setUpgradeLoading(false); | |||
| } else if (!response.status || response.status !== 'success') { | |||
| enqueueSnackbar(`Error upgrading release ${releaseName}`, { | |||
| variant: 'error', | |||
| autoHideDuration: 5000, | |||
| }); | |||
| handleEditor(false); | |||
| } else { | |||
| enqueueSnackbar(`Release ${releaseName} upgraded successfully`, { | |||
| variant: 'success', | |||
| autoHideDuration: 5000, | |||
| }); | |||
| handleEditor(false); | |||
| setUpgradeLoading(false); | |||
| handleUpdate(); | |||
| } | |||
| }) | |||
| .catch(() => { | |||
| setUpgradeLoading(false); | |||
| handleEditor(false); | |||
| }); | |||
| }, 1000); | |||
|
|
|||
| function handleEditorChange(value?: string) { | |||
| if (!value) return; | |||
|
|
|||
| try { | |||
| const parsed = yamlToJSON(value); | |||
| setYamlError(null); | |||
|
|
|||
| if (checkBoxRef.current?.checked) { | |||
| setUserValues(parsed); | |||
| } else { | |||
| setValues(parsed); | |||
| } | |||
| } catch { | |||
| setYamlError('Invalid YAML format'); | |||
| } | |||
There was a problem hiding this comment.
The checkbox toggle UI was removed, but the code still branches on checkBoxRef.current?.checked in handleEditorChange. Since no checkbox is rendered, this will always be false and userValues will never be updated, making the “user values” path dead code. Either restore the checkbox/FormControlLabel UI (and use handleValueChange) or remove the unused state/ref and simplify the editor change handling.
| }); | ||
| return; | ||
| } finally { | ||
| setIsLoading(false); |
There was a problem hiding this comment.
setIsLoading(false) is called in the finally block even after unmount, because only setVersions is guarded by isMounted. This can cause React warnings about setting state on an unmounted component. Guard setIsLoading(false) with if (isMounted) (or cancel the request via AbortController) before setting state.
| setIsLoading(false); | |
| if (isMounted) { | |
| setIsLoading(false); | |
| } |
| const [versions, setVersions] = useState<any[]>([]); | ||
| const [selectedVersion, setSelectedVersion] = useState<any>(); | ||
| const [isLoading, setIsLoading] = useState(false); |
There was a problem hiding this comment.
This component now uses any for versions and selectedVersion, whereas other dialogs in this codebase model Autocomplete options with a dedicated type (e.g. a FieldType containing value/title/...). Keeping strong types here will prevent runtime mistakes when accessing selectedVersion.value/.version and avoids losing compiler help.
| import { | ||
| Box, | ||
| Button, | ||
| Checkbox, | ||
| DialogActions, | ||
| DialogContent, | ||
| FormControlLabel, | ||
| TextField, | ||
| } from '@mui/material'; | ||
| import { Autocomplete } from '@mui/material'; | ||
| import _ from 'lodash'; | ||
| import { useSnackbar } from 'notistack'; | ||
| import { useEffect, useRef, useState } from 'react'; | ||
| import semver from 'semver'; | ||
| import { fetchChart, getActionStatus, upgradeRelease } from '../../api/releases'; | ||
| import { APP_CATALOG_HELM_REPOSITORY } from '../../constants/catalog'; | ||
| import { jsonToYAML, yamlToJSON } from '../../helpers'; |
There was a problem hiding this comment.
After removing the upgrade/status UI, several imports are now unused in this file (Checkbox, FormControlLabel, TextField, Autocomplete, and getActionStatus). This will typically fail lint/TS checks and adds noise. Remove the unused imports (or restore the UI that uses them).
| {isLoading ? ( | ||
| <Loader title="Loading Chart Versions" /> | ||
| ) : ( | ||
| <> | ||
| <Box display="flex" p={2} pt={0}> | ||
| <Box ml={2}> | ||
| {isUpdateRelease && ( | ||
| <TextField | ||
| id="release-description" | ||
| style={{ | ||
| width: '20vw', | ||
| }} | ||
| error={isFormSubmitting && !releaseUpdateDescription} | ||
| label="Release Description" | ||
| value={releaseUpdateDescription} | ||
| onChange={event => setReleaseUpdateDescription(event.target.value)} | ||
| /> | ||
| )} | ||
| </Box> | ||
| {isUpdateRelease && ( | ||
| <Box ml={2}> | ||
| <Autocomplete | ||
| style={{ | ||
| width: '20vw', | ||
| }} | ||
| options={versions} | ||
| getOptionLabel={option => option.version} | ||
| value={selectedVersion} | ||
| onChange={( | ||
| event, | ||
| newValue: { value: string; title: string; version: string } | ||
| ) => { | ||
| setSelectedVersion(newValue); | ||
| }} | ||
| renderInput={params => ( | ||
| <TextField | ||
| {...params} | ||
| label="Versions" | ||
| placeholder="Select Version" | ||
| error={isFormSubmitting && !selectedVersion} | ||
| /> | ||
| )} | ||
| /> | ||
| <DialogContent> | ||
| <Editor | ||
| value={jsonToYAML(valuesToShow)} | ||
| language="yaml" | ||
| height="400px" | ||
| theme={themeName === 'dark' ? 'vs-dark' : 'light'} | ||
| onChange={handleEditorChange} | ||
| /> | ||
| {yamlError && ( | ||
| <Box color="error.main" mt={1}> | ||
| {yamlError} | ||
| </Box> | ||
| )} | ||
| </Box> | ||
| <Box ml={2}> | ||
| <FormControlLabel | ||
| control={ | ||
| <Checkbox | ||
| checked={isUserValues} | ||
| onChange={handleValueChange} | ||
| inputProps={{ 'aria-label': 'Switch between default and user defined values' }} | ||
| inputRef={checkBoxRef} | ||
| /> | ||
| } | ||
| label="user defined values only" | ||
| /> | ||
| </Box> | ||
| <DialogContent> | ||
| <Box pt={2} height="100%" my={1} p={1}> | ||
| {openEditor && ( | ||
| <Editor | ||
| value={jsonToYAML(valuesToShow)} | ||
| language="yaml" | ||
| height="400px" | ||
| options={{ | ||
| selectOnLineNumbers: true, | ||
| }} | ||
| onChange={value => { | ||
| handleEditorChange(value); | ||
| }} | ||
| theme={themeName === 'dark' ? 'vs-dark' : 'light'} | ||
| onMount={editor => { | ||
| setIsUserValues(false); | ||
| setValuesToShow(Object.assign({}, release.chart.values, release.config)); | ||
| if (!isUpdateRelease) { | ||
| editor.updateOptions({ readOnly: true }); | ||
| } | ||
| }} | ||
| /> | ||
| )} | ||
| </Box> | ||
| </DialogContent> | ||
| </> | ||
| )} | ||
| <DialogActions | ||
| style={{ | ||
| padding: 0, | ||
| margin: '1rem 0.5rem', | ||
| }} | ||
| > | ||
| <Button onClick={() => handleEditor(false)}>Close</Button> | ||
| {isUpdateRelease && | ||
| (upgradeLoading ? ( | ||
| <Button disabled={upgradeLoading}>{upgradeLoading ? 'Upgrading' : 'Upgrade'}</Button> | ||
| ) : ( | ||
| <Button onClick={() => upgradeReleaseHandler()} disabled={upgradeLoading || isLoading}> | ||
|
|
||
| <DialogActions> | ||
| <Button onClick={() => handleEditor(false)}>Close</Button> | ||
| <Button | ||
| onClick={upgradeReleaseHandler} | ||
| disabled={upgradeLoading || !!yamlError} | ||
| > | ||
| Upgrade | ||
| </Button> | ||
| ))} | ||
| </DialogActions> | ||
| </DialogActions> |
There was a problem hiding this comment.
The upgrade flow now requires releaseUpdateDescription and selectedVersion, but the dialog no longer renders any inputs to set them (TextField/Autocomplete were removed). As a result, clicking “Upgrade” will always fail validation and makes upgrading impossible. Re-introduce the update-only form controls (description + version selector) or relax the validation when those controls are not present, and only render the Upgrade action when isUpdateRelease is true.
| title: string; | ||
| version: string; | ||
| }>(); | ||
| const [versions, setVersions] = useState<any[]>([]); |
There was a problem hiding this comment.
Unused variable versions.
| version: string; | ||
| }>(); | ||
| const [versions, setVersions] = useState<any[]>([]); | ||
| const [selectedVersion, setSelectedVersion] = useState<any>(); |
There was a problem hiding this comment.
Unused variable setSelectedVersion.
| const [selectedVersion, setSelectedVersion] = useState<any>(); | |
| const [selectedVersion] = useState<any>(); |
2 participants
Problem
The EditorDialog component assumed optional Helm release fields
(e.g. chart metadata and values) were always present. In real-world
clusters, partial or delayed responses can cause runtime crashes.
Solution
Testing