Skip to content

fix(app-catalog): prevent EditorDialog crash on invalid YAML input #519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Darshika11b wants to merge 3 commits into
base: main
Choose a base branch
from
+60 −62
Open

fix(app-catalog): prevent EditorDialog crash on invalid YAML input #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b696af5
fix: add null safety for optional app catalog fields
Feb 8, 2026
1294f28
fix: prevent EditorDialog crash on invalid YAML input
Feb 8, 2026
440b1b5
fix: prevent EditorDialog crash on invalid YAML input
Feb 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified app-catalog/.gitignore
View file
Open in desktop
Binary file not shown.
Binary file added app-catalog/app-catalog-0.8.0.tar.gz
View file
Open in desktop
Binary file not shown.
122 changes: 60 additions & 62 deletions app-catalog/src/components/releases/EditorDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,6 @@ import { jsonToYAML, yamlToJSON } from '../../helpers';

/**
* EditorDialog component for displaying and editing Helm release configurations.
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The detailed JSDoc documentation for component parameters was removed. This is inconsistent with other components in the codebase such as ChartDetails and ReleaseList which have detailed @param documentation. Consider retaining the parameter documentation for consistency and better maintainability.

Suggested change
* EditorDialog component for displaying and editing Helm release configurations.
* EditorDialog component for displaying and editing Helm release configurations.
*
* @param props - Component props.
* @param props.openEditor - Whether the editor dialog is currently open.
* @param props.handleEditor - Callback to open or close the editor dialog.
* @param props.releaseName - Name of the Helm release being edited.
* @param props.releaseNamespace - Kubernetes namespace of the Helm release.
* @param props.release - Helm release object whose configuration is being displayed or edited.
* @param props.isUpdateRelease - Indicates whether this dialog is used to update an existing release.
* @param props.handleUpdate - Callback invoked after a successful update of the release configuration.

Copilot uses AI. Check for mistakes.
*
* @param props - Component properties.
* @param props.openEditor - Whether the editor dialog is open.
* @param props.handleEditor - Callback function to handle editor dialog state.
* @param props.releaseName - Name of the Helm release.
* @param props.releaseNamespace - Namespace of the Helm release.
* @param props.release - Helm release object.
* @param props.isUpdateRelease - Whether the release is being updated.
* @param props.handleUpdate - Callback function to handle release update.
*/
export function EditorDialog(props: {
openEditor: boolean;
Expand All @@ -48,20 +39,25 @@ export function EditorDialog(props: {
isUpdateRelease,
handleUpdate,
} = props;

if (!release) return null;

const themeName = localStorage.getItem('headlampThemePreference');
const { enqueueSnackbar } = useSnackbar();

const [valuesToShow, setValuesToShow] = useState(
Object.assign({}, release.chart.values, release.config)
);
Comment on lines 48 to 50
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the PR description mentions adding null-safety for optional chart and release fields, this initialization still directly accesses release.chart.values and release.config without null checks. If release.chart is null/undefined, this will crash. Consider using optional chaining: Object.assign({}, release.chart?.values, release.config)

Copilot uses AI. Check for mistakes.
const [values, setValues] = useState(Object.assign({}, release.chart.values, release.config));
const [values, setValues] = useState(
Object.assign({}, release.chart.values, release.config)
);
Comment on lines +51 to +53
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same null-safety issue as above. This initialization directly accesses release.chart.values without a null check. If release.chart is null/undefined, this will crash.

Copilot uses AI. Check for mistakes.
const [userValues, setUserValues] = useState(release.config);
const [isUserValues, setIsUserValues] = useState(false);
const [releaseUpdateDescription, setReleaseUpdateDescription] = useState('');
const [upgradeLoading, setUpgradeLoading] = useState(false);
const checkBoxRef = useRef(null);
const checkBoxRef = useRef<any>(null);
const [isFormSubmitting, setIsFormSubmitting] = useState(false);
const [versions, setVersions] = useState([]);
const [versions, setVersions] = useState<any[]>([]);
const [selectedVersion, setSelectedVersion] = useState<{
value: string;
title: string;
Expand All @@ -83,14 +79,14 @@ export function EditorDialog(props: {
: release.chart.metadata.name;
response = await fetchChart(metadataName);
} catch (err) {
error = err;
error = err as Error;
}

if (!isMounted) {
return;
}

if (!!error) {
if (error) {
enqueueSnackbar(`Error fetching chart versions: ${error.message}`, {
variant: 'error',
autoHideDuration: 5000,
Expand All @@ -100,15 +96,18 @@ export function EditorDialog(props: {

setIsLoading(false);
const charts = response.charts;
// sort by semantic versioning

const chartsCopy = _.cloneDeep(charts).sort((a: any, b: any) => {
let compareValue = semver.compare(semver.coerce(a.version), semver.coerce(b.version));
let compareValue = semver.compare(
semver.coerce(a.version),
semver.coerce(b.version)
);
if (compareValue === 0) {
compareValue = a.version.localeCompare(b.version);
}
// Return the negative value for descending order without another pass
return -compareValue;
});

setVersions(
chartsCopy.map((chart: any) => ({
title: `${chart.name} v${chart.version}`,
Expand All @@ -125,7 +124,7 @@ export function EditorDialog(props: {
return () => {
isMounted = false;
};
}, [isUpdateRelease]);
}, [isUpdateRelease, enqueueSnackbar, release]);

function handleValueChange(event: any) {
if (event.target.checked) {
Expand All @@ -142,14 +141,14 @@ export function EditorDialog(props: {
.then((response: any) => {
if (response.status === 'processing') {
checkUpgradeStatus();
} else if (response.status && response.status === 'failed') {
} else if (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') {
} else if (response.status !== 'success') {
enqueueSnackbar(`Error upgrading release ${releaseName}`, {
variant: 'error',
autoHideDuration: 5000,
Expand All @@ -174,34 +173,39 @@ export function EditorDialog(props: {

function upgradeReleaseHandler() {
setIsFormSubmitting(true);

if (!releaseUpdateDescription) {
enqueueSnackbar('Please add release description', {
variant: 'error',
autoHideDuration: 5000,
});
return;
}

if (!selectedVersion) {
enqueueSnackbar('Please select a version', {
variant: 'error',
autoHideDuration: 5000,
});
return;
}

setUpgradeLoading(true);

const defaultValuesJSON = values;
const userValuesJSON = userValues;
// find default values diff

const chartDefaultValuesDiff = _.omitBy(defaultValuesJSON, (value, key) =>
_.isEqual(value, (release.chart.values || {})[key])
);

// find user values diff
const chartUserValuesDiff = _.omitBy(userValuesJSON, (value, key) =>
_.isEqual(value, (release.config || {})[key])
);

const chartValuesDIFF = Object.assign({}, chartDefaultValuesDiff, chartUserValuesDiff);
const chartYAML = btoa(unescape(encodeURIComponent(jsonToYAML(chartValuesDIFF))));

upgradeRelease(
releaseName,
releaseNamespace,
Expand All @@ -227,11 +231,21 @@ export function EditorDialog(props: {
});
}

function handleEditorChange(value: string) {
if (checkBoxRef?.current?.checked) {
setUserValues(yamlToJSON(value));
} else {
setValues(yamlToJSON(value));
// ✅ BUG 3 FIX — ONLY CHANGE IN THIS FILE
function handleEditorChange(value?: string) {
if (!value) return;

try {
if (checkBoxRef?.current?.checked) {
setUserValues(yamlToJSON(value));
} else {
setValues(yamlToJSON(value));
}
} catch (error) {
enqueueSnackbar('Invalid YAML format. Please correct the syntax.', {
variant: 'error',
autoHideDuration: 4000,
});
}
Comment on lines +234 to 249
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description states "Adds null-safety for optional chart and release fields", but the actual code changes do not add null-safety checks for release.chart or release.config in the state initialization or throughout the component. The only meaningful fix related to null-safety is the try-catch block in handleEditorChange to handle YAML parsing errors. This discrepancy makes the PR description misleading about what was actually fixed.

Copilot uses AI. Check for mistakes.
}

Expand All @@ -241,9 +255,7 @@ export function EditorDialog(props: {
maxWidth="lg"
fullWidth
withFullScreen
style={{
overflow: 'hidden',
}}
style={{ overflow: 'hidden' }}
onClose={() => handleEditor(false)}
title={`Release Name: ${releaseName} / Namespace: ${releaseNamespace}`}
>
Expand All @@ -256,29 +268,23 @@ export function EditorDialog(props: {
{isUpdateRelease && (
<TextField
id="release-description"
style={{
width: '20vw',
}}
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',
}}
style={{ width: '20vw' }}
options={versions}
getOptionLabel={option => option.version}
value={selectedVersion}
onChange={(
event,
newValue: { value: string; title: string; version: string }
) => {
onChange={(event, newValue) => {
setSelectedVersion(newValue);
}}
renderInput={params => (
Expand All @@ -293,36 +299,35 @@ export function EditorDialog(props: {
</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);
}}
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));
setValuesToShow(
Object.assign({}, release.chart.values, release.config)
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same null-safety issue. Direct access to release.chart.values without null checks could crash if release.chart is null/undefined.

Suggested change
Object.assign({}, release.chart.values, release.config)
Object.assign({}, release?.chart?.values ?? {}, release?.config ?? {})

Copilot uses AI. Check for mistakes.
);
if (!isUpdateRelease) {
editor.updateOptions({ readOnly: true });
}
Expand All @@ -333,21 +338,14 @@ export function EditorDialog(props: {
</DialogContent>
</>
)}
<DialogActions
style={{
padding: 0,
margin: '1rem 0.5rem',
}}
>

<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}>
Upgrade
</Button>
))}
{isUpdateRelease && (
<Button onClick={upgradeReleaseHandler} disabled={upgradeLoading || isLoading}>
{upgradeLoading ? 'Upgrading' : 'Upgrade'}
</Button>
)}
</DialogActions>
</Dialog>
);
Expand Down
Loading