fix(app-catalog): prevent EditorDialog crash on invalid YAML input#519
fix(app-catalog): prevent EditorDialog crash on invalid YAML input#519Darshika11b wants to merge 3 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
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent the EditorDialog component from crashing when handling invalid YAML input in Helm releases. The main fix adds error handling to the handleEditorChange function by wrapping YAML parsing in a try-catch block and displaying a user-friendly error message. However, the PR description claims to add null-safety for optional chart and release fields, which is not actually implemented in the code changes.
Changes:
- Added try-catch error handling for YAML parsing in handleEditorChange function
- Added null check for empty string values in Monaco Editor onChange callback
- Minor code cleanup (formatting, type annotations, simplified conditionals)
- Added new .gitignore file (with formatting issues)
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| app-catalog/src/components/releases/EditorDialog.tsx | Added error handling for YAML parsing, improved type safety, code formatting improvements, and removed JSDoc parameter documentation |
| app-catalog/.gitignore | New gitignore file added with standard Node.js/React patterns (contains formatting errors on lines 28-29) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [valuesToShow, setValuesToShow] = useState( | ||
| Object.assign({}, release.chart.values, release.config) | ||
| ); |
There was a problem hiding this comment.
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)
| const [values, setValues] = useState( | ||
| Object.assign({}, release.chart.values, release.config) | ||
| ); |
There was a problem hiding this comment.
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.
| const metadataName = | ||
| release.chart.metadata.name === APP_CATALOG_HELM_REPOSITORY | ||
| ? '/' + release.chart.metadata.name | ||
| : release.chart.metadata.name; |
There was a problem hiding this comment.
Direct access to release.chart.metadata.name without null-safety checks. If release.chart or release.chart.metadata is null/undefined, this will crash. Consider using optional chaining: release.chart?.metadata?.name
| setIsUserValues(false); | ||
| setValuesToShow(Object.assign({}, release.chart.values, release.config)); | ||
| setValuesToShow( | ||
| Object.assign({}, release.chart.values, release.config) |
There was a problem hiding this comment.
Same null-safety issue. Direct access to release.chart.values without null checks could crash if release.chart is null/undefined.
| Object.assign({}, release.chart.values, release.config) | |
| Object.assign({}, release?.chart?.values ?? {}, release?.config ?? {}) |
| @@ -20,15 +20,6 @@ | |||
|
|
|||
| /** | |||
| * EditorDialog component for displaying and editing Helm release configurations. | |||
There was a problem hiding this comment.
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.
| * 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. |
| // ✅ 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, | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
2 participants
What this PR does
Why
In real-world clusters, Helm releases may have missing metadata or invalid values.yaml.
This caused EditorDialog to crash. The fix ensures graceful handling.
Testing