EDM-2249: The yaml tab is now a YAML editor similar to the OCP console#366
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-10-09T12:43:53.274ZApplied to files:
📚 Learning: 2025-10-09T12:47:50.431ZApplied to files:
📚 Learning: 2025-04-16T12:05:30.744ZApplied to files:
📚 Learning: 2025-03-12T09:09:44.139ZApplied to files:
🧬 Code graph analysis (3)libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (2)
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx (2)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
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: 1
🧹 Nitpick comments (2)
package.json (1)
37-55: Drop redundant@types/fast-json-patch.
fast-json-patch@^3already ships bundled TypeScript declarations, so keeping the separate DefinitelyTyped package is redundant and risks getting out of sync with the runtime API. Please remove@types/fast-json-patchunless you have a specific augmentation to keep.libs/ui-components/src/components/Fleet/FleetDetails/FleetYaml.tsx (1)
8-11: Consider explicit field removal for clarity.The current approach sets
devicesSummary: undefinedand relies on serialization to omit it. While functional, explicitly destructuring the field is more self-documenting:-const FleetYaml = ({ fleet, refetch }: { fleet: Fleet; refetch: VoidFunction }) => { - const fleetWithoutSummary = { ...fleet, status: { ...fleet.status, devicesSummary: undefined } } as Fleet; +const FleetYaml = ({ fleet, refetch }: { fleet: Fleet; refetch: VoidFunction }) => { + const { devicesSummary, ...statusWithoutSummary } = fleet.status || {}; + const fleetWithoutSummary: Fleet = { ...fleet, status: statusWithoutSummary };This avoids the type assertion and makes the field removal explicit. However, if the Fleet type definition allows omitting
devicesSummary, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
libs/i18n/locales/en/translation.json(1 hunks)libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx(2 hunks)libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx(2 hunks)libs/ui-components/src/components/Fleet/FleetDetails/FleetYaml.tsx(1 hunks)libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryDetails.tsx(1 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx(3 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx(3 hunks)libs/ui-components/src/utils/error.ts(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useThemePreferences.ts (1)
useThemePreferences(41-91)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (3)
libs/ui-components/src/utils/error.ts (2)
getErrorMessage(6-17)isResourceVersionTestFailure(26-29)libs/ui-components/src/utils/labels.ts (1)
fromAPILabel(7-12)libs/ui-components/src/utils/patch.ts (1)
getLabelPatches(368-418)
libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (2)
libs/ui-components/src/utils/devices.ts (1)
getEditDisabledReason(13-21)libs/ui-components/src/utils/tooltip.ts (1)
getDisabledTooltipProps(1-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build ocp plugin
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (3)
libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (2)
88-89: LGTM! Good refactor for reusability.Extracting
editDisabledReasoneliminates duplicate logic and enables consistent disabled state across both the edit action dropdown and the YAML editor.
192-192: LGTM! Props align with the updated YamlEditor API.The YAML editor now correctly receives
disabledEditReasonto enforce edit restrictions based on fleet binding and device enrollment status.libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx (1)
18-18: LGTM! Clean delegation to FleetYaml component.Using a dedicated wrapper component encapsulates fleet-specific YAML editing concerns and simplifies the props interface.
Also applies to: 103-103
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (1)
138-149: Validate YAML structure after parsing.The YAML is parsed and cast to
Rwithout validating that it matches the expected resource structure. While the patch creation and backend validation will catch structural mismatches, consider adding a basic validation step (e.g., checking for required fields likekind,metadata, andspec) to provide earlier, more user-friendly error messages.Apply this diff to add basic validation:
let updatedObj: R; try { updatedObj = load(updatedYaml) as R; + // Basic structure validation + if (!updatedObj || typeof updatedObj !== 'object' || !updatedObj.kind || !updatedObj.metadata) { + throw new Error('Invalid resource structure: missing kind or metadata'); + } } catch (error) { setSaveError({ hasConflict: false, message: getErrorMessage(error) }); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx(3 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: celdrake
PR: flightctl/flightctl-ui#366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:59-64
Timestamp: 2025-10-09T12:43:53.256Z
Learning: In the flightctl-ui codebase, the YamlEditor and YamlEditorBase components are only used for updating existing resources via PATCH operations, not for creating or deleting resources. Empty YAML content is not a valid use case, so checking for truthy yamlContent before calling onSave is appropriate.
📚 Learning: 2025-10-09T12:43:53.256Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:59-64
Timestamp: 2025-10-09T12:43:53.256Z
Learning: In the flightctl-ui codebase, the YamlEditor and YamlEditorBase components are only used for updating existing resources via PATCH operations, not for creating or deleting resources. Empty YAML content is not a valid use case, so checking for truthy yamlContent before calling onSave is appropriate.
Applied to files:
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx
🧬 Code graph analysis (1)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (3)
libs/ui-components/src/utils/error.ts (2)
getErrorMessage(6-17)isResourceVersionTestFailure(26-29)libs/ui-components/src/utils/labels.ts (1)
fromAPILabel(7-12)libs/ui-components/src/utils/patch.ts (1)
getLabelPatches(368-418)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
🔇 Additional comments (15)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (15)
1-17: LGTM!The imports are well-organized and all additions align with the new save/conflict-handling functionality. The use of the
typemodifier for monaco-editor imports is appropriate.
19-28: LGTM!The type definitions correctly reflect the new API surface:
filenameis now derived internally viagetFilename, anddisabledEditReasonis exposed for controlling editability.
30-40: LGTM!The error handling gracefully converts exceptions to user-visible YAML comments, and
lineWidth: -1prevents unwanted line wrapping in the editor.
42-55: LGTM!The resource-to-endpoint mapping is clear and includes appropriate error handling for unsupported kinds.
57-58: LGTM!Limiting mutable paths to
/specand/metadata/labelsaligns with typical Kubernetes-style resource editing patterns where status and system-managed metadata are immutable.
60-80: Conservative fallback strategy looks good.The fallback correctly handles labels via
getLabelPatchesand spec via replace. UsingJSON.stringifyfor deep equality onspecis acceptable in this fallback path, though note that key ordering differences could trigger false positives.
82-88: LGTM!Using the Device's alias label (when available) for the filename provides better UX, with a sensible fallback to
metadata.name.
90-104: Patch generation logic is sound.The strategy of filtering for mutable paths and checking operation support is appropriate. The fallback to
getFallbackPatcheswhen unsupported operations are detected is conservative and prevents unexpected modifications. The inline comment (lines 101-102) correctly acknowledges the trade-off.
106-123: LGTM!The state management is comprehensive and properly tracks the save lifecycle. The
saveErrorstructure distinguishes conflict scenarios from other errors, enabling appropriate user guidance. TheeditorRefenables programmatic editor updates during reload.
125-136: LGTM!The effect properly synchronizes the editor's content with external updates. Using
editorRef.current.setValue()ensures Monaco reflects the reloaded state, and thedoUpdateflag prevents infinite re-renders.
162-166: Excellent concurrency control pattern.Prepending the
testoperation ensures the resource is only modified if the UI has the latest version, preventing race conditions and lost updates.
177-186: LGTM!The error handling correctly distinguishes resource version conflicts from other errors, enabling tailored user guidance. The
finallyblock ensuresisSavingis always reset.
189-207: LGTM!All props are correctly wired to
YamlEditorBase, including the newly addeddisabledEditReasonandeditorReffor programmatic control.
209-241: Clear user feedback for all save states.The alerts provide appropriate guidance for each scenario:
- Success: Confirms the update with the new resource version
- Conflict: Instructs users to preserve changes, reload, and reapply (preventing data loss)
- Error: Shows the error message and advises reload or fix
The UX patterns align well with similar editors in OpenShift console.
243-247: LGTM!The reload notice alerts users when the resource has been updated externally, prompting them to refresh to avoid working with stale data.
celdrake
force-pushed
the
EDM-2249-yaml-editor
branch
from
a120c0a to
59007f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (1)
155-160: Address UX for no-op saves (no patches).Silent return on no-op save can confuse users; surface a "No changes detected" message or similar.
As previously discussed, track with a follow-up issue and align with UX.
🧹 Nitpick comments (2)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (2)
210-218: Avoid showing “version unknown” in success alert.If resourceVersion is missing, the alert renders “version unknown”. Prefer hiding the version or using a neutral string.
Apply a guard:
- title={t('{{ resourceName }} has been updated to version {{ version }}', { - resourceName, - version: yamlResourceVersion, - })} + title={ + yamlResourceVersion === 'unknown' + ? t('{{ resourceName }} has been updated', { resourceName }) + : t('{{ resourceName }} has been updated to version {{ version }}', { + resourceName, + version: yamlResourceVersion, + }) + }
233-239: Use neutral copy for generic save errors (not always invalid YAML).Non-conflict failures may be server-side validation or network errors. “The current YAML is invalid” can mislead.
Replace with a neutral message:
- {t('The current YAML is invalid. Fix the errors, or click ʼReloadʼ to discard your changes.')} + {t('Save failed. Review the error below, fix the content, or click ʼReloadʼ to discard your changes.')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
libs/ui-components/src/components/Fleet/FleetDetails/FleetYaml.tsx(1 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx(3 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx(3 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- libs/ui-components/src/components/Fleet/FleetDetails/FleetYaml.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: celdrake
PR: flightctl/flightctl-ui#366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:59-64
Timestamp: 2025-10-09T12:43:53.256Z
Learning: In the flightctl-ui codebase, the YamlEditor and YamlEditorBase components are only used for updating existing resources via PATCH operations, not for creating or deleting resources. Empty YAML content is not a valid use case, so checking for truthy yamlContent before calling onSave is appropriate.
📚 Learning: 2025-10-09T12:43:53.256Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:59-64
Timestamp: 2025-10-09T12:43:53.256Z
Learning: In the flightctl-ui codebase, the YamlEditor and YamlEditorBase components are only used for updating existing resources via PATCH operations, not for creating or deleting resources. Empty YAML content is not a valid use case, so checking for truthy yamlContent before calling onSave is appropriate.
Applied to files:
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsxlibs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx
📚 Learning: 2025-10-09T12:47:50.396Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:0-0
Timestamp: 2025-10-09T12:47:50.396Z
Learning: In the flightctl-ui codebase, validation logic for YAML content (such as checking for empty/invalid content) is handled in the `YamlEditor` component rather than `YamlEditorBase`, to keep the base component content-agnostic and focused on editor functionality.
Applied to files:
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx
🧬 Code graph analysis (2)
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useThemePreferences.ts (1)
useThemePreferences(41-91)
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (3)
libs/ui-components/src/utils/error.ts (2)
getErrorMessage(6-17)isResourceVersionTestFailure(26-29)libs/ui-components/src/utils/labels.ts (1)
fromAPILabel(7-12)libs/ui-components/src/utils/patch.ts (1)
getLabelPatches(368-418)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration-tests
- GitHub Check: Build ocp plugin
- GitHub Check: Build
celdrake
force-pushed
the
EDM-2249-yaml-editor
branch
from
59007f3 to
b7ecb71
Compare
celdrake
force-pushed
the
EDM-2249-yaml-editor
branch
from
b7ecb71 to
39675a7
Compare
|
@coderabbitai can you send the review? The CI is pending due to this |
|
@celdrake, I'll trigger the review now. The review will be posted shortly and should unblock your CI. 🧠 Learnings used✅ Actions performedReview triggered.
|
2 participants
Features:
We use fast-json-patch to obtain the list of JSON patches that are needed to modify the object.
We also use a "test" operation that ensures that the resource will only be modified if the UI loaded the last version of the resource.
Some of the features in the OpenShift console's YAML editor are still missing (schema suggestions, sidebar, etc). These will be done in follow-up PRs.
Summary by CodeRabbit
New Features
Translations
Chores