OpenAPI schema for applications changed#463
Conversation
WalkthroughSimplifies ApplicationProviderSpec into a union of concrete app types; adds generated application type models and new form variants; implements a typed API↔form conversion pipeline with granular change detection; updates Formik-driven components to derive state from fields; adjusts i18n keys and an ESLint import restriction. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as App Form UI
participant Formik as Formik State
participant Utils as deviceSpecUtils
participant API as Patch Builder
participant Server as Backend
UI->>Formik: user edits AppForm (Container/Helm/Quadlet/Compose)
Formik->>Utils: getApplicationPatches(basePath, currentApps, updatedAppForms)
Utils->>Utils: toFormApps -> toApiApplication -> toApiContainerApp|toApiHelmApp|toApiQuadletApp|toApiComposeApp
Utils->>Utils: run granular change detectors (env/volumes/files/ports/resources)
Utils->>API: produce PatchRequest
API->>Server: send PatchRequest
Server-->>API: 200 / 4xx
API-->>UI: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
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/steps/ApplicationHelmForm.tsx (1)
49-56:⚠️ Potential issue | 🟡 MinorPotential conflict between
nameandvalueprops on TextField.The Namespace field has both
name(for Formik binding) andvalue={app.namespace || ''}. This could cause the field to behave as a controlled component with a potentially stale value, conflicting with Formik's state management. Other refactored fields in this PR (likeApplicationImageForm) removed the explicitvalueprop to rely purely on Formik's wiring.🔧 Suggested fix: Remove explicit value prop
<FormGroupWithHelperText label={t('Namespace')} content={t('The namespace to install the Helm chart into.')}> <TextField aria-label={t('Namespace')} name={`${appFieldName}.namespace`} - value={app.namespace || ''} isDisabled={isReadOnly} placeholder={t('Type namespace here')} />
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts`:
- Around line 697-718: The CELIA-WIP TODO in createInitialAppForm leaves
specType uninitialized and can cause inconsistent defaults when AppType changes;
update createInitialAppForm to set a deterministic specType for each branch
(e.g., set app.specType = <appropriate default> inside the
AppType.AppTypeContainer, AppType.AppTypeHelm, AppType.AppTypeQuadlet, and
AppType.AppTypeCompose cases) so the returned AppForm always has a valid
specType, or if you cannot decide on defaults right now, replace the WIP comment
with a clear TODO referencing a ticket/ID and add code to explicitly set
app.specType = null or a sentinel to make the behavior explicit. Ensure you
modify createInitialAppForm and the branches that call toContainerAppForm,
toHelmAppForm, and createQuadletOrComposeAppForm so specType is consistently
initialized.
🧹 Nitpick comments (2)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVariablesForm.tsx (1)
21-22: Consider improving type safety for error handling.The
@ts-expect-errorcomment indicates a typing limitation with Formik's error structure. While this works, it could be improved by using a more explicit type cast or by defining a custom error type interface.♻️ Optional: Define explicit error type
+type AppVarsError = string | { variables?: string }; + const ApplicationVariablesForm = ({ appFieldName, isReadOnly }: ApplicationVariablesFormProps) => { const { t } = useTranslation(); - const [{ value: variables = [] }, { error }] = useField<VariablesForm>(`${appFieldName}.variables`); + const [{ value: variables = [] }, { error }] = useField<VariablesForm>(`${appFieldName}.variables`); + const typedError = error as AppVarsError | undefined; - // `@ts-expect-error` Formik error object includes "variables" - const appVarsError = typeof error?.variables === 'string' ? (error.variables as string) : undefined; + const appVarsError = typeof typedError === 'object' && typeof typedError?.variables === 'string' + ? typedError.variables + : undefined;libs/ui-components/src/components/form/validations.ts (1)
656-773: Validation schema refactored to discriminated approach - verify default case.The new discriminated validation using
appTypeandspecTypeis cleaner than the previous shape-based checks. However, the default case at line 763 assumes any remaining form is a Compose inline application. If a new application type is added in the future, it would silently use Compose validation.Consider adding an explicit check or throwing an error for unknown types to fail fast during development.
♻️ Optional: Make default case explicit
// Inline compose applications + if (value.appType === AppType.AppTypeCompose && value.specType === AppSpecType.INLINE) { return Yup.object<ComposeAppForm>().shape({ specType: appSpecTypeSchema(t), appType: Yup.string().oneOf([AppType.AppTypeCompose]).required(t('Application type is required')), name: validApplicationAndVolumeName(t).required(t('Name is required for compose applications.')), files: inlineAppFileSchema(t) .test('unique-file-paths', uniqueFilePathsTest(t)) .test('compose-file-name', composeFileName(t)), volumes: composeQuadletVolumesSchema(t), variables: appVariablesSchema(t), }); + } + + // Fallback - should not be reached with current types + return Yup.object().shape({ + appType: Yup.string().required(t('Unknown application type')), + });
celdrake
force-pushed
the
openapi-app-schema-changed
branch
from
3341d6c to
f352bea
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts`:
- Around line 711-731: createInitialAppForm returns a quadlet form with the
wrong appType because toQuadletAppForm constructs/returns a form with
AppType.AppTypeCompose; fix toQuadletAppForm so the returned AppForm has appType
set to AppType.AppTypeQuadlet (and ensure any defaults specific to quadlets are
applied), then leave createInitialAppForm unchanged; verify by calling
createInitialAppForm(AppType.AppTypeQuadlet) returns an AppForm whose appType
=== AppType.AppTypeQuadlet.
- Around line 706-709: toQuadletAppForm currently calls toComposeAppForm(app)
which sets appType to AppType.AppTypeCompose and the spread leaves that value in
place; change toQuadletAppForm (the function) to explicitly set appType to
AppType.AppTypeQuadlet in the returned object while preserving other fields and
the runAs default (i.e., return { ...baseApp, appType: AppType.AppTypeQuadlet,
runAs: app?.runAs || RUN_AS_DEFAULT_USER }); ensure AppType is in scope/imports
if needed.
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx`:
- Around line 51-64: The reset misbehavior is caused by toQuadletAppForm
returning a Compose form instead of a Quadlet form; fix toQuadletAppForm so it
constructs and returns a form with appType set to AppType.AppTypeQuadlet (and
correct Quadlet fields), ensure createInitialAppForm delegates to the corrected
toQuadletAppForm for AppType.AppTypeQuadlet, and keep the useEffect logic
(shouldResetApp, createInitialAppForm, setValue) as-is so switching to Quadlet
produces the proper initial Quadlet form.
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVariablesForm.tsx`:
- Around line 29-32: The current mapping in variables.map uses the array index
as the React key (FormSection key={variableIndex}), which causes input
reconciliation bugs when items are removed; update the VariablesForm type to
include a stable id field (e.g., id: string), ensure existing variables are
normalized to have ids when loaded, and change the render to use that id as the
key (replace key={variableIndex} with key={variable.id}). Also update the logic
that pushes new variables (where new variable objects are created) to generate a
stable id for each new entry using crypto.randomUUID() (or uuid if you add the
dependency) so newly added rows have unique persistent keys. Ensure any
references that build variableFieldName (const variableFieldName =
`${appFieldName}.variables[${variableIndex}]`) continue to compute form paths
correctly by using the index for field names but use the stable id only for the
React key.
🧹 Nitpick comments (5)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVariablesForm.tsx (1)
36-49: Consider adding consistent grid column spans for visual alignment.The Name and Value fields are in a Grid without explicit column sizing. For consistent form layout across the application, consider specifying column spans.
♻️ Optional: Add explicit grid columns
<Grid hasGutter> - <FormGroup label={t('Name')} isRequired> + <GridItem span={6}> + <FormGroup label={t('Name')} isRequired> <TextField name={`${variableFieldName}.name`} aria-label={t('Variable name')} isDisabled={isReadOnly} /> - </FormGroup> - <FormGroup label={t('Value')} isRequired> + </FormGroup> + </GridItem> + <GridItem span={6}> + <FormGroup label={t('Value')} isRequired> <TextField name={`${variableFieldName}.value`} aria-label={t('Variable value')} isDisabled={isReadOnly} /> - </FormGroup> + </FormGroup> + </GridItem> </Grid>libs/ui-components/src/types/deviceSpec.ts (1)
125-129: Consider logging or handling the empty identifier case more explicitly.The
getAppIdentifierfunction returns an empty string when bothnameandimageare missing. While this may be intentional for new/unsaved applications, callers might benefit from understanding this case.If this is expected behavior for new applications, a brief inline comment would improve clarity:
export const getAppIdentifier = (app: AppForm | ApplicationProviderSpec): string => { if (app.name) return app.name; if ('image' in app && app.image) return app.image; + // Returns empty string for new/unsaved applications without name or image return ''; };libs/ui-components/src/components/form/validations.ts (1)
763-773: Consider explicit appType check in fallback branch for defensive validation.The fallback branch validates as
ComposeAppFormwithout explicitly checkingappType === AppType.AppTypeCompose. While currently correct (all other types are handled above), this could silently accept unknown app types in the future.♻️ Suggested defensive check
// Inline compose applications + if (value.appType !== AppType.AppTypeCompose) { + throw new Error(`Unknown application type: ${value.appType}`); + } return Yup.object<ComposeAppForm>().shape({libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
296-297: JSON.stringify comparison may produce false positives on object key reordering.
haveHelmValuesChangedusesJSON.stringifywhich is sensitive to key order. If the same values are reordered (e.g., from YAML parsing), this could flag unchanged values as changed. Consider using a deep equality check if this becomes an issue.libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx (1)
148-148: Dead code:isContaineris always false in this context.At line 148, execution is within the
elsebranch (neither Container nor Helm), soisContaineris alwaysfalse. The condition(isQuadlet || isContainer)simplifies to justisQuadlet.♻️ Suggested simplification
- {(isQuadlet || isContainer) && <ApplicationIntegritySettings index={index} isReadOnly={isReadOnly} />} + {isQuadlet && <ApplicationIntegritySettings index={index} isReadOnly={isReadOnly} />}
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
Show resolved
Hide resolved
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
Show resolved
Hide resolved
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVariablesForm.tsx
Show resolved
Hide resolved
celdrake
force-pushed
the
openapi-app-schema-changed
branch
2 times, most recently
from
c09bca2 to
5dd6c95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/form/validations.ts (1)
661-668:⚠️ Potential issue | 🟠 MajorContainer app name now required — verify image-app rule.
Container apps are image-based, but this schema now requires
name. If image-based apps should allow omitted names, drop the required constraint to avoid blocking valid input.Suggested change
- name: validApplicationAndVolumeName(t).required(t('Name is required for single container applications.')), + name: validApplicationAndVolumeName(t),Based on learnings: In the FlightCtl UI project,
nameis required for inline applications but optional for image applications.
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts`:
- Around line 426-448: The YAML parsing in toApiHelmApp currently treats any
parsed result as a mapping and omits values silently for non-object YAML; update
toApiHelmApp to only set helmApp.values when yaml.load(app.valuesYaml) returns a
plain object (i.e., typeof === 'object', not null, and not an Array) and has
Object.keys(...).length > 0, and catch parse errors as before. Also update the
Yup 'valid-yaml' custom test used in the form validation to parse the YAML and
explicitly return false (validation fail) if the parsed value is not a mapping
object (same plain-object check) so the form prevents non-mapping YAML from
being submitted.
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx`:
- Around line 127-132: The array push in ApplicationVolumeForm is always adding
an entry with mountPath, which can produce invalid payloads for
non-single-container apps; update the logic in ApplicationVolumeForm (the push
call that creates new volume entries) to only include mountPath (and allow
adding volumes at all) when isSingleContainerApp is true — otherwise either
disable the add action or push a minimal object without mountPath (or prevent
push entirely) so non-single-container apps do not get mountPath/volume fields
that break OpenAPI validation.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
285-294: JSON.stringify is order-sensitive for Helm values.
If YAML keys are reordered, you’ll detect a change even when semantics are identical. Consider a stable deep-compare to avoid unnecessary patches.
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
Show resolved
Hide resolved
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVolumeForm.tsx
Show resolved
Hide resolved
celdrake
force-pushed
the
openapi-app-schema-changed
branch
from
5701ab4 to
cd38750
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/ui-components/src/components/form/validations.ts (1)
446-496:⚠️ Potential issue | 🟡 MinorAvoid extension mis-detection when directories contain dots.
quadletFileTypesValidationcallsgetFileExtension(file.path)on the full path. If a folder contains a dot (e.g.,dir.v1/app.container), the extension becomes malformed and the supported/unsupported checks can fail. Consider extracting the basename before the extension.🔧 Suggested fix (use basename before extracting extension)
-const getFileExtension = (path: string): string => { - const lastDot = path.lastIndexOf('.'); - return lastDot !== -1 ? path.substring(lastDot) : ''; -}; +const getFileExtension = (path: string): string => { + const fileName = path.split('/').pop() || path; + const lastDot = fileName.lastIndexOf('.'); + return lastDot !== -1 ? fileName.substring(lastDot) : ''; +};libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx (1)
50-56:⚠️ Potential issue | 🟡 MinorRemove the explicit
valueprop from TextField to prevent controlled/uncontrolled conflict.TextField uses Formik's
useFieldhook internally and spreads field props before component props. When you pass an explicitvalue={app.namespace || ''}, it overrides Formik's managed value. This causes the displayed value to diverge from Formik's state when the user types, creating an inconsistency where the form state and UI don't match.♻️ Suggested fix
<TextField aria-label={t('Namespace')} name={`${appFieldName}.namespace`} - value={app.namespace || ''} isDisabled={isReadOnly} placeholder={t('Type namespace here')} />
🤖 Fix all issues with AI agents
In `@libs/types/models/ApplicationUser.ts`:
- Around line 6-9: The ApplicationUser.runAs property documentation is out of
date: update the OpenAPI schema description (not the generated libs/types/models
files) for the ApplicationUser model so the runAs default text states the
default user is 'flightctl' (matching RUN_AS_DEFAULT_USER in flightctl-ui)
instead of the agent user/root; modify the runAs description in the openapi.yaml
schema (the ApplicationUser -> properties -> runAs description) to reflect this
change and regenerate the types so ApplicationUser.runAs comment will be
correct.
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx`:
- Around line 37-38: The duplicate-port detection is wrong because
isDuplicatePortMapping in validations.ts compares the new mapping against itself
(or the same array index), making every check true when ports is non-empty;
update isDuplicatePortMapping to loop/filter ports and compare each existing
mapping's hostPort and containerPort to the new host port (port) and
containerPort but skip the same entry (by index or by strict identity) so only
other entries are considered; then use that corrected function from
ApplicationContainerForm (and the other call sites around the other occurrence)
so adding or editing a different port mapping no longer reports a false
duplicate.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationVariablesForm.tsx (1)
21-22: Consider improving the error typing.The
@ts-expect-errorsuppression works but indicates a type mismatch. Consider defining a proper error type or using type assertion more explicitly.♻️ Alternative approach
- // `@ts-expect-error` Formik error object includes "variables" - const appVarsError = typeof error?.variables === 'string' ? (error.variables as string) : undefined; + const appVarsError = typeof (error as { variables?: string } | undefined)?.variables === 'string' + ? (error as { variables: string }).variables + : undefined;
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationContainerForm.tsx
Show resolved
Hide resolved
* OpenAPI schema for applications changed (cherry picked from commit bcc78b1)
2 participants
OpenAPI schema for application changed to a more structured pattern.
Before:
After:
Summary by CodeRabbit
New Features
Refactor