Adding application type Quadlet support#375
Conversation
WalkthroughAdds support for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
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)
206-242: AddappTypeto image-based application API payload.The backend
ApplicationProviderSpectype supportsappTypefor all three application variants (image, artifact, and inline). However,toAPIApplicationonly includesappTypefor inline applications (line 231), omitting it for image applications (lines 220-226).This creates data loss during round-trip editing:
getApplicationValuesreadsappTypefrom the API for both image and inline apps, but only inline apps preserve it when sent back viatoAPIApplication.Add
appTypeto the image application payload at line 226:const data = { image: app.image, envVars, volumes, name: app.name, appType: app.appType || AppType.AppTypeCompose, }; return app.name ? data : { ...data, name: undefined };
🧹 Nitpick comments (2)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (2)
47-57: Refactor to use AppType enum instead of type-cast string literal.The type casting
('compose' as unknown as AppForm['appType'])is a code smell that bypasses TypeScript's type safety. SincedeviceSpecUtils.tsusesAppType.AppTypeComposeas the default value (line 231), this code should follow the same pattern for consistency and type safety.Apply this change to use the enum value:
+import { AppType } from '@flightctl/types'; + const ApplicationSection = ({ index, isReadOnly }: { index: number; isReadOnly?: boolean }) => { // ... React.useEffect(() => { if (isInlineIncomplete) { setValue( { specType: AppSpecType.INLINE, - appType: (app.appType || ('compose' as unknown as AppForm['appType'])), + appType: app.appType || AppType.AppTypeCompose, name: app.name || '', files: [{ path: '', content: '' }], variables: [], },
100-112: Consider using AppType enum values for improved maintainability.The FormSelect uses hardcoded string literals
'compose'and'quadlet'as keys (lines 104-105). While this may work if the enum values match these strings exactly, using the enum values directly would provide better type safety and maintainability, preventing potential mismatches during refactoring.Consider this approach using enum values:
+import { AppType } from '@flightctl/types'; + // ... {isInlineAppForm(app) && ( <FormGroup label={t('Application format')} isRequired> <FormSelect items={{ - compose: t('Compose'), - quadlet: t('Quadlet'), + [AppType.AppTypeCompose]: t('Compose'), + [AppType.AppTypeQuadlet]: t('Quadlet'), }} name={`${appFieldName}.appType`} placeholderText={t('Select a format')} isDisabled={isReadOnly} /> </FormGroup> )}This ensures compile-time checking and makes any enum value changes immediately visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(3 hunks)libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx(4 hunks)libs/ui-components/src/types/deviceSpec.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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.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/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/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-02-17T08:52:50.747Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 218
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx:109-111
Timestamp: 2025-02-17T08:52:50.747Z
Learning: When wrapping a Button component (like `deleteAction`) inside another Button in React/PatternFly, use `component="a"` on the outer Button to avoid accessibility issues caused by button nesting. Include `tabIndex={0}` to ensure proper keyboard navigation.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
📚 Learning: 2025-03-19T08:55:03.335Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceApplications.tsx:44-50
Timestamp: 2025-03-19T08:55:03.335Z
Learning: In FlightCtl, the name field is required for Inline applications but optional for Image-based applications. This requirement is enforced by validation logic even though TypeScript type definitions might not explicitly show this distinction.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
📚 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/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
📚 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
🧬 Code graph analysis (2)
libs/ui-components/src/types/deviceSpec.ts (1)
libs/types/index.ts (1)
AppType(15-15)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
libs/ui-components/src/types/deviceSpec.ts (2)
AppForm(86-86)isInlineAppForm(96-96)
🔇 Additional comments (6)
libs/ui-components/src/types/deviceSpec.ts (2)
2-2: LGTM - Clean import of AppType.The import follows the established pattern for generated types from the @flightctl/types package.
48-58: LGTM - AppType field properly integrated.The optional
appTypefield is correctly added toAppBase, making it available to bothImageAppFormandInlineAppForm. The optional nature ensures backward compatibility.libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
391-420: LGTM - AppType correctly propagated from API to form values.The
getApplicationValuesfunction correctly propagatesappTypefrom the API specifications to form values for both image-based (line 399) and inline (line 409) applications. This ensures the field is preserved when loading existing device specs for editing.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (3)
3-6: LGTM - Import optimization for tree-shaking.The change from aggregated imports to direct, module-specific imports from PatternFly components can improve tree-shaking and reduce bundle size.
58-70: LGTM - Proper state management and dependency tracking.The code correctly preserves
appTypewhen switching application types (line 62) and properly includes it in theuseEffectdependency array (line 70), ensuring the effect runs whenappTypechanges and preventing stale closures.
100-112: Well-implemented conditional UI feature.The application format selector is appropriately shown only for inline applications where the compose/quadlet distinction is relevant. The integration with the existing form structure and Formik is clean and follows established patterns.
celdrake
left a comment
celdrake
left a comment
There was a problem hiding this comment.
We'd be missing validations since for quadlet applications we only want to allow the user to set certain types (eg. .container, .volume), while attempting to save other types (eg. .kube) would get a 400 on the API side.
| setValue( | ||
| { | ||
| specType: AppSpecType.INLINE, | ||
| appType: (app.appType || ('compose' as unknown as AppForm['appType'])), |
There was a problem hiding this comment.
Seems like it would work without the casting
| appType: (app.appType || ('compose' as unknown as AppForm['appType'])), | |
| appType: app.appType, |
|
Full implementation here #381 |
2 participants
Inline applications now support Compose/Quadlet in the editor, appType is preserved from the API, and emitted on save.
Vibe coding
Summary by CodeRabbit