EM-2629 add support for single container apps#382
Conversation
WalkthroughAdds explicit single-container app support, new volume and port handling, ApplicationContainerForm and ApplicationVolumeForm components, extensive form/validation updates for ports, CPU/memory and volumes, i18n keys, updated device spec types, and JSON-patch-based diff generation for application updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Templates as ApplicationTemplates
participant ContainerForm as ApplicationContainerForm
participant VolumeForm as ApplicationVolumeForm
participant Validations
participant Utils as DeviceSpecUtils
User->>Templates: choose "Single Container" and open app
Templates->>ContainerForm: render SingleContainerAppForm
User->>ContainerForm: add/edit port "8080:3000"
ContainerForm->>Validations: validate mapping & duplicates
Validations-->>ContainerForm: validation result
ContainerForm->>VolumeForm: add volume (type-dependent fields)
VolumeForm->>Validations: validate volume fields
User->>Templates: save app
Templates->>Utils: toAPIApplication(form)
Utils->>Utils: convertVolumesToForm & build API spec
Utils-->>Templates: API-ready application object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
celdrake
force-pushed
the
EDM-2629-add-support-for-single-container-apps
branch
from
6506b33 to
e6d0863
Compare
celdrake
force-pushed
the
EDM-2629-add-support-for-single-container-apps
branch
2 times, most recently
from
d4a5070 to
3561f60
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css (1)
1-5: Border override works but consider PF tokens / avoiding!importantThe rule is fine functionally, but using a hardcoded
gray+!importantcan diverge from PatternFly theming (especially in dark mode). Consider a PF border color token (e.g.,var(--pf-v5-global--BorderColor--100)) and increasing specificity instead of!importantif possible.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (1)
9-26: Centralized app labeling is clean; verify behavior for nameless, non‑image appsThe new
getAppFormatLabel/getAppNamehelpers make type/label handling clearer and keep AppType mapping explicit with a sensible'Unknown'fallback. Rendering{name} ({formatType})reads well for typical cases (named or image-based apps).For apps that have neither
namenor animageproperty (if such forms exist),getAppNamereturns an empty string, so the UI will show just"(Format)". If that’s not desirable, you could default to something liket('Unnamed')in that branch.Also applies to: 37-43
libs/ui-components/src/types/deviceSpec.ts (1)
143-149: Consider adding validation for volumeType combinations.While making
volumeTypeoptional (Line 145) provides flexibility during form entry, ensure validation logic enforces that at least one ofimageReformountPathis present when submitting, to prevent invalid volume configurations.This aligns with the validation suggested in the
toAPIApplicationfunction review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/i18n/locales/en/translation.json(6 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(7 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx(2 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(10 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
📚 Learning: 2025-10-29T16:47:29.614Z
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.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx
📚 Learning: 2025-03-20T12:37:36.986Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/types/deviceSpec.ts:0-0
Timestamp: 2025-03-20T12:37:36.986Z
Learning: In the FlightCtl UI project, `name` is required for inline applications but optional for image applications. This is enforced at the type level in the `InlineAppForm` type.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsxlibs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx
📚 Learning: 2025-06-17T09:28:36.257Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 298
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/DeviceTemplateStep.tsx:95-100
Timestamp: 2025-06-17T09:28:36.257Z
Learning: In the FlightCtl UI components, the `useAppLinks('createAcmRepo')` hook is designed to always return a valid link for ACM deployments, so conditional rendering of LearnMoreLink components using this link is not necessary.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx
📚 Learning: 2025-02-04T09:04:36.106Z
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.
Applied to files:
libs/i18n/locales/en/translation.json
📚 Learning: 2025-03-19T08:57:16.233Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:57:16.233Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by both frontend validation logic and backend API constraints, ensuring that no inline application can exist in the system without a name field.
Applied to files:
libs/i18n/locales/en/translation.json
📚 Learning: 2025-11-24T13:42:36.897Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-12T08:41:34.720Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewTrackedSystemdServices.tsx:3-3
Timestamp: 2025-03-12T08:41:34.720Z
Learning: In the flightctl-ui codebase, SystemdUnitsFormValues is defined as { systemdUnits: SystemdUnitFormValue[] }, allowing for direct destructuring of the systemdUnits property in component props.
Applied to files:
libs/ui-components/src/components/form/validations.ts
📚 Learning: 2025-05-26T11:41:40.796Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 300
File: libs/ui-components/src/components/form/validations.ts:459-461
Timestamp: 2025-05-26T11:41:40.796Z
Learning: In TypeScript validation contexts, when working with properly typed form data like `BatchForm`, required array fields (without `?` marker) are guaranteed to be defined arrays, making defensive checks for undefined unnecessary in those specific contexts.
Applied to files:
libs/ui-components/src/components/form/validations.ts
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
libs/ui-components/src/types/deviceSpec.ts
🧬 Code graph analysis (6)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx (1)
libs/ui-components/src/types/deviceSpec.ts (2)
QuadletImageAppForm(70-74)ComposeImageAppForm(83-87)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (4)
libs/types/index.ts (1)
ImagePullPolicy(123-123)libs/ui-components/src/types/deviceSpec.ts (1)
ApplicationVolumeForm(143-149)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx (1)
libs/ui-components/src/types/deviceSpec.ts (2)
QuadletInlineAppForm(76-81)ComposeInlineAppForm(89-94)
libs/ui-components/src/components/form/validations.ts (2)
libs/ui-components/src/types/deviceSpec.ts (9)
QuadletInlineAppForm(76-81)ComposeInlineAppForm(89-94)PortMapping(56-59)AppForm(113-118)isSingleContainerAppForm(134-135)isQuadletImageAppForm(126-127)isComposeImageAppForm(130-131)QuadletImageAppForm(70-74)ComposeImageAppForm(83-87)libs/types/index.ts (1)
AppType(19-19)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
libs/types/index.ts (1)
AppType(19-19)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (3)
libs/types/index.ts (1)
AppType(19-19)libs/ui-components/src/types/deviceSpec.ts (2)
AppForm(113-118)getAppIdentifier(153-162)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
🪛 Biome (2.1.2)
libs/ui-components/src/components/form/validations.ts
[error] 641-641: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 656-656: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (22)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx (1)
100-105: Using config name as section title looks goodFalling back to the translated “Configuration {{configNum}}” when
nameis empty makes sections easier to identify without changing behavior for unnamed configs. The duplication withdescription={name}is acceptable and consistent.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx (1)
8-16: Prop typing aligned with Quadlet/Compose image formsNarrowing
apptoQuadletImageAppForm | ComposeImageAppFormmatches how this component is used and keepsapp.imageaccess type-safe for both variants. No behavioral concerns here.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (2)
24-39: Volume/pull‑policy option helpers look consistent with the new model
getPullPolicyOptionsandgetVolumeTypeOptionscorrectly expose the allowed combinations, especially hidingIMAGE_ONLYfor single-container apps while keepingIMAGE_MOUNTavailable. This matches the intended capability split for container vs. non-container apps.
58-198: Overall volume form structure and Formik wiring look solidBeyond the issues above, the FieldArray integration, conditional
hasImage/hasMountrendering, and defaultimagePullPolicyfor new entries are all consistent with the new volume model and with other wizard steps. Error handling viauseField+ErrorHelperTextis also aligned with existing patterns.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx (1)
12-21: Inline app type widening is consistent with new form variantsUsing
QuadletInlineAppForm | ComposeInlineAppFormfor bothfileandappprops cleanly aligns this component with the new AppForm union, and it preserves the “inline apps require a name” invariant. No issues from a typing or runtime perspective.Also applies to: 54-61
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
33-37: Container-aware templates, labeling, and name behavior look coherentAdding
AppType.AppTypeContainertoappTypeOptions, using the application name as the section title, and reusing the same “Application name” helper text across types keeps the UX consistent while introducing the single-container path. The defaults pushed in the “Add application” handler (container + OCI_IMAGE with empty ports/volumes) match the newApplicationContainerFormand validation schema expectations.Also applies to: 106-121, 171-181, 256-318
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx (1)
26-90: Single-container form wiring and port/volume handling look solidFormik wiring for
ports, inline add/edit/delete behavior, CPU/memory limit fields, and delegation toApplicationVolumeFormare consistent with the new validation helpers and types. No functional issues spotted in this component; any port-number semantics are correctly delegated tovalidatePortNumber/isValidPortMappinginvalidations.ts.Also applies to: 93-172, 191-273, 274-315
libs/i18n/locales/en/translation.json (1)
339-361: New translations cover all added validation and container UI textThe added keys for port mappings, image references, CPU/memory limits, volume configuration, pull policies, and stricter name rules line up with usages in the new container/volume forms and validation helpers. This should keep CI i18n checks happy and avoids hard‑coded strings in the components.
Also applies to: 370-391, 392-416, 485-489, 710-753, 732-743
libs/ui-components/src/components/form/validations.ts (2)
226-230: Shared name validator keeps app and volume naming consistent (and inline semantics intact)
validApplicationAndVolumeNamecentralizes the existing lowercase/dash rule and is applied consistently across:
- single-container app names (required),
- quadlet/compose inline app names (required),
- quadlet/compose image app names (optional but validated when present), and
- volume names in single-container apps (required).
This preserves the “inline apps must have a name” behavior while giving all named entities a uniform, Kubernetes‑friendly pattern. Based on learnings, this is aligned with the existing inline-name requirement and extends it cleanly to the new forms.
Also applies to: 588-666
639-661: The proposed refactoring syntax is incorrect and does not match Yup's functional form API.Yup's functional form of
when()uses the signature(values: any[], schema: Schema) => Schema, where a single dependency key receives an array with one element. The correct pattern is([volumeType], schema) => { ... }, not(volumeType: VolumeType, schema) => { ... }as suggested in the diff.The codebase already demonstrates the correct pattern elsewhere in the same file (lines 1033–1035), where
.when('mode', ([mode]) => ...)is used. Following the review comment's suggestion would cause a runtime or TypeScript error becausevolumeTypewould beundefinedinstead of receiving the field value.The refactored code should destructure the values array:
- imageRef: Yup.string().when('volumeType', { - is: (val: VolumeType) => val === VolumeType.IMAGE_ONLY || val === VolumeType.IMAGE_MOUNT, - then: (schema) => - schema - .required(t('Image reference is required for this volume type')) - .matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')), - otherwise: (schema) => - schema.matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')), - }), + imageRef: Yup.string().when('volumeType', ([volumeType], schema) => { + if (volumeType === VolumeType.IMAGE_ONLY || volumeType === VolumeType.IMAGE_MOUNT) { + return schema + .required(t('Image reference is required for this volume type')) + .matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')); + } + return schema.matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')); + }),Likely an incorrect or invalid review comment.
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (4)
1-1: LGTM! New dependency for patch generation.The
fast-json-patchlibrary provides a robust implementation for generating JSON patches, replacing the previous bespoke logic. This is a good choice for maintainability.
244-264: LGTM! Proper handling of single-container apps.The implementation correctly maps single-container app forms to the API spec, including:
- Optional name field
- Port mapping in "hostPort:containerPort" format
- Conditional resource limits
266-277: LGTM! Correct handling for Quadlet/Compose image apps.This branch appropriately handles image-based Quadlet and Compose applications without port mappings or resource limits, which aligns with the differentiation between single-container and Quadlet/Compose apps.
295-341: The 'test' operation is already backend-supported and actively used in the codebase.The
PatchRequesttype explicitly definesop: 'add' | 'replace' | 'remove' | 'test', confirming backend API support. The refactored code uses the samesupportedOpsarray that already exists inYamlEditor.tsx, where the 'test' operation is actively used to ensure patches only succeed when updating the latest resourceVersion. The refactored function maintains consistency with established patterns and introduces no new risk.Likely an incorrect or invalid review comment.
libs/ui-components/src/types/deviceSpec.ts (8)
1-16: LGTM! Necessary imports for new functionality.The added imports
ApplicationResourceLimitsandImagePullPolicysupport the new single-container app features (resource limits and volume image pull policies).
48-54: LGTM! AppBase provides solid foundation.Making
specTypea required field (Line 50) improves type safety by ensuring all app forms explicitly declare whether they're image-based or inline.
56-68: LGTM! SingleContainerAppForm properly typed.The type correctly requires
name(Line 64) for single-container apps and includes optionalportsandlimitsfields. Using strings for port fields is appropriate for form handling.
70-94: LGTM! Clear separation of Quadlet and Compose variants.The four types properly distinguish between:
- Image vs. Inline (via
specType)- Quadlet vs. Compose (via
appType)Inline forms correctly require
name(Lines 79, 92), while image forms keep it optional, aligning with FlightCtl requirements.Based on learnings, name is required for inline applications but optional for image applications.
113-118: LGTM! Comprehensive AppForm union.The union type encompasses all five explicit app form variants, providing exhaustive coverage of the application type combinations.
125-136: LGTM! Type guards correctly discriminate app forms.The type guards properly check both
appTypeandspecTypefor Quadlet/Compose variants. TheisSingleContainerAppFormguard (Line 135) only needs to checkappTypesinceAppTypeContaineris exclusively used for single-container apps.
137-141: LGTM! Clear volume type semantics.The
VolumeTypeenum provides explicit distinction between image-only, mount-only, and combined image-mount volumes, improving type safety and clarity.
153-162: LGTM! Robust identifier resolution.The function correctly prioritizes
nameand falls back toimagefor image-based apps (Lines 155, 158), while inline apps use onlyname(Line 161) since it's always required for them.
celdrake
force-pushed
the
EDM-2629-add-support-for-single-container-apps
branch
from
3561f60 to
7540c56
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
221-242: Volume mapping still has type safety concerns.The type safety issues flagged in previous review remain:
- Line 223: Empty string default for
v.namemasks upstream validation issues- Lines 227-228: If
volumeTypeis undefined (it's optional inApplicationVolumeForm), bothhasImageandhasMountare false, resulting in a volume with only anamefield that violates theApplicationVolumeAPI contract- Line 241: The
as ApplicationVolumecast hides these violationsWhile form validation should prevent these cases from reaching this function, defensive checks here would improve robustness and make type violations explicit.
Consider adding assertions:
const volumes = app.volumes?.map((v) => { + if (!v.name) { + throw new Error('Volume name is required'); + } + if (!v.volumeType) { + throw new Error('Volume type is required'); + } const volume: Partial<ApplicationVolume & ImageMountVolumeProviderSpec> = { - name: v.name || '', + name: v.name, };
🧹 Nitpick comments (2)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (2)
398-419: Default empty strings inconvertVolumesToFormmay cause confusion.The function defaults
imageRefandmountPathto empty strings when the API doesn't provide them, which could lead to edge cases:
- Lines 410-416: If both
imageRefandmountPathare empty strings (falsy),volumeTyperemainsundefined- This creates inconsistency with the API contract where volumes must have at least one provider type
Consider explicit undefined defaults and volume type determination:
const convertVolumesToForm = (volumes?: ApplicationVolume[]) => { if (!volumes) return []; return volumes.map((vol) => { const fullVolume = vol as ApplicationVolume & ImageMountVolumeProviderSpec; const volForm: ApplicationVolumeForm = { name: fullVolume.name, - imageRef: fullVolume.image?.reference || '', + imageRef: fullVolume.image?.reference, imagePullPolicy: fullVolume.image?.pullPolicy || ImagePullPolicy.PullIfNotPresent, - mountPath: fullVolume.mount?.path || '', + mountPath: fullVolume.mount?.path, }; // Determine volumeType based on which fields are present if (volForm.imageRef && volForm.mountPath) { volForm.volumeType = VolumeType.IMAGE_MOUNT; } else if (volForm.imageRef) { volForm.volumeType = VolumeType.IMAGE_ONLY; } else if (volForm.mountPath) { volForm.volumeType = VolumeType.MOUNT_ONLY; } + // If none are present, volumeType remains undefined (edge case that should be validated) return volForm; }); };
295-341: Clarify necessity ofsupportedOpsfilter.The code filters patches to only
['add', 'replace', 'remove', 'test']operations and falls back to a simple replace strategy if other operations are found. However,fast-json-patch'scomparefunction should only generate these standard operations by default.If the filter is defensive (for future-proofing or edge cases), consider adding a comment explaining when unsupported operations might occur. Otherwise, the fallback logic may be unnecessary:
- const supportedOps = ['add', 'replace', 'remove', 'test']; - const hasOnlySupportedOperations = apiPatches.every((patch) => supportedOps.includes(patch.op)); - if (hasOnlySupportedOperations) { - return apiPatches; - } + // fast-json-patch compare only generates standard RFC 6902 operations + return apiPatches; - - // If there are unsupported operations, use a simple replace strategy - if (currentApps.length === 0 && updatedApiApps.length > 0) { - return [ - { - path: patchPath, - op: 'add', - value: updatedApiApps, - }, - ]; - } else if (currentApps.length > 0 && updatedApiApps.length === 0) { - return [ - { - path: patchPath, - op: 'remove', - }, - ]; - } - return [ - { - path: patchPath, - op: 'replace', - value: updatedApiApps, - }, - ];If the fallback is needed for compatibility with the backend API (e.g., the API doesn't support certain operations), document this requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/i18n/locales/en/translation.json(6 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(7 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx(2 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(10 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
📚 Learning: 2025-10-29T16:47:29.614Z
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.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-06-17T09:28:36.257Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 298
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/DeviceTemplateStep.tsx:95-100
Timestamp: 2025-06-17T09:28:36.257Z
Learning: In the FlightCtl UI components, the `useAppLinks('createAcmRepo')` hook is designed to always return a valid link for ACM deployments, so conditional rendering of LearnMoreLink components using this link is not necessary.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx
📚 Learning: 2025-02-04T09:04:36.106Z
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.
Applied to files:
libs/i18n/locales/en/translation.json
📚 Learning: 2025-03-20T12:37:36.986Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/types/deviceSpec.ts:0-0
Timestamp: 2025-03-20T12:37:36.986Z
Learning: In the FlightCtl UI project, `name` is required for inline applications but optional for image applications. This is enforced at the type level in the `InlineAppForm` type.
Applied to files:
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-19T08:57:16.233Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:57:16.233Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by both frontend validation logic and backend API constraints, ensuring that no inline application can exist in the system without a name field.
Applied to files:
libs/i18n/locales/en/translation.json
📚 Learning: 2025-11-17T15:48:44.018Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx:210-213
Timestamp: 2025-11-17T15:48:44.018Z
Learning: In the flightctl-ui codebase, the UX pattern for handling disabled actions is: (1) when actions are disabled due to configuration constraints (e.g., a device belongs to a fleet and cannot be edited directly), show a disabled element with a reason; (2) when elements are hidden due to lack of user permissions (RBAC), hide the element entirely. The YAML editor follows this pattern: when canEdit is false (no permission), the Save button is completely hidden (onSave=undefined), so disabledEditReason should not be passed; when canEdit is true, disabledEditReason can be passed to disable the button for configuration reasons.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-05-26T11:41:40.796Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 300
File: libs/ui-components/src/components/form/validations.ts:459-461
Timestamp: 2025-05-26T11:41:40.796Z
Learning: In TypeScript validation contexts, when working with properly typed form data like `BatchForm`, required array fields (without `?` marker) are guaranteed to be defined arrays, making defensive checks for undefined unnecessary in those specific contexts.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/form/validations.ts
📚 Learning: 2025-03-12T09:09:44.139Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx:87-87
Timestamp: 2025-03-12T09:09:44.139Z
Learning: The `getUpdatePolicyValues` function in libs/ui-components/src/components/Fleet/CreateFleet/fleetSpecUtils.ts is designed to handle undefined input through its optional parameter definition and consistent use of optional chaining.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-03-12T09:09:44.139Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx:87-87
Timestamp: 2025-03-12T09:09:44.139Z
Learning: The `getUpdatePolicyValues` function in libs/ui-components/src/components/Fleet/CreateFleet/fleetSpecUtils.ts is designed to safely handle undefined values for the updatePolicy parameter.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-10-09T12:47:50.431Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:0-0
Timestamp: 2025-10-09T12:47:50.431Z
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/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-11-24T13:42:36.897Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-12T08:41:34.720Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewTrackedSystemdServices.tsx:3-3
Timestamp: 2025-03-12T08:41:34.720Z
Learning: In the flightctl-ui codebase, SystemdUnitsFormValues is defined as { systemdUnits: SystemdUnitFormValue[] }, allowing for direct destructuring of the systemdUnits property in component props.
Applied to files:
libs/ui-components/src/components/form/validations.ts
🧬 Code graph analysis (5)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx (1)
libs/ui-components/src/types/deviceSpec.ts (2)
QuadletImageAppForm(70-74)ComposeImageAppForm(83-87)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (4)
libs/types/index.ts (1)
ImagePullPolicy(123-123)libs/ui-components/src/types/deviceSpec.ts (1)
ApplicationVolumeForm(143-149)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
libs/types/index.ts (1)
AppType(19-19)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx (4)
libs/ui-components/src/types/deviceSpec.ts (2)
SingleContainerAppForm(61-68)PortMapping(56-59)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/form/validations.ts (3)
isValidPortMapping(577-586)validatePortNumber(542-558)isDuplicatePortMapping(566-575)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
libs/ui-components/src/components/form/validations.ts (2)
libs/ui-components/src/types/deviceSpec.ts (10)
QuadletInlineAppForm(76-81)ComposeInlineAppForm(89-94)PortMapping(56-59)AppForm(113-118)isSingleContainerAppForm(134-135)isQuadletImageAppForm(126-127)isComposeImageAppForm(130-131)QuadletImageAppForm(70-74)ComposeImageAppForm(83-87)isQuadletInlineAppForm(128-129)libs/types/index.ts (2)
AppType(19-19)ImagePullPolicy(123-123)
🪛 Biome (2.1.2)
libs/ui-components/src/components/form/validations.ts
[error] 641-641: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 656-656: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ 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 (21)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
244-264: LGTM! Single container app handling is well-structured.The implementation correctly:
- Uses the type guard to branch for single container apps
- Maps ports to the required string format
- Conditionally includes resource limits only when present
- Properly types the returned object
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx (1)
8-15: LGTM! Type narrowing aligns with the new app form type system.The change from a generic
ImageAppFormto the explicit unionQuadletImageAppForm | ComposeImageAppFormimproves type safety and aligns with the PR's refactoring of app form types into distinct variants.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (3)
30-39: Verify IMAGE_ONLY volume type restriction for single container apps.The code excludes
VolumeType.IMAGE_ONLYfor single container applications (line 35-37). This means single container apps can only useMOUNT_ONLYorIMAGE_MOUNTvolumes.Is this restriction intentional? If so, consider adding a comment explaining why single container apps cannot use image-only volumes:
const getVolumeTypeOptions = (t: TFunction, isSingleContainerApp: boolean) => { const validTypes: Partial<Record<VolumeType, string>> = { [VolumeType.MOUNT_ONLY]: t('Mount volume'), [VolumeType.IMAGE_MOUNT]: t('Image mount volume'), }; + // Single container apps require actual mount paths; image-only volumes are not supported if (!isSingleContainerApp) { validTypes[VolumeType.IMAGE_ONLY] = t('Image volume'); } return validTypes; };Based on learnings, this may be a deliberate design decision similar to the quadlet file extension restrictions in FlightCtl's declarative approach.
183-189: LGTM! New volume initialization correctly requires user selection.Initializing
volumeType: undefinedforces users to explicitly select a volume type, which is a good UX pattern that prevents accidental misconfiguration. Form validation will ensure a type is selected before submission.
41-200: Well-structured volume management component.The implementation demonstrates good practices:
- Proper Formik integration with
FieldArrayanduseField- Conditional field rendering based on
volumeTypeselection- Consistent
isReadOnlyprop handling throughout- Accessibility attributes and translation support
- Clean separation between UI and business logic
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx (3)
78-162: Excellent port editing state management.The implementation handles complex editing scenarios well:
- Lines 84-90: Properly adjusts
editingPortIndexwhen ports are deleted- Lines 157-162:
useEffectcleanup prevents stale editing state- Lines 127-146: Validation during edit with error preservation
This defensive approach prevents edge cases and maintains consistency.
274-307: Clear and helpful resource limits configuration.The resource limits section provides excellent user guidance:
- Detailed explanations of CPU fractional values and memory units
- Inline helper text with examples
- Emphasis on device capacity considerations
This level of documentation in the UI helps prevent misconfigurations.
191-273: Well-designed port mapping UI.The port management interface demonstrates excellent UX:
- Inline editing of existing port mappings with Label's
onEditComplete- Real-time validation feedback during input
- Keyboard support (Enter to add)
- Visual error indicators (
color={hasError ? 'red' : undefined})- Disabled state management prevents editing when errors exist
The combination of
LabelGroupfor display andSplitlayout for input creates an intuitive interface.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (4)
43-51: LGTM! Container incomplete detection properly implemented.The fix correctly addresses the previous review comment by checking for container-specific fields (
portsandvolumes) rather than just the presence of animagefield. This ensures container apps are properly initialized when switching app types.
62-76: Container app initialization is comprehensive.The initialization properly sets up all required fields for single container apps, including the container-specific
portsandvolumesarrays. The early return prevents fallthrough to other initialization logic.
122-190: Clear rendering separation for container vs. non-container apps.The conditional rendering structure is well-organized:
- Container apps get simplified UI via
ApplicationContainerForm- Non-container apps retain the Definition source selection
- Type guards (
isQuadletImageAppForm, etc.) ensure proper component renderingThis maintains backward compatibility while cleanly integrating the new container type.
306-314: Container as default app type improves new user experience.Changing the default from Quadlet/Inline to Single Container provides a simpler starting point for users. Container apps are more straightforward than Quadlet configurations, making this a good UX decision for the "Add application" button.
libs/i18n/locales/en/translation.json (1)
339-416: Translation keys comprehensively cover new container functionality.The new translation entries properly support the container-focused UI changes:
- Port mapping validation messages (lines 339-341)
- Container app configuration labels (lines 342-361)
- Volume management UI text (lines 392-416)
- Resource limit guidance and validation
All entries use clear, user-friendly language with helpful examples where appropriate (e.g., "512m", "2g", "1024k" for memory values).
Based on learnings, the codebase has automated CI checks that verify translation coverage for all text strings, ensuring completeness across all locales.
libs/ui-components/src/types/deviceSpec.ts (6)
56-59: LGTM! PortMapping type appropriately uses strings for form values.Using
stringfor port values is correct for form handling, as it allows validation to provide helpful error messages before parsing to integers. This follows common form field patterns.
61-68: Well-defined SingleContainerAppForm type.The type correctly:
- Discriminates via
appType: AppType.AppTypeContainer- Requires
namefield (unlike some other app forms where it's optional)- Includes container-specific fields (
ports,limits)- Extends
AppBasefor common fieldsThis provides strong type safety for container app handling.
70-94: Explicit app form types provide excellent type safety.The four additional app form types properly discriminate between:
- Quadlet vs. Compose (
appType)- Image vs. Inline (
specType)- Required fields per type (e.g.,
namerequired for inline apps)This explicit typing prevents invalid combinations and enables precise type guards, improving type safety throughout the codebase.
Based on learnings, the
namefield requirement for inline apps but optional for image apps is an intentional design decision enforced at both the TypeScript type level and validation level.
126-136: Type guards correctly implement discriminated union narrowing.The five type guards properly check both
appTypeandspecTypeto narrow theAppBaseunion to specific types. This enables TypeScript to provide accurate type information in conditional branches, improving IDE support and preventing type errors.
137-149: ApplicationVolumeForm optional fields enable flexible form state.The type design with optional
volumeType,imageRef, andmountPathallows for:
- Partial form state during user input
- Flexibility in form initialization
However, this requires careful validation before API submission to ensure:
volumeTypeis always defined- Required provider fields (
imageReformountPath) are present based on the selected typeThe validation concerns raised in
deviceSpecUtils.tsfortoAPIApplicationare directly related to these optional fields.
153-162: getAppIdentifier correctly handles new app form variants.The updated function properly:
- Prioritizes
namebut falls back toimagefor container and image apps- Returns
namedirectly for inline apps (where it's required)- Uses type guards to ensure correct narrowing
The logic aligns with the type constraints and provides consistent app identification.
libs/ui-components/src/components/form/validations.ts (2)
533-586: Port validation consistency issue resolved!The port validation helpers now enforce identical rules. Both
validatePortNumberandisValidPortNumberinclude thenum.toString() === portcheck (lines 551, 563), preventing values like"01"from appearing valid while silently disabling the Add button.This addresses the inconsistency flagged in the previous review.
639-661: Static analysis warnings about "then" are false positives.Biome flags lines 641 and 656 for using the
thenproperty. These are false positives—Yup's.when()method uses{ is, then, otherwise }as part of its conditional validation DSL, not as Promise-like thenables.You can safely ignore these linter warnings.
celdrake
force-pushed
the
EDM-2629-add-support-for-single-container-apps
branch
from
7540c56 to
f12db06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
221-242: Validate volumeType before mapping volumes.If
volumeTypeis undefined (which is allowed byApplicationVolumeForm), bothhasImageandhasMountwill be false, resulting in a volume with only anamefield. Theas ApplicationVolumecast at line 241 hides this type contract violation.While form validation should prevent this, adding defensive validation here provides a safety net:
const volumes = app.volumes?.map((v) => { + if (!v.volumeType) { + console.warn(`Volume "${v.name}" has no volumeType, skipping`); + return null; + } const volume: Partial<ApplicationVolume & ImageMountVolumeProviderSpec> = { name: v.name || '', }; // ... return volume as ApplicationVolume; -}); +}).filter(Boolean) as ApplicationVolume[] | undefined;Alternatively, ensure form validation prevents undefined
volumeTypefrom reaching this point.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx (1)
47-60: Port duplicate validation is asymmetric.The
validatePortfunction always usescontainerPortstate variable for the duplicate check (line 53), regardless of whether it's validating the host port or container port input. When called forcontainerPortError(line 60), it checksisDuplicatePortMapping(containerPort, containerPort, ...)which is incorrect.Consider making the duplicate check symmetric:
-const validatePort = (port: string): string | undefined => { +const validatePort = (port: string, isHostPort: boolean): string | undefined => { const error = validatePortNumber(port, t); if (error) { return error; } - // Check for duplicate if both ports match the number pattern - if (isDuplicatePortMapping(port, containerPort, ports || [])) { - return t('This port mapping already exists'); + // Only check duplicates when both ports are filled + if (hostPort && containerPort) { + const checkHost = isHostPort ? port : hostPort; + const checkContainer = isHostPort ? containerPort : port; + if (isDuplicatePortMapping(checkHost, checkContainer, ports || [])) { + return t('This port mapping already exists'); + } } return undefined; }; -const hostPortError = hostPortTouched ? validatePort(hostPort) : undefined; -const containerPortError = containerPortTouched ? validatePort(containerPort) : undefined; +const hostPortError = hostPortTouched ? validatePort(hostPort, true) : undefined; +const containerPortError = containerPortTouched ? validatePort(containerPort, false) : undefined;libs/ui-components/src/components/form/validations.ts (1)
666-671: Verify that theimagePullPolicyconditional logic was applied.A previous review comment (marked as addressed in commit 7540c56) suggested making
imagePullPolicyconditional based onvolumeType, similar toimageRef. However, the current code still showsimagePullPolicyas unconditionally required for all volumes, includingMOUNT_ONLYvolumes that don't involve images.Please confirm whether the conditional logic was applied in a different location, or if this issue still needs to be addressed.
🧹 Nitpick comments (2)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (1)
183-189: Consider setting a default volumeType when adding a new volume.New volumes are added with
volumeType: undefined, which means no fields (image ref, mount path) are shown until the user selects a type. This is acceptable UX, but you might want to provide a sensible default to reduce clicks:onClick={() => { push({ name: '', - volumeType: undefined, + volumeType: isSingleContainerApp ? VolumeType.MOUNT_ONLY : undefined, imagePullPolicy: ImagePullPolicy.PullIfNotPresent, }); }}libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (1)
18-26: Minor: Empty name results in awkward display.When an app has neither
namenorimage,getAppNamereturns an empty string, resulting in the display(Quadlet)with a leading space and empty name portion. Consider returning a default label:const getAppName = (app: AppForm, t: TFunction): string => { if (app.name) { return app.name; } if ('image' in app && app.image) { return `${t('Unnamed')} (${app.image})`; } - return ''; + return t('Unnamed'); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/i18n/locales/en/translation.json(6 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(7 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx(2 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(10 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css
- libs/i18n/locales/en/translation.json
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
📚 Learning: 2025-11-24T13:42:36.897Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
Applied to files:
libs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx
📚 Learning: 2025-10-29T16:47:29.614Z
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.
Applied to files:
libs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-10-09T12:47:50.431Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:0-0
Timestamp: 2025-10-09T12:47:50.431Z
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/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
📚 Learning: 2025-03-20T12:37:36.986Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/types/deviceSpec.ts:0-0
Timestamp: 2025-03-20T12:37:36.986Z
Learning: In the FlightCtl UI project, `name` is required for inline applications but optional for image applications. This is enforced at the type level in the `InlineAppForm` type.
Applied to files:
libs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx
📚 Learning: 2025-03-12T08:41:34.720Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewTrackedSystemdServices.tsx:3-3
Timestamp: 2025-03-12T08:41:34.720Z
Learning: In the flightctl-ui codebase, SystemdUnitsFormValues is defined as { systemdUnits: SystemdUnitFormValue[] }, allowing for direct destructuring of the systemdUnits property in component props.
Applied to files:
libs/ui-components/src/components/form/validations.ts
📚 Learning: 2025-05-26T11:41:40.796Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 300
File: libs/ui-components/src/components/form/validations.ts:459-461
Timestamp: 2025-05-26T11:41:40.796Z
Learning: In TypeScript validation contexts, when working with properly typed form data like `BatchForm`, required array fields (without `?` marker) are guaranteed to be defined arrays, making defensive checks for undefined unnecessary in those specific contexts.
Applied to files:
libs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-11-17T15:48:44.018Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx:210-213
Timestamp: 2025-11-17T15:48:44.018Z
Learning: In the flightctl-ui codebase, the UX pattern for handling disabled actions is: (1) when actions are disabled due to configuration constraints (e.g., a device belongs to a fleet and cannot be edited directly), show a disabled element with a reason; (2) when elements are hidden due to lack of user permissions (RBAC), hide the element entirely. The YAML editor follows this pattern: when canEdit is false (no permission), the Save button is completely hidden (onSave=undefined), so disabledEditReason should not be passed; when canEdit is true, disabledEditReason can be passed to disable the button for configuration reasons.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-12T09:09:44.139Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx:87-87
Timestamp: 2025-03-12T09:09:44.139Z
Learning: The `getUpdatePolicyValues` function in libs/ui-components/src/components/Fleet/CreateFleet/fleetSpecUtils.ts is designed to handle undefined input through its optional parameter definition and consistent use of optional chaining.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-03-12T09:09:44.139Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx:87-87
Timestamp: 2025-03-12T09:09:44.139Z
Learning: The `getUpdatePolicyValues` function in libs/ui-components/src/components/Fleet/CreateFleet/fleetSpecUtils.ts is designed to safely handle undefined values for the updatePolicy parameter.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-04-10T12:22:38.239Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 266
File: libs/ansible/src/const.ts:20-20
Timestamp: 2025-04-10T12:22:38.239Z
Learning: In the flightctl-ui project, `appRoutes` is defined as a `Record<ROUTE, string>` across multiple applications, requiring all applications to define entries for all route enum values, even when certain routes (like COMMAND_LINE_TOOLS) are only meant to be used in specific applications (like Standalone). For routes not applicable to a particular application, placeholder values are used with explanatory comments.
Applied to files:
libs/ui-components/src/types/deviceSpec.ts
🧬 Code graph analysis (2)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (4)
libs/types/index.ts (1)
ImagePullPolicy(123-123)libs/ui-components/src/types/deviceSpec.ts (1)
ApplicationVolumeForm(143-149)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (2)
libs/types/index.ts (1)
AppType(19-19)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
🪛 Biome (2.1.2)
libs/ui-components/src/components/form/validations.ts
[error] 659-659: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 674-674: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ 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 (22)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx (1)
101-105: LGTM!Clean improvement to show the actual configuration name in the title when available, with a sensible fallback to the numbered format.
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (2)
41-56: LGTM!Well-structured component with proper handling of volume types for single container apps. The memoized options and conditional volume type filtering based on
isSingleContainerAppis clean.
64-96: LGTM!The conditional field rendering logic based on
volumeTypeis clear and correctly determines when to show image-related fields vs mount-related fields.libs/ui-components/src/types/deviceSpec.ts (4)
48-54: LGTM!Good decision to make
specTypea required field inAppBase- this ensures all app forms have a consistent discriminator for the union type.
56-68: LGTM!The
SingleContainerAppFormtype is well-designed withnameas required (unlike other image forms where it's optional), and thePortMappingtype using strings allows for flexible validation handling in the form layer.
125-135: LGTM!The type guards are correctly implemented. Note that
isSingleContainerAppFormonly checksappTypesinceAppTypeContaineruniquely identifies single container apps (always OCI_IMAGE).
153-162: LGTM!The
getAppIdentifierfunction correctly handles all app form variants with appropriate fallbacks for display purposes.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (1)
9-16: LGTM!Clean helper function with proper fallback handling for unknown app types.
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx (2)
171-189: LGTM!Clean form layout with proper accessibility attributes and
isDisabledhandling for read-only mode.
274-307: LGTM!Good resource limit section with helpful tooltip content explaining the format and trade-offs.
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (4)
1-1: LGTM!Good choice using
fast-json-patchfor efficient patch generation instead of manual comparisons.
295-341: LGTM!The patch generation strategy is well-designed:
- Uses fast-json-patch for efficient diffing
- Correctly transforms paths to match the API's expected structure
- Falls back to simple replace strategy for unsupported operations (like
move/copy)
398-419: LGTM!The
convertVolumesToFormhelper correctly reverse-maps API volume data to form values, inferringvolumeTypebased on which fields are present.
427-451: LGTM!Single container app parsing is correct. The port format validation concern from a previous review was addressed - the API validates the format, so client-side parsing can trust the structure.
libs/ui-components/src/components/form/validations.ts (4)
533-612: LGTM! Port and resource validation utilities are well-designed.The port validation logic correctly enforces consistent rules across helpers, including the string equality check to prevent leading zeros. The CPU and memory validators appropriately handle empty/whitespace values and validate formats.
686-726: LGTM! Image and inline app validation branches are correct.The validation correctly enforces that
nameis optional for image-based apps (line 695) and required for inline apps (lines 708, 721), consistent with FlightCtl's established patterns. The use ofvalidApplicationAndVolumeNameis appropriate across all branches.Based on learnings, name requirements differ by app spec type.
657-665: Note: Biome static analysis warnings are false positives.The static analysis tool flags the
thenproperty in the Yup conditional schema objects (lines 659, 674), but these are legitimate Yup.when()configuration objects, not Promise thenables. No changes needed.Also applies to: 672-679
5-36: LGTM! Validation schema expansion is comprehensive and well-structured.The additions and refactoring support single-container apps effectively:
- New imports and type guards enable proper form classification
validApplicationAndVolumeNameconsolidates naming validation for both apps and volumes- Port and resource validation utilities are thorough
- The expanded
validApplicationsSchemacorrectly handles container, image, and inline app types with appropriate conditional logicAlso applies to: 226-230, 614-764
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (4)
14-18: LGTM! Imports and labels correctly support container apps.The updated type guard imports enable proper form classification, and the "Single Container application" label clearly distinguishes this type from Quadlet and Compose applications.
Also applies to: 29-29, 34-34
43-50: LGTM! Container state detection correctly identifies incomplete apps.The
isContainerIncompletecheck now properly detects missing container-specific fields (ports,volumes), addressing the previous issue where switching to Container type from other app types could skip initialization.
57-104: LGTM! Initialization logic properly handles app type transitions.The
useEffectcorrectly initializes type-specific fields when switching between Container, Inline, and Image app types. The dependencies are complete, and the early return optimization is appropriate.
122-190: LGTM! Conditional rendering and default app creation are well-structured.The component correctly renders type-specific UI:
- Container apps show
ApplicationContainerFormdirectly- Quadlet/Compose apps show definition source selection, name field, and the appropriate image or inline form
The default app creation (lines 307-314) appropriately initializes a Container app with
portsandvolumesarrays, consistent with validation requirements.Also applies to: 307-314
celdrake
force-pushed
the
EDM-2629-add-support-for-single-container-apps
branch
from
f12db06 to
c4d6930
Compare
celdrake
force-pushed
the
EDM-2629-add-support-for-single-container-apps
branch
from
c4d6930 to
3b89622
Compare
|
@CodeRabbit please report so we can merge |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css (1)
1-5: Border styling for added ports is acceptable given PF5 contrast constraintsThe dedicated border on
.fctl-containerapp__added-portskeeps the added-ports chips readable in PF5 light mode, and the scope of thegray !importantoverride is tightly contained. Given the prior discussion about token contrast, this looks like a reasonable trade-off.libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
215-243: Consider guarding against impossible volume shapes in volume mappingThe volume conversion logic is generally sound, but it still assumes frontend validation will always enforce a valid shape:
toAPIApplicationdefaults the API volume name tov.name || ''and will happily emit a volume with only anamefield ifvolumeType/provider fields are missing or inconsistent.convertVolumesToForminfersvolumeTypefrom the presence ofimage/mount, but if a backend volume ever arrives without either, the form will get a volume with a name, empty refs/paths, default pull policy, andvolumeTypeleft undefined.These states “should never happen” if
validApplicationsSchemais always enforced, but adding a bit of defensive code would make the mapping more robust against malformed or legacy data. For example:- const volumes = app.volumes?.map((v) => { - const volume: Partial<ApplicationVolume & ImageMountVolumeProviderSpec> = { - name: v.name || '', - }; + const volumes = app.volumes?.map((v) => { + if (!v.name) { + throw new Error('Volume name is required'); + } + const volume: Partial<ApplicationVolume & ImageMountVolumeProviderSpec> = { + name: v.name, + }; @@ - const hasImage = v.volumeType === VolumeType.IMAGE_ONLY || v.volumeType === VolumeType.IMAGE_MOUNT; - const hasMount = v.volumeType === VolumeType.MOUNT_ONLY || v.volumeType === VolumeType.IMAGE_MOUNT; + const hasImage = v.volumeType === VolumeType.IMAGE_ONLY || v.volumeType === VolumeType.IMAGE_MOUNT; + const hasMount = v.volumeType === VolumeType.MOUNT_ONLY || v.volumeType === VolumeType.IMAGE_MOUNT; + if (!hasImage && !hasMount) { + throw new Error(`Invalid volumeType for volume ${v.name}`); + }and optionally in
convertVolumesToFormskip volumes that have neitherimagenormount.Not strictly required, but would make the to/from mapping more resilient if invariants are ever violated upstream.
Also applies to: 398-419
libs/ui-components/src/components/form/validations.ts (1)
651-681: Make volumeimagePullPolicyconditional onvolumeTypeand switch Yup.whento function formRight now the volume schema always requires
imagePullPolicy, even forVolumeType.MOUNT_ONLYvolumes that don’t have an image. That’s confusing for users and stores meaningless data. Also, the object-stylewhen('volumeType', { is, then, otherwise })onimageRef/mountPathis what Biome is flagging withnoThenProperty; using the function callback form avoids that lint issue while keeping the same behavior.You can address both by:
- Keeping
imageRef/mountPathlogic as-is, but rewriting.whento the function form, and- Making
imagePullPolicyrequired only when the volume type actually uses an image (IMAGE_ONLY or IMAGE_MOUNT), while still validating the value for all volume types.For example:
- imageRef: Yup.string().when('volumeType', { - is: (val: VolumeType) => val === VolumeType.IMAGE_ONLY || val === VolumeType.IMAGE_MOUNT, - then: (schema) => - schema - .required(t('Image reference is required for this volume type')) - .matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')), - otherwise: (schema) => - schema.matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')), - }), - imagePullPolicy: Yup.string() - .oneOf( - [ImagePullPolicy.PullAlways, ImagePullPolicy.PullIfNotPresent, ImagePullPolicy.PullNever], - t('Pull policy must be one of: Always, IfNotPresent, or Never'), - ) - .required(t('Pull policy is required')), - mountPath: Yup.string().when('volumeType', { - is: (val: VolumeType) => val === VolumeType.MOUNT_ONLY || val === VolumeType.IMAGE_MOUNT, - then: (schema) => - schema - .required(t('Mount path is required for this volume type')) - .matches(absolutePathRegex, t('Mount path must be absolute.')), - otherwise: (schema) => schema.matches(absolutePathRegex, t('Mount path must be absolute.')), - }), + imageRef: Yup.string().when('volumeType', (volumeType: VolumeType | undefined, schema) => { + if (volumeType === VolumeType.IMAGE_ONLY || volumeType === VolumeType.IMAGE_MOUNT) { + return schema + .required(t('Image reference is required for this volume type')) + .matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')); + } + return schema.matches(APPLICATION_IMAGE_REGEXP, t('Image reference includes invalid characters.')); + }), + imagePullPolicy: Yup.string().when( + 'volumeType', + (volumeType: VolumeType | undefined, schema: Yup.StringSchema) => { + const policySchema = schema.oneOf( + [ImagePullPolicy.PullAlways, ImagePullPolicy.PullIfNotPresent, ImagePullPolicy.PullNever], + t('Pull policy must be one of: Always, IfNotPresent, or Never'), + ); + if (volumeType === VolumeType.IMAGE_ONLY || volumeType === VolumeType.IMAGE_MOUNT) { + return policySchema.required(t('Pull policy is required')); + } + return policySchema; + }, + ), + mountPath: Yup.string().when('volumeType', (volumeType: VolumeType | undefined, schema) => { + if (volumeType === VolumeType.MOUNT_ONLY || volumeType === VolumeType.IMAGE_MOUNT) { + return schema + .required(t('Mount path is required for this volume type')) + .matches(absolutePathRegex, t('Mount path must be absolute.')); + } + return schema.matches(absolutePathRegex, t('Mount path must be absolute.')); + }),This keeps validation semantics, makes
imagePullPolicymeaningful, and removes thethenproperty that Biome complains about.
🧹 Nitpick comments (2)
libs/ui-components/src/types/deviceSpec.ts (2)
56-59: Consider using numeric types for port numbers.Port numbers are defined as strings, but port values are inherently numeric (valid range: 1–65535). While string types are common for form inputs, consider whether the form layer should convert to numbers or if validation ensures these are numeric strings.
If numeric types are preferred:
export type PortMapping = { - hostPort: string; - containerPort: string; + hostPort: number; + containerPort: number; };Alternatively, if strings are intentional for form binding, ensure validation enforces numeric-only values before API submission.
137-149: Consider discriminated union for stronger type safety.The
ApplicationVolumeFormallows all fields to be optional regardless ofvolumeType, but logically:
IMAGE_ONLYneedsimageRef/imagePullPolicyMOUNT_ONLYneedsmountPathIMAGE_MOUNTneeds all threeIf form validation handles these constraints, the current design is acceptable. Otherwise, a discriminated union could provide compile-time safety.
export type ApplicationVolumeForm = | { name: string; volumeType: VolumeType.IMAGE_ONLY; imageRef: string; imagePullPolicy?: ImagePullPolicy } | { name: string; volumeType: VolumeType.MOUNT_ONLY; mountPath: string } | { name: string; volumeType: VolumeType.IMAGE_MOUNT; imageRef: string; imagePullPolicy?: ImagePullPolicy; mountPath: string };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/i18n/locales/en/translation.json(6 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(7 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.css(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx(2 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(10 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ConfigurationTemplates.tsx
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationImageForm.tsx
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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.
📚 Learning: 2025-11-17T15:48:44.018Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx:210-213
Timestamp: 2025-11-17T15:48:44.018Z
Learning: In the flightctl-ui codebase, the UX pattern for handling disabled actions is: (1) when actions are disabled due to configuration constraints (e.g., a device belongs to a fleet and cannot be edited directly), show a disabled element with a reason; (2) when elements are hidden due to lack of user permissions (RBAC), hide the element entirely. The YAML editor follows this pattern: when canEdit is false (no permission), the Save button is completely hidden (onSave=undefined), so disabledEditReason should not be passed; when canEdit is true, disabledEditReason can be passed to disable the button for configuration reasons.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx
📚 Learning: 2025-10-29T16:47:29.614Z
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.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-03-20T12:37:36.986Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/types/deviceSpec.ts:0-0
Timestamp: 2025-03-20T12:37:36.986Z
Learning: In the FlightCtl UI project, `name` is required for inline applications but optional for image applications. This is enforced at the type level in the `InlineAppForm` type.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.tslibs/i18n/locales/en/translation.json
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/form/validations.ts
📚 Learning: 2025-10-09T12:47:50.431Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 366
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:0-0
Timestamp: 2025-10-09T12:47:50.431Z
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/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-11-24T13:42:36.897Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 381
File: libs/ui-components/src/components/form/validations.ts:71-88
Timestamp: 2025-11-24T13:42:36.897Z
Learning: In FlightCtl, quadlet applications intentionally support only a subset of Podman's quadlet file extensions (.container, .volume, .network, .image, .pod) because other valid Podman extensions (.build, .artifact, .kube) don't work well with FlightCtl's declarative approach. This is a deliberate design decision reflected in supportedQuadletExtensions and unsupportedQuadletExtensions constants in libs/ui-components/src/components/form/validations.ts.
Applied to files:
libs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-04-10T12:22:38.239Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 266
File: libs/ansible/src/const.ts:20-20
Timestamp: 2025-04-10T12:22:38.239Z
Learning: In the flightctl-ui project, `appRoutes` is defined as a `Record<ROUTE, string>` across multiple applications, requiring all applications to define entries for all route enum values, even when certain routes (like COMMAND_LINE_TOOLS) are only meant to be used in specific applications (like Standalone). For routes not applicable to a particular application, placeholder values are used with explanatory comments.
Applied to files:
libs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-12T08:41:34.720Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewTrackedSystemdServices.tsx:3-3
Timestamp: 2025-03-12T08:41:34.720Z
Learning: In the flightctl-ui codebase, SystemdUnitsFormValues is defined as { systemdUnits: SystemdUnitFormValue[] }, allowing for direct destructuring of the systemdUnits property in component props.
Applied to files:
libs/ui-components/src/components/form/validations.ts
📚 Learning: 2025-05-26T11:41:40.796Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 300
File: libs/ui-components/src/components/form/validations.ts:459-461
Timestamp: 2025-05-26T11:41:40.796Z
Learning: In TypeScript validation contexts, when working with properly typed form data like `BatchForm`, required array fields (without `?` marker) are guaranteed to be defined arrays, making defensive checks for undefined unnecessary in those specific contexts.
Applied to files:
libs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-03-12T09:09:44.139Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx:87-87
Timestamp: 2025-03-12T09:09:44.139Z
Learning: The `getUpdatePolicyValues` function in libs/ui-components/src/components/Fleet/CreateFleet/fleetSpecUtils.ts is designed to safely handle undefined values for the updatePolicy parameter.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-03-12T09:09:44.139Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 227
File: libs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsx:87-87
Timestamp: 2025-03-12T09:09:44.139Z
Learning: The `getUpdatePolicyValues` function in libs/ui-components/src/components/Fleet/CreateFleet/fleetSpecUtils.ts is designed to handle undefined input through its optional parameter definition and consistent use of optional chaining.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-03-19T08:57:16.233Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:57:16.233Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by both frontend validation logic and backend API constraints, ensuring that no inline application can exist in the system without a name field.
Applied to files:
libs/i18n/locales/en/translation.json
🧬 Code graph analysis (1)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (2)
libs/types/index.ts (1)
AppType(19-19)libs/ui-components/src/types/deviceSpec.ts (2)
AppForm(113-118)getAppIdentifier(153-162)
🪛 Biome (2.1.2)
libs/ui-components/src/components/form/validations.ts
[error] 659-659: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 674-674: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ 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: Lint
- GitHub Check: Build ocp plugin
- GitHub Check: Build
🔇 Additional comments (11)
libs/i18n/locales/en/translation.json (1)
336-413: New Single Container, port, resource, and volume translations are consistent with validation logicThe added strings for port mappings, CPU/memory limits, Single Container labels, volume fields, and name/port validation all line up with the new form/validation behavior (regexes, ranges, and helper text). I don’t see any missing or conflicting keys for the new UI flows.
Also applies to: 482-486, 707-749
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (1)
7-26: Name/format helpers make application review more robust and readableUsing
getAppNameandgetAppFormatLabelcentralizes the fallback logic (e.g., “Unnamed (image)”) and ensures consistent labels for Container/Quadlet/Compose/Unknown. The render now handles missing names gracefully while keeping keys stable viagetAppIdentifier.Also applies to: 35-43
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx (1)
17-22: Volume form structure and read-only handling look solidThe FieldArray-based volume editor, conditional sections for image vs. mount volumes, and
isReadOnlygating are all wired correctly. Combined with the schema fixes invalidApplicationsSchema, this should give a clear and consistent UX for configuring volumes.Also applies to: 24-39, 41-199
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
39-51: Container-aware initialization and switching logic for applications looks consistentThe updated
ApplicationSectioncorrectly distinguishes container vs. non-container apps, initializes Single Container apps with the required fields (image, ports, volumes), and resets shapes appropriately when switching between app types/spec types while preserving the name. The default “Add application” now creating a container+OCI image app also matches the new Single Container flow.Also applies to: 57-105, 122-190, 298-314
libs/ui-components/src/types/deviceSpec.ts (7)
48-54: LGTM!The
AppBasetype correctly includesspecTypefor discriminating between image and inline application forms. The optionalnameat this level is appropriate since specific subtypes (inline variants) override it to be required.
61-68: LGTM!The
SingleContainerAppFormcorrectly requiresname(for container identification) andimage, with optionalportsand resourcelimits. The fixedappTypeandspecTypeensure proper discrimination.
70-81: LGTM!The Quadlet form types correctly enforce the naming constraint:
nameremains optional for image-based apps (inherited fromAppBase) but is required for inline apps. This aligns with the project's established pattern.
83-94: LGTM!Compose form types follow the same pattern as Quadlet forms, maintaining consistency across application types.
113-118: LGTM!The
AppFormunion exhaustively covers all five application form variants, providing a complete discriminated union.
125-135: LGTM!Type guards are correctly implemented. The
isSingleContainerAppFormguard only checksappTypesinceAppTypeContaineris unique to this form, while Quadlet/Compose guards require bothappTypeandspecTypefor discrimination.
153-162: LGTM!The
getAppIdentifierlogic correctly handles all form types:
- Single container and image-based forms use
name || image- Inline forms rely on the required
namefieldThe explicit handling of
SingleContainerAppFormseparately from other image forms improves readability and accommodates potential future divergence.
|
@celdrake, here's a summary of PR #382: OverviewThis PR successfully adds comprehensive support for Single Container applications to the UI, introducing a new application type alongside the existing Quadlet and Compose types. Key AdditionsType System & Architecture
UI Components
Validation & Error Handling
Patch Generation
Code Quality
The changes are well-structured, maintain backward compatibility with existing app types, and follow established patterns in the codebase. The PR is ready to merge. 🧠 Learnings used✅ Actions performedComments resolved. |
2 participants
Adds support for Single Container applications.
Rebased on top of #381
single-container-apps-ui1.mp4
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.