EDM-2290 quadlet applications support#381
Conversation
WalkthroughAdds explicit AppType (Quadlet, Compose) and Definition Source (OCI_IMAGE, INLINE) distinctions across types, schemas, UI, and translations; expands volume model to support image/mount combinations; updates validation, form types, UI flows, and device/application status to carry appType and embedded metadata. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as ApplicationTemplates
participant Val as validations.ts
participant Mapper as deviceSpecUtils
participant API
User->>UI: Choose AppType (Quadlet / Compose / Image-based)
UI->>UI: set appType
UI->>UI: show Definition source (OCI_IMAGE or INLINE)
User->>UI: Fill fields (image OR files) + volumes
User->>UI: Submit
UI->>Val: Validate payload (uses appType + specType)
alt specType == OCI_IMAGE
Val->>Val: validate image, image-volume shapes
else specType == INLINE
Val->>Val: validate files (names, extensions, root)
end
Val-->>UI: validation result
UI->>Mapper: toAPIApplication(payload with appType)
Mapper->>Mapper: map volumes to ApplicationVolume (image/mount combos)
Mapper-->>API: ApplicationProviderSpec (includes appType)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
209-259: Changes toappTypeandvolumesare not reflected ingetApplicationPatches
toAPIApplicationandgetApplicationValuescorrectly introduceappTypeandvolumesinto the API shape, butgetApplicationPatchesstill only compares:
- spec type (inline vs image),
name,envVars, andimage/ inlinefiles.It never checks
appTypeorvolumes. As a result:
- Toggling the “Application format” (e.g. Compose → Quadlet) for an existing app without changing name/image/envVars/files will not produce a patch, so the backend never sees the new
appType.- Editing volumes for an existing app (adding/removing/changing image/mount) likewise won’t trigger a patch, so volume changes are silently dropped.
Given this PR’s goal is to introduce quadlet support and richer volume handling, this is a functional blocker.
A minimal fix is to include
appTypeandvolumesin the change detection insidegetApplicationPatches’sneedsPatchcallback. One way to do this without rewriting the whole function:} else { - const needsPatch = currentApps.some((currentApp, index) => { - const updatedApp = updatedApps[index]; + const needsPatch = currentApps.some((currentApp, index) => { + const updatedApp = updatedApps[index]; + const updatedApiApp = toAPIApplication(updatedApp); const isCurrentImageApp = isImageAppProvider(currentApp); const currentAppSpecType = isCurrentImageApp ? AppSpecType.OCI_IMAGE : AppSpecType.INLINE; if (currentAppSpecType !== updatedApp.specType || updatedApp.name !== currentApp.name) { return true; } const currentVars = Object.entries(currentApp.envVars || {}); if (currentVars.length !== updatedApp.variables.length) { return true; } else { const hasChangedVars = updatedApp.variables.some((variable) => { const currentValue = currentApp.envVars ? currentApp.envVars[variable.name] : undefined; return !currentValue || currentValue !== variable.value; }); if (hasChangedVars) { return true; } } + + // New: detect changes in appType (compose vs quadlet, etc.) + if ((currentApp.appType || undefined) !== (updatedApiApp.appType || undefined)) { + return true; + } + + // New: detect changes in volumes + const currentVolumes = currentApp.volumes || []; + const updatedVolumes = updatedApiApp.volumes || []; + if (currentVolumes.length !== updatedVolumes.length) { + return true; + } + const haveVolumeChanges = currentVolumes.some((currentVolume, volIndex) => { + const updatedVolume = updatedVolumes[volIndex]; + // Shallow/deep compare via JSON is acceptable here given small payload sizes + return JSON.stringify(currentVolume) !== JSON.stringify(updatedVolume); + }); + if (haveVolumeChanges) { + return true; + } if (isCurrentImageApp) { - return (updatedApp as ImageAppForm).image !== currentApp.image; + return updatedApiApp.image !== currentApp.image; } return hasInlineApplicationChanged(currentApp, updatedApp as InlineAppForm); });This keeps your existing envVars/image/inline-diff logic but ensures the new
appTypeandvolumesfields participate in change detection so user edits are actually persisted.Also applies to: 276-340, 397-421
🧹 Nitpick comments (3)
libs/ui-components/src/components/form/validations.ts (1)
5-6: Inline app validation split by image/compose/quadlet is solid; consider a couple of small refinementsThe new validation structure for applications is well factored:
inlineAppNameSchema,inlineAppFilePathSchema,inlineAppFileSchema, anduniqueFilePathsTestgive you a clean, shared baseline for inline apps.- Quadlet-specific checks (
supportedQuadletExtensions/unsupportedQuadletExtensions,quadletFileTypesValidation,quadletFilesAtRoot) nicely encode the quadlet rules.validApplicationsSchemabranching onisImageAppForm→isQuadletAppForm→ compose keeps image apps out of inline-specific rules while still enforcing that quadlet inlines haveappType = AppTypeQuadlet.Two optional improvements you might want to consider:
- Explicitly require
appType=composefor compose inline appsRight now the compose branch doesn’t validate
appTypeat all, even thoughComposeAppForm’s type guaranteesappType: AppTypeComposeand the UI always sets it. Adding an explicit check would catch any inconsistent data and align the schema with the types:- // Inline compose applications - return Yup.object<ComposeAppForm>().shape({ - specType: appSpecTypeSchema(t), + // Inline compose applications + return Yup.object<ComposeAppForm>().shape({ + appType: Yup.string().oneOf([AppType.AppTypeCompose]).required(t('Application type is required')), + specType: appSpecTypeSchema(t), name: inlineAppNameSchema(t, 'compose'), files: inlineAppFileSchema(t) .test('unique-file-paths', uniqueFilePathsTest(t)) .test('compose-file-name', composeFileName(t)), variables: appVariablesSchema(t), });
- Compose filenames: optionally include common
*.override.ymlvariants
validComposeFileNamesandvalidComposeFileNameDisplaycurrently accept.override.yamlbut not.override.yml. If you intend to accept both common docker/podman compose override filename variants, you could extend the allowed list:const validComposeFileNames = [ 'podman-compose.yaml', 'podman-compose.yml', - 'podman-compose.override.yaml', + 'podman-compose.override.yaml', + 'podman-compose.override.yml', 'docker-compose.yaml', 'docker-compose.yml', - 'docker-compose.override.yaml', + 'docker-compose.override.yaml', + 'docker-compose.override.yml', ]; const validComposeFileNameDisplay = [ '(podman|docker)-compose.yaml', '(podman|docker)-compose.yml', - '(podman|docker)-compose.override.yaml', + '(podman|docker)-compose.override.yaml', + '(podman|docker)-compose.override.yml', ].join(', ');If
.override.ymlis intentionally unsupported for your use case, the current behavior is fine; otherwise, this small tweak makes the rule match common compose conventions more closely.Overall, the quadlet and compose validation additions look robust and well-aligned with the new app types.
Also applies to: 12-13, 20-21, 28-32, 71-88, 330-397, 398-427, 438-527, 529-610
libs/ui-components/src/types/deviceSpec.ts (2)
49-55: AppBase extension keeps inline/image name contract while centralizing volumesAdding
appType?: AppTypeandvolumes?: ApplicationVolumeForm[]at the base works well with the new Compose/Quadlet split, and thename-required-for-inline / optional-for-image rule is still enforced by the derived form types rather than the base.You could add a brief clarifying comment that
appTypeis only meaningful forspecType === AppSpecType.INLINEto avoid confusion for future contributors, e.g.:type AppBase = { specType: AppSpecType; - appType?: AppType; + // For INLINE apps, specifies the concrete inline type (compose, quadlet, ...) + appType?: AppType; name?: string; variables: { name: string; value: string }[]; volumes?: ApplicationVolumeForm[]; };Based on learnings.
100-103: TightenisQuadletAppFormguard withspecTypecheckRight now the guard only checks
appType, so a malformed object withappType = AppType.AppTypeQuadletbut a non-inlinespecTypewould still pass. You can make the predicate more robust (and self-documenting) by also asserting that it’s an inline app:-export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => app.appType === AppType.AppTypeQuadlet; +export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => + isInlineAppForm(app) && app.appType === AppType.AppTypeQuadlet;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
libs/i18n/locales/en/translation.json(3 hunks)libs/types/index.ts(4 hunks)libs/types/models/AppType.ts(1 hunks)libs/types/models/ApplicationPort.ts(1 hunks)libs/types/models/ApplicationResourceLimits.ts(1 hunks)libs/types/models/ApplicationResources.ts(1 hunks)libs/types/models/ApplicationVolume.ts(1 hunks)libs/types/models/AuthDynamicRoleAssignment.ts(1 hunks)libs/types/models/AuthProviderSpec.ts(1 hunks)libs/types/models/ImageApplicationProviderSpec.ts(1 hunks)libs/types/models/ImageMountVolumeProviderSpec.ts(1 hunks)libs/types/models/K8sProviderSpec.ts(0 hunks)libs/types/models/MountVolumeProviderSpec.ts(1 hunks)libs/types/models/OpenShiftProviderSpec.ts(1 hunks)libs/types/models/TokenRequest.ts(1 hunks)libs/types/models/TokenResponse.ts(1 hunks)libs/types/models/UserInfoResponse.ts(1 hunks)libs/types/models/VolumeMount.ts(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(1 hunks)libs/ui-components/src/components/form/validations.ts(3 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
💤 Files with no reviewable changes (1)
- libs/types/models/K8sProviderSpec.ts
🧰 Additional context used
🧠 Learnings (16)
📓 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-02-17T08:41:57.993Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-02-17T08:41:57.993Z
Learning: All files in `libs/types/models` are auto-generated using OpenAPI Generator from the schema defined in https://github.com/flightctl/flightctl/blob/main/api/v1alpha1/openapi.yaml. Changes to these files should be made in the schema instead of modifying the generated files directly.
Applied to files:
libs/types/models/ApplicationResourceLimits.tslibs/types/models/OpenShiftProviderSpec.tslibs/types/models/MountVolumeProviderSpec.tslibs/types/models/TokenResponse.tslibs/ui-components/src/types/deviceSpec.tslibs/types/index.ts
📚 Learning: 2025-10-10T11:35:04.458Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-10-10T11:35:04.458Z
Learning: In the flightctl-ui repository, TypeScript types are generated using hey-api/openapi-ts and output to a single index.ts file (previously used openapi-typescript-codegen with one file per type under /libs/types/models).
Applied to files:
libs/types/models/ApplicationResourceLimits.tslibs/types/models/TokenRequest.tslibs/types/models/OpenShiftProviderSpec.tslibs/types/models/TokenResponse.tslibs/ui-components/src/types/deviceSpec.tslibs/types/index.ts
📚 Learning: 2025-11-20T07:33:34.656Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/utils/authProvider.ts:5-18
Timestamp: 2025-11-20T07:33:34.656Z
Learning: In the flightctl-ui repository, for AuthProvider objects, the metadata.name field is always present at runtime as it serves as the ID of the authprovider. The OpenAPI schema incorrectly defines it as optional (name?: string in ObjectMeta), but the type assertion `as string` is appropriate when accessing provider.metadata.name since it's guaranteed to exist.
Applied to files:
libs/types/models/OpenShiftProviderSpec.tslibs/types/models/AuthProviderSpec.ts
📚 Learning: 2025-11-20T09:24:41.950Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Applied to files:
libs/types/models/OpenShiftProviderSpec.tslibs/types/models/AuthProviderSpec.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/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.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/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.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/types/models/AppType.tslibs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.ts
📚 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/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/form/validations.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.tslibs/ui-components/src/types/deviceSpec.ts
📚 Learning: 2025-03-20T12:34:33.199Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/types/models/FileSpec.ts:8-8
Timestamp: 2025-03-20T12:34:33.199Z
Learning: Files in the types directory are autogenerated from the OpenAPI spec in the flightctl project and should not be directly modified.
Applied to files:
libs/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/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 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
🧬 Code graph analysis (18)
libs/types/models/ApplicationResourceLimits.ts (1)
libs/types/index.ts (1)
ApplicationResourceLimits(12-12)
libs/types/models/ApplicationVolume.ts (3)
libs/types/models/ImageVolumeProviderSpec.ts (1)
ImageVolumeProviderSpec(6-8)libs/types/models/MountVolumeProviderSpec.ts (1)
MountVolumeProviderSpec(9-11)libs/types/models/ImageMountVolumeProviderSpec.ts (1)
ImageMountVolumeProviderSpec(10-13)
libs/types/models/TokenRequest.ts (1)
libs/types/index.ts (1)
TokenRequest(184-184)
libs/types/models/OpenShiftProviderSpec.ts (1)
libs/types/index.ts (1)
OpenShiftProviderSpec(143-143)
libs/types/models/ImageApplicationProviderSpec.ts (3)
libs/types/models/ApplicationVolumeProviderSpec.ts (1)
ApplicationVolumeProviderSpec(6-11)libs/types/models/ApplicationPort.ts (1)
ApplicationPort(8-8)libs/types/models/ApplicationResources.ts (1)
ApplicationResources(9-11)
libs/types/models/ApplicationResources.ts (2)
libs/types/index.ts (2)
ApplicationResources(13-13)ApplicationResourceLimits(12-12)libs/types/models/ApplicationResourceLimits.ts (1)
ApplicationResourceLimits(8-17)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (3)
libs/types/index.ts (1)
AppType(19-19)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/types/deviceSpec.ts (1)
AppForm(93-93)
libs/types/models/VolumeMount.ts (1)
libs/types/index.ts (1)
VolumeMount(189-189)
libs/types/models/ImageMountVolumeProviderSpec.ts (2)
libs/types/index.ts (3)
ImageMountVolumeProviderSpec(122-122)ImageVolumeSource(125-125)VolumeMount(189-189)libs/types/models/VolumeMount.ts (1)
VolumeMount(8-13)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (3)
libs/ui-components/src/types/deviceSpec.ts (2)
AppForm(93-93)isImageAppForm(100-100)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/types/index.ts (1)
AppType(19-19)
libs/types/models/AuthProviderSpec.ts (1)
libs/types/models/OpenShiftProviderSpec.ts (1)
OpenShiftProviderSpec(8-49)
libs/types/models/UserInfoResponse.ts (1)
libs/types/index.ts (2)
UserInfoResponse(187-187)Organization(144-144)
libs/types/models/MountVolumeProviderSpec.ts (2)
libs/types/index.ts (2)
MountVolumeProviderSpec(138-138)VolumeMount(189-189)libs/types/models/VolumeMount.ts (1)
VolumeMount(8-13)
libs/types/models/TokenResponse.ts (1)
libs/types/index.ts (1)
TokenResponse(185-185)
libs/types/models/ApplicationPort.ts (1)
libs/types/index.ts (1)
ApplicationPort(10-10)
libs/ui-components/src/components/form/validations.ts (1)
libs/ui-components/src/types/deviceSpec.ts (7)
InlineAppForm(61-61)ComposeAppForm(63-67)QuadletAppForm(70-74)AppForm(93-93)isImageAppForm(100-100)ImageAppForm(57-59)isQuadletAppForm(102-102)
libs/ui-components/src/types/deviceSpec.ts (1)
libs/types/index.ts (4)
AppType(19-19)ApplicationVolume(16-16)ImageVolumeSource(125-125)VolumeMount(189-189)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (5)
libs/types/index.ts (5)
ApplicationVolume(16-16)ImageMountVolumeProviderSpec(122-122)ApplicationProviderSpec(11-11)AppType(19-19)InlineApplicationProviderSpec(126-126)libs/types/models/ApplicationVolume.ts (1)
ApplicationVolume(8-13)libs/ui-components/src/types/deviceSpec.ts (2)
isImageAppForm(100-100)InlineAppForm(61-61)libs/types/models/ApplicationProviderSpec.ts (1)
ApplicationProviderSpec(9-15)libs/types/models/InlineApplicationProviderSpec.ts (1)
InlineApplicationProviderSpec(7-12)
⏰ 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 (24)
libs/types/models/AppType.ts (1)
8-12: AppType enum extension is consistent with planned container supportAdding
AppTypeContainer = 'container'alongsidecomposeandquadletis coherent with the app-type taxonomy and matches the PR’s aim of laying groundwork for single-container apps. Since this file is generated, the key is that the upstream OpenAPI schema continues to reflect the intended set of app types. Based on learningslibs/types/models/ApplicationPort.ts (1)
1-8: String alias for port mapping is clear for generated typesModeling
ApplicationPortas astringwith the documented"hostPort:containerPort"convention is reasonable for generated types; stricter structure/validation can be enforced in the OpenAPI schema or UI validation if needed.libs/types/models/AuthDynamicRoleAssignment.ts (1)
17-20: Separator field addition matches documented org-scoped role behaviorThe new optional
separator?: stringcleanly documents theorg:roleconvention and keeps the default (':') as a runtime concern, which is appropriate for a generated type. Just ensure any consumers treatundefinedas “use the default separator” rather than a distinct mode.libs/types/models/UserInfoResponse.ts (1)
1-31: UserInfoResponse shape aligns with typical OIDC user info plus orgsThe
UserInfoResponsetype mirrors standard OIDC fields and cleanly extends them withorganizations?: Array<Organization>and an optionalerrorcode, which fits the intended usage while keeping everything optional for robustness against provider differences.libs/types/models/VolumeMount.ts (1)
1-14: VolumeMount is a concise, reusable mount descriptorDefining
VolumeMountwith a single requiredpath: stringprovides a simple, reusable mount descriptor for volume providers; it’s appropriate for generated typings, with any additional semantics captured in the API schema and docs.libs/types/models/MountVolumeProviderSpec.ts (1)
1-11: MountVolumeProviderSpec correctly composes the shared VolumeMount typeThe
MountVolumeProviderSpecwrapper aroundmount: VolumeMountis consistent with the other provider-spec patterns and cleanly reuses the shared mount descriptor.libs/types/models/TokenResponse.ts (1)
1-41: TokenResponse model matches typical OAuth2 token response semanticsThe
TokenResponsefields and the nestedtoken_typeenum (BEARER = 'Bearer') align with standard OAuth2 expectations, and marking all fields optional makes the client resilient to partial or error responses.libs/types/models/TokenRequest.ts (1)
1-47: TokenRequest definition cleanly models refresh and authorization_code grantsThe
TokenRequestpayload and nestedgrant_typeenum forrefresh_tokenandauthorization_codemap well onto standard OAuth2 token endpoint usage, with requiredgrant_type/client_idand other fields optional/nullable as appropriate for each flow.libs/types/models/OpenShiftProviderSpec.ts (1)
1-49: LGTM! Auto-generated type definition.This file is auto-generated from the OpenAPI schema. The OpenShiftProviderSpec type structure is appropriate for an OAuth2 provider configuration with OpenShift-specific fields.
Based on learnings
libs/i18n/locales/en/translation.json (2)
226-232: LGTM! Translation keys for application format support.The addition of "Compose" and "Quadlet" format labels and related translation keys appropriately supports the PR's objective to add Quadlet application support.
549-559: LGTM! Validation messages updated for multi-format support.The changes appropriately:
- Parameterize the name requirement message to work for both Compose and Quadlet
- Generalize the file requirement message
- Add Quadlet-specific validation messages for file name restrictions, supported types, and directory structure requirements
libs/types/models/ApplicationVolume.ts (1)
1-14: LGTM! Extended volume provider specifications.This auto-generated file now supports three volume provider types (image-only, mount-only, and combined image+mount), providing the flexibility needed for different application volume configurations.
Based on learnings
libs/types/models/ImageApplicationProviderSpec.ts (1)
1-19: LGTM! Extended image application specification.This auto-generated file now includes optional ports and resources fields, enabling port mappings and resource limit configurations for image-based applications.
Based on learnings
libs/types/models/ApplicationResourceLimits.ts (1)
1-18: LGTM! Resource limits type definition.This auto-generated file defines CPU and memory limits using string types with appropriate format descriptions (cores for CPU, Podman format for memory). The string type choice is appropriate for these fields given the need to support various formats and units.
Based on learnings
libs/types/models/AuthProviderSpec.ts (1)
1-11: LGTM! Extended auth provider union.This auto-generated file now includes OpenShiftProviderSpec in the AuthProviderSpec union, enabling OpenShift as a supported authentication provider type.
Based on learnings
libs/types/models/ImageMountVolumeProviderSpec.ts (1)
1-14: LGTM! Combined image and mount volume provider.This auto-generated file defines a volume provider spec that combines both an OCI image source and a mount path, completing the set of volume provider types (image-only, mount-only, and combined).
Based on learnings
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (2)
14-19: LGTM! Helper function for application format labels.The
getAppFormatLabelhelper appropriately maps the AppType to user-facing format labels, with Quadlet explicitly mapped and Compose as the default fallback.
24-33: LGTM! Updated app display logic for multi-format support.The changes appropriately:
- Switch from
isInlineAppFormtoisImageAppFormlogic (inverted condition)- Compose type labels as "specType - formatType" (e.g., "Image based - Quadlet", "Inline - Compose")
- Handle name display with correct fallback to "Unnamed (image)" for unnamed image-based apps
The logic correctly handles all app type combinations introduced by the Quadlet support.
libs/types/models/ApplicationResources.ts (1)
1-11: ApplicationResources wrapper type looks correctThe generated
ApplicationResourcestype cleanly wrapsApplicationResourceLimitswith an optionallimitsfield and is consistent with the existing limits model. Nothing to change here.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
7-107: App format handling and spec-type reset logic are coherentThe new
appSpecTypeOptions/appFormatOptionshelpers,isInlineIncomplete/shouldResetAppflags, and theuseEffectthat seeds required fields whenspecTypechanges are all consistent and side‑effect safe. DefaultingappTypetoAppTypeComposeon transitions keeps the new “Application format” select well-behaved for both inline and image apps.libs/types/index.ts (1)
10-13: New model exports are consistent with generated modelsRe-exporting
ApplicationPort,ApplicationResourceLimits/ApplicationResources, volume provider specs, OpenShift auth specs, token types,UserInfoResponse, andVolumeMountfrom the generated index is consistent with the correspondingmodels/*.tsfiles and cleanly extends the public typings surface.Also applies to: 122-123, 138-139, 143-143, 184-189
libs/ui-components/src/types/deviceSpec.ts (3)
1-14: Imports align with new app/volume modelPulling
ApplicationVolume,AppType,ImageVolumeSource, andVolumeMountfrom@flightctl/typeskeeps this form layer tightly coupled to the generated API model and matches the new backend surface. No changes needed here.
57-74: Compose/Quadlet form split cleanly preserves inline name requirementsDefining
ComposeAppFormandQuadletAppFormas extensions ofAppBasewithappTypediscriminants and requiredname/files, then exposingInlineAppForm = ComposeAppForm | QuadletAppForm, nicely preserves the “inline apps must have name” invariant while adding a type-safe Quadlet dimension. This aligns with the earlier preparatoryAppTypeQuadletwork and the inline-vs-image naming rules.Based on learnings.
104-107: ApplicationVolumeForm overlay matches evolving backend volume typesWrapping
ApplicationVolumewith form-onlyimage?: ImageVolumeSourceandmount?: VolumeMountgives the UI a convenient, strongly-typed place to collect image-backed and mount configuration while staying aligned with the generated OpenAPI types and the planned backend volume support.Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-components/src/components/form/validations.ts (1)
529-616: Consider explicit validation of Compose type in fallback branch.The validation correctly discriminates between Image (line 533), Quadlet (line 556), and Compose (fallback at line 570). However, the Compose branch relies on fallthrough logic. Consider adding an explicit
appTypecheck forAppType.AppTypeComposein the final branch to catch invalid app types early and provide clearer error messages if an unsupported type is encountered.Apply this diff to add explicit Compose validation:
// Inline compose applications - return Yup.object<ComposeAppForm>().shape({ + // Explicitly validate it's a compose app to catch invalid types + if (value.appType !== AppType.AppTypeCompose) { + return Yup.object().shape({ + appType: Yup.string().oneOf([AppType.AppTypeCompose]).required(t('Application type is required')), + }); + } + return Yup.object<ComposeAppForm>().shape({ specType: appSpecTypeSchema(t), appType: Yup.string().oneOf([AppType.AppTypeCompose]).required(t('Application type is required')),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/i18n/locales/en/translation.json(4 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/form/validations.ts(3 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🧰 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-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: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/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.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/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.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/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.tslibs/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/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/form/validations.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-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
📚 Learning: 2025-10-10T11:35:04.458Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-10-10T11:35:04.458Z
Learning: In the flightctl-ui repository, TypeScript types are generated using hey-api/openapi-ts and output to a single index.ts file (previously used openapi-typescript-codegen with one file per type under /libs/types/models).
Applied to files:
libs/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/types/deviceSpec.ts
📚 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
🧬 Code graph analysis (4)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (4)
libs/types/models/ApplicationVolume.ts (1)
ApplicationVolume(8-13)libs/ui-components/src/types/deviceSpec.ts (3)
isImageAppForm(101-101)isImageAppProvider(98-99)InlineAppForm(62-62)libs/types/models/ApplicationProviderSpec.ts (1)
ApplicationProviderSpec(9-15)libs/types/models/InlineApplicationProviderSpec.ts (1)
InlineApplicationProviderSpec(7-12)
libs/ui-components/src/components/form/validations.ts (1)
libs/ui-components/src/types/deviceSpec.ts (7)
InlineAppForm(62-62)ComposeAppForm(64-68)QuadletAppForm(71-75)AppForm(94-94)isImageAppForm(101-101)ImageAppForm(57-60)isQuadletAppForm(103-103)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/types/deviceSpec.ts (1)
AppForm(94-94)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
libs/ui-components/src/types/deviceSpec.ts (1)
libs/types/index.ts (4)
AppType(19-19)ApplicationVolume(16-16)ImageVolumeSource(125-125)VolumeMount(189-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Build ocp plugin
🔇 Additional comments (13)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (4)
29-32: LGTM!The
appFormatOptionshelper cleanly maps application types to translated labels for UI rendering.
94-137: LGTM!The new "Definition source" UI provides clear guidance on the difference between image-based and inline applications. The helper text and radio controls are well-structured.
268-274: Default application correctly initializes Quadlet inline type.The default values set a new application to Quadlet with inline definition source. Note that this creates an "incomplete" app (missing the
filesarray), which will trigger theshouldResetApplogic in theuseEffectat lines 47-76 to add the required fields. This is by design but creates an indirect dependency between these two code sections.
38-76: No issues found - reset logic is intentional and correct.The
shouldResetApplogic is working as designed. New applications intentionally start in an incomplete state (INLINE withoutfilesfield), which triggers the effect to initialize them properly. When users switch between "Inline" and "Image reference" modes, the reset prevents invalid intermediate states by reconstructing the form with the required fields for the new type. The dependency array is correct, and Formik does not remove fields during validation, so there are no edge case concerns.libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
214-228: Volume mapping correctly handles new union types.The updated volume mapping logic properly handles
ApplicationVolumevariants (image-only, mount-only, or both viaImageMountVolumeProviderSpec). The type assertion is necessary here since the dynamic field assignment prevents TypeScript from inferring the correct discriminated union member.libs/ui-components/src/types/deviceSpec.ts (3)
57-75: LGTM! Well-structured discriminated unions.The refactored application form types use proper discriminated unions with
appTypeas the discriminator. This enables TypeScript to correctly narrow types and enforce constraints (e.g.,nameis required for inline apps). The structure is clean and type-safe.
103-108: LGTM! Type guard and volume form type added.The
isQuadletAppFormtype guard enables runtime discrimination of Quadlet applications. TheApplicationVolumeFormtype properly extendsApplicationVolumewith the union of possible volume provider fields, supporting the flexible volume configuration UI.
49-54: Based on my verification of the codebase, the review comment is accurate and well-founded.
appTypeis now required on all application forms.The verification confirms:
- All AppForm subtypes (ImageAppForm, ComposeAppForm, QuadletAppForm) explicitly specify appType values
- The error handling at line 397 throws "Application appType is required" if appType is missing
- New applications are always created with appType (defaulting to AppTypeQuadlet in ApplicationTemplates.tsx)
- The volumes field addition is properly handled with a fallback:
app.volumes || []All code paths that create or manipulate AppForm objects consistently provide the appType field. The type system enforces this requirement at compile time, and the runtime error check ensures data integrity if any legacy data lacks this field.
libs/ui-components/src/components/form/validations.ts (5)
71-87: LGTM! Comprehensive compose filename validation.The valid compose filename list covers standard docker-compose and podman-compose file naming conventions, including override files. The display string provides clear guidance for users.
327-396: LGTM! Well-structured validation schemas.The common inline app validation schemas are properly separated and reusable. The
uniqueFilePathsTestcorrectly identifies duplicate file paths and attaches the error to the first duplicate's path field for clear user feedback. The logic is complex but handles edge cases well.
398-425: LGTM! Compose filename validation correctly handles subdirectories.The validation properly extracts filenames from paths (supporting subdirectories) and validates against the allowed compose filename list. Error reporting is specific and user-friendly.
427-495: LGTM! Quadlet file type validation with clear error messages.The two-phase validation approach (check unsupported types first, then verify at least one supported type) provides specific, actionable error messages. The logic correctly handles edge cases like empty files or missing paths.
498-527: LGTM! Quadlet root-level validation correctly scoped.The validation wisely only enforces root-level placement for files with supported quadlet extensions (
.container,.volume, etc.), allowing other files to exist in subdirectories. This provides the right balance of constraint and flexibility.
celdrake
force-pushed
the
EDM-2290-quadlet-applications-support
branch
2 times, most recently
from
8ebe2b7 to
1d5d3da
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/ui-components/src/types/deviceSpec.ts (1)
101-103: Consider checking both specType and appType in isQuadletAppForm.The
isQuadletAppFormguard only checksappType === AppType.AppTypeQuadlet, but according to the type definitions, QuadletAppForm requiresspecType: AppSpecType.INLINE. For proper type narrowing and runtime safety, consider:-export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => app.appType === AppType.AppTypeQuadlet; +export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => + app.specType === AppSpecType.INLINE && app.appType === AppType.AppTypeQuadlet;This ensures the guard correctly narrows to QuadletAppForm and prevents false positives when an image-based app has AppTypeQuadlet.
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
214-228: Consider a more type-safe approach for conditional volume properties.The current implementation uses
@ts-expect-errorand type assertions to construct ApplicationVolume objects with conditional fields. While functional, consider using a more type-safe pattern:const volumes = app.volumes?.map((v) => { const volume: ApplicationVolume = { name: v.name, ...(v.image && { image: v.image }), ...(v.mount && { mount: v.mount }), } as ApplicationVolume; return volume; });This approach uses object spread with conditional properties, which is more idiomatic TypeScript and avoids type assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/i18n/locales/en/translation.json(4 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/form/validations.ts(3 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 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-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/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/ApplicationTemplates.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.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/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.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/form/validations.tslibs/ui-components/src/types/deviceSpec.tslibs/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/form/validations.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/ui-components/src/components/form/validations.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.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-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/deviceSpecUtils.ts
📚 Learning: 2025-11-20T07:33:34.656Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/utils/authProvider.ts:5-18
Timestamp: 2025-11-20T07:33:34.656Z
Learning: In the flightctl-ui repository, for AuthProvider objects, the metadata.name field is always present at runtime as it serves as the ID of the authprovider. The OpenAPI schema incorrectly defines it as optional (name?: string in ObjectMeta), but the type assertion `as string` is appropriate when accessing provider.metadata.name since it's guaranteed to exist.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-02-11T10:20:50.496Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 208
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx:30-44
Timestamp: 2025-02-11T10:20:50.496Z
Learning: In the FlightCtl UI, error handling for device decommissioning is implemented in the modal component created by `useDecommissionAction`. Failed PUT requests show an error in the modal with retry/close options.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 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
🧬 Code graph analysis (4)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/types/deviceSpec.ts (1)
AppForm(94-94)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)
libs/ui-components/src/components/form/validations.ts (1)
libs/ui-components/src/types/deviceSpec.ts (7)
InlineAppForm(62-62)ComposeAppForm(64-68)QuadletAppForm(71-75)AppForm(94-94)isImageAppForm(101-101)ImageAppForm(57-60)isQuadletAppForm(103-103)
libs/ui-components/src/types/deviceSpec.ts (1)
libs/types/index.ts (4)
AppType(19-19)ApplicationVolume(16-16)ImageVolumeSource(125-125)VolumeMount(189-189)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (4)
libs/types/models/ApplicationVolume.ts (1)
ApplicationVolume(8-13)libs/ui-components/src/types/deviceSpec.ts (3)
isImageAppForm(101-101)isImageAppProvider(98-99)InlineAppForm(62-62)libs/types/models/ApplicationProviderSpec.ts (1)
ApplicationProviderSpec(9-15)libs/types/models/InlineApplicationProviderSpec.ts (1)
InlineApplicationProviderSpec(7-12)
⏰ 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 (21)
libs/ui-components/src/types/deviceSpec.ts (4)
1-14: LGTM! Imports align with expanded volume and app type support.The new imports (AppType, ApplicationVolume, ImageVolumeSource, VolumeMount) are appropriately added to support the expanded volume model and multi-type application handling introduced in this PR.
49-55: Verify appType is required in the API schema.The
appTypefield is now mandatory inAppBase, but according to the past review discussion on deviceSpecUtils.ts (lines 397-420), the API type definition marksappTypeas optional. Ensure the API schema has been updated to makeappTyperequired before this change is merged.Based on learnings about planned API changes.
57-75: LGTM! Well-structured discriminated unions for app forms.The type definitions clearly distinguish between image-based apps (which can be Compose or Quadlet) and inline apps (ComposeAppForm or QuadletAppForm). The name field requirements are correctly enforced: required for inline applications, optional for image-based applications.
Based on learnings about name requirements in FlightCtl.
105-108: LGTM! Extended volume form supports new volume provider specs.The optional
imageandmountfields properly extend ApplicationVolumeForm to support the new ImageMountVolumeProviderSpec model introduced in this PR.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (5)
1-32: LGTM! Clean imports and i18n-friendly app format mapping.The new imports support the enhanced UI for app type selection, and the
appFormatOptionsfunction properly uses the translation function to provide localized labels for Quadlet and Compose applications.
38-76: LGTM! Reset logic correctly preserves appType during specType transitions.The unified reset mechanism properly handles the transition between image and inline definition sources while preserving the selected app type (Quadlet or Compose). The dependency array is correctly configured to trigger only when the app becomes incomplete.
85-92: LGTM! Application type field correctly binds to appType.The field properly uses the
appTypeproperty and the localized format options to allow users to select between Quadlet and Compose applications.
94-137: LGTM! Clear UI for definition source selection.The new Definition Source section provides helpful context about OCI reference vs Inline options, making it easier for users to understand when to use each approach.
230-284: LGTM! Applications list with helpful guidance and sensible defaults.The updated UI provides clear guidance about supported application formats. The default values for new applications (Quadlet + Inline) are reasonable starting points for users.
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (2)
1-17: LGTM! Imports support expanded volume model.The new imports (ApplicationVolume, ImageMountVolumeProviderSpec) are necessary for the enhanced volume handling in this file.
394-420: LGTM! Application values extraction properly handles appType.The function correctly throws an error when
appTypeis missing, aligning with the expectation that the API will make this field required. Both image and inline app branches properly extract and include theappTypein the form values.Based on past review discussion about appType becoming required in the API.
libs/ui-components/src/components/form/validations.ts (8)
1-32: LGTM! Imports support multi-type application validation.The new imports (AppType, ComposeAppForm, QuadletAppForm, isImageAppForm, isQuadletAppForm) are properly added to enable validation of different application types.
71-87: LGTM! Well-defined validation constants for Compose and Quadlet formats.The validation constants clearly define allowed file names for Compose applications and supported/unsupported extensions for Quadlet applications, providing good foundation for validation logic.
327-364: LGTM! Well-factored common validation schemas.The extracted common schemas (inlineAppNameSchema, inlineAppFilePathSchema, inlineAppFileSchema) are properly parameterized and reusable across different inline application types.
367-396: LGTM! Duplicate file path detection is well-implemented.The uniqueFilePathsTest correctly identifies duplicate paths and attaches the error to the first occurrence, providing clear feedback to users.
398-425: LGTM! Compose file name validation is correctly implemented.The validation properly extracts filenames from paths and checks them against the allowed compose file names, providing clear error messages with examples.
427-436: LGTM! Clean helper functions for Quadlet validation.The helper functions (getFileExtension, isAtRoot) are simple, focused, and correctly implemented for use in Quadlet-specific validations.
439-527: LGTM! Comprehensive Quadlet file validations.The Quadlet validations properly enforce both the supported file types and the root-level requirement, with clear error messages that guide users to correct their configuration.
529-616: LGTM! Comprehensive validation schema for all application types.The schema correctly handles the three application form types (image-based, inline Quadlet, inline Compose) using a discriminated union pattern with appropriate type-specific validations for each branch.
libs/i18n/locales/en/translation.json (2)
224-319: LGTM! Translation keys align with new app type support.The new translation keys properly support Quadlet and Compose application types, providing clear labels and descriptions for the UI. The keys follow the existing naming conventions.
Based on learnings about automated CI checks for translation coverage.
553-566: LGTM! Validation messages are clear and actionable.The new and updated validation messages provide helpful feedback to users, with appropriate parameterization for dynamic content (e.g., appType, allowedFileNames).
celdrake
force-pushed
the
EDM-2290-quadlet-applications-support
branch
from
1d5d3da to
e4b0ae1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
libs/types/models/DeviceApplicationStatus.ts (1)
7-26: App status extension withembeddedandappTypelooks consistentAdding
embedded: booleanandappType: AppTypetoDeviceApplicationStatusaligns with the new application model and gives the UI everything it needs to distinguish embedded/quadlet/compose apps. Just ensure the OpenAPI spec is updated in lockstep so regeneration doesn’t drop these fields.libs/ui-components/src/types/deviceSpec.ts (1)
49-75: Form discriminators are solid; tightenisQuadletAppFormto avoid unsound narrowingUsing
specType(inline vs image) plusappType(compose vs quadlet) to buildImageAppForm,ComposeAppForm, andQuadletAppFormmakes the form model easy to reason about and matches the “name required for inline, optional for image” rule.One thing worth tightening is the
isQuadletAppFormtype guard:export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => app.appType === AppType.AppTypeQuadlet;This will return
truefor any quadlet app, including image‑based ones, even though the predicate says the result is aQuadletAppForm(which hasfiles). That’s currently safe only because you call it after!isImageAppForm(value)in the validation schema.To keep the guard sound and future‑proof if it’s reused elsewhere, consider also checking
specType:-export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => app.appType === AppType.AppTypeQuadlet; +export const isQuadletAppForm = (app: AppBase): app is QuadletAppForm => + app.specType === AppSpecType.INLINE && app.appType === AppType.AppTypeQuadlet;
ApplicationVolumeForm = ApplicationVolume & { image?: ImageVolumeSource; mount?: VolumeMount }also looks fine as a UI‑friendly superset of the API type for editing before deciding the final provider.Also applies to: 94-108
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx (1)
3-14: Review UI now correctly composes “Image/Inline” with “Quadlet/Compose”Using
isImageAppFormplusgetAppFormatLabel(app.appType, t)gives clear labels like “Image based – Quadlet” vs “Inline – Compose”, and thenameresolution (!isImageApp || app.name→ fallback toUnnamed (image)) still respects the rule that inline apps must be named while image apps may not be.Only minor consideration: if a new
AppTypeis added later (e.g. single container),getAppFormatLabelwill currently label it as “Compose”. When that type lands, it’ll be worth revisiting this helper to avoid silently lumping new formats into the compose bucket.Also applies to: 25-33
libs/ui-components/src/components/form/validations.ts (2)
71-88: Refine duplicate-path validation to exclude empty paths at both call sitesThe
uniqueFilePathsTestfunction (line 367) usescountBy(files.map((file) => file.path))to detect duplicates. When files have undefined or empty paths, they contribute to the count, causing multiple incomplete rows to show both "File path is required" and "Each file … must use different paths" errors simultaneously.Filter falsy paths before the duplicate check to streamline the validation feedback:
- const duplicateFilePaths = Object.entries(countBy(files.map((file) => file.path))) + const duplicateFilePaths = Object.entries( + countBy(files.map((file) => file.path).filter((p): p is string => !!p)), + )This affects both inline app types:
- Line 562: QuadletAppForm files validation
- Line 575: ComposeAppForm files validation
327-338: Optional refactoring to tighten inline app validation is recommendedYour suggestion is valid. The branching logic correctly validates different app forms, but appSpecTypeSchema currently allows both INLINE and OCI_IMAGE for inline (compose/quadlet) apps. While invalid combinations will fail validation when required fields like
imageare missing, restricting appSpecTypeSchema to only INLINE in inline contexts would provide more explicit and defensive validation.Current behavior: An inline app with
specType = OCI_IMAGEfalls through to ImageAppForm validation, failing due to missingimagefield (implicit error).Suggested behavior: Restrict appSpecTypeSchema to INLINE for inline shapes, catching the mismatch immediately with a direct error message.
The type guard
isImageAppFormchecks onlyspecType === OCI_IMAGE, so it would catch a quadlet app with the wrong specType early, but creating separate schemas (inlineAppSpecTypeSchemarestricted to INLINE, keepingimageAppSpecTypeSchemarestricted to OCI_IMAGE) would make the validation logic more explicit and easier to maintain.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (3)
39-41: Consider preserving variables when resetting app on specType changes
setValuereplaces the wholeAppForm, and both reset branches currently hard‑resetvariablesto[]. If a user switches the definition source (INLINE ↔ OCI_IMAGE) after entering variables, those values will be silently dropped.If keeping variables across source switches is desirable, you can preserve them from the previous value:
if (app.specType === AppSpecType.INLINE) { // Switching to inline - need files setValue( { specType: AppSpecType.INLINE, appType: app.appType, name: app.name || '', files: [{ path: '', content: '' }], - variables: [], + variables: app.variables ?? [], } as AppForm, false, ); } else if (app.specType === AppSpecType.OCI_IMAGE) { // Switching to image - need image field setValue( { specType: AppSpecType.OCI_IMAGE, appType: app.appType, name: app.name || '', image: '', - variables: [], + variables: app.variables ?? [], } as AppForm, false, ); }If the current behavior (dropping variables on source switch) is intentional, this can be left as-is.
Also applies to: 45-45, 48-76
94-137: Use semantic list markup instead of<u>with<li>elementsThe helper text currently uses
<u>wrapping<li>elements without a<ul>/<ol>, which isn’t valid HTML and can confuse screen readers:<u> <li>...</li> <li>...</li> </u>Consider switching to a semantic list and using CSS for styling:
- <Stack hasGutter> + <Stack hasGutter> <StackItem> <strong>{t('Configuration Sources')}:</strong> </StackItem> <StackItem> - <u> - <li> - <strong>{t('OCI reference')}</strong> -{' '} - {t('Pull definitions from container registry (reusable, versioned).')} - </li> - <li> - <strong>{t('Inline')}</strong> -{' '} - {t('Define application files directly in this interface (custom, one-off).')} - </li> - </u> + <ul className="fctl-application-template__config-sources"> + <li> + <strong>{t('OCI reference')}</strong> -{' '} + {t('Pull definitions from container registry (reusable, versioned).')} + </li> + <li> + <strong>{t('Inline')}</strong> -{' '} + {t('Define application files directly in this interface (custom, one-off).')} + </li> + </ul> </StackItem> </Stack>This preserves the visual intent while improving semantics and accessibility.
230-285: Verify help text and default appType align with actual supported flowsTwo small points to double‑check in this block:
The intro text says “You can deploy single containers, Quadlet applications…”. If “single container applications” aren’t actually available yet in this wizard (and this PR is only adding Quadlet + existing inline/image flows), this may overpromise. It might be worth rephrasing to match what’s currently supported or guarding the wording behind the same feature rollout plan.
The “Add application” handler seeds new entries with
appType: AppType.AppTypeQuadletby default. If Compose applications are meant to be equally common, you may want to:
- default to a more neutral choice (e.g., first option, or last‑used type), or
- explicitly confirm with UX that Quadlet should indeed be the default here.
Neither is a blocker, but aligning these with the intended UX will avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/i18n/locales/en/translation.json(4 hunks)libs/types/models/ApplicationVolume.ts(1 hunks)libs/types/models/DeviceApplicationStatus.ts(2 hunks)libs/types/models/TokenResponse.ts(1 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(2 hunks)libs/ui-components/src/components/form/validations.ts(3 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/types/models/TokenResponse.ts
- libs/i18n/locales/en/translation.json
- libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
🧰 Additional context used
🧠 Learnings (10)
📓 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-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/types/deviceSpec.tslibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.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/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/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsxlibs/ui-components/src/components/form/validations.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/types/deviceSpec.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
📚 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.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/ui-components/src/components/form/validations.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-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
🧬 Code graph analysis (3)
libs/types/models/DeviceApplicationStatus.ts (1)
libs/types/index.ts (1)
AppType(19-19)
libs/ui-components/src/types/deviceSpec.ts (1)
libs/types/index.ts (4)
AppType(19-19)ApplicationVolume(16-16)ImageVolumeSource(125-125)VolumeMount(189-189)
libs/types/models/ApplicationVolume.ts (4)
libs/types/index.ts (4)
ApplicationVolume(16-16)ImageVolumeProviderSpec(124-124)MountVolumeProviderSpec(138-138)ImageMountVolumeProviderSpec(122-122)libs/types/models/ImageVolumeProviderSpec.ts (1)
ImageVolumeProviderSpec(6-8)libs/types/models/MountVolumeProviderSpec.ts (1)
MountVolumeProviderSpec(9-11)libs/types/models/ImageMountVolumeProviderSpec.ts (1)
ImageMountVolumeProviderSpec(10-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration-tests
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (2)
libs/types/models/ApplicationVolume.ts (1)
5-13: Volume union update andnameintersection look goodIntersecting
{ name: string }with the provider-spec union and addingImageMountVolumeProviderSpecis a clean way to ensure everyApplicationVolumeis named while supporting all three backing variants. This lines up with the provider spec definitions in related models.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
3-4: App type imports and option mapping to the select look goodThe AppType import,
appTypeOptionshelper, and bindingitems={appTypesOptions}toFormSelectonappTypeare consistent and keep the enum-to-label mapping centralized. No issues from my side here.Also applies to: 7-9, 20-20, 29-32, 85-92
celdrake
force-pushed
the
EDM-2290-quadlet-applications-support
branch
from
e4b0ae1 to
efee30c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/ui-components/src/components/form/validations.ts (2)
71-83: Compose inline validation is very strict; consider allowing helper files alongside main compose filesThe new compose helpers are nicely structured, but
composeFileNamecurrently enforces that every inline compose file’s basename is one of the known compose filenames fromvalidComposeFileNames. That means any additional helpers (for example.env, override snippets, or sidecar YAMLs) in the same inline app will be rejected.If your intent is “this inline app represents only the main compose file (+override) and nothing else,” this is fine. If you expect users to bundle extra files, you may want to relax this to:
- Require at least one file whose basename is in
validComposeFileNames, but- Allow other files with arbitrary names, or
- Scope the check only to YAML/YML files and ignore non‑YAML file extensions.
That keeps the main compose file guarded while still allowing realistic setups.
Also applies to: 354-365, 366-397, 398-425
85-88: Quadlet extension classification still conflicts with Podman spec; will reject valid quadlet units
supportedQuadletExtensions/unsupportedQuadletExtensionsand the quadlet tests are well wired to give clear, path-specific errors, but the extension lists are still misaligned with the actual Quadlet support:
.kubeand.buildare valid Quadlet unit types in current Podman releases.- Podman 5.2 also introduces
.artifactsupport.- Your code treats
.build,.artifact,.kubeas unsupported, so valid quadlet units will be blocked with “Unsupported quadlet file type” errors, and the “must include at least one supported type” test will never pass for those files.These lists should be adjusted so supported Quadlet unit types live in
supportedQuadletExtensions, and only truly invalid ones are rejected. Otherwise quadlet apps using kube/build/artifact units cannot be authored from the UI.What file extensions are officially supported for Podman Quadlet units (e.g., .container, .volume, .network, .build, .pod, .kube, .artifact), and which versions added .kube / .artifact / .build support?Also applies to: 439-495, 497-527
🧹 Nitpick comments (5)
libs/ui-components/src/types/deviceSpec.ts (1)
49-55: AppForm / appType modeling aligns with requirements; consider narrowing specType for inline variantsThe separation into
ImageAppFormvsInlineAppFormwithappTypeas a discriminant looks consistent with the new Quadlet/Compose flows, andnamebeing required only onComposeAppForm/QuadletAppFormpreserves the “inline must have a name, image apps may omit it” rule. TheApplicationVolumeFormextension also leaves room for future image/mount support without impacting current behavior.If you want slightly stronger typing, you could specialize
specTypeon the concrete forms, e.g.:
ImageAppForm→specType: AppSpecType.OCI_IMAGEComposeAppForm/QuadletAppForm→specType: AppSpecType.INLINEso the compiler can catch impossible combinations instead of relying solely on validation. Not urgent, but it would tighten the discriminated union around the same invariants your schema already enforces. Based on learnings, this keeps the inline-vs-image name rule explicit at the type level.
Also applies to: 57-75, 94-104, 105-108
libs/ui-components/src/components/DetailsPage/Tables/ApplicationsTable.tsx (1)
2-2: MapappTypeto localized display labels instead of showing raw valueRight now the Type column prints the raw
appDetails.appTypevalue in aLabel. Depending on the backend enum, this may surface implementation strings instead of the user-facing “Quadlet” / “Compose” wording you already have in translations.You could introduce a small mapper (similar to
appTypeOptionsin the wizard) and rendert('Quadlet')/t('Compose')(or a generic fallback) based on the enum, so the table stays localized and resilient to future enum name changes.Also applies to: 38-39, 43-48, 58-60
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (2)
39-41: Spec-type switch behavior is sound but aggressively resets app fields; consider preserving variables and warning on data lossThe
useEffect“incomplete app” handling is a good way to ensure image/inline apps always have the requiredimage/filesfields, and it keeps existing apps untouched. However, when users toggle the “Definition source”:
- Switching INLINE → IMAGE drops any existing
filesand resetsvariablesto[].- Switching IMAGE → INLINE drops
imageand also resetsvariablesto[].If users experiment with sources, they can easily lose nontrivial inline content or variable definitions without any warning.
You might want to either:
- Preserve
variables(and potentially reusename) when switching specType, or- Show a confirmation dialog similar to the repository-type switch flow before discarding data.
Also, the default added app is hard-coded to
AppType.AppTypeQuadlet+AppSpecType.INLINE. If compose or image-based apps are expected to be more common, it may be worth double-checking this default with the product UX.Also applies to: 45-77, 85-92, 151-152, 268-274
94-115: Use semantic list markup instead of<u>for the definition-source bulletsIn the “Configuration Sources” helper text,
<li>elements are wrapped in a<u>tag rather than a<ul>. That’s invalid HTML and may confuse assistive technologies.Switching the wrapper to
<ul>(and dropping the underline) will keep the visual structure while remaining semantically correct and more accessible.libs/i18n/locales/en/translation.json (1)
348-359: New application/quadlet translation keys match usage; minor wording tweak optionalThe added strings for Quadlet/Compose application types, “Definition source”, configuration source descriptions, and the new validation messages all align with how they’re used in the wizard and schemas (including placeholders like
{{ appType }}and{{ supportedTypes }}).One small English nit you might consider polishing:
"Definition source must be image for this type of applications"→ e.g."Definition source must be set to image for this application type"or"for this type of application".Purely cosmetic; the current text is understandable.
Also applies to: 366-369, 440-441, 675-688
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/i18n/locales/en/translation.json(4 hunks)libs/ui-components/src/components/DetailsPage/Tables/ApplicationsTable.tsx(4 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(5 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx(2 hunks)libs/ui-components/src/components/form/validations.ts(3 hunks)libs/ui-components/src/types/deviceSpec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
- libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewApplications.tsx
🧰 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.
📚 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/DetailsPage/Tables/ApplicationsTable.tsxlibs/ui-components/src/components/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/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/steps/ApplicationTemplates.tsxlibs/ui-components/src/types/deviceSpec.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/form/validations.tslibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/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/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/form/validations.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-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.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/types/deviceSpec.ts
📚 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
🧬 Code graph analysis (1)
libs/ui-components/src/types/deviceSpec.ts (1)
libs/types/index.ts (4)
AppType(19-19)ApplicationVolume(16-16)ImageVolumeSource(125-125)VolumeMount(189-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
2 participants
Adds basic support for Quadlet applications, both as Inline or Image-based applications.
Lays the foundation for adding also Single Container Applications.
quadlets.mp4
Summary by CodeRabbit
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.